-
-
Notifications
You must be signed in to change notification settings - Fork 604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issues with getEventTimeline and thread roots #2444
Changes from all commits
db7bed4
8fd209d
8e25e52
93c293b
45fc333
a178516
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -5245,14 +5245,15 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa | |||||||
* <p>If the EventTimelineSet object already has the given event in its store, the | ||||||||
* corresponding timeline will be returned. Otherwise, a /context request is | ||||||||
* made, and used to construct an EventTimeline. | ||||||||
* If the event does not belong to this EventTimelineSet then undefined will be returned. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
* | ||||||||
* @param {EventTimelineSet} timelineSet The timelineSet to look for the event in | ||||||||
* @param {EventTimelineSet} timelineSet The timelineSet to look for the event in, must be bound to a room | ||||||||
t3chguy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
* @param {string} eventId The ID of the event to look for | ||||||||
* | ||||||||
* @return {Promise} Resolves: | ||||||||
* {@link module:models/event-timeline~EventTimeline} including the given event | ||||||||
*/ | ||||||||
public async getEventTimeline(timelineSet: EventTimelineSet, eventId: string): Promise<EventTimeline> { | ||||||||
public async getEventTimeline(timelineSet: EventTimelineSet, eventId: string): Promise<EventTimeline | undefined> { | ||||||||
// don't allow any timeline support unless it's been enabled. | ||||||||
if (!this.timelineSupport) { | ||||||||
throw new Error("timeline support is disabled. Set the 'timelineSupport'" + | ||||||||
|
@@ -5297,38 +5298,44 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa | |||||||
...res.events_before.map(mapper), | ||||||||
]; | ||||||||
|
||||||||
// 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 | ||||||||
if (Thread.hasServerSideSupport && | ||||||||
this.supportsExperimentalThreads() && | ||||||||
event.isRelation(THREAD_RELATION_TYPE.name) | ||||||||
) { | ||||||||
const [, threadedEvents] = timelineSet.room.partitionThreadedEvents(events); | ||||||||
let thread = timelineSet.room.getThread(event.threadRootId); | ||||||||
if (!thread) { | ||||||||
thread = timelineSet.room.createThread(event.threadRootId, undefined, threadedEvents, true); | ||||||||
if (this.supportsExperimentalThreads()) { | ||||||||
const { threadId, shouldLiveInRoom } = timelineSet.room.eventShouldLiveIn(event); | ||||||||
|
||||||||
if (!timelineSet.thread && !shouldLiveInRoom) { | ||||||||
// Thread response does not belong in this timelineSet | ||||||||
return undefined; | ||||||||
} | ||||||||
|
||||||||
const opts: IRelationsRequestOpts = { | ||||||||
direction: Direction.Backward, | ||||||||
limit: 50, | ||||||||
}; | ||||||||
if (timelineSet.thread?.id !== threadId) { | ||||||||
t3chguy marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A
Won't this logic prevent a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, I think this is the cause of element-hq/element-web#22539 The if check should be
t3chguy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
// Event does not belong in this timelineSet | ||||||||
return undefined; | ||||||||
} | ||||||||
t3chguy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
await thread.fetchInitialEvents(); | ||||||||
let nextBatch = thread.liveTimeline.getPaginationToken(Direction.Backward); | ||||||||
// 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 | ||||||||
if (Thread.hasServerSideSupport && timelineSet.thread) { | ||||||||
const thread = timelineSet.thread; | ||||||||
const opts: IRelationsRequestOpts = { | ||||||||
direction: Direction.Backward, | ||||||||
limit: 50, | ||||||||
}; | ||||||||
|
||||||||
// Fetch events until we find the one we were asked for, or we run out of pages | ||||||||
while (!thread.findEventById(eventId)) { | ||||||||
if (nextBatch) { | ||||||||
opts.from = nextBatch; | ||||||||
await thread.fetchInitialEvents(); | ||||||||
let nextBatch = thread.liveTimeline.getPaginationToken(Direction.Backward); | ||||||||
|
||||||||
// Fetch events until we find the one we were asked for, or we run out of pages | ||||||||
while (!thread.findEventById(eventId)) { | ||||||||
if (nextBatch) { | ||||||||
opts.from = nextBatch; | ||||||||
} | ||||||||
|
||||||||
({ nextBatch } = await thread.fetchEvents(opts)); | ||||||||
if (!nextBatch) break; | ||||||||
} | ||||||||
|
||||||||
({ nextBatch } = await thread.fetchEvents(opts)); | ||||||||
if (!nextBatch) break; | ||||||||
return thread.liveTimeline; | ||||||||
} | ||||||||
|
||||||||
return thread.liveTimeline; | ||||||||
} | ||||||||
|
||||||||
// Here we handle non-thread timelines only, but still process any thread events to populate thread summaries. | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ import { RoomState } from "./room-state"; | |
import { TypedEventEmitter } from "./typed-event-emitter"; | ||
import { RelationsContainer } from "./relations-container"; | ||
import { MatrixClient } from "../client"; | ||
import { Thread } from "./thread"; | ||
|
||
const DEBUG = true; | ||
|
||
|
@@ -110,7 +111,7 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime | |
* map from event_id to timeline and index. | ||
* | ||
* @constructor | ||
* @param {?Room} room | ||
* @param {Room=} room | ||
* Room for this timelineSet. May be null for non-room cases, such as the | ||
* notification timeline. | ||
* @param {Object} opts Options inherited from Room. | ||
|
@@ -119,13 +120,15 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime | |
* Set to true to enable improved timeline support. | ||
* @param {Object} [opts.filter = null] | ||
* The filter object, if any, for this timelineSet. | ||
* @param {MatrixClient} client the Matrix client which owns this EventTimelineSet, | ||
* @param {MatrixClient=} client the Matrix client which owns this EventTimelineSet, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this only necessary for threads? That's the only place we use this functionality. We should add a comment with the reasoning for when this is necessary and the thread use case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its used for the notification timeline (or any without a room) where a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add that statement to the JSDoc here? It's great context for why we use it. |
||
* can be omitted if room is specified. | ||
* @param {Thread=} thread the thread to which this timeline set relates. | ||
*/ | ||
constructor( | ||
public readonly room: Room | undefined, | ||
opts: IOpts = {}, | ||
client?: MatrixClient, | ||
public readonly thread?: Thread, | ||
Comment on lines
130
to
+131
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like we should put this stuff in We can still assign There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why? https://github.com/vector-im/element-meta/pull/216/files doesn't favour it in any way shape or form There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feel free to defer in favor of the style-guide. The function signature here is getting pretty complex and we already have |
||
) { | ||
super(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title and changelog of this PR is not every descriptive: "Fix issues with getEventTimeline and thread roots". It just describes the code and not what's being fixed.
Potential option:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this does more than that, that's just one symptom it fixes, changelog entries should be relatively short even if that's vague
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What other symptoms does it fix? The other problems are all variations of that AFAICT.
The problem is that following the link to this PR from the changelog also doesn't have any of the extra details either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except it does, in the canonical source, the issue it links to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't "Fix events from main timeline getting mixed in threads" work to describe what this does then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it also stops other effects, like stops an unrelated timeline being returned if you call with a threaded-timelineset and an event id from a diff thread/main timeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description and linked issue don't explain this. And my changelog example can be extended to something like:
Which is way more useful than
Fix issues with getEventTimeline and thread roots
where thread roots are one of the edge cases that it actually doesn't fix anyway. And doesn't describe the main issue it fixes that everyone is running into in the first place.I had to decode all of this information from the diff (which is painful) and when I did, I found bugs. The expectations and why were not spelled out in the code or description where we can easily compare how the code behaves to those expectations. One (maybe 2) symptoms are explain in the linked issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nor do they need to given threads in the js-sdk is marked
@experimental
This isn't true, it fixed the issue for event timeline sets accessing thread roots which caused the issue you were hitting with main timeline events showing up in threads, and created a regression around accessing thread roots from main timeline event timeline sets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While compromises and known problems are probably part of an
@experimental
feature, seems like we shouldn't just drop the ball on all of it. I think this problem generally exists in most PR's though. And regardless, our current plan/goal for threads is to make this not experimental where this code is bound to live on.We should so we can better maintain this. We're not going to remember this fresh context that I am driving for in 6 months when we find another regression. I understand that some of this code will change when we get backend changes to better support threads but that info will just make it easier to refactor this in the future as well to see that we're still maintaining all of the previous behavior plus the extra benefits.
I see what you mean. This is an example of what we should be explaining though.