Skip to content

fix: address code review feedback (PR #13, #15)#17

Merged
ZhaoChaoqun merged 2 commits into
mainfrom
fix/review-comments
Mar 23, 2026
Merged

fix: address code review feedback (PR #13, #15)#17
ZhaoChaoqun merged 2 commits into
mainfrom
fix/review-comments

Conversation

@ZhaoChaoqun

Copy link
Copy Markdown
Owner

Summary

  • AudioEngineManager: Add thread-safety doc comments to onSamples and onAudioLevel callbacks, clarifying they must be set before start() and not mutated while the engine is running
  • ASREngineFactory: Remove unused termNormalizer field from Result struct (the pipeline sets it directly, caller never reads it)
  • ModelDownloader: Remove dead destDir assignment and clarify why the second parameter of extractionHandler is unused

Test plan

  • Build succeeds (xcodebuild -scheme Typeless -configuration Debug)
  • Verify no runtime regressions in ASR model loading
  • Verify no runtime regressions in model downloading/extraction

🤖 Generated with Claude Code

- Add thread-safety doc comments to AudioEngineManager callbacks
  (onSamples/onAudioLevel must be set before start())
- Remove unused termNormalizer field from ASREngineFactory.Result
- Remove dead destDir assignment in ModelDownloader delegate method

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Clean, well-scoped PR. All three changes verified:

  1. ASREngineFactory.ResulttermNormalizer removal is safe. The only caller (RecordingManager.swift:208) accesses result.engine only; the normalizer is set directly on the pipeline at factory line 58.

  2. AudioEngineManager — Doc comments accurately describe the threading contract. Both onSamples and onAudioLevel are indeed set before start() in current usage. Note: there's no runtime enforcement (no locks/actors) — the contract is convention-based, which the docs now make explicit.

  3. ModelDownloader — Dead destDir assignment correctly removed (both ternary branches were identical). Comment about unused second parameter is accurate — the handler closure (SherpaOnnxManager.swift:134) captures modelsDirectory directly and ignores the parameter with _.

No issues found.

Version in project.pbxproj was stuck at 1.3.0 since the v1.3.0 release.
Updates both Debug and Release configurations.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ZhaoChaoqun ZhaoChaoqun merged commit 573a97c into main Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant