element-hq / element-web

A glossy Matrix collaboration client for the web.
https://element.io
GNU Affero General Public License v3.0
11.04k stars 1.96k forks source link

Only show files in the file panel with a truthy url #6250

Open turt2live opened 6 years ago

turt2live commented 6 years ago

Description

As per https://gitlab.gnome.org/GNOME/fractal/-/issues/101

... or to summarize the issue, an event like the following will appear in the file panel:

  "content": {
    "body": "Ooh, Fractal has an icon in the Papirus icon pack",
    "url": null,
    "msgtype": "m.text",
    "formatted_body": null,
    "format": null
  },

Version information

uhoreg commented 6 years ago

see also https://github.com/vector-im/riot-web/issues/4480 and https://github.com/vector-im/riot-web/issues/5076.

t3chguy commented 6 years ago

events are indexed with whether or not they contains_url so this'll be a Synapse bug :(

t3chguy commented 6 years ago

https://github.com/matrix-org/synapse/blob/752b7b32ed1c33651c0c64fbbb4289c3b62ac89b/synapse/storage/events.py#L1119-L1122

it'll also need some migration magic to fix this for all existing events in db

t3chguy commented 6 years ago

Although according to the above code null shouldn't be true for isinstance(event.content["url"], basestring)

t3chguy commented 6 years ago

This seems fine, so maybe its a fixed Synapse bug: image

t3chguy commented 6 years ago

weird, even though its right in the db and the filter seems fine the event is in my FilePanel image

t3chguy commented 6 years ago

ah turns out it is a Synapse bug where the is_url "formula" is inconsistent between files

t3chguy commented 6 years ago

though as per the spec: image it won't exclude non-truthy but instead non-string

t3chguy commented 6 years ago

This is actually a weird combination of a Synapse bug and a js-sdk one. So if the event was paginated in then the js-sdk bug would hit and otherwise the Synapse bug would hit.

t3chguy commented 6 years ago

though technically the matrix spec here could be understood to mean that "url": null should trip this Filter, Synapse in 2/3 places checked whether it was a string, Dendrite does not. So its a 50/50 right now. I might write a proposal to improve contains_url to checking for a truthy string to get around this

kittykat commented 3 years ago

@turt2live What would it take to resolve this issue? To do the checks in Dendrite?

turt2live commented 3 years ago

I don't think this is a dendrite issue. We should either filter it out locally, or declare it a synapse bug and move it there.