refactor: use delay to inject repositories#293
Merged
Conversation
Contributor
WalkthroughThis PR refactors dependency injection patterns across federation SDK services, converting several dependencies to lazy/delayed injection using tsyringe decorators, removing unused dependencies from multiple services, and relocating EventService's startup invocation from the service to the SDK bootstrap flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Bootstrap as SDK Bootstrap<br/>(index.ts)
participant Container as tsyringe Container
participant EventSvc as EventService
participant DB as Database
rect rgb(230, 240, 250)
Note over Bootstrap,DB: Before: Internal Startup in EventService
Bootstrap->>Container: Initialize & register
Container->>EventSvc: Instantiate (constructor)
EventSvc->>EventSvc: setTimeout(...processOldStagedEvents)
EventSvc->>DB: processOldStagedEvents (async)
end
rect rgb(240, 250, 230)
Note over Bootstrap,DB: After: Deferred Startup in Bootstrap
Bootstrap->>Container: Initialize & register
Bootstrap->>EventSvc: (no immediate processing)
Bootstrap->>Bootstrap: setTimeout(5s)
Bootstrap->>Container: Resolve EventService
Container->>EventSvc: Return instance
Bootstrap->>EventSvc: processOldStagedEvents()
EventSvc->>DB: processOldStagedEvents (async)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
==========================================
- Coverage 60.47% 60.46% -0.02%
==========================================
Files 67 67
Lines 6670 6672 +2
==========================================
Hits 4034 4034
- Misses 2636 2638 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
packages/federation-sdk/src/index.ts(2 hunks)packages/federation-sdk/src/sdk.ts(2 hunks)packages/federation-sdk/src/services/edu.service.ts(0 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(2 hunks)packages/federation-sdk/src/services/event-fetcher.service.ts(2 hunks)packages/federation-sdk/src/services/event.service.ts(3 hunks)packages/federation-sdk/src/services/federation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(1 hunks)packages/federation-sdk/src/services/message.service.ts(0 hunks)packages/federation-sdk/src/services/profiles.service.ts(0 hunks)packages/federation-sdk/src/services/room.service.ts(2 hunks)packages/federation-sdk/src/services/send-join.service.ts(0 hunks)packages/federation-sdk/src/services/server.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(2 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/federation-sdk/src/services/edu.service.ts
- packages/federation-sdk/src/services/message.service.ts
- packages/federation-sdk/src/services/profiles.service.ts
- packages/federation-sdk/src/services/send-join.service.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/state.service.tspackages/federation-sdk/src/services/invite.service.tspackages/federation-sdk/src/services/event.service.tspackages/federation-sdk/src/services/room.service.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/server.service.ts (2)
packages/federation-sdk/src/services/event.service.ts (1)
singleton(43-884)packages/federation-sdk/src/services/room.service.ts (1)
singleton(43-1559)
🔇 Additional comments (5)
packages/homeserver/src/homeserver.module.ts (1)
93-96: Verify the asymmetric default behavior is intentional.The two EDU feature flags have different default behaviors:
processTypingdefaults to enabled (opt-out: disabled only whenEDU_PROCESS_TYPING === 'false')processPresencedefaults to disabled (opt-in: enabled only whenEDU_PROCESS_PRESENCE === 'true')This asymmetry may be intentional based on feature maturity or stability concerns, but please confirm this is the desired behavior rather than an oversight.
</review_comment_end -->
packages/federation-sdk/src/services/invite.service.ts (1)
35-40: EventService removal is verified as safe.The verification confirms that EventService had no references in the file and was not needed. EventAuthorizationService is properly used elsewhere in the service (line 218), and the constructor change is correct.
packages/federation-sdk/src/services/event-authorization.service.ts (1)
12-12: LGTM! Delayed injection applied correctly.The delayed injection pattern for
UploadRepositoryis appropriate here since the repository is only accessed in methods (e.g.,canAccessMediaat line 355) and not during construction. This pattern helps avoid potential circular dependency issues.Also applies to: 35-36
packages/federation-sdk/src/services/event.service.ts (2)
26-26: LGTM! Delayed injection correctly applied to repositories.The delayed injection pattern for
EventRepository,EventStagingRepository, andLockRepositoryis correctly implemented. Since these repositories are only accessed within methods (not during construction), the delayed resolution is safe and helps manage potential circular dependencies.Also applies to: 55-61
669-669: No action required —processOldStagedEvents()is correctly called during SDK bootstrap.Verified that the method is invoked in
packages/federation-sdk/src/index.ts:328within the SDK initialization flow, 5 seconds after database initialization completes. The method visibility change from private to public is properly utilized.
Comment on lines
+324
to
+329
|
|
||
| // once the db is initialized we look for old staged events and try to process them | ||
| setTimeout(async () => { | ||
| const eventService = container.resolve(EventService); | ||
| await eventService.processOldStagedEvents(); | ||
| }, 5000); |
Contributor
There was a problem hiding this comment.
Consider improving the initialization pattern for processing old staged events.
The hard-coded 5-second delay and lack of error handling present several concerns:
- Hard-coded delay: The 5000ms timeout is arbitrary. If the database initialization takes longer than expected, this could still execute too early.
- Missing error handling: The async callback doesn't catch errors from
processOldStagedEvents(), which could lead to silent failures. - Fire-and-forget pattern: The Promise returned by the async function is not awaited, so errors won't surface to callers.
Consider one of these alternatives:
Option 1: Make it part of the initialization flow
// this is required to initialize the listener and register the queue handler
container.resolve(StagingAreaListener);
-
- // once the db is initialized we look for old staged events and try to process them
- setTimeout(async () => {
- const eventService = container.resolve(EventService);
- await eventService.processOldStagedEvents();
- }, 5000);
+
+ // Process old staged events after DB initialization
+ try {
+ const eventService = container.resolve(EventService);
+ await eventService.processOldStagedEvents();
+ } catch (error) {
+ // Log error but don't block initialization
+ console.error('Failed to process old staged events:', error);
+ }
}Option 2: If the delay is truly necessary, make it configurable and add error handling
- // once the db is initialized we look for old staged events and try to process them
- setTimeout(async () => {
- const eventService = container.resolve(EventService);
- await eventService.processOldStagedEvents();
- }, 5000);
+ // Process old staged events after allowing time for other services to initialize
+ const processingDelay = process.env.STAGED_EVENTS_DELAY_MS || 5000;
+ setTimeout(async () => {
+ try {
+ const eventService = container.resolve(EventService);
+ await eventService.processOldStagedEvents();
+ } catch (error) {
+ console.error('Failed to process old staged events:', error);
+ }
+ }, processingDelay);📝 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
| // once the db is initialized we look for old staged events and try to process them | |
| setTimeout(async () => { | |
| const eventService = container.resolve(EventService); | |
| await eventService.processOldStagedEvents(); | |
| }, 5000); | |
| // Process old staged events after allowing time for other services to initialize | |
| const processingDelay = process.env.STAGED_EVENTS_DELAY_MS || 5000; | |
| setTimeout(async () => { | |
| try { | |
| const eventService = container.resolve(EventService); | |
| await eventService.processOldStagedEvents(); | |
| } catch (error) { | |
| console.error('Failed to process old staged events:', error); | |
| } | |
| }, processingDelay); |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/index.ts around lines 324 to 329, the current
setTimeout with a hard-coded 5000ms and an unhandled async callback risks
running before DB is ready and will swallow errors; change this so processing
old staged events is part of the initialization flow (preferred) by invoking
await eventService.processOldStagedEvents() from the async init function after
the DB init completes and propagate or log errors, or if a delay is truly
required, replace the hard-coded setTimeout with a configurable retry/delay
mechanism and wrap the call in try/catch that logs and rethrows or reports
failures, and ensure callers can await the returned Promise instead of
fire-and-forgetting it.
ggazzo
approved these changes
Nov 3, 2025
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
3 participants
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.
Summary by CodeRabbit
New Features
Refactor