fix(desktop): anchor history pagination through visual commit - #7006
fix(desktop): anchor history pagination through visual commit#7006wesbillman wants to merge 6 commits into
Conversation
Reserve one older-page transaction through deferred rendering and stable measurement. Admit rows and metadata together, reconcile Virtua sizes by key, and stage reconnect refreshes without evicting the reading window. Add recovery controls, concurrent-send rollback protection and mounted/browser regressions for reader intent, cancellation, mixed updates and late reflow. Co-authored-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
🔐 Codex Security Review
|
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent reviewed exact head f0335c3b1859712818004b1cf0e4dfb810081445 against base c432a111ca9ddd31a85e1312d5995f8b92191b82.
Blocking findings
-
High — successful main-timeline deletes are not removed from the authoritative window.
useDeleteMessageMutation.onSuccessonly filterschannelMessagesKey(desktop/src/features/messages/hooks.ts:883-888), but this PR makeschannelWindowKeyauthoritative: the neighboring edit path explicitly updates the window first because every live merge re-flattens it over the message cache (desktop/src/features/messages/hooks.ts:962-975). A successful delete can therefore remain visible or be resurrected by the next projection/live merge. This is not theoretical: CI Smoke E2E shard 2 failed all three attempts for both ordinary and owned-agent deletion (empty-edit-delete.spec.ts:51,human-edit-agent-content.spec.ts:119), always receiving a visible row after the 5s hidden assertion; an independent exact-head local rerun reproduced both failures.Author action: functionally remove the event from the current authoritative channel window (pages/live overlay/auxiliary storage as applicable), then project it without restoring a captured snapshot that could erase concurrent paging/live writes. Add a real-
QueryClientregression proving the deletion survives reprojection while concurrent window updates remain intact, and rerun both deletion E2Es plus the full smoke shard. -
High — fresh live traffic can disappear before reconnect is even attempted. CI Smoke E2E shard 4 failed all three attempts in
relay-reconnect.spec.ts:657at the pre-disconnect assertion on line 669: after mock live delivery, the timeline never containedreconnect e2e seen before disconnectwithin 5s. An independent exact-head focused rerun failed at the same assertion. Since execution never reaches the disconnect on line 673, this is a live-tail admission failure, not merely uncertainty in reconnect backfill; users at the apparent bottom can silently miss a newly delivered message.Author action: repair bottom/live-tail admission and add a deterministic fresh-channel live-arrival regression before the reconnect/cursor-race scenario, then prove the >subscription-limit reconnect backfill.
Validation and integration notes
- Exact-head Desktop unit suite: 5,825/5,825 passed; typecheck passed.
- Focused pagination/revalidation/reaction/Virtua tests: 54/54 passed.
history-transactions.spec.ts: 7/7 passed in Chromium, including deep-history reconnect, concurrent older request, deletion/live metadata during prepend, Retry/Load latest, and<5pxanchor checks. - Mutation checks caused the expected failures when visual-commit acknowledgement, concurrent-send rollback preservation, or fresh-cursor revalidation were weakened.
- The
virtua@0.49.3ESM and CJS patches were checked for parity across keyed structure, late measurement, reader-input retirement, and estimate forms. - PR #6601 edits the same window store, pagination, timeline, settle-gate, and E2E contracts with real merge conflicts. These are competing semantics; whichever proceeds must reconcile and re-prove behavior rather than mechanically combine diffs.
- Confidence gaps, not additional blockers: native WKWebView/physical-trackpad momentum and scrollbar behavior, live-relay observation, and screen-reader recording remain unverified. An interactive macOS tester should validate a dense live channel, one-page-per-gesture, late media/grouping reflow, and
<5pxanchoring after the code defects are fixed.
Current CI is blocked with Desktop and Smoke shards 2–4 failed. Verification owner after author fixes: reviewer reruns the focused dual-cache/concurrency, delete, live-arrival/reconnect, unit/typecheck, and relevant full smoke evidence at the new exact head.
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Combined review from three independent passes at exact head f0335c3: a source-level review, a CI-failure/diff analysis, and a live-relay E2E run against the PR-built desktop bundle. Requesting changes — two blocking defects, both confirmed by deterministic exact-head CI failures (3/3 retries exhausted) and independently root-caused in source.
Blocking
1. IMPORTANT — Successful deletes only mutate the derived message cache, so the authoritative window resurrects the row. useDeleteMessageMutation.onSuccess filters only channelMessagesKey (desktop/src/features/messages/hooks.ts:883-888), but this PR makes channelWindowKey the projection source: projectChannelWindowMessages() rebuilds the message cache from the window store (projectChannelWindow.ts:12-23), so any subsequent live event, scrollback page, or explicit projection restores the deleted row. The adjacent edit path already documents and handles this dual-cache rule. CI confirms the user-visible failure: empty-edit-delete.spec.ts:51 and human-edit-agent-content.spec.ts:119 both fail 3/3 with the deleted row still visible after 5s.
Fix: remove the event from every applicable current-window location (page rows/aux and live overlays), then reproject from that current window — do not restore a captured snapshot, which would erase concurrent page/live commits. Add a real-QueryClient regression that deletes while concurrent window state advances and proves reprojection cannot resurrect the row.
2. IMPORTANT — A live-tail event is not reliably admitted while the channel appears at bottom. relay-reconnect.spec.ts:657 fails 3/3 at line 669 — before the disconnect: the emitted seenBeforeDisconnect live event never enters the rendered timeline on a fresh channel that should be at bottom. The affected path is the new semantic-bottom/buffering handoff (useAdmittedTimelineSnapshot → useBufferedTimelineMessages, driven from MessageTimeline.tsx:254-267, 398-424), which can freeze the tail off a virtualizer bottom-state transition even though the reader never established scrollback intent. This also blocks reconnect validation, since the test never reaches the reconnect.
Fix: make a live event delivered to an apparently-bottom fresh channel enter the admitted snapshot deterministically; add that as a standalone regression before re-proving the greater-than-subscription-limit reconnect journey.
Both failure families are absent from main: CI succeeded at this PR's merge-base a3730784 and again at c432a111 on the same smoke lanes ~30 minutes before this run. Also needing triage under the same umbrella: message-copy-link.spec.ts:160 fails 3/3 (a pending row exposes a copy-link surface it must not), consistent with the pending-only reconciliation filter in channelWindowReconciliation.ts.
Merge-ordering (unresolved, needs an explicit decision)
Open #6601 is a material competing implementation of these same paths — the PR body itself says both must be reconciled before either merges. A merge-tree check between #6601's head d73ba9c19 and this head produces content conflicts in channelWindowStore.ts, pageOlderMessages.ts, MessageTimeline.tsx, TimelineMessageList.tsx, useSettleGatedPrependMessages.ts, scroll-history.spec.ts, and virtualization.spec.ts. These are semantic, not mechanical — whichever lands second needs real reconciliation and re-review.
Non-blocking
useHistoryPagination.start()reserves the page synchronously and retirement paths (channel switch, unmount, explicit intent, error, undefined receipt, stale generation) were all traced and hold; but a Tauri invoke that never settles wedges pagination for that channel with no liveness bound. Worth either a defensive bound on the request or an explicit comment stating why the invoke is trusted to settle.- The held-prepend state in
useSettleGatedPrependMessageshas no dev-visible signal; a stuck hold in the field would be invisible. A debug counter would help. reconcileFetchedChannelWindow'sliveSummariesfilter relies on object identity (!==) across a parse boundary — pin it with a comment or test.- The extended
virtua@0.49.3patch is a large behavioral fork carried in minified vendor output. The installed ESM/CJS tests are unusually strong, but upstream divergence is a standing maintenance cost worth acknowledging in the patch header.
What held up under independent verification
- Live-relay E2E at this exact head (real signed events through relay ingest, dedicated relay/Postgres/Redis/MinIO stack): repeated upward pagination (50→121 rows across 5 real window requests, 0.00px anchor drift), rapid channel switch mid-pagination (no cross-channel row leakage), and relay restart during the receipt/visual-commit gap (0.00px drift, history still pageable) — all passed.
- Reconnect revalidation stages off-cache, follows exact composite cursors, verifies the first join with a fresh page, re-reads the current tail before synchronous publication, and leaves the retained store untouched on failure.
- Failed-send rollback is correctly narrowed to the optimistic id, preserving concurrent history/live writes.
- The store
revision/generationtokens correctly retire stale in-flight pages, and theuseUpwardPaginationWheeltimer→deadline rewrite is a real simplification.
Native WKWebView / physical-trackpad behavior remains unvalidated, as the PR body already acknowledges.
… updates Scope explicit Load latest intent to one fetch and its retries, retiring unclaimed intent after cancellation. Update the authoritative window on accepted deletion without erasing concurrent history or live arrivals. Admit backdated live rows in exhausted windows and reconnect gap rows before the buffered tail. Add real-hook and browser regressions; give the huddle copy-link fixture its own signed-event id. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
|
Pinky is commenting on Wes’s behalf. Pushed follow-up
Verification: full pre-push gate at this exact head: 5,835/5,835 desktop tests, typecheck, lint/policy and file-size checks passed. Fresh-build cross-file browser matrix of the same production sources: 76/76 passed, covering all four previously persistent smoke failures and anchor/reflow/paging regressions. Then two new standalone live-arrival tests plus the reconnect case passed 3/3 on that unchanged build. New complete CI run is pending; I have not claimed full local smoke-shard or CI success. Remaining: reviewer clearance, security review for the new exact range, #6601 merge-order/semantic reconciliation, and physical macOS trackpad/native WKWebView plus before/after visual validation. PR body updated with exact evidence and limitations. |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
🤖 Re-review at exact head 4c25af1660fa7890cee8340f201ecab26ba8f18f (base c432a111ca9ddd31a85e1312d5995f8b92191b82): approved. Three independent passes found the previous delete-resurrection and live-admission blockers repaired, with no remaining author-actionable defect.
Prior blockers cleared
-
Authoritative deletes preserve concurrent state.
useDeleteMessageMutationnow removes the accepted event from the current window and derived cache (desktop/src/features/messages/hooks.ts:875-900).removeEventFromChannelWindowremoves it across page rows/auxiliary rows and live overlays without rewinding cursor, revision, or generation metadata (channelWindowStore.ts:248-277). The mounted real-QueryClient regression holds deletion in flight, commits an older page and concurrent live write, then proves deletion survives later projection while those writes remain (historyPaginationLifecycle.test.mjs:832-933). Removing the production window update makes that regression fail; restored head passes. -
Exhausted-history live delivery is admitted. Below-boundary live rows are rejected only while the oldest page has more history (
channelWindowStore.ts:192-230). Once exhausted, no unresolved retained gap exists, so both current and backdated live rows enter the overlay. Reverting the condition fails the dedicated store regression. Exact-head browser coverage passes both no-gesture fresh-arrival cases and the greater-than-subscription-limit reconnect journey. -
Recovered gaps render without releasing the buffered future tail.
useBufferedTimelineMessages.ts:20-38freezes a retained tail boundary rather than frozen membership: recovered rows before the tail render, newer rows remain buffered, and deletion/replacement of a frozen ID fails open to current authoritative messages rather than stale objects. -
Load-latest intent is scoped to one fetch lifecycle.
channelWindowRefreshIntent.ts:5-29,hooks.ts:314-364, andHistoryRefreshNotice.tsx:43-77claim intent at query entry, retain it across actual TanStack retries via that fetch'sAbortSignal, clear only matching unclaimed intent on cancellation/unmount, preserve retained history on exhausted retry, and suppress stale post-navigation effects. Real-hook/DOM tests cover send cancellation, channel navigation, retry, exhausted Retry/Load latest, and pre-invalidation unmount.
Exact-head evidence
- Full Desktop unit package: 5,835/5,835 passed across 83 suites; Desktop typecheck passed.
- Fresh E2E builds: focused systems matrix 29/29 passed; broader product matrix 36/36 passed, covering ordinary/owned-agent/thread deletes, copy-link cases, history transactions, fresh/backdated exhausted-window live delivery, and reconnect/backfill.
- Both targeted production mutations failed their intended regression and passed after restoring the exact head.
- GitHub smoke shards 1–4, both Desktop E2E integration shards, macOS Desktop build, Windows Rust, relay E2E seam, mobile, web, and release-candidate build are green at review time.
Desktop Coreremains in progress; the named gate owns completion.
Confidence gaps / integration gates — non-blocking to this code review
- Author action: none from this re-review.
- Verification owner: CI for the still-running Desktop Core gate; reviewer/tooling for native WKWebView, physical-trackpad momentum/rubber-band/scrollbar behavior, native visual evidence, and screen-reader announcement observation.
- The exact-range security workflow was skipped after marking the previous review stale; the security gate owner must provide the required disposition. This is not converted into author rework without a concrete finding.
- Open #6601 materially overlaps the channel-window/timeline/settle-gate design. Merge ordering requires semantic reconciliation and a fresh exact-head review of whichever implementation lands second, not mechanical conflict resolution.
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Re-reviewed at head 4c25af1. Both blocking findings from the previous review are addressed, with regression coverage, and full CI is now green — including the four previously-failing smoke specs (3/3-retry deterministic failures at the prior head), which is the strongest possible confirmation the fixes land where the defects were.
-
Delete resurrection — fixed.
useDeleteMessageMutation.onSuccessnow removes the accepted target from the authoritative window store viamapChannelWindowEventsreturningnull(page rows, page aux,liveOverlay,liveAux) before filtering the derived cache, preserving page cursors and the store'srevision/generation. The new mounted real-QueryClientregression holds deletion in flight while an older page and a live row commit, then proves a later reprojection cannot resurrect the row — exactly the concurrency shape requested, with no snapshot restore. -
Live-tail admission — fixed, one seam earlier than the review hypothesized. The actual defect was
mergeLiveChannelWindowEventrejecting any live row older than the retained oldest row even on an exhausted window; the predicate now rejects below-boundary rows only whileoldestPage.hasMore. The companionuseBufferedTimelineMessageschange correctly freezes a tail boundary rather than exact membership, so reconnect gap-fills before the tail are admitted while newer output stays buffered. New standalone browser regressions cover bothnowand backdated arrivals on a fresh exhausted channel before the reconnect journey runs. -
The
message-copy-linkspec change is a genuine test repair (the huddle fixture had reused the seeded custom-emoji reaction target ID with a conflicting kind), not a weakened assertion — the guard assertions are unchanged. -
The new
channelWindowRefreshIntentrecovery-token mechanism (consumed at fetch entry, scoped to that fetch'sAbortSignalacross TanStack retries, reset at the community boundary) is cleaner than the durable-flag approach it replaces and does not touch the pagination reservation, reconnect cursor traversal, or rollback paths previously verified — including the live-relay E2E pass at the prior head (0.00px anchor drift across repeated pagination, channel-switch, and relay-restart journeys), which remains valid for those unchanged paths.
Reminder outside this PR's control: open #6601 is a competing implementation with semantic conflicts against this branch in seven files; whichever lands second needs real reconciliation and re-review. That is a merge-ordering decision, not a defect here.
Non-blocking notes from the previous review (liveness bound on the pagination reservation, held-prepend observability, virtua-fork maintenance cost) remain open as optional follow-ups.
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Re-review of the new head after my earlier changes-requested review — both concerns are addressed.
Delete resurrection via window reprojection: fixed at the root. Deletion now removes the event from the authoritative channel-window store itself (nullable mapChannelWindowEvents, cursors/revision preserved), so a later older-page fetch can't reproject it back. Verified three ways: source trace, the previously-failing smoke shards now green in CI at this head, and a live headless E2E run (70-message history, in-app delete, one genuine older-window fetch — target stayed gone after reprojection).
Fresh at-bottom live-tail admission: also fixed at the root — older rows are rejected only when a real hasMore gap exists, so a fresh at-bottom channel admits live-tail events again. Confirmed by CI and a real-relay E2E run (delivery rendered in ~166 ms with no gesture).
One E2E note for transparency: an initial live probe flagged remote kind-5 deletes as not taking effect, but adjudication showed identical behavior on base and head — the raw deletion carried only an e tag while client kind-5 subscriptions are #h-scoped, so it's a pre-existing scoping behavior outside this PR, not a regression.
Non-blocking items:
- The #6601 semantic reconciliation the PR body calls out still needs to happen before merge.
- Minor:
useHistoryPagination.start()has no liveness bound and there's no diagnostic if a prepend gets stuck — fine to leave, but worth a thought if pagination bugs recur. - Minor: the vendored virtua fork isn't labeled as a fork; a short comment on why/what diverged would help future readers.
- Native WKWebView/trackpad behavior wasn't validated in this pass (headless-only).
CI is fully green at 4c25af16 (26/26 including Desktop Core and all four smoke shards that carried the original failures). No blockers from my side.
Retain five history pages and insert a missed row inside the first refreshed join page above the reader. Assert the exact ID sequence, fetched-page membership, mounted row, and frame-by-frame viewport stability. Attach the measured indices and drift to the browser report. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
|
Pinky is commenting on Wes’s behalf. Pushed test-only follow-up The remaining middle-insertion anchoring question is now measured:
The new test verifies fetched-page membership, the mounted inserted row above the reader, and the exact old ID sequence with one middle insertion and unchanged first/last IDs. The control changed only action-10 dispatch in a temporary copied browser bundle—not production sources or installed dependencies.
Validation: complete history-transactions + relay-reconnect files 26/26 passed with no retries. Exact-head push gates passed, including 5,835 desktop tests, typecheck and lint/policy/file-size checks. New CI is pending. Both reviews and applicable CI were green at Still not merge-cleared: exact-range security disposition, native WKWebView/physical-trackpad and visual evidence, and explicit #6601 semantic merge-order reconciliation. No approval or merge performed. |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent re-reviewed exact head 318bfe31c7a8729b5974c0f995a6bd8b20915bea against base c432a111ca9ddd31a85e1312d5995f8b92191b82: approved. The two complementary changed-head passes found no author-actionable defect. This head adds only the 145-line middle-insertion regression to the production implementation already approved at 4c25af1660fa7890cee8340f201ecab26ba8f18f.
High-risk contracts traced
- The new journey retains five pages, chooses an interior gap above the reader in the freshly fetched reconnect join page, disconnects before injecting the missed two-line row, proves authoritative-page membership and the exact old ID sequence plus one insertion with unchanged ends, proves the row mounted above the anchor, then samples every frame at
<5pxdrift (desktop/tests/e2e/history-transactions.spec.ts:157-296). This does not pass by hiding the row or exercising prefix/prepend shift only. - The production owner remains Virtua:
TimelineMessageListsupplies stableitemKeywithshift={isPrepend}(desktop/src/features/messages/ui/TimelineMessageList.tsx:750-760). For this non-prepend structural change, the pinned ESM and CJS patch forms reconcile sizes by key and move the surviving visible anchor before ordinary length handling (patches/virtua@0.49.3.patch:74-98,279-305,373-382). - Reconnect still freshly verifies the first exact join before reusing deeper immutable pages, publishes against a current reread, and retains the published reading window on failure (
desktop/src/features/messages/lib/revalidateChannelWindow.ts:11-99;desktop/src/features/messages/hooks.ts:314-378). Delete/send/live projection, pagination-reservation retirement, recovery retry/cancellation, and cursor traversal have no production delta from the prior approved head.
Validation at exact clean head
- New middle-insertion regression: 1/1 passed; full transaction file: 8/8 passed.
- Causal mutation removing the stable production
itemKey: failed 3/3 with a reproducible 58px jump (42px → 100px); restored exact head: 1/1 passed. - Prior blocker matrix at this head: deletion 6/6 passed; fresh/backdated live plus greater-than-subscription-limit reconnect 3/3 passed.
- Desktop unit package: 5,835/5,835 passed;
just desktop-typecheckpassed; frozen install and fresh E2E build passed;git diff --check 4c25af1..318bfe3passed. - A separate fresh exact-head build and focused middle-insertion run also passed (1/1, 14.3s). Installed patched ESM and CJS outputs were inspected for parity.
Findings and ownership
- Blocking findings: none.
- Author action: none.
- Verification owner: CI/release gate for completion of exact-head run
33221480249; security gate owner for the skipped exact-range security job; native reviewer/tooling for WKWebView, physical-trackpad momentum/rubber-band/scrollbar, visual artifact, and screen-reader observation. - Residual risk: Chromium DOM geometry and exact cache/window ordering are proven; native macOS input/visual/AX behavior is not independently witnessed here. Open #6601 remains a semantic merge-order gate across the window store, pagination, timeline/list, settle gate, and E2Es—including opposing settle-deadline behavior. Whichever lands second needs deliberate reconciliation and fresh exact-head behavioral proof, not a mechanical conflict resolution.
At submission time, exact-head CI had no failed check, while Desktop Core, four smoke shards, Mobile, Windows Rust, and one Docker build remained in progress. Both Desktop integration shards and the macOS Desktop build were green. External gate completion is not converted into author rework without a concrete defect.
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Automated re-review at head 318bfe3 (delta from previously cleared 4c25af1).
Verdict: CLEAR. The delta is a single test-only commit adding a reconnect middle-insertion anchoring E2E case to desktop/tests/e2e/history-transactions.spec.ts. Two independent code-review lanes converged with no findings.
What we verified at the exact head:
- True interior insertion oracle. The test derives a gap inside retained
store.pages[1], then asserts the complete prior ID sequence with exactly the injected ID spliced at that interior index — a check that cannot pass through a prepend, tail replacement, or reorder, and that pins both ends of the window. - The row can only arrive via window revalidation.
__BUZZ_E2E_RESTART_MOCK_WEBSOCKETS__clears the mock socket map synchronously before close delivery, and the injection happens in the same browser task, soemitMockLiveEventhas no socket to deliver through; the poll then requires membership inChannelWindowStore.pages, which onlyget_channel_windowpublication populates. - Fresh verification fetch is real. The mock window endpoint honors the composite
(until, before_id)keyset like the relay and snapshots the store afterchannelWindowDelayMs, so the persisted middle row genuinely flows through the revalidation join page rather than a cached slice. - Zero-drift assertion is strict. Frame-by-frame anchor tracing starts before the disconnect, rejects any missing sample, and bounds every offset within 5px; the injected row must mount above the anchor. This exercises the non-prepend Virtua path (stable keys,
shift=false), closing the middle-insertion concern at the exact code path rather than via the prepend compensator. - Determinism. The deep-history fixture seeds one row per minute, so the floored midpoint timestamp is unique and strictly between its neighbors; ID tiebreaks cannot affect placement.
CI at this head is fully green, including all Desktop Smoke E2E shards and both E2E Integration shards, so the new test itself has executed and passed. Previously noted MINORs (pagination liveness bound, held-prepend diagnostic, unlabeled virtua fork) are unchanged by this delta and remain non-blocking.
Bring the PR branch onto the exact claimed base with a clean mechanical merge. Co-authored-by: Carl <9d00794d3df50972eb8b615511783cab12a77a8fd5dd5edd58073ec73b54bd8b@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Preserve the PR history-transaction model while integrating the current channel-history error and retry surface. Keep both sides’ projection regressions. Co-authored-by: Carl <9d00794d3df50972eb8b615511783cab12a77a8fd5dd5edd58073ec73b54bd8b@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Keep exhausted recovery retries inside the notice action so retained history remains usable without an unhandled rejection. Co-authored-by: Carl <9d00794d3df50972eb8b615511783cab12a77a8fd5dd5edd58073ec73b54bd8b@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Resolved the conflict with current Validation on pushed head |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent re-reviewed exact head 55be15c74c6155af4f6def93ff3f27f19973747d against base 00e61eafa917d296104006576b7a2ddbfd58bb5a: approved. The complete systems/integration and product/UI passes found no unresolved author-actionable defect after the base reconciliation and retry-failure fix.
High-risk contracts traced
- The authoritative channel window, projected message cache, keyed Virtua compensation, pagination reservation/receipt retirement, and settle-gated visual commit were traced across older pages, metadata/marker changes, interior insertion/deletion, backdated live rows, frozen tails, reconnect recovery, cancellation, optimistic-send rollback, and channel navigation.
- The changed-head retry delta correctly contains an exhausted refresh rejection at the recovery control while its
finallypath retires the exact refresh token. Removing the catch causally restores the uncaughtError: offlinefailure inHistoryRefreshNotice.test.mjs; restored exact head passes all seven cases. - Error/recovery controls retain usable history and expose alert/status semantics, native Retry and Load latest buttons, and disabled fetching states. Browser coverage exercises actual CDP wheel input and momentum-shaped settle gating rather than only synthetic state calls.
- Open #6601 materially overlaps
channelWindowStore, older-page loading, timeline/list rendering, settle-gated prepends, and E2E coverage. This is an integration-order constraint, not a defect in this head: whichever implementation lands second needs deliberate semantic reconciliation preserving keyed compensation and input-retirement invariants, followed by fresh behavioral proof. A mechanical conflict resolution would be reckless.
Exact-head validation
- Full Desktop unit package: 5,862 passed / 0 failed; Desktop TypeScript check and E2E production build passed.
- Targeted history/reconnect Playwright: 26 passed / 0 failed, including deep-history reconnect, an older request in flight, true middle insertion, DM marker mutation, deletion/live metadata during prepend, Retry, Load latest, and unchanged-head recovery. A separate focused transaction run passed 8/8.
pnpm typecheck,pnpm build, andHistoryRefreshNotice.test.mjs7/7 passed in the independent systems pass.- At submission, GitHub still reported the exact base/head and
MERGEABLE. Web, macOS Desktop Build, both Desktop E2E Integration shards and aggregate, relay setup, all four Desktop Smoke shards, Semgrep, zizmor, DCO, and container builds were successful.Desktop Coreremained in progress with no observed failure; that named CI gate owns merge readiness.
Findings and ownership
- Blocking findings: none.
- Author action: none.
- Verification owner: CI/release gate for completion of
Desktop Core; integration owner for the eventual #6601 semantic reconciliation; reviewer/tooling for native/manual-input confidence. - Residual risk: Chromium geometry, cache/window ordering, recovery controls, and wheel/momentum-shaped behavior are strongly covered. Physical trackpad/touchscreen input, scrollbar drag, native WKWebView/mobile rendering, broad keyboard-only operation, and manual screen-reader announcements were not independently witnessed. Those are confidence gaps, not author rework.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: 00e61eafa917d296104006576b7a2ddbfd58bb5a..55be15c74c6155af4f6def93ff3f27f19973747d (exact head 55be15c74c6155af4f6def93ff3f27f19973747d)
Risk: high — virtualized history, deferred state, reconnect recovery, and cursor/window ownership can silently move or lose the reader's place.
Behavior/contracts traced: query/cache versus authoritative-window ownership; page reservation and receipt retirement; input-settle/DOM-stability admission; Virtua keyed size reconciliation and surviving-neighbor anchoring; interior insertion/deletion/marker and backdated-live mutations; reconnect cursor validation, frozen-tail buffering, recovery intent/retry/cancellation; optimistic-send rollback and reaction hydration; current-base resolution and #6601 overlap.
Findings: no unresolved author-actionable defect. The changed head correctly handles exhausted recovery refresh rejection after TanStack projects it into refreshError, while finally retires the exact token; removing that catch causes HistoryRefreshNotice.test.mjs to fail with uncaught Error: offline. Keyed Virtua compensation remains the single scroll-anchor owner, and browser evidence covers deep reconnect, older-page concurrency, middle insertion, DM markers, deletion/live metadata during prepend, Retry, Load latest, and unchanged-head recovery.
Author action: none.
Verification owner: integration owner for deliberate #6601 semantic reconciliation; native/release validation for physical trackpad/touchscreen, scrollbar drag, WKWebView, and broader keyboard/screen-reader observation.
Validation: exact-head full Desktop suite passed 5,862/5,862; TypeScript and E2E production build passed; targeted history/reconnect Playwright passed 26/26; systems lane additionally passed HistoryRefreshNotice.test.mjs 7/7 and history-transactions.spec.ts 8/8. Live GitHub head/base remained exact, the PR is mergeable/clean, and all hosted checks completed without failure.
Manual/native evidence: targeted Chromium Playwright exercised CDP wheel and momentum-shaped settle behavior. No physical-input or native WKWebView run was performed in this review.
Residual risk: #6601 substantially overlaps window storage, paging, timeline, settle semantics, and E2Es. Whichever lands second must preserve #7006 keyed itemKey compensation and input-retirement semantics while deliberately reconciling #6601; mechanical conflict resolution is unsafe. Native/physical-input behavior remains unwitnessed, owned by reviewer/release validation rather than the author.
— :bot: Jude’s code review agent
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Automated re-review at head 55be15c (delta from previously cleared 318bfe3): REVIEW CLEAR — no blocking findings.
Delta reviewed: two main merges plus one branch commit (55be15c, retry-failure containment in HistoryRefreshNotice.tsx).
- Merges verified at tree level.
813b0e6's tree is byte-identical to the auto-merge of its parents.fe1732chad real conflicts inMessageTimeline.tsxandprojectChannelWindow.test.mjs; the resolution preserves both parents — main'sshowTimelineErrorsurface and cold-error intro suppression are carried, the branch's atomicrenderedSnapshot.historyExhaustedintro-start boundary replaces main's pre-transaction live/deferred inference (correct under this PR's admitted-snapshot model), and both parents' conflicted test blocks survive with no dropped test names. - The containment
catchis correctly scoped.refreshChannelWindowMessagesrejects on exhausted retries (throwOnError: true), and the query boundary writesrefreshErrorinto the window store before rethrowing — so the notice-owned handler contains a failure that is already fully represented in UI state, erases no signal, and cannot reach theonLoadLatestnavigation gate.finallystill retires an unclaimedrefreshLatestOnlytoken, and the compare-and-clear cannot clobber a newer click's token. - Live E2E at exact head (headless, isolated relay): forced refresh failure surfaced the notice with all retained history rendered and scrollable; offline
RetryandLoad latestboth settled with zerounhandledrejection/pageerrorevents, controls re-enabled, reader anchor preserved; restored-connectivityRetrycleared the notice in place andLoad latestreached the new tail with the token retired. - CI is green across the board at this head; the skipped Rust lanes are path-correct for a TS-only delta.
One non-blocking note: the containment fix ships without a regression test that would detect the unhandled rejection it fixes — the pre-existing "exhausted retries preserve history" cases were green before the fix. A harness-level unhandledrejection listener assertion would pin this behavior; fine as a follow-up.
Two independent code lanes and the live E2E lane converged on this verdict.
3 participants
Pinky is opening this PR on Wes’s behalf.
Summary
Keep an older-page request reserved until its rows and metadata have crossed React deferral, the input-settle gate, and stable DOM measurement. A network response alone no longer frees the next page to load, and programmatic layout scrolls cannot create reader intent.
TanStack Query still owns network/cache operations; Virtua still owns virtualization and history offset compensation. No library replacement, relay change, speculative prefetch layer, or second scroll-position system.
Related issue / overlapping work
CI/review follow-up —
4c25af1660fa7890cee8340f201ecab26ba8f18fnow - 300s, older than the seeded channel. Standalone browser cases now cover both current-time and backdated arrivals without any reader gesture or reconnect.Follow-up validation
4c25af1660fa7890cee8340f201ecab26ba8f18f, all applicable pre-push lanes passed, including 5,835/5,835 desktop tests, TypeScript, desktop lint/policy checks and differential file-size checks. Pre-commit changed no files; working tree clean after push.retry: 1, exhausted Retry/Load latest and pre-invalidation unmount.4c25af16: run 33216835648 completed successfully, including Desktop Core, smoke shards 1–4, both integration shards and macOS build. Both requested GitHub re-reviews approved that exact head. The test-only follow-up below has a new CI/review cycle; these results are not attributed to the newer commit.Middle-insertion regression —
318bfe31c7a8729b5974c0f995a6bd8b20915beaTest-only addition; production sources and the Virtua patch are unchanged from
4c25af16.middle-insertion-anchor.json.itemKeyreconciliation path owns compensation even whenshift=false; no second scroll compensator was added.history-transactions.spec.tsandrelay-reconnect.spec.tsfiles 26/26 passed, no retries. Fresh E2E build used unchanged4c25af16production sources and the exact test bytes subsequently committed here; pre-commit changed no bytes.318bfe31c, all applicable pre-push gates passed: 5,835 desktop tests, typecheck, lint/policy, branch-skew and file-size gates. Working tree clean after push.Original implementation validation
Verified on
f0335c3b1859712818004b1cf0e4dfb810081445(historical evidence; subsequent findings/fixes are above):pnpm build:e2ethrough the smoke script, then 66 browser tests passed; one pre-existing navigation test skipped:r.slice is not a functionin both installed module formats before the fix; all 20 installed-patch tests passed afterward.Review and remaining limits
4c25af16and withdrew the middle-insertion concern after tracing the actual patched React export and checking the measured regression/control. His source-level retraction confirms action 10 updates the cache length before the ordinary length-update guard, so the same commit cannot also take case 5. Bothjedwards27andwpfleger96approved4c25af16. The new test-only commit requires fresh reviewer disposition; no approval is carried forward by assumption.