fix: enable DI for FeatureOptInService #24
Conversation
WalkthroughThis PR refactors dependency injection wiring by extracting FeaturesRepository DI setup into a dedicated module and establishing a new FeatureOptInService module with proper DI containers. It introduces an IFeatureOptInService interface contract and adds a resolveFeatureStatesAcrossTeams method. Multiple import paths are updated to reference the reorganized DI modules. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant DIContainer as DI Container
participant FeatureOptInModule as FeatureOptInService Module
participant FeaturesRepoModule as FeaturesRepository Module
participant Prisma as Prisma Client
participant Service as FeatureOptInService Instance
participant Repository as FeaturesRepository Instance
App->>DIContainer: getFeatureOptInService()
DIContainer->>FeatureOptInModule: createContainer()
DIContainer->>FeatureOptInModule: featureOptInServiceModuleLoader.loadModule(container)
FeatureOptInModule->>FeaturesRepoModule: load featuresRepositoryModuleLoader
FeaturesRepoModule->>FeaturesRepoModule: featuresRepositoryModuleLoader.loadModule(container)
FeaturesRepoModule->>Prisma: resolve PRISMA_CLIENT dependency
FeaturesRepoModule->>Repository: instantiate FeaturesRepository(prismaClient)
FeatureOptInModule->>Service: instantiate FeatureOptInService(repository)
DIContainer->>DIContainer: resolve IFeatureOptInService token
DIContainer-->>App: return FeatureOptInService Instance
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/di/containers/AvailableSlots.ts (1)
43-44: Duplicate module load detected.Lines 43 and 44 both load
busyTimesModulewith the same tokenDI_TOKENS.BUSY_TIMES_SERVICE_MODULE. This appears to be a copy-paste error.🔧 Proposed fix
container.load(DI_TOKENS.BUSY_TIMES_SERVICE_MODULE, busyTimesModule); -container.load(DI_TOKENS.BUSY_TIMES_SERVICE_MODULE, busyTimesModule);
🤖 Fix all issues with AI agents
In `@packages/features/di/containers/FeaturesRepository.ts`:
- Around line 4-8: The module export in the FeaturesRepository module
incorrectly sets token: moduleToken causing the container lookup (used by
getFeaturesRepository which calls featuresRepositoryModuleLoader.token) to miss
the registered binding; update the FeaturesRepository module (where
bindModuleToClassOnToken is used) to export token: token (the same token
parameter used when registering the FeaturesRepository class) instead of token:
moduleToken so the container.get in getFeaturesRepository resolves the correct
token.
In `@packages/features/di/modules/FeaturesRepository.ts`:
- Around line 19-22: The moduleLoader export incorrectly assigns
moduleLoader.token to moduleToken; change it to use the class binding token
named token so container resolution works (i.e., set moduleLoader.token =
token). Update the object literal for moduleLoader (symbol: moduleLoader) to
reference token instead of moduleToken; this aligns behavior with
getFeaturesRepository and the pattern used in FeatureOptInService.ts.
In
`@packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts`:
- Around line 82-84: The test builds separate containers for
getFeatureOptInService() and getFeaturesRepository(), so
clearFeaturesCache(featuresRepository) may not affect the service's cache;
change setup() to resolve both service and repository from the same DI container
instance (e.g., create or get a single container in setup(), then call
container.getFeatureOptInService() and container.getFeaturesRepository()), and
then call clearFeaturesCache(featuresRepository) on that shared repository so
the service and repository use the same cached instance.
🧹 Nitpick comments (1)
packages/features/di/containers/FeatureOptInService.ts (1)
6-10: Move container creation to module level for consistency.This creates a new container on each
getFeatureOptInService()call, which differs from the pattern used by most other DI container files (e.g.,AvailableSlots.ts,BookingAccessService.ts,LuckyUser.ts). Moving the container creation to module level aligns with the established pattern and avoids redundant container initialization on each invocation.♻️ Suggested refactor
import type { IFeatureOptInService } from "@calcom/features/feature-opt-in/services/IFeatureOptInService"; import { createContainer } from "../di"; import { moduleLoader as featureOptInServiceModuleLoader } from "../modules/FeatureOptInService"; +const featureOptInServiceContainer = createContainer(); +featureOptInServiceModuleLoader.loadModule(featureOptInServiceContainer); + export function getFeatureOptInService(): IFeatureOptInService { - const featureOptInServiceContainer = createContainer(); - featureOptInServiceModuleLoader.loadModule(featureOptInServiceContainer); return featureOptInServiceContainer.get<IFeatureOptInService>(featureOptInServiceModuleLoader.token); }
📜 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 (7)
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/di/containers/FeaturesRepository.ts (2)
packages/features/di/modules/FeaturesRepository.ts (1)
FeaturesRepository(24-24)packages/features/di/di.ts (1)
createContainer(114-114)
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/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)
packages/trpc/server/routers/viewer/featureOptIn/_router.ts (1)
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/features/feature-opt-in/services/FeatureOptInService.ts (2)
packages/features/di/modules/FeatureOptInService.ts (1)
FeatureOptInService(24-24)packages/features/feature-opt-in/services/IFeatureOptInService.ts (1)
IFeatureOptInService(16-39)
🔇 Additional comments (15)
packages/features/flags/di/tokens.ts (1)
1-4: LGTM!Clean token definitions following the established Symbol-based convention. The separation of flags-related tokens into their own module improves modularity.
packages/features/feature-opt-in/di/tokens.ts (1)
1-4: LGTM!Consistent token structure with FLAGS_DI_TOKENS. The feature-opt-in tokens are properly scoped to their domain.
packages/features/di/tokens.ts (2)
3-4: LGTM!Imports are well-organized and follow the existing pattern of importing domain-specific token collections.
39-40: Clean composition of DI tokens.Using spread to compose domain-specific token collections into the main
DI_TOKENSobject maintains a clear separation of concerns while providing a unified token registry.packages/features/di/modules/FeatureOptInService.ts (1)
1-24: LGTM!The module wiring follows the established pattern:
- Single dependency injection via
depparameter- Proper token assignments
- Clean re-export of the service type
packages/features/di/containers/AvailableSlots.ts (1)
12-12: LGTM!Import path update aligns with the FeaturesRepository module extraction.
packages/features/booking-audit/di/BookingAuditTaskConsumer.module.ts (1)
5-5: LGTM!Import path update is consistent with the FeaturesRepository module reorganization.
packages/features/bookings/di/RegularBookingService.module.ts (1)
6-6: LGTM!Import path update is consistent with the FeaturesRepository module reorganization.
packages/features/feature-opt-in/services/IFeatureOptInService.ts (2)
1-14: LGTM!Well-structured type definition. The
ResolvedFeatureStatetype clearly captures the hierarchical feature state model with both raw states and computed effective values.
16-39: Interface design looks solid.The discriminated union pattern for
setUserFeatureStateandsetTeamFeatureState(requiringassignedByonly for explicit enable/disable) is a good approach for type safety.Minor observation:
userStateis typed asFeatureState | undefined, but the current implementation inFeatureOptInService.tsalways defaults to"inherit"when undefined. This is fine for forward compatibility, but consider documenting whenundefinedwould be expected vs"inherit".packages/features/feature-opt-in/services/FeatureOptInService.ts (3)
7-7: LGTM!Clean import of the interface types.
13-14: LGTM!Class now properly implements the
IFeatureOptInServiceinterface, enabling proper DI patterns.
33-115: Well-implemented core method.Good practices observed:
- Efficient batching of team IDs (line 46) to minimize queries
- Parallel data fetching with
Promise.all(lines 48-60)- Clear separation of raw states vs transformed states for auto-opt-in
- Comprehensive documentation of precedence rules in the method comment
packages/trpc/server/routers/viewer/featureOptIn/_router.ts (1)
3-16: Centralized DI wiring looks good.
Keeps the router aligned with the new container-based service resolution.packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts (1)
3-9: DI-based test wiring + interface type looks good.
Nice alignment with the new DI contract in tests.Also applies to: 34-36
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| export function getFeaturesRepository(): FeaturesRepository { | ||
| const featuresRepositoryContainer = createContainer(); | ||
| featuresRepositoryModuleLoader.loadModule(featuresRepositoryContainer); | ||
| return featuresRepositoryContainer.get<FeaturesRepository>(featuresRepositoryModuleLoader.token); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check for moduleLoader.token pattern across DI containers
rg -n "moduleLoader\.token" --type ts packages/features/di/Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 68
🏁 Script executed:
#!/bin/bash
# Now let's read the FeaturesRepository.ts file to check the actual code
cat -n packages/features/di/containers/FeaturesRepository.tsRepository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 599
🏁 Script executed:
#!/bin/bash
# Also check FeaturesRepository.ts in the di folder (might be at a different location)
find packages/features/di -name "FeaturesRepository.ts" -type fRepository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Let's read the FeaturesRepository module to see the moduleLoader implementation
cat -n packages/features/di/modules/FeaturesRepository.tsRepository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 1010
🏁 Script executed:
#!/bin/bash
# Let's verify the token values to understand the impact better
rg -n "FLAGS_DI_TOKENS\.(FEATURES_REPOSITORY|FEATURES_REPOSITORY_MODULE)" --type ts packages/features/di/Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 306
🏁 Script executed:
#!/bin/bash
# Check if there are other similar patterns to understand the intended design
rg -n "export const moduleLoader.*ModuleLoader.*=.*{" -A 2 --type ts packages/features/di/modules/ | head -50Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 239
🏁 Script executed:
#!/bin/bash
# Check other moduleLoader patterns to understand the intended design
rg -n "export const moduleLoader" -A 2 --type ts packages/features/di/modules/Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 3820
🏁 Script executed:
#!/bin/bash
# Let's check what bindModuleToClassOnToken does to confirm the registration
rg -n "bindModuleToClassOnToken" -A 15 --type ts packages/features/di/di/ | head -40Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Find where bindModuleToClassOnToken is defined
find packages/features/di -name "*.ts" -type f | xargs rg -l "bindModuleToClassOnToken"Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 728
🏁 Script executed:
#!/bin/bash
# Also check the import in FeaturesRepository module
rg -n "bindModuleToClassOnToken" -B 2 -A 10 --type ts packages/features/di/modules/FeaturesRepository.tsRepository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 786
🏁 Script executed:
#!/bin/bash
# Check the di.ts file to understand bindModuleToClassOnToken's behavior
rg -n "bindModuleToClassOnToken" -A 20 --type ts packages/features/di/di.ts | head -50Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 1721
🏁 Script executed:
#!/bin/bash
# Get the full implementation of bindModuleToClassOnToken
rg -n "export function bindModuleToClassOnToken" -A 40 --type ts packages/features/di/di.ts | tail -50Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 1861
Change moduleLoader.token to token in the FeaturesRepository module.
Line 20 in packages/features/di/modules/FeaturesRepository.ts sets token: moduleToken, but should set token: token. The bindModuleToClassOnToken function registers the FeaturesRepository class with the token parameter, not moduleToken. Using moduleToken in the container helper will cause a runtime error when attempting to retrieve the repository, since the container will look for the wrong token. All other modules in this directory correctly use token in their moduleLoader exports.
🤖 Prompt for AI Agents
In `@packages/features/di/containers/FeaturesRepository.ts` around lines 4 - 8,
The module export in the FeaturesRepository module incorrectly sets token:
moduleToken causing the container lookup (used by getFeaturesRepository which
calls featuresRepositoryModuleLoader.token) to miss the registered binding;
update the FeaturesRepository module (where bindModuleToClassOnToken is used) to
export token: token (the same token parameter used when registering the
FeaturesRepository class) instead of token: moduleToken so the container.get in
getFeaturesRepository resolves the correct token.
| export const moduleLoader: ModuleLoader = { | ||
| token: moduleToken, | ||
| loadModule, | ||
| }; |
There was a problem hiding this comment.
Bug: moduleLoader.token should reference token, not moduleToken.
The moduleLoader.token is used by container helpers (like getFeaturesRepository) to resolve the bound class via container.get(moduleLoader.token). Here it's incorrectly set to moduleToken (the module's load token) instead of token (the class binding token).
Compare with FeatureOptInService.ts line 20 which correctly uses token.
🐛 Proposed 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, The
moduleLoader export incorrectly assigns moduleLoader.token to moduleToken;
change it to use the class binding token named token so container resolution
works (i.e., set moduleLoader.token = token). Update the object literal for
moduleLoader (symbol: moduleLoader) to reference token instead of moduleToken;
this aligns behavior with getFeaturesRepository and the pattern used in
FeatureOptInService.ts.
| const featuresRepository = getFeaturesRepository(); | ||
| const service = getFeatureOptInService(); | ||
|
|
There was a problem hiding this comment.
Ensure service and repository share the same container.
getFeatureOptInService() and getFeaturesRepository() each build a new container; clearFeaturesCache(featuresRepository) may not clear the cache used by service, causing stale reads or flaky tests.
Consider resolving both from a single container in setup() so the cache clear targets the same instance.
🔧 Example approach (shared container)
- const featuresRepository = getFeaturesRepository();
- const service = getFeatureOptInService();
+ // Resolve both from a single DI container so caches are shared
+ const container = createContainer();
+ featureOptInServiceModuleLoader.loadModule(container);
+ featuresRepositoryModuleLoader.loadModule(container);
+ const featuresRepository = container.get<FeaturesRepository>(featuresRepositoryModuleLoader.token);
+ const service = container.get<IFeatureOptInService>(featureOptInServiceModuleLoader.token);🤖 Prompt for AI Agents
In
`@packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts`
around lines 82 - 84, The test builds separate containers for
getFeatureOptInService() and getFeaturesRepository(), so
clearFeaturesCache(featuresRepository) may not affect the service's cache;
change setup() to resolve both service and repository from the same DI container
instance (e.g., create or get a single container in setup(), then call
container.getFeatureOptInService() and container.getFeaturesRepository()), and
then call clearFeaturesCache(featuresRepository) on that shared repository so
the service and repository use the same cached instance.
Benchmark PR from qodo-benchmark#696
Summary by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.