fix(md-notebook): cancel autosave timer on unmount

[adiarora06]

Problem / Motivation

MdNotebookPage arms a 1000ms autosave debounce on every keystroke and never clears it when the page unmounts. The callback outlives the component and fires about a second later against a page that is gone.

In the test suite that lands inside whichever test is running by then, hitting the file-shared api.saveNote mock. #2964 proved it with a stack capture showing the second saveNote arriving via listOnTimeout from a prior test's unmounted component, and had to drop an otherwise useful call-count assertion because of it.

Why it matters

The user-facing stake is the opposite of what the leak looks like. Leaving Notes by in-app navigation does not fire beforeunload, so inside that 1s window the pending timer is the ONLY thing that would ever have written the edit to disk. The bug is not that a stale write happens -- the write carries the newest buffer -- it is that a correct write is riding on an uncancelled timer, and the obvious "fix" of clearing it would silently lose whatever the user typed in the last second before navigating away.

For the suite the cost is concrete: a real cross-test race, and an assertion the project wanted but could not keep.

What changed (motivation -> approach -> change)

Observed symptom: a saveNote call appears in a test that never made one.

Root cause: the window.setTimeout armed in edit outlives the component, because no teardown effect clears it.

Why the obvious change is wrong: cancelling alone is not sufficient, because that timer is load-bearing for in-app navigation. This file had already answered the same question once -- the minsTimer teardown effect clears its timer and then SENDS the pending auto-sync interval, with a comment saying a value the user watched land in the field must not be silently discarded.

The change does the same for the editor buffer, in one teardown effect:

• Clears the debounce, removing the leak.
• Runs whenever the buffer is dirty, not only when a timer or a tracked request exists. A FAILED save is the state that needs this: flushSave clears the debounce on entry and releases its tracking in finally, but leaves the buffer dirty and re-arms nothing, since only a keystroke arms the debounce.
• Sends the pending edit rather than dropping it, preserving the only path that saved it.
• When an autosave is already in flight, waits for it to settle first so the write carries the mtime that save produced. Sending alongside its last retry would carry a superseded mtime, come back ESTALE, and lose the edit -- the outcome the effect exists to prevent. That wait is bounded by the retry loop it waits on (at most 3 sequential saveNote calls) and holds no timer, listener or interval, only a promise closure, so unlike the debounce it replaces it cannot run component logic at an arbitrary later time.
• Waits for an in-flight MOVE for a different reason than a save: a move is what retargets pathRef onto the note's new path, so writing before it lands addresses the path the move is vacating and the edit is lost to a swallowed ESTALE. Registered through the retarget, not merely around the request, because it is the retarget that makes the wait sufficient.
• Snapshots the buffer at unmount rather than reading it after the wait. No keystroke can arrive once the page is gone, and a request settling during the wait can repoint contentRef at disk content -- a move reopens the note at its new path -- which would write the file back unchanged and drop the edit.
• Does not reuse flushSave, and not because of its state setters: React 18 makes a post-unmount setState a silent no-op. The reason is that flushSave re-reads contentRef.current live on every retry attempt, so it is exactly the path that would send those disk bytes back and clear the dirty flag against them. It also mutates mtimeRef and saveTimer that a still-running relocate reads.
• Re-checks targetsSameNote(deletingRef.current, ...) before writing. This is defense in depth, not a live hazard, and the code comment says so: removeNote flushes and bails while still dirty BEFORE it arms deletingRef, and edit and markDirty -- the only two sites that set the dirty flag -- both refuse while a delete is in flight, so the state is unreachable today. It is kept local because this write bypasses flushSave, and with it the ESTALE branch that recognises the backend refusing to resurrect a deleted note; the callers swallow that rejection, so without the check this write's safety would rest entirely on three invariants held in two other functions.

Tests

Three tests in MdNotebookPageCoverage.test.tsx:

• The pending edit is written exactly once on unmount, and the cancelled debounce does not fire a second time.
• Three deferred saves driven by hand prove teardown waits for the in-flight retry and then writes the final snapshot with the fresh mtime.
• With a delete held open on the open note, typing commits nothing and arms nothing, so unmount has nothing to resurrect. This pins the invariant that makes the delete guard unreachable, rather than asserting the unreachable state itself.
• With a rename held open on the open note, typing and then unmounting writes nothing to the path being vacated, and once the move settles the edit lands on the new path.
• After a save fails and leaves the buffer dirty, unmount still writes the edit.

Mutation-verified rather than assumed:

mutation	result
drop edit's delete guard, keep the teardown guard	third test passes -- the teardown guard independently closes the hole
drop both guards	fails: expected "vi.fn()" to not be called at all, but actually been called 1 times
stop tracking the in-flight move, keep everything else	fails: the write is addressed to "One.md", the path the rename vacated
drop the dirtyRef term from the teardown gate	fails: expected 2nd "vi.fn()" call ... but called only 1 times -- the edit after a failed save is never written
remove the whole teardown effect (pre-fix behaviour), run the file 10x	the restored call-count assertion reds in 8 of 10 runs
with the fix, run the file 10x	10 of 10 green

The second row is why the guard is kept: without it, typing during an in-flight delete and then navigating away does POST a write that resurrects the note. The last two rows are why the assertion #2964 dropped is safe to keep now, and they make it the regression pin for this issue -- delete the cancel and the suite goes red instead of silent.

Gates: tsc --noEmit clean; eslint clean on both changed files (one pre-existing a11y warning elsewhere in the file); 317 of 317 green across the 12 md-notebook test files. Rebased onto current main (146 commits, zero conflicts), which clears the base-owned reds this branch had been sitting on -- Frontend Tests (3) was failing in src/i18n/style/hiStyle.test.ts on an auto-research string, and Backend Tests shard 4 was red on three platforms for a diff containing zero Python.

Manual verification

Why no screenshot: nothing rendered changes -- the diff is one unmount-teardown effect, comments and tests, with no markup, style, layout or copy touched; it makes an autosave that already happened happen deliberately.

N/A -- unit coverage sufficient. The change has no rendered surface: it makes an autosave that already happened happen deliberately, and the two states worth checking (does the edit reach disk, does a second call follow) are exactly what the mutation table above measures. There is no visual variant a screenshot could show.

Notes and decisions

Two things #2984 asked for that turned out not to be worth building, recorded as decisions rather than omissions.

A mountDirtyFakeTimers helper is no longer needed. Post-fix the timer is cancelled at unmount, so none of the 8 real-timer mountDirty sites leaks one. What they do instead is issue the teardown saveNote synchronously during cleanup, recorded against the test that owns it, which cannot cross into the next test. The helper would be machinery with nothing left to protect.

The savesInFlightRef Set plus manually resolved deferred is more ceremony than a counter, but flushSave runs can overlap, so a set is the overlap-safe shape, and restructuring a delicate 100-line save function to save four lines is risk without payoff.

Related Issues

Closes #2984

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[adiarora06]

The requested code-review race fix and one-commit PR Hygiene correction are present in the latest commit. The refreshed suite has two remaining root blockers: (1) Screenshot Evidence needs the maintainer-only no-screenshots label because this timer-teardown change has no rendered visual delta; and (2) Frontend Tests (2) failed in the unrelated LocalStorageDebugCoverage.test.tsx (clears only the scroll-height orphan caches could not find Delete cached scroll positions). Frontend Coverage Merge and Coverage Gate are downstream failures from that shard. The focused notebook suite passes 111/111 locally. Could a maintainer please apply no-screenshots and rerun the failed CI jobs?

[github-actions]

UX Review (Fable 5, fork) — ✅ PASS

UX-level review of c7dbbaf88ad993a182a0b0ee526bfec448e88264 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The change is entirely behavioral — an unmount teardown that flushes the pending autosave instead of leaking (or dropping) it. No user-facing strings, markup, layout, or flows are added or altered; the base file already has the beforeunload dirty guard and the minsTimer send-on-teardown precedent this mirrors. From the user's seat, the only delta is that an edit typed in the last second before in-app navigation — or left dirty by a failed save, or typed during a rename — now reaches disk instead of silently vanishing. Autosave was already silent, so a silent unmount flush matches feedback proportionality. The residual gap (a flush that fails after unmount has no surface to report on) predates this PR and has no in-page fix within its scope.

UX-Verdict: PASS

Pure teardown fix with zero rendered surface: edits typed just before in-app navigation now persist instead of silently vanishing — strictly less data loss, nothing new to comprehend.

[UX-REVIEWED] c7dbbaf

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed c7dbbaf88ad993a182a0b0ee526bfec448e88264 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c7dbbaf

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of c7dbbaf88ad993a182a0b0ee526bfec448e88264 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The design is well-grounded: all moveNote paths funnel through the tracked relocate, openNote refuses to retarget pathRef while dirty, and the send-don't-cancel teardown mirrors the file's existing minsTimer precedent. The one candidate concern — a second bespoke save path bypassing flushSave — is justified in-diff (live-ref reads would ship disk bytes back), documented at the site, and every claimed behavior is mutation-pinned by tests. The declined refactor (module-level write coordinator) was considered and recorded; it exceeds this PR's scope.

Design-Verdict: PASS

Correctly rejects the naive cancel-only fix; send-on-unmount with tracked in-flight writes follows the file's own minsTimer precedent, mutation-pinned.

[DESIGN-REVIEWED] c7dbbaf

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed c7dbbaf88ad993a182a0b0ee526bfec448e88264 via the fork AI-review pipeline; updated in place on each push.

Review details

I've verified the crux. api_note_move (server.py:2016) uses os.rename(src, dst). POSIX rename(2) does not alter a file's content-modification time (st_mtime) — only its ctime and the parent directories' mtimes. So after the move, os.stat(dst).st_mtime equals the pre-move mtime. The teardown save to the new path carries the pre-move mtimeRef, which still matches the renamed file within MTIME_TOLERANCE_MS in _assert_note_is_fresh (server.py:1546), so the save succeeds. The candidate's outcome (c) — ESTALE and silent edit loss — does not occur. The candidate's own confidence was "low" for exactly this unverified assumption, and it is now falsified.

No new grounded defect emerges in the diff.

No findings.

[OPUS-REVIEWED] c7dbbaf

[iamwhatever]

👋 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:

• ## Problem / Motivation
• ## What changed

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.

[bolichen97]

👋 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:

• ## Problem / Motivation
• ## What changed

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.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]: This PR has been inactive for 7+ days with failing CI. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author.

Assessment: The single red frontend shard is the unrelated LocalStorageDebugCoverage.test.tsx flake (not the notebook diff), with Coverage Gate/Merge downstream of it; the stored GPT 🔴 BLOCK targeted the superseded commit d9bc0705 and the current head 40b89680 already implements the requested serialization (savesInFlightRef waits for the active flush before the final snapshot). Plan: rebase onto latest main to clear base drift, rerun the flaked shard, add the no-screenshots marker for this no-visual-delta change, and rewrite the description onto the ## Problem / Motivation / ## What changed template scaffold so fork workflows auto-approve and the AI lanes re-run on the current head.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

[iamwhatever]

👋 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:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

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.

[dwu96]

👋 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:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

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.

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of c7dbbaf88ad993a182a0b0ee526bfec448e88264 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified against the base: the minsTimer send-on-unmount precedent exists (MdNotebookPage.tsx:353–368), flushSave does re-read contentRef.current live per retry and mutate mtimeRef/saveTimer (648, 655, 621), every other timer in the file has cleanup (0 unfixed leaked siblings), and the existing writePendingRef/writeChainRef serve settings writes only, so the new tracker is not a second spelling. Final review:

First-Principles-Verdict: PASS

Cancels the one leaked timer in the file at cause level — teardown now owns the debounce's lifecycle — while preserving the write that timer was load-bearing for.

What this change ships

Intent: stop the autosave debounce from firing after the Notes page is gone, without losing the edit it was about to save — a FIX.

1. Leaving Notes no longer fires a stray save ~1s later — justified; reported defect (fix(md-notebook): arm the save-failure test's autosave debounce on fake timers (#2914) #2964).
2. The pending edit is written at leave instead of by the leaked timer — justified; in-file precedent (minsTimer teardown, MdNotebookPage.tsx:353).
3. An edit stranded by a FAILED save is now written on leave — rides along, declared; named data-loss harm, mutation-pinned.
4. Teardown waits for an in-flight save so the write carries the fresh mtime — justified; ESTALE protocol rule.
5. Teardown waits for an in-flight rename so the edit lands on the new path — justified; same rule.
6. Internal write tracker (trackWrite, Set of promises) — justified; 2 registrants counted (flushSave, relocate), concurrency is real, existing writeChainRef (line 263) serializes settings writes only.
7. Delete re-check in teardown, unreachable today — declared defense in depth; load-bearing when edit's guard is dropped (mutation row 2).
8. The call-count assertion fix(md-notebook): arm the save-failure test's autosave debounce on fake timers (#2914) #2964 dropped is restored — justified; it is the regression pin.
9. Five tests pin flush-once, mtime ordering, delete, rename, failed-save — justified.

Sibling count: grepped window.setTimeout|setInterval in website/src/apps/md-notebook — 7 sites, all others cleaned up; 0 unfixed siblings. Nothing to subtract: the smallest honest version needs the gate, the snapshot, and both waits, and each carries a derived one-line reason in place.

[FIRST-PRINCIPLES-REVIEWED] c7dbbaf

[chenmingwei23]

Round 1 dispositioned, pushed as c05751097. Design, UX and Opus were clean; GPT blocked and First Principles raised CONCERNS. Both were right about something.

GPT BLOCKING (teardown save races note moves) -- confirmed and fixed

Reproduced before touching anything. relocate retargets pathRef only in its continuation after await notesApi.moveNote(...), and edit has no move guard, so typing during an in-flight rename and then navigating away writes to the path the move is vacating. Test first, on the previous head:

AssertionError: expected "vi.fn()" to not be called at all, but actually been called 1 times
  "One.md",

Worth stating plainly why this is a regression this PR owns rather than a pre-existing property: on the base the same race exists, but the write is 1000ms late, and that delay is usually enough for the move response to land and repoint pathRef first. Cancelling the timer and writing immediately removes that accidental grace period. So the finding is real and it is mine.

Took the second half of your own recommendation -- serialize it -- rather than the first. Reverting the flush would restore the data loss the PR exists to fix. The in-flight set was only ever tracking saves; it now tracks anything that retargets pathRef, and relocate registers THROUGH its retarget rather than merely around the request, because it is the assignment and not the response that makes the wait sufficient.

That fix also surfaced a second hazard in the same span, so it is closed in the same round: the flush used to read contentRef.current after the wait, and a move's own continuation reopens the note at its new path, which repoints contentRef at DISK content. The buffer is now snapshotted at unmount, where no keystroke can still arrive.

mutation	result
stop tracking the in-flight move, keep everything else	fails: write addressed to "One.md", the vacated path
with the fix	318/318 green, both writes addressed to Renamed.md

The new test asserts two writes, not one, and that is deliberate. The second is relocate's own reopen going through openNote, which flushes the outgoing note -- pre-existing behaviour, same bytes, same path, refused by the backend on mtime. I tried clearing the dirty flag after the flush's write to collapse it to one and reverted that: the clear necessarily lands after its await, so openNote's flush has already read the flag, and shipping a line whose comment claims to prevent a duplicate it demonstrably does not prevent is worse than the duplicate. What regressed, and what the test now pins, is that NEITHER write is addressed to the vacated path.

First Principles CONCERNS -- one premise conceded, subtraction declined with evidence

You are right that "it updates React state, and those setters would run against a component that is already gone" is not a defect under React 18, and that my guard's comment leaned on the bypass to justify itself. That comment is now replaced with the actual reason.

The actual reason also answers the proposed subtraction. Promise.all(activeWrites).then(() => flushSave()) cannot be used, because flushSave's retry loop re-reads contentRef.current on every attempt -- so it is precisely the path that would send back the disk bytes that a move's post-unmount reopen has just written into contentRef, then clear the dirty flag against them. The claim that "its retry loop terminates in one attempt once contentRef can no longer move" is the load-bearing premise, and in the move case contentRef demonstrably can still move after unmount. flushSave also mutates mtimeRef and saveTimer, which a still-running relocate reads.

So the second write site earns its existence: it is the one that writes a SNAPSHOT rather than a live ref. The targetsSameNote guard stays for the reason the mutation table in the body gives -- drop it together with edit's guard and an unmount after typing during a delete does POST a resurrect.

Two of us were within one round of deleting the mechanism that turned out to be load-bearing, which is worth recording in case a later reader reaches for the same subtraction.

[chenmingwei23]

Round 2 dispositioned, pushed as c7dbbaf88. UX, Design and First Principles are PASS on c05751097 -- First Principles moved from CONCERNS to PASS after the premise concession and the corrected comment. GPT blocked on a new, different span, and it was right again.

GPT BLOCKING (settled save failures bypass the flush) -- fixed as prescribed

Confirmed by reading the failure path rather than the happy one. A save that FAILS leaves the exact state my gate did not cover: flushSave clears saveTimer on entry and releases its tracking in finally, so afterwards there is no timer and nothing in the tracked set, while the buffer is still dirty -- and nothing re-arms the debounce, because only a keystroke does. Navigating away then sent the edit nowhere, which is the loss this effect exists to prevent, arriving through the failure path instead of the leak.

Took the prescribed fix verbatim: !dirtyRef.current joins the early-return condition. Dirty is precisely "there is an unpersisted edit", so it belongs in the gate rather than only in the write below.

mutation	result
drop the dirtyRef term from the gate	expected 2nd "vi.fn()" call to have been called with [ 'v1', 'One.md', ...(2) ], but called only 1 times
with the term	319/319 green

One thing worth stating so the scope is honest: unlike round 1's move race, this is not a regression this PR introduced. On the base a failed save loses the edit on navigation too, because there is no teardown flush at all. It is an incompleteness in the mechanism I am adding, which is a fair thing to block on -- the feature's whole claim is that a pending edit survives leaving the page, and it did not hold on the failure path.

I also checked the one case where writing here could be undesirable rather than merely useless. If the failure was ESTALE, the conflict banner is asking the user to choose between their buffer and the file on disk; teardown writing their version might look like resolving that silently in the buffer's favour. It cannot: mtimeRef still holds the pre-conflict mtime, so the write is refused with the same ESTALE and swallowed. Nothing on disk is clobbered, and for a transient failure such as ENOSPC the write is simply retried, which is the point.

Round 1 sat in the move/retarget span and this one in the teardown gate, so they are separate causes rather than the same finding recurring -- no escalation warranted yet. Local gates on the new head: tsc clean, eslint 0 errors, 319/319 across the 12 md-notebook test files.

[chenmingwei23]

Review-ready at c7dbbaf88. 59 checks green, 2 skipped, zero failures; PR Readiness passed; all five review lanes clean on this head, GPT included after two rounds. mergeStateStatus is BLOCKED only on "Review required" -- branch protection wants a code-owner approval, not another check.

What changed since the takeover, in one place:

• Rebased onto current main (146 commits, zero conflicts), which cleared 7 base-owned reds -- Frontend Tests (3) was failing on an auto-research i18n string and Backend Tests shard 4 on three platforms, for a diff with zero Python.
• Round 1, GPT blocking, real and mine: the flush wrote to the path an in-flight rename was vacating. Reproduced first, then fixed by making the in-flight set track anything that retargets pathRef -- relocate registers through its retarget, not merely around the request. Same round closed a second hazard in that span: the buffer is now snapshotted at unmount, so a move's post-unmount reopen cannot repoint contentRef at disk content.
• Round 2, GPT blocking, a real gap though not a regression: a FAILED save clears the debounce and releases its tracking while leaving the buffer dirty, so the gate returned early and the edit was lost. !dirtyRef.current joined the gate.
• First Principles moved CONCERNS to PASS: its React-18 premise was right and the comment leaning on the weak justification is gone, replaced by the actual reason flushSave cannot be reused -- it re-reads contentRef live, which is exactly what round 1 proved unsafe after unmount.

Every fix mechanism is mutation-verified; the table is in the description. Two asks from #2984 were deliberately not built (mountDirtyFakeTimers, and simplifying the tracked-set bookkeeping), with reasons under "Notes and decisions".

Not merging -- that is the maintainer's call.

[bolichen97]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: cancel the md-notebook autosave timer on unmount, fixing a leaked timer; UI-lifecycle only, no auth or input change. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.