chore: organization onboarding refactor#24381
Conversation
Server Action for gettting slug satus of org
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
packages/features/ee/organizations/components/CreateANewOrganizationForm.tsx (1)
88-89: Arbitrary timeout for persistence remains unresolved.This 50ms timeout for Zustand persistence has been extensively discussed in previous reviews. The proposed solutions include implementing a promise-based
resetAndWaitmethod or usingonRehydrateStoragecallbacks for deterministic synchronization.packages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.ts (4)
68-68: Use precise return type (no any).Return the concrete type to enforce contract consistency.
- abstract createOnboardingIntent(input: CreateOnboardingIntentInput): Promise<any>; + abstract createOnboardingIntent(input: CreateOnboardingIntentInput): Promise<OnboardingIntentResult>;
338-343: Email verification gating already handled.sendEmailVerification itself checks the global "email-verification" flag and returns early; an extra feature-gate here is redundant.
243-245: Redact emails in info-level logs (PII).Info-level logs currently emit raw emails. Mask or log counts at info; move details to debug with masking/hashing.
- log.info( - `Creating organization for owner ${owner.email} with slug ${orgData.slug} and canSetSlug=${canSetSlug}` - ); + log.info(`Creating organization (slug=${orgData.slug}, canSetSlug=${canSetSlug})`); + log.debug("Org creation owner (masked)", safeStringify({ ownerEmailLen: owner.email?.length ?? 0 })); @@ - log.info( - `Processing ${invitedMembers.length} member invites for organization ${organization.id}`, - safeStringify({ - invitedMembers: invitedMembers.map((m) => ({ - email: m.email, - teamId: m.teamId, - teamName: m.teamName, - role: m.role, - })), - }) - ); + log.info(`Processing ${invitedMembers.length} invites for org ${organization.id}`); + log.debug( + "Invites (masked)", + safeStringify({ + invites: invitedMembers.map((m) => ({ + emailLen: m.email.length, + teamId: m.teamId, + teamName: m.teamName, + role: m.role, + })), + }) + ); @@ - log.info( - "Invite categorization complete", - safeStringify({ - orgInvites: invitesToOrg.length, - teamInvites: invitesToTeams.size, - teamBreakdown: Array.from(invitesToTeams.entries()).map(([teamId, members]) => ({ - teamId, - count: members.length, - emails: members.map((m) => m.email), - })), - }) - ); + log.info("Invite categorization complete", safeStringify({ orgInvites: invitesToOrg.length, teamInvites: invitesToTeams.size })); + log.debug( + "Invite teams breakdown", + safeStringify({ + teamBreakdown: Array.from(invitesToTeams.entries()).map(([teamId, members]) => ({ + teamId, + count: members.length, + })), + }) + );Based on learnings
Also applies to: 406-416, 485-496
506-509: Harden invite role mapping (avoid lax casts).Map explicitly to allowed enum; prevent invalid strings flowing in.
- invitations: invitesToOrg.map((member) => ({ - usernameOrEmail: member.email, - role: (member.role as MembershipRole) || MembershipRole.MEMBER, - })), + invitations: invitesToOrg.map((member) => ({ + usernameOrEmail: member.email, + role: member.role?.toUpperCase() === "ADMIN" ? MembershipRole.ADMIN : MembershipRole.MEMBER, + })), @@ - invitations: members.map((member: InvitedMember) => ({ - usernameOrEmail: member.email, - role: (member.role as MembershipRole) || MembershipRole.MEMBER, - })), + invitations: members.map((member: InvitedMember) => ({ + usernameOrEmail: member.email, + role: member.role?.toUpperCase() === "ADMIN" ? MembershipRole.ADMIN : MembershipRole.MEMBER, + })),Also applies to: 523-526
packages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.ts (3)
38-45: Avoid logging PII; log counts/names only.Redact invite emails; log team names only.
- log.debug( - "BillingEnabledOrgOnboardingService.createOnboardingIntent - INPUT", - safeStringify({ - invitedMembers: input.invitedMembers, - teams: input.teams, - }) - ); + log.debug( + "BillingEnabledOrgOnboardingService.createOnboardingIntent - INPUT", + safeStringify({ + invitedMembersCount: input.invitedMembers?.length ?? 0, + teams: input.teams?.map((t) => t.name), + }) + ); @@ - log.debug( - "BillingEnabledOrgOnboardingService - After filterTeamsAndInvites", - safeStringify({ - teamsData, - invitedMembersData, - }) - ); + log.debug( + "BillingEnabledOrgOnboardingService - After filterTeamsAndInvites", + safeStringify({ + invitedMembersCount: invitedMembersData.length, + teams: teamsData.map((t) => t.name), + }) + );Also applies to: 48-55
160-160: Align slug-conflict error message with repo/tests.Use the canonical message "organization_url_taken".
- throw new Error("organization_already_exists_with_this_slug"); + throw new Error("organization_url_taken");
173-188: Pass through isPlatform from onboarding.Hardcoding false loses the org type.
- isPlatform: false, + isPlatform: organizationOnboarding.isPlatform,packages/features/ee/organizations/lib/onboardingStore.ts (3)
232-233: Bug: parse NEXT_PUBLIC_IS_E2E explicitly (string env)."false" is truthy and will wrongly disable billing.
- const isBillingEnabled = process.env.NEXT_PUBLIC_IS_E2E ? false : IS_TEAM_BILLING_ENABLED_CLIENT; + const isBillingEnabled = + process.env.NEXT_PUBLIC_IS_E2E === "true" ? false : IS_TEAM_BILLING_ENABLED_CLIENT;
98-110: Remove manual persistence in setOnboardingId (race with persist).Let the custom storage handle migration; avoid direct writes that can conflict with persist.
- setOnboardingId: (onboardingId) => { - const currentId = get().onboardingId; - // If we're setting an onboardingId for the first time, migrate the data - if (onboardingId && !currentId && typeof window !== "undefined") { - const currentState = get(); - // Save to the new key - const newKey = getStorageKey(onboardingId); - localStorage.setItem(newKey, JSON.stringify({ state: currentState, version: 0 })); - // Remove the old key - localStorage.removeItem("org-creation-onboarding"); - } - set({ onboardingId }); - }, + setOnboardingId: (onboardingId) => set({ onboardingId }),
124-161: Same race/unsafe access in reset; simplify.Delegate persistence to the storage adapter; remove direct localStorage manipulation.
reset: (state) => { if (state) { - const currentId = get().onboardingId; - const newId = state.onboardingId; - - // If onboardingId is changing, handle localStorage migration - if (typeof window !== "undefined" && currentId !== newId) { - // Remove old key if it exists - if (currentId) { - localStorage.removeItem(getStorageKey(currentId)); - } else { - localStorage.removeItem("org-creation-onboarding"); - } - - // Set new state which will be persisted with new key - set(state); - - // Immediately save to new key - if (newId) { - const newKey = getStorageKey(newId); - localStorage.setItem(newKey, JSON.stringify({ state, version: 0 })); - } - } else { - set(state); - } + set(state); } else { - // Clear the localStorage entry for this store - if (typeof window !== "undefined") { - const currentId = get().onboardingId; - if (currentId) { - localStorage.removeItem(getStorageKey(currentId)); - } else { - localStorage.removeItem("org-creation-onboarding"); - } - } set(initialState); } },
🧹 Nitpick comments (3)
packages/features/ee/organizations/lib/service/onboarding/__tests__/BillingEnabledOrgOnboardingService.test.ts (1)
212-225: Avoid asserting a hardcoded onboarding ID.Use a shape/type matcher to reduce brittleness.
- expect(result).toEqual({ + expect(result).toEqual({ userId: mockUser.id, orgOwnerEmail: mockInput.orgOwnerEmail, name: mockInput.name, slug: mockInput.slug, seats: mockInput.seats, pricePerSeat: mockInput.pricePerSeat, billingPeriod: mockInput.billingPeriod, isPlatform: mockInput.isPlatform, - organizationOnboardingId: "onboarding-123", + organizationOnboardingId: expect.any(String), checkoutUrl: "https://stripe.com/checkout/session", organizationId: null, });packages/features/ee/organizations/lib/onboardingStore.ts (2)
170-221: Harden storage adapter around key enumeration and failures.Wrap window.localStorage access in try/catch and fall back gracefully.
- getItem: (name): StorageValue<OnboardingStoreState> | null => { - if (typeof window === "undefined") return null; - - const keys = Object.keys(window.localStorage).filter((key) => - key.startsWith("org-creation-onboarding") - ); + getItem: (name): StorageValue<OnboardingStoreState> | null => { + if (typeof window === "undefined") return null; + let keys: string[] = []; + try { + // eslint-disable-next-line @calcom/eslint/avoid-web-storage + keys = Object.keys(window.localStorage).filter((key) => key.startsWith("org-creation-onboarding")); + } catch { + keys = [name]; + } @@ - const fallbackItem = localStorage.getItem(name); + const fallbackItem = localStorage.getItem(name); return fallbackItem as unknown as StorageValue<OnboardingStoreState> | null; }, - setItem: (name, value) => { + setItem: (name, value) => { if (typeof window === "undefined") return; - - const valueStr = typeof value === "string" ? value : JSON.stringify(value); + const valueStr = typeof value === "string" ? value : JSON.stringify(value); @@ - localStorage.setItem(key, valueStr); + localStorage.setItem(key, valueStr); @@ - } catch (e) { - localStorage.setItem(name, valueStr); + } catch { + // Silently skip persistence in restricted contexts } }, - removeItem: () => { + removeItem: (_name?: string) => { if (typeof window === "undefined") return; - - const keys = Object.keys(window.localStorage).filter((key) => - key.startsWith("org-creation-onboarding") - ); - keys.forEach((key) => localStorage.removeItem(key)); + try { + // eslint-disable-next-line @calcom/eslint/avoid-web-storage + const keys = Object.keys(window.localStorage).filter((key) => key.startsWith("org-creation-onboarding")); + keys.forEach((key) => localStorage.removeItem(key)); + } catch { + localStorage.removeItem("org-creation-onboarding"); + } },Based on learnings
226-239: Remove unused step param/variable.step is assigned but never used; simplify the hook API.
-export const useOnboarding = (params?: { step?: "start" | "status" | null }) => { +export const useOnboarding = () => { @@ - const step = params?.step ?? null;
📜 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 (7)
apps/web/app/(use-page-wrapper)/settings/organizations/new/resume/page.tsx(1 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/features/ee/organizations/components/CreateANewOrganizationForm.tsx(3 hunks)packages/features/ee/organizations/lib/onboardingStore.ts(10 hunks)packages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.ts(1 hunks)packages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.ts(1 hunks)packages/features/ee/organizations/lib/service/onboarding/__tests__/BillingEnabledOrgOnboardingService.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/(use-page-wrapper)/settings/organizations/new/resume/page.tsx
- apps/web/public/static/locales/en/common.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/features/ee/organizations/lib/onboardingStore.tspackages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.tspackages/features/ee/organizations/lib/service/onboarding/__tests__/BillingEnabledOrgOnboardingService.test.tspackages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.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/features/ee/organizations/lib/onboardingStore.tspackages/features/ee/organizations/components/CreateANewOrganizationForm.tsxpackages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.tspackages/features/ee/organizations/lib/service/onboarding/__tests__/BillingEnabledOrgOnboardingService.test.tspackages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.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/features/ee/organizations/lib/onboardingStore.tspackages/features/ee/organizations/components/CreateANewOrganizationForm.tsxpackages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.tspackages/features/ee/organizations/lib/service/onboarding/__tests__/BillingEnabledOrgOnboardingService.test.tspackages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.ts
**/*.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/ee/organizations/components/CreateANewOrganizationForm.tsx
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.tspackages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.ts
🧬 Code graph analysis (5)
packages/features/ee/organizations/lib/onboardingStore.ts (4)
packages/lib/server/repository/organizationOnboarding.ts (1)
create(32-55)packages/lib/webstorage.ts (1)
localStorage(6-36)packages/features/ee/organizations/lib/service/onboarding/OrganizationOnboardingFactory.ts (1)
isBillingEnabled(40-52)packages/lib/constants.ts (1)
IS_TEAM_BILLING_ENABLED_CLIENT(120-121)
packages/features/ee/organizations/components/CreateANewOrganizationForm.tsx (1)
packages/features/auth/lib/next-auth-options.ts (1)
session(746-771)
packages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.ts (5)
packages/prisma/zod-utils.ts (2)
orgOnboardingInvitedMembersSchema(60-68)orgOnboardingTeamsSchema(70-78)packages/features/ee/organizations/lib/service/onboarding/types.ts (4)
CreateOnboardingIntentInput(19-35)OnboardingIntentResult(37-50)OrganizationOnboardingData(59-77)OrganizationData(79-94)packages/lib/server/repository/organizationOnboarding.ts (1)
OrganizationOnboardingRepository(31-143)packages/lib/constants.ts (1)
IS_SELF_HOSTED(60-60)packages/features/ee/organizations/lib/server/orgCreationUtils.ts (1)
findUserToBeOrgOwner(264-292)
packages/features/ee/organizations/lib/service/onboarding/__tests__/BillingEnabledOrgOnboardingService.test.ts (4)
packages/features/ee/organizations/lib/service/onboarding/types.ts (1)
CreateOnboardingIntentInput(19-35)packages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.ts (1)
BillingEnabledOrgOnboardingService(36-258)packages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.ts (1)
inviteMembersWithNoInviterPermissionCheck(134-242)packages/trpc/server/routers/viewer/organizations/createTeams.handler.ts (1)
createTeamsHandler(31-132)
packages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.ts (11)
packages/prisma/zod-utils.ts (3)
orgOnboardingInvitedMembersSchema(60-68)orgOnboardingTeamsSchema(70-78)teamMetadataStrictSchema(396-412)packages/features/ee/organizations/lib/server/orgCreationUtils.ts (3)
findUserToBeOrgOwner(264-292)assertCanCreateOrg(176-262)setupDomain(294-347)packages/features/ee/organizations/lib/OrganizationPaymentService.ts (1)
OrganizationPaymentService(80-402)packages/features/ee/organizations/lib/OrganizationPermissionService.ts (1)
OrganizationPermissionService(31-141)packages/lib/server/repository/organizationOnboarding.ts (1)
OrganizationOnboardingRepository(31-143)packages/lib/constants.ts (1)
WEBAPP_URL(12-18)packages/emails/email-manager.ts (1)
sendOrganizationCreationEmail(598-600)packages/features/ee/organizations/lib/orgDomains.ts (1)
getOrgFullOrigin(147-155)packages/features/auth/lib/verifyEmail.ts (1)
sendEmailVerification(30-86)packages/trpc/server/routers/viewer/organizations/createTeams.handler.ts (1)
createTeamsHandler(31-132)packages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.ts (1)
inviteMembersWithNoInviterPermissionCheck(134-242)
🪛 Biome (2.1.2)
packages/features/ee/organizations/lib/onboardingStore.ts
[error] 288-288: This array contains an empty slots..
The presences of empty slots may cause incorrect information and might be a typo.
Unsafe fix: Replace hole with undefined
(lint/suspicious/noSparseArray)
⏰ 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 (3)
packages/features/ee/organizations/components/CreateANewOrganizationForm.tsx (2)
13-13: Good: Centralized enum imports.Importing
BillingPeriod,CreationSource, andUserPermissionRolefrom@calcom/prisma/enumscentralizes enum definitions and improves maintainability.
124-149: Clear flow branching logic.The submission handler clearly distinguishes between three flows:
- Resuming existing onboarding (has onboardingId)
- Admin handover (admin creating for someone else)
- Regular flow (user or admin creating for self)
The comments effectively explain each branch, and the intentional asymmetry (immediate mutation for handover vs. local storage for regular) is well-documented.
packages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.ts (1)
441-451: Confirm handling of placeholder teamId (-1) in invites.If UI ever sends invites referencing new teams by teamId -1, the map keyed by the temp id can mis-route when multiple new teams exist. Current logic favors teamName (good). Please confirm UI only uses teamName for new teams; otherwise we should generate per-new-team temp ids.
Also applies to: 456-476
| intentToCreateOrgMutation.mutate({ ...v, creationSource: CreationSource.WEBAPP }); | ||
| } else { | ||
| // Check if this is admin handover flow based on the submitted form value | ||
| const isAdminHandoverFlow = isAdmin && v.orgOwnerEmail !== session.data.user.email; |
There was a problem hiding this comment.
Email comparison may be fragile for detecting admin handover.
The check v.orgOwnerEmail !== session.data.user.email is case-sensitive and may fail if emails differ only in case (e.g., "Admin@example.com" vs "admin@example.com").
Apply this diff to normalize email comparison:
- const isAdminHandoverFlow = isAdmin && v.orgOwnerEmail !== session.data.user.email;
+ const isAdminHandoverFlow = isAdmin && v.orgOwnerEmail.toLowerCase() !== session.data.user.email?.toLowerCase();Note: A previous review comment questioned why the userId comparison was removed. Email comparison is generally less robust than userId comparison, but should work if case-normalized.
🤖 Prompt for AI Agents
In packages/features/ee/organizations/components/CreateANewOrganizationForm.tsx
around line 129, the admin handover check uses a case-sensitive email comparison
which can mis-detect identical emails with different casing; normalize both
sides (e.g., trim and toLowerCase()) before comparing so the check becomes
case-insensitive (or, if possible, prefer comparing userId instead as a more
robust alternative).
packages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/features/ee/organizations/lib/onboardingStore.ts (5)
232-232: Critical bug: String environment variable treated as boolean.
NEXT_PUBLIC_IS_E2Eis a string, so even"false"would be truthy and incorrectly disable billing. This was flagged in a previous review but remains unfixed.Parse the string explicitly:
- const isBillingEnabled = process.env.NEXT_PUBLIC_IS_E2E ? false : IS_TEAM_BILLING_ENABLED_CLIENT; + const isBillingEnabled = process.env.NEXT_PUBLIC_IS_E2E === "true" ? false : IS_TEAM_BILLING_ENABLED_CLIENT;
288-288: Fix sparse dependency array.Line 288 contains an empty slot (
, ,betweenresetandpath). Remove the stray comma:- }, [organizationOnboarding, isLoadingOrgOnboarding, isAdmin, reset, , path]); + }, [organizationOnboarding, isLoadingOrgOnboarding, isAdmin, reset, path]);
98-110: Race condition: Manual localStorage manipulation competes with persist middleware.Despite being marked as addressed in previous reviews, lines 101-107 still manually write to and remove from localStorage while the persist middleware is also managing it. This creates a race where state updates may be lost or overwritten.
The custom storage implementation (lines 169-221) already handles onboardingId-aware persistence. Let the middleware handle all persistence automatically:
setOnboardingId: (onboardingId) => { - const currentId = get().onboardingId; - // If we're setting an onboardingId for the first time, migrate the data - if (onboardingId && !currentId && typeof window !== "undefined") { - const currentState = get(); - // Save to the new key - const newKey = getStorageKey(onboardingId); - localStorage.setItem(newKey, JSON.stringify({ state: currentState, version: 0 })); - // Remove the old key - localStorage.removeItem("org-creation-onboarding"); - } set({ onboardingId }); },The persist middleware will automatically call
setItemin the custom storage, which already migrates keys based on onboardingId.
124-161: Race condition: Manual localStorage operations compete with persist middleware.Similar to
setOnboardingId, theresetaction (lines 126-148, 152-157) manually manages localStorage while the persist middleware is also active. This creates race conditions where state may be inconsistent.Simplify by relying on the custom storage implementation to handle all persistence:
reset: (state) => { if (state) { - const currentId = get().onboardingId; - const newId = state.onboardingId; - - // If onboardingId is changing, handle localStorage migration - if (typeof window !== "undefined" && currentId !== newId) { - // Remove old key if it exists - if (currentId) { - localStorage.removeItem(getStorageKey(currentId)); - } else { - localStorage.removeItem("org-creation-onboarding"); - } - - // Set new state which will be persisted with new key - set(state); - - // Immediately save to new key - if (newId) { - const newKey = getStorageKey(newId); - localStorage.setItem(newKey, JSON.stringify({ state, version: 0 })); - } - } else { - set(state); - } + set(state); } else { - // Clear the localStorage entry for this store - if (typeof window !== "undefined") { - const currentId = get().onboardingId; - if (currentId) { - localStorage.removeItem(getStorageKey(currentId)); - } else { - localStorage.removeItem("org-creation-onboarding"); - } - } set(initialState); } },The custom storage's
removeItemwill automatically handle cleanup when the store resets.
169-221: Critical: Directwindow.localStorageaccess bypasses safe wrapper.Lines 173-174 and 216-217 use
Object.keys(window.localStorage)which directly accesses the native localStorage object, bypassing the safe wrapper imported at line 8. This can throw exceptions in restricted contexts (Chrome Incognito third-party frames, quota exceeded, etc.).Wrap the key enumeration in try-catch and use the safe wrapper consistently:
getItem: (name): StorageValue<OnboardingStoreState> | null => { if (typeof window === "undefined") return null; - const keys = Object.keys(window.localStorage).filter((key) => - key.startsWith("org-creation-onboarding") - ); + let keys: string[] = []; + try { + // eslint-disable-next-line @calcom/eslint/avoid-web-storage + keys = Object.keys(window.localStorage).filter((key) => + key.startsWith("org-creation-onboarding") + ); + } catch (e) { + // In restricted contexts, fall back to checking only the default key + keys = [name]; + } for (const key of keys) { const item = localStorage.getItem(key);removeItem: () => { if (typeof window === "undefined") return; - const keys = Object.keys(window.localStorage).filter((key) => - key.startsWith("org-creation-onboarding") - ); - keys.forEach((key) => localStorage.removeItem(key)); + try { + // eslint-disable-next-line @calcom/eslint/avoid-web-storage + const keys = Object.keys(window.localStorage).filter((key) => + key.startsWith("org-creation-onboarding") + ); + keys.forEach((key) => localStorage.removeItem(key)); + } catch (e) { + // In restricted contexts, can't enumerate; just remove default key + localStorage.removeItem("org-creation-onboarding"); + } },
🧹 Nitpick comments (1)
packages/features/ee/organizations/lib/onboardingStore.ts (1)
238-238: Remove unusedstepvariable.The
stepvariable is extracted but never used in the function body. Clean up the dead code:export const useOnboarding = (params?: { step?: "start" | "status" | null }) => { const session = useSession(); const router = useRouter(); const path = usePathname(); const isAdmin = session.data?.user?.role === UserPermissionRole.ADMIN; const isBillingEnabled = process.env.NEXT_PUBLIC_IS_E2E ? false : IS_TEAM_BILLING_ENABLED_CLIENT; const searchParams = useSearchParams(); const { data: organizationOnboarding, isPending: isLoadingOrgOnboarding } = trpc.viewer.organizations.getOrganizationOnboarding.useQuery(); const { reset, onboardingId } = useOnboardingStore(); - const step = params?.step ?? null;Also consider removing the
stepparameter from the function signature if it's not needed:-export const useOnboarding = (params?: { step?: "start" | "status" | null }) => { +export const useOnboarding = () => {
📜 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/features/ee/organizations/lib/onboardingStore.ts(10 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/features/ee/organizations/lib/onboardingStore.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/features/ee/organizations/lib/onboardingStore.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/features/ee/organizations/lib/onboardingStore.ts
🧬 Code graph analysis (1)
packages/features/ee/organizations/lib/onboardingStore.ts (4)
packages/lib/server/repository/organizationOnboarding.ts (1)
create(32-55)packages/lib/webstorage.ts (1)
localStorage(6-36)packages/features/ee/organizations/lib/service/onboarding/OrganizationOnboardingFactory.ts (1)
isBillingEnabled(40-52)packages/lib/constants.ts (1)
IS_TEAM_BILLING_ENABLED_CLIENT(120-121)
🪛 Biome (2.1.2)
packages/features/ee/organizations/lib/onboardingStore.ts
[error] 288-288: This array contains an empty slots..
The presences of empty slots may cause incorrect information and might be a typo.
Unsafe fix: Replace hole with undefined
(lint/suspicious/noSparseArray)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
16 issues found across 46 files
Prompt for AI agents (all 16 issues)
Understand the root cause of the following 16 issues and fix them.
<file name="packages/features/ee/organizations/components/CreateANewOrganizationForm.tsx">
<violation number="1" location="packages/features/ee/organizations/components/CreateANewOrganizationForm.tsx:147">
Skipping the intentToCreateOrg mutation here means normal/admin-self onboarding flows never create the onboarding intent, so onboardingId stays null and later steps that depend on that record will fail.</violation>
</file>
<file name="apps/web/modules/settings/organizations/new/resume-view.tsx">
<violation number="1" location="apps/web/modules/settings/organizations/new/resume-view.tsx:49">
Resuming onboarding calls reset without clearing invitedMembers/teams, so stale persisted entries leak into the flow and can trigger incorrect invitations.</violation>
</file>
<file name="packages/features/ee/organizations/lib/service/onboarding/SelfHostedOnboardingService.ts">
<violation number="1" location="packages/features/ee/organizations/lib/service/onboarding/SelfHostedOnboardingService.ts:86">
Avoid resetting the onboarding domain configuration state; otherwise resumed flows will try to provision an already-configured domain and fail.</violation>
</file>
<file name="packages/features/ee/organizations/components/AdminOnboardingHandover.tsx">
<violation number="1" location="packages/features/ee/organizations/components/AdminOnboardingHandover.tsx:28">
The Alert message is hard-coded in English instead of pulling from t(), which breaks localization requirements.</violation>
<violation number="2" location="packages/features/ee/organizations/components/AdminOnboardingHandover.tsx:39">
This instructional paragraph embeds hard-coded English copy instead of retrieving it via t(), violating the localization guideline.</violation>
</file>
<file name="packages/features/ee/organizations/lib/server/orgCreationUtils.test.ts">
<violation number="1" location="packages/features/ee/organizations/lib/server/orgCreationUtils.test.ts:59">
The test description says this case should return true, but the assertion expects false, which is misleading. Please align the description with the assertion so the test clearly communicates its intent.</violation>
</file>
<file name="apps/web/modules/settings/organizations/new/_components/OnboardMembersView.tsx">
<violation number="1" location="apps/web/modules/settings/organizations/new/_components/OnboardMembersView.tsx:216">
Rule violated: **Avoid Logging Sensitive Information**
Logging orgOwnerEmail (an email address) alongside other identifiers in console.error violates the security guideline to avoid logging sensitive information.</violation>
<violation number="2" location="apps/web/modules/settings/organizations/new/_components/OnboardMembersView.tsx:228">
`creationSource` is validated as a Prisma enum (`CreationSource`), but this mutation now sends the uppercase string "WEBAPP". That value is invalid (`CreationSource.WEBAPP` resolves to the lowercase string "webapp"), so intentToCreateOrg will throw and the onboarding flow breaks.</violation>
</file>
<file name="packages/trpc/server/routers/viewer/organizations/intentToCreateOrg.handler.test.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/organizations/intentToCreateOrg.handler.test.ts:29">
Use the CreationSource enum constant instead of a raw string so the mock input stays type-safe and the import isn’t left unused.</violation>
</file>
<file name="packages/features/ee/organizations/lib/utils.ts">
<violation number="1" location="packages/features/ee/organizations/lib/utils.ts:49">
Trailing whitespace in the domain makes personal addresses look like company emails because `emailParts[1]` isn’t trimmed before the includes check. Please trim the domain before comparing so values like `"user@gmail.com "` aren’t misclassified.</violation>
</file>
<file name="packages/features/ee/organizations/lib/service/onboarding/OrganizationOnboardingSetup.md">
<violation number="1" location="packages/features/ee/organizations/lib/service/onboarding/OrganizationOnboardingSetup.md:36">
The factory never checks user.role, so this ADMIN-specific branch in the decision logic is inaccurate and misleads readers.</violation>
<violation number="2" location="packages/features/ee/organizations/lib/service/onboarding/OrganizationOnboardingSetup.md:143">
This scenario description is incorrect: when billing is disabled, all users—including regular users—get the self-hosted onboarding service.</violation>
</file>
<file name="packages/features/ee/organizations/lib/onboardingStore.ts">
<violation number="1" location="packages/features/ee/organizations/lib/onboardingStore.ts:174">
`onboardingId` is used inside this effect but was dropped from the dependency array, so the sync logic will miss onboardingId updates; please include it and remove the empty slot.</violation>
</file>
<file name="packages/features/ee/organizations/lib/service/onboarding/__tests__/OrganizationOnboardingFactory.test.ts">
<violation number="1" location="packages/features/ee/organizations/lib/service/onboarding/__tests__/OrganizationOnboardingFactory.test.ts:114">
The test at this line duplicates the first "regular user with billing enabled" test verbatim, adding no new coverage and increasing maintenance surface. Please remove or consolidate the duplicate scenario.</violation>
</file>
<file name="packages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.ts">
<violation number="1" location="packages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.ts:38">
Rule violated: **Avoid Logging Sensitive Information**
Logging the onboarding payload here serializes `invitedMembers` and `teams` objects that contain email addresses, and the later debug call logs `this.user.email`. This directly violates our "Avoid Logging Sensitive Information" rule because it prints PII into application logs. Please remove or sanitize these values before logging so no emails reach the logs.</violation>
</file>
<file name="packages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.ts">
<violation number="1" location="packages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.ts:493">
Rule violated: **Avoid Logging Sensitive Information**
This log serializes invited member email addresses into the info log, leaking PII and violating the "Avoid Logging Sensitive Information" rule. Please remove or redact the email values before logging.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
packages/features/ee/organizations/components/CreateANewOrganizationForm.tsx
Show resolved
Hide resolved
packages/features/ee/organizations/lib/service/onboarding/SelfHostedOnboardingService.ts
Show resolved
Hide resolved
packages/features/ee/organizations/components/AdminOnboardingHandover.tsx
Show resolved
Hide resolved
packages/features/ee/organizations/components/AdminOnboardingHandover.tsx
Show resolved
Hide resolved
...ures/ee/organizations/lib/service/onboarding/__tests__/OrganizationOnboardingFactory.test.ts
Show resolved
Hide resolved
apps/web/modules/settings/organizations/new/_components/OnboardMembersView.tsx
Show resolved
Hide resolved
packages/features/ee/organizations/lib/service/onboarding/BillingEnabledOrgOnboardingService.ts
Show resolved
Hide resolved
packages/features/ee/organizations/lib/service/onboarding/BaseOnboardingService.ts
Show resolved
Hide resolved
|
just tested this branch again and got this:
edit: while the error persists in the 4th flow, I don't know how to replicate this error for other environment. might be just my browser issue (zen browser). it's working on my Chrome, and Sean's zen browser. |
There was a problem hiding this comment.
Reviewed changes from recent commits (found 1 issue).
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/features/ee/organizations/lib/onboardingStore.test.ts">
<violation number="1" location="packages/features/ee/organizations/lib/onboardingStore.test.ts:170">
Clearing mockRouterPush before asserting wipes out call history, so the test can’t fail if additional redirects fire. Remove the clear and assert on the actual call count instead.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
…nboarding-refactor' into feat/organziation-onboarding-refactor
eunjae-lee
left a comment
eunjae-lee
left a comment
There was a problem hiding this comment.
all 4 flows work well for me
|
|
||
| if (isBillingEnabled) { | ||
| return new BillingEnabledOrgOnboardingService(user); | ||
| } else { |
There was a problem hiding this comment.
This else is redundant
* feat: redirect to new onboarding flow * Getting started * Brand details * Preview organization brands * Orgs team pages * Invite team steps * Move to global zustand store * Few darkmdoe fixes * Wip onboarding + stripe flow * Default plan state Server Action for gettting slug satus of org * Remove onboardingId * Confirmation prompt * Update old onboarding flow handlers to handle new fields * update onboarding hook * Filter out organization section for none -company emails * Match placeholders to users domain * Drop migration * Wip new onboarding intent * WIP flow for self-hosted. Same service call just split logic * WIP * Add TODO * Use onboarding user type instead of trpc session * WIP * WIP * pass role and team name from onboarding to save in schema * Add test to ensure role + name + team are persisted into onboarding table * migrate roles to enum values * Update ENUM * Fix type error * Redirect if flag is disabled * Remove web * WIP * WIP * Fix migration * Fix calls * User onboarding User types instead of trpc session * Fix factory tests * Fix flow for self hoste * Type error * More type fixes * Fix handler tests * Fix enum return type being different * Use consistant types across the oganization stuff * Fix * Use TEAM_BILLING for e2e test * Refactor is not company email and add tests * Fix * Fix * Refactor flow to submit after form complete * Fix flow with billing disabled * Fix tests * Apply suggestion from @coderabbitai[bot] Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Rename and move test files * WIP * Fix types * Update repo paths + tests * Move to service folder * Fix tests * Fix types * Remove old test files * Restore lock * Fix path * Fix tests with new paths and factory logic * Fix updaetdAt * WIP onboardingID isolation * Fix e2e test * verify test * Code rabbit * Rename SelfHostedOnboardongService -> SelfHostedOrganizationOnboardingService * Fix stores * Fix type error * Fix types * remove tsignore * Apply suggestion from @coderabbitai[bot] Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * NITS * Add the logic to auto complete admin org when billing enabled * Fix store being weird * We need to return the parsed value * fixes * sync from db always * Add onboardingSgtore tests * fix test * remove step and status --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Hariom Balhara <hariombalhara@gmail.com>
4 participants
Setup guide for configuring billing between selfhosted and billing enabled factory can be fouund in this readme
https://github.com/calcom/cal.com/blob/f3b9fb6a3b17ad3596752112aed0be42ee531f8a/packages/features/ee/organizations/lib/onboarding/OrganizationOnboardingSetup.md
How to test
Setup stripe
Enabled IS_STRIPE_ENABLED,IS_TEAM_BILLING_ENABLED,IS_TEAM_BILLING_ENABLED_CLIENT in constants
As an admin go create onboarding for your own admin@example.com account
then go create it for teampro@example.com
then reset your DB.
Then login as teampro@example.com and create an onboarding flow with no custom plan
Disable stripe
As admin create an org for yourself
As an admin create org for teampro@example.com
Reset DB
Create an org for teampro@example.com when logged in as teampro@example.com