chore(clerk-js,types,localizations): API keys form refactor #6153
Conversation
🦋 Changeset detectedLatest commit: 7240b09 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
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 Git ↗︎
|
wobsoriano
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx (1)
152-174: Localization implementation correctly addresses previous concerns.The expiration caption now properly uses localization keys instead of hardcoded English strings, which resolves the internationalization issue mentioned in previous reviews.
🧹 Nitpick comments (1)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx (1)
65-75: Consider adding input validation for edge cases.The function correctly handles the null case but could benefit from additional safeguards for potential edge cases in date calculations.
const getTimeLeftInSeconds = (expirationOption: Expiration): number | undefined => { if (!expirationOption) { return; } const now = new Date(); + + // Validate that we have a valid date + if (isNaN(now.getTime())) { + return; + } + const future = new Date(now); EXPIRATION_DURATIONS[expirationOption](future); - return Math.floor((future.getTime() - now.getTime()) / 1000); + const diff = Math.floor((future.getTime() - now.getTime()) / 1000); + + // Ensure we don't return negative values + return Math.max(0, diff); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/clerk-js/bundlewatch.config.json(1 hunks)packages/clerk-js/src/ui/components/ApiKeys/ApiKeysTable.tsx(1 hunks)packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx(3 hunks)packages/localizations/src/en-US.ts(7 hunks)packages/types/src/localization.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/clerk-js/bundlewatch.config.json
- packages/clerk-js/src/ui/components/ApiKeys/ApiKeysTable.tsx
- packages/localizations/src/en-US.ts
- packages/types/src/localization.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx (4)
34-42: LGTM! Clean and functional approach to date manipulation.The
EXPIRATION_DURATIONSobject provides a clear mapping from expiration options to date manipulation functions. The use of function references that mutate date objects is appropriate here sincegetTimeLeftInSecondscreates freshDateinstances for each calculation.
82-129: Well-implemented dropdown component with proper accessibility.The
ExpirationSelectorcomponent demonstrates good React practices:
- Proper use of
useLayoutEffectfor DOM measurements- Correct accessibility attributes with
aria-labelledby- Responsive width calculation for consistent UI
- Clean separation of concerns
195-242: Responsive layout implementation looks solid.The flexbox layout with proper responsive breakpoints using
mqu.smprovides a good user experience across different screen sizes. The side-by-side layout on larger screens and stacked layout on mobile is appropriate for this form.
244-261: Conditional rendering pattern is clean and effective.The conditional rendering of the description field based on the
showDescriptionprop from context provides good flexibility while maintaining clean component structure.
| dates: { | ||
| lastUsed__seconds: LocalizationValue<'seconds'>; | ||
| lastUsed__minutes: LocalizationValue<'minutes'>; | ||
| lastUsed__hours: LocalizationValue<'hours'>; | ||
| lastUsed__days: LocalizationValue<'days'>; | ||
| lastUsed__months: LocalizationValue<'months'>; | ||
| lastUsed__years: LocalizationValue<'years'>; | ||
| }; |
There was a problem hiding this comment.
Could this be considered a breaking change? 🤔
There was a problem hiding this comment.
Hi @anagstef! This component is in early preview and you need to have your instance flagged for API keys access. We also don't have the locales documented. It should be safe. Unless I'm missing something 🤔
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Alex Carpenter <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx(3 hunks)
🧰 Additional context used
🪛 ESLint
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx
[error] 1-1: 'useLayoutEffect' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 1-1: 'useLayoutEffect' is defined but never used.
(unused-imports/no-unused-imports)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx (8)
14-27: LGTM! Well-structured constants and types.The expiration values provide a comprehensive set of options, and the type definitions properly support the new dropdown-based expiration selection approach.
34-42: LGTM! Correct date manipulation logic.The expiration duration functions properly handle different time periods and correctly mutate the Date object to calculate future expiration times.
44-63: LGTM! Comprehensive localization key mapping.The function properly maps all expiration values to their localization keys, ensuring full internationalization support as requested in previous reviews.
65-75: LGTM! Correct time calculation logic.The function properly calculates expiration time in seconds by leveraging the duration mappings and handles the "never expire" case appropriately.
77-122: LGTM! Well-implemented dropdown component.The ExpirationSelector properly implements the Select component with localized options and good accessibility. This successfully addresses the previous review suggestion about using
referenceElementfor sizing.
145-167: LGTM! Proper localization implementation.The expiration caption logic correctly uses localized strings and maintains proper date formatting. This successfully addresses the previous review concern about hardcoded English text.
188-235: LGTM! Well-structured responsive form layout.The form layout properly implements responsive design with good spacing, accessibility attributes, and user-friendly labeling. The flexible layout adapts well to different screen sizes.
237-254: LGTM! Proper conditional rendering implementation.The description field is correctly hidden behind the
showDescriptionprop as specified in the PR objectives, while maintaining proper styling and form structure when displayed.
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx (5)
1-1: LGTM: Unused import removedGood fix removing the unused
useLayoutEffectimport as flagged in previous reviews.
34-42: Date manipulation approach is acceptableThe expiration duration functions use direct date manipulation methods. While this approach might have edge cases (e.g., adding 30 days to January 31st), it's acceptable for this API key expiration use case where precise date arithmetic isn't critical.
82-122: Well-implemented dropdown componentThe ExpirationSelector properly addresses the previous review comment about using
referenceElementfor sizing and includes good accessibility attributes (aria-labelledby,id).
188-235: Responsive layout implementation looks goodThe flex-based responsive layout properly adapts from row to column on smaller screens using media queries, and the form fields are appropriately structured.
237-254: Conditional description field rendering works correctlyThe conditional rendering based on the
showDescriptioncontext prop is properly implemented with appropriate styling for the separator border.
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx
Outdated
Show resolved
Hide resolved
Added some 👍🏼 |
| <Text> | ||
| {selectedExpiration?.label || t(localizationKeys('formFieldInputPlaceholder__apiKeyExpirationDate'))} | ||
| </Text> |
There was a problem hiding this comment.
Do we need this custom text? It leads to invalid markup having a p tag within a button. I think it can be removed from some quick testing, but if you need it, make the text a span.
There was a problem hiding this comment.
oh ugly, this was me testing the Select component 👀 will remove it in favor of the placeholder
| variant='caption' | ||
| colorScheme='secondary' | ||
| > | ||
| Optional |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx (6)
1-13: LGTM - Clean imports and proper dependenciesThe imports are well-organized and all are being used appropriately. Good to see the unused
useLayoutEffecthas been removed as flagged in previous reviews.
14-42: LGTM - Well-structured constants and expiration logicThe expanded expiration options and the
EXPIRATION_DURATIONSmapping provide a clean, maintainable approach to handle different expiration periods. The use of date mutation functions is appropriate since new Date objects are created each time.
44-75: LGTM - Proper localization mapping and time calculationsThe
getExpirationLocalizationKeyfunction provides comprehensive localization support, andgetTimeLeftInSecondscorrectly calculates the time difference using the duration functions.
82-118: LGTM - Excellent dropdown implementation with proper accessibilityThe
ExpirationSelectorcomponent is well-implemented with proper accessibility attributes (aria-labelledby,id), localization, and styling. The use ofreferenceElementfor positioning is a good practice.
184-232: LGTM - Excellent responsive layout and accessibilityThe form layout with responsive design (flex row on desktop, column on mobile) is well-implemented. The accessibility attributes and proper label associations are correctly set up.
234-251: LGTM - Proper conditional renderingThe conditional rendering of the description field based on the
showDescriptionprop from context is correctly implemented with appropriate styling.
| const expirationCaption = useMemo(() => { | ||
| const timeLeftInSeconds = getTimeLeftInSeconds(selectedExpiration?.value); | ||
|
|
||
| if (!selectedExpiration?.value || !timeLeftInSeconds) { | ||
| return t(localizationKeys('apiKeys.formFieldCaption__expiration__never')); | ||
| } | ||
|
|
||
| const expirationDate = new Date(Date.now() + timeLeftInSeconds * 1000); | ||
| return t( | ||
| localizationKeys('apiKeys.formFieldCaption__expiration__expiresOn', { | ||
| date: expirationDate.toLocaleString(undefined, { | ||
| year: 'numeric', | ||
| month: 'long', | ||
| day: '2-digit', | ||
| hour: 'numeric', | ||
| minute: '2-digit', | ||
| second: '2-digit', | ||
| hour12: true, | ||
| timeZoneName: 'short', | ||
| }), | ||
| }), | ||
| ); | ||
| }, [selectedExpiration?.value]); |
There was a problem hiding this comment.
Fix localization key usage for dynamic content
The expirationCaption is already a localized string (result of t(localizationKeys(...))), but it's being passed as localizationKey to the Text component on line 228. This is inconsistent with other Text usages in the component where localizationKey receives raw localization keys.
Since expirationCaption contains dynamic content (formatted date), it should be passed as children instead:
<Text
variant='caption'
colorScheme='secondary'
- localizationKey={expirationCaption}
elementDescriptor={descriptors.apiKeysCreateFormExpirationCaption}
-/>
+>
+ {expirationCaption}
+</Text>📝 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.
| const expirationCaption = useMemo(() => { | |
| const timeLeftInSeconds = getTimeLeftInSeconds(selectedExpiration?.value); | |
| if (!selectedExpiration?.value || !timeLeftInSeconds) { | |
| return t(localizationKeys('apiKeys.formFieldCaption__expiration__never')); | |
| } | |
| const expirationDate = new Date(Date.now() + timeLeftInSeconds * 1000); | |
| return t( | |
| localizationKeys('apiKeys.formFieldCaption__expiration__expiresOn', { | |
| date: expirationDate.toLocaleString(undefined, { | |
| year: 'numeric', | |
| month: 'long', | |
| day: '2-digit', | |
| hour: 'numeric', | |
| minute: '2-digit', | |
| second: '2-digit', | |
| hour12: true, | |
| timeZoneName: 'short', | |
| }), | |
| }), | |
| ); | |
| }, [selectedExpiration?.value]); | |
| --- a/packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx | |
| +++ b/packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx | |
| @@ -226,7 +226,7 @@ const CreateApiKeyForm: React.FC<CreateApiKeyFormProps> = ({ descriptors, ... }) => { | |
| <Text | |
| variant='caption' | |
| colorScheme='secondary' | |
| - localizationKey={expirationCaption} | |
| + elementDescriptor={descriptors.apiKeysCreateFormExpirationCaption} | |
| + > | |
| + {expirationCaption} | |
| + </Text> | |
| /> | |
| )} | |
| </Field> |
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/components/ApiKeys/CreateApiKeyForm.tsx around lines
141 to 163, the expirationCaption variable is a localized string with dynamic
content, but it is incorrectly passed as a localizationKey prop to the Text
component on line 228. To fix this, pass expirationCaption as children to the
Text component instead of using the localizationKey prop, ensuring consistent
handling of dynamic localized content.
Description
showDescriptionpropBefore:
After:
Screen.Recording.2025-06-18.at.6.34.17.PM.mov
Resolves USER-2223
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Improvements
Localization
Style
Chores