Conversation
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/02/25)1 reviewer was added to this PR based on Keith Williams's automation. |
|
✅ No security or compliance issues detected. Reviewed everything up to b270660. Security Overview
Detected Code Changes
Reply to this PR with |
There was a problem hiding this comment.
cubic found 9 issues across 22 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
| export const QUERY_KEY = "get-installed-conferencing-apps"; | ||
|
|
||
| export const useAtomsGetInstalledConferencingApps = (teamId?: number) => { | ||
| export const useAtomsGetInstalledConferencingApps = (teamId?: number, orgId?: number) => { |
There was a problem hiding this comment.
The function signature now allows both teamId and orgId, but the logic does not handle the case where both are provided. This could lead to ambiguous or unintended API calls. Consider validating that only one of teamId or orgId is provided at a time.
packages/app-store/server.ts
Outdated
| }, | ||
| }); | ||
|
|
||
| if (user?.organizationId) { |
There was a problem hiding this comment.
When the user belongs to an organization this branch replaces the previous { userId } filter with { teamId: { in: [user.organizationId] } }, causing personal credentials owned by the user to be ignored. As a result users who are part of an org will no longer see their own conferencing credentials in the location selector.
apps/api/v2/src/modules/atoms/controllers/atoms.conferencing-apps.controller.ts
Show resolved
Hide resolved
apps/api/v2/src/modules/atoms/controllers/atoms.conferencing-apps.controller.ts
Outdated
Show resolved
Hide resolved
| const fallbackUrl = decodedCallbackState.onErrorReturnTo || ""; | ||
| return { url: fallbackUrl }; | ||
| } | ||
| } else if (decodedCallbackState.orgId) { |
There was a problem hiding this comment.
This new branch repeats almost the same logic as the preceding team-level branch (building the same params / headers, performing the same axios call, and handling identical success & error flows). Copy-pasting this block increases maintenance cost and risks future inconsistencies. Extract the shared logic into a reusable helper instead of duplicating it.
packages/platform/atoms/connect/conferencing-apps/hooks/useGetDefaultConferencingApp.ts
Show resolved
Hide resolved
This reverts commit 82db763.
supalarry
left a comment
There was a problem hiding this comment.
couple small comments, besides that like discussed in Slack we need a how to guide for customers how to use this
|
|
||
| @IsNumber() | ||
| @IsOptional() | ||
| @Expose() |
There was a problem hiding this comment.
I think that the properties in DefaultConferencingAppsOutputDto won't show up in v2 docs because missing ApiProperty - we need to add it so customers know what data to expect.
| } | ||
|
|
||
| async findTeamConferencingApps(teamId: number) { | ||
| const parentTeam = await this.dbRead.prisma.team.findUnique({ |
There was a problem hiding this comment.
shouldn't this be just const team because we then check ?.parentId
| }); | ||
| } | ||
|
|
||
| async findMultipleTeamsConferencingApp(teamIds: number[], app: string) { |
There was a problem hiding this comment.
Why in findTeamConferencingApps we check parentId and here not?
packages/app-store/server.ts
Outdated
| if (!existingOption) { | ||
| apps[groupByCategory] = [...apps[groupByCategory], option]; | ||
| } | ||
| // const existingOption = apps[groupByCategory].find((o) => o.value === option.value); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/v2/src/modules/users/users.repository.ts (1)
309-318: Consider a more explicit null check.The condition
typeof metadata === "object"evaluates totruefornulldue to a JavaScript quirk. While spreadingnullresults in an empty object (matching the else branch behavior), a more explicit check would improve clarity.Apply this diff to make the intent clearer:
metadata: - typeof metadata === "object" + metadata && typeof metadata === "object" ? { ...metadata,
📜 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 (3)
apps/api/v2/src/modules/conferencing/outputs/get-default-conferencing-app.output.ts(2 hunks)apps/api/v2/src/modules/teams/teams/teams.repository.ts(2 hunks)apps/api/v2/src/modules/users/users.repository.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/v2/src/modules/conferencing/outputs/get-default-conferencing-app.output.ts
- apps/api/v2/src/modules/teams/teams/teams.repository.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{service,repository}.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
apps/api/v2/src/modules/users/users.repository.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/api/v2/src/modules/users/users.repository.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/v2/src/modules/users/users.repository.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/v2/src/modules/users/users.repository.ts
🧬 Code graph analysis (1)
apps/api/v2/src/modules/users/users.repository.ts (2)
packages/platform/libraries/index.ts (1)
userMetadata(73-73)packages/prisma/zod-utils.ts (1)
userMetadata(327-349)
🔇 Additional comments (2)
apps/api/v2/src/modules/users/users.repository.ts (2)
10-10: LGTM!The import of
userMetadatais necessary for type-safe metadata parsing and aligns with the PR's introduction ofcredentialIdsupport.
297-297: LGTM!The addition of the optional
credentialIdparameter correctly extends the method to support credential-specific default conferencing apps, aligning with the PR objectives.
What does this PR do?
This pull request introduces support for installing conferencing applications at the team/organization level.
Benefits
No individual setup required: Users within a team or organization no longer need to connect their personal accounts to Zoom or other conferencing apps.
Shared credentials: Once installed by a team admin, the conferencing app credentials are available for all sub-teams and users within the org.
Users can leverage the shared team/org-installed credentials when creating personal events, as well as sub-team events.
Reduced redundancy: Instead of every user configuring the same app, the admin installs it once and all members can use it seamlessly.