fix(web): improve auth and store setup flow#348
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Warning Review limit reached
More reviews will be available in 31 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThree authentication and setup pages add safe redirect destination validation and input normalization: login sanitizes the redirect query parameter, registration normalizes Nigerian phone formats, and store setup applies both patterns alongside form placeholder standardization and a timeout guard on the existing-store check. ChangesSecure Redirects and User Input Validation
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/web/src/app/(auth)/login/page.tsx (1)
24-34: ⚡ Quick winConsider aligning the return-type pattern with store setup for consistency.
The store setup flow uses
getSafePostSetupRedirect()which returns a fallback string directly, while this function returnsnulland relies on nullish coalescing at the call site. Both approaches work, but unifying the pattern would improve consistency.♻️ Optional refactor to match store setup pattern
-function getSafeRedirectDestination(value: string | null): string | null { +function getSafeRedirectDestination(value: string | null): string { if (!value || !value.startsWith("/") || value.startsWith("//")) { - return null; + return DEFAULT_LOGIN_DESTINATION; } if (value === "/login" || value.startsWith("/login?")) { - return null; + return DEFAULT_LOGIN_DESTINATION; } return value; }Then simplify the call site:
- const destination = - getSafeRedirectDestination(searchParams.get("redirect")) ?? - DEFAULT_LOGIN_DESTINATION; + const destination = getSafeRedirectDestination(searchParams.get("redirect"));🤖 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/web/src/app/`(auth)/login/page.tsx around lines 24 - 34, The getSafeRedirectDestination function currently returns null for unsafe values; change it to follow the store pattern by returning a fallback string (e.g., "/") instead of null so its signature becomes string and callers don't have to use nullish coalescing. Update getSafeRedirectDestination to return the fallback for all invalid inputs (preserve the existing checks for value, value.startsWith("/"), "//", and "/login" cases) and remove the ?? fallback at call sites that use this helper so the call site simply uses getSafeRedirectDestination(...).apps/web/src/app/(store)/store/setup/StoreSetupClient.tsx (1)
144-148: 💤 Low valueAbort the controller on cleanup to cancel in-flight requests.
The
cancelledflag prevents state updates, but the fetch request continues until completion or timeout. Aborting the controller on unmount frees network resources immediately.Proposed fix
Move controller outside the async function and abort in cleanup:
useEffect(() => { let cancelled = false; + const controller = new AbortController(); async function checkExistingStore() { if (!API_BASE) { setCheckError("The API URL is not configured for this environment."); setCheckingStore(false); return; } - const controller = new AbortController(); const timeoutId = window.setTimeout( () => controller.abort(), STORE_CHECK_TIMEOUT_MS, ); // ... rest of function } checkExistingStore(); return () => { cancelled = true; + controller.abort(); }; }, [router]);Also applies to: 187-189
🤖 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/web/src/app/`(store)/store/setup/StoreSetupClient.tsx around lines 144 - 148, The AbortController instance is created inside the async task so it never gets aborted during component cleanup; move the AbortController creation (const controller = new AbortController()) and its timeout (window.setTimeout(..., STORE_CHECK_TIMEOUT_MS)) to the outer scope of the effect (outside the inner async function) and in the effect cleanup callback call controller.abort() and clearTimeout(timeoutId) so in-flight fetches started by the fetch that uses controller.signal are cancelled on unmount; apply the same change to the other occurrence that creates a controller (the block around lines 187-189) so both fetch flows cancel properly.
🤖 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/web/src/app/`(auth)/login/page.tsx:
- Line 51: The call to api.post in apps/web/src/app/(auth)/login/page.tsx
includes an unused response type annotation { redirectTo: string }; remove this
unused generic from the api.post call (or replace it with a more accurate/empty
type like void or unknown) so the code no longer claims to use redirectTo.
Locate the api.post invocation in the login form submit handler (the call site
shown as await api.post<{ redirectTo: string }>("/auth/login", data)) and update
the generic to reflect that the response is ignored or not relied upon.
In `@apps/web/src/app/`(store)/store/setup/StoreSetupClient.tsx:
- Around line 62-72: getSafePostSetupRedirect currently misses variants like
trailing slash or fragments; update it to parse the input using URL (e.g., new
URL(value, location.origin)) to extract pathname and detect setup routes
robustly: if the pathname equals "/store/setup" or "/store/setup/" (or otherwise
matches the exact setup route) treat it as unsafe and return "/store/dashboard";
keep the existing guard rejecting null, non-leading-slash, and "//" values and
return the original value for all other cases. Use the function name
getSafePostSetupRedirect to locate and modify the logic.
---
Nitpick comments:
In `@apps/web/src/app/`(auth)/login/page.tsx:
- Around line 24-34: The getSafeRedirectDestination function currently returns
null for unsafe values; change it to follow the store pattern by returning a
fallback string (e.g., "/") instead of null so its signature becomes string and
callers don't have to use nullish coalescing. Update getSafeRedirectDestination
to return the fallback for all invalid inputs (preserve the existing checks for
value, value.startsWith("/"), "//", and "/login" cases) and remove the ??
fallback at call sites that use this helper so the call site simply uses
getSafeRedirectDestination(...).
In `@apps/web/src/app/`(store)/store/setup/StoreSetupClient.tsx:
- Around line 144-148: The AbortController instance is created inside the async
task so it never gets aborted during component cleanup; move the AbortController
creation (const controller = new AbortController()) and its timeout
(window.setTimeout(..., STORE_CHECK_TIMEOUT_MS)) to the outer scope of the
effect (outside the inner async function) and in the effect cleanup callback
call controller.abort() and clearTimeout(timeoutId) so in-flight fetches started
by the fetch that uses controller.signal are cancelled on unmount; apply the
same change to the other occurrence that creates a controller (the block around
lines 187-189) so both fetch flows cancel properly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c2ef920-6738-4a52-b5df-43f55ffa9a76
📒 Files selected for processing (3)
apps/web/src/app/(auth)/login/page.tsxapps/web/src/app/(auth)/register/page.tsxapps/web/src/app/(store)/store/setup/StoreSetupClient.tsx
1 participant
What does this PR do?
Improves the auth and store setup flow by adding safe redirect handling for login, normalizing Nigerian phone numbers during registration, removing hardcoded demo examples from onboarding/setup copy, and making store setup redirect safely after creation or when an existing store is detected.
Type of change
Area affected
How to test this
/login,/register, and/store/setup./homewhen no safe redirect is provided.//..., and/login?...are rejected.090...,901..., and+234..../store/dashboard.Expected result: auth/store setup flows work without unsafe redirects, hardcoded demo values, or store setup redirect loops.
Pre-commit checklist
console.logleft in production code.envfiles committedanytypes addeddb pushScreenshots
N/A
Notes for reviewer
This PR intentionally keeps frontend registration age validation aligned with the current backend account rule and does not change account registration to 18+. Store creation remains handled separately by the existing store eligibility flow.
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements