fix: prevent add prev_event or auth_events if the initial events are already informed #248
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #248 +/- ##
=======================================
Coverage 81.84% 81.85%
=======================================
Files 63 63
Lines 4705 4717 +12
=======================================
+ Hits 3851 3861 +10
- Misses 854 856 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughConditional logic was added to only add auth and previous events when missing during event construction. EventWrapper now tracks auth_events and prev_events via Sets, updates accessors to return arrays from these Sets, and adjusts mutation methods to operate on Sets instead of raw arrays. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant S as StateService
participant EW as EventWrapper
C->>S: buildEvent(instance)
S->>EW: inspect event.auth_events / prev_events
alt Missing auth/prev events
S->>S: addAuthEvents(instance)
S->>S: addPrevEvents(instance)
else Already present
S->>S: Skip population
end
S-->>C: Built event
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull Request Overview
Fix ensures auth_events and prev_events are not redundantly appended when they are already provided on incoming events by introducing internal Sets to track unique IDs and adding conditional population logic in state handling.
- Introduces authEventsIds and prevEventsIds Sets to enforce uniqueness and prevent duplicate additions.
- Skips addAuthEvents/addPrevEvents in StateService when events are already present.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/room/src/manager/event-wrapper.ts | Adds internal Sets for auth/prev event IDs and updates methods to use them instead of mutating rawEvent arrays. |
| packages/federation-sdk/src/services/state.service.ts | Conditionally invokes population of auth/prev events based on existing event data. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| } | ||
| if (this.rawEvent.prev_events) { | ||
| for (const id of this.rawEvent.prev_events) { | ||
| this.prevEventsIds.add(id); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The constructor copies auth_events and prev_events into new Sets, creating two sources of truth (rawEvent.auth_events / rawEvent.prev_events vs authEventsIds / prevEventsIds) without marking the originals as deprecated or keeping them in sync. Consider either (a) normalizing by replacing rawEvent.auth_events/prev_events with the Set-backed arrays at creation time, (b) updating the rawEvent arrays whenever the Sets change, or (c) encapsulating access so external code cannot read stale rawEvent.* arrays.
| } | |
| } | |
| if (this.rawEvent.prev_events) { | |
| for (const id of this.rawEvent.prev_events) { | |
| this.prevEventsIds.add(id); | |
| } | |
| } | |
| } | |
| } | |
| delete this.rawEvent.auth_events; | |
| } | |
| if (this.rawEvent.prev_events) { | |
| for (const id of this.rawEvent.prev_events) { | |
| this.prevEventsIds.add(id); | |
| } | |
| delete this.rawEvent.prev_events; | |
| } | |
| } | |
| /** | |
| * Returns the current list of auth event IDs as an array. | |
| */ | |
| getAuthEventsArray(): EventID[] { | |
| return Array.from(this.authEventsIds); | |
| } | |
| /** | |
| * Returns the current list of prev event IDs as an array. | |
| */ | |
| getPrevEventsArray(): EventID[] { | |
| return Array.from(this.prevEventsIds); | |
| } |
| for (const event of events) { | ||
| this.prevEventsIds.add(event.eventId); | ||
| } |
There was a problem hiding this comment.
addPrevEvents and authedBy now update only the Sets and no longer mutate rawEvent.prev_events or rawEvent.auth_events, which can leave rawEvent.* arrays stale if any other code paths still access them directly. To avoid divergence, either also update rawEvent.* arrays here, or refactor remaining internal/external usages to always go through the Set-backed accessors and document rawEvent.* as legacy.
|
|
||
| authedBy(event: PersistentEventBase<Version>) { | ||
| this.rawEvent.auth_events.push(event.eventId); | ||
| this.authEventsIds.add(event.eventId); |
There was a problem hiding this comment.
addPrevEvents and authedBy now update only the Sets and no longer mutate rawEvent.prev_events or rawEvent.auth_events, which can leave rawEvent.* arrays stale if any other code paths still access them directly. To avoid divergence, either also update rawEvent.* arrays here, or refactor remaining internal/external usages to always go through the Set-backed accessors and document rawEvent.* as legacy.
| await Promise.all([ | ||
| this.addAuthEvents(instance), | ||
| this.addPrevEvents(instance), | ||
| instance.event.auth_events.length === 0 && this.addAuthEvents(instance), | ||
| instance.event.prev_events.length === 0 && this.addPrevEvents(instance), | ||
| ]); |
There was a problem hiding this comment.
Using logical && inside Promise.all results in boolean false entries when conditions fail, which is unconventional and can obscure intent. Recommend building the promise list explicitly: const tasks = []; if (instance.event.auth_events.length === 0) tasks.push(this.addAuthEvents(instance)); if (instance.event.prev_events.length === 0) tasks.push(this.addPrevEvents(instance)); await Promise.all(tasks);
ggazzo
changed the title
…lready informed
Summary by CodeRabbit
Bug Fixes
Refactor