feat(Mode): Add mode selector#5987
Conversation
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Viewers
 |
||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
feat/mode-selector
|
| Run status |
|
| Run duration | 02m 18s |
| Commit |
|
| Committer | Igor Octaviano |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
37
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
This does not look like the same image/setup. Also, these are all getting replaced in another PR to be just viewport comparisons. Lets wait for that other PR to merge shortly and then you shouldn't need to do the updates here.
There was a problem hiding this comment.
Sounds good. Thx.
There was a problem hiding this comment.
@wayfarer3130, the PR you mentioned was merged? What is left here beyond updating the tests? @dan-rukas any thoughts?
|
@dan-rukas - can you check the mode selector here? We can leave it like this, or I could provide a customization framework allowing app-config defined customizations to allow injecting the mode selector in specific configurations, or you could provide a bit of UI for where this should go. My feeling is it is ok as is, but you might want it differently. |
|
@igoroctaviano - reading this, it looks to me like this loses the measurements etc on changing modes. |
sedghi
left a comment
sedghi
left a comment
There was a problem hiding this comment.
Lots of tests are updated to an incorrect image.
|
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
| return useMemo(() => { | ||
| const next = new URLSearchParams(locationSearch); | ||
| preserveQueryParameters(next); | ||
| const s = next.toString(); | ||
| return s ? `?${s}` : ''; | ||
| }, [locationSearch]); |
There was a problem hiding this comment.
next is already built from locationSearch (which equals window.location.search), so preserveQueryParameters(next) — whose current defaults to new URLSearchParams(window.location.search) — calls next.append(key, value) for each preserved key that is already present, producing duplicates like ?configUrl=https://...&StudyInstanceUIDs=xxx&configUrl=https://.... Any OHIF deployment that passes ?configUrl=… will encounter this on every mode switch. Deleting the keys from next before re-adding them via preserveQueryParameters prevents the duplication.
| return useMemo(() => { | |
| const next = new URLSearchParams(locationSearch); | |
| preserveQueryParameters(next); | |
| const s = next.toString(); | |
| return s ? `?${s}` : ''; | |
| }, [locationSearch]); | |
| return useMemo(() => { | |
| const next = new URLSearchParams(locationSearch); | |
| // Remove preserved keys first so preserveQueryParameters doesn't append duplicates | |
| // (next was initialised from the same search string that preserveQueryParameters reads). | |
| for (const key of preserveKeys) { | |
| next.delete(key); | |
| } | |
| preserveQueryParameters(next); | |
| const s = next.toString(); | |
| return s ? `?${s}` : ''; | |
| }, [locationSearch]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/default/src/utils/modeSelectorUtils.ts
Line: 193-198
Comment:
**Duplicate preserved query parameters on mode-switch links**
`next` is already built from `locationSearch` (which equals `window.location.search`), so `preserveQueryParameters(next)` — whose `current` defaults to `new URLSearchParams(window.location.search)` — calls `next.append(key, value)` for each preserved key that is already present, producing duplicates like `?configUrl=https://...&StudyInstanceUIDs=xxx&configUrl=https://...`. Any OHIF deployment that passes `?configUrl=…` will encounter this on every mode switch. Deleting the keys from `next` before re-adding them via `preserveQueryParameters` prevents the duplication.
```suggestion
return useMemo(() => {
const next = new URLSearchParams(locationSearch);
// Remove preserved keys first so preserveQueryParameters doesn't append duplicates
// (next was initialised from the same search string that preserveQueryParameters reads).
for (const key of preserveKeys) {
next.delete(key);
}
preserveQueryParameters(next);
const s = next.toString();
return s ? `?${s}` : '';
}, [locationSearch]);
```
How can I resolve this? If you propose a fix, please make it concise.3 participants
Context
Fixes #5188
Users needed a way to discover and switch OHIF viewer modes (workflows) from the UI instead of guessing or hacking URLs.
This change introduces a toolbar control that opens a compact popover listing available modes from
appConfig.loadedModes. For each mode,mode.isValidModeis evaluated withmodalities(normalized/→\to match viewer semantics) and a study envelope resolved fromdataSource.query.studies.searchorDicomMetadataStore, aligning with validators such as TMTV.Valid destinations navigate with
Link(/{modeRoute}[/{dataSource}]) whilepreserveQueryParameterskeeps the viewer query string. Invalid modes remain non-navigation rows withTooltipexplanations whenvalidity.descriptionis present.Changes & Results
New / updated
ToolbarModeSelector(Viewers/extensions/default/src/Toolbar/ToolbarModeSelector.tsx): Popover-driven mode list;LayoutSelectorTrigger pattern for the toolbarButton(h-10 w-10,size="icon", Ghost); header Modes shares popover surface (no tinted header slab); selectable rowsLink, current mode styled selection chip, unavailable modes withTooltip(z-[220]so tooltips stack above popoverz-[100]).ohif.modeSelectoringetToolbarModule.tsx;Toolbarindex export optional.ToolbarModeSelectorstrings (Browse modes, Modes, loading line,Current mode,Unable to evaluate this mode),en-US,zh,nl,fr,test-LNGand localeindex.jsregistrations.Modes that include the control use
uiType: 'ohif.modeSelector'in their toolbar definitions (basic, segmentation, tmtv, usAnnotation, preclinical-4d, basic-test-mode, basic-dev-mode).Before → after
isValidModeonly inferred from elsewhere(Optional: attach a short screen recording or screenshot of toolbar → modes popover, current-mode row, disabled row + tooltip.)
Testing
loadedModesas needed).Linkto/{routeName}preserving query params;dataSourcesegment preserved when configured.descriptionabove the panel (no inner scroll trap).Checklist
PR
Suggested PR title:
feat(Default): toolbar mode selector with study validation(adjust scope if reviewers preferToolbarModeSelector).Code
Public Documentation Updates
(Consider a short
docsfollow-up noting the toolbarohif.modeSelectorand **Modes/isValidModebehavior.)Tested Environment
Greptile Summary
This PR introduces a toolbar mode selector (
ohif.modeSelector) that opens a popover listing available OHIF modes, evaluating each against the current study's modalities viaisValidMode, and navigating via React RouterLinkwith preserved query parameters. The implementation is well-structured and follows existing toolbar patterns.ToolbarModeSelectorcomponent renders a popover-driven mode list, correctly gating rendering onstudyEnvelopepresence, memoizingdataSource, and guarding against null validity results.modeSelectorUtils.tsaddsfetchStudyEnvelope(with correct method-callthisbinding),evaluateModeValidity, andusePreservedViewerSearch; the hook has a bug wherepreserveQueryParametersduplicates anypreserveKeysalready present in the URL (see inline comment).ohif.modeSelectorentry across all six affected modes, and all i18n locales are in sync.Confidence Score: 4/5
Safe to merge after fixing the duplicate query parameter issue in
usePreservedViewerSearch.The
usePreservedViewerSearchhook initialisesnextfromlocationSearchand then callspreserveQueryParameters(next), which appends the same preserved keys (configUrl,multimonitor,screenNumber,hangingProtocolId) a second time. In any OHIF deployment that uses?configUrl=…, every mode-switch link will carry a doubledconfigUrl, producing a malformed URL on navigation. The rest of the component logic — null-validity guards, dataSource memoization, study envelope fetching, and locale files — looks correct.extensions/default/src/utils/modeSelectorUtils.ts— specifically theusePreservedViewerSearchhook's interaction withpreserveQueryParameters.Important Files Changed
modeMenuRowsguarded well against null validity; rendering logic correctly gated onstudyEnvelopepresence;dataSourcenow memoized;buildHrefForModedefined with useCallback.fetchStudyEnvelopecorrectly calls search as a method;usePreservedViewerSearchhas a duplicate-param bug — preserved keys are added to a URLSearchParams that already contains them.ohif.modeSelectortoolbar type — straightforward addition matching existingohif.layoutSelectorpattern.ohif.modeSelectorbutton entry before Layout — consistent pattern applied across all 6 modes.Prompt To Fix All With AI
Reviews (8): Last reviewed commit: "Merge branch 'master' into feat/mode-sel..." | Re-trigger Greptile