feat: 🎸 initialize base ui and replace button component #218
Conversation
|
@borabaloglu is attempting to deploy a commit to the ossdotnow Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughReplaces Button’s asChild pattern with a render prop across the app, updates the Button component implementation and variants/sizes, adds UI styling changes (selection, scrollbars, .root), introduces conditional repo queries in LaunchSidebar, restructures the admin edit page composition, and updates package/config files with new dependencies and registries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant LS as LaunchSidebar
participant V as isValidProvider/isValidGitHost
participant Q1 as repoQuery
participant Q2 as repoDataQuery
U->>LS: Open Launch Page
LS->>V: Validate provider and repo URL
alt Valid (both true)
LS->>Q1: Fetch repo (enabled)
Q1-->>LS: repo data
alt repo data exists
LS->>Q2: Fetch repo details (enabled)
Q2-->>LS: repo details
else no repo data
Note over LS: Skip repoDataQuery
end
else Invalid
Note over LS: Skip repo queries
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| size="icon-sm" | ||
| render={(props) => ( | ||
| <Link | ||
| href={ensureHttpProtocol(profile.git.blog!)} |
There was a problem hiding this comment.
| href={ensureHttpProtocol(profile.git.blog!)} | |
| href={ensureHttpProtocol(profile.git.blog)} |
A non-null assertion operator (!) was added to profile.git.blog! where it wasn't present in the original code, which could cause a runtime error if the value is undefined.
View Details
Analysis
In the migration from asChild to render props, line 190 changed from ensureHttpProtocol(profile.git.blog) to ensureHttpProtocol(profile.git.blog!). The non-null assertion operator ! was added, but this is unsafe since the original code didn't have it and the surrounding condition profile?.git?.blog && suggests that blog can be undefined. If profile.git.blog is undefined at runtime, this will throw a TypeError when passed to ensureHttpProtocol().
The fix is to remove the non-null assertion operator and keep the original behavior: ensureHttpProtocol(profile.git.blog).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/(admin)/admin/projects/[id]/edit/page.tsx (1)
59-97: Prevent form data clobbering on background revalidationReact Query may revalidate after focus/network changes. The effect resets the form whenever dependencies flip, which can wipe unsaved edits. Guard with isDirty and key the effect by projectId.
React.useEffect(() => { - if ( + // Avoid clobbering user edits on background revalidations + if (form.formState.isDirty) return; + if ( projectData && isProject(projectData) && !projectStatusesLoading && !projectTypesLoading && !tagsLoading ) { const currentTags = projectData.tagRelations?.map((relation) => relation.tag?.name).filter(Boolean) ?? []; const formData = { name: projectData.name, description: projectData.description || '', gitRepoUrl: projectData.gitRepoUrl, gitHost: projectData.gitHost || 'github', logoUrl: projectData.logoUrl || '', approvalStatus: projectData.approvalStatus, status: projectData.status?.name || '', type: projectData.type?.name || '', tags: currentTags, socialLinks: { twitter: projectData.socialLinks?.twitter || '', discord: projectData.socialLinks?.discord || '', linkedin: projectData.socialLinks?.linkedin || '', website: projectData.socialLinks?.website || '', }, isLookingForContributors: projectData.isLookingForContributors, isLookingForInvestors: projectData.isLookingForInvestors, isHiring: projectData.isHiring, isPublic: projectData.isPublic, hasBeenAcquired: projectData.hasBeenAcquired, isPinned: projectData.isPinned, isRepoPrivate: projectData.isRepoPrivate, }; form.reset(formData); } - }, [projectData, projectStatusesLoading, projectTypesLoading, tagsLoading, form]); + }, [projectId, projectData, projectStatusesLoading, projectTypesLoading, tagsLoading, form.formState.isDirty, form]);
♻️ Duplicate comments (2)
apps/web/app/(admin)/admin/layout.tsx (1)
46-55: Same forwardRef concern for Link used in Button.render.
Please apply the Link forwardRef change noted in profile-tabs.tsx to ensure ref and focus handling works here as well.apps/web/components/user/profile.tsx (1)
185-197: Remove unsafe non-null assertion and add rel.
The blog href uses profile.git.blog!; this was flagged earlier as unsafe and can throw if undefined. Also add rel to match security best practices.Apply this diff:
- <Link - href={ensureHttpProtocol(profile.git.blog!)} - target="_blank" - {...props} - > + <Link + href={ensureHttpProtocol(profile.git.blog)} + target="_blank" + rel="noopener noreferrer" + {...props} + >
🧹 Nitpick comments (12)
packages/ui/package.json (1)
10-10: Peer dependencies verified; optional pin of prerelease
PeerDependencies "@base-ui-components/react@^1.0.0-beta.2" require React, React-DOM, @types/react "^17 || ^18 || ^19", so React 17–19 compatibility is satisfied.
Optional: drop the caret and pin to "1.0.0-beta.2" if you want to lock the exact beta version and avoid future beta or stable upgrades.packages/ui/src/styles/globals.css (1)
122-141: Global selection + scrollbar styles — minor a11y nits.
- 4px scrollbar is quite thin; consider 6–8px for accessibility.
- Consider adding focus-visible styles to complement selection changes where relevant.
- ::-webkit-scrollbar { - width: 4px; - } + ::-webkit-scrollbar { + width: 6px; + }packages/ui/src/components/button.tsx (1)
19-20: Align “link” variant with text-link expectations (optional).If “link” should look like inline text, drop button sizing and add better focus/underline affordances.
- link: 'text-foreground hover:underline', + link: 'text-foreground p-0 h-auto underline-offset-4 hover:underline focus-visible:underline',apps/web/components/project/claim-project-dialog.tsx (1)
69-92: Fix analytics event for GitLab caseEvent name is always GitHub-specific; make it conditional by provider.
- <Link - href="/login" - event="claim_project_dialog_connect_github_button_clicked" - {...props} - > + <Link + href="/login" + event={ + provider === 'github' + ? 'claim_project_dialog_connect_github_button_clicked' + : 'claim_project_dialog_connect_gitlab_button_clicked' + } + {...props} + >apps/web/app/(public)/launches/[id]/components/launch-comments.tsx (1)
49-56: Add analytics event and encode redirect- <Link href={`/login?redirect=/launches/${projectId}`} {...props}> + <Link + href={`/login?redirect=${encodeURIComponent(`/launches/${projectId}`)}`} + event="launch_comments_login_click" + {...props} + >apps/web/components/layout/public-nav.tsx (1)
28-43: Stabilize event names for analyticsLabels may include spaces/symbols; normalize for consistent tracking keys.
- event={`${item.label.toLowerCase()}_nav_click`} + event={ + item.event ?? + `${item.label.toLowerCase().replace(/\s+/g, '_').replace(/[^a-z0-9_]/g, '')}_nav_click` + }apps/web/app/(public)/launches/[id]/components/launch-sidebar.tsx (1)
69-98: Good guardrails on queriesConditional enablement for repo queries prevents invalid calls. Consider modest cache to reduce refetch churn.
{ - enabled: isValidRepoUrl && isValidGitHost, - retry: false, + enabled: isValidRepoUrl && isValidGitHost, + retry: false, + staleTime: 60_000, },{ - enabled: !!repoQuery.data && isValidRepoUrl && isValidGitHost, - retry: false, + enabled: !!repoQuery.data && isValidRepoUrl && isValidGitHost, + retry: false, + staleTime: 60_000, },apps/web/components/layout/temp-nav.tsx (1)
19-30: Add aria-labels to icon-only links; align stylingImprove a11y and keep icon buttons visually consistent.
@@ - <Button + <Button variant="ghost" size="icon" - className="rounded-none text-neutral-400 hover:text-white" + className="rounded-none text-neutral-400 hover:text-white" render={(props) => ( <Link href="https://l.oss.now/discord/" target="_blank" rel="noopener noreferrer" event="discord_nav_click" + aria-label="Open Discord" {...props} > <Icons.discord className="size-4 fill-white sm:size-5" /> </Link> )} /> @@ - <Button + <Button variant="ghost" size="icon" - className="rounded-none" + className="rounded-none text-neutral-400 hover:text-white" render={(props) => ( <Link href="https://l.oss.now/gh/" target="_blank" rel="noopener noreferrer" event="github_nav_click" + aria-label="Open GitHub" {...props} > <Icons.github className="size-4 sm:size-5" /> </Link> )} /> @@ - <Button + <Button variant="ghost" size="icon" - className="rounded-none text-neutral-400 hover:text-white" + className="rounded-none text-neutral-400 hover:text-white" render={(props) => ( <Link href="https://l.oss.now/x/" target="_blank" rel="noopener noreferrer" event="twitter_nav_click" + aria-label="Open X (Twitter)" {...props} > <Icons.twitter className="size-4 sm:size-5" /> </Link> )} />Also applies to: 47-62, 67-78
apps/web/app/layout.tsx (2)
58-76: Place TempNav before main content for correct reading/tab order.
Rendering TempNav after Footer places it late in the DOM, which can hurt a11y and screen-reader navigation. Suggest moving it before children.Apply this diff:
- <Providers> - {children} - <Footer /> - - <TempNav /> - - <Databuddy + <Providers> + <TempNav /> + {children} + <Footer /> + + <Databuddy clientId={env.DATABUDDY_CLIENT_ID} enableBatching={true} trackErrors trackOutgoingLinks /> <Toaster /> </Providers>
58-76: Background duplication between body and .root.
body sets a hard-coded bg while .root applies bg via tokens. Consider relying on one source (prefer tokens) to avoid inconsistencies.apps/web/app/(admin)/admin/projects/[id]/edit/page.tsx (2)
41-53: Naming nit: function used like a hook; consider renaming for clarityProjectEditForm returns hook-like state/handlers and is invoked as a function. Consider renaming to useProjectEditForm to match React conventions and improve readability.
117-121: Minor prop order nit to avoid accidental href overridesSafer to spread props before setting the constant href so a stray href in props can’t override your target.
-<Link href="/admin/projects" {...props}> +<Link {...props} href="/admin/projects">Apply in all three renderers.
Also applies to: 137-141, 197-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
apps/web/app/(admin)/admin/layout.tsx(1 hunks)apps/web/app/(admin)/admin/projects/[id]/edit/page.tsx(5 hunks)apps/web/app/(public)/launches/[id]/components/launch-comments.tsx(2 hunks)apps/web/app/(public)/launches/[id]/components/launch-sidebar.tsx(2 hunks)apps/web/app/(public)/page.tsx(1 hunks)apps/web/app/layout.tsx(2 hunks)apps/web/components/layout/public-nav.tsx(2 hunks)apps/web/components/layout/temp-nav.tsx(1 hunks)apps/web/components/project/claim-project-dialog.tsx(1 hunks)apps/web/components/user/profile-tabs.tsx(1 hunks)apps/web/components/user/profile.tsx(1 hunks)package.json(1 hunks)packages/ui/components.json(1 hunks)packages/ui/package.json(1 hunks)packages/ui/src/components/button.tsx(1 hunks)packages/ui/src/styles/globals.css(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
apps/web/components/user/profile-tabs.tsx (1)
packages/ui/src/components/link.tsx (1)
Link(16-42)
apps/web/components/project/claim-project-dialog.tsx (1)
packages/ui/src/components/link.tsx (1)
Link(16-42)
apps/web/components/layout/public-nav.tsx (2)
packages/ui/src/components/link.tsx (1)
Link(16-42)packages/ui/src/lib/utils.ts (1)
cn(4-6)
apps/web/app/(public)/launches/[id]/components/launch-comments.tsx (2)
apps/web/components/comments/reply-form.tsx (1)
ReplyForm(35-180)packages/ui/src/components/link.tsx (1)
Link(16-42)
apps/web/app/(public)/page.tsx (1)
packages/ui/src/components/link.tsx (1)
Link(16-42)
apps/web/app/(admin)/admin/layout.tsx (1)
packages/ui/src/components/link.tsx (1)
Link(16-42)
apps/web/app/(public)/launches/[id]/components/launch-sidebar.tsx (1)
packages/ui/src/components/link.tsx (1)
Link(16-42)
apps/web/app/layout.tsx (4)
apps/web/components/providers.tsx (1)
Providers(8-23)apps/web/components/layout/footer.tsx (1)
Footer(26-96)apps/web/components/layout/temp-nav.tsx (1)
TempNav(5-81)packages/env/src/server.ts (1)
env(4-32)
apps/web/app/(admin)/admin/projects/[id]/edit/page.tsx (2)
apps/web/types/project.ts (1)
isProject(111-158)packages/ui/src/components/link.tsx (1)
Link(16-42)
apps/web/components/layout/temp-nav.tsx (1)
packages/ui/src/components/link.tsx (1)
Link(16-42)
apps/web/components/user/profile.tsx (1)
packages/ui/src/components/link.tsx (1)
Link(16-42)
packages/ui/src/components/button.tsx (1)
packages/ui/src/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (13)
package.json (1)
36-36: Confirmed shadcn CLI v3 support for “registries”. npm view shadcn version returned 3.0.0 and packages/ui/components.json contains the “registries” field.packages/ui/components.json (1)
20-22: LGTM: 9ui registry configured.The custom registry entry is correct and aligns with the CLI v3 capabilities.
apps/web/app/(public)/launches/[id]/components/launch-comments.tsx (2)
3-4: Imports reorg looks goodAlso applies to: 8-8
42-42: ReplyForm usage LGTMapps/web/components/layout/public-nav.tsx (1)
56-61: Login link migration LGTMapps/web/app/(public)/launches/[id]/components/launch-sidebar.tsx (2)
33-33: Importing isValidProvider is appropriate
384-393: “View Project” button render-prop migration LGTMapps/web/components/layout/temp-nav.tsx (1)
31-46: Duplicate Discord button – confirm intentThere are two Discord buttons back-to-back. If unintentional, remove the duplicate.
- <Button - variant="ghost" - size="icon" - className="rounded-none" - render={(props) => ( - <Link - href="https://l.oss.now/discord/" - target="_blank" - rel="noopener noreferrer" - event="discord_nav_click" - {...props} - > - <Icons.discord className="size-4 fill-white sm:size-5" /> - </Link> - )} - />apps/web/app/layout.tsx (2)
4-4: Import looks good.
Aligned with TempNav usage elsewhere.
68-75: Verify duplicate outbound-link tracking
Databuddy’strackOutgoingLinksauto-captures link clicks and ourLinkcomponent’s manualtrack(...)inlink.tsxwill fire as well—resulting in two events per outbound click. Confirm whether to disable one or adjust event names to avoid duplicates.apps/web/components/user/profile-tabs.tsx (1)
623-638: LGTM on render-prop migration.
Props are correctly forwarded; rel noopener is included; aria-label/title present.apps/web/app/(public)/page.tsx (1)
36-42: LGTM on Button.render conversion.
Clean migration; props forwarded correctly.apps/web/app/(admin)/admin/projects/[id]/edit/page.tsx (1)
1-233: Sanity check: verify Button migration across TSX files
Use ripgrep with globs to catch any staleasChildusages or direct Link wrapping:rg -nP -g '*.tsx' '\basChild\b' rg -nP -g '*.tsx' '<Button[^>]*\brender\s*=' rg -nP -g '*.tsx' '<Button[^>]*>\s*<(?:Next)?Link\b' -C2
| <Button | ||
| className="mt-4" | ||
| render={(props) => ( | ||
| <Link href="/admin/projects" {...props}> | ||
| Back to Projects | ||
| </Link> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Button render + Link must forward ref to avoid broken focus/disabled semantics
9ui’s Button render prop will pass a ref and stateful attrs (e.g., data-disabled). Your Link component (packages/ui/src/components/link.tsx) is a function component without forwardRef, so the ref is dropped. This can break focus rings, keyboard focus management, and disabled behavior, and may emit React warnings. Convert Link to forwardRef and pass the ref to NextLink’s underlying anchor.
Update Link (outside this file) along these lines:
import NextLink from 'next/link';
import * as React from 'react';
import { cn } from '../lib/cn'; // adjust import to your utils
type Props = React.ComponentPropsWithoutRef<typeof NextLink> & {
event?: string;
eventObject?: Record<string, unknown>;
};
const Link = React.forwardRef<HTMLAnchorElement, Props>(function Link(
{ className, href, children, event, eventObject, onClick, ...props },
ref,
) {
return (
<NextLink
href={href}
ref={ref}
className={cn(className)}
{...props}
onClick={(e) => {
if (event) {
track(event, eventObject);
}
onClick?.(e);
}}
>
{children}
</NextLink>
);
});
export default Link;Also applies to: 132-142, 193-201
🏁 Script executed:
#!/bin/bash
# Search for Link component files in the UI package
echo "Searching for link.tsx files..."
find packages/ui -type f -iname "link.tsx"
# Inspect the first Link component found
LINK_FILE=$(find packages/ui -type f -iname "link.tsx" | head -n1)
if [ -n "$LINK_FILE" ]; then
echo "---- $LINK_FILE (first 200 lines) ----"
sed -n '1,200p' "$LINK_FILE"
echo "---- Checking for forwardRef usage ----"
rg -n "forwardRef" -C3 "$LINK_FILE" || echo "No forwardRef usage found in $LINK_FILE"
else
echo "No Link component file found"
fi
# Search for Button component files in the UI package
echo "Searching for button component files..."
BUTTON_FILES=$(find packages/ui -type f -iname "button*.tsx")
if [ -n "$BUTTON_FILES" ]; then
for file in $BUTTON_FILES; do
echo "---- $file (first 200 lines) ----"
sed -n '1,200p' "$file"
echo "---- Searching for render prop usage in $file ----"
rg -n "render" -C3 "$file" || echo "No render usage found in $file"
done
else
echo "No Button component files found"
fiLength of output: 5319
Forward Link ref to support Button render
Wrap Link (packages/ui/src/components/link.tsx) in React.forwardRef<HTMLAnchorElement, Props> and forward the passed-in ref to <NextLink> so Button’s render-prop ref and data-disabled attributes aren’t dropped. Apply the same change for all other Button render usages (lines 115–122, 132–142, 193–201).
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| render={(props) => ( | ||
| <Link | ||
| href={pr.url} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| aria-label="Open pull request in a new tab" | ||
| title="Open pull request in a new tab" | ||
| {...props} | ||
| > | ||
| <ExternalLink className="h-4 w-4" /> | ||
| </Link> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Forward ref support for Link when used via Button.render.
Base-UI Buttons often pass refs to the rendered element. Our Link component isn’t ref-forwarding, which can break focus styles and a11y. Update Link to forwardRef.
Apply to packages/ui/src/components/link.tsx:
-import NextLink from "next/link"
-import { cn } from "../lib/utils"
-
-export default function Link({
- className,
- href,
- children,
- event,
- eventObject,
- onClick,
- ...props
-}: Props) {
- return (
- <NextLink
- href={href}
- className={cn(className)}
- {...props}
- onClick={(e) => {
- if (event) {
- track(event, eventObject);
- }
- if (onClick) {
- onClick(e);
- }
- }}
- >
- {children}
- </NextLink>
- );
-}
+import * as React from "react"
+import NextLink from "next/link"
+import { cn } from "../lib/utils"
+
+const Link = React.forwardRef<HTMLAnchorElement, Props>(
+ ({ className, href, children, event, eventObject, onClick, ...props }, ref) => {
+ return (
+ <NextLink
+ href={href}
+ className={cn(className)}
+ ref={ref}
+ {...props}
+ onClick={(e) => {
+ if (event) track(event, eventObject);
+ onClick?.(e);
+ }}
+ >
+ {children}
+ </NextLink>
+ );
+ },
+);
+export default Link;📝 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.
| <Button | |
| variant="ghost" | |
| size="sm" | |
| render={(props) => ( | |
| <Link | |
| href={pr.url} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| aria-label="Open pull request in a new tab" | |
| title="Open pull request in a new tab" | |
| {...props} | |
| > | |
| <ExternalLink className="h-4 w-4" /> | |
| </Link> | |
| )} | |
| /> | |
| import * as React from "react" | |
| import NextLink from "next/link" | |
| import { cn } from "../lib/utils" | |
| const Link = React.forwardRef<HTMLAnchorElement, Props>( | |
| ({ className, href, children, event, eventObject, onClick, ...props }, ref) => { | |
| return ( | |
| <NextLink | |
| href={href} | |
| className={cn(className)} | |
| ref={ref} | |
| {...props} | |
| onClick={(e) => { | |
| if (event) track(event, eventObject); | |
| onClick?.(e); | |
| }} | |
| > | |
| {children} | |
| </NextLink> | |
| ); | |
| }, | |
| ); | |
| export default Link; |
| <Button | ||
| variant="ghost" | ||
| size="icon-sm" | ||
| render={(props) => ( | ||
| <Link | ||
| href={`https://${profile?.git?.provider}.com/${profile?.git?.login}`} | ||
| target="_blank" | ||
| {...props} | ||
| > | ||
| {profile?.git?.provider === 'github' ? ( | ||
| <Icons.github className="size-4" /> | ||
| ) : ( | ||
| <Icons.gitlab className="size-4" /> | ||
| )} | ||
| </Link> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to external link.
target="_blank" without rel is a tabnabbing risk.
Apply this diff:
- <Link
- href={`https://${profile?.git?.provider}.com/${profile?.git?.login}`}
- target="_blank"
- {...props}
- >
+ <Link
+ href={`https://${profile?.git?.provider}.com/${profile?.git?.login}`}
+ target="_blank"
+ rel="noopener noreferrer"
+ {...props}
+ >📝 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.
| <Button | |
| variant="ghost" | |
| size="icon-sm" | |
| render={(props) => ( | |
| <Link | |
| href={`https://${profile?.git?.provider}.com/${profile?.git?.login}`} | |
| target="_blank" | |
| {...props} | |
| > | |
| {profile?.git?.provider === 'github' ? ( | |
| <Icons.github className="size-4" /> | |
| ) : ( | |
| <Icons.gitlab className="size-4" /> | |
| )} | |
| </Link> | |
| )} | |
| /> | |
| <Button | |
| variant="ghost" | |
| size="icon-sm" | |
| render={(props) => ( | |
| <Link | |
| href={`https://${profile?.git?.provider}.com/${profile?.git?.login}`} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| {...props} | |
| > | |
| {profile?.git?.provider === 'github' ? ( | |
| <Icons.github className="size-4" /> | |
| ) : ( | |
| <Icons.gitlab className="size-4" /> | |
| )} | |
| </Link> | |
| )} | |
| /> |
🤖 Prompt for AI Agents
In apps/web/components/user/profile.tsx around lines 166 to 182, the external
Link rendered inside Button uses target="_blank" but lacks rel="noopener
noreferrer"; add rel="noopener noreferrer" to the Link props (i.e., pass
rel="noopener noreferrer" along with target and {...props}) to mitigate
tabnabbing risk.
|
|
||
| const buttonVariants = cva( | ||
| "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-[color,box-shadow] disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0 outline-none focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px] aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive", | ||
| "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all duration-200 outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50 disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make “disabled” styling work for non-button renders (e.g., Link).
When rendering an anchor, the CSS “disabled:” variants won’t apply. Add data-attribute fallbacks to base classes and keep them in default props.
- "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all duration-200 outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50 disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0",
+ "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all duration-200 outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50 disabled:pointer-events-none disabled:opacity-50 data-[disabled]:pointer-events-none data-[disabled]:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0",📝 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.
| "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all duration-200 outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50 disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0", | |
| "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all duration-200 outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50 disabled:pointer-events-none disabled:opacity-50 data-[disabled]:pointer-events-none data-[disabled]:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0", |
🤖 Prompt for AI Agents
In packages/ui/src/components/button.tsx around line 9, the current class string
only uses the "disabled:" Tailwind variants which don't apply to non-button
elements like anchors; update the base class string to include data-attribute
fallbacks (e.g. add selectors targeting [data-disabled] alongside the disabled:
variants) and ensure the Button component sets the data-disabled attribute when
the disabled prop is true (and preserves it for non-button renders via default
props or element-specific props) so disabled styling works for anchors/links as
well.
| function Button({ className, variant, size, render = <button />, ...props }: ButtonProps) { | ||
| const defaultProps = { | ||
| 'data-slot': 'button', | ||
| className: cn(buttonVariants({ variant, size, className })), | ||
| } as const; | ||
|
|
||
| const element = useRender({ | ||
| render, | ||
| props: mergeProps<'button'>(defaultProps, props), | ||
| }); | ||
|
|
||
| return ( | ||
| <Comp | ||
| data-slot="button" | ||
| className={cn(buttonVariants({ variant, size, className }))} | ||
| {...props} | ||
| /> | ||
| ); | ||
| return element; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Forward ref and set safe default type.
Refs won’t reach the inner element without forwardRef, and defaulting type avoids unintended form submits.
-function Button({ className, variant, size, render = <button />, ...props }: ButtonProps) {
- const defaultProps = {
- 'data-slot': 'button',
- className: cn(buttonVariants({ variant, size, className })),
- } as const;
-
- const element = useRender({
- render,
- props: mergeProps<'button'>(defaultProps, props),
- });
-
- return element;
-}
+const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
+ ({ className, variant, size, render = <button />, disabled, type, ...props }, ref) => {
+ const defaultProps = {
+ 'data-slot': 'button',
+ // data/aria disabled also handled below for non-button renders
+ 'aria-disabled': disabled ? true : undefined,
+ 'data-disabled': disabled ? '' : undefined,
+ className: cn(buttonVariants({ variant, size, className })),
+ } as const;
+
+ const element = useRender({
+ render,
+ props: mergeProps<'button'>(
+ defaultProps,
+ { ...props, disabled, type: type ?? 'button', ref }
+ ),
+ });
+
+ return element;
+ }
+);
+Button.displayName = 'Button';📝 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.
| function Button({ className, variant, size, render = <button />, ...props }: ButtonProps) { | |
| const defaultProps = { | |
| 'data-slot': 'button', | |
| className: cn(buttonVariants({ variant, size, className })), | |
| } as const; | |
| const element = useRender({ | |
| render, | |
| props: mergeProps<'button'>(defaultProps, props), | |
| }); | |
| return ( | |
| <Comp | |
| data-slot="button" | |
| className={cn(buttonVariants({ variant, size, className }))} | |
| {...props} | |
| /> | |
| ); | |
| return element; | |
| } | |
| const Button = React.forwardRef<HTMLButtonElement, ButtonProps>( | |
| ({ className, variant, size, render = <button />, disabled, type, ...props }, ref) => { | |
| const defaultProps = { | |
| 'data-slot': 'button', | |
| // data/aria disabled also handled below for non-button renders | |
| 'aria-disabled': disabled ? true : undefined, | |
| 'data-disabled': disabled ? '' : undefined, | |
| className: cn(buttonVariants({ variant, size, className })), | |
| } as const; | |
| const element = useRender({ | |
| render, | |
| props: mergeProps<'button'>( | |
| defaultProps, | |
| { ...props, disabled, type: type ?? 'button', ref } | |
| ), | |
| }); | |
| return element; | |
| } | |
| ); | |
| Button.displayName = 'Button'; |
🤖 Prompt for AI Agents
In packages/ui/src/components/button.tsx around lines 44 to 56, the Button
component currently doesn't forward refs to its inner element and lacks a safe
default button type, which can prevent parent refs from reaching the DOM node
and cause unintended form submissions; wrap the component in React.forwardRef,
accept a ref parameter and pass it through to the rendered element (ensure
mergeProps includes the ref), add a default type: 'button' to defaultProps so
the element won't submit forms by default, and update the ButtonProps/type
signature to allow a forwarded ref and the render polymorphism accordingly
before exporting the forwarded component.
1 participant
Description
Type of Change
Testing
Checklist
Summary by CodeRabbit