Include redundant members in most timeline filters

[morguldir]

Synapse never tries to avoid redundant members anyway, so it's hard to ensure that it works correctly (https://github.com/element-hq/synapse/blob/568051c0f07393b786b9d813a1db53dd332c9fc2/synapse/handlers/pagination.py#L639)
It also seems like it would be difficult when the "Show current profile picture and name for users in message history" option is turned off

Conduwuit and Dendrite do attempt to remove redundant members, and it seems to cause some problems, so this commit tells them to include redundant members

Similar to element-hq/element-web#7417, which is also why there might still be missing data in rare cases after this

element-web notes: Fix some cases where profiles would be missing when using conduwuit or dendrite

Signed-off-by: morguldir [email protected]

Checklist

• Tests written for new code (and old code if feasible).
• New or updated public/exported symbols have accurate TSDoc documentation.
• Linter and other CI checks pass.
• Sign-off given on the changes (see CONTRIBUTING.md).

[t3chguy]

I believe this will have a lack of migration problem, changing filters with an existing sync token is not explicitly supported within the spec

[morguldir]

I believe this will have a lack of migration problem, changing filters with an existing sync token is not explicitly supported within the spec

This matrix-org/matrix-spec#1304?

My intuition says that it would be fine in this case, since it only changes the returned state and not timeline events themselves 🤔

[t3chguy]

When processing a sequence of events (e.g. by looping on /sync or paginating /messages), it is common for blocks of events in the sequence to share a similar set of senders. Rather than responses in the sequence sending duplicate membership events for these senders to the client, the server MAY assume that clients will remember membership events they have already been sent, and choose to skip sending membership events for members whose membership has not changed. These are called ‘redundant membership events’. Clients may request that redundant membership events are always included in responses by setting include_redundant_members to true in the filter.

This doesn't sound like a thing we need though, even if changing the filter mid-sync-stream is fine. js-sdk clients do remember memberships.

[morguldir]

That's true, but an older event obtained via scrollback will probably have a different state, and this might to be why element-web expects the state to be included, I'm not totally sure how else a client would implement historically accurate memberships in messages anyway

But perhaps it would be better to make this an option that e.g. the react-sdk would set if useOnlyCurrentProfiles is disabled, although this doesn't change anything for synapse users anyway, and my intention was to just do it for scrollback-like timelines

[t3chguy]

I think this could do with an integ test which demonstrates that edge, rather than just testing the right params are sent

[morguldir]

Hopefully that roughly demonstrates it

[t3chguy]

Your tests don't look to be passing

[morguldir]

Indeed, i swear they passed locally earlier 😔

Maybe it requires a bit more pagination for the bug to appear

[richvdh]

@morguldir are you still working on this? We'll need the tests to pass before it can land, obviously.

[richvdh]

Things exported in the public API should have documentation