View Annotations and Datasets from Command Palette #9087
Conversation
📝 WalkthroughWalkthroughAdds an exported helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
frontend/javascripts/viewer/view/components/command_palette.tsx
Outdated
Show resolved
Hide resolved
knollengewaechs
changed the title
frontend/javascripts/viewer/view/components/command_palette.tsx
Outdated
Show resolved
Hide resolved
This reverts commit 4fd2ecb.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/javascripts/viewer/view/components/command_palette.tsx (3)
73-75: Remove leftover debugconsole.logstatementsThe
console.log("Rendering Command Palette")andconsole.log("h")logs look like temporary debugging output and were already flagged to be removed in a previous review. They should be dropped before merging to avoid noisy consoles in production.- console.log("Rendering Command Palette"); ... - console.log("h");Also applies to: 117-119
269-285: View mode entries and tool shortcuts integration are well structured
getViewModeEntries(including “Reset layout”) andshortCutDictForTools/getToolEntriesare wired cleanly:
- View mode commands correctly dispatch
setViewModeActionand emitLayoutEvents.resetLayout.- Tool commands include readable shortcut labels while still dispatching
setToolAction.Once the typing for
shortcutis adjusted as suggested above, this section is good to go.Also applies to: 286-297, 299-315
31-38: Fix command typing forshortcutandhighlightto unblock TypeScript/CIThe new
shortcutfield on commands (line 310 ingetToolEntries) and use ofhighlightinrenderCommand(line 366) conflict with theCommand/CommandWithoutIdtypings with strict TypeScript enabled:
- Object literal with
shortcutcannot be assigned toCommandWithoutId[]type.- Destructuring
highlightandshortcutinrenderCommandaccesses fields not present onCommandtype.Fix by extending the local
CommandWithoutIdtype to include optional fields and casting inrenderCommand:- type CommandWithoutId = Omit<Command, "id">; + type CommandWithoutId = Omit<Command, "id"> & { + shortcut?: string; + };And in
renderCommand:- renderCommand={(command) => { - const { name, shortcut, highlight: maybeDirtyString } = command; + renderCommand={(command) => { + const { name, shortcut, highlight: maybeDirtyString } = command as Command & { + shortcut?: string; + highlight?: string; + };This allows your local commands to carry shortcuts and access the library's
highlightfield without TS errors.Also remove debug
console.logstatements at lines 74 and 118.
🧹 Nitpick comments (2)
frontend/javascripts/dashboard/advanced_dataset/dataset_action_view.tsx (1)
23-24: UsegetViewDatasetURLconsistently for “View Dataset” navigationThe new use of
getViewDatasetURL(dataset)for the inline “View Dataset” link looks good and keeps URL construction centralized.To avoid future drift, you could also switch the context‑menu “View” action to use the same helper:
- onClick: () => { - window.location.href = `/datasets/${getReadableURLPart(dataset)}/view`; - }, + onClick: () => { + window.location.href = getViewDatasetURL(dataset); + },This keeps all dataset view URLs in sync.
Also applies to: 262-265, 313-320
frontend/javascripts/viewer/view/components/command_palette.tsx (1)
69-72: Reconsider manual HTML sanitization beforedangerouslySetInnerHTML
cleanStringOfMostHTMLplusdangerouslySetInnerHTMLis a reasonable first defense, but it’s still a hand‑rolled sanitizer around user‑influenced strings (e.g. dataset/annotation names viahighlight). This is brittle and easy to get wrong over time.If you keep
dangerouslySetInnerHTML, consider:
- Using a dedicated sanitizer (e.g. DOMPurify) with a strict allowlist (only
<b>).- Or avoiding HTML altogether by rendering plain text and letting React handle highlighting via JSX instead of raw HTML.
That would fully address the XSS warning and make the code easier to reason about.
Also applies to: 365-381
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/dashboard/advanced_dataset/dataset_action_view.tsx(2 hunks)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts(1 hunks)frontend/javascripts/viewer/view/components/command_palette.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
🧬 Code graph analysis (2)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
frontend/javascripts/types/api_types.ts (2)
APIDataset(243-246)APIDatasetCompact(271-276)
frontend/javascripts/dashboard/advanced_dataset/dataset_action_view.tsx (1)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
getViewDatasetURL(753-755)
🪛 ast-grep (0.40.0)
frontend/javascripts/viewer/view/components/command_palette.tsx
[warning] 374-374: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 GitHub Actions: CI Pipeline
frontend/javascripts/viewer/view/components/command_palette.tsx
[error] 310-310: TypeScript error TS2353: Object literal may only specify known properties, and 'shortcut' does not exist in type 'CommandWithoutId'.
🪛 GitHub Check: backend-tests
frontend/javascripts/viewer/view/components/command_palette.tsx
[failure] 310-310:
Object literal may only specify known properties, and 'shortcut' does not exist in type 'CommandWithoutId'.
[failure] 366-366:
Property 'highlight' does not exist on type 'Command'.
[failure] 366-366:
Property 'shortcut' does not exist on type 'Command'.
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/view/components/command_palette.tsx
[failure] 310-310:
Object literal may only specify known properties, and 'shortcut' does not exist in type 'CommandWithoutId'.
[failure] 366-366:
Property 'highlight' does not exist on type 'Command'.
[failure] 366-366:
Property 'shortcut' does not exist on type 'Command'.
🔇 Additional comments (2)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
743-755: New URL helper is consistent and type‑safe
getViewDatasetURLcorrectly reusesgetReadableURLPartand matches the existing/datasets/{readable}/viewpattern for bothAPIDatasetandAPIDatasetCompact. No issues from a typing or correctness perspective.frontend/javascripts/viewer/view/components/command_palette.tsx (1)
117-145: Dynamic dataset/annotation commands and palette reset logic look soundThe dynamic commands (
viewDataset,viewAnnotation) plushandleSelect/closePaletteand the static command aggregation inallStaticCommandsare coherent:
- Selecting “> View Dataset...” / “> View Annotation...” expands into concrete items via
getDatasetItems/getAnnotationItems.- Empty lists are handled with a Toast message.
- Final selections close the palette via
closePaletteand reset commands ononRequestClose.No functional issues stand out here.
Also applies to: 146-157, 164-175, 177-181, 327-341
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/view/components/command_palette.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
🪛 ast-grep (0.40.0)
frontend/javascripts/viewer/view/components/command_palette.tsx
[warning] 376-376: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/view/components/command_palette.tsx
[failure] 368-368:
Property 'highlight' does not exist on type 'Command'.
[failure] 368-368:
Property 'shortcut' does not exist on type 'Command'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
frontend/javascripts/viewer/view/components/command_palette.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/viewer/view/components/command_palette.tsx (1)
34-36: Fix extended command types forshortcut/highlightto satisfy TypeScript.
frontend-testsis failing becauseCommandWithoutIddoesn’t declareshortcut, andrenderCommanddestructuresshortcut/highlightfromCommand, which doesn’t include those fields in the lib typings. Extending the type locally and casting insiderenderCommandkeeps things type-safe while allowing the extra props.-// duplicate fields because otherwise, optional fields of Command yield errors -type CommandWithoutId = Omit<Command, "id">; +// Extend react-command-palette's Command with our own optional fields. +type ExtendedCommand = Command & { + shortcut?: string; + highlight?: string; +}; + +// duplicate fields because otherwise, optional fields of Command yield errors +type CommandWithoutId = Omit<ExtendedCommand, "id">; @@ - renderCommand={(command) => { - const { name, shortcut, highlight: maybeDirtyString } = command; + renderCommand={(command) => { + const { name, shortcut = "", highlight: maybeDirtyString } = + command as ExtendedCommand;This also resolves the object-literal error in
getToolEntries, sinceshortcutbecomes part ofCommandWithoutId.Also applies to: 285-297, 309-309, 365-367
🧹 Nitpick comments (2)
frontend/javascripts/viewer/view/components/command_palette.tsx (2)
71-74: Custom HTML cleaning +dangerouslySetInnerHTMLis brittle; consider a safer approach.
cleanStringOfMostHTMLplusdangerouslySetInnerHTMLis narrowly scoped to allow only<b>tags, but regex-based HTML sanitizers are easy to get wrong and static analysis flags this as a potential XSS vector. If feasible, consider either using a small sanitizer (e.g., DOMPurify) onhighlight, or parsing the<b>markers and rebuilding JSX instead of injecting raw HTML.Also applies to: 373-377
117-145: TightenhandleSelecttyping and add error handling for async dataset/annotation loads.
handleSelectis typed asRecord<string, unknown>, and failures ingetDatasets/getReadableAnnotationswould currently bubble up and break the palette. You can leverage the lib’sCommandtype and surface backend failures via Toast instead.- const handleSelect = useCallback(async (command: Record<string, unknown>) => { - if (typeof command === "string") { - return; - } - - if (command.name === DynamicCommands.viewDataset) { - const items = await getDatasetItems(); - if (items.length > 0) { - setCommands(items); - } else { - Toast.info("No datasets available."); - } - return; - } - - if (command.name === DynamicCommands.viewAnnotation) { - const items = await getAnnotationItems(); - if (items.length > 0) { - setCommands(items); - } else { - Toast.info("No annotations available."); - } - return; - } - - closePalette(); - }, []); + const handleSelect = useCallback( + async (command: Command | string) => { + if (typeof command === "string") { + return; + } + + if (command.name === DynamicCommands.viewDataset) { + try { + const items = await getDatasetItems(); + if (items.length > 0) { + setCommands(items); + } else { + Toast.info("No datasets available."); + } + } catch { + Toast.info("Failed to load datasets."); + } + return; + } + + if (command.name === DynamicCommands.viewAnnotation) { + try { + const items = await getAnnotationItems(); + if (items.length > 0) { + setCommands(items); + } else { + Toast.info("No annotations available."); + } + } catch { + Toast.info("Failed to load annotations."); + } + return; + } + + closePalette(); + }, + [], + );This makes the callback type-accurate and avoids uncaught errors when the dataset/annotation requests fail.
Also applies to: 145-155, 163-175, 176-180
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/view/components/command_palette.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
📚 Learning: 2025-09-04T10:01:56.727Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8895
File: frontend/javascripts/viewer/view/action_bar_view.tsx:0-0
Timestamp: 2025-09-04T10:01:56.727Z
Learning: In frontend/javascripts/viewer/view/action_bar_view.tsx, knollengewaechs prefers not to use onKeyUpCapture for preventing modal close on modifier keys because: 1) holding ctrl while opening modal will still close on keyup, 2) normal case of opening modal then pressing ctrl doesn't cause issues with just onKeyDownCapture.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/components/command_palette.tsx (4)
frontend/javascripts/admin/rest_api.ts (2)
getDatasets(995-1024)getReadableAnnotations(472-479)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
getViewDatasetURL(753-757)frontend/javascripts/viewer/constants.ts (1)
ViewModeValues(4-4)frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
setViewModeAction(115-119)
🪛 ast-grep (0.40.0)
frontend/javascripts/viewer/view/components/command_palette.tsx
[warning] 373-373: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/view/components/command_palette.tsx
[failure] 309-309:
Object literal may only specify known properties, and 'shortcut' does not exist in type 'CommandWithoutId'.
[failure] 365-365:
Property 'highlight' does not exist on type 'Command'.
[failure] 365-365:
Property 'shortcut' does not exist on type 'Command'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/javascripts/viewer/view/components/command_palette.tsx (2)
268-283: View-mode and tool commands + shortcuts look good; keep them aligned with actual keybindings.
getViewModeEntriesandshortCutDictForToolsmake the palette much more discoverable and give users a clear mapping from tools to"Ctrl + K, …"combos. Just ensure these strings stay in sync with whatever actually binds those shortcuts so the palette doesn’t drift out of date.Also applies to: 285-297, 298-314
326-339: Palette state andpaletteKeyhandling are consistent with the dynamic submenu UX.Using
allStaticCommandsas the baseline pluscommandsstate, apaletteKey-driven remount,closeOnSelect={false}, andonRequestClose={() => setCommands(allStaticCommands)}matches the intended behaviour (expand datasets/annotations in-place, but reset to the static set whenever the palette is closed). Given the palette blocks interaction with the rest of the UI, this should keep the visible commands in sync with the viewer state on each open.Also applies to: 341-349, 353-364
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/javascripts/viewer/view/components/command_palette.tsx (2)
367-384: Cast command to extended type to fix TypeScript errors.The
renderCommandcallback receives aCommandtype from react-command-palette, which doesn't includeshortcutorhighlightproperties, causing build failures.Apply this diff to cast the command to your extended type:
renderCommand={(command) => { - const { shortcut, highlight: maybeDirtyString, name } = command; + const { shortcut, highlight: maybeDirtyString, name } = + command as Command & { shortcut?: string; highlight?: string }; const cleanString = cleanStringOfMostHTML(maybeDirtyString);As per the static analysis hints. This mirrors the fix needed for the type definition at line 34.
34-34: Fix TypeScript build errors by extending the Command type.The
CommandWithoutIdtype doesn't include theshortcutandhighlightproperties you're using throughout the file, causing build failures at lines 312 and 368.Apply this diff to extend the type:
-// duplicate fields because otherwise, optional fields of Command yield errors -type CommandWithoutId = Omit<Command, "id">; +// Extend Command type to include optional shortcut and highlight fields +type CommandWithoutId = Omit<Command, "id"> & { + shortcut?: string; + highlight?: string; +};As per the static analysis hints.
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/components/command_palette.tsx (1)
329-339: Consider memoizing allStaticCommands for performance.The
allStaticCommandsarray is recreated on every render, which calls multiple getter functions. While the performance impact is likely minor, memoizing it could avoid unnecessary work.- const allStaticCommands = [ + const allStaticCommands = useMemo(() => [ viewDatasetsItem, viewAnnotationItems, ...getNavigationEntries(), ...getThemeEntries(), ...getToolEntries(), ...getViewModeEntries(), ...mapMenuActionsToCommands(menuActions), ...getTabsAndSettingsMenuItems(), ...getSuperUserItems(), - ]; + ], [ + activeUser, + isInTracingView, + isViewMode, + userConfig, + restrictions, + menuActions, + ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/viewer/view/components/command_palette.tsx(7 hunks)frontend/stylesheets/command_palette.less(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
📚 Learning: 2025-09-04T10:01:56.727Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8895
File: frontend/javascripts/viewer/view/action_bar_view.tsx:0-0
Timestamp: 2025-09-04T10:01:56.727Z
Learning: In frontend/javascripts/viewer/view/action_bar_view.tsx, knollengewaechs prefers not to use onKeyUpCapture for preventing modal close on modifier keys because: 1) holding ctrl while opening modal will still close on keyup, 2) normal case of opening modal then pressing ctrl doesn't cause issues with just onKeyDownCapture.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/components/command_palette.tsx (4)
frontend/javascripts/admin/rest_api.ts (2)
getDatasets(995-1024)getReadableAnnotations(472-479)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
getViewDatasetURL(753-757)frontend/javascripts/viewer/constants.ts (1)
ViewModeValues(4-4)frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
setViewModeAction(115-119)
🪛 ast-grep (0.40.0)
frontend/javascripts/viewer/view/components/command_palette.tsx
[warning] 376-376: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/view/components/command_palette.tsx
[failure] 312-312:
Object literal may only specify known properties, and 'shortcut' does not exist in type 'CommandWithoutId'.
[failure] 368-368:
Property 'highlight' does not exist on type 'Command'.
[failure] 368-368:
Property 'shortcut' does not exist on type 'Command'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/stylesheets/command_palette.less (1)
103-110: LGTM! Proper scrolling support for expanded suggestions.The additions of
overflow-y: scroll;andscrollbar-width: thin;correctly enable vertical scrolling in the suggestions container while maintaining the fixed 315px max-height. The CSS cascade is sound—overflow-y: scroll;properly overrides the y-axis behavior from the earlieroverflow: hidden;shorthand, keeping the x-axis hidden to prevent horizontal scroll. This aligns well with the PR's goal of dynamically populating the palette with datasets and annotations, which can exceed the container height.frontend/javascripts/viewer/view/components/command_palette.tsx (1)
288-299: Nice addition!The shortcut dictionary provides helpful keyboard shortcut hints for users, improving discoverability of tool switching commands.
frontend/javascripts/viewer/view/components/command_palette.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/viewer/view/components/command_palette.tsx (1)
34-36: Fix TypeScript typings for extended command fields (shortcut/highlight)Current typing causes the frontend TS build to fail:
CommandWithoutId = Omit<Command, "id">doesn’t declareshortcut, yet several commands specifyshortcut.renderCommanddestructuresshortcutandhighlightfrom the incomingcommand, which is typed asCommandbyreact-command-paletteand doesn’t expose those properties.Define an extended command type and assert before destructuring so both object literals and
renderCommandcompile cleanly.Suggested diff:
@@ -// duplicate fields because otherwise, optional fields of Command yield errors -type CommandWithoutId = Omit<Command, "id">; +type ExtendedCommand = Command & { + // Extra optional fields used in our UI but not declared on `react-command-palette`'s Command. + shortcut?: string; + highlight?: string; +}; + +type CommandWithoutId = Omit<ExtendedCommand, "id">; @@ const shortCutDictForTools: Record<string, string> = { @@ commands.push({ name: `Switch to ${tool.readableName}`, command: () => Store.dispatch(setToolAction(tool)), - shortcut: shortCutDictForTools[tool.id] || "", + shortcut: shortCutDictForTools[tool.id] || "", color: commandEntryColor, }); @@ - renderCommand={(command) => { - const { shortcut, highlight: maybeDirtyString, name } = command; + renderCommand={(command) => { + const { name, shortcut = "", highlight: maybeDirtyString } = + command as ExtendedCommand; const cleanString = cleanStringOfMostHTML(maybeDirtyString); @@ - <span>{shortcut}</span> + <span>{shortcut}</span>After this change, rerun
yarn typecheck/yarn test-frontendto confirm the TS errors aboutshortcut/highlightare gone.#!/bin/bash yarn typecheckAlso applies to: 298-323, 378-392
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/components/command_palette.tsx (1)
118-149: Dynamic dataset/annotation commands: behaviour and UX look solid; consider optional limitsThe async handling for
> View Dataset…and> View Annotation…looks good:
- Errors are surfaced via
Toast.error, empty results viaToast.info.- Annotations are sorted by
modifieddescending, matching the dashboard behaviour.- Dataset/annotation items navigate directly via
getViewDatasetURL(dataset)and/annotations/${annotation.id}, which aligns with the rest of the UI.One optional refinement:
getDatasets()without alimitmight become heavy in very large installations. If you start seeing performance issues, consider passing a reasonablelimit(e.g., 100–200) togetDatasetsand/or adding simple client-side filtering, but that’s not required for correctness now.Also applies to: 154-193
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
frontend/javascripts/viewer/view/components/command_palette.tsx(7 hunks)package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
📚 Learning: 2025-09-04T10:01:56.727Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8895
File: frontend/javascripts/viewer/view/action_bar_view.tsx:0-0
Timestamp: 2025-09-04T10:01:56.727Z
Learning: In frontend/javascripts/viewer/view/action_bar_view.tsx, knollengewaechs prefers not to use onKeyUpCapture for preventing modal close on modifier keys because: 1) holding ctrl while opening modal will still close on keyup, 2) normal case of opening modal then pressing ctrl doesn't cause issues with just onKeyDownCapture.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/components/command_palette.tsx (4)
frontend/javascripts/admin/rest_api.ts (2)
getDatasets(995-1024)getReadableAnnotations(472-479)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
getViewDatasetURL(753-757)frontend/javascripts/viewer/constants.ts (1)
ViewModeValues(4-4)frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
setViewModeAction(115-119)
🪛 ast-grep (0.40.0)
frontend/javascripts/viewer/view/components/command_palette.tsx
[warning] 386-386: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/view/components/command_palette.tsx
[failure] 322-322:
Object literal may only specify known properties, and 'shortcut' does not exist in type 'CommandWithoutId'.
[failure] 378-378:
Property 'highlight' does not exist on type 'Command'.
[failure] 378-378:
Property 'shortcut' does not exist on type 'Command'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (3)
package.json (1)
124-155: DOMPurify as a runtime dependency looks appropriateUsing DOMPurify from the viewer at runtime justifies adding it under
dependencies(and only there). No issues spotted here.If you want to double‑check compatibility, verify that
dompurify@^3.3.0is supported by your bundler/TS setup (e.g.,yarn add dompurify@^3.3.0in a throwaway branch and runyarn typecheck).frontend/javascripts/viewer/view/components/command_palette.tsx (2)
281-297: View‑mode/layout commands and palette lifecycle wiring look consistentThe added view-mode entries (
ViewModeValues+setViewModeAction) and “Reset layout” (layoutEmitter.emit(LayoutEvents.resetLayout)) integrate cleanly with the existing viewer state.The palette lifecycle wiring (
commands+commandsWithIds,paletteKey-based remount,closeOnSelect={false},onRequestClose={() => setCommands(allStaticCommands)}) matches the dynamic-lists example fromreact-command-paletteand should behave as intended (static commands on open, dynamic lists only after explicit selection).No changes needed here from my side.
Also applies to: 339-357, 358-367, 372-377
71-75: Now let me check the web search results about DOMPurify 3.3.0 support forALLOWED_ATTR:
DOMPurify 3.3.0 documentation ALLOWED_ATTR
And let me also check the official DOMPurify repository or changelog:
DOMPurify version 3.3.0 release notes ALLOWED_ATTR support
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/view/components/command_palette.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
📚 Learning: 2025-09-04T10:01:56.727Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8895
File: frontend/javascripts/viewer/view/action_bar_view.tsx:0-0
Timestamp: 2025-09-04T10:01:56.727Z
Learning: In frontend/javascripts/viewer/view/action_bar_view.tsx, knollengewaechs prefers not to use onKeyUpCapture for preventing modal close on modifier keys because: 1) holding ctrl while opening modal will still close on keyup, 2) normal case of opening modal then pressing ctrl doesn't cause issues with just onKeyDownCapture.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
🪛 ast-grep (0.40.0)
frontend/javascripts/viewer/view/components/command_palette.tsx
[warning] 394-394: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/view/components/command_palette.tsx
[failure] 386-386:
Property 'highlight' does not exist on type 'Command'.
[failure] 386-386:
Property 'shortcut' does not exist on type 'Command'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (3)
frontend/javascripts/viewer/view/components/command_palette.tsx (3)
31-41: Command modeling and tool shortcut wiring look solidDefining a local
Command/CommandWithoutIdshape with optionalshortcut/highlightand usingshortCutDictForToolsto populateshortcutintogetToolEntries, then aggregating everything viaallStaticCommands, is clear and maintainable. The structure should make it easy to extend with more commands or shortcuts later without surprises.Also applies to: 306-317, 330-330, 347-357
79-82: DOMPurify sanitization makesdangerouslySetInnerHTMLacceptable hereUsing
cleanStringOfMostHTMLwithDOMPurify.sanitizeandALLOWED_TAGS: ["b"]before passing content intodangerouslySetInnerHTMLis a reasonable mitigation against XSS in this context. The static warning aboutdangerouslySetInnerHTMLcan be considered addressed, since only sanitized markup with a single safe tag is rendered, and the fallback to plain{name}handles the non-highlighted case cleanly.Also applies to: 393-399
386-401: Verify the proposed TypeScript fix by running the actual TypeScript compiler on the codebase after applying the change.The review comment proposes casting the
commandparameter in therenderCommandcallback to resolve TypeScript errors. However, based on examination of the codebase:
Current code state: The file defines its own local
Commandtype (lines 33–40) that includesshortcut?andhighlight?as optional fields.Objects passed: The
commandsWithIds(lines 366–370) constructs objects with{ id: index, ...command }wherecommandisCommandWithoutId, giving them all required properties includingshortcutandhighlight.Parameter typing issue: The
renderCommandcallback parameter at line 385 is not explicitly typed, so TypeScript infers its type from theReactCommandPaletteprop definition. Ifreact-command-paletteexports aCommandtype that doesn't include these optional fields, then destructuring them would indeed cause type errors.Proposed fix: Casting to
CommandWithoutId & { highlight?: string }as shown in the diff would make the type explicit and safe.However, I could not definitively verify:
- Whether TypeScript errors actually exist (the "frontend-tests" mentioned are not located in the repository)
- The exact exported
Commandtype from@types/react-command-palette(node_modules not available in sandbox)The fix appears reasonable for type safety, but confirmation requires running
tscor the actual test pipeline to verify the errors are resolved.
| onRequestClose={() => setCommands(allStaticCommands)} | ||
| closeOnSelect={false} | ||
| renderCommand={(command) => { | ||
| const { shortcut, highlight: maybeDirtyString, name } = command; |
There was a problem hiding this comment.
@philippotto do you have any idea why this fails and how to fix it? no matter if use the import form command_palette.d.ts or whether I redefine the type in the module, this error occurs. Furthermore, it only occurs in the pipeline, not locally when running yarn typecheck.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/javascripts/viewer/view/components/command_palette.tsx (2)
363-365: Optional: Reset commands when closing to prevent stuck dynamic list (repeat suggestion).
closePalette()bumps the key to remountReactCommandPalettebut doesn't resetcommandsback toallStaticCommands. If a future change adds dynamic items that don't navigate (currently all items usewindow.location.hrefwhich reloads the page), users could get stuck viewing only the dynamic list on subsequent opens.For correctness and future-proofing, consider adding
setCommands(allStaticCommands)before bumping the key, as previously suggested.const closePalette = () => { + setCommands(allStaticCommands); setPaletteKey((prevKey) => prevKey + 1); };
387-387: TypeScript error: Cast command to extended type before destructuring.The
frontend-testspipeline fails becauserenderCommandreceives aCommandtype from the library (react-command-palette) which lacksshortcutandhighlightproperties. The code attempts to destructure these properties without casting, causing TypeScript errors.Apply this fix:
- renderCommand={(command) => { - const { shortcut, highlight: maybeDirtyString, name } = command; + renderCommand={(command) => { + const { shortcut, highlight: maybeDirtyString, name } = command as Command;Note: This was flagged in a previous review and marked as addressed, but the fix is not present in the current code.
🧹 Nitpick comments (3)
frontend/stylesheets/command_palette.less (1)
103-110: Scrolling behavior correctly added; consider consolidating overflow properties.The addition of
overflow-y: auto(Line 108) correctly overrides the y-axis behavior ofoverflow: hidden(Line 104), enabling vertical scrolling withscrollbar-width: thin. This change is appropriate for supporting an expanded suggestions list with datasets and annotations.For clarity, consider replacing
overflow: hiddenwithoverflow-x: hiddento make the intent explicit, reducing cognitive load when reading the ruleset..command-palette-suggestionsContainerOpen { - overflow: hidden; + overflow-x: hidden; border: 1px solid var(--color-bg); max-height: 315px; margin-top: 10px; overflow-y: auto; scrollbar-width: thin; }frontend/javascripts/viewer/view/components/command_palette.tsx (2)
33-41: Consider addressing the TODO comment about the Command type definition.The custom
Commandtype redefines properties from the library to addshortcutandhighlight. While this works, the TODO suggests exploring alternatives. However, given that the library doesn't export an extensible type and TypeScript doesn't allow easy augmentation of third-party types without module augmentation, this approach is pragmatic.If you want to keep this pattern, consider removing the TODO and adding a brief comment explaining why the redefinition is necessary (e.g., "Extends react-command-palette's Command type with optional fields").
163-173: Consider using React Router navigation instead of full page reload.The dataset navigation uses
window.location.href = getViewDatasetURL(dataset)which triggers a full page reload. If the app uses React Router, consider usingnavigate()orhistory.push()to maintain SPA benefits (faster navigation, preserved state).Same applies to annotation navigation at line 189.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/viewer/view/components/command_palette.tsx(7 hunks)frontend/stylesheets/command_palette.less(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T10:01:56.727Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8895
File: frontend/javascripts/viewer/view/action_bar_view.tsx:0-0
Timestamp: 2025-09-04T10:01:56.727Z
Learning: In frontend/javascripts/viewer/view/action_bar_view.tsx, knollengewaechs prefers not to use onKeyUpCapture for preventing modal close on modifier keys because: 1) holding ctrl while opening modal will still close on keyup, 2) normal case of opening modal then pressing ctrl doesn't cause issues with just onKeyDownCapture.
Applied to files:
frontend/javascripts/viewer/view/components/command_palette.tsx
🪛 ast-grep (0.40.0)
frontend/javascripts/viewer/view/components/command_palette.tsx
[warning] 395-395: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/view/components/command_palette.tsx
[failure] 387-387:
Property 'highlight' does not exist on type 'Command'.
[failure] 387-387:
Property 'shortcut' does not exist on type 'Command'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (5)
frontend/javascripts/viewer/view/components/command_palette.tsx (5)
79-82: LGTM! Proper HTML sanitization.The use of DOMPurify with a strict allowlist (
ALLOWED_TAGS: ["b"]) is the correct approach for sanitizing the highlighted text while preserving bold formatting. This effectively mitigates XSS risks.
182-195: LGTM! Annotation sorting and error handling.The annotations are correctly sorted by
modifieddate in descending order (most recent first), and error handling with Toast notifications provides good user feedback.
290-305: LGTM! View mode switching and layout reset.The view mode entries are correctly guarded for tracing view, and the layout reset integration using
layoutEmitteris appropriate.
367-371: LGTM! Proper command transformation with IDs.The
useMemoensures commands are efficiently transformed with IDs only when the commands array changes.
394-396: LGTM! Safe use of dangerouslySetInnerHTML with DOMPurify.The HTML content is sanitized with DOMPurify before rendering, with a strict allowlist of only
<b>tags. This effectively prevents XSS while preserving the highlighted text formatting. The static analysis warning can be safely ignored in this case.
URL of deployed dev instance (used for testing):
Steps to test:
Notes
TODOs:
omit dangerouslySetInnerHTMLIssues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)