fix(ui): migrate off Next.js <Link legacyBehavior> — fix hydration & ref-forwarding issues#24418
fix(ui): migrate off Next.js <Link legacyBehavior> — fix hydration & ref-forwarding issues#24418livebitz1 wants to merge 6 commits intocalcom:mainfrom
Conversation
github-actions
bot
added
ui
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
@livebitz1 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis change migrates Next.js Link usage off Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
dosubot
bot
added
the
🧹 Improvements
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/app-store/_components/AppNotInstalledMessage.tsx (1)
19-26: Nested interactive elements violate the PR's accessibility goals.The current implementation wraps a
Buttoninside aLink, creating nested interactive elements (an anchor containing a button). This pattern:
- Violates HTML validity (interactive elements shouldn't be nested)
- Creates accessibility issues for keyboard navigation and screen readers
- Contradicts the PR's stated objective to "prevent nested interactive elements (anchors/buttons inside each other)"
Per the PR summary,
Buttonnow supports anhrefprop that internally renders aLink. Use that pattern instead.Apply this diff:
<div className="mt-5"> - <Link href={`/apps/${appName}`} className="inline-block"> - <Button type="button" color="secondary"> - {t("go_to_app_store")} - <Icon name="arrow-up-right" className="ml-1" size={20} /> - </Button> - </Link> + <Button href={`/apps/${appName}`} type="button" color="secondary"> + {t("go_to_app_store")} + <Icon name="arrow-up-right" className="ml-1" size={20} /> + </Button> </div>apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx (1)
48-54: Remove nested interactive elements (Link > button).The Link wrapping a button element creates nested interactive elements, which violates accessibility guidelines and can break keyboard navigation and screen reader behavior.
Apply this diff to fix the issue by using just the Link element with appropriate styling:
- <Link href="/auth/forgot-password" className="flex w-full justify-center"> - <button - type="button" - className="flex w-full justify-center px-4 py-2 text-sm font-medium text-blue-600 focus:outline-none focus:ring-2 focus:ring-black focus:ring-offset-2"> - {t("try_again")} - </button> - </Link> + <Link + href="/auth/forgot-password" + className="flex w-full justify-center px-4 py-2 text-sm font-medium text-blue-600 focus:outline-none focus:ring-2 focus:ring-black focus:ring-offset-2"> + {t("try_again")} + </Link>packages/ui/components/button/Button.tsx (1)
323-368: Tooltip support broken for links.The
Wrappercomponent (which provides tooltip functionality) only wraps the button element (lines 359-368) but not the Link element (lines 323-340). This means tooltips won't display whenhrefis provided.Apply this diff to wrap both Link and button in the Wrapper:
const linkClass = classNames(buttonClasses({ color, size, loading, variant }), props.className); + const element = props.href ? ( - if (props.href) { - return ( <Link data-testid="link-component" href={props.href} shallow={shallow} // Link forwards refs to the underlying anchor automatically with the new Link API ref={forwardedRef as React.Ref<HTMLAnchorElement>} className={linkClass} onClick={ disabled ? (e: React.MouseEvent) => e.preventDefault() : (props.onClick as unknown as React.MouseEventHandler<HTMLAnchorElement>) }> {content} </Link> + ) - ); - } - - const element = React.createElement( + ) : React.createElement( "button", { ...passThroughProps, disabled, type: !isLink ? type : undefined, ref: forwardedRef, className: linkClass, onClick: disabled ? (e: React.MouseEvent<HTMLElement, MouseEvent>) => { e.preventDefault(); } : props.onClick, }, content ); return ( <Wrapper data-testid="wrapper" tooltip={props.tooltip} tooltipSide={tooltipSide} tooltipOffset={tooltipOffset} tooltipClassName={tooltipClassName}> {element} </Wrapper> ); });
🧹 Nitpick comments (3)
packages/ui/components/button/Button.tsx (2)
335-335: Avoid unsafe type casting throughunknown.The onClick handler uses
as unknown ascasting which completely bypasses TypeScript's type safety. This pattern can hide type errors.Apply this diff to use a more type-safe approach:
onClick={ disabled ? (e: React.MouseEvent) => e.preventDefault() - : (props.onClick as unknown as React.MouseEventHandler<HTMLAnchorElement>) + : (props.onClick as React.MouseEventHandler<HTMLAnchorElement> | undefined) }>
347-347: Remove redundant condition.The check
!isLink ? type : undefinedis redundant since this code path only executes when!props.href(which means!isLink).Apply this diff:
...passThroughProps, disabled, - type: !isLink ? type : undefined, + type: type, ref: forwardedRef,packages/ui/components/dropdown/Dropdown.tsx (1)
157-159: No incompatible prop usage found; consider refactoringDropdownItemto use a discriminated union forpropsin Dropdown.tsx (lines 157–159, 167–169) to enforce correct attribute sets and avoid unchecked spreads.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- 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)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
PR_DESCRIPTION.md(1 hunks)apps/web/components/apps/make/Setup.tsx(2 hunks)apps/web/components/apps/zapier/Setup.tsx(3 hunks)apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx(1 hunks)apps/web/modules/bookings/views/bookings-single-view.tsx(7 hunks)package.json(1 hunks)packages/app-store/_components/AppNotInstalledMessage.tsx(1 hunks)packages/features/shell/CalAiBanner.tsx(2 hunks)packages/lib/turndownService.ts(1 hunks)packages/ui/components/button/Button.tsx(3 hunks)packages/ui/components/dropdown/Dropdown.tsx(2 hunks)packages/ui/components/form/step/Stepper.tsx(1 hunks)packages/ui/components/list/List.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/ui/components/form/step/Stepper.tsxapps/web/components/apps/make/Setup.tsxapps/web/modules/bookings/views/bookings-single-view.tsxpackages/app-store/_components/AppNotInstalledMessage.tsxpackages/ui/components/list/List.tsxpackages/ui/components/button/Button.tsxapps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsxpackages/features/shell/CalAiBanner.tsxapps/web/components/apps/zapier/Setup.tsxpackages/ui/components/dropdown/Dropdown.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/ui/components/form/step/Stepper.tsxapps/web/components/apps/make/Setup.tsxapps/web/modules/bookings/views/bookings-single-view.tsxpackages/app-store/_components/AppNotInstalledMessage.tsxpackages/ui/components/list/List.tsxpackages/ui/components/button/Button.tsxapps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsxpackages/lib/turndownService.tspackages/features/shell/CalAiBanner.tsxapps/web/components/apps/zapier/Setup.tsxpackages/ui/components/dropdown/Dropdown.tsx
**/*.{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.tsxapps/web/components/apps/make/Setup.tsxapps/web/modules/bookings/views/bookings-single-view.tsxpackages/app-store/_components/AppNotInstalledMessage.tsxpackages/ui/components/list/List.tsxpackages/ui/components/button/Button.tsxapps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsxpackages/lib/turndownService.tspackages/features/shell/CalAiBanner.tsxapps/web/components/apps/zapier/Setup.tsxpackages/ui/components/dropdown/Dropdown.tsx
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/turndownService.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/turndownService.ts
🧬 Code graph analysis (3)
apps/web/components/apps/make/Setup.tsx (1)
packages/ui/components/button/Button.tsx (1)
Button(221-369)
packages/features/shell/CalAiBanner.tsx (1)
packages/lib/webstorage.ts (1)
localStorage(6-36)
apps/web/components/apps/zapier/Setup.tsx (1)
packages/ui/components/button/Button.tsx (1)
Button(221-369)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (12)
packages/ui/components/form/step/Stepper.tsx (1)
29-42: LGTM! Clean refactor of Stepper Link rendering.The simplified approach using a single Link element with dynamic className and proper aria-current attribute is cleaner and more maintainable than the previous multi-branch conditional rendering. The accessibility features are properly preserved.
packages/lib/turndownService.ts (1)
41-58: LGTM! Robust null safety improvements.The refactored
isShiftEnterfunction now properly handles null/undefined nodes, traverses to the nearest element node, and includes defensive checks for parent and childNodes. This addresses the runtime null parentNode errors mentioned in the PR objectives.PR_DESCRIPTION.md (1)
1-114: LGTM! Comprehensive PR documentation.The PR description provides excellent context for the migration, including motivation, scope, manual QA steps, and a reviewer checklist. This documentation will help ensure thorough review and testing of the changes.
packages/features/shell/CalAiBanner.tsx (1)
14-24: LGTM! Hydration-safe visibility management.The refactor defers localStorage checks to
useEffect, starting with a hidden state (false) to ensure deterministic SSR output. The effect properly guards against SSR context and sets visibility based on pathname and localStorage. This prevents hydration mismatch warnings.apps/web/modules/bookings/views/bookings-single-view.tsx (3)
800-807: LGTM! Clean Link usage without nested elements.The login and reschedule links now use direct Link components with proper styling, removing unnecessary nested spans. This maintains accessibility and aligns with the new Link API pattern.
Also applies to: 830-839
1107-1110: LGTM! RescheduledToLink refactored correctly.The component now renders a direct Link with an inline icon, removing the previous nested structure. The flex layout with gap provides clean spacing while maintaining accessibility.
381-381: Verify removal of old variable referencesPlease confirm there are no remaining occurrences of
canCancelAndReschedule,canCancel, orcanReschedulein apps/web/modules/bookings/views/bookings-single-view.tsx.packages/ui/components/list/List.tsx (2)
1-2: LGTM! Proper "use client" directive.The "use client" directive is correctly placed at the top of the file, which is necessary for using React hooks like
useLocalein Next.js app directory.
35-57: LGTM! Clean separation of ListItem and ListLinkItem.The refactor removes
hrefsupport fromListItem(enforcinghref?: neverin types) and ensures it always renders as a plainlielement. This prevents nested interactive elements and provides clear separation: useListItemfor static list items,ListLinkItemfor navigable items.package.json (1)
142-143: Pinned @radix-ui/react-slot@1.1.2 exists and has no known vulnerabilities.apps/web/components/apps/make/Setup.tsx (1)
136-138: LGTM!The migration to
Button[href]correctly leverages the updated Button component API, eliminating the need for Link wrappers and addressing legacy behavior issues.apps/web/components/apps/zapier/Setup.tsx (1)
13-13: LGTM!The explicit comment documenting the migration strategy and the consistent use of
Button[href]align with the PR objectives and Button component updates.Also applies to: 142-144
| @@ -1,5 +1,6 @@ | |||
| "use client"; | |||
|
|
|||
| /* eslint-disable */ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use targeted ESLint rule disables instead of blanket disable.
The blanket eslint-disable comment disables all linting for the entire file, which can hide legitimate issues and makes the codebase harder to maintain.
Replace with targeted rule disables only where necessary, for example:
-/* eslint-disable */
+/* eslint-disable react-hooks/exhaustive-deps */Or better yet, fix the specific linting issues and remove the need for disables altogether.
📝 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.
| /* eslint-disable */ | |
| /* eslint-disable react-hooks/exhaustive-deps */ |
🤖 Prompt for AI Agents
In apps/web/modules/bookings/views/bookings-single-view.tsx around line 3 the
file uses a blanket "/* eslint-disable */" which disables all linting; remove
that top-level disable, run ESLint to identify the specific violations, fix them
where possible, and if a disable is still required use targeted comments (either
inline "// eslint-disable-next-line <rule>" for a single line or "/*
eslint-disable <rule1>, <rule2> */" scoped narrowly) only around the offending
code so linting remains active for the rest of the file.
| export const ButtonOrLink = forwardRef<HTMLAnchorElement | HTMLButtonElement, ButtonOrLinkProps>( | ||
| ({ href, children, ...props }, forwardedRef) => { | ||
| const isLink = typeof href !== "undefined"; | ||
|
|
||
| if (isLink) { | ||
| // Forward ref and anchor props to Next's Link. Use explicit casts to the correct HTML attribute types | ||
| // to avoid `any` and satisfy ESLint rules. | ||
| return ( | ||
| <Link | ||
| href={href as string} | ||
| ref={forwardedRef as Ref<HTMLAnchorElement>} | ||
| {...(props as AnchorHTMLAttributes<HTMLAnchorElement>)}> | ||
| {children} | ||
| </Link> | ||
| ); | ||
| } | ||
|
|
||
| if (isLink) { | ||
| return ( | ||
| <Link href={href} legacyBehavior> | ||
| {content} | ||
| </Link> | ||
| <button | ||
| ref={forwardedRef as Ref<HTMLButtonElement>} | ||
| {...(props as ButtonHTMLAttributes<HTMLButtonElement>)}> | ||
| {children} | ||
| </button> | ||
| ); | ||
| } | ||
|
|
||
| return content; | ||
| } | ||
| ); | ||
| ButtonOrLink.displayName = "ButtonOrLink"; |
There was a problem hiding this comment.
Type casts bypass safety; consider discriminated union.
Multiple explicit type casts (as string, as Ref<...>, as AnchorHTMLAttributes<...>) bypass TypeScript's type checking. Additionally, the current intersection type (lines 142-146) allows invalid prop combinations (e.g., both type and target simultaneously).
Consider refactoring to use a discriminated union:
-type ButtonOrLinkProps = ButtonHTMLAttributes<HTMLButtonElement> &
- AnchorHTMLAttributes<HTMLAnchorElement> & {
- href?: string;
- children?: React.ReactNode;
- };
+type ButtonOrLinkProps =
+ | ({ href: string } & AnchorHTMLAttributes<HTMLAnchorElement> & { children?: React.ReactNode })
+ | ({ href?: never } & ButtonHTMLAttributes<HTMLButtonElement> & { children?: React.ReactNode });
export const ButtonOrLink = forwardRef<HTMLAnchorElement | HTMLButtonElement, ButtonOrLinkProps>(
({ href, children, ...props }, forwardedRef) => {
- const isLink = typeof href !== "undefined";
-
- if (isLink) {
- // Forward ref and anchor props to Next's Link. Use explicit casts to the correct HTML attribute types
- // to avoid `any` and satisfy ESLint rules.
+ if (href) {
return (
<Link
- href={href as string}
- ref={forwardedRef as Ref<HTMLAnchorElement>}
- {...(props as AnchorHTMLAttributes<HTMLAnchorElement>)}>
+ href={href}
+ ref={forwardedRef as Ref<HTMLAnchorElement>}
+ {...props}>
{children}
</Link>
);
}
return (
<button
ref={forwardedRef as Ref<HTMLButtonElement>}
- {...(props as ButtonHTMLAttributes<HTMLButtonElement>)}>
+ {...props}>
{children}
</button>
);
}
);This approach provides compile-time guarantees that appropriate props are passed to each element type.
📝 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.
| export const ButtonOrLink = forwardRef<HTMLAnchorElement | HTMLButtonElement, ButtonOrLinkProps>( | |
| ({ href, children, ...props }, forwardedRef) => { | |
| const isLink = typeof href !== "undefined"; | |
| if (isLink) { | |
| // Forward ref and anchor props to Next's Link. Use explicit casts to the correct HTML attribute types | |
| // to avoid `any` and satisfy ESLint rules. | |
| return ( | |
| <Link | |
| href={href as string} | |
| ref={forwardedRef as Ref<HTMLAnchorElement>} | |
| {...(props as AnchorHTMLAttributes<HTMLAnchorElement>)}> | |
| {children} | |
| </Link> | |
| ); | |
| } | |
| if (isLink) { | |
| return ( | |
| <Link href={href} legacyBehavior> | |
| {content} | |
| </Link> | |
| <button | |
| ref={forwardedRef as Ref<HTMLButtonElement>} | |
| {...(props as ButtonHTMLAttributes<HTMLButtonElement>)}> | |
| {children} | |
| </button> | |
| ); | |
| } | |
| return content; | |
| } | |
| ); | |
| ButtonOrLink.displayName = "ButtonOrLink"; | |
| type ButtonOrLinkProps = | |
| | ({ href: string } & AnchorHTMLAttributes<HTMLAnchorElement> & { children?: React.ReactNode }) | |
| | ({ href?: never } & ButtonHTMLAttributes<HTMLButtonElement> & { children?: React.ReactNode }); | |
| export const ButtonOrLink = forwardRef<HTMLAnchorElement | HTMLButtonElement, ButtonOrLinkProps>( | |
| ({ href, children, ...props }, forwardedRef) => { | |
| if (href) { | |
| return ( | |
| <Link | |
| href={href} | |
| ref={forwardedRef as Ref<HTMLAnchorElement>} | |
| {...props}> | |
| {children} | |
| </Link> | |
| ); | |
| } | |
| return ( | |
| <button | |
| ref={forwardedRef as Ref<HTMLButtonElement>} | |
| {...props}> | |
| {children} | |
| </button> | |
| ); | |
| } | |
| ); | |
| ButtonOrLink.displayName = "ButtonOrLink"; |
livebitz1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ISSUE.md(1 hunks)ISSUE_BODY.md(1 hunks)PR_DESCRIPTION.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- PR_DESCRIPTION.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
ISSUE.md
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
- GitHub Check: Tests / Unit
- GitHub Check: Codacy Static Code Analysis
| ### Issue Summary | ||
| One-sentence explanation: Next.js `Link` components using `legacyBehavior` caused DOM nesting, ref-forwarding and hydration/runtime errors (e.g. `null parentNode`); this PR migrates `Link` usage to the new API and fixes the related issues. | ||
|
|
||
| --- | ||
|
|
||
| ### Steps to Reproduce | ||
| 1. Prepare the repository and dependencies: | ||
| - `yarn install && yarn dedupe` | ||
| 2. Start the app in dev or production build: | ||
| - `yarn dev` (or `yarn build && yarn start`) | ||
| 3. Open pages that use Link / interactive components such as: | ||
| - Bookings single view, Forgot-password single view, AppNotInstalledMessage, Make & Zapier setup pages, pages with Dropdowns/Buttons-as-Links/List/Stepper | ||
| 4. Interact and observe: | ||
| - Watch the browser console for hydration warnings and runtime errors when opening dropdowns or navigating via keyboard. | ||
| 5. Reproduce specific runtime error path: | ||
| - Trigger flows that convert HTML → markdown (turndownService) or render server/client inconsistent Link markup to reproduce `Cannot read properties of null (reading 'parentNode')` or hydration mismatch. | ||
|
|
||
| Any other relevant information: this is a bug because `legacyBehavior` produced inconsistent server vs client HTML structure and unsafe ref usage, causing developer-facing console warnings and potential user-facing UI breakage. Expected behavior is deterministic SSR → client render with correct element types, correct ref forwarding, and preserved accessibility (keyboard focus, ARIA). | ||
|
|
||
| --- | ||
|
|
||
| ### Actual Results | ||
| - Hydration mismatch warnings appear in the browser console on initial page load. | ||
| - Runtime errors such as `Cannot read properties of null (reading 'parentNode')` from `turndownService` in certain flows. | ||
| - Broken ref forwarding and nested interactive elements (anchor inside button or vice-versa) leading to unexpected keyboard behavior and accessibility regressions. | ||
| - ESLint warnings related to unsafe `any` casts and unused imports in affected files. | ||
|
|
||
| --- | ||
|
|
||
| ### Expected Results | ||
| - No hydration mismatch warnings on page load. | ||
| - No runtime exceptions from `turndownService` or component render paths. | ||
| - Correct ref forwarding (use of `forwardRef` where needed) and no nested interactive elements. | ||
| - Keyboard interaction and focus behavior remain correct and accessible. | ||
| - Clean lint/type output for the modified files. | ||
|
|
||
| --- | ||
|
|
||
| ### Technical details | ||
| - Environment used for testing: macOS (local dev), Node.js 18+ recommended, Yarn | ||
| - Commands used: | ||
| - `yarn install && yarn dedupe` | ||
| - `yarn dev` | ||
| - `yarn build && yarn start` | ||
| - `yarn lint` | ||
| - `yarn test` | ||
| - Branch with fixes: `fix/next-link-hydration` | ||
| - Key files changed: | ||
| - `packages/ui/components/dropdown/Dropdown.tsx` | ||
| - `packages/ui/components/button/Button.tsx` | ||
| - `packages/ui/components/list/List.tsx` | ||
| - `packages/ui/components/form/step/Stepper.tsx` | ||
| - `packages/lib/turndownService.ts` | ||
| - `packages/features/shell/CalAiBanner.tsx` | ||
| - `packages/app-store/_components/AppNotInstalledMessage.tsx` | ||
| - `apps/web/modules/bookings/views/bookings-single-view.tsx` | ||
| - `apps/web/components/apps/make/Setup.tsx` | ||
| - `apps/web/components/apps/zapier/Setup.tsx` | ||
| - `apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx` | ||
|
|
||
| --- | ||
|
|
||
| ### Evidence | ||
| - Manual tests performed: | ||
| - Dev and production builds: confirmed hydration warnings present before the change and resolved after migration to the new `Link` API. | ||
| - Interacted with Dropdowns, List items, Buttons-as-Links, and Stepper to validate keyboard navigation and no nested anchors/buttons. | ||
| - Reproduced and fixed the `null parentNode` error path in `turndownService`. | ||
| - Attachments to include with the issue/PR: | ||
| - Console screenshot showing hydration warning(s) (before) | ||
| - Short GIF showing dropdown/button behavior before vs after (recommended) | ||
| - Relevant console stack trace(s) for the runtime exception(s) | ||
| - Testing notes: run the smoke test checklist in `PR_DESCRIPTION.md` and verify CI (typecheck/lint/tests) passes. |
There was a problem hiding this comment.
Fix heading hierarchy to satisfy markdownlint
After the top-level # heading, the next heading jumps straight to ###, triggering MD001. Please promote the section headings to H2 so the hierarchy increments by one level.
-### Issue Summary
+## Issue Summary
@@
-### Steps to Reproduce
+## Steps to Reproduce
@@
-### Actual Results
+## Actual Results
@@
-### Expected Results
+## Expected Results
@@
-### Technical details
+## Technical details
@@
-### Evidence
+## Evidence📝 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.
| ### Issue Summary | |
| One-sentence explanation: Next.js `Link` components using `legacyBehavior` caused DOM nesting, ref-forwarding and hydration/runtime errors (e.g. `null parentNode`); this PR migrates `Link` usage to the new API and fixes the related issues. | |
| --- | |
| ### Steps to Reproduce | |
| 1. Prepare the repository and dependencies: | |
| - `yarn install && yarn dedupe` | |
| 2. Start the app in dev or production build: | |
| - `yarn dev` (or `yarn build && yarn start`) | |
| 3. Open pages that use Link / interactive components such as: | |
| - Bookings single view, Forgot-password single view, AppNotInstalledMessage, Make & Zapier setup pages, pages with Dropdowns/Buttons-as-Links/List/Stepper | |
| 4. Interact and observe: | |
| - Watch the browser console for hydration warnings and runtime errors when opening dropdowns or navigating via keyboard. | |
| 5. Reproduce specific runtime error path: | |
| - Trigger flows that convert HTML → markdown (turndownService) or render server/client inconsistent Link markup to reproduce `Cannot read properties of null (reading 'parentNode')` or hydration mismatch. | |
| Any other relevant information: this is a bug because `legacyBehavior` produced inconsistent server vs client HTML structure and unsafe ref usage, causing developer-facing console warnings and potential user-facing UI breakage. Expected behavior is deterministic SSR → client render with correct element types, correct ref forwarding, and preserved accessibility (keyboard focus, ARIA). | |
| --- | |
| ### Actual Results | |
| - Hydration mismatch warnings appear in the browser console on initial page load. | |
| - Runtime errors such as `Cannot read properties of null (reading 'parentNode')` from `turndownService` in certain flows. | |
| - Broken ref forwarding and nested interactive elements (anchor inside button or vice-versa) leading to unexpected keyboard behavior and accessibility regressions. | |
| - ESLint warnings related to unsafe `any` casts and unused imports in affected files. | |
| --- | |
| ### Expected Results | |
| - No hydration mismatch warnings on page load. | |
| - No runtime exceptions from `turndownService` or component render paths. | |
| - Correct ref forwarding (use of `forwardRef` where needed) and no nested interactive elements. | |
| - Keyboard interaction and focus behavior remain correct and accessible. | |
| - Clean lint/type output for the modified files. | |
| --- | |
| ### Technical details | |
| - Environment used for testing: macOS (local dev), Node.js 18+ recommended, Yarn | |
| - Commands used: | |
| - `yarn install && yarn dedupe` | |
| - `yarn dev` | |
| - `yarn build && yarn start` | |
| - `yarn lint` | |
| - `yarn test` | |
| - Branch with fixes: `fix/next-link-hydration` | |
| - Key files changed: | |
| - `packages/ui/components/dropdown/Dropdown.tsx` | |
| - `packages/ui/components/button/Button.tsx` | |
| - `packages/ui/components/list/List.tsx` | |
| - `packages/ui/components/form/step/Stepper.tsx` | |
| - `packages/lib/turndownService.ts` | |
| - `packages/features/shell/CalAiBanner.tsx` | |
| - `packages/app-store/_components/AppNotInstalledMessage.tsx` | |
| - `apps/web/modules/bookings/views/bookings-single-view.tsx` | |
| - `apps/web/components/apps/make/Setup.tsx` | |
| - `apps/web/components/apps/zapier/Setup.tsx` | |
| - `apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx` | |
| --- | |
| ### Evidence | |
| - Manual tests performed: | |
| - Dev and production builds: confirmed hydration warnings present before the change and resolved after migration to the new `Link` API. | |
| - Interacted with Dropdowns, List items, Buttons-as-Links, and Stepper to validate keyboard navigation and no nested anchors/buttons. | |
| - Reproduced and fixed the `null parentNode` error path in `turndownService`. | |
| - Attachments to include with the issue/PR: | |
| - Console screenshot showing hydration warning(s) (before) | |
| - Short GIF showing dropdown/button behavior before vs after (recommended) | |
| - Relevant console stack trace(s) for the runtime exception(s) | |
| - Testing notes: run the smoke test checklist in `PR_DESCRIPTION.md` and verify CI (typecheck/lint/tests) passes. | |
| ## Issue Summary | |
| One-sentence explanation: Next.js `Link` components using `legacyBehavior` caused DOM nesting, ref-forwarding and hydration/runtime errors (e.g. `null parentNode`); this PR migrates `Link` usage to the new API and fixes the related issues. | |
| --- | |
| ## Steps to Reproduce | |
| 1. Prepare the repository and dependencies: | |
| - `yarn install && yarn dedupe` | |
| 2. Start the app in dev or production build: | |
| - `yarn dev` (or `yarn build && yarn start`) | |
| 3. Open pages that use Link / interactive components such as: | |
| - Bookings single view, Forgot-password single view, AppNotInstalledMessage, Make & Zapier setup pages, pages with Dropdowns/Buttons-as-Links/List/Stepper | |
| 4. Interact and observe: | |
| - Watch the browser console for hydration warnings and runtime errors when opening dropdowns or navigating via keyboard. | |
| 5. Reproduce specific runtime error path: | |
| - Trigger flows that convert HTML → markdown (turndownService) or render server/client inconsistent Link markup to reproduce `Cannot read properties of null (reading 'parentNode')` or hydration mismatch. | |
| Any other relevant information: this is a bug because `legacyBehavior` produced inconsistent server vs client HTML structure and unsafe ref usage, causing developer-facing console warnings and potential user-facing UI breakage. Expected behavior is deterministic SSR → client render with correct element types, correct ref forwarding, and preserved accessibility (keyboard focus, ARIA). | |
| --- | |
| ## Actual Results | |
| - Hydration mismatch warnings appear in the browser console on initial page load. | |
| - Runtime errors such as `Cannot read properties of null (reading 'parentNode')` from `turndownService` in certain flows. | |
| - Broken ref forwarding and nested interactive elements (anchor inside button or vice-versa) leading to unexpected keyboard behavior and accessibility regressions. | |
| - ESLint warnings related to unsafe `any` casts and unused imports in affected files. | |
| --- | |
| ## Expected Results | |
| - No hydration mismatch warnings on page load. | |
| - No runtime exceptions from `turndownService` or component render paths. | |
| - Correct ref forwarding (use of `forwardRef` where needed) and no nested interactive elements. | |
| - Keyboard interaction and focus behavior remain correct and accessible. | |
| - Clean lint/type output for the modified files. | |
| --- | |
| ## Technical details | |
| - Environment used for testing: macOS (local dev), Node.js 18+ recommended, Yarn | |
| - Commands used: | |
| - `yarn install && yarn dedupe` | |
| - `yarn dev` | |
| - `yarn build && yarn start` | |
| - `yarn lint` | |
| - `yarn test` | |
| - Branch with fixes: `fix/next-link-hydration` | |
| - Key files changed: | |
| - `packages/ui/components/dropdown/Dropdown.tsx` | |
| - `packages/ui/components/button/Button.tsx` | |
| - `packages/ui/components/list/List.tsx` | |
| - `packages/ui/components/form/step/Stepper.tsx` | |
| - `packages/lib/turndownService.ts` | |
| - `packages/features/shell/CalAiBanner.tsx` | |
| - `packages/app-store/_components/AppNotInstalledMessage.tsx` | |
| - `apps/web/modules/bookings/views/bookings-single-view.tsx` | |
| - `apps/web/components/apps/make/Setup.tsx` | |
| - `apps/web/components/apps/zapier/Setup.tsx` | |
| - `apps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsx` | |
| --- | |
| ## Evidence | |
| - Manual tests performed: | |
| - Dev and production builds: confirmed hydration warnings present before the change and resolved after migration to the new `Link` API. | |
| - Interacted with Dropdowns, List items, Buttons-as-Links, and Stepper to validate keyboard navigation and no nested anchors/buttons. | |
| - Reproduced and fixed the `null parentNode` error path in `turndownService`. | |
| - Attachments to include with the issue/PR: | |
| - Console screenshot showing hydration warning(s) (before) | |
| - Short GIF showing dropdown/button behavior before vs after (recommended) | |
| - Relevant console stack trace(s) for the runtime exception(s) | |
| - Testing notes: run the smoke test checklist in `PR_DESCRIPTION.md` and verify CI (typecheck/lint/tests) passes. |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
In ISSUE.md around lines 3 to 74, the heading hierarchy jumps from the top-level
H1 to H3 headings causing markdownlint MD001; update the section headings (e.g.,
"Issue Summary", "Steps to Reproduce", "Actual Results", "Expected Results",
"Technical details", "Evidence") from H3 (###) to H2 (##) so each heading level
increments by one after the initial H1, preserving existing text and
indentation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts (3)
1-1: File-scoped ESLint disable is too broad; narrow itAvoid disabling rules for the whole file. After typing baseData and fixing Zod masks, you can drop this. If needed, use line-scoped disables just where unavoidable.
-/* eslint-disable @typescript-eslint/no-explicit-any, no-unsafe-optional-chaining */ +// Consider removing global disables and using line-scoped disables only where strictly needed.
122-136: Removeas anyin Zod pick/omit; use a typed maskReplace the
as anymasks with a typed mask that aligns with the Zod object’s shape, or minimally add line-scoped disables.Example without
any:const managedKeys = Object.keys(allManagedEventTypeProps) as Array<keyof typeof _ManagedEventTypeModel['shape']>; const managedMask = Object.fromEntries(managedKeys.map((k) => [k, true])) as { [K in keyof typeof _ManagedEventTypeModel['shape']]?: true; }; const allManagedEventTypePropsZod = _ManagedEventTypeModel.pick(managedMask); const managedEventTypeValues = allManagedEventTypePropsZod .omit({ locations: true, scheduleId: true, destinationCalendar: true }) // build unlocked mask explicitly .parse(eventType); const unlockedMask = { locations: true, scheduleId: true, destinationCalendar: true } as const; const unlockedEventTypeValues = allManagedEventTypePropsZod.pick(unlockedMask).parse(eventType);If keeping current approach, limit to:
// eslint-disable-next-line @typescript-eslint/no-explicit-any const allManagedEventTypePropsZod = _ManagedEventTypeModel.pick(allManagedEventTypeProps as any); // eslint-disable-next-line @typescript-eslint/no-explicit-any const managedEventTypeValues = allManagedEventTypePropsZod.omit(unlockedManagedEventTypeProps as any).parse(eventType); // eslint-disable-next-line @typescript-eslint/no-explicit-any const unlockedEventTypeValues = allManagedEventTypePropsZod.pick(unlockedManagedEventTypeProps as any).parse(eventType);
90-98: Clean up unused param and prefer named export
_updatedValuesis unused. Remove it from the function params (and from the props type if not needed) to reduce noise.- Prefer named export over default, per repo guidelines. As per coding guidelines.
-export default async function handleChildrenEventTypes({ +export async function handleChildrenEventTypes({ eventTypeId: parentId, oldEventType, updatedEventType, children, prisma, profileId, - updatedValues: _updatedValues, }: handleChildrenEventTypesProps) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts(5 hunks)packages/ui/components/address/Select.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/ui/components/address/Select.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts
**/*.{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/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts
🧠 Learnings (1)
📚 Learning: 2025-09-12T07:15:58.056Z
Learnt from: hbjORbj
PR: calcom/cal.com#23475
File: packages/features/credentials/handleDeleteCredential.ts:5-10
Timestamp: 2025-09-12T07:15:58.056Z
Learning: When EventTypeAppMetadataSchema is used only in z.infer<typeof EventTypeAppMetadataSchema> type annotations or type casts, it can be imported as a type-only import since this usage is purely at the TypeScript type level and doesn't require the runtime value.
Applied to files:
packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts
🧬 Code graph analysis (1)
packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts (1)
packages/prisma/zod-utils.ts (2)
allManagedEventTypeProps(627-696)unlockedManagedEventTypeProps(700-704)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
- GitHub Check: Codacy Static Code Analysis
| // build data object to avoid duplicate property definitions in object literal | ||
| data: (() => { | ||
| const baseData: any = { | ||
| profileId: profileId ?? null, | ||
| ...managedValuesWithoutExplicit, | ||
| ...unlockedEventTypeValues, | ||
| // pre-genned as allowed null | ||
| locations: Array.isArray(unlockedEventTypeValues.locations) | ||
| ? unlockedEventTypeValues.locations | ||
| : undefined, | ||
| }, | ||
| bookingLimits: | ||
| (managedEventTypeValues.bookingLimits as unknown as Prisma.InputJsonObject) ?? undefined, | ||
| recurringEvent: | ||
| (managedEventTypeValues.recurringEvent as unknown as Prisma.InputJsonValue) ?? undefined, | ||
| metadata: (managedEventTypeValues.metadata as Prisma.InputJsonValue) ?? undefined, | ||
| bookingFields: (managedEventTypeValues.bookingFields as Prisma.InputJsonValue) ?? undefined, | ||
| durationLimits: (managedEventTypeValues.durationLimits as Prisma.InputJsonValue) ?? undefined, | ||
| eventTypeColor: (managedEventTypeValues.eventTypeColor as Prisma.InputJsonValue) ?? undefined, | ||
| onlyShowFirstAvailableSlot: managedEventTypeValues.onlyShowFirstAvailableSlot ?? false, | ||
| userId, | ||
| users: { | ||
| connect: [{ id: userId }], | ||
| }, | ||
| parentId, | ||
| hidden: children?.find((ch) => ch.owner.id === userId)?.hidden ?? false, | ||
| workflows: currentWorkflowIds && { | ||
| create: currentWorkflowIds.map((wfId) => ({ workflowId: wfId })), | ||
| }, | ||
| /** | ||
| * RR Segment isn't applicable for managed event types. | ||
| */ | ||
| rrSegmentQueryValue: undefined, | ||
| assignRRMembersUsingSegment: false, | ||
| useEventLevelSelectedCalendars: false, | ||
| restrictionScheduleId: null, | ||
| useBookerTimezone: false, | ||
| allowReschedulingCancelledBookings: | ||
| managedEventTypeValues.allowReschedulingCancelledBookings ?? false, | ||
| }, | ||
| bookingLimits: | ||
| (managedEventTypeValues.bookingLimits as unknown as Prisma.InputJsonObject) ?? undefined, | ||
| recurringEvent: | ||
| (managedEventTypeValues.recurringEvent as unknown as Prisma.InputJsonValue) ?? undefined, | ||
| metadata: (managedEventTypeValues.metadata as Prisma.InputJsonValue) ?? undefined, | ||
| bookingFields: (managedEventTypeValues.bookingFields as Prisma.InputJsonValue) ?? undefined, | ||
| durationLimits: (managedEventTypeValues.durationLimits as Prisma.InputJsonValue) ?? undefined, | ||
| eventTypeColor: (managedEventTypeValues.eventTypeColor as Prisma.InputJsonValue) ?? undefined, | ||
| onlyShowFirstAvailableSlot: managedEventTypeValues.onlyShowFirstAvailableSlot ?? false, | ||
| userId, | ||
| users: { | ||
| connect: [{ id: userId }], | ||
| }, | ||
| parentId, | ||
| hidden: children?.find((ch) => ch.owner.id === userId)?.hidden ?? false, | ||
| workflows: currentWorkflowIds && { | ||
| create: currentWorkflowIds.map((wfId) => ({ workflowId: wfId })), | ||
| }, | ||
| /** | ||
| * RR Segment isn't applicable for managed event types. | ||
| */ | ||
| rrSegmentQueryValue: undefined, | ||
| assignRRMembersUsingSegment: false, | ||
| useEventLevelSelectedCalendars: false, | ||
| restrictionScheduleId: null, | ||
| useBookerTimezone: false, | ||
| allowReschedulingCancelledBookings: | ||
| managedEventTypeValues.allowReschedulingCancelledBookings ?? false, | ||
| }; | ||
| // set instantMeetingScheduleId explicitly after building baseData to avoid duplicate definitions | ||
| baseData.instantMeetingScheduleId = eventType.instantMeetingScheduleId ?? undefined; | ||
| return baseData; | ||
| })(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
instantMeetingScheduleId is never loaded; create payload always sets it to undefined
You set baseData.instantMeetingScheduleId from eventType, but eventType is fetched with select: allManagedEventTypeProps, which doesn’t include instantMeetingScheduleId. Result: field is always undefined in creates.
Fix by selecting it explicitly (keep it out of managed picks) and type the payload:
TypeScript (outside this hunk):
// ensure it's selected
const eventType = await prisma.eventType.findFirst({
where: { id: parentId },
select: { ...allManagedEventTypeProps, instantMeetingScheduleId: true },
});Apply within this hunk:
- const baseData: any = {
+ const baseData: Prisma.EventTypeCreateInput = {
profileId: profileId ?? null,
...managedValuesWithoutExplicit,
...unlockedEventTypeValues,
// pre-genned as allowed null
locations: Array.isArray(unlockedEventTypeValues.locations)
? unlockedEventTypeValues.locations
: undefined,
@@
- workflows: currentWorkflowIds && {
- create: currentWorkflowIds.map((wfId) => ({ workflowId: wfId })),
- },
+ workflows: currentWorkflowIds?.length
+ ? { create: currentWorkflowIds.map((wfId) => ({ workflowId: wfId })) }
+ : undefined,
@@
- baseData.instantMeetingScheduleId = eventType.instantMeetingScheduleId ?? undefined;
+ baseData.instantMeetingScheduleId = eventType.instantMeetingScheduleId ?? undefined;Verification script to confirm the field exists in the repo:
🏁 Script executed:
#!/bin/bash
# Look for the field in Prisma model and usages
rg -nP -C2 '\binstantMeetingScheduleId\b'Length of output: 13202
🏁 Script executed:
#!/bin/bash
rg -n 'allManagedEventTypeProps' -C5Length of output: 12699
🏁 Script executed:
#!/bin/bash
rg -n 'eslint-disable' -C2 packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.tsLength of output: 151
🏁 Script executed:
#!/bin/bash
rg -n 'export const allManagedEventTypeProps' -A200 packages/prisma/zod-utils.tsLength of output: 7243
Select and propagate instantMeetingScheduleId in the create payload
The eventType is fetched with select: allManagedEventTypeProps, which doesn’t include instantMeetingScheduleId, so baseData.instantMeetingScheduleId is always undefined. Include it explicitly and tighten your typings:
// ensure it’s selected
const eventType = await prisma.eventType.findFirst({
where: { id: parentId },
select: { ...allManagedEventTypeProps, instantMeetingScheduleId: true },
});- const baseData: any = {
+ const baseData: Prisma.EventTypeCreateInput = {
profileId: profileId ?? null,
…
- workflows: currentWorkflowIds && {
- create: currentWorkflowIds.map((wfId) => ({ workflowId: wfId })),
- },
+ workflows: currentWorkflowIds?.length
+ ? { create: currentWorkflowIds.map((wfId) => ({ workflowId: wfId })) }
+ : undefined,
};
- baseData.instantMeetingScheduleId = eventType.instantMeetingScheduleId ?? undefined;
+ baseData.instantMeetingScheduleId = eventType.instantMeetingScheduleId ?? undefined;🤖 Prompt for AI Agents
In packages/features/ee/managed-event-types/lib/handleChildrenEventTypes.ts
around lines 170 to 213, the code assigns baseData.instantMeetingScheduleId from
eventType but the earlier prisma query did not select instantMeetingScheduleId
so it will always be undefined; update the prisma.eventType.findFirst/select to
include instantMeetingScheduleId: true (e.g., spread allManagedEventTypeProps
and add instantMeetingScheduleId: true), and tighten the TypeScript typings for
the fetched eventType to include instantMeetingScheduleId (and handle possible
null eventType) so baseData.instantMeetingScheduleId can be populated correctly.
keithwillcode
left a comment
keithwillcode
left a comment
There was a problem hiding this comment.
Please fix yarn.lock. Too many things removed from it.
Also please remove the ISSUE.md, ISSUE_BODY.md and PR_DESCRIPTION.md files
|
Hello @livebitz1, please address the requested changes, making the PR draft until then. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 18 files
Prompt for AI agents (all 6 issues)
Understand the root cause of the following 6 issues and fix them.
<file name="packages/ui/components/button/Button.tsx">
<violation number="1" location="packages/ui/components/button/Button.tsx:325">
When rendering the link variant we stop forwarding passThroughProps, so external attributes like target, rel, aria-* and LinkProps (prefetch, replace, etc.) are lost, breaking existing Button-as-link usage.</violation>
</file>
<file name="ISSUE_BODY.md">
<violation number="1" location="ISSUE_BODY.md:2">
Wrap `<Link legacyBehavior>` in backticks (or escape it) so the Summary renders the literal component name.</violation>
</file>
<file name="PR_DESCRIPTION.md">
<violation number="1" location="PR_DESCRIPTION.md:1">
Escape `<Link legacyBehavior>` in the title so it renders as text; otherwise Markdown interprets it as an HTML `<link>` tag and the literal string disappears.</violation>
</file>
<file name="packages/ui/components/form/step/Stepper.tsx">
<violation number="1" location="packages/ui/components/form/step/Stepper.tsx:41">
The updated Link rendering for the active step removes the visual indicator spans, so the current step no longer shows the highlighted dot/ring. Please restore the aria-hidden spans (outer ring + inner dot) so the active step remains visible.</violation>
</file>
<file name="packages/lib/turndownService.ts">
<violation number="1" location="packages/lib/turndownService.ts:46">
Referencing `Node.ELEMENT_NODE` here breaks turndown in Node/SSR contexts because the global `Node` constructor is undefined there. Please keep the numeric nodeType comparison (and update the guard below) so the helper remains environment-agnostic.</violation>
</file>
<file name="packages/app-store/_components/AppNotInstalledMessage.tsx">
<violation number="1" location="packages/app-store/_components/AppNotInstalledMessage.tsx:20">
Using the new Link API this wraps an <a> around a Button that still renders a <button>, creating nested interactive elements and breaking keyboard/a11y semantics. Please pass the href directly to Button (so it renders the anchor itself) or render a plain <a> instead.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| if (props.href) { | ||
| return ( | ||
| <Link |
There was a problem hiding this comment.
When rendering the link variant we stop forwarding passThroughProps, so external attributes like target, rel, aria-* and LinkProps (prefetch, replace, etc.) are lost, breaking existing Button-as-link usage.
Prompt for AI agents
Address the following comment on packages/ui/components/button/Button.tsx at line 325:
<comment>When rendering the link variant we stop forwarding passThroughProps, so external attributes like target, rel, aria-* and LinkProps (prefetch, replace, etc.) are lost, breaking existing Button-as-link usage.</comment>
<file context>
@@ -332,11 +318,45 @@ export const Button = forwardRef<HTMLAnchorElement | HTMLButtonElement, ButtonPr
+
+ if (props.href) {
+ return (
+ <Link
+ data-testid="link-component"
+ href={props.href}
</file context>
| @@ -0,0 +1,36 @@ | |||
| Summary | |||
| I encountered several runtime and hydration issues when running the app locally that traced back to usage of Next.js <Link legacyBehavior>. This repo-wide migration removes legacyBehavior usage and fixes ref-forwarding, DOM nesting (nested anchors/buttons), hydration mismatch warnings, and a runtime null parentNode error in turndownService. | |||
There was a problem hiding this comment.
Wrap <Link legacyBehavior> in backticks (or escape it) so the Summary renders the literal component name.
Prompt for AI agents
Address the following comment on ISSUE_BODY.md at line 2:
<comment>Wrap `<Link legacyBehavior>` in backticks (or escape it) so the Summary renders the literal component name.</comment>
<file context>
@@ -0,0 +1,36 @@
+Summary
+I encountered several runtime and hydration issues when running the app locally that traced back to usage of Next.js <Link legacyBehavior>. This repo-wide migration removes legacyBehavior usage and fixes ref-forwarding, DOM nesting (nested anchors/buttons), hydration mismatch warnings, and a runtime null parentNode error in turndownService.
+
+Why this matters
</file context>
| @@ -0,0 +1,91 @@ | |||
| fix(ui): migrate off Next.js <Link legacyBehavior> — fix hydration & ref-forwarding issues | |||
There was a problem hiding this comment.
Escape <Link legacyBehavior> in the title so it renders as text; otherwise Markdown interprets it as an HTML <link> tag and the literal string disappears.
Prompt for AI agents
Address the following comment on PR_DESCRIPTION.md at line 1:
<comment>Escape `<Link legacyBehavior>` in the title so it renders as text; otherwise Markdown interprets it as an HTML `<link>` tag and the literal string disappears.</comment>
<file context>
@@ -0,0 +1,91 @@
+fix(ui): migrate off Next.js <Link legacyBehavior> — fix hydration & ref-forwarding issues
+
+Fixes
</file context>
| fix(ui): migrate off Next.js <Link legacyBehavior> — fix hydration & ref-forwarding issues | |
| fix(ui): migrate off Next.js `<Link legacyBehavior>` — fix hydration & ref-forwarding issues |
| : "bg-emphasis block h-2.5 w-2.5 rounded-full hover:bg-gray-400" | ||
| } | ||
| aria-current={index + 1 === props.step ? "step" : undefined}> | ||
| <span className="sr-only">{mapStep.title}</span> |
There was a problem hiding this comment.
The updated Link rendering for the active step removes the visual indicator spans, so the current step no longer shows the highlighted dot/ring. Please restore the aria-hidden spans (outer ring + inner dot) so the active step remains visible.
Prompt for AI agents
Address the following comment on packages/ui/components/form/step/Stepper.tsx at line 41:
<comment>The updated Link rendering for the active step removes the visual indicator spans, so the current step no longer shows the highlighted dot/ring. Please restore the aria-hidden spans (outer ring + inner dot) so the active step remains visible.</comment>
<file context>
@@ -30,27 +30,15 @@ function Stepper<T extends DefaultStep>(props: {
+ : "bg-emphasis block h-2.5 w-2.5 rounded-full hover:bg-gray-400"
+ }
+ aria-current={index + 1 === props.step ? "step" : undefined}>
+ <span className="sr-only">{mapStep.title}</span>
</Link>
</li>
</file context>
| currentNode = currentNode.parentElement || currentNode.parentNode; | ||
| // Walk up to the nearest element node | ||
| let currentNode: Node | null = node; | ||
| while (currentNode && currentNode.nodeType !== Node.ELEMENT_NODE) { |
There was a problem hiding this comment.
Referencing Node.ELEMENT_NODE here breaks turndown in Node/SSR contexts because the global Node constructor is undefined there. Please keep the numeric nodeType comparison (and update the guard below) so the helper remains environment-agnostic.
Prompt for AI agents
Address the following comment on packages/lib/turndownService.ts at line 46:
<comment>Referencing `Node.ELEMENT_NODE` here breaks turndown in Node/SSR contexts because the global `Node` constructor is undefined there. Please keep the numeric nodeType comparison (and update the guard below) so the helper remains environment-agnostic.</comment>
<file context>
@@ -38,19 +38,23 @@ turndownService.addRule("ignoreEmphasized", {
- currentNode = currentNode.parentElement || currentNode.parentNode;
+ // Walk up to the nearest element node
+ let currentNode: Node | null = node;
+ while (currentNode && currentNode.nodeType !== Node.ELEMENT_NODE) {
+ currentNode = currentNode.parentNode;
}
</file context>
|
|
||
| <div className="mt-5"> | ||
| <Link href={`/apps/${appName}`} passHref={true} legacyBehavior> | ||
| <Link href={`/apps/${appName}`} className="inline-block"> |
There was a problem hiding this comment.
Using the new Link API this wraps an around a Button that still renders a , creating nested interactive elements and breaking keyboard/a11y semantics. Please pass the href directly to Button (so it renders the anchor itself) or render a plain instead.
Prompt for AI agents
Address the following comment on packages/app-store/_components/AppNotInstalledMessage.tsx at line 20:
<comment>Using the new Link API this wraps an <a> around a Button that still renders a <button>, creating nested interactive elements and breaking keyboard/a11y semantics. Please pass the href directly to Button (so it renders the anchor itself) or render a plain <a> instead.</comment>
<file context>
@@ -17,7 +17,7 @@ export default function AppNotInstalledMessage({ appName }: { appName: string })
<div className="mt-5">
- <Link href={`/apps/${appName}`} passHref={true} legacyBehavior>
+ <Link href={`/apps/${appName}`} className="inline-block">
<Button type="button" color="secondary">
{t("go_to_app_store")}
</file context>
3 participants
Fixes #24417
Summary
LinklegacyBehaviorusage across the codebase and migrated affected components to the newLinkAPI to resolve ref-forwarding, DOM nesting, and hydration/runtime errors observed while running the application locally.null parentNode), improved SSR determinism, and preserved accessibility/interactive behavior for core UI components (Dropdown, Button, List, Stepper, etc.).Impact / Scope
Motivation
Using
legacyBehaviorproduced inconsistent server/client markup, broken refs, and nested interactive elements (anchor inside button), leading to hydration warnings and runtime exceptions. Migrating to the newLinkAPI prevents these problems and prepares the codebase for React 19 / Next.js 15.Files changed (high level)
packages/ui/components/dropdown/Dropdown.tsxpackages/ui/components/button/Button.tsxpackages/ui/components/list/List.tsxpackages/ui/components/form/step/Stepper.tsxpackages/lib/turndownService.tspackages/features/shell/CalAiBanner.tsxpackages/app-store/_components/AppNotInstalledMessage.tsxapps/web/modules/bookings/views/bookings-single-view.tsxapps/web/components/apps/make/Setup.tsxapps/web/components/apps/zapier/Setup.tsxapps/web/modules/auth/forgot-password/[id]/forgot-password-single-view.tsxKey changes
legacyBehaviorand updated Link usage to the new API (passclassName/props directly toLink).anycasts in Dropdown and Button components.ListLinkItemto avoid nested interactive elements.turndownServiceto avoid traversing null parent nodes.CalAiBannervisibility to client mount to prevent hydration mismatches.Manual QA steps (recommended)
yarn install && yarn dedupeyarn dev# or build withyarn buildyarn lintyarn testnull parentNode, ref issues) no longer appear in logs/consoleWhat reviewers should verify
fix:) — required for CIlegacyBehavioranycasts)Commands (copy to terminal)
Update PR title (run once)
gh pr edit 24417 --title "fix(ui): migrate off Next.js — fix hydration & ref-forwarding issues"
Update PR body from this file (run once)
gh pr edit 24417 --body-file PR_DESCRIPTION.md
Optional: add reviewers/labels
gh pr edit 24417 --add-reviewer maintainer1 --add-reviewer maintainer2 --add-label "area:ui" --add-label "type:bug"
Local checks
yarn install && yarn dedupe
yarn build
yarn lint
yarn test
Visual demo (strongly recommended)
Checklist (DO NOT REMOVE)
Notes