fix(workbench): revalidate session and workspace lists on session.changed - #526
fix(workbench): revalidate session and workspace lists on session.changed#526Zerlight wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Several updated doc comments claim importSession is covered by session.changed→workspace touch ordering, but engine importSession touches the workspace after importing/announcing the record, making those statements inaccurate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes the workbench runtime respond to host-pushed session.changed events by revalidating the SWR caches for the session list and workspace list, fixing cases where sessions created in previously-unknown workspaces didn’t appear in the sidebar until a full reload.
Changes:
- Add a per-connection-generation subscription to
session.changedin the runtime provider and revalidate only thelistSessions/listWorkspacescaches. - Update the dev mock host to emit
session.changedevents for start/import/resume/title updates to match engine behavior. - Add an integration test asserting the two lists update after a session is started in an unknown workspace.
File summaries
| File | Description |
|---|---|
| packages/client/workbench/src/runtime/provider.tsx | Adds session.changed subscription and targeted SWR cache revalidation for session/workspace lists. |
| packages/client/workbench/src/workspace/hooks.ts | Updates documentation comment for useWorkspaces behavior (push-triggered revalidation). |
| packages/client/workbench/src/mock/dev-mock-host.ts | Emits session.changed frames on relevant mock host actions to match engine push behavior. |
| packages/client/workbench/tests/integration/session-changed-revalidation.test.tsx | New integration test covering revalidation behavior when a session is started in a new workspace. |
| packages/client/workbench/AGENTS.md | Documents runtime/SWR revalidation behavior on session.changed. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The diagnosis and the fix's shape are both right, and the new test genuinely fails without the provider change (I verified by neutering the subscription body). One issue needs addressing before merge: the new revalidation has no burst coalescing, and SWR's key-filter mutate explicitly defeats its own dedupe window, so a bulk history import turns into hundreds of forced list round trips.
Reviewed changes — full review of the single commit fef197b8, covering all 5 files, with the SWR/tayori key mechanics and the engine's emission ordering traced against the installed sources.
session.changedsubscription in the runtime provider —ReadyRevalidatorbecomesHostRevalidatorand gains onesubscribeSessionChangedper connection generation that callsmutate(isHostListKey), filtered to thelistSessions/listWorkspacescache entries.- Dev mock host emissions — four new
session.changedsends (start and import ascreated, resume and first-prompt title asupdated) so the mock approximates engine behavior. - Doc corrections —
useWorkspaces's "No push invalidation yet" comment and theAGENTS.mdruntime paragraph both updated to describe the new push path and its remaining gaps. - New integration test — drives the mock host and asserts both lists pick up a session started in a previously unknown workspace.
A few things I checked and found sound, so they don't need re-examination: generation.client.raw is a constructor-assigned readonly field, so the effect's deps are stable and it doesn't resubscribe per render; returning the unsubscribe from foxact/use-abortable-effect is correct since its callback type is ReturnType<React.EffectCallback>; the engine's "workspace is registered before the record is announced" premise genuinely holds for start and resumeHistory; and the whole downstream render pipeline (groupThreadsByWorkspace → ordering → selectVisibleSessions) does surface a worktree-backed session under its parent project once the lists are fresh, with no collapse/pin/order store able to hide it.
ℹ️ The three disclosed gaps have no named follow-up
The PR body documents three things that need the rejected additive workspace.changed frame — an explicit workspace.register / rename / archive from another client, the importSession ordering, and mobile's pull-only useWorkspaces — and states none is covered here. That's an honest scope call, but no follow-up issue is named for any of them, and two of the three are now encoded as permanent caveats in AGENTS.md and the useWorkspaces docstring rather than as tracked work.
Technical details
# Follow-up tracking for the deferred `workspace.changed` frame
## Affected sites
- `packages/client/workbench/AGENTS.md:34-36` — the caveat "an explicit `workspace.register` / rename / archive from *another* client has no push and waits for the next focus revalidation" is now documented as steady state.
- `packages/client/workbench/src/workspace/hooks.ts:5-8` — same caveat in the `useWorkspaces` docstring.
- `packages/host/engine/src/session/lifecycle-service.ts:154-155` — `records.importRecord` (which fires `onChanged('created')`) runs before `workspaceTouch`, so a brand-new cwd imported by another client can miss the first revalidation.
- `apps/mobile/src/runtime/use-workspaces.ts` — still pull-only.
## Required outcome
- Either a tracked issue exists for the `workspace.changed` frame and the three gaps it closes, or the decision to leave them permanently is stated explicitly.
## Open questions for the human
- Does CODE-654 stay open to cover the follow-up, or should a new issue be filed?
- Is the `importSession` ordering gap worth fixing on its own? Swapping lines 154 and 155 so `workspaceTouch` precedes `importRecord` would close it without any wire change, matching what `start` and `resumeHistory` already do — is there a reason the import path orders them the other way?Claude Opus ｜ 𝕏
|
Verified the review findings and pushed bd9f642 + 68841ed. All six inline threads have replies and are resolved. Burst revalidation is coalesced, both object/lazy tayori keys are covered, and the import/deletion limitations are now explicit.
The deferred workspace synchronization gaps remain tracked under CODE-654, rather than being declared permanent or silently closed by this PR. Import ordering can be fixed separately without a wire change; the PR description has been corrected accordingly. |
There was a problem hiding this comment.
🔵 Needs a closer look
The mock should refresh the workspace before emitting session.changed on resume to match engine behavior.
Review details
Suppressed comments (1)
packages/client/workbench/src/mock/dev-mock-host.ts:1094
resumeSessionnow emits the same invalidation as the engine, but it never re-registers/freshenssession.cwdbefore that push. If the workspace was archived while this cold session was stopped, the revalidation still cannot list it, unlike the engine'sworkspaceTouchbefore resume; touch the workspace before sendingsession.changedso the mock exercises the same ordering and state transition.
session.status = 'idle';
// Parity with the engine: a relaunch appends a run, which re-points the listed identity.
this.send({ kind: 'session.changed', sessionId, reason: 'updated' });
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
ℹ️ No blocking issues — all three prior findings are genuinely fixed. One minor observation inline, one scope question below.
Reviewed changes — the delta since the prior pullfrog review at fef197b8, i.e. commits bd9f642d and 68841edf.
- Added
coalesceRunsand routed the push subscription through it — a newruntime/coalesce.tscollapses a burst into one in-flight run plus at most one trailing run, replacing the per-framevoid mutate(isHostListKey). - Widened
isHostListKeyto the resolved key tuple —Array.isArray(key)in place of tayori'sisInternalSWRKeybrand check, so the lazy/function-arg form ofuseDatais no longer silently skipped. - Reordered the mock's import emission —
session.changednow precedestouchWorkspace, matching the engine'simportRecord-then-workspaceTouchorder. - Documented the mock's remaining divergences —
AGENTS.mdnow states that the mock's synchronous touch cannot reproduce the engine's async import race, and that there is nosession.deletehandler and therefore noremovedemission. - Expanded coverage — three
coalesceRunsunit tests, the integration test parameterized over object and lazy key forms, and a six-concurrent-start burst test bounding fetches per list.
I verified the new coverage rather than taking it on trust: all six tests pass locally, and the burst test genuinely fails when I revert provider.tsx:219 to the previous un-coalesced call — real coverage, not theatre. I also traced coalesceRuns exhaustively for lost wakeups and found none: running = true executes synchronously before drain's first await, so the trigger's if (running) check has no window, and takeQueued() plus the finally share one uninterrupted continuation. SWR's key-filter mutate resolves only after Promise.all over the matched keys' revalidations settles, so the burst test's <= 2 bound measures real round trips. createFixedArray(6) is a dense [0..5], and neither foxts nor foxact ships an in-flight-collapse helper, so hand-writing this one is justified.
ℹ️ coalesceRuns landed in a package the loop it generalizes cannot import
packages/client/core/src/react.tsx:158-184 already runs the same serialize-and-collapse loop against the same session.changed stream, and the new helper is a faithful, framework-agnostic generalization of it — but it lives in packages/client/workbench, which depends on client-core, so the original can never adopt it. The two are already allowed to drift and already have: the new helper's run().catch(noop) keeps draining after a failure, while client-core's bare await refresh() throws out of its loop and abandons the queued trailing run.
Technical details
# Two copies of the collapse loop, in packages that cannot share
## Affected sites
- `packages/client/workbench/src/runtime/coalesce.ts:10` — the new `coalesceRuns`, framework-agnostic
and dependency-free apart from `foxts/noop`.
- `packages/client/core/src/react.tsx:158-184` — `useSessions`'s `revalidatingRef` / `queuedRef` loop:
the same algorithm, hook-shaped, mounted only by `apps/mobile`.
## Behavioral divergence that already exists
- `coalesce.ts:27` swallows a failed run (`await run().catch(noop)`) so the drain continues to the
queued trailing run.
- `react.tsx:175` does not: a rejected `refresh()` propagates out of the `do/while`, `finally` clears
`revalidatingRef`, and `queuedRef` stays `true` but unread — the trailing revalidation for that
burst is dropped until an unrelated frame arrives. Mobile's session list can stay stale.
## Required outcome
- A decision, not necessarily a change: either the two loops stay deliberately independent, or the
helper moves somewhere `client-core` can import it. `packages/foundation/common` is the stated home
for framework-agnostic utilities, and `coalesceRuns` has no React or SWR coupling.
## Open questions for the human
- Is a shared helper wanted here, or is duplicating ~20 lines across the client-core / workbench
boundary the preferred trade for keeping `client-core` dependency-free?
- If it stays duplicated, is client-core's drop-the-trailing-run-on-error behavior intentional?ℹ️ Nitpicks
packages/client/workbench/AGENTS.md:41— the new mock bullet sits as a top-level sibling of theapp//runtime//surface/entries in the "Source layout" list, but it describessrc/mock/, which has no entry of its own. It reads as a stray item mid-list; either nest it underruntime/or givemock/its own- \mock/` — …` entry.packages/client/workbench/src/runtime/__tests__/coalesce.test.ts— the three tests advance with a fixedawait Promise.resolve()count matched to the current microtask depth ofdrain. They pin the right invariants today, but adding any intermediateawaitinside the loop would break them without changing observable behavior.
Claude Opus ｜ 𝕏
|
Latest review follow-up is published in 66e3270.
Validation: check:ci passed, all 10 focused tests passed, and the complete serial suite passed (3,053 tests; one existing skip). The default parallel suite hit an unchanged engine process-timeout test twice; it passed in isolation and in the serial suite. No assertions or timeout values were weakened. |
There was a problem hiding this comment.
🟡 Changes recommended
The integration test reuses a closed transport across cases, preventing subsequent reconnections.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/client/workbench/src/mock/dev-mock-host.ts:1094
- This comment claims the mock appends a run, but
MockSessiononly exposesSessionInfoand this path never updatesrunsorupdatedAt; that parity is not implemented. Describe the workspace-recency/event-ordering behavior instead so the mock’s list semantics are not overstated.
// Parity with the engine: a relaunch appends a run, which re-points the listed identity.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
| const connectionSource = { | ||
| resolve: () => ({ endpoint: 'mock://session-changed', transport: createDevMockTransport() }), | ||
| }; |
There was a problem hiding this comment.
ℹ️ No new issues. The teardown fix is correct and its coverage is real — one correction to my own prior finding, and one doc nitpick.
Reviewed changes — the delta since the prior pullfrog review at 68841edf, i.e. the single commit 66e32703.
- Gave
coalesceRunsanAbortSignal— the trigger no-ops once aborted and the drain loop stops before taking a queued run, so generation teardown discards the trailing revalidation. Work already in flight is deliberately left to settle. - Added a second, independent guard inside the run — each run re-reads
controller.getSnapshot()and only callsmutatewhen the controller isreadyand still on this generation. - Made the mock touch the workspace before announcing a resume —
resumeSessionnow callstouchWorkspace(session.cwd, …)ahead of itssession.changedframe, matching the engine. - Shared the
deferredtest helper and de-coupled the unit tests from microtask depth —coalesce.test.tsnow importsdeferredfromconnection-controller-test-helpersand advances withvi.waitFor/wait(0)instead of a countedawait Promise.resolve()chain, addressing the prior nitpick. - Four new tests — two
it.eachabort cases (in-flight run settling via resolve and via reject), an archived-workspace-restored-on-resume integration test, and a disposed-generation-during-recovery integration test. - Recorded the helper-placement decision in
AGENTS.md— the coalescer stays workbench-local because SWR owns fetch errors here while client-core's direct refresh loop has different failure semantics. That answers the scope question from the prior review; I'm treating it as settled.
I checked the two guards are not redundant, because that was the non-obvious part. The abort signal alone does not cover the recovery case: ConnectionController.handleReadyClose → startRecovery → cancelRecovery → releaseGeneration calls client.dispose() while publish keeps the old contextGeneration in the snapshot and only flips status to 'connecting', so React never unmounts HostRevalidator and the effect cleanup — hence the abort — never fires. Only the in-run getSnapshot() check stops the drain there. I also confirmed the gate cannot lose a push: every path out of status !== 'ready' ends with a new generation being promoted, which remounts the component with a fresh revalidatedRef and re-fires mutate(trueFn).
All four new tests fail without their fix — I reverted each mechanism in isolation rather than taking it on trust. Deleting the gate fails the recovery test (expected "listWorkspaces" to be called 1 times, but got 2 times); dropping the two signal.aborted checks fails both abort cases; removing the mock's resume touchWorkspace fails the archive test. The mock's archive semantics match the engine's (WorkspaceRegistry.archive deletes the record outright and touch re-mints it), and lifecycle-service.ts:318-325 does touch before launchRun, so the new ordering is faithful. mutate from useSWRConfig() is stable in practice — the queued trailing run survives the re-renders in the recovery test, so the effect is not re-running per render and cannot drop a queue that way. eslint is clean on all five changed source files.
ℹ️ The client disposed console noise this commit targets is unchanged — my prior attribution was wrong
The burst test still prints exactly two [LinkCode data error] Error: client disposed lines, same as before this commit. I instrumented it: the trailing run starts while the generation is still ready (the gate passes), and cleanup() disposes the client ~10 ms later, mid-fetch. So the noise was never a post-teardown queued run — it is an in-flight SWR fetch cut off by disposal, which coalesceRuns explicitly declines to cancel. My prior review named the wrong mechanism and cited that log line as the symptom; the fix in this commit is still correct and still closes a real hole, just not the one the log line proves. No action needed — I'm recording it so the risk isn't carried forward as closed when the observable is unchanged.
Technical details
# `client disposed` on teardown is an interrupted in-flight fetch, not a queued run
## Evidence
- Instrumented `provider.tsx:221-227` and `handleFetchError`. Trace from
`vitest run … -t "collapses a burst"`:
`mutate start 79729` → `mutate end 80030` → `mutate start 80031` →
`[LinkCode data error] Error: client disposed 80041` (×2, `at drain (coalesce.ts:24)`) →
`mutate end 80042`.
- The trailing run passed the `snapshot.status === 'ready'` gate before starting; the client is
disposed by `cleanup()` while its two list fetches are outstanding.
## Why there is nothing to fix here
- `coalesceRuns`' contract is "abort discards the queued run without cancelling active work", which
this behavior matches exactly.
- The same window already exists for the protocol-ready `mutate(trueFn)` at `provider.tsx:211-215`,
independent of this PR.
## Open question for the human (optional)
- If the reconnect log noise is worth removing, the lever is `handleFetchError`
(`provider.tsx:269-272`) suppressing a disposed-client error, not the coalescer.ℹ️ Nitpicks
packages/client/workbench/AGENTS.md:41— the line ending… during recovery. This coalescer stays workbench-local:runs to ~112 columns while the rest of the paragraph wraps at ~100. Re-wrapping also lets the trailingIt does not own connection state.sit next to its antecedent (SWR) instead of after two sentences about the coalescer, where it now reads as if the coalescer is the subject.
Claude Opus ｜ 𝕏
Part of CODE-654. Deferred workspace synchronization work remains tracked in that issue.
A session started by another client in a new workspace did not appear in the Projects sidebar until reload: the workbench did not consume the host's existing
session.changedevent.The runtime now subscribes once per connection generation and refreshes the session and workspace list caches. Bursts share one in-flight revalidation and one pending trailing revalidation, preserving changes that arrive during a fetch. The filter matches both object and lazy/function tayori keys. Queued runs are cancelled on generation teardown, and each run checks the controller's current ready generation so a disposed client retained during reconnect cannot be revalidated. Protocol-ready revalidation remains unchanged; there is no wire change.
The mock emits created/updated notifications for start, import, resume and first-prompt title changes. Resume touches/restores the workspace before announcing the updated session. Import announces before touching its workspace, matching the engine's ordering, but its synchronous touch does not reproduce the engine's async race. The mock has no session-delete handler or removed notification, so these tests do not establish deletion parity.
Validation covers object and lazy keys through the real React runtime and mock wire transport, both lists receiving a new workspace/session without local mutate calls, burst request counts, and the coalescer's trailing-run behavior.
Remaining work tracked under CODE-654:
Validation at
66e32703:devenv shell -- pnpm check:cipassed (format, lint with 0 errors, typecheck); the complete suite passed viadevenv shell -- pnpm test --no-file-parallelism(3,053 tests; one pre-existing skip). The ten focused tests cover burst/lazy keys, abort on both resolve and rejection, disposed-generation reconnect, and archived-workspace resume. The default parallel suite twice hit an existing 5-second timeout in the unchanged engine process test; that test passed in isolation and in the full serial run. No assertions or timeout values were changed.