feat: add scope configuration for feature opt-in #25
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: [email protected] <[email protected]>
…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: [email protected] <[email protected]>
- 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: [email protected] <[email protected]>
…on tests Co-Authored-By: [email protected] <[email protected]>
WalkthroughThis pull request introduces scope-based configuration for opt-in features, allowing features to be scoped to organization, team, or user levels. New utilities filter features by scope and validate feature availability at each scope, with updated service methods accepting scope parameters and enforcing scope-based constraints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/trpc/server/routers/viewer/featureOptIn/_router.ts (1)
103-108: Await the user state write to avoid silent failures.The
setUserFeatureStatemethod is async and returnsPromise<void>, but the call is not awaited. This causes the mutation to return success before the write completes, leaving any rejection unhandled. Similar calls in the same file (e.g.,setTeamFeatureState) are properly awaited.✅ 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 (1)
264-275: Inverted conditional logic causes incorrect behavior.The condition is inverted compared to
setTeamFeatureState. According to the input type:
"enabled"/"disabled"states haveassignedByand should pass it to the repository"inherit"state has noassignedByand shouldn't try to access itCurrently, the code does the opposite: it omits
assignedByfor enabled/disabled states and tries to access it for inherit (where it doesn't exist).🐛 Proposed fix
- 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 2-5: Remove the unused constant by deleting the declaration of
UNUSED_CONSTANT from the file; locate the constant declaration (UNUSED_CONSTANT
= "this-should-be-removed") in the config module and remove it so the import of
types (OptInFeaturePolicy, OptInFeatureScope) remains if needed and
no-unused-vars lint/CI errors are resolved.
In `@packages/features/feature-opt-in/services/FeatureOptInService.test.ts`:
- Around line 203-280: The conditional that decides whether to include
assignedBy in the payload is inverted; in setUserFeatureState call
isFeatureAllowedForScope and throw the ErrorWithCode (with ErrorCode.BadRequest)
if not allowed, then build the repository payload so that assignedBy is only
added when state !== "inherit" (convert numeric assignedBy to the string format
`user-${assignedBy}`) and omit assignedBy entirely for the "inherit" case;
finally call featuresRepository.setUserFeatureState with that constructed
object. Ensure you reference the existing functions/methods
isFeatureAllowedForScope, setUserFeatureState, and
featuresRepository.setUserFeatureState when making the change.
📜 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 (4)
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/config.ts (3)
packages/features/flags/config.ts (1)
FeatureId(51-51)packages/features/feature-opt-in/types.ts (2)
OptInFeaturePolicy(12-12)OptInFeatureScope(21-21)packages/features/feature-opt-in/services/FeatureOptInService.test.ts (1)
isFeatureAllowedForScope(23-25)
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)
packages/features/feature-opt-in/services/FeatureOptInService.test.ts (1)
packages/features/feature-opt-in/services/FeatureOptInService.ts (1)
FeatureOptInService(72-309)
🔇 Additional comments (24)
packages/features/feature-opt-in/types.ts (1)
14-21: Scope type addition looks solid.Clear scope union and docs; fits the new config.
packages/features/feature-opt-in/config.ts (4)
12-17: Scope metadata + ALL_SCOPES export look good.
23-31: No action needed for the example comment update.
54-61: Scope-specific availability flags are straightforward.
63-80: Scope filtering and allowlist logic look correct.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (4)
14-18: Scoped opt-in imports align with the new config.
81-89: User features tab gating looks correct.
207-215: Org features tab gating looks correct.
663-672: Team features tab gating looks correct.packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts (1)
1-19: Test-level scope bypass is reasonable for DB-focused integration coverage.packages/trpc/server/routers/viewer/featureOptIn/_router.ts (4)
72-72: Team listing is now properly scoped.
81-83: Org listing scoped correctly.
131-137: Team state write is now scoped to team.
160-167: Org state write is now scoped to org.packages/features/feature-opt-in/services/IFeatureOptInService.ts (3)
4-5: Scope type import looks good.
28-39: Service signatures updated cleanly for scope-aware listing.
47-48: Scope parameter on team state setter looks good.packages/features/feature-opt-in/services/FeatureOptInService.ts (4)
1-14: LGTM!Imports are properly structured. The new error handling imports and scope utilities from the config module are correctly added.
187-208: LGTM!The scope-aware filtering using
getOptInFeaturesForScope("user")correctly limits the features returned to those available at the user level.
210-243: LGTM!The
listFeaturesForTeammethod correctly accepts an optionalscopeparameter with a sensible default of"team", and consistently uses the scoped feature set throughout.
282-308: LGTM!The
setTeamFeatureStateimplementation correctly validates scope before mutating state and properly handles theassignedByparameter based on the state value.packages/features/feature-opt-in/services/FeatureOptInService.test.ts (3)
10-28: LGTM!The mock configuration correctly sets up scope-aware features and exposes the
mockIsFeatureAllowedForScopefunction for per-test control of scope validation behavior.
282-407: LGTM!Comprehensive test coverage for
setTeamFeatureStateincluding:
- Success paths for both enabled and inherit states
- Error paths for team and org scope violations
- Default scope behavior verification
- Scope validation before setting inherit state
53-201: LGTM!The
listFeaturesForTeamtests are appropriately updated to reflect the expanded mock feature set including scope-specific features.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| import type { OptInFeaturePolicy, OptInFeatureScope } from "./types"; | ||
|
|
||
| // Unused import that should be caught by linting | ||
| const UNUSED_CONSTANT = "this-should-be-removed"; |
There was a problem hiding this comment.
Remove the unused constant to avoid lint/CI failures.
UNUSED_CONSTANT is never referenced and the comment indicates it should be removed. Keeping it may trip no-unused-vars rules.
🧹 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.
| import type { OptInFeaturePolicy, OptInFeatureScope } from "./types"; | |
| // Unused import that should be caught by linting | |
| const UNUSED_CONSTANT = "this-should-be-removed"; | |
| import type { OptInFeaturePolicy, OptInFeatureScope } from "./types"; |
🤖 Prompt for AI Agents
In `@packages/features/feature-opt-in/config.ts` around lines 2 - 5, Remove the
unused constant by deleting the declaration of UNUSED_CONSTANT from the file;
locate the constant declaration (UNUSED_CONSTANT = "this-should-be-removed") in
the config module and remove it so the import of types (OptInFeaturePolicy,
OptInFeatureScope) remains if needed and no-unused-vars lint/CI errors are
resolved.
| describe("setUserFeatureState", () => { | ||
| it("should set user feature state when scope allows", async () => { | ||
| mockIsFeatureAllowedForScope.mockReturnValue(true); | ||
| mockFeaturesRepository.setUserFeatureState.mockResolvedValue(undefined); | ||
|
|
||
| await service.setUserFeatureState({ | ||
| userId: 1, | ||
| featureId: "test-feature-1", | ||
| state: "enabled", | ||
| assignedBy: 2, | ||
| }); | ||
|
|
||
| expect(mockFeaturesRepository.setUserFeatureState).toHaveBeenCalledWith({ | ||
| userId: 1, | ||
| featureId: "test-feature-1", | ||
| state: "enabled", | ||
| assignedBy: "user-2", | ||
| }); | ||
| }); | ||
|
|
||
| it("should set user feature state to inherit when scope allows", async () => { | ||
| mockIsFeatureAllowedForScope.mockReturnValue(true); | ||
| mockFeaturesRepository.setUserFeatureState.mockResolvedValue(undefined); | ||
|
|
||
| await service.setUserFeatureState({ | ||
| userId: 1, | ||
| featureId: "test-feature-1", | ||
| state: "inherit", | ||
| }); | ||
|
|
||
| expect(mockFeaturesRepository.setUserFeatureState).toHaveBeenCalledWith({ | ||
| userId: 1, | ||
| featureId: "test-feature-1", | ||
| state: "inherit", | ||
| }); | ||
| }); | ||
|
|
||
| it("should throw ErrorWithCode when feature is not allowed at user scope", async () => { | ||
| mockIsFeatureAllowedForScope.mockReturnValue(false); | ||
|
|
||
| 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', | ||
| }); | ||
|
|
||
| expect(mockFeaturesRepository.setUserFeatureState).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should validate scope before setting inherit state", async () => { | ||
| mockIsFeatureAllowedForScope.mockReturnValue(false); | ||
|
|
||
| await expect( | ||
| service.setUserFeatureState({ | ||
| userId: 1, | ||
| featureId: "team-only-feature", | ||
| state: "inherit", | ||
| }) | ||
| ).rejects.toThrow(ErrorWithCode); | ||
|
|
||
| expect(mockFeaturesRepository.setUserFeatureState).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Tests correctly specify expected behavior, but will fail with current implementation.
The test at lines 204-221 expects assignedBy: "user-2" to be passed when state is "enabled", and the test at lines 223-238 expects no assignedBy when state is "inherit". These expectations are correct based on the input type definitions.
However, as noted in the implementation review, the conditional logic in setUserFeatureState is inverted, so these tests will fail until the implementation is fixed.
🤖 Prompt for AI Agents
In `@packages/features/feature-opt-in/services/FeatureOptInService.test.ts` around
lines 203 - 280, The conditional that decides whether to include assignedBy in
the payload is inverted; in setUserFeatureState call isFeatureAllowedForScope
and throw the ErrorWithCode (with ErrorCode.BadRequest) if not allowed, then
build the repository payload so that assignedBy is only added when state !==
"inherit" (convert numeric assignedBy to the string format `user-${assignedBy}`)
and omit assignedBy entirely for the "inherit" case; finally call
featuresRepository.setUserFeatureState with that constructed object. Ensure you
reference the existing functions/methods isFeatureAllowedForScope,
setUserFeatureState, and featuresRepository.setUserFeatureState when making the
change.
Benchmark PR from qodo-benchmark#684
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.