Fix some of the freezes experienced by the admins when the community updates #16384
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrainville
requested review from
a team,
iurimatias,
glitchminer,
igor-sirotin and
caybro
and removed request for
a team
Jenkins BuildsClick to see older builds (77)
|
jrainville
force-pushed
the
fix/slow-admin-tab
branch
from
e55e907 to
9100e1c
Compare
jrainville
force-pushed
the
fix/freeze-on-update
branch
from
f8e458b to
e389f38
Compare
micieslak
reviewed
Sep 25, 2024
Comment on lines
527
to
529
| function matchesAlias(name, filter) { | ||
| return name.split(" ").some(p => p.startsWith(filter)) | ||
| } |
There was a problem hiding this comment.
I wonder why alias needs to be handled in a special way here. If it's not necessary we could optimize it in a similar way as described here: https://github.com/status-im/status-desktop/pull/16363/files#r1774985033
There was a problem hiding this comment.
I tried this and it ended up being way slower actually 🤔
Maybe it's because there are so many properties?
There was a problem hiding this comment.
That's quite surprising 🤔 So let's leave it as it is :)
Bth, looking into code of SFPM I have a temptation to rewrite it from scratch 🤦
| FastExpressionFilter { | ||
| expression: !!model.airdropAddress | ||
| expectedRoles: ["airdropAddress"] | ||
| } |
There was a problem hiding this comment.
If it's about check if airdropAddress is not empty string, ValueFilter could be used:
ValueFilter {
roleName: "airdropAddress"
value: ""
inverted: true
}
jrainville
force-pushed
the
fix/slow-admin-tab
branch
from
9100e1c to
6d8058f
Compare
jrainville
force-pushed
the
fix/freeze-on-update
branch
from
caf63e6 to
c8c7aef
Compare
jrainville
force-pushed
the
fix/slow-admin-tab
branch
from
c11f308 to
6bf5125
Compare
jrainville
force-pushed
the
fix/freeze-on-update
branch
from
c8c7aef to
af6bf72
Compare
jrainville
force-pushed
the
fix/slow-admin-tab
branch
2 times, most recently
from
f3efcdb to
fbd6c7c
Compare
jrainville
force-pushed
the
fix/freeze-on-update
branch
from
af6bf72 to
e3d922e
Compare
|
@igor-sirotin reminder to review please |
igor-sirotin
approved these changes
Sep 27, 2024
|
@glitchminer can you test when you have time. No need for a full regression test. You can check the "what to test" section in the PR |
jrainville
force-pushed
the
fix/freeze-on-update
branch
2 times, most recently
from
82cb9d8 to
377f364
Compare
|
status-go updated to point to develop. |
jrainville
force-pushed
the
fix/freeze-on-update
branch
from
377f364 to
dc533c9
Compare
3 tasks
jrainville
force-pushed
the
fix/freeze-on-update
branch
from
dc533c9 to
1231e7a
Compare
jrainville
force-pushed
the
fix/freeze-on-update
branch
from
1231e7a to
e32bf8d
Compare
|
Rebased. @caybro could you do the testing on this one please. |
Sure; any hints on how to trigger the behavior and how to test? |
I don,t think you'll be able to replicate the freezes, because you're gonna need to be an admin in a community as big as the Status community, but just testing that everything in the Affected Areas listed above still works as expected would be good. For example, making sure you get the airdrop address of new joiners without having to restart the app (check the member list in the airdrop admin panel) |
caybro
approved these changes
Oct 10, 2024
This was referenced Oct 10, 2024
Iterates #16043 When the community gets updated by any means, we reconstruct the section item, which is already bad, but we also re-fetch all tokens and all shared addresses, which in turn re-updates models for no reason. Instead, I make sure to only fetch those on first section build, then, I get the new shared addresses when members join using the request to join response that comes from status-go
jrainville
force-pushed
the
fix/freeze-on-update
branch
from
e32bf8d to
d89fc4e
Compare
|
Rebased. I'll merge once it's green |
|
Force merging as the e2e fail is a test in the onboarding, which I didn't touch at all |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does the PR do
Iterates #16043
When the community gets updated by any means, we reconstruct the section item, which is already bad, but we also re-fetch all tokens and all shared addresses, which in turn re-updates models for no reason.
Instead, I make sure to only fetch those on first section build, then, I get the new shared addresses when members join using the request to join response that comes from status-go
Status-go PR to fix the correct return of the shared addresses: status-im/status-go#5867
A further improvement IMO would be to stop redoing the section when the community updates, but we'll need to be careful, because it might break a lot of places that would normally update automatically.
Also includes a commit that makes the input focus and fixes the order of members in the airdrop popups.
Affected areas
Architecture compliance
My PR is consistent with this document: Architecture guidelines
Impact on end user
The general responsiveness of the app when the community updates for admins should be better (no or less freezes)
How to test
Update the community, make people join, check that you have their airdrop address in the airdrop popup.
Risk
Tick one:
Worst case is that some properties don't get updated until a restart (for example the airdrop addresses of members)