Page MenuHomePhabricator
Differential D14416

[native] Avoid running the handler multiple times
ClosedPublic
Actions

Authored by tomek on Feb 27 2025, 6:17 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 4, 4:15 PM
Unknown Object (File)
Fri, Apr 4, 10:45 AM
Unknown Object (File)
Fri, Apr 4, 1:21 AM
Unknown Object (File)
Thu, Apr 3, 4:49 AM
Unknown Object (File)
Wed, Apr 2, 9:04 PM
Unknown Object (File)
Wed, Apr 2, 2:31 AM
Unknown Object (File)
Tue, Apr 1, 11:37 PM
Unknown Object (File)
Tue, Apr 1, 12:18 AM
View All 48 Files
Subscribers
ashoat

Details

Reviewers
kamil
bartek
Commits
rCOMM672473b38cfc: [native] Avoid running the handler multiple times
Summary

The effect contains some async logic that is run based on a condition that is later modified in the effect. This means that it is possible to run this effect multiple times before the condition is noticed to be true (without this change the logic runs usually 4 times).

https://linear.app/comm/issue/ENG-10271/generating-backups-for-siwe-users-who-used-v1-fallback-doesnt-work

Test Plan

Check if the logic is run just once, by adding some console logs. This code can be run when a SIWE user doesn't have a backup.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek created this revision.Feb 27 2025, 6:17 AM
Herald added a subscriber: ashoat. · View Herald TranscriptFeb 27 2025, 6:17 AM
tomek added a child revision: D14417: [native] Remove background identity login handler.Feb 27 2025, 6:22 AM
Harbormaster completed remote builds in B33642: Diff 47291.Feb 28 2025, 6:00 AM
tomek requested review of this revision.Feb 28 2025, 6:00 AM
tomek updated this revision to Diff 47310.Feb 28 2025, 7:44 AM

Delete unnecessary dependency

tomek updated this revision to Diff 47311.Feb 28 2025, 7:46 AM

Update the ref immediatelly

kamil accepted this revision.Feb 28 2025, 7:46 AM
kamil added inline comments.
native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
41 ↗(On Diff #47291)

I would also call this in the final step of the promise below to avoid waiting for re-render, but this should also do the job

87 ↗(On Diff #47291)

I think this can be removed now?

This revision is now accepted and ready to land.Feb 28 2025, 7:46 AM
Harbormaster failed remote builds in B33657: Diff 47311!Feb 28 2025, 7:58 AM
Harbormaster completed remote builds in B33656: Diff 47310.Feb 28 2025, 8:11 AM
tomek updated this revision to Diff 47312.Feb 28 2025, 8:23 AM

Fix eslint

Harbormaster failed remote builds in B33658: Diff 47312!Feb 28 2025, 8:38 AM
tomek updated this revision to Diff 47313.Feb 28 2025, 8:45 AM

Fix eslint

tomek edited child revisions, added: D14425: [native] Schedule backup generation and upload after saving SIWE secrets; removed: D14417: [native] Remove background identity login handler.Feb 28 2025, 8:55 AM
Harbormaster completed remote builds in B33659: Diff 47313.Feb 28 2025, 9:08 AM
Closed by commit rCOMM672473b38cfc: [native] Avoid running the handler multiple times (authored by tomek). · Explain WhyFeb 28 2025, 9:10 AM
This revision was automatically updated to reflect the committed changes.
tomek added a commit: rCOMM672473b38cfc: [native] Avoid running the handler multiple times.

Revision Contents
Changeset List

PathSize
native/
account/
registration/
missing-registration-data/
17 lines

Diff 47313

View Options

native/account/registration/missing-registration-data/missing-registration-data-handler.react.js

Show First 20 Lines • Show All 147 Lines • ▼ Show 20 Lines `${permission} can be disabled by a block, but threadHasPermission can't ` +
'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!', 'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!',
); );
if (threadInfo.minimallyEncoded) { if (threadInfo.minimallyEncoded) {
return hasPermission(threadInfo.currentUser.permissions, permission); return hasPermission(threadInfo.currentUser.permissions, permission);
} }
return permissionLookup(threadInfo.currentUser.permissions, permission); return permissionLookup(threadInfo.currentUser.permissions, permission);
} }
function useThreadHasPermission( function useThreadsWithPermission(
threadInfo: ?ThreadInfo, threadInfos: $ReadOnlyArray<ThreadInfo>,
permission: ThreadPermission, permission: ThreadPermission,
): boolean { ): $ReadOnlyArray<ThreadInfo> {
const loggedInUserInfo = useLoggedInUserInfo(); const loggedInUserInfo = useLoggedInUserInfo();
const userInfos = useSelector(state => state.userStore.userInfos); const userInfos = useSelector(state => state.userStore.userInfos);
return React.useMemo(() => { return React.useMemo(() => {
if (!threadInfo) { return threadInfos.filter((threadInfo: ThreadInfo) => {
return false;
}
const permissions = threadFrozenDueToBlock( const permissions = threadFrozenDueToBlock(
threadInfo, threadInfo,
loggedInUserInfo?.id, loggedInUserInfo?.id,
userInfos, userInfos,
) )
? filterOutDisabledPermissions(threadInfo.currentUser.permissions) ? filterOutDisabledPermissions(threadInfo.currentUser.permissions)
: threadInfo.currentUser.permissions; : threadInfo.currentUser.permissions;
return hasPermission(permissions, permission); return hasPermission(permissions, permission);
}, [loggedInUserInfo?.id, permission, threadInfo, userInfos]); });
}, [loggedInUserInfo?.id, permission, threadInfos, userInfos]);
}
function useThreadHasPermission(
threadInfo: ?ThreadInfo,
permission: ThreadPermission,
): boolean {
return (
useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
.length === 1
);
ashoatUnsubmitted
Done
Inline Actions
): boolean {
- return (
- useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
- .length === 1
- );
+ const threads = useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission);
+ return threads.length === 1;
}
function viewerIsMember(

Can we extract this to its own line? Similar to how we want await to be visible, I think it's good for hooks to be as well

ashoat: Can we extract this to its own line? Similar to how we want `await` to be visible, I think it's…
} }
function viewerIsMember( function viewerIsMember(
threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo), threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo),
): boolean { ): boolean {
return !!( return !!(
threadInfo && threadInfo &&
threadInfo.currentUser.role !== null && threadInfo.currentUser.role !== null &&
▲ Show 20 Lines • Show All 1,812 Lines • Show Last 20 Lines