Fix: cancel the Notes autosave debounce on unmount - #3526
Conversation
dbc10a6 to
89ec989
Compare
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed BLOCKING -- website/src/apps/md-notebook/MdNotebookPage.tsx:288 -- failed unmount save permanently loses the pending edit |
UX Review (Fable 5, fork) — ✅ PASSAdvisory UX-level review of UX-Verdict: PASS Invisible-positive change: an edit made just before navigating away is now saved instead of silently lost; no new strings, surfaces, or interaction patterns. [UX-REVIEWED] aba1512 |
Design Review (Fable 5, fork) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Real, instrumented harm; the cancel-plus-flush shape is proportionate and the edit-loss case a bare clearTimeout would create is explicitly covered. Suggestions
[DESIGN-REVIEWED] aba1512 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSAdvisory premise-level review of Both evidence files and the base page are read; I verified the claims against First-Principles-Verdict: CONCERNS The unmount flush hand-rolls a second save path when What this change shipsIntent: stop the Notes autosave timer from firing after the page is gone (#2984) — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] aba1512 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsI've independently traced the single behavioral change — the unmount Verifying the risk paths myself against the actual code:
The candidate discovery pass reported no candidates, and my own falsification reaches the same conclusion. No AUTOSDE No findings. [OPUS-REVIEWED] aba1512 |
89ec989 to
dc87ae9
Compare
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
dc87ae9 to
6e03cc4
Compare
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
2 similar comments
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
9 similar comments
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
4 similar comments
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
🤖 Kiro Crew [operator: bolichen97#bb3ad1ca]: Triage disposition — routing this PR to human decision instead of the drive-to-green queue. This PR is a rival implementation of #2984 alongside #3101 (same author, adiarora06). #3101 is already in the drive-to-green pipeline under another operator instance and its PR-side work is complete (escalated 2026-08-25: all remaining red checks reproduce on pristine main, i.e. main-side). This PR's own description explicitly defers to "maintainer discretion on which lands" — that pick is a human decision, and driving both rivals green would duplicate work and produce two competing merges for one issue. The substantive difference (per this PR's description): #3101 cancels the debounce timer on unmount; this PR additionally fires one best-effort direct Maintainer options:
Labels: removing |
|
Closing as a duplicate of #3101, which is the same author's earlier attempt at the same fix for #2984. Why #3101 is the one we keep:
Both branches got the important call right, and it is worth stating so it does not get "simplified" away later: a bare |
Pull request was closed
Summary
Fixes #2984. Note: PR #3101 already targets this issue — opening this as an independent implementation per maintainer discretion on which lands.
MdNotebookPagearms a 1000mswindow.setTimeout(SAVE_DEBOUNCE_MS) on every edit, callingflushSave()when it fires. Nothing cancelled that timer on unmount: React Router navigation (or testing-library'sunmount()) removes the page but the timer keeps running, and its callback —flushSave, which callssetDirty/setError/setFileConflict— fires later against a component that no longer exists.Proven by instrumentation on PR #2964 (fixes #2914): a stack capture showed a second
saveNotecall arriving vialistOnTimeoutfrom a prior test's unmounted component — a cross-test race with a 1/5–1/15 repro rate under load-dependent timing drift.Fix
An unmount-only effect:
notesApi.saveNote(using the current vault/path/content/mtime refs) instead of losing the edit.This deliberately does not go through
flushSave— that function sets React state (setDirty/setError/setFileConflict), which can no longer safely run once the component is unmounting. Errors are swallowed: there is no component left to show them to, and the note simply stays dirty for whichever surface opens it next to pick up.Test plan
New tests in
MdNotebookPageCoverage.test.tsx:SAVE_DEBOUNCE_MSpost-unmount produces no secondsaveNotecall).npx vitest run src/test/MdNotebookPageCoverage.test.tsx src/test/MdNotebookPage.coverage.test.tsx— 112 passedtsc --noEmit— cleaneslinton changed files — 0 errors (1 pre-existing unrelated warning elsewhere in the file)scripts/docs_lint.py/BRAND_BASE_REF=upstream/main scripts/check_brand_name.py— clean