Feat: remove container #190
Open
Conversation
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
WalkthroughRefactors DI across modules by introducing module-scoped factories for auth, certificate, messaging, and wallet, updating controllers/routes/tests to use them, and removing the legacy global container. Adds factory-focused unit tests. Updates README with a Supabase Integration section and reorders documentation sections. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Route/Auth Controller as Auth Controller
participant AuthModuleFactory as Auth Factory
participant Repo as PrismaUserRepository
participant UC1 as Use-Cases
Note over Route/Auth Controller: On module load
Route/Auth Controller->>AuthModuleFactory: createAuthController()
AuthModuleFactory->>Repo: new PrismaUserRepository()
AuthModuleFactory->>UC1: new Send/Resend/Verify Email, Validate/Verify Wallet (deps)
AuthModuleFactory-->>Route/Auth Controller: {use-cases}
Note over Route/Auth Controller: Handlers use injected use-cases
sequenceDiagram
autonumber
participant Router as Messaging Routes
participant MsgFactory as Messaging Factory
participant Prisma as PrismaClient
participant Repo as MessagePrismaRepository
participant Service as MessagingService
participant Controller as MessagingController
Router->>MsgFactory: createMessagingController()
MsgFactory->>Prisma: new PrismaClient()
MsgFactory->>Repo: new MessagePrismaRepository(Prisma)
MsgFactory->>Service: new MessagingService(Repo, Prisma)
MsgFactory->>Controller: new MessagingController(Service)
MsgFactory-->>Router: Controller
Router->>Controller: sendMessage/getConversation/markAsRead
sequenceDiagram
autonumber
participant CertCtrl as Certificate Controller
participant CertFactory as Certificate Factory
participant CertService as CertificateService
Note over CertCtrl: On module load
CertCtrl->>CertFactory: createCertificateService()
CertFactory->>CertService: new CertificateService()
CertFactory-->>CertCtrl: ICertificateService
CertCtrl->>CertService: create/get/download/log
sequenceDiagram
autonumber
participant WalletConsumer as Wallet Module Consumer
participant WalletFactory as Wallet Factory
participant WalletRepo as HorizonWalletRepository
participant VWF as ValidateWalletFormatUseCase
participant VW as VerifyWalletUseCase
participant WalletService as WalletService
WalletConsumer->>WalletFactory: createWalletService()
WalletFactory->>WalletRepo: new HorizonWalletRepository()
WalletFactory->>VWF: new ValidateWalletFormatUseCase()
WalletFactory->>VW: new VerifyWalletUseCase(WalletRepo)
WalletFactory->>WalletService: new WalletService()
Note over WalletFactory: Assign deps onto service
WalletFactory-->>WalletConsumer: WalletService
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/modules/certificate/presentation/controllers/certificate.controller.ts (2)
31-36: Null-check cert before logging; remove non-null assertion.
cert!.idcan throw if no record exists. Handle 404 (or create record) beforelogDownload.- const cert = await prisma.certificate.findUnique({ + const cert = await prisma.certificate.findUnique({ where: { volunteerId }, }); - - await certificateService.logDownload(cert!.id, userId); + if (!cert) { + res.status(404).json({ error: "Certificate not found" }); + return; + } + await certificateService.logDownload(cert.id, userId);
37-41: Sanitize filename in Content-Disposition to prevent header errors.Volunteer names may include invalid header/file chars. Sanitize to avoid runtime errors or injection vectors.
- res.setHeader( - "Content-Disposition", - `attachment; filename="certificate-${volunteer.name}.pdf"` - ); + const safeName = String(volunteer.name).replace(/[^\w.\- ]+/g, "_").trim() || "volunteer"; + res.setHeader( + "Content-Disposition", + `attachment; filename="certificate-${safeName}.pdf"` + );
🧹 Nitpick comments (12)
src/modules/wallet/factories/wallet.factory.ts (1)
8-11: Consider passing configuration to Horizon repo.If Horizon base URL or network is configurable, plumb env/config here rather than hardcoding defaults inside the repo.
src/modules/certificate/presentation/controllers/certificate.controller.ts (1)
21-23: Consider 404 instead of 403 for missing volunteer.Resource absence typically maps to 404; 403 implies known resource but forbidden.
src/modules/messaging/factories/messaging.factory.ts (1)
6-13: Optional: expose createMessagingService for tests.Providing a factory method that returns MessagingService simplifies service-level tests without controller wiring.
src/modules/certificate/factories/certificate.factory.ts (1)
5-7: Consider accepting infrastructure deps (Prisma/S3) via parametersRight now the service pulls singletons from module scope, which keeps hidden globals. Expose optional args to improve testability and avoid tight coupling.
Example:
export class CertificateModuleFactory { - static createCertificateService(): ICertificateService { - return new CertificateService(); - } + static createCertificateService(deps?: { + prisma?: PrismaClient; + s3?: S3Client; + bucket?: string; + }): ICertificateService { + return new CertificateService(deps); + } }src/modules/auth/presentation/controllers/Auth.controller.ts (1)
17-25: Avoid module-scope singletons; wire at composition timeCreating use cases at import-time makes them global within the process, which reduces test isolation and flexibility.
Minimal approach:
- Wrap a getter and call it inside handlers, or
- Export a
createAuthHandlers()that returns bound handlers usingAuthModuleFactory.createAuthController()at router assembly time.Example (outside this hunk):
export const createAuthHandlers = () => { const uc = AuthModuleFactory.createAuthController(); return { register: (req,res)=>registerWith(uc, req, res), // ... }; };src/modules/certificate/__tests__/factories/certificate.factory.test.ts (1)
25-33: Don’t assert on class identity when using jest class mocks
toBeInstanceOf(CertificateService)is brittle with jest-mocked classes. Assert on behavior/shape instead (you already do below).Apply this diff:
- expect(certificateService).toBeInstanceOf(CertificateService);src/modules/auth/__tests__/controllers/AuthController.int.test.ts (1)
5-9: Good factory-based setup; consider testing handlers via injected depsThis validates wiring presence, but the
registerimport still uses module-scope instances from the controller file. Prefer invoking handlers produced by acreateAuthHandlers()to ensure end-to-end factory wiring.src/modules/messaging/__tests__/factories/messaging.factory.test.ts (1)
35-47: Avoid creating a new PrismaClient per factory call in productionThe factory under test instantiates
new PrismaClient()each call, which can exhaust DB connections. Prefer injecting a shared PrismaClient or accepting it as an optional arg.Example:
export class MessagingModuleFactory { static createMessagingController(prisma = sharedPrisma): MessagingController { const repo = new MessagePrismaRepository(prisma); const svc = new MessagingService(repo, prisma); return new MessagingController(svc); } }src/modules/auth/factories/auth.factory.ts (2)
9-24: Method name is misleading; add explicit return type and a non-breaking alias.This returns a bag of use-cases, not a controller. Add a clear return type and (optionally) expose a better-named alias to reduce confusion without breaking existing imports.
Apply:
export class AuthModuleFactory { - static createAuthController() { + static createAuthController(): AuthUseCases { const userRepository = new PrismaUserRepository(); @@ return { sendVerificationEmailUseCase, resendVerificationEmailUseCase, verifyEmailUseCase, validateWalletFormatUseCase, verifyWalletUseCase, }; } + + // Alias with clearer intent; keep old name for now. + static createAuthUseCases(): AuthUseCases { + return this.createAuthController(); + } }Outside this hunk, add the type:
export type AuthUseCases = { sendVerificationEmailUseCase: SendVerificationEmailUseCase; resendVerificationEmailUseCase: ResendVerificationEmailUseCase; verifyEmailUseCase: VerifyEmailUseCase; validateWalletFormatUseCase: ValidateWalletFormatUseCase; verifyWalletUseCase: VerifyWalletUseCase; };
10-15: Permit dependency overrides for testability and composition.Allow callers to inject a custom user repository (e.g., in-memory) while preserving the default Prisma implementation.
- static createAuthController(): AuthUseCases { - const userRepository = new PrismaUserRepository(); + static createAuthController( + deps: { userRepository?: PrismaUserRepository } = {} + ): AuthUseCases { + const userRepository = deps.userRepository ?? new PrismaUserRepository();src/modules/auth/__tests__/factories/auth.factory.test.ts (2)
65-71: Strengthen “different instances” assertion.Also assert another use-case differs across factory calls to avoid false positives.
expect(firstInstance).not.toBe(secondInstance); expect(firstInstance.sendVerificationEmailUseCase).not.toBe(secondInstance.sendVerificationEmailUseCase); + expect(firstInstance.verifyEmailUseCase).not.toBe(secondInstance.verifyEmailUseCase);
73-83: Actually verify all use-cases share the same repository instance.Counting constructor calls doesn’t prove the same repo was injected. Assert constructor args equality.
it("should create use cases with the same user repository instance", () => { const authUseCases = AuthModuleFactory.createAuthController(); // All use cases that depend on user repository should use the same instance expect(authUseCases.sendVerificationEmailUseCase).toBeDefined(); expect(authUseCases.resendVerificationEmailUseCase).toBeDefined(); expect(authUseCases.verifyEmailUseCase).toBeDefined(); - // Verify the user repository was created only once - expect(PrismaUserRepository).toHaveBeenCalledTimes(1); + // Verify the same repo instance was passed to each constructor + const repoArgs = [ + (SendVerificationEmailUseCase as jest.Mock).mock.calls[0][0], + (ResendVerificationEmailUseCase as jest.Mock).mock.calls[0][0], + (VerifyEmailUseCase as jest.Mock).mock.calls[0][0], + ]; + expect(new Set(repoArgs).size).toBe(1); + expect(PrismaUserRepository).toHaveBeenCalledTimes(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
README.md(1 hunks)src/modules/auth/__tests__/controllers/AuthController.int.test.ts(2 hunks)src/modules/auth/__tests__/factories/auth.factory.test.ts(1 hunks)src/modules/auth/factories/auth.factory.ts(1 hunks)src/modules/auth/presentation/controllers/Auth.controller.ts(1 hunks)src/modules/certificate/__tests__/factories/certificate.factory.test.ts(1 hunks)src/modules/certificate/factories/certificate.factory.ts(1 hunks)src/modules/certificate/presentation/controllers/certificate.controller.ts(1 hunks)src/modules/messaging/__tests__/factories/messaging.factory.test.ts(1 hunks)src/modules/messaging/factories/messaging.factory.ts(1 hunks)src/modules/messaging/routes/messaging.routes.ts(1 hunks)src/modules/wallet/factories/wallet.factory.ts(1 hunks)src/shared/infrastructure/container.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/shared/infrastructure/container.ts
🧰 Additional context used
🧬 Code graph analysis (10)
src/modules/auth/factories/auth.factory.ts (4)
src/modules/user/repositories/PrismaUserRepository.ts (1)
PrismaUserRepository(7-80)src/modules/auth/use-cases/send-verification-email.usecase.ts (1)
SendVerificationEmailUseCase(9-62)src/modules/auth/use-cases/resend-verification-email.usecase.ts (1)
ResendVerificationEmailUseCase(9-62)src/modules/auth/use-cases/verify-email.usecase.ts (1)
VerifyEmailUseCase(7-54)
src/modules/certificate/presentation/controllers/certificate.controller.ts (2)
src/modules/certificate/domain/interfaces/ICertificateService.ts (1)
ICertificateService(9-20)src/modules/certificate/factories/certificate.factory.ts (1)
CertificateModuleFactory(4-8)
src/modules/messaging/__tests__/factories/messaging.factory.test.ts (5)
tests/__mocks__/prisma.ts (1)
PrismaClient(1-3)src/modules/messaging/repositories/implementations/message-prisma.repository.ts (1)
MessagePrismaRepository(5-150)src/modules/messaging/application/services/MessagingService.ts (1)
MessagingService(5-111)src/modules/messaging/controllers/MessagingController.ts (1)
MessagingController(8-166)src/modules/messaging/factories/messaging.factory.ts (1)
MessagingModuleFactory(6-13)
src/modules/certificate/__tests__/factories/certificate.factory.test.ts (2)
src/modules/certificate/application/services/CertificateService.ts (1)
CertificateService(26-103)src/modules/certificate/factories/certificate.factory.ts (1)
CertificateModuleFactory(4-8)
src/modules/certificate/factories/certificate.factory.ts (2)
src/modules/certificate/domain/interfaces/ICertificateService.ts (1)
ICertificateService(9-20)src/modules/certificate/application/services/CertificateService.ts (1)
CertificateService(26-103)
src/modules/messaging/factories/messaging.factory.ts (4)
src/modules/messaging/controllers/MessagingController.ts (1)
MessagingController(8-166)tests/__mocks__/prisma.ts (1)
PrismaClient(1-3)src/modules/messaging/repositories/implementations/message-prisma.repository.ts (1)
MessagePrismaRepository(5-150)src/modules/messaging/application/services/MessagingService.ts (1)
MessagingService(5-111)
src/modules/auth/presentation/controllers/Auth.controller.ts (1)
src/modules/auth/factories/auth.factory.ts (1)
AuthModuleFactory(8-25)
src/modules/auth/__tests__/factories/auth.factory.test.ts (5)
src/modules/user/repositories/PrismaUserRepository.ts (1)
PrismaUserRepository(7-80)src/modules/auth/use-cases/send-verification-email.usecase.ts (1)
SendVerificationEmailUseCase(9-62)src/modules/auth/use-cases/resend-verification-email.usecase.ts (1)
ResendVerificationEmailUseCase(9-62)src/modules/auth/use-cases/verify-email.usecase.ts (1)
VerifyEmailUseCase(7-54)src/modules/auth/factories/auth.factory.ts (1)
AuthModuleFactory(8-25)
src/modules/messaging/routes/messaging.routes.ts (1)
src/modules/messaging/factories/messaging.factory.ts (1)
MessagingModuleFactory(6-13)
src/modules/auth/__tests__/controllers/AuthController.int.test.ts (1)
src/modules/auth/factories/auth.factory.ts (1)
AuthModuleFactory(8-25)
🪛 LanguageTool
README.md
[grammar] ~239-~239: There might be a mistake here.
Context: ...eed ``` --- ## 🔌 Supabase Integration This project uses Supabase for external ...
(QB_NEW_EN)
🔇 Additional comments (9)
src/modules/certificate/presentation/controllers/certificate.controller.ts (1)
3-8: Factory swap LGTM.Using CertificateModuleFactory removes the container dependency cleanly.
src/modules/messaging/factories/messaging.factory.ts (1)
1-4: Imports LGTM.Wiring aligns with module boundaries and responsibilities.
src/modules/certificate/factories/certificate.factory.ts (1)
4-8: Factory wiring looks goodSimple, explicit creation; aligns with PR goal of removing global container.
src/modules/certificate/__tests__/factories/certificate.factory.test.ts (1)
35-41: Constructor call count expectations depend on setupWith the prior fix, each factory call triggers exactly one constructor call, so current expectations should pass. If you keep custom implementations that reuse instances, adjust assertions accordingly.
src/modules/messaging/__tests__/factories/messaging.factory.test.ts (3)
42-47: Assertion style is good; relies on fixed constructor countsAfter the fix above, these will pass (each factory call constructs once). If you later reuse singletons in the factory, switch to
toHaveBeenCalled()instead of strict counts.
49-55: Per-call instance creation expectation OKWith corrected mocks, expecting two controller constructions across two calls is valid.
57-64: Method shape checks are sufficientVerifies controller surface without over-coupling to implementation details.
src/modules/auth/factories/auth.factory.ts (1)
1-6: Legacy container references fully removed
Ripgrep checks forcontainer.resolve|get|register,container.ts, imports from “container”, and mentions of Typedi/Inversify/Tsyringe returned no matches.src/modules/auth/__tests__/factories/auth.factory.test.ts (1)
56-63: Constructor call count assertions rely on the fix above.With the revised setup, only the factory calls the constructors, so
toHaveBeenCalledTimes(1)will be stable. If you keep any directnewcalls, change this totoHaveBeenCalledTimes(2)or>= 1.
Comment on lines
+239
to
+248
| ## 🔌 Supabase Integration | ||
|
|
||
| This project uses Supabase for external data access and future integrations. | ||
|
|
||
| Update your `.env` file with: | ||
|
|
||
| ```bash | ||
| SUPABASE_URL=... | ||
| SUPABASE_ANON_KEY=... | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Supabase env vars: add service role key note and align .env.example.
Document the server-only SUPABASE_SERVICE_ROLE_KEY and warn against committing secrets. Also ensure .env.example includes these vars.
Apply:
## 🔌 Supabase Integration
This project uses Supabase for external data access and future integrations.
Update your `.env` file with:
```bash
SUPABASE_URL=...
SUPABASE_ANON_KEY=...
+# Server-only key for admin operations (never expose publicly)
+SUPABASE_SERVICE_ROLE_KEY=...
Run to verify placeholders exist in .env.example and code refs are consistent:
---
🏁 Script executed:
```shell
#!/bin/bash
rg -nP 'SUPABASE_(URL|ANON_KEY|SERVICE_ROLE_KEY)' -S .env.example || true
rg -nP 'process\.env\.SUPABASE_(URL|ANON_KEY|SERVICE_ROLE_KEY)' -S | sed -E 's/^(.{0,120}).*/\1/'
Length of output: 400
Add SUPABASE_SERVICE_ROLE_KEY to README and .env.example
- README.md (lines 239–248): append
SUPABASE_SERVICE_ROLE_KEY=… # Server-only key for admin operations (never expose publicly)
to the env snippet. - .env.example (after line 56): add
SUPABASE_SERVICE_ROLE_KEY=your-service-role-key.
🧰 Tools
🪛 LanguageTool
[grammar] ~239-~239: There might be a mistake here.
Context: ...eed ``` --- ## 🔌 Supabase Integration This project uses Supabase for external ...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In README.md around lines 239 to 248 and .env.example after line 56, the
instructions and example are missing the Supabase service role key; append to
the README env snippet a new line: SUPABASE_SERVICE_ROLE_KEY=… with a comment
that it's a server-only key (never expose publicly), and add
SUPABASE_SERVICE_ROLE_KEY=your-service-role-key to .env.example after line 56 so
the example includes the server-only key placeholder.
Comment on lines
+25
to
+43
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
|
|
||
| // Create mock instances | ||
| mockUserRepository = new PrismaUserRepository() as jest.Mocked<PrismaUserRepository>; | ||
| mockSendVerificationEmailUseCase = new SendVerificationEmailUseCase(mockUserRepository) as jest.Mocked<SendVerificationEmailUseCase>; | ||
| mockResendVerificationEmailUseCase = new ResendVerificationEmailUseCase(mockUserRepository) as jest.Mocked<ResendVerificationEmailUseCase>; | ||
| mockVerifyEmailUseCase = new VerifyEmailUseCase(mockUserRepository) as jest.Mocked<VerifyEmailUseCase>; | ||
| mockValidateWalletFormatUseCase = new ValidateWalletFormatUseCase() as jest.Mocked<ValidateWalletFormatUseCase>; | ||
| mockVerifyWalletUseCase = new VerifyWalletUseCase() as jest.Mocked<VerifyWalletUseCase>; | ||
|
|
||
| // Mock the constructors | ||
| (PrismaUserRepository as jest.Mock).mockImplementation(() => mockUserRepository); | ||
| (SendVerificationEmailUseCase as jest.Mock).mockImplementation(() => mockSendVerificationEmailUseCase); | ||
| (ResendVerificationEmailUseCase as jest.Mock).mockImplementation(() => mockResendVerificationEmailUseCase); | ||
| (VerifyEmailUseCase as jest.Mock).mockImplementation(() => mockVerifyEmailUseCase); | ||
| (ValidateWalletFormatUseCase as jest.Mock).mockImplementation(() => mockValidateWalletFormatUseCase); | ||
| (VerifyWalletUseCase as jest.Mock).mockImplementation(() => mockVerifyWalletUseCase); | ||
| }); |
There was a problem hiding this comment.
Do not instantiate mocked classes in setup; it corrupts constructor call counts.
Creating mocks via new before wiring assertions makes toHaveBeenCalledTimes(1) fail and couples tests to Jest internals. Use stub objects + mockReturnValue instead.
beforeEach(() => {
jest.clearAllMocks();
- // Create mock instances
- mockUserRepository = new PrismaUserRepository() as jest.Mocked<PrismaUserRepository>;
- mockSendVerificationEmailUseCase = new SendVerificationEmailUseCase(mockUserRepository) as jest.Mocked<SendVerificationEmailUseCase>;
- mockResendVerificationEmailUseCase = new ResendVerificationEmailUseCase(mockUserRepository) as jest.Mocked<ResendVerificationEmailUseCase>;
- mockVerifyEmailUseCase = new VerifyEmailUseCase(mockUserRepository) as jest.Mocked<VerifyEmailUseCase>;
- mockValidateWalletFormatUseCase = new ValidateWalletFormatUseCase() as jest.Mocked<ValidateWalletFormatUseCase>;
- mockVerifyWalletUseCase = new VerifyWalletUseCase() as jest.Mocked<VerifyWalletUseCase>;
+ // Create stub instances (no constructor calls)
+ mockUserRepository = {} as unknown as jest.Mocked<PrismaUserRepository>;
+ mockSendVerificationEmailUseCase = {} as unknown as jest.Mocked<SendVerificationEmailUseCase>;
+ mockResendVerificationEmailUseCase = {} as unknown as jest.Mocked<ResendVerificationEmailUseCase>;
+ mockVerifyEmailUseCase = {} as unknown as jest.Mocked<VerifyEmailUseCase>;
+ mockValidateWalletFormatUseCase = {} as unknown as jest.Mocked<ValidateWalletFormatUseCase>;
+ mockVerifyWalletUseCase = {} as unknown as jest.Mocked<VerifyWalletUseCase>;
// Mock the constructors
- (PrismaUserRepository as jest.Mock).mockImplementation(() => mockUserRepository);
- (SendVerificationEmailUseCase as jest.Mock).mockImplementation(() => mockSendVerificationEmailUseCase);
- (ResendVerificationEmailUseCase as jest.Mock).mockImplementation(() => mockResendVerificationEmailUseCase);
- (VerifyEmailUseCase as jest.Mock).mockImplementation(() => mockVerifyEmailUseCase);
- (ValidateWalletFormatUseCase as jest.Mock).mockImplementation(() => mockValidateWalletFormatUseCase);
- (VerifyWalletUseCase as jest.Mock).mockImplementation(() => mockVerifyWalletUseCase);
+ (PrismaUserRepository as jest.Mock).mockReturnValue(mockUserRepository);
+ (SendVerificationEmailUseCase as jest.Mock).mockReturnValue(mockSendVerificationEmailUseCase);
+ (ResendVerificationEmailUseCase as jest.Mock).mockReturnValue(mockResendVerificationEmailUseCase);
+ (VerifyEmailUseCase as jest.Mock).mockReturnValue(mockVerifyEmailUseCase);
+ (ValidateWalletFormatUseCase as jest.Mock).mockReturnValue(mockValidateWalletFormatUseCase);
+ (VerifyWalletUseCase as jest.Mock).mockReturnValue(mockVerifyWalletUseCase);
});📝 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.
Suggested change
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| // Create mock instances | |
| mockUserRepository = new PrismaUserRepository() as jest.Mocked<PrismaUserRepository>; | |
| mockSendVerificationEmailUseCase = new SendVerificationEmailUseCase(mockUserRepository) as jest.Mocked<SendVerificationEmailUseCase>; | |
| mockResendVerificationEmailUseCase = new ResendVerificationEmailUseCase(mockUserRepository) as jest.Mocked<ResendVerificationEmailUseCase>; | |
| mockVerifyEmailUseCase = new VerifyEmailUseCase(mockUserRepository) as jest.Mocked<VerifyEmailUseCase>; | |
| mockValidateWalletFormatUseCase = new ValidateWalletFormatUseCase() as jest.Mocked<ValidateWalletFormatUseCase>; | |
| mockVerifyWalletUseCase = new VerifyWalletUseCase() as jest.Mocked<VerifyWalletUseCase>; | |
| // Mock the constructors | |
| (PrismaUserRepository as jest.Mock).mockImplementation(() => mockUserRepository); | |
| (SendVerificationEmailUseCase as jest.Mock).mockImplementation(() => mockSendVerificationEmailUseCase); | |
| (ResendVerificationEmailUseCase as jest.Mock).mockImplementation(() => mockResendVerificationEmailUseCase); | |
| (VerifyEmailUseCase as jest.Mock).mockImplementation(() => mockVerifyEmailUseCase); | |
| (ValidateWalletFormatUseCase as jest.Mock).mockImplementation(() => mockValidateWalletFormatUseCase); | |
| (VerifyWalletUseCase as jest.Mock).mockImplementation(() => mockVerifyWalletUseCase); | |
| }); | |
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| // Create stub instances (no constructor calls) | |
| mockUserRepository = {} as unknown as jest.Mocked<PrismaUserRepository>; | |
| mockSendVerificationEmailUseCase = {} as unknown as jest.Mocked<SendVerificationEmailUseCase>; | |
| mockResendVerificationEmailUseCase = {} as unknown as jest.Mocked<ResendVerificationEmailUseCase>; | |
| mockVerifyEmailUseCase = {} as unknown as jest.Mocked<VerifyEmailUseCase>; | |
| mockValidateWalletFormatUseCase = {} as unknown as jest.Mocked<ValidateWalletFormatUseCase>; | |
| mockVerifyWalletUseCase = {} as unknown as jest.Mocked<VerifyWalletUseCase>; | |
| // Mock the constructors | |
| (PrismaUserRepository as jest.Mock).mockReturnValue(mockUserRepository); | |
| (SendVerificationEmailUseCase as jest.Mock).mockReturnValue(mockSendVerificationEmailUseCase); | |
| (ResendVerificationEmailUseCase as jest.Mock).mockReturnValue(mockResendVerificationEmailUseCase); | |
| (VerifyEmailUseCase as jest.Mock).mockReturnValue(mockVerifyEmailUseCase); | |
| (ValidateWalletFormatUseCase as jest.Mock).mockReturnValue(mockValidateWalletFormatUseCase); | |
| (VerifyWalletUseCase as jest.Mock).mockReturnValue(mockVerifyWalletUseCase); | |
| }); |
🤖 Prompt for AI Agents
In src/modules/auth/__tests__/factories/auth.factory.test.ts around lines 25 to
43, the beforeEach currently instantiates mocked classes (new
PrismaUserRepository(), new SendVerificationEmailUseCase(), etc.) which corrupts
constructor call counts; replace those "new" instantiations with lightweight
stub objects that implement the minimal shape needed for the tests (or typed as
Partial/ jest.Mocked) and then wire the constructor mocks using
mockReturnValue(mockStub) instead of mockImplementation(() => mockInstance).
Specifically: remove all `new ...` calls, create plain stub objects for
mockUserRepository and each use case, and change `(X as
jest.Mock).mockImplementation(() => mockX)` to `(X as
jest.Mock).mockReturnValue(mockXStub)` so constructors are not invoked and
call-count assertions remain accurate.
Comment on lines
+11
to
+15
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| mockCertificateService = new CertificateService() as jest.Mocked<CertificateService>; | ||
| (CertificateService as jest.Mock).mockImplementation(() => mockCertificateService); | ||
| }); |
There was a problem hiding this comment.
Test mutates constructor call counts before assertions
Instantiating new CertificateService() in beforeEach increments the constructor count, breaking toHaveBeenCalledTimes expectations.
Apply this diff to avoid pre-instantiation and still control the returned mock:
beforeEach(() => {
- jest.clearAllMocks();
- mockCertificateService = new CertificateService() as jest.Mocked<CertificateService>;
- (CertificateService as jest.Mock).mockImplementation(() => mockCertificateService);
+ jest.clearAllMocks();
+ // Return a fresh mock on each factory call without invoking the constructor early
+ (CertificateService as unknown as jest.Mock).mockImplementation(() => {
+ return Object.assign(Object.create(CertificateService.prototype), {
+ createCertificate: jest.fn(),
+ getCertificateUrl: jest.fn(),
+ getCertificateBuffer: jest.fn(),
+ logDownload: jest.fn(),
+ }) as CertificateService;
+ });
});📝 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.
Suggested change
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| mockCertificateService = new CertificateService() as jest.Mocked<CertificateService>; | |
| (CertificateService as jest.Mock).mockImplementation(() => mockCertificateService); | |
| }); | |
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| // Return a fresh mock on each factory call without invoking the constructor early | |
| (CertificateService as unknown as jest.Mock).mockImplementation(() => { | |
| return Object.assign(Object.create(CertificateService.prototype), { | |
| createCertificate: jest.fn(), | |
| getCertificateUrl: jest.fn(), | |
| getCertificateBuffer: jest.fn(), | |
| logDownload: jest.fn(), | |
| }) as CertificateService; | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In src/modules/certificate/__tests__/factories/certificate.factory.test.ts
around lines 11-15, the beforeEach currently calls new CertificateService(),
which increments the mocked constructor call count and breaks
toHaveBeenCalledTimes assertions; remove the instantiation and instead create a
mocked object (e.g. const mockCertificateService = {} as
jest.Mocked<CertificateService> or build it via jest.createMockFromModule /
manual jest.fn() fields) and then wire it up with (CertificateService as
jest.Mock).mockImplementation(() => mockCertificateService) (or
mockImplementationOnce where appropriate) so the test controls the returned mock
without incrementing the constructor call count.
Comment on lines
+22
to
+33
| // Create mock instances | ||
| mockPrisma = new PrismaClient() as jest.Mocked<PrismaClient>; | ||
| mockMessageRepository = new MessagePrismaRepository(mockPrisma) as jest.Mocked<MessagePrismaRepository>; | ||
| mockMessagingService = new MessagingService(mockMessageRepository, mockPrisma) as jest.Mocked<MessagingService>; | ||
| mockMessagingController = new MessagingController(mockMessagingService) as jest.Mocked<MessagingController>; | ||
|
|
||
| // Mock the constructors | ||
| (PrismaClient as jest.Mock).mockImplementation(() => mockPrisma); | ||
| (MessagePrismaRepository as jest.Mock).mockImplementation(() => mockMessageRepository); | ||
| (MessagingService as jest.Mock).mockImplementation(() => mockMessagingService); | ||
| (MessagingController as jest.Mock).mockImplementation(() => mockMessagingController); | ||
| }); |
There was a problem hiding this comment.
Pre-instantiating mocks inflates constructor call counts
Creating instances with new before setting mockImplementation causes toHaveBeenCalledTimes failures.
Apply this diff to return stubs without early constructor calls and preserve instanceof:
- // Create mock instances
- mockPrisma = new PrismaClient() as jest.Mocked<PrismaClient>;
- mockMessageRepository = new MessagePrismaRepository(mockPrisma) as jest.Mocked<MessagePrismaRepository>;
- mockMessagingService = new MessagingService(mockMessageRepository, mockPrisma) as jest.Mocked<MessagingService>;
- mockMessagingController = new MessagingController(mockMessagingService) as jest.Mocked<MessagingController>;
-
- // Mock the constructors
- (PrismaClient as jest.Mock).mockImplementation(() => mockPrisma);
- (MessagePrismaRepository as jest.Mock).mockImplementation(() => mockMessageRepository);
- (MessagingService as jest.Mock).mockImplementation(() => mockMessagingService);
- (MessagingController as jest.Mock).mockImplementation(() => mockMessagingController);
+ // Provide stub instances without calling constructors
+ mockPrisma = {} as unknown as PrismaClient;
+ mockMessageRepository = {} as unknown as MessagePrismaRepository;
+ mockMessagingService = {
+ sendMessage: jest.fn(),
+ getConversation: jest.fn(),
+ markMessageAsRead: jest.fn(),
+ } as unknown as MessagingService;
+ const makeController = () => {
+ const ctrl = Object.create(MessagingController.prototype) as MessagingController;
+ (ctrl as any).sendMessage = jest.fn();
+ (ctrl as any).getConversation = jest.fn();
+ (ctrl as any).markAsRead = jest.fn();
+ return ctrl as jest.Mocked<MessagingController>;
+ };
+
+ // Mock the constructors to return the stubs
+ (PrismaClient as unknown as jest.Mock).mockImplementation(() => mockPrisma);
+ (MessagePrismaRepository as unknown as jest.Mock).mockImplementation(() => mockMessageRepository);
+ (MessagingService as unknown as jest.Mock).mockImplementation(() => mockMessagingService);
+ (MessagingController as unknown as jest.Mock).mockImplementation(() => makeController());📝 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.
Suggested change
| // Create mock instances | |
| mockPrisma = new PrismaClient() as jest.Mocked<PrismaClient>; | |
| mockMessageRepository = new MessagePrismaRepository(mockPrisma) as jest.Mocked<MessagePrismaRepository>; | |
| mockMessagingService = new MessagingService(mockMessageRepository, mockPrisma) as jest.Mocked<MessagingService>; | |
| mockMessagingController = new MessagingController(mockMessagingService) as jest.Mocked<MessagingController>; | |
| // Mock the constructors | |
| (PrismaClient as jest.Mock).mockImplementation(() => mockPrisma); | |
| (MessagePrismaRepository as jest.Mock).mockImplementation(() => mockMessageRepository); | |
| (MessagingService as jest.Mock).mockImplementation(() => mockMessagingService); | |
| (MessagingController as jest.Mock).mockImplementation(() => mockMessagingController); | |
| }); | |
| // Provide stub instances without calling constructors | |
| mockPrisma = {} as unknown as PrismaClient; | |
| mockMessageRepository = {} as unknown as MessagePrismaRepository; | |
| mockMessagingService = { | |
| sendMessage: jest.fn(), | |
| getConversation: jest.fn(), | |
| markMessageAsRead: jest.fn(), | |
| } as unknown as MessagingService; | |
| const makeController = () => { | |
| const ctrl = Object.create(MessagingController.prototype) as MessagingController; | |
| (ctrl as any).sendMessage = jest.fn(); | |
| (ctrl as any).getConversation = jest.fn(); | |
| (ctrl as any).markAsRead = jest.fn(); | |
| return ctrl as jest.Mocked<MessagingController>; | |
| }; | |
| // Mock the constructors to return the stubs | |
| (PrismaClient as unknown as jest.Mock).mockImplementation(() => mockPrisma); | |
| (MessagePrismaRepository as unknown as jest.Mock).mockImplementation(() => mockMessageRepository); | |
| (MessagingService as unknown as jest.Mock).mockImplementation(() => mockMessagingService); | |
| (MessagingController as unknown as jest.Mock).mockImplementation(() => makeController()); |
🤖 Prompt for AI Agents
In src/modules/messaging/__tests__/factories/messaging.factory.test.ts around
lines 22 to 33, the test currently pre-instantiates real objects with `new`
before calling `mockImplementation`, which increments constructor call counts
and breaks `toHaveBeenCalledTimes` assertions; instead remove the `new` calls
and create plain stub instances (e.g., via Object.create(Class.prototype) or an
empty object with its prototype set to the real class prototype) for
PrismaClient, MessagePrismaRepository, MessagingService and MessagingController,
then set each `(Class as jest.Mock).mockImplementation(() => stubInstance)` so
constructors are not invoked but `instanceof` still holds; finally ensure mocks
are cleaned up/reset in afterEach if not already done.
Comment on lines
+8
to
+12
| const prisma = new PrismaClient(); | ||
| const messageRepository = new MessagePrismaRepository(prisma); | ||
| const messagingService = new MessagingService(messageRepository, prisma); | ||
| return new MessagingController(messagingService); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Accept PrismaClient from caller; don’t instantiate here.
Creating Prisma inside the factory impedes lifecycle control and can leak connections. Accept an injected instance instead.
-export class MessagingModuleFactory {
- static createMessagingController(): MessagingController {
- const prisma = new PrismaClient();
- const messageRepository = new MessagePrismaRepository(prisma);
- const messagingService = new MessagingService(messageRepository, prisma);
- return new MessagingController(messagingService);
- }
-}
+export class MessagingModuleFactory {
+ static createMessagingController(prisma: PrismaClient): MessagingController {
+ const messageRepository = new MessagePrismaRepository(prisma);
+ const messagingService = new MessagingService(messageRepository, prisma);
+ return new MessagingController(messagingService);
+ }
+}Add graceful shutdown (app-level):
// e.g., index.ts
process.on("SIGINT", async () => { await prisma.$disconnect(); process.exit(0); });
process.on("SIGTERM", async () => { await prisma.$disconnect(); process.exit(0); });🤖 Prompt for AI Agents
In src/modules/messaging/factories/messaging.factory.ts around lines 8 to 12,
the factory currently instantiates a new PrismaClient inside the factory which
can leak connections and prevents proper lifecycle control; change the factory
to accept a PrismaClient instance from the caller (add a prisma: PrismaClient
parameter to the factory function), remove the new PrismaClient() call, pass the
injected prisma into MessagePrismaRepository and MessagingService, and return
the controller; do not add shutdown handlers here — ensure graceful shutdown
(calling prisma.$disconnect()) is implemented at the application entrypoint
(e.g., index.ts) on SIGINT/SIGTERM.
Comment on lines
+2
to
8
| import { MessagingModuleFactory } from "../factories/messaging.factory"; | ||
| import { authMiddleware } from "../../../middleware/authMiddleware"; | ||
| import { PrismaClient } from "@prisma/client"; | ||
|
|
||
| const router = Router(); | ||
| const prisma = new PrismaClient(); | ||
|
|
||
| const messageRepository = new MessagePrismaRepository(prisma); | ||
| const messagingService = new MessagingService(messageRepository, prisma); | ||
| const messagingController = new MessagingController(messagingService); | ||
| const messagingController = MessagingModuleFactory.createMessagingController(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid new PrismaClient per factory; inject app-scoped Prisma.
Factory currently creates its own Prisma client, risking excess connections and no shutdown disposal. Import the shared prisma and pass it to the factory.
-import { MessagingModuleFactory } from "../factories/messaging.factory";
+import { MessagingModuleFactory } from "../factories/messaging.factory";
+import { prisma } from "../../../config/prisma";
-const messagingController = MessagingModuleFactory.createMessagingController();
+const messagingController = MessagingModuleFactory.createMessagingController(prisma);And adjust the factory (see factory file comment) to accept a PrismaClient.
📝 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.
Suggested change
| import { MessagingModuleFactory } from "../factories/messaging.factory"; | |
| import { authMiddleware } from "../../../middleware/authMiddleware"; | |
| import { PrismaClient } from "@prisma/client"; | |
| const router = Router(); | |
| const prisma = new PrismaClient(); | |
| const messageRepository = new MessagePrismaRepository(prisma); | |
| const messagingService = new MessagingService(messageRepository, prisma); | |
| const messagingController = new MessagingController(messagingService); | |
| const messagingController = MessagingModuleFactory.createMessagingController(); | |
| import { MessagingModuleFactory } from "../factories/messaging.factory"; | |
| import { prisma } from "../../../config/prisma"; | |
| import { authMiddleware } from "../../../middleware/authMiddleware"; | |
| const router = Router(); | |
| const messagingController = MessagingModuleFactory.createMessagingController(prisma); |
🤖 Prompt for AI Agents
In src/modules/messaging/routes/messaging.routes.ts around lines 2 to 8, the
route file imports the MessagingModuleFactory which currently creates its own
PrismaClient; instead import the application-scoped Prisma instance (e.g., from
your shared prisma export) and pass that PrismaClient into the factory when
calling createMessagingController so the factory uses the injected client;
update the factory signature to accept a PrismaClient parameter and construct
controllers/services using that instance (do not instantiate a new PrismaClient
here).
Comment on lines
+12
to
+15
| const walletService = new WalletService(); | ||
| (walletService as any).walletRepository = walletRepository; | ||
| (walletService as any).verifyWalletUseCase = verifyWalletUseCase; | ||
| (walletService as any).validateWalletFormatUseCase = validateWalletFormatUseCase; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove any-cast field injection; use typed constructor injection.
Assigning private deps via (walletService as any) is unsafe and brittle. Inject dependencies via the WalletService constructor.
- const walletService = new WalletService();
- (walletService as any).walletRepository = walletRepository;
- (walletService as any).verifyWalletUseCase = verifyWalletUseCase;
- (walletService as any).validateWalletFormatUseCase = validateWalletFormatUseCase;
+ const walletService = new WalletService({
+ walletRepository,
+ verifyWalletUseCase,
+ validateWalletFormatUseCase,
+ });Outside this file, update WalletService to accept deps:
// WalletService.ts
export interface WalletServiceDeps {
walletRepository: HorizonWalletRepository; // or interface type
verifyWalletUseCase: VerifyWalletUseCase;
validateWalletFormatUseCase: ValidateWalletFormatUseCase;
}
export class WalletService {
constructor(private readonly deps: WalletServiceDeps) {}
// use: this.deps.verifyWalletUseCase, etc.
}🤖 Prompt for AI Agents
In src/modules/wallet/factories/wallet.factory.ts around lines 12-15 the factory
is assigning private dependencies via unsafe `(walletService as any)` casts;
change to constructor injection by updating WalletService to accept a typed
dependency object (WalletServiceDeps) containing walletRepository,
verifyWalletUseCase and validateWalletFormatUseCase, then instantiate here with
`new WalletService({ walletRepository, verifyWalletUseCase,
validateWalletFormatUseCase })`; also update imports/types in both files and
refactor WalletService internals to read deps from the constructor
(this.deps.*), and remove the any-cast assignments.
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.
🚀 Volunchain Pull Request
Mark with an x all the checkboxes that apply (like
[x])container.ts(legacy DI) and adopt module-scoped factories #184📌 Type of Change
📝 Changes Description
This PR removes the legacy global Dependency Injection container (
container.ts) and replaces it with explicit module-scoped factories, aligning with our Domain-Driven Design architecture.Key changes:
src/shared/infrastructure/container.tsand all its referencesauth.factory.ts,user.factory.ts,project.factory.ts, etc.)Benefits achieved:
📸 Evidence (Required)
Screenshot showing the new AuthModuleFactory implementation
Screenshot showing all tests passing with the new factory pattern
⏰ Time Spent Breakdown
Total: ~23 hours
🌌 Comments
This refactoring represents a significant improvement in our codebase's maintainability and adherence to clean architecture principles. The transition from global DI to explicit factories:
The changes are comprehensive but surgical — all existing functionality remains the same, with a much cleaner architecture underneath.
💙 Thank you for contributing to Volunchain!
We are glad you have chosen us as your project of choice and we hope you continue contributing so that together we can make our mark at the top!
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests