feat(indicator): embed cue assets and app-owned i18n copy#2
Merged
Conversation
- embed start/stop/complete/cancel cue WAV files in the binary via go:embed\n- remove indicator text and cue-file keys from user config parsing\n- add locale scaffolding with English catalog and fallback behavior\n- update docs/tests for non-configurable indicator assets and text
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis pull request removes user-configurable indicator cue WAV files and text labels from the configuration schema, embedding WAV cue assets and adding an internal localization scaffold (English catalog shipped). Indicator playback now uses embedded data with context-aware playback and falls back to synthesis. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/sotto/internal/config/parser_test.go`:
- Around line 206-223: Replace t.Fatal/t.Fatalf usage in
TestParseIndicatorTextKeysRejected and TestParseIndicatorSoundFileKeysRejected
with testify's require helpers to match project test conventions: use
require.Error/require.ErrorContains (or require.Contains on err.Error()) for the
expected error checks and require.NoError/require.Nil where appropriate;
reference the Parse and Default calls to assert that an error was returned and
that the error message contains "unknown field". Ensure you import
"github.com/stretchr/testify/require" in the test file.
In `@apps/sotto/internal/indicator/sound.go`:
- Around line 60-66: The cue playback is dropping the caller's context so
cancellation isn't propagated; update playCue to accept a context.Context and
have the callers (ShowRecording, CueStop, CueComplete, CueCancel) pass their
context through, then change emitCue to emitCue(ctx context.Context, kind
cueKind) and playCueData to playCueData(ctx context.Context, data []byte);
inside playCueData wrap the incoming ctx with a timeout using
context.WithTimeout(ctx, ...) (instead of context.Background()) and ensure the
cancel is deferred so playback can be cancelled by the original context.
- thread context through playCue -> emitCue -> playCueData\n- wrap playback timeout around caller context instead of Background\n- short-circuit synth fallback when context is canceled\n- align new parser rejection tests to testify/require style
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
go:embed) and play them directly viapw-playWhy
~/Downloads/*.wav)Breaking change
Indicator config keys removed:
indicator.sound_start_fileindicator.sound_stop_fileindicator.sound_complete_fileindicator.sound_cancel_fileindicator.text_recordingindicator.text_processingindicator.text_errorindicator.text_transcribingVerification
go test ./apps/sotto/...just fmtjust ci-checknix build 'path:.#sotto'Summary by CodeRabbit
New Features
Documentation
Bug Fixes