fix(web-shell): stable pinned-section order and instant pin feedback - #9560
Conversation
The sidebar's pinned section rendered rows in the daemon's activity order, so any session activity reshuffled pinned sessions, and pin/unpin gave no feedback until the updateSessionOrganization RPC plus a full catalog refetch landed. - Sort the pinned section by pinnedAt ascending (new pins append at the bottom); rows pinned before pinnedAt existed fall back to activity time so the order stays deterministic. - Apply pin/unpin optimistically in the sidebar: an optimisticPins map keyed by session identity overrides isPinned/pinnedAt for the pinned section and the unpinned list, rolls back when the RPC fails, and reconciles away once an authoritative list carries the target state (for unpins, once the row leaves every list).
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with solid evidence — linked issue #9465 reports both defects (pin/unpin takes seconds to reflect; the pinned section re-sorts by session activity instead of pin time). The PR also adds regression tests that fail on unpatched Direction: aligned. Stable pinned order and instant toggle feedback are basic UX expectations for the Web Shell sidebar, and the fix is scoped to the component that renders the section — no daemon, store, or API surface involved. Size: not applicable — no core paths touched (client component only). Production change is ~189 lines in Approach: scope feels right — ordering and optimistic toggle are two facets of the same pinned section and belong in one fix. One question worth keeping an eye on during review: ordering is fixed client-side only, so other consumers of the session list keep activity order. The PR calls this out as intentional, and it matches the issue's scope (Web Shell sidebar only), but I'll check the fallback logic for rows pinned before Risk: no elevated risk signals — none of the changed files match the high-risk paths from the revert-history analysis. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，证据充分——关联 issue #9465 报告了两个缺陷（置顶/取消置顶要等好几秒才反映；置顶区按会话活跃时间而非置顶时间排序，导致顺序不断变化）。PR 还针对这两个行为补充了在未打补丁的 方向：对齐。置顶区顺序稳定、切换即时反馈是 Web Shell 侧边栏的基本 UX 预期，修复范围也限于渲染该分区的组件——不涉及 daemon、store 或 API 层面。 规模：不适用——未触及核心路径（仅客户端组件）。生产代码改动约 189 行（ 方案：范围合理——排序与乐观切换是同一置顶分区的两个面，放在一个修复里合适。一个值得在 review 中关注的问题：排序只在客户端修复，其他会话列表消费方仍保持活跃时间排序。PR 已说明这是有意为之，且与 issue 范围（仅 Web Shell 侧边栏）一致，但我会检查引入 风险：无升级风险信号——改动文件均未命中基于 revert 历史分析的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewBefore reading the diff, my independent take was: sort the pinned section client-side by
One non-blocking edge worth a look: if another client re-pins a session while this client's optimistic unpin entry is still active, and the refresh lands showing the row pinned, the reconciliation condition (drop only when the authoritative state matches the entry's target) never fires — the entry persists and the optimistic override hides the re-pinned row until the user's next toggle or a reload. It needs two clients toggling the same session inside one RPC window, and the sequential re-pin case is covered by a test, so I'm raising it as a question rather than a blocker — e.g. reconciling unpin entries when the authoritative The optimistic-lifecycle is the heart of the change, so here it is: sequenceDiagram
participant P1 as User
participant P2 as WebShellSidebar pin toggle
participant P3 as optimisticPins state
participant P4 as Daemon organization RPC
participant P5 as Session catalog refresh
P1->>P2: click Pin or Unpin
P2->>P3: apply entry immediately (provisional pinnedAt from client clock)
P2->>P4: send isPinned toggle
alt RPC succeeds
P4-->>P5: workspace reload and refreshWorkspace
P5-->>P3: drop entry once an authoritative list matches the target state
else RPC fails
P4-->>P3: roll entry back and surface organizationFailed
end
Tests: 7 new colocated tests ( Testing evidence — the PR's own CIUnattended run: I do not build or execute PR code; the evidence below is this commit's CI, fetched via API. The unit suite ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the remaining gap: 中文说明代码审查：实现与最小方案一致——客户端按 一个非阻塞的边界场景值得关注：当本端的乐观「取消置顶」条目仍在生效时，若另一客户端重新置顶了同一会话，且刷新返回的行处于已置顶状态，收敛条件（权威状态须与条目目标一致）不会触发，该条目会一直存在并把重新置顶的行乐观地翻回未置顶，直到用户下一次切换或刷新页面。这需要两个客户端在一个 RPC 窗口内操作同一会话，且顺序场景（先收敛、后重新置顶）已有测试覆盖，因此作为问题提出而非阻塞项——例如当权威 测试：7 条新的同目录测试（命名遵循现有 测试证据：无人值守运行不构建、不执行 PR 代码，以上证据来自该 commit 的 CI（经 API 获取）。单元测试与 Web-shell 视觉采集仍在运行，表格将由 finalize workflow 在 CI 结束后原地更新；macOS/Windows 矩阵与 CLI 集成任务显示 skipped 是仓库 CI profile 按改动路径分类的结果（本 PR 仅涉及 web-shell），并非失败。 沙箱验证可补齐剩余缺口： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a focused, well-tested fix that matches the minimal approach I'd have taken; the two reservations are a narrow multi-client reconciliation edge and jsdom-only behavioral evidence, neither blocking. Reflection: this is exactly the kind of PR the gate should wave through. The problem is an observed, triaged bug (#9465, P2) in a daily-driver surface; the fix is the minimal one — client-side pin-time ordering plus an optimistic toggle local to What keeps this at 4 instead of 5: the reconciliation condition can strand an optimistic unpin entry if another client re-pins the same session inside the RPC window (detailed in my review comment — narrow, but real), and the behavioral evidence is jsdom-only since the author's host has no browser. The Verdict: approve. CI is still running on this commit, so approval is deferred until CI lands green on 中文说明置信度：4/5——聚焦且测试充分的修复，与我独立设想的最小方案一致；保留两点：一个狭窄的多客户端收敛边界场景，以及行为证据仅来自 jsdom，均不构成阻塞。 复盘：这正是应当放行的 PR。问题是已观测、已分诊的 bug（#9465，P2），发生在日常使用的界面；修复是最小方案——客户端按置顶时间排序 + 限于 未给 5 分的原因：收敛条件在极端场景下可能滞留乐观的「取消置顶」条目——另一客户端在 RPC 窗口内重新置顶同一会话时（详见审查评论中的场景描述，狭窄但真实存在）；且作者环境无浏览器，行为证据仅来自 jsdom。本 commit 上正在运行的 Web-shell 视觉采集与上文给出的 结论：通过。CI 仍在运行，待 CI 在该 commit 上全部转绿后由 finalize workflow 代发固定到该 commit 的批准；若有检查转红或 head 移动则不会批准。 — 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 |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
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.
Not explored to full depth (tool budget reached): "agent 6a": I did not execute the pin→archive/delete sequence in a running web-shell to confirm the ghost row visually; the finding above is traced from code only (hence Co…; "agent 1c": none — the full walk completed within the tool budget..
Test Plan (not a blocker): client/App.test.tsx — no such file or directory.
中文说明
未探索到全部深度（达到工具调用预算）："agent 6a"：I did not execute the pin→archive/delete sequence in a running web-shell to confirm the ghost row visually; the finding above is traced from code only (hence Co…；"agent 1c"：none — the full walk completed within the tool budget.。
Test Plan（非阻断）：client/App.test.tsx — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.14)
|
Local UI verification: PASS
Regression fix verificationThe follow-up fixes now cover the review findings around optimistic-state settlement:
Targeted verification: 9/9 pinning tests passed,
The before/after screenshots below were recaptured against the new revision after the follow-up fixes; the before arm still reproduces the original issue behavior. Pinned orderingBefore:
After:
Optimistic pinBefore:
After:
Optimistic unpinBefore:
After:
|
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review findings
Reviewed with AI assistance.
🔴 Blocker — reconciliation never clears a pin entry the server silently contradicts
File: WebShellSidebar.tsx, reconciliation useEffect (~line 1514)
The condition that removes a stale optimistic entry is current.isPinned === entry.pinned. When the pin RPC succeeds but the subsequent catalog refresh returns isPinned: false (server disagreement, timing race, or a second tab that unpinned in the refetch window), neither the rollback path (.catch) nor the reconciliation path fires. The entry stays in optimisticPins indefinitely.
Because applyOptimisticPin guards with entry.pinned === (session.isPinned === true) → true !== false → applies the overlay, every re-render forces isPinned: true back onto the session. The result is a ghost row in the pinned section and a "Unpin" button for a session the server does not consider pinned. Only a page reload clears it.
Mechanism traced end-to-end:
pin RPC success → bumpWorkspaceReload() → catalog refresh →
server returns { isPinned: false } →
reconciliation: current.isPinned(false) === entry.pinned(true) → false →
staleIdentities is empty → setOptimisticPins never called →
applyOptimisticPin overlay persists on every render
Fix direction: Record a "catalog refresh generation" (or monotonic sequence) on the entry at creation. Drop the entry once the first post-RPC refresh has settled — regardless of whether the settled state agrees with the target. This decouples "did we get a fresh answer from the server?" from "does the server agree with us?".
🟠 Major — primary regression test cannot distinguish pinnedAt sort from updatedAt sort
File: WebShellSidebar.session-pinning.test.tsx, ~line 313
The fixture aligns pinnedAt and updatedAt monotonically (both in the same direction). Mutating getPinnedSectionOrderTime to return updatedAt instead of pinnedAt — exactly the original bug — leaves all 7 tests green. The test was introduced to prevent this regression but is currently vacuous against it.
Fix: Cross the timestamps so that the session pinned first has the newest activity time, and vice versa. The two sort orderings then disagree and the expectation ['Older activity', 'Recent activity'] fails under any activity-based sort.
// session pinned FIRST, but has the NEWEST activity time
makeSession('older', { displayName: 'Older activity', isPinned: true,
pinnedAt: '2026-01-01T00:00:00.000Z', updatedAt: '2026-01-05T00:00:00.000Z' }),
// session pinned LAST, but has the OLDEST activity time
makeSession('recent', { displayName: 'Recent activity', isPinned: true,
pinnedAt: '2026-01-02T00:00:00.000Z', updatedAt: '2026-01-01T00:00:00.000Z' }),💡 Suggestion — optimistic pin's pinnedAt timestamp is unverified by tests
No test seeds an existing pinned row and then pins a second session while the RPC is in flight. Mutating new Date().toISOString() to new Date(0).toISOString() causes every new optimistic pin to sort to the top of the section (re-introducing the original reshuffle complaint), and the suite still passes 7/7.
Suggested fixture: create one already-pinned session with a past pinnedAt, pin a second session while the mock RPC hangs, then assert the new pin appears last in the pinned section.
💡 Suggestion — absence-based reconciliation fires prematurely for snapshot-only rows
The comment at ~line 1511 states: "An unpin entry is only ever created while its row is rendered from a list, so disappearance cannot race the toggle itself." This invariant is false for rows rendered from the optimistic snapshot loop (sessions not present in any of the three authoritative arrays). Clicking Unpin on such a row creates an unpin entry, but the absence rule (entry.pinned === false → reconciled = true) drops it before the unpin RPC settles. The pin-direction refetch then lands with isPinned: true, causing the session to reappear. Transient self-healing, but the comment should be corrected to avoid misleading future maintainers.
Cross-check
The prior CHANGES_REQUESTED review's findings R1-1 (ghost-row reconciliation), R1-4 (race on snapshot-only row), R1-8 (fixture mutation survival), and R1-9 (epoch-timestamp mutation) are all independently confirmed above. R1-5 (identity expression divergence) and R1-6/7 (code-quality items) are confirmed as pre-existing or low-severity. The LGTM review that was submitted after the CHANGES_REQUESTED appears to have overlooked the reconciliation gap; the blocker still stands.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
jifeng
left a comment
jifeng
left a comment
There was a problem hiding this comment.
Real-environment verification — PASS
Merge recommendation: suitable to merge within the tested scope, pending normal CI.
Exact head tested: 65db7cf8099d9caec791e31fe9412f3701da15a4
Merge-base baseline: 4b750b5b13f32c09ea7354d06b6d2ba17e1936e7
I tested production Web Shell builds against real local qwen serve processes on macOS arm64, Node.js 24.18.0, and Chromium, using isolated workspace/runtime state. Sessions were created through the real POST /session endpoint and pin state was mutated through the real PATCH /session/:id/organization endpoint. The E2E observations did not use a mocked daemon or jsdom.
| Scenario | Result |
|---|---|
| Stable pin order | The merge base preserved the daemon's reverse/newest-first response (555 → 444), while the exact PR head rendered ascending pinnedAt order (111 → 222). |
| Delayed Pin | With the real daemon process paused using SIGSTOP, the row moved into the pinned section within 200 ms, before RPC could settle. After SIGCONT, the real PATCH returned 200 and the row remained exactly once in the correct section. |
| Delayed Unpin | Under the same paused-daemon setup, the row moved back within 150 ms. After the real PATCH returned 200, the final state remained correct with no duplicate or loss. |
| Genuine failure rollback | With the isolated organization sidecar directory made unwritable, the real PATCH returned HTTP 500 / EACCES. Within 500 ms the optimistic copy was removed, exactly one enabled workspace row was restored, and the error was surfaced. After permissions were restored, the daemon remained healthy. |
| Reconciliation | Repeated delayed pin/unpin cycles produced no visible flicker, duplication, or state loss. |
Verification also passed the focused suite (9/9), Web Shell typecheck/build/bundle, and the exact-head real-daemon reruns. A clean full-repository build and bundle passed on the immediately preceding tested head; the exact head then added only the four-line RPC-settlement adjustment, which was rebuilt and rerun through the success and real-500 paths.
I found no blocking behavioral issue. One non-blocking PR-description mismatch is noted inline: the implementation's legacy fallback is deterministic session-ID ordering rather than the activity-time fallback currently described, and the test count is now 9 rather than 7.
中文版本
真实环境验证 — 通过
合并建议： 在本次测试覆盖范围内适合合并，仍以常规 CI 结果为准。
精确测试 head： 65db7cf8099d9caec791e31fe9412f3701da15a4
基线 merge-base： 4b750b5b13f32c09ea7354d06b6d2ba17e1936e7
我在 macOS arm64、Node.js 24.18.0 和 Chromium 环境中，对生产构建的 Web Shell 与真实本地 qwen serve 进程进行了测试，并隔离了 workspace/runtime 状态。会话通过真实的 POST /session 创建，固定状态通过真实的 PATCH /session/:id/organization 修改。E2E 观察未使用模拟 daemon 或 jsdom。
| 场景 | 结果 |
|---|---|
| 固定顺序稳定性 | merge-base 保留了 daemon 返回的反向/最新优先顺序（555 → 444）；PR 精确 head 按 pinnedAt 升序渲染（111 → 222）。 |
| 延迟 Pin | 真实 daemon 被 SIGSTOP 暂停时，行在 200 ms 内进入固定区，此时 RPC 不可能完成；SIGCONT 后真实 PATCH 返回 200，最终仅保留一行且位置正确。 |
| 延迟 Unpin | 同样在暂停 daemon 的条件下，行在 150 ms 内移回；真实 PATCH 返回 200 后，无重复、无丢失。 |
| 真实失败回滚 | 将隔离的 organization sidecar 目录设为不可写后，真实 PATCH 返回 HTTP 500 / EACCES；500 ms 内乐观副本被移除，恰好恢复一条可操作的 workspace 行，并展示错误。恢复权限后 daemon 仍然健康。 |
| 状态协调 | 重复延迟 pin/unpin 未出现可见闪烁、重复或状态丢失。 |
此外，聚焦测试 9/9、Web Shell typecheck/build/bundle，以及精确 head 的真实 daemon 复测均通过。紧邻的上一测试 head 已通过干净依赖下的全仓 build 和 bundle；当前精确 head 只新增了 4 行 RPC settled 时序调整，并已重新构建且覆盖成功与真实 500 路径。
未发现阻塞合并的行为问题。仅有一项非阻塞的 PR 描述不一致已在行级评论中指出：实现对旧数据的 fallback 是按 session ID 确定性排序，而当前描述写的是按活动时间 fallback；测试数量也已从 7 增至 9。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6b": running WebShellSidebar.session-pinning.test.tsx (and the activity-descending mutation of getPinnedSectionOrderTime ) to empirically confirm finding 1 — bloc….
Test Plan (not a blocker): client/App.test.tsx — no such file or directory.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/sidebar/WebShellSidebar.session-pinning.test.tsx:8 — [review] ~200 lines of sidebar mock scaffolding triplicated across three sibling test files (copies already diverged) — anchored on code unchanged sin…
中文说明
未探索到全部深度（达到工具调用预算）："agent 6b"：running WebShellSidebar.session-pinning.test.tsx (and the activity-descending mutation of getPinnedSectionOrderTime ) to empirically confirm finding 1 — bloc…。
Test Plan（非阻断）：client/App.test.tsx — no such file or directory。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
- Snapshot catalog pages at RPC settle time (ref), not from the click-time closure, so in-flight page churn does not drop a settled optimistic entry. - Trim the reconciliation effect deps to values its body reads. - Share getIdentityForSession across optimistic-pin read/write sites. - Cross pin-time vs activity keys in the ordering fixture; add coverage for missing/invalid pinnedAt ordering, failed unpin rollback, and mid-RPC page churn.
Closeout — round-3 review pass (head 92752cc)Addressed all 23 unresolved review threads; one push, one commit on top of 65db7cf. Fixed in 92752cc
Already fixed at head (round-2 commits f6a2f82, 65db7cf — replied with evidence)
Needs maintainer decision (left unresolved)
Verification note: jifeng's real-environment A/B, SIGSTOP-daemon, HTTP 500, and exact-head settlement verifications (threads in this PR) cover the timing paths; the timing-race fixes here are not screenshot-capturable, so no UI re-verification was run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): client/App.test.tsx — no such file or directory.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/sidebar/WebShellSidebar.tsx:1625 — [review] the optimistic snapshot-render loop (the only instant-pin path for rows no loaded page carries) is never exercised — deleting it survives the suite
中文说明
Test Plan（非阻断）：client/App.test.tsx — no such file or directory。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): client/App.test.tsx — no such file or directory.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/sidebar/WebShellSidebar.tsx:1637 — [probe] the pinned-section merge lets the all-sessions page overwrite the authoritative pinned-page row for the same identitypackages/web-shell/client/components/sidebar/WebShellSidebar.tsx:5166 — [probe] both mapSession={applyOptimisticPin} integration sites are unverified — removing either prop ships green
中文说明
Test Plan（非阻断）：client/App.test.tsx — no such file or directory。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): client/App.test.tsx — no such file or directory.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/sidebar/WebShellSidebar.tsx:1736 — [review] successful-then-contradicted toggles are dropped silently — no log or onError on the reconciliation drop pathpackages/web-shell/client/components/sidebar/WebShellSidebar.session-pinning.test.tsx:594 — [probe] 'uses the settle-time catalog baseline after an in-flight page change' does not discriminate — the click-time-closure mutant passes itpackages/web-shell/client/components/sidebar/WebShellSidebar.tsx:1769 — [probe] the optimistic snapshot-render loop is load-bearing but untested — deleting it ships green
中文说明
Test Plan（非阻断）：client/App.test.tsx — no such file or directory。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
…ed evidence mechanism The previous wording claimed churn (patchSession, live-state ticks) is never evidence, but the evidence collection only compares page references, so churned pages do enter the evidence set; only the downstream outcome checks neutralize them, and asymmetrically. Describe the implemented mechanism, including the residual churn-drop entrance tracked in the R5-1 review thread.
|
Closeout pass on the 21:30Z review round (head was 53ef6af):
Verification: |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): client/App.test.tsx — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/sidebar/WebShellSidebar.session-pinning.test.tsx:610 — [probe] 'uses the settle-time catalog baseline...' does not discriminate settle-time from click-time capturepackages/web-shell/client/components/sidebar/WebShellSidebar.tsx:1778 — [probe] snapshot-render loop, secondary-workspace slots and all-page merge arm never executed by any testpackages/web-shell/client/components/sidebar/WebShellSidebar.session-pinning.test.tsx:118 — [probe] post-toggle catalog refresh trigger (invalidateWorkspace) never assertedpackages/web-shell/client/components/sidebar/WebShellSidebar.session-pinning.test.tsx:305 — [probe] multi-entry reconciliation paths unfalsifiable — no test holds two concurrent optimistic entriespackages/web-shell/client/components/sidebar/WebShellSidebar.session-pinning.test.tsx:747 — [probe] key-membership guard (baselinePages.has(slot.key)) has no falsifying testpackages/web-shell/client/components/sidebar/WebShellSidebar.session-pinning.test.tsx:642 — [probe] neither outcome guard of the reconciliation effect (success arm, still-pinned veto) has a falsifying testpackages/web-shell/client/components/sidebar/WebShellSidebar.tsx:1771 — [probe] stale all-page row surfaces as a ghost pinned session via the merge arm when the pinned page refreshed but the all-page has notpackages/web-shell/client/components/sidebar/WebShellSidebar.session-pinning.test.tsx:398 — [probe] handleTogglePin's busy set/clear lifecycle has no falsifying testpackages/web-shell/client/components/sidebar/WebShellSidebar.tsx:1773 — [probe] all-page merge overwrites the fresher pinned-filter row for identities carried by both pagespackages/web-shell/client/components/sidebar/WorkspaceSection.tsx:433 — [probe] WorkspaceSection's mapSession application (and both wirings) is unfalsifiable — no test routes a session through it
中文说明
Test Plan（非阻断）：client/App.test.tsx — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 10 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| corroborated = | ||
| success || | ||
| (pinnedHomeRefreshed && | ||
| !slots.some( | ||
| (slot) => | ||
| slot.page !== undefined && showsIdentityPinned(slot.page), | ||
| )); |
There was a problem hiding this comment.
[Critical] R5-1 (still stands; re-posted under its original id): the churn-drop entrances of the optimistic-pin reconciliation remain open at this head — the round-6 delta (53ef6afe0e..b401fddfff) is verified comment-only. The freshness model is still keyed on page-reference identity: the pin-direction absence arm here counts a churned pinned-group page as refreshed, so a settled, RPC-successful pin is dropped before the toggle's own invalidateWorkspace refetch lands. Five sibling entrances of the same model stand with it: the re-pin mask (an unpin entry becomes immortal once every refreshed carrier page shows the row pinned again), zero-carrier unpin (trackedCarriers.length > 0 unreachable for snapshot-only rows), staggered-settlement duplicate (the success arm drops on the first carrier landing), untracked live-section pages (organizationEnabled={false} query keys no slot tracks), and baseline keys leaving the tracked set (entries can never corroborate again — phantom pinned rows). Your closeout already concedes entrance 1 — "verified real at this head… left unresolved as human-gated pending a maintainer decision" — and this code's comment now documents the residual.
Concrete trigger: a workspace with two pinned sessions; the user pins session X, the RPC succeeds, and before the refetch lands the other pinned session admits a prompt or is renamed — patchSession/applyLiveState recreates the pinned-group page reference without touching isPinned. The churned page counts as evidence: success is false, pinnedHomeRefreshed is true, no loaded page shows X pinned, so the entry drops — X vanishes from the Pinned section until the refetch lands, silently and indefinitely if that refetch fails (console.warn-only, 30s error backoff). That reintroduces exactly the delay #9465 asked this PR to remove, and the suite stays green throughout because it never replays this shape (its churn test churns only the row-carrying all-page).
Witness (probe at this head, real component in jsdom):
PROBE-A pinnedListTitles after churn: ["Other pin"] <- expected [ 'Other pin', 'Plain session' ]
with the absence arm disabled (corroborated = success): PROBE-A -> ["Other pin","Plain session"]
Class-closing fix direction (needs a maintainer decision on this PR's scope): apply the toggle to the catalog store as a controller operation beside renamed()/promptAdmitted (patchSession + invalidateWorkspace), so the overlay's lifetime is owned by the store's invalidate/refresh lifecycle and every loaded page is covered; or expose a per-query refresh generation advancing only on authoritative fetch commits and key evidence on that instead of page-reference identity. This is the sixth round the same family has recurred; per-entrance patching on page-reference identity has proven unbounded.
中文说明
[Critical] R5-1（仍然成立；以原编号重新发布）：乐观置顶收敛机制的 churn-drop 入口在当前 head 上仍然敞开——第 6 轮增量（53ef6afe0e..b401fddfff）已核实为纯注释改动。新鲜度模型仍以页面引用身份为键：此处置顶方向的缺失分支会把被抖动的置顶组页面计为已刷新，于是已落定、RPC 已成功的置顶会在本切换自身的 invalidateWorkspace 刷新落地之前被删除。同一模型下的五个孪生入口同样仍然存在：重新置顶遮蔽（一旦所有刷新后的承载页再次显示该行为置顶，取消置顶条目永久滞留）、零承载取消置顶（快照独有的行永远无法满足 trackedCarriers.length > 0）、交错落定重复行（success 分支在第一个承载页落地时即删除条目）、未跟踪的 Live 分区页（organizationEnabled={false} 的查询键不在任何槽位中）、基线键离开被跟踪集合（条目从此永远无法收敛——幽灵置顶行）。作者的收尾评论已确认入口 1——"在当前 head 上独立复现……因需维护者决策而暂不解决"——本处代码注释现在也记录了这一残留。
具体触发：在已有两个置顶会话的工作区，用户置顶会话 X，RPC 成功；刷新落地之前另一个置顶会话受理了提示词或被重命名——patchSession/applyLiveState 重建置顶组页面引用但不触碰 isPinned。被抖动的页面计为证据：success 为假、pinnedHomeRefreshed 为真、没有任何已加载页面显示 X 已置顶，于是条目被删除——X 从置顶区消失直到刷新落地；若该刷新失败则静默且无限期存在（仅 console.warn，30 秒错误退避）。这恰好重新引入了 #9465 要求本 PR 消除的延迟；测试套件从未复现该形状（其抖动测试只抖动承载行的全量页），因此全程保持绿色。
见证（当前 head 上的探针，jsdom 中运行真实组件）：
PROBE-A 抖动后 pinnedListTitles: ["Other pin"] <- 期望 [ 'Other pin', 'Plain session' ]
禁用缺失分支（corroborated = success）后: PROBE-A -> ["Other pin","Plain session"]
类别级修复方向（需维护者就本 PR 范围作出决策）：把切换作为控制器操作应用到目录存储——置于 renamed()/promptAdmitted 旁（patchSession + invalidateWorkspace），让覆盖层的生命周期由 store 自身的失效/刷新生命周期管理，覆盖所有已加载页面；或由 store 暴露仅在权威拉取提交时推进的按查询刷新代际，证据按该代际而非页面引用身份键控。这是同一缺陷家族第六轮出现；在页面引用身份上逐入口打补丁已被证明是无界的。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Same disposition as the R5-1 lineage reply above — no code change; this needs a maintainer decision on this PR's scope.
Both fix directions are store-surface changes, verified at this head: the controller operation would sit beside patchSession (session-catalog-store.ts:469) / applyLiveState (:497) in SessionCatalogStore, and the per-query refresh-generation direction requires a new store-exposed revision API. This PR's declared scope excludes both — PR body: "No daemon, store, or API changes — the fix is local to WebShellSidebar".
This is the sixth round of the same family (R1-1 → R2-1 → R3-1 → R4-1 → R5-1 → this); the identical store move was declined twice on scope grounds (round-4 R2-5 twins; round-9 R5-1 held unresolved as human-gated), and the reviewer's own analysis shows per-entrance patching on page-reference identity is exhausted. Entrance 1 (churn-drop) was independently reproduced at this head in the prior round and is documented in-code (b401fdd) as the known residual.
Decision needed from a maintainer: expand scope to the catalog store, or land as-is with the documented residual. Leaving this thread deliberately unresolved.
There was a problem hiding this comment.
Independent exact-head verification confirms this blocker at c9a66105894795273c65a45ebe59b3535c230d9c. I reproduced it with the real component after all shipped pin tests (16/16), the full sidebar suite (182/182), typecheck and changed-file ESLint passed: start with one existing pin, optimistically pin another row, settle the successful RPC, then apply patchSession-style churn only to the existing pinned row. Expected pinned titles were [Existing pin renamed, Plain session]; actual output was [Existing pin renamed]. The successful pin disappears until its own catalog refetch lands and can remain wrong through refetch failure/backoff. This independently confirms that page-reference churn is not authoritative refresh evidence; the store-owned operation or fetch-generation direction is required before merge.
中文独立复验
已在精确 head c9a66105894795273c65a45ebe59b3535c230d9c 独立确认该阻塞项。置顶专项测试 16/16、完整侧栏测试 182/182、类型检查及改动文件 ESLint 均通过后，使用真实组件复现：先保留一个既有 pin，再乐观置顶另一会话并让 RPC 成功，随后仅对既有 pinned row 模拟 patchSession churn。期望标题为 [Existing pin renamed, Plain session]，实际为 [Existing pin renamed]。成功置顶会在自身 catalog refetch 到达前消失；若 refetch 失败或退避，错误状态会持续。这独立证明页面引用 churn 不能作为权威刷新证据，合并前需要 store 管理的操作或 fetch generation 方案。
There was a problem hiding this comment.
Thanks for the independent reproduction — it matches our probes at the same head.
Status update: main is now merged and the conflict resolved (new head ebc6d18830, sidebar suite 13 files / 188 tests green, typecheck clean). The merge does not address this blocker: the store-owned / fetch-generation reconciliation rework is still outstanding, and this thread stays open for it. Will re-request review once that lands.
There was a problem hiding this comment.
The store-owned direction is now implemented at 1135e7c2a8.
SessionCatalogStore.applySessionPinToggle writes the toggle into every loaded page of the workspace (pinned-view pages gain or lose the row; every other page patches it in place), so the optimistic state's lifetime is owned by the store instead of component-side page-reference tracking:
- Churn can no longer drop a settled pin:
patchSession/applyLiveStaterecreate page references without touching pin state, and the toggled row stays in the page data. Covered by the new component-level R5-1 regression test — one existing pin, optimistically pin another row, settle the successful RPC, then churn only the existing pinned row — which keeps[Existing pin renamed, Plain session], the exact shape of the reproduction above. Store-level churn coverage added as well. - The baseline/evidence reconciliation model is gone: the component overlay only renders rows no loaded page carries yet, and drops an entry once any loaded page does (the page's own state renders then, whether store-applied or authoritative). RPC failure rolls back via the same operation with the opposite target.
- The sibling entrances close with it: the re-pin mask, zero-carrier unpin, staggered-settlement duplicate, untracked live-section pages, and baseline-keys-leaving-the-tracked-set all depended on the page-reference freshness model, which no longer exists.
Verified at this head: pin suite 17/17; sidebar + session-catalog + App 792/792 across 17 files; web-shell typecheck, ESLint and Prettier clean.
|
Closeout for the round-10 CR on Fixed ( Deliberately unresolved (needs maintainer decision) — the two recurring reconciliation Criticals (this thread family's 5th/6th round, R1-1 → R2-1 → R3-1 → R4-1 → R5-1):
Evidence replies on both threads. One non-force push |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 5": executing the suite ( npx vitest run client/components/sidebar/WebShellSidebar.session-pinning.test.tsx ) was blocked by the shared review worktree — repeated f…; "agent 6b": run the pinning test suite to completion (blocked by the worktree's node_modules/dist bootstrap described above); "agent 6b": confirm unit-test CI status at head commit c9a6610589.
Test Plan (not a blocker): client/App.test.tsx — no such file or directory.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/sidebar/WebShellSidebar.session-pinning.test.tsx:302 — [probe] the load-bearing omission of vi.restoreAllMocks() (present in all 26 sibling suites) is unexplained — re-adding it silently re-opens R6-1's …
[Critical] R7-1: The optimistic-pin reconciliation freshness model is keyed on page-reference identity, and that surface is unbounded — six rounds of this review (R1-1 → R2-1 → R3-1 → R4-1 → the round-5 six-entrance finding → R5-1) each patched named entrances and produced new ones; round 5 verified the design space is exhausted (drop-on-success, drop-on-contradiction, churn-survival and staggered-survival cannot all hold on reference identity — its toy fixes flip some probes but break this PR's own reconciliation test). This supersedes R5-1 and the whole lineage; per-entrance patching stops here. The round-7 delta is test-only, so every entrance stands at this head. Representative trigger (probe at b401fdd; WebShellSidebar.tsx is byte-identical at the reviewed head): pin session X in a workspace with another pinned session; the RPC succeeds; before the invalidateWorkspace refetch lands the other session admits a prompt — patchSession/applyLiveState recreates the pinned-group page reference without touching isPinned; the churned page counts as evidence, the absence arm drops the settled entry, and X vanishes from the Pinned section until the refetch lands — silently and indefinitely if it fails (console.warn-only, 30s backoff). Witness: PROBE-A pinnedListTitles after churn: ['Other pin'] (expected ['Other pin', 'Plain session']); with the absence arm disabled: ['Other pin', 'Plain session']. Close it structurally, not entrance by entrance: apply the toggle to the catalog store as a controller operation beside renamed()/promptAdmitted (patchSession + invalidateWorkspace), or expose a per-query refresh generation that advances only on authoritative fetch commits and key evidence on that instead of page-reference identity. Both directions change the store surface, which this PR's declared scope excludes — that is the maintainer scope decision the author has been holding on since round 6, and until it lands (or the reconciliation is removed), the blocker stands.
中文说明
未探索到全部深度（达到工具调用预算）："agent 5"：executing the suite ( npx vitest run client/components/sidebar/WebShellSidebar.session-pinning.test.tsx ) was blocked by the shared review worktree — repeated f…；"agent 6b"：run the pinning test suite to completion (blocked by the worktree's node_modules/dist bootstrap described above)；"agent 6b"：confirm unit-test CI status at head commit c9a6610589。
Test Plan（非阻断）：client/App.test.tsx — no such file or directory。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
[Critical] R7-1: The optimistic-pin reconciliation freshness model is keyed on page-reference identity, and that surface is unbounded — six rounds of this review (R1-1 → R2-1 → R3-1 → R4-1 → the round-5 six-entrance finding → R5-1) each patched named entrances and produced new ones; round 5 verified the design space is exhausted (drop-on-success, drop-on-contradiction, churn-survival and staggered-survival cannot all hold on reference identity — its toy fixes flip some probes but break this PR's own reconciliation test). This supersedes R5-1 and the whole lineage; per-entrance patching stops here. The round-7 delta is test-only, so every entrance stands at this head. Representative trigger (probe at b401fdd; WebShellSidebar.tsx is byte-identical at the reviewed head): pin session X in a workspace with another pinned session; the RPC succeeds; before the invalidateWorkspace refetch lands the other session admits a prompt — patchSession/applyLiveState recreates the pinned-group page reference without touching isPinned; the churned page counts as evidence, the absence arm drops the settled entry, and X vanishes from the Pinned section until the refetch lands — silently and indefinitely if it fails (console.warn-only, 30s backoff). Witness: PROBE-A pinnedListTitles after churn: ['Other pin'] (expected ['Other pin', 'Plain session']); with the absence arm disabled: ['Other pin', 'Plain session']. Close it structurally, not entrance by entrance: apply the toggle to the catalog store as a controller operation beside renamed()/promptAdmitted (patchSession + invalidateWorkspace), or expose a per-query refresh generation that advances only on authoritative fetch commits and key evidence on that instead of page-reference identity. Both directions change the store surface, which this PR's declared scope excludes — that is the maintainer scope decision the author has been holding on since round 6, and until it lands (or the reconciliation is removed), the blocker stands.
— qwen3.8-max via Qwen Code /review (v0.21.15)
jifeng
left a comment
jifeng
left a comment
There was a problem hiding this comment.
Local validation report — request changes
Validated exact head c9a66105894795273c65a45ebe59b3535c230d9c in an isolated worktree with a clean install/build and Chromium browser runtime.
Evidence
- Shipped pin suite: 16/16 passed.
- Entire Web Shell sidebar suite: 182/182 passed across 12 files.
- Web Shell typecheck and ESLint on the three changed files: passed.
- Real Chromium + mock daemon: with the organization PATCH deliberately held open, the clicked row moved immediately into Pinned, appended after the existing pin by
pinnedAt, exposed the disabled pending Unpin control, remained unique, and reconciled after the successful response.
Merge blocker
An exact-head probe independently reproduced the unresolved reconciliation race in the existing R5-1 inline thread: churn of an unrelated pinned row can make a successful optimistic pin disappear before the toggle's authoritative refetch lands. The expected pinned titles were [Existing pin renamed, Plain session]; the actual output was [Existing pin renamed]. If the refetch fails or enters backoff, the wrong state persists. Page-reference identity is not an authoritative freshness signal.
GitHub also currently reports the PR as conflicting with its base (mergeable_state: dirty). Recommendation: do not merge until the store-level freshness/reconciliation mechanism is implemented and the branch is rebased. I added independent reproduction evidence to the existing line-level thread.
中文验证报告
本地验证报告——当前不建议合并
已在隔离 worktree 中对精确 head c9a66105894795273c65a45ebe59b3535c230d9c 完成干净安装/构建及 Chromium 浏览器验证。
验证证据
- 置顶专项测试：16/16 通过。
- Web Shell 侧栏完整测试：12 个文件、182/182 通过。
- Web Shell 类型检查以及三个改动文件的 ESLint：通过。
- 真实 Chromium + mock daemon：人为挂起 organization PATCH 后，会话会立即进入置顶区、按
pinnedAt追加到既有置顶项之后、显示禁用的 pending Unpin 控件、保持唯一，并在成功响应后完成协调。
合并阻塞项
精确 head 的专项 probe 独立复现了既有 R5-1 行级线程中的协调竞态：另一条 pinned row 的无关 churn 会让已经成功的乐观置顶在本次 toggle 的权威 refetch 到达前消失。期望置顶标题为 [Existing pin renamed, Plain session]，实际为 [Existing pin renamed]。若 refetch 失败或进入退避，错误状态会持续。页面引用身份不能作为权威新鲜度信号。
GitHub 当前还报告该 PR 与 base 冲突（mergeable_state: dirty）。建议先实现 store 级 freshness/协调机制并完成 rebase，再考虑合并。我已在既有行级线程补充本次独立复现证据。
jifeng
left a comment
jifeng
left a comment
There was a problem hiding this comment.
Local validation report — request changes
Validated exact head c9a66105894795273c65a45ebe59b3535c230d9c in an isolated worktree with a clean install/build and Chromium browser runtime.
Evidence
- Shipped pin suite: 16/16 passed.
- Entire Web Shell sidebar suite: 182/182 passed across 12 files.
- Web Shell typecheck and ESLint on the three changed files: passed.
- Real Chromium + mock daemon: with the organization PATCH deliberately held open, the clicked row moved immediately into Pinned, appended after the existing pin by
pinnedAt, exposed the disabled pending Unpin control, remained unique, and reconciled after the successful response.
Merge blocker
An exact-head probe independently reproduced the unresolved reconciliation race in the existing R5-1 inline thread: churn of an unrelated pinned row can make a successful optimistic pin disappear before the toggle's authoritative refetch lands. The expected pinned titles were [Existing pin renamed, Plain session]; the actual output was [Existing pin renamed]. If the refetch fails or enters backoff, the wrong state persists. Page-reference identity is not an authoritative freshness signal.
GitHub also currently reports the PR as conflicting with its base (mergeable_state: dirty). Recommendation: do not merge until the store-level freshness/reconciliation mechanism is implemented and the branch is rebased. I added independent reproduction evidence to the existing line-level thread.
中文验证报告
本地验证报告——当前不建议合并
已在隔离 worktree 中对精确 head c9a66105894795273c65a45ebe59b3535c230d9c 完成干净安装/构建及 Chromium 浏览器验证。
验证证据
- 置顶专项测试：16/16 通过。
- Web Shell 侧栏完整测试：12 个文件、182/182 通过。
- Web Shell 类型检查以及三个改动文件的 ESLint：通过。
- 真实 Chromium + mock daemon：人为挂起 organization PATCH 后，会话会立即进入置顶区、按
pinnedAt追加到既有置顶项之后、显示禁用的 pending Unpin 控件、保持唯一，并在成功响应后完成协调。
合并阻塞项
精确 head 的专项 probe 独立复现了既有 R5-1 行级线程中的协调竞态：另一条 pinned row 的无关 churn 会让已经成功的乐观置顶在本次 toggle 的权威 refetch 到达前消失。期望置顶标题为 [Existing pin renamed, Plain session]，实际为 [Existing pin renamed]。若 refetch 失败或进入退避，错误状态会持续。页面引用身份不能作为权威新鲜度信号。
GitHub 当前还报告该 PR 与 base 冲突（mergeable_state: dirty）。建议先实现 store 级 freshness/协调机制并完成 rebase，再考虑合并。我已在既有行级线程补充本次独立复现证据。
jifeng
left a comment
jifeng
left a comment
There was a problem hiding this comment.
Local validation report — request changes
Validated exact head c9a66105894795273c65a45ebe59b3535c230d9c in an isolated worktree with a clean install/build and Chromium browser runtime.
Evidence
- Shipped pin suite: 16/16 passed.
- Entire Web Shell sidebar suite: 182/182 passed across 12 files.
- Web Shell typecheck and ESLint on the three changed files: passed.
- Real Chromium + mock daemon: with the organization PATCH deliberately held open, the clicked row moved immediately into Pinned, appended after the existing pin by
pinnedAt, exposed the disabled pending Unpin control, remained unique, and reconciled after the successful response.
Merge blocker
An exact-head probe independently reproduced the unresolved reconciliation race in the existing R5-1 inline thread: churn of an unrelated pinned row can make a successful optimistic pin disappear before the toggle's authoritative refetch lands. The expected pinned titles were [Existing pin renamed, Plain session]; the actual output was [Existing pin renamed]. If the refetch fails or enters backoff, the wrong state persists. Page-reference identity is not an authoritative freshness signal.
GitHub also currently reports the PR as conflicting with its base (mergeable_state: dirty). Recommendation: do not merge until the store-level freshness/reconciliation mechanism is implemented and the branch is rebased. I added independent reproduction evidence to the existing line-level thread.
中文验证报告
本地验证报告——当前不建议合并
已在隔离 worktree 中对精确 head c9a66105894795273c65a45ebe59b3535c230d9c 完成干净安装/构建及 Chromium 浏览器验证。
验证证据
- 置顶专项测试：16/16 通过。
- Web Shell 侧栏完整测试：12 个文件、182/182 通过。
- Web Shell 类型检查以及三个改动文件的 ESLint：通过。
- 真实 Chromium + mock daemon：人为挂起 organization PATCH 后，会话会立即进入置顶区、按
pinnedAt追加到既有置顶项之后、显示禁用的 pending Unpin 控件、保持唯一，并在成功响应后完成协调。
合并阻塞项
精确 head 的专项 probe 独立复现了既有 R5-1 行级线程中的协调竞态：另一条 pinned row 的无关 churn 会让已经成功的乐观置顶在本次 toggle 的权威 refetch 到达前消失。期望置顶标题为 [Existing pin renamed, Plain session]，实际为 [Existing pin renamed]。若 refetch 失败或进入退避，错误状态会持续。页面引用身份不能作为权威新鲜度信号。
GitHub 当前还报告该 PR 与 base 冲突（mergeable_state: dirty）。建议先实现 store 级 freshness/协调机制并完成 rebase，再考虑合并。我已在既有行级线程补充本次独立复现证据。
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.
Conflict resolution pass — new head
|
The optimistic pin overlay reconciled against page-reference identity: churn that recreated a page without touching pin state (patchSession, live-state ticks) counted as refresh evidence, so a settled, successful pin could drop before its authoritative refetch landed, and sibling entrances stranded or masked entries. Move the toggle into the store instead: applySessionPinToggle writes the pin state into every loaded page of the workspace (pinned-view pages gain or lose the row, other pages patch in place). Local writes then churn around the toggled state without dropping it, and authoritative refetches replace it wholesale. The component overlay shrinks to rendering rows no loaded page carries yet, dropping an entry once any loaded page does. Rollback is the same operation with the opposite target. Adds the R5-1 regression test (churn of an unrelated pinned row after a successful pin) plus store-level coverage for the new operation. Verified: pin suite 17/17, sidebar + session-catalog + App 792/792, web-shell typecheck, ESLint and Prettier clean.
R5-1 store-level fix landed — new head
|
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Tier: Standard — Stable pinned-section order + optimistic pin toggle with catalog-store-owned state (R5-1 fix).
What I verified
| Area | Check | Result |
|---|---|---|
| Sort direction | comparePinnedSectionSessions: a − b ascending = oldest pinnedAt first, new pins append to bottom |
✓ |
| Fixture mutation resistance | Fixture crosses pinnedAt/updatedAt keys: activity-descending mutant returns wrong order, test fails |
✓ |
applySessionPinToggle — pin |
Appends to pinned-view page (or patches in place); patches isPinned/pinnedAt in other pages |
✓ |
applySessionPinToggle — unpin |
Removes row from pinned page; clears isPinned/pinnedAt in non-pinned pages |
✓ |
| Churn survival (R5-1) | Toggle lands in page data; patchSession and live-state ticks recreate page refs without touching pin state → toggle survives |
✓ |
| Rollback on RPC failure (both directions) | storeToggle(!targetPinned, session.pinnedAt) reverts store; rollbackOptimistic() clears overlay |
✓ |
| Reconciliation (my earlier Blocker) | OLD: current.isPinned === entry.pinned — ghost row on server disagreement. CURRENT: entry.rpcSettled && carried — any catalog page carrying the identity drops the overlay, regardless of pin state |
✓ addressed |
| Server disagreement after successful RPC | rpcSettled=true + server returns isPinned:false → carried=true → overlay dropped → server truth wins |
✓ |
| Optimistic pin appends below existing | Existing pin has past pinnedAt; optimistic pin gets new Date().toISOString() → sorts after → confirmed by test |
✓ |
markRpcSettled / rollback race |
markRpcSettled guards if (!entry) return previous; rollback deletes entry before rpcSettled can fire on a failed RPC (rpcSucceeded stays false) |
✓ |
Cross-check against prior reviews
My earlier chiga0 comment filed two findings against an earlier head:
- 🔴 Blocker — reconciliation never clears a ghost row on server disagreement: addressed in R5-1 by switching the drop condition to
rpcSettled && carried(identity-based, pin-state-agnostic). - 🟠 Major — regression test fixture didn't cross pin/activity times: addressed by the current fixture (opposite
pinnedAt/updatedAtfor each session, activity-descending mutant now fails).
The CI bot's round-3 CHANGES_REQUESTED (on 92752cc3) was on an intermediate design that the R5-1 commit replaced wholesale; the structural concerns it raised are superseded by the store-owned approach.
One non-blocking note (deferred from round 3): the snapshot-render overlay loop in pinnedSessions (rows in no loaded page) has no test that isolates it from the store-owned path. Deleting it would survive the suite. The scenario it covers is rare; the primary instant-pin path now works entirely via store-owned pages.
CI: No automated web-shell test run visible in CI checks for the current head. Commit message reports "pin suite 17/17, sidebar + session-catalog + App 792/792, typecheck/ESLint/Prettier clean" from a local run. jifeng's real-environment PASS covered an earlier head (65db7cf8). Disclosed — the web-shell test suite does not appear to be configured in CI for this PR.
No blockers found.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Independent review at 1135e7c2 — R5-1 is genuinely closed by the store-owned rework; approving
I took the standing Critical as the whole job here, since it is the sixth round of one family and a human reproduced it independently at c9a66105. Rather than re-litigate the old model, I read the new one and tried to break it.
The fix removes the model the bug lived in, rather than patching an entrance
SessionCatalogStore.applySessionPinToggle writes the toggle into the page data of every loaded page for the workspace — pinned-group pages gain or lose the row, other pages patch isPinned in place. That is the structural difference that matters: the old defect needed page-reference identity to serve as refresh evidence, and there is no longer any evidence model to fool. patchSession / applyLiveState recreate a page reference while preserving the sessions it carries, so a churned pinned page still contains the toggled row and nothing can drop it. The five sibling entrances (re-pin mask, zero-carrier unpin, staggered-settlement duplicate, untracked live-section pages, baseline keys leaving the tracked set) all depended on that model and go with it.
The regression test at WebShellSidebar.session-pinning.test.tsx:740 replays the exact shape reported above — one existing pin, optimistically pin another row, resolve the RPC, then churn only the existing pinned row with a fresh pinned-page reference — and asserts ['Existing pin renamed', 'Plain session']. That is the reproduction, not an adjacent case.
What I tried to break, and why it holds
Duplicate rows across pinned pages. applySessionPinToggle appends to every loaded pinned page that lacks the row, which would duplicate a row when several pinned pages are loaded. It does not surface: pinnedSessions collapses through a byId Map keyed on getIdentityForSession before rendering.
Order flicker — the PR's own headline claim. The store appends the new row to the end, and comparePinnedSectionSessions sorts by pinnedAt ascending with a sessionId tie-break, so ordering is computed in the component and does not depend on append position. The risk was getPinnedSectionOrderTime returning 0 for a missing pinnedAt, which would sort an optimistic pin to the top and then move it to the bottom once the refetch supplied a real timestamp. It does not happen: the toggle handler mints optimisticPinnedAt = new Date().toISOString() and passes it to both the overlay and storeToggle, so the optimistic row already sorts where the authoritative one will land.
Rollback fidelity. A failed unpin re-pins through storeToggle(true, session.pinnedAt) — the row's original pin time, so it returns to its original position rather than jumping to the bottom. A failed pin unpins, and the store's unpin branch deletes pinnedAt on a fresh copy. Correct in both directions.
Overlay lifetime — the phantom-pin shape. The component still keeps an optimisticPins overlay, so I checked whether an entry can outlive its truth and force a stale pin state. It cannot, for two independent reasons: applyOptimisticPin no-ops when the page already agrees (entry.pinned === (session.isPinned === true)), and the pruning effect drops any settled entry once a loaded page carries the row. Crucially pinCatalogPages spans the active/all pages too, not just pinned ones — which is what lets an unpin entry be pruned (its row is gone from pinned pages but still present, unpinned, in the active page). Had it covered only pinned pages, unpin entries would have lingered.
mapSession is not a dead switch — declared on WorkspaceSection, read in visibleSessions, and set at two call sites with applyOptimisticPin, which is what makes a pinned row leave the workspace section immediately under excludePinned.
No new Critical or Suggestion from my pass. House style is clean (no any, no cross-package relative imports, tests collocated).
One procedural note
reviewDecision is still CHANGES_REQUESTED because the blocking review sits at c9a66105 — three commits before this fix. Verified against this head, the blocker it named no longer reproduces, so that review needs a re-review or dismissal to unblock the merge; nothing in the code is holding it. CI is green apart from review-pr, still running.
Not verified by me: the suites did not run locally (no node_modules in my worktree), so the 792/792 figure is the author's and CI's, not mine — my evidence is the reconciliation logic read end to end at this head plus the regression test's shape. I also did not exercise the UI in a browser.
|
Released in v0.22.2. |
5 participants
What this PR does
This PR changes the Web Shell sidebar's pinned-sessions section in two ways. First, the pinned section now keeps a stable order based on when each session was pinned: newly pinned sessions are appended to the bottom, and existing pinned sessions stay in place regardless of session activity. Rows with a missing or invalid
pinnedAtget sort key0and are ordered deterministically by session ID among themselves — never by activity time. (This branch is defensive only: pinning andpinnedAtshipped atomically andviewOrganizationderivesisPinnedfrompinnedAtpresence, so current daemons cannot emit a pinned row withoutpinnedAt.) Second, pinning and unpinning now gives immediate feedback: the toggle is applied optimistically in the sidebar while the daemonupdateSessionOrganizationRPC is in flight, rolled back automatically if the RPC fails, and reconciled away once the authoritative catalog reflects the target state. No daemon or API changes. The optimistic toggle is owned by the session catalog store (SessionCatalogStore.applySessionPinTogglewrites the pin state into every loaded page of the workspace), so the pinned section, the primary session list, and per-workspaceWorkspaceSectionlists all update instantly and churn of unrelated rows cannot drop the toggle; the component overlay only renders rows no loaded page carries yet. (The R5-1 review thread documents why the earlier component-local reconciliation was insufficient.)Why it's needed
Fixes two defects reported in #9465. The pinned section was rendered in the order the daemon returns it (activity time,
updatedAtdescending), andpinnedAt— although recorded by the session-organization service and present in session summaries — was never used for ordering, so any session activity reshuffled the pinned list. Separately,handleTogglePinapplied no optimistic state: the pin icon and section membership only changed after the RPC resolved and a full sidebar refetch landed, which takes several seconds in workspaces with many sessions.Reviewer Test Plan
How to verify
Automated:
cd packages/web-shell && npx vitest run --config vitest.config.ts client/components/sidebar/WebShellSidebar.session-pinning.test.tsx— 12 tests, all passing. They render the realWebShellSidebar(mocked daemon hooks, same harness as the existing sidebar tests) and cover: ordering by pin time instead of activity (fixture crosses pin-time and activity keys so an activity-descending mutant — the exact regression direction from #9465 — fails); deterministic sessionId ordering for pins with a missing or invalidpinnedAt; instant optimistic pin and unpin while the RPC is pending; rollback on RPC failure in both directions (pin and unpin); the reconciliation guard keeping in-flight entries alive across unrelated catalog page churn; optimistic pins appending below existing pins; no duplicate row when the authoritative pinned page lands; and reconciliation of an unpin whose row leaves every list (a stale entry must never hide a later re-pin). Both reported defects were first captured as failing tests against unpatchedmain(activity order rendered instead of pin order; pinned section stayed empty while the RPC was pending) and turned green with this change. Wider regression:client/components/sidebar+client/session-catalog250 tests andclient/App.test.tsx453 tests all pass;npm run typecheck, ESLint, and Prettier are clean. Manual: in Web Shell, pin two sessions, produce activity on the first-pinned one, and confirm the order does not change; click Pin/Unpin and confirm the section updates instantly, and that a failed toggle (e.g. stopped daemon) restores the previous state.Evidence (Before & After)
Tested on
Environment (optional)
Unit/component tests only (
vitest+ jsdom) on Linux; no live Web Shell instance was exercised on this host.Risk & Scope
useScopedSessions) reflect the toggle only after the existing background catalog invalidation lands. Optimistic pin timestamps use the client clock until the daemon's authoritativepinnedAtlands. Rollback on RPC failure is the same store operation with the opposite target.filteredSessionsmapping.Linked Issues
Fixes #9465
中文说明
这个 PR 做了什么
本 PR 从两方面修改 Web Shell 侧边栏的置顶会话区。第一，置顶区改为按「置顶时间」保持稳定排序：新置顶的会话追加到底部，已置顶的会话无论其活动如何都保持原位。缺失或非法
pinnedAt的行排序键为0，彼此之间按 session ID 确定性排序——绝不按活跃时间排序。（该分支仅为防御性：置顶与pinnedAt同批交付，且viewOrganization由pinnedAt是否存在推导isPinned，当前 daemon 不可能产生没有pinnedAt的置顶行。）第二，置顶/取消置顶现在立即反馈：RPC（daemonupdateSessionOrganization）在途期间侧边栏先乐观应用该切换，RPC 失败时自动回滚，权威目录刷新到位后自动收敛。不涉及 daemon 或 API 改动。乐观置顶切换由 session catalog store 持有（SessionCatalogStore.applySessionPinToggle把置顶状态写入该工作区所有已加载分页），置顶区、主会话列表与各工作区WorkspaceSection列表均立即更新，且无关行的 churn 不会打丢该切换；组件侧覆盖层仅负责渲染尚无任何已加载分页携带的行。（R5-1 评审线程记录了为什么早先组件本地的 reconciliation 不够。）为什么需要
修复 #9465 报告的两个缺陷。置顶区此前按 daemon 返回顺序（活跃时间，
updatedAt降序）渲染，而pinnedAt虽然由 session-organization 服务记录并随会话摘要返回，却从未用于排序，因此任何会话活动都会打乱置顶区顺序。另外，handleTogglePin不做任何乐观更新：置顶图标与分区归属要等 RPC 完成并完成一次整页 refetch 后才变化，会话多的工作区要等好几秒。评审测试计划
如何验证
自动化：
cd packages/web-shell && npx vitest run --config vitest.config.ts client/components/sidebar/WebShellSidebar.session-pinning.test.tsx，共 12 条测试全部通过。测试渲染真实的WebShellSidebar（mock daemon hooks，与现有侧边栏测试同一 harness），覆盖：按置顶时间而非活跃时间排序（fixture 交叉了置顶时间与活跃时间，活跃时间降序变异——正是 #9465 报告的回归方向——会失败）；缺失/非法pinnedAt的置顶按 session ID 确定性排序；RPC 在途时立即乐观置顶/取消置顶；置顶与取消置顶两个方向的 RPC 失败回滚；收敛守卫在无关目录分页变动期间保住未落定条目；乐观置顶追加在已有置顶之下；权威置顶页落地后不出现重复行；以及「行从所有列表消失」的取消置顶收敛（过期条目不得隐藏之后的重新置顶）。两个报告缺陷均先以失败测试在未修改的main上复现（渲染出活跃序而非置顶序；RPC 在途时置顶区为空），修复后转绿。更大范围回归：client/components/sidebar+client/session-catalog共 250 条、client/App.test.tsx453 条全部通过；npm run typecheck、ESLint、Prettier 均干净。手动验证：在 Web Shell 中置顶两个会话，让先置顶的会话产生活跃，确认顺序不变；点击置顶/取消置顶，确认分区立即更新，且失败的切换（如 daemon 已停止）会恢复原状。前后对比证据
测试环境
环境（可选）
仅单元/组件测试（
vitest+ jsdom），Linux；本机未运行真实 Web Shell 实例。风险与范围
useScopedSessions提供的对话框）要等既有的后台目录失效刷新落地后才反映切换。乐观置顶时间戳使用客户端时钟，直到 daemon 权威pinnedAt落地。RPC 失败的回滚是同一 store 操作的反向应用。filteredSessions映射外未改动。关联 Issue
Fixes #9465