Refactor PaginatedDependencies to use loader-resolved data #1595
Conversation
|
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. WalkthroughPaginatedDependencies was simplified from a memoized, Suspense/Await-driven component to a plain exported function Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
Fixed
Show fixed
Hide fixed
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
Fixed
Show fixed
Hide fixed
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
Fixed
Show fixed
Hide fixed
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
40-45: Consider passingcommunityIdtoListingDependencywhen available.The
communityIdprop is optional but enables better routing. If this component has access tocommunityIdfrom route params or context, consider passing it through for optimal link behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx(1 hunks)apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
apps/cyberstorm-remix/app/commonComponents/ListingDependency/ListingDependency.tsx (1)
ListingDependency(10-100)
🪛 ESLint
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
[error] 7-7: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 8-8: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 24-24: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
🪛 GitHub Check: ESLint
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
[failure] 7-7: Disallow the any type
Unexpected any. Specify a different type.
[failure] 8-8: Disallow the any type
Unexpected any. Specify a different type.
[failure] 24-24: Disallow unused expressions
Expected an assignment or function call and instead saw an expression.
⏰ 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: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (2)
apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (1)
62-72: LGTM! Loader now returns resolved data.The change correctly awaits both dapper calls before returning, ensuring
PaginatedDependenciesreceives resolved objects instead of promises. This aligns with the PR objective and matches the server loader pattern.apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
13-18: Clean refactor to plain function component.The destructured props with inline defaults is a clear pattern. Removing the memo wrapper is appropriate here since the parent loader controls re-renders.
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
Outdated
Show resolved
Hide resolved
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1595 +/- ##
=========================================
+ Coverage 9.88% 9.91% +0.02%
=========================================
Files 308 308
Lines 22502 22445 -57
Branches 404 404
=========================================
Hits 2225 2225
+ Misses 20277 20220 -57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Roffenlund
force-pushed
the
fix-package-version-dependendencies-pagination
branch
from
98b86ff to
589157b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (2)
33-38: Guard against invalidpagequery values before hitting the API.If the URL contains something like
?page=abcor?page=-1,Number(page)yieldsNaN. ThatNaNthen flows intogetPackageVersionDependencies, which can trigger backend validation errors (or default to page0) and break pagination for the user. Please validate the search param and fall back toundefined(or1) when it isn’t a positive integer.Consider patching along these lines:
- const page = searchParams.get("page"); + const rawPage = searchParams.get("page"); + const parsedPage = + rawPage && /^\d+$/.test(rawPage) && Number(rawPage) > 0 + ? Number(rawPage) + : undefined; @@ - dependencies: await dapper.getPackageVersionDependencies( + dependencies: await dapper.getPackageVersionDependencies( params.namespaceId, params.packageId, listing.latest_version_number, - page === null ? undefined : Number(page) + parsedPage ),
61-73: Mirror the same validation in the client loader.For parity with the server loader and to avoid client-side NaN pagination, please run the same positive-integer guard in the client loader before invoking
getPackageVersionDependencies.apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx (2)
28-34: Validate thepagesearch param before passing it downstream.Same issue here:
Number(page)returnsNaNfor anything non-numeric, which bubbles intogetPackageVersionDependenciesand can break responses. Add the same positive-integer guard used inRequired.tsx.
52-62: Apply the validation in the client loader as well.Please duplicate the guard in the client loader to keep behaviour identical between server/client execution paths.
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx (2)
28-34: Harden the loader’s page parsing.Identical to the other loaders: ensure the
pagequery param is a positive integer before handing it togetPackageVersionDependencies. Invalid values currently becomeNaN, resulting in broken pagination or upstream errors.
52-62: Do the same in the client loader path.Please add the same validation here so both loader variants stay aligned.
♻️ Duplicate comments (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
24-35: Default to a sanepagewhen the URL param is invalid.
Number(searchParams.get("page") ?? 1)returnsNaNif the param is absent, empty, negative, or non-numeric. PassingNaNintoNewPaginationwill break the control (current page becomes NaN, pagination UI stops responding). Please reuse the same positive-integer guard as in the loaders and fall back to 1.Example:
- const page = Number(searchParams.get("page") ?? 1); + const rawPage = searchParams.get("page"); + const page = + rawPage && /^\d+$/.test(rawPage) && Number(rawPage) > 0 + ? Number(rawPage) + : 1;
🧹 Nitpick comments (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
7-14: Spell the response type correctly for clarity.Minor nit: the interface is named
DependecyResponse. Please rename it toDependencyResponseto avoid propagating the typo in future imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx(1 hunks)apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx(2 hunks)apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx(2 hunks)apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx (2)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx (1)
clientLoader(39-66)apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
PaginatedDependencies(18-67)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
apps/cyberstorm-remix/app/commonComponents/ListingDependency/ListingDependency.tsx (1)
ListingDependency(10-100)
apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (2)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx (2)
loader(10-37)clientLoader(39-66)apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
PaginatedDependencies(18-67)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx (2)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx (2)
loader(10-37)clientLoader(39-66)apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
PaginatedDependencies(18-67)
⏰ 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: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (1)
apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (1)
79-80: Propagated page state should use the validated value.After validating
parsedPage, make sure the value returned from the loader is consistent soPaginatedDependenciesreceives clean data (page defaults to 1 when undefined).
- Remove Promise.all, Suspense, and useMemo from the component - Simplify Props to accept resolved objects instead of promises - Use URL query param as single source of truth for page - Remove local/debounced state, refs, and navigation effects - Ensure stable keys for list items - Pass only the resolved dependencies to PaginatedDependencies - Component now renders cleanly with correct pagination Previously, mixing unresolved promises, local state, and debouncing caused stale/mis-synced data and infinite re-render loops. Now the component is simpler, predictable, and fully driven by loader data and URL. Refs. TS-2750
Roffenlund
force-pushed
the
fix-package-version-dependendencies-pagination
branch
from
589157b to
1b57d78
Compare
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx (1)
21-65: Sanitizepagebefore hitting the dependencies endpointBoth loader branches convert the query param with
Number(page), so?page=abc,?page=-2, or?page=will result inNaN/non-positive values being forwarded togetPackageVersionDependencies. That yields requests with invalid page numbers and can surface 400s or empty data. Please validate and clamp the value before passing it downstream.- const page = searchParams.get("page"); + const rawPage = searchParams.get("page"); + const page = + rawPage && /^\d+$/.test(rawPage) ? Math.max(1, Number(rawPage)) : undefined; ... - page === null ? undefined : Number(page) + pageApply the same guard in the
clientLoader.apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (1)
21-75: Clamp thepagequery param before callinggetPackageVersionDependenciesSame pattern here:
Number(page)forwardsNaN/<=0 when the URL contains an invalid page. Please re-use the guarded parsing so the loader only ever sendsundefined(default first page) or a positive integer to the API, and mirror it inclientLoader.- const page = searchParams.get("page"); + const rawPage = searchParams.get("page"); + const page = + rawPage && /^\d+$/.test(rawPage) ? Math.max(1, Number(rawPage)) : undefined; ... - page === null ? undefined : Number(page) + pageapps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx (1)
21-65: Guard thepageparameter before forwarding it to the dependencies callHere too,
Number(page)will yieldNaN/non-positive values for malformed input, which then travels togetPackageVersionDependencies. Please sanitize the query param (and mirror inclientLoader) so onlyundefinedor a positive integer goes to the API.- const page = searchParams.get("page"); + const rawPage = searchParams.get("page"); + const page = + rawPage && /^\d+$/.test(rawPage) ? Math.max(1, Number(rawPage)) : undefined; ... - page === null ? undefined : Number(page) + page
♻️ Duplicate comments (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
24-36: Validate page query parsing before using it
Number(searchParams.get("page") ?? 1)treats any non-numeric value (e.g.?page=abc,?page=-1) asNaNor <= 0, which then gets passed ascurrentPageto<NewPagination>. That breaks pagination state and can desync the UI from the URL when a user (or crawler) hits an invalidpagevalue. Please guard the query param so we always fall back to page 1 for invalid/negative inputs.- const page = Number(searchParams.get("page") ?? 1); + const rawPage = searchParams.get("page"); + const parsedPage = + rawPage && /^\d+$/.test(rawPage) ? Math.max(1, Number(rawPage)) : 1; + const page = parsedPage;
🧹 Nitpick comments (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
7-14: Fix the typo inDependecyResponseThe interface name is misspelled (
DependecyResponse). Renaming it toDependencyResponsekeeps things readable and avoids propagation of the typo.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx(1 hunks)apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx(2 hunks)apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx(2 hunks)apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
PaginatedDependencies(18-67)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
apps/cyberstorm-remix/app/commonComponents/ListingDependency/ListingDependency.tsx (1)
ListingDependency(10-100)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
PaginatedDependencies(18-67)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
PaginatedDependencies(18-67)
⏰ 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: Build
- GitHub Check: Generate visual diffs
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
24-24: Invalid page values can produce NaN.If the URL contains an invalid page parameter like
page=abc,Number("abc")returnsNaN, which could break pagination logic.Apply this diff to ensure robust parsing:
- const page = Number(searchParams.get("page") ?? 1); + const page = Math.max(1, Number(searchParams.get("page")) || 1);
🧹 Nitpick comments (2)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (2)
50-50: Remove redundant type annotation.The
idx: numberannotation is unnecessary as TypeScript infers the type from themapcallback.Apply this diff:
- {dependencies.results.map((dep, idx: number) => ( + {dependencies.results.map((dep, idx) => (
49-56: Consider adding empty state handling.When
dependencies.countis 0, the component still renders the title and pagination UI. Consider showing a more user-friendly empty state message instead.Apply this diff:
<div className="paginated-dependencies__body"> + {dependencies.results.length === 0 ? ( + <div className="paginated-dependencies__empty"> + No dependencies found. + </div> + ) : ( {dependencies.results.map((dep, idx) => ( <ListingDependency key={`${dep.name}-${dep.version_number}-${idx}`} dependency={dep} /> ))} + )} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
apps/cyberstorm-remix/app/commonComponents/ListingDependency/ListingDependency.tsx (1)
ListingDependency(10-100)
⏰ 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: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (7)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (7)
1-10: LGTM!Clean imports and well-defined types. The
DependencyResponseinterface correctly captures the paginated structure.
12-16: LGTM!Props are cleanly defined with proper types. The removal of the
versionprop and elimination ofanytypes align well with the refactoring goals.
18-22: LGTM!The plain function export with sensible defaults is much cleaner than the previous memoized version.
26-36: LGTM!The page change handler correctly manages URL state, removing the param for page 1 to keep URLs clean. Good use of
preventScrollReset.
40-47: LGTM, but note the specificity.The hardcoded "Required mods" title and description make this component specific to the package dependencies use case. If broader reusability is needed later, consider making these configurable via props.
58-64: LGTM!Pagination props are correctly wired. Just ensure the page parsing issue (Line 24) is addressed to prevent passing NaN to
currentPage.
69-69: LGTM!Good practice for React DevTools, though optional with named function exports.
| <ListingDependency | ||
| key={`${dep.name}-${dep.version_number}-${idx}`} | ||
| dependency={dep} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if communityId should be passed to ListingDependency.
The ListingDependency component accepts an optional communityId prop to determine which link variant to use. Currently, no communityId is passed, so it will always use the "WithoutCommunity" links. Confirm whether this is intentional based on where this component is used, or if communityId should be added to the props and passed through.
Run the following script to check how this component is consumed:
🏁 Script executed:
#!/bin/bash
# Description: Check how PaginatedDependencies is used in the codebase
# Search for PaginatedDependencies usage
rg -n -C5 '<PaginatedDependencies' --type=tsx --type=tsLength of output: 27
🏁 Script executed:
#!/bin/bash
# Description: Search for PaginatedDependencies usage and ListingDependency prop definition
# Using glob patterns instead of file types
# Find PaginatedDependencies usage
echo "=== PaginatedDependencies Usage ==="
rg -n -C5 '<PaginatedDependencies' -g '*.tsx' -g '*.ts'
# Find ListingDependency component definition
echo -e "\n=== ListingDependency Component Definition ==="
rg -n -B2 -A10 'interface.*ListingDependency|type.*ListingDependency|function ListingDependency|const ListingDependency.*=' -g '*.tsx' -g '*.ts' | head -40
# Find ListingDependency prop interface/type
echo -e "\n=== ListingDependency Props ==="
rg -n -B2 -A15 'interface.*ListingDependencyProps|type.*ListingDependencyProps' -g '*.tsx' -g '*.ts'Length of output: 8678
🏁 Script executed:
#!/bin/bash
# Description: Check PaginatedDependencies component definition and parent context
# Find PaginatedDependencies component definition
echo "=== PaginatedDependencies Definition ==="
rg -n -B2 -A20 'interface.*PaginatedDependenciesProps|type.*PaginatedDependenciesProps|function PaginatedDependencies|export.*PaginatedDependencies' -g '*.tsx' -g '*.ts' apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/
# Check parent component for communityId availability
echo -e "\n=== Required.tsx context (first 50 lines) ==="
head -50 apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx
# Check PackageVersionWithoutCommunityRequired for comparison
echo -e "\n=== PackageVersionWithoutCommunityRequired.tsx context (first 50 lines) ==="
head -50 apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsxLength of output: 6471
🏁 Script executed:
#!/bin/bash
# Description: Check how communityId is used in ListingDependency and find other usages
# Look at ListingDependency implementation to see how communityId is used
echo "=== ListingDependency Implementation (full file) ==="
cat apps/cyberstorm-remix/app/commonComponents/ListingDependency/ListingDependency.tsx
# Search for other usages of ListingDependency with communityId passed
echo -e "\n=== Other ListingDependency usages in codebase ==="
rg -n -B2 -A2 '<ListingDependency' -g '*.tsx' -g '*.ts'Length of output: 4188
🏁 Script executed:
#!/bin/bash
# Description: Confirm PackageVersionRequired has communityId
echo "=== PackageVersionRequired.tsx loader (first 40 lines) ==="
head -40 apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx | grep -A 10 "export async function loader"Length of output: 530
Add communityId prop to PaginatedDependencies and pass it to ListingDependency; update Required.tsx to provide it.
The review comment is valid. ListingDependency uses communityId to select between link variants ("PackageVersion" with community vs "PackageVersionWithoutCommunity" without). Currently, PaginatedDependencies doesn't accept or forward this prop, so Required.tsx—which has access to communityId—cannot use it. This causes dependency links in Required.tsx to render with the wrong link type.
Changes needed:
- Add
communityId?: stringto PaginatedDependencies props - Pass it to the
ListingDependencycomponent (line 51) - Update Required.tsx to pass
communityIdfrom route params toPaginatedDependencies
Note: PackageVersionRequired.tsx and PackageVersionWithoutCommunityRequired.tsx have no communityId in their route params, so they remain unchanged.
🤖 Prompt for AI Agents
In
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
around lines 51–54, PaginatedDependencies does not accept or forward the
communityId prop so ListingDependency renders the wrong link variant; add
communityId?: string to PaginatedDependencies props, pass communityId into each
<ListingDependency ... /> (preserve current key/props) and then update
Required.tsx to extract communityId from route params and pass it into
PaginatedDependencies when rendering it.
- loader fetches and resolves dependencies - clientLoader returns promises - Resolve promises with Await and render skeletonbox - Pass only resolved dependencies to PaginatedDependencies Refs. TS-2750
Refs. TS-2750
Roffenlund
force-pushed
the
fix-package-version-dependendencies-pagination
branch
from
356e9fe to
e914a3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (1)
21-75: Sanitise page before calling the API
Number(page)happily producesNaN(or 0) for inputs such as?page=abc, and we forward that straight intogetPackageVersionDependencies. That gives undefined behaviour upstream. Please validate the query param and only forward positive integers, defaulting to page 1 otherwise. For example:- const page = searchParams.get("page"); + const rawPage = searchParams.get("page"); + const pageNumber = + rawPage && /^\d+$/.test(rawPage) ? Math.max(1, Number(rawPage)) : 1; + const pageParam = pageNumber === 1 ? undefined : pageNumber; ... - dependencies: await dapper.getPackageVersionDependencies( + dependencies: await dapper.getPackageVersionDependencies( ... - page === null ? undefined : Number(page) + pageParam ),Make the same adjustment inside
clientLoaderso both loaders stay aligned.apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx (1)
21-65: Sanitise page before fetching dependenciesHere as well,
Number(page)can becomeNaN/0 and we relay that straight togetPackageVersionDependencies. Please reuse the positive-integer parsing from Required.tsx in both the server and client loaders. For example:- const page = searchParams.get("page"); + const rawPage = searchParams.get("page"); + const pageNumber = + rawPage && /^\d+$/.test(rawPage) ? Math.max(1, Number(rawPage)) : 1; + const pageParam = pageNumber === 1 ? undefined : pageNumber; ... - page === null ? undefined : Number(page) + pageParamApply the same change inside
clientLoaderto keep behaviour consistent.apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx (1)
21-65: Sanitise page before fetching dependenciesLike the other tabs, this loader forwards
Number(page)directly, so?page=abcresults inNaNhitting the API. Please validate the query param and only pass positive integers (default to 1/undefined). Mirroring the earlier pattern:- const page = searchParams.get("page"); + const rawPage = searchParams.get("page"); + const pageNumber = + rawPage && /^\d+$/.test(rawPage) ? Math.max(1, Number(rawPage)) : 1; + const pageParam = pageNumber === 1 ? undefined : pageNumber; ... - page === null ? undefined : Number(page) + pageParamCarry the same fix into
clientLoaderto keep parity.
♻️ Duplicate comments (2)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (2)
49-54: Propagate communityId to ListingDependencyWithout forwarding
communityId, dependencies rendered inside community-scoped routes always use the non-community link variant, so clicks jump out of the community pages. Please accept the optional id and pass it through:interface Props { dependencies: DependencyResponse; + communityId?: string; } export function PaginatedDependencies({ dependencies, + communityId, pageSize = 20, // Default page size from backend siblingCount = 4, }: Props) { ... <ListingDependency key={`${dep.name}-${dep.version_number}-${idx}`} dependency={dep} + communityId={communityId} />Then plumb the identifier from
Required.tsx(and any other community-aware caller) so links stay in the correct namespace.
24-35: Guard the page search param before using itIf someone lands here with
?page=abcorpage=0,Number(...)turns intoNaN/0, soNewPaginationand the handler get an invalid page and the UI behaves unpredictably. Please validate that the query string is a positive integer and fall back to 1. For example:- const page = Number(searchParams.get("page") ?? 1); + const rawPage = searchParams.get("page"); + const page = + rawPage && /^\d+$/.test(rawPage) ? Math.max(1, Number(rawPage)) : 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx(1 hunks)apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx(2 hunks)apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx(2 hunks)apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
PaginatedDependencies(18-67)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
apps/cyberstorm-remix/app/commonComponents/ListingDependency/ListingDependency.tsx (1)
ListingDependency(10-100)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionWithoutCommunityRequired.tsx (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
PaginatedDependencies(18-67)
apps/cyberstorm-remix/app/p/tabs/Required/PackageVersionRequired.tsx (1)
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx (1)
PaginatedDependencies(18-67)
⏰ 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: Build
- GitHub Check: Generate visual diffs
| {(resolvedDependencies) => ( | ||
| <PaginatedDependencies dependencies={resolvedDependencies} /> | ||
| )} | ||
| </Await> |
There was a problem hiding this comment.
Pass communityId through to PaginatedDependencies
We need the community context so ListingDependency emits the community-specific links. Right now we drop it, so Required → dependency links omit the community segment. Please pull it from useParams() (or loader data) and forward it:
-import { useLoaderData, Await } from "react-router";
+import { useLoaderData, Await, useParams } from "react-router";
...
- const { dependencies } = useLoaderData();
+ const { dependencies } = useLoaderData();
+ const { communityId } = useParams();
...
- <PaginatedDependencies dependencies={resolvedDependencies} />
+ <PaginatedDependencies
+ dependencies={resolvedDependencies}
+ communityId={communityId}
+ />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {(resolvedDependencies) => ( | |
| <PaginatedDependencies dependencies={resolvedDependencies} /> | |
| )} | |
| </Await> | |
| {(resolvedDependencies) => ( | |
| <PaginatedDependencies | |
| dependencies={resolvedDependencies} | |
| communityId={communityId} | |
| /> | |
| )} | |
| </Await> |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx around lines 93 to 96,
the communityId from route params (or loader data) is not being passed into
PaginatedDependencies so ListingDependency cannot build community-specific
links; retrieve communityId via useParams() (or from the loader data already
available in this component), add it as a prop when rendering
<PaginatedDependencies />, and update PaginatedDependencies prop types/signature
if needed to accept and forward communityId down to ListingDependency so links
include the community segment.
Oksamies
left a comment
Oksamies
left a comment
There was a problem hiding this comment.
Everything else is looking good, but the one question needs to be addressed 👍
apps/cyberstorm-remix/app/commonComponents/PaginatedDependencies/PaginatedDependencies.tsx
Show resolved
Hide resolved
Previously, mixing unresolved promises, local state, and debouncing caused stale/mis-synced data and infinite re-render loops. Now the component is simpler, predictable, and fully driven by loader data and URL.
Refs. TS-2750