Fix issues with implementation for MSC3981 #3448
Conversation
| @@ -29,6 +29,7 @@ import { | |||
| PendingEventOrdering, | |||
| RelationType, | |||
| Room, | |||
| UNSIGNED_THREAD_ID_FIELD, | |||
There was a problem hiding this comment.
generally, I think that using constants like this in tests is an antipattern. It means that the tests don't help us spot typos or unexpected changes in the behaviour of the constant, and it makes the tests harder to grok.
It would be better just to hardcode org.matrix.msc4023.thread_id.
| if (client.canSupport.get(Feature.RelationsRecursion) === ServerSupport.Unstable) { | ||
| params = replaceParam("recurse", "org.matrix.msc3981.recurse", params); | ||
| } |
There was a problem hiding this comment.
this feels like a sledgehammer to crack a nut. Shouldn't the calling test know if we expect recurse or org.matrix.msc3981.recurse?
| @@ -5774,6 +5776,10 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa | |||
| ...resOlder.chunk.map(mapper), | |||
| ]; | |||
| for (const event of events) { | |||
| event.setUnsigned({ | |||
| ...event.getUnsigned(), | |||
| [UNSIGNED_THREAD_ID_FIELD.name]: timelineSet.thread.id, | |||
There was a problem hiding this comment.
isn't this overriding the response from the server? Why are we doing that?
| unsigned: { | ||
| [UNSIGNED_THREAD_ID_FIELD.name]: THREAD_ROOT.event_id, | ||
| }, |
There was a problem hiding this comment.
I'm somewhat concerned that by changing all these, we never test the fallback behaviour. Do we have any separate tests for the fallback? Do we actually need to change all these definitions?
Type: Defect
Uses: matrix-org/matrix-spec-proposals#3981
Uses: matrix-org/matrix-spec-proposals#4023
Related: #3248
Related: #3427
Here's what your changelog entry will look like:
🐛 Bug Fixes