#48 made every change revertible; this makes that reachable. The bar the issue set: after the agent rearranges a bed, undoing it should take one obvious click, not a hunt through a menu.
The rail-layout decision (the part #53 and #57 depend on)
Four things wanted one strip of screen — inspector, history, journal (#53), chat (#57). They're tabs in one EditorRail rather than each bolting on its own chrome. The canvas stays one width instead of a different width per panel, and adding the journal or chat later is adding a tab.
Both constraints from the issue held:
Selecting an object still lands you in the inspector without a click. The page watches the selection and switches tabs itself, so the rail never becomes a thing you operate before you can edit. Deselecting drops back out of the inspector but leaves History open if that's where you were — that tab isn't about the selection.
The canvas stays worth watching while the agent edits it. The rail is a fixed 20rem column that closes completely when nothing needs it.
On a phone the same tabs render in the bottom sheet the inspector already lived in, so mobile didn't need a second layout.
Honest reporting is most of the work here
A reverted entry stays in the list, struck through and marked, and the revert appears as its own entry — because that's what it is. Making the original disappear would be rewriting history rather than appending to it, and would leave no way to undo the undo.
A 409 from a revert is not a plain failure. It carries the change set that did apply alongside the entities deliberately left alone, so the message is:
2 of 3 changes undone — “North Bed” was edited since, so it was left alone.
A bare failure toast would be a lie about the two that applied. A bare success would hide the one that didn't. Every conflict is listed, not just the first, and each reason (changed / missing / exists / unsupported) gets its own phrasing.
The panel also says plainly that deleting a whole garden isn't covered and can't be undone, rather than leaving that to be discovered — matching the deliberate gap documented in #48.
One undo, not two
The issue asked for a single implementation shared with #57's inline undo. useUndo owns the mutation, the per-change-set outcome and the phrasing; UndoButton renders it. #57 will call the same hook, so undo behaves identically wherever it's offered — including how it explains a partial result.
Verification
64 tests (9 new), covering the phrasing logic that carries the user-facing behaviour: pluralization per entity, "says nothing rather than 0 changes", named vs unnamed conflicts, and all three undo outcomes (clean / partial / nothing-applied). tsc --noEmit, npm run build, go test ./... green.
I'll exercise the real loop — move a bed, undo it, watch the canvas redraw — against the live deploy once this merges, and report back on the PR.
Closes #49. Completes Phase 0 of #58.
#48 made every change revertible; this makes that reachable. The bar the issue set: after the agent rearranges a bed, undoing it should take **one obvious click**, not a hunt through a menu.
### The rail-layout decision (the part #53 and #57 depend on)
Four things wanted one strip of screen — inspector, history, journal (#53), chat (#57). They're **tabs in one `EditorRail`** rather than each bolting on its own chrome. The canvas stays one width instead of a different width per panel, and adding the journal or chat later is adding a tab.
Both constraints from the issue held:
- **Selecting an object still lands you in the inspector without a click.** The page watches the selection and switches tabs itself, so the rail never becomes a thing you operate *before* you can edit. Deselecting drops back out of the inspector but leaves History open if that's where you were — that tab isn't about the selection.
- **The canvas stays worth watching while the agent edits it.** The rail is a fixed 20rem column that closes completely when nothing needs it.
On a phone the same tabs render in the bottom sheet the inspector already lived in, so mobile didn't need a second layout.
### Honest reporting is most of the work here
A reverted entry **stays in the list**, struck through and marked, and the revert appears as its own entry — because that's what it is. Making the original disappear would be rewriting history rather than appending to it, and would leave no way to undo the undo.
A 409 from a revert **is not a plain failure**. It carries the change set that *did* apply alongside the entities deliberately left alone, so the message is:
> 2 of 3 changes undone — “North Bed” was edited since, so it was left alone.
A bare failure toast would be a lie about the two that applied. A bare success would hide the one that didn't. Every conflict is listed, not just the first, and each reason (`changed` / `missing` / `exists` / `unsupported`) gets its own phrasing.
The panel also says plainly that **deleting a whole garden isn't covered and can't be undone**, rather than leaving that to be discovered — matching the deliberate gap documented in #48.
### One undo, not two
The issue asked for a single implementation shared with #57's inline undo. `useUndo` owns the mutation, the per-change-set outcome and the phrasing; `UndoButton` renders it. #57 will call the same hook, so undo behaves identically wherever it's offered — including how it explains a partial result.
### Verification
64 tests (9 new), covering the phrasing logic that carries the user-facing behaviour: pluralization per entity, "says nothing rather than 0 changes", named vs unnamed conflicts, and all three undo outcomes (clean / partial / nothing-applied). `tsc --noEmit`, `npm run build`, `go test ./...` green.
I'll exercise the real loop — move a bed, undo it, watch the canvas redraw — against the live deploy once this merges, and report back on the PR.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
#48 made every change revertible; this makes that reachable. The bar was that
undoing what the agent just did should take one obvious click, not a hunt.
Settles the rail-layout question, which is the part #53 and #57 depend on. Four
things wanted one strip of screen — inspector, history, journal, chat — so they
are tabs in one EditorRail rather than each bolting on its own chrome. That
keeps the canvas at one width instead of a different width per panel, and adding
the journal or chat later is adding a tab.
Two constraints held. Selecting an object still lands you in the inspector with
no extra click: the page watches the selection and switches tabs itself, so the
rail never becomes something you operate before you can edit. And the canvas
stays worth watching while the agent edits it — the rail is a fixed 20rem column
that closes completely when nothing needs it. On a phone the same tabs render in
the bottom sheet the inspector already lived in.
A reverted entry stays in the list, struck through and marked, and the revert
appears as its own entry — because that is what it is. Making the original
disappear would be rewriting history rather than appending to it, and would
leave no way to undo the undo.
Conflicts are reported as what actually happened. A 409 from a revert is not a
plain failure: it carries the change set that DID apply alongside the entities
deliberately left alone, so the message is "2 of 3 changes undone — “North Bed”
was edited since, so it was left alone" rather than a generic toast. A bare
failure would be a lie about the two that applied; a bare success would hide the
one that didn't.
Undo is one implementation, not two: useUndo owns the mutation, the per-change-
set outcome and the phrasing, and UndoButton renders it. #57's inline undo on an
agent turn uses the same hook, so undo behaves identically wherever it appears.
The panel says plainly that deleting a whole garden isn't covered and can't be
undone, rather than leaving that to be discovered.
Closes#49
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-21 05:21:13Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Blocking issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — No material issues found
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Blocking issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ⚠️ **performance** — could not complete
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 19 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 19 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
- **State:** TanStack Query for all server state (editor keyed on `gardens/:id/full`; optimistic mutations with version-conflict rollback). One small Zustand store for ephemeral editor state only: viewport, selection, focused object, active tool, in-flight drag.
- **Editor components (`web/src/editor/`):**`GardenCanvas` (svg root + viewport g), `useViewport` (use-gesture pan/zoom/pinch), `ObjectShape`, `PlopMarker` (semantic-zoom branching), `Palette` (drag-to-place object kinds), `Inspector` (side panel; bottom sheet on mobile), `PlantPicker`. Pure geometry helpers (local↔world transforms, unit formatting) in `web/src/lib/geometry.ts`, unit-tested.
- **Editor components (`web/src/editor/`):**`GardenCanvas` (svg root + viewport g), `useViewport` (use-gesture pan/zoom/pinch), `ObjectShape`, `PlopMarker` (semantic-zoom branching), `Palette` (drag-to-place object kinds), `EditorRail` (the one side panel), `Inspector`, `HistoryPanel`, `PlantPicker`.
- **One rail, tabs inside it.** The inspector, history, and later the journal and chat panel all want the same strip of screen; rather than each bolting on its own chrome they are tabs in `EditorRail` — so the canvas is one width instead of a different width per panel, and adding a panel is adding a tab. Selecting an object switches to the Inspector tab automatically, so the rail is never something you operate before you can edit; on a phone the same tabs render in the bottom sheet the inspector already used. Pure geometry helpers (local↔world transforms, unit formatting) in `web/src/lib/geometry.ts`, unit-tested.
web/src/editor/store.ts:97-106 — resetTransient omits the new railTab field, contradicting its own "clear all transient state" comment. Confirmed: GardenEditorPage.tsx's parallel clear() (lines 86-94) does call setRailTab(null), but resetTransient (called from PublicGardenPage.tsx:28) does not. - web/src/editor/HistoryPanel.tsx:118 — relativeTime is a pure, dependency-free helper exported from a component file rather than web/src/lib/, breaking the project's established l…
🪰 Gadfly · advisory
🟡 **VERDICT: Minor issues**
_maintainability · flagged by 1 model_
- `web/src/editor/store.ts:97-106` — `resetTransient` omits the new `railTab` field, contradicting its own "clear all transient state" comment. Confirmed: `GardenEditorPage.tsx`'s parallel `clear()` (lines 86-94) does call `setRailTab(null)`, but `resetTransient` (called from `PublicGardenPage.tsx:28`) does not. - `web/src/editor/HistoryPanel.tsx:118` — `relativeTime` is a pure, dependency-free helper exported from a component file rather than `web/src/lib/`, breaking the project's established l…
<sub>🪰 Gadfly · advisory</sub>
⚪badge render condition stacks three ad-hoc truthiness checks instead of a named helper
maintainability · flagged by 1 model
web/src/editor/EditorRail.tsx:69 — badge render condition stacks three ad-hoc truthiness checks.tab.badge != null && tab.badge !== false && tab.badge !== 0 (line 69) is three stacked checks to express "a count, or a plain marker, never shown for zero/false." A named helper (e.g. hasBadge) or normalizing the badge type to number | true | undefined at the RailTab interface would make the intent legible at the call site. Severity trivial, high confidence.
🪰 Gadfly · advisory
⚪ **badge render condition stacks three ad-hoc truthiness checks instead of a named helper**
_maintainability · flagged by 1 model_
- **`web/src/editor/EditorRail.tsx:69` — `badge` render condition stacks three ad-hoc truthiness checks.** `tab.badge != null && tab.badge !== false && tab.badge !== 0` (line 69) is three stacked checks to express "a count, or a plain marker, never shown for zero/false." A named helper (e.g. `hasBadge`) or normalizing the `badge` type to `number | true | undefined` at the `RailTab` interface would make the intent legible at the call site. Severity trivial, high confidence.
<sub>🪰 Gadfly · advisory</sub>
🟡HistoryPanel re-derives sets and per-entry counts on every render with no memoization
performance · flagged by 1 model
HistoryPanel re-runs flatMap over all loaded pages on every render (web/src/editor/HistoryPanel.tsx:19). sets is recomputed each render from history.data?.pages, and each entry maps to a HistoryEntry that calls describeCounts/totalChanges (reduce over counts). With offset paging at 30/page and a typical user scrolling a page or two this is negligible; but describeCounts and totalChanges are re-derived per render per entry with no memoization, and the parent re-renders o…
🪰 Gadfly · advisory
🟡 **HistoryPanel re-derives sets and per-entry counts on every render with no memoization**
_performance · flagged by 1 model_
- **`HistoryPanel` re-runs `flatMap` over all loaded pages on every render** (`web/src/editor/HistoryPanel.tsx:19`). `sets` is recomputed each render from `history.data?.pages`, and each entry maps to a `HistoryEntry` that calls `describeCounts`/`totalChanges` (reduce over counts). With offset paging at 30/page and a typical user scrolling a page or two this is negligible; but `describeCounts` and `totalChanges` are re-derived per render per entry with no memoization, and the parent re-renders o…
<sub>🪰 Gadfly · advisory</sub>
🟡history.isError shows "Could not load history" even when only the next page failed, not the initial load
error-handling, maintainability · flagged by 1 model
web/src/editor/HistoryPanel.tsx:26 — misleading error message when only the next page fails.history.isError becomes true on a fetchNextPage failure too (TanStack sets error/isError for any fetch, not just the initial one). When the user clicks "Load older" and that request fails, the panel renders <Alert>Could not load history.</Alert> above the already-loaded entries — which reads as "the whole history failed," even though the older entries are still on screen and retryable…
🪰 Gadfly · advisory
🟡 **history.isError shows "Could not load history" even when only the next page failed, not the initial load**
_error-handling, maintainability · flagged by 1 model_
- **`web/src/editor/HistoryPanel.tsx:26` — misleading error message when only the *next* page fails.** `history.isError` becomes true on a `fetchNextPage` failure too (TanStack sets `error`/`isError` for any fetch, not just the initial one). When the user clicks "Load older" and that request fails, the panel renders `<Alert>Could not load history.</Alert>` above the already-loaded entries — which reads as "the whole history failed," even though the older entries are still on screen and retryable…
<sub>🪰 Gadfly · advisory</sub>
🟠Pagination failure silently swallowed — no user feedback when fetching older history fails
error-handling · flagged by 2 models
web/src/editor/HistoryPanel.tsx:45 — Pagination failure is silently swallowed.history.fetchNextPage() is fire-and-forget via void. If the network fails while loading older history, the button stops spinning and no error is shown to the user. TanStack Query exposes isFetchNextPageError / fetchNextPageError for this; the component ignores them. Fix: Wrap the call, catch errors, and surface them in the panel (e.g., a small inline alert or a toast).
🪰 Gadfly · advisory
🟠 **Pagination failure silently swallowed — no user feedback when fetching older history fails**
_error-handling · flagged by 2 models_
- **`web/src/editor/HistoryPanel.tsx:45` — Pagination failure is silently swallowed.** `history.fetchNextPage()` is fire-and-forget via `void`. If the network fails while loading older history, the button stops spinning and no error is shown to the user. TanStack Query exposes `isFetchNextPageError` / `fetchNextPageError` for this; the component ignores them. **Fix:** Wrap the call, catch errors, and surface them in the panel (e.g., a small inline alert or a toast).
<sub>🪰 Gadfly · advisory</sub>
🟡totalChanges recomputed per entry when describeCounts already walked the same counts
maintainability · flagged by 1 model
web/src/editor/HistoryPanel.tsx:104 — totalChanges recomputed per entry despite describeCounts already walking the same counts.HistoryEntry computes counts = describeCounts(changeSet) at line 70 (which filters n > 0), then the undo-button guard at line 104 calls totalChanges(changeSet) — a second full reduce over cs.counts (history.ts:115). Both helpers are already imported; a single-pass "has any change" check would suffice. Minor churn, easy to mistake for doing diff…
🪰 Gadfly · advisory
🟡 **totalChanges recomputed per entry when describeCounts already walked the same counts**
_maintainability · flagged by 1 model_
- **`web/src/editor/HistoryPanel.tsx:104` — `totalChanges` recomputed per entry despite `describeCounts` already walking the same counts.** `HistoryEntry` computes `counts = describeCounts(changeSet)` at line 70 (which filters `n > 0`), then the undo-button guard at line 104 calls `totalChanges(changeSet)` — a second full `reduce` over `cs.counts` (`history.ts:115`). Both helpers are already imported; a single-pass "has any change" check would suffice. Minor churn, easy to mistake for doing diff…
<sub>🪰 Gadfly · advisory</sub>
correctness, maintainability · flagged by 3 models
web/src/editor/store.ts:97-106 — resetTransient omits the new railTab field, contradicting its own "clear all transient state" comment. Confirmed: GardenEditorPage.tsx's parallel clear() (lines 86-94) does call setRailTab(null), but resetTransient (called from PublicGardenPage.tsx:28) does not. - web/src/editor/HistoryPanel.tsx:118 — relativeTime is a pure, dependency-free helper exported from a component file rather than web/src/lib/, breaking the project's established l…
🪰 Gadfly · advisory
🟡 **VERDICT: Minor issues**
_correctness, maintainability · flagged by 3 models_
- `web/src/editor/store.ts:97-106` — `resetTransient` omits the new `railTab` field, contradicting its own "clear all transient state" comment. Confirmed: `GardenEditorPage.tsx`'s parallel `clear()` (lines 86-94) does call `setRailTab(null)`, but `resetTransient` (called from `PublicGardenPage.tsx:28`) does not. - `web/src/editor/HistoryPanel.tsx:118` — `relativeTime` is a pure, dependency-free helper exported from a component file rather than `web/src/lib/`, breaking the project's established l…
<sub>🪰 Gadfly · advisory</sub>
🟡relativeTime misreports future dates as 'just now'
error-handling · flagged by 1 model
web/src/editor/HistoryPanel.tsx:128 — relativeTime returns "just now" for any timestamp in the future (e.g. server clock skew), because the negative seconds value falls through the first < 60 check. Future dates should be handled explicitly rather than misreporting them as the recent past.
🪰 Gadfly · advisory
🟡 **relativeTime misreports future dates as 'just now'**
_error-handling · flagged by 1 model_
- `web/src/editor/HistoryPanel.tsx:128` — `relativeTime` returns `"just now"` for any timestamp in the future (e.g. server clock skew), because the negative `seconds` value falls through the first `< 60` check. Future dates should be handled explicitly rather than misreporting them as the recent past.
<sub>🪰 Gadfly · advisory</sub>
🟠changeCountSchema allows negative counts, which can hide undo buttons or produce incorrect totals
error-handling · flagged by 1 model
web/src/lib/history.ts:21 — n in changeCountSchema accepts negative numbers.z.number() allows negatives. A server bug returning n: -1 would silently distort totalChanges (possibly dropping below zero and hiding the undo button) while describeCounts filters negatives out entirely. Fix: Tighten to z.number().int().nonnegative() so a bad payload fails loudly at parse time instead of producing confusing UI.
🪰 Gadfly · advisory
🟠 **changeCountSchema allows negative counts, which can hide undo buttons or produce incorrect totals**
_error-handling · flagged by 1 model_
- **`web/src/lib/history.ts:21` — `n` in `changeCountSchema` accepts negative numbers.** `z.number()` allows negatives. A server bug returning `n: -1` would silently distort `totalChanges` (possibly dropping below zero and hiding the undo button) while `describeCounts` filters negatives out entirely. **Fix:** Tighten to `z.number().int().nonnegative()` so a bad payload fails loudly at parse time instead of producing confusing UI.
<sub>🪰 Gadfly · advisory</sub>
🔴Unconstrained z.number() in changeCountSchema allows invalid server data to hide undo button or show fractional counts
error-handling · flagged by 1 model
web/src/lib/history.ts:27 — changeCountSchema uses z.number() without .int() or .nonnegative(), allowing any numeric value (negative, fractional, NaN, Infinity) from a misbehaving server to pass validation. totalChanges sums these blindly, so a negative or NaN count causes totalChanges(changeSet) > 0 to evaluate to false and silently hides the Undo button, while describeCounts would render strings like "2.5 plantings added" for fractional values. The schema should c…
🪰 Gadfly · advisory
🔴 **Unconstrained z.number() in changeCountSchema allows invalid server data to hide undo button or show fractional counts**
_error-handling · flagged by 1 model_
- `web/src/lib/history.ts:27` — `changeCountSchema` uses `z.number()` without `.int()` or `.nonnegative()`, allowing any numeric value (negative, fractional, `NaN`, `Infinity`) from a misbehaving server to pass validation. `totalChanges` sums these blindly, so a negative or `NaN` count causes `totalChanges(changeSet) > 0` to evaluate to false and silently hides the **Undo** button, while `describeCounts` would render strings like `"2.5 plantings added"` for fractional values. The schema should c…
<sub>🪰 Gadfly · advisory</sub>
🟡useRevertChangeSet and revertConflicts are exported but only used within history.ts
maintainability · flagged by 1 model
web/src/lib/history.ts:97 and :86 — unnecessarily exported.useRevertChangeSet and revertConflicts are both exported, but grep confirms each is only called from within history.ts itself (useRevertChangeSet at line 168, revertConflicts at lines 108 and 178; no test or component imports either). They don't need to be part of the module's public surface. Dropping the export keeps the API surface to what useUndo/describeUndo/the schemas actually offer. Small, but it's exa…
🪰 Gadfly · advisory
🟡 **useRevertChangeSet and revertConflicts are exported but only used within history.ts**
_maintainability · flagged by 1 model_
- **`web/src/lib/history.ts:97` and `:86` — unnecessarily exported.** `useRevertChangeSet` and `revertConflicts` are both `export`ed, but grep confirms each is only called from within `history.ts` itself (`useRevertChangeSet` at line 168, `revertConflicts` at lines 108 and 178; no test or component imports either). They don't need to be part of the module's public surface. Dropping the `export` keeps the API surface to what `useUndo`/`describeUndo`/the schemas actually offer. Small, but it's exa…
<sub>🪰 Gadfly · advisory</sub>
🟠409 with unparseable body skips cache refresh despite likely partial write
correctness, error-handling, performance · flagged by 3 models
web/src/lib/history.ts:107-108 — a 409 whose body fails to parse skips the refresh, even though partial work likely applied. The top-level mutation's onError only calls refresh() when revertConflicts(err) is truthy, i.e. only when the 409 body successfully parses against revertResultSchema. But a 409 is by this PR's own design the "part of it applied" path. If the server returns a 409 with a body that omits the changeSet key (the schema requires it; there's no .default) or…
🪰 Gadfly · advisory
🟠 **409 with unparseable body skips cache refresh despite likely partial write**
_correctness, error-handling, performance · flagged by 3 models_
- **`web/src/lib/history.ts:107-108` — a 409 whose body fails to parse skips the refresh, even though partial work likely applied.** The top-level mutation's `onError` only calls `refresh()` when `revertConflicts(err)` is truthy, i.e. only when the 409 body *successfully* parses against `revertResultSchema`. But a 409 is by this PR's own design the "part of it applied" path. If the server returns a 409 with a body that omits the `changeSet` key (the schema requires it; there's no `.default`) or…
<sub>🪰 Gadfly · advisory</sub>
🟡Comments reference #57 as 'inline undo' but GardenEditorPage/DESIGN describe #57 as the chat panel — misleading issue references
maintainability · flagged by 1 model
web/src/lib/history.ts:161-162 and web/src/editor/UndoButton.tsx:5 — stale/contradictory issue references. The comment in history.ts says the shared undo is "shared by the history list and (per #49) the agent turn's inline Undo in #57", and UndoButton's doc comment says "the agent turn's inline undo (#57)". But GardenEditorPage.tsx:289 (and the PR's DESIGN.md change) describe #57 as the chat panel ("the journal (#53) and chat (#57) panels too"). So the code comments point a…
🪰 Gadfly · advisory
🟡 **Comments reference #57 as 'inline undo' but GardenEditorPage/DESIGN describe #57 as the chat panel — misleading issue references**
_maintainability · flagged by 1 model_
- **`web/src/lib/history.ts:161-162` and `web/src/editor/UndoButton.tsx:5` — stale/contradictory issue references.** The comment in `history.ts` says the shared undo is "shared by the history list and (per #49) the agent turn's inline Undo in #57", and `UndoButton`'s doc comment says "the agent turn's inline undo (#57)". But `GardenEditorPage.tsx:289` (and the PR's `DESIGN.md` change) describe #57 as the **chat panel** ("the journal (#53) and chat (#57) panels too"). So the code comments point a…
<sub>🪰 Gadfly · advisory</sub>
correctness, maintainability, performance · flagged by 2 models
web/src/lib/history.ts:169 + web/src/editor/EditorRail.tsx:85 — useUndo stores per-change-set outcomes in local useState, but EditorRail mounts only the active tab (active.render()). If a user clicks Undo, switches to the Inspector tab while the mutation is in flight, and later returns to History, HistoryPanel remounts with a fresh useUndo instance. The pending/success/error outcome is lost, so the undo button appears clickable again even though the revert mutation may…
🪰 Gadfly · advisory
🟠 **useUndo outcomes lost when History tab unmounts**
_correctness, maintainability, performance · flagged by 2 models_
- **`web/src/lib/history.ts:169` + `web/src/editor/EditorRail.tsx:85`** — `useUndo` stores per-change-set outcomes in local `useState`, but `EditorRail` mounts only the active tab (`active.render()`). If a user clicks **Undo**, switches to the Inspector tab while the mutation is in flight, and later returns to History, `HistoryPanel` remounts with a fresh `useUndo` instance. The pending/success/error outcome is lost, so the undo button appears clickable again even though the revert mutation may…
<sub>🪰 Gadfly · advisory</sub>
🟡A 409 with an unparseable body is reported as a flat failure even though part of the revert may have applied
error-handling · flagged by 1 model
🪰 Gadfly · advisory
🟡 **A 409 with an unparseable body is reported as a flat failure even though part of the revert may have applied**
_error-handling · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
🟡useUndo returns isPending but no caller uses it (dead public API)
maintainability · flagged by 2 models
web/src/lib/history.ts:192 — isPending is dead public API.useUndo returns isPending: revert.isPending, but no caller uses it. UndoButton keys its disabled/loading state off outcome?.tone === 'pending' (verified by grepping isPending/outcomeFor/.undo( usages across web/src; every other isPending reference is on a different query/mutation, not the undo hook). The field is exported surface that nothing reads. Either wire it up (e.g. disable all undo buttons while any re…
🪰 Gadfly · advisory
🟡 **useUndo returns isPending but no caller uses it (dead public API)**
_maintainability · flagged by 2 models_
- **`web/src/lib/history.ts:192` — `isPending` is dead public API.** `useUndo` returns `isPending: revert.isPending`, but no caller uses it. `UndoButton` keys its disabled/loading state off `outcome?.tone === 'pending'` (verified by grepping `isPending`/`outcomeFor`/`.undo(` usages across `web/src`; every other `isPending` reference is on a different query/mutation, not the undo hook). The field is exported surface that nothing reads. Either wire it up (e.g. disable all undo buttons while any re…
<sub>🪰 Gadfly · advisory</sub>
🔴describeUndo reports 'Undone.' on a backend no-op (changeSet: null, conflicts: []), reachable via 'Undo again' on a create-type entry whose entity is already gone
correctness, error-handling · flagged by 2 models
Finding 1 (web/src/lib/history.ts:209-211): The draft claims describeUndo returns "Undone." in ok tone when nothing was actually undone. Confirmed: describeUndo at line 209 has if (result.conflicts.length === 0) return { tone: 'ok', message: 'Undone.' }. The backend history.go:73-76 returns 200 with changeSet: nil when "every revision resolved to a no-op." In that case conflicts is empty (len 0), so the UI reports "Undone." in ok tone — even though nothing was reverted. This…
🪰 Gadfly · advisory
🔴 **describeUndo reports 'Undone.' on a backend no-op (changeSet: null, conflicts: []), reachable via 'Undo again' on a create-type entry whose entity is already gone**
_correctness, error-handling · flagged by 2 models_
**Finding 1 (`web/src/lib/history.ts:209-211`):** The draft claims `describeUndo` returns `"Undone."` in ok tone when nothing was actually undone. Confirmed: `describeUndo` at line 209 has `if (result.conflicts.length === 0) return { tone: 'ok', message: 'Undone.' }`. The backend `history.go:73-76` returns 200 with `changeSet: nil` when "every revision resolved to a no-op." In that case `conflicts` is empty (len 0), so the UI reports "Undone." in ok tone — even though nothing was reverted. This…
<sub>🪰 Gadfly · advisory</sub>
🟠Rail auto-switch-to-Inspector effect keys off a boolean, so it misses selection changes while already selected
correctness · flagged by 3 models
web/src/pages/GardenEditorPage.tsx:123-126 — selection-change id does not auto-switch to Inspector. The effect that auto-switches the rail tab depends only on the hasSelectionboolean, not on the actual selection ids: ts const hasSelection = selectedId != null || selectedPlantingId != null useEffect(() => { if (hasSelection) setRailTab('inspector') else if (useEditorStore.getState().railTab === 'inspector') setRailTab(null) }, [hasSelection, setRailTab]) Because hasSelection…
🪰 Gadfly · advisory
🟠 **Rail auto-switch-to-Inspector effect keys off a boolean, so it misses selection changes while already selected**
_correctness · flagged by 3 models_
* **`web/src/pages/GardenEditorPage.tsx:123-126` — selection-change id does not auto-switch to Inspector.** The effect that auto-switches the rail tab depends only on the `hasSelection` *boolean*, not on the actual selection ids: ```ts const hasSelection = selectedId != null || selectedPlantingId != null useEffect(() => { if (hasSelection) setRailTab('inspector') else if (useEditorStore.getState().railTab === 'inspector') setRailTab(null) }, [hasSelection, setRailTab]) ``` Because `hasSelection`…
<sub>🪰 Gadfly · advisory</sub>
🟠Closing rail from History tab clears canvas selection
correctness · flagged by 1 model
web/src/pages/GardenEditorPage.tsx:407 — EditorRail's onClose unconditionally calls select(null) and selectPlanting(null). Closing the rail from the History tab (where selection is irrelevant) still clears the user's canvas selection. A user viewing history for the currently selected object loses that selection when they close the panel, forcing them to re-select before they can keep editing. The close handler should only clear selection when the active tab is inspector, or…
🪰 Gadfly · advisory
🟠 **Closing rail from History tab clears canvas selection**
_correctness · flagged by 1 model_
- **`web/src/pages/GardenEditorPage.tsx:407`** — `EditorRail`'s `onClose` unconditionally calls `select(null)` and `selectPlanting(null)`. Closing the rail from the **History** tab (where selection is irrelevant) still clears the user's canvas selection. A user viewing history for the currently selected object loses that selection when they close the panel, forcing them to re-select before they can keep editing. The close handler should only clear selection when the active tab is `inspector`, or…
<sub>🪰 Gadfly · advisory</sub>
Verdict: Blocking issues found · 21 findings (8 with multi-model agreement)
Finding
Where
Models
Lens
🟠
409 with unparseable body skips cache refresh despite likely partial write
web/src/lib/history.ts:101
3/5
correctness, error-handling, performance
🟠
Rail auto-switch-to-Inspector effect keys off a boolean, so it misses selection changes while already selected
web/src/pages/GardenEditorPage.tsx:123
3/5
correctness
🟡
VERDICT: Minor issues
web/src/editor/HistoryPanel.tsx:118
3/5
correctness, maintainability
🔴
describeUndo reports 'Undone.' on a backend no-op (changeSet: null, conflicts: []), reachable via 'Undo again' on a create-type entry whose entity is already gone
web/src/lib/history.ts:209
2/5
correctness, error-handling
🟠
Pagination failure silently swallowed — no user feedback when fetching older history fails
web/src/editor/HistoryPanel.tsx:45
2/5
error-handling
🟠
resetTransient doesn't clear the new railTab state, breaking the one-place-clears-all transient-state pattern
web/src/editor/store.ts:97
2/5
maintainability
🟠
useUndo outcomes lost when History tab unmounts
web/src/lib/history.ts:169
2/5
correctness, maintainability, performance
🟡
useUndo returns isPending but no caller uses it (dead public API)
web/src/lib/history.ts:192
2/5
maintainability
13 single-model findings (lower confidence)
Finding
Where
Model
Lens
🔴
Unconstrained z.number() in changeCountSchema allows invalid server data to hide undo button or show fractional counts
web/src/lib/history.ts:27
kimi-k2.6:cloud
error-handling
🟠
changeCountSchema allows negative counts, which can hide undo buttons or produce incorrect totals
web/src/lib/history.ts:21
opencode/kimi-k2.6:cloud
error-handling
🟠
Closing rail from History tab clears canvas selection
web/src/pages/GardenEditorPage.tsx:407
kimi-k2.6:cloud
correctness
🟡
VERDICT: Minor issues
DESIGN.md:109
claude-code/sonnet
maintainability
🟡
VERDICT: Minor issues
PublicGardenPage.tsx:28
claude-code/sonnet
maintainability
🟡
HistoryPanel re-derives sets and per-entry counts on every render with no memoization
web/src/editor/HistoryPanel.tsx:18
glm-5.2:cloud
performance
🟡
history.isError shows "Could not load history" even when only the next page failed, not the initial load
web/src/editor/HistoryPanel.tsx:23
opencode/glm-5.2:cloud
error-handling, maintainability
🟡
totalChanges recomputed per entry when describeCounts already walked the same counts
web/src/editor/HistoryPanel.tsx:104
glm-5.2:cloud
maintainability
🟡
relativeTime misreports future dates as 'just now'
web/src/editor/HistoryPanel.tsx:128
kimi-k2.6:cloud
error-handling
🟡
useRevertChangeSet and revertConflicts are exported but only used within history.ts
web/src/lib/history.ts:97
opencode/glm-5.2:cloud
maintainability
🟡
Comments reference #57 as 'inline undo' but GardenEditorPage/DESIGN describe #57 as the chat panel — misleading issue references
web/src/lib/history.ts:161
opencode/glm-5.2:cloud
maintainability
🟡
A 409 with an unparseable body is reported as a flat failure even though part of the revert may have applied
web/src/lib/history.ts:178
opencode/glm-5.2:cloud
error-handling
⚪
badge render condition stacks three ad-hoc truthiness checks instead of a named helper
web/src/editor/EditorRail.tsx:69
glm-5.2:cloud
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Blocking issues found
This backend authz behaves correctly (existence masking preserved, viewer/editor role gating enforced independently of client state). This diff is frontend-only (no backend changes), and the backend endpoints it consumes were already reviewed under #48. My security-lens review of this diff specifically:
VERDICT: No material issues found
Findings, all verified:
Authz is server-enforced, not client-trusted.canEdit (GardenEditorPage.tsx:74) gates the Undo button only for UX; the actual revert (internal/service/revisions.go:281-291) independently requires roleEditor via requireGardenRole, and history reads require roleViewer. A tampered client state can't escalate privilege.
No injection surface.historyKey/useGardenHistory build query params through api.get(..., { params }), which routes through buildUrl → URLSearchParams (web/src/lib/api.ts:47-57) — no string concatenation into the URL. gardenId/changeSetId used in path segments (web/src/lib/history.ts:59,73) are numeric, sourced from route params / server-issued ChangeSet.id, not free-form user text.
No XSS. All server-sourced strings that reach the DOM — changeSet.summary, actorName, RevertConflict.name (rendered via describeConflict, HistoryPanel.tsx, UndoButton.tsx) — are rendered as JSX text children, which React escapes; no dangerouslySetInnerHTML, no eval, no manual HTML construction anywhere in the new files.
Response validation.changeSetSchema/revertResultSchema/historyPageSchema (web/src/lib/history.ts) use zod to validate the shape of server responses before use, so a malformed/unexpected payload fails closed (schema parse throws / safeParse returns unsuccessful) rather than being trusted blindly.
409 body handling is safe.revertConflicts() (web/src/lib/history.ts:56-60) only trusts err.body after revertResultSchema.safeParse, so an unexpected error body can't be coerced into a fabricated "partial success" UI state.
Nothing in this diff introduces a new authn/authz gap, injection point, unsafe deserialization, or secret exposure. The one thing worth flagging as a pattern to watch (not a bug here): errorMessage() surfaces the raw server error message verbatim in the UI (HistoryPanel.tsx Alert, UndoButton.tsx/describeUndo fallback) — fine as long as the Go service never puts sensitive detail (row contents, internal IDs of entities the actor can't see) into writeServiceError messages, which is pre-existing behavior this PR doesn't change.
🎯 Correctness — Minor issues
Both findings check out exactly against the code. Line numbers, logic, and referenced files all confirmed by direct inspection.
VERDICT: Minor issues
web/src/pages/GardenEditorPage.tsx:122-126 — the rail auto-switch-to-Inspector effect keys off the derived boolean hasSelection (selectedId != null || selectedPlantingId != null), not the selection identity, so it only fires on a false → true transition. Confirmed: GardenCanvas.tsx:247 wires onSelect={select} directly to any object click, and store.ts:71 (select: (id) => set({ selectedId: id, selectedPlantingId: null })) never clears selection before setting a new id — so clicking bed B while bed A is already selected keeps hasSelectiontrue throughout and the effect's [hasSelection, setRailTab] deps don't re-run. Reproduction: select bed A (auto-switches to Inspector) → manually open History via the unconditional toggle at GardenEditorPage.tsx:345-351 → click bed B on the canvas. The rail stays on History instead of switching to Inspector for B, contradicting the documented invariant at lines 118-121 ("Selecting anything lands you in the inspector without a click"). Same gap crossing kinds: selectPlanting (store.ts:74) clears selectedId but sets selectedPlantingId, so hasSelection stays true when selecting a plop while History is open. Fix: depend on [selectedId, selectedPlantingId, setRailTab] instead of the boolean.
web/src/lib/history.ts:207-218 (describeUndo) — when the backend returns changeSet: null, conflicts: [] (the documented cs == nil / 200 "already undone by hand" no-op path at internal/api/history.go:73-76), the result.conflicts.length === 0 check short-circuits to { tone: 'ok', message: 'Undone.' } even though nothing was actually reverted this time. Confirmed this exact input combination isn't exercised by history.test.ts, which only covers conflicts-empty-with-a-real-changeSet (plain success), partial, and error-with-conflicts cases — the null-changeSet/empty-conflicts combination falls through untested to the same "Undone." message as a real revert.
🧹 Code cleanliness & maintainability — Minor issues
I independently verified both findings in the draft against the actual source — both survive:
VERDICT: Minor issues
web/src/editor/store.ts:97-106 — resetTransient omits the new railTab field, contradicting its own "clear all transient state" comment. Confirmed: GardenEditorPage.tsx's parallel clear() (lines 86-94) does call setRailTab(null), but resetTransient (called from PublicGardenPage.tsx:28) does not.
web/src/editor/HistoryPanel.tsx:118 — relativeTime is a pure, dependency-free helper exported from a component file rather than web/src/lib/, breaking the project's established lib+test convention (confirmed via DESIGN.md:109 and grep — it's used only within HistoryPanel.tsx and has no test).
Full corrected review written to the plan file. This was a read-only verification task (no code changes), so there's nothing to implement — the plan file just holds the finalized review output.
⚡ Performance — Minor issues
Good, that confirms the surrounding "nothing else stood out" commentary matches the actual code (lazy render via active.render() at line 87, render() only called for the active tab). The finding itself checks out.
Everything in the draft is verified against the actual source: useRevertChangeSet (history.ts:97-111) unconditionally invalidates historyKey(gardenId) on both success and 409-with-partial-apply, useGardenHistory is confirmed as an offset-paginated useInfiniteQuery (history.ts:68-83), and @tanstack/react-query is pinned at ^5.62.0 (web/package.json:19), whose documented default behavior on invalidating an active infinite query is to sequentially refetch every already-fetched page. Since HistoryPanel/UndoButton only mount while the History tab is active (EditorRail.tsx's lazy render()), the query is active exactly when a user clicks undo, so the finding holds.
VERDICT: Minor issues
web/src/lib/history.ts:97-111 — useRevertChangeSet's refresh() invalidates historyKey(gardenId) unconditionally on every undo (success or 409-with-partial-apply). useGardenHistory (history.ts:68-83) is a useInfiniteQuery (TanStack Query ^5.62.0, confirmed in web/package.json:19) using offset-based paging. TanStack Query's default behavior for an active infinite query is to refetch every already-loaded page sequentially on invalidation, not just the first page. Since the History tab (and thus the query) is only mounted while active — EditorRail renders tabs lazily — a user who has clicked "Load older" a few times will trigger an N-request cascade on every subsequent undo click, just to redraw the same list plus one new entry. Cost grows with how far the user has paginated. A cheap fix: after a successful/partial revert, only refetch the first page (e.g. qc.setQueryData to drop pages beyond the first, or history.refetch({ refetchPage: (_, index) => index === 0 })) rather than invalidating the whole infinite-query cache.
🧯 Error handling & edge cases — Blocking issues found
Review — 🧯 Error handling & edge cases
VERDICT: Blocking issues found
describeUndo reports "Undone." even when nothing was actually reverted
This treats "zero conflicts" as synonymous with "fully applied," but the backend contract (internal/api/history.go:28-30, 63-77) explicitly allows a 200 response with {changeSet: null, conflicts: []} — the comment there says exactly this: "ChangeSet is null when nothing needed reverting", and the handler's case cs == nil: branch returns 200 with that body.
Traced how that's reached in internal/service/revisions.go:462-466:
cur,err:=ops.get(ctx,r.EntityID)iferrors.Is(err,domain.ErrNotFound){ifr.Op==domain.OpCreate{returnnil,nil,nil// already gone: the inverse is a no-op, not a conflict}returnnil,conflict(r,domain.ConflictMissing,""),nil}
Only OpCreate gets this silent no-op treatment. This is concretely reachable via the "Undo again" affordance this PR ships: HistoryEntry (web/src/editor/HistoryPanel.tsx:104-111) renders the undo button unconditionally whenever canEdit && totalChanges(changeSet) > 0, with label={reverted ? 'Undo again' : 'Undo'} — it is not disabled or hidden for an already-reverted entry. Clicking it calls undo.undo(changeSet) (web/src/editor/UndoButton.tsx) on the original changeSet object, not the revert entry. RevertChangeSet (internal/service/revisions.go:281-331) has no guard on target.RevertedByID anywhere in the function.
So: create a bed → undo it (bed deleted) → click "Undo again" on the now-struck-through original entry → RevertChangeSet is invoked again on the original create changeset → its inverse (a removal) finds the entity already gone → hits the OpCreate/ErrNotFound no-op branch → no revisions recorded → commitScope returns (nil, nil) since sc.taken() is empty → handler returns 200 with {changeSet: null, conflicts: []} → describeUndo hits the zero-conflict branch and returns "Undone.", even though nothing happened.
The same early-return also means any mixed change set where one create sub-revision silently no-ops while other revisions genuinely apply is reported as a plain "Undone." — applied is never compared against target's total in the conflicts.length === 0 branch, so a partial-but-silently-short revert is indistinguishable from a full one.
web/src/lib/history.test.ts's "is a plain success when nothing conflicted" case supplies changeSet: changeSet({ id: 2 }) (non-null) — the changeSet: null, conflicts: [] case is not exercised by any test.
Fix suggestion: in describeUndo, when conflicts.length === 0, compare totalChanges(result.changeSet ?? …) against totalChanges(target); if result.changeSet is null or under-counts relative to target, report a "nothing to undo" / partial state instead of an unconditional "Undone."
I traced the undo flow end-to-end through the security lens and found no material gaps:
Authz is enforced server-side, not just by canEdit.RevertChangeSet calls requireGardenRole(..., roleEditor) (internal/service/revisions.go:286) and GardenHistory requires roleViewer (internal/service/revisions.go:244). The frontend canEdit gate on the UndoButton is purely cosmetic; a forged/incorrect value can't bypass the backend. Verified by reading both service methods.
No injection surface. The two new endpoints take only numeric IDs (parseIDParam on :id, intQuery on limit/offset in internal/api/history.go:34-41), and the client builds paths from number-typed gardenId/changeSetId only (web/src/lib/history.ts:71,94). No string interpolation into URLs or queries.
Untrusted server data is rendered safely.changeSet.summary, actorName, and conflict name flow into JSX as text children (HistoryPanel.tsx, UndoButton.tsx) or into template strings inside describeConflict/describeCounts — never via dangerouslySetInnerHTML. React escapes text content, so no stored-XSS via a crafted summary/name.
Response parsing is validated.historyPageSchema.parse and revertResultSchema.parse validate shapes (history.ts:74,95); the 409 path uses safeParse (revertConflicts, history.ts:84) so a malformed conflict body degrades to a generic error rather than crashing or trusting raw data.
No credential/secret exposure in the new files; requests use credentials: 'same-origin' via the existing api wrapper (api.ts:97), unchanged.
One non-security note I confirmed is fine: useGardenHistory runs regardless of canEdit, but the backend's viewer-role check authorizes that, so a shared viewer legitimately seeing history is intended behavior — not a leak.
No findings to report for this lens.
🎯 Correctness — Minor issues
Both findings verified against the actual code.
Verdict: Minor issues
web/src/pages/GardenEditorPage.tsx:118-126 — The selection auto-switch effect's comment claims it "leaves History/Chat open if that's where you were" when you deselect, but it does not, after a selection round-trip. Trace: user is on the History tab (railTab === 'history', no selection). They select a bed → hasSelection true → effect sets railTab = 'inspector' (line 124). They deselect → hasSelection false, and useEditorStore.getState().railTab === 'inspector' → effect sets railTab = null (line 125). The rail closes; it does not return to History. The store only keeps a single railTab (no previousTab), and the deselect branch unconditionally clears to null whenever the current tab is 'inspector'. So the "leave History open" guarantee only holds if you never selected anything while there; the moment you select, History is dropped on deselect. This contradicts the code's own comment (lines 119-121) and the PR description. Fix: remember the pre-inspector tab and restore to it (e.g. 'history') on deselect rather than null, or track a previousTab.
web/src/lib/history.ts:207-217 (unverified edge, not blocking) — describeUndo uses totalChanges(target) as the denominator and totalChanges(result.changeSet) as the numerator. This is correct as long as the revert's counts map to the target's revisions. The one fragile case: a target whose counts array is empty/zero while conflicts are non-empty would render "0 of 0 changes undone — …" as partial rather than error, because the applied === 0 branch (line 213) only triggers when result.changeSet is null (line 212). Whether a non-null result.changeSet can ever carry zero counts alongside conflicts depends on server-side counts population, which I could not confirm in this pass. Low-confidence.
🧹 Code cleanliness & maintainability — Minor issues
All findings verified. Now confirming the editor page (GardenEditorPage) does clear railTab in its own unmount/mount effect, but resetTransient (used by PublicGardenPage) does not — confirming finding 1.
Let me verify finding 2's claim about the useUndo outcomes growth and finding 3/4 details are all confirmed from what I've read.
All four findings are confirmed against the actual code:
resetTransient omits railTab — Confirmed. store.ts:97-106 resets all other transient fields but not railTab. PublicGardenPage.tsx:28 calls resetTransient() on mount, so a stale railTab survives. (Note: GardenEditorPage.tsx:93 does clear railTab in its own separate effect, so only the public page / the resetTransient contract is affected — still a real inconsistency.)
useUndo outcomes grow unbounded — Confirmed. history.ts:169 holds outcomes in useState, each undo adds an entry keyed by id with no eviction (history.ts:172,175,179). The doc comment at line 165 even acknowledges "a single instance serves a whole list."
totalChanges recomputed per entry — Confirmed. HistoryPanel.tsx:70 calls describeCounts (which filters n > 0), then line 104 separately calls totalChanges(changeSet) (a full reduce over cs.counts, history.ts:115). Both walk counts.
EditorRail badge condition — Confirmed. EditorRail.tsx:69 (the draft said 62; the actual badge check is at line 69): tab.badge != null && tab.badge !== false && tab.badge !== 0.
Verdict: Minor issues
web/src/editor/store.ts:97 — resetTransient doesn't clear the new railTab state. Every other transient field (selectedId, selectedPlantingId, focusedObjectId, armedPlant, armedKind, liveObject, livePlanting) is reset in resetTransient, but railTab was left out. PublicGardenPage.tsx:28 calls resetTransient() on mount to enter a garden view fresh, so a railTab left from a previous garden (e.g. a stale 'inspector'/'history') survives the reset. (GardenEditorPage.tsx:93 clears railTab in its own mount/unmount effect, so the editor page itself isn't affected — but the shared resetTransient contract is now inconsistent.) Fix: add railTab: null to the set({...}) in resetTransient. Severity medium, high confidence.
web/src/lib/history.ts:169 — useUndo's outcomes record grows unbounded. Each undo adds an entry keyed by change-set id (history.ts:172,175,179) and never evicts one. The hook stays mounted across the whole paged history list (and, per the #57 inline-undo sharing noted in the file's own doc comment at line 165, can outlive a single list view), so outcomes grows with every undo action. Bounded in practice by user actions, so minor; worth a comment or scoping to visible entries. Severity small, medium confidence.
web/src/editor/HistoryPanel.tsx:104 — totalChanges recomputed per entry despite describeCounts already walking the same counts.HistoryEntry computes counts = describeCounts(changeSet) at line 70 (which filters n > 0), then the undo-button guard at line 104 calls totalChanges(changeSet) — a second full reduce over cs.counts (history.ts:115). Both helpers are already imported; a single-pass "has any change" check would suffice. Minor churn, easy to mistake for doing different things. Severity small, high confidence.
web/src/editor/EditorRail.tsx:69 — badge render condition stacks three ad-hoc truthiness checks.tab.badge != null && tab.badge !== false && tab.badge !== 0 (line 69) is three stacked checks to express "a count, or a plain marker, never shown for zero/false." A named helper (e.g. hasBadge) or normalizing the badge type to number | true | undefined at the RailTab interface would make the intent legible at the call site. Severity trivial, high confidence.
⚡ Performance — Minor issues
All three findings are confirmed against the actual code:
Outcomes unbounded — history.ts:169 confirms outcomes is a Record<number, UndoOutcome> that only accumulates via setOutcomes; there is no eviction logic anywhere.
Lazy render discards state — EditorRail.tsx renders only active.render(); HistoryPanel mounts useUndo (line 18), so tab-switch unmounts discard the hook's state.
Per-render re-derivation — HistoryPanel.tsx:19 recomputes sets via flatMap every render, and HistoryEntry calls describeCounts/totalChanges with no memoization.
The draft review is accurate. Outputting the corrected review unchanged.
Verdict: Minor issues
useUndo outcomes state grows unbounded and is never cleared (web/src/lib/history.ts:169-187). outcomes is a Record<number, UndoOutcome> keyed by change-set id that only ever accumulates — there is no eviction when entries scroll out of the visible list, no TTL, and the history itself is paged with flatMap over all loaded pages. After an undo on an older entry, that outcome lingers in component state for the life of the mounted HistoryPanel. Impact is small (history lists are short and ids are sparse integers), but it is a genuine unbounded-growth leak — a long-lived editor session that loads many pages and undoes several entries keeps every outcome forever. Suggested fix: clear outcomes[id] (or cap the record) when the change set is no longer in history.data, or simply store the outcome keyed to the currently visible set and drop stale keys on history refetch.
useUndo is instantiated per HistoryPanel mount, and HistoryPanel is rendered lazily inside the rail tab (web/src/editor/HistoryPanel.tsx:18, web/src/editor/EditorRail.tsx:71). Because EditorRail only renders active.render() for the active tab, switching away from the History tab unmounts HistoryPanel, and its useUndo state (including pending outcome) is discarded — switching back remounts and re-fetches. That is acceptable (it's the documented lazy-render contract), but it means a pending "Undoing…" outcome is lost on tab switch. Verified: EditorRail.tsx:71 renders only active.render(), and useUndo's state lives in the hook within HistoryPanel. Not a perf regression, just noting the in-flight outcome has no persistence across tab toggles; if persistence matters for #57's inline undo, this hook will need to lift the outcomes store out of the panel.
HistoryPanel re-runs flatMap over all loaded pages on every render (web/src/editor/HistoryPanel.tsx:19). sets is recomputed each render from history.data?.pages, and each entry maps to a HistoryEntry that calls describeCounts/totalChanges (reduce over counts). With offset paging at 30/page and a typical user scrolling a page or two this is negligible; but describeCounts and totalChanges are re-derived per render per entry with no memoization, and the parent re-renders on every useUndo state change (each setOutcomes triggers a re-render of the whole list). For a long list this is O(n) work on each undo click rather than O(1) for the one changed row. Suggested fix: memoize sets with useMemo and/or memoize HistoryEntry so an undo outcome update only re-renders the affected entry.
🧯 Error handling & edge cases — Minor issues
Both findings verify against the actual source. revertConflicts (history.ts:86-90) returns null both when the status isn't 409 and when safeParse fails on a 409 body, so onError's if (revertConflicts(err)) refresh() (line 108) genuinely skips the refresh on an unparseable 409. And mutationFn's revertResultSchema.parse(...) (line 105) throws a ZodError on a 2xx with an unexpected shape, which rejects the mutation before onSuccess: refresh (line 106) runs; the per-call onError in useUndo (lines 177-185) then gets revertConflicts(ZodError) === null and shows the generic error with no invalidation. No global MutationCache/onError in queryClient.ts compensates. Both survive.
Verdict: Minor issues
web/src/lib/history.ts:107-108 — a 409 whose body fails to parse skips the refresh, even though partial work likely applied. The top-level mutation's onError only calls refresh() when revertConflicts(err) is truthy, i.e. only when the 409 body successfully parses against revertResultSchema. But a 409 is by this PR's own design the "part of it applied" path. If the server returns a 409 with a body that omits the changeSet key (the schema requires it; there's no .default) or otherwise drifts, safeParse fails → revertConflicts returns null → no invalidateQueries. The user then sees the generic "That couldn't be undone." (from useUndo's per-call onError) while the canvas/history stay stale despite a real partial write. The fix is to gate the refresh on the status, not the parse: if (err instanceof ApiError && err.status === 409) refresh(). Refreshing is the safe thing to do precisely when you can't phrase what happened — it re-fetches the truth. (Verified by reading revertConflicts at lines 86-90 and the mutation at 103-110.)
web/src/lib/history.ts:104-105 — a successful revert whose response fails schema validation is reported as a failure with no refresh.mutationFn runs revertResultSchema.parse(...) on the 2xx body. If the server returns a 2xx with an unexpected shape, .parse throws a ZodError, the mutation rejects, onSuccess: refresh never runs, and useUndo's per-call onError produces { tone: 'error', message: errorMessage(err, …) } (since revertConflicts(ZodError) is null). So a revert that did succeed server-side leaves the editor canvas and history list out of sync and shows a misleading failure. Lower-likelihood (schema is co-located with the contract), but it's a genuine unhappy path introduced by this diff. Suggested fix: refresh in a finally-style path, or catch the parse failure and still invalidate. (Verified by reading mutationFn/onSuccess at 103-106.)
Both are the same root shape: the cache-refresh that keeps the canvas honest is conditioned on being able to describe the outcome, when the case where you can't describe it is exactly when you most need to re-fetch the truth.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Security review of the history panel + undo changes:
Authn/Authz: Verified that both new endpoints (GET /gardens/:id/history and POST /change-sets/:id/revert) sit behind requireAuth() (internal/api/api.go:78,117). GardenHistory correctly enforces roleViewer and RevertChangeSet enforces roleEditor (internal/service/revisions.go:244,286).
Input validation: Path parameters pass through parseIDParam (positive int64 validation, rejects < 1) and query params through intQuery (non-negative integer clamping) (internal/api/errors.go:74-95).
Injection: No raw string concatenation into SQL or commands. The source parameter is hardcoded to domain.SourceUI at the handler level before reaching the service layer.
XSS / unsafe rendering: Untrusted fields from the server (changeSet.summary, changeSet.actorName, conflict.name) are rendered via React JSX ({…}), which auto-escapes HTML. The describeConflict helper returns plain strings that are likewise JSX-escaped when displayed in OutcomeNote.
IDOR:RevertChangeSet fetches the target change set by ID, then validates the actor's role against the owning garden resolved from that record — not from user input — preventing cross-garden revert attempts.
Nothing in the security lane warrants a finding.
🎯 Correctness — Minor issues
Verdict: Minor issues
web/src/pages/GardenEditorPage.tsx:407 — EditorRail's onClose unconditionally calls select(null) and selectPlanting(null). Closing the rail from the History tab (where selection is irrelevant) still clears the user's canvas selection. A user viewing history for the currently selected object loses that selection when they close the panel, forcing them to re-select before they can keep editing. The close handler should only clear selection when the active tab is inspector, or the rail should close without touching canvas state at all.
web/src/lib/history.ts:169 + web/src/editor/EditorRail.tsx:85 — useUndo stores per-change-set outcomes in local useState, but EditorRail mounts only the active tab (active.render()). If a user clicks Undo, switches to the Inspector tab while the mutation is in flight, and later returns to History, HistoryPanel remounts with a fresh useUndo instance. The pending/success/error outcome is lost, so the undo button appears clickable again even though the revert mutation may still be running. The user can then accidentally fire a second revert for the same change set. Either the outcomes (or the mutation itself) need to outlive the tab mount, e.g. by hoisting them into the existing Zustand store or by giving useMutation a stable key so remounts reconnect to in-flight work.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
Minor issues
web/src/editor/store.ts:97 — railTab missing from resetTransient resetTransient is documented as clearing "all transient (non-persisted) editor state at once". The new railTab state is transient by design (it closes when nothing needs it and is not persisted), yet it is not included in resetTransient. This leaks rail state when resetTransient is called from outside GardenEditorPage — e.g. PublicGardenPage relies on it to clear stale editor state when a logged-in user navigates to a public view. While GardenEditorPage has its own mount-time cleanup that manually calls setRailTab(null), duplicating the clear breaks the abstraction and means future call-sites of resetTransient will silently retain the rail tab. Fix: Add railTab: null to the object inside resetTransient.
⚡ Performance — Minor issues
Minor issues
web/src/lib/history.ts:169-193 — Unbounded outcomes state growth in useUndo.
The outcomes record is append-only: every undo adds a key and is never evicted. Each setOutcomes spreads the entire object ({ ...prev, [cs.id]: … }), so after n undos the cumulative work is O(n²). In a long session on the History tab this leaks memory and slows successive undos. Fix: Cap the map (e.g., keep only the most recent 20 outcomes) or store ephemeral per-ID state in a ref keyed by change-set ID rather than React state.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/lib/history.ts:27 — changeCountSchema uses z.number() without .int() or .nonnegative(), allowing any numeric value (negative, fractional, NaN, Infinity) from a misbehaving server to pass validation. totalChanges sums these blindly, so a negative or NaN count causes totalChanges(changeSet) > 0 to evaluate to false and silently hides the Undo button, while describeCounts would render strings like "2.5 plantings added" for fractional values. The schema should constrain this with .int().nonnegative().
Minor issues
web/src/editor/HistoryPanel.tsx:128 — relativeTime returns "just now" for any timestamp in the future (e.g. server clock skew), because the negative seconds value falls through the first < 60 check. Future dates should be handled explicitly rather than misreporting them as the recent past.
I have enough to assess the security lens. Let me verify the rendering paths for server-provided strings and the authz on the endpoints.
I traced through the relevant code:
Authn/authz on the new endpoints (verified):
GET /gardens/:id/history → h.getGardenHistory → svc.GardenHistory → requireGardenRole(..., roleViewer) (internal/service/revisions.go:244). Existence masked for non-participants via the standard ErrNotFound path.
POST /change-sets/:id/revert → h.revertChangeSet → svc.RevertChangeSet → loads the change set, then requireGardenRole(target.GardenID, roleEditor) (internal/service/revisions.go:281-288). No IDOR: a caller can't revert another garden's change set, and a viewer can't revert at all. Both branches (revisions_test.go:369-376) cover the stranger/viewer cases.
Both route groups are registered under h.requireAuth() (internal/api/api.go:78, 117).
Client-side gating (verified): HistoryPanel only renders the UndoButton when canEdit && totalChanges(changeSet) > 0. canEdit is the same owner/editor computation used for every other mutation in GardenEditorPage.tsx:74. This is defense-in-depth only — the service rejects regardless — but it's correct.
Injection: All path components are numeric. gardenId is Number(gardenId) from the route param (GardenEditorPage.tsx:30); changeSetId is cs.id from a zod-validated ChangeSet (history.ts:31). Query params (limit, offset) go through URLSearchParams with number values. No string interpolation into URLs from user input.
Untrusted data rendering (verified): server-provided strings that reach the DOM — actorName, summary, conflict name, and the assembled outcome.message — are all rendered as React text children ({changeSet.actorName}, {changeSet.summary}, {outcome.message} in UndoButton.tsx/HistoryPanel.tsx). React escapes these; no dangerouslySetInnerHTML anywhere in the new files. describeConflict builds a string by interpolating c.name into a template literal, but the result is still rendered as text.
Schema validation: history.ts validates every response with zod (historyPageSchema, revertResultSchema, changeSetSchema) before use, including the 409 body via revertConflicts → revertResultSchema.safeParse. A malformed 409 body fails safe (treated as a plain error, not a fake "partial success").
Secrets / leakage: No credentials, tokens, or PII beyond the actor's display name (already part of the existing history surface). agentRunId is parsed but not rendered.
Defaults: useGardenHistory defaults enabled = true; gardenId is always a real value here. No insecure defaults.
No material issues found through the security lens.
🎯 Correctness — Minor issues
I've verified both findings against the actual source. Let me confirm the key claims.
Let me verify the service-level RevertChangeSet to confirm the "zero counts / 201" path the first finding claims.
I now have enough to make a final assessment. Let me verify both findings:
Finding 1 (web/src/lib/history.ts:209-211): The draft claims describeUndo returns "Undone." in ok tone when nothing was actually undone. Confirmed: describeUndo at line 209 has if (result.conflicts.length === 0) return { tone: 'ok', message: 'Undone.' }. The backend history.go:73-76 returns 200 with changeSet: nil when "every revision resolved to a no-op." In that case conflicts is empty (len 0), so the UI reports "Undone." in ok tone — even though nothing was reverted. This is the "bare success hides the one that didn't" failure mode. The draft's secondary claim about "201 with zero counts" is weaker: looking at commitScope (revisions.go:152-155), a scope with no revisions returns nil — so the 201 path with a non-nil cs would only happen when revisions actually wrote. The 201-with-zero-counts path doesn't really exist per the service code. But the 200 with nil changeSet path is real and confirmed. The finding survives on that primary path.
Finding 2 (web/src/lib/history.ts:107-109): Confirmed: onError only calls refresh() when revertConflicts(err) parses successfully (returns non-null). If a 409 body fails to parse, revertConflicts returns null, and no refresh fires — even though a 409 from this endpoint means "part may have applied." The surrounding code is explicitly careful about this (the comment on line 108 says "a partial revert still changed things"). This is a real defensive gap.
Both findings are confirmed against the actual source. The GardenEditorPage selection-tracking "finding" was already withdrawn by the original reviewer (it's documenting a non-issue), so it correctly does not appear in the findings array.
VERDICT: Minor issues
I verified the phrasing logic (describeUndo / describeConflict / describeCounts) against the backend RevertChangeSet contract in internal/api/history.go and internal/service/revisions.go, and against the test expectations. The status-code → tone mapping (201 clean / 409 partial / 200 no-op) and the "2 of 3 changes undone — …" derivation all check out, and revertConflicts correctly recovers the partial result from the 409 body. Two real correctness gaps in the honest-reporting logic this PR centers on:
web/src/lib/history.ts:209-211 — describeUndo reports "Undone." (ok tone) when the server actually changed nothing. The early return if (result.conflicts.length === 0) return { tone: 'ok', message: 'Undone.' } fires without checking whether result.changeSet is null. The backend's "nothing happened" path — internal/api/history.go:73-76 returns 200 with changeSet: null when every revision resolved to a no-op (already undone by hand) — has conflicts empty, so it lands here. The user clicks Undo, the server does nothing, and the UI answers "Undone." in the muted/ok tone: the exact "bare success would hide the one that didn't" failure mode the PR says it's eliminating, for the zero-applied case. (The draft also floated a "201 with zero counts" path; that one does not hold — commitScope at internal/service/revisions.go:152-155 returns nil for a scope with no revisions, so a 201 always carries a real change set. The 200/null path is the real one.) Verified by reading describeUndo (history.ts:207-218) and tracing internal/api/history.go:68-79 plus internal/service/revisions.go:151-164. Suggested fix: drop the early return when conflicts.length === 0 and let the existing applied === 0 branch handle it — e.g. if (applied === 0) return { tone: 'error', message: 'Nothing was undone — it was already back to this state.' } before the conflicts check, or add a distinct neutral tone.
web/src/lib/history.ts:107-109 — a malformed 409 body leaves the canvas/history stale.useRevertChangeSet.onError only calls refresh() when revertConflicts(err) parses successfully. If a 409 arrives with an unparseable body (proxy rewrite, version skew, schema drift), revertConflicts returns null, no refresh fires, but the backend may have applied part of the revert — so the canvas and history list stay out of sync while the user is shown a generic error. Low severity (defensive edge), but it's the same class of "partial revert changed things, refresh anyway" the surrounding code is careful about (the inline comment on line 108 says "a partial revert still changed things"). Suggested fix: refresh on any 409 regardless of body parse, since a 409 from this endpoint specifically means "something may have applied."
🧹 Code cleanliness & maintainability — Minor issues
I'll verify each finding against the actual code.
Now let me verify the specific findings. First, find the #57/#53 references in DESIGN.md and PR description, and check isPending usage.
Let me verify the GardenEditorPage comment about #57/#53 to confirm the contradictory references finding.
I've confirmed all findings. Let me verify the findings:
#57 contradictory references - Confirmed. UndoButton.tsx:7 and history.ts:162 say "#57 = inline undo". GardenEditorPage.tsx:289 says "chat (#57)". Also DESIGN.md (per diff) describes #57 as chat. The codebase itself contradicts what #57 means. Keep.
isPending dead API - Confirmed. history.ts:192 returns isPending: revert.isPending. The only isPending references in callers are history.isPending (the query, not undo) and outcome?.tone === 'pending'. No caller uses undo.isPending. Keep.
useRevertChangeSet/revertConflicts unnecessarily exported - Confirmed. Grep shows both only used within history.ts (lines 108, 168, 178). No external import. Keep.
HistoryPanel redundant heading - Confirmed. <h2>History</h2> at line 23, while the tab is labeled "History" (from GardenEditorPage railTabs). For a screen reader, the tab + heading both announce "History". Keep as trivial.
relativeTime in component file - Confirmed. relativeTime is exported from HistoryPanel.tsx (a .tsx file), pure function with no JSX. Other pure helpers live in web/src/lib/. Keep as trivial.
All five findings survive verification.
VERDICT: Minor issues
web/src/lib/history.ts:161-162 and web/src/editor/UndoButton.tsx:5 — stale/contradictory issue references. The comment in history.ts says the shared undo is "shared by the history list and (per #49) the agent turn's inline Undo in #57", and UndoButton's doc comment says "the agent turn's inline undo (#57)". But GardenEditorPage.tsx:289 (and the PR's DESIGN.md change) describe #57 as the chat panel ("the journal (#53) and chat (#57) panels too"). So the code comments point at #57 meaning one thing and the rest of the codebase points at #57 meaning another. Whichever is wrong, a reader chasing the reference will be misled. Fix: correct the issue numbers in the comments to match whatever #57 actually is, or drop the numeric references and describe the consumer by name.
web/src/lib/history.ts:192 — isPending is dead public API.useUndo returns isPending: revert.isPending, but no caller uses it. UndoButton keys its disabled/loading state off outcome?.tone === 'pending' (verified by grepping isPending/outcomeFor/.undo( usages across web/src; every other isPending reference is on a different query/mutation, not the undo hook). The field is exported surface that nothing reads. Either wire it up (e.g. disable all undo buttons while any revert is in flight) or drop it from the return.
web/src/lib/history.ts:97 and :86 — unnecessarily exported.useRevertChangeSet and revertConflicts are both exported, but grep confirms each is only called from within history.ts itself (useRevertChangeSet at line 168, revertConflicts at lines 108 and 178; no test or component imports either). They don't need to be part of the module's public surface. Dropping the export keeps the API surface to what useUndo/describeUndo/the schemas actually offer. Small, but it's exactly the kind of leaky surface that accretes over time.
web/src/editor/HistoryPanel.tsx:23 — redundant heading. The panel renders <h2>History</h2> while the tab button that opened it is already labeled "History" (the EditorRail tab in GardenEditorPage.tsx with aria-current="page" marking the active tab). For a screen-reader user the panel's name is announced twice. Either drop the <h2> (the tab is the label) or give the heading something the tab doesn't say (e.g. the garden name). Trivial.
web/src/editor/HistoryPanel.tsx:118 — relativeTime is a pure util living in a component file. It's a stateless, testable string formatter exported from a .tsx component module (no JSX in the function). The rest of the codebase puts pure helpers in web/src/lib/ (e.g. lib/geometry.ts, lib/units.ts); lib/history.ts already lives next door and would be its natural home (it has no JSX and is the file that owns the ChangeSet shape). Move it so the component file is just the component. Trivial, but it's the pattern break the CLAUDE.md "conventions that bite" section is about.
⚡ Performance — No material issues found
No material issues found
Through the performance lens, I verified the new code paths and found nothing material:
useGardenHistory (web/src/lib/history.ts:68) — single useInfiniteQuery, offset-paged at PAGE_SIZE = 30. HistoryPanel is only mounted when the History tab is the active rail tab (EditorRail calls active.render() exclusively, line 84 of EditorRail.tsx), so the query doesn't fire in the background while the inspector is open. No N+1, no unbounded fetch.
useUndo outcomes state (history.ts:169) — keyed by change set id, one entry added per user undo click. Bounded by user actions, and the whole map is discarded when HistoryPanel unmounts on tab switch. Not a leak.
HistoryPanel flatten (HistoryPanel.tsx:19) — pages.flatMap runs per render; for a few pages of 30 entries this is trivial and not in a hot loop (undo is a user action, not a per-frame path).
railTabs array (GardenEditorPage.tsx:291) — recreated per render with new closures, but only the active tab's render() is invoked, so inactive tabs (e.g. the Inspector/PlopInspector subtree) are not built. This is actually a slight win over the old layout, which always mounted the inspector when something was selected.
Invalidation on undo (history.ts:99-102) — refresh() invalidates gardenFullKey + historyKey. Fires once per undo (user action), not in a drag/loop. Correct, not a regression.
No quadratic behavior, no missing limits, no hot-path blocking.
🧯 Error handling & edge cases — Minor issues
All three findings confirmed against the actual code.
Verdict: Minor issues
web/src/editor/HistoryPanel.tsx:45 — unhandled rejection on fetchNextPage. The "Load older" button does onClick={() => void history.fetchNextPage()}. In TanStack Query v5 (repo pins @tanstack/react-query ^5.62.0), fetchNextPage() returns a promise that rejects when the page request fails (network error, 401, etc.). void-discarding it without .catch() leaves an unhandled promise rejection — visible in the console and, in some environments, surfaces as a runtime warning. Suggested fix: onClick={() => { history.fetchNextPage().catch(() => {}) }}. Confirmed the only fetchNextPage caller in the repo, so no existing convention to match.
web/src/editor/HistoryPanel.tsx:26 — misleading error message when only the next page fails.history.isError becomes true on a fetchNextPage failure too (TanStack sets error/isError for any fetch, not just the initial one). When the user clicks "Load older" and that request fails, the panel renders <Alert>Could not load history.</Alert> above the already-loaded entries — which reads as "the whole history failed," even though the older entries are still on screen and retryable via the same button. Concrete impact: a transient network blip on paging makes it look like history is gone. Suggested fix: distinguish initial-load failure from next-page failure (e.g. history.isError && !history.data for the alert; show a smaller inline retry message near "Load older" when history.data exists but the last fetch failed).
web/src/lib/history.ts:177-186 — a 409 with an unparseable body is reported as a plain failure even though part may have applied.revertConflicts uses safeParse and returns null if the 409 body doesn't match revertResultSchema. The hook then falls back to { tone: 'error', message: "That couldn't be undone." } (history.ts:183). The whole premise of this code (per the file's header comment, lines 4-7) is that a 409 means "partly applied, report both." If the server ever returns a 409 with a body shape the client doesn't recognize, the user is told "couldn't be undone" while the canvas may have redrawn with real changes — exactly the lie the PR set out to avoid. This is a contract-coupling concern more than a likely bug (same team owns both sides), so confidence is medium. Suggested fix: on a 409 with an unparseable body, still refresh and report a generic "Something was undone, but details weren't available" partial-style message rather than a flat error.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
Security lens: the new history and revert endpoints are properly authenticated, authorized, and validated; the frontend renders user-controlled data safely; and the state-changing revert is behind the existing CSRF guard.
GET /gardens/:id/history — parseIDParam enforces positive int64; intQuery sanitizes limit/offset; GardenHistory clamps limit to [1, 100] and requires roleViewer via requireGardenRole (existence masked as ErrNotFound).
POST /change-sets/:id/revert — parseIDParam enforces positive int64; RevertChangeSet loads the change set then requires roleEditor on its garden. The v1 group already applies csrfGuard() (Origin-check), so the state-changing POST is CSRF-protected.
No injection: store queries are fully parameterized; no SQL injection vector.
No XSS: changeSet.summary, actorName, conflict names, and relativeTime output are all rendered as React text nodes (no dangerouslySetInnerHTML, no template strings interpolated into HTML).
No secret leakage or unsafe deserialization: API responses are Zod-schematized; no credentials in code.
Race/authorization: canEdit only gates the undo button in the UI; the backend independently re-checks editor role, so bypassing the UI still fails authz.
🎯 Correctness — Minor issues
Verdict: Minor issues
Two real correctness problems, both introduced by this PR:
web/src/pages/GardenEditorPage.tsx:123-126 — selection-change id does not auto-switch to Inspector.
The effect that auto-switches the rail tab depends only on the hasSelectionboolean, not on the actual selection ids:
Because hasSelection stays true when the user clicks from object A to object B, the effect never fires and the rail stays on whatever tab it was on (e.g. History). This contradicts the stated design constraint: "Selecting an object still lands you in the inspector without a click." The fix is to put selectedId and selectedPlantingId in the dependency array so any change of selection re-routes to the inspector.
web/src/editor/HistoryPanel.tsx:118-131 — relativeTime can mislabel same-day events as "yesterday".
The day computation uses Math.round at every boundary:
At the day boundary this over-rounds: an event at 00:00 today viewed at 23:30 today is 23.5 hours ago → hours = Math.round(1410/60) = 24 → days = Math.round(24/24) = 1 → "yesterday", even though the event occurred today. Using Math.floor for the day step (or comparing calendar dates) would fix the semantic error.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
relativeTime is exported from a component file instead of lib/ HistoryPanel.tsx:118 exports relativeTime, a pure date-formatting utility with no React or UI dependencies. If the journal (#53) or chat (#57) later wants the same phrasing, they would have to import it from a panel component, breaking the lib/ vs. components/ boundary the rest of the codebase observes (e.g., geometry.ts, units.ts). Fix: move relativeTime into web/src/lib/history.ts (or a new time.ts) and import it in HistoryPanel.
useUndo exposes isPending that no caller consumes history.ts:189–193 returns isPending: revert.isPending, but neither HistoryPanel nor UndoButton reads it; each button tracks its own pending state via outcomeFor(id)?.tone === 'pending'. The exported flag is therefore dead weight that misleadingly suggests "any revert is pending" rather than per-item status. Fix: drop isPending from the return object, or replace it with a per-id predicate such as isPendingFor(id) if a future consumer needs it.
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/editor/HistoryPanel.tsx:45 — Pagination failure is silently swallowed.history.fetchNextPage() is fire-and-forget via void. If the network fails while loading older history, the button stops spinning and no error is shown to the user. TanStack Query exposes isFetchNextPageError / fetchNextPageError for this; the component ignores them. Fix: Wrap the call, catch errors, and surface them in the panel (e.g., a small inline alert or a toast).
web/src/lib/history.ts:21 — n in changeCountSchema accepts negative numbers.z.number() allows negatives. A server bug returning n: -1 would silently distort totalChanges (possibly dropping below zero and hiding the undo button) while describeCounts filters negatives out entirely. Fix: Tighten to z.number().int().nonnegative() so a bad payload fails loudly at parse time instead of producing confusing UI.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Blocking issues found** · 21 findings (8 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | 409 with unparseable body skips cache refresh despite likely partial write | `web/src/lib/history.ts:101` | 3/5 | correctness, error-handling, performance |
| 🟠 | Rail auto-switch-to-Inspector effect keys off a boolean, so it misses selection changes while already selected | `web/src/pages/GardenEditorPage.tsx:123` | 3/5 | correctness |
| 🟡 | VERDICT: Minor issues | `web/src/editor/HistoryPanel.tsx:118` | 3/5 | correctness, maintainability |
| 🔴 | describeUndo reports 'Undone.' on a backend no-op (changeSet: null, conflicts: []), reachable via 'Undo again' on a create-type entry whose entity is already gone | `web/src/lib/history.ts:209` | 2/5 | correctness, error-handling |
| 🟠 | Pagination failure silently swallowed — no user feedback when fetching older history fails | `web/src/editor/HistoryPanel.tsx:45` | 2/5 | error-handling |
| 🟠 | resetTransient doesn't clear the new railTab state, breaking the one-place-clears-all transient-state pattern | `web/src/editor/store.ts:97` | 2/5 | maintainability |
| 🟠 | useUndo outcomes lost when History tab unmounts | `web/src/lib/history.ts:169` | 2/5 | correctness, maintainability, performance |
| 🟡 | useUndo returns isPending but no caller uses it (dead public API) | `web/src/lib/history.ts:192` | 2/5 | maintainability |
<details><summary>13 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🔴 | Unconstrained z.number() in changeCountSchema allows invalid server data to hide undo button or show fractional counts | `web/src/lib/history.ts:27` | kimi-k2.6:cloud | error-handling |
| 🟠 | changeCountSchema allows negative counts, which can hide undo buttons or produce incorrect totals | `web/src/lib/history.ts:21` | opencode/kimi-k2.6:cloud | error-handling |
| 🟠 | Closing rail from History tab clears canvas selection | `web/src/pages/GardenEditorPage.tsx:407` | kimi-k2.6:cloud | correctness |
| 🟡 | VERDICT: Minor issues | `DESIGN.md:109` | claude-code/sonnet | maintainability |
| 🟡 | VERDICT: Minor issues | `PublicGardenPage.tsx:28` | claude-code/sonnet | maintainability |
| 🟡 | HistoryPanel re-derives sets and per-entry counts on every render with no memoization | `web/src/editor/HistoryPanel.tsx:18` | glm-5.2:cloud | performance |
| 🟡 | history.isError shows "Could not load history" even when only the next page failed, not the initial load | `web/src/editor/HistoryPanel.tsx:23` | opencode/glm-5.2:cloud | error-handling, maintainability |
| 🟡 | totalChanges recomputed per entry when describeCounts already walked the same counts | `web/src/editor/HistoryPanel.tsx:104` | glm-5.2:cloud | maintainability |
| 🟡 | relativeTime misreports future dates as 'just now' | `web/src/editor/HistoryPanel.tsx:128` | kimi-k2.6:cloud | error-handling |
| 🟡 | useRevertChangeSet and revertConflicts are exported but only used within history.ts | `web/src/lib/history.ts:97` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | Comments reference #57 as 'inline undo' but GardenEditorPage/DESIGN describe #57 as the chat panel — misleading issue references | `web/src/lib/history.ts:161` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | A 409 with an unparseable body is reported as a flat failure even though part of the revert may have applied | `web/src/lib/history.ts:178` | opencode/glm-5.2:cloud | error-handling |
| ⚪ | badge render condition stacks three ad-hoc truthiness checks instead of a named helper | `web/src/editor/EditorRail.tsx:69` | glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
This backend authz behaves correctly (existence masking preserved, viewer/editor role gating enforced independently of client state). This diff is frontend-only (no backend changes), and the backend endpoints it consumes were already reviewed under #48. My security-lens review of this diff specifically:
**VERDICT: No material issues found**
Findings, all verified:
- **Authz is server-enforced, not client-trusted.** `canEdit` (`GardenEditorPage.tsx:74`) gates the Undo button only for UX; the actual revert (`internal/service/revisions.go:281-291`) independently requires `roleEditor` via `requireGardenRole`, and history reads require `roleViewer`. A tampered client state can't escalate privilege.
- **No injection surface.** `historyKey`/`useGardenHistory` build query params through `api.get(..., { params })`, which routes through `buildUrl` → `URLSearchParams` (`web/src/lib/api.ts:47-57`) — no string concatenation into the URL. `gardenId`/`changeSetId` used in path segments (`web/src/lib/history.ts:59,73`) are numeric, sourced from route params / server-issued `ChangeSet.id`, not free-form user text.
- **No XSS.** All server-sourced strings that reach the DOM — `changeSet.summary`, `actorName`, `RevertConflict.name` (rendered via `describeConflict`, `HistoryPanel.tsx`, `UndoButton.tsx`) — are rendered as JSX text children, which React escapes; no `dangerouslySetInnerHTML`, no `eval`, no manual HTML construction anywhere in the new files.
- **Response validation.** `changeSetSchema`/`revertResultSchema`/`historyPageSchema` (`web/src/lib/history.ts`) use zod to validate the shape of server responses before use, so a malformed/unexpected payload fails closed (schema parse throws / `safeParse` returns unsuccessful) rather than being trusted blindly.
- **409 body handling is safe.** `revertConflicts()` (`web/src/lib/history.ts:56-60`) only trusts `err.body` after `revertResultSchema.safeParse`, so an unexpected error body can't be coerced into a fabricated "partial success" UI state.
Nothing in this diff introduces a new authn/authz gap, injection point, unsafe deserialization, or secret exposure. The one thing worth flagging as a pattern to watch (not a bug here): `errorMessage()` surfaces the raw server error message verbatim in the UI (`HistoryPanel.tsx` Alert, `UndoButton.tsx`/`describeUndo` fallback) — fine as long as the Go service never puts sensitive detail (row contents, internal IDs of entities the actor can't see) into `writeServiceError` messages, which is pre-existing behavior this PR doesn't change.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings check out exactly against the code. Line numbers, logic, and referenced files all confirmed by direct inspection.
**VERDICT: Minor issues**
- `web/src/pages/GardenEditorPage.tsx:122-126` — the rail auto-switch-to-Inspector effect keys off the derived boolean `hasSelection` (`selectedId != null || selectedPlantingId != null`), not the selection identity, so it only fires on a `false → true` transition. Confirmed: `GardenCanvas.tsx:247` wires `onSelect={select}` directly to any object click, and `store.ts:71` (`select: (id) => set({ selectedId: id, selectedPlantingId: null })`) never clears selection before setting a new id — so clicking bed B while bed A is already selected keeps `hasSelection` `true` throughout and the effect's `[hasSelection, setRailTab]` deps don't re-run. Reproduction: select bed A (auto-switches to Inspector) → manually open History via the unconditional toggle at `GardenEditorPage.tsx:345-351` → click bed B on the canvas. The rail stays on History instead of switching to Inspector for B, contradicting the documented invariant at lines 118-121 ("Selecting anything lands you in the inspector without a click"). Same gap crossing kinds: `selectPlanting` (`store.ts:74`) clears `selectedId` but sets `selectedPlantingId`, so `hasSelection` stays `true` when selecting a plop while History is open. Fix: depend on `[selectedId, selectedPlantingId, setRailTab]` instead of the boolean.
- `web/src/lib/history.ts:207-218` (`describeUndo`) — when the backend returns `changeSet: null, conflicts: []` (the documented `cs == nil` / 200 "already undone by hand" no-op path at `internal/api/history.go:73-76`), the `result.conflicts.length === 0` check short-circuits to `{ tone: 'ok', message: 'Undone.' }` even though nothing was actually reverted this time. Confirmed this exact input combination isn't exercised by `history.test.ts`, which only covers conflicts-empty-with-a-real-changeSet (plain success), partial, and error-with-conflicts cases — the null-changeSet/empty-conflicts combination falls through untested to the same "Undone." message as a real revert.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
I independently verified both findings in the draft against the actual source — both survive:
**VERDICT: Minor issues**
- `web/src/editor/store.ts:97-106` — `resetTransient` omits the new `railTab` field, contradicting its own "clear all transient state" comment. Confirmed: `GardenEditorPage.tsx`'s parallel `clear()` (lines 86-94) does call `setRailTab(null)`, but `resetTransient` (called from `PublicGardenPage.tsx:28`) does not.
- `web/src/editor/HistoryPanel.tsx:118` — `relativeTime` is a pure, dependency-free helper exported from a component file rather than `web/src/lib/`, breaking the project's established lib+test convention (confirmed via DESIGN.md:109 and grep — it's used only within `HistoryPanel.tsx` and has no test).
Full corrected review written to the plan file. This was a read-only verification task (no code changes), so there's nothing to implement — the plan file just holds the finalized review output.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
Good, that confirms the surrounding "nothing else stood out" commentary matches the actual code (lazy render via `active.render()` at line 87, `render()` only called for the active tab). The finding itself checks out.
Everything in the draft is verified against the actual source: `useRevertChangeSet` (history.ts:97-111) unconditionally invalidates `historyKey(gardenId)` on both success and 409-with-partial-apply, `useGardenHistory` is confirmed as an offset-paginated `useInfiniteQuery` (history.ts:68-83), and `@tanstack/react-query` is pinned at `^5.62.0` (web/package.json:19), whose documented default behavior on invalidating an active infinite query is to sequentially refetch every already-fetched page. Since `HistoryPanel`/`UndoButton` only mount while the History tab is active (`EditorRail.tsx`'s lazy `render()`), the query is active exactly when a user clicks undo, so the finding holds.
**VERDICT: Minor issues**
- `web/src/lib/history.ts:97-111` — `useRevertChangeSet`'s `refresh()` invalidates `historyKey(gardenId)` unconditionally on every undo (success or 409-with-partial-apply). `useGardenHistory` (history.ts:68-83) is a `useInfiniteQuery` (TanStack Query `^5.62.0`, confirmed in `web/package.json:19`) using offset-based paging. TanStack Query's default behavior for an active infinite query is to refetch every already-loaded page sequentially on invalidation, not just the first page. Since the History tab (and thus the query) is only mounted while active — `EditorRail` renders tabs lazily — a user who has clicked "Load older" a few times will trigger an N-request cascade on every subsequent undo click, just to redraw the same list plus one new entry. Cost grows with how far the user has paginated. A cheap fix: after a successful/partial revert, only refetch the first page (e.g. `qc.setQueryData` to drop pages beyond the first, or `history.refetch({ refetchPage: (_, index) => index === 0 })`) rather than invalidating the whole infinite-query cache.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
## Review — 🧯 Error handling & edge cases
**VERDICT: Blocking issues found**
### `describeUndo` reports "Undone." even when nothing was actually reverted
`web/src/lib/history.ts:207-211`:
```ts
export function describeUndo(target: ChangeSet, result: RevertResult): UndoOutcome {
const skipped = result.conflicts.map(describeConflict).join('; ')
if (result.conflicts.length === 0) {
return { tone: 'ok', message: 'Undone.' }
}
...
```
This treats "zero conflicts" as synonymous with "fully applied," but the backend contract (`internal/api/history.go:28-30`, `63-77`) explicitly allows a 200 response with `{changeSet: null, conflicts: []}` — the comment there says exactly this: *"ChangeSet is null when nothing needed reverting"*, and the handler's `case cs == nil:` branch returns 200 with that body.
Traced how that's reached in `internal/service/revisions.go:462-466`:
```go
cur, err := ops.get(ctx, r.EntityID)
if errors.Is(err, domain.ErrNotFound) {
if r.Op == domain.OpCreate {
return nil, nil, nil // already gone: the inverse is a no-op, not a conflict
}
return nil, conflict(r, domain.ConflictMissing, ""), nil
}
```
Only `OpCreate` gets this silent no-op treatment. This is concretely reachable via the "Undo again" affordance this PR ships: `HistoryEntry` (`web/src/editor/HistoryPanel.tsx:104-111`) renders the undo button unconditionally whenever `canEdit && totalChanges(changeSet) > 0`, with `label={reverted ? 'Undo again' : 'Undo'}` — it is not disabled or hidden for an already-reverted entry. Clicking it calls `undo.undo(changeSet)` (`web/src/editor/UndoButton.tsx`) on the *original* changeSet object, not the revert entry. `RevertChangeSet` (`internal/service/revisions.go:281-331`) has no guard on `target.RevertedByID` anywhere in the function.
So: create a bed → undo it (bed deleted) → click "Undo again" on the now-struck-through original entry → `RevertChangeSet` is invoked again on the original create changeset → its inverse (a removal) finds the entity already gone → hits the `OpCreate`/`ErrNotFound` no-op branch → no revisions recorded → `commitScope` returns `(nil, nil)` since `sc.taken()` is empty → handler returns 200 with `{changeSet: null, conflicts: []}` → `describeUndo` hits the zero-conflict branch and returns **"Undone."**, even though nothing happened.
The same early-return also means any mixed change set where one `create` sub-revision silently no-ops while other revisions genuinely apply is reported as a plain "Undone." — `applied` is never compared against `target`'s total in the `conflicts.length === 0` branch, so a partial-but-silently-short revert is indistinguishable from a full one.
`web/src/lib/history.test.ts`'s "is a plain success when nothing conflicted" case supplies `changeSet: changeSet({ id: 2 })` (non-null) — the `changeSet: null, conflicts: []` case is not exercised by any test.
**Fix suggestion:** in `describeUndo`, when `conflicts.length === 0`, compare `totalChanges(result.changeSet ?? …)` against `totalChanges(target)`; if `result.changeSet` is null or under-counts relative to `target`, report a "nothing to undo" / partial state instead of an unconditional "Undone."
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Verdict: No material issues found
I traced the undo flow end-to-end through the security lens and found no material gaps:
- **Authz is enforced server-side, not just by `canEdit`.** `RevertChangeSet` calls `requireGardenRole(..., roleEditor)` (`internal/service/revisions.go:286`) and `GardenHistory` requires `roleViewer` (`internal/service/revisions.go:244`). The frontend `canEdit` gate on the `UndoButton` is purely cosmetic; a forged/incorrect value can't bypass the backend. Verified by reading both service methods.
- **No injection surface.** The two new endpoints take only numeric IDs (`parseIDParam` on `:id`, `intQuery` on `limit`/`offset` in `internal/api/history.go:34-41`), and the client builds paths from `number`-typed `gardenId`/`changeSetId` only (`web/src/lib/history.ts:71,94`). No string interpolation into URLs or queries.
- **Untrusted server data is rendered safely.** `changeSet.summary`, `actorName`, and conflict `name` flow into JSX as text children (`HistoryPanel.tsx`, `UndoButton.tsx`) or into template strings inside `describeConflict`/`describeCounts` — never via `dangerouslySetInnerHTML`. React escapes text content, so no stored-XSS via a crafted summary/name.
- **Response parsing is validated.** `historyPageSchema.parse` and `revertResultSchema.parse` validate shapes (`history.ts:74,95`); the 409 path uses `safeParse` (`revertConflicts`, `history.ts:84`) so a malformed conflict body degrades to a generic error rather than crashing or trusting raw data.
- **No credential/secret exposure** in the new files; requests use `credentials: 'same-origin'` via the existing `api` wrapper (`api.ts:97`), unchanged.
One non-security note I confirmed is fine: `useGardenHistory` runs regardless of `canEdit`, but the backend's viewer-role check authorizes that, so a shared viewer legitimately seeing history is intended behavior — not a leak.
No findings to report for this lens.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings verified against the actual code.
**Verdict: Minor issues**
- **`web/src/pages/GardenEditorPage.tsx:118-126`** — The selection auto-switch effect's comment claims it "leaves History/Chat open if that's where you were" when you deselect, but it does not, after a selection round-trip. Trace: user is on the History tab (`railTab === 'history'`, no selection). They select a bed → `hasSelection` true → effect sets `railTab = 'inspector'` (line 124). They deselect → `hasSelection` false, and `useEditorStore.getState().railTab === 'inspector'` → effect sets `railTab = null` (line 125). The rail closes; it does *not* return to History. The store only keeps a single `railTab` (no `previousTab`), and the deselect branch unconditionally clears to `null` whenever the current tab is `'inspector'`. So the "leave History open" guarantee only holds if you never selected anything while there; the moment you select, History is dropped on deselect. This contradicts the code's own comment (lines 119-121) and the PR description. Fix: remember the pre-inspector tab and restore to it (e.g. `'history'`) on deselect rather than `null`, or track a `previousTab`.
- **`web/src/lib/history.ts:207-217`** (unverified edge, not blocking) — `describeUndo` uses `totalChanges(target)` as the denominator and `totalChanges(result.changeSet)` as the numerator. This is correct as long as the revert's counts map to the target's revisions. The one fragile case: a target whose `counts` array is empty/zero while conflicts are non-empty would render "0 of 0 changes undone — …" as `partial` rather than `error`, because the `applied === 0` branch (line 213) only triggers when `result.changeSet` is `null` (line 212). Whether a non-null `result.changeSet` can ever carry zero counts alongside conflicts depends on server-side counts population, which I could not confirm in this pass. Low-confidence.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All findings verified. Now confirming the editor page (GardenEditorPage) does clear railTab in its own unmount/mount effect, but `resetTransient` (used by PublicGardenPage) does not — confirming finding 1.
Let me verify finding 2's claim about the `useUndo` outcomes growth and finding 3/4 details are all confirmed from what I've read.
All four findings are confirmed against the actual code:
1. **`resetTransient` omits `railTab`** — Confirmed. `store.ts:97-106` resets all other transient fields but not `railTab`. `PublicGardenPage.tsx:28` calls `resetTransient()` on mount, so a stale `railTab` survives. (Note: `GardenEditorPage.tsx:93` does clear `railTab` in its own separate effect, so only the public page / the `resetTransient` contract is affected — still a real inconsistency.)
2. **`useUndo` outcomes grow unbounded** — Confirmed. `history.ts:169` holds `outcomes` in `useState`, each `undo` adds an entry keyed by id with no eviction (`history.ts:172,175,179`). The doc comment at line 165 even acknowledges "a single instance serves a whole list."
3. **`totalChanges` recomputed per entry** — Confirmed. `HistoryPanel.tsx:70` calls `describeCounts` (which filters `n > 0`), then line 104 separately calls `totalChanges(changeSet)` (a full `reduce` over `cs.counts`, `history.ts:115`). Both walk `counts`.
4. **`EditorRail` badge condition** — Confirmed. `EditorRail.tsx:69` (the draft said 62; the actual badge check is at line 69): `tab.badge != null && tab.badge !== false && tab.badge !== 0`.
**Verdict: Minor issues**
- **`web/src/editor/store.ts:97` — `resetTransient` doesn't clear the new `railTab` state.** Every other transient field (`selectedId`, `selectedPlantingId`, `focusedObjectId`, `armedPlant`, `armedKind`, `liveObject`, `livePlanting`) is reset in `resetTransient`, but `railTab` was left out. `PublicGardenPage.tsx:28` calls `resetTransient()` on mount to enter a garden view fresh, so a `railTab` left from a previous garden (e.g. a stale `'inspector'`/`'history'`) survives the reset. (`GardenEditorPage.tsx:93` clears `railTab` in its own mount/unmount effect, so the editor page itself isn't affected — but the shared `resetTransient` contract is now inconsistent.) Fix: add `railTab: null` to the `set({...})` in `resetTransient`. Severity medium, high confidence.
- **`web/src/lib/history.ts:169` — `useUndo`'s `outcomes` record grows unbounded.** Each `undo` adds an entry keyed by change-set id (`history.ts:172,175,179`) and never evicts one. The hook stays mounted across the whole paged history list (and, per the #57 inline-undo sharing noted in the file's own doc comment at line 165, can outlive a single list view), so `outcomes` grows with every undo action. Bounded in practice by user actions, so minor; worth a comment or scoping to visible entries. Severity small, medium confidence.
- **`web/src/editor/HistoryPanel.tsx:104` — `totalChanges` recomputed per entry despite `describeCounts` already walking the same counts.** `HistoryEntry` computes `counts = describeCounts(changeSet)` at line 70 (which filters `n > 0`), then the undo-button guard at line 104 calls `totalChanges(changeSet)` — a second full `reduce` over `cs.counts` (`history.ts:115`). Both helpers are already imported; a single-pass "has any change" check would suffice. Minor churn, easy to mistake for doing different things. Severity small, high confidence.
- **`web/src/editor/EditorRail.tsx:69` — `badge` render condition stacks three ad-hoc truthiness checks.** `tab.badge != null && tab.badge !== false && tab.badge !== 0` (line 69) is three stacked checks to express "a count, or a plain marker, never shown for zero/false." A named helper (e.g. `hasBadge`) or normalizing the `badge` type to `number | true | undefined` at the `RailTab` interface would make the intent legible at the call site. Severity trivial, high confidence.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
All three findings are confirmed against the actual code:
1. **Outcomes unbounded** — `history.ts:169` confirms `outcomes` is a `Record<number, UndoOutcome>` that only accumulates via `setOutcomes`; there is no eviction logic anywhere.
2. **Lazy render discards state** — `EditorRail.tsx` renders only `active.render()`; `HistoryPanel` mounts `useUndo` (line 18), so tab-switch unmounts discard the hook's state.
3. **Per-render re-derivation** — `HistoryPanel.tsx:19` recomputes `sets` via `flatMap` every render, and `HistoryEntry` calls `describeCounts`/`totalChanges` with no memoization.
The draft review is accurate. Outputting the corrected review unchanged.
**Verdict: Minor issues**
- **`useUndo` outcomes state grows unbounded and is never cleared** (`web/src/lib/history.ts:169-187`). `outcomes` is a `Record<number, UndoOutcome>` keyed by change-set id that only ever accumulates — there is no eviction when entries scroll out of the visible list, no TTL, and the history itself is paged with `flatMap` over all loaded pages. After an undo on an older entry, that outcome lingers in component state for the life of the mounted `HistoryPanel`. Impact is small (history lists are short and ids are sparse integers), but it is a genuine unbounded-growth leak — a long-lived editor session that loads many pages and undoes several entries keeps every outcome forever. Suggested fix: clear `outcomes[id]` (or cap the record) when the change set is no longer in `history.data`, or simply store the outcome keyed to the currently visible set and drop stale keys on history refetch.
- **`useUndo` is instantiated per `HistoryPanel` mount, and `HistoryPanel` is rendered lazily inside the rail tab** (`web/src/editor/HistoryPanel.tsx:18`, `web/src/editor/EditorRail.tsx:71`). Because `EditorRail` only renders `active.render()` for the active tab, switching away from the History tab unmounts `HistoryPanel`, and its `useUndo` state (including pending `outcome`) is discarded — switching back remounts and re-fetches. That is acceptable (it's the documented lazy-render contract), but it means a pending "Undoing…" outcome is lost on tab switch. Verified: `EditorRail.tsx:71` renders only `active.render()`, and `useUndo`'s state lives in the hook within `HistoryPanel`. Not a perf regression, just noting the in-flight outcome has no persistence across tab toggles; if persistence matters for #57's inline undo, this hook will need to lift the outcomes store out of the panel.
- **`HistoryPanel` re-runs `flatMap` over all loaded pages on every render** (`web/src/editor/HistoryPanel.tsx:19`). `sets` is recomputed each render from `history.data?.pages`, and each entry maps to a `HistoryEntry` that calls `describeCounts`/`totalChanges` (reduce over counts). With offset paging at 30/page and a typical user scrolling a page or two this is negligible; but `describeCounts` and `totalChanges` are re-derived per render per entry with no memoization, and the parent re-renders on every `useUndo` state change (each `setOutcomes` triggers a re-render of the whole list). For a long list this is O(n) work on each undo click rather than O(1) for the one changed row. Suggested fix: memoize `sets` with `useMemo` and/or memoize `HistoryEntry` so an undo outcome update only re-renders the affected entry.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Both findings verify against the actual source. `revertConflicts` (history.ts:86-90) returns `null` both when the status isn't 409 *and* when `safeParse` fails on a 409 body, so `onError`'s `if (revertConflicts(err)) refresh()` (line 108) genuinely skips the refresh on an unparseable 409. And `mutationFn`'s `revertResultSchema.parse(...)` (line 105) throws a `ZodError` on a 2xx with an unexpected shape, which rejects the mutation before `onSuccess: refresh` (line 106) runs; the per-call `onError` in `useUndo` (lines 177-185) then gets `revertConflicts(ZodError) === null` and shows the generic error with no invalidation. No global `MutationCache`/`onError` in `queryClient.ts` compensates. Both survive.
## Verdict: Minor issues
- **`web/src/lib/history.ts:107-108` — a 409 whose body fails to parse skips the refresh, even though partial work likely applied.** The top-level mutation's `onError` only calls `refresh()` when `revertConflicts(err)` is truthy, i.e. only when the 409 body *successfully* parses against `revertResultSchema`. But a 409 is by this PR's own design the "part of it applied" path. If the server returns a 409 with a body that omits the `changeSet` key (the schema requires it; there's no `.default`) or otherwise drifts, `safeParse` fails → `revertConflicts` returns `null` → no `invalidateQueries`. The user then sees the generic "That couldn't be undone." (from `useUndo`'s per-call `onError`) while the canvas/history stay stale despite a real partial write. The fix is to gate the refresh on the *status*, not the parse: `if (err instanceof ApiError && err.status === 409) refresh()`. Refreshing is the safe thing to do precisely when you can't phrase what happened — it re-fetches the truth. (Verified by reading `revertConflicts` at lines 86-90 and the mutation at 103-110.)
- **`web/src/lib/history.ts:104-105` — a successful revert whose response fails schema validation is reported as a failure with no refresh.** `mutationFn` runs `revertResultSchema.parse(...)` on the 2xx body. If the server returns a 2xx with an unexpected shape, `.parse` throws a `ZodError`, the mutation rejects, `onSuccess: refresh` never runs, and `useUndo`'s per-call `onError` produces `{ tone: 'error', message: errorMessage(err, …) }` (since `revertConflicts(ZodError)` is `null`). So a revert that *did* succeed server-side leaves the editor canvas and history list out of sync and shows a misleading failure. Lower-likelihood (schema is co-located with the contract), but it's a genuine unhappy path introduced by this diff. Suggested fix: refresh in a `finally`-style path, or catch the parse failure and still invalidate. (Verified by reading `mutationFn`/`onSuccess` at 103-106.)
Both are the same root shape: the cache-refresh that keeps the canvas honest is conditioned on being able to *describe* the outcome, when the case where you can't describe it is exactly when you most need to re-fetch the truth.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
Security review of the history panel + undo changes:
- **Authn/Authz:** Verified that both new endpoints (`GET /gardens/:id/history` and `POST /change-sets/:id/revert`) sit behind `requireAuth()` (`internal/api/api.go:78,117`). `GardenHistory` correctly enforces `roleViewer` and `RevertChangeSet` enforces `roleEditor` (`internal/service/revisions.go:244,286`).
- **Input validation:** Path parameters pass through `parseIDParam` (positive int64 validation, rejects `< 1`) and query params through `intQuery` (non-negative integer clamping) (`internal/api/errors.go:74-95`).
- **Injection:** No raw string concatenation into SQL or commands. The `source` parameter is hardcoded to `domain.SourceUI` at the handler level before reaching the service layer.
- **XSS / unsafe rendering:** Untrusted fields from the server (`changeSet.summary`, `changeSet.actorName`, `conflict.name`) are rendered via React JSX (`{…}`), which auto-escapes HTML. The `describeConflict` helper returns plain strings that are likewise JSX-escaped when displayed in `OutcomeNote`.
- **IDOR:** `RevertChangeSet` fetches the target change set by ID, then validates the actor's role against the *owning garden* resolved from that record — not from user input — preventing cross-garden revert attempts.
Nothing in the security lane warrants a finding.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`web/src/pages/GardenEditorPage.tsx:407`** — `EditorRail`'s `onClose` unconditionally calls `select(null)` and `selectPlanting(null)`. Closing the rail from the **History** tab (where selection is irrelevant) still clears the user's canvas selection. A user viewing history for the currently selected object loses that selection when they close the panel, forcing them to re-select before they can keep editing. The close handler should only clear selection when the active tab is `inspector`, or the rail should close without touching canvas state at all.
- **`web/src/lib/history.ts:169` + `web/src/editor/EditorRail.tsx:85`** — `useUndo` stores per-change-set outcomes in local `useState`, but `EditorRail` mounts only the active tab (`active.render()`). If a user clicks **Undo**, switches to the Inspector tab while the mutation is in flight, and later returns to History, `HistoryPanel` remounts with a fresh `useUndo` instance. The pending/success/error outcome is lost, so the undo button appears clickable again even though the revert mutation may still be running. The user can then accidentally fire a second revert for the same change set. Either the outcomes (or the mutation itself) need to outlive the tab mount, e.g. by hoisting them into the existing Zustand store or by giving `useMutation` a stable key so remounts reconnect to in-flight work.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
**Minor issues**
- **`web/src/editor/store.ts:97` — `railTab` missing from `resetTransient`**
`resetTransient` is documented as clearing "all transient (non-persisted) editor state at once". The new `railTab` state is transient by design (it closes when nothing needs it and is not persisted), yet it is not included in `resetTransient`. This leaks rail state when `resetTransient` is called from outside `GardenEditorPage` — e.g. `PublicGardenPage` relies on it to clear stale editor state when a logged-in user navigates to a public view. While `GardenEditorPage` has its own mount-time cleanup that manually calls `setRailTab(null)`, duplicating the clear breaks the abstraction and means future call-sites of `resetTransient` will silently retain the rail tab.
**Fix:** Add `railTab: null` to the object inside `resetTransient`.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Minor issues**
- **`web/src/lib/history.ts:169-193` — Unbounded `outcomes` state growth in `useUndo`.**
The `outcomes` record is append-only: every undo adds a key and is never evicted. Each `setOutcomes` spreads the entire object (`{ ...prev, [cs.id]: … }`), so after *n* undos the cumulative work is O(n²). In a long session on the History tab this leaks memory and slows successive undos.
**Fix:** Cap the map (e.g., keep only the most recent 20 outcomes) or store ephemeral per-ID state in a ref keyed by change-set ID rather than React state.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- `web/src/lib/history.ts:27` — `changeCountSchema` uses `z.number()` without `.int()` or `.nonnegative()`, allowing any numeric value (negative, fractional, `NaN`, `Infinity`) from a misbehaving server to pass validation. `totalChanges` sums these blindly, so a negative or `NaN` count causes `totalChanges(changeSet) > 0` to evaluate to false and silently hides the **Undo** button, while `describeCounts` would render strings like `"2.5 plantings added"` for fractional values. The schema should constrain this with `.int().nonnegative()`.
**Minor issues**
- `web/src/editor/HistoryPanel.tsx:128` — `relativeTime` returns `"just now"` for any timestamp in the future (e.g. server clock skew), because the negative `seconds` value falls through the first `< 60` check. Future dates should be handled explicitly rather than misreporting them as the recent past.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I have enough to assess the security lens. Let me verify the rendering paths for server-provided strings and the authz on the endpoints.
I traced through the relevant code:
**Authn/authz on the new endpoints** (verified):
- `GET /gardens/:id/history` → `h.getGardenHistory` → `svc.GardenHistory` → `requireGardenRole(..., roleViewer)` (`internal/service/revisions.go:244`). Existence masked for non-participants via the standard `ErrNotFound` path.
- `POST /change-sets/:id/revert` → `h.revertChangeSet` → `svc.RevertChangeSet` → loads the change set, then `requireGardenRole(target.GardenID, roleEditor)` (`internal/service/revisions.go:281-288`). No IDOR: a caller can't revert another garden's change set, and a viewer can't revert at all. Both branches (`revisions_test.go:369-376`) cover the stranger/viewer cases.
- Both route groups are registered under `h.requireAuth()` (`internal/api/api.go:78, 117`).
**Client-side gating** (verified): `HistoryPanel` only renders the `UndoButton` when `canEdit && totalChanges(changeSet) > 0`. `canEdit` is the same owner/editor computation used for every other mutation in `GardenEditorPage.tsx:74`. This is defense-in-depth only — the service rejects regardless — but it's correct.
**Injection**: All path components are numeric. `gardenId` is `Number(gardenId)` from the route param (`GardenEditorPage.tsx:30`); `changeSetId` is `cs.id` from a zod-validated `ChangeSet` (`history.ts:31`). Query params (`limit`, `offset`) go through `URLSearchParams` with number values. No string interpolation into URLs from user input.
**Untrusted data rendering** (verified): server-provided strings that reach the DOM — `actorName`, `summary`, conflict `name`, and the assembled `outcome.message` — are all rendered as React text children (`{changeSet.actorName}`, `{changeSet.summary}`, `{outcome.message}` in `UndoButton.tsx`/`HistoryPanel.tsx`). React escapes these; no `dangerouslySetInnerHTML` anywhere in the new files. `describeConflict` builds a string by interpolating `c.name` into a template literal, but the result is still rendered as text.
**Schema validation**: `history.ts` validates every response with zod (`historyPageSchema`, `revertResultSchema`, `changeSetSchema`) before use, including the 409 body via `revertConflicts` → `revertResultSchema.safeParse`. A malformed 409 body fails safe (treated as a plain error, not a fake "partial success").
**Secrets / leakage**: No credentials, tokens, or PII beyond the actor's display name (already part of the existing history surface). `agentRunId` is parsed but not rendered.
**Defaults**: `useGardenHistory` defaults `enabled = true`; `gardenId` is always a real value here. No insecure defaults.
No material issues found through the security lens.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
I've verified both findings against the actual source. Let me confirm the key claims.
Let me verify the service-level RevertChangeSet to confirm the "zero counts / 201" path the first finding claims.
I now have enough to make a final assessment. Let me verify both findings:
**Finding 1 (`web/src/lib/history.ts:209-211`):** The draft claims `describeUndo` returns `"Undone."` in ok tone when nothing was actually undone. Confirmed: `describeUndo` at line 209 has `if (result.conflicts.length === 0) return { tone: 'ok', message: 'Undone.' }`. The backend `history.go:73-76` returns 200 with `changeSet: nil` when "every revision resolved to a no-op." In that case `conflicts` is empty (len 0), so the UI reports "Undone." in ok tone — even though nothing was reverted. This is the "bare success hides the one that didn't" failure mode. The draft's secondary claim about "201 with zero counts" is weaker: looking at `commitScope` (revisions.go:152-155), a scope with no revisions returns `nil` — so the 201 path with a non-nil `cs` would only happen when revisions actually wrote. The 201-with-zero-counts path doesn't really exist per the service code. But the **200 with nil changeSet** path is real and confirmed. The finding survives on that primary path.
**Finding 2 (`web/src/lib/history.ts:107-109`):** Confirmed: `onError` only calls `refresh()` when `revertConflicts(err)` parses successfully (returns non-null). If a 409 body fails to parse, `revertConflicts` returns `null`, and no refresh fires — even though a 409 from this endpoint means "part may have applied." The surrounding code is explicitly careful about this (the comment on line 108 says "a partial revert still changed things"). This is a real defensive gap.
Both findings are confirmed against the actual source. The GardenEditorPage selection-tracking "finding" was already withdrawn by the original reviewer (it's documenting a non-issue), so it correctly does not appear in the findings array.
## VERDICT: Minor issues
I verified the phrasing logic (`describeUndo` / `describeConflict` / `describeCounts`) against the backend `RevertChangeSet` contract in `internal/api/history.go` and `internal/service/revisions.go`, and against the test expectations. The status-code → tone mapping (201 clean / 409 partial / 200 no-op) and the "2 of 3 changes undone — …" derivation all check out, and `revertConflicts` correctly recovers the partial result from the 409 body. Two real correctness gaps in the honest-reporting logic this PR centers on:
- **`web/src/lib/history.ts:209-211` — `describeUndo` reports `"Undone."` (ok tone) when the server actually changed nothing.** The early return `if (result.conflicts.length === 0) return { tone: 'ok', message: 'Undone.' }` fires without checking whether `result.changeSet` is null. The backend's "nothing happened" path — `internal/api/history.go:73-76` returns **200** with `changeSet: null` when every revision resolved to a no-op (already undone by hand) — has `conflicts` empty, so it lands here. The user clicks Undo, the server does nothing, and the UI answers `"Undone."` in the muted/ok tone: the exact "bare success would hide the one that didn't" failure mode the PR says it's eliminating, for the zero-applied case. (The draft also floated a "201 with zero counts" path; that one does not hold — `commitScope` at `internal/service/revisions.go:152-155` returns `nil` for a scope with no revisions, so a 201 always carries a real change set. The 200/null path is the real one.) Verified by reading `describeUndo` (history.ts:207-218) and tracing `internal/api/history.go:68-79` plus `internal/service/revisions.go:151-164`. Suggested fix: drop the early return when `conflicts.length === 0` and let the existing `applied === 0` branch handle it — e.g. `if (applied === 0) return { tone: 'error', message: 'Nothing was undone — it was already back to this state.' }` before the conflicts check, or add a distinct neutral tone.
- **`web/src/lib/history.ts:107-109` — a malformed 409 body leaves the canvas/history stale.** `useRevertChangeSet.onError` only calls `refresh()` when `revertConflicts(err)` parses successfully. If a 409 arrives with an unparseable body (proxy rewrite, version skew, schema drift), `revertConflicts` returns `null`, no refresh fires, but the backend may have applied part of the revert — so the canvas and history list stay out of sync while the user is shown a generic error. Low severity (defensive edge), but it's the same class of "partial revert changed things, refresh anyway" the surrounding code is careful about (the inline comment on line 108 says "a partial revert still changed things"). Suggested fix: refresh on any 409 regardless of body parse, since a 409 from this endpoint specifically means "something may have applied."
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
I'll verify each finding against the actual code.
Now let me verify the specific findings. First, find the #57/#53 references in DESIGN.md and PR description, and check `isPending` usage.
Let me verify the GardenEditorPage comment about #57/#53 to confirm the contradictory references finding.
I've confirmed all findings. Let me verify the findings:
1. **#57 contradictory references** - Confirmed. `UndoButton.tsx:7` and `history.ts:162` say "#57 = inline undo". `GardenEditorPage.tsx:289` says "chat (#57)". Also `DESIGN.md` (per diff) describes #57 as chat. The codebase itself contradicts what #57 means. Keep.
2. **isPending dead API** - Confirmed. `history.ts:192` returns `isPending: revert.isPending`. The only `isPending` references in callers are `history.isPending` (the query, not undo) and `outcome?.tone === 'pending'`. No caller uses `undo.isPending`. Keep.
3. **useRevertChangeSet/revertConflicts unnecessarily exported** - Confirmed. Grep shows both only used within history.ts (lines 108, 168, 178). No external import. Keep.
4. **HistoryPanel redundant heading** - Confirmed. `<h2>History</h2>` at line 23, while the tab is labeled "History" (from GardenEditorPage railTabs). For a screen reader, the tab + heading both announce "History". Keep as trivial.
5. **relativeTime in component file** - Confirmed. `relativeTime` is exported from `HistoryPanel.tsx` (a `.tsx` file), pure function with no JSX. Other pure helpers live in `web/src/lib/`. Keep as trivial.
All five findings survive verification.
**VERDICT: Minor issues**
- **`web/src/lib/history.ts:161-162` and `web/src/editor/UndoButton.tsx:5` — stale/contradictory issue references.** The comment in `history.ts` says the shared undo is "shared by the history list and (per #49) the agent turn's inline Undo in #57", and `UndoButton`'s doc comment says "the agent turn's inline undo (#57)". But `GardenEditorPage.tsx:289` (and the PR's `DESIGN.md` change) describe #57 as the **chat panel** ("the journal (#53) and chat (#57) panels too"). So the code comments point at #57 meaning one thing and the rest of the codebase points at #57 meaning another. Whichever is wrong, a reader chasing the reference will be misled. Fix: correct the issue numbers in the comments to match whatever #57 actually is, or drop the numeric references and describe the consumer by name.
- **`web/src/lib/history.ts:192` — `isPending` is dead public API.** `useUndo` returns `isPending: revert.isPending`, but no caller uses it. `UndoButton` keys its disabled/loading state off `outcome?.tone === 'pending'` (verified by grepping `isPending`/`outcomeFor`/`.undo(` usages across `web/src`; every other `isPending` reference is on a different query/mutation, not the undo hook). The field is exported surface that nothing reads. Either wire it up (e.g. disable all undo buttons while any revert is in flight) or drop it from the return.
- **`web/src/lib/history.ts:97` and `:86` — unnecessarily exported.** `useRevertChangeSet` and `revertConflicts` are both `export`ed, but grep confirms each is only called from within `history.ts` itself (`useRevertChangeSet` at line 168, `revertConflicts` at lines 108 and 178; no test or component imports either). They don't need to be part of the module's public surface. Dropping the `export` keeps the API surface to what `useUndo`/`describeUndo`/the schemas actually offer. Small, but it's exactly the kind of leaky surface that accretes over time.
- **`web/src/editor/HistoryPanel.tsx:23` — redundant heading.** The panel renders `<h2>History</h2>` while the tab button that opened it is already labeled "History" (the `EditorRail` tab in `GardenEditorPage.tsx` with `aria-current="page"` marking the active tab). For a screen-reader user the panel's name is announced twice. Either drop the `<h2>` (the tab is the label) or give the heading something the tab doesn't say (e.g. the garden name). Trivial.
- **`web/src/editor/HistoryPanel.tsx:118` — `relativeTime` is a pure util living in a component file.** It's a stateless, testable string formatter exported from a `.tsx` component module (no JSX in the function). The rest of the codebase puts pure helpers in `web/src/lib/` (e.g. `lib/geometry.ts`, `lib/units.ts`); `lib/history.ts` already lives next door and would be its natural home (it has no JSX and is the file that owns the `ChangeSet` shape). Move it so the component file is just the component. Trivial, but it's the pattern break the CLAUDE.md "conventions that bite" section is about.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
Through the performance lens, I verified the new code paths and found nothing material:
- **`useGardenHistory`** (`web/src/lib/history.ts:68`) — single `useInfiniteQuery`, offset-paged at `PAGE_SIZE = 30`. `HistoryPanel` is only mounted when the History tab is the active rail tab (`EditorRail` calls `active.render()` exclusively, line 84 of `EditorRail.tsx`), so the query doesn't fire in the background while the inspector is open. No N+1, no unbounded fetch.
- **`useUndo` outcomes state** (`history.ts:169`) — keyed by change set id, one entry added per user undo click. Bounded by user actions, and the whole map is discarded when `HistoryPanel` unmounts on tab switch. Not a leak.
- **`HistoryPanel` flatten** (`HistoryPanel.tsx:19`) — `pages.flatMap` runs per render; for a few pages of 30 entries this is trivial and not in a hot loop (undo is a user action, not a per-frame path).
- **`railTabs` array** (`GardenEditorPage.tsx:291`) — recreated per render with new closures, but only the active tab's `render()` is invoked, so inactive tabs (e.g. the `Inspector`/`PlopInspector` subtree) are not built. This is actually a slight win over the old layout, which always mounted the inspector when something was selected.
- **Invalidation on undo** (`history.ts:99-102`) — `refresh()` invalidates `gardenFullKey` + `historyKey`. Fires once per undo (user action), not in a drag/loop. Correct, not a regression.
No quadratic behavior, no missing limits, no hot-path blocking.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
All three findings confirmed against the actual code.
**Verdict: Minor issues**
- **`web/src/editor/HistoryPanel.tsx:45` — unhandled rejection on `fetchNextPage`.** The "Load older" button does `onClick={() => void history.fetchNextPage()}`. In TanStack Query v5 (repo pins `@tanstack/react-query ^5.62.0`), `fetchNextPage()` returns a promise that rejects when the page request fails (network error, 401, etc.). `void`-discarding it without `.catch()` leaves an unhandled promise rejection — visible in the console and, in some environments, surfaces as a runtime warning. Suggested fix: `onClick={() => { history.fetchNextPage().catch(() => {}) }}`. Confirmed the only `fetchNextPage` caller in the repo, so no existing convention to match.
- **`web/src/editor/HistoryPanel.tsx:26` — misleading error message when only the *next* page fails.** `history.isError` becomes true on a `fetchNextPage` failure too (TanStack sets `error`/`isError` for any fetch, not just the initial one). When the user clicks "Load older" and that request fails, the panel renders `<Alert>Could not load history.</Alert>` above the already-loaded entries — which reads as "the whole history failed," even though the older entries are still on screen and retryable via the same button. Concrete impact: a transient network blip on paging makes it look like history is gone. Suggested fix: distinguish initial-load failure from next-page failure (e.g. `history.isError && !history.data` for the alert; show a smaller inline retry message near "Load older" when `history.data` exists but the last fetch failed).
- **`web/src/lib/history.ts:177-186` — a 409 with an unparseable body is reported as a plain failure even though part may have applied.** `revertConflicts` uses `safeParse` and returns `null` if the 409 body doesn't match `revertResultSchema`. The hook then falls back to `{ tone: 'error', message: "That couldn't be undone." }` (history.ts:183). The whole premise of this code (per the file's header comment, lines 4-7) is that a 409 means "partly applied, report both." If the server ever returns a 409 with a body shape the client doesn't recognize, the user is told "couldn't be undone" while the canvas may have redrawn with real changes — exactly the lie the PR set out to avoid. This is a contract-coupling concern more than a likely bug (same team owns both sides), so confidence is medium. Suggested fix: on a 409 with an unparseable body, still refresh and report a generic "Something was undone, but details weren't available" partial-style message rather than a flat error.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
Security lens: the new history and revert endpoints are properly authenticated, authorized, and validated; the frontend renders user-controlled data safely; and the state-changing revert is behind the existing CSRF guard.
- **`GET /gardens/:id/history`** — `parseIDParam` enforces positive int64; `intQuery` sanitizes `limit`/`offset`; `GardenHistory` clamps `limit` to [1, 100] and requires `roleViewer` via `requireGardenRole` (existence masked as `ErrNotFound`).
- **`POST /change-sets/:id/revert`** — `parseIDParam` enforces positive int64; `RevertChangeSet` loads the change set then requires `roleEditor` on its garden. The v1 group already applies `csrfGuard()` (Origin-check), so the state-changing POST is CSRF-protected.
- **No injection**: store queries are fully parameterized; no SQL injection vector.
- **No XSS**: `changeSet.summary`, `actorName`, conflict names, and `relativeTime` output are all rendered as React text nodes (no `dangerouslySetInnerHTML`, no template strings interpolated into HTML).
- **No secret leakage or unsafe deserialization**: API responses are Zod-schematized; no credentials in code.
- **Race/authorization**: `canEdit` only gates the undo button in the UI; the backend independently re-checks editor role, so bypassing the UI still fails authz.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Verdict: Minor issues**
Two real correctness problems, both introduced by this PR:
* **`web/src/pages/GardenEditorPage.tsx:123-126` — selection-change id does not auto-switch to Inspector.**
The effect that auto-switches the rail tab depends only on the `hasSelection` *boolean*, not on the actual selection ids:
```ts
const hasSelection = selectedId != null || selectedPlantingId != null
useEffect(() => {
if (hasSelection) setRailTab('inspector')
else if (useEditorStore.getState().railTab === 'inspector') setRailTab(null)
}, [hasSelection, setRailTab])
```
Because `hasSelection` stays `true` when the user clicks from object A to object B, the effect never fires and the rail stays on whatever tab it was on (e.g. History). This contradicts the stated design constraint: *"Selecting an object still lands you in the inspector without a click."* The fix is to put `selectedId` and `selectedPlantingId` in the dependency array so any change of selection re-routes to the inspector.
* **`web/src/editor/HistoryPanel.tsx:118-131` — `relativeTime` can mislabel same-day events as "yesterday".**
The day computation uses `Math.round` at every boundary:
```ts
const hours = Math.round(minutes / 60)
// ...
const days = Math.round(hours / 24)
```
At the day boundary this over-rounds: an event at 00:00 today viewed at 23:30 today is 23.5 hours ago → `hours = Math.round(1410/60) = 24` → `days = Math.round(24/24) = 1` → **"yesterday"**, even though the event occurred today. Using `Math.floor` for the day step (or comparing calendar dates) would fix the semantic error.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`relativeTime` is exported from a component file instead of `lib/`**
`HistoryPanel.tsx:118` exports `relativeTime`, a pure date-formatting utility with no React or UI dependencies. If the journal (#53) or chat (#57) later wants the same phrasing, they would have to import it from a panel component, breaking the `lib/` vs. `components/` boundary the rest of the codebase observes (e.g., `geometry.ts`, `units.ts`).
**Fix:** move `relativeTime` into `web/src/lib/history.ts` (or a new `time.ts`) and import it in `HistoryPanel`.
- **`useUndo` exposes `isPending` that no caller consumes**
`history.ts:189–193` returns `isPending: revert.isPending`, but neither `HistoryPanel` nor `UndoButton` reads it; each button tracks its own pending state via `outcomeFor(id)?.tone === 'pending'`. The exported flag is therefore dead weight that misleadingly suggests "any revert is pending" rather than per-item status.
**Fix:** drop `isPending` from the return object, or replace it with a per-id predicate such as `isPendingFor(id)` if a future consumer needs it.
</details>
<details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`web/src/editor/HistoryPanel.tsx:45` — Pagination failure is silently swallowed.** `history.fetchNextPage()` is fire-and-forget via `void`. If the network fails while loading older history, the button stops spinning and no error is shown to the user. TanStack Query exposes `isFetchNextPageError` / `fetchNextPageError` for this; the component ignores them. **Fix:** Wrap the call, catch errors, and surface them in the panel (e.g., a small inline alert or a toast).
- **`web/src/lib/history.ts:21` — `n` in `changeCountSchema` accepts negative numbers.** `z.number()` allows negatives. A server bug returning `n: -1` would silently distort `totalChanges` (possibly dropping below zero and hiding the undo button) while `describeCounts` filters negatives out entirely. **Fix:** Tighten to `z.number().int().nonnegative()` so a bad payload fails loudly at parse time instead of producing confusing UI.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
The best catch was one I'd have missed: the store already has resetTransient(),
a single list of ephemeral editor state, and the new railTab didn't join it —
so the public garden page cleared everything except the rail. Rather than adding
railTab in two places, the editor page now calls resetTransient() instead of
maintaining its own parallel list, which is what let them drift in the first
place.
The rail auto-switch keyed off a "something is selected" boolean, so it fired on
the transition into having a selection and never again. Select a bed, switch to
History, select a different bed — the boolean never changed, so the inspector
never came forward, breaking the constraint the whole design was built around.
Now keyed off the selected ids.
Closing the rail cleared the canvas selection regardless of which tab you were
on, so dismissing History deselected your bed. Only the inspector is about the
selection, so only closing it deselects.
describeUndo said "Undone." when the server reported a complete no-op (200 with
a null change set, reachable by undoing a creation whose object is already
gone). That reports work that didn't happen; it now says so.
relativeTime rounded, so 18 hours ago read as "yesterday" and 90 minutes as "2h
ago" — rounding up into the next unit reads as a bigger gap than actually
elapsed. Floors throughout. Clock skew that puts a just-written entry slightly
in the future now reads "just now" rather than falling through the negative.
A failed "Load older" was swallowed entirely: the button simply stopped working.
It now reports the error, while the top-level alert only takes over when there
is nothing on screen at all.
changeCountSchema took any number. Counts come from COUNT(*) and can only be
non-negative integers, so constraining them makes bad data fail loudly instead
of quietly hiding an Undo button (totalChanges gates it) or rendering
"1.5 beds changed". Dropped useUndo's unused isPending — the per-change-set
outcome already carries it, and per-row is right anyway.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Fixed in 9dc4d3a. Two of these were real design breaks, not polish.
The store already had a reset, and my new state didn't join it
resetTransient() is the single list of ephemeral editor state, and railTab wasn't in it — so PublicGardenPage cleared everything except the rail. The fix isn't to add railTab in two places: the editor page now calls resetTransient() instead of maintaining its own parallel list, which is exactly what let the two drift. Good catch — I'd added state next to a mechanism designed to prevent this and walked straight past it.
The auto-switch broke the constraint the whole design rests on
It keyed off a hasSelection boolean, so it fired on the transition into having a selection and never again. Select a bed → switch to History → select a different bed: the boolean never changed, so the inspector never came forward. That's the one thing #49 said the rail must never do. Now keyed off the selected ids.
Related, from kimi: closing the rail cleared the canvas selection no matter which tab you were on, so dismissing History deselected your bed. Only the inspector is about the selection, so only closing it deselects.
The rest
describeUndo said "Undone." for a complete no-op (200 + null change set — reachable by undoing a creation whose object is already gone). That reports work that didn't happen. It now says Nothing left to undo — this was already reversed., with a test.
relativeTime rounded, so 18 hours read as "yesterday" and 90 minutes as "2h ago" — rounding up into the next unit reads as a bigger gap than actually elapsed. Floors throughout. Clock skew putting a just-written entry slightly in the future now reads "just now" rather than falling through a negative.
A failed "Load older" was swallowed — the button just stopped working. It now reports the error, and the top-level alert only takes over when there's nothing on screen at all.
changeCountSchema took any number. Counts come from COUNT(*); constraining to non-negative integers makes bad data fail loudly rather than quietly hiding an Undo button (totalChanges gates it) or rendering "1.5 beds changed".
Dropped useUndo's unused isPending — the per-change-set outcome already carries it, and per-row is the right granularity anyway.
Declined, with reasoning
Undo invalidates the whole infinite query. Intentional. An undo prepends a row, which shifts every offset; refetching only page one would leave the later pages misaligned with the new offsets. At household scale that's one or two pages.
useUndo outcomes are lost when the History tab unmounts. They're transient feedback about an action you just took, and the canvas and the list have both already updated to show the result. Persisting them past a tab switch would mean stale messages about changes now several edits old.
Unbounded outcomes growth. One entry per change set you personally undo in one session, cleared on navigation.
65 tests, tsc --noEmit, npm run build green.
Fixed in `9dc4d3a`. Two of these were real design breaks, not polish.
### The store already had a reset, and my new state didn't join it
`resetTransient()` is the single list of ephemeral editor state, and `railTab` wasn't in it — so `PublicGardenPage` cleared everything *except* the rail. The fix isn't to add `railTab` in two places: the editor page now calls `resetTransient()` instead of maintaining its own parallel list, which is exactly what let the two drift. Good catch — I'd added state next to a mechanism designed to prevent this and walked straight past it.
### The auto-switch broke the constraint the whole design rests on
It keyed off a `hasSelection` boolean, so it fired on the transition *into* having a selection and never again. Select a bed → switch to History → select a **different** bed: the boolean never changed, so the inspector never came forward. That's the one thing #49 said the rail must never do. Now keyed off the selected ids.
Related, from `kimi`: closing the rail cleared the canvas selection no matter which tab you were on, so dismissing History deselected your bed. Only the inspector is *about* the selection, so only closing it deselects.
### The rest
- **`describeUndo` said "Undone." for a complete no-op** (200 + null change set — reachable by undoing a creation whose object is already gone). That reports work that didn't happen. It now says `Nothing left to undo — this was already reversed.`, with a test.
- **`relativeTime` rounded**, so 18 hours read as "yesterday" and 90 minutes as "2h ago" — rounding *up* into the next unit reads as a bigger gap than actually elapsed. Floors throughout. Clock skew putting a just-written entry slightly in the future now reads "just now" rather than falling through a negative.
- **A failed "Load older" was swallowed** — the button just stopped working. It now reports the error, and the top-level alert only takes over when there's nothing on screen at all.
- **`changeCountSchema` took any number.** Counts come from `COUNT(*)`; constraining to non-negative integers makes bad data fail loudly rather than quietly hiding an Undo button (`totalChanges` gates it) or rendering "1.5 beds changed".
- **Dropped `useUndo`'s unused `isPending`** — the per-change-set outcome already carries it, and per-row is the right granularity anyway.
### Declined, with reasoning
- **Undo invalidates the whole infinite query.** Intentional. An undo *prepends* a row, which shifts every offset; refetching only page one would leave the later pages misaligned with the new offsets. At household scale that's one or two pages.
- **`useUndo` outcomes are lost when the History tab unmounts.** They're transient feedback about an action you just took, and the canvas and the list have both already updated to show the result. Persisting them past a tab switch would mean stale messages about changes now several edits old.
- **Unbounded `outcomes` growth.** One entry per change set you personally undo in one session, cleared on navigation.
65 tests, `tsc --noEmit`, `npm run build` green.
steve
merged commit 2a8fe24766 into main2026-07-21 05:23:25 +00:00
steve
deleted branch feat/history-panel2026-07-21 05:23:25 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #49. Completes Phase 0 of #58.
#48 made every change revertible; this makes that reachable. The bar the issue set: after the agent rearranges a bed, undoing it should take one obvious click, not a hunt through a menu.
The rail-layout decision (the part #53 and #57 depend on)
Four things wanted one strip of screen — inspector, history, journal (#53), chat (#57). They're tabs in one
EditorRailrather than each bolting on its own chrome. The canvas stays one width instead of a different width per panel, and adding the journal or chat later is adding a tab.Both constraints from the issue held:
On a phone the same tabs render in the bottom sheet the inspector already lived in, so mobile didn't need a second layout.
Honest reporting is most of the work here
A reverted entry stays in the list, struck through and marked, and the revert appears as its own entry — because that's what it is. Making the original disappear would be rewriting history rather than appending to it, and would leave no way to undo the undo.
A 409 from a revert is not a plain failure. It carries the change set that did apply alongside the entities deliberately left alone, so the message is:
A bare failure toast would be a lie about the two that applied. A bare success would hide the one that didn't. Every conflict is listed, not just the first, and each reason (
changed/missing/exists/unsupported) gets its own phrasing.The panel also says plainly that deleting a whole garden isn't covered and can't be undone, rather than leaving that to be discovered — matching the deliberate gap documented in #48.
One undo, not two
The issue asked for a single implementation shared with #57's inline undo.
useUndoowns the mutation, the per-change-set outcome and the phrasing;UndoButtonrenders it. #57 will call the same hook, so undo behaves identically wherever it's offered — including how it explains a partial result.Verification
64 tests (9 new), covering the phrasing logic that carries the user-facing behaviour: pluralization per entity, "says nothing rather than 0 changes", named vs unnamed conflicts, and all three undo outcomes (clean / partial / nothing-applied).
tsc --noEmit,npm run build,go test ./...green.I'll exercise the real loop — move a bed, undo it, watch the canvas redraw — against the live deploy once this merges, and report back on the PR.
🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-21 05:21:13Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 19 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -107,2 +107,3 @@- **State:** TanStack Query for all server state (editor keyed on `gardens/:id/full`; optimistic mutations with version-conflict rollback). One small Zustand store for ephemeral editor state only: viewport, selection, focused object, active tool, in-flight drag.- **Editor components (`web/src/editor/`):** `GardenCanvas` (svg root + viewport g), `useViewport` (use-gesture pan/zoom/pinch), `ObjectShape`, `PlopMarker` (semantic-zoom branching), `Palette` (drag-to-place object kinds), `Inspector` (side panel; bottom sheet on mobile), `PlantPicker`. Pure geometry helpers (local↔world transforms, unit formatting) in `web/src/lib/geometry.ts`, unit-tested.- **Editor components (`web/src/editor/`):** `GardenCanvas` (svg root + viewport g), `useViewport` (use-gesture pan/zoom/pinch), `ObjectShape`, `PlopMarker` (semantic-zoom branching), `Palette` (drag-to-place object kinds), `EditorRail` (the one side panel), `Inspector`, `HistoryPanel`, `PlantPicker`.- **One rail, tabs inside it.** The inspector, history, and later the journal and chat panel all want the same strip of screen; rather than each bolting on its own chrome they are tabs in `EditorRail` — so the canvas is one width instead of a different width per panel, and adding a panel is adding a tab. Selecting an object switches to the Inspector tab automatically, so the rail is never something you operate before you can edit; on a phone the same tabs render in the bottom sheet the inspector already used. Pure geometry helpers (local↔world transforms, unit formatting) in `web/src/lib/geometry.ts`, unit-tested.🟡 VERDICT: Minor issues
maintainability · flagged by 1 model
web/src/editor/store.ts:97-106—resetTransientomits the newrailTabfield, contradicting its own "clear all transient state" comment. Confirmed:GardenEditorPage.tsx's parallelclear()(lines 86-94) does callsetRailTab(null), butresetTransient(called fromPublicGardenPage.tsx:28) does not. -web/src/editor/HistoryPanel.tsx:118—relativeTimeis a pure, dependency-free helper exported from a component file rather thanweb/src/lib/, breaking the project's established l…🪰 Gadfly · advisory
@@ -0,0 +66,4 @@)}>{tab.label}{tab.badge != null && tab.badge !== false && tab.badge !== 0 && (⚪ badge render condition stacks three ad-hoc truthiness checks instead of a named helper
maintainability · flagged by 1 model
web/src/editor/EditorRail.tsx:69—badgerender condition stacks three ad-hoc truthiness checks.tab.badge != null && tab.badge !== false && tab.badge !== 0(line 69) is three stacked checks to express "a count, or a plain marker, never shown for zero/false." A named helper (e.g.hasBadge) or normalizing thebadgetype tonumber | true | undefinedat theRailTabinterface would make the intent legible at the call site. Severity trivial, high confidence.🪰 Gadfly · advisory
@@ -0,0 +15,4 @@*/export function HistoryPanel({ gardenId, canEdit }: { gardenId: number; canEdit: boolean }) {const history = useGardenHistory(gardenId)const undo = useUndo(gardenId)🟡 HistoryPanel re-derives sets and per-entry counts on every render with no memoization
performance · flagged by 1 model
HistoryPanelre-runsflatMapover all loaded pages on every render (web/src/editor/HistoryPanel.tsx:19).setsis recomputed each render fromhistory.data?.pages, and each entry maps to aHistoryEntrythat callsdescribeCounts/totalChanges(reduce over counts). With offset paging at 30/page and a typical user scrolling a page or two this is negligible; butdescribeCountsandtotalChangesare re-derived per render per entry with no memoization, and the parent re-renders o…🪰 Gadfly · advisory
@@ -0,0 +20,4 @@return (<div className="flex flex-col gap-3"><h2 className="text-sm font-semibold text-fg">History</h2>🟡 history.isError shows "Could not load history" even when only the next page failed, not the initial load
error-handling, maintainability · flagged by 1 model
web/src/editor/HistoryPanel.tsx:26— misleading error message when only the next page fails.history.isErrorbecomes true on afetchNextPagefailure too (TanStack setserror/isErrorfor any fetch, not just the initial one). When the user clicks "Load older" and that request fails, the panel renders<Alert>Could not load history.</Alert>above the already-loaded entries — which reads as "the whole history failed," even though the older entries are still on screen and retryable…🪰 Gadfly · advisory
@@ -0,0 +42,4 @@variant="ghost"className="text-sm"disabled={history.isFetchingNextPage}onClick={() => void history.fetchNextPage()}🟠 Pagination failure silently swallowed — no user feedback when fetching older history fails
error-handling · flagged by 2 models
web/src/editor/HistoryPanel.tsx:45— Pagination failure is silently swallowed.history.fetchNextPage()is fire-and-forget viavoid. If the network fails while loading older history, the button stops spinning and no error is shown to the user. TanStack Query exposesisFetchNextPageError/fetchNextPageErrorfor this; the component ignores them. Fix: Wrap the call, catch errors, and surface them in the panel (e.g., a small inline alert or a toast).🪰 Gadfly · advisory
@@ -0,0 +101,4 @@{reverted && <p className="mt-1 text-xs text-muted">Undone</p>}</div>{canEdit && totalChanges(changeSet) > 0 && (🟡 totalChanges recomputed per entry when describeCounts already walked the same counts
maintainability · flagged by 1 model
web/src/editor/HistoryPanel.tsx:104—totalChangesrecomputed per entry despitedescribeCountsalready walking the same counts.HistoryEntrycomputescounts = describeCounts(changeSet)at line 70 (which filtersn > 0), then the undo-button guard at line 104 callstotalChanges(changeSet)— a second fullreduceovercs.counts(history.ts:115). Both helpers are already imported; a single-pass "has any change" check would suffice. Minor churn, easy to mistake for doing diff…🪰 Gadfly · advisory
@@ -0,0 +115,4 @@}/** A compact "3m ago" / "yesterday" for a UTC timestamp. */export function relativeTime(iso: string): string {🟡 VERDICT: Minor issues
correctness, maintainability · flagged by 3 models
web/src/editor/store.ts:97-106—resetTransientomits the newrailTabfield, contradicting its own "clear all transient state" comment. Confirmed:GardenEditorPage.tsx's parallelclear()(lines 86-94) does callsetRailTab(null), butresetTransient(called fromPublicGardenPage.tsx:28) does not. -web/src/editor/HistoryPanel.tsx:118—relativeTimeis a pure, dependency-free helper exported from a component file rather thanweb/src/lib/, breaking the project's established l…🪰 Gadfly · advisory
@@ -0,0 +125,4 @@const hours = Math.round(minutes / 60)if (hours < 24) return `${hours}h ago`const days = Math.round(hours / 24)if (days === 1) return 'yesterday'🟡 relativeTime misreports future dates as 'just now'
error-handling · flagged by 1 model
web/src/editor/HistoryPanel.tsx:128—relativeTimereturns"just now"for any timestamp in the future (e.g. server clock skew), because the negativesecondsvalue falls through the first< 60check. Future dates should be handled explicitly rather than misreporting them as the recent past.🪰 Gadfly · advisory
@@ -0,0 +18,4 @@export const changeCountSchema = z.object({entityType: z.enum(['garden', 'object', 'planting']),op: z.enum(['create', 'update', 'delete']),n: z.number(),🟠 changeCountSchema allows negative counts, which can hide undo buttons or produce incorrect totals
error-handling · flagged by 1 model
web/src/lib/history.ts:21—ninchangeCountSchemaaccepts negative numbers.z.number()allows negatives. A server bug returningn: -1would silently distorttotalChanges(possibly dropping below zero and hiding the undo button) whiledescribeCountsfilters negatives out entirely. Fix: Tighten toz.number().int().nonnegative()so a bad payload fails loudly at parse time instead of producing confusing UI.🪰 Gadfly · advisory
@@ -0,0 +24,4 @@export const changeSetSchema = z.object({id: z.number(),gardenId: z.number(),🔴 Unconstrained z.number() in changeCountSchema allows invalid server data to hide undo button or show fractional counts
error-handling · flagged by 1 model
web/src/lib/history.ts:27—changeCountSchemausesz.number()without.int()or.nonnegative(), allowing any numeric value (negative, fractional,NaN,Infinity) from a misbehaving server to pass validation.totalChangessums these blindly, so a negative orNaNcount causestotalChanges(changeSet) > 0to evaluate to false and silently hides the Undo button, whiledescribeCountswould render strings like"2.5 plantings added"for fractional values. The schema should c…🪰 Gadfly · advisory
@@ -0,0 +94,4 @@* the history list so the new entry (and the "reverted" mark on its target)* appear — on failure too, because a 409 means part of it applied.*/export function useRevertChangeSet(gardenId: number) {🟡 useRevertChangeSet and revertConflicts are exported but only used within history.ts
maintainability · flagged by 1 model
web/src/lib/history.ts:97and:86— unnecessarily exported.useRevertChangeSetandrevertConflictsare bothexported, but grep confirms each is only called from withinhistory.tsitself (useRevertChangeSetat line 168,revertConflictsat lines 108 and 178; no test or component imports either). They don't need to be part of the module's public surface. Dropping theexportkeeps the API surface to whatuseUndo/describeUndo/the schemas actually offer. Small, but it's exa…🪰 Gadfly · advisory
@@ -0,0 +98,4 @@const qc = useQueryClient()const refresh = () => {void qc.invalidateQueries({ queryKey: gardenFullKey(gardenId) })void qc.invalidateQueries({ queryKey: historyKey(gardenId) })🟠 409 with unparseable body skips cache refresh despite likely partial write
correctness, error-handling, performance · flagged by 3 models
web/src/lib/history.ts:107-108— a 409 whose body fails to parse skips the refresh, even though partial work likely applied. The top-level mutation'sonErroronly callsrefresh()whenrevertConflicts(err)is truthy, i.e. only when the 409 body successfully parses againstrevertResultSchema. But a 409 is by this PR's own design the "part of it applied" path. If the server returns a 409 with a body that omits thechangeSetkey (the schema requires it; there's no.default) or…🪰 Gadfly · advisory
@@ -0,0 +158,4 @@}/*** The one undo implementation, shared by the history list and (per #49) the🟡 Comments reference #57 as 'inline undo' but GardenEditorPage/DESIGN describe #57 as the chat panel — misleading issue references
maintainability · flagged by 1 model
web/src/lib/history.ts:161-162andweb/src/editor/UndoButton.tsx:5— stale/contradictory issue references. The comment inhistory.tssays the shared undo is "shared by the history list and (per #49) the agent turn's inline Undo in #57", andUndoButton's doc comment says "the agent turn's inline undo (#57)". ButGardenEditorPage.tsx:289(and the PR'sDESIGN.mdchange) describe #57 as the chat panel ("the journal (#53) and chat (#57) panels too"). So the code comments point a…🪰 Gadfly · advisory
@@ -0,0 +166,4 @@*/export function useUndo(gardenId: number) {const revert = useRevertChangeSet(gardenId)const [outcomes, setOutcomes] = useState<Record<number, UndoOutcome>>({})🟠 useUndo outcomes lost when History tab unmounts
correctness, maintainability, performance · flagged by 2 models
web/src/lib/history.ts:169+web/src/editor/EditorRail.tsx:85—useUndostores per-change-set outcomes in localuseState, butEditorRailmounts only the active tab (active.render()). If a user clicks Undo, switches to the Inspector tab while the mutation is in flight, and later returns to History,HistoryPanelremounts with a freshuseUndoinstance. The pending/success/error outcome is lost, so the undo button appears clickable again even though the revert mutation may…🪰 Gadfly · advisory
@@ -0,0 +175,4 @@setOutcomes((prev) => ({ ...prev, [cs.id]: describeUndo(cs, result) }))},onError: (err) => {const result = revertConflicts(err)🟡 A 409 with an unparseable body is reported as a flat failure even though part of the revert may have applied
error-handling · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +189,4 @@return {undo,outcomeFor: (id: number): UndoOutcome | undefined => outcomes[id],isPending: revert.isPending,🟡 useUndo returns isPending but no caller uses it (dead public API)
maintainability · flagged by 2 models
web/src/lib/history.ts:192—isPendingis dead public API.useUndoreturnsisPending: revert.isPending, but no caller uses it.UndoButtonkeys its disabled/loading state offoutcome?.tone === 'pending'(verified by greppingisPending/outcomeFor/.undo(usages acrossweb/src; every otherisPendingreference is on a different query/mutation, not the undo hook). The field is exported surface that nothing reads. Either wire it up (e.g. disable all undo buttons while any re…🪰 Gadfly · advisory
@@ -0,0 +206,4 @@*/export function describeUndo(target: ChangeSet, result: RevertResult): UndoOutcome {const skipped = result.conflicts.map(describeConflict).join('; ')if (result.conflicts.length === 0) {🔴 describeUndo reports 'Undone.' on a backend no-op (changeSet: null, conflicts: []), reachable via 'Undo again' on a create-type entry whose entity is already gone
correctness, error-handling · flagged by 2 models
Finding 1 (
web/src/lib/history.ts:209-211): The draft claimsdescribeUndoreturns"Undone."in ok tone when nothing was actually undone. Confirmed:describeUndoat line 209 hasif (result.conflicts.length === 0) return { tone: 'ok', message: 'Undone.' }. The backendhistory.go:73-76returns 200 withchangeSet: nilwhen "every revision resolved to a no-op." In that caseconflictsis empty (len 0), so the UI reports "Undone." in ok tone — even though nothing was reverted. This…🪰 Gadfly · advisory
@@ -113,0 +120,4 @@// back out of the inspector, but leaves History/Chat open if that's where you// were, since those aren't about the selection.const hasSelection = selectedId != null || selectedPlantingId != nulluseEffect(() => {🟠 Rail auto-switch-to-Inspector effect keys off a boolean, so it misses selection changes while already selected
correctness · flagged by 3 models
web/src/pages/GardenEditorPage.tsx:123-126— selection-change id does not auto-switch to Inspector. The effect that auto-switches the rail tab depends only on thehasSelectionboolean, not on the actual selection ids:ts const hasSelection = selectedId != null || selectedPlantingId != null useEffect(() => { if (hasSelection) setRailTab('inspector') else if (useEditorStore.getState().railTab === 'inspector') setRailTab(null) }, [hasSelection, setRailTab])BecausehasSelection…🪰 Gadfly · advisory
@@ -366,0 +404,4 @@tabs={railTabs}activeId={railTab}onActivate={setRailTab}onClose={() => {🟠 Closing rail from History tab clears canvas selection
correctness · flagged by 1 model
web/src/pages/GardenEditorPage.tsx:407—EditorRail'sonCloseunconditionally callsselect(null)andselectPlanting(null). Closing the rail from the History tab (where selection is irrelevant) still clears the user's canvas selection. A user viewing history for the currently selected object loses that selection when they close the panel, forcing them to re-select before they can keep editing. The close handler should only clear selection when the active tab isinspector, or…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 21 findings (8 with multi-model agreement)
web/src/lib/history.ts:101web/src/pages/GardenEditorPage.tsx:123web/src/editor/HistoryPanel.tsx:118web/src/lib/history.ts:209web/src/editor/HistoryPanel.tsx:45web/src/editor/store.ts:97web/src/lib/history.ts:169web/src/lib/history.ts:19213 single-model findings (lower confidence)
web/src/lib/history.ts:27web/src/lib/history.ts:21web/src/pages/GardenEditorPage.tsx:407DESIGN.md:109PublicGardenPage.tsx:28web/src/editor/HistoryPanel.tsx:18web/src/editor/HistoryPanel.tsx:23web/src/editor/HistoryPanel.tsx:104web/src/editor/HistoryPanel.tsx:128web/src/lib/history.ts:97web/src/lib/history.ts:161web/src/lib/history.ts:178web/src/editor/EditorRail.tsx:69Per-model detail
claude-code/sonnet (claude-code) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
This backend authz behaves correctly (existence masking preserved, viewer/editor role gating enforced independently of client state). This diff is frontend-only (no backend changes), and the backend endpoints it consumes were already reviewed under #48. My security-lens review of this diff specifically:
VERDICT: No material issues found
Findings, all verified:
canEdit(GardenEditorPage.tsx:74) gates the Undo button only for UX; the actual revert (internal/service/revisions.go:281-291) independently requiresroleEditorviarequireGardenRole, and history reads requireroleViewer. A tampered client state can't escalate privilege.historyKey/useGardenHistorybuild query params throughapi.get(..., { params }), which routes throughbuildUrl→URLSearchParams(web/src/lib/api.ts:47-57) — no string concatenation into the URL.gardenId/changeSetIdused in path segments (web/src/lib/history.ts:59,73) are numeric, sourced from route params / server-issuedChangeSet.id, not free-form user text.changeSet.summary,actorName,RevertConflict.name(rendered viadescribeConflict,HistoryPanel.tsx,UndoButton.tsx) — are rendered as JSX text children, which React escapes; nodangerouslySetInnerHTML, noeval, no manual HTML construction anywhere in the new files.changeSetSchema/revertResultSchema/historyPageSchema(web/src/lib/history.ts) use zod to validate the shape of server responses before use, so a malformed/unexpected payload fails closed (schema parse throws /safeParsereturns unsuccessful) rather than being trusted blindly.revertConflicts()(web/src/lib/history.ts:56-60) only trustserr.bodyafterrevertResultSchema.safeParse, so an unexpected error body can't be coerced into a fabricated "partial success" UI state.Nothing in this diff introduces a new authn/authz gap, injection point, unsafe deserialization, or secret exposure. The one thing worth flagging as a pattern to watch (not a bug here):
errorMessage()surfaces the raw server error message verbatim in the UI (HistoryPanel.tsxAlert,UndoButton.tsx/describeUndofallback) — fine as long as the Go service never puts sensitive detail (row contents, internal IDs of entities the actor can't see) intowriteServiceErrormessages, which is pre-existing behavior this PR doesn't change.🎯 Correctness — Minor issues
Both findings check out exactly against the code. Line numbers, logic, and referenced files all confirmed by direct inspection.
VERDICT: Minor issues
web/src/pages/GardenEditorPage.tsx:122-126— the rail auto-switch-to-Inspector effect keys off the derived booleanhasSelection(selectedId != null || selectedPlantingId != null), not the selection identity, so it only fires on afalse → truetransition. Confirmed:GardenCanvas.tsx:247wiresonSelect={select}directly to any object click, andstore.ts:71(select: (id) => set({ selectedId: id, selectedPlantingId: null })) never clears selection before setting a new id — so clicking bed B while bed A is already selected keepshasSelectiontruethroughout and the effect's[hasSelection, setRailTab]deps don't re-run. Reproduction: select bed A (auto-switches to Inspector) → manually open History via the unconditional toggle atGardenEditorPage.tsx:345-351→ click bed B on the canvas. The rail stays on History instead of switching to Inspector for B, contradicting the documented invariant at lines 118-121 ("Selecting anything lands you in the inspector without a click"). Same gap crossing kinds:selectPlanting(store.ts:74) clearsselectedIdbut setsselectedPlantingId, sohasSelectionstaystruewhen selecting a plop while History is open. Fix: depend on[selectedId, selectedPlantingId, setRailTab]instead of the boolean.web/src/lib/history.ts:207-218(describeUndo) — when the backend returnschangeSet: null, conflicts: [](the documentedcs == nil/ 200 "already undone by hand" no-op path atinternal/api/history.go:73-76), theresult.conflicts.length === 0check short-circuits to{ tone: 'ok', message: 'Undone.' }even though nothing was actually reverted this time. Confirmed this exact input combination isn't exercised byhistory.test.ts, which only covers conflicts-empty-with-a-real-changeSet (plain success), partial, and error-with-conflicts cases — the null-changeSet/empty-conflicts combination falls through untested to the same "Undone." message as a real revert.🧹 Code cleanliness & maintainability — Minor issues
I independently verified both findings in the draft against the actual source — both survive:
VERDICT: Minor issues
web/src/editor/store.ts:97-106—resetTransientomits the newrailTabfield, contradicting its own "clear all transient state" comment. Confirmed:GardenEditorPage.tsx's parallelclear()(lines 86-94) does callsetRailTab(null), butresetTransient(called fromPublicGardenPage.tsx:28) does not.web/src/editor/HistoryPanel.tsx:118—relativeTimeis a pure, dependency-free helper exported from a component file rather thanweb/src/lib/, breaking the project's established lib+test convention (confirmed via DESIGN.md:109 and grep — it's used only withinHistoryPanel.tsxand has no test).Full corrected review written to the plan file. This was a read-only verification task (no code changes), so there's nothing to implement — the plan file just holds the finalized review output.
⚡ Performance — Minor issues
Good, that confirms the surrounding "nothing else stood out" commentary matches the actual code (lazy render via
active.render()at line 87,render()only called for the active tab). The finding itself checks out.Everything in the draft is verified against the actual source:
useRevertChangeSet(history.ts:97-111) unconditionally invalidateshistoryKey(gardenId)on both success and 409-with-partial-apply,useGardenHistoryis confirmed as an offset-paginateduseInfiniteQuery(history.ts:68-83), and@tanstack/react-queryis pinned at^5.62.0(web/package.json:19), whose documented default behavior on invalidating an active infinite query is to sequentially refetch every already-fetched page. SinceHistoryPanel/UndoButtononly mount while the History tab is active (EditorRail.tsx's lazyrender()), the query is active exactly when a user clicks undo, so the finding holds.VERDICT: Minor issues
web/src/lib/history.ts:97-111—useRevertChangeSet'srefresh()invalidateshistoryKey(gardenId)unconditionally on every undo (success or 409-with-partial-apply).useGardenHistory(history.ts:68-83) is auseInfiniteQuery(TanStack Query^5.62.0, confirmed inweb/package.json:19) using offset-based paging. TanStack Query's default behavior for an active infinite query is to refetch every already-loaded page sequentially on invalidation, not just the first page. Since the History tab (and thus the query) is only mounted while active —EditorRailrenders tabs lazily — a user who has clicked "Load older" a few times will trigger an N-request cascade on every subsequent undo click, just to redraw the same list plus one new entry. Cost grows with how far the user has paginated. A cheap fix: after a successful/partial revert, only refetch the first page (e.g.qc.setQueryDatato drop pages beyond the first, orhistory.refetch({ refetchPage: (_, index) => index === 0 })) rather than invalidating the whole infinite-query cache.🧯 Error handling & edge cases — Blocking issues found
Review — 🧯 Error handling & edge cases
VERDICT: Blocking issues found
describeUndoreports "Undone." even when nothing was actually revertedweb/src/lib/history.ts:207-211:This treats "zero conflicts" as synonymous with "fully applied," but the backend contract (
internal/api/history.go:28-30,63-77) explicitly allows a 200 response with{changeSet: null, conflicts: []}— the comment there says exactly this: "ChangeSet is null when nothing needed reverting", and the handler'scase cs == nil:branch returns 200 with that body.Traced how that's reached in
internal/service/revisions.go:462-466:Only
OpCreategets this silent no-op treatment. This is concretely reachable via the "Undo again" affordance this PR ships:HistoryEntry(web/src/editor/HistoryPanel.tsx:104-111) renders the undo button unconditionally whenevercanEdit && totalChanges(changeSet) > 0, withlabel={reverted ? 'Undo again' : 'Undo'}— it is not disabled or hidden for an already-reverted entry. Clicking it callsundo.undo(changeSet)(web/src/editor/UndoButton.tsx) on the original changeSet object, not the revert entry.RevertChangeSet(internal/service/revisions.go:281-331) has no guard ontarget.RevertedByIDanywhere in the function.So: create a bed → undo it (bed deleted) → click "Undo again" on the now-struck-through original entry →
RevertChangeSetis invoked again on the original create changeset → its inverse (a removal) finds the entity already gone → hits theOpCreate/ErrNotFoundno-op branch → no revisions recorded →commitScopereturns(nil, nil)sincesc.taken()is empty → handler returns 200 with{changeSet: null, conflicts: []}→describeUndohits the zero-conflict branch and returns "Undone.", even though nothing happened.The same early-return also means any mixed change set where one
createsub-revision silently no-ops while other revisions genuinely apply is reported as a plain "Undone." —appliedis never compared againsttarget's total in theconflicts.length === 0branch, so a partial-but-silently-short revert is indistinguishable from a full one.web/src/lib/history.test.ts's "is a plain success when nothing conflicted" case supplieschangeSet: changeSet({ id: 2 })(non-null) — thechangeSet: null, conflicts: []case is not exercised by any test.Fix suggestion: in
describeUndo, whenconflicts.length === 0, comparetotalChanges(result.changeSet ?? …)againsttotalChanges(target); ifresult.changeSetis null or under-counts relative totarget, report a "nothing to undo" / partial state instead of an unconditional "Undone."glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
I traced the undo flow end-to-end through the security lens and found no material gaps:
canEdit.RevertChangeSetcallsrequireGardenRole(..., roleEditor)(internal/service/revisions.go:286) andGardenHistoryrequiresroleViewer(internal/service/revisions.go:244). The frontendcanEditgate on theUndoButtonis purely cosmetic; a forged/incorrect value can't bypass the backend. Verified by reading both service methods.parseIDParamon:id,intQueryonlimit/offsetininternal/api/history.go:34-41), and the client builds paths fromnumber-typedgardenId/changeSetIdonly (web/src/lib/history.ts:71,94). No string interpolation into URLs or queries.changeSet.summary,actorName, and conflictnameflow into JSX as text children (HistoryPanel.tsx,UndoButton.tsx) or into template strings insidedescribeConflict/describeCounts— never viadangerouslySetInnerHTML. React escapes text content, so no stored-XSS via a crafted summary/name.historyPageSchema.parseandrevertResultSchema.parsevalidate shapes (history.ts:74,95); the 409 path usessafeParse(revertConflicts,history.ts:84) so a malformed conflict body degrades to a generic error rather than crashing or trusting raw data.credentials: 'same-origin'via the existingapiwrapper (api.ts:97), unchanged.One non-security note I confirmed is fine:
useGardenHistoryruns regardless ofcanEdit, but the backend's viewer-role check authorizes that, so a shared viewer legitimately seeing history is intended behavior — not a leak.No findings to report for this lens.
🎯 Correctness — Minor issues
Both findings verified against the actual code.
Verdict: Minor issues
web/src/pages/GardenEditorPage.tsx:118-126— The selection auto-switch effect's comment claims it "leaves History/Chat open if that's where you were" when you deselect, but it does not, after a selection round-trip. Trace: user is on the History tab (railTab === 'history', no selection). They select a bed →hasSelectiontrue → effect setsrailTab = 'inspector'(line 124). They deselect →hasSelectionfalse, anduseEditorStore.getState().railTab === 'inspector'→ effect setsrailTab = null(line 125). The rail closes; it does not return to History. The store only keeps a singlerailTab(nopreviousTab), and the deselect branch unconditionally clears tonullwhenever the current tab is'inspector'. So the "leave History open" guarantee only holds if you never selected anything while there; the moment you select, History is dropped on deselect. This contradicts the code's own comment (lines 119-121) and the PR description. Fix: remember the pre-inspector tab and restore to it (e.g.'history') on deselect rather thannull, or track apreviousTab.web/src/lib/history.ts:207-217(unverified edge, not blocking) —describeUndousestotalChanges(target)as the denominator andtotalChanges(result.changeSet)as the numerator. This is correct as long as the revert's counts map to the target's revisions. The one fragile case: a target whosecountsarray is empty/zero while conflicts are non-empty would render "0 of 0 changes undone — …" aspartialrather thanerror, because theapplied === 0branch (line 213) only triggers whenresult.changeSetisnull(line 212). Whether a non-nullresult.changeSetcan ever carry zero counts alongside conflicts depends on server-side counts population, which I could not confirm in this pass. Low-confidence.🧹 Code cleanliness & maintainability — Minor issues
All findings verified. Now confirming the editor page (GardenEditorPage) does clear railTab in its own unmount/mount effect, but
resetTransient(used by PublicGardenPage) does not — confirming finding 1.Let me verify finding 2's claim about the
useUndooutcomes growth and finding 3/4 details are all confirmed from what I've read.All four findings are confirmed against the actual code:
resetTransientomitsrailTab— Confirmed.store.ts:97-106resets all other transient fields but notrailTab.PublicGardenPage.tsx:28callsresetTransient()on mount, so a stalerailTabsurvives. (Note:GardenEditorPage.tsx:93does clearrailTabin its own separate effect, so only the public page / theresetTransientcontract is affected — still a real inconsistency.)useUndooutcomes grow unbounded — Confirmed.history.ts:169holdsoutcomesinuseState, eachundoadds an entry keyed by id with no eviction (history.ts:172,175,179). The doc comment at line 165 even acknowledges "a single instance serves a whole list."totalChangesrecomputed per entry — Confirmed.HistoryPanel.tsx:70callsdescribeCounts(which filtersn > 0), then line 104 separately callstotalChanges(changeSet)(a fullreduceovercs.counts,history.ts:115). Both walkcounts.EditorRailbadge condition — Confirmed.EditorRail.tsx:69(the draft said 62; the actual badge check is at line 69):tab.badge != null && tab.badge !== false && tab.badge !== 0.Verdict: Minor issues
web/src/editor/store.ts:97—resetTransientdoesn't clear the newrailTabstate. Every other transient field (selectedId,selectedPlantingId,focusedObjectId,armedPlant,armedKind,liveObject,livePlanting) is reset inresetTransient, butrailTabwas left out.PublicGardenPage.tsx:28callsresetTransient()on mount to enter a garden view fresh, so arailTableft from a previous garden (e.g. a stale'inspector'/'history') survives the reset. (GardenEditorPage.tsx:93clearsrailTabin its own mount/unmount effect, so the editor page itself isn't affected — but the sharedresetTransientcontract is now inconsistent.) Fix: addrailTab: nullto theset({...})inresetTransient. Severity medium, high confidence.web/src/lib/history.ts:169—useUndo'soutcomesrecord grows unbounded. Eachundoadds an entry keyed by change-set id (history.ts:172,175,179) and never evicts one. The hook stays mounted across the whole paged history list (and, per the #57 inline-undo sharing noted in the file's own doc comment at line 165, can outlive a single list view), sooutcomesgrows with every undo action. Bounded in practice by user actions, so minor; worth a comment or scoping to visible entries. Severity small, medium confidence.web/src/editor/HistoryPanel.tsx:104—totalChangesrecomputed per entry despitedescribeCountsalready walking the same counts.HistoryEntrycomputescounts = describeCounts(changeSet)at line 70 (which filtersn > 0), then the undo-button guard at line 104 callstotalChanges(changeSet)— a second fullreduceovercs.counts(history.ts:115). Both helpers are already imported; a single-pass "has any change" check would suffice. Minor churn, easy to mistake for doing different things. Severity small, high confidence.web/src/editor/EditorRail.tsx:69—badgerender condition stacks three ad-hoc truthiness checks.tab.badge != null && tab.badge !== false && tab.badge !== 0(line 69) is three stacked checks to express "a count, or a plain marker, never shown for zero/false." A named helper (e.g.hasBadge) or normalizing thebadgetype tonumber | true | undefinedat theRailTabinterface would make the intent legible at the call site. Severity trivial, high confidence.⚡ Performance — Minor issues
All three findings are confirmed against the actual code:
history.ts:169confirmsoutcomesis aRecord<number, UndoOutcome>that only accumulates viasetOutcomes; there is no eviction logic anywhere.EditorRail.tsxrenders onlyactive.render();HistoryPanelmountsuseUndo(line 18), so tab-switch unmounts discard the hook's state.HistoryPanel.tsx:19recomputessetsviaflatMapevery render, andHistoryEntrycallsdescribeCounts/totalChangeswith no memoization.The draft review is accurate. Outputting the corrected review unchanged.
Verdict: Minor issues
useUndooutcomes state grows unbounded and is never cleared (web/src/lib/history.ts:169-187).outcomesis aRecord<number, UndoOutcome>keyed by change-set id that only ever accumulates — there is no eviction when entries scroll out of the visible list, no TTL, and the history itself is paged withflatMapover all loaded pages. After an undo on an older entry, that outcome lingers in component state for the life of the mountedHistoryPanel. Impact is small (history lists are short and ids are sparse integers), but it is a genuine unbounded-growth leak — a long-lived editor session that loads many pages and undoes several entries keeps every outcome forever. Suggested fix: clearoutcomes[id](or cap the record) when the change set is no longer inhistory.data, or simply store the outcome keyed to the currently visible set and drop stale keys on history refetch.useUndois instantiated perHistoryPanelmount, andHistoryPanelis rendered lazily inside the rail tab (web/src/editor/HistoryPanel.tsx:18,web/src/editor/EditorRail.tsx:71). BecauseEditorRailonly rendersactive.render()for the active tab, switching away from the History tab unmountsHistoryPanel, and itsuseUndostate (including pendingoutcome) is discarded — switching back remounts and re-fetches. That is acceptable (it's the documented lazy-render contract), but it means a pending "Undoing…" outcome is lost on tab switch. Verified:EditorRail.tsx:71renders onlyactive.render(), anduseUndo's state lives in the hook withinHistoryPanel. Not a perf regression, just noting the in-flight outcome has no persistence across tab toggles; if persistence matters for #57's inline undo, this hook will need to lift the outcomes store out of the panel.HistoryPanelre-runsflatMapover all loaded pages on every render (web/src/editor/HistoryPanel.tsx:19).setsis recomputed each render fromhistory.data?.pages, and each entry maps to aHistoryEntrythat callsdescribeCounts/totalChanges(reduce over counts). With offset paging at 30/page and a typical user scrolling a page or two this is negligible; butdescribeCountsandtotalChangesare re-derived per render per entry with no memoization, and the parent re-renders on everyuseUndostate change (eachsetOutcomestriggers a re-render of the whole list). For a long list this is O(n) work on each undo click rather than O(1) for the one changed row. Suggested fix: memoizesetswithuseMemoand/or memoizeHistoryEntryso an undo outcome update only re-renders the affected entry.🧯 Error handling & edge cases — Minor issues
Both findings verify against the actual source.
revertConflicts(history.ts:86-90) returnsnullboth when the status isn't 409 and whensafeParsefails on a 409 body, soonError'sif (revertConflicts(err)) refresh()(line 108) genuinely skips the refresh on an unparseable 409. AndmutationFn'srevertResultSchema.parse(...)(line 105) throws aZodErroron a 2xx with an unexpected shape, which rejects the mutation beforeonSuccess: refresh(line 106) runs; the per-callonErrorinuseUndo(lines 177-185) then getsrevertConflicts(ZodError) === nulland shows the generic error with no invalidation. No globalMutationCache/onErrorinqueryClient.tscompensates. Both survive.Verdict: Minor issues
web/src/lib/history.ts:107-108— a 409 whose body fails to parse skips the refresh, even though partial work likely applied. The top-level mutation'sonErroronly callsrefresh()whenrevertConflicts(err)is truthy, i.e. only when the 409 body successfully parses againstrevertResultSchema. But a 409 is by this PR's own design the "part of it applied" path. If the server returns a 409 with a body that omits thechangeSetkey (the schema requires it; there's no.default) or otherwise drifts,safeParsefails →revertConflictsreturnsnull→ noinvalidateQueries. The user then sees the generic "That couldn't be undone." (fromuseUndo's per-callonError) while the canvas/history stay stale despite a real partial write. The fix is to gate the refresh on the status, not the parse:if (err instanceof ApiError && err.status === 409) refresh(). Refreshing is the safe thing to do precisely when you can't phrase what happened — it re-fetches the truth. (Verified by readingrevertConflictsat lines 86-90 and the mutation at 103-110.)web/src/lib/history.ts:104-105— a successful revert whose response fails schema validation is reported as a failure with no refresh.mutationFnrunsrevertResultSchema.parse(...)on the 2xx body. If the server returns a 2xx with an unexpected shape,.parsethrows aZodError, the mutation rejects,onSuccess: refreshnever runs, anduseUndo's per-callonErrorproduces{ tone: 'error', message: errorMessage(err, …) }(sincerevertConflicts(ZodError)isnull). So a revert that did succeed server-side leaves the editor canvas and history list out of sync and shows a misleading failure. Lower-likelihood (schema is co-located with the contract), but it's a genuine unhappy path introduced by this diff. Suggested fix: refresh in afinally-style path, or catch the parse failure and still invalidate. (Verified by readingmutationFn/onSuccessat 103-106.)Both are the same root shape: the cache-refresh that keeps the canvas honest is conditioned on being able to describe the outcome, when the case where you can't describe it is exactly when you most need to re-fetch the truth.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Security review of the history panel + undo changes:
GET /gardens/:id/historyandPOST /change-sets/:id/revert) sit behindrequireAuth()(internal/api/api.go:78,117).GardenHistorycorrectly enforcesroleViewerandRevertChangeSetenforcesroleEditor(internal/service/revisions.go:244,286).parseIDParam(positive int64 validation, rejects< 1) and query params throughintQuery(non-negative integer clamping) (internal/api/errors.go:74-95).sourceparameter is hardcoded todomain.SourceUIat the handler level before reaching the service layer.changeSet.summary,changeSet.actorName,conflict.name) are rendered via React JSX ({…}), which auto-escapes HTML. ThedescribeConflicthelper returns plain strings that are likewise JSX-escaped when displayed inOutcomeNote.RevertChangeSetfetches the target change set by ID, then validates the actor's role against the owning garden resolved from that record — not from user input — preventing cross-garden revert attempts.Nothing in the security lane warrants a finding.
🎯 Correctness — Minor issues
Verdict: Minor issues
web/src/pages/GardenEditorPage.tsx:407—EditorRail'sonCloseunconditionally callsselect(null)andselectPlanting(null). Closing the rail from the History tab (where selection is irrelevant) still clears the user's canvas selection. A user viewing history for the currently selected object loses that selection when they close the panel, forcing them to re-select before they can keep editing. The close handler should only clear selection when the active tab isinspector, or the rail should close without touching canvas state at all.web/src/lib/history.ts:169+web/src/editor/EditorRail.tsx:85—useUndostores per-change-set outcomes in localuseState, butEditorRailmounts only the active tab (active.render()). If a user clicks Undo, switches to the Inspector tab while the mutation is in flight, and later returns to History,HistoryPanelremounts with a freshuseUndoinstance. The pending/success/error outcome is lost, so the undo button appears clickable again even though the revert mutation may still be running. The user can then accidentally fire a second revert for the same change set. Either the outcomes (or the mutation itself) need to outlive the tab mount, e.g. by hoisting them into the existing Zustand store or by givinguseMutationa stable key so remounts reconnect to in-flight work.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
Minor issues
web/src/editor/store.ts:97—railTabmissing fromresetTransientresetTransientis documented as clearing "all transient (non-persisted) editor state at once". The newrailTabstate is transient by design (it closes when nothing needs it and is not persisted), yet it is not included inresetTransient. This leaks rail state whenresetTransientis called from outsideGardenEditorPage— e.g.PublicGardenPagerelies on it to clear stale editor state when a logged-in user navigates to a public view. WhileGardenEditorPagehas its own mount-time cleanup that manually callssetRailTab(null), duplicating the clear breaks the abstraction and means future call-sites ofresetTransientwill silently retain the rail tab.Fix: Add
railTab: nullto the object insideresetTransient.⚡ Performance — Minor issues
Minor issues
web/src/lib/history.ts:169-193— Unboundedoutcomesstate growth inuseUndo.The
outcomesrecord is append-only: every undo adds a key and is never evicted. EachsetOutcomesspreads the entire object ({ ...prev, [cs.id]: … }), so after n undos the cumulative work is O(n²). In a long session on the History tab this leaks memory and slows successive undos.Fix: Cap the map (e.g., keep only the most recent 20 outcomes) or store ephemeral per-ID state in a ref keyed by change-set ID rather than React state.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/lib/history.ts:27—changeCountSchemausesz.number()without.int()or.nonnegative(), allowing any numeric value (negative, fractional,NaN,Infinity) from a misbehaving server to pass validation.totalChangessums these blindly, so a negative orNaNcount causestotalChanges(changeSet) > 0to evaluate to false and silently hides the Undo button, whiledescribeCountswould render strings like"2.5 plantings added"for fractional values. The schema should constrain this with.int().nonnegative().Minor issues
web/src/editor/HistoryPanel.tsx:128—relativeTimereturns"just now"for any timestamp in the future (e.g. server clock skew), because the negativesecondsvalue falls through the first< 60check. Future dates should be handled explicitly rather than misreporting them as the recent past.opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I have enough to assess the security lens. Let me verify the rendering paths for server-provided strings and the authz on the endpoints.
I traced through the relevant code:
Authn/authz on the new endpoints (verified):
GET /gardens/:id/history→h.getGardenHistory→svc.GardenHistory→requireGardenRole(..., roleViewer)(internal/service/revisions.go:244). Existence masked for non-participants via the standardErrNotFoundpath.POST /change-sets/:id/revert→h.revertChangeSet→svc.RevertChangeSet→ loads the change set, thenrequireGardenRole(target.GardenID, roleEditor)(internal/service/revisions.go:281-288). No IDOR: a caller can't revert another garden's change set, and a viewer can't revert at all. Both branches (revisions_test.go:369-376) cover the stranger/viewer cases.h.requireAuth()(internal/api/api.go:78, 117).Client-side gating (verified):
HistoryPanelonly renders theUndoButtonwhencanEdit && totalChanges(changeSet) > 0.canEditis the same owner/editor computation used for every other mutation inGardenEditorPage.tsx:74. This is defense-in-depth only — the service rejects regardless — but it's correct.Injection: All path components are numeric.
gardenIdisNumber(gardenId)from the route param (GardenEditorPage.tsx:30);changeSetIdiscs.idfrom a zod-validatedChangeSet(history.ts:31). Query params (limit,offset) go throughURLSearchParamswith number values. No string interpolation into URLs from user input.Untrusted data rendering (verified): server-provided strings that reach the DOM —
actorName,summary, conflictname, and the assembledoutcome.message— are all rendered as React text children ({changeSet.actorName},{changeSet.summary},{outcome.message}inUndoButton.tsx/HistoryPanel.tsx). React escapes these; nodangerouslySetInnerHTMLanywhere in the new files.describeConflictbuilds a string by interpolatingc.nameinto a template literal, but the result is still rendered as text.Schema validation:
history.tsvalidates every response with zod (historyPageSchema,revertResultSchema,changeSetSchema) before use, including the 409 body viarevertConflicts→revertResultSchema.safeParse. A malformed 409 body fails safe (treated as a plain error, not a fake "partial success").Secrets / leakage: No credentials, tokens, or PII beyond the actor's display name (already part of the existing history surface).
agentRunIdis parsed but not rendered.Defaults:
useGardenHistorydefaultsenabled = true;gardenIdis always a real value here. No insecure defaults.No material issues found through the security lens.
🎯 Correctness — Minor issues
I've verified both findings against the actual source. Let me confirm the key claims.
Let me verify the service-level RevertChangeSet to confirm the "zero counts / 201" path the first finding claims.
I now have enough to make a final assessment. Let me verify both findings:
Finding 1 (
web/src/lib/history.ts:209-211): The draft claimsdescribeUndoreturns"Undone."in ok tone when nothing was actually undone. Confirmed:describeUndoat line 209 hasif (result.conflicts.length === 0) return { tone: 'ok', message: 'Undone.' }. The backendhistory.go:73-76returns 200 withchangeSet: nilwhen "every revision resolved to a no-op." In that caseconflictsis empty (len 0), so the UI reports "Undone." in ok tone — even though nothing was reverted. This is the "bare success hides the one that didn't" failure mode. The draft's secondary claim about "201 with zero counts" is weaker: looking atcommitScope(revisions.go:152-155), a scope with no revisions returnsnil— so the 201 path with a non-nilcswould only happen when revisions actually wrote. The 201-with-zero-counts path doesn't really exist per the service code. But the 200 with nil changeSet path is real and confirmed. The finding survives on that primary path.Finding 2 (
web/src/lib/history.ts:107-109): Confirmed:onErroronly callsrefresh()whenrevertConflicts(err)parses successfully (returns non-null). If a 409 body fails to parse,revertConflictsreturnsnull, and no refresh fires — even though a 409 from this endpoint means "part may have applied." The surrounding code is explicitly careful about this (the comment on line 108 says "a partial revert still changed things"). This is a real defensive gap.Both findings are confirmed against the actual source. The GardenEditorPage selection-tracking "finding" was already withdrawn by the original reviewer (it's documenting a non-issue), so it correctly does not appear in the findings array.
VERDICT: Minor issues
I verified the phrasing logic (
describeUndo/describeConflict/describeCounts) against the backendRevertChangeSetcontract ininternal/api/history.goandinternal/service/revisions.go, and against the test expectations. The status-code → tone mapping (201 clean / 409 partial / 200 no-op) and the "2 of 3 changes undone — …" derivation all check out, andrevertConflictscorrectly recovers the partial result from the 409 body. Two real correctness gaps in the honest-reporting logic this PR centers on:web/src/lib/history.ts:209-211—describeUndoreports"Undone."(ok tone) when the server actually changed nothing. The early returnif (result.conflicts.length === 0) return { tone: 'ok', message: 'Undone.' }fires without checking whetherresult.changeSetis null. The backend's "nothing happened" path —internal/api/history.go:73-76returns 200 withchangeSet: nullwhen every revision resolved to a no-op (already undone by hand) — hasconflictsempty, so it lands here. The user clicks Undo, the server does nothing, and the UI answers"Undone."in the muted/ok tone: the exact "bare success would hide the one that didn't" failure mode the PR says it's eliminating, for the zero-applied case. (The draft also floated a "201 with zero counts" path; that one does not hold —commitScopeatinternal/service/revisions.go:152-155returnsnilfor a scope with no revisions, so a 201 always carries a real change set. The 200/null path is the real one.) Verified by readingdescribeUndo(history.ts:207-218) and tracinginternal/api/history.go:68-79plusinternal/service/revisions.go:151-164. Suggested fix: drop the early return whenconflicts.length === 0and let the existingapplied === 0branch handle it — e.g.if (applied === 0) return { tone: 'error', message: 'Nothing was undone — it was already back to this state.' }before the conflicts check, or add a distinct neutral tone.web/src/lib/history.ts:107-109— a malformed 409 body leaves the canvas/history stale.useRevertChangeSet.onErroronly callsrefresh()whenrevertConflicts(err)parses successfully. If a 409 arrives with an unparseable body (proxy rewrite, version skew, schema drift),revertConflictsreturnsnull, no refresh fires, but the backend may have applied part of the revert — so the canvas and history list stay out of sync while the user is shown a generic error. Low severity (defensive edge), but it's the same class of "partial revert changed things, refresh anyway" the surrounding code is careful about (the inline comment on line 108 says "a partial revert still changed things"). Suggested fix: refresh on any 409 regardless of body parse, since a 409 from this endpoint specifically means "something may have applied."🧹 Code cleanliness & maintainability — Minor issues
I'll verify each finding against the actual code.
Now let me verify the specific findings. First, find the #57/#53 references in DESIGN.md and PR description, and check
isPendingusage.Let me verify the GardenEditorPage comment about #57/#53 to confirm the contradictory references finding.
I've confirmed all findings. Let me verify the findings:
#57 contradictory references - Confirmed.
UndoButton.tsx:7andhistory.ts:162say "#57 = inline undo".GardenEditorPage.tsx:289says "chat (#57)". AlsoDESIGN.md(per diff) describes #57 as chat. The codebase itself contradicts what #57 means. Keep.isPending dead API - Confirmed.
history.ts:192returnsisPending: revert.isPending. The onlyisPendingreferences in callers arehistory.isPending(the query, not undo) andoutcome?.tone === 'pending'. No caller usesundo.isPending. Keep.useRevertChangeSet/revertConflicts unnecessarily exported - Confirmed. Grep shows both only used within history.ts (lines 108, 168, 178). No external import. Keep.
HistoryPanel redundant heading - Confirmed.
<h2>History</h2>at line 23, while the tab is labeled "History" (from GardenEditorPage railTabs). For a screen reader, the tab + heading both announce "History". Keep as trivial.relativeTime in component file - Confirmed.
relativeTimeis exported fromHistoryPanel.tsx(a.tsxfile), pure function with no JSX. Other pure helpers live inweb/src/lib/. Keep as trivial.All five findings survive verification.
VERDICT: Minor issues
web/src/lib/history.ts:161-162andweb/src/editor/UndoButton.tsx:5— stale/contradictory issue references. The comment inhistory.tssays the shared undo is "shared by the history list and (per #49) the agent turn's inline Undo in #57", andUndoButton's doc comment says "the agent turn's inline undo (#57)". ButGardenEditorPage.tsx:289(and the PR'sDESIGN.mdchange) describe #57 as the chat panel ("the journal (#53) and chat (#57) panels too"). So the code comments point at #57 meaning one thing and the rest of the codebase points at #57 meaning another. Whichever is wrong, a reader chasing the reference will be misled. Fix: correct the issue numbers in the comments to match whatever #57 actually is, or drop the numeric references and describe the consumer by name.web/src/lib/history.ts:192—isPendingis dead public API.useUndoreturnsisPending: revert.isPending, but no caller uses it.UndoButtonkeys its disabled/loading state offoutcome?.tone === 'pending'(verified by greppingisPending/outcomeFor/.undo(usages acrossweb/src; every otherisPendingreference is on a different query/mutation, not the undo hook). The field is exported surface that nothing reads. Either wire it up (e.g. disable all undo buttons while any revert is in flight) or drop it from the return.web/src/lib/history.ts:97and:86— unnecessarily exported.useRevertChangeSetandrevertConflictsare bothexported, but grep confirms each is only called from withinhistory.tsitself (useRevertChangeSetat line 168,revertConflictsat lines 108 and 178; no test or component imports either). They don't need to be part of the module's public surface. Dropping theexportkeeps the API surface to whatuseUndo/describeUndo/the schemas actually offer. Small, but it's exactly the kind of leaky surface that accretes over time.web/src/editor/HistoryPanel.tsx:23— redundant heading. The panel renders<h2>History</h2>while the tab button that opened it is already labeled "History" (theEditorRailtab inGardenEditorPage.tsxwitharia-current="page"marking the active tab). For a screen-reader user the panel's name is announced twice. Either drop the<h2>(the tab is the label) or give the heading something the tab doesn't say (e.g. the garden name). Trivial.web/src/editor/HistoryPanel.tsx:118—relativeTimeis a pure util living in a component file. It's a stateless, testable string formatter exported from a.tsxcomponent module (no JSX in the function). The rest of the codebase puts pure helpers inweb/src/lib/(e.g.lib/geometry.ts,lib/units.ts);lib/history.tsalready lives next door and would be its natural home (it has no JSX and is the file that owns theChangeSetshape). Move it so the component file is just the component. Trivial, but it's the pattern break the CLAUDE.md "conventions that bite" section is about.⚡ Performance — No material issues found
No material issues found
Through the performance lens, I verified the new code paths and found nothing material:
useGardenHistory(web/src/lib/history.ts:68) — singleuseInfiniteQuery, offset-paged atPAGE_SIZE = 30.HistoryPanelis only mounted when the History tab is the active rail tab (EditorRailcallsactive.render()exclusively, line 84 ofEditorRail.tsx), so the query doesn't fire in the background while the inspector is open. No N+1, no unbounded fetch.useUndooutcomes state (history.ts:169) — keyed by change set id, one entry added per user undo click. Bounded by user actions, and the whole map is discarded whenHistoryPanelunmounts on tab switch. Not a leak.HistoryPanelflatten (HistoryPanel.tsx:19) —pages.flatMapruns per render; for a few pages of 30 entries this is trivial and not in a hot loop (undo is a user action, not a per-frame path).railTabsarray (GardenEditorPage.tsx:291) — recreated per render with new closures, but only the active tab'srender()is invoked, so inactive tabs (e.g. theInspector/PlopInspectorsubtree) are not built. This is actually a slight win over the old layout, which always mounted the inspector when something was selected.history.ts:99-102) —refresh()invalidatesgardenFullKey+historyKey. Fires once per undo (user action), not in a drag/loop. Correct, not a regression.No quadratic behavior, no missing limits, no hot-path blocking.
🧯 Error handling & edge cases — Minor issues
All three findings confirmed against the actual code.
Verdict: Minor issues
web/src/editor/HistoryPanel.tsx:45— unhandled rejection onfetchNextPage. The "Load older" button doesonClick={() => void history.fetchNextPage()}. In TanStack Query v5 (repo pins@tanstack/react-query ^5.62.0),fetchNextPage()returns a promise that rejects when the page request fails (network error, 401, etc.).void-discarding it without.catch()leaves an unhandled promise rejection — visible in the console and, in some environments, surfaces as a runtime warning. Suggested fix:onClick={() => { history.fetchNextPage().catch(() => {}) }}. Confirmed the onlyfetchNextPagecaller in the repo, so no existing convention to match.web/src/editor/HistoryPanel.tsx:26— misleading error message when only the next page fails.history.isErrorbecomes true on afetchNextPagefailure too (TanStack setserror/isErrorfor any fetch, not just the initial one). When the user clicks "Load older" and that request fails, the panel renders<Alert>Could not load history.</Alert>above the already-loaded entries — which reads as "the whole history failed," even though the older entries are still on screen and retryable via the same button. Concrete impact: a transient network blip on paging makes it look like history is gone. Suggested fix: distinguish initial-load failure from next-page failure (e.g.history.isError && !history.datafor the alert; show a smaller inline retry message near "Load older" whenhistory.dataexists but the last fetch failed).web/src/lib/history.ts:177-186— a 409 with an unparseable body is reported as a plain failure even though part may have applied.revertConflictsusessafeParseand returnsnullif the 409 body doesn't matchrevertResultSchema. The hook then falls back to{ tone: 'error', message: "That couldn't be undone." }(history.ts:183). The whole premise of this code (per the file's header comment, lines 4-7) is that a 409 means "partly applied, report both." If the server ever returns a 409 with a body shape the client doesn't recognize, the user is told "couldn't be undone" while the canvas may have redrawn with real changes — exactly the lie the PR set out to avoid. This is a contract-coupling concern more than a likely bug (same team owns both sides), so confidence is medium. Suggested fix: on a 409 with an unparseable body, still refresh and report a generic "Something was undone, but details weren't available" partial-style message rather than a flat error.opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Security lens: the new history and revert endpoints are properly authenticated, authorized, and validated; the frontend renders user-controlled data safely; and the state-changing revert is behind the existing CSRF guard.
GET /gardens/:id/history—parseIDParamenforces positive int64;intQuerysanitizeslimit/offset;GardenHistoryclampslimitto [1, 100] and requiresroleViewerviarequireGardenRole(existence masked asErrNotFound).POST /change-sets/:id/revert—parseIDParamenforces positive int64;RevertChangeSetloads the change set then requiresroleEditoron its garden. The v1 group already appliescsrfGuard()(Origin-check), so the state-changing POST is CSRF-protected.changeSet.summary,actorName, conflict names, andrelativeTimeoutput are all rendered as React text nodes (nodangerouslySetInnerHTML, no template strings interpolated into HTML).canEditonly gates the undo button in the UI; the backend independently re-checks editor role, so bypassing the UI still fails authz.🎯 Correctness — Minor issues
Verdict: Minor issues
Two real correctness problems, both introduced by this PR:
web/src/pages/GardenEditorPage.tsx:123-126— selection-change id does not auto-switch to Inspector.The effect that auto-switches the rail tab depends only on the
hasSelectionboolean, not on the actual selection ids:Because
hasSelectionstaystruewhen the user clicks from object A to object B, the effect never fires and the rail stays on whatever tab it was on (e.g. History). This contradicts the stated design constraint: "Selecting an object still lands you in the inspector without a click." The fix is to putselectedIdandselectedPlantingIdin the dependency array so any change of selection re-routes to the inspector.web/src/editor/HistoryPanel.tsx:118-131—relativeTimecan mislabel same-day events as "yesterday".The day computation uses
Math.roundat every boundary:At the day boundary this over-rounds: an event at 00:00 today viewed at 23:30 today is 23.5 hours ago →
hours = Math.round(1410/60) = 24→days = Math.round(24/24) = 1→ "yesterday", even though the event occurred today. UsingMath.floorfor the day step (or comparing calendar dates) would fix the semantic error.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
relativeTimeis exported from a component file instead oflib/HistoryPanel.tsx:118exportsrelativeTime, a pure date-formatting utility with no React or UI dependencies. If the journal (#53) or chat (#57) later wants the same phrasing, they would have to import it from a panel component, breaking thelib/vs.components/boundary the rest of the codebase observes (e.g.,geometry.ts,units.ts).Fix: move
relativeTimeintoweb/src/lib/history.ts(or a newtime.ts) and import it inHistoryPanel.useUndoexposesisPendingthat no caller consumeshistory.ts:189–193returnsisPending: revert.isPending, but neitherHistoryPanelnorUndoButtonreads it; each button tracks its own pending state viaoutcomeFor(id)?.tone === 'pending'. The exported flag is therefore dead weight that misleadingly suggests "any revert is pending" rather than per-item status.Fix: drop
isPendingfrom the return object, or replace it with a per-id predicate such asisPendingFor(id)if a future consumer needs it.⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/editor/HistoryPanel.tsx:45— Pagination failure is silently swallowed.history.fetchNextPage()is fire-and-forget viavoid. If the network fails while loading older history, the button stops spinning and no error is shown to the user. TanStack Query exposesisFetchNextPageError/fetchNextPageErrorfor this; the component ignores them. Fix: Wrap the call, catch errors, and surface them in the panel (e.g., a small inline alert or a toast).web/src/lib/history.ts:21—ninchangeCountSchemaaccepts negative numbers.z.number()allows negatives. A server bug returningn: -1would silently distorttotalChanges(possibly dropping below zero and hiding the undo button) whiledescribeCountsfilters negatives out entirely. Fix: Tighten toz.number().int().nonnegative()so a bad payload fails loudly at parse time instead of producing confusing UI.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
Fixed in
9dc4d3a. Two of these were real design breaks, not polish.The store already had a reset, and my new state didn't join it
resetTransient()is the single list of ephemeral editor state, andrailTabwasn't in it — soPublicGardenPagecleared everything except the rail. The fix isn't to addrailTabin two places: the editor page now callsresetTransient()instead of maintaining its own parallel list, which is exactly what let the two drift. Good catch — I'd added state next to a mechanism designed to prevent this and walked straight past it.The auto-switch broke the constraint the whole design rests on
It keyed off a
hasSelectionboolean, so it fired on the transition into having a selection and never again. Select a bed → switch to History → select a different bed: the boolean never changed, so the inspector never came forward. That's the one thing #49 said the rail must never do. Now keyed off the selected ids.Related, from
kimi: closing the rail cleared the canvas selection no matter which tab you were on, so dismissing History deselected your bed. Only the inspector is about the selection, so only closing it deselects.The rest
describeUndosaid "Undone." for a complete no-op (200 + null change set — reachable by undoing a creation whose object is already gone). That reports work that didn't happen. It now saysNothing left to undo — this was already reversed., with a test.relativeTimerounded, so 18 hours read as "yesterday" and 90 minutes as "2h ago" — rounding up into the next unit reads as a bigger gap than actually elapsed. Floors throughout. Clock skew putting a just-written entry slightly in the future now reads "just now" rather than falling through a negative.changeCountSchematook any number. Counts come fromCOUNT(*); constraining to non-negative integers makes bad data fail loudly rather than quietly hiding an Undo button (totalChangesgates it) or rendering "1.5 beds changed".useUndo's unusedisPending— the per-change-set outcome already carries it, and per-row is the right granularity anyway.Declined, with reasoning
useUndooutcomes are lost when the History tab unmounts. They're transient feedback about an action you just took, and the canvas and the list have both already updated to show the result. Persisting them past a tab switch would mean stale messages about changes now several edits old.outcomesgrowth. One entry per change set you personally undo in one session, cleared on navigation.65 tests,
tsc --noEmit,npm run buildgreen.