Listing version cleanup #1672
Listing version cleanup #1672
Conversation
WalkthroughThe component in Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
anttimaki
force-pushed
the
secure-listing-version
branch
from
046f1cd to
e384759
Compare
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.
anttimaki
force-pushed
the
listing-version-cleanup
branch
from
8f7247d to
1b17ae0
Compare
|
@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: 1
🧹 Nitpick comments (1)
apps/cyberstorm-remix/app/p/packageListingVersion.tsx (1)
162-162: Optional: Simplify redundant nullish coalescing.The
?? undefinedis redundant sincelisting.icon_urlalready resolves toundefinedif nullish.🔎 Simplify to:
- <meta property="og:image" content={listing.icon_url ?? undefined} /> + <meta property="og:image" content={listing.icon_url} />
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cyberstorm-remix/app/p/packageListingVersion.tsx(6 hunks)apps/cyberstorm-remix/app/routes.ts(1 hunks)
🧰 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
🧬 Code graph analysis (1)
apps/cyberstorm-remix/app/p/packageListingVersion.tsx (1)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getPublicEnvVariables(19-48)
⏰ 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 (5)
apps/cyberstorm-remix/app/p/packageListingVersion.tsx (4)
31-31: LGTM - Clean import simplification.Removing unused hooks aligns well with the refactoring goals.
125-128: LGTM - Component rename is consistent.The rename to
PackageListingVersionaligns with the file name and clarifies the component's purpose.
243-243: LGTM - Consistent with simplified function signature.Both calls correctly pass only the
listingparameter.Also applies to: 333-333
383-388: Verify ifsuppressHydrationWarningis still necessary.Since the hydration logic was removed from the component, double-check whether
suppressHydrationWarningis still needed onRelativeTime. It may still be required ifRelativeTimeinternally usesDate.now()or similar time-based values that differ between server and client renders.apps/cyberstorm-remix/app/routes.ts (1)
36-47: LGTM - Route correctly updated to use refactored component.The route now points to
packageListingVersion.tsx, which aligns with the component rename and refactoring work.
| <meta | ||
| title={`${formatToDisplayName( | ||
| listing.full_version_name | ||
| )} | Thunderstore - The ${listing.community_name} Mod Database`} | ||
| /> |
There was a problem hiding this comment.
Fix invalid title attribute on <meta> tag.
Line 140 uses <meta title={...}> which is invalid HTML. For setting the document title in React 19, use <title>{...}</title> instead.
🔎 Apply this diff to fix the title tag:
- <meta
- title={`${formatToDisplayName(
- listing.full_version_name
- )} | Thunderstore - The ${listing.community_name} Mod Database`}
- />
+ <title>
+ {`${formatToDisplayName(
+ listing.full_version_name
+ )} | Thunderstore - The ${listing.community_name} Mod Database`}
+ </title>📝 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.
| <meta | |
| title={`${formatToDisplayName( | |
| listing.full_version_name | |
| )} | Thunderstore - The ${listing.community_name} Mod Database`} | |
| /> | |
| <title> | |
| {`${formatToDisplayName( | |
| listing.full_version_name | |
| )} | Thunderstore - The ${listing.community_name} Mod Database`} | |
| </title> |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/packageListingVersion.tsx around lines 140 to
144, the JSX uses an invalid <meta title={...}> attribute; replace that meta tag
with a proper <title>{...}</title> element (rendered inside the document head or
the component that controls the page head) so the document title is set
correctly in React 19, keeping the same formatted string
`${formatToDisplayName(listing.full_version_name)} | Thunderstore - The
${listing.community_name} Mod Database`.
There was a problem hiding this comment.
@Oksamies do you reckon this is a valid issue? Would affect other pages as well if it is.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## secure-listing-version #1672 +/- ##
=======================================================
Coverage 11.64% 11.65%
=======================================================
Files 322 322
Lines 22990 22963 -27
Branches 512 511 -1
=======================================================
- Hits 2678 2676 -2
+ Misses 20312 20287 -25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
VilppeRiskidev
left a comment
VilppeRiskidev
left a comment
There was a problem hiding this comment.
Changes look good and don't seem to break anything, IDK about the title attribute though, so your call if you want to ask/wait for oksamies' comment.
No description provided.