-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add password reset flow #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
bitnimble
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@intern address review comments
| } | ||
|
|
||
| runInAction(() => (this.store.submitting = true)); | ||
| const redirectTo = `${window.location.origin}${routeFor([RoutePath.UPDATE_PASSWORD])}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the baseUrl env var instead of window.location.origin
src/app/reset-password/page.tsx
Outdated
| import { ResetPassword } from './reset_password'; | ||
| import { ResetPasswordPresenter, ResetPasswordStore } from './reset_password_presenter'; | ||
|
|
||
| function createResetPasswordPage(supabase: ReturnType<typeof useApi>['supabase']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to DI supabase, it should be possible to use the global create in the presenter directly instead of drilling
src/app/reset-password/page.tsx
Outdated
|
|
||
| return observer(() => { | ||
| return ( | ||
| <ResetPassword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline the contents of reset_password.tsx into here instead of a separate file
src/app/update-password/page.tsx
Outdated
| import { UpdatePassword } from './update_password'; | ||
| import { UpdatePasswordPresenter, UpdatePasswordStore } from './update_password_presenter'; | ||
|
|
||
| function createUpdatePasswordPage(supabase: ReturnType<typeof useApi>['supabase']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here, don't drill this
src/app/update-password/page.tsx
Outdated
|
|
||
| return observer(() => { | ||
| return ( | ||
| <UpdatePassword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here, no need for a separate file for this component
| }); | ||
| } | ||
|
|
||
| onChangePassword = (value: string) => (this.store.password = value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap in mobx action()
| private readonly store: UpdatePasswordStore | ||
| ) { | ||
| super(store); | ||
| makeObservable(this, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove in favour of wrapping the function definition in the action directly
| private readonly store: ResetPasswordStore | ||
| ) { | ||
| super(store); | ||
| makeObservable(this, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove in favour of wrapping the function definition in mobx.action.bound directly
src/utils/routes.ts
Outdated
| MAP = 'map', | ||
| LOGIN = 'login', | ||
| LOGOUT = 'logout', | ||
| RESET_PASSWORD = 'reset-password', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make the reset path /password/reset, and the update route /password/reset/update
|
Claude finished @banana-claude[bot]'s task in 4m 32s —— View job Addressed all review comments
Changes pushed to claude/issue-22-20260106-1209. @bitnimble ready for re-review. |
- Add /reset-password page to request password reset email - Add /update-password page to set new password from reset link - Add "Forgot password?" link on login page - Uses Supabase Auth resetPasswordForEmail and updateUser APIs Co-authored-by: bitnimble <bitnimble@users.noreply.github.com>
- Move routes from /reset-password and /update-password to /password/reset and /password/reset/update - Inline separate component files into page.tsx - Use global createClient() instead of DI for supabase - Use action.bound and setSubmitting/setSuccess helpers instead of runInAction - Use NEXT_PUBLIC_BASE_URL env var instead of window.location.origin Co-authored-by: bitnimble <bitnimble@users.noreply.github.com>
7ffbd28 to
fda66ec
Compare
Add new sections covering: - React patterns (mobx-react-lite, observer structure, page.tsx patterns) - MobX conventions (action.bound, makeObservable generics) - Dependency injection (prefer createClient() at usage site) - Routing conventions (nested routes over hyphens) - Forms/UX (password confirmation, required fields) - TypeScript (don't export internal types) These guidelines address patterns that required manual fixes in the password reset feature implementation. Closes #55 Co-authored-by: bitnimble <bitnimble@users.noreply.github.com>
Summary
/reset-passwordpage to request password reset email via Supabase Auth/update-passwordpage to set new password from reset linkCloses #22
Generated with Claude Code