-
Notifications
You must be signed in to change notification settings - Fork 23
[jules] enhance: Add password strength meter to signup #277
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
[jules] enhance: Add password strength meter to signup #277
Conversation
Co-authored-by: Devasy23 <110348311+Devasy23@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for split-but-wiser canceled.
|
WalkthroughAdds a web Password Strength Meter and integrates it into the Auth signup UI; introduces a mobile haptics system (withHapticFeedback, triggerPullRefreshHaptic, and Haptic* UI wrappers) and replaces many mobile screen components to use the haptic wrappers; updates changelog and TODO entries. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
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.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@web/components/ui/PasswordStrength.tsx`:
- Around line 13-57: The password strength meter in PasswordStrength.tsx uses a
length criterion of 8 (criteria.length: password.length >= 8) which conflicts
with the form validation that allows 6-char passwords; update the length
threshold in the useMemo block (the criteria object used to compute
score/label/metCriteria and returned as score/label) to match the form
validation (change criteria.length to password.length >= 6) or, if you prefer
raising validation, update the form minimum to 8—ensure the change is applied
where criteria is defined in the useMemo and that the normalized score/label
logic (s and l) remains consistent with the new threshold.
- Line 74: The container div in PasswordStrength (the element rendered in
web/components/ui/PasswordStrength.tsx with aria-label="Password strength
indicator") uses aria-label without an explicit role, so add role="region" to
that div to ensure the aria-label is recognized by assistive tech; update the
JSX for the div that currently has className="w-full flex flex-col gap-2 mt-2"
and aria-label to include role="region" so the label becomes effective.
- Around line 121-124: Replace the visually hidden <div role="status"> live
region in the PasswordStrength component with a semantic <output> element:
locate the block that renders the sr-only live region and change it to use
<output> (keep className="sr-only" and aria-live="polite" but remove
role="status"), preserving the text content "Password strength: {label}." and
the conditional hint based on score so screen readers still receive the same
message.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@jules I have made some changes in this branch so first take a pull. And then resolve the merge conflicts that would arise from merging main into the current branch. After that, look into the below review command In |
I have merged main, resolved conflicts, and updated the password strength meter to use a 6-character minimum length to match the form validation. The criteria checklist and scoring logic have been updated accordingly. |
Co-authored-by: Devasy23 <110348311+Devasy23@users.noreply.github.com>
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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mobile/screens/SignupScreen.js (1)
15-36:⚠️ Potential issue | 🟡 MinorNo minimum password length validation on mobile signup.
The
handleSignupfunction only checks for empty fields and password match, but doesn't enforce any minimum password length. The web form enforces a 6-character minimum. Consider adding a similar check here so users don't create accounts with trivially short passwords.Proposed fix
if (password !== confirmPassword) { Alert.alert('Error', "Passwords don't match!"); return; } + if (password.length < 6) { + Alert.alert('Error', 'Password must be at least 6 characters.'); + return; + } setIsLoading(true);
🤖 Fix all issues with AI agents
In `@mobile/components/ui/hapticUtils.js`:
- Around line 13-49: There are many near-identical haptic wrapper files causing
import sprawl; create a single barrel export file (e.g., index.js) that
re-exports all the haptic wrappers so screens can import from the barrel instead
of individual files. Locate the wrapper creator withHapticFeedback in
hapticUtils and the one-line wrapper modules (e.g., HapticButton, HapticCard,
HapticFAB, etc.), then add an index that imports each wrapper by name and
re-exports them (export { default as HapticButton } from './HapticButton';
etc.), update import sites to use the barrel, and ensure the exported names
match the components’ display names or default exports so existing references
continue to work.
- Around line 19-29: The handlePress callback calls Haptics.impactAsync without
handling its Promise; update the useCallback for handlePress to await
Haptics.impactAsync inside a try/catch (or append .catch()) so rejections are
swallowed and won't cause unhandled promise crashes, keeping the originalHandler
invocation intact; reference the handlePress function and Haptics.impactAsync
and ensure the change preserves the existing behavior (trigger haptics when
appropriate and then call originalHandler(...args)).
In `@web/components/ui/PasswordStrength.tsx`:
- Line 74: In the PasswordStrength component replace the generic div that uses
role="region" with a semantic <section> element (keep the same className and
aria-label), i.e. change the element around the password strength UI in
PasswordStrength.tsx so it becomes a section with aria-label="Password strength
indicator" and remove the explicit role attribute; this keeps behavior and
styling intact while using the semantic element instead of div+role.
- Around line 25-48: In PasswordStrength.tsx the reachable-score bug comes from
always incrementing s for criteria.length so any >=6-char password becomes at
least score 2; remove the automatic length bonus (delete or disable the "if
(criteria.length) s++" increment) so length only gates scoring and only
complexity increments s (via complexity >=2 / >=4 checks), leaving s=1 =>
"Weak", s=2 => "Fair", s=3 => "Good", s>=4 => "Strong" (refer to variables s, l,
criteria.length and the complexity calculation).
Devasy
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.
LGTM
Implemented a password strength meter in the signup form.
Features:
Changes:
PR created automatically by Jules for task 12713428636481219202 started by @Devasy23
Summary by CodeRabbit