Repository navigation
Fix development Settings window and Xcode diagnostics - #237
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughSummary
Configuration and security
Security/privacy boundary changes
Validation
Affected areas
WalkthroughAuthentication presentation-anchor lookup is centralized across Apple, Google, and Microsoft authentication flows, multi-scene support is enabled through conditional Info.plist preprocessing, and account connection error paths explicitly discard refresh results. ChangesAuthentication and development settings
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ASAuthorizationController
participant SignInWithAppleService
participant ASWebAuthenticationSession
participant GoogleGmailOAuthService
participant MicrosoftGraphOAuthService
participant AuthenticationPresentationAnchorStore
ASAuthorizationController->>SignInWithAppleService: request presentation anchor
SignInWithAppleService->>AuthenticationPresentationAnchorStore: captureCurrent()
AuthenticationPresentationAnchorStore-->>SignInWithAppleService: captured anchor
SignInWithAppleService-->>ASAuthorizationController: return anchor
ASWebAuthenticationSession->>GoogleGmailOAuthService: request presentation anchor
GoogleGmailOAuthService->>AuthenticationPresentationAnchorStore: current()
AuthenticationPresentationAnchorStore-->>GoogleGmailOAuthService: return anchor
ASWebAuthenticationSession->>MicrosoftGraphOAuthService: request presentation anchor
MicrosoftGraphOAuthService->>AuthenticationPresentationAnchorStore: current()
AuthenticationPresentationAnchorStore-->>MicrosoftGraphOAuthService: return anchor
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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: 37ab26363d
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@apps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swift`:
- Around line 28-38: Update AuthenticationPresentationAnchor.current() and its
callers in SignInWithAppleService so the presenting window from the sign-in UI
is passed through and used as the ASAuthorizationController presentation anchor.
Do not select the first foreground-active scene or arbitrary AppKit window; when
no presenting window is available, return no anchor using the existing API’s
optional behavior.
- Around line 13-23: Update the synchronous MainActor access around
currentOnMainActor so it uses a bridge that actually establishes MainActor
isolation instead of relying on Thread.isMainThread and
MainActor.assumeIsolated. Preserve synchronous behavior for both main-thread and
background callers, and remove or replace assumeIsolated in this path to prevent
isolation traps.
In `@apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift`:
- Around line 51-80: Update
testAuthenticationPresentationAnchorsCanBeRequestedOffMainThread to create
deterministic multiple active scenes with distinct windows, then assert that
SignInWithAppleService and each web provider return the intended authentication
window rather than ignoring the anchors. Keep the off-main-thread request
coverage, ensure scene/window setup and teardown are isolated, and wait for
assertions to complete without relying on the detached fallback.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6ab697b-62df-4329-8bb8-f473247cdef7
📒 Files selected for processing (8)
.changeset/open-development-settings-window.mdapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Info.plistapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swiftapps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Create a Changeset for runtime behavior changes or exported type/API changes; tests-only, internal refactors, docs-only changes, and JSDoc-only maintenance may skip it.
Files:
.changeset/open-development-settings-window.md
apps/unwired-mail/**/*.{swift,m,mm}
📄 CodeRabbit inference engine (AGENTS.md)
apps/unwired-mail/**/*.{swift,m,mm}: The Apple app must provide formatter, linter, and test commands; usezsh scripts/check-apple-lint.zshfor formatting and linting.
Run Apple tests withxcodebuild test -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=iOS Simulator,name=iPhone 17'.
Every Apple code change must receive formatter, linter, and test validation unless Apple tooling or a platform dependency is unavailable.
Files:
apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
SwiftLint runs in strict mode, so warnings fail validation.
Files:
apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift
⚙️ CodeRabbit configuration file
apps/unwired-mail/**/*.swift: Prioritize functional correctness, data protection, and Apple platform
behavior over style already enforced by swift-format and SwiftLint.
Check actor isolation, cancellation, lifecycle transitions, persistence,
retry and offline behavior, and Keychain accessibility. Flag credentials,
tokens, mail content, or decrypted Product Sync data that can be logged,
persisted outside approved device-local storage, or cross a documented
privacy boundary. Require focused tests for changed behavior and failure
paths.
Files:
apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift
**/*.{swift,m,mm}
📄 CodeRabbit inference engine (AGENTS.md)
Run the smallest meaningful checks for the touched Apple area first, then broaden validation for shared tooling or cross-project behavior.
Files:
apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Touch only files required by the request; do not refactor, reformat, or improve adjacent code, and remove only unused items created by your changes.
Files:
apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Info.plistapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift
**/*Tests/**/*.swift
⚙️ CodeRabbit configuration file
**/*Tests/**/*.swift: Verify externally observable behavior, privacy invariants, error paths,
cancellation or restart behavior where relevant, and deterministic
isolation. Flag fragile sleeps, ordering dependencies, and tests that
duplicate the implementation instead of exercising it.
Files:
apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift
🪛 markdownlint-cli2 (0.23.1)
.changeset/open-development-settings-window.md
[warning] 4-4: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (8)
apps/unwired-mail/unwired-mail/Info.plist (1)
28-32: LGTM!apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift (1)
193-199: LGTM!.changeset/open-development-settings-window.md (1)
1-3: 🗄️ Data Integrity & IntegrationNo changes needed.
Empty front matter is the repo’s normal Changesets pattern for app-level release notes, so this file doesn’t need package-specific bump metadata.> Likely an incorrect or invalid review comment.apps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swift (1)
317-320: LGTM!apps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swift (1)
4094-4097: LGTM!apps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swift (1)
284-287: LGTM!apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift (1)
1228-1230: LGTM!apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)
6511-6518: LGTM!Also applies to: 6580-6580
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 (1)
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift (1)
51-95: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the no-anchor error paths.
This only tests a populated store. Add deterministic tests that an empty store prevents launch and returns
presentationAnchorUnavailablefor Apple andwebAuthenticationUnavailablefor Google/Microsoft.As per path instructions, “Verify externally observable behavior, privacy invariants, error paths, cancellation or restart behavior where relevant, and deterministic isolation.”
🤖 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 `@apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift` around lines 51 - 95, Extend the authentication presentation tests around testAuthenticationPresentationAnchorsCanBeRequestedOffMainThread to use empty AuthenticationPresentationAnchorStore instances and verify the externally observable failure behavior: Apple authorization must not launch and must return presentationAnchorUnavailable, while Google and Microsoft web authentication must not launch and must return webAuthenticationUnavailable. Keep the tests deterministic and isolated, covering each provider’s no-anchor path without relying on a populated presentation anchor.Source: Path instructions
🤖 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.
Outside diff comments:
In `@apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift`:
- Around line 51-95: Extend the authentication presentation tests around
testAuthenticationPresentationAnchorsCanBeRequestedOffMainThread to use empty
AuthenticationPresentationAnchorStore instances and verify the externally
observable failure behavior: Apple authorization must not launch and must return
presentationAnchorUnavailable, while Google and Microsoft web authentication
must not launch and must return webAuthenticationUnavailable. Keep the tests
deterministic and isolated, covering each provider’s no-anchor path without
relying on a populated presentation anchor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd03305e-d723-4fa8-905e-61624b1b7fea
📒 Files selected for processing (6)
apps/unwired-mail/unwired-mail.xcodeproj/project.pbxprojapps/unwired-mail/unwired-mail/Info.plistapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swiftapps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Apple · Unwired Mail
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Touch only files required by the request; do not refactor, reformat, or improve adjacent code, and remove only unused items created by your changes.
Files:
apps/unwired-mail/unwired-mail/Info.plistapps/unwired-mail/unwired-mail.xcodeproj/project.pbxprojapps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swift
apps/unwired-mail/**/*.{swift,m,mm}
📄 CodeRabbit inference engine (AGENTS.md)
apps/unwired-mail/**/*.{swift,m,mm}: The Apple app must provide formatter, linter, and test commands; usezsh scripts/check-apple-lint.zshfor formatting and linting.
Run Apple tests withxcodebuild test -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=iOS Simulator,name=iPhone 17'.
Every Apple code change must receive formatter, linter, and test validation unless Apple tooling or a platform dependency is unavailable.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
SwiftLint runs in strict mode, so warnings fail validation.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swift
⚙️ CodeRabbit configuration file
apps/unwired-mail/**/*.swift: Prioritize functional correctness, data protection, and Apple platform
behavior over style already enforced by swift-format and SwiftLint.
Check actor isolation, cancellation, lifecycle transitions, persistence,
retry and offline behavior, and Keychain accessibility. Flag credentials,
tokens, mail content, or decrypted Product Sync data that can be logged,
persisted outside approved device-local storage, or cross a documented
privacy boundary. Require focused tests for changed behavior and failure
paths.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swift
**/*.{swift,m,mm}
📄 CodeRabbit inference engine (AGENTS.md)
Run the smallest meaningful checks for the touched Apple area first, then broaden validation for shared tooling or cross-project behavior.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swift
**/*Tests/**/*.swift
⚙️ CodeRabbit configuration file
**/*Tests/**/*.swift: Verify externally observable behavior, privacy invariants, error paths,
cancellation or restart behavior where relevant, and deterministic
isolation. Flag fragile sleeps, ordering dependencies, and tests that
duplicate the implementation instead of exercising it.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift
🧬 Code graph analysis (1)
apps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swift (2)
apps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swift (1)
presentationAnchor(292-292)apps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swift (1)
presentationAnchor(340-340)
🔇 Additional comments (7)
apps/unwired-mail/unwired-mail/Services/ProductAccount/SignInWithAppleService.swift (2)
13-19: Use the initiating scene’s window.
connectedScenesis unordered, so.first(where: \.isKeyWindow)can select a different active window after enabling multiple scenes. Pass the presenting window/scene from the sign-in UI instead of globally scanning scenes. This prior concern remains after centralization. As per path instructions, “Prioritize functional correctness, data protection, and Apple platform behavior over style already enforced by swift-format and SwiftLint.”Source: Path instructions
28-53: LGTM!Also applies to: 96-118, 193-205, 237-239, 339-342
apps/unwired-mail/unwired-mail/Services/ProductAccount/GoogleGmailOAuthService.swift (1)
142-159: LGTM!Also applies to: 226-229, 291-295
apps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swift (1)
3925-3947: LGTM!Also applies to: 4041-4044, 4101-4105
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift (1)
1243-1243: LGTM!apps/unwired-mail/unwired-mail.xcodeproj/project.pbxproj (1)
676-677: LGTM!Also applies to: 713-713
apps/unwired-mail/unwired-mail/Info.plist (1)
28-34: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f2ab6fdc2
ℹ️ 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".
1 participant
Summary
Root cause
The development Settings entry point opens a separate scene, but the app manifest did not declare multiple-scene support. AuthenticationServices can also invoke its synchronous presentation-context callbacks outside the main actor; those callbacks directly accessed
UIApplication, scene, and window state, producing Main Thread Checker reports. Three recovery paths also discarded optionaltry?results without explicitly assigning them.User impact
The development Settings window opens on Mac Catalyst, authentication flows resolve their presentation window without off-main UIKit access, and the related Xcode diagnostics are removed.
Validation
mise exec -- zsh scripts/check-apple-lint.zshMTC_CRASH_ON_REPORT=1 mise exec -- xcodebuild test -quiet -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=iOS Simulator,name=iPhone 17' -only-testing:unwired-mailTests/ProductAccountSessionTests/testAuthenticationPresentationAnchorsCanBeRequestedOffMainThreadmise exec -- xcodebuild test -quiet -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=iOS Simulator,name=iPhone 17'mise exec -- xcodebuild build -quiet -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=macOS,variant=Mac Catalyst' CODE_SIGNING_ALLOWED=NOUIApplicationSupportsMultipleScenes = truegit diff --checkA Catalyst-hosted focused test could not launch in this checkout because no development signing team is configured; the unsigned Catalyst build completed successfully.
Refs #115
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.