Sean/magic links#393
Draft
seanperez29 wants to merge 27 commits into
Draft
Conversation
Implement email_link as a first-factor authentication strategy: - Add NativeMagicLink module for PKCE generation and flow storage - Add signInWithEmailLink and completeMagicLink methods to Auth - Add sendEmailLink method to SignIn for preparing the factor - Add emailLink case to FactorStrategy enum - Add SignInFactorOneEmailLinkView for the UI flow - Add Clerk.handle(_:) for routing incoming URLs - Update factor selection logic to prefer email link for email identifiers # Conflicts: # Sources/ClerkKitUI/Components/Auth/AuthView.swift # Tests/Domains/Auth/AuthTests.swift
Extend magic link handling to support sign-up flows alongside sign-in. Rename NativeMagicLink types to MagicLink for broader applicability. - Add SignUp.sendEmailLink() with PKCE code challenge generation - Unify EmailLinkVerificationView for both sign-in and sign-up flows - Add URLHandlingCoordinator to deduplicate concurrent callbacks - Update AuthNavigation to defer email link verification until other requirements are satisfied - Return TransferFlowResult from completeMagicLink for flexibility - Add tests for URL deduplication and sign-up email link verification
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds native email magic-link: PKCE generation and keychain persistence, deep-link parsing and deduplicated URL handling, prepare/complete API flows with new Auth entrypoints, UI screens and localization, DI for a MagicLinkStore, example app URL scheme, and extensive unit/UI tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as "App / AuthView"
participant Clerk as "Clerk"
participant URLCoord as "URLHandlingCoordinator"
participant Auth as "Auth"
participant Store as "MagicLinkStore"
participant API as "Clerk API"
participant Session as "Session Service"
User->>App: Request email magic link
App->>Clerk: signInWithEmailLink(email)
Clerk->>Auth: signInWithEmailLink
Auth->>API: POST prepareFirstFactor (strategy=email_link, code_challenge, redirect_uri)
API-->>Auth: SignIn prepared
Auth->>Store: save(kind:.signIn, codeVerifier)
Note over User,App: User taps deep link
App->>Clerk: handle(url)
Clerk->>URLCoord: handle(route, operation)
URLCoord->>Auth: completeMagicLink(flowId, approvalToken)
Auth->>Store: load() -> codeVerifier
Auth->>API: POST /v1/client/magic_links/complete (flow_id, approval_token, code_verifier)
API-->>Auth: { ticket }
Auth->>Session: signInWithTicket(ticket) / activate
Session-->>Auth: session active
Auth->>Store: clear()
URLCoord-->>Clerk: return success
Clerk-->>App: return true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
Sources/ClerkKitUI/Extensions/SignUp+Ext.swift (1)
26-27: Use enum raw value instead of hard-coded"email_link"literal.This avoids silent drift between domain constants and fallback matching logic.
♻️ Suggested refactor
- if let verifications = Clerk.shared.environment?.userSettings.attributes["email_address"]?.verifications, - verifications.contains("email_link") + if let verifications = Clerk.shared.environment?.userSettings.attributes["email_address"]?.verifications, + verifications.contains(FactorStrategy.emailLink.rawValue)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKitUI/Extensions/SignUp`+Ext.swift around lines 26 - 27, The code is matching the verification type using the hard-coded string "email_link"; replace this literal with the enum raw value to avoid drift—use the existing verification enum (e.g., VerificationMethod.emailLink.rawValue) when checking Clerk.shared.environment?.userSettings.attributes["email_address"]?.verifications so the contains check becomes contains(VerificationMethod.emailLink.rawValue) (or, if verifications are strongly typed, compare to VerificationMethod.emailLink directly).Tests/Domains/Auth/SignIn/SignInTests.swift (1)
69-73: Consider reusing a helper for dependency+configuration setup in this test.This block bypasses the file’s existing setup helper path; centralizing setup will make the redirect/option assumptions less brittle over time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Domains/Auth/SignIn/SignInTests.swift` around lines 69 - 73, Replace the direct assignment to Clerk.shared.dependencies (the MockDependencyContainer construction using createMockAPIClient(), keychain, and signInService: service) with the file’s existing dependency/configuration setup helper used by other tests (call the helper that centralizes dependency wiring and pass the same createMockAPIClient(), keychain, and service values), so tests reuse the common setup path instead of instantiating MockDependencyContainer inline.Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift (1)
231-241: Consider edge case: both URL schemes unavailable on simulator.The
openEmailApp()function triesmessage://andmailto:but on iOS Simulator neither may be available, which would always show an error. This is acceptable for production but may cause confusion during development/testing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift` around lines 231 - 241, openEmailApp currently sets an error if neither "message://" nor "mailto:" can be opened (common on Simulator); update openEmailApp to handle this edge case by providing a graceful fallback: if both schemes fail, copy a mailto link or support email address to the clipboard via UIPasteboard.general and present a user-facing fallback (e.g., an alert or in-view message) with a "Paste/Copy" action so developers/testers can manually open mail in the Simulator; keep the existing ClerkClientError assignment for production but invoke the fallback before setting error to improve developer UX.Sources/ClerkKitUI/Extensions/SignIn+Ext.swift (1)
42-47: Consider preserving optionality semantics.Defaulting to
[]whensupportedFirstFactorsisnilconflates "no factors available" with "factors not yet loaded." While pragmatic for UI rendering (no options to show in either case), this loses semantic distinction that could be useful for showing loading states vs empty states.If the distinction matters in calling code, consider keeping this as
[Factor]?:- var availableFirstFactors: [Factor] { - supportedFirstFactors?.filter { factor in + var availableFirstFactors: [Factor]? { + supportedFirstFactors?.filter { factor in if case .unknown = factor.strategy { return false } return true - } ?? [] + } }However, if all consumers already handle nil appropriately upstream, the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKitUI/Extensions/SignIn`+Ext.swift around lines 42 - 47, The computed property availableFirstFactors currently returns a non-optional [Factor] by defaulting nil supportedFirstFactors to [], which loses the "not yet loaded" vs "empty" distinction; change availableFirstFactors's signature to return [Factor]? and remove the "?? []" so it simply maps supportedFirstFactors?.filter { if case .unknown = $0.strategy { return false } ; return true }, and update any callers of availableFirstFactors to handle the optional (e.g., show loading state when nil and empty-state when []).Sources/ClerkKit/Domains/Auth/MagicLink.swift (1)
87-127: Synchronous keychain I/O on main actor may block UI.
MagicLinkStoreis marked@MainActorbutKeychainStorageoperations (viaSystemKeychain) perform synchronous calls to Security framework APIs (SecItemAdd,SecItemCopyMatching, etc.). These can block the main thread, potentially causing UI hitches—especially during first unlock or under disk pressure.Consider one of these approaches:
- Move keychain operations off the main actor using
Task.detachedor a dedicated actor- Accept this trade-off if the operations are consistently fast in practice
Also,
save()throws butload()silently swallows errors withtry?. Consider logging decode failures for debugging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKit/Domains/Auth/MagicLink.swift` around lines 87 - 127, MagicLinkStore is annotated `@MainActor` but performs synchronous keychain I/O via the keychain property (any KeychainStorage), which can block the UI; move those operations off the main actor by performing keychain accesses from a background context (e.g., wrap calls in Task.detached or implement a dedicated KeychainActor) and update save(), load(), and clear() to call into that background execution so the main thread isn’t blocked. Also change load() to surface or log decoding/keychain errors instead of silently swallowing them with try? — capture the thrown error from keychain.data(...) and JSONDecoder.clerkDecoder.decode(...) and log or rethrow it for diagnostics while still handling expiry logic for PendingMagicLinkFlow.Tests/Domains/Auth/AuthTests.swift (1)
283-343: Thorough callback completion test with minor inconsistency.The test correctly validates the magic link callback flow including HTTP request verification, session activation, and keychain cleanup.
One minor inconsistency: Line 342 uses
Clerk.shared.dependencies.keychainwhile the rest of the test uses the localkeychainvariable. They should be the same reference afterconfigureDependencies, but using the local variable would be more consistent:- `#expect`(try (Clerk.shared.dependencies.keychain.hasItem(forKey: ClerkKeychainKey.pendingMagicLinkFlow.rawValue)) == false) + `#expect`(try keychain.hasItem(forKey: ClerkKeychainKey.pendingMagicLinkFlow.rawValue) == false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Domains/Auth/AuthTests.swift` around lines 283 - 343, In handleMagicLinkCallbackCompletesPendingFlowAndActivatesSession, the final assertion references Clerk.shared.dependencies.keychain but the test uses the local keychain variable; replace Clerk.shared.dependencies.keychain with the local keychain and call keychain.hasItem(forKey: ClerkKeychainKey.pendingMagicLinkFlow.rawValue) to assert the item was removed, keeping the rest of the check the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ClerkKit/Core/Auth.swift`:
- Around line 324-386: The docstring for completeMagicLink claims it may
complete as sign-up or sign-in but the implementation always calls
signInWithTicket and returns .signIn(signIn); either implement sign-up routing
or update docs: if sign-up should be supported, add logic after receiving
MagicLinkCompleteResponse (or inspect completionResponse.type/fields) to call
the appropriate sign-up flow and return .signUp(...) (or a
TransferFlowResult.signUp case), otherwise change the comment text to state it
only completes sign-in and remove references to sign-up so the docstring matches
the behavior of completeMagicLink, signInWithTicket, and the returned
TransferFlowResult.
In `@Sources/ClerkKit/Domains/Auth/FactorStrategy.swift`:
- Line 16: Add explicit handling for the new FactorStrategy.emailLink case
wherever the code switches on the factor strategy: update the switch in
SignInClientTrustView (the switch that currently falls through to GetHelpView)
to include a .emailLink branch that routes to the proper factor view for
magic-link UX; and update the computed properties actionText and iconName in
SignInFactorOneForgotPasswordView to include a .emailLink case returning the
appropriate label and icon (instead of nil). Ensure you reference the
FactorStrategy.emailLink enum case in each switch so the magic-link flow renders
correct text and icon.
In `@Sources/ClerkKit/Domains/Auth/MagicLink.swift`:
- Around line 45-48: canHandle(_:) currently returns true when either
queryParam(named: "flow_id", in: url) or queryParam(named: "approval_token", in:
url) exists, but init(url:) requires both values and will fail if one is
missing; change canHandle to require both params (use AND logic) so it only
returns true when queryParam(named: "flow_id", in: url) != nil &&
queryParam(named: "approval_token", in: url) != nil, ensuring callers routed by
canHandle won't hit init(url:) errors and update any tests or callers that
relied on the looser behavior of canHandle(_:).
In `@Sources/ClerkKit/Domains/Auth/SignIn/SignIn.swift`:
- Around line 135-148: The verifier is saved after the network call, causing a
race if the magic link is used quickly; move the
MagicLinkStore.save(codeVerifier:) call to occur immediately after obtaining
pkcePair (from MagicLinkPKCE.generatePair()) and before awaiting
signInService.prepareFirstFactor(...); ensure you still return the signIn result
and keep error behavior unchanged if prepareFirstFactor fails (orphaned verifier
TTL is acceptable).
In `@Sources/ClerkKit/Domains/Auth/SignUp/SignUp.swift`:
- Around line 171-184: The PKCE verifier is saved after calling
signUpService.prepareVerification, which can leave the server-side flow created
without a local verifier if keychain/save fails; move the call to
MagicLinkStore.save(codeVerifier: pkcePair.verifier) to occur immediately after
creating pkcePair (i.e., after MagicLinkPKCE.generatePair() and before calling
signUpService.prepareVerification(...)) and ensure any save error is
propagated/stops the flow so prepareVerification is not invoked without a
persisted verifier.
In
`@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift`:
- Around line 225-228: The catch block in sendLink() updates deliveryState to
.failed(error) but never assigns the thrown error to the view's error property,
so the UI (which uses .clerkErrorPresenting($error)) won't show it; modify the
catch in sendLink() to also set self.error = error (or call a helper that sets
both deliveryState and error) so the error is surfaced, while keeping the
ClerkLogger.error("Failed to send email link", error: error) call intact.
In `@Sources/ClerkKitUI/Resources/Localizable.xcstrings`:
- Around line 3998-4001: The new localization entries for the email-link UI
(e.g., the key "Didn't receive an email?" and the other affected keys around the
identical pattern at the other reported block) only contain metadata and no
localized values, so add proper localized payloads under the "localizations"
dictionary for each locale needed (or add an explicit "nonLocalized" marker if
the string must remain untranslated); update the Localizable.xcstrings entries
to include per-locale mappings for the keys (preserving the existing
"comment"/"isCommentAutoGenerated" fields) so non-English users see the correct
copy rather than the English fallback.
- Around line 7303-7308: The two string-catalog records for "No email app is
available on this device." and "Open email app" are empty objects; replace each
empty entry with the full string-catalog structure (include metadata and
per-locale values) so localization tooling picks them up — e.g., add the
standard keys (NSStringLocalizedFormatKey and a nested value table with
NSStringFormatValueTypeKey and an "en" (and other locales as needed)
localization) for both the "No email app is available on this device." and "Open
email app" entries and populate the English/default text.
In `@Tests/UI/SignInFactorSelectionTests.swift`:
- Around line 9-11: SignInFactorSelectionTests in the ClerkKitUITests target
cannot access configureClerkForTesting (from
Tests/TestSupport/TestHelpers.swift) because that file and its Mocker dependency
are only in the ClerkKitTests target; to fix, create a lightweight test
utilities file inside Tests/UI (e.g., UI/TestHelpers.swift) that defines
configureClerkForTesting() and the minimal InMemoryKeychain replacement used by
SignInFactorSelectionTests without importing Mocker, update
SignInFactorSelectionTests.swift to import/use these local helpers, and keep the
original TestHelpers.swift intact for ClerkKitTests; alternatively if you prefer
sharing the existing helpers, move TestHelpers.swift and InMemoryKeychain.swift
into Tests/UI or add Mocker and include Tests/TestSupport/ in the
ClerkKitUITests target in Package.swift so the UITests target can access them.
In `@Tests/UI/SignUpEmailVerificationStrategyTests.swift`:
- Around line 9-11: The compile error is because configureClerkForTesting() is
not visible to the ClerkKitUITests target; fix it by placing the helper into a
source file that is compiled into the ClerkKitUITests target (e.g., a shared
test helper in the Tests target) or by making the helper visible to tests (mark
it public/internal in its module and use `@testable` import ClerkKit in the UI
test target). Update or add the helper so
SignUpEmailVerificationStrategyTests.swift can call configureClerkForTesting()
directly; ensure the symbol name matches exactly and the file is included in the
ClerkKitUITests target.
---
Nitpick comments:
In `@Sources/ClerkKit/Domains/Auth/MagicLink.swift`:
- Around line 87-127: MagicLinkStore is annotated `@MainActor` but performs
synchronous keychain I/O via the keychain property (any KeychainStorage), which
can block the UI; move those operations off the main actor by performing
keychain accesses from a background context (e.g., wrap calls in Task.detached
or implement a dedicated KeychainActor) and update save(), load(), and clear()
to call into that background execution so the main thread isn’t blocked. Also
change load() to surface or log decoding/keychain errors instead of silently
swallowing them with try? — capture the thrown error from keychain.data(...) and
JSONDecoder.clerkDecoder.decode(...) and log or rethrow it for diagnostics while
still handling expiry logic for PendingMagicLinkFlow.
In
`@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift`:
- Around line 231-241: openEmailApp currently sets an error if neither
"message://" nor "mailto:" can be opened (common on Simulator); update
openEmailApp to handle this edge case by providing a graceful fallback: if both
schemes fail, copy a mailto link or support email address to the clipboard via
UIPasteboard.general and present a user-facing fallback (e.g., an alert or
in-view message) with a "Paste/Copy" action so developers/testers can manually
open mail in the Simulator; keep the existing ClerkClientError assignment for
production but invoke the fallback before setting error to improve developer UX.
In `@Sources/ClerkKitUI/Extensions/SignIn`+Ext.swift:
- Around line 42-47: The computed property availableFirstFactors currently
returns a non-optional [Factor] by defaulting nil supportedFirstFactors to [],
which loses the "not yet loaded" vs "empty" distinction; change
availableFirstFactors's signature to return [Factor]? and remove the "?? []" so
it simply maps supportedFirstFactors?.filter { if case .unknown = $0.strategy {
return false } ; return true }, and update any callers of availableFirstFactors
to handle the optional (e.g., show loading state when nil and empty-state when
[]).
In `@Sources/ClerkKitUI/Extensions/SignUp`+Ext.swift:
- Around line 26-27: The code is matching the verification type using the
hard-coded string "email_link"; replace this literal with the enum raw value to
avoid drift—use the existing verification enum (e.g.,
VerificationMethod.emailLink.rawValue) when checking
Clerk.shared.environment?.userSettings.attributes["email_address"]?.verifications
so the contains check becomes contains(VerificationMethod.emailLink.rawValue)
(or, if verifications are strongly typed, compare to
VerificationMethod.emailLink directly).
In `@Tests/Domains/Auth/AuthTests.swift`:
- Around line 283-343: In
handleMagicLinkCallbackCompletesPendingFlowAndActivatesSession, the final
assertion references Clerk.shared.dependencies.keychain but the test uses the
local keychain variable; replace Clerk.shared.dependencies.keychain with the
local keychain and call keychain.hasItem(forKey:
ClerkKeychainKey.pendingMagicLinkFlow.rawValue) to assert the item was removed,
keeping the rest of the check the same.
In `@Tests/Domains/Auth/SignIn/SignInTests.swift`:
- Around line 69-73: Replace the direct assignment to Clerk.shared.dependencies
(the MockDependencyContainer construction using createMockAPIClient(), keychain,
and signInService: service) with the file’s existing dependency/configuration
setup helper used by other tests (call the helper that centralizes dependency
wiring and pass the same createMockAPIClient(), keychain, and service values),
so tests reuse the common setup path instead of instantiating
MockDependencyContainer inline.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 72f90154-36a3-4f07-9f1a-f4df55cd2e8f
📒 Files selected for processing (31)
Examples/Quickstart/Quickstart/Info.plistSources/ClerkKit/Core/Auth.swiftSources/ClerkKit/Core/Clerk.swiftSources/ClerkKit/Core/ClerkURLRoute.swiftSources/ClerkKit/Core/URLHandlingCoordinator.swiftSources/ClerkKit/Domains/Auth/FactorStrategy.swiftSources/ClerkKit/Domains/Auth/MagicLink.swiftSources/ClerkKit/Domains/Auth/SignIn/SignIn+ServiceParams.swiftSources/ClerkKit/Domains/Auth/SignIn/SignIn.swiftSources/ClerkKit/Domains/Auth/SignUp/SignUp+ServiceParams.swiftSources/ClerkKit/Domains/Auth/SignUp/SignUp.swiftSources/ClerkKit/Mocks/MockModels.swiftSources/ClerkKit/Storage/Keychain/ClerkKeychainKey.swiftSources/ClerkKitUI/Components/Auth/AuthNavigation.swiftSources/ClerkKitUI/Components/Auth/AuthView.swiftSources/ClerkKitUI/Components/Auth/SignIn/SignInFactorAlternativeMethodsView.swiftSources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swiftSources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneView.swiftSources/ClerkKitUI/Extensions/Factor+Sorting.swiftSources/ClerkKitUI/Extensions/SignIn+Ext.swiftSources/ClerkKitUI/Extensions/SignUp+Ext.swiftSources/ClerkKitUI/Resources/Localizable.xcstringsTests/Core/ClerkTests.swiftTests/Domains/Auth/AuthTests.swiftTests/Domains/Auth/MagicLinkTests.swiftTests/Domains/Auth/SignIn/SignInServiceTests.swiftTests/Domains/Auth/SignIn/SignInTests.swiftTests/Domains/Auth/SignUp/SignUpTests.swiftTests/Storage/ClerkKeychainKeyTests.swiftTests/UI/SignInFactorSelectionTests.swiftTests/UI/SignUpEmailVerificationStrategyTests.swift
- Change URL validation to require both flow_id AND approval_token - Save PKCE code verifier before API call to avoid loss on failure - Use FactorStrategy enum instead of hardcoded string literal - Remove outdated doc comment about sign-in/sign-up determination
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ClerkKit/Domains/Auth/SignIn/SignIn.swift`:
- Around line 105-148: The current sendEmailLink flow saves a PKCE verifier via
MagicLinkStore.save which will overwrite any existing verifier and can orphan a
concurrent magic-link flow; update sendEmailLink to first check for an existing
unexpired verifier (via MagicLinkStore.hasUnexpiredVerifier or similar) and
either prevent/start a new flow accordingly or extend MagicLinkStore to expose a
method to atomically save-if-none or save-with-unique-key, or alternatively add
documentation in SignIn.sendEmailLink and MagicLinkStore explaining that only
one pending magic-link flow is supported; reference SendEmailLink
(sendEmailLink), MagicLinkStore.save, and the MagicLink store implementation to
implement the check-or-document fix.
In `@Tests/UI/TestHelpers.swift`:
- Around line 13-16: The configureClerkForTesting function currently calls
Clerk.configure(publishableKey: testPublishableKey) before
Clerk.shared.cleanupManagers(), which tears down managers created by configure
and can leave Clerk.shared partially initialized; swap the calls so you invoke
Clerk.shared.cleanupManagers() first to ensure a clean slate, then call
Clerk.configure(...) to create managers afresh (reference: function
configureClerkForTesting, Clerk.configure, Clerk.shared.cleanupManagers).
- Around line 8-11: Update the doc comment that begins with "Configures Clerk
for UI-focused unit tests." to remove historical/contextual reasoning (e.g.
"These tests only..." and "so they do not need...") and leave a concise,
behavior-focused description of what the helper does and how it should be used;
keep the summary statement and any usage or parameter notes, but strip any
past-setup rationale or history from the comment above the TestHelpers
configurator.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8f2bd400-fdef-4978-8fe5-2a0ab39dc777
📒 Files selected for processing (7)
Sources/ClerkKit/Core/Auth.swiftSources/ClerkKit/Domains/Auth/MagicLink.swiftSources/ClerkKit/Domains/Auth/SignIn/SignIn.swiftSources/ClerkKit/Domains/Auth/SignUp/SignUp.swiftSources/ClerkKitUI/Extensions/SignUp+Ext.swiftSources/ClerkKitUI/Resources/Localizable.xcstringsTests/UI/TestHelpers.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/ClerkKitUI/Extensions/SignUp+Ext.swift
- Sources/ClerkKitUI/Resources/Localizable.xcstrings
- Sources/ClerkKit/Domains/Auth/MagicLink.swift
Remove TransferFlowResult wrapper from handleMagicLinkCallback and completeMagicLink since sign-up magic links are handled separately. Add documentation for single-flow storage semantics, include email link factor in forgot password view, and set error state on send failure. Simplify test helpers by removing unnecessary cleanup call.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Tests/Domains/Auth/AuthTests.swift (1)
239-339: Please add regression coverage for duplicate/stale callbacks too.The happy path is covered well, but the risky branches are still missing: the same callback arriving twice, and an older callback returning
pkce_verification_failedwhile a newer verifier is stored. Those cases exercise the dedup and cleanup behavior most likely to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Domains/Auth/AuthTests.swift` around lines 239 - 339, Add two new tests around Clerk.shared.auth.handleMagicLinkCallback to cover duplicate and stale callbacks: (1) duplicate callback — simulate the completion endpoint returning the same MagicLinkCompleteResponse twice and call handleMagicLinkCallback twice; assert MockSignInService.create is only invoked once, MockSessionService.setActive only activates one session (use signInParams and activatedSessionId locks), and the pending magic link key (ClerkKeychainKey.pendingMagicLinkFlow) is cleared after the first/second call. (2) stale PKCE callback — save a different verifier with MagicLinkStore.save prior to invoking handleMagicLinkCallback, mock the completion endpoint to return an error body with "pkce_verification_failed" for the older flow, and assert that no session is activated, signInParams.ticket is not set (or create not called), and the pending magic link flow is cleaned up appropriately. Use existing helpers (MockSignInService, MockSessionService, Mock for completionUrl, MagicLinkStore.save) and mirror the structure/assertions from signInWithEmailLinkCreatesAndPreparesFirstFactor and handleMagicLinkCallbackCompletesPendingFlowAndActivatesSession tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ClerkKit/Core/Auth.swift`:
- Around line 313-321: The handleMagicLinkCallback(_:) must go through the same
deduplication coordinator used by Clerk.handle(_:) to avoid duplicate
processing: instead of directly calling completeMagicLink(flowId:approvalToken:)
after creating MagicLinkCallback, build the equivalent .magicLink Route (or call
the existing Clerk.handle(route:) path) and pass it into the dedup coordinator /
Clerk.handle so deduplication runs; once the routed work completes, return the
resulting SignIn (or forward the completion result from completeMagicLink) and
do not bypass the coordinator that prevents duplicated handling of the same
flowId/approvalToken pair.
In `@Sources/ClerkKit/Domains/Auth/MagicLink.swift`:
- Around line 22-26: The current completeMagicLink path unconditionally clears
the stored PendingMagicLinkFlow when a PKCE failure occurs, but
PendingMagicLinkFlow has no flow_id so a stale callback can wipe out a newer
valid pending flow; update completeMagicLink so that it does NOT remove or
overwrite the stored PendingMagicLinkFlow on a pkce_verification_failed
error—only remove the pending flow on successful completion or on explicit
terminal errors that are known to apply to the current flow; keep
PendingMagicLinkFlow, completeMagicLink, and the pkce_verification_failed check
as the points to change.
---
Nitpick comments:
In `@Tests/Domains/Auth/AuthTests.swift`:
- Around line 239-339: Add two new tests around
Clerk.shared.auth.handleMagicLinkCallback to cover duplicate and stale
callbacks: (1) duplicate callback — simulate the completion endpoint returning
the same MagicLinkCompleteResponse twice and call handleMagicLinkCallback twice;
assert MockSignInService.create is only invoked once,
MockSessionService.setActive only activates one session (use signInParams and
activatedSessionId locks), and the pending magic link key
(ClerkKeychainKey.pendingMagicLinkFlow) is cleared after the first/second call.
(2) stale PKCE callback — save a different verifier with MagicLinkStore.save
prior to invoking handleMagicLinkCallback, mock the completion endpoint to
return an error body with "pkce_verification_failed" for the older flow, and
assert that no session is activated, signInParams.ticket is not set (or create
not called), and the pending magic link flow is cleaned up appropriately. Use
existing helpers (MockSignInService, MockSessionService, Mock for completionUrl,
MagicLinkStore.save) and mirror the structure/assertions from
signInWithEmailLinkCreatesAndPreparesFirstFactor and
handleMagicLinkCallbackCompletesPendingFlowAndActivatesSession tests.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 44ac057c-fa0a-4c89-afc6-277af0363dce
📒 Files selected for processing (7)
Sources/ClerkKit/Core/Auth.swiftSources/ClerkKit/Domains/Auth/MagicLink.swiftSources/ClerkKit/Domains/Auth/SignIn/SignIn.swiftSources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swiftSources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneForgotPasswordView.swiftTests/Domains/Auth/AuthTests.swiftTests/UI/TestHelpers.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/ClerkKit/Domains/Auth/SignIn/SignIn.swift
- Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift
Convert MagicLinkStore from a static enum to an injectable class, enabling isolated test instances and proper unit testing. Update Auth, SignIn, and SignUp to use the injected store via Dependencies. Also add deduplication for concurrent handleMagicLinkCallback calls and preserve pending flow state when PKCE verification fails due to stale callbacks, allowing fresh magic links to still complete.
The magic link tests were sharing mockBaseUrl, which caused mock URL collisions when tests ran in parallel. Each test now uses a distinct base URL to ensure proper mock isolation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/ClerkKit/Core/Auth.swift (1)
371-375:⚠️ Potential issue | 🟠 MajorDon't clear the stored verifier on callback-specific API errors.
PendingMagicLinkFlowstill has noflowIdinSources/ClerkKit/Domains/Auth/MagicLink.swift:22-26, so this block cannot tell whetherapproval_token_expired,approval_token_consumed,approval_token_invalid, orflow_not_approvedbelongs to the currently stored flow or to an older link the user opened later. In that stale-link case, clearing here deletes the verifier for the newer valid link and makes that callback unrecoverable. Until the pending flow is bound to aflowId, the only safe automatic cleanup is on success.🐛 Smallest safe change
- } catch let error as ClerkAPIError { - if magicLinkTerminalErrorCodes.contains(error.code), error.code != "pkce_verification_failed" { - magicLinkStore.clear() - } - throw error - } + } catch { + throw error + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKit/Core/Auth.swift` around lines 371 - 375, The catch block in Auth.swift currently calls magicLinkStore.clear() for certain ClerkAPIError codes (checking magicLinkTerminalErrorCodes and excluding "pkce_verification_failed"), which can mistakenly remove the verifier for a newer pending flow; remove that automatic clear from this catch so callback-specific errors (e.g., approval_token_* and flow_not_approved) do not clear the stored verifier. Ensure magicLinkStore.clear() is only invoked on successful completion of the magic-link flow (or after the pending flow has been bound to a flowId), and keep references to magicLinkTerminalErrorCodes, the "pkce_verification_failed" string, and magicLinkStore.clear() to locate and update the code; do not clear in the ClerkAPIError catch that handles callback errors.
🧹 Nitpick comments (1)
Tests/Domains/Auth/AuthTests.swift (1)
289-290: Minor inconsistency in URL construction.Line 289 uses
try#require(URL(string:))while line 290 uses force-unwrap. Both approaches work for known-valid test URLs, but consider using the same pattern consistently.Suggested consistency fix
let callbackUrl = try `#require`(URL(string: "com.clerk.Quickstart://callback?flow_id=flow_123&approval_token=approval_123")) - let completionUrl = URL(string: mockBaseUrl.absoluteString + "/v1/client/magic_links/complete")! + let completionUrl = try `#require`(URL(string: mockBaseUrl.absoluteString + "/v1/client/magic_links/complete"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Domains/Auth/AuthTests.swift` around lines 289 - 290, The two test URL initializations are inconsistent: callbackUrl uses the test helper pattern try `#require`(URL(string: ...)) while completionUrl is force-unwrapped; make them consistent by using the same construction pattern for both variables (e.g., change completionUrl to use try `#require`(URL(string: mockBaseUrl.absoluteString + "/v1/client/magic_links/complete")) or change callbackUrl to a force-unwrap) so update the completionUrl or callbackUrl declaration accordingly in Tests/Domains/Auth/AuthTests.swift (referencing the callbackUrl and completionUrl variables).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ClerkKit/Domains/Auth/MagicLink.swift`:
- Around line 45-48: The canHandle(_:) matcher in MagicLink.swift currently
returns true solely based on presence of query params via queryParam(named:in:),
which lets any URL with those params be treated as a Clerk callback; update
canHandle(_:) to also validate the URL's scheme/host/path against the configured
redirect callback (or accept an expected callback URL as a parameter) before
returning true—e.g., compare url.scheme, url.host and url.path (or compare to a
preconfigured redirect URL) in MagicLink.canHandle(_:) so only legitimate
redirect URLs claiming flow_id and approval_token are accepted, or move this
logic into an API that receives the expected callback target and performs both
param and origin checks.
In `@Sources/ClerkKitUI/Resources/Localizable.xcstrings`:
- Around line 4492-4563: The new localization key "Email link to %@" is not used
— SignInFactorOneForgotPasswordView.swift currently builds the label with "Email
link to \(safeIdentifier)" directly; update that construction to fetch the
localized template and format it with safeIdentifier (e.g. use
NSLocalizedString("Email link to %@", comment: ...) or the String(localized:...)
API) so the UI uses the Localizable.xcstrings entry instead of a hardcoded
English string; change the string construction in
SignInFactorOneForgotPasswordView (the code that currently builds the email link
label around safeIdentifier) to use the localization key and pass safeIdentifier
as the argument.
---
Duplicate comments:
In `@Sources/ClerkKit/Core/Auth.swift`:
- Around line 371-375: The catch block in Auth.swift currently calls
magicLinkStore.clear() for certain ClerkAPIError codes (checking
magicLinkTerminalErrorCodes and excluding "pkce_verification_failed"), which can
mistakenly remove the verifier for a newer pending flow; remove that automatic
clear from this catch so callback-specific errors (e.g., approval_token_* and
flow_not_approved) do not clear the stored verifier. Ensure
magicLinkStore.clear() is only invoked on successful completion of the
magic-link flow (or after the pending flow has been bound to a flowId), and keep
references to magicLinkTerminalErrorCodes, the "pkce_verification_failed"
string, and magicLinkStore.clear() to locate and update the code; do not clear
in the ClerkAPIError catch that handles callback errors.
---
Nitpick comments:
In `@Tests/Domains/Auth/AuthTests.swift`:
- Around line 289-290: The two test URL initializations are inconsistent:
callbackUrl uses the test helper pattern try `#require`(URL(string: ...)) while
completionUrl is force-unwrapped; make them consistent by using the same
construction pattern for both variables (e.g., change completionUrl to use try
`#require`(URL(string: mockBaseUrl.absoluteString +
"/v1/client/magic_links/complete")) or change callbackUrl to a force-unwrap) so
update the completionUrl or callbackUrl declaration accordingly in
Tests/Domains/Auth/AuthTests.swift (referencing the callbackUrl and
completionUrl variables).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 883587d4-1da3-43a5-868a-cd64ad1ce12c
📒 Files selected for processing (12)
Sources/ClerkKit/Core/Auth.swiftSources/ClerkKit/Core/Clerk.swiftSources/ClerkKit/Core/URLHandlingCoordinator.swiftSources/ClerkKit/Dependencies/Dependencies.swiftSources/ClerkKit/Dependencies/DependencyContainer.swiftSources/ClerkKit/Domains/Auth/MagicLink.swiftSources/ClerkKit/Domains/Auth/SignIn/SignIn.swiftSources/ClerkKit/Domains/Auth/SignUp/SignUp.swiftSources/ClerkKit/Mocks/MockDependencyContainer.swiftSources/ClerkKitUI/Resources/Localizable.xcstringsTests/Core/ClerkTests.swiftTests/Domains/Auth/AuthTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/ClerkKit/Core/URLHandlingCoordinator.swift
- Sources/ClerkKit/Domains/Auth/SignIn/SignIn.swift
Validate that incoming callback URLs match the configured redirect URL before processing magic links. This prevents handling callbacks from mismatched origins. - Add redirectUrl dependency to Auth for URL validation - Introduce matchesRedirectUrl helper to compare scheme, host, and path - Move URL routing logic from ClerkURLRoute init to Clerk.handle - Make canHandleMagicLinkCallback internal - Remove terminal error code clearing (already handled elsewhere) - Add test for rejecting mismatched callback origins
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorAlternativeMethodsView.swift (2)
59-72: Code duplication:identifierActionText(factor:)is identical in both files.This helper function is duplicated verbatim in
SignInFactorOneForgotPasswordView.swift(lines 49-62). Consider extracting it to a shared extension onFactoror a common utility to avoid maintaining the same logic in multiple places.♻️ Suggested shared extension
// In a shared file, e.g., Factor+ActionText.swift extension Factor { func identifierActionText() -> LocalizedStringKey? { guard let safeIdentifier = safeIdentifier else { return nil } switch strategy { case .phoneCode: return "Send SMS code to \(safeIdentifier.formattedAsPhoneNumberIfPossible)" case .emailCode: return "Email code to \(safeIdentifier)" case .emailLink: return "Email link to \(safeIdentifier)" default: return nil } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorAlternativeMethodsView.swift` around lines 59 - 72, The function identifierActionText(factor:) is duplicated; extract this logic into a shared extension on Factor (e.g., add an instance method Factor.identifierActionText() or static utility) and replace the duplicated implementations in SignInFactorAlternativeMethodsView and SignInFactorOneForgotPasswordView to call the new Factor.identifierActionText() method; ensure the method uses factor.safeIdentifier and factor.strategy and returns the same LocalizedStringKey values (including formattedAsPhoneNumberIfPossible for .phoneCode) so behavior remains identical.
80-83: Minor: Combine.emailCodeand.emailLinkcases.These cases return the same icon. For consistency with
SignInFactorOneForgotPasswordView.swift(line 70), combine them:♻️ Suggested fix
- case .emailCode: - "icon-email" - case .emailLink: - "icon-email" + case .emailCode, .emailLink: + "icon-email"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorAlternativeMethodsView.swift` around lines 80 - 83, In SignInFactorAlternativeMethodsView update the switch that handles the factor cases to combine the duplicate branches for .emailCode and .emailLink into a single branch (e.g., use a single case handling both .emailCode and .emailLink) so they return the same "icon-email" value, matching the pattern used in SignInFactorOneForgotPasswordView and removing the redundant duplicate case.Sources/ClerkKit/Core/Auth.swift (1)
370-376: Redundant catch block.This
catch let error as ClerkAPIErrorblock immediately rethrows, which is identical to not catching at all. If you intend to add terminal-error handling here (e.g., clearingmagicLinkStoreon specific error codes), implement it; otherwise remove the catch.♻️ Suggested simplification
do { completionResponse = try await apiClient.send(request).value magicLinkStore.clear() - } catch let error as ClerkAPIError { - throw error }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKit/Core/Auth.swift` around lines 370 - 376, The catch block that matches `ClerkAPIError` and immediately rethrows is redundant; in the `Auth.swift` function where you await `apiClient.send(request).value` into `completionResponse: MagicLinkCompleteResponse` and call `magicLinkStore.clear()`, either remove the `catch let error as ClerkAPIError { throw error }` entirely so normal propagation happens, or if you need custom handling (e.g., clear `magicLinkStore` for specific error codes), replace that catch with logic that inspects the `ClerkAPIError` and performs the conditional cleanup before rethrowing; reference `completionResponse`, `apiClient.send(request).value`, `magicLinkStore.clear()`, and `ClerkAPIError` to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/ClerkKit/Core/Auth.swift`:
- Around line 370-376: The catch block that matches `ClerkAPIError` and
immediately rethrows is redundant; in the `Auth.swift` function where you await
`apiClient.send(request).value` into `completionResponse:
MagicLinkCompleteResponse` and call `magicLinkStore.clear()`, either remove the
`catch let error as ClerkAPIError { throw error }` entirely so normal
propagation happens, or if you need custom handling (e.g., clear
`magicLinkStore` for specific error codes), replace that catch with logic that
inspects the `ClerkAPIError` and performs the conditional cleanup before
rethrowing; reference `completionResponse`, `apiClient.send(request).value`,
`magicLinkStore.clear()`, and `ClerkAPIError` to locate the code to change.
In
`@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorAlternativeMethodsView.swift`:
- Around line 59-72: The function identifierActionText(factor:) is duplicated;
extract this logic into a shared extension on Factor (e.g., add an instance
method Factor.identifierActionText() or static utility) and replace the
duplicated implementations in SignInFactorAlternativeMethodsView and
SignInFactorOneForgotPasswordView to call the new Factor.identifierActionText()
method; ensure the method uses factor.safeIdentifier and factor.strategy and
returns the same LocalizedStringKey values (including
formattedAsPhoneNumberIfPossible for .phoneCode) so behavior remains identical.
- Around line 80-83: In SignInFactorAlternativeMethodsView update the switch
that handles the factor cases to combine the duplicate branches for .emailCode
and .emailLink into a single branch (e.g., use a single case handling both
.emailCode and .emailLink) so they return the same "icon-email" value, matching
the pattern used in SignInFactorOneForgotPasswordView and removing the redundant
duplicate case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a706481-19af-4fdd-8aae-b570c4e712ec
📒 Files selected for processing (10)
Sources/ClerkKit/Core/Auth.swiftSources/ClerkKit/Core/Clerk.swiftSources/ClerkKit/Core/ClerkURLRoute.swiftSources/ClerkKit/Domains/Auth/MagicLink.swiftSources/ClerkKitUI/Components/Auth/SignIn/SignInFactorAlternativeMethodsView.swiftSources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneForgotPasswordView.swiftTests/Core/ClerkTests.swiftTests/Domains/Auth/AuthTests.swiftTests/Domains/Auth/MagicLinkTests.swiftTests/TestSupport/TestHelpers.swift
✅ Files skipped from review due to trivial changes (1)
- Tests/Domains/Auth/MagicLinkTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Tests/TestSupport/TestHelpers.swift
- Tests/Domains/Auth/AuthTests.swift
- Tests/Core/ClerkTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/ClerkKit/Core/Auth.swift (2)
344-374:⚠️ Potential issue | 🟠 MajorRoute the public
completeMagicLinkentry point throughurlHandlingCoordinator.Deduplication only happens in
handle(_:). BecauseAuthis@MainActor, a second task can still entercompleteMagicLink(flowId:approvalToken:)after the first suspends on the network await, so two direct callers can load the same verifier and race the same approval token. Move the current body into a private helper and have the public method delegate throughhandle(.magicLink(...)).Also applies to: 389-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKit/Core/Auth.swift` around lines 344 - 374, The public completeMagicLink(flowId:approvalToken:) must be routed through urlHandlingCoordinator to benefit from deduplication in handle(_:); refactor by extracting the current body (validation, magicLinkStore.load(), creating Request<MagicLinkCompleteResponse>, clearing store, and calling signInWithTicket) into a private helper (e.g., private func completeMagicLinkImpl(flowId:approvalToken:) async throws -> SignIn) and change the public method to simply await urlHandlingCoordinator.handle(.magicLink(flowId: resolvedFlowId, approvalToken: resolvedApprovalToken)) so the call path goes through handle(_:) and avoids concurrent races on magicLinkStore and codeVerifier. Ensure the private helper name and the public completeMagicLink signature are referenced consistently and preserve existing error behavior.
321-333:⚠️ Potential issue | 🟠 MajorReject non-matching URLs inside the public magic-link handler.
handleMagicLinkCallback(_:)skips the redirect-origin check and will process any URL that happens to containflow_idandapproval_token. SincecanHandleMagicLinkCallback(_:)is internal, callers of the public API cannot reuse that guard themselves. Validate the URL here before parsing it.🛡️ Small fix
`@discardableResult` public func handleMagicLinkCallback(_ url: URL) async throws -> SignIn { + guard canHandleMagicLinkCallback(url) else { + throw ClerkClientError(message: "URL does not match the configured magic link redirect.") + } let callback = try MagicLinkCallback(url: url) return try await handle(.magicLink(flowId: callback.flowId, approvalToken: callback.approvalToken)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKit/Core/Auth.swift` around lines 321 - 333, Public handler handleMagicLinkCallback(_:) currently parses any URL with flow_id and approval_token; add a redirect-origin check first to reject non-matching URLs. Inside handleMagicLinkCallback(_ url: URL), call matchesRedirectUrl(url) (or reuse canHandleMagicLinkCallback(url)) and throw an appropriate error if it returns false before creating MagicLinkCallback(url), then proceed with extracting flowId/approvalToken and calling handle(.magicLink(...)); reference the existing methods handleMagicLinkCallback(_:), matchesRedirectUrl(_:), canHandleMagicLinkCallback(_:), and MagicLinkCallback(url) to locate where to add the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ClerkKit/Core/Auth.swift`:
- Around line 375-383: The catch currently suppresses setActive errors by
checking sessions.contains(where: { $0.id == sessionId }) which only tells if
the session exists, not whether it became the active session; change that check
to verify the active session identity (e.g., compare activeSession?.id ==
sessionId or use the client/instance property that stores the current active
session id) and only swallow the error when that check confirms the session is
actually active; update the block around setActive(sessionId:) to use
active-session comparison instead of sessions.contains so real activation
failures are not silently ignored.
---
Duplicate comments:
In `@Sources/ClerkKit/Core/Auth.swift`:
- Around line 344-374: The public completeMagicLink(flowId:approvalToken:) must
be routed through urlHandlingCoordinator to benefit from deduplication in
handle(_:); refactor by extracting the current body (validation,
magicLinkStore.load(), creating Request<MagicLinkCompleteResponse>, clearing
store, and calling signInWithTicket) into a private helper (e.g., private func
completeMagicLinkImpl(flowId:approvalToken:) async throws -> SignIn) and change
the public method to simply await
urlHandlingCoordinator.handle(.magicLink(flowId: resolvedFlowId, approvalToken:
resolvedApprovalToken)) so the call path goes through handle(_:) and avoids
concurrent races on magicLinkStore and codeVerifier. Ensure the private helper
name and the public completeMagicLink signature are referenced consistently and
preserve existing error behavior.
- Around line 321-333: Public handler handleMagicLinkCallback(_:) currently
parses any URL with flow_id and approval_token; add a redirect-origin check
first to reject non-matching URLs. Inside handleMagicLinkCallback(_ url: URL),
call matchesRedirectUrl(url) (or reuse canHandleMagicLinkCallback(url)) and
throw an appropriate error if it returns false before creating
MagicLinkCallback(url), then proceed with extracting flowId/approvalToken and
calling handle(.magicLink(...)); reference the existing methods
handleMagicLinkCallback(_:), matchesRedirectUrl(_:),
canHandleMagicLinkCallback(_:), and MagicLinkCallback(url) to locate where to
add the guard.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d0b2219-8ca6-4774-b586-274d670b2739
📒 Files selected for processing (2)
Sources/ClerkKit/Core/Auth.swiftSources/ClerkKit/Domains/Auth/MagicLink.swift
Remove stored redirectUrl property from Auth struct and instead read directly from Clerk.shared.options when needed. This eliminates passing the same value through multiple layers and ensures the single source of truth is used consistently. Also simplifies session activation check to compare against lastActiveSessionId instead of iterating through all sessions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Tests/Core/ClerkTests.swift (1)
276-276: Callback URL construction relies onClerk.sharedconfig.The tests create a local
clerkinstance but construct the callback URL usingClerk.shared.options.redirectConfig.redirectUrl. This works becausematchesRedirectUrlinAuthalso referencesClerk.shared, but it couples the test to shared global state set byconfigureClerkForTesting().This is a minor test isolation concern—if
configureClerkForTesting()changes the redirect config, these tests would need to match. Consider documenting this dependency or extracting the redirect URL to a shared test constant.Also applies to: 331-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Core/ClerkTests.swift` at line 276, The test couples to global state by building callbackUrl from Clerk.shared.options.redirectConfig.redirectUrl; update the test to use the local test instance instead (use clerk.options.redirectConfig.redirectUrl when constructing callbackUrl) or replace both occurrences with a shared test constant (e.g., TEST_REDIRECT_URL) used by configureClerkForTesting() and the tests, ensuring consistency with Auth.matchesRedirectUrl; modify the lines using Clerk.shared to reference the local clerk or the shared test constant so the test no longer depends on Clerk.shared.Tests/Domains/Auth/AuthTests.swift (1)
333-333: SameClerk.sharedcallback URL coupling as in ClerkTests.Multiple tests construct callback URLs using
Clerk.shared.options.redirectConfig.redirectUrlwhile testing a localclerkinstance. As noted in ClerkTests, this works but creates implicit coupling to shared global state.Also applies to: 394-394, 463-463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Domains/Auth/AuthTests.swift` at line 333, Tests are using the global Clerk.shared.options.redirectConfig.redirectUrl when building callback URLs which couples tests to global state; update the tests to use the local test instance's redirect URL (e.g., replace Clerk.shared.options.redirectConfig.redirectUrl with clerk.options.redirectConfig.redirectUrl or a helper like makeCallbackURL(for: clerk, flowId:..., approvalToken:...)) so each test constructs callbackUrl from the local clerk instance (same change needed for the other occurrences that use Clerk.shared).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ClerkKit/Core/Clerk.swift`:
- Line 544: The cleanup currently calls urlHandlingCoordinator.cancelAll() which
only signals cancellation and returns immediately; change the coordinator to
expose an awaitable drain (e.g., cancelAllAndWait() async or
drainInFlightTasks() async) that awaits any tracked Task(s) (like the Task
created by completeMagicLink) before returning, then update
cleanupManagersAndDrainCache to await that new async method (mirroring how
invalidAuthRefreshTask is awaited via its .value) so in-flight network work
cannot continue and mutate magicLinkStore after cleanup.
---
Nitpick comments:
In `@Tests/Core/ClerkTests.swift`:
- Line 276: The test couples to global state by building callbackUrl from
Clerk.shared.options.redirectConfig.redirectUrl; update the test to use the
local test instance instead (use clerk.options.redirectConfig.redirectUrl when
constructing callbackUrl) or replace both occurrences with a shared test
constant (e.g., TEST_REDIRECT_URL) used by configureClerkForTesting() and the
tests, ensuring consistency with Auth.matchesRedirectUrl; modify the lines using
Clerk.shared to reference the local clerk or the shared test constant so the
test no longer depends on Clerk.shared.
In `@Tests/Domains/Auth/AuthTests.swift`:
- Line 333: Tests are using the global
Clerk.shared.options.redirectConfig.redirectUrl when building callback URLs
which couples tests to global state; update the tests to use the local test
instance's redirect URL (e.g., replace
Clerk.shared.options.redirectConfig.redirectUrl with
clerk.options.redirectConfig.redirectUrl or a helper like makeCallbackURL(for:
clerk, flowId:..., approvalToken:...)) so each test constructs callbackUrl from
the local clerk instance (same change needed for the other occurrences that use
Clerk.shared).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 76d457c3-6e21-4846-bc3c-cd8f20390b2a
📒 Files selected for processing (4)
Sources/ClerkKit/Core/Auth.swiftSources/ClerkKit/Core/Clerk.swiftTests/Core/ClerkTests.swiftTests/Domains/Auth/AuthTests.swift
Move URL matching and parsing logic from Auth into ClerkURLRoute and MagicLinkCallback. This centralizes route detection while preparing for additional URL route types.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/ClerkKit/Core/Clerk.swift (1)
541-556:⚠️ Potential issue | 🟡 Minor
cancelAll()still doesn’t guarantee in-flight URL work is drained in async cleanup.
cleanupManagersAndDrainCache()is async, but URL handling uses non-awaiting cancellation. A canceled in-flight magic-link task may still run briefly and mutate state after cleanup starts.💡 Suggested direction
- urlHandlingCoordinator.cancelAll() + await urlHandlingCoordinator.cancelAllAndWait()This assumes
URLHandlingCoordinatorexposes an awaitable drain API (for example,cancelAllAndWait()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKit/Core/Clerk.swift` around lines 541 - 556, The async cleanup (cleanupManagersAndDrainCache) calls urlHandlingCoordinator.cancelAll() which only signals cancellation and can allow in-flight magic-link tasks to mutate state after cleanup; change URLHandlingCoordinator to expose an awaitable drain API (e.g., cancelAllAndWait() that cancels and then awaits completion of internal tasks) and update cleanupManagersAndDrainCache() to call and await urlHandlingCoordinator.cancelAllAndWait(); keep the existing synchronous cancelAll() for non-async paths but ensure the new awaited variant is used wherever cleanup must guarantee no in-flight URL work remains (reference: urlHandlingCoordinator, cleanupManagersAndDrainCache(), cancelAll()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ClerkKit/Core/Auth.swift`:
- Around line 377-381: The call to magicLinkStore.clear() is happening before
the ticket exchange and should be deferred until
signInWithTicket(completionResponse.ticket) succeeds; change the sequence so you
call apiClient.send(request).value to get completionResponse, then call
signInWithTicket(completionResponse.ticket) and only after that completes
successfully call magicLinkStore.clear(), ensuring you still propagate errors
from signInWithTicket and do not clear the pending flow on failure.
---
Duplicate comments:
In `@Sources/ClerkKit/Core/Clerk.swift`:
- Around line 541-556: The async cleanup (cleanupManagersAndDrainCache) calls
urlHandlingCoordinator.cancelAll() which only signals cancellation and can allow
in-flight magic-link tasks to mutate state after cleanup; change
URLHandlingCoordinator to expose an awaitable drain API (e.g.,
cancelAllAndWait() that cancels and then awaits completion of internal tasks)
and update cleanupManagersAndDrainCache() to call and await
urlHandlingCoordinator.cancelAllAndWait(); keep the existing synchronous
cancelAll() for non-async paths but ensure the new awaited variant is used
wherever cleanup must guarantee no in-flight URL work remains (reference:
urlHandlingCoordinator, cleanupManagersAndDrainCache(), cancelAll()).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 80daffd9-489e-469c-a33c-58cde218be89
📒 Files selected for processing (4)
Sources/ClerkKit/Core/Auth.swiftSources/ClerkKit/Core/Clerk.swiftSources/ClerkKit/Core/ClerkURLRoute.swiftSources/ClerkKit/Domains/Auth/MagicLink.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/ClerkKit/Domains/Auth/MagicLink.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/ClerkKit/Core/ClerkURLRoute.swift
Move scheme/host/path normalization and required param matching into ClerkURLRoute. Extract query param parsing into reusable URL extension that also checks the URL fragment, enabling support for magic link callbacks that use fragments instead of query params.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/ClerkKit/Core/Auth.swift (1)
378-381:⚠️ Potential issue | 🟠 MajorDefer
magicLinkStore.clear()until after ticket exchange succeeds.The pending flow is cleared on line 379 before
signInWithTicketcompletes on line 381. If ticket exchange fails (e.g., transient network error), the verifier is lost and the user cannot retry.Suggested fix
let completionResponse = try await apiClient.send(request).value - magicLinkStore.clear() let signIn = try await signInWithTicket(completionResponse.ticket) + magicLinkStore.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKit/Core/Auth.swift` around lines 378 - 381, The code currently calls magicLinkStore.clear() before exchanging the ticket, which loses the verifier if signInWithTicket fails; move the magicLinkStore.clear() call so it runs only after try await signInWithTicket(completionResponse.ticket) returns successfully (i.e., call magicLinkStore.clear() after signInWithTicket completes), or alternatively wrap the signInWithTicket call and on success then clear — update the sequence around apiClient.send(...).value, completionResponse.ticket and signInWithTicket to ensure the store is only cleared after a successful ticket exchange.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/ClerkKit/Core/Auth.swift`:
- Around line 378-381: The code currently calls magicLinkStore.clear() before
exchanging the ticket, which loses the verifier if signInWithTicket fails; move
the magicLinkStore.clear() call so it runs only after try await
signInWithTicket(completionResponse.ticket) returns successfully (i.e., call
magicLinkStore.clear() after signInWithTicket completes), or alternatively wrap
the signInWithTicket call and on success then clear — update the sequence around
apiClient.send(...).value, completionResponse.ticket and signInWithTicket to
ensure the store is only cleared after a successful ticket exchange.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2d5fa170-d90f-4dd9-90de-fe2efd92e221
📒 Files selected for processing (5)
Sources/ClerkKit/Core/Auth.swiftSources/ClerkKit/Core/ClerkURLRoute.swiftSources/ClerkKit/Domains/Auth/MagicLink.swiftSources/ClerkKit/Utils/URL+Ext.swiftTests/Domains/Auth/MagicLinkTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/Domains/Auth/MagicLinkTests.swift
Handle native email-link callbacks as either sign-in or sign-up results. Persist pending flow kind, expose auth presentation requirements, and let AuthView resume continuation or session-task flows after app return. Add example wiring, investigation notes, and tests for the new continuation behavior.
Expose ignored, handled, and continuation URL results and rename the stored pending auth result to callback continuation. Remove the auth presentation requirement helper and update examples and tests to present AuthView from URL results or session tasks.
Reduce `Clerk.handle(_:)` to a `Bool` return value instead of a custom result enum and rely on `clerk.auth.events` to surface continuation state. Update the quickstart flow and tests to follow the new API.
Group `Auth` computed properties, session APIs, events, and magic link helpers into separate extensions and remove the temporary native magic link investigation note.
Use isolated Clerk instances in callback tests to avoid shared state leaking through `Clerk.shared`. Simplify concurrent callback deduplication coverage by testing `URLHandlingCoordinator` directly.
Replace `Clerk.shared` usage with per-test `Clerk` instances to avoid shared state across test cases. Add a helper that creates and configures an isolated test instance for these tests.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
Sources/ClerkKit/Core/Auth.swift (2)
655-663:⚠️ Potential issue | 🟠 MajorClear the pending verifier after ticket exchange succeeds.
The store is cleared immediately after
/magic_links/complete; ifsignInWithTicketorsignUpWithTicketthen fails, the local recovery state is gone. MovemagicLinkStore.clear()until after the ticket flow returns aTransferFlowResult.🛠️ Minimal ordering fix
) let completionResponse = try await apiClient.send(request).value - magicLinkStore.clear() let result: TransferFlowResult = switch pendingFlow.kind { case .signIn: try await .signIn(signInWithTicket(completionResponse.ticket)) case .signUp: try await .signUp(signUpWithTicket(completionResponse.ticket)) } + + magicLinkStore.clear() if let sessionId = result.createdSessionId { try await activateSession(sessionId: sessionId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKit/Core/Auth.swift` around lines 655 - 663, The magic link store is being cleared too early (magicLinkStore.clear()) before the ticket exchange flows complete, losing recovery state if signInWithTicket or signUpWithTicket fails; move the magicLinkStore.clear() to after the TransferFlowResult is produced so that pendingFlow handling (the switch on pendingFlow.kind producing .signIn via signInWithTicket and .signUp via signUpWithTicket) runs successfully before clearing the store, i.e., call magicLinkStore.clear() only after the await of signInWithTicket/signUpWithTicket completes and the result is returned.
627-641:⚠️ Potential issue | 🟠 MajorRoute public completion through the same coalescer.
handleMagicLinkCallback(_:)is deduped, but direct concurrent calls to publiccompleteMagicLink(flowId:approvalToken:)can both load the same verifier before either clears it and double-post the completion request. Split the raw body into a private helper, have publiccompleteMagicLinkcallhandle(.magicLink(...)), and have route dispatch call the private helper.Also applies to: 676-687
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKit/Core/Auth.swift` around lines 627 - 641, Public completeMagicLink(flowId:approvalToken:) must route through the same coalescer used by handleMagicLinkCallback(_:) to avoid concurrent double-posting; create a private helper (e.g., completeMagicLinkRaw or completeMagicLinkInternal) that contains the current body logic that reads magicLinkStore.load(), trims/validates flowId and approvalToken, and performs the completion request, then change public completeMagicLink to call handle(.magicLink(flowId:resolvedApprovalToken:)) so the request is deduped by the coalescer, and update handleMagicLinkCallback(_:) (route dispatch) to call the new private helper directly to preserve existing behavior. Ensure you reference and reuse magicLinkStore.load(), the TransferFlowResult return, and existing error throws in the private helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ClerkKit/Core/Clerk.swift`:
- Around line 160-161: The callbackContinuation property (callbackContinuation)
must be nulled during cleanup: update resetManagerStateForCleanup() to set
callbackContinuation = nil, and likewise add the same clear in any other
manager-reset/cleanup functions that currently reset related state (the methods
that clear auth continuation/current flow around where other manager state is
reset) so a reused/test Clerk instance cannot retain a stale continuation.
In `@Sources/ClerkKit/Core/URLHandlingCoordinator.swift`:
- Around line 29-39: The deferred cleanup can overwrite a newer task if
cancelAll() clears and a new task is inserted before the old task completes;
modify the logic that sets and clears inFlightTasks so each insertion records a
unique entry id (e.g., a UUID or incrementing counter) alongside the Task in the
inFlightTasks dictionary, have the defer closure compare the stored entry id
before removing so it only nils the tuple it created, and ensure cancelAll()
still cancels and removes all entries by clearing the whole dictionary; update
references to inFlightTasks, the route key, the local task variable, and the
defer removal to use and check this entry id.
In
`@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift`:
- Around line 204-223: In sendLink (in SignInFactorOneEmailLinkView) the guards
that check clerk.auth.currentSignIn and currentSignUp return early while leaving
deliveryState as .sending; update both guard failure branches to reset
deliveryState (e.g., set deliveryState = .idle) before clearing navigation.path
and returning so the UI isn't left in a sending state. Ensure you modify the
guard branches within the sendLink function that reference signIn and signUp to
set deliveryState back and then return.
In `@Tests/Domains/Auth/AuthTests.swift`:
- Around line 41-63: Update continuation state to be instance-scoped: change
production code in Auth.swift so that any writes use the owning Clerk instance's
callbackContinuation (e.g., assign to clerk.callbackContinuation or call
instance method setCallbackContinuation on that instance) instead of writing to
Clerk.shared; then update the test helper makeIsolatedClerk() to clear the
isolated clerk's continuation (call setCallbackContinuation(nil) or set
clerk.callbackContinuation = nil on the returned clerk) rather than clearing
Clerk.shared, and update the tests' assertions to check the isolated clerk
instance's callbackContinuation (or its instance setCallbackContinuation/getter)
instead of Clerk.shared so AuthView and tests align with instance-scoped state.
---
Duplicate comments:
In `@Sources/ClerkKit/Core/Auth.swift`:
- Around line 655-663: The magic link store is being cleared too early
(magicLinkStore.clear()) before the ticket exchange flows complete, losing
recovery state if signInWithTicket or signUpWithTicket fails; move the
magicLinkStore.clear() to after the TransferFlowResult is produced so that
pendingFlow handling (the switch on pendingFlow.kind producing .signIn via
signInWithTicket and .signUp via signUpWithTicket) runs successfully before
clearing the store, i.e., call magicLinkStore.clear() only after the await of
signInWithTicket/signUpWithTicket completes and the result is returned.
- Around line 627-641: Public completeMagicLink(flowId:approvalToken:) must
route through the same coalescer used by handleMagicLinkCallback(_:) to avoid
concurrent double-posting; create a private helper (e.g., completeMagicLinkRaw
or completeMagicLinkInternal) that contains the current body logic that reads
magicLinkStore.load(), trims/validates flowId and approvalToken, and performs
the completion request, then change public completeMagicLink to call
handle(.magicLink(flowId:resolvedApprovalToken:)) so the request is deduped by
the coalescer, and update handleMagicLinkCallback(_:) (route dispatch) to call
the new private helper directly to preserve existing behavior. Ensure you
reference and reuse magicLinkStore.load(), the TransferFlowResult return, and
existing error throws in the private helper.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d9f07fdd-4593-4344-ad92-ca70b401569c
📒 Files selected for processing (19)
Examples/Quickstart/Quickstart/ContentView.swiftSources/ClerkKit/Core/Auth.swiftSources/ClerkKit/Core/Clerk.swiftSources/ClerkKit/Core/URLHandlingCoordinator.swiftSources/ClerkKit/Domains/Auth/MagicLink.swiftSources/ClerkKit/Domains/Auth/Session/Session+Tasks.swiftSources/ClerkKit/Domains/Auth/SignIn/SignIn+Status.swiftSources/ClerkKit/Domains/Auth/SignIn/SignIn.swiftSources/ClerkKit/Domains/Auth/SignUp/SignUp+Status.swiftSources/ClerkKit/Domains/Auth/SignUp/SignUp.swiftSources/ClerkKit/Domains/Auth/TransferFlowResult.swiftSources/ClerkKit/Events/AuthEvent.swiftSources/ClerkKitUI/Components/Auth/AuthView.swiftSources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swiftSources/ClerkKitUI/Extensions/Session+Ext.swiftTests/Core/ClerkResponseClientStateTests.swiftTests/Core/ClerkTests.swiftTests/Domains/Auth/AuthTests.swiftTests/Middleware/ClerkAuthEventEmitterResponseMiddlewareTests.swift
💤 Files with no reviewable changes (1)
- Sources/ClerkKitUI/Extensions/Session+Ext.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/ClerkKit/Domains/Auth/SignUp/SignUp.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/ClerkKitUI/Components/Auth/AuthView.swift
Clear pending callback continuations during cleanup and use isolated `Clerk` instances in tests. Also prevent stale URL handling tasks from removing newer entries and reset email link delivery state when auth context is missing.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Only repository collaborators, contributors, or members can run CodeRabbit commands. |
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 `@Sources/ClerkKit/Core/Clerk.swift`:
- Around line 341-361: Update the documentation on public func handle(_ url:
URL) async throws -> Bool to explicitly state that unrecognized URLs return
false, but URLs that match Clerk callback structure (created via ClerkURLRoute /
MagicLinkCallback) but lack required parameters (e.g. flow_id or approval_token)
will throw a ClerkClientError; mention that such errors propagate from the
initializer (ClerkURLRoute or MagicLinkCallback) and from the subsequent call to
auth.handle(route).
In
`@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift`:
- Around line 234-246: The openEmailApp() method uses
UIApplication.shared.canOpenURL(url) with the "mailto:" scheme which will fail
on iOS9+ unless integrators add "mailto" to LSApplicationQueriesSchemes; update
the method to either (A) document in README/Docs that integrators must add
"mailto" to LSApplicationQueriesSchemes, referencing openEmailApp() and
ClerkClientError for the error path, or (B) remove the canOpenURL check and call
UIApplication.shared.open(url, options: [:]) with the completion handler and set
error = ClerkClientError(...) only if the completion's success == false; choose
one approach and apply it consistently in openEmailApp().
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2db22fd9-d065-4c69-a1f3-b753121a1184
📒 Files selected for processing (6)
Sources/ClerkKit/Core/Clerk.swiftSources/ClerkKit/Core/URLHandlingCoordinator.swiftSources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swiftTests/Core/ClerkResponseClientStateTests.swiftTests/Core/ClerkTests.swiftTests/Domains/Auth/AuthTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/Core/ClerkTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift (2)
234-245: Past feedback addressed —canOpenURLreplaced withopen+completion.
openEmailApp()now callsUIApplication.shared.open(url, options:)and reacts to thesuccessparameter, which does not requiremailtoto be listed inLSApplicationQueriesSchemes. One small note: the completion handler is invoked on the main thread, but assigningerrorfrom a non-@MainActorfunction inherits the enclosing isolation — sinceEmailLinkVerificationViewis aView, SwiftUI property mutations from that closure are fine, but you may want to annotateopenEmailApp()with@MainActorfor consistency withsendLink()/sendInitialLinkIfNeeded().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift` around lines 234 - 245, Annotate openEmailApp() with `@MainActor` to ensure UI state mutations are performed on the main actor consistently with other methods (e.g., sendLink() and sendInitialLinkIfNeeded()) in EmailLinkVerificationView; update the openEmailApp() declaration to add the `@MainActor` attribute so assignments to the error property from the UIApplication.shared.open completion handler are guaranteed to run on the main thread and conform to the view's isolation.
192-194: Prefer a typed accessor onSignUpmirroringfirstFactorVerificationonSignIn, rather than dictionary access with magic strings.The double-optional chaining
verifications["email_address"]??.strategyis fragile—a typo in the string literal or upstream changes to the dictionary value type would silently fail or returnnilindefinitely.SignUpcurrently lacks a clean typed accessor for email verification state; consider addingemailVerification: Verification?to match the pattern established inSignIn. Alternatively, call the existingemailVerificationStrategycomputed property fromSignUp+Ext.swift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift` around lines 192 - 194, Replace the fragile dictionary lookup `verifications["email_address"]??.strategy` with a typed accessor on SignUp (e.g., add `var emailVerification: Verification?` to the SignUp model mirroring `SignIn.firstFactorVerification`) or call the existing `emailVerificationStrategy` computed property in `SignUp+Ext.swift`; update the usage in `SignInFactorOneEmailLinkView` to read the typed accessor or `emailVerificationStrategy` instead of the string-keyed dictionary to avoid double-optional chaining and magic-string fragility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift`:
- Around line 236-242: Replace the hard-coded user-facing error strings in
SignInFactorOneEmailLinkView where ClerkClientError is constructed (both the
pre-return assignment and inside the UIApplication.shared.open completion) with
localized values: use String(localized:bundle:) (iOS 15+) or NSLocalizedString
routed to the package bundle (bundle: .module) and reference the matching
Localizable key so the message uses the localized entry instead of a raw String.
- Around line 189-202: The current alreadySent boolean only checks strategy ==
.emailLink and thus treats expired/failed/consumed verifications as already
sent; change the check to include verification.status == .unverified as well
(for signIn: clerk.auth.currentSignIn?.firstFactorVerification?.strategy ==
.emailLink && clerk.auth.currentSignIn?.firstFactorVerification?.status ==
.unverified; for signUp:
clerk.auth.currentSignUp?.verifications["email_address"]??.strategy ==
.emailLink && clerk.auth.currentSignUp?.verifications["email_address"]??.status
== .unverified) so deliveryState is only set to .sent for an active unverified
email link, or alternatively always call sendLink()/sendEmailLink() on the first
appearance to force a fresh PKCE verifier; update the alreadySent logic
accordingly and keep deliveryState/sendLink flow unchanged.
---
Nitpick comments:
In
`@Sources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift`:
- Around line 234-245: Annotate openEmailApp() with `@MainActor` to ensure UI
state mutations are performed on the main actor consistently with other methods
(e.g., sendLink() and sendInitialLinkIfNeeded()) in EmailLinkVerificationView;
update the openEmailApp() declaration to add the `@MainActor` attribute so
assignments to the error property from the UIApplication.shared.open completion
handler are guaranteed to run on the main thread and conform to the view's
isolation.
- Around line 192-194: Replace the fragile dictionary lookup
`verifications["email_address"]??.strategy` with a typed accessor on SignUp
(e.g., add `var emailVerification: Verification?` to the SignUp model mirroring
`SignIn.firstFactorVerification`) or call the existing
`emailVerificationStrategy` computed property in `SignUp+Ext.swift`; update the
usage in `SignInFactorOneEmailLinkView` to read the typed accessor or
`emailVerificationStrategy` instead of the string-keyed dictionary to avoid
double-optional chaining and magic-string fragility.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a2e5c11-2324-4cb3-b39a-5a31758db4ab
📒 Files selected for processing (2)
Sources/ClerkKit/Core/Clerk.swiftSources/ClerkKitUI/Components/Auth/SignIn/SignInFactorOneEmailLinkView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/ClerkKit/Core/Clerk.swift
Use the sign-up email verification accessor when checking whether an email link has already been sent, and require the verification to still be unverified before suppressing another send. Also keep the email app failure error update on the main actor.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 by CodeRabbit
New Features
Localization
Chore
Tests