Open goplayoutside3 opened 1 year ago
This should have been fixed by #2549.
I’m unable to reproduce this. Tested in mobile Safari on an iPhone.
No problems here in Firefox (MacOS 11.7.8.) The URL updates as expected, and the correct page shows when I reload.
https://github.com/zooniverse/front-end-monorepo/assets/59547/25e15e03-d2dc-4403-846d-0aa8ca9fb9eb
No problems in Edge 114 or Chrome 114 on WIndows 11 either. The subject ID updates in the window location bar, as expected. However, I am seeing the subject occasionally reset to the first page of a set when I refresh Chrome, so it looks like there is a bug somewhere in the initial subject setup.
I couldn't reproduce the refresh bug when I recorded the session, so it's an intermittent bug.
https://github.com/zooniverse/front-end-monorepo/assets/59547/27cbd102-0880-4be8-9b1d-7078a6ce4289
No errors in Sentry for that subject URL either. If any errors were thrown, they were silently swallowed by the NextJS app without logging them.
@goplayoutside3 could this be a bug in the NextJS router?
This bug triggers when the classifier is loaded from a URL that doesn't specify a subject. eg. https://www.zooniverse.org/projects/profdrrogerlouismartinez-davila/deciphering-secrets-unlocking-the-manuscripts-of-medieval-spain/classify/workflow/19276/subject-set/106935
Compare the behaviour of the classifier at that URL with its behaviour at this URL: https://www.zooniverse.org/projects/profdrrogerlouismartinez-davila/deciphering-secrets-unlocking-the-manuscripts-of-medieval-spain/classify/workflow/19276/subject-set/106935/subject/79594594
The first URL will fail the query.subjectID
check here, which is meant to stop this callback from running on projects like PH-TESS and Gravity Spy.
https://github.com/zooniverse/front-end-monorepo/blob/7d19264db4c12b672c8a3520cdc655fbe1abeff6/packages/app-project/src/screens/ClassifyPage/components/ClassifierWrapper/ClassifierWrapper.js#L64-L69
The classifier isn't updating when the onSubjectChange
prop changes.
This bug is an example of why you must declare external dependencies for useEffect
.
The React docs now recommend avoiding useEffect
for many common use cases.
This is broken again.
Go to PRINT https://www.zooniverse.org/projects/printmigrationnetwork/print/classify, pick a subject and note the subject number in the URL. When using the next button or "take me to next available subject" button, the url does not change.
Should #2537 be reopened too?
Might be worth testing whether the router query
bug is fixed by upgrading to Next 14.
EDIT: Upgrading to Next 14.0.4 doesn't fix this. router.query
is populated on first render, in the browser, then empty on subsequent renders.
Note it was decided not to fix this Issue in https://github.com/zooniverse/front-end-monorepo/pull/5776.
Shallowing routing is not supported in Next.js 13, so relying on router.replace
in reaction to the subject changing in the Classifier component is a blocker for the Next/Prev subject feature going forward. We can't use history
, or non-shallow routing due to downsides described here: https://github.com/zooniverse/front-end-monorepo/pull/5776#discussion_r1427216645
At the moment, there's a disconnect between wanting the subject id reflected in the url, controlling the url via NextJS, yet controlling the current subject id in the isolated Classifier component. This Issue can only be fixed by a large refactor of the prioritized subject selection architecture, so will remain open until dedicated funding or effort is allocated to that feature.
NB. this bug also means that the page URL doesn’t update when you press Done, or Done & Talk. That makes it difficult for volunteers to comment on Talk, then return to the Classify page and start the next page in an ordered set (see #2537.)
Going back from Talk will return you to the subject in the Classify page URL, which is the first page you worked on during the current session. Particularly frustrating if you keep the same tab open, to transcribe in, across multiple sessions.
Related Architecture Decision Records:
I previously made a start on rewriting the classifier store architecture, to preserve subject state across page visits, in ADR 42: Record classifier store snapshots. That ADR might provide useful context too.
Shallowing routing is not supported in Next.js 13…
This is incorrect. Shallow routing is supported in Next 13 and 14: Shallow routing in the pages router.
However, it only works for the current page (see the Caveats section.) My code routes to a new subject page, so can't be shallow routed.
Package
app-project lib-classifier
Describe the bug
For projects with sequential subject selection, the subject number is reflected in the url, and a banner appears on the subject image with Prev and Next arrows. When I click on the Prev or Next arrows, the classifier component switches to the prev or next sequential subject, but the url does not change.
To Reproduce
Steps to reproduce the behavior:
Expected behavior
The url should always reflect the subject number for projects such as Deciphering Secrets.
Screenshots
Additional context
Banner component code
Potentially linked to https://github.com/zooniverse/front-end-monorepo/issues/2537
Engaging Crowds: https://github.com/orgs/zooniverse/projects/23
The classifier's
onSubjectChange
callback updates the window location in the NextJS app. https://github.com/zooniverse/front-end-monorepo/blob/7d19264db4c12b672c8a3520cdc655fbe1abeff6/packages/app-project/src/screens/ClassifyPage/components/ClassifierWrapper/ClassifierWrapper.js#L61-L70