refactor: enhance command palette modularity #6139
Conversation
WalkthroughThis pull request introduces enhancements to the command palette feature within the application. It includes new TypeScript type definitions for command actions and shortcuts, new modal components for issue and project management, and a command palette store for managing modal states. Additionally, existing components are refactored to streamline modal handling, and import paths are updated for better module organization. The changes collectively enhance the functionality and structure of the command palette. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (12)
packages/types/src/command-palette.d.ts (1)
12-15: Consider changingkeysto an array of stringsUsing an array
keys: string[]instead of a comma-separated string enhances type safety and makes it easier to manipulate individual keys.Apply this change:
export type TCommandPaletteShortcut = { - keys: string; // comma separated keys + keys: string[]; description: string; };web/ce/components/command-palette/modals/workspace-level.tsx (1)
19-22: Review the necessity oftoString()onworkspaceSlugIf
workspaceSlugis already a string, callingtoString()is unnecessary. Removing it can simplify the code.Apply this change:
<CreateProjectModal isOpen={isCreateProjectModalOpen} onClose={() => toggleCreateProjectModal(false)} - workspaceSlug={workspaceSlug.toString()} + workspaceSlug={workspaceSlug} />web/ce/components/command-palette/modals/project-level.tsx (2)
10-13: Consider strengthening prop types.The props interface could be more specific about the expected types of
workspaceSlugandprojectId. If these are always strings, the type conversion in the component could be avoided.export type TProjectLevelModalsProps = { - workspaceSlug: string; - projectId: string; + workspaceSlug: `${string}`; // template literal type for more specific validation + projectId: `${string}`; // template literal type for more specific validation };
34-35: Remove redundant toString() calls.The
.toString()calls are unnecessary if the prop types are properly defined as strings. This would also improve code clarity.- workspaceSlug={workspaceSlug.toString()} - projectId={projectId.toString()} + workspaceSlug={workspaceSlug} + projectId={projectId}Also applies to: 40-41, 46-47, 50-51
web/ce/components/command-palette/modals/issue-level.tsx (1)
19-73: Consider breaking down the component for better maintainability.The component has multiple responsibilities (issue creation, deletion, bulk deletion). Consider splitting it into smaller, focused components for better maintainability and testing.
Suggested structure:
// CreateIssueModal.tsx export const CreateIssueModal = observer(() => { // Creation-specific logic }); // DeleteIssueModal.tsx export const DeleteIssueModal = observer(() => { // Deletion-specific logic }); // BulkDeleteModal.tsx export const BulkDeleteModal = observer(() => { // Bulk deletion-specific logic }); // IssueLevelModals.tsx export const IssueLevelModals = observer(() => { return ( <> <CreateIssueModal /> <DeleteIssueModal /> <BulkDeleteModal /> </> ); });web/ce/helpers/command-palette.ts (2)
60-69: Consider consolidating duplicate shortcut definitionsThe
backspaceanddeleteshortcuts have identical implementations. Consider consolidating them to reduce code duplication.- backspace: { - title: "Bulk delete issues", - description: "Bulk delete issues in the current project", - action: () => toggleBulkDeleteIssueModal(true), - }, - delete: { - title: "Bulk delete issues", - description: "Bulk delete issues in the current project", - action: () => toggleBulkDeleteIssueModal(true), - }, + ['backspace', 'delete']: { + title: "Bulk delete issues", + description: "Bulk delete issues in the current project", + action: () => toggleBulkDeleteIssueModal(true), + },
80-93: Consider extracting platform-specific logicThe platform-specific shortcut logic is embedded within the shortcuts list. Consider extracting this into a separate helper function for better maintainability.
+const getPlatformSpecificCopyShortcut = (platform: string): TCommandPaletteShortcut => ({ + keys: platform === "MacOS" ? "Ctrl,control,C" : "Ctrl,Alt,C", + description: "Copy issue URL from the issue details page", +}); export const getCommonShortcutsList = (platform: string): TCommandPaletteShortcut[] => [ { keys: "P", description: "Create project" }, // ... other shortcuts ... - { - keys: platform === "MacOS" ? "Ctrl,control,C" : "Ctrl,Alt,C", - description: "Copy issue URL from the issue details page", - }, + getPlatformSpecificCopyShortcut(platform), ];web/core/components/command-palette/shortcuts-modal/commands-list.tsx (2)
6-11: Group related imports togetherConsider grouping the command palette helper imports with other helpers for better organization.
// helpers import { substringMatch } from "@/helpers/string.helper"; +import { + getAdditionalShortcutsList, + getCommonShortcutsList, + getNavigationShortcutsList, +} from "@/plane-web/helpers/command-palette"; // hooks import { usePlatformOS } from "@/hooks/use-platform-os"; -// plane web helpers -import { - getAdditionalShortcutsList, - getCommonShortcutsList, - getNavigationShortcutsList, -} from "@/plane-web/helpers/command-palette";
24-32: Consider memoizing keyboard shortcutsThe keyboard shortcuts array is recreated on every render. Consider using
useMemoto optimize performance.+import { useMemo } from "react"; export const ShortcutCommandsList: React.FC<Props> = (props) => { const { searchQuery } = props; const { platform } = usePlatformOS(); - const KEYBOARD_SHORTCUTS = [ + const KEYBOARD_SHORTCUTS = useMemo(() => [ { key: "navigation", title: "Navigation", shortcuts: getNavigationShortcutsList(), }, { key: "common", title: "Common", shortcuts: getCommonShortcutsList(platform), }, ...getAdditionalShortcutsList(), - ]; + ], [platform]);web/core/store/base-command-palette.store.ts (3)
Line range hint
12-37: LGTM! Consider adding interface documentation.The interface is well-structured with clear grouping of observables, computed properties, and actions. The "Base" prefix accurately reflects its role as a base interface.
Consider adding JSDoc documentation for the interface to explain its purpose and extension pattern:
+/** + * Base interface for command palette stores. + * Implements core modal management functionality that can be extended by specific implementations. + */ export interface IBaseCommandPaletteStore {
86-100: Minor optimization: Remove redundant Boolean wrapper.The implementation is correct, but the
Boolean()wrapper is unnecessary as the expression already returns a boolean value through the OR operations.- get isAnyBaseModalOpen() { - return Boolean( + get isAnyBaseModalOpen(): boolean { + return ( this.isCreateIssueModalOpen || this.isCreateCycleModalOpen || this.isCreateProjectModalOpen || this.isCreateModuleModalOpen || this.isCreateViewModalOpen || this.isShortcutModalOpen || this.isBulkDeleteIssueModalOpen || this.isDeleteIssueModalOpen || this.createPageModal.isOpen ); }
Line range hint
103-214: Consider reducing code duplication in toggle methods.The toggle methods follow a consistent pattern, which suggests an opportunity for a more DRY implementation using a generic toggle helper.
private toggleHelper<T>( field: keyof this, value?: boolean | T, defaultValue?: T ) { if (value !== undefined) { this[field] = value; if (defaultValue !== undefined) { this.createIssueStoreType = defaultValue as any; } } else { this[field] = !this[field]; if (defaultValue !== undefined) { this.createIssueStoreType = defaultValue as any; } } } // Example usage: toggleCreateIssueModal = (value?: boolean, storeType?: TCreateModalStoreTypes) => { this.toggleHelper('isCreateIssueModalOpen', value, storeType || EIssuesStoreType.PROJECT); }; toggleShortcutModal = (value?: boolean) => { this.toggleHelper('isShortcutModalOpen', value); };This would reduce code duplication while maintaining the same functionality. The special case for
createPageModalwould still need its own implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
packages/types/src/command-palette.d.ts(1 hunks)packages/types/src/index.d.ts(1 hunks)web/ce/components/command-palette/modals/index.ts(1 hunks)web/ce/components/command-palette/modals/issue-level.tsx(1 hunks)web/ce/components/command-palette/modals/project-level.tsx(1 hunks)web/ce/components/command-palette/modals/workspace-level.tsx(1 hunks)web/ce/helpers/command-palette.ts(1 hunks)web/ce/store/command-palette.store.ts(1 hunks)web/core/components/command-palette/command-palette.tsx(4 hunks)web/core/components/command-palette/shortcuts-modal/commands-list.tsx(3 hunks)web/core/hooks/store/use-command-palette.ts(1 hunks)web/core/store/base-command-palette.store.ts(5 hunks)web/core/store/root.store.ts(1 hunks)web/ee/components/command-palette/modals/index.ts(1 hunks)web/ee/store/command-palette.store.ts(1 hunks)web/helpers/command-palette.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- web/helpers/command-palette.ts
- web/ee/components/command-palette/modals/index.ts
- web/ce/components/command-palette/modals/index.ts
- web/ee/store/command-palette.store.ts
🔇 Additional comments (17)
web/core/components/command-palette/command-palette.tsx (9)
5-5: Simplified route parameter extraction using useParams
Replacing usePathname with useParams from next/navigation streamlines the retrieval of route parameters and improves code readability.
15-20: Enhanced modularity by importing consolidated modal components
Importing IssueLevelModals, ProjectLevelModals, and WorkspaceLevelModals from a centralized module improves code organization and promotes reusability of modal components.
23-24: Consistent use of user permission constants
Importing EUserPermissions and EUserPermissionsLevel ensures consistent usage of permission levels across the application.
25-29: Refactored to use helper functions for shortcut lists
Utilizing getGlobalShortcutsList, getProjectShortcutsList, and getWorkspaceShortcutsList enhances maintainability by centralizing shortcut definitions.
33-33: Efficient extraction of route parameters
Destructuring workspaceSlug, projectId, and issueId directly from useParams simplifies access to these values.
38-38: Improved readability with destructuring from useUser hook
Extracting currentUser and canPerformAnyCreateAction directly enhances code clarity.
129-133: Verify dependency array in useMemo for shortcutsList
Ensure that getGlobalShortcutsList, getWorkspaceShortcutsList, and getProjectShortcutsList do not depend on external variables or state. If they have dependencies, consider including them in the dependency array to prevent stale memoized values.
204-205: Extended keydown event handling with handleAdditionalKeyDownEvents
Including handleAdditionalKeyDownEvents(e) allows for additional keyboard event processing, enhancing extensibility of event handling.
235-239: Confirm necessity of calling toString() on workspaceSlug and projectId
If workspaceSlug and projectId are already strings, invoking toString() may be redundant. Verify whether these variables could be of a different type and adjust accordingly to prevent potential runtime errors.
packages/types/src/command-palette.d.ts (2)
1-4: Well-defined type for TCommandPaletteActionList
The use of Record<string, { title: string; description: string; action: () => void }> accurately represents the action list structure.
6-10: Clear structure for TCommandPaletteShortcutList
Defining key, title, and shortcuts provides a clear schema for shortcut groups.
web/core/hooks/store/use-command-palette.ts (1)
5-5: Updated import path for ICommandPaletteStore
The import path has been updated to @/plane-web/store/command-palette.store. Verify that this path is correct and that all references to ICommandPaletteStore are consistently updated throughout the codebase.
web/ce/store/command-palette.store.ts (1)
13-19: Ensure computed properties are properly observed
Confirm that isAnyBaseModalOpen is correctly defined and marked as an observable in BaseCommandPaletteStore. This ensures that isAnyModalOpen reacts to changes as expected.
packages/types/src/index.d.ts (1)
35-35: LGTM! Export follows alphabetical ordering.
The addition of the command-palette export aligns with the PR's modularity objectives and maintains the file's alphabetical ordering convention.
web/ce/components/command-palette/modals/project-level.tsx (1)
15-59: LGTM! Component structure follows best practices.
The component demonstrates good practices:
- Uses MobX observer pattern correctly
- Centralizes modal state management through hooks
- Maintains consistent prop passing
- Follows the single responsibility principle
web/core/store/root.store.ts (1)
2-3: LGTM! Import path update aligns with modularization
The change to use an absolute import path from plane-web store is consistent with the PR's objective to enhance command palette modularity.
web/core/store/base-command-palette.store.ts (1)
Line range hint 40-67: LGTM! Well-structured base class implementation.
The abstract class pattern is appropriate for a base implementation, and all observables are properly configured with MobX. The default values are sensible and align with the expected behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/core/store/base-command-palette.store.ts (1)
86-100: Consider refactoring the modal state check for better maintainabilityWhile the logic is correct, the long chain of boolean conditions could be refactored for better maintainability.
Consider using an array of modal states:
get isAnyModalOpen() { + const modalStates = [ + this.isCreateIssueModalOpen, + this.isCreateCycleModalOpen, + this.isCreateProjectModalOpen, + this.isCreateModuleModalOpen, + this.isCreateViewModalOpen, + this.isShortcutModalOpen, + this.isBulkDeleteIssueModalOpen, + this.isDeleteIssueModalOpen, + this.createPageModal.isOpen + ]; + return modalStates.some(Boolean); - return Boolean( - this.isCreateIssueModalOpen || - this.isCreateCycleModalOpen || - this.isCreateProjectModalOpen || - this.isCreateModuleModalOpen || - this.isCreateViewModalOpen || - this.isShortcutModalOpen || - this.isBulkDeleteIssueModalOpen || - this.isDeleteIssueModalOpen || - this.createPageModal.isOpen - ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/ce/store/command-palette.store.ts(1 hunks)web/core/store/base-command-palette.store.ts(5 hunks)web/core/store/root.store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/ce/store/command-palette.store.ts
- web/core/store/root.store.ts
🔇 Additional comments (4)
web/core/store/base-command-palette.store.ts (4)
Line range hint 12-38: Fix inconsistency in modal property naming
The interface declares isAnyModalOpen but the implementation uses a different name. This should be consistent across both.
As mentioned in the previous review, change the name back to isAnyModalOpen for consistency.
Line range hint 40-82: LGTM! Well-structured MobX store implementation
The abstract class implementation with proper MobX decorators looks good. The new createIssueStoreType observable is correctly initialized and configured.
Line range hint 101-214: LGTM! Well-documented and consistent toggle implementations
The toggle methods are well-documented with JSDoc comments and follow a consistent implementation pattern. The toggleCreateIssueModal correctly handles the additional store type parameter.
24-24: Verify store type usage in dependent components
The addition of createIssueStoreType might affect components that interact with issue creation.
Let's verify the usage:
Also applies to: 67-67
✅ Verification successful
Store type changes are properly integrated across the codebase
The createIssueStoreType is correctly used throughout the codebase:
- It's properly defined in
base-command-palette.store.ts - The
toggleCreateIssueModalfunction correctly accepts and handles the store type parameter - All components using this functionality pass the correct store type:
- Project issues:
EIssuesStoreType.PROJECT - Cycles:
EIssuesStoreType.CYCLE - Project views:
EIssuesStoreType.PROJECT_VIEW - Modules:
EIssuesStoreType.MODULE
- Project issues:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for components using createIssueStoreType or toggleCreateIssueModal
# Search for direct references to createIssueStoreType
echo "Checking createIssueStoreType usage:"
rg "createIssueStoreType" --type ts --type tsx
# Search for toggleCreateIssueModal calls that might need updating
echo -e "\nChecking toggleCreateIssueModal usage:"
rg "toggleCreateIssueModal" --type ts --type tsx
Length of output: 327
Script:
#!/bin/bash
# Let's try without file type restrictions since tsx might not be supported
echo "Checking createIssueStoreType usage:"
rg "createIssueStoreType"
echo -e "\nChecking toggleCreateIssueModal usage:"
rg "toggleCreateIssueModal"
# Also search for related issue creation patterns
echo -e "\nChecking create issue related patterns:"
rg "CreateModal" -A 2
Length of output: 11611
* refactor: enhance command palette modularity * chore: minor updates to command palette store
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
CommandPalettecomponent to improve modularity and maintainability.