feat: add scope configuration for feature opt-in#125
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>
Review Summary by QodoAdd scope configuration for feature opt-in with validation
WalkthroughsDescription• Add scope configuration to feature opt-in system enabling features to be scoped to org/team/user levels • Implement scope validation in setUserFeatureState and setTeamFeatureState with ErrorWithCode • Add comprehensive unit tests for scope validation across user and team feature state operations • Update settings menu visibility to use scope-specific feature availability checks • Filter features by scope in listFeaturesForUser and listFeaturesForTeam methods Diagramflowchart LR
A["OptInFeatureConfig"] -->|"add scope field"| B["OptInFeatureScope type"]
B -->|"org/team/user"| C["Feature Scoping"]
C -->|"filter by scope"| D["getOptInFeaturesForScope"]
D -->|"validate access"| E["isFeatureAllowedForScope"]
E -->|"throw ErrorWithCode"| F["setUserFeatureState"]
E -->|"throw ErrorWithCode"| G["setTeamFeatureState"]
H["Settings Menu"] -->|"use scope checks"| I["HAS_USER/TEAM/ORG_OPT_IN_FEATURES"]
File Changes1. packages/features/feature-opt-in/config.ts
|
Code Review by Qodo
1. HAS_USER_OPT_IN_FEATURES line too long
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+55
to
+61
| export const HAS_USER_OPT_IN_FEATURES: boolean = OPT_IN_FEATURES.some((f) => !f.scope || f.scope.includes("user")); | ||
|
|
||
| /** Whether there are opt-in features available for the team scope */ | ||
| export const HAS_TEAM_OPT_IN_FEATURES: boolean = OPT_IN_FEATURES.some((f) => !f.scope || f.scope.includes("team")); | ||
|
|
||
| /** Whether there are opt-in features available for the org scope */ | ||
| export const HAS_ORG_OPT_IN_FEATURES: boolean = OPT_IN_FEATURES.some((f) => !f.scope || f.scope.includes("org")); |
There was a problem hiding this comment.
1. has_user_opt_in_features line too long 📘 Rule violation ✓ Correctness
New constants are written as single lines exceeding the 110 character line-width requirement, which breaks the standardized Biome formatting rules. This will cause formatter/lint churn and inconsistent code style.
Agent Prompt
## Issue description
The newly added `HAS_USER_OPT_IN_FEATURES`, `HAS_TEAM_OPT_IN_FEATURES`, and `HAS_ORG_OPT_IN_FEATURES` constants exceed the 110 character line-width requirement.
## Issue Context
Formatting must follow the standardized Biome configuration (110 char line width).
## Fix Focus Areas
- packages/features/feature-opt-in/config.ts[55-61]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
191
to
201
| async listFeaturesForUser(input: { | ||
| userId: number; | ||
| orgId: number | null; | ||
| teamIds: number[]; | ||
| }): Promise<ListFeaturesForUserResult[]> { | ||
| const { userId, orgId, teamIds } = input; | ||
| const featureIds = OPT_IN_FEATURES.map((config) => config.slug); | ||
| const userScopedFeatures = getOptInFeaturesForScope("user"); | ||
| const featureIds = userScopedFeatures.map((config) => config.slug); | ||
|
|
||
| const resolvedStates = await this.resolveFeatureStatesAcrossTeams({ | ||
| userId, |
There was a problem hiding this comment.
2. Global filter inverted 🐞 Bug ✓ Correctness
FeatureOptInService.listFeaturesForUser now filters for !state.globalEnabled, returning only globally-disabled features (often an empty list) instead of globally-enabled ones. This breaks the user opt-in features listing returned by featureOptInRouter.listForUser.
Agent Prompt
### Issue description
`FeatureOptInService.listFeaturesForUser` incorrectly filters results using `!state.globalEnabled`, which returns only globally-disabled features and breaks the opt-in features list for users.
### Issue Context
The method docstring states it should only return globally enabled features, and `listFeaturesForTeam` still filters on `globalEnabled`.
### Fix Focus Areas
- packages/features/feature-opt-in/services/FeatureOptInService.ts[187-208]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
250
to
267
| async setUserFeatureState( | ||
| input: | ||
| | { userId: number; featureId: FeatureId; state: "enabled" | "disabled"; assignedBy: number } | ||
| | { userId: number; featureId: FeatureId; state: "inherit" } | ||
| ): Promise<void> { | ||
| const { userId, featureId, state } = input; | ||
| if (state === "inherit") { | ||
|
|
||
| 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; |
There was a problem hiding this comment.
3. Assignedby branch swapped 🐞 Bug ✓ Correctness
FeatureOptInService.setUserFeatureState omits assignedBy for enabled/disabled updates and instead tries to use assignedBy in the inherit branch where it does not exist. This conflicts with the repository contract (assignedBy required for enabled/disabled, not used for inherit) and will fail compilation and/or throw at runtime.
Agent Prompt
### Issue description
`setUserFeatureState` currently sends `assignedBy` in the wrong branch: enabled/disabled updates are missing `assignedBy`, while the `inherit` branch attempts to use `assignedBy` even though it is not part of the inherit input type.
### Issue Context
The repository contract requires `assignedBy` for enabled/disabled and does not use it for inherit (inherit deletes the row).
### Fix Focus Areas
- packages/features/feature-opt-in/services/FeatureOptInService.ts[245-275]
- packages/features/flags/features.repository.interface.ts[14-23]
- packages/features/flags/features.repository.ts[350-384]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
100
to
106
| }); | ||
| } | ||
|
|
||
| await featureOptInService.setUserFeatureState({ | ||
| featureOptInService.setUserFeatureState({ | ||
| userId: ctx.user.id, | ||
| featureId: input.slug, | ||
| state: input.state, |
There was a problem hiding this comment.
4. Unawaited user state write 🐞 Bug ⛯ Reliability
featureOptInRouter.setUserState no longer awaits featureOptInService.setUserFeatureState, so the mutation can return success before the DB write finishes and any rejection may become an unhandled promise. This causes inconsistent behavior and can hide errors from clients.
Agent Prompt
### Issue description
The `setUserState` mutation does not `await` the async `setUserFeatureState` call, returning success early and potentially dropping errors.
### Issue Context
`FeatureOptInService.setUserFeatureState` returns a Promise and performs DB writes.
### Fix Focus Areas
- packages/trpc/server/routers/viewer/featureOptIn/_router.ts[88-111]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+4
to
+5
| // Unused import that should be caught by linting | ||
| const UNUSED_CONSTANT = "this-should-be-removed"; |
There was a problem hiding this comment.
5. Unused constant left in config 🐞 Bug ⛯ Reliability
packages/features/feature-opt-in/config.ts introduces UNUSED_CONSTANT which is never referenced, leaving dead code in a core configuration module. This adds noise and can mislead future maintainers.
Agent Prompt
### Issue description
An unused constant was added to `feature-opt-in/config.ts`, creating dead code.
### Issue Context
This constant has no functional purpose and should be removed.
### Fix Focus Areas
- packages/features/feature-opt-in/config.ts[1-6]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from agentic-review-benchmarks#14