Conversation
…ecording permission issues - Fix TranslationService.testConnection() to create provider lazily for third-party engines (Google, DeepL, Baidu, LLM) that require credentials before the provider can be instantiated - Fix OnboardingViewModel.requestScreenRecordingPermission() to trigger ScreenCaptureKit API before opening System Settings, ensuring the app appears in the Screen Recording permission list Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthrough本次变更调整了屏幕录制权限请求流程为异步注册后再打开系统设置并启动轮询；同时调整了 TranslationService.testConnection(for:)，优先复用现有提供者并按需创建新提供者以检测连通性。 Changes
Sequence Diagram(s)sequenceDiagram
participant App as 应用
participant SCContent as SCShareableContent
participant Settings as 系统设置
participant Poller as 权限轮询器
App->>SCContent: call excludingDesktopWindows(...)
SCContent-->>App: 返回（忽略错误）
App->>Settings: 在 MainActor 上 打开 系统设置
Settings->>Settings: 用户在系统设置中授予/拒绝权限
App->>Poller: 启动权限轮询
Poller->>Poller: 周期性检查权限状态
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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.
🧹 Nitpick comments (2)
ScreenTranslate/Features/Onboarding/OnboardingViewModel.swift (1)
194-196:MainActor.run是多余的由于
OnboardingViewModel类已经标记为@MainActor（第 9 行），类内所有方法默认在 MainActor 上执行。在 Task 闭包内部，继承了 actor 上下文，因此await MainActor.run是不必要的。♻️ 简化代码
Task { do { // This will trigger the system to register the app in Screen Recording permissions _ = try await SCShareableContent.excludingDesktopWindows(false, onScreenWindowsOnly: true) } catch { // Expected when permission not granted - app is now registered } // Open System Settings after triggering the API - await MainActor.run { - openScreenRecordingSettings() - } + openScreenRecordingSettings() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Features/Onboarding/OnboardingViewModel.swift` around lines 194 - 196, Remove the redundant await MainActor.run wrapper inside OnboardingViewModel (class is already annotated `@MainActor`) and directly call openScreenRecordingSettings() from the Task closure; update the Task block that currently calls await MainActor.run { openScreenRecordingSettings() } to simply invoke openScreenRecordingSettings() so the call uses the existing actor context.ScreenTranslate/Services/TranslationService.swift (1)
395-398: 内置引擎返回false可能导致 UI 误导根据
EngineConfigSheet.swift(Context snippet 2)，当testConnection返回false时，UI 会显示红色 "xmark.circle.fill" 图标和 "engine.config.test.failed" 消息。对于内置引擎 (apple, mtranServer, ollama)，如果 provider 由于某种原因未注册，返回
false会让用户误以为连接失败，而实际上可能只是 provider 初始化问题。建议区分"引擎不可用"和"连接测试失败"两种情况，或者在日志中记录这种边界情况以便调试。
♻️ 建议：添加日志以便调试
guard engine.requiresAPIKey else { // Built-in engines should already be registered + logger.warning("Built-in engine \(engine.rawValue) has no registered provider") return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Services/TranslationService.swift` around lines 395 - 398, The guard in testConnection that returns false when engine.requiresAPIKey is false causes built-in engines to appear as failing in the UI; update the testConnection implementation so that for built-in engines (when engine.requiresAPIKey == false) it does NOT return false—instead detect if the provider is missing/unregistered, emit a clear debug/warning log (use the existing logging facility) with details about the missing provider, and return a positive/neutral result (e.g., true or a distinct successful/unknown result consistent with EngineConfigSheet.swift expectations) so the UI doesn't show an erroneous connection failure; reference the testConnection method and engine.requiresAPIKey check and add logging around the provider lookup/registration path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ScreenTranslate/Features/Onboarding/OnboardingViewModel.swift`:
- Around line 194-196: Remove the redundant await MainActor.run wrapper inside
OnboardingViewModel (class is already annotated `@MainActor`) and directly call
openScreenRecordingSettings() from the Task closure; update the Task block that
currently calls await MainActor.run { openScreenRecordingSettings() } to simply
invoke openScreenRecordingSettings() so the call uses the existing actor
context.
In `@ScreenTranslate/Services/TranslationService.swift`:
- Around line 395-398: The guard in testConnection that returns false when
engine.requiresAPIKey is false causes built-in engines to appear as failing in
the UI; update the testConnection implementation so that for built-in engines
(when engine.requiresAPIKey == false) it does NOT return false—instead detect if
the provider is missing/unregistered, emit a clear debug/warning log (use the
existing logging facility) with details about the missing provider, and return a
positive/neutral result (e.g., true or a distinct successful/unknown result
consistent with EngineConfigSheet.swift expectations) so the UI doesn't show an
erroneous connection failure; reference the testConnection method and
engine.requiresAPIKey check and add logging around the provider
lookup/registration path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4790527-a7a1-4bea-b1d6-0f36a2631b04
📒 Files selected for processing (2)
ScreenTranslate/Features/Onboarding/OnboardingViewModel.swiftScreenTranslate/Services/TranslationService.swift
- Remove redundant MainActor.run wrapper in OnboardingViewModel (class is already @mainactor) - Add logging for missing built-in engine providers and return true instead of false to avoid false connection failure in UI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ScreenTranslate/Services/TranslationService.swift`:
- Around line 395-400: The current guard handling for engine.requiresAPIKey
returns true when a provider is missing which causes false-positive connectivity
(e.g., .ollama is requiresAPIKey == false but can be unreachable); change the
logic in the guard block so that when a provider is not found you log the
warning via logger.warning("Built-in engine \(engine.rawValue) provider not
found in registry") and return false (or perform an actual connectivity probe)
instead of returning true, ensuring engine.requiresAPIKey == false does not
bypass real connectivity checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 258bf1b4-fbc8-43f2-9f35-5e8a0ad8f8d7
📒 Files selected for processing (2)
ScreenTranslate/Features/Onboarding/OnboardingViewModel.swiftScreenTranslate/Services/TranslationService.swift
Comment on lines
+395
to
+400
| guard engine.requiresAPIKey else { | ||
| // Built-in engines (apple, mtranServer) should already be registered in init | ||
| // If missing, log warning but return true to avoid false failure in UI | ||
| logger.warning("Built-in engine \(engine.rawValue) provider not found in registry") | ||
| return true | ||
| } |
There was a problem hiding this comment.
requiresAPIKey == false 分支会导致 .ollama 连通性误报成功。
当 provider 不存在时直接 return true 会跳过真实连接检查；而 .ollama 也属于 requiresAPIKey == false，因此会被误判为“连接成功”。
建议修复
- guard engine.requiresAPIKey else {
- // Built-in engines (apple, mtranServer) should already be registered in init
- // If missing, log warning but return true to avoid false failure in UI
- logger.warning("Built-in engine \(engine.rawValue) provider not found in registry")
- return true
- }
+ if !engine.requiresAPIKey {
+ switch engine {
+ case .apple, .mtranServer:
+ // 仅对预注册内建引擎保留当前兜底行为
+ logger.warning("Built-in engine \(engine.rawValue) provider not found in registry")
+ return true
+ default:
+ // 例如 .ollama：仍需创建 provider 并执行真实连通性检测
+ break
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ScreenTranslate/Services/TranslationService.swift` around lines 395 - 400,
The current guard handling for engine.requiresAPIKey returns true when a
provider is missing which causes false-positive connectivity (e.g., .ollama is
requiresAPIKey == false but can be unreachable); change the logic in the guard
block so that when a provider is not found you log the warning via
logger.warning("Built-in engine \(engine.rawValue) provider not found in
registry") and return false (or perform an actual connectivity probe) instead of
returning true, ensuring engine.requiresAPIKey == false does not bypass real
connectivity checks.
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
TranslationService.testConnection()to create provider lazily for third-party engines (Google, DeepL, Baidu, LLM) that require credentials before the provider can be instantiatedOnboardingViewModel.requestScreenRecordingPermission()to trigger ScreenCaptureKit API before opening System Settings, ensuring the app appears in the Screen Recording permission listProblem
Solution
TranslationService.testConnection()to automatically create the provider usingregistry.createProvider()if it doesn't existOnboardingViewModel.requestScreenRecordingPermission()to callSCShareableContent.excludingDesktopWindows()first, matching the behavior inSettingsViewModelTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit