Fix/remove links legacybehavior #5
Conversation
…ut legacyBehavior - Button.tsx: Only pass ref to button element, not to Link (Link manages its own anchor) - Dropdown.tsx: Strip ref from props when using Link to avoid type incompatibility This fixes the type errors that were causing API V1 and V2 builds to fail. Co-Authored-By: Volnei Munhoz <[email protected]>
Link component doesn't accept disabled or type props, so these should only be passed when rendering a button element. Co-Authored-By: Volnei Munhoz <[email protected]>
The passThroughProps spread was including a ref property that's incompatible with Link's expected ref type. This strips the ref when rendering a Link component. Co-Authored-By: Volnei Munhoz <[email protected]>
… types The Button component uses a union type for props that can be either Link or button props. TypeScript can't narrow the union type properly when using React.createElement with a dynamic element type, so we use a type assertion to cast the props to the correct type. Co-Authored-By: Volnei Munhoz <[email protected]>
Co-Authored-By: Volnei Munhoz <[email protected]>
Co-Authored-By: Volnei Munhoz <[email protected]>
….devin.ai/proxy/github.com/calcom/cal.com into fix/remove-links-legacybehavior
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
@refacto-visz |
WalkthroughThis pull request refactors Next.js Link usage patterns across multiple UI components and application modules. The changes remove legacy Link behaviors (legacyBehavior, passHref) and simplify navigation wrapper structures. Several components are updated to render Links and buttons through explicit conditional branches instead of dynamic element creation. The Button component now accepts href props directly for navigation. Link elements are repositioned from wrapper contexts to direct usage. Navigation destinations and overall component behaviors remain unchanged across all modifications. Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on modernizing the application's navigation by updating all instances of Next.js Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
codeant-ai
bot
added
the
size:L
🔒 Limited Access – You're on the free plan.🚀 Upgrade to Pro and unlock:
---### Refacto PR Summary Key Changes:
Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant App as Application
participant Router as Next.js Router
participant UI as UI Components
Dev->>App: Click navigation element
App->>UI: Render Link component
UI->>UI: Check if href prop exists
alt Has href (Link)
UI->>Router: Navigate with Link
Router-->>App: Route change
else No href (Button)
UI->>App: Execute onClick handler
App-->>UI: Handle button action
end
App-->>Dev: Updated UI state
Testing GuideClick to expand
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/components/list/List.tsx (1)
59-65: Critical: Nested anchors created when href is provided.When
hrefis provided:
- Line 40 sets
elementType = "a"- Lines 42-57 create an
<a>element viacreateElement- Line 60 wraps that anchor in
<Link>In Next.js 15, this produces nested anchors:
<a><a>...</a></a>, which is invalid HTML and breaks navigation.Apply this diff to fix:
export function ListItem(props: ListItemProps) { const { href, expanded, rounded = true, ...passThroughProps } = props; - const elementType = href ? "a" : "li"; + const elementType = href ? "div" : "li"; const element = createElement( elementType,By changing to
"div", the Link will wrap a div instead of an anchor, preventing the nesting issue. The Link itself renders as an anchor in Next.js 15.
🧹 Nitpick comments (2)
packages/app-store/_components/AppNotInstalledMessage.tsx (1)
5-5: Consider using a named export instead of default export.Named exports provide better tree-shaking, easier refactoring, and clearer imports.
As per coding guidelines.
Apply this diff:
-export default function AppNotInstalledMessage({ appName }: { appName: string }) { +export function AppNotInstalledMessage({ appName }: { appName: string }) {apps/web/components/apps/make/Setup.tsx (1)
73-73: Consider using Next.js Image component for better optimization.While the ESLint disable allows the img tag, consider using
next/imagefor automatic optimization, lazy loading, and proper sizing.If the SVG endpoint supports it, replace with:
<Image src="/api/app-store/make/icon.svg" alt="Make Logo" width={44} height={44} />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/components/apps/make/Setup.tsx(2 hunks)apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx(1 hunks)apps/web/modules/bookings/views/bookings-single-view.tsx(2 hunks)packages/app-store/_components/AppNotInstalledMessage.tsx(1 hunks)packages/ui/components/button/Button.tsx(2 hunks)packages/ui/components/dropdown/Dropdown.tsx(1 hunks)packages/ui/components/form/step/Stepper.tsx(1 hunks)packages/ui/components/list/List.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/ui/components/form/step/Stepper.tsxpackages/app-store/_components/AppNotInstalledMessage.tsxpackages/ui/components/dropdown/Dropdown.tsxapps/web/modules/bookings/views/bookings-single-view.tsxpackages/ui/components/button/Button.tsxapps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsxapps/web/components/apps/make/Setup.tsxpackages/ui/components/list/List.tsx
🧬 Code graph analysis (1)
packages/app-store/_components/AppNotInstalledMessage.tsx (1)
packages/ui/components/button/Button.tsx (1)
Button(221-361)
🔇 Additional comments (8)
apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx (1)
48-52: LGTM! Link refactor aligns with Next.js 15.The change from a wrapper pattern to direct Link usage with inline styling is correct and follows Next.js 15's simplified Link API.
packages/app-store/_components/AppNotInstalledMessage.tsx (1)
18-21: LGTM! Button with href correctly replaces Link wrapper.The Button component properly handles href navigation internally (verified in Button.tsx), making the Link wrapper unnecessary.
packages/ui/components/form/step/Stepper.tsx (1)
29-61: LGTM! Stepper refactor correctly applies styles directly to Link.The change from a single Link wrapper with conditional content to three separate Link branches is clean and maintains the same navigation behavior. Each Link properly wraps span elements (not anchors), avoiding nested anchor issues.
packages/ui/components/dropdown/Dropdown.tsx (1)
144-154: LGTM! Clean refactor with proper ref handling.The explicit ref stripping when using Link is correct since Link manages its own anchor element. The added comment clearly documents this behavior.
apps/web/components/apps/make/Setup.tsx (1)
137-139: LGTM! Button with href correctly replaces Link wrapper.Using Button's built-in href support simplifies the code while maintaining the same navigation behavior.
apps/web/modules/bookings/views/bookings-single-view.tsx (2)
809-816: LGTM! Simplified Link structure maintains functionality.Removing the span wrapper and applying styles directly to Link is cleaner. The
data-testidis correctly moved to the Link element.
839-848: LGTM! Consistent Link refactor.Same clean pattern as the login link—removes wrapper and applies styles directly to Link while preserving the test identifier.
packages/ui/components/button/Button.tsx (1)
242-361: LGTM! Excellent refactor—clearer separation of Link vs button rendering.The explicit branching for Link and button cases improves type safety and readability. Key improvements:
- Reusable
buttonContenteliminates duplication- Proper ref handling (Link manages its own anchor, button receives forwardedRef)
- Explicit disabled state that includes loading
- Clean type casts avoid prop conflicts
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| </Link> | ||
| </span> | ||
| <Link | ||
| href={`/auth/login?callbackUrl=${encodeURIComponent( |
There was a problem hiding this comment.
Suggestion: The login link's callback URL always points to /booking/{uid} and ignores the current query string (e.g. ?cancel=true), so after logging in the user is redirected to the generic booking page instead of back to the exact state they were on, contradicting the "redirect to here" comment and breaking flows that rely on those query parameters. [logic error]
Severity Level: Minor
| href={`/auth/login?callbackUrl=${encodeURIComponent( | |
| `${pathname}${searchParams?.toString() ? `?${searchParams.toString()}` : ""}` |
Why it matters? ⭐
The PR currently encodes a callbackUrl that always points to /booking/${bookingInfo?.uid} and discards the current pathname/search params. The comment above the link explicitly says "redirect to here" — preserving the current query string (e.g. cancel/reschedule flags, rescheduledBy, seatReferenceUid) is necessary to restore the same UI state after login. The proposed change (encoding pathname + search params) fixes a real UX bug rather than a stylistic nit.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/modules/bookings/views/bookings-single-view.tsx
**Line:** 810:810
**Comment:**
*Logic Error: The login link's callback URL always points to `/booking/{uid}` and ignores the current query string (e.g. `?cancel=true`), so after logging in the user is redirected to the generic booking page instead of back to the exact state they were on, contradicting the "redirect to here" comment and breaking flows that rely on those query parameters.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| className="underline" | ||
| data-testid="reschedule-link"> | ||
| {t("login")} | ||
| </Link> |
There was a problem hiding this comment.
Suggestion: The login link is marked with data-testid="reschedule-link", which semantically refers to the reschedule action; this conflates two different actions under the same test identifier and will cause automated tests or future code that target the reschedule control by test id to mistakenly interact with the login link instead. [possible bug]
Severity Level: Critical 🚨
| className="underline" | |
| data-testid="reschedule-link"> | |
| {t("login")} | |
| </Link> | |
| data-testid="login-link"> |
Why it matters? ⭐
The login Link introduced in the requiresLoginToUpdate block reuses data-testid="reschedule-link". That identifier is also used for the real reschedule control elsewhere in this component. Reusing the same test id for two different actions will cause brittle or incorrect automated tests (and makes it harder to target elements reliably). Renaming the login link's test id to something like "login-link" or "login-redirect" is a small, sensible change that prevents test collisions.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/modules/bookings/views/bookings-single-view.tsx
**Line:** 813:816
**Comment:**
*Possible Bug: The login link is marked with `data-testid="reschedule-link"`, which semantically refers to the reschedule action; this conflates two different actions under the same test identifier and will cause automated tests or future code that target the reschedule control by test id to mistakenly interact with the login link instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| {index + 1 < props.step ? ( | ||
| <Link | ||
| href={props.disableSteps ? "#" : `${href}?step=${index + 1}`} | ||
| shallow | ||
| replace | ||
| className="hover:bg-inverted block h-2.5 w-2.5 rounded-full bg-gray-600"> | ||
| <span className="sr-only">{mapStep.title}</span> | ||
| </Link> | ||
| ) : index + 1 === props.step ? ( | ||
| <Link | ||
| href={props.disableSteps ? "#" : `${href}?step=${index + 1}`} | ||
| shallow | ||
| replace | ||
| className="relative flex items-center justify-center" | ||
| aria-current="step"> | ||
| <span className="absolute flex h-5 w-5 p-px" aria-hidden="true"> | ||
| <span className="bg-emphasis h-full w-full rounded-full" /> | ||
| </span> | ||
| <span | ||
| className="relative block h-2.5 w-2.5 rounded-full bg-gray-600" | ||
| aria-hidden="true" | ||
| /> | ||
| <span className="sr-only">{mapStep.title}</span> | ||
| </Link> | ||
| ) : ( | ||
| <Link | ||
| href={props.disableSteps ? "#" : `${href}?step=${index + 1}`} |
There was a problem hiding this comment.
Suggestion: When the flag to disable steps is set, the indicators are still rendered as clickable links pointing to "#", so users can still click them and cause hash navigation/page jumps instead of being truly disabled; rendering non-interactive elements when steps are disabled avoids this incorrect behavior. [logic error]
Severity Level: Minor
| {index + 1 < props.step ? ( | |
| <Link | |
| href={props.disableSteps ? "#" : `${href}?step=${index + 1}`} | |
| shallow | |
| replace | |
| className="hover:bg-inverted block h-2.5 w-2.5 rounded-full bg-gray-600"> | |
| <span className="sr-only">{mapStep.title}</span> | |
| </Link> | |
| ) : index + 1 === props.step ? ( | |
| <Link | |
| href={props.disableSteps ? "#" : `${href}?step=${index + 1}`} | |
| shallow | |
| replace | |
| className="relative flex items-center justify-center" | |
| aria-current="step"> | |
| <span className="absolute flex h-5 w-5 p-px" aria-hidden="true"> | |
| <span className="bg-emphasis h-full w-full rounded-full" /> | |
| </span> | |
| <span | |
| className="relative block h-2.5 w-2.5 rounded-full bg-gray-600" | |
| aria-hidden="true" | |
| /> | |
| <span className="sr-only">{mapStep.title}</span> | |
| </Link> | |
| ) : ( | |
| <Link | |
| href={props.disableSteps ? "#" : `${href}?step=${index + 1}`} | |
| {props.disableSteps ? ( | |
| index + 1 === props.step ? ( | |
| <span className="relative flex items-center justify-center" aria-current="step"> | |
| <span className="absolute flex h-5 w-5 p-px" aria-hidden="true"> | |
| <span className="bg-emphasis h-full w-full rounded-full" /> | |
| </span> | |
| <span | |
| className="relative block h-2.5 w-2.5 rounded-full bg-gray-600" | |
| aria-hidden="true" | |
| /> | |
| <span className="sr-only">{mapStep.title}</span> | |
| </span> | |
| ) : ( | |
| <span | |
| className={ | |
| index + 1 < props.step | |
| ? "hover:bg-inverted block h-2.5 w-2.5 rounded-full bg-gray-600" | |
| : "bg-emphasis block h-2.5 w-2.5 rounded-full hover:bg-gray-400" | |
| }> | |
| <span className="sr-only">{mapStep.title}</span> | |
| </span> | |
| ) | |
| ) : index + 1 < props.step ? ( | |
| <Link | |
| href={`${href}?step=${index + 1}`} | |
| shallow | |
| replace | |
| className="hover:bg-inverted block h-2.5 w-2.5 rounded-full bg-gray-600"> | |
| <span className="sr-only">{mapStep.title}</span> | |
| </Link> | |
| ) : index + 1 === props.step ? ( | |
| <Link | |
| href={`${href}?step=${index + 1}`} | |
| shallow | |
| replace | |
| className="relative flex items-center justify-center" | |
| aria-current="step"> | |
| <span className="absolute flex h-5 w-5 p-px" aria-hidden="true"> | |
| <span className="bg-emphasis h-full w-full rounded-full" /> | |
| </span> | |
| <span | |
| className="relative block h-2.5 w-2.5 rounded-full bg-gray-600" | |
| aria-hidden="true" | |
| /> | |
| <span className="sr-only">{mapStep.title}</span> | |
| </Link> | |
| ) : ( | |
| <Link | |
| href={`${href}?step=${index + 1}`} |
Why it matters? ⭐
The PR currently conditionally sets href to "#" when props.disableSteps is true, but still renders an interactive Link. That allows users to click and change the URL hash (or cause focus/navigation), which is behaviour the flag intends to prevent. Replacing Link with non-interactive spans (or disabling pointer events / removing href) when disableSteps is true fixes a real UX bug and avoids accidental navigation. This is a relevant, non-cosmetic correctness fix scoped to this component.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/ui/components/form/step/Stepper.tsx
**Line:** 29:55
**Comment:**
*Logic Error: When the flag to disable steps is set, the indicators are still rendered as clickable links pointing to "#", so users can still click them and cause hash navigation/page jumps instead of being truly disabled; rendering non-interactive elements when steps are disabled avoids this incorrect behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully removes legacyBehavior from next/link components across the codebase, which is a great step towards modernizing the Next.js usage. The changes in the Button, Dropdown, and List components are well-implemented to encapsulate link behavior. The other files are updated accordingly to use these refactored components. The code is cleaner and follows current Next.js best practices. I have one suggestion to reduce code duplication in the Stepper component for better maintainability.
| {index + 1 < props.step ? ( | ||
| <Link | ||
| href={props.disableSteps ? "#" : `${href}?step=${index + 1}`} | ||
| shallow | ||
| replace | ||
| className="hover:bg-inverted block h-2.5 w-2.5 rounded-full bg-gray-600"> | ||
| <span className="sr-only">{mapStep.title}</span> | ||
| </Link> | ||
| ) : index + 1 === props.step ? ( | ||
| <Link | ||
| href={props.disableSteps ? "#" : `${href}?step=${index + 1}`} | ||
| shallow | ||
| replace | ||
| className="relative flex items-center justify-center" | ||
| aria-current="step"> | ||
| <span className="absolute flex h-5 w-5 p-px" aria-hidden="true"> | ||
| <span className="bg-emphasis h-full w-full rounded-full" /> | ||
| </span> | ||
| <span | ||
| className="relative block h-2.5 w-2.5 rounded-full bg-gray-600" | ||
| aria-hidden="true" | ||
| /> | ||
| <span className="sr-only">{mapStep.title}</span> | ||
| </Link> | ||
| ) : ( | ||
| <Link | ||
| href={props.disableSteps ? "#" : `${href}?step=${index + 1}`} | ||
| shallow | ||
| replace | ||
| className="bg-emphasis block h-2.5 w-2.5 rounded-full hover:bg-gray-400"> | ||
| <span className="sr-only">{mapStep.title}</span> | ||
| </Link> | ||
| )} |
There was a problem hiding this comment.
The logic for rendering the different states of a stepper item is duplicated across three separate <Link> components. This repetition makes the code harder to read and maintain. Consider refactoring to use a single <Link> component and conditionally determine its className and children based on the step's state. This will make the component more concise and easier to manage.
{(() => {
const isCompleted = index + 1 < props.step;
const isCurrent = index + 1 === props.step;
return (
<Link
href={props.disableSteps ? "#" : `${href}?step=${index + 1}`}
shallow
replace
className={
isCompleted
? "hover:bg-inverted block h-2.5 w-2.5 rounded-full bg-gray-600"
: isCurrent
? "relative flex items-center justify-center"
: "bg-emphasis block h-2.5 w-2.5 rounded-full hover:bg-gray-400"
}
aria-current={isCurrent ? "step" : undefined}>
{isCurrent && (
<>
<span className="absolute flex h-5 w-5 p-px" aria-hidden="true">
<span className="bg-emphasis h-full w-full rounded-full" />
</span>
<span
className="relative block h-2.5 w-2.5 rounded-full bg-gray-600"
aria-hidden="true"
/>
</>
)}
<span className="sr-only">{mapStep.title}</span>
</Link>
);
})()}
|
@refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Next.js Link ModernizationPR Confidence Score: 🟨 4 / 5👍 Well Done
📁 Selected files for review (8)
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| <Link href={href}> | ||
| {element} | ||
| </Link> |
There was a problem hiding this comment.
Invalid Nested Link Tag
The refactor to remove legacyBehavior incorrectly wraps a manually created anchor tag (element) inside the Link component. This results in invalid HTML with nested anchor tags, which can cause unpredictable rendering, styling, accessibility, and routing behavior in browsers.
<Link href={href} {...element.props}>
{element.props.children}
</Link>
Commitable Suggestion
| <Link href={href}> | |
| {element} | |
| </Link> | |
| <Link href={href} {...element.props}> | |
| {element.props.children} | |
| </Link> |
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| if (isLink) { | ||
| return ( | ||
| <Link | ||
| {...(passThroughProps as Omit<JSX.IntrinsicElements["a"], "href" | "onClick" | "ref"> & LinkProps)} | ||
| shallow={shallow && shallow} | ||
| className={buttonClassName} | ||
| onClick={handleClick}> | ||
| {buttonContent} | ||
| </Link> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Broken Ref Forwarding
The component's forwardRef contract promises to forward refs to both button and anchor elements. However, the refactored code only forwards refs when rendering a button, not when rendering a Link component. This breaks the component's contract and will cause refs to be ignored on any Button with an href prop.
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Postcondition-Violation
CodeAnt-AI Description
Remove legacy link behavior and fix link/button prop handling across UI
What Changed
Impact
✅ Fewer TypeScript build failures during API V1/V2 builds✅ Links render with correct attributes and styling (no invalid disabled/type on anchors)✅ More reliable navigation and button behavior across setup, auth, bookings, and list UIs💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.