fix(web-shell): keep split-view session list fresh and preserve panes across view switches - #6418
Conversation
… across view switches The in-window split view's "add pane" picker read a stale session snapshot — `useSessions` only fetches on mount — so sessions created after entering the split never appeared. And switching away from the split and back cleared the panes, because the live pane set lived in local state that died on unmount while the seed it re-mounted from was never updated (and the no-arg "Open Split View" button reset it to empty). - Reload the picker list when it opens and when the parent's session-list reload token changes, so it never offers a removed session or misses a new one. - Mirror the live pane set up to the app via onPanesChange so it survives SplitView unmounting; restore it (instead of reseeding empty) when the split is reopened without an explicit selection.
|
Thanks for the PR @wenshao — re-running the gate after the follow-up commits. Template looks good ✓ — thorough description, bilingual body, reviewer test plan with Playwright-driven before/after screenshots against the real Problem: observed and well-evidenced. Both rough edges are real UX issues — the picker freezing at the entry-time snapshot and panes collapsing on view switch. The before/after screenshots in the PR body demonstrate both concretely. Direction: aligned. Split view is a core web-shell surface, and "picker shows current state" + "panes survive a view switch" are baseline expectations for any multi-pane UI. No mission drift. Size: 4 files changed, all within Approach: minimal and clean. Two optional props added to Moving on to code review. 🔍 中文说明感谢 @wenshao 的 PR —— 在后续提交后重新跑门禁。 模板完整 ✓ —— 详尽的描述、双语正文、附带基于 Playwright 驱动真实 问题： 已观察到且有充分证据。两个粗糙边缘都是真实的 UX 问题——选择器在进入时冻结、切走视图后面板塌缩。PR 正文里的 before/after 截图具体展示了两个问题。 方向： 对齐。分屏是 web-shell 的核心界面，"选择器显示当前状态"+"面板在视图切换后保留"是任何多面板 UI 的基线期望。无使命偏移。 规模： 4 个文件，全部在 方案： 最小且干净。给 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewI read the diff against an independent baseline. Before looking at the code, I'd have: (1) added a Critical blockers: none. Traced the effect loops that could have gone wrong:
Reuse / conventions: no issues. Test coverage: the follow-up commits strengthened the suite significantly. The TestingThis is a React web-shell component — it runs in a browser under Vite, not in a terminal. Tmux real-scenario testing is not applicable here. The PR body includes Playwright-driven before/after screenshots from a harness that mounts the real I was able to install dependencies and run the SplitView test suite locally: All 16 pass, including the 4 new tests that directly cover both fixes. App.test.tsx could not run in this worktree due to 中文说明代码审查我把 diff 与独立基线做了对比。看代码之前，我会：(1) 在 picker 打开时加 关键阻塞： 无。 追踪了三个可能出问题的 effect 循环：
复用/规范： 无问题。 测试覆盖： 后续提交显著加强了测试。 测试React web-shell 组件——在浏览器里由 Vite 驱动。tmux 真实场景测试不适用。 PR 包含基于 Playwright 的 before/after 截图。 本地成功运行 SplitView 测试：16 项全部通过，含 4 个新测试直接覆盖两个修复。App.test.tsx 在此 worktree 因 — Qwen Code · qwen3.7-max |
|
Stepping back to reflect before deciding. This is a focused, ~64-line production fix for two concrete UX rough edges in the split view that any user who actually works in the surface would hit within minutes. The problem is observed (not theoretical), the evidence is in the PR's before/after screenshots, and the fix matches what I'd have done independently — with the bonus of The follow-up commits show a conscientious author: the in-flight reload guard was removed with a well-reasoned explanation (sequence counter in The code is minimal: two optional props, three small effects, one preservation tweak to I ran the SplitView test suite locally — 16/16 pass, including the 4 new tests. App.test.tsx couldn't run in this worktree (environment issue), but the code review traced every effect's dependency graph and found no feedback loops, stale closures, or unnecessary re-renders. CI will be the authoritative run. Independent proposal vs. PR's approach: the PR matches or exceeds it. Approving. 中文说明决定前先退一步反思。 这是一个聚焦的约 64 行生产代码修复，针对分屏视图中两个具体的 UX 粗糙边缘——任何真正在该界面工作的用户几分钟内就会撞到。问题是观察到的（不是理论性的），证据在 before/after 截图里，修复方案与我独立想到的吻合，并额外加了 后续提交展示了认真负责的作者：in-flight reload 守卫被移除并附有充分推理（ 代码最小化：两个可选 prop、三个小 effect、一个对 本地跑了 SplitView 测试：16/16 通过，含 4 个新测试。App.test.tsx 在此 worktree 无法运行（环境问题），但代码审查追踪了每个 effect 的依赖图，未发现反馈循环、过时闭包或不必要重渲染。CI 将是权威跑测。 独立方案 vs PR 方案：PR 匹配或超过之。批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Downgraded from Request changes to Comment: self-PR; CI still running.
…e token reloads Addresses review feedback on QwenLM#6418: - SplitView: skip a token-driven reload while one is already in flight, so a burst of session-list changes (bulk create/delete) doesn't fire a redundant concurrent round-trip per bump (matches the sidebar's poll guard). - SplitView test: the freshness test now proves the picker re-renders with the refreshed list — a session appearing only after reload shows up — not just that reload() was called. - App test: cover the openSplitView preserve/restore path end-to-end — a reported pane set survives leaving the split and is restored on reopen.
|
Addressed the review in 7092e70 (tests + a small robustness tweak):
On the two [Critical] stale-session threads I've pushed back rather than changed code: a pane/picker option referencing a session removed elsewhere is a pre-existing edge (the seed could always hold a stale id), each pane is contained by a per-pane All four 中文已在 7092e70 处理评审(测试 + 一处小的健壮性改动):
两条 [Critical] stale-session 线程我选择有据反驳而非改代码:pane/picker 选项引用别处已删除的会话是既有边角(种子本来就可能含 stale id),每个 pane 由独立 四个 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Downgraded from Request changes to Comment: self-PR; CI still running.
…ht guard) The in-flight guard added in the previous commit could drop a session-list reload token that arrives while a reload is still running: the effect has already run for that token value, and clearing the in-flight flag in `finally` doesn't re-run it, so the picker could stay stale after burst create/delete/ rename activity — and the split has no polling fallback to recover. Reload on every distinct token bump instead. `useDaemonResource` serializes responses via its sequence counter (last write wins), so overlapping reloads are correct, and the token is bumped only on discrete session-change events — an occasional redundant fetch is far cheaper than a lost refresh.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
No new Suggestion-level findings this round — all prior suggestions have been addressed or superseded. — qwen3.7-max via Qwen Code /review |
…e + cap) Per review: the restore branch of openSplitView was covered but the explicit-selection branch (dedupe + MAX_SPLIT_PANES cap, replacing any prior set) was only exercised, not asserted. Add a `?split=` URL test with duplicate and over-cap ids that asserts the split seeds exactly the deduped, capped selection.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ Downgraded from Approve to Comment: self-PR; CI still running.
— qwen3.7-max via Qwen Code /review
| // Mirror the live pane set up to the parent so it outlives this component | ||
| // unmounting when the user switches views. On re-entry the parent reseeds | ||
| // `initialSessionIds` from it, restoring the exact panes instead of clearing. | ||
| useEffect(() => { |
There was a problem hiding this comment.
[Suggestion] The reporting effect includes onPanesChange directly in its dependency array. Today the parent passes setSplitSessionIds (a stable useState setter), so this works correctly. However, the contract is enforced only by a JSDoc comment — if a future refactor wraps the callback in an inline arrow (e.g. onPanesChange={(ids) => setSplitSessionIds(ids)}), the effect fires every render, each call triggers a parent state update, and the new callback identity re-fires the effect, producing an infinite render loop.
Stabilize the callback inside SplitView with a ref so the component is safe regardless of what the parent passes:
const onPanesChangeRef = useRef(onPanesChange);
onPanesChangeRef.current = onPanesChange;
useEffect(() => {
onPanesChangeRef.current?.(paneIds);
}, [paneIds]);— qwen3.7-max via Qwen Code /review
| ) ?? false; | ||
| const { sessions } = useSessions({ | ||
| const { sessions, reload } = useSessions({ | ||
| autoLoad: true, |
There was a problem hiding this comment.
[Suggestion] useSessions returns an error field (from useDaemonResource's state) that is silently discarded here. When a reload fails (daemon timeout, network issue), the picker shows stale data with no console warning or UI signal. Unlike the sidebar (which has polling as a fallback), SplitView has no fallback — stale data persists until the user remounts.
Consider destructuring error and logging it:
const { sessions, reload, error } = useSessions({ ... });
useEffect(() => { if (error) console.warn('[SplitView] reload error:', error); }, [error]);— qwen3.7-max via Qwen Code /review
| // Open the in-window split view showing 2+ sessions side by side. `splitSessionIds` | ||
| // is the live pane set — SplitView mirrors add/remove back into it via | ||
| // onPanesChange — so it must be preserved across entries, not blindly reset. | ||
| const openSplitView = useCallback( |
There was a problem hiding this comment.
[Suggestion] openSplitView includes connection.sessionId in its useCallback deps, giving it a new identity on every reconnection. The ?split= URL effect depends on openSplitView, so it re-runs on each reconnect. Safe today (the URL param is already consumed), but the coupling is fragile — any future side effect added to that effect would fire far more often than expected.
Use a ref to keep the callback identity stable:
const sessionIdRef = useRef(connection.sessionId);
sessionIdRef.current = connection.sessionId;
const openSplitView = useCallback((sessionIds?: string[]) => {
setActivePanel(null);
setSplitSessionIds((prev) => {
const requested = Array.from(new Set((sessionIds ?? []).filter(Boolean))).slice(0, MAX_SPLIT_PANES);
if (requested.length > 0) return requested;
if (prev.length > 0) return prev;
return sessionIdRef.current ? [sessionIdRef.current] : [];
});
setMainView('split');
}, []); // stable identity— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: self-PR.
— GPT-5 Codex via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review pass on current head 70a203c1d548bb1fe36e07380d68dd6ee991bd81: no critical correctness, security, data-loss, build-breaking, or severe regression issues found. The latest Qwen Code CI workflow run is also green.
— GPT-5 Codex via Qwen Code /review
3 participants
What this PR does
Fixes two rough edges in the in-window split view (the side-by-side multi-session surface reached from the Session Overview or the "Open Split View" toolbar button): the "Add session" picker now stays fresh, and the split now survives leaving it and coming back.
Why it's needed
useSessionsonly fetches on mount. The split view read that snapshot once and never refreshed, so a session created after you entered the split never appeared in the "Add session" picker, and a session deleted elsewhere was still offered. Meanwhile the live pane set lived inSplitView's local state, which is destroyed when the split unmounts (it is conditionally rendered onmainView === 'split'). The seed it re-mounted from was never updated with panes you added, and the toolbar "Open Split View" button reset that seed to empty — so switching away from the split and back collapsed it down to just the current session. Both are things a user hits quickly once they actually work in the split for a while.Reviewer Test Plan
How to verify
Automated coverage (run in
packages/web-shell):npx vitest run client/components/SplitView.test.tsx client/App.test.tsx— 54 pass, including three new SplitView tests (reloads the session list when the picker opens,reloads the picker list when the parent bumps the reload token,mirrors the live pane set up to the parent as panes change). The four split-related suites (SplitView, App, SessionOverviewPanel, WebShellSidebar) total 110 passing.The screenshots below are from a small harness that mounts the real
SplitView(real CSS module + i18n) driven in real Chrome via Playwright, with only the chat-pane body and the daemon session list stubbed — so it exercises the exact component and picker/pane wiring this PR changes. The "before" image is the same harness with the pre-fix app wiring (?mode=old).Evidence (Before & After)
The split view — each pane is an independent session (own transcript, approvals, streaming):
Fix 1 — "Add session" picker stays fresh. A session is created elsewhere; reopening the picker reloads and now offers it (before the fix the list was frozen at entry and it would be missing):
Fix 2 — panes survive a view switch. Switch away from the split and reopen it:
Tested on
Environment (optional)
Unit tests + a Vite/Playwright screenshot harness (real Chrome,
channel:'chrome'). Not run against a live daemon on Windows/Linux — covered by CI.Risk & Scope
SplitViewnow reports its pane set up viaonPanesChange. To avoid a render loop the parent must pass a referentially stable setter (it passessetSplitSessionIdsdirectly); this is documented on the prop and covered by the tests.onPanesChangeandsessionListReloadTokenare optional props; existing behavior is unchanged when they are omitted.Linked Issues
N/A — found during manual testing of the split view.
中文说明
这个 PR 做了什么
修复窗口内分屏视图（从「会话总览」或工具栏「打开分屏」按钮进入的多会话并排界面）的两个问题：「添加会话」选择器现在保持最新，且分屏在切走再切回后不再丢失。
为什么需要
useSessions只在挂载时抓取一次。分屏读到这份快照后再不刷新，所以进入分屏后新建的会话不会出现在「添加会话」选择器里，别处删除的会话却仍被列出。同时，实时的面板集合存在SplitView的本地 state 中，而分屏是按mainView === 'split'条件渲染的，切走即卸载、state 丢失；它重新挂载所依据的种子既不反映用户新加的面板，工具栏「打开分屏」按钮还会把该种子重置为空——于是切走再切回时分屏塌缩到只剩当前会话。用户真正在分屏里工作一会儿后很容易撞到这两点。审阅者验证步骤
如何验证
自动化覆盖（在
packages/web-shell下运行）：npx vitest run client/components/SplitView.test.tsx client/App.test.tsx——54 项通过，含 3 个新增 SplitView 用例（选择器打开时 reload、父级 reload token 变化时 reload、面板增减上报给父级）。四个分屏相关套件（SplitView、App、SessionOverviewPanel、WebShellSidebar）共 110 项通过。下方截图来自一个小型 harness：它挂载真实的
SplitView（真实 CSS module + i18n），用 Playwright 在真实 Chrome 中驱动，仅将聊天面板正文与 daemon 会话列表打桩——因此运行的正是本 PR 改动的组件与选择器/面板接线逻辑。「修复前」那张是同一 harness 套用修复前的应用接线（?mode=old）。证据（前后对比）
分屏视图——每个面板都是独立会话（各自的 transcript、审批、流式输出）：
修复 1 —— 「添加会话」选择器保持最新。 别处新建了一个会话，重新打开选择器时会 reload 并把它列出（修复前列表冻结在进入时，会缺这一项）：
修复 2 —— 面板在切换视图后仍在。 切走再重新打开分屏：
风险与范围
SplitView现在通过onPanesChange上报面板集合。为避免渲染循环，父级必须传入引用稳定的 setter（现直接传setSplitSessionIds）；该约束已写在 prop 注释里并有测试覆盖。onPanesChange与sessionListReloadToken均为可选 prop，省略时行为不变。