lib-user: Refactor usePanoptesProjects hook with auth, update usage accordingly #6140
Conversation
…anoptesProjects changes
There was a problem hiding this comment.
PR Review
Package: lib-user
This PR fixes an issue where, on the logged-in user home page, the "Recents Classification" feature would crash the whole page, IF the user recently classified on a non-public project.
- Analysis: issue appears to be triggered by
usePanoptesProjects()not returning non-public projects. Problem is solved by passing in anauthorizationvalue to the fetch function, so non-public projects that the user owns (and/or has contributed to?) is fetched. - Modified functions:
usePanoptesProjects()now assumes/requires(?) that user is logged in.
- Modified components:
- Major: RecentProjectsContainer and RecentSubjectsContainer have their data fetches rewritten.
- Other components have more minor modifications to the way they call usePanoptesProjects()
While the code changes overall look good, and the bug being fixed is properly fixed, I have a bunch of "gotcha" questions. I'm not concerned enough to block this PR (if it ain't user-facing yet, I'm fine with WIPs), but I'm still making a concerned "hmmm" face with furrowed eyebrows. 🤨
For additional details, please see the summary/Status section below, or see the PR notes associated with this review. (What are they called? Single comments? Inline PR notes?)
Testing
Tested on app-root on localhost with macOS + Chrome 125
Testing Steps:
- As a test user (darkeshard), create a classification on a random non-public test project (example)
- Load app-root
- As the same test user, view the (signed-in) home page
- The page should NOT crash ✔️
- TO CHECK: how should the non-public projects & non-public classifications be displayed instead? Should they be visible, or hidden?
- compare & contrast how non-public projects and non-public classifications are supposed to look. (On master, before crashing, the Recent Projects showed an 'undefined' project & Recent Subjects is perma-loading. On this branch, the non-public project and non-public classifications are simply unlisted, and I'm not 100% sure that's expected.)
- impact of modified
usePanoptesProjects()on other components, e.g. user stats page. (Looks fine at a quick glance) - ❓ functionality of modified
usePanoptesProjects()if user isn't logged in. (I think all the components I've seen that use this hook are located in "logged-in only" pages. Maybe the Contributors component could be double checked, but I'm not sure how to access that page normally.)
Status
I'm giving this my seal of approval, though I would caution a few things:
- [moderate concern] RecentProjectsContainer and RecentSubjectsContainer are (probably) modifying their input values, instead of making copies of their input values then modifying and returning them.
- [high concern] if a user isn't logged in, usePanoptesProjects()'s fetchProjects() will send a weird
Authorization: 'Bearer (blank)'parameter, instead of sending noAuthorizationparameter at all. - [unknown concern level?] even if the root problem comes from the data-fetching function, our components shouldn't hard-fail the entire page if the input isn't expected.
As for item 3, I'm actually interested enough in this bug that I will, after approving this review, dig deeper into where the fatal error is occurring, as it's indicating to me that there's a component or function that's not robust enough to graciously reject unexpected/missing input.
| if (matchedProjectObj) { | ||
| preference.project = matchedProjectObj | ||
| } | ||
| return preference | ||
| }) |
There was a problem hiding this comment.
I think preference.project = whatever will actually modify the original object(s) in the projectPreferences[] array (aka the data fetched via fetchUserProjectPreferences()).
In practice, this is fine because I don't see anything else using projectPreferences, nor for that matter the response.body.project_preferences data being pulled in RecentProjectsContainer.fetchUserProjectPreferences().
Recommendation: make a copy of the object you want to modify and return. e.g.:
let copyOfPreference = { ...preference } // Assumes preference is never undefined/null
if (matchedProjectObj) {
copyOfPreference.project = matchedProjectObj
}
return copyOfPreference
| if (isBrowser) { | ||
| auth.checkCurrent() | ||
| } |
There was a problem hiding this comment.
[curiosity] This is the first time I'm seeing this pattern - auth.checkCurrent() is placed in the root function? Is this synchronous and will this block execution of the rest of the code, until auth current is checked? Or it is running asynchronously as the rest of the page executes?
There was a problem hiding this comment.
This code is asynchronous and won't block other code execution. It's similar to what is implemented in lib-react-components' useUnreadMessages and useUnreadNotifications hooks.
| page, | ||
| ...query | ||
| }, | ||
| authorization |
There was a problem hiding this comment.
[observation] Ah, so what was missing previously was the Authorization, which meant non-public projects wouldn't be fetched. Gotcha.
There was a problem hiding this comment.
usePanoptesProjects() is meant to only work when a user is logged in?
Because if there's no user, the token var above will be blank or undefined, which means that panoptesProjects.get() will be getting a parameter that looks like { authorization: 'Bearer )' } or { authorization: 'Bearer undefined)' }
If usePanoptesProjects() is meant to be used with logged-in users only, we need to add some documentation to alert people of the gotcha. Maybe add in a if (!token) throw new Error('usePanoptesProjects() requires a logged-in user')
If usePanoptesProjects() is meant to be used regardless of logged-in state, then perhaps something needs to be changed. Suggestion: const authorization = (token) ? Bearer ${token} : undefined
There was a problem hiding this comment.
Refactored as noted - const authorization = (token) ? Bearer ${token} : undefined
| if (matchedProjectObj) { | ||
| recent.projectSlug = matchedProjectObj.slug | ||
| } | ||
| return recent |
There was a problem hiding this comment.
projectPreferences[], I think this recent.projectSlug = whatever might actually be modifying the original project(s) data.
I'm now curious to see if any other component that uses usePanoptesProject()'s data value is also suddenly seeing data[x].projectSlug 🤔
Similar suggestion applies: create a copy of the project before modifying and returning it.
const copiedRecent = { ...recent } // Assumes recent isn't null or undefined
if (matchedProjectObj) {
copiedRecent.projectSlug = matchedProjectObj.slug
}
return copiedRecent
|
Thanks @shaunanoordin ! These components were recently added or refactored in #6125 , and I think there are plans to revisit for tests and extracting hooks.
|
|
|
||
| // Attach project slug to each recent | ||
| let recentsWithProjectSlugs | ||
| if (projects?.length) { | ||
| recentsWithProjectSlugs = recents.map(recent => { | ||
| const { slug } = projects.find(project => recent.links.project === project.id) |
There was a problem hiding this comment.
@shaunanoordin - this was the cause of the page crash. The recents included a project that was not included in the usePanoptesProjects projects, and when this line attempted to get the slug from nothing it caused the crash.
| } | ||
| return recent | ||
| }) | ||
| .filter(recent => recent?.projectSlug) |
There was a problem hiding this comment.
This filter will help prevent similar crashes.
| } | ||
| return preference | ||
| }) | ||
| .filter(preference => preference?.project?.slug) |
There was a problem hiding this comment.
This filter will help prevent similar crashes.
|
Excellent, thanks for addressing those concerns, Mark! 👍 |
Package
Linked Issue and/or Talk Post
usePanotpesProjectshook would return a list of projects that would not include the private or development projectDescribe your changes
usePanoptesProjectsso private or development projects will be returnedusePanoptesProjectsHow to Review
Helpful explanations that will make your reviewer happy:
yarn buildandyarn startChecklist
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
yarn panic && yarn bootstrapordocker-compose up --buildand FEM works as expectedBug Fix