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.

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

correctly accumulate sync summaries. #3366

Merged
merged 1 commit into from
May 16, 2023

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented May 16, 2023

if a sync summary for (say) invited_member_count goes from 1 to 0, it should be accumluated as 0, rather than 1.

Fixes element-hq/element-web#23345


Here's what your changelog entry will look like:

🐛 Bug Fixes

if a sync summary for (say) invited_member_count goes from 1 to 0, it should be
accumluated as 0, rather than 1.

Should fix element-hq/element-web#23345
@ara4n ara4n requested a review from a team as a code owner May 16, 2023 10:34
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Fantastic, thank you!
Some attempts to clarify wording in the suggestions.

@@ -545,6 +545,23 @@ describe("SyncAccumulator", function () {
expect(summary["m.heroes"]).toEqual(["@bob:bar"]);
});

it("should reset summary properties", function () {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it("should reset summary properties", function () {
it("should correctly update summary properties to zero", function () {

@@ -545,6 +545,23 @@ describe("SyncAccumulator", function () {
expect(summary["m.heroes"]).toEqual(["@bob:bar"]);
});

it("should reset summary properties", function () {
sa.accumulate(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sa.accumulate(
// When we receive updates of a summary property, the last of which is 0
sa.accumulate(

"m.invited_member_count": 0,
}),
);
const summary = sa.getJSON().roomsData.join["!foo:bar"].summary;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const summary = sa.getJSON().roomsData.join["!foo:bar"].summary;
// Then we give an answer of 0
const summary = sa.getJSON().roomsData.join["!foo:bar"].summary;

@ara4n ara4n added this pull request to the merge queue May 16, 2023
Merged via the queue into develop with commit d459a91 May 16, 2023
@ara4n ara4n deleted the matthew/fix-accumulated-sync-summaries branch May 16, 2023 10:56
@ara4n ara4n restored the matthew/fix-accumulated-sync-summaries branch May 16, 2023 11:03
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Dec 13, 2023
* 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)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synthetic names for unnamed 3-way rooms are useless.
3 participants