Rerendering cluster names #769
Conversation
📦 Next.js Bundle AnalysisThis analysis was generated by the next.js bundle analysis action 🤖
|
| Page | Size (compressed) |
|---|---|
global |
516.83 KB (🟡 +187 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
Codecov Report
@@ Coverage Diff @@
## master #769 +/- ##
==========================================
+ Coverage 83.40% 83.42% +0.02%
==========================================
Files 462 462
Lines 7850 7861 +11
Branches 1535 1537 +2
==========================================
+ Hits 6547 6558 +11
Misses 1249 1249
Partials 54 54
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I think it's better to insert clusterNames into the work request body of this plot. The work request body is hashed to get the ETag, changes to the clusterNames would change the work request body and the Etag. I suggest you make the changes inside generatePlotWorkBody
| const { | ||
| loading: cellSetsLoading, | ||
| error: cellSetsError, | ||
| hierarchy: cellSetHierarcy, | ||
| properties: cellSetProperties, | ||
| } = cellSets; | ||
|
|
||
|
|
||
| const clusterNames = Object.values(cellSetProperties).map((el) => el.name); |
There was a problem hiding this comment.
I don't think you should get the values here clusterNames here. Implementing it here would add another parameter to be passed to fetchPlotDataWork. I think it would be better to attach clusterNames inside the body request body that's generated for this plot.
src/utils/work/fetchWork.js
Outdated
| extras = undefined, | ||
| timeout = 180, | ||
| broadcast = false, | ||
| clusterNames = undefined, |
There was a problem hiding this comment.
why not using these optionals and extras and adding the cluster names in them?
There was a problem hiding this comment.
FetchWork function is used for fetching every work, clusterNames are something specific to just some of the fetchWork calls, then why would we want to introduce a new parameter for fetchWork that is relevant only to some work types?
There was a problem hiding this comment.
This is exactly Agi's point which I agree with. I put the cluster names in the dot plot's work body where they are relevant. I also didn't like how I did it initially but was not sure how to pass them otherwise. I think now it should be good
| @@ -9,17 +9,18 @@ const composeDotPlotWorkBody = (config) => { | |||
| numberOfMarkers: config.nMarkerGenes, | |||
| customGenesList: config.selectedGenes, | |||
| groupBy: config.selectedCellSet, | |||
| clusterNames, | |||
There was a problem hiding this comment.
I think it would be good to put clusterNames as the last entry in the body and put a comment, saying that this property is added to make the worker reload if there are changes to the cluster name. The comment would be useful because the property is not used for the calculation,
Description
Added cluster names to the object used for generating the ETag so renaming clusters would trigger a new work request. The ETag cached in the browser would no longer match the ETag that would be generated when a cluster name changes so a new work request would be sent and the cache would be updated. This fixes the problem with the dot plot not showing the new cluster names due to the one in cache being used. Staging deployment: https://ui-kristian-ui769.scp-staging.biomage.net/ (bypassing the integration tests).
Details
URL to issue
https://biomage.atlassian.net/browse/BIOMAGE-2013
Link to staging deployment URL (or set N/A)
N/A
Integration test branch
master
Merge checklist
Your changes will be ready for merging after all of the steps below have been completed.
Code updates
Have best practices and ongoing refactors being observed in this PR
Manual/unit testing
Integration testing
You must check the box below to run integration tests on the latest commit on your PR branch.
Integration tests have to pass before the PR can be merged. Without checking the box, your PR
will not pass the required status checks for merging.
Documentation updates
Optional