-
Notifications
You must be signed in to change notification settings - Fork 0
Devin/self hoster admin email consent 1765209823 #6
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
df7dfff
ce1160f
1a4524e
9657dde
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,154 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "use client"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { zodResolver } from "@hookform/resolvers/zod"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useState } from "react"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Controller, FormProvider, useForm } from "react-hook-form"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { z } from "zod"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { emailRegex } from "@calcom/lib/emailSchema"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Button } from "@calcom/ui/components/button"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+13
to
+14
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. External Data TransmissionSelf-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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type AdminEmailConsentFormValues = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| email: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| productChangelog: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| marketingConsent: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const AdminEmailConsent = (props: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defaultEmail?: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onSubmit: () => void; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onSkip: () => void; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onPrevStep: () => void; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } & Omit<JSX.IntrinsicElements["form"], "onSubmit">) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { defaultEmail = "", onSubmit, onSkip, onPrevStep, ...rest } = props; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { t } = useLocale(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [isSubmitting, setIsSubmitting] = useState(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const formSchema = z.object({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| email: z | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .string() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .refine((val) => val === "" || emailRegex.test(val), { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| message: t("enter_valid_email"), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| productChangelog: z.boolean(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| marketingConsent: z.boolean(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const formMethods = useForm<AdminEmailConsentFormValues>({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defaultValues: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| email: defaultEmail, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| productChangelog: false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| marketingConsent: false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolver: zodResolver(formSchema), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const handleSubmit = formMethods.handleSubmit(async (values) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setIsSubmitting(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const params = new URLSearchParams(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| params.append("form", FORM_ID); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (values.email) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| params.append("email", values.email); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (values.productChangelog) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| params.append("consent", "product"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (values.marketingConsent) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| params.append("consent", "marketing"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const url = `${HEADLESS_ROUTER_URL}?${params.toString()}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. Admin Email ExposureAdmin 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await fetch(url, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| method: "GET", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mode: "no-cors", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.error("Failed to submit admin email consent:", error); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setIsSubmitting(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onSubmit(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+76
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. 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 Severity Level: Minor
Suggested change
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const handleSkip = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onSkip(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <FormProvider {...formMethods}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <form {...rest} className="stack-y-6" onSubmit={handleSubmit}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <p className="text-subtle text-sm"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| {t("self_hosted_admin_email_description")} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </p> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className="stack-y-4"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <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" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+96
to
+109
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. The
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Controller | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name="productChangelog" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| control={formMethods.control} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| render={({ field: { onChange, value } }) => ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <CheckboxField | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id="product" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description={t("product_changelog_consent")} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| checked={value} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onChange={(e) => onChange(e.target.checked)} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Controller | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name="marketingConsent" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| control={formMethods.control} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| render={({ field: { onChange, value } }) => ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <CheckboxField | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id="marketing" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description={t("marketing_consent")} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| checked={value} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onChange={(e) => onChange(e.target.checked)} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className="flex justify-end gap-2"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Button type="button" color="secondary" onClick={onPrevStep}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| {t("prev_step")} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </Button> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+139
to
+141
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. 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
Suggested change
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Button type="button" color="minimal" onClick={handleSkip}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| {t("skip")} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </Button> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Button type="submit" color="primary" loading={isSubmitting}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| {t("submit")} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </Button> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </form> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </FormProvider> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export default AdminEmailConsent; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,12 +4,13 @@ import { useRouter } from "next/navigation"; | |||||||||
| import { useMemo, useState } from "react"; | ||||||||||
|
|
||||||||||
| import AdminAppsList from "@calcom/features/apps/AdminAppsList"; | ||||||||||
| import { APP_NAME } from "@calcom/lib/constants"; | ||||||||||
| import { APP_NAME, IS_CALCOM } from "@calcom/lib/constants"; | ||||||||||
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | ||||||||||
| import type { inferSSRProps } from "@calcom/types/inferSSRProps"; | ||||||||||
| import { WizardForm } from "@calcom/ui/components/form"; | ||||||||||
| import type { WizardStep } from "@calcom/ui/components/form/wizard/WizardForm"; | ||||||||||
|
|
||||||||||
| import AdminEmailConsent from "@components/setup/AdminEmailConsent"; | ||||||||||
| import { AdminUserContainer as AdminUser } from "@components/setup/AdminUser"; | ||||||||||
| import LicenseSelection from "@components/setup/LicenseSelection"; | ||||||||||
|
|
||||||||||
|
|
@@ -29,6 +30,9 @@ export function Setup(props: PageProps) { | |||||||||
| const [licenseOption, setLicenseOption] = useState<"FREE" | "EXISTING">( | ||||||||||
| props.hasValidLicense ? "EXISTING" : "FREE" | ||||||||||
| ); | ||||||||||
| const [adminEmail, setAdminEmail] = useState<string>(""); | ||||||||||
|
|
||||||||||
| const showAdminEmailConsent = !IS_CALCOM; | ||||||||||
|
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. 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 Severity Level: Minor
Suggested change
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. |
||||||||||
|
|
||||||||||
| const defaultStep = useMemo(() => { | ||||||||||
| if (props.userCount > 0) { | ||||||||||
|
|
@@ -51,9 +55,15 @@ export function Setup(props: PageProps) { | |||||||||
| onSubmit={() => { | ||||||||||
| setIsPending(true); | ||||||||||
| }} | ||||||||||
| onSuccess={() => { | ||||||||||
| // If there's already a valid license or user picked AGPLv3, skip to apps step | ||||||||||
| if (props.hasValidLicense || hasPickedAGPLv3) { | ||||||||||
| onSuccess={(email?: string) => { | ||||||||||
| if (email) { | ||||||||||
| setAdminEmail(email); | ||||||||||
| } | ||||||||||
| // For self-hosters, go to admin email consent step next | ||||||||||
| if (showAdminEmailConsent) { | ||||||||||
| nav.onNext(); | ||||||||||
| } else if (props.hasValidLicense || hasPickedAGPLv3) { | ||||||||||
| // If there's already a valid license or user picked AGPLv3, skip to apps step | ||||||||||
| nav.onNext(); | ||||||||||
| nav.onNext(); // Skip license step | ||||||||||
| } else { | ||||||||||
|
|
@@ -70,6 +80,44 @@ export function Setup(props: PageProps) { | |||||||||
| }, | ||||||||||
| ]; | ||||||||||
|
|
||||||||||
| // For self-hosters (!IS_CALCOM), add admin email consent step | ||||||||||
| if (showAdminEmailConsent) { | ||||||||||
| steps.push({ | ||||||||||
| title: t("stay_informed"), | ||||||||||
| description: t("stay_informed_description"), | ||||||||||
| customActions: true, | ||||||||||
| content: (setIsPending, nav) => { | ||||||||||
| return ( | ||||||||||
| <AdminEmailConsent | ||||||||||
| id="wizard-step-2" | ||||||||||
| name="wizard-step-2" | ||||||||||
|
Comment on lines
+92
to
+93
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. Suggestion: Both the admin email consent step and the license selection step use the same Severity Level: Critical 🚨
Suggested change
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. |
||||||||||
| 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(); | ||||||||||
| } | ||||||||||
|
Comment on lines
+98
to
+112
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. Duplicated Navigation LogicThe navigation logic within the Standards
|
||||||||||
| }} | ||||||||||
| onPrevStep={nav.onPrev} | ||||||||||
| /> | ||||||||||
| ); | ||||||||||
|
Comment on lines
+90
to
+116
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. The navigation logic within |
||||||||||
| }, | ||||||||||
| }); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Only show license selection step if there's no valid license already and AGPLv3 wasn't picked | ||||||||||
| if (!props.hasValidLicense && !hasPickedAGPLv3) { | ||||||||||
| steps.push({ | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| "use client"; | ||
|
|
||
| import { useRouter } from "next/navigation"; | ||
|
|
||
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | ||
| import { Button } from "@calcom/ui/components/button"; | ||
| import { Icon } from "@calcom/ui/components/icon"; | ||
|
|
||
| export const AdminEmailConsentBanner = () => { | ||
| const { t } = useLocale(); | ||
| const router = useRouter(); | ||
|
|
||
| const handleOpenAdminStep = () => { | ||
| router.push("/auth/setup?step=2"); | ||
|
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. Incorrect Wizard Step NavigationThe banner navigates to Standards
|
||
| }; | ||
|
|
||
| return ( | ||
| <div className="border-subtle bg-subtle relative mb-6 overflow-hidden rounded-lg border p-4"> | ||
| <div className="flex items-start gap-4"> | ||
| <div className="bg-emphasis flex h-10 w-10 items-center justify-center rounded-full"> | ||
| <Icon name="mail" className="text-default h-5 w-5" /> | ||
| </div> | ||
| <div className="flex flex-1 flex-col gap-2"> | ||
| <div className="flex flex-col"> | ||
| <h3 className="text-default text-sm font-semibold">{t("stay_informed")}</h3> | ||
| <p className="text-subtle text-sm">{t("self_hosted_admin_email_description")}</p> | ||
| </div> | ||
| <div className="mt-2"> | ||
| <Button color="secondary" EndIcon="external-link" onClick={handleOpenAdminStep}> | ||
| {t("update_preferences")} | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; | ||
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.
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 🚨
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 🤖