Fix issues with getEventTimeline and thread roots #2444
Conversation
| @@ -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.
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.
Its used for the notification timeline (or any without a room) where a client is still necessary but cannot be accessed via room.client
There was a problem hiding this comment.
Can we add that statement to the JSDoc here? It's great context for why we use it.
| client?: MatrixClient, | ||
| public readonly thread?: Thread, |
There was a problem hiding this comment.
Seems like we should put this stuff in opts
We can still assign this.thread = opts.thread in the constructor.
There was a problem hiding this comment.
Seems like we should put this stuff in opts
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.
Feel free to defer in favor of the style-guide.
The function signature here is getting pretty complex and we already have opts where this seems like it would fit.
| @@ -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.
| * If the event does not belong to this EventTimelineSet then undefined will be returned. | |
| * If the event does not belong to this EventTimelineSet then undefined will be returned | |
| * (like an `eventId` that doesn't belong in the thread represented by the given `EventTimelineSet`). |
| direction: Direction.Backward, | ||
| limit: 50, | ||
| }; | ||
| if (timelineSet.thread?.id !== threadId) { |
There was a problem hiding this comment.
A threadRoot returns:
{
shouldLiveInRoom: true,
shouldLiveInThread: true,
threadId: event.getId(),
}
Won't this logic prevent a threadRoot from showing in the main room timeline? undefined !== $abc
There was a problem hiding this comment.
Indeed, I think this is the cause of element-hq/element-web#22539
The if check should be timelineSet.thread && timelineSet.thread.id !== threadId
| @@ -1,5 +1,5 @@ | |||
| import * as utils from "../test-utils/test-utils"; | |||
There was a problem hiding this comment.
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:
Fix events from main timeline getting mixed in threads
There was a problem hiding this comment.
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.
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.
The problem is that following the link to this PR from the changelog also doesn't have any of the extra details either.
Except it does, in the canonical source, the issue it links to
There was a problem hiding this comment.
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.
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.
The description and linked issue don't explain this. And my changelog example can be extended to something like:
Fix events from main timeline getting mixed in threads and generally events being mixed across timelines.
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.
The description and linked issue don't explain this.
Nor do they need to given threads in the js-sdk is marked @experimental
where thread roots are one of the edge cases that it actually doesn't fix anyway.
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.
Nor do they need to given threads in the js-sdk is marked
@experimental
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.
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
I see what you mean. This is an example of what we should be explaining though.
* Remove unused sessionStore ([\matrix-org#2455](matrix-org#2455)). * Implement MSC3827: Filtering of `/publicRooms` by room type ([\matrix-org#2469](matrix-org#2469)). * expose latestLocationEvent on beacon model ([\matrix-org#2467](matrix-org#2467)). Contributed by @kerryarchibald. * Live location share - add start time leniency ([\matrix-org#2465](matrix-org#2465)). Contributed by @kerryarchibald. * Log real errors and not just their messages, traces are useful ([\matrix-org#2464](matrix-org#2464)). * Various changes to `src/crypto` files for correctness ([\matrix-org#2137](matrix-org#2137)). Contributed by @ShadowJonathan. * Update MSC3786 implementation: Check the `state_key` ([\matrix-org#2429](matrix-org#2429)). * Timeline needs to refresh when we see a MSC2716 marker event ([\matrix-org#2299](matrix-org#2299)). Contributed by @MadLittleMods. * Try to load keys from key backup when a message fails to decrypt ([\matrix-org#2373](matrix-org#2373)). Fixes element-hq/element-web#21026. Contributed by @duxovni. * Send call version `1` as a string ([\matrix-org#2471](matrix-org#2471)). Fixes element-hq/element-web#22629. * Fix issue with `getEventTimeline` returning undefined for thread roots in main timeline ([\matrix-org#2454](matrix-org#2454)). Fixes element-hq/element-web#22539. * Add missing `type` property on `IAuthData` ([\matrix-org#2463](matrix-org#2463)). * Clearly indicate that `lastReply` on a Thread can return falsy ([\matrix-org#2462](matrix-org#2462)). * Fix issues with getEventTimeline and thread roots ([\matrix-org#2444](matrix-org#2444)). Fixes element-hq/element-web#21613. * Live location sharing - monitor liveness of beacons yet to start ([\matrix-org#2437](matrix-org#2437)). Contributed by @kerryarchibald. * Refactor Relations to not be per-EventTimelineSet ([\matrix-org#2412](matrix-org#2412)). Fixes matrix-org#2399 and element-hq/element-web#22298. * Add tests for sendEvent threadId handling ([\matrix-org#2435](matrix-org#2435)). Fixes element-hq/element-web#22433. * Make sure `encryptAndSendKeysToDevices` assumes devices are unique per-user. ([\matrix-org#2136](matrix-org#2136)). Fixes matrix-org#2135. Contributed by @ShadowJonathan. * Don't bug the user while re-checking key backups after decryption failures ([\matrix-org#2430](matrix-org#2430)). Fixes element-hq/element-web#22416. Contributed by @duxovni.
…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
Fixes element-hq/element-web#21613
Here's what your changelog entry will look like:
🐛 Bug Fixes