From 47a417ff2424cf791fedc845c18520dd2b4eb41f Mon Sep 17 00:00:00 2001 From: Matthew Robert Wesney <157447210+dovvnloading@users.noreply.github.com> Date: Mon, 7 Sep 2026 11:11:49 -0400 Subject: [PATCH] fix(frontend): keep unsaved memory edits when a memory is added Editing a memory row and then adding a new memory reverted the edit, with no warning and nothing to undo it. The draft list is re-seeded from the server whenever the server's answer changes. That effect exists for a reason -- #241 added it so a row the server normalized away (trimmed to nothing, a case-insensitive duplicate) stops sitting on screen looking saved -- but it re-seeded wholesale. Adding a memory also changes the server's answer, so every other row the user had edited and not yet saved was silently overwritten with the server's copy. Its own comment claimed the opposite ("leaves in-progress edits alone between saves"); that only held while nothing else changed the list. The effect now reconciles instead of replacing. Each row remembers the server value it was seeded from, and a server list that has changed carries every surviving row's in-progress value across by matching on that origin, building a fresh row only for genuinely new entries. Matching on the origin rather than the current value is the point: an edited row no longer equals its server value, which is exactly the case being preserved. Both behaviours are now pinned, including one test that exercises them together -- a normalized-away row is dropped in the same update that an edit to a surviving row is preserved -- so the fix cannot be satisfied by simply preferring the draft over the server or the reverse. Co-Authored-By: Claude Opus 5 --- .../features/settings/MemoryPanel.test.tsx | 68 +++++++++++++++++++ .../src/features/settings/MemoryPanel.tsx | 40 +++++++++-- 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/frontend/src/features/settings/MemoryPanel.test.tsx b/frontend/src/features/settings/MemoryPanel.test.tsx index 9f0839e..81b1474 100644 --- a/frontend/src/features/settings/MemoryPanel.test.tsx +++ b/frontend/src/features/settings/MemoryPanel.test.tsx @@ -1,5 +1,6 @@ import { render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; +import { useState } from "react"; import { describe, expect, it, vi } from "vitest"; import { MemoryPanel } from "./MemoryPanel"; @@ -24,6 +25,39 @@ describe("MemoryPanel", () => { expect(input).toHaveFocus(); }); + it("keeps unsaved edits to other rows when a memory is added", async () => { + // Adding a memory changes the server list, which re-seeded the whole + // draft. Every other row the user had edited but not yet saved silently + // reverted to the server's copy. + const user = userEvent.setup(); + const onReplace = vi.fn<(memos: string[]) => Promise>().mockResolvedValue(); + + function Host() { + const [memos, setMemos] = useState(["First", "Second"]); + return ( + { setMemos((current) => [...current, memo]); }} + onReplace={onReplace} + onClear={vi.fn<() => Promise>().mockResolvedValue()} + /> + ); + } + render(); + + await user.clear(screen.getByRole("textbox", { name: "Memory 2" })); + await user.type(screen.getByRole("textbox", { name: "Memory 2" }), "Edited"); + + await user.type(screen.getByRole("textbox", { name: "New memory" }), "Third"); + await user.click(screen.getByRole("button", { name: "Add memory" })); + await screen.findByRole("textbox", { name: "Memory 3" }); + + expect(screen.getByRole("textbox", { name: "Memory 2" })).toHaveValue("Edited"); + await user.click(screen.getByRole("button", { name: "Save changes" })); + expect(onReplace).toHaveBeenCalledWith(["First", "Edited", "Third"]); + }); + it("preserves edited rows when removing a neighboring row", async () => { const user = userEvent.setup(); const onReplace = vi.fn<(memos: string[]) => Promise>().mockResolvedValue(); @@ -161,4 +195,38 @@ describe("MemoryPanel server reconciliation", () => { }); expect(screen.getByDisplayValue("kept")).toBeInTheDocument(); }); + + it("drops a normalized-away row without discarding an edit to a surviving one", async () => { + // Both halves at once: reconciliation must not be satisfied by simply + // preferring the draft (which would keep the rejected row on screen) or + // by simply preferring the server (the original defect). + const user = userEvent.setup(); + const { waitFor } = await import("@testing-library/react"); + + function Harness() { + const [memos, setMemos] = useState(["kept", "duplicate"]); + return ( + <> + + Promise>().mockResolvedValue()} + onReplace={vi.fn<(memos: string[]) => Promise>().mockResolvedValue()} + onClear={vi.fn<() => Promise>().mockResolvedValue()} + /> + + ); + } + render(); + + await user.clear(screen.getByRole("textbox", { name: "Memory 1" })); + await user.type(screen.getByRole("textbox", { name: "Memory 1" }), "kept and edited"); + await user.click(screen.getByRole("button", { name: "server responded" })); + + await waitFor(() => { + expect(screen.queryByDisplayValue("duplicate")).not.toBeInTheDocument(); + }); + expect(screen.getByRole("textbox", { name: "Memory 1" })).toHaveValue("kept and edited"); + }); }); diff --git a/frontend/src/features/settings/MemoryPanel.tsx b/frontend/src/features/settings/MemoryPanel.tsx index 4316998..5aa7e06 100644 --- a/frontend/src/features/settings/MemoryPanel.tsx +++ b/frontend/src/features/settings/MemoryPanel.tsx @@ -9,15 +9,30 @@ type Props = { onClear: () => Promise; }; +type DraftRow = { + id: number; + value: string; + // The server value this row was last seeded from. An edited row no longer + // equals it, which is exactly why reconciliation cannot match on `value`. + origin: string; +}; + export function MemoryPanel({ memos, busy, onAdd, onReplace, onClear }: Props) { const [memo, setMemo] = useState(""); - const [draft, setDraft] = useState(() => memos.map((value, id) => ({ id, value }))); + const [draft, setDraft] = useState( + () => memos.map((value, id) => ({ id, value, origin: value })), + ); // `memos` is the authoritative list the server returned. The draft was // seeded from it once and never re-derived, so an entry the server // normalized away -- trimmed to nothing, or a case-insensitive duplicate -- - // stayed on screen looking saved. Re-seed whenever the server's answer - // actually changes, which leaves in-progress edits alone between saves. + // stayed on screen looking saved. + // + // Re-seeding wholesale fixed that and broke something else: adding a memory + // also changes the server's answer, so every *other* row the user had edited + // and not yet saved silently reverted. Reconcile instead -- carry each + // surviving row's in-progress value across by matching on the server value + // it came from, and build a fresh row only for genuinely new entries. const lastServerMemos = useRef(memos); useEffect(() => { const previous = lastServerMemos.current; @@ -25,7 +40,22 @@ export function MemoryPanel({ memos, busy, onAdd, onReplace, onClear }: Props) { previous.length !== memos.length || previous.some((value, index) => value !== memos[index]); if (!changed) return; lastServerMemos.current = memos; - setDraft(memos.map((value, id) => ({ id, value }))); + setDraft((current) => { + const byOrigin = new Map(); + for (const row of current) { + const bucket = byOrigin.get(row.origin); + if (bucket) bucket.push(row); + else byOrigin.set(row.origin, [row]); + } + let nextId = current.reduce((highest, row) => Math.max(highest, row.id), -1) + 1; + return memos.map((value) => { + // shift(), so duplicate server values claim distinct rows. + const existing = byOrigin.get(value)?.shift(); + return existing + ? { ...existing, origin: value } + : { id: nextId++, value, origin: value }; + }); + }); }, [memos]); const handleSubmit = (event: FormEvent) => { @@ -36,7 +66,7 @@ export function MemoryPanel({ memos, busy, onAdd, onReplace, onClear }: Props) { setDraft((current) => { if (current.some((item) => item.value.toLocaleLowerCase() === value.toLocaleLowerCase())) return current; const id = current.reduce((highest, item) => Math.max(highest, item.id), -1) + 1; - return [...current, { id, value }]; + return [...current, { id, value, origin: value }]; }); setMemo(""); }).catch(() => undefined);