Improve dependency list data fetching #1674

anttimaki · 2025-12-19T06:30:03Z

No description provided.

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

coderabbitai · 2025-12-19T06:30:08Z

Walkthrough

This PR refactors package listing and version loaders to use request-scoped Dapper instances via getDapperForRequest() instead of constructing instances from config or session data. It consolidates two separate package version required tab files (PackageVersionRequired.tsx and PackageVersionWithoutCommunityRequired.tsx) into a unified Required.tsx loader with improved SSR failure handling and client-side hydration. The routes configuration is updated to point both versioned and community-agnostic routes to the shared Required.tsx implementation.

Possibly related PRs

7.0.3 fixes #1516: Modifies the same packageListing.tsx clientLoader to change Dapper initialization strategy.
Remove non-needed null checks #1545: Updates packageListing.tsx clientLoader with related Dapper configuration changes.
Package Version pages #1547: Introduces the package version "Required" tab files that are now being consolidated and restructured in this PR.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Description check	❓ Inconclusive	No pull request description was provided by the author.	Add a brief description explaining the refactoring rationale and impact, such as why request-scoped Dapper improves data fetching or what issues this resolves.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Improve dependency list data fetching' accurately reflects the main change: refactoring how dependencies are loaded across multiple files by switching from session-based to request-scoped Dapper initialization.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch version-dependency-merge

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1b17ae0 and 7aa6c26.

📒 Files selected for processing (6)

apps/cyberstorm-remix/app/p/packageListing.tsx (2 hunks)
apps/cyberstorm-remix/app/p/packageListingVersion.tsx (2 hunks)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx (0 hunks)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx (0 hunks)
apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (1 hunks)
apps/cyberstorm-remix/app/routes.ts (2 hunks)

💤 Files with no reviewable changes (2)

apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-12-09T12:41:16.448Z

Learnt from: CR
Repo: thunderstore-io/thunderstore-ui PR: 0
File: apps/cyberstorm-remix/docs/error-boundary.md:0-0
Timestamp: 2025-12-09T12:41:16.448Z
Learning: Applies to apps/cyberstorm-remix/docs/**/*.route.{ts,tsx} : Use `RouteErrorBoundary` in route components by exporting it as `ErrorBoundary` from the route component file

Applied to files:

apps/cyberstorm-remix/app/routes.ts

📚 Learning: 2025-12-09T12:41:16.448Z

Learnt from: CR
Repo: thunderstore-io/thunderstore-ui PR: 0
File: apps/cyberstorm-remix/docs/error-boundary.md:0-0
Timestamp: 2025-12-09T12:41:16.448Z
Learning: Applies to apps/cyberstorm-remix/docs/**/*.route.{ts,tsx} : `RouteErrorBoundary` catches errors in loaders and Promise rejections - ensure Suspense and Await are used in the main component to handle loader Promise rejections

Applied to files:

apps/cyberstorm-remix/app/routes.ts
apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx

🧬 Code graph analysis (3)

apps/cyberstorm-remix/app/p/packageListingVersion.tsx (3)

apps/cyberstorm-remix/app/p/packageListing.tsx (1)

clientLoader (136-175)

apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (1)

clientLoader (66-95)

apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)

getDapperForRequest (62-102)

apps/cyberstorm-remix/app/p/packageListing.tsx (2)

apps/cyberstorm-remix/app/p/packageListingVersion.tsx (1)

clientLoader (78-99)

apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)

getDapperForRequest (62-102)

apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (3)

apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)

getPublicEnvVariables (19-48)

apps/cyberstorm-remix/app/p/listingUtils.ts (2)

getPublicListing (16-35)

getPrivateListing (43-76)

apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)

getDapperForRequest (62-102)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: Generate visual diffs
GitHub Check: Test

🔇 Additional comments (8)

apps/cyberstorm-remix/app/p/packageListingVersion.tsx (1)

78-98: LGTM! Request-scoped Dapper pattern applied correctly.

The clientLoader now properly uses getDapperForRequest(request) for request-scoped caching and promise deduplication, consistent with the pattern in packageListing.tsx.

apps/cyberstorm-remix/app/routes.ts (2)

44-46: Route consolidation looks good.

Using the shared Required.tsx with a route id to differentiate contexts is a clean approach.

53-75: Clear documentation for disabled routes.

The comment explains the rationale for disabling the community-agnostic routes. Good practice to document the "why" when commenting out code blocks rather than deleting them.

apps/cyberstorm-remix/app/p/packageListing.tsx (1)

136-174: LGTM! Consistent request-scoped Dapper usage.

The clientLoader correctly uses getDapperForRequest(request) and maintains the deferred loading pattern (returning Promises for Suspense/Await consumption).

apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (4)

12-20: Clean helper functions.

Dependency404 as a reusable Response and getPageFromUrl for pagination extraction are well-defined utilities.

22-64: SSR loader handles unauthenticated fallback gracefully.

Returning { dependencies: undefined } instead of throwing when the listing isn't publicly available allows the client to retry with authentication. The await on getPackageVersionDependencies is correct for SSR since the data must be serialized in the response.

66-95: Consider consistency: await vs deferred Promise.

The loader (line 57) awaits getPackageVersionDependencies, while clientLoader (line 88) returns the Promise directly. This is fine for deferred rendering with Suspense/Await, but worth confirming it's intentional.

If you want consistency with other loaders that defer promises, this looks correct. If you need the data resolved before navigation completes on the client, add await.

99-122: SSR failure retry pattern implemented correctly.

When dependencies === undefined, rendering a SkeletonBox combined with clientLoader.hydrate = true ensures the client-side loader runs and retries with authentication. This aligns with the retrieved learning about using Suspense and Await for handling loader Promise rejections.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

anttimaki · 2025-12-19T06:30:33Z

@coderabbitai full review

coderabbitai · 2025-12-19T06:30:39Z

✅ Actions performed

Full review triggered.

codecov · 2025-12-19T06:32:38Z

Codecov Report

❌ Patch coverage is 0% with 80 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.73%. Comparing base (1b17ae0) to head (7aa6c26).

Files with missing lines	Patch %	Lines
.../cyberstorm-remix/app/p/tabs/Required/Required.tsx	0.00%	70 Missing ⚠️
...s/cyberstorm-remix/app/p/packageListingVersion.tsx	0.00%	4 Missing ⚠️
apps/cyberstorm-remix/app/p/packageListing.tsx	0.00%	3 Missing ⚠️
apps/cyberstorm-remix/app/routes.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##           listing-version-cleanup    #1674      +/-   ##
===========================================================
+ Coverage                    11.65%   11.73%   +0.08%     
===========================================================
  Files                          322      320       -2     
  Lines                        22963    22796     -167     
  Branches                       511      509       -2     
===========================================================
  Hits                          2676     2676              
+ Misses                       20287    20120     -167

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

anttimaki · 2025-12-19T06:51:10Z

FYI @VilppeRiskidev 7aa6c26 contains an example of deduplicating requests made by stacked routes (only for clientLoaders currently).

anttimaki added 3 commits December 19, 2025 07:16

anttimaki requested a review from Roffenlund December 19, 2025 06:49

Roffenlund approved these changes Dec 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve dependency list data fetching #1674

Improve dependency list data fetching #1674

Uh oh!

anttimaki commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025 •

edited

Loading

Uh oh!

anttimaki commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025

Uh oh!

codecov bot commented Dec 19, 2025

Uh oh!

anttimaki commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Improve dependency list data fetching #1674

Are you sure you want to change the base?

Improve dependency list data fetching #1674

Uh oh!

Conversation

anttimaki commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

anttimaki commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025

Uh oh!

codecov bot commented Dec 19, 2025

Codecov Report

Uh oh!

anttimaki commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Dec 19, 2025 •

edited

Loading