History panel + undo in the editor (#49) #63

Merged
steve merged 2 commits from feat/history-panel into main 2026-07-21 05:23:25 +00:00
9 changed files with 725 additions and 41 deletions
+2 -1
View File
@@ -105,7 +105,8 @@ React 19 + TypeScript + Vite + Tailwind 4 (`@tailwindcss/vite`), `@tanstack/reac
- **Routes:** `/login`, `/register`, `/gardens` (list), `/gardens/:id` (editor, `?focus=objectId`), `/plants` (catalog). Auth guard on the router root via `/auth/me`.
- **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.
Review

🟡 VERDICT: Minor issues

maintainability · flagged by 1 model

  • web/src/editor/store.ts:97-106resetTransient 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:118relativeTime 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>
## Roadmap
+88
View File
@@ -0,0 +1,88 @@
import type { ReactNode } from 'react'
import { cn } from '@/lib/cn'
/**
* The editor's side rail, and the answer to "four things want one rail".
*
* 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 one rail — which keeps the canvas one width instead of a different
* width per panel, and means adding the journal or chat is adding a tab.
*
* Two constraints shaped it:
*
* - Selecting an object must land you in the inspector with no extra click.
* The editor watches the selection and switches to that tab itself, so the
* rail never becomes a thing you have to operate before you can edit.
* - The canvas has to stay worth watching while the agent edits it, so the
* rail is a fixed 20rem column and closes completely when nothing needs it.
*
* On a phone the same tabs render in a bottom sheet, which is where the
* inspector already lived.
*/
export interface RailTab {
id: string
label: string
/** Rendered lazily: only the active tab's content is built. */
render: () => ReactNode
/** Shown as a dot on the tab — a count, or true for a plain marker. */
badge?: number | boolean
}
export function EditorRail({
tabs,
activeId,
onActivate,
onClose,
}: {
tabs: RailTab[]
activeId: string
onActivate: (id: string) => void
onClose: () => void
}) {
const active = tabs.find((t) => t.id === activeId) ?? tabs[0]
if (!active) return null
return (
<div
className={cn(
// Phone: a bottom sheet over the canvas. Desktop: a column beside it.
'fixed inset-x-0 bottom-0 z-30 flex max-h-[70vh] flex-col rounded-t-xl border-t border-border bg-surface shadow-lg',
'md:static md:max-h-none md:w-80 md:shrink-0 md:rounded-xl md:border md:shadow-sm',
)}
>
<div className="flex items-center gap-1 border-b border-border px-2 py-1.5">
{tabs.map((tab) => (
<button
key={tab.id}
type="button"
onClick={() => onActivate(tab.id)}
aria-current={tab.id === active.id ? 'page' : undefined}
className={cn(
'flex items-center gap-1.5 rounded-md px-2.5 py-1 text-sm font-medium outline-none transition-colors',
'focus-visible:ring-2 focus-visible:ring-accent/40',
tab.id === active.id ? 'bg-border/60 text-fg' : 'text-muted hover:text-fg',
)}
>
{tab.label}
{tab.badge != null && tab.badge !== false && tab.badge !== 0 && (
Review

badge render condition stacks three ad-hoc truthiness checks instead of a named helper

maintainability · flagged by 1 model

  • web/src/editor/EditorRail.tsx:69badge 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>
<span className="rounded-full bg-accent/20 px-1.5 text-[10px] font-semibold text-accent-strong">
{tab.badge === true ? '•' : tab.badge}
</span>
)}
</button>
))}
<button
type="button"
onClick={onClose}
aria-label="Close panel"
className="ml-auto rounded px-1.5 text-sm text-muted outline-none hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40"
>
</button>
</div>
<div className="min-h-0 flex-1 overflow-y-auto p-4">{active.render()}</div>
</div>
)
}
+146
View File
@@ -0,0 +1,146 @@
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { errorMessage } from '@/lib/api'
import { describeCounts, totalChanges, useGardenHistory, useUndo, type ChangeSet } from '@/lib/history'
import { cn } from '@/lib/cn'
import { UndoButton } from './UndoButton'
/**
* The garden's change history, newest first, with an undo on each entry.
*
* A reverted entry stays in the list, 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 adding to it, and would leave no way to undo the
* undo.
*/
export function HistoryPanel({ gardenId, canEdit }: { gardenId: number; canEdit: boolean }) {
const history = useGardenHistory(gardenId)
const undo = useUndo(gardenId)
Review

🟡 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>
const sets = history.data?.pages.flatMap((p) => p.changeSets) ?? []
return (
<div className="flex flex-col gap-3">
<h2 className="text-sm font-semibold text-fg">History</h2>
Review

🟡 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>
{history.isPending && <p className="text-sm text-muted">Loading</p>}
{history.isError && sets.length === 0 && (
<Alert>{errorMessage(history.error, 'Could not load history.')}</Alert>
)}
{history.isSuccess && sets.length === 0 && (
<p className="text-sm text-muted">
Nothing yet. Every change you make here shows up in this list, and can be undone from it.
</p>
)}
<ol className="flex flex-col gap-2">
{sets.map((cs) => (
<HistoryEntry key={cs.id} changeSet={cs} canEdit={canEdit} undo={undo} />
))}
</ol>
{history.hasNextPage && (
<>
<Button
variant="ghost"
Outdated
Review

🟠 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>
className="text-sm"
disabled={history.isFetchingNextPage}
onClick={() => void history.fetchNextPage()}
>
{history.isFetchingNextPage ? 'Loading…' : 'Load older'}
</Button>
{/* isError stays true for a failed page even though earlier pages
loaded, so say so rather than leaving a button that did nothing. */}
{history.isError && sets.length > 0 && (
<p className="text-xs text-red-700 dark:text-red-400">
{errorMessage(history.error, "Couldn't load older entries.")}
</p>
)}
</>
)}
{/* Said plainly rather than discovered: deleting a garden bypasses this
list entirely, because the delete cascades below the layer that records
changes. Better to state the gap than to imply cover we don't have. */}
<p className="border-t border-border pt-2 text-xs text-muted">
Deleting a whole garden isn't covered here and can't be undone.
</p>
</div>
)
}
function HistoryEntry({
changeSet,
canEdit,
undo,
}: {
changeSet: ChangeSet
canEdit: boolean
undo: ReturnType<typeof useUndo>
}) {
const counts = describeCounts(changeSet)
const reverted = changeSet.revertedById != null
const isRevert = changeSet.revertsId != null
return (
<li
className={cn(
'rounded-lg border border-border px-2.5 py-2 text-sm',
reverted && 'opacity-60',
)}
>
<div className="flex items-start gap-2">
<div className="min-w-0 flex-1">
<p className={cn('text-fg', reverted && 'line-through decoration-muted')}>{changeSet.summary}</p>
<p className="mt-0.5 flex flex-wrap items-center gap-x-1.5 gap-y-1 text-xs text-muted">
{changeSet.source === 'agent' && (
<span className="rounded bg-accent/20 px-1 py-px font-medium uppercase tracking-wide text-accent-strong">
agent
</span>
)}
{isRevert && <span className="rounded bg-border/60 px-1 py-px font-medium">undo</span>}
<span>{changeSet.actorName}</span>
<span aria-hidden>·</span>
<time dateTime={changeSet.createdAt}>{relativeTime(changeSet.createdAt)}</time>
Review

🟡 totalChanges recomputed per entry when describeCounts already walked the same counts

maintainability · flagged by 1 model

  • web/src/editor/HistoryPanel.tsx:104totalChanges 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>
{counts && (
<>
<span aria-hidden>·</span>
<span>{counts}</span>
</>
)}
</p>
{reverted && <p className="mt-1 text-xs text-muted">Undone</p>}
</div>
{canEdit && totalChanges(changeSet) > 0 && (
<UndoButton
changeSet={changeSet}
undo={undo}
Review

🟡 VERDICT: Minor issues

correctness, maintainability · flagged by 3 models

  • web/src/editor/store.ts:97-106resetTransient 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:118relativeTime 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>
className="w-32 shrink-0"
label={reverted ? 'Undo again' : 'Undo'}
/>
)}
</div>
</li>
)
}
/** A compact "3m ago" / "yesterday" for a UTC timestamp. */
Review

🟡 relativeTime misreports future dates as 'just now'

error-handling · flagged by 1 model

  • web/src/editor/HistoryPanel.tsx:128relativeTime 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>
export function relativeTime(iso: string): string {
const then = Date.parse(iso)
if (Number.isNaN(then)) return iso
const seconds = (Date.now() - then) / 1000
// Clock skew between the server and this browser can put a just-written entry
// slightly in the future; that's "just now", not a negative duration.
if (seconds < 60) return 'just now'
// Floor throughout: 18 hours ago is "18h ago", not "yesterday". Rounding up
// into the next unit reads as a bigger gap than actually elapsed.
const minutes = Math.floor(seconds / 60)
if (minutes < 60) return `${minutes}m ago`
const hours = Math.floor(minutes / 60)
if (hours < 24) return `${hours}h ago`
const days = Math.floor(hours / 24)
if (days === 1) return 'yesterday'
if (days < 30) return `${days}d ago`
return new Date(then).toLocaleDateString()
}
+51
View File
@@ -0,0 +1,51 @@
import { Button } from '@/components/ui/Button'
import { cn } from '@/lib/cn'
import type { ChangeSet, UndoOutcome, useUndo } from '@/lib/history'
/**
* Undo, plus what happened. Paired with useUndo so the history list and the
* agent turn's inline undo (#57) behave identically — including the part that
* actually matters, which is reporting a partial result honestly rather than as
* a success or a failure.
*/
export function UndoButton({
changeSet,
undo,
className,
label = 'Undo',
}: {
changeSet: ChangeSet
undo: ReturnType<typeof useUndo>
className?: string
label?: string
}) {
const outcome = undo.outcomeFor(changeSet.id)
return (
<div className={cn('flex flex-col items-end gap-1', className)}>
<Button
variant="ghost"
className="px-2 py-1 text-xs"
disabled={outcome?.tone === 'pending'}
onClick={() => undo.undo(changeSet)}
>
{outcome?.tone === 'pending' ? 'Undoing…' : label}
</Button>
{outcome && outcome.tone !== 'pending' && <OutcomeNote outcome={outcome} />}
</div>
)
}
const TONE_CLASS: Record<Exclude<UndoOutcome['tone'], 'pending'>, string> = {
ok: 'text-muted',
partial: 'text-amber-700 dark:text-amber-400',
error: 'text-red-700 dark:text-red-400',
}
function OutcomeNote({ outcome }: { outcome: UndoOutcome }) {
if (outcome.tone === 'pending') return null
return (
<p role="status" className={cn('text-right text-xs', TONE_CLASS[outcome.tone])}>
{outcome.message}
</p>
)
}
+10
View File
@@ -26,6 +26,12 @@ interface EditorState {
focusedObjectId: number | null
setFocusedObject: (id: number | null) => void
// Which rail tab is showing, or null when the rail is closed. Selecting an
// object switches this to 'inspector' (see GardenEditorPage), so editing never
// starts with a click on the rail itself.
railTab: string | null
setRailTab: (tab: string | null) => void
// The plant armed for placing plops (set after the PlantPicker choice); stays
// armed for repeat-placement until cleared (Escape / done). null = not placing.
armedPlant: Plant | null
@@ -70,6 +76,9 @@ export const useEditorStore = create<EditorState>((set) => ({
focusedObjectId: null,
setFocusedObject: (id) => set({ focusedObjectId: id }),
railTab: null,
setRailTab: (tab) => set({ railTab: tab }),
armedPlant: null,
setArmedPlant: (p) => set({ armedPlant: p }),
@@ -94,5 +103,6 @@ export const useEditorStore = create<EditorState>((set) => ({
armedKind: null,
liveObject: null,
livePlanting: null,
railTab: null,
}),
}))
+115
View File
@@ -0,0 +1,115 @@
import { describe, expect, it } from 'vitest'
import { describeConflict, describeCounts, describeUndo, totalChanges, type ChangeSet } from './history'
function changeSet(over: Partial<ChangeSet> = {}): ChangeSet {
return {
id: 1,
gardenId: 1,
actorId: 1,
actorName: 'Steve',
source: 'ui',
summary: 'Did a thing',
counts: [],
createdAt: '2026-07-21T00:00:00Z',
...over,
}
}
describe('describeCounts', () => {
it('pluralizes per entity and names the operation', () => {
const cs = changeSet({
counts: [
{ entityType: 'planting', op: 'create', n: 3 },
{ entityType: 'object', op: 'update', n: 1 },
],
})
expect(describeCounts(cs)).toBe('3 plantings added, 1 bed changed')
})
it('says nothing rather than "0 changes"', () => {
expect(describeCounts(changeSet())).toBe('')
expect(describeCounts(changeSet({ counts: [{ entityType: 'object', op: 'create', n: 0 }] }))).toBe('')
})
})
describe('describeConflict', () => {
it('names the entity when it has a name', () => {
expect(describeConflict({ entityType: 'object', entityId: 4, reason: 'changed', name: 'North Bed' })).toBe(
'“North Bed” was edited since, so it was left alone',
)
})
it('falls back to the entity kind when it has no name', () => {
expect(describeConflict({ entityType: 'planting', entityId: 9, reason: 'missing' })).toBe(
'that planting no longer exists',
)
})
})
describe('describeUndo', () => {
const target = changeSet({ counts: [{ entityType: 'object', op: 'update', n: 3 }] })
it('is a plain success when nothing conflicted', () => {
const out = describeUndo(target, {
changeSet: changeSet({ id: 2, counts: [{ entityType: 'object', op: 'update', n: 3 }] }),
conflicts: [],
})
expect(out).toEqual({ tone: 'ok', message: 'Undone.' })
})
// The server answers 200 with a null change set when every revision was
// already a no-op — reachable by undoing a creation whose object is gone.
// Claiming "Undone." there reports work that didn't happen.
it("doesn't claim to have undone a no-op", () => {
const out = describeUndo(target, { changeSet: null, conflicts: [] })
expect(out.tone).toBe('ok')
expect(out.message).toBe('Nothing left to undo — this was already reversed.')
})
// The case the issue was specific about: a partial revert really did change
// things, so reporting a bare failure would be a lie about what applied.
it('says how much applied and what was skipped', () => {
const out = describeUndo(target, {
changeSet: changeSet({ id: 2, counts: [{ entityType: 'object', op: 'update', n: 2 }] }),
conflicts: [{ entityType: 'object', entityId: 7, reason: 'changed', name: 'North Bed' }],
})
expect(out.tone).toBe('partial')
expect(out.message).toBe('2 of 3 changes undone — “North Bed” was edited since, so it was left alone.')
})
it('is an error when nothing applied at all', () => {
const out = describeUndo(target, {
changeSet: null,
conflicts: [{ entityType: 'object', entityId: 7, reason: 'changed', name: 'North Bed' }],
})
expect(out.tone).toBe('error')
expect(out.message).toBe('Nothing was undone — “North Bed” was edited since, so it was left alone.')
})
it('lists every conflict, not just the first', () => {
const out = describeUndo(target, {
changeSet: changeSet({ id: 2, counts: [{ entityType: 'object', op: 'update', n: 1 }] }),
conflicts: [
{ entityType: 'object', entityId: 7, reason: 'changed', name: 'North Bed' },
{ entityType: 'planting', entityId: 8, reason: 'missing' },
],
})
expect(out.message).toContain('“North Bed”')
expect(out.message).toContain('that planting no longer exists')
})
})
describe('totalChanges', () => {
it('sums every tally', () => {
expect(
totalChanges(
changeSet({
counts: [
{ entityType: 'planting', op: 'create', n: 12 },
{ entityType: 'object', op: 'update', n: 1 },
],
}),
),
).toBe(13)
})
})
+226
View File
@@ -0,0 +1,226 @@
// Change history and undo (#49): zod-validated shapes for /gardens/:id/history
// and /change-sets/:id/revert, plus the hooks the editor's History tab uses.
//
// A revert is a real write that can partly succeed, so its mutation is unusual:
// a 409 is not a plain failure. It carries the change set that DID apply
// alongside the entities it deliberately left alone, and the caller is expected
// to report both.
import { useState } from 'react'
import { useInfiniteQuery, useMutation, useQueryClient } from '@tanstack/react-query'
import { z } from 'zod'
import { ApiError, api, errorMessage } from './api'
import { gardenFullKey } from './objects'
export const changeSourceSchema = z.enum(['ui', 'agent', 'api'])
export type ChangeSource = z.infer<typeof changeSourceSchema>
export const changeCountSchema = z.object({
entityType: z.enum(['garden', 'object', 'planting']),
op: z.enum(['create', 'update', 'delete']),
// COUNT(*) can only ever be a non-negative integer; constraining it means bad
Outdated
Review

🟠 changeCountSchema allows negative counts, which can hide undo buttons or produce incorrect totals

error-handling · flagged by 1 model

  • web/src/lib/history.ts:21n 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>
// data fails loudly rather than quietly hiding an Undo button (totalChanges
// gates it) or rendering "1.5 beds changed".
n: z.number().int().nonnegative(),
})
export type ChangeCount = z.infer<typeof changeCountSchema>
Review

🔴 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:27changeCountSchema 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>
export const changeSetSchema = z.object({
id: z.number(),
gardenId: z.number(),
actorId: z.number(),
actorName: z.string().default(''),
source: changeSourceSchema,
summary: z.string(),
agentRunId: z.string().optional(),
// Set when this change set reverts another.
revertsId: z.number().optional(),
// Set when another change set reverted this one.
revertedById: z.number().optional(),
counts: z.array(changeCountSchema).default([]),
createdAt: z.string(),
})
export type ChangeSet = z.infer<typeof changeSetSchema>
export const revertConflictSchema = z.object({
entityType: z.enum(['garden', 'object', 'planting']),
entityId: z.number(),
reason: z.enum(['changed', 'missing', 'exists', 'unsupported']),
name: z.string().optional(),
})
export type RevertConflict = z.infer<typeof revertConflictSchema>
const historyPageSchema = z.object({
changeSets: z.array(changeSetSchema),
hasMore: z.boolean(),
})
const revertResultSchema = z.object({
changeSet: changeSetSchema.nullable(),
conflicts: z.array(revertConflictSchema).default([]),
})
export type RevertResult = z.infer<typeof revertResultSchema>
const PAGE_SIZE = 30
export function historyKey(gardenId: number) {
return ['gardens', gardenId, 'history'] as const
}
/** A garden's change sets, newest first, paged on demand. */
export function useGardenHistory(gardenId: number, enabled = true) {
return useInfiniteQuery({
queryKey: historyKey(gardenId),
enabled,
initialPageParam: 0,
queryFn: async ({ pageParam }) =>
historyPageSchema.parse(
await api.get(`/gardens/${gardenId}/history`, { params: { limit: PAGE_SIZE, offset: pageParam } }),
),
// Offset paging, not cursor: history is append-only at the FRONT, so a new
// entry arriving mid-scroll shifts the window by one. That's a duplicate row
// at worst, and the alternative (a cursor on created_at) buys little for a
// list you scroll a page or two of.
getNextPageParam: (last, pages) => (last.hasMore ? pages.length * PAGE_SIZE : undefined),
})
}
/** The conflicts carried by a 409 from a revert, or null if this isn't one. */
export function revertConflicts(err: unknown): RevertResult | null {
if (!(err instanceof ApiError) || err.status !== 409) return null
const parsed = revertResultSchema.safeParse(err.body)
return parsed.success ? parsed.data : null
}
/**
* Undo a change set. Invalidates the editor payload so the canvas redraws, and
* the history list so the new entry (and the "reverted" mark on its target)
Review

🟡 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>
* appear — on failure too, because a 409 means part of it applied.
*/
export function useRevertChangeSet(gardenId: number) {
const qc = useQueryClient()
Review

🟠 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>
const refresh = () => {
void qc.invalidateQueries({ queryKey: gardenFullKey(gardenId) })
void qc.invalidateQueries({ queryKey: historyKey(gardenId) })
}
return useMutation({
mutationFn: async (changeSetId: number): Promise<RevertResult> =>
revertResultSchema.parse(await api.post(`/change-sets/${changeSetId}/revert`, undefined)),
onSuccess: refresh,
onError: (err) => {
if (revertConflicts(err)) refresh() // a partial revert still changed things
},
})
}
/** How many rows a change set touched, for "3 changes" in the list. */
export function totalChanges(cs: ChangeSet): number {
return cs.counts.reduce((sum, c) => sum + c.n, 0)
}
const ENTITY_NOUNS: Record<ChangeCount['entityType'], [string, string]> = {
garden: ['garden setting', 'garden settings'],
object: ['bed', 'beds'],
planting: ['planting', 'plantings'],
}
const OP_VERBS: Record<ChangeCount['op'], string> = {
create: 'added',
update: 'changed',
delete: 'removed',
}
/**
* The change set's counts as a human phrase: "3 plantings added, 1 bed changed".
* Returns '' when there's nothing to say, so callers can skip the line entirely
* rather than render "0 changes".
*/
export function describeCounts(cs: ChangeSet): string {
return cs.counts
.filter((c) => c.n > 0)
.map((c) => {
const [one, many] = ENTITY_NOUNS[c.entityType]
return `${c.n} ${c.n === 1 ? one : many} ${OP_VERBS[c.op]}`
})
.join(', ')
}
/** What a revert left alone, phrased for the person who asked for the undo. */
export function describeConflict(c: RevertConflict): string {
const noun = c.name ? `${c.name}` : `that ${ENTITY_NOUNS[c.entityType][0]}`
switch (c.reason) {
case 'changed':
return `${noun} was edited since, so it was left alone`
case 'missing':
return `${noun} no longer exists`
case 'exists':
return `${noun} exists again, so it wasn't restored over the top`
case 'unsupported':
return `${noun} can't be undone automatically`
}
}
Review

🟡 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>
/**
* The one undo implementation, shared by the history list and (per #49) the
* agent turn's inline Undo in #57 — so an undo behaves identically wherever it
* is offered, including how it explains a partial result.
*
* Outcomes are keyed by change set id, so a single instance serves a whole list.
*/
Review

🟠 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:85useUndo 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>
export function useUndo(gardenId: number) {
const revert = useRevertChangeSet(gardenId)
const [outcomes, setOutcomes] = useState<Record<number, UndoOutcome>>({})
const undo = (cs: ChangeSet) => {
setOutcomes((prev) => ({ ...prev, [cs.id]: { tone: 'pending', message: 'Undoing…' } }))
revert.mutate(cs.id, {
onSuccess: (result) => {
setOutcomes((prev) => ({ ...prev, [cs.id]: describeUndo(cs, result) }))
Review

🟡 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>
},
onError: (err) => {
const result = revertConflicts(err)
setOutcomes((prev) => ({
...prev,
[cs.id]: result
? describeUndo(cs, result)
: { tone: 'error', message: errorMessage(err, "That couldn't be undone.") },
}))
},
})
}
return {
Review

🟡 useUndo returns isPending but no caller uses it (dead public API)

maintainability · flagged by 2 models

  • web/src/lib/history.ts:192isPending 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>
undo,
// Per change set rather than a single global flag, so one row's pending
// state can't disable the others.
outcomeFor: (id: number): UndoOutcome | undefined => outcomes[id],
}
}
export interface UndoOutcome {
tone: 'pending' | 'ok' | 'partial' | 'error'
message: string
}
/**
* What actually happened, said in full. A revert can partly succeed, and "2 of 3
* changes undone — “north bed” was edited since and was left alone" is the only
* useful thing to report: a bare failure would be a lie about the two that did
* apply, and a bare success would hide the one that didn't.
Review

🔴 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>
*/
export function describeUndo(target: ChangeSet, result: RevertResult): UndoOutcome {
const skipped = result.conflicts.map(describeConflict).join('; ')
const applied = result.changeSet ? totalChanges(result.changeSet) : 0
if (result.conflicts.length === 0) {
// The server answers 200 with a null change set when every revision was
// already a no-op — reachable by undoing a creation whose object is gone.
// Claiming "Undone." there would be reporting work that didn't happen.
if (applied === 0) return { tone: 'ok', message: 'Nothing left to undo — this was already reversed.' }
return { tone: 'ok', message: 'Undone.' }
}
if (applied === 0) {
return { tone: 'error', message: `Nothing was undone — ${skipped}.` }
}
const total = totalChanges(target)
return { tone: 'partial', message: `${applied} of ${total} changes undone — ${skipped}.` }
}
+5 -1
View File
@@ -69,7 +69,11 @@ export function toEditorObject(o: ServerObject): EditorObject {
}
}
const fullKey = (gardenId: number) => ['garden-full', gardenId] as const
// Exported so anything that changes a garden's contents from outside this file
// — undo, in particular — can invalidate the editor payload and make the canvas
// redraw.
export const gardenFullKey = (gardenId: number) => ['garden-full', gardenId] as const
const fullKey = gardenFullKey
export function gardenFullQueryOptions(gardenId: number) {
return queryOptions({
+82 -39
View File
@@ -3,6 +3,8 @@ import { getRouteApi } from '@tanstack/react-router'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { GardenCanvas } from '@/editor/GardenCanvas'
import { EditorRail, type RailTab } from '@/editor/EditorRail'
import { HistoryPanel } from '@/editor/HistoryPanel'
import { Inspector } from '@/editor/Inspector'
import { PlopInspector } from '@/editor/PlopInspector'
import { PlantPicker } from '@/editor/PlantPicker'
@@ -38,11 +40,10 @@ export function GardenEditorPage() {
const selectPlanting = useEditorStore((s) => s.selectPlanting)
const focusedObjectId = useEditorStore((s) => s.focusedObjectId)
const setFocusedObject = useEditorStore((s) => s.setFocusedObject)
const railTab = useEditorStore((s) => s.railTab)
const setRailTab = useEditorStore((s) => s.setRailTab)
const armedPlant = useEditorStore((s) => s.armedPlant)
const setArmedPlant = useEditorStore((s) => s.setArmedPlant)
const setArmedKind = useEditorStore((s) => s.setArmedKind)
const setLiveObject = useEditorStore((s) => s.setLiveObject)
const setLivePlanting = useEditorStore((s) => s.setLivePlanting)
const updatePlanting = useUpdatePlanting(gid)
const updateObject = useUpdateObject(gid)
@@ -79,14 +80,9 @@ export function GardenEditorPage() {
// gardens (and on leaving). `focus` is intentionally read once here, not a dep,
// so URL syncs below don't retrigger a full reset.
useEffect(() => {
const clear = () => {
select(null)
selectPlanting(null)
setArmedKind(null)
setArmedPlant(null)
setLiveObject(null)
setLivePlanting(null)
}
// resetTransient is the single list of ephemeral editor state, so new state
// (the rail tab did exactly this) can't be forgotten here.
const clear = () => useEditorStore.getState().resetTransient()
clear()
setFocusedObject(focus ?? null)
return () => {
@@ -110,6 +106,17 @@ export function GardenEditorPage() {
}
}, [focusedObjectId, objects, full.data, setFocusedObject])
// Selecting anything lands you in the inspector without a click — the rail
// must never be something you operate before you can edit. Keyed off the
// selected ids rather than a "something is selected" boolean, so selecting a
// DIFFERENT object while History is open still brings the inspector forward.
// Deselecting drops back out of the inspector but leaves History/Chat open if
// that's where you were, since those aren't about the selection.
useEffect(() => {
if (selectedId != null || selectedPlantingId != null) setRailTab('inspector')
else if (useEditorStore.getState().railTab === 'inspector') setRailTab(null)
}, [selectedId, selectedPlantingId, setRailTab])
const exitFocus = () => {
setFocusedObject(null)
setArmedPlant(null)
1
@@ -270,6 +277,48 @@ export function GardenEditorPage() {
setPicker(null)
}
// One rail, tabs inside it — the layout decision #49 settles for the journal
// (#53) and chat (#57) panels too: they add a tab here rather than each
// claiming their own strip of screen.
const railTabs: RailTab[] = [
{
id: 'inspector',
label: 'Inspector',
render: () =>
selectedObject ? (
<Inspector
key={`obj-${selectedObject.id}`}
object={selectedObject}
gardenId={gid}
unit={garden.unitPref}
readOnly={!canEdit}
onFocus={() => {
setFocusedObject(selectedObject.id)
select(null)
}}
/>
) : selectedPlop ? (
<PlopInspector
key={`plop-${selectedPlop.id}`}
plop={selectedPlop}
plant={plantsById.get(selectedPlop.plantId)}
gardenId={gid}
unit={garden.unitPref}
readOnly={!canEdit}
onChangePlant={() => setPicker('change')}
onClose={() => selectPlanting(null)}
/>
) : (
<p className="text-sm text-muted">Select a bed or a planting to edit it.</p>
),
},
{
id: 'history',
label: 'History',
render: () => <HistoryPanel gardenId={gid} canEdit={canEdit} />,
},
]
return (
<div className="flex h-[calc(100vh-8rem)] flex-col gap-3 md:flex-row">
<div className="shrink-0 md:w-40">
@@ -285,6 +334,13 @@ export function GardenEditorPage() {
<p className="mb-2 rounded-md bg-border/40 px-2 py-1 text-xs text-muted">👁 View only</p>
)}
{focusedObjectId == null && canEdit && <Palette />}
<Button
variant="ghost"
className="mt-2 w-full text-sm"
onClick={() => setRailTab(railTab === 'history' ? null : 'history')}
>
History
</Button>
</div>
<div className="relative min-h-0 flex-1">
@@ -335,34 +391,21 @@ export function GardenEditorPage() {
)}
</div>
{(selectedObject || selectedPlop) && (
<div className="fixed inset-x-0 bottom-0 z-30 max-h-[65vh] overflow-y-auto rounded-t-xl border-t border-border bg-surface p-4 shadow-lg md:static md:max-h-none md:w-72 md:shrink-0 md:rounded-xl md:border md:p-4 md:shadow-sm">
{selectedObject && (
<Inspector
key={`obj-${selectedObject.id}`}
object={selectedObject}
gardenId={gid}
unit={garden.unitPref}
readOnly={!canEdit}
onFocus={() => {
setFocusedObject(selectedObject.id)
select(null)
}}
/>
)}
{selectedPlop && (
<PlopInspector
key={`plop-${selectedPlop.id}`}
plop={selectedPlop}
plant={plantsById.get(selectedPlop.plantId)}
gardenId={gid}
unit={garden.unitPref}
readOnly={!canEdit}
onChangePlant={() => setPicker('change')}
onClose={() => selectPlanting(null)}
/>
)}
</div>
{railTab && (
<EditorRail
tabs={railTabs}
activeId={railTab}
onActivate={setRailTab}
onClose={() => {
// Only the inspector is *about* the selection, so only closing it
// deselects; dismissing History leaves the canvas as you had it.
if (railTab === 'inspector') {
select(null)
selectPlanting(null)
}
setRailTab(null)
}}
Review

🟠 Closing rail from History tab clears canvas selection

correctness · flagged by 1 model

  • web/src/pages/GardenEditorPage.tsx:407EditorRail'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>
/>
)}
{picker && (