Repository navigation
feat(player): opt-in caption narration synchronized with playback - #30
Conversation
…ack in React preview
…-screen audio controls
christophersesugh
left a comment
christophersesugh
left a comment
There was a problem hiding this comment.
Independent technical review of PR #30 at e1ec376c9329814a96b406dd298e0b0e264484e9 against the proposed TL-I17 acceptance criteria and Throughline playback contract. Five actionable findings below (2 P1, 3 P2). The reported GitHub Actions run 43202793534 passed, but the speech tests use a mocked Web Speech API; audible hardware, non-Chromium, and screen-reader verification remain outstanding. Recommendation: resolve the P1 speech-ownership/interruption issues and control-width regression, add focused tests for the observed edge cases, then rerun CI and manually verify speech before maintainer merge. This is a technical assessment, not a maintainer approval.
| if (current.current !== session) return; | ||
| session.ended = true; | ||
| session.failed = true; | ||
| if (event.error === "canceled" || event.error === "interrupted") return; |
There was a problem hiding this comment.
[P1 — High] Unexpected speech cancellation leaves autoplay running with no clock or error. stop() clears its own utterance handlers before calling cancel(), so a canceled/interrupted event that reaches this handler is not one of this controller's deliberate seek/unmount cancellations. Here it sets session.failed = true and returns without pausing or announcing anything. If another speech consumer calls speechSynthesis.cancel() while this player is playing, externalClock remains active, the current step never advances, and the UI still says Pause indefinitely. Handle unexpected cancellation/interruption like a failed utterance (pause and surface a recoverable alert), while suppressing only the controller's explicitly owned cancellation. Add a browser test emitting interrupted on the active utterance while playback is running.
There was a problem hiding this comment.
Addressed in the follow-up commits (8d23188, 3f83e78). An unexpected canceled/interrupted event that reaches the active utterance handler is now treated as failure: it releases ownership, pauses speech-driven playback, and displays a recoverable alert. Deliberate Throughline cancellations detach handlers before canceling, so they are not misreported. Added a Chromium regression that interrupts an active autoplay utterance, verifies the player returns to Play without advancing, and tests retry. Full CI #43208921297 passed.
| // A browser cancellation may synchronously invoke onerror/onend. | ||
| voice.utterance.onend = null; | ||
| voice.utterance.onerror = null; | ||
| try { speechEngine()?.cancel(); } catch { /* Speech is optional. */ } |
There was a problem hiding this comment.
[P1 — High] Canceling one player cancels every other player's speech. window.speechSynthesis has a page-wide utterance queue and cancel() removes all queued/current utterances (see MDN). Each useNarration instance stores ownership only in its own ref; when two <ThroughlinePlayer narration /> instances are active, stepping, muting, or unmounting A calls this global cancel and interrupts B. B then remains in a failed speech session (see the separate cancellation-handling finding), so independently embedded players cannot reliably play. Introduce a shared speech owner/coordinator for Throughline instances, with explicit takeover/queue behavior and predictable pause/error for displaced sessions; never assume per-instance cancel() only affects that instance. Add a two-player integration test for both seeking and unmounting one player while the other narrates.
There was a problem hiding this comment.
Addressed with src/react/speech-owner.ts (8d23188) and multi-player regressions (3f83e78). A shared per-engine coordinator gives the newest Throughline narrator ownership and explicitly pauses/disables the displaced player. Only the owner can cancel/pause/resume global speech; a displaced player's seek or unmount cannot cancel the new owner's utterance. External speech scripts remain outside our coordinator, with unexpected interruption handled as an error. Two-player takeover, seek, and unmount tests passed in CI #43208921297.
| } | ||
| /* A very narrow embedded player with both optional audio controls requires | ||
| a separate utility row; never clip or shrink accessible touch targets. */ | ||
| @container (max-width: 300px) { |
There was a problem hiding this comment.
[P2 — Medium] Narrow desktop/mobile embeds can clip the new narration controls. The @container (max-width: 300px) split happens below the width needed for the two control rows. With default 16px fonts and both audio controls offered, mobile transport needs ~124px (three 40px buttons + gaps); utilities need ~182px (56px speed select + three 40px buttons + gaps), plus the 8px grid gap = ~314px. For example, a 350px-wide .thl has ~302px of content after its 24px side padding, so this query does not fire, the control rows cannot fit, and .thl { overflow: hidden } clips a control. The current browser test only checks a 320px viewport, where the three-row layout does fire. Raise the three-row breakpoint to at least the measured combined minimum (with some font/zoom margin), or make layout depend on actual available space, and test embedded content widths around 301–320px as well as 320px viewport.
There was a problem hiding this comment.
Addressed in 8d23188: the extra utility row is now activated at player content widths up to 360px, but only when both optional sound and narration buttons are present. Existing one-audio/no-audio toolbar geometry remains unchanged. Added embedded-width Chromium coverage across 348–420px player widths, including approximately 300–320px content widths and checks for every control staying within its container (3f83e78). Full CI #43208921297 passed.
| if (!voice || voice.trace !== trace || voice.step !== step) start(); | ||
| }, [enabled, offered, trace, step, start, stop]); | ||
|
|
||
| // Play/Pause acts on the voice, not an independent timer. Do not |
There was a problem hiding this comment.
[P2 — Medium] Updating authored guidance leaves stale spoken text attached to the current frame. player.tsx derives text from the current guidance prop on every render, and latest.current.text updates, but this effect only compares trace and step. If a host replaces or corrects guidance while the same trace/frame is displayed, the visible caption changes and the old utterance keeps speaking the previous explanation; its onend may still tick to the next step. Track the utterance's text (or another guidance revision identity) in Session, invalidate/cancel stale speech when it changes, and resume/restart under a defined user-activation policy. Cover an in-place guidance update with a browser test.
There was a problem hiding this comment.
Addressed in 8d23188: each speech session now records its exact narration text; a corrected guidance string invalidates/replaces the old utterance even when trace identity and frame index are unchanged. Completion also verifies the latest text before ticking, preventing a stale callback from advancing before the next effect. Added a same-frame guidance-update Chromium test (3f83e78) and documented this behavior. Full CI #43208921297 passed.
| }, [enabled, offered, trace, step, start, stop]); | ||
|
|
||
| // Play/Pause acts on the voice, not an independent timer. Do not | ||
| // immediately pause a voice that was just started by a manual seek. |
There was a problem hiding this comment.
[P2 — Medium] Seeking/restarting the current step does not restart its narration. seekReducer accepts step actions even when to === state.step, including Restart chapter while already at that chapter's first frame. The effect compares only trace/step, so the old utterance remains (and can merely get paused by the play-state effect) instead of being canceled and reread. After a completed utterance, clicking Restart chapter on the same frame produces no new speech at all. Track an explicit seek/restart action generation separately from the frame index, or expose a narration restart path on an intentional seek, and test that repeating the current step restarts spoken content rather than preserving stale completion/position.
There was a problem hiding this comment.
Addressed in 8d23188: usePlayer now increments an internal seek revision on every explicit step dispatch, including a no-op frame index. Narration sessions track that revision, so Restart chapter on its current frame cancels/rereads, even after a completed utterance. Added a regression for both cases (3f83e78), passed in CI #43208921297.
…fresh corrected captions
…and narrow embeds
…ame-step restarts
christophersesugh
left a comment
christophersesugh
left a comment
There was a problem hiding this comment.
Follow-up review of c968b2e against the five previous findings: the speech interruption, shared ownership, guidance refresh, and same-step seek fixes are implemented and have targeted Chromium regression coverage. The dual-audio narrow-layout fix is present, but its replacement of the previous generic <=300px fallback introduces one actionable responsive regression (inline). GitHub Actions run #43208921297 passed on this head; real audible output, non-Chromium, and screen-reader checks are still not independently verified. Recommendation: restore the fallback for single/no-audio players, add embedded-width tests across the offered-control combinations, and rerun CI before maintainer merge. This is not a merge approval.
| only dual-audio players so regular 375px previews keep their compact row. | ||
| The query uses the player's CONTENT width, not the viewport width. */ | ||
| @container (max-width: 360px) { | ||
| .thl-controls:has(.thl-narration):has(.thl-sound) { |
There was a problem hiding this comment.
[P2 — Medium] Keep the narrow layout fallback for players without both audio buttons. This replaces the old unconditional @container (max-width: 300px) three-row layout with a selector that only applies when both .thl-narration and .thl-sound exist. A player embedding just one audio feature (or neither) is now forced into the two-column transport+utilities row even below 300px of content width. At 16px root font, the transport alone takes 3×40px + gaps ≈124px, and utilities need at least ~98px with no audio (speed select + fullscreen) or ~140px with one audio toggle; with the grid gap, those layouts need ~230px/~272px respectively. A 300px-wide player has only ~252px of content after .thl's 24px side padding, so the one-audio layout overflows/clips. A 260px player also clips even without audio. Restore a generic three-row fallback at the narrowest widths, retaining the wider 360px split for dual-audio players. Add embedded-width assertions for narration only, sonification only, neither, and both (the new test only exercises both).
There was a problem hiding this comment.
Addressed in 8bcf309 while preserving the existing dual-audio adjustment. The layout now uses two complementary container rules: at content widths <=360px, three rows for players offering both narration and sonification; at <=300px, the original generic three-row fallback for every configuration, including neither, only narration, only step sounds, and both. The new Chromium regression runs all four configurations across embedded player widths 260, 280, 296, 300, 320, 348, 368, 380, 420px, verifying that each control is within the toolbar and the overall player has no horizontal overflow. Full CI #43211760492 passed: 235 unit tests, 82 Chromium browser tests, package/SSR/hydration and framework checks. Leaving this thread for final maintainer review rather than resolving it on the reviewer's behalf.
Motivation
This is a distinct follow-up to merged Throughline PR #29. Step sonification provides short tones, not caption narration. Learners should be able to opt into spoken, authored step explanations without disrupting the core or the existing sound-effect toggle.
Implementation
narrationReact prop, with a Narrate captions control that starts off and requires user activation.sonificationcontrol and core compiler independent; no new dependencies, grammar changes, trace-schema changes, backend or audio files.Review follow-up (five findings addressed)
This update responds to the five inline review threads on
e1ec376(two P1 and three P2):canceledorinterruptedevent pauses playback and shows a recoverable error instead of hanging the speech clock.Restart chaptereven when the frame index does not change, including after speech completes.Each inline thread has a response with its corresponding implementation/test evidence. The additional follow-up finding (P2, generic narrow-control fallback) is addressed in
8bcf309with four-configuration browser coverage. Threads remain unresolved pending final maintainer re-review; no approval is claimed. Updated public docs describe the ownership/takeover behavior and speech limitations.Verification
8bcf309: 235 unit tests (33 files) and 82 Chromium browser tests (14 files).main; no context check-refs success claimed.Review boundary
This PR is unmerged, does not publish npm or deploy documentation, and awaits sole maintainer review. The underlying package publication, TL-I11 documentation-app integration and release acceptance gates remain independent.