From 88239fd84998b56b20b35910f4561f5a6328b4f7 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 16 Sep 2021 00:21:25 -0500 Subject: [PATCH 1/4] Show updated relation reply from edited message Part of https://github.com/vector-im/element-web/issues/10391 When `m.relates_to` -> `m.in_reply_to` is provided in `m.new_content` for an edited message, use the updated reply. ex. ```json { "type": "m.room.message", "content": { "body": " * foo bar", "msgtype": "m.text", "m.new_content": { "body": "foo bar", "msgtype": "m.text", "m.relates_to": { "m.in_reply_to": { "event_id": "$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og" } } }, "m.relates_to": { "rel_type": "m.replace", "event_id": "$lX9MRe9ZTFOOvnU8PRVbvr1wqGtYvNQ1rSot-iUTN5k" } } } ``` --- src/components/views/elements/ReplyThread.tsx | 2 +- .../views/elements/ReplyThread-test.js | 210 ++++++++++++++++++ 2 files changed, 211 insertions(+), 1 deletion(-) create mode 100644 test/components/views/elements/ReplyThread-test.js diff --git a/src/components/views/elements/ReplyThread.tsx b/src/components/views/elements/ReplyThread.tsx index d061d52f461..91aaa64db77 100644 --- a/src/components/views/elements/ReplyThread.tsx +++ b/src/components/views/elements/ReplyThread.tsx @@ -88,7 +88,7 @@ export default class ReplyThread extends React.Component { // could be used here for replies as well... However, the helper // currently assumes the relation has a `rel_type`, which older replies // do not, so this block is left as-is for now. - const mRelatesTo = ev.getWireContent()['m.relates_to']; + const mRelatesTo = ev.getContent()['m.relates_to'] || ev.getWireContent()['m.relates_to']; if (mRelatesTo && mRelatesTo['m.in_reply_to']) { const mInReplyTo = mRelatesTo['m.in_reply_to']; if (mInReplyTo && mInReplyTo['event_id']) return mInReplyTo['event_id']; diff --git a/test/components/views/elements/ReplyThread-test.js b/test/components/views/elements/ReplyThread-test.js new file mode 100644 index 00000000000..8ade07fb203 --- /dev/null +++ b/test/components/views/elements/ReplyThread-test.js @@ -0,0 +1,210 @@ + + +import * as testUtils from '../../../test-utils'; +import ReplyThread from '../../../../src/components/views/elements/ReplyThread'; + +describe("ReplyThread", () => { + describe('getParentEventId', () => { + it('retrieves relation reply from unedited event', () => { + const originalEventWithRelation = testUtils.mkEvent({ + event: true, + type: "m.room.message", + content: { + msgtype: "m.text", + body: "> Reply to this message\n\n foo", + "m.relates_to": { + "m.in_reply_to": { + "event_id": "$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og" + } + } + }, + user: "some_other_user", + room: "room_id", + }); + + expect(ReplyThread.getParentEventId(originalEventWithRelation)) + .toStrictEqual('$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og'); + }); + + it('retrieves relation reply from original event when edited', () => { + const originalEventWithRelation = testUtils.mkEvent({ + event: true, + type: "m.room.message", + content: { + msgtype: "m.text", + body: "> Reply to this message\n\n foo", + "m.relates_to": { + "m.in_reply_to": { + "event_id": "$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og" + } + } + }, + user: "some_other_user", + room: "room_id", + }); + + const editEvent = testUtils.mkEvent({ + event: true, + type: "m.room.message", + content: { + msgtype: "m.text", + body: "> Reply to this message\n\n * foo bar", + "m.new_content": { + "msgtype": "m.text", + "body": "foo bar" + }, + "m.relates_to": { + "rel_type": "m.replace", + "event_id": originalEventWithRelation.event_id + }, + }, + user: "some_other_user", + room: "room_id", + }); + + // The edit replaces the original event + originalEventWithRelation.makeReplaced(editEvent); + + // The relation should be pulled from the original event + expect(ReplyThread.getParentEventId(originalEventWithRelation)) + .toStrictEqual('$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og'); + }); + + it('retrieves relation reply from edit event when provided', () => { + const originalEvent = testUtils.mkEvent({ + event: true, + type: "m.room.message", + content: { + msgtype: "m.text", + body: "foo", + }, + user: "some_other_user", + room: "room_id", + }); + + const editEvent = testUtils.mkEvent({ + event: true, + type: "m.room.message", + content: { + msgtype: "m.text", + body: "> Reply to this message\n\n * foo bar", + "m.new_content": { + "msgtype": "m.text", + "body": "foo bar", + "m.relates_to": { + "m.in_reply_to": { + "event_id": "$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og" + } + } + }, + "m.relates_to": { + "rel_type": "m.replace", + "event_id": originalEvent.event_id + }, + }, + user: "some_other_user", + room: "room_id", + }); + + // The edit replaces the original event + originalEvent.makeReplaced(editEvent); + + // The relation should be pulled from the edit event + expect(ReplyThread.getParentEventId(originalEvent)) + .toStrictEqual('$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og'); + }); + + it('prefers relation reply from edit event over original event', () => { + const originalEventWithRelation = testUtils.mkEvent({ + event: true, + type: "m.room.message", + content: { + msgtype: "m.text", + body: "> Reply to this message\n\n foo", + "m.relates_to": { + "m.in_reply_to": { + "event_id": "$111" + } + } + }, + user: "some_other_user", + room: "room_id", + }); + + const editEvent = testUtils.mkEvent({ + event: true, + type: "m.room.message", + content: { + msgtype: "m.text", + body: "> Reply to this message\n\n * foo bar", + "m.new_content": { + "msgtype": "m.text", + "body": "foo bar", + "m.relates_to": { + "m.in_reply_to": { + "event_id": "$999" + } + } + }, + "m.relates_to": { + "rel_type": "m.replace", + "event_id": originalEventWithRelation.event_id + }, + }, + user: "some_other_user", + room: "room_id", + }); + + // The edit replaces the original event + originalEventWithRelation.makeReplaced(editEvent); + + // The relation should be pulled from the edit event + expect(ReplyThread.getParentEventId(originalEventWithRelation)).toStrictEqual('$999'); + }); + + it('able to clear relation reply from original event by providing empty relation field', () => { + const originalEventWithRelation = testUtils.mkEvent({ + event: true, + type: "m.room.message", + content: { + msgtype: "m.text", + body: "> Reply to this message\n\n foo", + "m.relates_to": { + "m.in_reply_to": { + "event_id": "$111" + } + } + }, + user: "some_other_user", + room: "room_id", + }); + + const editEvent = testUtils.mkEvent({ + event: true, + type: "m.room.message", + content: { + msgtype: "m.text", + body: "> Reply to this message\n\n * foo bar", + "m.new_content": { + "msgtype": "m.text", + "body": "foo bar", + // Clear the relation from the original event + "m.relates_to": {} + }, + "m.relates_to": { + "rel_type": "m.replace", + "event_id": originalEventWithRelation.event_id + }, + }, + user: "some_other_user", + room: "room_id", + }); + + // The edit replaces the original event + originalEventWithRelation.makeReplaced(editEvent); + + // The relation should be pulled from the edit event + expect(ReplyThread.getParentEventId(originalEventWithRelation)).toStrictEqual(undefined); + }); + }) +}) From 3f9ad91691d5c19494bf13db4d61b44919862346 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 16 Sep 2021 00:27:15 -0500 Subject: [PATCH 2/4] Fix lints --- .../views/elements/ReplyThread-test.js | 93 +++++++++---------- 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/test/components/views/elements/ReplyThread-test.js b/test/components/views/elements/ReplyThread-test.js index 8ade07fb203..cb5d5f8e6b6 100644 --- a/test/components/views/elements/ReplyThread-test.js +++ b/test/components/views/elements/ReplyThread-test.js @@ -1,5 +1,4 @@ - import * as testUtils from '../../../test-utils'; import ReplyThread from '../../../../src/components/views/elements/ReplyThread'; @@ -10,18 +9,18 @@ describe("ReplyThread", () => { event: true, type: "m.room.message", content: { - msgtype: "m.text", - body: "> Reply to this message\n\n foo", + "msgtype": "m.text", + "body": "> Reply to this message\n\n foo", "m.relates_to": { "m.in_reply_to": { - "event_id": "$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og" - } - } + "event_id": "$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og", + }, + }, }, user: "some_other_user", room: "room_id", }); - + expect(ReplyThread.getParentEventId(originalEventWithRelation)) .toStrictEqual('$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og'); }); @@ -31,13 +30,13 @@ describe("ReplyThread", () => { event: true, type: "m.room.message", content: { - msgtype: "m.text", - body: "> Reply to this message\n\n foo", + "msgtype": "m.text", + "body": "> Reply to this message\n\n foo", "m.relates_to": { "m.in_reply_to": { - "event_id": "$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og" - } - } + "event_id": "$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og", + }, + }, }, user: "some_other_user", room: "room_id", @@ -47,15 +46,15 @@ describe("ReplyThread", () => { event: true, type: "m.room.message", content: { - msgtype: "m.text", - body: "> Reply to this message\n\n * foo bar", + "msgtype": "m.text", + "body": "> Reply to this message\n\n * foo bar", "m.new_content": { "msgtype": "m.text", - "body": "foo bar" + "body": "foo bar", }, "m.relates_to": { "rel_type": "m.replace", - "event_id": originalEventWithRelation.event_id + "event_id": originalEventWithRelation.event_id, }, }, user: "some_other_user", @@ -64,7 +63,7 @@ describe("ReplyThread", () => { // The edit replaces the original event originalEventWithRelation.makeReplaced(editEvent); - + // The relation should be pulled from the original event expect(ReplyThread.getParentEventId(originalEventWithRelation)) .toStrictEqual('$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og'); @@ -86,20 +85,20 @@ describe("ReplyThread", () => { event: true, type: "m.room.message", content: { - msgtype: "m.text", - body: "> Reply to this message\n\n * foo bar", + "msgtype": "m.text", + "body": "> Reply to this message\n\n * foo bar", "m.new_content": { "msgtype": "m.text", "body": "foo bar", "m.relates_to": { "m.in_reply_to": { - "event_id": "$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og" - } - } + "event_id": "$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og", + }, + }, }, "m.relates_to": { "rel_type": "m.replace", - "event_id": originalEvent.event_id + "event_id": originalEvent.event_id, }, }, user: "some_other_user", @@ -108,7 +107,7 @@ describe("ReplyThread", () => { // The edit replaces the original event originalEvent.makeReplaced(editEvent); - + // The relation should be pulled from the edit event expect(ReplyThread.getParentEventId(originalEvent)) .toStrictEqual('$qkjmFBTEc0VvfVyzq1CJuh1QZi_xDIgNEFjZ4Pq34og'); @@ -119,13 +118,13 @@ describe("ReplyThread", () => { event: true, type: "m.room.message", content: { - msgtype: "m.text", - body: "> Reply to this message\n\n foo", + "msgtype": "m.text", + "body": "> Reply to this message\n\n foo", "m.relates_to": { "m.in_reply_to": { - "event_id": "$111" - } - } + "event_id": "$111", + }, + }, }, user: "some_other_user", room: "room_id", @@ -135,20 +134,20 @@ describe("ReplyThread", () => { event: true, type: "m.room.message", content: { - msgtype: "m.text", - body: "> Reply to this message\n\n * foo bar", + "msgtype": "m.text", + "body": "> Reply to this message\n\n * foo bar", "m.new_content": { "msgtype": "m.text", "body": "foo bar", "m.relates_to": { "m.in_reply_to": { - "event_id": "$999" - } - } + "event_id": "$999", + }, + }, }, "m.relates_to": { "rel_type": "m.replace", - "event_id": originalEventWithRelation.event_id + "event_id": originalEventWithRelation.event_id, }, }, user: "some_other_user", @@ -157,7 +156,7 @@ describe("ReplyThread", () => { // The edit replaces the original event originalEventWithRelation.makeReplaced(editEvent); - + // The relation should be pulled from the edit event expect(ReplyThread.getParentEventId(originalEventWithRelation)).toStrictEqual('$999'); }); @@ -167,13 +166,13 @@ describe("ReplyThread", () => { event: true, type: "m.room.message", content: { - msgtype: "m.text", - body: "> Reply to this message\n\n foo", + "msgtype": "m.text", + "body": "> Reply to this message\n\n foo", "m.relates_to": { "m.in_reply_to": { - "event_id": "$111" - } - } + "event_id": "$111", + }, + }, }, user: "some_other_user", room: "room_id", @@ -183,17 +182,17 @@ describe("ReplyThread", () => { event: true, type: "m.room.message", content: { - msgtype: "m.text", - body: "> Reply to this message\n\n * foo bar", + "msgtype": "m.text", + "body": "> Reply to this message\n\n * foo bar", "m.new_content": { "msgtype": "m.text", "body": "foo bar", // Clear the relation from the original event - "m.relates_to": {} + "m.relates_to": {}, }, "m.relates_to": { "rel_type": "m.replace", - "event_id": originalEventWithRelation.event_id + "event_id": originalEventWithRelation.event_id, }, }, user: "some_other_user", @@ -206,5 +205,5 @@ describe("ReplyThread", () => { // The relation should be pulled from the edit event expect(ReplyThread.getParentEventId(originalEventWithRelation)).toStrictEqual(undefined); }); - }) -}) + }); +}); From 9f6a5b8fcdb4a1f2e442c4892cfac65af9b15e53 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 16 Sep 2021 00:36:51 -0500 Subject: [PATCH 3/4] Fix test failing because skinned sdk not initialized Fixes: ``` Attempted to get a component (views.elements.Flair) before a skin has been loaded. This is probably because either: a) Your app has not called sdk.loadSkin(), or b) A component has called getComponent at the root level 23 | if (!name) throw new Error(`Invalid component name: ${name}`); 24 | if (this.components === null) { > 25 | throw new Error( | ^ 26 | `Attempted to get a component (${name}) before a skin has been loaded.`+ 27 | " This is probably because either:"+ 28 | " a) Your app has not called sdk.loadSkin(), or"+ at Skinner.getComponent (src/Skinner.js:25:19) at Object.getComponent (src/index.js:29:20) at src/utils/replaceableComponent.ts:41:22 at Object. (src/components/views/elements/Flair.js:65:94) at Object. (src/components/views/messages/SenderProfile.tsx:21:1) at Object. (src/components/views/rooms/ReplyTile.tsx:23:1) at Object. (src/components/views/elements/ReplyThread.tsx:34:1) at Object. (test/components/views/elements/ReplyThread-test.js:3:1) ``` --- test/components/views/elements/ReplyThread-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/components/views/elements/ReplyThread-test.js b/test/components/views/elements/ReplyThread-test.js index cb5d5f8e6b6..ee81c2f210c 100644 --- a/test/components/views/elements/ReplyThread-test.js +++ b/test/components/views/elements/ReplyThread-test.js @@ -1,4 +1,4 @@ - +import "../../../skinned-sdk"; import * as testUtils from '../../../test-utils'; import ReplyThread from '../../../../src/components/views/elements/ReplyThread'; From 7d88e4d24c9d26f6cf3e147682d475cbb41ea5f0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 16 Sep 2021 01:14:29 -0500 Subject: [PATCH 4/4] Add comment explaining why both content methods --- src/components/views/elements/ReplyThread.tsx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/components/views/elements/ReplyThread.tsx b/src/components/views/elements/ReplyThread.tsx index 91aaa64db77..59c827d5d81 100644 --- a/src/components/views/elements/ReplyThread.tsx +++ b/src/components/views/elements/ReplyThread.tsx @@ -88,6 +88,12 @@ export default class ReplyThread extends React.Component { // could be used here for replies as well... However, the helper // currently assumes the relation has a `rel_type`, which older replies // do not, so this block is left as-is for now. + // + // We're prefer ev.getContent() over ev.getWireContent() to make sure + // we grab the latest edit with potentially new relations. But we also + // can't just rely on ev.getContent() by itself because historically we + // still show the reply from the original message even though the edit + // event does not include the relation reply. const mRelatesTo = ev.getContent()['m.relates_to'] || ev.getWireContent()['m.relates_to']; if (mRelatesTo && mRelatesTo['m.in_reply_to']) { const mInReplyTo = mRelatesTo['m.in_reply_to'];