test: harden translation pipeline regressions#69
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough本次提交将多处控制台打印替换为结构化 Logger，新增请求级 promptTemplate 协议与兼容提示上下文、注入可替换的 TranslationServicing 并将 scene 上下文传播至翻译调用；调整引擎注册、兼容提供者标识与回退合并逻辑，并新增大量单元测试覆盖翻译管线与提示解析（50 字内）。 Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Flow as TextTranslationFlow
participant Service as TranslationService
participant Registry as TranslationEngineRegistry
participant Primary as PrimaryProvider
participant Fallback as FallbackProvider
User->>Flow: 请求 translate(segments, to, scene)
Flow->>Service: translate(segments, to, preferredEngine, from, scene, mode, fallbackEnabled, parallelEngines, sceneBindings)
Service->>Registry: resolve engines(for scene/config)
Registry-->>Service: 返回 primary + fallback 列表
Service->>Primary: translate(texts, from, to, promptTemplate?, scene)
alt 主引擎成功
Primary-->>Service: 成功结果
Service-->>Flow: 返回主引擎结果包
else 主引擎失败
Primary-->>Service: 抛出错误
Service->>Fallback: translate(texts, from, to, promptTemplate?, scene)
Fallback-->>Service: 成功结果
Service-->>Flow: 返回合并包（包含 synthetic 主引擎失败结果 + 回退结果）
end
Flow-->>User: 最终 TranslationResultBundle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
诗歌
🚥 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ca9f5f1b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| targetLanguage: String | ||
| ) async { | ||
| guard let llmProvider = provider as? LLMTranslationProvider else { return } | ||
| guard let promptConfigurableProvider = provider as? TranslationPromptConfigurable else { return } |
There was a problem hiding this comment.
This new as? TranslationPromptConfigurable guard never succeeds for the real OpenAI/Claude/Gemini/Ollama providers, because TranslationEngineRegistry still creates LLMTranslationProvider instances and that actor only declares TranslationProvider conformance (ScreenTranslate/Services/Translation/Providers/LLMTranslationProvider.swift:12), even though it implements setCustomPromptTemplate at line 164. In production, every LLM translation will therefore skip the custom engine/scene prompt and the special translate-and-insert prompt, while the added tests still pass because their mock provider explicitly conforms to TranslationPromptConfigurable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 22b3f82. Verified with `xcodebuild test -project ScreenTranslate.xcodeproj -scheme ScreenTranslate -destination 'platform=macOS'
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ScreenTranslate/Features/TextTranslation/TextTranslationFlow.swift (1)
177-183:⚠️ Potential issue | 🟠 Major这里仍然走的是无
scene的旧服务入口。
TextTranslationConfig.forTranslateAndInsert()虽然区分了插入场景的配置，但这次调用的TranslationServicing.translate(...)没法把.translateAndInsert传到TranslationService。而ScreenTranslate/Services/TranslationService.swift里的applyPromptConfig是靠scene选 insert prompt 的，所以这条 UI 流程实际上还是拿不到插入专用 prompt。建议把 scene/context 加进TextTranslationConfig和TranslationServicing，并在这里走带 scene 的入口。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Features/TextTranslation/TextTranslationFlow.swift` around lines 177 - 183, The current call to TranslationServicing.translate(...) uses the old entry that lacks a scene, so the insert-specific prompt from TextTranslationConfig.forTranslateAndInsert() is never applied; update the types and call path to propagate the scene: add a scene/context property to TextTranslationConfig (e.g., include a Scene enum with .translateAndInsert), extend the TranslationServicing.translate(...) signature to accept a scene/context parameter, and adjust this call site in TextTranslationFlow to call translationService.translate(..., scene: .translateAndInsert) (or pass the config.scene) so TranslationService.applyPromptConfig can pick the insert prompt.
🧹 Nitpick comments (1)
ScreenTranslateTests/TranslationServicePipelineTests.swift (1)
76-89: 让 mock 在批量结果数量不匹配时显式失败。这里把单条
batchResults复制到所有输入，会把“结果数和输入数不一致”的真实回归吞掉。生产里的ScreenTranslate/Services/Translation/Providers/LLMTranslationProvider.swift:101-130是按输入逐条映射结果的；测试 mock 最好在数量不匹配时直接抛错，这样批量拆分问题不会被假阳性掩盖。🔧 建议修改
- if batchResults.count == 1, let first = batchResults.first { - return texts.map { text in - TranslationResult( - sourceText: text, - translatedText: first.translatedText, - sourceLanguage: first.sourceLanguage, - targetLanguage: first.targetLanguage - ) - } - } + if !batchResults.isEmpty { + throw TranslationProviderError.translationFailed( + "Mock misconfigured: expected \(texts.count) results, got \(batchResults.count)" + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslateTests/TranslationServicePipelineTests.swift` around lines 76 - 89, The mock in TranslationServicePipelineTests currently masks mismatched result counts by duplicating a single TranslationResult across all inputs; instead, change the mock so that when batchResults.count != texts.count it fails explicitly (throw an error or call XCTFail/fatalError) rather than mapping the first result to every input; locate the logic that inspects batchResults and texts and replace the "if batchResults.count == 1" duplication branch with an explicit failure path, ensuring callers receive a clear error when counts differ while preserving the successful return path when counts match and still returning [TranslationResult] with sourceText/translatedText/sourceLanguage/targetLanguage mapped per input.
🤖 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/Features/Settings/EngineConfigSheet.swift`:
- Line 344: The log line in EngineConfigSheet.swift that calls
logger.error("Failed to save credentials: \(error.localizedDescription, privacy:
.public)") exposes potential sensitive details; change the privacy level to
.private or mask/hash the error before logging (i.e., update the logger.error
call in EngineConfigSheet/related saveCredentials flow to use privacy: .private
or a redacted string instead of .public) so error.localizedDescription is not
logged publicly.
In `@ScreenTranslate/Services/ClaudeVLMProvider.swift`:
- Around line 167-168: 不要将 error.localizedDescription 直接写入公开日志：在 catch 块中（涉及
logWarning(...) 的位置）改为只记录尝试次数和错误类别（例如 String(describing: type(of: error)) 或
error.localizedDescription 的脱敏版本），或者将敏感文本改为私有日志级别；参照同文件的 parseVLMContent(_:)
做脱敏处理，修改触发的 logWarning 调用（保留函数名 logWarning 和相关上下文）以避免把 OCR/翻译原文暴露为 privacy:
.public。
In `@ScreenTranslate/Services/OpenAIVLMProvider.swift`:
- Around line 190-191: The current catch block logs error.localizedDescription
which can leak user/model content; update the catch in the OpenAIVLMProvider
parsing routine to avoid printing raw error text — call logWarning with only the
attempt number and the error type (e.g., String(describing: type(of: error)) or
error.localizedDescription redacted) and/or a generic message, and remove or
sanitize any response fragments before logging; locate the catch that calls
logWarning("Parse error on attempt \(attempt + 1):
\(error.localizedDescription)") and replace it with a message that does not
include the raw error content.
In `@ScreenTranslate/Services/TextSelectionService.swift`:
- Line 171: The log line using logger.warning currently emits the clipboard
restore error with privacy: .public; update the logging call in the
clipboard-restore code path (the logger.warning call that interpolates
error.localizedDescription) to use a private privacy level (e.g., privacy:
.private) so the error details are not exposed publicly in logs—locate the
logger.warning that references error.localizedDescription (in the clipboard
restore / TextSelectionService restore routine) and change the privacy specifier
accordingly.
In `@ScreenTranslate/Services/Translation/Providers/LLMTranslationProvider.swift`:
- Around line 164-166: The method setCustomPromptTemplate currently exists but
the type LLMTranslationProvider does not explicitly declare conformance to
TranslationPromptConfigurable, so the dynamic dispatch used by callers won't
pick it up; update the LLMTranslationProvider type declaration to conform to
TranslationPromptConfigurable and ensure the setCustomPromptTemplate signature
exactly matches the protocol's requirement (including async/throws and parameter
optionality if specified) so the provider's custom prompt will be applied at
runtime.
In `@ScreenTranslate/Services/Translation/TranslationEngineRegistry.swift`:
- Around line 31-42: The init registers built-in providers conditionally
(init(registerBuiltInProviders:)), but createProvider still unconditionally
throws an alreadyRegistered error for .apple/.mtranServer, causing semantic
mismatch when registration was skipped; update the createProvider logic (the
method handling provider creation/registration) to check the current providers
dictionary for the key (providers[.apple], providers[.mtranServer] or
providers[providerKey]) and only throw alreadyRegistered if an entry exists,
otherwise proceed to create and insert the provider into providers and return
it; ensure you reference the same provider keys/enums and preserve existing
behavior when the provider is present.
---
Outside diff comments:
In `@ScreenTranslate/Features/TextTranslation/TextTranslationFlow.swift`:
- Around line 177-183: The current call to TranslationServicing.translate(...)
uses the old entry that lacks a scene, so the insert-specific prompt from
TextTranslationConfig.forTranslateAndInsert() is never applied; update the types
and call path to propagate the scene: add a scene/context property to
TextTranslationConfig (e.g., include a Scene enum with .translateAndInsert),
extend the TranslationServicing.translate(...) signature to accept a
scene/context parameter, and adjust this call site in TextTranslationFlow to
call translationService.translate(..., scene: .translateAndInsert) (or pass the
config.scene) so TranslationService.applyPromptConfig can pick the insert
prompt.
---
Nitpick comments:
In `@ScreenTranslateTests/TranslationServicePipelineTests.swift`:
- Around line 76-89: The mock in TranslationServicePipelineTests currently masks
mismatched result counts by duplicating a single TranslationResult across all
inputs; instead, change the mock so that when batchResults.count != texts.count
it fails explicitly (throw an error or call XCTFail/fatalError) rather than
mapping the first result to every input; locate the logic that inspects
batchResults and texts and replace the "if batchResults.count == 1" duplication
branch with an explicit failure path, ensuring callers receive a clear error
when counts differ while preserving the successful return path when counts match
and still returning [TranslationResult] with
sourceText/translatedText/sourceLanguage/targetLanguage mapped per input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d3c74ae-4fb0-40ee-b0df-de3640f4d92c
📒 Files selected for processing (14)
ScreenTranslate/Features/Capture/ScreenDetector.swiftScreenTranslate/Features/Preview/AnnotationCanvas.swiftScreenTranslate/Features/Settings/EngineConfigSheet.swiftScreenTranslate/Features/TextTranslation/TextTranslationFlow.swiftScreenTranslate/Services/ClaudeVLMProvider.swiftScreenTranslate/Services/OpenAIVLMProvider.swiftScreenTranslate/Services/TextInsertService.swiftScreenTranslate/Services/TextSelectionService.swiftScreenTranslate/Services/Translation/Providers/LLMTranslationProvider.swiftScreenTranslate/Services/Translation/TranslationEngineRegistry.swiftScreenTranslate/Services/TranslationProvider.swiftScreenTranslate/Services/TranslationService.swiftScreenTranslateTests/TranslationConfigurationTests.swiftScreenTranslateTests/TranslationServicePipelineTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ScreenTranslate/Services/OpenAIVLMProvider.swift (2)
506-509:⚠️ Potential issue | 🟠 Major错误消息中同样包含原始内容片段，应移除。
与上文 279 行相同的问题，
cleanedContent.prefix(200)可能包含用户屏幕截图中的敏感文本。🔧 建议修改
throw VLMProviderError.parsingFailed( - "Failed to parse VLM response JSON: \(error.localizedDescription). Content: \(cleanedContent.prefix(200))..." + "Failed to parse VLM response JSON: \(error.localizedDescription). Content length: \(cleanedContent.count) chars" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Services/OpenAIVLMProvider.swift` around lines 506 - 509, The parsing error currently includes a snippet of the original response via cleanedContent.prefix(200), which may leak sensitive user content; update the throw of VLMProviderError.parsingFailed to remove the cleanedContent fragment and only include non-sensitive details (e.g., error.localizedDescription and a generic note like "response content omitted" or an internal parse error code) so that VLMProviderError.parsingFailed no longer contains any user-provided content.
277-280:⚠️ Potential issue | 🟠 Major抛出的错误信息中包含原始 JSON 片段，存在数据泄露风险。
VLMProviderError.parsingFailed的错误消息包含rawJSON.prefix(300)，这些内容可能含有用户屏幕文本或 API 返回的敏感信息。虽然不是直接记录到日志，但错误可能被上层捕获并记录、显示给用户或发送到监控系统。建议仅在错误消息中保留必要的诊断信息（如响应长度、状态码），移除原始内容。
🔧 建议修改
let rawJSON = String(data: data, encoding: .utf8) ?? "<unable to decode>" - throw VLMProviderError.parsingFailed("Failed to decode OpenAI response: \(error.localizedDescription). Raw: \(rawJSON.prefix(300))") + throw VLMProviderError.parsingFailed("Failed to decode OpenAI response: \(error.localizedDescription). Response length: \(data.count) bytes")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Services/OpenAIVLMProvider.swift` around lines 277 - 280, The thrown parsing error currently embeds a raw JSON snippet (rawJSON.prefix(300)) which risks leaking sensitive text; update the error in the OpenAIVLMProvider code path so VLMProviderError.parsingFailed does NOT include rawJSON or any raw response content, and instead include safe diagnostics such as the HTTP status (if available), the response byte count (data.count) and the parsing error.localizedDescription; remove or stop constructing the rawJSON variable where it is only used for the error message (or keep it locally but do not propagate), and update any references to VLMProviderError.parsingFailed in this function to use the sanitized message.
🧹 Nitpick comments (4)
ScreenTranslateTests/TranslationServicePipelineTests.swift (3)
113-115: 冗余的 nil 合并操作符。
Array.last已经返回Optional，当数组为空时返回nil。?? nil是多余的。♻️ 建议修改
func lastPromptTemplate() async -> String? { - promptTemplates.last ?? nil + promptTemplates.last }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslateTests/TranslationServicePipelineTests.swift` around lines 113 - 115, The function lastPromptTemplate() contains a redundant nil-coalescing operator; since promptTemplates.last already returns an Optional, remove the "?? nil" and return promptTemplates.last directly in lastPromptTemplate() to simplify the implementation (referencing the lastPromptTemplate() function and the promptTemplates.last expression).
164-618: 文件和类体长度超过 SwiftLint 阈值。静态分析提示：
- 类体长度：401 行（阈值 350 行）
- 文件长度：543 行（阈值 400 行）
对于测试文件，这通常可以接受。如果后续添加更多测试，可考虑按功能域拆分为多个测试文件（如
TranslationServiceTests.swift、TextTranslationFlowTests.swift）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslateTests/TranslationServicePipelineTests.swift` around lines 164 - 618, The file and the TranslationServicePipelineTests class are too long for SwiftLint thresholds; split tests by responsibility: move all TranslationService-related tests (e.g., testPrimaryEngineAppliesCustomPromptAndReturnsBundle, testPrimaryWithFallbackUsesFallbackWhenPrimaryFails, testUnavailablePrimaryEngineFallsBackWithoutExecutingPrimaryTranslation, testRegistryCreatesBuiltInProviderWhenRegistrationWasSkipped, testRegistryCreatesLLMProvidersThatArePromptConfigurable, testParallelModeCapturesSuccessAndFailurePerEngine, testQuickSwitchUsesPrimaryEngineWithoutFallback, testSceneBindingHonorsSceneSpecificPrimaryEngine, testAllEnginesFailThrowsMultiEngineError, testEmptyInputReturnsEmptyBundleWithoutQueryingProviders) into a new TranslationServiceTests.swift with the TranslationServiceRegistry/TranslationService-focused class, and move TextTranslationFlow tests (testTextTranslationFlowUpdatesStateAndResultOnSuccess, testTextTranslationFlowMapsServiceFailureToUserFacingErrorState) into TextTranslationFlowTests.swift with a TextTranslationFlowTests class; ensure each new file has appropriate imports and retains helper helpers like makeResult and any Mock* types or move shared helpers to a TestHelpers.swift helper file so both test classes stay under SwiftLint length limits.
612-616: 断言依赖具体错误消息格式，可能导致测试脆弱。Lines 614-615 使用硬编码的错误消息字符串进行断言。如果
TextTranslationError的消息格式发生变化，测试会意外失败。建议改为检查错误类型或关键信息是否存在。♻️ 建议：使用更灵活的断言
- XCTAssertEqual(currentPhase, .failed(.translationFailed("Connection failed: offline"))) - XCTAssertEqual(lastError, .translationFailed("Connection failed: offline")) + if case .failed(let failedError) = currentPhase, + case .translationFailed(let message) = failedError { + XCTAssertTrue(message.contains("offline")) + } else { + XCTFail("Expected failed phase with translationFailed error") + } + if case .translationFailed(let message) = lastError { + XCTAssertTrue(message.contains("offline")) + } else { + XCTFail("Expected translationFailed error") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslateTests/TranslationServicePipelineTests.swift` around lines 612 - 616, The assertions are brittle because they compare full error message strings; update the checks to assert the error case/type and key content instead of exact wording: replace the XCTAssertEqual comparing currentPhase to .failed(.translationFailed("Connection failed: offline")) and lastError equality check with pattern-matching or case assertions against the translationFailed enum case (e.g., verify currentPhase == .failed(.translationFailed) via enum case or switch and assert lastError is a .translationFailed) and then assert the message contains the substring "offline" (or other key token) rather than matching the entire message; keep the final XCTAssertEqual(serviceRequestCount, 1) as-is.ScreenTranslate/Services/Translation/TranslationEngineRegistry.swift (1)
117-141:forceRefresh对内置 provider 和其他 provider 的行为不一致。当
forceRefresh=true时：
- 对于
.apple/.mtranServer：如果已注册则抛出alreadyRegistered错误- 对于 LLM/云服务 provider：会重新创建并注册
这种不一致可能导致 API 使用者困惑。如果内置 provider 设计上不支持刷新，可以考虑在文档中明确说明，或让其行为与其他类型一致（直接返回现有实例而非抛错）。
♻️ 可选：统一 forceRefresh 行为
case .apple: if providers[.apple] != nil { - throw RegistryError.alreadyRegistered + // Built-in providers are singleton-like; return existing instead of recreating + return providers[.apple]! } provider = AppleTranslationProvider() case .mtranServer: if providers[.mtranServer] != nil { - throw RegistryError.alreadyRegistered + return providers[.mtranServer]! } provider = MTranServerEngine.shared🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Services/Translation/TranslationEngineRegistry.swift` around lines 117 - 141, The createProvider function treats forceRefresh inconsistently: for TranslationEngineType.apple and .mtranServer it throws RegistryError.alreadyRegistered when an instance exists, but other providers are recreated; change this to a consistent policy by ignoring forceRefresh for built-in providers and returning the existing instance if providers[.apple] or providers[.mtranServer] is present instead of throwing. Update the logic in createProvider (referencing TranslationEngineType.apple, TranslationEngineType.mtranServer, providers dictionary, RegistryError.alreadyRegistered, AppleTranslationProvider, and MTranServerEngine.shared) so built-in providers are always returned if already registered; keep recreation/registration behavior for LLM/cloud providers unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ScreenTranslate/Services/OpenAIVLMProvider.swift`:
- Around line 506-509: The parsing error currently includes a snippet of the
original response via cleanedContent.prefix(200), which may leak sensitive user
content; update the throw of VLMProviderError.parsingFailed to remove the
cleanedContent fragment and only include non-sensitive details (e.g.,
error.localizedDescription and a generic note like "response content omitted" or
an internal parse error code) so that VLMProviderError.parsingFailed no longer
contains any user-provided content.
- Around line 277-280: The thrown parsing error currently embeds a raw JSON
snippet (rawJSON.prefix(300)) which risks leaking sensitive text; update the
error in the OpenAIVLMProvider code path so VLMProviderError.parsingFailed does
NOT include rawJSON or any raw response content, and instead include safe
diagnostics such as the HTTP status (if available), the response byte count
(data.count) and the parsing error.localizedDescription; remove or stop
constructing the rawJSON variable where it is only used for the error message
(or keep it locally but do not propagate), and update any references to
VLMProviderError.parsingFailed in this function to use the sanitized message.
---
Nitpick comments:
In `@ScreenTranslate/Services/Translation/TranslationEngineRegistry.swift`:
- Around line 117-141: The createProvider function treats forceRefresh
inconsistently: for TranslationEngineType.apple and .mtranServer it throws
RegistryError.alreadyRegistered when an instance exists, but other providers are
recreated; change this to a consistent policy by ignoring forceRefresh for
built-in providers and returning the existing instance if providers[.apple] or
providers[.mtranServer] is present instead of throwing. Update the logic in
createProvider (referencing TranslationEngineType.apple,
TranslationEngineType.mtranServer, providers dictionary,
RegistryError.alreadyRegistered, AppleTranslationProvider, and
MTranServerEngine.shared) so built-in providers are always returned if already
registered; keep recreation/registration behavior for LLM/cloud providers
unchanged.
In `@ScreenTranslateTests/TranslationServicePipelineTests.swift`:
- Around line 113-115: The function lastPromptTemplate() contains a redundant
nil-coalescing operator; since promptTemplates.last already returns an Optional,
remove the "?? nil" and return promptTemplates.last directly in
lastPromptTemplate() to simplify the implementation (referencing the
lastPromptTemplate() function and the promptTemplates.last expression).
- Around line 164-618: The file and the TranslationServicePipelineTests class
are too long for SwiftLint thresholds; split tests by responsibility: move all
TranslationService-related tests (e.g.,
testPrimaryEngineAppliesCustomPromptAndReturnsBundle,
testPrimaryWithFallbackUsesFallbackWhenPrimaryFails,
testUnavailablePrimaryEngineFallsBackWithoutExecutingPrimaryTranslation,
testRegistryCreatesBuiltInProviderWhenRegistrationWasSkipped,
testRegistryCreatesLLMProvidersThatArePromptConfigurable,
testParallelModeCapturesSuccessAndFailurePerEngine,
testQuickSwitchUsesPrimaryEngineWithoutFallback,
testSceneBindingHonorsSceneSpecificPrimaryEngine,
testAllEnginesFailThrowsMultiEngineError,
testEmptyInputReturnsEmptyBundleWithoutQueryingProviders) into a new
TranslationServiceTests.swift with the
TranslationServiceRegistry/TranslationService-focused class, and move
TextTranslationFlow tests
(testTextTranslationFlowUpdatesStateAndResultOnSuccess,
testTextTranslationFlowMapsServiceFailureToUserFacingErrorState) into
TextTranslationFlowTests.swift with a TextTranslationFlowTests class; ensure
each new file has appropriate imports and retains helper helpers like makeResult
and any Mock* types or move shared helpers to a TestHelpers.swift helper file so
both test classes stay under SwiftLint length limits.
- Around line 612-616: The assertions are brittle because they compare full
error message strings; update the checks to assert the error case/type and key
content instead of exact wording: replace the XCTAssertEqual comparing
currentPhase to .failed(.translationFailed("Connection failed: offline")) and
lastError equality check with pattern-matching or case assertions against the
translationFailed enum case (e.g., verify currentPhase ==
.failed(.translationFailed) via enum case or switch and assert lastError is a
.translationFailed) and then assert the message contains the substring "offline"
(or other key token) rather than matching the entire message; keep the final
XCTAssertEqual(serviceRequestCount, 1) as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5dca5fcf-3bbc-4051-86bd-32c2999a3b17
📒 Files selected for processing (7)
ScreenTranslate/Features/Settings/EngineConfigSheet.swiftScreenTranslate/Services/ClaudeVLMProvider.swiftScreenTranslate/Services/OpenAIVLMProvider.swiftScreenTranslate/Services/TextSelectionService.swiftScreenTranslate/Services/Translation/Providers/LLMTranslationProvider.swiftScreenTranslate/Services/Translation/TranslationEngineRegistry.swiftScreenTranslateTests/TranslationServicePipelineTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- ScreenTranslate/Services/Translation/Providers/LLMTranslationProvider.swift
- ScreenTranslate/Services/ClaudeVLMProvider.swift
There was a problem hiding this comment.
💡 Codex Review
https://github.com/hubo1989/ScreenTranslate/blob/22b3f82a9cd52c48677f708e7e785ba2ff5fff10/ScreenTranslate/Services/TranslationService.swift#L340-L343
This always resolves promptPreview with compatibleIndex: nil, so .custom translations can never match the per-instance prompts that PromptSettingsView saves in compatibleEnginePrompts[index]. When a user configures a prompt for a specific OpenAI-compatible endpoint, the translation service will still fall back to the default/scene prompt instead of that saved prompt.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let bilingualSegments = try await translationService.translate( | ||
| segments: [trimmedText], | ||
| to: effectiveTargetLanguage, | ||
| preferredEngine: effectiveEngine, |
There was a problem hiding this comment.
The new pipeline tests assert that .translateAndInsert uses the dedicated insert prompt, but this path still calls the legacy TranslationServicing.translate(segments:to:preferredEngine:from:) overload that has no scene parameter. TextTranslationCoordinator uses TextTranslationFlow.shared.translate(...) for the real Translate & Insert feature, so those requests continue reaching TranslationService.applyPromptConfig with scene == nil and fall back to the screenshot/default prompt instead of the translate-and-insert prompt or scene binding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7355f8b. TextTranslationFlow now carries scene in TextTranslationConfig, TextTranslationCoordinator paths resolve to .textSelection / .translateAndInsert, and TranslationServicing forwards the scene through to TranslationService so prompt selection and scene binding use the real flow context. Verified with `xcodebuild test -project ScreenTranslate.xcodeproj -scheme ScreenTranslate -destination 'platform=macOS'
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7355f8b6fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return TextTranslationConfig( | ||
| targetLanguage: targetLanguage, | ||
| sourceLanguage: sourceLanguage, | ||
| preferredEngine: preferredEngine | ||
| preferredEngine: preferredEngine, | ||
| scene: .translateAndInsert |
There was a problem hiding this comment.
When users invoke Translate & Insert through TextTranslationCoordinator, this config still resolves preferredEngine via PreferredTranslationEngine, so the only possible engines are .apple and .mtranServer; neither AppleTranslationProvider nor MTranServerEngine conforms to TranslationPromptConfigurable. That means the new .translateAndInsert scene flag added here never reaches a provider that can consume the dedicated insert prompt, so real Translate & Insert requests continue using the providers' default behavior while the new test passes only because it injects a mock configurable provider.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b1813db. Text translation configs no longer derive their engine solely from PreferredTranslationEngine: they now resolve the primary engine from the active multi-engine settings and scene bindings, so translate-and-insert can route through .custom / LLM engines that consume the insert prompt when the user has configured them. Verified with `xcodebuild build-for-testing -project ScreenTranslate.xcodeproj -scheme ScreenTranslate -destination 'platform=macOS'
| let resolvedInstanceIndex = await MainActor.run { | ||
| AppSettings.shared.compatibleProviderConfigs.firstIndex(where: { $0.id == resolvedCompatibleConfig.id }) ?? 0 | ||
| } |
There was a problem hiding this comment.
This new firstIndex-based instanceIndex lookup makes compatible prompts follow array positions rather than the provider's stable UUID. PromptSettingsView persists custom prompts in compatibleEnginePrompts[index], and MultiEngineSettingsSection.deleteCompatibleEngine removes entries without rekeying that dictionary, so deleting or reordering compatible endpoints causes the surviving endpoint to inherit another endpoint's prompt. After removing index 0, for example, the former index 1 endpoint will now receive index 0's prompt and its own prompt becomes unreachable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b1813db. Compatible prompts are now keyed by stable provider UUIDs end-to-end, the settings UI edits prompts by provider ID, and delete/migration paths no longer shift credentials or prompts by position, so reorder/delete operations do not reassign another endpoint\s prompt. Verified with `xcodebuild build-for-testing -project ScreenTranslate.xcodeproj -scheme ScreenTranslate -destination 'platform=macOS'
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ScreenTranslateTests/TranslationServicePipelineTests.swift (1)
174-175: 建议按职责拆分这个测试文件。SwiftLint 已经报了
file_length和type_body_length。现在一个文件同时放 mock、service pipeline 用例和TextTranslationFlow用例，后续再加回归测试会越来越难维护。把 mock 挪到TestSupport，再把 pipeline / flow 测试拆成独立XCTestCase会更稳。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslateTests/TranslationServicePipelineTests.swift` around lines 174 - 175, 当前文件 TranslationServicePipelineTests 包含太多职责（mocks、pipeline 测试、TextTranslationFlow 测试），触发 SwiftLint 的 file_length 和 type_body_length 规则；请把所有 mock 类型移动到独立的 TestSupport 测试辅助模块/文件，并将 pipeline 相关测试从 TranslationServicePipelineTests 中拆分为一个或多个专门的 XCTestCase（例如 TranslationPipelineTests），把 TextTranslationFlow 的用例移到单独的 XCTestCase（例如 TextTranslationFlowTests），确保每个 XCTestCase 只包含一类职责并更新 import/visibility，如果有共享 setup/teardown 提取到基类或 TestSupport 中以避免重复。
🤖 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/Translation/Providers/CompatibleTranslationProvider.swift`:
- Around line 90-94: The code incorrectly maps "not found" to index 0 by using
"?? 0" when resolving the compatible provider instance; change the lookup to
return an optional Int (remove the "?? 0") so resolvedInstanceIndex is nil when
the id isn't found, and assign that nil into instanceIndex (or update
instanceIndex to be Int? if needed) so the provider falls back to the generic
prompt; apply the same change to the other occurrence (the block referenced at
lines 225-227) and ensure callers like compatiblePromptIndex() continue to
accept/use nil as the "no matching instance" signal.
In `@ScreenTranslate/Services/TranslationProvider.swift`:
- Around line 53-56: 当前把 prompt 模板当成 provider
的共享可变状态（TranslationPromptConfigurable /
setCustomPromptTemplate(_:)）会导致并发请求互相覆盖并让
CompatibleTranslationProvider.buildPrompt(...) 读取到错误的模板；把 prompt/context 从
provider 状态剥离，改为将模板作为 translate(...) 的请求参数传递，或者在 provider 上新增一个原子化接口（如
configureAndTranslate(prompt:..., request:...)）来同时传入模板并执行翻译，更新
TranslationService 调用点以传递模板而不是先调用
setCustomPromptTemplate(_:)，并移除/弃用对共享可变模板状态的依赖以避免竞态。
---
Nitpick comments:
In `@ScreenTranslateTests/TranslationServicePipelineTests.swift`:
- Around line 174-175: 当前文件 TranslationServicePipelineTests
包含太多职责（mocks、pipeline 测试、TextTranslationFlow 测试），触发 SwiftLint 的 file_length 和
type_body_length 规则；请把所有 mock 类型移动到独立的 TestSupport 测试辅助模块/文件，并将 pipeline 相关测试从
TranslationServicePipelineTests 中拆分为一个或多个专门的 XCTestCase（例如
TranslationPipelineTests），把 TextTranslationFlow 的用例移到单独的 XCTestCase（例如
TextTranslationFlowTests），确保每个 XCTestCase 只包含一类职责并更新 import/visibility，如果有共享
setup/teardown 提取到基类或 TestSupport 中以避免重复。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f3cc851-f422-4dba-8781-48747192cfbe
📒 Files selected for processing (7)
ScreenTranslate/Features/TextTranslation/TextTranslationFlow.swiftScreenTranslate/Services/OpenAIVLMProvider.swiftScreenTranslate/Services/Protocols/TranslationServicing.swiftScreenTranslate/Services/Translation/Providers/CompatibleTranslationProvider.swiftScreenTranslate/Services/TranslationProvider.swiftScreenTranslate/Services/TranslationService.swiftScreenTranslateTests/TranslationServicePipelineTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- ScreenTranslate/Features/TextTranslation/TextTranslationFlow.swift
- ScreenTranslate/Services/OpenAIVLMProvider.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1813dbe53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let config = try? JSONDecoder().decode(TranslationPromptConfig.self, from: data) { | ||
| return config | ||
| } |
There was a problem hiding this comment.
Because JSON dictionaries always serialize object keys as strings, a pre-upgrade compatibleEnginePrompts: [Int: String] payload will still decode successfully into the new [String: String] field here, so the LegacyTranslationPromptConfig branch never runs. On upgraded installs, the saved keys remain "0", "1", etc., while the new lookups in PromptSettingsView.swift:112-119 and TranslationService.swift:355-358 expect provider UUID strings, which makes every previously saved compatible-engine prompt disappear until the user re-enters it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8eec53d. AppSettings.loadPromptConfig now migrates numeric-string legacy keys like "0" / "1" to the current compatible provider UUIDs even when the payload decodes into the new [String: String] shape, so saved compatible prompts survive upgrade. Verified with `xcodebuild test -project ScreenTranslate.xcodeproj -scheme ScreenTranslate -destination 'platform=macOS'
| switch settings.engineSelectionMode { | ||
| case .sceneBinding: | ||
| return settings.sceneBindings[scene]?.primaryEngine ?? SceneEngineBinding.default(for: scene).primaryEngine | ||
| case .parallel, .quickSwitch, .primaryWithFallback: | ||
| if let firstConfiguredEngine = settings.parallelEngines.first { | ||
| return firstConfiguredEngine | ||
| } |
There was a problem hiding this comment.
This helper now branches on engineSelectionMode, but it can only return a single preferredEngine. TextTranslationFlow.translate still calls TranslationServicing.translate(...) with just that engine (Features/TextTranslation/TextTranslationFlow.swift:181-186), and TranslationService implements that overload by hard-coding mode: .primaryWithFallback (Services/TranslationService.swift:398-404). As a result, text selection and translate-and-insert ignore .parallel, .quickSwitch, and scene-binding fallback settings, so users who pick those modes still get a single primary-with-default-fallback translation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8eec53d. TextTranslationFlow now carries engineSelectionMode, fallback, parallel engines, and scene bindings through TranslationServicing into TranslationService, so text selection and translate-and-insert honor .parallel, .quickSwitch, and scene-specific routing instead of collapsing to .primaryWithFallback. Verified with `xcodebuild test -project ScreenTranslate.xcodeproj -scheme ScreenTranslate -destination 'platform=macOS'
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ScreenTranslate/Models/AppSettings.swift (1)
764-798: 旧版提示配置迁移逻辑基本正确，但有小的代码风格问题。迁移逻辑合理：
- 先尝试解码新格式
TranslationPromptConfig- 失败则尝试解码旧版结构并迁移
- 通过索引查找对应的
compatibleConfig并使用其 UUID注意：当旧索引在当前
compatibleConfigs中不存在时（line 789），该提示会被静默丢弃。这是合理的容错行为。关于 SwiftLint 报告的
closure_parameter_position警告（lines 786-787），建议将闭包参数移到与左花括号同一行：♻️ 可选的格式修复
- let migratedCompatiblePrompts = legacyConfig.compatibleEnginePrompts.reduce(into: [String: String]()) { - result, - entry in + let migratedCompatiblePrompts = legacyConfig.compatibleEnginePrompts.reduce(into: [String: String]()) { result, entry in let (index, prompt) = entry🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Models/AppSettings.swift` around lines 764 - 798, The SwiftLint closure_parameter_position warning in loadPromptConfig is caused by the multiline reduce closure for migratedCompatiblePrompts; fix it by moving the closure parameters to the same line as the opening brace and optionally simplify the initial dictionary literal. Specifically, update the reduce call on migratedCompatiblePrompts (the closure starting with "result, entry in") so it reads like reduce(into: [String: String]()) { result, entry in ... (or reduce(into: [:]) { result, entry in ...) ensuring the "{ result, entry in" is on one line with the opening brace.ScreenTranslateTests/TranslationServicePipelineTests.swift (1)
382-393: 这个 registry 用例有点绑死实现细节了。这里真正要验证的是“跳过预注册时仍能创建并缓存
.appleprovider”；provider is AppleTranslationProvider` 会把后续的 wrapper/decorator 重构也一起锁死。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslateTests/TranslationServicePipelineTests.swift` around lines 382 - 393, 测试不应该锁死实现细节（具体类型 AppleTranslationProvider）；在 testRegistryCreatesBuiltInProviderWhenRegistrationWasSkipped 中，保留对 TranslationEngineRegistry(registerBuiltInProviders: false)、registry.createProvider(for: .apple, config: .default(for: .apple)) 和 registry.provider(for: .apple) 的调用，但删除对 provider is AppleTranslationProvider 的断言；改为断言 provider 和从 registry.provider(for:) 取得的 registeredProvider 非空且为相同实例（即创建后被缓存），这样后续对 provider 的 wrapper/decorator 重构不会破坏测试。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ScreenTranslateTests/TranslationConfigurationTests.swift`:
- Around line 163-178: The test
testTranslationProviderErrorExposesRecoveryGuidance currently only asserts that
TranslationProviderError.errorDescription and .recoverySuggestion are non-nil,
which allows empty strings; update the assertions to ensure the strings are
non-empty (e.g. assert that error.errorDescription and error.recoverySuggestion
are not empty via their .isEmpty checks) so empty "" will fail—refer to the
testTranslationProviderErrorExposesRecoveryGuidance loop and the
TranslationProviderError.errorDescription / .recoverySuggestion properties to
locate and change the XCTAssertNotNil checks to non-empty string assertions.
In `@ScreenTranslateTests/TranslationServicePipelineTests.swift`:
- Around line 50-63: The test currently masks a provider/aggregation contract
violation by turning a non-empty input + empty translate() results into a
successful TranslationResult (sourceText == translatedText); instead detect this
as an error and fail/propagate instead of synthesizing success: in the translate
call handling (the results variable and the guard that checks results.first)
replace the fallback return with a thrown error or test failure (e.g., throw a
TranslationError.emptyResponse or call XCTFail) so empty-array responses for
non-empty input surface as test failures rather than silent successful
translations.
- Around line 679-683: lastError is an optional (TextTranslationError?) but the
pattern match uses .translationFailed without unwrapping; change the pattern to
include the optional operator so the match unpacks the optional (e.g., use the
`?` in the case pattern for `.translationFailed`), i.e. update the `if case
.translationFailed(let message) = lastError` line to the optional-pattern form
so `message` is extracted when lastError contains .translationFailed.
---
Nitpick comments:
In `@ScreenTranslate/Models/AppSettings.swift`:
- Around line 764-798: The SwiftLint closure_parameter_position warning in
loadPromptConfig is caused by the multiline reduce closure for
migratedCompatiblePrompts; fix it by moving the closure parameters to the same
line as the opening brace and optionally simplify the initial dictionary
literal. Specifically, update the reduce call on migratedCompatiblePrompts (the
closure starting with "result, entry in") so it reads like reduce(into: [String:
String]()) { result, entry in ... (or reduce(into: [:]) { result, entry in ...)
ensuring the "{ result, entry in" is on one line with the opening brace.
In `@ScreenTranslateTests/TranslationServicePipelineTests.swift`:
- Around line 382-393: 测试不应该锁死实现细节（具体类型 AppleTranslationProvider）；在
testRegistryCreatesBuiltInProviderWhenRegistrationWasSkipped 中，保留对
TranslationEngineRegistry(registerBuiltInProviders:
false)、registry.createProvider(for: .apple, config: .default(for: .apple)) 和
registry.provider(for: .apple) 的调用，但删除对 provider is AppleTranslationProvider
的断言；改为断言 provider 和从 registry.provider(for:) 取得的 registeredProvider
非空且为相同实例（即创建后被缓存），这样后续对 provider 的 wrapper/decorator 重构不会破坏测试。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54ac0ae9-0df0-4547-8fa3-421ce5c1f821
📒 Files selected for processing (14)
ScreenTranslate/App/Coordinators/TextTranslationCoordinator.swiftScreenTranslate/Features/Settings/CompatibleEngineConfigSheet.swiftScreenTranslate/Features/Settings/MultiEngineSettingsSection.swiftScreenTranslate/Features/Settings/PromptSettingsView.swiftScreenTranslate/Features/TextTranslation/TextTranslationFlow.swiftScreenTranslate/Models/AppSettings.swiftScreenTranslate/Models/TranslationPromptConfig.swiftScreenTranslate/Services/Translation/Providers/CompatibleTranslationProvider.swiftScreenTranslate/Services/Translation/Providers/LLMTranslationProvider.swiftScreenTranslate/Services/Translation/TranslationEngineRegistry.swiftScreenTranslate/Services/TranslationProvider.swiftScreenTranslate/Services/TranslationService.swiftScreenTranslateTests/TranslationConfigurationTests.swiftScreenTranslateTests/TranslationServicePipelineTests.swift
💤 Files with no reviewable changes (1)
- ScreenTranslate/Features/Settings/CompatibleEngineConfigSheet.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- ScreenTranslate/Services/Translation/TranslationEngineRegistry.swift
- ScreenTranslate/Services/TranslationProvider.swift
- ScreenTranslate/Services/TranslationService.swift
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 (1)
ScreenTranslate/Models/AppSettings.swift (1)
833-848:⚠️ Potential issue | 🟠 Major已保存的 scene binding 无法恢复。
第 844 行将
reduce(into:)的返回值丢弃，result始终保持为allDefaults，用户持久化的 scene binding 在启动时无法恢复。在 Swift 中，reduce(into:)不会修改传入的原始变量，它只返回修改后的累积结果。修复方案
private static func loadSceneBindings() -> [TranslationScene: SceneEngineBinding] { - // Start with defaults - let result = SceneEngineBinding.allDefaults - - // Load saved bindings and merge guard let data = UserDefaults.standard.data(forKey: Keys.sceneBindings), let bindings = try? JSONDecoder().decode([SceneEngineBinding].self, from: data) else { - return result + return SceneEngineBinding.allDefaults } - // Merge loaded bindings over defaults (using reduce to handle duplicates safely) - _ = bindings.reduce(into: result) { dict, binding in + return bindings.reduce(into: SceneEngineBinding.allDefaults) { dict, binding in dict[binding.scene] = binding } - - return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Models/AppSettings.swift` around lines 833 - 848, The saved scene bindings never get merged because the code creates let result = SceneEngineBinding.allDefaults and then discards the return value of bindings.reduce(into: result) (using _ = ...), so persisted bindings are not applied; fix by either making result mutable and assigning the merge back (e.g. change let result to var result and set result = bindings.reduce(into: result) { dict, binding in dict[binding.scene] = binding }) or capture the reducer result into a new variable and return that; update loadSceneBindings to use the returned merged dictionary (Keys.sceneBindings, SceneEngineBinding.allDefaults, bindings.reduce(into:)) so persisted bindings are restored.
🧹 Nitpick comments (1)
ScreenTranslate/Services/Protocols/TranslationServicing.swift (1)
24-34: 建议把翻译路由参数收敛成一个请求对象。这个协议入口现在需要同时传 9 个参数，
TranslationService、TextTranslationFlow和测试桩都得镜像同一组字段。后续再加一个路由选项就会形成多处破坏性改动，而且调用方也更容易把标签/参数传错；把scene、mode、fallbackEnabled、parallelEngines、sceneBindings收进一个Sendablerequest/options 类型会更稳。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Services/Protocols/TranslationServicing.swift` around lines 24 - 34, The translate(...) protocol entry on TranslationServicing is taking too many parameters; create a Sendable request type (e.g., TranslationRequest or TranslationOptions) that bundles scene: TranslationScene?, mode: EngineSelectionMode, fallbackEnabled: Bool, parallelEngines: [TranslationEngineType], sceneBindings: [TranslationScene: SceneEngineBinding] plus the existing segments, targetLanguage, preferredEngine and optional sourceLanguage, then change the protocol method signature to func translate(request: TranslationRequest) async throws -> [BilingualSegment]; update all implementers (TranslationService, TextTranslationFlow) and test/stub types to accept and use the new TranslationRequest, ensure the request type conforms to Sendable and adjust all call sites to construct and pass the request object instead of nine separate parameters.
🤖 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/Models/AppSettings.swift`:
- Around line 797-803: The migration currently silently drops prompts when
legacyConfig.compatibleEnginePrompts contains an index not present in
compatibleConfigs (inside migratedCompatiblePrompts), which can lose user data;
change the reduce so that if compatibleConfigs.indices.contains(index) is false
you still insert the prompt using the original index string (e.g. use "\(index)"
as the key) instead of returning early, so unknown keys are preserved during
migration; update the block that builds migratedCompatiblePrompts (referencing
legacyConfig.compatibleEnginePrompts, compatibleConfigs and
migratedCompatiblePrompts) to fall back to the index-string key when the mapped
config is missing.
- Around line 780-784: loadPromptConfig() returns a migrated
TranslationPromptConfig which is assigned to promptConfig in init, but that
init-time assignment does not trigger the promptConfig didSet so
savePromptConfig() is never called; update the logic so the migrated config is
persisted immediately—either have loadPromptConfig() indicate migration (e.g.,
return a tuple or throw a MigrationPerformed flag) or detect a migration result
in init after assigning promptConfig and call savePromptConfig() explicitly;
reference the symbols loadPromptConfig(), init, promptConfig,
savePromptConfig(), TranslationPromptConfig and ensure compatibleProviderConfigs
migration is persisted right after creating the migrated TranslationPromptConfig
to avoid future remapping issues.
In `@ScreenTranslate/Services/TranslationService.swift`:
- Around line 352-359: The code incorrectly forces nil scene to .screenshot by
using sceneToUse; instead pass the actual optional scene through so scene-less
requests don't match scene-specific prompts: replace the use of sceneToUse with
the original optional scene when calling
promptConfig.promptPreview(for:scene:compatiblePromptID:) (i.e. pass scene:
scene), ensure TranslationPromptContextProviding.compatiblePromptIdentifier()
usage remains the same, and add a unit test verifying that when scene is nil the
prompt resolution does not consult scenePrompts[.screenshot] (or any
scene-specific prompt).
---
Outside diff comments:
In `@ScreenTranslate/Models/AppSettings.swift`:
- Around line 833-848: The saved scene bindings never get merged because the
code creates let result = SceneEngineBinding.allDefaults and then discards the
return value of bindings.reduce(into: result) (using _ = ...), so persisted
bindings are not applied; fix by either making result mutable and assigning the
merge back (e.g. change let result to var result and set result =
bindings.reduce(into: result) { dict, binding in dict[binding.scene] = binding
}) or capture the reducer result into a new variable and return that; update
loadSceneBindings to use the returned merged dictionary (Keys.sceneBindings,
SceneEngineBinding.allDefaults, bindings.reduce(into:)) so persisted bindings
are restored.
---
Nitpick comments:
In `@ScreenTranslate/Services/Protocols/TranslationServicing.swift`:
- Around line 24-34: The translate(...) protocol entry on TranslationServicing
is taking too many parameters; create a Sendable request type (e.g.,
TranslationRequest or TranslationOptions) that bundles scene: TranslationScene?,
mode: EngineSelectionMode, fallbackEnabled: Bool, parallelEngines:
[TranslationEngineType], sceneBindings: [TranslationScene: SceneEngineBinding]
plus the existing segments, targetLanguage, preferredEngine and optional
sourceLanguage, then change the protocol method signature to func
translate(request: TranslationRequest) async throws -> [BilingualSegment];
update all implementers (TranslationService, TextTranslationFlow) and test/stub
types to accept and use the new TranslationRequest, ensure the request type
conforms to Sendable and adjust all call sites to construct and pass the request
object instead of nine separate parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8eb79bf9-9590-4c6f-a719-8c2b9a1740be
📒 Files selected for processing (6)
ScreenTranslate/Features/TextTranslation/TextTranslationFlow.swiftScreenTranslate/Models/AppSettings.swiftScreenTranslate/Services/Protocols/TranslationServicing.swiftScreenTranslate/Services/TranslationService.swiftScreenTranslateTests/TranslationConfigurationTests.swiftScreenTranslateTests/TranslationServicePipelineTests.swift
✅ Files skipped from review due to trivial changes (1)
- ScreenTranslate/Features/TextTranslation/TextTranslationFlow.swift
| return TranslationPromptConfig( | ||
| enginePrompts: config.enginePrompts, | ||
| compatibleEnginePrompts: migratedCompatiblePrompts, | ||
| scenePrompts: config.scenePrompts | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
在 Swift 中，init里的属性赋值会触发didSet/willSet 吗？
💡 Result:
不会。在 Swift 中，init 初始化器中对属性的赋值不会触发 willSet 或 didSet 属性观察器。这是 Swift 语言的设计特性，目的是确保在初始化过程中对象状态完整，避免观察器访问未初始化的其他属性导致崩溃或未定义行为。 官方文档（引自多个来源）和社区共识确认：属性观察器仅在初始化上下文之外设置属性值时调用。对于根类（非子类）自身的属性，在 init 中直接赋值绕过观察器；子类中对超类属性在 super.init 后赋值会触发，但对自身属性仍不触发。 工作围绕： - 使用 defer { property = value } 在初始化结束时赋值，退出初始化上下文后触发观察器。 - 将逻辑提取到独立方法，在 init 后手动调用。 - 对于隐式解包可选类型（!），初始 nil 赋值后后续赋值可能触发，但标准非可选属性不触发。
Citations:
- 1: https://stackoverflow.com/questions/29619846/in-swift-didset-doesn-t-fire-when-invoked-from-init
- 2: https://stackoverflow.com/questions/25230780/is-it-possible-to-allow-didset-to-be-called-during-initialization-in-swift
- 3: https://nilcoalescing.com/blog/TriggerPropertyObserversFromInitializersInSwift
- 4: https://stackoverflow.com/questions/48915486/swift-property-observers-initial-value
- 5: https://docs.swift.org/swift-book/LanguageGuide/TheBasics.html
🏁 Script executed:
#!/bin/bash
# Check the actual code around line 538 and the init method
cd ScreenTranslate && sed -n '535,545p' Models/AppSettings.swift
echo "---"
# Find the didSet observer for promptConfig
rg -n "promptConfig.*didSet" Models/AppSettings.swift
echo "---"
# Find savePromptConfig method
rg -n "func savePromptConfig" Models/AppSettings.swift
echo "---"
# Show context around the savePromptConfig method
rg -n -A 3 "func savePromptConfig" Models/AppSettings.swiftRepository: hubo1989/ScreenTranslate
Length of output: 976
🏁 Script executed:
#!/bin/bash
# Find the promptConfig property definition with its didSet observer
rg -n -B 2 -A 5 "var promptConfig" Models/AppSettings.swift | head -40Repository: hubo1989/ScreenTranslate
Length of output: 132
🏁 Script executed:
#!/bin/bash
# First, find the AppSettings.swift file
find . -name "AppSettings.swift" -type fRepository: hubo1989/ScreenTranslate
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Find the promptConfig property definition with its didSet observer
rg -n -B 2 -A 5 "var promptConfig" ./ScreenTranslate/Models/AppSettings.swift | head -50Repository: hubo1989/ScreenTranslate
Length of output: 319
🏁 Script executed:
#!/bin/bash
# View the loadPromptConfig method around lines 780-784 and 805-809
sed -n '765,820p' ./ScreenTranslate/Models/AppSettings.swiftRepository: hubo1989/ScreenTranslate
Length of output: 2426
迁移后的 promptConfig 需要立即持久化。
loadPromptConfig 返回的迁移后配置（行 780-784 和 805-809）在 init 中赋值给 promptConfig（行 538），但 init 内的属性赋值不会触发 didSet 观察器，所以 savePromptConfig() 不会被调用。这导致 UserDefaults 中仍保留旧的 JSON 格式。若用户调整 compatibleProviderConfigs 的顺序或数量，下次启动重新迁移时会基于新的索引顺序把 prompt 绑定到错误的 provider，造成数据丢失或配置错误。
建议修复
+ private static func persistPromptConfig(_ config: TranslationPromptConfig) {
+ if let data = try? JSONEncoder().encode(config) {
+ UserDefaults.standard.set(data, forKey: Keys.promptConfig)
+ }
+ }
+
private static func loadPromptConfig(
compatibleConfigs: [CompatibleTranslationProvider.CompatibleConfig]
) -> TranslationPromptConfig {
guard let data = UserDefaults.standard.data(forKey: Keys.promptConfig) else {
return TranslationPromptConfig()
@@
- return TranslationPromptConfig(
+ let migrated = TranslationPromptConfig(
enginePrompts: config.enginePrompts,
compatibleEnginePrompts: migratedCompatiblePrompts,
scenePrompts: config.scenePrompts
)
+ persistPromptConfig(migrated)
+ return migrated
}
@@
- return TranslationPromptConfig(
+ let migrated = TranslationPromptConfig(
enginePrompts: legacyConfig.enginePrompts,
compatibleEnginePrompts: migratedCompatiblePrompts,
scenePrompts: legacyConfig.scenePrompts
)
+ persistPromptConfig(migrated)
+ return migrated
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ScreenTranslate/Models/AppSettings.swift` around lines 780 - 784,
loadPromptConfig() returns a migrated TranslationPromptConfig which is assigned
to promptConfig in init, but that init-time assignment does not trigger the
promptConfig didSet so savePromptConfig() is never called; update the logic so
the migrated config is persisted immediately—either have loadPromptConfig()
indicate migration (e.g., return a tuple or throw a MigrationPerformed flag) or
detect a migration result in init after assigning promptConfig and call
savePromptConfig() explicitly; reference the symbols loadPromptConfig(), init,
promptConfig, savePromptConfig(), TranslationPromptConfig and ensure
compatibleProviderConfigs migration is persisted right after creating the
migrated TranslationPromptConfig to avoid future remapping issues.
| let migratedCompatiblePrompts = legacyConfig.compatibleEnginePrompts.reduce(into: [String: String]()) { | ||
| result, | ||
| entry in | ||
| let (index, prompt) = entry | ||
| guard compatibleConfigs.indices.contains(index) else { return } | ||
| result[compatibleConfigs[index].id.uuidString] = prompt | ||
| } |
There was a problem hiding this comment.
遗留迁移分支不要静默丢弃未解析的 prompt。
Line 801 解析失败就直接跳过，会在配置列表缺项或解码失败时把用户自定义 prompt 清空。上面的现行格式分支已经保留未知 key 了，这里至少也应把 index 原样转成字符串保存，避免迁移时的数据丢失。
可选修复
- let migratedCompatiblePrompts = legacyConfig.compatibleEnginePrompts.reduce(into: [String: String]()) {
- result,
- entry in
+ let migratedCompatiblePrompts = legacyConfig.compatibleEnginePrompts.reduce(into: [String: String]()) { result, entry in
let (index, prompt) = entry
- guard compatibleConfigs.indices.contains(index) else { return }
- result[compatibleConfigs[index].id.uuidString] = prompt
+ if compatibleConfigs.indices.contains(index) {
+ result[compatibleConfigs[index].id.uuidString] = prompt
+ } else {
+ result[String(index)] = prompt
+ }
}🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 798-798: Closure parameters should be on the same line as opening brace
(closure_parameter_position)
[Warning] 799-799: Closure parameters should be on the same line as opening brace
(closure_parameter_position)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ScreenTranslate/Models/AppSettings.swift` around lines 797 - 803, The
migration currently silently drops prompts when
legacyConfig.compatibleEnginePrompts contains an index not present in
compatibleConfigs (inside migratedCompatiblePrompts), which can lose user data;
change the reduce so that if compatibleConfigs.indices.contains(index) is false
you still insert the prompt using the original index string (e.g. use "\(index)"
as the key) instead of returning early, so unknown keys are preserved during
migration; update the block that builds migratedCompatiblePrompts (referencing
legacyConfig.compatibleEnginePrompts, compatibleConfigs and
migratedCompatiblePrompts) to fall back to the index-string key when the mapped
config is missing.
| let sceneToUse = scene ?? .screenshot | ||
| let compatiblePromptID = await (provider as? TranslationPromptContextProviding)?.compatiblePromptIdentifier() | ||
|
|
||
| let customPrompt = promptConfig.promptPreview( | ||
| let resolvedPrompt = promptConfig.promptPreview( | ||
| for: engine, | ||
| scene: sceneToUse, | ||
| compatibleIndex: nil | ||
| compatiblePromptID: compatiblePromptID | ||
| ) |
There was a problem hiding this comment.
scene == nil 时不要默认套用 .screenshot 提示词。
这里把无场景请求强制按截图场景解析 prompt，而下面这个 legacy 入口又把 scene 默认成 nil。这样现有调用方即使完全不感知 scene，也会静默命中 scenePrompts[.screenshot]；一旦用户配置了截图场景 prompt，非截图流程的翻译结果也会被错误污染。无场景时应只解析 engine/compatible prompt，或者要求调用方显式提供默认场景；建议顺手补一个“不传 scene 不命中 scene prompt”的回归测试。
Also applies to: 393-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ScreenTranslate/Services/TranslationService.swift` around lines 352 - 359,
The code incorrectly forces nil scene to .screenshot by using sceneToUse;
instead pass the actual optional scene through so scene-less requests don't
match scene-specific prompts: replace the use of sceneToUse with the original
optional scene when calling
promptConfig.promptPreview(for:scene:compatiblePromptID:) (i.e. pass scene:
scene), ensure TranslationPromptContextProviding.compatiblePromptIdentifier()
usage remains the same, and add a unit test verifying that when scene is nil the
prompt resolution does not consult scenePrompts[.screenshot] (or any
scene-specific prompt).
1 participant
Summary
Testing
Summary by CodeRabbit
发布说明
新功能
改进与优化
测试