Avoid unnecessary work when processing threads

[davidisaaclee]

Checklist

• Tests written for new code (and old code if feasible)
• Linter and other CI checks pass
• Sign-off given on the changes (see CONTRIBUTING.md)

---

This PR currently has none of the required changelog labels.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

[davidisaaclee]

I think this points to another possible improvement where the async code in Thread's constructor is moved to an async context. This would be a big breaking change, though.

[davidisaaclee]

@t3chguy to update here: I'm running into at least one failing test starting with the first commit in this PR
Avoid unnecessary fetch of thread root, which I'm working on fixing, but it's possible that this is opening a can of worms:

• The test I'm focusing on is https://github.com/davidisaaclee/matrix-js-sdk/blob/7cfedaae876d63ff5df48557c7001d3d86b9b5d3/spec/unit/room.spec.ts#L2521 – gist is that a thread is created, it gets a reply, that reply gets edited, Thread.replyToEvent should reflect the edited content of the reply
• What actually happens: the reply does not get the edit (makeReplaced is never called)
• Why? I'm still investigating this – some clues here are that eventShouldLiveIn returns shouldLiveInThread: false for edits made to a thread reply, which eventually prevents makeReplaced from getting called (I think? still reading...)
• Why did this work before 7cfedaa? I'm also not 💯 on this, but it seems like somehow this code relied on the fetchRootEvent calls to operate[0]. I still think we should be able to get rid of these (imo) extra fetchRootEvents, so I'm continuing to plug away here, but there is the chance that (1) this is requiring too much knowledge of matrix-js-sdk for me to implement quickly/cleanly, and (2) there will be many other broken bits due to this change. I'll update here and in MaxListenersExceededWarning on threads; many redundant fetches of thread root event #3463 if I give up :)

[0] tbc, this does not appear to be simply "the thread root's m.thread.latest_event was updated in the last call to fetchRootEvent, so the replyToEvent got updated" – changing this mocked fetchRootEvent to have a different m.thread payload does not fail the test.

[davidisaaclee]

Dumping some notes here for myself, and in case it helps someone down the line:

matrix-js-sdk/spec/unit/room.spec.ts

Line 2560 in c271e15

await room.addLiveEvents([randomMessage, threadRoot, threadResponse]);

• When this is run, a Thread is created. Thread constructor runs async code in sync context; a ThreadEvent.Update signals its completion.
• At the same time, these events are added to the the Thread via Room#addLiveEvents / Thread#addEvents. This triggers a ThreadEvent.Update.
• The emitPromise below tries to catch the completion of the Thread setup. But it really catches the addLiveEvents Update event.
• The effect is that the rest of this test runs before the Thread constructors has completed.
• Notably, the reply edit event is sent before the thread fetches its first set of messages.
• When the thread finishes its first fetch, it emits the Update event (which we tried to listen for initially) – at this point, it has processed the edited reply as part of its construction.

1. add root event + replies
2. test hangs on first await emitPromise
3. thread is constructed and begins async processing...
4. Thread.addEvents emits Thread.update, fulfills the test's first await emitPromise
5. add edit event
6. test hangs on second await emitPromise
7. thread paginates in all events as part of Thread.updateThreadMetadata, including the edit event
8. thread construction completes, emits Thread.update, fulfills the test's second await emitPromise
9. thread.replyToEvent is checked in test: it has the edit thanks to step 7

[davidisaaclee]

This waits for the correct signal that Thread.constructor has completed

[davidisaaclee]

adding events here (to Thread#replayEvents) means that Thread.constructor inserts these correctly regardless of async ordering; previously:

1. addLiveEvents called with multiple thread events
2. Thread.constructor is called using the thread root, starts async work...
3. the original addLiveEvents continues and tries to add the thread responses to the newly-created (but not fully initialized) Thread; fails check in Thread#addEvent where this.initialEventsFetched, so fails to add events to timeline
4. continuing Thread.constructor async: updateThreadMetadata is called, and tries to get initial fetch of events. somehow this doesn't work – I forget now how (sorry, writing these notes a few days after investigating...)

[davidisaaclee]

workaround for not refetching the thread root (on refetch, we'd get the latest reply event ID in the event's content)

[davidisaaclee]

I'm giving up on this for now :( too many headaches following racing async codepaths.

I'd love it if someone wanted to take over this branch. I added notes on this PR for my WIP, and I'd be more than happy to help handoff in any other way, since I still think this change is worthwhile.