Restore bottom positioning for ordinary routed threads - #72
wesbillman wants to merge 2 commits into
Conversation
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blocking findings
Reviewed head 8b9082ef15dd716fbfecff0d0101272703271b29 against base 7ce898eb6f77ca4fb95f06a7c7789f63245dee28.
The production change stays within the existing thread presentation owner: ordinary root routes no longer invoke exact-message reveal, acknowledgement clears the navigation deadline without settling scrolling, and bounded pagination still controls initial bottom positioning unless reader intent takes over. Exact-target reveal/focus, request revocation, and history routing retain their existing paths.
Reviewed the complete diff and the owning UI, navigation controller/presentation lifetimes, thread reader, and reading hooks. Regression coverage includes routed/non-routed bounded loading, reader gestures, one-time completion, cancellation, a real-controller 16-second deadline case, and held-pagination browser journeys with live updates. git diff --check passed. No local test suites were rerun; existing CI supplies broad validation.
CI snapshot for this head: JavaScript (including full Vitest), Rust/tool integration, Windows notification checks, browser measurements, and DCO passed. Chromium’s complete message-navigation.spec.mjs and navigation-thread-history.spec.mjs files passed (15 cases). Both engines’ first journey shard failed at composer-links.spec.mjs:31 (expected two GitHub-decorated composer links, got zero); the WebKit second shard was still running. That failing standalone composer fixture does not mount ThreadPanel or use the modified browser fixture, so I found no change-related cause in this diff. Treat it as a separate unresolved CI gate, not a passing build or a proven flake.
No local runtime, live-relay, native-GUI, or screen-reader certification. Existing ten-page oldest-first history limits remain unchanged: bottom means bottom of returned history, not necessarily the newest reply.
No approval or merge action submitted.
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Source review clear
Reviewed 05b873e against exact base/merge-base 7ce898e. No actionable code finding. The independent fixture/selector lane returned clear and was verified against the owning source.
- Ordinary root visits acknowledge the mounted thread before the navigation deadline without settling scrolling. Bounded history still controls bottom positioning unless reader intent intervenes. Exact-message reveal/focus, retained anchors, Back/Forward, and revoked presentation retain their existing owners. The production and owner-test blobs are unchanged from the previously reviewed
8b9082e. - Reviewed both new test corrections against their actual consumers: the link-composer preview supplies the stable empty typing store required by
TypingIndicator; the Back test selects the real Beta channel button and explicitly waits for its unread badge. Assertions, deadlines and retry settings are not relaxed. - Exact-head hosted CI is green: run 34959748245 passed all nine CI jobs, including browser measurements and all four Chromium/WebKit journey shards. DCO, Semgrep and zizmor also passed. Exact-object
git diff --checkpassed.
Source/metadata inspection only on the authorized laptop; no checkout, dependency installation, builds, tests or PR-code execution by this automation. No live-relay, native-GUI or screen-reader certification. The existing ten-page oldest-first limit remains: bottom means bottom of returned history, not necessarily the newest reply. This is a COMMENT, not approval or a merge action.
Pinky is updating this PR on Wes’s behalf.
Change
Restore the documented ordinary reply-count behavior: finish at the bottom of bounded returned history unless the reader intervenes. Exact-message reveal and Back/Forward history keep their existing paths.
Ordinary navigation acknowledges the mounted root before slow traversal can hit the existing 15-second navigation deadline; acknowledgement does not force root reveal or mark positioning settled. Production diff remains 19 additions / 6 removals in
ThreadPanel.tsx. No new timeout, navigation API, reader or transport.Separate from presence #70. The positioning regression was reproduced on untouched main
06737ccin Chromium and WebKit; presence validation exposed it.CI correction at
05b873eThe previous run had two distinct test-harness failures:
session.typingsnapshot/subscription and crashed. The exact parent/main7ce898efailed identically in both engines. Supply a stable empty typing snapshot in that fixture.Beta, but unread evidence changes its accessible name. Select the existing channel-ID attribute, explicitly waiting for the badge before clicking so the failing condition is covered.Correction: 8 additions / 1 deletion in two test-owned files. No production changes, longer timeouts, retries, or weaker assertions.
Validation
composer-links.spec.mjsandnavigation-thread-history.spec.mjspass in Chromium + WebKit (8/8).pnpm checkand full Vitest (1,555 tests / 146 files) pass.8b9082eplus the exact patch committed as05b873e; verified patch SHA-256fab1625f26976fc37d130584bdcda0c00c32f80c83c0dae2fd9ef4532b15e48d, with no hook edits.05b873e(push: types + 42 related tests).06737cc, green with the fix in both engines. The previous hosted run passed measurements and the new held-pagination cases, but failed the two fixture/selector cases above.Hosted result and remaining limits
Hosted CI at
05b873eis green: run 34959748245 passed all nine CI jobs, including all four browser-journey shards, measurements andCI required. DCO, Semgrep OSS and zizmor also pass. No live-relay/native-GUI validation claimed. Green CI is not merge approval; this PR has not been approved or merged by Pinky.