Add Panoptes auth to app-root #5459

eatyourgreens · 2023-10-12T15:21:38Z

configure ESLint.
add the Panoptes auth client, from panoptes-client.
add the useSWR hook, from 'swr'.
add usePanoptesUser for SWR-style user fetching.
add an express server to handle HTTPS in local development.
add a page context component to provide global styling, the Grommet theme, and Panoptes auth context.
add the panoptes user to both page header and footer.
fetch your unread message count (useUnreadMessages) and unread notifications count (useUnreadNotifications), once you've signed in.
add an admin toggle to the page footer, and an admin border to the page body.

Please request review from @zooniverse/frontend team or an individual member of that team.

Package

app-root

How to Review

Either yarn dev or yarn start should run a local HTTPS server on https://local.zooniverse.org:3000.

If you have a Panoptes session cookie for local.zooniverse.org:3000, you should be logged in when the page loads, and see your username top right.

Screenshot of the app home page, with a logged-in user top right.

There's no sign-in form yet. If you aren't logged in, you'll need to log in to local.zooniverse.org:3000 in another tab (eg. with the content pages app), in order to set the session cookie.

Admin mode should toggle the top-level admin menu and page border, and persist across page loads.

Once you are logged in, the user object is persisted in local storage for faster sign-in across tabs.

Screen.Recording.2023-10-14.at.12.00.53.mov

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

Tests are passing locally and on Github
Documentation is up to date and changelog has been updated if appropriate
You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

All pages of a FEM project load: Home Page, Classify Page, and About Pages
Can submit a classification
Can sign-in and sign-out
The component is accessible
- Can be used with a screen reader BBC guide to VoiceOver
- Can be used from the keyboard WebAIM guide to keyboard testing
- It is passing accessibility checks in the storybook

New Feature

The PR creator has listed user actions to use when testing the new feature
Unit tests are included for the new feature
A storybook story has been created or updated
- Example of SlideTutorial component with docgen comments, and its story

coveralls · 2023-10-12T15:29:20Z

coverage: 82.227%. remained the same when pulling b90779c on root-auth into 8a48d12 on master.

eatyourgreens · 2023-10-13T14:35:00Z

@mcbouslog might be a good person to review the code for notifications and messages. I copied it more-or-less straight from the project app.

eatyourgreens · 2023-10-13T16:51:05Z

I think this is ready for review now.

packages/app-root/src/hooks/usePanoptesUser.js

eatyourgreens · 2023-10-18T08:39:50Z

Here's a short recording when I have notifications.

Screen.Recording.2023-10-18.at.09.38.28.mov

goplayoutside3

I think this is looking good! I'm able to load https://local.zooniverse.org:3000 and see my username signed-in in the ZooHeader. I added a note to the app-root project board to revisit unit tests for notifications, too.

I have a couple of "big picture" question for understanding:

This setup uses useContext rather than mobx like the older apps in FEM. All app-root components that have useContext will re-render anytime PanoptesAuthContext changes (anything in adminMode, user, etc). I don't think that's a bad thing, but is this the same or different re-rendering behavior than using mobx?
The user fetching functions usePanoptesUser and fetchPanoptesUser look quite a bit different than the same-name functions in app-project. Is the decision to not use swr directly in the hooks a more robust solution than how it's implemented in app-project?

packages/app-root/next.config.mjs

goplayoutside3 · 2023-10-19T20:33:17Z

packages/app-root/server/server.js

@@ -0,0 +1,53 @@
+if (process.env.NEWRELIC_LICENSE_KEY) {


Because this express server is only used for local development, do we need new relic? Just want to think through this server while we have the chance and consider the build bug from app-content-pages too: #5428

The express server runs on yarn start too, but we need to test that on Kubernetes.

Your comment reminds me that the live content pages app isn't logging to New Relic either.

Right! I think that's what I'm getting at. The content pages app will probably never use a custom express server in deployment again, so New Relic should not be setup for app-root either. Thoughts?

How do we debug slow pages and Postgres queries without New Relic?

I mean, content pages should be using New Relic in production too. It should be imported when the Next app loads and starts, at the top of next.config.js.

Does the debugging via New Relic happen via logging from a production deploy, a local build, or both?

🤔 app-project does have New Relic imported into next.config.js but app-content-pages does not.

Any environment that has NEWRELIC_LICENSE_KEY set, according to the code. That’s production and staging deploys.

Looks like they only have examples for the pages router.
https://github.com/newrelic-experimental/newrelic-nextjs-integration

In addition to lack of New Relic + App Router documentation, I strongly suggest that New Relic isn't included in app-root (for now). We don't really know what deployment of this app will look like yet, and I'd prefer to setup logging tools that are used in staging + production envs when we get to that step. I've added it as a to-do in the project board.

Otherwise everything here is looking good!

goplayoutside3 · 2023-10-19T20:33:32Z

packages/app-root/server/server.js

+
+const hostnames = {
+  development: 'local.zooniverse.org',
+  branch: 'fe-project-branch.preview.zooniverse.org',


Suggested change

branch: 'fe-project-branch.preview.zooniverse.org',

This change will crash when APP_ENV is set to 'branch' but I'm not sure if that's a big deal.

Is branch included here in anticipation of doing a branch deploy with app-root?

I think it’s just in all the server setups by default. The content pages app defines it too.

front-end-monorepo/packages/app-content-pages/server/server.js

Line 19 in 6d8d03e

branch: 'fe-project-branch.preview.zooniverse.org',

goplayoutside3 · 2023-10-19T20:46:08Z

packages/app-root/src/hooks/usePanoptesUser.js

+/*
+  Null users crash the ZooHeader component.
+  Set them to undefined for now.
+*/


Suggestion to create a Github Issue for this so we can bug-fix in ZooHeader.

I’m not sure that it’s a bug in the header. The header has a default set for user. However, default values are applied to undefined props in React, not null props, and undefined isn’t an allowed value in JSON.

I think the default here should be null, not an empty object. Probably depends on what we expect user to be when no one is logged in.

front-end-monorepo/packages/lib-react-components/src/ZooHeader/ZooHeader.js

Line 46 in d2720c1

user = {},

With Server Components, it might even be better to pass a promise that resolves to a user.

https://react.dev/reference/react/use#streaming-data-from-server-to-client

Ah okay, I understand the need to set to undefined now - thanks!

eatyourgreens · 2023-10-20T03:45:07Z

A couple of quick answers:

The global store object in the project app (and the classifier) is also injected into components with useContext.
You might find it useful, for your own understanding, to take a look at the usePanoptesUser hook in the classifier too. Off the top of my head, I don't know how it's been implemented in the project app, but I'll take a look when I'm back next week.

eatyourgreens · 2023-10-23T13:24:38Z

I've rewritten usePanoptesuser to use the SWR hook for data fetching, and a couple of effect hooks for local data storage and subscribing to auth change events.

Note that useSWR will always return the same, stale user object until this issue is fixed: zooniverse/panoptes-javascript-client#207

goplayoutside3 · 2023-10-25T23:24:32Z

I initially said let's remove New Relic here, but following #5522, do you think New Relic should be required in next.config.js instead?

In this PR I want to be clear about why New Relic is in server.js.

eatyourgreens · 2023-10-26T04:33:28Z

New Relic should be loaded in before the app starts running, so at the top of server.js for a custom server or at the top of next.config.js otherwise.

If you look at the project app, you should see that newrelic is loaded at the top of both files.

Looking at the original PR (#926), there's a config file that needs to be added too.

eatyourgreens · 2023-10-26T07:35:57Z

#1812 has some useful context for why we use custom servers, including links to the original issues.

goplayoutside3 · 2023-10-26T15:51:37Z

#1812 has some useful context for why we use custom servers, including links to the original issues.

This is super helpful thank you!

eatyourgreens · 2023-10-26T16:15:13Z

I also looked up the latest advice for using New Relic with Next, and it looks like it has changed since we originally set everything up. There's now @newrelic/next middleware that can be run at startup with NODE_OPTIONS.

eatyourgreens · 2023-10-26T16:16:42Z

You're right, though, that this won't be needed until we're running the new user pages on Kubernetes. It will be useful, then, to trace how long page builds take and to tune our JS code on the server.

goplayoutside3

I think this PR is fine to merge, but wanted to check-in if any discussion on New Relic needs resolving? It looks like if merged as is, New Relic is setup in server.js but not used until this app is deployed. At deploy, we'll need to revisit New Relic config if, for example, server.js can't be used in a deployed version due to issues recently seen in app-content-pages.

eatyourgreens · 2023-10-28T06:01:40Z

None of our Next.js apps are using @newrelic/next so I'll follow up next week with a PR to redo New Relic across the whole monorepo.

- add the Panoptes auth client, from `panoptes-client`. - add `usePanoptesUser` for SWR-style user fetching. - add an express server to handle HTTPS in local development. - add the panoptes user to both page header and footer.

- add `swr`. - `useUnreadMessages` fetches your unread message count with `useSWR`. - `useUnreadNotifications` fetchs your unread notifications count with `useSWR`.

- persist the current user in local storage. - check admin mode with `useAdminMode`. - add the admin mode toggle to the footer, and the admin mode border to the page.

Add `PageContextProvider`, responsible for: - global page styles. - providing the Zooniverse Grommet theme. - providing Panoptes auth context (user and admin mode.)

- move `fetchPanoptesUser` into a helper. - add explanatory comments. - restore the missing `avatar_src` when getting the user object from a JWT.

- use `useSWR` to fetch the user object from Panoptes. - persist the returned data in local storage. - reset the current Panoptes session when auth changes.

goplayoutside3

This was somehow missed in review, but this commit "Add named page headers" added header elements to /about/page.js and /projects/page.js without a parent element. These two pages now throw an error during the app-root build process locally.

goplayoutside3 · 2023-10-31T19:00:49Z

These two pages now throw an error during the app-root build process locally.

Fixed in #5551

eatyourgreens requested a review from goplayoutside3 October 12, 2023 15:21

eatyourgreens force-pushed the root-auth branch 6 times, most recently from 00d42df to b8f46f0 Compare October 13, 2023 14:21

eatyourgreens force-pushed the root-auth branch 2 times, most recently from aeee0f3 to c37ea0b Compare October 13, 2023 16:45

eatyourgreens force-pushed the root-auth branch 3 times, most recently from 6bd43bf to feb3d61 Compare October 13, 2023 20:36

eatyourgreens commented Oct 14, 2023

View reviewed changes

packages/app-root/src/hooks/usePanoptesUser.js Show resolved Hide resolved

eatyourgreens commented Oct 15, 2023

View reviewed changes

packages/app-root/src/hooks/usePanoptesUser.js Outdated Show resolved Hide resolved

eatyourgreens mentioned this pull request Oct 15, 2023

Add avatar_src to the Panoptes JWT zooniverse/panoptes#4217

Open

eatyourgreens force-pushed the root-auth branch from 3367e6e to 4d68293 Compare October 16, 2023 09:12

eatyourgreens force-pushed the root-auth branch 2 times, most recently from fad9dbe to 60748c2 Compare October 18, 2023 09:29

goplayoutside3 self-assigned this Oct 18, 2023

eatyourgreens force-pushed the root-auth branch from 60748c2 to 1a94a4e Compare October 19, 2023 09:12

goplayoutside3 reviewed Oct 19, 2023

View reviewed changes

eatyourgreens force-pushed the root-auth branch 2 times, most recently from 76f8e6c to 3bf2629 Compare October 23, 2023 13:22

eatyourgreens force-pushed the root-auth branch from 3bf2629 to b6dacdd Compare October 23, 2023 13:43

eatyourgreens force-pushed the root-auth branch from 2b3291d to 750b884 Compare October 25, 2023 13:06

eatyourgreens mentioned this pull request Oct 26, 2023

app-content-pages: Style AboutHeader and move it to a general layout.js file #5488

Merged

7 tasks

eatyourgreens force-pushed the root-auth branch from 750b884 to 4e32691 Compare October 27, 2023 14:16

goplayoutside3 approved these changes Oct 27, 2023

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Oct 27, 2023

eatyourgreens enabled auto-merge (squash) October 28, 2023 06:03

eatyourgreens added 10 commits October 28, 2023 07:03

add Panoptes auth to app-root

7a7133d

- add the Panoptes auth client, from `panoptes-client`. - add `usePanoptesUser` for SWR-style user fetching. - add an express server to handle HTTPS in local development. - add the panoptes user to both page header and footer.

Use a global user object (experimental)

bb96440

Add unread messages and notifications

afe0f0d

- add `swr`. - `useUnreadMessages` fetches your unread message count with `useSWR`. - `useUnreadNotifications` fetchs your unread notifications count with `useSWR`.

Add admin mode

e6290e3

- persist the current user in local storage. - check admin mode with `useAdminMode`. - add the admin mode toggle to the footer, and the admin mode border to the page.

Global page context

b12b88b

Add `PageContextProvider`, responsible for: - global page styles. - providing the Zooniverse Grommet theme. - providing Panoptes auth context (user and admin mode.)

Clean up usePanoptesUser

9d9f9ff

- move `fetchPanoptesUser` into a helper. - add explanatory comments. - restore the missing `avatar_src` when getting the user object from a JWT.

Configure ESLint

a2cda99

Optimise package imports

e1b6239

Rewrite usePanoptesUser with useSWR

8eefa1c

- use `useSWR` to fetch the user object from Panoptes. - persist the returned data in local storage. - reset the current Panoptes session when auth changes.

Add named page headers

b90779c

eatyourgreens force-pushed the root-auth branch from 4e32691 to b90779c Compare October 28, 2023 06:03

eatyourgreens merged commit 5a9a09f into master Oct 28, 2023
7 checks passed

eatyourgreens deleted the root-auth branch October 28, 2023 06:10

goplayoutside3 reviewed Oct 31, 2023

View reviewed changes

Add Panoptes auth to app-root #5459

Add Panoptes auth to app-root #5459

Conversation

eatyourgreens commented Oct 12, 2023 • edited by goplayoutside3 Loading

Package

How to Review

Checklist

General

General UX

New Feature

coveralls commented Oct 12, 2023 • edited Loading

eatyourgreens commented Oct 13, 2023

eatyourgreens commented Oct 13, 2023

eatyourgreens commented Oct 18, 2023

goplayoutside3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goplayoutside3 Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goplayoutside3 Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens Oct 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens commented Oct 20, 2023 • edited Loading

eatyourgreens commented Oct 23, 2023

goplayoutside3 commented Oct 25, 2023

eatyourgreens commented Oct 26, 2023 • edited Loading

eatyourgreens commented Oct 26, 2023

goplayoutside3 commented Oct 26, 2023

eatyourgreens commented Oct 26, 2023

eatyourgreens commented Oct 26, 2023 • edited Loading

goplayoutside3 left a comment • edited Loading

Choose a reason for hiding this comment

eatyourgreens commented Oct 28, 2023

goplayoutside3 left a comment

Choose a reason for hiding this comment

goplayoutside3 commented Oct 31, 2023

eatyourgreens commented Oct 12, 2023 •

edited by goplayoutside3

Loading

coveralls commented Oct 12, 2023 •

edited

Loading

goplayoutside3 Oct 23, 2023 •

edited

Loading

goplayoutside3 Oct 23, 2023 •

edited

Loading

eatyourgreens Oct 22, 2023 •

edited

Loading

eatyourgreens commented Oct 20, 2023 •

edited

Loading

eatyourgreens commented Oct 26, 2023 •

edited

Loading

eatyourgreens commented Oct 26, 2023 •

edited

Loading

goplayoutside3 left a comment •

edited

Loading