WD-33303 add countdown for resend code in verification flow#847
WD-33303 add countdown for resend code in verification flow#847abhigyanghosh30 wants to merge 16 commits intomainfrom
Conversation
d23f078 to
255b958
Compare
BarcoMasile
left a comment
There was a problem hiding this comment.
Refactor needed. This approach relies on two separate timeouts for the same logic, we can do with only one.
Some naming refactors are necessary to improve readability and make intentions clear, so please avoid vague names. Some logic wrapping is necessary here and there.
The only real change would be the count down implementation, the current one is fine but it'd be nice if we could rely on one timeout only. Also, it's important to move the timeout resend logic out of the current NodeInputSubmit component
Code lenght validation is missing from the input, we need to make sure the code is 6 digits.
Also, the email template needs to conform to the design.
In the UI I found two other issues;
- the input for the code is green when empty. From the design it's supposed to be white.
- the info message about the timeout only works once. When you resend the email it doesn't appear again.
ui/components/NodeInputSubmit.tsx
Outdated
| await setValue(attributes.value as string).then(() => | ||
| dispatchSubmit(e), | ||
| ); | ||
| if (node.meta.label?.id === 1070008) { |
There was a problem hiding this comment.
issue(readability): same here please
| ...flow, | ||
| ui: { | ||
| ...flow.ui, | ||
| nodes: flow.ui.nodes.map((node) => { |
There was a problem hiding this comment.
The map approach here is a little overkill, since you're applying two different processing logics to the whole array with if statements to limit your action. The proper way would be to extract the nodes you're interested in, process them as you need, and then fit them back in the array. But I understand that doesn't look as "in order".
issue: I'm ok with the map approach, but the way the logic here mixes 2 different processing needs some refactor to make things clearer, for example through the use of ad-hoc functions
ui/pages/verification.tsx
Outdated
| } | ||
|
|
||
| const Verification: NextPage = () => { | ||
| const UiNodePredicate = (node: UiNode) => |
There was a problem hiding this comment.
can be extracted from the component definition
| if ( | ||
| data.state === "sent_email" && | ||
| data.ui.messages?.find((msg) => msg.type === "error") === undefined | ||
| ) { | ||
| // Check if email is sent and there is no error message | ||
| const codeUiNode = data.ui.nodes.find(UiNodePredicate); | ||
| if (codeUiNode) { | ||
| codeUiNode.messages = [ | ||
| ...codeUiNode.messages, | ||
| { | ||
| id: 11, | ||
| type: "success", | ||
| text: "Code sent. You can request a new one in 00:10s", | ||
| }, | ||
| ]; | ||
| } | ||
| } else if (data.ui.messages?.find((msg) => msg.type === "error")) { | ||
| const codeUiNode = data.ui.nodes.find(UiNodePredicate); | ||
| data.ui.messages?.forEach((message) => { | ||
| if (message.type === "error") { | ||
| codeUiNode?.messages.push({ | ||
| id: message.id, | ||
| type: "error", | ||
| text: "Verification code incorrect. Check your email or resend the code.", | ||
| }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
issue(readibility): same as above, please make this clearer and more readable
There was a problem hiding this comment.
Pull request overview
Adds UX improvements to the email verification flow by introducing a resend-code cooldown/countdown and surfacing clearer verification error/success messaging.
Changes:
- Add resend-code cooldown behavior and success/error messaging in the verification page flow handling.
- Introduce new UI building blocks (
EmailVerificationPrompt,CountDown) and wire them into existing node renderers. - Misc formatting/typing refactors and update Next typed-routes reference for
distDir=dist.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/pages/verification.tsx | Injects email prompt, alters submit handling to add success/error messages, and tweaks label appearance for resend node. |
| ui/components/NodeInputSubmit.tsx | Adds local disabled-state + timeout logic for the resend submit control. |
| ui/components/NodeInputText.tsx | Renders injected before/after components and adds success countdown + adjusted error handling. |
| ui/components/CountDown.tsx | New countdown component used to render the 10s timer text. |
| ui/components/EmailVerificationPrompt.tsx | New helper prompt shown above the code input (includes a copy typo). |
| ui/next-env.d.ts | Updates typed routes reference path to match distDir: "dist". |
| ui/config/useAppConfig.tsx | Formatting-only refactor. |
| ui/util/featureFlags.tsx | Formatting-only refactor. |
| ui/util/replaceAuthLabel.tsx | Formatting-only refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ( | ||
| <p className="u-text--muted"> | ||
| An email with a verification code has been sent to {email}. Please check your | ||
| inbox and enter the code below. If the email is not recieved, ensure the |
There was a problem hiding this comment.
Typo in user-facing copy: "recieved" should be "received".
| inbox and enter the code below. If the email is not recieved, ensure the | |
| inbox and enter the code below. If the email is not received, ensure the |
ui/pages/verification.tsx
Outdated
| if ( | ||
| node.group === "code" && | ||
| node.type === "input" && | ||
| (node.attributes as UiNodeInputAttributes).name === "code" | ||
| ) { | ||
| if (node.meta.label) { | ||
| node.meta.label.context = { | ||
| ...node.meta.label.context, | ||
| beforeComponent: <EmailVerificationPrompt email={userEmail} />, | ||
| }; | ||
| } | ||
| } | ||
| if (node.meta.label?.id === 1070008) { | ||
| node.meta.label.context = { | ||
| ...node.meta.label.context, | ||
| appearance: "link", | ||
| }; | ||
| } | ||
| return node; | ||
| }), | ||
| }, | ||
| }; | ||
| }, [flow]); |
There was a problem hiding this comment.
lookupFlow maps over flow.ui.nodes but mutates each existing node (node.meta.label.context = ...) before returning it. This mutates the flow object held in React state and can lead to subtle bugs with memoization/state updates. Prefer returning new node objects when you need to adjust label context (clone node / meta / label / context as needed) instead of mutating in place.
| if ( | |
| node.group === "code" && | |
| node.type === "input" && | |
| (node.attributes as UiNodeInputAttributes).name === "code" | |
| ) { | |
| if (node.meta.label) { | |
| node.meta.label.context = { | |
| ...node.meta.label.context, | |
| beforeComponent: <EmailVerificationPrompt email={userEmail} />, | |
| }; | |
| } | |
| } | |
| if (node.meta.label?.id === 1070008) { | |
| node.meta.label.context = { | |
| ...node.meta.label.context, | |
| appearance: "link", | |
| }; | |
| } | |
| return node; | |
| }), | |
| }, | |
| }; | |
| }, [flow]); | |
| const isCodeInputNode = | |
| node.group === "code" && | |
| node.type === "input" && | |
| (node.attributes as UiNodeInputAttributes).name === "code"; | |
| const label = node.meta?.label; | |
| const shouldAddBeforeComponent = isCodeInputNode && !!label; | |
| const shouldSetAppearanceLink = label?.id === 1070008; | |
| if (!shouldAddBeforeComponent && !shouldSetAppearanceLink) { | |
| return node; | |
| } | |
| const newContext = { | |
| ...label?.context, | |
| ...(shouldAddBeforeComponent && { | |
| beforeComponent: <EmailVerificationPrompt email={userEmail} />, | |
| }), | |
| ...(shouldSetAppearanceLink && { | |
| appearance: "link", | |
| }), | |
| }; | |
| const newLabel = { | |
| ...label, | |
| context: newContext, | |
| }; | |
| const newMeta = { | |
| ...node.meta, | |
| label: newLabel, | |
| }; | |
| return { | |
| ...node, | |
| meta: newMeta, | |
| }; | |
| }), | |
| }, | |
| }; | |
| }, [flow, userEmail]); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 21 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If no error message, add success message and disable resend button for 10 seconds | ||
| const codeUiNode = data.ui.nodes.find(UiNodePredicate); | ||
| if (codeUiNode) { | ||
| codeUiNode.messages = [ | ||
| ...codeUiNode.messages, | ||
| { | ||
| id: 11, | ||
| type: "success", | ||
| text: "Code sent. You can request a new one in 00:10s", | ||
| }, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
The success message is being added to the node messages array, but it will never be displayed to the user because the countdown logic in NodeInputText.tsx (lines 90-100) replaces it with a CountDownText component. The text "Code sent. You can request a new one in 00:10s" will never be shown. Consider either removing this hardcoded text or ensuring consistency between the message text here and what's displayed by CountDownText.
| // If no error message, add success message and disable resend button for 10 seconds | |
| const codeUiNode = data.ui.nodes.find(UiNodePredicate); | |
| if (codeUiNode) { | |
| codeUiNode.messages = [ | |
| ...codeUiNode.messages, | |
| { | |
| id: 11, | |
| type: "success", | |
| text: "Code sent. You can request a new one in 00:10s", | |
| }, | |
| ]; | |
| } |
| data.ui.messages?.forEach((message) => { | ||
| if (message.type === "error") { | ||
| codeUiNode?.messages.push({ | ||
| id: message.id, | ||
| type: "error", | ||
| text: "Verification code incorrect. Check your email or resend the code.", | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Mutating the data object received from the API is not a good practice. The code directly modifies 'codeUiNode.messages' by pushing new messages. This can lead to unexpected side effects and makes the code harder to reason about. Consider creating a new flow object with updated messages rather than mutating the existing data structure.
| const beforeComponent = ( | ||
| node.meta.label?.context as { | ||
| beforeComponent: Component; | ||
| } |
There was a problem hiding this comment.
The 'Component' type from React is incorrect here. It should be 'React.ReactElement' or 'React.ReactNode' since you're storing JSX elements, not component classes. The Component type refers to class-based React components, not rendered elements.
| if ( | ||
| node.group === "code" && | ||
| node.type === "input" && | ||
| (node.attributes as UiNodeInputAttributes).name === "code" | ||
| ) { | ||
| if (node.meta.label) { | ||
| node.meta.label.context = { | ||
| ...node.meta.label.context, | ||
| beforeComponent: <EmailVerificationPrompt email={userEmail} />, | ||
| }; | ||
| } | ||
| } | ||
| if (isResendVerificationCode(node)) { | ||
| node.meta.label.context = { | ||
| ...node.meta.label.context, | ||
| appearance: "link", | ||
| }; | ||
| (node.attributes as UiNodeInputAttributes).disabled = resendDisabled; | ||
| } | ||
| return node; |
There was a problem hiding this comment.
Mutating the node object directly within the map function violates immutability principles. The code modifies node.meta.label.context and node.attributes.disabled in place. This can lead to unexpected side effects since the original flow object is being mutated. Instead, create and return new node objects with the updated properties.
| if ( | |
| node.group === "code" && | |
| node.type === "input" && | |
| (node.attributes as UiNodeInputAttributes).name === "code" | |
| ) { | |
| if (node.meta.label) { | |
| node.meta.label.context = { | |
| ...node.meta.label.context, | |
| beforeComponent: <EmailVerificationPrompt email={userEmail} />, | |
| }; | |
| } | |
| } | |
| if (isResendVerificationCode(node)) { | |
| node.meta.label.context = { | |
| ...node.meta.label.context, | |
| appearance: "link", | |
| }; | |
| (node.attributes as UiNodeInputAttributes).disabled = resendDisabled; | |
| } | |
| return node; | |
| let updatedNode = node; | |
| if ( | |
| node.group === "code" && | |
| node.type === "input" && | |
| (node.attributes as UiNodeInputAttributes).name === "code" | |
| ) { | |
| if (node.meta.label) { | |
| updatedNode = { | |
| ...updatedNode, | |
| meta: { | |
| ...updatedNode.meta, | |
| label: { | |
| ...node.meta.label, | |
| context: { | |
| ...(node.meta.label.context || {}), | |
| beforeComponent: ( | |
| <EmailVerificationPrompt email={userEmail} /> | |
| ), | |
| }, | |
| }, | |
| }, | |
| }; | |
| } | |
| } | |
| if (isResendVerificationCode(node) && node.meta.label) { | |
| const attrs = node.attributes as UiNodeInputAttributes; | |
| updatedNode = { | |
| ...updatedNode, | |
| meta: { | |
| ...updatedNode.meta, | |
| label: { | |
| ...node.meta.label, | |
| context: { | |
| ...(node.meta.label.context || {}), | |
| appearance: "link", | |
| }, | |
| }, | |
| }, | |
| attributes: { | |
| ...attrs, | |
| disabled: resendDisabled, | |
| } as UiNodeInputAttributes, | |
| }; | |
| } | |
| return updatedNode; |
ui/pages/verification.tsx
Outdated
| codeUiNode.messages = [ | ||
| ...codeUiNode.messages, | ||
| { | ||
| id: 11, |
There was a problem hiding this comment.
The hardcoded message ID '11' appears arbitrary and could conflict with actual message IDs from the backend. Consider using a unique identifier or a more appropriate approach to distinguish client-added messages from server messages, such as using a negative number or a specific prefix.
| id: 11, | |
| id: -1, |
| @@ -60,30 +85,47 @@ export const NodeInputText: FC<NodeInputProps> = ({ | |||
| } | |||
|
|
|||
| return undefined; | |||
| }; | |||
| }, [message, node.messages, isDuplicate, attributes.name, isWebauthn, error]); | |||
There was a problem hiding this comment.
The getError function is converted to useMemo but is now being passed directly to the error prop. Since useMemo returns the computed value (not a function), the error prop receives the error string or undefined directly. This is correct, but the naming 'getError' is misleading since it's no longer a function. Consider renaming to 'errorMessage' or similar to reflect that it's now a value, not a function.
| node.meta.label.context = { | ||
| ...node.meta.label.context, | ||
| appearance: "link", | ||
| }; |
There was a problem hiding this comment.
Potential null reference error. The code accesses node.meta.label.context without checking if node.meta.label exists first (the check on line 194 only guards the inner block). If isResendVerificationCode returns true but node.meta.label is undefined, this will throw a runtime error. Add a null check before accessing node.meta.label.context.
| node.meta.label.context = { | |
| ...node.meta.label.context, | |
| appearance: "link", | |
| }; | |
| if (node.meta?.label) { | |
| node.meta.label.context = { | |
| ...node.meta.label.context, | |
| appearance: "link", | |
| }; | |
| } |
ui/pages/verification.tsx
Outdated
| const emailNode = flow.ui.nodes.find( | ||
| (node) => (node.attributes as UiNodeInputAttributes).name === "email", | ||
| ); | ||
| return emailNode ? (emailNode.attributes as UiNodeInputAttributes).value : ""; |
There was a problem hiding this comment.
The type assertion as UiNodeInputAttributes is used without checking if the node attributes are actually of type UiNodeInputAttributes. This could cause runtime errors if the node has a different attribute type. Consider adding a type guard to verify the node type before accessing the 'value' property.
ui/pages/verification.tsx
Outdated
| <PageLayout title="Verify your email"> | ||
| {flow ? <Flow onSubmit={handleSubmit} flow={flow} /> : <Spinner />} | ||
| <PageLayout title="Check your email"> | ||
| {flow ? <Flow onSubmit={handleSubmit} flow={lookupFlow} /> : <Spinner />} |
There was a problem hiding this comment.
This use of variable 'flow' always evaluates to true.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const disableButtonWithTimeout = () => { | ||
| setResendDisabled(true); | ||
| const timer = setTimeout(() => { | ||
| setResendDisabled(false); | ||
| }, RESEND_CODE_TIMEOUT); | ||
| return () => clearTimeout(timer); | ||
| }; |
There was a problem hiding this comment.
Potential memory leak: The cleanup function returned from disableButtonWithTimeout is never used. When this function is called, it returns a cleanup function but that return value is ignored. This means the timer is never cleared if the component unmounts while the timer is active.
| }>; | ||
| } | ||
| ).continue_with; | ||
| if (continue_with[0].action === "redirect_browser_to") { |
There was a problem hiding this comment.
Array access without bounds checking: The code accesses continue_with[0] without verifying that the array has at least one element. If continue_with is an empty array, this will result in a runtime error. Add a check to ensure the array is not empty before accessing its first element.
| if (continue_with[0].action === "redirect_browser_to") { | |
| if ( | |
| continue_with.length > 0 && | |
| continue_with[0].action === "redirect_browser_to" | |
| ) { |
ui/pages/verification.tsx
Outdated
| <PageLayout title="Verify your email"> | ||
| {flow ? <Flow onSubmit={handleSubmit} flow={flow} /> : <Spinner />} | ||
| <PageLayout title="Check your email"> | ||
| {flow ? <Flow onSubmit={handleSubmit} flow={lookupFlow} /> : <Spinner />} |
There was a problem hiding this comment.
This use of variable 'flow' always evaluates to true.
| {flow ? <Flow onSubmit={handleSubmit} flow={lookupFlow} /> : <Spinner />} | |
| <Flow onSubmit={handleSubmit} flow={lookupFlow} /> |
ui/components/CountDownText.tsx
Outdated
| initialSeconds?: number; | ||
| }) => { | ||
| const [seconds, setSeconds] = useState(initialSeconds); | ||
| console.log("CountDownText rendered with seconds:", initialSeconds); |
There was a problem hiding this comment.
question: can we remove these console.logs?
| ]; | ||
| } | ||
| // Disable resend button for 10 seconds | ||
| disableButtonWithTimeout(); |
There was a problem hiding this comment.
issue: as copilot states, this function returns another function that needs to be called, but it's not, effectively non clearing the timer.
f9ab970 to
33770c7
Compare
natalian98
left a comment
There was a problem hiding this comment.
I got the following lint error when building the app with make npm-build build:
Failed to compile.
./pages/verification.tsx
234:16 Error: Replace `·title={flow.state==="passed_challenge"?"Verification·successful":"Check·your·email"}` with `⏎······title={⏎········flow.state·===·"passed_challenge"⏎··········?·"Verification·successful"⏎··········:·"Check·your·email"⏎······}⏎····` prettier/prettier
There was a problem hiding this comment.
Email address validation is not working properly. For example, if you type test@e no error is displayed.
| return ( | ||
| <p className="u-text--muted"> | ||
| An email with a verification code has been sent to {email}. Please check | ||
| your inbox and enter the code below. If the email is not recieved, ensure |
There was a problem hiding this comment.
| your inbox and enter the code below. If the email is not recieved, ensure | |
| your inbox and enter the code below. If the email is not received, ensure |
There was a problem hiding this comment.
Some questions about the successful verification scenario.
According to the design we should get a message "You will be redirected to ..." which should then redirect to return_to url if included in parameters. Can we implement this? For reference, we have this kind of pattern for setup complete page.
Right now after successful verification, I get redirected to a page with Continue button:
Upon clicking on it I get to /ui which doesn't exist. Could we instead redirect to manage_details page if return_to parameter is not specified?
|
Please increase the timer to 60s instead of the current 10s, to allow for reasonable time for the code to be sent from the server and 10s is too short for the purpose of having a cool off time to avoid any abuse of continuous activity on the resend code button. |
265a37c to
6a86de7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 7 comments.
Files not reviewed (1)
- ui/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const resendButton = flow.ui.nodes.find(isResendVerificationCode); | ||
| return ( | ||
| <> | ||
| <span>{getNodeLabel(node)}</span> | ||
| <Button | ||
| appearance={"link"} | ||
| tabIndex={4} | ||
| onClick={async (e) => { | ||
| e.preventDefault(); | ||
| // On click, we set this value, and once set, dispatch the submission! | ||
| await setValue( | ||
| (resendButton?.attributes as UiNodeInputAttributes) | ||
| .value as string, | ||
| resendButton ? getNodeId(resendButton) : undefined, | ||
| ).then(() => dispatchSubmit(e)); | ||
| }} | ||
| style={{ float: "right", marginBottom: 0 }} | ||
| disabled={ | ||
| (resendButton?.attributes as UiNodeInputAttributes).disabled | ||
| } |
There was a problem hiding this comment.
In the resend button label, resendButton can be undefined, but the code still dereferences (resendButton?.attributes as UiNodeInputAttributes).value/disabled. Because the optional chain only applies to attributes, this can still evaluate to undefined.disabled/undefined.value and crash. Guard for a missing resend node (e.g., don’t render the button or make it disabled) before reading its attributes.
| useEffect(() => { | ||
| if (message) { | ||
| setInputValue(message); | ||
| } | ||
| }, [message, setInputValue]); | ||
|
|
||
| const getError = () => { | ||
| const beforeComponent = ( | ||
| node.meta.label?.context as { | ||
| beforeComponent: React.ReactNode; | ||
| } | ||
| )?.beforeComponent; | ||
|
|
||
| const afterComponent = ( | ||
| node.meta.label?.context as { | ||
| afterComponent: React.ReactNode; | ||
| } | ||
| )?.afterComponent; | ||
|
|
||
| useEffect(() => { | ||
| if (node.messages.length === 0) { | ||
| return; | ||
| } | ||
| for (const msg of node.messages) { | ||
| if (msg.type !== "info") { | ||
| return; | ||
| } | ||
| } | ||
| if (message) { | ||
| setInputValue(message); | ||
| } | ||
| }, [message, setInputValue]); |
There was a problem hiding this comment.
The new useEffect that checks node.messages (info-only) doesn’t include node.messages in its dependency list, so it won’t rerun when messages change (which is the main trigger for this logic). Also, the earlier useEffect already sets inputValue for any non-empty message, which makes the info-only effect ineffective and can overwrite user input with error text. Consider removing/conditioning the first effect and add node.messages (or message computed from it) to the dependency list of the remaining effect.
| const [resendDisabled, setResendDisabled] = useState<boolean>(false); | ||
| const disableButtonWithTimeout = () => { | ||
| setResendDisabled(true); | ||
|
|
||
| const timer = setTimeout(() => { | ||
| setResendDisabled(false); | ||
| clearTimeout(timer); | ||
| }, RESEND_CODE_TIMEOUT); | ||
| return () => clearTimeout(timer); | ||
| }; |
There was a problem hiding this comment.
disableButtonWithTimeout returns a cleanup function but the return value is never used, so the timeout won’t be cleared on unmount. This can lead to state updates on an unmounted component and flaky behavior if the user navigates away. Store the timeout id in a ref and clear it in a useEffect cleanup (and avoid calling clearTimeout from inside the timeout callback).
| if ( | ||
| data.state === "sent_email" && | ||
| data.ui.messages?.find((msg) => msg.type === "error") === undefined | ||
| ) { | ||
| // Check if email is sent and there is no error message | ||
| // If no error message, add success message and disable resend button for 10 seconds | ||
| const codeUiNode = data.ui.nodes.find(UiNodePredicate) as UiNode; | ||
| if (codeUiNode) { | ||
| codeUiNode.meta = { | ||
| ...codeUiNode.meta, | ||
| label: { | ||
| ...codeUiNode.meta.label, | ||
| context: { | ||
| ...codeUiNode.meta.label?.context, | ||
| afterComponent: ( | ||
| <CountDownText | ||
| initialSeconds={RESEND_CODE_TIMEOUT / 1000} | ||
| wrapperText="Code sent. You can request again in 00:" | ||
| key={new Date().toISOString()} | ||
| /> | ||
| ), | ||
| }, | ||
| }, | ||
| } as UiNodeMeta; | ||
| } | ||
| // Disable resend button for 10 seconds | ||
| disableButtonWithTimeout(); | ||
| } else if (data.ui.messages?.find((msg) => msg.type === "error")) { | ||
| const codeUiNode = data.ui.nodes.find(UiNodePredicate); | ||
| data.ui.messages?.forEach((message) => { | ||
| if (message.type === "error") { | ||
| codeUiNode?.messages.push({ | ||
| id: message.id, | ||
| type: "error", | ||
| text: "Verification code incorrect. Check your email or resend the code.", | ||
| }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
There are Playwright specs for other auth flows under ui/tests, but the new verification resend cooldown / countdown behavior has no automated coverage. Add a Playwright test for /ui/verification that asserts the resend control disables for ~10s and re-enables, and that an invalid code shows the new error text.
| return ( | ||
| <p className="u-text--muted"> | ||
| An email with a verification code has been sent to {email}. Please check | ||
| your inbox and enter the code below. If the email is not recieved, ensure |
There was a problem hiding this comment.
Typo: “recieved” should be “received”.
| your inbox and enter the code below. If the email is not recieved, ensure | |
| your inbox and enter the code below. If the email is not received, ensure |
| {children} | ||
| </AppConfigContext> | ||
| ) | ||
| return <AppConfigContext value={contextValue}>{children}</AppConfigContext>; |
There was a problem hiding this comment.
AppConfigProvider is rendering the context object directly (<AppConfigContext ...>) instead of the provider (<AppConfigContext.Provider ...>). This will throw at runtime (invalid element type) and prevents consumers from receiving the context value. Update the return to use AppConfigContext.Provider (and close the matching tag).
| return <AppConfigContext value={contextValue}>{children}</AppConfigContext>; | |
| return ( | |
| <AppConfigContext.Provider value={contextValue}> | |
| {children} | |
| </AppConfigContext.Provider> | |
| ); |
ui/pages/verification.tsx
Outdated
| : "Check your email" | ||
| } | ||
| > | ||
| {flow ? <Flow onSubmit={handleSubmit} flow={lookupFlow} /> : <Spinner />} |
There was a problem hiding this comment.
This use of variable 'flow' always evaluates to true.
| {flow ? <Flow onSubmit={handleSubmit} flow={lookupFlow} /> : <Spinner />} | |
| <Flow onSubmit={handleSubmit} flow={lookupFlow} /> |
9a2fce7 to
5f346a2
Compare
845ec49 to
ff9a463
Compare
ui/pages/verification.tsx
Outdated
| function setFlowIDQueryParam(router: NextRouter, flowId: string) { | ||
| void router.push( | ||
| { | ||
| pathname: router.pathname, |
There was a problem hiding this comment.
We need to remove this to fix the paths issue, otherwise any request to ui/verification is rewritten to /verification and you can't refresh the page
| pathname: router.pathname, |
nsklikas
left a comment
There was a problem hiding this comment.
Not a frontend engineer so I will not comment on the code, but there are some issues with the implementation.
Run some tests on my own and also used antigravity to test it, this is the report generated from antigravity:
Summary
The PR successfully implements the core "countdown" and "disable button" logic. However, several critical issues regarding state persistence, routing, and user experience were identified during testing. These issues make the feature fragile and easily bypassable.
✅ Verified Features
- Countdown Timer: Correctly initialized when valid code is sent.
- Resend Disable: Button is correctly disabled during countdown.
- Input Validation: Invalid codes are correctly rejected with appropriate error messages.
- Success Flow: Valid codes lead to successful verification and redirection.
⚠️ Issues & Change Requests
1. Countdown Persistence (Critical)
- Issue: The countdown functionality relies entirely on the temporary page state. Reloading the page resets the state, effectively clearing the countdown.
- Impact: Users can bypass the resend throttle by simply refreshing the browser.
- Recommendation: Persist the timestamp (e.g., in
localStorageorsessionStorage) so the countdown resumes correctly after a reload.
2. Routing & URL Structure
- Issue: During the verification flow, the browser URL redirects to
/verification?flow=..., dropping the necessary/uiprefix. - Impact: If a user reloads the page while in the flow, they receive a 404 Not Found error because the application is served under
/ui. - Recommendation: Ensure all redirects and router pushes maintain the
/uiprefix.
3. Back Button Behavior
- Issue: Clicking the browser's "Back" button from the code entry screen changes the URL but does not update the UI (it remains on the code entry screen). Subsequent clicks lead to 404 errors.
- Impact: Users cannot navigate back to correct a typo in their email address.
- Recommendation: Ensure the router history is correctly managed (e.g., using
replaceinstead ofpushwhere appropriate, or handling popstate events) to allow proper navigation.
4. Response Message UX
- Issue: The success notification displays: "You will be redirected to /ui/manage_details".
- Impact: This exposes internal URL paths to the end-user, looking unpolished.
- Recommendation: Update the text to a user-friendly message, such as "You will be redirected to your dashboard" or simply "Redirecting...".
5. Email Link
- Note: The verification link sent in emails (
/self-service/verification...) is incorrect and leads to a 404.
ui/pages/verification.tsx
Outdated
| import CountDownText from "../components/CountDownText"; | ||
|
|
||
| function setFlowIDQueryParam(router: NextRouter, flowId: string) { | ||
| void router.push( |
There was a problem hiding this comment.
question: In some other places I see that replaceState is used. AFAICT replaceState replaces the entry in history where as a push adds a new entry. Maybe this is a different case and using push makes sense.
ui/pages/verification.tsx
Outdated
| function setFlowIDQueryParam(router: NextRouter, flowId: string) { | ||
| void router.push( | ||
| { | ||
| pathname: router.pathname, |
There was a problem hiding this comment.
issue: This is wrong. AFAICT you are setting the browser path based on the pathname in the nextjs router. These 2 paths are not the same, nextjs cannot know the actual route of the page as most of the time the files are served using a reverse proxy.
For example if you go to the verification page and try to refresh you will get a 404 error.
| if (isVerificationCodeInput(node)) { | ||
| const resendButton = flow.ui.nodes.find(isResendVerificationCode); | ||
| return ( | ||
| <> | ||
| <span>{getNodeLabel(node)}</span> | ||
| <Button | ||
| appearance={"link"} | ||
| tabIndex={4} | ||
| onClick={async (e) => { | ||
| e.preventDefault(); | ||
| // On click, we set this value, and once set, dispatch the submission! | ||
| await setValue( | ||
| (resendButton?.attributes as UiNodeInputAttributes) | ||
| .value as string, | ||
| resendButton ? getNodeId(resendButton) : undefined, | ||
| ).then(() => dispatchSubmit(e)); | ||
| }} | ||
| style={{ float: "right", marginBottom: 0 }} | ||
| disabled={ | ||
| (resendButton?.attributes as UiNodeInputAttributes).disabled | ||
| } | ||
| > | ||
| Resend code | ||
| </Button> | ||
| </> | ||
| ); | ||
| } | ||
| return getNodeLabel(node); | ||
| }, [node, flow]); |
There was a problem hiding this comment.
issue: Not sure if this is the correct approach. If I go to the verification page and input my email, I will get an email and the Resend code button will be unclickable. BUT if I just refresh the page (I have to change the path because of the bug with router.push), I can click the resend button even if 1min has not passed. Not sure how this should be implemented.
5f346a2 to
a2babee
Compare
| afterComponent: ( | ||
| <CountDownText | ||
| initialSeconds={RESEND_CODE_TIMEOUT / 1000} | ||
| wrapperText="Code sent. You can request again in 00:" |
There was a problem hiding this comment.
The countdown is gone after you refresh the page.
also a nit: At the beginning of the countdown, 00:60s is shown. Should be 01:00 perhaps?
ff9a463 to
5eeba57
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- ui/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [hasLocalValidation, setLocalValidation] = React.useState(false); | ||
|
|
||
| const emailRegex = /^[^\s@]+@[^\s@]+$/; | ||
| const emailRegex = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,6}$/; |
There was a problem hiding this comment.
The email regex limits TLD (top-level domain) to 2-6 characters, but some valid TLDs can be longer (e.g., ".construction" has 12 characters). Consider using a more flexible pattern like {2,} to allow TLDs of any length 2 or greater, or verify that the 6-character limit aligns with your application's requirements.
| const emailRegex = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,6}$/; | |
| const emailRegex = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/; |
|
|
||
| const timer = setTimeout(() => { | ||
| setResendDisabled(false); | ||
| clearTimeout(timer); | ||
| }, RESEND_CODE_TIMEOUT); | ||
| return () => clearTimeout(timer); | ||
| }; | ||
|
|
There was a problem hiding this comment.
The cleanup function returned from disableButtonWithTimeout will never be used because it's not being returned from a useEffect or stored anywhere. The function is called directly (line 157), not used as an effect cleanup. This means the timer could potentially continue running even after the component unmounts, leading to a memory leak. Consider moving this logic into a useEffect hook with proper cleanup, or remove the unused return statement.
| const timer = setTimeout(() => { | |
| setResendDisabled(false); | |
| clearTimeout(timer); | |
| }, RESEND_CODE_TIMEOUT); | |
| return () => clearTimeout(timer); | |
| }; | |
| }; | |
| useEffect(() => { | |
| if (!resendDisabled) { | |
| return; | |
| } | |
| const timer = setTimeout(() => { | |
| setResendDisabled(false); | |
| }, RESEND_CODE_TIMEOUT); | |
| return () => { | |
| clearTimeout(timer); | |
| }; | |
| }, [resendDisabled]); |
| window.location.href = returnTo; | ||
| } else { | ||
| const timer = setTimeout(() => { | ||
| clearTimeout(timer); |
There was a problem hiding this comment.
Calling clearTimeout immediately after setting the timeout is redundant since the timeout has already fired at that point. The clearTimeout call on line 107 has no effect and can be removed.
| clearTimeout(timer); |
| // Disable resend button for 10 seconds | ||
| disableButtonWithTimeout(); |
There was a problem hiding this comment.
The comment says "Disable resend button for 10 seconds" but the actual timeout is RESEND_CODE_TIMEOUT which is 60000ms (60 seconds). Update the comment to match the actual timeout duration.
|
|
||
| const timer = setTimeout(() => { | ||
| setResendDisabled(false); | ||
| clearTimeout(timer); |
There was a problem hiding this comment.
Calling clearTimeout inside the setTimeout callback is redundant since the timeout has already fired at that point. The clearTimeout call on line 43 has no effect and can be removed. The cleanup function returned on line 45 would be the proper place to clear the timeout if it were actually used.
| clearTimeout(timer); |
| setInputValue(newValue); | ||
| void setValue(newValue); | ||
| }, | ||
| [setValue], |
There was a problem hiding this comment.
The handleChange callback is missing node and setInputValue in its dependency array. The callback uses node to check if it's a verification code input and uses setInputValue to update local state, but only includes setValue in the dependency array. This could lead to stale closures. Add node and setInputValue to the dependency array.
| [setValue], | |
| [setValue, node, setInputValue], |
…meout during verification flow
…e into a constant
…orm values for any key from any node
…r input node functions
…and limit code input to numbers onlu
…from the designs of other success pages
5eeba57 to
6975cdc
Compare
… with other routes. Timer shows minutes and seconds in countdown text
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 9 comments.
Files not reviewed (1)
- ui/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
ui/pages/verification.tsx:88
- Missing dependency in useEffect: The effect on line 53 uses
verificationCode(line 62) but doesn't include it in the dependency array on line 88. This could cause the effect to use stale values ifverificationCodechanges. AddverificationCodeto the dependency array.
useEffect(() => {
if (!router.isReady || flow) {
return;
}
if (flowId) {
kratos
.getVerificationFlow({ id: String(flowId) })
.then(({ data }) => {
if (verificationCode) {
const codeUiNode = data.ui.nodes.find(isVerificationCodeInput);
if (codeUiNode) {
(codeUiNode.attributes as UiNodeInputAttributes).value =
String(verificationCode);
}
}
setFlowIDQueryParam(String(data.id));
setFlow(data);
})
.catch(handleFlowError("verification", setFlow))
.catch(redirectToErrorPage);
return;
}
kratos
.createBrowserVerificationFlow({
returnTo: returnTo ? String(returnTo) : undefined,
})
.then(({ data }) => {
setFlow(data);
setFlowIDQueryParam(String(data.id));
})
.catch(handleFlowError("verification", setFlow))
.catch(redirectToErrorPage);
}, [flowId, router, router.isReady, returnTo]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| severity="positive" | ||
| borderless | ||
| className="u-no-margin--bottom" | ||
| >{`${wrapperText}${seconds >= 60 ? `${Math.floor(seconds / 60)}:${(seconds % 60).toString().padStart(2, "0")}` : `${seconds.toString().padStart(2, "0")}s`}`}</Notification> |
There was a problem hiding this comment.
The long inline ternary expression on line 34 is difficult to read and maintain. Consider extracting the time formatting logic into a separate function for better readability. For example, create a formatTime function that takes seconds and returns the formatted string.
| setResendDisabled(false); | ||
| clearTimeout(timer); | ||
| }, RESEND_CODE_TIMEOUT); | ||
| return () => clearTimeout(timer); |
There was a problem hiding this comment.
Memory leak: The cleanup function returns the result of clearTimeout, which should return void. The cleanup function should not return anything. Remove the return statement on line 45.
| return () => clearTimeout(timer); |
|
|
||
| const getLabel = useMemo(() => { | ||
| if (isVerificationCodeInput(node)) { | ||
| const resendButton = flow.ui.nodes.find(isResendVerificationCode); |
There was a problem hiding this comment.
Unsafe context access: On line 110, the code accesses flow.ui.nodes without checking if flow is properly initialized. Given that FlowContext is initialized with an empty object cast to the flow types (see FlowContext.tsx:10-11), this will cause a runtime error when trying to access ui.nodes on an empty object. Add a null check or ensure this component is only rendered within a proper FlowContext.Provider.
| const resendButton = flow.ui.nodes.find(isResendVerificationCode); | |
| const resendButton = flow?.ui?.nodes?.find(isResendVerificationCode); |
| <Button | ||
| appearance={"link"} | ||
| tabIndex={4} | ||
| onClick={async (e) => { | ||
| e.preventDefault(); | ||
| // On click, we set this value, and once set, dispatch the submission! | ||
| await setValue( | ||
| (resendButton?.attributes as UiNodeInputAttributes) | ||
| .value as string, | ||
| resendButton ? getNodeId(resendButton) : undefined, | ||
| ).then(() => dispatchSubmit(e)); | ||
| }} | ||
| style={{ float: "right", marginBottom: 0 }} |
There was a problem hiding this comment.
The "Resend code" button uses inline styles with float: "right" (line 126), which can cause accessibility issues with screen readers as the visual order differs from the DOM order. The button appears after the label text in the DOM but is displayed on the right side visually. Consider using CSS flexbox or grid for proper layout instead of float to maintain consistency between visual and DOM order.
| }); | ||
| }, 1000); | ||
| return () => clearInterval(timerId); | ||
| }, [initialSeconds]); |
There was a problem hiding this comment.
The initialSeconds dependency in the useEffect on line 23 will cause the timer to restart if initialSeconds changes. Since this prop shouldn't change during the component's lifecycle, this dependency should be removed to prevent unnecessary timer resets. Use an empty dependency array [] instead.
| }, [initialSeconds]); | |
| }, []); |
| if (node.meta.label) { | ||
| node.meta.label.context = { | ||
| ...node.meta.label.context, | ||
| beforeComponent: <EmailVerificationPrompt email={userEmail} />, | ||
| }; | ||
| } | ||
| } | ||
| if (isResendVerificationCode(node)) { | ||
| node.meta.label.context = { | ||
| ...node.meta.label.context, | ||
| appearance: "link", | ||
| }; | ||
| (node.attributes as UiNodeInputAttributes).disabled = | ||
| resendDisabled; | ||
| } | ||
| return node; | ||
| }), | ||
| }, | ||
| }; | ||
| }, [flow, resendDisabled]); |
There was a problem hiding this comment.
Direct mutation of node object: The code directly mutates the node.meta.label.context object (lines 213-216 and 220-225) within a useMemo. This is problematic because: 1) It mutates the original flow data structure, which could cause issues if the flow object is used elsewhere, and 2) Mutations inside useMemo violate React's principles and can lead to unpredictable behavior. Instead, create a deep copy of the node before mutating it, or restructure this to return a new object without mutations.
| if (node.meta.label) { | |
| node.meta.label.context = { | |
| ...node.meta.label.context, | |
| beforeComponent: <EmailVerificationPrompt email={userEmail} />, | |
| }; | |
| } | |
| } | |
| if (isResendVerificationCode(node)) { | |
| node.meta.label.context = { | |
| ...node.meta.label.context, | |
| appearance: "link", | |
| }; | |
| (node.attributes as UiNodeInputAttributes).disabled = | |
| resendDisabled; | |
| } | |
| return node; | |
| }), | |
| }, | |
| }; | |
| }, [flow, resendDisabled]); | |
| const label = node.meta?.label; | |
| if (!label) { | |
| return node; | |
| } | |
| return { | |
| ...node, | |
| meta: { | |
| ...node.meta, | |
| label: { | |
| ...label, | |
| context: { | |
| ...label.context, | |
| beforeComponent: ( | |
| <EmailVerificationPrompt email={userEmail} /> | |
| ), | |
| }, | |
| }, | |
| }, | |
| }; | |
| } | |
| if (isResendVerificationCode(node)) { | |
| const attrs = node.attributes as UiNodeInputAttributes; | |
| return { | |
| ...node, | |
| meta: { | |
| ...node.meta, | |
| label: { | |
| ...node.meta?.label, | |
| context: { | |
| ...node.meta?.label?.context, | |
| appearance: "link", | |
| }, | |
| }, | |
| }, | |
| attributes: { | |
| ...attrs, | |
| disabled: resendDisabled, | |
| } as UiNodeInputAttributes, | |
| }; | |
| } | |
| return node; | |
| }), | |
| }, | |
| }; | |
| }, [flow, resendDisabled, userEmail]); |
| const timer = setTimeout(() => { | ||
| clearTimeout(timer); | ||
| window.location.href = "/ui/manage_details"; | ||
| }, 3000); | ||
| } |
There was a problem hiding this comment.
Timer cleanup issue: The timer created on line 106 is stored in a variable but never cleaned up if the component unmounts before the 3-second timeout completes. This could cause the redirect to execute after unmount or cause a memory leak. Consider storing the timer ID in state or a ref and clearing it in a cleanup function.
| }), | ||
| }, | ||
| }; | ||
| }, [flow, resendDisabled]); |
There was a problem hiding this comment.
The lookupFlow useMemo on line 198 depends on userEmail (used on line 215), but userEmail is not included in the dependency array on line 231. This could cause stale data to be used. Add userEmail to the dependency array.
| }, [flow, resendDisabled]); | |
| }, [flow, resendDisabled, userEmail]); |
| {shallow: true}, | ||
| ); | ||
| export function setFlowIDQueryParam(flowId: string) { | ||
| window.history.replaceState(null, "", `?flow=${flowId}`); |
There was a problem hiding this comment.
The refactored setFlowIDQueryParam function replaces the entire query string with just the flow parameter using window.history.replaceState(null, "", "?flow=${flowId}"). This will remove any other existing query parameters (like return_to or code). The original implementation preserved other query parameters by spreading router.query. This could break functionality where multiple query parameters need to coexist. Consider preserving existing query parameters using URLSearchParams or similar approach.
| window.history.replaceState(null, "", `?flow=${flowId}`); | |
| const searchParams = new URLSearchParams(window.location.search); | |
| searchParams.set("flow", flowId); | |
| const newUrl = | |
| window.location.pathname + | |
| "?" + | |
| searchParams.toString() + | |
| window.location.hash; | |
| window.history.replaceState(null, "", newUrl); |


Done:
QA:
Setup the repo according to the doc: https://docs.google.com/document/d/15PrLDpaERf7FmyHwZl4GlFLPIxaX2zF2rKENxCcKgNU/edit?tab=t.0