Skip to content

Fix synced transcript auto-scroll for non-Latin transcripts (CJK, Thai, Hindi, Arabic, and more) - #5494

Open
joashrajin wants to merge 7 commits into
mainfrom
pcdroid-639-android-synced-transcript-auto-scroll-jumps-back-to-top-of
Open

Fix synced transcript auto-scroll for non-Latin transcripts (CJK, Thai, Hindi, Arabic, and more)#5494
joashrajin wants to merge 7 commits into
mainfrom
pcdroid-639-android-synced-transcript-auto-scroll-jumps-back-to-top-of

Conversation

@joashrajin

@joashrajin joashrajin commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Description

The synced ("follow along") transcript never kept the highlighted line anchored for Japanese (CJK) transcripts: each time playback advanced to the next line, the transcript scrolled back to the top, so the user had to manually scroll down to find where playback was. It reproduced only with CJK text, not with Latin-text transcripts.

Root cause: the synced-transcript post-processing (TranscripSanitization.joinSplitSentences()) splits a transcript into per-sentence TranscriptEntry blocks using endsAsSentence() / findMidSentence(), whose punctuation lists were Latin-only (. ! ? … - ) ] > } " ” ' ’). Japanese sentences end with , so no cue was ever treated as a sentence boundary and the whole transcript collapsed into a single TranscriptEntry. With one entry, AutoScrollEffect (which anchors the scroll on highlightState.entryIndex) had no per-line target: the index stayed at 0, so the list stayed pinned to the top while only the highlighted word advanced.

Changes:

  • Recognise CJK sentence terminators (and half/full-width full stops ) so CJK transcripts split into per-sentence entries again, restoring per-line auto-scroll anchoring.
  • Recognise CJK closing marks as sentence-ending only as terminal combos (。」, !」, …), never bare — a bare closing quote also wraps a mid-sentence quoted term (これは「AI」について話します。), which must keep accumulating rather than split.
  • Don't inject an ASCII space when joining adjacent CJK fragments, so a Japanese sentence spanning multiple word-timed cues renders without spurious gaps between characters (これはペンです。, not これは ペンです。).
  • Single-source the CJK terminator/closer sets, and add unit tests for sentence/mid-sentence detection, the no-space join, terminal-combo splitting, the bare-closer case, mixed-script quoting, and CJK timing/word-offset preservation.

Extended to other non-Latin scripts (follow-up commits in this PR — the same root cause affected every script the punctuation list didn't know about):

  • Recognise sentence terminators for other scripts with their own sentence-final punctuation: Arabic (؟), Urdu (۔), Devanagari/Hindi ( ), Burmese (), Khmer ( ), Ethiopic (), and Armenian (։).
  • Cap the phrase accumulator for transcripts no terminator list can ever segment — Thai has no sentence-final punctuation, and unpunctuated auto-generated captions are common. Once the accumulated phrase outgrows a sentence-sized length (160 chars), it flushes at the next cue boundary, so every transcript yields multiple scrollable entries and the auto-scroll always has per-line anchors, regardless of script.
  • Extend the no-space join rule from CJK to all space-less scripts (Thai, Lao, Khmer, Burmese), so their rejoined fragments render without injected gaps.

Pure-Latin transcripts are unaffected — the new punctuation is endsWith-matched and can't appear at the end of Latin text, and the accumulator cap only triggers where no sentence boundary was found for 160+ characters (punctuated text flushes on terminators long before that).

Fixes PCDROID-639

Out of scope / follow-ups (from review)
  • AI context window for CJKTranscriptWindowExtractor.windowText() gates on split("\s+").size >= MIN_WORDS; CJK has no whitespace, so AI bookmark/summary enrichment silently returns nothing for Japanese podcasts. Pre-existing, separate feature; not addressed here.
  • Nested quoted speechfindMidSentence() prefers a terminator+quote combo over a later bare terminator, so 彼は「はい。」と答えた。それから帰った splits at the inner 。」 rather than the true outer . Leftover is preserved (degraded, not lost). Mirrors the existing Latin quotation-first heuristic.
  • More CJK closers — only 」 』 ) are covered; lenticular/angle brackets (】 》 〉 etc.) used in some Japanese headings are not yet recognised as terminal-combo closers.
  • Generalisation — sentence detection is a hand-maintained character list; java.text.BreakIterator.getSentenceInstance(Locale) could replace it locale-aware, but isn't a clean drop-in given the surrounding split/timing/offset logic.
  • U+3000compactWhiteSpace() doesn't normalise the ideographic space   (cosmetic, pre-existing).

Testing Instructions

Automated:

  • Run the transcript sanitization unit tests on JDK 21: ./gradlew :modules:services:repositories:testDebugUnitTest --tests "*TranscriptSanitizationTest*" — 114 tests, 0 failures (includes the CJK cases, the new-script terminators, the Thai/unpunctuated cue-boundary flush, and the Thai/Devanagari spacing cases).

On device (see the before/after recordings below):

  1. With synced_transcripts enabled and a Plus account, play an episode of a Japanese-language podcast that has a synced transcript — e.g. EASY JAPANESE PODCAST (podcast UUID 08b59600-4245-0139-3351-0acc26574db2).
  2. Open the transcript / "follow along" view and let it play without touching the screen.
  3. Confirm:
    • Each newly highlighted line auto-scrolls to stay in view (anchored ~30% from the top) instead of jumping to the top.
    • The Japanese text renders without stray spaces between characters.

Latin transcripts are unaffected (covered by the existing Latin unit tests; the new punctuation can't match Latin text).

Screenshots or Screencast

Before:

Screen_Recording_20260630_135738_Pocket.Casts.mp4

After:

Screen_Recording_20260630_203717_Pocket.Debug.mp4

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews
  • I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics.

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

Japanese (and other CJK) transcripts end sentences with 。!? and use 」』
quotes, none of which were in the sentence-splitting punctuation lists. So
joinSplitSentences() never split a CJK transcript and collapsed it into a single
TranscriptEntry, leaving the synced "follow along" auto-scroll with no per-line
anchor — it stayed pinned to the top while only the highlighted word advanced.
Recognise CJK sentence terminators and closing brackets so CJK transcripts split
per sentence again and the auto-scroll can anchor each line.

PCDROID-639
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @joashrajin's task in 1m 59s —— View job


Code Review — PR #5494: CJK transcript auto-scroll fix

  • Read PR diff
  • Analyze TranscripSanitization.kt changes
  • Analyze test coverage
  • Assess correctness, edge cases, performance
  • Post review

Overall this is a well-targeted, low-risk fix with a clear root-cause writeup and strong test coverage. The change is purely additive to two punctuation lists, and the new characters are endsWith/findLastAnyOf-matched so pure-Latin transcripts genuinely can't regress. The regression test (keep CJK sentences as separate entries) directly exercises the reported bug, and the parameterized tests cover both full-sentence and mid-sentence detection for the new terminators and 。」/ combos. Nice work.

A few observations, none blocking:

1. CHANGELOG entry is checked off but missing (should fix)

The PR checklist marks "I have added an entry in CHANGELOG.md" as done, but the diff only touches the two transcript files — there's no CHANGELOG change. This is a user-facing bug fix, so it warrants an entry.
Fix this →

2. Cross-product produces spurious mixed-locale combos (minor / cosmetic)

MidSentenceQuotationPunctuation (TranscripSanitization.kt:226) is a full cross-product of all 7 terminators × all 6 quotes. After this change it now generates nonsensical pairs such as .", .」, 。", 。” alongside the meaningful 。」/!」/?」/。』. These can never match real text, so they're harmless, but they do bloat the list and slightly muddy intent. Not worth changing unless you want to be tidy — if you do, pairing CJK punctuation only with CJK quotes (and Latin with Latin) would express the intent more precisely.

3. / are end-of-sentence but not standalone mid-sentence terminators (intentional, just confirming)

You added / to EndOfSentencePunctuation and to the quotation mid-sentence set, but not to bare MidSentencePunctuation. That means a cue like 「こんにちは」続き… won't split at the standalone . This is consistent with the existing Latin behavior (bare )/] are end-of-sentence but not mid-sentence splitters), so I believe it's the correct/intended choice — flagging only so it's a conscious decision rather than an oversight.

4. Space-joining cosmetic issue — worth prioritizing among the follow-ups

You've correctly scoped this out, but I'd highlight it as the most user-visible of the deferred items: a Japanese sentence spread across word-timed cues now renders as こ ん に ち は 。 (spaces inserted by appendToAccumulator at TranscripSanitization.kt:40). Since the EASY JAPANESE PODCAST repro is exactly the per-word-timed case, users following this PR's fix will likely see those spaces immediately. The scroll anchoring (this PR's goal) is unaffected, so deferring is fine — just suggest filing the follow-up promptly.

Testing

  • The --tests "*TranscriptSanitizationTest*" run is the right verification and the new cases look correct under trace-through.
  • The on-device repro remains unverified (acknowledged in the description). Given the fix is data-driven and unit-tested, that's a reasonable residual risk, but a short screencast on a real Japanese synced transcript would be the definitive confirmation before merge.

Verdict: Correct, well-tested, and safe. I'd recommend adding the CHANGELOG entry before merge; the rest are optional polish / follow-ups.

@joashrajin joashrajin added [Type] Bug Not functioning as intended. [Project] Transcripts labels Jun 30, 2026
- Don't inject an ASCII space when joining adjacent CJK fragments, so a
  multi-cue Japanese sentence renders without spurious gaps between characters.
- Treat CJK closing marks (」』)) as sentence-ending only as terminal combos
  (。」, !」, …) rather than bare, so a mid-sentence quoted term like 「AI」 sitting
  at a cue boundary no longer splits the sentence.
- Recognise half/full-width full stops 。 and . as terminators.
- Single-source the CJK terminator/closer sets and add tests for the no-space
  join, terminal-combo splitting, the bare-closer case, and CJK timing/offsets.

PCDROID-639
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @joashrajin's task in 2m 37s —— View job


Code Review — PR #5494: CJK transcript auto-scroll fix

  • Read PR diff (incl. latest commits)
  • Analyze TranscripSanitization.kt
  • Analyze test coverage
  • Verify behavior (static trace-through; Gradle blocked — see note)
  • Post review

Re-reviewed after the latest commits. Since my first pass the PR has addressed every actionable point: the CHANGELOG entry is now present, and the cosmetic CJK space-joining issue I'd flagged as the top follow-up has been fixed directly via needsSpaceBetween/isCjk. This is a clean, well-scoped fix and I'd be comfortable merging it.

What's good

  • Root cause fix is correct. Adding the CJK terminators/combos to EndOfSentencePunctuation, MidSentencePunctuation, and the quotation set makes joinSplitSentences() produce per-sentence TranscriptEntry blocks, which restores the per-line entryIndex that AutoScrollEffect anchors on. The collapse-to-single-entry behavior is exactly what caused the jump-to-top.
  • Space-joining now handled (TranscripSanitization.kt:35-48, :57-64). CJK fragments rejoin without injected spaces (こんにちは。 not こ ん に ち は 。), while Latin/mixed boundaries keep their spaces. The appendToAccumulator rewrite also correctly skips empty fragments, avoiding the old stray leading/trailing space.
  • The //-as-terminal-combo-only design is right. Treating bare closers as end-of-sentence only when preceded by a terminator (the CjkTerminalCombos cross-product) means これは「AI」について話します。 keeps accumulating instead of splitting at the inline quote — and there's a dedicated regression test for exactly that.
  • Strong test coverage. I traced the timing/offset test (split CJK mid-sentence preserves timing and word offsets) by hand — the 625ms proportional split and 続きは/次の文。 offsets (0–3, 3–7) are correct. The parameterized tables now cover the new terminators, combos, and bare closers in the right sets.
  • No Latin regression. New punctuation is endsWith/findLastAnyOf-matched and can't appear at the end of Latin text; the space logic returns true for any non-CJK boundary, preserving prior behavior.

Minor / non-blocking (left as inline comments)

  1. Surrogate-pair CJK (isCjk, line 37–48): Character.UnicodeBlock.of(Char) doesn't see supplementary-plane ideographs (CJK Ext B+), so a join touching one would re-insert a space. Cosmetic, BMP is covered.
  2. Latin↔CJK boundary across a cue split (line 60): a ('I', '」') boundary inserts a space before the closer (「AI 」). Only when a Latin token is split across cues right at a bracket — rare and cosmetic.
  3. Mixed-locale cross-product combos (carried over from last review): MidSentenceQuotationPunctuation and CjkTerminalCombos generate some impossible pairs (。", ."). Harmless dead entries; only tidy if you want precise intent.

Testing note

I couldn't execute :modules:services:repositories:testDebugUnitTest here — the Gradle command needs an approval this job doesn't have. I verified the new cases by tracing the code paths instead, and they hold up. The author reports the suite passing locally (76 tests, 0 failures); CI on this PR is the authoritative confirmation. The on-device Japanese-transcript repro remains the one outstanding manual check (acknowledged in the description) — a short screencast would be the definitive sign-off.

Verdict: Correct, well-tested, and responsive to prior feedback. No blocking issues — the three inline notes are optional polish/follow-ups.

- Suppress the join space whenever either side of a boundary is CJK (not only
  when both are), so no stray space hugs a CJK closing mark (「AI」, not 「AI 」).
- Inspect boundary code points so supplementary-plane ideographs (CJK Ext B+,
  encoded as surrogate pairs) are recognised.
- Build the mid-sentence quotation combos per script (Latin x Latin, CJK x CJK)
  instead of a full cross-product, dropping the impossible mixed-locale pairs.

PCDROID-639
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @joashrajin's task in 1m 56s —— View job


Code Review — PR #5494: CJK transcript auto-scroll fix

  • Read latest diff (all 4 commits)
  • Analyze TranscripSanitization.kt
  • Analyze test coverage
  • Assess correctness, edge cases, performance
  • Post review

Re-reviewed after the latest push (Refine CJK… + Polish CJK join handling). The notable thing this round: every actionable point from the previous reviews is now fixed in code, not just deferred. This is a clean, well-scoped, well-tested change and I'd be comfortable merging it.

Prior feedback — now resolved

  • Surrogate-pair CJK (my earlier inline nit): isCjk now operates on a code point (Character.isIdeographic(Int) + the UnicodeBlock.of(Int) overload) and needsSpaceBetween reads Character.codePointBefore/codePointAt, so supplementary-plane ideographs (CJK Ext B+) are recognised. The new test do not insert spaces around supplementary-plane CJK ideographs (𠀋) locks this in. ✅
  • Latin↔CJK closer across a cue boundary (the ('I', '」') case): needsSpaceBetween now inserts a space only when neither side is CJK (!(left.isCjk() || right.isCjk())). Since is CJK, 「AI + 」について joins as 「AI」について with no stray gap — covered by do not insert a space before a CJK closer after a Latin run. ✅
  • Mixed-locale cross-product combos: MidSentenceQuotationPunctuation is now Latin×Latin + CjkTerminalCombos (= Cjk×Cjk), so the impossible 。"/." pairs are gone and intent is explicit (per the comment at TranscripSanitization.kt:252). ✅
  • CHANGELOG: present and correctly placed under 8.16 → Bug Fixes. ✅

What's correct in the current state

  • Root-cause fix holds. CJK terminators (。!?。.) in EndOfSentencePunctuation/MidSentencePunctuation make joinSplitSentences() emit per-sentence TranscriptEntry blocks again, restoring the per-line entryIndex that AutoScrollEffect anchors on. Regression test keep CJK sentences as separate entries exercises exactly the reported bug.
  • Bare-closer design is right. 」 』 ) are end-of-sentence only as CjkTerminalCombos (terminator+closer), never standalone — so これは「AI」について話します。 keeps accumulating instead of splitting at the inline quote (do not treat a bare CJK closing mark as a sentence end). This is consistent with the existing Latin )/] behavior.
  • Timing/offset math verified by hand. In split CJK mid-sentence preserves timing and word offsets, the 625ms proportional split (splitTimeMs: 1000 * 5/8 = 625) and the 続きは(0–3)/次の文。(3–7) offsets in 続きは次の文。 are all correct.
  • No Latin regression. New punctuation is endsWith/findLastAnyOf-matched and can't terminate Latin text; needsSpaceBetween returns true for any all-Latin boundary, preserving prior spacing (MidSentenceIgnoredPatterns confirms bare closers don't split).

Non-blocking observations (no change required)

  1. appendToAccumulator builds a single WordTiming per fragment. A fragment that is itself multiple words (e.g. "Period. Unfinished") is stored as one timing rather than per-word. This is pre-existing behavior, unchanged by this PR, and the offset recalculation/tests account for it — just noting it isn't a new regression.
  2. Genuinely-deferred items remain reasonable to defer: nested quoted speech splitting at the inner 。」, the AI windowText() MIN_WORDS whitespace gate for CJK, additional closers (】 》 〉), and U+3000 ideographic-space normalisation in compactWhiteSpace (its \s/[ \t] regexes don't match  ). All cosmetic or pre-existing and clearly scoped out in the description.

Testing

  • The added cases trace through correctly; --tests "*TranscriptSanitizationTest*" is the right gate and CI on this PR is the authoritative pass/fail (I can't run Gradle in this job).
  • The one outstanding manual check is still the on-device Japanese synced-transcript repro (acknowledged as a TODO in the description). Given the data-driven nature and the unit coverage, that's low residual risk — a short screencast would be the definitive sign-off before merge.

Verdict: Correct, well-tested, and fully responsive to prior feedback. No blocking issues.

A Japanese sentence can be wrapped in ASCII / curly quotes (彼は"はい。") and English
can appear in CJK brackets (「OK.」), so terminator + closer sentence boundaries
cross scripts. Build the terminal-combo set as the full terminator x closer
cross-product across both scripts (not same-script only), so findMidSentence keeps
the closing quote with its sentence instead of splitting before it.

PCDROID-639
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @joashrajin's task in 1m 46s —— View job


Code Review — PR #5494: CJK transcript auto-scroll fix

  • Read latest diff (newest commit Keep closing quotes attached to mixed-script CJK sentences)
  • Analyze TranscripSanitization.kt
  • Analyze test coverage
  • Assess correctness, edge cases, performance
  • Post review

Re-reviewed after the latest push. This commit reverses an earlier decision (the previous round had split combos into same-script-only, Latin×Latin + CJK×CJK) and goes back to a full cross-product — but with a documented reason: real transcripts mix scripts and quote styles (Japanese speech in ASCII/curly quotes 彼は"はい。", English in CJK brackets 「OK.」), so a terminator and its closer can legitimately come from different scripts. The change consolidates everything into single-sourced SentenceTerminators/ClosingMarks and one TerminalCombos = terminators × closers. This is a sound, well-justified call and the new mixed-script test (keep a Latin closing quote attached to a CJK sentence) is exactly the right justification.

What's correct in the current state

  • Mixed-script combo handling is right. 彼は"はい。"それから now splits at 。" (combo found via findLastAnyOf(MidSentenceQuotationPunctuation)) instead of detaching the trailing " onto the next line. Traced by hand against the new test — passes.
  • No Latin regression. The new cross-script pairs (.」, 。", …) are endsWith/findLastAnyOf-matched and their CJK halves can't appear in pure-Latin text, so Latin transcripts are untouched.
  • Bare-closer design still holds. 」 』 ) remain sentence-ends only as terminal combos, never standalone, so これは「AI」について話します。 keeps accumulating (no . precedes the inner , so no combo matches there). The dedicated regression test still covers this.
  • Root-cause fix intact. CJK terminators in EndOfSentencePunctuation/MidSentencePunctuation keep producing per-sentence TranscriptEntry blocks, restoring the per-line entryIndex that AutoScrollEffect anchors on — the actual bug.
  • Single-sourcing is a real readability win. SentenceTerminators/ClosingMarks/TerminalCombos now have one definition each, used by both endsAsSentence() and findMidSentence(); the previous duplication between the end-of-sentence and mid-sentence sets is gone.

One trade-off worth a conscious note (non-blocking)

Broadening the combo set slightly widens the already-documented "nested quoted speech" degradation. Adding .」 to MidSentenceQuotationPunctuation means a mixed-script nested quote like 彼は「I am happy.」と言った。 now splits at the inner .」 (leftover と言った。) rather than staying whole. Before this commit, .」 wasn't a recognized combo, so that exact string fell through to the bare-terminator fallback and stayed as one sentence. It's the same class of limitation already called out in the PR's "nested quoted speech" follow-up (the quotation-first heuristic preferring an inner combo over a later bare terminator), now reachable by one more script combination. The leftover is preserved (degraded, not lost), it's an edge case, and it's consistent with the existing heuristic — so deferring is fine, but it's worth being a conscious bound rather than a surprise.

Carried-forward items (all still reasonable)

  • The mixed-script cross-product now produces some pairs that are individually unusual (。", .」) but are all intentional per the new commit's rationale — they're no longer "dead" entries, they're the point of the change. The earlier "spurious combos" note is resolved by making the intent explicit.
  • Genuinely-deferred follow-ups remain reasonable: nested quoted speech, the AI windowText() MIN_WORDS whitespace gate for CJK, more closers (】 》 〉), and U+3000 normalisation. All cosmetic/pre-existing and scoped out.

Testing

  • New parameterized cases (。", 。”, .」) and the mixed-script entry test trace through correctly; --tests "*TranscriptSanitizationTest*" is the right gate and CI on this PR is authoritative (I can't run Gradle in this job).
  • The on-device Japanese synced-transcript repro remains the one outstanding manual check (acknowledged as a TODO in the description). Given the data-driven nature and unit coverage, low residual risk — a short screencast would be the definitive sign-off.

Verdict: Correct, well-tested, and the mixed-script broadening is justified. No blocking issues; the nested-quote trade-off is the only thing worth a deliberate ack.

@joashrajin joashrajin self-assigned this Jun 30, 2026
@joashrajin
joashrajin marked this pull request as ready for review June 30, 2026 19:17
@joashrajin
joashrajin requested a review from a team as a code owner June 30, 2026 19:17
@joashrajin
joashrajin requested review from sztomek and removed request for a team June 30, 2026 19:17
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @joashrajin's task in 1m 31s —— View job


Code Review — PR #5494: CJK transcript auto-scroll fix

  • Read PR diff (all 5 commits)
  • Analyze TranscripSanitization.kt
  • Analyze test coverage
  • Assess correctness, edge cases, performance
  • Post review

Re-reviewed at ready_for_review against the current head (Keep closing quotes attached to mixed-script CJK sentences). This is a clean, well-scoped, well-tested fix and I'd be comfortable merging it. CI on the PR is green (5 passed, 0 failed). Every actionable point from the earlier review rounds is now resolved in code — single-sourced punctuation sets, code-point-based CJK detection (surrogate-safe), no-space CJK joins, and the CHANGELOG entry are all in place.

Correctness — verified by trace-through

  • Root cause is fixed. Adding the CJK terminators (。!?。.) to SentenceTerminators (hence EndOfSentencePunctuation + MidSentencePunctuation) makes joinSplitSentences() emit per-sentence TranscriptEntry blocks again, which restores the per-line entryIndex that AutoScrollEffect anchors on. keep CJK sentences as separate entries exercises the exact reported bug.
  • Bare-closer design is right. 」 』 ) are end-of-sentence only as TerminalCombos (terminator+closer), never standalone, so これは「AI」について話します。 keeps accumulating. Confirmed by do not treat a bare CJK closing mark as a sentence end and MidSentenceIgnoredPatterns.
  • Mixed-script combos hold. I traced 彼は"はい。"それから + 帰った。: findLastAnyOf(TerminalCombos) matches 。" at the right index, the closer stays attached, and the leftover それから rejoins 帰った。 space-free (both CJK). Matches keep a Latin closing quote attached to a CJK sentence.
  • Spacing logic is sound. needsSpaceBetween returns false when either boundary code point is CJK, so 「AI + 」について joins without a stray gap, and the surrogate-pair case (𠀋) is covered via Character.codePointBefore/codePointAt + isIdeographic(Int).
  • Timing/offset math checks out. In split CJK mid-sentence preserves timing and word offsets, splitTimeMs = 1000 * 5/8 = 625 and the 続きは(0–3)/次の文。(3–7) offsets are correct.
  • No Latin regression. All new punctuation is endsWith/findLastAnyOf-matched and its CJK halves can't terminate Latin text; needsSpaceBetween returns true for any all-Latin boundary, preserving prior spacing. The original Latin parameterized tests are unchanged and still pass.

Non-blocking notes (no change required)

  1. HALFWIDTH_AND_FULLWIDTH_FORMS is broad — it also covers fullwidth ASCII (e.g. , fullwidth digits). Treating those as "CJK" for spacing is the correct outcome here, just worth knowing the block isn't strictly ideographic.
  2. Nested mixed-script quotes — broadening TerminalCombos to the full cross-product means a string like 彼は「I am happy.」と言った。 now splits at the inner .」 rather than the outer (leftover preserved, not lost). This is the same documented quotation-first heuristic limitation, now reachable by one more script combination — a conscious, bounded trade-off, already in the "nested quoted speech" follow-up.
  3. Deferred items remain reasonable: the AI windowText() MIN_WORDS whitespace gate for CJK, additional closers (】 》 〉), and U+3000 ideographic-space normalisation in compactWhiteSpace. All cosmetic/pre-existing and clearly scoped out.

Testing

  • The new parameterized + scenario cases trace through correctly; --tests "*TranscriptSanitizationTest*" is the right gate and PR CI is the authoritative pass (green here).
  • The one outstanding manual check is the on-device Japanese synced-transcript repro (acknowledged as TODO; the before/after recordings in the description cover it). Given the data-driven nature and the unit coverage, residual risk is low.

Verdict: Correct, well-tested, and fully responsive to prior feedback. No blocking issues.

@joashrajin joashrajin added this to the 8.18 milestone Jul 16, 2026
…ynced-transcript-auto-scroll-jumps-back-to-top-of

# Conflicts:
#	CHANGELOG.md
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @joashrajin's task in 1m 30s —— View job


Code Review — PR #5494: CJK transcript auto-scroll fix

  • Read PR diff (all commits, incl. main merge)
  • Analyze TranscripSanitization.kt
  • Analyze test coverage
  • Assess correctness, edge cases, performance
  • Post review

Re-reviewed after the latest sync (main merged into the branch — 5c0a0ae). No source changes since the last round; the fix carries forward cleanly and CHANGELOG is now under 8.18 → Bug Fixes. This remains a clean, well-scoped, well-tested change with no blocking issues.

Correctness — holds up under trace-through

  • Root cause fixed. CJK terminators (。!?。.) in SentenceTerminatorsEndOfSentencePunctuation/MidSentencePunctuation make joinSplitSentences() emit per-sentence TranscriptEntry blocks again, restoring the per-line entryIndex that AutoScrollEffect anchors on. keep CJK sentences as separate entries targets the exact bug.
  • Bare-closer design is right. 」 』 ) are sentence-ends only as TerminalCombos (terminator+closer), never standalone, so これは「AI」について話します。 keeps accumulating. Confirmed by do not treat a bare CJK closing mark as a sentence end.
  • Spacing logic is sound and surrogate-safe. needsSpaceBetween inspects boundary code points (codePointBefore/codePointAt + isIdeographic(Int)), so BMP and supplementary-plane ideographs (𠀋) both suppress the injected space; a space is inserted only when neither side is CJK, preserving Latin behavior.
  • Timing/offset math checks out. split CJK mid-sentence preserves timing and word offsets: splitTimeMs = 1000 * 5/8 = 625 and the 続きは(0–3)/次の文。(3–7) offsets are correct.
  • No Latin regression. New punctuation is endsWith/findLastAnyOf-matched and its CJK halves can't terminate Latin text; the original Latin parameterized cases are untouched.
  • Single-sourcing is a genuine readability win. SentenceTerminators/ClosingMarks/TerminalCombos each have one definition, shared by endsAsSentence() and findMidSentence().

Non-blocking notes (no change required — consistent with prior rounds)

  1. Nested mixed-script quotes. The full-cross-product TerminalCombos means 彼は「I am happy.」と言った。 splits at the inner .」 rather than the outer (leftover preserved, not lost). Same documented quotation-first heuristic bound, already in the "nested quoted speech" follow-up.
  2. HALFWIDTH_AND_FULLWIDTH_FORMS is broad — also covers fullwidth ASCII (, fullwidth digits). Treating those as CJK-for-spacing is the desired outcome; just noting the block isn't strictly ideographic.
  3. Deferred items remain reasonable: AI windowText() MIN_WORDS whitespace gate for CJK, additional closers (】 》 〉), and U+3000 ideographic-space normalisation in compactWhiteSpace. All cosmetic/pre-existing and clearly scoped out.

Testing

  • The parameterized + scenario cases (sentence/mid-sentence detection, no-space join, terminal combos, bare closer, mixed-script quoting, surrogate-pair, CJK timing/offsets) trace through correctly. --tests "*TranscriptSanitizationTest*" is the right gate; PR CI is authoritative (I can't run Gradle in this job).
  • The one outstanding manual check is the on-device Japanese synced-transcript repro — covered by the before/after recordings in the description. Given the data-driven nature and unit coverage, residual risk is low.

Verdict: Correct, well-tested, and fully responsive to the prior review rounds. No blocking issues.

… transcripts

- Add sentence terminators for Arabic (؟), Urdu (۔), Devanagari (। ॥),
  Burmese (။), Khmer (។ ៕), Ethiopic (።), and Armenian (։) so those
  transcripts split into per-sentence entries for auto-scroll anchoring.
- Cap the phrase accumulator: scripts with no sentence-final punctuation
  (Thai) and unpunctuated auto-generated captions can never match a
  terminator, so flush at the cue boundary once the accumulated phrase
  outgrows a sentence-sized length. Every transcript now yields multiple
  scrollable entries regardless of script.
- Extend the no-space join rule to all space-less scripts (Thai, Lao,
  Khmer, Burmese) instead of CJK only.
- Extract flushAccumulatedPhrase() so the forced flush, sentence flush,
  and trailing flush share one implementation.
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @joashrajin's task in 3m 23s —— View job


Code Review — PR #5494 (synchronize)

Re-reviewed after the new commit Extend sentence splitting beyond CJK: other scripts and unsegmentable transcripts (f9689c2).

  • Read the new commit diff vs. prior state
  • Analyze TranscripSanitization.kt changes
  • Analyze test coverage
  • Assess correctness, edge cases, performance
  • Post review

This commit generalises the PCDROID-639 fix from CJK to every script — and, importantly, to scripts and captions that have no sentence-final punctuation at all. It's the logical completion of the fix: the root cause was "one giant TranscriptEntry → no per-line entryIndex for AutoScrollEffect to anchor on," and that failure mode isn't CJK-specific. Solid, well-scoped, and well-tested. No blocking issues.

What's correct — verified by trace-through

  • New terminators are right. OtherScriptSentenceTerminators (؟ ۔ । ॥ ။ ។ ៕ ። ։) covers Arabic, Urdu, Devanagari, Burmese, Khmer, Ethiopic, and Armenian sentence-final marks, and they flow into SentenceTerminatorsEndOfSentencePunctuation/MidSentencePunctuation/TerminalCombos the same way the CJK set does. The Devanagari regression test mirrors the CJK one exactly. Notably these are all genuine sentence terminators (e.g. Ethiopic full stop U+1362, not the word-separator U+1361 — correctly excluded), so splitting on them is semantically right.
  • Accumulator cap handles the unsegmentable case. The MAX_ACCUMULATED_PHRASE_LENGTH = 160 flush at TranscripSanitization.kt:145 is checked at the start of each cue against the previous accumulation, so it flushes cleanly at a cue boundary rather than mid-token. I traced both new tests: flush Thai transcript at cue boundaries (4×80 space-less chars → two 160-char entries, times [0,2000]/[2000,4000]) and flush unpunctuated captions at cue boundaries (4×80 Latin → two "$cue $cue" = 161-char entries) both hold, including the timing merge.
  • Space-less join extended correctly. isSpacelessScript() adds THAI/LAO/KHMER/MYANMAR alongside CJK, and Arabic/Devanagari/Ethiopic/Armenian are deliberately left out — they use inter-word spaces. The insert spaces when joining Devanagari fragments vs do not insert spaces when joining Thai fragments pair pins exactly this distinction. Good.
  • flushAccumulatedPhrase() extraction is clean. The forced flush, buildFullSentence, and the trailing flush now share one implementation; the empty-entry guards (length >= 160, isNotEmpty(), append-before-flush) mean no spurious empty Text entries are produced, and the sanitize() pipeline's filter(isNotEmpty) would drop any anyway.
  • No Latin regression on the terminator side. The new punctuation is endsWith/findLastAnyOf-matched and can't terminate Latin text; the original Latin parameterized cases are untouched.

Non-blocking observations

  1. The cap is a genuine behaviour change for all long unpunctuated runs, including Latin — not just the newly-supported scripts. A punctuated sentence that legitimately runs past 160 chars while spanning multiple cues without an internal terminator will now be force-split mid-sentence at a cue boundary, where previously it stayed whole. For normal punctuated content this never triggers (sentences flush on terminators well under 160), and the degraded result is still readable and scrollable — so this is a reasonable, intended trade-off. Worth being a conscious bound rather than a surprise, and a one-line note in the PR description's "out of scope" block would document it.
  2. Cap is UTF-16 length, not code points. Supplementary-plane ideographs (surrogate pairs) count as 2 toward the 160 cap, so it triggers marginally earlier for such text. Purely cosmetic; the entry text itself is unaffected.
  3. HALFWIDTH_AND_FULLWIDTH_FORMS in isSpacelessScript() also covers fullwidth ASCII (, fullwidth digits) — treating those as space-less is the desired outcome here, just noting the block isn't strictly ideographic (carried over from prior rounds).
  4. Speaker entries don't flush the accumulator (:137) — a Speaker appearing mid-accumulation is emitted before the pending text flushes after it, a pre-existing ordering quirk unchanged by this commit; the cap doesn't worsen it.

Testing

  • New parameterized rows (the 9 other-script terminators) are added to both the end-of-sentence and mid-sentence tables, and the four scenario tests (Devanagari split, Devanagari space-join, Thai no-space, Thai/Latin cap flush) trace through correctly.
  • --tests "*TranscriptSanitizationTest*" remains the right gate. On this run I can see Gradle-wrapper validation + Danger are green and the full unit suite runs on Buildkite (authoritative). The on-device repro (now ideally spot-checked with a non-CJK non-Latin transcript too — e.g. Hindi or Thai — given the broadened scope) stays the one manual sign-off, consistent with prior rounds.

Verdict: Correct, well-tested, and a sensible generalisation of the original fix. No blocking issues; the accumulator-cap behaviour change (note 1) is the only item worth a deliberate ack.

@joashrajin joashrajin changed the title Fix synced transcript auto-scroll for Japanese (CJK) transcripts Fix synced transcript auto-scroll for non-Latin transcripts (CJK, Thai, Hindi, Arabic, and more) Jul 16, 2026
@wpmobilebot wpmobilebot modified the milestones: 8.18, 8.19 Aug 3, 2026
@wpmobilebot

Copy link
Copy Markdown
Collaborator

Version 8.18 has now entered code-freeze, so the milestone of this PR has been updated to 8.19.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Project] Transcripts [Type] Bug Not functioning as intended.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants