Add Sherpa ONNX speech models and manager - #592
Open
cogwheel0 wants to merge 14 commits into
Open
Conversation
Add curated STT/TTS model management, isolate-backed speech runtimes, and Sherpa Silero VAD for server and local recognition.
📝 WalkthroughWalkthroughSherpa ONNX support is added across native storage bridges, model catalogs and installation, isolate-based STT/TTS workers, persisted settings, audio services, model-management screens, routing, localization, and automated tests. ChangesSherpa offline speech
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AudioSettingsPage
participant SherpaModelsPage
participant SherpaModelManager
participant SherpaStorage
participant SherpaSttWorker
User->>AudioSettingsPage: Select Sherpa engine
AudioSettingsPage->>SherpaModelsPage: Open model selection
SherpaModelsPage->>SherpaModelManager: Enqueue model
SherpaModelManager->>SherpaStorage: Download, extract, validate
SherpaSttWorker->>SherpaStorage: Load installed runtime files
SherpaSttWorker-->>User: Emit speech events
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Macroscope has since reviewed this pull request. An earlier review was skipped by a cost limit; a review has now completed, so that notice no longer applies. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
|
@macroscope-app review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/chat/services/voice_input_service.dart (1)
987-1054: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDedupe the two recorder start paths and drop the no-op
try/catch.
_startServerRecordingand_startSherpaRecordingdiffer only in the Sherpa preflight andfeedRecognizer; the ~20-lineRecordConfigis copy-pasted, so future audio-config fixes have to be made twice. Thetry { ... } catch (error) { rethrow; }at Lines 987–1011 is also dead code.♻️ Sketch
+ RecordConfig _vadRecordConfig({ + required bool iosAudioSessionManagedExternally, + }) => RecordConfig( + encoder: AudioEncoder.pcm16bits, + sampleRate: _vadSampleRate, + numChannels: 1, + bitRate: 16, + echoCancel: true, + autoGain: false, + noiseSuppress: true, + androidConfig: _androidServerVadRecordConfig( + voiceCallSession: + Platform.isAndroid && iosAudioSessionManagedExternally, + ), + iosConfig: iosAudioSessionManagedExternally + ? _iosManagedServerVadRecordConfig + : _iosStandaloneServerVadRecordConfig, + );Then both paths call
_vadRecorder.start(..., recordConfig: _vadRecordConfig(...))withfeedRecognizeras the only difference, and thetry/catchwrapper is removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/voice_input_service.dart` around lines 987 - 1054, Extract the shared RecordConfig construction from _startServerRecording and _startSherpaRecording into a reusable _vadRecordConfig helper, then have both _vadRecorder.start calls use it while retaining their distinct feedRecognizer values and existing preflight behavior. Remove the no-op try/catch wrapper from _startServerRecording and allow errors to propagate directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/core/router/app_router.dart`:
- Around line 580-594: Remove the unused select query parameter from all callers
navigating to Routes.sherpaModels, while preserving the existing kind parameter
and SherpaModelsPage selectionKind behavior. Do not add parsing for select
unless a distinct selection behavior is required by the surrounding API.
In `@lib/core/sherpa/sherpa_catalog.dart`:
- Around line 461-471: Update the streaming Parakeet Unified entry in
_parakeet() to set modelType to 'nemo_transducer', matching the other Nemo
streaming models that use SherpaRuntimeAdapter.onlineTransducer and
_nemoTransducerFiles.
In `@lib/core/sherpa/sherpa_model_manager.dart`:
- Around line 117-137: Update enqueue in SherpaModelManager to reject models in
any in-flight installation phase, including downloading, verifying, extracting,
and validating, rather than checking only downloading. Ensure retry does not
remove progress and enqueue a duplicate while the model is actively being
installed, while preserving existing queue and disposed guards.
In `@lib/core/sherpa/sherpa_runtime.dart`:
- Around line 433-445: Update the timeout handler in the command flow around
_commandTimeout and _fail to invoke the same teardown path as dispose() after
killing the isolate, ensuring all three ReceivePorts and their subscriptions are
closed. Preserve the existing TimeoutException behavior while making timeout
cleanup complete even when the worker is no longer alive.
- Around line 46-49: Update the worker load paths around SherpaRuntime and the
corresponding second occurrence to reuse an injected or persistent SherpaStorage
instance instead of constructing SherpaStorage inside each load call. Thread the
existing caller-owned storage, such as voice_input_service.dart’s instance,
through the worker constructors or retain it as a field so resolveRuntimeFiles
uses the same instance across loads and tests do not require a platform-channel
mock.
- Around line 475-496: Make dispose() re-entrant-safe by removing the temporary
_disposed = false assignment around call('dispose'). Introduce or reuse a
separate internal state/flag to permit the farewell RPC while disposal is in
progress, ensuring concurrent dispose() calls return immediately and teardown
operations such as _isolate.kill() and subscription cancellation execute only
once.
In `@lib/core/sherpa/sherpa_storage.dart`:
- Around line 217-236: Update brokenModelIds() and the related installed-model
refresh flow to avoid calling installedModel() once per catalog entry after
installedModels() has already scanned the directories. Compute the installed and
broken model ID sets in a shared pass, reusing the existing scan results so each
model directory is recursively inspected only once while preserving both
methods’ current results.
- Around line 106-127: Update deviceInfo() to catch PlatformException in
addition to MissingPluginException and return the same unknown-device
SherpaDeviceInfo fallback for either platform failure. Preserve the existing
successful response parsing and fallback values.
In `@lib/core/sherpa/sherpa_vad_recorder.dart`:
- Around line 21-24: Reduce the retained VAD history configured by
SherpaVADRecorder: update bufferSizeSeconds and the resulting
bufferSizeSamples/maximumHistorySamples so they are bounded to the required
speech duration plus preRollSamples and postRollSamples, rather than 120
seconds. Preserve sufficient capacity for maxSpeechDurationSeconds and both roll
windows while avoiding the oversized List<double> allocation.
In `@lib/core/utils/native_sheet_utils.dart`:
- Around line 264-270: The native sheet configuration and _nativeVoiceSubtitle
must agree on Sherpa voice-picker behavior: either include voicePickerNav for
TtsEngine.sherpa so its subtitle branch can render, or remove the unreachable
Sherpa-specific subtitle branch if Sherpa voice selection is intentionally
hidden. Apply the chosen behavior consistently across both symbols.
In `@lib/features/chat/services/tts_manager.dart`:
- Around line 675-683: Update didChangeAppLifecycleState to enqueue the Sherpa
unload through _sherpaLoadSerial instead of invoking _sherpaTts.unload()
directly. Preserve the existing pause, inactive-session, and large-model guards,
and keep cache invalidation ordered with the serialized unload so it cannot race
_ensureSherpaLoadedSerial.
- Around line 233-236: Make TtsManager observer registration lazy by removing
registration from the private constructor TtsManager._(). Register the instance
with WidgetsBinding only after binding initialization, such as through
initialize() or a guarded initializer method, ensuring registration occurs once
before TtsManager functionality requires it.
In `@lib/features/chat/services/voice_input_service.dart`:
- Around line 1162-1173: Update the `_sherpaStt.load` catch block to mark the
model broken only for deterministic runtime or validation failures. Keep
`TimeoutException`, isolate spawn failures, and other transient startup/resource
errors retryable by skipping `_sherpaStorage.markModelBroken`, while preserving
the existing logging, availability reset, and return behavior.
In `@lib/features/profile/views/sherpa_models_page.dart`:
- Around line 176-198: Update the _ModelSection-to-_handleAction flow to pass
the broken-state information from _ModelSection into _ModelCard and the action
callback. In _handleAction, when the model is installed and marked broken, call
sherpaModelManagerProvider.retry(model) before activation; preserve the existing
_activate path for healthy installed models and all other progress states.
In `@lib/l10n/app_en.arb`:
- Around line 1595-1600: Update the localization metadata for sttSilenceDuration
so its description refers to the silence duration setting for both Server and
Sherpa speech-to-text, matching the sttSilenceDurationDescription string; leave
the localized string itself unchanged.
In `@lib/l10n/app_es.arb`:
- Around line 2538-2539: Update the sherpaChooseSpeechModel Spanish localization
to explicitly indicate a speech-recognition or transcription model,
distinguishing it from the existing synthesized-voice label in
sherpaChooseVoiceModel; leave the TTS wording unchanged.
- Line 2550: Update the sherpaInstalledSummary localization value to include an
explicit Spanish noun for the installed models while preserving the {count},
{size}, and separator formatting.
In `@lib/l10n/app_it.arb`:
- Line 2532: Translate the value of the hermesMemoryKeyDescription localization
key into Italian while preserving the original meaning and the
X-Hermes-Session-Key placeholder exactly.
In `@lib/l10n/app_ja.arb`:
- Line 3872: Translate the hermesMemoryKeyDescription value in app_ja.arb into
natural Japanese, preserving the meaning of memory scoping, the
X-Hermes-Session-Key identifier, and automatic stable-key generation when the
field is left blank.
In `@lib/l10n/app_nl.arb`:
- Line 2535: Update the Dutch localization entries for sherpaTtsDescription and
the corresponding STT description: use “spraaksynthese” instead of the broader
“spraak” for TTS, and “spraakherkenningsmodel” instead of “spraakmodel” for STT.
In `@lib/l10n/app_zh_Hant.arb`:
- Around line 2533-2551: After updating the ARB entries, regenerate the
localization Dart output using the project’s configured localization workflow
rather than editing generated files directly, then run both flutter test and
flutter analyze and resolve any failures before handoff.
- Line 2532: Translate the value of the hermesMemoryKeyDescription localization
key into natural Traditional Chinese, preserving the meaning of long-term memory
scoping, the X-Hermes-Session-Key identifier, automatic first-chat generation,
and the blank-field condition.
In `@test/core/sherpa/sherpa_model_manager_test.dart`:
- Around line 210-251: Extend the SherpaModelManager download tests around
`_download` to cover resume handling: accept and append a 206 response with a
valid matching Content-Range, restart from offset 0 when ETag or Last-Modified
validators mismatch, and reject malformed or mismatched Content-Range responses
without appending stale bytes. Reuse the existing test helpers and assertions to
verify request offsets, partial-file contents, and resulting phases.
---
Outside diff comments:
In `@lib/features/chat/services/voice_input_service.dart`:
- Around line 987-1054: Extract the shared RecordConfig construction from
_startServerRecording and _startSherpaRecording into a reusable _vadRecordConfig
helper, then have both _vadRecorder.start calls use it while retaining their
distinct feedRecognizer values and existing preflight behavior. Remove the no-op
try/catch wrapper from _startServerRecording and allow errors to propagate
directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dcbb00ac-d021-4a83-a6df-629259644ac8
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
android/app/src/main/kotlin/app/cogwheel/conduit/MainActivity.ktios/Podfileios/Runner.xcodeproj/project.pbxprojios/Runner/AppDelegate.swiftlib/core/persistence/persistence_keys.dartlib/core/router/app_router.dartlib/core/services/navigation_service.dartlib/core/services/settings_service.dartlib/core/sherpa/sherpa_catalog.dartlib/core/sherpa/sherpa_model.dartlib/core/sherpa/sherpa_model_manager.dartlib/core/sherpa/sherpa_runtime.dartlib/core/sherpa/sherpa_storage.dartlib/core/sherpa/sherpa_vad_recorder.dartlib/core/utils/native_sheet_utils.dartlib/core/utils/tts_voice_utils.dartlib/features/chat/providers/text_to_speech_provider.dartlib/features/chat/services/text_to_speech_service.dartlib/features/chat/services/tts_manager.dartlib/features/chat/services/voice_input_service.dartlib/features/chat/voice_mode/chat_voice_mode_controller.dartlib/features/profile/views/app_customization_page.dartlib/features/profile/views/audio_settings_page.dartlib/features/profile/views/sherpa_models_page.dartlib/l10n/app_cs.arblib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_ja.arblib/l10n/app_ko.arblib/l10n/app_nl.arblib/l10n/app_ru.arblib/l10n/app_sk.arblib/l10n/app_zh.arblib/l10n/app_zh_Hant.arblib/main.dartpubspec.yamltest/core/services/settings_service_test.darttest/core/sherpa/sherpa_catalog_test.darttest/core/sherpa/sherpa_model_manager_test.darttest/core/sherpa/sherpa_vad_recorder_test.darttest/features/chat/services/tts_manager_test.darttest/features/chat/services/voice_input_service_test.darttest/features/chat/voice_mode/chat_voice_mode_controller_test.dart
💤 Files with no reviewable changes (2)
- ios/Podfile
- ios/Runner.xcodeproj/project.pbxproj
|
Manual reviews triggered for commit All prior checks · these links stay valid even if you push more commits. |
|
Review in progress. Results will be posted as check runs when complete. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/l10n/app_it.arb (1)
2550-2550: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the installed-model summary singular-safe.
{count} installatiis incorrect when{count}is1. Use neutral wording or an ICU plural expression, for exampleModelli installati: {count} • {size}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/l10n/app_it.arb` at line 2550, Update the sherpaInstalledSummary localization string to avoid the incorrect singular “installati” wording, using neutral wording or an ICU plural expression while preserving the count and size placeholders.lib/features/profile/views/audio_settings_page.dart (1)
478-491: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve the selected Sherpa speaker by ID, not array index.
setSherpaTtsSpeakerIdstoresSherpaSpeaker.id, but this code indexesmodel.speakers[speaker]. Non-contiguous speaker IDs show the wrong fallback or subtitle. Searchmodel.speakersforcandidate.id == speakerId, matching the native-sheet implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/profile/views/audio_settings_page.dart` around lines 478 - 491, Update the Sherpa voice metadata resolution in the ttsEngine == TtsEngine.sherpa branch to treat sherpaTtsSpeakerId as a SherpaSpeaker.id, not an array index. Parse the selected ID, search model.speakers for a candidate whose id matches, and use that candidate’s name; preserve the existing choose-voice and numbered fallback behavior when no ID or matching speaker exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/main.dart`:
- Around line 601-606: Update the Sherpa STT branch in lib/main.dart:601-606 and
the Sherpa TTS branch in lib/main.dart:641-646 to resolve the selected installed
model first; when present, activate it via
setSttPreference(SttPreference.sherpa) or
setTtsEngineSelection(TtsEngine.sherpa) respectively, then refresh the native
voice detail, and only route to the model chooser when no selected installed
model exists.
---
Outside diff comments:
In `@lib/features/profile/views/audio_settings_page.dart`:
- Around line 478-491: Update the Sherpa voice metadata resolution in the
ttsEngine == TtsEngine.sherpa branch to treat sherpaTtsSpeakerId as a
SherpaSpeaker.id, not an array index. Parse the selected ID, search
model.speakers for a candidate whose id matches, and use that candidate’s name;
preserve the existing choose-voice and numbered fallback behavior when no ID or
matching speaker exists.
In `@lib/l10n/app_it.arb`:
- Line 2550: Update the sherpaInstalledSummary localization string to avoid the
incorrect singular “installati” wording, using neutral wording or an ICU plural
expression while preserving the count and size placeholders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a917f304-c278-41bd-a873-76002c9f1f8e
📒 Files selected for processing (24)
lib/core/services/navigation_service.dartlib/core/services/settings_service.dartlib/core/sherpa/sherpa_catalog.dartlib/core/sherpa/sherpa_model_manager.dartlib/core/sherpa/sherpa_runtime.dartlib/core/sherpa/sherpa_storage.dartlib/core/sherpa/sherpa_vad_recorder.dartlib/core/utils/native_sheet_utils.dartlib/features/chat/services/tts_manager.dartlib/features/chat/services/voice_input_service.dartlib/features/profile/views/audio_settings_page.dartlib/features/profile/views/sherpa_models_page.dartlib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_it.arblib/l10n/app_ja.arblib/l10n/app_nl.arblib/l10n/app_zh_Hant.arblib/main.darttest/core/services/settings_service_test.darttest/core/sherpa/sherpa_catalog_test.darttest/core/sherpa/sherpa_model_manager_test.darttest/core/sherpa/sherpa_vad_recorder_test.darttest/features/chat/services/voice_input_service_test.dart
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/chat/services/voice_input_service.dart (1)
1124-1189: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSherpa model stays resident in the worker after the selection is invalidated. Both the STT and TTS availability-refresh helpers reset only their own cached model-id/available flags when the configured model becomes null, unknown, or uninstalled — neither ever calls the corresponding worker's
unload(), so a previously loaded (possiblylarge-tier) model keeps consuming isolate memory until something unrelated eventually triggers an unload (e.g., app backgrounding, forlargetier only).
lib/features/chat/services/voice_input_service.dart#L1124-L1189: in theid == null, invalid-model-kind, andinstalled == nullbranches of_prepareSherpaSttNow, callawait _sherpaStt.unload()(guarded by_loadedSherpaSttModelId != null) before/while clearing the cache, routed through the existing_serializeSherpaLifecyclequeue.lib/features/chat/services/tts_manager.dart#L1627-L1641: in_refreshSherpaModelAvailability, when the engine/model/kind no longer resolves to a valid Sherpa TTS model, chain an unload of_sherpaTts(clearing_loadedSherpaModelId/_loadedSherpaLanguageCode) onto_sherpaLoadSerialinstead of only flipping_sherpaModelAvailable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/voice_input_service.dart` around lines 1124 - 1189, The Sherpa availability refresh paths must unload invalidated workers, not only clear cache flags. In lib/features/chat/services/voice_input_service.dart:1124-1189, update _prepareSherpaSttNow’s null, invalid-kind, and uninstalled branches to conditionally await _sherpaStt.unload() when _loadedSherpaSttModelId is set, using the existing _serializeSherpaLifecycle queue. In lib/features/chat/services/tts_manager.dart:1627-1641, update _refreshSherpaModelAvailability to clear the loaded model/language and chain _sherpaTts.unload() through _sherpaLoadSerial when the configured TTS model is invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/features/chat/services/voice_input_service.dart`:
- Around line 1124-1189: The Sherpa availability refresh paths must unload
invalidated workers, not only clear cache flags. In
lib/features/chat/services/voice_input_service.dart:1124-1189, update
_prepareSherpaSttNow’s null, invalid-kind, and uninstalled branches to
conditionally await _sherpaStt.unload() when _loadedSherpaSttModelId is set,
using the existing _serializeSherpaLifecycle queue. In
lib/features/chat/services/tts_manager.dart:1627-1641, update
_refreshSherpaModelAvailability to clear the loaded model/language and chain
_sherpaTts.unload() through _sherpaLoadSerial when the configured TTS model is
invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 732a513e-492f-4496-93e2-1a3bb79492b2
📒 Files selected for processing (4)
lib/features/chat/services/tts_manager.dartlib/features/chat/services/voice_input_service.dartlib/main.darttest/features/chat/services/tts_manager_test.dart
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
lib/features/chat/services/tts_manager.dart (5)
661-668: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not recycle session IDs in
reset().Asynchronous callbacks from an old session can still finish after
reset(). Resetting_sessionCounterlets the next session reuse the same ID, causing stale callbacks to pass the new session guards and enqueue old audio or emit incorrect events.Proposed fix
- _sessionCounter = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/tts_manager.dart` around lines 661 - 668, Remove the `_sessionCounter = 0` assignment from `reset()` so session IDs remain monotonic across resets. Keep the existing playback cleanup, active-session clearing, and idle unload scheduling unchanged, ensuring callbacks from pre-reset sessions cannot match later sessions.
943-960: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope
_serverFetchingIndicescleanup to the originating session.The
finallyblock removes an index unconditionally. If a new streaming session reuses that index before the old fetch completes, the old callback removes the new session’s in-flight marker and can trigger premature completion.Proposed fix
} finally { - _serverFetchingIndices.remove(index); + if (_activeSession?.id == session.id) { + _serverFetchingIndices.remove(index); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/tts_manager.dart` around lines 943 - 960, Update the finally block in the server chunk fetch flow to remove the index from _serverFetchingIndices only when the originating session is still the active session. Keep the existing completion check tied to session.id, so stale callbacks cannot clear a newer session’s marker or trigger premature completion.
1113-1139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate startup cancellation to
speak().
_startServerPlaybackreturns normally when the session becomes stale, butspeak()then returns that inactive session as if playback started. Re-check the session ID after startup or propagate cancellation as an error.Proposed guard
if (engine != TtsEngine.device) { await _startServerPlayback(session); + if (_activeSession?.id != session.id) return null; } else { await _startDevicePlayback(session); }Also applies to: 422-448
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/tts_manager.dart` around lines 1113 - 1139, Update _startServerPlayback and its caller speak() so startup cancellation is propagated instead of returning an inactive session as successfully started. After each startup await and before speak() returns, re-check that _activeSession?.id still matches the session ID; when stale, return the existing cancellation result or throw the established cancellation error, preserving normal playback behavior for active sessions.
680-694: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftJoin background synthesis before disposing Sherpa resources.
The prefetch and retry paths are launched with
unawaited, whiledispose()only waits for_sherpaLoadSerial. An in-flight_sherpaTts.synthesize()or player task can therefore continue after_sherpaTts.dispose()or_player.dispose(). Track these tasks and await/cancel them before releasing the workers.Also applies to: 1246-1307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/tts_manager.dart` around lines 680 - 694, The dispose flow around TtsManager.dispose must join all background prefetch and retry work before releasing _player and _sherpaTts. Track the unawaited synthesis/player tasks launched by the prefetch and retry paths around the referenced methods, then await or cancel those tracked tasks in dispose alongside _sherpaLoadSerial before calling _player.dispose() and _sherpaTts.dispose().
633-654: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep playlist mutations serialized across session changes.
An
addAudioSourcecall can still be in flight whenstop()resets_serverPlaylistSerialand returns. The next session may then clear/configure the player concurrently, after which the stale operation appends old audio and overwrites shared enqueue state. Preserve the operation queue across resets and await outstanding playlist work before starting the next session.Also applies to: 1440-1473, 1755-1771
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/tts_manager.dart` around lines 633 - 654, Update stop() and the related session-start/reset paths to preserve _serverPlaylistSerial across session changes instead of resetting it while addAudioSource work may still be pending. Await the outstanding playlist operation before clearing or reconfiguring the player for a new session, keeping addAudioSource mutations serialized so stale audio cannot append or overwrite shared enqueue state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/features/chat/services/tts_manager.dart`:
- Around line 661-668: Remove the `_sessionCounter = 0` assignment from
`reset()` so session IDs remain monotonic across resets. Keep the existing
playback cleanup, active-session clearing, and idle unload scheduling unchanged,
ensuring callbacks from pre-reset sessions cannot match later sessions.
- Around line 943-960: Update the finally block in the server chunk fetch flow
to remove the index from _serverFetchingIndices only when the originating
session is still the active session. Keep the existing completion check tied to
session.id, so stale callbacks cannot clear a newer session’s marker or trigger
premature completion.
- Around line 1113-1139: Update _startServerPlayback and its caller speak() so
startup cancellation is propagated instead of returning an inactive session as
successfully started. After each startup await and before speak() returns,
re-check that _activeSession?.id still matches the session ID; when stale,
return the existing cancellation result or throw the established cancellation
error, preserving normal playback behavior for active sessions.
- Around line 680-694: The dispose flow around TtsManager.dispose must join all
background prefetch and retry work before releasing _player and _sherpaTts.
Track the unawaited synthesis/player tasks launched by the prefetch and retry
paths around the referenced methods, then await or cancel those tracked tasks in
dispose alongside _sherpaLoadSerial before calling _player.dispose() and
_sherpaTts.dispose().
- Around line 633-654: Update stop() and the related session-start/reset paths
to preserve _serverPlaylistSerial across session changes instead of resetting it
while addAudioSource work may still be pending. Await the outstanding playlist
operation before clearing or reconfiguring the player for a new session, keeping
addAudioSource mutations serialized so stale audio cannot append or overwrite
shared enqueue state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6730549-0970-4514-b160-b1ce1efa306f
📒 Files selected for processing (2)
lib/features/chat/services/tts_manager.darttest/features/chat/services/tts_manager_test.dart
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/chat/services/tts_manager.dart (1)
1751-1758: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCleanup failures mask the original load error.
If
markModelBroken(file I/O) orunload()(RPC into a worker that just failed to initialize) throws, that secondary error propagates instead of theSherpaModelLoadExceptionand therethrownever executes — callers lose the "needs repair" signal.🛡️ Preserve the original error
} catch (error) { if (error is SherpaModelLoadException) { _sherpaModelAvailable = false; - await _sherpaStorage.markModelBroken(id, error); - await _sherpaTts.unload(); + try { + await _sherpaStorage.markModelBroken(id, error); + } catch (_) {} + try { + await _sherpaTts.unload(); + } catch (_) {} } rethrow; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/tts_manager.dart` around lines 1751 - 1758, Update the Sherpa model-load catch block around _sherpaModelAvailable, _sherpaStorage.markModelBroken, and _sherpaTts.unload so cleanup failures are caught and suppressed, ensuring the original SherpaModelLoadException is rethrown. Preserve the existing cleanup attempts and model-unavailable state while preventing either cleanup operation from replacing the load error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/features/chat/services/tts_manager.dart`:
- Around line 1751-1758: Update the Sherpa model-load catch block around
_sherpaModelAvailable, _sherpaStorage.markModelBroken, and _sherpaTts.unload so
cleanup failures are caught and suppressed, ensuring the original
SherpaModelLoadException is rethrown. Preserve the existing cleanup attempts and
model-unavailable state while preventing either cleanup operation from replacing
the load error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4415a3a0-149e-4447-8390-1e5c70f02765
📒 Files selected for processing (6)
lib/core/services/api_service.dartlib/core/sherpa/sherpa_runtime.dartlib/features/chat/services/tts_manager.dartlib/features/chat/services/voice_input_service.darttest/core/sherpa/sherpa_runtime_test.darttest/features/chat/services/tts_manager_test.dart
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/chat/services/voice_input_service.dart (2)
1258-1269: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winError path in
_processSherpaSamplesskips the generation check.The success path re-validates
_listenGeneration != listenGenerationbefore calling_handleSherpaResult, but thecatchblock reports the error unconditionally via_reportRecognitionError, which writes to whatever_textStreamController/_transcriptEventControllerare currently active. Iffinalize()throws after a newer listening session has started (controllers replaced), a stale error would leak into the new session's streams. Given today's stop/start serialization this window looks unreachable, but the asymmetry with the success-path guard is a latent hazard if that serialization ever changes.🛡️ Proposed guard
} catch (error) { - _reportRecognitionError(error); + if (_listenGeneration != listenGeneration) return; + _reportRecognitionError(error); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/voice_input_service.dart` around lines 1258 - 1269, Update _processSherpaSamples so its catch block re-checks _listenGeneration against listenGeneration before calling _reportRecognitionError. Preserve the existing success-path guard and suppress errors from stale listening generations, while still reporting errors for the current session.
1544-1553: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLarge-model unload check reads stale loaded-model state outside the serialized queue.
model?.tieris evaluated against_loadedSherpaSttModelIdsynchronously, before the unload is enqueued via_serializeSherpaLifecycle. If a_prepareSherpaStt()load is still in-flight when the app backgrounds (_loadedSherpaSttModelIdis stillnullat that point, since it's only set afterload()succeeds), this check short-circuits and no unload is scheduled — even though the load will shortly complete with a large model that then stays resident while the app is paused.tts_manager.dart's equivalent_scheduleIdleLargeSherpaUnloadavoids this by re-checking the tier inside the serialized_sherpaLoadSerialchain, after any in-flight load has settled.♻️ Proposed fix: move the tier check inside the serialized unload
void didChangeAppLifecycleState(AppLifecycleState state) { if (state != AppLifecycleState.paused || _isListening) return; - final model = sherpaModelById(_loadedSherpaSttModelId); - if (model?.tier != SherpaModelTier.large) return; - _loadedSherpaSttModelId = null; - _loadedSherpaSttLanguageCode = null; - _sherpaSttAvailable = false; - unawaited(_unloadSherpaRecognizer()); + unawaited(_serializeSherpaLifecycle(() async { + if (_isListening) return; + final model = sherpaModelById(_loadedSherpaSttModelId); + if (model?.tier != SherpaModelTier.large) return; + await _unloadSherpaRecognizerNow(); + })); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/voice_input_service.dart` around lines 1544 - 1553, Move the large-model tier check from didChangeAppLifecycleState into the serialized _unloadSherpaRecognizer flow, using the state after _sherpaLoadSerial has settled so in-flight _prepareSherpaStt loads are observed. Keep the paused-state and _isListening guards in didChangeAppLifecycleState, and only clear the loaded-model fields and unload when the serialized check confirms a large model.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/features/chat/services/voice_input_service.dart`:
- Around line 1258-1269: Update _processSherpaSamples so its catch block
re-checks _listenGeneration against listenGeneration before calling
_reportRecognitionError. Preserve the existing success-path guard and suppress
errors from stale listening generations, while still reporting errors for the
current session.
- Around line 1544-1553: Move the large-model tier check from
didChangeAppLifecycleState into the serialized _unloadSherpaRecognizer flow,
using the state after _sherpaLoadSerial has settled so in-flight
_prepareSherpaStt loads are observed. Keep the paused-state and _isListening
guards in didChangeAppLifecycleState, and only clear the loaded-model fields and
unload when the serialized check confirms a large model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 12d72055-02ad-4021-912c-4ac699507253
📒 Files selected for processing (3)
lib/features/chat/services/tts_manager.dartlib/features/chat/services/voice_input_service.darttest/features/chat/services/voice_input_service_test.dart
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/features/chat/services/voice_input_service.dart (4)
224-231: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor
forceLocalSttwhen Sherpa is selected.
checkOnDeviceSupport()andtestOnDeviceStt()explicitly callinitialize(forceLocalStt: true), but this early Sherpa return bypasses_loadLocales()and_initializeNativeLocalStt(). Those APIs will therefore always report native on-device STT as unavailable whenever Sherpa is the selected preference.Proposed fix
- if (_preference == SttPreference.sherpa) { + if (_preference == SttPreference.sherpa && !forceLocalStt) { await _prepareSherpaStt(); _isInitialized = true; return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/voice_input_service.dart` around lines 224 - 231, Update the initialization branch around _prepareSherpaStt so forceLocalStt takes precedence over the Sherpa preference: when forced, continue through _loadLocales() and _initializeNativeLocalStt() instead of returning from the Sherpa path. Preserve the existing Sherpa-only preparation and early return when forceLocalStt is false.
1036-1072: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up resources when VAD startup is cancelled.
_stopListeningInternal()stops the recorder before awaiting_vadRecordingStartup. If cancellation wins between the generation checks and_setupVadStreams()or_vadRecorder.start(), startup can then attach subscriptions or start recording, fail the final generation check, and be silently ignored by_launchVadRecording(). No later stop is guaranteed, leaving the recorder or subscriptions active.Proposed fix
Future<void> _startVadRecording({ required int generation, required bool iosAudioSessionManagedExternally, required bool feedRecognizer, }) async { - _checkListeningGeneration(generation); - await _stopVadRecording(); - _checkListeningGeneration(generation); - _vadPendingSamples = null; - await _setupVadStreams(); - _checkListeningGeneration(generation); - final settings = _ref?.read(appSettingsProvider); - ... - await _vadRecorder.start(...); - _checkListeningGeneration(generation); + try { + _checkListeningGeneration(generation); + await _stopVadRecording(); + _checkListeningGeneration(generation); + _vadPendingSamples = null; + await _setupVadStreams(generation: generation); + _checkListeningGeneration(generation); + final settings = _ref?.read(appSettingsProvider); + ... + await _vadRecorder.start(...); + _checkListeningGeneration(generation); + } catch (_) { + await _stopVadRecording(); + rethrow; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/voice_input_service.dart` around lines 1036 - 1072, Update _startVadRecording to clean up VAD resources when a generation check fails during startup: ensure cancellation after _setupVadStreams() or _vadRecorder.start() stops the recorder and disposes any subscriptions before the cancellation propagates. Preserve _launchVadRecording() behavior while guaranteeing no recorder or stream remains active after a cancelled startup.
1074-1101: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBind VAD subscriptions to the listening generation.
The callbacks only inspect mode flags and
_isListening. During teardown,_isListeningis cleared before the mode flags, and subscription cancellation occurs afterrecorder.stop(). Queued events from an older session can therefore populate_vadPendingSamples, report an error, or stop a newly started session.Pass
generationinto_setupVadStreams()and guard every callback with_isCurrentListeningGeneration(generation).Proposed fix
- Future<void> _setupVadStreams() async { + Future<void> _setupVadStreams({required int generation}) async { ... _vadSpeechEndSub = _vadRecorder.onSpeechEnd.listen((samples) { - if (!_usingServerStt && !_usingSherpaStt) return; + if (!_isCurrentListeningGeneration(generation)) return; if (samples.isEmpty) return; _vadPendingSamples = samples; - if (_isListening) { - unawaited(_stopListening()); - } + unawaited(_stopListening()); }); ... _vadFrameSub = _vadRecorder.onFrameProcessed.listen((frame) { - if (!_isListening) return; + if (!_isCurrentListeningGeneration(generation)) return; ... }); ... _vadErrorSub = _vadRecorder.onError.listen((message) { + if (!_isCurrentListeningGeneration(generation)) return; _reportRecognitionError(Exception(message)); - if (_isListening) { - unawaited(_stopListening()); - } + unawaited(_stopListening()); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/voice_input_service.dart` around lines 1074 - 1101, Update _setupVadStreams to accept the listening generation and capture it for all VAD event callbacks. In the onSpeechEnd, onFrameProcessed, and onError handlers, require _isCurrentListeningGeneration(generation) before processing events, updating _vadPendingSamples or intensity, reporting errors, or stopping listening; retain the existing mode, sample, and listening checks after this generation guard.
1523-1557: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not unload Sherpa while a transition or disposal is in flight.
!_isListeningdoes not imply the recognizer is idle: stop clears that flag before Sherpa finalization, and start can still be preparing the model or VAD. A pause callback can therefore unload the worker during finalization/startup. Also remove the lifecycle observer before awaiting teardown; otherwise a callback can enqueue work after the awaited lifecycle future was captured and race_sherpaStt.dispose().Proposed fix
Future<void> dispose() async { + if (_observingLifecycle) { + WidgetsBinding.instance.removeObserver(this); + _observingLifecycle = false; + } await stopListening(); ... - if (_observingLifecycle) { - WidgetsBinding.instance.removeObserver(this); - _observingLifecycle = false; - } } Future<void> _unloadIdleLargeSherpaRecognizerNow() async { - if (_lifecycleState != AppLifecycleState.paused || _isListening) return; + if (!_observingLifecycle || + _lifecycleState != AppLifecycleState.paused || + _isListening || + _startListeningInFlight != null || + _stopListeningInFlight != null || + _vadRecordingStartup != null) { + return; + } ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/voice_input_service.dart` around lines 1523 - 1557, Update dispose, didChangeAppLifecycleState, and _unloadIdleLargeSherpaRecognizerNow to coordinate with the Sherpa lifecycle serialization and disposal state rather than relying only on _isListening. Remove the lifecycle observer before awaiting teardown, prevent pause-triggered unloads while start/stop/finalization or disposal is in flight, and ensure any queued lifecycle work completes before _sherpaStt.dispose() runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/features/chat/services/voice_input_service.dart`:
- Around line 224-231: Update the initialization branch around _prepareSherpaStt
so forceLocalStt takes precedence over the Sherpa preference: when forced,
continue through _loadLocales() and _initializeNativeLocalStt() instead of
returning from the Sherpa path. Preserve the existing Sherpa-only preparation
and early return when forceLocalStt is false.
- Around line 1036-1072: Update _startVadRecording to clean up VAD resources
when a generation check fails during startup: ensure cancellation after
_setupVadStreams() or _vadRecorder.start() stops the recorder and disposes any
subscriptions before the cancellation propagates. Preserve _launchVadRecording()
behavior while guaranteeing no recorder or stream remains active after a
cancelled startup.
- Around line 1074-1101: Update _setupVadStreams to accept the listening
generation and capture it for all VAD event callbacks. In the onSpeechEnd,
onFrameProcessed, and onError handlers, require
_isCurrentListeningGeneration(generation) before processing events, updating
_vadPendingSamples or intensity, reporting errors, or stopping listening; retain
the existing mode, sample, and listening checks after this generation guard.
- Around line 1523-1557: Update dispose, didChangeAppLifecycleState, and
_unloadIdleLargeSherpaRecognizerNow to coordinate with the Sherpa lifecycle
serialization and disposal state rather than relying only on _isListening.
Remove the lifecycle observer before awaiting teardown, prevent pause-triggered
unloads while start/stop/finalization or disposal is in flight, and ensure any
queued lifecycle work completes before _sherpaStt.dispose() runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 551e4db7-7308-4050-8ffa-e2e959f14f27
📒 Files selected for processing (1)
lib/features/chat/services/voice_input_service.dart
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/chat/services/voice_input_service.dart (1)
687-707: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA
stopListening()during Sherpa model preparation is silently dropped, so recording starts anyway.
_startListeningInternalawaits_prepareSherpaStt()(isolate spawn + model load, can take seconds) at Line 694-696 before_listenGenerationis advanced and_isListeningis set totrue(Line 707/713). IfstopListening()is called during this window,_stopListeningInternalreadswasListening = _isListeningasfalseand returns early as a no-op — even though it unconditionally bumps_listenGenerationfirst. The pending start never re-checks the generation after itsawait _prepareSherpaStt(), so once the model finishes loading it proceeds straight into_startSherpaRecording, which gets a brand-new (unaffected) generation number at Line 707 and starts recording — despite the caller having already received a completedstopListening().Net effect: caller believes they cancelled the request to listen, but the microphone starts anyway once the Sherpa model finishes loading.
🐛 Proposed fix: honor cancellation requested during Sherpa prep
- if (_preference == SttPreference.sherpa && !_sherpaSttAvailable) { - await _prepareSherpaStt(); - } + if (_preference == SttPreference.sherpa && !_sherpaSttAvailable) { + final prepGeneration = _listenGeneration; + await _prepareSherpaStt(); + if (_listenGeneration != prepGeneration) { + // A stop() call raced in while the model was loading; honor it + // instead of starting a session nobody asked for anymore. + return const Stream<String>.empty(); + } + }Worth adding a regression test mirroring the existing "waits for an in-flight stop before starting a new session" test in
test/features/chat/services/voice_input_service_test.dart, but gated on Sherpa model preparation instead of native STT'sstopListening(). Want me to draft it?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/chat/services/voice_input_service.dart` around lines 687 - 707, Update _startListeningInternal to re-check the listen-generation cancellation state immediately after awaiting _prepareSherpaStt(), and abort without starting recording when stopListening() advanced the generation during preparation. Preserve normal startup when the generation is unchanged, ensuring _startSherpaRecording is not invoked after a completed cancellation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/features/chat/services/voice_input_service.dart`:
- Around line 687-707: Update _startListeningInternal to re-check the
listen-generation cancellation state immediately after awaiting
_prepareSherpaStt(), and abort without starting recording when stopListening()
advanced the generation during preparation. Preserve normal startup when the
generation is unchanged, ensuring _startSherpaRecording is not invoked after a
completed cancellation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1a8b1d0e-d06e-4e6e-a824-ccb126389b13
📒 Files selected for processing (2)
lib/features/chat/services/voice_input_service.darttest/features/chat/services/voice_input_service_test.dart
…x-model-settings # Conflicts: # lib/l10n/app_cs.arb # lib/l10n/app_de.arb # lib/l10n/app_es.arb # lib/l10n/app_fr.arb # lib/l10n/app_it.arb # lib/l10n/app_ja.arb # lib/l10n/app_ko.arb # lib/l10n/app_nl.arb # lib/l10n/app_ru.arb # lib/l10n/app_sk.arb # lib/l10n/app_zh.arb # lib/l10n/app_zh_Hant.arb # test/core/services/settings_service_test.dart # test/core/utils/native_sheet_utils_test.dart
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/l10n/app_nl.arb (1)
2532-2532: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate this string for the Dutch locale.
hermesMemoryKeyDescriptionis still English, so Dutch users will see an untranslated description in the settings UI.Proposed fix
- "hermesMemoryKeyDescription": "The memory key scopes the agent's long-term memory to you (X-Hermes-Session-Key). A stable key is generated automatically the first time you chat if you leave this blank.", + "hermesMemoryKeyDescription": "<Dutch translation preserving X-Hermes-Session-Key and the blank-value behavior>",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/l10n/app_nl.arb` at line 2532, Translate the hermesMemoryKeyDescription value in the Dutch ARB locale into natural Dutch, preserving the meaning, placeholder text “X-Hermes-Session-Key,” and blank-value behavior description.lib/l10n/app_ru.arb (1)
2532-2532: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the Hermes memory-key description.
This Russian locale entry remains entirely in English, so the Hermes settings screen displays untranslated copy to Russian users.
Proposed translation
- "hermesMemoryKeyDescription": "The memory key scopes the agent's long-term memory to you (X-Hermes-Session-Key). A stable key is generated automatically the first time you chat if you leave this blank.", + "hermesMemoryKeyDescription": "Ключ памяти ограничивает долгосрочную память агента вашими данными (X-Hermes-Session-Key). Если оставить поле пустым, при первом чате ключ будет создан автоматически.",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/l10n/app_ru.arb` at line 2532, Translate the hermesMemoryKeyDescription value in the Russian ARB locale into natural Russian while preserving the meaning, placeholder text “X-Hermes-Session-Key,” and blank-line behavior.Source: Coding guidelines
lib/core/utils/native_sheet_utils.dart (1)
321-335: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSherpa speaker resolution is duplicated in three places.
The same "parse
sherpaTtsSpeakerId, look up the model, matchcandidate.id, fall back tosherpaVoiceNumber(id + 1)" logic exists here, in_voiceSubtitle(lib/features/profile/views/audio_settings_page.dartlines 509-522), and partially in_handleNativeTtsVoiceSelection(lib/main.dartlines 919-930). Extract a single helper (e.g. alongsideformatTtsVoiceDisplayNameinlib/core/utils/tts_voice_utils.dart) so the fallback text and the off-by-one+ 1stay in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/core/utils/native_sheet_utils.dart` around lines 321 - 335, Extract the duplicated Sherpa speaker resolution into a shared helper near formatTtsVoiceDisplayName, then update this code and _voiceSubtitle to use it while refactoring _handleNativeTtsVoiceSelection to reuse the same lookup. Preserve null handling and the fallback sherpaVoiceNumber(id + 1) behavior so all call sites remain consistent.lib/main.dart (1)
660-666: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSherpa STT language code is persisted without validation.
The sibling
stt-language-codecase normalizes and rejects invalid input viaSettingsService.normalizeSttLanguageCode; this branch stores whatever string the native sheet sends. Validate against the configured Sherpa model'slanguagesbefore persisting so a stale/unsupported tag cannot reach the recognizer.🛡️ Proposed fix
case 'sherpa-stt-language-code': if (value is String) { + final model = sherpaModelById( + ref.read(appSettingsProvider).sherpaSttModelId, + ); + if (value.isNotEmpty && + !(model?.supportsLanguage(value) ?? false)) { + DebugLogger.validation( + 'Ignoring unsupported Sherpa STT language code', + scope: 'native/sheet', + data: {'value': value}, + ); + await _refreshNativeVoiceDetail(); + return; + } await ref .read(appSettingsProvider.notifier) .setSherpaSttLanguageCode(value.isEmpty ? null : value); await _refreshNativeVoiceDetail(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/main.dart` around lines 660 - 666, Update the sherpa-stt-language-code handling in the settings change switch to validate non-empty values against the configured Sherpa model’s languages using the existing SettingsService.normalizeSttLanguageCode flow or equivalent sibling logic. Persist only a supported normalized language code, retain null for empty input, and avoid updating settings or refreshing native voice details for invalid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/Runner/NativeSheetBridge.swift`:
- Line 4500: Update the showsDisclosure predicate in the native sheet row
configuration so dismissOnSelect-only actions remain action-only without a
chevron. Restrict the added disclosure behavior to Sherpa rows, or require an
actual navigation target such as item.url or canNavigate(item), while preserving
disclosure for genuinely navigable rows.
In `@lib/core/services/native_sheet_hydration_service.dart`:
- Around line 724-744: Update the hydration flow around the ttsService voice
load and sherpaInstalledModelsProvider inventory load to start both independent
operations concurrently, then await their results together. Preserve the
existing per-source try/catch behavior and warning identifiers, including null
or fallback results when either operation fails.
In `@lib/features/profile/views/sherpa_models_page.dart`:
- Around line 846-862: Update the failed-install banner in the Sherpa install
progress UI to display a concise message derived from progress?.error when
available, falling back to _phaseLabel(l10n, SherpaInstallPhase.failed) when no
error exists. Preserve the existing styling, truncation, and layout around the
failed phase.
- Around line 96-108: Update the model categorization around active and
available so available excludes every model already included in active, while
retaining the existing installed-model exclusion. Use the existing activeIds or
active collection consistently, ensuring settings-referenced but uninstalled
models render only in active.
- Around line 375-386: After awaiting ThemedDialogs.confirm in the model
deletion handler, check that the ConsumerState is still mounted before using
ref. Return immediately when disposed, then perform
ref.read(sherpaModelManagerProvider).delete and
ref.invalidate(sherpaInstalledModelsProvider) only while mounted.
In `@lib/features/profile/widgets/adaptive_segmented_selector.dart`:
- Around line 34-47: Update the useIcons calculation in the LayoutBuilder
builder to incorporate MediaQuery.textScalerOf(context).scale(1) into the width
threshold so larger accessibility text disables decorative icons on narrow
layouts. Remove the redundant !constraints.maxWidth.isFinite branch, while
preserving showIcons and the options.length < 3 behavior.
In `@lib/l10n/app_es.arb`:
- Line 2565: Update the sherpaMeteredNetworkWarning localization value to the
clearer Spanish wording “La red actual es de uso medido.”
- Line 2571: Update the sherpaDownloadSizeSummary translation so the
installed-size placeholder uses grammatically correct storage wording, such as
indicating the amount after installation, while preserving the download-size
portion and placeholders.
In `@lib/l10n/app_fr.arb`:
- Line 2551: Update the ARB entries sherpaInstalledSummary and
sherpaDownloadSizeSummary to use ICU pluralization for correct singular/plural
grammar and explicit wording around the size value. Keep the changes in the ARB
source, regenerate the localization outputs, then run the Flutter analysis and
test gates.
In `@lib/l10n/app_ja.arb`:
- Line 3901: Update the sherpaVoices localization value to use the natural
Japanese counter phrase "{count} 種類の音声", preserving the existing {count}
placeholder.
In `@lib/l10n/app_ru.arb`:
- Line 2561: Update the sherpaVoices localization message in app_ru.arb to use
ICU pluralization for the integer count, with grammatically correct Russian
forms for the relevant numeric ranges. Declare count as an integer placeholder
in the message metadata, then regenerate the localization output.
In `@test/core/utils/native_sheet_utils_test.dart`:
- Around line 12-25: Add a test case covering buildNativeAudioSheetParts when
installedModelCount is null, and assert the sherpa-models manager subtitle
equals l10n.sherpaModelsSubtitle. Keep the existing populated-summary test
unchanged and verify the manager remains discoverable through parts.mainItems.
In `@test/features/profile/views/sherpa_models_page_test.dart`:
- Around line 78-154: Extend the sherpa model page widget tests to cover both
stateful branches: emit a pending-to-installed transition through the overridden
sherpaInstallProgressProvider and verify auto-activation calls the
installed-model flow and pops the page, and add a broken-model scenario that
exercises retry through _handleAction. Reuse the existing router and provider
setup, asserting the page is dismissed and no exception occurs after each path.
In `@test/features/profile/widgets/adaptive_segmented_selector_test.dart`:
- Around line 60-111: Add a sibling widget test for the wide-layout threshold
using a width of at least 360, reusing the AdaptiveSegmentedSelector options and
platform setup from the compact test. Assert all labels remain present and each
platform-appropriate icon is found, so the useIcons threshold behavior is
verified in both layouts.
---
Outside diff comments:
In `@lib/core/utils/native_sheet_utils.dart`:
- Around line 321-335: Extract the duplicated Sherpa speaker resolution into a
shared helper near formatTtsVoiceDisplayName, then update this code and
_voiceSubtitle to use it while refactoring _handleNativeTtsVoiceSelection to
reuse the same lookup. Preserve null handling and the fallback
sherpaVoiceNumber(id + 1) behavior so all call sites remain consistent.
In `@lib/l10n/app_nl.arb`:
- Line 2532: Translate the hermesMemoryKeyDescription value in the Dutch ARB
locale into natural Dutch, preserving the meaning, placeholder text
“X-Hermes-Session-Key,” and blank-value behavior description.
In `@lib/l10n/app_ru.arb`:
- Line 2532: Translate the hermesMemoryKeyDescription value in the Russian ARB
locale into natural Russian while preserving the meaning, placeholder text
“X-Hermes-Session-Key,” and blank-line behavior.
In `@lib/main.dart`:
- Around line 660-666: Update the sherpa-stt-language-code handling in the
settings change switch to validate non-empty values against the configured
Sherpa model’s languages using the existing
SettingsService.normalizeSttLanguageCode flow or equivalent sibling logic.
Persist only a supported normalized language code, retain null for empty input,
and avoid updating settings or refreshing native voice details for invalid
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f98268fc-3837-4f53-bf0a-eab652eb4b4b
📒 Files selected for processing (25)
ios/Runner/NativeSheetBridge.swiftlib/core/services/native_sheet_hydration_service.dartlib/core/utils/native_sheet_utils.dartlib/features/profile/views/audio_settings_page.dartlib/features/profile/views/sherpa_models_page.dartlib/features/profile/widgets/adaptive_segmented_selector.dartlib/features/profile/widgets/settings_page_scaffold.dartlib/l10n/app_cs.arblib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_ja.arblib/l10n/app_ko.arblib/l10n/app_nl.arblib/l10n/app_ru.arblib/l10n/app_sk.arblib/l10n/app_zh.arblib/l10n/app_zh_Hant.arblib/main.dartlib/shared/widgets/conduit_components.darttest/core/utils/native_sheet_utils_test.darttest/features/profile/views/sherpa_models_page_test.darttest/features/profile/widgets/adaptive_segmented_selector_test.dart
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/core/services/settings_service_test.dart (1)
574-590: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert lower-bound clamping is persisted.
Line 587 verifies only state after
0.1; a regression that persists0.1instead of the clamped0.5still passes.Proposed test addition
await notifier.setSherpaTtsSpeed(0.1); check(container.read(appSettingsProvider).sherpaTtsSpeed).equals(0.5); + check( + PreferencesStore.get<double>(PreferenceKeys.sherpaTtsSpeed), + ).equals(0.5);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/services/settings_service_test.dart` around lines 574 - 590, Extend the test for setSherpaTtsSpeed to also read PreferenceKeys.sherpaTtsSpeed after setting 0.1 and assert that persistence contains the clamped value 0.5, matching the existing state assertion.lib/core/services/settings_service.dart (1)
1414-1500: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for settings hydration before writing Sherpa selections.
PreferencesStore.putskips writes before initialization. These APIs can updatestateduring_pendingLoad, after which_hydrateFromPrefs()replaces it with stale storage. Mirror Lines 1251-1255 before every new Sherpa mutation API.Proposed fix
+ Future<bool> _awaitSettingsHydration() async { + final pendingLoad = _pendingLoad; + if (pendingLoad != null) await pendingLoad; + return ref.mounted; + } + Future<void> activateSherpaStt({ required String modelId, String? languageCode, }) async { + if (!await _awaitSettingsHydration()) return; final normalizedLanguageCode = SettingsService.normalizeSherpaLanguageCode( languageCode, );Apply the same guard to
activateSherpaTts,setSherpaSttLanguageCode,setSherpaTtsLanguageCode,setSherpaTtsSpeakerId, andsetSherpaTtsSpeed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/core/services/settings_service.dart` around lines 1414 - 1500, Ensure all Sherpa mutation APIs wait for settings hydration before updating preferences or state, matching the existing guard pattern around lines 1251-1255. Add the guard at the start of activateSherpaTts, setSherpaSttLanguageCode, setSherpaTtsLanguageCode, setSherpaTtsSpeakerId, and setSherpaTtsSpeed; preserve the existing normalization, clamping, persistence, and state updates afterward.lib/l10n/app_cs.arb (1)
4154-4156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate all newly added English strings in
lib/l10n/app_cs.arb.The changed entries introduce untranslated English into the Czech locale.
lib/l10n/app_cs.arb#L4154-L4156: translate the OpenRouter validation and description strings.lib/l10n/app_cs.arb#L4237-L4290: translate the model, reasoning, sync, preview, and Ollama strings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/l10n/app_cs.arb` around lines 4154 - 4156, Translate every newly added English string in lib/l10n/app_cs.arb into Czech, covering the OpenRouter entries at lines 4154-4156 and the model, reasoning, sync, preview, and Ollama entries at lines 4237-4290; preserve all ARB keys and placeholders while leaving already translated content unchanged.Source: Coding guidelines
lib/l10n/app_zh.arb (1)
2499-2501: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate all newly added UI strings in the Chinese locales.
lib/l10n/app_zh.arb#L2499-L2501: Translate the OpenRouter validation and description strings.lib/l10n/app_zh.arb#L2582-L2635: Translate the reasoning, proxy, sync, preview, and Ollama strings.lib/l10n/app_zh_Hant.arb#L2499-L2501: Translate the OpenRouter validation and description strings.lib/l10n/app_zh_Hant.arb#L2582-L2635: Translate the reasoning, proxy, sync, preview, and Ollama strings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/l10n/app_zh.arb` around lines 2499 - 2501, Translate all newly added UI strings in lib/l10n/app_zh.arb at lines 2499-2501 and 2582-2635, including the OpenRouter, reasoning, proxy, sync, preview, and Ollama strings. Apply equivalent Simplified Chinese translations in lib/l10n/app_zh_Hant.arb at lines 2499-2501 and 2582-2635 using Traditional Chinese appropriate for that locale, while preserving the existing ARB keys and formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/core/services/settings_service.dart`:
- Around line 1414-1500: Ensure all Sherpa mutation APIs wait for settings
hydration before updating preferences or state, matching the existing guard
pattern around lines 1251-1255. Add the guard at the start of activateSherpaTts,
setSherpaSttLanguageCode, setSherpaTtsLanguageCode, setSherpaTtsSpeakerId, and
setSherpaTtsSpeed; preserve the existing normalization, clamping, persistence,
and state updates afterward.
In `@lib/l10n/app_cs.arb`:
- Around line 4154-4156: Translate every newly added English string in
lib/l10n/app_cs.arb into Czech, covering the OpenRouter entries at lines
4154-4156 and the model, reasoning, sync, preview, and Ollama entries at lines
4237-4290; preserve all ARB keys and placeholders while leaving already
translated content unchanged.
In `@lib/l10n/app_zh.arb`:
- Around line 2499-2501: Translate all newly added UI strings in
lib/l10n/app_zh.arb at lines 2499-2501 and 2582-2635, including the OpenRouter,
reasoning, proxy, sync, preview, and Ollama strings. Apply equivalent Simplified
Chinese translations in lib/l10n/app_zh_Hant.arb at lines 2499-2501 and
2582-2635 using Traditional Chinese appropriate for that locale, while
preserving the existing ARB keys and formatting.
In `@test/core/services/settings_service_test.dart`:
- Around line 574-590: Extend the test for setSherpaTtsSpeed to also read
PreferenceKeys.sherpaTtsSpeed after setting 0.1 and assert that persistence
contains the clamped value 0.5, matching the existing state assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8a09f0f1-70e4-4adc-a5c8-173904bb7397
📒 Files selected for processing (21)
ios/Runner/NativeSheetBridge.swiftlib/core/persistence/persistence_keys.dartlib/core/services/native_sheet_hydration_service.dartlib/core/services/settings_service.dartlib/core/utils/native_sheet_utils.dartlib/l10n/app_cs.arblib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_ja.arblib/l10n/app_ko.arblib/l10n/app_nl.arblib/l10n/app_ru.arblib/l10n/app_sk.arblib/l10n/app_zh.arblib/l10n/app_zh_Hant.arblib/main.darttest/core/services/settings_service_test.darttest/core/utils/native_sheet_utils_test.dart
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
Testing
Review
Note
Add Sherpa ONNX as a local STT and TTS engine with model management
sherpa_onnxas a new STT and TTS engine alongside existing device and server options, replacing thevaddependency.VadHandler-based pipeline inVoiceInputService./profile/audio/sherpa-modelsroute (sherpa_models_page.dart) for browsing, installing, and activating models; integrates Sherpa model/speaker/language/speed controls into audio settings and the native sheet.sherpa_storageFlutter method channel on both iOS (AppDelegate.swift) and Android (MainActivity.kt) to supply no-backup storage paths and device ABI/memory info.activateSherpaStt/activateSherpaTtsnotifier methods.VoiceInputServicenow throwsStateErroron post-disposal calls and no longer falls back implicitly between STT engines; callers must handle generation-mismatch early-return streams.Macroscope summarized b154fa1.
Summary by CodeRabbit
Greptile Summary
Adds Sherpa ONNX speech support and addresses the previously reported worker-disposal races.
Confidence Score: 5/5
The pull request appears safe to merge because the previously reported worker-disposal failures are addressed and no blocking failures remain.
The worker-disposal failures are addressed and no blocking failure remains.
What T-Rex did
Important Files Changed
Reviews (14): Last reviewed commit: "fix: address adaptive Sherpa UI review" | Re-trigger Greptile