feat(web-shell): persist the split view across refresh, per tab - #7136
Merged
Conversation
The split view (2+ sessions side by side) was lost on every refresh: its pane set lived only in React state, and the one-shot ?split= deep link is consumed on load. Persist the live session set to sessionStorage while the split is the active view, and restore it on load when no ?split= deep link is present, so a refresh brings the split back. sessionStorage (not localStorage) is deliberate: it is scoped per browser tab, so a split opened in its own tab and the in-window split never clobber each other, and a fresh unrelated tab restores nothing — while still surviving a refresh of the same tab. The ?split= URL stays the shareable, cross-tab channel. Only an explicit close (the split's back button) clears the persisted set; detours to a single session keep it, so the split is treated as the user's lasting context until they close it. Controlled hosts (which own their split lifecycle) never auto-persist or auto-restore. Co-authored-by: wenshao <wenshao@example.com>
Collaborator
|
Re-run after the author addressed a Critical from Template: PR body follows Problem: real and clearly demonstrated. The split view pane set lived only in React state, so a refresh dropped the user back to a single session. The PR includes before/after screenshots showing the split surviving a reload, per-tab isolation, and explicit-close clearing storage. Direction: aligned. Persisting UI state across refreshes is basic UX hygiene, and Size: not applicable — no core module paths touched. All changes are in Approach: scope feels right. ~60 production lines (3 utility functions + 2 effects + 1 line in Re: the [Critical] from the earlier Moving on to code review. 🔍 中文说明Re-run：作者回应了 模板：PR body 正确遵循了 问题：真实且演示清晰。分屏视图的 pane 集合仅存在于 React 状态中，刷新即丢失。PR 提供了 before/after 截图，展示了刷新恢复、per-tab 隔离、以及主动关闭后清除存储。 方向：对齐。跨刷新持久化 UI 状态是基本 UX， 规模：不适用——未触及核心模块路径，所有改动在 方案：范围合理。约 60 行生产代码，约 250 行测试。 关于 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Code review: the implementation matches what I'd propose independently — three sessionStorage utilities ( No critical blockers. No AGENTS.md violations. Re: the [Critical] from the earlier Unit tests ( Component tests ( The three new test cases verify: restore a persisted split on load, no-op when nothing persisted, and clear storage when the user leaves the split view. All pass. Dev server (tmux — Vite starts cleanly): E2E / browser testing: the split persistence feature is browser-only (sessionStorage, page reload, per-tab isolation). tmux cannot drive browser-level interactions. The PR includes Playwright E2E spec ( 中文说明代码审查： 实现方案与独立提案一致——在 无阻塞问题。无 AGENTS.md 违规。 关于 单元测试（ 组件测试（ 开发服务器（tmux — Vite 正常启动）：见上方输出。 E2E / 浏览器测试： 分屏持久化功能是浏览器端的（sessionStorage、页面刷新、按标签隔离）。tmux 无法驱动浏览器级别的交互。PR 包含 Playwright E2E spec 覆盖了所有三个场景，作者提供了真实 Playwright 运行的 before/after 截图。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after
Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Collaborator
|
Confidence: 5/5 — clean, focused implementation of a real UX fix; all tests pass independently; the Critical from This is a good PR. The problem is real (split view lost on every refresh), the solution is minimal (~60 production lines doing exactly what's needed), and the test coverage is thorough across three levels (unit, component, E2E spec). The My independent proposal matched the PR's approach exactly. No simpler path exists — sessionStorage with save-on-change and load-on-init is the minimum viable persistence for this use case. All tests pass independently: 15/15 unit tests ( The [Critical] from the earlier If I had to touch this in six months, I'd thank the author: three small utility functions with clear names, two effects with concise comments, and defensive error handling that doesn't over-engineer. 中文说明这是一个好的 PR。问题真实存在（分屏视图每次刷新都丢失），方案精简（约 60 行生产代码），测试覆盖充分。独立提案与 PR 方案完全一致。所有测试独立通过：15/15 单元测试，114/114 组件测试（含 3 个新增分屏测试）。开发服务器正常启动。
— Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
approved these changes
Jul 17, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
requested changes
Jul 17, 2026
Comment on lines
+2320
to
+2325
| useEffect(() => { | ||
| if (externalSplitControlled) return; | ||
| if (mainView === 'split' && splitSessionIds.length > 0) { | ||
| saveSplitSessions(splitSessionIds); | ||
| } | ||
| }, [mainView, splitSessionIds, externalSplitControlled]); |
Collaborator
There was a problem hiding this comment.
[Critical] Navigation away from the split view via sidebar session click, new-session creation, or approval-notice button calls setMainView('chat') directly — clearSplitSessions() is never invoked. Storage retains ["A","B"], so a page refresh restores the split the user intentionally navigated away from.
Failure scenario: user in split with sessions A,B → clicks session C in sidebar → setMainView('chat') at line ~6081 bypasses handleSplitExit → refresh → loadSplitSessions() returns ["A","B"] → user is thrown back into the abandoned split.
Multiple setMainView('chat') call sites (~10) share this gap. Consider extending the mirror effect to clear storage when mainView transitions away from 'split' outside a shrink-fold, or routing all intentional departures through handleSplitExit.
Additionally, no App-level unit test asserts that this save effect actually writes to sessionStorage — the write path is only covered by the E2E Playwright spec.
Suggested change
| useEffect(() => { | |
| if (externalSplitControlled) return; | |
| if (mainView === 'split' && splitSessionIds.length > 0) { | |
| saveSplitSessions(splitSessionIds); | |
| } | |
| }, [mainView, splitSessionIds, externalSplitControlled]); | |
| useEffect(() => { | |
| if (externalSplitControlled) return; | |
| if (mainView === 'split' && splitSessionIds.length > 0) { | |
| saveSplitSessions(splitSessionIds); | |
| } else if (mainView !== 'split' && !splitFoldedByShrinkRef.current) { | |
| clearSplitSessions(); | |
| } | |
| }, [mainView, splitSessionIds, externalSplitControlled]); |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
Maintainer Verification ReportTest Results
Screenshots (Local E2E Run)1. Split view with 2 panes (via
2. After reload — split restored from sessionStorage (URL is bare
3. New tab — no split inherited (sessionStorage per-tab isolation):
Verification Summary
Environment
中文验证报告测试结果
截图（本地 E2E 运行）1. 双窗格分屏视图（通过
2. 刷新后 — 分屏从 sessionStorage 恢复（URL 为裸
3. 新标签页 — 未继承分屏（sessionStorage 按标签隔离）：
验证总结
环境
|
ytahdn
reviewed
Jul 18, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Unresolved, please confirm: [Critical] App.tsx:2325 (existing blocker re-check): multiple setMainView('chat') call sites bypass handleSplitExit/clearSplitSessions(). Mechanism still operates, but PR author explicitly designed this behavior ('detours to a single session keep the persisted set'). Product design disagreement — cannot determine whether this is a defect or intended behavior. Not reviewed: coverage — the plan could not be used (ENOENT: no such file or directory, open '.qwen/tmp/qwen-review-pr-7136-fetch.json'), so this run cannot show that any of the diff was read. Not reviewed: verification — could not check that Step 4 and Step 5 ran (ENOENT: no such file or directory, open '.qwen/tmp/qwen-review-pr-7136-fetch.json').
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
Re: review 4728077699 — the Confirmed intentional — this is a design choice, not a defect. The split is treated as the user's lasting context until they explicitly close it via the back button ( The alternative (clear on every split→chat transition) was considered and rejected:
This is documented in the PR body ("Only an explicit close clears the persisted set; detours keep it") and the design doc. No data loss or wrong-session risk — the persisted set always matches the last split the user had open. If the product direction shifts to "any navigation away clears persistence," the change is ~4 lines (track |
ytahdn
approved these changes
Jul 18, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Re: the existing [Critical] at App.tsx:2325 — the setMainView('chat') detour flagged by the earlier review: the author has explicitly confirmed this is intentional design, not a defect. The split is treated as the user's lasting context until an explicit close via the back button; transient detours (sidebar click, "Go to approval") are expected to leave the persisted set intact so a refresh restores the user's split working set. This mirrors how many apps restore the last major workspace on refresh.
A full 12-dimension review of the diff (line-by-line correctness, removed-behavior audit, cross-file tracer, security, code quality, performance, test coverage, three adversarial personas, and build & test) confirmed the implementation is correct and the tests are comprehensive — zero defects in the new code. Build + tests pass.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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
3 participants
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.
What this PR does
Persist the split view's live session set to
sessionStoragewhile the split is the active main view, and restore it on load when no?split=deep link is present. Only an explicit close (the split's back button) clears the persisted set; detours to a single session keep it, so the split is treated as the user's lasting context until they close it. Controlled hosts (which own their split lifecycle) never auto-persist or auto-restore. The?split=URL stays the shareable, cross-tab channel.Why it's needed
The split view (2+ sessions side by side) was lost on every refresh: its pane set lived only in React state, and the one-shot
?split=deep link is consumed on load. This made the feature fragile — a stray reload dropped the user back to a single session.localStoragewas considered but rejected because it is shared across all tabs of the same origin, which would clobber a split opened in its own tab (via the overview's "open in new tab") with the in-window split, and would auto-restore into unrelated fresh tabs.sessionStorageis per-tab and survives a refresh of the same tab — exactly the semantic we want.Reviewer Test Plan
How to verify
?split=a,bdeep link).Evidence (Before & After)
Real run against a live daemon (qwen-latest-series-invite-beta-v92 via IdealLab). Two sessions each asked to introduce a city in one Chinese sentence.
Split view with 2 panes:
After reload — split restored from sessionStorage (URL is bare
/):New tab — no split inherited (sessionStorage isolation; localStorage would wrongly reopen it):
Tested on
Environment
npm run devinpackages/web-shell, real daemon viaqwen serve --port 4170with default modelqwen-latest-series-invite-beta-v92(IdealLab). Playwright chromium drove the browser for screenshots.Risk & Scope
?split=deep link and the controlled-host API are unchanged.Linked Issues
N/A
中文说明
本 PR 在分屏作为主视图时，把当前会话集合写入
sessionStorage；加载时若无?split=深链则从中恢复。只有用户显式点分屏的返回键才会清除存储——临时切到单会话不清除，分屏被视为用户的持续上下文。受控宿主（自行管理分屏生命周期的嵌入方）不自动持久化也不自动恢复。?split=URL 继续作为可分享、跨标签的通道。此前分屏每次刷新都会丢失：窗格集合只存在 React 状态里，
?split=深链在加载时被消费一次。localStorage曾被考虑但被否决——它同源所有标签页共享，会把"在新标签打开"的分屏和窗口内分屏互相覆盖，还会把无关的新标签页误恢复成上次分屏。sessionStorage按标签隔离且同标签刷新不丢，正好契合需求。垂直分割和拖拽 resize 曾被评估后主动砍掉：聊天内容天然是"高"的，切高度对深读长对话是负优化；单会话最大化已覆盖"专注一个窗格"的需求。
Co-authored-by: wenshao wenshao@example.com