Skip to content

fix(narration): match atWord anchors outside ASCII - #45

Merged
shreyaskarnik merged 2 commits into
shreyaskarnik:mainfrom
Joilence:pr/atword-unicode
Aug 25, 2026
Merged

fix(narration): match atWord anchors outside ASCII#45
shreyaskarnik merged 2 commits into
shreyaskarnik:mainfrom
Joilence:pr/atword-unicode

Conversation

@Joilence

Copy link
Copy Markdown
Contributor

Why

atWord normalises the anchor and every transcript token through normalizeWord:

const normalizeWord = (s) => s.toLowerCase().replace(/[^\w']/g, '');

\w without the u flag is [A-Za-z0-9_], so any token with no ASCII letters strips to '' and they all compare equal:

"日本" -> ""     "語" -> ""        normalizeWord('日本') === normalizeWord('語') -> true
"Привет" -> ""   "Мир" -> ""       "Anträge" -> "antrge"

The match loop returns on the first equal token, so a Cyrillic, Han, Devanagari, Greek, Hebrew or Arabic anchor does not miss. It matches word one and returns a confident wrong time. A three-word Russian scene gives 1000ms for its last word, and 1000ms for a word it does not contain at all. Accented Latin is mangled rather than erased, so Anträge is reachable only by a caller who also drops the umlaut. NarrationTimeline is public API (index.ts:29).

What

  • \p{L}\p{M}\p{N} under the u flag, which Unicode property escapes require.
  • \p{M} included: combining marks carry sound in Devanagari, Thai and Arabic, so dropping them collapses काल onto कल and the anchor still lands wrong.
  • NFC first, since significant marks make a decomposed anchor stop equalling a precomposed transcript.
  • Reject an anchor that strips to nothing, which previously returned a confident 1000ms for punctuation.

ASCII behaviour is unchanged: case and trailing punctuation still fold away.

Test

12 cases in tests/narration-atword.test.ts covering ru, zh, hi, de, en, two combining-mark pairs, NFC/NFD and punctuation. Reverting src/narration.ts to 040d8b5, 10 of the 12 fail:

 Tests  10 failed | 2 passed (12)

 FAIL  atWord across scripts > returns null for a word the scene does not contain
 AssertionError: expected 1000 to be null

The 2 passers are de and en, the cases the bug does not reach: diacritic Latin strips identically on both sides, ASCII is untouched.

npm test on the branch: 752 passed. npm run build clean. The test has no capability gate and the repo has no vitest config, so npm test in CI picks it up under vitest's default include.

Notes

  • Sole in-tree caller showcase.demo.ts:41 reads atWord(...) ?? fb, so a stricter null falls through to its fixed timeout.
  • atWord is not in the README, so no docs change.

`normalizeWord` strips a token with `[^\w']`, and JavaScript's `\w` is
`[A-Za-z0-9_]` without the `u` flag. Every Cyrillic, Han, Devanagari, Greek,
Hebrew and Arabic token therefore normalises to the empty string and they all
compare equal, so `atWord` returns whichever word comes first in the scene for
any anchor. It returns a plausible number rather than null, so nothing reports
it. Accented Latin is mangled rather than erased: "Anträge" only matches a
caller who writes "antrge".

`\p{L}\p{M}\p{N}` with the `u` flag. `\p{M}` matters as much as the rest:
combining marks carry the sound in Devanagari, Thai and Arabic, so without them
"काल" still collapses onto "कल" and the anchor still lands on the wrong word.
Keeping marks means a decomposed spelling stops equalling a precomposed one, so
normalise to NFC first, otherwise an NFD "Anträge" never matches Whisper's NFC
"Anträge" and the effect silently never fires.

Also refuse an anchor that strips to nothing rather than matching the first
token that also strips to nothing, which returned a confident 1000ms for
punctuation.

10 of the 12 new cases fail against the previous expression. The one caller in
demos/showcase.demo.ts reads `atWord(...) ?? fb`, so a stricter miss falls back
to its fixed timeout.
@shreyaskarnik

Copy link
Copy Markdown
Owner

Thank you @Joilence good catch

@shreyaskarnik

Copy link
Copy Markdown
Owner

Thanks @Joilence — this one is worse than it looks from the diff, which is exactly why it's a good fix.

\w is [A-Za-z0-9_], so every non-Latin token stripped to '' and they all compared equal. atWord didn't fail to match — it matched the first non-Latin token in the scene and returned a confidently wrong timestamp. A silent-wrong-answer bug, not a silent-no-answer one.

Verified:

  • The transform is symmetric — both sides go through the same function, so there's no asymmetry to introduce a new class of mismatch.
  • \p{M} genuinely earns its place: Devanagari matras and Thai tone marks are load-bearing, and dropping them would just relocate the collision.
  • NFC-first means decomposed and precomposed forms fold together.
  • The empty-anchor guard is well placed — it also makes pure-punctuation transcript tokens permanently unmatchable, which is what you want.
  • The test isn't vacuous: reverting src/narration.ts to main produces 4 failures (expected 999 to be greater than 2500, expected 2000 to be null); restored, 12/12 pass.

One edge case for a follow-up, not a change request here. Turkish/Azerbaijani dotted capital I still misses: toLowerCase() on U+0130 emits i + U+0307, and \p{M} retains the mark, so an İstanbul anchor doesn't match an istanbul token.

Worth recording that the obvious fix doesn't work — I checked before suggesting it:

anchor  İstanbul        → 0130 0073 ...
NFC → lower  (this PR)  → 0069 0307 0073 ...
lower → NFC  (proposed) → 0069 0307 0073 ...   ← identical
transcript "Istanbul"   → 0069 0073 ...

i + U+0307 has no precomposed form, so NFC has nothing to compose and reordering changes nothing. Any real fix has to special-case that mark — which trades against the Devanagari/Thai behaviour this PR is specifically fixing. It also fails safe (null → the caller's fallback), so it's strictly better than what's on main today either way.

Filing that separately. Merging this. Thanks again — both of these were well-scoped and well-tested.

@shreyaskarnik
shreyaskarnik merged commit 9acc78e into shreyaskarnik:main Aug 25, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants