refactor: rename confusing origin getter to senderDomain in EventWrapper #329
+33
−20
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
|
|
Contributor
WalkthroughThis PR refactors event origin tracking by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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}
Contributor
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; }
Author
|
I have addressed the feedback from CodeRabbit in the latest commit ( |
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.
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
✏️ Tip: You can customize this high-level summary in your review settings.