Skip to content

Serialize spell checks and keep their results anchored across edits - #94

Merged
Wavesonics merged 5 commits into
mainfrom
fix/spellcheck-check-serialization
Sep 24, 2026
Merged

Wavesonics merged 5 commits into
mainfrom
fix/spellcheck-check-serialization

Conversation

@Wavesonics

@Wavesonics Wavesonics commented Aug 4, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to the spell-check investigation in #93.

What was wrong

  • Nothing stopped two spell checks running at once. They interleave suspending lookups against a single EditorSpellChecker session and race each other's span swaps. Two full checks can start at init: one from rememberSpellCheckState when the checker resolves, and one from the document-replacement listener in SpellCheckingTextEditor, depending on which runs first.
  • A partial check's range is computed before it runs, and its results before its lookups finish. An edit in either window left it decorating the wrong text. Serializing the checks lengthens the first window, since a partial check can now queue behind a full scan.
  • A full check whose lookups raced an edit re-scanned the whole document, and after three attempts gave up without installing anything. Nothing re-ran it, so typing through the init check of a long document behind a slow checker left the document undecorated.
  • Misspelled words and sentence corrections were kept in side lists with the ranges they had when found. Edits move the spans but not the lists, so a right-click on a squiggle below an edit found no segment and fell back to the plain menu.

What changed

  • A Mutex serializes runFullSpellCheck, runPartialSpellCheck and checkWordSegment. Main-dispatcher confinement is stated on the class, since the non-suspending entry points can't take a suspending lock and the spans are Compose state.
  • A partial check records the lines its range addresses (textLines is immutable and unchanged lines keep their identity, so the list works as a revision). Before scanning and again before installing, LineDiff compares those lines with the current document as an untouched prefix and suffix around one changed band:
    • edits clear of the range shift it and its results by whole lines;
    • edits on it widen it over the changed lines and scan again. Nothing is dropped: invalidateSpellCheckSpans has already stripped the range, and the edit's own check covers only the text it touched.
  • checkWordSegment records the lines its segment addresses the same way: an edit clear of the word's line shifts it, and an edit on that line re-checks the line.
  • The debounced collector captures the lines once per batch, so later ranges in a batch still address the document they were computed against.
  • A full check uses the same LineDiff: it installs its results on the lines the edits left alone, shifted onto the current document, then re-checks only the changed lines through the partial check. There is no attempt cap.
  • The side lists are gone. Each span's style records its finding, and the editor already moves styles with their spans: a word reads its text from the span, and a correction takes the span's current range.

API change

SpellCheckStyle is now an open class whose companion is the plain underline, so the spell check module can subclass it. Source that uses SpellCheckStyle as a value or with is is unaffected. Two things change:

  • binary compatibility (SpellCheckStyle.INSTANCE becomes SpellCheckStyle.Companion);
  • style == SpellCheckStyle no longer matches spans the spell checker installs. Use is SpellCheckStyle.

Scope

Rebased onto main. Main already announces whole-document replacements (#95); its full-check retry loop is replaced here. The review found bugs in this PR's original partial-check hash guards and full-check de-dup, so both were removed.

getSuggestions still calls the checker outside the lock on purpose: locking it would hold the context menu behind a full document check.

Tests

  • two checks never run against the spell checker at once
  • a partial check queued behind another follows a line inserted above its range
  • a partial check re-scans a line edited during its lookups
  • a click on a squiggle an edit shifted finds its word
  • a click on a sentence squiggle an edit shifted finds its correction
  • a full check that keeps racing edits still decorates the document
  • checkWordSegment follows a line inserted above its word
  • checkWordSegment re-checks its line when an edit lands on it
  • LineDiffTest unit tests

The eight SpellCheckStateTest cases fail without their fix. ./gradlew check is green.

@codacy-production

codacy-production Bot commented Aug 4, 2026 •

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 52 complexity · -2 duplication

Metric Results
Complexity 52
Duplication -2

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@Wavesonics
Wavesonics marked this pull request as draft August 4, 2026 06:04
@Wavesonics

Copy link
Copy Markdown
Collaborator Author

Code review findings

High-effort review of this branch against main. 34 candidates, 8 refuted, 10 distinct defects confirmed or plausible. Drafting this PR until the guard design is reworked.

They collapse to two root causes.

The de-dup key is under-specified

spellCheckMode is not part of the key (SpellCheckState.kt:203, CONFIRMED)
A host that sets spellCheckMode = Sentence and calls runFullSpellCheck() while the init Word check is in flight gets its request swallowed: the Word check completes over the same text, bumps completedFullChecks, and the queued sentence request sees satisfiedWhileWaiting. sentenceCorrections stays empty until the document is edited.

The bookkeeping stamps the current checker, not the one that ran (SpellCheckState.kt:214, CONFIRMED)
runFullWordCheck captures sp at entry and does every lookup through it, but the completion stamp re-reads the mutable spellChecker field. A check started with C1 that finishes after a swap to C2 credits C2 with C1's results, so C2's own full check is skipped and the document keeps the previous language's squiggles.

The guards are whole-document; the work they gate is range-local or queued

Partial checks snapshot the revision after the mutex wait (SpellCheckState.kt:329, CONFIRMED)
The range argument is computed before the call blocks on checkMutex, but revision is read after the lock is granted. Edits that land during the wait are invisible to the guard, so the stale range is used: spans are stripped from a region the user did not edit and wordSegmentsInRange clamps silently and squiggles the wrong words. This re-admits the exact corruption the PR set out to prevent, through the queuing the mutex introduces.

A whole-document hash discards results that were still valid (SpellCheckState.kt:342, also :377, CONFIRMED)
An edit in paragraph 5 drops paragraph 1's computed misspellings even though its ranges were untouched. invalidateSpellCheckSpans already stripped paragraph 1's spans, and the next debounce batch only covers paragraph 5, so the misspelling is left with no squiggle and no scheduled re-check. The comment's claim that "the edit that invalidated it schedules its own check" holds only for the newly edited range.

checkWordSegment returns "misspelled" without installing the span (SpellCheckState.kt:411, CONFIRMED)
On a hash mismatch the guard skips the span swap while the return value stays unconditional. The caller is told the word is misspelled, no SpellCheckStyle span is added, nothing is appended to misspelledWords, and a right-click there gets no suggestions. Nothing re-checks that word.

The retry loop abandons the document (SpellCheckState.kt:279, CONFIRMED)
After MAX_FULL_CHECK_ATTEMPTS the check returns null and no code path ever calls runFullSpellCheck again: its callers are the one-shot LaunchedEffect(spellChecker) and setSpellCheckingEnabled. Typing into a long document against a slow checker during the init check leaves the untouched bulk of the document with no squiggles for the rest of the session. The previous code always installed a result, so whole-document coverage was guaranteed even when the result was stale.

requestedHash is captured before the lock (SpellCheckState.kt:196, PLAUSIBLE)
setText emits no edit operation, so the debounced collector never sees a programmatic load and only an explicit runFullSpellCheck covers it. If importMarkdown's setText lands while such a request is queued, the request wakes comparing a hash the document no longer has, sees satisfiedWhileWaiting, and returns. The freshly loaded document gets zero squiggles.

Remaining

Partial checks queue behind up to three full re-scans (SpellCheckState.kt:224, PLAUSIBLE)
The retry loop holds checkMutex across complete re-scans, so squiggles for freshly typed text arrive seconds late, and if the user keeps typing during the wait the partial check is then dropped by its own guard.

getSuggestions calls the checker outside the mutex (SpellCheckState.kt:97, PLAUSIBLE)
Known and noted in the PR description. It means the "lookups never interleave against a single session" invariant the mutex KDoc claims is not actually enforced: a right-click during a full check runs suggestions/isCorrectWord concurrently with the scan.

The full-check log line sits outside the retry loop (SpellCheckState.kt:242, also :291, cleanup)
The document can be scanned three times per call while the line prints once, so the log understates the work done.

Direction

The de-dup key fixes are mechanical: add the mode, record sp. The rest wants a different guard: a monotonic revision counter on TextEditorState, captured at request time and carried through the mutex wait, with partial checks validating their own range rather than the whole document, and the exhausted-retry path scheduling a retry rather than returning. Tests for mode switch, checker swap, and a partial check queued behind a full one should come first.

A smaller alternative: keep the Mutex alone and drop every revision guard and retry loop. That fixes the duplicate init check and the interleaving, carries none of the dropped-work findings above, and leaves the stale-swap race for a later change.

Full checks, partial checks and checkWordSegment now take one lock, so two
checks can't interleave lookups against a single spell-checker session or race
each other's span swaps. Two full checks can start at init: one from
rememberSpellCheckState and one from the document-replacement listener.
@Wavesonics
Wavesonics force-pushed the fix/spellcheck-check-serialization branch from 872e23d to 93892e9 Compare September 24, 2026 05:48
@Wavesonics Wavesonics changed the title Serialize spell checks and drop results the document outran Serialize spell checks behind a mutex Sep 24, 2026
@Wavesonics
Wavesonics marked this pull request as ready for review September 24, 2026 05:48
A partial check's range is computed before it waits on the lock, and its
results before its lookups finish, so edits in either window left it
decorating the wrong text. The check now records the lines its range addresses
and diffs them against the current document: edits clear of the range shift it
and its results by whole lines, and edits on it widen it over the changed lines
and scan again instead of dropping the result.
@Wavesonics Wavesonics changed the title Serialize spell checks behind a mutex Serialize spell checks and carry partial ranges across mid-check edits Sep 24, 2026
Misspelled words and sentence corrections were kept in side lists with the
ranges they had when found. Edits move the spans but not the lists, so a click
on a squiggle below an edit found nothing and fell back to the plain menu.
Each span's style now records its finding: a word reads its text from the
span, and a correction takes the span's current range. SpellCheckStyle becomes
an open class whose companion is the plain underline, so the spell check
module can subclass it.
@Wavesonics Wavesonics changed the title Serialize spell checks and carry partial ranges across mid-check edits Serialize spell checks and keep their results anchored across edits Sep 24, 2026
A full check whose lookups raced an edit re-scanned the whole document, and
after three attempts gave up without installing anything; nothing re-ran it.
It now installs its results on the lines the edits left alone, carried onto
the current document, and re-checks only the changed lines through the
partial check.
The segment addresses the document when the call is made, but the span went in
after the lock wait and the lookup. An edit clear of the word's line now shifts
the range; an edit on that line re-checks the line instead.
@Wavesonics
Wavesonics merged commit 999f083 into main Sep 24, 2026
2 checks passed
@Wavesonics
Wavesonics deleted the fix/spellcheck-check-serialization branch September 24, 2026 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant