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

Feeds event with relation to unknown to the widget #12283

Merged
merged 3 commits into from
Feb 29, 2024
Merged

Feeds event with relation to unknown to the widget #12283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
33f576c
Feeds event with relation to unknown to the widget
Feb 27, 2024
a7143bb
Merge branch 'develop' into nic/feat/feed_event_with_relation_unknown
maheichyk Feb 27, 2024
1e203cb
Smaller changes
Feb 27, 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
23 changes: 17 additions & 6 deletions src/stores/widgets/StopGapWidget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,14 +495,20 @@ export class StopGapWidget extends EventEmitter {
//
// This approach of "read up to" prevents widgets receiving decryption spam from startup or
// receiving out-of-order events from backfill and such.
//
// Skip marker timeline check for events with relations to unknown parent because these
// events are not added to the timeline here and will be ignored otherwise:
// https://github.com/matrix-org/matrix-js-sdk/blob/d3dfcd924201d71b434af3d77343b5229b6ed75e/src/models/room.ts#L2207-L2213
let isRelationToUnknown: boolean | undefined = undefined;
const upToEventId = this.readUpToMap[ev.getRoomId()!];
if (upToEventId) {
// Small optimization for exact match (prevent search)
if (upToEventId === ev.getId()) {
return;
}

let isBeforeMark = true;
// should be true to forward the event to the widget
let shouldForward = false;

const room = this.client.getRoom(ev.getRoomId()!);
if (!room) return;
Expand All @@ -515,14 +521,19 @@ export class StopGapWidget extends EventEmitter {
if (timelineEvent.getId() === upToEventId) {
break;
} else if (timelineEvent.getId() === ev.getId()) {
isBeforeMark = false;
shouldForward = true;
break;
}
}

if (isBeforeMark) {
// Ignore the event: it is before our interest.
return;
if (!shouldForward) {
// checks that the event has a relation to unknown event
isRelationToUnknown =
!ev.replyEventId && !!ev.relationEventId && !room.findEventById(ev.relationEventId);
if (!isRelationToUnknown) {
// Ignore the event: it is before our interest.
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this seems reasonable, although maybe we should default to sending the event on if we can't find either the event or the receipt, rather than special-casing events whose parent we can't find? (ie. rename isBeforeMark to shouldForward or something, set it to true to start with and set it to false in the first clause rather than the second... and comment it, obviously).

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 have changed isBeforeMark to shouldForward that makes easier to understand this logic, but I can’t follow on the suggestion to change:

although maybe we should default to sending the event on if we can't find either the event or the receipt, rather than special-casing events whose parent we can't find

It not looks clear to me how event or the receipt find could resolve the same issue. Could you please explain more exactly what you mean with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbkr could you please have a look on the changes?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I was just suggesting that, since the problem here is that we can't prove that the event is after the read marker because we can't find the event, we could also just default to forwarding if we can't prove the event hasn't been read, which I think would probably be safe enough. That said, this is a smaller change, so it seems fine.

}
}

Expand All @@ -533,7 +544,7 @@ export class StopGapWidget extends EventEmitter {
const evId = ev.getId();
if (evRoomId && evId) {
const room = this.client.getRoom(evRoomId);
if (room && room.getMyMembership() === "join") {
if (room && room.getMyMembership() === "join" && !isRelationToUnknown) {
this.readUpToMap[evRoomId] = evId;
}
}
Expand Down
101 changes: 100 additions & 1 deletion test/stores/widgets/StopGapWidget-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ limitations under the License.

import { mocked, MockedObject } from "jest-mock";
import { last } from "lodash";
import { MatrixEvent, MatrixClient, ClientEvent } from "matrix-js-sdk/src/matrix";
import { MatrixEvent, MatrixClient, ClientEvent, EventTimeline } from "matrix-js-sdk/src/matrix";
import { ClientWidgetApi, WidgetApiFromWidgetAction } from "matrix-widget-api";
import { waitFor } from "@testing-library/react";

Expand Down Expand Up @@ -88,6 +88,105 @@ describe("StopGapWidget", () => {
expect(messaging.feedToDevice).toHaveBeenCalledWith(event.getEffectiveEvent(), false);
});

describe("feed event", () => {
let event1: MatrixEvent;
let event2: MatrixEvent;

beforeEach(() => {
event1 = mkEvent({
event: true,
id: "$event-id1",
type: "org.example.foo",
user: "@alice:example.org",
content: { hello: "world" },
room: "!1:example.org",
});

event2 = mkEvent({
event: true,
id: "$event-id2",
type: "org.example.foo",
user: "@alice:example.org",
content: { hello: "world" },
room: "!1:example.org",
});

const room = mkRoom(client, "!1:example.org");
client.getRoom.mockImplementation((roomId) => (roomId === "!1:example.org" ? room : null));
room.getLiveTimeline.mockReturnValue({
getEvents: (): MatrixEvent[] => [event1, event2],
} as unknown as EventTimeline);

messaging.feedEvent.mockResolvedValue();
});

it("feeds incoming event to the widget", async () => {
client.emit(ClientEvent.Event, event1);
expect(messaging.feedEvent).toHaveBeenCalledWith(event1.getEffectiveEvent(), "!1:example.org");

client.emit(ClientEvent.Event, event2);
expect(messaging.feedEvent).toHaveBeenCalledTimes(2);
expect(messaging.feedEvent).toHaveBeenLastCalledWith(event2.getEffectiveEvent(), "!1:example.org");
});

it("should not feed incoming event to the widget if seen already", async () => {
client.emit(ClientEvent.Event, event1);
expect(messaging.feedEvent).toHaveBeenCalledWith(event1.getEffectiveEvent(), "!1:example.org");

client.emit(ClientEvent.Event, event2);
expect(messaging.feedEvent).toHaveBeenCalledTimes(2);
expect(messaging.feedEvent).toHaveBeenLastCalledWith(event2.getEffectiveEvent(), "!1:example.org");

client.emit(ClientEvent.Event, event1);
expect(messaging.feedEvent).toHaveBeenCalledTimes(2);
expect(messaging.feedEvent).toHaveBeenLastCalledWith(event2.getEffectiveEvent(), "!1:example.org");
});

it("should not feed incoming event if not in timeline", () => {
const event = mkEvent({
event: true,
id: "$event-id",
type: "org.example.foo",
user: "@alice:example.org",
content: {
hello: "world",
},
room: "!1:example.org",
});

client.emit(ClientEvent.Event, event);
expect(messaging.feedEvent).toHaveBeenCalledWith(event.getEffectiveEvent(), "!1:example.org");
});

it("feeds incoming event that is not in timeline but relates to unknown parent to the widget", async () => {
const event = mkEvent({
event: true,
id: "$event-idRelation",
type: "org.example.foo",
user: "@alice:example.org",
content: {
"hello": "world",
"m.relates_to": {
event_id: "$unknown-parent",
rel_type: "m.reference",
},
},
room: "!1:example.org",
});

client.emit(ClientEvent.Event, event1);
expect(messaging.feedEvent).toHaveBeenCalledWith(event1.getEffectiveEvent(), "!1:example.org");

client.emit(ClientEvent.Event, event);
expect(messaging.feedEvent).toHaveBeenCalledTimes(2);
expect(messaging.feedEvent).toHaveBeenLastCalledWith(event.getEffectiveEvent(), "!1:example.org");

client.emit(ClientEvent.Event, event1);
expect(messaging.feedEvent).toHaveBeenCalledTimes(2);
expect(messaging.feedEvent).toHaveBeenLastCalledWith(event.getEffectiveEvent(), "!1:example.org");
});
});

describe("when there is a voice broadcast recording", () => {
let voiceBroadcastInfoEvent: MatrixEvent;
let voiceBroadcastRecording: VoiceBroadcastRecording;
Expand Down
Loading