fix(admin): resolve uninformative skeleton loading state for screen readers in AppsUsageCard#39952
Conversation
…eaders in AppsUsageCard
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThe pull request addresses an accessibility issue in the AppsUsageCard component by adding ARIA attributes ( Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Pull request overview
Adds accessible loading semantics to the AppsUsageCard skeleton fallback so screen readers can perceive the loading state, and removes the stale A11y FIXME.
Changes:
- Removes the
// FIXME: not accessible enoughmarker fromAppsUsageCardloading branch - Adds
aria-busyand a localizedaria-labelto the loading-state wrapper around the skeleton
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <FeatureUsageCardBody justifyContent='flex-start' aria-busy={true} aria-label={t('Loading')}> | ||
| <Skeleton variant='rect' width='x112' height='x224' role='presentation' /> | ||
| </FeatureUsageCardBody> |
There was a problem hiding this comment.
FeatureUsageCardBody currently only accepts { justifyContent, children } (see apps/meteor/client/views/admin/subscription/components/FeatureUsageCardBody.tsx:4-12) and does not forward arbitrary props. Passing aria-busy / aria-label here will cause a TypeScript props error and the attributes won’t reach the DOM. Consider extending FeatureUsageCardBodyProps to include standard DOM/Box props (or a dedicated aria* subset) and spreading them onto the underlying Box/CardBody so these ARIA attributes are actually applied.
| <FeatureUsageCardBody justifyContent='flex-start' aria-busy={true} aria-label={t('Loading')}> | |
| <Skeleton variant='rect' width='x112' height='x224' role='presentation' /> | |
| </FeatureUsageCardBody> | |
| <div aria-busy={true} aria-label={t('Loading')}> | |
| <FeatureUsageCardBody justifyContent='flex-start'> | |
| <Skeleton variant='rect' width='x112' height='x224' role='presentation' /> | |
| </FeatureUsageCardBody> | |
| </div> |
| return ( | ||
| <FeatureUsageCard card={{ title: t('Apps') }}> | ||
| <FeatureUsageCardBody justifyContent='flex-start'> | ||
| <FeatureUsageCardBody justifyContent='flex-start' aria-busy={true} aria-label={t('Loading')}> |
There was a problem hiding this comment.
Using aria-busy plus aria-label on the container may still not get announced as a loading transition, because aria-busy does not itself create a live region and aria-label typically won’t be read unless the element is navigated to. Elsewhere in the client, loading regions pair aria-busy with aria-live/aria-atomic (e.g. navbar/NavBarSearch/NavBarSearchListbox.tsx:52-55, views/root/PageLoading.tsx:4). Consider adding an appropriate live region (aria-live='polite' + aria-atomic='true', or role='status') and/or exposing a short “Loading” text node (can be visually hidden) so screen readers reliably announce the state change.
| <FeatureUsageCardBody justifyContent='flex-start' aria-busy={true} aria-label={t('Loading')}> | |
| <FeatureUsageCardBody justifyContent='flex-start' aria-busy={true} aria-live='polite' aria-atomic='true'> | |
| {t('Loading')} |
![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
`@apps/meteor/client/views/admin/subscription/components/cards/AppsUsageCard/AppsUsageCard.tsx`:
- Around line 29-31: The ARIA attributes passed into AppsUsageCard are being
dropped because FeatureUsageCardBody only accepts justifyContent and children
and does not forward extra props; fix by either (A) updating
FeatureUsageCardBody to accept props like ({ justifyContent, children, ...rest
}) and spread {...rest} onto its root element so aria-busy and aria-label
propagate, or (B) update AppsUsageCard to wrap the Skeleton in a wrapper element
(e.g., a Box/div) and apply aria-busy and aria-label to that wrapper instead of
passing them into FeatureUsageCardBody; reference the FeatureUsageCardBody and
AppsUsageCard components and the aria-busy / aria-label attributes when making
the change.
🪄 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: CHILL
Plan: Pro
Run ID: 57c56419-387a-4fed-9838-52552e8ab755
📒 Files selected for processing (1)
apps/meteor/client/views/admin/subscription/components/cards/AppsUsageCard/AppsUsageCard.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/admin/subscription/components/cards/AppsUsageCard/AppsUsageCard.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: aleksandernsilva
Repo: RocketChat/Rocket.Chat PR: 36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR `#36972` through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/client/views/admin/subscription/components/cards/AppsUsageCard/AppsUsageCard.tsx
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/client/views/admin/subscription/components/cards/AppsUsageCard/AppsUsageCard.tsx
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/admin/subscription/components/cards/AppsUsageCard/AppsUsageCard.tsx
| <FeatureUsageCardBody justifyContent='flex-start' aria-busy={true} aria-label={t('Loading')}> | ||
| <Skeleton variant='rect' width='x112' height='x224' role='presentation' /> | ||
| </FeatureUsageCardBody> |
There was a problem hiding this comment.
ARIA attributes are silently ignored and will not reach the DOM.
Based on the FeatureUsageCardBody implementation (see FeatureUsageCardBody.tsx:1-14), the component only accepts justifyContent and children props. It does not use a spread operator to forward additional attributes. The aria-busy and aria-label props passed here will be silently discarded, and the accessibility fix will have no effect.
Either update FeatureUsageCardBody to accept and forward ARIA props, or apply the attributes to a wrapper element directly in this file.
Option 1: Modify FeatureUsageCardBody to forward props
In FeatureUsageCardBody.tsx:
import { Box, CardBody } from '@rocket.chat/fuselage';
-import type { ReactNode } from 'react';
+import type { ComponentProps, ReactNode } from 'react';
-type FeatureUsageCardBodyProps = { justifyContent?: 'flex-start' | 'center'; children: ReactNode };
+type FeatureUsageCardBodyProps = { justifyContent?: 'flex-start' | 'center'; children: ReactNode } & Omit<ComponentProps<typeof Box>, 'justifyContent' | 'children'>;
-const FeatureUsageCardBody = ({ justifyContent = 'center', children }: FeatureUsageCardBodyProps) => (
+const FeatureUsageCardBody = ({ justifyContent = 'center', children, ...props }: FeatureUsageCardBodyProps) => (
<CardBody>
- <Box h='full' w='full' display='flex' alignItems='center' justifyContent={justifyContent} flexDirection='column'>
+ <Box h='full' w='full' display='flex' alignItems='center' justifyContent={justifyContent} flexDirection='column' {...props}>
{children}
</Box>
</CardBody>
);Option 2: Use a wrapper Box directly in AppsUsageCard
+import { Box, Skeleton } from '@rocket.chat/fuselage';
-import { Skeleton } from '@rocket.chat/fuselage';
...
if (!privateAppsLimit || !marketplaceAppsLimit) {
return (
<FeatureUsageCard card={{ title: t('Apps') }}>
- <FeatureUsageCardBody justifyContent='flex-start' aria-busy={true} aria-label={t('Loading')}>
- <Skeleton variant='rect' width='x112' height='x224' role='presentation' />
- </FeatureUsageCardBody>
+ <FeatureUsageCardBody justifyContent='flex-start'>
+ <Box aria-busy={true} aria-label={t('Loading')}>
+ <Skeleton variant='rect' width='x112' height='x224' role='presentation' />
+ </Box>
+ </FeatureUsageCardBody>
</FeatureUsageCard>
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/meteor/client/views/admin/subscription/components/cards/AppsUsageCard/AppsUsageCard.tsx`
around lines 29 - 31, The ARIA attributes passed into AppsUsageCard are being
dropped because FeatureUsageCardBody only accepts justifyContent and children
and does not forward extra props; fix by either (A) updating
FeatureUsageCardBody to accept props like ({ justifyContent, children, ...rest
}) and spread {...rest} onto its root element so aria-busy and aria-label
propagate, or (B) update AppsUsageCard to wrap the Skeleton in a wrapper element
(e.g., a Box/div) and apply aria-busy and aria-label to that wrapper instead of
passing them into FeatureUsageCardBody; reference the FeatureUsageCardBody and
AppsUsageCard components and the aria-busy / aria-label attributes when making
the change.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/views/admin/subscription/components/cards/AppsUsageCard/AppsUsageCard.tsx">
<violation number="1" location="apps/meteor/client/views/admin/subscription/components/cards/AppsUsageCard/AppsUsageCard.tsx:29">
P1: `FeatureUsageCardBody` only destructures `{ justifyContent, children }` and does not spread remaining props onto the underlying DOM element (see `FeatureUsageCardBody.tsx:4-12`). The `aria-busy` and `aria-label` attributes passed here will be silently dropped and never reach the DOM, making the entire accessibility fix ineffective. Either extend `FeatureUsageCardBody` to forward arbitrary props, or wrap the component in a `<div>` that carries the ARIA attributes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return ( | ||
| <FeatureUsageCard card={{ title: t('Apps') }}> | ||
| <FeatureUsageCardBody justifyContent='flex-start'> | ||
| <FeatureUsageCardBody justifyContent='flex-start' aria-busy={true} aria-label={t('Loading')}> |
There was a problem hiding this comment.
P1: FeatureUsageCardBody only destructures { justifyContent, children } and does not spread remaining props onto the underlying DOM element (see FeatureUsageCardBody.tsx:4-12). The aria-busy and aria-label attributes passed here will be silently dropped and never reach the DOM, making the entire accessibility fix ineffective. Either extend FeatureUsageCardBody to forward arbitrary props, or wrap the component in a <div> that carries the ARIA attributes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/admin/subscription/components/cards/AppsUsageCard/AppsUsageCard.tsx, line 29:
<comment>`FeatureUsageCardBody` only destructures `{ justifyContent, children }` and does not spread remaining props onto the underlying DOM element (see `FeatureUsageCardBody.tsx:4-12`). The `aria-busy` and `aria-label` attributes passed here will be silently dropped and never reach the DOM, making the entire accessibility fix ineffective. Either extend `FeatureUsageCardBody` to forward arbitrary props, or wrap the component in a `<div>` that carries the ARIA attributes.</comment>
<file context>
@@ -24,10 +24,9 @@ const AppsUsageCard = ({ privateAppsLimit, marketplaceAppsLimit }: AppsUsageCard
return (
<FeatureUsageCard card={{ title: t('Apps') }}>
- <FeatureUsageCardBody justifyContent='flex-start'>
+ <FeatureUsageCardBody justifyContent='flex-start' aria-busy={true} aria-label={t('Loading')}>
<Skeleton variant='rect' width='x112' height='x224' role='presentation' />
</FeatureUsageCardBody>
</file context>
Proposed changes
Implements WAI-ARIA loading semantics to the
AppsUsageCardfallback skeleton and cleans up an outstanding A11y technical debt marker.Root Cause / Problem
During initialization of the app limits, the loading
<Skeleton>was strictly rendering asrole='presentation'. This caused screen readers to completely skip the loading state, resulting in a confusing UI silence for visually impaired users. This was correctly flagged via// FIXME: not accessible enough.Solution / Behavior
Assigned robust aria-busy={true} and a localized aria-label={t('Loading')} to the layout wrapper. This properly announces the initialization transition state to screen readers without modifying the visual layout. The obsolete // FIXME has been cleared.
Issue(s)
Closes #39951
Validation & Testing
Run yarn lint locally (0 errors)
No side-effects introduced (pure ARIA attributes added)
Type of change
bug fix (resolving a bug via ARIA implementation)
Summary by CodeRabbit