feat(web-shell): Improve split-view session navigation - #11250

[P1] Please split the independent history and CI changes from the split-view feature.

The history fix itself matches the failure I independently reproduced from #11282: pagination can start before the virtual rows mount, so capture() returns no reading anchor; after the prepend, the virtualizer positions the old row using the 19,200 px estimate and later measures it at 18,914 px, leaving the stable 286 px drift seen in CI. Waiting for a visible anchor before viewport.load() addresses the actual race, and the unit plus CPU-throttled browser coverage is useful.

That fix is independent of split-view navigation, as is moving the Web Shell browser job to hosted runners. The current PR is now 23 files and +1,319/-88 across three separately reviewable changes: split-view UX, CI runner routing, and the history-anchor bugfix. The latter was reproduced on an unrelated PR because it already exists on main; it should not need this feature branch to land. Could we move 0de61b5 into a focused fix(web-shell) PR and handle the runner change separately, leaving #11250 scoped to split-view behavior?

Review at head 7c392d2c — no findings. Verification of the two claims this PR most needs to hold

Comment rather than approve: this head is a merge of main landed at 09:13Z and every substantive lane is still in_progress, so there is no CI evidence to approve against yet. What follows is what I read at head.

The keyboard-isolation claim holds, and the code states the invariant itself

The description's strongest claim is that after toolbar navigation, "Enter, Escape, and digits immediately afterward cannot submit an answer". I traced it end to end rather than accepting it, because if it were wrong a user could approve a tool by navigating:

• SplitView.tsx:388-392 goToPendingPane picks the next pending pane; :402-404 resolves the focus target by data-pane-session-id and scrolls it into view; the target is the pane slot at :551-555 — role="group", tabIndex={-1} — which is an ancestor of ChatPane, not the approval panel.
• :405-406 documents the rule verbatim: "Approval panels submit on Escape, Enter, and digits. Navigation must stop outside those keyboard scopes until the user deliberately enters."
• Both approval surfaces scope their handling to the panel, not to document/window: ToolApproval.tsx:487 onKeyDown={handleKeyDown} with the intent recorded at :403 ("Keyboard handling is scoped to the panel (onKeyDown), so it only fires while focus is inside this approval"), and AskUserQuestion.tsx:668 likewise. With focus on the ancestor slot, a keydown does not reach either handler.
• The isolation is therefore structural (focus scope), not a suppression flag or a time window. That matters: there is no flag that can fail to be cleared (leaving later approvals dead) or be cleared early (letting a stray Enter through). keyboardActive at ToolApproval.tsx:371 / AskUserQuestion.tsx:589 only governs auto-focusing a safe default, not whether onKeyDown runs, so Tab/click into the panel still works normally.
• SplitView.tsx has exactly two document-level keydown listeners, :273 and :437; neither calls a confirm or submit path.

R3-2 (round 4's only finding) is load-bearing and correct — with corrected line numbers

The ledger anchors it at App.tsx:7958; after this head's merge of main that region is an unrelated shadow-surface focus trap. The callback is now at App.tsx:8062-8066:

const handleSplitPendingPanesChange = useCallback(
  (ids: string[]) =>
    setOuterSplitPanePending(ids.includes(connection.sessionId ?? '')),
  [connection.sessionId],
);

The [connection.sessionId] dependency does the work the finding asked about: when the main session changes the callback identity changes, and SplitView.tsx:208-211 pairs a report effect with useEffect(() => () => onPendingPanesChange?.([]), [onPendingPanesChange]). React runs the cleanup before the re-report within the same commit, so the terminal state is evaluated against the new main session — the notice follows it, as the description claims.

One thing I checked and am recording as not a finding

Pane-level pending reporting (ChatPane.tsx:573-578) uses an approvalActive derived from extractPendingPermission(blocks) at :549-572 with no canActOnPendingApproval gate, while App.tsx:6628-6629 derives approvalOverlayActive from a gated value. That looks like two different notions of "pending" for one session, and it is worth a moment's suspicion — but canActOnPendingApproval is defined at App.tsx:6607-6609 as !(connection.catchingUp && sidebarSwitchingSessionId !== null), i.e. a transient guard for the outer surface's single connection being repurposed mid sidebar-switch. It is not an ownership or shared-session check. A split pane has its own connection and renders its own approval panel ungated, so ChatPane reporting exactly what it renders is self-consistent, and the outer notice being suppressed during catch-up is the guard doing its job. No defect. Writing it down so the next reviewer does not spend the same time.

The CI reroute

.github/workflows/ci.yml moves web_shell_e2e_smoke from the shared-pool expression to runs-on: 'ubuntu-latest' with a flat timeout-minutes: 20, and keeps all three self-hosted recovery steps (workspace ownership, stale .qwen sweep, disk floor gate behind runner.environment == 'self-hosted') so a future reroute is a one-line change. Two details I want to credit explicitly:

• scripts/tests/review-worktree-cleanup-workflow.test.js widens its selection to also match any job carrying the Clean stale .qwen before checkout step, not just pool-routed jobs — so moving this job to hosted cannot silently drop its sweep out of coverage. That is the right direction for a test that exists to catch reroutes.
• The deleted keeps web_shell_e2e_smoke above its build-plus-browser budget on ECS test carried measured evidence (hk3-9 npm ci 19m46s of a 20m budget; hk4-23 install 10m33s vs 4m53s on hk5-2). The replacement pins runs-on == 'ubuntu-latest' plus 20 everywhere, so a reroute back to the pool now fails loudly instead of quietly restoring a flat timeout. The measurements themselves are only in git history now; if the pool is expected to come back, that context is worth preserving in the design doc this PR adds.

Cross-PR observation, flagged as inference

web-shell.history-viewport.spec.ts gains CPU throttling (Emulation.setCPUThrottlingRate: 4) for the 200-record case only, and 0de61b5c4 is "fix(web-shell): Preserve history anchors during slow rendering". On PR #11152 the web-shell Browser Regression lane is red at two consecutive heads on exactly this spec's 200-record case (expect(received).toBeLessThanOrEqual(expected), 3/3 attempts), and I attributed it to main-side #11208/#11323 rather than to that PR. I did not run either, so this is an observation and not a claim: if the slow-render paging race is what that lane is hitting, the fix lives here, which would confirm the attribution and mean #11152 should not be charged for it. Worth saying out loud in whichever PR lands second.

Scope

Three areas travel together: the split-view UX feature, the CI runner reroute, and the history-paging fix. Normally I would flag config/CI changes bundled with application code, but the reroute is motivated by this PR's own browser gates repeatedly losing resources on the shared pool, and the paging fix is in the same package and the same e2e spec. Defensible as one change; noting it only because the reroute has repo-wide blast radius (it decides where every PR's browser gate runs) and a reader of the diff deserves to see it called out rather than discover it.

State

reviewDecision is CHANGES_REQUESTED from ci-bot reviews anchored at 5fb20297b (09-07T09:59Z) and e5202ac3a (09-07T15:18Z) — many heads back. The latest ledger is round 4 at 9098dd333 with floor: "o", no Critical and one Suggestion (R3-2, verified above). 37/37 threads resolved. I will re-check once Lint & Static, Test (ubuntu-latest), Integration Tests (no-AK) and the web-shell lanes report on this head.

Approving at head 7c392d2c — the one red lane is main's, proven by an identical failure on the base

In pullrequestreview-5139858213 I recorded no findings and held the vote only because the merge of main at 09:13Z had every substantive lane still running. They have now reported.

Result: Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, Capture web-shell visuals, Desktop Shell (ubuntu-22.04 and windows-2022), Classify PR, assign, label, route ×2 and Remind on force-push are success. Test (ubuntu-latest, Node 22.x) is failure.

That failure is not this PR's, and I verified it rather than assuming it from file-disjointness:

• At this head: src/serve/capabilities-docs-contract.test.ts > conditional serve capability documentation > keeps the daemon index capability counts in sync, AssertionError: expected 158 to be 159 // Object.is equality, Tests 1 failed | 29102 passed | 90 skipped (29193).
• At 7dedcbfd71 — which is main, and is this PR's base: the same job is failure (09:25:20Z -> 09:45:55Z) with the same test, same assertion, same tallies: expected 158 to be 159, 1 failed | 29102 passed | 90 skipped (29193).

Identical failure on the base and on the head, in a file this PR does not touch (its 23 files are web-shell client, .github/workflows/ci.yml, three scripts/tests/*ci* pins, one e2e spec and one design doc — nothing under src/serve/ and no daemon capability docs). So this PR inherits it.

Main is red and someone should own it separately. docs/developers/daemon/00-index.md reads 158 on main while the registry the test compares against has 159, i.e. a capability was added to the registry without the index doc's count being bumped. Every PR that merges main from 7dedcbfd71 onward will show a red Test (ubuntu-latest) until that is reconciled, and it will keep costing reviewers this same attribution pass. Worth its own fix rather than riding along here.

My code position is unchanged from the earlier review and I am not restating it at length: the keyboard-isolation claim holds structurally (focus scope, not a suppression flag), R3-2's [connection.sessionId] dependency is load-bearing and correct at App.tsx:8062-8066, the CI reroute keeps its recovery steps and widens review-worktree-cleanup-workflow.test.js so the reroute cannot silently drop coverage, and the one thing I suspected — pane-level pending reporting bypassing canActOnPendingApproval — is not a defect once that flag's actual definition is read.

To be explicit about what this vote does not cover: it does not certify Test (ubuntu-latest) green, because it is not. It certifies that the failure is main's and that nothing in this diff contributes to it.

reviewDecision is still CHANGES_REQUESTED from ci-bot reviews anchored at 5fb20297b and e5202ac3a, both many heads back and both predating the round-4 ledger that found no Critical. Those need dismissing or a fresh round; main wants two approvals and @wenshao cannot supply one as author.

Read-only review pass at head 52484fec. I re-verified all three recorded Criticals against the code at this exact head and scanned the substantive production logic. All three are fixed, I found no Critical of my own, and CI is green with nothing failing or pending, so this is an approval.

Recorded blocking findings — all verified fixed at this head

1. R1-1 (SplitView.tsx — the "go to next session awaiting input" cycle moved DOM focus onto an option button inside an approval panel whose panel-wide key handler submits a response unconditionally, so Enter/Escape/a digit right after navigating could answer a request the user never chose to answer). Navigation now stops outside every approval keyboard scope. The focus effect resolves the pane container by data-pane-session-id and focuses that (SplitView.tsx:399-409), the container is genuinely focusable because the pane wrapper carries tabIndex={-1} (:555) beside data-pane-session-id={sessionId} (:551), and the invariant is written at the site: "Approval panels submit on Escape, Enter, and digits. Navigation must stop outside those keyboard scopes until the user deliberately enters." scrollIntoView({ block: 'nearest', inline: 'nearest' }) handles the reveal, setPaneFocusId(null) makes the effect one-shot, and the related focus hand-offs are covered too — a focused pending button that disappears passes focus to Back (:213-219). The @smoke test navigates hidden tool and question approvals without answering them pins it end to end: it presses Enter, 2, 3 and Escape after navigating and asserts nothing was answered, with expect(daemon.permissionRequests()).toHaveLength(0) as the backstop.
2. R2-1 (the @smoke containment test sampled the popover's box once, before floating-ui's collision shift had finished, so it asserted against a mid-transition position). The containment check is now a polling assertion — expect.poll(async () => { …boundingBox()… }).toBe(true) over the details dialog against both the pane and the shell root (web-shell.split-persist.spec.ts:461-477) — so it waits for the settled position instead of racing the transition.
3. R2-2 (the @smoke test pressed Escape once and asserted the tiled row returned, without accounting for a popover its own previous interaction could open). Each iteration of the containment loop now closes the popover it just opened with an explicit Escape before moving to the next pane (:478), and in the restore flow the Escape at :421 follows a .focus()-only sequence whose focus landing on back is asserted first (:418-420), so no popover is open to swallow the keypress.

Critical-only scan of the production diff — no blocking issue found

• SplitView.tsx (+144/-2). The active-pane derivation falls back to a neighbour on close by clamping the previous index into the new array, and the sync effect only runs when the paneIds identity actually changed and only takes focus when document.activeElement === document.body, so it cannot steal focus from a user mid-interaction. pendingIds is memoized to keep the report identity stable (the comment names the loop it prevents), the unmount cleanup reports an empty pending set, and handleApprovalChange returns the current set unchanged when the value already matches, which is what keeps an approval change in another pane from re-rendering the whole shell.
• ChatPane.tsx (+40/-3). The approval report is effect-driven with a cleanup that reports false on unmount or session change (:575-578), so a pane that detaches or fails cannot leave a stale pending count behind — the property the main-session notice depends on. The active indicator is exposed as data-pane-active plus aria-current="location" and an aria-label, matching the accessibility claims in the description.
• TranscriptViewport.tsx (+32/-3). History paging now waits for a real reading row before requesting: load() retries through requestAnimationFrame up to eight frames, assigns the anchor only once capture() succeeds, and bails if scrollIntent moved in the meantime; a single-flight guard prevents stacking pending loads, handleScrollIntent cancels the pending frame, and a useLayoutEffect cleanup keyed on viewKey cancels it on unmount or view change. The failure direction is the safe one — a pathologically slow mount skips the page rather than loading with no anchor to restore, which is the 286-pixel jump this closes.
• ci.yml (+14/-22). The browser job is pinned to hosted ubuntu-latest with the same 20-minute limit it already had on hosted, the shared-pool recovery steps are deliberately retained so a future reroute is a runner change only, and no assertion, budget or trigger is relaxed; the change is confined to this job, so other jobs' routing is untouched, and the workflow-contract suites that pin these lanes were updated in the same commit.

Recorded non-blocking items I did not treat as gates. A human reviewer asked at P1 that the independent history and CI changes be split out of the feature, and then extracted the history-anchor fix into its own PR preserving the original author; this PR still carries both changes, and that reviewer approved this head anyway, so I read the request as scope preference rather than a blocker. The last two bot rounds are Suggestions-only, and their open items — the tooltip timestamp re-derivation, the pane-specific collision-boundary rule, an onSelect handler identity, and the untested surface the triage pass named — are non-blocking on my reading too.

What I did not read line by line. App.tsx (+16/-3, the wiring for showSessionDetails and the pending-pane report), SessionDetailsTooltip.tsx (+11/-4), i18n.tsx (+4), the two CSS modules, and the ~900 lines of new unit and e2e test bodies beyond the regions bearing the three Criticals. I relied on the green lanes for those, including the browser smoke on hosted at this head.

CI at this head. 15 checks pass with 28 skipped and nothing failing, cancelled or pending — the browser job included, on the hosted runner this PR routes it to. Nothing is attributable to a defect introduced by this PR, and no lane I read is a gate either way.

中文说明：本次在 head 52484fec 上评审，结论为 APPROVE。三项历史 Critical 我都对照当前代码逐条确认已修复，且自行扫描了主要生产逻辑，未发现新的 Critical。R1-1（"跳到下一个待输入会话"把 DOM 焦点移到审批面板内部的选项按钮上，而该面板的全局键盘处理会无条件提交响应，因此导航后紧接着的 Enter/Escape/数字可能回答一个用户并未选择回答的请求）：导航现在停在所有审批键盘作用域之外——焦点副作用按 data-pane-session-id 找到窗格容器并对其调用 focus（SplitView.tsx:399-409），该容器确实可聚焦，因为窗格外层带 tabIndex={-1}（:555）与 data-pane-session-id={sessionId}（:551），现场注释也写明了该不变量："审批面板会在 Escape、Enter 与数字上提交，导航必须停在那些键盘作用域之外，直到用户主动进入"；scrollIntoView({block:'nearest',inline:'nearest'}) 负责揭示，setPaneFocusId(null) 使该副作用只生效一次，相关的焦点交接也已覆盖——处于焦点状态的待处理按钮消失时把焦点交给 Back（:213-219）。@smoke 用例 navigates hidden tool and question approvals without answering them 端到端钉住了它：导航后依次按 Enter、2、3、Escape 并断言没有任何请求被回答，并以 expect(daemon.permissionRequests()).toHaveLength(0) 兜底。R2-1（该 @smoke 包含性用例只采样一次弹层外框，早于 floating-ui 的碰撞位移完成，因而断言的是过渡中途的位置）：包含性判定现在改为轮询断言——expect.poll(async () => { …boundingBox()… }).toBe(true)，同时对照窗格与外壳根元素（web-shell.split-persist.spec.ts:461-477）——等待位置稳定而不再与过渡竞争。R2-2（该 @smoke 用例只按一次 Escape 就断言平铺行已恢复，却没有考虑它自己前一步交互可能打开的弹层）：包含性循环的每一轮现在都会用显式 Escape 关闭刚打开的弹层再进入下一个窗格（:478）；而恢复流程中 :421 的 Escape 之前只有 .focus() 序列，且先断言焦点落在 back（:418-420），因此不存在会吞掉该按键的弹层。生产 diff 的 Critical-only 扫描未发现阻塞问题：SplitView.tsx(+144/-2) 的活跃窗格推导在关闭时以"把原索引夹入新数组"的方式回落到相邻窗格，同步副作用只在 paneIds 身份确实变化时运行、且只在 document.activeElement === document.body 时取焦，因此不会从正在交互的用户手中抢走焦点；pendingIds 经 memo 保持上报身份稳定（注释点名了它防止的循环），卸载清理会上报空集合，handleApprovalChange 在值已一致时原样返回当前集合——这正是"其他窗格的审批变化不会导致整个外壳重渲染"的实现方式。ChatPane.tsx(+40/-3) 的审批上报由副作用驱动，并在卸载或会话切换时以清理函数上报 false（:575-578），因此分离或失败的窗格不会留下陈旧的待处理计数，而主会话提示正依赖该属性；活跃指示以 data-pane-active 加 aria-current="location" 与 aria-label 暴露，与描述中的可访问性主张一致。TranscriptViewport.tsx(+32/-3) 的历史翻页现在会等到真正出现可读行才发请求：load() 通过 requestAnimationFrame 最多重试八帧，只有在 capture() 成功后才写入锚点，且期间 scrollIntent 变化即放弃；单飞守卫避免堆积多个待处理加载，handleScrollIntent 会取消待处理帧，键于 viewKey 的 useLayoutEffect 清理会在卸载或视图切换时取消它。失败方向是安全的一侧——渲染极慢时会跳过这一页，而不是在没有可恢复锚点的情况下加载，而那正是本次要闭合的 286 像素跳变。ci.yml(+14/-22) 把浏览器作业固定在托管 ubuntu-latest 上并沿用它在托管环境本就有的 20 分钟上限，刻意保留共享池的恢复步骤以便将来改回只需换 runner，且没有放宽任何断言、预算或触发条件；改动仅限于该作业，因此其他作业的路由未受影响，而钉住这些通道的 workflow 契约测试也在同一提交中更新。我未作为门禁的既有非阻塞项：一位人类评审者以 P1 建议把独立的历史与 CI 改动从本特性中拆出，并随后自行把历史锚点修复抽成独立 PR、保留原作者；本 PR 仍同时携带这两部分改动，而该评审者对本 head 依然投了批准，因此我把该请求读作范围偏好而非阻塞项。最近两轮机器人评审只有 Suggestion，其未决项——tooltip 时间戳的重复推导、窗格特定的碰撞边界规则、某个 onSelect 处理函数的身份，以及 triage 指出的未测试面——按我的读法同样非阻塞。我未逐行阅读的部分：App.tsx(+16/-3，showSessionDetails 与待处理窗格上报的接线)、SessionDetailsTooltip.tsx(+11/-4)、i18n.tsx(+4)、两个 CSS module，以及约 900 行新增单元与 e2e 测试中除三项 Critical 相关区域以外的部分；对这些我依据的是本 head 上的绿色通道，包括跑在本 PR 所路由的托管 runner 上的浏览器冒烟。CI：本 head 上 15 项通过、28 项跳过，无失败、无取消、无 Pending——其中包含浏览器作业，且正跑在本 PR 为其指定的托管 runner 上。没有可归因于本 PR 的缺陷，我读到的通道中也没有任何一项构成门禁。