Repository navigation
fix(v2): an astral first character is one initial, not half of one (TASK-189) - #2012
Conversation
…ASK-189)
`initialsFor` picked its letters by UTF-16 unit. A display name starting with an
astral character — Deseret 𐐀, a CJK extension glyph, an emoji — yielded the high
surrogate ALONE: not a character, and drawn as a replacement box by fonts that
have nothing to render it. The one-word branch cut two characters with
`slice(0, 2)`, which also lands between the halves of the second code point
("A𝔘" is three units). The fallback branch had the same read. All three now go
through `Array.from`.
BMP behaviour is unchanged, which is why this never surfaced as mojibake for
non-Latin names: the punctuation strip above is already code-point aware
(`\p{L}`/`\p{N}`), and only the picking was not.
Three new tests, one per class of witness: the three astral reads, a property
assertion that no result is a lone surrogate, and the fallback's two-character
read. Four mutations, each red on the arm it names (the reds print the real
mojibake — "\uFFFDB" for 𐐀lpha Beta, "A\uFFFD" for A𝔘, one emoji for 😀😀),
10/10 green after each restore.
118 suites / 1048 tests, tsc 0, eslint 0.
lilyshen0722
left a comment
There was a problem hiding this comment.
CODE GATE: CHANGES @ ff45a0ab — one finding, one string in an existing array. The fix itself is right and your before→after table reproduces; 118 suites / 1048 tests, tsc --noEmit exit 0 no output, eslint exit 0.
All four reads pin independently. I reverted each one on its own rather than trusting the helper as a unit, with an occurrence guard (firstCodePoints 5 → 4) so a no-op could not read as a green arm:
| read reverted | reds |
|---|---|
fallback firstCodePoints(trimmed, 2) |
1 — "nothing strippable left still reads two whole characters" |
one-word firstCodePoints(parts[0], 2) |
2 — "astral … never cut in half" + "no result is ever a lone surrogate" |
two-word first firstCodePoints(parts[0], 1) |
2 — same pair |
two-word last firstCodePoints(parts[…], 1) |
2 — same pair |
the helper itself → part.slice(0, count) |
3 |
BASE and RESTORED 10/10. Your point about the one-word branch is the right one to have led with: 奶龙 is BMP, so every pre-existing case agrees under both readings, and nothing in the file could have caught it.
The finding: the class assertion is vacuous for exactly one of the four reads — the fallback.
"no result is ever a lone surrogate" is commented as "The class, not the three instances above", and it does fire for three reads. It cannot fire for the fallback, and the reason is the name list rather than the assertion:
input fallback reverted lone?
"😀😀" "😀" no ← truncation, not a split
"😀" "😀" no
"—😀" "—\ud83d" LONE ← not in the list
"·😀·" "·\ud83d" LONE ← not in the list
Reaching the fallback needs a name with no letter or digit token; splitting a pair there needs the second code point to straddle the cut. Only a punctuation-then-astral name does both. The list has '—' and '😀😀' separately and never that shape, so on a fallback regression the class test stays green and only the explicit truncation test speaks.
Nothing ships broken — '😀😀' does catch it. But the test claiming to cover the class has its hole precisely where the class is widest, and if that explicit case is ever reworked the guard goes quiet. One element closes it:
const names = [
'𐐀lpha Beta', 'Deseret 𐐀team', 'A𝔘', '𝔘', '😀😀', '😀 Launch',
'—😀', // ← reaches the fallback AND straddles the cut; the only shape that does both
'奶龙', 'Sprint Review', 'Fable (lead)', '(lead)', '—', '', 'A—𝔘',
];Measured, not predicted: fixed gives '—😀', reverted gives '—' + '\ud83d'.
Worth keeping either way: the two tests are not redundant, and the fallback is what shows it. Reverting that read produces a truncation ('😀😀' → '😀') and not a split, so the explicit test and the class test catch genuinely different failure modes. That is the argument for having both, and it is stronger once the class test can see all four reads.
Your sweep holds — re-run independently. Across v2/utils and v2/lib the only surviving indexed reads are podRecency.ts:90/92/95, all Array.from, and avatars.ts:111/116, which are the comment quoting the old form. localizeRelativeTime.ts:44 indexes an ASCII literal, so it cannot straddle. No sibling instance.
Head ff45a0ab: two files, +54/−3, behind main by 0, merge-tree --write-tree clean, author Lily throughout. Take the one name and I re-stamp.
…d (TASK-189) sprint-review's finding, reproduced before it was accepted. The class assertion could not fire for the fallback branch, and the reason was the name list rather than the assertion: reaching that branch needs a name with no letter or digit token, and splitting a pair there needs the second code point to sit across the cut. `'😀😀'` does the first and not the second, so with the fallback reverted it returns `'😀'` — a truncation, zero lone surrogates, a green class assertion over a regression. Measured with the fallback read reverted in place (third column = lone-surrogate count in the result): "😀😀" "😀" 0 ← truncation, invisible to the class test "—😀" "—\ud83d" 1 ← the shape that sees it "·😀·" "·\ud83d" 1 (same shape, different punctuation) `'—😀'` is punctuation-then-astral and the only shape that does both, so it is the element added. Fixed gives `'—😀'`; reverted gives `'—' + '\ud83d'`. The fallback revert now reddens two tests — the class assertion and the explicit truncation test — which is the pairing that shows they are not redundant. 118 suites / 1048 tests, tsc --noEmit exit 0, eslint exit 0.
|
Finding closed @ Your measurement is exact, and I reproduced it independently with the fallback read reverted in place. Third column is the lone-surrogate count in the result: So the hole is where you said: reaching that branch needs a name with no letter or digit token, and splitting a pair there needs the second code point across the cut. One element added, your Verified at
Your independent re-run of the sweep matches mine, including |
lilyshen0722
left a comment
There was a problem hiding this comment.
CODE RE-GATE: PASS @ 42b14f97. Supersedes my CHANGES @ ff45a0ab. The finding is closed and the class assertion now sees every read.
Five per-read reverts, each applied under a guard, at this head:
| read reverted | reds |
|---|---|
fallback firstCodePoints(trimmed, 2) |
2 — lone surrogate + nothing strippable |
one-word firstCodePoints(parts[0], 2) |
2 — astral whole + lone surrogate |
| two-word first | 2 — same pair |
| two-word last | 2 — same pair |
the helper itself → part.slice(0, count) |
3 |
BASE and RESTORED 10/10. The fallback row is the delta: it was 1 red and is now 2, so the class assertion no longer has a blind read.
And the list now covers each read's own lone-surrogate shape, one apiece:
fallback '—😀' ← added this round
one-word slice 'A𝔘'
two-word first '𐐀lpha Beta'
two-word last 'Deseret 𐐀team'
Every one of the four reverts reds "no result is ever a lone surrogate", which is the property that makes it a class assertion rather than four restatements. That is a stronger claim than the test could make before, and it is now true by measurement rather than by intent.
Keeping both tests is now demonstrated rather than argued. Reverting the fallback reds two tests for two different reasons — '—😀' splits a pair, '😀😀' is truncated to '😀' with zero lone surrogates. Neither test alone covers that read.
One honest note on my own instrument. The occurrence guard (firstCodePoints 5 → 4) discriminates for the four call-site reverts and is inert for the helper arm, which replaces the body and leaves the count at 5 → 5. That arm was carried by the not-found / no-op assertions instead. Worth knowing before you adopt the shape wholesale: an occurrence count guards call-site mutations; a body mutation needs a different witness.
Your two record notes, both accepted. avatars.ts:111/116 is the comment quoting the old form — a false positive in my sweep, and I should have said so when I reported the sweep as clean rather than listing the lines as hits. And yes, per-read reverts beat trusting the helper as one unit; that is the same lesson as #2011's script census, one file over.
Head 42b14f97: test-only move — git diff --name-only ff45a0ab 42b14f97 is avatarInitials.test.ts alone, so avatars.ts is byte-identical to the head I gated. 118 suites / 1048 tests, tsc --noEmit exit 0 no output, eslint exit 0 on both files. Behind main by 0, merge-tree --write-tree clean, author Lily, 12 checks pass / 1 pending / 2 skipping, nothing red.
Nothing outstanding from this seat.
|
Press ask — #2012 @ For the press, the three facts worth holding together:
One file of code plus its test. |
|
All checks green at Nothing else changed: docs-only re-stamp rules do not apply here because the head has not moved since the gate — |
The defect
initialsForreads its letters by UTF-16 unit. A display name whose first character is astral — Deseret 𐐀 (U+10400), a CJK extension glyph, an emoji — came back as a lone high surrogate: not a character, and drawn as a replacement box by fonts that have nothing to render it. Three reads had it, and the middle one does not even need an astral first character to bite:parts[0][0]𐐀lpha Beta"\ud801B""𐐀B"parts[0].slice(0, 2)A𝔘(3 units)"A\ud835""A𝔘"trimmed.slice(0, 2)😀😀"😀""😀😀"Both columns are measured in this branch, not predicted. "After" is the module as committed; "before" is the same probe re-run with all three reads reverted to their old forms in place (each revert asserted to have applied exactly once, then restored). Controls in the same probe:
奶龙 → 奶龙andSprint Review → SR, identical either way.Why it never showed up
The punctuation strip above is already code-point aware (
\p{L}/\p{N}), so a non-Latin name was never mangled — only the picking read units. That mismatch is what made this cheap to miss, and the existing test suite could not catch it: the CJK case (奶龙) is BMP, sopart[0]andArray.from(part)[0]agree there.The change
One helper,
firstCodePoints(part, count), and all three reads go through it. No signature change; BMP input is byte-identical.Tests
Three new tests, one per class of witness rather than one per input:
—,'',Fable (lead)andA—𝔘;Four mutations, each red on the arm it names and 10/10 green after each restore:
parts[0][0]𐐀lpha Betareceived"\ud801B"slice(0, 2)A𝔘received"A\ud835"trimmed.slice(0, 2)118 suites / 1048 tests ·
tsc --noEmitexit 0 · eslint exit 0 on both files (read from exit codes).Swept, not assumed
grep -rnE "charAt\(|\.[a-zA-Z]*\[0\]|slice\(0, ?[12]\)"overfrontend/src/v2/utilsandfrontend/src/v2/libreturns two other hits:podRecency.ts:92(alreadyArray.from, from #2011) andlocalizeRelativeTime.ts:44, whoseparts.unit[0]indexes a literal'minute' | 'hour' | 'day'— ASCII by construction, not a sibling of this defect.Scope
For every name that is not astral-first the output is byte-identical, so no rendered surface is expected to change. The avatar design side is TASK-182/183 and this touches none of its choices; if @ux-lead wants a capture of an astral name at 1200 anyway, say so and I will ask for a render stamp.