Repository navigation
feat(site/src/pages/AgentsPage/components): render git panel files as rounded cards - #31030
tracyjohnsonux wants to merge 24 commits into
Conversation
…el files as rounded cards
…panel header and file list
…crollbar gutter beside diff code
…der corners rounded and tighten scrollbar spacing
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.13.0 | Round 8 | Last posted: Round 8, 37 findings (1 P1, 1 P2, 17 P3, 8 P4, 10 Nit), APPROVE. Review Finding inventoryFinding inventory: PR #31030Findings
Contested and acknowledgedCRF-45 (P3, DiffViewer.tsx:108) - Firefox classic scrollbars: cards taller than metrics
Round logRound 1Full panel (round 1). Netero first pass: 1 P2, 3 P3, 1 Note; no P0, panel proceeded. Law not run (effective additions 95). Panel: ging-ts, ging-react, nami, pariston, mafuuu, gon, leorio, bisky; wildcard kite (random). Size 9 vs focused target 6-8: ging-ts, ging-react, nami required by .tsx triggers; gon and leorio by once-per-PR floor. Round 2Fix round (not restructure): no files added, renamed, or moved; DiffViewer.stories.tsx newly in the diff but existing; effective additions +110 since R1 (95 to 205); FILE_CARD_CSS changed from array to template literal (patch for CRF-8). Churn guard PROCEED, 11/11 addressed. CRF-1 verified: title run 118351389388 passed on ea9a1b6 (118351273899 ran before the rename). Netero: 1 P3 (CRF-18), 1 P4 (CRF-19). Panel: nami, pariston, kite, mafuuu (CRF-6 raisers), gon, leorio, bisky, ging-react (CRF-7..12 raisers), ging-ts (.tsx), komugi (story play functions), wildcard chopper (random). All new agents (no contested findings). Round 3Fix round (not restructure): no files added, renamed, or moved; effective additions +52 since R2 (205 to 257); base merge 0f8bac6. Churn guard PROCEED, 10/10 addressed. Netero: no findings; verified CRF-18 (story fails with padding hardcoded and with the content observer removed), CRF-19, CRF-22 (probe shows stuck header). Panel: bisky, chopper, kite, mafuuu, leorio, gon (R2 raisers), ging-ts, ging-react, nami (.tsx), komugi (story timing), wildcard zoro (random). All new agents. Round 4Fix round (not restructure): no files added, renamed, or moved; effective additions +27 since R3 (257 to 284); base merge 76d9275. CRF-34 fix changed production CSS (scrollbar-color reset, 6px gutter pin), so panel added Pariston and Hisoka for the scrollbar premise and cross-browser edges. Churn guard PROCEED, 6/6 addressed. Netero: 1 P3 (CRF-43); verified CRF-34 (with real scrollbars head passes, reverting reset fails -46px), CRF-35, CRF-36. Panel: bisky, zoro, gon (R3 raisers), ging-ts, ging-react, nami (.tsx), komugi, pariston, hisoka, wildcard knov (random). All new agents. Round 5Fix round (not restructure): no files added, renamed, or moved; effective additions +33 since R4 (284 to 317); base merge 1e7baed. Churn guard PROCEED: 3 addressed, 1 contested (CRF-45). Netero: 1 P3 (CRF-54); verified CRF-43, CRF-44. Panel: nami, hisoka (CRF-45 raisers, reused R4 agents via message_agent), bisky (CRF-44), pariston (CRF-46), gon (CRF-43 note), ging-ts, ging-react (.tsx), komugi (story timing), wildcard razor (random). Dispute check-in does not count toward size. Round 6Fix round (not restructure): no files added, renamed, or moved; effective additions +22 since R5 (317 to 339); base merge 211ab2b. Churn guard PROCEED, 2/2 addressed. Netero: no findings, 1 Note (CRF-59); verified CRF-54 by mutation and CRF-45 rule precedence in headless Firefox. Panel: nami, hisoka (CRF-45 raisers), bisky (tests), gon and leorio (comment and error-message lines touched), ging-ts, ging-react (.tsx), wildcard meruem (random). New agents (no contested findings). Round 7Fix round (not restructure): comment-only commits 1fb8f26 and 8a0b88c; effective additions +4 since R6 (339 to 343). Churn guard PROCEED, 4/4 addressed. Netero: no findings; verified CRF-60..63 against library source, tsc, biome, 26/26 stories. Panel: hisoka, leorio, gon (R6 raisers), ging-ts, ging-react, nami (.tsx), wildcard knov (random). New agents. Round 8Fix round (not restructure): one comment line changed in 40ac0a9; PR description Firefox bullet corrected (CRF-66 content). Churn guard PROCEED, 1/1 addressed. Netero: no findings (tsc, biome, 26/26 stories). Panel: gon (CRF-65 raiser), leorio (CRF-66 description), ging-ts, ging-react, nami (.tsx), wildcard mafuuu (random). No findings; CRF-65 verified by Gon, Leorio, Nami, Mafuuu, Netero. All in-scope findings closed. Reviewed against ced985f..40ac0a9. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
This PR restyles the Agents git panel file list as rounded cards. Findings: 1 P1, 1 P2, 6 P3, 1 P4, 2 Nit.
The description gives pixel values but no screenshot. For a visual restyle, add a before and after image showing an expanded card, a collapsed card, and a stuck header (.claude/docs/PR_STYLE_GUIDE.md asks for visual evidence on UI changes).
Out of scope (needs a ticket or explicit acceptance by a human):
DiffViewer.tsx:519: the existingcontainerWidtheffect measures inuseEffect, soshowTreecan flip one frame after the list first paints.DiffViewer.tsx:222: thefileTreeUnsafeCSScomment says the library forces classic scrollbars throughscrollbar-gutter: stable, which gives a different cause than the new comment at line 95 for the same gutter strip.RemoteDiffPanel.tsx:120:{/* Compact PR/branch sub-header */}restates the condition on the next line and adds no information.
Notes:
DiffViewer.tsx:296: the plain-text+N −Nmarkup is now inlined in three places (ChatDiffStats.tsx:44-46,ChatSearchResults.tsx:306-311, and here) with different gaps, so a style change has to be made three times.
site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.tsx:397
P3 [CRF-3] The skeleton comment still describes "flat, full-width file headers with a change badge and stat pill", and this PR changes both. (Netero)
SkeletonFile and DiffViewerSkeleton now render rounded, inset cards, and the stat pill is now plain text. Update the comment to describe rounded file cards with plain-text stats.
🤖
🤖 This review was automatically generated with Coder Agents.
| " pointer-events: none;", | ||
| "}", | ||
| "pre[data-diff] {", | ||
| " margin: 0 4px 4px;", |
There was a problem hiding this comment.
P1 [CRF-6] The 4px bottom margin on pre[data-diff] makes each expanded card 4px taller than the height CodeView computes, so the end of the file list cannot be scrolled into view and file-tree jumps land below the target. (Nami, Pariston, Kite, Mafuuu)
CodeView sizes each file from itemMetrics (header, lines, separators, and paddingBottom, default spacing = 8) and measures only line rows; diffViewerMetrics does not include this margin. In Storybook (Chromium), 6 one-hunk files render at 670px while CodeView sets the container to 646px, and with 40 to 80 files the last card's code stays below the viewport at maximum scrollTop; scrollTo from the file tree puts the target header 8 to 12px below the top. Adding paddingBottom: 12 (8 default plus the 4px inset) to diffViewerMetrics made computed and DOM heights equal in the same runs. Define the 4px inset once and use it in both FILE_CARD_CSS and paddingBottom so they cannot drift.
🤖
There was a problem hiding this comment.
Fixed in 7100ffa. diffViewerMetrics.paddingBottom is now DIFF_FILE_SPACING + FILE_CODE_INSET (12), and FILE_CODE_INSET also drives the pre margin. The new OverflowScrollsToLastCard story fails with the last card 16px below the list when the fix is reverted.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 7100ffae3e. Reverting only the paddingBottom line makes OverflowScrollsToLastCard fail with the last card 16px below the list.
🤖
| }> = ({ repoRoot, repo, diffStats, onCommit }) => { | ||
| return ( | ||
| <div className="flex shrink-0 items-center gap-2 border-0 border-b border-solid border-border px-3 py-1.5"> | ||
| <div className="flex shrink-0 items-center gap-2 px-3 py-1.5"> |
There was a problem hiding this comment.
P2 [CRF-1] The PR title scope site/src/pages/AgentsPage/components/DiffViewer does not contain this file, so the required title check fails. (Netero)
.github/workflows/contrib.yaml:203-216 rejects a title whose scope does not contain every changed file, and both title runs on this head failed. Change the scope to site/src/pages/AgentsPage/components, which commit 10f8dc7bf6 already uses for the same files.
🤖
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 fixed: the title check passed on ea9a1b6ead after the rename.
🤖
| return () => observer.disconnect(); | ||
| }, [containerEl]); | ||
| useEffect(() => { | ||
| const scroller = containerEl?.lastElementChild; |
There was a problem hiding this comment.
P3 [CRF-2] The scrollbar effect finds the CodeView scroller with containerEl?.lastElementChild, although CodeView accepts a containerRef prop for that element. (Netero)
@pierre/diffs/dist/react/CodeView.js assigns containerRef to the scroll root. The lastElementChild lookup is correct only while CodeView is the last child of this flex container; if anything is rendered after it, the effect measures the wrong element and hasScrollbar is wrong with no error. Pass a state setter as containerRef and key the effect on that element.
🤖
There was a problem hiding this comment.
Fixed. The probe now uses the scrollerEl set through CodeView's containerRef (added in #31029) and is keyed on that element.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 7100ffae3e.
🤖
| observer.observe(containerEl); | ||
| return () => observer.disconnect(); | ||
| }, [containerEl]); | ||
| useEffect(() => { |
There was a problem hiding this comment.
P3 [CRF-4] No story or test covers the new behavior: the scrollbar-dependent right padding, the full radius on collapsed headers, or the sticky-header corner mask. (Netero)
The unit suites for DiffViewer/ and GitPanel/ never render DiffViewer (GitPanel.test.tsx:23 mocks LocalDiffPanel), and no story in DiffViewer.stories.tsx collapses a file or overflows the list vertically, which is also why the card height mismatch at line 91 passed every check. Add stories with play functions that (a) render enough files to overflow, scroll to the end, and assert the last card is fully visible, (b) assert the CodeView root's padding-right is 0px when offsetWidth > clientWidth, and (c) collapse a file and assert the header has a 6px radius on all corners.
🤖
There was a problem hiding this comment.
Added OverflowScrollsToLastCard, which covers (a) and (b): the last card is fully visible at max scroll, and padding-right is 0px when offsetWidth > clientWidth, otherwise 8px. Added CollapsedHeaderIsFullyRounded for (c). The sticky-corner mask is a visual effect, so it is not asserted.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
P3 [CRF-18] In the Storybook test run, OverflowScrollsToLastCard only takes the overlay-scrollbar branch, so the code that drops the right padding for a classic scrollbar is still untested. (Netero)
A temporary throw after DiffViewer.stories.tsx:615 printed 700 700 8px (offsetWidth, clientWidth, padding-right): headless Chromium hides scrollbars, so expected is always "8px", and injecting ::-webkit-scrollbar { width: 12px } gave the same result. The story would still pass if DiffViewer.tsx:765 always used FILE_LIST_PADDING, and the content-element observation at line 617 never fires. Stub the scroller's offsetWidth getter to return clientWidth + 12, collapse a file so the observer runs update(), and assert padding-right is 0px; the stub approach is untested, so confirm the assertion fails when line 765 is reverted.
🤖
There was a problem hiding this comment.
Added ClassicScrollbarDropsRightPadding. It checks padding-right is 8px, stubs the scroller's offsetWidth getter to clientWidth + 12, collapses a file so the content observer runs update(), then asserts 0px. Confirmed it fails when the padding is hardcoded to FILE_LIST_PADDING. OverflowScrollsToLastCard no longer checks padding.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 32c57c5867: ClassicScrollbarDropsRightPadding fails when the padding is hardcoded to FILE_LIST_PADDING.
🤖
| const DIFF_VIEWER_LINE_HEIGHT = 16.5; | ||
| const DIFF_HEADER_HEIGHT = 32; | ||
| const HUNK_SEPARATOR_HEIGHT = 28; | ||
| // Space between file cards and around the list, in pixels. |
There was a problem hiding this comment.
P3 [CRF-7] The comment says FILE_LIST_PADDING is the space "around the list", but it sets only the top and bottom padding; the left and right 8px are pl-2/pr-2 at line 682, and the skeleton repeats both values as p-2 gap-0.5 at line 464. (Leorio, Gon, Mafuuu, Bisky)
Changing FILE_LIST_PADDING or FILE_CARD_GAP changes only the vertical spacing of the loaded list, so its sides and the skeleton keep the old values. Apply the constants through style on CodeView (paddingLeft: FILE_LIST_PADDING, paddingRight: hasScrollbar ? 0 : FILE_LIST_PADDING) and on the skeleton container, or make the comment say the values are vertical only and name the Tailwind classes that must match.
🤖
There was a problem hiding this comment.
Fixed. CodeView gets paddingLeft and paddingRight from FILE_LIST_PADDING through style, and the skeleton container uses FILE_CARD_GAP and FILE_LIST_PADDING. The comments now say gap is vertical and padding is on every side.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 7100ffae3e.
🤖
| const FILE_CARD_CSS = [ | ||
| ":host {", | ||
| " display: block;", | ||
| " border-radius: 6px;", |
There was a problem hiding this comment.
P3 [CRF-8] The card radius 6px and code inset 4px are literals repeated in FILE_CARD_CSS (lines 67, 71, 75, 86, 91, 92) and again as rounded-md and mx-1 mb-1 rounded in the skeleton. (Gon)
The box-shadow spread at line 86 must stay large enough to cover the rounded corner, so it depends on the radius, but nothing in the code says which values change together. Add FILE_CARD_RADIUS and FILE_CODE_INSET constants and build the CSS with a template literal that uses them, including the spread and the paddingBottom fix for line 91.
🤖
There was a problem hiding this comment.
Fixed. Added FILE_CARD_RADIUS and FILE_CODE_INSET. FILE_CARD_CSS is now a template literal that uses them for the radius, the shadow spread, and the pre margin and radius. The skeleton uses the same constants.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 7100ffae3e.
🤖
| "[data-diffs-header]:not(:has(~ pre[data-diff])) {", | ||
| " border-radius: 6px;", | ||
| "}", | ||
| // A stuck header sits on top of its own card, so the card background |
There was a problem hiding this comment.
P3 [CRF-9] The sticky-corner mask paints --surface-primary, which is correct only while the panel behind the list is surface-primary, and the comment does not say so. (Gon, Kite)
This holds today (RightPanel.tsx:373 and the body both use bg-surface-primary), but if the git panel is placed on another surface every stuck header shows mismatched corners with no error. State that dependency in the comment, and drop the clauses that restate clip-path: inset(0) and the box-shadow color.
🤖
There was a problem hiding this comment.
Fixed. The comment now states that the mask assumes a surface-primary panel and that the spread must be at least the radius. The restating clauses are gone.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 7100ffae3e.
🤖
| // enough horizontal room, rather than keying off the viewport width. | ||
| const [containerEl, setContainerEl] = useState<HTMLDivElement | null>(null); | ||
| const [containerWidth, setContainerWidth] = useState(0); | ||
| // Whether the file list shows a space-taking vertical scrollbar. The |
There was a problem hiding this comment.
Nit [CRF-10] hasScrollbar is false while an overlay scrollbar is visible, because it checks offsetWidth - clientWidth > 0. (Gon)
Rename it to hasClassicScrollbar so the name carries the condition, and shorten the comment to the reason the right padding is dropped.
🤖
There was a problem hiding this comment.
Renamed to hasClassicScrollbar and shortened the comment to the reason.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 7100ffae3e.
🤖
| const FILE_CARD_GAP = 2; | ||
| const FILE_LIST_PADDING = 8; | ||
|
|
||
| // Renders each file as a rounded card: the header row and the code sit on a |
There was a problem hiding this comment.
Nit [CRF-11] This comment and the "Observe the content too" comment at line 534 restate the rules and calls below them. (Gon)
Keep only what the code does not show: here, that the <pre> is inset so its horizontal scrollbar stays inside the card and that a collapsed file renders no <pre>; at line 534, that collapsing files changes content height without resizing the scroller.
🤖
There was a problem hiding this comment.
Trimmed both comments to what the code doesn't show.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 7100ffae3e.
🤖
| observer.observe(containerEl); | ||
| return () => observer.disconnect(); | ||
| }, [containerEl]); | ||
| useEffect(() => { |
There was a problem hiding this comment.
P4 [CRF-12] The scrollbar probe runs in useEffect, so with a classic scrollbar the first frame after the list mounts can paint with pr-2 before hasScrollbar switches it to pr-0. (Ging-React)
This is inferred from React effect timing and was not measured in a browser. CommentableDiffViewer.tsx:112 uses useLayoutEffect for the same measure-then-set pattern; using it here applies the correct padding on the first paint.
🤖
There was a problem hiding this comment.
Switched the probe to useLayoutEffect.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 7100ffae3e; the probe runs in useLayoutEffect, so the first paint has the correct padding.
🤖
…wer with base before merging
…it-panel' into tracy/git-panel-file-cards
…feedback on file cards
…d overflow, padding, and collapsed radius
|
/coder-agents-review |
There was a problem hiding this comment.
This round checks the round 1 fixes and the new stories. All 11 round 1 findings are fixed; the replies below confirm each one we verified. New findings: 4 P3, 3 P4, 3 Nit, mostly about the new stories passing in cases they are meant to catch.
The description says OverflowScrollsToLastCard checks that the right padding is 0 with a classic scrollbar. In the Storybook test run the scroller has no classic scrollbar, so that branch never runs (see the reply on the coverage thread).
site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.stories.tsx:387
P3 [CRF-23] The RenameWithLongPaths comment still says the story checks that "the stat-count pills remain visible", and this PR removes the pill. (Gon)
HeaderContent now renders the counts as plain colored text, so nothing in this story is a pill. Change item 2 to say the +N −N line counts remain visible (not clipped).
🤖
🤖 This review was automatically generated with Coder Agents.
| } | ||
| const cardBottom = lastCard.getBoundingClientRect().bottom; | ||
| const scrollerBottom = scroller.getBoundingClientRect().bottom; | ||
| if (cardBottom > scrollerBottom + 0.5) { |
There was a problem hiding this comment.
P3 [CRF-20] OverflowScrollsToLastCard only fails when the last card crosses the scroller's bottom edge, so itemMetrics height errors of up to 8px in either direction pass. (Bisky, Chopper, Kite, Mafuuu)
At maximum scrollTop with correct metrics, the last card ends exactly FILE_LIST_PADDING (8px) above the scroller bottom. With paddingBottom 1px too small the gap drops to 2px and 6px of code is hidden behind the padding, and with it 12px too large the gap is 50px; the story passes in both runs. Assert Math.abs(scrollerBottom - cardBottom - FILE_LIST_PADDING) <= 0.5 and print the measured gap in the error.
🤖
There was a problem hiding this comment.
Fixed in 32c57c5. The story now checks that the gap between the last card and the list bottom is within 0.5px of FILE_LIST_PADDING, and prints the measured gap. With paddingBottom 1px short, it fails.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 32c57c5867: with paddingBottom 1px short the story fails with Last card ends 2px above the list bottom, want 8px.
🤖
| style.borderBottomRightRadius, | ||
| style.borderBottomLeftRadius, | ||
| ]; | ||
| if (radii.some((radius) => radius !== "6px")) { |
There was a problem hiding this comment.
P3 [CRF-21] CollapsedHeaderIsFullyRounded checks only the collapsed header, so it still passes when every header, expanded or collapsed, gets the full radius. (Bisky, Kite, Chopper, Leorio)
Replacing [data-diffs-header]:not(:has(~ pre[data-diff])) with [data-diffs-header] in FILE_CARD_CSS gives expanded headers 6px 6px 6px 6px (rounded bottom corners above the code), and the story still passes. Before clicking Collapse, assert the expanded header computes 6px 6px 0px 0px, then keep the post-collapse check. Include the expected radii in both error messages, as the padding-right check already does.
🤖
There was a problem hiding this comment.
Fixed. The story first checks that the expanded header is 6px 6px 0px 0px, then that the collapsed header is 6px 6px 6px 6px. Both error messages include the expected radii. It fails when the :not(:has(~ pre)) selector is replaced with [data-diffs-header].
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 32c57c5867: matching every header with the full radius fails with Expanded header radii are 6px 6px 6px 6px, want 6px 6px 0px 0px.
🤖
| // Inset of the code block from the card's sides and bottom, in pixels. | ||
| const FILE_CODE_INSET = 4; | ||
| // CodeView's default per-file bottom padding (VirtualFileMetrics.spacing). | ||
| const DIFF_FILE_SPACING = 8; |
There was a problem hiding this comment.
P4 [CRF-19] DIFF_FILE_SPACING = 8 copies DEFAULT_CODE_VIEW_FILE_METRICS.spacing, which @pierre/diffs exports. (Netero)
If a library upgrade changes the default spacing, line 127 keeps adding 8, computed file heights stop matching the DOM, and the last card is cut off again. Use DEFAULT_CODE_VIEW_FILE_METRICS.spacing + FILE_CODE_INSET and delete the local constant.
🤖
There was a problem hiding this comment.
Fixed in e296585. The value is now DEFAULT_CODE_VIEW_FILE_METRICS.spacing + FILE_CODE_INSET, and the local constant is gone.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in e2965853bb.
🤖
| corners. Mask the corners with the panel background. This assumes the | ||
| panel behind the list is surface-primary; on another surface the corners | ||
| would show the wrong color. The spread must be at least the radius. */ | ||
| [data-diffs-header]::before { |
There was a problem hiding this comment.
P4 [CRF-22] No story scrolls the list until a header is stuck, so neither a play function nor a Storybook snapshot renders the corner mask. (Bisky, Chopper)
A change to the box-shadow spread or clip-path shows square or wrong-colored corners only while scrolling, and no check fails. Add a story with the overflow files and scrollTop set partway into a file so its header is stuck; the snapshot then covers the corners without asserting colors in the play function.
🤖
There was a problem hiding this comment.
Added StickyHeaderCorners. It scrolls 600px into a 300-line file so the header is stuck when the snapshot is taken.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 32c57c5867: at snapshot time the first header is stuck at the top of the list and its corner mask is rendered.
🤖
| const lastOverflowFile = [...overflowFiles] | ||
| .sort((a, b) => compareTreePaths(a.name, b.name)) | ||
| .at(-1); | ||
| const lastOverflowLine = `context line 5 in ${lastOverflowFile?.name}`; |
There was a problem hiding this comment.
P4 [CRF-26] The 5 in context line 5 works only because it matches the 5 passed to generateLargeDiff(30, 5) at line 582. (Gon)
If line 582 changes to 3, the story waits for a line that is never generated and times out with "Last file has not rendered yet". Add const overflowLinesPerFile = 5; and use it in both places.
🤖
There was a problem hiding this comment.
Added overflowLinesPerFile and used it in both places.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 32c57c5867.
🤖
| const HUNK_SEPARATOR_HEIGHT = 28; | ||
| // Vertical gap between file cards, in pixels. | ||
| const FILE_CARD_GAP = 2; | ||
| // Padding on every side of the file list, in pixels. |
There was a problem hiding this comment.
Nit [CRF-24] "Padding on every side of the file list" is false while a classic scrollbar is shown, because line 765 sets the right padding to 0. (Gon, Mafuuu)
For example: // Padding around the file list, in pixels. The right side is 0 while a classic scrollbar is shown.
🤖
There was a problem hiding this comment.
Updated with your wording.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in e2965853bb.
🤖
| // file headers with a change badge and stat pill, gutter-aligned code | ||
| // lines, and centered hunk separators. Keeping the same shape avoids a | ||
| // jarring swap when the parsed diff replaces the placeholder. | ||
| // The loading state mirrors the real diff layout: rounded file cards with a |
There was a problem hiding this comment.
Nit [CRF-25] The skeleton comment lists visual details (badge, plain-text stats, gutter alignment), and that list is what went stale in round 1 and again in the RenameWithLongPaths comment. (Gon)
The skeleton JSX shows the layout; keep only the reason. For example: // The loading state mirrors the real file card layout so the parsed diff replaces it without a visible jump. Change both together.
🤖
There was a problem hiding this comment.
Reduced to the reason only.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in e2965853bb.
🤖
| // enough horizontal room, rather than keying off the viewport width. | ||
| const [containerEl, setContainerEl] = useState<HTMLDivElement | null>(null); | ||
| const [containerWidth, setContainerWidth] = useState(0); | ||
| // A classic scrollbar already separates the cards from the panel edge, so |
There was a problem hiding this comment.
Nit [CRF-27] hasClassicScrollbar is declared between containerWidth and the effect that sets containerWidth, under the comment about the file tree threshold. (Gon)
Move the state and its comment to just above the useLayoutEffect that sets it.
🤖
There was a problem hiding this comment.
Moved the state and its comment above the useLayoutEffect.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in e2965853bb.
🤖
… with base before merging
…it-panel' into tracy/git-panel-file-cards
… review feedback on file cards
|
/coder-agents-review |
There was a problem hiding this comment.
This round checks the round 2 fixes. All 10 round 2 findings are fixed and verified (each story fails when the code it covers is reverted). New findings: 2 P3, 2 P4, 2 Nit. The two P3s only show up with classic (always visible) scrollbars, which headless Chromium in CI hides, so they need a check outside the default Storybook test run.
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/index.css:743: the global:root { scrollbar-color }makes Chromium ignore the@pierre/diffs[data-code]::-webkit-scrollbarrules in every code view in the app, which causes both the vertical gutter strip this PR works around and a 15px horizontal scrollbar track that CodeView's height metrics do not count.
🤖 This review was automatically generated with Coder Agents.
| const gap = | ||
| scroller.getBoundingClientRect().bottom - | ||
| lastCard.getBoundingClientRect().bottom; | ||
| if (Math.abs(gap - listPadding) > 0.5) { |
There was a problem hiding this comment.
P3 [CRF-34] With classic scrollbars, each [data-code] element shows a 15px horizontal scrollbar track that diffViewerMetrics does not count, so the last card ends 34px below the list and the last code line cannot be scrolled into view. (Bisky)
Running this story with Playwright's --hide-scrollbars removed fails with Last card ends -34px above the list bottom, want 8px; each [data-code] measures offsetHeight 98 / clientHeight 83 even though scrollWidth == clientWidth. The base fails the same way (-42px), so this PR does not cause it, but the users it misses (Windows and Linux Chrome, macOS with "always show scrollbars") are the ones the PR's gutter fix targets, and the comment at line 733 says the heights match. Handle the horizontal track in the same [data-code] override in FILE_CARD_CSS (for example, resetting scrollbar-color there so the library's ::-webkit-scrollbar rule applies again, which may also make the scrollbar-gutter override unnecessary; untested), or limit the story comment to overlay scrollbars and state the gap.
🤖
There was a problem hiding this comment.
Fixed in d3dffcb. Resetting scrollbar-color alone was not enough: the library measures --diffs-scrollbar-gutter once, before unsafeCSS applies, so it still read 15px and subtracted that from the code's bottom padding (story failed at -34px, then +29px in headless). FILE_CARD_CSS now resets scrollbar-color: auto on [data-code] outside Firefox so the library's ::-webkit-scrollbar rules apply (vertical 0, horizontal gutter-height), and diffViewerStyle pins --diffs-scrollbar-gutter-override: 6px (the library's fallback). This also replaces the old scrollbar-gutter: auto override: with the vertical bar at width 0 the strip is gone (offsetWidth == clientWidth).
With --hide-scrollbars removed, each [data-code] now measures offsetHeight 91 / clientHeight 85 and OverflowScrollsToLastCard passes. In headless runs the story zeroes the override because no scrollbar fills the gutter there. All 46 Storybook tests pass in both modes. Removing the scrollbar-color reset fails at -46px; removing the pin fails at -34px.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in d3dffcb93a for Chromium: with --hide-scrollbars removed the head passes, and removing the scrollbar-color reset fails at -46px. The follow-up findings on lines 108, 110, and 129 and on the story cover Firefox, the thumb, iOS, and CI coverage.
🤖
| await waitForDiffBody(canvasElement, `in ${firstName}`); | ||
| const scroller = findScroller(canvasElement); | ||
| const paddingRight = () => getComputedStyle(scroller).paddingRight; | ||
| if (paddingRight() !== `${listPadding}px`) { |
There was a problem hiding this comment.
P3 [CRF-35] ClassicScrollbarDropsRightPadding fails in any browser that shows real classic scrollbars, because its first check expects 8px from whatever scrollbar the environment has. (Bisky)
overflowFiles overflows the 500px frame, so with a real classic scrollbar the layout effect already sets padding-right: 0px before the stub; with --hide-scrollbars removed the story fails with padding-right is 0px without a scrollbar, want 8px. Anyone running this story in Storybook on Windows or Linux Chrome sees a failed interaction with no code change. Stub both states: set offsetWidth to clientWidth, trigger a resize, assert 8px; then set it to clientWidth + 12, trigger a resize, assert 0px (see the note on line 617 for how to trigger the resize).
🤖
There was a problem hiding this comment.
Fixed in 6266d41. The story stubs offsetWidth for both states (clientWidth + 0, then clientWidth + 12) and nudges maxWidth down 1px each time to fire the ResizeObserver. It passes with and without --hide-scrollbars. Hardcoding the padding fails with padding-right is 8px with a classic scrollbar, want 0px.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 6266d41e6d: the story passes with and without --hide-scrollbars, and hardcoding the padding fails with padding-right is 8px with a classic scrollbar, want 0px.
🤖
| observer.observe(scrollerEl); | ||
| // Collapsing files changes the content height without resizing the | ||
| // scroller, which can add or remove the scrollbar. | ||
| if (scrollerEl.firstElementChild) { |
There was a problem hiding this comment.
P4 [CRF-36] The comment says collapsing files can add or remove the scrollbar without resizing the scroller, but a classic scrollbar shrinks the scroller's content box, so observer.observe(scrollerEl) already fires and the second observation is redundant. (Zoro)
Checked in Playwright Chromium without --hide-scrollbars: a ResizeObserver on an overflow: auto element alone reported 300x200 to 285x200 when its content grew past the height and a 15px scrollbar appeared, and back to 300x200 when it shrank. Only the stubbed story depends on the content observation, because stubbing offsetWidth changes no layout. Delete lines 615 to 619, and in the story trigger the re-check by resizing the scroller (for example scroller.style.width = `${scroller.clientWidth - 1}px` ) instead of collapsing a file.
🤖
There was a problem hiding this comment.
Fixed in d3dffcb: removed the content observation and its comment. The story now triggers the re-check by resizing the scroller (maxWidth; width has no effect on the flex-1 scroller).
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in d3dffcb93a.
🤖
| }, | ||
| play: async ({ canvasElement }) => { | ||
| await waitForDiffBody(canvasElement, `in ${firstLongFile}`); | ||
| findScroller(canvasElement).scrollTop = 600; |
There was a problem hiding this comment.
P4 [CRF-38] scrollTop = 600 and "context line 60" on the next line depend on each other but are unnamed. (Gon)
Line 60 is a valid wait target only because, at a 16.5px line height in the 500px frame, it is not rendered before the scroll and is rendered after it. Changing either number, the line height, or the frame height can make the wait pass before the scroll renders, or never pass. Name both values and add one comment saying the line is not rendered at scrollTop 0.
🤖
There was a problem hiding this comment.
Fixed in 6266d41: stickyHeaderScrollTop = 600 and stickyHeaderWaitLine = 60, with a comment that the line is not rendered at scrollTop 0 at the 16.5px line height in the 500px frame.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 6266d41e6d: at scrollTop 0 the highest rendered line of the first file is 34, so the wait for line 60 cannot pass before the scroll.
🤖
| }, | ||
| }; | ||
|
|
||
| // Scrolls partway into a long file so its header is stuck, which renders the |
There was a problem hiding this comment.
Nit [CRF-37] The comment says a stuck header "renders the rounded-corner mask", but FILE_CARD_CSS gives every header the mask; sticking only puts card content behind the corners, which is the case the mask changes. (Gon)
For example: // Scrolls partway into a long file so its stuck header covers the card body, the case the corner mask in FILE_CARD_CSS exists for.
🤖
There was a problem hiding this comment.
Fixed in 6266d41 with the suggested wording.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 6266d41e6d.
🤖
| if (!lastCard?.shadowRoot?.textContent?.includes(lastOverflowLine)) { | ||
| throw new Error("Last file has not rendered yet"); | ||
| } | ||
| const gap = |
There was a problem hiding this comment.
Nit [CRF-39] gap here is the bottom list padding, while the PR uses "gap" for the space between cards (FILE_CARD_GAP, layout.gap). (Gon)
Rename it to spaceBelowLastCard.
🤖
There was a problem hiding this comment.
Fixed in 6266d41: renamed to spaceBelowLastCard.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 6266d41e6d.
🤖
…it-panel' into tracy/git-panel-file-cards
…lbars to the card metrics
…scrollbars in card stories
|
/coder-agents-review |
There was a problem hiding this comment.
This round checks the round 3 fixes. All 6 round 3 findings are fixed. With real classic scrollbars, the head passes all 26 DiffViewer stories, and reverting the scrollbar-color reset makes OverflowScrollsToLastCard fail at -46px. New findings: 4 P3, all about the new scrollbar CSS: where the 6px pin is wrong, what the reset also changes, Firefox, and what CI can see. Two of them (Firefox, iOS) could not be reproduced in headless browsers and are marked unverified.
Out of scope (needs a ticket or explicit acceptance by a human):
@pierre/diffsdist/style.js(Firefox[data-code]rule):padding-bottomignores the scrollbar gutter, so everyCodeViewconsumer's computed heights are wrong in Firefox with space-taking scrollbars.@pierre/diffsdist/style.js(Firefox[data-code]rule): the code scrollbar thumb is colored close to the code background, so it has low contrast before and after this PR.site/vite.config.mts: the Storybook test runner uses Playwright's default--hide-scrollbars, so no story in the project can observe classic-scrollbar layout in CI.
🤖 This review was automatically generated with Coder Agents.
| // from the code's bottom padding. The library measures it once, before the | ||
| // FILE_CARD_CSS scrollbar rules apply, so a classic 15px scrollbar would | ||
| // make each card taller than its metrics. Matches the library's fallback. | ||
| "--diffs-scrollbar-gutter-override": "6px", |
There was a problem hiding this comment.
P3 [CRF-43] Pinning --diffs-scrollbar-gutter-override to 6px on every platform bypasses the library's 0px iOS fallback, so where the code's horizontal scrollbar takes no layout space each expanded card is 6px shorter than its metrics. (Netero, Gon)
[data-code] gets padding-bottom: max(0px, gap - var(--diffs-scrollbar-gutter)), so the card matches itemMetrics only when the scrollbar is exactly as tall as the gutter; the library sets --diffs-scrollbar-gutter-fallback: 0px under @supports (-webkit-touch-callout: none) for iOS. Removing the story's hidden-scrollbar branch in headless Chromium (no scrollbar space) fails with Last card ends 32px above the list bottom, want 8px, which shows the size of the error; iOS itself is unverified. Declare the override in FILE_CARD_CSS as :host { --diffs-scrollbar-gutter-override: var(--diffs-scrollbar-gutter-fallback); } so it resolves to 6px on desktop and 0px on iOS (untested; this also makes "Matches the library's fallback" true everywhere), and move the story's 0px override to where that :host rule does not win.
🤖
There was a problem hiding this comment.
Fixed in 9a6a097. The pin moved into FILE_CARD_CSS as :host { --diffs-scrollbar-gutter-override: var(--diffs-scrollbar-gutter-fallback); }, inside the non-Firefox block. It resolves to the library's 6px on desktop and to its 0px iOS fallback, and the inline 6px in diffViewerStyle is gone. The story's headless override is now a document diffs-container { ... } rule, which beats the :host rule. iOS itself is still unverified.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 9a6a097421: the pin resolves through --diffs-scrollbar-gutter-fallback (6px on desktop, 0px under the library's iOS query), and removing it fails the gutter check. iOS itself was not run.
🤖
| the code with scrollbar-color, so it keeps the library's value. */ | ||
| @supports not (-moz-appearance: none) { | ||
| [data-code] { | ||
| scrollbar-color: auto; |
There was a problem hiding this comment.
P3 [CRF-46] Resetting scrollbar-color turns on all of the library's ::-webkit-scrollbar rules, including [data-code]::-webkit-scrollbar-thumb { background-color: #0000 }, so in Chromium the code's horizontal thumb is invisible until the pointer is over the file. (Pariston, Hisoka)
Before this change the thumb used the global surface-scrollbar color and was always visible. With the library rules copied into a shadow root under the app's :root scrollbar-color (Chromium, classic scrollbars), the thumb pixel was rgb(113,113,122) before the reset and rgb(255,255,255) after it without hover, rgb(251,251,251) with hover on a white background; this was not sampled in the real component. Mouse users in Chrome and Edge cannot see or drag the scrollbar to reach the end of a long line, and touch users get no sign that the line scrolls. Add [data-code]::-webkit-scrollbar-thumb { background-color: hsl(var(--surface-scrollbar)); } inside the same block, and mention the thumb in the comment above.
🤖
There was a problem hiding this comment.
Confirmed and fixed in 9a6a097: added [data-code]::-webkit-scrollbar-thumb { background-color: hsl(var(--surface-scrollbar)); } in the same block, and the comment above now mentions the thumb. Sampled in the real component (Chromium, classic scrollbars, dark theme, no hover, an overflowing line). Without the rule the thumb pixel is rgb(10,12,16) (the background). With it the pixel is rgb(113,113,122), which matches the rest of the app.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 9a6a097421, including on hover: unsafeCSS is in a later cascade layer than the library's hover thumb rule, and the thumb stays rgb(113, 113, 122) before and after hovering the code.
🤖
| ::-webkit-scrollbar rules for the code, which hide its vertical scrollbar | ||
| and size the horizontal one to --diffs-scrollbar-gutter. Firefox styles | ||
| the code with scrollbar-color, so it keeps the library's value. */ | ||
| @supports not (-moz-appearance: none) { |
There was a problem hiding this comment.
P3 [CRF-45] The @supports not (-moz-appearance: none) guard leaves Firefox on the library rule that gives [data-code] the full 8px padding-bottom without subtracting its scrollbar-width: thin horizontal scrollbar, so with space-taking scrollbars each expanded card is taller than its metrics, the same failure fixed here for Chromium. (Nami, Hisoka)
[data-code] is overflow-x: scroll, so the thin track is always present when scrollbars take space (Windows 10 default, or "always show scrollbars" on macOS, Windows 11, GNOME), and the 6px override has no effect in Firefox. Unverified: headless Firefox shows no scrollbars even with ui.useOverlayScrollbars: 0 and widget.gtk.overlay-scrollbars.enabled: false (padding-bottom: 8px, card height equal to metrics only because the track is absent); OverflowScrollsToLastCard in headed Firefox with classic scrollbars settles it. If it reproduces, add a Firefox rule that subtracts the scrollbar height from the code's bottom padding (or hides the code's horizontal scrollbar there), and name the covered browsers in the comment at line 104.
🤖
There was a problem hiding this comment.
Reproduced, not fixed here. Headed Firefox under Xvfb with classic scrollbars (ui.useOverlayScrollbars: 0, 17px document scrollbar) measures [data-code] at offsetHeight 99 / clientHeight 91 with padding-bottom: 8px, so each expanded card is 8px taller than its metrics.
This comes from the library's Firefox [data-code] rule, which this PR does not touch. main and every other CodeView consumer have the same mismatch, as the round 4 out-of-scope note on dist/style.js says. CSS cannot fix it cleanly here: Firefox's thin scrollbar takes 0px with overlay scrollbars and about 8px with classic ones, so any fixed subtraction is wrong on one of them. Hiding the code's horizontal scrollbar in Firefox would remove the only drag affordance for long lines. This is better handled as a library fix (subtract the measured scrollbar height in Firefox the way the WebKit path does) under its own ticket. The comment already says Firefox keeps the library's value.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Still open: the reproduction confirms the 8px per card, and the reason for not fixing it here does not hold, because the subtraction does not need a fixed value. (Nami, Hisoka)
Since 9a6a097421 the gutter pin sits inside the non-Firefox block, so in Firefox --diffs-scrollbar-gutter resolves to the library's runtime measurement. getMeasuredScrollbarGutter measures an element with the data-code attribute, which gets the same Firefox scrollbar-width: thin rule as the real code, so it should read about 8px with classic scrollbars and 0px with overlay ones (headless Firefox probe: gutter=0px measured=0px; classic mode unverified, your Xvfb setup can confirm it). Adding @supports (-moz-appearance: none) { [data-code] { padding-bottom: max(0px, calc(var(--diffs-gap-block, var(--diffs-gap-fallback)) - var(--diffs-scrollbar-gutter))); } } to FILE_CARD_CSS applies the subtraction the WebKit path uses and keeps the scrollbar. If the measurement turns out wrong in classic mode, link an issue for the library fix instead.
🤖
There was a problem hiding this comment.
You're right, the measured gutter works. Fixed in 19edeb1 with the suggested @supports (-moz-appearance: none) rule in FILE_CARD_CSS. Measured in headed Firefox under Xvfb, as scrollbar height + padding-bottom:
- Classic (default style): bar 6, padding 2, gutter
6px, total 8. - Classic (
scrollbar.style: 4): bar 8, padding 0, gutter8px, total 8. - Overlay: bar 0, padding 8, gutter
0px, total 8.
The card now matches its metrics in all three, and the scrollbar stays.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 19edeb1943 for overlay scrollbars: in headless Firefox the rule wins over the library's Firefox rule (forced gutter 3px gives padding-bottom: 5px, 20px gives 0px), and with the real 0px gutter the last card ends 8px above the list. Classic mode was not reproducible here; your headed measurements cover it.
🤖
| play: async ({ canvasElement }) => { | ||
| await waitForDiffBody(canvasElement, `in ${sortedOverflowFiles[0]}`); | ||
| const scroller = findScroller(canvasElement); | ||
| if (codeScrollbarHidden(canvasElement)) { |
There was a problem hiding this comment.
P3 [CRF-44] In CI the story replaces the production 6px gutter override with 0px, so it measures a configuration production does not ship, and reverting the classic-scrollbar fix leaves every story passing. (Bisky)
The Storybook runner is headless Chromium, which hides scrollbars, so codeScrollbarHidden is always true there. Deleting the --diffs-scrollbar-gutter-override line, the scrollbar-color: auto reset, or both leaves the DiffViewer suite at Tests 26 passed (26); with --hide-scrollbars removed the same revert fails at -34px. Add assertions that hold in headless runs and fail on a revert, for example that [data-code] computes scrollbar-color: auto and that --diffs-scrollbar-gutter resolves to the intended value (untested), and say in the story comment that the height check only covers hidden scrollbars in CI.
🤖
There was a problem hiding this comment.
Fixed in ae66299. Before any override, OverflowScrollsToLastCard now asserts that [data-code] computes scrollbar-color: auto and that --diffs-scrollbar-gutter resolves to 6px. Both hold in headless and headed Chromium. In the default headless run, removing the reset fails with Code scrollbar-color is rgb(113, 113, 122) rgba(0, 0, 0, 0), want auto. Removing the :host pin fails with Code scrollbar gutter is 0px, want 6px. The story comment now says the height check only covers hidden scrollbars in CI. All 46 Storybook tests pass with and without --hide-scrollbars.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in ae66299964: removing the reset, the :host pin, or the zero-gutter style each fails OverflowScrollsToLastCard in the default headless run.
🤖
…it-panel' into tracy/git-panel-file-cards
…crollbar thumb visible and the gutter pin platform-aware
…e scrollbar styles in headless runs
|
/coder-agents-review |
There was a problem hiding this comment.
This round checks the round 4 fixes. Three of the four are fixed and verified: the gutter pin now follows the library's per-platform fallback, the thumb stays visible with and without hover, and reverting any part of the classic-scrollbar fix now fails a story in CI. New findings: 1 P3.
The Firefox height mismatch stays open. Both reviewers who raised it checked the defense and found that the subtraction does not need a fixed value (see the reply on that thread). No ticket is linked, so the response needed is a fix in this PR, a linked issue, or an explanation of why the measured-gutter approach does not work.
🤖 This review was automatically generated with Coder Agents.
| // CI runs headless Chromium, which hides scrollbars, so there the height | ||
| // check only covers the hidden-scrollbar case: the story zeroes the gutter | ||
| // because no scrollbar fills it. The style checks before that hold in Chromium | ||
| // whether or not scrollbars are shown, and fail if the FILE_CARD_CSS scrollbar |
There was a problem hiding this comment.
P3 [CRF-54] The story comment says the style checks fail if the FILE_CARD_CSS scrollbar rules are dropped, but deleting the thumb rule (DiffViewer.tsx:120-122) keeps all 26 DiffViewer stories passing. (Netero)
Nothing reads the thumb color, so a regression back to the library's transparent-until-hover thumb would go undetected. In the headless Storybook run, getComputedStyle(code, "::-webkit-scrollbar-thumb").backgroundColor returns rgb(113, 113, 122) with the rule and rgba(0, 0, 0, 0) without it. Add that check next to the scrollbar-color check.
🤖
There was a problem hiding this comment.
Fixed in c373f45: OverflowScrollsToLastCard now also checks that getComputedStyle(code, "::-webkit-scrollbar-thumb").backgroundColor is not transparent. Deleting the thumb rule fails it in the default headless run with Code scrollbar thumb is transparent.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in c373f45be1: deleting the thumb rule makes OverflowScrollsToLastCard fail with Code scrollbar thumb is transparent.
🤖
…it-panel' into tracy/git-panel-file-cards
…de scrollbar from its padding in Firefox
…e scrollbar thumb is visible
|
/coder-agents-review |
There was a problem hiding this comment.
This round checks the round 5 fixes. Both are fixed and verified: the Firefox padding rule subtracts the measured gutter (checked in headless Firefox with overlay scrollbars and a forced gutter value; the classic-scrollbar case relies on your headed measurement), and the thumb check fails when the thumb rule is removed. New findings: 2 P4, 2 Nit, all on the Firefox rule and the comments around it.
Out of scope (needs a ticket or explicit acceptance by a human):
@pierre/diffsdist/utils/scrollbarGutter.js:5:getMeasuredScrollbarGuttercaches the gutter for the page's lifetime, so every consumer of--diffs-scrollbar-gutter-measuredkeeps a stale value after the OS scrollbar mode changes.
Notes:
DiffViewer.tsx:128: no CI test fails when the Firefoxpadding-bottomrule is removed, because the Storybook runner is Chromium-only and hides scrollbars (the runner item from round 4).
🤖 This review was automatically generated with Coder Agents.
| 0px, | ||
| calc( | ||
| var(--diffs-gap-block, var(--diffs-gap-fallback)) - | ||
| var(--diffs-scrollbar-gutter) |
There was a problem hiding this comment.
P4 [CRF-60] The Firefox rule subtracts a gutter that the library measures once per page, so if Firefox switches from overlay to classic scrollbars after the first diff renders, each expanded card is again taller than its metrics until reload. (Hisoka)
getMeasuredScrollbarGutter stores its result in a module-level measuredScrollbarGutter and returns it on every later call, so --diffs-scrollbar-gutter stays 0px after the switch (for example, macOS "Show scroll bars: Automatically" when a mouse is connected). This is inferred from the library source; headless Firefox cannot produce a classic scrollbar. The cache is in the library, so state this limit in the comment above the rule.
🤖
There was a problem hiding this comment.
Fixed in 1fb8f26: the comment above the Firefox rule now says the library measures once per page, so switching between overlay and classic scrollbars needs a reload to correct the heights.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 1fb8f2672f: getMeasuredScrollbarGutter caches its result in a module variable, as the comment now says.
🤖
| // | ||
| // CI runs headless Chromium, which hides scrollbars, so there the height | ||
| // check only covers the hidden-scrollbar case: the story zeroes the gutter | ||
| // because no scrollbar fills it. The style checks before that hold in Chromium |
There was a problem hiding this comment.
P4 [CRF-61] The comment says the style checks fail if the FILE_CARD_CSS scrollbar rules are dropped, but the Firefox padding-bottom rule added in 19edeb1943 has no check, and deleting it keeps every story passing. (Leorio)
The rule sits under @supports (-moz-appearance: none), which is false in the Chromium-only runner. A maintainer who trusts the comment can break the Firefox heights with no failing test. Say that the checks cover the Chromium scrollbar rules and that no CI test covers the Firefox rule.
🤖
There was a problem hiding this comment.
Fixed in 8a0b88c: the story comment now says the checks cover the Chromium scrollbar rules, and that no CI test covers the Firefox rule because the runner is Chromium only.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 8a0b88cc0f.
🤖
| /* The library's Firefox rule keeps the full bottom padding under the code's | ||
| thin horizontal scrollbar, which makes the card taller than its metrics | ||
| whenever scrollbars take space. Subtract the measured gutter as the | ||
| WebKit rule does; it is 0 with overlay scrollbars. */ |
There was a problem hiding this comment.
Nit [CRF-62] "Subtract the measured gutter as the WebKit rule does" points to a rule that does not exist: the subtraction is the library's base [data-code] padding-bottom rule, which applies in every browser until its Firefox rule replaces it with the full gap. (Gon)
The new block copies that base rule exactly. For example: The library's Firefox rule replaces its base [data-code] padding-bottom with the full gap, so the code's horizontal scrollbar makes cards taller than their metrics when scrollbars take space. Restore the base rule; the measured gutter is 0 with overlay scrollbars. (The "WebKit path" wording came from the round 5 reply on the Firefox thread.)
🤖
There was a problem hiding this comment.
Fixed in 1fb8f26 with the suggested wording (restore the base rule).
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 1fb8f2672f: the rule copies the library's base [data-code] padding-bottom expression exactly.
🤖
| so give it the app's scrollbar color. The library measures the gutter | ||
| before these rules apply (15px with classic scrollbars), so pin it to the | ||
| library's fallback: the styled scrollbar height on desktop, 0 on iOS. | ||
| Firefox styles the code with scrollbar-color, so it keeps the library's |
There was a problem hiding this comment.
Nit [CRF-63] "Firefox styles the code with scrollbar-color, so it keeps the library's value" names a property that only sets colors; the measured value is right in Firefox because it ignores ::-webkit-scrollbar and the library's scrollbar-width: thin rule applies when the library measures. (Leorio)
For example: Firefox ignores ::-webkit-scrollbar and sizes the code scrollbar with the library's scrollbar-width rule, which is in effect when the library measures, so it keeps the measured value.
🤖
There was a problem hiding this comment.
Fixed in 1fb8f26 with the suggested wording.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 1fb8f2672f: the library measures a [data-code] element in the same shadow root, so its Firefox scrollbar-width: thin rule applies when it measures.
🤖
…rollbar CSS comments
…llbar rules the story covers
|
/coder-agents-review |
There was a problem hiding this comment.
This round checks the round 6 fixes. All four are fixed and match the @pierre/diffs 1.5.1 source, and all 26 DiffViewer stories pass. New findings: 1 Nit. Both items in this round come from wording in earlier review comments that was imprecise.
The PR description's Firefox bullet still says [data-code] subtracts the gutter "the way the WebKit rule does", which is the wording corrected in the code comment; since the description becomes the squash commit body, say instead that the library's Firefox rule replaces the base [data-code] padding-bottom with the full gap and this rule restores the base rule.
🤖 This review was automatically generated with Coder Agents.
| // CI runs headless Chromium, which hides scrollbars, so there the height | ||
| // check only covers the hidden-scrollbar case: the story zeroes the gutter | ||
| // because no scrollbar fills it. The style checks before that hold in Chromium | ||
| // whether or not scrollbars are shown, and fail if the Chromium scrollbar |
There was a problem hiding this comment.
Nit [CRF-65] "the Chromium scrollbar rules in FILE_CARD_CSS" names the @supports not (-moz-appearance: none) block after one browser, but it applies to every browser except Firefox, including the Safari and iOS path its own comment mentions. (Gon)
Write "the non-Firefox scrollbar rules in FILE_CARD_CSS" so the story and the CSS comment use one name for the block. ("Chromium scrollbar rules" came from the suggestion on the round 6 thread.)
🤖
There was a problem hiding this comment.
Fixed in 40ac0a9: the comment now says "the non-Firefox scrollbar rules in FILE_CARD_CSS". I also corrected the Firefox bullet in the PR description: the library's Firefox rule replaces the base padding-bottom with the full gap, and FILE_CARD_CSS restores the base rule.
Generated by Coder Agents on behalf of @tracyjohnsonux.
There was a problem hiding this comment.
Verified fixed in 40ac0a9329.
🤖
…irefox scrollbar rules consistently
|
/coder-agents-review |
There was a problem hiding this comment.
This round checks the round 7 fix. The story comment now names the non-Firefox scrollbar rules, and the PR description's Firefox bullet describes the base rule correctly. The panel found no new issues, and all 26 DiffViewer stories pass. Every in-scope finding from rounds 1 to 7 is fixed.
The out-of-scope items listed in earlier rounds still need a ticket or explicit acceptance by a human: the global scrollbar-color in site/src/index.css, the @pierre/diffs Firefox padding and thumb-contrast rules, the library's page-lifetime gutter cache, the Chromium-only Storybook runner that hides scrollbars, and the three round 1 items (containerWidth effect timing, the fileTreeUnsafeCSS comment, the RemoteDiffPanel sub-header comment).
🤖 This review was automatically generated with Coder Agents.
Restyles the file list in the Agents git panel diff so each file reads as a rounded card. Stacked on #31029 (per-file collapse).
surface-secondarycard with 6px radius, 2px between cards, and 8px padding from the panel edges.+N −Nstat pill is replaced with plain colored text.GitPanel.tsxrepo header,RemoteDiffPanel.tsxPR/branch sub-header).scrollbar-colordisables the library's::-webkit-scrollbarrules, which left an empty vertical strip beside the code and a 15px horizontal track that card heights did not count. In Firefox the library's rule did not subtract the thin horizontal track. In both cases the end of the list could not be scrolled into view. The code's horizontal thumb uses the app's scrollbar color.Before and after screenshots are not attached: this PR was opened through an API that cannot upload images. The
DiffViewerstories in Storybook show the new layout.Implementation notes
DiffViewer/DiffViewer.tsx,DiffViewer/DiffViewer.stories.tsx,DiffViewer/RemoteDiffPanel.tsx,GitPanel/GitPanel.tsx.CodeView'sunsafeCSS(shadow DOM) plus itslayoutpadding/gap.FILE_CARD_RADIUS,FILE_CODE_INSET,FILE_CARD_GAP, andFILE_LIST_PADDINGdrive the CSS, the inline list padding, and the skeleton.itemMetrics.paddingBottomisDEFAULT_CODE_VIEW_FILE_METRICS.spacingplus the 4px code inset. Without it each expanded card is 4px taller than CodeView computes.FILE_CARD_CSSresetsscrollbar-colortoautoon[data-code]so the library's zero-width vertical and gutter-height horizontal::-webkit-scrollbarrules apply again, and colors the thumbsurface-scrollbar(the library's is transparent until hover). The library measures--diffs-scrollbar-gutteronce, before those rules apply (15px with classic scrollbars), so:hostpins--diffs-scrollbar-gutter-overrideto the library's fallback (6px on desktop, 0px on iOS).[data-code]padding-bottomwith the full gap. Its measured gutter is correct there, soFILE_CARD_CSSrestores the base rule, which subtracts the gutter. Measured in headed Firefox: scrollbar plus padding is 8px with overlay scrollbars and with both classic styles.<pre>, so[data-diffs-header]:not(:has(~ pre[data-diff]))gets the full radius.::beforewith a spreadbox-shadowinsurface-primarypaints only the area outside the rounded header. This assumes the panel behind the list issurface-primary.useLayoutEffectResizeObserveron CodeView's scroll root (from itscontainerRef) comparesoffsetWidthandclientWidth. A classic scrollbar appearing or leaving changes the content box, which fires the observer.OverflowScrollsToLastCard(asserts the code'sscrollbar-color, gutter, and thumb color, then that the last card ends exactly the list padding above the bottom at max scroll),ClassicScrollbarDropsRightPadding(stubsoffsetWidthfor both states and resizes the scroller to re-run the check),CollapsedHeaderIsFullyRounded(expanded6 6 0 0, collapsed6 6 6 6), andStickyHeaderCorners(snapshot only). Each assertion story was checked to fail with its fix reverted.--hide-scrollbarsremoved (real 15px classic scrollbars). The Storybook runner is Chromium only; Firefox was checked with a headed Playwright script.tsc --noEmit,oxlint,biome,lint:compiler,knip, DiffViewer and GitPanel Vitest suites (56 tests), DiffViewer and GitPanel Storybook tests (46).Generated by Coder Agents.