feat(vue-mri): open the analysis wizard from an exploration card (#3121) - #3290
Open
khairul-syazwan wants to merge 3 commits into
Open
feat(vue-mri): open the analysis wizard from an exploration card (#3121)#3290khairul-syazwan wants to merge 3 commits into
khairul-syazwan wants to merge 3 commits into
Conversation
Closes #3121. Wiring only: the wizard flow already exists in the cohort builder and none of its dialogs change. The five modals move out of ChartToolbar.vue into DashboardFlowModals.vue verbatim, so both the builder and the exploration page mount one copy instead of the block being duplicated. The Analyze placeholder PR 4 shipped is wired rather than replaced, so it keeps the exported Figma glyph. Availability copies ChartToolbar's own canOpenDashboard: the dataset must allow materialising and the wizards feature must be on, and the tooltip swaps to the disabled wording otherwise. Analyze needs the active bookmark, because dashboardContext returns nulls without one, so unlike the filter summary it cannot avoid committing SET_ACTIVE_BOOKMARK. PatientAnalytics watches that to auto-switch to the cohort builder, which would unmount this page mid-click, so a flag on the explorations store suppresses the switch for the duration of the flow. Card actions on this page do not relocate the user; raise it if the product owner would rather Analyze did. Two things found by hand that the interfaces do not advertise: useDashboardFlow returns raw refs. ChartToolbar gets nested-ref unwrapping for free by holding it in an Options API data(), but props are shallowReactive, so the same object read through a prop yields the Ref rather than its value. Both new consumers wrap it in reactive(). The modals teleport to #app, which is an ancestor of this page, so leaving them mounted meant unmounting the page tore down a teleport whose target was itself being removed. Vue threw during unmount, the update aborted, and clicking a card silently stopped opening the builder. They are now mounted only while the flow is live. The error was invisible until app.config.errorHandler was temporarily unmasked.
The mount condition no longer depends on a sticky flag. It was 'analyzeInProgress || dashboardFlowModalOpen', and analyzeInProgress had exactly one clearing path — the true-to-false edge of the modal flags, itself skipped when isProcessingDashboardFlow is set. Several real paths end the flow with every modal closed and that flag still true: openDashboardModal returning early when no bookmark is active, handleOpenDashboard bailing on an unready config, and the catch in handleRequiredFiltersSubmit. Once stranded it never clears, the modals stay mounted, and unmounting the page then tears down a teleport whose target is going away — the exact crash this PR already fixed once, but permanent, silent, and only recoverable by reloading. The condition is now the modal flags alone, which are refs and cannot strand. isProcessingDashboardFlow cannot serve that purpose: it is a plain let boolean, not a ref, so a computed over it never re-evaluates. analyzeInProgress is now raised only across the loadbookmarkToState dispatch, cleared in a finally. That is the whole window the PatientAnalytics watcher cares about, since it fires on the unset-to-set transition and the bookmark stays set afterwards. Stranding it also disabled the deep-link auto-switch for the rest of the session. openDashboardModal is awaited. It is async, so an unawaited rejection escaped the handler entirely. Closing the flow now clears the active bookmark and resets the query and chart, which pr9/01-analyze-action section 2c requires and the first cut dropped. resetDashboardFlowState discards its record of the filter cards the wizard added without reverting them, so the builder opened carrying a filter the user never added, useUnsavedChanges reported dirty, and the browser tab could not be closed. A failed load no longer leaves the bookmark active. loadbookmarkToState commits SET_ACTIVE_BOOKMARK before the IFR conversion can reject, so an incompatible exploration left the cohort-builder nav tab offering a half-restored cohort. Overlapping opens are rejected, and the dataset id expression now matches ChartToolbar's rather than using ?? against a value that defaults to an empty string.
khairul-syazwan
requested review from
LSriragavan,
brandantck,
jerome-ng,
maggie-li-yd and
p-hoffmann
as code owners
September 8, 2026 05:35
The review noted that no test touched a line this PR added, and that every high-severity defect it found would have been caught by one. The three decisions are extracted into a pure helper so they can be tested without mounting, which is what the repository's testing policy asks for, and the page now calls them rather than inlining the logic: - analyzeBookmarkId, which accepts only a bookmark id. A cohort-definition or Atlas id comes from a different table and the two can collide, and getBookmarkById dereferences a .find() with no guard. The same defect was found in the filter summary PR. - isDashboardFlowOpen, the mount condition for the wizard modals, derived from the modal flags alone so it cannot be left stuck on. - shouldResetDashboardFlow, which resets only on the true-to-false edge and never while the flow reports itself busy, because it closes one modal before opening the next across an await. 13 cases, 996 passing overall. Re-verified in the browser after the refactor: the wizard opens, required filters is reached, the modals unmount on close and a card still opens the cohort builder.
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.
Closes #3121.
Seventh in the Data Exploration redesign stack. Based on
khairul-syazwan/d2e-filter-summary(#3271), notdevelop— review that one first.What this adds
An Analyze action on the exploration card that opens the existing dashboard-wizard flow. This is wiring: the flow already works in the cohort builder and no dialog's content changes.
The five wizard modals move out of
ChartToolbar.vueintoDashboardFlowModals.vueverbatim, so the builder and the exploration page mount one copy instead of the block being duplicated. The Analyze placeholder PR 4 shipped is wired rather than replaced, so it keeps the exported Figma glyph. Availability copiesChartToolbar's owncanOpenDashboard.The design decision
Analyze needs the active bookmark —
dashboardContextreturns nulls without one — so unlike the filter summary it cannot avoid committingSET_ACTIVE_BOOKMARK.PatientAnalyticswatches that to auto-switch to the cohort builder, which would unmount this page mid-click.Chosen: the wizard opens over the exploration page, and the switch is suppressed for the duration of the load. Rationale: #3120 established that a card action does not relocate the user, and two card actions behaving differently is worse than either alone. If the product owner would rather Analyze land in the cohort builder, say so — it is a small change, only moving where
DashboardFlowModalsis mounted.Three things the interfaces do not advertise
useDashboardFlowreturns raw refs.ChartToolbargets nested-ref unwrapping for free by holding it in an Options-APIdata(), but props areshallowReactive, so the same object read through a prop yields theRefrather than its value. Both new consumers wrap it inreactive(). Without this every:disabledand tooltip binding silently misbehaves.isProcessingDashboardFlowis a plainletboolean, not a ref. A computed over it never re-evaluates, so it cannot drive a reactive mount condition — and it can itself remaintrueafter a failed flow.The modals
<Teleport to="#app">, and#appis an ancestor of this page. Leaving them mounted meant unmounting the page tore down a teleport whose target was itself being removed. Vue threwCannot destructure property 'bum' of 'ne' as it is nullduring unmount, the update aborted, and clicking a card silently stopped opening the cohort builder. They are mounted only while a modal is actually open.That last one is worth dwelling on: it reproduced with 983 tests green and three clean builds, and produced no console output, because
app.config.errorHandleris() => nullin bothlifecycles.tsandmain.tsand production strips Vue warnings. It was found only by clicking a card in the running app and then temporarily unmasking the handler. Worth fixing that error handler separately.Review findings, all fixed
A review of the first cut found three HIGH defects:
true, re-creating the teleport crash above — permanently, silently, until reload. Several real paths end the flow with every modal closed and the flag still set:openDashboardModalreturning early with no active bookmark,handleOpenDashboardbailing on an unready config, and thecatchinhandleRequiredFiltersSubmit. The condition is now the modal flags alone, which are refs and cannot strand, and the flag is raised only across the load and cleared in afinally.resetDashboardFlowStatediscards its record of the filter cards the wizard added without reverting them, so the builder opened carrying a filter the user never added,useUnsavedChangesreported dirty, and the browser tab could not be closed. Closing now clears the active bookmark and resets query and chart.openDashboardModal()was not awaited, so a rejection escaped the handler that clears state.Plus: a failed load no longer leaves the bookmark active and offering a half-restored cohort; overlapping opens are rejected; and the dataset id expression matches
ChartToolbar's.Validation evidence — agent reported
No end-to-end suite was run; do not read this as full E2E coverage.
npx vitest run --coverageon Node 20 — the command CI runs — 983 passed, 0 failed.plugins/ui/node_moduleshoists eslint 7.32, which cannot read the app's flat config.Browser, hot-deployed against a local runtime:
>40, GenderFEMALE.Not exercised: Apply Filters onward, which renders a Shiny dashboard the local stack cannot serve. Exercising any of this at all required local database setup — the
wizardsfeature flag and injecting the shipped wizard definitions into the dataset's PA config, which carried nowizardsConfigkey.Reviewer notes
ShinyDashboardModal'sdataset-idwasgetSelectedDataset.idand is now the shared?.id || ''prop, turning a loud failure into an empty string. Deliberate, so both mount sites agree.ChartToolbarretains deadshowDashboardModal/handleOpenDashboardmembers. They were dead before this change; not touched here.