fix(desktop): stabilize WorkHub projection checkpoints - #4210
Conversation
Generated-by: Codex
ARE404
left a comment
There was a problem hiding this comment.
Approving fix(desktop): stabilize WorkHub projection checkpoints at head bde188f8.
I reviewed the source fix (workhub-coordination-lifecycle.ts) and its test updates:
- The change tracks
resolvedHostId(the host the active WorkHub projection resolved to) and skips re-resolving when the same already-resolved default host emits a benign event (event.hostId === resolvedHostIdwith readiness not unavailable and not removed). This stops unnecessary re-checkpointing/flicker while still re-resolving on a genuine change: a different default host, or the current host becoming unavailable/removed. - Edge handling is sound: when
resolvedHostIdisundefined(no resolution yet) the guard is bypassed and behavior matches before; a default-host swap or host-loss still revokes and resolves. - The added
parseDesktopSessionKeyimport is validly exported fromshared/runtime-host-identity.ts;hostIdis now included inWorkHubCoordinationHostChange. testis green on this exact head (run33268495079, 16m04s); 0 unresolved review threads.
No P0–P2, and nothing warrants an inline P3.
简体中文
批准 fix(desktop): stabilize WorkHub projection checkpoints,head bde188f8。
审查源码修复(workhub-coordination-lifecycle.ts)及其测试改动:该变更记录 resolvedHostId(当前 WorkHub 投影解析到的 host),当同一个已解析的默认 host 发出良性事件时(event.hostId === resolvedHostId 且 readiness 非 unavailable、未 removed)跳过重新解析——避免无谓的重新建点/闪烁;而真正变化时仍会重解析:默认 host 换人、或当前 host 变不可用/被移除。边界处理合理:resolvedHostId 为 undefined(尚未解析)时走旧逻辑;换默认 host 或 host 丢失仍 revoke+resolve。新增的 parseDesktopSessionKey import 在 shared/runtime-host-identity.ts 有合法导出;hostId 已加入 WorkHubCoordinationHostChange。test 在 exact head 绿(run 33268495079,16m04s);0 未解决线程。无 P0–P2,也无值得行内的 P3。
Generated-by: Codex
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head 29480dda (verified unchanged at review time). The head gained a second commit during review (test(desktop): remove duplicate WorkHub checkpoints); git range-diff confirms the original fix commit is patch-identical, and I verified the follow-up separately — see the last bullet.
Lifecycle fix, verified rather than assumed:
- The guard is fail-safe in the right direction: the renderer keeps the resolved projection only when the event's
hostIdequals the identity parsed out of the already-resolved Session key. I traced the publisher inruntime-host-boot.ts: wire events carryhostIdwhenever the manager knows it (readyevents take it from the live candidate;reconnecting/unavailablefrom the target's last known value). An event without a knownhostIdcan never match, so unknown identity falls through to the old revoke-and-resolve path rather than accidentally pinning a stale projection. - The retained behaviors are the right ones:
unavailableandremovedalways revoke; a different Host'sreadyrevokes and re-resolves; the stale-resolution generation guard is untouched. The semantics deliberately key authority to the Host (not the profile): two default profiles pointing at the same Host share the same coordination Session key, so keeping the projection across that swap is consistent. - E2E readiness now observes authority, not pixels:
waitForWorkHubReadywaits on the default Host'sreadiness === 'ready'snapshot plus the durable work-count projection, replacing assertions on a mountedmain(which stays mounted during reload) and on.workhub-result(transient local feedback). The layout contract now measures the specific submitted Turn it intends to measure instead of whichever result happened to be in the DOM. - The follow-up dedup is accurate: the deleted standalone regression test ("keeps its resolved projection while the same default Host reconnects") is subsumed by the updated second lifecycle test, which fires a same-Host
reconnectingevent after resolution and asserts no new resolve occurs. The guard is a single condition over all non-unavailablereadiness values, so pinning it onreconnectingcovers the mechanism. No assertion was lost.
Executed on a real Windows machine at this head: clean rebuild of the workspace libs (including the install-time model-metadata generation this head now requires) and Desktop main, then the WorkHub coordination lifecycle suite 5/5 — including the updated authority-change sequence that pins the same-Host reconnect behavior.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
简体中文
生命周期修复,全部核实而非假设:守卫的失效方向是对的——只有事件的 hostId 与「已解析 Session key 里编码的 Host 身份」一致才保留投影;发布端(runtime-host-boot)在已知时恒带 hostId,未知则恒不匹配、落回旧的 revoke+resolve 路径。unavailable/removed 恒撤销,异 Host ready 恒重解析,generation 防陈旧守卫未动;权威键是 Host 而非 profile,与协调 Session key 的结构一致。E2E 改为观察 ready 快照 + 持久 work-count 投影,不再把「main 还挂着」当就绪。后续去重提交核实为真重复(被删的专项测试被更新后的序列测试涵盖,机制是单条件)。本机真 Windows 干净重建(含本 head 要求的安装期元数据生成)+ 生命周期套件 5/5。
jackwener
left a comment
There was a problem hiding this comment.
I reviewed this at 29480dda2eb85b66384fd46337ff4ee826bafc95. No P0, P1, P2, or P3.
The renderer used to treat every default Host profile event as an authority replacement. A brief reconnecting from the same Host revoked the resolved WorkHub generation and swapped in the loading surface. The change keeps that generation when the already-resolved desktop Session key's hostId still matches, and the event is not unavailable or removed. A real default Host switch still revokes and re-resolves. Unavailable/removed still reports failure instead of spinning forever. Reconnect events from the Desktop Host manager include hostId once a candidate has been ready, which is the path this skip needs.
The E2E helper now waits for the default Host snapshot to be ready and for the durable work-count projection, then asserts the reconstructed terminal turn instead of a transient .workhub-result. That matches the production boundary: Host and Session stay the authority; this layer only decides when to keep or drop the already-resolved projection. Hosted test is green on this head, including Desktop e2e. MERGEABLE. This is a bugfix; I am not merging it.
简体中文
我审的是 29480dda2eb85b66384fd46337ff4ee826bafc95。没有 P0/P1/P2/P3。
同一默认 Host 短暂 reconnecting 时不再拆掉已经解析好的 WorkHub 投影。Host 真的换了、不可用、或被移除时仍会撤销并重新解析。E2E 改成等 Host ready 和 durable 工作计数,再断言重建后的终态,不再盯瞬时 .workhub-result。hosted test 绿了。这是 bugfix,我不合入。
Automated review notice: This comment was posted by an automated review agent operated by WAWQAQ. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
Approved at exact head 29480dda2eb85b66384fd46337ff4ee826bafc95. I found no P0-P3 issues.
The production change is narrowly scoped and matches the ownership boundary: workhub-coordination-lifecycle.ts records the Host identity encoded in the resolved Coordination Session, preserves the mounted projection only for non-terminal events from that same Host, and still revokes on unknown/different Host identity, unavailable, or removed. I traced the event producer through runtime-host-boot.ts and runtime-host-desktop-manager.ts; reconnect events retain the target's Host identity while the target epoch remains stable, and a genuine target replacement/removal emits the revocation path. The existing generation check still rejects stale asynchronous resolutions.
The E2E changes wait on the default Host's authoritative ready snapshot plus WorkHub's durable work-count projection, scope selectors to the intended turn, and assert the reconstructed terminal state. I do not consider the existing P2 comment blocking: the layout regression's mutation-sensitive invariant is that the button contains its two-line metadata (buttonContainsProject), which failed before the original height: auto fix. .workhub-result is intentionally transient and optional after durable delegation replaces local feedback; because both elements remain in normal document flow, containing the metadata within the button is the non-vacuous overlap contract.
Review conclusions:
- Optimal for this scope: yes; one Host identity field removes the false authority transition without adding another lifecycle or retry.
- Deletable production code: none introduced or exposed.
- Deletable/replaceable tests: the follow-up commit correctly removes duplicate readiness and same-Host checkpoints while retaining the behavior in the authority-change sequence.
- Deeper refactor: not required.
- Ready to merge: yes at this exact head.
- Residual risk: timing-sensitive behavior always retains some platform variance, but exact-head hosted CI and the affected local suites are green. No protocol, storage, migration, or security boundary changed.
Validation on this exact head: Desktop typecheck; 85 WorkHub unit/integration tests; 4 targeted Electron E2E tests under Xvfb; GitHub test run 33269619737 (success, 13m25s); merge-ref tree equals the head tree; zero new code changes after the head refresh.
Codex-assisted review performed under the maintainer-approved review workflow.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Generated-by: Codex
Summary
Stabilize WorkHub reconstruction across transient Runtime Host reconnects without introducing another coordination authority.
reconnectingorready. Revoke it only when Host authority actually changes or becomes unavailable/removed.Fixes #4205
Root cause and architecture boundary
The renderer lifecycle treated every default Host profile event as an authority replacement. When the existing Host briefly reported
reconnecting, the lifecycle revoked its resolved generation and replaced WorkHub with the loading surface, detaching actions at the reconstruction and correction checkpoints.The E2E coverage compounded that lifecycle defect by treating a mounted WorkHub
mainand.workhub-resultas readiness. The former remains mounted while coordination reloads; the latter is transient local feedback and can disappear when the durable delegation projection replaces it.Runtime Host and Session remain the authority. This change only compares the Host identity encoded in the already-resolved desktop Session key: reconnects from that same Host preserve the projection, while a real default Host switch or unavailable/removed Host still revokes and re-resolves it. Latest
mainalready owns active-target admission and idle recovery in the Runtime Host boundary via #4185; this PR does not duplicate or replace that work.Why this is not a wait or retry fix
No fixed sleeps, suite retries, operation retries, timeout increases, parallel store, or second lifecycle were added. The E2E helper observes the existing default Runtime Host profile until its owner reports
ready, then waits for the corresponding WorkHub work-count projection before interacting.Verification
npm run build— passed--repeat-each=10 --workers=1— 40/40 passed; after the test-only simplification, reconstruction passed 12/12 and the final exact-head layout contract passed 20/20main— 89 passed, 1 skipped; one unrelatednew-task-draft-targetproject-picker test failed because its menu did not reopen. A bounded three-repeat rerun of that file was 5/6 with the same independent failure.npm run typecheck— passednpm run lint— passednpm run format:check— passednpm run check:asf-headers— passedgit diff --check— passedAI use
Select exactly one:
Tool(s) and scope: Codex diagnosed the lifecycle/readiness boundary, implemented the production and E2E changes, and added the regression test. The commit carries the required
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?