zooniverse / front-end-monorepo

A rebuild of the front-end for zooniverse.org
https://www.zooniverse.org
Apache License 2.0
104 stars 30 forks source link

Project Stats: Project Stats numbers (Classifications Today / Classifications Total) don't show on initial Next.js page fetch #6190

Closed shaunanoordin closed 1 month ago

shaunanoordin commented 1 month ago

Inconsistent UI Bug?

Package: app-project and/or lib-react-components Confirmed as of commit: Also see: #5974 (previous fix for AnimatedNumbers), #5971 (similar issue) Possibly related Talk issue: https://www.zooniverse.org/talk/17/3402644 ?

In certain cases when the Next.JS server first fetches a page, it's possible for the Project Stats ("Classifications Today" and "Classifications Total" count on the Classify page) to not display accurate numbers, despite fetching the correct data.

Replicating this issue is a faff, but can be done on localhost:

Screenshot: Project Stats show 0 Classifications Today and 0 Classifications Total, but the bar chart indicates otherwise.

image

Extended Testing

Asides from the issue seemingly stemming from the "initial Next.JS page fetch", additional testing shows that the Project Stats otherwise works fine. - Tested with macOS + Chrome 126 on https://www.zooniverse.org/projects/fulsdavid/the-daily-minor-planet/classify/workflow/22354 and [local](https://local.zooniverse.org:3000/projects/fulsdavid/the-daily-minor-planet/classify/workflow/22354?env=production) (app-project, master) - I'm able to load the FEM Classify page (while already logged in) and see the initial Classifications Today and Total counts. While logged in, I can make classifications, and both the Classifications Today and Total counts update as expected. (About 5 classifications per test) - I can log out, and the counts will reset to 0 - I can login to the same account, or to a different account, and the Classifications Today and Total counts update as expected.

Dev Notes

Thoughts 1: I don't have enough expertise on the Next.JS infrastructure to understand why/how the fetch process would be a problem. Is it a hydration thing?

Even if it is, I can't tell if it's anything user-facing (i.e. if it affects the non-dev servers), and how I'd confirm/check that.

Thoughts 2: could it be possible that in certain scenarios, the animation of the <AnimatedNumbers> used in the project stats is interrupted, causing the numbers to "freeze" at 0 (for small counts) or appear to "lag" (for larger counts)?

I'm starting to think if it's worth removing AnimatedNumbers altogether, if it's a potential cause of problems.

Thoughts 3: this may issue or may not be related to the Talk post that initiated this investigation, since that volunteer said that despite refreshing their page, the lag still persisted.

But since then it has started to fall behind the 'classifications today' counter, until it was lagging behind by 55 classifications. I tried refreshing the page, clearing my cache, waiting for an hour and refreshing again, none of which made any difference

But then again:

Logging out of my account and back in again did seem to do something, though - the total classifications counter is now off by just two classifications. Better, but still not quite what I wanted.

Status

Unknown priority. Impact level unclear, but some volunteers are understandably upset when this occurs. Replicability/consistency of the issue is dubious since it relies on some Next.JS infrastructure shenanigans that I can only consistently replicate on my localhost.

UPDATE (1 Aug): now set to low priority.

eatyourgreens commented 1 month ago

If it helps, classification stats aren’t fetched server-side. They’re fetched in the browser, after you’ve been logged in.

shaunanoordin commented 1 month ago

Update

I'm filing this investigation under low priority for now, as the original volunteer has informed us that they couldn't replicate the problem at the moment. I'll still keep this issue open until we know for sure what's been happening.

Also, of interest, the volunteer also added something else that may either be related to this issue, or is tangential to it:

PS: I have come across the inconsistent UI problem as well, for what it's worth, although for me it usually manifests slightly differently: when I start a fresh session after booting my computer, both stats counters will initially be at 0, with the bar chart for the preceding days also incorrectly at 0. The problem mostly goes away after doing a few classifications and refreshing the page, although occasionally I need to open the classify page in a new tab for the bar chart to be displayed correctly. I have always assumed that this is connected to the fact that I normally stay logged into my zooniverse account.

eatyourgreens commented 1 month ago

It might also be worth noting that user project classification counts are cached in session storage, to prevent them being lost when you go to Talk.

https://github.com/zooniverse/front-end-monorepo/issues/5134#issuecomment-1693125389

eatyourgreens commented 1 month ago

@shaunanoordin I can reproduce this bug on the Zooniverse site:

  1. Open a Classify page in a tab and do some classifications: https://www.zooniverse.org/projects/fulsdavid/the-daily-minor-planet/classify/workflow/22354

    Classification statistics for Daily Minor Planet: 4 today and 50 total.
  2. Open the same page in a new tab. Today's total is correct but total project classifications are now 0:

    Classification statistics for Daily Minor Planet: 4 today and 0 total.

The project user snapshot, which includes personal stats, is saved to session storage, to preserve it when you visit Talk, or your inbox, or recents, or collections. Session storage isn't shared across tabs. You could maybe change that to use local storage, which is shared across tabs. I'm not sure what the other side effects of that might be. For example, if you have two tabs, each on a different project, then the user snapshots for each of those tabs will be different and you wouldn't want the snapshot from project A to be loaded into the Classify page for project B (see #5134 and #5964.)

eatyourgreens commented 1 month ago

I’m not 100% sure this is relevant, but each tab has its own, independent Panoptes session. See #4892 and https://github.com/zooniverse/panoptes-javascript-client/issues/207.

So, in theory, the new tab should log you in, with your secure Panoptes cookie, then request your classification count from the API. I think there might be a bug in the code that makes that request, which is in the user store. TBH, making network requests from a MobX store isn’t a recommended pattern so that code probably needs to be replaced.

EDIT: after looking at the AnimatedNumber component, I realised the bug was actually missing dependencies for a useEffect hook, plus an animation function that will animate even if the initial and final values are both 0. Data-fetching seems to be working as expected. However, if data-fetching from the stats API fails, I don't think that error is communicated to a volunteer. The bug has nothing to do with Next.js.

The next few comments are me trying to figure out where AnimatedNumber is failing. You can skip forward to this comment, https://github.com/zooniverse/front-end-monorepo/issues/6190#issuecomment-2266235889, if you want to know how to reliably reproduce the bug.

Skip forward to this comment, https://github.com/zooniverse/front-end-monorepo/issues/6190#issuecomment-2267146691, if you want to get straight to an explanation of the bug. It's to do with closures and stale variable scopes being captured.

eatyourgreens commented 1 month ago

Here's the action that fetches your personal stats, when the React app mounts in the browser. If newUser is false, then daily counts should have been loaded from a user snapshot in browser session storage.

https://github.com/zooniverse/front-end-monorepo/blob/094daf5997cea823dc647b78af31808b9f8a75a9/packages/app-project/stores/User/UserPersonalization/UserPersonalization.js#L75-L83

eatyourgreens commented 1 month ago

I just refeshed my browser tab from one hour ago. Project stats have changed, but today's 4 classifications are missing from the overall total. The correct numbers should be; 4 classifications today, 50 classifications overall.

I think what's going here is the bug that I described in https://github.com/zooniverse/front-end-monorepo/issues/5134#issuecomment-1693125389, where session storage loads stale classification counts. The 4 classifications that I made today were all made in a different tab, so they haven't been added to the total count cached in browser storage in this tab.

If mutabilitie is using long-running sessions in a single tab, refreshing that tab to log back in, then a combination of #5134 and https://github.com/zooniverse/panoptes-javascript-client/issues/207 could explain why she's seeing stale classification counts.

Daily Minor Planet statistics: 4 classifications today, 46 classifications overall.
eatyourgreens commented 1 month ago

I added console logs for the counts prop, and ran a production build of Daily Minor Planet on local.zooniverse.org. The counts prop updates as expected, so counts are being fetched properly from the API, but the YourStats component doesn't reflect changes to counts.total. counts.today does update when it changes.

The stats component for Daily Minor Planet, with the browser dev console open. The stats component says 5 classifications today, 0 total. The console log says 5 classifications today, 50 total.
eatyourgreens commented 1 month ago

Logging the props for the Stat component shows that the displayed value doesn't match the prop either.

The stats component, showing 0 classifications today, with a dev console log that shows the Stat component value is 5.
eatyourgreens commented 1 month ago

The AnimatedNumber component is being passed a value of 51, but showing a value of 0. That's the same issue I saw in #5971. I find that the bug triggers if I load my classification counts offscreen, so the bug might be in the intersection observer that animates the number.

https://github.com/user-attachments/assets/6f4bcb2d-26eb-43e9-b4f7-30bdbaa2712c

https://github.com/user-attachments/assets/939436b8-8454-4c50-b933-a2f0c5872838

eatyourgreens commented 1 month ago

If I load my stats offscreen, animated is always false in the AnimatedNumber component, when the value first changes from 0 to 5.

Console logs, including the internal state of the AnimatedNumber component, when stats load offscreen.

When animated is false, AnimatedNumber shows 0, so maybe there's a bug here? Should initialValue here store the previous value of the animated number, rather than 0? https://github.com/zooniverse/front-end-monorepo/blob/094daf5997cea823dc647b78af31808b9f8a75a9/packages/lib-react-components/src/AnimatedNumber/AnimatedNumber.js#L80

eatyourgreens commented 1 month ago

NB. that code also means that project stats are always rendered as 0 during SSR, even though they've been set. Here are the Node console logs from that same page, running locally.

animated is always false in Node, so total Classifications, Volunteers, and Completed Subjects will all be rendered as 0.

counts: { today: 0, total: 0 }
Stat: test-classify-your-stats-today-count Classifications today 0
{ animated: false, value: 0 }
Stat: test-classify-your-stats-total-count Classifications total 0
{ animated: false, value: 0 }
Stat: undefined Volunteers 6190
{ animated: false, value: 6190 }
Stat: undefined Classifications 810216
{ animated: false, value: 810216 }
Stat: undefined Subjects 71804
{ animated: false, value: 71804 }
Stat: undefined Completed subjects 28715
{ animated: false, value: 28715 }
eatyourgreens commented 1 month ago

Now that I know how to reliably reproduce this bug, here's a screenshot showing the AnimatedNumber component in production, with the value prop highlighted in the React component inspector. value is 51, but the displayed value is 0, which is pretty much the same issue as #5971.

To reproduce this bug:

  1. Use a new tab, so that user preferences (including activity_count) aren't already cached in session storage and must be fetched from Panoptes. Alternatively, open session storage and delete the stored user, if present.
  2. Make sure the YourStats component is offscreen, so that its displayed value is still 0 after stats have loaded from the API.
  3. Load the Classify page for a project that you've classified on today.
  4. Wait for user stats to load offscreen.
  5. Scroll down to your stats. One will animate to its loaded value, but I find that one will stay stuck at 0 most of the time. Sometimes today's count is stuck at 0, sometimes the total count is stuck at 0.
  6. Inspect AnimatedNumber in the React component inspector to see that its value prop doesn’t match the displayed number.

Those steps reliably reproduce the bug for me in desktop Firefox. Mobile Safari seems to be unaffected.

https://www.zooniverse.org/projects/fulsdavid/the-daily-minor-planet/classify/workflow/22354

Daily Minor Planet in Firefox, with the React component inspector open to show that the value prop for AnimatedNumber doesn't match the displayed value.
eatyourgreens commented 1 month ago

I think I see a bug. The effect hook here captures the animateValue closure when value is 0. It doesn’t update when animateValue changes, so I think it’s always going to animate to 0, even though a new animateValue closure is generated on every render. I think the effect hook executes a stale reference to an older value of animateValue, from a previous render.

The linter should really be warning about missing useEffect dependencies here. I’d also be careful about generating callback functions on every render, like you’re doing here, since it’s really easy to accidentally capture a stale variable scope inside a closure. If possible, move your callbacks outside the render function, or declare them inside the useEffect hook.

https://github.com/zooniverse/front-end-monorepo/blob/094daf5997cea823dc647b78af31808b9f8a75a9/packages/lib-react-components/src/AnimatedNumber/AnimatedNumber.js#L65

eatyourgreens commented 1 month ago

I've opened #6192 to fix the useEffect hook, so that the animated value always uses the latest version of the value prop.

I’ve also added a check so that it won’t animate from 0 to 0, then set animated to true, while the value is still loading from an external API.

eatyourgreens commented 1 month ago

@shaunanoordin this isn't particularly exhaustive testing, but I'm seeing stats show up correctly in dev mode, with the changes in #6192. Screen recording from Firefox:

https://github.com/user-attachments/assets/bb1ddc1d-a850-43fc-8b19-78ac8808fa2a

eatyourgreens commented 1 month ago

Here’s an explanation of the bug.

  1. When the AnimatedNumber component mounts, value is 0 and animated is false. useEffect runs and creates an intersection observer which will animate the displayed number from 0 to 0.
  2. value changes to your actual count and a new animateValue function is created, which will animate the span to that value, but useEffect doesn’t run, so the intersection observer doesn’t update to use the new animateValue. It has captured the value of value when it was created, which is 0.
  3. When you scroll the number into view, it runs an outdated animateValue from the first render, animates the displayed number to 0, then sets animated to true.

So the component will always show 0, if it loads offscreen and value loads asynchronously. There’s no story or test for the case where value is set asynchronously but the code is written to display the value of value that’s captured when the component mounts.

For a more technical explanation, I recommend MDN’s explanation of lexical scoping in closures. Each time the useEffect hook runs, it's capturing animateValue and lessAnimation from the current render. They, in turn, are capturing the current values of value and duration. TBH, the easiest way to avoid bugs like this is to avoid nested closures. Also, set up ESLint to warn when it detects nested closures, and look out for nested closures as a code smell in PR reviews.

eatyourgreens commented 1 month ago

Here's a story that reproduces the bug. On the main branch, this story always shows 0. I've included it in #6192 so that you can reproduce the bug and check the fix.

I think the bug was introduced in https://github.com/zooniverse/front-end-monorepo/pull/5974, or possibly https://github.com/zooniverse/front-end-monorepo/pull/5947. Neither of those PRs included value or duration in the dependencies for useEffect, so the effect hook only captured value on component mount.

export const DeferredAnimation = () => {
  const [value, setValue] = useState(0)
  setTimeout(() => setValue(700000000), 2000)
  return (
    <Box pad={{ vertical: '120vh' }}>
      // this will always display 0, even if value changes.
      <AnimatedNumber duration={4000} value={value} />
    </Box>
  )
}

production-release:

https://github.com/user-attachments/assets/100439eb-34f7-4437-aab5-2aaa88b21457

PR #6192:

https://github.com/user-attachments/assets/4824f070-b8f5-49c9-829e-cce8d41f431a

eatyourgreens commented 1 month ago

@shaunanoordin this might not be a low priority bug after all, given that volunteer classification counts are always displayed as 0 when they load from Zooniverse APIs. They display fine if they load from session storage, which is why refreshing the page is a workaround.

eatyourgreens commented 1 month ago

For anyone who's made it this far, congratulations, and the title of this issue is wrong. The bug has nothing to to do with Next.js.

eatyourgreens commented 1 month ago

I've posted a link to Talk that reproduces this bug every time you click it: https://www.zooniverse.org/talk/17/3402644?comment=5592887

eatyourgreens commented 1 month ago

Here's a quick screen recording in Chrome, where I click that link three times and get the wrong counts each time. Weirdly, the daily stats sometimes show up, sometimes show as 0. Total counts are always 0.

https://github.com/user-attachments/assets/65875a9e-7f50-4609-a368-6fb2cf0a797d