Devin/self hoster admin email consent 1765209823 #6
Conversation
- Add new step 2 for self-hosters (!IS_CALCOM) to collect optional admin email - Prefill email from step 1 admin user creation - Add checkboxes for product changelog and marketing consent - Submit to Cal.com form via headless routing API - Add skip button to allow users to bypass this step - Add translation strings for new UI elements Co-Authored-By: [email protected] <[email protected]>
…ted admins Co-Authored-By: [email protected] <[email protected]>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a crucial feature for self-hosted Cal.com instances, enabling administrators to provide their email address and consent to receive important updates, such as product changelogs and security advisories, directly from Cal.com. This is achieved by integrating a new email consent form into the initial setup wizard and providing a persistent banner in the admin's profile settings for managing these preferences, ensuring self-hosters stay informed about their instance. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis change introduces an email consent workflow for admin users during setup. It adds a new AdminEmailConsent component for collecting admin email preferences, updates the AdminUser component's callback to pass email, integrates email consent into the setup flow for self-hosted instances, and adds a banner in account settings to redirect users to complete email consent. Localization strings are added for the new UI elements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AdminUser
participant AdminEmailConsent
participant SetupFlow
participant LicenseStep
User->>AdminUser: Complete admin setup
activate AdminUser
AdminUser->>AdminUser: Validate admin details
AdminUser->>SetupFlow: onSuccess(email)
deactivate AdminUser
activate SetupFlow
SetupFlow->>SetupFlow: Store admin email in state
alt showAdminEmailConsent (self-hosted)
SetupFlow->>AdminEmailConsent: Navigate to email consent step
activate AdminEmailConsent
User->>AdminEmailConsent: Submit email & consent preferences<br/>or skip
AdminEmailConsent->>SetupFlow: onSubmit/onSkip
deactivate AdminEmailConsent
end
SetupFlow->>LicenseStep: Navigate to next step<br/>(license or app setup)
deactivate SetupFlow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@refacto-visz |
codeant-ai
bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an admin email consent step for self-hosted instances, which is a valuable addition for keeping administrators informed. The implementation is solid, creating a new component for the consent form and integrating it into the setup wizard. A banner is also added to the admin's profile page to allow for future preference updates. My review includes a couple of suggestions to improve code quality by simplifying a form controller and removing duplicated logic in the setup wizard.
| <Controller | ||
| name="email" | ||
| control={formMethods.control} | ||
| render={({ field: { onBlur, onChange, value } }) => ( | ||
| <EmailField | ||
| label={t("admin_email_label")} | ||
| value={value || ""} | ||
| onBlur={onBlur} | ||
| onChange={(e) => onChange(e.target.value)} | ||
| className="my-0" | ||
| name="email" | ||
| /> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
The Controller for the EmailField can be simplified by spreading the field object from react-hook-form. This reduces boilerplate and improves readability by automatically passing props like onChange, onBlur, value, and name.
| <Controller | |
| name="email" | |
| control={formMethods.control} | |
| render={({ field: { onBlur, onChange, value } }) => ( | |
| <EmailField | |
| label={t("admin_email_label")} | |
| value={value || ""} | |
| onBlur={onBlur} | |
| onChange={(e) => onChange(e.target.value)} | |
| className="my-0" | |
| name="email" | |
| /> | |
| )} | |
| /> | |
| <Controller | |
| name="email" | |
| control={formMethods.control} | |
| render={({ field }) => ( | |
| <EmailField | |
| label={t("admin_email_label")} | |
| {...field} | |
| value={field.value || ""} | |
| className="my-0" | |
| /> | |
| )} | |
| /> |
| return ( | ||
| <AdminEmailConsent | ||
| id="wizard-step-2" | ||
| name="wizard-step-2" | ||
| defaultEmail={adminEmail} | ||
| onSubmit={() => { | ||
| setIsPending(true); | ||
| // After consent step, go to license step or apps step | ||
| if (props.hasValidLicense || hasPickedAGPLv3) { | ||
| nav.onNext(); | ||
| nav.onNext(); // Skip license step | ||
| } else { | ||
| nav.onNext(); | ||
| } | ||
| }} | ||
| onSkip={() => { | ||
| // After skipping, go to license step or apps step | ||
| if (props.hasValidLicense || hasPickedAGPLv3) { | ||
| nav.onNext(); | ||
| nav.onNext(); // Skip license step | ||
| } else { | ||
| nav.onNext(); | ||
| } | ||
| }} | ||
| onPrevStep={nav.onPrev} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
The navigation logic within onSubmit and onSkip for the AdminEmailConsent component is duplicated. To improve maintainability and reduce redundancy, this logic should be extracted into a shared function.
const handleNext = () => {
// After consent step, go to license step or apps step
if (props.hasValidLicense || hasPickedAGPLv3) {
nav.onNext();
nav.onNext(); // Skip license step
} else {
nav.onNext();
}
};
return (
<AdminEmailConsent
id="wizard-step-2"
name="wizard-step-2"
defaultEmail={adminEmail}
onSubmit={() => {
setIsPending(true);
handleNext();
}}
onSkip={handleNext}
onPrevStep={nav.onPrev}
/>
);
Refacto PR SummaryAdded admin email consent collection during self-hosted Cal.com setup to enable communication about security updates and product changes. The implementation integrates a new consent step in the setup wizard for self-hosted instances only, with optional marketing and changelog subscriptions. Key Changes:
Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant A as Admin User
participant S as Setup Wizard
participant C as Consent Form
participant R as Router API
participant P as Profile Settings
A->>S: Complete admin user creation
S->>S: Check IS_CALCOM flag
S->>C: Navigate to consent step (self-hosted only)
A->>C: Enter email and preferences
C->>R: Submit to headless router endpoint
R-->>C: Consent recorded
C->>S: Continue to license/apps step
Note over P: Later access via settings
A->>P: Visit profile settings
P->>P: Check admin role + self-hosted
P->>A: Show consent banner if applicable
Testing GuideClick to expand
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| import { CheckboxField, EmailField } from "@calcom/ui/components/form"; | ||
|
|
||
| const FORM_ID = "d4fe10cb-6cb3-4c84-ae61-394a817c419e"; | ||
| const HEADLESS_ROUTER_URL = "https://i.cal.com/router"; |
There was a problem hiding this comment.
Suggestion: The headless router URL is hardcoded to the production endpoint, which makes the behavior inflexible for different environments and forces all self-hosted instances to send data to that fixed URL instead of allowing configuration. [possible bug]
Severity Level: Critical 🚨
| const HEADLESS_ROUTER_URL = "https://i.cal.com/router"; | |
| const HEADLESS_ROUTER_URL = | |
| process.env.NEXT_PUBLIC_HEADLESS_ROUTER_URL ?? "https://i.cal.com/router"; |
Why it matters? ⭐
Hardcoding an external production URL in component code forces all deployments (including self-hosted) to contact that endpoint and prevents per-environment configuration. Replacing it with a NEXT_PUBLIC_ env fallback is a practical improvement for configurability and testing.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/components/setup/AdminEmailConsent.tsx
**Line:** 14:14
**Comment:**
*Possible Bug: The headless router URL is hardcoded to the production endpoint, which makes the behavior inflexible for different environments and forces all self-hosted instances to send data to that fixed URL instead of allowing configuration.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| } catch (error) { | ||
| console.error("Failed to submit admin email consent:", error); | ||
| } finally { | ||
| setIsSubmitting(false); | ||
| onSubmit(); |
There was a problem hiding this comment.
Suggestion: If the consent submission request fails (for example due to network issues or the headless router being unreachable), the step callback is still invoked in the finally block, causing the setup wizard to advance as if consent was recorded even though the data was never sent successfully. [logic error]
Severity Level: Minor
| } catch (error) { | |
| console.error("Failed to submit admin email consent:", error); | |
| } finally { | |
| setIsSubmitting(false); | |
| onSubmit(); | |
| onSubmit(); | |
| } catch (error) { | |
| console.error("Failed to submit admin email consent:", error); | |
| } finally { | |
| setIsSubmitting(false); |
Why it matters? ⭐
The suggestion correctly highlights a logic issue: onSubmit() is invoked in the finally block so the wizard advances even when the network request fails. Moving onSubmit() into the successful path (after the await fetch) ensures the step only advances when the submission completed without throwing. This fixes a real UX/data-loss problem rather than being purely cosmetic.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/components/setup/AdminEmailConsent.tsx
**Line:** 76:80
**Comment:**
*Logic Error: If the consent submission request fails (for example due to network issues or the headless router being unreachable), the step callback is still invoked in the `finally` block, causing the setup wizard to advance as if consent was recorded even though the data was never sent successfully.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| <Button type="button" color="secondary" onClick={onPrevStep}> | ||
| {t("prev_step")} | ||
| </Button> |
There was a problem hiding this comment.
Suggestion: While the async consent submission is in progress, the "Prev step" and "Skip" buttons remain active, so a user can navigate away and then the pending submission callback will run later, causing inconsistent multi-step navigation (e.g. skipping or double-advancing steps). [race condition]
Severity Level: Minor
| <Button type="button" color="secondary" onClick={onPrevStep}> | |
| {t("prev_step")} | |
| </Button> | |
| <Button type="button" color="secondary" onClick={onPrevStep} disabled={isSubmitting}> | |
| {t("prev_step")} | |
| </Button> | |
| <Button type="button" color="minimal" onClick={handleSkip} disabled={isSubmitting}> |
Why it matters? ⭐
This is a valid concurrency/race concern. While submit disables the submit button via its loading prop, Prev/Skip remain active and can let a user navigate away while an in-flight request later calls callbacks (or onSubmit) and produce inconsistent wizard state. Disabling Prev/Skip when isSubmitting prevents that class of race and is a minimal, sensible UX/safety fix.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/components/setup/AdminEmailConsent.tsx
**Line:** 139:141
**Comment:**
*Race Condition: While the async consent submission is in progress, the "Prev step" and "Skip" buttons remain active, so a user can navigate away and then the pending submission callback will run later, causing inconsistent multi-step navigation (e.g. skipping or double-advancing steps).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| ); | ||
| const [adminEmail, setAdminEmail] = useState<string>(""); | ||
|
|
||
| const showAdminEmailConsent = !IS_CALCOM; |
There was a problem hiding this comment.
Suggestion: The admin email consent step is always added for self-hosters regardless of user count, which shifts the step indices and breaks the existing defaultStep logic that assumes the second step is the license selection and the third is the apps step; restricting the consent step to the initial admin-user flow keeps the wizard step indexes consistent with the constants. [logic error]
Severity Level: Minor
| const showAdminEmailConsent = !IS_CALCOM; | |
| const showAdminEmailConsent = !IS_CALCOM && props.userCount === 0; |
Why it matters? ⭐
The PR always inserts the AdminEmailConsent step for self-hosted installs (showAdminEmailConsent = !IS_CALCOM), regardless of props.userCount. defaultStep is computed earlier using numeric constants (ADMIN_USER = 1, LICENSE = 2, APPS = 3). If the consent step is pushed into the steps array between ADMIN_USER and LICENSE, the numeric defaults (2 and 3) no longer point at LICENSE/APPS respectively and will target the wrong step (e.g. point at the consent step). The suggested change (restrict consent to the initial admin-user flow) is a valid fix to keep step indices consistent or, alternatively, the defaultStep calculation should be adjusted to reflect dynamic insertion of steps. This is a real logic bug, not a cosmetic nit.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/modules/auth/setup-view.tsx
**Line:** 35:35
**Comment:**
*Logic Error: The admin email consent step is always added for self-hosters regardless of user count, which shifts the step indices and breaks the existing `defaultStep` logic that assumes the second step is the license selection and the third is the apps step; restricting the consent step to the initial admin-user flow keeps the wizard step indexes consistent with the constants.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| id="wizard-step-2" | ||
| name="wizard-step-2" |
There was a problem hiding this comment.
Suggestion: Both the admin email consent step and the license selection step use the same id and name ("wizard-step-2"), which can cause duplicate DOM IDs and ambiguous form field references; giving the consent step a distinct identifier avoids collisions. [possible bug]
Severity Level: Critical 🚨
| id="wizard-step-2" | |
| name="wizard-step-2" | |
| id="wizard-step-2-consent" | |
| name="wizard-step-2-consent" |
Why it matters? ⭐
The consent step and the license selection step both use id/name "wizard-step-2" in the PR. If both components are rendered in the DOM (or during testing/SSR snapshots) this will produce duplicate DOM IDs and can cause ambiguous form submissions or accessibility issues. Giving the consent step a distinct id/name (or deriving them from the current steps length) prevents collisions. This is a valid, actionable fix.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/modules/auth/setup-view.tsx
**Line:** 92:93
**Comment:**
*Possible Bug: Both the admin email consent step and the license selection step use the same `id` and `name` ("wizard-step-2"), which can cause duplicate DOM IDs and ambiguous form field references; giving the consent step a distinct identifier avoids collisions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| user={user} | ||
| userOrganization={user.organization} | ||
| _userOrganization={user.organization} |
There was a problem hiding this comment.
Suggestion: The _isFallbackImg and _userOrganization props are passed into the ProfileForm component but never used, creating dead props that increase coupling and can confuse future maintainers about their purpose. [code quality]
Severity Level: Minor
| user={user} | |
| userOrganization={user.organization} | |
| _userOrganization={user.organization} | |
| user={user} |
Why it matters? ⭐
The props _isFallbackImg and _userOrganization are indeed passed from ProfileView into ProfileForm but are unused inside ProfileForm. Keeping them around creates dead surface area and can confuse future maintainers. Removing these from the caller (and the component signature) is a safe cleanup that reduces coupling and improves the component API.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/modules/settings/my-account/profile-view.tsx
**Line:** 281:282
**Comment:**
*Code Quality: The `_isFallbackImg` and `_userOrganization` props are passed into the `ProfileForm` component but never used, creating dead props that increase coupling and can confuse future maintainers about their purpose.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| defaultValues: FormValues; | ||
| onSubmit: (values: ExtendedFormValues) => void; | ||
| handleAddSecondaryEmail: () => void; | ||
| handleResendVerifyEmail: (email: string) => void; | ||
| handleAccountDisconnect: (values: ExtendedFormValues) => void; | ||
| extraField?: React.ReactNode; | ||
| isPending: boolean; | ||
| isFallbackImg: boolean; | ||
| _isFallbackImg: boolean; | ||
| user: RouterOutputs["viewer"]["me"]["get"]; | ||
| userOrganization: RouterOutputs["viewer"]["me"]["get"]["organization"]; | ||
| _userOrganization: RouterOutputs["viewer"]["me"]["get"]["organization"]; | ||
| isCALIdentityProvider: boolean; |
There was a problem hiding this comment.
Suggestion: The _isFallbackImg and _userOrganization properties are declared in the ProfileForm parameter list and its props type but never used inside the component, leading to unnecessary props that can get out of sync with callers and make the component API misleading. [code quality]
Severity Level: Minor
| user, | |
| isCALIdentityProvider, | |
| }: { | |
| defaultValues: FormValues; | |
| onSubmit: (values: ExtendedFormValues) => void; | |
| handleAddSecondaryEmail: () => void; | |
| handleResendVerifyEmail: (email: string) => void; | |
| handleAccountDisconnect: (values: ExtendedFormValues) => void; | |
| extraField?: React.ReactNode; | |
| isPending: boolean; | |
| user: RouterOutputs["viewer"]["me"]["get"]; |
Why it matters? ⭐
The props _isFallbackImg and _userOrganization are declared in the ProfileForm signature and its props type but they are never referenced inside the component. This makes the component API misleading. Removing them from the parameter list and the type definition is a straightforward, safe cleanup that prevents API drift and reduces maintenance burden.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/modules/settings/my-account/profile-view.tsx
**Line:** 519:529
**Comment:**
*Code Quality: The `_isFallbackImg` and `_userOrganization` properties are declared in the `ProfileForm` parameter list and its props type but never used inside the component, leading to unnecessary props that can get out of sync with callers and make the component API misleading.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| }; | ||
|
|
||
| const { data: usersAttributes, isPending: usersAttributesPending } = | ||
| const { data: usersAttributes, isPending: _usersAttributesPending } = |
There was a problem hiding this comment.
Suggestion: The _usersAttributesPending value is destructured from the trpc.viewer.attributes.getByUserId.useQuery result but never used, which adds noise and can trigger lint warnings without providing any functional benefit. [code quality]
Severity Level: Minor
| const { data: usersAttributes, isPending: _usersAttributesPending } = | |
| const { data: usersAttributes } = |
Why it matters? ⭐
The query result includes isPending aliased to _usersAttributesPending, but that variable is never read. Removing the unused destructured binding reduces lint noise and is safe — the query's data is still used (usersAttributes). This is a small cleanup with no behavioral impact.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/modules/settings/my-account/profile-view.tsx
**Line:** 618:618
**Comment:**
*Code Quality: The `_usersAttributesPending` value is destructured from the `trpc.viewer.attributes.getByUserId.useQuery` result but never used, which adds noise and can trigger lint warnings without providing any functional benefit.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
Code Review: Admin Consent Setup FlowPR Confidence Score: 🟨 4 / 5👍 Well Done
📁 Selected files for review (6)
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| params.append("consent", "marketing"); | ||
| } | ||
|
|
||
| const url = `${HEADLESS_ROUTER_URL}?${params.toString()}`; |
There was a problem hiding this comment.
Admin Email Exposure
Admin email addresses are transmitted via GET request URL parameters to external service. URLs are logged in server access logs, browser history, and referrer headers exposing sensitive admin contact information. This creates data exposure risk for self-hosted administrators.
Standards
- CWE-200
- OWASP-A02
- NIST-SSDF-PW.1
| const FORM_ID = "d4fe10cb-6cb3-4c84-ae61-394a817c419e"; | ||
| const HEADLESS_ROUTER_URL = "https://i.cal.com/router"; |
There was a problem hiding this comment.
External Data Transmission
Self-hosted admin data is transmitted to external Cal.com infrastructure without explicit user consent disclosure. This creates potential data sovereignty and privacy compliance issues for organizations with strict data residency requirements. Administrators may not realize their information leaves their infrastructure.
Standards
- CWE-359
- OWASP-A02
- Compliance-GDPR
| const router = useRouter(); | ||
|
|
||
| const handleOpenAdminStep = () => { | ||
| router.push("/auth/setup?step=2"); |
There was a problem hiding this comment.
Incorrect Wizard Step Navigation
The banner navigates to step=2 in the setup wizard. However, the new admin consent step is dynamically inserted at index 1 of the wizard's step array in setup-view.tsx. This incorrect index will lead the user to the wrong step, breaking the intended navigation flow from the profile settings.
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| if (props.hasValidLicense || hasPickedAGPLv3) { | ||
| nav.onNext(); | ||
| nav.onNext(); // Skip license step | ||
| } else { | ||
| nav.onNext(); | ||
| } | ||
| }} | ||
| onSkip={() => { | ||
| // After skipping, go to license step or apps step | ||
| if (props.hasValidLicense || hasPickedAGPLv3) { | ||
| nav.onNext(); | ||
| nav.onNext(); // Skip license step | ||
| } else { | ||
| nav.onNext(); | ||
| } |
There was a problem hiding this comment.
Duplicated Navigation Logic
The navigation logic within the onSubmit and onSkip handlers for the AdminEmailConsent component is identical. This duplication violates the DRY (Don't Repeat Yourself) principle. If the navigation flow after this step needs to change, it will require updates in two separate places, increasing the risk of introducing inconsistencies and making maintenance more difficult.
Standards
- Clean-Code-DRY
![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 (5)
apps/web/modules/settings/my-account/components/AdminEmailConsentBanner.tsx (1)
13-15: Decouple banner from hard‑coded setup step indexThe
/auth/setup?step=2target assumes AdminEmailConsent will always be step 2 of the wizard. That’s true today for self‑hosted setups, but it’s easy to break if steps are reordered again.Consider centralizing the “email consent step index” in setup-view (e.g. an exported constant or helper) or using a more semantic query flag that setup-view translates into the right step, to avoid future coupling.
Also applies to: 25-31
apps/web/public/static/locales/en/common.json (1)
770-776: Tighten wording of new locale stringsThe keys are consistent and descriptive, but you may want to tweak phrasing slightly:
self_hosted_admin_email_description: “To stay informed about upcoming updates, vulnerabilities**,** and more, we highly recommend…” reads a bit smoother, and maybe split into two sentences for readability.admin_email_label: “Admin email address” or “Administrator email” is more natural than “Email of admin”.These are minor UX polish changes; behavior is fine as-is.
apps/web/components/setup/AdminEmailConsent.tsx (1)
32-82: Clarify consent submission behavior and normalize emailA few small improvements to consider here:
- Normalize email before sending: Right now you validate the raw
stringand send it as-is. To match the admin user flow and avoid subtle duplicates, consider trimming and lowercasing before appending, e.g.const email = values.email.trim().toLowerCase();and validate against that.- Multiple
consentparams: When both checkboxes are checked you end up with...?consent=product&consent=marketing. If the headless router expects repeated params, this is fine; if not, you may want to encode asconsent=product,marketingorconsents[]=.... Worth double‑checking the backend expectation.- Proceeding on network failure: Because
onSubmit()is called infinally, the wizard will advance even if thefetchfails (network error, DNS, etc). If “best effort, never block setup” is the product requirement, this is ideal; otherwise you might only callonSubmit()on success and surface an error state instead.If you decide to adjust any of the above, you can keep the rest of the form logic and UI exactly as-is.
Also applies to: 95-148
apps/web/modules/auth/setup-view.tsx (1)
34-47: Align step constants/IDs with the new AdminEmailConsent stepThe new self‑hosted flow looks correct, but there are a couple of maintainability nits:
SETUP_VIEW_SETPS.LICENSEis still2, yet for self‑hosters the second step is now AdminEmailConsent. Defaulting to step2whenuserCount > 0effectively lands on the consent step, not license. That’s intended behavior now, but the constant name is misleading.- Both AdminEmailConsent and LicenseSelection use
"wizard-step-2"asid/namein some combinations, and for self‑hosters the license step is no longer the second step. This is mostly cosmetic, but unique, position‑accurate IDs make debugging and test automation easier.- The “skip license” navigation logic (
nav.onNext()twice) is duplicated in AdminUser’sonSuccessand AdminEmailConsent’sonSubmit/onSkip. A small helper likegoToPostLicenseOrApps(nav, props.hasValidLicense, hasPickedAGPLv3)would centralize the logic and reduce chances of these branches drifting.Functionally everything should work, but tightening these points would make the wizard flow easier to reason about for future changes.
Also applies to: 48-81, 83-119
apps/web/modules/settings/my-account/profile-view.tsx (1)
267-270: Self-hosted admin gating is correct; consider optional attribute-based gating later
isSelfHostedAdmincorrectly limits the banner to non‑cal.com deployments and users with theADMINrole, and the conditional render block is straightforward and safe with the session optional chaining.If you later store an “email consent completed” flag in user attributes, this would be a good place to additionally gate the banner on that signal so it only appears when consent is missing (or switch to a subtler entry point once consent has been recorded). For now, the always-on admin banner behavior looks reasonable.
Also applies to: 328-332
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/components/setup/AdminEmailConsent.tsx(1 hunks)apps/web/components/setup/AdminUser.tsx(3 hunks)apps/web/modules/auth/setup-view.tsx(4 hunks)apps/web/modules/settings/my-account/components/AdminEmailConsentBanner.tsx(1 hunks)apps/web/modules/settings/my-account/profile-view.tsx(10 hunks)apps/web/public/static/locales/en/common.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/web/modules/settings/my-account/components/AdminEmailConsentBanner.tsx (2)
packages/ui/components/test-setup.tsx (1)
useRouter(44-52)packages/ui/components/button/Button.tsx (1)
Button(221-351)
apps/web/components/setup/AdminEmailConsent.tsx (1)
packages/ui/components/button/Button.tsx (1)
Button(221-351)
apps/web/modules/settings/my-account/profile-view.tsx (2)
packages/lib/constants.ts (1)
IS_CALCOM(43-49)apps/web/modules/settings/my-account/components/AdminEmailConsentBanner.tsx (1)
AdminEmailConsentBanner(9-37)
apps/web/modules/auth/setup-view.tsx (1)
packages/lib/constants.ts (1)
IS_CALCOM(43-49)
🔇 Additional comments (3)
apps/web/components/setup/AdminUser.tsx (1)
16-27: onSuccess email propagation is wired correctlyThe updated
onSuccess?(email?: string)signature plus:
AdminUserpassingdata.email_address.toLowerCase()andAdminUserContainersafely callingonSuccess?.()without argsgives setup-view access to the email when available without breaking the existing “admin already created” path. This looks consistent and type-safe.
Also applies to: 38-43, 78-100
apps/web/modules/settings/my-account/profile-view.tsx (2)
19-21: Imports and underscore-prefixed props look consistent and intentionalUsing
IS_CALCOM,UserPermissionRole, andAdminEmailConsentBannerhere is consistent with the surrounding architecture, and the new_isFallbackImg,_userOrganization, and_usersAttributesPendingnames follow the common underscore convention for intentionally unused values while keeping the typings and API surface intact. No changes needed.Also applies to: 25-26, 50-52, 276-283, 506-530
618-622: usersAttributes query aliasing is safe and non-breakingRenaming
isPendingto_usersAttributesPendingpreserves the query behavior while clearly indicating the pending flag is intentionally unused right now, which should keep linting happy without altering runtime behavior.
CodeAnt-AI Description
Add self-hosted admin email consent step and profile banner
What Changed
Impact
✅ Collect admin emails from self-hosters✅ Clearer opt-in for product changelogs and marketing✅ Easier update of admin preferences from profile💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.