Fix audio recorder init method #3783
Conversation
SDK Size
|
SDK Performance
|
WalkthroughReplaced CHANGELOG upcoming entry with a StreamChat "Fixed" subsection and added a bugfix line. Modified StreamAudioRecorder initializers: introduced a public designated initializer with an optional audioSessionConfigurator parameter and added an internal designated initializer for full dependency injection; updated wiring and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StreamAudioRecorder
participant StreamAudioSessionConfigurator
Client->>StreamAudioRecorder: init(configuration, audioSessionConfigurator = default)
activate StreamAudioRecorder
StreamAudioRecorder->>StreamAudioSessionConfigurator: use/configure audio session
StreamAudioRecorder-->>Client: instance ready (after setUp)
deactivate StreamAudioRecorder
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Generated by 🚫 Danger |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/StreamChat/Audio/AudioRecorder/AudioRecording.swift (1)
148-167: Public designated initializer with DI is a solid improvement. Consider defaultingconfigurationto.default.Defaulting
configurationmakes the call site simpler and reduces reliance on the convenienceinit(), while keeping API source-compatible. It also clarifies the default behavior in the doc comment.Apply this diff to the doc and signature:
@@ - /// - configuration: Configuration for the recorder. + /// - configuration: Configuration for the recorder. Defaults to `.default`. @@ - public init( - configuration: Configuration, + public init( + configuration: Configuration = .default, audioSessionConfigurator: AudioSessionConfiguring = StreamAudioSessionConfigurator() ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)Sources/StreamChat/Audio/AudioRecorder/AudioRecording.swift(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build LLC + UI (Xcode 15)
- GitHub Check: Test LLC (Debug)
- GitHub Check: Automated Code Review
🔇 Additional comments (3)
CHANGELOG.md (1)
6-8: Changelog entry LGTM.Accurately captures the fix. No further action needed.
Sources/StreamChat/Audio/AudioRecorder/AudioRecording.swift (2)
148-167: Initialization order and dependency setup look correct.
- All stored properties are initialized before
super.init().setUp()is invoked aftersuper.init(), which is appropriate since it subscribes to app state changes.- Keeping the
required convenience init()forAudioRecordingconformance while funneling into the new designated initializer avoids breaking changes.
169-193: Internal DI initializer improves testability and flexibility.Exposing a fully-injectable internal initializer is great for tests and internal customization without widening the public API surface. No issues spotted.
SDK Size
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
Sources/StreamChat/Audio/AudioRecorder/AudioRecording.swift (7)
176-193: Internal DI initializer is solid; consider depending on protocols for testabilityThe internal initializer cleanly supports full dependency injection. One improvement: if a protocol exists for the meter normalizer (e.g., something like
AudioValueNormalising), prefer that over the concreteAudioValuePercentageNormaliserto broaden substitutability in tests/mocks. Same idea applies to the AV provider: if you have a typealias (e.g.,typealias AudioRecorderAVProvider = (URL, [String: Any]) throws -> AVAudioRecorder) or protocol, using it can improve readability in call sites and docs.
306-315: Prefer moving the recorded file instead of reading/writing the entire DataLoading the whole file into memory and writing it out can be wasteful for longer recordings. Moving the file avoids unnecessary allocations and is faster. Suggest:
- let data = try Data(contentsOf: recorder.url.standardizedFileURL) - try data.write(to: newLocation) + try FileManager.default.moveItem( + at: recorder.url.standardizedFileURL, + to: newLocation + )
57-64: Fix small documentation typos and terminologyMinor nits in doc comments. Suggested edits:
- Use “AudioRecorder” instead of “AudioPlayer” in
Configurationdocs.- “bused” -> “used”
- “observer the duration” -> “observe the duration”
- “customise of the underline ...” -> “customise the underlying audio session configuration ...”
- “ensures that there will be conflicts ...” -> “ensures that there will be no conflicts ...”
- Grammar fixes in background/foreground handlers.
Apply these diffs:
- /// The interval at which the `AudioPlayer` will be fetching updates on the active `AVAudioPlayer` + /// The interval at which the `AudioRecorder` will be fetching updates on the active `AVAudioRecorder` /// meters.- /// The interval at which the `AudioPlayer` will be fetching updates on the duration of the + /// The interval at which the `AudioRecorder` will be fetching updates on the duration of the /// recorded track.- /// The default Configuration that is being bused by `StreamAudioRecorder` + /// The default configuration that is being used by `StreamAudioRecorder`- /// to observer the duration of the recorded track. + /// to observe the duration of the recorded track.- /// Provides a way to customise of the underline audioSessionConfiguration, in order to allow extension - /// on the logic that handles the `AVAudioSession.shared`. + /// Provides a way to customise the underlying audio session configuration, in order to allow extension + /// of the logic that handles `AVAudioSession.shared`.- .appendingPathComponent(UUID().uuidString) // Using UUID here ensures that there will be conflicts between file names + .appendingPathComponent(UUID().uuidString) // Using UUID here ensures there will be no conflicts between file names .appendingPathExtension(configuration.audioRecorderFileExtension) // Use the file extension provided with configuration- /// If an we move to the background then we want to stop the recording as we don't + /// If we move to the background, we want to stop the recording as we don't /// have the ability to pause and resume it afterwards.- /// Once we return to the foreground and the execution return back to us, we have an opportunity + /// Once we return to the foreground and the execution returns to us, we have an opportunity /// to resume the interrupted recording.Also applies to: 81-82, 135-137, 195-200, 304-306, 355-358, 361-365
144-147: Add a small unit test to prove external subclassing worksGiven IOS-1076’s goal, consider a test that verifies an external subclass can call
super.init(configuration:audioSessionConfigurator:)and that therequired init()remains available. I can draft this if helpful.Here’s a quick outline for a test subclass within the module tests (or a fixture in a test helper module):
final class TestAudioRecorderSubclass: StreamAudioRecorder { var didInit = false public override init( configuration: Configuration, audioSessionConfigurator: AudioSessionConfiguring = StreamAudioSessionConfigurator() ) { super.init(configuration: configuration, audioSessionConfigurator: audioSessionConfigurator) didInit = true } } func test_subclass_can_call_designated_super_init() { let sut = TestAudioRecorderSubclass(configuration: .default) XCTAssertTrue(sut.didInit) }If you want, I can open a follow-up PR to add this.
Also applies to: 148-167
148-167: Optionally expose more DI via additional public defaults (only if needed)If customers also need to override the meter normaliser or
AVAudioRecordercreation, consider overloads with additional parameters defaulted to current values. Keep as-is unless such requests arise; your current API is appropriately conservative.
302-312: Minor: ensure you don’t leave the temp file behind (only if you keep copying)If you keep the current copy approach, consider deleting the original temp file after write succeeds to avoid orphaned files.
148-167: Optional: consolidate init duplicationBoth initializers duplicate
multicastDelegate = .init()and callsetUp(). Not a big deal, but you could move common work to a smallpostInit()called aftersuper.init()in both places to DRY. Given Swift’s initializer rules, current version is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)Sources/StreamChat/Audio/AudioRecorder/AudioRecording.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
Sources/StreamChat/Audio/AudioRecorder/AudioRecording.swift (1)
Sources/StreamChat/Audio/AudioPlayer/AudioPlaying.swift (1)
setUp(226-283)
🔇 Additional comments (6)
Sources/StreamChat/Audio/AudioRecorder/AudioRecording.swift (6)
148-167: Designated public init resolves subclassing limitation — good fixMaking
init(configuration:audioSessionConfigurator:)a designated initializer (with a sensible default for the configurator), then callingsuper.init()followed bysetUp(), is correct and enables external subclasses to properly callsuper.init(...). This aligns with the goal to allow customers to override the recorder.
195-203: configure(_:) remains useful for runtime swapsKeeping
configure(_:)is good for runtime replacement of the session configurator without re-instantiation. No action needed.
210-226: Completion handler semantics match docsCompletion is invoked only upon successful start; errors go to the delegate. Behavior aligns with the docstring.
428-452: Observers lifecycle is correctClean stop before re-register, weak captures, and resume calls look good. This helps avoid duplicate timers and leaks.
486-489: Meter normalisation use is correctNormalising
averagePower(forChannel: 0)before publishing context is appropriate.
148-167: Double-check StreamAudioRecorder init usageI ran ripgrep across the Swift sources and found no occurrences of:
- Direct
StreamAudioRecorder(…)calls- Subclasses of
StreamAudioRecordersuper.init(configuration: …)invocationsHowever, absence of matches isn’t proof that no call sites or subclass initializers depend on the old convenience initializer. Please manually verify across the codebase (including tests and example apps) that no existing initializations or subclass
initimplementations rely on the previous convenience signature.
Public Interface open class StreamAudioRecorder: NSObject, AudioRecording, AVAudioRecorderDelegate, AppStateObserverDelegate
- public convenience init(configuration: Configuration)
+ public init(configuration: Configuration,audioSessionConfigurator: AudioSessionConfiguring = StreamAudioSessionConfigurator()) |
|
🔗 Issue Links
Resolves https://linear.app/stream/issue/IOS-1076/openwav-fix-initializers-in-streamaudiorecorder.
🎯 Goal
Allow customers to override the audio recorder.
📝 Summary
Provide bullet points with the most important changes in the codebase.
🛠 Implementation
Provide a detailed description of the implementation and explain your decisions if you find them relevant.
🎨 Showcase
Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.
🧪 Manual Testing Notes
Explain how this change can be tested manually, if applicable.
☑️ Contributor Checklist
docs-contentrepoSummary by CodeRabbit
Bug Fixes
Changed
Documentation