Page MenuHomePhabricator
Differential D13464

[lib] Check ID conflicts when processing operations
ClosedPublic
Actions

Authored by tomek on Sep 25 2024, 10:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 9, 3:53 PM
Unknown Object (File)
Sun, Mar 30, 11:57 PM
Unknown Object (File)
Fri, Mar 28, 4:58 PM
Unknown Object (File)
Thu, Mar 20, 9:39 AM
Unknown Object (File)
Mar 3 2025, 2:54 PM
Unknown Object (File)
Mar 3 2025, 6:17 AM
Unknown Object (File)
Mar 3 2025, 6:17 AM
Unknown Object (File)
Mar 3 2025, 6:17 AM
View All Files
Subscribers
ashoat

Details

Reviewers
kamil
will
Commits
rCOMM082447a2d740: [lib] Check ID conflicts when processing operations
Summary

We should check whether an entity with the same ID exists and discard an operation in a case of conflict

https://linear.app/comm/issue/ENG-9190/alternative-solution-update-canbeprocessed-functions

Test Plan

Tested some scenarios:

  1. Repeated text message with the same ID
  2. Creating sidebars with
    1. the same thread ID
    2. the same sidebar source message ID
    3. the same create sidebar message ID
  3. Repeated changing thread settings with the same message ID prefix

Diff Detail

Repository
rCOMM Comm
Branch
improvements2
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek created this revision.Sep 25 2024, 10:13 AM
Herald added a subscriber: ashoat. · View Herald TranscriptSep 25 2024, 10:13 AM
Harbormaster completed remote builds in B31847: Diff 44557.Sep 25 2024, 10:31 AM
tomek requested review of this revision.Sep 25 2024, 10:31 AM
tomek updated this revision to Diff 44585.Sep 26 2024, 12:33 AM

Check message ID conflict for all the operations

tomek updated this revision to Diff 44588.Sep 26 2024, 12:50 AM

Validate change thread settings operation

Harbormaster completed remote builds in B31867: Diff 44585.Sep 26 2024, 12:57 AM
tomek updated this revision to Diff 44589.Sep 26 2024, 12:58 AM

Check create sidebar message conflicts

Harbormaster completed remote builds in B31870: Diff 44588.Sep 26 2024, 1:07 AM
Harbormaster completed remote builds in B31871: Diff 44589.Sep 26 2024, 1:15 AM
tomek edited the test plan for this revision. (Show Details)Sep 26 2024, 2:26 AM
tomek added a child revision: D13483: [lib] Validate DM operations.Sep 26 2024, 3:14 AM
kamil accepted this revision.Sep 26 2024, 7:25 AM
kamil added inline comments.
lib/shared/dm-ops/process-dm-ops.js
150–161 ↗(On Diff #44589)

Do you think we can merge these two and use Promise.all?

This revision is now accepted and ready to land.Sep 26 2024, 7:25 AM
tomek updated this revision to Diff 44621.Sep 27 2024, 3:21 AM

Use Promise.all

Harbormaster completed remote builds in B31896: Diff 44621.Sep 27 2024, 3:38 AM
Closed by commit rCOMM082447a2d740: [lib] Check ID conflicts when processing operations (authored by tomek). · Explain WhySep 27 2024, 3:54 AM
This revision was automatically updated to reflect the committed changes.
tomek added a commit: rCOMM082447a2d740: [lib] Check ID conflicts when processing operations.

Revision Contents
Changeset List

Diff 44557

View Options

lib/shared/dm-ops/create-entry-spec.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
View Options

lib/shared/dm-ops/create-sidebar-spec.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
View Options

lib/shared/dm-ops/create-thread-spec.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
View Options

lib/shared/dm-ops/dm-op-spec.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
View Options

lib/shared/dm-ops/dm-op-utils.js

Loading...
View Options

lib/shared/dm-ops/send-edit-message-spec.js

Loading...
View Options

lib/shared/dm-ops/send-multimedia-message-spec.js

Loading...
View Options

lib/shared/dm-ops/send-reaction-message-spec.js

Loading...
View Options

lib/shared/dm-ops/send-text-message-spec.js

Loading...