Code Review Bench PR #26801 - feat: add scope configuration for feature opt-in

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#14

feat: add scope configuration for feature opt-in

---

Summary by Gitar

• New type system:
  • OptInFeatureScope in types.ts defines three configuration levels: org, team, and user
• Scope configuration:
  • getOptInFeaturesForScope and isFeatureAllowedForScope helpers filter features by scope with backward-compatible defaults
  • Scope-specific flags HAS_USER_OPT_IN_FEATURES, HAS_TEAM_OPT_IN_FEATURES, HAS_ORG_OPT_IN_FEATURES control UI visibility
• Service validation:
  • FeatureOptInService validates scope before state changes with ErrorWithCode for unauthorized operations
• API integration:
  • tRPC endpoints in _router.ts explicitly pass scope parameters for team and org contexts
• UI updates:
  • SettingsLayoutAppDirClient.tsx uses scope-specific flags to conditionally render Features menu

_{This will update automatically on new commits.}

---

[gitar-bot-staging]

Code Review 🚫 Blocked 0 resolved / 5 findings

Three critical bugs introduced: inverted globalEnabled filter in listFeaturesForUser, swapped inherit/non-inherit branches in setUserFeatureState causing data corruption, and missing await in the tRPC setUserState mutation silently dropping errors.

🚨 Bug: Inverted filter: listFeaturesForUser returns only disabled features

📄 packages/features/feature-opt-in/services/FeatureOptInService.ts:207

The filter on line 207 was changed from state.globalEnabled to !state.globalEnabled, which inverts the logic. This causes listFeaturesForUser to return only features that are globally disabled instead of globally enabled.

The method's own JSDoc says "Only returns features that are in the allowlist, globally enabled, and scoped to 'user'", and listFeaturesForTeam uses .filter((result) => result.globalEnabled) (line 242) — confirming this is a bug.

Users will see no features in their settings when features are properly enabled, and will see features that are disabled (which should be hidden).

Suggested fix

    return featureIds.map((featureId) => resolvedStates[featureId]).filter((state) => state.globalEnabled);

🚨 Bug: Swapped branches in setUserFeatureState corrupt saved data

📄 packages/features/feature-opt-in/services/FeatureOptInService.ts:264

The condition on line 264 was changed from state === "inherit" to state !== "inherit", which swaps the two branches:

• When state is "enabled" or "disabled" (needs assignedBy), it now takes the else branch and tries to destructure assignedBy from the input — this works but saves it with assignedBy even though the simple (no-assignedBy) branch was intended.
• When state is "inherit" (no assignedBy field), it now takes the if branch and calls setUserFeatureState({ userId, featureId, state }) without assignedBy, losing attribution.

More critically, the else branch destructures assignedBy from input — when state === "inherit", the input type union doesn't include assignedBy, so assignedBy will be undefined, resulting in assignedBy: "user-undefined" being persisted to the database.

The sibling method setTeamFeatureState (line 297) uses the correct state === "inherit" condition, confirming this is a regression.

Suggested fix

    if (state === "inherit") {
      await this.featuresRepository.setUserFeatureState({ userId, featureId, state });
    } else {

🚨 Bug: Missing await on setUserFeatureState — fire-and-forget mutation

📄 packages/trpc/server/routers/viewer/featureOptIn/_router.ts:103

In the tRPC setUserState mutation (line 103), the await was removed from the featureOptInService.setUserFeatureState() call. This means:

1. The mutation returns { success: true } immediately before the database write completes.
2. If the setUserFeatureState promise rejects (e.g., scope validation error, DB failure), the error is silently swallowed — the client receives a success response.
3. This is a fire-and-forget pattern on a data-mutation endpoint, which violates user expectations of data consistency.

The sibling endpoints setTeamState (line 131) and setOrganizationState (line 161) both correctly use await.

Suggested fix

      await featureOptInService.setUserFeatureState({

💡 Quality: Dead code: UNUSED_CONSTANT left in config.ts

📄 packages/features/feature-opt-in/config.ts:4-5

Line 5 in config.ts declares const UNUSED_CONSTANT = "this-should-be-removed" with a comment saying "Unused import that should be caught by linting." This is dead code that was likely added for testing and should be removed before merging. The comment itself even acknowledges it shouldn't be there.

💡 Quality: Test mock getOptInFeaturesForScope ignores scope parameter

📄 packages/features/feature-opt-in/services/FeatureOptInService.test.ts

In FeatureOptInService.test.ts, the mock for getOptInFeaturesForScope (line 22 of the mock) is defined as () => mockFeatures, which returns all features regardless of the scope argument. This means listFeaturesForTeam and listFeaturesForUser tests don't verify that scope filtering works correctly — they always get all features.

While the config-level filtering is tested implicitly through isFeatureAllowedForScope, the service-level list methods won't catch regressions if the scope parameter stops being passed through to getOptInFeaturesForScope.

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: Appended technical summary with 5 categories covering type system, scope configuration, service validation, API integration, and UI updates

_{5 rules not applicable. Show all rules by commenting gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[gitar-bot-staging]

🚨 Bug: Inverted filter: listFeaturesForUser returns only disabled features

The filter on line 207 was changed from state.globalEnabled to !state.globalEnabled, which inverts the logic. This causes listFeaturesForUser to return only features that are globally disabled instead of globally enabled.

The method's own JSDoc says "Only returns features that are in the allowlist, globally enabled, and scoped to 'user'", and listFeaturesForTeam uses .filter((result) => result.globalEnabled) (line 242) — confirming this is a bug.

Users will see no features in their settings when features are properly enabled, and will see features that are disabled (which should be hidden).

_{Was this helpful? React with 👍 / 👎}

Suggested change

	return featureIds.map((featureId) => resolvedStates[featureId]).filter((state) => !state.globalEnabled);
	return featureIds.map((featureId) => resolvedStates[featureId]).filter((state) => state.globalEnabled);

• Apply suggested fix

[gitar-bot-staging]

🚨 Bug: Swapped branches in setUserFeatureState corrupt saved data

The condition on line 264 was changed from state === "inherit" to state !== "inherit", which swaps the two branches:

• When state is "enabled" or "disabled" (needs assignedBy), it now takes the else branch and tries to destructure assignedBy from the input — this works but saves it with assignedBy even though the simple (no-assignedBy) branch was intended.
• When state is "inherit" (no assignedBy field), it now takes the if branch and calls setUserFeatureState({ userId, featureId, state }) without assignedBy, losing attribution.

More critically, the else branch destructures assignedBy from input — when state === "inherit", the input type union doesn't include assignedBy, so assignedBy will be undefined, resulting in assignedBy: "user-undefined" being persisted to the database.

The sibling method setTeamFeatureState (line 297) uses the correct state === "inherit" condition, confirming this is a regression.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	if (state !== "inherit") {
	if (state === "inherit") {
	await this.featuresRepository.setUserFeatureState({ userId, featureId, state });
	} else {

• Apply suggested fix

[gitar-bot-staging]

🚨 Bug: Missing await on setUserFeatureState — fire-and-forget mutation

In the tRPC setUserState mutation (line 103), the await was removed from the featureOptInService.setUserFeatureState() call. This means:

1. The mutation returns { success: true } immediately before the database write completes.
2. If the setUserFeatureState promise rejects (e.g., scope validation error, DB failure), the error is silently swallowed — the client receives a success response.
3. This is a fire-and-forget pattern on a data-mutation endpoint, which violates user expectations of data consistency.

The sibling endpoints setTeamState (line 131) and setOrganizationState (line 161) both correctly use await.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	featureOptInService.setUserFeatureState({
	await featureOptInService.setUserFeatureState({

• Apply suggested fix

[gitar-bot-staging]

⚠️ This PR is blocked due to unresolved code review findings.

_{Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.}