[WEB-4208]chore: refactored work item quick actions#7136
[WEB-4208]chore: refactored work item quick actions#7136sriramveeraghanta merged 5 commits intopreviewfrom
Conversation
WalkthroughThis update introduces comprehensive support for nested submenus in dropdown and context menu components across the UI library and issue quick action dropdowns. It refactors menu item logic into reusable hooks, adds new modal and helper modules, implements portal-based rendering for submenus, and enhances menu components to handle keyboard navigation, submenu coordination, and improved outside click detection. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MenuButton
participant CustomMenu
participant SubMenu
participant Portal
participant MenuContext
User->>MenuButton: Clicks menu button
MenuButton->>CustomMenu: Opens menu
CustomMenu->>MenuContext: Registers submenu closers
User->>CustomMenu: Hovers/clicks menu item with nestedMenuItems
CustomMenu->>SubMenu: Opens nested submenu
SubMenu->>Portal: Renders submenu in portal
User->>SubMenu: Navigates submenu (mouse/keyboard)
SubMenu->>MenuContext: Closes other submenus if needed
User->>SubMenu: Selects submenu item
SubMenu->>MenuContext: Closes all submenus
SubMenu->>CustomMenu: Triggers action and closes menu
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (3)
packages/ui/src/dropdowns/helper.tsx (1)
1-1:⚠️ Potential issueAddress or remove the FIXME comment.
The FIXME comment lacks context about what needs to be fixed. Please either:
- Add a descriptive explanation of what needs to be fixed
- Create a GitHub issue to track this and reference it in the comment
- Remove the comment if it's no longer relevant
web/core/components/issues/issue-layouts/quick-action-dropdowns/all-issue.tsx (1)
141-238: 🛠️ Refactor suggestionExtract duplicated menu item rendering logic into a reusable component.
The rendering logic for menu items is duplicated between regular items and nested items. Consider creating a shared component to reduce code duplication and improve maintainability.
+const MenuItemContent = ({ item, disabled }: { item: any; disabled?: boolean }) => ( + <> + {item.icon && <item.icon className={cn("h-3 w-3", item.iconClassName)} />} + <div> + <h5>{item.title}</h5> + {item.description && ( + <p + className={cn("text-custom-text-300 whitespace-pre-line", { + "text-custom-text-400": disabled || item.disabled, + })} + > + {item.description} + </p> + )} + </div> + </> +); {MENU_ITEMS.map((item) => { if (item.shouldRender === false) return null; // Render submenu if nestedMenuItems exist if (item.nestedMenuItems && item.nestedMenuItems.length > 0) { return ( <CustomMenu.SubMenu key={item.key} trigger={ <div className="flex items-center gap-2"> - {item.icon && <item.icon className={cn("h-3 w-3", item.iconClassName)} />} - <h5>{item.title}</h5> - {item.description && ( - <p - className={cn("text-custom-text-300 whitespace-pre-line", { - "text-custom-text-400": item.disabled, - })} - > - {item.description} - </p> - )} + <MenuItemContent item={item} /> </div> }web/core/components/issues/issue-layouts/quick-action-dropdowns/cycle-issue.tsx (1)
141-249: 🛠️ Refactor suggestionSignificant code duplication with all-issue.tsx submenu rendering.
This entire nested submenu rendering logic (lines 141-249) is nearly identical to the code in
all-issue.tsx. This duplication extends across multiple quick action components. Consider extracting this into a shared component or utility.Consider creating a shared
QuickActionMenucomponent that handles the common rendering logic for all quick action dropdowns. This would:
- Eliminate code duplication across all quick action components
- Ensure consistent behavior and styling
- Make future updates easier to maintain
- Reduce bundle size
Example structure:
// shared/QuickActionMenu.tsx export const QuickActionMenu = ({ items, parentRef, customButton, portalElement, placements }) => { // Common rendering logic here };
🧹 Nitpick comments (12)
web/ce/store/issue/issue-details/root.store.ts (1)
14-16: Consider removing empty makeObservable call.The
makeObservablecall with an empty observables configuration is unusual. If no observables are needed at this level, consider removing the call entirely, or if observables will be added later, add a TODO comment.Apply this diff to either remove the empty call or add a TODO:
- makeObservable(this, { - // observables - }); + // TODO: Add observables if needed for CE-specific functionality + makeObservable(this, {});Or simply remove it if no observables are planned:
- makeObservable(this, { - // observables - });web/ce/components/issues/issue-layouts/quick-action-dropdowns/duplicate-modal.tsx (2)
11-11: Implement the DuplicateWorkItemModal component or add TODO comment.The component currently returns an empty fragment, which is a placeholder implementation. Since this component is being integrated into multiple quick action dropdowns, it should either be implemented or clearly marked as a TODO.
Apply this diff to add a TODO comment:
-export const DuplicateWorkItemModal: FC<TDuplicateWorkItemModalProps> = () => <></>; +// TODO: Implement DuplicateWorkItemModal functionality +export const DuplicateWorkItemModal: FC<TDuplicateWorkItemModalProps> = () => <></>;Would you like me to help implement the modal functionality or create an issue to track this implementation?
11-11: Props are defined but unused.The component accepts props but doesn't use them. Consider adding an underscore prefix to the parameter or destructuring only when implementation is added.
Apply this diff to indicate unused props:
-export const DuplicateWorkItemModal: FC<TDuplicateWorkItemModalProps> = () => <></>; +export const DuplicateWorkItemModal: FC<TDuplicateWorkItemModalProps> = (_props) => <></>;web/core/components/dropdowns/project.tsx (1)
205-205: Consider the accessibility implications of removing flex-grow.The removal of
flex-growfrom the project name span may affect text truncation behavior. Ensure this change aligns with the intended layout design.web/core/components/issues/issue-layouts/kanban/block.tsx (1)
114-114: Consider memoizing the custom action button for performance.The customActionButton is recreated on every render, which could impact performance in large kanban boards.
Consider using
useMemooruseCallbackto optimize:+ const customActionButton = useMemo(() => ( <div ref={menuActionRef} className={`flex items-center h-full w-full cursor-pointer rounded p-1 text-custom-sidebar-text-400 hover:bg-custom-background-80 ${ isMenuActive ? "bg-custom-background-80 text-custom-text-100" : "text-custom-text-200" }`} onClick={() => setIsMenuActive(!isMenuActive)} > <MoreHorizontal className="h-3.5 w-3.5" /> </div> + ), [isMenuActive]);web/core/components/issues/issue-layouts/quick-action-dropdowns/module-issue.tsx (1)
149-212: Consider extracting submenu rendering logic to reduce duplication.The submenu rendering logic is nearly identical to the one in
project-issue.tsx. Consider creating a shared component or utility function to render nested menu items, which would improve maintainability and ensure consistency.You could create a shared component like:
// In a shared file like menu-item-renderer.tsx export const MenuItemRenderer = ({ item }: { item: TContextMenuItem }) => { if (item.shouldRender === false) return null; if (item.nestedMenuItems && item.nestedMenuItems.length > 0) { return <CustomMenu.SubMenu>...</CustomMenu.SubMenu>; } return <CustomMenu.MenuItem>...</CustomMenu.MenuItem>; };Then use it in both components to reduce duplication.
web/core/components/issues/issue-layouts/quick-action-dropdowns/all-issue.tsx (1)
55-62: Consider resetting additional fields when duplicating issues.The current implementation only renames the issue and sets the source ID. Consider resetting other fields that shouldn't be copied:
- Creation/update timestamps
- State/status (if it should default to initial state)
- Assignees (if they should be cleared)
- Comments/activity history
packages/ui/src/dropdowns/context-menu/item.tsx (2)
19-29: Add type safety for nested menu items.The
renderedNestedItemscalculation could be more type-safe and the filtering logic could be extracted for clarity.-const renderedNestedItems = item.nestedMenuItems?.filter((nestedItem) => nestedItem.shouldRender !== false) || []; +const renderedNestedItems = React.useMemo( + () => (item.nestedMenuItems?.filter((nestedItem) => nestedItem.shouldRender !== false) ?? []), + [item.nestedMenuItems] +);
96-106: Consider more flexible closing behavior for nested items.Currently, clicking a nested item always closes the entire context menu. Consider allowing nested items to control whether they close just the submenu or the entire menu.
const handleNestedItemClick = (nestedItem: TContextMenuItem, e?: React.MouseEvent) => { if (e) { e.preventDefault(); e.stopPropagation(); } nestedItem.action(); if (nestedItem.closeOnClick !== false) { - handleClose(); // Close the entire context menu + // Allow controlling whether to close just submenu or entire menu + if (nestedItem.closeEntireMenu !== false) { + handleClose(); // Close the entire context menu + } else { + closeNestedMenu(); // Close just the submenu + } } };packages/ui/src/dropdowns/context-menu/root.tsx (1)
168-198: Consider simplifying the submenu element detection.The implementation is solid, but line 178's check (
target.hasAttribute("data-context-submenu")) appears redundant sincetarget.closest('[data-context-submenu="true"]')on line 174 would already match the target element if it has the attribute.- // Check if the click is on a nested menu element - const isNestedMenuClick = target.closest('[data-context-submenu="true"]'); - const isMainMenuClick = contextMenuRef.current?.contains(target); - - // Also check if the target itself has the data attribute - const isNestedMenuElement = target.hasAttribute("data-context-submenu"); - - // If it's a nested menu click, main menu click, or nested menu element, don't close - if (isNestedMenuClick || isMainMenuClick || isNestedMenuElement) { + // Check if the click is on a nested menu element or main menu + const isNestedMenuClick = target.closest('[data-context-submenu="true"]'); + const isMainMenuClick = contextMenuRef.current?.contains(target); + + // If it's a nested menu click or main menu click, don't close + if (isNestedMenuClick || isMainMenuClick) {packages/ui/src/dropdowns/custom-menu.tsx (1)
158-183: Consider consistent data attribute naming across dropdown components.The custom click handler works correctly, but it uses
data-prevent-outside-clickwhile the context menu usesdata-context-submenu. Consider using consistent attribute names across both components for better maintainability.web/core/components/issues/issue-layouts/quick-action-dropdowns/helper.tsx (1)
232-335: Consider optimizing useMemo dependencies.The specialized context hooks are well-structured and properly memoized. However, including the entire
factoryobject in the dependency arrays might cause unnecessary re-renders since the factory methods are recreated on each render.Consider destructuring only the needed factory methods to optimize the dependency arrays:
export const useProjectIssueMenuItems = (props: MenuItemFactoryProps): TContextMenuItem[] => { const factory = useMenuItemFactory(props); + const { + createEditMenuItem, + createCopyMenuItem, + createOpenInNewTabMenuItem, + createCopyLinkMenuItem, + createArchiveMenuItem, + createDeleteMenuItem, + } = factory; return useMemo( () => [ - factory.createEditMenuItem(), - factory.createCopyMenuItem(), - factory.createOpenInNewTabMenuItem(), - factory.createCopyLinkMenuItem(), - factory.createArchiveMenuItem(), - factory.createDeleteMenuItem(), + createEditMenuItem(), + createCopyMenuItem(), + createOpenInNewTabMenuItem(), + createCopyLinkMenuItem(), + createArchiveMenuItem(), + createDeleteMenuItem(), ], - [factory] + [ + createEditMenuItem, + createCopyMenuItem, + createOpenInNewTabMenuItem, + createCopyLinkMenuItem, + createArchiveMenuItem, + createDeleteMenuItem, + ] ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/ui/src/dropdowns/context-menu/item.tsx(2 hunks)packages/ui/src/dropdowns/context-menu/root.tsx(4 hunks)packages/ui/src/dropdowns/custom-menu.tsx(10 hunks)packages/ui/src/dropdowns/helper.tsx(2 hunks)web/ce/components/issues/issue-layouts/quick-action-dropdowns/copy-menu-helper.tsx(1 hunks)web/ce/components/issues/issue-layouts/quick-action-dropdowns/duplicate-modal.tsx(1 hunks)web/ce/components/issues/issue-layouts/quick-action-dropdowns/index.ts(1 hunks)web/ce/store/issue/issue-details/root.store.ts(1 hunks)web/core/components/dropdowns/project.tsx(4 hunks)web/core/components/issues/issue-layouts/kanban/block.tsx(4 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/all-issue.tsx(5 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/archived-issue.tsx(2 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/cycle-issue.tsx(5 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/draft-issue.tsx(4 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/helper.tsx(1 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/index.ts(1 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/module-issue.tsx(5 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/project-issue.tsx(5 hunks)web/core/store/issue/root.store.ts(1 hunks)web/ee/components/issues/issue-layouts/quick-action-dropdowns/index.ts(1 hunks)web/ee/store/issue/issue-details/root.store.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
web/core/components/issues/issue-layouts/kanban/block.tsx (2)
packages/hooks/src/use-platform-os.tsx (1)
usePlatformOS(3-34)packages/hooks/src/use-outside-click-detector.tsx (1)
useOutsideClickDetector(3-29)
web/core/components/issues/issue-layouts/quick-action-dropdowns/project-issue.tsx (3)
web/core/components/issues/issue-layouts/quick-action-dropdowns/helper.tsx (2)
MenuItemFactoryProps(14-43)useProjectIssueMenuItems(233-247)web/core/store/router.store.ts (1)
workspaceSlug(69-71)packages/ui/src/dropdowns/custom-menu.tsx (1)
CustomMenu(527-527)
web/ce/components/issues/issue-layouts/quick-action-dropdowns/copy-menu-helper.tsx (1)
packages/ui/src/dropdowns/context-menu/root.tsx (1)
TContextMenuItem(12-25)
web/core/components/issues/issue-layouts/quick-action-dropdowns/module-issue.tsx (3)
web/core/components/issues/issue-layouts/quick-action-dropdowns/helper.tsx (2)
MenuItemFactoryProps(14-43)useModuleIssueMenuItems(291-315)web/core/store/router.store.ts (1)
workspaceSlug(69-71)packages/ui/src/dropdowns/custom-menu.tsx (1)
CustomMenu(527-527)
packages/ui/src/dropdowns/context-menu/root.tsx (2)
packages/hooks/src/use-platform-os.tsx (1)
usePlatformOS(3-34)packages/ui/src/dropdowns/context-menu/item.tsx (1)
ContextMenuItem(16-241)
packages/ui/src/dropdowns/context-menu/item.tsx (1)
packages/ui/src/dropdowns/context-menu/root.tsx (3)
ContextMenuContext(50-54)TContextMenuItem(12-25)Portal(33-47)
🔇 Additional comments (28)
web/ce/store/issue/issue-details/root.store.ts (1)
4-5:⚠️ Potential issueFix import naming collision.
Both the class and interface are imported and aliased with the same name
IIssueDetailCore, which creates a naming collision and confusion.Apply this diff to fix the naming collision:
- IssueDetail as IssueDetailCore, - IIssueDetail as IIssueDetailCore, + IssueDetail as IssueDetailCore, + IIssueDetail as IIssueDetailCoreInterface,And update the type export accordingly:
-export type IIssueDetail = IIssueDetailCore; +export type IIssueDetail = IIssueDetailCoreInterface;Likely an incorrect or invalid review comment.
web/core/store/issue/root.store.ts (1)
8-8: LGTM! Import path refactoring looks good.The change from relative to absolute import path aligns well with the modular restructuring being introduced in this PR.
web/ce/components/issues/issue-layouts/quick-action-dropdowns/index.ts (1)
1-2: LGTM! Clean index file structure.The re-export pattern is clean and follows standard conventions for module aggregation.
web/ee/store/issue/issue-details/root.store.ts (1)
1-1: LGTM! Standard EE proxy pattern.The re-export from CE module follows the expected pattern for EE/CE separation and allows for future EE-specific extensions.
web/ee/components/issues/issue-layouts/quick-action-dropdowns/index.ts (1)
1-1: LGTM! Clean EE proxy implementation.The re-export statement correctly follows the established pattern for EE modules to inherit CE functionality, enabling consistent access to the refactored quick action dropdown components.
web/core/components/issues/issue-layouts/quick-action-dropdowns/index.ts (1)
1-7: LGTM! Well-organized export structure.The reorganization of exports and addition of the helper module export supports the centralized menu item creation pattern mentioned in the refactoring objectives.
web/core/components/dropdowns/project.tsx (2)
32-32: LGTM! Good addition for improved UX.The new
currentProjectIdprop enables filtering out the current project from dropdown options, improving user experience by preventing unnecessary selections.
113-115: LGTM! Consistent filtering implementation.The filtering logic correctly excludes the current project from both empty query and search query scenarios, ensuring consistent behavior across all dropdown states.
web/core/components/issues/issue-layouts/kanban/block.tsx (3)
65-67: LGTM! Clean state management setup.The ref and state setup for menu management follows React best practices and properly supports the interactive menu functionality.
91-91: LGTM! Proper outside click handling.The outside click detector correctly closes the menu when clicking outside, providing good UX and preventing menu state issues.
107-107: LGTM! Smart conditional visibility logic.The menu visibility logic properly handles both mobile/desktop states and active menu state, ensuring the menu remains visible when active regardless of hover state.
packages/ui/src/dropdowns/helper.tsx (1)
24-107: Well-structured interfaces for portal and submenu support!The new interfaces properly define the contract for portal rendering and nested submenu functionality, aligning perfectly with the broader dropdown enhancements in this PR.
web/core/components/issues/issue-layouts/quick-action-dropdowns/project-issue.tsx (3)
79-101: Excellent refactoring to centralize menu item logic!The use of
MenuItemFactoryPropsanduseProjectIssueMenuItemshook effectively consolidates menu item generation logic, improving maintainability and consistency across different issue quick action components.
131-139: Good defensive programming with conditional modal rendering.The conditional check for
issue.project_idandworkspaceSlugbefore rendering the duplicate modal prevents potential runtime errors.
154-217: Well-implemented nested submenu support!The submenu rendering logic is properly structured with:
- Clear separation between submenu and regular menu item rendering
- Proper event handling to prevent bubbling
- Consistent styling and disabled state handling
- Good accessibility considerations
web/core/components/issues/issue-layouts/quick-action-dropdowns/module-issue.tsx (1)
72-95: Consistent refactoring pattern with module-specific context.Good job maintaining consistency with the project issue quick actions while properly including module-specific context (moduleId). This makes the codebase more maintainable.
web/core/components/issues/issue-layouts/quick-action-dropdowns/all-issue.tsx (1)
64-86: LGTM! Well-structured refactoring to centralize menu logic.The migration to use
useAllIssueMenuItemshook with a comprehensive props object is a clean architectural improvement that promotes code reuse and maintainability.packages/ui/src/dropdowns/context-menu/root.tsx (3)
27-47: LGTM! Clean Portal implementation.The Portal component is well-implemented with proper lifecycle management and fallback to document.body when no container is specified.
49-88: Well-designed submenu management system.The context-based approach for managing submenu closers is elegant and properly handles cleanup when submenus unmount. Using a Set for tracking closers is an efficient choice.
203-229: Good integration of submenu support.The z-index increase ensures proper layering for nested menus, and the context provider is correctly positioned to provide submenu management capabilities to all child menu items.
packages/ui/src/dropdowns/custom-menu.tsx (5)
21-46: Verify the purpose ofdata-radix-portalattribute.The Portal implementation looks good, but the
data-radix-portal=""attribute on line 45 appears to be for Radix UI compatibility. Please confirm if this is intentional and whether it's needed for styling or other functionality.
48-103: Consistent submenu management implementation.The submenu tracking and management logic mirrors the context menu implementation, which maintains consistency across the dropdown components.
302-445: Well-implemented SubMenu component with clever hover state management.The SubMenu implementation is comprehensive and handles positioning, state management, and coordination well. The synthetic event dispatching (lines 422-437) for maintaining hover state when moving between menu and submenu is a clever solution.
However, consider potential conflicts between the document-level click handler (lines 372-385) and the main menu's click handler. Both are listening for clicks and might cause unexpected behavior.
447-477: Good integration of submenu closing logic.The MenuItem component correctly uses the SubMenuContext to close its parent submenu when clicked, ensuring proper menu behavior.
521-525: Clean API design with static properties.Exposing submenu-related components as static properties provides a clean, organized API for consumers of the CustomMenu component.
web/core/components/issues/issue-layouts/quick-action-dropdowns/helper.tsx (3)
14-43: Well-designed, comprehensive interface.The
MenuItemFactoryPropsinterface effectively captures all necessary properties for menu item creation across different contexts. The optional properties provide good flexibility for various use cases.
45-98: Robust action handlers with good user feedback.The
useIssueActionHandlershook provides well-implemented common actions with proper error handling and user feedback through toast notifications. The memoized work item link generation is efficient.
100-230:Details
✅ Verification successful
Excellent factory pattern implementation.
The
useMenuItemFactoryhook implements a clean factory pattern with consistent method signatures and proper handling of permissions and states. Each factory method is focused and reusable.Note: The
createCopyMenuWithDuplicationfunction is imported on line 12 but its implementation is not shown. Please ensure this function is properly exported from the specified module.
🏁 Script executed:
#!/bin/bash # Description: Verify that createCopyMenuWithDuplication is exported from the quick-action-dropdowns module # Search for the export of createCopyMenuWithDuplication rg -A 5 "export.*createCopyMenuWithDuplication" --glob "web/core/components/issues/issue-layouts/quick-action-dropdowns/**"Length of output: 125
🏁 Script executed:
#!/bin/bash # Search for all occurrences of createCopyMenuWithDuplication in the codebase rg "createCopyMenuWithDuplication" -nLength of output: 536
createCopyMenuWithDuplication export verified.
The
useMenuItemFactoryhook implements a clean factory pattern with consistent method signatures, permission checks, and translation usage. We’ve confirmed thatcreateCopyMenuWithDuplicationis properly exported fromweb/ce/components/issues/issue-layouts/quick-action-dropdowns/copy-menu-helper.tsx. Approving these changes.
web/ce/components/issues/issue-layouts/quick-action-dropdowns/copy-menu-helper.tsx
Show resolved
Hide resolved
web/core/components/issues/issue-layouts/quick-action-dropdowns/archived-issue.tsx
Show resolved
Hide resolved
web/core/components/issues/issue-layouts/quick-action-dropdowns/draft-issue.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ui/src/dropdowns/context-menu/item.tsx (1)
185-242: Missing accessibility attributes for nested menus.The nested menu implementation still lacks proper ARIA attributes for screen reader support, as flagged in the previous review.
Add the required accessibility attributes:
<div ref={setPopperElement} style={styles.popper} {...attributes.popper} + role="menu" + aria-label={`Submenu for ${item.title}`} className={cn( "fixed z-[35] min-w-[12rem] overflow-hidden rounded-md border-[0.5px] border-custom-border-300 bg-custom-background-100 px-2 py-2.5 text-xs shadow-custom-shadow-lg", "ring-1 ring-black ring-opacity-5" )} data-context-submenu="true" >Also add to the parent button:
<button ref={setReferenceElement} type="button" + aria-haspopup={hasNestedItems ? "menu" : undefined} + aria-expanded={hasNestedItems ? isNestedOpen : undefined} className={cn(Additionally, add
role="menuitem"to each nested button:<button key={nestedItem.key} type="button" + role="menuitem" className={cn(
🧹 Nitpick comments (3)
packages/ui/src/dropdowns/context-menu/item.tsx (3)
67-82: Consider adding defensive checks for action execution.The click handler logic is sound, but consider adding a safety check to ensure the action function exists before calling it.
} else { // Execute action for regular items - item.action(); + if (typeof item.action === 'function') { + item.action(); + } if (item.closeOnClick !== false) handleClose(); }
96-106: Add defensive checks for nested item actions.Similar to the main item action, add safety checks for nested item actions to prevent runtime errors.
const handleNestedItemClick = (nestedItem: TContextMenuItem, e?: React.MouseEvent) => { if (e) { e.preventDefault(); e.stopPropagation(); } - nestedItem.action(); + if (typeof nestedItem.action === 'function') { + nestedItem.action(); + } if (nestedItem.closeOnClick !== false) { handleClose(); // Close the entire context menu } };
199-238: Consider adding keyboard support for nested items.While the parent component has keyboard navigation, the individual nested items could benefit from additional keyboard accessibility features like proper focus management and aria-current attributes.
<button key={nestedItem.key} type="button" role="menuitem" + aria-current={index === activeNestedIndex ? "true" : undefined} className={cn( "w-full flex items-center gap-2 px-1 py-1.5 text-left text-custom-text-200 rounded text-xs select-none", { "bg-custom-background-90": index === activeNestedIndex, "text-custom-text-400": nestedItem.disabled, }, nestedItem.className )} onClick={(e) => { e.preventDefault(); e.stopPropagation(); handleNestedItemClick(nestedItem, e); }} onMouseEnter={() => setActiveNestedIndex(index)} disabled={nestedItem.disabled} data-context-submenu="true" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui/src/dropdowns/context-menu/item.tsx(2 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/archived-issue.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/issues/issue-layouts/quick-action-dropdowns/archived-issue.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/ui/src/dropdowns/context-menu/item.tsx (1)
packages/ui/src/dropdowns/context-menu/root.tsx (3)
ContextMenuContext(50-54)TContextMenuItem(12-25)Portal(33-47)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/ui/src/dropdowns/context-menu/item.tsx (5)
1-7: LGTM! Clean imports and integration with the context system.The imports properly include the new
ContextMenuContextandPortalfrom the root module, and the necessary dependencies for nested menu functionality are correctly imported.
19-28: Well-structured state management for nested menus.The state variables are appropriately scoped and named, covering all aspects of nested menu functionality including positioning, active item tracking, and DOM references.
30-53: Robust popper configuration with good fallback options.The popper configuration includes comprehensive placement options and proper overflow prevention. The fallback placements provide good coverage for edge cases where the default right-start placement won't work.
55-65: Proper cleanup and registration pattern.The nested menu registration with the context and cleanup callback is well-implemented. The useCallback ensures stable references and prevents unnecessary re-renders.
134-143: Good fix for the keyboard event listener scope issue.The implementation now correctly scopes the keyboard event listeners to the specific menu element instead of using global window listeners, addressing the previous review concern. The focus management and tabindex setting are also appropriate.
web/core/components/issues/issue-layouts/quick-action-dropdowns/draft-issue.tsx
Outdated
Show resolved
Hide resolved
web/core/components/issues/issue-layouts/quick-action-dropdowns/helper.tsx
Outdated
Show resolved
Hide resolved
web/core/components/issues/issue-layouts/quick-action-dropdowns/module-issue.tsx
Show resolved
Hide resolved
vamsikrishnamathala
force-pushed
the
chore-refactor_quick_actions
branch
from
212d3f6 to
b7ba101
Compare
web/core/components/issues/issue-layouts/quick-action-dropdowns/draft-issue.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/core/components/issues/issue-layouts/quick-action-dropdowns/helper.tsx (2)
210-211: Use translation keys for hardcoded strings.Several menu item titles are hardcoded instead of using the translation system that's already imported and used elsewhere in the file.
Apply this diff to use translation keys:
const createRemoveFromCycleMenuItem = (): TContextMenuItem => ({ key: "remove-from-cycle", - title: "Remove from cycle", + title: t("common.actions.remove_from_cycle"), icon: XCircle, action: () => handleOptionalAction(handleRemoveFromView, "Remove from cycle"), shouldRender: isEditingAllowed, }); const createRemoveFromModuleMenuItem = (): TContextMenuItem => ({ key: "remove-from-module", - title: "Remove from module", + title: t("common.actions.remove_from_module"), icon: XCircle, action: () => handleOptionalAction(handleRemoveFromView, "Remove from module"), shouldRender: isEditingAllowed, }); const createRestoreMenuItem = (): TContextMenuItem => ({ key: "restore", - title: "Restore", + title: t("common.actions.restore"), icon: ArchiveRestoreIcon, action: actionHandlers.handleIssueRestore, shouldRender: isRestoringAllowed, });Also applies to: 218-219, 238-239
137-267: Consider memoizing the factory object for better performance.The
useMenuItemFactoryhook returns a new object on every render, which could cause unnecessary re-renders in consuming components.Consider memoizing the returned factory object:
export const useMenuItemFactory = (props: MenuItemFactoryProps) => { const { t } = useTranslation(); const actionHandlers = useIssueActionHandlers(props); // ... existing code for destructuring props and creating functions ... - return { - ...actionHandlers, - createEditMenuItem, - createCopyMenuItem, - createOpenInNewTabMenuItem, - createCopyLinkMenuItem, - createRemoveFromCycleMenuItem, - createRemoveFromModuleMenuItem, - createArchiveMenuItem, - createRestoreMenuItem, - createDeleteMenuItem, - }; + return useMemo( + () => ({ + ...actionHandlers, + createEditMenuItem, + createCopyMenuItem, + createOpenInNewTabMenuItem, + createCopyLinkMenuItem, + createRemoveFromCycleMenuItem, + createRemoveFromModuleMenuItem, + createArchiveMenuItem, + createRestoreMenuItem, + createDeleteMenuItem, + }), + [actionHandlers, /* add other dependencies as needed */] + ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/core/components/issues/issue-layouts/quick-action-dropdowns/helper.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
web/core/components/issues/issue-layouts/quick-action-dropdowns/helper.tsx (4)
49-78: Well-structured interface with comprehensive props.The
MenuItemFactoryPropsinterface is well-organized with clear grouping of related properties through comments. It covers all necessary props for the different menu item contexts while maintaining good TypeScript typing.
269-372: Excellent use of context-specific hooks with proper memoization.The context-specific hooks (
useProjectIssueMenuItems,useAllIssueMenuItems, etc.) are well-implemented with:
- Proper use of
useMemofor performance optimization- Context-specific customizations (like cycle/module ID handling)
- Appropriate menu item selection for each context
- Correct dependency arrays in memoization
14-47: Good implementation of type-safe utility function with overloads.The
handleOptionalActionfunction provides excellent type safety with proper TypeScript overloads and graceful error handling when optional functions are undefined. The implementation correctly handles both parameterized and non-parameterized function calls.
107-127:⚠️ Potential issueFix logic error in handleIssueRestore function.
There's a contradiction in the logic: line 109 calls
handleOptionalActionwhenhandleRestoreis undefined, but then line 112 still attempts to callhandleRestore(), which will throw a runtime error since it's undefined.Apply this diff to fix the logic:
const handleIssueRestore = async () => { if (!handleRestore) { handleOptionalAction(handleRestore, "Restore"); return; } await handleRestore() .then(() => { setToast({ type: TOAST_TYPE.SUCCESS, title: "Restore success", message: "Your work item can be found in project work items.", }); }) .catch(() => { setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: "Work item could not be restored. Please try again.", }); }); };should be:
const handleIssueRestore = async () => { if (!handleRestore) { handleOptionalAction(handleRestore, "Restore"); return; } try { await handleRestore(); setToast({ type: TOAST_TYPE.SUCCESS, title: "Restore success", message: "Your work item can be found in project work items.", }); } catch { setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: "Work item could not be restored. Please try again.", }); } };Likely an incorrect or invalid review comment.
prateekshourya29
added
ready to merge
and removed
changes required
requires approval
labels
* chore: refactored work item quick actions * chore: update event handling for menu * chore: reverted unwanted changes * fix: update archive copy link * chore: handled undefined function implementation
3 participants
Description
This update refactors the quick action menu for the work items in all the layouts and pages.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores