[M1] Add menu bar UI shell#35
Merged
Merged
Conversation
Peerapat-J
added
enhancement
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: 2
🧹 Nitpick comments (4)
Sources/LinguistMac/AppShellModel.swift (1)
231-244: ⚡ Quick winSimplify clipboard service with
@MainActorinstead ofactor.Since both
readText()andwriteText()immediately dispatch toMainActor.run, wrapping the service in anactoradds isolation overhead without concurrency benefit. NSPasteboard requires main-thread access, so the entire type can be@MainActor.♻️ Proposed refactor to use `@MainActor` directly
-actor SystemClipboardService: ClipboardServicing { +@MainActor +final class SystemClipboardService: ClipboardServicing { func readText() async -> String? { - await MainActor.run { - NSPasteboard.general.string(forType: .string) - } + NSPasteboard.general.string(forType: .string) } func writeText(_ text: String) async { - await MainActor.run { - NSPasteboard.general.clearContents() - NSPasteboard.general.setString(text, forType: .string) - } + NSPasteboard.general.clearContents() + NSPasteboard.general.setString(text, forType: .string) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/LinguistMac/AppShellModel.swift` around lines 231 - 244, Replace the actor-based SystemClipboardService with a main-isolated type by removing `actor` and marking the type `@MainActor` so you avoid unnecessary actor isolation and still satisfy NSPasteboard's main-thread requirement; update the declaration of SystemClipboardService (which conforms to ClipboardServicing) and keep the implementations of readText() and writeText() but remove the surrounding await MainActor.run { ... } calls so they run on the main actor naturally and continue to call NSPasteboard.general.string(forType: .string), clearContents(), and setString(_:forType:) directly.Sources/LinguistMac/TranslationPopupView.swift (1)
125-146: ⚡ Quick winPrefer display names over raw values in user-facing error messages.
The error text interpolates
kind.rawValuefor permission kinds andproviderID.rawValuefor provider IDs, which may expose technical identifiers (e.g., "screenRecording", "deepl") rather than user-friendly names ("Screen Recording", "DeepL"). This reduces clarity for non-technical users.♻️ Proposed improvement using display-friendly strings
extension TranslationFailure { var displayText: String { switch self { case let .permissionDenied(kind): - "Permission required: \(kind.rawValue)." + "Permission required: \(kind.displayName)." case .captureCancelled: "Screen capture was cancelled." case .noTextRecognized: "No text was recognized." case .emptyInput: "Enter text before translating." case .unsupportedLanguagePair: "This language pair is not available yet." case let .providerUnavailable(providerID): - "Provider is unavailable: \(providerID.rawValue)." + "Translation provider \(providerID.displayName) is unavailable." case let .missingAPIKey(providerID): - "API key required for \(providerID.rawValue)." + "API key required for \(providerID.displayName)." case let .providerFailed(message): message } } }If
PermissionKindandTranslationProviderIDdon't have adisplayNameproperty, add one or maintain a lookup mapping in the extension.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/LinguistMac/TranslationPopupView.swift` around lines 125 - 146, The user-facing strings in TranslationFailure.displayText use kind.rawValue and providerID.rawValue; update the .permissionDenied(kind), .providerUnavailable(providerID) and .missingAPIKey(providerID) branches to use a human-friendly display name instead (e.g., kind.displayName or a lookup for PermissionKind and providerID.displayName or a TranslationProviderID-to-name map) so messages show "Screen Recording" / "DeepL" style labels rather than technical identifiers; add a displayName computed property or local mapping on PermissionKind and TranslationProviderID if they don't exist and interpolate that into the existing cases.Sources/LinguistMac/OnboardingView.swift (1)
78-82: ⚡ Quick winExtract duplicated Settings window opener to shared helper.
The
openSettingsWindow()pattern usingSelector(("showSettingsWindow:"))andNSApp.activateis duplicated in MenuBarMenuView.swift lines 109–113. Centralizing this logic reduces maintenance burden and ensures consistent behavior.♻️ Proposed extraction to AppShellModel extension
Add to
AppShellModel.swift:extension AppShellModel { func openSettingsWindow() { record(.settings) NSApp.sendAction(Selector(("showSettingsWindow:")), to: nil, from: nil) NSApp.activate(ignoringOtherApps: true) } }Then update both call sites:
MenuBarMenuView.swift:
private func openSettingsWindow() { - model.record(.settings) - NSApp.sendAction(Selector(("showSettingsWindow:")), to: nil, from: nil) - activateApp() + model.openSettingsWindow() }OnboardingView.swift:
private func openSettingsWindow() { - model.record(.settings) - NSApp.sendAction(Selector(("showSettingsWindow:")), to: nil, from: nil) - NSApp.activate(ignoringOtherApps: true) + model.openSettingsWindow() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/LinguistMac/OnboardingView.swift` around lines 78 - 82, Extract the duplicated settings-opening logic into an AppShellModel extension method and update both call sites to use it: add an extension on AppShellModel with a new func openSettingsWindow() that calls record(.settings), NSApp.sendAction(Selector(("showSettingsWindow:")), to: nil, from: nil) and NSApp.activate(ignoringOtherApps: true); then remove or replace the private openSettingsWindow() in OnboardingView and the duplicate code in MenuBarMenuView to call model.openSettingsWindow() instead so both use the shared implementation.Sources/LinguistMac/SettingsView.swift (1)
50-54: 💤 Low valueConsider hiding unimplemented launch-at-login control.
The toggle for "Launch at login" is permanently disabled with a caption explaining the feature will land later. Disabled controls reduce discoverability and can confuse users who attempt to interact with them.
♻️ Alternative: hide the toggle until the feature ships
Toggle("Auto-copy result", isOn: $model.settings.autoCopyEnabled) - Toggle("Launch at login", isOn: $model.settings.launchAtLoginEnabled) - .disabled(true) - Text("Launch-at-login wiring lands with the app preferences milestone.") - .font(.caption) - .foregroundStyle(.secondary) + // Launch at login lands with M2 app preferences milestoneOr keep it visible but use a custom label with "(Coming soon)" suffix instead of a disabled toggle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/LinguistMac/SettingsView.swift` around lines 50 - 54, The "Launch at login" Toggle is permanently disabled and should be hidden or clarified; update the view around Toggle("Launch at login", isOn: $model.settings.launchAtLoginEnabled) so it is only shown when the feature flag or milestone is enabled (e.g., wrap it in an if model.settings.isLaunchAtLoginAvailable) or replace the disabled Toggle with a non-interactive label that reads "Launch at login (Coming soon)" alongside the explanatory Text; ensure you remove the .disabled(true) call if using the "(Coming soon)" label and keep the explanatory caption Text("Launch-at-login wiring lands with the app preferences milestone.") as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/LinguistMac/MenuBarMenuView.swift`:
- Around line 100-107: In summary(for result: TranslationResult) the truncation
check is off: change the condition so text lengths up to 27 are returned
unchanged and anything longer is truncated to 27 characters plus "..." (i.e.,
use text.count <= 27 instead of 30) so the else branch String(text.prefix(27)) +
"..." yields a shorter displayed string for longer inputs; locate the
summary(for:) function and update the threshold logic around the text variable
accordingly.
In `@Sources/LinguistMac/SettingsView.swift`:
- Around line 89-100: The .appleTranslation and .cloudProvider branches in the
ReadinessRow action currently only call model.record(.settings) and do not open
any UI; change those branches to call model.openSettingsWindow() (matching
OnboardingView.swift) and then record the event (i.e., call
model.openSettingsWindow() before model.record(.settings)) so clicking "Open"
opens the settings window and still logs the action; locate these changes in the
ForEach over model.readiness.items and the ReadinessRow closure where item.kind
is switched.
---
Nitpick comments:
In `@Sources/LinguistMac/AppShellModel.swift`:
- Around line 231-244: Replace the actor-based SystemClipboardService with a
main-isolated type by removing `actor` and marking the type `@MainActor` so you
avoid unnecessary actor isolation and still satisfy NSPasteboard's main-thread
requirement; update the declaration of SystemClipboardService (which conforms to
ClipboardServicing) and keep the implementations of readText() and writeText()
but remove the surrounding await MainActor.run { ... } calls so they run on the
main actor naturally and continue to call NSPasteboard.general.string(forType:
.string), clearContents(), and setString(_:forType:) directly.
In `@Sources/LinguistMac/OnboardingView.swift`:
- Around line 78-82: Extract the duplicated settings-opening logic into an
AppShellModel extension method and update both call sites to use it: add an
extension on AppShellModel with a new func openSettingsWindow() that calls
record(.settings), NSApp.sendAction(Selector(("showSettingsWindow:")), to: nil,
from: nil) and NSApp.activate(ignoringOtherApps: true); then remove or replace
the private openSettingsWindow() in OnboardingView and the duplicate code in
MenuBarMenuView to call model.openSettingsWindow() instead so both use the
shared implementation.
In `@Sources/LinguistMac/SettingsView.swift`:
- Around line 50-54: The "Launch at login" Toggle is permanently disabled and
should be hidden or clarified; update the view around Toggle("Launch at login",
isOn: $model.settings.launchAtLoginEnabled) so it is only shown when the feature
flag or milestone is enabled (e.g., wrap it in an if
model.settings.isLaunchAtLoginAvailable) or replace the disabled Toggle with a
non-interactive label that reads "Launch at login (Coming soon)" alongside the
explanatory Text; ensure you remove the .disabled(true) call if using the
"(Coming soon)" label and keep the explanatory caption Text("Launch-at-login
wiring lands with the app preferences milestone.") as-is.
In `@Sources/LinguistMac/TranslationPopupView.swift`:
- Around line 125-146: The user-facing strings in TranslationFailure.displayText
use kind.rawValue and providerID.rawValue; update the .permissionDenied(kind),
.providerUnavailable(providerID) and .missingAPIKey(providerID) branches to use
a human-friendly display name instead (e.g., kind.displayName or a lookup for
PermissionKind and providerID.displayName or a TranslationProviderID-to-name
map) so messages show "Screen Recording" / "DeepL" style labels rather than
technical identifiers; add a displayName computed property or local mapping on
PermissionKind and TranslationProviderID if they don't exist and interpolate
that into the existing cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b748e67a-dc73-4dca-9da4-5a8066e2e1f5
📒 Files selected for processing (12)
LinguistMac.xcodeproj/project.pbxprojSources/LinguistMac/AppShellModel.swiftSources/LinguistMac/ContentView.swiftSources/LinguistMac/LinguistMacApp.swiftSources/LinguistMac/MenuBarMenuView.swiftSources/LinguistMac/OnboardingView.swiftSources/LinguistMac/QuickTranslateView.swiftSources/LinguistMac/SettingsView.swiftSources/LinguistMac/TranslationPopupView.swiftSources/LinguistMacCore/AppSettings.swiftSources/LinguistMacCore/AppShellModels.swiftTests/LinguistMacCoreTests/AppShellModelsTests.swift
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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
Issues
Closes #7
Closes #10
Closes #11
Closes #12
Closes #13
Validation
Summary by CodeRabbit