Skip to content

threads: Bulk process thread subscription updates from sync and companion enpoint#5848

Merged
Hywan merged 2 commits intomatrix-org:mainfrom
razvp:thread-subs-bulk-upsert
Dec 17, 2025
Merged

threads: Bulk process thread subscription updates from sync and companion enpoint#5848
Hywan merged 2 commits intomatrix-org:mainfrom
razvp:thread-subs-bulk-upsert

Conversation

@razvp
Copy link
Copy Markdown
Contributor

@razvp razvp commented Nov 10, 2025

Fixes #5610

  • Public API changes documented in changelogs (optional)

Signed-off-by: Razvan razvanvp@gmail.com

@razvp razvp requested a review from a team as a code owner November 10, 2025 17:50
@razvp razvp requested review from Hywan and removed request for a team November 10, 2025 17:50
@razvp razvp force-pushed the thread-subs-bulk-upsert branch from 2417153 to cfcfa42 Compare November 10, 2025 17:59
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 10, 2025

Codecov Report

❌ Patch coverage is 94.27313% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.55%. Comparing base (1af22a7) to head (d4d5d6e).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...tes/matrix-sdk-base/src/store/integration_tests.rs 93.42% 0 Missing and 10 partials ⚠️
crates/matrix-sdk-sqlite/src/state_store.rs 94.28% 0 Missing and 2 partials ⚠️
...ates/matrix-sdk/src/client/thread_subscriptions.rs 97.22% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #5848    +/-   ##
========================================
  Coverage   88.54%   88.55%            
========================================
  Files         363      363            
  Lines      103703   103885   +182     
  Branches   103703   103885   +182     
========================================
+ Hits        91821    91992   +171     
- Misses       7517     7519     +2     
- Partials     4365     4374     +9     

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

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Nov 10, 2025

CodSpeed Performance Report

Merging #5848 will not alter performance

Comparing razvp:thread-subs-bulk-upsert (d4d5d6e) with main (1af22a7)

Summary

✅ 50 untouched

@razvp razvp force-pushed the thread-subs-bulk-upsert branch from cfcfa42 to 9a19738 Compare November 10, 2025 21:10
@Hywan
Copy link
Copy Markdown
Member

Hywan commented Nov 11, 2025

Thanks! It looks like a solid contribution, thank you. However, I wonder if you could add a bit of context and to explain what you're doing, and why, please?

@razvp razvp force-pushed the thread-subs-bulk-upsert branch from 9a19738 to 07a7e45 Compare November 12, 2025 08:07
@razvp
Copy link
Copy Markdown
Contributor Author

razvp commented Nov 12, 2025

Thanks! It looks like a solid contribution, thank you. However, I wonder if you could add a bit of context and to explain what you're doing, and why, please?

The sliding sync and back-pagination endpoints provide thread subscription updates in batches, but currently the SDK stores/updates them one by one. (MSC4308)
This PR changes that by processing them in bulk, in one database transaction.

The first commit adds the upsert_thread_subscriptions() method to the StateStore trait and implements it for the 3 storage types. The IndexeddbStateStore and MemoryStore implementations are very similar to the existing single update per transaction
The second commit updates ThreadSubscriptionCatchup::sync_subscriptions and ThreadSubscriptionCatchup::thread_subscriptions_catchup_task to actually use the bulk api.

Most of the bump_stamp logic is gathered from the existing test. (ex: if an update doesn't have a bump_stamp and the stored subscription has one, then keep the existing one)

Switching to a bulk api was suggested in this comment.

@Hywan
Copy link
Copy Markdown
Member

Hywan commented Nov 25, 2025

Can you please fix the broken test?

@razvp
Copy link
Copy Markdown
Contributor Author

razvp commented Nov 25, 2025

cargo test --package matrix-sdk-ui --test integration -- room_list_service::test_thread_subscriptions_extension_enabled_only_if_server_advertises_it

test room_list_service::test_thread_subscriptions_extension_enabled_only_if_server_advertises_it ... ok

It passes locally and in the first CI run all tests passed. It does contain thread_subscriptions in the name, but i didn't edit this test or logic around it :D

I don't think i can re-run the pipeline. Can you re-run it or should i do a rebase to trigger it?

@Hywan
Copy link
Copy Markdown
Member

Hywan commented Dec 16, 2025

For a reason I ignore, I can't re-run the jobs… Can you push an empty commit please, or rebase on top of main and force-push (better)?

@razvp
Copy link
Copy Markdown
Contributor Author

razvp commented Dec 16, 2025

Sure, i'll rebase it

@razvp razvp force-pushed the thread-subs-bulk-upsert branch from 07a7e45 to d4d5d6e Compare December 16, 2025 18:47
Copy link
Copy Markdown
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

It looks good to me, thanks!

@Hywan Hywan merged commit ea43e3f into matrix-org:main Dec 17, 2025
53 checks passed
@Hywan
Copy link
Copy Markdown
Member

Hywan commented Dec 17, 2025

I think it's worth considering the upsert_thread_subscription (singular, not plural) method now, what do you think?

@razvp
Copy link
Copy Markdown
Contributor Author

razvp commented Dec 17, 2025

Thank you for reviewing!

Yes, the old method can be removed. Could you please assign the new issue to me?

@Hywan
Copy link
Copy Markdown
Member

Hywan commented Dec 19, 2025

I can't assign it to you, but feel free to comment you're working on it!

Comment on lines +53 to +54
- Add `StateStore::upsert_thread_subscriptions()` method for bulk upserts.
([#5848](https://github.com/matrix-org/matrix-rust-sdk/pull/5848))
Copy link
Copy Markdown
Member

@richvdh richvdh Jan 6, 2026

Choose a reason for hiding this comment

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

In our changelogs, the most recent changes go at the top. Also, this did not land in the 0.16.0 release, so it is in completely the wrong place.

@Hywan as reviewer, please can you take responsibility for fixing up the changelogs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Done in #6011.

@razvp razvp deleted the thread-subs-bulk-upsert branch April 27, 2026 12:16
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.

store: implement a bulk API to store thread subscriptions

3 participants