refactor: rename confusing origin getter to senderDomain in EventWrapper#329
refactor: rename confusing origin getter to senderDomain in EventWrapper#329Muawiya-contact wants to merge 3 commits intoRocketChat:mainfrom
Conversation
|
|
WalkthroughReplace several uses of event origin with explicit sender domain access and adjust origin accessors: add a senderDomain getter, make PersistentEventBase.origin return the PDU origin, change RoomState.origin to use createEvent.residentServer, update authorization checks to compare senderDomain, and alter federation/room service callsites accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/core/src/events/m.room.create.spec.ts`:
- Around line 112-113: The test is passing an undefined value because it uses
finalEvent.senderDomain (which doesn't exist) when calling createFromRawEvent;
replace that reference with the existing property finalEvent.origin (or derive
the domain from finalEvent.sender if that was intended) so the origin argument
passed to createFromRawEvent uses the correct value; update the call site that
references finalEvent.senderDomain to use finalEvent.origin (or adjust to
extract domain from finalEvent.sender) and run the tests.
In `@packages/core/src/events/m.room.member.spec.ts`:
- Around line 309-310: signedKickEvent is a plain object returned by signEvent,
so tests should access raw fields not getters; replace uses of
signedKickEvent.senderDomain with signedKickEvent.content?.sender ||
signedKickEvent.content?.user_id as appropriate (or compare
signedKickEvent.origin and/or signedKickEvent.content fields) and keep the
origin_server_ts assertion to check signedKickEvent.origin_server_ts directly;
update the assertions around senderDomain to reference the actual property
produced by signEvent (e.g., content.sender or sender) instead of relying on a
getter.
- Around line 477-478: The test is trying to use the senderDomain getter on
signedBanEvent which is a plain object; replace that usage by either
constructing a MatrixEvent wrapper (e.g., new MatrixEvent(signedBanEvent)) and
asserting its .senderDomain, or compute the domain directly from the plain
object's sender string (e.g., parse signedBanEvent.sender.split(':')[1]) and
compare that to serverName; keep the existing assertion for origin_server_ts
as-is.
- Around line 145-146: The test is asserting a non-existent property
`senderDomain` on the plain object returned by signEvent; update the assertion
to check the actual property present on the SignedEvent object (use
`signedLeaveEvent.origin`) instead of `signedLeaveEvent.senderDomain`. Locate
the test around the `signEvent` usage and replace the `senderDomain` expectation
with `origin`, keeping the `origin_server_ts` assertion as-is; this uses the
`signEvent` return value (SignedEvent<T>) rather than the PersistentEventBase
class getter.
🧹 Nitpick comments (2)
packages/room/src/manager/event-wrapper.ts (1)
128-139: Unreachable null check due toextractDomainFromIdthrowing behavior.The local
extractDomainFromIdfunction (lines 23-29) throws anErrorwhen no domain is found, so it never returns a falsy value. Theif (!domain)check on line 135-136 is unreachable dead code.🔧 Remove unreachable check
get senderDomain() { - const domain = extractDomainFromId(this.rawEvent.sender); - if (!domain) { - throw new Error('Invalid sender, no domain found'); - } - return domain; + return extractDomainFromId(this.rawEvent.sender); }packages/room/src/manager/room-state.ts (1)
112-128: Comment is now misleading after refactor.The comment on line 112 states "origin is the origin of the room gotten from the room id", but the implementation now uses
createEvent.senderDomainwhich is derived from the sender, not the room_id. While these should match for valid rooms (per Matrix authorization rules), the comment should be updated to reflect the actual implementation.Additionally,
senderDomainthrows if the domain is invalid, so theif (!origin)check on line 123 may be unreachable.📝 Update comment to match implementation
- // origin is the origin of the room gotten from the room id + // origin is the domain of the room creator (sender of m.room.create event) get origin() { const createEvent = getStateByMapKey(this.stateMap, { type: 'm.room.create', }); if (!createEvent) { throw new Error('Room create event not found'); } - const origin = createEvent.senderDomain; - if (!origin) { - throw new Error('Room create event has no origin'); - } - - return origin; + return createEvent.senderDomain; }
|
I have addressed the feedback from CodeRabbit in the latest commit ( |
| * This is metadata stored separately from the PDU and is used to track | ||
| * which server the event came from in federation scenarios. | ||
| */ | ||
| public eventOrigin?: string; |
There was a problem hiding this comment.
why is this new property needed?
There was a problem hiding this comment.
Great catch — you’re right, that extra property is not needed.
I removed eventOrigin and kept the refactor scoped strictly to naming semantics:
senderDomainis used only for sender-domain checks (authentication logic).originremains the federation/server-origin accessor for compatibility.
This keeps the behavior clear without introducing extra mutable state in PersistentEventBase.
|
@sampaiodiego I removed
I also resolved the rebase conflict cleanly, ran a full build, and confirmed that all packages compile successfully. Ready for final review and approval. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/room/src/manager/event-wrapper.ts (2)
123-124: Stale TODO comment should be removed.
// TODO: This should be removed or different name used instead?was presumably written while debating how to rename the oldorigingetter. Now that the rename tosenderDomainis complete, the comment is ambiguous and misleading — it implies the new getter itself is uncertain. It should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/room/src/manager/event-wrapper.ts` around lines 123 - 124, Remove the stale TODO comment "// TODO: This should be removed or different name used instead?" from event-wrapper.ts near the origin/senderDomain code; the rename to senderDomain is complete, so delete that ambiguous comment (search for the origin getter or senderDomain getter in EventWrapper to locate the comment) and ensure no other TODO implies uncertainty about the new getter name.
125-131:if (!domain)guard insenderDomainis dead code.The local
extractDomainFromId(line 23–29) throws immediately when no colon is found — it never returnsundefined. TypeScript infers its return asstring, so theif (!domain)branch at line 127 can never be entered; it is unreachable dead code.Note that the
extractDomainFromIdexported frompackages/federation-sdk/src/index.tsdoes returnstring | undefined, but that is a different function — this getter calls the local one.🧹 Proposed cleanup
get senderDomain() { - const domain = extractDomainFromId(this.rawEvent.sender); - if (!domain) { - throw new Error('Invalid sender, no domain found'); - } - return domain; + return extractDomainFromId(this.rawEvent.sender); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/room/src/manager/event-wrapper.ts` around lines 125 - 131, The guard in the senderDomain getter is unreachable because the local helper extractDomainFromId already throws when no colon is found; remove the if (!domain) branch and simply return the domain from the getter (or alternatively, if you intended to use the exported extractDomainFromId that returns string | undefined, replace the call with that exported function and keep the guard). Update the senderDomain getter to either (A) return the local extractDomainFromId(this.rawEvent.sender) directly and drop the dead throw, or (B) import and call the packages/federation-sdk extractDomainFromId and retain the existing null-check; adjust imports accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/room/src/manager/event-wrapper.tspackages/room/src/manager/room-state.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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/room/src/manager/room-state.ts
🧬 Code graph analysis (1)
packages/room/src/manager/event-wrapper.ts (1)
packages/federation-sdk/src/index.ts (1)
extractDomainFromId(143-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (1)
packages/room/src/manager/room-state.ts (1)
117-122: The getter itself guarantees a non-null result. TheresidentServerproperty inevent-wrapper.tsincludes internal guarding:get residentServer() { const residentServer = extractDomainFromId(this.rawEvent.room_id); if (!residentServer) { throw new Error('Invalid room_id, no domain found'); } return residentServer; }The getter throws if domain extraction fails, so the property either returns a valid string or throws an error—it never returns
undefined. The code at lines 117–122 is safe without an additional guard. No action needed.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/room/src/manager/event-wrapper.ts`:
- Around line 133-135: Update the get origin() getter to include an explicit
return type annotation of string | undefined (i.e. declare it as get origin():
string | undefined) so callers know origin can be absent, and then remove the
dead if (!domain) guard inside the senderDomain getter (which uses
extractDomainFromId()) and either delete or update the stale TODO above
senderDomain to reflect that origin was removed from the PDU schema; this keeps
types explicit (origin -> this.rawEvent.origin) and eliminates unreachable code
in senderDomain.
---
Nitpick comments:
In `@packages/room/src/manager/event-wrapper.ts`:
- Around line 123-124: Remove the stale TODO comment "// TODO: This should be
removed or different name used instead?" from event-wrapper.ts near the
origin/senderDomain code; the rename to senderDomain is complete, so delete that
ambiguous comment (search for the origin getter or senderDomain getter in
EventWrapper to locate the comment) and ensure no other TODO implies uncertainty
about the new getter name.
- Around line 125-131: The guard in the senderDomain getter is unreachable
because the local helper extractDomainFromId already throws when no colon is
found; remove the if (!domain) branch and simply return the domain from the
getter (or alternatively, if you intended to use the exported
extractDomainFromId that returns string | undefined, replace the call with that
exported function and keep the guard). Update the senderDomain getter to either
(A) return the local extractDomainFromId(this.rawEvent.sender) directly and drop
the dead throw, or (B) import and call the packages/federation-sdk
extractDomainFromId and retain the existing null-check; adjust imports
accordingly.
| get origin() { | ||
| return this.rawEvent.origin; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, read the event-wrapper.ts file to verify the code structure
wc -l packages/room/src/manager/event-wrapper.tsRepository: RocketChat/homeserver
Length of output: 111
🏁 Script executed:
# Read the relevant sections mentioned in the review
sed -n '40,80p' packages/room/src/manager/event-wrapper.tsRepository: RocketChat/homeserver
Length of output: 1173
🏁 Script executed:
# Read lines around 125-135 to see the getters
sed -n '120,140p' packages/room/src/manager/event-wrapper.tsRepository: RocketChat/homeserver
Length of output: 569
🏁 Script executed:
# Check the type definition around line 45-47
sed -n '40,50p' packages/room/src/manager/event-wrapper.tsRepository: RocketChat/homeserver
Length of output: 355
🏁 Script executed:
# Search for all .origin usages in the codebase
rg -n '\.origin\b' --type ts --glob '!**/*.test.ts' --glob '!**/*.spec.ts' -C2Repository: RocketChat/homeserver
Length of output: 2623
🏁 Script executed:
# Check the extractDomainFromId function implementation at lines 23-29
sed -n '20,35p' packages/room/src/manager/event-wrapper.tsRepository: RocketChat/homeserver
Length of output: 530
🏁 Script executed:
# Check if there are any .origin usages specifically in the room packages
rg -n '\.origin\b' packages/room --type ts -C2Repository: RocketChat/homeserver
Length of output: 351
🏁 Script executed:
# Look for the imports to understand which extractDomainFromId is used
sed -n '1,25p' packages/room/src/manager/event-wrapper.tsRepository: RocketChat/homeserver
Length of output: 801
🏁 Script executed:
# Check staging-area.service.ts to see what event.origin expects
sed -n '195,225p' packages/federation-sdk/src/services/staging-area.service.tsRepository: RocketChat/homeserver
Length of output: 847
🏁 Script executed:
# Check what type event.origin is expected to be in the call sites
sed -n '195,240p' packages/federation-sdk/src/services/staging-area.service.tsRepository: RocketChat/homeserver
Length of output: 1253
🏁 Script executed:
# Check the federationService.getMissingEvents signature to see if it expects origin to be optional
rg -n 'getMissingEvents' packages/federation-sdk/src --type ts -A 5 | head -40Repository: RocketChat/homeserver
Length of output: 3070
🏁 Script executed:
# Check state.service.ts to see how origin is used
sed -n '120,135p' packages/federation-sdk/src/services/state.service.tsRepository: RocketChat/homeserver
Length of output: 435
🌐 Web query:
Matrix PDU origin field removed spec v1.12
💡 Result:
In the Matrix spec, the origin field was removed from the Server‑Server (federation) PDU schema earlier (documented as a spec clarification: “Remove largely unused origin field from PDUs.”) in Matrix spec v1.4. [1]
By Matrix spec v1.12, the change you’re seeing is just a documentation/example fix: the spec removes origin from a PDU example because it no longer exists in the schema (“Remove origin field from PDU example because it doesn't exist in the schema anymore.”). [2]
Sources:
[1] Matrix Specification v1.4 changelog (Server‑Server API spec clarifications) https://spec.matrix.org/v1.4/changelog/
[2] Matrix v1.12 release notes (Server‑Server API spec clarifications) https://matrix.org/blog/2024/10/07/matrix-v1.12-release/
get origin() lacks an explicit return type annotation — changed from string to string | undefined.
The previous implementation derived origin from the always-defined sender field, returning a string. Now it returns this.rawEvent.origin, which is typed as string | undefined (the PduWithFederationOrigin type declares origin? as optional). Without an explicit return type annotation, TypeScript infers string | undefined silently. Any call site that consumed origin as a non-nullable string could receive undefined at runtime.
This modeling is correct — the origin field was removed from the PDU schema in Matrix spec v1.4, so modern PDUs often omit it. However, the type change should be explicit in the code.
Add the explicit annotation:
📝 Proposed fix
- get origin() {
+ get origin(): string | undefined {
return this.rawEvent.origin;
}Additionally, the if (!domain) guard at line 127 in the adjacent get senderDomain() getter is dead code — the local extractDomainFromId() function always throws on invalid input and never returns undefined, so the check will never execute. Consider removing it or clarifying intent.
Also remove or clarify the stale TODO comment at line 123 above senderDomain — it appears to have been intended for the old origin logic before the refactor.
📝 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.
| get origin() { | |
| return this.rawEvent.origin; | |
| } | |
| get origin(): string | undefined { | |
| return this.rawEvent.origin; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/room/src/manager/event-wrapper.ts` around lines 133 - 135, Update
the get origin() getter to include an explicit return type annotation of string
| undefined (i.e. declare it as get origin(): string | undefined) so callers
know origin can be absent, and then remove the dead if (!domain) guard inside
the senderDomain getter (which uses extractDomainFromId()) and either delete or
update the stale TODO above senderDomain to reflect that origin was removed from
the PDU schema; this keeps types explicit (origin -> this.rawEvent.origin) and
eliminates unreachable code in senderDomain.
3 participants
Description
This PR addresses the
TODOinevent-wrapper.tsregarding the naming of theorigingetter.Previously,
EventWrapper.originwas extracting the domain from the Sender's ID (e.g.,@user:domain.com). In the Matrix protocol, "Origin" refers specifically to the server that created the event. This naming was misleading and has been clarified.Changes
EventWrapper: Renamed the domain extractor tosenderDomain.originproperty to return the actualrawEvent.origin, maintaining alignment with the Matrix specification andPersistentEventBaseinterface.senderDomainproperty where appropriate.Testing
bun run build(FULL TURBO).Summary by CodeRabbit