feat: add scope configuration for feature opt-in#41
Conversation
Add scope field to OptInFeatureConfig that allows features to be scoped to specific levels (org, team, user). This enables features to be shown only at certain settings pages rather than all three. Changes: - Add OptInFeatureScope type with values 'org', 'team', 'user' - Add optional scope field to OptInFeatureConfig interface - Add getOptInFeaturesForScope helper function to filter features by scope - Update FeatureOptInService to filter features based on scope - Update tRPC router to pass scope parameter for org/team endpoints Features without a scope field default to all scopes for backward compatibility. Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…tate - Add isFeatureAllowedForScope helper function to check if a feature is allowed for a scope - Update setUserFeatureState to reject if feature is not scoped to 'user' - Update setTeamFeatureState to accept scope parameter and reject if feature is not allowed - Update tRPC router to pass scope parameter for team and org endpoints - Fix unit test mock to include new config exports Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Replace raw Error with ErrorWithCode using ErrorCode.BadRequest - Add comprehensive tests for setUserFeatureState scope validation - Add comprehensive tests for setTeamFeatureState scope validation - Test both enabled/disabled and inherit state scenarios - Test error messages include feature ID and scope name Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…on tests Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
WalkthroughThe changes introduce scope-aware feature opt-in configuration, allowing features to be gated at organization, team, or user levels. New type definitions, helper functions, and service methods are added to support scope-based feature selection and validation across the system. Changes
Sequence Diagram(s)sequenceDiagram
actor User as Settings UI
participant Router as TRPC Router
participant Service as FeatureOptInService
participant Config as Config & Helpers
participant Repo as Repository
User->>Router: listForTeam(teamId, scope: "team")
Router->>Service: listFeaturesForTeam({teamId, scope: "team"})
Service->>Config: getOptInFeaturesForScope("team")
Config-->>Service: features scoped to "team"
Service->>Repo: Query states for scoped features
Repo-->>Service: Feature states
Service-->>Router: Scope-aware feature list
Router-->>User: Display team features
User->>Router: setTeamState(featureId, state, scope: "team")
Router->>Service: setTeamFeatureState({featureId, state, scope: "team"})
Service->>Config: isFeatureAllowedForScope(featureId, "team")
alt Scope allowed
Config-->>Service: true
Service->>Repo: Update team feature state
Repo-->>Service: Success
Service-->>Router: void
else Scope not allowed
Config-->>Service: false
Service-->>Router: ErrorWithCode BadRequest
end
Router-->>User: State updated or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/trpc/server/routers/viewer/featureOptIn/_router.ts (1)
103-110: Missingawaitcauses silent failures and incorrect success response.The
awaitkeyword was removed fromfeatureOptInService.setUserFeatureState(), making this a fire-and-forget call. This introduces several problems:
- Errors (including scope validation errors via
ErrorWithCode) become unhandled promise rejections- The response
{ success: true }is returned before the operation completes, regardless of actual outcome- Inconsistent with
setTeamState(Line 131) andsetOrganizationState(Line 161) which both properlyawait🔧 Proposed fix
- featureOptInService.setUserFeatureState({ + await featureOptInService.setUserFeatureState({ userId: ctx.user.id, featureId: input.slug, state: input.state, assignedBy: ctx.user.id, });packages/features/feature-opt-in/services/FeatureOptInService.ts (2)
187-208: Comment contradicts the filter logic — verify intended behavior.Line 189 states features should be "globally enabled", but Line 207 filters them out with
!state.globalEnabled. This appears to be either:
- An inverted filter (should be
.filter((state) => state.globalEnabled))- An incorrect comment that should say "not globally enabled"
Please clarify the intended behavior and fix either the comment or the filter logic.
264-275: Critical bug:assignedByhandling logic is inverted.The type signature shows:
- Non-inherit states (
"enabled" | "disabled") includeassignedBy: number- Inherit state does NOT include
assignedByBut the code does the opposite:
- When
state !== "inherit"(lines 264-265): ignoresassignedBythat IS available- When
state === "inherit"(lines 267-273): tries to useassignedBythat IS NOT available (will beundefined)Compare with
setTeamFeatureState(lines 297-307) which correctly handles this.🐛 Proposed fix
if (!isFeatureAllowedForScope(featureId, "user")) { throw new ErrorWithCode( ErrorCode.BadRequest, `Feature "${featureId}" is not available at the user scope` ); } - if (state !== "inherit") { - await this.featuresRepository.setUserFeatureState({ userId, featureId, state }); - } else { - const { assignedBy } = input; + if (state === "inherit") { + await this.featuresRepository.setUserFeatureState({ userId, featureId, state }); + } else { + const { assignedBy } = input as { assignedBy: number }; await this.featuresRepository.setUserFeatureState({ userId, featureId, state, assignedBy: `user-${assignedBy}`, }); }
🤖 Fix all issues with AI agents
In `@packages/features/feature-opt-in/config.ts`:
- Around line 4-5: Remove the dead code declaration UNUSED_CONSTANT from the
module: locate the const UNUSED_CONSTANT = "this-should-be-removed"; in
config.ts (symbol UNUSED_CONSTANT) and delete the unused declaration so the file
no longer contains the unused constant.
🧹 Nitpick comments (1)
packages/features/feature-opt-in/services/FeatureOptInService.test.ts (1)
243-264: Consider consolidating redundant assertions.The same async operation is awaited twice to check
toThrowandtoMatchObjectseparately. This works but calls the service method twice.💡 Consolidation suggestion
- await expect( - service.setUserFeatureState({ - userId: 1, - featureId: "org-only-feature", - state: "enabled", - assignedBy: 2, - }) - ).rejects.toThrow(ErrorWithCode); - - await expect( - service.setUserFeatureState({ - userId: 1, - featureId: "org-only-feature", - state: "enabled", - assignedBy: 2, - }) - ).rejects.toMatchObject({ - code: ErrorCode.BadRequest, - message: 'Feature "org-only-feature" is not available at the user scope', - }); + const promise = service.setUserFeatureState({ + userId: 1, + featureId: "org-only-feature", + state: "enabled", + assignedBy: 2, + }); + + await expect(promise).rejects.toThrow(ErrorWithCode); + // Re-call to verify object properties (or use a single toMatchObject check) + await expect( + service.setUserFeatureState({ + userId: 1, + featureId: "org-only-feature", + state: "enabled", + assignedBy: 2, + }) + ).rejects.toMatchObject({ + code: ErrorCode.BadRequest, + message: 'Feature "org-only-feature" is not available at the user scope', + });Alternatively,
toMatchObjectalone may suffice since it implicitly verifies the error is thrown.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsxpackages/features/feature-opt-in/config.tspackages/features/feature-opt-in/services/FeatureOptInService.integration-test.tspackages/features/feature-opt-in/services/FeatureOptInService.test.tspackages/features/feature-opt-in/services/FeatureOptInService.tspackages/features/feature-opt-in/services/IFeatureOptInService.tspackages/features/feature-opt-in/types.tspackages/trpc/server/routers/viewer/featureOptIn/_router.ts
🧰 Additional context used
🧬 Code graph analysis (5)
packages/features/feature-opt-in/services/IFeatureOptInService.ts (2)
packages/features/feature-opt-in/types.ts (1)
OptInFeatureScope(21-21)packages/features/flags/config.ts (2)
FeatureId(51-51)FeatureState(49-49)
packages/features/feature-opt-in/config.ts (2)
packages/features/flags/config.ts (1)
FeatureId(51-51)packages/features/feature-opt-in/types.ts (2)
OptInFeaturePolicy(12-12)OptInFeatureScope(21-21)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (1)
packages/features/feature-opt-in/config.ts (3)
HAS_USER_OPT_IN_FEATURES(55-55)HAS_ORG_OPT_IN_FEATURES(61-61)HAS_TEAM_OPT_IN_FEATURES(58-58)
packages/features/feature-opt-in/services/FeatureOptInService.test.ts (1)
packages/features/feature-opt-in/services/FeatureOptInService.ts (1)
FeatureOptInService(72-309)
packages/features/feature-opt-in/services/FeatureOptInService.ts (2)
packages/features/feature-opt-in/config.ts (2)
getOptInFeaturesForScope(67-69)isFeatureAllowedForScope(76-80)packages/features/feature-opt-in/types.ts (1)
OptInFeatureScope(21-21)
🔇 Additional comments (22)
packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts (1)
11-19: LGTM! Good test isolation strategy.Mocking
isFeatureAllowedForScopeto always returntruein integration tests is a sound approach. This cleanly separates concerns: unit tests cover scope validation logic, while integration tests focus on database behavior without scope-gating interference.packages/features/feature-opt-in/types.ts (1)
14-21: LGTM! Well-documented type definition.The
OptInFeatureScopetype is cleanly defined with comprehensive JSDoc documentation explaining each scope level. The union type approach is appropriate for this fixed set of values.packages/trpc/server/routers/viewer/featureOptIn/_router.ts (4)
72-72: LGTM!Correctly passes
scope: "team"for team-level feature listing.
81-82: LGTM!Correctly passes
scope: "org"for organization-level feature listing.
136-136: LGTM!Correctly passes
scope: "team"for team feature state mutations.
166-166: LGTM!Correctly passes
scope: "org"for organization feature state mutations.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (4)
14-18: LGTM! Good refactor to granular scope-based feature flags.The import change from a single
HAS_OPT_IN_FEATURESto three scope-specific flags (HAS_USER_OPT_IN_FEATURES,HAS_TEAM_OPT_IN_FEATURES,HAS_ORG_OPT_IN_FEATURES) correctly aligns with the new scope-aware feature configuration system.
81-89: LGTM!Correctly uses
HAS_USER_OPT_IN_FEATURESto conditionally show the features tab in the user's "my_account" section.
207-215: LGTM!Correctly uses
HAS_ORG_OPT_IN_FEATURESto conditionally show the features tab in the organization section.
663-672: LGTM!Correctly uses
HAS_TEAM_OPT_IN_FEATURESto conditionally show the features tab for teams.packages/features/feature-opt-in/services/IFeatureOptInService.ts (3)
4-4: LGTM!Import correctly added for the new
OptInFeatureScopetype.
28-39: LGTM! Well-structured interface extension.The
listFeaturesForUserandlistFeaturesForTeamsignatures are cleanly formatted. The optionalscopeparameter inlistFeaturesForTeammaintains backward compatibility while enabling scope-aware filtering.
45-49: LGTM!The optional
scopeparameter in both union variants ofsetTeamFeatureStatecorrectly allows distinguishing between team-level and org-level operations while maintaining backward compatibility.packages/features/feature-opt-in/services/FeatureOptInService.test.ts (3)
10-28: LGTM! Comprehensive mock setup for scope-aware testing.The mock configuration correctly includes:
- Features with different scope configurations (org-only, team-only, user-only)
- A controllable
isFeatureAllowedForScopemock via getter pattern- Proper preservation of other config exports
203-279: LGTM! Thorough test coverage forsetUserFeatureState.Tests cover all important scenarios:
- Successful state changes when scope allows
- Inherit state handling
- Error throwing with correct
ErrorCode.BadRequestwhen scope is disallowed- Scope validation before inherit state changes
282-406: LGTM! Comprehensive test coverage forsetTeamFeatureState.Tests appropriately cover:
- Team and org scope validation
- Default scope behavior (defaults to "team")
- Error codes and messages for scope violations
- Inherit state with scope validation
packages/features/feature-opt-in/config.ts (3)
7-17: LGTM!The interface extension with optional
scopefield and theALL_SCOPESconstant provide a clean, well-documented foundation for scope-based feature configuration. The JSDoc comment clearly explains the default behavior.
54-61: LGTM!The per-scope availability constants correctly implement the documented fallback behavior where features without explicit scope are available for all scopes.
63-80: LGTM!Both helper functions are well-implemented with clear documentation. The
isFeatureAllowedForScopecorrectly returnsfalsefor unconfigured features, enforcing explicit configuration as documented.packages/features/feature-opt-in/services/FeatureOptInService.ts (3)
3-14: LGTM!Imports are appropriate for the new scope validation functionality.
210-243: LGTM!The scope parameter is properly integrated with a sensible default. The filter correctly retains only globally enabled features, consistent with the documentation.
277-308: LGTM!Scope validation and
assignedByhandling are correctly implemented. The logic correctly handlesinheritvs non-inherit states.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Unused import that should be caught by linting | ||
| const UNUSED_CONSTANT = "this-should-be-removed"; |
There was a problem hiding this comment.
Remove dead code: UNUSED_CONSTANT is declared but never used.
The comment explicitly states this should be removed. This appears to be leftover debug/test code.
🧹 Proposed fix
-// Unused import that should be caught by linting
-const UNUSED_CONSTANT = "this-should-be-removed";📝 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.
| // Unused import that should be caught by linting | |
| const UNUSED_CONSTANT = "this-should-be-removed"; |
🤖 Prompt for AI Agents
In `@packages/features/feature-opt-in/config.ts` around lines 4 - 5, Remove the
dead code declaration UNUSED_CONSTANT from the module: locate the const
UNUSED_CONSTANT = "this-should-be-removed"; in config.ts (symbol
UNUSED_CONSTANT) and delete the unused declaration so the file no longer
contains the unused constant.
1 participant
Benchmark PR from agentic-review-benchmarks#14
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.