Conversation
|
@greptile |
Greptile SummaryThis PR implements the security settings page with password change, session management, and logout functionality. The implementation correctly imports the shared Key changes:
Issues to address:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/components/settings/pages/security-settings-content.tsx | Implements password change, session management, and logout functionality with form validation, though contains unused code and a minor UX issue with success message state |
| src/components/settings/settings-dialog.tsx | Changed DialogContent from overflow-hidden to overflow-y-auto to enable scrolling in settings dialog |
Last reviewed commit: 96edc26
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const [loading, setLoading] = useState(false); | ||
| const [message, setMessage] = useState<{ type: 'success' | 'error'; text: string } | null>(null); | ||
|
|
||
| const authClient = createAuthClient() |
There was a problem hiding this comment.
Creates a new auth client instance on every render. Move outside component or memoize.
| const authClient = createAuthClient() | |
| const authClient = React.useMemo(() => createAuthClient(), []); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/settings/pages/security-settings-content.tsx
Line: 20:20
Comment:
Creates a new auth client instance on every render. Move outside component or memoize.
```suggestion
const authClient = React.useMemo(() => createAuthClient(), []);
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Yeah this is really important, you should just import it from where greptile mentioned
| @@ -1,3 +1,188 @@ | |||
| import React, { useState } from 'react'; | |||
| import { createAuthClient } from 'better-auth/client'; | |||
There was a problem hiding this comment.
Importing from better-auth/client but existing codebase uses better-auth/react (see src/lib/auth/client.ts:3). Use the shared authClient from @/lib/auth/client instead of creating a new instance.
| import { createAuthClient } from 'better-auth/client'; | |
| import { authClient } from '@/lib/auth/client'; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/settings/pages/security-settings-content.tsx
Line: 2:2
Comment:
Importing from `better-auth/client` but existing codebase uses `better-auth/react` (see `src/lib/auth/client.ts:3`). Use the shared `authClient` from `@/lib/auth/client` instead of creating a new instance.
```suggestion
import { authClient } from '@/lib/auth/client';
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| disabled={loading} | ||
| /> | ||
| </div> | ||
| <Button onClick={handleChangePassword} disabled={loading}> |
There was a problem hiding this comment.
Button is not disabled when required fields are empty. Users can submit with empty passwords.
| <Button onClick={handleChangePassword} disabled={loading}> | |
| <Button onClick={handleChangePassword} disabled={loading || !currentPassword || !newPassword || !confirmPassword}> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/settings/pages/security-settings-content.tsx
Line: 128:128
Comment:
Button is not disabled when required fields are empty. Users can submit with empty passwords.
```suggestion
<Button onClick={handleChangePassword} disabled={loading || !currentPassword || !newPassword || !confirmPassword}>
```
How can I resolve this? If you propose a fix, please make it concise.
theosiemensrhodes
left a comment
theosiemensrhodes
left a comment
There was a problem hiding this comment.
Looks mostly good just fix the comments
| <div className="space-y-4"> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="current-password">Current Password</Label> | ||
| <Input |
There was a problem hiding this comment.
We have a way to do forms that isn't manual. See the login and signup pages for examples.
| const [loading, setLoading] = useState(false); | ||
| const [message, setMessage] = useState<{ type: 'success' | 'error'; text: string } | null>(null); | ||
|
|
||
| const authClient = createAuthClient() |
There was a problem hiding this comment.
Yeah this is really important, you should just import it from where greptile mentioned
|
@greptile |
| const { error } = await authClient.changePassword({ | ||
| currentPassword: data.currentPassword, | ||
| newPassword: data.newPassword, | ||
| revokeOtherSessions: false, |
There was a problem hiding this comment.
revokeOtherSessions: false is a security risk
When a user changes their password, failing to revoke other active sessions means any compromised session (e.g., an attacker who already has a valid token on another device) remains valid after the password change. Revoking other sessions on password change is a widely accepted security best practice.
Consider setting this to true by default — the user can still use the separate "Revoke All Sessions" button if needed, but a password change should at minimum invalidate all sessions besides the current one.
| revokeOtherSessions: false, | |
| revokeOtherSessions: true, |
| } | ||
|
|
||
| reset(); | ||
| setError("root", { type: "success" } as any); |
There was a problem hiding this comment.
Misusing errors.root to track success state
Setting a "success" sentinel value into errors.root via setError("root", { type: "success" } as any) is a hack that misuses react-hook-form's error system. It also requires the as any cast to silence TypeScript, which is a red flag.
A cleaner approach is to use a separate useState variable (similar to how sessionMessage is handled in the session management section):
const [passwordSuccess, setPasswordSuccess] = useState(false);
// on success:
reset();
setPasswordSuccess(true);Then render the success alert based on passwordSuccess instead of isPasswordSuccess, and clear it when the form becomes dirty again.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const isPasswordSuccess = (errors.root as any)?.type === "success"; | ||
|
|
||
| return ( | ||
| <div className="space-y-6 max-h-[calc(100vh-200px)] overflow-y-auto px-1"> |
There was a problem hiding this comment.
Nested scrollbars due to conflicting overflow rules
This wrapper div applies max-h-[calc(100vh-200px)] overflow-y-auto, giving the security content its own independent scrollbar. At the same time, settings-dialog.tsx was changed in this PR from overflow-hidden to overflow-y-auto on the DialogContent. The result is two stacked scrollable containers on the security tab — one inside the other — producing a double-scrollbar experience.
The security content's max-h + overflow-y-auto is redundant because the parent dialog already constrains height and scrolls. Remove these from the content wrapper and let the dialog handle vertical overflow (which is already what the settings-dialog change enables):
| <div className="space-y-6 max-h-[calc(100vh-200px)] overflow-y-auto px-1"> | |
| <div className="space-y-6 px-1"> |
| <div> | ||
| <h3 className="text-lg font-medium">Security</h3> | ||
| <p className="text-sm text-muted-foreground"> | ||
| Manage your account security and sessions | ||
| </p> | ||
| </div> | ||
| <Separator /> |
There was a problem hiding this comment.
Duplicate "Security" heading
settings-dialog.tsx already renders a DialogTitle containing the tab's label (i.e. "Security") for every settings pane. This block renders a second <h3>Security</h3> heading directly inside the content, resulting in two "Security" headings visible in the UI.
The other content components (e.g. AvailabilitySettingsContent) do not add their own headings and instead rely on the DialogTitle from the parent. Consider removing this block and instead adding a description to the security entry in the settingsItems array in settings-dialog.tsx if the subtitle is needed.
|
@greptile |
| } | ||
| }; | ||
|
|
||
| const isPasswordSuccess = (errors.root as any)?.type === "success"; |
There was a problem hiding this comment.
Unused variable - isPasswordSuccess is never referenced. Remove this line.
| const isPasswordSuccess = (errors.root as any)?.type === "success"; |
| </Alert> | ||
| )} | ||
|
|
||
| {passwordSuccess && !isDirty && ( |
There was a problem hiding this comment.
Success message can reappear if user types then clears form back to empty. Once user starts editing (isDirty becomes true), passwordSuccess should be set to false so success doesn't reappear when form returns to default state.
Add useEffect to clear success state:
useEffect(() => {
if (isDirty) setPasswordSuccess(false);
}, [isDirty]);2 participants
No description provided.