Skip to content

Commit

Permalink
Aggregate relations regardless of whether event fits into the timeline (
Browse files Browse the repository at this point in the history 
#3496)
  • Loading branch information
@t3chguy
t3chguy authored Jun 26, 2023
1 parent f884c78 commit f16a6bc
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 29 deletions.
29 changes: 29 additions & 0 deletions spec/unit/event-timeline-set.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { mocked } from "jest-mock";

import * as utils from "../test-utils/test-utils";
import {
DuplicateStrategy,
Expand Down Expand Up @@ -160,6 +162,33 @@ describe("EventTimelineSet", () => {
eventTimelineSet.addEventToTimeline(messageEvent, liveTimeline, true, false);
}).not.toThrow();
});

it("should aggregate relations which belong to unknown timeline without adding them to any timeline", () => {
// If threads are disabled all events go into the main timeline
mocked(client.supportsThreads).mockReturnValue(true);
const reactionEvent = utils.mkReaction(messageEvent, client, client.getSafeUserId(), roomId);

const liveTimeline = eventTimelineSet.getLiveTimeline();
expect(liveTimeline.getEvents().length).toStrictEqual(0);
eventTimelineSet.addEventToTimeline(reactionEvent, liveTimeline, {
toStartOfTimeline: true,
});
expect(liveTimeline.getEvents().length).toStrictEqual(0);

eventTimelineSet.addEventToTimeline(messageEvent, liveTimeline, {
toStartOfTimeline: true,
});
expect(liveTimeline.getEvents()).toHaveLength(1);
const [event] = liveTimeline.getEvents();
const reactions = eventTimelineSet.relations!.getChildEventsForEvent(
event.getId()!,
"m.annotation",
"m.reaction",
)!;
const relations = reactions.getRelations();
expect(relations).toHaveLength(1);
expect(relations[0].getId()).toBe(reactionEvent.getId());
});
});

describe("addEventToTimeline (thread timeline)", () => {
Expand Down
20 changes: 10 additions & 10 deletions src/models/event-timeline-set.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -721,13 +721,17 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
"in timelineSet(threadId=${this.thread?.id})`);
}

const eventId = event.getId()!;
this.relations.aggregateParentEvent(event);
this.relations.aggregateChildEvent(event, this);

// Make sure events don't get mixed in timelines they shouldn't be in (e.g. a
// threaded message should not be in the main timeline).
//
// We can only run this check for timelines with a `room` because `canContain`
// requires it
if (this.room && !this.canContain(event)) {
let eventDebugString = `event=${event.getId()}`;
let eventDebugString = `event=${eventId}`;
if (event.threadRootId) {
eventDebugString += `(belongs to thread=${event.threadRootId})`;
}
Expand All @@ -738,17 +742,13 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
return;
}

const eventId = event.getId()!;
timeline.addEvent(event, {
toStartOfTimeline,
roomState,
timelineWasEmpty,
});
this._eventIdToTimeline.set(eventId, timeline);

this.relations.aggregateParentEvent(event);
this.relations.aggregateChildEvent(event, this);

const data: IRoomTimelineData = {
timeline: timeline,
liveEvent: !toStartOfTimeline && timeline == this.liveTimeline && !fromCache,
Expand Down Expand Up @@ -782,13 +782,17 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
"in timelineSet(threadId=${this.thread?.id})`);
}

const eventId = event.getId()!;
this.relations.aggregateParentEvent(event);
this.relations.aggregateChildEvent(event, this);

// Make sure events don't get mixed in timelines they shouldn't be in (e.g. a
// threaded message should not be in the main timeline).
//
// We can only run this check for timelines with a `room` because `canContain`
// requires it
if (this.room && !this.canContain(event)) {
let eventDebugString = `event=${event.getId()}`;
let eventDebugString = `event=${eventId}`;
if (event.threadRootId) {
eventDebugString += `(belongs to thread=${event.threadRootId})`;
}
Expand Down Expand Up @@ -830,13 +834,9 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
// If we got to the end of the loop, insertIndex points at the end of
// the list.

const eventId = event.getId()!;
timeline.insertEvent(event, insertIndex, roomState);
this._eventIdToTimeline.set(eventId, timeline);

this.relations.aggregateParentEvent(event);
this.relations.aggregateChildEvent(event, this);

const data: IRoomTimelineData = {
timeline: timeline,
liveEvent: timeline == this.liveTimeline,
Expand Down
19 changes: 0 additions & 19 deletions src/models/room.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2103,7 +2103,6 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
threadId?: string;
} {
if (!this.client?.supportsThreads()) {
logger.debug(`Room::eventShouldLiveIn: eventId=${event.getId()} client does not support threads`);
return {
shouldLiveInRoom: true,
shouldLiveInThread: false,
Expand All @@ -2112,11 +2111,6 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {

// A thread root is always shown in both timelines
if (event.isThreadRoot || roots?.has(event.getId()!)) {
if (event.isThreadRoot) {
logger.debug(`Room::eventShouldLiveIn: eventId=${event.getId()} isThreadRoot is true`);
} else {
logger.debug(`Room::eventShouldLiveIn: eventId=${event.getId()} is a known thread root`);
}
return {
shouldLiveInRoom: true,
shouldLiveInThread: true,
Expand All @@ -2127,9 +2121,6 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
// A thread relation (1st and 2nd order) is always only shown in a thread
const threadRootId = event.threadRootId;
if (threadRootId != undefined) {
logger.debug(
`Room::eventShouldLiveIn: eventId=${event.getId()} threadRootId=${threadRootId} is part of a thread`,
);
return {
shouldLiveInRoom: false,
shouldLiveInThread: true,
Expand All @@ -2141,9 +2132,6 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
let parentEvent: MatrixEvent | undefined;
if (parentEventId) {
parentEvent = this.findEventById(parentEventId) ?? events?.find((e) => e.getId() === parentEventId);
logger.debug(
`Room::eventShouldLiveIn: eventId=${event.getId()} parentEventId=${parentEventId} found=${!!parentEvent}`,
);
}

// Treat relations and redactions as extensions of their parents so evaluate parentEvent instead
Expand All @@ -2152,7 +2140,6 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
}

if (!event.isRelation()) {
logger.debug(`Room::eventShouldLiveIn: eventId=${event.getId()} not a relation`);
return {
shouldLiveInRoom: true,
shouldLiveInThread: false,
Expand All @@ -2161,11 +2148,6 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {

// Edge case where we know the event is a relation but don't have the parentEvent
if (roots?.has(event.relationEventId!)) {
logger.debug(
`Room::eventShouldLiveIn: eventId=${event.getId()} relationEventId=${
event.relationEventId
} is a known root`,
);
return {
shouldLiveInRoom: true,
shouldLiveInThread: true,
Expand All @@ -2176,7 +2158,6 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
// We've exhausted all scenarios,
// we cannot assume that it lives in the main timeline as this may be a relation for an unknown thread
// adding the event in the wrong timeline causes stuck notifications and can break ability to send read receipts
logger.debug(`Room::eventShouldLiveIn: eventId=${event.getId()} belongs to an unknown timeline`);
return {
shouldLiveInRoom: false,
shouldLiveInThread: false,
Expand Down

0 comments on commit f16a6bc

Please sign in to comment.