fix: remove redundant permission dialogs and fix language code handling#70
Merged
Conversation
- Remove custom permission explanation dialogs, use system prompts directly - Fix double dialog issue when requesting accessibility permission - Add fuzzy matching for language codes (zh → zh-Hans, zh-TW → zh-Hant) - Fix localeLanguage to preserve script subtags for Apple Translation - Remove unreliable LanguageAvailability pre-check, rely on TranslationSession errors - Use full BCP 47 identifiers in error messages instead of minimalIdentifier - Improve translation error display with dedicated showTranslationError method
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough该 PR 引入多引擎翻译结果汇总与展示（engine-level result bundle），改造翻译协议/服务以返回每引擎结果并在弹窗中展示多引擎卡片；同时调整多处设置持久化调用、权限请求简化与翻译/插入错误呈现，本地化新增插入失败文案。 Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Coordinator as TextTranslationCoordinator
participant PermissionMgr as PermissionManager
participant Service as TranslationService
participant Engine as TranslationEngine
participant Provider as TranslationProvider
participant UI as TextTranslationPopupView
User->>Coordinator: 发起翻译请求
rect rgba(100,150,200,0.5)
Coordinator->>PermissionMgr: requestAccessibilityPermission()
PermissionMgr->>PermissionMgr: 调用系统权限请求
PermissionMgr-->>Coordinator: 返回权限状态
end
rect rgba(100,200,150,0.5)
Coordinator->>Service: translateBundle(segments, target, preferredEngine, mode, ...)
Service->>Service: filterEnabledEngines()
par 并行/多引擎调用
Service->>Engine: translateWithEngine(engine, ...)
Engine->>Provider: 调用对应提供商
Provider-->>Engine: 返回 segments / error
Engine-->>Service: 返回 EngineResult
and
Service->>Service: 收集 EngineResult -> EngineTranslationInfo 列表
end
Service-->>Coordinator: 返回 TranslationResultBundle (engineResults...)
end
rect rgba(200,150,100,0.5)
alt 有成功引擎
Coordinator->>UI: 显示 engineResults
UI->>User: 渲染多引擎卡片或单一翻译
else 全部失败
Coordinator->>Coordinator: showTranslationError(error)
Coordinator->>User: NSAlert 显示错误
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Owner
Author
|
Tip For best results, initiate chat on the files or code changes.
请告诉我您需要什么帮助！ |
Owner
Author
|
@coderabbitai 代码审查 — 审查此 PR 的代码变更 |
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ScreenTranslate/Services/PermissionManager.swift (1)
104-116:⚠️ Potential issue | 🟠 MajorInput Monitoring 权限检查使用了错误的 API。
hasInputMonitoringPermission当前通过AXIsProcessTrustedWithOptions检查，但这个 API 只覆盖无障碍权限，而非 Input Monitoring 权限。当用户已授予无障碍权限但未授予 Input Monitoring 权限时，此函数会错误返回true，导致输入监控功能实际失效。应使用
CGPreflightListenEventAccess()检查实际的 Input Monitoring 权限状态，并在拒绝后才用CGRequestListenEventAccess()请求权限。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Services/PermissionManager.swift` around lines 104 - 116, The current hasInputMonitoringPermission check and request flow is using AXIsProcessTrustedWithOptions (accessibility) which is wrong for Input Monitoring; update the hasInputMonitoringPermission implementation to call CGPreflightListenEventAccess() to determine actual Input Monitoring status, and in requestInputMonitoringPermission replace the AX-based request with CGRequestListenEventAccess() to prompt the system (falling back to openInputMonitoringSettings() only if CGRequestListenEventAccess() fails or is insufficient); ensure references to hasInputMonitoringPermission, requestInputMonitoringPermission, and openInputMonitoringSettings are updated accordingly so the permission flow uses CGPreflightListenEventAccess() for checks and CGRequestListenEventAccess() for requests.ScreenTranslate/Services/TranslationService.swift (1)
194-237:⚠️ Potential issue | 🟠 Major并行结果顺序现在是不稳定的。
TaskGroup收集结果时按完成先后 append，bundle.results的顺序会随网络延迟变化。下游在TextTranslationFlow中用first(where: { $0.isSuccess })选择主显示文本，导致主译文在不同运行间可能随机切到不同引擎的结果。建议按engines的输入顺序重建结果数组。建议修改
- let results = await withTaskGroup(of: EngineResult.self, returning: [EngineResult].self) { group in - for engine in engines { + let results = await withTaskGroup(of: (Int, EngineResult).self, returning: [EngineResult].self) { group in + for (index, engine) in engines.enumerated() { group.addTask { do { let start = Date() let provider = try await self.resolvedProvider(for: engine) @@ - guard !bilingualSegments.isEmpty else { - return EngineResult.failed( + guard !bilingualSegments.isEmpty else { + return (index, EngineResult.failed( engine: engine, error: TranslationProviderError.translationFailed( "\(provider.name) returned no results" ) - ) + )) } - return EngineResult( + return (index, EngineResult( engine: engine, segments: bilingualSegments, latency: Date().timeIntervalSince(start) - ) + )) } catch { - return EngineResult.failed(engine: engine, error: error) + return (index, EngineResult.failed(engine: engine, error: error)) } } } - var collectedResults: [EngineResult] = [] - for await result in group { - collectedResults.append(result) + var collectedResults = Array<EngineResult?>(repeating: nil, count: engines.count) + for await (index, result) in group { + collectedResults[index] = result } - return collectedResults + return collectedResults.compactMap { $0 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Services/TranslationService.swift` around lines 194 - 237, The concurrent TaskGroup collects EngineResult in completion order, making the returned `results` unstable; rebuild the final array to match the input `engines` order: inside the await `withTaskGroup` after collecting `collectedResults`, create a lookup (e.g., [Engine: EngineResult]) from `collectedResults` keyed by `engine` and then return engines.map { lookup[$0] ?? EngineResult.failed(engine: $0, error: TranslationProviderError.translationFailed("no result")) } so the returned [EngineResult] preserves the original `engines` order (refer to `engines`, `EngineResult`, and the translateWithResolvedPrompt/resolvedProvider task block).
🧹 Nitpick comments (2)
ScreenTranslate/Features/Settings/MultiEngineSettingsSection.swift (1)
641-641: 优化：移除冗余的let关键字。根据 SwiftLint 提示，当丢弃函数返回值时，应使用
_ = foo()而非let _ = foo()。♻️ 建议修复
- let _ = Logger.settings.info("engineCard \(engine.rawValue): isEnabled=\(config.isEnabled), fromDefault=\(viewModel.settings.engineConfigs[engine] == nil)") + _ = Logger.settings.info("engineCard \(engine.rawValue): isEnabled=\(config.isEnabled), fromDefault=\(viewModel.settings.engineConfigs[engine] == nil)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Features/Settings/MultiEngineSettingsSection.swift` at line 641, The Logger call currently discards its return value using the redundant `let _ =` pattern; update the invocation in MultiEngineSettingsSection (the `Logger.settings.info("engineCard \(engine.rawValue): isEnabled=\(config.isEnabled), fromDefault=\(viewModel.settings.engineConfigs[engine] == nil)")` line) to discard the result correctly by removing `let` (either call `Logger.settings.info(...)` directly or use `_ = Logger.settings.info(...)`), so the `let _ =` is eliminated.ScreenTranslate/Features/Settings/SettingsViewModel.swift (1)
301-307:nil分支不要直接绕过AppSettings。这两个 setter 的写入路径都走
settings，但删除路径直接落到UserDefaults.standard。SettingsViewModel本身允许注入自定义AppSettings，所以测试或非默认 suite 下会出现 set/remove 走不同后端的问题。建议给AppSettings补一个 remove helper，并统一从同一抽象层处理。Also applies to: 338-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Features/Settings/SettingsViewModel.swift` around lines 301 - 307, The nil branch currently removes keys directly via UserDefaults.standard, bypassing the injected AppSettings and causing inconsistent backends; add a remove(forKey:) helper to AppSettings (or an equivalent method on the existing protocol) and change the nil branches in SettingsViewModel setters (e.g., the translationTargetLanguage setter and the similar setter at the 338-344 region) to call settings.remove(forKey: AppSettings.Keys.translationTargetLanguage) instead of UserDefaults.standard.removeObject(...), ensuring all get/set/remove operations go through the same AppSettings abstraction.
🤖 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/App/Coordinators/TextTranslationCoordinator.swift`:
- Around line 242-250: The fallback localization key used in
showTranslationError (alert.messageText) is a format string with a %@
placeholder but is being used without parameters; update showTranslationError to
either use a non‑formatted/localization key (e.g., a generic
"textTranslation.error.generic") or call the formatted localization with a
supplied argument (for example the error.localizedDescription or
error.errorDescription) so the %@ placeholder is satisfied; adjust
alert.messageText assignment accordingly and keep TextTranslationError handling
consistent.
In `@ScreenTranslate/Features/Settings/SettingsViewModel.swift`:
- Around line 368-370: 当前的 vlmAPIKey 属性在 setter 中调用 settings.save(..., forKey:
AppSettings.Keys.vlmAPIKey) 将敏感 API key 写入通用设置（可能为 UserDefaults/
plist）；请改为使用项目现有的 KeychainService 持久化：在 SettingsViewModel 的 vlmAPIKey setter
中替换对 settings.save 的调用，改为通过 KeychainService 提供的存/取方法保存和读取 vlmAPIKey（保留 getter 对
settings.vlmAPIKey 的读取逻辑或改为从 Keychain 读取，确保使用相同的唯一键名并处理可能的失败/错误情况），并移除将 API key
写入通用设置的路径。
In `@ScreenTranslate/Features/TextTranslation/TextTranslationPopupView.swift`:
- Around line 274-278: Remove the trailing comma after the last element in the
engineResults array literal (the EngineTranslationInfo entries) so the array no
longer ends with an extraneous comma; update the array in
TextTranslationPopupView where engineResults is defined/initialized to ensure
the final EngineTranslationInfo item has no following comma to satisfy
SwiftLint's trailing_comma rule.
- Around line 133-169: The translatedText and errorMessage Text views in
engineResultSection currently hardcode .leading alignment; change
engineResultSection to read the environment layout direction (add
`@Environment`(\.layoutDirection) var layoutDirection) and compute an alignment
variable (let textAlignment: TextAlignment = layoutDirection == .rightToLeft ?
.trailing : .leading and let frameAlignment: Alignment = layoutDirection ==
.rightToLeft ? .trailing : .leading) then apply
multilineTextAlignment(textAlignment) and .frame(maxWidth: .infinity, alignment:
frameAlignment) to both the Text(result.translatedText) and Text(error) branches
so RTL languages render with trailing alignment; keep other UI behavior intact
and reuse the same alignment logic for both success and error text branches.
In `@ScreenTranslate/Models/AppSettings.swift`:
- Around line 874-880: 在 loadParallelEngines() 中修复“脏数据回退”不一致的问题：当前 guard 分支对
missing key 返回 [.apple]，但当持久化数组存在但全部解析失败（engines.isEmpty）时又返回 [.apple,
.mtranServer]。将 engines.isEmpty 的回退值改为与 guard 分支一致（即只返回 [.apple]），确保
Keys.parallelEngines 的默认恢复行为与新的默认值一致，引用符号：loadParallelEngines(),
Keys.parallelEngines, TranslationEngineType, engines.
In `@ScreenTranslate/Services/TranslationEngine.swift`:
- Around line 76-93: The prefix-match in fromTranslationCode(_:) is reversed:
currently it checks candidate.rawValue.lowercased().hasPrefix(lowercased) which
fails for longer BCP47 inputs like "en-US" or "zh-Hans-CN"; change the predicate
to test whether the input (lowercased) hasPrefix the candidate raw value (e.g.
lowercased.hasPrefix(candidate.rawValue.lowercased())) so region/script variants
map to their base TranslationLanguage. Update the final return that uses
TranslationLanguage.allCases.first(where:) to use this corrected hasPrefix
direction and keep the existing normalization flow (use normalized/lowercased
variables already present).
In `@ScreenTranslate/Services/TranslationService.swift`:
- Around line 404-412: filterEnabledEngines currently lets a config explicitly
disable .apple because it only defaults to true when the config is missing;
change the predicate to short-circuit and always include .apple by first
checking if engine == .apple and returning true, otherwise consult
AppSettings.shared.engineConfigs[engine]?.isEnabled (treat missing entries as
false). Update the filter inside filterEnabledEngines (and reference
AppSettings.shared.engineConfigs and TranslationEngineType.isEnabled) so Apple
is unconditionally kept.
---
Outside diff comments:
In `@ScreenTranslate/Services/PermissionManager.swift`:
- Around line 104-116: The current hasInputMonitoringPermission check and
request flow is using AXIsProcessTrustedWithOptions (accessibility) which is
wrong for Input Monitoring; update the hasInputMonitoringPermission
implementation to call CGPreflightListenEventAccess() to determine actual Input
Monitoring status, and in requestInputMonitoringPermission replace the AX-based
request with CGRequestListenEventAccess() to prompt the system (falling back to
openInputMonitoringSettings() only if CGRequestListenEventAccess() fails or is
insufficient); ensure references to hasInputMonitoringPermission,
requestInputMonitoringPermission, and openInputMonitoringSettings are updated
accordingly so the permission flow uses CGPreflightListenEventAccess() for
checks and CGRequestListenEventAccess() for requests.
In `@ScreenTranslate/Services/TranslationService.swift`:
- Around line 194-237: The concurrent TaskGroup collects EngineResult in
completion order, making the returned `results` unstable; rebuild the final
array to match the input `engines` order: inside the await `withTaskGroup` after
collecting `collectedResults`, create a lookup (e.g., [Engine: EngineResult])
from `collectedResults` keyed by `engine` and then return engines.map {
lookup[$0] ?? EngineResult.failed(engine: $0, error:
TranslationProviderError.translationFailed("no result")) } so the returned
[EngineResult] preserves the original `engines` order (refer to `engines`,
`EngineResult`, and the translateWithResolvedPrompt/resolvedProvider task
block).
---
Nitpick comments:
In `@ScreenTranslate/Features/Settings/MultiEngineSettingsSection.swift`:
- Line 641: The Logger call currently discards its return value using the
redundant `let _ =` pattern; update the invocation in MultiEngineSettingsSection
(the `Logger.settings.info("engineCard \(engine.rawValue):
isEnabled=\(config.isEnabled),
fromDefault=\(viewModel.settings.engineConfigs[engine] == nil)")` line) to
discard the result correctly by removing `let` (either call
`Logger.settings.info(...)` directly or use `_ = Logger.settings.info(...)`), so
the `let _ =` is eliminated.
In `@ScreenTranslate/Features/Settings/SettingsViewModel.swift`:
- Around line 301-307: The nil branch currently removes keys directly via
UserDefaults.standard, bypassing the injected AppSettings and causing
inconsistent backends; add a remove(forKey:) helper to AppSettings (or an
equivalent method on the existing protocol) and change the nil branches in
SettingsViewModel setters (e.g., the translationTargetLanguage setter and the
similar setter at the 338-344 region) to call settings.remove(forKey:
AppSettings.Keys.translationTargetLanguage) instead of
UserDefaults.standard.removeObject(...), ensuring all get/set/remove operations
go through the same AppSettings abstraction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab41c3a9-ee8f-45ca-abb5-af85e2214503
📒 Files selected for processing (22)
ScreenTranslate/App/Coordinators/TextTranslationCoordinator.swiftScreenTranslate/Features/Settings/MultiEngineSettingsSection.swiftScreenTranslate/Features/Settings/SettingsViewModel.swiftScreenTranslate/Features/TextTranslation/TextTranslationFlow.swiftScreenTranslate/Features/TextTranslation/TextTranslationPopupView.swiftScreenTranslate/Features/TextTranslation/TextTranslationPopupWindow.swiftScreenTranslate/Models/AppSettings.swiftScreenTranslate/Resources/de.lproj/Localizable.stringsScreenTranslate/Resources/en.lproj/Localizable.stringsScreenTranslate/Resources/es.lproj/Localizable.stringsScreenTranslate/Resources/fr.lproj/Localizable.stringsScreenTranslate/Resources/it.lproj/Localizable.stringsScreenTranslate/Resources/ja.lproj/Localizable.stringsScreenTranslate/Resources/ko.lproj/Localizable.stringsScreenTranslate/Resources/pt.lproj/Localizable.stringsScreenTranslate/Resources/ru.lproj/Localizable.stringsScreenTranslate/Resources/zh-Hans.lproj/Localizable.stringsScreenTranslate/Services/AppleTranslationProvider.swiftScreenTranslate/Services/PermissionManager.swiftScreenTranslate/Services/Protocols/TranslationServicing.swiftScreenTranslate/Services/TranslationEngine.swiftScreenTranslate/Services/TranslationService.swift
Comment on lines
368
to
+370
| var vlmAPIKey: String { | ||
| get { settings.vlmAPIKey } | ||
| set { settings.vlmAPIKey = newValue } | ||
| set { settings.vlmAPIKey = newValue; settings.save(newValue, forKey: AppSettings.Keys.vlmAPIKey) } |
There was a problem hiding this comment.
不要把 vlmAPIKey 存到通用设置存储。
这里把敏感凭证走 settings.save(...) 落盘；如果底层是 UserDefaults / plist，这会把 API key 以普通偏好项保存。项目里已经有 KeychainService，这里应改成 Keychain 持久化。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ScreenTranslate/Features/Settings/SettingsViewModel.swift` around lines 368 -
370, 当前的 vlmAPIKey 属性在 setter 中调用 settings.save(..., forKey:
AppSettings.Keys.vlmAPIKey) 将敏感 API key 写入通用设置（可能为 UserDefaults/
plist）；请改为使用项目现有的 KeychainService 持久化：在 SettingsViewModel 的 vlmAPIKey setter
中替换对 settings.save 的调用，改为通过 KeychainService 提供的存/取方法保存和读取 vlmAPIKey（保留 getter 对
settings.vlmAPIKey 的读取逻辑或改为从 Keychain 读取，确保使用相同的唯一键名并处理可能的失败/错误情况），并移除将 API key
写入通用设置的路径。
Comment on lines
+133
to
+169
| private func engineResultSection(_ result: EngineTranslationInfo) -> some View { | ||
| VStack(alignment: .leading, spacing: 6) { | ||
| HStack(spacing: 6) { | ||
| if result.isSuccess { | ||
| Image(systemName: "checkmark.circle.fill") | ||
| .font(.system(size: 11, weight: .medium)) | ||
| .foregroundStyle(.green) | ||
| } else { | ||
| Image(systemName: "xmark.circle.fill") | ||
| .font(.system(size: 11, weight: .medium)) | ||
| .foregroundStyle(.red) | ||
| } | ||
| Text(result.engine.localizedName) | ||
| .font(.system(size: 10, weight: .semibold)) | ||
| .foregroundStyle(result.isSuccess ? .green : .red) | ||
| .tracking(0.3) | ||
| Spacer() | ||
| if result.isSuccess { | ||
| Text(String(format: "%.1fs", result.latency)) | ||
| .font(.system(size: 9, weight: .regular, design: .monospaced)) | ||
| .foregroundStyle(.tertiary) | ||
| } | ||
| } | ||
|
|
||
| if let text = result.translatedText { | ||
| Text(text) | ||
| .font(.system(size: 14, weight: .medium)) | ||
| .foregroundStyle(.primary) | ||
| .multilineTextAlignment(.leading) | ||
| .textSelection(.enabled) | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| } else if let error = result.errorMessage { | ||
| Text(error) | ||
| .font(.system(size: 12, weight: .regular)) | ||
| .foregroundStyle(.red) | ||
| .multilineTextAlignment(.leading) | ||
| .frame(maxWidth: .infinity, alignment: .leading) |
There was a problem hiding this comment.
多引擎结果卡片丢了 RTL 对齐。
单结果分支会根据语言/文本切到 .trailing，但这里把成功和失败文案都固定成了 .leading。翻译到阿拉伯语、希伯来语这类 RTL 语言时，多引擎视图会退回成错误的阅读方向。
建议修改
private func engineResultSection(_ result: EngineTranslationInfo) -> some View {
- VStack(alignment: .leading, spacing: 6) {
+ let previewText = result.translatedText ?? result.errorMessage ?? ""
+ let isResultRTL = Self.isRTLLanguage(targetLanguage) || Self.containsRTLText(previewText)
+
+ VStack(alignment: isResultRTL ? .trailing : .leading, spacing: 6) {
HStack(spacing: 6) {
if result.isSuccess {
Image(systemName: "checkmark.circle.fill")
.font(.system(size: 11, weight: .medium))
.foregroundStyle(.green)
@@
if let text = result.translatedText {
Text(text)
.font(.system(size: 14, weight: .medium))
.foregroundStyle(.primary)
- .multilineTextAlignment(.leading)
+ .multilineTextAlignment(isResultRTL ? .trailing : .leading)
.textSelection(.enabled)
- .frame(maxWidth: .infinity, alignment: .leading)
+ .frame(maxWidth: .infinity, alignment: isResultRTL ? .trailing : .leading)
} else if let error = result.errorMessage {
Text(error)
.font(.system(size: 12, weight: .regular))
.foregroundStyle(.red)
- .multilineTextAlignment(.leading)
- .frame(maxWidth: .infinity, alignment: .leading)
+ .multilineTextAlignment(isResultRTL ? .trailing : .leading)
+ .frame(maxWidth: .infinity, alignment: isResultRTL ? .trailing : .leading)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ScreenTranslate/Features/TextTranslation/TextTranslationPopupView.swift`
around lines 133 - 169, The translatedText and errorMessage Text views in
engineResultSection currently hardcode .leading alignment; change
engineResultSection to read the environment layout direction (add
`@Environment`(\.layoutDirection) var layoutDirection) and compute an alignment
variable (let textAlignment: TextAlignment = layoutDirection == .rightToLeft ?
.trailing : .leading and let frameAlignment: Alignment = layoutDirection ==
.rightToLeft ? .trailing : .leading) then apply
multilineTextAlignment(textAlignment) and .frame(maxWidth: .infinity, alignment:
frameAlignment) to both the Text(result.translatedText) and Text(error) branches
so RTL languages render with trailing alignment; keep other UI behavior intact
and reuse the same alignment logic for both success and error text branches.
Comment on lines
+274
to
+278
| engineResults: [ | ||
| EngineTranslationInfo(engine: .mtranServer, translatedText: nil, errorMessage: "Connection refused", latency: 2.1), | ||
| EngineTranslationInfo(engine: .apple, translatedText: "你好，今天怎么样？", errorMessage: nil, latency: 0.5), | ||
| EngineTranslationInfo(engine: .google, translatedText: "你好，你今天好吗？", errorMessage: nil, latency: 1.0), | ||
| ], |
There was a problem hiding this comment.
删掉预览数组最后一项后的多余逗号。
这个会直接触发当前 SwiftLint 的 trailing_comma 警告。
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 277-277: Collection literals should not have trailing commas
(trailing_comma)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ScreenTranslate/Features/TextTranslation/TextTranslationPopupView.swift`
around lines 274 - 278, Remove the trailing comma after the last element in the
engineResults array literal (the EngineTranslationInfo entries) so the array no
longer ends with an extraneous comma; update the array in
TextTranslationPopupView where engineResults is defined/initialized to ensure
the final EngineTranslationInfo item has no following comma to satisfy
SwiftLint's trailing_comma rule.
- Fix prefix match direction in fromTranslationCode (en-US → .english now works) - Fix dirty data fallback to return [.apple] consistently - Fix filterEnabledEngines to truly always include Apple engine - Fix showTranslationError fallback key to avoid %@ placeholder issue
Owner
Author
CodeRabbit Review Replies#1 TextTranslationCoordinator L250 — ✅ Fixed in 90f521a. Changed fallback key to #2 SettingsViewModel L370 — Acknowledged. Pre-existing issue (vlmAPIKey in UserDefaults). Will migrate to Keychain in a separate PR. #3 TextTranslationPopupView L169 — Acknowledged. Pre-existing issue. Will add RTL alignment in a follow-up i18n PR. #4 TextTranslationPopupView L278 — Acknowledged. Pre-existing trailing comma. Will fix separately. #5 AppSettings — ✅ Fixed in 90f521a. Dirty data fallback now returns #6 TranslationEngine — ✅ Fixed in 90f521a. Prefix match direction corrected: input hasPrefix candidate, so "en-US" and "zh-Hans-CN" now match correctly. #7 TranslationService L412 — ✅ Fixed in 90f521a. Changed to |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ScreenTranslate/Models/AppSettings.swift (1)
99-99: 建议保持Keys枚举为private。将
Keys从private改为internal会暴露 UserDefaults 的实现细节。外部调用方应通过公开的save*方法来持久化数据，而不需要直接访问键名。如果确实有外部需要访问键名的场景，建议明确记录在注释中。建议保持封装
- enum Keys { + private enum Keys {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Models/AppSettings.swift` at line 99, The Keys enum currently exposes UserDefaults key names; make it private to retain encapsulation by changing the visibility of enum Keys to private and ensuring all internal references (e.g., in saveFoo/saveBar and related load methods in AppSettings) still access Keys directly; if external consumers really need key names, add a documented public accessor or comment rather than leaving Keys non‑private.
🤖 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/App/Coordinators/TextTranslationCoordinator.swift`:
- Around line 89-95: The current flow in TextTranslationCoordinator returns
false after calling permissionManager.requestAccessibilityPermission() without
informing the user if the permission was previously denied; update the logic in
the method that uses permissionManager.hasAccessibilityPermission to, after
calling requestAccessibilityPermission(), re-check
permissionManager.hasAccessibilityPermission and if it remains false call
showPermissionDeniedError(for: .accessibility) to present the "Open System
Settings" guidance to the user (this targets the block around
permissionManager.requestAccessibilityPermission() in TextTranslationCoordinator
and uses the existing showPermissionDeniedError(for:) helper).
---
Nitpick comments:
In `@ScreenTranslate/Models/AppSettings.swift`:
- Line 99: The Keys enum currently exposes UserDefaults key names; make it
private to retain encapsulation by changing the visibility of enum Keys to
private and ensuring all internal references (e.g., in saveFoo/saveBar and
related load methods in AppSettings) still access Keys directly; if external
consumers really need key names, add a documented public accessor or comment
rather than leaving Keys non‑private.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 159aed24-f84b-431c-aeca-e02c81ef53b6
📒 Files selected for processing (4)
ScreenTranslate/App/Coordinators/TextTranslationCoordinator.swiftScreenTranslate/Models/AppSettings.swiftScreenTranslate/Services/TranslationEngine.swiftScreenTranslate/Services/TranslationService.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- ScreenTranslate/Services/TranslationService.swift
System prompt is shown first (no custom dialog). If user denies, show "Open System Settings" guidance to help them grant permission later.
Owner
Author
Updated reply to inline reviewTextTranslationCoordinator L89-95 — ✅ Fixed in cf984b0. Now re-checks AppSettings Keys enum visibility — Acknowledged as nitpick. Pre-existing design choice, not in scope for this PR. Will consider in a future settings refactor. |
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/App/Coordinators/TextTranslationCoordinator.swift`:
- Around line 89-97: After calling
permissionManager.requestAccessibilityPermission(), re-check
permissionManager.hasAccessibilityPermission and return true if it is now
granted; only call permissionManager.showPermissionDeniedError(for:
.accessibility) and return false when the updated hasAccessibilityPermission
remains false. Update the logic in TextTranslationCoordinator (the block using
permissionManager.hasAccessibilityPermission,
permissionManager.requestAccessibilityPermission(), and
permissionManager.showPermissionDeniedError(for:)) so the post-request decision
is based on the refreshed hasAccessibilityPermission state instead of
unconditionally returning false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15c8cd99-1f06-4ed7-8fc2-6541952e1c6b
📒 Files selected for processing (1)
ScreenTranslate/App/Coordinators/TextTranslationCoordinator.swift
Re-check hasAccessibilityPermission after requestAccessibilityPermission(). If user granted in the system prompt, return true and continue the flow. Only show System Settings guidance when still denied after refresh.
Owner
Author
|
TextTranslationCoordinator L89-97 — ✅ Fixed in cf984b0. Now refreshes permission status after the system prompt via |
19 tasks
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
localeLanguage属性保留脚本子标签（zh-Hans 而非 zh），确保 Apple Translation 正确识别LanguageAvailability预检查，直接依赖TranslationSession的错误反馈showTranslationError方法Test plan
Summary by CodeRabbit
发布说明
新功能
改进
修复
本地化