Fix: lock timezone on booking page breaks atoms#23060
Merged
Conversation
Contributor
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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
CodeRabbit Configuration File (
|
Contributor
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
keithwillcode
added
core
dosubot
bot
added
booking-page
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/13/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (08/13/25)1 label was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.changeset/deep-owls-hang.md (1)
2-2: Semver bump should be patch, not minor.This is a bug fix with no public API change. Consider marking this as a patch release to align with semantic versioning expectations.
Apply this diff:
--- -"@calcom/atoms": minor +"@calcom/atoms": patch ---packages/features/eventtypes/components/tabs/advanced/EventAdvancedTab.tsx (2)
13-17: Typo in alias; improve naming clarity.Rename
PlatformTimzoneSelect→PlatformTimezoneSelectto avoid confusion and keep naming consistent with “Timezone.”Apply this diff:
-import { Timezone as PlatformTimzoneSelect } from "@calcom/atoms/timezone"; +import { Timezone as PlatformTimezoneSelect } from "@calcom/atoms/timezone";
569-572: Simplify: use direct conditional instead of useMemo for component selection.
useMemoisn’t needed here; the component refs are stable and cheaper than the extra hook. A direct conditional improves readability. Also, ensure both platform/web variants share the same props (notably theonChangesignature).Apply this diff to simplify:
- const TimezoneSelect = useMemo(() => { - return isPlatform ? PlatformTimzoneSelect : WebTimezoneSelect; - }, [isPlatform]); + const TimezoneSelect = isPlatform ? PlatformTimezoneSelect : WebTimezoneSelect;Verification request:
- Confirm that both
@calcom/atoms/timezoneTimezone and webTimezoneSelectaccept the same props and emit the sameonChangeshape. The current usage expects an object with a.valuestring. If the platform variant returns a raw string, update the handler to normalize both:Example outside this hunk (for clarity only):
<TimezoneSelect id="lockedTimeZone" value={value ?? "Europe/London"} onChange={(e: { value: string } | string) => { const tz = typeof e === "string" ? e : e?.value; if (tz) formMethods.setValue("lockedTimeZone", tz, { shouldDirty: true }); }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/deep-owls-hang.md(1 hunks)packages/features/eventtypes/components/tabs/advanced/EventAdvancedTab.tsx(3 hunks)packages/platform/atoms/event-types/wrappers/EventTypePlatformWrapper.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/eventtypes/components/tabs/advanced/EventAdvancedTab.tsxpackages/platform/atoms/event-types/wrappers/EventTypePlatformWrapper.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/eventtypes/components/tabs/advanced/EventAdvancedTab.tsxpackages/platform/atoms/event-types/wrappers/EventTypePlatformWrapper.tsx
🧬 Code Graph Analysis (1)
packages/features/eventtypes/components/tabs/advanced/EventAdvancedTab.tsx (1)
packages/features/components/timezone-select/TimezoneSelect.tsx (1)
TimezoneSelect(42-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Detect changes
🔇 Additional comments (1)
packages/platform/atoms/event-types/wrappers/EventTypePlatformWrapper.tsx (1)
154-156: Good: propagate error to external callbacks.Forwarding a normalized error to
callbacksRef.current?.onErrorensures external consumers get notified on failures triggered by the update mutation.
Comment on lines
+178
to
+198
| const handleFormSubmit = useCallback( | ||
| (customCallbacks?: { onSuccess?: () => void; onError?: (error: Error) => void }) => { | ||
| if (customCallbacks) { | ||
| callbacksRef.current = customCallbacks; | ||
| } | ||
|
|
||
| if (saveButtonRef.current) { | ||
| saveButtonRef.current.click(); | ||
| } else { | ||
| form.handleSubmit((data) => { | ||
| try { | ||
| handleSubmit(data); | ||
| customCallbacks?.onSuccess?.(); | ||
| } catch (error) { | ||
| customCallbacks?.onError?.(error as Error); | ||
| } | ||
| })(); | ||
| } | ||
| }, [handleSubmit, form]); | ||
| if (saveButtonRef.current) { | ||
| saveButtonRef.current.click(); | ||
| } else { | ||
| form.handleSubmit((data) => { | ||
| try { | ||
| handleSubmit(data); | ||
| customCallbacks?.onSuccess?.(); | ||
| } catch (error) { | ||
| customCallbacks?.onError?.(error as Error); | ||
| } | ||
| })(); | ||
| } | ||
| }, | ||
| [handleSubmit, form] | ||
| ); |
Contributor
There was a problem hiding this comment.
Avoid duplicate/early success callbacks in handleFormSubmit.
customCallbacks?.onSuccess?.() is invoked immediately after handleSubmit(data), which likely fires an async mutation. This will:
- Call onSuccess too early (before the mutation settles).
- Potentially call onSuccess twice (here and again in
updateMutation.onSuccessviacallbacksRef).
Rely on mutation callbacks (already wired via callbacksRef) and remove the immediate invocations in this handler.
Apply this diff:
- const handleFormSubmit = useCallback(
- (customCallbacks?: { onSuccess?: () => void; onError?: (error: Error) => void }) => {
- if (customCallbacks) {
- callbacksRef.current = customCallbacks;
- }
-
- if (saveButtonRef.current) {
- saveButtonRef.current.click();
- } else {
- form.handleSubmit((data) => {
- try {
- handleSubmit(data);
- customCallbacks?.onSuccess?.();
- } catch (error) {
- customCallbacks?.onError?.(error as Error);
- }
- })();
- }
- },
- [handleSubmit, form]
- );
+ const handleFormSubmit = useCallback(
+ (customCallbacks?: { onSuccess?: () => void; onError?: (error: Error) => void }) => {
+ if (customCallbacks) {
+ callbacksRef.current = customCallbacks;
+ }
+ if (saveButtonRef.current) {
+ // Triggers the underlying submit which will call mutation and invoke callbacksRef on settle
+ saveButtonRef.current.click();
+ return;
+ }
+ // Fallback: submit programmatically; rely on mutation onSuccess/onError to invoke callbacksRef
+ form.handleSubmit((data) => {
+ handleSubmit(data);
+ })();
+ },
+ [handleSubmit, form]
+ );📝 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.
Suggested change
| const handleFormSubmit = useCallback( | |
| (customCallbacks?: { onSuccess?: () => void; onError?: (error: Error) => void }) => { | |
| if (customCallbacks) { | |
| callbacksRef.current = customCallbacks; | |
| } | |
| if (saveButtonRef.current) { | |
| saveButtonRef.current.click(); | |
| } else { | |
| form.handleSubmit((data) => { | |
| try { | |
| handleSubmit(data); | |
| customCallbacks?.onSuccess?.(); | |
| } catch (error) { | |
| customCallbacks?.onError?.(error as Error); | |
| } | |
| })(); | |
| } | |
| }, [handleSubmit, form]); | |
| if (saveButtonRef.current) { | |
| saveButtonRef.current.click(); | |
| } else { | |
| form.handleSubmit((data) => { | |
| try { | |
| handleSubmit(data); | |
| customCallbacks?.onSuccess?.(); | |
| } catch (error) { | |
| customCallbacks?.onError?.(error as Error); | |
| } | |
| })(); | |
| } | |
| }, | |
| [handleSubmit, form] | |
| ); | |
| const handleFormSubmit = useCallback( | |
| (customCallbacks?: { onSuccess?: () => void; onError?: (error: Error) => void }) => { | |
| if (customCallbacks) { | |
| callbacksRef.current = customCallbacks; | |
| } | |
| if (saveButtonRef.current) { | |
| // Triggers the underlying submit which will call mutation and invoke callbacksRef on settle | |
| saveButtonRef.current.click(); | |
| return; | |
| } | |
| // Fallback: submit programmatically; rely on mutation onSuccess/onError to invoke callbacksRef | |
| form.handleSubmit((data) => { | |
| handleSubmit(data); | |
| })(); | |
| }, | |
| [handleSubmit, form] | |
| ); |
🤖 Prompt for AI Agents
In packages/platform/atoms/event-types/wrappers/EventTypePlatformWrapper.tsx
around lines 178-198, remove the immediate invocations of
customCallbacks?.onSuccess and customCallbacks?.onError inside the
form.handleSubmit branch so callbacks are only invoked via the mutation
callbacks wired through callbacksRef; keep the assignment callbacksRef.current =
customCallbacks when provided and still call saveButtonRef.current.click() when
present, but inside the form.handleSubmit try/catch do not call customCallbacks
directly — let updateMutation.onSuccess/onError (via callbacksRef) handle
success/error notifications.
supalarry
approved these changes
Aug 13, 2025
Contributor
E2E results are ready! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
EventTypeSettings Atom crashes on enabling
Lock timezone on bookingin advanced tabVisual 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):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)