feat: add password visibility toggle to login page MXWAR-48#70
Open
vaishnavirrapolu-06 wants to merge 7 commits intoopenMF:devfrom
Open
feat: add password visibility toggle to login page MXWAR-48#70vaishnavirrapolu-06 wants to merge 7 commits intoopenMF:devfrom
vaishnavirrapolu-06 wants to merge 7 commits intoopenMF:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughLogin UI input and accessibility were adjusted; Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/login/Login.tsx`:
- Around line 202-212: The icon-only password toggle button lacks an accessible
name and state; update the button (the element using onClick={() =>
setShowPassword(!showPassword)}) to include an appropriate accessible label and
state—e.g., add aria-label that reflects the action ("Show password" / "Hide
password") and aria-pressed or aria-expanded tied to the showPassword boolean so
screen readers announce current state; ensure the label text changes based on
showPassword and keep the visual icon components Eye and EyeOff unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 17: The package.json currently lists platform-specific binary packages
(`@esbuild/win32-x64` and `@rollup/rollup-win32-x64-msvc`) as direct dependencies;
remove these entries or move them into optionalDependencies so they are not
treated as mandatory orphaned binaries — update package.json to delete the
"@esbuild/win32-x64" and "@rollup/rollup-win32-x64-msvc" entries from
"dependencies" (or add them under "optionalDependencies") and ensure the parent
packages (esbuild and rollup) remain as the canonical dependencies.
In `@src/pages/login/Login.tsx`:
- Around line 197-203: The file contains unresolved Git merge conflict markers
(<<<<<<<, =======, >>>>>>>) that break TSX parsing; open the Login component
(src/pages/login/Login.tsx) and remove all conflict markers at the reported
locations and choose the correct code variants (e.g., use
t('auth:login.password') for the password placeholder and keep a single
consistent string quote style if desired), ensuring elements like the password
input (type={showPassword ? 'text' : 'password'}
placeholder={t('auth:login.password')}), any surrounding JSX in the Login
component, and related handlers/rendered blocks are syntactically valid TSX
after cleanup; run a typecheck/build to confirm all markers are gone.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env.samplepackage.jsonsrc/pages/login/Login.tsx
💤 Files with no reviewable changes (1)
- .env.sample
| "generate-sdk": "npx openapi-generator-cli generate -i fineract.yaml -g typescript-axios -o src/fineract-api --additional-properties=supportsES6=true" | ||
| }, | ||
| "dependencies": { | ||
| "@esbuild/win32-x64": "^0.27.3", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat package.jsonRepository: openMF/mifos-x-web-app-react
Length of output: 2984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for pkg in "@esbuild/win32-x64" "@rollup/rollup-win32-x64-msvc"; do
encoded="${pkg/@/%40}"
encoded="${encoded/\//%2f}"
echo "=== $pkg ==="
curl -s "https://registry.npmjs.org/${encoded}" | jq -r '
.["dist-tags"].latest as $v |
"Package: " + . as $name |
"Latest version: " + $v +
"\nos: " + ((.versions[$v].os // ["<none>"] | join(","))) +
"\ncpu: " + ((.versions[$v].cpu // ["<none>"] | join(",")))' 2>/dev/null || echo "Failed to fetch"
echo ""
doneRepository: openMF/mifos-x-web-app-react
Length of output: 240
Remove Windows-specific binaries from dependencies.
@esbuild/win32-x64 and @rollup/rollup-win32-x64-msvc are declared with os: win32, cpu: x64 constraints. These should not be listed as direct dependencies without their parent packages (esbuild, rollup). While modern npm gracefully skips platform-mismatched packages, having orphaned platform binaries here is unconventional and fragile. Either move to optionalDependencies or remove—these binary variants are typically managed automatically by their parent packages or toolchain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 17, The package.json currently lists platform-specific
binary packages (`@esbuild/win32-x64` and `@rollup/rollup-win32-x64-msvc`) as direct
dependencies; remove these entries or move them into optionalDependencies so
they are not treated as mandatory orphaned binaries — update package.json to
delete the "@esbuild/win32-x64" and "@rollup/rollup-win32-x64-msvc" entries from
"dependencies" (or add them under "optionalDependencies") and ensure the parent
packages (esbuild and rollup) remain as the canonical dependencies.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/login/Login.tsx (1)
279-307:⚠️ Potential issue | 🟠 MajorAvoid hard-coded menu labels; keep translation keys.
Lines 279-307 replaced localized labels with fixed English strings. This is a localization regression in the login page navigation menus.
Proposed fix
- <a href="https://groups.google.com/g/mifosusers">User Group</a> + <a href="https://groups.google.com/g/mifosusers">{t('common:nav.userGroup')}</a> @@ - <a href="https://groups.google.com/g/mifosdeveloper">Developer Group</a> + <a href="https://groups.google.com/g/mifosdeveloper">{t('common:nav.developerGroup')}</a> @@ - <a href="https://mifos.org/resources/community/communications/#mifos-irc">IRC</a> + <a href="https://mifos.org/resources/community/communications/#mifos-irc">{t('common:nav.irc')}</a> @@ - <a href="https://mifosforge.jira.com/wiki/spaces/MDZ/pages/92012624/Key+Design+Principles">Key Design Principles</a> + <a href="https://mifosforge.jira.com/wiki/spaces/MDZ/pages/92012624/Key+Design+Principles"> + {t('common:nav.keyDesignPrinciples')} + </a> @@ - <a href="https://sourceforge.net/projects/mifos/">Working with code</a> + <a href="https://sourceforge.net/projects/mifos/">{t('common:nav.workingWithCode')}</a> @@ - <a href="https://mifos.org/take-action/donate-now/">Donate</a> + <a href="https://mifos.org/take-action/donate-now/">{t('common:nav.donate')}</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/login/Login.tsx` around lines 279 - 307, The dropdown menu items in Login.tsx (components DropdownMenu, DropdownMenuItem, DropdownMenuTrigger/Button) use hard-coded English labels ("User Group", "Developer Group", "IRC", "Key Design Principles", "Working with code", "Donate"); replace those literal strings with calls to the i18n translator (t('...')) using appropriate translation keys (e.g., common:nav.userGroup, common:nav.developerGroup, common:nav.irc, common:nav.keyDesignPrinciples, common:nav.workingWithCode, common:nav.donate) so the links show translated text while keeping the same anchors and className attributes. Ensure the t function is in scope (import/use react-i18next hook if needed) and add the new keys to the translation files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/locales/es-ES/common.json`:
- Line 150: The value for the locale key "MU" is currently English
("Mauritius"); update the Spanish localization by replacing that value with the
proper Spanish name (e.g., "Mauricio" or "Isla Mauricio") so it matches the rest
of the es-ES/common.json locale style and remains consistent with other country
translations.
In `@src/pages/login/Login.tsx`:
- Around line 198-208: Replace the hard-coded English strings in the Login
component with i18n keys: use the translation function (e.g., t from
useTranslation) for the password input placeholder and the Button aria-labels so
they read something like t('login.passwordPlaceholder') and
aria-label={showPassword ? t('login.hidePassword') : t('login.showPassword')};
update any missing import/use of useTranslation in Login.tsx and keep
aria-pressed={showPassword} and the onClick handler (setShowPassword) as-is so
behavior is unchanged.
---
Outside diff comments:
In `@src/pages/login/Login.tsx`:
- Around line 279-307: The dropdown menu items in Login.tsx (components
DropdownMenu, DropdownMenuItem, DropdownMenuTrigger/Button) use hard-coded
English labels ("User Group", "Developer Group", "IRC", "Key Design Principles",
"Working with code", "Donate"); replace those literal strings with calls to the
i18n translator (t('...')) using appropriate translation keys (e.g.,
common:nav.userGroup, common:nav.developerGroup, common:nav.irc,
common:nav.keyDesignPrinciples, common:nav.workingWithCode, common:nav.donate)
so the links show translated text while keeping the same anchors and className
attributes. Ensure the t function is in scope (import/use react-i18next hook if
needed) and add the new keys to the translation files.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/locales/en-US/common.jsonsrc/locales/es-ES/common.jsonsrc/locales/fr-FR/common.jsonsrc/locales/it-IT/common.jsonsrc/pages/login/Login.tsx
src/locales/es-ES/common.json
Outdated
| "MR": "Mauritania", | ||
| "MT": "Malta", | ||
| "MU": "Mauricio", | ||
| "MU": "Mauritius", |
There was a problem hiding this comment.
Spanish localization regression for country code MU.
On Line 150, "Mauritius" is not Spanish-localized and is inconsistent with the locale style in this file. Consider restoring the Spanish form.
Suggested fix
- "MU": "Mauritius",
+ "MU": "Mauricio",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "MU": "Mauritius", | |
| "MU": "Mauricio", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/locales/es-ES/common.json` at line 150, The value for the locale key "MU"
is currently English ("Mauritius"); update the Spanish localization by replacing
that value with the proper Spanish name (e.g., "Mauricio" or "Isla Mauricio") so
it matches the rest of the es-ES/common.json locale style and remains consistent
with other country translations.
Comment on lines
+198
to
+208
| placeholder="Password" | ||
| className="dark:bg-zinc-800 dark:text-white pr-10" | ||
| /> | ||
| <Button | ||
| type="button" | ||
| variant="ghost" | ||
| size="icon" | ||
| className="absolute right-0 top-0 h-full px-3 hover:bg-transparent" | ||
| onClick={() => setShowPassword(!showPassword)} | ||
| onMouseDown={(e) => e.preventDefault()} | ||
| aria-label={showPassword ? t('auth:login.hidePassword') : t('auth:login.showPassword')} | ||
| // CodeRabbit Fix: Adding accessibility labels | ||
| aria-label={showPassword ? "Hide password" : "Show password"} | ||
| aria-pressed={showPassword} |
There was a problem hiding this comment.
Restore i18n for password placeholder and toggle labels.
Line 198 and Lines 207-208 reintroduce hard-coded English text in a translated screen. This regresses localization for non-English users.
Proposed fix
- type={showPassword ? "text" : "password"}
- placeholder="Password"
+ type={showPassword ? 'text' : 'password'}
+ placeholder={t('auth:login.password')}
@@
- onClick={() => setShowPassword(!showPassword)}
+ onClick={() => setShowPassword((prev) => !prev)}
@@
- aria-label={showPassword ? "Hide password" : "Show password"}
+ aria-label={showPassword ? t('auth:login.hidePassword') : t('auth:login.showPassword')}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/login/Login.tsx` around lines 198 - 208, Replace the hard-coded
English strings in the Login component with i18n keys: use the translation
function (e.g., t from useTranslation) for the password input placeholder and
the Button aria-labels so they read something like
t('login.passwordPlaceholder') and aria-label={showPassword ?
t('login.hidePassword') : t('login.showPassword')}; update any missing
import/use of useTranslation in Login.tsx and keep aria-pressed={showPassword}
and the onClick handler (setShowPassword) as-is so behavior is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds a functional show/hide toggle to the password input on the login page, improving user accessibility and experience.
Changes made:
Fixes: https://www.google.com/search?q=https://mifosforge.jira.com/browse/MXWAR-48
Summary by CodeRabbit
Style
Accessibility
Localization
Chores