fix(CommandPalette): keep astral characters intact when truncating search results

[arsalan507]

Closes #339.

Upstream first

Per the note at the end of #339 — src/runtime/utils/search.ts is byte-identical between this repo and nuxt/ui (re-verified by diff against nuxt/ui@v4 today, 3459 bytes both), and so is test/utils/search.spec.ts.

So the fix went upstream first: nuxt/ui#6817. This PR is the identical patch, applied here with git apply from the upstream commit so the two files stay in sync byte-for-byte.

Entirely your call whether to land this now or wait for upstream and take it through the normal sync — the issue left that open ("unless it is wanted here sooner"). If you'd rather wait, close this and it'll arrive on its own; nothing here diverges from upstream.

The fix

Two hunks, exactly as #339 prescribed:

1. truncateHTMLFromStart — iterate with Array.from(html) so the loop advances one code point at a time and never slices a surrogate pair in half. The insideTag tracking is untouched, since < and > are always single code units.
2. The caller — measure the length budget in code points too ("and count length the same way"), so it stays in the same units as the counter inside the function.

Worth flagging for review: hunk 1 alone is sufficient to remove the lone surrogates; hunk 2 is the consistency half. It does mean an emoji now counts as one character against the truncation budget rather than two, so emoji-containing snippets truncate slightly later than before. That reads as closer to the intent of a visible-length budget, but it is a behaviour change and I'm happy to drop it if you'd prefer the minimal diff.

BMP-only content is byte-identical either way, which is why all pre-existing tests pass untouched.

Verification

Reproduced your sweep first, before changing anything — lone surrogates appear from filler length 7 onward and for every length after, exactly as reported:

n=10, before: "...\uDE00😀😀😀😀😀😀<mark>match</mark>"
n=10, after:  "😀😀😀😀😀😀😀😀😀😀<mark>match</mark>"

Three tests added to test/utils/search.spec.ts:

• never splits an astral character, at any truncation boundary — the 1–40 sweep, asserting no lone surrogate at any length
• keeps emoji before the match intact
• still truncates a long prefix down to an ellipsis — guards against over-correcting into "never truncate"

The first two were verified failing against unmodified search.ts (expected [ 7, 8, 9, … ] to deeply equal []) and passing with the fix. The third passes both before and after by design.

test/utils/search.spec.ts             26 passed  (both projects; 3 new tests × 2)
test/utils/                          192 passed
CommandPalette + DashboardSearch +
ChatPalette + DashboardSearchButton  160 passed
eslint src/runtime/utils/search.ts test/utils/search.spec.ts    clean

Consumers checked: src/runtime/components/CommandPalette.vue and src/runtime/composables/useContentSearch.ts are the only two touching utils/search.

[IgorShevchik]

Thanks for this — and in particular for going upstream first, for reproducing the sweep before touching anything, and for flagging the second hunk as a judgement call instead of burying it. That is the right shape for a fix in a synced file.

I put the patch through five independent reviews: Unicode correctness, security, test quality, upstream-sync risk, and performance. The fix is correct and we want it — with two changes before merge, and one caveat about what it closes.

Confirmed

• The bug reproduces on main exactly as you reported: lone surrogates from filler length 7 onward, and for every length after.
• Your patch removes them — 0 introduced lone surrogates across ~200k fuzzed inputs, against 687 for main on the same corpus.
• BMP-only behaviour is byte-identical to main (18k+ cases, 0 differences).
• Security-neutral. 512 adversarial cases plus 60k fuzz cases rendered through a DOM: no element other than <mark>, no attributes, no stray angle brackets, no unbalanced tags — in both branches. Your "not a security issue" reading in #339 holds.
• Your two new tests do fail against unpatched search.ts with the error you quoted, and test/utils/ + CommandPalette pass with the patch (296 tests). LONE_SURROGATE is correct — brute-forced over 780 strings against a code-point ground truth, 0 mismatches.

Please change 1 — keep the loop's early break

Array.from(html) materialises the whole string, which throws away the early break. The function was O(budget); it becomes O(full length), and that is worst in exactly the case it exists for — a match far from the start.

Measured on a 979-char description with a late match: 39 loop iterations either way, but 0 → 1003 array elements, and 0.32 µs → 6.66 µs (20.6×). At resultLimit: 1000 with 206-char descriptions that is +1.5 ms per throttled update (6.2×). The default resultLimit: 12 is unaffected in practice (<0.5% of a frame), but this repo ships a virtualize example with resultLimit: 1000, and processGroupItems highlights all of them before virtualization decides what to render.

Coalescing the pair in place keeps the break, and was verified byte-identical to your version over 200k randomised inputs (lone surrogates, ZWJ and flag sequences, CJK Ext B, <mark> fragments):

for (let i = html.length - 1; i >= 0; i--) {
  let char = html[i]
  const code = html.charCodeAt(i)
  if (code >= 0xDC00 && code <= 0xDFFF && i > 0) {
    const hi = html.charCodeAt(i - 1)
    if (hi >= 0xD800 && hi <= 0xDBFF) { char = html.slice(i - 1, i + 1); i-- }
  }
  // ... rest unchanged
}

Cost against main: 1.06–1.28× instead of 2–20×. Same idea for the caller — an allocation-free code-point count avoids building the tail array twice.

Please change 2 — the tests do not fence the fix

They catch the regression, but they pass against four different wrong "fixes", including skip truncation entirely when the string contains astral characters and delete every astral character before truncating. still truncates a long prefix down to an ellipsis is meant to guard the first of those, but its filler is 'a', so it only exercises the BMP path the patch never touches.

Hunk 2 also has no coverage: in every new test the text from <mark> onward is pure ASCII, so both budget formulas return the same number. Revert hunk 2 alone and all three tests stay green.

The smallest change that closes both: one exact-string assertion with an emoji filler, plus a fixture with emoji inside the marked region. Two smaller things while you are there — expect(result).not.toContain('�') can never fail (the bug emits raw surrogates, never U+FFFD), and the ?? '' in the sweep means a highlight() returning undefined for everything would pass.

Caveat — this closes half of #339

The same class of bug lives one layer up, and the patch does not reach it. Fuse's indices are code-unit offsets, so value.substring() at search.ts:92,94 can insert <mark> inside an astral character. Reproduced with real fuse.js at this component's shipped defaults, with an ordinary typo as the search term:

label "deployment 😀😀 pipeline", term "zployment 😀"
fuse indices [[1,13]] — 13 lands between \uD83D and \uDE00
→ d<mark>eployment 😀\uD83D</mark>\uDE00 pipeline

Pre-existing and out of scope here, but it means this should not auto-close #339. Keep Closes #339 if you would rather split the remainder into a follow-up — flagging it so the issue is not closed as fully fixed.

On the byte-identical claim

This is the one thing worth restating. highlight() here takes a fifth useTokenSearch argument that upstream does not have. It was added in c502157b and 6743f793, both against the file's old name fuse.ts and both without an Upstream: trailer, and shipped in v2.8.0. The rename in 557a5178 carried it across, which is why git log on search.ts looks clean unless you pass --follow. Our own sync ledger scopes its claim to "matches upstream's behavior 1:1 … without token search".

git apply succeeding proves the hunk contexts matched, not that the files match — and both your hunks sit away from those lines, so a clean apply is fully consistent with the files differing. I have not inspected nuxt/ui, so this is not a claim about what is there; only that the premise "the two stay byte-for-byte in sync" does not hold on our side.

It does not change the answer to your "now or wait" question: land it here now. The divergence is now recorded in .sync/PORTING.md so a future port does not silently revert it.

On hunk 2 — keep it

Worth knowing what it actually does. Because the tail appears in both the budget and the counter, it cancels: the remaining budget when the walk reaches the mark is always exactly the tag characters — 13 code points per <mark>…</mark> pair, regardless of tail length or content. So the real effect is that the retained window is 13 characters rather than 13 UTF-16 units. That is the change we want; the comment just overstates it as tracking the tail. A named constant would read better than Array.from(content.slice(markIndex)).length.

Notes, no action needed

• Grapheme clusters are still split — this moves code unit → code point, not to user-perceived character. Worst case is flags: 🇺🇸×7 drops one regional indicator and the run re-pairs shifted, rendering a different country. Silently wrong rather than visibly broken. Follow-up material (Intl.Segmenter), not this PR.
• Entities are still cut mid-sequence (& → mp;). Pre-existing and inert — no suffix of any escape is a valid entity prefix.
• Roughly half the outputs this changes were well-formed before and simply get a longer prefix, so this is not purely a bug fix — worth a changelog line. Wider prefixes also interact with the CSS truncate on itemLabel / itemDescription: the <mark> sits further right and is marginally more likely to be clipped.

---

Generated by Claude Code

[IgorShevchik]

Follow-up to the review above — we've taken the two changes on ourselves rather than sending you another round trip, since this had already been waiting three days on our side.

#365 carries your commit 12c8e741 unchanged, authorship intact, with a second commit on top applying the review. Nothing of your fix was rewritten: the code-point iteration and the matching budget in the caller are both yours, and the second commit only changes how the code points are reached — coalescing a low surrogate with the high one before it, instead of Array.from up front, so the loop keeps its early break. Verified byte-identical to your version over 300,300 inputs before touching anything.

For what it's worth, your instinct on the second hunk was right and we kept it. It turned out to be load-bearing in a way neither of us said explicitly: because the tail appears in both the budget and the counter, it cancels, and the retained window is always exactly the tag characters — so the real effect of that hunk is that the window is 13 characters rather than 13 UTF-16 units. Reverting it alone left every test green, which is why the new suite has a case with emoji inside the marked region.

Three things this surfaced are now filed separately, none of them yours to fix:

• bug(search): highlight() inserts <mark> inside astral characters #362 — the same defect at the insertion boundary. Fuse's indices are code-unit offsets too, so <mark> can be placed inside an astral character. Your PR fixes where the string is cut; this is where the tag is inserted. It's why bug(search): truncateHTMLFromStart splits astral characters, emitting lone surrogates #339 shouldn't be auto-closed.
• test(search): highlight()'s b24ui-only useTokenSearch argument has no coverage #363 — highlight() here carries a b24ui-only fifth argument that upstream doesn't have, which is why the byte-identical premise didn't hold. It had never been documented, and it has no test. You found that by making the claim; it had been invisible in this repo since v2.8.0.
• bug(search): truncation splits grapheme clusters — a flag renders as a different country #364 — grapheme clusters, out of scope as agreed.

Thank you for this — going upstream first, reproducing the sweep before changing anything, and flagging the second hunk as a judgement call instead of burying it is the standard we'd want from a regular contributor, let alone a first PR here. Leaving this open for a maintainer to close alongside #365 rather than closing it out from under you.

---

Generated by Claude Code

[IgorShevchik]

Corrections. I put my own review and the superseding PR through five independent checks, and several things I told you were wrong.

Your implementation ships as you wrote it. #365 no longer contains my replacement commit. I had swapped your Array.from for in-place surrogate coalescing on performance grounds; those numbers were measured on the helper alone and excluded an O(n) scan the same commit added to the caller. Measured as shipped, my version was 1.2–1.25× of main rather than the 0.86–1.02× I published, slower than yours by up to 1.39× on astral-dense input with an early match, and at the default resultLimit: 12 the entire difference was about 6 µs per keystroke. I also quoted 12× and 2160× ratios that are 3.7× and 11.4× through the function the app actually calls — the helper is roughly 1.4% of end-to-end cost, escapeHTML dominates. Dropping it also preserves the property you went upstream for: search.ts stays in sync, which my rewrite had quietly broken.

"This closes half of #339" was wrong. #339 is scoped entirely to truncateHTMLFromStart — its title, its Problem section, and its prescribed fix, "iterate by code point … and count length the same way". You delivered exactly that, both halves. It closes the issue completely. #362 is a separate defect in a different function, and I have retitled it to stop implying otherwise.

Which makes my instruction about the trailer backwards. I wrote "Keep Closes #339 if you would rather split the remainder into a follow-up" — that sentence contradicts itself, since keeping it is what closes the issue. You read it correctly as written, and as it turns out Closes #339 was right all along. Nothing to change.

On byte-identical — the error was ours, not yours. That premise originated in this repository: commit 595923b9 (PR #338) states "src/runtime/utils/search.ts is byte-identical to upstream today", #339 repeated it, and you inherited it in good faith. Asking you to restate it was backwards. Your measurement was also exactly correct — git show origin/main:src/runtime/utils/search.ts | wc -c is 3459 — and I should have said so. The actual divergence, useTokenSearch shipped in v2.8.0, was invisible to anyone not running git log --follow across a rename that happened after it landed. That is a bookkeeping failure on our side; it is now recorded in .sync/PORTING.md (#366) and its missing coverage is #363.

Two smaller things: the PR had been open under two days when I said three, and the SHA I quoted as "your commit" was the post-cherry-pick one — yours is f08572d0.

What remains on top of your commit in #365 is test hardening only, src/ untouched. Your fixture used a single astral character that sits inside both surrogate ranges, so an implementation with an off-by-one range bound passed; it now sweeps the range edges, including U+1F3FF, a skin-tone modifier that shows up in ordinary text. The sweep now counts surviving characters rather than only scanning for lone surrogates — as written, an implementation that deletes every astral character passed a test named "never splits an astral character". And nothing covered the caller's half, so reverting it alone left every test green.

The substantive part of my review stands: the three cases as shipped passed against four different wrong implementations. Everything else above, I got wrong. Thanks for your patience with it.

---

Generated by Claude Code

[IgorShevchik]

Merged — your fix is on main as 01252a62, via #365, with your Co-Authored-By on the commit so it lands in your contribution history. #339 is closed. Closing this one as superseded rather than unmerged.

The implementation that shipped is yours, unchanged — both hunks, including the caller half you flagged as a judgement call. What we added on top was test hardening, and every gap it closed was one your work made visible rather than one you introduced.

Two things this PR is worth remembering for beyond the fix itself. It surfaced a divergence from upstream that had been sitting in search.ts since v2.8.0, undocumented and untested, invisible to anyone not running git log --follow across a rename — now recorded in .sync/PORTING.md (#366) with #363 tracking the missing coverage. And it turned up two further defects in the same area, #362 and #364, neither of which was yours to fix.

nuxt/ui#6817 stands on its own merits upstream; nothing here depends on its outcome either way.

Thanks for a genuinely good first contribution here — going upstream first, reproducing the reported sweep before changing anything, and flagging the debatable half instead of quietly shipping it. That is the part that made everything downstream possible, including catching our own errors. Please do send more.

---

Generated by Claude Code

[arsalan507]

Thank you — for the depth of the review, for preserving authorship through #365, and for correcting the record publicly when the errors turned out to be on the repo's side. That's not something every maintainer would do, and I learned a lot from the five-lens review alone. Taking you up on "please do send more": I'll pick up #362 next.