Undo reported "nothing left to undo" after a successful undo (#72)

Co-authored-by: Steve Dudenhoeffer <[email protected]>

internal/service/revisions.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"sort"
 	"sync"
 
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
@@ -334,9 +335,48 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6
 	if err != nil {
 		return nil, nil, err
 	}
+	// Fill in the per-op counts. commitScope returns the freshly inserted row,
+	// which carries no tally — and a caller receiving a change set with an empty
+	// Counts cannot tell "nothing was reverted" from "the tally wasn't loaded".
+	// That ambiguity is not theoretical: it made the chat panel report a
+	// successful undo as "nothing left to undo".
+	if cs != nil {
+		cs.Counts = countRevisions(sc.taken())
+	}
 	return cs, conflicts, nil
 }
 
+// countRevisions tallies revisions by (entity type, op).
+//
+// This deliberately reproduces in Go what ListChangeSets does in SQL, because a
+// change set that has just been written has no rows to GROUP BY yet — the
+// alternative is a second round trip to count what we are already holding.
+// The parity is a real cross-layer contract, and TestRevertResultCarriesItsCounts
+// compares the two breakdowns row for row so it can't drift silently.
+func countRevisions(revs []domain.Revision) []domain.ChangeCount {
+	type key struct{ entityType, op string }
+	seen := map[key]int{}
+	order := []key{}
+	for _, r := range revs {
+		k := key{r.EntityType, r.Op}
+		if _, ok := seen[k]; !ok {
+			order = append(order, k)
+		}
+		seen[k]++
+	}
+	sort.Slice(order, func(i, j int) bool {
+		if order[i].entityType != order[j].entityType {
+			return order[i].entityType < order[j].entityType
+		}
+		return order[i].op < order[j].op
+	})
+	counts := make([]domain.ChangeCount, 0, len(order))
+	for _, k := range order {
+		counts = append(counts, domain.ChangeCount{EntityType: k.entityType, Op: k.op, N: seen[k]})
+	}
+	return counts
+}
+
 // entityKey identifies a row across the entity types a revision can name.
 type entityKey struct {
 	entityType string

internal/service/revisions_test.go

@@ -704,3 +704,104 @@ func TestClearObjectOnlyClearsWhatItSnapshotted(t *testing.T) {
 		t.Errorf("cleared %d of %d with %d revisions — all three should match", n, len(before), len(revs))
 	}
 }
+
+// TestRevertResultCarriesItsCounts — found by using the thing.
+//
+// commitScope returns the freshly inserted row, which carries no tally. A caller
+// receiving a change set with empty Counts cannot tell "nothing was reverted"
+// from "the tally wasn't loaded", and the chat panel read that ambiguity the
+// wrong way: a successful undo reported "nothing left to undo", which is the
+// worst possible thing to say about an action that just worked.
+//
+// It also pins the cross-layer contract that fix created. countRevisions tallies
+// in Go what ListChangeSets tallies in SQL, and nothing but this test stops the
+// two drifting — so it compares the FULL per-(entity, op) breakdown, not just
+// the totals, which would agree even if the groupings had diverged.
+func TestRevertResultCarriesItsCounts(t *testing.T) {
+	s := newTestService(t, openConfig())
+	owner := seedUser(t, s, "[email protected]")
+	g := seedGarden(t, s, owner)
+	bed := seedBed(t, s, owner, g.ID)
+	plant := seedOwnPlant(t, s, owner, 15)
+	ctx := context.Background()
+
+	if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil {
+		t.Fatalf("fill: %v", err)
+	}
+	// A second, different kind of change, so the breakdown has more than one row
+	// to get wrong.
+	if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{Name: strPtr("North Bed")}, bed.Version); err != nil {
+		t.Fatalf("rename: %v", err)
+	}
+	sets := history(t, s, owner, g.ID)
+	rename, fill := sets[0], sets[1]
+
+	undoRename, conflicts, err := s.RevertChangeSet(ctx, owner, rename.ID, domain.SourceUI)
+	if err != nil || len(conflicts) != 0 {
+		t.Fatalf("revert rename: err=%v conflicts=%+v", err, conflicts)
+	}
+	undoFill, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI)
+	if err != nil || len(conflicts) != 0 {
+		t.Fatalf("revert fill: err=%v conflicts=%+v", err, conflicts)
+	}
+
+	for _, undo := range []*domain.ChangeSet{undoRename, undoFill} {
+		if undo == nil {
+			t.Fatal("a revert that did work returned no change set")
+		}
+		if len(undo.Counts) == 0 {
+			t.Fatalf("change set %d came back with no tally — an empty tally reads as 'nothing happened'", undo.ID)
+		}
+	}
+	// Undoing a fill deletes every plop it created, so the tallies must match.
+	if got, want := countsTotal(undoFill.Counts), countsTotal(fill.Counts); got != want {
+		t.Errorf("undo of the fill reports %d changes, want %d", got, want)
+	}
+
+	// The SQL tally and the Go tally must agree, row for row.
+	listed := history(t, s, owner, g.ID)
+	byID := map[int64][]domain.ChangeCount{}
+	for _, cs := range listed {
+		byID[cs.ID] = cs.Counts
+	}
+	for _, undo := range []*domain.ChangeSet{undoRename, undoFill} {
+		fromSQL, ok := byID[undo.ID]
+		if !ok {
+			t.Fatalf("revert %d is missing from the history list", undo.ID)
+		}
+		if !sameCounts(undo.Counts, fromSQL) {
+			t.Errorf("change set %d: revert returned %+v, the list read says %+v — countRevisions and the SQL grouping have drifted",
+				undo.ID, undo.Counts, fromSQL)
+		}
+	}
+}
+
+// sameCounts compares two tallies regardless of order.
+func sameCounts(a, b []domain.ChangeCount) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	index := func(cs []domain.ChangeCount) map[string]int {
+		m := map[string]int{}
+		for _, c := range cs {
+			m[c.EntityType+"/"+c.Op] = c.N
+		}
+		return m
+	}
+	ia, ib := index(a), index(b)
+	for k, v := range ia {
+		if ib[k] != v {
+			return false
+		}
+	}
+	return true
+}
+
+// countsTotal sums a tally — the server-side twin of the client's totalChanges.
+func countsTotal(counts []domain.ChangeCount) int {
+	n := 0
+	for _, c := range counts {
+		n += c.N
+	}
+	return n
+}

internal/store/revisions.go

@@ -119,7 +119,9 @@ func (d *DB) ListChangeSets(ctx context.Context, gardenID int64, limit, offset i
 		return sets, nil
 	}
 
-	// One grouped query for the whole page rather than N per-row counts.
+	// One grouped query for the whole page rather than N per-row counts. The
+	// service's countRevisions mirrors this grouping for change sets it has just
+	// written; TestRevertResultCarriesItsCounts holds the two in step.
 	countRows, err := d.sql.QueryContext(ctx,
 		`SELECT change_set_id, entity_type, op, COUNT(*)
 		   FROM revisions

web/src/lib/history.test.ts

@@ -49,6 +49,14 @@ describe('describeConflict', () => {
 describe('describeUndo', () => {
   const target = changeSet({ counts: [{ entityType: 'object', op: 'update', n: 3 }] })
 
+  // The bug this pair exists for: a real revert response carries a change set
+  // whose counts the server didn't populate. Reading that as "nothing happened"
+  // told the user a successful undo had done nothing.
+  it('trusts the change set, not the tally, for whether anything happened', () => {
+    const out = describeUndo({ id: 5 }, { changeSet: changeSet({ id: 6, counts: [] }), conflicts: [] })
+    expect(out).toEqual({ tone: 'ok', message: 'Undone.' })
+  })
+
   it('is a plain success when nothing conflicted', () => {
     const out = describeUndo(target, {
       changeSet: changeSet({ id: 2, counts: [{ entityType: 'object', op: 'update', n: 3 }] }),
@@ -61,6 +69,7 @@ describe('describeUndo', () => {
   // 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", () => {
+    // A NULL change set — not an empty tally — is the server's no-op signal.
     const out = describeUndo(target, { changeSet: null, conflicts: [] })
     expect(out.tone).toBe('ok')
     expect(out.message).toBe('Nothing left to undo — this was already reversed.')

web/src/lib/history.ts

@@ -224,20 +224,23 @@ export interface UndoOutcome {
  */
 export function describeUndo(target: UndoTarget, result: RevertResult): UndoOutcome {
   const skipped = result.conflicts.map(describeConflict).join('; ')
+  // A NULL change set is the server's signal that nothing needed doing. An empty
+  // `counts` is not the same thing and must not be read as one — that conflation
+  // made a successful undo report "nothing left to undo", which is the worst
+  // possible thing to tell someone about an action that just worked.
+  const didSomething = result.changeSet != null
   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
+    // Reachable by undoing a creation whose object is already gone.
-    // already a no-op — reachable by undoing a creation whose object is gone.
+    if (!didSomething) return { tone: 'ok', message: 'Nothing left to undo — this was already reversed.' }
-    // 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) {
+  if (!didSomething) {
     return { tone: 'error', message: `Nothing was undone — ${skipped}.` }
   }
   // Only claim a denominator when we have one. "2 of 3" from a caller that
   // never knew the total would be a number invented to fill a sentence.
   const total = totalChanges(target)
-  const scale = total > 0 ? `${applied} of ${total} changes undone` : 'Partly undone'
+  const scale = total > 0 && applied > 0 ? `${applied} of ${total} changes undone` : 'Partly undone'
   return { tone: 'partial', message: `${scale} — ${skipped}.` }
 }