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

Homepage: Build avatar and profile banner into user dashboard #6117

Merged
merged 8 commits into from
Jun 7, 2024

Conversation

goplayoutside3
Copy link
Contributor

@goplayoutside3 goplayoutside3 commented Jun 5, 2024

Package

lib-user
app-root

Linked Issue and/or Talk Post

Similar to #6112 this builds an isolated section of the signed-in hompage
Figma: https://www.figma.com/design/qbqbmR3t5XV6eKcpuRj7mG/Group-Stats?node-id=1981-11239&t=FXkBJkbFeX6RQhvA-4

Describe your changes

How to Review

Checklist

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)

@goplayoutside3 goplayoutside3 added the enhancement New feature or request label Jun 5, 2024
@goplayoutside3 goplayoutside3 requested a review from a team June 5, 2024 02:22
@coveralls
Copy link

Coverage Status

coverage: 80.398%. remained the same
when pulling 8204ca8 on lib-user-dashboard
into d89db9a on master.

@goplayoutside3
Copy link
Contributor Author

There's something happening when running app-root locally where a user's avatar and profile banner image aren't consistently displayed 🤔 I'm going to investigate tomorrow before I complete the rest of the Dashboard (draft PR #6121)

@eatyourgreens
Copy link
Contributor

@goplayoutside3 could that be this issue? avatar_src is empty if you get the user details from the encrypted JWT.
zooniverse/panoptes#4217

@coveralls
Copy link

Coverage Status

coverage: 79.323% (+0.02%) from 79.306%
when pulling 0065641 on lib-user-dashboard
into 4deea7e on master.

@coveralls
Copy link

Coverage Status

coverage: 79.289% (-0.02%) from 79.306%
when pulling 8ba217e on lib-user-dashboard
into 4deea7e on master.

@goplayoutside3
Copy link
Contributor Author

@goplayoutside3 could that be this issue? avatar_src is empty if you get the user details from the encrypted JWT.
zooniverse/panoptes#4217

@eatyourgreens that could definitely be the case 🤔 The UserStats component uses similar prop drilling where authUser is passed from app-root to UserStatsContainer > UserStats > MainContent > ProfileHeader to display an avatar. I was just about to say authUser.avatar_src supplies the image src without a problem, but that's not true because UserStatsContainer fetches more data via usePanoptesUser. I'll likely need to do the same for DashboardContainer!

@eatyourgreens
Copy link
Contributor

@goplayoutside3 I thought decrypting the token, since it contains the username, display name etc., would be faster than making API requests to Panoptes to get the authenticated user, but it might not really be worth it if the token doesn't contain the avatar. I might have been better off just making the network request to get the User resource.

@coveralls
Copy link

Coverage Status

coverage: 79.289% (-0.02%) from 79.306%
when pulling 0965ff3 on lib-user-dashboard
into 4deea7e on master.

@goplayoutside3
Copy link
Contributor Author

....it might not really be worth it if the token doesn't contain the avatar. I might have been better off just making the network request to get the User resource.

An interesting point! For now, I refactored DashboardContainer to include a fetch for the user resource of the currently signed-in user, similar to UserStats. I couldn't directly use the existing usePanoptesUser hook from lib-user because of the required include: profile_header in the Dashboard. We might be able to refactor that shared hook in lib-user to take extra query params as needed.

I'll add a card to the Stats Pages & Homepages project board for investigating changes to app-root's authContext, and how it might affect the user prop passed to various lib components.

@coveralls
Copy link

Coverage Status

coverage: 79.281% (-0.03%) from 79.306%
when pulling 0685e44 on lib-user-dashboard
into 4deea7e on master.

Copy link
Contributor

@eatyourgreens eatyourgreens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of notes about subtle bugs in your SWR code.

}

export default function DashboardContainer({ authUser }) {
const key = { authUser }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If authUser is empty or null, you should set the key to null. That tells SWR to skip the request.

Otherwise, your code here might inadvertently run the SWR hook when it shouldn't, and cache an unexpected value for this key in the SWR cache.

When you pass a key to useSWR, it looks up that key in its global cache and immediately returns the stored result (which is why keys must be unique.) If the stored result is stale, it revalidates the cache by fetching a new result from the remote source. Hence 'stale while revalidate.'

Copy link
Contributor

@eatyourgreens eatyourgreens Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a warning that if you've used { authUser } as the key for any other queries, elsewhere in your code, useSWR(key, fetcher, options) will returned the cached values for those queries. That can lead to some weird bugs.

You could avoid that here by passing the query object as the key. I think that's unique to this specific API request. I think that when I was writing SWR hooks for translations, I used the API endpoint and query as the key for each request, but I don't remember the details. That was where I learnt that reusing a key leads to bugs!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DashboardContainer is in UserHome, and UserHome only renders if authUser.login exists. I don't expect authUser to ever be null in this component.

UserHome is conditionally rendered in app-root here. Do you suggest handling the homepage differently?

{user?.login ? <UserHome authUser={user}/> : <DefaultHome />}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you know that authuser will always be defined, then this should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Good catch with the need for a unique key too, so I'll definitely address that here!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that unique key caught me out here, which is why I added the endpoint.

const defaultKey = {
user: storedUser,
endpoint: '/me'
}

Copy link
Contributor

@eatyourgreens eatyourgreens Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this specific API request, but it can also be a good idea to invalidate the cache when an auth token is refreshed, like this example for user project preferences, which includes authorization in the key:

const key = { endpoint, projectID, userID, authorization }

I think there might be some discussion of that in the SWR docs for query keys. The idea is that you don't want to accidentally show stale private data to another user, eg. on a shared browser, so always clear the cache when the auth header expires and is refreshed.

https://swr.vercel.app/docs/arguments#multiple-arguments

Comment on lines 22 to 23
const { body } = await panoptes.get('/users', query, { authorization })
const user = body.users?.[0]
Copy link
Contributor

@eatyourgreens eatyourgreens Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API request runs a search across the Users table, returning all users that match query. If the user ID is undefined, it will return the first 20 users from the Users table.

When you want a single, specific resource by ID, the form of the URL to use is GET /api/users/1234?include=profile_header.

You don't need an Authorization header here either, since users are public resources.

When you add Authorization headers to a request, the browser generally won't cache the response, because auth'ed requests are seen as private and not cacheable, so you might end up generating unnecessary network traffic. That can become an important consideration when you're building a busy page with a lot of users, like the Zooniverse home page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know! Thanks, I'll refactor this fetch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding correctly I think this means we can delete auth (lines 17 and 18), remove user_id from the query, and refactor line 22 to const { body } = await panoptes.get(/users/${authUser.id}, query)?

Copy link
Contributor

@eatyourgreens eatyourgreens Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. A unique cache key for this URL is:

const key = {
  endpoint: `/users/{authUser.id}`,
  query: { include: 'profile_header' }
}

With that key, the client request becomes:

panoptes.get(key.endpoint, key.query)

Alternatively, you could set the key as the full URL:

const key = `/users/{authUser.id}?include=profile_header`

panoptes.get(key)

Copy link
Contributor

@eatyourgreens eatyourgreens Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one other thing. panoptes.get uses an old version of SuperAgent (I think it’s still on version 8? Version 9 came out recently, along with a couple of patches), which means that these requests aren’t using fetch.

The app router in Next.js is built on top of fetch so code like this won’t be able to take advantage of Next’s built-in data-caching. Not a big deal right now, when you’re testing with just a single visitor, but something to have in mind when you roll out and need to run the home page under load.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does Next.js's built-in data-caching play with SWR? This request is using SWR in a client component, which is in contrast to using fetch directly in a page.js in app-root:

async function fetchBlogFeed(url) {
try {
const response = await fetch(url)

(Linking the existing superagent Issue to this comment: #317)

Copy link
Contributor

@eatyourgreens eatyourgreens Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the app router, with Next.js 13.5 and higher, Vercel replace native fetch with their own global fetch that caches the return value.

https://nextjs.org/docs/app/building-your-application/data-fetching/fetching-caching-and-revalidating#fetching-data-on-the-server-with-fetch

Copy link
Contributor

@eatyourgreens eatyourgreens Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SWR can be used with fetch, like react-query, but will work with any function that returns a Promise-like object.

This page has notes on using it with the app router.
https://swr.vercel.app/docs/with-nextjs

Copy link
Contributor

@eatyourgreens eatyourgreens Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s some documentation on data caching with libraries that don’t use fetch, such as panoptes-js.
https://nextjs.org/docs/app/building-your-application/data-fetching/fetching-caching-and-revalidating#fetching-data-on-the-server-with-third-party-libraries

Here’s documentation for the app router data cache, which you’ll need to manage across app instances and deployments, since Zooniverse Next.js is self-hosted on Node.
https://nextjs.org/docs/app/building-your-application/caching#data-cache

I haven’t played around with app router caching, but I believe you can also cache rendered HTML across requests, similar to how the current pages router setup serves pages via a CDN, so that HTML can be served immediately, without waiting for a page build to run.

Copy link
Contributor

@eatyourgreens eatyourgreens Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s a couple of old issues and PRs about slow page build times, for context. Worth bearing in mind for both the new home page and the user stats pages.

@eatyourgreens
Copy link
Contributor

The UserStats component uses similar prop drilling where authUser is passed from app-root to UserStatsContainer > UserStats > MainContent > ProfileHeader to display an avatar

Since SWR caches everything that it fetches, you can avoid prop drilling by calling your custom SWR hook from within ProfileHeader. The second call to useSWR will return the cached user (plus header and avatar) and shouldn't make a request to Panoptes while the cache is fresh.

@mcbouslog mcbouslog self-requested a review June 7, 2024 15:41
@mcbouslog mcbouslog self-assigned this Jun 7, 2024
Copy link
Contributor

@mcbouslog mcbouslog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with a user with an avatar and profile image and with a user without and works as expected, looking good! 👍

Good conversation about SWR hooks, I agree it'd be preferable to use the usePanoptesUser hook, but as noted that hook will need refactoring to work accordingly. I'm sending an array of user IDs for TopContributors and the (full) Contributors components which complicates things, but I think this can all be revisited in subsequent PR.

@goplayoutside3 - do you intend to refactor the SWR key and user fetch in this PR?

Comment on lines 22 to 23
const { body } = await panoptes.get('/users', query, { authorization })
const user = body.users?.[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding correctly I think this means we can delete auth (lines 17 and 18), remove user_id from the query, and refactor line 22 to const { body } = await panoptes.get(/users/${authUser.id}, query)?

@goplayoutside3
Copy link
Contributor Author

@goplayoutside3 - do you intend to refactor the SWR key and user fetch in this PR?

Yes, done 👍

I think this means we can delete auth (lines 17 and 18)

Also done.

subjectID: number.isRequired
subjectID: string.isRequired
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor fix for prop type in SubjectCard

@coveralls
Copy link

Coverage Status

coverage: 79.281% (-0.03%) from 79.306%
when pulling 6359f59 on lib-user-dashboard
into 4deea7e on master.

@github-actions github-actions bot added the approved This PR is approved for merging label Jun 7, 2024
@goplayoutside3 goplayoutside3 merged commit 9a7a630 into master Jun 7, 2024
10 checks passed
@goplayoutside3 goplayoutside3 deleted the lib-user-dashboard branch June 7, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants