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.

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

Timeline displays edits to state events incorrectly #21851

Closed
richvdh opened this issue Apr 19, 2022 · 5 comments · Fixed by matrix-org/matrix-js-sdk#2306
Closed

Timeline displays edits to state events incorrectly #21851

richvdh opened this issue Apr 19, 2022 · 5 comments · Fixed by matrix-org/matrix-js-sdk#2306
Assignees
Labels
A-Message-Editing A-Timeline O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect

Comments

@richvdh
Copy link
Member

richvdh commented Apr 19, 2022

Steps to reproduce

Suppose I send an m.room.topic event with this content:

{
  "topic":"spam spam spam spam", 
  "m.new_content": {"topic": "normal topic"},
  "m.relates_to": {"rel_type":"m.replace","event_id":"<previous topic event>"}
}

This changes the topic of the room to spam spam spam spam. However, the timeline replaces the previous topic change event with:

image

(it doesn't even label it (edited) or allow you to view the edit history).

I feel like we probably shouldn't process edits of state events (or indeed anything that's not a (possibly encrypted) m.room.message).

Outcome

What did you expect?

The timeline should reflect reality. Since there were two separate topic changes, I think these should be shown as separate events, and the m.replace relation should be ignored.

What happened instead?

The timeline showed a state change that never happened

Operating system

No response

Browser information

No response

URL for webapp

develop.element.io

Application version

Element version: f46a6f2-react-70cdd57a5c61-js-540514c805ea Olm version: 3.2.8

Homeserver

No response

Will you send logs?

No

@t3chguy
Copy link
Member

t3chguy commented Apr 20, 2022

I feel like we probably shouldn't process edits of state events (or indeed anything that's not a (possibly encrypted) m.room.message).

Made a PR which ignores edits on state events, the latter would not match the MSC so erred away from it

@t3chguy t3chguy added S-Major Severely degrades major functionality or product features, with no satisfactory workaround O-Uncommon Most users are unlikely to come across this or unexpected workflow labels Apr 20, 2022
@richvdh
Copy link
Member Author

richvdh commented Apr 20, 2022

Made a PR which ignores edits on state events,

brilliant, thanks for jumping on this so quickly.

the latter would not match the MSC so erred away from it

I don't think the MSC is currently particularly clear on this point. We need to define what the expected behaviour actually is. Your interpretation is certainly valid, though, so I think I'll make the MSC match it.

Out of interest though: would we be able to tell the difference? In other words, does Element-web have any support for showing events in the timeline which aren't any of (a) a state event, (b) an m.room.message, (c) an encrypted m.room.message ?

@t3chguy
Copy link
Member

t3chguy commented Apr 20, 2022

I don't think the MSC is currently particularly clear on this point. We need to define what the expected behaviour actually is. Your interpretation is certainly valid, though, so I think I'll make the MSC match it.

The MSC explicitly says not state events, but yes you're right in that it doesn't specify an allow set

Only non-state events for now.

Out of interest though: would we be able to tell the difference? In other words, does Element-web have any support for showing events in the timeline which aren't any of (a) a state event, (b) an m.room.message, (c) an encrypted m.room.message ?

Polls can be edited via the UI even, they're of type org.matrix.msc3381.poll.start

Editing stickers is probably valid and functional too (m.sticker) - but EW has no UI to do such

@richvdh
Copy link
Member Author

richvdh commented Apr 20, 2022

The MSC explicitly says not state events, but yes you're right in that it doesn't specify an allow set

yeah, tbh I'm treating that "edge cases" section as "a bunch of stuff we haven't yet defined". But fine, let's do that.

Polls, stickers.

That's super-helpful, thanks.

su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this issue May 10, 2022
* Live location sharing: handle encrypted messages in processBeaconEvents ([\matrix-org#2327](matrix-org#2327)).
* Fix race conditions around threads ([\matrix-org#2331](matrix-org#2331)). Fixes element-hq/element-web#21627.
* Ignore m.replace relations on state events, they're invalid ([\matrix-org#2306](matrix-org#2306)). Fixes element-hq/element-web#21851.
* fix example in readme ([\matrix-org#2315](matrix-org#2315)).
* Don't decrement the length count of a thread when root redacted ([\matrix-org#2314](matrix-org#2314)).
* Prevent attempt to create thread with id "undefined" ([\matrix-org#2308](matrix-org#2308)).
* Update threads handling for replies-to-thread-responses as per MSC update ([\matrix-org#2305](matrix-org#2305)). Fixes element-hq/element-web#19678.
@richvdh
Copy link
Member Author

richvdh commented May 19, 2022

I'm not entirely convinced this is fixed. It's better, but not great. See #22280

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Message-Editing A-Timeline O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants