Command palette #8392
Command palette #8392
Conversation
|
Warning Rate limit exceeded@knollengewaechs has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces a command palette feature activated with Ctrl+K, allowing users to navigate pages, switch tools, and manage settings. Several utility functions and modal state properties have been added or refactored to improve UI state management, including changes to reducers, actions, and component properties. Additionally, dynamic routing and string formatting improvements have been implemented alongside style adjustments and dependency updates supporting the new command palette. Changes
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
frontend/javascripts/oxalis/view/components/command_palette_helper.tsx (2)
22-31: Consider providing a clearer fallback label.Currently, the function returns an empty string if neither
action.titlenoraction.labelexists. You might want to provide a more explicit fallback label or omit such items entirely to avoid ambiguous or invisible command entries.
33-50: Filter out empty-labeled actions or handle sub-menu items explicitly.The function ignores menu items if they lack labels/titles but returns an empty string if the label is empty. Additionally, Ant Design menu items can include sub-menus or divider items—if those are not intended to become commands, consider filtering them out or handling them separately.
frontend/javascripts/oxalis/model/actions/ui_actions.ts (1)
145-159: New actions match established naming.These added actions maintain a consistent naming convention (
setXModalVisibilityAction) and return shapes that align with the existing pattern. Be sure to add or update unit tests to cover dispatching these new actions.frontend/javascripts/oxalis/view/action-bar/tracing_actions_view.tsx (2)
55-61: Imports for new modal actions are consistent and clear.All newly imported modal visibility actions cleanly extend the existing UI actions structure. Be sure to remove any unused imports before merging.
269-516: Centralized logic for modals and menu items is a good refactor.Extracting modal and menu handling into
getTracingViewModalsAndMenuItemsimproves clarity and makes the code more modular. Consider adding unit tests or integration tests to verify that each modal and menu action behaves as intended in various scenarios.
📜 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 (15)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/libs/utils.ts(1 hunks)frontend/javascripts/navbar.tsx(2 hunks)frontend/javascripts/oxalis/default_state.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/tool_accessor.ts(2 hunks)frontend/javascripts/oxalis/model/actions/ui_actions.ts(3 hunks)frontend/javascripts/oxalis/model/reducers/ui_reducer.ts(1 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/action-bar/tracing_actions_view.tsx(7 hunks)frontend/javascripts/oxalis/view/action-bar/view_dataset_actions_view.tsx(3 hunks)frontend/javascripts/oxalis/view/components/command_palette_helper.tsx(1 hunks)frontend/javascripts/oxalis/view/statusbar.tsx(3 hunks)frontend/javascripts/router.tsx(4 hunks)frontend/stylesheets/main.less(1 hunks)package.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (26)
CHANGELOG.unreleased.md (1)
14-14: LGTM! Clear and concise changelog entry.The changelog entry effectively communicates the new command palette feature and its functionality.
frontend/javascripts/oxalis/view/statusbar.tsx (2)
51-52: LGTM! Appropriate margin adjustment.The margin change ensures proper spacing with the new command palette component.
357-357: LGTM! Good placement of the command palette shortcut.The command palette component is appropriately placed at the end of the shortcuts section, making it easily discoverable for users.
frontend/javascripts/libs/utils.ts (1)
1304-1309: LGTM! Well-implemented utility function.The
getPhraseFromCamelCaseStringfunction is a clean implementation that:
- Uses regex to properly split at uppercase letters
- Capitalizes each word consistently
- Joins the words with spaces
This will provide user-friendly display text for the command palette items.
package.json (2)
30-30: Type definition for react-command-palette addedThis adds the TypeScript type definitions for the new
react-command-palettelibrary, which will enable proper type checking and intellisense support when implementing the command palette feature.
189-189: Command palette dependency addedThis adds the
react-command-palettepackage (v0.22.1) that will be used to implement the keyboard-driven command palette feature mentioned in the PR objectives.frontend/javascripts/oxalis/model/accessors/tool_accessor.ts (2)
6-6: Import for utility function addedThe
capitalizefunction is now imported from the utils library to support the new tool label formatting.
414-419: New utility function for formatting tool namesThis function transforms annotation tool enum values into user-friendly labels by splitting on underscores, converting to lowercase, capitalizing first letters, and joining with spaces. This will improve readability in the command palette UI.
Example transformation:
ERASE_BRUSH→Erase Brushfrontend/javascripts/oxalis/default_state.ts (1)
235-237: New modal visibility state properties addedThese properties will track the visibility state of three modals that can be controlled through the command palette:
- Script addition modal
- Annotation merging modal
- Zarr private links modal
All are initialized to
falseto ensure modals are hidden by default.frontend/javascripts/oxalis/store.ts (1)
541-543: Type definitions added for new modal visibility propertiesThese type definitions ensure type safety when working with the new modal visibility state properties. The
readonlymodifier enforces that these values should only be modified through proper Redux actions, maintaining the unidirectional data flow.frontend/javascripts/oxalis/model/reducers/ui_reducer.ts (1)
109-125: Good addition of new modal visibility reducers.These new reducer cases follow the established pattern and are consistent with existing code, properly handling the modal visibility states for merge annotations, user scripts, and Zarr private links.
frontend/javascripts/navbar.tsx (2)
203-203: Export function for reuse with command palette.Exporting this function allows its reuse in the command palette feature, promoting code modularity.
248-248: Good update to dynamic organization path.Changing the key to use a template string with the organization variable creates a more precise routing path that matches the link destination.
frontend/javascripts/oxalis/view/action-bar/view_dataset_actions_view.tsx (3)
42-61: Good extraction of menu creation logic to reusable function.The
getViewDatasetMenufunction encapsulates the menu item structure previously defined inline, enhancing reusability for the command palette feature.
87-87: Updated to use the new menu generator function.Correctly updated to use the extracted
getViewDatasetMenufunction, simplifying the component code.
9-9:❓ Verification inconclusive
Changed import path for menu interface types.
The import path has been updated from
antd/es/menu/interfacetoantd/lib/menu/interface, which may affect build configuration.
🏁 Script executed:
#!/bin/bash # Check if both import paths are valid or if there are other imports using the old path rg "antd/es/menu/interface" --type ts rg "antd/lib/menu/interface" --type tsLength of output: 1077
Attention: Verify Consistency of Antd Menu Interface Import Paths
The updated import in
frontend/javascripts/oxalis/view/action-bar/view_dataset_actions_view.tsxnow correctly uses"antd/lib/menu/interface". However, our search shows several other files still import from"antd/es/menu/interface", which could lead to build configuration inconsistencies if a uniform import path is expected.
- Confirm whether the change to
"antd/lib/menu/interface"should be applied globally.- Verify the build configuration accommodates the mixed usage of these paths, or update the remaining references for consistency.
frontend/javascripts/router.tsx (4)
76-76: Added import for CommandPalette component.Imported the new CommandPalette component for integration with the router.
90-90: Added isAdminView property to StateProps type.This new property will track whether the current view is an admin view, which is needed for conditional rendering of the CommandPalette.
270-270: Good conditional rendering of CommandPalette component.The CommandPalette is correctly rendered only when both
isAuthenticatedandisAdminVieware true, ensuring it appears only for authenticated users in admin views.
808-808: Appropriate definition of isAdminView in mapStateToProps.The
isAdminViewproperty is correctly defined as the negation ofisInAnnotationView, meaning admin view is active when not in annotation view.frontend/javascripts/oxalis/view/components/command_palette_helper.tsx (1)
52-53: Validate input string before splitting.
key.split("/")might cause an out-of-bounds scenario ifkeycontains no slash. Consider verifying thatkey.includes("/")before accessingkey.split("/")[1]or provide a safe fallback if the index is undefined.frontend/javascripts/oxalis/model/actions/ui_actions.ts (2)
37-39: Types are consistent with existing patterns.These new type definitions for modal visibility align well with the existing approach.
57-59: Expanded union type is correct.Adding these new actions to the
UiActionunion looks appropriate and ensures proper type coverage for your modal operations.frontend/javascripts/oxalis/view/action-bar/tracing_actions_view.tsx (3)
91-91: Allowingnullfor layoutMenu improves flexibility.Switching
layoutMenuto a nullable type fits the new usage scenario, avoiding potential runtime errors when no menu is provided. The updatedTracingViewMenuPropstype also clearly documents the new boolean fields for modals.Also applies to: 100-113
473-483: Merge button logic aligns with new merge modal feature.Adding the "Merge Annotation" menu item and pushing the corresponding
<MergeModalView>intomodalsaccurately extends the UI. Ensure user roles or permissions are properly enforced if merging is restricted to certain users.
785-787: State mapping for newly introduced modals is consistent.The boolean flags (
showMergeAnnotationModal,showAddScriptModal,showZarrPrivateLinksModal) are properly mapped, matching the newly added UI actions. Confirm these are correctly reset if users navigate away or log out.
frontend/javascripts/oxalis/view/components/command_palette_helper.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/view/components/command_palette_helper.tsx (1)
12-12: 🛠️ Refactor suggestionRemove
actimport from React as it should only be used in tests.The React
act()function is designed specifically for testing environments, not for production code. It helps with flushing effects and updates in tests.-import { act } from "react";
🧹 Nitpick comments (5)
frontend/javascripts/oxalis/view/components/command_palette_helper.tsx (5)
147-147: Consider using React Router's navigation instead of direct window.location assignment.Using
window.location.hrefcauses a full page reload. For a smoother user experience, consider using React Router's navigation methods.-window.location.href = entry.path; +import { useNavigate } from "react-router-dom"; +// Add at component level: +const navigate = useNavigate(); +// Then replace with: +navigate(entry.path);
46-46: Extract hardcoded color value into a constant or theme variable.The color "#5660ff" is repeated multiple times throughout the component. Extract this to a constant or theme variable to maintain consistency and make future updates easier.
+const COMMAND_COLOR = "#5660ff"; // Then use COMMAND_COLOR instead of "#5660ff" in all placesAlso applies to: 104-104, 149-149, 172-172
178-185: Consider memoizing command lists to prevent unnecessary recalculations.The command lists are recreated on every render, which could impact performance for large lists. Consider using
useMemoto optimize this.-const menuActions = getMenuActions(isViewMode); - -const allCommands = [ - ...getNavigationEntries(), - ...getToolEntries(), - ...mapMenuActionsToCommands(menuActions), - ...getTabsAndSettingsMenuItems(), -]; +const menuActions = useMemo(() => getMenuActions(isViewMode), [isViewMode, isInTracingView, props]); + +const allCommands = useMemo(() => [ + ...getNavigationEntries(), + ...getToolEntries(), + ...mapMenuActionsToCommands(menuActions), + ...getTabsAndSettingsMenuItems(), +], [menuActions, userConfig, activeUser, isInTracingView, isViewMode, restrictions]);
122-125: Handle possible null adminMenu more explicitly.The code uses optional chaining to access
adminMenu?.children, but doesn't check ifadminMenuis null before callingmap(). This could be clearer.const adminMenu = getAdministrationSubMenu(false, activeUser); const adminCommands: Array<{ name: string; path: string }> = []; -adminMenu?.children.map((entry: { key: string }) => { - adminCommands.push({ name: getLabelForPath(entry.key), path: entry.key }); -}); +if (adminMenu && adminMenu.children) { + adminMenu.children.forEach((entry: { key: string }) => { + adminCommands.push({ name: getLabelForPath(entry.key), path: entry.key }); + }); +}
189-194: Simplify map operation by updating id directly inside command object.The current code spreads the command and then overrides the id property. This can be simplified.
<ReactCommandPalette - commands={allCommands.map((command, counter) => { - return { - ...command, - id: counter, - }; - })} + commands={allCommands.map((command, counter) => ({ + ...command, + id: counter, + }))} hotKeys={["ctrl+k", "command+k"]} trigger={label} closeOnSelect resetInputOnOpen maxDisplayed={100} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/oxalis/view/components/command_palette_helper.tsx(1 hunks)frontend/javascripts/router.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/router.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (1)
frontend/javascripts/oxalis/view/components/command_palette_helper.tsx (1)
168-171: Removeact()wrapper around Redux dispatch.
act()is designed for testing to flush effects and updates. Using it in a production component might cause unexpected behavior. Dispatch the store action directly.-act(() => { - Store.dispatch(setToolAction(tool)); -}); +Store.dispatch(setToolAction(tool));
| { | ||
| key: "share-button", | ||
| onClick: () => Store.dispatch(setShareModalVisibilityAction(true)), | ||
| icon: <ShareAltOutlined />, | ||
| label: "Share", | ||
| }, | ||
| screenshotMenuItem, | ||
| renderAnimationMenuItem, | ||
| { | ||
| key: "python-client-button", | ||
| onClick: () => Store.dispatch(setPythonClientModalVisibilityAction(true)), | ||
| icon: <DownloadOutlined />, | ||
| label: "Download", | ||
| }, |
There was a problem hiding this comment.
screenshotMenuItem and renderAnimationMenuItem are already defined at the top level. how about doing the same for the others?
| if (layoutMenu != null) items.push(layoutMenu); | ||
| return { items }; |
There was a problem hiding this comment.
I think, the functions interface is a bit convoluted (e.g., the wrapping in items and the optional pushing of layout menu which is not needed for the command palette case).
how about sth like this:
// top level
const screenshotMenuItem = ...;
...
const viewDatasetMenuItems = [screenshotMenuItem, ...];
// in ViewDatasetActionsView:
const overlayMenu: MenuProps = {
items: [...viewDatasetMenu, props.layoutMenu];
}
// in the command palette
const items = viewDatasetMenu; // or inline directly| @@ -409,3 +410,10 @@ export function adaptActiveToolToShortcuts( | |||
|
|
|||
| return activeTool; | |||
| } | |||
|
|
|||
| export const getLabelForTool = (tool: AnnotationTool) => { | |||
| return tool | |||
There was a problem hiding this comment.
there is already an explicit mapping from tool to string in toolbar_view.tsx. it's named TOOL_NAMES. could you move the constant into this accessor file and make use of it in this function?
| import { getViewDatasetMenu } from "../action-bar/view_dataset_actions_view"; | ||
|
|
||
| const getLabelForAction = (action: ItemType) => { | ||
| if (action == null) return ""; |
There was a problem hiding this comment.
ideally, the function should never be called with null. if this is not possible for some reason, then please don't use empty strings as a return value because this loses typechecking information (the caller won't be forced to handle empty strings explicitly).
my suggestion would be to use action: NonNullable<ItemType> as a parameter and remove the empty string handling.
| if ("label" in action && action.label != null) { | ||
| return action.label.toString(); | ||
| } | ||
| return ""; |
There was a problem hiding this comment.
when does this happen? I'd suggest to either remove the case, return null or throw an error.
|
|
||
| navigationEntries.forEach((entry, counter) => { | ||
| commands.push({ | ||
| id: counter, |
There was a problem hiding this comment.
how about adding a category: "Navigate" property here. will look like this: https://main--5c34dd1c22fa8200244c31dd.chromatic.com/?path=/story/command-palette--with-multiple-highlights
the same for settings-related stuff
| id: counter, | ||
| name: `Switch to ${getLabelForTool(tool)} Tool`, | ||
| command: () => Store.dispatch(setToolAction(tool)), | ||
| color: "#5660ff", |
There was a problem hiding this comment.
this color appears multiple times in this module. extract into var?
| commands={allCommands.map((command, counter) => { | ||
| return { | ||
| ...command, | ||
| id: counter, |
There was a problem hiding this comment.
so, this overwrites all previously defined ids (which is good because they would be not unique otherwise). can we avoid defining the other ids in the first place?
| @@ -261,11 +266,258 @@ export function getLayoutMenu(props: LayoutMenuProps): SubMenuType { | |||
| }; | |||
| } | |||
|
|
|||
| export const getTracingViewModalsAndMenuItems = ( | |||
There was a problem hiding this comment.
cool refactoring 👍
two things:
- could you move all handle* variables to the top level? I think they don't depend on the parameters of this function. if they need to access the store, they can do Store.getState() for that (since the handler is only executed at will from the outside, it doesn't need to be reactive).
- I would remove the modal stuff from this function again. it's not needed by the command palette. either, move it back to TracingActionsView or extract it into another helper function. or is there a reason why they need to stay together here?
afterwards, it might make sense to check again whether it's necessary to require all TracingViewMenuProps for this function. maybe we can do with less (which could also make the useSelector stuff in the command palette module simpler).
| @@ -1300,3 +1300,10 @@ export function generateRandomId(length: number) { | |||
| } | |||
| return result; | |||
| } | |||
|
|
|||
| export function getPhraseFromCamelCaseString(stringInCamelCase: string): string { | |||
There was a problem hiding this comment.
I'm not a big fan of such automatic conversions to make something readable for humans. I'd rather have explicit (and type-enforced) mappings. we can keep it for this iteration, but just fyi :)
URL of deployed dev instance (used for testing):
Steps to test:
tested
TODOs:
ideas for follow-up
Issues:
asked for to-dos here
feed-back and follow-up ideas here
(Please delete unneeded items, merge only when none are left open)