fix: allow team with same slug for diff cases #24029
Conversation
WalkthroughSlug uniqueness check during team updates was moved to TeamRepository.isSlugAvailableForUpdate and is performed only when a new Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
dosubot
bot
added
the
teams
anikdhabal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/playwright/teams.e2e.ts (1)
7-14: testName is used but not imported — tests will fail at runtimeRe-add testName import from testUtils (assuming it’s still exported).
-import { +import { bookTimeSlot, confirmReschedule, fillStripeTestCheckout, selectFirstAvailableTimeSlotNextMonth, - submitAndWaitForResponse, + submitAndWaitForResponse, + testName, } from "./lib/testUtils";
🧹 Nitpick comments (3)
packages/features/ee/teams/pages/team-profile-view.tsx (2)
201-202: Localize toast stringUse t() for the “Copied to clipboard” message to comply with i18n guidelines.
- showToast("Copied to clipboard", "success"); + showToast(t("copied_to_clipboard"), "success");
495-495: Prefer named export over defaultImproves tree-shaking and refactoring.
-export default ProfileView; +export { ProfileView };packages/trpc/server/routers/viewer/teams/update.handler.ts (1)
34-50: Make slug uniqueness check case-insensitive and exclude selfFor parity with the REST handler and to avoid case-based collisions, compare case-insensitive and exclude the current team in the query rather than post-filtering.
- if (input.slug) { - const whereClause: Prisma.TeamWhereInput = { slug: input.slug, parentId: null }; + if (input.slug) { + const whereClause: Prisma.TeamWhereInput = { + slug: { equals: input.slug, mode: "insensitive" }, + parentId: null, + NOT: { id: input.id }, + }; const currentTeam = await prisma.team.findUnique({ where: { id: input.id }, select: { parentId: true }, }); if (currentTeam?.parentId) { whereClause.parentId = currentTeam?.parentId; } - const userConflict = await prisma.team.findMany({ - where: whereClause, - }); - if (userConflict.some((t) => t.id !== input.id)) { - throw new TRPCError({ code: "CONFLICT", message: "Slug already in use." }); - } + const conflict = await prisma.team.findFirst({ where: whereClause }); + if (conflict) throw new TRPCError({ code: "CONFLICT", message: "Slug already in use." }); }
📜 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 (5)
apps/api/v1/pages/api/teams/[teamId]/_patch.ts(1 hunks)apps/web/playwright/fixtures/users.ts(5 hunks)apps/web/playwright/teams.e2e.ts(2 hunks)packages/features/ee/teams/pages/team-profile-view.tsx(1 hunks)packages/trpc/server/routers/viewer/teams/update.handler.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/api/v1/pages/api/teams/[teamId]/_patch.tsapps/web/playwright/teams.e2e.tspackages/trpc/server/routers/viewer/teams/update.handler.tsapps/web/playwright/fixtures/users.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/api/v1/pages/api/teams/[teamId]/_patch.tspackages/features/ee/teams/pages/team-profile-view.tsxapps/web/playwright/teams.e2e.tspackages/trpc/server/routers/viewer/teams/update.handler.tsapps/web/playwright/fixtures/users.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/api/v1/pages/api/teams/[teamId]/_patch.tspackages/features/ee/teams/pages/team-profile-view.tsxapps/web/playwright/teams.e2e.tspackages/trpc/server/routers/viewer/teams/update.handler.tsapps/web/playwright/fixtures/users.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/teams/pages/team-profile-view.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-26T20:23:28.396Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/calAIPhone/providers/retellAI/services/AgentService.ts:83-88
Timestamp: 2025-08-26T20:23:28.396Z
Learning: In calcom/cal.com PR #22995, the workflow update handler in packages/trpc/server/routers/viewer/workflows/update.handler.ts includes workflow-level authorization via isAuthorized(userWorkflow, ctx.user.id, "workflow.update") which validates the user can update the workflow before calling updateToolsFromAgentId (per maintainer Udit-takkar).
Applied to files:
packages/trpc/server/routers/viewer/teams/update.handler.ts
🧬 Code graph analysis (3)
packages/features/ee/teams/pages/team-profile-view.tsx (1)
packages/ui/components/button/Button.tsx (1)
Button(221-349)
apps/web/playwright/teams.e2e.ts (1)
apps/web/playwright/lib/testUtils.ts (1)
submitAndWaitForResponse(488-497)
packages/trpc/server/routers/viewer/teams/update.handler.ts (1)
packages/platform/libraries/index.ts (1)
TRPCError(66-66)
⏰ 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: Check for E2E label
🔇 Additional comments (6)
packages/features/ee/teams/pages/team-profile-view.tsx (1)
469-475: Test id addition on submit button — LGTMThe data-testid enables stable e2e selection without altering behavior.
apps/web/playwright/teams.e2e.ts (2)
343-346: Confirm waitForResponse URL matching for tRPC requestsUsing a plain string may not match if the request has query params (e.g., ?batch=1). Ensure the helper matches via predicate or regex, or confirm your tRPC client uses the exact path.
If needed, update the helper to accept a predicate/RegExp; otherwise these waits can flake or never resolve.
Also applies to: 404-406
305-408: New slug validation e2e coverage — solid scenariosCovers cross-org allow, intra-org deny, and org vs non-org allow. Good alignment with backend changes.
Please re-run the suite after fixing the import and status code to ensure green.
apps/web/playwright/fixtures/users.ts (2)
163-177: Deterministic team slug support — LGTMOptional teamSlug param and precedence in slug derivation look good and improve test reliability.
Also applies to: 182-185
720-741: getAllTeamMembership helper — LGTMUseful addition; filters out org teams and parses metadata.
apps/api/v1/pages/api/teams/[teamId]/_patch.ts (1)
58-66: Ensure DB uniqueness aligns with new scoping (slug within parent context)If the DB still enforces a global unique constraint on team.slug, writes will fail even if app logic allows duplicates across different parents. Confirm composite unique (lowercased) on (parentId, slug) or equivalent case-insensitive index.
E2E results are ready! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/lib/server/repository/team.ts (1)
401-420: Tighten slug check: exclude self in WHERE, use case-insensitive compare, and avoid findManyCurrent approach does an extra pass in JS and may be case-sensitive. Recommend building the exclusion into the query, using
findFirst, and comparing case-insensitively.
- Please confirm if slug uniqueness should be case-insensitive (previous checks used
mode: "insensitive"). If yes, apply the change below.- async isSlugAvailableForUpdate({ id, slug }: { id: number; slug: string }) { - const whereClause: Prisma.TeamWhereInput = { slug, parentId: null }; - - const currentTeam = await this.prismaClient.team.findUnique({ - where: { id }, - select: { parentId: true }, - }); - - if (currentTeam?.parentId) { - whereClause.parentId = currentTeam.parentId; - } - - const conflictingTeams = await this.prismaClient.team.findMany({ - where: whereClause, - select: { id: true }, - }); - - return !conflictingTeams.some((team) => team.id !== id); - } + async isSlugAvailableForUpdate({ id, slug }: { id: number; slug: string }) { + const currentTeam = await this.prismaClient.team.findUnique({ + where: { id }, + select: { parentId: true }, + }); + + const conflict = await this.prismaClient.team.findFirst({ + where: { + slug: { equals: slug, mode: "insensitive" }, + parentId: currentTeam?.parentId ?? null, + NOT: { id }, + }, + select: { id: true }, + }); + + return !conflict; + }Additionally, please confirm intended scope for root-level teams (parentId null). As written, this enforces global uniqueness across both organizations and standalone teams with parentId null—verify this aligns with “allow same slug for diff cases.”
packages/trpc/server/routers/viewer/teams/update.handler.ts (1)
34-40: Optional: move slug check after fetching prevTeam to short‑circuit unchanged slugs and return NOT_FOUND firstThis avoids an extra DB round-trip and ensures 404 beats 409 for invalid ids.
Proposed shape outside current hunk:
- Fetch
prevTeam- If not found, throw NOT_FOUND
- If
input.slugandinput.slug !== prevTeam.slug, then run repository check
📜 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 (4)
apps/api/v1/pages/api/teams/[teamId]/_patch.ts(2 hunks)apps/web/playwright/teams.e2e.ts(2 hunks)packages/lib/server/repository/team.ts(1 hunks)packages/trpc/server/routers/viewer/teams/update.handler.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/playwright/teams.e2e.ts
🧰 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/lib/server/repository/team.tsapps/api/v1/pages/api/teams/[teamId]/_patch.tspackages/trpc/server/routers/viewer/teams/update.handler.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/lib/server/repository/team.tsapps/api/v1/pages/api/teams/[teamId]/_patch.tspackages/trpc/server/routers/viewer/teams/update.handler.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/lib/server/repository/team.tsapps/api/v1/pages/api/teams/[teamId]/_patch.tspackages/trpc/server/routers/viewer/teams/update.handler.ts
🧠 Learnings (1)
📚 Learning: 2025-08-23T13:45:16.529Z
Learnt from: shaun-ak
PR: calcom/cal.com#23233
File: packages/features/users/components/UserTable/EditSheet/EditUserForm.tsx:100-101
Timestamp: 2025-08-23T13:45:16.529Z
Learning: In Cal.com's team structure, the organization team (root team) is identified by having no parentId (!team.parentId), while sub-teams within an organization have a parentId. Use selectedUser?.teams?.find((team) => !team.parentId) to get the organization team, not by matching org?.id.
Applied to files:
packages/trpc/server/routers/viewer/teams/update.handler.ts
🧬 Code graph analysis (2)
apps/api/v1/pages/api/teams/[teamId]/_patch.ts (1)
packages/lib/server/repository/team.ts (1)
TeamRepository(170-421)
packages/trpc/server/routers/viewer/teams/update.handler.ts (1)
packages/lib/server/repository/team.ts (1)
TeamRepository(170-421)
🪛 GitHub Actions: PR Update
packages/trpc/server/routers/viewer/teams/update.handler.ts
[error] 36-36: TypeScript error TS18004: No value exists in scope for the shorthand property 'id'. Either declare one or provide an initializer.
⏰ 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
anikdhabal
dismissed stale reviews from Udit-takkar, volnei, and emrysal
via
1ec282e
| const slugIndex = index ? `-count-${index}` : ""; | ||
| const slug = | ||
| orgRequestedSlug ?? `${isOrg ? "org" : "team"}-${workerInfo.workerIndex}-${Date.now()}${slugIndex}`; | ||
| teamSlug ?? |
There was a problem hiding this comment.
NIT: can we abstract this logic into its own function instead of just using the nullish coalescing operator? this could get messy really fast if in future we might need to update the logic and add some stuff in here
What does this PR do?
The changes scope team slug uniqueness to the team’s parent context by constructing a dynamic where clause using parentId in both the API route and TRPC update handler, which now throws a CONFLICT error on collisions. Playwright fixtures are extended to accept an optional teamSlug and add getAllTeamMembership. E2E tests are added to validate slug rules across organizations and non-organization teams, and a test helper import is updated. A data-testid is added to the team profile update button to support tests.