From 25358a0088dafc48c829054db8050047ee391ab2 Mon Sep 17 00:00:00 2001
From: Robert Knight <robertknight@gmail.com>
Date: Tue, 15 Oct 2024 12:34:32 +0100
Subject: [PATCH] Initialize route state in store before fetching groups

The behavior of `GroupsService.load` varies depending on whether the current
route is the sidebar or not. During sidebar startup, `GroupsService.load` was
called before `RouterService.sync`. As a result the route was `null` during the
initial call to `GroupsService._loadGroupsForUserAndDocument` and hence the
service did not execute the sidebar-specific code path to wait for the document
URI to become known at that point.

During the first `GroupsService._loadGroupsForUserAndDocument` call,
`_setupAutoReload` would set up a watcher that would react to changes in
`store.mainURI()`. This watcher would fire after the main guest frame connects.
Depending on when that happens, requests from the initial `GroupsService.load`
request could still be in-flight. If the second set of groups API calls
completed first, followed by the first set of groups API calls, then the final
set of loaded groups might reflect the _first_ call to `GroupsService.load`,
where the document URI was not used (because the route was unknown).

This commit fixes the issue by re-arranging the sidebar startup sequence to
initialize the router service before other services.

Fixes https://github.com/hypothesis/support/issues/79
---
 src/sidebar/index.tsx | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/src/sidebar/index.tsx b/src/sidebar/index.tsx
index 6dea662bb4e..1b982704c01 100644
--- a/src/sidebar/index.tsx
+++ b/src/sidebar/index.tsx
@@ -64,6 +64,8 @@ if (configFromSidebar.sentry && envOk) {
 disableOpenerForExternalLinks(document.body);
 
 /**
+ * Configure the Hypothesis API client.
+ *
  * @inject
  */
 function setupApi(api: APIService, streamer: StreamerService) {
@@ -71,19 +73,22 @@ function setupApi(api: APIService, streamer: StreamerService) {
 }
 
 /**
- * Perform the initial fetch of groups and user profile and then set the initial
- * route to match the current URL.
+ * Update the route in the store based on the initial URL.
  *
  * @inject
  */
-function setupRoute(
-  groups: GroupsService,
-  session: SessionService,
-  router: RouterService,
-) {
+function syncRoute(router: RouterService) {
+  router.sync();
+}
+
+/**
+ * Perform the initial fetch of groups and user profile.
+ *
+ * @inject
+ */
+function loadGroupsAndProfile(groups: GroupsService, session: SessionService) {
   groups.load();
   session.load();
-  router.sync();
 }
 
 /**
@@ -170,9 +175,14 @@ function startApp(settings: SidebarSettings, appEl: HTMLElement) {
     .register('settings', { value: settings });
 
   // Initialize services.
+  //
+  // We sync the route with the initial URL as the first step, because
+  // initialization of other services may depend on the route (eg. enabling
+  // sidebar-only behavior).
+  container.run(syncRoute);
   container.run(initServices);
   container.run(setupApi);
-  container.run(setupRoute);
+  container.run(loadGroupsAndProfile);
   container.run(startRPCServer);
   container.run(setupFrameSync);