Swift best-practices: close the review backlog (F, G, docstrings, A)#21
Open
samkeen wants to merge 1 commit into
Open
Swift best-practices: close the review backlog (F, G, docstrings, A)#21samkeen wants to merge 1 commit into
samkeen wants to merge 1 commit into
Conversation
Completes planning/swift-best-practices-review.md — every actionable item is now applied; only cosmetic H/I and the declined preset→switch are skipped. - G: ModelStores' six init overloads → one nil-defaulted init. The nil defaults sidestep the @mainactor default-arg blocker (the ?? fallbacks run in the init body); all call sites unchanged. - F: RecorderViewModel.stopAndTranscribe() teardown dedup — the two identical light teardowns collapse into one up-front block (capture a local session, nil the stored one, finish/cancel on the local). Catch paths cancel the local, preserving behavior without the heavier cleanupAfterFailure(). - Docstrings: first CodeRabbit-gate pass over the transcriber entry-point spine + recording view model (ModelStores, RecorderViewModel, Transcriber/ TranscriptionSession, TranscriberFactory, AppleSpeechTranscriber). Targeted, not metric-chased. - A: ParakeetDecoder per-step `joint_net[2] as? Linear` cast + fatalError removed by retyping joint_net [Module] → [UnaryLayer], so the joint applies joint_net[2](activated) directly. MLXNN keys array children by runtime value, so the joint_net.2 safetensors key is unchanged. Device-validated on the iPhone 15 Pro Max (ParakeetSmoke: decode + chunking gates PASS). Build + 61-test suite green (iPhone 17 Pro simulator); A also device-green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR applies a "Swift best-practices round 4" pass: ChangesSwift Best-Practices Round 4
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 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 `@Relay` Notes/Recording/RecorderViewModel.swift:
- Around line 175-181: The combined guard statement on line 179 checking `guard
let session, let url` returns immediately without properly cleaning up resources
when either value is missing. If session exists but url is nil, or url exists
but session is nil, the resources remain orphaned. Instead of a single combined
guard check, validate session and url separately - if session is present, cancel
it; if url is present, delete the audio file - and only then set state to
.failed. This ensures complete resource cleanup regardless of which value is
missing.
🪄 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: CHILL
Plan: Pro Plus
Run ID: e17ef875-1a45-4a4b-bcca-4793f8d093e3
📒 Files selected for processing (8)
CHANGE_LOG.mdRelay Notes/Recording/RecorderViewModel.swiftRelay Notes/Transcription/AppleSpeechTranscriber.swiftRelay Notes/Transcription/ModelStores.swiftRelay Notes/Transcription/Parakeet/ParakeetDecoder.swiftRelay Notes/Transcription/Transcriber.swiftRelay Notes/Transcription/TranscriberFactory.swiftplanning/swift-best-practices-review.md
Comment on lines
+175
to
181
| let session = self.session | ||
| self.session = nil | ||
| currentAudioURL = nil | ||
|
|
||
| guard let session, let url else { | ||
| updatesTask?.cancel() | ||
| updatesTask = nil | ||
| self.session = nil | ||
| currentAudioURL = nil | ||
| state = .failed(message: "Recording could not be saved. Please try again.") | ||
| return |
There was a problem hiding this comment.
Handle partial guard failure without leaking session/audio cleanup.
Line 179’s combined guard let session, let url drops into .failed without canceling an existing session or deleting an existing audio file when only one of the two is missing. This leaves a resource-leak/orphan-file gap exactly in the path this refactor centralized.
Suggested fix
- let session = self.session
- self.session = nil
- currentAudioURL = nil
-
- guard let session, let url else {
+ let session = self.session
+ self.session = nil
+ let fallbackURL = currentAudioURL
+ currentAudioURL = nil
+
+ guard let session else {
+ if let orphanURL = url ?? fallbackURL {
+ try? FileManager.default.removeItem(at: orphanURL)
+ }
+ state = .failed(message: "Recording could not be saved. Please try again.")
+ return
+ }
+ guard let url = url ?? fallbackURL else {
+ await session.cancel()
state = .failed(message: "Recording could not be saved. Please try again.")
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let session = self.session | |
| self.session = nil | |
| currentAudioURL = nil | |
| guard let session, let url else { | |
| updatesTask?.cancel() | |
| updatesTask = nil | |
| self.session = nil | |
| currentAudioURL = nil | |
| state = .failed(message: "Recording could not be saved. Please try again.") | |
| return | |
| let session = self.session | |
| self.session = nil | |
| let fallbackURL = currentAudioURL | |
| currentAudioURL = nil | |
| guard let session else { | |
| if let orphanURL = url ?? fallbackURL { | |
| try? FileManager.default.removeItem(at: orphanURL) | |
| } | |
| state = .failed(message: "Recording could not be saved. Please try again.") | |
| return | |
| } | |
| guard let url = url ?? fallbackURL else { | |
| await session.cancel() | |
| state = .failed(message: "Recording could not be saved. Please try again.") | |
| 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 `@Relay` Notes/Recording/RecorderViewModel.swift around lines 175 - 181, The
combined guard statement on line 179 checking `guard let session, let url`
returns immediately without properly cleaning up resources when either value is
missing. If session exists but url is nil, or url exists but session is nil, the
resources remain orphaned. Instead of a single combined guard check, validate
session and url separately - if session is present, cancel it; if url is
present, delete the audio file - and only then set state to .failed. This
ensures complete resource cleanup regardless of which value is missing.
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.
Completes
planning/swift-best-practices-review.md— every actionable item is now applied. Only the cosmetic H (error-type naming) / I (formatter) and the declined preset →switchare intentionally skipped.Changes
ModelStoressix inits → one. Collapsed the sixinitoverloads into a singlenil-defaultedinit(whisper:parakeet:cleanup:), each slot falling back to its real store via??. Thenildefaults sidestep the original blocker (a non-nildefault arg can't call the@MainActorstore init); every call site is unchanged.stopAndTranscribe()teardown dedup. The two identical light teardowns (guard-else + success) collapse into one up-front block: capture a localsession, nil the stored property, thenfinish()/cancel()on the local. Catch pathsawait session.cancel()on the local — the onlycleanupAfterFailure()work not already done up-front — so behavior is preserved.ModelStores,RecorderViewModel,Transcriber/TranscriptionSession,TranscriberFactory,AppleSpeechTranscriber(was 0%). Targeted at the non-obvious — not metric-chased filler.ParakeetDecoderper-stepfatalError. Removed the per-decode-stepjoint_net[2] as? Linearcast +fatalErrorby retypingjoint_net[Module]→[UnaryLayer], so the joint appliesjoint_net[2](activated)directly (no cast, no new stored property — which would have aliased a duplicate parameter key). Safe because MLXNN keys array children by runtime value (build(value:)inspects each elementas Module), sojoint.joint_net.2.*is keyed identically regardless of static element type.Validation
xcodebuild build+ 61-test suite green on the iPhone 17 Pro simulator.ParakeetSmoke.run(): full model loaded underverify: .noUnusedKeys(throws ifjoint_net.2goes unconsumed — it didn't), T2.1d decodesubstring check = PASS ✅, T2.1echunking check = PASS ✅, peak 1345.5 MB (unchanged). Output is numerically identical to the pre-change path (sameLinear, same weights).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor