diff --git a/app/atomic-write.js b/app/atomic-write.js new file mode 100644 index 00000000..95a1b3cb --- /dev/null +++ b/app/atomic-write.js @@ -0,0 +1,26 @@ +const fs = require('fs'); +const path = require('path'); +const crypto = require('crypto'); + +// Write via a temp file in the SAME directory, then rename. rename(2) is atomic +// within a filesystem, so a reader either sees the old file or the new one and +// never a truncated one. The temp name is randomised so two concurrent writers +// cannot collide, and it is dot-prefixed so a directory listing stays clean. +function writeFileAtomicSync(targetPath, data) { + const dir = path.dirname(targetPath); + const base = path.basename(targetPath); + const tmpPath = path.join(dir, `.${base}.${crypto.randomBytes(6).toString('hex')}.tmp`); + try { + fs.writeFileSync(tmpPath, data, 'utf8'); + fs.renameSync(tmpPath, targetPath); + } catch (err) { + try { + fs.unlinkSync(tmpPath); + } catch (_) { + // The temp file may never have been created. Nothing to clean up. + } + throw err; + } +} + +module.exports = { writeFileAtomicSync }; diff --git a/app/atomic-write.test.js b/app/atomic-write.test.js new file mode 100644 index 00000000..2c62b82e --- /dev/null +++ b/app/atomic-write.test.js @@ -0,0 +1,50 @@ +const { test } = require('node:test'); +const assert = require('node:assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +const { writeFileAtomicSync } = require('./atomic-write'); + +function tmpFile(contents) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'atomic-write-')); + const file = path.join(dir, 'note.md'); + fs.writeFileSync(file, contents, 'utf8'); + return file; +} + +test('writeFileAtomicSync replaces the file contents', () => { + const file = tmpFile('old'); + writeFileAtomicSync(file, 'new'); + assert.strictEqual(fs.readFileSync(file, 'utf8'), 'new'); +}); + +test('writeFileAtomicSync creates a file that does not exist yet', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'atomic-write-')); + const file = path.join(dir, 'fresh.md'); + writeFileAtomicSync(file, 'hello'); + assert.strictEqual(fs.readFileSync(file, 'utf8'), 'hello'); +}); + +test('a failed write leaves the original intact and removes the temp file', (t) => { + const file = tmpFile('original'); + const dir = path.dirname(file); + const before = fs.readdirSync(dir); + + // Force the failure AFTER the temp file has genuinely been written to disk, + // by making the rename step itself throw. This is the real window the + // atomic-write guarantee protects: the temp file exists, the rename fails, + // and the catch block's cleanup must remove it without touching the + // original. Using node:test's built-in mock keeps this dependency-free and + // restores fs.renameSync automatically once the test ends. + t.mock.method(fs, 'renameSync', () => { + throw new Error('boom'); + }); + + assert.throws(() => { + writeFileAtomicSync(file, 'new'); + }); + + assert.strictEqual(fs.readFileSync(file, 'utf8'), 'original'); + assert.deepStrictEqual(fs.readdirSync(dir).sort(), before.sort()); +}); diff --git a/app/e2e-mock-ipc.js b/app/e2e-mock-ipc.js index 1ed12fb5..b63d0532 100644 --- a/app/e2e-mock-ipc.js +++ b/app/e2e-mock-ipc.js @@ -203,6 +203,20 @@ function install({ ipcMain }) { // note file; the T1 seeds are consts, so we overlay here instead. const meetingOverlay = {}; + // Every update-meeting call, verbatim, so a T1 spec can assert the bridge + // was invoked exactly once with exactly the fields it expects (note-edit.t1). + // Read via ElectronApplication.evaluate (the main process), not through + // window.stenoai in the page: contextBridge's exposed API is read-only from + // the renderer's world, so monkey-patching window.stenoai.meetings.update in + // page.evaluate silently no-ops instead of recording anything. + global.__stenoaiE2eUpdateMeetingCalls = []; + + // Every reprocess-meeting call, same recording trick and the same reason. A + // spec that has to prove a rebuild did NOT start cannot assert on the UI + // (with the editor open the streaming view is suppressed on purpose), so the + // only honest evidence is that the IPC was never reached. + global.__stenoaiE2eReprocessCalls = []; + // In-flight soft-deletes (#234), id → the deleted meeting. Mirrors main's // pendingDelete map just far enough for undo to hand the row back. const pendingDeletes = {}; @@ -462,15 +476,45 @@ function install({ ipcMain }) { // permissive default hand the toast an undefined array to map over. 'list-pending-deletes': async () => ({ success: true, pending: [] }), - // My notes autosave: persist the overlay so a follow-up get-meeting sees - // the edit (mirrors the real update-meeting body-section upsert). + // My notes autosave AND the note editor (D9) share this one handler, like + // the real update-meeting IPC. `user_notes` overlays the My notes tab; the + // four structural note fields overlay the Standard note AND accumulate + // into `edited_fields`, mirroring app/note-snapshot.js's markEdited so the + // regenerate guard (which reads meeting.edited_fields) sees the same shape + // under mock IPC that it would from the real sidecar. + // Recorded, then answered exactly the way the permissive unknown-channel + // default did (`{ success: true }`, no events): the renderer stays in its + // "analyzing" state, which is what the floating-bar T1 already relies on. + 'reprocess-meeting': async (_event, summaryFile, regenerateTitle, sessionName) => { + global.__stenoaiE2eReprocessCalls.push({ summaryFile, regenerateTitle, sessionName }); + return { success: true }; + }, + 'update-meeting': async (_event, summaryFile, patch) => { + global.__stenoaiE2eUpdateMeetingCalls.push({ summaryFile, patch }); if (patch && typeof patch.user_notes === 'string') { meetingOverlay[summaryFile] = { ...(meetingOverlay[summaryFile] || {}), user_notes: patch.user_notes, }; } + const changed = []; + for (const key of ['summary', 'key_points', 'action_items', 'discussion_areas']) { + if (patch && patch[key] !== undefined) { + meetingOverlay[summaryFile] = { + ...(meetingOverlay[summaryFile] || {}), + [key]: patch[key], + }; + changed.push(key); + } + } + if (changed.length) { + const existing = meetingOverlay[summaryFile]?.edited_fields || []; + meetingOverlay[summaryFile] = { + ...(meetingOverlay[summaryFile] || {}), + edited_fields: [...new Set([...existing, ...changed])], + }; + } return { success: true, message: 'ok' }; }, diff --git a/app/main.js b/app/main.js index 413cb0df..fe920b86 100644 --- a/app/main.js +++ b/app/main.js @@ -63,6 +63,15 @@ const processingLog = require('./processing-log'); const { isMeetingApp, allowsDeviceLevelFallback, isMacos14Plus } = require('./meeting-detect'); const { sweepOrphanedLiveSnapshots } = require('./live-snapshot-sweep'); const { userNotesFilePath } = require('./notes-file'); +const { + setSummary, + setKeyPoints, + setActionItems, + setDiscussionAreas, + containsStructuralLine, +} = require('./note-sections'); +const { writeFileAtomicSync } = require('./atomic-write'); +const { readSnapshot, captureSnapshot, markEdited, editedFieldNames } = require('./note-snapshot'); const { makeLineReader } = require('./backend-stream'); // Pure deep-link (stenoai://) parsing/sanitizing lives in ./shortcut-url // (unit-tested). The stateful side — window creation, IPC dispatch, @@ -2832,11 +2841,17 @@ ipcMain.handle('get-meeting', async (_event, summaryFile) => { // TranscriptPanel), so we return everything parseMeetingMarkdown yields. const mdMeeting = parseMeetingMarkdown(content, realResolved); const mdSidecar = await readReportsSidecar(realResolved, allowedOutputDirs); - return { success: true, meeting: { ...mdMeeting, reports: mdSidecar.reports, active_report: mdSidecar.active_report } }; + // edited_fields comes LAST in every spread so it is always main's own + // reading of the sidecar, never a value that happened to be parsed out of + // the note file. It drives the regenerate guard, so a note that cannot + // supply one has to arrive as an empty list rather than as undefined - + // editedFieldNames guarantees that for a missing, corrupt or + // wrong-versioned sidecar alike. + return { success: true, meeting: { ...mdMeeting, reports: mdSidecar.reports, active_report: mdSidecar.active_report, edited_fields: editedFieldNames(realResolved) } }; } const jsonMeeting = JSON.parse(content); const jsonSidecar = await readReportsSidecar(realResolved, allowedOutputDirs); - return { success: true, meeting: { ...jsonMeeting, reports: jsonSidecar.reports, active_report: jsonSidecar.active_report } }; + return { success: true, meeting: { ...jsonMeeting, reports: jsonSidecar.reports, active_report: jsonSidecar.active_report, edited_fields: editedFieldNames(realResolved) } }; } catch (error) { return { success: false, error: error.message }; } @@ -3996,6 +4011,12 @@ function upsertUserNotesSection(body, notes) { } ipcMain.handle('update-meeting', async (event, summaryFilePath, updates) => { + // Which note content fields this call rewrote. Declared at handler scope, not + // inside the markdown branch, so the single write site further down can record + // the edit without guessing whether the variable exists. Only the markdown + // branch fills it: the legacy .json format has no snapshot sidecar and keeps + // its previous behaviour, so it always reports an empty list. + const changed = []; try { // Security: the renderer is untrusted, so containment-check the summary path // (symlink-safe, output/ only) and operate exclusively on the canonical @@ -4006,6 +4027,94 @@ ipcMain.handle('update-meeting', async (event, summaryFilePath, updates) => { } const { realPath } = validated; + if (!updates || typeof updates !== 'object') { + return { success: false, error: 'Invalid update payload' }; + } + + // Type-check the note content fields before anything is read or written. + // The section writers coerce with String(), so a wrong-typed value would + // not fail loudly - it would land in the note as "[object Object]", and a + // nested one would slip a forged heading past the structural scan below, + // which can only inspect strings: key_points: [['## Transcript']] holds no + // string to scan yet stringifies straight back into a heading. Rejecting + // the whole payload is both safer and kinder than writing a mangled note. + const isStringArray = (value) => + Array.isArray(value) && value.every((item) => typeof item === 'string'); + // `analysis` may be absent (or null, which is how an empty analysis can + // come back through a JSON round trip); a title is what makes a topic. + const isDiscussionAreaList = (value) => + Array.isArray(value) && + value.every( + (area) => + area !== null && + typeof area === 'object' && + !Array.isArray(area) && + typeof area.title === 'string' && + (area.analysis === undefined || area.analysis === null || typeof area.analysis === 'string'), + ); + + // A bullet-list entry is ONE line by construction: renderBulletList writes + // "- ", and both parsers keep only lines that start with "- " + // (app/main.js:2639/2647, and the Python mirror). A second line in an entry + // is therefore dropped on the next read, and the save after that rewrites + // the section from the parsed array and deletes it from the file too. The + // renderer is where such a string first becomes reachable, so refuse it + // here rather than joining or truncating it behind the user's back. + const LINE_BREAK = /[\r\n]/; + + if (updates.summary !== undefined && typeof updates.summary !== 'string') { + return { success: false, error: 'summary must be a string.' }; + } + if (updates.key_points !== undefined) { + if (!isStringArray(updates.key_points)) { + return { success: false, error: 'key_points must be an array of strings.' }; + } + if (updates.key_points.some((item) => LINE_BREAK.test(item))) { + return { success: false, error: 'A key point may not contain a line break.' }; + } + } + if (updates.action_items !== undefined) { + if (!isStringArray(updates.action_items)) { + return { success: false, error: 'action_items must be an array of strings.' }; + } + if (updates.action_items.some((item) => LINE_BREAK.test(item))) { + return { success: false, error: 'An action item may not contain a line break.' }; + } + } + if (updates.discussion_areas !== undefined && !isDiscussionAreaList(updates.discussion_areas)) { + return { + success: false, + error: 'discussion_areas must be an array of { title, analysis } objects.', + }; + } + + // The renderer is untrusted: a field containing a '## ' line would forge a + // section boundary and silently rewrite the note's structure for every + // consumer (both parsers, the clipboard export, the PDF export, org share). + // + // Scan the NORMALIZED text, because that is the grammar the parsers + // actually split on: both of them run normalizeMarkdownForParsing (and its + // Python mirror) BEFORE the '## ' split, which breaks a reasoning close-tag + // away from a heading glued to it - so "b## Summary" is a legal + // mid-line string at write time and a real heading at read time. Anchoring + // on '^' alone would let that through, and the forged heading would then + // win the parser's last-occurrence rule and blank the real section. This is + // not only an attack: the normalizer exists precisely because models emit + // that shape, so a user pasting model output hits it by accident. Tying the + // gate to the parser's own normalizer keeps the two from drifting apart. + // Reject, never strip: silently rewriting the user's text is worse. + const textCandidates = [ + updates.summary, + ...(Array.isArray(updates.key_points) ? updates.key_points : []), + ...(Array.isArray(updates.action_items) ? updates.action_items : []), + ...(Array.isArray(updates.discussion_areas) + ? updates.discussion_areas.flatMap((area) => [area && area.title, area && area.analysis]) + : []), + ].filter((value) => typeof value === 'string'); + if (textCandidates.some((value) => containsStructuralLine(normalizeMarkdownForParsing(value)))) { + return { success: false, error: 'A note field may not contain a markdown heading.' }; + } + // Read existing data if (!fs.existsSync(realPath)) { return { @@ -4043,6 +4152,24 @@ ipcMain.handle('update-meeting', async (event, summaryFilePath, updates) => { let body = raw; let updatedRaw = raw; + // Does this call touch the note's generated content (as opposed to only + // its title or the user's own notes)? Drives the snapshot capture below. + const structural = + updates.summary !== undefined || + updates.key_points !== undefined || + updates.action_items !== undefined || + updates.discussion_areas !== undefined; + + // Does it touch the note BODY at all? Every body writer - the generated + // sections AND the user's own notes - lives inside the frontmatter branch + // below, so this is what the missing-frontmatter guard has to key off. + // `structural` alone would leave the My-notes autosave (which records no + // edited field) reporting success over a file it never changed. + const bodyEdit = structural || updates.user_notes !== undefined; + // Set only where the body is actually reachable. A note whose frontmatter + // is missing or never closed leaves this false, and the guard fires. + let frontmatterParsed = false; + if (raw.startsWith('---')) { // Split with NO limit and rejoin the tail: split('---', 3) DISCARDS any // '---' in the body (markdown thematic breaks in summaries, and the @@ -4052,6 +4179,7 @@ ipcMain.handle('update-meeting', async (event, summaryFilePath, updates) => { // clearNoteProcessingFlag / parseMeetingMarkdown's split/slice(2).join. const parts = raw.split('---'); if (parts.length >= 3) { + frontmatterParsed = true; const fmText = parts[1]; body = parts.slice(2).join('---'); const lines = fmText.split('\n'); @@ -4095,11 +4223,83 @@ ipcMain.handle('update-meeting', async (event, summaryFilePath, updates) => { if (updates.user_notes !== undefined) { body = upsertUserNotesSection(body, updates.user_notes); } + + // Snapshot BEFORE the first structural edit. For a note that predates + // this feature the current content is the model's own output, because + // the note has never been editable - so a lazy capture here is still + // an accurate original. + if (structural && !readSnapshot(realPath)) { + try { + const before = parseMeetingMarkdown(raw, realPath); + captureSnapshot( + realPath, + { + summary: before.summary, + key_points: before.key_points, + action_items: before.action_items, + discussion_areas: before.discussion_areas, + participants: before.participants, + }, + 'first_edit', + ); + } catch (snapshotError) { + // note-snapshot's writes throw by design (a path it cannot derive + // a sidecar from must not be written to). That contract protects + // the note, and it must not also cost the user their edit: the + // sidecar is a diff base for later learning, the edit is the + // product. Log it and save anyway. The cost is that this note has + // no original to compare against, so a later regenerate cannot + // warn that it is about to discard edits (see the regenerate + // guard) - worth surfacing in the log, not worth refusing a save + // that would otherwise succeed. + console.error('Note snapshot capture failed (saving anyway):', snapshotError); + } + } + + if (updates.summary !== undefined) { + body = setSummary(body, updates.summary); + changed.push('summary'); + } + if (updates.discussion_areas !== undefined) { + body = setDiscussionAreas(body, updates.discussion_areas); + changed.push('discussion_areas'); + } + if (updates.key_points !== undefined) { + body = setKeyPoints(body, updates.key_points); + changed.push('key_points'); + } + if (updates.action_items !== undefined) { + body = setActionItems(body, updates.action_items); + changed.push('action_items'); + } + updatedRaw = `---${newLines.join('\n')}---${body}`; } } - fs.writeFileSync(realPath, updatedRaw, 'utf8'); + // A content edit only reaches `body` inside the frontmatter block above. + // If we get here having been asked for one while that block was skipped, + // the note had no parsable frontmatter and the edit was silently dropped + // - report that instead of returning success over a file we did not + // change. This covers My notes as well as the generated sections: it is + // the autosaving editor, so it is the one that loses text unprompted. + if (bodyEdit && !frontmatterParsed) { + return { success: false, error: 'Note has no readable frontmatter; refusing to edit it.' }; + } + + // Atomic: a crash or a full disk mid-write must not leave a truncated + // note behind, and this write can now carry the summary itself. + writeFileAtomicSync(realPath, updatedRaw); + if (changed.length) { + try { + markEdited(realPath, changed); + } catch (snapshotError) { + // Same trade as the capture above: the note is already saved, so a + // sidecar bookkeeping failure must not turn a successful save into a + // reported failure. + console.error('Note snapshot update failed (note was saved):', snapshotError); + } + } data = { session_info: { @@ -4138,7 +4338,10 @@ ipcMain.handle('update-meeting', async (event, summaryFilePath, updates) => { return { success: true, - message: 'Meeting updated successfully' + message: 'Meeting updated successfully', + // Markdown notes only: which generated sections this call rewrote. Empty + // for a title/notes-only save and for the legacy .json format. + edited_fields: changed, }; } catch (error) { console.error('Update meeting error:', error); @@ -4223,8 +4426,9 @@ ipcMain.handle('delete-meeting', async (event, meetingData) => { // --- Enumerate the ANCILLARY file set (unlinked only at commit), bound to // this note's STEM ONLY (never the renderer-supplied transcript_file / - // audio_file): the _reports.json sidecar and the stem-derived - // transcript + recording(s). Only the SUMMARY file itself is hidden. + // audio_file): the _reports.json and _original.json sidecars and + // the stem-derived transcript + recording(s). Only the SUMMARY file itself + // is hidden. // // Deliberately EXCLUDED: `_notes.txt`. That draft-notes file is // named from the renderer-controlled session title, NOT the stem, so two @@ -4232,18 +4436,27 @@ ipcMain.handle('delete-meeting', async (event, meetingData) => { // permanently unlink another note's draft. It isn't safely bindable to this // note's identity, so we leave it orphaned (the fail-safe direction). const ancillaryCandidates = []; - // Reports sidecar: _summary.{md,json} -> _reports.json. - { + // Stem-derived sidecars: _summary.{md,json} -> _. + // _reports.json - generated template reports (#249 backups included). + // _original.json - the model's own output for this note (app/note-snapshot.js + // and simple_recorder.py's _write_original_snapshot). It holds summary, + // key_points, action_items, discussion_areas AND participants, i.e. the + // substance of the meeting and who was in it. Leaving it behind would + // mean a committed delete still leaves a readable copy of the meeting on + // disk, with no UI that ever shows it - a privacy regression against the + // local-first promise, not just untidiness. Same "unlinked only at + // commit" semantics as the rest: an undo must restore a COMPLETE note. + for (const sidecarSuffix of ['_reports.json', '_original.json']) { let sidecarBase = null; for (const suf of ['_summary.md', '_summary.json']) { if (summaryBase.endsWith(suf)) { - sidecarBase = summaryBase.slice(0, -suf.length) + '_reports.json'; + sidecarBase = summaryBase.slice(0, -suf.length) + sidecarSuffix; break; } } if (!sidecarBase) { const ext = path.extname(summaryBase); - sidecarBase = summaryBase.slice(0, summaryBase.length - ext.length) + '_reports.json'; + sidecarBase = summaryBase.slice(0, summaryBase.length - ext.length) + sidecarSuffix; } ancillaryCandidates.push(path.join(outputDir, sidecarBase)); } diff --git a/app/note-sections.js b/app/note-sections.js new file mode 100644 index 00000000..6af2893b --- /dev/null +++ b/app/note-sections.js @@ -0,0 +1,173 @@ +// Pure, section-scoped transforms over the BODY of a meeting note (.md) - +// everything after the closing frontmatter '---'. Every function replaces +// exactly one '## ' section and leaves the CONTENT of every other section +// intact, with one deliberate exception: joinSections trims the tail of the +// whole body, so trailing whitespace on the last line of the LAST section is +// dropped even when that section was not the one edited. That trim is what +// stops blank lines accreting across repeated edits, and it costs nothing - +// both parsers trim each line on the way in. This is the same contract +// upsertUserNotesSection (main.js) already proves for User Notes; this module +// generalises it so the note editor can patch the other sections. +// +// The canonical order is the one simple_recorder.py writes: the model's +// markdown (Summary, Key Topics, Key Points, Action Items), then Transcript, +// then User Notes. A section that does not exist yet is inserted at its +// canonical position so the file keeps the shape a user recognises. +// +// Section headings are matched case-insensitively because parseMeetingMarkdown +// lowercases them; they are WRITTEN in canonical casing. + +const SECTION_ORDER = [ + 'Summary', + 'Key Topics', + 'Key Points', + 'Action Items', + 'Participants', + 'Transcript', + 'User Notes', +]; + +// A line that would create or close a markdown section if it reached the file. +// Leading whitespace counts: parseMeetingMarkdown trims nothing on the way in, +// but a pasted " ## Transcript" is still a user trying to forge structure. +const STRUCTURAL_LINE = /^\s*#{1,6}\s/m; + +function containsStructuralLine(text) { + return STRUCTURAL_LINE.test(String(text ?? '')); +} + +function canonicalRank(heading) { + const i = SECTION_ORDER.findIndex( + (h) => h.toLowerCase() === String(heading).trim().toLowerCase(), + ); + // An unknown section sorts last so a future section we do not know about is + // never re-ordered ahead of Transcript. + return i === -1 ? SECTION_ORDER.length : i; +} + +// Split the body into ordered blocks. The first block carries any preamble +// before the first '## ' heading and has heading === null. +function splitSections(body) { + const blocks = [{ heading: null, lines: [] }]; + for (const line of String(body ?? '').split('\n')) { + // '### Topic' does NOT match: index 2 is '#', not a space. Same rule as + // parseMeetingMarkdown, so topics stay inside their parent section. + if (line.startsWith('## ')) { + blocks.push({ heading: line.slice(3).trim(), lines: [] }); + } else { + blocks[blocks.length - 1].lines.push(line); + } + } + return blocks; +} + +function joinSections(blocks) { + const out = []; + for (const block of blocks) { + if (block.heading !== null) out.push(`## ${block.heading}`); + out.push(...block.lines); + } + // Trim the tail so repeated edits cannot accrete blank lines, then restore + // exactly one terminating newline. + return `${out.join('\n').replace(/\s+$/, '')}\n`; +} + +// The body lines of a section: one blank line after the heading, the content, +// one blank line before whatever follows. +function sectionLines(content) { + return ['', ...String(content).split('\n'), '']; +} + +// Replace (or insert, or with empty content remove) exactly one '## ' section. +// Every other section keeps its heading, its position and its text; the one +// thing not preserved is trailing whitespace at the very end of the body, which +// joinSections trims (see above). Say it precisely rather than promising +// byte-for-byte identity the trim does not deliver. +function setSection(body, heading, content) { + const blocks = splitSections(body); + const trimmed = String(content ?? '').replace(/\s+$/, ''); + const matches = []; + for (let i = 0; i < blocks.length; i += 1) { + if (blocks[i].heading !== null && blocks[i].heading.toLowerCase() === heading.toLowerCase()) { + matches.push(i); + } + } + + if (matches.length > 0) { + if (!trimmed) { + // An empty value removes the section, matching upsertUserNotesSection. + // Remove EVERY matching block, not just the last: parseMeetingMarkdown + // (main.js) walks the body and keeps overwriting sections[currentSection] + // as it goes, so it would still read a stale earlier duplicate as the + // section's content if we left one behind - resurrecting text the user + // just cleared. Splice from the highest index down so earlier removals + // don't shift the indices still queued for removal. + for (let i = matches.length - 1; i >= 0; i -= 1) { + blocks.splice(matches[i], 1); + } + return joinSections(blocks); + } + // parseMeetingMarkdown's parse loop keeps overwriting + // sections[currentSection] on every '## ' heading it walks past, so when a + // heading repeats, the LAST occurrence is the one that survives into the + // parsed sections object - not the first. Write the edit at that same + // last position, then drop every earlier duplicate, so a later read + // through the parser sees exactly the value just written here. Do not + // "simplify" this to first-wins; that reintroduces the drift class + // that bit #346 and #313. + const last = matches[matches.length - 1]; + blocks[last].lines = sectionLines(trimmed); + for (let i = matches.length - 2; i >= 0; i -= 1) { + blocks.splice(matches[i], 1); + } + return joinSections(blocks); + } + + if (!trimmed) return joinSections(blocks); + + const rank = canonicalRank(heading); + let insertAt = blocks.length; + for (let i = 0; i < blocks.length; i += 1) { + if (blocks[i].heading !== null && canonicalRank(blocks[i].heading) > rank) { + insertAt = i; + break; + } + } + blocks.splice(insertAt, 0, { heading, lines: sectionLines(trimmed) }); + return joinSections(blocks); +} + +function renderBulletList(items) { + return (Array.isArray(items) ? items : []) + .map((item) => String(item).trim()) + .filter(Boolean) + .map((item) => `- ${item}`) + .join('\n'); +} + +function renderTopics(areas) { + return (Array.isArray(areas) ? areas : []) + .map((area) => { + const title = String(area && area.title ? area.title : '').trim(); + if (!title) return ''; + const analysis = String(area && area.analysis ? area.analysis : '').trim(); + return analysis ? `### ${title}\n\n${analysis}` : `### ${title}`; + }) + .filter(Boolean) + .join('\n\n'); +} + +const setSummary = (body, text) => setSection(body, 'Summary', String(text ?? '').trim()); +const setKeyPoints = (body, items) => setSection(body, 'Key Points', renderBulletList(items)); +const setActionItems = (body, items) => setSection(body, 'Action Items', renderBulletList(items)); +const setDiscussionAreas = (body, areas) => setSection(body, 'Key Topics', renderTopics(areas)); + +module.exports = { + SECTION_ORDER, + containsStructuralLine, + setSection, + setSummary, + setKeyPoints, + setActionItems, + setDiscussionAreas, +}; diff --git a/app/note-sections.test.js b/app/note-sections.test.js new file mode 100644 index 00000000..eef9eb32 --- /dev/null +++ b/app/note-sections.test.js @@ -0,0 +1,243 @@ +const { test } = require('node:test'); +const assert = require('node:assert'); + +const { + setSection, + setSummary, + setKeyPoints, + setActionItems, + setDiscussionAreas, + containsStructuralLine, +} = require('./note-sections'); + +// A note body in the exact shape simple_recorder.py writes: the model's +// markdown, then ## Transcript, then ## User Notes. +const BODY = [ + '', + '## Summary', + '', + 'We agreed the budget.', + '', + '## Key Topics', + '', + '### Budget', + '', + 'Numbers were reviewed.', + '', + '## Key Points', + '', + '- Budget approved', + '', + '## Action Items', + '', + '- Anna sends the draft', + '', + '## Transcript', + '', + '[You] Hello.', + '', + '## User Notes', + '', + 'my own note', + '', +].join('\n'); + +test('setSummary replaces only the summary and leaves every other section byte-identical', () => { + const out = setSummary(BODY, 'We agreed the budget for Q3.'); + assert.match(out, /## Summary\n\nWe agreed the budget for Q3\.\n/); + assert.match(out, /## Transcript\n\n\[You\] Hello\.\n/); + assert.match(out, /## User Notes\n\nmy own note\n/); + assert.match(out, /### Budget\n\nNumbers were reviewed\.\n/); + assert.strictEqual(out.includes('We agreed the budget.\n'), false); +}); + +test('setKeyPoints rewrites the bullet list', () => { + const out = setKeyPoints(BODY, ['Budget approved', 'Anna owns the draft']); + assert.match(out, /## Key Points\n\n- Budget approved\n- Anna owns the draft\n/); + assert.match(out, /## Action Items\n\n- Anna sends the draft\n/); +}); + +test('setActionItems with an empty list removes the section entirely', () => { + const out = setActionItems(BODY, []); + assert.strictEqual(out.includes('## Action Items'), false); + assert.match(out, /## Key Points\n\n- Budget approved\n/); + assert.match(out, /## Transcript\n/); +}); + +test('setDiscussionAreas rewrites the ### topics under Key Topics', () => { + const out = setDiscussionAreas(BODY, [ + { title: 'Budget', analysis: 'Numbers were reviewed.' }, + { title: 'Hiring', analysis: 'Two roles open.' }, + ]); + assert.match(out, /## Key Topics\n\n### Budget\n\nNumbers were reviewed\.\n\n### Hiring\n\nTwo roles open\.\n/); +}); + +test('a topic without analysis renders as a bare heading', () => { + const out = setDiscussionAreas(BODY, [{ title: 'Budget' }]); + assert.match(out, /### Budget\n/); + assert.strictEqual(out.includes('undefined'), false); +}); + +test('a missing section is inserted at its canonical position, not appended', () => { + const withoutActions = setActionItems(BODY, []); + const out = setActionItems(withoutActions, ['Ben books the room']); + const actionsAt = out.indexOf('## Action Items'); + const keyPointsAt = out.indexOf('## Key Points'); + const transcriptAt = out.indexOf('## Transcript'); + assert.ok(keyPointsAt < actionsAt, 'Action Items must follow Key Points'); + assert.ok(actionsAt < transcriptAt, 'Action Items must precede Transcript'); +}); + +test('repeated edits do not accrete blank lines', () => { + let out = setSummary(BODY, 'One.'); + out = setSummary(out, 'Two.'); + out = setSummary(out, 'Three.'); + assert.strictEqual(/\n{3,}/.test(out), false); +}); + +// The one place "every other section is left alone" is not literally true, so +// it is pinned here rather than only described in prose. joinSections trims the +// tail of the WHOLE joined body before restoring the final newline, so trailing +// whitespace on the last line of the LAST section disappears even when that +// section was not the one edited. Keep it: it is what stops blank lines +// accreting across repeated edits, and both parsers trim each line anyway, so +// nothing downstream can tell. If this test starts failing because the trim was +// removed, check the accretion test above before "fixing" it. +test('the tail trim reaches the last section, so trailing whitespace there is not preserved', () => { + const body = '## Summary\nold\n## Transcript\nkeep \n'; + const out = setSection(body, 'Summary', 'new'); + assert.strictEqual(out, '## Summary\n\nnew\n\n## Transcript\nkeep\n'); + assert.strictEqual(out.includes('keep '), false); + + // Everything before the final line is preserved exactly, trailing whitespace + // included - the trim is a tail trim, not a per-line one. + const midBody = '## Summary\nold\n## Transcript\nkeep \nlast\n'; + const midOut = setSection(midBody, 'Summary', 'new'); + assert.ok(midOut.includes('keep \nlast\n'), 'inner trailing whitespace must survive'); +}); + +test('containsStructuralLine catches a heading a user could paste into a field', () => { + assert.strictEqual(containsStructuralLine('## Transcript'), true); + assert.strictEqual(containsStructuralLine('ok\n### Sneaky'), true); + assert.strictEqual(containsStructuralLine(' ## indented'), true); + assert.strictEqual(containsStructuralLine('a #hashtag is fine'), false); + assert.strictEqual(containsStructuralLine('C# is fine'), false); +}); + +// Mirrors parseMeetingMarkdown's section-splitting loop (main.js:2607-2620): it +// walks the body top to bottom and keeps overwriting sections[currentSection] +// on every '## ' heading it sees, so a repeated heading resolves to the LAST +// occurrence, not the first. Reimplemented here (rather than requiring +// main.js, which boots the Electron main process) purely to prove +// note-sections.js output stays parser-compatible on duplicate headings. +function readSectionsLikeParser(body) { + const sections = {}; + let currentSection = null; + let currentLines = []; + for (const line of body.split('\n')) { + if (line.startsWith('## ')) { + if (currentSection) sections[currentSection] = currentLines.join('\n').trim(); + currentSection = line.slice(3).trim().toLowerCase(); + currentLines = []; + } else { + currentLines.push(line); + } + } + if (currentSection) sections[currentSection] = currentLines.join('\n').trim(); + return sections; +} + +// A small local model repeating a '## Summary' heading is a realistic +// streamed-markdown output, and is exactly the parser-drift class that has +// bitten this project twice already (#346, #313). +const DUP_SUMMARY_BODY = [ + '', + '## Summary', + '', + 'First summary.', + '', + '## Summary', + '', + 'Second summary.', + '', + '## Key Points', + '', + '- Budget approved', + '', +].join('\n'); + +const DUP_ACTIONS_BODY = [ + '', + '## Key Points', + '', + '- Budget approved', + '', + '## Action Items', + '', + '- First action', + '', + '## Action Items', + '', + '- Second action', + '', + '## Transcript', + '', + '[You] Hello.', + '', +].join('\n'); + +const DUP_SUMMARY_WITH_NEIGHBOR_BODY = [ + '', + '## Summary', + '', + 'First summary.', + '', + '## Key Points', + '', + '- Point between', + '', + '## Summary', + '', + 'Second summary.', + '', + '## Transcript', + '', + '[You] Hello.', + '', +].join('\n'); + +test('a duplicate heading collapses to one, written at the LAST occurrence, matching parseMeetingMarkdown last-wins', () => { + const out = setSummary(DUP_SUMMARY_BODY, 'Third summary.'); + const headingCount = (out.match(/^## Summary$/gm) || []).length; + assert.strictEqual(headingCount, 1); + assert.match(out, /## Summary\n\nThird summary\.\n/); + assert.strictEqual(out.includes('First summary.'), false); + assert.strictEqual(out.includes('Second summary.'), false); +}); + +test('clearing a duplicated section removes every occurrence, not just the last', () => { + const out = setActionItems(DUP_ACTIONS_BODY, []); + assert.strictEqual(out.includes('## Action Items'), false); + assert.strictEqual(out.includes('First action'), false); + assert.strictEqual(out.includes('Second action'), false); + assert.match(out, /## Key Points\n\n- Budget approved\n/); + assert.match(out, /## Transcript\n/); +}); + +test('a duplicate-heading edit is what the parser actually reads back', () => { + const out = setSummary(DUP_SUMMARY_BODY, 'Edited summary.'); + const sections = readSectionsLikeParser(out); + assert.strictEqual(sections.summary, 'Edited summary.'); +}); + +test('the surviving block keeps the LAST occurrence position relative to its neighbours', () => { + const out = setSummary(DUP_SUMMARY_WITH_NEIGHBOR_BODY, 'Edited summary.'); + const summaryAt = out.indexOf('## Summary'); + const keyPointsAt = out.indexOf('## Key Points'); + const transcriptAt = out.indexOf('## Transcript'); + assert.ok( + keyPointsAt < summaryAt, + 'the surviving Summary must sit after Key Points, where the LAST occurrence was', + ); + assert.ok(summaryAt < transcriptAt); +}); diff --git a/app/note-snapshot.js b/app/note-snapshot.js new file mode 100644 index 00000000..3f48c70f --- /dev/null +++ b/app/note-snapshot.js @@ -0,0 +1,222 @@ +const fs = require('fs'); +const { writeFileAtomicSync } = require('./atomic-write'); + +// The model's original output for one note, kept in a sidecar rather than in +// the note itself because reprocess rebuilds the note file completely +// (simple_recorder.py). Without this file there is no diff base: a regenerate +// would silently destroy user corrections, and no later learning mechanism +// could tell a correction from the text it replaced. +const SNAPSHOT_VERSION = 1; + +// WHO MAY OVERWRITE _original.json. Two processes write this file with +// deliberately OPPOSITE rules; this is the single place that definition lives, +// and simple_recorder.py's _write_original_snapshot docstring points here. +// +// * Python, after WRITING THE NOTE (_write_original_snapshot, called from the +// pipeline and from reprocess): overwrites unconditionally. It has just +// replaced the note's entire generated content, so any snapshot on disk now +// describes text that no longer exists. Keeping it would leave a diff base +// that is wrong about every field, which is worse than none - and its +// edited_fields would go on claiming edits the regenerate already discarded, +// so the regenerate confirm would fire forever on a note with nothing left +// to lose. +// +// * JS (captureSnapshot, below): never overwrites. It only ever LAZILY +// captures a note it did not generate, so an existing file is by definition +// something it did not write and cannot interpret - possibly a newer +// version's format. Replacing it with a version-1 snapshot would destroy +// data this process does not understand. +// +// The rule in one line: only the writer that just (re)generated the note's +// content may replace its snapshot. Everyone else defers. +// +// Known consequence, accepted: downgrading the app and then regenerating a note +// replaces a future-version sidecar, because the Python writer cannot check a +// version it has never heard of. That costs a diff base for one note and no +// note content; guarding it properly needs a forward-compatible version +// negotiation in both writers, which is not worth building before a version 2 +// exists. + +function noteSnapshotPath(summaryPath) { + const str = String(summaryPath); + // String.replace returns its input unchanged when the pattern does not + // match. Without this check, a caller bug that passes a path not ending in + // "_summary.md" would silently get that same path back, and the write + // functions below would then overwrite the note itself with snapshot JSON + // instead of writing a sidecar next to it. Fail loudly here instead. + if (!str.endsWith('_summary.md')) { + throw new Error(`noteSnapshotPath: expected a path ending in "_summary.md", got: ${str}`); + } + return str.replace(/_summary\.md$/, '_original.json'); +} + +// Reads never throw; writes fail loudly. This asymmetry is deliberate, not an +// oversight: a read backs "open this note", which must never fail because of +// its sidecar, while a write backs "create/update this sidecar", where +// silently swallowing a malformed path risks the write landing on the note +// itself (see noteSnapshotPath). Do not "fix" this back into symmetry - +// noteSnapshotPath is called inside the try below (not before it) precisely +// so a malformed summaryPath is caught here and returns null, exactly like a +// missing or corrupt sidecar file. +function readSnapshot(summaryPath) { + try { + const file = noteSnapshotPath(summaryPath); + if (!fs.existsSync(file)) return null; + const parsed = JSON.parse(fs.readFileSync(file, 'utf8')); + // A snapshot from a future version is not ours to interpret. + if (!parsed || parsed.version !== SNAPSHOT_VERSION) return null; + return parsed; + } catch (_) { + // A corrupt sidecar, an unreadable file, or a malformed summaryPath must + // never break opening a note. All read as absent, which costs the + // learning signal for this note and nothing else. + return null; + } +} + +// `capture` is 'generation' when Python or main snapshots freshly generated +// output, and 'first_edit' when main snapshots a pre-existing note the moment +// the user first edits it. The second is accurate only because the note was +// never editable before this feature; later consumers should treat it as +// slightly weaker evidence. +function captureSnapshot(summaryPath, fields, capture) { + const file = noteSnapshotPath(summaryPath); + // Whether we may write depends on whether a sidecar FILE is already there, + // not on whether readSnapshot can make sense of it. readSnapshot returns + // null both for "nothing here" and for "something here we don't + // understand" (corrupt JSON, or a future version's format) - collapsing + // that distinction is correct for a reader, which just wants "no usable + // snapshot", but wrong for a writer. If we wrote whenever readSnapshot + // returned null, a sidecar from a newer app version would look absent and + // get silently replaced with a version-1 snapshot, destroying whatever the + // newer format held. So: file exists -> never write, return whatever + // readSnapshot makes of it (the valid snapshot, or null if unreadable). + // Only a genuinely absent file is safe to create. See "WHO MAY OVERWRITE" + // at the top of this file for why Python's writer is allowed to do the + // opposite. + if (fs.existsSync(file)) { + const existing = readSnapshot(summaryPath); + if (!existing) { + // The file is there but unusable (corrupt JSON, or a version we don't + // know). We decline to write, so this note has no diff base and never + // gets one: markEdited below will find nothing, edited_fields stays + // empty forever, and the regenerate confirm can never fire for it again. + // Neither this nor markEdited throws, so update-meeting's two + // console.error handlers never see it - without this line the whole + // failure is silent. Recovering (distinguishing corrupt from + // newer-version and rewriting the corrupt case) is a separate change. + console.warn( + `Note snapshot: existing sidecar is unreadable, leaving it alone. ` + + `This note keeps no record of its edits: ${file}`, + ); + } + return existing; + } + const snapshot = { + version: SNAPSHOT_VERSION, + captured_at: new Date().toISOString(), + capture: capture === 'generation' ? 'generation' : 'first_edit', + original: { + summary: fields.summary ?? '', + key_points: fields.key_points ?? [], + action_items: fields.action_items ?? [], + discussion_areas: fields.discussion_areas ?? [], + participants: fields.participants ?? [], + }, + edited_fields: [], + edited_at: null, + }; + writeFileAtomicSync(file, JSON.stringify(snapshot, null, 2)); + return snapshot; +} + +// Read-modify-write with no locking and no compare-and-swap. There is no +// single-writer property to lean on: simple_recorder.py's +// _write_original_snapshot writes this same file from the backend process +// (after a pipeline run and after every reprocess), so two processes really can +// touch it. What makes the missing lock tolerable is which interleavings are +// reachable, not that none are: +// +// * JS vs JS: impossible. Every JS caller runs synchronously inside the one +// Electron main process. +// * JS vs Python, on the SAME note: the only ways to start a rebuild while an +// editor is open now go through startReprocess (MeetingDetail.tsx), whose +// rebuildInFlight() refuses while `editing` is true, and the note is busy +// for the duration of a rebuild (isSummaryBusy). So a Save cannot normally +// land in the middle of a regenerate. +// * What that gate does NOT close: a save arriving through the IPC while a +// rebuild started BEFORE the editor opened is still running (a background +// regenerate, or a second window). Then Python's unconditional overwrite +// and this read-modify-write race, and last-writer-wins silently discards +// the loser's edited_fields/edited_at. The cost is the confirm not firing +// on a later regenerate, never note content - both writers only ever +// rewrite this sidecar, never the note. +// +// A caller that widens the concurrent path must not discover this by losing an +// edit. Closing it properly means either funnelling every write through the +// main process or adding real concurrency control (a file lock, or a +// compare-and-swap on version / edited_at) - not attempted here because the +// reachable window is narrow and costs only the diff base. +function markEdited(summaryPath, changedFields) { + // noteSnapshotPath is called directly (not only via readSnapshot below) so + // a malformed summaryPath throws here, before anything else runs. readSnapshot + // catches that same throw internally and would otherwise turn it into a + // silent null, which would make this write-path function swallow a caller + // bug instead of surfacing it - the opposite of the intended asymmetry. + const file = noteSnapshotPath(summaryPath); + const snapshot = readSnapshot(summaryPath); + if (!snapshot) { + // No usable snapshot: either captureSnapshot never got to write one, or the + // file on disk is corrupt / from a newer version. Either way this edit goes + // unrecorded and the regenerate confirm will not fire for it. Returning + // null silently is what made that invisible; the caller's catch cannot see + // it because nothing throws here. + console.warn( + `Note snapshot: no usable sidecar, so this edit is not recorded and a ` + + `later regenerate will not warn about it: ${file}`, + ); + return null; + } + const merged = new Set([...(snapshot.edited_fields || []), ...(changedFields || [])]); + snapshot.edited_fields = [...merged]; + snapshot.edited_at = new Date().toISOString(); + writeFileAtomicSync(file, JSON.stringify(snapshot, null, 2)); + return snapshot; +} + +// A field key the writers above could plausibly have recorded: lowercase ASCII +// with underscores. The sidecar is an ordinary file on disk, and its +// edited_fields ends up named in a confirm dialog, so anything that is not +// shaped like a key is dropped rather than rendered. +const FIELD_KEY = /^[a-z][a-z0-9_]{0,31}$/; +// A sane sidecar lists at most the five snapshotted sections. The cap is a +// bound on a corrupt or hostile file, not a semantic limit. +const MAX_EDITED_FIELDS = 8; + +// The sanitized edited_fields for one note, for the renderer's regenerate +// guard. Never throws and always returns an array: "no sidecar", "corrupt +// sidecar", "malformed path" and "never edited" must all reach the UI as the +// same empty list, so the guard cannot fire on a note that has no edits to +// lose. This is a reader, and it keeps readSnapshot's never-throws contract +// rather than relaxing it. +function editedFieldNames(summaryPath) { + const snapshot = readSnapshot(summaryPath); + if (!snapshot || !Array.isArray(snapshot.edited_fields)) return []; + const seen = []; + for (const field of snapshot.edited_fields) { + if (typeof field !== 'string' || !FIELD_KEY.test(field)) continue; + if (seen.includes(field)) continue; + seen.push(field); + if (seen.length >= MAX_EDITED_FIELDS) break; + } + return seen; +} + +module.exports = { + noteSnapshotPath, + readSnapshot, + captureSnapshot, + markEdited, + editedFieldNames, + SNAPSHOT_VERSION, +}; diff --git a/app/note-snapshot.test.js b/app/note-snapshot.test.js new file mode 100644 index 00000000..5f685c54 --- /dev/null +++ b/app/note-snapshot.test.js @@ -0,0 +1,216 @@ +const { test } = require('node:test'); +const assert = require('node:assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +const { + noteSnapshotPath, + readSnapshot, + captureSnapshot, + markEdited, + editedFieldNames, +} = require('./note-snapshot'); + +function tmpNote() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'note-snapshot-')); + const file = path.join(dir, 'Weekly_Sync_summary.md'); + fs.writeFileSync(file, '---\ntitle: "Weekly Sync"\n---\n\n## Summary\n\nhi\n', 'utf8'); + return file; +} + +const FIELDS = { + summary: 'We agreed the budget.', + key_points: ['Budget approved'], + action_items: ['Anna sends the draft'], + discussion_areas: [{ title: 'Budget', analysis: 'Reviewed.' }], + participants: ['Anna'], +}; + +test('noteSnapshotPath swaps the _summary.md suffix for _original.json', () => { + assert.strictEqual( + path.basename(noteSnapshotPath('/x/Weekly_Sync_summary.md')), + 'Weekly_Sync_original.json', + ); +}); + +test('noteSnapshotPath throws for a path that does not end in _summary.md', () => { + assert.throws(() => noteSnapshotPath('/x/Weekly_Sync.md'), /_summary\.md/); +}); + +test('readSnapshot returns null when no sidecar exists', () => { + assert.strictEqual(readSnapshot(tmpNote()), null); +}); + +test('captureSnapshot writes the original fields and is readable back', () => { + const note = tmpNote(); + captureSnapshot(note, FIELDS, 'generation'); + const back = readSnapshot(note); + assert.strictEqual(back.version, 1); + assert.strictEqual(back.capture, 'generation'); + assert.deepStrictEqual(back.original, FIELDS); + assert.deepStrictEqual(back.edited_fields, []); +}); + +test('captureSnapshot never overwrites an existing snapshot', () => { + const note = tmpNote(); + captureSnapshot(note, FIELDS, 'generation'); + captureSnapshot(note, { ...FIELDS, summary: 'DIFFERENT' }, 'first_edit'); + assert.strictEqual(readSnapshot(note).original.summary, 'We agreed the budget.'); + assert.strictEqual(readSnapshot(note).capture, 'generation'); +}); + +test('captureSnapshot does not clobber a sidecar written by a newer version', () => { + const note = tmpNote(); + const file = noteSnapshotPath(note); + const future = JSON.stringify({ version: 999, future: true }, null, 2); + fs.writeFileSync(file, future, 'utf8'); + const result = captureSnapshot(note, FIELDS, 'generation'); + assert.strictEqual(result, null); + assert.strictEqual(fs.readFileSync(file, 'utf8'), future); +}); + +test('markEdited accumulates field names without duplicates and stamps a time', () => { + const note = tmpNote(); + captureSnapshot(note, FIELDS, 'generation'); + markEdited(note, ['summary']); + const back = markEdited(note, ['summary', 'action_items']); + assert.deepStrictEqual(back.edited_fields.sort(), ['action_items', 'summary']); + assert.ok(back.edited_at); +}); + +test('a corrupt sidecar reads as null rather than throwing', () => { + const note = tmpNote(); + fs.writeFileSync(noteSnapshotPath(note), '{ not json', 'utf8'); + assert.strictEqual(readSnapshot(note), null); +}); + +test('markEdited on a note with no snapshot is a no-op that does not throw', () => { + const note = tmpNote(); + assert.strictEqual(markEdited(note, ['summary']), null); + assert.strictEqual(fs.existsSync(noteSnapshotPath(note)), false); +}); + +// Both declines below are silent by construction: neither function throws, so +// update-meeting's two catch blocks never fire and the user's note keeps saving +// while its edit bookkeeping is quietly dead. The log line is the only trace, +// which is exactly why it is worth pinning. +test('captureSnapshot warns when it declines to write over an unreadable sidecar', (t) => { + const warn = t.mock.method(console, 'warn', () => {}); + const note = tmpNote(); + fs.writeFileSync(noteSnapshotPath(note), '{ not json', 'utf8'); + assert.strictEqual(captureSnapshot(note, FIELDS, 'first_edit'), null); + assert.strictEqual(warn.mock.callCount(), 1); + assert.match(warn.mock.calls[0].arguments[0], /unreadable/); + assert.match(warn.mock.calls[0].arguments[0], /Weekly_Sync_original\.json/); +}); + +test('captureSnapshot does not warn when the existing sidecar is perfectly usable', (t) => { + const warn = t.mock.method(console, 'warn', () => {}); + const note = tmpNote(); + captureSnapshot(note, FIELDS, 'generation'); + assert.ok(captureSnapshot(note, FIELDS, 'first_edit')); + assert.strictEqual(warn.mock.callCount(), 0); +}); + +test('markEdited warns when there is no usable snapshot to record the edit in', (t) => { + const warn = t.mock.method(console, 'warn', () => {}); + const note = tmpNote(); + fs.writeFileSync(noteSnapshotPath(note), JSON.stringify({ version: 999 }), 'utf8'); + assert.strictEqual(markEdited(note, ['summary']), null); + assert.strictEqual(warn.mock.callCount(), 1); + assert.match(warn.mock.calls[0].arguments[0], /not recorded/); +}); + +test('markEdited does not warn on the ordinary path', (t) => { + const warn = t.mock.method(console, 'warn', () => {}); + const note = tmpNote(); + captureSnapshot(note, FIELDS, 'generation'); + assert.ok(markEdited(note, ['summary'])); + assert.strictEqual(warn.mock.callCount(), 0); +}); + +test('readSnapshot returns null for a malformed note path instead of throwing', () => { + assert.strictEqual(readSnapshot('/x/not-a-note.txt'), null); +}); + +test('captureSnapshot still throws for a malformed note path', () => { + assert.throws(() => captureSnapshot('/x/not-a-note.txt', FIELDS, 'generation'), /_summary\.md/); +}); + +test('markEdited still throws for a malformed note path', () => { + assert.throws(() => markEdited('/x/not-a-note.txt', ['summary']), /_summary\.md/); +}); + +// editedFieldNames is what get-meeting hands the renderer, so it is the input +// to the regenerate guard. Every "nothing was edited" shape has to come back as +// the SAME empty array: a guard that fires on an unedited note trains the user +// to click through it, which costs exactly the edits it exists to protect. +test('editedFieldNames returns [] when there is no sidecar', () => { + assert.deepStrictEqual(editedFieldNames(tmpNote()), []); +}); + +test('editedFieldNames returns [] for a freshly captured (unedited) snapshot', () => { + const note = tmpNote(); + captureSnapshot(note, FIELDS, 'generation'); + assert.deepStrictEqual(editedFieldNames(note), []); +}); + +test('editedFieldNames returns [] for a corrupt sidecar', () => { + const note = tmpNote(); + fs.writeFileSync(noteSnapshotPath(note), '{ not json', 'utf8'); + assert.deepStrictEqual(editedFieldNames(note), []); +}); + +test('editedFieldNames returns [] for a malformed note path instead of throwing', () => { + assert.deepStrictEqual(editedFieldNames('/x/not-a-note.txt'), []); +}); + +test('editedFieldNames reports the sections markEdited recorded', () => { + const note = tmpNote(); + captureSnapshot(note, FIELDS, 'generation'); + markEdited(note, ['summary', 'action_items']); + assert.deepStrictEqual(editedFieldNames(note).sort(), ['action_items', 'summary']); +}); + +// The sidecar is a file on disk, so its edited_fields can be anything. It ends +// up rendered in a dialog and must not carry arbitrary text there, and a +// non-array must not make the guard throw on the way to the renderer. +test('editedFieldNames drops entries that are not plain field keys', () => { + const note = tmpNote(); + captureSnapshot(note, FIELDS, 'generation'); + const file = noteSnapshotPath(note); + const snapshot = JSON.parse(fs.readFileSync(file, 'utf8')); + snapshot.edited_fields = [ + 'summary', + 'summary', + 42, + null, + { key: 'summary' }, + '', + 'a'.repeat(200), + 'action_items', + ]; + fs.writeFileSync(file, JSON.stringify(snapshot), 'utf8'); + assert.deepStrictEqual(editedFieldNames(note).sort(), ['action_items', 'summary']); +}); + +test('editedFieldNames returns [] when edited_fields is not an array', () => { + const note = tmpNote(); + captureSnapshot(note, FIELDS, 'generation'); + const file = noteSnapshotPath(note); + const snapshot = JSON.parse(fs.readFileSync(file, 'utf8')); + snapshot.edited_fields = 'summary'; + fs.writeFileSync(file, JSON.stringify(snapshot), 'utf8'); + assert.deepStrictEqual(editedFieldNames(note), []); +}); + +test('editedFieldNames caps a pathological sidecar rather than passing it through', () => { + const note = tmpNote(); + captureSnapshot(note, FIELDS, 'generation'); + const file = noteSnapshotPath(note); + const snapshot = JSON.parse(fs.readFileSync(file, 'utf8')); + snapshot.edited_fields = Array.from({ length: 5000 }, (_, i) => `field_${i}`); + fs.writeFileSync(file, JSON.stringify(snapshot), 'utf8'); + assert.strictEqual(editedFieldNames(note).length, 8); +}); diff --git a/app/package.json b/app/package.json index 904fc7bc..df54bfaf 100644 --- a/app/package.json +++ b/app/package.json @@ -19,7 +19,7 @@ "typecheck:renderer": "tsc -p renderer/tsconfig.json --noEmit", "lint:renderer": "eslint --config renderer/eslint.config.mjs renderer/src", "format:renderer": "prettier --write renderer/src", - "test:unit": "node --test processing-log.test.js meeting-detect.test.js notes-file.test.js backend-stream.test.js ipc-contract.test.js shortcut-url.test.js setup-check-parse.test.js diagnostics-forward.test.js analytics-helpers.test.js live-snapshot-sweep.test.js backend-cli.test.js debug-log.test.js teardown.test.js folders-ipc.test.js settings-ipc.test.js regen-title-busy-guard.test.js update-idle-gate.test.js update-os-gate.test.js update-error-copy.test.js && vitest run", + "test:unit": "node --test processing-log.test.js meeting-detect.test.js notes-file.test.js note-sections.test.js atomic-write.test.js note-snapshot.test.js backend-stream.test.js ipc-contract.test.js shortcut-url.test.js setup-check-parse.test.js diagnostics-forward.test.js analytics-helpers.test.js live-snapshot-sweep.test.js backend-cli.test.js debug-log.test.js teardown.test.js folders-ipc.test.js settings-ipc.test.js regen-title-busy-guard.test.js update-idle-gate.test.js update-os-gate.test.js update-error-copy.test.js && vitest run", "build": "npm run build:renderer && electron-builder", "pack:unsigned": "npm run build:renderer && electron-builder --dir --config electron-builder.ci.yml", "build-mac": "npm run build:renderer && electron-builder --mac", diff --git a/app/renderer/src/lib/ipc.ts b/app/renderer/src/lib/ipc.ts index 04f35f3b..cc43501d 100644 --- a/app/renderer/src/lib/ipc.ts +++ b/app/renderer/src/lib/ipc.ts @@ -64,6 +64,12 @@ export interface Meeting { notes?: string; reports?: Report[]; active_report?: string; + /** Sections of this note the user has edited since it was generated, read by + * main from the `_original.json` sidecar (`editedFieldNames`). Always an + * array when it comes from `get-meeting`; optional because the renderer also + * builds synthetic Meeting objects for the live recording. Drives the + * confirm before a regenerate replaces those sections. */ + edited_fields?: string[]; /** Synthetic flag set by the renderer for the in-progress recording. Never sent by backend. */ is_recording?: boolean; /** Synthetic flag set by the renderer when a recording is in the processing pipeline (post-stop, pre-summary). */ @@ -108,12 +114,20 @@ export interface CalendarEvent { color?: string; } +/** A partial update for one meeting note. Every content field below is written + * into its own `## ` section of the .md, leaving the rest of the file + * (transcript included) byte-identical; main rejects any value carrying a + * markdown heading, since that would forge a section boundary. */ export interface UpdateMeetingPatch { name?: string; summary?: string; + /** Not editable on .md notes yet (the participants editor is a later build); + * still honoured by the legacy .json format. */ participants?: unknown[]; key_points?: string[]; - action_items?: unknown[]; + action_items?: string[]; + /** `## Key Topics`: one `### title` subsection per area, analysis below it. */ + discussion_areas?: { title: string; analysis?: string }[]; /** The user's own notes (My notes tab). Upserts the `## User Notes` body * section of the .md (or the `user_notes` field of a legacy .json); an * empty string removes the section. */ @@ -384,7 +398,13 @@ export type RecordingsDirResponse = Result<{ path: string }>; export type ListMeetingsResponse = Result<{ meetings: Meeting[] }>; export type GetMeetingResponse = Result<{ meeting: Meeting }>; -export type UpdateMeetingResponse = Result<{ message: string; updatedData: Meeting }>; +export type UpdateMeetingResponse = Result<{ + message: string; + updatedData: Meeting; + /** Which generated sections the save rewrote. Markdown notes only: empty for + * a title/notes-only save and for the legacy .json format. */ + edited_fields?: string[]; +}>; // Soft-delete (#234): main hides only the summary and returns an `id` + a // MAIN-owned `deadline` (epoch ms) so the renderer can offer Undo. `message` is // set instead when there was nothing to delete (no summary / already gone). diff --git a/app/renderer/src/routes/MeetingDetail.regenerate-guard.test.tsx b/app/renderer/src/routes/MeetingDetail.regenerate-guard.test.tsx new file mode 100644 index 00000000..aab581ff --- /dev/null +++ b/app/renderer/src/routes/MeetingDetail.regenerate-guard.test.tsx @@ -0,0 +1,352 @@ +import { describe, test, expect, beforeEach, vi } from 'vitest'; +import { act, render, screen, fireEvent, within } from '@testing-library/react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { TooltipProvider } from '@/components/ui/tooltip'; +import type { Meeting } from '@/lib/ipc'; +import { streamCache, pendingTitleRegens } from '@/lib/meetingDetailState'; + +/** + * The regenerate guard, driven through the REAL MeetingDetail wiring rather + * than a standalone dialog component. + * + * Every path here rebuilds the note from the transcript, which throws away the + * user's corrections. The guard is only worth having if it fires on all of them + * (the header CTA, the retry banner, and the floating Generate-notes bar that + * MeetingDetail publishes for a stale/pending note) and only if it stays quiet + * on a note with nothing to lose. So the tests click the real controls and + * assert against the real reprocess mutation, not against a prop. + * + * Radix Dialog/Popover/Select need these; jsdom implements none of them. + */ +if (!Element.prototype.hasPointerCapture) { + Element.prototype.hasPointerCapture = () => false; +} +if (!Element.prototype.setPointerCapture) { + Element.prototype.setPointerCapture = () => {}; +} +if (!Element.prototype.releasePointerCapture) { + Element.prototype.releasePointerCapture = () => {}; +} +if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = () => {}; +} +// MeetingDetail remembers the last opened note. Node's global localStorage is +// unavailable without --localstorage-file, so give it an in-memory one. +if (!globalThis.localStorage) { + const store = new Map(); + Object.defineProperty(globalThis, 'localStorage', { + configurable: true, + value: { + getItem: (k: string) => store.get(k) ?? null, + setItem: (k: string, v: string) => void store.set(k, String(v)), + removeItem: (k: string) => void store.delete(k), + clear: () => store.clear(), + }, + }); +} + +const h = vi.hoisted(() => { + const noop = () => () => {}; + return { + meeting: null as Meeting | null, + reprocess: { mutate: vi.fn(), isPending: false }, + retranscribe: { mutate: vi.fn(), isPending: false }, + recordingAvailable: { data: false as boolean | undefined }, + publish: vi.fn(), + clear: vi.fn(), + navigate: vi.fn(), + noop, + }; +}); + +vi.mock('@/components/MeetingsShell', () => ({ + MeetingsShell: ({ children }: { children: React.ReactNode }) =>
{children}
, +})); + +vi.mock('@/hooks/useMeetings', () => ({ + meetingsKeys: { + all: ['meetings'] as const, + detail: (f: string) => ['meetings', 'detail', f] as const, + }, + useMeeting: () => ({ + data: h.meeting, + isLoading: false, + isFetching: false, + isError: false, + error: null, + }), + useReprocessMeeting: () => h.reprocess, + useRetranscribeMeeting: () => h.retranscribe, + useRecordingAvailable: () => h.recordingAvailable, + useDeleteMeeting: () => ({ mutate: vi.fn(), mutateAsync: vi.fn(), isPending: false }), + useGenerateReport: () => ({ mutate: vi.fn(), isPending: false }), + useSetActiveReport: () => ({ mutate: vi.fn() }), + useDeleteReport: () => ({ mutate: vi.fn() }), + useUpdateMeeting: () => ({ mutate: vi.fn(), mutateAsync: vi.fn(), isPending: false }), + useUpdateUserNotes: () => ({ mutate: vi.fn() }), +})); + +vi.mock('@/hooks/useTemplates', () => ({ useTemplates: () => ({ templates: [] }) })); + +vi.mock('@/hooks/useOrg', () => ({ + useOrgSession: () => ({ data: { signedIn: false } }), + useShareToOrg: () => ({ mutateAsync: vi.fn(), isPending: false }), + useOrgBackupState: () => ({ data: undefined }), + useUnshareFromOrgBySummary: () => ({ mutateAsync: vi.fn(), isPending: false }), +})); + +vi.mock('@/hooks/useFolders', () => ({ + useFolders: () => ({ data: [] }), + useAddMeetingToFolder: () => ({ mutateAsync: vi.fn() }), + useRemoveMeetingFromFolder: () => ({ mutateAsync: vi.fn() }), + useCreateFolder: () => ({ mutateAsync: vi.fn() }), +})); + +vi.mock('@/lib/askBarContext', () => ({ useActiveMeeting: () => {} })); + +vi.mock('@/lib/router', () => ({ navigate: (...a: unknown[]) => h.navigate(...a) })); + +vi.mock('@/hooks/useRecording', () => ({ + useRecording: () => ({ status: 'idle', recordingSummaryFile: null }), +})); + +vi.mock('@/hooks/reprocessBridgeStore', () => ({ + useReprocessBridge: (select: (s: unknown) => unknown) => + select({ publish: h.publish, clear: h.clear }), +})); + +vi.mock('@/lib/ipc', () => ({ + ipc: () => ({ + on: { + summaryChunk: h.noop, + summaryComplete: h.noop, + processingComplete: h.noop, + processingProgress: h.noop, + }, + meetings: { + revealFolder: vi.fn(), + exportTranscript: vi.fn(), + exportNotePdf: vi.fn(), + regenTitle: vi.fn(), + }, + }), +})); + +// Imported after the mocks so the module graph picks them up. +const { MeetingDetail } = await import('./MeetingDetail'); + +const SUMMARY_FILE = '/tmp/output/quarterly_summary.md'; + +function makeMeeting(overrides: Partial = {}): Meeting { + return { + session_info: { + name: 'Quarterly Review', + summary_file: SUMMARY_FILE, + date: '2026-07-28T10:00:00', + duration_seconds: 600, + }, + summary: 'The team agreed to ship on Friday.', + key_points: ['Ship Friday'], + action_items: ['Alice pings the vendor'], + discussion_areas: [{ title: 'Billing', analysis: 'Blocked on the vendor.' }], + participants: ['Alice'], + transcript: 'Alice: we ship Friday.', + ...overrides, + } as Meeting; +} + +function renderDetail(meeting: Meeting) { + h.meeting = meeting; + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render( + + + + + + ); +} + +const headerRegenerate = () => screen.getByRole('button', { name: 'Generate notes' }); + +/** The confirm dialog if it is on screen, else null. */ +function dialog() { + return document.querySelector('[data-confirm-dialog]'); +} + +/** ConfirmDialog's handler is async (it awaits onConfirm before clearing its + * busy flag), so the click has to be flushed inside act. */ +async function clickConfirm(name: RegExp) { + const button = within(dialog() as HTMLElement).getByRole('button', { name }); + await act(async () => { + fireEvent.click(button); + }); +} + +beforeEach(() => { + vi.clearAllMocks(); + // Module-level state that outlives a render: a previous test's "analyzing" + // entry would trip startReprocess's re-entrancy guard in the next one and + // make a broken guard look like a working one. + streamCache.clear(); + pendingTitleRegens.clear(); + h.reprocess = { mutate: vi.fn(), isPending: false }; + h.retranscribe = { mutate: vi.fn(), isPending: false }; + h.recordingAvailable = { data: false }; +}); + +describe('the regenerate guard', () => { + test('asks before regenerating a note that has edits, naming the sections', () => { + renderDetail(makeMeeting({ edited_fields: ['summary', 'action_items'] })); + fireEvent.click(headerRegenerate()); + + expect(h.reprocess.mutate).not.toHaveBeenCalled(); + const box = dialog(); + expect(box).toBeTruthy(); + // Human section names, not the raw field keys the sidecar stores. + expect(within(box as HTMLElement).getByText(/Summary/)).toBeTruthy(); + expect(within(box as HTMLElement).getByText(/Action items/)).toBeTruthy(); + expect(box!.textContent).not.toContain('action_items'); + // The edited version's fate is stated once, not "replaced" and "kept" in + // the same breath, and it names the on-screen control (the menu next to + // the Summary switcher) rather than the data-testid. + expect(box!.textContent).toContain( + 'Your edited version stays available as "Standard" with a timestamp, in the menu next to Summary.' + ); + expect(box!.textContent).not.toContain('note view menu'); + }); + + test('confirming goes ahead with the rebuild', async () => { + renderDetail(makeMeeting({ edited_fields: ['summary'] })); + fireEvent.click(headerRegenerate()); + await clickConfirm(/regenerate notes/i); + expect(h.reprocess.mutate).toHaveBeenCalledTimes(1); + expect(h.reprocess.mutate.mock.calls[0][0]).toMatchObject({ summaryFile: SUMMARY_FILE }); + }); + + test('cancelling does nothing at all', async () => { + renderDetail(makeMeeting({ edited_fields: ['summary'] })); + fireEvent.click(headerRegenerate()); + await clickConfirm(/keep my edits/i); + expect(h.reprocess.mutate).not.toHaveBeenCalled(); + expect(h.navigate).not.toHaveBeenCalled(); + }); + + // The three shapes that all mean "nothing to lose". A prompt on any of them + // teaches the user to click through the one that matters. + test.each([ + ['an empty list', [] as string[]], + ['a missing field', undefined], + ['a null field', null as unknown as string[]], + ])('regenerates immediately with %s', (_label, editedFields) => { + renderDetail(makeMeeting({ edited_fields: editedFields })); + fireEvent.click(headerRegenerate()); + expect(dialog()).toBeNull(); + expect(h.reprocess.mutate).toHaveBeenCalledTimes(1); + }); + + test('the retry banner CTA is guarded too', async () => { + renderDetail(makeMeeting({ edited_fields: ['key_points'] })); + // Drive the banner the way the user does: a failed regenerate renders it. + fireEvent.click(headerRegenerate()); + await clickConfirm(/regenerate notes/i); + const onError = h.reprocess.mutate.mock.calls[0][1].onError as () => void; + act(() => onError()); + h.reprocess.mutate.mockClear(); + + const banner = screen.getByTestId('reprocess-retry'); + fireEvent.click(within(banner).getByRole('button', { name: /generate notes/i })); + expect(h.reprocess.mutate).not.toHaveBeenCalled(); + expect(within(dialog() as HTMLElement).getByText(/Key points/)).toBeTruthy(); + }); + + // The floating GenerateNotesBar doesn't live in this tree: MeetingDetail + // publishes a `start` callback to the bridge and the bar calls it. If the + // guard sat on the click handlers instead of on the shared entry point, this + // path would regenerate with no prompt at all. + test('the published Generate-notes trigger is guarded too', () => { + renderDetail( + makeMeeting({ + edited_fields: ['summary'], + session_info: { + ...makeMeeting().session_info, + notes_stale: true, + }, + }) + ); + const published = h.publish.mock.calls.at(-1)?.[0] as { start: () => void }; + expect(published).toBeTruthy(); + act(() => published.start()); + expect(h.reprocess.mutate).not.toHaveBeenCalled(); + expect(within(dialog() as HTMLElement).getByText(/Summary/)).toBeTruthy(); + }); + + // Re-transcribe re-runs ASR and then rewrites the note, so it discards edits + // exactly like a reprocess. It already had a confirm; the guard has to make + // that confirm say what it is about to throw away rather than add a second + // dialog on top of it. + test('the re-transcribe confirm names the edited sections', () => { + h.recordingAvailable = { data: true }; + renderDetail(makeMeeting({ edited_fields: ['discussion_areas'] })); + fireEvent.click(screen.getByRole('button', { name: /more options/i })); + fireEvent.click(screen.getByTestId('retranscribe-action')); + const box = dialog() as HTMLElement; + // Human section names, not the raw field keys the sidecar stores. + expect(within(box).getByText(/Key topics/)).toBeTruthy(); + expect(box.textContent).not.toContain('discussion_areas'); + // The edited version's fate is stated once, not "replaced" and "kept" in + // the same breath, and it names the on-screen control (the menu next to + // the Summary switcher) rather than the data-testid. + expect(box.textContent).toContain( + 'Your edited version stays available as "Standard" with a timestamp, in the menu next to Summary.' + ); + expect(box.textContent).not.toContain('note view menu'); + expect(h.retranscribe.mutate).not.toHaveBeenCalled(); + }); + + test('the re-transcribe confirm stays plain when nothing was edited', () => { + h.recordingAvailable = { data: true }; + renderDetail(makeMeeting({ edited_fields: [] })); + fireEvent.click(screen.getByRole('button', { name: /more options/i })); + fireEvent.click(screen.getByTestId('retranscribe-action')); + const box = dialog() as HTMLElement; + expect(box.textContent).not.toMatch(/edited/i); + }); +}); + +describe('describeEditedSections', () => { + test('maps the stored field keys to the names the note editor uses', async () => { + const { describeEditedSections } = await import('./MeetingDetail'); + expect( + describeEditedSections(['summary', 'key_points', 'action_items', 'discussion_areas']) + ).toEqual(['Summary', 'Key topics', 'Key points', 'Action items']); + }); + + // Canonical order, not the order the sidecar happened to accumulate them in: + // the dialog should read the same way whichever section was edited first. + test('lists sections in the order the note shows them', async () => { + const { describeEditedSections } = await import('./MeetingDetail'); + expect(describeEditedSections(['action_items', 'summary'])).toEqual([ + 'Summary', + 'Action items', + ]); + }); + + // A newer app version could record a field this one has no label for. Naming + // it readably beats either printing a raw key or silently not warning. + test('humanises a field key it does not know', async () => { + const { describeEditedSections } = await import('./MeetingDetail'); + expect(describeEditedSections(['summary', 'decision_log'])).toEqual([ + 'Summary', + 'Decision log', + ]); + }); + + test('is empty for every "nothing was edited" shape', async () => { + const { describeEditedSections } = await import('./MeetingDetail'); + expect(describeEditedSections([])).toEqual([]); + expect(describeEditedSections(undefined)).toEqual([]); + expect(describeEditedSections(null as unknown as string[])).toEqual([]); + expect(describeEditedSections('summary' as unknown as string[])).toEqual([]); + }); +}); diff --git a/app/renderer/src/routes/MeetingDetail.tsx b/app/renderer/src/routes/MeetingDetail.tsx index b8ff8a1d..8c17aa20 100644 --- a/app/renderer/src/routes/MeetingDetail.tsx +++ b/app/renderer/src/routes/MeetingDetail.tsx @@ -37,6 +37,7 @@ import { useGenerateReport, useSetActiveReport, useDeleteReport, + useUpdateMeeting, useUpdateUserNotes, meetingsKeys, } from '@/hooks/useMeetings'; @@ -66,7 +67,7 @@ import { useCreateFolder, } from '@/hooks/useFolders'; import { useActiveMeeting } from '@/lib/askBarContext'; -import { ipc, type Meeting, type Report, type Template } from '@/lib/ipc'; +import { ipc, type Meeting, type Report, type Template, type UpdateMeetingPatch } from '@/lib/ipc'; import { buildTranscriptBundle, defaultExportFilename } from '@/lib/transcriptBundle'; import { buildNotesCopyText, type StructuredNoteSections } from '@/lib/notesCopy'; import { buildNotesHtml, hasNotesContent } from '@/lib/notesPdf'; @@ -78,6 +79,7 @@ import { pendingTitleRegens, streamCache, type StreamPhase } from '@/lib/meeting import { useReprocessBridge } from '@/hooks/reprocessBridgeStore'; import { useRecording } from '@/hooks/useRecording'; import { useAutoSummarizeSetting } from '@/hooks/useSettings'; +import { NoteEditor, type NoteDraft } from './NoteEditor'; const LAST_OPENED_KEY = 'steno-last-opened-meeting'; @@ -89,6 +91,49 @@ const LAST_OPENED_KEY = 'steno-last-opened-meeting'; // of truth that keeps them aligned. const EXPORT_CANCELED_ERROR = 'canceled'; +// The note-snapshot sidecar records which sections were edited by their storage +// keys (`action_items`). A confirm dialog has to name them the way the note +// editor does, in the order the note lays them out, so the user recognises what +// is about to be replaced. +const SECTION_LABELS: ReadonlyArray = [ + ['summary', 'Summary'], + ['discussion_areas', 'Key topics'], + ['key_points', 'Key points'], + ['action_items', 'Action items'], + ['participants', 'Participants'], +]; + +/** `action_items` -> `Action items`, for a key this version has no label for. + * A newer app version can record a section this one doesn't know about; naming + * it readably is better than printing a raw key, and far better than declining + * to warn at all. */ +function humaniseFieldKey(field: string): string { + const words = field.replace(/_/g, ' ').trim(); + return words ? words[0].toUpperCase() + words.slice(1) : field; +} + +/** The human names of the note sections the user has edited, in the order the + * note shows them (not the order the sidecar accumulated them). Empty for + * every "nothing was edited" shape (absent, null, empty, or not an array), + * because a confirm on a note with no edits to lose is worse than none. */ +export function describeEditedSections(editedFields: string[] | undefined | null): string[] { + if (!Array.isArray(editedFields) || editedFields.length === 0) return []; + const known = SECTION_LABELS.filter(([key]) => editedFields.includes(key)).map( + ([, label]) => label + ); + const knownKeys = new Set(SECTION_LABELS.map(([key]) => key)); + const unknown = editedFields + .filter((f) => typeof f === 'string' && !knownKeys.has(f)) + .map(humaniseFieldKey); + return [...known, ...[...new Set(unknown)]]; +} + +/** "Summary", "Summary and Action items", "Summary, Key points and Action items". */ +function formatSectionList(labels: string[]): string { + if (labels.length <= 1) return labels[0] ?? ''; + return `${labels.slice(0, -1).join(', ')} and ${labels[labels.length - 1]}`; +} + interface MeetingDetailProps { summaryFile: string; } @@ -276,6 +321,24 @@ function DetailContent({ const [reprocessFailed, setReprocessFailed] = React.useState(false); const qc = useQueryClient(); + // Note editing (D9): the generated note is a document until the user asks to + // edit it. Declared up here because the streaming listeners below have to see + // it; they're registered once per meeting, so they read it through a ref + // rather than re-subscribing on every keystroke's re-render. + const [editing, setEditing] = React.useState(false); + // Lifted out of the editor so the paths that would unmount it can ask whether + // there is anything to lose before they do. + const [noteDirty, setNoteDirty] = React.useState(false); + const [confirmLeaveEdit, setConfirmLeaveEdit] = React.useState(false); + const updateMeeting = useUpdateMeeting(); + const editingRef = React.useRef(editing); + // Layout effect, not a render-time assignment: it still lands before the + // renderer yields to the next task, so no IPC chunk can observe a stale + // value, and it keeps the ref out of the render path. + React.useLayoutEffect(() => { + editingRef.current = editing; + }, [editing]); + // Report switch: null = the structured Standard summary, otherwise the id of // a generated report in meeting.reports. Seeded from the meeting's persisted // active_report so reopening a note lands on whatever was last viewed. @@ -307,6 +370,11 @@ function DetailContent({ const sessionName = info.name; const offChunk = ipc().on.summaryChunk((e) => { if (e.summaryFile !== summaryFile) return; + // A regenerate that finishes while the user is editing must not swap the + // note out from under them. Holding the stream is the conservative choice: + // the regenerated note is still on disk and appears as soon as they leave + // edit mode. + if (editingRef.current) return; // Promote from idle too (not just analyzing): an instant-stop note's // first-ever summary streams in with no prior reprocess to seed // 'analyzing', so without idle→generating the StreamingView (gated on @@ -432,23 +500,52 @@ function DetailContent({ ); }; - const startReprocess = () => { - // Synchronous re-entrancy guard (#313 review): the floating dock button's - // disabled state arrives one commit late (published via effect to the - // reprocess bridge), so a fast double-click there could fire two - // overlapping `reprocess` jobs for the same file — main.js deliberately - // allows concurrent jobs across files and has no same-file dedupe. - // streamCache is a module-level Map written synchronously below, so it - // can't lag the way state/props can. + // Which sections of this note the user has edited since it was generated, as + // main read them from the `_original.json` sidecar. Every path that rebuilds + // the note replaces these, so each one asks first. Empty whenever there is + // nothing to lose (no sidecar, an unreadable one, or an unedited note), which + // is what keeps the confirm off notes it would only teach people to dismiss. + const editedSections = React.useMemo( + () => describeEditedSections(meeting.edited_fields), + [meeting.edited_fields] + ); + const hasNoteEdits = editedSections.length > 0; + const editedSectionsText = formatSectionList(editedSections); + const [confirmRegenerate, setConfirmRegenerate] = React.useState(false); + + // Synchronous re-entrancy guard (#313 review): the floating dock button's + // disabled state arrives one commit late (published via effect to the + // reprocess bridge), so a fast double-click there could fire two + // overlapping `reprocess` jobs for the same file - main.js deliberately + // allows concurrent jobs across files and has no same-file dedupe. + // streamCache is a module-level Map written synchronously by the starters + // below, so it can't lag the way state/props can. + // + // `editing` belongs here rather than on each click handler because the two + // rebuild paths that are NOT this component's own buttons (the floating + // GenerateNotesBar, which calls the published `start`, and the re-transcribe + // menu item) would otherwise slip through. With an open editor the stream is + // suppressed (editingRef), so a rebuild started here is invisible: Python + // rewrites the note, and the next Save patches the FRESH note with the + // PRE-regeneration draft, silently replacing content the user never saw. The + // #249 standard-backup does not rescue that - it holds the note as it was + // BEFORE the regenerate, not the generated text that was just overwritten. + const rebuildInFlight = () => { const cached = streamCache.get(summaryFile); - if ( + return ( + editing || reprocess.isPending || streamPhase !== 'idle' || cached?.phase === 'analyzing' || cached?.phase === 'generating' - ) { - return; - } + ); + }; + + const runReprocess = () => { + // Re-checked here and not only at the entry points: the confirm dialog puts + // an arbitrary amount of user time between the click and this call, and a + // background job for this note can start in that window. + if (rebuildInFlight()) return; setStreamText(''); setStreamPhase('analyzing'); setChunkProgress(null); @@ -473,23 +570,30 @@ function DetailContent({ ); }; + // The single entry point for "rebuild this note from the transcript". The + // header icon, the retry banner and the floating GenerateNotesBar all come + // through here, so the guard lives here rather than on each click handler: + // one of those three is not even in this component's tree. + const startReprocess = () => { + if (rebuildInFlight()) return; + if (hasNoteEdits) { + setConfirmRegenerate(true); + return; + } + runReprocess(); + }; + // Re-transcribe (#266): re-run ASR on the source recording with the current // settings, then re-summarise. Reuses the SAME streaming UI as reprocess — // the backend drives summary-chunk/-complete keyed by summaryFile — so we only // swap which mutation fires. Transcription is silent (no CHUNK), so the view // stays in "analyzing" until summarisation streams, matching reprocess's // pre-first-chunk state. Mirrors startReprocess's re-entrancy guard + onError. + // It rewrites the note too, so it discards edits exactly like a reprocess: + // its existing confirm below carries the warning instead of stacking a second + // dialog on top of it. const startRetranscribe = () => { - const cached = streamCache.get(summaryFile); - if ( - retranscribe.isPending || - reprocess.isPending || - streamPhase !== 'idle' || - cached?.phase === 'analyzing' || - cached?.phase === 'generating' - ) { - return; - } + if (retranscribe.isPending || rebuildInFlight()) return; setStreamText(''); setStreamPhase('analyzing'); setChunkProgress(null); @@ -635,6 +739,38 @@ function DetailContent({ }; const summary = meeting.summary?.trim(); + // Seeded from what the read-only note actually shows, reasoning stripped. + // Editing the summary must not resurrect a block the UI hides. + const noteDraft: NoteDraft = { + summary: summary ? stripReasoning(summary) : '', + keyPoints: meeting.key_points ?? [], + actionItems: asStringArray(meeting.action_items), + discussionAreas: asDiscussionAreas(meeting.discussion_areas), + }; + const closeEditor = () => { + setEditing(false); + setNoteDirty(false); + }; + // A rejected mutation (main refuses a forged heading, or the write fails) + // propagates to the editor, which keeps edit mode and the typing. + const saveNoteEdits = async (patch: UpdateMeetingPatch) => { + await updateMeeting.mutateAsync({ summaryFile, patch }); + closeEditor(); + }; + // The in-view back button is the one exit from an open editor this view owns. + // The sidebar and the command palette still unmount it without asking; that + // needs a router-level unsaved-changes hook, tracked separately. + const leaveDetail = () => { + if (editing && noteDirty) { + setConfirmLeaveEdit(true); + return; + } + navigate('/'); + }; + // Same "is there a note here at all" test the PDF export uses, plus the two + // states that are about to rewrite the note anyway. + const canEditNote = + canExportNotesPdf && !activeReport && streamPhase === 'idle' && !reprocess.isPending; const participants = asStringArray(meeting.participants); const keyPoints = meeting.key_points ?? []; const actionItems = asStringArray(meeting.action_items); @@ -743,7 +879,7 @@ function DetailContent({
+ {/* Edit the generated note (D9). Only for the Standard structured + note (a template report is generated output with no section + grammar to patch) and only while nothing else is rewriting it. */} + {tab === 'summary' && !editing && ( + + + setEditing(true)} + disabled={!canEditNote} + > + + + + Edit note + + )} {/* Disabled while a summary/report stream is on screen — the @@ -798,7 +951,14 @@ function DetailContent({ // Disable while a recording is live on THIS note — same // reason the floating CTA hides: don't summarise a // still-growing transcript out from under the recording. - disabled={reprocess.isPending || streamPhase !== 'idle' || isRecordingThisNote} + // Also off while the note editor is open: a regenerate + // would discard the edit being typed. + disabled={ + reprocess.isPending || + streamPhase !== 'idle' || + isRecordingThisNote || + editing + } > @@ -1086,6 +1253,7 @@ function DetailContent({ onDeleteReport={onDeleteReport} onGenerate={onGenerateReport} generating={generateReport.isPending} + disabled={editing} /> {tab === 'summary' && ( @@ -1112,13 +1280,23 @@ function DetailContent({ )} - {streamPhase !== 'idle' ? ( + {editing ? ( + + ) : streamPhase !== 'idle' ? ( ) : activeReport ? (
{ setRetranscribeOpen(false); startRetranscribe(); }} /> + + {/* Every rebuild path funnels through startReprocess, so this one dialog + covers the header CTA, the retry banner and the floating bar. */} + { + setConfirmRegenerate(false); + runReprocess(); + }} + /> + + { + setConfirmLeaveEdit(false); + closeEditor(); + navigate('/'); + }} + /> ); } @@ -1366,6 +1599,7 @@ function NoteViewToggle({ onDeleteReport, onGenerate, generating, + disabled = false, }: { tab: 'summary' | 'notes'; onTab: (t: 'summary' | 'notes') => void; @@ -1377,6 +1611,9 @@ function NoteViewToggle({ onDeleteReport: (reportId: string) => void; onGenerate: (templateId: string) => void; generating: boolean; + /** Locked while the note editor is open: every path out of this control + * (switching view, generating a report) would drop unsaved edits. */ + disabled?: boolean; }) { const [menuOpen, setMenuOpen] = React.useState(false); const [deleteTarget, setDeleteTarget] = React.useState(null); @@ -1404,7 +1641,7 @@ function NoteViewToggle({ role="tablist" aria-label="Note view" className="inline-flex items-stretch overflow-hidden rounded-full" - style={{ border: '1px solid var(--border-subtle)' }} + style={{ border: '1px solid var(--border-subtle)', opacity: disabled ? 0.5 : 1 }} > {/* Left — My notes */}