Reduce navigation fixture cost and stabilize typing thread opening - #68
Merged
Merged
Conversation
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com>
comp615
force-pushed
the
lean-message-navigation
branch
from
September 15, 2026 15:12
13cde94 to
ffdc369
Compare
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
wesbillman
reviewed
Sep 16, 2026
wesbillman
left a comment
Collaborator
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blocking findings. Reviewed head e70c3dec0e19828b3a7aeb225a261fda38150ad8 against base d21f43364fe9b7bdf2643779dab819fd9e8c16bb, including independent source review of the navigation scenarios.
- The smaller navigation histories retain the old-target and multi-page preconditions: 120 ordinary Alpha rows still put the exact reply before the initial window, while the separately generated 80 replies still require thread continuation. The new assertions use observed requests. Existing recovery, cancellation, reconnect, membership, anchor, focus and deletion assertions remain intact; measurement datasets are not reduced.
- The typing change waits for both signed completion messages to mount, then uses the existing real-wheel helper and geometry-settling checks before the native View thread click. This closes the setup/layout gap without forcing activation, relaxing assertions, adding retries or increasing timeouts. The separate completion/expiry geometry cases remain unchanged.
- Verified hosted run 35014114522: both complete affected files passed in Chromium and WebKit, 30/30 cases. Their artifacts record navigation histories of 120/0 and typing histories of 640/80; all five measurement artifacts also retain 640/80. The existing narrow WebKit ResizeObserver allowance is unchanged.
Validation scope: the hosted artifacts identify clean merge commit 55f2464396f826702b86ca5f6da3bc18534ddaf8, incorporating newer main, not the bare PR tree. Both changed test files and the shared timeline helper match the reviewed head. I did not run local tests or independently reproduce the reported prior flake; this evidence does not establish that every possible scheduling flake is eliminated. This is a comment review, not an approval.
wesbillman
approved these changes
Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
September 15 — typing navigation fix verified in CI
🤖 Follow-up commit fixes the missing lifecycle barrier in the typing journey. After two signed completion messages, the test now observes both appended rows and uses the existing real-wheel/end-of-timeline helper to establish settled geometry before clicking View thread. Native pointer activation, typing assertions, authenticated transport, expiry and publication checks remain intact. No production or shared-fixture changes, new browser cases, sleeps, forced clicks, timeout increases or retry configuration changes.
Failure evidence: the previous CI run timed out waiting for the thread composer. Its trace shows the same button targeted at y=244, 752, 442 and 244, with the timeline intercepting multiple pointer attempts, followed by a completed click with no thread opened. The immediate typing-indicator assertion did not establish message-layout completion. The unmodified local typing file passed, so this is not claimed as a deterministic local reproduction.
Validation: all 30 cases in the complete typing and message-navigation files passed across Chromium and WebKit after the change (1.6m local wall time); the complete typing file also passed three repetitions in both engines (18/18, retries disabled). Biome, diff checks and unchanged hooks passed; all PR commits retain sign-offs. Full current-head CI passed on its first attempt: JavaScript, Rust/tool integration, Windows, measurements, all four functional browser shards, and
CI required. Hosted DCO, Semgrep and zizmor also passed. Command:bin/pnpm test:browser tests/browser/typing.spec.mjs tests/browser/message-navigation.spec.mjs --project chromium --project webkit --no-deps.Original fixture-sizing timing evidence below remains historical; the synchronization fix adds necessary waiting and does not claim further speedup. Targets main independently; required review remains outstanding.
Purpose
Reduce the cost of the largest measured browser file without moving or weakening its navigation coverage. "Targets
maindirectly after #63 merged, independent of the remaining PRs."Message-navigation now declares 120 Alpha / 0 Beta ordinary rows per community instead of inheriting 640/80. That is 240 rather than 1,440 signed ordinary history records per case. The existing exact root, all 80 replies and their edit/reaction/deletion evidence remain unchanged. No production code, shared fixture APIs, dependencies or PR templates change.
The initial channel window must remain newer than the exact reply, and thread navigation must actually issue a continuation query. New assertions enforce those boundaries using the observed query ledger rather than copying production page-size constants. The full file still exercises virtual-row reveal, missing-root recovery, slow traversal, late responses, reconnect, membership loss, reading anchors, dwell-read behavior, stream repair and deletion.
Local timing evidence
Baseline: #63 head. After: this head.
Same worktree and macOS arm64 machine, pinned Node 24.18.0 / pnpm 11.8.0 / Playwright 1.60.0, two workers, Chromium and WebKit. Identical 24 case identities; one before run, then two after runs. No concurrent local test/build commands during measurements.
Local wall time fell 31–32%; summed history signing fell about 83%. Signing/build work overlaps across workers and cannot be subtracted directly from wall time. Builds were also faster after the first run, so not all wall-time change is attributed to history generation. Installation is outside these measurements; builds are regenerated per invocation. One baseline sample is not a confidence interval or a whole-CI speedup claim.
Verification and coverage
just scanincluding unrelated browsers/measurements/native checks, hosted CI and required code-owner review. Draft for CI measurement, not integration readiness.Hosted CI evidence
Run 34898525844 passed at this head, including all lanes,
CI requiredand DCO. Verified all 364 unique functional identities match #63, with 95/87 per engine, plus five isolated measurement cases; no failures, skips or retries. Measurement artifacts retain 640/80 histories and existing budgets. Chromium warm switching measured 49.1–58.5ms; the three existing exact WebKit ResizeObserver warnings remain recorded, without new allowances.Message-navigation's rounded summed case time fell from 122.7s to 76.7s in Chromium and 153.0s to 89.0s in WebKit: combined 275.7s to 165.7s, about 40% less. All changed cases remain on shard 2.
Both affected shards were faster, but there is no demonstrated overall critical-path improvement: the slowest functional step increased from 445s to 538s on the unchanged WebKit shard 1. These step times exclude separate CI setup/native-build/cache steps but include Playwright-owned frontend builds. Runner variability and unchanged work prevent attributing full-shard differences solely to this change. Vitest took 50s; measurements took 128s. Full local scan, code-owner review and merged-main verification remain outstanding.
🤖 Prepared with Amp for Charlie Croom.