Repository navigation
feat(site/src/pages/AgentsPage/components): expand and collapse file diffs in the git panel - #31029
tracyjohnsonux wants to merge 14 commits into
Conversation
…lapse file diffs in the git panel
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.13.0 | Round 6 | Last posted: Round 6, 38 findings (2 P2, 7 P3, 10 P4, 19 Nit), APPROVE. Review Finding inventoryFinding inventory: PR #31029Findings
Contested and acknowledgedCRF-23 (P4, DiffViewer.tsx:475) - scrollerEl held in state
Round logRound 1Full panel (round 1). Netero first pass: 1 P3, 3 Nit, 1 Note; no P0, panel proceeded. Panel: ging-ts, ging-react, nami, pariston, mafuuu, bisky, gon, leorio, wildcard razor (8 + wildcard, focused frontend PR). Law not spawned (73 effective additions). Round 2Restructure round (diffItemVersion.test.ts added; toggleFileCollapsed and diffItemVersion new). Churn guard PROCEED: 11/11 prior findings addressed by author claim. Netero (advisory, post-panel): re-raised CRF-9 with a repro (scrollTo's scroll event restores activeFile after setActiveFile(null)); new CRF-19, CRF-20, CRF-21. Panel: ging-ts, ging-react, nami, pariston, mafuuu, bisky, gon, leorio, wildcard kite. All new agents (no contested findings). Law not spawned (259 effective additions). Round 3Fix round (no files added, no signature change, effective additions +196 since R2). Churn guard PROCEED: 16 addressed, 1 contested (CRF-23). CRF-20 verified: title check run 118366227805 passed after the title edit (setup reported the earlier failed run 118366136648). Netero: no findings; mutation runs confirm CRF-9, 19 (partial), 22, 27, 28 stories. Panel: nami (reused R2 agent for CRF-23 dispute), ging-ts, ging-react, bisky, gon, leorio (comment and doc-comment lines touched), komugi (async story timing), wildcard razor. Round 4Fix round (stories, test, and one comment changed; +22 effective additions). Churn guard PROCEED: 9/9 addressed. Netero: no findings; mutation runs confirm CRF-19 and CRF-42 stories fail with the fix reverted. Panel: bisky, razor (CRF-19), komugi (CRF-42), gon, leorio (comments touched), ging-ts, ging-react, nami, wildcard meruem. Round 5Fix round (one test commit, +12 -4 in DiffViewer.stories.tsx). Churn guard PROCEED: 3/3 addressed. Netero: no findings; CRF-42 story fails 5/5 and 3/3 under the round 2 and round 1 bug shapes. Panel: komugi (CRF-42), gon (CRF-57, 58), leorio (comment and commit message touched), ging-ts, ging-react, bisky, wildcard razor. Round 6Fix round (comment-only commit ced985f in DiffViewer.stories.tsx). Churn guard PROCEED: 1/1 addressed. Netero: no findings; 42 storybook and 44 unit tests pass. Panel: gon, leorio (CRF-61 raisers), komugi, wildcard mafuuu. All no findings. CRF-61 verified fixed. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
This PR adds a per-file collapse toggle to the Agents git panel diff by passing CodeView's collapsed flag and folding it into the item version. Findings: 2 P2, 3 P3, 2 P4, 4 Nit.
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/pages/AgentsPage/components/GitPanel/GitPanel.tsx:564:LocalRepoContenthas no per-repokey, so an open inline comment box inCommentableDiffViewercarries over when switching local repos; the remote view prevents this withkey={viewRefId}.site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.tsx:295: clicking the tree row of the already-selected file does not scroll back to its header, becauseonSelectionChangedoes not fire when the selection is unchanged.
🤖 This review was automatically generated with Coder Agents.
| <HeaderContent | ||
| fileDiff={item.fileDiff} | ||
| collapsed={collapsedFiles.has(item.id)} | ||
| onToggleCollapsed={() => |
There was a problem hiding this comment.
P2 [CRF-11] Collapsing a file from its sticky header keeps the old scrollTop, so the collapsed header scrolls out of view, keyboard focus is lost, and the viewport lands inside the next file. (Razor)
When the header is sticky, CodeView's scroll anchor is a line inside the file; a collapsed file has no lines, so resolveAnchoredScrollTop returns nothing and scrollTop is kept (@pierre/diffs/dist/components/CodeView.js:1928-1929). A storybook repro with generateLargeDiff(4, 300) in a 500px container, scrollTop = 3000 inside components/module3.ts, then clicking "Collapse components/module3.ts" printed after click scrollTop 3000 scrollHeight 18566 expandBtn not rendered headers Collapse lib/module1.ts@6. When collapsing a file whose top is above the current scroll position, also call codeViewRef.current?.scrollTo({ type: "item", id, align: "start", behavior: "instant" }), as the tree-select path does, and keep focus on the toggle.
🤖
There was a problem hiding this comment.
Fixed in 401955b. toggleFileCollapsed checks whether the card's top is above the scroller's top and, if so, scrolls the item to its start before collapsing, so the header stays in place and keeps focus.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 401955b: in a repro, scrollTop returned to the file's start and the header stayed rendered.
🤖
| collapsed, | ||
| // CodeView only re-syncs an item when its version changes, so the | ||
| // collapsed flag must be part of it. | ||
| version: annotationsVersion(annotations) * 2 + (collapsed ? 1 : 0), |
There was a problem hiding this comment.
P2 [CRF-6] version still omits the file content, so CodeView keeps showing an older fileDiff for a path until its annotations or collapse state change, and with this PR the content then changes on the next collapse or expand click. (Pariston)
syncItemRecord returns early on item.version === nextItem.version (@pierre/diffs/dist/components/CodeView.js:1320); a throwaway vitest that called setItems with fileDiff A then B at the same version left the record holding A. LocalDiffPanel reparses repo.unified_diff on every update with no remount, so a file the agent edits keeps its earlier diff. The new comment states the rule this line must follow; apply it to every item input by folding fileDiff.cacheKey (the content hash from parseDiffString) into version together with annotations and collapsed.
🤖
There was a problem hiding this comment.
Fixed. The new diffItemVersion(fileDiff, annotations, collapsed) folds in fileDiff.cacheKey, and diffItemVersion.test.ts covers it.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 401955b: diffItemVersion folds cacheKey, which every caller gets from parseDiffString.
🤖
| collapsed, | ||
| // CodeView only re-syncs an item when its version changes, so the | ||
| // collapsed flag must be part of it. | ||
| version: annotationsVersion(annotations) * 2 + (collapsed ? 1 : 0), |
There was a problem hiding this comment.
P3 [CRF-1] The collapse toggle, the tree-click expand at :593, the scrollToFile expand at :555, and the version encoding have no test or story. (Netero, Bisky)
FE1 requires behavior coverage, and removing collapsed from version leaves the toggle doing nothing with no failing check. An assertion on aria-expanded or the button label is not enough: with version reverted to annotationsVersion(annotations) the button still flips to Expand src/main.ts while the body stays rendered. Add a story play function that clicks Collapse <file>, waits until no element's shadowRoot.innerHTML contains a line from that file, then clicks Expand <file> and asserts the line is back; a second story that sets scrollToFile to a collapsed file and asserts its body line is present covers the expand path.
🤖
There was a problem hiding this comment.
Added the CollapseAndExpandFile and ScrollToFileExpandsCollapsedFile stories with play functions. Both fail when the version change is reverted.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 7e53423: both stories fail when collapsed is removed from the item or from the version.
🤖
| activePath={activeFile} | ||
| onUserSelectPath={(fileName) => { | ||
| setActiveFile(fileName); | ||
| setFileCollapsed(fileName, false); |
There was a problem hiding this comment.
P3 [CRF-9] Clicking the tree row of a file that is already selected does not expand it, and the file you collapse is usually that file. (Mafuuu)
setFileCollapsed(fileName, false) runs only from onUserSelectPath, which fires from onSelectionChange; @pierre/trees skips that event when the selected set is unchanged (FileTreeController.js:805-812, render/FileTree.js:342), and the activePath effect at :317-330 has already made the file at the top of the viewport the only selected row. Fixing the scroll position after collapse keeps the collapsed file active, so this case becomes the normal one. Expand on row activation instead of on selection change, or clear the tree selection when a file collapses so the next click counts as a change.
🤖
There was a problem hiding this comment.
Fixed. Collapsing now clears the active file, and DiffFileTree deselects everything when activePath is null, so clicking the row again fires the select and expands the file.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Still open when the file is collapsed from its sticky header. The scrollTo at :512 changes scrollTop, the resulting scroll event reaches useActiveFileTracking through CodeView.handleScroll (CodeView.js:1790-1794), and activeFile is set back to the collapsed file on the next frame, overwriting setActiveFile(null). A temporary story (generateLargeDiff(4, 300), tree visible, scrollTop += 2000, collapse) printed after collapse selected ["lib/module1.ts"], then after tree click expanded? false. Derive the selection instead: pass activePath={activeFile !== null && !collapsedFiles.has(activeFile) ? activeFile : null} to DiffFileTree and delete setActiveFile(null).
🤖
There was a problem hiding this comment.
Fixed in b93ff12 with the derived activePath you suggested. TreeClickExpandsFileCollapsedFromStickyHeader scrolls 2000px into a 300-line file, collapses it from the sticky header, checks the tree drops its selection, clicks the row, and waits for the body. It fails with activePath={activeFile}.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in b93ff12: reverting to activePath={activeFile} fails TreeClickExpandsFileCollapsedFromStickyHeader.
🤖
| const [activeFile, setActiveFile] = useState<string | null>(null); | ||
| // Files whose diff body is hidden, keyed by file name. Every file starts | ||
| // expanded. | ||
| const [collapsedFiles, setCollapsedFiles] = useState<ReadonlySet<string>>( |
There was a problem hiding this comment.
P3 [CRF-7] Collapse state leaks between local repositories that share a file path. (Pariston)
collapsedFiles is keyed by the repo-relative fileDiff.name and lives for the DiffViewer mount, but GitPanel.tsx:564 renders LocalRepoContent without a key, so switching repos reuses the same DiffViewer and a collapsed README.md in repo A shows collapsed in repo B. The remote view avoids this with key={viewRefId} at GitPanel.tsx:553. Add key={effectiveView.repoRoot} to LocalRepoContent.
🤖
There was a problem hiding this comment.
Fixed. LocalRepoContent in GitPanel.tsx is now keyed by repoRoot, so collapse state resets per repo.
Generated by Coder Agents on behalf of @tracyjohnsonux.
| "shrink-0 text-[11px] font-semibold leading-none", | ||
| changeColor(fileDiff.type), | ||
| )} | ||
| <div className="flex h-8 min-w-0 items-center justify-between gap-3 border-0 border-b border-solid border-border bg-surface-secondary py-2 pr-1.5 pl-1 font-sans text-sm"> |
There was a problem hiding this comment.
P4 [CRF-12] The loading skeleton header in SkeletonFile (:382) still uses pl-2.5 with no chevron slot, so the file label moves right by about 20px when loading finishes. (Nami)
The loaded header now starts its label at 4px padding + 20px button + 6px gap = 30px; the skeleton's first block starts at 10px. Change the skeleton header to pl-1 and add a size-5 slot before the label block; the Loading story in DiffViewer.stories.tsx:73 shows it.
🤖
There was a problem hiding this comment.
Fixed. The skeleton header now uses pl-1, gap-1.5, and a size-5 spacer to match the toggle.
Generated by Coder Agents on behalf of @tracyjohnsonux.
| <HeaderContent fileDiff={item.fileDiff} /> | ||
| <HeaderContent | ||
| fileDiff={item.fileDiff} | ||
| collapsed={collapsedFiles.has(item.id)} |
There was a problem hiding this comment.
Nit [CRF-2] HeaderContent reads collapsedFiles.has(item.id) instead of item.collapsed, the value CodeView uses for the body. (Netero)
Two sources for one state let the chevron and the body disagree whenever version fails to change. Pass collapsed={item.collapsed === true} and toggle with setFileCollapsed(item.id, !item.collapsed).
🤖
There was a problem hiding this comment.
Done. The header reads item.collapsed.
Generated by Coder Agents on behalf of @tracyjohnsonux.
| // null. Skip the scroll without signalling completion so the parent | ||
| // keeps the pending target and retries once the view is ready. | ||
| if (!canScroll || !codeViewRef.current) return; | ||
| setFileCollapsed(scrollToFile, false); |
There was a problem hiding this comment.
Nit [CRF-3] The scrollToFile effect now sets React state from a prop, which FE8 asks to avoid. (Netero)
The effect exists to drive CodeView.scrollTo; the added setFileCollapsed causes an extra render after commit. Expand where the parent sets scrollToFile, or add a comment stating why this effect is the exception.
🤖
There was a problem hiding this comment.
Kept the state change in the effect because the scroll has to wait until CodeView is mounted. Added a comment explaining why DiffViewer owns the expansion.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
The added comment covers it.
🤖
| )} | ||
| <div className="flex h-8 min-w-0 items-center justify-between gap-3 border-0 border-b border-solid border-border bg-surface-secondary py-2 pr-1.5 pl-1 font-sans text-sm"> | ||
| <div className="flex min-w-0 items-center gap-1.5 overflow-hidden"> | ||
| <button |
There was a problem hiding this comment.
Nit [CRF-4] The toggle is a hand-styled <button> instead of the Button primitive from #/components/Button/Button. (Netero)
FE3 asks for shared primitives; Button has variant="subtle" and size="icon", and CommentableDiffViewer.tsx:143 already uses Button size="icon" with a size override instead of reimplementing hover and focus-visible styles.
🤖
There was a problem hiding this comment.
Done. The toggle is now Button variant="subtle" size="icon".
Generated by Coder Agents on behalf of @tracyjohnsonux.
| const codeViewRef = useRef<CodeViewHandle<string, undefined>>(null); | ||
| const isDark = theme.palette.mode === "dark"; | ||
| const [activeFile, setActiveFile] = useState<string | null>(null); | ||
| // Files whose diff body is hidden, keyed by file name. Every file starts |
There was a problem hiding this comment.
Nit [CRF-14] The collapsedFiles comment restates the declaration, and "Every file starts expanded" is true only for the first time a file name appears in a mount. (Gon, Leorio)
The set keeps names after a file leaves sortedFiles, so a file the user collapsed, which the agent reverts and later edits again, comes back collapsed. Replace the comment with that lifetime fact, or delete it.
🤖
There was a problem hiding this comment.
Done. The comment now states only the lifetime: names persist for the mount.
Generated by Coder Agents on behalf of @tracyjohnsonux.
… and expand with stories
… per-file diff collapse
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2 of the per-file collapse toggle in the Agents git panel diff. Ten of the eleven round 1 findings are fixed in the code; the tree-click expand fix is incomplete (reply on that thread). New findings: 4 P3, 6 P4, 6 Nit.
The title CI check fails because the PR title scope no longer contains GitPanel/GitPanel.tsx (inline comment at GitPanel.tsx:567).
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.tsx:295: clicking the tree row of the already-selected file does not scroll back to its header, becauseonSelectionChangedoes not fire when the selection is unchanged.site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.stories.tsx:451: the comment saysCodeViewnever paints lines in the test environment, butCollapseAndExpandFilewaits forCodeView-rendered line text and passes, so CodeView-path behavior has been tested throughFileDiffinstead.
Notes:
- Collapse state is keyed by file name only, so a file collapsed before the agent rewrites it stays collapsed afterward, with the header's
+N -Mcounts as the only signal of new content. Whether collapse resets when the content changes is a product decision for the pending design check.
site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.tsx:155
P4 [CRF-33] The syncItemRecord contract is documented on annotationsVersion, but diffItemVersion now produces item.version, and its own comment only lists inputs the body already shows. (Gon)
Move the contract to diffItemVersion: // CodeView's syncItemRecord re-syncs an item only when its version changes, so every render input must feed it. cacheKey is the content hash parseDiffString stamps. Reduce the annotationsVersion comment to what it does: fold each annotation's side and line so moving the comment box changes the result, truncated to 32 bits.
🤖
site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.tsx:1
Nit [CRF-21] The PR description says "Only file changed: DiffViewer.tsx" and lists the toggle test as not done, but the PR now changes four files and adds two stories and diffItemVersion.test.ts. (Netero)
Update the implementation notes and the "Draft: needs ... regression tests" line.
🤖
🤖 This review was automatically generated with Coder Agents.
| }); | ||
| } | ||
| // Clear the tree selection so clicking this file's row expands it. | ||
| setActiveFile(null); |
There was a problem hiding this comment.
P3 [CRF-22] Collapsing any file clears the tree highlight of the file at the top of the viewport, and the highlight stays gone until the next scroll. (Nami, Mafuuu, Kite)
setActiveFile(null) runs on every collapse, not only when the collapsed file is activeFile, and collapsing a file lower in the viewport does not change scrollTop, so useActiveFileTracking never restores it; in a diff too short to scroll it never comes back. A temporary story (generateLargeDiff(20, 3), tree visible, scrollTop = 5) had components/module13.ts selected, and after clicking "Collapse components/module18.ts" the selection was []. The derived activePath proposed in the tree-click reply fixes this too, because the highlight then clears only when the active file is the collapsed one.
🤖
There was a problem hiding this comment.
Fixed in b93ff12. setActiveFile(null) is gone, and the tree gets activePath derived as activeFile unless that file is collapsed. Collapsing another file now leaves the highlight alone. CollapsingAnotherFileKeepsTreeSelection covers this and fails when the old clear is restored.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in b93ff12: restoring setActiveFile(null) fails CollapsingAnotherFileKeepsTreeSelection.
🤖
| // A line from src/main.ts in sampleDiff. | ||
| const firstFileLine = "Starting server..."; | ||
|
|
||
| export const CollapseAndExpandFile: Story = { |
There was a problem hiding this comment.
P3 [CRF-19] No story covers the sticky-header scrollTo branch in toggleFileCollapsed (DiffViewer.tsx:505-518) or the tree-click expand path (DiffViewer.tsx:656 with the null-activePath branch at :341-349). (Netero)
CollapseAndExpandFile collapses the first file at scrollTop 0, where the < check at :509 is false, and neither story renders the file tree because isExpanded defaults to false at 700px. The incomplete tree-click fix passed all 19 stories. Add a story with the tree visible that collapses a file from its sticky header, clicks its tree row, and asserts the body is back; also assert the tree selection after collapsing a non-active file.
🤖
There was a problem hiding this comment.
Added TreeClickExpandsFileCollapsedFromStickyHeader (tree visible, collapse from the sticky header, tree-row click expands) and CollapsingAnotherFileKeepsTreeSelection in 8971af5.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
The tree-click half is covered; the sticky-header half is not. TreeClickExpandsFileCollapsedFromStickyHeader reaches the scrollTo branch, but after the collapse it checks only that the body is gone and the tree has no selection, and the row click that follows scrolls to the file anyway. Disabling the branch (scrollerEl && false && at DiffViewer.tsx:511) leaves all 42 stories passing. Right after waitForNoDiffBody at :662, add await canvas.findByRole("button", { name: `Expand ${firstLongFile}` }); that was run and passes at HEAD, and with the branch disabled fails with Unable to find role="button" and name "Expand components/module3.ts".
🤖
There was a problem hiding this comment.
Fixed in 45c3f3e: the story now waits for the Expand ${firstLongFileName} button right after the body disappears. With scrollerEl && false && it fails with Unable to find role="button" and name "Expand components/module3.ts".
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 45c3f3e: disabling the sticky-header branch fails the story at the Expand lookup.
🤖
| })); | ||
|
|
||
| describe("diffItemVersion", () => { | ||
| it("changes when the file content changes", () => { |
There was a problem hiding this comment.
P3 [CRF-27] The fix for stale diff content is tested only on diffItemVersion, so reverting the call site at DiffViewer.tsx:596 to annotationsVersion(annotations) passes every test. (Bisky)
The unit test checks that the helper returns different numbers; no test checks that DiffViewer passes that number to CodeView, and ReparseSamePathAfterEdit renders FileDiff, not CodeView. Add a DiffViewer story whose render holds reparseFirstBody in state and passes parseDiffString(body) as parsedFiles; the play function waits for "const v = 2", clicks a button that sets reparseSecondBody, and waits for "const v = 3". That story was run: it passes at HEAD and fails with cacheKey removed from the version.
🤖
There was a problem hiding this comment.
Added ReparseSamePathInDiffViewer as you described. It fails with cacheKey removed from the version.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 8971af5: removing cacheKey from the version fails ReparseSamePathInDiffViewer.
🤖
| <LocalRepoContent | ||
| // A remount per repo keeps collapsed files and any open | ||
| // inline comment box from carrying over to another repo. | ||
| key={effectiveView.repoRoot} |
There was a problem hiding this comment.
P3 [CRF-20] The PR title scope site/src/pages/AgentsPage/components/DiffViewer does not contain GitPanel/GitPanel.tsx, so the title CI check fails. (Netero)
.github/workflows/contrib.yaml:203-215 fails the job when a changed file is outside the scope. Change the scope to site/src/pages/AgentsPage/components, as commit 401955b does.
🤖
There was a problem hiding this comment.
Fixed. The title scope is now site/src/pages/AgentsPage/components.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified: the title check passed after the title edit.
🤖
| <LocalRepoContent | ||
| // A remount per repo keeps collapsed files and any open | ||
| // inline comment box from carrying over to another repo. | ||
| key={effectiveView.repoRoot} |
There was a problem hiding this comment.
P4 [CRF-28] No test covers key={effectiveView.repoRoot}; removing it brings back collapse state carrying over between repos that share a path. (Bisky)
Copy ViewSwitcherOpen, give both repos unified_diff: sampleDiff, collapse src/main.ts, switch repos through git-panel-view-switcher, and assert findByRole("button", { name: "Collapse src/main.ts" }) resolves. This story was not run.
🤖
There was a problem hiding this comment.
Added GitPanel CollapseStateResetsPerRepo in edba422. It fails with the key removed.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in edba422: removing the key fails CollapseStateResetsPerRepo.
🤖
| }); | ||
|
|
||
| it("changes when the collapsed flag flips, even with many annotations", () => { | ||
| // A floating-point version loses its low bit after about 10 |
There was a problem hiding this comment.
Nit [CRF-29] The comment says a floating-point version loses its low bit "after about 10 annotations", but with the cache key folded in, a float fold loses it for every file. (Bisky, Leorio)
A float version of the fold on a parsed file with 0 annotations gives v*31 === v*31+1 as true. Say that any float fold over the cache key drops the collapsed bit, and replace the count loop with one case.
🤖
There was a problem hiding this comment.
Updated the comment and replaced the loop with one case.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Fixed as asked in 307a4d8; the leftover annotations(12) is a separate comment.
🤖
| ): number { | ||
| let version = annotationsVersion(annotations); | ||
| for (const char of fileDiff.cacheKey ?? "") { | ||
| version = (Math.imul(version, 31) + char.charCodeAt(0)) | 0; |
There was a problem hiding this comment.
Nit [CRF-32] The step (Math.imul(version, 31) + n) | 0 and the multiplier 31 appear inline three times (lines 169, 188, 190). (Gon)
Add one hashStep(version, value) helper and call it from all three places.
🤖
There was a problem hiding this comment.
Added hashStep and used it in all three places. The syncItemRecord contract now sits on diffItemVersion (CRF-33).
Generated by Coder Agents on behalf of @tracyjohnsonux.
| path: file.name, | ||
| status: gitStatusForFile(file), | ||
| })); | ||
| // Tracks the row currently selected in the model so scroll-driven syncing |
There was a problem hiding this comment.
Nit [CRF-35] The sentence added to the selectedPathRef comment describes the null-activePath branch of the effect at :340, and that effect's own comment does not mention the branch. (Gon)
Move the sentence to the effect comment at :335.
🤖
There was a problem hiding this comment.
Moved the sentence to the effect comment.
Generated by Coder Agents on behalf of @tracyjohnsonux.
| ); | ||
| }; | ||
|
|
||
| // A file reference jump must land on visible code, so it expands the file. |
There was a problem hiding this comment.
Nit [CRF-38] "A file reference jump must land on visible code, so it expands the file" repeats the rationale at DiffViewer.tsx:616, and the story name already says what it checks. (Gon)
Delete the comment.
🤖
There was a problem hiding this comment.
Deleted.
Generated by Coder Agents on behalf of @tracyjohnsonux.
| // its start first, which leaves the header where it is on screen; | ||
| // otherwise CodeView keeps scrollTop and the collapsed header | ||
| // scrolls out of view. | ||
| const card = button.closest("diffs-container"); |
There was a problem hiding this comment.
Nit [CRF-39] card names the diffs-container element, which the stories call host (DiffViewer.stories.tsx:509). (Gon)
Rename it to host or fileContainer.
🤖
There was a problem hiding this comment.
Renamed to host.
Generated by Coder Agents on behalf of @tracyjohnsonux.
…psed flag version test
…ection from collapse state
…ection and reparse through DiffViewer
…tate reset across repos
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3 of the per-file collapse toggle in the Agents git panel diff. The round 2 findings are fixed except the story for the sticky-header collapse, which runs that branch but does not assert its result (reply on that thread). The scrollerEl state is accepted because #31030 needs it as an effect dependency. New findings: 2 P4, 6 Nit, all in stories, tests, or comments.
Notes:
DiffViewer.stories.tsx:659:clickAfterScrollruns before the scroll fromscroller.scrollTop = 2000is handled, so the pointer-events wait its comment describes has not started at that call site. The outcome is the same either way.
🤖 This review was automatically generated with Coder Agents.
| await canvas.findByRole("button", { name: `Collapse ${firstLongFile}` }), | ||
| ); | ||
| await waitForNoDiffBody(canvasElement, bodyText); | ||
| await waitFor(() => { |
There was a problem hiding this comment.
P4 [CRF-42] The "row is not selected" waitFor passes on its first check, so if the scroll-driven reselect of the collapsed file lands late, the story passes with that bug present. (Komugi)
With the bug, the row is unselected between the collapse click and the reselect (scroll event, useActiveFileTracking rAF, commit, DiffFileTree effect), and the row click lands in that window and expands the file. With the derived activePath reverted and a 120 ms delay added before onActiveFileChange (useActiveFileTracking.ts:68), the story passed 3 of 3 runs. Before the negative check, wait for a positive signal that the collapse's scroll was handled, for example the scroller's scroll event followed by the pointer-events restore clickAfterScroll waits for.
🤖
There was a problem hiding this comment.
Fixed in 45c3f3e. The story now captures the scroll event the collapse fires (nextScrollEvent), waits for it, then waits for the Expand button's pointer events to come back (waitForScrollSettled) before the not-selected check. Mutation-checked with the derived activePath reverted plus a 120 ms delay around onActiveFileChange: fails 3 of 3 runs with "Collapsed file is still selected in the tree".
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Still open at P4. waitForScrollSettled waits for restoreScrollInteractions, a setTimeout of 120 ms (@pierre/diffs/dist/components/CodeView.js:113,309), while the reselect runs on a separate path: the useActiveFileTracking rAF, the setActiveFile commit, then the DiffFileTree effect. With the bug restored (activePath={activeFile} plus setActiveFile(null) on collapse) and a delay before onActiveFileChange, 0 and 100 ms fail, while 130, 150, and 200 ms pass with the bug present, so a loaded runner can hide the regression. The story cannot fail on correct code. Wait for something that comes after the reselect, for example one requestAnimationFrame after await collapseScroll (the tracker's rAF was queued first in the same scroll event) followed by the selection check; whether React commits and the tree effect has run by then is unverified.
🤖
There was a problem hiding this comment.
Fixed in 9244ae2. After await collapseScroll the story waits two requestAnimationFrames. The tracker's rAF was queued first in that scroll event, so by the second frame its setActiveFile has committed and the tree effect has run. The pointer-events wait still follows before the check. With the real bug restored (activePath={activeFile} plus setActiveFile(null) on collapse, no delay) it fails 3 of 3 runs with "Collapsed file is still selected in the tree". With an added 10 ms delay before onActiveFileChange it fails 2 of 2. It passes on the fixed code.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 9244ae2: with the bug restored, the story failed every run under render busy loops up to 300 ms and a reselect delayed by up to two frames.
🤖
| const bodyText = `in ${firstLongFile}`; | ||
| await waitForDiffBody(canvasElement, bodyText); | ||
| const scroller = findScroller(canvasElement); | ||
| scroller.scrollTop = 2000; |
There was a problem hiding this comment.
P4 [CRF-52] scroller.scrollTop = 2000 is what puts the first file's header in the sticky state, and nothing says so. (Gon)
If a change to generateLargeDiff(4, 300) or line height moves 2000 outside the first file, toggleFileCollapsed skips the scrollTo branch and the story still passes without testing the path it is named for. Name it, for example const insideFirstFileScrollTop = 2000; // inside the first file's 300 lines; scrollTop = 5 at :702 exists only to fire a scroll event and needs the same treatment.
🤖
There was a problem hiding this comment.
Fixed in 45c3f3e: insideFirstLongFileScrollTop = 2000 with a comment that it sits inside the first file's 300 lines. The scrollTop = 5 line now says any nonzero offset fires the scroll event that sets the active file. The new Expand assertion (other thread) also fails if 2000 drifts outside the first file.
Generated by Coder Agents on behalf of @tracyjohnsonux.
| [...longFiles].sort((a, b) => compareTreePaths(a.name, b.name))[0]?.name ?? | ||
| ""; | ||
|
|
||
| // Collapsing from a sticky header scrolls the file to its start, and the |
There was a problem hiding this comment.
Nit [CRF-44] "or clicking its row would not expand it" repeats DiffViewer.tsx:654-655, and the comment at :685 restates the story name CollapsingAnotherFileKeepsTreeSelection. (Gon)
The fact a reader cannot get elsewhere is that the sticky-header scrollTo fires a scroll event that makes the collapsed file active again. Rewrite this one to // The sticky-header collapse calls scrollTo, and its scroll event makes the collapsed file active again. and delete the one at :685.
🤖
There was a problem hiding this comment.
Fixed in 45c3f3e: rewrote the comment as suggested and deleted the one on CollapsingAnotherFileKeepsTreeSelection.
Generated by Coder Agents on behalf of @tracyjohnsonux.
| ); | ||
| } | ||
|
|
||
| // CodeView's syncItemRecord re-syncs an item only when its version changes, |
There was a problem hiding this comment.
Nit [CRF-45] "The hash stays a 32-bit integer so later inputs are not lost to floating-point precision" describes hashStep's | 0 but sits on diffItemVersion, so someone editing hashStep will not see it. (Gon)
Move that sentence to hashStep at :159.
🤖
There was a problem hiding this comment.
Fixed in 3c0643f: the 32-bit sentence now sits on hashStep.
Generated by Coder Agents on behalf of @tracyjohnsonux.
|
|
||
| // FileDiff renders hunks synchronously enough for play tests; CodeView | ||
| // virtualizes and never paints lines in this environment. | ||
| // FileDiff renders hunks synchronously enough for play tests. |
There was a problem hiding this comment.
Nit [CRF-48] With the "CodeView ... never paints lines" clause removed, "FileDiff renders hunks synchronously enough" has nothing to contrast with and reads as if DiffViewer would not work here. (Gon)
ReparseSamePathInDiffViewer (:521) shows CodeView paints lines in play tests. Delete the comment.
🤖
There was a problem hiding this comment.
Fixed in 45c3f3e: comment deleted.
Generated by Coder Agents on behalf of @tracyjohnsonux.
| }); | ||
|
|
||
| it("changes when the collapsed flag flips", () => { | ||
| // Any floating-point fold over the cache key drops the collapsed bit, |
There was a problem hiding this comment.
Nit [CRF-50] The comment now blames the cache key fold, but the test still passes annotations(12), so a reader cannot tell why 12 annotations are there. (Leorio, Gon)
In node, a plain v * 31 + charCode fold over a 19-character key like content-1a2b3c4d-5f exceeds 2^53 at 0 annotations. Pass [] and write: // A plain float fold (version * 31 + char) over the ~19-character cacheKey exceeds 2^53, so adding 1 for the collapsed flag would not change the value.
🤖
There was a problem hiding this comment.
Fixed in 2197640: the test passes [] and uses the suggested comment.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 2197640: the test fails under a plain float hashStep.
🤖
| }; | ||
|
|
||
| const shortFiles = parseDiffString(generateLargeDiff(20, 3)); | ||
| const sortedShortFiles = [...shortFiles] |
There was a problem hiding this comment.
Nit [CRF-51] sortedShortFiles holds file names while shortFiles on :680 holds FileDiffMetadata, and firstLongFile (:633) is also a name. (Gon)
Rename to sortedShortFileNames and firstLongFileName, matching firstFileName at :38.
🤖
There was a problem hiding this comment.
Fixed in 45c3f3e: renamed to sortedShortFileNames and firstLongFileName.
Generated by Coder Agents on behalf of @tracyjohnsonux.
| }, | ||
| }; | ||
|
|
||
| // CodeView's scroll root is the nearest scrolling ancestor of a file card. |
There was a problem hiding this comment.
Nit [CRF-54] "file card" is a third name for the diffs-container element, which DiffViewer.tsx:506 and waitForNoDiffBody (:536) call host. (Gon)
Write "of a diffs-container host".
🤖
There was a problem hiding this comment.
Fixed in 45c3f3e: the comment now says "of a diffs-container host".
Generated by Coder Agents on behalf of @tracyjohnsonux.
…notations from collapsed-flag test
…t note to hashStep
…header collapse and wait for its scroll
|
/coder-agents-review |
There was a problem hiding this comment.
Round 4 of the per-file collapse toggle in the Agents git panel diff. The round 3 findings are fixed except the timing guard before the "not selected" check, which now waits on a fixed 120 ms timer rather than on the reselect (reply on that thread). New findings: 2 Nit in the stories.
🤖 This review was automatically generated with Coder Agents.
| ""; | ||
|
|
||
| // Inside the first file's 300 lines, so its header is stuck. | ||
| const insideFirstLongFileScrollTop = 2000; |
There was a problem hiding this comment.
Nit [CRF-57] "Inside the first file's 300 lines" copies the 300 from generateLargeDiff(4, 300) at :640 instead of referring to it, so changing the line count leaves the comment stale. (Gon)
Add const longFileLineCount = 300;, pass it to generateLargeDiff, and write the comment against that name.
🤖
There was a problem hiding this comment.
Fixed in 9244ae2: longFileLineCount = 300 feeds generateLargeDiff, and the comment refers to it.
Generated by Coder Agents on behalf of @tracyjohnsonux.
| const bodyText = `in ${firstLongFileName}`; | ||
| await waitForDiffBody(canvasElement, bodyText); | ||
| const scroller = findScroller(canvasElement); | ||
| const stuck = nextScrollEvent(scroller); |
There was a problem hiding this comment.
Nit [CRF-58] stuck names a state the promise does not check; it resolves on the next scroll event, like collapseScroll at :671. (Gon)
Rename it to initialScroll.
🤖
There was a problem hiding this comment.
Fixed in 9244ae2: renamed to initialScroll.
Generated by Coder Agents on behalf of @tracyjohnsonux.
…eselect frame before checking the tree
|
/coder-agents-review |
There was a problem hiding this comment.
Round 5 of the per-file collapse toggle in the Agents git panel diff. The three round 4 findings are fixed. With the tree-selection bug restored, the sticky-header story failed every run, including runs with slowed renders and a reselect delayed by up to two frames. One new Nit on the comment above the two nextFrame() calls.
🤖 This review was automatically generated with Coder Agents.
| name: `Expand ${firstLongFileName}`, | ||
| }); | ||
| await collapseScroll; | ||
| // The scroll handler picks the active file in a rAF queued before this |
There was a problem hiding this comment.
Nit [CRF-61] The comment above the two nextFrame() calls says "the scroll handler", "this one", and "the next frame" without naming the code or saying which call each phrase means, so a reader cannot tell why there are two. (Gon, Leorio)
The order it relies on comes from the browser's scheduling of rAF, React's commit, and the passive effect, not from a guarantee, and on correct code a broken order only means the story misses a regression. Suggested text: // useActiveFileTracking queued its rAF in the collapse's scroll event, so it runs in the first frame. In Chromium, React commits that setActiveFile and the DiffFileTree selection effect runs before the second frame.
🤖
There was a problem hiding this comment.
Fixed in ced985f with the suggested wording.
Generated by Coder Agents on behalf of @tracyjohnsonux.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 6 of the per-file collapse toggle in the Agents git panel diff. The comment above the two nextFrame() calls is fixed, and no findings remain open.
Two earlier items still need a human decision, not a change in this PR: clicking the tree row of the already-selected file does not scroll back to its header (DiffViewer.tsx:295, out of scope), and whether a collapsed file should expand again when the agent changes its content is open for the design check.
🤖 This review was automatically generated with Coder Agents.
Adds a chevron to each file header in the Agents git panel diff so a single file can be collapsed to its header and expanded again (DES-22201).
@pierre/diffsCodeView's built-in per-itemcollapsedflag. The itemversionhashes the collapsed flag, the annotations, and the file's contentcacheKey, becauseCodeViewonly re-syncs an item when its version changes.scrollToFile, expands it if collapsed. A collapsed file is never highlighted in the tree, so clicking its row always expands it.DiffViewermount and resets when switching repos; all files start expanded.Buttonprimitive witharia-expandedand anExpand/Collapse <file>label.Draft: needs a design check against the Figma before review.
Implementation notes
DiffViewer/DiffViewer.tsx,DiffViewer/DiffViewer.stories.tsx,DiffViewer/diffItemVersion.test.ts,GitPanel/GitPanel.tsx(keysLocalRepoContentby repo root),GitPanel/GitPanel.stories.tsx.CollapseAndExpandFile,ScrollToFileExpandsCollapsedFile,ReparseSamePathInDiffViewer,TreeClickExpandsFileCollapsedFromStickyHeader,CollapsingAnotherFileKeepsTreeSelection, andGitPanelCollapseStateResetsPerRepo. Each was checked to fail with its fix reverted.tsc --noEmit,oxlint,biome,lint:compiler,knip, DiffViewer and GitPanel Vitest suites (56 tests), DiffViewer and GitPanel Storybook tests (42).Generated by Coder Agents.