refactor(subtitles): simplify state machine and improve loading display

[taiiiyang]

Type of Changes

• ♻️ Code refactoring (refactor)

Description

• Simplify subtitle state machine: Reduce state types from 6 to 3 (idle, loading, active), removing redundant intermediate states (fetching-subtitles, waiting-for-cue, translating)
• Extract display rules: Create display-rules.ts module with pure functions (shouldShowLoading, shouldShowSubtitles) to decouple display logic from state management
• Move loading indicator to bottom: Position loading state message at bottom-20 instead of center, so it doesn't block video content
• Add "Read Frog" prefix to i18n messages: All subtitle status messages now prefixed with "Read Frog |" for clear branding (8 locale files updated)
• Simplify StateMessage component: Remove internal state tracking, derive display directly from props

How Has This Been Tested?

• Added unit tests
• Verified through manual testing

Unit tests for display-rules.ts covering all state × translation combinations (8 test cases).

All checks pass:

• pnpm type-check ✓
• pnpm test — 554 tests passed
• pnpm lint — clean (only pre-existing sidebar warning)

Checklist

• I have tested these changes locally
• I have updated the documentation accordingly if necessary
• My code follows the code style of this project
• My changes do not break existing functionality
• If my code was generated by AI, I have proofread and improved it as necessary.

---

Summary by cubic

Simplifies the subtitle state to idle/loading/error and refactors display logic so loading shows only when the current cue has no renderable subtitle, with loading at the bottom and error centered.

• Refactors
  • Reduced states from 6 to 3; adapters now set 'loading' during fetch and processing.
  • Added utils/subtitles/display-rules.ts and used it in SubtitlesContainer/SubtitlesView to decide visibility; loading is hidden when content is renderable.
  • TranslationCoordinator now emits deduped state changes via lastEmittedState and updateLoadingStateAt, based on the active cue; removed segmenting/processing transitions.
  • StateMessage removed idle config and derives text/color/position from props; loading at bottom, error centered.
  • Updated loading copy with “Read Frog” prefix across 8 locales; added unit tests for display rules.

^{Written for commit 148a3ac. Summary will update on new commits.}

[changeset-bot]

🦋 Changeset detected

Latest commit: 148a3ac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@read-frog/extension	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dosubot]

Related Documentation

Checked 14 published document(s) in 1 knowledge base(s). No updates required.

^{How did I do? Any feedback?}  

[claude]

PR Review: refactor(subtitles): simplify state machine and improve loading display

Overall Assessment

Clean refactor. Reducing 6 states to 3 and extracting pure display rules is the right direction — it eliminates special cases and makes the state machine easier to reason about.

---

Code Quality

Good:

• display-rules.ts is pure, testable, and has clear single responsibility
• deriveSubtitleDisplayDecision creates a clean decision matrix — the logic is declarative and obvious
• hasRenderableSubtitleByMode correctly handles the translationOnly edge case (subtitle exists but no translation yet → not renderable)
• Removing currentSubtitleAtom from subtitles-view.tsx is correct — it was never consumed there directly; subtitle-lines.tsx handles the actual rendering

Suggestions:

• findActiveCue uses Array.find() (linear scan). For large subtitle lists this is O(n) on every tick. Not a problem now, but if fragment counts grow significantly, binary search would be better since fragments are time-sorted. Low priority — noting for awareness.

---

Potential Issues

1. Redundant updateLoadingStateAt calls in translateNearby

   In translation-coordinator.ts, handleTranslationTick already calls updateLoadingStateAt, then immediately calls translateNearby. Inside translateNearby, updateLoadingStateAt is called again in two places (empty batch return at ~line 113, and before starting translation at ~line 116). After translation completes, it's called a third time (~line 130).

   The calls at lines ~113 and ~116 happen in the same tick as the one in handleTranslationTick, so fragments/time haven't changed — those two calls are redundant. Only the post-translation call (~line 130, using getCurrentVideoTimeMs + fresh getFragments()) is meaningful because time has actually advanced.

   Consider removing the two redundant calls inside translateNearby (the ones before the try block) and keeping only the post-translation one. This would reduce unnecessary onStateChange invocations.

2. idle state text is defined but may never display

   StateMessage returns null when stateData is null or state is idle (line 28-30 of state-message.tsx). The STATE_CONFIG for idle defines text/color/position, but this config is dead code since it's never reached. Consider removing the idle entry from STATE_CONFIG or adding a comment noting it's unreachable.

3. State flicker potential

   updateLoadingStateAt transitions to idle when there's no active cue (between subtitle gaps). Then when the next cue starts, it transitions to loading if untranslated. This rapid idle → loading → idle cycling during gaps could cause visual flicker of the state message on/off. The <Activity> component may handle this gracefully via its hidden mode, but worth verifying in manual testing — particularly during sections where subtitles have short gaps between cues.

---

Performance

• The getFragments() callback is invoked multiple times per tick (in handleTranslationTick and again in translateNearby). Since it reads from segmentationPipeline.processedFragments or this.processedFragments, these should be cheap property accesses. No concern here.
• deriveSubtitleDisplayDecision is called on every render of SubtitlesContainer. It's pure and trivial — no issue.

---

Security

No security concerns. Changes are purely UI/state management with no external input handling, network calls, or storage changes.

---

Test Coverage

• Unit tests for display-rules.ts cover the full decision matrix: all 3 states × relevant display modes. Good coverage.
• Missing test case: hasRenderableSubtitleByMode with translationOnly and a subtitle where translation is an empty string (''). Currently !!'' returns false, which is likely the desired behavior, but an explicit test would document this edge case.
• TranslationCoordinator.updateLoadingStateAt is not directly unit-tested. Its logic is simple, but testing the state transitions (no cue → idle, untranslated cue → loading, translated cue → idle) would prevent regressions.

---

i18n

• All 8 locale files updated consistently — good.
• The "Read Frog" brand prefix is hardcoded in translation strings rather than parameterized. This is fine for a brand name that shouldn't be localized.
• The fetchFailed state was removed but error state remains. The old fetchFailed message gave users a specific hint about what failed. Now all errors go through a generic "Error occurred" message, with specifics relying on stateData.message. Confirm that all error paths (fetch failure, translation failure, etc.) actually populate stateData.message to avoid users seeing only a generic error.

---

Summary

Area	Rating
State simplification	Clean reduction, well-motivated
Display rules extraction	Good separation of concerns
Test coverage	Adequate, minor gaps noted
Risk of regressions	Low — state transitions are simpler now

The redundant updateLoadingStateAt calls and the potential idle flicker between cue gaps are the main items worth addressing. Otherwise this is a solid refactor.

[cubic-dev-ai]

6 issues found across 17 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/locales/en.yml">

<violation number="1" location="src/locales/en.yml:645">
P3: The new loading message is missing the "Read Frog |" prefix/format, so it doesn’t match the branded status string format and reads awkwardly in English.</violation>
</file>

<file name="src/entrypoints/subtitles.content/translation-coordinator.ts">

<violation number="1" location="src/entrypoints/subtitles.content/translation-coordinator.ts:87">
P2: The new per-tick updateLoadingStateAt call overwrites `error` with `idle`/`loading` on the next timeupdate, so error messages won't persist (the scheduler’s 5s auto-hide is bypassed). Consider skipping loading-state updates while an error is active or preserving the error state until it expires.</violation>

<violation number="2" location="src/entrypoints/subtitles.content/translation-coordinator.ts:87">
P3: This `updateLoadingStateAt` call is also redundant for the same reason - the caller already invoked it with identical parameters. Consider removing both redundant calls inside `translateNearby` and keeping only the post-translation call (which uses fresh `getCurrentVideoTimeMs` and `getFragments()` values after async work completes).</violation>
</file>

<file name="src/locales/zh-TW.yml">

<violation number="1" location="src/locales/zh-TW.yml:644">
P3: Prefix should match the required "Read Frog |" format for subtitle status messages.</violation>
</file>

<file name="src/locales/ru.yml">

<violation number="1" location="src/locales/ru.yml:643">
P3: Loading message misses the required `Read Frog |` prefix format. This breaks the branding consistency described in the PR.</violation>
</file>

<file name="src/locales/ja.yml">

<violation number="1" location="src/locales/ja.yml:641">
P3: Japanese loading message is missing the "Read Frog |" prefix separator; it should match the new branding format used for other locales.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P2: The new per-tick updateLoadingStateAt call overwrites error with idle/loading on the next timeupdate, so error messages won't persist (the scheduler’s 5s auto-hide is bypassed). Consider skipping loading-state updates while an error is active or preserving the error state until it expires.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/entrypoints/subtitles.content/translation-coordinator.ts, line 87:

<comment>The new per-tick updateLoadingStateAt call overwrites `error` with `idle`/`loading` on the next timeupdate, so error messages won't persist (the scheduler’s 5s auto-hide is bypassed). Consider skipping loading-state updates while an error is active or preserving the error state until it expires.</comment>

<file context>
@@ -81,21 +81,19 @@ export class TranslationCoordinator {
+    const currentTimeMs = video.currentTime * 1000
+    const fragments = this.getFragments()
+
+    this.updateLoadingStateAt(currentTimeMs, fragments)
+
     if (this.segmentationPipeline && !this.segmentationPipeline.isRunning
</file context>

[claude]

PR Review: refactor(subtitles): simplify state machine and improve loading display

Overall Assessment

🟢 Good taste. This is a well-motivated simplification — collapsing 6 states into 3 removes accidental complexity that didn't serve the user. The extracted display-rules.ts module with pure functions is the right call for testability and separation of concerns.

Net -100 deletions across 17 files for equivalent (better) behavior. That's a good refactor.

---

Positive Highlights

• State reduction is correct: fetching, fetchSuccess, fetchFailed, segmenting, processing were implementation-detail states leaking into UI. Users don't care which phase is loading — idle | loading | error is the right abstraction.
• display-rules.ts is clean: Pure functions, no side effects, clear contract via SubtitleDisplayDecision. The displayMode-aware logic (translationOnly needs subtitle.translation to be renderable) is correct.
• lastEmittedState dedup: Prevents redundant re-renders from repeated state emissions during timeupdate events. Good performance improvement.
• Loading position at bottom: Not blocking video content is the right UX decision.

---

Issues & Suggestions

1. Potential stale state after error recovery (Medium)

translation-coordinator.ts:170-172 — updateLoadingStateAt only toggles between 'loading' and 'idle'. Once universal-adapter.ts or the catch block in translateNearby sets 'error', there's no path in updateLoadingStateAt to clear it. The lastEmittedState field only tracks what this method emitted, not error states set externally.

This means after an error:

• lastEmittedState may still be 'loading' or 'idle'
• updateLoadingStateAt could emit 'idle' and clear the error from the UI prematurely on the next timeupdate tick

The error state set by the catch block in translateNearby (line 145) will survive exactly until the next handleTranslationTick call, which immediately calls updateLoadingStateAt and may overwrite it with 'idle'.

Suggestion: Either track error state in lastEmittedState when the catch block fires, or have updateLoadingStateAt skip emission when current state is 'error' (requires knowing the current scheduler state). At minimum, verify this scenario manually — it may cause error messages to flash briefly then vanish.

2. findActiveCue is O(n) on every tick (Low)

translation-coordinator.ts:163-164 — Array.find scans fragments linearly. With timeupdate firing ~4 times/sec and potentially hundreds of subtitle fragments, this is called frequently. Fragments are sorted by start, so binary search would be O(log n). Not critical given typical subtitle counts (~200-500 cues), but worth noting for the AI segmentation path which may produce more fragments.

Not a blocker — just something to keep in mind if performance issues surface.

3. Test coverage gap: error → idle transition

The tests for display-rules.ts are solid for what they cover, but there's no test for TranslationCoordinator state transitions. The most interesting (and potentially buggy) scenario — error state being overwritten by the next tick — is untested. Even a focused unit test for updateLoadingStateAt interaction with error states would add confidence.

4. Minor: StateMessage still reads its own atom independently

state-message.tsx:21 reads subtitlesStateAtom directly, while subtitles-container.tsx:15 reads it via subtitlesDisplayAtom. The container already decides whether to show StateMessage via the <Activity> wrapper, but StateMessage also independently checks stateData.state === 'idle' to return null. This double-gating is harmless (belt + suspenders), but the StateMessage null check is now dead code since the Activity wrapper already hides it when showStateMessage is false. Consider removing the internal idle guard for clarity, or adding a comment explaining the defensive check.

5. i18n: idle key retained but unused

All locale files still define subtitles.state.idle: Ready (e.g., en.yml:644), but STATE_CONFIG in state-message.tsx excludes idle via Exclude<SubtitlesState, 'idle'>. The i18n key is dead. Either remove it or leave a comment if it's kept for potential future use.

---

Summary

Area	Verdict
Code quality	🟢 Clean separation, good use of pure functions
Bugs	🟡 Error-to-idle transition timing needs verification (see #1)
Performance	🟢 lastEmittedState dedup is a net improvement
Security	🟢 No concerns
Tests	🟡 display-rules well tested; coordinator state transitions untested

The PR is in good shape. The error state lifecycle (issue #1) is the main thing worth investigating before merge — the rest are minor polish items.

[cubic-dev-ai]

2 issues found across 2 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/entrypoints/subtitles.content/translation-coordinator.ts">

<violation number="1" location="src/entrypoints/subtitles.content/translation-coordinator.ts:170">
P2: This method may prematurely clear error states. The `nextState` calculation only considers 'loading' or 'idle', but if an external error state was set (e.g., from the catch block in `translateNearby`), calling `updateLoadingStateAt` on the next tick will overwrite it with 'idle'. Consider checking if the current state is 'error' and preserving it, or track error states in `lastEmittedState` when they're set.</violation>

<violation number="2" location="src/entrypoints/subtitles.content/translation-coordinator.ts:174">
P2: Error states can become sticky because lastEmittedState is not updated when onStateChange('error') is emitted, so updateLoadingStateAt may suppress transitions back to idle/loading if the nextState matches the pre-error lastEmittedState.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P2: Error states can become sticky because lastEmittedState is not updated when onStateChange('error') is emitted, so updateLoadingStateAt may suppress transitions back to idle/loading if the nextState matches the pre-error lastEmittedState.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/entrypoints/subtitles.content/translation-coordinator.ts, line 174:

<comment>Error states can become sticky because lastEmittedState is not updated when onStateChange('error') is emitted, so updateLoadingStateAt may suppress transitions back to idle/loading if the nextState matches the pre-error lastEmittedState.</comment>

<file context>
@@ -168,17 +167,15 @@ export class TranslationCoordinator {
 
-    if (!this.translatedStarts.has(activeCue.start)) {
-      this.onStateChange('loading')
+    if (nextState === this.lastEmittedState)
       return
-    }
</file context>

[cubic-dev-ai]

P2: This method may prematurely clear error states. The nextState calculation only considers 'loading' or 'idle', but if an external error state was set (e.g., from the catch block in translateNearby), calling updateLoadingStateAt on the next tick will overwrite it with 'idle'. Consider checking if the current state is 'error' and preserving it, or track error states in lastEmittedState when they're set.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/entrypoints/subtitles.content/translation-coordinator.ts, line 170:

<comment>This method may prematurely clear error states. The `nextState` calculation only considers 'loading' or 'idle', but if an external error state was set (e.g., from the catch block in `translateNearby`), calling `updateLoadingStateAt` on the next tick will overwrite it with 'idle'. Consider checking if the current state is 'error' and preserving it, or track error states in `lastEmittedState` when they're set.</comment>

<file context>
@@ -168,17 +167,15 @@ export class TranslationCoordinator {
-      this.onStateChange('idle')
-      return
-    }
+    const nextState: SubtitlesState = activeCue && !this.translatedStarts.has(activeCue.start)
+      ? 'loading'
+      : 'idle'
</file context>