fix(chat): drop a waiting follow scroll when PageUp leaves the end by devswha · Pull Request #985 · devswha/ovis

devswha · 2026-10-13T19:30:50Z

Problem

scripts/chat-virtual-browser-qa.ts flakes at line 169, assert.ok(away.bottom > 1 && away.anchor), right after window.chatQa.page(-1). It failed in the light variant after the same steps passed in dark (run 38423674546), and on main for d046b99 (run 38424958299). Every chat-virtual failure in the last ~100 CI runs is at this line.

Root cause

This is a product race, not a test-only one. ChatView asks the list to follow the end with timeline.current.scrollToEnd({ animated: false }) while the reader is at the end. Here the ask comes from the ResizeObserver after the composer-height resize; a streamed revision does the same. In @legendapp/list 3.3.5 that call does not scroll at once. It stores pendingScrollToEnd, re-renders the list, and runs the request in the list's next layout effect, after up to 800 ms more if the layout is still settling after a data change. On a loaded runner that commit can come after the test's PageUp. PageUp scrolls the native node up by a page, but the waiting request then runs and puts the view back at the end. ChatView's onScroll sees distance 0 and turns follow back on.

A probe that traced every scrollTo/scrollTop write on the page caught it:

48546.4 scrollTo {"top":118287.75,"behavior":"instant"}   <- PageUp (ChatView page scroll)
75082.3 scrollTo {"behavior":"auto","top":119163}         <- ~200 ms later, back to the end
    at scrollToEnd ... at runScrollRequest ... at state.runPendingScrollToEnd
    at commitHookLayoutEffects ...                         (the list's deferred commit)

Find, minimap ticks and restored places scroll through the list API, and any newer list request replaces a waiting one (startImperativeScroll clears pendingScrollToEnd). PageUp is the one path that scrolls the native node directly, so it never replaced it.

Change

ChatView.tsx: local dropPendingListScroll(list) (keeps the timeline lazily loaded). It calls list.scrollToItem({ item: NO_ROW, animated: false }) with a frozen object that is never in data. The list treats this as a newer request: it bumps its request token and clears pendingScrollToEnd right away. The run then finds no index and returns false without setting scrollingTo, so nothing moves. 3.3.5 has no public cancel API, and this is the only request that replaces a waiting one without scrolling. The comment in the code explains it.
ChatView.tsx: the PageUp path (direction < 0, view actually moved) calls it before onScroll().

Why only PageUp, and not every upward scroll in onScroll: an upward scroll event is ambiguous. The list's own MVCP and prepend adjustments also make scrollTop go down, so dropping a follow on any "scrolling up" event would also drop follows that should land. PageUp is clear reader intent and runs synchronously before any scroll event can come in between. A wheel or touch move away from the end can still hit the same window. That case is left as it is.

The test is unchanged: its assertion was right and the product was wrong.

Validation

Throttled probe: the script's steps, with the resize → resize back → PageUp → measure sequence looped 25 times per variant. CDP CPU throttle 2, 3 copies at once, nice -n 10, while the PC was shared with the other agents' browser runs.
- Before the fix: 3 failures in about 90 PageUp trials, all bottom: 0, all with the runPendingScrollToEnd stack above.
- After the fix: 0 failures in 105 trials (3 × 35).
The real script without throttle: 6 of 6 passed (3 at once, twice). It never failed locally without throttle, so these runs only check for regressions, not the race.
bun run check fast: all ok (typecheck, build, 3455 unit tests).

Observed, not addressed

Under the CPU-throttled probe, a different and pre-existing failure appeared 3 times, once before any product change. After resize(height - 180) the view stayed 180 px from the end and "New messages" showed, so follow had been turned off by an upward adjust during the shrink. That would be script line 162/164, not 169. CI logs show no failure there, so it is out of scope here. It is noted in case it shows up later.

🤖 Generated with Claude Code

Rebased on main `6707797`, verification

Rebased onto 6707797ecf28bd17ec763268287e3f9dd312be08; tested head 3cf4033deccc6244716fc19714f74eaed989a07a. Resolved the ChatView import conflict by keeping main's lazy timeline loader and moving the cancellation helper into ChatView (no eager list import).
Root cause remains: @legendapp/list is pinned to 3.3.5 and ChatView still requests nonanimated follow through scrollToEnd.
bun run typecheck and bun run check fast: exit 0. Fast report: 6a714475-64ca-4e90-9de4-cfd37411dc47. No src/lib files changed.
One CHECK_WAIT_SECONDS=3600 bun run check run --build ran the unchanged chat browser script five times sequentially: 5/5, all four desktop/mobile and light/dark variants. Report: da25f8ec-6e8f-4b0f-9ea3-6c66bf8893d0.
Adversarial build probe held a real list scrollToEnd request until after PageUp, then awaited its completion. Removing only the cancellation call reproduced AssertionError: false == true at script line 169 with bottom: 0 and a pending follow. With the same gate and the guard restored, the pending follow was cleared and the whole script passed in all four variants (bottom: 875 desktop, 819 mobile). Report: d975629c-2ec0-42f7-a3bc-59439b902ce4.
All scratch build/test edits removed; git status --porcelain empty and git diff --exit-code clean. The original PageUp assertion is unchanged. No merge performed.

coderabbitai · 2026-10-13T19:31:24Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: devswha/herdr-web-ui/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 9791404c-de6b-4f50-9ce6-9b661206d26d

📥 Commits

Reviewing files that changed from the base of the PR and between d5a2da5 and 3cf4033.

📒 Files selected for processing: 1

src/components/ChatView.tsx

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

📝 Summary

Summary by CodeRabbit

Bug Fixes
- Paging up in chat now stays at the selected earlier messages instead of unexpectedly jumping back to the latest message after a resize or new output.
- This prevents a delayed scroll from interrupting navigation through chat history.

Upward page scrolling now drops a pending list scroll after setting paging-away intent. This prevents that pending scroll from overriding PageUp navigation. The changelog records the fix.

Changes: Chat paging

Layer / File(s)	Summary
Drop pending scroll on PageUp `src/components/ChatView.tsx`, `changelog.d/chat-pageup-stale-follow.md`	A helper requests a non-animated scroll to a frozen no-row target. Upward page scrolling calls the helper after setting paging-away intent. The changelog describes the fixed behavior.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: kilhyeonjun

Merge Risk: ⚪ Minimal · up to 3cf40

The change aims to prevent a delayed scroll from pulling the chat back to the latest message after PageUp. No actionable failure is established in the supplied evidence, so merge risk is minimal; the pinned list’s behavior for a missing-row request remains unverified.

Pre-merge checks: 5

Passed checks: 5 passed

Check name	Status	Explanation
Description check		The description clearly explains the problem, root cause, implementation, scope, and validation results. It includes the required Change and Validation sections and provides reproducible test evidence…
Title check		The title is concise, specific, and accurately describes the main change: clearing a pending follow scroll when PageUp moves away from the end.
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

Commit to this branch
Create a new PR

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

devswha · 2026-10-13T19:35:44Z

Parked as a draft: a UI/UX rework is planned, and it will rewrite the browser tests this PR deflakes. Revisit after the rework if the flake still shows on the new UI.

## Change Over the last day about three in ten runs of CI's browser lanes failed, on `main` as on pull requests, each time somewhere else. Four of the recurring places are races in the checks themselves, not in the app. Each was reproduced with a named delay before it was changed: - **`terminal-input-regression.ts:65`, "input regression timed out"** (5 times). The check waits for the "Waiting for terminal input…" banner to be hidden, then dispatches `online` and waits for a ping. The banner is drawn only on a connected pane, so before the socket opens it is hidden too, and `wake()` then has nothing to probe (`probe()` returns unless the socket is open and the snapshot has said `ping`). No later event sends one. The check now waits for `.conn-live` first. Reproduced by opening the page's first socket 3 s late: 2 of 2 failed with CI's message at that line; 3 of 3 pass with the fix under the same delay. - **`update-notice-regression.ts:74`, "one tap asks for the install: 0 !== 1"** (2 times). The line says "Starting the update…" as the button is pressed; the route that counts the request can run after that text is seen. The check now waits for the install request's response before it reads the count, for the first press and for Try again. Reproduced by counting the request 400 ms late: 2 of 2 failed with CI's message; 3 of 3 pass with the fix. - **`usage-regression.ts:157`, "a hidden strip asks for nothing: 7 !== 6"**. Opening Settings mounts its own `useUsage`, which asks once; that request could be counted after the check had read its count and reloaded. The check now marks the reloaded page's requests with a header and asserts that none of those asked for usage. Reproduced by counting requests 150 ms late: 3 of 3 failed with CI's message; 3 of 3 pass with the fix. With plan limits switched back on before the reload the assertion still fails, as it must. - **`mobile-viewport-regression.ts:37`, `'' !== 'contain'`** (3 times). The transcript's first node is replaced as the conversation loads, 42 to 126 ms after the composer appears in three local runs. A locator that found the first node and read its style after the swap reads a node outside the document, whose computed style is empty. The check now finds the node and reads the style in one step in the page. The swap and the empty style of the replaced node were observed; the timing that lands the read between them was not reproduced. No app code changes. `sticky-modifiers` and the split drop are #984's, `chat-virtual` is #985's. ## Validation - macOS arm64, Bun 1.4.0, herdr 0.9.3. `bun run typecheck`: pass. `scripts/conventions.test.ts`, `changelog-fold.test.ts`, `pages-gate.test.ts`: 39 pass. - `bun run check run --build bun scripts/ui-regression.ts`: 65 PASS, exit 0. - `bun scripts/ui-contexts-regression.ts`: exit 0, all 24 checks. - The before/after runs above used a throwaway runner and delays that are not in this PR. - Not run locally: the integration lane and the other browser scripts.

…laky (#1002) ## Change CI's browser lanes failed about three in ten runs over the last day (57 of 192 jobs on the self-hosted runner, 29 of 89 on GitHub's before that), on `main` as on pull requests, at more than twenty different places. Until those races are fixed one by one (#984, #985 and the four in the companion PR are a start), every PR waits on reruns by hand. `scripts/ci-browser-parallel.ts` now runs a failed script a second time. A script that then passes does not fail the lane, and is not hidden as a pass either: - its first log is kept as `<script>.attempt-1.log` (uploaded with `browser-logs`) and its last 400 lines are printed; - the summary marks it `~`, and `summary.json` lists it under `flaky` with the first log's path; - on GitHub it gets a warning annotation and a line in the job summary. A script that fails twice fails the lane as before. `BROWSER_RETRY=0` turns the second run off. The serial lane (`ci-browser.sh`, `bun run check browser`) is unchanged: it still stops at the first failure. The cost is real and worth saying: a failure that only happens sometimes can now merge. That is why a flaky pass is loud. This keeps one PR from waiting on another change's race; it does not make a race acceptable. ## Validation - macOS arm64, Bun 1.4.0. `bun run typecheck`: pass. `scripts/pages-gate.test.ts` (new case for `scriptOutcome`), `conventions.test.ts`, `changelog-fold.test.ts`: 40 pass. - Run for real with `BROWSER_SCRIPTS=scripts/deep-link-demo-regression.ts bun scripts/ci-browser-parallel.ts` and a throwaway line that made that script exit 1: - failing on its first run only: exit 0, `FLAKY` line, `~` in the summary, `flaky: ["scripts/deep-link-demo-regression.ts"]` in `summary.json`, both logs present, the `::warning` line and the job-summary text written; - failing every time: exit 1, "FAILED … (exit 1, twice)"; - failing on its first run with `BROWSER_RETRY=0`: exit 1 after one run. - Not run locally: the lanes themselves beyond that one script.

## Change Fix the intermittent `server/omo.contract.test.ts:52` failure in “labels a running omo pane when herdr supplies no agent kind”. ## Root cause and evidence The fixture waits for its script path in one herdr `pane.process_info` response, but `labelOmoPanes` performs a separate fresh process probe. The fixture marker is not a readiness barrier for that probe: a missing matching foreground process or a failed probe leaves the original snapshot's agent undefined. The test asserted the first label sample immediately instead of waiting for the state it describes. Main run 38555205694 on October 14, 2026 failed after 199.60 ms: expected `"omo"`, received `undefined` at line 52:77. Five local pre-fix isolated runs passed, including an attempted concurrent unit-suite load, so the failure remained intermittent locally. The CI log and the two independent process probes establish the synchronization gap without guessing which negative-probe branch occurred. Open PRs #984 and #985 do not touch this test. PR #923 changes custom OmO runtime recognition but leaves this one-shot assertion unchanged, so this is not a duplicate. ## Fix Poll the exact labeled state with a bounded five-second deadline and 50 ms interval. The same original snapshot is retained, so all three existing assertions still prove that herdr supplied no agent kind, the derived snapshot is labeled `omo`, and the input snapshot is not mutated. No assertion retry, timeout increase, fixed sleep, production change, or changelog entry. ## Verification - Five consecutive isolated contract runs passed on one unchanged source fingerprint: `CHECK_WAIT_SECONDS=3600 bun run check run bash -c 'for i in 1 2 3 4 5; do bun test --timeout 15000 ./server/omo.contract.test.ts || exit 1; done'`. Report: `23297fbe-e853-45cf-ad71-a4c6990b77e0`. - `bun run check fast` passed on the same source fingerprint. Generated types, typecheck, build, and unit tests all passed: 3,574 passed, 50 skipped, 0 failed. Report: `19778f35-f21d-4c74-987e-9c48ae6620a9`. - `git diff --check` passed. Toolchain: Bun 1.4.2, Node 22.23.1, herdr 0.9.3, macOS arm64. The lane handoff records the root cause, CI timing evidence, diff, full verification log references, and this PR URL at `.omo/mass-ulw/2026-10-14/A3.md`.

…owth (#1098) ## Change ### Root cause The stale-navigation test equates complete bottom-relative scroll snapshots after a substring output receipt. The receipt can precede remaining cat/terminal-echo output, so history can grow while the viewport stays anchored. Main CI run 38559049536 expected max/offset 462/308 and received 463/309: both have absolute top row 154 and viewport_rows 40. paneFind rejects a stale previous revision before copy_search or pane.scroll. The test was treating history growth as viewport movement. ### Fix Compare the exact absolute viewport top (max_offset_from_bottom - offset_from_bottom) and viewport height in both navigation directions. Preserve the positive scrolled-offset precondition, HTTP 409, and stale_content assertions. A one-row viewport move still fails. No timeout increase, retry, sleep, or production change. ### Evidence - Retrieved the failed CI log with `gh run view 38559049536 --log-failed`; the failure is at `server/find.contract.test.ts:174`. Both snapshots describe top row 154, not a viewport move. The copy-revision guard in `server/herdr/client.ts:511` rejects this request before search or scroll. - Checked open file/offset PRs and read #984, #985 and #1090; none fixes this exact failure. - Five pre-fix isolated iterations passed (18 tests each), and a sixth iteration running alongside `HERDR_TEST_MODE=unit bun run test:unit` passed; the load also exited 0. No local reproduction claimed: the CI log and scroll-coordinate contract prove the cause. - Replaying the CI snapshots as assertions makes the original equality fail and the fixed invariant pass; a one-row movement, height change or null scroll metadata still fails. ## Validation - `CHECK_WAIT_SECONDS=3600 bun run check run bash -c 'for i in 1 2 3 4 5; do bun test --timeout 15000 ./server/find.contract.test.ts || exit 1; done'`: 5/5 consecutive passes, 18 tests each, runner exit 0; source unchanged throughout. Baseline and fixed reports: `b79813b2-1dd9-4c2e-8121-011ea62d78d2`, `3bedd127-711e-479a-89e4-03081936ad09` under `node_modules/.cache/check/`. - `UNIT_TEST_WORKERS=2 CHECK_WAIT_SECONDS=3600 bun run check fast`: exit 1 only in untouched `server/portal.test.ts`, `running Portal > does not follow redirects when checking the relay version` (10-second readiness timeout reporting Portal not installed). Unit totals: 3573 pass, 1 fail, 50 skip across 287 files. Generated-type freshness, typecheck and build all exit 0. Report: `130c8ec7-b2ec-46d0-ae89-86daacbda03f`. This is recorded as an out-of-scope load/environment failure under the assignment's explicit exception; PR CI is the authoritative gate, and local Fast is not claimed green. - `git diff --check`: exit 0. LSP unavailable (`typescript-language-server` not installed); repository typecheck passed instead. Workflow syntax is platform-skipped on darwin arm64 and remains CI-owned. - Only the contract test changes. No production behavior or user-visible change, so no changelog fragment is required by `changelog.d/README.md`.

LegendList's scrollToEnd does not scroll at once: it runs in the list's next commit, and waits up to 800 ms more for layout to settle after a data change. A follow requested while the reader sat at the end (the resize observer, a streamed revision) could so land after PageUp had moved the view away, snapping it back to the latest message and re-arming follow. This is what scripts/chat-virtual-browser-qa.ts caught at line 169 on loaded CI runners. The PageUp path scrolls the native node, which does not supersede a waiting list request (Find, minimap ticks and restored places scroll through the list and already do). It now drops the waiting request with a list scroll to no row, which supersedes it without moving. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

devswha marked this pull request as draft October 13, 2026 19:35

This was referenced Oct 13, 2026

test(browser): four checks no longer race a slow run #1001

Merged

ci(browser): run a failed browser script once more and report it as flaky #1002

Merged

This was referenced Oct 14, 2026

test(omo): wait for the observed pane label #1097

Merged

test(find): assert the unchanged viewport independently of history growth #1098

Merged

test(opencode): isolate paging from later fixture changes #1100

Merged

devswha force-pushed the fix/flake-chat-virtual branch from d5a2da5 to 3cf4033 Compare October 14, 2026 14:37

devswha marked this pull request as ready for review October 14, 2026 14:38

devswha merged commit c46b7be into main Oct 14, 2026
14 of 17 checks passed

devswha deleted the fix/flake-chat-virtual branch October 14, 2026 14:55

snowykr mentioned this pull request Oct 19, 2026

Chat: GJC tool streaming moves the viewport and loses expanded tool details #1388

Closed

Navigation Menu

Repository navigation

fix(chat): drop a waiting follow scroll when PageUp leaves the end - #985

fix(chat): drop a waiting follow scroll when PageUp leaves the end#985
devswha merged 1 commit into
mainfrom
fix/flake-chat-virtual

devswha commented Oct 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 13, 2026 •

edited

Loading

Summary by CodeRabbit

Uh oh!

devswha commented Oct 13, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

devswha commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root cause

Change

Validation

Observed, not addressed

Rebased on main 6707797, verification

Uh oh!

coderabbitai Bot commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

devswha commented Oct 13, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

devswha commented Oct 13, 2026 •

edited

Loading

Rebased on main `6707797`, verification

coderabbitai Bot commented Oct 13, 2026 •

edited

Loading