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.

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 switch to a private or restricted group.
You should see at least one API call to the groups membership endpoint. Each call will load up to 100 members, and continue doing up to 10 calls to load all the members.

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 (5b4b46c).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6756   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files         270      270           
  Lines       10235    10267   +32     
  Branches     2447     2455    +8     
=======================================
+ Hits        10177    10209   +32     
  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

robertknight · 2025-01-20T10:13:11Z

src/sidebar/services/groups.ts

+
+    if (
+      // For now, group members are going to be used for at-mentions only, so we
+      // can skip loading them if the feature is not enabled


The feature flag is just a temporary measure in development. We plan to ship a version of this where the flag is enabled for everyone. At that point we'll be fetching group members regardless of whether the user edits any annotations in the session.

robertknight · 2025-01-20T10:14:28Z

src/sidebar/services/groups.ts

+   * Fetch members for focused group from the API and load them into the store.
+   */
+  private async _loadFocusedGroupMembers(groupId: string): Promise<void> {
+    // Open groups do not have members, so we can save the API calls


All group types can have members in the data model. There isn't a way for the owner of an open group to invite members to join yet, but the intent is that will be added.

Good. Then I'll simplify it so that the call is always done. If an empty list is returned for whatever reason, so be it.

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 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

Add logic to load focused group members

5b4b46c

acelaya force-pushed the group-members-api branch from 3616880 to 5b4b46c Compare January 20, 2025 16:20

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

robertknight Jan 20, 2025

acelaya Jan 20, 2025

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

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

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

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

acelaya commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

acelaya Jan 20, 2025 •

edited

Loading