diff --git a/.pi-web/relays/worktree-autodetect/log.md b/.pi-web/relays/worktree-autodetect/log.md index 19893c4..1cfbe23 100644 --- a/.pi-web/relays/worktree-autodetect/log.md +++ b/.pi-web/relays/worktree-autodetect/log.md @@ -445,3 +445,71 @@ web/UI service path only. **No — this was the final leg.** All five charter finish-line conditions are met and `npm run verify` is green, so per the charter this runner stops instead of spawning. The branch `feat/worktree-autodetect` is ready for human review and merge. + +--- + +## Post-relay — selected-workspace freshness fix (review follow-up, not a leg) + +Triggered by a human review question after the relay finished: "do we have pragmatic +reasonable and stable code?" I reviewed the production diff against +`code-quality-architecture` and probed the runtime rather than trusting the leg summaries. + +### What the review checked and found sound + +- `handleWorkspaceChange` early-returns on equal workspace id → no `clearActiveSession`, + no terminal teardown on resume. +- `WorkspaceList.updated()` re-scrolls on any `workspaces` change, but via + `scrollIntoView({ block: "nearest" })`, a no-op when the row is already visible. +- Open row menu survives refresh: guarded by an id membership check, and ids are path-derived. +- Stale responses guarded on both machine and project id; background failures go to + `onBackgroundError`, never `state.error`, so a flaky resume shows no error toast. + +### The one real gap, and the fix + +`applyProjectWorkspaces` wrote a fresh `workspaces` array but left `selectedWorkspace` +pointing at the pre-refresh object. Reproduced with a scratch test: after a refresh where a +worktree's branch changed outside PI WEB, the list row showed `feature-b` while +`selectedWorkspace.branch` was still `feature-a`. User-visible in the collapsed Workspaces +header and the mobile context bar until reselect. Not a regression (both were stale before), +but a new fresh/stale inconsistency introduced by making the list refresh. + +Fixed by re-pointing `selectedWorkspace` at its refreshed entry, keyed by `id`. Safety rests +on two things: `id` is derived from the path, so this can never change *which* workspace is +selected; and `handleWorkspaceChange` gates on `id`, so no session/terminal teardown fires. +The patch is skipped when metadata is unchanged, because `patchChangesState` is +identity-based and a real HTTP response returns fresh-but-equal objects every resume — +without that guard every browser focus would push a new object into state. + +### Decisions + +- **Left `locked` parsed-but-unconsumed.** Flagged it as YAGNI in review, then kept it: it + documents the deliberate policy that locked worktrees are *kept*, and that policy is pinned + by a real `workspaceService` test. Removing the field would not remove the policy. +- **Left the two refresh entry points un-deduped.** Both are idempotent, stale-guarded, and + ~2ms; cross-path collapsing would add a concept for no user-visible gain. +- **Compared metadata field-by-field** (`sameWorkspaceMetadata`) rather than `JSON.stringify`, + which is key-order sensitive, or a deep-equal helper this file does not otherwise need. + +### Checks run + +- `npx vitest --run src/client/src/controllers/workspaceController.test.ts` → **9 passed** +- mutation A, never re-point (restores the original bug) → the re-point test failed, alone +- mutation B, always re-point (drops the unchanged guard) → the identity test failed, alone +- `npx eslint` on both changed files → clean; `npm run typecheck` → clean +- **`npm run verify` → green**: 228 files, **1842 passed**, 2 skipped (was 1840) +- pre-commit `verify:staged` → 6 files / 26 tests passed + +### Artifacts changed + +- `src/client/src/controllers/workspaceController.ts` (`refreshedSelection` + + `sameWorkspaceMetadata`; `applyProjectWorkspaces` early-returns for the non-selected project) +- `src/client/src/controllers/workspaceController.test.ts` (+2 tests, 9 total) +- committed as `79577e4 fix(workspaces): keep selected workspace metadata fresh on refresh` +- no changeset added: `.changeset/worktree-autodetect.md` already promises the list stays + correct without user action, and this fix delivers that promise rather than adding to it +- `status.md`: new "Post-relay review fix" section and the second invariant recorded + +### Blockers + +None. No new timer, watcher, process, endpoint, or push channel; no sessiond code touched, so +**no manual session daemon restart is required**. Branch still ready for review and merge. diff --git a/.pi-web/relays/worktree-autodetect/status.md b/.pi-web/relays/worktree-autodetect/status.md index 874a5ac..d8e9b47 100644 --- a/.pi-web/relays/worktree-autodetect/status.md +++ b/.pi-web/relays/worktree-autodetect/status.md @@ -7,6 +7,21 @@ green. No further leg was spawned; leg 3 was the last one by design. Remaining human action: review the branch and merge it. Nothing is blocked. +## Post-relay review fix + +A human review question after leg 3 ("do we have pragmatic reasonable and stable code?") +found one real gap, fixed in `79577e4`: `applyProjectWorkspaces` replaced the list but left +`selectedWorkspace` pointing at the old object, so a branch switched inside a worktree +outside PI WEB showed the new name in the list and the old one in the collapsed Workspaces +header and mobile context bar. Now re-pointed by id, and skipped entirely when metadata is +unchanged so a normal resume does not churn identity. Two tests added (9 total in +`workspaceController.test.ts`), each mutation-checked in both directions. `npm run verify` +green: 1842 passed, 2 skipped. + +Reviewed and deliberately left alone: `locked` is parsed but unconsumed (it documents the +kept-worktree policy and is pinned by a `workspaceService` test), and the two refresh entry +points are not deduped across each other (idempotent, stale-guarded, ~2ms). + ## Current position Every charter finish-line condition is satisfied: @@ -49,7 +64,12 @@ under `#worktree-list-out-of-date`. ## Relevant context if anyone picks this branch up - Commits: `266f941` (server filter), `d0f8f9f` (client refresh method), - `84545fb` (wiring + docs + changeset), plus three `docs(relay)` packet commits. + `84545fb` (wiring + docs + changeset), `79577e4` (selected-workspace freshness fix), + plus the `docs(relay)` packet commits. +- Second invariant, added by `79577e4`: re-pointing `selectedWorkspace` is safe **only** + while it is keyed by `id` and skipped on unchanged metadata. Keying it by anything that can + differ between two lists would change the selection on a background refresh; dropping the + unchanged-metadata guard would push a new object into state on every browser focus. - The invariant to protect on any future edit: **never route a background topology refresh through `selectWorkspace`.** It calls `clearActiveSession()` and `resetWorkspaceScopedState()` with no already-selected guard, which would close the session