Edit a generated note, without losing what the model wrote - #446
Open
Optic00 wants to merge 24 commits into
Open
Edit a generated note, without losing what the model wrote#446Optic00 wants to merge 24 commits into
Optic00 wants to merge 24 commits into
Conversation
…nup path The previous test's forced failure (a toString() that throws) happened during fs's argument validation, before the temp file was ever created, so the directory-listing assertion passed vacuously - it would have passed even with the cleanup unlinkSync deleted. Mock fs.renameSync to throw instead, so the failure happens after the temp file genuinely exists on disk and the real cleanup path runs.
- noteSnapshotPath now throws for a path not ending in _summary.md instead of silently returning it unchanged, which previously let write calls clobber the note file itself. - captureSnapshot now bases its write decision on file existence, not on whether readSnapshot can interpret the content, so a sidecar written by a newer app version is never mistaken for absent and overwritten. - markEdited documents its single-writer assumption, since a later task adds a second (Python) writer to this sidecar. - markEdited's no-op test now also asserts no sidecar file is created.
Moving noteSnapshotPath's validation into readSnapshot's try block in the previous round fixed the write-side clobber risk but broke the read-side promise that a malformed path never throws when opening a note. Reads now resolve a malformed path the same way as a missing or corrupt sidecar file (return null); writes (captureSnapshot, markEdited) still throw, since markEdited now derives its path directly instead of only through readSnapshot.
Anchor the sidecar path derivation on the end of the string (raising, caught by the writer's own best-effort guard, on a path that does not end in _summary.md) instead of an unanchored str.replace() that could silently overwrite the note itself. Use the shared _atomic_write_json writer instead of a plain write_text() so a crash mid-write cannot leave a torn sidecar that permanently loses a note's diff base. Read the note back from disk and parse it with _parse_meeting_markdown (the mirror of app/main.js's parseMeetingMarkdown) instead of reusing _parse_streamed_markdown's output, so the snapshot agrees with what the note editor itself reads back from the same file. Drop the now-unused parsed argument and the two dead parsed assignments this leaves behind. Add reprocess-level coverage that the sidecar actually gets written, and a unit test for the anchored-path guard.
The note stays a document until the user clicks Edit; Save or Cancel leaves edit mode, so the boundary between the model's output and the user's corrections stays visible. - NoteEditor owns the draft, sends only changed sections as snake_case patch keys, and mirrors main's markdown-heading gate so a refusal shows next to the field instead of arriving as a failed write. - List rows and topic titles are single-line inputs, and a line break in any of them is refused client-side: renderBulletList would drop the remainder of a key point or action item, and a break in a topic title slides text into the analysis and desynchronises the JS and Python parsers. - A failed save keeps edit mode and the typing. - A summary chunk arriving while the editor is open is held rather than swapping the note out mid-edit; the view toggle and Generate notes are locked for the same reason.
…e way out One measure and one remove-button gutter on the editor body instead of a font-relative 64ch per field, which resolved to three different pixel widths and three ragged right edges once the fields had borders. The Home button now asks before discarding an unsaved draft, the save error is pinned inside the sticky bar with the button that produced it, and a rejected value names and highlights the row it is in. The reprocess-failure banner's Generate-notes button is locked while editing, matching the header button.
Every path that rebuilds a note from the transcript now asks first when the user has edited it, naming the sections in the words the note editor uses. get-meeting reads the `_original.json` sidecar and returns `edited_fields` on the meeting, via a new editedFieldNames() reader that keeps readSnapshot's never-throws contract: a missing, corrupt, future-version or malformed sidecar all read as "no edits", so the confirm cannot fire on a note with nothing to lose. Entries are sanitized because the sidecar is an ordinary file on disk and its contents end up rendered in the dialog. The guard sits on startReprocess, the shared entry point behind the header CTA, the reprocess-failure banner and the floating Generate-notes bar (which lives in a different component tree). Re-transcribe rewrites the note too, so its existing confirm gains the same warning rather than a second dialog on top.
…ection Collapse embedded newlines in each participant name and action item to a single space before joining, so one entry can never inject a blank line followed by text shaped like a new section header. Also fix two em dashes in newly authored prose and correct the query prompt's stale description of what the context contains.
… T2 gaps Add note-edit.t1.spec.ts (renderer-only, mock IPC): read-only-until-Edit, Cancel discards without calling the bridge, Save sends only the changed field, and the regenerate confirm gates on a real edit. Extend the update-meeting mock to overlay the four structural note fields and accumulate edited_fields like the real sidecar, and to record every call for test introspection (window.stenoai is read-only from the renderer, so recording lives in the mock and is read back via app.evaluate). Extend the existing T2 specs with the two properties the task named as missing: note-editing.t2 now asserts the Transcript section is byte-identical (modulo the documented single-trailing-newline normalization) across an edit to Summary/Action Items, and note-regenerate-guard.t2 now asserts the _original.json sidecar holds the pre-edit values with capture: 'first_edit' provenance.
…n editor leave Deleting a meeting unlinked its summary, transcript, recording and reports sidecar but not <stem>_original.json, which this branch writes for every generated note. That file holds the summary, key points, action items, discussion areas and attendee names, and no UI ever shows it, so a committed delete left the meeting readable on disk indefinitely. It now derives from the stem in the same block as the reports sidecar, so it survives the undo window and goes at commit like everything else. A rebuild could also still start over an open note editor. The header CTA and the retry banner were locked while editing, but the floating Generate-notes bar (which calls the detail's published start) and the re-transcribe menu item were not, and a note edited for the first time has no edited_fields yet, so no confirm appeared either. The stream is suppressed while editing, so the rebuild was invisible and the next Save wrote the pre-regeneration draft over the freshly generated note. Moving the check into rebuildInFlight closes all three paths at once, since startReprocess is the single funnel. Also in this pass: - log the two snapshot-bookkeeping paths that decline silently, so an unreadable sidecar stops being an invisible dead end - correct the single-writer comment, false since Python became the second writer of this sidecar on this same branch - write down once which writer may overwrite the sidecar and why, and point the Python writer's docstring at it - say in the regenerate and re-transcribe confirms that the edited version is kept in the note view menu, which stenolabs#249 has made true - number the validation message from the row the user sees, not the row in the patch - collapse newlines in the global chat corpus' key points and action items, as the single-meeting builder already does
chat_global_streaming reads legacy .json notes with a raw json.load, so a participant, key point or action item can arrive as a dict. _single_line does text.split() and needs a str, so such an entry raised AttributeError out of the corpus loop, which has no try of its own: the renderer got no CHAT_STREAM_ERROR marker, and one such note broke chat over every note. Coerce each entry with _item_text from src/reports.py, the helper the report builder and the note view already use for these shapes, so a dict renders as its text rather than a dict repr. Also correct the comment that claimed key-point parity with _build_meeting_chat_context_parts, which collapses only participants and action items.
…ten file The frontmatter guard only covered structural edits, so a user_notes patch on a note whose frontmatter is missing or unclosed fell through the whole body-writing branch, rewrote the file unchanged and returned success. The My-notes editor autosaves, so this discarded typed text with no user action. Key the guard off whether the body was reachable at all rather than off which generated sections were recorded, which leaves a notes-only save on a well-formed note untouched. Also correct note-sections.js: joinSections trims the body tail, so trailing whitespace on the last section's final line is not preserved. The trim stays (it stops blank lines accreting); the claim that every other byte survives does not.
The dialog said edits "are replaced" then, after a hyphen, "kept" - readable only on a second pass - and pointed at "the note view menu", a testid no user sees. Now it says once that the edited version stays available as "Standard" with a timestamp, in the menu next to the Summary switcher.
…ribe The re-transcribe confirmation had the same defect just fixed in the regenerate dialog: "replaced, but kept" contradicted itself in one breath, and pointed at "the note view menu", a testid no user sees. Now it states once that the edited version stays available as "Standard" with a timestamp, in the menu next to Summary.
Two conflicts. app/package.json: main's stenolabs#440 appended update-error-copy.test.js to test:unit while this branch inserted note-sections/atomic-write/note-snapshot. Both kept - the lists are additive and all four files exist. simple_recorder.py, three times: main's stenolabs#444 replaced summary_path.write_text(...) with _atomic_write_text(...), and this branch appends _write_original_snapshot(summary_path) after each note write. Resolved as the atomic write FOLLOWED BY the snapshot, at all three sites. The order is load-bearing in both directions: _write_original_snapshot reads the note back off disk to derive the editor's diff base, so it cannot run first, and taking this branch's write_text over the atomic one would have quietly undone stenolabs#444 on exactly the file it was filed for. Their combination is strictly better than either alone: the snapshot now reads back a note that was replaced by a rename, so it can no longer capture a torn or half-written file. Neither side introduces a lock, so nothing changes about concurrent writers between the write and the read-back. The write sites correspond one-to-one across the two branches, so stenolabs#444 left no note write without a snapshot and this branch left none unatomic. Tests: python -m unittest discover tests - 526 passed, 8 skipped. Renderer typecheck clean, 167 unit tests, T1 e2e suite 63 passed. ruff is 27 findings against main's 29, all pre-existing.
One conflict, in MeetingDetail.tsx: adjacent import lines - main's stenolabs#442 added useAutoSummarizeSetting, this branch added NoteEditor. Both kept.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Edit a generated note, without losing what the model wrote
A generated note has been read-only since the app shipped. When the model misspells a name, mishears a company, or gets a detail wrong, there is no way to correct it.
This makes the note editable section by section, and makes that safe: the model's original output is kept, and every path that would rebuild the note asks first.
What it does
Editing. An Edit affordance in the Summary tab turns the note into a form: the body text becomes a textarea, key points and action items become editable lists with add and remove, and Key Topics gets its title and analysis. Save writes only what changed. Cancel discards. The note stays a document until you ask to edit it, which is deliberate - it is read far more often than it is written, and keeping the boundary between the model's output and your corrections visible is the point.
Nothing gets silently discarded. The model's own output is snapshotted to a
<stem>_original.jsonsidecar at generation time. Regenerating, reprocessing or re-transcribing a note you have edited now names the sections it would replace and asks. Leaving the screen with an unsaved draft asks too.The chat can see your corrections. The single-meeting chat context was missing participants and action items; the cross-note chat was missing participants. Both now carry them, so a name you fixed is a name the chat knows.
How it is built
app/note-sections.js- pure, section-scoped transforms over the note body. Each replaces exactly one##section and leaves every other byte untouched. Duplicate headings resolve last-wins, matchingparseMeetingMarkdown, because a note whose model output repeated a heading would otherwise lose the edit on the next read.app/atomic-write.js- temp file plus rename, extracted from the pattern three handlers inmain.jsalready used inline. The markdown branch ofupdate-meetingpreviously ended in a barewriteFileSync.app/note-snapshot.js- the sidecar: the original fields, anedited_fieldslist, and the provenance of the capture. Reads never throw and returnnullfor anything unusable, because opening a note must never fail because of its sidecar. Writes throw on a path they cannot derive from, because the alternative is overwriting the note.update-meetinggained the four content fields on markdown notes. It rejects a value that would forge a section boundary, rejects a line break inside a list entry, writes atomically, and snapshots lazily for notes that predate this change - accurate, because those notes have never been editable, so their current content is the model's output.simple_recorder.pywrites the same sidecar whenever a note is generated, reading the note back and parsing it with_parse_meeting_markdownso both sides capture the same thing rather than two differently normalised versions of it.Two things worth a reviewer's attention
The heading gate scans normalised text, not raw. Both parsers run
normalizeMarkdownForParsingbefore splitting on##, and that transform rewrites a reasoning close-tag glued to a heading:</think>## Summarybecomes a real heading at read time. A gate anchored on^would pass it. The failure was not adversarial - the normaliser exists because models emit exactly that shape, so pasting model output into a field triggered it, and the forged heading then won under last-wins parsing and made the real summary read as empty, unrecoverably through the UI.Deleting a meeting now removes the sidecar too. It is stem-derived like
<stem>_reports.jsonand follows the same unlink-only-at-commit semantics, so an undo restores a complete meeting. Without this, a deleted meeting would have left a readable copy of its summary, action items and attendee names on disk.Why the diff touches one thing that has nothing to do with editing
update-meeting's markdown path could report a successful save while writing nothing. A.mdnote whose frontmatter is missing or unclosed skipped the branch that rewrites the body, wrote the file back unchanged, and returnedsuccess: true. The My-notes autosave therefore told the user it had saved and discarded what they typed.That is pre-existing and predates this branch. It is fixed here because this branch added the same guard for the note's structural fields and left the sibling case half-done, in the same handler, one condition apart. Leaving it would have meant shipping a handler where one kind of edit fails loudly and another fails silently.
Worth knowing how it surfaced: it was found by a review from a different model family, after ten task-scoped reviews, a whole-branch review and a fix round had all closed clean.
What the chat change does and does not send
The single-meeting chat context gained participants and action items; the cross-note context gained participants.
Action items now travel where they did not before. That is not a new class of data - the summary of the same meeting already went, and action items are part of it.
Participants currently travel as an empty list. The markdown prompt never asks the model for a
## Participantssection (src/summarizer.py), so no note the current pipeline produces has one. Checked against a real install of 21 notes, 16 markdown and 5 legacy JSON: none had the field populated. The wiring is in place; the field is not yet filled by anything.That matters for a later change, not this one. Once participants are populated - by calendar prefill or by the user typing them - real names will reach the provider on every single-meeting question, and for a cross-note question, names from every note in the corpus. On the local provider that stays on the machine; on a cloud or org provider it is an expansion of what leaves it, and it deserves a line in the release notes at that point.
Email addresses never reach a model in either case. Participants are a list of names, and the summariser is never asked to produce them.
Testing
ruffdrops from 29 findings to 27 with an identical rule breakdown - two dead assignments were removed, no new class.t2-macos/t2-windowslanes.Known and deliberately left
DetailContentis keyed on the summary file.