Devin/feature opt in system 1765208720 #1
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 · |
Summary of ChangesHello @ayushjrathod-dd, 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 introduces a comprehensive feature opt-in system, empowering users, teams, and organizations to manage their feature preferences through dedicated settings interfaces. It includes a user-friendly banner mechanism to highlight and allow opting into new functionalities, such as 'bookings-v3'. The system is built on robust backend services and tRPC endpoints for managing feature states and is seamlessly integrated with the existing Permission-Based Access Control (PBAC) system to ensure secure and authorized access to feature configurations. Highlights
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
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request introduces a comprehensive feature management system, adding navigation tabs and settings pages for feature preferences across user, organization, and team scopes. It includes UI components for displaying feature toggles, a TRPC-backed service layer for feature operations, a configuration-driven opt-in banner system with localStorage persistence, permission registry extensions, and repository methods for feature flag persistence. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Refacto PR SummaryImplemented comprehensive feature management system enabling users, teams, and organizations to control feature flags through settings UI and opt-in banner system. The architecture uses row-existence semantics where database records indicate feature enablement, with hierarchical permission controls and TRPC endpoints for real-time feature toggling. 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: Toggle feature switch
UI->>TRPC: setUserFeatureEnabled(slug, enabled)
TRPC->>FMS: setUserFeatureEnabled()
FMS->>FR: setUserFeatureEnabled()
alt Enable Feature
FR->>DB: INSERT UserFeatures row
else Disable Feature
FR->>DB: DELETE UserFeatures row
end
DB-->>FR: Operation result
FR-->>FMS: Success
FMS-->>TRPC: Success response
TRPC-->>UI: Feature updated
UI->>UI: Refresh feature list
Testing GuideClick to expand
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
apps/web/modules/settings/organizations/organization-features-view.tsx (1)
30-32: Consider adding error state handling for the query.If the query fails, users see no feedback. The component will either stay in loading state forever or show "no features available" without indicating an error occurred.
- const { data: features, isLoading } = trpc.viewer.featureManagement.listForOrganization.useQuery({ + const { data: features, isLoading, isError } = trpc.viewer.featureManagement.listForOrganization.useQuery({ organizationId, });Then handle
isErrorin the render logic to show an appropriate error message.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsx (1)
26-39: UnusedcanUpdatepermission is computed but never used.The
getResourcePermissionscall computes bothcanReadandcanUpdate(via fallbackRoles), but onlycanReadis destructured and used. IfcanUpdateis intended for future use, consider documenting this; otherwise, remove theupdatefallback to avoid unnecessary computation.- const { canRead } = await getResourcePermissions({ + const { canRead } = 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], - }, }, });packages/features/feature-management/hooks/useFeatureOptInBanner.ts (1)
49-62: Potential race: dismissed features may not be loaded when eligibility check runs.The localStorage effect and the eligibility effect can race. If
eligibleFeaturesresolves before the localStorage read completes (both happen on mount),dismissedFeatureswill still be the initial empty array, potentially showing a dismissed banner briefly.Consider initializing
dismissedFeaturessynchronously or combining the effects.- const [dismissedFeatures, setDismissedFeatures] = useState<string[]>([]); + const [dismissedFeatures, setDismissedFeatures] = useState<string[]>(() => { + if (typeof window !== "undefined") { + try { + const stored = localStorage.getItem("cal_feature_banners_dismissed"); + return stored ? JSON.parse(stored) : []; + } catch { + return []; + } + } + return []; + });This initializes state synchronously on the client, avoiding the race.
apps/web/modules/settings/my-account/features-view.tsx (1)
10-20: Consider extractingSkeletonLoaderto a shared location.This
SkeletonLoadercomponent is duplicated verbatim inteam-features-view.tsx. Consider extracting it to a shared module (e.g.,@calcom/ui/components/skeletonor a local shared module) to reduce duplication.packages/features/feature-management/trpc/router.ts (1)
146-161: Use a TRPC-specific error type for invalid opt-in requests
optInToFeaturethrows a plainError("Feature is not available for opt-in"). In TRPC, this will surface as an internal error rather than a clear client error code. Prefer throwing aTRPCErrorwith aBAD_REQUEST(or similar) code so callers can distinguish invalid opt-in attempts from server failures.Example:
- if (!service.isFeatureInOptInAllowlist(input.featureSlug)) { - throw new Error("Feature is not available for opt-in"); - } + if (!service.isFeatureInOptInAllowlist(input.featureSlug)) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Feature is not available for opt-in", + }); + }(You’d need to import
TRPCErrorfrom your TRPC server package.)packages/features/feature-management/components/FeatureOptInBanner.tsx (2)
11-36: Local dismissal storage is robust enough; consider a light shape checkThe dismissal helpers correctly guard against
windowbeing undefined and swallowlocalStorageerrors, which is appropriate for this banner use case. If you ever expect other code to write toDISMISSED_BANNERS_KEY, you may want to defensively ensureJSON.parsereturns an array before calling.includes, but it’s not required for current usage.
48-63: Consider basic error feedback on failed opt-in mutationsOn opt-in failure, the banner silently stays in place with no feedback. Depending on how critical these opt-ins are, you might want to at least log or surface a minimal error state (e.g., toast, disabled retry button) in the mutation
onErrorcallback.packages/features/flags/features.repository.ts (3)
300-400: Row-existence setters for user/team features match the documented semantics
setUserFeatureEnabledandsetTeamFeatureEnabledcorrectly implement the “row-existence = enabled” model by upserting whenenabled === trueand deleting whenenabled === false. Unique compound keys (userId_featureId/teamId_featureId) are used appropriately.The two methods are nearly identical; if this pattern grows further, you may consider a small internal helper to reduce duplication, but that’s optional.
402-430: Clarify return typing and JSDoc forgetUserFeature/getTeamFeatureBoth methods return the raw Prisma model instances via
findFirst, but:
- The interface signatures use
Promise<unknown>.- The JSDoc for
getTeamFeaturesaysPromise<TeamFeatures | null>, whereTeamFeaturesis also used as a map type from./config.This isn’t a runtime bug, but the naming and typing are a bit confusing. Consider explicitly typing these to the Prisma model types (e.g.,
Prisma.UserFeatures | null,Prisma.TeamFeatures | null) and tightening the interface signatures accordingly, or adjusting the JSDoc to avoid clashing with the configTeamFeaturestype.
495-515: Cache invalidation after user/team toggles is conservative but potentially unnecessary
getTeamsWithFeatureEnableddepends onTeamFeaturesrows and the global feature map, butsetUserFeatureEnabled/setTeamFeatureEnabledonly mutateUserFeatures/TeamFeatures. Clearing the globalfeaturesCachein these setters is safe but may cause extra DB reads forFeaturedefinitions that haven’t changed.If you want to shave a bit of overhead, you could skip
this.clearCache()in the user/team setters and only clear it whereFeaturerows themselves are modified.packages/features/flags/features.repository.interface.ts (1)
3-14: Interface extensions match implementation; consider tightening return typesThe new methods (
checkIfUserHasFeatureNonHierarchical, setters, and getFeature APIs) align with the repository implementation, but the signatures for the getters all returnunknown/unknown[]. Since the concrete class returns Prisma model shapes, you could improve type safety by exporting those model types and using them here instead ofunknown, especially forgetUserFeatures/getTeamFeaturesWithDetailswhich are used by the feature-management service.packages/features/feature-management/services/FeatureManagementService.ts (1)
32-76: LGTM! Row-existence semantics are consistently applied.The list methods correctly implement the feature-enabled logic based on row existence. The delegation from
listFeaturesForOrganizationtolistFeaturesForTeamis appropriate given the organizational structure.Consider adding input validation for
userId,teamId, andorganizationId(e.g., checking for positive integers) to fail fast on invalid inputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx(3 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/features/page.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/features/page.tsx(1 hunks)apps/web/modules/settings/my-account/features-view.tsx(1 hunks)apps/web/modules/settings/organizations/organization-features-view.tsx(1 hunks)apps/web/modules/settings/teams/team-features-view.tsx(1 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/features/feature-management/components/FeatureOptInBanner.tsx(1 hunks)packages/features/feature-management/config/feature-management.config.ts(1 hunks)packages/features/feature-management/hooks/useFeatureOptInBanner.ts(1 hunks)packages/features/feature-management/index.ts(1 hunks)packages/features/feature-management/services/FeatureManagementService.ts(1 hunks)packages/features/feature-management/trpc/router.ts(1 hunks)packages/features/flags/features.repository.interface.ts(1 hunks)packages/features/flags/features.repository.ts(9 hunks)packages/features/pbac/domain/types/permission-registry.ts(2 hunks)packages/trpc/server/routers/viewer/_router.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
apps/web/modules/settings/organizations/organization-features-view.tsx (1)
packages/trpc/react/trpc.ts (1)
trpc(50-134)
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)/organizations/features/page.tsx (5)
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)/teams/[id]/features/page.tsx (1)
generateMetadata(5-12)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/pbac/domain/types/permission-registry.ts (1)
packages/platform/libraries/pbac.ts (1)
Scope(2-2)
packages/features/feature-management/config/feature-management.config.ts (2)
packages/features/feature-management/index.ts (5)
OptInFeatureConfig(10-10)OPT_IN_FEATURES(5-5)getOptInFeatureConfig(6-6)isFeatureInOptInAllowlist(7-7)getOptInFeatureSlugs(8-8)packages/features/feature-management/services/FeatureManagementService.ts (2)
getOptInFeatureConfig(163-165)isFeatureInOptInAllowlist(156-158)
packages/features/flags/features.repository.interface.ts (1)
packages/features/flags/config.ts (1)
AppFlags(5-37)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (1)
packages/ui/components/navigation/tabs/VerticalTabs.tsx (1)
VerticalTabItem(6-6)
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/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/app/(use-page-wrapper)/settings/(settings-layout)/my-account/features/page.tsx (4)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsx (1)
generateMetadata(12-19)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/features/page.tsx (1)
generateMetadata(5-12)apps/web/app/_utils.tsx (1)
_generateMetadata(53-81)apps/web/lib/buildLegacyCtx.ts (1)
buildLegacyRequest(47-49)
apps/web/modules/settings/teams/team-features-view.tsx (1)
packages/trpc/react/trpc.ts (1)
trpc(50-134)
🔇 Additional comments (24)
packages/features/pbac/domain/types/permission-registry.ts (2)
16-16: LGTM!The new
Featureresource enum value follows the established naming convention.
754-787: LGTM!The permission registry entry for
Featureis well-structured:
- Follows the same pattern as other resources (e.g.,
Watchlist)- Create and Delete are appropriately scoped to
Organizationonly- Read and Update are available in both scopes
- All write operations correctly depend on
feature.read- i18n keys follow the established naming convention
packages/trpc/server/routers/viewer/_router.tsx (1)
2-2: LGTM!The
featureManagementRouteris correctly imported and mounted. The naming distinction betweenfeatures(existing flag router) andfeatureManagement(new management router) is clear.Also applies to: 79-79
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (3)
47-47: LGTM!The features tab for my_account follows the established pattern with appropriate tracking metadata.
95-99: LGTM!The features tab for organizations is correctly placed and follows the existing pattern.
540-547: LGTM!The team features navigation item follows the same pattern as other team settings tabs (appearance, billing, settings) and includes appropriate tracking metadata with the team ID.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/features/page.tsx (2)
20-30: LGTM!The page correctly implements server-side authentication with proper redirect handling. Since this is for "my-account", the
FeaturesViewcomponent appropriately uses the session user's context rather than requiring an explicit user ID prop.
11-18: LGTM!The metadata generation follows the established pattern used by other feature pages.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/features/page.tsx (1)
14-16: The original review comment is incorrect. No changes needed.
TeamFeaturesViewis a client-side component that uses theuseParams()hook to access the team ID from route parameters (line 26-27 in team-features-view.tsx). It does not acceptteamIdas a prop. ThePagecomponent correctly renders<TeamFeaturesView />without passing props, allowing the client component to retrieve the ID from the URL params itself.Likely an incorrect or invalid review comment.
apps/web/modules/settings/organizations/organization-features-view.tsx (1)
68-72: The review comment is incorrect. TheFeatureWithStatusinterface returned bylistFeaturesForOrganizationonly contains the fieldsslug,enabled,globallyEnabled,description, andtype—there is nonameortitlefield available. The suggested refactor usingfeature.name || feature.slugwould fail at runtime. Usingfeature.slugas the title is the appropriate approach given the available data structure.Likely an incorrect or invalid review comment.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsx (2)
12-19: LGTM!The
generateMetadataimplementation follows the established pattern used in other feature pages (my-account, teams), correctly passing the organization-specific description key and path.
41-54: LGTM!The permission-based conditional rendering is well-implemented. The no-permission fallback provides clear user feedback, and the happy path correctly passes the organizationId to the view component.
apps/web/modules/settings/teams/team-features-view.tsx (2)
35-43: LGTM!The mutation setup correctly invalidates the cache on success and provides user feedback via toasts. Disabling toggles during
isPendingprevents race conditions from rapid toggling.
60-90: LGTM!The feature list rendering logic is solid—filtering by
globallyEnabled, handling empty state, and properly keying list items byfeature.slug.packages/features/feature-management/hooks/useFeatureOptInBanner.ts (1)
64-70: LGTM!The
dismissCurrentFeaturecallback properly persists the dismissal, updates local state, and clears the displayed feature in a single action.apps/web/modules/settings/my-account/features-view.tsx (2)
22-47: LGTM!The query and mutation setup is correct. Cache invalidation on success and error feedback via toasts follow established patterns.
49-78: LGTM!The feature list rendering correctly filters by
globallyEnabled, handles empty states gracefully, and disables toggles during pending mutations to prevent race conditions.packages/features/feature-management/config/feature-management.config.ts (2)
32-48: LGTM!The utility functions are clean, well-documented, and appropriate for the current allowlist size. The implementations are straightforward and idiomatic.
19-26: ThelearnMoreUrlpointing tohttps://cal.com/docs/features/bookings-v3is valid and publicly accessible.packages/features/feature-management/index.ts (1)
1-20: Barrel exports look consistent and cohesiveThe index cleanly re-exports the router, service, config helpers, banner, and hook, giving consumers a single entrypoint for feature-management concerns. No issues spotted.
apps/web/public/static/locales/en/common.json (1)
1256-1269: New feature-management and PBAC strings are consistent and descriptiveThe added keys for feature pages, opt-in CTA, and feature PBAC descriptions read well and align with the new UI and permission concepts. No localization or naming issues from what’s visible here.
packages/features/flags/features.repository.ts (2)
274-294: UpdatingassignedByon team re-assignment is correctThe
enableFeatureForTeamupsert now updatesassignedByon existing rows, which keeps audit metadata current while preserving row-existence semantics. This change is sound.
517-557: Detail fetchers for user/team features align with feature-management needs
getUserFeaturesandgetTeamFeaturesWithDetailsinclude the relatedfeaturewithslug,enabled,description, andtype, which is exactly what the feature-management service needs to build rich views. The queries look correct and match the row-existence model.packages/features/feature-management/services/FeatureManagementService.ts (1)
156-174: LGTM! Helper methods are straightforward and correct.The helper methods follow consistent patterns:
- Proper delegation to configuration utilities
- Correct row-existence semantics in
hasUserOptedIn
| 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 } | ||
| ); |
There was a problem hiding this comment.
Handle potential NaN from parseInt.
If params.id contains a non-numeric string, parseInt returns NaN, which is truthy. This would cause the query to execute with NaN as teamId. Consider adding explicit validation.
- const teamId = params?.id ? parseInt(params.id, 10) : null;
+ const parsedId = params?.id ? parseInt(params.id, 10) : NaN;
+ const teamId = Number.isNaN(parsedId) ? null : parsedId;📝 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 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 params = useParams<{ id: string }>(); | |
| const parsedId = params?.id ? parseInt(params.id, 10) : NaN; | |
| const teamId = Number.isNaN(parsedId) ? null : parsedId; | |
| const utils = trpc.useUtils(); | |
| const { data: features, isLoading } = trpc.viewer.featureManagement.listForTeam.useQuery( | |
| { teamId: teamId! }, | |
| { enabled: !!teamId } | |
| ); |
🤖 Prompt for AI Agents
In apps/web/modules/settings/teams/team-features-view.tsx around lines 26 to 33,
parseInt(params.id, 10) can return NaN which is truthy and causes the query to
run with an invalid teamId; validate the parsed value explicitly (e.g. const
parsedId = parseInt(params.id, 10); const teamId = Number.isInteger(parsedId) ?
parsedId : null) and change the query enabled flag to check teamId !== null (or
Boolean(teamId) if zero is impossible) so the hook never runs with NaN.
| 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.
Unsafe JSON.parse can throw on corrupted localStorage data.
If localStorage contains malformed JSON, JSON.parse will throw and break the component. Wrap in try/catch with a fallback.
useEffect(() => {
if (typeof window !== "undefined") {
- const stored = localStorage.getItem("cal_feature_banners_dismissed");
- setDismissedFeatures(stored ? JSON.parse(stored) : []);
+ try {
+ const stored = localStorage.getItem("cal_feature_banners_dismissed");
+ setDismissedFeatures(stored ? JSON.parse(stored) : []);
+ } 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") { | |
| try { | |
| const stored = localStorage.getItem("cal_feature_banners_dismissed"); | |
| setDismissedFeatures(stored ? JSON.parse(stored) : []); | |
| } catch { | |
| setDismissedFeatures([]); | |
| } | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
In packages/features/feature-management/hooks/useFeatureOptInBanner.ts around
lines 42 to 47, the code calls JSON.parse on localStorage data which can throw
on malformed JSON; wrap the parse in a try/catch, default to an empty array on
error (and optionally when parsed value is not an array), and then call
setDismissedFeatures with the safe fallback so the component won’t crash if
localStorage is corrupted.
| async setUserFeatureEnabled( | ||
| userId: number, | ||
| featureSlug: string, | ||
| enabled: boolean, | ||
| assignedBy: string | ||
| ): Promise<void> { | ||
| await this.featuresRepository.setUserFeatureEnabled(userId, featureSlug, enabled, assignedBy); | ||
| } | ||
|
|
||
| /** | ||
| * Set the enabled status of a feature for a team. | ||
| */ | ||
| async setTeamFeatureEnabled( | ||
| teamId: number, | ||
| featureSlug: string, | ||
| enabled: boolean, | ||
| assignedBy: string | ||
| ): Promise<void> { | ||
| await this.featuresRepository.setTeamFeatureEnabled(teamId, featureSlug, enabled, assignedBy); | ||
| } | ||
|
|
||
| /** | ||
| * Set the enabled status of a feature for an organization. | ||
| * Organizations are teams, so we use the same method. | ||
| */ | ||
| async setOrganizationFeatureEnabled( | ||
| organizationId: number, | ||
| featureSlug: string, | ||
| enabled: boolean, | ||
| assignedBy: string | ||
| ): Promise<void> { | ||
| await this.featuresRepository.setTeamFeatureEnabled(organizationId, featureSlug, enabled, assignedBy); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find TRPC router files related to feature management
fd -e ts -e tsx 'router|trpc' packages/features/feature-management/ | grep -E '(router|trpc)' | head -20Repository: coderabbit-test/cal.com
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Search for the feature management router and the set methods
rg -l 'setUserFeatureEnabled|setTeamFeatureEnabled|setOrganizationFeatureEnabled' packages/features/feature-management/ --type=ts -g '*.ts'Repository: coderabbit-test/cal.com
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Read the TRPC router file to examine authorization
cat -n packages/features/feature-management/trpc/router.tsRepository: coderabbit-test/cal.com
Length of output: 5887
Add authorization checks before allowing feature modifications for teams and organizations.
The setTeamFeatureEnabled and setOrganizationFeatureEnabled mutations use authedProcedure only, which enforces authentication but not authorization. Any authenticated user can enable/disable features for any team or organization. The router comments claim "Requires appropriate PBAC permissions" but the code does not enforce these checks. Add permission validation (e.g., verifying the user is a team admin or has org management rights) before delegating to the service layer.
🤖 Prompt for AI Agents
packages/features/feature-management/services/FeatureManagementService.ts lines
81-113: The issue is that setTeamFeatureEnabled and
setOrganizationFeatureEnabled perform only authentication but no authorization,
allowing any authenticated user to modify any team/org feature; before calling
the repository, validate the caller's permissions (e.g., require team admin or
org management rights) using the request context or an injected authorization
service/ PBAC check, and if the check fails throw a Forbidden/Unauthorized
error; for organization treat organizationId as a team id and reuse the same
permission check; only call this.featuresRepository.setTeamFeatureEnabled(...)
after the permission check succeeds.
| async getEligibleOptInFeatures(userId: number): Promise<EligibleOptInFeature[]> { | ||
| const eligibleFeatures: EligibleOptInFeature[] = []; | ||
| const optInSlugs = getOptInFeatureSlugs(); | ||
|
|
||
| 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] | ||
| ); | ||
| if (!isGloballyEnabled) continue; | ||
|
|
||
| eligibleFeatures.push({ | ||
| slug: config.slug, | ||
| titleI18nKey: config.titleI18nKey, | ||
| descriptionI18nKey: config.descriptionI18nKey, | ||
| learnMoreUrl: config.learnMoreUrl, | ||
| }); | ||
| } | ||
|
|
||
| return eligibleFeatures; | ||
| } |
There was a problem hiding this comment.
N+1 query performance issue in opt-in feature eligibility check.
The method makes two database calls per opt-in feature (lines 130 and 137), resulting in an N+1 query problem. If there are 10 opt-in features, this generates 20+ database queries.
Refactor to batch the queries:
async getEligibleOptInFeatures(userId: number): Promise<EligibleOptInFeature[]> {
const eligibleFeatures: EligibleOptInFeature[] = [];
const optInSlugs = getOptInFeatureSlugs();
+
+ // Batch fetch user features for all opt-in slugs
+ const userFeatures = await this.featuresRepository.getUserFeatures(userId);
+ const userFeatureSlugs = new Set(userFeatures.map(uf => uf.feature.slug));
+
+ // Batch fetch globally enabled features
+ const allFeatures = await this.featuresRepository.getAllFeatures();
+ const globallyEnabledSlugs = new Set(
+ allFeatures.filter(f => f.enabled).map(f => f.slug)
+ );
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;
+ if (!globallyEnabledSlugs.has(slug)) continue;
eligibleFeatures.push({
slug: config.slug,
titleI18nKey: config.titleI18nKey,
descriptionI18nKey: config.descriptionI18nKey,
learnMoreUrl: config.learnMoreUrl,
});
}
return eligibleFeatures;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/features/feature-management/services/FeatureManagementService.ts
around lines 122 to 151, the loop issues N+1 DB queries by calling
getUserFeature and checkIfFeatureIsEnabledGlobally for each slug; instead, fetch
data in batches up-front: collect optInSlugs, call the repository once to get
all user features for userId and those slugs, and call the repository once to
get global-enabled statuses for all slugs (or a single query returning enabled
slugs), then iterate optInSlugs synchronously using the in-memory maps/sets to
skip already opted-in or globally-disabled features and push eligibleFeatures;
remove per-iteration awaits to eliminate the N+1 queries.
| const isGloballyEnabled = await this.featuresRepository.checkIfFeatureIsEnabledGlobally( | ||
| slug as Parameters<typeof this.featuresRepository.checkIfFeatureIsEnabledGlobally>[0] | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the file and examine the context around lines 137-139
cat -n packages/features/feature-management/services/FeatureManagementService.ts | sed -n '130,150p'Repository: coderabbit-test/cal.com
Length of output: 883
🏁 Script executed:
#!/bin/bash
# Find the checkIfFeatureIsEnabledGlobally method signature
rg -n -A10 'checkIfFeatureIsEnabledGlobally' packages/features/feature-management/services/FeatureManagementService.tsRepository: coderabbit-test/cal.com
Length of output: 567
🏁 Script executed:
#!/bin/bash
# Find the repository interface to see parameter type
fd -e ts -e interface | xargs rg -l 'checkIfFeatureIsEnabledGlobally' 2>/dev/null | head -5Repository: coderabbit-test/cal.com
Length of output: 364
🏁 Script executed:
#!/bin/bash
# Look for the features repository interface
find packages -name '*.interface.ts' -o -name '*.repository.ts' | xargs rg -l 'checkIfFeatureIsEnabledGlobally' 2>/dev/nullRepository: coderabbit-test/cal.com
Length of output: 241
🏁 Script executed:
#!/bin/bash
# Find getOptInFeatureSlugs method
rg -n -B2 -A5 'getOptInFeatureSlugs' packages/features/feature-management/services/FeatureManagementService.tsRepository: coderabbit-test/cal.com
Length of output: 738
🏁 Script executed:
#!/bin/bash
# Check the repository interface for checkIfFeatureIsEnabledGlobally signature
cat -n packages/features/flags/features.repository.interface.ts | grep -A5 'checkIfFeatureIsEnabledGlobally'Repository: coderabbit-test/cal.com
Length of output: 609
🏁 Script executed:
#!/bin/bash
# Find and check the feature-management.config file
fd -e ts 'feature-management.config' packages/features/feature-management/Repository: coderabbit-test/cal.com
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Find the return type of getOptInFeatureSlugs
rg -n -A10 'export.*getOptInFeatureSlugs' packages/features/feature-management/Repository: coderabbit-test/cal.com
Length of output: 404
🏁 Script executed:
#!/bin/bash
# Check the implementation of checkIfFeatureIsEnabledGlobally in the repository
rg -n -B2 -A8 'checkIfFeatureIsEnabledGlobally' packages/features/flags/features.repository.ts | head -30Repository: coderabbit-test/cal.com
Length of output: 954
🏁 Script executed:
#!/bin/bash
# Find AppFlags type definition
rg -n 'type AppFlags\|interface AppFlags' packages/features/ | head -5Repository: coderabbit-test/cal.com
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check OPT_IN_FEATURES definition
rg -n -B2 -A20 'const OPT_IN_FEATURES' packages/features/feature-management/config/feature-management.config.tsRepository: coderabbit-test/cal.com
Length of output: 899
🏁 Script executed:
#!/bin/bash
# Check if there's a type that defines the relationship between OPT_IN_FEATURES and AppFlags
rg -n 'AppFlags' packages/features/feature-management/config/feature-management.config.tsRepository: coderabbit-test/cal.com
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Find AppFlags type definition - search more broadly
rg -n 'export.*AppFlags|type AppFlags|interface AppFlags' packages/features/flags/Repository: coderabbit-test/cal.com
Length of output: 538
🏁 Script executed:
#!/bin/bash
# Search the entire features package for AppFlags
rg -n 'type AppFlags\s*=' packages/Repository: coderabbit-test/cal.com
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Look for where AppFlags might be defined
fd -e ts packages/features/ | xargs rg -l 'AppFlags' | head -10Repository: coderabbit-test/cal.com
Length of output: 976
🏁 Script executed:
#!/bin/bash
# Read the AppFlags type definition
cat -n packages/features/flags/config.ts | head -50Repository: coderabbit-test/cal.com
Length of output: 1511
🏁 Script executed:
#!/bin/bash
# Check if "bookings-v3" is in AppFlags
rg -A30 'export type AppFlags' packages/features/flags/config.tsRepository: coderabbit-test/cal.com
Length of output: 1032
Type assertion indicates loose typing that could cause future bugs.
The type assertion on line 138 bypasses type safety because getOptInFeatureSlugs() returns untyped string[], while checkIfFeatureIsEnabledGlobally() expects keyof AppFlags. Currently this works since "bookings-v3" is a valid AppFlags key, but if future opt-in features are added without updating AppFlags, the type mismatch won't be caught by TypeScript.
Consider refining OPT_IN_FEATURES to explicitly use keyof AppFlags for slug values, eliminating the need for the type assertion and providing compile-time guarantees.
| 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 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.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and locate key files
find . -type f -name "router.ts" | grep -E "feature-management|trpc" | head -20Repository: coderabbit-test/cal.com
Length of output: 120
🏁 Script executed:
# Look for authedProcedure definition
rg -l "authedProcedure" --type ts --type tsx | head -10Repository: coderabbit-test/cal.com
Length of output: 94
🏁 Script executed:
# Search for FeatureManagementService implementation
rg -l "FeatureManagementService" --type ts --type tsx | head -20Repository: coderabbit-test/cal.com
Length of output: 94
🏁 Script executed:
# Search for authedProcedure in ts files
rg "authedProcedure\s*=" --type ts | head -20Repository: coderabbit-test/cal.com
Length of output: 217
🏁 Script executed:
# Search for FeatureManagementService class or export
rg "class FeatureManagementService|export.*FeatureManagementService" --type ts | head -20Repository: coderabbit-test/cal.com
Length of output: 452
🏁 Script executed:
# Also look at the router file itself to see imports
head -50 ./packages/features/feature-management/trpc/router.tsRepository: coderabbit-test/cal.com
Length of output: 1661
🏁 Script executed:
# Get the FeatureManagementService file
cat ./packages/features/feature-management/services/FeatureManagementService.tsRepository: coderabbit-test/cal.com
Length of output: 5772
🏁 Script executed:
# Also check the setTeamFeatureEnabled and setOrganizationFeatureEnabled methods specifically
rg "setTeamFeatureEnabled|setOrganizationFeatureEnabled" ./packages/features/feature-management/services/FeatureManagementService.ts -A 20Repository: coderabbit-test/cal.com
Length of output: 1414
🏁 Script executed:
# Find FeaturesRepository implementation
rg -l "class FeaturesRepository" --type tsRepository: coderabbit-test/cal.com
Length of output: 181
🏁 Script executed:
# Get the setTeamFeatureEnabled method from FeaturesRepository
rg "setTeamFeatureEnabled" --type ts -A 15 ./packages/features/flags/Repository: coderabbit-test/cal.com
Length of output: 1994
🏁 Script executed:
# Search for PBAC or permission-related patterns in the codebase
rg "PBAC|permission|authorize|can.*team|can.*organization" --type ts ./packages/features/feature-management/ | head -20Repository: coderabbit-test/cal.com
Length of output: 389
🏁 Script executed:
# Get full FeaturesRepository file to understand setTeamFeatureEnabled completely
cat ./packages/features/flags/features.repository.ts | head -150Repository: coderabbit-test/cal.com
Length of output: 5113
🏁 Script executed:
# Get more of the FeaturesRepository file, particularly the setTeamFeatureEnabled method
sed -n '1,300p' ./packages/features/flags/features.repository.tsRepository: coderabbit-test/cal.com
Length of output: 10046
🏁 Script executed:
# Also check the router.ts comments context around the PBAC statements
sed -n '70,130p' ./packages/features/feature-management/trpc/router.tsRepository: coderabbit-test/cal.com
Length of output: 1674
🏁 Script executed:
# Let me verify the complete setTeamFeatureEnabled in FeaturesRepository to be thorough
rg "async setTeamFeatureEnabled" --type ts -A 25 ./packages/features/flags/features.repository.tsRepository: coderabbit-test/cal.com
Length of output: 659
Add PBAC permission checks before modifying team/organization features
The setTeamFeatureEnabled and setOrganizationFeatureEnabled procedures declare "Requires appropriate PBAC permissions" but do not enforce any. The call chain is:
- Router: uses
authedProcedure(authentication only viaisAuthedmiddleware) - Service: delegates directly to repository without permission validation
- Repository: performs direct
upsertonteamFeaturestable
An authenticated user can toggle features for any teamId or organizationId they don't own. Add explicit permission checks in the router or service layer to verify the user has permission to modify the specified team/organization before calling the service.
🤖 Prompt for AI Agents
In packages/features/feature-management/trpc/router.ts around lines 78–118 the
procedures claim "Requires appropriate PBAC permissions" but perform no
authorization; before calling the service methods, verify the caller has
permission to modify the given teamId/organizationId (either by calling an
existing PBAC helper on ctx, e.g. ctx.pbac.canModifyTeam(ctx.user.id,
input.teamId) / ctx.pbac.canModifyOrg(ctx.user.id, input.organizationId), or by
querying the database to confirm membership/role), and if the check fails throw
an unauthorized error (e.g. TRPCError with code "FORBIDDEN"); alternatively move
these checks into the service layer and call them there before performing the
upsert. Ensure the permission check happens synchronously prior to
service.setTeamFeatureEnabled and service.setOrganizationFeatureEnabled.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| category: "feature", | ||
| i18nKey: "pbac_action_read", | ||
| descriptionI18nKey: "pbac_desc_view_features", | ||
| }, |
There was a problem hiding this comment.
Suggestion: The read permission for feature flags is not scoped to organizations, unlike create and delete, so it will be exposed and considered valid at team scope where feature management is likely not supported, leading to inconsistent permission behavior across scopes. [logic error]
Severity Level: Minor
| }, | |
| scope: [Scope.Organization], |
Why it matters? ⭐
In the PR's current PermissionRegistry the Feature resource has Create and Delete actions explicitly scoped to Scope.Organization, but Read is missing a scope entry. That makes "feature.read" implicitly available at Team scope (or at least not excluded), which is inconsistent with the other CRUD actions and likely a logic bug: feature flag management is clearly treated as an org-level concern elsewhere in the same resource. Adding scope: [Scope.Organization] for read fixes a real permission/visibility inconsistency, not a mere stylistic change.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/features/pbac/domain/types/permission-registry.ts
**Line:** 771:771
**Comment:**
*Logic Error: The read permission for feature flags is not scoped to organizations, unlike create and delete, so it will be exposed and considered valid at team scope where feature management is likely not supported, leading to inconsistent permission behavior across scopes.
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.| category: "feature", | ||
| i18nKey: "pbac_action_update", | ||
| descriptionI18nKey: "pbac_desc_update_features", | ||
| dependsOn: ["feature.read"], |
There was a problem hiding this comment.
Suggestion: The update permission for feature flags is also missing an organization scope, so it will be treated as valid for team scope even though feature flag management is configured as an organization-level concern for the other actions on this resource, causing scope-inconsistent permission checks and UI. [logic error]
Severity Level: Minor
| dependsOn: ["feature.read"], | |
| scope: [Scope.Organization], |
Why it matters? ⭐
Similar to the read action, Update for Feature lacks an explicit scope while Create/Delete declare Scope.Organization. This results in inconsistent permission model where update could be considered valid at Team scope. Adding scope: [Scope.Organization] corrects the registry semantics and prevents accidental exposure of feature update capabilities at the wrong scope — a real logic/authorization fix.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/features/pbac/domain/types/permission-registry.ts
**Line:** 777:777
**Comment:**
*Logic Error: The update permission for feature flags is also missing an organization scope, so it will be treated as valid for team scope even though feature flag management is configured as an organization-level concern for the other actions on this resource, causing scope-inconsistent permission checks and UI.
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 stored = localStorage.getItem("cal_feature_banners_dismissed"); | ||
| setDismissedFeatures(stored ? JSON.parse(stored) : []); |
There was a problem hiding this comment.
Suggestion: The JSON.parse call on the value from localStorage is not guarded, so if the stored value is malformed (e.g. manually edited, corrupted, or written by an older version), this hook will throw at runtime and break rendering of any component using it; wrapping the access and parse in a try/catch and falling back to an empty list prevents the banner system from crashing in these cases. [possible bug]
Severity Level: Critical 🚨
| const stored = localStorage.getItem("cal_feature_banners_dismissed"); | |
| setDismissedFeatures(stored ? JSON.parse(stored) : []); | |
| try { | |
| const stored = localStorage.getItem("cal_feature_banners_dismissed"); | |
| setDismissedFeatures(stored ? JSON.parse(stored) : []); | |
| } catch { | |
| setDismissedFeatures([]); | |
| } |
Why it matters? ⭐
The existing code calls JSON.parse on a localStorage value without guarding against malformed JSON.
If the stored value is corrupted or manually edited, JSON.parse will throw and that exception will bubble up:
breaking rendering for components using this hook. Wrapping the read/parse in try/catch and falling back
to an empty array is a small, low-risk defensive change that prevents a runtime crash and preserves UX.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/features/feature-management/hooks/useFeatureOptInBanner.ts
**Line:** 44:45
**Comment:**
*Possible Bug: The `JSON.parse` call on the value from `localStorage` is not guarded, so if the stored value is malformed (e.g. manually edited, corrupted, or written by an older version), this hook will throw at runtime and break rendering of any component using it; wrapping the access and parse in a try/catch and falling back to an empty list prevents the banner system from crashing in these cases.
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.| import authedProcedure from "@calcom/trpc/server/procedures/authedProcedure"; | ||
| import { router } from "@calcom/trpc/server/trpc"; | ||
|
|
||
| import { FeatureManagementService } from "../services/FeatureManagementService"; | ||
|
|
||
| const getFeatureManagementService = (prisma: PrismaClient) => { | ||
| const featuresRepository = new FeaturesRepository(prisma); | ||
| return new FeatureManagementService(featuresRepository); | ||
| }; | ||
|
|
||
| export const featureManagementRouter = router({ | ||
| /** | ||
| * List all features for the current user with their enabled status. | ||
| */ | ||
| listForUser: authedProcedure.query(async ({ ctx }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
| return service.listFeaturesForUser(ctx.user.id); | ||
| }), | ||
|
|
||
| /** | ||
| * List all features for a team with their enabled status. | ||
| */ | ||
| listForTeam: authedProcedure | ||
| .input( | ||
| z.object({ | ||
| teamId: z.number(), | ||
| }) | ||
| ) | ||
| .query(async ({ ctx, input }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
| return service.listFeaturesForTeam(input.teamId); | ||
| }), | ||
|
|
||
| /** | ||
| * List all features for an organization with their enabled status. | ||
| */ | ||
| listForOrganization: authedProcedure | ||
| .input( | ||
| z.object({ | ||
| organizationId: z.number(), | ||
| }) | ||
| ) | ||
| .query(async ({ ctx, input }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
| return service.listFeaturesForOrganization(input.organizationId); | ||
| }), | ||
|
|
||
| /** | ||
| * Set the enabled status of a feature for the current user. | ||
| * Users can always control their own features - no PBAC check needed. | ||
| */ | ||
| 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 }; | ||
| }), | ||
|
|
||
| /** | ||
| * 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 an organization. | ||
| * Requires appropriate PBAC permissions. | ||
| */ | ||
| setOrganizationFeatureEnabled: authedProcedure |
There was a problem hiding this comment.
Suggestion: Team and organization feature endpoints currently use the generic authenticated procedure, meaning any logged-in user can list or mutate feature flags for arbitrary teams or organizations by ID, which violates the comments' PBAC requirement and creates an authorization vulnerability; switch these routes to the appropriate admin/org-admin procedures to enforce proper permission checks. [security]
Severity Level: Critical 🚨
| import authedProcedure from "@calcom/trpc/server/procedures/authedProcedure"; | |
| import { router } from "@calcom/trpc/server/trpc"; | |
| import { FeatureManagementService } from "../services/FeatureManagementService"; | |
| const getFeatureManagementService = (prisma: PrismaClient) => { | |
| const featuresRepository = new FeaturesRepository(prisma); | |
| return new FeatureManagementService(featuresRepository); | |
| }; | |
| export const featureManagementRouter = router({ | |
| /** | |
| * List all features for the current user with their enabled status. | |
| */ | |
| listForUser: authedProcedure.query(async ({ ctx }) => { | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForUser(ctx.user.id); | |
| }), | |
| /** | |
| * List all features for a team with their enabled status. | |
| */ | |
| listForTeam: authedProcedure | |
| .input( | |
| z.object({ | |
| teamId: z.number(), | |
| }) | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForTeam(input.teamId); | |
| }), | |
| /** | |
| * List all features for an organization with their enabled status. | |
| */ | |
| listForOrganization: authedProcedure | |
| .input( | |
| z.object({ | |
| organizationId: z.number(), | |
| }) | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForOrganization(input.organizationId); | |
| }), | |
| /** | |
| * Set the enabled status of a feature for the current user. | |
| * Users can always control their own features - no PBAC check needed. | |
| */ | |
| 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 }; | |
| }), | |
| /** | |
| * 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 an organization. | |
| * Requires appropriate PBAC permissions. | |
| */ | |
| setOrganizationFeatureEnabled: authedProcedure | |
| import authedProcedure, { | |
| authedAdminProcedure, | |
| authedOrgAdminProcedure, | |
| } from "@calcom/trpc/server/procedures/authedProcedure"; | |
| import { router } from "@calcom/trpc/server/trpc"; | |
| import { FeatureManagementService } from "../services/FeatureManagementService"; | |
| const getFeatureManagementService = (prisma: PrismaClient) => { | |
| const featuresRepository = new FeaturesRepository(prisma); | |
| return new FeatureManagementService(featuresRepository); | |
| }; | |
| export const featureManagementRouter = router({ | |
| /** | |
| * List all features for the current user with their enabled status. | |
| */ | |
| listForUser: authedProcedure.query(async ({ ctx }) => { | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForUser(ctx.user.id); | |
| }), | |
| /** | |
| * List all features for a team with their enabled status. | |
| * Requires appropriate PBAC permissions. | |
| */ | |
| listForTeam: authedAdminProcedure | |
| .input( | |
| z.object({ | |
| teamId: z.number(), | |
| }) | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForTeam(input.teamId); | |
| }), | |
| /** | |
| * List all features for an organization with their enabled status. | |
| * Requires appropriate PBAC permissions. | |
| */ | |
| listForOrganization: authedOrgAdminProcedure | |
| .input( | |
| z.object({ | |
| organizationId: z.number(), | |
| }) | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForOrganization(input.organizationId); | |
| }), | |
| /** | |
| * Set the enabled status of a feature for the current user. | |
| * Users can always control their own features - no PBAC check needed. | |
| */ | |
| 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 }; | |
| }), | |
| /** | |
| * Set the enabled status of a feature for a team. | |
| * Requires appropriate PBAC permissions. | |
| */ | |
| setTeamFeatureEnabled: authedAdminProcedure | |
| .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 an organization. | |
| * Requires appropriate PBAC permissions. | |
| */ | |
| setOrganizationFeatureEnabled: authedOrgAdminProcedure |
Why it matters? ⭐
The PR currently uses authedProcedure for team- and organization-scoped endpoints while the comments explicitly say PBAC checks are required. That means any authenticated user can call those routes for arbitrary team/org IDs — a real authorization surface area issue. Replacing those handlers with stricter procedures (e.g., admin/org-admin wrappers that perform PBAC) addresses a genuine security/authorization bug rather than a cosmetic preference.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/features/feature-management/trpc/router.ts
**Line:** 5:101
**Comment:**
*Security: Team and organization feature endpoints currently use the generic authenticated procedure, meaning any logged-in user can list or mutate feature flags for arbitrary teams or organizations by ID, which violates the comments' PBAC requirement and creates an authorization vulnerability; switch these routes to the appropriate admin/org-admin procedures to enforce proper permission checks.
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.![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 new settings pages for users, organizations, and teams, along with a banner for eligible features. It integrates new TRPC endpoints and extends the FeaturesRepository to manage feature states for different entities. New translation keys have been added to support the new UI elements. The overall structure is well-organized, and the use of TRPC, PBAC, and localStorage for banner dismissal is implemented correctly.
| <SettingsToggle | ||
| key={feature.slug} | ||
| toggleSwitchAtTheEnd={true} | ||
| title={feature.slug} |
There was a problem hiding this comment.
The title prop for SettingsToggle currently uses feature.slug. While slug is a unique identifier, it's often a technical string and might not be user-friendly or translatable for display purposes. For consistency with FeatureOptInBanner, which uses feature.titleI18nKey, consider introducing an i18n key for the feature title in the Feature model or mapping the slug to a more descriptive, translatable title.
| key={feature.slug} | ||
| toggleSwitchAtTheEnd={true} | ||
| title={feature.slug} | ||
| description={feature.description || t("no_description_available")} |
There was a problem hiding this comment.
The description prop for SettingsToggle uses feature.description. If feature.description is intended to be a user-facing string, it should ideally be an i18n key to support localization. Currently, it appears to be a direct string from the database, which might not be translatable. For better internationalization, consider using an i18n key for feature descriptions, similar to feature.descriptionI18nKey used in EligibleOptInFeature for the opt-in banner.
| <SettingsToggle | ||
| key={feature.slug} | ||
| toggleSwitchAtTheEnd={true} | ||
| title={feature.slug} |
There was a problem hiding this comment.
Similar to features-view.tsx, using feature.slug directly as the title for SettingsToggle might not be user-friendly or translatable. It's recommended to use an i18n key for the feature title to ensure consistency and proper localization across the application.
| key={feature.slug} | ||
| toggleSwitchAtTheEnd={true} | ||
| title={feature.slug} | ||
| description={feature.description || t("no_description_available")} |
There was a problem hiding this comment.
The description prop for SettingsToggle uses feature.description. For improved localization, consider using an i18n key for feature descriptions. This would align with the approach taken in FeatureOptInBanner and provide a more consistent user experience.
| <SettingsToggle | ||
| key={feature.slug} | ||
| toggleSwitchAtTheEnd={true} | ||
| title={feature.slug} |
There was a problem hiding this comment.
Using feature.slug directly as the title for SettingsToggle can lead to a less user-friendly interface, as slugs are typically technical identifiers. Please consider mapping this to a translatable i18n key for better user experience and consistency with other feature display components.
| key={feature.slug} | ||
| toggleSwitchAtTheEnd={true} | ||
| title={feature.slug} | ||
| description={feature.description || t("no_description_available")} |
There was a problem hiding this comment.
The description prop for SettingsToggle uses feature.description. To ensure that feature descriptions are localizable, it's best practice to use an i18n key rather than a direct string from the database. This would provide a more consistent and internationalized experience.
| slug: feature.slug, | ||
| enabled: !!userFeature, // Row exists = enabled | ||
| globallyEnabled: feature.enabled, | ||
| description: feature.description, | ||
| type: feature.type, | ||
| }; |
There was a problem hiding this comment.
The FeatureWithStatus interface includes slug and description as direct strings. However, the EligibleOptInFeature interface (used for the banner) uses titleI18nKey and descriptionI18nKey for translatable strings. This creates an inconsistency in how feature titles and descriptions are handled across the application's UI components. For a unified and localizable user experience, consider either adding titleI18nKey and descriptionI18nKey to the Feature Prisma model or providing a mapping in the service to derive these i18n keys for FeatureWithStatus.
| const teamFeature = teamFeatures.find((tf) => tf.feature.slug === feature.slug); | ||
| return { | ||
| slug: feature.slug, | ||
| enabled: !!teamFeature, // Row exists = enabled | ||
| globallyEnabled: feature.enabled, | ||
| description: feature.description, |
There was a problem hiding this comment.
The FeatureWithStatus interface includes slug and description as direct strings. However, the EligibleOptInFeature interface (used for the banner) uses titleI18nKey and descriptionI18nKey for translatable strings. This creates an inconsistency in how feature titles and descriptions are handled across the application's UI components. For a unified and localizable user experience, consider either adding titleI18nKey and descriptionI18nKey to the Feature Prisma model or providing a mapping in the service to derive these i18n keys for FeatureWithStatus.
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
|
@refacto-visz |
1 similar comment
|
@refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
@refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Feature Management SystemPR Confidence Score: 🟥 3 / 5👍 Well Done
📁 Selected files for review (18)
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| 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 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.
Missing Backend Permission Checks
The setTeamFeatureEnabled and setOrganizationFeatureEnabled tRPC mutations only use authedProcedure, which verifies user authentication but does not check for authorization. This allows any authenticated user to enable or disable features for any team or organization if they know the ID, bypassing role-based access control. The backend must enforce that the user has the appropriate role (e.g., ADMIN or OWNER) for the target team/organization before allowing the modification, as noted in the comments but not implemented.
setTeamFeatureEnabled: authedProcedure
.input(
z.object({
teamId: z.number(),
featureSlug: z.string(),
enabled: z.boolean(),
})
)
.mutation(async ({ ctx, input }) => {
// Check if user has permission to manage team features
const hasPermission = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
teamId: input.teamId,
role: { in: ['ADMIN', 'OWNER'] }
}
});
if (!hasPermission) {
throw new Error('Insufficient permissions to manage 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 }) => {
// Check if user has permission to manage organization features
const hasPermission = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
team: { parentId: input.organizationId },
role: { in: ['ADMIN', 'OWNER'] }
}
});
if (!hasPermission) {
throw new Error('Insufficient permissions to manage organization features');
}
const service = getFeatureManagementService(ctx.prisma);
await service.setOrganizationFeatureEnabled(
input.organizationId,
input.featureSlug,
input.enabled,
`user:${ctx.user.id}`
);
return { success: true };
}),
Commitable Suggestion
| 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 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 }; | |
| }), | |
| setTeamFeatureEnabled: authedProcedure | |
| .input( | |
| z.object({ | |
| teamId: z.number(), | |
| featureSlug: z.string(), | |
| enabled: z.boolean(), | |
| }) | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| // Check if user has permission to manage team features | |
| const hasPermission = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| teamId: input.teamId, | |
| role: { in: ['ADMIN', 'OWNER'] } | |
| } | |
| }); | |
| if (!hasPermission) { | |
| throw new Error('Insufficient permissions to manage 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 }) => { | |
| // Check if user has permission to manage organization features | |
| const hasPermission = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| team: { parentId: input.organizationId }, | |
| role: { in: ['ADMIN', 'OWNER'] } | |
| } | |
| }); | |
| if (!hasPermission) { | |
| throw new Error('Insufficient permissions to manage organization features'); | |
| } | |
| const service = getFeatureManagementService(ctx.prisma); | |
| await service.setOrganizationFeatureEnabled( | |
| input.organizationId, | |
| input.featureSlug, | |
| input.enabled, | |
| `user:${ctx.user.id}` | |
| ); | |
| return { success: true }; | |
| }), |
Standards
- Business-Rule-Input-Validation
- Logic-Verification-Data-Integrity
| 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.
Unhandled JSON Parse Exception
The code parses a JSON string from localStorage without error handling. If the stored data is corrupted or invalid JSON, JSON.parse will throw an exception. An unhandled exception within a useEffect hook can crash the component and potentially the entire application if no error boundary is present above it.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- SRE-Error-Handling
| 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] | ||
| ); | ||
| if (!isGloballyEnabled) continue; | ||
|
|
||
| eligibleFeatures.push({ | ||
| slug: config.slug, | ||
| titleI18nKey: config.titleI18nKey, | ||
| descriptionI18nKey: config.descriptionI18nKey, | ||
| learnMoreUrl: config.learnMoreUrl, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Sequential Awaits In Loop
The method sequentially awaits two database queries inside a for loop for each feature slug. This pattern leads to poor performance and scalability, as the total execution time increases linearly with the number of opt-in features. This is an N+1 query problem that will cause significant latency as more features are added.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Batch-Processing
- Algorithmic-Complexity-Linear-Optimization
| if (!service.isFeatureInOptInAllowlist(input.featureSlug)) { | ||
| throw new Error("Feature is not available for opt-in"); |
There was a problem hiding this comment.
Missing Feature Slug Validation
Feature slug validation only checks allowlist membership without input sanitization. Malicious feature slugs could be processed through the system before validation occurs. This could lead to injection attacks or system manipulation through crafted feature identifiers.
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Feature Management SystemPR Confidence Score: 🟥 2 / 5👍 Well Done
📁 Selected files for review (18)
📝 Additional Comments
|
| 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.
Missing Authorization Validation
Team and organization feature modification endpoints lack permission validation allowing any authenticated user to modify features for any team or organization. Attackers could enable dangerous features or disable security features across organizations they don't belong to. This enables privilege escalation and unauthorized feature manipulation across tenant boundaries.
setTeamFeatureEnabled: authedProcedure
.input(
z.object({
teamId: z.number(),
featureSlug: z.string(),
enabled: z.boolean(),
})
)
.mutation(async ({ ctx, input }) => {
// Verify user has admin permissions for the team
const membership = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
teamId: input.teamId,
role: { in: ['ADMIN', 'OWNER'] }
}
});
if (!membership) {
throw new Error('Unauthorized: insufficient permissions for team');
}
const service = getFeatureManagementService(ctx.prisma);
await service.setTeamFeatureEnabled(
input.teamId,
input.featureSlug,
input.enabled,
`user:${ctx.user.id}`
);
return { success: true };
}),
setOrganizationFeatureEnabled: authedProcedure
.input(
z.object({
organizationId: z.number(),
featureSlug: z.string(),
enabled: z.boolean(),
})
)
.mutation(async ({ ctx, input }) => {
// Verify user has admin permissions for the organization
const membership = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
teamId: input.organizationId,
role: { in: ['ADMIN', 'OWNER'] }
}
});
if (!membership) {
throw new Error('Unauthorized: insufficient permissions for organization');
}
const service = getFeatureManagementService(ctx.prisma);
await service.setOrganizationFeatureEnabled(
input.organizationId,
input.featureSlug,
input.enabled,
`user:${ctx.user.id}`
);
return { success: true };
}),
Commitable Suggestion
| 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 }; | |
| }), | |
| setTeamFeatureEnabled: authedProcedure | |
| .input( | |
| z.object({ | |
| teamId: z.number(), | |
| featureSlug: z.string(), | |
| enabled: z.boolean(), | |
| }) | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| // Verify user has admin permissions for the team | |
| const membership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| teamId: input.teamId, | |
| role: { in: ['ADMIN', 'OWNER'] } | |
| } | |
| }); | |
| if (!membership) { | |
| throw new Error('Unauthorized: insufficient permissions for team'); | |
| } | |
| const service = getFeatureManagementService(ctx.prisma); | |
| await service.setTeamFeatureEnabled( | |
| input.teamId, | |
| input.featureSlug, | |
| input.enabled, | |
| `user:${ctx.user.id}` | |
| ); | |
| return { success: true }; | |
| }), | |
| setOrganizationFeatureEnabled: authedProcedure | |
| .input( | |
| z.object({ | |
| organizationId: z.number(), | |
| featureSlug: z.string(), | |
| enabled: z.boolean(), | |
| }) | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| // Verify user has admin permissions for the organization | |
| const membership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| teamId: input.organizationId, | |
| role: { in: ['ADMIN', 'OWNER'] } | |
| } | |
| }); | |
| if (!membership) { | |
| throw new Error('Unauthorized: insufficient permissions for organization'); | |
| } | |
| 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] | ||
| ); | ||
| if (!isGloballyEnabled) continue; |
There was a problem hiding this comment.
N+1 Query Anti-Pattern
The for loop iterates through optInSlugs and executes two database queries inside the loop: getUserFeature and checkIfFeatureIsEnabledGlobally. This creates an N+1 query problem where N is the number of opt-in features. As this list grows, the number of database calls will increase linearly, causing significant performance degradation and database load.
// 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;
// Row exists = user has already opted in
if (userFeatureMap.has(slug)) {
continue;
}
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] | |
| ); | |
| if (!isGloballyEnabled) continue; | |
| // 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; | |
| // Row exists = user has already opted in | |
| if (userFeatureMap.has(slug)) { | |
| continue; | |
| } | |
| 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 Injection
Feature slug parameter accepts arbitrary strings without validation against allowed features list. Malicious users could enable non-existent or restricted features by injecting arbitrary feature identifiers. This could bypass feature access controls and enable unauthorized functionality or create database pollution with invalid feature references.
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
| async setUserFeatureEnabled( | ||
| userId: number, | ||
| featureSlug: string, | ||
| enabled: boolean, | ||
| assignedBy: string | ||
| ): Promise<void> { | ||
| await this.featuresRepository.setUserFeatureEnabled(userId, featureSlug, enabled, assignedBy); | ||
| } |
There was a problem hiding this comment.
Missing Feature Existence Validation
Service methods directly pass feature slugs to repository without validating feature exists or is globally enabled. Attackers could create arbitrary feature assignments for non-existent features leading to database inconsistency. This bypasses feature governance and could enable unauthorized feature access through invalid feature references.
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
| const FeaturesView = () => { | ||
| const { t } = useLocale(); | ||
| const utils = trpc.useUtils(); | ||
|
|
||
| const { data: features, isLoading } = trpc.viewer.featureManagement.listForUser.useQuery(); | ||
|
|
||
| const setFeatureEnabledMutation = trpc.viewer.featureManagement.setUserFeatureEnabled.useMutation({ | ||
| onSuccess: () => { | ||
| utils.viewer.featureManagement.listForUser.invalidate(); | ||
| showToast(t("settings_updated_successfully"), "success"); | ||
| }, | ||
| onError: () => { | ||
| showToast(t("error_updating_settings"), "error"); | ||
| }, | ||
| }); | ||
|
|
||
| if (isLoading) { | ||
| return ( | ||
| <SettingsHeader | ||
| title={t("features")} | ||
| description={t("features_description")} | ||
| borderInShellHeader={true}> | ||
| <SkeletonLoader /> | ||
| </SettingsHeader> | ||
| ); | ||
| } | ||
|
|
||
| const userControlledFeatures = features?.filter((f) => f.globallyEnabled) ?? []; | ||
|
|
||
| return ( | ||
| <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} | ||
| toggleSwitchAtTheEnd={true} | ||
| title={feature.slug} | ||
| description={feature.description || t("no_description_available")} | ||
| disabled={setFeatureEnabledMutation.isPending} | ||
| checked={feature.enabled} | ||
| onCheckedChange={(checked) => { | ||
| setFeatureEnabledMutation.mutate({ | ||
| featureSlug: feature.slug, | ||
| enabled: checked, | ||
| }); | ||
| }} | ||
| /> | ||
| ))} | ||
| </div> | ||
| )} | ||
| </div> | ||
| </SettingsHeader> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Duplicated UI View Logic
The three new view components contain nearly identical logic for fetching data, handling loading states, and rendering feature toggles. This duplication violates the DRY principle and increases future maintenance effort, as any change will need to be applied in three places. A single, reusable component should be created that can be parameterized with the appropriate tRPC hooks and entity ID to handle all three use cases.
Standards
- Clean-Code-DRY
|
@refacto-visz |
| 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 Team Membership
Team feature listing endpoint allows any authenticated user to view feature configurations for any team without membership validation. Attackers could enumerate team feature settings and discover enabled security features across organizations. This constitutes information disclosure that could aid in targeted attacks against specific teams.
listForTeam: authedProcedure
.input(
z.object({
teamId: z.number(),
})
)
.query(async ({ ctx, input }) => {
// Validate team membership
const membership = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
teamId: input.teamId,
accepted: true
}
});
if (!membership) {
throw new Error('Access denied: Not a member of this team');
}
const service = getFeatureManagementService(ctx.prisma);
return service.listFeaturesForTeam(input.teamId);
}),
Commitable Suggestion
| 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 }) => { | |
| // Validate team membership | |
| const membership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| teamId: input.teamId, | |
| accepted: true | |
| } | |
| }); | |
| if (!membership) { | |
| throw new Error('Access denied: Not a member of this team'); | |
| } | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForTeam(input.teamId); | |
| }), |
Standards
- CWE-862
- OWASP-A01
- NIST-SSDF-PW.1
| 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.
Organization Access Control
Organization feature listing endpoint lacks organization membership validation allowing unauthorized users to view organizational feature configurations. Attackers could discover security feature settings across multiple organizations revealing security posture information. This enables reconnaissance for targeted attacks and potential compliance violations.
listForOrganization: authedProcedure
.input(
z.object({
organizationId: z.number(),
})
)
.query(async ({ ctx, input }) => {
// Validate organization membership
const membership = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
teamId: input.organizationId,
accepted: true
}
});
if (!membership) {
throw new Error('Access denied: Not a member of this organization');
}
const service = getFeatureManagementService(ctx.prisma);
return service.listFeaturesForOrganization(input.organizationId);
}),
Commitable Suggestion
| listForOrganization: authedProcedure | |
| .input( | |
| z.object({ | |
| organizationId: z.number(), | |
| }) | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForOrganization(input.organizationId); | |
| }), | |
| listForOrganization: authedProcedure | |
| .input( | |
| z.object({ | |
| organizationId: z.number(), | |
| }) | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| // Validate organization membership | |
| const membership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| teamId: input.organizationId, | |
| accepted: true | |
| } | |
| }); | |
| if (!membership) { | |
| throw new Error('Access denied: Not a member of this organization'); | |
| } | |
| const service = getFeatureManagementService(ctx.prisma); | |
| return service.listFeaturesForOrganization(input.organizationId); | |
| }), |
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] | ||
| ); | ||
| if (!isGloballyEnabled) continue; | ||
|
|
||
| eligibleFeatures.push({ | ||
| slug: config.slug, | ||
| titleI18nKey: config.titleI18nKey, | ||
| descriptionI18nKey: config.descriptionI18nKey, | ||
| learnMoreUrl: config.learnMoreUrl, | ||
| }); | ||
| } |
There was a problem hiding this comment.
N+1 Query Pattern
The getEligibleOptInFeatures method iterates through optInSlugs and executes database queries (getUserFeature and checkIfFeatureIsEnabledGlobally) inside the loop. This creates an N+1 query problem, where N is the number of opt-in features. As this list grows, the number of database queries will increase linearly, causing significant performance degradation and increased database load.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Batch-Processing
- Algorithmic-Complexity-Linear-Optimization
| const Page = async () => { | ||
| return <TeamFeaturesView />; | ||
| }; |
There was a problem hiding this comment.
Missing Permission Check
The new Team Features page does not perform a specific permission check for the Feature resource, unlike the corresponding Organization Features page which explicitly checks for canRead permissions. This inconsistency introduces a potential security vulnerability, as it might allow users to access feature management settings for a team without the required authorization. An explicit PBAC check should be added to ensure consistent and secure access control.
Standards
- Business-Rule-Input-Validation
| useEffect(() => { | ||
| if (typeof window !== "undefined") { | ||
| const stored = localStorage.getItem("cal_feature_banners_dismissed"); | ||
| setDismissedFeatures(stored ? JSON.parse(stored) : []); | ||
| } | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (!featureParam || !eligibleFeatures) { | ||
| setFeatureToShow(null); | ||
| return; | ||
| } | ||
|
|
||
| if (dismissedFeatures.includes(featureParam)) { | ||
| setFeatureToShow(null); | ||
| return; | ||
| } | ||
|
|
||
| const feature = eligibleFeatures.find((f) => f.slug === featureParam); | ||
| setFeatureToShow(feature || null); | ||
| }, [featureParam, eligibleFeatures, dismissedFeatures]); |
There was a problem hiding this comment.
Race Condition Causes Flicker
The hook uses two separate useEffect calls, one to read dismissed banner states from localStorage and another to decide whether to show a banner. This creates a race condition where the banner might be shown for a feature that is already dismissed, causing a UI flicker. The effect checking for eligible features can complete and render the banner before the effect reading localStorage has updated the component's state.
Standards
- Logic-Verification-Data-Flow
| optInToFeature: authedProcedure | ||
| .input( | ||
| z.object({ | ||
| featureSlug: z.string(), | ||
| }) | ||
| ) | ||
| .mutation(async ({ ctx, input }) => { | ||
| const service = getFeatureManagementService(ctx.prisma); | ||
|
|
||
| if (!service.isFeatureInOptInAllowlist(input.featureSlug)) { | ||
| throw new Error("Feature is not available for opt-in"); | ||
| } | ||
|
|
||
| await service.setUserFeatureEnabled(ctx.user.id, input.featureSlug, true, `user:${ctx.user.id}`); | ||
| return { success: true }; |
There was a problem hiding this comment.
Feature Slug Injection
Feature slug parameter accepts arbitrary strings without validation before database operations. While allowlist validation exists, malformed slugs could cause database errors or injection attacks if slug is used in dynamic queries. Input validation should sanitize feature slug format to prevent potential injection vectors through malicious slug values.
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
| const FeaturesView = () => { | ||
| const { t } = useLocale(); | ||
| const utils = trpc.useUtils(); | ||
|
|
||
| const { data: features, isLoading } = trpc.viewer.featureManagement.listForUser.useQuery(); | ||
|
|
||
| const setFeatureEnabledMutation = trpc.viewer.featureManagement.setUserFeatureEnabled.useMutation({ | ||
| onSuccess: () => { | ||
| utils.viewer.featureManagement.listForUser.invalidate(); | ||
| showToast(t("settings_updated_successfully"), "success"); | ||
| }, | ||
| onError: () => { | ||
| showToast(t("error_updating_settings"), "error"); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Duplicated Feature Management UI
The FeaturesView component is nearly identical to OrganizationFeaturesView and TeamFeaturesView. All three components share the same structure, skeleton loader, and rendering logic, leading to significant code duplication. This violates the DRY principle and increases maintenance overhead, as future UI changes will need to be replicated in three places. A single generic component should be created that accepts the context (e.g., 'user', 'team', or 'organization') and an optional ID as props to dynamically use the appropriate tRPC procedures.
Standards
- Clean-Code-DRY
- Maintainability-Quality-Duplication
|
PR already reviewed at the latest commit: 3c54b1f. |
|
@refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Feature Management SystemPR Confidence Score: 🟥 2 / 5👍 Well Done
📁 Selected files for review (18)
📝 Additional Comments
|
| 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.
Missing Team Authorization
Team and organization feature management endpoints lack authorization checks to verify user permissions for the specified team/organization. Attackers could manipulate features for teams they don't belong to by providing arbitrary teamId/organizationId values. This enables privilege escalation and unauthorized feature control across organizational boundaries.
setTeamFeatureEnabled: authedProcedure
.input(
z.object({
teamId: z.number(),
featureSlug: z.string(),
enabled: z.boolean(),
})
)
.mutation(async ({ ctx, input }) => {
// Verify user has admin permissions for the team
const teamMembership = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
teamId: input.teamId,
role: { in: ['ADMIN', 'OWNER'] }
}
});
if (!teamMembership) {
throw new Error('Unauthorized: insufficient permissions for team feature management');
}
const service = getFeatureManagementService(ctx.prisma);
await service.setTeamFeatureEnabled(
input.teamId,
input.featureSlug,
input.enabled,
`user:${ctx.user.id}`
);
return { success: true };
}),
setOrganizationFeatureEnabled: authedProcedure
.input(
z.object({
organizationId: z.number(),
featureSlug: z.string(),
enabled: z.boolean(),
})
)
.mutation(async ({ ctx, input }) => {
// Verify user has admin permissions for the organization
const orgMembership = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
team: {
isOrganization: true,
id: input.organizationId
},
role: { in: ['ADMIN', 'OWNER'] }
}
});
if (!orgMembership) {
throw new Error('Unauthorized: insufficient permissions for organization feature management');
}
const service = getFeatureManagementService(ctx.prisma);
await service.setOrganizationFeatureEnabled(
input.organizationId,
input.featureSlug,
input.enabled,
`user:${ctx.user.id}`
);
return { success: true };
}),
Commitable Suggestion
| 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 }; | |
| }), | |
| setTeamFeatureEnabled: authedProcedure | |
| .input( | |
| z.object({ | |
| teamId: z.number(), | |
| featureSlug: z.string(), | |
| enabled: z.boolean(), | |
| }) | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| // Verify user has admin permissions for the team | |
| const teamMembership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| teamId: input.teamId, | |
| role: { in: ['ADMIN', 'OWNER'] } | |
| } | |
| }); | |
| if (!teamMembership) { | |
| throw new Error('Unauthorized: insufficient permissions for team feature management'); | |
| } | |
| const service = getFeatureManagementService(ctx.prisma); | |
| await service.setTeamFeatureEnabled( | |
| input.teamId, | |
| input.featureSlug, | |
| input.enabled, | |
| `user:${ctx.user.id}` | |
| ); | |
| return { success: true }; | |
| }), | |
| setOrganizationFeatureEnabled: authedProcedure | |
| .input( | |
| z.object({ | |
| organizationId: z.number(), | |
| featureSlug: z.string(), | |
| enabled: z.boolean(), | |
| }) | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| // Verify user has admin permissions for the organization | |
| const orgMembership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| team: { | |
| isOrganization: true, | |
| id: input.organizationId | |
| }, | |
| role: { in: ['ADMIN', 'OWNER'] } | |
| } | |
| }); | |
| if (!orgMembership) { | |
| throw new Error('Unauthorized: insufficient permissions for organization feature management'); | |
| } | |
| 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
| return allFeatures.map((feature) => { | ||
| // Row existence = feature enabled for user | ||
| const userFeature = userFeatures.find((uf) => uf.feature.slug === feature.slug); | ||
| return { | ||
| slug: feature.slug, | ||
| enabled: !!userFeature, // Row exists = enabled | ||
| globallyEnabled: feature.enabled, | ||
| description: feature.description, | ||
| type: feature.type, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Inefficient Feature Status Lookup
The code iterates through all system features and performs a find operation on the userFeatures array for each one. This results in O(M*N) complexity, which will degrade performance as the number of features or user-enabled features grows. This can be optimized to O(M+N) by converting the userFeatures list into a Set for O(1) lookups.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Algorithmic-Complexity-Linear-Optimization
| 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] | ||
| ); | ||
| if (!isGloballyEnabled) continue; | ||
|
|
There was a problem hiding this comment.
N+1 Query in Loop
The getEligibleOptInFeatures method executes two database queries (getUserFeature and checkIfFeatureIsEnabledGlobally) inside a for loop. This creates an N+1 query anti-pattern, where N is the number of opt-in features. As this list grows, the number of database round-trips will increase linearly, degrading API response time.
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 input accepts arbitrary strings without validation against existing features or allowlists. Attackers could create database entries for non-existent features or bypass intended feature restrictions. This could lead to database pollution and potential security control bypass if feature validation logic relies on database state.
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
| const FeaturesView = () => { | ||
| const { t } = useLocale(); | ||
| const utils = trpc.useUtils(); | ||
|
|
||
| const { data: features, isLoading } = trpc.viewer.featureManagement.listForUser.useQuery(); | ||
|
|
||
| const setFeatureEnabledMutation = trpc.viewer.featureManagement.setUserFeatureEnabled.useMutation({ | ||
| onSuccess: () => { | ||
| utils.viewer.featureManagement.listForUser.invalidate(); | ||
| showToast(t("settings_updated_successfully"), "success"); | ||
| }, | ||
| onError: () => { | ||
| showToast(t("error_updating_settings"), "error"); | ||
| }, | ||
| }); | ||
|
|
||
| if (isLoading) { | ||
| return ( | ||
| <SettingsHeader | ||
| title={t("features")} | ||
| description={t("features_description")} | ||
| borderInShellHeader={true}> | ||
| <SkeletonLoader /> | ||
| </SettingsHeader> | ||
| ); | ||
| } | ||
|
|
||
| const userControlledFeatures = features?.filter((f) => f.globallyEnabled) ?? []; | ||
|
|
||
| return ( | ||
| <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} | ||
| toggleSwitchAtTheEnd={true} | ||
| title={feature.slug} | ||
| description={feature.description || t("no_description_available")} | ||
| disabled={setFeatureEnabledMutation.isPending} | ||
| checked={feature.enabled} | ||
| onCheckedChange={(checked) => { | ||
| setFeatureEnabledMutation.mutate({ | ||
| featureSlug: feature.slug, | ||
| enabled: checked, | ||
| }); | ||
| }} | ||
| /> | ||
| ))} | ||
| </div> | ||
| )} | ||
| </div> | ||
| </SettingsHeader> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Duplicated UI View Logic
The three new feature view components (FeaturesView, OrganizationFeaturesView, TeamFeaturesView) share nearly identical structure and logic, including skeleton loaders, headers, and toggle rendering. This duplication violates the DRY principle, increasing maintenance effort. A single, reusable component should be created to encapsulate the common UI and accept entity-specific data-fetching logic (e.g., tRPC hooks) and identifiers as props.
Standards
- Clean-Code-DRY
- Maintainability-Quality-Reusability
|
@refacto-visz |
|
PR already reviewed at the latest commit: 3c54b1f. |
|
@refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Feature Management SystemPR Confidence Score: 🟥 2 / 5👍 Well Done
📁 Selected files for review (18)
📝 Additional Comments
|
| * Set the enabled status of a feature for a team. | ||
| * Requires appropriate PBAC permissions. | ||
| */ | ||
| setTeamFeatureEnabled: authedProcedure |
There was a problem hiding this comment.
Missing Team Authorization
Team feature modification endpoint lacks authorization check to verify user has permission to modify team features. Attackers could modify features for teams they don't belong to or lack administrative rights for. This enables privilege escalation and unauthorized team configuration changes.
setTeamFeatureEnabled: authedProcedure
.input(
z.object({
teamId: z.number(),
featureSlug: z.string(),
enabled: z.boolean(),
})
)
.mutation(async ({ ctx, input }) => {
// Verify user has admin permissions 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('Unauthorized: admin privileges required for team feature management');
}
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 }) => { | |
| // Verify user has admin permissions 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('Unauthorized: admin privileges required for team feature management'); | |
| } | |
| 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 Authorization
Organization feature modification endpoint lacks authorization check to verify user has permission to modify organization features. Attackers could modify features for organizations they don't belong to or lack administrative rights for. This enables privilege escalation and unauthorized organization-wide configuration changes affecting multiple users.
setOrganizationFeatureEnabled: authedProcedure
.input(
z.object({
organizationId: z.number(),
featureSlug: z.string(),
enabled: z.boolean(),
})
)
.mutation(async ({ ctx, input }) => {
// Verify user has admin permissions for the organization
const adminMembership = await ctx.prisma.membership.findFirst({
where: {
userId: ctx.user.id,
teamId: input.organizationId,
role: { in: ['ADMIN', 'OWNER'] }
}
});
if (!adminMembership) {
throw new Error('Unauthorized: admin privileges required for organization feature management');
}
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 }) => { | |
| // Verify user has admin permissions for the organization | |
| const adminMembership = await ctx.prisma.membership.findFirst({ | |
| where: { | |
| userId: ctx.user.id, | |
| teamId: input.organizationId, | |
| role: { in: ['ADMIN', 'OWNER'] } | |
| } | |
| }); | |
| if (!adminMembership) { | |
| throw new Error('Unauthorized: admin privileges required for organization feature management'); | |
| } | |
| 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); |
There was a problem hiding this comment.
N+1 Query Pattern
A database query (getUserFeature) is executed inside a for loop that iterates over all opt-in feature slugs. This creates an N+1 query problem, where N is the number of opt-in features. This pattern significantly increases database load and API latency as the number of features grows, impacting scalability.
const eligibleFeatures: EligibleOptInFeature[] = [];
const optInSlugs = getOptInFeatureSlugs();
// Batch fetch all user features to avoid N+1 queries
const userFeatures = await this.featuresRepository.getUserFeatures(userId);
const userFeatureMap = new Map(userFeatures.map(uf => [uf.feature.slug, uf]));
for (const slug of optInSlugs) {
const config = getOptInFeatureConfig(slug);
if (!config) continue;
// Use cached user feature instead of individual query
const userFeature = userFeatureMap.get(slug);
Commitable Suggestion
| for (const slug of optInSlugs) { | |
| const config = getOptInFeatureConfig(slug); | |
| if (!config) continue; | |
| const userFeature = await this.featuresRepository.getUserFeature(userId, slug); | |
| const eligibleFeatures: EligibleOptInFeature[] = []; | |
| const optInSlugs = getOptInFeatureSlugs(); | |
| // Batch fetch all user features to avoid N+1 queries | |
| const userFeatures = await this.featuresRepository.getUserFeatures(userId); | |
| const userFeatureMap = new Map(userFeatures.map(uf => [uf.feature.slug, uf])); | |
| for (const slug of optInSlugs) { | |
| const config = getOptInFeatureConfig(slug); | |
| if (!config) continue; | |
| // Use cached user feature instead of individual query | |
| const userFeature = userFeatureMap.get(slug); |
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Batch-Processing
| * Set the enabled status of a feature for the current user. | ||
| * Users can always control their own features - no PBAC check needed. | ||
| */ | ||
| setUserFeatureEnabled: authedProcedure |
There was a problem hiding this comment.
Missing Feature Validation
Feature slug input lacks validation against allowed feature list before enabling. Attackers could enable non-existent or restricted features by providing arbitrary slug values. This could lead to system instability or unauthorized access to development features not intended for production use.
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
|
@refacto-visz |
|
PR already reviewed at the latest commit: 3c54b1f. |
|
@refacto-visz |
2 similar comments
|
@refacto-visz |
|
@refacto-visz |
CodeAnt-AI Description
Add feature opt-in and management UI with opt-in banners
What Changed
Impact
✅ Manage team and organization feature toggles from Settings✅ Can opt into new features from a URL-driven banner✅ Dismissed opt-in banners persist across sessions💡 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.