Archived
docs(relay): record worktree-autodetect leg 2
This commit is contained in:
@@ -273,3 +273,89 @@ None. Nothing ambiguous, no design decision needed, no intervention trigger fire
|
|||||||
### Handing off?
|
### Handing off?
|
||||||
|
|
||||||
**Yes.** Work committed, packet updated, leg 2 dispatched via `spawn_session`.
|
**Yes.** Work committed, packet updated, leg 2 dispatched via `spawn_session`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Leg 2 — Non-disruptive workspace topology refresh in the client
|
||||||
|
|
||||||
|
**Commit:** `d0f8f9f` — `feat(workspaces): add non-disruptive workspace topology refresh`
|
||||||
|
|
||||||
|
### What I did
|
||||||
|
|
||||||
|
Added `WorkspaceController.refreshSelectedProjectTopology()`: it reads the selected project
|
||||||
|
and machine, calls `api.workspaces(project.id, machineId)`, re-reads state, discards the
|
||||||
|
response if machine or selected project changed mid-flight, and applies the list via the
|
||||||
|
existing private `applyProjectWorkspaces` — and nothing else. No selection is re-derived, no
|
||||||
|
session is cleared, no workspace-scoped state is reset, no URL update.
|
||||||
|
|
||||||
|
Added `src/client/src/controllers/workspaceController.test.ts` (7 tests, new file).
|
||||||
|
|
||||||
|
### Decisions and why
|
||||||
|
|
||||||
|
- **Did not use `selectPreferredWorkspace` / `selectWorkspace`.** `plan.md` documented this
|
||||||
|
as the plausible-looking destructive shape; I confirmed it in the source before writing.
|
||||||
|
`selectWorkspace` has no already-selected guard, so it unconditionally calls
|
||||||
|
`sessions.clearActiveSession()` and `setState({ ...resetWorkspaceScopedState() })` — which
|
||||||
|
clears `sessions`, `fileTree`, `expandedDirs`, `selectedFilePath`, `gitStatus`, the three
|
||||||
|
diff fields, and `selectedTerminalId`. Since leg 3 calls this on every browser resume, that
|
||||||
|
would blank the UI on every alt-tab. Applying the list alone is sufficient.
|
||||||
|
- **Selected workspace that disappeared: selection left untouched**, per plan. No new
|
||||||
|
recovery path; `refreshAfterWorkspaceDeleted` still owns that. Covered by a test asserting
|
||||||
|
the vanished workspace stays selected and `clearActiveSession` is not called.
|
||||||
|
- **Errors: injected sink, not `state.error`.** Added optional
|
||||||
|
`onBackgroundError(message, error)` to `WorkspaceControllerDependencies`, defaulting to
|
||||||
|
`console.warn`, mirroring `SessionNotificationController`'s existing pattern. This keeps the
|
||||||
|
behavior testable without stubbing `console` and guarantees a background refresh never
|
||||||
|
paints an error banner over a working session. The method therefore never rejects.
|
||||||
|
- **Guard style copied from `selectProject`** (compare `selectedMachineId` and
|
||||||
|
`selectedProject?.id` after the await) rather than inventing a generation counter — the
|
||||||
|
method is a single request with no trailing-pass semantics, so
|
||||||
|
`ProjectActivityOwnershipCoordinator`'s heavier machinery would have been disproportionate.
|
||||||
|
- **Wrote a `why` comment on the method**, not a `what` one: it records the invariant and why
|
||||||
|
the simpler-looking `selectWorkspace` route is unsafe, so a future editor cannot "simplify"
|
||||||
|
it back into the bug.
|
||||||
|
- **Nothing wired into `PiWebApp`**, per plan — deliberately leg 3.
|
||||||
|
|
||||||
|
### Verification that the guard is not vacuous
|
||||||
|
|
||||||
|
Mutation-tested the tests: temporarily patched the method to the plausible wrong version
|
||||||
|
(`selectPreferredWorkspace(...)` + `await this.selectWorkspace(...)`) and re-ran the file —
|
||||||
|
**2 of 7 tests failed** (selection-preserved and vanished-workspace). Restored the file from
|
||||||
|
backup and re-confirmed 7/7 green. The tests genuinely pin the invariant.
|
||||||
|
|
||||||
|
### Checks run
|
||||||
|
|
||||||
|
- `npx vitest --run src/client/src/controllers/workspaceController.test.ts` → **7 passed**
|
||||||
|
- `npx vitest --run src/client/src/controllers` → **26 files, 204 tests passed** (no
|
||||||
|
regression in sibling controllers)
|
||||||
|
- `npm run typecheck` → **clean** (one iteration: `expandedDirs` is
|
||||||
|
`Record<string, FileTreeEntry[]>`, not booleans — fixed in the fixture)
|
||||||
|
- `npx eslint` on both changed files → **clean**
|
||||||
|
- pre-commit `npm run verify:staged` on commit → typecheck (cached), knip, eslint, and
|
||||||
|
related vitest (5 files / 21 tests) all **passed**
|
||||||
|
|
||||||
|
`npm run verify` was not run: the charter assigns it to leg 3, and this leg's change is
|
||||||
|
contained to one controller.
|
||||||
|
|
||||||
|
### Artifacts changed
|
||||||
|
|
||||||
|
- `src/client/src/controllers/workspaceController.ts` (modified: new method, new optional
|
||||||
|
`onBackgroundError` dependency)
|
||||||
|
- `src/client/src/controllers/workspaceController.test.ts` (new, 7 tests)
|
||||||
|
- `status.md` (leg tracking → last completed 2 / next 3, next task set to leg 3 with the
|
||||||
|
method's exact call signature, added notes on test coverage + the mutation check + the new
|
||||||
|
`onBackgroundError` dependency, rewrote Blockers to record that the UI-churn risk did not
|
||||||
|
materialize)
|
||||||
|
|
||||||
|
Still no changeset — leg 3 owns it, and `status.md` now flags it explicitly as missing.
|
||||||
|
|
||||||
|
### Blockers
|
||||||
|
|
||||||
|
None. The charter's main intervention trigger for this leg (refresh cannot be made
|
||||||
|
non-disruptive without visible UI churn) did not fire: no workaround was needed. No watcher,
|
||||||
|
timer, process, endpoint, or push channel added. No sessiond code touched, so **no manual
|
||||||
|
session daemon restart is required**.
|
||||||
|
|
||||||
|
### Handing off?
|
||||||
|
|
||||||
|
**Yes.** Work committed, packet updated, leg 3 dispatched via `spawn_session`.
|
||||||
|
|||||||
@@ -3,39 +3,40 @@
|
|||||||
## ✅ APPROVED — relay is live
|
## ✅ APPROVED — relay is live
|
||||||
|
|
||||||
The human approved the reduced scope in leg 0 and answered every open question. There are
|
The human approved the reduced scope in leg 0 and answered every open question. There are
|
||||||
no outstanding decisions. Run leg 2.
|
no outstanding decisions. Run leg 3 — the final leg.
|
||||||
|
|
||||||
## Current position
|
## Current position
|
||||||
|
|
||||||
Leg 1 is complete and committed (`266f941`). The **server** side of the feature is done:
|
Legs 1 and 2 are complete and committed (`266f941`, `d0f8f9f`). The server hides prunable
|
||||||
prunable worktrees are no longer reported as workspaces. Nothing in the client has changed
|
worktrees, and the client now has a proven-inert refresh method. **Nothing calls it yet**,
|
||||||
yet, so newly created worktrees still only appear on a full page load.
|
so newly created worktrees still only appear on a full page load. Leg 3 wires the trigger
|
||||||
|
and is the last leg.
|
||||||
|
|
||||||
The design remains reduced scope: detection piggybacked on browser resume. No watchers,
|
The design remains reduced scope: detection piggybacked on browser resume. No watchers,
|
||||||
no timers, no new processes, no new push channel. Breakdown is in `plan.md`.
|
no timers, no new processes, no new push channel. Breakdown is in `plan.md`.
|
||||||
|
|
||||||
## Leg tracking
|
## Leg tracking
|
||||||
|
|
||||||
- **Last completed leg:** 1 (server: hide removed worktrees)
|
- **Last completed leg:** 2 (client: non-disruptive topology refresh method)
|
||||||
- **Next leg to run:** 2
|
- **Next leg to run:** 3 (final)
|
||||||
|
|
||||||
## Next task — leg 2
|
## Next task — leg 3
|
||||||
|
|
||||||
Non-disruptive workspace topology refresh in the client.
|
Wire the refresh to the existing resume path, document it, add the changeset.
|
||||||
|
|
||||||
See `plan.md` → "Leg 2" and **read it fully before writing the method** — it spells out the
|
See `plan.md` → "Leg 3". Summary: call `this.workspaces.refreshSelectedProjectTopology()`
|
||||||
plausible-looking wrong implementation and exactly why it is destructive. Summary: add
|
from `PiWebApp.refreshAfterBrowserResume` (~line 432) and `refreshAppData` (~line 485) —
|
||||||
`refreshSelectedProjectTopology()` to `WorkspaceController` that re-lists the selected
|
touch only those two methods in that 2300-line file. Optionally also on
|
||||||
project's workspaces and applies them through `applyProjectWorkspaces` **only** — never
|
`connectRealtime`'s `onReconnect`, only if it costs nothing. Then one short honest doc
|
||||||
through `selectWorkspace`, which has no already-selected guard and would clear the active
|
paragraph under `docs/` (detection is resume-scoped, not instant; do not grow `README.md`),
|
||||||
session and all workspace-scoped state on every alt-tab. Guard against machine/project
|
and the `.changeset/*.md` fragment — **still missing, and leg 3 owns it**.
|
||||||
changing mid-flight; `console.warn` on failure, never `setState({ error })`.
|
|
||||||
|
|
||||||
Files: `src/client/src/controllers/workspaceController.ts`,
|
Finish with `npm run verify` (charter requires green) — this is the final leg.
|
||||||
new `src/client/src/controllers/workspaceController.test.ts`.
|
|
||||||
|
|
||||||
Do **not** wire anything into `PiWebApp` in leg 2 — that is leg 3, deliberately after the
|
Signature detail leg 3 needs: `refreshSelectedProjectTopology()` takes no arguments,
|
||||||
refresh is proven inert.
|
returns `Promise<void>`, never rejects (failures go to the injected background error sink,
|
||||||
|
defaulting to `console.warn`), and no-ops when no project is selected. So it can be dropped
|
||||||
|
directly into the existing `Promise.all` without a `.catch`.
|
||||||
|
|
||||||
## Relevant context for the next runner
|
## Relevant context for the next runner
|
||||||
|
|
||||||
@@ -65,6 +66,20 @@ real git; do not re-derive them:
|
|||||||
- **Remote machines come for free.** `workspacesApi.workspaces(projectId, machineId)`
|
- **Remote machines come for free.** `workspacesApi.workspaces(projectId, machineId)`
|
||||||
routes through `machinePrefix`, and `GET /projects/:projectId/workspaces` is already in
|
routes through `machinePrefix`, and `GET /projects/:projectId/workspaces` is already in
|
||||||
`FEDERATED_HTTP_ROUTES` in `src/shared/federatedRoutes.ts`. No transport work needed.
|
`FEDERATED_HTTP_ROUTES` in `src/shared/federatedRoutes.ts`. No transport work needed.
|
||||||
|
- **Leg 2's method is inert by construction and covered.** `refreshSelectedProjectTopology()`
|
||||||
|
applies results via `applyProjectWorkspaces` only, so it writes at most `workspaces` and
|
||||||
|
`workspacesByProjectId`. Seven tests in
|
||||||
|
`src/client/src/controllers/workspaceController.test.ts` cover: new worktree appears,
|
||||||
|
selection + session + file tree + terminal state preserved and `clearActiveSession` not
|
||||||
|
called, selected workspace disappeared (selection deliberately left alone), stale project
|
||||||
|
response discarded, stale machine response discarded, rejection reported to the error sink
|
||||||
|
without touching `state.error`, and no-project no-op. Leg 2 mutation-tested this: injecting
|
||||||
|
the plausible `selectPreferredWorkspace` + `selectWorkspace` version made 2 tests fail, so
|
||||||
|
the guard is real and not vacuous. Leg 3 must not weaken these tests to fit its wiring.
|
||||||
|
- **`WorkspaceControllerDependencies` gained `onBackgroundError`** (optional, defaults to
|
||||||
|
`console.warn`). `PiWebApp` constructs `WorkspaceController`; leg 3 may pass a message-
|
||||||
|
prefixing sink to match sibling controllers, but the default is already correct — this is
|
||||||
|
not required work.
|
||||||
- **Leg 1 shipped a seam leg 2 does not need but should know about.** `WorkspaceService`
|
- **Leg 1 shipped a seam leg 2 does not need but should know about.** `WorkspaceService`
|
||||||
now takes an optional `WorkspaceGitPort` (`{ isGitRepository, discoverGitWorktrees }`)
|
now takes an optional `WorkspaceGitPort` (`{ isGitRepository, discoverGitWorktrees }`)
|
||||||
in its constructor, defaulting to the real git implementation, so workspace policy is
|
in its constructor, defaulting to the real git implementation, so workspace policy is
|
||||||
@@ -101,6 +116,12 @@ real git; do not re-derive them:
|
|||||||
|
|
||||||
## Blockers
|
## Blockers
|
||||||
|
|
||||||
None. Leg 2 is clear to run. The one thing to watch is leg 2's own intervention trigger:
|
None. Leg 2's main known risk (UI churn on refresh) did **not** materialize: applying the
|
||||||
if the refresh cannot be made non-disruptive without visible UI churn, stop rather than
|
list through `applyProjectWorkspaces` alone required no workaround, and
|
||||||
working around it.
|
`handleWorkspaceChange` early-returns on an unchanged selected workspace id, so a
|
||||||
|
fresh-but-equal list causes no downstream churn. No intervention signal fired.
|
||||||
|
|
||||||
|
Leg 3's own watch item: after wiring, confirm nothing in the resume path re-derives
|
||||||
|
selection from the refreshed list. If wiring turns out to need a change *inside*
|
||||||
|
`refreshSelectedProjectTopology` to stay non-disruptive, that is the intervention signal —
|
||||||
|
stop rather than relaxing the invariant.
|
||||||
|
|||||||
Reference in New Issue
Block a user