Skip to content

Conversation

@bprize15
Copy link

Describe changes proposed in this pull request:

  • Added a tab showcasing the OncoKB Patient Report in the Patient View page.

Checks

  • Has tests or has a separate issue that describes the types of test that should be created. If no test is included it should explicitly be mentioned in the PR why there is no test.
  • The commit log is comprehensible. It follows 7 rules of great commit messages. For most PRs a single commit should suffice, in some cases multiple topical commits can be useful. During review it is ok to see tiny commits (e.g. Fix reviewer comments), but right before the code gets merged to master or rc branch, any such commits should be squashed since they are useless to the other developers. Definitely avoid merge commits, use rebase instead.
  • Is this PR adding logic based on one or more clinical attributes? If yes, please make sure validation for this attribute is also present in the data validation / data loading layers (in backend repo) and documented in File-Formats Clinical data section!

Any screenshots or GIFs?

If this is a new visual feature please add a before/after screenshot or gif
here with e.g. Giphy CAPTURE or Peek

Notify reviewers

Read our Pull request merging
policy
. It can help to figure out who worked on the
file before you. Please use git blame <filename> to determine that
and notify them either through slack or by assigning them as a reviewer on the PR

@bprize15 bprize15 requested review from alisman and inodb October 23, 2025 14:38
const clinicalDataGroupedBySample =
patientViewPageStore.clinicalDataGroupedBySample.result;

const patientMetadata = new PatientMetadata();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets put this code in a utitility file somewhere and add some comments about what it does.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mean the whole contents of the this useEffect hook. be careful though to dereference data from the mobx promises before you pass them into this function

const IFRAME_NAME = 'patient-report-iframe';

const PatientReportTab = observer(
({ patientViewPageStore }: IPatientReportTabProps) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should type this using React.FunctionComponent generic

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we want to do this since React does not recommend. It's a rather trivial change so I'm find with it if it helps maintain the style of the codebase, but wondering your thoughts on this: facebook/create-react-app#8177

@bprize15 bprize15 requested a review from alisman November 19, 2025 14:54

let hugoSymbol = '';
let entrezGeneId = 0;
if ('gene' in alt) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i believe that typescript will not check this. i.e. you could put 'banana' in alt. better of just doing
alt.gene

@@ -0,0 +1,296 @@
import {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bprize15 this file could use some summary comments and testing. Claude would do very well helping to build add a few tests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added doc comment and test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants