Conversation
added 4 commits
March 26, 2026 13:36
Reduce onboarding from 4 steps to 3 by removing the engine configuration step (PaddleOCR/MTranServer setup moved to Settings). Add permission purpose descriptions, skip-for-now option with timeout hint, and menubar location guidance on completion page.
Move lastCounter and timer references to class properties to avoid sending non-Sendable values across isolation boundaries.
- Fix button calling both requestAction and openSettingsAction (caused wrong permission dialog to appear) - Increase window height from 520 to 580 to fit all content - Defer Keychain access on launch to avoid prompt during onboarding - Add note about removing and re-adding app for permission changes
…aults - Skip Sparkle updater initialization during first launch to avoid overlapping with onboarding window; initialize after onboarding completes - Add GLM OCR provider support after rebasing onto latest main
|
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 (2)
📝 WalkthroughWalkthrough将引导流程从4步简化为3步（移除配置），新增“跳过权限”路径并记录；延迟 Sparkle 更新器初始化直到引导完成或被取消；移除 PaddleOCR/翻译测试相关功能并更新本地化与 UI 布局/文案。 Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as AppDelegate
participant Onboarding as OnboardingViewModel
participant Window as OnboardingWindowController
participant NC as NotificationCenter
User->>App: 启动应用
App->>Onboarding: 显示 Onboarding（onboardingCompleted=false）
App--xOnboarding: 不初始化 updaterController
App->>NC: 注册 .onboardingCompleted / .onboardingDismissed 观察者
User->>Window: 交互权限页面
alt 用户跳过权限
User->>Onboarding: 调用 onSkip() / skipPermissions()
Onboarding->>Onboarding: hasSkippedPermissions = true
Onboarding->>NC: 发布 .onboardingCompleted
else 用户完成或授予权限
User->>Onboarding: 完成授权 -> completeOnboarding()
Onboarding->>NC: 发布 .onboardingCompleted
end
NC->>App: 收到 .onboardingCompleted / .onboardingDismissed
App->>App: initializeUpdaterIfNeeded()
App->>App: 访问 updaterController -> Sparkle 初始化
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ScreenTranslate/Features/Preview/PreviewWindow.swift (1)
103-113:⚠️ Potential issue | 🟠 Major重复定时器未失效会导致后台持续轮询。
Line 103 创建了重复
Timer，但当前没有对应invalidate()。这会在窗口关闭后继续触发回调（或在重复调用观察逻辑时并存多个定时器），带来不必要的 CPU 唤醒和资源泄漏风险。建议修复
`@MainActor` private func observeImageSizeChanges() { + imageSizeTimer?.invalidate() + imageSizeTimer = nil lastImageSizeCounter = viewModel.imageSizeChangeCounter imageSizeTimer = Timer.scheduledTimer(withTimeInterval: 0.1, repeats: true) { [weak self] _ in Task { `@MainActor` [weak self] in guard let self = self else { return } let currentCounter = self.viewModel.imageSizeChangeCounter if currentCounter != self.lastImageSizeCounter { self.lastImageSizeCounter = currentCounter self.resizeToFitImage() } } } } + +deinit { + imageSizeTimer?.invalidate() + imageSizeTimer = nil +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Features/Preview/PreviewWindow.swift` around lines 103 - 113, The repeating Timer assigned to imageSizeTimer keeps firing after the window is closed or when multiple observers start because it's never invalidated; update the lifecycle to (1) avoid creating duplicate timers by invalidating any existing imageSizeTimer before scheduling a new one, and (2) invalidate and nil out imageSizeTimer when the preview window is closed or the owner is deinitialized (e.g., in windowWillClose/deinit or an appropriate cleanup method) so resizeToFitImage and the imageSizeChangeCounter polling stop running and resources are released.
🧹 Nitpick comments (2)
ScreenTranslate/Features/Settings/SettingsViewModel.swift (1)
353-353: 移除未使用的oldValue捕获。这里先读取了旧值，但后续未使用，容易误导读者以为存在前后值比较逻辑。建议直接删除该行，保持 setter 语义清晰。
♻️ 建议修改
- let oldValue = settings.vlmProvider settings.vlmProvider = newValue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Features/Settings/SettingsViewModel.swift` at line 353, Remove the unused capture of the previous value by deleting the line that declares oldValue (let oldValue = settings.vlmProvider) in SettingsViewModel's setter for vlmProvider; ensure no other code depends on oldValue and keep the setter logic using settings.vlmProvider directly to avoid misleading readers about a before/after comparison.ScreenTranslate/Features/Onboarding/OnboardingComponents.swift (1)
77-83: 让OnboardingPermissionRow的 API 和实际行为保持一致。
openSettingsAction现在已经不会被调用了，但OnboardingPermissionsStepView.swift的 Line 38-54 仍然要为它传 closure。要么删掉这个参数和调用端，要么在 UI 上恢复一个明确的“打开设置”入口；保留失效回调会误导后续调用者。Also applies to: 112-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScreenTranslate/Features/Onboarding/OnboardingComponents.swift` around lines 77 - 83, The OnboardingPermissionRow's API exposes openSettingsAction but the UI no longer invokes it; update code to align behavior by either removing the openSettingsAction parameter from the OnboardingPermissionRow struct and all call sites (e.g., where OnboardingPermissionsStepView passes a closure at lines referenced) or reintroducing a visible "Open Settings" control in OnboardingPermissionRow that calls openSettingsAction; locate the symbol OnboardingPermissionRow and the usage in OnboardingPermissionsStepView and either (A) delete the openSettingsAction property and remove the passed closures, or (B) add a clear button/tap target in the row UI that invokes openSettingsAction so the API is functional.
🤖 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/AppDelegate.swift`:
- Around line 71-74: The code skips initializing Sparkle/updater by only
creating updaterController when settings.onboardingCompleted is true, which
misses the case where the user manually closes the onboarding window; modify the
onboarding-close path to call initializeUpdaterIfNeeded() (or create
updaterController) and register the checkForUpdates observer when onboarding is
dismissed. Concretely, update the OnboardingWindowController's close/completion
handler (referenced around OnboardingWindowController line 60-68 and 96-103) to
invoke AppDelegate.initializeUpdaterIfNeeded() or to access
AppDelegate.updaterController so the same initialization and observer
registration that occurs when settings.onboardingCompleted == true also runs
when onboarding is closed early; also mirror this fix for the alternate branch
referenced at lines 85-91 so both code paths initialize Sparkle consistently.
In `@ScreenTranslate/Features/Onboarding/OnboardingPermissionsStepView.swift`:
- Around line 82-88: The Skip button currently calls onSkip() even when
canGoNext == true, causing OnboardingViewModel.skipPermissions() to incorrectly
set hasSkippedPermissions/userSkippedPermissions to true; update the UI to
disable or hide the Skip Button when canGoNext is true (i.e., wrap the Button
with a conditional or set .disabled(canGoNext)) or add a guard inside
OnboardingViewModel.skipPermissions() to no-op when canGoNext (or both) so skip
cannot be applied after all permissions are granted.
In `@ScreenTranslate/Features/Onboarding/OnboardingViewModel.swift`:
- Around line 109-113: The skipPermissions() implementation currently calls
completeOnboarding() and bypasses the Complete step and its UI flag; change
skipPermissions() to instead mark hasSkippedPermissions = true and advance the
onboarding flow to the final step (e.g., set the current step/state to the
completion step rather than calling completeOnboarding()), so that
OnboardingCompleteStepView.can read hasSkippedPermissions and show the
skipped-permissions warning; keep completeOnboarding() as the actual
finalization action invoked by the Start button handler in the completion view.
In `@ScreenTranslate/Models/AppSettings.swift`:
- Around line 556-558: The paddleOCRCloudAPIKey is being set to an empty string
which permanently prevents loading the saved Keychain value; instead make this
value lazily loaded (not pre-initialized to "") so existing saved keys are
returned when first needed. Change the property paddleOCRCloudAPIKey to be
nil/optional or a lazy/computed property that fetches the key from the Keychain
on first access (or provide a loadPaddleOCRCloudAPIKey() helper that reads
Keychain without triggering UI until invoked), and ensure callers in
SettingsViewModel and PaddleOCRVLMProvider read via that lazy getter or call the
loader before sending requests so old users get their saved API key after
restart.
---
Outside diff comments:
In `@ScreenTranslate/Features/Preview/PreviewWindow.swift`:
- Around line 103-113: The repeating Timer assigned to imageSizeTimer keeps
firing after the window is closed or when multiple observers start because it's
never invalidated; update the lifecycle to (1) avoid creating duplicate timers
by invalidating any existing imageSizeTimer before scheduling a new one, and (2)
invalidate and nil out imageSizeTimer when the preview window is closed or the
owner is deinitialized (e.g., in windowWillClose/deinit or an appropriate
cleanup method) so resizeToFitImage and the imageSizeChangeCounter polling stop
running and resources are released.
---
Nitpick comments:
In `@ScreenTranslate/Features/Onboarding/OnboardingComponents.swift`:
- Around line 77-83: The OnboardingPermissionRow's API exposes
openSettingsAction but the UI no longer invokes it; update code to align
behavior by either removing the openSettingsAction parameter from the
OnboardingPermissionRow struct and all call sites (e.g., where
OnboardingPermissionsStepView passes a closure at lines referenced) or
reintroducing a visible "Open Settings" control in OnboardingPermissionRow that
calls openSettingsAction; locate the symbol OnboardingPermissionRow and the
usage in OnboardingPermissionsStepView and either (A) delete the
openSettingsAction property and remove the passed closures, or (B) add a clear
button/tap target in the row UI that invokes openSettingsAction so the API is
functional.
In `@ScreenTranslate/Features/Settings/SettingsViewModel.swift`:
- Line 353: Remove the unused capture of the previous value by deleting the line
that declares oldValue (let oldValue = settings.vlmProvider) in
SettingsViewModel's setter for vlmProvider; ensure no other code depends on
oldValue and keep the setter logic using settings.vlmProvider directly to avoid
misleading readers about a before/after comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5899511-555f-4421-8edc-2fc061f465f2
📒 Files selected for processing (13)
ScreenTranslate/App/AppDelegate.swiftScreenTranslate/Features/Onboarding/OnboardingCompleteStepView.swiftScreenTranslate/Features/Onboarding/OnboardingComponents.swiftScreenTranslate/Features/Onboarding/OnboardingConfigurationStepView.swiftScreenTranslate/Features/Onboarding/OnboardingPermissionsStepView.swiftScreenTranslate/Features/Onboarding/OnboardingView.swiftScreenTranslate/Features/Onboarding/OnboardingViewModel.swiftScreenTranslate/Features/Onboarding/OnboardingWindowController.swiftScreenTranslate/Features/Preview/PreviewWindow.swiftScreenTranslate/Features/Settings/SettingsViewModel.swiftScreenTranslate/Models/AppSettings.swiftScreenTranslate/Resources/en.lproj/Localizable.stringsScreenTranslate/Resources/zh-Hans.lproj/Localizable.strings
💤 Files with no reviewable changes (1)
- ScreenTranslate/Features/Onboarding/OnboardingConfigurationStepView.swift
Comment on lines
+556
to
+558
| // Load PaddleOCR cloud API key from Keychain (secure storage) | ||
| paddleOCRCloudAPIKey = Self.loadPaddleOCRAPIKeyFromKeychain() | ||
| // Defer keychain access to avoid triggering UI on first launch | ||
| paddleOCRCloudAPIKey = "" |
There was a problem hiding this comment.
不要把已保存的 PaddleOCR Cloud API Key 永久丢成空串。
这里为了避免首启触发 Keychain UI 把 paddleOCRCloudAPIKey 直接初始化成 ""，但这个文件里已经没有任何后续回填路径了。SettingsViewModel.swift 的 Line 996-1001 和 PaddleOCRVLMProvider.swift 的 Line 75-85 都直接读取这个属性发请求，所以老用户重启后云端 PaddleOCR 会一直拿不到已保存密钥，直到手动重新输入。请改成“延迟加载”，不要变成“不再加载”。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ScreenTranslate/Models/AppSettings.swift` around lines 556 - 558, The
paddleOCRCloudAPIKey is being set to an empty string which permanently prevents
loading the saved Keychain value; instead make this value lazily loaded (not
pre-initialized to "") so existing saved keys are returned when first needed.
Change the property paddleOCRCloudAPIKey to be nil/optional or a lazy/computed
property that fetches the key from the Keychain on first access (or provide a
loadPaddleOCRCloudAPIKey() helper that reads Keychain without triggering UI
until invoked), and ensure callers in SettingsViewModel and PaddleOCRVLMProvider
read via that lazy getter or call the loader before sending requests so old
users get their saved API key after restart.
- skipPermissions() now navigates to Complete step instead of skipping it - Disable skip button when all permissions are already granted - Initialize Sparkle updater when onboarding window is dismissed without completing
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Onboarding/OnboardingViewModel.swift`:
- Around line 29-30: The hasSkippedPermissions flag is never cleared, causing
sessions where the user skips then later grants permissions to still be treated
as skipped; update the ViewModel so that when the Permissions step reports
success (the method handling permission grant confirmation in
OnboardingViewModel / the Permissions step completion handler) you reset
hasSkippedPermissions = false, and additionally add a guard in the logic that
decides whether the session is skipped (e.g., the code used by
goToPreviousStep(), the Complete step warning and the completion logic
referenced around the existing hasSkippedPermissions checks) to verify
currentMissingPermissions is empty (or call a helper like
hasMissingPermissions()) instead of relying solely on the boolean; ensure the
Complete-page warning and finalization path consult this runtime permission
check so granting permissions after a skip clears the skipped state.
- Around line 244-247: The code currently only writes
settings.userSkippedPermissions = true when hasSkippedPermissions is true, so
the persisted flag never gets cleared; in OnboardingViewModel where you post
.onboardingCompleted, replace the conditional with an explicit write of the
final state (e.g. set settings.userSkippedPermissions = hasSkippedPermissions)
so the persisted value reflects the actual outcome (skipped or not) before
posting the onboardingCompleted notification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 214553a6-fbd4-40b6-854d-b218d58bfa9a
📒 Files selected for processing (4)
ScreenTranslate/App/AppDelegate.swiftScreenTranslate/Features/Onboarding/OnboardingPermissionsStepView.swiftScreenTranslate/Features/Onboarding/OnboardingViewModel.swiftScreenTranslate/Features/Onboarding/OnboardingWindowController.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- ScreenTranslate/Features/Onboarding/OnboardingWindowController.swift
- ScreenTranslate/Features/Onboarding/OnboardingPermissionsStepView.swift
Comment on lines
+29
to
+30
| /// Whether user has skipped the permissions step | ||
| var hasSkippedPermissions = false |
There was a problem hiding this comment.
hasSkippedPermissions 会“粘住”，回退后补齐权限仍会被当成 skipped。
这里一旦置成 true 就没有回退路径了；而 goToPreviousStep() 允许用户从 Complete 回到 Permissions。用户如果先点 Skip、再返回补齐两个权限，Complete 页 warning 仍会显示，后面的完成逻辑也会把这次会话当成 skipped。建议在权限页成功通过后清掉这个标志，并在 ViewModel 里也 guard 一下“当前仍有缺失权限”，别只靠按钮禁用维持不变量。
Also applies to: 109-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ScreenTranslate/Features/Onboarding/OnboardingViewModel.swift` around lines
29 - 30, The hasSkippedPermissions flag is never cleared, causing sessions where
the user skips then later grants permissions to still be treated as skipped;
update the ViewModel so that when the Permissions step reports success (the
method handling permission grant confirmation in OnboardingViewModel / the
Permissions step completion handler) you reset hasSkippedPermissions = false,
and additionally add a guard in the logic that decides whether the session is
skipped (e.g., the code used by goToPreviousStep(), the Complete step warning
and the completion logic referenced around the existing hasSkippedPermissions
checks) to verify currentMissingPermissions is empty (or call a helper like
hasMissingPermissions()) instead of relying solely on the boolean; ensure the
Complete-page warning and finalization path consult this runtime permission
check so granting permissions after a skip clears the skipped state.
- Restore PaddleOCR cloud API key loading from Keychain on init to avoid losing saved credentials for existing users - Clear hasSkippedPermissions flag when all permissions are granted during polling, preventing stale skipped state after user goes back - Check actual permission state in completeOnboarding() before persisting userSkippedPermissions to UserDefaults
Owner
Author
Replied to CodeRabbit review feedbackAll issues have been addressed: Already fixed in commit 0eaa366:
Fixed in commit 33a1cbf:
|
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
Changes
@MainActorisolation raceTest plan
ScreenTranslate.onboardingCompleted, verify 3-step flowSummary by CodeRabbit
发布说明
新功能
改进
本地化