Skip to content
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.

Sign up for GitHub

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

Replace MatrixClient.isRoomEncrypted by MatrixClient.CryptoApi.isEncryptionEnabledInRoom in FilePanel #28275

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from
Draft

Replace MatrixClient.isRoomEncrypted by MatrixClient.CryptoApi.isEncryptionEnabledInRoom in FilePanel #28275

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
42fcb9c
Replace `MatrixClient.isRoomEncrypted` by `MatrixClient.CryptoApi.isE…
florianduros Oct 23, 2024
b718c09
Display `SearchWarning` when `isRoomEncrypted` is loaded
florianduros Oct 23, 2024
0ba0840
Display timeline panel when room `isRoomEncrypted` is loaded
florianduros Oct 23, 2024
7507e3c
Fix potential racy dangling listener
florianduros Oct 23, 2024
3ccfd9b
Move room encryption check earlier in mount
florianduros Oct 23, 2024
3797644
Fix didMount condition
florianduros Oct 23, 2024
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
55 changes: 36 additions & 19 deletions src/components/structures/FilePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ interface IProps {
interface IState {
timelineSet: EventTimelineSet | null;
narrow: boolean;
/**
* Whether the room is encrypted or not. If null, the state is still being determined.
*/
isRoomEncrypted: boolean | null;
}

/*
Expand All @@ -58,10 +62,13 @@ class FilePanel extends React.Component<IProps, IState> {
private decryptingEvents = new Set<string>();
public noRoom = false;
private card = createRef<HTMLDivElement>();
// This is used to track if the component is unmounting to avoid adding dangling listeners
private isUnloading = false;

public state: IState = {
timelineSet: null,
narrow: false,
isRoomEncrypted: null,
};

private onRoomTimeline = (
Expand Down Expand Up @@ -110,10 +117,14 @@ class FilePanel extends React.Component<IProps, IState> {

public async componentDidMount(): Promise<void> {
const client = MatrixClientPeg.safeGet();
const isRoomEncrypted = Boolean(await client.getCrypto()?.isEncryptionEnabledInRoom(this.props.roomId));
this.setState({
isRoomEncrypted,
});

await this.updateTimelineSet(this.props.roomId);

if (!client.isRoomEncrypted(this.props.roomId)) return;
if (!isRoomEncrypted) return;

// The timelineSets filter makes sure that encrypted events that contain
// URLs never get added to the timeline, even if they are live events.
Expand All @@ -123,17 +134,18 @@ class FilePanel extends React.Component<IProps, IState> {
// We do this only for encrypted rooms and if an event index exists,
// this could be made more general in the future or the filter logic
// could be fixed.
if (EventIndexPeg.get() !== null) {
//
// `componentDidMount` is async and we need to be sure to not put this listener when the component is unmount before the mounting is done.
if (EventIndexPeg.get() !== null && this.isUnloading) {
client.on(RoomEvent.Timeline, this.onRoomTimeline);
client.on(MatrixEventEvent.Decrypted, this.onEventDecrypted);
}
}

public componentWillUnmount(): void {
this.isUnloading = true;
const client = MatrixClientPeg.get();
if (client === null) return;

if (!client.isRoomEncrypted(this.props.roomId)) return;
if (client === null || !this.state.isRoomEncrypted) return;

if (EventIndexPeg.get() !== null) {
client.removeListener(RoomEvent.Timeline, this.onRoomTimeline);
Comment on lines -134 to 151
Copy link
Member

Choose a reason for hiding this comment

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

Since componentDidMount is async, I think there's another potential for races here: the component mounts, then if it unmounts before componentDidMount even finishes determining whether the room is encrypted, it will set up event listeners that will never get removed.

Expand Down Expand Up @@ -173,7 +185,7 @@ class FilePanel extends React.Component<IProps, IState> {
// the event index to fulfill the pagination request. Asking the server
// to paginate won't ever work since the server can't correctly filter
// out events containing URLs
if (room && client.isRoomEncrypted(roomId) && eventIndex !== null) {
if (room && this.state.isRoomEncrypted && eventIndex !== null) {
return eventIndex.paginateTimelineWindow(room, timelineWindow, direction, limit);
} else {
return timelineWindow.paginate(direction, limit);
Copy link
Member

Choose a reason for hiding this comment

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

Will anything go wrong if this branch gets executed while this.state.isRoomEncrypted is null?

Copy link
Member Author

Choose a reason for hiding this comment

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

I conditioned the TimelinePanel under the isRoomEncryptedLoaded check. So this case should not happen now

Expand Down Expand Up @@ -206,7 +218,7 @@ class FilePanel extends React.Component<IProps, IState> {
// event index to populate the timelineSet for us. This call
// will add 10 events to the live timeline of the set. More can
// be requested using pagination.
if (client.isRoomEncrypted(roomId) && eventIndex !== null) {
if (this.state.isRoomEncrypted && eventIndex !== null) {
Comment on lines -209 to +221
Copy link
Member

Choose a reason for hiding this comment

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

Same kind of question here… will this method only get called after this.state.isRoomEncrypted is loaded? If not, is that okay?

const timeline = timelineSet.getLiveTimeline();
await eventIndex.populateFileTimeline(timelineSet, timeline, room, 10);
}
Expand Down Expand Up @@ -265,7 +277,8 @@ class FilePanel extends React.Component<IProps, IState> {
/>
);

const isRoomEncrypted = this.noRoom ? false : MatrixClientPeg.safeGet().isRoomEncrypted(this.props.roomId);
const isRoomEncryptedLoaded = this.state.isRoomEncrypted !== null;
const isRoomEncrypted = Boolean(this.noRoom ? false : this.state.isRoomEncrypted);

if (this.state.timelineSet) {
return (
Expand All @@ -286,17 +299,21 @@ class FilePanel extends React.Component<IProps, IState> {
{this.card.current && (
<Measured sensor={this.card.current} onMeasurement={this.onMeasurement} />
)}
<SearchWarning isRoomEncrypted={isRoomEncrypted} kind={WarningKind.Files} />
<TimelinePanel
manageReadReceipts={false}
manageReadMarkers={false}
timelineSet={this.state.timelineSet}
showUrlPreview={false}
onPaginationRequest={this.onPaginationRequest}
resizeNotifier={this.props.resizeNotifier}
empty={emptyState}
layout={Layout.Group}
/>
{isRoomEncryptedLoaded && (
<>
<SearchWarning isRoomEncrypted={isRoomEncrypted} kind={WarningKind.Files} />
<TimelinePanel
manageReadReceipts={false}
manageReadMarkers={false}
timelineSet={this.state.timelineSet}
showUrlPreview={false}
onPaginationRequest={this.onPaginationRequest}
resizeNotifier={this.props.resizeNotifier}
empty={emptyState}
layout={Layout.Group}
/>
</>
)}
</BaseCard>
</RoomContext.Provider>
);
Expand Down
Loading