fix: skip Select Account step if only one account is available#23432
fix: skip Select Account step if only one account is available#23432dhairyashiil merged 32 commits intocalcom:mainfrom
Conversation
|
@sahitya-chandra is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe server-side onboarding logic now computes an initialStep based on the URL step param and whether the user has teams, defaulting to EVENT_TYPES when there are none. Rendering and redirects use this initialStep. If there are no teams and no existing credential, a default installation credential is created on the fly with error handling for duplicates. The step passed to the client is updated to initialStep. On the client, step numbering and max steps are adjusted to account for absence of teams, with teams defaulting to an empty array. Playwright fixtures are updated to conditionally skip the accounts step based on the current URL. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
✨ 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 |
github-actions
bot
added
consumer
ui
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/modules/apps/installation/[[...step]]/step-view.tsx (1)
190-209: Fix shadowed variable and avoid translating raw error text.
const messageshadows the outermessage, andt(message)treats plain messages as keys.- onError: (err) => { - let message = ""; - if (err instanceof HttpError) { - const message = `${err.statusCode}: ${err.message}`; - showToast(message, "error"); - } + onError: (err) => { + let message = ""; + if (err instanceof HttpError) { + message = `${err.statusCode}: ${err.message}`; + } ... - showToast(message ? t(message) : t(err.message), "error"); + showToast(message || err.message, "error"); },apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts (2)
180-195: Select only needed credential fields (avoid fetchingkey).Complies with the guideline and reduces risk of accidental exposure.
- const appInstalls = await prisma.credential.findMany({ + const appInstalls = await prisma.credential.findMany({ where: { OR: [ { appId: appSlug, userId: userId }, teamIds && Boolean(teamIds.length) ? { appId: appSlug, teamId: { in: teamIds } } : {}, ], }, + select: { id: true, appId: true, userId: true, teamId: true }, });
329-345: Don’t returnappin props (it includeskeys).
appisn’t used by the client component and may contain sensitive metadata (keys). Drop it from SSR props.- app, appMetadata, showEventTypesStep, step: initialStep,If you truly need
app, restrict the selection atgetAppBySlugto non-sensitive fields and only those used by the client.
🧹 Nitpick comments (1)
apps/web/modules/apps/installation/[[...step]]/step-view.tsx (1)
349-350: Prefer named export over default.Improves tree-shaking and refactors.
Example:
-export default OnboardingPage; +export { OnboardingPage };And adjust imports accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts(4 hunks)apps/web/modules/apps/installation/[[...step]]/step-view.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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:
apps/web/modules/apps/installation/[[...step]]/step-view.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/modules/apps/installation/[[...step]]/step-view.tsxapps/web/lib/apps/installation/[[...step]]/getServerSideProps.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:
apps/web/modules/apps/installation/[[...step]]/step-view.tsxapps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts
**/*.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:
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts
🧬 Code graph analysis (1)
apps/web/modules/apps/installation/[[...step]]/step-view.tsx (1)
packages/lib/constants.ts (1)
WEBAPP_URL(12-18)
🔇 Additional comments (4)
apps/web/modules/apps/installation/[[...step]]/step-view.tsx (2)
105-106: Good: default empty array prevents undefined branches.Initializing
teams = []simplifies consumers and avoids nullish checks.
133-134: Verify progress indicator counts when Accounts is skipped.When
teams.length === 0or!installableOnTeams,stepNumberandmaxStepsmust remain consistent (Event Types = 1, Configure = 2). Please sanity-check the UI for: (a) no teams, (b) teams exist butinstallableOnTeams === false.Also applies to: 139-140, 151-157
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts (2)
323-326: Keep the hard guard once autogeneration is removed.With the above removal, this redirect becomes the correct, safe path for users lacking a credential.
232-242: RecomputehasTeamsafter expanding org subteams
Move thehasTeamscheck (or deferinitialStepdecision) to after the block that appendssubTeams, so users under an org without direct teams aren’t misrouted.
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts
Outdated
Show resolved
Hide resolved
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts (1)
180-197: Prisma: select only needed fields and avoid{}insideOR(security + correctness).This query fetches full
credentialrows (includingkey) and uses{}inOR, which can match everything. Select minimal fields and scopeappIdoutsideOR.Apply this diff:
const getAppInstallsBySlug = async (appSlug: string, userId: number, teamIds?: number[]) => { - const appInstalls = await prisma.credential.findMany({ - where: { - OR: [ - { - appId: appSlug, - userId: userId, - }, - teamIds && Boolean(teamIds.length) - ? { - appId: appSlug, - teamId: { in: teamIds }, - } - : {}, - ], - }, - }); + const appInstalls = await prisma.credential.findMany({ + where: { + appId: appSlug, + OR: [ + { userId }, + ...(teamIds?.length ? [{ teamId: { in: teamIds } }] : []), + ], + }, + select: { id: true, userId: true, teamId: true }, + }); return appInstalls; };
♻️ Duplicate comments (1)
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts (1)
330-331: Fix typo in comment.Apply this diff:
- // dont allow app installation without credentialId + // don't allow app installation without credentialId
🧹 Nitpick comments (1)
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts (1)
324-324: Avoid noisy server logs in SSR.Drop the
console.logor guard it behind non-production checks / app logger.Apply this diff:
- console.log(`Auto-installed ${parsedAppSlug} for user ${user.id} (personal account - no teams)`); + if (process.env.NODE_ENV !== "production") { + console.debug(`Auto-installed ${parsedAppSlug} for user ${user.id} (no teams)`); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts(5 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:
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.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:
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.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:
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: sahitya-chandra
PR: calcom/cal.com#23432
File: apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts:309-322
Timestamp: 2025-08-28T21:44:46.004Z
Learning: When a user has no teams during app installation in Cal.com, instead of creating placeholder credentials, use createDefaultInstallation() from calcom/app-store/_utils/installation server-side to auto-install the app for their personal account. This creates proper credentials through the standard installation flow and eliminates the need for account selection when there's only one option.
📚 Learning: 2025-08-28T21:44:46.004Z
Learnt from: sahitya-chandra
PR: calcom/cal.com#23432
File: apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts:309-322
Timestamp: 2025-08-28T21:44:46.004Z
Learning: When a user has no teams during app installation in Cal.com, instead of creating placeholder credentials, use createDefaultInstallation() from calcom/app-store/_utils/installation server-side to auto-install the app for their personal account. This creates proper credentials through the standard installation flow and eliminates the need for account selection when there's only one option.
Applied to files:
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts
⏰ 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
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts
Outdated
Show resolved
Hide resolved
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts (4)
69-74: Critical: Do not fetch or send app.keys to the clientYou're selecting
keysand then returningappin props, which risks leaking secrets to the browser. Removekeysfrom the select.const getAppBySlug = async (appSlug: string) => { const app = await prisma.app.findUnique({ where: { slug: appSlug, enabled: true }, - select: { slug: true, keys: true, enabled: true, dirName: true }, + select: { slug: true, enabled: true, dirName: true }, }); return app; };Also applies to: 343-349
180-195: Fix query logic: OR with empty object returns all credentials (data leak + wrong matches)
OR: [A, {}]matches everything. Also over-selects. BuildORdynamically and scope byappId; select only needed fields.- const appInstalls = await prisma.credential.findMany({ - where: { - OR: [ - { - appId: appSlug, - userId: userId, - }, - teamIds && Boolean(teamIds.length) - ? { - appId: appSlug, - teamId: { in: teamIds }, - } - : {}, - ], - }, - }); + const or: Prisma.CredentialWhereInput[] = [{ userId }]; + if (teamIds?.length) or.push({ teamId: { in: teamIds } }); + const appInstalls = await prisma.credential.findMany({ + where: { appId: appSlug, OR: or }, + select: { id: true, userId: true, teamId: true }, + });
76-99: Minimize relation payloads: restrict destinationCalendar selectionAvoid shipping unnecessary/sensitive fields in SSR props. Select only what the UI needs (typically presence/id).
const eventTypeSelect = { @@ - destinationCalendar: true, + destinationCalendar: { select: { id: true } }, } satisfies Prisma.EventTypeSelect;
263-269: Also restrict the standalone destinationCalendar queryLimit fields to avoid leaking credentials/foreign keys.
- const destinationCalendar = await prisma.destinationCalendar.findFirst({ + const destinationCalendar = await prisma.destinationCalendar.findFirst({ where: { userId: user.id, eventTypeId: null, - }, + }, + select: { id: true }, });
🧹 Nitpick comments (3)
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts (3)
225-232: Harden step validation to avoid accidental 500sPrefer safeParse and fallback to a default instead of throwing.
- stepsEnum.parse(initialStep); + const validated = stepsEnum.safeParse(initialStep); + initialStep = validated.success + ? validated.data + : hasTeams ? AppOnboardingSteps.ACCOUNTS_STEP : AppOnboardingSteps.EVENT_TYPES_STEP;
309-336: Auto-install path is solid; idempotency handledGood use of
createDefaultInstallation()and P2002 recovery. Ifkeyisn’t required by the helper, drop it to reduce ambiguity.- key: {},
338-341: Tiny typo in commentUse “don’t”.
- // dont allow app installation without credentialId + // don't allow app installation without credentialId
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts(5 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:
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.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:
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.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:
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: sahitya-chandra
PR: calcom/cal.com#23432
File: apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts:309-322
Timestamp: 2025-08-28T21:44:46.004Z
Learning: When a user has no teams during app installation in Cal.com, instead of creating placeholder credentials, use createDefaultInstallation() from calcom/app-store/_utils/installation server-side to auto-install the app for their personal account. This creates proper credentials through the standard installation flow and eliminates the need for account selection when there's only one option.
📚 Learning: 2025-08-28T21:44:46.004Z
Learnt from: sahitya-chandra
PR: calcom/cal.com#23432
File: apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts:309-322
Timestamp: 2025-08-28T21:44:46.004Z
Learning: When a user has no teams during app installation in Cal.com, instead of creating placeholder credentials, use createDefaultInstallation() from calcom/app-store/_utils/installation server-side to auto-install the app for their personal account. This creates proper credentials through the standard installation flow and eliminates the need for account selection when there's only one option.
Applied to files:
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts
⏰ 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). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
apps/web/lib/apps/installation/[[...step]]/getServerSideProps.ts (2)
205-206: Step param normalization looks goodCovers both string and array cases. ✔️
245-245: Using initialStep for gating is correctSwitching the gate to
initialStepensures the skip logic applies consistently. ✔️
E2E results are ready! |
|
@sahitya-chandra Tests are failing. can you please fix them? |
|
@anikdhabal any updates on this sir ?? |
It will be reviewed. It is going to take some time. |
| if (!hasTeams && initialStep === AppOnboardingSteps.EVENT_TYPES_STEP) { | ||
| if (!personalAccount.alreadyInstalled) { | ||
| try { | ||
| const newCredential = await createDefaultInstallation({ | ||
| appType: appMetadata.type, | ||
| user: { id: user.id }, | ||
| slug: parsedAppSlug, | ||
| key: {}, | ||
| teamId: undefined, | ||
| }); | ||
| credentialId = newCredential.id; | ||
| console.log(`Auto-installed ${parsedAppSlug} for user ${user.id} (personal account - no teams)`); | ||
| } catch (error) { | ||
| console.error(`Failed to auto-install app ${parsedAppSlug} for user ${user.id}:`, error); | ||
| if ((error as any)?.code === "P2002") { | ||
| const existing = await prisma.credential.findFirst({ | ||
| where: { appId: parsedAppSlug, userId: user.id, teamId: null }, | ||
| select: { id: true }, | ||
| }); | ||
| if (existing?.id) credentialId = existing.id; | ||
| } else { | ||
| return { redirect: { permanent: false, destination: "/apps" } }; | ||
| } | ||
| } | ||
| } else { | ||
| credentialId = appInstalls.find((item) => item.userId === user.id)?.id ?? null; | ||
| } | ||
| } else if (parsedTeamIdParam) { | ||
| credentialId = appInstalls.find((item) => item.teamId === parsedTeamIdParam)?.id ?? null; | ||
| } else { | ||
| credentialId = appInstalls.find((item) => item.userId === user.id)?.id ?? null; | ||
| } |
There was a problem hiding this comment.
This doesn't look clean to me, also so many nested if else. Try to optimize it
| teamId: undefined, | ||
| }); | ||
| credentialId = newCredential.id; | ||
| console.log(`Auto-installed ${parsedAppSlug} for user ${user.id} (personal account - no teams)`); |
There was a problem hiding this comment.
Pls remove all the console log
| console.log(`Auto-installed ${parsedAppSlug} for user ${user.id} (personal account - no teams)`); | ||
| } catch (error) { | ||
| console.error(`Failed to auto-install app ${parsedAppSlug} for user ${user.id}:`, error); | ||
| if ((error as any)?.code === "P2002") { |
There was a problem hiding this comment.
Let's avoids the any type here and found a diff approach
|
@anikdhabal I will fix them soon sir |
|
This PR is being marked as stale due to inactivity. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
4 participants
What does this PR do?
Visual Demo (For contributors especially)
User with only 1 account (OWNER/ADMIN of 0 team)
Screencast.from.2025-08-29.02-25-11.webm
User with personal and team account both (OWNER/ADMIN of at least 1 team)
Screencast.from.2025-08-29.02-27-12.webm
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist