Update data fetching of a specific version of a listed package #1671
Conversation
|
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 WalkthroughThis PR extends package listing detail fetching to support optional version specification across the entire stack. Changes propagate from UI components through the Dapper SDK and into the API schemas and request handling. The Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 #1671 +/- ##
===================================================================
+ Coverage 11.61% 11.63% +0.02%
===================================================================
Files 322 322
Lines 23056 22990 -66
Branches 513 511 -2
===================================================================
- Hits 2677 2676 -1
+ Misses 20379 20314 -65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dapper-ts/src/methods/packageListings.ts (1)
121-143: Type signature mismatch withGetPackageListingDetails.The implementation includes a
useSessionparameter that isn't reflected in the public type definition atpackages/dapper/src/types/methods.ts. The type only defines 4 parameters, but the implementation (and actual usage inapps/cyberstorm-remix/app/p/listingUtils.ts:63-69) passes a 5th argument asuseSession.Add
useSession?: booleanto theGetPackageListingDetailstype definition to align with the implementation.
🧹 Nitpick comments (1)
packages/thunderstore-api/src/schemas/requestSchemas.ts (1)
169-178: Consider adding version format validation.The schema accepts any string for
version_number. Other schemas in this file (e.g.,packageChangelogRequestParamsSchemaat line 150) use a union withz.literal("latest"). If your API expects semantic versioning, consider adding a regex constraint.🔎 Optional: Add semver format validation
export const packageListingDetailsRequestParamsSchema = z.object({ community_id: z.string(), namespace_id: z.string(), package_name: z.string(), - version_number: z.string().optional(), + version_number: z.string().regex(/^\d+\.\d+\.\d+$/).optional(), });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/cyberstorm-remix/app/p/listingUtils.ts(4 hunks)apps/cyberstorm-remix/app/p/packageVersion.tsx(9 hunks)packages/dapper-fake/src/fakers/package.ts(2 hunks)packages/dapper-ts/src/methods/packageListings.ts(2 hunks)packages/dapper/src/types/methods.ts(1 hunks)packages/thunderstore-api/src/get/packageListingDetails.ts(1 hunks)packages/thunderstore-api/src/schemas/requestSchemas.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/dapper-fake/src/fakers/package.ts (1)
packages/dapper-fake/src/fakers/utils.ts (1)
setSeed(74-77)
apps/cyberstorm-remix/app/p/packageVersion.tsx (1)
apps/cyberstorm-remix/app/p/listingUtils.ts (2)
getPublicListing(16-35)getPrivateListing(43-76)
packages/thunderstore-api/src/get/packageListingDetails.ts (1)
packages/thunderstore-api/src/index.ts (1)
BASE_LISTING_PATH(17-17)
🔇 Additional comments (9)
packages/dapper/src/types/methods.ts (1)
44-49: LGTM!The optional
versionparameter addition is consistent with similar type signatures in this file (e.g.,GetPackageChangelog,GetPackageReadme). Backward compatible change.packages/dapper-fake/src/fakers/package.ts (1)
130-136: Optional version parameter correctly integrated.The logic to use provided version or fall back to generated one is clean. Note that the seed (line 133) doesn't include version, so all versions of the same package will produce identical fake data - acceptable for test fixtures.
apps/cyberstorm-remix/app/p/listingUtils.ts (3)
4-9: LGTM!Clean extension of
ListingIdentifierswith optionalpackageVersion. The property flows correctly through both public and private listing fetchers.
16-35: LGTM!Version parameter correctly threaded through to
dapper.getPackageListingDetails.
43-76: LGTM!Private listing fallback correctly includes
packageVersionin both the initial attempt (lines 50-55) and the authenticated retry (lines 63-68).apps/cyberstorm-remix/app/p/packageVersion.tsx (4)
61-87: LGTM!Server loader correctly awaits all data before returning. The guard now properly checks for
packageVersionpresence.
89-118: LGTM!Client loader correctly uses
getPrivateListingfor authenticated access. The un-awaitedcommunityandteampromises enable streaming with Suspense/Await pattern in the component. Thehydrate = trueflag is appropriately set.
392-425: LGTM!Actions component signature updated to use
listingandteam. Property access correctly useslisting.download_url.
427-464: LGTM!
packageMetahelper correctly updated to uselistingproperties for download count, size, and dependency string.
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
anttimaki
force-pushed
the
secure-listing-version
branch
from
046f1cd to
e384759
Compare
No description provided.