fix: Plain chat not opening from help button for free users and on bookings list page#23491
fix: Plain chat not opening from help button for free users and on bookings list page#23491anikdhabal merged 3 commits intomainfrom
Conversation
WalkthroughPlainContactForm was refactored to be a controlled component accepting props Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
| e?.preventDefault(); | ||
| e?.stopPropagation(); | ||
|
|
||
| setMenuOpen(false); | ||
|
|
||
| // Defer to next frame to avoid the originating click closing the dialog | ||
| requestAnimationFrame(() => { | ||
| // double RAF is extra-safe if your dropdown unmounts with a transition | ||
| requestAnimationFrame(() => { | ||
| if (window.Plain) window.Plain.open(); | ||
| }); |
There was a problem hiding this comment.
Avoids closing the PlainContactForm component
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (4)
apps/web/components/plain/PlainContactForm.tsx (3)
69-73: Fix upload error handling: await response text, don’t stop “uploading” early, and clean up failed placeholders.
uploadRes.textmust be awaited.setIsUploadingImage(false)is called before the actual upload; defer until success/failure.- Remove the optimistic placeholder on pre-upload failure.
Apply this diff:
@@ - setIsUploadingImage(false); + // keep uploading state until the actual file POST completes @@ - if (!res.ok) { - showToast("Error uploading attachment", "error"); - setIsUploadingImage(false); - return; - } + if (!res.ok) { + showToast("Error uploading attachment", "error"); + // remove optimistic entry + setUploads((prev) => prev.filter((u) => u.id !== newFile.id)); + setIsUploadingImage(false); + return; + } @@ - if (!uploadRes.ok) { - showToast(`Failed while uploading file: ${uploadRes.text}`, "error"); - setIsUploadingImage(false); - return; - } + if (!uploadRes.ok) { + const errText = await uploadRes.text().catch(() => uploadRes.statusText); + showToast(`Failed while uploading file: ${errText}`, "error"); + // remove optimistic entry + setUploads((prev) => prev.filter((u) => u.id !== newFile.id)); + setIsUploadingImage(false); + return; + }Also applies to: 55-55, 43-47
34-37: Process all added/removed files; don’t ignore subsequent items.Currently only the first item of
newFiles/removedFilesis handled.Apply this diff:
- const handleUpload = async (allFiles: FileData[], newFiles: FileData[], removedFiles: FileData[]) => { - if (newFiles.length > 0) { - const newFile = newFiles[0]; + const handleUpload = async (_allFiles: FileData[], newFiles: FileData[], removedFiles: FileData[]) => { + // Handle added files + for (const newFile of newFiles) { setUploads((prev) => [...prev, { file: newFile.file, uploading: true, id: newFile.id }]); setIsUploadingImage(true); @@ - setUploads((prev) => - prev.map((upload) => (upload.file === file ? { ...upload, uploading: false, attachmentId } : upload)) - ); + setUploads((prev) => + prev.map((u) => (u.id === newFile.id ? { ...u, uploading: false, attachmentId } : u)) + ); setData((prev) => ({ ...prev, attachmentIds: [...prev.attachmentIds, attachmentId], })); setIsUploadingImage(false); showToast("File uploaded successfully", "success"); - } else if (removedFiles.length > 0) { - const removedFile = removedFiles[0]; - const file = uploads.find((upload) => upload.id === removedFile.id); - if (!file) { - console.warn("File not found in uploads: ", removedFile.id); - return; - } - setData((prev) => ({ - ...prev, - attachmentIds: prev.attachmentIds.filter((id) => id !== file.attachmentId), - })); - setUploads((prev) => prev.filter((upload) => upload.id !== removedFile.id)); - } + } + // Handle removed files + for (const removedFile of removedFiles) { + const entry = uploads.find((u) => u.id === removedFile.id); + if (!entry) continue; + setData((prev) => ({ + ...prev, + attachmentIds: prev.attachmentIds.filter((id) => id !== entry.attachmentId), + })); + setUploads((prev) => prev.filter((u) => u.id !== removedFile.id)); + }Also applies to: 75-82, 84-97
196-199: Mismatch: multiple=false but maxFiles=5.Either set
multipleto true or reducemaxFilesto 1. Likely the intent is multiple attachments.Apply this diff:
- multiple={false} + multiple showFilesList - maxFiles={5} + maxFiles={5}apps/web/components/plain/plainChat.tsx (1)
252-259: Async cleanup from initConfig is ignored by useEffect — timer leak risk.Returning a cleanup function from an async function doesn’t register with useEffect; the setTimeout may survive unmounts.
Apply this diff within the range to store the timer and avoid returning a cleanup from the async function:
- if (shouldOpenPlain) { - const timer = setTimeout(() => { - if (window.Plain) { - window.Plain.open(); - } - }, 100); - return () => clearTimeout(timer); - } + if (shouldOpenPlain) { + // Defer open; cleanup handled in the effect's cleanup via openTimerRef + openTimerRef.current = window.setTimeout(() => { + if (window.Plain) { + window.Plain.open(); + } + }, 100); + }And update the effect’s cleanup to clear the stored timer:
- return () => window.removeEventListener("resize", checkScreenSize); + return () => { + window.removeEventListener("resize", checkScreenSize); + if (openTimerRef.current) { + clearTimeout(openTimerRef.current); + openTimerRef.current = null; + } + };Add the supporting pieces outside this range:
// import import { useEffect, useState, useCallback, useMemo, useRef } from "react"; // inside component, near other state const openTimerRef = useRef<number | null>(null);
🧹 Nitpick comments (7)
packages/features/shell/user-dropdown/UserDropdown.tsx (1)
3-3: Type nit + safer call: import MouseEvent and use optional chaining.Minor polish to avoid relying on the React namespace and to guard the call.
Apply this diff:
-import { useEffect, useState } from "react"; +import { useEffect, useState, type MouseEvent } from "react"; @@ - const handleHelpClick = (e?: React.MouseEvent) => { + const handleHelpClick = (e?: MouseEvent) => { @@ - if (window.Plain) window.Plain.open(); + window.Plain?.open();Also applies to: 59-71
apps/web/lib/hooks/useIsBookingPage.ts (1)
7-16: Micro: hoist static route arrays to module scope.Avoids re-allocating arrays on each render.
Apply this diff:
+const BOOKING_ROUTES = [ + "/booking", + "/cancel", + "/reschedule", + "/instant-meeting", // Instant booking page + "/team", // Team booking pages + "/d", // Private Link of booking page + "/apps/routing-forms/routing-link", // Routing Form page + "/forms/", // Rewrites to /apps/routing-forms/routing-link +] as const; + +const BOOKING_LIST_SUFFIXES = ["/upcoming", "/unconfirmed", "/recurring", "/cancelled", "/past"] as const; @@ - const isBookingPage = [ - "/booking", - "/cancel", - "/reschedule", - "/instant-meeting", // Instant booking page - "/team", // Team booking pages - "/d", // Private Link of booking page - "/apps/routing-forms/routing-link", // Routing Form page - "/forms/", // Rewrites to /apps/routing-forms/routing-link - ].some((route) => pathname?.startsWith(route)); - const isBookingsListPage = ["/upcoming", "/unconfirmed", "/recurring", "/cancelled", "/past"].some((route) => pathname?.endsWith(route)); + const isBookingPage = BOOKING_ROUTES.some((route) => pathname?.startsWith(route)); + const isBookingsListPage = BOOKING_LIST_SUFFIXES.some((route) => pathname?.endsWith(route));Also applies to: 17-17
apps/web/components/plain/PlainContactForm.tsx (3)
57-61: Remove any-cast and typeuploadFormData.This avoids the eslint suppression and tightens the contract.
Apply this diff:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - uploadFormData.forEach(({ key, value }: any) => { - formData.append(key, value); - }); + type UploadField = { key: string; value: string }; + (uploadFormData as UploadField[]).forEach(({ key, value }) => { + formData.append(key, value); + });
141-141: Reset the form when the popover closes.Prevents reopening to a stale “Message Sent” state.
Apply this diff:
- <Popover open={open} onOpenChange={setIsOpen}> + <Popover + open={open} + onOpenChange={(next) => { + setIsOpen(next); + if (!next) resetForm(); + }}>Also applies to: 130-137
155-161: Accessibility: add an aria-label to the close button.Improves screen reader UX.
Apply this diff:
- <Button + <Button color="minimal" variant="button" StartIcon="x" size="sm" + aria-label={t("close")} onClick={() => setIsOpen(false)} />apps/web/components/plain/plainChat.tsx (2)
273-283: Stub for free users: also honor query‑param auto‑open.Mirror the paid flow so “?openPlain” opens the free contact form as well.
Apply:
- } else if (typeof window !== "undefined" && !window.Plain) { + } else if (typeof window !== "undefined" && !window.Plain) { window.Plain = { // eslint-disable-next-line @typescript-eslint/no-empty-function init: () => {}, open: () => { setIsOpen(true); } }; + // Parity with paid users + if (shouldOpenPlain) { + window.Plain.open(); + } }Note: After using shouldOpenPlain here, include it in the dependency list (see next comment).
285-285: Effect deps: remove redundant userEmail; include shouldOpenPlain if used above.userEmail is already captured by initConfig; adding it here causes extra runs. If you adopt the auto‑open change, add shouldOpenPlain.
- }, [isAppDomain, checkScreenSize, initConfig, userEmail, isPaidUser]); + }, [isAppDomain, checkScreenSize, initConfig, isPaidUser, shouldOpenPlain]);
📜 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.
📒 Files selected for processing (4)
apps/web/components/plain/PlainContactForm.tsx(4 hunks)apps/web/components/plain/plainChat.tsx(3 hunks)apps/web/lib/hooks/useIsBookingPage.ts(1 hunks)packages/features/shell/user-dropdown/UserDropdown.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/shell/user-dropdown/UserDropdown.tsxapps/web/components/plain/PlainContactForm.tsxapps/web/components/plain/plainChat.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/shell/user-dropdown/UserDropdown.tsxapps/web/lib/hooks/useIsBookingPage.tsapps/web/components/plain/PlainContactForm.tsxapps/web/components/plain/plainChat.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/shell/user-dropdown/UserDropdown.tsxapps/web/lib/hooks/useIsBookingPage.tsapps/web/components/plain/PlainContactForm.tsxapps/web/components/plain/plainChat.tsx
**/*.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:
apps/web/lib/hooks/useIsBookingPage.ts
🧠 Learnings (1)
📚 Learning: 2025-08-29T22:57:30.345Z
Learnt from: bandhan-majumder
PR: calcom/cal.com#23454
File: packages/features/form-builder/FormBuilder.tsx:11-11
Timestamp: 2025-08-29T22:57:30.345Z
Learning: FormBuilder.tsx in packages/features/form-builder/ does not have "use client" directive at the top despite using client-side React hooks and event handlers, which suggests it should be a client component.
Applied to files:
apps/web/components/plain/PlainContactForm.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: Install dependencies / Yarn install & cache
🔇 Additional comments (4)
packages/features/shell/user-dropdown/UserDropdown.tsx (1)
59-71: Deferring Plain.open with double rAF to avoid menu close race is solid.Stopping propagation + closing the menu before opening the widget is the right sequence here. LGTM.
apps/web/lib/hooks/useIsBookingPage.ts (1)
17-17: Correctly excludes booking list routes from booking pages.The
endsWithcheck for list pages and the updated return condition are accurate for URLs like/bookings/past. LGTM.Also applies to: 23-23
apps/web/components/plain/plainChat.tsx (2)
86-86: Controlled open state for free‑flow — good.This enables external control and aligns with the refactored PlainContactForm.
301-303: LGTM — passes controlled props to PlainContactForm.This correctly hooks the non‑paid flow into the shared UI.
There was a problem hiding this comment.
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)
apps/web/components/plain/__tests__/PlainContactForm.test.tsx (1)
119-135: Avoid persistent never-resolving mock and remove unnecessary awaitUse a one-shot impl so later tests aren’t affected;
toBeDisabledisn’t async.-mockFetch.mockImplementation(() => new Promise((_resolve) => {})); +mockFetch.mockImplementationOnce(() => new Promise((_resolve) => {})); -await expect(submitButton).toBeDisabled(); +expect(submitButton).toBeDisabled();
🧹 Nitpick comments (6)
apps/web/components/plain/__tests__/PlainContactForm.test.tsx (6)
45-50: Disambiguate launcher button queryUse an accessible name to avoid matching the wrong button if more are rendered.
-const button = screen.getByRole("button"); +const button = screen.getByRole("button", { name: /contact/i });
52-60: Also assert call count and prefer named queryPrevents false positives and ensures a single open action.
-const button = screen.getByRole("button"); +const button = screen.getByRole("button", { name: /contact/i }); fireEvent.click(button); -expect(mockSetIsOpen).toHaveBeenCalledWith(true); +expect(mockSetIsOpen).toHaveBeenCalledTimes(1); +expect(mockSetIsOpen).toHaveBeenCalledWith(true);
76-86: Target the close button by accessible name instead of DOM shapeQuerying SVG internals is brittle. Prefer role+name and assert presence.
-const buttons = screen.getAllByRole("button"); -const closeButton = buttons.find((button) => button.querySelector('svg use[href="#x"]')); -expect(closeButton).toBeDefined(); -fireEvent.click(closeButton!); +const closeButton = screen.getByRole("button", { name: /close|dismiss/i }); +expect(closeButton).toBeInTheDocument(); +fireEvent.click(closeButton); expect(mockSetIsOpen).toHaveBeenCalledWith(false);
88-117: Make fetch expectation stricter yet resilientAssert one call and match only the important parts to reduce brittleness if headers/body gain fields.
-expect(mockFetch).toHaveBeenCalledWith("/api/support", { - method: "POST", - headers: { - "Content-Type": "application/json", - }, - body: JSON.stringify({ - message: "Test message", - attachmentIds: [], - }), -}); +expect(mockFetch).toHaveBeenCalledTimes(1); +expect(mockFetch).toHaveBeenNthCalledWith( + 1, + "/api/support", + expect.objectContaining({ + method: "POST", + headers: expect.objectContaining({ "Content-Type": "application/json" }), + body: JSON.stringify( + expect.objectContaining({ + message: "Test message", + attachmentIds: [], + }) + ), + }) +);Consider adding a failure-path test asserting
showToastis called. I can draft it if you want.
136-161: Optional: assert full reset of UIAfter “Send another message”, also verify the submit button label resets and any attachments state clears (e.g., via uploader testId).
176-181: UsetoBeRequired()and dropawaitCleaner intent and correct sync matcher usage.
-await expect(screen.getByLabelText("Describe the issue")).toHaveAttribute("required"); +expect(screen.getByLabelText("Describe the issue")).toBeRequired();
📜 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.
📒 Files selected for processing (1)
apps/web/components/plain/__tests__/PlainContactForm.test.tsx(5 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:
apps/web/components/plain/__tests__/PlainContactForm.test.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:
apps/web/components/plain/__tests__/PlainContactForm.test.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:
apps/web/components/plain/__tests__/PlainContactForm.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
apps/web/components/plain/__tests__/PlainContactForm.test.tsx
🔇 Additional comments (3)
apps/web/components/plain/__tests__/PlainContactForm.test.tsx (3)
61-67: LGTM: form visibility assertions are clear and robust
69-74: LGTM: initial state check reads well
163-175: LGTM: unauthenticated session path coveredBaseline behavior asserted; no issues.
| const mockFetch = vi.fn(); | ||
| global.fetch = mockFetch; | ||
| global.fetch = mockFetch as unknown as typeof fetch; | ||
|
|
||
| describe("PlainContactForm", () => { | ||
| let mockSetIsOpen: ReturnType<typeof vi.fn>; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mockSetIsOpen = vi.fn(); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Reset the fetch mock between tests to avoid leaked implementations
The “loading” test installs a never-resolving impl; since only vi.clearAllMocks() runs, that impl can bleed into later tests. Reset the mock each test.
Apply in beforeEach:
beforeEach(() => {
vi.clearAllMocks();
+ mockFetch.mockReset();
mockSetIsOpen = vi.fn();🏁 Script executed:
#!/bin/bash
sed -n '1,200p' apps/web/components/plain/__tests__/PlainContactForm.test.tsxLength of output: 5663
Reset fetch mock implementation between tests
vi.clearAllMocks() only clears call history, not mock implementations—add mockFetch.mockReset() in each test’s setup to clear any custom implementations.
Apply in beforeEach:
beforeEach(() => {
vi.clearAllMocks();
+ mockFetch.mockReset();
mockSetIsOpen = vi.fn();📝 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.
| const mockFetch = vi.fn(); | |
| global.fetch = mockFetch; | |
| global.fetch = mockFetch as unknown as typeof fetch; | |
| describe("PlainContactForm", () => { | |
| let mockSetIsOpen: ReturnType<typeof vi.fn>; | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| mockSetIsOpen = vi.fn(); | |
| const mockFetch = vi.fn(); | |
| global.fetch = mockFetch as unknown as typeof fetch; | |
| describe("PlainContactForm", () => { | |
| let mockSetIsOpen: ReturnType<typeof vi.fn>; | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| mockFetch.mockReset(); | |
| mockSetIsOpen = vi.fn(); | |
| }); | |
| // … | |
| }); |
🤖 Prompt for AI Agents
In apps/web/components/plain/__tests__/PlainContactForm.test.tsx around lines 19
to 28, the test setup only calls vi.clearAllMocks() which clears call history
but does not reset mock implementations on global.fetch; add
mockFetch.mockReset() (or mockClear() as appropriate) inside the beforeEach
after vi.clearAllMocks() to reset any custom fetch implementations and ensure
each test starts with a clean mock implementation.
E2E results are ready! |
4 participants
What does this PR do?
Visual Demo (For contributors especially)
BEFORE:
Screen.Recording.2025-09-01.at.5.00.22.PM.mov
AFTER:
Screen.Recording.2025-09-01.at.4.58.30.PM.mov
Mandatory Tasks (DO NOT REMOVE)