fix(web-shell): keep sidebar sessions synchronized - #9533
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: this one checks out against the code, not just the description. Today Direction: aligned. Sidebar responsiveness is core Web Shell surface, and the change stays inside the session-catalog layer — no auth/sandbox/model-selection/telemetry/release or public-contract impact. Size: no core-module paths touched ( Approach: the scope feels right — threading the existing Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这个问题在代码层面可以确认，而不只是描述成立。目前 方向：对齐。侧边栏响应速度是 Web Shell 的核心界面，改动限制在 session-catalog 层内——不涉及 auth/sandbox/模型选择/telemetry/release 或公共契约。 规模：未触及核心模块路径（仅 方案：范围合理——把已有的 风险：无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewBefore reading the diff, my independent take from the description was: reuse the store's existing Traced the concurrency paths and found no blockers:
Non-blocking observations:
The added tests pin the behavior in both directions: the bypass tests fail if the bypass is removed (no new sequenceDiagram
participant P1 as WebShellSidebar
participant P2 as catalog controller
participant P3 as SessionCatalogStore
participant P4 as live-state poll loop
P1->>P2: refreshWorkspace(cwd) after mutation
P2->>P3: invalidateWorkspace(cwd) with interactive flag
P3->>P3: mark entries stale and bump revisions
P3->>P4: wake(cwd) with bypassRetry
alt no request in flight
P4->>P4: poll bypasses the 30s error backoff
P4->>P3: stage and commit fresh pages
else a request is already in flight
P4->>P4: set interactiveRefreshRequested
P4->>P4: finish() re-polls once the flight settles
P4->>P3: stage and commit fresh pages
end
Files changed (10)
Test evidence (PR's own CI, read via API — PR code never executed locally)At review time the two lanes that matter for this PR are still running, and there are no red checks. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 On sandboxed verification: no extra lane is named here, and this is a deliberate skip, not an omission. The central behavioral claim (explicit refreshes bypass backoff, background refreshes do not, in-flight refreshes are preserved) is pinned by the added unit tests in both directions as analyzed above, so a green suite is meaningful evidence rather than a green stamp. The only claim the unit suite cannot render is the visual spacing tweak — the oracle for that is the 中文说明代码审查在读 diff 之前，我根据描述给出的独立方案是：复用 store 已有的 并发路径逐条检查，无阻塞问题：
非阻塞观察：
新增测试双向锁定行为：绕过测试在移除绕过逻辑时会失败（退避期间不会再有新的 时序图展示了变更后的流程：侧边栏变更 → controller → store 标记过期并唤醒轮询循环；无在途请求时直接绕过退避执行 reconcile，有在途请求时先记录标志、在本次请求落地后由 finish() 补一次轮询。 测试证据（PR 自身的 CI，通过 API 读取——未在本地执行 PR 代码）审查时对此 PR 最关键的两条流水线仍在运行，且没有红色检查。 关于沙箱验证：这里没有点名额外的验证通道，这是刻意的跳过而非遗漏。核心行为主张（显式刷新绕过退避、后台刷新不绕过、在途刷新被保留）已被新增单测双向锁定（分析见上），因此绿色的测试套件是有意义的证据，而不是橡皮图章。单测无法呈现的唯一主张是视觉间距调整——其 oracle 是已在该提交上运行的 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a focused, architecturally consistent fix whose tests pin the behavior in both directions; only non-blocking nits (a bundled CSS tweak and deliberately sidebar-scoped coverage). Stepping back: the problem is real — I verified the staleness window in the base code rather than taking the PR's word for it, and the fix is the smallest natural extension of the machinery already there. Nothing new was invented: the Reservations, all minor: the rename path double-touches the store, the spacing CSS is a drive-by for this title, and mutation surfaces outside the sidebar still carry the same staleness window until someone extends the treatment. None of that blocks a merge; the first two are cleanliness notes and the third is acknowledged scope. I am approving this because the mechanism is sound and pinned by tests, not because I ran out of objections. The unit suite and the web-shell visuals job are still in flight on the reviewed commit, so approval is deferred until CI lands green on that commit; the finalize pass will confirm it, and if anything lands red it will flag it instead. 中文说明回顾整体：问题是真实存在的——我在基础代码里亲自确认了过期窗口，而不是听信 PR 的描述；修复是对现有机制最小且自然的扩展。没有发明任何新东西： 保留意见，均为次要：重命名路径会两次触碰 store，间距 CSS 相对标题是顺手改动，侧边栏之外的变更入口仍带有同样的过期窗口、有待后续扩展。这些都不阻塞合并；前两点是整洁性备注，第三点是已声明的范围。 我批准这个 PR 是因为机制可靠且被测试锁定，而不是因为没有反对意见了。在审查的提交上，单元测试和 web-shell 视觉任务仍在运行，因此批准推迟到 CI 在该提交上变绿之后；finalize 流程会予以确认，若有检查变红则会标出。 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
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 — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| const reload = useCallback( | ||
| async (reloadOptions: { interactive?: boolean } = {}) => { | ||
| try { | ||
| return (await reloadPage(reloadOptions))?.sessions; |
There was a problem hiding this comment.
[Suggestion] The { interactive: true } option the sidebar retry buttons forward here is never tested through either of the two hook hops — session-catalog-hooks.test.tsx only calls facade.reload() with no argument, and no sidebar test clicks the loadFailed retry button (its mocked reload ignores arguments), so only the store end (store.refresh(query, { interactive: true })) is covered. If a future refactor drops reloadOptions at either hop (e.g. reloadPage(reloadOptions) → reloadPage()), the retry button's backoff bypass silently disappears — the user clicks Retry while a backoff window is active and nothing happens until it expires — while the whole suite stays green. That is precisely the user-visible fix this PR adds. Verified with a mutation probe: after dropping options in useSessionCatalogQuery.reload, the full web-shell suite still passes (188 files / 3805 tests). A hooks-level test would pin this: render useWebShellSessions with a live-state-retained workspace, call reload({ interactive: true }), and assert the store's refresh received { interactive: true } (or a wake fired with bypassRetry: true).
中文说明
侧边栏重试按钮在这里传入的 { interactive: true } 选项，在两个 hook 层都没有任何测试覆盖——session-catalog-hooks.test.tsx 只调用无参的 facade.reload()，侧边栏测试也没有点击 loadFailed 重试按钮（mock 的 reload 会忽略参数），因此只有 store 端（store.refresh(query, { interactive: true })）被覆盖。未来重构时如果在任一层丢掉 reloadOptions（例如 reloadPage(reloadOptions) → reloadPage()），重试按钮绕过退避的能力会悄悄丢失——用户在退避窗口内点击重试却没有任何反应，直到窗口过期——而整个测试套件依然全绿。这正是本 PR 新增的用户可见修复。已通过变异探针验证：在 useSessionCatalogQuery.reload 中丢掉 options 后，web-shell 完整测试套件仍然全部通过（188 个文件 / 3805 个测试）。建议补一个 hooks 层测试来锁定：在保留了 live-state 的工作区上渲染 useWebShellSessions，调用 reload({ interactive: true })，断言 store 的 refresh 收到了 { interactive: true }（或 wake 以 bypassRetry: true 触发）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| const invalidate = useCallback(() => { | ||
| if (workspaceCwd) controller.invalidateWorkspace(workspaceCwd); | ||
| if (workspaceCwd) controller.refreshWorkspace(workspaceCwd); | ||
| }, [controller, workspaceCwd]); |
There was a problem hiding this comment.
[Suggestion] This upgrade is not pinned by any test: the only facade mutation test ('runs mutations directly and performs only the Store resynchronization') runs without retainWorkspaceLiveState, and with live-state disabled invalidateWorkspace and refreshWorkspace are behaviorally identical (interactive only changes requestLiveStateRefresh, which is skipped). A mutation probe reverting this line to controller.invalidateWorkspace(workspaceCwd) keeps the entire web-shell suite green (188 files / 3805 tests) — the facade-driven delete/archive/release actions could silently lose this immediate backoff-bypassing refresh (stale sidebar until the next poll tick during an error backoff) without any test failing. Extending the hooks test to retain live-state for the workspace, perform a facade mutation, and assert the refresh took the interactive path (store.invalidateWorkspace called with { interactive: true }, or a wake handler observing bypassRetry: true) would pin the change.
中文说明
这个升级没有被任何测试锁定：唯一的 facade 变更测试（'runs mutations directly and performs only the Store resynchronization'）运行时没有 retainWorkspaceLiveState，而在 live-state 未启用时，invalidateWorkspace 与 refreshWorkspace 的行为完全一致（interactive 只影响被跳过的 requestLiveStateRefresh）。变异探针显示：把这行还原为 controller.invalidateWorkspace(workspaceCwd) 后，web-shell 完整测试套件依然全绿（188 个文件 / 3805 个测试）——也就是说，facade 驱动的删除/归档/释放操作可能会悄悄失去这个立即绕过退避的刷新（错误退避期间侧边栏会一直过期到下一次轮询），而不会有任何测试失败。建议扩展 hooks 测试：保留工作区的 live-state，执行一次 facade 变更，并断言刷新走了 interactive 路径（store.invalidateWorkspace 被以 { interactive: true } 调用，或 wake 处理器观察到 bypassRetry: true），从而锁定这一改动。
— qwen3.8-max via Qwen Code /review (v0.21.14)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1 retry-button { interactive: true } wiring untested — already reported (comment 3818624636)
- R1-2 facade invalidate → refreshWorkspace rerouting untested — already reported (comment 3818624641)
中文说明
已审查——无阻断问题。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review — PR #9533 fix(web-shell): keep sidebar sessions synchronized
HEAD: dcb34ca1 | +300/−55, 10 files
No blocking findings. Mechanism traced end-to-end; tests pass on CI.
What was verified
Core bypass mechanism (workspace-session-live-state.ts)
refreshWorkspace→invalidateWorkspace({interactive:true})→requestLiveStateRefresh(kind=interactive, bypassRetry=true)→ wake handler →state.interactiveRefreshRequested = true→poll()bypassesliveRetryAtcheck. Chain is complete and correct.finish()closure correctly re-triggerspoll()if a new interactive refresh arrives while a request is in-flight, so the bypass is not lost during in-flight waits.bypassReconcileRetry = bypassRetry || state.interactiveRefreshRequested: if a second interactive request arrives after the live-state request but before reconcile, the reconcile still bypasses the retry window. Correct.requestLiveStateRefreshguard (kind !== interactive && existing === interactive): prevents a non-interactive call from downgrading a pending interactive request. Correct.- Background
invalidateWorkspace()still useskind=invalidatedwith no wake, so background refreshes remain rate-limited. Correct.
9 invalidateWorkspace → refreshWorkspace substitutions in WebShellSidebar.tsx
All are in .finally() blocks or explicit mutation-success positions. Checked the rename flow (new call at line 2495 on success + existing finally call at line 2511 guarded by !renamed): mutually exclusive, no double-fire.
New test cases (workspace-session-live-state.test.tsx)
6 new tests cover: bypass live-state backoff, background loads stay rate-limited, preserve refresh while live request is in-flight, bypass reconciliation backoff, background loads stay behind reconciliation backoff, preserve refresh during in-flight catalog request. All key permutations covered at the store/live-state layer.
CI: Ubuntu Test (18m30s) ✅ · E2E Smoke ✅ · Visual capture ✅. Windows and macOS test suites are SKIPPED — no concern for this change (no platform-specific paths).
Confirming qwen-code-ci-bot suggestions (no new threads)
Both inline suggestions are valid test-validity gaps (class 5):
hooks.ts:318—reload({ interactive: true })forwarding across two hook hops (useWebShellSessions.reload→reloadPage→store.refresh) is not tested at the hook layer; only the store end is covered. A refactor droppingreloadOptionsat either hop would keep the suite green.hooks.ts:327—invalidate → controller.refreshWorkspacechange is not pinned by any test withretainWorkspaceLiveState: true; with live-state disabled the two are behaviorally identical, so a mutation probe reverting this line passes the entire suite.
Neither is a blocker; the store-level test coverage for the bypass behavior is thorough. Actionable as follow-up tests.
Reviewed with AI assistance.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
LGTM (AI Generated)
核心机制已在 COMMENT 轮完整验证（见 #pullrequestreview-4979366458），此处补充 Approve。
CI: ubuntu Test ✅ · E2E Smoke ✅ · Visual capture ✅ · Desktop Shell (ubuntu + windows) ✅
两条 qwen-code-ci-bot Suggestion（hook 层转发未测试 / invalidate→refreshWorkspace 未被 live-state 启用的测试覆盖）均为非阻塞，可 follow-up 补测。
✅ Approve
|
Released in v0.21.15. |
Resolve the WorkspaceSection conflict by keeping the optimistic-pin mapSession seam ahead of the filter and adopting main's git-query search term (sessionMatchesGitQuery) in the same predicate. Also extend the pinning test harness controller mock with refreshWorkspace, added by main's #9533 sidebar-sync rework, so the settle path's workspace refresh no longer rejects in tests. Verified: web-shell typecheck clean; sidebar suite 13 files, 188/188.
4 participants
What this PR does
This change makes Web Shell sidebar mutations reconcile the affected workspace immediately after creating, renaming, deleting, archiving, pinning, grouping, or recoloring sessions. User-triggered retries can bypass transient catalog backoff, while background refreshes continue to respect error backoff and coalesce repeated work. It also adds spacing around the session loading indicator and right-side padding when no status icon is present.
Why it's needed
Sidebar state could remain stale when another client changed the same workspace or when a mutation updated data outside the currently cached page. Treating every internal refresh as a manual retry also caused avoidable request bursts during daemon failures. The new distinction keeps explicit user actions responsive without weakening background rate limiting.
Reviewer Test Plan
How to verify
Perform sidebar operations such as rename, pin or unpin, archive or unarchive, delete, group assignment, and color changes, then confirm the affected workspace is fully refreshed. Simulate live-state and catalog failures and confirm background refreshes wait for the retry window while clicking a visible Retry control triggers an immediate attempt. Trigger a refresh while a live-state request is already in flight and confirm the refresh is neither lost nor duplicated. Verify loading rows and rows without right-side icons retain comfortable right spacing.
Evidence (Before & After)
Before: mutation results could remain stale until a later poll, internal refreshes could bypass failure backoff, and session metadata could sit too close to the loading indicator or right edge.
After: sidebar mutations and explicit retries reconcile immediately, background refreshes remain rate-limited during failures, and the session metadata spacing is consistent.
Tested on
Environment (optional)
Local workspace with the Web Shell Vitest suite, repository lint, typecheck, and build.
Risk & Scope
Linked Issues
N/A
中文说明
此 PR 做了什么
此改动让 Web Shell 侧边栏在新建、重命名、删除、归档、置顶、分组或修改会话颜色后立即对受影响的工作区执行完整同步。用户主动点击重试时可以绕过临时的目录退避，而后台刷新仍会遵守错误退避并合并重复工作。同时调整会话 loading 图标周围的间距，并在右侧没有状态图标时增加留白。
为什么需要
当其他客户端修改同一工作区，或一次操作更新了当前缓存分页之外的数据时，侧边栏状态可能一直过期到后续轮询。此前把所有内部刷新都视为手动重试，也会在 daemon 故障时产生不必要的请求突发。新的区分让用户操作保持即时响应，同时不削弱后台限流。
Reviewer Test Plan
如何验证
执行重命名、置顶或取消置顶、归档或取消归档、删除、分组和颜色修改等侧边栏操作，确认受影响的工作区会被完整刷新。模拟 live-state 和目录请求失败，确认后台刷新会等待重试窗口，而点击可见的 Retry 控件会立即发起请求。在 live-state 请求进行中触发刷新，确认该刷新既不会丢失也不会重复执行。确认 loading 行以及右侧没有图标的行都保留合适的右侧间距。
证据（Before & After）
Before：操作结果可能在后续轮询前保持过期，内部刷新可能绕过故障退避，会话内容也可能靠 loading 图标或右边缘过近。
After：侧边栏操作和显式重试会立即同步，后台刷新在故障期间仍受限流保护，会话内容间距保持一致。
测试平台
环境（可选）
本地工作区，运行 Web Shell Vitest 完整测试、仓库 lint、typecheck 和 build。
风险与范围
关联 Issue
N/A