enhance: context menu on sidebar elements #3777
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
UnderKoen
changed the title
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/common/Link.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces significant updates to the In 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: 1
🧹 Outside diff range and nitpick comments (8)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (3)
171-172: Add type definitions for state variables.Consider adding explicit type definitions for better type safety:
-const [crossOffset, setCrossOffset] = useState(0); -const [offset, setOffset] = useState(0); +const [crossOffset, setCrossOffset] = useState<number>(0); +const [offset, setOffset] = useState<number>(0);
225-234: Consider preserving menu position for button clicks.The button press handler resets both offsets to 0, which means the menu will always appear at the default position when clicking the button, but at the click position when right-clicking. This inconsistency might be confusing for users.
Consider either:
- Using the button's position for consistent menu placement
- Always using the click position for both right-click and button click
onPress={() => { - setOffset(0); - setCrossOffset(0); + const rect = triggerRef.current.getBoundingClientRect(); + setCrossOffset(rect.width / 2); + setOffset(0); setMenuOpen(true); }}Also applies to: 245-249
Line range hint
170-267: Well-structured implementation of context menu functionality.The implementation follows good React practices with:
- Feature flag for controlled rollout
- Clean separation of concerns
- Proper event handling and positioning
- Reusable context menu pattern that can be applied to other components
This provides a solid foundation for extending context menu support to other sidebar elements.
packages/desktop-client/src/components/sidebar/Account.tsx (5)
138-149: RenameneedsTooltipfor clarity in context menu logic.The variable
needsTooltipis used to determine whether to enable the context menu, which may be misleading since it's related to tooltip functionality. Renaming it tohasAccountorisAccountItemwould better reflect its purpose and improve code readability.Apply this diff to rename the variable:
const needsTooltip = !!account?.id; const accountRow = ( <View innerRef={dropRef} style={{ flexShrink: 0, ...outerStyle }} onContextMenu={e => { - if (!needsTooltip || !contextMenusEnabled) return; + if (!isAccountItem || !contextMenusEnabled) return; e.preventDefault(); const rect = e.currentTarget.getBoundingClientRect(); setCrossOffset(e.clientX - rect.left); setOffset(e.clientY - rect.top); setMenuOpen(true); }} >
107-117: EnsureeditingRef.currentis not null before accessing properties.In the
useEffecthook, there is a check for!editingRef.current, but within the same block,editingRef.currentis accessed without null checks. Although unlikely, it's good practice to ensureeditingRef.currentis defined before accessing its properties to prevent potential runtime errors.Update the
useEffecthook to consistently check foreditingRef.current:useEffect(() => { if (!editingRef.current) return; if (isEditing) { editingRef.current.focus(); window.getSelection().selectAllChildren(editingRef.current); } else { editingRef.current.textContent = name; } }, [name, isEditing]);
218-229: Prevent default behavior for unsupported keys during editing.Currently, only the
EnterandEscapekeys have specific handlers in theonKeyDownevent. Consider preventing default behavior for other keys that might introduce unwanted content or behavior in thecontentEditablefield.If applicable, you can extend the
onKeyDownhandler to manage additional keys or explicitly allow only specific key inputs.
86-91: Initialize state variables with appropriate types.Ensure that state variables like
crossOffsetandoffset, which control positioning, are initialized with consistent and appropriate types (e.g.,number) to prevent type-related issues.Verify that the initial values match the expected data types used in calculations.
119-132: Add error handling for account name updates.When updating the account name, consider adding error handling to manage cases where the update might fail due to network issues or validation errors. Providing user feedback in such cases can improve the user experience.
Implement error handling logic within the
updateNamefunction to catch and respond to potential errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3777.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/sidebar/Account.tsx(4 hunks)packages/desktop-client/src/components/sidebar/Sidebar.tsx(4 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/desktop-client/src/components/sidebar/Account.tsx (3)
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-09-27T14:15:46.637Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-10-24T17:05:41.415Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)
Learnt from: tlesicka
PR: actualbudget/actual#3593
File: packages/desktop-client/src/components/sidebar/Sidebar.tsx:112-116
Timestamp: 2024-10-10T02:29:05.655Z
Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, the `BudgetName` component consists of three parts: `BudgetName`, `EditBudgetName`, and the Menu. Keeping `EditBudgetName` as a separate component helps maintain cleaner code by separating concerns.
🔇 Additional comments (4)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
11-11: LGTM: Good use of feature flag for controlled rollout.
The feature flag implementation allows for controlled rollout of the context menu functionality.
Also applies to: 170-170
262-263: LGTM: Clean implementation of positioned context menu.
The Popover implementation with dynamic positioning works well with both right-click and button click triggers.
Also applies to: 267-267
packages/desktop-client/src/components/sidebar/Account.tsx (2)
252-257: Handle asynchronous actions appropriately.
When dispatching actions like openAccountCloseModal and reopenAccount, ensure that any state updates or side effects dependent on these actions are handled properly, considering the asynchronous nature of Redux actions.
Confirm that the UI responds correctly after these actions are dispatched and that there are no unintended side effects.
141-147:
Fix offset calculation for context menu positioning.
Subtracting rect.bottom from e.clientY in the onContextMenu handler may cause the context menu to appear at an incorrect vertical position. Typically, you should subtract rect.top from e.clientY to calculate the correct offset relative to the element's top edge.
Apply this diff to fix the offset calculation:
const rect = e.currentTarget.getBoundingClientRect();
setCrossOffset(e.clientX - rect.left);
- setOffset(e.clientY - rect.bottom);
+ setOffset(e.clientY - rect.top);
setMenuOpen(true);⛔ Skipped due to learnings
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/rules/RuleRow.tsx:83-89
Timestamp: 2024-09-24T20:33:56.412Z
Learning: In the 'RuleRow' component, when handling `onContextMenu`, the offset calculation using `e.clientY - rect.bottom` is intentional and correct due to the alignment of the menu.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/rules/RuleRow.tsx:83-89
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In the 'RuleRow' component, when handling `onContextMenu`, the offset calculation using `e.clientY - rect.bottom` is intentional and correct due to the alignment of the menu.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:143-143
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In the `PayeeTableRow` component, the vertical offset for menu positioning is correctly calculated using `setOffset(e.clientY - rect.bottom);` due to the menu alignment.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:143-143
Timestamp: 2024-09-24T20:35:06.300Z
Learning: In the `PayeeTableRow` component, the vertical offset for menu positioning is correctly calculated using `setOffset(e.clientY - rect.bottom);` due to the menu alignment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (3)
171-172: Consider moving context menu state management to a separate file.Based on the existing pattern in the codebase (ref: PR #3381), context menu state management is typically handled in a separate file to ensure only one context menu is open at a time.
245-249: Add cleanup for menu state.Consider resetting the menu position state when the component unmounts to prevent stale state.
function EditableBudgetName() { // ... existing code ... + useEffect(() => { + return () => { + setOffset(0); + setCrossOffset(0); + setMenuOpen(false); + }; + }, []); // ... rest of the code ... }
262-264: Consider extracting BudgetName component.Based on the existing pattern in the codebase (ref: PR #3593), consider moving the
EditableBudgetNamecomponent to a separate file (BudgetName.tsx) to maintain cleaner code organization and separation of concerns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/sidebar/Sidebar.tsx(5 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
Learnt from: tlesicka
PR: actualbudget/actual#3593
File: packages/desktop-client/src/components/sidebar/Sidebar.tsx:112-116
Timestamp: 2024-10-10T02:29:05.655Z
Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, the `BudgetName` component consists of three parts: `BudgetName`, `EditBudgetName`, and the Menu. Keeping `EditBudgetName` as a separate component helps maintain cleaner code by separating concerns.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/budget/SidebarGroup.tsx:69-76
Timestamp: 2024-10-13T11:17:59.711Z
Learning: In the `SidebarGroup` component (`packages/desktop-client/src/components/budget/SidebarGroup.tsx`), context menu state management is handled in another file, ensuring that only one context menu is open at a time.
🔇 Additional comments (2)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
170-170: LGTM! Good use of feature flagging.
The feature flag is appropriately used to gate the context menu functionality, allowing for controlled rollout.
225-234: Consider edge cases in menu positioning.
The current positioning calculation might need additional handling for:
- Screen edges (preventing menu from going off-screen)
- Different window sizes
- High DPI displays
|
Only thing that seems strange is that if you take long enough to choose an option on one of the accounts, the account name/notes popover gets stuck open until you come back and hover accounts again. For example: choose to close one of the accounts, but take long enough that the popover shows up. They popover wont go away until you mouse back over to the sidebar after working with the account close modal. |
…_menus/sidebar # Conflicts: # packages/desktop-client/src/components/sidebar/Sidebar.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (2)
61-63: Consider adding type annotations for improved type safety.The implementation is clean, but could benefit from explicit type annotations for the offset states.
- const [crossOffset, setCrossOffset] = useState(0); - const [offset, setOffset] = useState(0); + const [crossOffset, setCrossOffset] = useState<number>(0); + const [offset, setOffset] = useState<number>(0);
Line range hint
1-162: Consider extracting context menu logic into a custom hook.The context menu positioning logic could be reused across other components (like Account.tsx mentioned in the summary). Consider extracting it into a custom hook for better maintainability and consistency.
Example implementation:
function useContextMenu() { const [menuOpen, setMenuOpen] = useState(false); const [position, setPosition] = useState({ crossOffset: 0, offset: 0 }); const handleContextMenu = (e: React.MouseEvent) => { e.preventDefault(); const rect = e.currentTarget.getBoundingClientRect(); setPosition({ crossOffset: e.clientX - rect.left, offset: e.clientY - rect.bottom }); setMenuOpen(true); }; const resetPosition = () => { setPosition({ crossOffset: 0, offset: 0 }); }; return { menuOpen, setMenuOpen, position, handleContextMenu, resetPosition }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/sidebar/BudgetName.tsx(5 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (2)
Learnt from: tlesicka
PR: actualbudget/actual#3593
File: packages/desktop-client/src/components/sidebar/Sidebar.tsx:112-116
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, the `BudgetName` component consists of three parts: `BudgetName`, `EditBudgetName`, and the Menu. Keeping `EditBudgetName` as a separate component helps maintain cleaner code by separating concerns.
Learnt from: tlesicka
PR: actualbudget/actual#3593
File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:114-136
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In 'packages/desktop-client/src/components/sidebar/BudgetName.tsx', empty budget names are handled elsewhere, so additional error handling within the 'EditableBudgetName' component is unnecessary.
🔇 Additional comments (3)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (3)
8-8: LGTM! Clean feature flag integration.
The feature flag integration follows best practices for controlled feature rollout.
133-137: LGTM! Clean position reset implementation.
The position reset ensures consistent menu positioning when triggered via button press.
154-156: Verify margin impact on popover positioning.
The added margin might affect the popover alignment with the trigger element.
✅ Verification successful
Margin usage consistent across components.
No layout issues detected related to the added margin.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Popover usage to ensure consistent margin usage
rg "margin: 1" --type tsx
Length of output: 55
Script:
#!/bin/bash
# Search for other Popover usage to ensure consistent margin usage
rg "margin: 1" --glob "*.tsx"
Length of output: 1554
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (1)
Line range hint
111-125: Consider adding cleanup for menu stateWhile the implementation is functionally correct, it could benefit from additional cleanup to prevent the popover persistence issue mentioned in the PR comments.
Consider adding a cleanup mechanism:
<View onContextMenu={handleContextMenu}> + <View + onMouseLeave={() => { + const timeout = setTimeout(() => setMenuOpen(false), 300); + return () => clearTimeout(timeout); + }} + > <Button ref={triggerRef} variant="bare" style={{ color: theme.buttonNormalBorder, fontSize: 16, fontWeight: 500, marginLeft: -5, flex: '0 auto', }} onPress={() => { resetPosition(); setMenuOpen(true); }} > + </View>packages/desktop-client/src/components/sidebar/Account.tsx (1)
228-264: Consider adding keyboard shortcuts for menu actionsThe context menu implementation is good, but could be enhanced with keyboard shortcuts for better accessibility.
Consider adding keyboard shortcuts to the menu items:
items={[ - { name: 'rename', text: 'Rename' }, + { name: 'rename', text: 'Rename', keyboardShortcut: 'F2' }, account.closed - ? { name: 'reopen', text: 'Reopen' } - : { name: 'close', text: 'Close' }, + ? { name: 'reopen', text: 'Reopen', keyboardShortcut: 'Ctrl+O' } + : { name: 'close', text: 'Close', keyboardShortcut: 'Ctrl+W' }, ]}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/desktop-client/src/components/sidebar/Account.tsx(5 hunks)packages/desktop-client/src/components/sidebar/BudgetName.tsx(5 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (2)
Learnt from: tlesicka
PR: actualbudget/actual#3593
File: packages/desktop-client/src/components/sidebar/Sidebar.tsx:112-116
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, the `BudgetName` component consists of three parts: `BudgetName`, `EditBudgetName`, and the Menu. Keeping `EditBudgetName` as a separate component helps maintain cleaner code by separating concerns.
Learnt from: tlesicka
PR: actualbudget/actual#3593
File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:114-136
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In 'packages/desktop-client/src/components/sidebar/BudgetName.tsx', empty budget names are handled elsewhere, so additional error handling within the 'EditableBudgetName' component is unnecessary.
packages/desktop-client/src/components/sidebar/Account.tsx (2)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
🔇 Additional comments (8)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (3)
8-8: LGTM! Clean abstraction of context menu logic
The addition of the useContextMenu hook import is a good approach to centralize context menu handling logic.
142-143: LGTM! Clean positioning implementation
The margin addition and position spreading from the context menu hook provide proper visual spacing and positioning.
60-61: Verify context menu behavior with modals
The implementation looks good, but there's a known issue with popover persistence when modals are involved.
Let's check if there are other components using modals with context menus:
✅ Verification successful
Context menu implementation looks safe with modals
After analyzing the codebase, I can confirm that the context menu implementation using useContextMenu is safe with modals. The key observations:
- All context menu implementations use
Popovercomponent with proper trigger refs and position management - Context menus are properly closed via
onOpenChangehandlers - The components using both modals and context menus handle them independently
- The
isNonModalprop is used where needed to prevent modal-related issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using both modals and context menus
ast-grep --pattern 'const { setMenuOpen, menuOpen } = useContextMenu();
$$$
return (
$$$
<Modal
$$$
>
$$$
</Modal>
$$$
);'
Length of output: 162
Script:
#!/bin/bash
# Let's try a different approach to find components using both Modal and useContextMenu
# First, let's find files containing both Modal and useContextMenu
rg -l "Modal" | xargs rg -l "useContextMenu"
# Then let's look at the actual usage patterns
rg -A 10 "useContextMenu" $(rg -l "Modal" | xargs rg -l "useContextMenu")
Length of output: 22545
Script:
#!/bin/bash
# Let's check how Modal and useContextMenu are used together
# Looking for Modal components near useContextMenu usage
rg -B 10 -A 10 "<Modal" $(rg -l "Modal" | xargs rg -l "useContextMenu")
# Also check for Popover usage since it's related to context menus
rg -B 5 -A 5 "<Popover" $(rg -l "Modal" | xargs rg -l "useContextMenu")
Length of output: 13768
packages/desktop-client/src/components/sidebar/Account.tsx (5)
2-21: LGTM: Clean import organization
The new imports are well-organized and properly grouped by their source/purpose.
86-89: LGTM: Successfully implemented suggested useContextMenu hook
Good job implementing the shared context menu logic using the custom hook as suggested in the previous reviews. This reduces code duplication across components.
305-305: LGTM: Fixed stuck popover issue
Good job addressing the feedback about stuck popovers by disabling the tooltip when the context menu is open.
117-130:
Add input validation and sanitization
The current implementation has two potential issues:
- No sanitization of HTML content from contentEditable (security risk)
- Weak validation allows whitespace-only names
Apply this diff to address both issues:
const updateName = () => {
if (account && isEditing) {
setIsEditing(false);
- const newName = editingRef.current.textContent;
+ const newName = editingRef.current.textContent
+ .replace(/<\/?[^>]+(>|$)/g, '')
+ .trim();
- if (newName !== account.name && newName.trim()) {
+ if (newName !== account.name && newName) {
dispatch(
updateAccount({
...account,
name: newName,
}),
);
}
}
};105-115: Verify edge cases in editing behavior
Please verify these potential edge cases:
- Starting edit mode while context menu is open
- Race conditions between edit state and menu state
Also applies to: 208-223
✅ Verification successful
Edge cases in editing behavior are properly handled
The code implements proper safeguards against potential race conditions and edge cases:
- The editing state is managed through a single
isEditingstate variable with clear state transitions - The
onBlurhandler properly cleans up the editing state viaupdateName() - Context menu interaction (
handleContextMenu) is conditionally rendered only whenneedsTooltipis true, preventing overlap with editing mode - The
useEffecthook has appropriate dependencies and null checks before accessing the ref
The implementation follows React best practices for managing contentEditable elements and their state transitions. No race conditions are apparent in the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions in state management
# Look for other components using similar editing + context menu pattern
rg -l 'contentEditable.*contextMenu'
# Check for existing issue reports about race conditions
gh issue list --search "in:title race condition contentEditable"
Length of output: 105
Script:
#!/bin/bash
# Let's analyze the component's state management and context menu interactions more thoroughly
# Find all state management related to editing in this component
rg -A 5 "isEditing|editingRef" "packages/desktop-client/src/components/sidebar/Account.tsx"
# Look for context menu implementations
rg -A 5 "contextMenu|onContextMenu" "packages/desktop-client/src/components/sidebar/Account.tsx"
# Search for similar patterns in other components
rg -l "useState.*isEditing.*useRef.*contentEditable"
# Check for event handlers that might interfere with editing
rg "onBlur|onFocus|onClick" "packages/desktop-client/src/components/sidebar/Account.tsx"
Length of output: 1519
|
The rename input box looks a little out of place, could it be aligned with the rename budget one used at the top of the sidebar?
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/desktop-client/src/components/sidebar/Account.tsx (5)
2-3: Remove unused importuseEffectto clean up the codebase.The
useEffecthook is imported but not used in this component. Removing unused imports helps keep the code clean and maintainable.Apply this diff to remove the unused import:
-import React, { type CSSProperties, useEffect, useRef, useState } from 'react'; +import React, { type CSSProperties, useRef, useState } from 'react';
88-90: Avoid redefiningtriggerRefby utilizing existing references.The
triggerRefis being created anew, but there might be an existing ref or a way to merge refs to avoid potential conflicts or unintended behavior.Consider checking if there's an existing ref that can be used or merged with
triggerRefto streamline the component's refs.
107-107: InitializeisEditingstate only when necessary.The
isEditingstate is introduced to handle inline editing of account names. Ensure that this state is only initialized when the account supports editing to prevent unnecessary state management.Consider wrapping the state initialization with a condition:
-const [isEditing, setIsEditing] = useState(false); +const [isEditing, setIsEditing] = useState(account ? false : null);
218-254: Refactor menu item generation to enhance readability and maintainability.The items array for the
Menucomponent can be extracted or generated more dynamically to make the code clearer and easier to maintain.Refactor the
itemsprop as follows:items={[ { name: 'rename', text: 'Rename' }, account.closed ? { name: 'reopen', text: 'Reopen' } : { name: 'close', text: 'Close' }, ]}Alternatively, you can define the items array outside of the JSX for clarity:
const menuItems = [ { name: 'rename', text: 'Rename' }, account.closed ? { name: 'reopen', text: 'Reopen' } : { name: 'close', text: 'Close' }, ]; // Then use it in the Menu component <Menu onMenuSelect={...} items={menuItems} />
15-15: Be cautious of potential conflicts with concurrent PRs modifyingAccount.tsx.According to retrieved learnings, the
Accountcomponent is being rewritten in a separate PR. Merging this PR might cause merge conflicts or override changes from the other PR.Consider coordinating with the authors of the other PRs to:
- Rebase your changes on top of the latest
Account.tsxupdates.- Collaborate to integrate the context menu enhancements into the rewritten component.
- Avoid duplication of effort and ensure a smooth merge process.
packages/desktop-client/src/components/common/Link.tsx (1)
38-38: Consider using ARIA attributes for disabled linksWhile adding the
isDisabledprop is a good start, consider enhancing accessibility by also addingaria-disabledwhen the link is disabled. This helps screen readers understand the link's state.Consider updating the implementation to include ARIA attributes:
type InternalLinkProps = { to?: string; style?: CSSProperties; activeStyle?: CSSProperties; children?: ReactNode; report?: CustomReportEntity; isDisabled?: boolean; + 'aria-disabled'?: boolean; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/desktop-client/src/components/common/Link.tsx(3 hunks)packages/desktop-client/src/components/sidebar/Account.tsx(5 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/Account.tsx (2)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
🔇 Additional comments (5)
packages/desktop-client/src/components/sidebar/Account.tsx (4)
105-106: Declare dispatch before its usage to prevent any scope issues.
Although dispatch is declared immediately before its usage, ensure that it's declared in the appropriate scope to avoid any potential scope-related bugs.
Please confirm that dispatch is accessible in all places where it's used within the component.
116-117: Ensure consistent handling of the context menu across components.
The onContextMenu handler is conditionally applied based on needsTooltip. Verify that this condition aligns with how other components handle context menus to maintain a consistent user experience.
Check if needsTooltip accurately reflects when the context menu should be available, and adjust if necessary.
295-295: Ensure tooltips are consistent when context menu is open.
Disabling the tooltip when the context menu is open may cause unexpected behavior. Verify that this interaction aligns with user expectations and platform guidelines.
Test the tooltip behavior when the context menu is open to ensure there are no conflicts or unintended side effects.
90-90: Avoid code duplication by reusing the useContextMenu hook.
Based on past review comments, there was a suggestion to create and reuse a useContextMenu hook to handle context menu logic across components. It appears you've imported useContextMenu here, which is great.
No further action needed as you've addressed the previous feedback.
packages/desktop-client/src/components/common/Link.tsx (1)
38-38: Verify integration with context menu items
The disabled link implementation looks good for context menu integration. Let's verify its usage in the context menus.
Also applies to: 125-125, 135-139
✅ Verification successful
Let's try a broader search to find the integration points and usage patterns of the Link component with context menus.
Let's check how the Menu components handle disabled items and Link components to verify the integration.
Integration with context menus is properly implemented
Based on the verification results, we can see that:
- The
useContextMenuhook is widely used across multiple components in the sidebar and other areas - The context menu system is feature-flagged and properly integrated
- The
isDisabledprop addition to the Link component aligns with the context menu integration pattern seen in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of InternalLink with isDisabled prop in context menu components
rg -l "InternalLink.*isDisabled"
# Search for context menu implementations that might need this functionality
ast-grep --pattern 'Menu {
$$$
<$_Link$_ $$$/>
$$$
}'
Length of output: 96
Script:
#!/bin/bash
# Search for usage of Link component with isDisabled prop
rg -A 2 "Link.*isDisabled"
# Search for context menu related files that might use Link
rg -l "Menu.*Link"
# Search for any context menu implementations
rg -A 5 "createContextMenu|useContextMenu"
Length of output: 13781
Script:
#!/bin/bash
# Let's search for Menu components that might use Link or InternalLink
rg -A 5 "<(Internal)?Link" packages/desktop-client/src/components/common/Menu.tsx
# Also search for any Menu component implementations with disabled items
rg -A 5 "isDisabled.*Menu|Menu.*isDisabled"
Length of output: 129
Add context menu's on the sidebar account names and the budget name at the top left.