Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideImplements a new in-app authentication modal flow that replaces the standalone /login page, wires it into global providers and routing, and updates the user menu to trigger the modal, including a new reusable login form and overlay components. Sequence diagram for the new in-app auth modal flow including /login redirectsequenceDiagram
participant Browser
participant Proxy
participant NextRouter
participant AuthModalProvider
participant AuthModal
participant AuthOverlay
participant LoginForm
participant AuthClient
Browser->>Proxy: GET /login
Proxy-->>Browser: 302 Redirect to /?auth=1
Browser->>AuthModalProvider: Render with URL /?auth=1
AuthModalProvider->>AuthModalProvider: read searchParams auth
AuthModalProvider-->>AuthModalProvider: showAuthModal = true
AuthModalProvider->>AuthModal: render AuthModal
AuthModal->>AuthOverlay: isOpen = true allowClose = !requireAuth
Browser->>AuthOverlay: User interacts (open drawer)
AuthOverlay->>LoginForm: render LoginForm
Browser->>LoginForm: Submit credentials
LoginForm->>AuthClient: signIn or signUp
AuthClient-->>LoginForm: success callback
LoginForm-->>AuthOverlay: onSuccess
AuthOverlay-->>AuthModalProvider: setShowAuthModal false
AuthModalProvider-->>AuthModal: showAuthModal = false
AuthModal-->>AuthOverlay: isOpen = false
Updated class diagram for auth modal, context, overlay, and user menuclassDiagram
class AuthModalProvider {
+boolean showAuthModal
+boolean requireAuth
+setShowAuthModal(show)
+setRequireAuth(requireAuth)
+openAuthModal(options)
}
class AuthModalContextValue {
+boolean showAuthModal
+function setShowAuthModal(show)
+boolean requireAuth
+function setRequireAuth(requireAuth)
+function openAuthModal(options)
}
class useAuthModal {
+AuthModalContextValue return
}
class AuthModal {
+render()
}
class AuthOverlay {
+boolean isOpen
+onClose()
+onSuccess()
+boolean allowClose
+render()
}
class LoginForm {
+string title
+string subtitle
+onSuccess()
+render()
-handleEmailChange(event)
-handleEmailKeyDown(event)
-handleGithubSignIn()
-handleGoogleSignIn()
-handleSubmit(event)
}
class UserMenu {
+render()
}
class AuthButton {
+children
+onClick()
+boolean isLoading
+boolean disabled
+string variant
+string type
+style
+icon
+render()
}
class ErrorText {
+string error
+render()
}
class signIn {
+social(config, callbacks)
+email(config, callbacks)
}
class signUp {
+email(config, callbacks)
}
class Proxy {
+proxy(request)
}
AuthModalProvider --> AuthModalContextValue : provides
useAuthModal --> AuthModalContextValue : returns
AuthModal --> AuthOverlay
AuthModal --> AuthModalProvider : uses context
AuthOverlay --> LoginForm
AuthOverlay --> AuthModalProvider : setShowAuthModal
LoginForm --> signIn : calls
LoginForm --> signUp : calls
UserMenu --> useAuthModal : uses
UserMenu --> AuthModalProvider : openAuthModal
Proxy --> AuthModalProvider : sets auth query param
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces a modal-based authentication system, shifting from a dedicated login page to a globally accessible modal. It adds authentication context, UI components for the overlay and login form with social/email auth, integrates them into the app providers, updates user navigation to trigger the modal, and redirects the Changes
Sequence DiagramsequenceDiagram
actor User
participant UserMenu
participant AuthModalProvider
participant AuthOverlay
participant LoginForm
participant AuthService
participant Router
User->>UserMenu: Click "Sign In"
UserMenu->>AuthModalProvider: useAuthModal().openAuthModal()
AuthModalProvider->>AuthModalProvider: setShowAuthModal(true)
AuthModalProvider->>AuthOverlay: isOpen=true
AuthOverlay->>User: Display animated modal overlay
User->>LoginForm: Enter email & password
LoginForm->>LoginForm: Validate credentials
User->>LoginForm: Click Sign In
LoginForm->>AuthService: signIn.email({ email, password })
AuthService->>AuthService: Authenticate user
alt Success
AuthService-->>LoginForm: onSuccess callback
LoginForm->>AuthOverlay: onSuccess triggered
AuthOverlay->>AuthModalProvider: setShowAuthModal(false)
AuthModalProvider->>AuthOverlay: isOpen=false
AuthOverlay->>User: Close modal with animation
else Error
AuthService-->>LoginForm: onError callback
LoginForm->>User: Display error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
AuthModalProvider,requireAuthis only ever set totrueand never reset on close or whenopenAuthModalis called withoutrequireAuth, so once an auth-required flow runs, all future openings will remain non-dismissible; consider explicitly resettingrequireAuthtofalseon close or when callingopenAuthModalwithoutrequireAuth. - The
/app/(auth)/login/page.tsxcomponent now returnsnullwhile/loginis redirected inproxy.ts; if the redirect is guaranteed, you can simplify by removing the page entirely to avoid maintaining an unused route component.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AuthModalProvider`, `requireAuth` is only ever set to `true` and never reset on close or when `openAuthModal` is called without `requireAuth`, so once an auth-required flow runs, all future openings will remain non-dismissible; consider explicitly resetting `requireAuth` to `false` on close or when calling `openAuthModal` without `requireAuth`.
- The `/app/(auth)/login/page.tsx` component now returns `null` while `/login` is redirected in `proxy.ts`; if the redirect is guaranteed, you can simplify by removing the page entirely to avoid maintaining an unused route component.
## Individual Comments
### Comment 1
<location> `apps/web/components/auth/auth-overlay.tsx:102` </location>
<code_context>
+export function AuthOverlay({ isOpen, onClose, onSuccess, allowClose = true }: Props) {
+ const closeButtonRef = useRef<HTMLButtonElement>(null)
+ const isMobile = useMediaQuery('(max-width: 768px)')
+ const y = useMotionValue(0)
+
+ useEffect(() => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid mixing an explicit motion value for `y` with `initial/animate/exit` that also drive `y`.
`y` is defined as a `useMotionValue` and passed via `style={{ y }}`, but is also controlled by `initial/animate/exit` (`initial={{ y: '100%' }}`, `animate={{ y: 0 }}`, `exit={{ y: '100%' }}`). Framer Motion expects one controller per animated property; mixing them can cause the enter/exit animation and drag behavior to conflict. Consider either:
- Dropping `useMotionValue` and relying solely on `initial/animate/exit` + `drag`, or
- Keeping `useMotionValue`/`style={{ y }}` and driving `y` only via that (e.g. `animate(y, ...)`/variants), removing `y` from `initial/animate/exit`.
This keeps `y` single-sourced and the drag/snap behavior predictable.
</issue_to_address>
### Comment 2
<location> `apps/web/components/auth/login-form.tsx:394-403` </location>
<code_context>
+ const handleSubmit = useCallback(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider explicitly clearing loading state on successful email sign-in/sign-up to avoid getting stuck in a loading state in non-redirect flows.
In `handleSubmit`, `setIsLoading(true)` / `setLoadingAction('email')` are only cleared on validation errors or in `onError`/`catch`, not on success. In flows where `signIn.email` / `signUp.email` don’t navigate away (e.g. a modal where `onSuccess` just closes the overlay), the form can be left with `isLoading === true` and `loadingAction === 'email'` after a successful submit. Please also reset these flags on success (in `onSuccess` or after the `await`) so state is consistent across all flows.
Suggested implementation:
```typescript
const handleSubmit = useCallback(
async (e: React.FormEvent) => {
e.preventDefault()
setGeneralError('')
setConfirmError('')
setIsLoading(true)
setLoadingAction('email')
try {
if (isRegisterMode) {
```
To fully implement the suggestion, you should also ensure that the `try` block for `handleSubmit` ends with a `finally` that clears the loading state, for example:
```ts
try {
if (isRegisterMode) {
// ... existing sign-up logic (validation, signUp.email, etc.)
} else {
// ... existing sign-in logic (signIn.email, etc.)
}
} catch (err) {
// ... existing error handling, including setGeneralError, setConfirmError, etc.
} finally {
setIsLoading(false)
setLoadingAction(null)
}
```
Key points:
- Keep all existing validation and error-handling logic in `try` / `catch`.
- The new `finally` will run on both success and error, ensuring `isLoading` and `loadingAction` are reset even when `signIn.email` / `signUp.email` succeeds without a redirect (e.g. modal flows where `onSuccess` only closes the overlay).
- If you already call `setIsLoading(false)` / `setLoadingAction(null)` in `catch`, you can remove those from `catch` once they're in `finally` to avoid redundant calls.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| export function AuthOverlay({ isOpen, onClose, onSuccess, allowClose = true }: Props) { | ||
| const closeButtonRef = useRef<HTMLButtonElement>(null) | ||
| const isMobile = useMediaQuery('(max-width: 768px)') | ||
| const y = useMotionValue(0) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Avoid mixing an explicit motion value for y with initial/animate/exit that also drive y.
y is defined as a useMotionValue and passed via style={{ y }}, but is also controlled by initial/animate/exit (initial={{ y: '100%' }}, animate={{ y: 0 }}, exit={{ y: '100%' }}). Framer Motion expects one controller per animated property; mixing them can cause the enter/exit animation and drag behavior to conflict. Consider either:
- Dropping
useMotionValueand relying solely oninitial/animate/exit+drag, or - Keeping
useMotionValue/style={{ y }}and drivingyonly via that (e.g.animate(y, ...)/variants), removingyfrominitial/animate/exit.
This keeps y single-sourced and the drag/snap behavior predictable.
Comment on lines
+394
to
+403
| const handleSubmit = useCallback( | ||
| async (e: React.FormEvent) => { | ||
| e.preventDefault() | ||
| setIsLoading(true) | ||
| setLoadingAction('email') | ||
| setGeneralError('') | ||
| setConfirmError('') | ||
|
|
||
| try { | ||
| if (isRegisterMode) { |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider explicitly clearing loading state on successful email sign-in/sign-up to avoid getting stuck in a loading state in non-redirect flows.
In handleSubmit, setIsLoading(true) / setLoadingAction('email') are only cleared on validation errors or in onError/catch, not on success. In flows where signIn.email / signUp.email don’t navigate away (e.g. a modal where onSuccess just closes the overlay), the form can be left with isLoading === true and loadingAction === 'email' after a successful submit. Please also reset these flags on success (in onSuccess or after the await) so state is consistent across all flows.
Suggested implementation:
const handleSubmit = useCallback(
async (e: React.FormEvent) => {
e.preventDefault()
setGeneralError('')
setConfirmError('')
setIsLoading(true)
setLoadingAction('email')
try {
if (isRegisterMode) {To fully implement the suggestion, you should also ensure that the try block for handleSubmit ends with a finally that clears the loading state, for example:
try {
if (isRegisterMode) {
// ... existing sign-up logic (validation, signUp.email, etc.)
} else {
// ... existing sign-in logic (signIn.email, etc.)
}
} catch (err) {
// ... existing error handling, including setGeneralError, setConfirmError, etc.
} finally {
setIsLoading(false)
setLoadingAction(null)
}Key points:
- Keep all existing validation and error-handling logic in
try/catch. - The new
finallywill run on both success and error, ensuringisLoadingandloadingActionare reset even whensignIn.email/signUp.emailsucceeds without a redirect (e.g. modal flows whereonSuccessonly closes the overlay). - If you already call
setIsLoading(false)/setLoadingAction(null)incatch, you can remove those fromcatchonce they're infinallyto avoid redundant calls.
![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: 4
🤖 Fix all issues with AI agents
In `@apps/web/app/providers.tsx`:
- Around line 58-72: Wrap the AuthModalProvider usage in the providers tree with
a React Suspense boundary to satisfy the useSearchParams() requirement in
auth-modal-context.tsx (line 26); import Suspense from React and enclose the
<AuthModalProvider> ... </AuthModalProvider> block inside <Suspense
fallback={null}>...</Suspense> so AuthModalProvider (and its consumer AuthModal)
are rendered within a Suspense boundary while keeping the rest of the provider
tree unchanged.
In `@apps/web/components/auth/auth-overlay.tsx`:
- Around line 99-199: AuthOverlay currently implements a custom modal without a
focus trap; replace the manual overlay/motion.div + backdrop logic with the
DrawerDialog component from `@skriuw/ui` to get built-in focus trapping,
role/aria-modal, escape handling, scroll lock and mobile drag-to-close.
Specifically, render DrawerDialog in place of the outer motion.div(s) and pass
AuthOverlay props (isOpen -> open, onClose -> onClose, allowClose -> closeable)
and keep LoginForm as the dialog content (preserve the onSuccess handler to call
onSuccess and onClose); remove manual escape listener, body overflow handling,
and the custom focus management (closeButtonRef/useEffect) and reuse
DrawerDialog’s close button or still pass closeButtonRef into DrawerDialog if
needed. Ensure you forward the motion y/drag behavior only via DrawerDialog
props (or remove y if DrawerDialog handles drag) so keyboard focus cannot escape
and the root has role="dialog" and aria-modal="true".
In `@apps/web/components/auth/login-form.tsx`:
- Line 689: The terms copy currently says "By clicking 'Get Started'" which
doesn't match the actual CTA; update the string in the login-form component
(LoginForm / the JSX near the consent text) to reference the actual submit
button label used by the form (e.g., "Sign in" or "Create account") or derive it
from the same variable/prop that renders the <button> label so the copy always
matches the CTA; locate the consent paragraph in
apps/web/components/auth/login-form.tsx and replace the hardcoded "Get Started"
text with the actual button label or a reference to the buttonLabel variable.
- Around line 500-551: The inputs in the LoginForm component rely only on
placeholder text (e.g., the Name input, the email input handled by
handleEmailChange/handleEmailKeyDown, and the password/confirm-password inputs)
which is inaccessible to screen readers; add explicit, accessible labels for
each input by either inserting visually-hidden <label> elements tied to each
input via id/htmlFor or by adding descriptive aria-label attributes (and keep
ids consistent with label htmlFor), and ensure these changes are applied for
Name, Email, Password, and Confirm Password inputs inside the form handled by
handleSubmit so screen readers announce them properly while preserving current
styling and focus behavior.
🧹 Nitpick comments (6)
apps/web/app/(auth)/login/page.tsx (1)
1-3: Consider a server-side redirect as a fallback.If middleware doesn't execute for any reason (e.g., edge cases in deployment), this page renders blank. A
redirect('/?auth=1')fromnext/navigationhere would be a safer fallback.🔧 Proposed fix
+import { redirect } from 'next/navigation' + export default function LoginPage() { - return null + redirect('/?auth=1') }apps/web/components/auth/auth-modal-context.tsx (1)
33-41: Context value exposes raw setters alongsideopenAuthModal— potential for inconsistent state.Consumers can call
setShowAuthModal(true)directly, bypassing therequireAuthreset thatopenAuthModalperforms. This could leaverequireAuthstale from a previous invocation. Consider narrowing the public API to onlyopenAuthModaland acloseAuthModalhelper, keeping the raw setters internal.♻️ Sketch
type AuthModalContextValue = { showAuthModal: boolean - setShowAuthModal: (show: boolean) => void requireAuth: boolean - setRequireAuth: (requireAuth: boolean) => void openAuthModal: (options?: OpenAuthModalOptions) => void + closeAuthModal: () => void }Then in the provider:
+const closeAuthModal = useCallback(() => { + setShowAuthModal(false) + setRequireAuth(false) +}, [])apps/web/components/auth/auth-overlay.tsx (1)
16-97: Inline style objects deviate from the project's Tailwind-first convention.The coding guidelines specify "Prefer utility-first styling and existing semantic classes with Tailwind v4" and "Follow existing tokens and CSS variables." This component defines ~80 lines of inline style objects instead of using Tailwind classes. While the CSS variables usage is correct, the delivery mechanism doesn't match the project convention.
This is a sizable refactor so it doesn't need to block the PR, but consider migrating to Tailwind classes in a follow-up to stay consistent with the rest of the codebase.
apps/web/components/auth/login-form.tsx (3)
43-193: ~150 lines of inline style objects — same Tailwind convention concern as inauth-overlay.tsx.Both this file and
auth-overlay.tsxdefine extensive inline style objects rather than using Tailwind utility classes. Consider migrating in a follow-up pass to stay consistent with the project's styling approach. As per coding guidelines, "Prefer utility-first styling and existing semantic classes with Tailwind v4."
226-294:AuthButtonis a one-off primitive — consider reusingButtonfrom@skriuw/ui.The project already has a
Buttoncomponent (imported inuser-menu.tsxfrom@skriuw/ui/button).AuthButtonre-implements button styling, disabled state, and loading animation from scratch. Consider extending@skriuw/ui/buttonwith anisLoadingprop or composing it instead of rolling a new primitive. As per coding guidelines, "Reuse@skriuw/uicomponents before creating one-off primitives."
350-392: Social sign-in handlers are near-identical — consider deduplicating.
handleGithubSignInandhandleGoogleSignInonly differ in the provider string and error message. Extract a sharedhandleSocialSignIn(provider: string)to reduce duplication.♻️ Sketch
+const handleSocialSignIn = useCallback(async (provider: 'github' | 'google') => { + setIsLoading(true) + setLoadingAction(provider) + setGeneralError('') + try { + await signIn.social( + { provider }, + { + onError: (ctx) => { + setGeneralError(ctx.error.message || `Failed to sign in with ${provider}.`) + setIsLoading(false) + setLoadingAction(null) + } + } + ) + } catch (err) { + setGeneralError(err instanceof Error ? err.message : `Failed to sign in with ${provider}.`) + setIsLoading(false) + setLoadingAction(null) + } +}, [])
Comment on lines
+58
to
+72
| <AuthModalProvider> | ||
| <SettingsProvider> | ||
| <NotesProvider> | ||
| <ShortcutProvider> | ||
| <ContextMenuProvider> | ||
| <EditorTabsProvider> | ||
| <AppLayoutManager>{children}</AppLayoutManager> | ||
| </EditorTabsProvider> | ||
| </ContextMenuProvider> | ||
| <CommandExecutor /> | ||
| </ShortcutProvider> | ||
| </NotesProvider> | ||
| </SettingsProvider> | ||
| <AuthModal /> | ||
| </AuthModalProvider> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# 1. Check if auth-modal-context.tsx exists and uses useSearchParams()
fd 'auth-modal' apps/web --type f | head -20Repository: remcostoeten/skriuw
Length of output: 150
🏁 Script executed:
# 2. Read the full providers.tsx file to understand context
wc -l apps/web/app/providers.tsxRepository: remcostoeten/skriuw
Length of output: 92
🏁 Script executed:
# 3. Check for Suspense usage in the app directory
rg -n 'Suspense' --type=tsx --type=ts apps/web/app/ | head -30Repository: remcostoeten/skriuw
Length of output: 90
🏁 Script executed:
# 4. Check how Providers is used in layout files
rg -n -B2 -A2 '<Providers' apps/web/app/layout.tsxRepository: remcostoeten/skriuw
Length of output: 234
🏁 Script executed:
# Read the auth-modal-context.tsx file to check for useSearchParams()
cat -n apps/web/components/auth/auth-modal-context.tsxRepository: remcostoeten/skriuw
Length of output: 2354
🏁 Script executed:
# Read the full providers.tsx file
cat -n apps/web/app/providers.tsxRepository: remcostoeten/skriuw
Length of output: 3178
🏁 Script executed:
# Check the root layout file for Suspense wrapping
cat -n apps/web/app/layout.tsx | head -100Repository: remcostoeten/skriuw
Length of output: 2821
🏁 Script executed:
# Try Suspense search with correct file type flag
rg 'Suspense' apps/web/app/ -A2 -B2 | head -50Repository: remcostoeten/skriuw
Length of output: 45
AuthModalProvider uses useSearchParams() — add a <Suspense> boundary for proper Next.js streaming support.
AuthModalProvider calls useSearchParams() at line 26 of auth-modal-context.tsx, which requires a <Suspense> boundary in Next.js App Router. While the route-level export const dynamic = 'force-dynamic' in layout.tsx mitigates the runtime issue by forcing dynamic rendering, wrapping AuthModalProvider in Suspense is still recommended for:
- Proper streaming and content delivery
- Following Next.js best practices
- Future-proofing in case
force-dynamicis removed
🔧 Proposed fix
+import { Suspense } from 'react'
// ... existing imports
<StorageInitializer>
+ <Suspense>
<AuthModalProvider>
<SettingsProvider>
...
</SettingsProvider>
<AuthModal />
</AuthModalProvider>
+ </Suspense>
</StorageInitializer>🤖 Prompt for AI Agents
In `@apps/web/app/providers.tsx` around lines 58 - 72, Wrap the AuthModalProvider
usage in the providers tree with a React Suspense boundary to satisfy the
useSearchParams() requirement in auth-modal-context.tsx (line 26); import
Suspense from React and enclose the <AuthModalProvider> ... </AuthModalProvider>
block inside <Suspense fallback={null}>...</Suspense> so AuthModalProvider (and
its consumer AuthModal) are rendered within a Suspense boundary while keeping
the rest of the provider tree unchanged.
Comment on lines
+99
to
+199
| export function AuthOverlay({ isOpen, onClose, onSuccess, allowClose = true }: Props) { | ||
| const closeButtonRef = useRef<HTMLButtonElement>(null) | ||
| const isMobile = useMediaQuery('(max-width: 768px)') | ||
| const y = useMotionValue(0) | ||
|
|
||
| useEffect(() => { | ||
| if (isOpen && closeButtonRef.current && allowClose) { | ||
| closeButtonRef.current.focus() | ||
| } | ||
| }, [isOpen, allowClose]) | ||
|
|
||
| useEffect(() => { | ||
| const handleEscape = (e: KeyboardEvent) => { | ||
| if (e.key === 'Escape' && isOpen && allowClose && onClose) { | ||
| onClose() | ||
| } | ||
| } | ||
| document.addEventListener('keydown', handleEscape) | ||
| return () => document.removeEventListener('keydown', handleEscape) | ||
| }, [isOpen, allowClose, onClose]) | ||
|
|
||
| useEffect(() => { | ||
| if (isOpen) { | ||
| document.body.style.overflow = 'hidden' | ||
| } else { | ||
| document.body.style.overflow = '' | ||
| } | ||
| return () => { | ||
| document.body.style.overflow = '' | ||
| } | ||
| }, [isOpen]) | ||
|
|
||
| useEffect(() => { | ||
| if (!isOpen) y.set(0) | ||
| }, [isOpen, y]) | ||
|
|
||
| return ( | ||
| <AnimatePresence> | ||
| {isOpen && ( | ||
| <motion.div | ||
| initial={{ opacity: 0 }} | ||
| animate={{ opacity: 1 }} | ||
| exit={{ opacity: 0 }} | ||
| transition={{ duration: 0.3 }} | ||
| style={styles.overlay} | ||
| > | ||
| <div style={styles.backdrop} onClick={allowClose && onClose ? onClose : undefined} /> | ||
|
|
||
| <motion.div | ||
| initial={{ y: '100%' }} | ||
| animate={{ y: 0 }} | ||
| exit={{ y: '100%' }} | ||
| transition={{ duration: 0.4, ease: [0.32, 0.72, 0, 1] }} | ||
| drag={isMobile && allowClose ? 'y' : false} | ||
| dragConstraints={{ top: 0, bottom: 0 }} | ||
| dragElastic={0.2} | ||
| onDragEnd={(_, info) => { | ||
| if (!isMobile || !allowClose) return | ||
| const shouldClose = | ||
| info.offset.y > window.innerHeight * 0.15 || info.velocity.y > 500 | ||
| if (shouldClose) { | ||
| onClose?.() | ||
| return | ||
| } | ||
| animate(y, 0, { duration: 0.2, ease: 'easeOut' }) | ||
| }} | ||
| style={{ | ||
| ...styles.drawer, | ||
| y | ||
| }} | ||
| > | ||
| {isMobile && allowClose && ( | ||
| <div style={styles.dragHandle}> | ||
| <div style={styles.dragHandleBar} aria-label='Drag to close' /> | ||
| </div> | ||
| )} | ||
|
|
||
| <div style={styles.contentWrapper}> | ||
| {allowClose && onClose && ( | ||
| <button | ||
| ref={closeButtonRef} | ||
| onClick={onClose} | ||
| style={styles.closeButton} | ||
| aria-label='Close' | ||
| > | ||
| <X style={{ width: 20, height: 20 }} /> | ||
| </button> | ||
| )} | ||
|
|
||
| <LoginForm | ||
| onSuccess={() => { | ||
| onSuccess?.() | ||
| onClose?.() | ||
| }} | ||
| /> | ||
| </div> | ||
| </motion.div> | ||
| </motion.div> | ||
| )} | ||
| </AnimatePresence> | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the project has a Dialog component in `@skriuw/ui` or a focus-trap dependency
echo "=== Looking for Dialog in `@skriuw/ui` ==="
fd -t f -e tsx -e ts 'dialog' packages/ui/ 2>/dev/null | head -20
echo ""
echo "=== Looking for focus-trap in package.json ==="
cat package.json | grep -i 'focus-trap' || echo "No focus-trap found in root package.json"
echo ""
echo "=== Checking packages/ui/package.json ==="
if [ -f packages/ui/package.json ]; then
cat packages/ui/package.json | grep -i 'focus-trap' || echo "No focus-trap in packages/ui/package.json"
fi
echo ""
echo "=== Looking for aria-modal usage patterns ==="
rg -n 'aria-modal' --type tsx --type ts | head -20
echo ""
echo "=== Checking for existing modal implementations ==="
rg -l 'role.*dialog' --type tsx --type ts | head -10
echo ""
echo "=== Checking auth-overlay.tsx styling approach ==="
head -100 apps/web/components/auth/auth-overlay.tsx | grep -A5 'style='Repository: remcostoeten/skriuw
Length of output: 615
🏁 Script executed:
#!/bin/bash
# Examine the existing dialog components
echo "=== dialog.tsx (first 80 lines) ==="
head -80 packages/ui/dialog.tsx
echo ""
echo "=== dialog-drawer.tsx (first 80 lines) ==="
head -80 packages/ui/dialog-drawer.tsx
echo ""
echo "=== auth-overlay.tsx complete styling section ==="
sed -n '16,98p' apps/web/components/auth/auth-overlay.tsxRepository: remcostoeten/skriuw
Length of output: 7042
🏁 Script executed:
#!/bin/bash
# Check full dialog-drawer implementation and its features
echo "=== dialog-drawer.tsx (complete file) ==="
wc -l packages/ui/dialog-drawer.tsx
echo ""
echo "=== Check for focus trap creation in dialog-drawer ==="
rg -A5 'createFocusTrap' packages/ui/dialog-drawer.tsx
echo ""
echo "=== Check exports from dialog-drawer ==="
rg 'export' packages/ui/dialog-drawer.tsx
echo ""
echo "=== Check mobile/drag features in dialog-drawer ==="
rg -n 'drag|mobile|Mobile' packages/ui/dialog-drawer.tsx | head -20Repository: remcostoeten/skriuw
Length of output: 2595
Missing focus trap — keyboard focus can escape the open modal.
When the overlay is open, pressing Tab moves focus to background elements behind the modal. Modal dialogs require a focus trap to keep keyboard navigation within the overlay. Additionally, the root element should have role="dialog" and aria-modal="true".
Rather than adding a focus-trap library, use the existing DrawerDialog component from @skriuw/ui, which already provides focus trap via createFocusTrap, handles Escape key, scroll lock, role/aria-modal, and supports mobile drag-to-close. This aligns with the guideline: "Reuse @skriuw/ui components before creating one-off primitives."
🤖 Prompt for AI Agents
In `@apps/web/components/auth/auth-overlay.tsx` around lines 99 - 199, AuthOverlay
currently implements a custom modal without a focus trap; replace the manual
overlay/motion.div + backdrop logic with the DrawerDialog component from
`@skriuw/ui` to get built-in focus trapping, role/aria-modal, escape handling,
scroll lock and mobile drag-to-close. Specifically, render DrawerDialog in place
of the outer motion.div(s) and pass AuthOverlay props (isOpen -> open, onClose
-> onClose, allowClose -> closeable) and keep LoginForm as the dialog content
(preserve the onSuccess handler to call onSuccess and onClose); remove manual
escape listener, body overflow handling, and the custom focus management
(closeButtonRef/useEffect) and reuse DrawerDialog’s close button or still pass
closeButtonRef into DrawerDialog if needed. Ensure you forward the motion y/drag
behavior only via DrawerDialog props (or remove y if DrawerDialog handles drag)
so keyboard focus cannot escape and the root has role="dialog" and
aria-modal="true".
Comment on lines
+500
to
+551
| <form onSubmit={handleSubmit} style={styles.form}> | ||
| {isRegisterMode && ( | ||
| <div style={styles.inputWrapper}> | ||
| <input | ||
| type='text' | ||
| placeholder='Name' | ||
| value={name} | ||
| onChange={(e) => setName(e.target.value)} | ||
| style={styles.input} | ||
| autoComplete='name' | ||
| /> | ||
| </div> | ||
| )} | ||
|
|
||
| <div style={styles.inputWrapper}> | ||
| {emailSuggestion && emailSuggestion.startsWith(email) && ( | ||
| <> | ||
| <div style={ghostInputStyle} aria-hidden='true'> | ||
| <span style={{ opacity: 0 }}>{email}</span> | ||
| <span style={{ opacity: 0.5 }}>{emailSuggestion.slice(email.length)}</span> | ||
| </div> | ||
| <div | ||
| style={{ | ||
| position: 'absolute', | ||
| right: '12px', | ||
| top: '50%', | ||
| transform: 'translateY(-50%)', | ||
| zIndex: 10, | ||
| opacity: 0.7 | ||
| }} | ||
| > | ||
| <Kbd>Tab</Kbd> | ||
| </div> | ||
| </> | ||
| )} | ||
| <input | ||
| type='email' | ||
| placeholder='Email address' | ||
| value={email} | ||
| onChange={handleEmailChange} | ||
| onKeyDown={handleEmailKeyDown} | ||
| required | ||
| autoComplete='email' | ||
| style={{ | ||
| ...styles.input, | ||
| position: 'relative', | ||
| zIndex: 1, | ||
| background: 'transparent', | ||
| ...(generalError ? styles.inputError : {}) | ||
| }} | ||
| /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Form inputs rely on placeholder text without visible <label> elements.
Screen readers and assistive technologies depend on explicit labels. Using only placeholder is insufficient — it disappears on input and isn't consistently announced. Add visually-hidden labels or use aria-label on each input at minimum.
As per coding guidelines, "Maintain accessible interactions: focus visibility, keyboard navigation, labels."
🔧 Example for the email input
<div style={styles.inputWrapper}>
+ <label htmlFor="login-email" className="sr-only">Email address</label>
<input
+ id="login-email"
type='email'
placeholder='Email address'Apply the same pattern for Name, Password, and Confirm Password inputs.
📝 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
| <form onSubmit={handleSubmit} style={styles.form}> | |
| {isRegisterMode && ( | |
| <div style={styles.inputWrapper}> | |
| <input | |
| type='text' | |
| placeholder='Name' | |
| value={name} | |
| onChange={(e) => setName(e.target.value)} | |
| style={styles.input} | |
| autoComplete='name' | |
| /> | |
| </div> | |
| )} | |
| <div style={styles.inputWrapper}> | |
| {emailSuggestion && emailSuggestion.startsWith(email) && ( | |
| <> | |
| <div style={ghostInputStyle} aria-hidden='true'> | |
| <span style={{ opacity: 0 }}>{email}</span> | |
| <span style={{ opacity: 0.5 }}>{emailSuggestion.slice(email.length)}</span> | |
| </div> | |
| <div | |
| style={{ | |
| position: 'absolute', | |
| right: '12px', | |
| top: '50%', | |
| transform: 'translateY(-50%)', | |
| zIndex: 10, | |
| opacity: 0.7 | |
| }} | |
| > | |
| <Kbd>Tab</Kbd> | |
| </div> | |
| </> | |
| )} | |
| <input | |
| type='email' | |
| placeholder='Email address' | |
| value={email} | |
| onChange={handleEmailChange} | |
| onKeyDown={handleEmailKeyDown} | |
| required | |
| autoComplete='email' | |
| style={{ | |
| ...styles.input, | |
| position: 'relative', | |
| zIndex: 1, | |
| background: 'transparent', | |
| ...(generalError ? styles.inputError : {}) | |
| }} | |
| /> | |
| </div> | |
| <form onSubmit={handleSubmit} style={styles.form}> | |
| {isRegisterMode && ( | |
| <div style={styles.inputWrapper}> | |
| <label htmlFor="login-name" className="sr-only">Name</label> | |
| <input | |
| id="login-name" | |
| type='text' | |
| placeholder='Name' | |
| value={name} | |
| onChange={(e) => setName(e.target.value)} | |
| style={styles.input} | |
| autoComplete='name' | |
| /> | |
| </div> | |
| )} | |
| <div style={styles.inputWrapper}> | |
| <label htmlFor="login-email" className="sr-only">Email address</label> | |
| {emailSuggestion && emailSuggestion.startsWith(email) && ( | |
| <> | |
| <div style={ghostInputStyle} aria-hidden='true'> | |
| <span style={{ opacity: 0 }}>{email}</span> | |
| <span style={{ opacity: 0.5 }}>{emailSuggestion.slice(email.length)}</span> | |
| </div> | |
| <div | |
| style={{ | |
| position: 'absolute', | |
| right: '12px', | |
| top: '50%', | |
| transform: 'translateY(-50%)', | |
| zIndex: 10, | |
| opacity: 0.7 | |
| }} | |
| > | |
| <Kbd>Tab</Kbd> | |
| </div> | |
| </> | |
| )} | |
| <input | |
| id="login-email" | |
| type='email' | |
| placeholder='Email address' | |
| value={email} | |
| onChange={handleEmailChange} | |
| onKeyDown={handleEmailKeyDown} | |
| required | |
| autoComplete='email' | |
| style={{ | |
| ...styles.input, | |
| position: 'relative', | |
| zIndex: 1, | |
| background: 'transparent', | |
| ...(generalError ? styles.inputError : {}) | |
| }} | |
| /> | |
| </div> |
🤖 Prompt for AI Agents
In `@apps/web/components/auth/login-form.tsx` around lines 500 - 551, The inputs
in the LoginForm component rely only on placeholder text (e.g., the Name input,
the email input handled by handleEmailChange/handleEmailKeyDown, and the
password/confirm-password inputs) which is inaccessible to screen readers; add
explicit, accessible labels for each input by either inserting visually-hidden
<label> elements tied to each input via id/htmlFor or by adding descriptive
aria-label attributes (and keep ids consistent with label htmlFor), and ensure
these changes are applied for Name, Email, Password, and Confirm Password inputs
inside the form handled by handleSubmit so screen readers announce them properly
while preserving current styling and focus behavior.
| lineHeight: '1.5' | ||
| }} | ||
| > | ||
| By clicking "Get Started" (or signing in), you acknowledge that you have read and |
Contributor
There was a problem hiding this comment.
User-facing text mismatch: "Get Started" doesn't match any button label.
The terms copy says "By clicking 'Get Started'" but the actual submit button reads "Sign in" or "Create account." This will confuse users. Update the copy to match the actual CTA.
✏️ Proposed fix
- By clicking "Get Started" (or signing in), you acknowledge that you have read and
+ By continuing, you acknowledge that you have read and🤖 Prompt for AI Agents
In `@apps/web/components/auth/login-form.tsx` at line 689, The terms copy
currently says "By clicking 'Get Started'" which doesn't match the actual CTA;
update the string in the login-form component (LoginForm / the JSX near the
consent text) to reference the actual submit button label used by the form
(e.g., "Sign in" or "Create account") or derive it from the same variable/prop
that renders the <button> label so the copy always matches the CTA; locate the
consent paragraph in apps/web/components/auth/login-form.tsx and replace the
hardcoded "Get Started" text with the actual button label or a reference to the
buttonLabel variable.
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\n- replace standalone /login page with in-app auth modal trigger flow\n- add auth modal context/provider and overlay components\n- update user menu sign-in action to open modal\n- redirect /login to /?auth=1 via proxy and consume query param in provider\n\n## Files\n- apps/web/app/(auth)/login/page.tsx\n- apps/web/app/providers.tsx\n- apps/web/components/auth/user-menu.tsx\n- apps/web/proxy.ts\n- apps/web/components/auth/auth-modal-context.tsx\n- apps/web/components/auth/auth-modal.tsx\n- apps/web/components/auth/auth-overlay.tsx\n- apps/web/components/auth/login-form.tsx\n\n## Notes\n- PR intentionally scoped to auth modal/login UX files only.
Summary by Sourcery
Introduce an in-app authentication modal flow and route redirection to replace the standalone /login page.
New Features:
Enhancements:
Summary by CodeRabbit