Fix/contextmenu - #57
Merged
Merged
Conversation
…nents Split the sidebar into smaller modules (tree builder, section, folder item, connection item, modal) to reduce file complexity and improve readability. Constraint: Keep existing sidebar behavior unchanged during structural split
reworked menu/submenu placement with measured dimensions, portal rendering, and clamping so menus stay within viewport bounds and close cleanly on escape/outside interactions.
Lifted connection context-menu state from each ConnectionItem into Sidebar and render a single shared menu instance. This simplifies ownership and keeps row components lighter.
Added a sidebar-level search input that filters both active and non-active connections through the existing tree pipeline. Also moved connection count context into the search placeholder and added an explicit accessible label so assistive tech can announce intent. Constraint: Keep sidebar interactions and host actions unchanged while adding filter UX Rejected: Separate search row plus standalone count badge | increased visual clutter in compact sidebar Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep connection filtering centralized in Sidebar/buildTree and avoid duplicating ad-hoc filters in child rows Tested: npm run type-check Not-tested: Manual visual QA on all theme variants and narrow-width layouts
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 12 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRefactors the Sidebar into subcomponents, adds a top-of-sidebar connection search and filtered tree builder, centralizes a single connection context menu, implements viewport-aware ContextMenu/submenu positioning with measure-then-reveal, and bumps version to 2.10.1 with changelog updates. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User (drag/drop)
participant CI as ConnectionItem (UI)
participant IPC as window.ipcRenderer
participant S as Backend SFTP (tauri)
participant Store as App Store
U->>CI: drop server-file payload
CI->>CI: read dataTransfer (application/json)
CI->>IPC: invoke('sftp:cwd', { id: conn.id })
IPC->>S: request cwd
S-->>IPC: cwd result
IPC-->>CI: cwd result
CI->>Store: addTransfer(...) and show info toast
CI->>IPC: invoke('sftp:copyToServer', { src, dest, id })
IPC->>S: perform copy
S-->>IPC: success/error
IPC-->>CI: result
CI->>Store: mark success or call failTransfer and show error toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/ui/ContextMenu.tsx (2)
183-185: Consider using stable keys instead of array indices.Using array index as
keyfor menu items can cause unexpected behavior if the items array is reordered or filtered dynamically. IfContextMenuItemhas a unique identifier (likelabel), prefer using that.♻️ Suggested improvement
- {items.map((item, i) => ( - <MenuItem key={i} item={item} onClose={onClose} /> + {items.map((item, i) => ( + <MenuItem key={'separator' in item ? `sep-${i}` : item.label} item={item} onClose={onClose} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ContextMenu.tsx` around lines 183 - 185, The current items.map rendering uses the array index as the React key which can cause UI bugs when items are reordered/filtered; update the mapping in the ContextMenu rendering to use a stable unique identifier from each item (e.g., item.id or item.label) instead of the index: locate the items.map(...) that returns <MenuItem ... /> and replace key={i} with key={item.id ?? item.label} (or another unique property available on the ContextMenu item objects) so MenuItem receives a stable key.
163-169: Potential flash on rapid re-positioning.The
useLayoutEffectresetsreadyimplicitly by running on[x, y]changes, butsetReady(true)is called synchronously after measuring. If the menu is already open and coordinates change (e.g., context menu re-triggered), there's no explicitsetReady(false)before measuring, so the old position might briefly show before the new position is calculated.Consider resetting
readyat the start of the effect:♻️ Suggested improvement
useLayoutEffect(() => { if (!ref.current) return; + setReady(false); const { width, height } = ref.current.getBoundingClientRect(); setPos(calcPosition(x, y, width, height)); setReady(true); }, [x, y]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ContextMenu.tsx` around lines 163 - 169, The layout effect in ContextMenu that measures and then reveals (useLayoutEffect) can briefly show the old position because it never explicitly clears ready before recalculating; update the effect to call setReady(false) at the start (before reading ref.current.getBoundingClientRect()) so the component is hidden while you compute the new position, then keep the existing setPos(calcPosition(x, y, width, height)) and setReady(true) after measurement; reference the useLayoutEffect block that uses ref, setPos, calcPosition, setReady and depends on [x, y].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/layout/sidebar/ConnectionItem.tsx`:
- Around line 24-25: When initiating the optimistic transfer via addTransfer
before calling the IPC channel 'sftp:copyToServer', capture the transfer id
returned and, in the catch path for the ipc invoke, mark that transfer as failed
(e.g., call a store action like failTransfer(updateTransferStatus) with the id
and error details or set status:'failed' and record the error). Update both
occurrences (the current addTransfer usage and the similar block at lines 47-70)
so the transfer is given a terminal failed state when the IPC call rejects; if a
fail/update action does not exist in the app store, add one that accepts the
transfer id, status, and optional error message and use it here.
In `@src/components/layout/sidebar/FolderFormModal.tsx`:
- Line 17: The edit-mode check using isEditMode = initialName !== undefined
incorrectly treats an empty string as edit mode; update the detection to require
a non-empty initialName (e.g., set isEditMode to a truthy/non-empty check such
as initialName != null && initialName !== '' or Boolean(initialName) depending
on whether empty strings should be considered new) so that FolderFormModal and
the rename flow (folderToRename, initialName) only enter edit mode when there is
an actual name to edit.
In `@src/components/layout/sidebar/FolderItem.tsx`:
- Around line 53-59: The drop handler currently builds newName and calls
onMoveFolder even when the computed newName equals the original srcFolderPath (a
no-op), so update the else-if block that uses srcFolderPath, node.path and
newName to normalize paths (trim trailing slashes) and add a guard that returns
early when the resolved target path equals the source (i.e., if newName ===
srcFolderPath after normalization), before calling onMoveFolder; reference the
variables node.path, srcFolderPath, newName and the call to onMoveFolder.
---
Nitpick comments:
In `@src/components/ui/ContextMenu.tsx`:
- Around line 183-185: The current items.map rendering uses the array index as
the React key which can cause UI bugs when items are reordered/filtered; update
the mapping in the ContextMenu rendering to use a stable unique identifier from
each item (e.g., item.id or item.label) instead of the index: locate the
items.map(...) that returns <MenuItem ... /> and replace key={i} with
key={item.id ?? item.label} (or another unique property available on the
ContextMenu item objects) so MenuItem receives a stable key.
- Around line 163-169: The layout effect in ContextMenu that measures and then
reveals (useLayoutEffect) can briefly show the old position because it never
explicitly clears ready before recalculating; update the effect to call
setReady(false) at the start (before reading
ref.current.getBoundingClientRect()) so the component is hidden while you
compute the new position, then keep the existing setPos(calcPosition(x, y,
width, height)) and setReady(true) after measurement; reference the
useLayoutEffect block that uses ref, setPos, calcPosition, setReady and depends
on [x, y].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee0fd519-6830-4ec7-9260-1374b727bbaa
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
CHANGELOG.mdpackage.jsonsrc-tauri/Cargo.tomlsrc-tauri/tauri.conf.jsonsrc/components/layout/Sidebar.tsxsrc/components/layout/sidebar/ConnectionItem.tsxsrc/components/layout/sidebar/FolderFormModal.tsxsrc/components/layout/sidebar/FolderItem.tsxsrc/components/layout/sidebar/SidebarSection.tsxsrc/components/layout/sidebar/buildTree.tssrc/components/layout/sidebar/types.tssrc/components/ui/ContextMenu.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/layout/sidebar/ConnectionItem.tsx`:
- Around line 167-176: The icon-only edit button lacks an explicit accessible
name; add an aria-label to the button element (the one with onClick={(e) => {
e.stopPropagation(); onEdit(conn); }} and the Settings icon) such as
aria-label="Edit connection" so assistive technologies can identify it (keep the
existing title if desired but ensure aria-label is present on that button).
- Around line 91-122: The connection row div in ConnectionItem.tsx is not
keyboard accessible; update the element (the container where handlers like
onContextMenu, onDoubleClick, onDragStart, onDrop are attached) to be focusable
and respond to keyboard activation by adding tabIndex={0} and role="button" (or
convert it to a semantic <button>), and implement onKeyDown that maps
Enter/Space to call openTab(conn.id) and maps Shift+F10, 'ContextMenu', or the
Menu key to call onOpenContextMenu(conn, e.clientX, e.clientY) (use
clientX/clientY fallback when not provided); ensure onKeyDown also supports
Space preventing default to avoid page scroll. Keep existing drag handlers
(handleDragOver, handleDrop, setDropTargetId) intact.
In `@src/components/layout/sidebar/FolderFormModal.tsx`:
- Around line 79-85: Normalize and dedupe tags before storing and render/remove
by index or unique id instead of value: trim and lowercase incoming tagInput and
initialTags and dedupe them (e.g., via a Set) when initializing state
(initialTags -> normalized unique list) and when adding in the onKeyDown handler
(check against normalized tags), ensure you generate or use a stable unique key
per tag (e.g., assign an id or use index) instead of key={tag} to avoid
collisions, and change the removal handler (the function that currently removes
by value) to remove by index/id so only the intended tag instance is removed.
- Around line 22-27: The effect in FolderFormModal is rehydrating form state
whenever initialName/initialTags change, wiping in-progress edits; change the
effect so it only initializes form state when the modal transitions from closed
to open (isOpen becomes true) — e.g., track the previous isOpen with a ref and
run the setName/setTags/setTagInput initialization only when prevIsOpen is false
and isOpen is true, instead of depending on initialName/initialTags identities
while the modal is already open.
In `@src/components/layout/sidebar/FolderItem.tsx`:
- Around line 73-94: The folder row is currently a non-focusable div; make it
keyboard accessible by adding tabIndex={0}, role="button",
aria-expanded={isCollapsed ? false : true} (or the inverse depending on your
collapsed semantics), and an onKeyDown handler that listens for Enter and Space
to call toggleFolder(node.path) (preventDefault on Space), while preserving
existing draggable, onDragStart, onDragOver, onDragLeave (setIsDragOver) and
onDrop={handleDrop}; also consider adding an aria-label or using node.name for
screen reader context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2109315-e050-476d-b820-6e38c9beac1a
📒 Files selected for processing (4)
src/components/layout/sidebar/ConnectionItem.tsxsrc/components/layout/sidebar/FolderFormModal.tsxsrc/components/layout/sidebar/FolderItem.tsxsrc/components/ui/ContextMenu.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/ContextMenu.tsx
gajendraxdev
force-pushed
the
fix/contextmenu
branch
2 times, most recently
from
April 9, 2026 16:09
ee4808b to
50c12b7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/layout/sidebar/FolderFormModal.tsx (1)
45-45:⚠️ Potential issue | 🟠 MajorProvide an accessible name for the create-mode dialog.
The Modal component currently derives its accessible name only from the
titleprop. Passing an empty string""in create mode leaves the dialog unlabeled for screen readers. Although FolderFormModal displays a visual "New Folder" heading (line 57), it is not connected to the dialog's accessibility layer.The Modal component at
src/components/ui/Modal.tsxdoes not includearia-label,aria-labelledby, orrole="dialog"attributes on its dialog container, so screen readers have no way to announce the dialog's purpose whentitleis empty.Suggested fix
- <Modal isOpen={isOpen} onClose={onClose} title={isEditMode ? "Rename Folder" : ""}> + <Modal isOpen={isOpen} onClose={onClose} title={isEditMode ? "Rename Folder" : "Create Folder"}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/sidebar/FolderFormModal.tsx` at line 45, The dialog is unlabeled in create mode because FolderFormModal passes an empty title to Modal when isEditMode is false; update FolderFormModal to provide an explicit accessible name (e.g., "New Folder") instead of "" so the Modal receives a non-empty title for screen readers (ensure the passed title matches the visual heading inside FolderFormModal). Reference: FolderFormModal component using Modal with props isOpen, onClose and title, and the visual "New Folder" heading rendered when not isEditMode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/layout/sidebar/FolderItem.tsx`:
- Around line 125-150: The hover-only visibility on the controls div in
FolderItem.tsx (the element with className "flex opacity-0
group-hover:opacity-100 transition-opacity") prevents keyboard users from seeing
the rename/delete buttons; update the CSS utility classes to also reveal on
focus—for example add the focus variant (e.g. "group-focus-within:opacity-100"
or "focus-within:opacity-100" as appropriate) to that div's className so the
buttons become visible when the parent row receives keyboard focus, and confirm
the parent group element is focusable (has tabIndex or an interactive element)
so onRenameFolder and onDeleteFolder buttons are perceivable via keyboard.
In `@src/components/ui/ContextMenu.tsx`:
- Around line 157-166: The outside-click/scroll handlers (handleClickOutside,
handleScroll) treat portaled submenu panels as outside because they only check
ref.current.contains(...); update both handlers to also consider any portaled
submenu containers by checking whether the event target is contained within any
DOM nodes that host the portaled submenus (the portaled submenu panels rendered
around lines 298-315). Concretely, add a helper that queries for the submenu
portal container(s) (e.g., a stable selector/class/ data-attribute you add to
the portaled panels such as "context-menu-submenu-portal") and return true if
any of those containers contains(e.target as Node); use this helper in both
handleClickOutside and handleScroll before calling onClose so interactions
inside portaled submenus don’t close the menu prematurely.
---
Duplicate comments:
In `@src/components/layout/sidebar/FolderFormModal.tsx`:
- Line 45: The dialog is unlabeled in create mode because FolderFormModal passes
an empty title to Modal when isEditMode is false; update FolderFormModal to
provide an explicit accessible name (e.g., "New Folder") instead of "" so the
Modal receives a non-empty title for screen readers (ensure the passed title
matches the visual heading inside FolderFormModal). Reference: FolderFormModal
component using Modal with props isOpen, onClose and title, and the visual "New
Folder" heading rendered when not isEditMode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d73e76e-b169-4425-85f3-b6d5628a2f50
📒 Files selected for processing (4)
src/components/layout/sidebar/ConnectionItem.tsxsrc/components/layout/sidebar/FolderFormModal.tsxsrc/components/layout/sidebar/FolderItem.tsxsrc/components/ui/ContextMenu.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/layout/sidebar/ConnectionItem.tsx
gajendraxdev
force-pushed
the
fix/contextmenu
branch
from
April 9, 2026 16:36
50c12b7 to
a6947bb
Compare
Merged
Closed
gajendraxdev
added a commit
to gajendraxdev/zync
that referenced
this pull request
Jun 14, 2026
Fix/contextmenu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
New Components
Bug Fixes
Chores