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) }) })