fix(platform): fix password visibility toggle and add forgot-password hints#1300
Merged
Conversation
… hints (#1280) Override browser autofill masking (WebkitTextSecurity) so toggling the password eye icon actually reveals the text in Chrome/Safari. Add a tooltip on the toggle button and forgot-password descriptions on the login and member-add forms.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR addresses a password visibility issue in the Input component where the password remains masked even when the show-password toggle is activated. The changes add Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/components/ui/forms/input.test.tsx`:
- Around line 97-109: The failing test "preserves caller style prop when
toggling password" asserts the color as { color: 'red' } but jsdom normalizes
colors to RGB; update the assertion in the test (around the Input component
usage and the getByLabelText('Password')/toggle interactions) to expect the
normalized value, e.g. use 'rgb(255, 0, 0)' (via toHaveStyle or
style.getPropertyValue) instead of 'red' so the color check matches jsdom's
output.
- Around line 243-257: The failing test "toggles visibility on pre-filled field"
is reading the CSS property via
input.style.getPropertyValue('-webkit-text-security'), which doesn't work under
jsdom; update the assertion to use the DOM-friendly property access
input.style.webkitTextSecurity (same change as the earlier test) so the test
checks input.style.webkitTextSecurity === 'none' after clicking the toggle; keep
the other assertions (type and value) unchanged.
- Around line 111-117: The test "does not apply webkit text-security to
non-password inputs" is asserting the wrong value; for non-password inputs the
-webkit-text-security style should be absent (empty string) rather than not
equal to 'none'. Update the assertion in the test (the it block that renders
<Input type="text" label="Name" /> and reads const input =
screen.getByLabelText('Name')) to expect
input.style.getPropertyValue('-webkit-text-security') toBe('') (or toBeFalsy())
instead of using not.toBe('none').
- Around line 77-95: The failing test 'clears webkit text-security masking when
password is visible' uses input.style.getPropertyValue('-webkit-text-security')
which jsdom doesn't support; update the test to assert the style via the DOM
style object or inline style string instead: replace uses of
input.style.getPropertyValue('-webkit-text-security') with either
input.style.WebkitTextSecurity (checking it equals 'none' or not) or check
input.getAttribute('style') includes 'WebkitTextSecurity: none'; alternatively
you can assert visibility by checking the Input component's underlying type
toggles (e.g., input.type === 'text' when visible) to avoid relying on
vendor-prefixed properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3cacaa06-e770-48f3-ad88-93d876a2dc73
📒 Files selected for processing (8)
services/platform/app/components/ui/forms/input.stories.tsxservices/platform/app/components/ui/forms/input.test.tsxservices/platform/app/components/ui/forms/input.tsxservices/platform/app/features/settings/organization/components/member-add-dialog.tsxservices/platform/app/globals.cssservices/platform/app/routes/_auth/log-in.tsxservices/platform/messages/de.jsonservices/platform/messages/en.json
Comment on lines
+77
to
+95
| it('clears webkit text-security masking when password is visible', async () => { | ||
| const { user } = render(<Input type="password" label="Password" />); | ||
| const input = screen.getByLabelText('Password'); | ||
| const toggle = screen.getByRole('button', { name: /show password/i }); | ||
|
|
||
| expect(input.style.getPropertyValue('-webkit-text-security')).not.toBe( | ||
| 'none', | ||
| ); | ||
|
|
||
| await user.click(toggle); | ||
| expect(input.style.getPropertyValue('-webkit-text-security')).toBe( | ||
| 'none', | ||
| ); | ||
|
|
||
| await user.click(toggle); | ||
| expect(input.style.getPropertyValue('-webkit-text-security')).not.toBe( | ||
| 'none', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Fix failing test: jsdom does not support -webkit-text-security property.
The test expects input.style.getPropertyValue('-webkit-text-security') to return 'none', but jsdom doesn't support this vendor-prefixed property. The inline style sets WebkitTextSecurity: 'none' (camelCase), but jsdom returns an empty string when querying the hyphenated form.
Consider testing the style object property directly or checking for the presence of the inline style attribute:
🛠️ Proposed fix
it('clears webkit text-security masking when password is visible', async () => {
const { user } = render(<Input type="password" label="Password" />);
- const input = screen.getByLabelText('Password');
+ const input = screen.getByLabelText('Password') as HTMLInputElement;
const toggle = screen.getByRole('button', { name: /show password/i });
- expect(input.style.getPropertyValue('-webkit-text-security')).not.toBe(
- 'none',
- );
+ // Before toggle: WebkitTextSecurity should not be set
+ expect(input.style.webkitTextSecurity).toBeFalsy();
await user.click(toggle);
- expect(input.style.getPropertyValue('-webkit-text-security')).toBe(
- 'none',
- );
+ // After toggle: WebkitTextSecurity should be 'none'
+ expect(input.style.webkitTextSecurity).toBe('none');
await user.click(toggle);
- expect(input.style.getPropertyValue('-webkit-text-security')).not.toBe(
- 'none',
- );
+ // After toggle back: WebkitTextSecurity should not be 'none'
+ expect(input.style.webkitTextSecurity).not.toBe('none');
});📝 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
| it('clears webkit text-security masking when password is visible', async () => { | |
| const { user } = render(<Input type="password" label="Password" />); | |
| const input = screen.getByLabelText('Password'); | |
| const toggle = screen.getByRole('button', { name: /show password/i }); | |
| expect(input.style.getPropertyValue('-webkit-text-security')).not.toBe( | |
| 'none', | |
| ); | |
| await user.click(toggle); | |
| expect(input.style.getPropertyValue('-webkit-text-security')).toBe( | |
| 'none', | |
| ); | |
| await user.click(toggle); | |
| expect(input.style.getPropertyValue('-webkit-text-security')).not.toBe( | |
| 'none', | |
| ); | |
| }); | |
| it('clears webkit text-security masking when password is visible', async () => { | |
| const { user } = render(<Input type="password" label="Password" />); | |
| const input = screen.getByLabelText('Password') as HTMLInputElement; | |
| const toggle = screen.getByRole('button', { name: /show password/i }); | |
| // Before toggle: WebkitTextSecurity should not be set | |
| expect(input.style.webkitTextSecurity).toBeFalsy(); | |
| await user.click(toggle); | |
| // After toggle: WebkitTextSecurity should be 'none' | |
| expect(input.style.webkitTextSecurity).toBe('none'); | |
| await user.click(toggle); | |
| // After toggle back: WebkitTextSecurity should not be 'none' | |
| expect(input.style.webkitTextSecurity).not.toBe('none'); | |
| }); |
🧰 Tools
🪛 GitHub Check: Test UI
[failure] 87-87: app/components/ui/forms/input.test.tsx > Input > password input > clears webkit text-security masking when password is visible
AssertionError: expected '' to be 'none' // Object.is equality
- Expected
- Received
- none
❯ app/components/ui/forms/input.test.tsx:87:69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/input.test.tsx` around lines 77 -
95, The failing test 'clears webkit text-security masking when password is
visible' uses input.style.getPropertyValue('-webkit-text-security') which jsdom
doesn't support; update the test to assert the style via the DOM style object or
inline style string instead: replace uses of
input.style.getPropertyValue('-webkit-text-security') with either
input.style.WebkitTextSecurity (checking it equals 'none' or not) or check
input.getAttribute('style') includes 'WebkitTextSecurity: none'; alternatively
you can assert visibility by checking the Input component's underlying type
toggles (e.g., input.type === 'text' when visible) to avoid relying on
vendor-prefixed properties.
Comment on lines
+97
to
+109
| it('preserves caller style prop when toggling password', async () => { | ||
| const { user } = render( | ||
| <Input type="password" label="Password" style={{ color: 'red' }} />, | ||
| ); | ||
| const input = screen.getByLabelText('Password'); | ||
| const toggle = screen.getByRole('button', { name: /show password/i }); | ||
|
|
||
| await user.click(toggle); | ||
| expect(input).toHaveStyle({ color: 'red' }); | ||
| expect(input.style.getPropertyValue('-webkit-text-security')).toBe( | ||
| 'none', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Fix failing test: color format mismatch.
The test asserts { color: 'red' } but jsdom normalizes CSS colors to RGB format. Use rgb(255, 0, 0) for the assertion.
🛠️ Proposed fix
it('preserves caller style prop when toggling password', async () => {
const { user } = render(
<Input type="password" label="Password" style={{ color: 'red' }} />,
);
- const input = screen.getByLabelText('Password');
+ const input = screen.getByLabelText('Password') as HTMLInputElement;
const toggle = screen.getByRole('button', { name: /show password/i });
await user.click(toggle);
- expect(input).toHaveStyle({ color: 'red' });
- expect(input.style.getPropertyValue('-webkit-text-security')).toBe(
- 'none',
- );
+ expect(input).toHaveStyle({ color: 'rgb(255, 0, 0)' });
+ expect(input.style.webkitTextSecurity).toBe('none');
});📝 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
| it('preserves caller style prop when toggling password', async () => { | |
| const { user } = render( | |
| <Input type="password" label="Password" style={{ color: 'red' }} />, | |
| ); | |
| const input = screen.getByLabelText('Password'); | |
| const toggle = screen.getByRole('button', { name: /show password/i }); | |
| await user.click(toggle); | |
| expect(input).toHaveStyle({ color: 'red' }); | |
| expect(input.style.getPropertyValue('-webkit-text-security')).toBe( | |
| 'none', | |
| ); | |
| }); | |
| it('preserves caller style prop when toggling password', async () => { | |
| const { user } = render( | |
| <Input type="password" label="Password" style={{ color: 'red' }} />, | |
| ); | |
| const input = screen.getByLabelText('Password') as HTMLInputElement; | |
| const toggle = screen.getByRole('button', { name: /show password/i }); | |
| await user.click(toggle); | |
| expect(input).toHaveStyle({ color: 'rgb(255, 0, 0)' }); | |
| expect(input.style.webkitTextSecurity).toBe('none'); | |
| }); |
🧰 Tools
🪛 GitHub Check: Test UI
[failure] 105-105: app/components/ui/forms/input.test.tsx > Input > password input > preserves caller style prop when toggling password
Error: expect(element).toHaveStyle()
-
Expected
-
color: red;
- color: rgb(255, 0, 0);
❯ app/components/ui/forms/input.test.tsx:105:21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/input.test.tsx` around lines 97 -
109, The failing test "preserves caller style prop when toggling password"
asserts the color as { color: 'red' } but jsdom normalizes colors to RGB; update
the assertion in the test (around the Input component usage and the
getByLabelText('Password')/toggle interactions) to expect the normalized value,
e.g. use 'rgb(255, 0, 0)' (via toHaveStyle or style.getPropertyValue) instead of
'red' so the color check matches jsdom's output.
Comment on lines
+111
to
+117
| it('does not apply webkit text-security to non-password inputs', () => { | ||
| render(<Input type="text" label="Name" />); | ||
| const input = screen.getByLabelText('Name'); | ||
| expect(input.style.getPropertyValue('-webkit-text-security')).not.toBe( | ||
| 'none', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Same fix needed for non-password input test.
🛠️ Proposed fix
it('does not apply webkit text-security to non-password inputs', () => {
render(<Input type="text" label="Name" />);
- const input = screen.getByLabelText('Name');
- expect(input.style.getPropertyValue('-webkit-text-security')).not.toBe(
- 'none',
- );
+ const input = screen.getByLabelText('Name') as HTMLInputElement;
+ expect(input.style.webkitTextSecurity).not.toBe('none');
});📝 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
| it('does not apply webkit text-security to non-password inputs', () => { | |
| render(<Input type="text" label="Name" />); | |
| const input = screen.getByLabelText('Name'); | |
| expect(input.style.getPropertyValue('-webkit-text-security')).not.toBe( | |
| 'none', | |
| ); | |
| }); | |
| it('does not apply webkit text-security to non-password inputs', () => { | |
| render(<Input type="text" label="Name" />); | |
| const input = screen.getByLabelText('Name') as HTMLInputElement; | |
| expect(input.style.webkitTextSecurity).not.toBe('none'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/input.test.tsx` around lines 111 -
117, The test "does not apply webkit text-security to non-password inputs" is
asserting the wrong value; for non-password inputs the -webkit-text-security
style should be absent (empty string) rather than not equal to 'none'. Update
the assertion in the test (the it block that renders <Input type="text"
label="Name" /> and reads const input = screen.getByLabelText('Name')) to expect
input.style.getPropertyValue('-webkit-text-security') toBe('') (or toBeFalsy())
instead of using not.toBe('none').
Comment on lines
+243
to
+257
| it('toggles visibility on pre-filled field', async () => { | ||
| const { user } = render(<PasswordForm />); | ||
| const input = screen.getByLabelText('Password'); | ||
| const toggle = screen.getByRole('button', { name: /show password/i }); | ||
|
|
||
| expect(input).toHaveAttribute('type', 'password'); | ||
| expect(input).toHaveValue('secret123'); | ||
|
|
||
| await user.click(toggle); | ||
| expect(input).toHaveAttribute('type', 'text'); | ||
| expect(input).toHaveValue('secret123'); | ||
| expect(input.style.getPropertyValue('-webkit-text-security')).toBe( | ||
| 'none', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Fix failing test: same jsdom issue with -webkit-text-security.
This test has the same issue as line 87 - use input.style.webkitTextSecurity instead.
🛠️ Proposed fix
it('toggles visibility on pre-filled field', async () => {
const { user } = render(<PasswordForm />);
- const input = screen.getByLabelText('Password');
+ const input = screen.getByLabelText('Password') as HTMLInputElement;
const toggle = screen.getByRole('button', { name: /show password/i });
expect(input).toHaveAttribute('type', 'password');
expect(input).toHaveValue('secret123');
await user.click(toggle);
expect(input).toHaveAttribute('type', 'text');
expect(input).toHaveValue('secret123');
- expect(input.style.getPropertyValue('-webkit-text-security')).toBe(
- 'none',
- );
+ expect(input.style.webkitTextSecurity).toBe('none');
});📝 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
| it('toggles visibility on pre-filled field', async () => { | |
| const { user } = render(<PasswordForm />); | |
| const input = screen.getByLabelText('Password'); | |
| const toggle = screen.getByRole('button', { name: /show password/i }); | |
| expect(input).toHaveAttribute('type', 'password'); | |
| expect(input).toHaveValue('secret123'); | |
| await user.click(toggle); | |
| expect(input).toHaveAttribute('type', 'text'); | |
| expect(input).toHaveValue('secret123'); | |
| expect(input.style.getPropertyValue('-webkit-text-security')).toBe( | |
| 'none', | |
| ); | |
| }); | |
| it('toggles visibility on pre-filled field', async () => { | |
| const { user } = render(<PasswordForm />); | |
| const input = screen.getByLabelText('Password') as HTMLInputElement; | |
| const toggle = screen.getByRole('button', { name: /show password/i }); | |
| expect(input).toHaveAttribute('type', 'password'); | |
| expect(input).toHaveValue('secret123'); | |
| await user.click(toggle); | |
| expect(input).toHaveAttribute('type', 'text'); | |
| expect(input).toHaveValue('secret123'); | |
| expect(input.style.webkitTextSecurity).toBe('none'); | |
| }); |
🧰 Tools
🪛 GitHub Check: Test UI
[failure] 254-254: app/components/ui/forms/input.test.tsx > Input > react hook form integration > toggles visibility on pre-filled field
AssertionError: expected '' to be 'none' // Object.is equality
- Expected
- Received
- none
❯ app/components/ui/forms/input.test.tsx:254:69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/input.test.tsx` around lines 243 -
257, The failing test "toggles visibility on pre-filled field" is reading the
CSS property via input.style.getPropertyValue('-webkit-text-security'), which
doesn't work under jsdom; update the assertion to use the DOM-friendly property
access input.style.webkitTextSecurity (same change as the earlier test) so the
test checks input.style.webkitTextSecurity === 'none' after clicking the toggle;
keep the other assertions (type and value) unchanged.
…lures Move react and react-dom from platform dependencies to the root package.json so the monorepo has a single copy. This fixes the "Cannot read properties of null" dispatcher error in UI tests caused by two separate React instances being loaded.
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.
Summary
-webkit-text-security) via inline style and a defensive CSS ruleCloses #1280
Test plan
Summary by CodeRabbit
New Features
Style
Tests
Chores