refactor(desktop): give the Renderer the transcript window - #5170
refactor(desktop): give the Renderer the transcript window#5170Astro-Han wants to merge 2 commits into
Conversation
Main owned the presentation window: a resident range with reading anchors, navigation versions, overlay settlement across pages and eviction bookkeeping, while the Renderer kept its own scroll state on top. Two owners of one window meant every reader gesture met Main's range accounting, and the reading position existed in six representations that had to agree. Main now keeps a tail cache and answers page requests pass-through. `loadBefore` / `loadAfter` return a page without touching the cache, `loadAround` and `loadLatest` return a reset snapshot, and catch-up evicts whole Turns from the oldest edge while always keeping the newest. The Renderer owns the window: it extends it by pixel bandwidth, trims it with `retain`, and re-opens the edge it trimmed as a gap. `hasOlder` / `hasNewer` are Host page cursors on an answer; the Renderer combines them with its own coverage, so paging reaching an end never means nothing exists outside the window. `navigationVersion` invalidates navigation only — Session id, replica generation and Host epoch stay independent identity checks. Overlay settlement follows the same rule. Main used to broadcast `completedOverlayMessageIds`, so a window retired an overlay whether or not it installed the durable row that replaced it. A window off the tail declines tail growth, so it deleted the overlay and dropped the body in the same batch and the Turn vanished from the reader's view. The catch-up broadcast also filtered its rows through Main's own tail residency, so a row installed and immediately evicted under budget never reached any window at all. Retirement is now the window's own inference — installing a durable row retires the overlay it settles — and the broadcast carries every row the catch-up read. The reading position is one representation: the authority publishes the Turn crossing the top of the scrollport, the prompt rail derives its tick from that instead of running its own observers, and the controller keeps a bookmark to re-anchor after a replica generation change. Behavior changes: opening a Session prefetches history until two viewports sit above the reader; tail growth marks read while any consumer is open, where it previously waited for the reader to have nothing newer. Supersedes #5147, which patched these symptoms at the old window authority. Closes #5163 Generated-by: Claude Code
d171fbf to
cd811e1
Compare
The suite asserted that paging works and stays bounded, and sampled the mounted count only once the range had settled. Neither says where the reader ended up while a page was installing, which is the whole of what #5163 reports. This probe reads every frame, and at each change of the mounted range compares a Turn present on both sides: with no input between two frames its document position must not move, so `Delta top + Delta scrollTop` is zero unless the boundary displaced the reader. Measured settled-to-settled rather than across the changing frames: the range passes through an intermediate commit that mounts far more Turns than it keeps, and scroll anchoring corrects after layout, so a reading taken inside the change reports a correction that never reached the screen. It fails on this branch. Pure trims land at 18px; every page install displaces the reader by about 1800px. Also stops asserting `data-search-highlight` after waiting for the jumped Turn to mount. That highlight clears itself 2.2s after the command lands, so the assertion fails whenever loading the page around the Turn takes longer than the flash - a 3s pass turning into an 18s timeout under load, reproduced 2 of 6 runs. The jump's landing place is read from the reading position instead, which does not expire. Restores the band-check guard removed in the previous commit. A controlled comparison over 6 runs each puts the flake at 2 of 6 without it and 3 of 6 with it, so the ablation that removed it rested on a single passing run and the guard is not what that flake was about. Generated-by: Claude Code
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
Automated review, posted at M4n5ter's request. Parallel deep-code-review and bounded simplify-audit, with independent adjudication. Reviewed bba473c27d9cc87c9cde51d0f68f68727ccd22e7 against merge base fa0ff028e14b96bc14d987f156a2ad79d8b45151; the late update was checked before submission.
Request changes: three Blocking findings and two Important findings are attached inline. Correctness verdict: not acceptable. Design verdict: acceptable — Renderer-owned presentation is a net simplification; these fixes do not require returning window ownership to Main. No additional structural simplification is requested.
Verification: 41 existing targeted tests passed (35 range-store/navigation-race + 6 scroll-hook), bundled from the affected sources. Diagnostic probes reproduced fragmented reconnect, extension-after-trim, and failed-prefetch retry. An additional pending-bookmark eviction probe failed on cd811e18 and passes with the guard restored in this head; that resolved issue is not reported as outstanding. Search cancellation was verified through the production call chain, and the new displacement oracle was checked algebraically. UI probes use synthetic geometry, not a real browser displacement trace. No production changes were made.
The latest required CI test check is red at the Electron E2E budget gate (3 recorded tests versus 4 in the spec). Full repository tests and the new browser trace were not independently run.
中文摘要
已包含提交前刚推送的增量，统一裁决后确认 3 项 Blocking、2 项 Important，逐项附在代码上。单一 Renderer 窗口 owner 的方向成立，没有值得追加的结构性简化；目前实现不能通过 correctness 审查。
41 个现有定向测试通过；重连、裁剪后旧扩页、失败重试三个诊断复现仍失败。已验证新提交修好了待定位书签被裁剪的问题，因此不再将它列为未解决项。另通过生产调用链确认预取取消搜索，并确认新增位移测试的公式不能区分正确与失效的滚动锚定。最新 CI 仍为红色。
| if (batch.reset) { | ||
| return batch.navigationVersion === undefined || | ||
| batch.navigationVersion === this.#navigationVersion || | ||
| batch.generation !== this.#liveGeneration; |
There was a problem hiding this comment.
Blocking — admit the whole reconnect snapshot, not only its reset batch.
After loadAround or loadLatest, the Renderer expects navigation N > 0. Registry recovery reopens the consumer, whose navigation version starts at 0. This exception admits the new generation's first batch, but only that batch has reset=true; lines 347–348 reject every continuation carrying version 0. Preload still ACKs those batches, including the final ready batch.
A direct probe against this head — initialize the store, expectNavigation(1), then feed an encoded new-generation/version-0 snapshot containing a 300 KiB message — leaves range().ready === false and the replacement message incomplete. The existing navigation-race tests pass because they do not exercise this combination.
Give the entire recovery snapshot one consistent generation-scoped admission rule, while keeping navigation filtering for command answers. Removing Main's navigation replay should not require restoring Main's presentation authority.
中文
执行过导航后，重连的新 consumer 从版本 0 开始；新 generation 仅首批因 reset 特例获准，续批及 ready 均被旧导航版本过滤，但仍被 ACK。300 KiB 快照定向复现停在 ready=false。恢复快照整条流应使用一致的 generation 接收规则，命令回复仍保留导航版本过滤。
| if (canLoad('up') && above < screen * 2) requestHistory('up'); | ||
| if (canLoad('down') && below < screen * 2) requestHistory('down'); |
There was a problem hiding this comment.
Blocking — automatic prefetch clears an outstanding search before its target arrives.
A global search can open another Session with a target outside its initial tail. If that bootstrap is short (e.g. two 600px Turns in a 600px viewport, with older history), the initial band check automatically requests an older page while the target's remote loadAround is still pending. This goes through loadHistory, whose cancel() unconditionally clears the current search target and cancels restoration. When the remote target finally mounts, the reveal command has already disappeared.
This does not require same-frame React timing or another reader gesture. The new mounted-target guard in this commit does not cover it: prefetch runs before that guard, and this target is not mounted yet.
Background filling must not consume explicit navigation intent. Keep it from cancelling an outstanding search/restoration, or defer that filling until the explicit navigation settles. A focused regression should delay the searched-for page, let the short-tail prefetch run, then deliver the target and assert it is actually revealed.
中文
跨 Session 搜索时，目标页仍在远端加载，短 tail bootstrap 会先自动补页；这经 loadHistory → cancel 清除尚未处理的搜索目标。目标稍后挂载也不再定位，无需假设同一帧内的渲染顺序。新 guard 在预取之后，而且只保护已挂载目标，未封住此路径。后台补页不能消费显式导航意图。
| const anchor = edge === 'older' ? range.oldestSequence : range.newestSequence; | ||
| const task = command(false, (value, navigation) => | ||
| edge === 'older' | ||
| ? value.loadBefore(anchor, maxBytes, navigation) | ||
| : value.loadAfter(anchor, maxBytes, navigation), |
There was a problem hiding this comment.
Blocking — reject extensions that no longer join the retained window.
An extension captures this edge anchor, but store.retain() can remove that edge without invalidating the request. Its answer still carries the current navigation version, so accept() installs it unconditionally as a command answer.
A delayed-read probe against the production controller/store starts with rows 10..20, begins loadBefore(10), retains 18..20 while the read is outstanding, then returns row 9. The resulting window is [9, 18, 19, 20]: rows 10..17 are missing internally, but the model only exposes outer-edge gaps. Subsequent loadBefore anchors at 9 and loadAfter at 20, so neither fills the missing interval. This is reachable when a reader moves far enough to trim an edge before its network page returns; a small returned Turn need not trigger another trim.
At admission, require an extension to remain connected to the current window, or invalidate the affected in-flight extension when trimming its edge. Keep the single contiguous-window model instead of introducing internal-gap bookkeeping.
中文
扩页以旧边缘为锚点，retain 裁掉该边缘却不使请求失效。定向复现：10..20 开始向前读，期间保留 18..20，旧页 9 返回后得到 [9,18,19,20]。10..17 成为模型无法表达、两端分页无法补回的内部缺口。应在回复接收时验证仍连接当前窗口，或裁剪相应边缘时让在途扩页失效，无须增加多段窗口模型。
| void Promise.resolve(load?.()).catch(() => undefined).finally(() => { | ||
| inFlight[direction] = false; | ||
| check(); |
There was a problem hiding this comment.
Important — do not immediately retry an unchanged failed prefetch.
The finally clears the in-flight flag and calls check() even after failure. If the geometry and history flag are unchanged, check() immediately issues the same request again. Desktop's loadHistory also catches errors, reports them, and resolves normally, so checking rejection alone would not fix the production path.
With the existing two-Turn fixture and a rejecting loader, a single mount issued 20 consecutive requests without input or range changes; the probe deliberately left request 20 pending to stop the loop. A persistent Host/IPC read failure therefore causes unbounded requests and error reporting.
The messages-change effect already rechecks the band after successful data installation. Remove the unconditional completion-driven retry, or require actual progress before continuing; failures should await a fresh input, range change, or recovery trigger.
中文
失败后 finally 立即 check，相同边缘和 history flag 又触发相同请求。单次挂载定向复现连续请求 20 次，测试主动悬停第 20 次才停止。Desktop 会捕获错误并正常 resolve，因此只判断 reject 不够。成功的数据变化已有重新检查入口；失败应等待新输入、范围变化或恢复事件。
| for (const [turnId, top] of current.tops) { | ||
| const wasAt = before.tops.get(turnId); | ||
| if (wasAt === undefined) continue; | ||
| carried += 1; | ||
| const displaced = Math.abs(top - wasAt + scrolled); |
There was a problem hiding this comment.
Important — this oracle measures document movement, not reader displacement.
top - wasAt + scrolled is the change in the Turn's document coordinate. Scroll anchoring is meant to preserve its viewport coordinate while its document coordinate changes.
For example, prepend 100px above a carried Turn with no reader input:
- Correct anchoring: viewport top is unchanged and
scrollTopincreases by 100 → this reports 100px. - Broken anchoring: viewport top increases by 100 and
scrollTopis unchanged → this also reports 100px.
Conversely, an unintended 100px scroll-offset jump gives Δtop=-100, ΔscrollTop=100, so the metric reports 0px when that Turn is compared. Thus the asserted threshold cannot establish the no-displacement behavior promised by this new test.
Compare the surviving reading anchor's viewport offset over a boundary without reader input, or compensate only for known reader input rather than the total scroll-offset change. Calibrate the oracle with a correctly anchored prepend and a deliberately displaced case before treating this as evidence for #5163.
中文
该公式算的是文档坐标变化，不是读者看到的位移。上方插入 100px，正确锚定（top 不变、scrollTop +100）与失效锚定（top +100、scrollTop 不变）都会报 100；反而纯 scrollTop 跳变会被抵消为 0。应测阅读锚点的视口偏移，排除或单独扣除读者输入，并先校准正确／错误锚定两种结果。
2 participants
Summary
Main owned the presentation window — a resident range with reading anchors, navigation versions, overlay settlement across pages and eviction bookkeeping — while the Renderer kept its own scroll state on top. Two owners of one window meant every reader gesture met Main's range accounting, and the reading position existed in six representations that all had to agree.
Main now keeps a tail cache and answers page requests pass-through.
loadBefore/loadAfterreturn a page without touching the cache,loadAroundandloadLatestreturn a reset snapshot, and catch-up evicts whole Turns from the oldest edge while always keeping the newest Turn. The Renderer owns the window: it extends it by pixel bandwidth, trims it withretain, and re-opens the edge it trimmed as a gap.This resolves the three contracts raised on #5163:
completedOverlayMessageIds, so a window retired an overlay whether or not it installed the durable row replacing it. A window off the tail declines tail growth — so it deleted the overlay and dropped the body in the same batch, and the Turn vanished from the reader's view. The catch-up broadcast also filtered its rows through Main's own tail residency, so a row installed and immediately evicted under budget reached no window at all. Retirement is now the window's own inference (installing a durable row retires the overlay it settles) and the broadcast carries every row the catch-up read.hasOlder/hasNewerare transport-level cursors on a Host answer. The Renderer combines them with its own coverage: an edge it trimmed to meet its budget becomes a gap again on the spot, so paging reaching an end never implies nothing exists outside the window.navigationVersioninvalidates navigation only. Session id, replica generation and Host epoch remain independent identity checks on every batch. Invalidation also covers late success, errors, replay and fragmented delivery: an in-flight page for a replaced window has its error swallowed rather than surfaced as the current window's failure, and a new version clears partially received fragments so a late fragment cannot splice into the new window.The reading position is now one representation: the scroll authority publishes the Turn crossing the top of the scrollport, the prompt rail derives its tick from that instead of running its own IntersectionObserver / MutationObserver / rAF loop, and the controller keeps a bookmark to re-anchor after a replica generation change.
Supersedes #5147, which patched these symptoms at the old window authority.
Closes #5163
Behavior change
Verification
Regression for the vanishing Turn (
transcript-overlay-settlement.test.ts, "a window parked off the tail keeps the Turn it is reading when that Turn completes") fails without the fix, on the reader-visible body:Ablation removed one guard this refactor had introduced — a check that kept a mounted Turn a pending reveal command was about to scroll to from being trimmed by the bandwidth pass. Every unit test and both e2e specs still passed without it, so it is gone rather than kept on a race nobody can reproduce.
Not run: the full repository suite, and the cold-restart upward-scroll displacement trace proposed on #5163 — the existing
transcript-scroll-costsuite asserts structural cost and reachability, not frame-by-frame reader displacement. That trace is tracked as remaining work below.Remaining work
turnId + offsetPx, page requests, resets, mounted Turns and long tasks per gesture, asserting no displacement at page boundaries. The renderer-replacement variant asserts exactly one live transcript consumer first, so Desktop transcript consumers survive Renderer loss and accumulate WebContents listeners #5136 cannot contaminate it.AI use
Select exactly one:
Tool(s) and scope: Claude Code — implementation, test authoring and verification across Main, preload and Renderer, under human review.
Checklist
Does this PR entail a change in behavior?