fix: enable DI for FeatureOptInService#40
Conversation
WalkthroughThe PR refactors the dependency injection structure by consolidating DI tokens into feature-specific modules, moving FeaturesRepository DI configuration to a dedicated module, extracting FeatureOptInService into a public interface contract, creating new DI container accessor functions, and updating import paths across multiple files to reference the new module locations. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
🤖 Fix all issues with AI agents
In `@packages/features/di/modules/FeaturesRepository.ts`:
- Around line 19-22: moduleLoader currently exports moduleToken but should
export the repository token; update the ModuleLoader object (moduleLoader.token)
to use the FEATURES_REPOSITORY token (the same token used in
bindModuleToClassOnToken/FLAGS_DI_TOKENS.FEATURES_REPOSITORY) rather than
moduleToken/FEATURES_REPOSITORY_MODULE so container.get(moduleLoader.token)
resolves the FeaturesRepository class correctly; keep loadModule unchanged.
In
`@packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts`:
- Around line 3-9: Tests are constructing the FeaturesRepository and
FeatureOptInService from separate DI containers causing cache divergence;
instead obtain both instances from the same container so they share the same
repository/cache. Locate the test setup where getFeaturesRepository() and
getFeatureOptInService() are both called and change it to create or retrieve a
single DI container (or pass the same repo instance into the service), then
resolve both the FeaturesRepository and the IFeatureOptInService from that same
container so clearCache() on the repo affects the service's repo; ensure you
update any calls to getFeatureOptInService to accept or resolve the shared
repo/container (symbols: getFeaturesRepository, getFeatureOptInService,
IFeatureOptInService).
🧹 Nitpick comments (1)
packages/features/feature-opt-in/services/IFeatureOptInService.ts (1)
3-14: Well-structured type with comprehensive state tracking.The
ResolvedFeatureStatetype cleanly captures both raw and effective states alongside auto-opt-in flags, providing good visibility for UI consumption.Minor observation:
userStateis typed asFeatureState | undefined(line 8), but the current implementation inFeatureOptInServicealways defaults to"inherit"via nullish coalescing. Consider whetherundefinedis needed for future implementations or if aligning the type toFeatureStatewould be more accurate.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/features/booking-audit/di/BookingAuditTaskConsumer.module.tspackages/features/bookings/di/RegularBookingService.module.tspackages/features/di/containers/AvailableSlots.tspackages/features/di/containers/FeatureOptInService.tspackages/features/di/containers/FeaturesRepository.tspackages/features/di/modules/FeatureOptInService.tspackages/features/di/modules/Features.tspackages/features/di/modules/FeaturesRepository.tspackages/features/di/tokens.tspackages/features/feature-opt-in/di/tokens.tspackages/features/feature-opt-in/services/FeatureOptInService.integration-test.tspackages/features/feature-opt-in/services/FeatureOptInService.tspackages/features/feature-opt-in/services/IFeatureOptInService.tspackages/features/flags/di/tokens.tspackages/trpc/server/routers/viewer/featureOptIn/_router.ts
💤 Files with no reviewable changes (1)
- packages/features/di/modules/Features.ts
🧰 Additional context used
🧬 Code graph analysis (6)
packages/features/di/containers/FeatureOptInService.ts (2)
packages/features/feature-opt-in/services/IFeatureOptInService.ts (1)
IFeatureOptInService(16-39)packages/features/di/di.ts (1)
createContainer(114-114)
packages/features/di/modules/FeaturesRepository.ts (3)
packages/features/di/di.ts (2)
bindModuleToClassOnToken(57-113)ModuleLoader(4-4)packages/features/flags/di/tokens.ts (1)
FLAGS_DI_TOKENS(1-4)packages/features/booking-audit/di/BookingAuditTaskConsumer.module.ts (1)
moduleLoader(27-30)
packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts (4)
packages/features/feature-opt-in/services/IFeatureOptInService.ts (1)
IFeatureOptInService(16-39)scripts/prepare-local-for-delegation-credentials-testing.js (1)
featuresRepository(12-12)packages/features/di/containers/FeaturesRepository.ts (1)
getFeaturesRepository(4-8)packages/features/di/containers/FeatureOptInService.ts (1)
getFeatureOptInService(6-10)
packages/features/feature-opt-in/services/IFeatureOptInService.ts (1)
packages/features/flags/config.ts (2)
FeatureId(51-51)FeatureState(49-49)
packages/trpc/server/routers/viewer/featureOptIn/_router.ts (1)
packages/features/di/containers/FeatureOptInService.ts (1)
getFeatureOptInService(6-10)
packages/features/di/tokens.ts (2)
packages/features/flags/di/tokens.ts (1)
FLAGS_DI_TOKENS(1-4)packages/features/feature-opt-in/di/tokens.ts (1)
FEATURE_OPT_IN_DI_TOKENS(1-4)
🔇 Additional comments (12)
packages/features/flags/di/tokens.ts (1)
1-4: Clean DI token grouping.
This neatly scopes flag-related tokens for reuse.packages/features/booking-audit/di/BookingAuditTaskConsumer.module.ts (1)
5-5: Import update aligns with new module location.
Looks consistent with the refactor.packages/features/di/containers/AvailableSlots.ts (1)
12-12: Import path update is consistent.
No concerns with this change.packages/features/di/tokens.ts (1)
3-4: Good consolidation of DI token sources.
The composed token surface looks clean and consistent.Also applies to: 39-40
packages/trpc/server/routers/viewer/featureOptIn/_router.ts (1)
3-16: DI-based service resolution looks correct.
This keeps routing logic intact while aligning with the new DI setup.packages/features/di/containers/FeatureOptInService.ts (1)
6-9: LGTM — container accessor is clear and consistent.packages/features/feature-opt-in/di/tokens.ts (1)
1-4: LGTM — token definitions are clean and scoped.packages/features/bookings/di/RegularBookingService.module.ts (1)
6-6: LGTM — updated import matches the new module layout.packages/features/di/containers/FeaturesRepository.ts (1)
4-7: LGTM — container accessor matches the DI pattern.packages/features/di/modules/FeatureOptInService.ts (1)
11-16: No action needed —bindModuleToClassOnTokencorrectly supportsdepfor single dependencies.The helper function has multiple overloads:
dep: ModuleLoaderfor single dependencies anddepsMapfor multiple dependencies. Usingdep: featuresRepositoryModuleLoaderis the correct pattern for injecting one module loader, as confirmed by widespread usage throughout the codebase (e.g., all repository modules use this approach).Likely an incorrect or invalid review comment.
packages/features/feature-opt-in/services/FeatureOptInService.ts (1)
7-7: LGTM! Clean extraction to interface for DI enablement.The import and
implementsdeclaration correctly wire the class to the new interface contract. All method signatures in the implementation align withIFeatureOptInService.Also applies to: 13-13
packages/features/feature-opt-in/services/IFeatureOptInService.ts (1)
16-39: LGTM! Clean interface contract for DI.The interface methods are well-defined with appropriate input/output types. The union types for
setUserFeatureStateandsetTeamFeatureStatecorrectly model the conditionalassignedByrequirement based on state value.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| export const moduleLoader: ModuleLoader = { | ||
| token: moduleToken, | ||
| loadModule, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the FeaturesRepository.ts file
fd -n "FeaturesRepository.ts"Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 316
🏁 Script executed:
# Once found, read the entire file to understand the context
head -50 packages/features/di/modules/FeaturesRepository.tsRepository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 842
🏁 Script executed:
# Search for how bindModuleToClassOnToken is used with this module
rg "bindModuleToClassOnToken" -B 2 -A 2 --max-count 10Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 45311
🏁 Script executed:
# Search for FEATURES_REPOSITORY token definition
rg "FEATURES_REPOSITORY" --max-count 20Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 719
🏁 Script executed:
# Search for how moduleLoader.token is used (imports of moduleLoader from FeaturesRepository)
rg "moduleLoader.*token|\.token\s*[=:)]" packages/features/di/modules/ -B 2 -A 2 | head -100Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 68
🏁 Script executed:
# Check if moduleLoader is imported and how it's used
rg "featuresRepositoryModuleLoader|from.*FeaturesRepository" --max-count 20 -B 2 -A 2Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 7075
🏁 Script executed:
# Look at the bindModuleToClassOnToken function definition to understand what token param means
rg -A 20 "export function bindModuleToClassOnToken" packages/features/di/di.ts | head -50Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 1574
🏁 Script executed:
# Check how other modules with the same pattern (token: moduleToken) are used in their containers
rg "container.get.*moduleLoader.token" -B 5 -A 5Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 68
🏁 Script executed:
# Look at a similar module's container to see the pattern
cat packages/features/di/containers/User.tsRepository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 157
🏁 Script executed:
# Find container files
fd "container" packages/features/di/containers/ -type fRepository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 256
🏁 Script executed:
# Look at the FeaturesRepository container again more carefully
cat packages/features/di/containers/FeaturesRepository.tsRepository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 543
🏁 Script executed:
# Search for other container.get calls with moduleLoader tokens to understand the pattern
rg "container.get" packages/features/di/containers/ -B 3 -A 1Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 6714
🏁 Script executed:
# Verify the bindModuleToClassOnToken actually binds to the `token` parameter, not moduleToken
rg -A 50 "export function bindModuleToClassOnToken" packages/features/di/di.ts | grep -A 40 "module.bind"Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 2588
🏁 Script executed:
# Double-check by looking at actual implementation in di.ts
sed -n '1,200p' packages/features/di/di.ts | tail -100Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 3187
Change moduleLoader.token to export the repository token, not the module token.
bindModuleToClassOnToken binds the FeaturesRepository class to FLAGS_DI_TOKENS.FEATURES_REPOSITORY (the token parameter), but moduleLoader.token currently exports moduleToken (FLAGS_DI_TOKENS.FEATURES_REPOSITORY_MODULE). This causes container.get(moduleLoader.token) in the FeaturesRepository container to fail, as it attempts to retrieve using the wrong token.
✅ Suggested fix
export const moduleLoader: ModuleLoader = {
- token: moduleToken,
+ token,
loadModule,
};📝 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.
| export const moduleLoader: ModuleLoader = { | |
| token: moduleToken, | |
| loadModule, | |
| }; | |
| export const moduleLoader: ModuleLoader = { | |
| token, | |
| loadModule, | |
| }; |
🤖 Prompt for AI Agents
In `@packages/features/di/modules/FeaturesRepository.ts` around lines 19 - 22,
moduleLoader currently exports moduleToken but should export the repository
token; update the ModuleLoader object (moduleLoader.token) to use the
FEATURES_REPOSITORY token (the same token used in
bindModuleToClassOnToken/FLAGS_DI_TOKENS.FEATURES_REPOSITORY) rather than
moduleToken/FEATURES_REPOSITORY_MODULE so container.get(moduleLoader.token)
resolves the FeaturesRepository class correctly; keep loadModule unchanged.
| import { getFeatureOptInService } from "@calcom/features/di/containers/FeatureOptInService"; | ||
| import { getFeaturesRepository } from "@calcom/features/di/containers/FeaturesRepository"; | ||
| import type { FeatureId } from "@calcom/features/flags/config"; | ||
| import { FeaturesRepository } from "@calcom/features/flags/features.repository"; | ||
| import type { FeaturesRepository } from "@calcom/features/flags/features.repository"; | ||
| import { prisma } from "@calcom/prisma"; | ||
|
|
||
| import { FeatureOptInService } from "./FeatureOptInService"; | ||
| import type { IFeatureOptInService } from "./IFeatureOptInService"; |
There was a problem hiding this comment.
Use a shared container to avoid repo cache divergence in tests.
getFeaturesRepository() and getFeatureOptInService() create separate containers, so cache clearing on the test repo won’t affect the repo instance inside the service. If the repository caches data (the clearCache usage suggests it does), this can lead to stale reads or flaky tests.
🔧 Suggested change (shared container)
-import { getFeatureOptInService } from "@calcom/features/di/containers/FeatureOptInService";
-import { getFeaturesRepository } from "@calcom/features/di/containers/FeaturesRepository";
+import { createContainer } from "@calcom/features/di/di";
+import { moduleLoader as featureOptInServiceModuleLoader } from "@calcom/features/di/modules/FeatureOptInService";
+import { moduleLoader as featuresRepositoryModuleLoader } from "@calcom/features/di/modules/FeaturesRepository";
@@
- const featuresRepository = getFeaturesRepository();
- const service = getFeatureOptInService();
+ const container = createContainer();
+ featureOptInServiceModuleLoader.loadModule(container);
+ featuresRepositoryModuleLoader.loadModule(container);
+ const featuresRepository = container.get<FeaturesRepository>(featuresRepositoryModuleLoader.token);
+ const service = container.get<IFeatureOptInService>(featureOptInServiceModuleLoader.token);Also applies to: 82-84
🤖 Prompt for AI Agents
In
`@packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts`
around lines 3 - 9, Tests are constructing the FeaturesRepository and
FeatureOptInService from separate DI containers causing cache divergence;
instead obtain both instances from the same container so they share the same
repository/cache. Locate the test setup where getFeaturesRepository() and
getFeatureOptInService() are both called and change it to create or retrieve a
single DI container (or pass the same repo instance into the service), then
resolve both the FeaturesRepository and the IFeatureOptInService from that same
container so clearCache() on the repo affects the service's repo; ensure you
update any calls to getFeatureOptInService to accept or resolve the shared
repo/container (symbols: getFeaturesRepository, getFeatureOptInService,
IFeatureOptInService).
1 participant
Benchmark PR from agentic-review-benchmarks#13
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.