[PA] Reset busy state on cancellation and chart switch#2857
Open
khairul-syazwan wants to merge 11 commits into
Open
[PA] Reset busy state on cancellation and chart switch#2857khairul-syazwan wants to merge 11 commits into
khairul-syazwan wants to merge 11 commits into
Conversation
khairul-syazwan
requested review from
LSriragavan,
brandantck,
jerome-ng,
maggie-li-yd and
p-hoffmann
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses PA charts’ loading indicator getting stuck by removing module-level shared cancellation tokens and ensuring chartBusy is reset on cancellation, unmount, and chart switches. It updates multiple chart components to own request cancellation per instance and adds/updates unit tests to cover the busy-state lifecycle.
Changes:
- Refactors store actions to accept an optional caller-provided
cancelTokeninstead of using shared module-level cancellation. - Updates PA and cohort-comparison chart components to manage per-instance request cancellation and consistently emit/reset busy state on unmount and chart switches.
- Adds/updates unit tests and snapshots to validate busy-state lifecycle behavior.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/ui/apps/vue-mri-ui-lib/src/store/modules/query.ts | Removes shared cancel token; fireQuery accepts optional cancelToken. |
| plugins/ui/apps/vue-mri-ui-lib/src/store/modules/bookmark.ts | Removes shared cancel token; fireBookmarkQuery accepts optional cancelToken. |
| plugins/ui/apps/vue-mri-ui-lib/src/store/modules/cohortDefinition.ts | Removes shared cancel token and updates actions toward caller-owned cancellation. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/StackBarChart.vue | Moves chart/request state to instance; adds per-instance cancellation + busy lifecycle handling. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/PatientListContainer.vue | Adds per-instance cancellation + busy lifecycle handling for patient list requests. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/ChartController.vue | Resets busy state on chart switch and unmount. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/PatientAnalytics.vue | Resets chartBusy on active chart change and unmount. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/KaplanMeier.vue | Adds per-instance cancellation + busy lifecycle handling for KM chart. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/BoxplotChart.vue | Adds per-instance cancellation + busy lifecycle handling for boxplot chart. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/SACChart.vue | Adds per-instance cancellation + ensures busy state is emitted/cleared. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/StackBarCohortCompare.vue | Moves chart/request state to instance; adds per-instance cancellation + busy lifecycle handling. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/BoxplotCohortCompare.vue | Refactors cohort-compare boxplot to per-instance cancellation + busy lifecycle handling. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/KMCohortCompare.vue | Refactors cohort-compare KM to per-instance cancellation + busy lifecycle handling. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/CohortComparisonContainer.vue | Resets busy state on active chart switches. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/tests/StackBarChart.test.ts | Adds tests for busy-state lifecycle, cancellation behavior, and unmount reset. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/tests/PatientListContainer.test.ts | Adds new tests for patient-list busy-state lifecycle and unmount reset. |
| plugins/ui/apps/vue-mri-ui-lib/src/components/tests/snapshots/KaplanMeier.test.ts.snap | Updates snapshot for newly added request lifecycle state fields. |
Comments suppressed due to low confidence (1)
plugins/ui/apps/vue-mri-ui-lib/src/store/modules/cohortDefinition.ts:203
fireGetAtlasCohortDefinitionQueryis declared with a third positionalcancelTokenparameter, but Vuexdispatchonly passes a single payload argument. As written, callers can’t provide a cancelToken via dispatch (and this signature is inconsistent with the other actions updated to accept{ cancelToken }).
fireGetAtlasCohortDefinitionQuery({ commit, dispatch, getters, rootGetters }, cohortDefinitionId, cancelToken) {
return dispatch('ajaxAuth', {
url: `/d2e-webapi/cohortdefinition/${cohortDefinitionId}`,
method: 'GET',
cancelToken,
datasetId: rootGetters.getSelectedDataset.id,
b24b484 to
6f6a7ce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2852 — PA chart loading indicator could get stuck after navigating away and back, because
chartBusywas never reset when an in-flight request was cancelled.Root cause
fireQuery,fireBookmarkQuery, and several cohort-comparison charts used a module-level sharedcanceltoken. When any new request started, it cancelled the previous one. The cancelled component then skippedbusyEv(false)because the error message was'cancel', leavingchartBusy = truepermanently if the component was destroyed or replaced before another emit happened.Changes
Store layer (removes shared cancellation)
src/store/modules/query.tssrc/store/modules/bookmark.tssrc/store/modules/cohortDefinition.tsThese actions now accept an optional
cancelTokenfrom the caller instead of maintaining a sharedcancelvariable.Active chart components
src/components/StackBarChart.vue— main chart used in PA; module-levelstackBarChartmoved to instance state, requests owned per instancesrc/components/PatientListContainer.vue— patient list chartsrc/components/ChartController.vue— resetschartBusyon chart switch and unmountsrc/components/PatientAnalytics.vue— ownschartBusy; resets on active chart change and unmountOther chart types (not the reported bug, but same leak)
src/components/KaplanMeier.vuesrc/components/BoxplotChart.vuesrc/components/SACChart.vue— previously never emitted busy state; now doesCohort comparison charts (separate feature, same pattern)
src/components/StackBarCohortCompare.vuesrc/components/BoxplotCohortCompare.vuesrc/components/KMCohortCompare.vuesrc/components/CohortComparisonContainer.vue— resetschartBusyon active chart switch and unmountTests
src/components/__tests__/StackBarChart.test.ts— added busy-state lifecycle testssrc/components/__tests__/PatientListContainer.test.ts— new test file for busy-state lifecyclesrc/components/__tests__/__snapshots__/KaplanMeier.test.ts.snap— updated for added state fieldsFiles most important to the reported issue
The bug was reproduced with the stacked bar chart. The critical files are:
src/components/StackBarChart.vuesrc/components/ChartController.vuesrc/components/PatientAnalytics.vuesrc/components/PatientListContainer.vuesrc/store/modules/query.tsFiles not in the active reproduction path
These files fix the same busy-state leak but are not part of the create-cohort → navigate-away → return flow:
src/components/KaplanMeier.vue— only used when chart type is switched to KMsrc/components/BoxplotChart.vue— only used when chart type is switched to boxplotsrc/components/SACChart.vue— SAC custom charts featuresrc/components/StackBarCohortCompare.vue— cohort comparison featuresrc/components/BoxplotCohortCompare.vue— cohort comparison featuresrc/components/KMCohortCompare.vue— cohort comparison featuresrc/components/CohortComparisonContainer.vue— cohort comparison featureHow to test
Expected: API fires and returns, chart renders; loading indicator does not stay stuck.
Verification
StackBarChart,PatientListContainer,KaplanMeier,ChartControllerApp.startup.test.ts,app-segmented-button.test.ts)Merge Checklist
Please cross check this list if additions / modifications needs to be done on top of your core changes and tick them off. Reviewer can as well glance through and help the developer if something is missed out.
developbranch)