feat(jef-78): icon-only buttons on mobile - #218
Conversation
- Wrap button text in hidden sm:inline spans to hide on mobile - Add lucide-react icons to buttons that lacked them (Save, Cancel, Copy, Preview, Mark read/unread, etc.) - Add aria-labels to all updated buttons for accessibility - Update 33 buttons across 10 files (applications, notifications, settings, ErrorState) - Fix test queries to match new aria-labels
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR adds icons, explicit accessible labels, and responsive text visibility to action controls across application, notification, error, and settings pages. Related tests now use the updated accessible names. ChangesResponsive action controls
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/routes/_authenticated/settings/data.tsx (1)
122-132: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the import control labelled on small screens.
hidden sm:inlineremoves the only textual label below thesmbreakpoint. The file input also usesclassName="hidden", so assistive technology receives no labelled, focusable import control. Keep the text visually hidden withsr-only sm:not-sr-onlyand make the input visually hidden but focusable withsr-only, or provide another keyboard-focusable control with an explicit accessible name.🤖 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 `@apps/web/src/routes/_authenticated/settings/data.tsx` around lines 122 - 132, Update the import control in the onImport flow so its accessible label remains available below the sm breakpoint by changing the text span’s hidden styling to sr-only sm:not-sr-only. Replace the file input’s hidden class with sr-only so it remains visually hidden but focusable, preserving the existing importing state and file-selection behavior.
🤖 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 `@apps/web/src/routes/_authenticated/applications/`$applicationId/index.tsx:
- Around line 1988-1992: Update the copy button’s accessible feedback around the
copied state so assistive technology announces confirmation when copied is true.
Adjust the aria-label to reflect the existing copied value, or add a separate
aria-live element exposing “Copied,” while preserving the current visible label
behavior.
In `@apps/web/src/routes/_authenticated/settings/integrations.tsx`:
- Around line 211-230: Update the repeated action labels in
apps/web/src/routes/_authenticated/settings/integrations.tsx#L211-L230 to
include the row’s provider name for both Make default and Remove; update the
Revoke token label in
apps/web/src/routes/_authenticated/settings/integrations.tsx#L423-L429 to
include token.name; and update the Revoke session label in
apps/web/src/routes/_authenticated/settings/security.tsx#L557-L560 to include
the session device or user-agent.
---
Outside diff comments:
In `@apps/web/src/routes/_authenticated/settings/data.tsx`:
- Around line 122-132: Update the import control in the onImport flow so its
accessible label remains available below the sm breakpoint by changing the text
span’s hidden styling to sr-only sm:not-sr-only. Replace the file input’s hidden
class with sr-only so it remains visually hidden but focusable, preserving the
existing importing state and file-selection behavior.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 9b99ea87-8642-4ffb-9b0b-7cb6479e928a
📒 Files selected for processing (10)
apps/web/src/__tests__/components/ApplicationsPage.test.tsxapps/web/src/__tests__/components/settings/SettingsSecurityPage.test.tsxapps/web/src/components/ErrorState.tsxapps/web/src/routes/_authenticated/-notification-inbox.tsxapps/web/src/routes/_authenticated/applications/$applicationId/index.tsxapps/web/src/routes/_authenticated/applications/index.tsxapps/web/src/routes/_authenticated/settings/data.tsxapps/web/src/routes/_authenticated/settings/integrations.tsxapps/web/src/routes/_authenticated/settings/profile.tsxapps/web/src/routes/_authenticated/settings/security.tsx
| aria-label="Copy" | ||
| className="flex items-center gap-1 px-3 py-1.5 text-xs font-medium border border-gray-300 dark:border-gray-600 rounded-lg text-gray-600 dark:text-gray-400 hover:bg-gray-50 dark:hover:bg-gray-700 transition-colors" | ||
| > | ||
| {copied ? '✓ Copied' : 'Copy'} | ||
| <CopyIcon size={14} />{' '} | ||
| <span className="hidden sm:inline">{copied ? '✓ Copied' : 'Copy'}</span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expose the copy result to assistive technology.
aria-label="Copy" overrides the child text. When copied becomes true, the accessible name remains Copy, so a screen reader receives no confirmation. Make the label reflect copied or expose Copied through a separate aria-live element.
🤖 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 `@apps/web/src/routes/_authenticated/applications/`$applicationId/index.tsx
around lines 1988 - 1992, Update the copy button’s accessible feedback around
the copied state so assistive technology announces confirmation when copied is
true. Adjust the aria-label to reflect the existing copied value, or add a
separate aria-live element exposing “Copied,” while preserving the current
visible label behavior.
| aria-label="Make default" | ||
| className="flex items-center gap-1 text-xs text-blue-600 hover:underline disabled:opacity-60" | ||
| > | ||
| {settingDefaultProvider === key.provider ? 'Setting…' : 'Make default'} | ||
| <StarIcon size={14} />{' '} | ||
| <span className="hidden sm:inline"> | ||
| {settingDefaultProvider === key.provider ? 'Setting…' : 'Make default'} | ||
| </span> | ||
| </button> | ||
| )} | ||
| <button | ||
| type="button" | ||
| onClick={() => onRemoveLlmApiKey(key.provider)} | ||
| disabled={removingProvider === key.provider} | ||
| className="text-xs text-red-600 hover:underline disabled:opacity-60" | ||
| aria-label="Remove" | ||
| className="flex items-center gap-1 text-xs text-red-600 hover:underline disabled:opacity-60" | ||
| > | ||
| {removingProvider === key.provider ? 'Removing…' : 'Remove'} | ||
| <Trash2Icon size={14} />{' '} | ||
| <span className="hidden sm:inline"> | ||
| {removingProvider === key.provider ? 'Removing…' : 'Remove'} | ||
| </span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the target resource in repeated action labels.
The new aria-label values are identical across repeated rows. Because aria-label replaces the button content as the accessible name, a screen reader cannot identify the provider, token, or session that the focused button will affect. Build each label from the row identity.
apps/web/src/routes/_authenticated/settings/integrations.tsx#L211-L230: include the provider name inMake defaultandRemove.apps/web/src/routes/_authenticated/settings/integrations.tsx#L423-L429: includetoken.nameinRevoke token.apps/web/src/routes/_authenticated/settings/security.tsx#L557-L560: include the session device or user-agent inRevoke session.
📍 Affects 2 files
apps/web/src/routes/_authenticated/settings/integrations.tsx#L211-L230(this comment)apps/web/src/routes/_authenticated/settings/integrations.tsx#L423-L429apps/web/src/routes/_authenticated/settings/security.tsx#L557-L560
🤖 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 `@apps/web/src/routes/_authenticated/settings/integrations.tsx` around lines
211 - 230, Update the repeated action labels in
apps/web/src/routes/_authenticated/settings/integrations.tsx#L211-L230 to
include the row’s provider name for both Make default and Remove; update the
Revoke token label in
apps/web/src/routes/_authenticated/settings/integrations.tsx#L423-L429 to
include token.name; and update the Revoke session label in
apps/web/src/routes/_authenticated/settings/security.tsx#L557-L560 to include
the session device or user-agent.
|
Preview deployments for this PR: |
1 participant
Summary
Converts text buttons to icon-only buttons on mobile (< sm) to save horizontal space. On desktop (>= sm), the current text + icon layout is preserved via hidden sm:inline on the text span.
33 buttons updated across 10 files with aria-label for accessibility.
Changes
Bulk action bar (applications/index.tsx)
Application detail page (applicationId/index.tsx)
Notification inbox (-notification-inbox.tsx)
Settings pages (profile, security, data, integrations)
ErrorState component
Pattern
For buttons that already have icons, wrap text in hidden span and add aria-label:
Add noteFor buttons without icons, add icon + hidden span + aria-label:
SaveTests
Summary by CodeRabbit
Accessibility
Style
Tests