fix(markdown): make the code-region order an invariant instead of a sort

[PathGao]

Four unguarded things, three of them load-bearing and one of them a wrong reason for a right conclusion. Each section states the mechanism, then what was measured.

1. regions.sort_unstable() was the whole guard, and nothing tested it

in_code_region is a binary_search_by. code_region_ranges therefore has to return its regions in ascending document order. It did — via sort_unstable() on the last line of a ~90-line function, where it reads as tidiness.

The vector genuinely needed it. Fenced regions were pushed by the line walk in document order; inline code spans were appended afterwards, by a second pass over plain_segments. So for any document containing both, the vector was two sorted runs concatenated, and the sort was the only thing making the binary search correct.

Measured, on master with that one line deleted: cargo test → 144 passed, 0 failed. Nothing in the suite noticed. What actually happens with it gone, for a document with an inline span above a fence (prose containing `a code span`, then a ```text fence holding ![[embed.md]], [[wikilink]], ==highlight==, ^[footnote]):

regions is [(33, 98), (11, 24)]. A probe at offset 40 sits inside the fence, but the search compares against index 1 first, decides the target lies to the right, and returns Err. Every marker in the fence is reported as prose and rewritten — the exact bug class code_region_ranges was written to end (#375 / #389).

Removed rather than tested

A guarded invariant is worse than one that cannot be violated, and the two-pass build was the only reason the vector was ever unsorted. The scan now records a plain segment's inline spans at the moment that segment closes — immediately before the fence that ended it — so the two kinds of region interleave and every push is at a higher offset than the last. plain_segments is gone, the sort is gone, and the extracted push_inline_code_spans is the old inner loop verbatim.

A debug_assert! at the single construction site names the invariant and prints the offending vector. It is a diagnosis, not the guard: it cannot be satisfied by re-sorting somewhere else, because there is no longer a somewhere else.

The tests assert the consequence, not the sortedness

"the vector is sorted" is satisfiable by adding a sort back, which is why it is not what is asserted. Four tests, one per consumer of code_region_ranges, each on a document with an inline span at a lower offset than a fence:

test	consumer	asserts
..._to_embeds	process_internal_embeds	![[embed.md]] in the fence is not turned into <img>
..._to_wikilinks	process_wikilinks (4 passes)	[[wikilink]], ==highlight==, ^[footnote] all stay literal
..._to_autolinks	process_parenthesized_autolinks	a bare URL in the fence is not linkified
..._to_math	mask_math_spans	$x_1$ in the fence is not masked

All three markers in the wikilink case are checked because process_wikilinks runs a separate pass per kind, each probing at its own offset — one probe landing inside the region says nothing about the ones beside it.

Mutation check. Reintroducing the two-pass build order on top of this change:

test tests::an_inline_span_before_a_fence_does_not_expose_the_fence_to_embeds ... FAILED
test tests::an_inline_span_before_a_fence_does_not_expose_the_fence_to_wikilinks ... FAILED
test tests::an_inline_span_before_a_fence_does_not_expose_the_fence_to_autolinks ... FAILED
test tests::an_inline_span_before_a_fence_does_not_expose_the_fence_to_math ... FAILED

code_region_ranges emitted regions out of document order, which makes
in_code_region's binary search miss them: [(33, 98), (11, 24)]

The same tests also go red against master with sort_unstable() deleted (verified separately: 144 passed → 3 failed, before the math test existed).

2. The task-checkbox fail-safe depended on a binding name

annotate_task_checkboxes(html, markdown) is a fail-safe only while markdown is the raw, unpreprocessed buffer. Its doc comment says so at length. What enforced it was that the last statement of convert_markdown happened to spell the argument content, which happened to still be the function parameter.

The obvious way to add a preprocessing step deletes that:

 fn convert_markdown(content: &str) -> String {
-    let processed_autolinks = process_parenthesized_autolinks(content);
-    let processed_embeds = process_internal_embeds(&processed_autolinks);
+    let content = &process_parenthesized_autolinks(content);
+    let processed_embeds = process_internal_embeds(content);

Measured on master: cargo test → 144 passed, 0 failed. The guard is gone and nothing anywhere says so; the two sides it exists to cross-check now agree by definition. With a second, line-shifting step added later, a document renders a checkbox annotated as toggleable whose raw line 5 is "```" — reproduced directly.

What was chosen, and why not the alternatives

Not a newtype. "This string is the one the caller passed in" is provenance, not a type. RawBuffer(content) compiles just as happily around a shadowed content, so the wrapper would move the hazard rather than remove it.

Not the parameter name alone. Any hardening that leaves the raw buffer reachable only through the name content is defeated by shadowing that name — which is exactly the accident.

A capture, plus a source-level test. convert_markdown now copies its input to raw_buffer as its first statement and hands that to the fail-safe. The shadowing edit above becomes a no-op: raw_buffer still points at the parameter and the guard still works. Verified — under the line-shifting shadow, the checkbox stays inert where before it was emitted enabled onto a closing code fence.

That leaves three residual holes, and convert_markdown_hands_the_fail_safe_the_raw_buffer re-reads lib.rs and closes each. Mutation check, all three:

mutation	message
a step inserted above the capture	the raw buffer must be captured before the first preprocessing step, or the step can shadow content above it
raw_buffer bound a second time	raw_buffer is bound more than once — a second binding is the same hole under a new name
the argument changed back to content	the fail-safe is no longer handed raw_buffer; whatever it now receives can agree with the HTML by construction

A source test is not elegant. It is what is left when the property is provenance, and this file already uses the technique for the neighbouring contract (every_convert_markdown_preprocessing_step_is_registered).

The comment overstated the damage

It said a mis-aimed toggle "writes a - [x] marker into whatever happens to sit on that line". That was true of the pre-#352 frontend. documentSession.toggleTaskCheckbox now rewrites only lines already matching /^(\s*(?:>\s*)*(?:[-+*]|\d+[.)])\s+)\[( |x|X)\]/, so a mis-targeted prose line is a no-op and the toggle reports failure.

The comment is corrected rather than deleted, and the guard is still worth having — an overstated justification is dangerous precisely because the next reader checks it, finds it false, and concludes the whole thing is theatre. What still corrupts is a mis-targeted line that is itself task-shaped, and neither spelling is exotic:

• a task list quoted inside a fenced code block — ordinary content in a Markdown editor's own notes;
• a real task elsewhere in the same document — the user clicks one checkbox and a different one silently flips.

3. read_file_content deleted

Zero invoke('read_file_content' call sites in src/ (the checked variant has 7). Also checked and clear: no Rust-internal caller, no test that exercises it, nothing in src-tauri/capabilities/ (custom commands are not named there), nothing in tauri.conf.json. The only other mention is AGENTS.md, where it is a syntax example rather than a reference — left alone, that file is the maintainer's.

The command's defining property is that it returns the text without the lossy-decode verdict — the flag that stops Markpad writing U+FFFD over a GBK or Shift-JIS file. It survived #379 for callers re-reading an already-flagged tab, then lost its last call site and stayed registered: a one-invoke-away way to fill an editable buffer with unflagged mojibake.

Public-surface note for the maintainer: removing a registered Tauri command is a breaking change for anything outside this repository that invokes it. Nothing inside it does.

read_file_content_checked's doc comment absorbs the "deliberately async" rationale that was attached to the deleted function, so it is not lost.

The frontend guard was an allowlist; it is now a scan

checkedReadMigration.test.ts guarded this with a hard-coded three-file list — the files #379 migrated. Adding the call in a fourth file passed, demonstrated.

Deleting the assertion outright was the other option, on the grounds that the command no longer exists so an invoke of it now throws. Rejected: the name would be free to come back, and the Rust command is six lines to re-add. The assertion is instead widened to readSourceFiles('src') — the whole-tree pattern singleImplementationConvention.test.ts already uses, which a new file cannot slip under — and paired with two assertions that the Rust command stays deleted and unregistered. That pairing is what makes it a fact rather than a convention.

Verified: dropping an invoke('read_file_content', …) into a new file under src/lib/utils/ fails the test, naming the file.

4. A wrong reason for a right conclusion, and it is ours

update_pinned_tags explained why the frontend's #405 recent-files fix needed only a re-read while Rust needs a lock:

localStorage is per-document and single-threaded, making an RMW cycle atomic by construction

This is false, and it was written by this project — #424, the change that added this lock. It is also in that PR's body, in bold. Correcting it rather than softening it, because the conclusion it supports is right and someone reasoning from the stated reason about a different shared key would conclude they need no synchronisation at all.

Each document is single-threaded. Two Markpad windows are two documents sharing one origin's storage area. The storage mutex the HTML standard describes for exactly this case is not implemented by any shipping engine — WebKit and WebView2 included — so getItem … setItem in one window interleaves with the other's and loses the same update the diagram above draws.

The real asymmetry

The frontend fix is adequate, but on three empirical grounds rather than by construction:

1. Width. Before the fix the exposure was the whole lifetime of a window's in-memory copy — from the last time that window looked until it next wrote, i.e. minutes. After it the cycle is one synchronous turn of the event loop with no await in it: getItem, a JSON.parse of at most nine short strings, setItem.
2. Frequency. Writes happen on discrete user actions (open a file, remove an entry, rename). Colliding means two windows landing inside those microseconds.
3. Cost. One entry of a recent-file list, which the next open puts back.

None of the three holds on the Rust side. The cycle is a file read, a parse, a serialize and an atomic_write — milliseconds of I/O on a preemptively scheduled thread pool, not microseconds of straight-line JS. The collision is not a coincidence but the ordinary shape of quitting, since ⌘Q makes every window write from its own close handler at once. And a dropped pin is a thing the user made, with nothing to recreate it from. #424 measured this cycle losing 4–7 of 8 updates unlocked.

So: a difference of orders of magnitude in three independent dimensions, not a difference between atomic and not-atomic. recentFiles.ts gets a matching note so the same misreading cannot start from the other end.

This is not a finding that #405 is wrong. The residual race is real and deliberately accepted; no fix is proposed here.

Not covered

• The residual localStorage race in updateStoredRecentFiles. Documented, not fixed. Closing it needs something like a lease key with a compare-and-set retry — a design change and its own issue, and the three grounds above are why it has not been worth one.
• convert_markdown could be split so the raw-buffer binding lives in a scope containing no preprocessing at all, making the shadowing structurally impossible rather than merely harmless. That repoints every_convert_markdown_preprocessing_step_is_registered at a new function name, and lib.rs has other changes in flight in this region. Left for a quieter moment.
• lossyDecodeSaveGuard.test.ts's per-file invoke('read_file_content' assertions are now redundant with the tree scan. They are still correct and locally meaningful in their own tests; not touched.
• AGENTS.md uses read_file_content as its Tauri-command style example. Not edited — maintainer's file, and separately reported in AGENTS.md: three stale statements send contributors the wrong way #385.
• The debug_assert! compiles out in release. It is a development diagnosis for the invariant; the four behavioural tests are what hold the line, in every build.

Verification

cargo test 149 passed (144 → +5) · npm test 562 passed · npm run check 0 errors 0 warnings · npm run build clean · cargo clippy delta 0 — same two warnings as the pre-change baseline (push_str single-character literal, collapsible if), plus setup.rs's unused EXE_NAME under cargo test · cargo fmt diff count unchanged at 53 (rustfmt is not enforced in this repo).

Mutation checks for items 1 and 2 are in their sections above; each violation was re-applied and the suite confirmed red with a message that names the cause.

🤖 Generated with Claude Code