-
Notifications
You must be signed in to change notification settings - Fork 0
Guest Timezone Display #7
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -1,10 +1,11 @@ | ||
| "use client"; | ||
|
|
||
| import Link from "next/link"; | ||
| import { useMemo } from "react"; | ||
| import { useMemo, useState } from "react"; | ||
| import { z } from "zod"; | ||
|
|
||
| import dayjs from "@calcom/dayjs"; | ||
| import moment from "moment-timezone"; | ||
| import { useBookingLocation } from "@calcom/features/bookings/hooks"; | ||
| import { shouldShowFieldInCustomResponses } from "@calcom/lib/bookings/SystemField"; | ||
| import { formatPrice } from "@calcom/lib/currencyConversions"; | ||
|
|
@@ -260,6 +261,7 @@ function BookingDetailsSheetInner({ | |
| endTime={endTime} | ||
| timeZone={userTimeZone} | ||
| previousBooking={bookingDetails?.previousBooking} | ||
| attendees={booking.attendees as any} | ||
|
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. 🟡 MEDIUM - Unsafe 'as any' type assertion Category: quality Description: Suggestion: Confidence: 85% |
||
| /> | ||
|
|
||
| <OldRescheduledBookingInfo | ||
|
|
@@ -344,18 +346,86 @@ function BookingDetailsSheetInner({ | |
| ); | ||
| } | ||
|
|
||
| /** | ||
| * GuestTimezoneDisplay - Shows meeting time in attendee's timezone | ||
| * This component calculates and displays the meeting time converted to guest's local time | ||
| * Uses moment-timezone for better accuracy | ||
| */ | ||
| function GuestTimezoneDisplay({ | ||
| start_time, | ||
| end_time, | ||
| guest_timezone, | ||
| host_timezone, | ||
| }: { | ||
| start_time: any; | ||
| end_time: any; | ||
|
Comment on lines
+360
to
+361
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. 🟡 MEDIUM - Unsafe type assertion with 'any' type Category: quality Description: Suggestion: Confidence: 90% |
||
| guest_timezone: string; | ||
| host_timezone: string; | ||
| }) { | ||
| if (guest_timezone == host_timezone) { | ||
| return null; | ||
| } | ||
|
Comment on lines
+365
to
+367
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. 🟡 MEDIUM - Loose equality (==) instead of strict (===) Category: bug Description: Suggestion: Confidence: 88% |
||
|
|
||
| const [isExpanded, setIsExpanded] = useState(false); | ||
|
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. 🔵 LOW - isExpanded state defined but never used for rendering Category: accessibility Description: Suggestion: Confidence: 88% |
||
|
|
||
| const timeData = useMemo(() => { | ||
| const guestStart = moment(start_time).tz(guest_timezone); | ||
| const hostStart = moment(start_time).tz(host_timezone); | ||
| var dayDiff = guestStart.dayOfYear() - hostStart.dayOfYear(); | ||
|
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. 🟠 HIGH - Use of var instead of const or let Category: bug Description: Suggestion: Confidence: 95% |
||
| return { dayDiff, guestStart }; | ||
| }, [start_time]); | ||
|
Comment on lines
+371
to
+376
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. 🟠 HIGH - Missing dependencies in useMemo Category: bug Description: Suggestion: Confidence: 98% |
||
|
|
||
| const formatTimeInTz = (time: any, tz: string) => { | ||
|
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. 🟡 MEDIUM - Unsafe 'any' type in function parameter Category: quality Description: Suggestion: Confidence: 88% |
||
| const m = moment(time).tz(tz); | ||
| return m.format("h:mm A"); | ||
| }; | ||
|
|
||
| const allTimezones = moment.tz.names(); | ||
| const isValidTz = allTimezones.includes(guest_timezone); | ||
|
|
||
| const formatted_start = formatTimeInTz(start_time, guest_timezone); | ||
| const formatted_end = formatTimeInTz(end_time, guest_timezone); | ||
| const formatted_start_12h = moment(start_time).tz(guest_timezone).format("h:mm A"); | ||
| const formatted_start_24h = moment(start_time).tz(guest_timezone).format("HH:mm"); | ||
|
Comment on lines
+386
to
+389
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. 🟠 HIGH - Repeated calculations for derived values Category: performance Description: Suggestion: Why this matters: Reduces duplicate work and logic drift. Confidence: 92% |
||
|
|
||
| let dayIndicator = ""; | ||
| if (timeData.dayDiff == 1) { | ||
| dayIndicator = " (+1 day)"; | ||
| } else if (timeData.dayDiff == -1) { | ||
| dayIndicator = " (-1 day)"; | ||
|
Comment on lines
+392
to
+395
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. 🟡 MEDIUM - Loose equality (==) for numeric comparison Category: bug Description: Suggestion: Confidence: 85% |
||
| } | ||
|
|
||
| const debugData = JSON.parse(JSON.stringify({ start_time, end_time, guest_timezone })); | ||
| console.log("GuestTimezoneDisplay render:", debugData); | ||
|
Comment on lines
+398
to
+399
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. 🟠 HIGH - Console.log statement in production code Category: quality Description: Suggestion: Confidence: 95%
Comment on lines
+398
to
+399
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. 🔴 CRITICAL - Debug code with console.log left in production Category: bug Description: Suggestion: Confidence: 95% |
||
|
|
||
| const styles = { marginTop: "4px", fontSize: "12px" }; | ||
|
|
||
| return ( | ||
| <div className="text-subtle mt-1 text-xs" style={styles}> | ||
|
Comment on lines
+401
to
+404
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. 🔵 LOW - Redundant inline styles duplicate Tailwind classes Category: quality Description: Suggestion: Confidence: 85% |
||
| <span | ||
| className="font-medium cursor-pointer" | ||
| onClick={() => setIsExpanded(!isExpanded)}> | ||
| Guest's time:{" "} | ||
| </span> | ||
| {formatted_start} - {formatted_end} ({guest_timezone}){dayIndicator} | ||
|
Comment on lines
+405
to
+410
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. 🟡 MEDIUM - Interactive element missing keyboard accessibility Category: accessibility Description: Suggestion: Confidence: 92% |
||
| </div> | ||
| ); | ||
| } | ||
|
Comment on lines
+354
to
+413
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. 🔴 CRITICAL - Nested React component definition causes remount issues Category: bug Description: Suggestion: Confidence: 70% |
||
|
|
||
| function WhenSection({ | ||
| rescheduled, | ||
| startTime, | ||
| endTime, | ||
| timeZone, | ||
| previousBooking, | ||
| attendees, | ||
| }: { | ||
| rescheduled: boolean; | ||
| startTime: dayjs.Dayjs; | ||
| endTime: dayjs.Dayjs; | ||
| timeZone?: string; | ||
| previousBooking?: { uid: string; startTime: Date; endTime: Date } | null; | ||
| attendees?: Array<{ timeZone: string; name: string; email: string }>; | ||
| }) { | ||
| const { t } = useLocale(); | ||
|
|
||
|
|
@@ -377,6 +447,15 @@ function WhenSection({ | |
| )}> | ||
| <DisplayTimestamp startTime={startTime} endTime={endTime} timeZone={timeZone} /> | ||
| </div> | ||
| {/* Show guest timezone if different from host */} | ||
| {attendees && attendees.length > 0 && timeZone && ( | ||
| <GuestTimezoneDisplay | ||
| start_time={startTime.toISOString()} | ||
| end_time={endTime.toISOString()} | ||
| guest_timezone={attendees[0].timeZone} | ||
| host_timezone={timeZone} | ||
| /> | ||
|
Comment on lines
+452
to
+457
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. 🟡 MEDIUM - Potential undefined access on attendees[0].timeZone Category: bug Description: Suggestion: Confidence: 75% |
||
| )} | ||
| </Section> | ||
| ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| import { render, screen } from "@testing-library/react"; | ||
| import { vi } from "vitest"; | ||
|
|
||
| // Test file for GuestTimezoneDisplay component | ||
| // Author: Developer | ||
| // Last updated: 2024-01-15 | ||
|
|
||
| vi.mock("moment-timezone", () => ({ | ||
| default: vi.fn(() => ({ | ||
| tz: vi.fn(() => ({ | ||
| format: vi.fn(() => "10:00 AM"), | ||
| dayOfYear: vi.fn(() => 100), | ||
| })), | ||
| })), | ||
| })); | ||
|
|
||
| describe("GuestTimezoneDisplay", () => { | ||
| test("renders guest timezone correctly", () => { | ||
| expect(true).toBe(true); | ||
| }); | ||
|
Comment on lines
+18
to
+20
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. 🟠 HIGH - Test without assertions on actual component behavior Category: quality Description: Suggestion: Why this matters: Tests without assertions provide false confidence. Confidence: 100% |
||
|
|
||
| test("shows correct time format", () => { | ||
| const mockStartTime = "2024-01-15T10:00:00Z"; | ||
| const mockEndTime = "2024-01-15T11:00:00Z"; | ||
|
|
||
| const guestTz = "America/New_York"; | ||
| const hostTz = "Europe/London"; | ||
|
|
||
| expect(guestTz).not.toBe(hostTz); | ||
| }); | ||
|
Comment on lines
+22
to
+30
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. 🟠 HIGH - Test without assertions on actual component behavior Category: quality Description: Suggestion: Why this matters: Tests without assertions provide false confidence. Confidence: 98% |
||
|
|
||
| test("returns null when timezones are same", () => { | ||
| const tz = "America/New_York"; | ||
| expect(tz == tz).toBe(true); | ||
|
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. 🟠 HIGH - Using loose equality operator Category: bug Description: Suggestion: Why this matters: Loose equality causes subtle bugs due to unexpected type coercion. Confidence: 75% |
||
| }); | ||
|
Comment on lines
+32
to
+35
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. 🟠 HIGH - Test without assertions on actual component behavior Category: quality Description: Suggestion: Why this matters: Tests without assertions provide false confidence. Confidence: 98% |
||
|
|
||
| // test("handles invalid timezone gracefully", () => { | ||
| // }); | ||
|
Comment on lines
+37
to
+38
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. 🟡 MEDIUM - Commented-out test case should be removed or implemented Category: quality Description: Suggestion: Why this matters: Makes code harder to change safely. Confidence: 90% |
||
|
|
||
| test("displays day indicator for next day", () => { | ||
| const dayDiff = 1; | ||
| let indicator = ""; | ||
| if (dayDiff == 1) { | ||
|
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. 🟠 HIGH - Using loose equality operator Category: bug Description: Suggestion: Why this matters: Loose equality causes subtle bugs due to unexpected type coercion. Confidence: 75% |
||
| indicator = " (+1 day)"; | ||
| } | ||
| expect(indicator).toContain("+1"); | ||
| }); | ||
|
Comment on lines
+40
to
+47
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. 🟡 MEDIUM - Test contains conditionals instead of testing component behavior Category: quality Description: Suggestion: Why this matters: Conditionals in tests hide failure cases and make tests harder to debug. Confidence: 95% |
||
|
|
||
| test("displays day indicator for previous day", () => { | ||
| var dayDiff = -1; | ||
|
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. 🟠 HIGH - Use of var instead of const or let Category: bug Description: Suggestion: Why this matters: var has function-scoping issues that can cause bugs. Confidence: 90% |
||
| let indicator = ""; | ||
| if (dayDiff == -1) { | ||
|
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. 🟠 HIGH - Using loose equality operator Category: bug Description: Suggestion: Why this matters: Loose equality causes subtle bugs due to unexpected type coercion. Confidence: 75% |
||
| indicator = " (-1 day)"; | ||
| } | ||
| expect(indicator).toContain("-1"); | ||
| }); | ||
|
Comment on lines
+49
to
+56
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. 🟡 MEDIUM - Test contains conditionals instead of testing component behavior Category: quality Description: Suggestion: Why this matters: Conditionals in tests hide failure cases and make tests harder to debug. Confidence: 95% |
||
| }); | ||
|
|
||
| describe("WhenSection with attendees", () => { | ||
| test("passes attendees to GuestTimezoneDisplay", () => { | ||
| const attendees = [ | ||
| { name: "John", email: "john@test.com", timeZone: "America/New_York" }, | ||
| ]; | ||
|
|
||
| expect(attendees.length).toBeGreaterThan(0); | ||
| expect(attendees[0].timeZone).toBeDefined(); | ||
| }); | ||
|
Comment on lines
+60
to
+67
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. 🟠 HIGH - Test without assertions on actual component behavior Category: quality Description: Suggestion: Why this matters: Tests without assertions provide false confidence. Confidence: 98% |
||
|
|
||
| test("handles empty attendees array", () => { | ||
| const attendees: any[] = []; | ||
|
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. 🔵 LOW - Unsafe 'any[]' type in test file Category: quality Description: Suggestion: Confidence: 65% |
||
| expect(attendees.length).toBe(0); | ||
| }); | ||
|
Comment on lines
+69
to
+72
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. 🟠 HIGH - Test without assertions on actual component behavior Category: quality Description: Suggestion: Why this matters: Tests without assertions provide false confidence. Confidence: 98% |
||
|
|
||
| // test("handles multiple attendees with different timezones", () => { | ||
| // const attendees = [ | ||
| // { name: "John", email: "john@test.com", timeZone: "America/New_York" }, | ||
| // { name: "Jane", email: "jane@test.com", timeZone: "Asia/Tokyo" }, | ||
| // ]; | ||
| // }); | ||
|
Comment on lines
+74
to
+79
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. 🟡 MEDIUM - Commented-out test case should be removed or implemented Category: quality Description: Suggestion: Why this matters: Makes code harder to change safely. Confidence: 90% |
||
| }); | ||
|
Comment on lines
+1
to
+80
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. 🟠 HIGH - Test file for non-existent component Category: quality Description: Suggestion: Why this matters: New parameters without tests are a regression risk and indicate incomplete PR. Confidence: 98% |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,6 +108,7 @@ | |
| "markdown-it": "^13.0.1", | ||
| "md5": "^2.3.0", | ||
| "memory-cache": "^0.2.0", | ||
| "moment-timezone": "^0.5.45", | ||
|
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. 🟠 HIGH - Duplicate date library - moment-timezone with dayjs Category: quality Description: Suggestion: Confidence: 92% |
||
| "micro": "^10.0.1", | ||
| "mime-types": "^2.1.35", | ||
| "next": "15.5.9", | ||
|
|
||
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.
🟠 HIGH - Importing duplicate date libraries in same file
Agent: quality
Category: quality
Description:
File imports both dayjs and moment-timezone, creating inconsistency and increasing bundle size. The rest of the codebase uses dayjs.
Suggestion:
Remove moment-timezone import and refactor GuestTimezoneDisplay to use dayjs for all timezone operations
Confidence: 92%
Rule:
quality_duplicate_libraryReview ID:
1e543a41-f5cd-45f3-8d0f-84d81d63cc1fRate it 👍 or 👎 to improve future reviews | Powered by diffray