Repository navigation
fix(web): chat transcript no longer draws a stray scrollbar - #218
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
NoahHendrickson
left a comment
There was a problem hiding this comment.
Reviewed commit 5bf691ecbd70530499ef9c7a07a79cf2ad9efe50 against base 3223a4b28d9ba46ed06ebbe58b8be509399a6d99. Fetched PR #218 into the isolated worktree and verified the checked-out HEAD before reviewing.
No actionable findings. The scrollbar rules are scoped to the transcript, both mask declarations remove the obsolete scrollbar layer consistently, and the existing measured gutter compensation accounts for the reclaimed width.
Validation: all 67 tests passed across forkComposerShell, forkTranscriptHover, forkGlassStageCard, and forkCoolDarkerSidebarVibrancy; git diff --check passed. Reviewed the shared web/desktop rendering path; native mobile is unaffected by this CSS change. No browser or visual verification was performed.
Review by GPT-6.1 Sol via the Codex harness in T3 Code.
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code-quality review of 5bf691ecbd70530499ef9c7a07a79cf2ad9efe50.
The diff is small and mostly correct. Removing the third mask layer is the right consequence of hiding the scrollbar. The lane math still holds because ChatCanvas measures offsetWidth - clientWidth, so --chat-timeline-gutter drops to 0 and index.css:2468 gives that width back to the padding. theme.custom.css was already 3,074 lines before this PR and only grows by 4, so there's no file-size concern.
I'm not approving yet, for three reasons:
-
The written reason for removing the scrollbar is false, and a smaller fix exists. The new comment says "the minimap is the transcript's position indicator." But
TimelineMinimapreturnsnullbelowTIMELINE_MINIMAP_MIN_ITEMS(2 user messages) and ishiddenunlesspointer:fine(MessagesTimeline.tsx:1752-1759). So a long thread with a single prompt, which is the most common long transcript, now has no scroll position indicator and nothing to drag. The stated problem is that the rail runs past where the mask ends the content. That problem can be fixed directly by ending the track there:.fork-timeline-cutoff::-webkit-scrollbar-track { margin-block-end: var(--fork-timeline-cutoff-inset); }. The repo already shapes tracks this way ([&::-webkit-scrollbar-track]:my-2inCommandPaletteResultsandModelPickerContent). That is one declaration. It keeps the mask stack, the gutter andChatCanvasexactly as they are, and it removes no capability. If you still prefer hiding the scrollbar, the comment needs to state the real tradeoff instead of a guarantee the minimap doesn't give. -
The cutoff now depends on a new invariant that has no guard. The mask stopped exempting the gutter. That is only correct while
scrollbar-width: noneand::-webkit-scrollbar { display: none }stay on the same scroller..fork/AGENTS.mdrequires a guard asserting the outcome of every customization, andforkComposerShell.test.ts's cutoff test is the obvious home. Assert that the cutoff rule hides the scrollbar in both spellings, so an upstream sync or a later edit that drops one fails CI. Without it, the rail comes back and quietly fades out under the bottom ramp. Thefork-composer-shellintent in.fork/customizations.yamlshould also say the transcript has no scrollbar, because that is now part of what must stay true. (This becomes moot if you take the track-margin route in #1.) -
The rewritten failure-mode comment understates the failure the guard exists to catch. See the inline note. The guard test at
forkComposerShell.test.ts:944still describes "the solid third layer", which no longer exists.
Also, the PR body says the before/after screenshots are still pending, and they are required for UI changes.
| The transcript draws no scrollbar. It ran the full column height at the | ||
| stage's edge, down past where the mask ends the content, so it read as a | ||
| stray rail beside the composer; the minimap is the transcript's position | ||
| indicator. ChatCanvas measures the gutter, so the lane padding absorbs the |
There was a problem hiding this comment.
This premise doesn't hold. TimelineMinimap renders nothing below 2 user messages and is hidden without pointer:fine (MessagesTimeline.tsx:1752-1759). So a long thread with one prompt, which is the typical long agent run, loses its only position indicator and drag target.
The complaint above is that the rail runs past where the mask ends the content. The fix that matches that complaint is to end the track there:
:root[data-fork="noahhendrickson-t3code"] .fork-timeline-cutoff::-webkit-scrollbar-track {
margin-block-end: var(--fork-timeline-cutoff-inset);
}That is the same idea as the existing [&::-webkit-scrollbar-track]:my-2 usages. With it, the third mask layer, the gutter and the ChatCanvas lane all stay as they are, and nothing is taken away. Can we do that instead?
There was a problem hiding this comment.
Agreed that the premise was wrong. The minimap does not stand in for the scrollbar. Hiding the scrollbar entirely was an explicit request, though, so I kept it and rewrote the comment to state the real tradeoff: no position indicator or drag target on a single-prompt run or on touch devices. I've raised the track-margin alternative with the maintainer, and we can switch if they prefer it. Fixed in ebc36ac.
| 100% calc(100% - var(--fork-timeline-top-fade)), | ||
| var(--app-scrollbar-width) 100%; | ||
| 100% calc(100% - var(--fork-timeline-top-fade)); | ||
| scrollbar-width: none; |
There was a problem hiding this comment.
The mask above stopped exempting the gutter in this same change, so this line and the ::-webkit-scrollbar rule below are now what keep the cutoff correct, and no guard pins them. If an upstream sync or a later edit drops either one, the rail comes back and quietly fades out under the bottom ramp. Please add both spellings to the cutoff assertions in forkComposerShell.test.ts, and add one line to the fork-composer-shell intent saying the transcript has no scrollbar.
There was a problem hiding this comment.
Done in ebc36ac. The cutoff test in forkComposerShell.test.ts now asserts scrollbar-width: none on the cutoff rule and display: none on the marker-scoped ::-webkit-scrollbar rule. I confirmed it fails when the declaration is dropped. The fork-composer-shell intent now says the transcript draws no scrollbar.
| covers the element — the transcript paints straight through the composer. | ||
| The guard reads the name out of index.css so a rename fails CI instead. */ | ||
| time and every layer falls back to `auto`, stretching the top fade over the | ||
| whole column. The guard reads the name out of index.css so a rename fails CI instead. */ |
There was a problem hiding this comment.
This rewrite understates the failure. mask-composite defaults to add, and once both layers fall back to auto they each cover the full column. The stretched top fade is near-opaque at the bottom, and the bottom layer is opaque at the top. Each one fills in where the other is transparent, so the cutoff is erased and the transcript paints through the composer again. That is the same outcome the old text described, and it's why the guard exists. "Stretching the top fade over the whole column" reads like a cosmetic issue at the top. Please keep the real consequence here, and update the stale "solid third layer" wording in forkComposerShell.test.ts:944 to match.
There was a problem hiding this comment.
You're right, the layers compose with add and the cutoff disappears. I restored the real consequence (the transcript paints through the composer) along with the reason, and fixed the stale "solid third layer" wording in the guard. Fixed in ebc36ac.
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code review of 5bf691ecbd70530499ef9c7a07a79cf2ad9efe50.
The CSS does what the description says. I checked:
ChatCanvasmeasures(offsetWidth - clientWidth) / 2on the registered scroller and resizes on changes, so--chat-timeline-gutterdrops to 0 and the lane padding absorbs the width the scrollbar gave back.- Removing the third mask layer is right. With no scrollbar, a solid right-edge strip would only let text through beside the composer.
- The companion
.topbar-scroll-faderule still lines up with the two-layermask-sizelist. - The four guard suites named in the description pass locally (67/67).
Two findings, both inline:
- The minimap doesn't always replace the scrollbar. It isn't shown in several common states, and in those the transcript now has no position indicator at all.
- The rewritten comment about the invalid
mask-sizemisdescribes what goes wrong. The real result is still that the transcript paints through the composer.
Before/after screenshots are still pending per the description.
Reviewed by Opus 5.5 via Claude Code in T3 Code.
| 100% calc(100% - var(--fork-timeline-top-fade)), | ||
| var(--app-scrollbar-width) 100%; | ||
| 100% calc(100% - var(--fork-timeline-top-fade)); | ||
| scrollbar-width: none; |
There was a problem hiding this comment.
The scrollbar is hidden in states where the minimap doesn't stand in for it. The new header comment says "the minimap is the transcript's position indicator", but TimelineMinimap (MessagesTimeline.tsx) often isn't there:
- Fewer than 2 user messages: it returns
null(TIMELINE_MINIMAP_MIN_ITEMS = 2). This covers the very common case of one prompt followed by a long agent run, which is exactly the transcript where position matters most. - Touch devices: it is
hiddenunless[@media(pointer:fine)]. Mobile web and iPad lose the overlay scroll indicator, becausescrollbar-width: nonesuppresses that too. - Narrow chat column: when the side gutter is under 48px (right panel open, narrow window), it sits at
opacity-0until hovered. Under ~26px the hit strip also stops being interactive.
Even when it is visible, the minimap marks user turns and doesn't track scroll position, and the drag-to-scrub affordance is gone everywhere.
If the real problem is the rail running down past the cutoff beside the composer, a narrower fix might keep the scrollbar and end its track at the cutoff. For example, give ::-webkit-scrollbar-track a margin-block-end of var(--fork-timeline-cutoff-inset), or mask only the lane below the cutoff. Otherwise, hide the scrollbar only when the minimap is actually persistent (data-persistent-gutter="true", pointer:fine, ≥2 items).
There was a problem hiding this comment.
Agreed that the premise was wrong. The minimap does not stand in for the scrollbar. Hiding the scrollbar entirely was an explicit request, though, so I kept it and rewrote the comment to state the real tradeoff: no position indicator or drag target on a single-prompt run or on touch devices. I've raised the track-margin alternative with the maintainer, and we can switch if they prefer it. Fixed in ebc36ac.
| time, every layer falls back to `auto`, and the solid third layer then | ||
| covers the element — the transcript paints straight through the composer. | ||
| The guard reads the name out of index.css so a rename fails CI instead. */ | ||
| time and every layer falls back to `auto`, stretching the top fade over the |
There was a problem hiding this comment.
Nit: this explains why the guard exists, but it describes the wrong failure. When mask-size goes auto, both gradients cover the whole scroller, and the layers combine (mask-composite: add):
- Layer 1 (
to bottom…black 100%) is opaque at the bottom. - Layer 2 is opaque at the top.
The combined mask is therefore close to fully opaque everywhere. The cutoff disappears and the transcript paints straight through the composer, just as before. The top fade doesn't get stretched over the column. The old consequence ("the transcript paints straight through the composer") still holds; only the reason changed (no solid third layer any more). Worth keeping that wording so nobody downgrades the guard as cosmetic.
There was a problem hiding this comment.
You're right, the layers compose with add and the cutoff disappears. I restored the real consequence (the transcript paints through the composer) along with the reason, and fixed the stale "solid third layer" wording in the guard. Fixed in ebc36ac.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
The chat transcript's scrollbar ran the full column height at the stage's outer edge, down past where the cutoff mask already ends the messages, so it read as a stray rail beside the composer.
The transcript scroller now hides its scrollbar (
scrollbar-width: noneplus::-webkit-scrollbar { display: none }). Wheel/trackpad scrolling, "Scroll to end" and the minimap are unchanged. The cutoff mask's third layer existed only to keep the 6px scrollbar lane unmasked; with no scrollbar it would have let text through beside the composer, so it is removed. ChatCanvas measures the scroller's gutter, so the lane padding absorbs the width the scrollbar gave back and the transcript stays aligned with the composer.Scope: the transcript only. Settings, the right panel and other stage scrollers keep their scrollbars.
Verified with the fork guard tests touching the cutoff mask (forkComposerShell, forkTranscriptHover, forkGlassStageCard, forkCoolDarkerSidebarVibrancy — 67 passing). Before/after screenshots still to be added.
Opus 5.5 via Claude Code in T3 Code.
🤖 Generated with Claude Code