fix(web-shell): write session pin/archive changes through to cached l… - #9598
fix(web-shell): write session pin/archive changes through to cached l…#9598cactuser-Lu wants to merge 3 commits into
Conversation
|
Thanks for the PR! Gate check passes — moving on to code review. 🔍
中文说明感谢贡献！准入门检查通过，进入代码审查。🔍
— Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe approach is the right one, and it's executed carefully: write the daemon-confirmed state straight into every cached page that holds the session, keep a targeted invalidation as the reconciliation path, and stop disturbing other workspaces. I verified the load-bearing contracts against the base code, not just the diff:
No correctness blockers found in the code itself. The substantive concern is the branch state, below. The branch is stale and conflicts with
Minor, non-blocking notes: The write-through → reconcile loop, for orientation: sequenceDiagram
participant U as User
participant SB as Sidebar handler
participant D as Daemon RPC
participant ST as SessionCatalogStore
participant V as Sidebar render
U->>SB: click Pin or Archive
SB->>D: organization or archive RPC
D-->>SB: confirmed new state
SB->>ST: write-through into cached pages
ST-->>V: snapshot update, row moves at once
SB->>ST: targeted invalidation, affected lists only
ST->>D: background refetch of those lists
D-->>ST: authoritative page replaces the edit
Files changed (5)
Testing evidenceThis commit has no
Sandboxed verification would settle the behavioural claim once CI lands: 中文说明代码审查：方向正确、实现细致——守护进程确认后立即把新状态写入所有持有该会话的缓存页，用定向失效做对账，不再打扰其他工作区。关键契约都已对照基础代码核实：not-found 报错符合 webui actions 的真实返回值语义；守护进程在取消置顶时清除 pinnedAt、置顶时保留首次时间戳，"按置顶时间最早在前"正是 #9465 要的排序；web-shell 目前没有其他地方读 pinnedAt，陈旧时间戳在刷新窗口内不可见。代码本身没有发现正确性阻塞。 主要问题在分支状态：分支落后于 main，而 main 恰好重构了同一片代码（controller 新增交互式优先级的 refreshWorkspace、store 的 invalidateWorkspace 增加 interactive 选项）。冲突不只是文本层面——rebase 时作者需要有意识地决定：对账刷新保留交互优先级，还是写透已覆盖可见状态、后台优先级即可，并在 PR 描述里说明。另有两条非阻塞提醒：workspaceScopedReloadTokens 在 workspace 移除时不清理（实际影响可忽略）；分组/颜色操作仍是旧的全局刷新，超出本 issue 范围，可作后续跟进。 测试证据：该提交尚无任何 pull_request CI 运行——这是 fork 作者的首次推送，单测/typecheck/lint 工作流还未被批准运行，因此目前没有独立测试证据；PR 描述中"167 个单测通过"是作者自述，不能作为证据引用。CI 落地后，维护者可通过 @qwen-code /verify 赞助运行验证写透路径是否真正承重；"亚 100ms、无重复行"的视觉效果属于浏览器行为，两个沙箱通道都无法覆盖，需要维护者实际查看侧边栏。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean code review with no correctness blockers, but the branch conflicts with Stepping back: this is a good contribution. My independent proposal for #9465 — write the daemon-confirmed state through to the cached pages, sort the pinned section by pin time, scope the reconciliation refetch per workspace — is essentially what this PR does, and the implementation carries the details I'd want: dedupe guards, preserved cursors, deterministic tie-breaks, and tests aimed at each new path rather than at the suite's comfort zone. The two ride-along correctness fixes (not-found surfacing, partial-application reconcile) are genuine, verified against the daemon and webui action contracts, not scope creep. If this had landed on the main it was branched from, it would be close to approvable. It didn't land there, though. On evidence: the commit is minutes old and has no ⏸️ Deferring to @ytahdn — no approval from this pass. What this needs: (1) approve the fork's CI workflows so the commit gets real test evidence, (2) after the author rebases, sanity-check the refetch-priority decision and give the pin/unpin/archive flows one manual look in the web shell. Happy to re-run on the rebased branch with 中文说明总体判断：代码审查干净、没有正确性阻塞，但分支与 main 冲突的位置恰好是它重写的代码，且该提交尚无任何 CI 测试证据——需要先 rebase、再经人工确认才能合入。 这是一份高质量的贡献：我针对 #9465 的独立方案（写透守护进程确认后的状态、置顶区按置顶时间排序、按工作区限定对账刷新）与本 PR 基本一致，实现细节也到位——去重保护、游标保留、确定性排序兜底，测试都瞄准新路径。两个顺带的正确性修复（not-found 报错、部分落盘对账）经核实是真实问题，不算范围蔓延。如果它落在分叉时的 main 上，已接近可合入。 但 main 此后重构了同一批 handler 与目录存储的失效路径（交互式优先级刷新），分支因此产生冲突；rebase 时需要作者有意识地决定：对账刷新保留交互优先级，还是写透已接管可见状态、后台优先级即可——这个决定应写进 rebase 后的 PR 描述，而不是在解冲突时随手 guess。 证据方面：提交刚推送几分钟，尚无任何 pull_request CI（首次贡献的 fork 作者，工作流等待批准），作者的本地测试结果仍只是自述；而"即时移动、无重复行"这一核心承诺是浏览器行为，任何 CI 通道都无法覆盖——即便 CI 全绿，也需要维护者在真实侧边栏里看一眼。 暂缓、转交 @ytahdn：本轮不作批准。需要：(1) 批准该 fork 的 CI 工作流，让提交获得真实测试证据；(2) 作者 rebase 后，确认刷新优先级的取舍，并手动过一遍置顶/取消置顶/归档流程。rebase 后可用 @qwen-code /triage 重新运行。 — Qwen Code · qwen3.8-max Reviewed at |
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
@qwen-code /triage |
| if (itemError) { | ||
| onError(new Error(itemError.error), t('sidebar.archiveFailed')); | ||
| } else { | ||
| archived = true; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-17: The locked/restricted batch branches treat a daemon notFound outcome as success. They only inspect result.errors, so when the daemon returns { archived: [], alreadyArchived: [], notFound: [sessionId], errors: [] } (session deleted concurrently by another client/tab, or a stale rendered list), the code sets archived = true, shows no error toast, then removeSession drops the still-active row from active pages and addSession seeds a phantom row into the archived pages. handleUnarchive has the identical shape with unarchived = true, materializing a deleted session into the active list. This contradicts the not-found-as-failure contract this same PR introduces for the primary branch a few lines below, which exists precisely to stop silent no-ops.
Verified with a probe driving the real component through Archive/Restore on a trusted secondary row while the RPC resolves { archived: [], alreadyArchived: [], notFound: [id], errors: [] }:
BASE (PR code): onErrorCalls=[] addSessionCatalogCalls=[["/tmp/other",{"sessionId":"secondary-gone",…,"isArchived":true},{"archiveStates":["archived"]}]]
PR + fix: onErrorCalls=[[{},"Failed to archive session"]] addSessionCatalogCalls=[] removeSessionCatalogCalls=[]
The probe fails on the PR code and flips green once the notFound check is added (whole file 88/88). Every trusted secondary workspace session takes this batch branch.
Mirror the primary contract in both batch branches — only set archived = true when the session is in result.archived/result.alreadyArchived:
if (itemError) {
onError(new Error(itemError.error), t('sidebar.archiveFailed'));
} else if (result.notFound.includes(sessionId)) {
onError(new Error(`session not found: ${sessionId}`), t('sidebar.archiveFailed'));
} else {
archived = true;
}(and symmetrically unarchived/alreadyActive + sidebar.unarchiveFailed for restore).
中文说明
锁定/受限工作区的批量归档分支把 daemon 的 notFound 结果当成了成功：分支只检查 result.errors，因此当 daemon 返回 { archived: [], alreadyArchived: [], notFound: [sessionId], errors: [] }（会话被其他客户端/标签页并发删除，或列表渲染已陈旧）时，代码会置 archived = true、不弹任何错误，随后 removeSession 把仍在活跃列表的行删掉、addSession 向归档页种入一条幽灵行。handleUnarchive 是同样的形状（unarchived = true），会把一个已删除的会话重新具现到活跃列表。这与本 PR 在下方主作用域分支里引入的 not-found 即失败的契约自相矛盾——那个分支正是为了消灭静默无操作而存在的。
探针驱动真实组件、让 RPC 解析 notFound 形状：PR 代码下 onErrorCalls=[] 且种入了幽灵行；补上 notFound 检查后报错正常触发、零缓存写入，全文件 88/88 通过。所有受信任次要工作区的会话都走这个批量分支。修复：两个批量分支都对照 result.archived/result.alreadyArchived（恢复用 unarchived/alreadyActive）判定成员资格，notFound 时走 onError。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| if (entry.query.workspaceCwd !== workspaceCwd || !entry.snapshot.page) { | ||
| continue; | ||
| } | ||
| if (entry.query.options.group === 'pinned') continue; |
There was a problem hiding this comment.
[Suggestion] R1-4: addSession unconditionally skips group: 'pinned' pages, but the sidebar's pinned query is archiveState: 'active' — so unarchiving a still-pinned session genuinely changes pinned-page membership, and the seeded row ends up hidden from every section until the refetch lands. The daemon keeps pin organization across an archive move (applyOrganization applies isPinned to both archive states), and handleUnarchive spreads the archived row, which carries isPinned: true.
Pin a session, archive it, then unarchive it: the row is seeded into active group: 'all' pages with isPinned: true; the regular list filters pinned rows out and the pinned section renders from the group: 'pinned' page this line skipped — so the row is absent from every sidebar section until the interactive refetch settles. That is the same invisibility window this PR eliminates for the other flows, and it is asymmetric with removeSession, which does drop the row from pinned pages on archive.
Store-level probe over the real handler sequence: PR as-is → pinned page [] and the regular list renders [] (row absent everywhere); with the implied fix below → pinned page ['S'] (probe assertion expected [ 'S' ] to deeply equal [] fails, proving the probe discriminates).
| if (entry.query.options.group === 'pinned') continue; | |
| if ( | |
| entry.query.options.group === 'pinned' && | |
| !( | |
| session.isPinned && | |
| (entry.query.options.archiveState ?? 'active') === | |
| (session.isArchived ? 'archived' : 'active') | |
| ) | |
| ) { | |
| continue; | |
| } |
中文说明
addSession 无条件跳过 group: 'pinned' 页，但侧边栏的置顶查询是 archiveState: 'active'——因此取消归档一条仍被置顶的会话确实会改变置顶页成员关系，而该行会在 refetch 落地前从所有分区消失。daemon 在归档移动中保留置顶组织（applyOrganization 对两种归档状态都应用 isPinned），且 handleUnarchive 展开的归档行携带 isPinned: true。探针证实：当前代码下置顶页与普通列表双双为空；按建议修复后置顶页出现该行。建议按归档状态匹配与否决定是否跳过置顶页（如上 suggestion），与 removeSession 在归档时确实会把行移出置顶页的行为对齐。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| this.setSnapshot(entry, { | ||
| ...entry.snapshot, | ||
| page: { | ||
| ...entry.snapshot.page, | ||
| sessions: [...entry.snapshot.page.sessions, session], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R1-16: addSession matches cached pages only on workspaceCwd, group !== 'pinned', and archiveState — it ignores the page's sourceType/sourceId/parentSessionId query filters, even though getSessionCatalogQueryKey keys entries on all three. Seeding therefore inserts rows into concurrently mounted pages whose daemon-side filter would exclude them (removeSession is directionally safe; this is insertion-only).
With SessionOverviewPanel or SplitView open (subscribed via useScopedSessions with sourceType: 'default', archiveState: 'active', same store singleton), the sidebar's unfiltered archived list still renders non-default-source rows — matchesSessionSource returns true for every row when no source filter is selected, and Restore is source-agnostic. Unarchiving a channel session then seeds it into the panel's 'default'-filtered page too: the panel renders a row its own query would never return until the targeted refetch lands, and a panel reopened within the 30s retention window shows the polluted page on first paint.
Probe replicating the scenario: unmodified PR → overview page ["live","chan"] (assertion expected [ 'live' ] fails, + "chan"); skipping entries whose filters exclude the session → ["live"], controls unchanged, 54/54 existing store tests pass. This also works against the JSDoc's own rationale ("the row appears in its destination list") — a row the destination query would exclude is not destination-list material.
Before the setSnapshot call, skip entries whose query filters exclude the session — require the sourceType filter to match (mirroring matchesSessionSource's undefined ≡ 'default' rule) and skip entries with sourceId/parentSessionId set, the same way the group === 'pinned' skip above guards membership.
中文说明
addSession 匹配缓存页时只看 workspaceCwd、group !== 'pinned' 与 archiveState，忽略了 sourceType/sourceId/parentSessionId 查询过滤——而 getSessionCatalogQueryKey 是以这三者作为键的。于是种子行会被插入到那些 daemon 侧过滤本会排除它的并发挂载页。当概览面板/分屏经 useScopedSessions（sourceType: 'default'）打开时，侧边栏未过滤的归档列表仍会渲染非默认来源的行（未选来源过滤时 matchesSessionSource 恒真，Restore 也不看来源），取消归档一条频道会话就会把它种进面板的 'default' 过滤页，直到定向 refetch 落地；30 秒保留窗口内重开面板还会首屏渲染被污染的页。探针：未修复时概览页 ["live","chan"]（断言失败），加入来源过滤跳过后 ["live"]，既有 54 个 store 测试不受影响。请在 setSnapshot 之前跳过过滤不匹配的 entry（sourceType 匹配规则对齐 matchesSessionSource 的 undefined ≡ 'default'，带 sourceId/parentSessionId 的 entry 直接跳过）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| Omit<DaemonSessionSummary, 'sessionId' | 'workspaceCwd'> | ||
| >, | ||
| ) { | ||
| update(() => store.patchSession(workspaceCwd, sessionId, patch)); |
There was a problem hiding this comment.
[Suggestion] R1-3: The newly added controller-level patchSession wrapper has zero production callers — dead API surface. The pre-existing internal paths bypass it: promptAdmitted and renamed call store.patchSession directly (hooks.ts:248, 265). The test mock wires a patchSessionCatalog spy that is reset in beforeEach but never invoked or asserted (expect(patchSessionCatalog) has zero matches). The four sibling methods added in the same hunk (invalidateSessionLists, removeSession, applySessionPin, addSession) each have real WebShellSidebar.tsx call sites. AGENTS.md's review rules ask to grep the read sites of every added method — none exist here, and the simplicity-first principle points the same way.
Drop the patchSession method from the controller object (and the patchSession/patchSessionCatalog mock plumbing in the workspace-removal test) until a consumer exists.
中文说明
新增的控制器级 patchSession 包装方法没有任何生产调用方——是死 API。既有的内部路径（promptAdmitted、renamed）直接调用 store.patchSession，绕过了这个包装；测试 mock 里的 patchSessionCatalog 探针被 reset 但从未被调用或断言。同一 hunk 新增的另外四个方法都有真实调用点。按 AGENTS.md 的评审规则（每个新增方法都要 grep 其读取点）与简洁优先原则，建议在出现真实调用方之前删除该方法及相应 mock 接线。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| setWorkspaceSessionsReloadToken((v) => v + 1); | ||
| return; | ||
| } | ||
| setWorkspaceScopedReloadTokens((prev) => { |
There was a problem hiding this comment.
[Suggestion] R1-11: workspaceScopedReloadTokens is add-only. bumpWorkspaceReload(workspaceCwd) — the map's only writer — inserts an entry per workspace cwd, but nothing ever removes one: reconcileRemovedWorkspace (~line 2190) bumps only the global token when a workspace is removed. Readers (getWorkspaceReloadToken(ws.cwd)) are only invoked for currently displayed workspaces, so removed workspaces' entries become permanently unread garbage, and every subsequent scoped bump copies the whole (growing) map.
Growth is bounded by distinct removed cwds within one mount and entries are tiny, so the cost is modest — but the diff makes this map the per-workspace state model, and only the establish side exists. Prune on removal, e.g. in reconcileRemovedWorkspace:
setWorkspaceScopedReloadTokens((prev) => {
if (!prev.has(removed.cwd)) return prev;
const next = new Map(prev);
next.delete(removed.cwd);
return next;
});中文说明
workspaceScopedReloadTokens 只增不减：唯一的写入方 bumpWorkspaceReload(workspaceCwd) 会为每个工作区 cwd 插入条目，但没有任何删除路径——reconcileRemovedWorkspace 移除工作区时只递增全局 token。读取方只针对当前展示的工作区调用，因此被移除工作区的条目成为永久无人读取的垃圾，且之后每次作用域 bump 都要整表复制。增长有界、条目很小，代价不大，但既然本 diff 让这张表成为按工作区的状态模型，建议补上删除侧：在 reconcileRemovedWorkspace 中删除对应 cwd 的条目（如上示例）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| ): Promise<DaemonSessionListPage> => | ||
| options?.archiveState === 'archived' | ||
| ? { | ||
| sessions: [{ sessionId: 'archived-only', workspaceCwd: '/w' }], |
There was a problem hiding this comment.
[Suggestion] R1-9 (location 2): 'removes a session only from pages matching the archive-state filter' cannot detect a removeSession that ignores options.archiveStates: it removes 'drop' with ['active'], but the archived fixture page holds only 'archived-only' — a broken filter would still pass both assertions (nothing to wrongly remove on the archived page). Probe: mutating removeSession to ignore archiveStates keeps 54/54 green; giving the archived page a 'drop' row expected to survive fails under the mutation with expected [ 'archived-only' ] to deeply equal [ 'archived-only', 'drop' ]. Give the archived branch [{ sessionId: 'drop', … }, { sessionId: 'archived-only', … }] and assert it keeps both after the targeted removal.
中文说明
“只从匹配归档状态过滤的页中移除会话”这个测试无法发现忽略 options.archiveStates 的 removeSession：它用 ['active'] 移除 'drop'，但归档 fixture 页里只有 'archived-only'——过滤即使失效，两个断言也照样通过（归档页没有可被误删的行）。探针：让 removeSession 忽略 archiveStates 后 54/54 全绿；让归档页也持有应存活的 'drop' 行后，变异下断言失败。请给归档分支同时放置 'drop' 与 'archived-only'，并断言定向移除后两者仍在。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| // A stale pinned page can briefly still hold a just-unpinned session | ||
| // that the main list already shows; filtering on the field keeps the | ||
| // row from appearing in both lists at once. | ||
| if (!session.isPinned) continue; |
There was a problem hiding this comment.
[Suggestion] R1-6: This pinned-section dedupe guard — the direct fix for the duplicate-row symptom reported in the linked issue — has no test that ever feeds the pinned section a session with isPinned: false. Every pinned fixture in the suite carries isPinned: true (workspace-removal and collapse-persist test files alike). Probe: deleting this line keeps 117/117 sidebar tests green, while a stale pinned page holding a just-unpinned session would then render the row in both the pinned section and the main list simultaneously — the exact regression the guard exists to catch. Add a sidebar test where the pinned-group page returns a session with isPinned: false that is also present in the active list, and assert it renders only in the main list.
中文说明
这个置顶区去重守卫——针对所关联 issue 报告的重复行症状的直接修复——没有任何测试向置顶区投喂 isPinned: false 的会话：全套测试中所有置顶 fixture 都带 isPinned: true。探针：删除这一行后 117/117 侧边栏测试全绿，而陈旧置顶页里刚取消置顶的会话将同时出现在置顶区与普通列表——正是该守卫要拦的回归。请补一个测试：置顶组页返回一条同时存在于活跃列表、但 isPinned: false 的会话，断言它只出现在普通列表。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| return next; | ||
| }); | ||
| }, []); | ||
| const getWorkspaceReloadToken = useCallback( |
There was a problem hiding this comment.
[Suggestion] R1-7: The per-workspace scoped reload-token mechanism (workspaceScopedReloadTokens, bumpWorkspaceReload(workspaceCwd), getWorkspaceReloadToken) — the change that stops an archive in one workspace from disturbing other workspaces' sections — is referenced by zero tests (grep across packages/web-shell test files finds no occurrence; WorkspaceSection.test.tsx covers only the prop-driven consumer, not this scoping decision). Probes: making bumpWorkspaceReload bump the global token again (restoring the all-workspaces group/channel/git reload storm this PR removes) keeps 173/173 green; making getWorkspaceReloadToken ignore the scoped map (silently stopping the affected section's reload signal) keeps 173/173 green. Add a two-workspace test asserting an archive in one changes only that section's effective reload signal (e.g. the other workspace's group-catalog refetch count stays unchanged).
中文说明
按工作区限定的重载 token 机制（workspaceScopedReloadTokens、bumpWorkspaceReload(workspaceCwd)、getWorkspaceReloadToken）——即“一个工作区的归档不再打扰其他工作区分区”的关键改动——没有任何测试引用（全包测试文件中零出现；WorkspaceSection.test.tsx 只覆盖 prop 驱动的消费端，不覆盖这里的限定决策）。探针：让 bumpWorkspaceReload 重新递增全局 token（恢复本 PR 移除的全工作区重载风暴）173/173 全绿；让 getWorkspaceReloadToken 忽略作用域表（受影响的分区再也收不到重载信号）同样 173/173 全绿。请补双工作区测试：在其中一个归档，断言只有该分区的重载信号变化（如另一工作区的组目录 refetch 次数不变）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| // An archive move only shifts the session between the active and archived | ||
| // lists, so a targeted invalidation suffices; the workspace-wide invalidate | ||
| // would also refetch unrelated queries of the same workspace. | ||
| const invalidateArchiveLists = useCallback(() => { |
There was a problem hiding this comment.
[Suggestion] R1-8: The primary-scope archive/unarchive reconcile now relies entirely on this invalidateArchiveLists finally, but no test exercises the path: session-catalog-hooks.test.tsx never invokes archiveSession/unarchiveSession (they appear only in mock setup), and every component consumer (sidebar, App, ChatPane, SideTaskPanel, useScopedSessions tests) mocks the whole module. The PR's own new sidebar test even asserts invalidateSessionCatalog).not.toHaveBeenCalled() after a primary-scope archive — codifying reliance on this hook invalidation while it is mocked away everywhere. Probe: removing invalidateArchiveLists() from both finallys keeps 873/873 tests green across every consumer — for a primary-scope archive the write-through would move the row, but no refetch would ever reconcile server ordering until the next periodic poll. In session-catalog-hooks.test.tsx, call facade.archiveSession(...) against a spied store and assert invalidateSessionLists was called with (cwd, ['active','archived'], { interactive: true }) and invalidateWorkspace was not.
中文说明
主作用域的归档/取消归档对账现在完全依赖这个 invalidateArchiveLists finally，但没有任何测试走这条路径：hooks 测试文件只在 mock 装配中出现这两个方法、从未调用；所有组件消费方测试都整模块 mock。本 PR 新增的侧边栏测试甚至断言主作用域归档后 invalidateSessionCatalog 未被调用——把对该钩子失效的依赖写进了契约，而该钩子在所有测试中都被 mock 掉。探针：从两个 finally 中移除 invalidateArchiveLists() 后 873/873 全绿——主作用域归档时写透会移动行，但再也不会有 refetch 对账服务端顺序。请在 hooks 测试中真实调用 facade.archiveSession(...) 并断言调用了 invalidateSessionLists(cwd, ['active','archived'], { interactive: true }) 且未调用 invalidateWorkspace。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| patchSessionCatalog.mockReset(); | ||
| removeSessionCatalog.mockReset(); |
There was a problem hiding this comment.
[Suggestion] R1-20: beforeEach resets every active.* action mock but — of the archived.* set — only archived.reload. The persistent archived.unarchiveSession.mockResolvedValue(false) set by 'reports a primary restore not-found' (~line 5187) therefore leaks into all later tests. Latent today (the only later test, 'orders the pinned section by pin time', never unarchives), but any future test exercising a primary unarchive inherits a stale 'session missing' mock and fails order-dependently with Failed to restore session — confusing to diagnose. Note 'seeds a restored primary row' already defensively re-sets mockResolvedValue(true), a symptom of the same leak. Reset the archived.* action mocks symmetrically with active.* here (and restore the fixture's default true resolutions).
中文说明
beforeEach 重置了所有 active.* 动作 mock，但 archived.* 里只重置了 archived.reload。因此 'reports a primary restore not-found'（约 5187 行）设置的持久 archived.unarchiveSession.mockResolvedValue(false) 会泄漏到之后所有测试。目前处于潜伏状态（其后唯一的测试不做取消归档），但未来任何走主作用域取消归档路径的测试都会继承这个陈旧的“会话不存在”mock，以 Failed to restore session order-dependent 地失败，难以诊断。'seeds a restored primary row' 已经不得不防御性地重设 mockResolvedValue(true)——同一泄漏的症状。请在此对称地重置 archived.* 动作 mock（并恢复 fixture 默认的 true 解析）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
…ists on RPC success
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-7 pinned-section dedupe guard untested — already reported (comment 3824178066)
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| if (entry.query.workspaceCwd !== workspaceCwd || !entry.snapshot.page) { | ||
| continue; | ||
| } | ||
| if (entry.query.options.group === 'pinned') continue; |
There was a problem hiding this comment.
[Suggestion] R1-4: Still standing from round 1. addSession unconditionally skips group: 'pinned' pages, but the sidebar's pinned query is archiveState: 'active' — so unarchiving a still-pinned session genuinely changes pinned-page membership, and the seeded row lands nowhere visible: the main lists filter out pinned rows (filteredSessions keeps !session.isPinned), the pinned section renders only from group: 'pinned' pages, and the pinned page does not hold the session. Concretely: pin an active session, archive it (pin state survives archiving), then unarchive — the row leaves the archived list, is seeded into no visible list, and the session disappears from the sidebar entirely until the paired invalidateSessionLists(['active','archived']) refetch lands; before this PR the stale row stayed visible during that window. Probe-verified at store level: after the write-through the active all-page holds the row with isPinned: true, the pinned page is empty, and the render filters exclude it. Seeding pinned pages for pinned sessions closes the gap:
| if (entry.query.options.group === 'pinned') continue; | |
| if (entry.query.options.group === 'pinned' && session.isPinned !== true) continue; |
中文说明
第一轮遗留（R1-4），仍然存在。addSession 无条件跳过 group: 'pinned' 页，但侧边栏的置顶区查询是 archiveState: 'active'——因此取消归档一条仍然置顶的会话确实会改变置顶页的成员关系，而种入的行落在任何可见位置之外：主列表过滤掉置顶行（filteredSessions 只保留 !session.isPinned），置顶区只从 group: 'pinned' 页渲染，而置顶页里并没有这条会话。具体场景：置顶一条活跃会话、归档（置顶状态保留）、再取消归档——该行离开归档列表，没有被种入任何可见列表，会话在成对的 invalidateSessionLists(['active','archived']) 刷新落地前从侧边栏完全消失；本 PR 之前陈旧行在那段时间里仍然可见。已在 store 层探针验证：写透后活跃 all 页持有该行且 isPinned: true，置顶页为空，渲染过滤将其排除。为置顶会话种入置顶页即可闭环：
— qwen3.8-max via Qwen Code /review (v0.21.15)
| if ( | ||
| options.archiveStates && | ||
| !options.archiveStates.includes( | ||
| entry.query.options.archiveState ?? 'active', | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R1-16: Still standing from round 1. addSession matches cached pages only on workspaceCwd, group !== 'pinned', and archiveState — it ignores the page's sourceType/sourceId/parentSessionId query filters, even though these are wire-affecting cache-key dimensions used by real production queries (the sidebar's source-scoped lists, App.tsx parent-scoped side-task lists). With a source-scoped page cached, unarchiving seeds the session into that page even when the daemon's listing for that source would not return it — the row renders until the paired refetch reconciles. The same matching shape appears in removeSession/applySessionPin. Probe: seeding a sourceType: 'default' session into a cached sourceType: 'channel' page lands the row ([channel-session, restored-default-session]); skipping entries whose source filter the session does not satisfy flips it back. Consider gating on the page's source/parent filters as well.
中文说明
第一轮遗留（R1-16），仍然存在。addSession 只按 workspaceCwd、group !== 'pinned' 和 archiveState 匹配缓存页——忽略了页面查询里的 sourceType/sourceId/parentSessionId 过滤器，尽管它们是影响线上行为的缓存键维度，真实生产查询也在用（侧边栏的按来源分区列表、App.tsx 的按父会话分区侧任务列表）。当缓存中存在按来源过滤的页面时，取消归档会把会话种入该页，即使 daemon 对该来源的列表并不会返回它——该行会一直渲染到成对刷新对账为止。removeSession/applySessionPin 也是同样的匹配形状。探针：把 sourceType: 'default' 的会话种入缓存的 sourceType: 'channel' 页，该行落入了（[channel-session, restored-default-session]）；跳过来源过滤器不匹配的条目即可还原。建议也把页面的 source/parent 过滤条件纳入判定。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| patchSession( | ||
| workspaceCwd: string, | ||
| sessionId: string, | ||
| patch: Partial< |
There was a problem hiding this comment.
[Suggestion] R1-3: Still standing from round 1. The newly added controller-level patchSession wrapper has zero production callers — dead API surface. The pre-existing internal paths (promptAdmitted, renamed) call store.patchSession directly, bypassing it, and the patchSessionCatalog spy the test mock wires up is set up and reset but never asserted. Repo-wide grep: .patchSession( appears exactly 4 times under packages/ — the wrapper body, the two pre-existing store-level call sites, and the store test. AGENTS.md § Code Review: "for every added field, option, or optional parameter, grep its read sites". Remove the wrapper (and its mock entry) until a caller exists, or land it together with the caller and its test.
中文说明
第一轮遗留（R1-3），仍然存在。新增的 controller 层 patchSession 包装方法没有任何生产调用方——死 API 面。既有的内部路径（promptAdmitted、renamed）直接调用 store.patchSession，绕过了它；测试 mock 里接好的 patchSessionCatalog 探针只有 setup 和 reset，从未被断言。全仓 grep：packages/ 下 .patchSession( 恰好出现 4 次——包装方法自身、两个既有的 store 层调用点、store 测试。AGENTS.md § Code Review 要求"对每个新增字段/选项/可选参数 grep 其读取点"。建议在有调用方之前移除该包装（及其 mock 条目），或与调用方及其测试一起提交。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| } finally { | ||
| invalidate(); | ||
| invalidateArchiveLists(); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-10: Still standing from round 1. The primary-scope archive/unarchive reconcile now relies entirely on this invalidateArchiveLists finally, but no test exercises the path: session-catalog-hooks.test.tsx never invokes archiveSession/unarchiveSession, and every sidebar test mocks the hook's actions directly — the primary write-through test even asserts the sidebar itself does NOT invalidate ("Primary scope relies on the catalog hook's own invalidation"), making this finally the only reconcile for primary-scope archive moves. Mutation-verified: removing both finally calls keeps the full web-shell suite green (4010/4010). Add a hook-level test (spy store.invalidateSessionLists) asserting it is called with (workspaceCwd, ['active','archived'], { interactive: true }) after resolve, including on rejection.
中文说明
第一轮遗留（R1-10），仍然存在。主作用域的归档/取消归档对账现在完全依赖这个 invalidateArchiveLists finally，但没有任何测试走到这条路径：session-catalog-hooks.test.tsx 从不调用 archiveSession/unarchiveSession，所有侧边栏测试都直接 mock 了 hook 的 action——主作用域写透测试甚至断言侧边栏自身不做失效（"Primary scope relies on the catalog hook's own invalidation"），使这个 finally 成为主作用域归档移动的唯一对账。变异验证：删除两处 finally 后完整 web-shell 套件仍然全绿（4010/4010）。建议补一个 hook 层测试（spy store.invalidateSessionLists），断言 resolve 后（含 reject 时）它以 (workspaceCwd, ['active','archived'], { interactive: true }) 被调用。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| setWorkspaceScopedReloadTokens((prev) => { | ||
| const next = new Map(prev); | ||
| next.set(workspaceCwd, (prev.get(workspaceCwd) ?? 0) + 1); | ||
| return next; | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R1-11: Still standing from round 1. workspaceScopedReloadTokens is add-only: bumpWorkspaceReload(workspaceCwd) — the map's only writer — inserts an entry per workspace cwd, but nothing ever removes one; reconcileRemovedWorkspace (~2183) invalidates the workspace's catalog and bumps the global token but never deletes the removed workspace's scoped entry. Each archive/unarchive in a later-removed workspace leaves a permanent entry for the sidebar's lifetime — growth is bounded by the count of distinct workspace cwds, but the global sibling token has no such accumulation. Delete the removed cwd's entry in reconcileRemovedWorkspace alongside the existing invalidateWorkspace call.
中文说明
第一轮遗留（R1-11），仍然存在。workspaceScopedReloadTokens 只增不减：bumpWorkspaceReload(workspaceCwd)——该 map 唯一的写入方——按工作区 cwd 插入条目，但没有任何地方删除；reconcileRemovedWorkspace（约 2183 行）会失效该工作区的目录并抬升全局 token，却从不删除被移除工作区的 scoped 条目。每次发生在"后来被移除的工作区"里的归档/取消归档都会留下一个伴随侧边栏余下寿命的永久条目——增长上限是不同 cwd 的数量，但全局兄弟 token 并无此类累积。建议在 reconcileRemovedWorkspace 中连同既有的 invalidateWorkspace 一起删除该 cwd 的条目。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // Primary scope relies on the catalog hook's own invalidation. | ||
| expect(invalidateSessionCatalog).not.toHaveBeenCalled(); | ||
| expect(onError).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] R2-10: None of the new write-through success tests (pin :5027, primary archive here, primary restore :5144) assert refreshWorkspaceSessionCatalog was NOT called — grep shows zero such assertions in :4966-5326 — so a regression re-adding the workspace-wide refresh this PR removes ships green (the invalidateSessionCatalog assertions use a different spy). The PR's own locked-unpin step (:1502) already uses this negative assertion; the pattern is not applied in this describe block. Probe: re-adding refreshWorkspace to the archive path ships green file-wide (88 passed); the negative assertion catches it. (Verification notes the pin-path half is partly guarded by the locked-unpin assertion; the archive/unarchive half is completely unguarded.) Add expect(refreshWorkspaceSessionCatalog).not.toHaveBeenCalled() to the pin, primary-archive, and primary-restore success tests.
中文说明
新的写透成功测试（置顶 :5027、此处的主作用域归档、主作用域恢复 :5144）都没有断言 refreshWorkspaceSessionCatalog 未被调用——grep 显示 :4966-5326 中此类断言为零——因此把本 PR 移除的工作区级刷新加回来的回归可以绿着上线（invalidateSessionCatalog 断言用的是另一个探针）。本 PR 自己的锁定 unpin 步骤（:1502）已经使用了该负向断言；此 describe 块却没有套用。探针：把 refreshWorkspace 加回归档路径，整个文件全绿（88 passed）；负向断言即可捕获。（验证注明置顶路径一半由锁定 unpin 断言部分守护；归档/取消归档一半完全无守护。）请在置顶、主归档、主恢复三个成功测试中补上 expect(refreshWorkspaceSessionCatalog).not.toHaveBeenCalled()。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| pinnedAt: '2026-01-01T00:00:00.000Z', | ||
| isPinned: false, | ||
| }); | ||
| expect(onError).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] R2-11: 'unpins without writing a stale pinnedAt' never asserts the targeted invalidation its pin twin pins (:5027 expect(invalidateSessionCatalog).toHaveBeenCalledWith('/tmp/project')), even though production handleTogglePin calls invalidateSessionLists(workspaceCwd, ['active'], { interactive: true }) direction-agnostically in its .then. Probe: gating the invalidation in if (result.isPinned) keeps this test green; adding the assertion catches it. (Verification notes the locked-unpin step detects a direction-gating regression in the full suite; the asymmetry inside this describe block is nonetheless real.) Add expect(invalidateSessionCatalog).toHaveBeenCalledWith('/tmp/project') before the onError assertion here.
中文说明
'unpins without writing a stale pinnedAt' 从未断言其置顶孪生测试所钉住的定向失效（:5027 expect(invalidateSessionCatalog).toHaveBeenCalledWith('/tmp/project')），尽管生产的 handleTogglePin 在其 .then 中不区分方向地调用 invalidateSessionLists(workspaceCwd, ['active'], { interactive: true })。探针：把失效包进 if (result.isPinned) 本测试仍绿；补上断言即可捕获。（验证注明锁定 unpin 步骤在完整套件中能检测方向限定回归；但此 describe 块内部的不对称依然真实存在。）请在此处 onError 断言前补上 expect(invalidateSessionCatalog).toHaveBeenCalledWith('/tmp/project')。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| archiveStates && | ||
| !archiveStates.includes(entry.query.options.archiveState ?? 'active') |
There was a problem hiding this comment.
[Suggestion] R2-12: The ?? 'active' default for queries carrying no archiveState — duplicated verbatim in this diff's invalidateEntries, removeSession, and addSession — is pinned by no test: every query fixture in session-catalog-store.test.ts sets an explicit archiveState (including via the query() helper), and invalidateSessionLists is invoked once (:692) with only explicit-state entries loaded. Production genuinely loads unfiltered queries (App.tsx loadSessionCatalogOnce with { pageSize: 200 }, no archiveState). Probe: flipping all three defaults to ?? 'archived' fails a probe loading an archiveState-less query while all 54 existing store tests stay green. In 'invalidates only catalog entries matching the requested archive states', subscribe a third entry whose options omit archiveState and assert it is marked stale and refetched when ['active'] is requested.
中文说明
针对不带 archiveState 查询的 ?? 'active' 默认值——在本 diff 的 invalidateEntries、removeSession、addSession 中逐字重复了三遍——没有任何测试钉住：session-catalog-store.test.ts 的每个查询 fixture 都显式设置了 archiveState（包括经 query() helper），且 invalidateSessionLists 只在 :692 被调用一次、当时只加载了显式状态条目。生产中确实会加载无过滤查询（App.tsx 的 loadSessionCatalogOnce 只带 { pageSize: 200 }，无 archiveState）。探针：把三处默认值都翻转为 ?? 'archived'，一个加载无 archiveState 查询的探针即失败，而全部 54 个既有 store 测试仍绿。请在 'invalidates only catalog entries matching the requested archive states' 中订阅第三个 options 省略 archiveState 的条目，并断言请求 ['active'] 时它同样被标记陈旧并刷新。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| removeSession( | ||
| workspaceCwd: string, | ||
| sessionId: string, |
There was a problem hiding this comment.
[Suggestion] R2-13: removeSession deliberately does NOT skip group: 'pinned' pages (unlike addSession's documented server-owned skip), and that non-skip is load-bearing: archiving a pinned session must drop it from the cached active pinned page — this method is the only mechanism clearing it, and the path is reachable today (pin an active session, then archive it from the pinned-section row's menu; the sidebar mounts the pinned query at archiveState: 'active', group: 'pinned'). No test anywhere exercises removeSession against a pinned entry: the sole store test (:499) loads group-less queries, and every sidebar test mocks the controller. Probe: adding addSession's pinned-skip here keeps all 54 store tests and all 88 sidebar tests green while the archived row lingers in the cached pinned page; a store test loading a group: 'pinned' entry alongside the active entry catches it. Worth pinning before a "symmetry" refactor does the damage.
中文说明
removeSession 刻意不跳过 group: 'pinned' 页（不同于 addSession 文档化的服务端所有跳过），而这个"不跳过"是承重的：归档一条置顶会话必须把它从缓存的活跃置顶页中删除——本方法是清除它的唯一机制，且该路径今天可达（置顶一条活跃会话，然后从置顶区行的菜单归档；侧边栏以 archiveState: 'active', group: 'pinned' 挂载置顶查询）。没有任何测试让 removeSession 作用于 pinned 条目：唯一的 store 测试（:499）只加载无 group 查询，所有侧边栏测试都 mock 了 controller。探针：在此处加上 addSession 的 pinned 跳过后，54 个 store 测试与 88 个侧边栏测试全绿，而被归档的行滞留在缓存置顶页；让 store 测试在活跃条目之外再加载一个 group: 'pinned' 条目即可捕获。值得在某次"对称性"重构造成损害之前钉住它。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // Unpin reconciles through a targeted invalidation of the active lists | ||
| // instead of the workspace-wide refresh. | ||
| expect(invalidateSessionCatalog).toHaveBeenLastCalledWith('/tmp/other'); |
There was a problem hiding this comment.
[Suggestion] R2-14: The new "targeted invalidation" assertions (archive step :1452, unpin step here) cannot distinguish invalidateSessionLists from invalidateWorkspace, because the controller mock wires both methods to the same invalidateSessionCatalog spy — and the invalidateSessionLists leg of that wiring was added by this diff. A regression widening the pin-toggle's reconcile to invalidateWorkspace(workspaceCwd) ships green while every pin toggle also refetches the workspace's archived page and any other cached workspace queries — exactly the extra refetches invalidateSessionLists was introduced to avoid (['active'] skips archived entries; invalidateWorkspace does not). Probe: the widening mutant passes the entire file (88/88); wiring invalidateSessionLists to a dedicated spy flips it red. Distinct from the option-dropping mock finding reported earlier. Wire the mock's invalidateSessionLists to its own spy and point these assertions at it (the write-through suite's pin assertion would move to the new spy too).
中文说明
新的"定向失效"断言（归档步骤 :1452、此处的 unpin 步骤）无法区分 invalidateSessionLists 与 invalidateWorkspace，因为 controller mock 把两个方法接到了同一个 invalidateSessionCatalog 探针——而其中 invalidateSessionLists 这条接线正是本 diff 新增的。把置顶切换的对账放宽为 invalidateWorkspace(workspaceCwd) 的回归可以绿着上线，同时每次置顶切换都会额外刷新该工作区的归档页及其他缓存查询——恰是 invalidateSessionLists 被引入以避免的多余刷新（['active'] 跳过归档条目，invalidateWorkspace 不会）。探针：放宽变异通过整个文件（88/88）；把 invalidateSessionLists 接到专用探针即可使其变红。与先前报告的 mock 丢参问题不同。请把 mock 的 invalidateSessionLists 接到专用探针并让这些断言指向它（写透套件的置顶断言也要迁到新探针）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
@qwen-code /triage |
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
🖼️ 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 |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
静态/diff 审查，head 0688c89（含 merge commit 完整性核对；未本地运行测试）。
中文：
结论：💬 评论，我方未发现新的阻塞项。 两点核心结论：round 1 的 Critical（R1-17，批量分支把 notFound 当成功）在当前 head 上已被修复，证据如下；机器人已报告的 R1-4（恢复置顶会话时的消失窗口）经独立验证，建议合入前处理。
一、round 1 Critical（R1-17）已在当前 head 修复
两个批量分支（locked/restricted）现在都与主作用域契约一致：result.notFound.includes(sessionId) → onError(...)（head tree 中 WebShellSidebar.tsx 约 3118 与 3218 行），不会再置 archived = true 并种入幽灵行。来源核对：round 1 评审锚定的快照（父提交 3e99b71 一系）中 result.notFound.includes 出现次数为零，且比当前分支少约 105 行 web-shell 代码（16 行实现 + 89 行测试）；当前分支自 0cf0d86 起就包含这两处守卫。因此那条 CHANGES_REQUESTED 所依据的问题已不存在于 PR 代码中。
二、R1-4 建议合入前修复（对机器人已有发现的独立评估，非重复报告）
机制与 R1-4 描述一致（addSession 无条件跳过 group: 'pinned' 页）。补充一个被低估的失败模式：消失窗口不止一次刷新往返——当成对的交互式 refetch 失败时，store 会设 retryAt = Date.now() + SESSION_CATALOG_ERROR_RETRY_MS（30 秒，session-catalog-store.ts:98、约 1185 行），因此"置顶 → 归档 → 恢复"的会话最长可能看起来像被删除了 30 秒；本 PR 之前陈旧行在那个窗口里仍然可见，所以这是写透引入的用户可见回退，与本 PR 要消灭的可见性缺口同族。另外目前没有任何测试覆盖"恢复一条置顶会话"。机器人给出的一行修复方向正确（if (entry.query.options.group === 'pinned' && session.isPinned !== true) continue;，或恢复时对置顶行补一次 applySessionPin 种入），补上对应测试即可闭环。
三、其余交叉验证结果（均通过）
- merge 完整性：
git diff 0cf0d86e5..0688c895b -- packages/web-shell/为空，且 main 侧父提交在 web-shell 上没有与本分支竞争的改动——merge commit 未丢失任何 main 侧内容；triage 提出的"对账刷新优先级"问题也已有明确答案：分支保留了交互式优先级（四处写透后的失效均带{ interactive: true }，pin 失败走refreshWorkspace）。 - 不可变性：removeSession/addSession/applySessionPin 均产生新引用并经 setSnapshot 发布，无原地修改，快照消费者会正常重渲染。
- pin/unpin 对称：共用 handleTogglePin，两个方向的写透、
invalidateSessionLists(cwd, ['active'], { interactive: true })、失败对账完全一致；置顶区查询本身带archiveState: 'active'，所以['active']失效也能命中置顶页。 - 排序：pinnedAt 升序 + sessionId 确定性兜底，
!isPinned过滤先于排序执行，取消置顶的行不会混入置顶区。 - 契约：webui
archiveSession/unarchiveSession返回 boolean（false ⇔ daemon 报缺失且无错误条目）、批量结果形状{archived, alreadyArchived, notFound, errors}均与调用处匹配。 - removeSession 保留 nextCursor 不会丢行：服务端游标编码位置而非内容，且当前无挂载的游标分页页。
- R1-16 的影响面经核实为外观性（打开的 Resume/Delete/Release 对话框或概览面板里约一次往返的幽灵行，随后被对账替换），自愈，不阻塞。
🎉 值得肯定的点：写透 + 定向失效 + 仅失败时全量刷新的分层对账设计；按工作区限定的重载令牌严格弱于旧的全局 bump；not-found 即失败的双向契约；store 层测试断言真实页面内容（行、顺序、游标、去重）而非仅调用次数。
当前状态：分支在作者 merge main 之后又与 main 产生了新冲突（main 此后合入了同样改动 session-catalog-hooks.ts 的 PR）——合入前需要再 merge 一次 main；CI 在 0688c89 上尚有 Test (ubuntu-latest, Node 22.x) 在运行（其余已过或按 fork 惯例跳过）。机器人两轮约 40 条建议级行内评论仍开放，多为测试 mock 保真度问题。综合判断：代码方向与实现质量都不错，处理掉 R1-4、解决冲突、CI 全绿后即可按惯例批准。
English:
Static/diff review at head 0688c89 (including merge-commit integrity checks; tests not run locally).
Verdict: 💬 comment — no new blockers from this pass. Two load-bearing conclusions: the round-1 Critical (R1-17, batch branches treating notFound as success) is fixed at the current head, with evidence below; and the already-reported R1-4 (restored-pinned-session disappearance window) was independently verified and is worth addressing before merge.
1. The round-1 Critical (R1-17) is fixed at the current head. Both batch branches (locked/restricted) now mirror the primary-scope contract: result.notFound.includes(sessionId) → onError(...) (WebShellSidebar.tsx around lines 3118 and 3218 in the head tree), so a not-found outcome no longer sets archived = true and seeds a phantom row. Provenance: the snapshot round 1 reviewed (parent line 3e99b71) contains zero occurrences of result.notFound.includes and is ~105 web-shell lines shorter than the current branch (16 implementation + 89 test lines); the branch has carried both guards since 0cf0d86. The CHANGES_REQUESTED state therefore rests on a finding that no longer exists in the PR code.
2. R1-4 should be fixed before merge (independent assessment of an existing bot finding, not a re-report). The mechanism is as R1-4 describes (addSession unconditionally skips group: 'pinned' pages). One under-appreciated failure mode: the disappearance window is not just one refetch round trip — when the paired interactive refetch fails, the store sets retryAt = Date.now() + SESSION_CATALOG_ERROR_RETRY_MS (30 s; session-catalog-store.ts:98 and ~1185), so a "pin → archive → restore" session can look deleted for up to 30 seconds. Pre-PR the stale row stayed visible during that window, which makes this a user-visible regression introduced by the write-through — the same family of visibility gap this PR exists to eliminate. No test currently covers restoring a pinned session. The bot's one-line fix direction is right (if (entry.query.options.group === 'pinned' && session.isPinned !== true) continue;, or seed pinned rows via applySessionPin on restore), plus a test for the pinned-restore path.
3. Remaining cross-verification (all passed).
- Merge integrity:
git diff 0cf0d86e5..0688c895b -- packages/web-shell/is empty, and the main-side parent carried no competing web-shell changes — the merge commit dropped nothing; triage's refetch-priority question has a clear answer in the code: interactive priority is kept (all four write-throughs invalidate with{ interactive: true }; pin failure reconciles viarefreshWorkspace). - Immutability: removeSession/addSession/applySessionPin all build fresh references and publish via setSnapshot — no in-place mutation, snapshot consumers re-render.
- Pin/unpin symmetry: shared handleTogglePin; identical write-through,
invalidateSessionLists(cwd, ['active'], { interactive: true }), and failure reconcile in both directions; the pinned section's query carriesarchiveState: 'active', so the['active']invalidation reaches pinned pages too. - Ordering: ascending pinnedAt with a deterministic sessionId tie-break; the
!isPinnedfilter runs before sorting, so unpinned rows cannot leak into the pinned section. - Contracts: webui
archiveSession/unarchiveSessionreturn booleans (false ⇔ daemon reports missing with no error entry) and the batch result shape{archived, alreadyArchived, notFound, errors}matches the call sites. - removeSession preserving nextCursor cannot strand rows: server cursors encode position rather than contents, and no cursor-paginated pages are mounted today.
- R1-16's impact verified cosmetic (a phantom row in an open Resume/Delete/Release dialog or the overview panel for about one round trip, then reconciled); self-healing, non-blocking.
🎉 Highlights: the write-through + targeted invalidation + failure-only full refresh layering; workspace-scoped reload tokens that are strictly weaker than the old global bump; the bidirectional not-found-as-failure contract; store tests asserting real page contents (rows, order, cursors, dedupe) rather than call counts.
Current state: the branch conflicts with main again (main has since merged PRs touching session-catalog-hooks.ts after the author's merge) — one more merge of main is needed before this can be queued; on 0688c89, Test (ubuntu-latest, Node 22.x) was still running at review time (everything else green or skipped per fork convention). Roughly forty suggestion-level bot comments remain open, mostly test-mock fidelity. Overall: direction and implementation quality are solid; with R1-4 addressed, the conflict resolved, and CI green, this is approvable as usual.
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
addSession skipped every group:'pinned' page unconditionally, so restoring a still-pinned session left its row absent from every sidebar section until the paired refetch landed — and when that refetch fails, the catalog's 30s error-retry window made the session look deleted for half a minute. The daemon keeps pin organization across an archive move, so a restored row carries isPinned and its pinned-section membership must be reseeded with it. Non-pinned rows still never enter pinned pages.
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.
16 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 controller mock fidelity (invalidateSessionLists drops archiveStates/options) — already reported (comment 3827414768)
- R2-1 removeSession mock drops options argument (location 2) — already reported (comment 3827414771)
- R1-12 archive step missing negative refresh assertion — already reported (comment 3827414773)
- R2-2 primary-restore twin missing negative assertion (location 2) — already reported (comment 3827414777)
- R1-20 beforeEach leaves archived.* mocks unreset — already reported (comment 3827414787)
- R2-4 TS2739 synchronous useWorkspaceSessionCatalog callback — already reported (comment 3827414791)
- R2-5 TS2739 synchronous callback (location 2) — already reported (comment 3827414795)
- R2-6 invalidateArchiveLists comment misdescribes scope — already reported (comment 3827414799)
- R2-9 pinned-section ordering test is one-directional — already reported (comment 3827414808)
- R2-10 locked-secondary unpin never asserts applySessionPin — already reported (comment 3827414812)
- R2-11 locked-archive step never asserts the write-through — already reported (comment 3827414816)
- R2-12 batch not-found tests miss the failure-path reconcile — already reported (comment 3827414820)
- R2-13 pin tests cannot distinguish confirmed vs optimistic payload — already reported (comment 3827414823)
- R2-14 write-through success tests lack no-full-refresh assertion — already reported (comment 3827414827)
- R2-15 unpin test missing the targeted-invalidation assertion — already reported (comment 3827414829)
- R2-18 shared spy cannot distinguish targeted from workspace invalidation — already reported (comment 3827414837)
中文说明
已审查——无阻断问题。 建议见行内评论。
本轮确认的 16 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (entry.query.options.group === 'pinned' && session.isPinned !== true) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-16: Still standing from round 2. addSession matches cached pages only on workspaceCwd, group !== 'pinned', and archiveState — it ignores the page's sourceType/sourceId/parentSessionId query filters, even though these are wire-affecting cache-key dimensions used by real production queries (the sidebar's source-scoped lists, App.tsx parent-scoped side-task lists). With a source-scoped page cached, unarchiving seeds the session into that page even when the daemon's listing for that source would not return it — the row renders until the paired refetch reconciles. The same matching shape appears in removeSession/applySessionPin. Probe: seeding a sourceType: 'default' session into a cached sourceType: 'channel' page lands the row ([channel-session, restored-default-session]); skipping entries whose source filter the session does not satisfy flips it back. Consider gating on the page's source/parent filters as well.
中文说明
第二轮遗留（R1-16），仍然存在。addSession 只按 workspaceCwd、group !== 'pinned' 和 archiveState 匹配缓存页——忽略了页面查询里的 sourceType/sourceId/parentSessionId 过滤器，尽管它们是影响线上行为的缓存键维度，真实生产查询也在用（侧边栏的按来源分区列表、App.tsx 的按父会话分区侧任务列表）。当缓存中存在按来源过滤的页面时，取消归档会把会话种入该页，即使 daemon 对该来源的列表并不会返回它——该行会一直渲染到成对刷新对账为止。removeSession/applySessionPin 也是同样的匹配形状。探针：把 sourceType: 'default' 的会话种入缓存的 sourceType: 'channel' 页，该行落入了（[channel-session, restored-default-session]）；跳过来源过滤器不匹配的条目即可还原。建议也把页面的 source/parent 过滤条件纳入判定。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| patchSession( | ||
| workspaceCwd: string, | ||
| sessionId: string, | ||
| patch: Partial< | ||
| Omit<DaemonSessionSummary, 'sessionId' | 'workspaceCwd'> | ||
| >, | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R1-3: Still standing from round 2. The newly added controller-level patchSession wrapper has zero production callers — dead API surface. The pre-existing internal paths (promptAdmitted, renamed) call store.patchSession directly, bypassing it, and the patchSessionCatalog spy the test mock wires up is set up and reset but never asserted. Repo-wide grep: .patchSession( appears exactly 4 times under packages/ — the wrapper body, the two pre-existing store-level call sites, and the store test. AGENTS.md § Code Review: "for every added field, option, or optional parameter, grep its read sites". Remove the wrapper (and its mock entry) until a caller exists, or land it together with the caller and its test.
中文说明
第二轮遗留（R1-3），仍然存在。新增的 controller 层 patchSession 包装方法没有任何生产调用方——死 API 面。既有的内部路径（promptAdmitted、renamed）直接调用 store.patchSession，绕过了它；测试 mock 里接好的 patchSessionCatalog 探针只有 setup 和 reset，从未被断言。全仓 grep：packages/ 下 .patchSession( 恰好出现 4 次——包装方法自身、两个既有的 store 层调用点、store 测试。AGENTS.md § Code Review 要求"对每个新增字段/选项/可选参数 grep 其读取点"。建议在有调用方之前移除该包装（及其 mock 条目），或与调用方及其测试一起提交。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| try { | ||
| return await workspace.actions.archiveSession(sessionId); | ||
| } finally { | ||
| invalidate(); | ||
| invalidateArchiveLists(); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-10: Still standing from round 2. The primary-scope archive/unarchive reconcile now relies entirely on this invalidateArchiveLists finally, but no test exercises the path: session-catalog-hooks.test.tsx never invokes archiveSession/unarchiveSession, and every sidebar test mocks the hook's actions directly — the primary write-through test even asserts the sidebar itself does NOT invalidate ("Primary scope relies on the catalog hook's own invalidation"), making this finally the only reconcile for primary-scope archive moves. Mutation-verified: removing both finally calls keeps the full web-shell suite green (4010/4010). Add a hook-level test (spy store.invalidateSessionLists) asserting it is called with (workspaceCwd, ['active','archived'], { interactive: true }) after resolve, including on rejection.
中文说明
第二轮遗留（R1-10），仍然存在。主作用域的归档/取消归档对账现在完全依赖这个 invalidateArchiveLists finally，但没有任何测试走到这条路径：session-catalog-hooks.test.tsx 从不调用 archiveSession/unarchiveSession，所有侧边栏测试都直接 mock 了 hook 的 action——主作用域写透测试甚至断言侧边栏自身不做失效（"Primary scope relies on the catalog hook's own invalidation"），使这个 finally 成为主作用域归档移动的唯一对账。变异验证：删除两处 finally 后完整 web-shell 套件仍然全绿（4010/4010）。建议补一个 hook 层测试（spy store.invalidateSessionLists），断言 resolve 后（含 reject 时）它以 (workspaceCwd, ['active','archived'], { interactive: true }) 被调用。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| setWorkspaceScopedReloadTokens((prev) => { | ||
| const next = new Map(prev); | ||
| next.set(workspaceCwd, (prev.get(workspaceCwd) ?? 0) + 1); | ||
| return next; | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R1-11: Still standing from round 2. workspaceScopedReloadTokens is add-only: bumpWorkspaceReload(workspaceCwd) — the map's only writer — inserts an entry per workspace cwd, but nothing ever removes one; reconcileRemovedWorkspace (~2183) invalidates the workspace's catalog and bumps the global token but never deletes the removed workspace's scoped entry. Each archive/unarchive in a later-removed workspace leaves a permanent entry for the sidebar's lifetime — growth is bounded by the count of distinct workspace cwds, but the global sibling token has no such accumulation. Delete the removed cwd's entry in reconcileRemovedWorkspace alongside the existing invalidateWorkspace call.
中文说明
第二轮遗留（R1-11），仍然存在。workspaceScopedReloadTokens 只增不减：bumpWorkspaceReload(workspaceCwd)——该 map 唯一的写入方——按工作区 cwd 插入条目，但没有任何地方删除；reconcileRemovedWorkspace（约 2183 行）会失效该工作区的目录并抬升全局 token，却从不删除被移除工作区的 scoped 条目。每次发生在"后来被移除的工作区"里的归档/取消归档都会留下一个伴随侧边栏余下寿命的永久条目——增长上限是不同 cwd 的数量，但全局兄弟 token 并无此类累积。建议在 reconcileRemovedWorkspace 中连同既有的 invalidateWorkspace 一起删除该 cwd 的条目。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const getWorkspaceReloadToken = useCallback( | ||
| (workspaceCwd: string) => | ||
| workspaceSessionsReloadToken + | ||
| (workspaceScopedReloadTokens.get(workspaceCwd) ?? 0), |
There was a problem hiding this comment.
[Suggestion] R1-8: Still standing from round 2. The per-workspace scoped reload-token mechanism (workspaceScopedReloadTokens, bumpWorkspaceReload(workspaceCwd), getWorkspaceReloadToken) — the change that stops an archive in one workspace from disturbing other workspaces' sections — has no test: nothing references any of the three, and the mocked session-catalog hooks swallow the prop. Mutation-verified: reverting both reloadToken={getWorkspaceReloadToken(ws.cwd)} sites to the shared global token keeps the full suite green (4010/4010) while restoring the cross-workspace reload regression this PR removes. Assert the scoping through the rendered tree or the polling spy — after archiving in '/tmp/other', the '/tmp/project' section must receive no reload signal while '/tmp/other''s token increments.
中文说明
第二轮遗留（R1-8），仍然存在。按工作区限定的重载 token 机制（workspaceScopedReloadTokens、bumpWorkspaceReload(workspaceCwd)、getWorkspaceReloadToken）——即"一个工作区的归档不再打扰其他工作区分区"这一变更——没有任何测试：三者均无引用，mock 的会话目录 hook 又吞掉了该 prop。变异验证：把两处 reloadToken={getWorkspaceReloadToken(ws.cwd)} 还原为共享全局 token，整个套件仍全绿（4010/4010），同时本 PR 要消除的跨工作区重载回归复活。建议通过渲染树或轮询探针断言限定性——在 '/tmp/other' 归档后，'/tmp/project' 分区不应收到任何重载信号，而 '/tmp/other' 的 token 递增。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| await store.loadOnce(activeQuery, { fresh: true }); | ||
| await store.loadOnce(archivedQuery, { fresh: true }); | ||
|
|
||
| store.removeSession('/w', 'drop', { archiveStates: ['active'] }); |
There was a problem hiding this comment.
[Suggestion] R2-3: Still standing from round 2. 'removes a session only from pages matching the archive-state filter' cannot detect a removeSession that ignores options.archiveStates: it removes 'drop' with ['active'], but 'drop' exists only in the active fixture — an archiveStates-ignoring implementation passes identically. Mutation-verified again this round: deleting the filter leaves the entire 55-test store file green; a both-pages discriminator probe fails against the mutant (expected [] to deeply equal ['drop']) and passes on the real code. Put the target session in both fixtures so the filter is genuinely under test.
中文说明
第二轮遗留（R2-3），仍然存在。'removes a session only from pages matching the archive-state filter' 无法检测忽略 options.archiveStates 的 removeSession：它用 ['active'] 删除 'drop'，但 'drop' 只存在于活跃 fixture——忽略 archiveStates 的实现同样能通过。本轮再次变异验证：删除过滤器后整个 55 用例的 store 测试文件仍全绿；让 'drop' 同时存在于两个 fixture 的判别探针在变异体上失败（expected [] to deeply equal ['drop']）、在真实代码上通过。请把目标会话放进两个 fixture，让过滤器真正受到检验。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * move. Cheaper than invalidateWorkspace when unrelated queries (other | ||
| * archive states, unfiltered variants) are mounted for the same workspace. |
There was a problem hiding this comment.
[Suggestion] R2-7: Still standing from round 2. The sibling of the hooks-comment finding: this JSDoc claims unfiltered query variants are skipped as "unrelated queries", but entries without an archiveState resolve via ?? 'active' and are invalidated by BOTH documented call shapes ('active' pin toggle and ['active','archived'] archive move) — never skipped — and with the enum holding exactly two values, the "Cheaper than invalidateWorkspace" premise never holds for the archive-move shape. Unfiltered entries genuinely exist in production (App.tsx loadSessionCatalogOnce with { pageSize: 200 }, no archiveState). Reword to match the implementation, e.g.: "Entries without an archive-state filter are treated as 'active' (the daemon default), so they are invalidated whenever 'active' is requested; only entries of a different archive state are skipped."
中文说明
第二轮遗留（R2-7），仍然存在。hooks 注释问题的姊妹篇：这段 JSDoc 声称无过滤条件的查询变体会作为"无关查询"被跳过，但没有 archiveState 的条目经 ?? 'active' 解析后，会被两种文档化调用形状（'active' 置顶切换与 ['active','archived'] 归档移动）都失效——从不被跳过——且在枚举恰有两个值的情况下，"Cheaper than invalidateWorkspace" 的前提对归档移动形状从不成立。生产中确实存在无过滤条目（App.tsx 的 loadSessionCatalogOnce 只带 { pageSize: 200 }，无 archiveState）。建议按实现改写，例如："没有归档状态过滤条件的条目按 'active'（daemon 默认值）处理，因此只要请求了 'active' 就会被失效；只有其他归档状态的条目会被跳过。"
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (!confirmed) { | ||
| // The RPC may have applied server-side before failing on the | ||
| // client (network drop mid-response), so reconcile fully on | ||
| // failure instead of trusting the untouched cache. |
There was a problem hiding this comment.
[Suggestion] R2-8: Still standing from round 2. The pin-handler failure reconcile (!confirmed → refreshWorkspace) — added by this PR precisely for the network-drop-mid-response case — has no test: updateSessionOrganization is only ever mockResolvedValue in this file (the only mockRejectedValues are removeWorkspace), and no test references sidebar.organizationFailed. Deleting the entire if (!confirmed) block keeps every test green, so the reconcile could silently disappear in a later refactor, leaving the sidebar cache diverged from daemon pin state until the next poll. Add a test with updateSessionOrganization.mockRejectedValueOnce(new Error('network')) asserting refreshWorkspaceSessionCatalog is called with the session's workspace and applySessionPinCatalog/invalidateSessionCatalog are not.
中文说明
第二轮遗留（R2-8），仍然存在。pin 处理器的失败对账（!confirmed → refreshWorkspace）——本 PR 专为"响应中途断网但服务端已应用"场景新增——没有任何测试：本文件中 updateSessionOrganization 只有 mockResolvedValue（仅有的 mockRejectedValue 属于 removeWorkspace），也没有测试引用 sidebar.organizationFailed。整体删除 if (!confirmed) 块所有测试仍绿，因此该对账可能在后续重构中静默消失，使侧边栏缓存与 daemon 置顶状态背离直到下次轮询。建议新增测试：updateSessionOrganization.mockRejectedValueOnce(new Error('network'))，断言 refreshWorkspaceSessionCatalog 以该会话的工作区被调用，且 applySessionPinCatalog/invalidateSessionCatalog 未被调用。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ( | ||
| archiveStates && | ||
| !archiveStates.includes(entry.query.options.archiveState ?? 'active') | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R2-16: Still standing from round 2. The ?? 'active' default for queries carrying no archiveState — duplicated verbatim in this diff's invalidateEntries, removeSession, and addSession — is pinned by no test: every query fixture in session-catalog-store.test.ts sets an explicit archiveState (including via the query() helper), and invalidateSessionLists is invoked once with only explicit-state entries loaded. Production genuinely loads unfiltered queries (App.tsx loadSessionCatalogOnce with { pageSize: 200 }, no archiveState). Probe: flipping all three defaults to ?? 'archived' fails a probe loading an archiveState-less query while all existing store tests stay green. In 'invalidates only catalog entries matching the requested archive states', subscribe a third entry whose options omit archiveState and assert it is marked stale and refetched when ['active'] is requested.
中文说明
第二轮遗留（R2-16），仍然存在。针对不带 archiveState 查询的 ?? 'active' 默认值——在本 diff 的 invalidateEntries、removeSession、addSession 中逐字重复了三遍——没有任何测试钉住：session-catalog-store.test.ts 的每个查询 fixture 都显式设置了 archiveState（包括经 query() helper），且 invalidateSessionLists 只被调用一次、当时只加载了显式状态条目。生产中确实会加载无过滤查询（App.tsx 的 loadSessionCatalogOnce 只带 { pageSize: 200 }，无 archiveState）。探针：把三处默认值都翻转为 ?? 'archived'，一个加载无 archiveState 查询的探针即失败，而全部既有 store 测试仍绿。请在 'invalidates only catalog entries matching the requested archive states' 中订阅第三个 options 省略 archiveState 的条目，并断言请求 ['active'] 时它同样被标记陈旧并刷新。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| removeSession( | ||
| workspaceCwd: string, | ||
| sessionId: string, | ||
| options: { | ||
| archiveStates?: ReadonlyArray<DaemonSessionArchiveState>; | ||
| } = {}, | ||
| ): void { |
There was a problem hiding this comment.
[Suggestion] R2-17: Still standing from round 2. removeSession deliberately does NOT skip group: 'pinned' pages (unlike addSession's documented server-owned gate), and that non-skip is load-bearing: archiving a pinned session must drop it from the cached active pinned page — this method is the only mechanism clearing it, and the path is reachable today (pin an active session, then archive it from the pinned-section row's menu; the sidebar mounts the pinned query at archiveState: 'active', group: 'pinned'). No test anywhere exercises removeSession against a pinned entry: the sole store test loads group-less queries, and every sidebar test mocks the controller. Probe: adding addSession's pinned-skip here keeps all store and sidebar tests green while the archived row lingers in the cached pinned page; a store test loading a group: 'pinned' entry alongside the active entry catches it. Worth pinning before a "symmetry" refactor does the damage.
中文说明
第二轮遗留（R2-17），仍然存在。removeSession 刻意不跳过 group: 'pinned' 页（不同于 addSession 文档化的服务端所有门控），而这个"不跳过"是承重的：归档一条置顶会话必须把它从缓存的活跃置顶页中删除——本方法是清除它的唯一机制，且该路径今天可达（置顶一条活跃会话，然后从置顶区行的菜单归档；侧边栏以 archiveState: 'active', group: 'pinned' 挂载置顶查询）。没有任何测试让 removeSession 作用于 pinned 条目：唯一的 store 测试只加载无 group 查询，所有侧边栏测试都 mock 了 controller。探针：在此处加上 addSession 的 pinned 跳过后，store 与侧边栏测试全绿，而被归档的行滞留在缓存置顶页；让 store 测试在活跃条目之外再加载一个 group: 'pinned' 条目即可捕获。值得在某次"对称性"重构造成损害之前钉住它。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
|
The new conflict comes from #9560 (stable pinned-section order + instant pin feedback), which landed on main after my last merge. #9560 and this PR both fix #9465, with the same store-level write-through architecture but a different trigger point on the pin side:
The archive half of this PR is fully complementary — #9560 doesn't touch archive paths, so the archive write-through, the batch not-found guards, the targeted invalidation, and the workspace-scoped reload tokens all remain valuable and conflict-free. The pin half is a different story: keeping both would mean two near-duplicate store primitives ( |
Should I rescope this PR to the archive half + shared infrastructure — drop the pin-side write-through and pin-section changes in favor of #9560's already-merged optimistic implementation, keep the archive/unarchive write-through, not-found guards, targeted invalidation, per-workspace reload tokens, and the R1-4 fix (which is archive-path work and compatible with #9560's pinned-section rendering). This effectively becomes the archive-focused companion PR that completes #9465. |
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
5 participants
What this PR does
This PR makes sidebar session organization actions — pin, unpin, archive, and unarchive — update the session lists the moment the daemon confirms the change, instead of waiting for a full catalog refresh to catch up.
When a pin or unpin succeeds, the new state is written directly into every cached page that holds the session, including the pinned section's dedicated query. The row therefore moves to or from the pinned section instantly. The pinned section is additionally ordered by pin time (oldest first) so toggling one row no longer reshuffles the entire section, and a client-side filter keeps a stale pinned page from briefly showing a just-unpinned session alongside its normal-list row.
When an archive or unarchive succeeds, the row is dropped from the source list and inserted into the destination list immediately. The follow-up refresh is now scoped to just the active and archived session lists rather than the entire workspace catalog, and reload signals are scoped per workspace so an archive in one workspace no longer disturbs other workspaces' sections.
Two correctness fixes ride along: a primary-scope archive RPC that reports the session missing now surfaces an error instead of silently no-opping (the previous behavior looked like success while doing nothing), and non-primary archive operations reconcile their lists even when the batch endpoint reports a per-item error, since a partial application may have landed.
Why it's needed
Issue #9465 reports that clicking Pin makes the row disappear for 2-3 seconds before it appears in the pinned section, and clicking Unpin leaves the same row visible in both the normal list and the pinned section for 3-5 seconds. Archiving similarly takes several seconds to refresh. The root cause is that these actions previously relied on a workspace-wide refetch after the RPC: the round trip plus the refresh dominated the perceived latency, and the pinned section — which renders from its own filtered query — could not see the change until its own refetch landed, producing the duplicate-row window.
Reviewer Test Plan
How to verify
Start the web shell, open the sidebar, and with at least a few sessions present:
Unit tests cover each path: pin write-through to both cached queries, unpin without carrying a stale pin timestamp, archive removal and seeding, unarchive restore, the not-found error path, and the workspace-scoped reload behavior.
Evidence (Before & After)
Before: pin → row vanishes from the normal list, pinned section shows it only after ~2-3s; unpin → row appears in the normal list but stays in the pinned section for 3-5s, so the same session is listed twice during that window; archive → several seconds before the lists reflect the new state.
After: all four actions reflect in their lists immediately upon RPC confirmation (sub-100ms in local testing); no duplicate-row window was observed.
Tested on
(Windows 本地验证：单测 167 通过、typecheck 与 lint 干净；macOS/Linux 请按 CI 结果填写)
Risk & Scope
Linked Issues
Closes #9465
中文说明
这个 PR 做了什么
本 PR 让侧边栏的会话组织操作——置顶、取消置顶、归档、取消归档——在守护进程确认变更的那一刻立即更新会话列表，而不再等待整个目录刷新追上来。
置顶或取消置顶成功后，新状态会直接写入所有持有该会话的缓存页，包括置顶区自己的专用查询，因此该行会立即移入或移出置顶区。置顶区另外改为按置顶时间排序（最早的在前），这样切换一行的置顶状态不会重排整个分区；同时增加了一层客户端过滤，防止陈旧的置顶页在短暂窗口内同时显示一条刚取消置顶的会话和它在普通列表中的行。
归档或取消归档成功后，该行立即从源列表删除并插入目标列表。后续刷新只限定在活跃与归档两个会话列表，而不是整个工作区目录；重载信号也按工作区限定，一个工作区里的归档操作不再打扰其他工作区的分区。
附带两个正确性修复：主作用域的归档 RPC 在守护进程报告会话不存在时现在会报错，而不是静默无操作（之前的行为看起来像成功但实际上什么都没做）；非主作用域的归档操作即使批量端点返回了单项错误也会对列表进行对账，因为可能已经有部分变更落盘。
为什么需要
Issue #9465 反馈：点击置顶后该行消失 2-3 秒才出现在置顶区；点击取消置顶后同一行在普通列表和置顶区同时存在 3-5 秒；归档同样要好几秒才刷新。根因是这些操作此前依赖 RPC 之后的全工作区 refetch：往返加刷新构成了感知延迟的主体，而置顶区——由它自己的过滤查询渲染——在它自己的 refetch 落地之前看不到变更，从而产生了重复行的窗口。
评审测试计划
如何验证
启动 web shell，打开侧边栏，在至少有几个会话的情况下：
单元测试覆盖了每条路径：置顶写透到两个缓存查询、取消置顶不携带陈旧的置顶时间戳、归档的移除与种入、取消归档的恢复、not-found 报错路径，以及按工作区限定的重载行为。
证据（前后对比）
修复前：置顶 → 行从普通列表消失，约 2-3 秒后才出现在置顶区；取消置顶 → 行出现在普通列表但在置顶区又停留 3-5 秒，期间同一会话被列出两次；归档 → 数秒后列表才反映新状态。
修复后：四项操作都在 RPC 确认后立即反映到列表（本地测试中低于 100ms）；未再观察到重复行窗口。
测试环境
（Windows 本地验证：单测 167 通过、typecheck 与 lint 干净；macOS/Linux 请按 CI 结果填写）
风险与范围
关联 Issue
Closes #9465