feat: identifier first login, reset password overhaul#759
feat: identifier first login, reset password overhaul#759anusha-c18 wants to merge 5 commits intocanonical:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #759 +/- ##
==========================================
- Coverage 44.72% 44.12% -0.60%
==========================================
Files 39 40 +1
Lines 3204 3388 +184
==========================================
+ Hits 1433 1495 +62
- Misses 1688 1798 +110
- Partials 83 95 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f39613f to
b627325
Compare
| const renderFlow: LoginFlow | undefined = flow | ||
| ? isIdentifierFirst | ||
| ? { | ||
| ...flow, | ||
| ui: { | ||
| ...flow.ui, | ||
| nodes: flow.ui.nodes.filter((n: UiNode) => { | ||
| if ( | ||
| n.attributes.node_type === "input" && | ||
| typeof (n.attributes as UiNodeInputAttributes).name === "string" | ||
| ) { | ||
| const name = (n.attributes as UiNodeInputAttributes).name; | ||
| return ( | ||
| name === "identifier" || | ||
| name === "csrf_token" || | ||
| name === "method" | ||
| ); | ||
| } | ||
| return false; | ||
| }), | ||
| }, | ||
| } | ||
| : isAuthCode | ||
| ? filterFlow(replaceAuthLabel(flow)) | ||
| : flow |
There was a problem hiding this comment.
issue: nested ternary operator inside ternary operator == hurts to read
we need to improve readability of the code, can you please break it down?
ui/pages/login.tsx
Outdated
| const title = isIdentifierFirst | ||
| ? "Sign in" | ||
| : isAuthCode | ||
| ? "Verify your identity" | ||
| : `Sign in${getTitleSuffix()}`; |
There was a problem hiding this comment.
issue(readability): same as below
ui/components/Password.tsx
Outdated
| onChange={(e) => { | ||
| if (!hasTouched) setHasTouched(true); | ||
| setPassword(e.target.value); | ||
| }} |
There was a problem hiding this comment.
can you please move the inline function to a React.useCallback?
ui/components/Password.tsx
Outdated
| const validateCheck = (check: PasswordCheckType, value: string): boolean => { | ||
| switch (check) { | ||
| case "lowercase": | ||
| return /[a-z]/.test(password) ? "success" : "error"; | ||
| return /[a-z]/.test(value); | ||
| case "uppercase": | ||
| return /[A-Z]/.test(password) ? "success" : "error"; | ||
| return /[A-Z]/.test(value); | ||
| case "number": | ||
| return /[0-9]/.test(password) ? "success" : "error"; | ||
| return /\d/.test(value); | ||
| case "length": | ||
| return password.length >= 8 ? "success" : "error"; | ||
| return value.length >= 8; | ||
| } | ||
| }; |
There was a problem hiding this comment.
issue(readability): please move this function out to refactor it to a static function, so the body of the component is not made heavy to read
ui/components/NodeInputPassword.tsx
Outdated
| setPassword(e.target.value); | ||
| void setValue(e.target.value); | ||
| }} | ||
| onKeyDown={(e) => { |
There was a problem hiding this comment.
same comment on inline function
ui/pages/login.tsx
Outdated
| return fetch( | ||
| `/api/kratos/self-service/login/id-first?flow=${encodeURIComponent(flowId)}`, | ||
| { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| ...values, | ||
| method, | ||
| flow: String(flow?.id), | ||
| }), | ||
| }, | ||
| ) | ||
| .then(async (res) => { | ||
| if (!res.ok) { | ||
| throw new Error(await res.text()); | ||
| } | ||
| return (await res.json()) as IdentifierFirstResponse; | ||
| }) | ||
| .then((data) => { | ||
| if ("redirect_to" in data) { | ||
| window.location.href = data.redirect_to; | ||
| return; | ||
| } | ||
| if (flow?.return_to) { | ||
| window.location.href = flow.return_to; | ||
| return; | ||
| } | ||
| setFlow(data); | ||
| }) | ||
| .catch(redirectToErrorPage); | ||
| } |
There was a problem hiding this comment.
IMO this should be in a function, ideally I think it should be part of the kratos object (defined in kratos.ts)
There was a problem hiding this comment.
Can't it be a method in the kratos sdk object? Also it should re-use the basePath from Kratos sdk, this way we treat it as a single API and if we want to refactor it in the future (e.g. change the basePath) we would just have to change the sdk instantiation instead of having to search the code for hard coded strings.
665f045 to
6204892
Compare
6204892 to
2e5b8d4
Compare
2e5b8d4 to
556d533
Compare
556d533 to
e5af588
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements identifier-first login flow and redesigns the password reset experience with improved validation and user feedback.
Changes:
- Implements identifier-first login with separate email and password entry screens
- Adds debounced password validation with real-time feedback and show/hide password functionality
- Updates E2E tests to accommodate the new login flow with improved screenshot handling
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/util/constants.ts | Adds constants for identifier-first login flow and account error handling |
| ui/tests/reset-password.spec.ts | Updates test to include email input step in identifier-first flow |
| ui/tests/helpers/visual.ts | Adds screenshot configuration for consistent visual testing |
| ui/tests/helpers/oidc_client.ts | Updates assertion text to match new simplified login page title |
| ui/tests/helpers/mail.ts | Improves email retrieval with explicit visibility check |
| ui/tests/helpers/login.ts | Updates login helper to handle two-step identifier-first flow |
| ui/static/sass/styles.scss | Adds styling for password visibility toggle and validation states |
| ui/pages/login.tsx | Implements identifier-first login logic with conditional rendering |
| ui/components/PasswordToggle.tsx | New component providing show/hide password functionality |
| ui/components/PasswordCheck.tsx | Refactors password validation feedback with improved status rendering |
| ui/components/Password.tsx | Adds debounced validation and real-time password matching feedback |
| ui/components/NodeInputText.tsx | Adds email validation and error handling for identifier-first flow |
| ui/components/NodeInputSubmit.tsx | Adds registration CTA for identifier-first login |
| ui/components/NodeInputPassword.tsx | Integrates password toggle and improves error messages |
| ui/components/Flow.tsx | Adds identifier_first to supported authentication methods |
| ui/api/kratos.ts | Implements API function for identifier-first login flow |
| docker/kratos/kratos.yml | Enables identifier-first login style in Kratos configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isValid !== computedValid) { | ||
| setValid(computedValid); | ||
| } | ||
| }, [computedValid, setValid]); |
There was a problem hiding this comment.
The useEffect dependency array is missing 'isValid'. This could cause the effect to not run when isValid changes externally, potentially leading to stale state. Add 'isValid' to the dependency array.
| }, [computedValid, setValid]); | |
| }, [computedValid, isValid, setValid]); |
| const url = new URL(window.location.href); | ||
| url.pathname = "/ui/login"; | ||
| url.searchParams.delete("flow"); | ||
| return url.pathname + url.search; |
There was a problem hiding this comment.
The return value may include a leading '?' even when there are no search parameters after deleting 'flow'. Consider using url.toString() or checking if search params exist before concatenation.
| return url.pathname + url.search; | |
| const search = url.searchParams.toString(); | |
| return search ? `${url.pathname}?${search}` : url.pathname; |
| const isIdentifierFirst = | ||
| flow?.ui.nodes.some( | ||
| (node) => | ||
| node.attributes.node_type === "input" && | ||
| (node.attributes as UiNodeInputAttributes).name === "method" && | ||
| (node.attributes as UiNodeInputAttributes).value === "identifier_first", | ||
| ) ?? false; |
There was a problem hiding this comment.
The isIdentifierFirst check runs on every render. Consider memoizing this computation with useMemo to avoid unnecessary iterations through ui.nodes, especially when the flow object hasn't changed.
62706e3 to
c026363
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return; | ||
| } | ||
|
|
||
| if (flowId && flow) { |
There was a problem hiding this comment.
The condition checks flowId from router query but flowId is not defined in scope. This should be router.query.flow to properly check if a flow ID exists in the URL.
| return "Please enter your email address."; | ||
| } | ||
|
|
||
| if (!/^\S+@\S+\.\S+$/.test(currentValue)) { |
There was a problem hiding this comment.
The email validation regex is overly simplistic and may not catch common email format errors. Consider using a more robust email validation pattern or extracting this to a reusable validation utility.
c026363 to
e05c375
Compare
e05c375 to
423e900
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flow?.requested_aal === "aal2" && | ||
| flow?.ui.nodes.find((node) => node.group === "webauthn") !== undefined; |
There was a problem hiding this comment.
The useEffect hook at line 83 has a missing dependency. The isWebauthn variable computed at line 71-73 depends on flow?.requested_aal and flow?.ui.nodes, but only flow is listed in the dependency array. This could lead to stale values. Consider memoizing isWebauthn with useMemo or adding the individual dependencies.
423e900 to
b5aa9e8
Compare
b5aa9e8 to
ca1499f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Identifier first login
Screen.Recording.2025-09-25.at.8.05.16.PM.mov
Email validation
Reset Password
Screen.Recording.2025-09-25.at.8.09.21.PM.mov