fix(CommandPalette): weigh the grapheme-snap ceiling against the value

.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

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<T>(item: T & { matches?: FuseResult<T>['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<T>(item: T & { matches?: FuseResult<T>['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

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

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 = '<mark></mark>'.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, '&amp;').length + MARKUP).toBeGreaterThan(GRAPHEME_SNAP_MAX_LENGTH)
+
+      expect(result).not.toMatch(LONE_SURROGATE)
       expect(firstCluster(result)).toBe(FLAG)
     })
   })