Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions apps/web/components/apps/make/Setup.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { InferGetServerSidePropsType } from "next";
import Link from "next/link";
import { useState } from "react";
import { Toaster } from "sonner";

Expand Down Expand Up @@ -71,9 +70,10 @@ export default function MakeSetup({ inviteLink }: InferGetServerSidePropsType<ty
<div className="bg-default m-auto max-w-[43em] overflow-auto rounded pb-10 md:p-10">
<div className="md:flex md:flex-row">
<div className="invisible md:visible">
{/* eslint-disable @next/next/no-img-element */}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It's generally better to address the underlying ESLint rule rather than disabling it. While this might be a quick fix, consider if there's a more permanent solution to allow img elements without the alt attribute or if the alt attribute can be dynamically provided.

<img className="h-11" src="/api/app-store/make/icon.svg" alt="Make Logo" />
</div>
<div className="ml-2 ltr:mr-2 rtl:ml-2 md:ml-5">
<div className="ml-2 md:ml-5 ltr:mr-2 rtl:ml-2">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS Class Ordering

CSS class ordering places responsive modifier 'md:ml-5' between base 'ml-2' and directional classes 'ltr:mr-2 rtl:ml-2'. This creates potential cascade conflicts where directional classes may override responsive spacing. Consistent ordering should group base, responsive, then directional modifiers.

Standards
  • CSS-Cascade-Logic
  • Responsive-Design-Consistency

<div className="text-default">{t("setting_up_make")}</div>

<>
Expand Down Expand Up @@ -134,9 +134,9 @@ export default function MakeSetup({ inviteLink }: InferGetServerSidePropsType<ty
<li>{t("make_setup_instructions_5")}</li>
<li>{t("make_setup_instructions_6")}</li>
</ol>
<Link href="/apps/installed/automation?hl=make" passHref={true} legacyBehavior>
<Button color="secondary">{t("done")}</Button>
</Link>
<Button href="/apps/installed/automation?hl=make" color="secondary">
{t("done")}
</Button>
</div>
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,10 @@ function Expired() {
<h2 className="text-emphasis text-center text-3xl font-extrabold">{t("request_is_expired")}</h2>
</div>
<p>{t("request_is_expired_instructions")}</p>
<Link href="/auth/forgot-password" passHref legacyBehavior>
<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
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>
</div>
</>
Expand Down
38 changes: 18 additions & 20 deletions apps/web/modules/bookings/views/bookings-single-view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -806,15 +806,14 @@ export default function Success(props: PageProps) {
</span>
{/* Login button but redirect to here */}
<span className="text-default inline">
<span className="underline" data-testid="reschedule-link">
<Link
href={`/auth/login?callbackUrl=${encodeURIComponent(
`/booking/${bookingInfo?.uid}`
)}`}
legacyBehavior>
{t("login")}
</Link>
</span>
<Link
href={`/auth/login?callbackUrl=${encodeURIComponent(
`/booking/${bookingInfo?.uid}`
)}`}
className="underline"
data-testid="reschedule-link">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The login link for users who must log in before updating a booking is tagged with the reschedule-link test id, which can cause automated tests or tooling that target the reschedule action to mistakenly interact with this login link instead, leading to incorrect flows being exercised in scenarios where login is required to update a booking. [possible bug]

Severity Level: Critical 🚨

Suggested change
data-testid="reschedule-link">
data-testid="login-link">
Why it matters? ⭐

The current code places data-testid="reschedule-link" on a login link used when requiresLoginToUpdate is true.
That re-uses a test id that's also used for the actual reschedule action elsewhere and can cause automated tests or tooling to
target the wrong element. Renaming the test id to something specific to the login action (e.g. "login-link") fixes an actual
test/selector collision rather than being a purely cosmetic change.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** apps/web/modules/bookings/views/bookings-single-view.tsx
**Line:** 814:814
**Comment:**
	*Possible Bug: The login link for users who must log in before updating a booking is tagged with the `reschedule-link` test id, which can cause automated tests or tooling that target the reschedule action to mistakenly interact with this login link instead, leading to incorrect flows being exercised in scenarios where login is required to update a booking.

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.

{t("login")}
</Link>
</span>
</div>
</>
Expand All @@ -837,17 +836,16 @@ export default function Success(props: PageProps) {
(!isBookingInPast || eventType.allowReschedulingPastBookings) &&
canReschedule && (
<span className="text-default inline">
<span className="underline" data-testid="reschedule-link">
<Link
href={`/reschedule/${seatReferenceUid || bookingInfo?.uid}${
currentUserEmail
? `?rescheduledBy=${encodeURIComponent(currentUserEmail)}`
: ""
}`}
legacyBehavior>
{t("reschedule")}
</Link>
</span>
<Link
href={`/reschedule/${seatReferenceUid || bookingInfo?.uid}${
currentUserEmail
? `?rescheduledBy=${encodeURIComponent(currentUserEmail)}`
: ""
}`}
className="underline"
data-testid="reschedule-link">
Comment on lines +839 to +846
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The reschedule link now has the data-testid="reschedule-link" on the <Link> element instead of the wrapping <span>, which breaks existing selectors like span[data-testid="reschedule-link"] > a used in e2e tests (e.g. booking-limits.e2e.ts) and prevents those tests from finding the reschedule URL, so moving the test id back to the span restores the DOM contract without changing user-facing behavior. [possible bug]

Severity Level: Critical 🚨

Suggested change
<Link
href={`/reschedule/${seatReferenceUid || bookingInfo?.uid}${
currentUserEmail
? `?rescheduledBy=${encodeURIComponent(currentUserEmail)}`
: ""
}`}
className="underline"
data-testid="reschedule-link">
<span className="text-default inline" data-testid="reschedule-link">
<Link
href={`/reschedule/${seatReferenceUid || bookingInfo?.uid}${
currentUserEmail
? `?rescheduledBy=${encodeURIComponent(currentUserEmail)}`
: ""
}`}
className="underline">
Why it matters? ⭐

The change in the PR moved data-testid="reschedule-link" from the wrapping span to the Link element. If existing e2e or DOM-based tests expect the test id on the wrapper span (selectors like span[data-testid="reschedule-link"] > a), the PR will break those tests.
Moving the test id back to the span preserves the prior DOM contract without affecting user-facing behavior, so this is a practical, low-risk fix to maintain test stability.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** apps/web/modules/bookings/views/bookings-single-view.tsx
**Line:** 839:846
**Comment:**
	*Possible Bug: The reschedule link now has the `data-testid="reschedule-link"` on the `<Link>` element instead of the wrapping `<span>`, which breaks existing selectors like `span[data-testid="reschedule-link"] > a` used in e2e tests (e.g. `booking-limits.e2e.ts`) and prevents those tests from finding the reschedule URL, so moving the test id back to the span restores the DOM contract without changing user-facing 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.

{t("reschedule")}
</Link>
{!isBookingInPast && canCancel && (
<span className="mx-2">{t("or_lowercase")}</span>
)}
Expand Down
12 changes: 4 additions & 8 deletions packages/app-store/_components/AppNotInstalledMessage.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import Link from "next/link";

import { useLocale } from "@calcom/lib/hooks/useLocale";
import { Button } from "@calcom/ui/components/button";
import { Icon } from "@calcom/ui/components/icon";
Expand All @@ -17,12 +15,10 @@ export default function AppNotInstalledMessage({ appName }: { appName: string })
<p className="text-subtle px-1 leading-normal">{t("visit_our_app_store")}</p>

<div className="mt-5">
<Link href={`/apps/${appName}`} passHref={true} legacyBehavior>
<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>
</div>
</div>
Expand Down
58 changes: 34 additions & 24 deletions packages/ui/components/button/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -239,25 +239,17 @@ export const Button = forwardRef<HTMLAnchorElement | HTMLButtonElement, ButtonPr
...passThroughProps
} = props;
// Buttons are **always** disabled if we're in a `loading` state
const disabled = props.disabled || loading;
// If pass an `href`-attr is passed it's `<a>`, otherwise it's a `<button />`
const disabled = props.disabled || loading || false;
// If pass an `href`-attr is passed it's Link, otherwise it's a `<button />`
const isLink = typeof props.href !== "undefined";
const elementType = isLink ? "a" : "button";
const element = React.createElement(
elementType,
{
...passThroughProps,
disabled,
type: !isLink ? type : undefined,
ref: forwardedRef,
className: classNames(buttonClasses({ color, size, loading, variant }), props.className),
// if we click a disabled button, we prevent going through the click handler
onClick: disabled
? (e: React.MouseEvent<HTMLElement, MouseEvent>) => {
e.preventDefault();
}
: props.onClick,
},
const buttonClassName = classNames(buttonClasses({ color, size, loading, variant }), props.className);
const handleClick = disabled
? (e: React.MouseEvent<HTMLElement, MouseEvent>) => {
e.preventDefault();
}
: props.onClick;
Comment on lines +246 to +250
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Error Handling

Button component passes through onClick handler without error boundary protection. Navigation failures or handler exceptions can propagate uncaught causing component crashes. User interactions become unreliable when handler throws unexpected errors.

Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
Context References
  • packages/ui/components/button/Button.tsx - Button component handles both link and button elements - no error handling wrapper


const buttonContent = (
<>
{CustomStartIcon ||
(StartIcon && (
Expand Down Expand Up @@ -334,18 +326,36 @@ export const Button = forwardRef<HTMLAnchorElement | HTMLButtonElement, ButtonPr
</>
);

return props.href ? (
<Link data-testid="link-component" passHref href={props.href} shallow={shallow && shallow} legacyBehavior>
{element}
</Link>
) : (
// Render Link or button separately to avoid type conflicts
// Link manages its own anchor element, so we don't pass ref to it
if (isLink) {
return (
<Link
{...(passThroughProps as Omit<JSX.IntrinsicElements["a"], "href" | "onClick" | "ref"> & LinkProps)}
shallow={shallow && shallow}
className={buttonClassName}
onClick={handleClick}>
{buttonContent}
</Link>
);
}

return (
<Wrapper
data-testid="wrapper"
tooltip={props.tooltip}
tooltipSide={tooltipSide}
tooltipOffset={tooltipOffset}
tooltipClassName={tooltipClassName}>
{element}
<button
{...(passThroughProps as Omit<JSX.IntrinsicElements["button"], "onClick" | "ref">)}
ref={forwardedRef as React.Ref<HTMLButtonElement>}
disabled={disabled}
type={type as "button" | "submit" | "reset"}
className={buttonClassName}
onClick={handleClick}>
{buttonContent}
</button>
</Wrapper>
);
});
Expand Down
13 changes: 4 additions & 9 deletions packages/ui/components/dropdown/Dropdown.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -143,19 +143,14 @@ type ButtonOrLinkProps = ComponentProps<"button"> & ComponentProps<"a">;

export function ButtonOrLink({ href, ...props }: ButtonOrLinkProps) {
const isLink = typeof href !== "undefined";
const ButtonOrLink = isLink ? "a" : "button";

const content = <ButtonOrLink {...props} />;

if (isLink) {
return (
<Link href={href} legacyBehavior>
{content}
</Link>
);
// Strip ref from props when using Link (Link manages its own anchor element)
const { ref: _ref, ...linkProps } = props;
return <Link href={href} {...linkProps} />;
}

return content;
return <button {...props} />;
}

export const DropdownItem = (props: DropdownItemProps) => {
Expand Down
59 changes: 33 additions & 26 deletions packages/ui/components/form/step/Stepper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,32 +26,39 @@ function Stepper<T extends DefaultStep>(props: {
<ol role="list" className="ml-8 flex items-center space-x-5" ref={stepperRef}>
{steps.map((mapStep, index) => (
<li key={mapStep.title}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Using mapStep.title alone as the React key for each step assumes titles are globally unique; if two steps share the same title, React's reconciliation can mis-associate DOM nodes and lead to incorrect UI updates when steps change, so the key should be made unique (for example by incorporating the index). [possible bug]

Severity Level: Critical 🚨

Suggested change
<li key={mapStep.title}>
<li key={`${mapStep.title}-${index}`}>
Why it matters? ⭐

Using only mapStep.title as the key can lead to duplicated keys if two steps share a title, which breaks React reconciliation and can cause UI bugs.
Appending the index (or, better, using a stable unique id on the step) makes the key unique and prevents accidental DOM reuse. This is a practical fix for a real potential bug.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** packages/ui/components/form/step/Stepper.tsx
**Line:** 28:28
**Comment:**
	*Possible Bug: Using `mapStep.title` alone as the React `key` for each step assumes titles are globally unique; if two steps share the same title, React's reconciliation can mis-associate DOM nodes and lead to incorrect UI updates when steps change, so the key should be made unique (for example by incorporating the index).

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.

<Link
href={props.disableSteps ? "#" : `${href}?step=${index + 1}`}
shallow
replace
legacyBehavior>
{index + 1 < props.step ? (
<a className="hover:bg-inverted block h-2.5 w-2.5 rounded-full bg-gray-600">
<span className="sr-only">{mapStep.title}</span>
</a>
) : index + 1 === props.step ? (
<a 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>
</a>
) : (
<a className="bg-emphasis block h-2.5 w-2.5 rounded-full hover:bg-gray-400">
<span className="sr-only">{mapStep.title}</span>
</a>
)}
</Link>
{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}`}
Comment on lines +31 to +55
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Building the step URLs by blindly appending ?step=${index + 1} to the href string will break when the base href already contains query parameters (e.g. /auth/setup?mode=admin), resulting in invalid URLs like /auth/setup?mode=admin?step=2 and incorrect routing behavior; you should append step with & when a ? already exists. [logic error]

Severity Level: Minor ⚠️

Suggested change
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}`}
href={
props.disableSteps
? "#"
: `${href}${href.includes("?") ? "&" : "?"}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}${href.includes("?") ? "&" : "?"}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}${href.includes("?") ? "&" : "?"}step=${index + 1}`
}
Why it matters? ⭐

The claim is correct: blindly appending ?step=... will create malformed URLs when href already has query params.
This leads to incorrect links and potential routing issues. The improvement (conditionally using & when href contains ?) fixes a real logic bug.
Preferably this should use a proper URL/query builder (URL or next/link's object form) to avoid edge cases, but the suggested patch is a valid, minimal fix for the reported problem.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** packages/ui/components/form/step/Stepper.tsx
**Line:** 31:55
**Comment:**
	*Logic Error: Building the step URLs by blindly appending `?step=${index + 1}` to the `href` string will break when the base `href` already contains query parameters (e.g. `/auth/setup?mode=admin`), resulting in invalid URLs like `/auth/setup?mode=admin?step=2` and incorrect routing behavior; you should append `step` with `&` when a `?` already exists.

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.

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>
)}
</li>
))}
</ol>
Expand Down
2 changes: 1 addition & 1 deletion packages/ui/components/list/List.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export function ListItem(props: ListItemProps) {
);

return href ? (
<Link passHref href={href} legacyBehavior>
<Link href={href}>
{element}
</Link>
) : (
Expand Down