fix(a11y): label Password and Profile Picture form landmarks on settings/general#14030
fix(a11y): label Password and Profile Picture form landmarks on settings/general#14030olayinkaadelakun wants to merge 1 commit into
Conversation
WalkthroughThe General settings forms now expose translated accessible names. New accessibility tests cover GeneralPage structure, language selection interactions, form landmarks, axe violations, and auto-login conditional rendering. ChangesSettings accessibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (6 passed)
✨ 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 |
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/pages/SettingsPage/pages/GeneralPage/components/ProfilePictureForm/index.tsx (1)
24-24: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the unused
handleGetProfilePicturesprop fromProfilePictureFormComponentPropsand theGeneralPagecall site. The form fetches profile pictures directly now, so this prop is dead plumbing.src/frontend/src/pages/SettingsPage/pages/GeneralPage/index.tsx:152🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/SettingsPage/pages/GeneralPage/components/ProfilePictureForm/index.tsx` at line 24, Remove the unused handleGetProfilePictures field from ProfilePictureFormComponentProps and stop passing it from the GeneralPage call site. Keep ProfilePictureForm’s direct profile-picture fetching unchanged.
🧹 Nitpick comments (1)
src/frontend/src/pages/SettingsPage/pages/GeneralPage/__tests__/GeneralPage.a11y.test.tsx (1)
15-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider Playwright for integration-level accessibility tests.
Per coding guidelines, frontend tests should use Playwright where appropriate and follow the project's Playwright testing patterns. The Radix Form mock (lines 15-30) replaces form primitives with plain HTML elements, and 7 total mocks are needed to render the page. A Playwright integration test would exercise real Radix behavior, real portal rendering, and real keyboard/focus interactions without these mocks, providing higher-fidelity accessibility coverage — especially for the popover and keyboard tests.
As per coding guidelines: "use Playwright where appropriate" and "Frontend tests should follow the project's Playwright testing patterns and verify behavior rather than acting as smoke tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/SettingsPage/pages/GeneralPage/__tests__/GeneralPage.a11y.test.tsx` around lines 15 - 30, Replace the mocked Radix Form-based accessibility tests in GeneralPage.a11y.test.tsx with a Playwright integration test following the project’s existing Playwright patterns. Exercise the real GeneralPage components, including popover rendering and keyboard/focus interactions, and verify accessibility behavior without the seven component mocks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/frontend/src/pages/SettingsPage/pages/GeneralPage/__tests__/GeneralPage.a11y.test.tsx`:
- Around line 182-189: Update the autoLogin test in the
“does_not_render_the_password_form_when_autologin_is_enabled” case to query the
form landmark by its translated accessible name, using queryByRole with name
“Password,” instead of querying the untranslated placeholder key. Keep the
assertion that the password form is not rendered.
---
Outside diff comments:
In
`@src/frontend/src/pages/SettingsPage/pages/GeneralPage/components/ProfilePictureForm/index.tsx`:
- Line 24: Remove the unused handleGetProfilePictures field from
ProfilePictureFormComponentProps and stop passing it from the GeneralPage call
site. Keep ProfilePictureForm’s direct profile-picture fetching unchanged.
---
Nitpick comments:
In
`@src/frontend/src/pages/SettingsPage/pages/GeneralPage/__tests__/GeneralPage.a11y.test.tsx`:
- Around line 15-30: Replace the mocked Radix Form-based accessibility tests in
GeneralPage.a11y.test.tsx with a Playwright integration test following the
project’s existing Playwright patterns. Exercise the real GeneralPage
components, including popover rendering and keyboard/focus interactions, and
verify accessibility behavior without the seven component mocks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d068d593-fe1f-4aba-ab86-37da488c2413
📒 Files selected for processing (4)
src/frontend/src/pages/SettingsPage/pages/GeneralPage/__tests__/GeneralPage.a11y.test.tsxsrc/frontend/src/pages/SettingsPage/pages/GeneralPage/components/LanguageForm/__tests__/LanguageForm.a11y.test.tsxsrc/frontend/src/pages/SettingsPage/pages/GeneralPage/components/PasswordForm/index.tsxsrc/frontend/src/pages/SettingsPage/pages/GeneralPage/components/ProfilePictureForm/index.tsx
| it("does_not_render_the_password_form_when_autologin_is_enabled", () => { | ||
| useAuthStore.setState({ autoLogin: true }); | ||
| renderGeneralPage(); | ||
|
|
||
| expect( | ||
| screen.queryByPlaceholderText("settings.passwordPlaceholder"), | ||
| ).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The autoLogin test is a false positive — queryByPlaceholderText uses the i18n key, not the translated value.
queryByPlaceholderText("settings.passwordPlaceholder") queries for the literal string "settings.passwordPlaceholder", but the component renders t("settings.passwordPlaceholder") which resolves to the translated value (e.g., "Password"). Other tests in this file confirm i18n returns translated values — line 161 expects name: "Password", not name: "settings.passwordTitle". Since the query never matches the rendered placeholder, queryByPlaceholderText returns null and the assertion passes regardless of whether the form is rendered.
Use queryByRole("form", { name: "Password" }) instead, which is consistent with line 161 and actually verifies the form landmark's presence.
🐛 Proposed fix
it("does_not_render_the_password_form_when_autologin_is_enabled", () => {
useAuthStore.setState({ autoLogin: true });
renderGeneralPage();
expect(
- screen.queryByPlaceholderText("settings.passwordPlaceholder"),
+ screen.queryByRole("form", { name: "Password" }),
).not.toBeInTheDocument();
});📝 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.
| it("does_not_render_the_password_form_when_autologin_is_enabled", () => { | |
| useAuthStore.setState({ autoLogin: true }); | |
| renderGeneralPage(); | |
| expect( | |
| screen.queryByPlaceholderText("settings.passwordPlaceholder"), | |
| ).not.toBeInTheDocument(); | |
| }); | |
| it("does_not_render_the_password_form_when_autologin_is_enabled", () => { | |
| useAuthStore.setState({ autoLogin: true }); | |
| renderGeneralPage(); | |
| expect( | |
| screen.queryByRole("form", { name: "Password" }), | |
| ).not.toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/src/pages/SettingsPage/pages/GeneralPage/__tests__/GeneralPage.a11y.test.tsx`
around lines 182 - 189, Update the autoLogin test in the
“does_not_render_the_password_form_when_autologin_is_enabled” case to query the
form landmark by its translated accessible name, using queryByRole with name
“Password,” instead of querying the untranslated placeholder key. Keep the
assertion that the password form is not rendered.
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release-1.11.0 #14030 +/- ##
==================================================
- Coverage 59.99% 59.80% -0.19%
==================================================
Files 2389 2337 -52
Lines 230242 229356 -886
Branches 34463 35162 +699
==================================================
- Hits 138125 137158 -967
- Misses 90501 90582 +81
Partials 1616 1616
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
2 participants
This PR is currently pointed to Release-1.11.0, but would be moved to release-1.12.0 when created
Summary
Test plan
Screenshot
Summary by CodeRabbit
Accessibility
Tests