fix: detect a document's encoding and save it back in the same one (#372) - #544
Merged
Conversation
) A document in a legacy encoding was decoded as UTF-8 with U+FFFD substituted for every byte the decoder disagreed with. #371 closed the data-loss half of that — the buffer is flagged and refused a write over its own file — but the document was still unreadable, so a GBK, Big5, Shift-JIS or CP-1252 file could be opened and not used. The decode now sniffs a BOM, then tries UTF-8, then asks chardetng (the detector Firefox ships) and decodes with its answer. The label travels to the frontend with the text, lives on the tab, and comes back with the save, so an unedited legacy document written back is byte-for-byte the file that was opened. UTF-8 BOM and UTF-16 LE/BE are kept too: the BOM is taken out of the buffer, where `\u{FEFF}# Title` silently stops being a heading, and put back on the save. Two refusals rather than a guess, both leaving the buffer dirty with the reason in a toast: - a file no encoding can read is still `lossy`, and still cannot be written over itself. Detection narrows that case, it does not remove it. - a character the document's own encoding cannot represent — an emoji typed into a Shift-JIS file — fails the save instead of writing encoding_rs's `😀` HTML escape into a Markdown file. Save As is unchanged and stays the way out of both: it writes UTF-8 and repoints the tab at it. encoding_rs was already in the lock file (reqwest), so the new crate is chardetng alone. Its legacy encoders are left on the default slow tables rather than the `fast-*-encode` features, which cost ~180KB each. Also deletes `open_markdown`, the last strict `read_to_string` path, which had no call site left. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
What review of the detection work turned up, all of it on the surface the user actually meets rather than in the decoding itself. **The status bar was still saying `UTF-8`.** It had been the literal string since before Markpad could open a non-UTF-8 file at all, which made it true. Detection made it a lie for exactly the documents an encoding indicator is for: a GBK file now opens, reads correctly and saves back as GBK, and the one place a reader could have caught a misdetection was asserting the opposite. It reads `tab.encoding` now — derived rather than synced, because the encoding belongs to the file the buffer came from and the store already carries it. Same defect #540 fixed one slot to the left. **The refusal was an English sentence from Rust.** Pasting an emoji into a GBK document is refused, correctly — but the reader got "auto-save failed, unsaved changes still in memory" in their own language and the reason in English, in a different corner of the screen. The reason is the half that has to be understood. So `encode_text` returns a marker and this side writes the sentence. The translation that matters is not English-to-Chinese; it is a fact about the program (`ENCODING_UNMAPPABLE`) turned into a fact about what to do next (this file is GBK, GBK cannot hold that character, write a UTF-8 copy). That the result exists in six languages is the cheap half, on machinery the app already has — and it makes this refusal the peer of `lossySaveBlocked`, which has been translated all along. The rule that follows is in the comment: a failure the user can act on gets a code and a sentence; the 91 that hand back an OS error keep the OS's own words, which are the whole of the information and the only string worth searching for. The marker carries nothing. It held the encoding label at first, and the frontend sliced it back out — a round trip that told us what we had passed into the call ourselves. Two claims in the Rust docs were also wrong: - "bytes the guess reproduces exactly are unchanged" overstates it. Several legacy encodings spell one character more than one way — Shift_JIS reaches U+2160 at both 0x8754 and 0xFA4A — so a decode/encode pair normalises to whichever the encoder prefers, with neither `lossy` nor `unmappable` set. The guarantee is over the document's TEXT. The test that pinned it is `..._reproduces_its_canonical_bytes` now, because its fixtures are encoded by `encoding_rs` itself and that is all they can prove. - BOM-less UTF-16 is not detected, by `chardetng`'s design, and reaches the single-byte guess. It round-trips, so nothing is destroyed. Named in `decode_text` rather than left for whoever reads the next bug report.
PathGao
force-pushed
the
fix/non-utf8-documents
branch
from
August 8, 2026 06:57
40ddf35 to
37318ce
Compare
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 was referenced Aug 8, 2026
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
1 participant
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.
Closes #372.
A document in a legacy encoding opened full of U+FFFD and, before the interim
guard in #406, saving wrote that back — permanently. The guard stopped the
destruction by refusing the save, which turned the bug from dangerous into
useless: the file could be read and copied out of, never edited.
This reads it properly instead.
Detecting
Three steps, in the order of how much they can be trusted:
and Windows-authored Markdown is full of them.
modern file is actually in, and the common path stays free.
chardetng— the detector Firefox ships for thisquestion. UTF-8 is denied because step 2 ruled it out; ISO-2022-JP is
allowed, which a browser must not do and an editor has no reason not to.
Worth being explicit about step 2, because it is what makes this narrower than
it looks: VS Code ships
files.autoGuessEncodingoff by default, and thatsetting guesses at files that are already valid UTF-8. Here the detector only
runs once UTF-8 has failed — the point where the alternative is mojibake.
Writing it back
save_file_contenttakes the label the file was decoded from, so a GBKdocument is written as GBK rather than silently converted by the first
auto-save. UTF-16 is encoded here rather than by
encoding_rs, whose UTF-16encoders are defined by WHATWG to emit UTF-8 — correct for the web, a silent
change of the file's encoding for an editor.
Encoding happens before
atomic_write, so a document holding a characterits encoding cannot represent fails without the file being touched. That
refusal matters:
encoding_rs::encodewould otherwise write😀— anHTML numeric reference, into a Markdown file, replacing the character that was
typed. Save As is the way out, and is deliberately always UTF-8.
Verified by hand on real files rather than only in tests — GBK at 26 KB and
98 KB, Shift-JIS at 3 KB:
lossy=false, byte-identical round tripwrote back as GBK with 26465 of 26596 bytes untouched — only the edited
region changed. A silent conversion to UTF-8 would have rewritten every
Chinese byte in the file.
Two things review changed (second commit)
The status bar was still the literal string
UTF-8. True of every fileMarkpad could open until now; a lie for exactly the documents an encoding
indicator exists for. The one place a reader could have caught a misdetection
was asserting the opposite. Same defect #540 fixed one slot to the left.
The refusal spoke English. Pasting an emoji into a GBK file is refused
correctly, but the reader got the generic "auto-save failed" in their own
language and the reason in English, in another corner of the screen. Rust
returns a marker now and the frontend writes the sentence — the same shape
toast.lossySaveBlockedhas always used. The translation that matters is notEnglish-to-Chinese; it is a fact about the program turned into a fact about
what to do next. Six languages is the cheap half, on machinery already here.
The line that follows from it is in the comment: a failure the user can act on
gets a code and a sentence we write; the 91 sites that hand back an OS error
keep the OS's own words, which are the whole of the information and the only
string worth searching for.
Known limits
chardetngdoes not look for it bydesign — and reaches the single-byte guess. It round-trips, so nothing is
destroyed, but it reads as nonsense. Named in
decode_text.lossyis a guarantee over text, not bytes. Several legacy encodingsspell one character more than one way (Shift_JIS reaches U+2160 at both
0x8754 and 0xFA4A), so a decode/encode pair normalises to whichever the
encoder prefers, with no flag raised. Every editor with one encoder per
encoding does this, VS Code included.
Encoding" behind the status-bar item; this has neither, so a misdetection is
recovered from only by Save As, and converting to UTF-8 is possible but
undiscoverable — it is a side effect of Save As rather than an action with a
name. Left out deliberately: it is a UI surface, and Non-UTF-8 documents open with replacement characters and are destroyed on save #372 asked for a call on
that separately. Filed as a follow-up.
Dependencies
encoding_rs0.8 andchardetng1 — the pair Firefox uses, and the ones #372named. Both are the maintainer's call, which is why the issue asked rather than
opening a PR.
145 Rust tests, 906 frontend tests,
npm run checkclean.