[codex] Fix changes sidebar modifier click behavior#4644
Conversation
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds intent-based click policy for the changes sidebar (diff, diffNewTab, file, external), a row-click handler hook, UI wiring in tree/file rows and context menus, barrel re-exports, tests, and migration of legacy sidebar file-link defaults. ChangesChanges Sidebar Click Policy
Sequence DiagramsequenceDiagram
participant User
participant FileRow
participant usePierreChangesSidebarRowClickPolicy
participant useChangesSidebarFilePolicy
participant ActionHandler
User->>FileRow: click (with modifiers)
FileRow->>usePierreChangesSidebarRowClickPolicy: onClickCapture(event)
usePierreChangesSidebarRowClickPolicy->>useChangesSidebarFilePolicy: getIntent(event)
useChangesSidebarFilePolicy->>usePierreChangesSidebarRowClickPolicy: intent
usePierreChangesSidebarRowClickPolicy->>ActionHandler: call openInExternalEditor / onOpenFile / onSelectDiff
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit 7324c93 on May 16, 2026 9:38pm UTC. |
Greptile SummaryThis PR introduces a centralized
Confidence Score: 4/5Safe to merge; the new click routing is well-isolated and the legacy migration is correctly gated on an exact map match. The centralized intent layer and migration logic are solid. Two edge cases worth watching: file-row clicks always consume the event before checking intent (unlike folder rows), so an unbound modifier tier silently swallows the click; and in FileRow, a "file" intent with no absolutePath falls through all branches without opening the diff as a fallback, which could surprise any caller that omits worktreePath. usePierreChangesSidebarRowClickPolicy.ts (event-consumption asymmetry) and FileRow.tsx (silent no-op when worktreePath is absent).
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/clickPolicy/policies/changesSidebarFilePolicy.ts | New policy module mapping LinkTierMap actions to ChangesSidebarFileIntent values; logic is clean and the "pane"-on-non-plain-tier → "file" pivot is well-isolated. |
| apps/desktop/src/renderer/lib/clickPolicy/policies/changesSidebarFilePolicy.test.ts | Test coverage is thorough: plain, shift, cmd/ctrl, cmd/ctrl-shift, and shortcut-tier lookups are all exercised. |
| apps/desktop/src/renderer/lib/clickPolicy/policies/useChangesSidebarFilePolicy.ts | Straightforward hook that composes useSidebarFilePolicy with the new intent helpers; memoisation is correct. |
| apps/desktop/src/renderer/lib/clickPolicy/usePierreChangesSidebarRowClickPolicy.ts | New click policy hook correctly intercepts Pierre tree clicks; minor asymmetry: file rows unconditionally consume the event even for null intent, while folder rows return early without consuming. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts | Legacy migration is correct (exact-match guard prevents touching custom configs); the pre-computed sidebarFileLinks variable that gets discarded on migration is harmless but slightly redundant. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.test.ts | New test validates that the exact legacy default is migrated to the new default; existing heal tests unchanged. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/ChangesTreeView.tsx | Clean swap to usePierreChangesSidebarRowClickPolicy; onOpenFile now correctly resolves relative → absolute paths before forwarding to parent. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/components/FileRowContextMenuItems/FileRowContextMenuItems.tsx | Policy switch from useSidebarFilePolicy to useChangesSidebarFilePolicy is correct; new "Open File" shortcut label wired up properly. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/FileRow/FileRow.tsx | Intent-based dispatch correctly opens file/diff/editor; silent no-op when intent is "file" and absolutePath is undefined may be unexpected for callers that omit worktreePath. |
| apps/desktop/src/renderer/lib/clickPolicy/index.ts | New exports are clean and consistent with existing patterns. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Mouse click on sidebar row] --> B{capture phase onClickCapture}
B --> C{data-item-path found?}
C -- No --> D[pass through]
C -- Yes --> E{path ends with /}
E -- Yes folder --> F[folderIntentFor]
F -- null --> G[return early - event NOT consumed]
F -- external --> H[preventDefault + stopPropagation + openInExternalEditor]
E -- No file --> I[preventDefault + stopPropagation always]
I --> J[getFileIntent - resolveChangesSidebarFileIntent]
J -- diff --> K[onSelectDiff openInNewTab=false]
J -- diffNewTab --> L[onSelectDiff openInNewTab=true]
J -- file --> M[onOpenFile abs path via worktreePath]
J -- external --> N[openInExternalEditor]
J -- null --> O[no-op - event already consumed]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/lib/clickPolicy/usePierreChangesSidebarRowClickPolicy.ts:58-65
**Silent click consumption for unbound file tiers**
For file rows, `e.preventDefault()` and `e.stopPropagation()` are called unconditionally _before_ `getFileIntent(e)` is evaluated. When `getFileIntent` returns `null` (unbound tier), the click event is fully consumed but no action is taken. In contrast, the folder branch does the opposite — it returns early without consuming the event when `folderIntentFor` returns null, allowing the tree to handle expansion/collapse normally. A user who explicitly unbinds a modifier tier for files will get a silent no-op with no visual feedback, while the same situation on a folder correctly falls through to the tree.
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/FileRow/FileRow.tsx:107-113
**`"file"` intent silently falls through when `absolutePath` is undefined**
When `intent === "file"` and `absolutePath` is `undefined` (i.e. `worktreePath` was not passed), the condition `intent === "file" && absolutePath` is falsy and all remaining `else if` branches also miss, leaving the click as a complete no-op. Under the old default (`meta: "external"`), Cmd/Ctrl-click reliably called `onOpenInEditor?.(file.path)` regardless of `worktreePath`. With the new default (`meta: "pane"` → `"file"` intent), any caller that omits `worktreePath` silently loses modifier-click behaviour after the legacy migration fires.
Reviews (1): Last reviewed commit: "Fix changes sidebar modifier click polic..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
|
|
||
| const intent = getFileIntent(e); | ||
| if (intent === "external") openInExternalEditor(trimmed); | ||
| else if (intent === "file") onOpenFile(trimmed, false); | ||
| else if (intent === "diffNewTab") onSelectDiff(trimmed, true); | ||
| else if (intent === "diff") onSelectDiff(trimmed, false); |
There was a problem hiding this comment.
For file rows, e.preventDefault() and e.stopPropagation() are called unconditionally before getFileIntent(e) is evaluated. When getFileIntent returns null (unbound tier), the click event is fully consumed but no action is taken. In contrast, the folder branch does the opposite — it returns early without consuming the event when folderIntentFor returns null, allowing the tree to handle expansion/collapse normally. A user who explicitly unbinds a modifier tier for files will get a silent no-op with no visual feedback, while the same situation on a folder correctly falls through to the tree.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/clickPolicy/usePierreChangesSidebarRowClickPolicy.ts
Line: 58-65
Comment:
**Silent click consumption for unbound file tiers**
For file rows, `e.preventDefault()` and `e.stopPropagation()` are called unconditionally _before_ `getFileIntent(e)` is evaluated. When `getFileIntent` returns `null` (unbound tier), the click event is fully consumed but no action is taken. In contrast, the folder branch does the opposite — it returns early without consuming the event when `folderIntentFor` returns null, allowing the tree to handle expansion/collapse normally. A user who explicitly unbinds a modifier tier for files will get a silent no-op with no visual feedback, while the same situation on a folder correctly falls through to the tree.
How can I resolve this? If you propose a fix, please make it concise.| const intent = policy.getIntent(e); | ||
| if (intent === "external") onOpenInEditor?.(file.path); | ||
| else if (intent === "file" && absolutePath) | ||
| onOpenFile?.(absolutePath, false); | ||
| else if (intent === "diffNewTab") onSelect?.(file.path, true); | ||
| else if (intent === "diff") onSelect?.(file.path, false); | ||
| }} |
There was a problem hiding this comment.
"file" intent silently falls through when absolutePath is undefined
When intent === "file" and absolutePath is undefined (i.e. worktreePath was not passed), the condition intent === "file" && absolutePath is falsy and all remaining else if branches also miss, leaving the click as a complete no-op. Under the old default (meta: "external"), Cmd/Ctrl-click reliably called onOpenInEditor?.(file.path) regardless of worktreePath. With the new default (meta: "pane" → "file" intent), any caller that omits worktreePath silently loses modifier-click behaviour after the legacy migration fires.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/FileRow/FileRow.tsx
Line: 107-113
Comment:
**`"file"` intent silently falls through when `absolutePath` is undefined**
When `intent === "file"` and `absolutePath` is `undefined` (i.e. `worktreePath` was not passed), the condition `intent === "file" && absolutePath` is falsy and all remaining `else if` branches also miss, leaving the click as a complete no-op. Under the old default (`meta: "external"`), Cmd/Ctrl-click reliably called `onOpenInEditor?.(file.path)` regardless of `worktreePath`. With the new default (`meta: "pane"` → `"file"` intent), any caller that omits `worktreePath` silently loses modifier-click behaviour after the legacy migration fires.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
* Fix changes sidebar modifier click policy * Respect unbound changes sidebar file click tiers
1 participant
Summary
Impact
Plain clicks still open diffs, Shift-click still opens diffs in a new tab, and existing settings remain the source of truth for modifier behavior. Menu shortcut labels and hover hints now come from the same centralized policy.
Validation
bun test apps/desktop/src/renderer/lib/clickPolicy/policies/changesSidebarFilePolicy.test.ts apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.test.tsbun run typecheck --filter=@superset/desktopbun run lint:fixbun run lintgit diff --checkSummary by cubic
Centralized the changes sidebar click policy and updated the tree and row components to use it. Hover hints and menu shortcuts now come from one source, and unbound tiers are respected.
Bug Fixes
Migration
sidebarFileLinksdefaults to the new default (meta: pane,metaShift: external) without changing custom mappings.Written for commit 7324c93. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Chores
Tests