Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix getLatestTimeline not working when the latest even in the room wa…
…s a threaded message

See matrix-org/matrix-react-sdk#8354 (comment)

Also have to keep in mind that we don't want to mix messages in the wrong timelines (main vs threaded timeline)

 - #2444
 - #2454
  • Loading branch information
MadLittleMods committed Jul 13, 2022
commit 5f4d10d478f418e0f120d83a050ff9d1b9417c6f
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ node_modules
/*.log
package-lock.json
.lock-wscript
.DS_Store
build/Release
coverage
lib-cov
Expand Down
2 changes: 1 addition & 1 deletion spec/integ/matrix-client-room-timeline.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -688,7 +688,7 @@ describe("MatrixClient room timelines", function() {
});

// Wait for the timeline to reset(when it goes blank) which means
// it's in the middle of the refrsh logic right before the
// it's in the middle of the refresh logic right before the
// `getEventTimeline()` -> `/context`. Then simulate a racey `/sync`
// to happen in the middle of all of this refresh timeline logic. We
// want to make sure the sync pagination still works as expected
Expand Down
4 changes: 0 additions & 4 deletions src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5288,10 +5288,6 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
];

if (this.supportsExperimentalThreads()) {
if (!timelineSet.canContain(event)) {
return undefined;
}
Comment on lines -5213 to -5215
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning undefined, we now just ignore the event and don't add it to the given timelineSet.

Did returning undefined have some other special meaning? The getEventTimeline usage didn't stand out to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did returning undefined have some other special meaning? The getEventTimeline usage didn't stand out to me.

I don't think it has any special meaning, see #2521 (comment)

Comment on lines -5213 to -5215
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to make sure we're not regressing #2444 and #2454 for sure, do we have a specific testing strategy in the Element app to reproduce previously (main timeline events in a thread for example)? How do we know that those previous PR's fixed the problem besides reasoning about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t3chguy Any hints here?


// Where the event is a thread reply (not a root) and running in MSC-enabled mode the Thread timeline only
// functions contiguously, so we have to jump through some hoops to get our target event in it.
// XXX: workaround for https://github.com/vector-im/element-meta/issues/150
Expand Down
12 changes: 12 additions & 0 deletions src/models/event-timeline-set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,18 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
);
}

if (timeline.getTimelineSet() !== this) {
throw new Error(`EventTimelineSet.addEventToTimeline: Timeline=${timeline.toString()} does not belong " +
"in timelineSet(threadId=${this.thread?.id})`);
}

// 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).
if (!this.canContain(event)) {
logger.warn(`EventTimelineSet.addEventToTimeline: Ignoring event=${event.getId()} that does not belong in timeline=${timeline.toString()} timelineSet(threadId=${this.thread?.id})`);
return;
}
Comment on lines +698 to +709
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split out to #2848


const eventId = event.getId();
timeline.addEvent(event, {
toStartOfTimeline,
Expand Down
4 changes: 4 additions & 0 deletions src/models/room.ts
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,10 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
newTimeline = await this.client.getEventTimeline(timelineSet, mostRecentEventInTimeline.getId());
}

if (!newTimeline) {
throw new Error(`[refreshLiveTimeline for ${this.roomId}] No new timeline created`);
}

// If a racing `/sync` beat us to creating a new timeline, use that
// instead because it's the latest in the room and any new messages in
// the scrollback will include the history.
Expand Down