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

Ask to refresh timeline when historical messages are imported (MSC2716) #8303

Closed
wants to merge 11 commits into from
Closed

Ask to refresh timeline when historical messages are imported (MSC2716) #8303

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
66ea757
Ask to refresh timeline when historical messages are imported (MSC2716)
MadLittleMods Apr 13, 2022
6dba3df
Back to false after getting screenshot
MadLittleMods Apr 13, 2022
12b0115
Fix lints
MadLittleMods Apr 13, 2022
eb5e899
Fix room not showing refresh timeline banner if not switched to the r…
MadLittleMods Apr 13, 2022
a50e011
Fix type lints
MadLittleMods Apr 13, 2022
ed910bb
Remove parenthesis change
MadLittleMods Apr 14, 2022
8d61226
Raw refreshLiveTimeline usage that seems to work
MadLittleMods Apr 16, 2022
d8f94ed
Clean up raw commit
MadLittleMods Apr 18, 2022
c43d309
Merge branch 'develop' into madlittlemods/refresh-timeline-when-we-se…
MadLittleMods Apr 19, 2022
623960b
Clean up event usage
MadLittleMods Apr 19, 2022
d9001ce
Remove unreadable image descriptions
MadLittleMods Apr 19, 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
6 changes: 6 additions & 0 deletions res/css/structures/_RoomStatusBar.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,12 @@ limitations under the License.
mask-image: url('$(res)/img/element-icons/retry.svg');
}
}

&.mx_RoomStatusBar_refreshTimelineBtn {
&::before {
mask-image: url('$(res)/img/element-icons/retry.svg');
}
}
}

.mx_InlineSpinner {
Expand Down
4 changes: 3 additions & 1 deletion src/components/structures/FilePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ class FilePanel extends React.Component<IProps, IState> {
}

if (!this.state.timelineSet.eventIdToTimeline(ev.getId())) {
this.state.timelineSet.addEventToTimeline(ev, timeline, false);
this.state.timelineSet.addEventToTimeline(ev, timeline, {
toStartOfTimeline: false,
});
}
}

Expand Down
62 changes: 61 additions & 1 deletion src/components/structures/RoomStatusBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ interface IState {
syncStateData: ISyncStateData;
unsentMessages: MatrixEvent[];
isResending: boolean;
timelineNeedsRefresh: boolean;
}

export default class RoomStatusBar extends React.PureComponent<IProps, IState> {
Expand All @@ -93,13 +94,15 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> {
syncStateData: this.context.getSyncStateData(),
unsentMessages: getUnsentMessages(this.props.room),
isResending: false,
timelineNeedsRefresh: this.props.room.getTimelineNeedsRefresh(),
};
}

public componentDidMount(): void {
const client = this.context;
client.on("sync", this.onSyncStateChange);
client.on("Room.localEchoUpdated", this.onRoomLocalEchoUpdated);
client.on("Room.historyImportedWithinTimeline", this.onRoomHistoryImportedWithinTimeline);
Copy link
Member

Choose a reason for hiding this comment

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

Please add the context type definition around line L87 as string emitter keys are no longer allowed in TypedEventEmitter, TS thinks this.context is any here though

Copy link
Member

Choose a reason for hiding this comment

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

Can we also not listen on the room directly, re-emitters suck

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we also not listen on the room directly, re-emitters suck

So I don't guess, what should we do? 🙇

Copy link
Member

Choose a reason for hiding this comment

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

this.props.room.on(...); but with componentDidUpdate comparing this.props.room with prevProps.room to remove old listener and setup a new one. This is all a lot nicer in hooks where its a one-liner.

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.props.room.on(...); but with componentDidUpdate comparing this.props.room with prevProps.room to remove old listener and setup a new one. This is all a lot nicer in hooks where its a one-liner.

Updated 👍

Please add the context type definition around line L87 as string emitter keys are no longer allowed in TypedEventEmitter

I'm not sure what to do here exactly. I've stopped using string keys. Is there anything further?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conversation continued at #8354 (comment)


this.checkSize();
}
Expand All @@ -115,6 +118,7 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> {
if (client) {
client.removeListener("sync", this.onSyncStateChange);
client.removeListener("Room.localEchoUpdated", this.onRoomLocalEchoUpdated);
client.removeListener("Room.historyImportedWithinTimeline", this.onRoomHistoryImportedWithinTimeline);
}
}

Expand Down Expand Up @@ -142,6 +146,19 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> {
dis.fire(Action.FocusSendMessageComposer);
};

private onRefreshTimelineClick = (): void => {
console.log('TODO: Refresh timeline');
// TODO: What's the best way to refresh the timeline? Something like
// `room.resetLiveTimeline(null, null);` although this just seems to
// clear the timeline. I also tried to split out
// `scrollbackFromPaginationToken` from the `scrollback` method in to
// paginate from the beginning of the room but it's just not right.
Copy link
Contributor Author

@MadLittleMods MadLittleMods Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What's the best way to refresh the timeline? Something like room.resetLiveTimeline(null, null); although this just seems to clear the timeline.

I also tried to split out the logic from scrollback into a new scrollbackFromPaginationToken method in order to paginate from the latest in the room (instead of from the current paginationToken) and fill in the timeline again after it goes blank but the room just stays blank.

scrollbackFromPaginationToken refactor 
    /**
     * Retrieve older messages from the given room and put them in the timeline.
     *
     * If this is called multiple times whilst a request is ongoing, the <i>same</i>
     * Promise will be returned. If there was a problem requesting scrollback, there
     * will be a small delay before another request can be made (to prevent tight-looping
     * when there is no connection).
     *
     * @param {Room} room The room to get older messages in.
     * @param {Integer} limit Optional. The maximum number of previous events to
     * pull in. Default: 30.
     * @param {module:client.callback} callback Optional.
     * @return {Promise} Resolves: Room. If you are at the beginning
     * of the timeline, <code>Room.oldState.paginationToken</code> will be
     * <code>null</code>.
     * @return {module:http-api.MatrixError} Rejects: with an error response.
     */
    public scrollback(room: Room, limit = 30, callback?: Callback): Promise<Room> {
        if (utils.isFunction(limit)) {
            callback = limit as any as Callback; // legacy
            limit = undefined;
        }
        let timeToWaitMs = 0;

        let info = this.ongoingScrollbacks[room.roomId] || {};
        if (info.promise) {
            return info.promise;
        } else if (info.errorTs) {
            const timeWaitedMs = Date.now() - info.errorTs;
            timeToWaitMs = Math.max(SCROLLBACK_DELAY_MS - timeWaitedMs, 0);
        }

        if (room.oldState.paginationToken === null) {
            return Promise.resolve(room); // already at the start.
        }
        // attempt to grab more events from the store first
        const numAdded = this.store.scrollback(room, limit).length;
        if (numAdded === limit) {
            // store contained everything we needed.
            return Promise.resolve(room);
        }
        // reduce the required number of events appropriately
        limit = limit - numAdded;

        const prom = Promise.resolve().then(async () => {
            try {
                // wait for a time before doing this request
                // (which may be 0 in order not to special case the code paths)
                await sleep(timeToWaitMs)

                await this.scrollbackFromPaginationToken({
                    room,
                    fromToken: room.oldState.paginationToken,
                    direction: Direction.Backward,
                    limit,
                })
                this.ongoingScrollbacks[room.roomId] = null;
                callback?.(null, room);
                return room;
            } catch(err) {
                this.ongoingScrollbacks[room.roomId] = {
                    errorTs: Date.now(),
                };
                callback?.(err);
                throw err;
            }
        });

        info = {
            promise: prom,
            errorTs: null,
        };

        this.ongoingScrollbacks[room.roomId] = info;
        return prom;
    }

    public async scrollbackFromPaginationToken({
        room,
        fromToken,
        direction,
        limit,
    }: {
        room: Room,
        fromToken: string | null,
        direction: Direction,
        limit?: number,
    }) {
        const res: IMessagesResponse = await this.createMessagesRequest(
            room.roomId,
            fromToken,
            limit,
            direction,
        );

        const matrixEvents = res.chunk.map(this.getEventMapper());
        if (res.state) {
            const stateEvents = res.state.map(this.getEventMapper());
            room.currentState.setUnknownStateEvents(stateEvents);
        }

        const [timelineEvents, threadedEvents] = this.partitionThreadedEvents(room, matrixEvents);

        room.addEventsToTimeline(timelineEvents, true, room.getLiveTimeline());
        await this.processThreadEvents(room, threadedEvents, true);

        room.oldState.paginationToken = res.end;
        if (res.chunk.length === 0) {
            room.oldState.paginationToken = null;
        }
        this.store.storeEvents(room, matrixEvents, res.end, true);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assigned review to get some advice for this question ^

Otherwise just need to add tests for this PR


this.setState({
timelineNeedsRefresh: false,
});
};
Copy link
Contributor Author

@MadLittleMods MadLittleMods Apr 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think the behavior of all of this would be best served by an end-to-end test. It looks like we just added Cypress recently but it's missing all of the utility necessary to complete something like this.

  • Needs a command to create a user behind the scenes and visit Element as the user already signed in
  • Needs commands to setup rooms and send events
  • Needs a way to enable experimental_features in the homeserver.yaml template
  • Needs a way to add an application service registration in the Synapse instance. The MSC2716 /batch_send endpoint is only accessible from a AS token. No extra AS server needed, just the AS token configured.
  • Needs a way to interact with the homserver directly from the application service token to call /batch_send (probably via matrix-js-sdk)

I can't find an overall issue describing the need/want to use Cypress so it's unclear how much we want to move forward with it. I've had many troubles using Cypress with Gitter.

It seems like we have other e2e tests using Puppeteer but I'm guessing we want to move all of these to Cypress. Better place I should be adding some e2e tests or approaching this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conversation continued at #8354 (comment)


private onRoomLocalEchoUpdated = (ev: MatrixEvent, room: Room) => {
if (room.roomId !== this.props.room.roomId) return;
const messages = getUnsentMessages(this.props.room);
Expand All @@ -151,6 +168,14 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> {
});
};

private onRoomHistoryImportedWithinTimeline = (markerEv: MatrixEvent, room: Room) => {
if (room.roomId !== this.props.room.roomId) return;

this.setState({
timelineNeedsRefresh: room.getTimelineNeedsRefresh(),
});
};

// Check whether current size is greater than 0, if yes call props.onVisible
private checkSize(): void {
if (this.getSize()) {
Expand All @@ -166,7 +191,11 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> {
private getSize(): number {
if (this.shouldShowConnectionError()) {
return STATUS_BAR_EXPANDED;
} else if (this.state.unsentMessages.length > 0 || this.state.isResending) {
} else if (
this.state.unsentMessages.length > 0 ||
this.state.isResending ||
this.state.timelineNeedsRefresh
) {
return STATUS_BAR_EXPANDED_LARGE;
}
return STATUS_BAR_HIDDEN;
Expand Down Expand Up @@ -306,6 +335,37 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> {
return this.getUnsentMessageContent();
}

if (this.state.timelineNeedsRefresh) {
return (
<div className="mx_RoomStatusBar mx_RoomStatusBar_unsentMessages">
<div role="alert">
<div className="mx_RoomStatusBar_unsentBadge">
<img
src={require("../../../res/img/feather-customised/warning-triangle.svg").default}
width="24"
height="24"
title="/!\ "
alt="/!\ " />
</div>
<div>
<div className="mx_RoomStatusBar_unsentTitle">
{ _t("History import detected.") }
</div>
<div className="mx_RoomStatusBar_unsentDescription">
{ _t("History was just imported somewhere in the room. " +
"In order to see the historical messages, refresh your timeline.") }
</div>
</div>
<div className="mx_RoomStatusBar_unsentButtonBar">
<AccessibleButton onClick={this.onRefreshTimelineClick} className="mx_RoomStatusBar_refreshTimelineBtn">
{ _t("Refresh timeline") }
</AccessibleButton>
</div>
</div>
</div>
);
}

return null;
}
}
3 changes: 3 additions & 0 deletions src/i18n/strings/en_EN.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3042,6 +3042,9 @@
"You can select all or individual messages to retry or delete": "You can select all or individual messages to retry or delete",
"Connectivity to the server has been lost.": "Connectivity to the server has been lost.",
"Sent messages will be stored until your connection has returned.": "Sent messages will be stored until your connection has returned.",
"History import detected.": "History import detected.",
"History was just imported somewhere in the room. In order to see the historical messages, refresh your timeline.": "History was just imported somewhere in the room. In order to see the historical messages, refresh your timeline.",
"Refresh timeline": "Refresh timeline",
"You seem to be uploading files, are you sure you want to quit?": "You seem to be uploading files, are you sure you want to quit?",
"You seem to be in a call, are you sure you want to quit?": "You seem to be in a call, are you sure you want to quit?",
"Search failed": "Search failed",
Expand Down
4 changes: 3 additions & 1 deletion src/indexing/EventIndex.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,9 @@ export default class EventIndex extends EventEmitter {
// Add the events to the timeline of the file panel.
matrixEvents.forEach(e => {
if (!timelineSet.eventIdToTimeline(e.getId())) {
timelineSet.addEventToTimeline(e, timeline, direction == EventTimeline.BACKWARDS);
timelineSet.addEventToTimeline(e, timeline, {
toStartOfTimeline: direction == EventTimeline.BACKWARDS,
});
}
});

Expand Down