-
Notifications
You must be signed in to change notification settings - Fork 4
Moderation tools review and panel visbility (TS-2907 & TS-2908 & TS-2910) #1666
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
base: moderation-tools-feature-branch
Are you sure you want to change the base?
Moderation tools review and panel visbility (TS-2907 & TS-2908 & TS-2910) #1666
Conversation
- Implement object schema for package listing status - Implement response schema for package listing status
- Implement a function for fetching a package listing's status from API - Update package details fetch function with useSession parameter
- Implement a getter function for dapper for getting listing status - Update getPackageListing details with useSession parameter
Implement two functions responsible for getting public or private package listings from the same endpoint. This is required for users who should be able to see for instance rejected package listings. The function for getting public package listings is to be used in the SSR loader function, and the function for getting private package listings is to be used in the clientLoader function.
- Move management tooling into an own file - Move ReviewPackageForm into an own file - Use revalidate in form to handle state updates - Update state usage in form - Add dynamic fetching of status colors - Add type interfaces - Small refactor to code for readability
- Update clientLoader and loader to use utils - Get public package listing with loader - Get public/private package listing in clientLoader - Get package listing status in clientLoader - Utilize getPackagePermissions in order to view/hide management tooling - Remove shouldRevalidate function - Utilize components split into other files - Simplify component code with less nested Suspense/Await blocks - Remove usage of useMemo for Promises
- Add and fix missing types - Fix styling issues - Fix report form
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
apps/cyberstorm-remix/app/p/components/PackageListing/ManagementTools.tsx
Show resolved
Hide resolved
apps/cyberstorm-remix/app/p/components/PackageListing/ManagementTools.tsx
Show resolved
Hide resolved
Oksamies
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few notes, few change requests and one bigger change of logic, that needs to be at least confirmed, but most likely will need to be reverted.
| PackageListingStatusResponseData, | ||
| } from "../schemas/responseSchemas"; | ||
|
|
||
| const basePath = "api/cyberstorm/listing/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good addition 👍 Move this const to thunderstore-ui\packages\thunderstore-api\src\index.ts so that other files can use it too. No need to add wide adoption currently, we can do that on a separate PR.
| const useSession = true; | ||
| const privateListing = await dapper.getPackageListingDetails( | ||
| communityId, | ||
| namespaceId, | ||
| packageId, | ||
| useSession |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just set the arg as true, people can look at the function input types if they want to know what the true is for.
| if (!startsHydrated.current && isHydrated) { | ||
| return; | ||
| } | ||
|
|
||
| if (!listing) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(!listing | (!startsHydrated.current && isHydrated))
|
|
||
| {ReportPackageButton} | ||
| </div> | ||
| <Suspense |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the use of Suspense/Await pattern makes it so that when the clientLoader fetches data and hydration/rehydration happens, the UI will not have skeletons for the time when data is loading and then the page will change "suddenly".
Not a negative per-se, but we've utilized the Suspense/Await to add more "perceived performance".
SSR doesn't suffer from using Suspense/Await as it'll just SSR the page with the data, as the promises are awaited.
I'll have to see how this looks when I'm locally testing later, but gut feeling is that you'll need to add Suspense/Await usages back to this file.
Wait for my local testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all Suspense/Await are removed, only where listing was used since this is awaited in the loader and clientLoader. Will Suspense/Await work as intended with an awaited object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, Suspense will show the fallback as long as there is a promise that is unresolved, inside of the block and Await can take in either a promise or the result of the same promise.
And while it's true that the listing data is being awaited in the clientLoader, I see no reason to remove the Suspenses where listing is used, as we are not 100% sure yet still if we can't return a promise there in the listing first.
We might even end up doing a "late re-hydration" with new data, but that's TDB.
So Add all the Suspense/Await blocks back and just pass them the listing (even when it's a "resolved value" already). If you want to get by with as little changes as possible; create a promise in the beginning of the layout (like in the SS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To emphasize the scope right now; small, precision changes. So changing up the data fetching logic to fit the needs and then doing some sketchy trick in the layout is quite alright at this time of year.
- Move baseUrl to index.ts for re-usability - Simplify if statement in packageListing.tsx - Remove useSession boolean variable and just pass true to getPackageListingDetails
This pull request implements several permission-based features on the package listing page. Specifically, it covers:
Management Panel Visibility
The management panel is now visible to authorized users based on their permissions (can_manage, can_moderate, etc.).
Package Listing Visibility (Rejected Packages only)
Users with the appropriate permissions can now see rejected package listings that were previously hidden. Unlisted packages not handled in this PR.
Review Package Listing
Authorized users can approve or reject packages through the existing “Review Package” modal.
Refactor
Some components and utils moved into own files.