Uh oh!
There was an error while loading. Please reload this page.
chore(clerk-js): Add API Keys component descriptors - #6095
Conversation
@clerk/agent-toolkit@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/dev-cli@clerk/elements@clerk/clerk-expo@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/clerk-react@clerk/react-router@clerk/remix@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/themes@clerk/types@clerk/upgrade@clerk/vuecommit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx (1)
3-3: Import list is getting long – consider multi-line formattingThe utilities import now spans many identifiers; for readability consider:
-import { Box, Button, Col, descriptors, Flex, FormLabel, localizationKeys, Text } from '@/ui/customizables';+import {+ Box,+ Button,+ Col,+ descriptors,+ Flex,+ FormLabel,+ localizationKeys,+ Text,+} from '@/ui/customizables';packages/clerk-js/src/ui/customizables/elementDescriptors.ts (1)
462-482: Manual list maintenance is error-proneThe API-keys keys are appended in three places:
ElementsConfigAPPEARANCE_KEYS(here)- Component usage
A single source of truth (e.g. generating
APPEARANCE_KEYSfromElementsConfigat build-time) would prevent future omissions & cut review overhead.packages/clerk-js/src/ui/components/ApiKeys/RevokeAPIKeyConfirmationModal.tsx (2)
84-87: Consider adding a row-specificelementIdfor stronger selector granularity
Card.Rootgains anelementDescriptor, which is great, but without a complementaryelementIdthe selector will match every revoke-modal in the DOM.
If multiple revoke modals can coexist (e.g. in tests or storybook), distinguishing them becomes impossible.- <Card.Root- role='alertdialog'- elementDescriptor={descriptors.apiKeysRevokeModal}- >+ <Card.Root+ role='alertdialog'+ elementDescriptor={descriptors.apiKeysRevokeModal}+ elementId={descriptors.apiKeysRevokeModal.setId(apiKeyId ?? 'revoke-modal')}+ >
46-55: Make the confirmation check case-insensitiveUsers frequently type revoke in lowercase; failing the check feels unnecessarily strict.
-const canSubmit = revokeField.value === 'Revoke';+const canSubmit = revokeField.value.trim().toLowerCase() === 'revoke';packages/clerk-js/src/ui/components/ApiKeys/ApiKeysTable.tsx (2)
54-56:aria-labelnot updated when state changesYou already added descriptor metadata to
CopySecretButton; while you’re touching this block, consider toggling thearia-labelafter the copy operation so screen-reader users know the button’s new purpose.-aria-label={hasCopied ? 'Copied API key to clipboard' : 'Copy API key'}+aria-label={hasCopied ? 'Copied API key to clipboard' : 'Copy API key'}(You may instead expose the string through
localizationKeyslike the rest of the UI.)
98-100:Show keylabel becomes stale after revealWhen
revealedtoggles, the button continues to say Show key. Swap the label to Hide key (and localise) for accurate accessibility.-aria-label={'Show key'}+aria-label={revealed ? 'Hide key' : 'Show key'}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/chilly-pears-film.md(1 hunks)packages/clerk-js/src/ui/components/ApiKeys/ApiKeys.tsx(6 hunks)packages/clerk-js/src/ui/components/ApiKeys/ApiKeysTable.tsx(8 hunks)packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx(4 hunks)packages/clerk-js/src/ui/components/ApiKeys/RevokeAPIKeyConfirmationModal.tsx(4 hunks)packages/clerk-js/src/ui/components/ApiKeys/useApiKeys.ts(0 hunks)packages/clerk-js/src/ui/customizables/elementDescriptors.ts(1 hunks)packages/types/src/appearance.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/clerk-js/src/ui/components/ApiKeys/useApiKeys.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx (1)
packages/clerk-js/src/ui/customizables/elementDescriptors.ts (1)
descriptors(545-545)
packages/clerk-js/src/ui/components/ApiKeys/ApiKeys.tsx (1)
packages/clerk-js/src/ui/customizables/elementDescriptors.ts (1)
descriptors(545-545)
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (nextjs, chrome, 13)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (tanstack-react-router, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (quickstart, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (18, --filter=@clerk/astro --filter=@clerk/backend --filter=@clerk/express --filter=@c...
- GitHub Check: Static analysis
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
packages/types/src/appearance.ts (1)
589-609: API-keys descriptors correctly wired intoElementsConfigThe new
apiKeys*entries are all declared with the properWithOptionssignature (WithOptions<string>for id-based parts, plainWithOptionsfor the rest) and the naming matches the keys used elsewhere (elementDescriptors.ts, components). No functional issues spotted.packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx (2)
100-106: Good:FormContaineris now targettableSupplying
elementDescriptor={descriptors.apiKeysCreateForm}ensures the form container can be themed or queried in tests – nice.
196-202: ```shell
#!/bin/bashLocate the definition of SubmitButton in Form.tsx to check supported props
rg -n --context 5 "SubmitButton" packages/clerk-js/src/ui/elements/Form.tsx
</details> <details> <summary>packages/clerk-js/src/ui/components/ApiKeys/ApiKeys.tsx (4)</summary> `8-18`: **Import keeps file-local barrel pattern consistent** Including `descriptors` in the barrel import keeps consistency with other pages. Looks good. --- `94-108`: **Root & header containers now descriptor-enabled** Adding `elementDescriptor` on the main `<Col>` and header `<Flex>` gives users fine-grained styling hooks – nice improvement. --- `118-129`: **Search input and “Add” button descriptors** These are the primary interactive controls; descriptor coverage here is valuable for e2e tests. --- `144-148`: **Table descriptor wired through** `ApiKeysTable` now receives `elementDescriptor={descriptors.apiKeysTable}` — make sure `ApiKeysTableProps` includes this optional prop (it was marked optional in the PR description). </details> <details> <summary>.changeset/chilly-pears-film.md (1)</summary> `1-7`: **Changeset OK** Version bumps & summary look correct. </details> <details> <summary>packages/clerk-js/src/ui/components/ApiKeys/RevokeAPIKeyConfirmationModal.tsx (1)</summary> `99-103`: **`Form.ControlRow` lacks an `elementId`; localisation fallback still hard-coded** 1. As with the modal root, adding an `elementId` would allow tests/themes to target the exact input when several modals are open. 2. The label/placeholder text (`"Revoke"`) remains hard-coded and English-only. There is already a TODO for localisation – consider wiring `localizationKeys` now so this PR is fully i18n-ready. ```diff - <Form.ControlRow - elementId={revokeField.id} - elementDescriptor={descriptors.apiKeysRevokeModalInput} - > + <Form.ControlRow + elementId={descriptors.apiKeysRevokeModalInput.setId(revokeField.id)} + elementDescriptor={descriptors.apiKeysRevokeModalInput} + >Likely an incorrect or invalid review comment.
packages/clerk-js/src/ui/components/ApiKeys/ApiKeysTable.tsx (1)
125-127: Passing a possiblyundefinedelementDescriptoris fine—guard insideTableto avoid prop-bleedIf
Tablespreads incoming props onto a DOM element,undefinedis harmless in React but can leak intodata-attributes if not filtered. Ensure theTablecomponent strips falsy descriptors internally. No action required here if that guard already exists.
alexcarpenter
left a comment
There was a problem hiding this comment.
Looking good, I do think we should remove the id usage. There is also a handful of elements with internal only classNames.
| }} | ||
| elementDescriptor={descriptors.apiKeysHeader} | ||
| > | ||
| <Box> |
There was a problem hiding this comment.
Box should also include a descriptor here.
Uh oh!
There was an error while loading. Please reload this page.
| export const Th = makeCustomizable(makeLocalizable(sanitizeDomProps(Primitives.Th))); | ||
| export const Td = makeCustomizable(makeLocalizable(sanitizeDomProps(Primitives.Td))); |
There was a problem hiding this comment.
Should we also include defaultDescriptors for th and td below?
Description
This PR adds element descriptors to the
<APIKeys />AIO component for customization.Coverage:
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Summary by CodeRabbit