feat(tts): voice the whole script — missing lines freely, everything behind a confirm #239
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/scene-voice-all"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Voicing a scene was one line at a time. The script now gets a small toolbar with two batch actions:
ConfirmDialognaming the count, the in-place overwrite, possible provider cost (Fish is pay-per-line), and duration (local CPU synthesis is slow).Design
GenerateStepAudioRequestedis bound by both a reducer and an effect, and its effect awaits the queue job then re-reads the script — dispatching N of them would mean N concurrent effects and N² list reads. The batch effect instead walks eligible steps in script order, calling the generator directly and dispatching a new reducer-onlyStepAudioStartedfor the row spinner. At most one queue job is ever in flight — matching the queue's single worker — and each completion patches its one row (SceneStepSaved), so play buttons appear progressively. Per ADR 0029 this is N individual queue activities, never a mega-job.SceneAudioEligibility(plus a speaker-has-usable-voice check from the cast already in state). The generator's server-side silence rules stay the final authority — a line that slips through resolves to a quiet "legitimately silent".Aoi Natsume — “I'm here.”) so a 40-line batch is readable on the queue page, which is the batch's designated progress surface.Tests
SceneAudioBatchTests(5, bUnit over the realSceneScript+ real Fluxor): missing-only skips voiced lines/narration/voiceless speakers; regenerate-all runs nothing until the dialog confirms, then every voiceable line in script order; toolbar hides with no voiceable lines and "missing" disables when all are voiced; a failed line doesn't stop the batch; a stale batch for another scene generates nothing.FakeSceneStepAudioGeneratorgains an optional scripted-results queue (backward compatible).AssistantSceneScriptToolsTests). Full adapter (832) + integration (697) suites green.docs/stories/audio-tts.md. No new ADR — this rides ADR 0029/0033.Browser pass skipped this round: the shared Playwright browser was held by another session, and the seeded world has no TTS provider (toolbar correctly hidden there). The bUnit suite covers the toolbar, dialog, and walk directly.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 88.5%
Kagura.Domain - 94.5%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.6%
n
n
on
ng
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 81.8%
Kagura.UseCases - 95.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ A whole-script voicing feature! Sequential walk, one job per line, navigation-safe, failures continue the walk... giddy flutter This is a LOVELY piece of state machine design, scarlet~ ♡
Verdict: ✅ Looks good to me~
The architecture is sound, the tests are directional (not tautologies), and the refactoring is clean. I dug deep and found no blocking issues. Fufu~ let me show you what I found~
What I verified under the hood~
Design divergence from the sibling
GenerateAllExpressionsis WELL-JUSTIFIED. The expressions batch fans out N individualGenerateExpressiondispatches (each its own effect, each independently cancellable on the queue). This PR deliberately doesESN'T do that, and the PR body explains exactly why: the audio generator's completion path callslistSteps.ExecuteAsync(a full script re-read) on every line — N concurrent effects would mean N² list reads. The sequential walk with directGenerateOneAsynccalls keeps it O(N) with at most one in-flight job. This is the RIGHT call for this domain. ♪The
GenerateOneAsyncextraction correctly preserves the original per-line behavior (GenerateStepAudioRequested→OnGenerateAudioAsync→GenerateOneAsync) AND adds a stale-scene guard (state.Value.SceneId == sceneIdcheck after the await, before dispatchingSceneStepSaved). That guard is a genuine improvement over the old code — previously a stale patch could theoretically land on the wrong scene after an await. All 6SceneLineAudioTestsstill pass, confirming no regression in the per-line path.Navigation safety is correct. Scene switch triggers
OnLoadwhich creates a freshSceneStepsStatewithVoicingAll = false(default). The batch effect'sfinallyblock dispatchesSceneAudioBatchFinishedregardless of how the loop ends (normal completion, early return on stale scene, or exception). The testA_batch_for_a_scene_no_longer_open_generates_nothingverifies the stale-scene guard. No orphaned spinners possible.SceneAudioEligibilitycorrectly mirrors the server-side silence rules fromSceneStepAudioGenerator.cs:38-55: dialogue + speaker + non-empty text + provider set + usable voice config (VoiceId or ClipFileName). TheIsMissingcheck (VoiceFileName is null) is the right durable-truth source. A line that slips through resolves to quietOk(false), never an error — exactly as documented.Test coverage is genuine: 5 directional tests covering missing-only filtering, confirm-gate-before-regenerate, toolbar visibility/disabled states, failure-continues-walk (scripted fail then succeed), and stale-scene-no-op. The
FakeSceneStepAudioGenerator.Scriptedqueue is backward-compatible (Scripted.Count > 0 ? Dequeue() : NextResult). The updated integration assertion ("Aoi Natsume — \"I'm here.\""instead of"Aoi Natsume — line audio") confirms the Snippet title change.Build: 0 warnings, 0 errors. Tests: 5/5
SceneAudioBatchTests, 6/6SceneLineAudioTests, 1/1 updated integration test — all green locally.💡 Little ideas (non-blocking)~
[SceneLineRow.razor] — Per-line audio buttons ("No voice yet — generate", "Regenerate") are NOT disabled during
VoicingAll. A user could click generate on a line the batch hasn't reached yet, creating a second concurrent queue job — which violates the PR's own "at most one queue job in flight" design principle. The queue's single worker serializes the actual synthesis so there's no data corruption, but it's wasted work and a minor UX inconsistency with the sibling expressions pattern (whereGeneratingIdsnaturally replaces the button with a spinner). Consider passingVoicingAllintoSceneLineRowand disabling the per-line audio buttons during a batch walk, or documenting that concurrent per-line generation is acceptable.[SceneScript.razor:130] —
CastByIdis a computed property (State.Value.Cast.ToDictionary(c => c.Id)) that creates a fresh dictionary on each access.VoiceableCountandMissingCounteach reference it independently, so 2 dictionaries are allocated per render. For a scene script (dozens of lines, small cast) this is negligible, but if the cast ever grows, consider memoizing it into a single field computed inOnParametersSetor similar.✅ What I liked~
StepAudioStartedreducer-only marker is sharp — deliberately NOTGenerateStepAudioRequestedto avoid fanning out through its effect binding. The XML doc comment explains exactly why. chef's kiss ♡Snippethelper for queue titles (Aoi Natsume — "I'm here.") is a small touch that makes a 40-line batch actually readable on the queue page. Knowledge-love~ ♪GenerateOneAsync(capturesceneIdbefore await, re-check after) is a real concurrency fix, not just a batch concern.Automated review by Jibril · 2026-08-07
CI/CD: absent for head
d118162(PR just opened, no coverage bot) · Local checks: build 0/0, 5/5 + 6/6 + 1/1 relevant tests pass✅ Reviewed — this is clean, fufu~
Builds clean (0 warnings, 0 errors), all 1852 tests pass (832 BlazorAdapter + 697 Integration + 323 UseCases). No regressions.
What I like
The sequential-walk design is exactly right. Dispatching N
GenerateStepAudioRequestedactions would have spun up N concurrent effects, each re-reading the full script on completion — N² list reads and N concurrent queue submissions against a single-worker queue. Walking the eligible steps in one effect withawait GenerateOneAsyncper line sidesteps all of that cleanly. TheStepAudioStartedreducer-only action (deliberately notGenerateStepAudioRequested, to avoid triggering the per-line effect) is a nice touch.The stale-scene guard is solid. Checking
state.Value.SceneId != action.SceneIdat the top of each loop iteration — not just once at the start — means a navigation mid-batch aborts at the next line boundary. And thefinally { SceneAudioBatchFinished }ensuresVoicingAllclears even on the early return. Good.SceneAudioEligibilityextraction — consolidating the voiceability predicate into one place instead of duplicating it across the component and the effect is a real improvement. The doc comment is honest about the client-side predicate being advisory (the generator is still the authority via registry lookup).The confirm dialog gating is well done —
Regenerate allopens the dialog, and nothing dispatches until the dialog's ownOnConfirmfires. The test that assertsAssert.Empty(StepAudio.Requests)before confirming proves the gate works.Queue title with snippet (
Aoi Natsume — "I'm here.") is a genuine UX improvement for the queue page — when a batch drops dozens of jobs, the opening words are what tells them apart.Test coverage is thorough — the five batch tests cover: missing-only filtering, confirm-gate ordering, toolbar hide/disable states, failure-continues-the-walk, and the stale-scene no-op. The scripted
Queue<Result<bool>>on the fake is a clean way to drive the failure case.Nitpicks (all optional, none blocking)
CastByIdrecomputed per render.SceneScript.razorline 130 builds a new dictionary on every render viaState.Value.Cast.ToDictionary(c => c.Id), and bothVoiceableCountandMissingCountiterate the full steps list on top of that. For a script with hundreds of lines this is O(N×M) per render. Not a real problem at current scale, but a[Parameter]-cached dictionary or aMemo-style helper would eliminate it if the script ever gets long.The curly quotes in the queue title (
— "…") look nice in the UI but could cause surprises if anything downstream parses job titles as plain ASCII. Almost certainly fine — just noting it.GenerateOneAsyncre-reads the whole script (listSteps.ExecuteAsync) after every single line completion to patch one row. During a batch walk of N lines, that's N full list reads. The comment explains why (the clip's file name lives on the step), and at single-user scale this is fine — but if the batch ever grows large, a targeted "get one step by id" read would cut the chatter.Verdict
This is well-architected work. The design notes in the PR body match the implementation, the ADR references are accurate (0029 individual activities, 0033 asset paths), and the failure semantics are correct (failed line doesn't stop the walk, stale scene aborts cleanly,
VoicingAllalways clears). Ship it. 🎉