Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix room history not being visible even if we have historical keys #8563

Merged
merged 3 commits into from
May 11, 2022
Merged

Fix room history not being visible even if we have historical keys #8563

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
33092f1
Fix room history not being visible even if we have historical keys
SimonBrandner May 11, 2022
cebd65b
Throttle `recheckFirstVisibleEventIndex()`
SimonBrandner May 11, 2022
8792fcd
Move `recheckFirstVisibleEventIndex()` out of `onEventDecrypted()`
SimonBrandner May 11, 2022
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
15 changes: 11 additions & 4 deletions src/components/structures/TimelinePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,16 +806,23 @@ class TimelinePanel extends React.Component<IProps, IState> {
// Can be null for the notification timeline, etc.
if (!this.props.timelineSet.room) return;

if (ev.getRoomId() !== this.props.timelineSet.room.roomId) return;

if (!this.state.events.includes(ev)) return;

const firstVisibleEventIndex = this.checkForPreJoinUISI(this.state.events);
if (firstVisibleEventIndex !== this.state.firstVisibleEventIndex) {
this.setState({ firstVisibleEventIndex });
}
SimonBrandner marked this conversation as resolved.
Show resolved Hide resolved

// Need to update as we don't display event tiles for events that
// haven't yet been decrypted. The event will have just been updated
// in place so we just need to re-render.
// TODO: We should restrict this to only events in our timeline,
// but possibly the event tile itself should just update when this
// happens to save us re-rendering the whole timeline.
if (ev.getRoomId() === this.props.timelineSet.room.roomId) {
this.buildCallEventGroupers(this.state.events);
this.forceUpdate();
}
this.buildCallEventGroupers(this.state.events);
this.forceUpdate();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious how this fixes element-hq/element-web#16983

What stopped it from working before?

What changes were made to make history visible now?

Copy link
Member

@t3chguy t3chguy May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We weren't re-checking checkForPreJoinUISI when we decrypted events, there's a comment in checkForPreJoinUISI which says it treats on-going decryptions the same as failed decryptions. This created a race condition between rendering and decryption. So if you receive keys after the initial render, its almost guaranteed to go down the unhappy path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This specific bit doesn't do it; the recheck method does the trick - now when an event is decrypted we check if we can show older events, we didn't do that before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beat me to it (again today :D)

Copy link
Contributor

@MadLittleMods MadLittleMods May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is it possible to write a test for this scenario so we don't regress? This stuff seems very order dependent and tricky to get right

How were you testing locally to confirm the change works? (reproduction steps)

Copy link
Member

@t3chguy t3chguy May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We can regression test the single edge we identified and fixed here, but there are so many edges, you will struggle to cover them all. Even hitting 100% coverage won't cover all the possible races & scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be possible - I'll have a look into it, would you mind creating an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

element-hq/element-web#22155 ⏩


private onSync = (clientSyncState: SyncState, prevState: SyncState, data: object): void => {
Expand Down