Skip to content

Commit

Permalink
Initialize route state in store before fetching groups
Browse files Browse the repository at this point in the history
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 hypothesis/support#79
  • Loading branch information
robertknight committed Oct 15, 2024
1 parent 6a6e3ce commit 25358a0
Showing 1 changed file with 19 additions and 9 deletions.
28 changes: 19 additions & 9 deletions src/sidebar/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,26 +64,31 @@ if (configFromSidebar.sentry && envOk) {
disableOpenerForExternalLinks(document.body);

/**
* Configure the Hypothesis API client.
*
* @inject
*/
function setupApi(api: APIService, streamer: StreamerService) {
api.setClientId(streamer.clientId);
}

/**
* 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();
}

/**
Expand Down Expand Up @@ -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);

Expand Down

0 comments on commit 25358a0

Please sign in to comment.