Repository navigation
feat(ui-web): scroll the transcript under a frosted chat header - #1151
Conversation
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the full diff and surrounding chat-column layout, the turn-nav and previous-user jump callers, desktop-shell and trajectory variants, and the two commits' history. I also checked the repository rules and Web UI context, import/DOM-touch architecture, backward behavior when the header is absent, and that the added tests preserve the existing cases rather than weakening them.
Verification: npm test (261 files, 4,512 tests passed), npm run type-check (passed), npm run lint (passed with four pre-existing warnings in untouched files), npm run gen:check (passed), npm run build plus uv run python ui-web/build.py and node ui-web/scripts/check-page.mjs (passed, including both boot snapshots). git diff --check and the merge-tree check against github/main also passed.
|
Windows compatibility review — PR #1151 @ Scope: the full diff at this head against base Result: no Windows-specific defect found by reading. The change is layout, Verification: no executable check was possible for the UI — the shared Coverage gaps: the visual result (frost over a scrolled line, the fade at the band's lower edge, the shell's |
6a82931 to
f35ecf4
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the UI test suite must pass with the new quick-jump landing position.
The rebased first two commits are patch-identical to the revision I previously reviewed. I reviewed the new frost-ramp commit, the final combined diff, and the latest comment-only follow-up. I covered the repository and Web UI rules, layout and import/DOM-touch architecture, affected jump callers, desktop-shell and trajectory variants, backward behavior without a mounted header, history/range comparison, and whether tests were weakened.
Verification: the full npm test run failed with 1 failure and 4,524 passes; on this exact head, npm test -- src/features/composer/ComposerPage.test.tsx reproduced the failure (1 failed, 69 passed). npm run type-check, npm run gen:check, and npm run build passed. npm run lint passed with four pre-existing warnings in untouched files. git diff --check and the merge-tree check against github/main passed.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The only delta updates the existing quick-jump integration assertion to the intentional 16px lead, resolving the failing gate without weakening or replacing coverage. I reviewed the delta against the full diff, caller behavior, history, and the repository/Web UI test and architecture rules. My prior blocker is fixed, replied to, and resolved; no other thread I opened remains outstanding.
Verification: npm test passed all 262 files and 4,525 tests. npm run type-check passed. npm run lint passed with four pre-existing warnings in untouched files. git diff --check passed.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the new split-layer frost delta at 160983f against the full PR and prior head. The blur-only ::before and tint-only ::after remain inside the header stacking context, preserve the existing geometry, and keep the opaque fallback when backdrop-filter is unavailable. The updated ChatTop tests strengthen coverage for both layers rather than weakening an assertion.
Covered the repository and Web UI rules, full diff and range history, affected callers/stacking context, backward compatibility and fallback behavior, test integrity, and the existing architecture constraints. Verification: git diff --check passed; npm test passed 262 files / 4,525 tests; npm run type-check passed; npm run lint passed with 0 errors and 4 pre-existing warnings outside this diff; npm run build passed. The head still matches 160983f, and no settled thread opened by me remains unresolved.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the frost-ramp revision and the test-only follow-up that landed while the review was running, through current head 530ec59. The translucent theme tokens, blur/tint ramps, header geometry consumers, ghost-button state specificity, and unsupported-backdrop fallback are internally consistent. The follow-up pins all four light/dark theme declarations and strengthens rather than weakens coverage.
Covered the repository and Web UI rules, full diff and range history, affected callers and selector interactions, backward compatibility/fallback behavior, test integrity, and architecture constraints. Verification on 6a955e2: git diff --check; focused tests 93/93; full UI suite 262 files / 4,525 tests; type-check; lint with 0 errors and 4 pre-existing warnings outside the diff; production build. Verification after the test-only 530ec59 follow-up: git diff --check; ChatTop tests 23/23; type-check; lint with the same 0 errors and 4 warnings. No settled thread opened by me remains unresolved.
|
Windows compatibility re-review — PR #1151 @ Scope: the PR's own diff at this head is Windows angle: none found. The change is DOM measurement, scroll positioning and CSS tokens; Verification: read the complete delta — Coverage gaps: the frost band (row height + fade tail) and the scroll landing were not rendered in a browser on Windows; this review rests on reading. No Windows-specific issue found. |
530ec59 to
6018c94
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the rebased revision through 6018c94, including the three new trajectory-integration commits after the patch-equivalent prior head. The trajectory glass island now clears the full frost band, while the parked transcript keeps the live scroll box's inset-0 geometry; the added cases cover those paths plus the up pill, turn nav, and shell-measured header row. I found no runtime or compatibility regression, and the tests strengthen rather than weaken the existing contracts.
Covered the repository and Web UI rules, refreshed target diff and range history, affected layout/state callers, backward compatibility and backdrop fallback, test integrity, and architecture constraints. Verification: full UI suite passed 273 files / 4,655 tests; npm run type-check passed; npm run lint passed with 0 errors and 4 pre-existing warnings outside the diff; npm run build passed. git diff --check reports only a trailing blank line in ChatTop.test.tsx, with no functional impact. The remote head matches 6018c94, and no settled thread opened by me remains unresolved.
|
Windows compatibility re-review — PR #1151 @ Scope: the branch is rebased onto main Windows angle: none. The change is CSS geometry and DOM measurement ( Verification (native Windows 11): a detached worktree
This closes the earlier "ui-web vitest blocked" gap for the files this PR touches; the test run is the same code path on Windows as on CI, with only the browser engine's layout differing by platform. Coverage gaps: no browser rendering of the frost band or the island was done here; the rest of the ui-web suite and the CSS gate scripts were not run (the PR's own diff does not change the gate files); the full Python suite is not relevant to this PR. No Windows-specific issue found. |
The chat header used to be a flex item above the scroller, so a message scrolled to the top was clipped at the header's edge. It is an overlay over the whole column now: the transcript passes underneath and is read through a semi-transparent ink tint with a 20px backdrop blur, and the band's last 20px fade out so the boundary is a gradient rather than a cut. The scroller gives the band's height back as top padding and scroll-padding, so the resting frame does not move and only a scrolled one passes under. The header height moves to --topbar-h (52px, or the shell's measured --tl-row), which the parked scroller, the up pill and the trajectory box also read. The up pill and the turn nav land a question below the band instead of at the scroller's edge, so a jump cannot hide what it brought into view. Where backdrop-filter is unsupported the band falls back to the opaque page ground the header had before.
The import-direction gate holds chrome below features, and the state-dom-touch gate caps how many modules may reach for the document, so a helper both islands import could not live under chrome/behaviour. It sits beside $ in lib/dom.ts: the page's one sanctioned selector, and the file the touch gate exempts.
The band used to be the header's own height at one tint with a 20px tail, so a scrolled message met a hard edge and only the last 20px softened. It now paints past the row (--topbar-fade-tail below it) and its mask runs from opaque behind the controls to nothing at the lower edge. Because the mask fades the filtered backdrop too, the blur itself thins out down the ramp: a bubble is eaten a little more per pixel instead of emerging at a cut. The ground is plain --page instead of a color-mix, which removes the window where backdrop-filter was supported but color-mix was not, the background was dropped and the title sat on a bare blur. The readable edge is --topbar-fade-h (row plus tail). scroll-padding, the up pill, the trajectory host and the turn nav use it; headerBand() returns it by measuring the row and reading the tail token, and both jumps leave the same 16px lead. The rules are scoped to .chat > .top, the selector headerBand() uses. Tests pin the fade token, the mask shape, the fallback body, the trajectory host's padding and the parked copy's inset; each was watched failing on its own mutation.
HEADER_LEAD is shared by both jumps now, so the pill lands 16px lower than the old expectation. The turn nav's test already asserts the same number.
An opaque ground on the same element as the backdrop-filter hides the blur on engines that paint the element's background over the filtered backdrop, which is the order the spec describes; the band there would be a paler sheet of sharp text rather than frost. The blur now has its own transparent layer, masked away over the band's last 22px, and the tint keeps the --page ground on a second layer behind the controls. Tests pin the split: the blur layer must carry no background, the tint layer must carry --page, and the fallback turns the tint opaque and drops the blur.
The band painted an almost opaque --page ground, so a bubble under it was washed out instead of frosted. It is now glass: a weak --topbar-tint (0.3 of --page) that unifies the tone and fades out below the controls, over a blur layer whose mask walks from full behind the controls to none at the lower edge. The colour and shape of a bubble stay visible through it, the way a title bar shows the window behind it. The legibility an opaque ground used to buy is now carried by the controls: a --topbar-halo shadow behind the name and the chip, and a page-coloured plate under each corner button. The plates ride on a --ghost-bg variable so :hover and the pressed toggle still win. The tail grows to 56px so the sharpening is gradual rather than a step: the band is 108px, with the blur full to 44px and then 0.8 at 68, 0.45 at 88, 0.18 at 100 and 0 at 108. The jumps keep landing 16px under it. Tests pin the low tint alpha, both layers, the halo and plate rules and the fallback; each was watched failing on its own mutation.
Each token is declared in the light defaults, the media dark block and both viewer overrides; a value that drifts in one of them is invisible until someone opens that theme. The test pins the declaration count and the shared alpha.
The trajectory view's host is the waterfall's glass island: the frost band is its top margin now, not padding inside it, so the card's top edge and rounded corners sit below the band instead of underneath it. The content position is unchanged.
The parked copy now uses inset 0, the live box's geometry, so it keeps the size and content offset the transcript had while the trajectory view is up. The comments and the test say the same thing the code does.
…s last The header-band cases now cover the up pill, the turn nav and the shell's measured title row, and move above the language describe that has to stay last. A mutation that reverts the island margin or the shell row turns them red.
The frost is the header row now, not the row plus a 56px tail. Remove the fade tokens, use --topbar-h for the range, and let scroll-padding, the up pill, the turn nav, the trajectory host and headerBand read the row. The parked copy keeps the live box's geometry.
The blur and tint masks are a continuous ramp across the row: full at the top, clear at the bottom, so a message under the title bar is eaten more the higher it sits. The new test parses the stops and requires a monotonic ramp with intermediate strength.
The blur rises to blur(44px) and holds nearly full strength through the top quarter before falling to clear at the row's lower edge; the test pins the max and the .9 at 25 percent and .3 at 75 percent stops.
6018c94 to
c778b2f
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the rebased revision through c778b2f, focusing on the three new frost-shaping commits after the patch-equivalent prior head. The frost, scroll padding, jump geometry, turn nav, up pill, and trajectory inset now consistently use the title-bar row height; both blur and tint ramps end clear at that row's lower edge, including the shell-measured height and unsupported-backdrop fallback. The new ramp tests strengthen rather than weaken the contract.
Covered the repository and Web UI rules, refreshed target diff and range history, affected callers and selector/layout interactions, backward compatibility/fallback behavior, test integrity, and architecture constraints. Verification: git diff --check passed; full UI suite passed 273 files / 4,655 tests; npm run type-check passed; npm run lint passed with 0 errors and 4 pre-existing warnings outside the diff; npm run build passed. The remote head matches c778b2f, and no settled thread opened by me remains unresolved.
|
Windows compatibility re-review — PR #1151 @ Scope: the branch is rebased onto main Windows angle: none. This is CSS and one DOM measurement; Verification (native Windows 11): a detached worktree
Coverage gaps: the frost and its lower edge were not rendered in a browser here; the rest of the ui-web suite and the CSS gate scripts were not run because this delta does not change them. No Windows-specific issue found. |
The reading line was still a fraction of the scroller's border box. With the header out of the flow that box starts at the column top and is one header row taller, so the line rose by row * (1 - READ_LINE) and the active turn advanced about 34px early. Measure it from the header band so it keeps the position it had before the header left the flow.
The headerBand comment claimed a missing header keeps every layout-free test on its old numbers. That holds for the turn nav, whose lead was already 16px, but not for the up pill, which gained the shared lead, so its tests moved by that much.
ZuyiZhou
left a comment
ZuyiZhou
left a comment
There was a problem hiding this comment.
Read the two commits after gloryfromca's last reviewed head (c778b2f) at bd34c8a: the turn-nav reading line is now band + (height - band) * 0.35 measured from the header band, which restores the position it had before the header left the flow, and a new test pins a turn that only the band-relative line counts; the second commit is a comment fix. CI is green at this head.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the two-commit delta from c778b2f and the full diff against main. The turn-nav reading line now measures 35% through the transcript area visible below the floating header, which algebraically preserves its pre-overlay position; the new integration case distinguishes that geometry from the old whole-scroller calculation. The accompanying headerBand documentation correction matches the actual turn-nav and up-pill behavior.
Coverage included AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, ui-web/CONTEXT.md and ui-web/CONTRIBUTING.md; the full diff and incremental delta; the headerBand caller and TurnNav history; backward/layout compatibility including the no-header test path and shell-sized band; architecture/import direction; and confirmation that tests were strengthened rather than weakened. I found no concrete failure to raise.
Verification: git diff --check passed; the targeted TurnNav suite passed 19/19; the complete UI suite passed 273 files and 4,656 tests; type-check passed; production build passed; lint passed with 0 errors and the four existing warnings in untouched files. An initial concurrent full-suite run had four failures in untouched session tests due resource contention (three timeouts and one dependent assertion); rerunning the suite alone passed completely. The remote head matched this commit. GitHub reported the PR merged while this review was in progress.
Summary
The chat header was a flex item above the transcript scroller, so a message scrolled to the top was clipped at the header's edge. The header is now an overlay over the whole chat column, and the transcript travels underneath it through a frost that strengthens from the row's lower edge to its top.
The frost is the header row itself (--topbar-h: 52px; the desktop shell's measured title bar). It is two layers. A blur layer with no ground of its own carries backdrop-filter: blur(44px) saturate(1.4), and its mask walks the blur from full at the top (1 at 0%) through .9 at 25%, .62 at 50%, .3 at 75% to none at the row's lower edge (0 at 100%). A tint layer carries --topbar-tint, a third of --page at most, with a matching upward ramp (1 at 0%, .7 at 25%, .45 at 50%, .2 at 75%, 0 at 100%). The band is glass: a bubble under it stays visible as colour and shape, eaten more the higher it sits, and it is the blur that eats it, not a painted ground. Neither layer uses color-mix.
The split matters because an engine that draws an element's background over the filtered backdrop, the order the spec describes, would hide the blur entirely under an opaque ground and leave only a paler sheet of sharp text. Measured in Chrome, a mask of 0 leaves the backdrop untouched and a mask of 1 draws the full blur.
Legibility over a dark bubble is carried by the controls, not by the tint: a soft --topbar-halo shadow behind the name, the chip and the corner glyphs, and a page-coloured plate under each corner button, so the band can stay translucent instead of turning opaque.
The readable edge is --topbar-h (the row; the shell's measured title bar). scroll-padding-top, the up pill (row + 12), the trajectory host (row + 6) and the turn nav (row) read it, and headerBand() returns it by measuring the row. Both jumps leave the same 16px lead, so the first line after a jump is clear of the frost; measured in the page, a turn-nav click lands the question at y=68 on the 52px row. The scroller's padding-top stays the row height, so the resting frame does not move. The turn nav's reading line is measured from the title bar's lower edge as well, so it keeps the position it had before the header left the flow: the scroller's box grew by the row, and the old fraction of it would have moved the line up about 34px.
Where backdrop-filter is unsupported the whole row is painted with the opaque --page ground and no mask, so content is hidden under the row rather than shown through a blurless tint.
The trajectory view's host is the waterfall's glass island: its top margin is the row plus its own 6px inset, so the card's top edge and rounded corners sit below the frost; its content stays where it was. The parked scroller is inset 0, the live box's geometry, so it keeps its size and scroll position. The branch is the feature, the follow-up that moves the header-height helper into lib/dom.ts, the previous review fixes (the island's top edge, the parked copy, the band's other readers), and this round's fixes: the frost is limited to the title bar row, it ramps upward continuously, and its top reaches 44px.
Type
Verification
npx vitest run src/chrome/ChatTop.test.tsx src/features/composer/userjump.test.ts src/features/transcript/TurnNav.test.tsx --maxWorkers=2gives 3 files and 53 tests passed.npx vitest run src/features/composer src/features/transcript --maxWorkers=2gives 16 files and 506 tests passed;npx vitest run src/chrome --maxWorkers=2gives 10 files and 177 tests passed.npx vitest run --no-file-parallelism scripts/gatesgives 57 files and 273 tests passed.npx vitest run --maxWorkers=2gives 273 files and 4,656 tests passed.npx eslintover the seven changed TypeScript files reports no issues;npm run type-checkexits 0;vite build,build.py,check-page,check-css,check-class-namespaceandgen:checkexit 0.The up pill's landing assertion in ComposerPage.test.tsx moved from 400 to 384: the shared 16px lead applies to both jumps, and its expectation encoded the old landing.
Mutation proof, each mutant reverted after its run: the new upward-ramp test is red with the old in-row fade (only the lower 12px changed) and green with the continuous mask; the blur assertion is red while the CSS is 28px and green at 44px; the top-heavy stop assertions are red without .9 at 25% and .3 at 75%. The previous round's island-margin and shell-row mutants still turn their tests red; headerBand ignoring the row fails the jump test. The reading line's band-relative case turns red with the scroller-box formula (the 370px turn is not counted as reached) and green measured from the band.
Chrome, real page: band 52px; blur(44px) saturate(1.4); blur mask 1 / .9 at 25% / .62 at 50% / .3 at 75% / 0 at 100%; tint mask 1 / .7 at 25% / .45 at 50% / .2 at 75% / 0 at 100%; scroll-padding-top 52px; up pill 64px; trajectory island top margin 58px; turn nav top 52px. Shell tokens give a 38px row, a 38px scroll-padding, a 50px pill, a 44px trajectory margin and a 38px turn nav.
A turn-nav click in the real page lands the question at y=68 (row 52 plus the 16px lead); the first line is clear of the frost.
At scrollTop 0 the first turn starts at y=88, below the 52px row: its top edge is sharp; a message under the title bar is eaten more the higher it sits and is sharp again below the row.
The frost tokens are declared four times (light defaults, media dark, dark override, light override); the test pins the count and the shared 0.3 alpha, and a single drifted block turns it red.
check_source_language.py,check_large_files.pyandcheck_commit_messages.pyover the three-dot range exit 0; the pre-commit hooks over origin/main..HEAD pass.CI on the new head is the run to read; this branch touches only ui-web files.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
User-visible: the frost is the title bar's 52px row, not a taller band. Its strength falls from full at the top to clear at the row's lower edge, so a scrolled message is progressively eaten while it is under the title bar and is sharp again below it. The band is translucent: a bubble under it stays visible as colour and shape. The corner buttons gained a page-coloured plate and the name a soft halo so they stay readable over a dark bubble. The up pill, the turn nav and the trajectory host clear the row; the trajectory island's top edge sits below the row at its 6px inset. The parked scroller keeps its live size. The resting layout is unchanged, and the first turn's top edge at y=88 is below the row, so it is sharp.
Performance: one backdrop-filter over a 52px strip, recomputed while the transcript scrolls.
Known gaps: WebKit was not available on this machine, so the -webkit- path was not run; the mask's effect on backdrop-filter was measured in Chrome only. The overlay scrollbar thumb's track now starts at the top of the chat column (the scroller's box used to start below the row), so the thumb can be drawn over the band's right edge; the header's controls are inset from that edge. This is new with the overlay header, not a pre-existing state.
Rollback: revert the commits; the change is confined to styles/page.css, one feature stylesheet, two jump helpers, one lib helper and the tests that pin them, with no data or wire changes.
Related Issues
N/A