Repository navigation
fix(desktop): reuse Pierre for large changes lists - #5782
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe Changes sidebar now renders tree and folder modes through a shared Pierre-backed view with path projection, translated interactions, capped host sizing, and updated git status mapping. Supporting documentation records renderer-churn procedures and latest memory findings, while Pierre dependencies are upgraded. ChangesChanges sidebar projection and integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ChangesFileList
participant ChangesTreeView
participant buildPierreProjection
participant PierreTree
ChangesFileList->>ChangesTreeView: pass files and viewMode
ChangesTreeView->>buildPierreProjection: project file paths
buildPierreProjection-->>ChangesTreeView: return projected paths and lookup maps
ChangesTreeView->>PierreTree: render projected paths and git status
PierreTree->>ChangesTreeView: return selected tree path
ChangesTreeView->>ChangesFileList: translate selection to file path
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Ready to review this PR? Stage has broken it down into 6 individual chapters for you: 
Chapters generated by Stage for commit ac3c539 on Jul 19, 2026 10:30pm UTC. |
Kitenite
force-pushed
the
agent/renderer-churn-current-main
branch
from
July 19, 2026 19:00
e9032b2 to
2f2ef33
Compare
Kitenite
changed the title
Kitenite
marked this pull request as ready for review
July 19, 2026 21:56
![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: 2
🤖 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/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx:
- Around line 89-92: Add the relative positioning utility to the scroll
container div in ChangesFileList, preserving its existing flex, overflow,
spacing, and data attribute configuration so child virtualizers calculate
offsetTop against this container.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsx:
- Around line 110-120: Fix the virtualizer setup in ChangesFoldersView so
scrollMargin and the scroll element are initialized and refreshed reactively
rather than relying on the mount-time listRef value. Track the element’s
offsetTop in React state, observe the relevant layout changes with
ResizeObserver (including sibling-driven repositioning), and update the state
when the element moves or resizes; ensure the virtualizer receives the current
margin and attaches to the scroll container once available.
🪄 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: c2c2b7fe-70bd-4e2b-84fb-ffb9a5ff26dc
⛔ Files ignored due to path filters (1)
apps/desktop/docs/artifacts/renderer-churn-visible-lifecycle-checkpoints.mp4is excluded by!**/*.mp4
📒 Files selected for processing (4)
apps/desktop/docs/MEMORY_AND_WORKERS_REVIEW.mdapps/desktop/docs/RENDERER_CHURN_UI_PROFILE_RUNBOOK.mdapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsx
Comment on lines
+89
to
+92
| <div | ||
| className="min-h-0 flex-1 space-y-2 overflow-y-auto pt-1" | ||
| data-changes-scroll-container | ||
| > |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Add relative positioning to establish the correct offsetParent.
Without relative positioning on this scroll container, listRef.current?.offsetTop in child virtualizers will be incorrectly calculated against a higher-level positioned ancestor (e.g., the sidebar root or document).
This introduces a permanent misalignment between the physical scroll position and the virtualizer's viewport math, causing it to unmount visible rows prematurely and leaving large blank gaps when scrolling.
🐛 Proposed fix
<div
- className="min-h-0 flex-1 space-y-2 overflow-y-auto pt-1"
+ className="min-h-0 flex-1 space-y-2 overflow-y-auto pt-1 relative"
data-changes-scroll-container
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div | |
| className="min-h-0 flex-1 space-y-2 overflow-y-auto pt-1" | |
| data-changes-scroll-container | |
| > | |
| <div | |
| className="min-h-0 flex-1 space-y-2 overflow-y-auto pt-1 relative" | |
| data-changes-scroll-container | |
| > |
🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx
around lines 89 - 92, Add the relative positioning utility to the scroll
container div in ChangesFileList, preserving its existing flex, overflow,
spacing, and data attribute configuration so child virtualizers calculate
offsetTop against this container.
Contributor
Greptile SummaryThis PR virtualizes the v2
Confidence Score: 4/5Virtualization is a real improvement but the scrollMargin computation is incorrect and may render wrong items in multi-section layouts. The core useVirtualizer wiring is correct and the benchmark confirms the freeze is gone. The scrollMargin uses the wrong coordinate system and is null on first render, which can produce incorrect visible-range calculations for non-first sections. ChangesFoldersView.tsx — the scrollMargin computation at line 119 needs to be replaced with a layout-effect-based BoundingClientRect calculation relative to the scroll container.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsx | Core virtualization change; correctly flattens folder/file rows and bounds mounted DOM to the visible slice, but scrollMargin is stale on first render and references the wrong coordinate system after mount. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx | Adds data-changes-scroll-container to the scroll wrapper so ChangesFoldersView can locate the scroll element via closest(); straightforward and correct. |
| apps/desktop/docs/MEMORY_AND_WORKERS_REVIEW.md | Documentation update with before/after churn rerun results and updated shipping status for companion PRs. |
| apps/desktop/docs/RENDERER_CHURN_UI_PROFILE_RUNBOOK.md | New runbook documenting the reproducible synthetic profiling procedure. Documentation only. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
SCR["div[data-changes-scroll-container]\n(overflow-y-auto, flex-1)"]
SCR --> CS1["ChangesSection (unstaged)"]
SCR --> CS2["ChangesSection (staged)"]
CS1 --> CFV1["ChangesFoldersView (listRef div)"]
CS2 --> CFV2["ChangesFoldersView (listRef div)"]
CFV1 --> VIRT1["useVirtualizer\nscrollMargin: listRef.current?.offsetTop ?? 0"]
CFV2 --> VIRT2["useVirtualizer\nscrollMargin: listRef.current?.offsetTop ?? 0"]
VIRT1 --> ISSUE["null ref on first render → 0\noffsetTop relative to offsetParent\nnot scroll container"]
VIRT2 --> ISSUE
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
SCR["div[data-changes-scroll-container]\n(overflow-y-auto, flex-1)"]
SCR --> CS1["ChangesSection (unstaged)"]
SCR --> CS2["ChangesSection (staged)"]
CS1 --> CFV1["ChangesFoldersView (listRef div)"]
CS2 --> CFV2["ChangesFoldersView (listRef div)"]
CFV1 --> VIRT1["useVirtualizer\nscrollMargin: listRef.current?.offsetTop ?? 0"]
CFV2 --> VIRT2["useVirtualizer\nscrollMargin: listRef.current?.offsetTop ?? 0"]
VIRT1 --> ISSUE["null ref on first render → 0\noffsetTop relative to offsetParent\nnot scroll container"]
VIRT2 --> ISSUE
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/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsx:110-120
**Stale and misreferenced `scrollMargin` on initial render**
`scrollMargin: listRef.current?.offsetTop ?? 0` has two compounding problems. On the first render pass, `listRef.current` is `null` so `scrollMargin` starts as `0` for every group. For groups physically far from the top of the scroll container (e.g. the "Staged" section when "Unstaged" is large), this places the first ~viewport-height items of that group in the visible range even though they are off-screen, partially defeating the fix for non-first groups until the next scroll event.
After mount, `element.offsetTop` is the distance to the element's `offsetParent`. Because the scroll container (`overflow-y-auto`) has no `position` set, its `offsetParent` is the nearest positioned ancestor above it (likely the sidebar shell or `body`). That reference point is in a different coordinate system than the `scrollTop` the virtualizer reads from the scroll container. The correct offset is `listRef.current.getBoundingClientRect().top - scrollEl.getBoundingClientRect().top + scrollEl.scrollTop`. The 8-row overscan buffers small mismatches, but the error grows with layout depth and the null-ref issue recurs on every mount.
The standard fix is a `useLayoutEffect` that runs after mount and re-runs when the container resizes, storing the computed margin in state and passing it to `useVirtualizer`.
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsx:1
`defaultRangeExtractor` is the library's built-in default, so importing and explicitly passing it is a no-op. The import can be dropped along with the explicit `rangeExtractor` option to reduce the bundle and avoid misleading readers into thinking a custom extractor is active.
```suggestion
import { useVirtualizer } from "@tanstack/react-virtual";
```
Reviews (1): Last reviewed commit: "docs(desktop): record final latest-main ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+110
to
+120
| const virtualizer = useVirtualizer({ | ||
| count: rows.length, | ||
| getScrollElement: () => | ||
| listRef.current?.closest( | ||
| "[data-changes-scroll-container]", | ||
| ) as HTMLElement | null, | ||
| estimateSize: () => ESTIMATED_ROW_HEIGHT, | ||
| rangeExtractor: defaultRangeExtractor, | ||
| overscan: OVERSCAN, | ||
| scrollMargin: listRef.current?.offsetTop ?? 0, | ||
| }); |
Contributor
There was a problem hiding this comment.
scrollMargin on initial render
scrollMargin: listRef.current?.offsetTop ?? 0 has two compounding problems. On the first render pass, listRef.current is null so scrollMargin starts as 0 for every group. For groups physically far from the top of the scroll container (e.g. the "Staged" section when "Unstaged" is large), this places the first ~viewport-height items of that group in the visible range even though they are off-screen, partially defeating the fix for non-first groups until the next scroll event.
After mount, element.offsetTop is the distance to the element's offsetParent. Because the scroll container (overflow-y-auto) has no position set, its offsetParent is the nearest positioned ancestor above it (likely the sidebar shell or body). That reference point is in a different coordinate system than the scrollTop the virtualizer reads from the scroll container. The correct offset is listRef.current.getBoundingClientRect().top - scrollEl.getBoundingClientRect().top + scrollEl.scrollTop. The 8-row overscan buffers small mismatches, but the error grows with layout depth and the null-ref issue recurs on every mount.
The standard fix is a useLayoutEffect that runs after mount and re-runs when the container resizes, storing the computed margin in state and passing it to useVirtualizer.
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/ChangesFoldersView/ChangesFoldersView.tsx
Line: 110-120
Comment:
**Stale and misreferenced `scrollMargin` on initial render**
`scrollMargin: listRef.current?.offsetTop ?? 0` has two compounding problems. On the first render pass, `listRef.current` is `null` so `scrollMargin` starts as `0` for every group. For groups physically far from the top of the scroll container (e.g. the "Staged" section when "Unstaged" is large), this places the first ~viewport-height items of that group in the visible range even though they are off-screen, partially defeating the fix for non-first groups until the next scroll event.
After mount, `element.offsetTop` is the distance to the element's `offsetParent`. Because the scroll container (`overflow-y-auto`) has no `position` set, its `offsetParent` is the nearest positioned ancestor above it (likely the sidebar shell or `body`). That reference point is in a different coordinate system than the `scrollTop` the virtualizer reads from the scroll container. The correct offset is `listRef.current.getBoundingClientRect().top - scrollEl.getBoundingClientRect().top + scrollEl.scrollTop`. The 8-row overscan buffers small mismatches, but the error grows with layout depth and the null-ref issue recurs on every mount.
The standard fix is a `useLayoutEffect` that runs after mount and re-runs when the container resizes, storing the computed margin in state and passing it to `useVirtualizer`.
How can I resolve this? If you propose a fix, please make it concise.| @@ -1,3 +1,4 @@ | |||
| import { defaultRangeExtractor, useVirtualizer } from "@tanstack/react-virtual"; | |||
Contributor
There was a problem hiding this comment.
defaultRangeExtractor is the library's built-in default, so importing and explicitly passing it is a no-op. The import can be dropped along with the explicit rangeExtractor option to reduce the bundle and avoid misleading readers into thinking a custom extractor is active.
Suggested change
| import { defaultRangeExtractor, useVirtualizer } from "@tanstack/react-virtual"; | |
| import { useVirtualizer } from "@tanstack/react-virtual"; |
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/ChangesFoldersView/ChangesFoldersView.tsx
Line: 1
Comment:
`defaultRangeExtractor` is the library's built-in default, so importing and explicitly passing it is a no-op. The import can be dropped along with the explicit `rangeExtractor` option to reduce the bundle and avoid misleading readers into thinking a custom extractor is active.
```suggestion
import { useVirtualizer } from "@tanstack/react-virtual";
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Kitenite
changed the title
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/hooks/useMeasuredTreeHeight/useMeasuredTreeHeight.ts (1)
39-42: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winCancel the previous animation frame to prevent callback pileup.
During high file-change churn,
model.subscribecan fire many times within a single frame. Without cancelling the previous frame, this schedules hundreds or thousands of concurrentrequestAnimationFramecallbacks that all execute on the next frame, contributing to event-loop delay and rendering jitter.⚡ Proposed fix
const unsubscribe = model.subscribe(() => { + cancelAnimationFrame(raf); raf = requestAnimationFrame(readHeight); });🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/hooks/useMeasuredTreeHeight/useMeasuredTreeHeight.ts around lines 39 - 42, Update the model subscription callback in useMeasuredTreeHeight to cancel any previously scheduled animation frame before assigning a new requestAnimationFrame(readHeight). Preserve the existing cleanup behavior, ensuring the latest scheduled frame remains active and the returned unsubscribe cleanup still handles teardown.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx (1)
88-91: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
relativepositioning to establish the correctoffsetParent.Without
relativepositioning on this scroll container,listRef.current?.offsetTopin child virtualizers will be incorrectly calculated against a higher-level positioned ancestor (e.g., the sidebar root or document).This introduces a permanent misalignment between the physical scroll position and the virtualizer's viewport math, causing it to unmount visible rows prematurely and leaving large blank gaps when scrolling.
🐛 Proposed fix
<div - className="min-h-0 flex-1 space-y-2 overflow-y-auto pt-1" + className="min-h-0 flex-1 space-y-2 overflow-y-auto pt-1 relative" data-changes-scroll-container >🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx around lines 88 - 91, Add the `relative` positioning class to the scroll container `<div>` in `ChangesFileList`, preserving its existing flex, spacing, overflow, and padding classes so child virtualizers calculate `listRef.current?.offsetTop` against this container.
🤖 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/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/ChangesTreeView.tsx:
- Around line 428-438: Replace the flatMap implementation in
buildPierreGitStatus with a simple loop that reuses a single result array,
appending entries only when treePathByFilePath contains a path. Preserve the
existing path and status mapping behavior while avoiding per-file temporary
arrays during updates.
- Around line 281-286: Normalize treePath by removing trailing slashes before
the fileByPath and directoryPathByTreePath lookups in openInExternalEditor. Use
the normalized path for both maps while preserving the existing realPath
selection and onOpenInEditor behavior.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/utils/buildPierreProjection/buildPierreProjection.ts:
- Around line 72-73: The file comparator in buildPierreProjection.ts currently
sorts files descending; change it to compare left.basename against
right.basename so files use A-Z order. Update the expected ordering in
buildPierreProjection.test.ts lines 87-90 to [file("a.ts"), file("z.ts")] to
match the corrected behavior.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/hooks/useMeasuredTreeHeight/useMeasuredTreeHeight.ts:
- Around line 39-42: Update the model subscription callback in
useMeasuredTreeHeight to cancel any previously scheduled animation frame before
assigning a new requestAnimationFrame(readHeight). Preserve the existing cleanup
behavior, ensuring the latest scheduled frame remains active and the returned
unsubscribe cleanup still handles teardown.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx:
- Around line 88-91: Add the `relative` positioning class to the scroll
container `<div>` in `ChangesFileList`, preserving its existing flex, spacing,
overflow, and padding classes so child virtualizers calculate
`listRef.current?.offsetTop` against this container.
🪄 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: 8d664b18-9a0b-4584-a9d4-a4790ed2acec
⛔ Files ignored due to path filters (2)
apps/desktop/docs/artifacts/renderer-churn-visible-lifecycle-checkpoints.mp4is excluded by!**/*.mp4bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
apps/desktop/docs/MEMORY_AND_WORKERS_REVIEW.mdapps/desktop/docs/RENDERER_CHURN_UI_PROFILE_RUNBOOK.mdapps/desktop/package.jsonapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/components/FolderHeader/FolderHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/components/FolderHeader/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/ChangesTreeView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/hooks/useMeasuredTreeHeight/useMeasuredTreeHeight.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/utils/buildPierreProjection/buildPierreProjection.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/utils/buildPierreProjection/buildPierreProjection.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/utils/buildPierreProjection/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/FileRow/FileRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/FileRow/index.ts
💤 Files with no reviewable changes (6)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/components/FolderHeader/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/components/FolderHeader/FolderHeader.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/FileRow/FileRow.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/FileRow/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/docs/RENDERER_CHURN_UI_PROFILE_RUNBOOK.md
- apps/desktop/docs/MEMORY_AND_WORKERS_REVIEW.md
Comment on lines
+281
to
286
| openInExternalEditor: (treePath) => { | ||
| const filePath = fileByPath.get(treePath)?.path; | ||
| const directoryPath = projection.directoryPathByTreePath.get(treePath); | ||
| const realPath = filePath ?? directoryPath; | ||
| if (realPath !== undefined) onOpenInEditor?.(realPath); | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Strip trailing slashes from treePath before map lookups.
When a directory row triggers this action (e.g., via middle-click), Pierre may pass the treePath with a trailing slash. Because directoryPathByTreePath keys do not contain trailing slashes, the lookup will miss and the action will silently fail.
🐛 Proposed fix
- openInExternalEditor: (treePath) => {
- const filePath = fileByPath.get(treePath)?.path;
- const directoryPath = projection.directoryPathByTreePath.get(treePath);
+ openInExternalEditor: (rawTreePath) => {
+ const treePath = stripTrailingSlash(rawTreePath);
+ const filePath = fileByPath.get(treePath)?.path;
+ const directoryPath = projection.directoryPathByTreePath.get(treePath);
const realPath = filePath ?? directoryPath;
if (realPath !== undefined) onOpenInEditor?.(realPath);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| openInExternalEditor: (treePath) => { | |
| const filePath = fileByPath.get(treePath)?.path; | |
| const directoryPath = projection.directoryPathByTreePath.get(treePath); | |
| const realPath = filePath ?? directoryPath; | |
| if (realPath !== undefined) onOpenInEditor?.(realPath); | |
| }, | |
| openInExternalEditor: (rawTreePath) => { | |
| const treePath = stripTrailingSlash(rawTreePath); | |
| const filePath = fileByPath.get(treePath)?.path; | |
| const directoryPath = projection.directoryPathByTreePath.get(treePath); | |
| const realPath = filePath ?? directoryPath; | |
| if (realPath !== undefined) onOpenInEditor?.(realPath); | |
| }, |
🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/ChangesTreeView.tsx
around lines 281 - 286, Normalize treePath by removing trailing slashes before
the fileByPath and directoryPathByTreePath lookups in openInExternalEditor. Use
the normalized path for both maps while preserving the existing realPath
selection and onOpenInEditor behavior.
Comment on lines
+428
to
438
| function buildPierreGitStatus( | ||
| files: ChangesetFile[], | ||
| treePathByFilePath: Map<string, string>, | ||
| ): PierreGitStatusEntry[] { | ||
| return files.flatMap((file) => { | ||
| const treePath = treePathByFilePath.get(file.path); | ||
| return treePath | ||
| ? [{ path: treePath, status: FILE_STATUS_TO_PIERRE[file.status] }] | ||
| : []; | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Optimize buildPierreGitStatus to reduce GC pressure during churn.
Using flatMap with an array of 20,000 files creates 20,000 short-lived, single-item arrays on every update, generating significant memory churn. Given the PR's focus on reducing renderer RSS and event-loop delays during high churn, switching to a simple loop avoids this allocation overhead.
⚡ Proposed fix
function buildPierreGitStatus(
files: ChangesetFile[],
treePathByFilePath: Map<string, string>,
): PierreGitStatusEntry[] {
- return files.flatMap((file) => {
- const treePath = treePathByFilePath.get(file.path);
- return treePath
- ? [{ path: treePath, status: FILE_STATUS_TO_PIERRE[file.status] }]
- : [];
- });
+ const entries: PierreGitStatusEntry[] = [];
+ for (const file of files) {
+ const treePath = treePathByFilePath.get(file.path);
+ if (treePath) {
+ entries.push({ path: treePath, status: FILE_STATUS_TO_PIERRE[file.status] });
+ }
+ }
+ return entries;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function buildPierreGitStatus( | |
| files: ChangesetFile[], | |
| treePathByFilePath: Map<string, string>, | |
| ): PierreGitStatusEntry[] { | |
| return files.flatMap((file) => { | |
| const treePath = treePathByFilePath.get(file.path); | |
| return treePath | |
| ? [{ path: treePath, status: FILE_STATUS_TO_PIERRE[file.status] }] | |
| : []; | |
| }); | |
| } | |
| function buildPierreGitStatus( | |
| files: ChangesetFile[], | |
| treePathByFilePath: Map<string, string>, | |
| ): PierreGitStatusEntry[] { | |
| const entries: PierreGitStatusEntry[] = []; | |
| for (const file of files) { | |
| const treePath = treePathByFilePath.get(file.path); | |
| if (treePath) { | |
| entries.push({ path: treePath, status: FILE_STATUS_TO_PIERRE[file.status] }); | |
| } | |
| } | |
| return entries; | |
| } |
🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/ChangesTreeView.tsx
around lines 428 - 438, Replace the flatMap implementation in
buildPierreGitStatus with a simple loop that reuses a single result array,
appending entries only when treePathByFilePath contains a path. Preserve the
existing path and status mapping behavior while avoiding per-file temporary
arrays during updates.
Comment on lines
+72
to
+73
| return right.basename.localeCompare(left.basename); | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix file sorting order (Z-A vs A-Z). Directories are sorted ascending, but files are sorted descending (right.basename.localeCompare(left.basename)). This results in an unexpected Z-A file order, likely due to swapped variables, which was then codified into the tests.
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/utils/buildPierreProjection/buildPierreProjection.ts#L72-L73: change the return statement toreturn left.basename.localeCompare(right.basename);so files sort A-Z.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/utils/buildPierreProjection/buildPierreProjection.test.ts#L87-L90: update the expected test output to[file("a.ts"), file("z.ts")]to match the ascending order.
📍 Affects 2 files
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/utils/buildPierreProjection/buildPierreProjection.ts#L72-L73(this comment)apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/utils/buildPierreProjection/buildPierreProjection.test.ts#L87-L90
🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesTreeView/utils/buildPierreProjection/buildPierreProjection.ts
around lines 72 - 73, The file comparator in buildPierreProjection.ts currently
sorts files descending; change it to compare left.basename against
right.basename so files use A-Z order. Update the expected ordering in
buildPierreProjection.test.ts lines 87-90 to [file("a.ts"), file("z.ts")] to
match the corrected behavior.
Kitenite
added a commit
that referenced
this pull request
Jul 20, 2026
…nked icons (#5807) * fix(desktop): revert changes list to pre-Pierre rows, keep icon-sprite fix Restores the pre-#5782 changes view: folder mode renders the old FolderHeader + FileRow grouped list (colored +N/-N, status badges, "/" paths, plain filenames), tree mode returns to the standalone Pierre tree. Pierre's single text-only row decoration + hardcoded git lane can't reproduce two-tone diff stats or our StatusIndicator icons, so the unified-Pierre folder view read as a degraded nested tree. Keeps the independent icon-sprite fix: fetchSymbolBody now rejects non-SVG responses. Missing icon assets resolve to the SPA index.html (HTTP 200), and that HTML, embedded in a <symbol>, aborted Pierre's innerHTML sprite parse mid-stream and silently dropped ~300 symbols — blanking .test.ts and many other file icons in the Pierre trees (Files tab + changes tree). All 535 symbols now parse. Tradeoff: folder mode is no longer list-virtualized (reverts #5782's perf change); tree mode keeps Pierre's virtualization. * perf(desktop): virtualize the folder changes view Window the folder-mode changes list with @tanstack/react-virtual so large changesets stay responsive, while keeping the restored FolderHeader + FileRow look (colored stats, StatusIndicator, "/" paths). Folder-header and file rows are flattened into one list; each section virtualizes into the shared [data-changes-scroll-container] via scrollMargin, matching v1's FileListGroupedVirtualized. Tree mode already virtualizes via Pierre. Removes the "folder mode no longer virtualized" tradeoff from the revert. * fix(desktop): stop Pierre file trees from truncating names that fit Pierre keeps an empty decoration lane (flex: 1 1 0) on every row when the git lane is active but the row has no custom decoration — every file in the Files-tab explorer, plus changes with no +/- summary. That empty lane grows to fill the row and starves the name column, so Pierre's truncation grid middle-truncates names that easily fit (CLAUDE.md -> CLAUD…d). Add a shared PIERRE_TREE_UNSAFE_CSS that lets the name column take the free space and the decoration hug its own content. Pierre wraps unsafeCSS in @layer unsafe, so it overrides @layer base by cascade order — no !important. * chore(desktop): apply biome formatting to CommitRow (fix pre-existing CI lint)
This was referenced Aug 3, 2026
This branch was previously deployed
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
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
origin/mainthrough20cde0296and rerun the production-shaped 8-workspace / 20k-file Changes workload@pierre/treesrenderer for Folders and Tree modes@pierre/diffs1.2.12 and@pierre/trees1.0.0-beta.5Evidence
The original before-fix renderer profile identified eager
ChangesFoldersView→FileRowmounting as the application hot path. During the Pierre consolidation, CDP caught that a full-content host mounted 604 rows; the final 528 px Pierre viewport exposes a 14,472 px folder-mode range with only 43 mounted shadow rows. Tree mode also mounts 43 rows across a 19,320 px range, and a real wheel event reachedscrollTop=12000.The final mutator completed 192,000 tracked-file writes in 59.617 s. All eleven real-pointer transitions landed in 506–681 ms. Folders ↔ Tree, real wheel scrolling, and a projected folder-row file click were exercised over the matched CDP page.
Validation
127.0.0.1:9520, pageF07A1B359DD295ECA14F9C98B3B9428B, renderer7505, API7501, renderer PID29562bun run --cwd apps/desktop typecheckbun run lint:fixbun run lintgit diff --checkSafety and risk
Only synthetic Git roots and this worktree local host-service DB were used; no production database, migration, repository, or credential literal was touched. The synthetic project/workspace rows, eight worktrees, repository roots, temporary evidence, processes, and ports were cleaned up.
Status freshness remains separate: the cooldown screenshot still showed the cached 600-file result while direct Git status was 800, then refreshed later. The 20-row Pierre viewport also moves long-list scrolling into the tree surface; pointer, wheel, mode-switch, and file-selection checks passed, while multi-section nested-scroll ergonomics remain the main UI risk.
Ready for review. Never merge from this task.
Summary by CodeRabbit