-
Notifications
You must be signed in to change notification settings - Fork 0
Grade-report fixes: test quality, security, performance #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7f0604a
b28bf6a
39c4a87
1a83d50
6e6caa1
093505b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -163,10 +163,11 @@ internal final class DataManager: DataManagerProtocol { | |||||
| try context.save() | ||||||
|
|
||||||
| Logger.dataManager.info("Saved transcription record with ID: \(record.id)") | ||||||
|
|
||||||
| // Perform cleanup after save to maintain retention policy | ||||||
| await cleanupExpiredRecordsQuietly() | ||||||
|
|
||||||
|
|
||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win Consider using The PR description mentions a "detached Task," but the code uses unstructured ♻️ Suggested change- Task { await cleanupExpiredRecordsQuietly() }
+ Task.detached { await self.cleanupExpiredRecordsQuietly() }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
|
|
||||||
| } catch { | ||||||
| Logger.dataManager.error("Failed to save transcription record: \(error.localizedDescription)") | ||||||
| throw DataManagerError.saveFailed(error) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import SwiftUI | ||
| import Combine | ||
|
|
||
| /// A stoppable per-frame timer for waveform animations. | ||
| /// | ||
| /// `Timer.publish(...).autoconnect()` runs forever the moment a view is | ||
| /// created — even for waveform tiles that are off-screen or in an idle state. | ||
| /// With the Visuals grid showing 8 styles at once that is hundreds of | ||
| /// main-thread state mutations per second while nothing is visible. | ||
| /// | ||
| /// `FrameTimer` instead exposes a passthrough `publisher` that only emits | ||
| /// while `start()` has been called and `stop()` has not. Views drive it from | ||
| /// `onAppear`/`onDisappear`, so the timer is fully cancelled — not merely | ||
| /// ignored — whenever the view leaves the screen. | ||
| 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 | ||
| } | ||
|
Comment on lines
+15
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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 While not strictly required by the guideline (which targets UI components), marking FrameTimer with Suggested change+@MainActor
final class FrameTimer {🤖 Prompt for AI Agents |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 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'srequests/urllib3check 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