Archived
chore: untrack the relay working notes
.pi-web/ is gitignored local state; the relay packet was force-added while running the work and should not land on main. The notes stay on disk for the worktree, they are just no longer tracked.
This commit is contained in:
Federico Jaramillo Martinez
@@ -1,131 +0,0 @@
|
||||
# Charter — relay "worktree-autodetect"
|
||||
|
||||
## Relay identity
|
||||
|
||||
- **Name:** `worktree-autodetect`
|
||||
- **Root:** `.pi-web/relays/worktree-autodetect/` in worktree `/srv/dev/pi-web-worktrees/worktree-autodetect`
|
||||
- **Branch:** `feat/worktree-autodetect` (based on `main`)
|
||||
|
||||
## Goal / finish line
|
||||
|
||||
Worktrees created or removed outside PI WEB become visible in the browser workspace
|
||||
list **with no user action of any kind**, on the next natural browser resume, on both
|
||||
local and remote machines.
|
||||
|
||||
Concretely, the relay is finished when all of the following are true:
|
||||
|
||||
1. `discoverGitWorktrees` no longer reports worktrees whose checkout directory is gone
|
||||
(`prunable` in `git worktree list --porcelain`), so worktrees deleted outside PI WEB
|
||||
stop appearing as selectable ghost workspaces.
|
||||
2. `WorkspaceController` can re-list the workspaces of the selected project and apply the
|
||||
result **without disturbing the current selection, session, or scroll state** when the
|
||||
selected workspace still exists, and without silently yanking the user out of a
|
||||
workspace that vanished while they were working in it.
|
||||
3. `PiWebApp` calls that refresh from the existing browser-resume path
|
||||
(`refreshAfterBrowserResume`) and the existing plugin-facing `refreshAppData` path.
|
||||
No new timer, no new watcher, no new process, no new WebSocket channel.
|
||||
4. Tests cover: prunable parsing/filtering, refresh-preserves-selection,
|
||||
refresh-when-selected-workspace-disappeared, and the resume wiring.
|
||||
5. `npm run verify` is green, and a changeset exists describing the user-visible behavior.
|
||||
|
||||
**Explicitly out of scope** (decided in leg 0, do not re-open without the human):
|
||||
|
||||
- Filesystem watchers on `.git/worktrees` or anywhere else.
|
||||
- Polling timers for worktree discovery.
|
||||
- Any server→browser push channel for workspace topology.
|
||||
- Instant (sub-second) detection while the browser tab already has focus.
|
||||
- Auto-*adopting* anything into `projects.json`. Worktrees are derived, never registered;
|
||||
nothing is being adopted, and no project registry write is part of this work.
|
||||
|
||||
## Sizing
|
||||
|
||||
**One leg = one vertical slice that leaves the tree green and committed.**
|
||||
|
||||
A leg is done when its slice is implemented, its tests are written and passing, the
|
||||
narrowest meaningful checks are run (`npm test -- --run <file>`, plus `npm run typecheck`
|
||||
if exported types changed), and the work is committed. Do not carry uncommitted work
|
||||
across a handoff.
|
||||
|
||||
Expected shape is three legs (see `plan.md`). If a leg turns out bigger than one slice,
|
||||
split it and hand off the remainder rather than doing "just a bit more".
|
||||
|
||||
## Task selection policy
|
||||
|
||||
1. Take the explicit **next leg** named in `status.md`.
|
||||
2. If `status.md` does not name one, take the next unfinished slice in `plan.md` in order.
|
||||
3. If neither is clear, or the next slice would change the design rather than implement it,
|
||||
**stop and raise the intervention signal**. Do not redesign inside a leg.
|
||||
|
||||
## Handover protocol
|
||||
|
||||
Before handing off, in this order:
|
||||
|
||||
1. Make the work durable: source + tests written, checks run, changes **committed** with a
|
||||
Conventional Commit message.
|
||||
2. Update `status.md`: current position, last completed leg, next leg to run, next task,
|
||||
relevant context for the next runner, blockers.
|
||||
3. Append a concise entry to `log.md`: what you did, decisions and why, artifacts changed,
|
||||
exact checks run and their results, handing-off vs stopping.
|
||||
4. Then `spawn_session` **once**, with a prompt starting:
|
||||
|
||||
```text
|
||||
Relay "worktree-autodetect" leg <N> begins now.
|
||||
|
||||
You are the next runner in this Relay method chain.
|
||||
|
||||
Read:
|
||||
- .pi-web/relays/worktree-autodetect/charter.md
|
||||
- .pi-web/relays/worktree-autodetect/status.md
|
||||
|
||||
Do not read log.md end-to-end. Use it only for targeted lookup if status.md or charter.md points you there.
|
||||
|
||||
Run one leg according to the charter. Before handing off, update status.md, append log.md, make work durable, then either spawn the next leg once or stop with a clear intervention note.
|
||||
```
|
||||
|
||||
## Intervention signal
|
||||
|
||||
**Stop, do not spawn**, and write a clearly marked `## BLOCKED` section at the top of
|
||||
`status.md` plus a log entry, if any of these happen:
|
||||
|
||||
- The next task is ambiguous, or doing it would require a design decision not in this charter.
|
||||
- You are tempted to add a watcher, a timer, a new process, or a new push channel. That
|
||||
means the design boundary is being crossed — get the human.
|
||||
- Refresh-on-resume cannot be made to preserve selection without visible UI churn
|
||||
(list reordering, chat scroll jump, session reload, terminal teardown). This is the
|
||||
main known risk; it is a stop, not a workaround.
|
||||
- Filtering `prunable` would remove a workspace the user could plausibly still want
|
||||
(for example a temporarily unmounted network path) and you cannot bound that safely.
|
||||
- `npm run verify` fails for a reason you did not introduce.
|
||||
|
||||
## Reading discipline
|
||||
|
||||
Read to orient: `charter.md`, then `status.md`, then only the files `status.md` names.
|
||||
|
||||
Do **not** read `log.md` end-to-end; use it only for targeted lookup when pointed there.
|
||||
Do **not** read the sibling worktrees `/srv/dev/pi-web-worktrees/worktree-create-ui` or
|
||||
`/srv/dev/pi-web-worktrees/model-questions-ux` — they are separate, parallel efforts. Per
|
||||
the human's decision, assume they contribute nothing to this relay; this relay owns the
|
||||
workspace-topology refresh seam outright.
|
||||
|
||||
Relevant source surface, small enough to read directly when your leg touches it:
|
||||
|
||||
- `src/server/workspaces/gitWorktreeDiscovery.ts` (39 lines)
|
||||
- `src/server/workspaces/workspaceService.ts` (44 lines)
|
||||
- `src/client/src/controllers/workspaceController.ts` (~105 lines)
|
||||
- `src/client/src/appShell/browserResumeController.ts` + its test
|
||||
- `src/client/src/components/PiWebApp.ts` — only `refreshAfterBrowserResume`
|
||||
(~line 432) and `refreshAppData` (~line 485). Do not read this 2300-line file whole.
|
||||
|
||||
## Project conventions that apply
|
||||
|
||||
- **Changesets:** this is user-visible. Add a `.changeset/*.md` fragment
|
||||
(see `.agents/skills/changeset-changelog/SKILL.md`). Never hand-edit `CHANGELOG.md`.
|
||||
- **Skills:** use `.agents/skills/code-quality-architecture/SKILL.md` when writing
|
||||
production code and `.agents/skills/testing-guide/SKILL.md` when writing tests.
|
||||
- **Session daemon:** this design deliberately touches **no** sessiond code, no session
|
||||
runtime ownership, and no daemon protocol. **No manual session daemon restart is
|
||||
required.** Changes land on the autoreloading `pi-web-web-ui-dev.service` path only.
|
||||
If a leg finds itself editing `src/server/sessiond.ts`, that is the intervention signal.
|
||||
- **Client URL conventions:** no new endpoints are added; the existing
|
||||
`workspacesApi.workspaces()` request path is reused unchanged.
|
||||
- **No `npm install`** — `node_modules` here is a symlink to the main checkout.
|
||||
@@ -1,580 +0,0 @@
|
||||
# Log — relay "worktree-autodetect"
|
||||
|
||||
Append-only. One entry per leg. Do not read end-to-end unless `status.md` points you here.
|
||||
|
||||
---
|
||||
|
||||
## Leg 0 — Assessment, design, packet creation
|
||||
|
||||
**Runner:** assessment/design session
|
||||
**Outcome:** recommendation = **reduced scope**. Packet created. Relay parked pending
|
||||
human approval. No production code written.
|
||||
|
||||
### Feature request
|
||||
|
||||
> "auto detect and show new worktrees, even when created outside of pi-web."
|
||||
|
||||
With the user's framing: nice-to-have, not a must, expects it may not be feasible, and
|
||||
**must require zero user intervention**.
|
||||
|
||||
### What I found in the codebase
|
||||
|
||||
The single most important finding reframed the whole feature:
|
||||
|
||||
**Worktree discovery is already fully dynamic. There is no cache and no registry.**
|
||||
|
||||
`WorkspaceService.list()` (`src/server/workspaces/workspaceService.ts`, 44 lines) calls
|
||||
`isGitRepository()` then `discoverGitWorktrees()` — which shells out to
|
||||
`git worktree list --porcelain` — on **every single** `GET /projects/:projectId/workspaces`
|
||||
request. I grepped for any cache/memo in that path: there is none. Workspace ids are
|
||||
derived by hashing `${project.id}:${worktree.path}`, so they are stable across calls
|
||||
without being stored anywhere.
|
||||
|
||||
And `projects.json` (`src/server/storage/projectStore.ts`) stores only
|
||||
`{ id, name, path, createdAt }` per **project**. Workspaces are never persisted.
|
||||
|
||||
Two consequences, both of which delete a large chunk of the anticipated problem:
|
||||
|
||||
1. **A worktree created outside PI WEB is already detected.** The server has no stale
|
||||
state to invalidate. The gap is not detection at all — it is that **the browser never
|
||||
re-asks**. `WorkspaceController` fetches workspaces in `selectProject()` and in
|
||||
`refreshProjectWorkspaces()`, and the only caller of the latter is the
|
||||
workspace-*deletion* flow. So the list is fetched on project selection and then frozen
|
||||
for the lifetime of that selection.
|
||||
2. **"Auto-adoption" is a non-question.** The brief asked whether zero-intervention
|
||||
detection implies zero-intervention adoption, and whether a discovered worktree should
|
||||
be a distinct "discovered" state. Since worktrees are derived and nothing is written to
|
||||
a registry, there is nothing to adopt and no state to distinguish. A new worktree is
|
||||
simply a member of the derived list. This was the design's biggest apparent risk and it
|
||||
evaporated on inspection.
|
||||
|
||||
Measured cost of the discovery itself: **~2ms**. 20 sequential
|
||||
`git worktree list --porcelain` runs on this repo took 42ms total.
|
||||
|
||||
I also found the delivery mechanism already built and already debounced:
|
||||
`src/client/src/appShell/browserResumeController.ts` listens to window `focus` and
|
||||
document `visibilitychange`, batches signals per animation frame, and collapses
|
||||
concurrent refreshes through `TrailingRefreshCoordinator`. It drives
|
||||
`PiWebApp.refreshAfterBrowserResume()`, which today refreshes the selected session,
|
||||
machine activities, and workspace-deletion runs. Workspace topology is conspicuously
|
||||
absent from that list.
|
||||
|
||||
And remote machines need no work: `GET /projects/:projectId/workspaces` is already in
|
||||
`FEDERATED_HTTP_ROUTES` (`src/shared/federatedRoutes.ts:25`) and `workspacesApi.workspaces`
|
||||
already takes a `machineId` and routes via `machinePrefix`.
|
||||
|
||||
### The inverse case, verified against real git
|
||||
|
||||
I built a throwaway repo in `/tmp/wtprobe` and checked what git actually reports.
|
||||
|
||||
- `.git/worktrees/` does not exist until the first linked worktree is added, then gains
|
||||
one directory per worktree.
|
||||
- After `rm -rf`ing a worktree's directory **without** `git worktree remove`,
|
||||
`git worktree list --porcelain` **still lists it**, with an extra line:
|
||||
`prunable gitdir file points to non-existent location`.
|
||||
- A locked worktree gets a bare valueless `locked` line.
|
||||
|
||||
The current parser ignores both keys. So **PI WEB today shows worktrees that no longer
|
||||
exist as normal, selectable workspaces** — a real bug, present regardless of whether the
|
||||
detection feature is built. Selecting one produces a workspace whose path does not exist.
|
||||
|
||||
### Options compared
|
||||
|
||||
**A. `git worktree list` on a timer (server or client poll).**
|
||||
Rejected. It is the obvious answer and it is the wrong one. A timer runs forever to catch
|
||||
an event that happens a few times a week, and it must run per project, per machine, or it
|
||||
does not actually satisfy "no intervention". For a nice-to-have, a permanent background
|
||||
cost to serve a rare event is exactly the trade the user warned against. It also has no
|
||||
natural interval: fast enough to feel automatic is wasteful, slow enough to be cheap is
|
||||
not noticeably better than the resume trigger, which is free.
|
||||
|
||||
**B. Watch `.git/worktrees/` with `fs.watch`/inotify.**
|
||||
This was the most interesting candidate and the one I most wanted to work. The watch
|
||||
target is genuinely small and precise — one directory in the main repo, one entry per
|
||||
worktree, written by git itself. That is far better than watching filesystems for new
|
||||
directories.
|
||||
|
||||
Rejected anyway, on cost and correctness:
|
||||
|
||||
- **Lifecycle ownership is the real problem, not the watcher.** A watcher must be created
|
||||
and destroyed as projects are added/removed, and it must live somewhere long-lived. The
|
||||
web/API process autoreloads (`pi-web-web-ui-dev.service`), so watchers there churn
|
||||
constantly. The natural long-lived home is the session daemon — but that would drag a
|
||||
purely presentational concern into session runtime ownership and, per `AGENTS.md`, make
|
||||
every change to it require a manual daemon restart. For a nice-to-have, that is a
|
||||
disproportionate architectural commitment.
|
||||
- **The directory does not exist until the first worktree exists**, so a repo with no
|
||||
linked worktrees needs a watch on `.git/` itself to catch `worktrees/` being created —
|
||||
a noisier target that fires on every ref update, index write, and fetch.
|
||||
- **It only fixes the local case.** Remote machines would need the event pushed over the
|
||||
machine transport, which means a new workspace-topology realtime event type, publishing
|
||||
it from the daemon, adding it to `FEDERATED_WEBSOCKET_ROUTES` plumbing, parsing it in
|
||||
`sessionSocket.ts`, and handling it in `PiWebApp`. That is a meaningful new protocol
|
||||
surface for a feature the user called optional.
|
||||
- **Environment caveats are real.** `fs.watch` is unreliable on Docker bind mounts on
|
||||
macOS/Windows (the repo ships `docker/compose.yml` with bind-mounted checkouts) and on
|
||||
network filesystems, and inotify watch limits are a known operational failure mode. So
|
||||
the "instant" promise would be silently broken for a subset of users — worse than an
|
||||
honest "updates when you come back to the tab".
|
||||
- **It still would not be enough.** `fs.watch` on `.git/worktrees` catches creation but
|
||||
the removal case still needs the `prunable` fix, because `rm -rf` of the *checkout*
|
||||
does not touch `.git/worktrees/<name>` at all — I verified the metadata directory
|
||||
survives. So the watcher does not even subsume the cheaper fix.
|
||||
|
||||
**C. Piggyback on events that already happen.** ← chosen
|
||||
The refresh already exists, is already debounced, already covers remote machines, and
|
||||
costs one ~2ms request per tab refocus. Marginal cost is as close to zero as this feature
|
||||
can get, and the code surface is ~60 production lines.
|
||||
|
||||
### Recommendation: reduced scope
|
||||
|
||||
Do the cheap 90%:
|
||||
|
||||
1. **Fix the inverse case** — filter `prunable` worktrees out of the workspace list.
|
||||
Read-only; PI WEB must not run `git worktree prune` as a side effect of listing.
|
||||
2. **Add a non-disruptive topology refresh** to `WorkspaceController` that re-lists the
|
||||
selected project's workspaces without touching selection or session state.
|
||||
3. **Call it from the existing resume path** — no new timer, watcher, process, or channel.
|
||||
|
||||
**Scope boundary, stated plainly:** detection is **resume-scoped, not instant**. A worktree
|
||||
created in another terminal while the PI WEB tab already has focus is not noticed until the
|
||||
tab is refocused or becomes visible again. That is the honest limit, and it should be
|
||||
documented rather than papered over.
|
||||
|
||||
Within that boundary it is genuinely zero-intervention: no button, no config, no opt-in,
|
||||
works on local and remote machines, works for creation and removal.
|
||||
|
||||
### The risk that could kill it
|
||||
|
||||
`WorkspaceController.selectWorkspace()` calls `clearActiveSession()` and
|
||||
`resetWorkspaceScopedState()`. If a background refresh routes through it, the user's chat
|
||||
is torn down every time they refocus the tab. The refresh must apply the list through
|
||||
`applyProjectWorkspaces` only. `ProjectActivityOwnershipCoordinator` is the existing
|
||||
precedent for background topology hydration that deliberately leaves selection alone, and
|
||||
is the model to follow. Leg 2 proves this with tests before leg 3 wires the trigger — and
|
||||
if it cannot be made non-disruptive, that is an explicit stop.
|
||||
|
||||
### What would change the recommendation
|
||||
|
||||
- If the user says instant-while-focused is actually required, option B comes back on the
|
||||
table — but with the session daemon commitment, the new realtime event type, and the
|
||||
Docker/network-filesystem caveats accepted as the price.
|
||||
- If a future need arises for reliable server-pushed workspace topology for another reason,
|
||||
the watcher becomes incremental rather than a feature-specific cost, and the ledger flips.
|
||||
|
||||
### Artifacts created
|
||||
|
||||
- `.pi-web/relays/worktree-autodetect/charter.md`
|
||||
- `.pi-web/relays/worktree-autodetect/status.md`
|
||||
- `.pi-web/relays/worktree-autodetect/plan.md`
|
||||
- `.pi-web/relays/worktree-autodetect/log.md` (this file)
|
||||
|
||||
### Checks run
|
||||
|
||||
None — no code was changed in this leg.
|
||||
|
||||
### Human decisions received at the end of leg 0
|
||||
|
||||
All three open questions were answered, and the reduced scope was approved:
|
||||
|
||||
1. **Latency** — resume-scoped detection is acceptable. No timer, no watcher.
|
||||
2. **Removed worktrees** — yes, hide worktrees whose directory is gone.
|
||||
3. **Sibling overlap** — assume the other session does nothing; this relay owns the
|
||||
refresh seam outright and should not design for sharing.
|
||||
|
||||
The human also asked for the `selectWorkspace` risk to be explained concretely. The
|
||||
worked failure mode is now recorded inline in `plan.md` → Leg 2, because the buggy version
|
||||
is the one that looks correct: re-resolving the selection after a refresh via
|
||||
`selectPreferredWorkspace` + `selectWorkspace` (mirroring `selectProject`) tears down the
|
||||
session on **every** browser resume, since `selectWorkspace` has no already-selected guard
|
||||
and always runs `clearActiveSession()` (closing the session socket mid-stream and dropping
|
||||
buffered deltas) plus `resetWorkspaceScopedState()` (clearing chat, file tree, open file,
|
||||
git status, open diff, terminal selection).
|
||||
|
||||
### Handing off?
|
||||
|
||||
**Yes.** Packet committed, `status.md` un-parked with the decisions recorded, leg 1
|
||||
dispatched via `spawn_session`.
|
||||
|
||||
---
|
||||
|
||||
## Leg 1 — Stop reporting removed worktrees (server truth)
|
||||
|
||||
**Commit:** `266f941` — `fix(workspaces): hide worktrees whose checkout directory is gone`
|
||||
|
||||
### What I did
|
||||
|
||||
1. **Parser reports facts.** `src/server/workspaces/gitWorktreeDiscovery.ts` now reads the
|
||||
`prunable` and `locked` keys. Extracted the pure parsing step into an exported
|
||||
`parseGitWorktreeList(stdout)`; `discoverGitWorktrees` is now just the `execFile`
|
||||
boundary plus that call. `GitWorktreeInfo` gained optional `prunable` / `locked`.
|
||||
2. **Service decides policy.** `src/server/workspaces/workspaceService.ts` filters prunable
|
||||
linked worktrees in a small private `selectable()` step, keeping the entry whose path
|
||||
equals `project.path` unconditionally. Existing "no worktrees" fallback then also covers
|
||||
the case where every listed worktree was filtered away, so a project can never present
|
||||
an empty workspace list.
|
||||
3. **Injected the git boundary.** `WorkspaceService` now takes an optional
|
||||
`WorkspaceGitPort` (`{ isGitRepository, discoverGitWorktrees }`) defaulting to the real
|
||||
implementation. This is what made the policy testable without a real repo or a
|
||||
subclass-override fake, and it left every existing `new WorkspaceService()` call site
|
||||
(`app.ts`, `app.testSupport.ts`, `sessiond.ts`) untouched.
|
||||
4. **Two new test files** (the plan predicted one; the policy assertions belong next to the
|
||||
service that owns them, not next to the parser):
|
||||
- `gitWorktreeDiscovery.test.ts` — parser only, fixtures captured verbatim from real git.
|
||||
- `workspaceService.test.ts` — prunable hidden, locked kept, project path kept even when
|
||||
marked prunable, fallback when everything is filtered, plus pre-existing labeling and
|
||||
non-git behavior pinned so leg 2/3 have a regression net.
|
||||
|
||||
### Decisions and why
|
||||
|
||||
- **Facts in the parser, policy in the service**, as `plan.md` preferred. The words
|
||||
`prunable` and `project.path` now appear in exactly one place each.
|
||||
- **Constructor injection rather than subclass-override fakes.** `workspaceDeletionRoutes.test.ts`
|
||||
fakes `WorkspaceService` by subclassing and overriding `list()`, which cannot test `list()`
|
||||
itself. A narrow port is the smaller seam and keeps the production default unchanged.
|
||||
- **Read-only, as mandated.** No `git worktree prune`, no repo metadata mutation anywhere.
|
||||
- **`locked` is parsed but deliberately not acted on.** A locked worktree is a real checkout
|
||||
and stays selectable. It is surfaced now because git emits it in the same records and
|
||||
future UI may want it; adding it later would mean touching the parser again.
|
||||
- **Re-verified the git behavior** rather than trusting it: created a throwaway repo,
|
||||
`rm -rf`'d a linked worktree and locked another. Confirmed `prunable gitdir file points to
|
||||
non-existent location` and `locked keep me`. Test fixtures are that exact output. Probe
|
||||
repo deleted afterwards.
|
||||
- **No API/type change.** `prunable`/`locked` never leave the server, so the shared
|
||||
`Workspace` type and the client are untouched. Leg 2 needs no type work.
|
||||
|
||||
### Checks run
|
||||
|
||||
- `npm test -- --run src/server/workspaces/gitWorktreeDiscovery.test.ts src/server/workspaces/workspaceService.test.ts` → 10 passed
|
||||
- `npm test -- --run src/server/workspaces/` → 12 files, 88 passed
|
||||
- `npm test -- --run src/server/app.projects.test.ts` → 5 passed (workspace list route contract)
|
||||
- `npm run typecheck` → clean
|
||||
- `npx eslint` on all four changed files → clean
|
||||
- pre-commit `verify:staged` → cached typecheck, knip, eslint, 14 related test files / 63 tests, all green
|
||||
|
||||
`npm run verify` was not run; per `plan.md` that is leg 3's gate.
|
||||
|
||||
### Artifacts changed
|
||||
|
||||
- `src/server/workspaces/gitWorktreeDiscovery.ts` (modified)
|
||||
- `src/server/workspaces/workspaceService.ts` (modified)
|
||||
- `src/server/workspaces/gitWorktreeDiscovery.test.ts` (new)
|
||||
- `src/server/workspaces/workspaceService.test.ts` (new)
|
||||
- `status.md` (leg tracking → last completed 1 / next 2, next task set to leg 2, added a
|
||||
note about the new `WorkspaceGitPort` seam)
|
||||
|
||||
No changeset yet — charter allows it any time up to leg 3, and leg 3 owns one fragment for
|
||||
the whole user-visible behavior.
|
||||
|
||||
### Blockers
|
||||
|
||||
None. Nothing ambiguous, no design decision needed, no intervention trigger fired.
|
||||
|
||||
### Handing off?
|
||||
|
||||
**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`.
|
||||
|
||||
---
|
||||
|
||||
## Leg 3 — wire the refresh to browser resume, document, changeset, full verify (final)
|
||||
|
||||
### What I did
|
||||
|
||||
Wired the leg-2 refresh into the two existing refresh paths, documented the behavior,
|
||||
added the changeset, and ran the full `npm run verify` the charter requires of the last leg.
|
||||
|
||||
1. **Wiring (`src/client/src/components/PiWebApp.ts`, 2 lines).** Added
|
||||
`this.workspaces.refreshSelectedProjectTopology()` to the `Promise.all` in
|
||||
`refreshAfterBrowserResume` (~438) and in `refreshAppData` (~496). Nothing else in that
|
||||
2300-line file was read or touched, per the charter's reading discipline.
|
||||
2. **Wiring test (`src/client/src/components/PiWebApp.workspaceTopology.test.ts`, new,
|
||||
3 tests).** Verifies the refresh is invoked by the exact callback
|
||||
`BrowserResumeController` calls (`browserResume.callbacks.refreshAfterResume`, not a
|
||||
hand-picked method reference), by `refreshAppData`, and that it still runs when a sibling
|
||||
refresh in the same batch rejects. Sibling refreshes are stubbed so the assertions observe
|
||||
only the topology call.
|
||||
3. **Docs (`docs/faq.html`).** New FAQ entry `#worktree-list-out-of-date` plus its TOC link:
|
||||
worktrees are listed on demand and never registered; the list is re-read on tab
|
||||
focus/visibility rather than continuously, so refocusing updates it; selection/session/
|
||||
scroll are preserved; worktrees with a missing checkout directory are hidden and git still
|
||||
tracks them until `git worktree prune`. Placed in the FAQ per
|
||||
`.agents/skills/documentation-guide/SKILL.md` (troubleshooting/edge-case content);
|
||||
`README.md` deliberately untouched.
|
||||
4. **Changeset (`.changeset/worktree-autodetect.md`).** `patch` for `@jmfederico/pi-web`
|
||||
(CalVer: patch is correct for a non-breaking user-facing capability), written as user
|
||||
behavior rather than an implementation log.
|
||||
|
||||
### Decisions and why
|
||||
|
||||
- **Did not add the `connectRealtime` `onReconnect` call.** `plan.md` marked it optional
|
||||
"only if it costs nothing". It is not free: `onReconnect` captures the machine id at
|
||||
connect time, while `refreshSelectedProjectTopology` reads the selected machine at call
|
||||
time, so wiring them together would either need a machine-scoped variant or would fire a
|
||||
refresh for a machine the user has since left. Adding a second concept for no user-visible
|
||||
gain over the resume path failed the cost test. Socket reconnect on resume is already
|
||||
covered by the resume path itself.
|
||||
- **Did not pass a message-prefixing `onBackgroundError` sink from `PiWebApp`.** Leg 2 made
|
||||
it optional with a `console.warn` default that already includes project and machine in the
|
||||
message. `status.md` explicitly recorded this as not-required work; adding it would be
|
||||
scope creep for identical output.
|
||||
- **Verified the wiring test is not vacuous.** Removed both call sites with `perl`, re-ran the
|
||||
file: 3/3 failed. Restored the file (verified 2 occurrences back) and re-ran: 3/3 passed.
|
||||
- **No charter boundary crossed.** No timer, watcher, process, endpoint, or push channel;
|
||||
no change inside `refreshSelectedProjectTopology`; leg 2's tests were not weakened; the
|
||||
resume path re-derives no selection from the refreshed list.
|
||||
|
||||
### Checks run
|
||||
|
||||
- `npx vitest --run src/client/src/components/PiWebApp.workspaceTopology.test.ts` →
|
||||
**3 passed** (first run had 1 failure from a stub-shape mistake in my own test helper,
|
||||
fixed by failing one named sibling refresh instead of all of them)
|
||||
- mutation check (call sites removed) → **3 failed**, as intended; restored → **3 passed**
|
||||
- `git diff --check` → clean
|
||||
- `npx eslint` on the new test and `PiWebApp.ts` → **clean**
|
||||
- **`npm run verify` → green**: typecheck, lint, knip, and 228 test files /
|
||||
1840 passed, 2 skipped
|
||||
- pre-commit `npm run verify:staged` → typecheck (cached), knip, eslint, related vitest
|
||||
(6 files / 20 tests) all **passed**
|
||||
|
||||
### Artifacts changed
|
||||
|
||||
- `src/client/src/components/PiWebApp.ts` (2 lines added)
|
||||
- `src/client/src/components/PiWebApp.workspaceTopology.test.ts` (new, 3 tests)
|
||||
- `docs/faq.html` (new FAQ entry + TOC link)
|
||||
- `.changeset/worktree-autodetect.md` (new)
|
||||
- committed as `84545fb feat(workspaces): refresh worktrees on browser resume`
|
||||
- `status.md` rewritten as a finished-relay baton: finish-line conditions checked off one by
|
||||
one with their commits, leg tracking set to last completed 3 / next none, shipped behavior
|
||||
described in user terms, and the never-route-through-`selectWorkspace` invariant recorded
|
||||
for whoever edits this code next
|
||||
|
||||
### Blockers
|
||||
|
||||
None. No intervention signal fired in this leg or any earlier one. No sessiond code touched,
|
||||
so **no manual session daemon restart is required**; the change lands on the autoreloading
|
||||
web/UI service path only.
|
||||
|
||||
### Handing off?
|
||||
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
## Post-relay — review nits 2 and 3 (locked flag, overlapping refreshes)
|
||||
|
||||
Human asked to fix nit 2 and to fold in nit 3 if it belonged to the same group. It did:
|
||||
both are small, contained changes to code this branch already introduced, and neither adds a
|
||||
concept. Done as two commits because they are different concerns.
|
||||
|
||||
### Nit 2 — `8f060d1` remove the unused `locked` flag
|
||||
|
||||
I had defended keeping it in the previous review, arguing it documented the keep-locked
|
||||
policy. That was wrong on inspection: the *test input* documents the policy, not the field.
|
||||
Removed `GitWorktreeInfo.locked` and its parse line. The parser test still feeds a real
|
||||
`locked keep me` porcelain line and now asserts it is ignored, and the service test still
|
||||
pins that a present non-prunable worktree is kept. So the policy is covered with less surface,
|
||||
and unknown porcelain keys were already ignored anyway.
|
||||
|
||||
### Nit 3 — `f8c3fdb` serialize overlapping topology refreshes
|
||||
|
||||
**This was not the cosmetic nit I rated it. It was a correctness bug**, and probing it first
|
||||
is what revealed that. Scratch reproduction: two `refreshSelectedProjectTopology()` calls
|
||||
issued 2 HTTP requests; resolving the newer one first and the older one second left
|
||||
`state.workspaces` at the *older* list. So a worktree created outside PI WEB could appear and
|
||||
then vanish again, which is precisely the bug this whole branch exists to fix. The machine and
|
||||
project stale guards do not order responses.
|
||||
|
||||
Fixed by routing the refresh through `TrailingRefreshCoordinator` keyed by
|
||||
`machineProjectKey(machineId, project.id)` — the primitive already used by
|
||||
`browserResumeController`, `sessionController`, and `activityController`, so this is reuse,
|
||||
not a new mechanism.
|
||||
|
||||
**Learned mid-implementation:** the coordinator does *not* collapse overlapping calls into one
|
||||
request. It runs the second as a single trailing pass after the first finishes. My first test
|
||||
asserted `toHaveBeenCalledOnce()` and hung for 5s until the vitest timeout. The code was
|
||||
right and my assumption was wrong, so I rewrote the test to assert the real and stronger
|
||||
guarantee: max-in-flight is 1, and the last response applied is the newest. Recorded in
|
||||
`status.md` so the next person does not repeat the mistaken assertion.
|
||||
|
||||
### Checks run
|
||||
|
||||
- `npx vitest --run workspaceController.test.ts src/server/workspaces/` → **98 passed**
|
||||
(10 in `workspaceController.test.ts`)
|
||||
- mutation check for nit 3: bypassed the coordinator by invoking the refresh body directly →
|
||||
the new serialization test failed, **alone**; restored → all pass
|
||||
- `grep` for leftover `locked` references → only the intentional test input/comments
|
||||
- `npx eslint` on all four changed files → clean
|
||||
- **`npm run verify` → green**: 228 files, **1843 passed**, 2 skipped
|
||||
- pre-commit `verify:staged` on each commit → 14 files/63 tests and 6 files/27 tests, passed
|
||||
|
||||
### Artifacts changed
|
||||
|
||||
- `src/server/workspaces/gitWorktreeDiscovery.ts` (removed `locked` field + parse line)
|
||||
- `src/server/workspaces/gitWorktreeDiscovery.test.ts`,
|
||||
`src/server/workspaces/workspaceService.test.ts` (retargeted to the policy, not the field)
|
||||
- `src/client/src/controllers/workspaceController.ts` (+`TrailingRefreshCoordinator`)
|
||||
- `src/client/src/controllers/workspaceController.test.ts` (+1 test, 10 total)
|
||||
- `status.md`: commit list, verify count, and the coordinator-semantics warning
|
||||
|
||||
### Blockers
|
||||
|
||||
None. No changeset added: `f8c3fdb` fixes a defect in unreleased work from this same branch,
|
||||
and the existing fragment already promises the list stays correct without user action. Still
|
||||
no timer, watcher, process, endpoint, or push channel, and no sessiond code touched, so
|
||||
**no manual session daemon restart is required**.
|
||||
@@ -1,168 +0,0 @@
|
||||
# Implementation plan — worktree-autodetect (reduced scope)
|
||||
|
||||
Three legs. Each is a vertical slice: source + tests + checks + commit.
|
||||
|
||||
The order is deliberate: server truth first, then client application of that truth,
|
||||
then the trigger that makes it zero-intervention.
|
||||
|
||||
---
|
||||
|
||||
## Leg 1 — Stop reporting removed worktrees (the inverse case)
|
||||
|
||||
**Why first:** it is independently valuable, has zero UI risk, and is the only part of
|
||||
the feature that is a straight bug fix. Today a worktree deleted with `rm -rf` outside
|
||||
PI WEB stays in the workspace list forever as a selectable ghost.
|
||||
|
||||
**Files**
|
||||
|
||||
- `src/server/workspaces/gitWorktreeDiscovery.ts`
|
||||
- new `src/server/workspaces/gitWorktreeDiscovery.test.ts`
|
||||
|
||||
**Work**
|
||||
|
||||
1. Extend the porcelain parser to read the valueless `prunable` and `locked` keys.
|
||||
`git worktree list --porcelain` emits `prunable <reason>` for a linked worktree whose
|
||||
checkout directory no longer exists, and a bare `locked` line for a locked one.
|
||||
Verified in leg 0 against real git.
|
||||
2. Surface `prunable` on `GitWorktreeInfo`, and filter prunable entries out of what
|
||||
`discoverGitWorktrees` returns — or return them and filter in `WorkspaceService`,
|
||||
whichever keeps the parser honest and the policy visible. Prefer: parser reports
|
||||
facts, `workspaceService` decides policy.
|
||||
3. Do **not** run `git worktree prune`. Read-only. PI WEB must not mutate the user's
|
||||
repo metadata as a side effect of listing.
|
||||
4. Keep the main worktree unconditionally: never filter the entry whose path equals
|
||||
`project.path`, so a project can never end up with an empty workspace list.
|
||||
|
||||
**Tests** (pure parser tests, no git process needed — inject or fake the exec boundary)
|
||||
|
||||
- parses `prunable` with a reason and `locked` without a value
|
||||
- a prunable linked worktree is excluded from the workspace list
|
||||
- a locked worktree is still included
|
||||
- the main worktree survives even if git somehow marks it prunable
|
||||
|
||||
**Checks:** `npm test -- --run src/server/workspaces/gitWorktreeDiscovery.test.ts`,
|
||||
plus the workspaceService/app.projects tests if they touch the shape, plus
|
||||
`npm run typecheck` (`GitWorktreeInfo` is exported).
|
||||
|
||||
---
|
||||
|
||||
## Leg 2 — Non-disruptive workspace topology refresh in the client
|
||||
|
||||
**Why second:** this is the risky part, and it must be provably non-disruptive before
|
||||
anything starts calling it automatically.
|
||||
|
||||
**Files**
|
||||
|
||||
- `src/client/src/controllers/workspaceController.ts`
|
||||
- new `src/client/src/controllers/workspaceController.test.ts`
|
||||
|
||||
**Work**
|
||||
|
||||
1. Add a method — suggested name `refreshSelectedProjectTopology()` — that re-lists the
|
||||
selected project's workspaces and applies them via the existing
|
||||
`applyProjectWorkspaces` path.
|
||||
2. **Selection invariants it must hold:**
|
||||
- If the currently selected workspace is still present, do **not** call
|
||||
`selectWorkspace`, do **not** clear the active session, do **not** reset
|
||||
workspace-scoped state. Only `workspaces` / `workspacesByProjectId` change.
|
||||
|
||||
**Read this before writing the method — the wrong version looks correct.** The
|
||||
tempting shape, mirroring `selectProject()` six lines above it, is: refresh the list,
|
||||
then "re-resolve the selection to be safe" via
|
||||
`selectPreferredWorkspace(...)` + `await this.selectWorkspace(...)`. That is the bug.
|
||||
`selectWorkspace` has **no already-selected guard**, so even when it re-picks the very
|
||||
same workspace it unconditionally runs:
|
||||
- `sessions.clearActiveSession()` → `socket.close()` (closes the session WebSocket
|
||||
mid-stream), `clearPendingUpdates()`, `streamWatermark = undefined` (buffered deltas
|
||||
dropped), and `setState({ selectedSession: undefined, messages: [] })` (chat empties);
|
||||
- `setState({ ...resetWorkspaceScopedState() })` → clears `sessions`, `fileTree`,
|
||||
`expandedDirs`, `selectedFilePath`, `selectedFileContent`, `gitStatus`,
|
||||
`selectedDiffPath`, `selectedDiff`, `selectedStagedDiff`, `selectedTerminalId`.
|
||||
|
||||
Because leg 3 calls this from `refreshAfterBrowserResume`, that would fire on **every**
|
||||
alt-tab back into PI WEB — not only when a worktree actually changed — blanking the
|
||||
chat, collapsing the file tree, and closing any open diff every time, and losing stream
|
||||
deltas that arrive while the socket is down. Applying the list via
|
||||
`applyProjectWorkspaces` alone is sufficient for the feature; `handleWorkspaceChange`
|
||||
early-returns when the selected workspace id is unchanged, so a fresh-but-equal list
|
||||
causes no downstream churn on its own.
|
||||
- If nothing is selected, just apply the list.
|
||||
- If the selected workspace **disappeared**, do not silently jump. Leave the
|
||||
selection as-is and let the existing deletion path own recovery; the user is
|
||||
currently working there and a surprise switch is worse than a stale label.
|
||||
If leg 2 finds this cannot be left alone safely, that is the intervention signal.
|
||||
3. Guard against machine/project changing mid-flight, exactly like `selectProject` does
|
||||
(compare `selectedMachineId` and `selectedProject?.id` before applying).
|
||||
4. Swallow-and-report errors the way sibling background refreshes do (`console.warn`,
|
||||
not `setState({ error })`) — a background topology refresh must never paint an error
|
||||
banner over a working session.
|
||||
|
||||
**Tests** (controller-layer, fake `api.workspaces`)
|
||||
|
||||
- a newly appeared worktree lands in `workspaces` and `workspacesByProjectId`
|
||||
- the selected workspace is preserved; `sessions.clearActiveSession` is **not** called
|
||||
- a stale response for a project the user has since left is discarded
|
||||
- a rejected request does not set `state.error`
|
||||
|
||||
**Checks:** `npm test -- --run src/client/src/controllers/workspaceController.test.ts`.
|
||||
|
||||
---
|
||||
|
||||
## Leg 3 — Wire it to the existing resume path, document, changeset
|
||||
|
||||
**Why last:** only after leg 2 proves the refresh is inert.
|
||||
|
||||
**Files**
|
||||
|
||||
- `src/client/src/components/PiWebApp.ts` — `refreshAfterBrowserResume` (~432) and
|
||||
`refreshAppData` (~485). Touch only these two methods.
|
||||
- possibly `src/client/src/components/PiWebApp.*.test.ts` (a focused new test file is fine)
|
||||
- `docs/` — one short paragraph where workspaces/worktrees are explained; follow
|
||||
`.agents/skills/documentation-guide/SKILL.md` and do **not** grow `README.md`
|
||||
- `.changeset/*.md`
|
||||
|
||||
**Work**
|
||||
|
||||
1. Add `this.workspaces.refreshSelectedProjectTopology()` to the `Promise.all` in
|
||||
`refreshAfterBrowserResume` and to `refreshAppData`.
|
||||
- `BrowserResumeController` already debounces per animation frame and collapses
|
||||
concurrent requests through `TrailingRefreshCoordinator`, so no extra throttling
|
||||
is needed. Verified in leg 0.
|
||||
- This inherits remote-machine support for free: `api.workspaces(projectId, machineId)`
|
||||
already routes through the machine proxy, and `/projects/:projectId/workspaces` is
|
||||
already in `FEDERATED_HTTP_ROUTES`.
|
||||
2. Optionally also refresh on realtime-socket reconnect (`connectRealtime`'s
|
||||
`onReconnect`), which is the same class of natural event. Only if it costs nothing.
|
||||
Add the call directly; this relay owns the seam and is not coordinating with any
|
||||
other branch.
|
||||
3. Document the behavior honestly: detection happens when the tab regains focus /
|
||||
becomes visible, not instantly.
|
||||
4. Add the changeset (`npm run changeset`, or write the fragment directly).
|
||||
|
||||
**Checks:** the new/affected client tests, then **`npm run verify`** — this is the final
|
||||
leg and the change is cross-cutting.
|
||||
|
||||
---
|
||||
|
||||
## Cost ledger (accepted in leg 0)
|
||||
|
||||
| Cost | Amount |
|
||||
|---|---|
|
||||
| New processes | 0 |
|
||||
| New watchers (inotify/fs.watch) | 0 |
|
||||
| New timers | 0 |
|
||||
| New endpoints / push channels | 0 |
|
||||
| Extra request per browser resume, per selected project | 1 (~2ms of `git worktree list` server-side) |
|
||||
| Production lines changed | ~60 |
|
||||
| New test files | 3 |
|
||||
|
||||
## Known risks
|
||||
|
||||
- **UI churn on refresh.** Mitigated by leg 2's invariants and its tests. This is the
|
||||
one that can kill the feature; it is an explicit intervention trigger.
|
||||
- **Latency expectation.** Detection is resume-scoped. A user staring at an already-focused
|
||||
tab while a worktree appears in another window sees nothing until they refocus. This is
|
||||
an accepted, documented limit — not a bug to fix with a timer.
|
||||
- **Overlap with the sibling `worktree-create-ui` effort.** Settled by the human: assume
|
||||
that session does nothing. This relay owns the refresh seam; build it here without
|
||||
designing for reuse, and do not read that worktree.
|
||||
@@ -1,98 +0,0 @@
|
||||
# Status — relay "worktree-autodetect"
|
||||
|
||||
## 🏁 FINISHED — the relay reached its finish line
|
||||
|
||||
All three legs are complete and committed on `feat/worktree-autodetect`. `npm run verify` is
|
||||
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 (1843 after the two follow-up fixes below).
|
||||
|
||||
The other two review nits were then fixed as well, after the human asked:
|
||||
|
||||
- `8f060d1` removed the unused `GitWorktreeInfo.locked` flag. The parser test still feeds a
|
||||
real `locked` porcelain line and asserts it is ignored, so the keep-locked-worktrees policy
|
||||
stays covered without a speculative field.
|
||||
- `f8c3fdb` fixed what turned out to be a **correctness bug, not the cosmetic nit I first
|
||||
rated it**. The resume path and the plugin-facing app refresh call
|
||||
`refreshSelectedProjectTopology` independently, and the stale guards check machine and
|
||||
project but *not ordering* — so a slower earlier response landing last overwrote a newer
|
||||
list and a just-created worktree disappeared again. Reproduced with a scratch test before
|
||||
fixing. Now routed through `TrailingRefreshCoordinator` keyed by machine+project.
|
||||
|
||||
Note for anyone extending this: `TrailingRefreshCoordinator` does **not** collapse two
|
||||
overlapping calls into one HTTP request. It runs the second as one trailing pass after the
|
||||
first completes, so the newest response is always applied last. An assertion of
|
||||
`toHaveBeenCalledOnce()` will hang against it.
|
||||
|
||||
## Current position
|
||||
|
||||
Every charter finish-line condition is satisfied:
|
||||
|
||||
1. ✅ **Prunable worktrees hidden** — `266f941`. `discoverGitWorktrees` parses `prunable`;
|
||||
`WorkspaceService` filters those worktrees out, while always keeping the project's own
|
||||
worktree so a project is never empty.
|
||||
2. ✅ **Non-disruptive client refresh** — `d0f8f9f`.
|
||||
`WorkspaceController.refreshSelectedProjectTopology()` applies results through
|
||||
`applyProjectWorkspaces` only, never `selectWorkspace`, so selection, session, file tree,
|
||||
git status, and terminal selection survive. A vanished selected workspace is left alone
|
||||
for the existing deletion path to handle.
|
||||
3. ✅ **Wired to the existing resume path** — `84545fb`. Called from
|
||||
`PiWebApp.refreshAfterBrowserResume` and `refreshAppData`. No new timer, watcher,
|
||||
process, endpoint, or push channel. Remote machines work through the existing
|
||||
`machinePrefix` + `FEDERATED_HTTP_ROUTES` plumbing.
|
||||
4. ✅ **Tests** — prunable parsing/filtering (`gitWorktreeDiscovery.test.ts`,
|
||||
`workspaceService.test.ts`), refresh-preserves-selection and
|
||||
refresh-when-selected-workspace-disappeared (`workspaceController.test.ts`, 7 tests),
|
||||
resume + app-data wiring (`PiWebApp.workspaceTopology.test.ts`, 3 tests).
|
||||
5. ✅ **`npm run verify` green** (228 files / 1843 passed, 2 skipped) and
|
||||
`.changeset/worktree-autodetect.md` exists.
|
||||
|
||||
Nothing in this work touched `src/server/sessiond.ts`, session runtime ownership, or the
|
||||
daemon protocol. **No manual session daemon restart is required.**
|
||||
|
||||
## Leg tracking
|
||||
|
||||
- **Last completed leg:** 3 (final — wiring, docs, changeset, full verify)
|
||||
- **Next leg to run:** none. Relay complete; do not spawn another runner.
|
||||
|
||||
## Shipped behavior, as a user sees it
|
||||
|
||||
A worktree created or deleted outside PI WEB shows up in (or disappears from) the workspace
|
||||
list the next time the browser tab regains focus or becomes visible. Detection is
|
||||
resume-scoped, not instant, by explicit human decision. A worktree whose checkout directory
|
||||
was `rm -rf`ed no longer appears as a selectable workspace. Documented in `docs/faq.html`
|
||||
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), `79577e4` (selected-workspace freshness fix),
|
||||
`8f060d1` (drop unused `locked`), `f8c3fdb` (serialize overlapping refreshes),
|
||||
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
|
||||
socket and blank the chat on every browser resume. `workspaceController.test.ts` was
|
||||
mutation-checked against exactly that regression in leg 2.
|
||||
- The relay packet lives under `.pi-web/`, which is gitignored; packet commits used
|
||||
`git add -f`.
|
||||
|
||||
## Blockers
|
||||
|
||||
None. No intervention signal fired in any leg. The one serious known risk — visible UI churn
|
||||
on refresh — did not materialize and needed no workaround.
|
||||
Reference in New Issue
Block a user