Uh oh!
There was an error while loading. Please reload this page.
fix(ui): announce pending workspace actions in the Mosaic user button - #9407
Conversation
Switching a workspace kept focus on the row while its spinner stayed aria-hidden, so the wait passed in silence. The indicator now carries a progressbar role and a name, paired with aria-busy on the row. Accept and Join announced the untranslated literal 'pending' and named no workspace. Both now take this surface's copy, and point at the row title through aria-describedby. The loading placeholder drops role='status': it mounts with its copy already in it, so there was no change for a live region to report.
🦋 Changeset detectedLatest commit: c6aecc8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/electron@clerk/electron-passkeys@clerk/eslint-plugin@clerk/expo@clerk/expo-google-signin@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
📝 WalkthroughWalkthroughUserButton workspace actions now expose accessible workspace descriptions, pending labels, Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/swingset/src/stories/user-button.stories.tsx (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the exported story return type.
Line 378 exports
LoadingOrganizationswithout an explicit return type. ImportJSXas a type and declare: JSX.Element.Proposed update
import { useEffect, useState } from 'react'; +import type { JSX } from 'react';-export function LoadingOrganizations(_args: Record<string, unknown>) {+export function LoadingOrganizations(_args: Record<string, unknown>): JSX.Element {As per coding guidelines, “Always define explicit return types for functions, especially public APIs.”
Also applies to: 378-398
🤖 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 `@packages/swingset/src/stories/user-button.stories.tsx` at line 12, Update the exported LoadingOrganizations story function to declare an explicit JSX.Element return type, and import JSX as a type from React alongside the existing hooks import. Keep the story implementation unchanged.Source: Coding guidelines
packages/swingset/src/stories/user-button.mdx (1)
141-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the compound-page section order.
Line 141 adds a top-level
Loadingsection. This compound-component page must useExample,Usage,Parts, thenStylingas its top-level sections. Move this loading content into an allowed section.As per coding guidelines, “Compound Components pages must use the exact section order:
Example,Usage,Parts, thenStyling.”🤖 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 `@packages/swingset/src/stories/user-button.mdx` around lines 141 - 154, Move the Loading section content and its LoadingOrganizations story into one of the existing allowed sections while preserving the required top-level order: Example, Usage, Parts, then Styling. Remove the standalone top-level Loading heading and keep the loading explanation and story together in the selected section.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 `@packages/ui/src/mosaic/user-button/__tests__/user-button.view.test.tsx`:
- Around line 724-731: Update the test case around
userButtonBusyKeys.selectOrganization in “leaves the rows standing down beside
it with nothing to report” to also assert that the Personal account row has
aria-disabled="true". Preserve the existing aria-busy and progressbar
assertions.
---
Nitpick comments:
In `@packages/swingset/src/stories/user-button.mdx`:
- Around line 141-154: Move the Loading section content and its
LoadingOrganizations story into one of the existing allowed sections while
preserving the required top-level order: Example, Usage, Parts, then Styling.
Remove the standalone top-level Loading heading and keep the loading explanation
and story together in the selected section.
In `@packages/swingset/src/stories/user-button.stories.tsx`:
- Line 12: Update the exported LoadingOrganizations story function to declare an
explicit JSX.Element return type, and import JSX as a type from React alongside
the existing hooks import. Keep the story implementation unchanged.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c790fa15-1113-4e14-a7b2-db923901992f
📒 Files selected for processing (6)
.changeset/mosaic-user-button-pending-announcement.mdpackages/swingset/src/stories/user-button.mdxpackages/swingset/src/stories/user-button.stories.tsxpackages/ui/src/mosaic/user-button/__tests__/user-button.view.test.tsxpackages/ui/src/mosaic/user-button/user-button.messages.tspackages/ui/src/mosaic/user-button/user-button.view.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/cli(auto-detected)clerk/clerk-android(auto-detected)
| // The rows waiting on it are not running anything, so they carry the indicator's opposite. | ||
| it('leaves the rows standing down beside it with nothing to report', () => { | ||
| render(surface(userButtonBusyKeys.selectOrganization('org_2'))); | ||
| const row = screen.getByRole('button', { name: 'Personal account' }); | ||
| expect(row).not.toHaveAttribute('aria-busy'); | ||
| expect(within(row).queryByRole('progressbar')).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that the waiting row stays disabled.
Line 724 states that these rows stand down. The test only checks that the row has no aria-busy attribute and no progressbar. Add an assertion for aria-disabled="true" so the test protects the disabled-state contract.
Proposed test update
const row = screen.getByRole('button', { name: 'Personal account' });
+expect(row).toHaveAttribute('aria-disabled', 'true');
expect(row).not.toHaveAttribute('aria-busy');
expect(within(row).queryByRole('progressbar')).toBeNull();As per coding guidelines, “Unit tests are required for all new functionality” and must verify edge cases.
📝 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.
| // The rows waiting on it are not running anything, so they carry the indicator's opposite. | |
| it('leaves the rows standing down beside it with nothing to report',()=>{ | |
| render(surface(userButtonBusyKeys.selectOrganization('org_2'))); | |
| constrow=screen.getByRole('button',{name: 'Personal account'}); | |
| expect(row).not.toHaveAttribute('aria-busy'); | |
| expect(within(row).queryByRole('progressbar')).toBeNull(); | |
| }); | |
| // The rows waiting on it are not running anything, so they carry the indicator's opposite. | |
| it('leaves the rows standing down beside it with nothing to report',()=>{ | |
| render(surface(userButtonBusyKeys.selectOrganization('org_2'))); | |
| constrow=screen.getByRole('button',{name: 'Personal account'}); | |
| expect(row).toHaveAttribute('aria-disabled','true'); | |
| expect(row).not.toHaveAttribute('aria-busy'); | |
| expect(within(row).queryByRole('progressbar')).toBeNull(); | |
| }); |
🤖 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 `@packages/ui/src/mosaic/user-button/__tests__/user-button.view.test.tsx`
around lines 724 - 731, Update the test case around
userButtonBusyKeys.selectOrganization in “leaves the rows standing down beside
it with nothing to report” to also assert that the Personal account row has
aria-disabled="true". Preserve the existing aria-busy and progressbar
assertions.
Source: Coding guidelines
Description
Pending states in the Mosaic user button were silent to VoiceOver.
aria-hidden, so nothing about the focused row changed and the wait passed in silence. Spinner now carriesrole='progressbar'+ a name, paired witharia-busyon the row. Same pairingSubmitButtonuses.SubmitButton's untranslated fallbackpending. Now takes this surface's copy.aria-describedbypointing at that title.role='status'— it mounts with its copy already in it, so there was no change for a live region to report. Comment claimed otherwise.Swingset gets a
LoadingOrganizationsexample, since every other example is handed its organizations on first render and never shows the placeholder.Verified by hand in VoiceOver, not just by assertion.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change