fix: ignore stale large-file loads - #247
Merged
alecdotdev merged 1 commit intoJul 27, 2026
Merged
Conversation
PathGao
marked this pull request as ready for review
July 27, 2026 15:45
PathGao
pushed a commit
that referenced
this pull request
Aug 3, 2026
`MarkdownViewer.svelte` declares six top-level bindings that the file never mentions again. They read as live state — a container element, a render debounce and its timer handle, a per-tab load revision map, a caret element and its measured offset — so anyone tracing how rendering or scroll sync works has to prove each one is inert before ruling it out. Five were never used at all. `containerEl`, `renderTimeout` and `renderDebounceMs` arrive in b46a283 and `caretEl`/`caretAbsoluteTop` in ef219cf, each already at exactly one occurrence in the commit that adds them: the declaration, and nothing else. `renderDebounceMs = 50` in particular suggests a debounce that does not exist. One is refactor residue. `loadRevisionByTab` had four occurrences when 41de13b (#247) added it here. bb8dbf3 (#274) moved document loading into `documentSession.svelte.ts`, which carries its own map and the three call sites; the declaration here stayed behind. Two identically named maps in two files, one of them dead, is worse than either alone — `scripts/largeFileLoadRevision.test.ts` pins the live one in `documentSession.svelte.ts` and is unaffected. Nothing else changes. `npm run check` and `npm run build` were the check that matters: an unreferenced binding is only safe to remove if the compiler agrees nothing reaches it, including through Svelte's `bind:this`, which is why this is verified by a build rather than by grep alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PathGao
pushed a commit
that referenced
this pull request
Aug 3, 2026
`MarkdownViewer.svelte` declares six top-level bindings that the file never mentions again. They read as live state — a container element, a render debounce and its timer handle, a per-tab load revision map, a caret element and its measured offset — so anyone tracing how rendering or scroll sync works has to prove each one is inert before ruling it out. Five were never used at all. `containerEl`, `renderTimeout` and `renderDebounceMs` arrive in b46a283 and `caretEl`/`caretAbsoluteTop` in ef219cf, each already at exactly one occurrence in the commit that adds them: the declaration, and nothing else. `renderDebounceMs = 50` in particular suggests a debounce that does not exist. One is refactor residue. `loadRevisionByTab` had four occurrences when 41de13b (#247) added it here. bb8dbf3 (#274) moved document loading into `documentSession.svelte.ts`, which carries its own map and the three call sites; the declaration here stayed behind. Two identically named maps in two files, one of them dead, is worse than either alone — `scripts/largeFileLoadRevision.test.ts` pins the live one in `documentSession.svelte.ts` and is unaffected. Nothing else changes. `npm run check` and `npm run build` were the check that matters: an unreferenced binding is only safe to remove if the compiler agrees nothing reaches it, including through Svelte's `bind:this`, which is why this is verified by a build rather than by grep alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PathGao
added a commit
that referenced
this pull request
Aug 3, 2026
`MarkdownViewer.svelte` declares six top-level bindings that the file never mentions again. They read as live state — a container element, a render debounce and its timer handle, a per-tab load revision map, a caret element and its measured offset — so anyone tracing how rendering or scroll sync works has to prove each one is inert before ruling it out. Five were never used at all. `containerEl`, `renderTimeout` and `renderDebounceMs` arrive in b46a283 and `caretEl`/`caretAbsoluteTop` in ef219cf, each already at exactly one occurrence in the commit that adds them: the declaration, and nothing else. `renderDebounceMs = 50` in particular suggests a debounce that does not exist. One is refactor residue. `loadRevisionByTab` had four occurrences when 41de13b (#247) added it here. bb8dbf3 (#274) moved document loading into `documentSession.svelte.ts`, which carries its own map and the three call sites; the declaration here stayed behind. Two identically named maps in two files, one of them dead, is worse than either alone — `scripts/largeFileLoadRevision.test.ts` pins the live one in `documentSession.svelte.ts` and is unaffected. Nothing else changes. `npm run check` and `npm run build` were the check that matters: an unreferenced binding is only safe to remove if the compiler agrees nothing reaches it, including through Svelte's `bind:this`, which is why this is verified by a build rather than by grep alone. Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
PathGao
added a commit
that referenced
this pull request
Aug 8, 2026
…ice (#547) (#552) A document over 50KB is read twice: `open_markdown_preview` returns the first 50KB so something renders at once, and a background full read replaces it. #247 gave every load a revision and made the second stage refuse to apply a stale result. The first stage was left unguarded, across two awaits. The startup path delivers a file on two channels that nothing dedupes. Rust's setup emits `file-path` with argv (`lib.rs:3063`) and `send_markdown_path` re-reads `std::env::args()` (`window_runtime.rs:575`); on macOS `RunEvent::Opened` does both in the same handler — pushes the path onto `startup_files` AND emits `file-path` (`lib.rs:3155-3160`). The frontend listens for the event (`MarkdownViewer.svelte:3087`, not awaited) and separately drains the stash (`:3366`). So two loads run on one tab, and which preview read returns first is a coin flip: t=0 load A rev=1 -> open_markdown_preview (slow) t=0+ load B rev=2 -> open_markdown_preview (fast) t=110 B stage 2 lands -> whole file, isTruncated false <- correct t=120 A stage 1 lands -> 50KB slice, isTruncated true <- overwrites it t=220 A stage 2 -> rev 2 != 1, correctly refused A destroys the good state and then declines to repair the damage it caused. The tab keeps `isTruncated`, nothing retries, and every save from then on is refused with 'Refusing to save a partially loaded document' — reported through the auto-save timer, so a load failure surfaces as a save failure. Which of `canApplyFullLoad`'s five conditions fails is only ever the revision: path, isDirty, isEditing and isSplit are all unchanged at bail time. The guard is the one the second stage already applies, moved to cover every write a load makes. It has to sit ahead of `setTabDecodedLossy`/`setTabEncoding` and not merely ahead of the buffer: #544 made the encoding verdict part of the same write, a 50KB byte cut can split a multi-byte character (`utf8_truncation_boundary` in `lib.rs:476` is UTF-8 only, and `samples/encoding-gbk-large.md` is 103,723 bytes of GBK), and `tab.encoding` is what the save writes with. A stale prefix's verdict is the wrong one. The non-markdown branch takes the same guard. It reads the whole file so it cannot strand a slice, but a stale one still overwrites the winner's buffer and encoding and flips the tab into the editor. Not fixed here: a tab can also carry `isTruncated` with an empty buffer, from `markTabContentUnavailable()` when session restore defers or fails a read (`windowSession.svelte.ts:255`, `:283`). That is the blank-pane half of the report, it is a different cause that happens to share the flag, and a guard on this race does not touch it. Also left alone: the refusal message is an untranslated English string where `toast.partialDocument` already exists, translated, and is what every editing entry point shows for this state. `scripts/largeFileLoadRevision.test.ts` was 18 lines of source regex, which is why nothing caught this — it passes with the bug present. It now drives the real TabManager and the real session through both orderings. Revert the guards and three of the four new tests go red; that regex test stays green. Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Fixes #246.
Summary
Validation
node --test --import tsx scripts/largeFileLoadRevision.test.tsnpm run check