[Frontend] "Append to beat notes" silently replaces them — the session list never carried beat_notes #454

New issue

Closed

opened 2026-08-31 01:37:23 +00:00 by claude-bot · 0 comments

claude-bot commented

2026-08-31 01:37:23 +00:00

Contributor

Severity: HIGH — active data loss, not a latent race.

Found while wiring conditional writes for #403 (PR #450).

GET /api/campaigns/{id}/sessions returns SessionListItem, which has never carried beat_notes (webapp/backend/app/schemas/session.py — the model has id, campaign_id, title, scheduling_mode, status, confirmed_time, end_time, created_at, has_summary, and now updated_at). Two frontend call sites read beat_notes off those list items anyway, so both silently see undefined.

Symptom 1 — the "Append" button replaces (data loss)

webapp/frontend/src/components/workbenchTools.jsx:542-546, in PrepResult.handleAppend:

const formatted = formatPrepSheet(output);
const current = sessions.find((s) => s.id === sessionId)?.beat_notes || "";
const next = current ? `${current}\n\n${formatted}` : formatted;
await updateBeatNotes(campaignId, sessionId, next);
setSaveStatus({ kind: "ok", msg: "Appended to beat notes." });

current is always "", so next is always just the newly generated prep sheet. The write replaces whatever the GM had written. The UI then reports "Appended to beat notes."

A GM with a session's worth of hand-written prep who generates a prep sheet and clicks Append loses all of it, and is told the opposite. There is no undo and no version history for beat_notes.

handleReplace (same file, ~line 559) is honest about replacing, so it is fine.

Symptom 2 — the beat-notes editor always opens empty

webapp/frontend/src/pages/CampaignPlanning.jsx, BeatPlanner: both the initial useEffect and handleSessionChange do setDraft(s?.beat_notes || "") against the same list items, so the textarea opens blank no matter what is stored. Typing into that blank box and saving replaces the stored notes.

Less sharp than symptom 1 (the GM sees an empty box rather than being told a false success), but the same root cause and the same outcome.

Why #403's conditional writes do not cover this

expected_updated_at stops a stale write. These writes are not stale — they are current writes built from data that was never fetched. The token matches, the guard passes, and the clobber goes through. Nothing about #403 helps here, which is exactly why it deserves its own issue rather than being folded in.

Proposed fix

Read the real value instead of inventing an empty one:

Add beat_notes to SessionListItem, or have both call sites fetch the session before composing an append. Adding it to the list schema is the smaller change and removes a round trip; beat_notes is GM-only, and the campaign session list is already GM/member-gated, so it exposes nothing new to a player — worth confirming that gating explicitly before doing it, since the list is served to every campaign member and beat notes are GM planning material.
If the value cannot be established, the append action must fail loudly rather than compose against "". A button labelled "Append" must never be able to replace.
Regression test: seed a session with existing beat_notes, click Append, assert the stored value still contains the original text.

Also

CampaignPlanning.jsx:251-254 carries a comment claiming the session list does not expose updated_at and that it is fetched per selection. That was true of an earlier draft of PR #450 and is not true of what shipped — the list does carry it now and it is read directly. Stale comment, worth correcting in the same change.

Acceptance criteria

PrepResult.handleAppend composes against the session's actual stored beat_notes, or refuses to run.
BeatPlanner's editor opens with the stored beat notes rather than blank.
Regression test: existing beat notes survive an Append.
Player-visibility of beat_notes is confirmed acceptable if it is added to the list schema.
The stale updated_at comment in CampaignPlanning.jsx is corrected.

**Severity: HIGH** — active data loss, not a latent race. Found while wiring conditional writes for #403 (PR #450). `GET /api/campaigns/{id}/sessions` returns `SessionListItem`, which has **never** carried `beat_notes` (`webapp/backend/app/schemas/session.py` — the model has `id`, `campaign_id`, `title`, `scheduling_mode`, `status`, `confirmed_time`, `end_time`, `created_at`, `has_summary`, and now `updated_at`). Two frontend call sites read `beat_notes` off those list items anyway, so both silently see `undefined`. ## Symptom 1 — the "Append" button replaces (data loss) `webapp/frontend/src/components/workbenchTools.jsx:542-546`, in `PrepResult.handleAppend`: ```js const formatted = formatPrepSheet(output); const current = sessions.find((s) => s.id === sessionId)?.beat_notes || ""; const next = current ? `${current}\n\n${formatted}` : formatted; await updateBeatNotes(campaignId, sessionId, next); setSaveStatus({ kind: "ok", msg: "Appended to beat notes." }); ``` `current` is **always** `""`, so `next` is always just the newly generated prep sheet. The write replaces whatever the GM had written. The UI then reports *"Appended to beat notes."* A GM with a session's worth of hand-written prep who generates a prep sheet and clicks **Append** loses all of it, and is told the opposite. There is no undo and no version history for `beat_notes`. `handleReplace` (same file, ~line 559) is honest about replacing, so it is fine. ## Symptom 2 — the beat-notes editor always opens empty `webapp/frontend/src/pages/CampaignPlanning.jsx`, `BeatPlanner`: both the initial `useEffect` and `handleSessionChange` do `setDraft(s?.beat_notes || "")` against the same list items, so the textarea opens blank no matter what is stored. Typing into that blank box and saving replaces the stored notes. Less sharp than symptom 1 (the GM sees an empty box rather than being told a false success), but the same root cause and the same outcome. ## Why #403's conditional writes do not cover this `expected_updated_at` stops a **stale** write. These writes are not stale — they are current writes built from data that was never fetched. The token matches, the guard passes, and the clobber goes through. Nothing about #403 helps here, which is exactly why it deserves its own issue rather than being folded in. ## Proposed fix Read the real value instead of inventing an empty one: 1. Add `beat_notes` to `SessionListItem`, or have both call sites fetch the session before composing an append. Adding it to the list schema is the smaller change and removes a round trip; `beat_notes` is GM-only, and the campaign session list is already GM/member-gated, so it exposes nothing new to a player — **worth confirming that gating explicitly before doing it**, since the list is served to every campaign member and beat notes are GM planning material. 2. If the value cannot be established, the append action must **fail loudly** rather than compose against `""`. A button labelled "Append" must never be able to replace. 3. Regression test: seed a session with existing `beat_notes`, click Append, assert the stored value still contains the original text. ## Also `CampaignPlanning.jsx:251-254` carries a comment claiming the session list does not expose `updated_at` and that it is fetched per selection. That was true of an earlier draft of PR #450 and is not true of what shipped — the list does carry it now and it is read directly. Stale comment, worth correcting in the same change. ## Acceptance criteria - [ ] `PrepResult.handleAppend` composes against the session's actual stored `beat_notes`, or refuses to run. - [ ] `BeatPlanner`'s editor opens with the stored beat notes rather than blank. - [ ] Regression test: existing beat notes survive an Append. - [ ] Player-visibility of `beat_notes` is confirmed acceptable if it is added to the list schema. - [ ] The stale `updated_at` comment in `CampaignPlanning.jsx` is corrected.