Skip to content

Commit

Permalink
Fix new threads not appearing. (#4009)
Browse files Browse the repository at this point in the history 
* Fix new threads not appearing.

We try to update the thread roots when creating a thread, but a thread
can take some time to be ready after being created so we were calling it
too soon. Add a listener for the Update event to update the thread roots
once it's ready.

Fixes element-hq/element-web#26799

* Don't recreate the event when we update

and also add a comment to the test

* Hopefully make sonarcloud happy
  • Loading branch information
@dbkr
dbkr authored Jan 17, 2024
1 parent 0d486ea commit 81b5838
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 53 deletions.
2 changes: 1 addition & 1 deletion spec/integ/matrix-client-event-timeline.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1555,7 +1555,7 @@ describe("MatrixClient event timelines", function () {
expect(threadIds).toContain(THREAD2_ROOT.event_id);
const [allThreads] = timelineSets!;
const timeline = allThreads.getLiveTimeline()!;
// Test threads are in chronological order
// Test threads are in chronological order (first thread should be first because it has a more recent reply)
expect(timeline.getEvents().map((it) => it.event.event_id)).toEqual([
THREAD_ROOT.event_id,
THREAD2_ROOT.event_id,
Expand Down
9 changes: 8 additions & 1 deletion src/models/room.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1979,6 +1979,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
}

this.on(ThreadEvent.NewReply, this.onThreadReply);
this.on(ThreadEvent.Update, this.onThreadUpdate);
this.on(ThreadEvent.Delete, this.onThreadDelete);
this.threadsReady = true;
}
Expand Down Expand Up @@ -2082,6 +2083,10 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
}
}

private onThreadUpdate(thread: Thread): void {
this.updateThreadRootEvents(thread, false, false);
}

private onThreadReply(thread: Thread): void {
this.updateThreadRootEvents(thread, false, true);
}
Expand Down Expand Up @@ -2329,7 +2334,9 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
this.lastThread = thread;
}

if (this.threadsReady) {
// We need to update the thread root events, but the thread may not be ready yet.
// If it isn't, it will fire ThreadEvent.Update when it is and we'll call updateThreadRootEvents then.
if (this.threadsReady && thread.initialEventsFetched) {
this.updateThreadRootEvents(thread, toStartOfTimeline, false);
}
this.emit(ThreadEvent.New, thread, toStartOfTimeline);
Expand Down
112 changes: 61 additions & 51 deletions src/models/thread.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,13 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
private readonly pendingEventOrdering: PendingEventOrdering;
private processRootEventPromise?: Promise<void>;

/**
* Whether or not we need to fetch the initial set of events for the thread. We can
* only do this if the server has support for it, so if it doesn't we just pretend
* that we've already fetched them.
*/
public initialEventsFetched = !Thread.hasServerSideSupport;

/**
* An array of events to add to the timeline once the thread has been initialised
* with server suppport.
Expand Down Expand Up @@ -363,7 +369,7 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
* to the start (and not the end) of the timeline.
* @param emit - whether to emit the Update event if the thread was updated or not.
*/
public async addEvent(event: MatrixEvent, toStartOfTimeline: boolean, emit = true): Promise<void> {
public addEvent(event: MatrixEvent, toStartOfTimeline: boolean, emit = true): void {
// Modify this event to point at our room's state, and mark its thread
// as this.
this.setEventMetadata(event);
Expand All @@ -382,56 +388,7 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
this.addEventToTimeline(event, false);
this.fetchEditsWhereNeeded(event);
} else if (event.isRelation(RelationType.Annotation) || event.isRelation(RelationType.Replace)) {
// If this event is not a direct member of the thread, but is a
// reference to something that is, then we have two cases:

if (!this.initialEventsFetched) {
// Case 1: we haven't yet fetched events from the server. In
// this case, when we do, the events we get back might only be
// the first-order ones, so this event (which is second-order -
// a reference to something directly in the thread) needs to be
// kept so we can replay it when the first-order ones turn up.

/**
* A thread can be fully discovered via a single sync response
* And when that's the case we still ask the server to do an initialisation
* as it's the safest to ensure we have everything.
* However when we are in that scenario we might loose annotation or edits
*
* This fix keeps a reference to those events and replay them once the thread
* has been initialised properly.
*/
this.replayEvents?.push(event);
} else {
// Case 2: this is happening later, and we have a timeline. In
// this case, these events might be out-of order.
//
// Specifically, if the server doesn't support recursion, so we
// only get these events through sync, they might be coming
// later than the first-order ones, so we insert them based on
// timestamp (despite the problems with this documented in
// #3325).
//
// If the server does support recursion, we should have got all
// the interspersed events from the server when we fetched the
// initial events, so if they are coming via sync they should be
// the latest ones, so we can add them as normal.
//
// (Note that both insertEventIntoTimeline and addEventToTimeline
// do nothing if we have seen this event before.)

const recursionSupport =
this.client.canSupport.get(Feature.RelationsRecursion) ?? ServerSupport.Unsupported;

if (recursionSupport === ServerSupport.Unsupported) {
this.insertEventIntoTimeline(event);
} else {
this.addEventToTimeline(event, toStartOfTimeline);
}
}
// Apply annotations and replace relations to the relations of the timeline only
this.timelineSet.relations?.aggregateParentEvent(event);
this.timelineSet.relations?.aggregateChildEvent(event, this.timelineSet);
this.addRelatedThreadEvent(event, toStartOfTimeline);
return;
} else if (this.initialEventsFetched) {
// If initial events have not been fetched, we are OK to throw away
Expand Down Expand Up @@ -468,6 +425,59 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
}
}

private addRelatedThreadEvent(event: MatrixEvent, toStartOfTimeline: boolean): void {
// If this event is not a direct member of the thread, but is a
// reference to something that is, then we have two cases:

if (!this.initialEventsFetched) {
// Case 1: we haven't yet fetched events from the server. In
// this case, when we do, the events we get back might only be
// the first-order ones, so this event (which is second-order -
// a reference to something directly in the thread) needs to be
// kept so we can replay it when the first-order ones turn up.

/**
* A thread can be fully discovered via a single sync response
* And when that's the case we still ask the server to do an initialisation
* as it's the safest to ensure we have everything.
* However when we are in that scenario we might loose annotation or edits
*
* This fix keeps a reference to those events and replay them once the thread
* has been initialised properly.
*/
this.replayEvents?.push(event);
} else {
// Case 2: this is happening later, and we have a timeline. In
// this case, these events might be out-of order.
//
// Specifically, if the server doesn't support recursion, so we
// only get these events through sync, they might be coming
// later than the first-order ones, so we insert them based on
// timestamp (despite the problems with this documented in
// #3325).
//
// If the server does support recursion, we should have got all
// the interspersed events from the server when we fetched the
// initial events, so if they are coming via sync they should be
// the latest ones, so we can add them as normal.
//
// (Note that both insertEventIntoTimeline and addEventToTimeline
// do nothing if we have seen this event before.)

const recursionSupport =
this.client.canSupport.get(Feature.RelationsRecursion) ?? ServerSupport.Unsupported;

if (recursionSupport === ServerSupport.Unsupported) {
this.insertEventIntoTimeline(event);
} else {
this.addEventToTimeline(event, toStartOfTimeline);
}
}
// Apply annotations and replace relations to the relations of the timeline only
this.timelineSet.relations?.aggregateParentEvent(event);
this.timelineSet.relations?.aggregateChildEvent(event, this.timelineSet);
}

public async processEvent(event: Optional<MatrixEvent>): Promise<void> {
if (event) {
this.setEventMetadata(event);
Expand Down

0 comments on commit 81b5838

Please sign in to comment.