Skip to content

chore: Remove the old Latest Event API#5624

Merged
Hywan merged 5 commits intomatrix-org:mainfrom
Hywan:chore-remove-old-latest-event-api
Dec 5, 2025
Merged

chore: Remove the old Latest Event API#5624
Hywan merged 5 commits intomatrix-org:mainfrom
Hywan:chore-remove-old-latest-event-api

Conversation

@Hywan
Copy link
Copy Markdown
Member

@Hywan Hywan commented Sep 3, 2025

This set of patches remove the old latest event API, since the new one is now available and working.

Finally, this set of patches also remove the new_ prefix from the new latest event API.

🎉


@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 3, 2025

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.50%. Comparing base (bfd8cfa) to head (3121f27).
⚠️ Report is 11 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/matrix-sdk-base/src/room/room_info.rs 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5624      +/-   ##
==========================================
- Coverage   88.57%   88.50%   -0.07%     
==========================================
  Files         363      362       -1     
  Lines      104838   103281    -1557     
  Branches   104838   103281    -1557     
==========================================
- Hits        92856    91407    -1449     
+ Misses       7625     7533      -92     
+ Partials     4357     4341      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Hywan Hywan force-pushed the chore-remove-old-latest-event-api branch from dcb8306 to 03e6102 Compare September 3, 2025 14:52
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Sep 3, 2025

CodSpeed Performance Report

Merging #5624 will not alter performance

Comparing Hywan:chore-remove-old-latest-event-api (3121f27) with main (bfd8cfa)

Summary

✅ 50 untouched

@Hywan Hywan force-pushed the chore-remove-old-latest-event-api branch 2 times, most recently from eba17c6 to 31e3911 Compare September 3, 2025 15:27
@Hywan Hywan marked this pull request as ready for review September 3, 2025 15:36
@Hywan Hywan requested a review from a team as a code owner September 3, 2025 15:36
@Hywan Hywan requested review from poljar and removed request for a team September 3, 2025 15:36
@poljar
Copy link
Copy Markdown
Contributor

poljar commented Sep 4, 2025

This depends on #5617, right?

@Hywan
Copy link
Copy Markdown
Member Author

Hywan commented Sep 5, 2025

There are orthogonal. #5617 is adding a new feature to the new Latest Event API, while this set of patches removes the old API.

The last patch of this set introduce a renaming by removing the new_ prefix. It can conflict with #5617, but it's not a big deal.

Copy link
Copy Markdown
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

I think this looks fine, though it should only be merged after #5617 is merged, otherwise we're regressing in functionality.

This is also missing a changelog entry.

@Hywan
Copy link
Copy Markdown
Member Author

Hywan commented Sep 5, 2025

#5617 can be merged after this PR, it won't add any regression. Right now, with the old Latest Event API was not supporting local events, so it was impossible to sort by them.

However, #3872 must be addressed so that re-decryption happens in the Event Cache. The old Latest Event API has its own re-decryption mechanism. Knowing that the new Latest Event API relies on the Event Cache, as long as the Event Cache doesn't support redecryption, we will have a regression.

So:

@poljar
Copy link
Copy Markdown
Contributor

poljar commented Sep 5, 2025

#5617 can be merged after this PR, it won't add any regression. Right now, with the old Latest Event API was not supporting local events, so it was impossible to sort by them.

Ah, because #5587 added the new way to do latest events, and #5617 will only do the sorting based on the latest event.

Right, sorry for being confused.

@Hywan Hywan force-pushed the chore-remove-old-latest-event-api branch from 31e3911 to 9d9a9dc Compare December 4, 2025 13:55
@Hywan
Copy link
Copy Markdown
Member Author

Hywan commented Dec 4, 2025

Rebased on top of main. Ready to be reviewed!

@Hywan Hywan requested a review from poljar December 4, 2025 13:57
@Hywan Hywan force-pushed the chore-remove-old-latest-event-api branch from 9d9a9dc to 8266ef8 Compare December 4, 2025 13:59
Copy link
Copy Markdown
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Nice, though I think we don't want to just drop the old values because a field got renamed.

Also, I think that this warrants a changelog entry.

/// TODO(@hywan): Rename to `latest_event`.
#[serde(default)]
pub(crate) new_latest_event: LatestEventValue,
pub(crate) latest_event_value: LatestEventValue,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't this cause some values to be lost? Don't you need a serde alias for this rename to be a no-op?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is it annoying? This feature hasn't been part of the stable version of most clients. At worse, we are just losing a “cached” data, which is easily computed. My question is: do we need to maintain compatibility in this case?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Didn't the EX clients switch to this API? Won't they lose the latest event in that case?

If not, feel free to leave the alias out.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We agreed in our internal chat room that it's fine to drop. Only “nightly” or “beta” versions of Element X are using it, and no other clients seem to have switched to it so far. Moreover, Element X is using the LatestEventValue only recently in the UI: prior to that, it was only used by the Room List to sort items, but it was “invisible” to the app.

@Hywan Hywan requested a review from poljar December 5, 2025 07:41
Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Dec 5, 2025
@Hywan Hywan force-pushed the chore-remove-old-latest-event-api branch from e6da730 to 3121f27 Compare December 5, 2025 07:54
@Hywan
Copy link
Copy Markdown
Member Author

Hywan commented Dec 5, 2025

I've resolved the merge conflict. All good now.

Copy link
Copy Markdown
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Very nice -3k. Thanks.

@Hywan Hywan merged commit 238e4e8 into matrix-org:main Dec 5, 2025
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants