Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize route state in store before fetching groups #6614

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

robertknight
Copy link
Member

@robertknight robertknight commented Oct 15, 2024

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


Testing:

See steps to reproduce in https://github.com/hypothesis/support/issues/79#issuecomment-2413707975. The issue should reproduce on the main branch but not on this branch.

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

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.42%. Comparing base (6a6e3ce) to head (97d4a3e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6614   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files         270      270           
  Lines       10175    10175           
  Branches     2419     2419           
=======================================
  Hits        10117    10117           
  Misses         58       58           

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

@robertknight
Copy link
Member Author

Future instances of this issue could be prevented in a stronger way by using the initial URL to initialize the route state in the store, preventing any consumers from observing a null route. This will require more significant refactoring of how initialization works.

@robertknight robertknight merged commit 25358a0 into main Oct 15, 2024
4 checks passed
@robertknight robertknight deleted the sync-route-earlier branch October 15, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants