Conversation
MichaelUnkey
requested review from
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ParentComponent
participant VisibleButton
User->>VisibleButton: Clicks button
VisibleButton->>ParentComponent: Calls setIsVisible(!isVisible)
ParentComponent->>VisibleButton: Passes updated isVisible prop
VisibleButton-->>User: Updates icon and aria-label/title
Suggested labels
Suggested reviewers
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![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
🧹 Nitpick comments (1)
apps/engineering/content/design/components/visual-button.mdx (1)
60-60: Fix grammatical issueMissing article "the" as flagged by static analysis.
-4. The timer resets if the content is hidden manually before timeout +4. The timer resets if the content is hidden manually before the timeout🧰 Tools
🪛 LanguageTool
[uncategorized] ~60-~60: You might be missing the article “the” here.
Context: ...f the content is hidden manually before timeout ## Accessibility - Includessr-only...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx(1 hunks)apps/dashboard/app/new/keys.tsx(1 hunks)apps/engineering/content/design/components/buttons/visual-button.examples.tsx(1 hunks)apps/engineering/content/design/components/visual-button.mdx(1 hunks)internal/ui/src/components/visible-button.tsx(1 hunks)internal/ui/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/engineering/content/design/components/buttons/visual-button.examples.tsx (2)
apps/engineering/app/components/render.tsx (1)
RenderComponentWithSnippet(12-67)internal/ui/src/components/visible-button.tsx (1)
VisibleButton(13-45)
🪛 LanguageTool
apps/engineering/content/design/components/visual-button.mdx
[uncategorized] ~60-~60: You might be missing the article “the” here.
Context: ...f the content is hidden manually before timeout ## Accessibility - Includes sr-only...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: autofix
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
internal/ui/src/index.ts (1)
19-19: Export addition looks good.The new export for
visible-buttonfollows the established pattern and correctly exposes the component from the UI package.internal/ui/src/components/visible-button.tsx (3)
1-1: Client directive correctly added.The
"use client"directive is necessary since this component uses React hooks (useEffect) and event handlers.
4-6: Import adjustments are appropriate for the package migration.The React import with biome ignore comment provides good explanation, and the relative path for utilities is correct for the new location in the UI package.
13-45: Component implementation is well-structured.The component properly handles:
- Auto-hide functionality with cleanup
- Accessibility via sr-only text
- Proper prop spreading and type safety
- Correct dependency array in useEffect
apps/dashboard/app/new/keys.tsx (1)
16-16:Details
✅ Verification successful
Import migration completed correctly.
The import change from the local dashboard component to the UI package aligns with the refactoring objective and should work correctly given the proper export in the UI package.
Verify that the component is properly accessible from the UI package:
🏁 Script executed:
#!/bin/bash # Description: Verify VisibleButton is exported from @unkey/ui package # Check if the component is properly exported ast-grep --pattern 'export * from "./components/visible-button"' # Verify the component file exists fd "visible-button.tsx" internal/ui/src/components/Length of output: 237
Import migration verified: VisibleButton is exported from @unkey/ui
Confirmed that
VisibleButtonis re-exported ininternal/ui/src/index.ts(line 19) and the source component exists atinternal/ui/src/components/visible-button.tsx.
No further action required—approve the change.apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
19-19: Consistent import migration pattern.The import change follows the same pattern as other dashboard files and correctly centralizes the component usage to the UI package.
apps/engineering/content/design/components/buttons/visual-button.examples.tsx (2)
1-1: LGTM: Correct client directive usageThe "use client" directive is properly placed for this interactive component that uses React state.
7-28: Well-structured demo component with practical examplesThe demo effectively showcases two common use cases:
- Simple visibility toggle with user feedback
- Sensitive content masking (API keys)
The shared state approach ensures both examples stay synchronized, which enhances the demonstration.
apps/engineering/content/design/components/visual-button.mdx (1)
1-75: Comprehensive and well-structured documentationThe documentation effectively covers all aspects of the VisibleButton component:
- Clear feature list and behavioral specifications
- Accurate props table matching the component interface
- Practical usage examples
- Accessibility and design considerations
The 10-second auto-hide timer and other behavioral details are correctly documented based on the component implementation.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~60-~60: You might be missing the article “the” here.
Context: ...f the content is hidden manually before timeout ## Accessibility - Includessr-only...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
apps/engineering/content/design/components/buttons/visual-button.examples.tsx
Outdated
Show resolved
Hide resolved
…on.examples.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ogzhanolguncu
left a comment
ogzhanolguncu
left a comment
There was a problem hiding this comment.
I think visual button should use the same styling as this one. TLDR; it should use our component. Let's just copy it from there, wdyt?
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
internal/ui/src/components/visible-button.tsx (2)
3-4: Track the TODO for icon migration.This TODO indicates technical debt that should be tracked. Consider creating an issue to convert these icons to Nucleo Icons and move them to the unkey/icons package.
Would you like me to create an issue to track this icon migration task?
5-6: Remove misleading biome comment.The comment is inaccurate as React is actively used for
useEffect, not just for types. Consider removing this comment or updating it to accurately reflect why the biome rule is being ignored.-// biome-ignore lint: React in this context is used throughout, so biome will change to types because no APIs are used even though React is needed. import React, { useEffect } from "react";apps/engineering/tailwind.config.js (2)
16-19: Address technical debt for theme consolidation.The comment indicates that colors and font sizes should be moved to
@unkey/ui. Since this PR is already refactoring UI components into the shared package, consider addressing this technical debt now to avoid duplication.Would you like me to help refactor these theme values into the
@unkey/uipackage or create an issue to track this work?
161-162: Use Object.hasOwn instead of hasOwnProperty.Replace
hasOwnPropertywith the saferObject.hasOwnmethod to avoid the biome warning.- // biome-ignore lint/suspicious/noPrototypeBuiltins: don't tell me what to do - if (obj2.hasOwnProperty(key)) { + if (Object.hasOwn(obj2, key)) {apps/engineering/app/global.css (1)
15-16: Fix inconsistent CSS color value formatting.Some HSL color values are missing spaces after commas. For consistency with other color definitions in the file, add spaces after commas.
- --warn: 46, 97%, 65%; - --warn-foreground: 46, 97%, 15%; + --warn: 46 97% 65%; + --warn-foreground: 46 97% 15%;- --content-warn: 46, 97%, 40%; + --content-warn: 46 97% 40%;- --warn: 46, 100%, 45%; - --warn-foreground: 46, 97%, 5%; + --warn: 46 100% 45%; + --warn-foreground: 46 97% 5%;- --content-warn: 46, 100%, 55%; + --content-warn: 46 100% 55%;Also applies to: 32-33, 51-52, 67-68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/dashboard/app/new/keys.tsx(1 hunks)apps/engineering/app/global.css(1 hunks)apps/engineering/app/layout.tsx(1 hunks)apps/engineering/content/design/components/buttons/visual-button.examples.tsx(1 hunks)apps/engineering/tailwind.config.js(2 hunks)internal/icons/src/icons/hide.tsx(1 hunks)internal/icons/src/icons/view.tsx(1 hunks)internal/icons/src/index.ts(2 hunks)internal/ui/src/components/visible-button.tsx(1 hunks)internal/ui/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- apps/engineering/app/layout.tsx
- internal/icons/src/index.ts
- internal/icons/src/icons/hide.tsx
- internal/icons/src/icons/view.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/ui/src/index.ts
- apps/dashboard/app/new/keys.tsx
- apps/engineering/content/design/components/buttons/visual-button.examples.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/ui/src/components/visible-button.tsx (3)
internal/ui/src/components/button.tsx (2)
ButtonProps(190-221)Button(396-396)internal/icons/src/icons/view.tsx (1)
View(16-47)internal/icons/src/icons/hide.tsx (1)
Hide(16-62)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
|
🔍 Review requested from @ogzhanolguncu |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
internal/ui/src/components/visible-button.tsx (1)
8-21: Maketitlerequired & drop redundantvariantoverride
titledrives thearia-label/titleattributes – when omitted the button renders “Hide undefined / Show undefined”, breaking a11y.
variantalready exists insideButtonProps; repeating it forces needless prop-picking and risks API drift.-type VisibleButtonProps = ButtonProps & { - /** - * Current visibility state - */ - isVisible: boolean; - /** - * Function to set the visibility state - */ - setIsVisible: (visible: boolean) => void; - /** - * Variant of the button - */ - variant?: ButtonProps["variant"]; -}; +type VisibleButtonProps = Omit<ButtonProps, "title" | "variant"> & { + /** Current visibility state */ + isVisible: boolean; + /** State setter provided by parent */ + setIsVisible: (visible: boolean) => void; + /** Descriptive label used for accessibility (“Password”, “API key”, …) */ + title: string; + /** Optional visual variant – falls back to `"outline"` */ + variant?: ButtonProps["variant"]; +};apps/dashboard/app/new/keys.tsx (1)
152-157: Still missing the mandatorytitleprop onVisibleButtonPrevious review highlighted the a11y issue; instances here still render “Hide undefined / Show undefined”.
-<VisibleButton - isVisible={showKey} - setIsVisible={setShowKey} +<VisibleButton + isVisible={showKey} + setIsVisible={setShowKey} + title="Root key"Apply analogous titles for the snippet (
"Root key snippet") and API key sections ("API key").Also applies to: 175-180, 215-220, 237-241
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
296-301: Add descriptivetitleprop for accessibilitySame omission as above – provide a clear noun so screen-reader users know what will be shown / hidden.
-<VisibleButton - isVisible={showKey} - setIsVisible={setShowKey} +<VisibleButton + isVisible={showKey} + setIsVisible={setShowKey} + title="Root key"Repeat for the snippet button (
title="Snippet").Also applies to: 313-317
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx(3 hunks)apps/dashboard/app/new/keys.tsx(5 hunks)apps/engineering/content/design/components/buttons/visual-button.examples.tsx(1 hunks)apps/engineering/content/design/components/buttons/visual-button.mdx(1 hunks)apps/engineering/package.json(2 hunks)internal/ui/src/components/visible-button.tsx(1 hunks)internal/ui/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/dashboard/app/new/keys.tsx (1)
internal/ui/src/components/visible-button.tsx (1)
VisibleButton(23-48)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
internal/ui/src/components/visible-button.tsx (1)
VisibleButton(23-48)
apps/engineering/content/design/components/buttons/visual-button.examples.tsx (2)
apps/engineering/app/components/render.tsx (1)
RenderComponentWithSnippet(12-67)internal/ui/src/components/visible-button.tsx (1)
VisibleButton(23-48)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test API / API Test Local
🔇 Additional comments (4)
internal/ui/src/index.ts (1)
22-22: Export looks good
VisibleButtonis now part of the public surface – perfect.apps/engineering/content/design/components/buttons/visual-button.examples.tsx (1)
18-24: Example is clear and passes required props – niceThe demo supplies
title, variant overrides, and custom classes correctly. Good showcase.Also applies to: 32-37
apps/engineering/content/design/components/buttons/visual-button.mdx (2)
5-6: Import statement looks good
TheVisibleButtonDemoimport path and MDX import syntax are correct.
18-36: Usage example is clear
The TSX snippet correctly demonstrates the component API and default behavior.
apps/engineering/content/design/components/buttons/visual-button.mdx
Outdated
Show resolved
Hide resolved
apps/engineering/content/design/components/buttons/visual-button.mdx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/engineering/package.json (2)
32-34: Tailwind plugins correctly moved to devDependencies
The build-time plugins (@tailwindcss/aspect-ratio,@tailwindcss/container-queries,@tailwindcss/typography) are now properly grouped underdevDependenciesas recommended.
27-28: Consolidate Tailwind CSS declaration to avoid version conflicts
Tailwind itself is a build‐time tool and should not be independencies, especially since it's already declared (v3.4.10) underdevDependencies. Remove the"tailwindcss": "^3.4.3"entry fromdependenciesto eliminate duplication and prevent production bundle bloat.--- a/apps/engineering/package.json +++ b/apps/engineering/package.json @@ dependencies - "tailwind-merge": "^2.5.4", - "tailwindcss": "^3.4.3", + "tailwind-merge": "^2.5.4", "zod": "^3.23.8"internal/ui/src/components/visible-button.tsx (2)
8-21:titlemust be required – avoid “Hide undefined” & drop redundantvariantprop
titleis interpolated into botharia-labelandtitleattributes but is currently optional because it comes fromButtonProps. When the caller omits it, assistive tech and tooltips receive “Hide undefined”, which is an a11y regression.
Additionally,variantis already part ofButtonProps; re-declaring it here bloats the surface API without adding value.-type VisibleButtonProps = ButtonProps & { +type VisibleButtonProps = Omit<ButtonProps, "title" | "variant"> & { /** * Current visibility state */ isVisible: boolean; /** * Function to set the visibility state */ setIsVisible: (visible: boolean) => void; - /** - * Variant of the button - */ - variant?: ButtonProps["variant"]; + /** + * Mandatory label used for aria / tooltip text + */ + title: string; };
28-44: Compose caller’sonClickand respectevent.defaultPrevented
onClickis destructured but never invoked, so consumers lose their handler.
Call it first, then toggle only if the event wasn’t cancelled:- onClick={(e) => { - if (!e.defaultPrevented) { - setIsVisible(!isVisible); - } - }} + onClick={(e) => { + onClick?.(e); + if (!e.defaultPrevented) { + setIsVisible(!isVisible); + } + }}This preserves extensibility (e.g., analytics, validation) and keeps the toggle cancellable.
Don’t forget to includeonClickin the destructuring list ({ onClick, ...rest }) as you already do.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/engineering/content/design/components/buttons/visual-button.mdx(1 hunks)apps/engineering/package.json(1 hunks)internal/ui/src/components/visible-button.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/ui/src/components/visible-button.tsx (3)
internal/ui/src/components/button.tsx (2)
ButtonProps(233-264)Button(439-439)internal/icons/src/icons/eye-slash.tsx (1)
EyeSlash(15-64)internal/icons/src/icons/eye.tsx (1)
Eye(15-47)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/engineering/package.json (1)
41-41: Animation plugin placement is correct
Listingtailwindcss-animateunderdevDependenciesis appropriate since it’s only needed during the build process.
apps/engineering/content/design/components/buttons/visual-button.mdx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/engineering/content/design/components/buttons/visual-button.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/design/components/buttons/visual-button.mdx
[uncategorized] ~12-~12: You might be missing the article “the” here.
Context: ...support. - Customizable styling through className prop. - Configurable button `...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/engineering/content/design/components/buttons/visual-button.mdx (9)
1-4: Frontmatter is correctly usingdescription.
Aligns with standard documentation conventions by usingdescriptioninstead ofsummary.
5-6: The importedVisibleButtonDemois utilized in the Examples section; no action needed.
7-14: Features list is clear and properly formatted.
Prop names are wrapped in backticks and each bullet ends with a period.
15-36: Usage example is clear and comprehensive.
The TSX code block correctly demonstrates state management, accessibility labels, and variant usage.
38-41: Example demo integration is present.
Including<VisibleButtonDemo />provides a concrete preview of the component.
52-53: Forwarded props note is helpful.
Linking to the Button component docs guides readers to explore additional props.
54-58: Behavior section is well-defined.
Numbered items are concise, descriptive, and punctuated correctly.
59-65: Accessibility guidelines are thorough.
Bullet points cover ARIA details and maintain punctuation consistency.
66-74: Design section clearly outlines visual characteristics.
Bullets succinctly cover styling, iconography, and responsive considerations.
2 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Placed used still look and function as production.
Created key dialog, Created Root key Dialog. shows hidden key when clicked
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Enhancements
Style
Chores