Add logic to load focused group members #6756

acelaya · 2025-01-15T15:01:04Z

Implement logic to call members API for focused group, and load all its members in the store.

The loaded members are not used yet. They will be used in a follow-up PR to improve the at-mentions suggestions when focused group is not a public group.

Members loading is deferred until an annotation is created/edited for the first time after a group is focused, assuming they are not needed before that.

In future, if we decide to use group members for some different purpose, we may need to move the loading to a different point in the user journey.

Considerations

The group members API is paginated, so we load members in batches. This has two implications:

We have to set a maximum number of calls, to avoid shooting ourselves in the foot. I randomly choose a maximum amount of 10 pages, with 100 members loaded per page.
I'll try to check some real numbers of groups in production, to adjust this based on that.
Membership calls are currently done sequentially. We first load the first page to see what's the total amount of members, then one call at a time until all are loaded or the limit mentioned above is reached.
We could probably parallelize these calls, loading 3 or 4 pages at a time. I'll explore this option separately, once I have the numbers.

Test steps

Check out this branch
Enable at_mentions feature in http://localhost:5000/admin/features
Open the browser console, in the network tab.
Open the sidebar and try to create/edit an annotation. You should see a request in the network tab, to load members for currently focused group.
Try to create/edit a different annotation. There should be no new requests for currently focused group.
Changed focused group. Steps 4 and 5 should work again as described.

TODO

Abort loading members when focused group changes before finishing previous load.

codecov · 2025-01-15T15:23:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.43%. Comparing base (f2f720d) to head (bca8af3).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6756   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files         270      270           
  Lines       10235    10265   +30     
  Branches     2447     2451    +4     
=======================================
+ Hits        10177    10207   +30     
  Misses         58       58

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

robertknight

I took an initial pass over this. A few notes:

I think we should defer fetching group members until either the point they are needed or shortly before (eg. when beginning to edit). That way we avoid these requests in sessions where no annotation editing happens. I think we could assume that any time an annotation is edited, the user may have need for at-mentions.
Open groups can have members in the data model. We just haven't finished the work within h to enable non-admin users to invite them yet. Even though membership isn't needed to write in these groups, they will still be able to have members who are moderators. In general I think it would be a good idea to minimize the amount of special-casing we do for different group types, as this is liable to get out of date as Hypothesis group functionality evolves.
Make sure we handle the case where the focused group is changed while group members are being fetched

src/sidebar/services/groups.ts

robertknight · 2025-01-20T10:23:30Z

src/sidebar/services/groups.ts

+      this._store.isFeatureEnabled('at_mentions') &&
+      this._store.getFocusedGroupMembers() === null
+    ) {
+      this._loadFocusedGroupMembers(groupId).catch(e => console.error(e));


This starts an async process to fetch focused group members, but this request is not canceled if the focused group is changed while it is in-flight. In general, code for dealing with async requests should assume:

Requests can take an arbitrary amount of time (up to any explicit timeout)

Requests may not finish in the order they are started

Yeah, good point.

robertknight · 2025-01-20T10:25:16Z

src/sidebar/services/groups.ts

+  }
+
+  private async _fetchAllMembers(groupId: string): Promise<GroupMember[]> {
+    // Fetch first page of members, to determine how many more pages there are


There is a race condition here that if group members are added while we are paging through, we will end up with duplicates. Unlikely, but possible in principle.

True. Something similar could happen if a member that's already fetched in a previous page is removed. That could cause a member from a subsequent page to not be fetched since it has moved a position "up" and is now part of a page which is supposedly fetched already.

None of these issues are easy to handle with current API contract though, where pagination is not done via cursor, but good to have in mind.

robertknight · 2025-01-20T10:29:20Z

src/sidebar/store/modules/groups.ts

@@ -18,12 +18,16 @@ export type State = {
  groups: Group[];
  /** ID of currently selected group. */
  focusedGroupId: string | null;
+
+  /** Members of currently selected group */
+  focusedGroupMembers: GroupMember[] | null;


Since only members for the current group are stored, this means we'll be reloading members each time you switch back and forth. That's OK to start with, as long as we ensure that this is cleared and in-flight requests are canceled when switching groups, but in future we might want to cache members from groups you've selected in the current session.

I'd prefer not to do client-side caching for now, as it could lead to things changing in the server and having to find a way to invalidate the cache remotely, or risk having outdated lists of users until you reload.

For annotations we do not do caching, for example. Every time the group changes we fetch all annotations for that group. Although it's true that group members will usually change less frequently than annotations, and annotations are loaded in one go.

That's fine, we can skip caching for an initial implementation.

acelaya · 2025-01-20T12:43:41Z

I took an initial pass over this. A few notes:

I think we should defer fetching group members until either the point they are needed or shortly before (eg. when beginning to edit). That way we avoid these requests in sessions where no annotation editing happens. I think we could assume that any time an annotation is edited, the user may have need for at-mentions.

Open groups can have members in the data model. We just haven't finished the work within h to enable non-admin users to invite them yet. Even though membership isn't needed to write in these groups, they will still be able to have members who are moderators. In general I think it would be a good idea to minimize the amount of special-casing we do for different group types, as this is liable to get out of date as Hypothesis group functionality evolves.

Make sure we handle the case where the focused group is changed while group members are being fetched

Thanks! I'll do the changes.

acelaya · 2025-01-21T09:58:51Z

src/sidebar/components/Annotation/AnnotationEditor.tsx

+    if (mentionsEnabled && focusedGroupMembers === null) {
+      groupsService.loadFocusedGroupMembers();
+    }
+  }, [focusedGroupMembers, groupsService, mentionsEnabled]);


I'd prefer not to add this as a side effect, but this component is loaded both when creating a new annotation or editing an existing one, so it's convenient to centralize the logic here.

acelaya · 2025-01-21T10:01:44Z

src/sidebar/services/groups.ts

+    const groupId = this._store.focusedGroupId();
+    if (!groupId) {
+      return; // TODO Throw error?
+    }


The store's loadFocusedGroupMembers method already checks that a group is focused before its members are set, and throws an error otherwise, so I thought maybe we don't need to throw a similar error in two different places.

On the other hand, if one should silently skip, maybe it should be the store and not the service, and an error should be thrown here.

I think the choice comes down to whether it is more useful for consumers if you allow them to call this when there might be no focused group, or whether such an action probably indicates an error on the caller's part that needs to be reported. Currently this should only be called in a context where there is a focused group.

At the moment it isn't clear to me that one approach is significantly better than the other.

src/sidebar/services/groups.ts

robertknight · 2025-01-21T11:20:41Z

src/sidebar/services/groups.ts

+    const groupId = this._store.focusedGroupId();
+    if (!groupId) {
+      return; // TODO Throw error?
+    }


I think the choice comes down to whether it is more useful for consumers if you allow them to call this when there might be no focused group, or whether such an action probably indicates an error on the caller's part that needs to be reported. Currently this should only be called in a context where there is a focused group.

At the moment it isn't clear to me that one approach is significantly better than the other.

robertknight · 2025-01-21T11:23:55Z

src/sidebar/services/groups.ts

+    }
+
+    const members = await this._fetchAllMembers(groupId);
+    this._store.loadFocusedGroupMembers(members);


This code still has the issue that the focused group could change in between the call to loadFocusedGroupMembers and store.loadFocusedGroupMembers being called. At a minimum you need to ensure that members loaded for group A don't get saved as the members of group B if the focused group changes from A to B while fetching. Ideally you also want to stop fetching any further pages of members for group A.

Yeah, sorry. This is not ready for review yet. I haven't finished applying all the changes from your previous review, but I forgot to move it to draft.

src/sidebar/services/groups.ts

src/sidebar/services/test/groups-test.js

src/types/api.ts

acelaya force-pushed the group-members-api branch from fe38a72 to 1054a69 Compare January 15, 2025 15:02

acelaya force-pushed the group-members-api branch 13 times, most recently from fc1e41b to 4bac1dc Compare January 17, 2025 10:47

acelaya requested a review from robertknight January 17, 2025 10:47

acelaya marked this pull request as ready for review January 17, 2025 10:47

robertknight reviewed Jan 20, 2025

View reviewed changes

acelaya force-pushed the group-members-api branch from 4bac1dc to 3616880 Compare January 20, 2025 12:21

acelaya force-pushed the group-members-api branch 5 times, most recently from 92e9994 to 8447360 Compare January 21, 2025 09:53

acelaya commented Jan 21, 2025

View reviewed changes

robertknight reviewed Jan 21, 2025

View reviewed changes

acelaya marked this pull request as draft January 21, 2025 12:11

acelaya force-pushed the group-members-api branch from 8447360 to badea8d Compare January 22, 2025 11:07

acelaya force-pushed the group-members-api branch 2 times, most recently from acc2eb8 to ebf1b56 Compare January 22, 2025 11:52

Add logic to load focused group members

bca8af3

acelaya force-pushed the group-members-api branch from ebf1b56 to bca8af3 Compare January 22, 2025 12:25

acelaya mentioned this pull request Jan 22, 2025

Allow an AbortSignal to be passed to api calls #6770

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logic to load focused group members #6756

Add logic to load focused group members #6756

acelaya commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

robertknight left a comment

robertknight Jan 20, 2025

acelaya Jan 20, 2025

robertknight Jan 20, 2025

acelaya Jan 20, 2025 •

edited

Loading

robertknight Jan 20, 2025

acelaya Jan 20, 2025

robertknight Jan 20, 2025

acelaya commented Jan 20, 2025

acelaya Jan 21, 2025

acelaya Jan 21, 2025

robertknight Jan 21, 2025

robertknight Jan 21, 2025

robertknight Jan 21, 2025

acelaya Jan 21, 2025

Add logic to load focused group members #6756

Are you sure you want to change the base?

Add logic to load focused group members #6756

Conversation

acelaya commented Jan 15, 2025 • edited Loading

Considerations

Test steps

TODO

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

acelaya Jan 20, 2025 •

edited

Loading