History panel + undo in the editor (#49) #63
@@ -23,7 +23,9 @@ export function HistoryPanel({ gardenId, canEdit }: { gardenId: number; canEdit:
|
||||
<h2 className="text-sm font-semibold text-fg">History</h2>
|
||||
|
|
||||
|
||||
{history.isPending && <p className="text-sm text-muted">Loading…</p>}
|
||||
{history.isError && <Alert>{errorMessage(history.error, 'Could not load history.')}</Alert>}
|
||||
{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">
|
||||
@@ -38,6 +40,7 @@ export function HistoryPanel({ gardenId, canEdit }: { gardenId: number; canEdit:
|
||||
</ol>
|
||||
|
||||
{history.hasNextPage && (
|
||||
<>
|
||||
<Button
|
||||
variant="ghost"
|
||||
|
gitea-actions
commented
🟠 Pagination failure silently swallowed — no user feedback when fetching older history fails error-handling · flagged by 2 models
🪰 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"
|
||||
@@ -46,6 +49,14 @@ export function HistoryPanel({ gardenId, canEdit }: { gardenId: number; canEdit:
|
||||
>
|
||||
{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
|
||||
@@ -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()
|
||||
|
||||
@@ -103,5 +103,6 @@ export const useEditorStore = create<EditorState>((set) => ({
|
||||
armedKind: null,
|
||||
liveObject: null,
|
||||
livePlanting: null,
|
||||
railTab: null,
|
||||
}),
|
||||
}))
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
+11
-3
@@ -18,7 +18,10 @@ export type ChangeSource = z.infer<typeof changeSourceSchema>
|
||||
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
|
||||
|
gitea-actions
commented
🟠 changeCountSchema allows negative counts, which can hide undo buttons or produce incorrect totals error-handling · flagged by 1 model
🪰 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>
|
||||
|
||||
|
gitea-actions
commented
🔴 Unconstrained z.number() in changeCountSchema allows invalid server data to hide undo button or show fractional counts error-handling · flagged by 1 model
🪰 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>
|
||||
@@ -188,8 +191,9 @@ export function useUndo(gardenId: number) {
|
||||
|
||||
return {
|
||||
|
gitea-actions
commented
🟡 useUndo returns isPending but no caller uses it (dead public API) maintainability · flagged by 2 models
🪰 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],
|
||||
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}.` }
|
||||
}
|
||||
|
||||
@@ -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={() => {
|
||||
setRailTab(null)
|
||||
// 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)
|
||||
}}
|
||||
|
gitea-actions
commented
🟠 Closing rail from History tab clears canvas selection correctness · flagged by 1 model
🪰 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>
|
||||
/>
|
||||
)}
|
||||
|
||||
Reference in New Issue
Block a user
🟡 history.isError shows "Could not load history" even when only the next page failed, not the initial load
error-handling, maintainability · flagged by 1 model
web/src/editor/HistoryPanel.tsx:26— misleading error message when only the next page fails.history.isErrorbecomes true on afetchNextPagefailure too (TanStack setserror/isErrorfor any fetch, not just the initial one). When the user clicks "Load older" and that request fails, the panel renders<Alert>Could not load history.</Alert>above the already-loaded entries — which reads as "the whole history failed," even though the older entries are still on screen and retryable…🪰 Gadfly · advisory