Conversation
…d moved AI Settings and Feedback - This could have caused crash during launch before of attributegraph race condition. Also this commit adds more whisper models (cpu based for now) ; Also fixed preview disabling not working
…en Filler words are toggled - bug fix after arch change
…em In/Out to match FV preferences
|
Caution Review failedThe pull request is closed. WalkthroughCentralizes app services with a new AppServices singleton, unifies speech model configuration and migration via SettingsStore.SpeechModel, moves many views to EnvironmentObject-based dependency injection, introduces AudioStartupGate for deferred audio initialization, and adds new AI/feedback UI components and related service refinements. Changes
Sequence Diagram(s)sequenceDiagram
participant SwiftUI as SwiftUI App
participant AppServices as AppServices (singleton)
participant ASR as ASRService
participant AudioObs as AudioHardwareObserver
participant Settings as SettingsStore
participant Gate as AudioStartupGate
SwiftUI->>AppServices: create StateObject & inject into environment
AppServices->>ASR: instantiate (deferred)
AppServices->>AudioObs: instantiate observer
AppServices->>AppServices: forward child objectWillChange -> own objectWillChange
rect rgb(200,220,240)
Note over SwiftUI,Gate: Deferred audio startup
SwiftUI->>Gate: scheduleOpenAfterInitialUISettled()
Gate->>Gate: yield to run loop (x2) + optional sleep
Gate->>SwiftUI: open gate (resume waiters)
end
rect rgb(220,240,200)
SwiftUI->>ASR: initialize()
ASR->>Gate: await waitUntilOpen()
ASR->>Settings: read selectedSpeechModel
ASR->>ASR: checkIfModelsExist(), prepare engine/whisper as needed
ASR->>AppServices: publish state changes
AppServices->>SwiftUI: propagate UI refresh
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/Fluid/Services/MeetingTranscriptionService.swift (1)
2-3: Remove duplicate imports.The file contains duplicate imports for
AVFoundation(lines 2-3) andCombine(lines 7-8). Remove one of each.Apply this diff:
import Foundation import AVFoundation -import AVFoundation #if arch(arm64) import FluidAudio #endif import Combine -import CombineAlso applies to: 7-8
Sources/Fluid/CustomAnimations.swift (2)
55-60: Fix the animation trigger logic.The comparison
abs(newLevel - (data.audioLevel))will always evaluate to 0 because within theonChange(of: data.audioLevel)closure,newLevelis the new value ofdata.audioLevel, making them identical. This prevents the animation from ever triggering.Apply this diff to fix the logic by using the old value parameter:
-.onChange(of: data.audioLevel) { _, newLevel in +.onChange(of: data.audioLevel) { oldLevel, newLevel in // Only trigger animation update for significant changes to prevent cycles - if abs(newLevel - (data.audioLevel)) > 0.1 { + if abs(newLevel - oldLevel) > 0.1 { animationId = UUID() } }
89-94: Fix the animation trigger logic.Same issue as in
PulseAudioVisualizationView: the comparisonabs(newLevel - (data.audioLevel))will always be 0, preventing animations from triggering.Apply this diff:
-.onChange(of: data.audioLevel) { _, newLevel in +.onChange(of: data.audioLevel) { oldLevel, newLevel in // Only trigger animation update for significant changes to prevent cycles - if abs(newLevel - (data.audioLevel)) > 0.1 { + if abs(newLevel - oldLevel) > 0.1 { animationId = UUID() } }
🧹 Nitpick comments (9)
Sources/Fluid/Services/MeetingTranscriptionService.swift (1)
163-177: Consider making export functions async to avoid blocking.The
nonisolatedmarking is appropriate since these functions don't access actor-isolated state. However, both functions perform synchronous file I/O operations (write(to:...)) that could block the caller. Consider making themasyncand wrapping the file I/O inTask.detachedor using async file APIs.Example refactor for
exportToText:nonisolated func exportToText(_ result: TranscriptionResult, to destinationURL: URL) async throws { let content = """ Transcription: \(result.fileName) Date: \(result.timestamp.formatted()) Duration: \(String(format: "%.1f", result.duration))s Processing Time: \(String(format: "%.1f", result.processingTime))s Confidence: \(String(format: "%.1f%%", result.confidence * 100)) --- \(result.text) """ try await Task.detached { try content.write(to: destinationURL, atomically: true, encoding: .utf8) }.value }Also applies to: 180-187
Sources/Fluid/UI/WelcomeView.swift (1)
467-477: Consider removing redundant@MainActorannotation.SwiftUI view bodies and
onAppearclosures already execute on the main actor. TheTask { @MainActor in }wrapper is redundant here since the enclosed code doesn't perform any async work that would hop off the main actor..onAppear { // CRITICAL FIX: Refresh microphone and model status immediately on appear // This prevents the Quick Setup from showing stale status before ASRService.initialize() runs - Task { @MainActor in - // Check microphone status without triggering the full initialize() delay - asr.micStatus = AVCaptureDevice.authorizationStatus(for: .audio) - - // Check if models exist on disk - asr.checkIfModelsExist() - } + // Check microphone status without triggering the full initialize() delay + asr.micStatus = AVCaptureDevice.authorizationStatus(for: .audio) + + // Check if models exist on disk + asr.checkIfModelsExist() }Sources/Fluid/UI/FeedbackView.swift (2)
237-249: Consider limiting log content size to prevent oversized payloads.Reading 30 lines is reasonable, but log lines can vary significantly in length. For robustness, consider also limiting total character count.
let recentLines = Array(lines.suffix(30)) // Last 30 lines feedbackContent += "Recent Log Entries:\n" -feedbackContent += recentLines.joined(separator: "\n") +let logSnippet = recentLines.joined(separator: "\n") +// Limit to ~10KB to prevent oversized payloads +feedbackContent += String(logSnippet.prefix(10_000))
258-286: Add timeout configuration for feedback submission.The URLRequest uses default timeout. For user-facing feedback submission, consider adding explicit timeout to provide better UX.
var request = URLRequest(url: url) request.httpMethod = "POST" request.setValue("application/json", forHTTPHeaderField: "Content-Type") +request.timeoutInterval = 30 request.httpBody = try JSONSerialization.data(withJSONObject: data)Sources/Fluid/UI/AISettingsView.swift (1)
393-405: Consider extractingisLocalEndpointto a shared utility.This function is duplicated from
ContentView.swift(lines 1097-1132 per relevant snippets). Extract to a shared location to reduce duplication.Based on the relevant code snippets,
ContentView.swifthas an identical implementation. Consider creating a shared utility:// In a shared file like NetworkUtilities.swift enum NetworkUtilities { static func isLocalEndpoint(_ urlString: String) -> Bool { guard let url = URL(string: urlString), let host = url.host else { return false } let hostLower = host.lowercased() if hostLower == "localhost" || hostLower == "127.0.0.1" { return true } if hostLower.hasPrefix("127.") || hostLower.hasPrefix("10.") || hostLower.hasPrefix("192.168.") { return true } if hostLower.hasPrefix("172.") { let components = hostLower.split(separator: ".") if components.count >= 2, let secondOctet = Int(components[1]), secondOctet >= 16 && secondOctet <= 31 { return true } } return false } }Sources/Fluid/Services/ASRService.swift (2)
152-171: Good crash mitigation approach with thorough explanation.The refactoring to store
AVAudioEngineasAnyObject?with a computed property wrapper is a reasonable workaround for the AttributeGraph reflection crash mentioned in the PR objectives. The extensive comments clearly document the issue and rationale.One observation: if the cast on line 165 ever fails unexpectedly, a new engine would be created without disposing of the old one. While unlikely in normal usage, consider adding debug logging if this edge case is a concern:
private var engine: AVAudioEngine { if let existing = engineStorage as? AVAudioEngine { return existing + } else if engineStorage != nil { + DebugLogger.shared.warning("AVAudioEngine cast failed, creating new instance", source: "ASRService") } let created = AVAudioEngine()
405-409: Reasonable UX decision with trade-offs.Suppressing error popups for common cases (e.g., "audio too short" from accidental hotkey presses) improves the user experience. However, this also silences potentially important errors like CoreML failures or out-of-memory conditions.
Consider optionally differentiating between expected vs. unexpected errors:
// Show popup only for unexpected/serious errors if case let error as NSError = error, error.domain != "ExpectedErrorDomain" { self.errorTitle = "Transcription Failed" self.errorMessage = error.localizedDescription self.showError = true }Sources/Fluid/Persistence/SettingsStore.swift (2)
70-73: Clarify comment about replaced settings.The comment "replaces above two" is positioned in a way that's slightly unclear. It actually replaces
selectedTranscriptionProvider(line 69) andwhisperModelSize(line 70), but the comment appears after the whisperModelSize declaration.Consider repositioning or rewording:
// Transcription Provider (ASR) static let selectedTranscriptionProvider = "SelectedTranscriptionProvider" static let whisperModelSize = "WhisperModelSize" -// Unified Speech Model (replaces above two) +// Unified Speech Model (replaces selectedTranscriptionProvider + whisperModelSize) static let selectedSpeechModel = "SelectedSpeechModel"
905-933: Legacy enum retained for migration - consider deprecation comment.The
WhisperModelSizeenum duplicates filename information from the newSpeechModelenum but is necessary for backward compatibility during migration.Consider adding a deprecation comment to prevent future use:
+/// Legacy Whisper model size enum - preserved for migration from old settings. +/// New code should use SpeechModel instead. enum WhisperModelSize: String, CaseIterable, Identifiable {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
Sources/Fluid/CustomAnimations.swift(2 hunks)Sources/Fluid/Networking/ModelDownloader.swift(0 hunks)Sources/Fluid/Persistence/SettingsStore.swift(4 hunks)Sources/Fluid/Services/ASRService.swift(8 hunks)Sources/Fluid/Services/AppServices.swift(1 hunks)Sources/Fluid/Services/AudioDeviceService.swift(2 hunks)Sources/Fluid/Services/AudioStartupGate.swift(1 hunks)Sources/Fluid/Services/CommandModeService.swift(1 hunks)Sources/Fluid/Services/GlobalHotkeyManager.swift(2 hunks)Sources/Fluid/Services/MeetingTranscriptionService.swift(1 hunks)Sources/Fluid/Services/MenuBarManager.swift(1 hunks)Sources/Fluid/Services/WhisperProvider.swift(1 hunks)Sources/Fluid/UI/AISettingsView.swift(1 hunks)Sources/Fluid/UI/FeedbackView.swift(1 hunks)Sources/Fluid/UI/MouseTracker.swift(1 hunks)Sources/Fluid/UI/RecordingView.swift(1 hunks)Sources/Fluid/UI/SettingsView.swift(7 hunks)Sources/Fluid/UI/WelcomeView.swift(4 hunks)Sources/Fluid/Views/CommandModeView.swift(4 hunks)Sources/Fluid/Views/NotchContentViews.swift(3 hunks)Sources/Fluid/Views/RewriteModeView.swift(3 hunks)Sources/Fluid/fluidApp.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Sources/Fluid/Networking/ModelDownloader.swift
🧰 Additional context used
🧬 Code graph analysis (8)
Sources/Fluid/UI/FeedbackView.swift (3)
Sources/Fluid/UI/GlossyEffects.swift (1)
buttonHoverEffect(50-52)Sources/Fluid/Services/FileLogger.swift (1)
currentLogFileURL(55-58)Sources/Fluid/Services/DebugLogger.swift (2)
error(141-143)info(133-135)
Sources/Fluid/UI/AISettingsView.swift (5)
Sources/Fluid/UI/MouseTracker.swift (1)
body(52-86)Sources/Fluid/ContentView.swift (4)
body(1919-1924)defaultModels(899-905)providerKey(892-896)isLocalEndpoint(1098-1133)Sources/Fluid/Views/CommandModeView.swift (1)
defaultModels(569-575)Sources/Fluid/Views/RewriteModeView.swift (1)
defaultModels(304-311)Sources/Fluid/Persistence/SettingsStore.swift (3)
hasCustomReasoningConfig(577-580)getReasoningConfig(527-560)setReasoningConfig(563-574)
Sources/Fluid/Views/CommandModeView.swift (1)
Sources/Fluid/Views/NotchContentViews.swift (1)
scrollToBottom(768-778)
Sources/Fluid/Persistence/SettingsStore.swift (1)
Sources/Fluid/Services/DebugLogger.swift (1)
info(133-135)
Sources/Fluid/UI/WelcomeView.swift (2)
Sources/Fluid/Services/WhisperProvider.swift (1)
modelsExistOnDisk(72-74)Sources/Fluid/Services/ASRService.swift (1)
checkIfModelsExist(253-256)
Sources/Fluid/Services/MenuBarManager.swift (1)
Sources/Fluid/Services/NotchOverlayManager.swift (1)
updateTranscriptionText(243-245)
Sources/Fluid/Views/NotchContentViews.swift (2)
Sources/Fluid/TalkingAnimations.swift (1)
updateBars(163-202)Sources/Fluid/Views/CommandModeView.swift (1)
scrollToBottom(359-365)
Sources/Fluid/UI/SettingsView.swift (3)
Sources/Fluid/Services/AudioStartupGate.swift (2)
scheduleOpenAfterInitialUISettled(26-41)waitUntilOpen(44-50)Sources/Fluid/ContentView.swift (1)
refreshDevices(882-886)Sources/Fluid/Services/AudioDeviceService.swift (2)
getDefaultInputDevice(65-68)getDefaultOutputDevice(70-73)
🔇 Additional comments (45)
Sources/Fluid/Services/MeetingTranscriptionService.swift (2)
25-160: Well-structured service with good practices.The service demonstrates several strong patterns:
- Dependency injection of ASRService to avoid duplicate model loading
- Comprehensive error handling with custom error types
- Progress tracking for user feedback
- Platform-specific compilation with clear error messages for unsupported architectures
- File format validation with helpful error messages
11-23: Verify intentional exclusion ofidfrom CodingKeys.The
CodingKeysenum excludes theidproperty, which means each decodedTranscriptionResultwill receive a new UUID rather than preserving the original. This could be problematic if:
- Results need to maintain stable identities across encode/decode cycles
- Results are persisted and later reloaded for comparison or referencing
Please confirm this is the intended behavior. If stable IDs are needed, add
idto theCodingKeysenum.Sources/Fluid/Services/CommandModeService.swift (1)
189-189: LGTM!Removing the unused
chatIDbinding simplifies the guard statement without changing behavior.Sources/Fluid/UI/MouseTracker.swift (1)
78-78: LGTM!The two-parameter onChange closure aligns with the project-wide pattern without altering behavior.
Sources/Fluid/Services/AudioDeviceService.swift (1)
170-173: LGTM!The shift from PassthroughSubject to a
@Publishedtick counter simplifies SwiftUI integration and avoids AttributeGraph issues. Using wrapping increment (&+=) prevents overflow.Also applies to: 216-223
Sources/Fluid/fluidApp.swift (1)
15-15: LGTM!The centralized AppServices injection enables environment-based dependency access across the UI, aligning with the architectural refactor.
Also applies to: 23-23
Sources/Fluid/UI/RecordingView.swift (1)
12-13: LGTM!The shift to AppServices-based dependency injection via a private computed accessor maintains encapsulation while enabling centralized service access.
Sources/Fluid/Services/MenuBarManager.swift (1)
92-96: LGTM!Gating the notch transcription update behind
enableStreamingPreviewcorrectly implements user control over the live preview feature, addressing issue #52 mentioned in the PR objectives.Sources/Fluid/Services/GlobalHotkeyManager.swift (2)
597-605: LGTM!The lazy initialization flag update prevents false "initializing" states when the event tap is already functional, making the health check more robust.
633-633: LGTM!Explicitly setting
isInitialized = trueafter successful recovery ensures consistent state tracking.Sources/Fluid/Services/AudioStartupGate.swift (1)
18-59: Well-designed actor-based gate for startup coordination.The actor isolation ensures thread-safe state management, and the continuation-based waiting pattern is appropriate. The
@MainActortask properly yields to the run loop before opening the gate.One consideration: if
scheduleOpenAfterInitialUISettled()is never called, anywaitUntilOpen()calls will hang indefinitely. This appears intentional given the design, but ensure all entry points reliably call the schedule method.Sources/Fluid/Views/NotchContentViews.swift (3)
350-362: LGTM - onChange signature updated to modern two-parameter form.The updated closure signatures correctly adopt the newer
onChange(of:)API that provides both old and new values. The unused old value is appropriately discarded with_.
755-764: LGTM - Consistent onChange signature updates.The conversation area's
onChangehandlers are correctly updated to the two-parameter form, maintaining the existing scroll-to-bottom behavior.
925-927: LGTM - ExpandedModeWaveformView onChange updated.Consistent with the rest of the file's onChange signature updates.
Sources/Fluid/Views/RewriteModeView.swift (3)
5-6: Clean dependency injection via AppServices.The computed property accessor pattern (
private var asr: ASRService { appServices.asr }) keeps the code readable while leveraging the centralized service container. SinceAppServicesforwardsobjectWillChangefromasr, UI updates will propagate correctly.
167-173: LGTM - Provider selection onChange with validation.The two-parameter onChange signature is correctly applied. The disabled Apple Intelligence tag handling prevents invalid selection states.
219-223: LGTM - finalText onChange updated.Correctly propagates ASR transcription to the input field.
Sources/Fluid/Services/AppServices.swift (1)
18-48: Well-structured service container with proper change propagation and deferred initialization.The Combine-based forwarding of
objectWillChangefrom child services is the correct approach to maintain SwiftUI reactivity while reducing type complexity. The@MainActorannotation ensures thread-safe access from views. BothAudioHardwareObserver()andASRService()constructors are lightweight—each contains only a comment explaining why framework initialization is deferred to later lifecycle methods (startObserving()andinitialize()respectively), avoiding race conditions with AttributeGraph metadata processing.Sources/Fluid/UI/WelcomeView.swift (3)
13-14: LGTM! Clean migration to environment-based dependency injection.The change from
@ObservedObject var asr: ASRServiceto@EnvironmentObject var appServices: AppServiceswith a computed accessor aligns well with the PR's centralized service architecture.
64-76: Good defensive UX improvement.Showing the model step as complete when models exist on disk (even if not loaded) provides better user feedback and avoids confusion when models are cached but not yet initialized.
389-392: Explicit binding is appropriate for computed property access.Using
Binding(get:set:)is the correct pattern when accessing a mutable property through a computed accessor since$asr.finalTextwouldn't work with the environment-based indirection.Sources/Fluid/UI/FeedbackView.swift (1)
12-21: LGTM! Clean state management for feedback form.Local state variables are appropriate for this self-contained feedback form component.
Sources/Fluid/Views/CommandModeView.swift (4)
5-6: LGTM! Consistent migration to AppServices pattern.The environment-based access pattern matches other views in this PR.
54-61: LGTM! Updated onChange signatures.The two-parameter
onChangeclosure form{ _, newValue in }is the modern SwiftUI API that provides both old and new values.
299-310: LGTM! Scroll behavior optimization.Good decision to remove onChange for
service.streamingText(as noted in comment on line 311) since scrolling on every token would be expensive. The current triggers for conversation history count, processing state, and step changes are appropriate.
457-462: LGTM! Proper guard against invalid provider selection.The onChange handler correctly prevents selecting the disabled Apple Intelligence option and falls back to OpenAI.
Sources/Fluid/UI/AISettingsView.swift (4)
14-16: LGTM! Clean enum definition for connection status.The
AIConnectionStatusenum clearly represents all possible states during connection testing.
85-112: LGTM! Proper lifecycle management with model status refresh.The
onAppearcorrectly refreshes model status, and theonChangehandlers properly sync state toSettingsStoreand related managers.
544-687: LGTM! Comprehensive connection testing with proper error handling.The
testAPIConnectionfunction handles various scenarios including local endpoints, reasoning config, and provides meaningful error messages for different failure modes.
626-637: Verify timeout is appropriate for all network conditions.The 30-second timeout (line 629) is reasonable, but connection testing sends a real API request with "test" content. This may incur costs on metered APIs.
Consider using a models list endpoint or a minimal request to verify connectivity without sending actual inference requests:
-"messages": [["role": "user", "content": "test"]] +// Consider using /models endpoint for cheaper verification +// or document that this sends a real (small) API requestSources/Fluid/UI/SettingsView.swift (5)
13-14: LGTM! Consistent AppServices migration.The environment-based access pattern matches other views in this refactor.
335-338: LGTM! Updated onChange signature.The two-parameter form with ignored old value is appropriate here.
451-458: LGTM! Device selection with proper recording state handling.Good defensive check to restart recording if device changes while recording is active.
588-600: Excellent fix for CoreAudio/SwiftUI race condition.The
AudioStartupGatepattern properly defers device queries until after the audio subsystem is initialized, preventing theEXC_BAD_ACCESScrashes mentioned in the comments. ThescheduleOpenAfterInitialUISettled()followed bywaitUntilOpen()ensures safe sequencing.
601-603: LGTM! Slider value persistence.The onChange handler properly persists visualizer threshold changes to SettingsStore.
Sources/Fluid/Services/ASRService.swift (6)
100-110: LGTM - Clean provider selection based on unified model.The refactored provider selection correctly uses the centralized
SettingsStore.shared.selectedSpeechModeland cleanly determines the appropriate provider based on the model type.
132-135: LGTM - Simplified provider naming.The refactored
activeProviderNamecorrectly delegates to the centralized speech model's display name.
145-149: Good UX fix - prevents false "Download" state after provider switch.The immediate disk check after reset ensures the UI accurately reflects the new model's availability, preventing confusion when switching between already-downloaded models.
211-223: Critical fix - deferred initialization prevents AttributeGraph race.The removal of framework-triggering API calls from
init()is essential for preventing the launch-time crashes mentioned in the PR objectives. The comprehensive comments clearly document which APIs to avoid and why.
234-236: LGTM - Properly deferred model check.The deferred
checkIfModelsExist()call aligns with the initialization strategy to avoid framework race conditions.
365-365: Good fix for Bluetooth headset release (addresses issue #50).Adding
engine.reset()after stopping the engine ensures proper release of Bluetooth audio hardware by invalidating the audio node graph. This pattern is consistently applied in both stop methods.Sources/Fluid/Persistence/SettingsStore.swift (4)
899-902: LGTM - Accurate description update.The updated description correctly clarifies that Whisper uses CPU-based transcription (whisper.cpp) and works universally across Mac architectures.
950-963: LGTM - Legacy property maintained for migration.The
whisperModelSizeproperty is correctly implemented as a UserDefaults-backed setting, preserved for backward compatibility during migration to the unifiedSpeechModel.
969-988: LGTM - Robust model selection with architecture validation.The
selectedSpeechModelproperty correctly validates architecture compatibility (lines 975-977) and falls back to.whisperBaseon Intel Macs if the saved model requires Apple Silicon. This handles cross-platform settings migration gracefully.
991-1019: LGTM - Comprehensive migration logic.The migration from old
TranscriptionProviderOption+WhisperModelSizeto the new unifiedSpeechModelis well-implemented:
- Covers all legacy provider options (auto, fluidAudio, whisper)
- Correctly maps all five Whisper model sizes
- Validates architecture compatibility for FluidAudio → Parakeet migration
- Persists the migrated value to prevent re-migration
- Logs the migration for debugging
The migration is automatically triggered on first access (line 982) and only runs once thanks to persistence (line 1015).
| private func defaultModels(for providerKey: String) -> [String] { | ||
| switch providerKey { | ||
| case "openai": return ["gpt-4.1"] | ||
| case "groq": return ["openai/gpt-oss-120b"] | ||
| default: return [] | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify defaultModels implementations across the codebase
rg -n "func defaultModels" --type swift -A 6Repository: altic-dev/FluidVoice
Length of output: 2415
Inconsistent default models with other views.
AISettingsView.swift returns ["gpt-4.1"] for OpenAI, but CommandModeView.swift and RewriteModeView.swift return ["gpt-4o", "gpt-4-turbo", "gpt-3.5-turbo"]. ContentView.swift also uses the older ["gpt-4.1"] model. This inconsistency creates different default model lists across views for the same provider.
🤖 Prompt for AI Agents
In Sources/Fluid/UI/AISettingsView.swift around lines 370 to 376, the
defaultModels(for:) function returns inconsistent defaults for the "openai"
provider (["gpt-4.1"]) compared to CommandModeView.swift and
RewriteModeView.swift; update the "openai" case to return the same list used
elsewhere (["gpt-4o", "gpt-4-turbo", "gpt-3.5-turbo"]) so all views share the
same defaults, and keep or confirm the "groq" and default cases unchanged.
altic-dev
changed the title
…th user-facing alert and retry option 2. DefaultModels: Update CommandModeView and RewriteModeView to use gpt-4.1 instead of deprecated gpt-4o/gpt-4-turbo/gpt-3.5-turbo 3. WhisperProvider: Fix stale model bug by tracking loadedModelName and forcing reload when user switches models
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Fluid/Services/WhisperProvider.swift (1)
35-65: Consider adding synchronization to prevent concurrentprepare()calls.The
prepare()method modifies shared mutable state (isReady,whisper,loadedModelName) without synchronization. If called concurrently:
- Multiple tasks could simultaneously detect the model change and reset
isReady = false- Both would pass the guard at line 45 and proceed to download/load
- Race conditions on writing
whisper,loadedModelName, andisReadyThis could lead to wasted downloads and potential file system conflicts.
Consider one of these approaches:
Option 1: Actor isolation (Swift 5.5+)
-final class WhisperProvider: TranscriptionProvider { +actor WhisperProvider: TranscriptionProvider {Option 2: Serial task queue
+private var prepareTask: Task<Void, Error>? + func prepare(progressHandler: ((Double) -> Void)? = nil) async throws { + // Reuse in-flight preparation + if let existing = prepareTask { + return try await existing.value + } + + let task = Task { // Detect model change: if a different model is now selected, force reload let currentModelName = modelName ... + } + prepareTask = task + defer { prepareTask = nil } + try await task.value }Option 3: NSLock (simpler but less Swift-native)
+private let prepareLock = NSLock() + func prepare(progressHandler: ((Double) -> Void)? = nil) async throws { + prepareLock.lock() + defer { prepareLock.unlock() } // Detect model change...Sources/Fluid/Views/RewriteModeView.swift (1)
281-302: Missing legacy key handling for backward compatibility.CommandModeView includes legacy key handling (
custom:\\(trimmed)) at line 564 to support configurations affected by a prior string interpolation bug, but RewriteModeView'sproviderKeysfunction lacks this fix. This inconsistency could prevent RewriteModeView from loading custom provider configurations that require the legacy key.Apply this diff to add the missing legacy key handling:
} else { keys.append("custom:\(trimmed)") keys.append(trimmed) } + // Add legacy key used in ContentView before the fix + keys.append("custom:\\(trimmed)") + return Array(Set(keys)) }
♻️ Duplicate comments (2)
Sources/Fluid/Services/MeetingTranscriptionService.swift (1)
9-9: Invalidnonisolatedmodifier on struct declaration.This issue was already flagged in a previous review. The
nonisolatedkeyword cannot be applied to struct declarations—it's only valid for methods and properties within actor-isolated contexts. Remove the modifier; theSendableconformance already ensures safe cross-concurrency usage.Sources/Fluid/Views/RewriteModeView.swift (1)
306-310: Verify the model name "gpt-4.1".Same issue as in CommandModeView:
"gpt-4.1"does not appear to be a valid OpenAI model name. This would cause API errors when attempting to use this model.Please confirm the intended model name matches valid OpenAI API models.
🧹 Nitpick comments (3)
Sources/Fluid/Services/WhisperProvider.swift (1)
83-85: Note:modelsExistOnDisk()checks selected model, not loaded model.This method checks for the currently selected model (via the computed
modelNameproperty), not the loaded model tracked byloadedModelName.If the user changes model settings after loading, this could return
falseeven whenisReady == trueand a different model is loaded. This is likely intentional (checking availability for the current configuration), but worth noting for future maintenance.If the semantics should reflect the loaded model instead, consider:
func modelsExistOnDisk() -> Bool { - return FileManager.default.fileExists(atPath: modelURL.path) + guard let loaded = loadedModelName else { return false } + let url = modelDirectory.appendingPathComponent(loaded) + return FileManager.default.fileExists(atPath: url.path) }Sources/Fluid/Views/CommandModeView.swift (1)
359-365: Consider consolidating duplicate scroll logic.Similar
scrollToBottomimplementations exist in NotchContentViews.swift (with 0.05s delay) and here (with 0.1s delay). Consider extracting a shared utility function if the timing difference isn't significant, or document why different delays are needed.Sources/Fluid/UI/FeedbackView.swift (1)
275-275: Extract API URL as a constant.The hardcoded feedback API URL could be extracted as a constant for better maintainability.
Add a constant at the top of the struct:
struct FeedbackView: View { @Environment(\.theme) private var theme private static let feedbackAPIURL = "https://altic.dev/api/fluid/feedback" // MARK: - State Variables (moved from ContentView)Then use it in the function:
- guard let url = URL(string: "https://altic.dev/api/fluid/feedback") else { + guard let url = URL(string: Self.feedbackAPIURL) else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Sources/Fluid/Services/MeetingTranscriptionService.swift(1 hunks)Sources/Fluid/Services/WhisperProvider.swift(4 hunks)Sources/Fluid/UI/FeedbackView.swift(1 hunks)Sources/Fluid/Views/CommandModeView.swift(5 hunks)Sources/Fluid/Views/RewriteModeView.swift(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Sources/Fluid/Views/CommandModeView.swift (1)
Sources/Fluid/Views/NotchContentViews.swift (1)
scrollToBottom(768-778)
Sources/Fluid/Services/WhisperProvider.swift (1)
Sources/Fluid/Services/DebugLogger.swift (1)
info(133-135)
🔇 Additional comments (10)
Sources/Fluid/Services/WhisperProvider.swift (1)
18-18: Excellent implementation of model change detection.The addition of
loadedModelNametracking and the model change detection logic inprepare()directly addresses the previous review concern about stale model references. The implementation correctly:
- Tracks the currently loaded model via
loadedModelName- Detects when
SettingsStoresettings change by capturing and comparing model names- Forces a reload by resetting state when the selected model differs from the loaded one
- Includes helpful logging for debugging
- Properly resets state in
clearCache()Also applies to: 22-24, 36-43, 62-62, 64-64, 93-93
Sources/Fluid/Views/CommandModeView.swift (4)
5-6: LGTM: Clean migration to centralized service access.The environment-based ASR access through AppServices is a solid architectural improvement that enables app-wide service management and simplifies dependency injection.
54-61: LGTM: onChange closures correctly updated.All
onChangehandlers properly migrated to the two-parameter form, using underscores for unused old values where appropriate.Also applies to: 299-310, 457-462
563-564: LGTM: Good backward compatibility fix.The legacy key handling addresses a prior string interpolation bug where provider keys were stored with literal
\(trimmed)text instead of the interpolated value. This ensures smooth migration for existing configurations.
571-574: The model name"gpt-4.1"is a valid OpenAI API model as of December 2025. No issues with this code.Likely an incorrect or invalid review comment.
Sources/Fluid/Views/RewriteModeView.swift (2)
5-6: LGTM: Consistent service access pattern.The migration to environment-based ASR access matches CommandModeView and maintains consistency across the codebase.
167-173: LGTM: onChange handlers correctly updated.Both
onChangeclosures properly use the two-parameter form, consistent with the broader refactoring.Also applies to: 219-223
Sources/Fluid/UI/FeedbackView.swift (3)
12-24: Well-structured state management.The state variables are properly organized and comprehensive. The addition of
showFeedbackErrorandfeedbackErrorMessagesuccessfully addresses the past review comment about missing user feedback on submission failure.
111-188: Well-implemented feedback form UI.The form provides good UX with appropriate validation, loading states, and visual feedback. The disabled state correctly prevents submission when inputs are empty or while sending.
214-241: Excellent error handling implementation.The function properly addresses the past review comment by providing user feedback on failure (lines 235-239), preserving form inputs for retry, and displaying a helpful error message. The async/await pattern with MainActor is correctly implemented.
|
|
||
| Spacer() | ||
|
|
||
| Link(destination: URL(string: "https://github.com/altic-dev/Fluid-oss")!) { |
There was a problem hiding this comment.
Avoid force-unwrapping URL.
Force-unwrapping the URL with ! will crash the app if the string is malformed. Even hardcoded URLs should be safely unwrapped.
Apply this diff to safely handle the URL:
- Link(destination: URL(string: "https://github.com/altic-dev/Fluid-oss")!) {
+ if let githubURL = URL(string: "https://github.com/altic-dev/Fluid-oss") {
+ Link(destination: githubURL) {
HStack(spacing: 8) {
Image(systemName: "star.fill")
Text("Star on GitHub")
.fontWeight(.semibold)
}
.font(.system(size: 14))
.foregroundStyle(.white)
.padding(.horizontal, 16)
.padding(.vertical, 10)
.background(
LinearGradient(
colors: [.purple, .blue],
startPoint: .leading,
endPoint: .trailing
)
)
.cornerRadius(8)
+ }
+ .buttonStyle(.plain)
+ .buttonHoverEffect()
}
- .buttonStyle(.plain)
- .buttonHoverEffect()🤖 Prompt for AI Agents
In Sources/Fluid/UI/FeedbackView.swift around line 85, the Link uses a
force-unwrapped URL("https://github.com/altic-dev/Fluid-oss")! which can crash
if the string is malformed; change it to safely unwrap the URL (e.g., create the
URL with URL(string:), guard/if-let it, and only render the Link when the URL is
non-nil or provide a safe fallback) so the view never force-unwraps and cannot
crash due to a nil URL.
| if includeDebugLogs { | ||
| feedbackContent += "\n\n--- Debug Information ---\n" | ||
| feedbackContent += "App Version: \(Bundle.main.infoDictionary?["CFBundleShortVersionString"] as? String ?? "Unknown")\n" | ||
| feedbackContent += "Build: \(Bundle.main.infoDictionary?["CFBundleVersion"] as? String ?? "Unknown")\n" | ||
| feedbackContent += "macOS Version: \(ProcessInfo.processInfo.operatingSystemVersionString)\n" | ||
| feedbackContent += "Date: \(Date().formatted())\n\n" | ||
|
|
||
| // Add recent log entries | ||
| let logFileURL = FileLogger.shared.currentLogFileURL() | ||
| if FileManager.default.fileExists(atPath: logFileURL.path) { | ||
| do { | ||
| let logContent = try String(contentsOf: logFileURL) | ||
| let lines = logContent.components(separatedBy: .newlines) | ||
| let recentLines = Array(lines.suffix(30)) // Last 30 lines | ||
| feedbackContent += "Recent Log Entries:\n" | ||
| feedbackContent += recentLines.joined(separator: "\n") | ||
| } catch { | ||
| feedbackContent += "Could not read log file: \(error.localizedDescription)\n" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential PII exposure in debug logs.
Including debug logs may expose sensitive user data (emails, user identifiers, etc.) if the application logs such information. While users opt in via the toggle, they may not understand the privacy implications.
Consider these mitigations:
- Add a privacy warning near the toggle (lines 153-154):
Toggle("Include debug logs", isOn: $includeDebugLogs)
.toggleStyle(GlassToggleStyle())
Text("Debug logs may include technical information. Sensitive data will be redacted.")
.font(.caption)
.foregroundStyle(.secondary)- Implement log sanitization in
createFeedbackData:
// Add recent log entries (sanitized)
let logFileURL = FileLogger.shared.currentLogFileURL()
if FileManager.default.fileExists(atPath: logFileURL.path) {
do {
let logContent = try String(contentsOf: logFileURL)
let lines = logContent.components(separatedBy: .newlines)
let recentLines = Array(lines.suffix(30))
// Sanitize PII: redact emails, IP addresses, etc.
let sanitizedLines = recentLines.map { sanitizePII($0) }
feedbackContent += "Recent Log Entries:\n"
feedbackContent += sanitizedLines.joined(separator: "\n")
} catch {
feedbackContent += "Could not read log file: \(error.localizedDescription)\n"
}
}Based on coding guidelines mentioned in PR objectives regarding PII handling.
🤖 Prompt for AI Agents
In Sources/Fluid/UI/FeedbackView.swift around lines 246-266, the code appends
raw recent log entries into feedback which can leak PII; sanitize log content
before appending by mapping recentLines through a sanitizePII(_:) helper that
redacts emails, IP addresses, personal identifiers (UUIDs, phone numbers) and
long tokens, then join and append the sanitizedLines instead of raw lines; also
add a privacy warning next to the "Include debug logs" toggle at ~lines 153-154
informing users that debug logs may contain sensitive data and that logs will be
redacted when sent.
| private func submitFeedback(data: [String: Any]) async -> Bool { | ||
| guard let url = URL(string: "https://altic.dev/api/fluid/feedback") else { | ||
| DebugLogger.shared.error("Invalid feedback API URL", source: "FeedbackView") | ||
| return false | ||
| } | ||
|
|
||
| do { | ||
| var request = URLRequest(url: url) | ||
| request.httpMethod = "POST" | ||
| request.setValue("application/json", forHTTPHeaderField: "Content-Type") | ||
| request.httpBody = try JSONSerialization.data(withJSONObject: data) | ||
|
|
||
| let (_, response) = try await URLSession.shared.data(for: request) | ||
|
|
||
| if let httpResponse = response as? HTTPURLResponse { | ||
| let success = (200...299).contains(httpResponse.statusCode) | ||
| if success { | ||
| DebugLogger.shared.info("Feedback submitted successfully", source: "FeedbackView") | ||
| } else { | ||
| DebugLogger.shared.error("Feedback submission failed with status: \(httpResponse.statusCode)", source: "FeedbackView") | ||
| } | ||
| return success | ||
| } | ||
| return false | ||
| } catch { | ||
| DebugLogger.shared.error("Network error submitting feedback: \(error.localizedDescription)", source: "FeedbackView") | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Add timeout to prevent indefinite hangs.
The network request lacks a timeout configuration, which could cause the UI to hang indefinitely if the server is unresponsive. This would leave isSendingFeedback stuck in the true state.
Apply this diff to add a timeout:
private func submitFeedback(data: [String: Any]) async -> Bool {
guard let url = URL(string: "https://altic.dev/api/fluid/feedback") else {
DebugLogger.shared.error("Invalid feedback API URL", source: "FeedbackView")
return false
}
do {
var request = URLRequest(url: url)
request.httpMethod = "POST"
request.setValue("application/json", forHTTPHeaderField: "Content-Type")
+ request.timeoutInterval = 30.0 // 30 second timeout
request.httpBody = try JSONSerialization.data(withJSONObject: data)
let (_, response) = try await URLSession.shared.data(for: request)📝 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.
| private func submitFeedback(data: [String: Any]) async -> Bool { | |
| guard let url = URL(string: "https://altic.dev/api/fluid/feedback") else { | |
| DebugLogger.shared.error("Invalid feedback API URL", source: "FeedbackView") | |
| return false | |
| } | |
| do { | |
| var request = URLRequest(url: url) | |
| request.httpMethod = "POST" | |
| request.setValue("application/json", forHTTPHeaderField: "Content-Type") | |
| request.httpBody = try JSONSerialization.data(withJSONObject: data) | |
| let (_, response) = try await URLSession.shared.data(for: request) | |
| if let httpResponse = response as? HTTPURLResponse { | |
| let success = (200...299).contains(httpResponse.statusCode) | |
| if success { | |
| DebugLogger.shared.info("Feedback submitted successfully", source: "FeedbackView") | |
| } else { | |
| DebugLogger.shared.error("Feedback submission failed with status: \(httpResponse.statusCode)", source: "FeedbackView") | |
| } | |
| return success | |
| } | |
| return false | |
| } catch { | |
| DebugLogger.shared.error("Network error submitting feedback: \(error.localizedDescription)", source: "FeedbackView") | |
| return false | |
| } | |
| } | |
| private func submitFeedback(data: [String: Any]) async -> Bool { | |
| guard let url = URL(string: "https://altic.dev/api/fluid/feedback") else { | |
| DebugLogger.shared.error("Invalid feedback API URL", source: "FeedbackView") | |
| return false | |
| } | |
| do { | |
| var request = URLRequest(url: url) | |
| request.httpMethod = "POST" | |
| request.setValue("application/json", forHTTPHeaderField: "Content-Type") | |
| request.timeoutInterval = 30.0 // 30 second timeout | |
| request.httpBody = try JSONSerialization.data(withJSONObject: data) | |
| let (_, response) = try await URLSession.shared.data(for: request) | |
| if let httpResponse = response as? HTTPURLResponse { | |
| let success = (200...299).contains(httpResponse.statusCode) | |
| if success { | |
| DebugLogger.shared.info("Feedback submitted successfully", source: "FeedbackView") | |
| } else { | |
| DebugLogger.shared.error("Feedback submission failed with status: \(httpResponse.statusCode)", source: "FeedbackView") | |
| } | |
| return success | |
| } | |
| return false | |
| } catch { | |
| DebugLogger.shared.error("Network error submitting feedback: \(error.localizedDescription)", source: "FeedbackView") | |
| return false | |
| } | |
| } |
🤖 Prompt for AI Agents
In Sources/Fluid/UI/FeedbackView.swift around lines 274 to 302, the POST request
has no timeout and can hang indefinitely; set a request timeout to prevent UI
from staying in the sending state (e.g. set request.timeoutInterval = 30 before
calling URLSession.shared.data or create a URLSession with a
URLSessionConfiguration where timeoutIntervalForRequest is set), ensuring the
request always fails within a bounded time so submitFeedback returns and the UI
can clear isSendingFeedback.
Update Parakeet TDT language support from English to 25 Languages in SettingsStore and AISettingsView to accurately reflect multilingual capability ; Removed Temperature setting for models with reasoning as they only take 1.0 ( can remove the temperature fully or add a new field for custom temp later if needed)
1 participant
if endifcondition duplication; proper update when Filler words are toggled (bug fix after architecture change).Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.