Grade-report fixes: test quality, security, performance#20
Merged
Conversation
AppDefault's update() compared values by casting to AnyHashable and returned false for any non-AnyHashable value — forcing a redundant DispatchQueue.main.async write on every body pass. Constraining Value: Equatable lets it compare directly; every @AppDefault value type is already Equatable. (grade-report C2) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
saveTranscription awaited cleanupExpiredRecordsQuietly() — a full predicate fetch + delete — before returning, so every save (and the UI waiting on it) paid that cost. Cleanup now runs in a detached Task, matching how initial cleanup already runs. (grade-report B5) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The MLX/Parakeet daemon subprocess inherited the full parent environment. It now receives only an allowlist — PATH, HOME, PYTHONUNBUFFERED, plus locale, temp, cache, and venv/HuggingFace variables the daemon genuinely needs — for least privilege. (grade-report E3) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ModelIntegrity trusted the first download of any model unconditionally. It now verifies app-shipped models against a known-good SHA-256 table (hard fail on mismatch, even on first download), mirroring how the bundled uv binary is verified; user-added models keep trust-on-first- use. The hash table ships empty with a documented placeholder — real release hashes must be populated before the pin takes effect. (grade-report E1) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Each of the 8 waveform renderers installed a 30fps Timer.publish that autoconnected on view init and ran forever — even off-screen or idle, churning the main thread (the Visuals grid alone = ~240 updates/sec). A new FrameTimer helper makes the timer subscription start on onAppear and fully cancel on onDisappear; on-screen visual behavior is unchanged. (grade-report G1) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The core TranscriptionPipeline tests caught every error into XCTAssertTrue(true), so a broken pipeline still passed. They now inject a stub speech service and assert the returned transcript, the correction outcome, and error propagation. Separately, all ~52 XCTAssertTrue(true) tautologies across the suite are replaced with concrete assertions (or removed where reaching the line is itself the assertion). (grade-report D1, D2) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements three major themes: hardening daemon subprocess isolation via environment allowlist, extending model integrity verification with optional pinned-hash support alongside trust-on-first-use, refactoring waveform animations to a centralized FrameTimer component, and systematically upgrading 20+ test files from placeholder assertions to meaningful behavior validation. ChangesCore Application Logic Changes
Waveform Animation Refactoring
Comprehensive Test Suite Enhancements
Sequence Diagram(s)sequenceDiagram
participant MLXModelManager
participant ModelIntegrity
participant Disk
MLXModelManager->>ModelIntegrity: verify(at: fileURL, modelIdentifier: "repo")
ModelIntegrity->>ModelIntegrity: knownHash(for: "repo")
alt Pinned Hash Found
ModelIntegrity->>Disk: Read file, compute SHA-256
ModelIntegrity->>ModelIntegrity: Compare computed vs. pinned
alt Mismatch
ModelIntegrity-->>MLXModelManager: throw pinnedMismatch error
else Match
ModelIntegrity->>Disk: Update sidecar (optional)
ModelIntegrity-->>MLXModelManager: Success
end
else No Pinned Hash
ModelIntegrity->>Disk: Read sidecar hash (if exists)
alt Sidecar Exists and Matches
ModelIntegrity-->>MLXModelManager: Success (TOFU verified)
else Sidecar Exists but Mismatches
ModelIntegrity-->>MLXModelManager: throw mismatch error
else No Sidecar
ModelIntegrity->>Disk: Write computed hash to sidecar
ModelIntegrity-->>MLXModelManager: Success (first use recorded)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@Sources/Managers/MLDaemonManager`+Process.swift:
- Around line 87-98: The environment pass-through array passThroughIfSet in
MLDaemonManager+Process.swift currently omits SSL certificate overrides; update
passThroughIfSet to include "SSL_CERT_FILE", "SSL_CERT_DIR",
"REQUESTS_CA_BUNDLE", and "CURL_CA_BUNDLE" so the for loop that reads
parent[...] and writes env[...] passes these through to the Python daemon;
ensure the new keys are added alongside the existing variables used by the env
population logic (passThroughIfSet, parent, env) so corporate/custom CA settings
are preserved.
In `@Sources/Stores/DataManager.swift`:
- Line 169: The call launches an unstructured Task which inherits the current
actor/context; replace Task { await cleanupExpiredRecordsQuietly() } with a
detached background task so the cleanup doesn't inherit MainActor/foreground
priority. Locate the invocation in DataManager (the
cleanupExpiredRecordsQuietly() call) and change it to use Task.detached { await
cleanupExpiredRecordsQuietly() } so the work runs off the main actor with
detached/default priority.
In `@Sources/Views/Components/Waveform/FrameTimer.swift`:
- Around line 15-38: Add MainActor isolation to the FrameTimer utility by
annotating the FrameTimer type with `@MainActor` so its mutable state (interval,
cancellable, publisher) and methods start() / stop() are executed on the main
actor; update any call sites if needed to await or hop to the main actor when
constructing or invoking FrameTimer, and ensure references to publisher,
cancellable, start(), stop(), and init(interval:) remain consistent after the
annotation.
In `@Tests/AppDelegate/AppDelegateNotificationsTests.swift`:
- Around line 24-30: awaitNotificationProcessed currently posts the notification
then uses a fixed delay to assume processing, which doesn't actually synchronize
with observers; replace the fixed-delay approach by creating an XCTest
expectation bound to the notification (e.g. using expectation(forNotification:
name, object: object, handler: nil) or
NotificationCenter.addObserver(forName:object:queue:using:)-backed expectation),
then post the notification and wait(for: [expectation], timeout:), removing the
DispatchQueue.main.asyncAfter-based fulfill so the test truly waits for the
observer invoked by awaitNotificationProcessed.
In `@Tests/Utilities/ResourceLocatorTests.swift`:
- Around line 35-49: The test
testURLForResourceWithDevRelativePathResolvesExistingFile is nondeterministic
because it silently allows url == nil; update it to assert deterministically by
checking the dev fixture file existence directly: call
ResourceLocator.url(forResource:withExtension:devRelativePath:) as before, then
compute the expectedDevPath ("Sources/parakeet_transcribe_pcm.py") and use
FileManager.default.fileExists(atPath:) to decide assertions—if the file exists
assert url is non-nil, lastPathComponent equals "parakeet_transcribe_pcm.py",
and fileExists(atPath: url.path) is true; otherwise assert url is nil. This
ensures failures occur when the fixture is present but resolution returns nil.
In `@Tests/Views/Transcription/TranscriptionRecordRowTests.swift`:
- Around line 126-131: The test in TranscriptionRecordRowTests.swift uses a
hardcoded English suffix ("s") to assert sub-minute formatting; change it to a
locale-agnostic check: assert formattedDuration is non-nil, does not use
colon-based time (no ":"), and that the numeric portion — parsed with
NumberFormatter to respect the current locale — is approximately equal to the
record.duration (or less than 60s). Locate the assertion around
formattedDuration and record in the test, replace the hasSuffix("s") check with
parsing via NumberFormatter and a numeric comparison to ensure the output
represents seconds in a locale-safe way.
🪄 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
Run ID: b9a60b2c-f287-47a3-9822-16a5ea9d4e48
📒 Files selected for processing (36)
Sources/Managers/MLDaemonManager+Process.swiftSources/Services/MLXModelManager+Downloads.swiftSources/Services/ModelManager.swiftSources/Stores/DataManager.swiftSources/Utilities/AppDefault.swiftSources/Utilities/DiskMutationSerializer.swiftSources/Views/Components/Waveform/ClassicWaveformView.swiftSources/Views/Components/Waveform/ConstellationWaveformView.swiftSources/Views/Components/Waveform/DialWaveformView.swiftSources/Views/Components/Waveform/FrameTimer.swiftSources/Views/Components/Waveform/HaloWaveformView.swiftSources/Views/Components/Waveform/HeartbeatPulseView.swiftSources/Views/Components/Waveform/NeonWaveformView.swiftSources/Views/Components/Waveform/SpectrumWaveformView.swiftSources/Views/Components/Waveform/StreamWaveformView.swiftSources/Views/Components/Waveform/WaveformContainer.swiftTests/AppDelegate/AppDelegateMenuTests.swiftTests/AppDelegate/AppDelegateNotificationsTests.swiftTests/AppDelegate/AppDelegateRecordingWindowTests.swiftTests/AudioEngineRecorderTests.swiftTests/ErrorPresenterExpandedTests.swiftTests/Integration/ErrorPropagationLayerTests.swiftTests/Integration/ErrorRecoveryIntegrationTests.swiftTests/SpeechToTextServiceTests.swiftTests/TranscriptionPipelineTests.swiftTests/Utilities/LoggerTests.swiftTests/Utilities/ResourceLocatorTests.swiftTests/Utilities/WindowTitlesTests.swiftTests/Views/Components/InkRippleViewTests.swiftTests/Views/Dashboard/DashboardTranscriptsViewTests.swiftTests/Views/Dashboard/MLXModelManagementViewTests.swiftTests/Views/Dashboard/UsageDashboardViewTests.swiftTests/Views/Transcription/TranscriptionRecordRowTests.swiftTests/Views/WelcomeViewTests.swiftTests/Waveform/ParticleSystemTests.swiftTests/WindowControllerTests.swift
💤 Files with no reviewable changes (2)
- Tests/AudioEngineRecorderTests.swift
- Tests/Utilities/WindowTitlesTests.swift
Comment on lines
+87
to
+98
| let passThroughIfSet = [ | ||
| "LANG", "LC_ALL", "LC_CTYPE", "TMPDIR", | ||
| "AUDIOWHISPER_APP_SUPPORT_DIR", | ||
| "VIRTUAL_ENV", "PYTHONPATH", | ||
| "UV_CACHE_DIR", "UV_PYTHON", | ||
| "HF_HOME", "HF_HUB_CACHE", "HUGGINGFACE_HUB_CACHE", "XDG_CACHE_HOME" | ||
| ] | ||
| for key in passThroughIfSet { | ||
| if let value = parent[key], !value.isEmpty { | ||
| env[key] = value | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding SSL certificate environment variables for corporate network compatibility.
The allowlist doesn't include SSL certificate override variables (SSL_CERT_FILE, SSL_CERT_DIR, REQUESTS_CA_BUNDLE, CURL_CA_BUNDLE). In corporate environments with custom CA certificates or MITM proxies, the Python daemon may fail SSL verification when downloading HuggingFace models, since Python's requests/urllib3 check these variables.
This is an edge case—most users rely on system certificates which will work—but corporate users could hit hard-to-diagnose download failures.
🔧 Proposed fix to add SSL certificate pass-through
let passThroughIfSet = [
"LANG", "LC_ALL", "LC_CTYPE", "TMPDIR",
"AUDIOWHISPER_APP_SUPPORT_DIR",
"VIRTUAL_ENV", "PYTHONPATH",
"UV_CACHE_DIR", "UV_PYTHON",
- "HF_HOME", "HF_HUB_CACHE", "HUGGINGFACE_HUB_CACHE", "XDG_CACHE_HOME"
+ "HF_HOME", "HF_HUB_CACHE", "HUGGINGFACE_HUB_CACHE", "XDG_CACHE_HOME",
+ "SSL_CERT_FILE", "SSL_CERT_DIR", "REQUESTS_CA_BUNDLE", "CURL_CA_BUNDLE"
]🤖 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 `@Sources/Managers/MLDaemonManager`+Process.swift around lines 87 - 98, The
environment pass-through array passThroughIfSet in MLDaemonManager+Process.swift
currently omits SSL certificate overrides; update passThroughIfSet to include
"SSL_CERT_FILE", "SSL_CERT_DIR", "REQUESTS_CA_BUNDLE", and "CURL_CA_BUNDLE" so
the for loop that reads parent[...] and writes env[...] passes these through to
the Python daemon; ensure the new keys are added alongside the existing
variables used by the env population logic (passThroughIfSet, parent, env) so
corporate/custom CA settings are preserved.
|
|
||
| // Retention cleanup runs off the save critical path — the caller | ||
| // (and the UI) shouldn't wait on a full predicate fetch + delete. | ||
| Task { await cleanupExpiredRecordsQuietly() } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider using Task.detached instead of Task for background cleanup.
The PR description mentions a "detached Task," but the code uses unstructured Task { }, which inherits the MainActor context and current task priority. For background cleanup work that shouldn't compete with UI operations, Task.detached { } would be more semantically accurate and would default to a lower priority.
♻️ Suggested change
- Task { await cleanupExpiredRecordsQuietly() }
+ Task.detached { await self.cleanupExpiredRecordsQuietly() }📝 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
| Task { await cleanupExpiredRecordsQuietly() } | |
| Task.detached { await self.cleanupExpiredRecordsQuietly() } |
🤖 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 `@Sources/Stores/DataManager.swift` at line 169, The call launches an
unstructured Task which inherits the current actor/context; replace Task { await
cleanupExpiredRecordsQuietly() } with a detached background task so the cleanup
doesn't inherit MainActor/foreground priority. Locate the invocation in
DataManager (the cleanupExpiredRecordsQuietly() call) and change it to use
Task.detached { await cleanupExpiredRecordsQuietly() } so the work runs off the
main actor with detached/default priority.
Comment on lines
+15
to
+38
| final class FrameTimer { | ||
| /// Per-frame tick stream. Emits only between `start()` and `stop()`. | ||
| let publisher = PassthroughSubject<Date, Never>() | ||
|
|
||
| private let interval: TimeInterval | ||
| private var cancellable: AnyCancellable? | ||
|
|
||
| init(interval: TimeInterval) { | ||
| self.interval = interval | ||
| } | ||
|
|
||
| /// Begin emitting ticks. No-op if already running. | ||
| func start() { | ||
| guard cancellable == nil else { return } | ||
| cancellable = Timer.publish(every: interval, on: .main, in: .common) | ||
| .autoconnect() | ||
| .sink { [publisher] date in publisher.send(date) } | ||
| } | ||
|
|
||
| /// Stop emitting ticks and cancel the underlying timer. | ||
| func stop() { | ||
| cancellable?.cancel() | ||
| cancellable = nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# Find all usages of FrameTimer
rg "FrameTimer" --type swift -B 2 -A 2Repository: jtn0123/AudioWhisper
Length of output: 4861
🏁 Script executed:
# Check file structure to understand if this is a UI component
fd -e swift Sources/Views/Components/Waveform/ | head -20Repository: jtn0123/AudioWhisper
Length of output: 506
🏁 Script executed:
# Look for similar timer patterns in the codebase with `@MainActor`
rg "`@MainActor`" --type swift -A 3 -B 1Repository: jtn0123/AudioWhisper
Length of output: 50377
🏁 Script executed:
# Check closure retention patterns in the codebase
rg "sink.*\[.*\].*in" --type swift | head -20Repository: jtn0123/AudioWhisper
Length of output: 365
Consider adding @MainActor isolation to FrameTimer for consistency with similar utility classes.
While not strictly required by the guideline (which targets UI components), marking FrameTimer with @MainActor aligns with the pattern used by similar helper classes in the codebase like MicTestCapture and LivePreviewSampler, and makes explicit that the timer's mutable state is main-actor-isolated.
Suggested change
+@MainActor
final class FrameTimer {🤖 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 `@Sources/Views/Components/Waveform/FrameTimer.swift` around lines 15 - 38, Add
MainActor isolation to the FrameTimer utility by annotating the FrameTimer type
with `@MainActor` so its mutable state (interval, cancellable, publisher) and
methods start() / stop() are executed on the main actor; update any call sites
if needed to await or hop to the main actor when constructing or invoking
FrameTimer, and ensure references to publisher, cancellable, start(), stop(),
and init(interval:) remain consistent after the annotation.
Comment on lines
+24
to
30
| /// Posts `.pressAndHoldSettingsChanged` and waits for the observer to run. | ||
| private func awaitNotificationProcessed(name: Notification.Name, object: Any?) { | ||
| NotificationCenter.default.post(name: name, object: object) | ||
| let processed = expectation(description: "Notification processed") | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { processed.fulfill() } | ||
| wait(for: [processed], timeout: 1.0) | ||
| } |
There was a problem hiding this comment.
awaitNotificationProcessed does not synchronize with the posted notification.
Line 26 posts before any expectation is registered, and Lines 27-29 always fulfill via fixed delay. This can pass even when observer wiring/regression exists and adds timing flakiness.
🔧 Proposed fix
private func awaitNotificationProcessed(name: Notification.Name, object: Any?) {
- NotificationCenter.default.post(name: name, object: object)
- let processed = expectation(description: "Notification processed")
- DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { processed.fulfill() }
- wait(for: [processed], timeout: 1.0)
+ let posted = expectation(forNotification: name, object: nil) { _ in true }
+ NotificationCenter.default.post(name: name, object: object)
+ wait(for: [posted], timeout: 1.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 `@Tests/AppDelegate/AppDelegateNotificationsTests.swift` around lines 24 - 30,
awaitNotificationProcessed currently posts the notification then uses a fixed
delay to assume processing, which doesn't actually synchronize with observers;
replace the fixed-delay approach by creating an XCTest expectation bound to the
notification (e.g. using expectation(forNotification: name, object: object,
handler: nil) or
NotificationCenter.addObserver(forName:object:queue:using:)-backed expectation),
then post the notification and wait(for: [expectation], timeout:), removing the
DispatchQueue.main.asyncAfter-based fulfill so the test truly waits for the
observer invoked by awaitNotificationProcessed.
Comment on lines
+35
to
49
| func testURLForResourceWithDevRelativePathResolvesExistingFile() { | ||
| // The dev fallback resolves a file that exists relative to the cwd. | ||
| // Sources/parakeet_transcribe_pcm.py ships with the package. | ||
| let url = ResourceLocator.url( | ||
| forResource: "parakeet_transcribe_pcm", | ||
| withExtension: "py", | ||
| devRelativePath: "Sources/parakeet_transcribe_pcm.py" | ||
| ) | ||
| if let url = url { | ||
| XCTAssertEqual(url.lastPathComponent, "parakeet_transcribe_pcm.py") | ||
| XCTAssertTrue(FileManager.default.fileExists(atPath: url.path)) | ||
| } | ||
| // When run outside the package root the dev path is absent and the | ||
| // result is nil; either way no other resolution mode applies here. | ||
| } |
There was a problem hiding this comment.
Make the dev-path resolution test deterministic.
On Line 43, this test silently passes when url == nil, which can hide regressions even when the file exists in the current run environment. Assert success when the fixture file exists, and only allow nil when it truly does not exist.
Suggested test tightening
func testURLForResourceWithDevRelativePathResolvesExistingFile() {
- // The dev fallback resolves a file that exists relative to the cwd.
- // Sources/parakeet_transcribe_pcm.py ships with the package.
+ // The dev fallback should resolve when the fixture exists in cwd.
let url = ResourceLocator.url(
forResource: "parakeet_transcribe_pcm",
withExtension: "py",
devRelativePath: "Sources/parakeet_transcribe_pcm.py"
)
- if let url = url {
- XCTAssertEqual(url.lastPathComponent, "parakeet_transcribe_pcm.py")
- XCTAssertTrue(FileManager.default.fileExists(atPath: url.path))
- }
- // When run outside the package root the dev path is absent and the
- // result is nil; either way no other resolution mode applies here.
+ let expected = URL(fileURLWithPath: FileManager.default.currentDirectoryPath)
+ .appendingPathComponent("Sources/parakeet_transcribe_pcm.py")
+
+ if FileManager.default.fileExists(atPath: expected.path) {
+ XCTAssertNotNil(url)
+ XCTAssertEqual(url?.lastPathComponent, "parakeet_transcribe_pcm.py")
+ XCTAssertEqual(url?.standardizedFileURL, expected.standardizedFileURL)
+ } else {
+ XCTAssertNil(url)
+ }
}📝 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
| func testURLForResourceWithDevRelativePathResolvesExistingFile() { | |
| // The dev fallback resolves a file that exists relative to the cwd. | |
| // Sources/parakeet_transcribe_pcm.py ships with the package. | |
| let url = ResourceLocator.url( | |
| forResource: "parakeet_transcribe_pcm", | |
| withExtension: "py", | |
| devRelativePath: "Sources/parakeet_transcribe_pcm.py" | |
| ) | |
| if let url = url { | |
| XCTAssertEqual(url.lastPathComponent, "parakeet_transcribe_pcm.py") | |
| XCTAssertTrue(FileManager.default.fileExists(atPath: url.path)) | |
| } | |
| // When run outside the package root the dev path is absent and the | |
| // result is nil; either way no other resolution mode applies here. | |
| } | |
| func testURLForResourceWithDevRelativePathResolvesExistingFile() { | |
| // The dev fallback should resolve when the fixture exists in cwd. | |
| let url = ResourceLocator.url( | |
| forResource: "parakeet_transcribe_pcm", | |
| withExtension: "py", | |
| devRelativePath: "Sources/parakeet_transcribe_pcm.py" | |
| ) | |
| let expected = URL(fileURLWithPath: FileManager.default.currentDirectoryPath) | |
| .appendingPathComponent("Sources/parakeet_transcribe_pcm.py") | |
| if FileManager.default.fileExists(atPath: expected.path) { | |
| XCTAssertNotNil(url) | |
| XCTAssertEqual(url?.lastPathComponent, "parakeet_transcribe_pcm.py") | |
| XCTAssertEqual(url?.standardizedFileURL, expected.standardizedFileURL) | |
| } else { | |
| XCTAssertNil(url) | |
| } | |
| } |
🤖 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 `@Tests/Utilities/ResourceLocatorTests.swift` around lines 35 - 49, The test
testURLForResourceWithDevRelativePathResolvesExistingFile is nondeterministic
because it silently allows url == nil; update it to assert deterministically by
checking the dev fixture file existence directly: call
ResourceLocator.url(forResource:withExtension:devRelativePath:) as before, then
compute the expectedDevPath ("Sources/parakeet_transcribe_pcm.py") and use
FileManager.default.fileExists(atPath:) to decide assertions—if the file exists
assert url is non-nil, lastPathComponent equals "parakeet_transcribe_pcm.py",
and fileExists(atPath: url.path) is true; otherwise assert url is nil. This
ensures failures occur when the fixture is present but resolution returns nil.
Comment on lines
+126
to
+131
| // A non-nil 0.0 duration is under a minute, so it is formatted as a | ||
| // seconds string (locale-formatted number followed by "s"). | ||
| let formatted = record.formattedDuration | ||
| XCTAssertNotNil(formatted) | ||
| XCTAssertTrue(formatted?.hasSuffix("s") ?? false, | ||
| "Sub-minute duration should be formatted in seconds, got \(formatted ?? "nil")") |
There was a problem hiding this comment.
Avoid locale-coupled suffix assertions in duration formatting tests.
Line 130 assumes English output ("s" suffix). This can fail on non-English locales even when formatting is correct. Prefer a locale-agnostic behavioral assertion.
Example locale-agnostic adjustment
let formatted = record.formattedDuration
XCTAssertNotNil(formatted)
- XCTAssertTrue(formatted?.hasSuffix("s") ?? false,
- "Sub-minute duration should be formatted in seconds, got \(formatted ?? "nil")")
+ let oneMinuteRecord = TranscriptionRecord(
+ text: "Test",
+ provider: .parakeet,
+ duration: 60.0
+ )
+ XCTAssertNotEqual(formatted, oneMinuteRecord.formattedDuration,
+ "Sub-minute and minute durations should format differently")🤖 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 `@Tests/Views/Transcription/TranscriptionRecordRowTests.swift` around lines 126
- 131, The test in TranscriptionRecordRowTests.swift uses a hardcoded English
suffix ("s") to assert sub-minute formatting; change it to a locale-agnostic
check: assert formattedDuration is non-nil, does not use colon-based time (no
":"), and that the numeric portion — parsed with NumberFormatter to respect the
current locale — is approximately equal to the record.duration (or less than
60s). Locate the assertion around formattedDuration and record in the test,
replace the hasSuffix("s") check with parsing via NumberFormatter and a numeric
comparison to ensure the output represents seconds in a locale-safe way.
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
Executes the highest-leverage items from the
2026-05-16codebase audit (.claude/grade-report.md). Six focused commits:TranscriptionPipelineTestscaught every error intoXCTAssertTrue(true)— a broken pipeline still passed. They now inject a stub speech service and assert the transcript, correction outcome, and error propagation. All ~52XCTAssertTrue(true)tautologies across the suite are replaced with concrete assertions.ModelIntegrityno longer trusts the first download of a shipped model unconditionally — it verifies against a known-good SHA-256 table (hard fail on mismatch), keeping trust-on-first-use only for user-added models. The hash table ships empty with a documented placeholder; real release hashes must be populated before the pin is effective.FrameTimerhelper starts/stops the subscription on appear/disappear.@AppDefault. Constrained toEquatablesoupdate()compares directly instead of forcing a redundant async write every body pass.Task.Not included
Test plan
swift build+swift build --build-testscleanswiftlint --strictclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Performance & Stability
Tests