-
Notifications
You must be signed in to change notification settings - Fork 10.5k
fix: multiple widgets for Booker
atom
#22925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 30 commits
5961240
fe7bd68
727bc29
649a9b3
de30ff0
a1c8765
40d9495
ab39b6c
1adbd90
c274667
74b0fee
eaa4cf1
ad040ae
5c7963c
921c074
5200d69
2f4484e
9faa02e
4a259b3
f716670
bd1ed1a
87d837f
12947ca
8ccdd02
4e2ad5a
e81eeff
14d85d9
b48345b
8e6f937
9f24f02
69ff62d
d677f5d
a27e129
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
"@calcom/atoms": patch | ||
--- | ||
|
||
Fixes an issues for the Booker atom wherein when multiple widgets were being placed on the same page, changes made in one widget would also get reflected in the others. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
"use client"; | ||
|
||
import { createContext, useContext, useRef, type ReactNode, useEffect } from "react"; | ||
import { useStore } from "zustand"; | ||
import type { StoreApi } from "zustand"; | ||
|
||
import { createBookerStore, type BookerStore, type StoreInitializeType } from "./store"; | ||
|
||
export const BookerStoreContext = createContext<StoreApi<BookerStore> | null>(null); | ||
|
||
export interface BookerStoreProviderProps { | ||
children: ReactNode; | ||
} | ||
|
||
export const BookerStoreProvider = ({ children }: BookerStoreProviderProps) => { | ||
const storeRef = useRef<StoreApi<BookerStore>>(); | ||
if (!storeRef.current) { | ||
storeRef.current = createBookerStore(); | ||
} | ||
|
||
return <BookerStoreContext.Provider value={storeRef.current}>{children}</BookerStoreContext.Provider>; | ||
}; | ||
|
||
export const useBookerStoreContext = <T,>( | ||
selector: (store: BookerStore) => T, | ||
equalityFn?: (a: T, b: T) => boolean | ||
): T => { | ||
const bookerStoreContext = useContext(BookerStoreContext); | ||
|
||
if (!bookerStoreContext) { | ||
throw new Error("useBookerStoreContext must be used within BookerStoreProvider"); | ||
} | ||
|
||
return useStore(bookerStoreContext, selector, equalityFn); | ||
}; | ||
|
||
export const useInitializeBookerStoreContext = ({ | ||
username, | ||
eventSlug, | ||
month, | ||
eventId, | ||
rescheduleUid = null, | ||
rescheduledBy = null, | ||
bookingData = null, | ||
verifiedEmail = null, | ||
layout, | ||
isTeamEvent, | ||
durationConfig, | ||
org, | ||
isInstantMeeting, | ||
timezone = null, | ||
teamMemberEmail, | ||
crmOwnerRecordType, | ||
crmAppSlug, | ||
crmRecordId, | ||
isPlatform = false, | ||
allowUpdatingUrlParams = true, | ||
}: StoreInitializeType) => { | ||
const bookerStoreContext = useContext(BookerStoreContext); | ||
|
||
if (!bookerStoreContext) { | ||
throw new Error("useInitializeBookerStoreContext must be used within BookerStoreProvider"); | ||
} | ||
|
||
const initializeStore = useStore(bookerStoreContext, (state) => state.initialize); | ||
|
||
useEffect(() => { | ||
initializeStore({ | ||
username, | ||
eventSlug, | ||
month, | ||
eventId, | ||
rescheduleUid, | ||
rescheduledBy, | ||
bookingData, | ||
layout, | ||
isTeamEvent, | ||
org, | ||
verifiedEmail, | ||
durationConfig, | ||
isInstantMeeting, | ||
timezone, | ||
teamMemberEmail, | ||
crmOwnerRecordType, | ||
crmAppSlug, | ||
crmRecordId, | ||
isPlatform, | ||
allowUpdatingUrlParams, | ||
}); | ||
}, [ | ||
initializeStore, | ||
org, | ||
username, | ||
eventSlug, | ||
month, | ||
eventId, | ||
rescheduleUid, | ||
rescheduledBy, | ||
bookingData, | ||
layout, | ||
isTeamEvent, | ||
verifiedEmail, | ||
durationConfig, | ||
isInstantMeeting, | ||
timezone, | ||
teamMemberEmail, | ||
crmOwnerRecordType, | ||
crmAppSlug, | ||
crmRecordId, | ||
isPlatform, | ||
allowUpdatingUrlParams, | ||
]); | ||
Comment on lines
+67
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainConsider the impact of frequent re-initializations The Consider whether all these dependencies are necessary. Typically, initialization should only occur when key identifiers change (e.g., 🏁 Script executed: #!/bin/bash
# Check how the initialize method handles re-initialization in the store
ast-grep --pattern 'initialize: (data: StoreInitializeType) => {
$$$
}' Length of output: 75 I didn’t find an 🏁 Script executed: #!/bin/bash
# Search for where initializeStore is defined or imported
rg -n "initializeStore" --glob "*.ts*" Length of output: 875 Let’s inspect the 🏁 Script executed: #!/bin/bash
# Show the initialize method in Booker store
sed -n '460,550p' packages/features/bookings/Booker/store.ts Length of output: 1233 Let’s inspect the actual 🏁 Script executed: #!/bin/bash
# Locate and display the initialize action implementation
rg -n -C 5 "initialize:" packages/features/bookings/Booker/store.ts Length of output: 883 Inspecting the full 🏁 Script executed: #!/bin/bash
# Extract the body of the initialize action
sed -n '295,380p' packages/features/bookings/Booker/store.ts Length of output: 2812 Align The
To resolve this mismatch, choose one of:
Places to update:
🤖 Prompt for AI Agents
|
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,18 +10,17 @@ import "@calcom/features/bookings/Booker/components/__mocks__/Section"; | |
import { constantsScenarios } from "@calcom/lib/__mocks__/constants"; | ||
import "@calcom/lib/__mocks__/logger"; | ||
|
||
import { render, screen } from "@testing-library/react"; | ||
import React from "react"; | ||
import { vi } from "vitest"; | ||
|
||
import "@calcom/dayjs/__mocks__"; | ||
import "@calcom/features/auth/Turnstile"; | ||
|
||
import { Booker } from "../Booker"; | ||
import { useBookerStore } from "../store"; | ||
import type { BookerState } from "../types"; | ||
import { render, screen } from "./test-utils"; | ||
|
||
Comment on lines
+20
to
21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add a test that verifies per-instance store isolation (core PR objective) Good to see the custom render wrapper. However, there’s no test asserting that two Booker instances on the same page don’t share state. Please add one. Here’s a starting point you can adapt to your test-utils and provider API: @@
describe("Booker", () => {
@@
it("should render DryRunMessage when in dry run mode", () => {
@@
});
+ it("isolates state across multiple Booker instances on the same page", () => {
+ const leftProps = {
+ ...defaultProps,
+ eventSlug: "left-event",
+ slots: {
+ ...defaultProps.slots,
+ setSelectedTimeslot: vi.fn(),
+ },
+ };
+ const rightProps = {
+ ...defaultProps,
+ eventSlug: "right-event",
+ slots: {
+ ...defaultProps.slots,
+ setSelectedTimeslot: vi.fn(),
+ },
+ };
+
+ // Render two instances; ensure each is wrapped with its own store/provider
+ // If your test-utils render() only supports a single mockStore,
+ // render them in two separate calls to simulate two widgets on one page.
+ const left = render(<Booker {...leftProps} />, {
+ mockStore: {
+ state: "booking",
+ selectedDate: "2024-01-01",
+ selectedTimeslot: "2024-01-01T10:00:00Z",
+ tentativeSelectedTimeslots: ["2024-01-01T10:00:00Z"],
+ },
+ });
+ const right = render(<Booker {...rightProps} />, {
+ mockStore: {
+ state: "booking",
+ selectedDate: "2024-01-02",
+ selectedTimeslot: "2024-01-02T10:00:00Z",
+ tentativeSelectedTimeslots: ["2024-01-02T10:00:00Z"],
+ },
+ });
+
+ // Act on the left instance (e.g., cancel)
+ left.getByRole("button", { name: /cancel/i }).click();
+
+ // Assert: left instance props updated, right instance untouched
+ expect((leftProps.slots.setSelectedTimeslot as any)).toHaveBeenCalledWith(null);
+ expect((rightProps.slots.setSelectedTimeslot as any)).not.toHaveBeenCalled();
+ }); If you prefer a single render with both siblings, ensure your test-utils can supply separate store providers per instance; I can help wire that up. 🤖 Prompt for AI Agents
|
||
vi.mock("framer-motion", async (importOriginal) => { | ||
const actual = await importOriginal(); | ||
const actual = (await importOriginal()) as any; | ||
return { | ||
...actual, | ||
}; | ||
|
@@ -87,27 +86,6 @@ vi.mock("@calcom/atoms/hooks/useIsPlatform", () => ({ | |
useIsPlatform: () => false, | ||
})); | ||
|
||
// Update mockStoreState to include all required state | ||
const mockStoreState = { | ||
state: "booking" as BookerState, | ||
setState: vi.fn(), | ||
selectedDate: "2024-01-01", | ||
seatedEventData: {}, | ||
setSeatedEventData: vi.fn(), | ||
tentativeSelectedTimeslots: [], | ||
setTentativeSelectedTimeslots: vi.fn(), | ||
dayCount: 7, | ||
setDayCount: vi.fn(), | ||
setSelectedTimeslot: vi.fn(), | ||
selectedTimeslot: null, | ||
formStep: 0, | ||
setFormStep: vi.fn(), | ||
bookerState: "booking", | ||
setBookerState: vi.fn(), | ||
layout: "default", | ||
setLayout: vi.fn(), | ||
}; | ||
|
||
// Update defaultProps to include missing required props | ||
const defaultProps = { | ||
username: "testuser", | ||
|
@@ -188,32 +166,21 @@ const defaultProps = { | |
describe("Booker", () => { | ||
beforeEach(() => { | ||
constantsScenarios.set({ | ||
PUBLIC_QUICK_AVAILABILITY_ROLLOUT: 100, | ||
PUBLIC_QUICK_AVAILABILITY_ROLLOUT: "100", | ||
POWERED_BY_URL: "https://go.cal.com/booking", | ||
APP_NAME: "Cal.com", | ||
}); | ||
vi.clearAllMocks(); | ||
}); | ||
|
||
it("should render null when in loading state", () => { | ||
useBookerStore.setState({ | ||
...mockStoreState, | ||
state: "loading", | ||
const { container } = render(<Booker {...defaultProps} />, { | ||
mockStore: { state: "loading" }, | ||
}); | ||
|
||
const { container } = render(<Booker {...defaultProps} />); | ||
expect(container).toBeEmptyDOMElement(); | ||
}); | ||
|
||
it("should render DryRunMessage when in dry run mode", () => { | ||
useBookerStore.setState({ | ||
...mockStoreState, | ||
state: "selecting_time", | ||
selectedDate: "2024-01-01", | ||
selectedTimeslot: "2024-01-01T10:00:00Z", | ||
tentativeSelectedTimeslots: ["2024-01-01T10:00:00Z"], | ||
}); | ||
|
||
const propsWithDryRun = { | ||
...defaultProps, | ||
isBookingDryRun: true, | ||
|
@@ -226,7 +193,14 @@ describe("Booker", () => { | |
}, | ||
}; | ||
|
||
render(<Booker {...propsWithDryRun} />); | ||
render(<Booker {...propsWithDryRun} />, { | ||
mockStore: { | ||
state: "selecting_time", | ||
selectedDate: "2024-01-01", | ||
selectedTimeslot: "2024-01-01T10:00:00Z", | ||
tentativeSelectedTimeslots: ["2024-01-01T10:00:00Z"], | ||
}, | ||
}); | ||
expect(screen.getByTestId("dry-run-message")).toBeInTheDocument(); | ||
}); | ||
|
||
|
@@ -242,12 +216,10 @@ describe("Booker", () => { | |
invalidate: mockInvalidate, | ||
}, | ||
}; | ||
useBookerStore.setState({ | ||
...mockStoreState, | ||
state: "booking", | ||
}); | ||
|
||
render(<Booker {...propsWithInvalidate} />); | ||
render(<Booker {...propsWithInvalidate} />, { | ||
mockStore: { state: "booking" }, | ||
}); | ||
screen.logTestingPlaygroundURL(); | ||
// Trigger form cancel | ||
const cancelButton = screen.getByRole("button", { name: /cancel/i }); | ||
|
@@ -267,12 +239,9 @@ describe("Booker", () => { | |
}, | ||
}; | ||
|
||
useBookerStore.setState({ | ||
...mockStoreState, | ||
state: "booking", | ||
render(<Booker {...propsWithQuickChecks} />, { | ||
mockStore: { state: "booking" }, | ||
}); | ||
|
||
render(<Booker {...propsWithQuickChecks} />); | ||
const bookEventForm = screen.getByTestId("book-event-form"); | ||
await expect(bookEventForm).toHaveAttribute("data-unavailable", "true"); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Prop threading sanity-check
Passing
timeslot={selectedTimeslot}
to BookEventForm andselectedTimeslot
to EventMeta requires updated prop types/usages downstream. If not already done, ensure those components accept and use these props.Run:
Also applies to: 407-407
🏁 Script executed:
Length of output: 904
🏁 Script executed:
Length of output: 10075
🏁 Script executed:
Length of output: 2807
🏁 Script executed:
Length of output: 4048
Update EventMeta to accept
selectedTimeslot
It looks like you’ve correctly threaded the
timeslot
prop intoBookEventForm
(its props type already includestimeslot: string | null
), but theEventMeta
component itself doesn’t define or consume aselectedTimeslot
prop. You’ll need to:• In
packages/features/bookings/Booker/components/EventMeta.tsx
, addselectedTimeslot?: string | null
(or required) to its props definition.• Destructure and use
selectedTimeslot
insideEventMeta
wherever you need to display or conditionally render based on the currently selected slot.This will keep the type system happy and ensure
EventMeta
actually responds to the timeslot state you’re passing in.🤖 Prompt for AI Agents