fix(communities)!: stop syncing community on `LastOpenedAt` update #5884

osmaczko · 2024-09-27T09:16:28Z

Syncing the entire community for potentially frequent actions like LastOpenedAt updates is highly inefficient when using Waku as the transport layer, as it can result in significant bandwidth overhead.

NOTE: it might break syncing flow on mobile as LastOpenedAt is no longer synced.

github-actions · 2024-09-27T09:18:01Z

Looks like you have BREAKING CHANGES in your PR.
Please make sure to follow 💔How to introduce breaking changes guide:

Check-list

Tried to avoid this breaking change
Updated status-desktop
Updated status-mobile

status-im-auto · 2024-09-27T09:20:19Z

Jenkins Builds

Click to see older builds (16)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`555a7dd`	#1	2024-09-27 09:20:04	~3 min	`tests-rpc`	📄`log`
✔️	`555a7dd`	#1	2024-09-27 09:20:54	~4 min	`linux`	📦`zip`
✔️	`555a7dd`	#1	2024-09-27 09:21:39	~4 min	`android`	📦`aar`
✔️	`555a7dd`	#1	2024-09-27 09:22:20	~5 min	`ios`	📦`zip`
✖️	`555a7dd`	#1	2024-09-27 09:49:44	~32 min	`tests`	📄`log`

✔️	`d6d810e`	#2	2024-09-27 09:22:18	~1 min	`tests-rpc`	📄`log`
✔️	`d6d810e`	#2	2024-09-27 09:23:12	~2 min	`linux`	📦`zip`
✔️	`d6d810e`	#2	2024-09-27 09:23:25	~1 min	`android`	📦`aar`
✔️	`d6d810e`	#2	2024-09-27 09:25:57	~3 min	`ios`	📦`zip`
✖️	`d6d810e`	#2	2024-09-27 10:22:46	~32 min	`tests`	📄`log`

✔️	`99e9780`	#3	2024-09-27 10:33:42	~1 min	`android`	📦`aar`
✔️	`99e9780`	#3	2024-09-27 10:34:21	~2 min	`linux`	📦`zip`
✔️	`99e9780`	#3	2024-09-27 10:35:18	~3 min	`ios`	📦`zip`
✔️	`99e9780`	#3	2024-09-27 10:35:26	~3 min	`tests-rpc`	📄`log`
✖️	`99e9780`	#3	2024-09-27 11:04:10	~31 min	`tests`	📄`log`
✔️	`99e9780`	#4	2024-09-27 13:56:43	~29 min	`tests`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`b9aaee7`	#4	2024-09-30 16:59:19	~2 min	`android`	📦`aar`
✔️	`b9aaee7`	#4	2024-09-30 16:59:35	~2 min	`linux`	📦`zip`
✖️	`b9aaee7`	#4	2024-09-30 17:00:20	~3 min	`tests-rpc`	📄`log`
✔️	`b9aaee7`	#4	2024-09-30 17:00:45	~3 min	`ios`	📦`zip`
✔️	`b9aaee7`	#5	2024-09-30 17:29:35	~32 min	`tests`	📄`log`
✔️	`b9aaee7`	#5	2024-10-01 07:21:27	~1 min	`tests-rpc`	📄`log`

✔️	`d184587`	#5	2024-10-02 10:05:19	~3 min	`ios`	📦`zip`
✔️	`d184587`	#6	2024-10-02 10:05:30	~3 min	`tests-rpc`	📄`log`
✔️	`d184587`	#5	2024-10-02 10:06:08	~3 min	`linux`	📦`zip`
✔️	`d184587`	#5	2024-10-02 10:06:57	~4 min	`android`	📦`aar`
✔️	`d184587`	#6	2024-10-02 10:34:32	~32 min	`tests`	📄`log`

codecov-commenter · 2024-09-27T10:27:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.10%. Comparing base (2c0178c) to head (d184587).

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5884      +/-   ##
===========================================
+ Coverage    45.47%   46.10%   +0.62%     
===========================================
  Files          891      891              
  Lines       158068   158063       -5     
===========================================
+ Hits         71880    72869     +989     
+ Misses       78086    76836    -1250     
- Partials      8102     8358     +256

Flag	Coverage Δ
functional	`11.85% <0.00%> (?)`
unit	`45.50% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
protocol/messenger_communities.go	`53.38% <100.00%> (-0.44%)`	⬇️

... and 134 files with indirect coverage changes

chaitanyaprem

LGTM

chaitanyaprem · 2024-09-30T05:40:05Z

NOTE: it might break syncing flow on mobile as LastOpenedAt is no longer synced.

what is the use of this in mobile? and why does community needs to be synced in this case.

osmaczko · 2024-09-30T10:29:35Z

NOTE: it might break syncing flow on mobile as LastOpenedAt is no longer synced.

what is the use of this in mobile? and why does community needs to be synced in this case.

I can only guess that it's meant to display communities in the same order on each device, but I might be wrong. @ilmotta or @qfrank, could you please confirm? Will this cause any issues on mobile, and if so, can we live with that?

qfrank · 2024-09-30T23:54:09Z

NOTE: it might break syncing flow on mobile as LastOpenedAt is no longer synced.

what is the use of this in mobile? and why does community needs to be synced in this case.

I can only guess that it's meant to display communities in the same order on each device, but I might be wrong. @ilmotta or @qfrank, could you please confirm? Will this cause any issues on mobile, and if so, can we live with that?

Sorry @osmaczko , I didn't touch this before, maybe QA or Icaro could give better confirmation

ilmotta

NOTE: it might break syncing flow on mobile as LastOpenedAt is no longer synced.

what is the use of this in mobile? and why does community needs to be synced in this case.

I can only guess that it's meant to display communities in the same order on each device, but I might be wrong. @ilmotta or @qfrank, could you please confirm? Will this cause any issues on mobile, and if so, can we live with that?

The timestamp was originally added to improve the way communities are sorted. The following issue has all the context status-im/status-mobile#17337. But as pointed out in this PR, syncing the timestamp may stop working. The bandwidth cost seems to be much more important to be fixed though.

I would request a Mobile QA to double-check before merging. Maybe @VolodLytvynenko, who originally reported the bug in status-im/status-mobile#17337 can test.

@osmaczko If you feel adventurous:

QAs will need an Android/iOS build to test the status-go PR indirectly. Because only the status-go revision needs to change, the PR (draft works too) can be opened with a single commit, being the result of running scripts/update-status-go.sh fix/stop-syncing-last-opened-at. Builds for Android and iOS will be generated automatically after a few minutes. The label request-manual-qa should be added to such PRs in status-mobile.

chaitanyaprem · 2024-10-01T06:30:01Z

The timestamp was originally added to improve the way communities are sorted

is this to sort communities list for the user to display locally? if so, shouldn't this info be stored in local storage? is there a need to sync this at all in first place.

ilmotta · 2024-10-01T06:51:01Z

The timestamp was originally added to improve the way communities are sorted

is this to sort communities list for the user to display locally? if so, shouldn't this info be stored in local storage? is there a need to sync this at all in first place.

I'm not sure @chaitanyaprem, but I thought the timestamp was being synced to support the case for multiple devices to display the communities sorted in the same way by last opened at. Maybe a user with 2 or more devices with many communities sorted in an inconsistent way would find the solution from this PR inconvenient.

If you ask me, I think your proposal for persisting the timestamp only locally would already provide a better UX for most users who only use one device most of the time, and/or which don't have many communities listed. And I would bet most users would prefer to save on bandwidth and live with inconsistent communities sorting across devices.

osmaczko · 2024-10-01T07:16:50Z

@osmaczko If you feel adventurous:

QAs will need an Android/iOS build to test the status-go PR indirectly. Because only the status-go revision needs to change, the PR (draft works too) can be opened with a single commit, being the result of running scripts/update-status-go.sh fix/stop-syncing-last-opened-at. Builds for Android and iOS will be generated automatically after a few minutes. The label request-manual-qa should be added to such PRs in status-mobile.

@ilmotta thanks, I went through this adventure yesterday: status-im/status-mobile#21353. I've added missing label just now though.

Syncing the entire community for potentially frequent actions like `LastOpenedAt` updates is highly inefficient when using Waku as the transport layer, as it can result in significant bandwidth overhead.

pavloburykh · 2024-10-02T10:23:45Z

Hey @osmaczko! Thank you for the PR. I got one question: does it affect Desktop as well or only mobile? Should we create a Desktop PR pointing at this go branch or it will be okay to use master Desktop for testing?

osmaczko · 2024-10-02T10:37:27Z

Hey @osmaczko! Thank you for the PR. I got one question: does it affect Desktop as well or only mobile? Should we create a Desktop PR pointing at this go branch or it will be okay to use master Desktop for testing?

Thanks for asking. It does not affect Desktop, as we are not using LastOpenedAt (yet?). I don't see a rationale to create Desktop PR for this exact case.

pavloburykh · 2024-10-03T13:14:02Z

@osmaczko thanks for the PR and sorry for delay with testing.

I did not find any issues. The only difference compared to behaviour in develop is that after this PR we will not be syncing the sorting of communities in the single Opened tab (see screenshot below). Agree with @ilmotta that the bandwidth cost is much more important to be fixed so from my perspective we can sacrifice syncing sorting of communities in this single tab.

PR is ready for merge.

osmaczko · 2024-10-03T14:10:50Z

@osmaczko thanks for the PR and sorry for delay with testing.

I did not find any issues. The only difference compared to behaviour in develop is that after this PR we will not be syncing the sorting of communities in the single Opened tab (see screenshot below). Agree with @ilmotta that the bandwidth cost is much more important to be fixed so from my perspective we can sacrifice syncing sorting of communities in this single tab.

PR is ready for merge.

Thank you!

osmaczko requested review from a team, qfrank, ilmotta and chaitanyaprem September 27, 2024 09:16

osmaczko force-pushed the fix/stop-syncing-last-opened-at branch from 555a7dd to d6d810e Compare September 27, 2024 09:17

osmaczko changed the title ~~fix(communities)_: stop syncing community on LastOpenedAt update~~ fix(communities)!: stop syncing community on LastOpenedAt update Sep 27, 2024

github-actions bot added the breaking change label Sep 27, 2024

osmaczko force-pushed the fix/stop-syncing-last-opened-at branch from d6d810e to 99e9780 Compare September 27, 2024 10:32

chaitanyaprem approved these changes Sep 30, 2024

View reviewed changes

qfrank approved these changes Sep 30, 2024

View reviewed changes

osmaczko force-pushed the fix/stop-syncing-last-opened-at branch from 99e9780 to b9aaee7 Compare September 30, 2024 16:56

osmaczko mentioned this pull request Sep 30, 2024

e2e: fix(communities)!: stop syncing community on LastOpenedAt update status-im/status-mobile#21353

Closed

ilmotta approved these changes Oct 1, 2024

View reviewed changes

fix(communities)!: stop syncing community on LastOpenedAt update

d184587

Syncing the entire community for potentially frequent actions like `LastOpenedAt` updates is highly inefficient when using Waku as the transport layer, as it can result in significant bandwidth overhead.

pavloburykh force-pushed the fix/stop-syncing-last-opened-at branch from b9aaee7 to d184587 Compare October 2, 2024 10:01

igor-sirotin approved these changes Oct 3, 2024

View reviewed changes

pavloburykh approved these changes Oct 3, 2024

View reviewed changes

osmaczko merged commit 5a0e06f into develop Oct 3, 2024
12 checks passed

osmaczko deleted the fix/stop-syncing-last-opened-at branch October 3, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(communities)!: stop syncing community on `LastOpenedAt` update #5884

fix(communities)!: stop syncing community on `LastOpenedAt` update #5884

osmaczko commented Sep 27, 2024

github-actions bot commented Sep 27, 2024 •

edited by igor-sirotin

Loading

status-im-auto commented Sep 27, 2024 •

edited

Loading

codecov-commenter commented Sep 27, 2024 •

edited

Loading

chaitanyaprem left a comment

chaitanyaprem commented Sep 30, 2024

osmaczko commented Sep 30, 2024

qfrank commented Sep 30, 2024

ilmotta left a comment

chaitanyaprem commented Oct 1, 2024

ilmotta commented Oct 1, 2024

osmaczko commented Oct 1, 2024

pavloburykh commented Oct 2, 2024

osmaczko commented Oct 2, 2024

pavloburykh commented Oct 3, 2024 •

edited

Loading

osmaczko commented Oct 3, 2024

fix(communities)!: stop syncing community on LastOpenedAt update #5884

fix(communities)!: stop syncing community on LastOpenedAt update #5884

Conversation

osmaczko commented Sep 27, 2024

github-actions bot commented Sep 27, 2024 • edited by igor-sirotin Loading

Check-list

status-im-auto commented Sep 27, 2024 • edited Loading

Jenkins Builds

codecov-commenter commented Sep 27, 2024 • edited Loading

Codecov Report

chaitanyaprem left a comment

Choose a reason for hiding this comment

chaitanyaprem commented Sep 30, 2024

osmaczko commented Sep 30, 2024

qfrank commented Sep 30, 2024

ilmotta left a comment

Choose a reason for hiding this comment

chaitanyaprem commented Oct 1, 2024

ilmotta commented Oct 1, 2024

osmaczko commented Oct 1, 2024

pavloburykh commented Oct 2, 2024

osmaczko commented Oct 2, 2024

pavloburykh commented Oct 3, 2024 • edited Loading

osmaczko commented Oct 3, 2024

fix(communities)!: stop syncing community on `LastOpenedAt` update #5884

fix(communities)!: stop syncing community on `LastOpenedAt` update #5884

github-actions bot commented Sep 27, 2024 •

edited by igor-sirotin

Loading

status-im-auto commented Sep 27, 2024 •

edited

Loading

codecov-commenter commented Sep 27, 2024 •

edited

Loading

pavloburykh commented Oct 3, 2024 •

edited

Loading