feat: Add application service (bridges) support#394
Open
sampaiodiego wants to merge 89 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a new ChangesApplication Service Bridge Integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EduService
participant EventRouterService
participant NamespaceMatcherService
participant TransactionSenderService
participant Bridge
EduService->>EventRouterService: routeEphemeral(typing|presence|receipt EDU)
EventRouterService->>NamespaceMatcherService: getInterestedAppServices(...)
NamespaceMatcherService-->>EventRouterService: interested appservices
EventRouterService->>TransactionSenderService: sendTransaction(appservice, batch)
TransactionSenderService->>Bridge: PUT /_matrix/app/v1/transactions/{txnId}
Bridge-->>TransactionSenderService: response
sequenceDiagram
participant Client
participant HomeserverController
participant resolveAppServiceAuth
participant FederationSDK
Client->>HomeserverController: appservice-authenticated request
HomeserverController->>resolveAppServiceAuth: resolveAppServiceAuth(...)
resolveAppServiceAuth->>FederationSDK: getRegistrationByAsToken(token)
FederationSDK-->>resolveAppServiceAuth: registration
resolveAppServiceAuth-->>HomeserverController: auth result or error
HomeserverController-->>Client: Matrix response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 #394 +/- ##
==========================================
+ Coverage 50.64% 51.16% +0.51%
==========================================
Files 101 113 +12
Lines 11533 12734 +1201
==========================================
+ Hits 5841 6515 +674
- Misses 5692 6219 +527 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sampaiodiego
force-pushed
the
add-application-service-support
branch
from
1fa0373 to
4ee3063
Compare
…ing batch handling
… handling in repository and service
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/appservice/src/services/transaction-sender.service.ts (1)
236-245: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTransport failures are logged/recorded as
HTTP undefined.When
fetch()fails at the transport layer it resolves withok: falseandstatus: undefined(it does not throw), so this branch — not thecatch— runs, loggingstatus: undefinedand callingmarkDown(..., 'HTTP undefined'). The down-marking is correct, but the reason is misleading for the common connection-failure case. Consider distinguishingresponse.status === undefined(connection failed) from a genuine non-2xx, mirroringping.service.ts.🪵 Suggested distinction
- this.logger.warn({ - msg: `Transaction delivery failed`, - asId: registration._id, - txnId, - status: response.status, - }); - - await this.txnRepo.markFailed(registration._id, txnId); - await this.stateRepo.markDown(registration._id, `HTTP ${response.status}`); - return false; + const reason = response.status === undefined ? 'Failed to connect to appservice' : `HTTP ${response.status}`; + this.logger.warn({ msg: 'Transaction delivery failed', asId: registration._id, txnId, status: response.status, reason }); + + await this.txnRepo.markFailed(registration._id, txnId); + await this.stateRepo.markDown(registration._id, reason); + return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/appservice/src/services/transaction-sender.service.ts` around lines 236 - 245, The failure handling in transaction-sender.service.ts logs and marks transport errors as HTTP undefined, which is misleading when fetch() returns a response with status unset. Update the non-ok response branch around the logger.warn, txnRepo.markFailed, and stateRepo.markDown calls to distinguish response.status === undefined from a real HTTP status, similar to ping.service.ts. Use a clearer reason string for connection/transport failures while keeping genuine non-2xx responses recorded with their actual HTTP status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/appservice/src/services/transaction-sender.service.ts`:
- Around line 236-245: The failure handling in transaction-sender.service.ts
logs and marks transport errors as HTTP undefined, which is misleading when
fetch() returns a response with status unset. Update the non-ok response branch
around the logger.warn, txnRepo.markFailed, and stateRepo.markDown calls to
distinguish response.status === undefined from a real HTTP status, similar to
ping.service.ts. Use a clearer reason string for connection/transport failures
while keeping genuine non-2xx responses recorded with their actual HTTP status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d5d88df-77c2-4d0a-b4cd-cc7a1ff75e11
📒 Files selected for processing (21)
docs/bridge-architecture-guide.mdpackages/appservice/src/index.tspackages/appservice/src/models/appservice.model.tspackages/appservice/src/repositories/appservice-state.repository.tspackages/appservice/src/repositories/appservice-txn.repository.tspackages/appservice/src/services/event-router.service.spec.tspackages/appservice/src/services/event-router.service.tspackages/appservice/src/services/namespace-matcher.service.spec.tspackages/appservice/src/services/namespace-matcher.service.tspackages/appservice/src/services/ping.service.tspackages/appservice/src/services/registration.service.tspackages/appservice/src/services/transaction-sender.service.tspackages/core/src/utils/fetch.tspackages/federation-sdk/src/index.tspackages/federation-sdk/src/repositories/room-alias.repository.tspackages/federation-sdk/src/services/directory.service.tspackages/federation-sdk/src/services/edu.service.tspackages/federation-sdk/src/services/event-sender.service.spec.tspackages/federation-sdk/src/services/event-sender.service.tspackages/federation-sdk/src/services/message.service.tspackages/federation-sdk/src/services/room.service.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/appservice/src/index.ts
- packages/federation-sdk/src/services/directory.service.ts
- packages/appservice/src/repositories/appservice-txn.repository.ts
- packages/appservice/src/services/event-router.service.spec.ts
- packages/appservice/src/models/appservice.model.ts
- packages/federation-sdk/src/services/edu.service.ts
- packages/federation-sdk/src/services/event-sender.service.spec.ts
- packages/appservice/src/repositories/appservice-state.repository.ts
- packages/appservice/src/services/registration.service.ts
- packages/federation-sdk/src/services/event-sender.service.ts
- packages/appservice/src/services/namespace-matcher.service.spec.ts
- packages/appservice/src/services/namespace-matcher.service.ts
- packages/federation-sdk/src/services/room.service.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Code Quality Checks(lint, test, tsc)
🔇 Additional comments (8)
packages/appservice/src/services/ping.service.ts (1)
52-74: LGTM!packages/federation-sdk/src/repositories/room-alias.repository.ts (1)
25-35: LGTM!docs/bridge-architecture-guide.md (1)
115-131: LGTM!packages/federation-sdk/src/index.ts (1)
156-192: LGTM!packages/core/src/utils/fetch.ts (1)
123-184: LGTM!Also applies to: 233-248
packages/appservice/src/services/event-router.service.ts (1)
30-68: LGTM!packages/appservice/src/services/transaction-sender.service.ts (1)
60-87: LGTM!Also applies to: 149-200
packages/federation-sdk/src/services/message.service.ts (1)
54-54: LGTM!Also applies to: 128-128, 182-182, 262-262, 289-289, 334-334, 372-372
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 17 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/federation-sdk/src/index.ts">
<violation number="1" location="packages/federation-sdk/src/index.ts:192">
P2: `init()` starts a background retry scheduler via `transactionSender.startRetryScheduler()`, but there is no corresponding teardown path exposed. The underlying `setInterval` in `TransactionSenderService` is not `.unref()`'d, so it will keep the Node process alive indefinitely. In tests, workers, or short-lived scripts this prevents clean exit, and the existing `stopRetryScheduler()` method is never called. Consider either exposing a shutdown hook from `init()` that calls `stopRetryScheduler()`, or `.unref()`'ing the timer so it does not block process exit.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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
2 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