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.

Sign up for GitHub

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

Jump to bottom

Ensure we do not add relations to the wrong timeline #3427

Merged
merged 19 commits into from
Jun 1, 2023
Merged

Ensure we do not add relations to the wrong timeline #3427

merged 19 commits into from
Jun 1, 2023

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented May 31, 2023 •
edited by github-actions bot
Loading

Fixes element-hq/element-web#25450
Fixes element-hq/element-web#25494

Uses matrix-org/matrix-spec-proposals#4023

If MSC4023 unsigned data is not included it'll use the /event/ API to fetch the parent event to determine what thread to associate the relation with, it'll then store that unsigned in the sync accumulator to prevent needing to do that work again.

Notes: Makes Room::addLiveEvents async so it is able to fetch more context about relations where needed.

Here's what your changelog entry will look like:

🚨 BREAKING CHANGES

germain-gg
germain-gg reviewed May 31, 2023
View reviewed changes
src/models/event.ts Outdated Show resolved Hide resolved
@Johennes Johennes mentioned this pull request Jun 1, 2023
Closed
@t3chguy t3chguy marked this pull request as ready for review June 1, 2023 09:58
@richvdh
Copy link
Member

richvdh commented Jun 1, 2023

If MSC4023 unsigned data is not included it'll use the /event/ API to fetch the parent event to determine what thread to associate the relation with, it'll then store that unsigned in the sync accumulator to prevent needing to do that work again.

According to the MSC, the thread_id is only included for reactions to threaded events... so we'll end up doing this for every reaction on the main thread? In which case, we might as well do it for threaded reactions too and the whole MSC is redundant.

Seems like something that needs changing in the MSC to me.

@richvdh richvdh mentioned this pull request Jun 1, 2023
Open
4 tasks
@t3chguy
Copy link
Member Author

t3chguy commented Jun 1, 2023 •
edited
Loading

According to the MSC, the thread_id is only included for reactions to threaded events... so we'll end up doing this for every reaction on the main thread? In which case, we might as well do it for threaded reactions too and the whole MSC is redundant.

Good point, "thread_id": null or signalling support via /versions for the MSC could be used to catch that edge

germain-gg
germain-gg approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Believe this is a net improvement to what we have today

src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Show resolved Hide resolved
@t3chguy t3chguy requested a review from germain-gg June 1, 2023 14:56
@t3chguy t3chguy enabled auto-merge June 1, 2023 15:08
@t3chguy t3chguy disabled auto-merge June 1, 2023 15:08
@artcodespace artcodespace removed their request for review June 1, 2023 15:22
@t3chguy t3chguy added this pull request to the merge queue Jun 1, 2023
Merged via the queue into develop with commit 71f9b25 Jun 1, 2023
@t3chguy t3chguy deleted the t3chguy/fix/25450 branch June 1, 2023 15:41
@Johennes Johennes mentioned this pull request Jun 2, 2023
Open
@richvdh
Copy link
Member

richvdh commented Jun 2, 2023

Should the changelog entry not explain what exactly about this is a breaking change?

@Johennes Johennes mentioned this pull request Jun 2, 2023
Open
t3chguy added a commit that referenced this pull request Jun 2, 2023
@t3chguy
Revert "Ensure we do not add relations to the wrong timeline (#3427)"
cc4a7c1 
This reverts commit 71f9b25.
@justjanne justjanne mentioned this pull request Jun 6, 2023
Closed
@andybalaam andybalaam mentioned this pull request Jun 9, 2023
Closed
@Johennes Johennes mentioned this pull request Jul 4, 2023
Closed
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Dec 13, 2023
@su-ex
Merge tag 'v26.0.0' into sc
54d9823 
* Ensure we do not add relations to the wrong timeline ([\matrix-org#3427](matrix-org#3427)). Fixes element-hq/element-web#25450 and element-hq/element-web#25494.
* Deprecate `QrCodeEvent`, `SasEvent` and `VerificationEvent` ([\matrix-org#3386](matrix-org#3386)).
* Move crypto classes into a separate namespace ([\matrix-org#3385](matrix-org#3385)).
* Mention deno support in the README ([\matrix-org#3417](matrix-org#3417)). Contributed by @sigmaSd.
* Mark room version 10 as safe ([\matrix-org#3425](matrix-org#3425)).
* Prioritise entirely supported flows for UIA ([\matrix-org#3402](matrix-org#3402)).
* Add methods to terminate idb worker ([\matrix-org#3362](matrix-org#3362)).
* Total summary count ([\matrix-org#3351](matrix-org#3351)). Contributed by @toger5.
* Audio concealment ([\matrix-org#3349](matrix-org#3349)). Contributed by @toger5.
* Correctly accumulate sync summaries. ([\matrix-org#3366](matrix-org#3366)). Fixes element-hq/element-web#23345.
* Keep measuring a call feed's volume after a stream replacement ([\matrix-org#3361](matrix-org#3361)). Fixes element-hq/element-call#1051.
* Element-R: Avoid uploading a new fallback key at every `/sync` ([\matrix-org#3338](matrix-org#3338)). Fixes element-hq/element-web#25215.
* Accumulate receipts for the main thread and unthreaded separately ([\matrix-org#3339](matrix-org#3339)). Fixes element-hq/element-web#24629.
* Remove spec non-compliant extended glob format ([\matrix-org#3423](matrix-org#3423)). Fixes element-hq/element-web#25474.
* Fix bug where original event was inserted into timeline instead of the edit event ([\matrix-org#3398](matrix-org#3398)). Contributed by @andybalaam.
* Only add a local receipt if it's after an existing receipt ([\matrix-org#3399](matrix-org#3399)). Contributed by @andybalaam.
* Attempt a potential workaround for stuck notifs ([\matrix-org#3384](matrix-org#3384)). Fixes element-hq/element-web#25406. Contributed by @andybalaam.
* Fix verification bug with `pendingEventOrdering: "chronological"` ([\matrix-org#3382](matrix-org#3382)).
@matrixbot matrixbot mentioned this pull request Dec 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@germain-gg germain-gg germain-gg approved these changes

@richvdh richvdh Awaiting requested review from richvdh richvdh is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees
No one assigned
Labels
T-Defect X-Breaking-Change
Projects
None yet
Milestone
No milestone
3 participants
@t3chguy @richvdh @germain-gg