Open
Conversation
swolfand
force-pushed
the
sam/mobile-431-android-changes
branch
from
8c3939d to
e15a634
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces native magic-link (email link) authentication with PKCE: public NativeMagicLinkManager and attestation provider, internal NativeMagicLinkService and NativeMagicLinkCompletionRunner, PKCE utilities, network models/endpoints and ClerkApi.magicLink client, and persistent pending-flow storage. Integrates native magic-link deep-link parsing/handling into Auth and SSO manager flows and adds Auth APIs for starting/handling/completing email-link sign-ins. Adds UI screens/viewmodels for sign-in/sign-up email links, EmailApp launcher, auth-bridge theme, storage key updates, telemetry environment DI changes, API param/path additions, and numerous tests. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/api/src/main/kotlin/com/clerk/api/auth/Auth.kt`:
- Around line 229-237: The current logic short-circuits on any return from
signInWithNativeEmailLink(identifier), making failures terminal and preventing
fallback to createAndPrepareOtpSignIn(identifier, isEmailFlow); change the
control flow so you only return/short-circuit when
signInWithNativeEmailLink(...) indicates a successful native email-link start
(e.g. a success flag or non-error result), and otherwise call
createAndPrepareOtpSignIn(identifier, isEmailFlow) so signInWithOtp(email) still
works as the EMAIL_CODE fallback; adjust the handling of nativeEmailLinkResult
in the sign-in flow (referencing signInWithNativeEmailLink and
createAndPrepareOtpSignIn) to only skip OTP creation on explicit success.
- Around line 746-758: The current handle(uri: Uri?) implementation blocks the
main thread via kotlinx.coroutines.runBlocking when calling
NativeMagicLinkService.handleMagicLinkDeepLink and
SSOService.completeAuthenticateWithRedirect; change this by making handle a
suspend fun handle(uri: Uri?): Boolean (or add a suspend alternative like
suspend fun handleAsync(...)) and call
NativeMagicLinkService.handleMagicLinkDeepLink(callbackUri) and
SSOService.completeAuthenticateWithRedirect(callbackUri) directly (remove
runBlocking), preserving the existing logic that checks
NativeMagicLinkService.canHandle(callbackUri) and the isClerkCallback scheme
test, and return the same boolean result; ensure callers from UI/Activity are
updated to call the suspend variant or a provided wrapper that launches a
coroutine off the main thread.
In
`@source/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.kt`:
- Around line 35-37: The pending PKCE state is currently held in
InMemoryPendingNativeMagicLinkStore (field pendingFlowStore in
NativeMagicLinkManager) which is lost when the Android process is killed;
replace or supplement that with a persistent implementation of
PendingNativeMagicLinkStore (e.g., SharedPreferences/Room-backed
PersistentPendingNativeMagicLinkStore) and wire it into NativeMagicLinkManager
instead of InMemoryPendingNativeMagicLinkStore so saved verifiers survive
process death; ensure the store serializes the verifier, state, timestamps and
any flow ID written by the methods that save pending flows (the code paths at
the save/persist locations referenced around the current verifier storage in
complete() and the start/resume flow routines) and make reads/writes atomic
(coordinated with the existing mutex) so complete() can recover and validate the
PKCE verifier after a process restart.
In `@source/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.kt`:
- Around line 46-55: The fast-path clears intent.data immediately which makes
the callback irrecoverable across activity recreation; instead persist and clear
safely: capture callbackUri into a persistent field (e.g., a lateinit or
nullable property on the activity) and save/restore it via
onSaveInstanceState/onCreate OR delay clearing intent.data until after
authorizationComplete finishes; update the block around
completionStarted/authorizationStarted/authorizationComplete to set a field like
pendingCallbackUri = callbackUri, call
authorizationComplete(pendingCallbackUri), and only null out intent.data (or
pendingCallbackUri) once the async completion has truly finished so a recreated
activity can replay pendingCallbackUri and not fall through to cancellation.
In
`@source/ui/src/main/java/com/clerk/ui/signin/code/SignInFactorCodeViewModel.kt`:
- Around line 143-155: The current logic treats factors as interchangeable by
comparing only strategy (e.g., StrategyKeys.EMAIL_CODE) when computing
supportedFirstFactors/supportedSecondFactors and prefersEmailLinkOverEmailCode;
update these checks to match the concrete factor identity instead of just
strategy: in the supportedFirstFactors.none and supportedSecondFactors?.none
calls (and any other strategy-only comparisons around isSecondFactor and
prefersEmailLinkOverEmailCode) compare both strategy and the unique identifier
fields from the factor (emailAddressId, phoneNumberId, safeIdentifier) so you
only skip/prefer the exact selected factor; also ensure
signIn.shouldPreferEmailLink is invoked or implemented to accept and consider
the fallback factor’s identity (not just its strategy) when deciding to prefer
EMAIL_LINK.
In `@source/ui/src/main/java/com/clerk/ui/signin/code/SignInPrepareHandler.kt`:
- Around line 95-113: The prepareSecondFactor handlers currently log failures
from inProgressSignIn.prepareSecondFactor(...) but never call the provided
onError callback, leaving the UI stuck; update the failure branches in
prepareSecondFactorPhoneCode (and the corresponding second-factor handler around
lines 141-159) to invoke onError with a user-friendly message (e.g.,
error.localizedMessage or a static "Failed to prepare second factor") in
addition to logging the error, by adding an onFailure { ClerkLog.e(...);
onError(...) } to the prepareSecondFactor(...).onFailure(...) chains so callers
receive the failure.
In `@source/ui/src/main/java/com/clerk/ui/signin/SignInFactorOneView.kt`:
- Around line 52-71: The logic currently prefers preparedFactor over
emailLinkFactor causing EMAIL_CODE prepared factors to block EMAIL_LINK; modify
the selection so emailLinkFactor is chosen when present or when preparedFactor
has strategy EMAIL_CODE. Concretely, update the return logic around
preparedFactor and emailLinkFactor in SignInFactorOneView (references:
preparedFactor, emailLinkFactor, supportedFactors.factorForStrategy,
currentSignIn.firstFactorVerification?.strategy, preferredEmailLinkFactor) to
either check emailLinkFactor first or add a conditional that if
preparedFactor?.strategy == EMAIL_CODE then yield emailLinkFactor (if non-null)
instead of preparedFactor; keep
fallbackIsSupported/fallback/currentSignIn.startingFirstFactor behavior
unchanged.
In `@source/ui/src/main/java/com/clerk/ui/userbutton/UserButton.kt`:
- Around line 71-80: Compute a single effective session and use it for both
visibility and MFA routing: create val effectiveSession = session ?:
Clerk.session and pass effectiveSession != null to resolveUserButtonState
(instead of session != null || Clerk.session != null) and read requiresForcedMfa
from effectiveSession (effectiveSession?.requiresForcedMfa == true); keep using
resolveUserButtonState, requiresForcedMfa, user and shouldShowButton as before
but all derived from effectiveSession to avoid the race between sessionFlow and
Clerk.session.
- Around line 175-193: The build fails due to an unresolved reference to
Modifier.matchParentSize used in UserButton (calls at Modifier.matchParentSize()
inside the image rendering branch and Icon branch); fix by adding the missing
import for androidx.compose.foundation.layout.matchParentSize to the imports in
UserButton.kt so Modifier.matchParentSize() resolves correctly.
In `@source/ui/src/test/java/com/clerk/ui/userbutton/UserButtonBehaviorTest.kt`:
- Around line 61-67: The test "shouldShowUserButton hides when only session user
exists and pending is treated as signed out" is passing the wrong inputs; update
the test call to shouldShowUserButton so it exercises the "session only" branch
by setting hasSession = true, hasActiveUser = false, treatPendingAsSignedOut =
true (instead of hasSession = false) so the pending-session behavior is actually
tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 836a0281-430b-44a3-b01c-d4984c532e2d
📒 Files selected for processing (69)
config/git/hooks/post-checkoutconfig/git/hooks/post-commitconfig/git/hooks/post-mergeconfig/git/hooks/pre-pushsource/api/src/main/AndroidManifest.xmlsource/api/src/main/kotlin/com/clerk/api/Constants.ktsource/api/src/main/kotlin/com/clerk/api/auth/Auth.ktsource/api/src/main/kotlin/com/clerk/api/log/SafeUriLog.ktsource/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkCompletionRunner.ktsource/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.ktsource/api/src/main/kotlin/com/clerk/api/magiclink/PkceUtil.ktsource/api/src/main/kotlin/com/clerk/api/network/ApiParams.ktsource/api/src/main/kotlin/com/clerk/api/network/ApiPaths.ktsource/api/src/main/kotlin/com/clerk/api/network/ClerkApi.ktsource/api/src/main/kotlin/com/clerk/api/network/api/MagicLinkApi.ktsource/api/src/main/kotlin/com/clerk/api/network/api/SignUpApi.ktsource/api/src/main/kotlin/com/clerk/api/network/model/magiclink/NativeMagicLinkRequests.ktsource/api/src/main/kotlin/com/clerk/api/network/serialization/ClerkApiResultConverterFactory.ktsource/api/src/main/kotlin/com/clerk/api/signin/SignIn.ktsource/api/src/main/kotlin/com/clerk/api/signin/SignInExtensions.ktsource/api/src/main/kotlin/com/clerk/api/signup/SignUp.ktsource/api/src/main/kotlin/com/clerk/api/signup/SignUpExtensions.ktsource/api/src/main/kotlin/com/clerk/api/sso/RedirectConfiguration.ktsource/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.ktsource/api/src/main/res/values/themes.xmlsource/api/src/test/java/com/clerk/api/log/SafeUriLogTest.ktsource/api/src/test/java/com/clerk/api/magiclink/MagicLinkDeepLinkParserTest.ktsource/api/src/test/java/com/clerk/api/magiclink/NativeMagicLinkErrorMappingTest.ktsource/api/src/test/java/com/clerk/api/magiclink/NativeMagicLinkServiceTest.ktsource/api/src/test/java/com/clerk/api/magiclink/PkceUtilTest.ktsource/api/src/test/java/com/clerk/api/signin/SignInExtensionsTest.ktsource/api/src/test/java/com/clerk/api/signup/SignUpEmailVerificationStrategyTest.ktsource/api/src/test/java/com/clerk/api/sso/RedirectConfigurationTest.ktsource/api/src/test/java/com/clerk/api/sso/SSOManagerActivityTest.ktsource/telemetry/src/androidMain/kotlin/com/clerk/telemetry/ClerkTelemetryEnvironment.ktsource/telemetry/src/commonMain/kotlin/com/clerk/telemetry/TelemetryCollector.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthStartView.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthState.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthView.ktsource/ui/src/main/java/com/clerk/ui/core/button/social/ClerkSocialButton.ktsource/ui/src/main/java/com/clerk/ui/core/common/StrategyKeys.ktsource/ui/src/main/java/com/clerk/ui/core/composition/CompositionLocals.ktsource/ui/src/main/java/com/clerk/ui/core/scaffold/ClerkThemedAuthScaffold.ktsource/ui/src/main/java/com/clerk/ui/core/scaffold/ClerkThemedProfileScaffold.ktsource/ui/src/main/java/com/clerk/ui/core/scaffold/LogoVisibility.ktsource/ui/src/main/java/com/clerk/ui/signin/SignInFactorOneView.ktsource/ui/src/main/java/com/clerk/ui/signin/code/SignInFactorCodeViewModel.ktsource/ui/src/main/java/com/clerk/ui/signin/code/SignInPrepareHandler.ktsource/ui/src/main/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkView.ktsource/ui/src/main/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkViewModel.ktsource/ui/src/main/java/com/clerk/ui/signup/code/SignUpCodeViewModel.ktsource/ui/src/main/java/com/clerk/ui/signup/emaillink/SignUpEmailLinkView.ktsource/ui/src/main/java/com/clerk/ui/signup/emaillink/SignUpEmailLinkViewModel.ktsource/ui/src/main/java/com/clerk/ui/userbutton/UserButton.ktsource/ui/src/main/java/com/clerk/ui/util/EmailAppLauncher.ktsource/ui/src/main/java/com/clerk/ui/util/TextIconHelper.ktsource/ui/src/main/res/values/strings.xmlsource/ui/src/test/java/com/clerk/ui/auth/AuthStateSignUpRoutingTest.ktsource/ui/src/test/java/com/clerk/ui/core/scaffold/LogoVisibilityTest.ktsource/ui/src/test/java/com/clerk/ui/signin/SignInFactorOneViewTest.ktsource/ui/src/test/java/com/clerk/ui/signin/code/SignInFactorCodeViewModelTest.ktsource/ui/src/test/java/com/clerk/ui/signin/code/SignInPrepareHandlerTest.ktsource/ui/src/test/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkViewModelTest.ktsource/ui/src/test/java/com/clerk/ui/userbutton/UserButtonBehaviorTest.ktworkbench/src/debug/AndroidManifest.xmlworkbench/src/debug/res/xml/workbench_debug_network_security_config.xmlworkbench/src/main/java/com/clerk/workbench/HomeView.ktworkbench/src/main/java/com/clerk/workbench/UiActivity2.ktworkbench/src/main/java/com/clerk/workbench/WorkbenchApplication.kt
source/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.kt
Show resolved
Hide resolved
source/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.kt
Outdated
Show resolved
Hide resolved
source/ui/src/main/java/com/clerk/ui/signin/code/SignInFactorCodeViewModel.kt
Outdated
Show resolved
Hide resolved
source/ui/src/main/java/com/clerk/ui/signin/code/SignInPrepareHandler.kt
Show resolved
Hide resolved
Comment on lines
+175
to
+193
| if (imageUrl.isNullOrBlank()) { | ||
| Icon( | ||
| painter = profilePainter, | ||
| contentDescription = stringResource(R.string.user_avatar), | ||
| modifier = Modifier.matchParentSize(), | ||
| tint = Color.Unspecified, | ||
| ) | ||
| } else { | ||
| val model = | ||
| ImageRequest.Builder(LocalContext.current).data(imageUrl).crossfade(true).build() | ||
| AsyncImage( | ||
| modifier = Modifier.matchParentSize().clip(CircleShape), | ||
| model = model, | ||
| contentDescription = stringResource(R.string.user_avatar), | ||
| contentScale = ContentScale.Crop, | ||
| placeholder = profilePainter, | ||
| fallback = profilePainter, | ||
| error = profilePainter, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
file="$(fd '^UserButton\.kt$' source | head -n1)"
sed -n '1,220p' "$file" | rg -n 'import androidx\.compose\.foundation\.layout\.matchParentSize|matchParentSize\('Repository: clerk/clerk-android
Length of output: 186
🏁 Script executed:
#!/bin/bash
file="$(fd '^UserButton\.kt$' source | head -n1)"
echo "=== Checking imports section ==="
head -n 50 "$file" | rg -n '^import'
echo ""
echo "=== All matchParentSize references ==="
rg -n 'matchParentSize' "$file"Repository: clerk/clerk-android
Length of output: 2163
Add the missing matchParentSize import or this won't compile.
Lines 179 and 186 call Modifier.matchParentSize(), but the file does not import androidx.compose.foundation.layout.matchParentSize. This is an unresolved reference at compile time.
Proposed fix
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.fillMaxSize
+import androidx.compose.foundation.layout.matchParentSize
import androidx.compose.foundation.layout.size📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (imageUrl.isNullOrBlank()) { | |
| Icon( | |
| painter = profilePainter, | |
| contentDescription = stringResource(R.string.user_avatar), | |
| modifier = Modifier.matchParentSize(), | |
| tint = Color.Unspecified, | |
| ) | |
| } else { | |
| val model = | |
| ImageRequest.Builder(LocalContext.current).data(imageUrl).crossfade(true).build() | |
| AsyncImage( | |
| modifier = Modifier.matchParentSize().clip(CircleShape), | |
| model = model, | |
| contentDescription = stringResource(R.string.user_avatar), | |
| contentScale = ContentScale.Crop, | |
| placeholder = profilePainter, | |
| fallback = profilePainter, | |
| error = profilePainter, | |
| ) | |
| import androidx.compose.foundation.layout.Box | |
| import androidx.compose.foundation.layout.fillMaxSize | |
| import androidx.compose.foundation.layout.matchParentSize | |
| import androidx.compose.foundation.layout.size |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/ui/src/main/java/com/clerk/ui/userbutton/UserButton.kt` around lines
175 - 193, The build fails due to an unresolved reference to
Modifier.matchParentSize used in UserButton (calls at Modifier.matchParentSize()
inside the image rendering branch and Icon branch); fix by adding the missing
import for androidx.compose.foundation.layout.matchParentSize to the imports in
UserButton.kt so Modifier.matchParentSize() resolves correctly.
source/ui/src/test/java/com/clerk/ui/userbutton/UserButtonBehaviorTest.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
source/ui/src/main/java/com/clerk/ui/signin/code/SignInPrepareHandler.kt (1)
109-112:⚠️ Potential issue | 🔴 CriticalPropagate second-factor prepare failures to
onError.Both second-factor branches still swallow
prepareSecondFactor(...)failures. If the backend rejects the prepare call, the user stays on the factor screen with no error because the caller never gets notified.Suggested fix
inProgressSignIn .prepareSecondFactor(phoneNumberId) .onSuccess { ClerkLog.v("Successfully prepared second factor for phone code") } - .onFailure { ClerkLog.e("Error preparing second factor for phone code: $it") } + .onFailure { + onError(it.errorMessage) + ClerkLog.e("Error preparing second factor for phone code: $it") + } @@ inProgressSignIn .prepareSecondFactor(emailAddressId = emailAddressId) .onSuccess { ClerkLog.v("Successfully prepared second factor for email code") } - .onFailure { ClerkLog.e("Error preparing second factor for email code: $it") } + .onFailure { + onError(it.errorMessage) + ClerkLog.e("Error preparing second factor for email code: $it") + }Also applies to: 155-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/src/main/java/com/clerk/ui/signin/code/SignInPrepareHandler.kt` around lines 109 - 112, The prepareSecondFactor(...) failure is currently only logged and not propagated, leaving the caller unaware; update both usages of inProgressSignIn.prepareSecondFactor(phoneNumberId) (lines using inProgressSignIn.prepareSecondFactor) to invoke the provided onError callback when the prepare call fails (i.e., inside the .onFailure lambda) instead of just logging—pass the throwable or an appropriate error object to onError so the UI can surface the error and prevent staying on the factor screen.source/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.kt (1)
46-52:⚠️ Potential issue | 🔴 CriticalPersist the callback URI until completion actually finishes.
Line 51 clears
intent.databefore the suspend work completes, butonSaveInstanceState()never writesURI_KEY. If the activity is recreated mid-request, the restored instance hascompletionStarted = trueand no callback URI to replay, so the nextonResume()can fall through toauthorizationCanceled()and turn a valid redirect intoRESULT_CANCELED. Please keep the callback URI in saved state, or delay clearing it untilauthorizationComplete()returns.Possible fix sketch
+ private var pendingCallbackUri: Uri? = null + override fun onResume() { super.onResume() - val callbackUri = intent.data?.takeIf(::isCallbackUri) + val callbackUri = pendingCallbackUri ?: intent.data?.takeIf(::isCallbackUri) if (callbackUri != null) { if (!completionStarted) { completionStarted = true authorizationStarted = true - intent = Intent(intent).apply { data = null } + pendingCallbackUri = callbackUri authorizationComplete(callbackUri) } return } } override fun onSaveInstanceState(outState: Bundle) { super.onSaveInstanceState(outState) outState.putBoolean(KEY_AUTHORIZATION_STARTED, authorizationStarted) outState.putBoolean(KEY_COMPLETION_STARTED, completionStarted) + pendingCallbackUri?.let { outState.putString(URI_KEY, it.toString()) } } private fun hydrateState(state: Bundle?) { if (state == null) return finish() authorizationStarted = state.getBoolean(KEY_AUTHORIZATION_STARTED, false) completionStarted = state.getBoolean(KEY_COMPLETION_STARTED, false) - state.getString(URI_KEY)?.let { desiredUri = it.toUri() } + state.getString(URI_KEY)?.let { pendingCallbackUri = it.toUri() } } private fun authorizationComplete(uri: Uri) { lifecycleScope.launch { try { ... } finally { + pendingCallbackUri = null + intent = Intent(intent).apply { data = null } finish() } } }Also applies to: 95-99, 107-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.kt` around lines 46 - 52, The activity clears intent.data (callbackUri) before the suspend work in authorizationComplete() finishes, so if the activity is recreated the saved state lacks URI_KEY and completionStarted=true causing authorizationCanceled(); instead persist the callback URI into saved instance state and only clear intent.data after completion returns: update onSaveInstanceState() to write URI_KEY from the local callbackUri, restore it in onCreate/onRestoreInstanceState to set callbackUri when recreating, and move the Intent.apply { data = null } or clearing logic to after authorizationComplete(callbackUri) (or clear it inside authorizationComplete() on successful completion) while keeping the completionStarted/authorizationStarted flags handling in the same places (referencing callbackUri, completionStarted, authorizationStarted, authorizationComplete, onSaveInstanceState, URI_KEY).source/ui/src/main/java/com/clerk/ui/userbutton/UserButton.kt (2)
175-186:⚠️ Potential issue | 🔴 CriticalAdd the missing
matchParentSizeimport or this won’t compile.
Modifier.matchParentSize()is used twice here, but the file still does not importandroidx.compose.foundation.layout.matchParentSize, so both calls are unresolved.Suggested fix
import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.matchParentSize import androidx.compose.foundation.layout.sizeQuick check, expected to show the usages and no matching import:
#!/bin/bash file="$(fd '^UserButton\.kt$' source | head -n1)" echo "=== matchParentSize usages ===" rg -n 'matchParentSize\(' "$file" echo "=== import check ===" rg -n '^import androidx\.compose\.foundation\.layout\.matchParentSize$' "$file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/src/main/java/com/clerk/ui/userbutton/UserButton.kt` around lines 175 - 186, The file uses Modifier.matchParentSize() in UserButton (both in the Icon and AsyncImage branches) but is missing the corresponding import; add the import for androidx.compose.foundation.layout.matchParentSize at the top of the file so Modifier.matchParentSize resolves; locate the usages in the UserButton composable and ensure the new import is present alongside other androidx.compose imports.
71-78:⚠️ Potential issue | 🟠 MajorUse one effective session for both visibility and forced-MFA routing.
resolvedcan make the button visible fromClerk.sessionwhilerequiresForcedMfastill reads onlysessionFlow. In that window, a forced-MFA session can open profile instead of auth.Suggested fix
+ val effectiveSession = session ?: Clerk.session val resolved = resolveUserButtonState( - sessionExists = session != null || Clerk.session != null, + sessionExists = effectiveSession != null, sessionUser = sessionUser, activeUser = Clerk.activeUser ?: Clerk.user, treatPendingAsSignedOut = treatPendingAsSignedOut, ) - val requiresForcedMfa = session?.requiresForcedMfa == true + val requiresForcedMfa = effectiveSession?.requiresForcedMfa == true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/src/main/java/com/clerk/ui/userbutton/UserButton.kt` around lines 71 - 78, Use one effective session for both visibility and forced-MFA routing: compute an effectiveSession = session ?: Clerk.session (or equivalent) and pass effectiveSession != null to resolveUserButtonState's sessionExists instead of mixing session and Clerk.session, then derive requiresForcedMfa from the same effectiveSession (effectiveSession?.requiresForcedMfa == true) so both visibility and MFA routing use the identical session source; update references in resolveUserButtonState call and the requiresForcedMfa assignment (referencing resolveUserButtonState, session, Clerk.session, and requiresForcedMfa).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@source/api/src/test/java/com/clerk/api/signup/SignUpEmailVerificationStrategyTest.kt`:
- Around line 33-44: In setUp(), save the original global value of
Clerk.environment to a local variable (e.g., prevEnvironment) before assigning
Clerk.environment = environment, and in tearDown() restore Clerk.environment =
prevEnvironment (or null if previously unset) before calling unmockkAll();
modify the SignUpEmailVerificationStrategyTest setUp and tearDown to capture and
restore the previous environment to avoid leaking the mocked environment across
tests.
In
`@source/ui/src/main/java/com/clerk/ui/core/scaffold/ClerkThemedProfileScaffold.kt`:
- Line 20: The scaffold currently reads the org logo directly from
Clerk.organizationLogoUrl (non-reactive) which can go stale; instead obtain the
auth state via LocalAuthState.current inside ClerkThemedProfileScaffold and
derive the logo URL from that (e.g., authState.organizationLogoUrl) and compute
shouldShowLogo from that reactive value. Locate references to
Clerk.organizationLogoUrl in ClerkThemedProfileScaffold (top bar / profile logo
logic) and replace them to read LocalAuthState.current, then use that reactive
property to control visibility and rendering of the logo.
In `@workbench/src/main/java/com/clerk/workbench/WorkbenchApplication.kt`:
- Around line 15-22: The initialization pins a hardcoded proxyUrl in
Clerk.initialize via ClerkConfigurationOptions, which forces every run to use
that environment; update the workbench startup to not hardcode proxyUrl—either
remove the proxyUrl field (leave it unset) or read it from the same
runtime-configurable source as the publicKey (e.g., a command-line arg, env var,
or runtime selection) and pass that value into ClerkConfigurationOptions so
proxy selection is configurable per run; adjust the code around Clerk.initialize
and ClerkConfigurationOptions to accept the runtime proxy value instead of the
fixed string.
---
Duplicate comments:
In `@source/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.kt`:
- Around line 46-52: The activity clears intent.data (callbackUri) before the
suspend work in authorizationComplete() finishes, so if the activity is
recreated the saved state lacks URI_KEY and completionStarted=true causing
authorizationCanceled(); instead persist the callback URI into saved instance
state and only clear intent.data after completion returns: update
onSaveInstanceState() to write URI_KEY from the local callbackUri, restore it in
onCreate/onRestoreInstanceState to set callbackUri when recreating, and move the
Intent.apply { data = null } or clearing logic to after
authorizationComplete(callbackUri) (or clear it inside authorizationComplete()
on successful completion) while keeping the
completionStarted/authorizationStarted flags handling in the same places
(referencing callbackUri, completionStarted, authorizationStarted,
authorizationComplete, onSaveInstanceState, URI_KEY).
In `@source/ui/src/main/java/com/clerk/ui/signin/code/SignInPrepareHandler.kt`:
- Around line 109-112: The prepareSecondFactor(...) failure is currently only
logged and not propagated, leaving the caller unaware; update both usages of
inProgressSignIn.prepareSecondFactor(phoneNumberId) (lines using
inProgressSignIn.prepareSecondFactor) to invoke the provided onError callback
when the prepare call fails (i.e., inside the .onFailure lambda) instead of just
logging—pass the throwable or an appropriate error object to onError so the UI
can surface the error and prevent staying on the factor screen.
In `@source/ui/src/main/java/com/clerk/ui/userbutton/UserButton.kt`:
- Around line 175-186: The file uses Modifier.matchParentSize() in UserButton
(both in the Icon and AsyncImage branches) but is missing the corresponding
import; add the import for androidx.compose.foundation.layout.matchParentSize at
the top of the file so Modifier.matchParentSize resolves; locate the usages in
the UserButton composable and ensure the new import is present alongside other
androidx.compose imports.
- Around line 71-78: Use one effective session for both visibility and
forced-MFA routing: compute an effectiveSession = session ?: Clerk.session (or
equivalent) and pass effectiveSession != null to resolveUserButtonState's
sessionExists instead of mixing session and Clerk.session, then derive
requiresForcedMfa from the same effectiveSession
(effectiveSession?.requiresForcedMfa == true) so both visibility and MFA routing
use the identical session source; update references in resolveUserButtonState
call and the requiresForcedMfa assignment (referencing resolveUserButtonState,
session, Clerk.session, and requiresForcedMfa).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bef036e9-90d4-424a-b2ca-762c73844ba5
📒 Files selected for processing (64)
source/api/src/main/AndroidManifest.xmlsource/api/src/main/kotlin/com/clerk/api/Constants.ktsource/api/src/main/kotlin/com/clerk/api/auth/Auth.ktsource/api/src/main/kotlin/com/clerk/api/log/SafeUriLog.ktsource/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkCompletionRunner.ktsource/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.ktsource/api/src/main/kotlin/com/clerk/api/magiclink/PkceUtil.ktsource/api/src/main/kotlin/com/clerk/api/network/ApiParams.ktsource/api/src/main/kotlin/com/clerk/api/network/ApiPaths.ktsource/api/src/main/kotlin/com/clerk/api/network/ClerkApi.ktsource/api/src/main/kotlin/com/clerk/api/network/api/MagicLinkApi.ktsource/api/src/main/kotlin/com/clerk/api/network/api/SignUpApi.ktsource/api/src/main/kotlin/com/clerk/api/network/model/magiclink/NativeMagicLinkRequests.ktsource/api/src/main/kotlin/com/clerk/api/network/serialization/ClerkApiResultConverterFactory.ktsource/api/src/main/kotlin/com/clerk/api/signin/SignIn.ktsource/api/src/main/kotlin/com/clerk/api/signin/SignInExtensions.ktsource/api/src/main/kotlin/com/clerk/api/signup/SignUp.ktsource/api/src/main/kotlin/com/clerk/api/signup/SignUpExtensions.ktsource/api/src/main/kotlin/com/clerk/api/sso/RedirectConfiguration.ktsource/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.ktsource/api/src/main/res/values/themes.xmlsource/api/src/test/java/com/clerk/api/log/SafeUriLogTest.ktsource/api/src/test/java/com/clerk/api/magiclink/MagicLinkDeepLinkParserTest.ktsource/api/src/test/java/com/clerk/api/magiclink/NativeMagicLinkErrorMappingTest.ktsource/api/src/test/java/com/clerk/api/magiclink/NativeMagicLinkServiceTest.ktsource/api/src/test/java/com/clerk/api/magiclink/PkceUtilTest.ktsource/api/src/test/java/com/clerk/api/signin/SignInExtensionsTest.ktsource/api/src/test/java/com/clerk/api/signup/SignUpEmailVerificationStrategyTest.ktsource/api/src/test/java/com/clerk/api/sso/RedirectConfigurationTest.ktsource/api/src/test/java/com/clerk/api/sso/SSOManagerActivityTest.ktsource/telemetry/src/androidMain/kotlin/com/clerk/telemetry/ClerkTelemetryEnvironment.ktsource/telemetry/src/commonMain/kotlin/com/clerk/telemetry/TelemetryCollector.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthStartView.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthState.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthView.ktsource/ui/src/main/java/com/clerk/ui/core/common/StrategyKeys.ktsource/ui/src/main/java/com/clerk/ui/core/composition/CompositionLocals.ktsource/ui/src/main/java/com/clerk/ui/core/scaffold/ClerkThemedAuthScaffold.ktsource/ui/src/main/java/com/clerk/ui/core/scaffold/ClerkThemedProfileScaffold.ktsource/ui/src/main/java/com/clerk/ui/core/scaffold/LogoVisibility.ktsource/ui/src/main/java/com/clerk/ui/signin/SignInFactorOneView.ktsource/ui/src/main/java/com/clerk/ui/signin/code/SignInFactorCodeViewModel.ktsource/ui/src/main/java/com/clerk/ui/signin/code/SignInPrepareHandler.ktsource/ui/src/main/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkView.ktsource/ui/src/main/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkViewModel.ktsource/ui/src/main/java/com/clerk/ui/signup/code/SignUpCodeViewModel.ktsource/ui/src/main/java/com/clerk/ui/signup/emaillink/SignUpEmailLinkView.ktsource/ui/src/main/java/com/clerk/ui/signup/emaillink/SignUpEmailLinkViewModel.ktsource/ui/src/main/java/com/clerk/ui/userbutton/UserButton.ktsource/ui/src/main/java/com/clerk/ui/util/EmailAppLauncher.ktsource/ui/src/main/java/com/clerk/ui/util/TextIconHelper.ktsource/ui/src/main/res/values/strings.xmlsource/ui/src/test/java/com/clerk/ui/auth/AuthStateSignUpRoutingTest.ktsource/ui/src/test/java/com/clerk/ui/core/scaffold/LogoVisibilityTest.ktsource/ui/src/test/java/com/clerk/ui/signin/SignInFactorOneViewTest.ktsource/ui/src/test/java/com/clerk/ui/signin/code/SignInFactorCodeViewModelTest.ktsource/ui/src/test/java/com/clerk/ui/signin/code/SignInPrepareHandlerTest.ktsource/ui/src/test/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkViewModelTest.ktsource/ui/src/test/java/com/clerk/ui/userbutton/UserButtonBehaviorTest.ktworkbench/src/debug/AndroidManifest.xmlworkbench/src/debug/res/xml/workbench_debug_network_security_config.xmlworkbench/src/main/java/com/clerk/workbench/HomeView.ktworkbench/src/main/java/com/clerk/workbench/UiActivity2.ktworkbench/src/main/java/com/clerk/workbench/WorkbenchApplication.kt
🚧 Files skipped from review as they are similar to previous changes (26)
- source/api/src/main/res/values/themes.xml
- source/ui/src/test/java/com/clerk/ui/signin/code/SignInPrepareHandlerTest.kt
- source/api/src/main/kotlin/com/clerk/api/log/SafeUriLog.kt
- source/api/src/main/kotlin/com/clerk/api/network/serialization/ClerkApiResultConverterFactory.kt
- source/ui/src/test/java/com/clerk/ui/auth/AuthStateSignUpRoutingTest.kt
- source/api/src/test/java/com/clerk/api/magiclink/MagicLinkDeepLinkParserTest.kt
- source/api/src/main/kotlin/com/clerk/api/network/api/MagicLinkApi.kt
- source/ui/src/test/java/com/clerk/ui/signin/code/SignInFactorCodeViewModelTest.kt
- source/api/src/test/java/com/clerk/api/sso/SSOManagerActivityTest.kt
- source/api/src/main/kotlin/com/clerk/api/Constants.kt
- source/api/src/main/kotlin/com/clerk/api/network/ClerkApi.kt
- source/api/src/test/java/com/clerk/api/log/SafeUriLogTest.kt
- workbench/src/debug/AndroidManifest.xml
- source/api/src/test/java/com/clerk/api/signin/SignInExtensionsTest.kt
- source/ui/src/main/java/com/clerk/ui/signup/code/SignUpCodeViewModel.kt
- source/ui/src/main/java/com/clerk/ui/core/scaffold/ClerkThemedAuthScaffold.kt
- source/api/src/main/kotlin/com/clerk/api/network/ApiParams.kt
- workbench/src/main/java/com/clerk/workbench/HomeView.kt
- source/api/src/main/kotlin/com/clerk/api/magiclink/PkceUtil.kt
- source/api/src/main/AndroidManifest.xml
- source/ui/src/main/res/values/strings.xml
- source/ui/src/main/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkViewModel.kt
- source/api/src/main/kotlin/com/clerk/api/network/model/magiclink/NativeMagicLinkRequests.kt
- workbench/src/debug/res/xml/workbench_debug_network_security_config.xml
- source/ui/src/main/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkView.kt
- source/ui/src/main/java/com/clerk/ui/util/EmailAppLauncher.kt
source/api/src/test/java/com/clerk/api/signup/SignUpEmailVerificationStrategyTest.kt
Show resolved
Hide resolved
| import androidx.compose.ui.graphics.Color | ||
| import androidx.compose.ui.tooling.preview.PreviewLightDark | ||
| import androidx.compose.ui.unit.Dp | ||
| import com.clerk.api.Clerk |
Contributor
There was a problem hiding this comment.
Read logo state from LocalAuthState, not Clerk.organizationLogoUrl.
This moves profile logo visibility onto a singleton read that Compose does not observe, so the top bar can stay stale or hide the instance logo incorrectly when the auth/org context changes. Please derive the logo URL from LocalAuthState and compute shouldShowLogo from that reactive state instead of Clerk.organizationLogoUrl. As per coding guidelines, "Use LocalAuthState for accessing authentication state in UI components".
Also applies to: 44-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@source/ui/src/main/java/com/clerk/ui/core/scaffold/ClerkThemedProfileScaffold.kt`
at line 20, The scaffold currently reads the org logo directly from
Clerk.organizationLogoUrl (non-reactive) which can go stale; instead obtain the
auth state via LocalAuthState.current inside ClerkThemedProfileScaffold and
derive the logo URL from that (e.g., authState.organizationLogoUrl) and compute
shouldShowLogo from that reactive value. Locate references to
Clerk.organizationLogoUrl in ClerkThemedProfileScaffold (top bar / profile logo
logic) and replace them to read LocalAuthState.current, then use that reactive
property to control visibility and rendering of the logo.
swolfand
added a commit
that referenced
this pull request
Mar 11, 2026
swolfand
added a commit
that referenced
this pull request
Mar 11, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.kt (1)
120-129:⚠️ Potential issue | 🔴 CriticalMagic link result ignored;
RESULT_OKset prematurely.
setResult(RESULT_OK)is called at line 124 before knowing whether the magic link handling succeeds. The return value ofhandleMagicLinkDeepLink(uri)(aClerkResult<SignIn, NativeMagicLinkError>) is completely ignored. If the magic link fails (e.g., missingapproval_tokenperparseMagicLinkCallbackvalidation), the caller still observesRESULT_OK, creating a silent authentication failure.Additionally, per context snippet 3,
canHandleNativeMagicLink()accepts URIs with only one offlow_id/approval_token, butparseMagicLinkCallback()requires both—so a partial URI routes here but then fails, and that failure is swallowed.Proposed fix: handle the result before setting success
private fun authorizationComplete(uri: Uri) { lifecycleScope.launch { try { - // Mark the Activity result as success so callers don't observe RESULT_CANCELED - setResult(RESULT_OK, Intent()) if (canHandleNativeMagicLink(uri)) { ClerkLog.d("authorizationComplete called with native magic link redirect: $uri") - NativeMagicLinkService.handleMagicLinkDeepLink(uri) - return@launch + val result = NativeMagicLinkService.handleMagicLinkDeepLink(uri) + when (result) { + is ClerkResult.Success -> setResult(RESULT_OK, Intent()) + is ClerkResult.Failure -> { + ClerkLog.e("Magic link handling failed: ${result.error}") + setResult(RESULT_CANCELED, Intent()) + } + } + return@launch } + // Mark the Activity result as success for other flows + setResult(RESULT_OK, Intent()) if (SSOService.hasPendingExternalAccountConnection()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.kt` around lines 120 - 129, authorizationComplete currently calls setResult(RESULT_OK, ...) before actually handling the native magic link and ignores the return value of NativeMagicLinkService.handleMagicLinkDeepLink(uri), which can fail because canHandleNativeMagicLink may accept partial URIs while parseMagicLinkCallback requires both flow_id and approval_token; change authorizationComplete to first call NativeMagicLinkService.handleMagicLinkDeepLink(uri), inspect the ClerkResult<SignIn, NativeMagicLinkError> it returns, and only call setResult(RESULT_OK, Intent()) and return on success; on failure, set an appropriate non-success result (e.g., RESULT_CANCELED or include error data) and ensure the failure path is not swallowed so callers observe the real outcome instead of always receiving RESULT_OK.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@source/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.kt`:
- Around line 190-218: canHandleNativeMagicLink() and complete() currently
accept any callback with flow_id/approval_token and then use the single stored
pending flow, which lets callbacks for different flows corrupt or clear the
wrong pendingFlowStore; update the logic to correlate the incoming callback's
flow_id to the persisted PendingNativeMagicLinkFlow.flowId before running
completion (or alternatively change pendingFlowStore to index multiple
PendingNativeMagicLinkFlow entries by flowId), so: ensure flowId is written when
creating/storing a PendingNativeMagicLinkFlow, modify canHandleNativeMagicLink()
to validate the callback URI matches that stored flowId, and in complete() only
invoke NativeMagicLinkCompletionRunner.complete (and call clearPendingFlow) for
the matching pending flow (do not clear unrelated flows); reference
functions/fields: canHandleNativeMagicLink(), complete(),
PendingNativeMagicLinkFlow.flowId, pendingFlowStore, clearPendingFlow(), and
NativeMagicLinkCompletionRunner.complete.
---
Outside diff comments:
In `@source/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.kt`:
- Around line 120-129: authorizationComplete currently calls
setResult(RESULT_OK, ...) before actually handling the native magic link and
ignores the return value of NativeMagicLinkService.handleMagicLinkDeepLink(uri),
which can fail because canHandleNativeMagicLink may accept partial URIs while
parseMagicLinkCallback requires both flow_id and approval_token; change
authorizationComplete to first call
NativeMagicLinkService.handleMagicLinkDeepLink(uri), inspect the
ClerkResult<SignIn, NativeMagicLinkError> it returns, and only call
setResult(RESULT_OK, Intent()) and return on success; on failure, set an
appropriate non-success result (e.g., RESULT_CANCELED or include error data) and
ensure the failure path is not swallowed so callers observe the real outcome
instead of always receiving RESULT_OK.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23081113-953f-4151-9c91-afb6c56fa5c4
📒 Files selected for processing (8)
source/api/src/main/kotlin/com/clerk/api/auth/Auth.ktsource/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.ktsource/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.ktsource/api/src/main/kotlin/com/clerk/api/storage/StorageHelper.ktsource/api/src/test/java/com/clerk/api/auth/AuthHandleTest.ktsource/api/src/test/java/com/clerk/api/auth/AuthOtpTest.ktsource/api/src/test/java/com/clerk/api/magiclink/NativeMagicLinkServiceTest.ktsource/api/src/test/java/com/clerk/api/magiclink/PersistentPendingNativeMagicLinkStoreTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- source/api/src/test/java/com/clerk/api/magiclink/NativeMagicLinkServiceTest.kt
Comment on lines
+190
to
+218
| override suspend fun complete( | ||
| flowId: String, | ||
| approvalToken: String, | ||
| ): ClerkResult<SignIn, NativeMagicLinkError> { | ||
| val pending = | ||
| mutex.withLock { | ||
| val flow = pendingFlowStore.load() | ||
| if (flow != null && flow.expiresAtEpochMs <= currentTimeMillis()) { | ||
| pendingFlowStore.clear() | ||
| null | ||
| } else { | ||
| flow | ||
| } | ||
| } | ||
|
|
||
| val clearPendingFlow: suspend () -> Unit = { mutex.withLock { pendingFlowStore.clear() } } | ||
| val completionRunner = | ||
| NativeMagicLinkCompletionRunner( | ||
| attestationProvider = attestationProvider, | ||
| clearPendingFlow = clearPendingFlow, | ||
| activateCreatedSession = { signIn -> activateCreatedSession(signIn) }, | ||
| refreshClientState = { refreshClientState() }, | ||
| ) | ||
| return if (pending == null) { | ||
| nativeMagicLinkFailure(NativeMagicLinkReason.NO_PENDING_FLOW) | ||
| } else { | ||
| completionRunner.complete(flowId = flowId, approvalToken = approvalToken, pending = pending) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Correlate callbacks to the persisted flow before completing or clearing.
canHandleNativeMagicLink() currently treats any URI with flow_id/approval_token as a magic-link callback, and complete() then uses whichever single pending verifier is currently stored. That breaks the flow as soon as callbacks arrive out of order or from the wrong origin: e.g. start link A, start link B, then open email A — this code submits B’s verifier with A’s callback, hits a terminal PKCE/token failure, and clearPendingFlow() deletes B as well. PendingNativeMagicLinkFlow.flowId already exists here but never gets populated or checked.
Please validate the expected Clerk callback URI before routing to native magic-link completion, and correlate the callback to the persisted flowId (or store multiple pending flows keyed by flowId) before completing/clearing. Otherwise a stale or crafted deep link can invalidate the active login attempt.
Also applies to: 263-269, 322-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.kt`
around lines 190 - 218, canHandleNativeMagicLink() and complete() currently
accept any callback with flow_id/approval_token and then use the single stored
pending flow, which lets callbacks for different flows corrupt or clear the
wrong pendingFlowStore; update the logic to correlate the incoming callback's
flow_id to the persisted PendingNativeMagicLinkFlow.flowId before running
completion (or alternatively change pendingFlowStore to index multiple
PendingNativeMagicLinkFlow entries by flowId), so: ensure flowId is written when
creating/storing a PendingNativeMagicLinkFlow, modify canHandleNativeMagicLink()
to validate the callback URI matches that stored flowId, and in complete() only
invoke NativeMagicLinkCompletionRunner.complete (and call clearPendingFlow) for
the matching pending flow (do not clear unrelated flows); reference
functions/fields: canHandleNativeMagicLink(), complete(),
PendingNativeMagicLinkFlow.flowId, pendingFlowStore, clearPendingFlow(), and
NativeMagicLinkCompletionRunner.complete.
Implement native PKCE email-link support across Auth API and AuthView for sign-in and sign-up. Prioritize email_link when available, complete native deep links through the magic-link endpoint, and surface backend error codes like too_many_requests in UI flows. Also harden redirect handling and callback parsing for non-default proxy ports, add safe URI logging, and include regression tests for native magic-link completion, factor routing, and sign-up verification strategies.
Add a native 'Open email app' action for sign-in and sign-up email-link screens and preserve a non-compose fallback when no email client is available. Update the check-email layout to match the intended hierarchy with secondary primary action, text actions for resend/use another method, and sign-up back navigation parity.
Treat logo visibility as the combination of instance logo support and a non-blank logo URL in both auth and profile scaffolds. Add regression tests for logo visibility decisions so instances without branding assets no longer render an empty logo slot.
Make user-button visibility session-driven in pending flows, add avatar fallbacks for null/error image loads, and align workbench auth surfaces with session state to avoid blank signed-in screens. Use a transparent bridge theme for auth callback activities so deep-link completion no longer presents a black transition screen.
Replace direct Clerk singleton reads in ClerkTelemetryEnvironment with injected provider lambdas and wire them from CompositionLocals. This keeps telemetry event emission behavior intact while reducing static coupling in telemetry setup.
Store the sign-in/sign-up flow ID alongside the PKCE verifier in PendingNativeMagicLinkFlow. On completion, validate that the callback's flow ID matches the pending flow; mismatches return a new FLOW_ID_MISMATCH error without consuming the pending state, so the original flow can still complete.
Store the callback URI in a field (and in savedInstanceState) so it survives configuration changes and process-death restores. Set RESULT_CANCELED when magic link handling or SSO completion fails so callers can distinguish success from failure. Also fixes test isolation in SignUpEmailVerificationStrategyTest by saving/restoring Clerk.environment.
Swap resolution order so email-link takes priority over a prepared email-code factor, preventing the code screen from appearing when an email-link flow is available. Factor matching now compares identifiers (emailAddressId, phoneNumberId, etc.) in addition to strategy, so a factor with a different identifier is correctly treated as unsupported.
Second-factor prepare failures for phone-code and email-code were silently logged. Now they call onError with the error message so the UI can display a meaningful error to the user.
Use an effectiveSession that falls back to Clerk.session when the StateFlow hasn't emitted yet, and derive the displayed user from the active session rather than relying on separate Clerk.activeUser state. This prevents the button from flickering or disappearing when session state is still propagating after sign-in.
swolfand
force-pushed
the
sam/mobile-431-android-changes
branch
from
19e07f6 to
78062bb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
source/ui/src/main/java/com/clerk/ui/userbutton/UserButton.kt (1)
4-6:⚠️ Potential issue | 🔴 CriticalMissing import for
matchParentSizewill cause compilation failure.Lines 184 and 191 use
Modifier.matchParentSize()but the required import is not present.Proposed fix
import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.matchParentSize import androidx.compose.foundation.layout.size🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/src/main/java/com/clerk/ui/userbutton/UserButton.kt` around lines 4 - 6, The file is missing the import for Modifier.matchParentSize(), causing compilation failure where Modifier.matchParentSize() is used in UserButton.kt (lines around the UserButton composable); add the appropriate Jetpack Compose import for matchParentSize to the top imports (e.g., import androidx.compose.ui.Modifier.matchParentSize) so the calls to Modifier.matchParentSize() compile. Ensure the import is added alongside the existing layout imports (Box, fillMaxSize, size) in UserButton.kt.workbench/src/main/java/com/clerk/workbench/WorkbenchApplication.kt (1)
15-23:⚠️ Potential issue | 🔴 CriticalRemove hardcoded
proxyUrl; it can break auth against non-matching environments.Line 21 pins all runs to one proxy host. If the selected
publicKeybelongs to another instance/environment, auth traffic is routed incorrectly and flows can fail.Minimal safe fix
Clerk.initialize( this, key, options = ClerkConfigurationOptions( enableDebugMode = true, - proxyUrl = "https://rapid-earwig-10.clerk.accounts.lclclerk.com:8443", ), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workbench/src/main/java/com/clerk/workbench/WorkbenchApplication.kt` around lines 15 - 23, The code hardcodes proxyUrl in the Clerk.initialize call which forces all runs to one proxy and can break auth; remove the fixed proxyUrl argument from the ClerkConfigurationOptions or replace it with a dynamic value read from configuration/env (e.g., a BuildConfig/ENV variable or a lookup based on the selected publicKey) so proxy selection matches the runtime environment; update the Clerk.initialize invocation and ClerkConfigurationOptions usage accordingly to accept no proxyUrl or a runtime-sourced proxy URL.source/ui/src/main/java/com/clerk/ui/signin/code/SignInFactorCodeViewModel.kt (1)
143-156:⚠️ Potential issue | 🔴 CriticalOnly reroute
EMAIL_CODEto the matchingEMAIL_LINK.
shouldPreferEmailLink()returns true for any email-code factor as soon as the sign-in has any email-link factor, because the helper never verifies that the link matches the selected factor. In multi-email accounts this can skip preparation for the chosen address and reroute the user to a different inbox; the same heuristic can also divert non-email flows once an email factor exists. Compare the candidateEMAIL_LINKagainst the selected factor’s concrete identity before short-circuiting.Also applies to: 159-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/src/main/java/com/clerk/ui/signin/code/SignInFactorCodeViewModel.kt` around lines 143 - 156, The current prefersEmailLinkOverEmailCode check (used in SignInFactorCodeViewModel.kt around the prefersEmailLinkOverEmailCode variable and the return logic) short-circuits to any EMAIL_LINK when any email-link factor exists; modify the logic so shouldPreferEmailLink(...) only returns true if the candidate EMAIL_LINK matches the selected factor's concrete identity (e.g., same email address/id) instead of just checking StrategyKeys.EMAIL_CODE vs EMAIL_LINK—ensure you compare the candidate factor (from signIn.supportedFirstFactors or supportedSecondFactors where applicable) against the selected factor via matches(factor) or an explicit identity check before preferring EMAIL_LINK, and apply the same fix to the analogous block at lines 159-174.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@source/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkCompletionRunner.kt`:
- Around line 65-70: The failure branch in NativeMagicLinkCompletionRunner uses
activationResult.error?.reasonCode but then passes activationResult.error
directly to ClerkResult.apiFailure which can be null; update the failure path to
ensure a non-null error is passed to ClerkResult.apiFailure (for example by
constructing a default error object using the computed
reasonCode/NativeMagicLinkReason.SESSION_ACTIVATION_FAILED.code) before calling
NativeMagicLinkLogger.completeFailure and ClerkResult.apiFailure, referencing
activationResult, clearPendingFlow(),
NativeMagicLinkLogger.completeFailure(...), and ClerkResult.apiFailure(...) to
locate where to insert this fallback.
In `@source/api/src/main/kotlin/com/clerk/api/signin/SignInExtensions.kt`:
- Around line 108-116: The selection logic should match the concrete factor
identity rather than any factor that "looks like" an email: update the branches
that use isEmailIdentifier (and similar blocks around
preparedFirstFactor/startingFirstFactor and the other noted ranges) to filter
availableFirstFactors by the prepared/current factor's identity fields (e.g.,
compare emailAddressId, phoneNumberId or safeIdentifier) before matching
strategy like EMAIL_LINK, instead of picking the first factor with that
strategy; specifically, when preparedFirstFactor or startingFirstFactor is
present, find the factor in availableFirstFactors whose strategy equals
EMAIL_LINK/PHONE_... and whose emailAddressId/phoneNumberId/safeIdentifier
equals the prepared factor's corresponding id, and return that factor (apply
same change to the blocks referenced at 140-148 and 162-179).
In `@source/api/src/main/kotlin/com/clerk/api/signup/SignUpExtensions.kt`:
- Around line 35-45: Add unit tests that exercise the new auth-strategy
branching in SignUpExtensions: create tests that (1) simulate
isEmailLinkVerificationSupported == true and verify prepareVerification is
called/behaves with SignUp.PrepareVerificationParams.Strategy.EmailLink, (2)
simulate isEmailLinkVerificationSupported == false and verify the fallback uses
SignUp.PrepareVerificationParams.Strategy.EmailCode, and (3) test the phone flow
where the code path returns SignUp.PrepareVerificationParams.Strategy.PhoneCode;
assert expected outputs/side-effects from prepareVerification(strategy) and mock
or spy on prepareVerification (or the surrounding SignUp logic) to validate the
chosen Strategy for each case.
In
`@source/api/src/test/java/com/clerk/api/integration/IntegrationTestHelpers.kt`:
- Around line 65-71: The area code can become negative because converting the
masked Long via .toInt() keeps high bits that set the sign; replace the current
mask with a 32-bit positive mask so the Int is always non-negative: change the
computation of areaSeed to mask with 0x7FFFFFFFL (e.g. areaSeed =
(seed.mostSignificantBits and 0x7FFFFFFFL).toInt()) and keep the rest
(firstDigit, secondDigit, thirdDigit, suffix, and the return string) unchanged
so digit arithmetic uses a non-negative areaSeed.
In
`@source/ui/src/test/java/com/clerk/ui/signin/code/SignInPrepareHandlerTest.kt`:
- Line 106: The tests in SignInPrepareHandlerTest.kt use Kotlin's assert(...)
which is disabled by default; replace each Kotlin assert call with the
appropriate JUnit Jupiter assertion: for checks comparing messages use
assertEquals(expectedMessage, capturedMessage) (replace every
assert(capturedMessage == "...") pattern), and for boolean flags named called
use assertFalse(called) (or assertTrue(...) where the test expects true). Also
add the JUnit import (org.junit.jupiter.api.Assertions.*) if not present and
update all occurrences in the file (variables: capturedMessage, called) to
ensure assertions run under the test runner.
---
Duplicate comments:
In
`@source/ui/src/main/java/com/clerk/ui/signin/code/SignInFactorCodeViewModel.kt`:
- Around line 143-156: The current prefersEmailLinkOverEmailCode check (used in
SignInFactorCodeViewModel.kt around the prefersEmailLinkOverEmailCode variable
and the return logic) short-circuits to any EMAIL_LINK when any email-link
factor exists; modify the logic so shouldPreferEmailLink(...) only returns true
if the candidate EMAIL_LINK matches the selected factor's concrete identity
(e.g., same email address/id) instead of just checking StrategyKeys.EMAIL_CODE
vs EMAIL_LINK—ensure you compare the candidate factor (from
signIn.supportedFirstFactors or supportedSecondFactors where applicable) against
the selected factor via matches(factor) or an explicit identity check before
preferring EMAIL_LINK, and apply the same fix to the analogous block at lines
159-174.
In `@source/ui/src/main/java/com/clerk/ui/userbutton/UserButton.kt`:
- Around line 4-6: The file is missing the import for
Modifier.matchParentSize(), causing compilation failure where
Modifier.matchParentSize() is used in UserButton.kt (lines around the UserButton
composable); add the appropriate Jetpack Compose import for matchParentSize to
the top imports (e.g., import androidx.compose.ui.Modifier.matchParentSize) so
the calls to Modifier.matchParentSize() compile. Ensure the import is added
alongside the existing layout imports (Box, fillMaxSize, size) in UserButton.kt.
In `@workbench/src/main/java/com/clerk/workbench/WorkbenchApplication.kt`:
- Around line 15-23: The code hardcodes proxyUrl in the Clerk.initialize call
which forces all runs to one proxy and can break auth; remove the fixed proxyUrl
argument from the ClerkConfigurationOptions or replace it with a dynamic value
read from configuration/env (e.g., a BuildConfig/ENV variable or a lookup based
on the selected publicKey) so proxy selection matches the runtime environment;
update the Clerk.initialize invocation and ClerkConfigurationOptions usage
accordingly to accept no proxyUrl or a runtime-sourced proxy URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76cf6651-e9f9-4fd6-a0b2-2f631ed72b1b
📒 Files selected for processing (70)
source/api/src/main/AndroidManifest.xmlsource/api/src/main/kotlin/com/clerk/api/Constants.ktsource/api/src/main/kotlin/com/clerk/api/auth/Auth.ktsource/api/src/main/kotlin/com/clerk/api/log/SafeUriLog.ktsource/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkCompletionRunner.ktsource/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.ktsource/api/src/main/kotlin/com/clerk/api/magiclink/PkceUtil.ktsource/api/src/main/kotlin/com/clerk/api/network/ApiParams.ktsource/api/src/main/kotlin/com/clerk/api/network/ApiPaths.ktsource/api/src/main/kotlin/com/clerk/api/network/ClerkApi.ktsource/api/src/main/kotlin/com/clerk/api/network/api/MagicLinkApi.ktsource/api/src/main/kotlin/com/clerk/api/network/api/SignUpApi.ktsource/api/src/main/kotlin/com/clerk/api/network/model/magiclink/NativeMagicLinkRequests.ktsource/api/src/main/kotlin/com/clerk/api/network/serialization/ClerkApiResultConverterFactory.ktsource/api/src/main/kotlin/com/clerk/api/signin/SignIn.ktsource/api/src/main/kotlin/com/clerk/api/signin/SignInExtensions.ktsource/api/src/main/kotlin/com/clerk/api/signup/SignUp.ktsource/api/src/main/kotlin/com/clerk/api/signup/SignUpExtensions.ktsource/api/src/main/kotlin/com/clerk/api/sso/RedirectConfiguration.ktsource/api/src/main/kotlin/com/clerk/api/sso/SSOManagerActivity.ktsource/api/src/main/kotlin/com/clerk/api/storage/StorageHelper.ktsource/api/src/main/res/values/themes.xmlsource/api/src/test/java/com/clerk/api/auth/AuthHandleTest.ktsource/api/src/test/java/com/clerk/api/auth/AuthOtpTest.ktsource/api/src/test/java/com/clerk/api/integration/AuthIntegrationTests.ktsource/api/src/test/java/com/clerk/api/integration/IntegrationTestHelpers.ktsource/api/src/test/java/com/clerk/api/log/SafeUriLogTest.ktsource/api/src/test/java/com/clerk/api/magiclink/MagicLinkDeepLinkParserTest.ktsource/api/src/test/java/com/clerk/api/magiclink/NativeMagicLinkErrorMappingTest.ktsource/api/src/test/java/com/clerk/api/magiclink/NativeMagicLinkServiceTest.ktsource/api/src/test/java/com/clerk/api/magiclink/PersistentPendingNativeMagicLinkStoreTest.ktsource/api/src/test/java/com/clerk/api/magiclink/PkceUtilTest.ktsource/api/src/test/java/com/clerk/api/signin/SignInExtensionsTest.ktsource/api/src/test/java/com/clerk/api/signup/SignUpEmailVerificationStrategyTest.ktsource/api/src/test/java/com/clerk/api/sso/RedirectConfigurationTest.ktsource/api/src/test/java/com/clerk/api/sso/SSOManagerActivityTest.ktsource/telemetry/src/androidMain/kotlin/com/clerk/telemetry/ClerkTelemetryEnvironment.ktsource/telemetry/src/commonMain/kotlin/com/clerk/telemetry/TelemetryCollector.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthStartView.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthState.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthView.ktsource/ui/src/main/java/com/clerk/ui/core/common/StrategyKeys.ktsource/ui/src/main/java/com/clerk/ui/core/composition/CompositionLocals.ktsource/ui/src/main/java/com/clerk/ui/core/scaffold/ClerkThemedAuthScaffold.ktsource/ui/src/main/java/com/clerk/ui/core/scaffold/ClerkThemedProfileScaffold.ktsource/ui/src/main/java/com/clerk/ui/core/scaffold/LogoVisibility.ktsource/ui/src/main/java/com/clerk/ui/signin/SignInFactorOneView.ktsource/ui/src/main/java/com/clerk/ui/signin/code/SignInFactorCodeViewModel.ktsource/ui/src/main/java/com/clerk/ui/signin/code/SignInPrepareHandler.ktsource/ui/src/main/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkView.ktsource/ui/src/main/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkViewModel.ktsource/ui/src/main/java/com/clerk/ui/signup/code/SignUpCodeViewModel.ktsource/ui/src/main/java/com/clerk/ui/signup/emaillink/SignUpEmailLinkView.ktsource/ui/src/main/java/com/clerk/ui/signup/emaillink/SignUpEmailLinkViewModel.ktsource/ui/src/main/java/com/clerk/ui/userbutton/UserButton.ktsource/ui/src/main/java/com/clerk/ui/util/EmailAppLauncher.ktsource/ui/src/main/java/com/clerk/ui/util/TextIconHelper.ktsource/ui/src/main/res/values/strings.xmlsource/ui/src/test/java/com/clerk/ui/auth/AuthStateSignUpRoutingTest.ktsource/ui/src/test/java/com/clerk/ui/core/scaffold/LogoVisibilityTest.ktsource/ui/src/test/java/com/clerk/ui/signin/SignInFactorOneViewTest.ktsource/ui/src/test/java/com/clerk/ui/signin/code/SignInFactorCodeViewModelTest.ktsource/ui/src/test/java/com/clerk/ui/signin/code/SignInPrepareHandlerTest.ktsource/ui/src/test/java/com/clerk/ui/signin/emaillink/SignInFactorOneEmailLinkViewModelTest.ktsource/ui/src/test/java/com/clerk/ui/userbutton/UserButtonBehaviorTest.ktworkbench/src/debug/AndroidManifest.xmlworkbench/src/debug/res/xml/workbench_debug_network_security_config.xmlworkbench/src/main/java/com/clerk/workbench/HomeView.ktworkbench/src/main/java/com/clerk/workbench/UiActivity2.ktworkbench/src/main/java/com/clerk/workbench/WorkbenchApplication.kt
💤 Files with no reviewable changes (1)
- source/api/src/test/java/com/clerk/api/integration/AuthIntegrationTests.kt
Comment on lines
+65
to
+70
| return if (activationResult is ClerkResult.Failure) { | ||
| clearPendingFlow() | ||
| val reasonCode = | ||
| activationResult.error?.reasonCode ?: NativeMagicLinkReason.SESSION_ACTIVATION_FAILED.code | ||
| NativeMagicLinkLogger.completeFailure(reasonCode) | ||
| ClerkResult.apiFailure(activationResult.error) |
Contributor
There was a problem hiding this comment.
Potential null error passed to apiFailure when activationResult.error is null.
Line 67 handles potential null via activationResult.error?.reasonCode, but line 70 passes activationResult.error directly to apiFailure. If the error is null, the caller receives a Failure with null error, which may cause downstream issues.
Proposed fix
private suspend fun completeAfterTicketSignIn(
signIn: SignIn
): ClerkResult<SignIn, NativeMagicLinkError> {
val activationResult = activateCreatedSession(signIn)
return if (activationResult is ClerkResult.Failure) {
clearPendingFlow()
val reasonCode =
activationResult.error?.reasonCode ?: NativeMagicLinkReason.SESSION_ACTIVATION_FAILED.code
NativeMagicLinkLogger.completeFailure(reasonCode)
- ClerkResult.apiFailure(activationResult.error)
+ ClerkResult.apiFailure(
+ activationResult.error ?: NativeMagicLinkError(reasonCode = reasonCode)
+ )
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@source/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkCompletionRunner.kt`
around lines 65 - 70, The failure branch in NativeMagicLinkCompletionRunner uses
activationResult.error?.reasonCode but then passes activationResult.error
directly to ClerkResult.apiFailure which can be null; update the failure path to
ensure a non-null error is passed to ClerkResult.apiFailure (for example by
constructing a default error object using the computed
reasonCode/NativeMagicLinkReason.SESSION_ACTIVATION_FAILED.code) before calling
NativeMagicLinkLogger.completeFailure and ClerkResult.apiFailure, referencing
activationResult, clearPendingFlow(),
NativeMagicLinkLogger.completeFailure(...), and ClerkResult.apiFailure(...) to
locate where to insert this fallback.
Comment on lines
+108
to
+116
| val availableFirstFactors = supportedFirstFactors ?: return null | ||
|
|
||
| if (isEmailIdentifier) { | ||
| availableFirstFactors | ||
| .firstOrNull { it.strategy == EMAIL_LINK } | ||
| ?.let { | ||
| return it | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Match first-factor selection to the actual factor identity.
These branches prefer EMAIL_LINK whenever isEmailIdentifier is true, but isEmailIdentifier becomes true if any supported factor looks like an email, and preparedFirstFactor then falls back to the first factor with the same strategy. On username/phone sign-ins or accounts with multiple email factors, startingFirstFactor can jump to EMAIL_LINK or surface the wrong prepared factor for a different address. Scope this to the current/prepared factor’s concrete identity (emailAddressId / phoneNumberId / safeIdentifier) instead of any email-looking factor.
Also applies to: 140-148, 162-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/api/src/main/kotlin/com/clerk/api/signin/SignInExtensions.kt` around
lines 108 - 116, The selection logic should match the concrete factor identity
rather than any factor that "looks like" an email: update the branches that use
isEmailIdentifier (and similar blocks around
preparedFirstFactor/startingFirstFactor and the other noted ranges) to filter
availableFirstFactors by the prepared/current factor's identity fields (e.g.,
compare emailAddressId, phoneNumberId or safeIdentifier) before matching
strategy like EMAIL_LINK, instead of picking the first factor with that
strategy; specifically, when preparedFirstFactor or startingFirstFactor is
present, find the factor in availableFirstFactors whose strategy equals
EMAIL_LINK/PHONE_... and whose emailAddressId/phoneNumberId/safeIdentifier
equals the prepared factor's corresponding id, and return that factor (apply
same change to the blocks referenced at 140-148 and 162-179).
Comment on lines
+35
to
45
| if (isEmailLinkVerificationSupported) { | ||
| SignUp.PrepareVerificationParams.Strategy.EmailLink() | ||
| } else { | ||
| SignUp.PrepareVerificationParams.Strategy.EmailCode() | ||
| } | ||
| } else { | ||
| SignUp.PrepareVerificationParams.Strategy.PhoneCode() | ||
| } | ||
|
|
||
| return ClerkApi.signUp.prepareSignUpVerification(this.id, strategy.strategy) | ||
| return prepareVerification(strategy) | ||
| } |
Contributor
There was a problem hiding this comment.
Add tests for the new auth-strategy branch before merge.
Line 35 introduces email-link-vs-email-code branching and Line 44 changes the verification call path, but no tests were added/updated in this PR segment for these paths. Please add coverage for: (1) email link supported, (2) email link unsupported fallback to email code, and (3) phone flow.
As per coding guidelines, "**/*: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/api/src/main/kotlin/com/clerk/api/signup/SignUpExtensions.kt` around
lines 35 - 45, Add unit tests that exercise the new auth-strategy branching in
SignUpExtensions: create tests that (1) simulate
isEmailLinkVerificationSupported == true and verify prepareVerification is
called/behaves with SignUp.PrepareVerificationParams.Strategy.EmailLink, (2)
simulate isEmailLinkVerificationSupported == false and verify the fallback uses
SignUp.PrepareVerificationParams.Strategy.EmailCode, and (3) test the phone flow
where the code path returns SignUp.PrepareVerificationParams.Strategy.PhoneCode;
assert expected outputs/side-effects from prepareVerification(strategy) and mock
or spy on prepareVerification (or the surrounding SignUp logic) to validate the
chosen Strategy for each case.
Comment on lines
+65
to
+71
| val seed = UUID.randomUUID() | ||
| val areaSeed = (seed.mostSignificantBits and Long.MAX_VALUE).toInt() | ||
| val firstDigit = 2 + (areaSeed % 8) | ||
| val secondDigit = (areaSeed / 8) % 10 | ||
| val thirdDigit = (areaSeed / 80) % 10 | ||
| val suffix = ((seed.leastSignificantBits and 0x7FFF).toInt()) % 100 | ||
| return "+1${firstDigit}${secondDigit}${thirdDigit}55501%02d".format(suffix) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies that current arithmetic can produce invalid area-code digits.
python - <<'PY'
import uuid
def to_int32(x):
x &= 0xFFFFFFFF
return x - 0x100000000 if x & 0x80000000 else x
def kdiv(a, b): # Kotlin/Java integer division: trunc toward zero
return int(a / b)
def krem(a, b): # Kotlin/Java remainder: sign follows dividend
return a - kdiv(a, b) * b
bad = 0
for _ in range(200000):
u = uuid.uuid4().int
msb = (u >> 64) & ((1 << 64) - 1)
area_seed_long = msb & ((1 << 63) - 1) # Long.MAX_VALUE mask
area_seed_int = to_int32(area_seed_long)
first = 2 + krem(area_seed_int, 8)
second = krem(kdiv(area_seed_int, 8), 10)
third = krem(kdiv(area_seed_int, 80), 10)
if not (2 <= first <= 9 and 0 <= second <= 9 and 0 <= third <= 9):
bad += 1
if bad <= 3:
print("example -> area_seed_int:", area_seed_int, "digits:", first, second, third)
print("invalid_count:", bad)
PYRepository: clerk/clerk-android
Length of output: 247
Fix signed truncation in area code generation
Line 66 converts a masked Long to Int via .toInt(), which can produce negative values. Subsequent arithmetic with % and / preserves the negative sign, generating invalid phone number digits. This causes integration tests to fail with unpredictable phone numbers.
Proposed fix
fun generateTestPhone(): String {
val seed = UUID.randomUUID()
- val areaSeed = (seed.mostSignificantBits and Long.MAX_VALUE).toInt()
- val firstDigit = 2 + (areaSeed % 8)
- val secondDigit = (areaSeed / 8) % 10
- val thirdDigit = (areaSeed / 80) % 10
+ val areaSeed = seed.mostSignificantBits and Long.MAX_VALUE
+ val firstDigit = 2 + (areaSeed % 8L).toInt()
+ val secondDigit = ((areaSeed / 8L) % 10L).toInt()
+ val thirdDigit = ((areaSeed / 80L) % 10L).toInt()
val suffix = ((seed.leastSignificantBits and 0x7FFF).toInt()) % 100
return "+1${firstDigit}${secondDigit}${thirdDigit}55501%02d".format(suffix)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val seed = UUID.randomUUID() | |
| val areaSeed = (seed.mostSignificantBits and Long.MAX_VALUE).toInt() | |
| val firstDigit = 2 + (areaSeed % 8) | |
| val secondDigit = (areaSeed / 8) % 10 | |
| val thirdDigit = (areaSeed / 80) % 10 | |
| val suffix = ((seed.leastSignificantBits and 0x7FFF).toInt()) % 100 | |
| return "+1${firstDigit}${secondDigit}${thirdDigit}55501%02d".format(suffix) | |
| fun generateTestPhone(): String { | |
| val seed = UUID.randomUUID() | |
| val areaSeed = seed.mostSignificantBits and Long.MAX_VALUE | |
| val firstDigit = 2 + (areaSeed % 8L).toInt() | |
| val secondDigit = ((areaSeed / 8L) % 10L).toInt() | |
| val thirdDigit = ((areaSeed / 80L) % 10L).toInt() | |
| val suffix = ((seed.leastSignificantBits and 0x7FFF).toInt()) % 100 | |
| return "+1${firstDigit}${secondDigit}${thirdDigit}55501%02d".format(suffix) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/api/src/test/java/com/clerk/api/integration/IntegrationTestHelpers.kt`
around lines 65 - 71, The area code can become negative because converting the
masked Long via .toInt() keeps high bits that set the sign; replace the current
mask with a 32-bit positive mask so the Int is always non-negative: change the
computation of areaSeed to mask with 0x7FFFFFFFL (e.g. areaSeed =
(seed.mostSignificantBits and 0x7FFFFFFFL).toInt()) and keep the rest
(firstDigit, secondDigit, thirdDigit, suffix, and the return string) unchanged
so digit arithmetic uses a non-negative areaSeed.
| ) | ||
|
|
||
| coVerify { mockSignIn.prepareSecondFactor(emailAddressId = "email_123") } | ||
| assert(capturedMessage == "Email second factor failed") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find Kotlin assert(...) usages in tests:"
rg -n --type=kt '\bassert\s*\(' source workbench || true
echo
echo "2) Check whether JVM assertions are explicitly enabled in Gradle config:"
rg -n '(-ea|enableAssertions|jvmArgs)' . || trueRepository: clerk/clerk-android
Length of output: 691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if the file exists and read specific lines
FILE="source/ui/src/test/java/com/clerk/ui/signin/code/SignInPrepareHandlerTest.kt"
if [ -f "$FILE" ]; then
echo "=== File exists. Reading key lines ==="
echo
echo "Line 106:"
sed -n '106p' "$FILE"
echo
echo "Line 165:"
sed -n '165p' "$FILE"
echo
echo "Line 379:"
sed -n '379p' "$FILE"
echo
echo "Line 396:"
sed -n '396p' "$FILE"
echo
echo "=== Searching for 'assert' in the file ==="
rg -n 'assert' "$FILE" || echo "No 'assert' found"
else
echo "File not found at: $FILE"
# Try to find the file
echo "Searching for SignInPrepareHandlerTest.kt:"
find . -name "SignInPrepareHandlerTest.kt" 2>/dev/null || echo "File not found in repository"
fiRepository: clerk/clerk-android
Length of output: 1031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="source/ui/src/test/java/com/clerk/ui/signin/code/SignInPrepareHandlerTest.kt"
echo "=== Imports section of test file ==="
head -30 "$FILE"
echo
echo "=== Checking for JUnit imports ==="
rg 'import.*assert|import.*junit' "$FILE" || echo "No direct JUnit assert imports"
echo
echo "=== Checking build.gradle for test configuration ==="
find . -name "build.gradle*" -type f | head -5 | while read f; do
echo "File: $f"
rg -A 5 'test|tasks\.named.*test' "$f" | head -20
done
echo
echo "=== Checking for gradle test configuration with jvmArgs ==="
rg -B 2 -A 2 'jvmArgs|test.*jvmArgs' . || echo "No explicit test jvmArgs found"Repository: clerk/clerk-android
Length of output: 1376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading build.gradle.kts for test configuration ==="
head -200 ./build.gradle.kts | tail -100
echo
echo "=== Searching for test tasks in build.gradle.kts ==="
rg -n 'test\s*{' ./build.gradle.kts -A 10 || true
echo
echo "=== Checking for jvmArgs in entire build.gradle.kts ==="
rg -n 'jvmArgs|enableAssertions|-ea' ./build.gradle.kts || true
echo
echo "=== Check UI module's build.gradle ==="
if [ -f "source/ui/build.gradle.kts" ]; then
rg -n 'test|jvmArgs' "source/ui/build.gradle.kts" | head -20 || true
fiRepository: clerk/clerk-android
Length of output: 4038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full UI module build.gradle.kts ==="
cat source/ui/build.gradle.kts | grep -n "test" -i -A 3 -B 1
echo
echo "=== Checking if Kotlin assert is actually being used (no JUnit Assert import) ==="
echo "Import analysis from test file:"
grep -n "^import" source/ui/src/test/java/com/clerk/ui/signin/code/SignInPrepareHandlerTest.kt | grep -i "assert\|test" || echo "No Assert-related imports found"
echo
echo "=== Verify Kotlin stdlib assert is being called ==="
echo "Since no JUnit Assert import exists, the 'assert' calls are using Kotlin stdlib assert()"Repository: clerk/clerk-android
Length of output: 1422
Replace all Kotlin assert(...) calls with JUnit assertions in this test file.
Kotlin stdlib assert() is disabled by default in JVM without the -ea flag (which is not configured in build.gradle.kts). This means all 10 assertions in this file can silently skip, allowing failing tests to pass and masking bugs.
Proposed fix for all assertions
import org.junit.After
import org.junit.Before
import org.junit.Test
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Assert.assertFalseReplace all assertions:
- Lines 106, 165, 258, 286, 308, 330, 379, 396: Use
assertEquals(expectedMessage, capturedMessage) - Lines 346, 362: Use
assertFalse(called)orassertTrue(!called)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/ui/src/test/java/com/clerk/ui/signin/code/SignInPrepareHandlerTest.kt`
at line 106, The tests in SignInPrepareHandlerTest.kt use Kotlin's assert(...)
which is disabled by default; replace each Kotlin assert call with the
appropriate JUnit Jupiter assertion: for checks comparing messages use
assertEquals(expectedMessage, capturedMessage) (replace every
assert(capturedMessage == "...") pattern), and for boolean flags named called
use assertFalse(called) (or assertTrue(...) where the test expects true). Also
add the JUnit import (org.junit.jupiter.api.Assertions.*) if not present and
update all occurrences in the file (variables: capturedMessage, called) to
ensure assertions run under the test runner.
Make the workbench initialize Clerk from the saved publishable key and optional proxy URL instead of a baked-in local instance. This keeps native email-link flows on the same backend the user configured in settings. Also align native email-link redirects with the callback host and enrich magic-link logs with safe runtime context so approval-token failures can be diagnosed without exposing tokens.
Stop marking the auth identifier field as email-only in all cases. When both email and username are enabled, advertise both content types so Compose semantics match the actual accepted identifiers. Add focused helper tests for the email-only, username-only, and mixed identifier cases.
Prioritize required field collection over verification when a sign-up still has missing fields. This prevents the auth flow from auto-starting a native email link before required inputs like password have been collected. Add a routing regression test that asserts sign-up email-link flows stay on field collection until missing requirements are satisfied.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.kt (1)
203-220:⚠️ Potential issue | 🔴 CriticalPrevent
complete()from clearing a newer pending flow.
pendingStateis loaded under the mutex, but the callback created at Line 216 still does a blindpendingFlowStore.clear(). If another sign-in or sign-up flow saves a new pending record beforeNativeMagicLinkCompletionRunnerinvokes that callback, this completion path deletes the newer flow and the next deep link fails withno_pending_flow.🛠️ Suggested fix
- val clearPendingFlow: suspend () -> Unit = { mutex.withLock { pendingFlowStore.clear() } } - val completionRunner = - NativeMagicLinkCompletionRunner( - attestationProvider = attestationProvider, - clearPendingFlow = clearPendingFlow, - activateCreatedSession = { signIn -> activateCreatedSession(signIn) }, - refreshClientState = { refreshClientState() }, - ) return when (pendingState) { PendingFlowLookup.None -> nativeMagicLinkFailure(NativeMagicLinkReason.NO_PENDING_FLOW) is PendingFlowLookup.Mismatched -> { ClerkLog.w( "event=native_magic_link_flow_id_mismatch expected=${pendingState.expectedFlowId} actual=$flowId" ) nativeMagicLinkFailure(NativeMagicLinkReason.FLOW_ID_MISMATCH) } - is PendingFlowLookup.Found -> - completionRunner.complete( - flowId = flowId, - approvalToken = approvalToken, - pending = pendingState.flow, - ) + is PendingFlowLookup.Found -> { + val pending = pendingState.flow + val completionRunner = + NativeMagicLinkCompletionRunner( + attestationProvider = attestationProvider, + clearPendingFlow = { + mutex.withLock { + val current = pendingFlowStore.load() + if ( + current?.flowId == pending.flowId && + current.codeVerifier == pending.codeVerifier && + current.createdAtEpochMs == pending.createdAtEpochMs + ) { + pendingFlowStore.clear() + } + } + }, + activateCreatedSession = { signIn -> activateCreatedSession(signIn) }, + refreshClientState = { refreshClientState() }, + ) + completionRunner.complete( + flowId = flowId, + approvalToken = approvalToken, + pending = pending, + ) + } }Also applies to: 232-237, 246-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.kt` around lines 203 - 220, The clearPendingFlow callback currently unconditionally calls pendingFlowStore.clear() and can remove a newer flow; change it to check under the same mutex that the stored pending flow still matches the original pendingState (compare by flowId or other unique id captured when creating pendingState) before clearing. Update the closure created for clearPendingFlow (used by NativeMagicLinkCompletionRunner) to capture the original flowId/pending token and perform mutex.withLock { if (pendingFlowStore.load()?.flowId == originalFlowId) pendingFlowStore.clear() } (or clear only when pendingFlowStore.load() equals the captured pendingState) so newer flows are not deleted. Ensure the same pattern is applied to the other clear sites referenced (around lines creating NativeMagicLinkCompletionRunner and the other callbacks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workbench/src/main/java/com/clerk/workbench/MainActivity.kt`:
- Around line 57-63: The onSave handler in MainActivity.kt currently persists
raw values which can leave stale keys; normalize inputs by trimming both
publishableKey and proxyUrl, then if the trimmed publishableKey is blank call
StorageHelper.deleteValue(StorageKey.PUBLIC_KEY) else
StorageHelper.saveValue(StorageKey.PUBLIC_KEY, trimmedPublishableKey), and if
the trimmed proxyUrl is blank call
StorageHelper.deleteValue(StorageKey.PROXY_URL) else
StorageHelper.saveValue(StorageKey.PROXY_URL, trimmedProxyUrl); update the
onSave lambda where StorageHelper.saveValue/ deleteValue and
StorageKey.PUBLIC_KEY/StorageKey.PROXY_URL are used to implement this behavior.
---
Duplicate comments:
In
`@source/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.kt`:
- Around line 203-220: The clearPendingFlow callback currently unconditionally
calls pendingFlowStore.clear() and can remove a newer flow; change it to check
under the same mutex that the stored pending flow still matches the original
pendingState (compare by flowId or other unique id captured when creating
pendingState) before clearing. Update the closure created for clearPendingFlow
(used by NativeMagicLinkCompletionRunner) to capture the original flowId/pending
token and perform mutex.withLock { if (pendingFlowStore.load()?.flowId ==
originalFlowId) pendingFlowStore.clear() } (or clear only when
pendingFlowStore.load() equals the captured pendingState) so newer flows are not
deleted. Ensure the same pattern is applied to the other clear sites referenced
(around lines creating NativeMagicLinkCompletionRunner and the other callbacks).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d33c5d7a-071b-4e1d-b23d-035cd9fe7f9e
📒 Files selected for processing (10)
source/api/src/main/kotlin/com/clerk/api/magiclink/NativeMagicLinkManager.ktsource/api/src/main/kotlin/com/clerk/api/sso/RedirectConfiguration.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthStartView.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthStartViewHelper.ktsource/ui/src/main/java/com/clerk/ui/auth/AuthState.ktsource/ui/src/test/java/com/clerk/ui/auth/AuthStartViewHelperTest.ktsource/ui/src/test/java/com/clerk/ui/auth/AuthStateSignUpRoutingTest.ktworkbench/src/main/java/com/clerk/workbench/MainActivity.ktworkbench/src/main/java/com/clerk/workbench/PreferencesManager.ktworkbench/src/main/java/com/clerk/workbench/WorkbenchApplication.kt
Comment on lines
+57
to
+63
| onSave = { publishableKey, proxyUrl -> | ||
| StorageHelper.saveValue(StorageKey.PUBLIC_KEY, publishableKey) | ||
| if (proxyUrl.isBlank()) { | ||
| StorageHelper.deleteValue(StorageKey.PROXY_URL) | ||
| } else { | ||
| StorageHelper.saveValue(StorageKey.PROXY_URL, proxyUrl) | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize and safely persist settings before process restart.
Current save flow persists raw values. A blank publishableKey silently leaves the previous PUBLIC_KEY in storage, and an untrimmed proxyUrl can be persisted and later used as base URL. Both can restart into a stale/invalid auth backend configuration.
Proposed fix
MainContent(
onSave = { publishableKey, proxyUrl ->
- StorageHelper.saveValue(StorageKey.PUBLIC_KEY, publishableKey)
- if (proxyUrl.isBlank()) {
+ val normalizedKey = publishableKey.trim()
+ val normalizedProxy = proxyUrl.trim()
+
+ if (normalizedKey.isEmpty()) {
+ StorageHelper.deleteValue(StorageKey.PUBLIC_KEY)
+ } else {
+ StorageHelper.saveValue(StorageKey.PUBLIC_KEY, normalizedKey)
+ }
+
+ if (normalizedProxy.isEmpty()) {
StorageHelper.deleteValue(StorageKey.PROXY_URL)
} else {
- StorageHelper.saveValue(StorageKey.PROXY_URL, proxyUrl)
+ StorageHelper.saveValue(StorageKey.PROXY_URL, normalizedProxy)
}
ProcessPhoenix.triggerRebirth(context)
},As per coding guidelines, "Restrict feedback to errors, security risks, or functionality-breaking problems."
Also applies to: 249-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workbench/src/main/java/com/clerk/workbench/MainActivity.kt` around lines 57
- 63, The onSave handler in MainActivity.kt currently persists raw values which
can leave stale keys; normalize inputs by trimming both publishableKey and
proxyUrl, then if the trimmed publishableKey is blank call
StorageHelper.deleteValue(StorageKey.PUBLIC_KEY) else
StorageHelper.saveValue(StorageKey.PUBLIC_KEY, trimmedPublishableKey), and if
the trimmed proxyUrl is blank call
StorageHelper.deleteValue(StorageKey.PROXY_URL) else
StorageHelper.saveValue(StorageKey.PROXY_URL, trimmedProxyUrl); update the
onSave lambda where StorageHelper.saveValue/ deleteValue and
StorageKey.PUBLIC_KEY/StorageKey.PROXY_URL are used to implement this behavior.
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:
Testing:
Summary by CodeRabbit
New Features
Bug Fixes / Improvements