From 8f851ddcca0dda88c7312a643669873fa4a127ac Mon Sep 17 00:00:00 2001 From: Shevchik Igor Date: Thu, 13 Aug 2026 12:00:45 +0000 Subject: [PATCH] fix(CommandPalette): weigh the grapheme-snap ceiling against the value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GRAPHEME_SNAP_MAX_LENGTH` is documented, and was tested, as a bound on the field text. It was not measured against it: `truncateHTMLFromStart` built its snapper from `content` β€” the value after escaping and after `` insertion β€” so the ceiling was reached early, by an amount nothing in the constant hints at. Escaping has no fixed overhead: `&` expands to five characters, `"` to six. 'a'.repeat(8140) + FLAG.repeat(10) + 'match' // value.length 8185 '&'.repeat(1600) + FLAG.repeat(50) + 'match' // value.length 1805 Both degraded, and both rendered `πŸ‡ΈπŸ‡Ί` with an orphaned regional indicator where the author wrote `πŸ‡ΊπŸ‡Έ` β€” a different country, silently. That is the defect #364 describes and #371 exists to prevent, reappearing above a threshold no reader could predict. `createClusterSnapper` now takes the length to weigh separately from the string to segment, named `fieldTextLength` for what it is. The parameter is required rather than defaulted: a default is what would let the next derived-string caller inherit this bug without noticing. Segmenting a string the ceiling would have excluded is affordable, but not because the call is cheap in isolation β€” on a long run of regional indicators it is not. It is that `Array.from(html)` already paid the larger price on the same string, unconditionally, on both sides of this fix. End to end the difference is tens of microseconds either way, within noise. `test/bench/search.bench.ts` gains a case that reaches the ceiling by escaping rather than by length, so the claim has a durable measurement behind it rather than a throwaway script. The two ceiling tests moved off `value.length + MARKUP` onto `value.length`, which makes `snaps clusters exactly at the ceiling` the plain-text repro β€” a value of exactly the advertised length previously degraded. The new case covers the unbounded half: every fixture in `degraded paths` was free of escapable characters, so the expansion never reached the ceiling arithmetic. `.sync/PORTING.md`'s guard entry said the threshold "is crossed 13 characters earlier than a reading of `value.length` suggests". That described the defect, not the design, and is corrected here rather than left for the documentation follow-up β€” a porter reading it could otherwise restore the bug on purpose. Closes #387. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LWWrBHgfqGSbeU3V6UuMF8 --- .sync/PORTING.md | 8 +++++--- src/runtime/utils/search.ts | 40 +++++++++++++++++++++++++++++++------ test/bench/search.bench.ts | 10 ++++++++++ test/utils/search.spec.ts | 39 ++++++++++++++++++++++++++++-------- 4 files changed, 80 insertions(+), 17 deletions(-) diff --git a/.sync/PORTING.md b/.sync/PORTING.md index a88c2bcf..6e9e744d 100644 --- a/.sync/PORTING.md +++ b/.sync/PORTING.md @@ -230,9 +230,11 @@ material. Reproduce its *intent* in b24ui by editing files under `src/` only. ~8k, two orders of magnitude worse at 100k. Past the guard only the surrogate snap applies: `οΏ½` is still prevented, but **every** multi-code-point cluster loses protection, not just flags β€” the same degradation as a runtime - without `Intl.Segmenter`. What the guard measures is the *marked-up* string, - not the value: truncation runs after the tags are inserted, so the threshold - is crossed 13 characters earlier than a reading of `value.length` suggests. + without `Intl.Segmenter`. What the guard measures is the value, at both call + sites β€” `createClusterSnapper` takes the length to weigh separately from the + string to segment, because truncation segments the escaped, marked-up copy. + Weighing that copy instead is #387: escaping expands `&` five-fold and `"` + six-fold, so the guard fired for values a fraction of its length, silently. Upstream has no equivalent β€” inferred from this file's history, not re-inspected β€” so replaying upstream's `generateHighlightedText` or diff --git a/src/runtime/utils/search.ts b/src/runtime/utils/search.ts index b0898cd0..6d4325fb 100644 --- a/src/runtime/utils/search.ts +++ b/src/runtime/utils/search.ts @@ -33,6 +33,9 @@ const LINE_FEED = 0x000A // surrogate-pair snap applies: `οΏ½` is still prevented, but every // multi-code-point cluster loses protection β€” flags, ZWJ sequences, combining // marks alike β€” the same degradation as a runtime without `Intl.Segmenter`. +// +// The length compared against it is always the field text, never a string +// derived from it β€” see `createClusterSnapper`'s `fieldTextLength`. const GRAPHEME_SNAP_MAX_LENGTH = 8192 // True when `index` points at the low half of a surrogate pair β€” i.e. cutting @@ -85,8 +88,18 @@ interface ClusterSpan { * `Segments.containing()` is called once per boundary, and a value can carry * hundreds of match regions: rebuilding the segmenter view for each of them * cost 8.1 ms over 1600 boundaries, against 0.6 ms when it is built once. + * + * @param value The string being sliced. + * @param fieldTextLength The length weighed against `GRAPHEME_SNAP_MAX_LENGTH`. + * Required rather than defaulted to `value.length`, because the two differ + * exactly where it matters: truncation slices the escaped, marked-up copy, and + * escaping expands `&` five-fold and `"` six-fold, so weighing that copy retired + * the snap for field text a fraction of the ceiling's length β€” silently, and for + * exactly the multi-code-point clusters the snap exists to keep whole (#387). A + * default would let the next derived-string caller inherit that bug in silence. + * @returns `toStart`/`toEnd`, bound to `value`. */ -function createClusterSnapper(value: string) { +function createClusterSnapper(value: string, fieldTextLength: number) { // `null` once resolved to "no segmenter for this value"; `undefined` while // still unresolved, so a value that never needs snapping never builds one. let segments: Intl.Segments | null | undefined @@ -106,7 +119,7 @@ function createClusterSnapper(value: string) { } if (segments === undefined) { - const segmenter = value.length <= GRAPHEME_SNAP_MAX_LENGTH ? getGraphemeSegmenter() : null + const segmenter = fieldTextLength <= GRAPHEME_SNAP_MAX_LENGTH ? getGraphemeSegmenter() : null segments = segmenter ? segmenter.segment(value) : null } @@ -136,7 +149,7 @@ function createClusterSnapper(value: string) { } } -function truncateHTMLFromStart(html: string, maxLength: number) { +function truncateHTMLFromStart(html: string, maxLength: number, fieldTextLength: number) { let keptLength = 0 let totalLength = 0 let insideTag = false @@ -186,7 +199,22 @@ function truncateHTMLFromStart(html: string, maxLength: number) { // A code point is not what the reader sees. Snap the cut past any grapheme // cluster it lands inside, so the visible character after the ellipsis is the // one the author wrote rather than its tail. - return '...' + html.slice(createClusterSnapper(html).toEnd(html.length - keptLength)) + // + // `html` is the escaped, marked-up copy, so its own length is the wrong thing + // to weigh against the ceiling β€” hence `fieldTextLength`. + // + // Segmenting a string the ceiling would have excluded is affordable, but the + // reason is not that the call is cheap in isolation β€” on a long run of + // regional indicators it is not, since `containing()` walks the run. It is + // that `Array.from(html)` above already paid the larger price on the same + // string, unconditionally and on both sides of this fix. Measured end to end + // against the old behaviour, the difference is tens of microseconds either + // way: within noise, and against rendering a flag as the wrong country. + // + // Run `pnpm run bench` before changing this. Bare figures rot, and the first + // ones written here were three orders of magnitude out β€” they timed one + // `Segments` view reused across iterations, where this builds a fresh one. + return '...' + html.slice(createClusterSnapper(html, fieldTextLength).toEnd(html.length - keptLength)) } /** @@ -247,7 +275,7 @@ export function highlight(item: T & { matches?: FuseResult['matches'] }, s let content = '' let nextUnhighlightedRegionStartingIndex = 0 - const snap = createClusterSnapper(value) + const snap = createClusterSnapper(value, value.length) // Fuse merges, sorts and integer-bounds `indices` itself, so this is a no-op // for the search path. It matters because `highlight()` is a published @@ -321,7 +349,7 @@ export function highlight(item: T & { matches?: FuseResult['matches'] }, s // Measure the budget in code points too, so it stays in the same units as // the counter inside `truncateHTMLFromStart`. Identical to `.length` for // BMP-only content. - content = truncateHTMLFromStart(content, Array.from(content.slice(markIndex)).length) + content = truncateHTMLFromStart(content, Array.from(content.slice(markIndex)).length, value.length) } return content diff --git a/test/bench/search.bench.ts b/test/bench/search.bench.ts index bcf6eb00..6d33e966 100644 --- a/test/bench/search.bench.ts +++ b/test/bench/search.bench.ts @@ -44,6 +44,12 @@ const flags = prefixed('\u{1F1FA}\u{1F1F8}') const underCeiling = itemFor(`${'a'.repeat(8000)}${'\u{1F1FA}\u{1F1F8}'.repeat(10)}${MATCH}`) const overCeiling = itemFor(`${'a'.repeat(9000)}${'\u{1F1FA}\u{1F1F8}'.repeat(10)}${MATCH}`) +// The same ceiling, reached by escaping rather than by length. `"` expands +// six-fold, so this value is a fifth of `underCeiling` and its marked-up copy is +// six times longer β€” the case #387 was about, and the one that decides whether +// weighing the field text rather than that copy costs anything worth having. +const underCeilingEscaped = itemFor(`${'"'.repeat(1300)}${'\u{1F1FA}\u{1F1F8}'.repeat(50)}${MATCH}`) + // A value carrying many match regions: snapping is bound to one segmenter view // per value, and this is the case that regresses hardest if that is undone. const manyRegions = (() => { @@ -86,6 +92,10 @@ describe('highlight', () => { highlight(overCeiling, MATCH, 'label') }) + bench('under the ceiling, escaped six-fold', () => { + highlight(underCeilingEscaped, MATCH, 'label') + }) + bench('many match regions', () => { highlight(manyRegions, MATCH, 'label') }) diff --git a/test/utils/search.spec.ts b/test/utils/search.spec.ts index 909671ee..585880bc 100644 --- a/test/utils/search.spec.ts +++ b/test/utils/search.spec.ts @@ -369,10 +369,13 @@ describe('highlight', () => { const GRAPHEME_SNAP_MAX_LENGTH = 8192 const FLAG = '\u{1F1FA}\u{1F1F8}' - // What the ceiling is measured against is the *marked-up* string, not the - // value: truncation runs on `content`, after the tags are inserted, so it is - // 13 characters longer. A fixture sized against `value.length` sits under the - // ceiling and degrades anyway. + // What the ceiling is measured against is the value itself, at both the + // mark-insertion and the truncation call. Truncation runs on `content` β€” + // escaped and marked up, so always longer β€” and weighing *that* put the + // boundary somewhere no reader could predict: 13 characters early for plain + // text, five times earlier for text made of `&`. The two cases below fence + // the constant against `value.length`; `measures the ceiling against the + // value` covers the expansion. const MARKUP = ''.length // Captured before the stub below replaces the global, since this helper has @@ -454,10 +457,10 @@ describe('highlight', () => { }) it('skips the segmenter one character past the length ceiling', () => { - const value = `${'a'.repeat(GRAPHEME_SNAP_MAX_LENGTH - 57)}${FLAG.repeat(10)}match` + const value = `${'a'.repeat(GRAPHEME_SNAP_MAX_LENGTH - 44)}${FLAG.repeat(10)}match` const result = highlight(matchAfter(value), 'match', 'label') ?? '' - expect(value.length + MARKUP).toBe(GRAPHEME_SNAP_MAX_LENGTH + 1) + expect(value.length).toBe(GRAPHEME_SNAP_MAX_LENGTH + 1) // The surrogate fallback still holds β€” that is the whole point of the // degradation being partial. @@ -472,10 +475,30 @@ describe('highlight', () => { // The mirror of the case above, one character shorter, so the pair fences // the constant to a single value: lower it by one and this fails, raise it // by one and the other does. - const value = `${'a'.repeat(GRAPHEME_SNAP_MAX_LENGTH - 58)}${FLAG.repeat(10)}match` + // + // This is also the plain-text half of #387. Weighing `content` put this + // value 13 characters past the ceiling, so it degraded β€” a value of + // exactly the advertised length, losing the snap with nothing to show why. + const value = `${'a'.repeat(GRAPHEME_SNAP_MAX_LENGTH - 45)}${FLAG.repeat(10)}match` + const result = highlight(matchAfter(value), 'match', 'label') ?? '' + + expect(value.length).toBe(GRAPHEME_SNAP_MAX_LENGTH) + expect(firstCluster(result)).toBe(FLAG) + }) + + it('measures the ceiling against the value, not its escaped copy', () => { + // Escaping expands: `&` to five characters, `"` to six. Weighing the + // escaped copy therefore retired the snap for values a fraction of the + // ceiling's length β€” this one is at 22% of it β€” and did so silently, for + // exactly the multi-code-point clusters the snap exists to keep whole + // (#387). Held against `value.length`, it snaps. + const value = `${'&'.repeat(1600)}${FLAG.repeat(50)}match` const result = highlight(matchAfter(value), 'match', 'label') ?? '' - expect(value.length + MARKUP).toBe(GRAPHEME_SNAP_MAX_LENGTH) + expect(value.length).toBeLessThan(GRAPHEME_SNAP_MAX_LENGTH) + expect(value.replace(/&/g, '&').length + MARKUP).toBeGreaterThan(GRAPHEME_SNAP_MAX_LENGTH) + + expect(result).not.toMatch(LONE_SURROGATE) expect(firstCluster(result)).toBe(FLAG) }) })