Devin/feature opt in system 1765208720 #25
Conversation
- Add enabled column to UserFeatures and TeamFeatures tables - Add Feature resource to PBAC registry with CRUD actions - Update FeaturesRepository with methods for enabled column - Create feature-management package with config, service, and tRPC router - Add user, team, and organization features settings pages - Add feature opt-in banner component and hook - Update settings navigation to include features pages - Add i18n strings for all new UI elements Co-Authored-By: [email protected] <[email protected]>
- Remove enabled column from UserFeatures and TeamFeatures schema - Delete migration that added the enabled column - Update FeaturesRepository to use row-existence pattern: - Row exists = feature enabled - Delete row to disable feature - Update FeatureManagementService to check row existence - Update tRPC router to use ctx.prisma instead of importing prisma directly Co-Authored-By: [email protected] <[email protected]>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughA comprehensive feature management system is introduced, enabling users, organizations, and teams to view and toggle features at their respective scopes. The implementation adds settings pages, view components for feature display, a new service layer with repository methods, opt-in banner functionality, TRPC endpoints, and PBAC permissions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a robust and user-friendly system for managing feature flags across the application. It provides distinct interfaces for individual users, organizations, and teams to control their access to various features. Additionally, it introduces a dynamic banner mechanism to promote new features, allowing users to easily opt-in, all backed by a scalable service layer and integrated with the application's permission system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
codeant-ai
bot
added
the
size:XXL
Refacto PR SummaryImplemented a comprehensive feature management system enabling users, teams, and organizations to control feature flags through dedicated settings pages. The system introduces opt-in banners for new features, row-existence semantics for feature state management, and PBAC-controlled feature administration with tRPC endpoints for all CRUD operations. Key Changes:
Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant U as User
participant UI as Settings UI
participant tRPC as tRPC Router
participant FMS as FeatureManagementService
participant FR as FeaturesRepository
participant DB as Database
U->>UI: Navigate to /settings/my-account/features
UI->>tRPC: listForUser()
tRPC->>FMS: listFeaturesForUser(userId)
FMS->>FR: getUserFeatures(userId)
FR->>DB: Query UserFeatures table
DB-->>FR: User feature rows
FR-->>FMS: Feature assignments
FMS-->>tRPC: Features with status
tRPC-->>UI: Feature list
UI-->>U: Display toggleable features
U->>UI: Toggle feature on/off
UI->>tRPC: setUserFeatureEnabled(slug, enabled)
tRPC->>FMS: setUserFeatureEnabled()
FMS->>FR: setUserFeatureEnabled()
FR->>DB: Upsert/Delete UserFeatures row
DB-->>FR: Success
FR-->>FMS: Success
FMS-->>tRPC: Success
tRPC-->>UI: Success
UI-->>U: Show success toast
Testing GuideClick to expand
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive feature opt-in system, including UI for users, teams, and organizations to manage feature flags, as well as a backend service and tRPC endpoints to support this functionality. It also adds a banner system for promoting new opt-in features.
My review has identified a critical security vulnerability where mutations to set feature flags for teams and organizations are missing permission checks. I've also pointed out a few areas for improvement regarding type safety, code duplication, and user experience. Addressing the security issue is paramount.
| setTeamFeatureEnabled: authedProcedure | ||
| .input( | ||
| z.object({ | ||
| teamId: z.number(), | ||
| featureSlug: z.string(), | ||
| enabled: z.boolean(), | ||
| }) | ||
| ) | ||
| .mutation(async ({ ctx, input }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
| await service.setTeamFeatureEnabled( | ||
| input.teamId, | ||
| input.featureSlug, | ||
| input.enabled, | ||
| `user:${ctx.user.id}` | ||
| ); | ||
| return { success: true }; | ||
| }), |
There was a problem hiding this comment.
This mutation is missing a permission check to ensure the user has the authority to modify feature flags for a team. The comment on line 76 indicates that this is required, but the implementation is missing. This could allow any authenticated user to enable or disable features for any team, which is a significant security risk.
You should add a permission check using getResourcePermissions from @calcom/features/pbac/lib/resource-permissions to verify that the user has the feature.update permission for the given team. A similar check should also be added to the setOrganizationFeatureEnabled mutation.
Here's an example of how you could implement this, similar to what's done in other parts of the codebase:
import { getResourcePermissions } from "@calcom/features/pbac/lib/resource-permissions";
import { Resource } from "@calcom/features/pbac/domain/types/permission-registry";
import { TRPCError } from "@trpc/server";
// Inside the mutation:
const { canUpdate } = await getResourcePermissions({
userId: ctx.user.id,
teamId: input.teamId,
resource: Resource.Feature,
userRole: membership.role, // You'll need to fetch the user's membership role for the team.
});
if (!canUpdate) {
throw new TRPCError({ code: "UNAUTHORIZED" });
}| setOrganizationFeatureEnabled: authedProcedure | ||
| .input( | ||
| z.object({ | ||
| organizationId: z.number(), | ||
| featureSlug: z.string(), | ||
| enabled: z.boolean(), | ||
| }) | ||
| ) | ||
| .mutation(async ({ ctx, input }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
| await service.setOrganizationFeatureEnabled( | ||
| input.organizationId, | ||
| input.featureSlug, | ||
| input.enabled, | ||
| `user:${ctx.user.id}` | ||
| ); | ||
| return { success: true }; | ||
| }), |
There was a problem hiding this comment.
Similar to setTeamFeatureEnabled, this mutation lacks a crucial permission check. Any authenticated user could potentially change feature flags for any organization. This is a critical security vulnerability.
Please implement a permission check to ensure the user has the feature.update permission for the specified organization before proceeding with the mutation.
|
|
||
| const setFeatureEnabledMutation = trpc.viewer.featureManagement.setTeamFeatureEnabled.useMutation({ | ||
| onSuccess: () => { | ||
| utils.viewer.featureManagement.listForTeam.invalidate({ teamId: teamId! }); |
There was a problem hiding this comment.
Using the non-null assertion operator (!) on teamId can be risky. While the mutation is likely only called when teamId is available, it's safer to add a guard to prevent potential runtime errors if teamId were to become null.
| utils.viewer.featureManagement.listForTeam.invalidate({ teamId: teamId! }); | |
| if (teamId) utils.viewer.featureManagement.listForTeam.invalidate({ teamId }); |
| <SettingsToggle | ||
| key={feature.slug} | ||
| toggleSwitchAtTheEnd={true} | ||
| title={feature.slug} |
There was a problem hiding this comment.
Using the feature slug directly as the title in the UI isn't very user-friendly, as slugs are often designed for programmatic use. This is also done in organization-features-view.tsx and team-features-view.tsx.
Consider adding a name or title field to the Feature model in the database to store a human-readable name for each feature. This would significantly improve the user experience on these new settings pages.
| if (typeof window !== "undefined") { | ||
| const stored = localStorage.getItem("cal_feature_banners_dismissed"); | ||
| setDismissedFeatures(stored ? JSON.parse(stored) : []); | ||
| } |
There was a problem hiding this comment.
The localStorage key "cal_feature_banners_dismissed" is hardcoded here, and the logic to parse it is duplicated. The FeatureOptInBanner.tsx component already exports a DISMISSED_BANNERS_KEY constant and a getDismissedBanners helper function.
To improve maintainability and reduce code duplication, you should use the exported helper function. You'll also need to update the imports to include getDismissedBanners.
setDismissedFeatures(getDismissedBanners());| getUserFeatures(userId: number): Promise<unknown[]>; | ||
| getTeamFeaturesWithDetails(teamId: number): Promise<unknown[]>; |
There was a problem hiding this comment.
The return types for getUserFeatures and getTeamFeaturesWithDetails are Promise<unknown[]>. This forces consumers of the repository, like FeatureManagementService, to use type assertions or any, which undermines type safety.
The implementation in features.repository.ts returns a more specific, typed result from Prisma. Please update the interface to reflect the actual return types to improve type safety and code clarity across the feature.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI Agents
In @packages/features/feature-management/components/FeatureOptInBanner.tsx:
- Around line 48-53: Add error handling to the opt-in mutation by supplying an
onError handler to trpc.viewer.featureManagement.optInToFeature.useMutation that
calls showToast with a user-friendly error message and the actual error, so
failures surface to the user; keep the existing onSuccess logic
(utils.viewer.featureManagement.getEligibleOptInFeatures.invalidate() and
onOptIn?.()) and ensure you import showToast at the top of
FeatureOptInBanner.tsx before using it.
In @packages/features/feature-management/hooks/useFeatureOptInBanner.ts:
- Around line 42-47: The effect in useFeatureOptInBanner reads
"cal_feature_banners_dismissed" and calls JSON.parse directly which can throw on
corrupt data; update the useEffect in useFeatureOptInBanner to wrap
JSON.parse(...) in a try/catch: attempt to parse the stored value, on success
call setDismissedFeatures(parsed), on failure fall back to an empty array (and
optionally remove the corrupt localStorage key via
localStorage.removeItem("cal_feature_banners_dismissed") or overwrite it with
"[]") so the component won't crash when JSON.parse throws.
In @packages/features/feature-management/trpc/router.ts:
- Around line 41-50: The listForOrganization procedure lacks an authorization
check: before calling
getFeatureManagementService(...).listFeaturesForOrganization(input.organizationId)
validate that ctx.user (or via ctx.prisma) is a member of the organization
identified by input.organizationId (same pattern as the team endpoint); if the
membership check fails, throw a TRPCError with code 'FORBIDDEN' and only proceed
to call service.listFeaturesForOrganization when membership is confirmed.
- Around line 97-118: The mutation setOrganizationFeatureEnabled currently lets
any authenticated user change org features; add an explicit PBAC/authorization
check (the same pattern used for the team mutation) before calling
getFeatureManagementService and service.setOrganizationFeatureEnabled: validate
ctx.user has the required permission (e.g., organization:features:update or an
"org admin" role) for input.organizationId using your existing PBAC helper (the
same helper used in the team mutation) and throw an authorization error if it
fails, then proceed to call setOrganizationFeatureEnabled with the actor
`user:${ctx.user.id}`.
- Around line 27-36: The listForTeam authedProcedure currently allows any
authenticated user to request features for any team; modify listForTeam to
verify the caller is a member of the requested team before calling
getFeatureManagementService(...).listFeaturesForTeam: fetch the current user id
from ctx.session (or ctx.userId), query your membership table (e.g., via
ctx.prisma.teamMember or an existing service method like
teamService.isMember/teamMembershipExists) for input.teamId and the user id, and
throw a 403 / TRPCError if not a member; only call
getFeatureManagementService(ctx.prisma).listFeaturesForTeam(input.teamId) after
the membership check succeeds.
🧹 Nitpick comments (14)
packages/trpc/server/routers/viewer/_router.tsx (1)
2-2: featureManagement router is correctly wired into viewerRouterImporting
featureManagementRouterand exposing it asfeatureManagementonviewerRoutermatches existing router patterns and will surface the new feature‑management endpoints under the viewer namespace as intended.If you expect
featuresandfeatureManagementto coexist long‑term, consider documenting or aligning their naming responsibilities to avoid confusion between instance‑level flags vs. per‑user/org feature management, but this is non‑blocking.Also applies to: 79-79
apps/web/modules/settings/my-account/features-view.tsx (1)
10-20: Consider extracting shared SkeletonLoader component.The SkeletonLoader is duplicated in both
features-view.tsxandteam-features-view.tsxwith identical implementation. Consider extracting this to a shared component to reduce duplication.📦 Suggested shared component location
Create a shared skeleton component:
// apps/web/components/settings/FeaturesSkeletonLoader.tsx import { SkeletonContainer, SkeletonText } from "@calcom/ui/components/skeleton"; export const FeaturesSkeletonLoader = () => { return ( <SkeletonContainer> <div className="border-subtle space-y-6 border-x px-4 py-8 sm:px-6"> <SkeletonText className="h-8 w-full" /> <SkeletonText className="h-8 w-full" /> <SkeletonText className="h-8 w-full" /> </div> </SkeletonContainer> ); };Then import and use in both feature views.
apps/web/modules/settings/teams/team-features-view.tsx (2)
24-47: Consider improving UX for invalid team ID.When
teamIdis null or invalid (line 45-47), the component silently returns null, rendering nothing. Consider showing an error message or redirecting to a valid route for better user experience.🔎 Suggested improvement
if (!teamId) { - return null; + return ( + <SettingsHeader + title={t("features")} + description={t("team_features_description")} + borderInShellHeader={true}> + <div className="border-subtle border-x px-4 py-8 sm:px-6"> + <p className="text-error text-sm">{t("invalid_team_id")}</p> + </div> + </SettingsHeader> + ); }
12-22: Significant code duplication with features-view.tsx.The TeamFeaturesView component shares substantial code with the user-level FeaturesView:
- Identical SkeletonLoader component (lines 12-22)
- Similar query/mutation patterns
- Nearly identical rendering logic
While the current implementation works correctly, consider refactoring to reduce duplication. This could involve:
- Extracting a shared base component with scope-specific props
- Creating shared utility hooks for feature management
- Extracting common UI patterns into reusable components
Also applies to: 60-94
apps/web/modules/settings/organizations/organization-features-view.tsx (2)
14-24: Consider reusing existing skeleton patterns.The skeleton loader is well-structured. Consider if there's an existing skeleton pattern in the codebase that could be reused for consistency across settings pages.
55-55: Consider handlingfeaturesbeingundefinedmore explicitly.While the nullish coalescing handles the case when
featuresis undefined, consider if an error state should be shown when the query fails (as opposed to treating it like an empty list).apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsx (1)
26-39:canUpdatepermission is defined but unused.The
fallbackRoles.updateis specified but onlycanReadis destructured and used. If the view component allows toggling features, consider passingcanUpdateto conditionally enable/disable the toggle controls.🔎 Proposed fix
- const { canRead } = await getResourcePermissions({ + const { canRead, canUpdate } = await getResourcePermissions({ userId: session.user.id, teamId: session.user.profile.organizationId, resource: Resource.Feature, userRole: session.user.org.role, fallbackRoles: { read: { roles: [MembershipRole.MEMBER, MembershipRole.ADMIN, MembershipRole.OWNER], }, update: { roles: [MembershipRole.ADMIN, MembershipRole.OWNER], }, }, });Then pass
canUpdatetoOrganizationFeaturesViewto control whether toggles are editable:return <OrganizationFeaturesView organizationId={session.user.profile.organizationId} canUpdate={canUpdate} />;packages/features/feature-management/components/FeatureOptInBanner.tsx (1)
11-19: Consider using the existing localStorage wrapper.Based on the relevant code snippets, there's a
localStoragewrapper inpackages/lib/webstorage.tsthat handles try/catch internally. Consider using it for consistency.🔎 Proposed fix
+import { localStorage } from "@calcom/lib/webstorage"; + function getDismissedBanners(): string[] { if (typeof window === "undefined") return []; - try { - const stored = localStorage.getItem(DISMISSED_BANNERS_KEY); - return stored ? JSON.parse(stored) : []; - } catch { - return []; - } + const stored = localStorage.getItem(DISMISSED_BANNERS_KEY); + if (!stored) return []; + try { + return JSON.parse(stored); + } catch { + return []; + } }packages/features/feature-management/trpc/router.ts (1)
155-157: Consider using TRPCError for consistent error handling.Using a raw
Errorwill result in an INTERNAL_SERVER_ERROR. Consider usingTRPCErrorwith an appropriate code for better client-side error handling.🔎 Proposed fix
+import { TRPCError } from "@trpc/server"; + if (!service.isFeatureInOptInAllowlist(input.featureSlug)) { - throw new Error("Feature is not available for opt-in"); + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Feature is not available for opt-in", + }); }packages/features/flags/features.repository.ts (2)
274-298:enableFeatureForTeamis now nearly identical tosetTeamFeatureEnabled(teamId, featureId, true, assignedBy).Consider deprecating or removing
enableFeatureForTeamin favor of the more generalsetTeamFeatureEnabledto reduce duplication and API surface area.
408-430: Consider adding error handling for consistency with other methods.
getUserFeatureandgetTeamFeaturelack try-catch blocks unlike other repository methods. While Prisma errors will propagate, explicit handling withcaptureExceptionmaintains consistency and observability.🔎 Proposed fix for getUserFeature
async getUserFeature(userId: number, featureId: string) { + try { return this.prismaClient.userFeatures.findFirst({ where: { userId, featureId, }, }); + } catch (err) { + captureException(err); + throw err; + } }packages/features/flags/features.repository.interface.ts (1)
11-14: Return types ofunknownreduce type safety.The implementation returns specific Prisma model types (
UserFeatures,TeamFeatures), but the interface usesunknown. This forces consumers to cast or use type guards. Consider defining proper return types or using generics.🔎 Suggested improvement
- getUserFeature(userId: number, featureId: string): Promise<unknown>; - getTeamFeature(teamId: number, featureId: string): Promise<unknown>; - getUserFeatures(userId: number): Promise<unknown[]>; - getTeamFeaturesWithDetails(teamId: number): Promise<unknown[]>; + getUserFeature(userId: number, featureId: string): Promise<{ userId: number; featureId: string; assignedBy: string } | null>; + getTeamFeature(teamId: number, featureId: string): Promise<{ teamId: number; featureId: string; assignedBy: string } | null>; + getUserFeatures(userId: number): Promise<Array<{ userId: number; featureId: string; feature: { slug: string; enabled: boolean; description: string | null; type: string } }>>; + getTeamFeaturesWithDetails(teamId: number): Promise<Array<{ teamId: number; featureId: string; feature: { slug: string; enabled: boolean; description: string | null; type: string } }>>;Alternatively, import and reuse Prisma-generated types or define shared DTOs.
packages/features/feature-management/services/FeatureManagementService.ts (2)
122-151: N+1 query pattern ingetEligibleOptInFeaturesloop.For each opt-in slug,
getUserFeatureandcheckIfFeatureIsEnabledGloballyare called sequentially. With many opt-in features, this could degrade performance. Consider batch-fetching user features and the global feature map upfront.🔎 Proposed optimization
async getEligibleOptInFeatures(userId: number): Promise<EligibleOptInFeature[]> { const eligibleFeatures: EligibleOptInFeature[] = []; const optInSlugs = getOptInFeatureSlugs(); + + // Batch fetch: user features and global feature map + const userFeatures = await this.featuresRepository.getUserFeatures(userId); + const userFeatureSlugs = new Set(userFeatures.map((uf) => uf.feature.slug)); + const globalFlags = await this.featuresRepository.getFeatureFlagMap(); for (const slug of optInSlugs) { const config = getOptInFeatureConfig(slug); if (!config) continue; - const userFeature = await this.featuresRepository.getUserFeature(userId, slug); - - // Row exists = user has already opted in - if (userFeature) { + // Row exists = user has already opted in + if (userFeatureSlugs.has(slug)) { continue; } - const isGloballyEnabled = await this.featuresRepository.checkIfFeatureIsEnabledGlobally( - slug as Parameters<typeof this.featuresRepository.checkIfFeatureIsEnabledGlobally>[0] - ); - if (!isGloballyEnabled) continue; + const isGloballyEnabled = globalFlags[slug as keyof typeof globalFlags]; + if (!isGloballyEnabled) continue; eligibleFeatures.push({ slug: config.slug, titleI18nKey: config.titleI18nKey, descriptionI18nKey: config.descriptionI18nKey, learnMoreUrl: config.learnMoreUrl, }); } return eligibleFeatures; }
137-139: Type assertion on line 138 is awkward.The cast
slug as Parameters<typeof this.featuresRepository.checkIfFeatureIsEnabledGlobally>[0]is verbose. If opt-in slugs should be validAppFlagskeys, consider typinggetOptInFeatureSlugs()to return(keyof AppFlags)[]instead.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/features/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/features/page.tsxapps/web/modules/settings/my-account/features-view.tsxapps/web/modules/settings/organizations/organization-features-view.tsxapps/web/modules/settings/teams/team-features-view.tsxapps/web/public/static/locales/en/common.jsonpackages/features/feature-management/components/FeatureOptInBanner.tsxpackages/features/feature-management/config/feature-management.config.tspackages/features/feature-management/hooks/useFeatureOptInBanner.tspackages/features/feature-management/index.tspackages/features/feature-management/services/FeatureManagementService.tspackages/features/feature-management/trpc/router.tspackages/features/flags/features.repository.interface.tspackages/features/flags/features.repository.tspackages/features/pbac/domain/types/permission-registry.tspackages/trpc/server/routers/viewer/_router.tsx
🧰 Additional context used
🧬 Code graph analysis (10)
packages/trpc/server/routers/viewer/_router.tsx (2)
packages/features/feature-management/trpc/router.ts (1)
featureManagementRouter(15-162)packages/features/feature-management/index.ts (1)
featureManagementRouter(1-1)
apps/web/modules/settings/teams/team-features-view.tsx (1)
packages/trpc/react/trpc.ts (1)
trpc(50-134)
packages/features/feature-management/components/FeatureOptInBanner.tsx (3)
packages/features/feature-management/index.ts (7)
DISMISSED_BANNERS_KEY(16-16)getDismissedBanners(13-13)dismissBanner(14-14)isBannerDismissed(15-15)FeatureOptInBannerProps(18-18)EligibleOptInFeature(3-3)FeatureOptInBanner(12-12)packages/lib/webstorage.ts (1)
localStorage(6-36)packages/features/feature-management/services/FeatureManagementService.ts (1)
EligibleOptInFeature(13-18)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsx (3)
apps/web/app/_utils.tsx (2)
_generateMetadata(53-81)getTranslate(15-19)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/actions/validateUserHasOrg.tsx (1)
validateUserHasOrg(19-30)packages/platform/libraries/index.ts (1)
MembershipRole(32-32)
packages/features/feature-management/trpc/router.ts (2)
packages/features/flags/features.repository.ts (1)
FeaturesRepository(18-558)packages/features/feature-management/services/FeatureManagementService.ts (1)
FeatureManagementService(25-175)
packages/features/flags/features.repository.interface.ts (1)
packages/features/flags/config.ts (1)
AppFlags(5-37)
apps/web/modules/settings/organizations/organization-features-view.tsx (1)
packages/trpc/react/trpc.ts (1)
trpc(50-134)
packages/features/flags/features.repository.ts (1)
packages/features/flags/config.ts (1)
AppFlags(5-37)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/features/page.tsx (3)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/features/page.tsx (1)
generateMetadata(11-18)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsx (1)
generateMetadata(12-19)apps/web/app/_utils.tsx (1)
_generateMetadata(53-81)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/features/page.tsx (2)
apps/web/app/_utils.tsx (1)
_generateMetadata(53-81)apps/web/lib/buildLegacyCtx.ts (1)
buildLegacyRequest(47-49)
🔇 Additional comments (32)
apps/web/public/static/locales/en/common.json (1)
1256-1269: New feature-management & PBAC copy looks consistentThe added strings are clear, follow existing naming patterns (
features_*,pbac_resource_*,pbac_desc_*), and fit the feature-management/opt‑in flows. No changes needed from a copy/consistency standpoint.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (3)
47-47: LGTM: User account features navigation entry added correctly.The new features navigation entry follows the established pattern with proper href and tracking metadata.
95-99: LGTM: Organization features navigation entry added correctly.The organization-level features entry is properly structured and consistently placed among other organization settings.
540-547: LGTM: Team features navigation entry properly gated.The team features entry is correctly placed within the admin/owner authorization check (lines 499-502), ensuring only authorized users can access team feature management.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/features/page.tsx (2)
11-18: LGTM: Metadata generation follows established pattern.The generateMetadata implementation correctly uses the shared
_generateMetadatautility with appropriate localization keys and pathname.
20-30: LGTM: Server-side authentication and rendering implemented correctly.The page properly:
- Awaits async Next.js 15 APIs (headers, cookies) per the framework requirements
- Checks for authenticated user before rendering
- Redirects unauthenticated users with appropriate callback URL
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/features/page.tsx (1)
5-12: LGTM: Async params handled correctly for Next.js 15.The generateMetadata correctly awaits the params Promise before accessing the id property, which is required in Next.js 15.
apps/web/modules/settings/my-account/features-view.tsx (1)
22-79: LGTM: Feature management UI implemented correctly.The component properly:
- Fetches features via TRPC with loading states
- Filters for user-controllable features (globallyEnabled)
- Implements mutations with cache invalidation and toast notifications
- Disables UI during pending mutations for better UX
- Handles empty states with localized messages
apps/web/modules/settings/teams/team-features-view.tsx (1)
30-43: LGTM: TRPC query and mutation with proper safeguards.The non-null assertions on
teamId!(lines 31, 37) are safe because:
- Line 32: Query is disabled when teamId is falsy
- Line 45-47: Component returns early if teamId is null
- Mutations can only be triggered after successful query (teamId must be valid)
The cache invalidation and toast notifications follow the same correct pattern as the user features view.
apps/web/modules/settings/organizations/organization-features-view.tsx (2)
26-42: LGTM!Good implementation with proper cache invalidation on mutation success and appropriate error handling via toast notifications. The use of
isPendingto disable toggles during mutation prevents double-submissions.
67-82: LGTM!The feature toggle implementation correctly passes all required props and handles the checked state change appropriately. Using
feature.slugas key is correct since slugs should be unique.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsx (1)
41-52: LGTM!Good implementation of the no-permission fallback UI that maintains consistent styling with the rest of the page.
packages/features/feature-management/config/feature-management.config.ts (2)
1-48: LGTM!Clean and well-documented configuration module. The helper functions provide a good abstraction for checking allowlist membership and retrieving feature configs.
24-24: The documentation URL is valid and accessible (HTTP 200 response).packages/features/feature-management/components/FeatureOptInBanner.tsx (1)
64-93: LGTM!Good implementation of the banner UI with proper accessibility attributes on the external link (
target="_blank",rel="noopener noreferrer"). The conditional rendering of the "Learn more" link is appropriate.packages/features/feature-management/trpc/router.ts (2)
10-13: LGTM!Good factory pattern for creating the service with the request's Prisma client.
146-161: LGTM!Good implementation with proper allowlist validation before enabling the feature. This prevents users from opting into features that aren't in the allowlist.
packages/features/pbac/domain/types/permission-registry.ts (2)
16-16: LGTM!New
Featureresource enum value follows the existing naming convention.
754-787: Verify if Update scope restriction for Feature is intentional.Update action lacks
scope: [Scope.Organization]while Create and Delete have it. This allows team-level users to update features despite being unable to create or delete them. No documentation explains this asymmetry in the codebase.packages/features/feature-management/index.ts (1)
1-20: LGTM!Well-organized barrel file that consolidates the public API surface. Good use of separate
export typestatements for type-only exports, which helps with tree-shaking and avoids potential issues with circular dependencies.packages/features/feature-management/hooks/useFeatureOptInBanner.ts (3)
49-62: SettingfeatureToShowtonullwhenfeatureParamis absent may cause brief flicker.When
eligibleFeaturesis still loading (undefined), the effect setsfeatureToShowtonulleven if a valid feature param exists. This is acceptable sinceisLoadingis exposed, but consider guarding withisLoadingto avoid the intermediate null state if the consuming component doesn't handle loading properly.
64-70:dismissCurrentFeaturecapturesfeatureToShowcorrectly.The callback depends on
featureToShowand properly updates both localStorage (viadismissBanner) and local state. Implementation is correct.
72-81: LGTM!The
onOptInSuccesscallback correctly clears the shown feature without affecting persisted dismissals. Return structure is clean.packages/features/flags/features.repository.ts (4)
310-349: Implementation is correct for row-existence semantics.The upsert/deleteMany pattern properly handles enable/disable.
deleteManyis appropriate since it's idempotent (no error if row doesn't exist). Cache invalidation is correctly placed.
361-400: LGTM!
setTeamFeatureEnabledmirrors the user version correctly with proper error handling and cache invalidation.
441-493: Early return optimization for direct feature check is good.The early
findUniquecheck before the recursive CTE avoids unnecessary query complexity for the common case. The CTE logic correctly handles hierarchy traversal.
522-557: LGTM!
getUserFeaturesandgetTeamFeaturesWithDetailscorrectly include feature metadata via Prisma relations. These support the service layer's listing functionality.packages/features/flags/features.repository.interface.ts (1)
3-15: LGTM on the method signatures and contract additions.The new methods properly extend the interface to support user/team feature CRUD operations needed by the service layer.
packages/features/feature-management/services/FeatureManagementService.ts (4)
5-11: Interface definitions are well-structured.
FeatureWithStatusandEligibleOptInFeatureprovide clear contracts for the service's return types.
32-47: LGTM!The mapping logic correctly uses row-existence semantics. Fetching all features and user features in two queries, then mapping in-memory, is an efficient approach.
74-76: Clean delegation for organization features.Correctly treats organizations as teams, avoiding code duplication.
156-174: LGTM!Utility methods cleanly wrap config functions and the
hasUserOptedIncheck follows the established row-existence pattern.
| const Page = async () => { | ||
| return <TeamFeaturesView />; | ||
| }; |
There was a problem hiding this comment.
Add server-side authentication and authorization checks.
Unlike the user-level features page (my-account/features/page.tsx lines 20-27), this team features page lacks authentication and authorization validation. Server components should verify:
- User is authenticated (session exists)
- User has permission to view/manage features for this specific team
Relying solely on client-side checks or route middleware may not be sufficient for sensitive settings pages.
🔎 Recommended server-side auth pattern
Based on the my-account features page implementation:
+import { headers, cookies } from "next/headers";
+import { redirect } from "next/navigation";
+
+import { getServerSession } from "@calcom/features/auth/lib/getServerSession";
+
+import { buildLegacyRequest } from "@lib/buildLegacyCtx";
+
import TeamFeaturesView from "~/settings/teams/team-features-view";
export const generateMetadata = async ({ params }: { params: Promise<{ id: string }> }) =>
await _generateMetadata(
(t) => t("features"),
(t) => t("team_features_description"),
undefined,
undefined,
`/settings/teams/${(await params).id}/features`
);
-const Page = async () => {
+const Page = async ({ params }: { params: Promise<{ id: string }> }) => {
+ const { id: teamId } = await params;
+ const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) });
+
+ if (!session?.user?.id) {
+ return redirect(`/auth/login?callbackUrl=/settings/teams/${teamId}/features`);
+ }
+
+ // TODO: Add team membership/permission check here
+ // e.g., verify session.user has access to this teamId
+
return <TeamFeaturesView />;
};| const optInMutation = trpc.viewer.featureManagement.optInToFeature.useMutation({ | ||
| onSuccess: () => { | ||
| utils.viewer.featureManagement.getEligibleOptInFeatures.invalidate(); | ||
| onOptIn?.(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Missing error handling for opt-in mutation.
The mutation has onSuccess but no onError handler. Consider showing a toast notification on failure for better UX, similar to OrganizationFeaturesView.
🔎 Proposed fix
const optInMutation = trpc.viewer.featureManagement.optInToFeature.useMutation({
onSuccess: () => {
utils.viewer.featureManagement.getEligibleOptInFeatures.invalidate();
onOptIn?.();
},
+ onError: () => {
+ showToast(t("error_opting_in_to_feature"), "error");
+ },
});You'll also need to import showToast:
+import { showToast } from "@calcom/ui/components/toast";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const optInMutation = trpc.viewer.featureManagement.optInToFeature.useMutation({ | |
| onSuccess: () => { | |
| utils.viewer.featureManagement.getEligibleOptInFeatures.invalidate(); | |
| onOptIn?.(); | |
| }, | |
| }); | |
| import { showToast } from "@calcom/ui/components/toast"; | |
| const optInMutation = trpc.viewer.featureManagement.optInToFeature.useMutation({ | |
| onSuccess: () => { | |
| utils.viewer.featureManagement.getEligibleOptInFeatures.invalidate(); | |
| onOptIn?.(); | |
| }, | |
| onError: () => { | |
| showToast(t("error_opting_in_to_feature"), "error"); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In @packages/features/feature-management/components/FeatureOptInBanner.tsx
around lines 48 - 53, Add error handling to the opt-in mutation by supplying an
onError handler to trpc.viewer.featureManagement.optInToFeature.useMutation that
calls showToast with a user-friendly error message and the actual error, so
failures surface to the user; keep the existing onSuccess logic
(utils.viewer.featureManagement.getEligibleOptInFeatures.invalidate() and
onOptIn?.()) and ensure you import showToast at the top of
FeatureOptInBanner.tsx before using it.
| useEffect(() => { | ||
| if (typeof window !== "undefined") { | ||
| const stored = localStorage.getItem("cal_feature_banners_dismissed"); | ||
| setDismissedFeatures(stored ? JSON.parse(stored) : []); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Wrap JSON.parse in try-catch to handle corrupt localStorage data.
If localStorage contains invalid JSON (e.g., manually corrupted or from a different app version), JSON.parse will throw and crash the component. Defensive parsing ensures resilience.
🔎 Proposed fix
useEffect(() => {
if (typeof window !== "undefined") {
const stored = localStorage.getItem("cal_feature_banners_dismissed");
- setDismissedFeatures(stored ? JSON.parse(stored) : []);
+ if (stored) {
+ try {
+ const parsed = JSON.parse(stored);
+ setDismissedFeatures(Array.isArray(parsed) ? parsed : []);
+ } catch {
+ setDismissedFeatures([]);
+ }
+ }
}
}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (typeof window !== "undefined") { | |
| const stored = localStorage.getItem("cal_feature_banners_dismissed"); | |
| setDismissedFeatures(stored ? JSON.parse(stored) : []); | |
| } | |
| }, []); | |
| useEffect(() => { | |
| if (typeof window !== "undefined") { | |
| const stored = localStorage.getItem("cal_feature_banners_dismissed"); | |
| if (stored) { | |
| try { | |
| const parsed = JSON.parse(stored); | |
| setDismissedFeatures(Array.isArray(parsed) ? parsed : []); | |
| } catch { | |
| setDismissedFeatures([]); | |
| } | |
| } | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
In @packages/features/feature-management/hooks/useFeatureOptInBanner.ts around
lines 42 - 47, The effect in useFeatureOptInBanner reads
"cal_feature_banners_dismissed" and calls JSON.parse directly which can throw on
corrupt data; update the useEffect in useFeatureOptInBanner to wrap
JSON.parse(...) in a try/catch: attempt to parse the stored value, on success
call setDismissedFeatures(parsed), on failure fall back to an empty array (and
optionally remove the corrupt localStorage key via
localStorage.removeItem("cal_feature_banners_dismissed") or overwrite it with
"[]") so the component won't crash when JSON.parse throws.
| listForTeam: authedProcedure | ||
| .input( | ||
| z.object({ | ||
| teamId: z.number(), | ||
| }) | ||
| ) | ||
| .query(async ({ ctx, input }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
| return service.listFeaturesForTeam(input.teamId); | ||
| }), |
There was a problem hiding this comment.
Missing authorization check for team access.
Any authenticated user can list features for any team by passing an arbitrary teamId. Consider verifying that the user is a member of the team before returning data.
🔎 Proposed fix
listForTeam: authedProcedure
.input(
z.object({
teamId: z.number(),
})
)
.query(async ({ ctx, input }) => {
+ // Verify user has access to this team
+ const membership = await ctx.prisma.membership.findFirst({
+ where: {
+ teamId: input.teamId,
+ userId: ctx.user.id,
+ },
+ });
+ if (!membership) {
+ throw new TRPCError({ code: "FORBIDDEN", message: "You do not have access to this team" });
+ }
const service = getFeatureManagementService(ctx.prisma);
return service.listFeaturesForTeam(input.teamId);
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| listForTeam: authedProcedure | |
| .input( | |
| z.object({ | |
| teamId: z.number(), | |
| }) | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForTeam(input.teamId); | |
| }), | |
| listForTeam: authedProcedure | |
| .input( | |
| z.object({ | |
| teamId: z.number(), | |
| }) | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| // Verify user has access to this team | |
| const membership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| teamId: input.teamId, | |
| userId: ctx.user.id, | |
| }, | |
| }); | |
| if (!membership) { | |
| throw new TRPCError({ code: "FORBIDDEN", message: "You do not have access to this team" }); | |
| } | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForTeam(input.teamId); | |
| }), |
🤖 Prompt for AI Agents
In @packages/features/feature-management/trpc/router.ts around lines 27 - 36,
The listForTeam authedProcedure currently allows any authenticated user to
request features for any team; modify listForTeam to verify the caller is a
member of the requested team before calling
getFeatureManagementService(...).listFeaturesForTeam: fetch the current user id
from ctx.session (or ctx.userId), query your membership table (e.g., via
ctx.prisma.teamMember or an existing service method like
teamService.isMember/teamMembershipExists) for input.teamId and the user id, and
throw a 403 / TRPCError if not a member; only call
getFeatureManagementService(ctx.prisma).listFeaturesForTeam(input.teamId) after
the membership check succeeds.
| listForOrganization: authedProcedure | ||
| .input( | ||
| z.object({ | ||
| organizationId: z.number(), | ||
| }) | ||
| ) | ||
| .query(async ({ ctx, input }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
| return service.listFeaturesForOrganization(input.organizationId); | ||
| }), |
There was a problem hiding this comment.
Missing authorization check for organization access.
Similar to the team endpoint, any authenticated user can list features for any organization. Verify user membership.
🤖 Prompt for AI Agents
In @packages/features/feature-management/trpc/router.ts around lines 41 - 50,
The listForOrganization procedure lacks an authorization check: before calling
getFeatureManagementService(...).listFeaturesForOrganization(input.organizationId)
validate that ctx.user (or via ctx.prisma) is a member of the organization
identified by input.organizationId (same pattern as the team endpoint); if the
membership check fails, throw a TRPCError with code 'FORBIDDEN' and only proceed
to call service.listFeaturesForOrganization when membership is confirmed.
| /** | ||
| * Set the enabled status of a feature for a team. | ||
| * Requires appropriate PBAC permissions. | ||
| */ | ||
| setTeamFeatureEnabled: authedProcedure | ||
| .input( | ||
| z.object({ | ||
| teamId: z.number(), | ||
| featureSlug: z.string(), | ||
| enabled: z.boolean(), | ||
| }) | ||
| ) | ||
| .mutation(async ({ ctx, input }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
| await service.setTeamFeatureEnabled( | ||
| input.teamId, | ||
| input.featureSlug, | ||
| input.enabled, | ||
| `user:${ctx.user.id}` | ||
| ); | ||
| return { success: true }; | ||
| }), |
There was a problem hiding this comment.
Critical: Missing authorization for team feature mutations.
The comment states "Requires appropriate PBAC permissions" but no authorization check is implemented. Any authenticated user can enable/disable features for any team, which is a significant security vulnerability.
🔎 Proposed fix
setTeamFeatureEnabled: authedProcedure
.input(
z.object({
teamId: z.number(),
featureSlug: z.string(),
enabled: z.boolean(),
})
)
.mutation(async ({ ctx, input }) => {
+ // Verify user has admin/owner access to this team
+ const membership = await ctx.prisma.membership.findFirst({
+ where: {
+ teamId: input.teamId,
+ userId: ctx.user.id,
+ role: { in: ["ADMIN", "OWNER"] },
+ },
+ });
+ if (!membership) {
+ throw new TRPCError({ code: "FORBIDDEN", message: "You do not have permission to modify team features" });
+ }
const service = getFeatureManagementService(ctx.prisma);
await service.setTeamFeatureEnabled(
input.teamId,
input.featureSlug,
input.enabled,
`user:${ctx.user.id}`
);
return { success: true };
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Set the enabled status of a feature for a team. | |
| * Requires appropriate PBAC permissions. | |
| */ | |
| setTeamFeatureEnabled: authedProcedure | |
| .input( | |
| z.object({ | |
| teamId: z.number(), | |
| featureSlug: z.string(), | |
| enabled: z.boolean(), | |
| }) | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| const service = getFeatureManagementService(ctx.prisma); | |
| await service.setTeamFeatureEnabled( | |
| input.teamId, | |
| input.featureSlug, | |
| input.enabled, | |
| `user:${ctx.user.id}` | |
| ); | |
| return { success: true }; | |
| }), | |
| /** | |
| * Set the enabled status of a feature for a team. | |
| * Requires appropriate PBAC permissions. | |
| */ | |
| setTeamFeatureEnabled: authedProcedure | |
| .input( | |
| z.object({ | |
| teamId: z.number(), | |
| featureSlug: z.string(), | |
| enabled: z.boolean(), | |
| }) | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| // Verify user has admin/owner access to this team | |
| const membership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| teamId: input.teamId, | |
| userId: ctx.user.id, | |
| role: { in: ["ADMIN", "OWNER"] }, | |
| }, | |
| }); | |
| if (!membership) { | |
| throw new TRPCError({ code: "FORBIDDEN", message: "You do not have permission to modify team features" }); | |
| } | |
| const service = getFeatureManagementService(ctx.prisma); | |
| await service.setTeamFeatureEnabled( | |
| input.teamId, | |
| input.featureSlug, | |
| input.enabled, | |
| `user:${ctx.user.id}` | |
| ); | |
| return { success: true }; | |
| }), |
| /** | ||
| * Set the enabled status of a feature for an organization. | ||
| * Requires appropriate PBAC permissions. | ||
| */ | ||
| setOrganizationFeatureEnabled: authedProcedure | ||
| .input( | ||
| z.object({ | ||
| organizationId: z.number(), | ||
| featureSlug: z.string(), | ||
| enabled: z.boolean(), | ||
| }) | ||
| ) | ||
| .mutation(async ({ ctx, input }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
| await service.setOrganizationFeatureEnabled( | ||
| input.organizationId, | ||
| input.featureSlug, | ||
| input.enabled, | ||
| `user:${ctx.user.id}` | ||
| ); | ||
| return { success: true }; | ||
| }), |
There was a problem hiding this comment.
Critical: Missing authorization for organization feature mutations.
Same issue as the team mutation - any authenticated user can modify organization features without permission verification.
🤖 Prompt for AI Agents
In @packages/features/feature-management/trpc/router.ts around lines 97 - 118,
The mutation setOrganizationFeatureEnabled currently lets any authenticated user
change org features; add an explicit PBAC/authorization check (the same pattern
used for the team mutation) before calling getFeatureManagementService and
service.setOrganizationFeatureEnabled: validate ctx.user has the required
permission (e.g., organization:features:update or an "org admin" role) for
input.organizationId using your existing PBAC helper (the same helper used in
the team mutation) and throw an authorization error if it fails, then proceed to
call setOrganizationFeatureEnabled with the actor `user:${ctx.user.id}`.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| userId: session.user.id, | ||
| teamId: session.user.profile.organizationId, | ||
| resource: Resource.Feature, | ||
| userRole: session.user.org.role, |
There was a problem hiding this comment.
Suggestion: validateUserHasOrg only guarantees that the user has an organizationId, not that session.user.org is non-null, so directly accessing session.user.org.role can throw a runtime error when org is missing; use optional chaining with a safe default role to avoid a null/undefined property access. [null pointer]
Severity Level: Minor
| userRole: session.user.org.role, | |
| userRole: session.user.org?.role ?? MembershipRole.MEMBER, |
Why it matters? ⭐
The suggestion is correct and pragmatic: if validateUserHasOrg guarantees only an organizationId but not a populated session.user.org object, accessing session.user.org.role could throw at runtime. Using optional chaining with a sensible fallback (MembershipRole.MEMBER) prevents a potential undefined property access and provides a safe default role for permission checks. MembershipRole is already imported in the file, so the change is minimal and fixes a real robustness issue rather than being purely cosmetic.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsx
**Line:** 30:30
**Comment:**
*Null Pointer: `validateUserHasOrg` only guarantees that the user has an `organizationId`, not that `session.user.org` is non-null, so directly accessing `session.user.org.role` can throw a runtime error when `org` is missing; use optional chaining with a safe default role to avoid a null/undefined property access.
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.| export const generateMetadata = async ({ params }: { params: Promise<{ id: string }> }) => | ||
| await _generateMetadata( | ||
| (t) => t("features"), | ||
| (t) => t("team_features_description"), | ||
| undefined, | ||
| undefined, | ||
| `/settings/teams/${(await params).id}/features` |
There was a problem hiding this comment.
Suggestion: The generateMetadata function incorrectly types and treats params as a Promise, but in Next.js generateMetadata receives a plain params object; this mismatch can break the expected contract, confuse callers and tooling, and makes the implementation harder to reason about even though await on a non-promise currently happens to work. [type error]
Severity Level: Minor
| export const generateMetadata = async ({ params }: { params: Promise<{ id: string }> }) => | |
| await _generateMetadata( | |
| (t) => t("features"), | |
| (t) => t("team_features_description"), | |
| undefined, | |
| undefined, | |
| `/settings/teams/${(await params).id}/features` | |
| export const generateMetadata = async ({ params }: { params: { id: string } }) => | |
| await _generateMetadata( | |
| (t) => t("features"), | |
| (t) => t("team_features_description"), | |
| undefined, | |
| undefined, | |
| `/settings/teams/${params.id}/features` |
Why it matters? ⭐
The current code types params as a Promise and uses await params. In Next.js (App Router) generateMetadata receives a plain params object (e.g. { id: string }). Treating it as a Promise is incorrect: it confuses readers, can break type-checking, and forces an unnecessary await. Changing the type to { id: string } and using params.id aligns with the framework contract and removes the needless await. This is a real correctness/type issue, not mere style.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/features/page.tsx
**Line:** 5:11
**Comment:**
*Type Error: The `generateMetadata` function incorrectly types and treats `params` as a `Promise`, but in Next.js `generateMetadata` receives a plain `params` object; this mismatch can break the expected contract, confuse callers and tooling, and makes the implementation harder to reason about even though `await` on a non-promise currently happens to work.
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 { t } = useLocale(); | ||
| const params = useParams<{ id: string }>(); | ||
| const teamId = params?.id ? parseInt(params.id, 10) : null; | ||
| const utils = trpc.useUtils(); | ||
|
|
||
| const { data: features, isLoading } = trpc.viewer.featureManagement.listForTeam.useQuery( | ||
| { teamId: teamId! }, | ||
| { enabled: !!teamId } | ||
| ); | ||
|
|
||
| const setFeatureEnabledMutation = trpc.viewer.featureManagement.setTeamFeatureEnabled.useMutation({ | ||
| onSuccess: () => { | ||
| utils.viewer.featureManagement.listForTeam.invalidate({ teamId: teamId! }); | ||
| showToast(t("settings_updated_successfully"), "success"); | ||
| }, | ||
| onError: () => { | ||
| showToast(t("error_updating_settings"), "error"); | ||
| }, | ||
| }); | ||
|
|
||
| if (!teamId) { | ||
| return null; | ||
| } | ||
|
|
||
| if (isLoading) { | ||
| return ( | ||
| <SettingsHeader | ||
| title={t("features")} | ||
| description={t("team_features_description")} | ||
| borderInShellHeader={true}> | ||
| <SkeletonLoader /> | ||
| </SettingsHeader> | ||
| ); | ||
| } | ||
|
|
||
| const teamControlledFeatures = features?.filter((f) => f.globallyEnabled) ?? []; | ||
|
|
||
| return ( | ||
| <SettingsHeader | ||
| title={t("features")} | ||
| description={t("team_features_description")} | ||
| borderInShellHeader={true}> | ||
| <div className="border-subtle border-x px-4 py-8 sm:px-6"> | ||
| {teamControlledFeatures.length === 0 ? ( | ||
| <p className="text-subtle text-sm">{t("no_features_available")}</p> | ||
| ) : ( | ||
| <div className="space-y-6"> | ||
| {teamControlledFeatures.map((feature) => ( | ||
| <SettingsToggle | ||
| key={feature.slug} | ||
| toggleSwitchAtTheEnd={true} | ||
| title={feature.slug} | ||
| description={feature.description || t("no_description_available")} | ||
| disabled={setFeatureEnabledMutation.isPending} | ||
| checked={feature.enabled} | ||
| onCheckedChange={(checked) => { | ||
| setFeatureEnabledMutation.mutate({ |
There was a problem hiding this comment.
Suggestion: The TRPC query and mutation are always instantiated with teamId! even when the URL param is missing or not a valid number, so teamId may be null or NaN at runtime and still be passed into listForTeam.useQuery and setTeamFeatureEnabled, which can violate the API's expected input shape and cause hard-to-debug runtime errors; splitting the component so that TRPC hooks only run when a valid numeric teamId exists avoids ever calling them with invalid input. [possible bug]
Severity Level: Critical 🚨
| const { t } = useLocale(); | |
| const params = useParams<{ id: string }>(); | |
| const teamId = params?.id ? parseInt(params.id, 10) : null; | |
| const utils = trpc.useUtils(); | |
| const { data: features, isLoading } = trpc.viewer.featureManagement.listForTeam.useQuery( | |
| { teamId: teamId! }, | |
| { enabled: !!teamId } | |
| ); | |
| const setFeatureEnabledMutation = trpc.viewer.featureManagement.setTeamFeatureEnabled.useMutation({ | |
| onSuccess: () => { | |
| utils.viewer.featureManagement.listForTeam.invalidate({ teamId: teamId! }); | |
| showToast(t("settings_updated_successfully"), "success"); | |
| }, | |
| onError: () => { | |
| showToast(t("error_updating_settings"), "error"); | |
| }, | |
| }); | |
| if (!teamId) { | |
| return null; | |
| } | |
| if (isLoading) { | |
| return ( | |
| <SettingsHeader | |
| title={t("features")} | |
| description={t("team_features_description")} | |
| borderInShellHeader={true}> | |
| <SkeletonLoader /> | |
| </SettingsHeader> | |
| ); | |
| } | |
| const teamControlledFeatures = features?.filter((f) => f.globallyEnabled) ?? []; | |
| return ( | |
| <SettingsHeader | |
| title={t("features")} | |
| description={t("team_features_description")} | |
| borderInShellHeader={true}> | |
| <div className="border-subtle border-x px-4 py-8 sm:px-6"> | |
| {teamControlledFeatures.length === 0 ? ( | |
| <p className="text-subtle text-sm">{t("no_features_available")}</p> | |
| ) : ( | |
| <div className="space-y-6"> | |
| {teamControlledFeatures.map((feature) => ( | |
| <SettingsToggle | |
| key={feature.slug} | |
| toggleSwitchAtTheEnd={true} | |
| title={feature.slug} | |
| description={feature.description || t("no_description_available")} | |
| disabled={setFeatureEnabledMutation.isPending} | |
| checked={feature.enabled} | |
| onCheckedChange={(checked) => { | |
| setFeatureEnabledMutation.mutate({ | |
| const params = useParams<{ id: string }>(); | |
| const parsedTeamId = params?.id ? Number(params.id) : null; | |
| const teamId = | |
| typeof parsedTeamId === "number" && !Number.isNaN(parsedTeamId) ? parsedTeamId : null; | |
| if (teamId === null) { | |
| return null; | |
| } | |
| return <TeamFeaturesViewContent teamId={teamId} />; | |
| }; | |
| const TeamFeaturesViewContent = ({ teamId }: { teamId: number }) => { | |
| const { t } = useLocale(); | |
| const utils = trpc.useUtils(); | |
| const { data: features, isLoading } = trpc.viewer.featureManagement.listForTeam.useQuery( | |
| { teamId }, | |
| { enabled: true } | |
| ); | |
| const setFeatureEnabledMutation = trpc.viewer.featureManagement.setTeamFeatureEnabled.useMutation({ | |
| onSuccess: () => { | |
| utils.viewer.featureManagement.listForTeam.invalidate({ teamId }); | |
| showToast(t("settings_updated_successfully"), "success"); | |
| }, | |
| onError: () => { | |
| showToast(t("error_updating_settings"), "error"); | |
| }, | |
| }); | |
| if (isLoading) { | |
| return ( | |
| <SettingsHeader | |
| title={t("features")} | |
| description={t("team_features_description")} | |
| borderInShellHeader={true}> | |
| <SkeletonLoader /> | |
| </SettingsHeader> | |
| ); | |
| } | |
| const teamControlledFeatures = features?.filter((f) => f.globallyEnabled) ?? []; | |
| return ( | |
| <SettingsHeader | |
| title={t("features")} | |
| description={t("team_features_description")} | |
| borderInShellHeader={true}> | |
| <div className="border-subtle border-x px-4 py-8 sm:px-6"> | |
| {teamControlledFeatures.length === 0 ? ( | |
| <p className="text-subtle text-sm">{t("no_features_available")}</p> | |
| ) : ( | |
| <div className="space-y-6"> | |
| {teamControlledFeatures.map((feature) => ( | |
| <SettingsToggle | |
| key={feature.slug} | |
| toggleSwitchAtTheEnd={true} | |
| title={feature.slug} | |
| description={feature.description || t("no_description_available")} | |
| disabled={setFeatureEnabledMutation.isPending} | |
| checked={feature.enabled} | |
| onCheckedChange={(checked) => { | |
| setFeatureEnabledMutation.mutate({ | |
| teamId, |
Why it matters? ⭐
The current code uses the non-null assertion (teamId!) when building the query/mutation args even though teamId may be null or NaN. While the query is disabled when teamId is falsy (enabled: !!teamId), the argument object is still constructed and the non-null assertion is unsafe and could mask bugs or produce unexpected values. Splitting into a small guard component or only instantiating TRPC hooks when a validated numeric teamId exists is a real, defensible safety improvement — it prevents accidental calls with invalid input and avoids relying on the enabled flag as the sole guard.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/modules/settings/teams/team-features-view.tsx
**Line:** 25:81
**Comment:**
*Possible Bug: The TRPC query and mutation are always instantiated with `teamId!` even when the URL param is missing or not a valid number, so `teamId` may be `null` or `NaN` at runtime and still be passed into `listForTeam.useQuery` and `setTeamFeatureEnabled`, which can violate the API's expected input shape and cause hard-to-debug runtime errors; splitting the component so that TRPC hooks only run when a valid numeric `teamId` exists avoids ever calling them with invalid input.
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.| * Gets all user features for a specific user. | ||
| * @param userId - The ID of the user | ||
| * @returns Promise<UserFeatures[]> | ||
| */ |
There was a problem hiding this comment.
Suggestion: The JSDoc for the new getUserFeatures method advertises Promise<UserFeatures[]>, but no such type is imported here and the public contract (via the interface) is Promise<unknown[]>, so the doc is misleading and can cause callers or maintainers to assume a stronger type than actually guaranteed. [code quality]
Severity Level: Minor
| */ | |
| * @returns Promise<unknown[]> |
Why it matters? ⭐
The JSDoc for getUserFeatures claims Promise<UserFeatures[]>, yet there is no UserFeatures type imported/used for the Prisma return shape and the implementation returns the Prisma model rows (with included feature object). That makes the docstring misleading. Fixing the JSDoc to Promise<unknown[]> or importing/declaring the correct type keeps documentation accurate and prevents incorrect assumptions by maintainers.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/features/flags/features.repository.ts
**Line:** 521:521
**Comment:**
*Code Quality: The JSDoc for the new `getUserFeatures` method advertises `Promise<UserFeatures[]>`, but no such type is imported here and the public contract (via the interface) is `Promise<unknown[]>`, so the doc is misleading and can cause callers or maintainers to assume a stronger type than actually guaranteed.
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.| /** | ||
| * Gets all team features for a specific team. | ||
| * @param teamId - The ID of the team | ||
| * @returns Promise<TeamFeatures[]> |
There was a problem hiding this comment.
Suggestion: The JSDoc for the new getTeamFeaturesWithDetails method states Promise<TeamFeatures[]>, but TeamFeatures here is a map alias from config.ts, not the Prisma model being returned, so the comment misdocuments the shape of the data and can lead to incorrect assumptions in consuming code or refactors. [code quality]
Severity Level: Minor
| * @returns Promise<TeamFeatures[]> | |
| * @returns Promise<unknown[]> |
Why it matters? ⭐
The TeamFeatures type imported at top is an alias representing a map of slugs to booleans (used elsewhere), but getTeamFeaturesWithDetails returns Prisma teamFeatures rows with an included feature sub-object. The JSDoc claiming Promise<TeamFeatures[]> misdocuments the API and can lead to incorrect assumptions during refactors or by callers.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/features/flags/features.repository.ts
**Line:** 541:541
**Comment:**
*Code Quality: The JSDoc for the new `getTeamFeaturesWithDetails` method states `Promise<TeamFeatures[]>`, but `TeamFeatures` here is a map alias from `config.ts`, not the Prisma model being returned, so the comment misdocuments the shape of the data and can lead to incorrect assumptions in consuming code or refactors.
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.| }, | ||
| [CrudAction.Update]: { | ||
| description: "Update feature flags", | ||
| category: "feature", | ||
| i18nKey: "pbac_action_update", |
There was a problem hiding this comment.
Suggestion: The read and update permissions for the feature resource are missing an explicit organization scope, so getPermissionsForScope(Scope.Team) will include feature.read and feature.update while create/delete are org-only, exposing feature flags at the team scope and breaking the intended scoping consistency for this new resource. [logic error]
Severity Level: Minor
| }, | |
| [CrudAction.Update]: { | |
| description: "Update feature flags", | |
| category: "feature", | |
| i18nKey: "pbac_action_update", | |
| scope: [Scope.Organization], | |
| }, | |
| [CrudAction.Update]: { | |
| description: "Update feature flags", | |
| category: "feature", | |
| i18nKey: "pbac_action_update", | |
| descriptionI18nKey: "pbac_desc_update_features", | |
| scope: [Scope.Organization], |
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/features/pbac/domain/types/permission-registry.ts
**Line:** 771:775
**Comment:**
*Logic Error: The read and update permissions for the feature resource are missing an explicit organization scope, so `getPermissionsForScope(Scope.Team)` will include `feature.read` and `feature.update` while create/delete are org-only, exposing feature flags at the team scope and breaking the intended scoping consistency for this new resource.
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.
| // Early return if team has feature directly assigned | ||
| // Early return if team has feature directly assigned (row exists = enabled) | ||
| const teamHasFeature = await this.prismaClient.teamFeatures.findUnique({ | ||
| where: { |
There was a problem hiding this comment.
Suggestion: Using findUnique on the composite key in the new early-return path of the team feature check is inconsistent with the earlier Prismock limitation noted for similar join tables and can cause test/runtime failures in environments where findUnique on composite unique constraints is not supported, even though a simple existence check is all that is needed. [possible bug]
Severity Level: Critical 🚨
|
CodeAnt AI finished reviewing your PR. |
💡 Enhance Your PR ReviewsWe noticed that 3 feature(s) are not configured for this repository. Enabling these features can help improve your code quality and workflow: 🚦 Quality GatesStatus: Quality Gates are not enabled at the organization level 🎫 Jira Ticket ComplianceStatus: Jira credentials file not found. Please configure Jira integration in your settings ⚙️ Custom RulesStatus: No custom rules configured. Add rules via organization settings or .codeant/review.json in your repository Want to enable these features? Contact your organization admin or check our documentation for setup instructions. |
Code Review: Feature Management SystemPR Confidence Score: 🟥 1 / 5👍 Well Done
📁 Selected files for review (18)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| * Set the enabled status of a feature for a team. | ||
| * Requires appropriate PBAC permissions. | ||
| */ | ||
| setTeamFeatureEnabled: authedProcedure |
There was a problem hiding this comment.
Missing PBAC Validation
Team feature modification endpoint lacks PBAC permission validation allowing unauthorized users to modify team features. Attackers could enable/disable features for teams they don't have admin access to. This bypasses the permission system implemented in the UI layer and could lead to privilege escalation.
setTeamFeatureEnabled: authedProcedure
.input(
z.object({
teamId: z.number(),
featureSlug: z.string(),
enabled: z.boolean(),
})
)
.mutation(async ({ ctx, input }) => {
// Enforce PBAC: verify user has admin role for the team
const adminMembership = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
teamId: input.teamId,
role: { in: ['ADMIN', 'OWNER'] }
}
});
if (!adminMembership) {
throw new Error('Access denied: insufficient team permissions');
}
const service = getFeatureManagementService(ctx.prisma);
await service.setTeamFeatureEnabled(
input.teamId,
input.featureSlug,
input.enabled,
`user:${ctx.user.id}`
);
return { success: true };
}),
Commitable Suggestion
| setTeamFeatureEnabled: authedProcedure | |
| setTeamFeatureEnabled: authedProcedure | |
| .input( | |
| z.object({ | |
| teamId: z.number(), | |
| featureSlug: z.string(), | |
| enabled: z.boolean(), | |
| }) | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| // Enforce PBAC: verify user has admin role for the team | |
| const adminMembership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| teamId: input.teamId, | |
| role: { in: ['ADMIN', 'OWNER'] } | |
| } | |
| }); | |
| if (!adminMembership) { | |
| throw new Error('Access denied: insufficient team permissions'); | |
| } | |
| const service = getFeatureManagementService(ctx.prisma); | |
| await service.setTeamFeatureEnabled( | |
| input.teamId, | |
| input.featureSlug, | |
| input.enabled, | |
| `user:${ctx.user.id}` | |
| ); | |
| return { success: true }; | |
| }), |
Standards
- CWE-862
- OWASP-A01
- NIST-SSDF-PW.1
| * Set the enabled status of a feature for an organization. | ||
| * Requires appropriate PBAC permissions. | ||
| */ | ||
| setOrganizationFeatureEnabled: authedProcedure |
There was a problem hiding this comment.
Missing Organization PBAC
Organization feature modification endpoint lacks PBAC permission validation allowing unauthorized access to organization-level feature controls. Users without proper organization admin privileges could modify critical organizational features. This represents a significant authorization bypass vulnerability.
setOrganizationFeatureEnabled: authedProcedure
.input(
z.object({
organizationId: z.number(),
featureSlug: z.string(),
enabled: z.boolean(),
})
)
.mutation(async ({ ctx, input }) => {
// Enforce PBAC: verify user has admin role for the organization
const adminMembership = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
teamId: input.organizationId,
role: { in: ['ADMIN', 'OWNER'] },
team: { isOrganization: true }
}
});
if (!adminMembership) {
throw new Error('Access denied: insufficient organization permissions');
}
const service = getFeatureManagementService(ctx.prisma);
await service.setOrganizationFeatureEnabled(
input.organizationId,
input.featureSlug,
input.enabled,
`user:${ctx.user.id}`
);
return { success: true };
}),
Commitable Suggestion
| setOrganizationFeatureEnabled: authedProcedure | |
| setOrganizationFeatureEnabled: authedProcedure | |
| .input( | |
| z.object({ | |
| organizationId: z.number(), | |
| featureSlug: z.string(), | |
| enabled: z.boolean(), | |
| }) | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| // Enforce PBAC: verify user has admin role for the organization | |
| const adminMembership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| teamId: input.organizationId, | |
| role: { in: ['ADMIN', 'OWNER'] }, | |
| team: { isOrganization: true } | |
| } | |
| }); | |
| if (!adminMembership) { | |
| throw new Error('Access denied: insufficient organization permissions'); | |
| } | |
| const service = getFeatureManagementService(ctx.prisma); | |
| await service.setOrganizationFeatureEnabled( | |
| input.organizationId, | |
| input.featureSlug, | |
| input.enabled, | |
| `user:${ctx.user.id}` | |
| ); | |
| return { success: true }; | |
| }), |
Standards
- CWE-862
- OWASP-A01
- NIST-SSDF-PW.1
| for (const slug of optInSlugs) { | ||
| const config = getOptInFeatureConfig(slug); | ||
| if (!config) continue; | ||
|
|
||
| const userFeature = await this.featuresRepository.getUserFeature(userId, slug); | ||
|
|
||
| // Row exists = user has already opted in | ||
| if (userFeature) { | ||
| continue; | ||
| } | ||
|
|
||
| const isGloballyEnabled = await this.featuresRepository.checkIfFeatureIsEnabledGlobally( | ||
| slug as Parameters<typeof this.featuresRepository.checkIfFeatureIsEnabledGlobally>[0] |
There was a problem hiding this comment.
N+1 Query Pattern
The loop iterates through optInSlugs and executes sequential database queries (getUserFeature and checkIfFeatureIsEnabledGlobally) for each iteration. This creates an N+1 query pattern where the number of database round-trips grows linearly with the number of opt-in features, potentially causing latency spikes.
// Batch fetch all user features and globally enabled features
const userFeatures = await this.featuresRepository.getUserFeatures(userId);
const allFeatures = await this.featuresRepository.getAllFeatures();
const userFeatureMap = new Map(userFeatures.map(uf => [uf.feature.slug, uf]));
const globalFeatureMap = new Map(allFeatures.map(f => [f.slug, f.enabled]));
for (const slug of optInSlugs) {
const config = getOptInFeatureConfig(slug);
if (!config) continue;
// Check if user has already opted in using cached data
if (userFeatureMap.has(slug)) {
continue;
}
// Check if globally enabled using cached data
const isGloballyEnabled = globalFeatureMap.get(slug);
if (!isGloballyEnabled) continue;
Commitable Suggestion
| for (const slug of optInSlugs) { | |
| const config = getOptInFeatureConfig(slug); | |
| if (!config) continue; | |
| const userFeature = await this.featuresRepository.getUserFeature(userId, slug); | |
| // Row exists = user has already opted in | |
| if (userFeature) { | |
| continue; | |
| } | |
| const isGloballyEnabled = await this.featuresRepository.checkIfFeatureIsEnabledGlobally( | |
| slug as Parameters<typeof this.featuresRepository.checkIfFeatureIsEnabledGlobally>[0] | |
| // Batch fetch all user features and globally enabled features | |
| const userFeatures = await this.featuresRepository.getUserFeatures(userId); | |
| const allFeatures = await this.featuresRepository.getAllFeatures(); | |
| const userFeatureMap = new Map(userFeatures.map(uf => [uf.feature.slug, uf])); | |
| const globalFeatureMap = new Map(allFeatures.map(f => [f.slug, f.enabled])); | |
| for (const slug of optInSlugs) { | |
| const config = getOptInFeatureConfig(slug); | |
| if (!config) continue; | |
| // Check if user has already opted in using cached data | |
| if (userFeatureMap.has(slug)) { | |
| continue; | |
| } | |
| // Check if globally enabled using cached data | |
| const isGloballyEnabled = globalFeatureMap.get(slug); | |
| if (!isGloballyEnabled) continue; |
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Batch-Processing
| setUserFeatureEnabled: authedProcedure | ||
| .input( | ||
| z.object({ | ||
| featureSlug: z.string(), | ||
| enabled: z.boolean(), | ||
| }) | ||
| ) | ||
| .mutation(async ({ ctx, input }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
| await service.setUserFeatureEnabled( | ||
| ctx.user.id, | ||
| input.featureSlug, | ||
| input.enabled, | ||
| `user:${ctx.user.id}` | ||
| ); | ||
| return { success: true }; | ||
| }), |
There was a problem hiding this comment.
Feature Slug Validation
Feature slug parameter accepts arbitrary strings without validation against allowed feature list. Attackers could attempt to enable non-existent or restricted features causing database errors or unexpected behavior. Input validation should restrict to known feature slugs only.
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
| <SettingsHeader title={t("features")} description={t("features_description")} borderInShellHeader={true}> | ||
| <div className="border-subtle border-x px-4 py-8 sm:px-6"> | ||
| {userControlledFeatures.length === 0 ? ( | ||
| <p className="text-subtle text-sm">{t("no_features_available")}</p> | ||
| ) : ( | ||
| <div className="space-y-6"> | ||
| {userControlledFeatures.map((feature) => ( | ||
| <SettingsToggle | ||
| key={feature.slug} |
There was a problem hiding this comment.
View Logic Duplication
The UI rendering logic for feature toggles is duplicated across user, team, and organization views. Extracting a shared FeatureSettingsList component would reduce code duplication and centralize UI updates.
Standards
- Clean-Code-DRY
- Maintainability-Quality-Component-Reuse
| const stored = localStorage.getItem("cal_feature_banners_dismissed"); | ||
| setDismissedFeatures(stored ? JSON.parse(stored) : []); |
There was a problem hiding this comment.
Hardcoded Key Duplication
Logic duplicates storage access using a hardcoded key instead of the exported getDismissedBanners utility and DISMISSED_BANNERS_KEY constant. This creates a risk of key mismatch (functional correctness) and bypasses the centralized error handling logic defined in the component.
Standards
- ISO-IEC-25010-Maintainability-Modularity
- DbC-Invariants
CodeAnt-AI Description
Add feature opt-in UI and management with per-user, team, and organization toggles and an opt-in banner
What Changed
Impact
✅ Can opt into beta features via URL banner✅ Manage features per account, team, and organization✅ Clearer permission messaging when users can't view feature settings💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.