feat: enable cal-video-event-type-settings#23789
feat: enable cal-video-event-type-settings#23789hemantmm wants to merge 23 commits intocalcom:mainfrom
Conversation
|
@hemantmm is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a defaultCalVideoSettings constant and uses it as the fallback when constructing form defaultValues and when preparing the submit payload. Detects cal video integration locations via hasCalVideoLocation (location.type === "integrations:daily") and, if present, ensures filteredPayload.calVideoSettings is populated from values.calVideoSettings or the default. Changes submit triggering to run when either a dirty field exists or a cal video location is present. Also includes a formatting change in .eslintrc.js (duplicate multiline message entry in an existing no-restricted-imports pattern). No exported/public signatures changed. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
github-actions
bot
added
platform
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/platform/atoms/event-types/hooks/useEventTypeForm.ts (3)
149-157: Avoid object aliasing; return a fresh, typed default each timeUsing a shared object for defaults risks accidental mutation/aliasing with RHF state. Prefer a factory that returns a new, strongly-typed object.
- const defaultCalVideoSettings = { - disableRecordingForGuests: false, - disableRecordingForOrganizer: false, - enableAutomaticTranscription: false, - enableAutomaticRecordingForOrganizer: false, - disableTranscriptionForGuests: false, - disableTranscriptionForOrganizer: false, - redirectUrlOnExit: null, - }; + const defaultCalVideoSettings = (): NonNullable<FormValues["calVideoSettings"]> => ({ + disableRecordingForGuests: false, + disableRecordingForOrganizer: false, + enableAutomaticTranscription: false, + enableAutomaticRecordingForOrganizer: false, + disableTranscriptionForGuests: false, + disableTranscriptionForOrganizer: false, + redirectUrlOnExit: null, + });
160-163: Initialize calVideoSettings with a fresh objectCall the factory to avoid sharing the same reference across forms.
- calVideoSettings: eventType?.calVideoSettings ?? defaultCalVideoSettings, + calVideoSettings: eventType?.calVideoSettings ?? defaultCalVideoSettings(),
433-435: Avoid submitting unchanged forms solely due to a Cal Video locationOnly submit when fields are dirty or when you need a one-time Cal Video settings upsert.
- if (dirtyFieldExists || hasCalVideoLocation) { + if (dirtyFieldExists || (hasCalVideoLocation && !eventType.calVideoSettings)) { onSubmit({ ...filteredPayload, id: eventType.id }); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/platform/atoms/event-types/hooks/useEventTypeForm.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
packages/platform/atoms/event-types/hooks/useEventTypeForm.ts
**/*.{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/platform/atoms/event-types/hooks/useEventTypeForm.ts
**/*.{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/platform/atoms/event-types/hooks/useEventTypeForm.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: anglerfishlyy
PR: calcom/cal.com#0
File: :0-0
Timestamp: 2025-08-27T16:39:38.192Z
Learning: anglerfishlyy successfully implemented CAL-3076 email invitation feature for Cal.com team event-types in PR #23312. The feature allows inviting people via email directly from assignment flow, with automatic team invitation if email doesn't belong to existing team member. Implementation includes Host type modifications (userId?: number, email?: string, isPending?: boolean), CheckedTeamSelect component updates with CreatableSelect, TRPC schema validation with zod email validation, and integration with existing teamInvite system.
📚 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:
packages/platform/atoms/event-types/hooks/useEventTypeForm.ts
🧬 Code graph analysis (1)
packages/platform/atoms/event-types/hooks/useEventTypeForm.ts (1)
packages/features/eventtypes/lib/types.ts (1)
FormValues(78-175)
⏰ 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 (1)
packages/platform/atoms/event-types/hooks/useEventTypeForm.ts (1)
427-431: Verify canonical Cal Video location type; gate calVideoSettings upsertrg failed to scan .tsx files ("unrecognized file type: tsx"); the canonical Cal Video location type (whether "integrations:daily", "integrations:cal-video", or both) couldn't be confirmed. Confirm canonical type(s) and update the allowlist. Only upsert calVideoSettings when a Cal Video location exists AND the event lacks settings or calVideoSettings was edited to avoid unnecessary writes.
File: packages/platform/atoms/event-types/hooks/useEventTypeForm.ts (around lines 427–431)
- const hasCalVideoLocation = (values.locations ?? []).some((loc) => loc.type === "integrations:daily"); - - if (hasCalVideoLocation) { - filteredPayload.calVideoSettings = values.calVideoSettings ?? defaultCalVideoSettings; - } + // TODO: adjust the allowlist after verification + const isCalVideoType = (t: string) => + ["integrations:cal-video", "integrations:daily"].includes(t); + const hasCalVideoLocation = (values.locations ?? []).some((loc) => isCalVideoType(loc.type)); + + const shouldUpsertCalVS = + hasCalVideoLocation && + (!eventType.calVideoSettings || ("calVideoSettings" in dirtyValues)); + + if (shouldUpsertCalVS) { + filteredPayload.calVideoSettings = values.calVideoSettings ?? defaultCalVideoSettings(); + }
kart1ka
left a comment
kart1ka
left a comment
There was a problem hiding this comment.
Left a comment. Also the change should be tested in event type settings atom. As per your attached video you have shown the change in event type settings web and not atom. Can you update and test if this works for event settings in atom?
Signed-off-by: Hemant M Mehta <hemant29mehta@gmail.com>
…mantmm/cal.com into cal-video-event-type-settings
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.eslintrc.js (1)
28-34: Also block root imports (@calcom/trpcand@trpc).
"@calcom/trpc/*"doesn’t match the bare root import"@calcom/trpc". Add root entries to close this gap.Apply:
{ patterns: [ { - group: ["@calcom/trpc/*", "@trpc/*"], + group: ["@calcom/trpc", "@calcom/trpc/*", "@trpc", "@trpc/*"], message: "tRPC imports are blocked in packages/app-store. Move UI to apps/web/components/apps or introduce an API boundary.", }, ], },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.eslintrc.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: anglerfishlyy
PR: calcom/cal.com#0
File: :0-0
Timestamp: 2025-08-27T16:39:38.192Z
Learning: anglerfishlyy successfully implemented CAL-3076 email invitation feature for Cal.com team event-types in PR #23312. The feature allows inviting people via email directly from assignment flow, with automatic team invitation if email doesn't belong to existing team member. Implementation includes Host type modifications (userId?: number, email?: string, isPending?: boolean), CheckedTeamSelect component updates with CreatableSelect, TRPC schema validation with zod email validation, and integration with existing teamInvite system.
⏰ 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 (1)
.eslintrc.js (1)
23-36: Confirmed: coreno-restricted-importsis in use — no change required.packages/config/eslint-preset.js defines the core
no-restricted-imports(no@typescript-eslint/no-restricted-importsfound). The core rule doesn't supportallowTypeImports, so omitting that option is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.eslintrc.js(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
.eslintrc.js
[error] 31-32: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
kart1ka
left a comment
kart1ka
left a comment
There was a problem hiding this comment.
Have you tested if this works for cal.com atoms? I still do not see any loom video that shows this fix works for atoms. Can you please test this and attach a loom video of the same? We do not merge changes without proper video evidence attached.
Signed-off-by: Hemant M Mehta <hemant29mehta@gmail.com>
|
@kart1ka, I have tested in Cal.com atoms as well. Here is the video Screen.Recording.2025-09-17.at.15.21.58.online-video-cutter.com.online-video-cutter.com.mp4 |
|
This PR is being marked as stale due to inactivity. |
|
@kart1ka, can you review this PR? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/platform/atoms/event-types/hooks/useEventTypeForm.ts">
<violation number="1" location="packages/platform/atoms/event-types/hooks/useEventTypeForm.ts:429">
calVideoSettings still gets submitted even when the event no longer has a Cal Video location because the property was already present on filteredPayload from the spread above; we need to explicitly clear it when hasCalVideoLocation is false so the guard is effective.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
Devin AI is completing this stale PRThis PR by @hemantmm has been marked as stale. A Devin session has been created to complete the remaining work. Devin will review the PR, address any feedback, and push updates to complete this PR. |
Address review feedback to explicitly delete calVideoSettings from the payload when the event type no longer has a Cal Video location. This prevents stale calVideoSettings from being submitted. Also removed duplicate logic for setting calVideoSettings. Co-authored-by: hemantmm <hemantmm@users.noreply.github.com> Co-Authored-By: unknown <>
Completed WorkI've addressed the review feedback from @cubic-dev-ai and cleaned up the code: Changes Made
Code Changesif (hasCalVideoLocation) {
filteredPayload.calVideoSettings = values.calVideoSettings ?? defaultCalVideoSettings;
+} else {
+ delete filteredPayload.calVideoSettings;
}
-const calVideoSettingsDirty = !!dirtyFields.calVideoSettings;
-
-if (dirtyFieldExists || calVideoSettingsDirty) {
- if (hasCalVideoLocation) {
- filteredPayload.calVideoSettings = values.calVideoSettings ?? defaultCalVideoSettings;
- }
+if (dirtyFieldExists) {
onSubmit({ ...filteredPayload, id: eventType.id });
}Verification
The PR is ready for review. Credit to @hemantmm for the original implementation. |
5 participants
closes: #21863
What does this PR do?
This PR enables Cal Video settings for event types by:
Updates since last revision
Addressed review feedback from @cubic-dev-ai:
calVideoSettingswas still being submitted even when the event no longer has a Cal Video location (the property was present from the spread infilteredPayload)delete filteredPayload.calVideoSettingswhenhasCalVideoLocationis falsecalVideoSettingsVisual Demo (For contributors especially)
Video Demo (if applicable):
Before:
Screen.Recording.2025-09-12.at.16.32.36.mov
After:
Screen.Recording.2025-09-12.at.16.30.49.mov
Cal.com Atoms testing:
Screen.Recording.2025-09-17.at.15.21.58.online-video-cutter.com.online-video-cutter.com.mp4
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Human Review Checklist
calVideoSettingswhen Cal Video location is removed is the intended behaviorChecklist
Link to Devin run: https://app.devin.ai/sessions/666a456bb2604467ab0a391c14e64184
Requested by: unknown ()