From 2a4269d496df884edc017bea006abb85869b1d91 Mon Sep 17 00:00:00 2001 From: Daniel Date: Fri, 24 Apr 2026 13:22:26 +0200 Subject: [PATCH] fix(notes): 5 bugs found in post-commit review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. flushAutosave dropped brand-new unsaved notes. The guard was `if (_dirty && _activeId != null)` but _activeId is null until the first POST succeeds. So tap-New-Note → type → tap- Back went through flushAutosave → skipped → closeToList → lost the typed content. saveNote() already handles isNew via POST; the guard only needs to check _dirty. 2. beforeunload sendBeacon used wrong HTTP method. navigator.sendBeacon is POST-only by spec, but my code used it to target PUT /api/notes/:id. Beacons silently hit a 404 route and the note didn't save. For brand-new notes the URL resolved to /api/notes/undefined. Replaced with fetch({ keepalive: true }) which browsers queue + ship even across unload, supports PUT, and handles POST for new notes correctly. 3. Mobile: empty-state card stacked below the sidebar in list view. The mobile media-query hid .notes-reader + .notes-editor when data-view="list" but didn't include .notes-empty-state, so the big "new note" card appeared below the list on phones. Added the missing selector. 4. Delete fallback used window.confirm. The `else if (window.confirm(...))` branch in deleteActive was a rule violation even as a fallback — feedback_no_native_dialogs says no native dialogs anywhere in frontend. Dropped it; showConfirm is loaded by app.js before any tab activates so the fallback path is unreachable in practice. Also upgraded the confirm call to use the danger + confirmText options so the modal renders red "Delete" button instead of neutral "Confirm". 5. Recorder kept running after switching tabs or notes. Dictating, then tapping another clinical-tools tab (or opening another note) left the MediaRecorder active and the mic stream open — wasted battery, privacy surprise. Added stopRecording (silent=true) to the non-notes branch of tabChanged, to startCreate, and to openReader so any in-flight recording cancels cleanly when the user moves on. Minor: • Dead "'Saving…' : 'Saving…'" ternary in saveNote simplified. SW cache bumped to pedscribe-v12-notes4. --- public/css/styles.css | 6 +++- public/js/notes.js | 64 +++++++++++++++++++++++++++++-------------- public/sw.js | 2 +- 3 files changed, 50 insertions(+), 22 deletions(-) diff --git a/public/css/styles.css b/public/css/styles.css index 79def95..f7faf6b 100644 --- a/public/css/styles.css +++ b/public/css/styles.css @@ -985,9 +985,13 @@ textarea.full-input{resize:vertical;} .notes-layout{grid-template-columns:1fr;gap:0;} .notes-sidebar,.notes-reader,.notes-editor,.notes-empty-state{max-height:none;} - /* Show only the pane matching data-view; hide the others */ + /* Show only the pane matching data-view; hide the others. The empty- + state card is purely a desktop affordance ("nothing picked yet, + pick from the sidebar"); on mobile the sidebar IS the list, so + we hide empty-state whenever the sidebar is showing. */ .notes-layout[data-view="list"] .notes-reader, .notes-layout[data-view="list"] .notes-editor, + .notes-layout[data-view="list"] .notes-empty-state, .notes-layout[data-view="reader"] .notes-sidebar, .notes-layout[data-view="reader"] .notes-editor, .notes-layout[data-view="reader"] .notes-empty-state, diff --git a/public/js/notes.js b/public/js/notes.js index 05e8cc0..7a1bd9f 100644 --- a/public/js/notes.js +++ b/public/js/notes.js @@ -39,10 +39,16 @@ // Wait for the Notes tab to be activated — the component HTML is // lazy-loaded, so elements don't exist until the user clicks the tab. document.addEventListener('tabChanged', function(e) { - if (e.detail.tab !== 'notes') return; - if (_inited) { refreshList(); return; } - _inited = true; - init(); + if (e.detail.tab === 'notes') { + if (_inited) { refreshList(); return; } + _inited = true; + init(); + return; + } + // Leaving the Notes tab — cancel any in-flight recording so the + // mic doesn't stay live + the recorder doesn't keep buffering. + if (_recActive) stopRecording(true); + flushAutosave(); }); function $(id) { return document.getElementById(id); } @@ -111,16 +117,27 @@ } }); - // Persist dirty state on tab close / page unload — best-effort final flush + // Persist dirty state on tab close / page unload — best-effort final flush. + // navigator.sendBeacon is POST-only so it can't drive a PUT update; fetch + // with keepalive:true is the right primitive here (works for both POST + // for new notes and PUT for existing). window.addEventListener('beforeunload', function() { - if (_dirty && _activeId != null) { - // Use sendBeacon for a guaranteed best-effort send - try { - var payload = JSON.stringify({ title: getTitle(), body: getBody() }); - var headers = { type: 'application/json' }; - navigator.sendBeacon && navigator.sendBeacon('/api/notes/' + _activeId, new Blob([payload], headers)); - } catch (e) {} - } + if (!_dirty) return; + var title = getTitle(); + if (!title) return; + var body = getBody(); + var isNew = _activeId == null; + var url = isNew ? '/api/notes' : '/api/notes/' + _activeId; + var method = isNew ? 'POST' : 'PUT'; + try { + fetch(url, { + method: method, + keepalive: true, + credentials: 'include', + headers: Object.assign({ 'Content-Type': 'application/json' }, getAuthHeaders()), + body: JSON.stringify({ title: title, body: body }), + }); + } catch (e) {} }); refreshList(); @@ -209,6 +226,7 @@ // ── Reader mode (default after opening or saving) ───────── function openReader(id) { flushAutosave(); + if (_recActive) stopRecording(true); var note = _notes.find(function(n) { return n.id === id; }); if (!note) return; _activeId = id; @@ -230,6 +248,7 @@ // ── Editor mode (create new / edit existing) ────────────── function startCreate() { flushAutosave(); + if (_recActive) stopRecording(true); _activeId = null; _dirty = false; $('note-title').value = ''; @@ -281,9 +300,11 @@ function flushAutosave() { if (_autosaveTimer) { clearTimeout(_autosaveTimer); _autosaveTimer = null; } - if (_dirty && _activeId != null) { - saveNote({ source: 'flush' }); - } + // Flush dirty state regardless of whether the note is new or existing. + // saveNote() handles both paths (POST for new, PUT for existing), so + // gating on _activeId here silently dropped the very first flush for + // a brand-new note — e.g. tap New note → type → tap Back. + if (_dirty) saveNote({ source: 'flush' }); } function saveNote(opts) { @@ -313,7 +334,7 @@ var method = isNew ? 'POST' : 'PUT'; _autosaveInFlight = true; - updateStatus(opts.source === 'auto' ? 'Saving…' : 'Saving…', 'saving'); + updateStatus('Saving…', 'saving'); fetch(url, { method: method, @@ -381,10 +402,13 @@ .catch(function(err) { updateStatus(err.message || 'Delete failed', 'err'); }); }; + // showConfirm is loaded by app.js before any tab activates, so the + // "if missing" fallback used to be a native window.confirm() — + // dropped here because Daniel's rule is no native dialogs anywhere + // in the frontend. If showConfirm is somehow unavailable, the click + // silently no-ops; the user can try again after a reload. if (typeof showConfirm === 'function') { - showConfirm('Delete "' + label + '"? This cannot be undone.', doDelete); - } else if (window.confirm('Delete "' + label + '"? This cannot be undone.')) { - doDelete(); + showConfirm('Delete "' + label + '"?', doDelete, { danger: true, confirmText: 'Delete' }); } } diff --git a/public/sw.js b/public/sw.js index 0a20185..b983a73 100644 --- a/public/sw.js +++ b/public/sw.js @@ -4,7 +4,7 @@ // API calls always fresh (critical for medical data accuracy) // ============================================================ -var CACHE_NAME = 'pedscribe-v12-notes3'; +var CACHE_NAME = 'pedscribe-v12-notes4'; var SHELL_ASSETS = [ '/', '/index.html',