Refuse to link live timelines into the forwards/backwards position when either is invalid #877
Conversation
See element-hq/element-web#8593 (comment) Previously (#873) we allowed half-linking timelines to each other if they satisfy the conditions, however this appears to not be helping. Instead, it seems like the timelines are getting stuck in a position where one direction is spliced but the other is broken. To avoid this case, we'll just avoid splicing in both directions when one of the directions is invalid.
There was a problem hiding this comment.
forwardsIsLive would be true when you forward-fill a gap that has an event that is also in the live timeline, right?
I'm slightly worried about breaking pagination and triggering /messages too often if TimelineWindow can't find a preceding timeline when scrolling up.
Also, I'm not sure when we would add an event to a timeline preceding the live timeline. I suppose a forward-fill to a timeline with /messages, but unsure when we actually do this? IIRC we backpaginate the live timeline in case of a gap in TimelineWindow? Just to say I'm not 100% sure this will fix anything...
|
It would be true only in the cases where the live timeline is being put in a position where it cannot be live (ie: A -> B -> C, where B would be the live timeline). The variables are already a bit confusing here, but The problem this aims to solve is certainly vague, and it might very well indeed not fix anything. The concern is that by allowing 50% of a link to go through, we're actually making the problem worse. The theory is that by preventing the link altogether that we'll end up with a stable timeline which can be nicely spliced later on if needed. |
|
Ok, so the only actual change is the continue statement so setNeighbour ... doesn't get called for either timelines. 👍 |
| } else { | ||
| existingTimeline.setNeighbouringTimeline(timeline, inverseDirection); | ||
| // We use independent logging to better discover the problem at a glance. | ||
| console.warn({backwardsIsLive, forwardsIsLive}); // debugging |
There was a problem hiding this comment.
is this intended to stay in? Should be clear from seeing either of the below messages already?
There was a problem hiding this comment.
It's intended to stay in to make it slightly easier to troubleshoot from the logs without having to go hunting for messages in the code.
Edit: I'll add a note to clean it up
See element-hq/element-web#8593 (comment)
Previously (#873) we allowed half-linking timelines to each other if they satisfy the conditions, however this appears to not be helping. Instead, it seems like the timelines are getting stuck in a position where one direction is spliced but the other is broken. To avoid this case, we'll just avoid splicing in both directions when one of the directions is invalid.