Open
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
- Move baseUrl to index.ts for re-usability - Simplify if statement in packageListing.tsx - Remove useSession boolean variable and just pass true to getPackageListingDetails
Use nested Promise/Await instead of Promise.all in component
…-and-panel-visbility Moderation tools review and panel visbility (TS-2907 & TS-2908 & TS-2910)
Add styling for displaying internal notes and rejection reason
- Add component for internal notes - Add component for rejection reason - Permissions handled in backend
Add InternalNotes and RejectionReason in packageListing.tsx under the ManagementTools as these components require the status data.
Review information access (TS-2909)
- Update loader and clientloader - Use getPrivateListing & getPublicListing - Get private package listing for authorized users else 404 - Add checks for if listing is undefined to prevent not expected 404's
- Fix the url for unlisting package listings
Ensure the category select field is properly populated when listing data loads.
- Wrap the categories section in a permissions check for managing categories
PackageListing as a concept is version-agnostic, but to ensure we don't serve pages for rejected package versions, they need to be community-scoped, as rejection is tied to PackageListing object. This commit adds the support to Dapper and listingUtils, the actual usage will be done in a separate commit. Since by default a PackageListing shows the latest package version, the version argument remains optional.
- Use Dapper's getPackageListingDetails method rather than getPackageVersionDetails as the latter may serve rejected content which we don't want to show - SSR loader and clientLoader mimic what their counterparts in packageListing.tsx does, namely loader makes the request unauthenticated and returns undefined if the response is 404, while clientLoader first does an unauthenticated request, and should that fail, another one as authenticated. This is done to improve cache hits - Since both loaders now await the responses for listing, it's never a promise and lot of the Suspense/Await elements become obsolete
The required information is now available in the listing data too.
I must admit I'm meddling with powers I don't comprehend. Why was this implemented this way? Is it still needed? At least for me both SSR and client side loading seem to work without issues.
Considered PackageVersionListing too but I think this pairs nicer with PackageListing component.
- Return null if status is undefined in InternalNotes - Return null if status is undefined in RejectionReason
- Add getUserPermissions to listingUtils.ts - Add getPackageListingStatus to ListingUtils.ts - Return undefined from getPackageListingStatus if API return 403
- Use functions from listingUtils.ts instead of implementing them in the component
…ting-page Bug 403 on package listing page (TS-2924)
Fix pacakge management (TS-2912)
Update data fetching of a specific version of a listed package
Listing version cleanup
Content moderators currently reject packages by marking the PackageListing object rejected. PackageListings are community-scoped. Some of the endpoints are package scoped, i.e. they are not related to any communities or listings. These endpoints therefore currently have no way of telling if a package has been rejected or not. In worst case, the rejected packages can contain malware, so under no circumstances do we want to show such packages on the website where unsuspecting users can be tricked to download the mods. Therefore the pages of the website that use such endpoints are disabled until we figure out how to address the problem. Since the old website doesn't have unscoped endpoints, from feature parity perspective the new one doesn't need to have them either. Footgun warning: the website still uses some endpoints that are not community-scoped, such as "get README for this version of the package". This is fine as long as the page where the data is shown takes measures to ensure the content can be shown by querying the package listing endpoint too and showing 404 error if the package is rejected.
- The only difference between the components was that one expected the
route to contain communityId which is used to fetch the latest
version number, while the two other expected packageVersion to be
included in the URL. Loaders can easily check the route params and
serve both cases
- We may want to do something similar to readme and versions
components that also exist in triplicate
- The part of the code that fetches the package listing details when
packageVersion is not provided by the URL now uses the same approach
as the PackageListing component does. This fixes an issue where the
tab would show 404 when viewing a rejected package's dependencies,
even if the user was authententicated and had permissions to view it
- React-router uses the component name as the default id, but since we
have multiple routes for the same component, we need to define ids
manually
- Parent route PackageListing and sub route Required both request package listing data from backend - Using the recently added juiced up Dapper instance, multiple request to same endpoint with same arguments withing a request are deduplicated - Technically this doesn't help in packageListingVersion. While it makes the request, its sub route doesn't since it has the required data available as URL parameter. I decided to change the parent route to use the deduplicating Dapper version anyway for consistency's sake
Improve dependency list data fetching
- Move package meta tags into an own component - Use PackageMetaTags in the PackageListing component
Add new team interface containing: - identifier - name - donation_link
- Implement PackageHeader comoponent - Use component in packageListing.tsx
Implement component for package boxes in an own re-usable file.
- Implement component to be used as sub-component for package drawer - Use PackageDrawer in packageListing.tsx
Implement package actions in an own component. Use this sub-component in the packageListing component.
Implement and use PackageTabs in packageListing.tsx.
- Removed unused helper functions which are moved into sub-components - Remove more unused imports - Use sub-components where applicable
Implement PackageSidebar and use this sub-component in packageListing.tsx
* Add null and make parameters nullable
Contributor
|
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 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## moderation-tools-feature-branch #1686 +/- ##
===================================================================
- Coverage 11.71% 11.67% -0.04%
===================================================================
Files 321 329 +8
Lines 22849 22958 +109
Branches 510 521 +11
===================================================================
+ Hits 2676 2681 +5
- Misses 20173 20277 +104 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+17
to
+36
| function packageTags( | ||
| listing: PackageListingDetails, | ||
| community: Community | Promise<Community> | ||
| ) { | ||
| return listing.categories.map((category) => { | ||
| return ( | ||
| <NewTag | ||
| key={category.name} | ||
| csMode="cyberstormLink" | ||
| linkId="Community" | ||
| community={community.identifier} | ||
| queryParams={`includedCategories=${category.id}`} | ||
| csSize="small" | ||
| csVariant="primary" | ||
| > | ||
| {category.name} | ||
| </NewTag> | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Type Safety Bug: Accessing property on potentially unresolved Promise
The packageTags function accepts community: Community | Promise<Community> but directly accesses community.identifier on line 27. If community is a Promise<Community> (which the type allows), this will fail at runtime with a TypeError since Promises don't have an identifier property.
// Current (buggy):
function packageTags(
listing: PackageListingDetails,
community: Community | Promise<Community>
) {
// ...
community={community.identifier} // Fails if community is Promise
}
// Fix: Change type to only accept resolved Community:
function packageTags(
listing: PackageListingDetails,
community: Community // Remove Promise option
) {
// ...
community={community.identifier} // Now safe
}While current usage wraps calls in <Await>, the function signature allows direct calls with unresolved Promises, creating a potential runtime crash.
Suggested change
| function packageTags( | |
| listing: PackageListingDetails, | |
| community: Community | Promise<Community> | |
| ) { | |
| return listing.categories.map((category) => { | |
| return ( | |
| <NewTag | |
| key={category.name} | |
| csMode="cyberstormLink" | |
| linkId="Community" | |
| community={community.identifier} | |
| queryParams={`includedCategories=${category.id}`} | |
| csSize="small" | |
| csVariant="primary" | |
| > | |
| {category.name} | |
| </NewTag> | |
| ); | |
| }); | |
| } | |
| function packageTags( | |
| listing: PackageListingDetails, | |
| community: Community | |
| ) { | |
| return listing.categories.map((category) => { | |
| return ( | |
| <NewTag | |
| key={category.name} | |
| csMode="cyberstormLink" | |
| linkId="Community" | |
| community={community.identifier} | |
| queryParams={`includedCategories=${category.id}`} | |
| csSize="small" | |
| csVariant="primary" | |
| > | |
| {category.name} | |
| </NewTag> | |
| ); | |
| }); | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Contributor
There was a problem hiding this comment.
Change the type to just Community and this should be good
Oksamies
approved these changes
Jan 12, 2026
Comment on lines
+17
to
+36
| function packageTags( | ||
| listing: PackageListingDetails, | ||
| community: Community | Promise<Community> | ||
| ) { | ||
| return listing.categories.map((category) => { | ||
| return ( | ||
| <NewTag | ||
| key={category.name} | ||
| csMode="cyberstormLink" | ||
| linkId="Community" | ||
| community={community.identifier} | ||
| queryParams={`includedCategories=${category.id}`} | ||
| csSize="small" | ||
| csVariant="primary" | ||
| > | ||
| {category.name} | ||
| </NewTag> | ||
| ); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Change the type to just Community and this should be good
Roffenlund
force-pushed
the
moderation-tools-feature-branch
branch
from
0d74972 to
8e553ae
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors packageListing.tsx to improve readability, maintainability, and type safety by decomposing the component into focused sub-components and cleaning up related logic.
Changes