Skip to content

Comments

fix: incorrect-slot-length-on-reschedule#23300

Closed
hemantmm wants to merge 7 commits intocalcom:mainfrom
hemantmm:slot-length
Closed

fix: incorrect-slot-length-on-reschedule#23300
hemantmm wants to merge 7 commits intocalcom:mainfrom
hemantmm:slot-length

Conversation

@hemantmm
Copy link
Contributor

closes: #23284

What does this PR do?

Visual Demo (For contributors especially)

A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).

Video Demo (if applicable):

Screen.Recording.2025-08-23.at.15.57.03.mov

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Are there environment variables that should be set?
  • What are the minimal test data to have?
  • What is expected (happy path) to have (input and output)?
  • Any other important info that could help to test that PR

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

@vercel
Copy link

vercel bot commented Aug 23, 2025

@hemantmm is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 23, 2025

Walkthrough

The PR updates packages/features/bookings/components/event-meta/Duration.tsx to ensure selectedDuration is initialized from event.length and kept in sync when event.length changes. It adds a second useEffect to update selectedDuration on event.length updates (respecting event.metadata?.multipleDuration and isDynamicEvent) and expands the initialization effect dependencies to include state. Rendering logic remains: in "booking" state only the selectedDuration is shown; otherwise all durations. No public API changes.

Assessment against linked issues

Objective Addressed Explanation
Fix incorrect slot length when event type length changes prior to rescheduling (GitHub #23284 / Linear CAL-6307)

Possibly related PRs

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d9f8e9a and ca4e8c1.

📒 Files selected for processing (1)
  • packages/features/bookings/components/event-meta/Duration.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/features/bookings/components/event-meta/Duration.tsx
⏰ 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). (1)
  • GitHub Check: Codacy Static Code Analysis
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@graphite-app graphite-app bot requested a review from a team August 23, 2025 10:31
@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Aug 23, 2025
@github-actions github-actions bot added booking-page area: booking page, public booking page, booker event-types area: event types, event-types 🐛 bug Something isn't working labels Aug 23, 2025
@dosubot dosubot bot added the bookings area: bookings, availability, timezones, double booking label Aug 23, 2025
@graphite-app
Copy link

graphite-app bot commented Aug 23, 2025

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (08/23/25)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (08/23/25)

1 label was added to this PR based on Keith Williams's automation.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/features/bookings/components/event-meta/Duration.tsx (1)

86-99: Remove duplicate initialization branch to avoid double sets and render churn.

This effect re-applies initialization when !selectedDuration and (multi-duration or dynamic). The first effect already handles the “no selection” case. Keeping both can trigger back-to-back identical setSelectedDuration calls.

   useEffect(() => {
     // If the event length changes (e.g., after rescheduling), update selectedDuration
     if (
       selectedDuration !== event.length &&
       (!event.metadata?.multipleDuration || event.metadata.multipleDuration.includes(event.length))
     ) {
       setSelectedDuration(event.length);
     }
-    // If no selectedDuration and event has multiple durations or is dynamic, set to event.length
-    if (!selectedDuration && (event.metadata?.multipleDuration || isDynamicEvent)) {
-      setSelectedDuration(event.length);
-    }
   }, [selectedDuration, setSelectedDuration, event.metadata?.multipleDuration, event.length, isDynamicEvent]);

Optional follow-up: we could merge both effects into a single effect to centralize the sync logic and simplify mental model. Happy to provide that refactor if you prefer.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d2f8076 and 04cab88.

📒 Files selected for processing (1)
  • packages/features/bookings/components/event-meta/Duration.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/features/bookings/components/event-meta/Duration.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/features/bookings/components/event-meta/Duration.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/features/bookings/components/event-meta/Duration.tsx
⏰ 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 (2)
packages/features/bookings/components/event-meta/Duration.tsx (2)

72-85: (Awaiting script results...)


72-74: Confirm rescheduling detection logic

The store’s initialize method handles reschedules by checking for rescheduleUid && bookingData and then clearing the selected timeslot and setting selectedDuration, but it doesn’t update the store’s state to "booking". Meanwhile, Duration.tsx uses state === "booking" to detect rescheduling, yet the store’s contract doesn’t guarantee that rescheduling ever sets the state to "booking".

• In packages/features/bookings/Booker/store.ts (inside the initialize method’s reschedule block), you update duration but never call setState("booking").
• In packages/features/bookings/Booker/types.ts, export type BookerState = "loading" | "selecting_date" | "selecting_time" | "booking"; shows “booking” is a valid state, but its relation to reschedules isn’t defined by the store itself.

Please verify in the UI code (Booker.tsx) whether on initialization with a non‐null rescheduleUid the component explicitly calls setState("booking"). If it does not, the if (state === "booking") branch in Duration.tsx will never fire during an actual reschedule flow.

Recommendations:

  • If the intent is to treat rescheduling as the “booking” state, update the UI to invoke setState("booking") when rescheduleUid is present.
  • Otherwise, modify the condition in Duration.tsx to also check the store’s rescheduleUid (e.g. if (rescheduleUid || !selectedDuration)) to ensure the original duration is applied on reschedule.

Signed-off-by: Hemant M Mehta <hemant29mehta@gmail.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
packages/features/bookings/components/event-meta/Duration.tsx (3)

86-95: Redundant second effect and noisy dependencies.

This effect largely overlaps with the initialization intent. It’s fine to keep it, but:

  • isDynamicEvent isn’t used in the body; drop it from deps.
  • Tracking multipleDuration by reference can cause unnecessary re-runs if a new array instance is passed each render. Track a stable key instead.

Apply this diff:

-  }, [selectedDuration, setSelectedDuration, event.metadata?.multipleDuration, event.length, isDynamicEvent]);
+  }, [
+    selectedDuration,
+    setSelectedDuration,
+    event.metadata?.multipleDuration?.join(","),
+    event.length,
+  ]);

Optional: You can merge both effects into a single effect that:

  • Initializes when !selectedDuration.
  • Syncs when event.length changes and is allowed by multipleDuration.
    This reduces re-renders and avoids potential double setSelectedDuration on mount.

114-116: Guarantee the selected duration exists in the rendered list.

If event.length isn’t in multipleDuration, the list can render no active chip while selectedDuration points to a value not present in durations. Ensure the chip list always contains the current event.length.

-  const durations = event?.metadata?.multipleDuration || [15, 30, 60, 90];
+  const baseDurations = event?.metadata?.multipleDuration || [15, 30, 60, 90];
+  const durations = baseDurations.includes(event.length)
+    ? baseDurations
+    : [...baseDurations, event.length].sort((a, b) => a - b);

118-125: Add accessible, localized labels for the scroll buttons.

Buttons use only icons, so they’re invisible to screen readers. Add aria-label (and optionally title) using t() per guidelines.

-        <button onClick={handleLeft} className="absolute bottom-0 left-0 flex">
+        <button
+          onClick={handleLeft}
+          aria-label={t("scroll_left")}
+          title={t("scroll_left")}
+          className="absolute bottom-0 left-0 flex"
+        >

Similarly for the right button (Lines 148-153):

-        <button onClick={handleRight} className="absolute bottom-0 right-0 flex">
+        <button
+          onClick={handleRight}
+          aria-label={t("scroll_right")}
+          title={t("scroll_right")}
+          className="absolute bottom-0 right-0 flex"
+        >
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 04cab88 and 5a7fae5.

📒 Files selected for processing (1)
  • packages/features/bookings/components/event-meta/Duration.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/features/bookings/components/event-meta/Duration.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/features/bookings/components/event-meta/Duration.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/features/bookings/components/event-meta/Duration.tsx
⏰ 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 (1)
packages/features/bookings/components/event-meta/Duration.tsx (1)

72-84: Don’t override the user’s chosen duration when entering “booking” state (add tests to cover multi-duration flows)

I searched for any existing tests around the Duration component or selectedDuration behavior and found none—so we need to both land this refactor and add tests to prevent regressions.

What to do:

  • Apply the refactor to only initialize when there’s no duration yet and respect allowed multipleDuration values:
       // Sets initial value of selected duration to the default duration.
       useEffect(() => {
  •  // Check if we're rescheduling (state is "booking") or if there's no selected duration yet
    
  •  if (state === "booking" || !selectedDuration) {
    
  •    // Always use the current event length when rescheduling or initializing
    
  •    setSelectedDuration(event.length);
    
  •  }
    
  •  // Initialize only once when nothing is selected yet; respect allowed durations if present
    
  •  if (!selectedDuration) {
    
  •    const allowed = event.metadata?.multipleDuration;
    
  •    const next =
    
  •      !allowed || allowed.includes(event.length) ? event.length : allowed[0];
    
  •    setSelectedDuration(next);
    
  •  }
    
  • }, [
  • selectedDuration,
  • setSelectedDuration,
  • event.metadata?.multipleDuration,
  • event.length,
  • isDynamicEvent,
  • state,
  • ]);
  • }, [
  • selectedDuration,
  • setSelectedDuration,
  • event.metadata?.multipleDuration,
  • event.length,
  • ]);

- **Add tests** under  
`packages/features/bookings/components/event-meta/Duration.test.tsx`  
to cover both:
1. Initial booking (state “booking”) with a non-default choice persists across steps.  
2. Rescheduling and dynamic length updates still sync `selectedDuration` when the event length changes.  

Ensure your tests drive the component through the “booking” state with a custom duration (e.g. 30 when default is 60) and assert it remains 30.

</blockquote></details>

</blockquote></details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Signed-off-by: Hemant M Mehta <hemant29mehta@gmail.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/features/bookings/components/event-meta/Duration.tsx (1)

86-98: Deduplicate effects: the second effect overlaps with the first

This second effect now duplicates the synchronization logic and can cause extra renders. After tightening the first effect, you can remove this one to keep state updates single-sourced.

-  useEffect(() => {
-    // If the event length changes (e.g., after rescheduling), update selectedDuration
-    if (
-      selectedDuration !== event.length &&
-      (!event.metadata?.multipleDuration || event.metadata.multipleDuration.includes(event.length))
-    ) {
-      setSelectedDuration(event.length);
-    }
-    // If no selectedDuration and event has multiple durations or is dynamic, set to event.length
-    if (!selectedDuration && (event.metadata?.multipleDuration || isDynamicEvent)) {
-      setSelectedDuration(event.length);
-    }
-  }, [selectedDuration, setSelectedDuration, event.metadata?.multipleDuration, event.length, isDynamicEvent]);
+  // Removed: handled by the initialization/reschedule effect above.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5a7fae5 and 7f309e1.

📒 Files selected for processing (1)
  • packages/features/bookings/components/event-meta/Duration.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/features/bookings/components/event-meta/Duration.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/features/bookings/components/event-meta/Duration.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/features/bookings/components/event-meta/Duration.tsx
⏰ 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

Signed-off-by: Hemant M Mehta <hemant29mehta@gmail.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/features/bookings/components/event-meta/Duration.tsx (1)

135-136: Align test attributes to data-testid
The project predominantly uses the lowercase data-testid across components and tests. Please update the camel-cased instances in Duration.tsx to match:

• In packages/features/bookings/components/event-meta/Duration.tsx (lines 135–136), change

- data-testId={`multiple-choice-${duration}mins`}
+ data-testid={`multiple-choice-${duration}mins`}
- data-active={selectedDuration === duration ? "true" : "false"}
+ data-active={selectedDuration === duration ? "true" : "false"}  // unaffected

This will ensure consistency with the Testing Library convention and the rest of the codebase.

🧹 Nitpick comments (6)
packages/features/bookings/components/event-meta/Duration.tsx (6)

115-116: Dynamic events: ensure the selectedDuration is visible even if not in the default list.

When isDynamicEvent is true and metadata.multipleDuration is absent, durations defaults to [15, 30, 60, 90]. If event.length is, say, 45, the list can render empty during “booking” because the filter keeps only dur === selectedDuration. Include the selectedDuration into the list when missing.

-  const durations = event?.metadata?.multipleDuration || [15, 30, 60, 90];
+  const baseDurations = event?.metadata?.multipleDuration || [15, 30, 60, 90];
+  const durations =
+    isDynamicEvent && selectedDuration && !baseDurations.includes(selectedDuration)
+      ? [selectedDuration, ...baseDurations]
+      : baseDurations;

131-147: Use stable, semantic keys for list items.

Using index as a key can cause unnecessary re-renders if the array changes. The duration value is unique and stable here.

-              key={index}
+              key={duration}

46-46: Key refs by duration using a map-like type (avoid sparse arrays).

itemRefs is indexed by “duration” values (e.g., 15, 30, 60), which creates a sparse array. A map-like type is clearer and avoids accidental array operations.

-  const itemRefs = useRef<(HTMLLIElement | null)[]>([]);
+  const itemRefs = useRef<Record<number, HTMLLIElement | null>>({});

The existing usages (itemRefs.current[duration] = el, itemRefs.current[selectedDuration]) remain valid.


129-129: Pass the scroll handler directly.

Saves a closure per render and is more idiomatic.

-        onScroll={(e) => calculateScroll(e)}
+        onScroll={calculateScroll}

68-69: Simplify dynamic flag extraction.

Since isDynamic is part of the picked type, the in-check is redundant.

-  const isDynamicEvent = "isDynamic" in event && event.isDynamic;
+  const isDynamicEvent = Boolean(event.isDynamic);

14-19: Avoid mutating the mins parameter for clarity.

Minor readability improvement: compute the remainder into a new variable.

 export const getDurationFormatted = (mins: number | undefined, t: TFunction) => {
   if (!mins) return null;
 
   const hours = Math.floor(mins / 60);
-  mins %= 60;
+  const remMins = mins % 60;
   // format minutes string
   let minStr = "";
-  if (mins > 0) {
+  if (remMins > 0) {
     minStr =
-      mins === 1
-        ? t("minute_one_short", { count: 1 })
-        : t("multiple_duration_timeUnit_short", { count: mins, unit: "minute" });
+      remMins === 1
+        ? t("minute_one_short", { count: 1 })
+        : t("multiple_duration_timeUnit_short", { count: remMins, unit: "minute" });
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 7f309e1 and a65350b.

📒 Files selected for processing (1)
  • packages/features/bookings/components/event-meta/Duration.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/features/bookings/components/event-meta/Duration.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/features/bookings/components/event-meta/Duration.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/features/bookings/components/event-meta/Duration.tsx
⏰ 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 (2)
packages/features/bookings/components/event-meta/Duration.tsx (2)

72-81: Good guard: initializes and resyncs selectedDuration only when needed (addresses the reschedule bug).

This effect now correctly:

  • Checks whether the new length is allowed by multipleDuration (with a sensible fallback).
  • Avoids redundant setSelectedDuration when the value hasn’t changed.
  • Covers reschedule path via state === "booking" and initial mount via !selectedDuration.

This aligns with the PR objective to reflect the updated event length during rescheduling.

If multipleDuration can be empty or unsorted in some flows, confirm that falling back to multipleDuration[0] is acceptable UX. If not, prefer a deterministic strategy (e.g., nearest to event.length).


117-157: Sanity check: filter behavior during reschedule.

The filter .filter((dur) => state !== "booking" || dur === selectedDuration) intentionally collapses the list to the current selection while rescheduling. Confirm this matches intended UX post-fix (i.e., user shouldn’t change duration during reschedule). If duration changes should be allowed during reschedule, the filter needs revisiting.

Signed-off-by: Hemant M Mehta <hemant29mehta@gmail.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (2)
packages/features/bookings/components/event-meta/Duration.tsx (2)

86-98: Remove redundant sync effect; it can set an invalid duration and double-update state

This second effect overlaps with the first, risks setting selectedDuration to a value not in multipleDuration, and adds extra renders. The first effect (once fixed) already handles initialization and reschedule sync safely.

Apply this diff to delete the redundant effect:

-  useEffect(() => {
-    // If the event length changes (e.g., after rescheduling), update selectedDuration
-    if (
-      selectedDuration !== event.length &&
-      (!event.metadata?.multipleDuration || event.metadata.multipleDuration.includes(event.length))
-    ) {
-      setSelectedDuration(event.length);
-    }
-    // If no selectedDuration and event has multiple durations or is dynamic, set to event.length
-    if (!selectedDuration && (event.metadata?.multipleDuration || isDynamicEvent)) {
-      setSelectedDuration(event.length);
-    }
-  }, [selectedDuration, setSelectedDuration, event.metadata?.multipleDuration, event.length, isDynamicEvent]);

72-84: Unconditional setSelectedDuration + empty if-block cause redundant renders; tighten guard and honor allowed durations

The if block is empty and setSelectedDuration(event.length) is called unconditionally every render of this effect. This can cause unnecessary renders (or a render loop depending on the store’s equality checks) and ignores multipleDuration constraints. Also, isDynamicEvent is listed in deps but unused.

Apply this diff to make the effect idempotent, align the selection with allowed durations, and drop the unused dep:

-  // Sets initial value of selected duration to the default duration.
-  useEffect(() => {
-    // Check if we're rescheduling (state is "booking") or if there's no selected duration yet
-    if (state === "booking" || !selectedDuration) {
-      // Always use the current event length when rescheduling or initializing
-    }
-    setSelectedDuration(event.length);
-  }, [
-    selectedDuration,
-    setSelectedDuration,
-    event.metadata?.multipleDuration,
-    event.length,
-    isDynamicEvent,
-    state,
-  ]);
+  // Initialize or sync selection (esp. during reschedule) only when needed and valid.
+  useEffect(() => {
+    const allowed =
+      !event.metadata?.multipleDuration ||
+      event.metadata.multipleDuration.includes(event.length);
+    const next = allowed ? event.length : event.metadata?.multipleDuration?.[0];
+
+    if ((state === "booking" || !selectedDuration) && next && selectedDuration !== next) {
+      setSelectedDuration(next);
+    }
+  }, [selectedDuration, setSelectedDuration, event.metadata?.multipleDuration, event.length, state]);
🧹 Nitpick comments (1)
packages/features/bookings/components/event-meta/Duration.tsx (1)

134-149: Stabilize keys and align test attribute; optional defensive guard for booking state

  • Use duration as the React key instead of index for stability.
  • Standardize to data-testid (if that’s what your tests expect).
  • Optional: once the first effect is fixed, the UI shouldn’t empty out; still, a tiny guard can prevent an empty list if selection ever drifts.

Apply this diff:

-        {durations
-          .filter((dur) => state !== "booking" || dur === selectedDuration)
-          .map((duration, index) => (
+        {durations
+          .filter((dur) => state !== "booking" || dur === selectedDuration)
+          .map((duration) => (
             <li
-              data-testId={`multiple-choice-${duration}mins`}
+              data-testid={`multiple-choice-${duration}mins`}
               data-active={selectedDuration === duration ? "true" : "false"}
-              key={index}
+              key={duration}
               onClick={() => setSelectedDuration(duration)}
               ref={(el) => (itemRefs.current[duration] = el)}

Optional defensive guard (if you want belt-and-suspenders despite the effect fix):

-        {durations
-          .filter((dur) => state !== "booking" || dur === selectedDuration)
+        {(
+          state === "booking"
+            ? (durations.includes(selectedDuration!) ? [selectedDuration!] : [])
+            : durations
+        )
           .map((duration) => (

Verification: please confirm whether your test suite uses data-testid or data-testId. If it’s the former, this change will make the element discoverable by RTL queries.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a65350b and d9f8e9a.

📒 Files selected for processing (1)
  • packages/features/bookings/components/event-meta/Duration.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/features/bookings/components/event-meta/Duration.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/features/bookings/components/event-meta/Duration.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/features/bookings/components/event-meta/Duration.tsx
⏰ 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

Signed-off-by: Hemant M Mehta <hemant29mehta@gmail.com>
Signed-off-by: Hemant M Mehta <hemant29mehta@gmail.com>
@hemantmm
Copy link
Contributor Author

Hey @eunjae-lee, can you have a look at this PR?

@kart1ka
Copy link
Contributor

kart1ka commented Aug 28, 2025

Thanks for your contribution. But we are going with #23290.

@kart1ka kart1ka closed this Aug 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

booking-page area: booking page, public booking page, booker bookings area: bookings, availability, timezones, double booking 🐛 bug Something isn't working community Created by Linear-GitHub Sync event-types area: event types, event-types

Projects

None yet

Development

Successfully merging this pull request may close these issues.

incorrect slot length when changing event type length and then rescheduling it

2 participants