test(notes-tree): mount a viewport of rows in the folder-rename suite - #1958
Merged
Conversation
The virtualizer mock mounted all 150 filler rows, and every keystroke into the rename input re-renders the whole tree, so each two-rename case cost ~4,800 row renders: 15s on a quiet CI worker against a 30s test timeout. Mount 20 rows, which is what the real virtualizer does.
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
…suite too Same mount-every-row virtualizer mock as the folder-rename suite, same 150 fillers, same whole-tree re-render per keystroke: its typing case took 6.6s on a quiet CI worker against the same 30s timeout.
The sidebar's default sort is manual, which falls back to newest-first, and createNote stamped every note with new Date(). On a fast machine all 151 stamps tie and insertion order survives; on the CI runner they spread across milliseconds and 'Note 0' sorted to row 150, outside the 20-row window. Each note is now one millisecond older than the one before it.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
h4yfans
marked this pull request as ready for review
September 2, 2026 16:31
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1921
Root cause. The suite's
useVirtualizermock mounted every row (Array.from({ length: count }), 151 rows with the 150 fillers that push the tree pastVIRTUALIZATION_THRESHOLD), and every keystroke into the folder-rename input re-renders the whole tree: the draft value lives inuseNoteTreeActionsat the tree root, andFolderRow/NoteRowinvirtualized-notes-tree.tsxare not memoized. An instrumented run counts 32VirtualizedNotesTreerenders fora second rename targets the new path(one per keystroke, clear, Enter and refetch), which is 4,832 row renders in jsdom for one test. On the last greenmainCI run (33613758936) that test took 15,136 ms,rename, rename again, delete15,677 ms anda renamed folder is still renameable15,111 ms, against a 30 stestTimeout. Their non-virtualized twins in the same file took 0.5 to 1.2 s. A 2x slowdown from parallel load turns the file red, which is what three unrelated branches saw.It is not a race in the rename path: no assertion ever fails, the tests run out of clock. It is not a leak from a sibling suite either, and this repo carries a stored note asserting that
focusManagerleaks across renderer test files, so the refutation is spelled out. The renderer project runs with the roottestblock inapps/desktop/config/vitest.config.ts:and vitest 4.1.10 only hands a finished worker to the next file when isolation is off (
node_modules/vitest/dist/chunks/cli-api.*.js):Every renderer file therefore gets a fresh worker thread. A two-file probe run in one
--no-file-parallelisminvocation confirmed it: file B saw a differentthreadId, an unset global, and nothing that file A had put infocusManagerorlocalStorage. Whatever that stored note observed, it was not a cross-file global under this configuration.Fix. Both suites that combine 150 fillers with the mount-every-row mock now mount
Math.min(count, 20)rows, which is what the real virtualizer does (visible rows plusoverscan: 10). The tree still has 150+ items, soshouldVirtualizestill picksVirtualizedNotesTreeand both files still cover the renderer #1529 taught to show a rename input.notes-tree-folder-rename-duplicate.test.tsx(the file in the issue). Folders sort before notes, so the folder under test and any phantom duplicate the original bug produced are rows 0 and 1, and its notesAlpha/Betaare rows 1 and 2 when expanded. Mutation check: a 1-row window failsan open folder stays open through a renameonUnable to find an element with the text: Alpha.notes-tree-virtualized-rename.test.tsx(same defect, less margin). On the same green CI run its typing caseabove the threshold: the inline input commits the renametook 6,650 ms, so it sat 4.5x under the same 30 s ceiling before this change, less headroom than the issue's file had on that run. ItslargeVaultis 150 root notes plusProjects/Alpha.md;Projectsis row 0 (folders sort first) and every other case addressesNote 0. Mutation check: a 1-row window fails bothNote 0cases onUnable to find an element with the text: Note 0; theProjectscases still pass at row 0, as they should.The first push of this window failed those two
Note 0cases in CI (run 33630793068) while passing locally, and that disagreement was a fixture defect, not a window size. The sidebar's default sort forcollectionsismanual, whose comparator falls back to newest-first (notes-tree-sort.ts,compareNotes), andcreateNotestamped every note withnew Date(). On a fast machine all 151 stamps land in one millisecond, tie, and the stable sort keeps insertion order, soNote 0is row 1. On the CI runner the same loop crosses millisecond boundaries, the last note created is newest, andNote 0sorts to row 150, outside any viewport. Reproduced locally by giving each note a distinct increasing stamp: the same two cases fail with the same message. The mount-every-row mock had hidden this since the file was written. The fixture now stamps each note one millisecond older than the previous one, so newest-first equals insertion order on every machine and noDate.now()is left in the fixture. Both files keep the same 20-row window; the issue's file never addresses a filler row (its cases name the folder and its two children, rows 0 to 2), so itsnew Date()stamps are left as they are.Checked and left alone:
virtualized-notes-tree.test.tsxcarries the same mock over small hand-built trees (slowest case 1.4 s on that CI run) andfolder-view/folder-table-view.test.tsxmounts every row of a handful of table rows (slowest 187 ms). Neither has 150 fillers or a typing loop, so a window there is churn with no timeout to remove.Minimal reproducer. Run either file alone and read vitest's per-test durations; no sibling file is needed:
Before/after on the same machine, back to back, load average ~10 to 12:
notes-tree-folder-rename-duplicate, above the thresholdnotes-tree-virtualized-rename, above the threshold (load ~29, both runs)An earlier pair at load ~12 read 999 ms to 241 ms for the typing case; the ratio is the same, the machine was busier for this pair.
Not changed. No timeout raised, no retry, no separate vitest project. The product-side "one keystroke re-renders the tree" is real but bounded in the app by the real virtualizer (roughly 40 mounted rows), so it is noted here rather than memoized for a test's sake.
Release note
none
Test plan
Full
test:rendererunder contention, three consecutive runs (first commit only; the second commit changes one sibling file, timed alone below), because contention is the failure condition the issue describes. Load averages are fromuptimeimmediately before and after each run; file durations are from vitest's JSON reporter.notes-tree-folder-rename-duplicatefile / slowest testcomposer.test.tsxfile / slowest testThe load was five sibling agents plus at least one other full suite on the same box; that is the condition, not a caveat. The measured headroom is 2.7x, not the 30x the isolated timings suggest: the worst single test under that load,
a second rename targets the new pathat 10,957 ms, is 2.7x inside the 30 stestTimeout. Its row work is 7.5x smaller than before (640 vs 4,832 mounted-row renders), so the old mock at the same load lands around 80 s, which is the failure the issue describes. Run fromapps/desktop(process.cwd()is whattests/utils/contrast.tsand the CSS contract tests read from); a run from the repo root fails seven unrelated files onENOENT .../src/renderer/src/assets/base.css.notes-tree-virtualized-rename.test.tsxalone at head, load ~29: 8/8, slowest case 1,287 ms (3,866 ms onorigin/mainone run earlier).notes-tree-folder-rename-duplicate.test.tsxre-run at head, load ~34: 14/14, slowest 2,514 ms. CI on this PR's head00dcae9dbis green: Unit & integration tests, run 33648007574. On that runner the issue's three two-rename cases took 3,771 / 4,306 / 3,526 ms (15,136 / 15,677 / 15,111 ms on the last greenmainrun before this change), and the sibling's typing case took 2,014 ms (6,650 ms before), with bothNote 0cases green at 784 ms and 1,017 ms.pnpm lintexit 0pnpm typecheckexit 0pnpm --filter @memry/desktop typecheck:testexit 0 (both changed test files compile)pnpm check:architecturepassedpnpm check:contractspassedgit diff --checkexit 0pnpm docs:impact --base origin/main --strict: no docs-relevant changesnpx -y react-doctor@latest .: no finding in the changed files