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) }} /> )}