fix(desktop): v2 file reveal — expand, highlight, scroll, open sidebar#4536
Conversation
- fix race in useFilesTabBridge.fetchDir where the Set-based dedup returned without awaiting the in-flight load; Pierre's synchronous expand-notify kicked off fetchDir before reveal's own await, so reveal resolved before children landed in knownPaths and only the first ancestor expanded - emulate selectOnlyPath via deselect + select on the item handles so the revealed row gets data-item-selected; works for folders too - replicate Pierre's sort + visibility math to compute the visible row index and set scrollTop directly, since Pierre auto-scrolls only when DOM focus lives inside the tree and exposes no public scrollTo API - open the right sidebar + switch to the Files tab when the user opens a file from Quick Open so the reveal is actually visible
|
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. |
|
Caution Review failedPull request was closed or merged during review 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 (1)
📝 WalkthroughWalkthroughEnhances Files tab selection and visibility: adds a virtualized-tree centering utility, dedupes concurrent directory loads, updates reveal() to select + focus + center rows, adjusts click interception, and wires Quick Open and sidebar tree-clicks to the new navigation callbacks. ChangesFile Selection and Reveal Enhancement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/FilesTab/hooks/useFilesTabBridge/useFilesTabBridge.ts:
- Around line 126-131: The finally block currently unconditionally calls
inflightDirsRef.current.delete(relDir) which can remove a newer promise when an
older request resolves; change the cleanup to only delete the entry if the
stored promise equals the local promise (the one assigned to the variable
promise inside the IIFE) so that inflightDirsRef retains newer in-flight
promises for the same relDir; update the finally in the function where
inflightDirsRef, relDir and promise are in scope to conditionally delete only
when inflightDirsRef.current.get(relDir) === promise.
🪄 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: 559a2391-f492-4f3a-9466-1ea4f3b85271
📒 Files selected for processing (5)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/hooks/useFilesTabActions/useFilesTabActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/hooks/useFilesTabBridge/useFilesTabBridge.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/utils/scrollTreeToRow/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/utils/scrollTreeToRow/scrollTreeToRow.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
Greptile SummaryThis PR fixes four bugs in the v2 file-reveal flow triggered by Quick Open (or any
Confidence Score: 3/5The four reveal fixes are directionally correct, but the Promise-Map cleanup in fetchDir has a race that can cause duplicate directory-listing fetches after a workspace switch. The inflightDirsRef finally block deletes by key rather than by promise identity, so a resolved stale promise from a previous workspace can silently evict a concurrently registered promise for the new workspace, undermining the deduplication guarantee the Map was introduced to provide. useFilesTabBridge.ts — the finally block in fetchDir needs an identity guard before deleting; scrollTreeToRow.ts — clientHeight may be stale when the sidebar CSS transition is still running.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/hooks/useFilesTabBridge/useFilesTabBridge.ts | Replaces Set-based in-flight dedup with a Promise-Map so concurrent callers await the same fetch; correctly fixes the reveal race, but the finally block unconditionally deletes by key rather than by promise identity, which can evict a newer workspace's promise on workspace switch. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/hooks/useFilesTabActions/useFilesTabActions.ts | Adds deselect-all + select emulation for visual highlight and delegates to scrollTreeToRow; logic is sound assuming getSelectedPaths() returns a snapshot array. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/utils/scrollTreeToRow/scrollTreeToRow.ts | New utility that replicates Pierre's sort + visibility walk to find a row's visible index and sets scrollTop directly; visibility logic is correct, but isPathVisible is called O(n) times with O(depth) work each, giving O(n×depth) total. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/utils/scrollTreeToRow/index.ts | Barrel export for scrollTreeToRow; no issues. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx | Adds handleQuickOpenSelectFile to open sidebar + switch to Files tab before delegating to openFilePane; straightforward and correct. |
Sequence Diagram
sequenceDiagram
participant QO as QuickOpen
participant Page as page.tsx
participant Actions as useFilesTabActions
participant Bridge as useFilesTabBridge
participant Pierre as FileTree (Pierre)
participant Scroll as scrollTreeToRow
QO->>Page: onSelectFile(filePath)
Page->>Page: setRightSidebarOpen(true)
Page->>Page: setRightSidebarTab("files")
Page->>Actions: openFilePane(filePath) reveal(absolutePath)
Actions->>Bridge: await fetchDir("") [root]
Bridge-->>Actions: root children loaded
loop for each ancestor segment
Actions->>Bridge: await fetchDir(parentRel)
Bridge-->>Actions: children loaded
Actions->>Pierre: handle.expand()
Note over Pierre,Bridge: expand() notifies model.subscribe synchronously
Bridge->>Bridge: fetchDir(dirRel) [returns same in-flight Promise]
Actions->>Bridge: await fetchDir(acc) [awaits same Promise]
Bridge-->>Actions: children in knownPaths
end
Actions->>Actions: requestAnimationFrame(...)
Actions->>Pierre: deselect all selected paths
Actions->>Pierre: getItem(targetKey).select()
Actions->>Pierre: focusPath(rel)
Actions->>Scroll: scrollTreeToRow(model, knownPaths, targetKey, rowHeight)
Scroll->>Pierre: getFileTreeContainer().shadowRoot.querySelector(...)
Scroll->>Scroll: prepareFileTreeInput + walk visible rows
Scroll->>Scroll: "scrollEl.scrollTop = centered position"
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/hooks/useFilesTabBridge/useFilesTabBridge.ts:126-128
**Stale promise can evict a newer promise from `inflightDirsRef` on workspace switch**
`inflightDirsRef.current.delete(relDir)` is unconditional, so if a stale P1 (from the previous workspace) resolves after the map was cleared and a new P2 was registered under the same key, P1's `finally` silently evicts P2. Subsequent concurrent callers then see no in-flight entry and start a redundant P3 fetch, bypassing the deduplication this Map was introduced to provide. The fix is to guard the delete with an identity check:
```ts
} finally {
if (inflightDirsRef.current.get(relDir) === promise) {
inflightDirsRef.current.delete(relDir);
}
}
```
### Issue 2 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/utils/scrollTreeToRow/scrollTreeToRow.ts:35-45
**`clientHeight` may be 0 when sidebar CSS transition is still running**
`handleQuickOpenSelectFile` opens the sidebar with a state update, then `openFilePane` → `reveal` awaits `fetchDir` calls. When all directories are already cached (`loadedDirs` hits), those awaits resolve in a single microtask tick, so the `requestAnimationFrame` can fire while the sidebar's open animation is mid-flight and `scrollEl.clientHeight` reads 0. The centering formula `targetTop - (viewportHeight - itemHeight) / 2` then becomes `targetTop + itemHeight/2`, placing the row near the top of the viewport rather than centered once the sidebar fully expands.
### Issue 3 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/utils/scrollTreeToRow/scrollTreeToRow.ts:49-62
**O(n × depth) visible-index walk on every scroll**
`computeVisibleRowIndex` calls `isPathVisible(path, model)` for every path before the target, and each `isPathVisible` call re-walks all ancestors of that path. For a workspace with thousands of files at several levels of depth this is measurably expensive per reveal, even inside `requestAnimationFrame`. A simple optimization is to track a `Set<string>` of directories already confirmed expanded and skip the ancestor walk for subsequent paths that share the same parent chain.
Reviews (1): Last reviewed commit: "fix(desktop): v2 file reveal — expand, h..." | Re-trigger Greptile
| } finally { | ||
| inflightDirsRef.current.delete(relDir); | ||
| } |
There was a problem hiding this comment.
Stale promise can evict a newer promise from
inflightDirsRef on workspace switch
inflightDirsRef.current.delete(relDir) is unconditional, so if a stale P1 (from the previous workspace) resolves after the map was cleared and a new P2 was registered under the same key, P1's finally silently evicts P2. Subsequent concurrent callers then see no in-flight entry and start a redundant P3 fetch, bypassing the deduplication this Map was introduced to provide. The fix is to guard the delete with an identity check:
} finally {
if (inflightDirsRef.current.get(relDir) === promise) {
inflightDirsRef.current.delete(relDir);
}
}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/components/FilesTab/hooks/useFilesTabBridge/useFilesTabBridge.ts
Line: 126-128
Comment:
**Stale promise can evict a newer promise from `inflightDirsRef` on workspace switch**
`inflightDirsRef.current.delete(relDir)` is unconditional, so if a stale P1 (from the previous workspace) resolves after the map was cleared and a new P2 was registered under the same key, P1's `finally` silently evicts P2. Subsequent concurrent callers then see no in-flight entry and start a redundant P3 fetch, bypassing the deduplication this Map was introduced to provide. The fix is to guard the delete with an identity check:
```ts
} finally {
if (inflightDirsRef.current.get(relDir) === promise) {
inflightDirsRef.current.delete(relDir);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| const viewportHeight = scrollEl.clientHeight; | ||
| const targetTop = visibleIndex * itemHeight; | ||
| const targetBottom = targetTop + itemHeight; | ||
| const currentTop = scrollEl.scrollTop; | ||
| const currentBottom = currentTop + viewportHeight; | ||
|
|
||
| if (targetTop >= currentTop && targetBottom <= currentBottom) return true; | ||
| scrollEl.scrollTop = Math.max( | ||
| 0, | ||
| targetTop - (viewportHeight - itemHeight) / 2, | ||
| ); |
There was a problem hiding this comment.
clientHeight may be 0 when sidebar CSS transition is still running
handleQuickOpenSelectFile opens the sidebar with a state update, then openFilePane → reveal awaits fetchDir calls. When all directories are already cached (loadedDirs hits), those awaits resolve in a single microtask tick, so the requestAnimationFrame can fire while the sidebar's open animation is mid-flight and scrollEl.clientHeight reads 0. The centering formula targetTop - (viewportHeight - itemHeight) / 2 then becomes targetTop + itemHeight/2, placing the row near the top of the viewport rather than centered once the sidebar fully expands.
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/components/FilesTab/utils/scrollTreeToRow/scrollTreeToRow.ts
Line: 35-45
Comment:
**`clientHeight` may be 0 when sidebar CSS transition is still running**
`handleQuickOpenSelectFile` opens the sidebar with a state update, then `openFilePane` → `reveal` awaits `fetchDir` calls. When all directories are already cached (`loadedDirs` hits), those awaits resolve in a single microtask tick, so the `requestAnimationFrame` can fire while the sidebar's open animation is mid-flight and `scrollEl.clientHeight` reads 0. The centering formula `targetTop - (viewportHeight - itemHeight) / 2` then becomes `targetTop + itemHeight/2`, placing the row near the top of the viewport rather than centered once the sidebar fully expands.
How can I resolve this? If you propose a fix, please make it concise.| function computeVisibleRowIndex( | ||
| targetKey: string, | ||
| knownPaths: ReadonlySet<string>, | ||
| model: FileTree, | ||
| ): number { | ||
| const prepared = prepareFileTreeInput(Array.from(knownPaths)); | ||
| let index = 0; | ||
| for (const path of prepared.paths) { | ||
| if (path === targetKey) { | ||
| return isPathVisible(path, model) ? index : -1; | ||
| } | ||
| if (isPathVisible(path, model)) index++; | ||
| } | ||
| return -1; |
There was a problem hiding this comment.
O(n × depth) visible-index walk on every scroll
computeVisibleRowIndex calls isPathVisible(path, model) for every path before the target, and each isPathVisible call re-walks all ancestors of that path. For a workspace with thousands of files at several levels of depth this is measurably expensive per reveal, even inside requestAnimationFrame. A simple optimization is to track a Set<string> of directories already confirmed expanded and skip the ancestor walk for subsequent paths that share the same parent chain.
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/components/FilesTab/utils/scrollTreeToRow/scrollTreeToRow.ts
Line: 49-62
Comment:
**O(n × depth) visible-index walk on every scroll**
`computeVisibleRowIndex` calls `isPathVisible(path, model)` for every path before the target, and each `isPathVisible` call re-walks all ancestors of that path. For a workspace with thousands of files at several levels of depth this is measurably expensive per reveal, even inside `requestAnimationFrame`. A simple optimization is to track a `Set<string>` of directories already confirmed expanded and skip the ancestor walk for subsequent paths that share the same parent chain.
How can I resolve this? If you propose a fix, please make it concise.- always intercept file-row clicks in usePierreRowClickPolicy instead of deferring plain+pane clicks to Pierre. Pierre's selectOnlyPath no-ops when the clicked row is already selected, so re-clicks (click-to-pin, reopen after Cmd+W) silently dropped through. Removing the defer restores the every-click-fires behavior the pre-Pierre tree had. - split openFilePane (focus-existing or openPane, no pin — used by the Quick Open picker and pane-registry openers) from openFilePaneFromTreeClick (pin-on-active wrapper — used by the sidebar tree). Picker pre-picks no longer pin-stick the active pane, while sidebar keeps the VS-Code click-again-to-pin gesture.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/clickPolicy/usePierreRowClickPolicy.ts (1)
37-40: ⚡ Quick winStale JSDoc contradicts new always-intercept behavior.
The doc block still states that "Unbound tiers and plain 'pane' defer to Pierre's own
onSelectionChange", which is the exact behavior the PR removed. The new inline comment at lines 83–86 correctly explains the always-intercept policy; please update this top-level JSDoc to match so future readers aren't misled.📝 Proposed JSDoc update
- * Unbound tiers and plain "pane" defer to Pierre's own `onSelectionChange` - * so the visual selection stays in sync; intercepting would swallow the - * click and leave Pierre out of date. + * When `filePolicy.resolve` returns a non-null `action`, the click is always + * intercepted (preventDefault + stopPropagation) rather than deferring to + * Pierre's `onSelectionChange`. Pierre's `selectOnlyPath` no-ops when the + * clicked row is already selected, which would otherwise silently drop + * legitimate re-clicks (click-to-pin, reopen after Cmd+W). The app drives + * selection explicitly via the reveal flow instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/lib/clickPolicy/usePierreRowClickPolicy.ts` around lines 37 - 40, Update the stale top-level JSDoc for usePierreRowClickPolicy to reflect the new always-intercept behavior: remove the line that says "Unbound tiers and plain 'pane' defer to Pierre's own `onSelectionChange`" and replace it with a short description that this hook always intercepts clicks (including unbound tiers and 'pane') and forwards selection changes through the hook instead of deferring to Pierre; keep or reference the existing inline comment at lines ~83–86 as supporting detail and ensure the JSDoc mentions the rationale that intercepting prevents Pierre from getting out of sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/desktop/src/renderer/lib/clickPolicy/usePierreRowClickPolicy.ts`:
- Around line 37-40: Update the stale top-level JSDoc for
usePierreRowClickPolicy to reflect the new always-intercept behavior: remove the
line that says "Unbound tiers and plain 'pane' defer to Pierre's own
`onSelectionChange`" and replace it with a short description that this hook
always intercepts clicks (including unbound tiers and 'pane') and forwards
selection changes through the hook instead of deferring to Pierre; keep or
reference the existing inline comment at lines ~83–86 as supporting detail and
ensure the JSDoc mentions the rationale that intercepting prevents Pierre from
getting out of sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb353d3b-9adf-47a0-acbd-12cf593e2596
📒 Files selected for processing (3)
apps/desktop/src/renderer/lib/clickPolicy/usePierreRowClickPolicy.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceFileNavigation/useWorkspaceFileNavigation.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
…sdoc fetchDir's finally was unconditionally deleting its in-flight map entry. On a workspace switch the map is cleared and a new promise can land under the same key, so a late-resolving stale promise would evict the live one and let subsequent callers start a redundant fetch. Identity-check the entry before deleting it. Update usePierreRowClickPolicy jsdoc to reflect the always-intercept policy (the previous "defer to Pierre's onSelectionChange" line was a leftover from before the fix).
The reveal flow programmatically selects the just-opened file's row in Pierre, which fires onSelectionChange synchronously. That echo re-entered onSelectFile → openFilePaneFromTreeClick, where active === target matched and pinned the pane we just opened. Result: every freshly opened pane came out pinned after its reveal completed. Guard the onSelect handler by skipping when treePath already matches selectedFilePath (the file we just opened). Real keyboard nav, which moves selection to a different path, still routes through onSelectFile.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Summary
Four fixes to v2 file reveal so opening a file from Quick Open (or any reveal trigger) actually shows the file in the tree:
useFilesTabBridge.fetchDirwhere theSet-based dedup returned immediately instead of awaiting the in-flight load. Pierre'shandle.expand()synchronously notifies subscribers, and ourmodel.subscribehook firesfetchDirbefore reveal's ownawait fetchDirruns — without shared promises, reveal resolved before children landed inknownPaths, so only the first ancestor expanded (apps/) and the rest silently no-op'd.focusPathonly setsdata-item-focused(keyboard focus). Visual row highlight isdata-item-selected.FileTreedoesn't exposeselectOnlyPath, so emulate it via deselect-all +handle.select(). Works for folders too (their handle has the sameselect()method).FileTreeViewshouldOwnDomFocusgate) and exposes no publicrevealPath/scrollTo. AddedscrollTreeToRowthat replicates Pierre's sort viaprepareFileTreeInput+ walks visible rows (ancestors-expanded check) to find the index, then setsscrollTopdirectly. Works regardless of virtualization window.openFilePanedirectly, so the sidebar/Files tab stayed closed and the reveal happened invisibly. AddedhandleQuickOpenSelectFilethat opens the sidebar + switches to Files before delegating. Tree clicks and other call sites still go throughopenFilePaneunchanged.Pierre upstream note: a public
revealPath/scrollToPath(or evengetFocusedIndex) would let us drop thescrollTreeToRowhelper. Worth filing.Test plan
Summary by cubic
Fixes v2 file reveal so opening a file from Quick Open (or any reveal) expands ancestors, highlights and scrolls the row into view, and shows the Files tab. Restores reliable tree click behavior: re-clicks fire, sidebar re-click pins, and Quick Open previews without pinning.
MapinuseFilesTabBridge.fetchDirso callers await the same fetch; add identity-check cleanup to avoid evicting a fresh promise after a workspace switch.select()+focusPath; works for files and folders.scrollTreeToRowusing@pierre/treesprepareFileTreeInput+ ancestor-expanded check to compute the visible index and center the row; independent of DOM focus/virtualization.handleQuickOpenSelectFileto open the right sidebar and switch to Files before opening the file.openFilePane(focus existing, no pin) fromopenFilePaneFromTreeClick(click-again-to-pin) so re-clicks and reopen-after-Cmd+W work and picker selections don’t pin-stick.onSelectionChangeecho when the selected row already matchesselectedFilePathto prevent auto-pinning newly opened panes; real keyboard nav still flows.Written for commit e2329f7. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes