From 4b18ac3b46ceed521c3d2a39ba045fad91f0eeb4 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 01:13:19 -0400 Subject: [PATCH 1/2] History panel and undo in the editor (#49) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- DESIGN.md | 3 +- web/src/editor/EditorRail.tsx | 88 ++++++++++++ web/src/editor/HistoryPanel.tsx | 131 +++++++++++++++++ web/src/editor/UndoButton.tsx | 51 +++++++ web/src/editor/store.ts | 9 ++ web/src/lib/history.test.ts | 103 ++++++++++++++ web/src/lib/history.ts | 218 +++++++++++++++++++++++++++++ web/src/lib/objects.ts | 6 +- web/src/pages/GardenEditorPage.tsx | 103 ++++++++++---- 9 files changed, 682 insertions(+), 30 deletions(-) create mode 100644 web/src/editor/EditorRail.tsx create mode 100644 web/src/editor/HistoryPanel.tsx create mode 100644 web/src/editor/UndoButton.tsx create mode 100644 web/src/lib/history.test.ts create mode 100644 web/src/lib/history.ts diff --git a/DESIGN.md b/DESIGN.md index cfd16aa..70508b5 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -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. ## Roadmap diff --git a/web/src/editor/EditorRail.tsx b/web/src/editor/EditorRail.tsx new file mode 100644 index 0000000..2aa76b4 --- /dev/null +++ b/web/src/editor/EditorRail.tsx @@ -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 ( +
+
+ {tabs.map((tab) => ( + + ))} + +
+
{active.render()}
+
+ ) +} diff --git a/web/src/editor/HistoryPanel.tsx b/web/src/editor/HistoryPanel.tsx new file mode 100644 index 0000000..b3a43a3 --- /dev/null +++ b/web/src/editor/HistoryPanel.tsx @@ -0,0 +1,131 @@ +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) + const sets = history.data?.pages.flatMap((p) => p.changeSets) ?? [] + + return ( +
+

History

+ + {history.isPending &&

Loading…

} + {history.isError && {errorMessage(history.error, 'Could not load history.')}} + + {history.isSuccess && sets.length === 0 && ( +

+ Nothing yet. Every change you make here shows up in this list, and can be undone from it. +

+ )} + +
    + {sets.map((cs) => ( + + ))} +
+ + {history.hasNextPage && ( + + )} + + {/* 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. */} +

+ Deleting a whole garden isn't covered here and can't be undone. +

+
+ ) +} + +function HistoryEntry({ + changeSet, + canEdit, + undo, +}: { + changeSet: ChangeSet + canEdit: boolean + undo: ReturnType +}) { + const counts = describeCounts(changeSet) + const reverted = changeSet.revertedById != null + const isRevert = changeSet.revertsId != null + + return ( +
  • +
    +
    +

    {changeSet.summary}

    +

    + {changeSet.source === 'agent' && ( + + agent + + )} + {isRevert && undo} + {changeSet.actorName} + · + + {counts && ( + <> + · + {counts} + + )} +

    + {reverted &&

    Undone

    } +
    + + {canEdit && totalChanges(changeSet) > 0 && ( + + )} +
    +
    • + ) +} + +/** A compact "3m ago" / "yesterday" for a UTC timestamp. */ +export function relativeTime(iso: string): string { + const then = Date.parse(iso) + if (Number.isNaN(then)) return iso + const seconds = Math.round((Date.now() - then) / 1000) + if (seconds < 60) return 'just now' + const minutes = Math.round(seconds / 60) + if (minutes < 60) return `${minutes}m ago` + const hours = Math.round(minutes / 60) + if (hours < 24) return `${hours}h ago` + const days = Math.round(hours / 24) + if (days === 1) return 'yesterday' + if (days < 30) return `${days}d ago` + return new Date(then).toLocaleDateString() +} diff --git a/web/src/editor/UndoButton.tsx b/web/src/editor/UndoButton.tsx new file mode 100644 index 0000000..f97ad66 --- /dev/null +++ b/web/src/editor/UndoButton.tsx @@ -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 + className?: string + label?: string +}) { + const outcome = undo.outcomeFor(changeSet.id) + return ( +
    + + {outcome && outcome.tone !== 'pending' && } +
    + ) +} + +const TONE_CLASS: Record, 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 ( +

    + {outcome.message} +

    + ) +} diff --git a/web/src/editor/store.ts b/web/src/editor/store.ts index dfe194c..ff39d5d 100644 --- a/web/src/editor/store.ts +++ b/web/src/editor/store.ts @@ -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((set) => ({ focusedObjectId: null, setFocusedObject: (id) => set({ focusedObjectId: id }), + railTab: null, + setRailTab: (tab) => set({ railTab: tab }), + armedPlant: null, setArmedPlant: (p) => set({ armedPlant: p }), diff --git a/web/src/lib/history.test.ts b/web/src/lib/history.test.ts new file mode 100644 index 0000000..272fde6 --- /dev/null +++ b/web/src/lib/history.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it } from 'vitest' +import { describeConflict, describeCounts, describeUndo, totalChanges, type ChangeSet } from './history' + +function changeSet(over: Partial = {}): 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 }), conflicts: [] }) + expect(out).toEqual({ tone: 'ok', message: 'Undone.' }) + }) + + // 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) + }) +}) diff --git a/web/src/lib/history.ts b/web/src/lib/history.ts new file mode 100644 index 0000000..a02420b --- /dev/null +++ b/web/src/lib/history.ts @@ -0,0 +1,218 @@ +// 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 + +export const changeCountSchema = z.object({ + entityType: z.enum(['garden', 'object', 'planting']), + op: z.enum(['create', 'update', 'delete']), + n: z.number(), +}) +export type ChangeCount = z.infer + +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 + +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 + +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 + +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) + * appear — on failure too, because a 409 means part of it applied. + */ +export function useRevertChangeSet(gardenId: number) { + const qc = useQueryClient() + const refresh = () => { + void qc.invalidateQueries({ queryKey: gardenFullKey(gardenId) }) + void qc.invalidateQueries({ queryKey: historyKey(gardenId) }) + } + return useMutation({ + mutationFn: async (changeSetId: number): Promise => + 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 = { + garden: ['garden setting', 'garden settings'], + object: ['bed', 'beds'], + planting: ['planting', 'plantings'], +} + +const OP_VERBS: Record = { + 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` + } +} + +/** + * 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. + */ +export function useUndo(gardenId: number) { + const revert = useRevertChangeSet(gardenId) + const [outcomes, setOutcomes] = useState>({}) + + 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) })) + }, + 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 { + undo, + outcomeFor: (id: number): UndoOutcome | undefined => outcomes[id], + isPending: revert.isPending, + } +} + +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. + */ +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.' } + } + const applied = result.changeSet ? totalChanges(result.changeSet) : 0 + 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}.` } +} diff --git a/web/src/lib/objects.ts b/web/src/lib/objects.ts index 3077f0a..322414c 100644 --- a/web/src/lib/objects.ts +++ b/web/src/lib/objects.ts @@ -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({ diff --git a/web/src/pages/GardenEditorPage.tsx b/web/src/pages/GardenEditorPage.tsx index 2a979d4..52123da 100644 --- a/web/src/pages/GardenEditorPage.tsx +++ b/web/src/pages/GardenEditorPage.tsx @@ -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,6 +40,8 @@ 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) @@ -86,6 +90,7 @@ export function GardenEditorPage() { setArmedPlant(null) setLiveObject(null) setLivePlanting(null) + setRailTab(null) } clear() setFocusedObject(focus ?? null) @@ -110,6 +115,16 @@ 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. 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. + const hasSelection = selectedId != null || selectedPlantingId != null + useEffect(() => { + if (hasSelection) setRailTab('inspector') + else if (useEditorStore.getState().railTab === 'inspector') setRailTab(null) + }, [hasSelection, setRailTab]) + const exitFocus = () => { setFocusedObject(null) setArmedPlant(null) @@ -270,6 +285,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 ? ( + { + setFocusedObject(selectedObject.id) + select(null) + }} + /> + ) : selectedPlop ? ( + setPicker('change')} + onClose={() => selectPlanting(null)} + /> + ) : ( +

    Select a bed or a planting to edit it.

    + ), + }, + { + id: 'history', + label: 'History', + render: () => , + }, + ] + return (
    @@ -285,6 +342,13 @@ export function GardenEditorPage() {

    👁 View only

    )} {focusedObjectId == null && canEdit && } +
    @@ -335,34 +399,17 @@ export function GardenEditorPage() { )}
    - {(selectedObject || selectedPlop) && ( -
    - {selectedObject && ( - { - setFocusedObject(selectedObject.id) - select(null) - }} - /> - )} - {selectedPlop && ( - setPicker('change')} - onClose={() => selectPlanting(null)} - /> - )} -
    + {railTab && ( + { + setRailTab(null) + select(null) + selectPlanting(null) + }} + /> )} {picker && ( -- 2.54.0 From 324cf2f5bb2be8662fb00864495b45a7e61de533 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 01:22:53 -0400 Subject: [PATCH 2/2] Address Gadfly review on the history panel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- web/src/editor/HistoryPanel.tsx | 41 ++++++++++++++++++++---------- web/src/editor/store.ts | 1 + web/src/lib/history.test.ts | 14 +++++++++- web/src/lib/history.ts | 14 +++++++--- web/src/pages/GardenEditorPage.tsx | 36 ++++++++++++-------------- 5 files changed, 69 insertions(+), 37 deletions(-) diff --git a/web/src/editor/HistoryPanel.tsx b/web/src/editor/HistoryPanel.tsx index b3a43a3..5762f31 100644 --- a/web/src/editor/HistoryPanel.tsx +++ b/web/src/editor/HistoryPanel.tsx @@ -23,7 +23,9 @@ export function HistoryPanel({ gardenId, canEdit }: { gardenId: number; canEdit:

    History

    {history.isPending &&

    Loading…

    } - {history.isError && {errorMessage(history.error, 'Could not load history.')}} + {history.isError && sets.length === 0 && ( + {errorMessage(history.error, 'Could not load history.')} + )} {history.isSuccess && sets.length === 0 && (

    @@ -38,14 +40,23 @@ export function HistoryPanel({ gardenId, canEdit }: { gardenId: number; canEdit: {history.hasNextPage && ( - + <> + + {/* 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 && ( +

    + {errorMessage(history.error, "Couldn't load older entries.")} +

    + )} + )} {/* Said plainly rather than discovered: deleting a garden bypasses this @@ -118,13 +129,17 @@ function HistoryEntry({ export function relativeTime(iso: string): string { const then = Date.parse(iso) if (Number.isNaN(then)) return iso - const seconds = Math.round((Date.now() - then) / 1000) + 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' - const minutes = Math.round(seconds / 60) + // 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.round(minutes / 60) + const hours = Math.floor(minutes / 60) if (hours < 24) return `${hours}h ago` - const days = Math.round(hours / 24) + const days = Math.floor(hours / 24) if (days === 1) return 'yesterday' if (days < 30) return `${days}d ago` return new Date(then).toLocaleDateString() diff --git a/web/src/editor/store.ts b/web/src/editor/store.ts index ff39d5d..d479bcd 100644 --- a/web/src/editor/store.ts +++ b/web/src/editor/store.ts @@ -103,5 +103,6 @@ export const useEditorStore = create((set) => ({ armedKind: null, liveObject: null, livePlanting: null, + railTab: null, }), })) diff --git a/web/src/lib/history.test.ts b/web/src/lib/history.test.ts index 272fde6..e098759 100644 --- a/web/src/lib/history.test.ts +++ b/web/src/lib/history.test.ts @@ -50,10 +50,22 @@ 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 }), conflicts: [] }) + 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', () => { diff --git a/web/src/lib/history.ts b/web/src/lib/history.ts index a02420b..f3e1a19 100644 --- a/web/src/lib/history.ts +++ b/web/src/lib/history.ts @@ -18,7 +18,10 @@ export type ChangeSource = z.infer export const changeCountSchema = z.object({ entityType: z.enum(['garden', 'object', 'planting']), op: z.enum(['create', 'update', 'delete']), - n: z.number(), + // COUNT(*) can only ever be a non-negative integer; constraining it means bad + // 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 @@ -188,8 +191,9 @@ export function useUndo(gardenId: number) { return { 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], - isPending: revert.isPending, } } @@ -206,10 +210,14 @@ export interface UndoOutcome { */ 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.' } } - const applied = result.changeSet ? totalChanges(result.changeSet) : 0 if (applied === 0) { return { tone: 'error', message: `Nothing was undone — ${skipped}.` } } diff --git a/web/src/pages/GardenEditorPage.tsx b/web/src/pages/GardenEditorPage.tsx index 52123da..92541a2 100644 --- a/web/src/pages/GardenEditorPage.tsx +++ b/web/src/pages/GardenEditorPage.tsx @@ -44,9 +44,6 @@ export function GardenEditorPage() { 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) @@ -83,15 +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) - setRailTab(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 () => { @@ -116,14 +107,15 @@ 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. 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. - const hasSelection = selectedId != null || selectedPlantingId != null + // 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 (hasSelection) setRailTab('inspector') + if (selectedId != null || selectedPlantingId != null) setRailTab('inspector') else if (useEditorStore.getState().railTab === 'inspector') setRailTab(null) - }, [hasSelection, setRailTab]) + }, [selectedId, selectedPlantingId, setRailTab]) const exitFocus = () => { setFocusedObject(null) @@ -405,9 +397,13 @@ export function GardenEditorPage() { 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) - select(null) - selectPlanting(null) }} /> )} -- 2.54.0