fix(webview): pair the parse-frontier gate with a structural-reparse check in the bounded block fields - #397
Merged
Conversation
Was `!syntaxTreeAvailable(...)` alone, which only covers a starved frontier. A structural reparse (an unclosed fence/HTML-block absorbing or releasing a table) re-shapes block boundaries OUTSIDE the changed span while the frontier stays complete, so neither the bounded reuse rule nor G2 could see it — a stale Table model could survive, or a new one go missing. Route the docChanged arm through the shared requiresFullBoundedRebuild(tr) admission test instead. Added three withUnstarvedFrontier rows pinning both directions (vanish/appear) across three distinct Lezer mechanisms (fence pairing, HTML-block absorption, fence re-pairing); all three failed before the fix. Repaired the pre-existing 'revert-check anchor' bounded pin, whose edit sat on a table's own '| H | I |' line and so now takes the TABLE-DELIM structural arm instead of the bounded arm it was meant to exercise — moved the edit into the inert 'prose' paragraph and added a touchesStructuralReparse negative pin so that drifts loudly if it ever regresses.
The three rows read syntaxTreeAvailable after an edit, which CodeMirror reparses under a 20ms wall-clock budget — a bare expect(...).toBe(true) there reds on a descheduled process rather than on the code. Route rows 1 and 3 through withUnstarvedFrontierState so a starved attempt is abandoned and retried, and an all-starved run throws instead of passing silently. Row 2 asserts an INCOMPLETE frontier from a never-finishing parser, which starvation cannot weaken, so it stays a direct assertion. This also restores test/build/no-bare-unstarved-gate.test.ts, which the original shape had turned red.
… check at its frontier gate
Gate the state the assertions read in the admission test: both unstarved rows built a second EditorState, so `requiresFullBoundedRebuild` read a frontier no gate had spoken for. Build the transaction first and hand `tr.state` to the gate; drop the now-unused `edited()` helper and repair the two comments that claimed the property the code did not deliver. Add a sibling revert-check anchor for `boundedUpdate`'s re-walk arm. The existing anchor's span holds no Table node, so only the reuse arm ran; the sibling puts the span on a table so the model comes from `collectTableRanges` + `buildModel`. Both carry the negative pin that keeps them off the full arm. Record the perf cost the shared admission test introduces. Its structural term is presence-based, and `imageBlockField` / `tableSkeletonField` came from an always-bounded baseline, so the "strict improvement" justification does not hold for them: qualify it per consumer, note the trigger set on both field headers, and carry the measured full-walk numbers into PERF.md. Correct four comments that no longer match the tree: the maintenance grep and the symbol a new bounded field should import, two fold-gutter docblocks that enumerated the fallbacks without the structural one, the image field's consumer list, and the reason the G3 rows sit outside the negative pin.
The guard grew three near-identical TypeScript AST walkers — one collecting
line numbers, two early-exiting booleans. Fold them onto a single
`matchingNodeLines(text, fileName, match)` so the traversal exists once and
each query is just its predicate; the three named functions and their
non-vacuity self-tests are unchanged.
Drop the allowlist's `reason` field: nothing ever read it, and the header's
ALLOWLIST block already owns those reasons. The text moves to a comment on
each entry so the rationale stays at the decision point, and the map becomes
`Map<string, number>` — which also removes the `{ count, reason }` noise from
the three synthetic fixtures in the both-directions comparison test.
No assertion changed and no allowance changed.
Comment corrections plus one additive test fixture across the frontier-gate work; no behaviour change. - image-field.ts: drop the false universal "every changed-range-bounded field routes through it" (fenced-code-collapse.ts keeps a narrower structural predicate of its own). Defer the roster to structural-guard.ts, which owns it, rather than restating a list that stales. - image-field.ts / structural-guard.ts / PERF.md: the image field's full-walk cost is now measured (0.745 ms at 19 KB / 150 block images, 0.710 ms at 154 KB / 1200 block images, 30-run averages), so the "measured and accepted" pointers no longer contradict the row they point at. Record that the curve is flat in document size while the table field's grows linearly — same guard and same trigger set, different cost — so neither row is annotated by analogy. - structural-guard.ts: point the scoping follow-up at TODO.md, where it lives. - cm-block-widget-bounded.test.ts: replace stale cross-file line numbers with the symbol and arm names they meant. - cm-table-skeleton.test.ts: the RE-WALK anchor's mechanism is overlap, not a span that contains the table; the quoted span/model geometry contradicted the old explanation. - frontier-gate-needs-structural-guard.test.ts: drop the inline reason comments the ALLOW docstring promises are kept in one place, and pin multiplicity — nothing exercised the walk past its first hit, so capping it left the file green while downgrading the exact-count guard to a file-level heuristic.
… fixture Two reviewers independently reached for the wrong break when checking this fixture by mutation: a `return` after the push only skips that node's descendants, so siblings are still visited, both gates are still found, and the fixture stays green. The cap has to sit at the top of `visit`. Both spellings were measured; the note saves the next reader that round trip.
…d guard The image field's full walk does not reuse alt/safeUrl/slice — computeFreshFull calls buildRange with no prev and re-derives them per node; the reuse is computeBounded's position-shift branch. The measured flatness of that curve had no isolated cause, so the comment now states the measurement and declines the explanation instead of asserting a mechanism. The table skeleton's re-walk anchor claimed the change boundary-touches the model's from. It does not: the insert ends at old offset 5 and the old model starts at 6, and touchesRange needs end >= from. The comment now records that the intersects term alone carries non-reuse there, which is what the anchor exists to exercise.
… sites The image and table docChanged arms carried the same four-line rationale verbatim, and that rationale is already told in full at requiresFullBoundedRebuild's own doc comment. The guard's other two consumers do not restate it either. Each call site now keeps only what is local to it — that its own node can vanish or appear with a complete frontier — and points at the definition for the rest.
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.
What
Two changed-range-bounded
StateFields —imageBlockFieldandtableSkeletonField— decidedwhether they could reuse records by asking only whether the parse frontier was complete
(
!syntaxTreeAvailable). That is half of the admission test. A structural Markdown reparsere-shapes block boundaries outside the changed span while the frontier stays complete,
so a node can vanish or appear with no edit to its own bytes — and the frontier check can never
see it.
Typing an unclosed
```on line 1 ofswallows the image into a
FencedCodenode. The field kept the stale widget, so a block imagestayed rendered over what is now code content. The same edit above a GFM table strands the whole
table model. The reverse direction is just as real: breaking an unclosed fence reveals a node
the bounded walk never looks for, so the widget is simply missing until something else forces a
rebuild.
Changes
structural-guard.tsgainsrequiresFullBoundedRebuild(tr)— the named admission test(
touchesStructuralReparse(tr) || !syntaxTreeAvailable(...)).callout-marker-conceal.tsandfold/index.tsalready spelled that disjunction by hand and now call it;image-field.tsandtable-skeleton.tsadopt it, which is the fix.test/build/frontier-gate-needs-structural-guard.test.tsmakes it a choke point: thereducer-shaped
syntaxTreeAvailable(<expr>.state, …)may be spelled in exactly one place, withan exact-count allowlist for the helper's own definition and for
fenced-code-collapse.ts,which solves the same problem with a deliberately narrower guard on an earlier arm. Pinning the
COUNT per file, not just the file, is what stops a second bare gate appearing inside an already
allowed one. Its known gaps — an import alias, a
const st = tr.stateindirection, and the factthat it cannot tell whether a consumer calls the helper on the right arm — are written into
its header rather than left implied.
sweep of the consumer counts in
structural-guard.ts— a census goes stale every time afield is added, which is how this drift hid.
Why not a narrower guard
The shared predicate over-approximates: any changed line carrying a newline delta, a container
marker, or a
|now takes the full arm on these two fields. Two things follow, and both arerecorded rather than argued.
It is a regression for these two, not an improvement. The justification already written into
structural-guard.ts— that presence-based firing beats the baseline — was written for thefold/callout consumers, whose baseline was an always-full rebuild. These two admitted on the
frontier term alone, so their baseline was always-bounded. The header now says so per
consumer instead of claiming the win uniformly.
The cost is measured. The table field's full walk is 0.40–0.56 ms on 25 KB / 150 tables
and 2.60–3.17 ms on a deliberately pessimistic 200 KB / 1200 tables — two independent 50-run
averages, i.e. run-to-run spread of one quantity, not two quantities. Every reading is inside a
16 ms frame and under the repo's 5 ms perceptibility bar. A cell keystroke returns
truefromthe admission test; a prose keystroke returns
false. PERF.md's two "Closed — landed" rows carrythe new trigger set rather than reading as if nothing changed.
A narrower, delta-based guard is a real option —
fenced-code-collapse.ts'sinsideBlock+topLevelBoundaryRiskis the in-repo precedent — but it needs its own soundness proof bydifferential fuzz. Shipping the fast-but-unproven version while that proof is outstanding is the
wrong order, so it is filed as a follow-up with an explicit re-evaluation trigger.
Verification
before (9 table, 8 image), 0 after.
calloutMarkerConcealFieldandfencedCodeCollapseFieldwere 0 in both runs — they already carried a structural guard.reverting of a single term in the helper reds a specific row.
withUnstarvedFrontierbounded pin incm-table-skeleton.test.tswould have been silentlyvacated by this guard (its edit sat on a line carrying a
|); its edit moved to structurallyinert prose, with a negative pin so the drift cannot recur unnoticed.