Skip to content

🤖 perf: hold the first row in view while scrolled up, before paint (#6773) - #6959

Merged
ThomasK33 merged 5 commits into
mainfrom
tk/f20-top-row-hold
Oct 18, 2026
Merged

ThomasK33 merged 5 commits into
mainfrom
tk/f20-top-row-hold

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Oct 18, 2026 •

Copy link
Copy Markdown
Member

Summary

While the reader is scrolled up, a row whose top is above the view and that grows (KaTeX arriving late, a lazy Mermaid diagram, a late image) no longer pushes the rows below it down. useAutoScroll now keeps its ResizeObserver connected while released and holds the first row that starts inside the view at its on-screen top, before paint. Fixes #6773 (F20 part 2b-2b-2, #4961).

Background

Chrome anchors to the visible part of a partly visible top row. Growth below that anchor, inside the same row, is outside what native scroll anchoring compensates, so every row below moved down by the growth. On main the observer that re-pins the bottom disconnected while scrolled up, so no code reacted.

Implementation

  • useReadingRowHold (in useAutoScroll.ts): snapshotReadingRow records the first [data-message-id] row whose top is inside the scrollport (binary search, O(log n) rect reads) and its top. holdReadingRow moves scrollTop by however far that row moved, then records again.
  • The ResizeObserver callback calls stickToBottom() while locked (unchanged) and holdReadingRow() while released. Snapshots are taken on every scroll event while released and on disableAutoScroll.
  • It measures a position, not a size. When native anchoring or ChatPane's Load older/Show newer anchor already restored the row, the delta is 0 and it writes nothing. A hold also updates the scroll-direction baseline, so it is never read as the reader scrolling toward the bottom.
  • Intended limit: growth inside the first row in view, or below it, is normal top-down flow and is not compensated. With no row starting in view (one tall row fills it), the hook does nothing and native anchoring stays in charge.
  • No ChatPane change. The lock, release and relock rules are unchanged.

Validation

  • Tests first (first commit): tests/ui/chat/partialTopRowGrowth.test.ts grows the partly visible top row by 397 px and fails on main (the next row moved: expected 44, received 441). Its control (growth of the first row in view) passes on both. Two hook controls in useAutoScroll.test.tsx: rows mounted above with the caller's anchor correction (Load older) move nothing more, and with no row starting in view the hook writes nothing. happy-dom has no layout, so the tests use a layout model; the browser proof is below.

  • Browser proof (release builds, headless Chrome 155, xum server, a synthetic 30-row demo chat): scrolled up (wheel) so a row shows only its bottom 44 px at the top.

    Growth Width main: next row moved head
    397 px <div> inserted into the row 1800 px 413 px 0 px
    397 px <div> inserted into the row 390 px 413 px 0 px
    real KaTeX upgrade (chunk held 8 s by CDP Fetch), row +215 px 1800 px 215 px 0 px
    real KaTeX upgrade, row +148 px 390 px 148 px 0 px

    Head after the 397 px insert (1800 px): the inserted block sits above the view and "Demo question 18" stays put.

    head 1800 px after the 397 px insert

    Main after the delayed KaTeX upgrade (1800 px): the upgraded math row pushed the rows below it down by 215 px.

    main 1800 px after the KaTeX upgrade

    Head after the same upgrade (1800 px): the rows below stay put.

    head 1800 px after the KaTeX upgrade

  • React Compiler: useAutoScroll and useReadingRowHold both compile (CompileSuccess, no CompileError). make check-react-compiler 23/24, the same as main.

  • make static-check, make crap-check (PASS; the hold is its own hook, so useAutoScroll's CC does not grow), useAutoScroll.test.tsx (29 pass), tests/ui/chat/{partialTopRowGrowth,composerDockAnchoring,bottomLayoutShift}.test.ts (9 pass).

  • Perf gates (local host, every run started at load1 < 10, one run per hold of the shared measurement lock; main = 83a8168, head = edbceef, whose product code is the same as 7900d92):

    • f20 Load-older harness, 40 presses per run, main and head alternating (ABBAAB): 21 of 24 runs done. 1240k has 1 main and 2 head runs; the host stayed busy (load 40-95) after 18:50 UTC.

      size main: max task per press (runs) head: max task per press (runs) heap after GC at press 40, main / head
      th20k 208, 148, 126 ms 116, 361, 139 ms 198-199 / 198 MB
      100k 198, 154, 401 ms 168, 110, 139 ms 166 / 166 MB
      500k 158, 164, 123 ms 134, 128, 187 ms 166 / 166 MB
      1240k 140 ms 130, 155 ms 166 / 166 MB

      Head: 1 of 320 presses over 200 ms (th20k press 38 of one run, a reveal at 361 ms). Main: 5 of 280 (one 100k run had 4, at 206-401 ms; one th20k press at 208 ms). Heap: 198 MB or less on both sides (limit 250 MB). Mounted rows end at 1,198-1,199 on both sides.

    • perf.chatSwitch + perf.workspaceOpen (Electron, 3 runs per side, ABBAAB): head medians are within run noise. They are equal or faster except workspace-open-large (data-loaded 775 -> 837 ms, run ranges 752-861 vs 739-846, overlapping) and chat-switch switch-back-small in-process caughtUp (55 -> 66 ms, non-overlapping runs). Neither path runs the hold: the transcript stays locked on open and on switch. The full table is in the lane evidence (e2e-table.md).

Risks

Medium-low, scroll behavior while scrolled up. While released, each ResizeObserver delivery and scroll event now does a querySelectorAll plus about log2(rows) rect reads. A wrong delta would move the reader's view; the hold is skipped when the held row is gone and re-snapshots after every scroll.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high • Cost: $8.95

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-18T13:22:00.911888Z 7900d92 Manual request
🔒 Security Review ✅ Completed 2026-10-18T13:23:14.659263Z 7900d92 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 714d503afc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/hooks/useAutoScroll.ts Outdated
Comment thread src/browser/hooks/useAutoScroll.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 3 of at most 3. The only change since your clean review of edbceef is 7900d92 (a test binds two saved rect getters for the unbound-method lint rule).

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 7900d92e13

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 7900d92e13

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@ThomasK33

Copy link
Copy Markdown
Member Author

Perf owner: #6959 is READY on review (Codex clean on 7900d92e13, Required pass, 0 threads) and the browser proof passes (row below a growing partly visible top row: main 413 / 215 / 148 px, head 0 px). Two perf gates are incomplete because the shared host stayed loaded. Decision: Option B, rerun at a quiet host before merge (I run the lane's restartable scripts myself: 3 missing 1240k f20 runs + 3 extra th20k head runs, then a second Electron chatSwitch/workspaceOpen block). Rules fixed before the data:

  1. f20: no head press > 250 ms in the new runs (the earlier 361 ms th20k press counts as noise only if none of the 3 new th20k head runs exceeds 250 ms); heap after GC <= 250 MB.
  2. Electron: pooled medians of both blocks (6 per side) for switch-back-small caught-up and workspace-open-large data-loaded within +5% or +10 ms of main, whichever is larger.
    Both pass -> I queue. Either fails -> I post the numbers and decide with the lane.

@ThomasK33

Copy link
Copy Markdown
Member Author

Perf owner: quiet-host rerun results (lane scripts, run by me):

  • f20 new runs: 1240k head max press 155 / 169 ms (0 over 200); th20k head 282 (1 press, start load 9.85 / CPU PSI 2.32), 119, 124. Rule 1 (no new head press > 250 ms) fails as written. Main shows the same tail on its own (401 ms at 100k with 4 presses > 200, 208 at th20k), so rule 1 (max of 40 presses per run) cannot separate host stalls from the hold. Heap after GC 166-199 MB both sides.
  • Electron, pooled 6 per side: switch-back-small in-process caught-up 55 -> 63 ms (+8 <= +10, pass; but every head run is at or above main's median, tracked below); workspace-open-large data-loaded (fullyLoadedMs) 818 -> 792 (pass). firstMessage 405 -> 429 is not a named metric (runs overlap).
    Decision (independent advisor agreed): no override. One more interleaved block, declared now: th20k ABBA main/head x3 each, start load recorded per run. If head has a press > 250 ms in more runs than main -> treat as real, lane investigates the hold's per-press cost. Otherwise ready. Tracked follow-up either way: count ResizeObserver callbacks / hold time on switch-back-small to explain the consistent +8 ms.

@ThomasK33

Copy link
Copy Markdown
Member Author

Perf owner: interleaved th20k block (ABBA, 3+3) result: main max press 155 / 235 / 111 ms (0 runs over 250); head 122 / 119 / 317. The 317 ms head run is disturbed: the queue's quiet check passed at lock time, but the run's start line shows load 27.92 and CPU PSI 19.57 (host load rose to ~31 during it), against the declared quiet-host precondition (load < 10, PSI < 10). I am not dropping it by judgment: I replace that one run with a single head th20k rerun under the same precondition and the same rule (head runs with a press > 250 ms must not outnumber main's). Replacement result decides.

@ThomasK33
ThomasK33 added this pull request to the merge queue Oct 18, 2026
@ThomasK33

Copy link
Copy Markdown
Member Author

Perf owner readiness record: QUEUED at 7900d92e13 (--match-head-commit, no --auto). Required pass, 0 open threads, Codex 3 rounds, last clean on this head (code + security, no unresolved findings). Browser proof: row below a growing partly visible top row moved 413 / 215 / 148 px on main, 0 px on head. Perf gates after quiet-host reruns: Electron pooled 6+6 pass (switch-back-small caught-up +8 ms <= +10; workspace-open-large data-loaded 818 -> 792); f20 interleaved th20k block main 0 / head 0 runs with a press > 250 ms after the declared replacement of one disturbed run (load 27.92; replacement 119 ms). Clean-context reviewer: ready with tracked follow-ups (on #4961). Lane spend 9.17 against a 9 stop (overrun reported to the user).

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 18, 2026
@ThomasK33
ThomasK33 added this pull request to the merge queue Oct 18, 2026
@ThomasK33

Copy link
Copy Markdown
Member Author

Perf owner: DROPPED 23:06Z (merge-group run 39174280603, Test / Storybook): RightSidebarTabStrip.stories.tsx single-tab story (new in #6995) timed out with the story still in sb-preparing-story ("Unable to find role=dialog name 'All tabs'"). #6959 touches only useAutoScroll and its tests; main's own run on 8d53b12 passed. Treated as a flake: RE-QUEUED once at the same head. A second drop on that story -> I file it against #6995's story.

Merged via the queue into main with commit 965aa79 Oct 18, 2026
61 of 64 checks passed
@ThomasK33
ThomasK33 deleted the tk/f20-top-row-hold branch October 18, 2026 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🤖 perf: growth inside a partly visible top row moves the rows below it (scroll anchoring)

1 participant