Move OncoKB card and icons into separate package oncokb-frontend-commons #4404

scarrero4660 · 2022-10-31T13:28:25Z

alisman · 2022-10-31T16:24:41Z

packages/oncokb-frontend-commons/src/components/OncoKB.tsx

    }

-    private multiAnnotationIcon() {
+    function multiAnnotationIcon() {


we can clean up the body of this function by moving these little functions outside (i.e. above ln 117). Also, apply React.useCallback hook to memoize them will give us an optimization similar to what we got with mobx computed. This is probably negligible in this case but still a good practice to adopt in our functional components.

alisman · 2022-10-31T16:32:04Z

packages/oncokb-frontend-commons/src/components/OncoKbCard.tsx

+
+    const oncokbLogo = (
+        <img
+            src={oncoKbLogoImgSrc}


i would be a little suprrised if this worked because how would it resolve the image path at runtime. usually we use the webpack image loader. where you set src to require("pathToImage"). for small images, webpack will inline the src of the image. let me know if i'm wrong and it actually works now.

@alisman I think rollup is doing a similar thing with this plugin:

cbioportal-frontend/packages/config/rollup.config.js

Lines 42 to 46 in 7578f43

plugins: [

postcssUrl({

url: 'inline',

}),

],

This may not be the best solution for larger images because it inlines everything regardless of its size. But ideally, we shouldn't have larger images in our packages.

This does currently work but I can change it if you prefer

if it works it works! you can just confirm that it inlines the image? like, in the browser if you inspect the image, you should see a big long data uri

package-lock.json

packages/oncokb-frontend-commons/package.json

onursumer · 2022-10-31T17:16:43Z

packages/oncokb-frontend-commons/src/components/OncoKbCard.tsx

+
+    const oncokbLogo = (
+        <img
+            src={oncoKbLogoImgSrc}


@alisman I think rollup is doing a similar thing with this plugin:

cbioportal-frontend/packages/config/rollup.config.js

Lines 42 to 46 in 7578f43

plugins: [

postcssUrl({

url: 'inline',

}),

],

This may not be the best solution for larger images because it inlines everything regardless of its size. But ideally, we shouldn't have larger images in our packages.

packages/oncokb-frontend-commons/src/components/OncoKbSummaryTable.tsx

packages/oncokb-frontend-commons/src/components/OncoKbTooltip.tsx

onursumer · 2022-10-31T17:44:46Z

packages/oncokb-frontend-commons/src/components/StatusHelpers.tsx

@@ -0,0 +1,34 @@
+import { DefaultTooltip } from 'cbioportal-frontend-commons';


This is the duplicate of the one in react-mutation-mapper. We should probably move StatusHelpers.tsx into cbioportal-frontend-commons to be able to reuse it in both packages.

@zhx828 @scarrero4660 @alisman Note that DefaultTooltip component has a mobx dependency, so I think when we import components from cbioportal-frontend-commons we end up with mobx dependency.

@onursumer yeah, seems like the biggest concern of this PR. Particularly two imports, DefaultTooltip and ICache/SimpleCache
@scarrero4660 I saw you moved SimpleCache from RMM to oncokb commons, I don't think this is correct.

onursumer · 2022-10-31T17:46:32Z

packages/oncokb-frontend-commons/src/components/annotation.module.scss

@@ -0,0 +1,23 @@
+.annotation-item-load {


This one is also duplicate, but maybe it's okay because it is relatively small?

I'm fine with it.

onursumer

circleci build is failing due to a missing oncokb-frontend-commons dependency for the main package.json as well as react-mutation-mapper. We should add oncokb-frontend-commons in the corresponding package.json files as a dependency.

"dependencies": {
    ...
    "oncokb-frontend-commons": "^1.0.0",
    ...
}

onursumer · 2022-11-01T17:25:12Z

packages/cbioportal-frontend-commons/src/index.tsx

@@ -60,3 +60,4 @@ export { default as TruncatedText } from './components/truncatedText/TruncatedTe
 export * from './lib/hashString';
 export * from './lib/getColor';
 export * from './components/appContext/AppContext';
+export { errorIcon, loaderIcon } from './components/StatusHelpers';


Let's just export everything similar to all other exports in this file?

Suggested change

export { errorIcon, loaderIcon } from './components/StatusHelpers';

export * from './components/StatusHelpers';

packages/oncokb-frontend-commons/package.json

onursumer

Unit tests are failing. We need to add a jest.config.ts file. We can copy this one.

packages/oncokb-frontend-commons/package.json

alisman · 2022-11-02T21:14:49Z

packages/oncokb-frontend-commons/src/components/OncoKB.tsx

+                                : undefined
+                        }
+                        showFeedback={showFeedback}
+                        handleFeedbackClose={() => handleFeedbackClose}


don't use ananyous function wrapper b/c it always be new.

alisman · 2022-11-02T21:19:38Z

packages/oncokb-frontend-commons/src/components/OncoKB.tsx

+        false
+    );
+
+    const handleFeedbackOpen = React.useMemo(() => {


this should be useCallback

packages/oncokb-frontend-commons/src/lib/StringUtils.ts

packages/react-mutation-mapper/src/component/column/MyVariantInfoHelper.tsx

packages/oncokb-frontend-commons/src/components/icon/LevelIcon.tsx

onursumer

Found some additional issues while manually testing:

OncoKB track tooltip crashes the entire page. To reproduce go to this example enable OncoKB track via Add annotation tracks dropdown menu on top left. Mouseover any annotation circle on the OncoKB track.
OncoKB card doesn't properly re-render, the loader icon keeps spinning.

packages/cbioportal-frontend-commons/src/lib/StringUtils.ts

zhx828 · 2022-11-17T14:36:44Z

packages/oncokb-frontend-commons/src/components/ReferenceList.tsx

+        const ret: any = [];
+        for (const pmidStr of Object.keys(pubMedRecords)) {
+            ret.push({
+                data: [pubMedRecords[pmidStr]],


We do not think it needs to be an array

zhx828 · 2022-11-17T14:38:04Z

packages/oncokb-frontend-commons/src/components/ReferenceList.tsx

@@ -54,24 +44,39 @@ export const ReferenceList: React.FunctionComponent<ReferenceListProps> = (
                });
        });

-        return pubMedRecords[query];
+        const ret: any = [];


Can we just return pubMedRecords?

right, we should avoid any wherever possible.

zhx828 · 2022-11-17T14:51:14Z

packages/oncokb-frontend-commons/src/components/OncoKbSummaryTable.tsx

@@ -87,7 +87,7 @@ export const OncoKbSummaryTable: React.FunctionComponent<OncoKbSummaryTableProps
                Header: 'Level',
                sortable: false,
                Cell: (props: { original: OncoKbSummary }) => (
-                    <React.Fragment>


Do we need the extra div?

Based on the type signature of Cell, I believe we do. I reverted in back to React.Fragment since I had only changed it to fix an error which is no longer relevant

onursumer · 2022-11-17T17:07:03Z

packages/oncokb-frontend-commons/src/components/ReferenceList.tsx

@@ -54,24 +44,39 @@ export const ReferenceList: React.FunctionComponent<ReferenceListProps> = (
                });
        });

-        return pubMedRecords[query];
+        const ret: any = [];


right, we should avoid any wherever possible.

onursumer · 2022-11-17T17:08:21Z

packages/oncokb-frontend-commons/src/components/ReferenceList.tsx

-                const cacheData = getPmidData(uid.toString())
-                    .then(value => (pmidData[uid.toString()] = value))
-                    .catch(err => console.log(err));
+            const cacheData = getPmidData(props.pmids)


the name cacheData is probably misleading because we don't use cache anymore

zhx828 · 2022-11-18T22:08:44Z

packages/oncokb-frontend-commons/src/components/OncoKB.tsx

@@ -0,0 +1,270 @@
+import { MobxCache, OncoKbCardDataType } from 'cbioportal-utils';


What's the difference now between MobxCache and in library Cache?

I think MobxCache is an unused import now. We should try cleaning up all the other unused imports as well.

zhx828 · 2022-11-18T22:21:17Z

@onursumer @scarrero4660 I see 429 Too Many Requests when hover over tooltip quickly from one to another. Let's regroup to address this.

onursumer · 2022-11-18T22:29:34Z

@onursumer @scarrero4660 I see 429 Too Many Requests when hover over tooltip quickly from one to another. Let's regroup to address this.

@zhx828 Seems like we are sending duplicate requests for the same tool tip. There should be only one API request per hover over.

zhx828 · 2022-11-18T22:33:53Z

packages/cbioportal-frontend-commons/src/components/StatusHelpers.tsx

@@ -0,0 +1,34 @@
+import DefaultTooltip from './defaultTooltip/DefaultTooltip';


Do we need this file if oncokb-frontend-commons already have? @onursumer

Ideally we should only have one copy of this. As long as we don't introduce a circular dependency I think it's okay to import it from oncokb-frontend-commons.

zhx828 · 2022-11-18T22:47:48Z

packages/oncokb-frontend-commons/src/components/OncoKbCardLevelsOfEvidenceDropdown.tsx

+    function handleLevelCollapse() {
+        updateLevelCollapse(!levelsCollapsed);
+    }


Why do we need a wrapper?

My apologies. I think I missed this comment but it is necessary because it is passed into onClick and would produce a type error without it. (Type 'void' is not assignable to type 'MouseEventHandler | undefined'.)

We can do onClick={() => updateLevelCollapse(!levelsCollapsed)}

onursumer

A few more remaining issues but almost there!

Some unit tests are failing in the oncokb-frontend-commons package
We need to rebase the PR and fix the conflicts

onursumer · 2022-12-05T16:07:23Z

packages/oncokb-frontend-commons/src/components/Cache.tsx

+export interface ICacheData<T = any> {
+    status: 'pending' | 'complete' | 'error';
+    data?: T;
+}
+
+export interface ICache<T = any> {
+    [queryId: string]: ICacheData<T>;
+}


Do we use these types anywhere anymore? We should remove this file if not used anymore.

zhx828 · 2022-12-08T19:23:34Z

@scarrero4660 I still see two API calls if I hover over the blue icon https://www.cbioportal.org/results/mutations?cancer_study_list=msk_impact_2017&tab_index=tab_visualize&case_set_id=msk_impact_2017_all&Action=Submit&gene_list=CDKN2A%250ACDKN2B

onursumer · 2022-12-08T19:40:39Z

@scarrero4660 I still see two API calls if I hover over the blue icon https://www.cbioportal.org/results/mutations?cancer_study_list=msk_impact_2017&tab_index=tab_visualize&case_set_id=msk_impact_2017_all&Action=Submit&gene_list=CDKN2A%250ACDKN2B

Hmm, yeah, I see it, too. https://deploy-preview-4404--cbioportalfrontend.netlify.app/results/mutations?cancer_study_list=msk_impact_2017&tab_index=tab_visualize&case_set_id=msk_impact_2017_all&Action=Submit&gene_list=CDKN2A%250ACDKN2B

@scarrero4660 I thought this was fixed, but maybe something went wrong during the rebase?

zhx828

Loading tab content, especially loading the PubMed API sometimes takes few seconds, should we add a loading gif? @onursumer I remember there was one, but maybe removed later on?

zhx828 · 2022-12-08T20:34:30Z

packages/oncokb-frontend-commons/package.json

+    "build": "yarn run tcm && cross-env NODE_ENV=production NODE_OPTIONS=--max-old-space-size=2048 yarn run rollup",
+    "start": "yarn run watch",
+    "watch": "concurrently \"yarn run tcm:watch\" \"yarn run rollup:watch\"",
+    "watchSSL": "yarn run watch",


@onursumer @scarrero4660 does not seem to be needed?

I think we still need watchSSL, this is required by the main startSSL

zhx828 · 2022-12-08T20:35:18Z

packages/oncokb-frontend-commons/package.json

+    "rollup:watch": "yarn run rollup --watch",
+    "tcm": "tcm -p src/**/*.module.scss",
+    "tcm:watch": "yarn run tcm --watch",
+    "prepare": "yarn run build",


@onursumer I assume we copied from the other repo, but do we need this?

This might be needed by publish bot, so safer to keep it this way.

zhx828 · 2022-12-08T20:41:55Z

packages/oncokb-frontend-commons/src/components/OncoKB.tsx

+export function sortValue(
+    indicator?: IndicatorQueryResp | undefined | null
+): number[] {
+    const values: number[] = [0, 0, 0];


@onursumer @scarrero4660 why we need to initiate the list? If we have to, I guess for sorting, should it be five elements?

I can't remember why we needed to initiate this one. Maybe we had 3 values originally, but now it's probably outdated. We can try removing the initial list and see if it breaks anything.

I'm not sure why it is initiated but I can change this. This was copied over from the original component

zhx828 · 2022-12-08T20:48:30Z

packages/oncokb-frontend-commons/src/components/OncoKB.tsx

+    const [
+        tooltipDataLoadComplete,
+        setTooltipDataLoadComplete,
+    ] = React.useState(false);


I don't see this used anywhere? Was it used for a loading gif?

I'm not sure what the purpose of this is. I also asked Onur about it a while ago

Looks like not used anymore. This might be used originally for the loading gif but I am not sure. We need to go over the change history to see why and when this became unused.

zhx828 · 2022-12-08T20:54:06Z

packages/oncokb-frontend-commons/src/components/OncoKbCard.tsx

+                props.indicator &&
+                props.indicator.variantExist


I actually don't think this is a valid rule, can we change to props.indicator?.query?.alteration

Yes I updated this. Thanks

zhx828 · 2022-12-08T22:38:06Z

packages/oncokb-frontend-commons/src/components/ReferenceList.tsx

+    async function getPmidData(pmids: number[]) {
+        const pubMedRecords = await new Promise<any>((resolve, reject) => {
+            request
+                .post(
+                    'https://eutils.ncbi.nlm.nih.gov/entrez/eutils/esummary.fcgi?db=pubmed&retmode=json'
+                )
+                .type('form')
+                .send({ id: props.pmids.join(',') })
+                .end((err, res) => {
+                    if (!err && res.ok) {
+                        const response = JSON.parse(res.text);
+                        const result = response.result;
+                        const uids = result.uids;
+                        const ret: { [uid: string]: any } = {};
+                        for (let uid of uids) {
+                            ret[uid] = result[uid];
+                        }
+                        resolve(ret);
+                    } else {
+                        reject(err);
+                    }
+                });
+        });
+
+        return pubMedRecords;
+    }
+
+    let pmidPending = false;
+
+    function isLoading() {


Should we move functions out of functional component? @onursumer what's the standard?

I think in general it's better to have the functions out of the component if possible.

zhx828 · 2022-12-08T22:43:41Z

packages/oncokb-frontend-commons/src/components/OncoKbCardLevelsOfEvidenceDropdown.tsx

+    function handleLevelCollapse() {
+        updateLevelCollapse(!levelsCollapsed);
+    }


We can do onClick={() => updateLevelCollapse(!levelsCollapsed)}

zhx828 · 2022-12-08T22:48:54Z

packages/oncokb-frontend-commons/src/components/OncoKbTooltip.tsx

+import { OncoKbCardDataType } from 'cbioportal-utils';
+import { IndicatorQueryResp } from 'oncokb-ts-api-client';
+import * as React from 'react';
+import _ from 'lodash';


zhx828 · 2022-12-08T22:49:57Z

packages/oncokb-frontend-commons/src/components/ReferenceList.tsx

+                    'https://eutils.ncbi.nlm.nih.gov/entrez/eutils/esummary.fcgi?db=pubmed&retmode=json'
+                )
+                .type('form')
+                .send({ id: props.pmids.join(',') })


I think you should use the parameter of the method instead?

Fixed this. This has been copied from another file. Thanks for pointing that out

zhx828 · 2022-12-08T22:55:06Z

packages/oncokb-frontend-commons/src/components/ReferenceList.tsx

+                        const response = JSON.parse(res.text);
+                        const result = response.result;
+                        const uids = result.uids;
+                        const ret: { [uid: string]: any } = {};
+                        for (let uid of uids) {
+                            ret[uid] = result[uid];
+                        }
+                        resolve(ret);


Any downside using this?

Suggested change

const response = JSON.parse(res.text);

const result = response.result;

const uids = result.uids;

const ret: { [uid: string]: any } = {};

for (let uid of uids) {

ret[uid] = result[uid];

}

resolve(ret);

const result = JSON.parse(res.text)?.result || {};

delete result.uids;

resolve(result);

Your format is much cleaner so I added this in my new commit.

zhx828

Great job! Sorry the PR becomes such a beast than we anticipated and took so much of your time! Don't forget to squash before merging. Please also get all approval from others.

onursumer

Looks good! Let's not forget to squash before merging.

…ansferred oncokb utils from cbioportal-frontend, removed mobx dependency, refactored into functional components moved rest of files to oncokb-frontend-commons and fixed errors Removed mobx dependency from oncokb-frontend-commons by changing classes to functional components, updated imports fixed some import/export errors fixed imports as well as functionality crashing issue fixed styling issue completed requested revisions, added callbacks to oncokb, moved status helpers, added dependencies, etc. updated package.json files, imports, and react hooks in OncoKB.tsx added jest.config.ts, changed oncokb-frontend-commons version to 0.0.1 updated react hooks with useCallback in OncoKB transferred oncokb-utils from cbioportal-utils, removed cbioportal-frontend-dependency, changed DefaultTooltip to Tooltip, resolved imports and bugs fixed StatusHelper imports, duplicate StringUtils function, and DefaultTooltip removal Fixed tooltip crashing, set up API request in reference list, still fixing blocked API, removed pubmedcache usage API request no longer blocked, single query instead of multiple, rendering issue fixed on tooltip simplified ReferenceList API response parsing, reverted OncoKBSummmaryTable changed cacheData name and instances of any fixed imports - still addressing pmid api issue manual testing shows no duplicate API calls - ready for review trying to fix logo error. removed unnecessary imports fixed ts.config.test.json fixed import for AlterationTypeConstants fixed unit test queryparams issue delete unnecessary files small fixes based on feedback, api calls fixed on other page removed intialized list

scarrero4660 requested review from onursumer and alisman October 31, 2022 13:28

alisman reviewed Oct 31, 2022

View reviewed changes

onursumer requested changes Oct 31, 2022

View reviewed changes

scarrero4660 requested review from alisman and onursumer November 1, 2022 04:16

onursumer requested changes Nov 1, 2022

View reviewed changes

scarrero4660 requested a review from onursumer November 2, 2022 13:56

onursumer requested changes Nov 2, 2022

View reviewed changes

packages/oncokb-frontend-commons/package.json Show resolved Hide resolved

alisman reviewed Nov 2, 2022

View reviewed changes

scarrero4660 requested review from onursumer, zhx828 and alisman November 8, 2022 14:34

onursumer requested changes Nov 8, 2022

View reviewed changes

scarrero4660 requested a review from onursumer November 8, 2022 21:34

onursumer requested changes Nov 9, 2022

View reviewed changes

packages/cbioportal-frontend-commons/src/lib/StringUtils.ts Show resolved Hide resolved

zhx828 requested changes Nov 17, 2022

View reviewed changes

zhx828 reviewed Nov 17, 2022

View reviewed changes

scarrero4660 requested review from zhx828 and onursumer November 17, 2022 16:23

onursumer reviewed Nov 17, 2022

View reviewed changes

zhx828 reviewed Nov 18, 2022

View reviewed changes

scarrero4660 requested review from onursumer and zhx828 December 1, 2022 19:11

onursumer requested changes Dec 2, 2022

View reviewed changes

scarrero4660 force-pushed the move-oncokbcard branch from 243c22f to 8532514 Compare December 2, 2022 19:53

scarrero4660 closed this Dec 2, 2022

scarrero4660 force-pushed the move-oncokbcard branch from 65ee930 to f7f25e0 Compare December 2, 2022 20:55

onursumer reopened this Dec 2, 2022

scarrero4660 force-pushed the move-oncokbcard branch from 8760d12 to 4024f31 Compare December 2, 2022 21:49

onursumer requested changes Dec 5, 2022

View reviewed changes

zhx828 requested changes Dec 8, 2022

View reviewed changes

scarrero4660 requested review from onursumer and zhx828 December 12, 2022 13:24

zhx828 approved these changes Dec 12, 2022

View reviewed changes

onursumer approved these changes Dec 13, 2022

View reviewed changes

scarrero4660 force-pushed the move-oncokbcard branch from 2996ee1 to 5e9f198 Compare December 13, 2022 18:56

alisman approved these changes Dec 13, 2022

View reviewed changes

alisman merged commit 78075a6 into cBioPortal:master Dec 13, 2022

alisman added chore This PR is related to code maintenance refactoring labels Dec 13, 2022

		@@ -0,0 +1,34 @@
		import { DefaultTooltip } from 'cbioportal-frontend-commons';

	export { errorIcon, loaderIcon } from './components/StatusHelpers';
	export * from './components/StatusHelpers';

		@@ -0,0 +1,270 @@
		import { MobxCache, OncoKbCardDataType } from 'cbioportal-utils';

		@@ -0,0 +1,34 @@
		import DefaultTooltip from './defaultTooltip/DefaultTooltip';

Move OncoKB card and icons into separate package oncokb-frontend-commons #4404

Move OncoKB card and icons into separate package oncokb-frontend-commons #4404

Conversation

scarrero4660 commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onursumer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onursumer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onursumer left a comment • edited by scarrero4660 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhx828 commented Nov 18, 2022

onursumer commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onursumer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhx828 commented Dec 8, 2022

onursumer commented Dec 8, 2022

zhx828 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhx828 left a comment

Choose a reason for hiding this comment

onursumer left a comment

Choose a reason for hiding this comment

onursumer left a comment •

edited

Loading

onursumer left a comment •

edited by scarrero4660

Loading