Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Clerk-based authentication, email verification, and auth-aware routing to the mobile app, wires in secure token storage and an authenticated API hook, and updates home and auth screens to match the new flow. ChangesClerk authentication integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SignInScreen
participant Clerk
participant Router
User->>SignInScreen: submit email/password
SignInScreen->>Clerk: signIn.create(identifier, password)
Clerk-->>SignInScreen: complete, second factor, or error
SignInScreen->>Clerk: setActive(session)
SignInScreen->>Router: navigate to /(tabs)/home or /(auth)/sign-in-verify
sequenceDiagram
participant User
participant SignUpScreen
participant Clerk
participant Router
participant VerifyScreen
User->>SignUpScreen: submit name/email/password
SignUpScreen->>Clerk: signUp.create(...)
SignUpScreen->>Clerk: prepareEmailAddressVerification(email_code)
SignUpScreen->>Router: navigate to /(auth)/verify
User->>VerifyScreen: enter 6-digit code
VerifyScreen->>Clerk: attemptEmailAddressVerification(code)
Clerk-->>VerifyScreen: complete + createdSessionId
VerifyScreen->>Clerk: setActive(session)
VerifyScreen->>Router: navigate to /(tabs)/home
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/mobile/app/(auth)/sign-in.tsx (1)
27-51: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider trimming the email before submitting.
Users frequently introduce leading/trailing whitespace (autocorrect, paste, keyboard suggestions). Passing an untrimmed
identifiertosignIn.createcan cause avoidable sign-in failures.♻️ Proposed tweak
- const attempt = await signIn.create({ identifier: email, password }); + const attempt = await signIn.create({ identifier: email.trim(), password });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/app/`(auth)/sign-in.tsx around lines 27 - 51, Trim the email before calling signIn.create in handleSignIn so accidental leading or trailing whitespace does not break sign-in. Update the identifier value used in the create call to use the cleaned email while keeping the rest of the sign-in flow in SignInScreen unchanged.apps/mobile/app/(auth)/sign-up.tsx (1)
28-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the Clerk sign-up flow into a reusable hook.
The submit handler embeds auth business logic directly in the screen, and the Clerk error-handling block (
isClerkAPIResponseError(err)→err.errors[0]?.message ?? "Something went wrong.") is now duplicated acrosssign-in.tsx,sign-up.tsx, andverify.tsx. A smallhooks/useAuthError(oruseSignUpFlow) helper would keep these screens focused on routing/composition and remove the duplication.As per coding guidelines: "In Expo Router screens under
apps/mobile/app/, keep files focused on routing and screen composition only; do not put large reusable UI blocks or business logic here."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/app/`(auth)/sign-up.tsx around lines 28 - 55, The sign-up screen’s handleCreateAccount in sign-up.tsx mixes reusable auth/business logic with routing, and the Clerk error mapping is duplicated across sign-in, sign-up, and verify. Extract the shared sign-up/auth error handling into a reusable hook such as useAuthError or useSignUpFlow, then have handleCreateAccount focus on form submission and navigation while reusing the centralized Clerk error translation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mobile/app/_layout.tsx`:
- Around line 39-42: The ClerkProvider setup in _layout.tsx should not read
EXPO_PUBLIC_CLERK_PUBLISHABLE_KEY inline without validation. Create a local
publishableKey from the env var, check it before rendering ClerkProvider, and
throw a clear error when it is missing so misconfigured builds fail fast. Keep
the change focused around the ClerkProvider wrapper and its publishableKey prop.
In `@apps/mobile/lib/api.ts`:
- Around line 4-23: The useApi hook should add an abort/timeout path so requests
in the useCallback wrapper do not hang indefinitely, especially for
sign-in/sign-up/verify flows. Update the fetch logic to create and pass an
AbortController signal with a configurable timeout, and ensure the timeout is
cleared after completion. Also change the non-ok response handling in useApi so
it reads the response body from res before throwing and includes that backend
message in the thrown error instead of only the status code.
In `@apps/mobile/package.json`:
- Line 23: The dependency entry for expo-auth-session is pinned to a version
that may not match Expo SDK 54. Update the package.json dependency using the
Expo-managed install flow so the version stays aligned with SDK 54, and verify
the app’s dependency declaration for expo-auth-session reflects the compatible
Expo version rather than a fixed manual pin.
---
Nitpick comments:
In `@apps/mobile/app/`(auth)/sign-in.tsx:
- Around line 27-51: Trim the email before calling signIn.create in handleSignIn
so accidental leading or trailing whitespace does not break sign-in. Update the
identifier value used in the create call to use the cleaned email while keeping
the rest of the sign-in flow in SignInScreen unchanged.
In `@apps/mobile/app/`(auth)/sign-up.tsx:
- Around line 28-55: The sign-up screen’s handleCreateAccount in sign-up.tsx
mixes reusable auth/business logic with routing, and the Clerk error mapping is
duplicated across sign-in, sign-up, and verify. Extract the shared sign-up/auth
error handling into a reusable hook such as useAuthError or useSignUpFlow, then
have handleCreateAccount focus on form submission and navigation while reusing
the centralized Clerk error translation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4869bb05-ad39-4807-956b-87f199dd1f30
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
apps/mobile/app/(auth)/forgot-password.tsxapps/mobile/app/(auth)/sign-in.tsxapps/mobile/app/(auth)/sign-up.tsxapps/mobile/app/(auth)/verify.tsxapps/mobile/app/_layout.tsxapps/mobile/app/index.tsxapps/mobile/lib/api.tsapps/mobile/lib/clerk.tsapps/mobile/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/mobile/app/(auth)/sign-in.tsx (2)
117-132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd
accessibilityStatefor the disabled submit button.The button visually disables and swaps to a spinner while
isSubmitting, but screen readers aren't told it's disabled/busy.♿ Proposed fix
<Pressable onPress={handleSignIn} disabled={isSubmitting} className="h-[52px] rounded-[5px] items-center justify-center bg-interactive-bg" style={({ pressed }) => ({ opacity: pressed || isSubmitting ? 0.85 : 1 })} accessibilityRole="button" accessibilityLabel="Sign in" + accessibilityState={{ disabled: isSubmitting, busy: isSubmitting }} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/app/`(auth)/sign-in.tsx around lines 117 - 132, The submit control in sign-in should expose its disabled/busy state to assistive tech. Update the Pressable in sign-in.tsx (the one using handleSignIn and isSubmitting) to include accessibilityState reflecting disabled and busy while submitting, and keep it in sync with the existing disabled prop and spinner logic so screen readers know the button is unavailable during submission.
27-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting auth logic out of the routing screen.
handleSignInembeds the full Clerk sign-in state machine (attempt creation, second-factor branching, error mapping) directly in an Expo Router screen. As per path instructions,apps/mobile/app/**/*.{ts,tsx}: "keep files focused on routing and screen composition only; do not put large reusable UI blocks or business logic here." Extracting this into auseSignInFlow-style hook would improve testability and keep the screen focused on composition, and would also make it easier to fix the second-factor strategy issue above in one shared place withsign-in-verify.tsx.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/app/`(auth)/sign-in.tsx around lines 27 - 57, The sign-in screen is handling the full Clerk auth state machine inside the route component, which should be moved out of the routing file. Extract the logic in handleSignIn from sign-in.tsx into a reusable hook or auth service such as useSignInFlow, keeping the screen focused on UI and navigation only. Preserve the existing branching for complete and needs_second_factor, and centralize error mapping there so sign-in and sign-in-verify can share the same flow.Source: Path instructions
apps/mobile/app/(auth)/sign-in-verify.tsx (1)
90-97: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd
accessibilityStatefor the disabled Verify button.Same gap as the sign-in screen: no disabled/busy state exposed to assistive tech during submission.
♿ Proposed fix
<Pressable onPress={handleVerify} disabled={isSubmitting} className="h-[52px] rounded-[5px] items-center justify-center bg-interactive-bg" style={({ pressed }) => ({ opacity: pressed || isSubmitting ? 0.85 : 1 })} accessibilityRole="button" accessibilityLabel="Verify code" + accessibilityState={{ disabled: isSubmitting, busy: isSubmitting }} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/app/`(auth)/sign-in-verify.tsx around lines 90 - 97, The Verify Pressable in sign-in-verify.tsx exposes an accessibility label but not its disabled/busy state during submission. Update the Pressable used for handleVerify to include accessibilityState reflecting isSubmitting, so assistive tech can announce when the button is disabled or busy. Keep the existing disabled prop and style behavior in sync with the new accessibilityState.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mobile/app/`(auth)/sign-in.tsx:
- Around line 39-47: The sign-in MFA flow is using the wrong Clerk APIs for
email-code verification. In the sign-in handler that branches on attempt.status,
replace the prepareSecondFactor path with Clerk’s email-code flow by calling
signIn.mfa.sendEmailCode() before navigating to the verify screen, and update
the verify screen to use signIn.mfa.verifyEmailCode({ code }) instead of
attemptSecondFactor. Keep the existing sign-in success and error handling around
the signIn and router logic, but ensure the email-code path is wired through
these mfa methods so the “needs_second_factor” case completes correctly.
---
Nitpick comments:
In `@apps/mobile/app/`(auth)/sign-in-verify.tsx:
- Around line 90-97: The Verify Pressable in sign-in-verify.tsx exposes an
accessibility label but not its disabled/busy state during submission. Update
the Pressable used for handleVerify to include accessibilityState reflecting
isSubmitting, so assistive tech can announce when the button is disabled or
busy. Keep the existing disabled prop and style behavior in sync with the new
accessibilityState.
In `@apps/mobile/app/`(auth)/sign-in.tsx:
- Around line 117-132: The submit control in sign-in should expose its
disabled/busy state to assistive tech. Update the Pressable in sign-in.tsx (the
one using handleSignIn and isSubmitting) to include accessibilityState
reflecting disabled and busy while submitting, and keep it in sync with the
existing disabled prop and spinner logic so screen readers know the button is
unavailable during submission.
- Around line 27-57: The sign-in screen is handling the full Clerk auth state
machine inside the route component, which should be moved out of the routing
file. Extract the logic in handleSignIn from sign-in.tsx into a reusable hook or
auth service such as useSignInFlow, keeping the screen focused on UI and
navigation only. Preserve the existing branching for complete and
needs_second_factor, and centralize error mapping there so sign-in and
sign-in-verify can share the same flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50d0b5e3-d4fc-4bfb-8b12-4269c9896a91
📒 Files selected for processing (5)
apps/mobile/app/(auth)/sign-in-verify.tsxapps/mobile/app/(auth)/sign-in.tsxapps/mobile/app/(tabs)/home.tsxapps/mobile/components/HomeHeader.tsxpackage.json
💤 Files with no reviewable changes (1)
- apps/mobile/app/(tabs)/home.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit