Add package management tools #1507
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. WalkthroughAdds package-permissions end-to-end: new API endpoints/schemas, Dapper client method and types, fake implementation, Remix clientLoader for hydrated permissions, permission-driven UI (management tools, review/report/unlist flows), a new package edit page/route, and supporting CSS and link updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (Remix clientLoader)
participant Client as PackageListing UI (Suspense/Await)
participant Dapper as DapperTs client
participant API as thunderstore-api.fetchPackagePermissions
participant HTTP as apiFetch
participant Server as Backend
Browser->>Dapper: clientLoader.getPackagePermissions(community, namespace, package)
Dapper->>API: fetchPackagePermissions(params, config)
API->>HTTP: apiFetch GET /api/cyberstorm/package/{cid}/{nid}/{pkg}/permissions (useSession)
HTTP->>Server: HTTP request
Server-->>HTTP: 200 / 401
HTTP-->>API: response (mapped to PackagePermissions or throws)
API-->>Dapper: PackagePermissions (or throws)
Dapper-->>Browser: Promise (or undefined on 401)
Browser->>Client: Suspense/Await resolves permissions
Client->>Client: render management tools / modals conditional on permissions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Oksamies
force-pushed
the
08-18-add_package_management_tools
branch
from
4f7d4b5 to
2225899
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
packages/dapper/src/types/methods.ts
Outdated
| export type GetPackagePermissions = ( | ||
| namespaceId: string, | ||
| communityId: string, | ||
| packageName: string | ||
| ) => Promise<PackagePermissions>; |
There was a problem hiding this comment.
The parameter order in the GetPackagePermissions type definition is inconsistent with its implementation. The type defines parameters as (namespaceId, communityId, packageName) while the implementation in getPackagePermissions uses (communityId, namespaceId, packageName). To maintain consistency and prevent potential bugs, please update the type definition to match the implementation order.
| export type GetPackagePermissions = ( | |
| namespaceId: string, | |
| communityId: string, | |
| packageName: string | |
| ) => Promise<PackagePermissions>; | |
| export type GetPackagePermissions = ( | |
| communityId: string, | |
| namespaceId: string, | |
| packageName: string | |
| ) => Promise<PackagePermissions>; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| this.getPackageVersions = this.getPackageVersions.bind(this); | ||
| this.getPackageWiki = this.getPackageWiki.bind(this); | ||
| this.getPackageWikiPage = this.getPackageWikiPage.bind(this); | ||
| this.getPackagePermissions = this.getPackagePermissions.bind(this); |
There was a problem hiding this comment.
There appears to be a parameter order inconsistency between the type definition in methods.ts and the implementation in package.ts. The type definition expects (namespaceId, communityId, packageName) while the implementation uses (communityId, namespaceId, packageName). This could lead to subtle bugs when the method is called through its bound reference. Consider aligning the parameter order in both locations to prevent confusion.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/dapper/src/types/methods.ts (1)
63-68: Parameter order mismatch with implementation will cause incorrect requestsThe type here defines (namespaceId, communityId, packageName) while the implementation (packages/dapper-ts/src/methods/package.ts, Line 155) uses (communityId, namespaceId, packageName). This inconsistency will lead consumers to pass arguments in the wrong order and produce invalid API paths.
Align the type to the existing implementation and to other Dapper methods that accept (community, namespace, name) in that order:
-export type GetPackagePermissions = ( - namespaceId: string, - communityId: string, - packageName: string -) => Promise<PackagePermissions>; +export type GetPackagePermissions = ( + communityId: string, + namespaceId: string, + packageName: string +) => Promise<PackagePermissions>;Run this script to confirm the signatures and inspect call sites:
#!/bin/bash set -euo pipefail echo "Type alias in methods.ts:" rg -n -C1 'export type GetPackagePermissions' packages/dapper/src/types/methods.ts echo echo "Implementation signature in dapper-ts:" rg -n -C2 'export async function getPackagePermissions' packages/dapper-ts/src/methods/package.ts echo echo "All call sites (verify argument order visually):" rg -n -C3 '\bgetPackagePermissions\s*\(' -g '!**/node_modules/**'apps/cyberstorm-remix/app/p/packageListing.tsx (2)
22-25: Remove unused import 'fetchPackagePermissions' (failing ESLint).This import isn’t used. It’s causing the “Disallow unused variables” failure.
Apply this diff:
-import { - ApiError, - fetchPackagePermissions, -} from "@thunderstore/thunderstore-api"; +import { ApiError } from "@thunderstore/thunderstore-api";
172-174: Remove unusedpermissionsfromuseLoaderDatadestructuring (failing ESLint).
permissionsisn’t used. This is the second ESLint failure.Apply this diff:
- const { community, listing, team, permissions } = useLoaderData< + const { community, listing, team } = useLoaderData< typeof loader | typeof clientLoader >();
🧹 Nitpick comments (8)
packages/thunderstore-api/src/schemas/objectSchemas.ts (1)
174-201: Schemas align with downstream types; consider making unknown-key handling explicitThe new schemas match the Dapper types (see packages/dapper/src/types/package.ts) and cover all expected permission flags. To make runtime behavior explicit and catch accidental API shape changes early, consider marking these Zod objects as strict (or explicitly passthrough if that’s preferred).
Apply this diff to opt into strict validation:
-export const packageInfoSchema = z.object({ +export const packageInfoSchema = z.object({ community_id: z.string().min(1), namespace_id: z.string().min(1), package_name: z.string().min(1), -}); +}).strict(); -export const permissionsSchema = z.object({ +export const permissionsSchema = z.object({ can_manage: z.boolean(), can_manage_deprecation: z.boolean(), can_manage_categories: z.boolean(), can_deprecate: z.boolean(), can_undeprecate: z.boolean(), can_unlist: z.boolean(), can_moderate: z.boolean(), can_view_package_admin_page: z.boolean(), can_view_listing_admin_page: z.boolean(), -}); +}).strict(); -export const packagePermissionsSchema = z.object({ +export const packagePermissionsSchema = z.object({ package: packageInfoSchema, permissions: permissionsSchema, -}); +}).strict();packages/thunderstore-api/src/schemas/requestSchemas.ts (1)
592-601: New request schema is consistent with existing patterns; consider enforcing non-empty IDscommunity_id, namespace_id, and package_name are path segments. Adding .min(1) guards against accidental empty strings.
Apply this diff to harden the schema:
export const packagePermissionsRequestParamsSchema = z.object({ - community_id: z.string(), - namespace_id: z.string(), - package_name: z.string(), + community_id: z.string().min(1), + namespace_id: z.string().min(1), + package_name: z.string().min(1), });packages/dapper-ts/src/methods/package.ts (1)
156-174: Add explicit return type for better contract clarity and early drift detectionTyping the return as the API’s response type (or the dapper interface type if preferred) makes the contract explicit and helps catch schema drift during compile.
Apply this diff to annotate the return type using the existing API type:
-import { - fetchPackageChangelog, - fetchPackageReadme, - fetchPackageVersions, - postPackageSubmission, - fetchPackageSubmissionStatus, - fetchPackageWiki, - fetchPackageWikiPage, - fetchPackagePermissions, -} from "@thunderstore/thunderstore-api"; +import { + fetchPackageChangelog, + fetchPackageReadme, + fetchPackageVersions, + postPackageSubmission, + fetchPackageSubmissionStatus, + fetchPackageWiki, + fetchPackageWikiPage, + fetchPackagePermissions, + type PackagePermissionsResponseData, +} from "@thunderstore/thunderstore-api"; @@ -export async function getPackagePermissions( +export async function getPackagePermissions( this: DapperTsInterface, communityId: string, namespaceId: string, packageName: string -) { +): Promise<PackagePermissionsResponseData> {If you’d rather bind to the dapper-level type, import and use that instead (provided it’s exported from the dapper package).
packages/dapper/src/types/package.ts (1)
60-77: Optionally mark top-level fields readonly to reflect server-provided dataThese values are fetched from the API and not mutated by consumers. Readonly helps prevent accidental writes.
Apply this diff:
export interface PackagePermissions { - package: { + readonly package: { community_id: string; namespace_id: string; package_name: string; }; - permissions: { + readonly permissions: { can_manage: boolean; can_manage_deprecation: boolean; can_manage_categories: boolean; can_deprecate: boolean; can_undeprecate: boolean; can_unlist: boolean; can_moderate: boolean; can_view_package_admin_page: boolean; can_view_listing_admin_page: boolean; }; }packages/thunderstore-api/src/get/package.ts (1)
9-25: Encode path segments to avoid malformed URLs with special charactersWhile identifiers are usually slug-like, defensive encoding avoids surprises.
Apply this diff:
- const path = `api/cyberstorm/package/${params.community_id}/${params.namespace_id}/${params.package_name}/permissions`; + const path = `api/cyberstorm/package/${encodeURIComponent( + params.community_id + )}/${encodeURIComponent(params.namespace_id)}/${encodeURIComponent( + params.package_name + )}/permissions`;apps/cyberstorm-remix/app/p/packageListing.tsx (3)
121-123: Consider omittingpermissions: undefinedfrom the server loader.Returning an explicit
permissions: undefinedis harmless, but it increases payload and complicates types without being consumed. If you’re not SSR-populating permissions, consider dropping it until it’s used.Apply this diff if you decide to simplify:
- team: await dapper.getTeamDetails(params.namespaceId), - permissions: undefined, + team: await dapper.getTeamDetails(params.namespaceId),
104-124: Fetch in parallel to reduce page load latency.The four independent requests can be made concurrently.
Apply this diff to parallelize:
const dapper = new DapperTs(() => { return { apiHost: import.meta.env.VITE_API_URL, sessionId: undefined, }; }); - return { - community: await dapper.getCommunity(params.communityId), - communityFilters: await dapper.getCommunityFilters(params.communityId), - listing: await dapper.getPackageListingDetails( - params.communityId, - params.namespaceId, - params.packageId - ), - team: await dapper.getTeamDetails(params.namespaceId), - permissions: undefined, - }; + const [community, communityFilters, listing, team] = await Promise.all([ + dapper.getCommunity(params.communityId), + dapper.getCommunityFilters(params.communityId), + dapper.getPackageListingDetails( + params.communityId, + params.namespaceId, + params.packageId + ), + dapper.getTeamDetails(params.namespaceId), + ]); + return { community, communityFilters, listing, team, permissions: undefined };
137-166: Guard against missingwindow.Dapperand fetch in parallel for performance.If
window.Dapperisn’t initialized yet, this will throw at runtime. Also, these requests can be parallelized.Apply this diff:
export async function clientLoader({ params }: LoaderFunctionArgs) { if (params.communityId && params.namespaceId && params.packageId) { try { - const dapper = window.Dapper; - return { - community: await dapper.getCommunity(params.communityId), - communityFilters: await dapper.getCommunityFilters(params.communityId), - listing: await dapper.getPackageListingDetails( - params.communityId, - params.namespaceId, - params.packageId - ), - team: await dapper.getTeamDetails(params.namespaceId), - permissions: await dapper.getPackagePermissions( - params.communityId, - params.namespaceId, - params.packageId - ), - }; + const dapper = (window as any).Dapper; + if (!dapper) { + throw new Error( + "Dapper instance not found on window. Ensure window.Dapper is initialized before navigating to this page." + ); + } + const [community, communityFilters, listing, team, permissions] = + await Promise.all([ + dapper.getCommunity(params.communityId), + dapper.getCommunityFilters(params.communityId), + dapper.getPackageListingDetails( + params.communityId, + params.namespaceId, + params.packageId + ), + dapper.getTeamDetails(params.namespaceId), + dapper.getPackagePermissions( + params.communityId, + params.namespaceId, + params.packageId + ), + ]); + return { community, communityFilters, listing, team, permissions };Follow-up:
- Verify that
window.Dapperis consistently bootstrapped before client navigation to this route.- If you have a global type for
window.Dapper, consider adding it (e.g.,declare global { interface Window { Dapper: DapperInterface } }) to avoidany.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/cyberstorm-remix/app/p/packageListing.tsx(4 hunks)packages/dapper-ts/src/index.ts(3 hunks)packages/dapper-ts/src/methods/package.ts(2 hunks)packages/dapper/src/dapper.ts(1 hunks)packages/dapper/src/types/methods.ts(2 hunks)packages/dapper/src/types/package.ts(1 hunks)packages/thunderstore-api/src/get/package.ts(1 hunks)packages/thunderstore-api/src/index.ts(1 hunks)packages/thunderstore-api/src/schemas/objectSchemas.ts(1 hunks)packages/thunderstore-api/src/schemas/requestSchemas.ts(1 hunks)packages/thunderstore-api/src/schemas/responseSchemas.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (11)
packages/dapper/src/dapper.ts (2)
packages/dapper/src/types/methods.ts (1)
GetPackagePermissions(63-67)packages/dapper/src/context.tsx (1)
DapperInterface(23-26)
packages/thunderstore-api/src/index.ts (1)
apps/sertra/api/models.ts (1)
Package(19-33)
packages/dapper/src/types/package.ts (1)
packages/thunderstore-api/src/schemas/objectSchemas.ts (1)
PackagePermissions(201-201)
packages/dapper/src/types/methods.ts (1)
packages/dapper/src/types/package.ts (1)
PackagePermissions(60-77)
packages/thunderstore-api/src/get/package.ts (4)
packages/thunderstore-api/src/index.ts (1)
ApiEndpointProps(9-15)packages/thunderstore-api/src/schemas/requestSchemas.ts (1)
PackagePermissionsRequestParams(599-601)packages/thunderstore-api/src/schemas/responseSchemas.ts (2)
PackagePermissionsResponseData(114-116)packagePermissionsResponseDataSchema(112-112)packages/thunderstore-api/src/apiFetch.ts (1)
apiFetch(59-114)
packages/dapper-ts/src/methods/package.ts (2)
packages/dapper-ts/src/index.ts (1)
DapperTsInterface(30-33)packages/thunderstore-api/src/get/package.ts (1)
fetchPackagePermissions(9-25)
packages/thunderstore-api/src/schemas/objectSchemas.ts (2)
packages/dapper/src/types/package.ts (1)
PackagePermissions(60-77)packages/cyberstorm/src/components/Links/LinkingProvider.tsx (1)
ThunderstoreLinkProps(22-28)
packages/thunderstore-api/src/schemas/requestSchemas.ts (1)
packages/cyberstorm/src/components/Links/LinkingProvider.tsx (1)
ThunderstoreLinkProps(22-28)
packages/thunderstore-api/src/schemas/responseSchemas.ts (1)
packages/thunderstore-api/src/schemas/objectSchemas.ts (1)
packagePermissionsSchema(196-199)
packages/dapper-ts/src/index.ts (1)
packages/dapper-ts/src/methods/package.ts (1)
getPackagePermissions(156-174)
apps/cyberstorm-remix/app/p/packageListing.tsx (3)
apps/cyberstorm-remix/app/root.tsx (1)
clientLoader(82-94)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)packages/dapper/src/types/package.ts (1)
PackageListing(4-18)
🪛 ESLint
apps/cyberstorm-remix/app/p/packageListing.tsx
[error] 24-24: 'fetchPackagePermissions' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 172-172: 'permissions' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: ESLint
apps/cyberstorm-remix/app/p/packageListing.tsx
[failure] 24-24: Disallow unused variables
'fetchPackagePermissions' is defined but never used.
[failure] 172-172: Disallow unused variables
'permissions' is assigned a value but never used.
⏰ 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). (1)
- GitHub Check: Generate visual diffs
🔇 Additional comments (10)
packages/thunderstore-api/src/index.ts (1)
25-25: Barrel export for get/package looks goodRe-exporting the new endpoint maintains parity with other GET modules and exposes the fetcher as expected.
packages/dapper/src/dapper.ts (1)
13-13: Interface surface extension is consistent with existing patternsAdding getPackagePermissions to DapperInterface matches the method added in dapper-ts and the types in methods.ts.
packages/dapper-ts/src/index.ts (1)
17-17: getPackagePermissions wiring and typing verified
- packages/dapper-ts/src/index.ts
• Imported at line 17
• Bound in constructor at line 55
• Exposed as public member at line 79- packages/dapper/src/types/methods.ts
•export type GetPackagePermissions = (namespaceId: string, communityId: string, packageName: string) => Promise<PackagePermissions>;(lines 63–67)All imports, bindings, and type definitions are correctly in place. No further changes required.
packages/dapper-ts/src/methods/package.ts (2)
9-10: Import of fetchPackagePermissions is correctMatches the API surface and fits the existing import style in this module.
156-174: Wiring Verified Across All LayersAll checks passed—
getPackagePermissionsis correctly:
- Re-exported in
packages/thunderstore-api/src/index.ts- Declared on the Dapper interface in
packages/dapper/src/dapper.ts- Bound and exported in
packages/dapper-ts/src/index.ts- Consumed by the UI in
apps/cyberstorm-remix/app/p/packageListing.tsxpackages/dapper/src/types/package.ts (1)
60-77: PackagePermissions shape looks good and aligns with the APIThe structure and flag set are clear and map well to permission checks in the UI.
packages/thunderstore-api/src/get/package.ts (1)
9-25: LGTM: authenticated GET with validated response schemauseSession: true is appropriate for permissions; response parsing via packagePermissionsResponseDataSchema ensures runtime safety.
packages/thunderstore-api/src/schemas/responseSchemas.ts (2)
19-20: Importing packagePermissionsSchema here is appropriateKeeps response schema definitions centralized and consistent with objectSchemas.
111-116: Response alias is correct and consistent with schema organizationDelegating to packagePermissionsSchema ensures a single source of truth for the shape.
apps/cyberstorm-remix/app/p/packageListing.tsx (1)
537-539: Permissions-gated header actions are still commented out.The change summary claims header actions are rendered based on permissions, but this block is disabled. Either enable the UI (and its imports) or align the summary and keep permissions out of the component until ready.
Oksamies
force-pushed
the
08-18-add_package_management_tools
branch
from
2225899 to
0439ac9
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
apps/cyberstorm-remix/app/p/packageListing.tsx (1)
23-26: Replace value import with type-safe usage to fix ESLint “unused variable”
fetchPackagePermissionsis only used for typing viatypeofand trips no-unused-vars. Import only what you execute at runtime; for typing, derive the type from DapperTs instead.Apply this diff:
-import { - ApiError, - fetchPackagePermissions, -} from "@thunderstore/thunderstore-api"; +import { ApiError } from "@thunderstore/thunderstore-api";Follow-up: Update type usages to rely on
DapperTs["getPackagePermissions"](see suggested diffs on Lines 286-288 and 151-166).apps/cyberstorm-remix/app/p/packageEdit.tsx (1)
172-175: Remove unusedpermissionsto satisfy ESLint
permissionsis destructured but never used in this component.Apply this diff:
- const { community, listing, team, permissions } = useLoaderData< + const { community, listing, team } = useLoaderData< typeof loader | typeof clientLoader >();
🧹 Nitpick comments (13)
apps/cyberstorm-remix/app/p/packageListing.css (1)
17-17: Prefer design tokens over hardcoded px gapsConsider replacing raw
8pxand6pxgaps with existing spacing tokens (e.g.,var(--gap-xs)/var(--space-8)) to keep spacing consistent and themeable.Also applies to: 43-43
packages/cyberstorm/src/components/Links/Links.tsx (1)
85-88: Guard against missing link mappings at runtimeIf a host app forgets to provide PackageEdit in LinkingProvider, Links[linkId] will be undefined and render will throw cryptically. Consider a clear dev-time assertion.
Apply:
- const Link = Links[linkId]; + const Link = Links[linkId]; + if (!Link) { + throw new Error(`Unknown linkId "${linkId}". Ensure LinkingProvider provides a mapping for it.`); + }apps/cyberstorm-storybook/LinkLibrary.tsx (2)
90-96: PackageEdit mapping added; consider normalizing trailing-slash policyThe new edit URL omits a trailing slash whereas some mappings include it. Consider a consistent convention (either always omit or always include) to avoid accidental double-fetches/redirects when integrating with other environments.
111-131: Wiki mappings added; consider centralizing URL construction and encodingTo avoid future drift across apps and ensure safe path segments, consider:
- A small helper to build package/wiki URLs consistently (and encodeURIComponent per segment).
- Reuse it across Storybook and Remix LinkLibraries.
This keeps slashes and encoding uniform without repeating template strings everywhere.
I can extract a tiny urlBuilder and wire it in both LinkLibraries if you’d like.
apps/cyberstorm-remix/app/p/packageEdit.css (2)
20-22: Scrollbar UX and cross-browser hidingUsing overflow-x: scroll forces a scrollbar even when not needed, and scrollbar-width: none only affects Firefox. Consider auto + WebKit hiding for parity.
Apply:
- overflow-x: scroll; - scrollbar-width: none; + overflow-x: auto; + scrollbar-width: none; /* Firefox */ + } + + /* WebKit scrollbar hide */ + .package-listing__section::-webkit-scrollbar { + display: none;
182-199: Media query range syntax compatibility(width <= 990px) is modern range syntax; older browsers/build chains may not support it consistently. max-width is universally safe.
Apply:
- @media (width <= 990px) { + @media (max-width: 990px) {apps/cyberstorm-remix/app/p/packageListing.tsx (4)
205-208: Remove debug logging
console.log(permissions)should not ship. It can leak user/session state to the console.Apply this diff:
- // const permissions = asd?.permissions; - // console.log(asd); - console.log(permissions); + // const permissions = asd?.permissions;
307-343: Avoid hard-coded Django admin IDs; derive or hide until wired
/djangoadmin/community/packagelisting/206/change/and/djangoadmin/repository/package/16/change/are hard-coded and likely incorrect outside your dev database. These will 404 or misroute for most users.Recommendation:
- Prefer server-provided admin URLs in the permissions payload (e.g., include links in the API response) or derive IDs from
listing/permissions data.- Until available, hide these actions or guard them behind an env flag.
Do you want me to wire these to fields from the permissions response or add a feature flag to hide them until correct?
114-124: Parallelize server loader fetches to reduce latencyThe four independent requests can be fetched concurrently.
Apply this diff:
- return { - community: await dapper.getCommunity(params.communityId), - communityFilters: await dapper.getCommunityFilters(params.communityId), - listing: await dapper.getPackageListingDetails( - params.communityId, - params.namespaceId, - params.packageId - ), - team: await dapper.getTeamDetails(params.namespaceId), - permissions: undefined, - }; + const [community, communityFilters, listing, team] = await Promise.all([ + dapper.getCommunity(params.communityId), + dapper.getCommunityFilters(params.communityId), + dapper.getPackageListingDetails( + params.communityId, + params.namespaceId, + params.packageId + ), + dapper.getTeamDetails(params.namespaceId), + ]); + return { community, communityFilters, listing, team, permissions: undefined };
169-178: Parallelize client loader fetches tooSame optimization applies here; the requests are independent.
Apply this diff:
- return { - community: await dapper.getCommunity(params.communityId), - communityFilters: await dapper.getCommunityFilters(params.communityId), - listing: await dapper.getPackageListingDetails( - params.communityId, - params.namespaceId, - params.packageId - ), - team: await dapper.getTeamDetails(params.namespaceId), - permissions: permissionsPromise, - }; + const [community, communityFilters, listing, team] = await Promise.all([ + dapper.getCommunity(params.communityId), + dapper.getCommunityFilters(params.communityId), + dapper.getPackageListingDetails( + params.communityId, + params.namespaceId, + params.packageId + ), + dapper.getTeamDetails(params.namespaceId), + ]); + return { community, communityFilters, listing, team, permissions: permissionsPromise };apps/cyberstorm-remix/app/p/packageEdit.tsx (3)
150-155: Avoid unnecessary permissions request when logged out
getPackagePermissionsis called unconditionally. Per DapperTs docs, it returnsundefinedon 401, but it still costs a network trip. Gate on login like in packageListing.Apply this diff:
- permissions: await dapper.getPackagePermissions( - params.communityId, - params.namespaceId, - params.packageId - ), + permissions: (await dapper.getCurrentUser()).username + ? await dapper.getPackagePermissions( + params.communityId, + params.namespaceId, + params.packageId + ) + : undefined,
104-124: Parallelize server loader fetches for the edit pageThese requests are independent; fetch concurrently to improve TTFB.
Apply this diff:
- return { - community: await dapper.getCommunity(params.communityId), - communityFilters: await dapper.getCommunityFilters(params.communityId), - listing: await dapper.getPackageListingDetails( - params.communityId, - params.namespaceId, - params.packageId - ), - team: await dapper.getTeamDetails(params.namespaceId), - permissions: undefined, - }; + const [community, communityFilters, listing, team] = await Promise.all([ + dapper.getCommunity(params.communityId), + dapper.getCommunityFilters(params.communityId), + dapper.getPackageListingDetails( + params.communityId, + params.namespaceId, + params.packageId + ), + dapper.getTeamDetails(params.namespaceId), + ]); + return { community, communityFilters, listing, team, permissions: undefined };
142-156: Parallelize client loader fetchesImprove client-side latency by fetching in parallel.
Apply this diff:
- return { - community: await dapper.getCommunity(params.communityId), - communityFilters: await dapper.getCommunityFilters(params.communityId), - listing: await dapper.getPackageListingDetails( - params.communityId, - params.namespaceId, - params.packageId - ), - team: await dapper.getTeamDetails(params.namespaceId), - permissions: await dapper.getPackagePermissions( - params.communityId, - params.namespaceId, - params.packageId - ), - }; + const [community, communityFilters, listing, team, currentUser] = await Promise.all([ + dapper.getCommunity(params.communityId), + dapper.getCommunityFilters(params.communityId), + dapper.getPackageListingDetails( + params.communityId, + params.namespaceId, + params.packageId + ), + dapper.getTeamDetails(params.namespaceId), + dapper.getCurrentUser(), + ]); + return { + community, + communityFilters, + listing, + team, + permissions: currentUser.username + ? await dapper.getPackagePermissions( + params.communityId, + params.namespaceId, + params.packageId + ) + : undefined, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
apps/cyberstorm-remix/app/p/packageEdit.css(1 hunks)apps/cyberstorm-remix/app/p/packageEdit.tsx(1 hunks)apps/cyberstorm-remix/app/p/packageListing.css(1 hunks)apps/cyberstorm-remix/app/p/packageListing.tsx(7 hunks)apps/cyberstorm-remix/app/routes.ts(1 hunks)apps/cyberstorm-remix/app/styles/layout.css(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx(1 hunks)apps/cyberstorm-storybook/LinkLibrary.tsx(2 hunks)packages/cyberstorm/src/components/Links/LinkingProvider.tsx(2 hunks)packages/cyberstorm/src/components/Links/Links.tsx(1 hunks)packages/cyberstorm/src/newComponents/BreadCrumbs/BreadCrumbs.css(1 hunks)packages/dapper-ts/src/index.ts(3 hunks)packages/dapper-ts/src/methods/package.ts(2 hunks)packages/dapper/src/dapper.ts(1 hunks)packages/dapper/src/types/methods.ts(2 hunks)packages/dapper/src/types/package.ts(1 hunks)packages/thunderstore-api/src/get/package.ts(1 hunks)packages/thunderstore-api/src/index.ts(1 hunks)packages/thunderstore-api/src/schemas/objectSchemas.ts(1 hunks)packages/thunderstore-api/src/schemas/requestSchemas.ts(1 hunks)packages/thunderstore-api/src/schemas/responseSchemas.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/thunderstore-api/src/index.ts
- packages/dapper/src/types/methods.ts
- packages/dapper/src/dapper.ts
- packages/thunderstore-api/src/get/package.ts
- packages/dapper/src/types/package.ts
- packages/thunderstore-api/src/schemas/responseSchemas.ts
- packages/thunderstore-api/src/schemas/objectSchemas.ts
- packages/dapper-ts/src/methods/package.ts
- packages/dapper-ts/src/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/cyberstorm/src/newComponents/BreadCrumbs/BreadCrumbs.css (1)
packages/cyberstorm/src/components/BreadCrumbs/BreadCrumbs.tsx (1)
BreadCrumbs(14-55)
apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx (1)
apps/cyberstorm-storybook/LinkLibrary.tsx (1)
Link(28-59)
packages/thunderstore-api/src/schemas/requestSchemas.ts (1)
packages/cyberstorm/src/components/Layout/PackageDetailLayout/PackageDetailLayout.tsx (2)
Props(41-45)PackageDetailLayout(47-208)
apps/cyberstorm-remix/app/p/packageEdit.tsx (8)
packages/dapper-ts/src/index.ts (1)
DapperTs(35-86)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)apps/cyberstorm-remix/app/root.tsx (1)
OutletContextShape(65-70)packages/cyberstorm/src/components/RelativeTime/RelativeTime.tsx (1)
RelativeTime(16-28)packages/cyberstorm/src/utils/utils.ts (2)
formatInteger(6-11)formatFileSize(17-31)apps/cyberstorm-remix/app/commonComponents/CopyButton/CopyButton.tsx (1)
CopyButton(12-48)apps/cyberstorm-remix/app/commonComponents/PageHeader/PageHeader.tsx (1)
PageHeader(17-81)packages/cyberstorm/src/svg/svg.tsx (1)
ThunderstoreLogo(1-11)
apps/cyberstorm-storybook/LinkLibrary.tsx (2)
apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx (1)
Link(18-58)packages/cyberstorm/src/index.ts (1)
Link(49-49)
apps/cyberstorm-remix/app/p/packageListing.tsx (4)
apps/cyberstorm-remix/app/p/packageEdit.tsx (3)
clientLoader(137-166)PackageListing(168-704)loader(104-134)apps/cyberstorm-remix/app/root.tsx (1)
clientLoader(82-94)packages/dapper-ts/src/methods/package.ts (1)
getPackagePermissions(157-184)packages/thunderstore-api/src/get/package.ts (1)
fetchPackagePermissions(9-27)
🪛 GitHub Check: ESLint
apps/cyberstorm-remix/app/p/packageEdit.tsx
[failure] 24-24: Disallow unused variables
'fetchPackagePermissions' is defined but never used.
[failure] 172-172: Disallow unused variables
'permissions' is assigned a value but never used.
🪛 ESLint
apps/cyberstorm-remix/app/p/packageEdit.tsx
[error] 24-24: 'fetchPackagePermissions' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 172-172: 'permissions' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🪛 Biome (2.1.2)
apps/cyberstorm-remix/app/p/packageListing.css
[error] 19-19: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
align-items is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
⏰ 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: CodeQL
🔇 Additional comments (8)
packages/thunderstore-api/src/schemas/requestSchemas.ts (1)
592-601: PackagePermissionsRequestParams schema and integration verifiedI’ve confirmed that:
fetchPackagePermissionsinpackages/thunderstore-api/src/get/package.tsusesApiEndpointProps<PackagePermissionsRequestParams, object, object>.- The endpoint is properly exported in
src/get/package.tsand re-exported inpackages/thunderstore-api/src/index.ts.Schema naming, field order, and end-to-end wiring are all correct—no further changes needed.
apps/cyberstorm-remix/app/p/packageListing.css (1)
25-33: New management tools layout reads wellThe actions column and the “island” affordance are clear and consistent with the existing design token usage.
Also applies to: 35-40, 41-50
packages/cyberstorm/src/newComponents/BreadCrumbs/BreadCrumbs.css (1)
5-5: LGTM: full-width breadcrumbsSetting
width: 100%aligns with the layout changes and works with the existing overflow handling.apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx (1)
85-91: Add PackageEdit link mapping — verified Remix route & Link type
- Route defined in
apps/cyberstorm-remix/app/routes.ts:
route("edit", "p/packageEdit.tsx")- Component file exists at
apps/cyberstorm-remix/app/p/packageEdit.tsxPackageEditincluded in shared Link types (Links.tsx&LinkingProvider.tsx)LGTM!
packages/cyberstorm/src/components/Links/Links.tsx (1)
31-31: All LinkLibrary implementations include PackageEdit
Verified that thePackageEditlink is declared and implemented in:
- packages/cyberstorm/src/components/Links/LinkingProvider.tsx
LinkLibraryinterface (line 86)- default
libraryno-op implementation (line 154)- apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx (line 85)
- apps/cyberstorm-storybook/LinkLibrary.tsx (line 90)
Ready to merge.
packages/cyberstorm/src/components/Links/LinkingProvider.tsx (2)
85-87: Interface surface extended with PackageEditAdding PackageEdit to LinkLibrary, with proper docs and signature, is aligned with existing link types.
154-154: Default library includes a no-op for PackageEditGood defensive default to avoid runtime crashes in environments that haven’t provided a mapping.
apps/cyberstorm-remix/app/p/packageListing.tsx (1)
531-541: Nice use of Suspense/Await with a conditional permissions fetchThis pattern cleanly defers rendering management tools until the permissions promise resolves and avoids a network call when the user is logged out.
| .package-listing__package-section { | ||
| display: flex; | ||
| flex: 1 0 0; | ||
| flex-direction: column; | ||
| gap: 8px; | ||
| align-items: center; | ||
| align-items: flex-start; | ||
| align-self: stretch; | ||
| justify-content: flex-start; | ||
| padding: 120px 48px 32px; | ||
| } |
There was a problem hiding this comment.
Fix duplicate align-items declaration (linter error)
There are two align-items rules in the same block; the first is redundant and triggers Biome’s noDuplicateProperties error. Keep the intended final value.
.package-listing__package-section {
display: flex;
flex: 1 0 0;
flex-direction: column;
gap: 8px;
- align-items: center;
align-items: flex-start;
align-self: stretch;
justify-content: flex-start;
padding: 120px 48px 32px;
}🧰 Tools
🪛 Biome (2.1.2)
[error] 19-19: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
align-items is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/packageListing.css around lines 13 to 23 there
are two align-items declarations in the same rule (align-items: center and
align-items: flex-start) which triggers the linter noDuplicateProperties error;
remove the redundant one and keep the intended final value (align-items:
flex-start) so only a single align-items remains in that block.
| import { OutletContextShape } from "~/root"; | ||
| import { CopyButton } from "~/commonComponents/CopyButton/CopyButton"; | ||
| import { getSessionTools } from "~/middlewares"; | ||
| import { getPackagePermissions } from "@thunderstore/dapper-ts/src/methods/package"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t import from package internals (src)
Importing getPackagePermissions from @thunderstore/dapper-ts/src/... reaches into internals that won’t exist in the published package and may break builds. Use the public API/types instead (e.g., DapperTs["getPackagePermissions"] for types).
Apply this diff:
-import { getPackagePermissions } from "@thunderstore/dapper-ts/src/methods/package";See type replacement suggestions on Lines 286-288 and 151-166.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/packageListing.tsx around line 56, remove the
deep import from @thunderstore/dapper-ts/src/methods/package and instead use the
package's public exports and types (do not reference `src`). Replace the direct
function import with either the public exported function from
`@thunderstore/dapper-ts` (if available) or use the exported type accessor
`DapperTs["getPackagePermissions"]` for typing; update the type usages suggested
on lines 151-166 and 286-288 to use the public type accessor or exported types
from the package root. Ensure no path contains `/src/` and adjust any call sites
or type annotations accordingly so the code compiles with the published package
API.
| const tools = getSessionTools(context); | ||
| const dapper = new DapperTs(() => { | ||
| return { | ||
| apiHost: tools?.getConfig().apiHost, | ||
| sessionId: tools?.getConfig().sessionId, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Guard missing session tools and avoid optional chaining in config
getSessionTools(context) may return undefined. You call tools.getSessionCurrentUser() below, which will throw if tools is falsy. Add the same guard as in root and avoid passing undefined into Dapper config.
Apply this diff:
- const tools = getSessionTools(context);
+ const tools = getSessionTools(context);
+ if (!tools) {
+ throw new Error("Session tools not found in context");
+ }
const dapper = new DapperTs(() => {
- return {
- apiHost: tools?.getConfig().apiHost,
- sessionId: tools?.getConfig().sessionId,
- };
+ const cfg = tools.getConfig();
+ return {
+ apiHost: cfg.apiHost,
+ sessionId: cfg.sessionId,
+ };
});📝 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.
| const tools = getSessionTools(context); | |
| const dapper = new DapperTs(() => { | |
| return { | |
| apiHost: tools?.getConfig().apiHost, | |
| sessionId: tools?.getConfig().sessionId, | |
| }; | |
| }); | |
| const tools = getSessionTools(context); | |
| if (!tools) { | |
| throw new Error("Session tools not found in context"); | |
| } | |
| const dapper = new DapperTs(() => { | |
| const cfg = tools.getConfig(); | |
| return { | |
| apiHost: cfg.apiHost, | |
| sessionId: cfg.sessionId, | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/packageListing.tsx around lines 142-148, you
currently call getSessionTools(context) and then pass tools?.getConfig() into
the DapperTs constructor and later call tools.getSessionCurrentUser(), which
will throw if tools is undefined; add the same guard used in root: check if
(!tools) and handle the missing session tools (return early/redirect or throw a
clear error) before creating DapperTs, then build the config from
tools.getConfig() without optional chaining (e.g. const cfg = tools.getConfig();
use cfg.apiHost and cfg.sessionId) so you never pass undefined into DapperTs or
call methods on a falsy tools object.
apps/cyberstorm-remix/app/routes.ts
Outdated
| route("edit", "p/packageEdit.tsx"), | ||
| ]), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New nested edit route is well placed and consistent
Route("/c/:communityId/p/:namespaceId/:packageId/.../edit") matches the new linking API and keeps parity with other tab-like routes.
Please verify the edit page enforces permissions server-side (so direct navigation is gated). For example, loader/action should 403/redirect when unauthorized.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="apps/cyberstorm-remix/app/p/packageEdit.tsx"
if [[ -f "$target" ]]; then
echo "Found $target"
echo
echo "Loader/action/clientLoader presence:"
rg -n -C3 '\b(loader|clientLoader|action)\b' "$target" || true
echo
echo "Permission gating indicators (adjust to your naming):"
rg -n -C2 -e 'permission|authorize|forbid|403|401|redirect' "$target" || true
else
echo "Missing page file: $target"
exit 1
fiLength of output: 2635
Implement Server-Side Authorization in the Edit Route
The loader in apps/cyberstorm-remix/app/p/packageEdit.tsx currently returns permissions: undefined (lines 120–122) and carries a “TODO” without enforcing any access checks. Likewise, there’s no authorization in the action, and the client-side UI gating is merely commented out (lines 537–539). As is, unauthorized users can navigate directly to this page.
Please add permission checks that throw a 403 or perform a redirect for unauthorized access in:
- Loader (lines 104–122): verify user rights before returning data.
- Action (if implemented): enforce the same checks on mutations.
- Remove/update the commented UI gating (lines 537–539) once server-side checks are in place.
This must be fixed before merging.
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/packageEdit.tsx around lines 104–122, the loader
currently returns permissions: undefined and lacks enforcement — update the
loader to fetch the current user/session and verify the required permission for
editing (if unauthorized, throw new Response("Forbidden", { status: 403 }) or
redirect to a safe page); apply the same authorization check at the start of the
action handler so mutations are protected server-side; then remove or update the
client-side commented UI gating at lines 537–539 since server-side checks now
enforce access, and ensure any returned loader data only includes permissions
when authorized.
| /* .layout__content { | ||
| gap: var(--gap-sm); | ||
| align-self: flex-start; | ||
| justify-content: flex-start; | ||
| max-width: var(--nimbus-layout-content-max-width, 90rem); | ||
| padding: var(--space-16); | ||
| } | ||
| } */ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove commented-out .layout__content block or restore intentionally
Shipping commented CSS adds noise and can conceal regressions if the class is still referenced. If this rule is no longer used, delete it; if it is needed, keep it active and adjust as required.
Proposed cleanup:
- /* .layout__content {
- gap: var(--gap-sm);
- align-self: flex-start;
- justify-content: flex-start;
- max-width: var(--nimbus-layout-content-max-width, 90rem);
- padding: var(--space-16);
- } */To verify there are no lingering references to .layout__content, run:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Searching for .layout__content usage across repo..."
rg -n -C2 'layout__content'Length of output: 6171
Restore .layout__content rule—it’s still in use across the app
The .layout__content selector is referenced in dozens of components (e.g., packageListing.tsx, packageEdit.tsx, markdownPreview.tsx, Dependants.tsx, etc.). Removing the CSS will break layouts wherever that class is applied.
• Keep the rule active (or remove all usage site-wide, which is more work)
• To restore, simply uncomment in apps/cyberstorm-remix/app/styles/layout.css:
- /* .layout__content {
- gap: var(--gap-sm);
- align-self: flex-start;
- justify-content: flex-start;
- max-width: var(--nimbus-layout-content-max-width, 90rem);
- padding: var(--space-16);
- } */
+ .layout__content {
+ gap: var(--gap-sm);
+ align-self: flex-start;
+ justify-content: flex-start;
+ max-width: var(--nimbus-layout-content-max-width, 90rem);
+ padding: var(--space-16);
+ }📝 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.
| /* .layout__content { | |
| gap: var(--gap-sm); | |
| align-self: flex-start; | |
| justify-content: flex-start; | |
| max-width: var(--nimbus-layout-content-max-width, 90rem); | |
| padding: var(--space-16); | |
| } | |
| } */ | |
| .layout__content { | |
| gap: var(--gap-sm); | |
| align-self: flex-start; | |
| justify-content: flex-start; | |
| max-width: var(--nimbus-layout-content-max-width, 90rem); | |
| padding: var(--space-16); | |
| } |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/styles/layout.css around lines 20 to 26, the
.layout__content rule was commented out but is still referenced across the app
and removing it breaks layouts; restore the rule by uncommenting the block so
the CSS properties (gap, align-self, justify-content, max-width and padding) are
applied again, or if you prefer a safer change add the same declarations into an
active rule with the .layout__content selector to preserve existing layout
behavior across components.
0439ac9 to
4a5e0d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
111-121: Fix stale return: returning submitOutput after setState is unreliable
submitreturns the state variablesubmitOutputimmediately aftersetSubmitOutput(output). Because state updates are async, callers can get a stale or undefined value even when the submission succeeded.Refactor to return the local
outputand avoid mixingawaitwith.then/.catch:- setSubmitting(true); - try { - await props - .submitor(submissionData) - .then((output) => { - setSubmitOutput(output); - if (props.onSubmitSuccess) { - props.onSubmitSuccess(output); - } - }) - .catch((error) => { + setSubmitting(true); + try { + const output = await props.submitor(submissionData); + setSubmitOutput(output); + if (props.onSubmitSuccess) { + props.onSubmitSuccess(output); + } + return output; + } catch (error) { + // Keep current semantics: field/query parse errors set state but don't toast, + // ParseError bubbles to outer onSubmitError via rethrow of the friendly error. + if (error instanceof RequestBodyParseError) { + setSubmitError( + new Error("Some of the field values are invalid") as SubmissionError + ); + setInputErrors(error.error.formErrors as InputErrors); + return undefined; + } else if (error instanceof RequestQueryParamsParseError) { + setSubmitError( + new Error("Some of the query parameters are invalid") as SubmissionError + ); + setInputErrors(error.error.formErrors as InputErrors); + return undefined; + } else if (error instanceof ParseError) { + const friendly = new Error( + "Request succeeded, but the response was invalid" + ) as SubmissionError; + setSubmitError(friendly); + setInputErrors(error.error.formErrors as InputErrors); + throw friendly; + } else { throw error; - }); - return submitOutput; - } catch (error) { - if (props.onSubmitError) { - props.onSubmitError(error as SubmissionError); } - throw error; - } finally { + } catch (error) { + if (props.onSubmitError) { + props.onSubmitError(error as SubmissionError); + } + throw error; + } finally { setSubmitting(false); - } + }Also applies to: 148-156
packages/thunderstore-api/src/schemas/responseSchemas.ts (1)
170-174: Wrong type alias for PackageWikiPageCreateResponseDataThe create response type infers from the edit schema. This is a copy/paste bug that breaks types for callers.
export const packageWikiPageCreateResponseDataSchema = packageWikiPageSchema; export type PackageWikiPageCreateResponseData = z.infer< - typeof packageWikiPageEditResponseDataSchema + typeof packageWikiPageCreateResponseDataSchema >;
♻️ Duplicate comments (6)
apps/cyberstorm-remix/app/p/packageListing.css (1)
48-50: Fix duplicate align-items declaration (Biome error)Redundant
align-itemswithin the same rule; keep the intended final value..package-listing__package-section { position: relative; display: flex; flex: 1 0 0; flex-direction: column; gap: 8px; - align-items: center; align-items: flex-start; align-self: stretch; justify-content: flex-start; padding: 7.5rem 3rem 2rem; }apps/cyberstorm-remix/app/p/packageListing.tsx (5)
28-28: Unused import: fetchPackagePermissionsThe
fetchPackagePermissionsfunction is imported but never used in the code.
62-62: Don't import from package internals (src)Importing
getPackagePermissionsfrom@thunderstore/dapper-ts/src/...reaches into internals that won't exist in the published package and may break builds. Use the public API instead.
148-198: Guard missing session tools and avoid Promise.withResolversMultiple issues in the clientLoader:
getSessionTools(context)may return undefined, but you call methods ontoolswithout proper guardsPromise.withResolvershas limited browser support and is unnecessary here- Optional chaining in config may pass undefined to DapperTs
203-203: Unused variable: permissionsThe
permissionsvariable is assigned from useLoaderData but never used directly in the component.
276-278: Type the permissions param via the public DapperTs APIUse the public API for typing instead of relying on internal function imports.
🧹 Nitpick comments (12)
apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
137-145: Rethrow a “friendly” error instead of the raw ParseErrorGood call to propagate ParseError up the stack. However, you set a user-facing submitError but rethrow the original ParseError, so onSubmitError receives a different shape/message than the one stored in state. Prefer rethrowing the same “friendly” error to keep UI state and callbacks consistent.
Apply one of the following:
Minimal change:
- } else if (error instanceof ParseError) { - setSubmitError( - new Error( - "Request succeeded, but the response was invalid" - ) as SubmissionError - ); - setInputErrors(error.error.formErrors as InputErrors); - throw error; + } else if (error instanceof ParseError) { + const friendly = new Error( + "Request succeeded, but the response was invalid" + ) as SubmissionError; + setSubmitError(friendly); + setInputErrors(error.error.formErrors as InputErrors); + throw friendly; } else {Or, adopt the broader refactor in the next comment to avoid mixing
awaitwith.then/.catch.apps/cyberstorm-remix/app/p/packageListing.css (3)
2-6: z-index without positioning is inert
.package-listinggetsz-index: 1but isn’t positioned. Unless a parent creates a stacking context, this won’t take effect..package-listing { --nimbus-layout-content-max-width: 120rem; - z-index: 1; + position: relative; + z-index: 1; }
8-16: Absolutely positioned background lacks anchoring; consider filling container
.package-community__backgroundisposition: absolutewith no inset/offsets or width, which can lead to unpredictable layout depending on content. The tint uses full width; mirror that for the background container for consistent layering..package-community__background { - position: absolute; + position: absolute; + inset: 0 auto auto 0; /* anchor top-left; adjust as needed */ display: flex; flex-direction: column; align-items: flex-start; height: 20rem; border-radius: var(--section-border-radius); overflow: hidden; } .package-community__background-tint { position: absolute; - width: 100%; + inset: 0 0 auto 0; /* full width */ height: 20rem; background: linear-gradient(180deg, rgb(16 16 40 / 0.4) 0%, #101028 85.94%); }Also applies to: 28-33
113-122: Hide horizontal scrollbar cross-browserFirefox-only
scrollbar-width: nonewon’t affect Chromium/WebKit. Add the common patterns for consistent UI..package-listing__package-content-section { display: flex; flex: 1 0 0; flex-direction: column; gap: var(--gap-xl); align-items: flex-start; padding-top: 1rem; overflow-x: scroll; - scrollbar-width: none; + scrollbar-width: none; /* Firefox */ + -ms-overflow-style: none; /* IE/Edge legacy */ +} +.package-listing__package-content-section::-webkit-scrollbar { + display: none; /* Chrome/Safari/Edge */ }apps/cyberstorm-remix/app/c/community.tsx (4)
82-84: Avoid mutating API response data when sorting sections
filters.sections.sort(...)mutates the original array, which can cause subtle bugs iffiltersis reused. Prefer sorting a copy.-const sortedSections = filters.sections.sort( +const sortedSections = [...filters.sections].sort( (a, b) => b.priority - a.priority );Apply in both loader and clientLoader.
Also applies to: 139-141
94-95: Sanitize page query param to avoid NaN/0 from empty strings
Number(page)will yield0for""andNaNfor invalid values, which may break pagination. Normalize to a positive integer or undefined.- page === null ? undefined : Number(page), + page && /^\d+$/.test(page) ? Number(page) : undefined,Also applies to: 151-152
96-98: Guard category filters against empty strings
includedCategories?.split(",")(and excluded) yields[""]when the param is present but empty. Filter falsy entries.- includedCategories?.split(",") ?? undefined, - excludedCategories?.split(",") ?? undefined, + includedCategories ? includedCategories.split(",").filter(Boolean) : undefined, + excludedCategories ? excludedCategories.split(",").filter(Boolean) : undefined,Also applies to: 153-155
247-254: Defensive access to outlet context for PackageSearch configIf the outlet context isn’t provided for some edge routes, this will throw. Optional-chain or provide a safe fallback.
- config={outletContext.requestConfig} + config={outletContext?.requestConfig}Optionally assert the shape at the route boundary to fail early with a clearer error.
apps/cyberstorm-remix/app/c/Community.css (1)
34-39: Make the tint overlay non-interactive and stack correctlyMinor, but prevents the overlay from capturing events and aligns with the background’s stacking.
.community__background-tint { position: absolute; width: 100%; height: 20rem; background: linear-gradient(180deg, rgb(16 16 40 / 0.4) 0%, #101028 85.94%); + inset: 0 0 auto 0; + pointer-events: none; }apps/cyberstorm-remix/app/p/packageEdit.css (2)
27-40: Improve small-screen responsiveness for row layoutThe fixed 5rem gap and horizontal row can overflow on narrow viewports. Add a breakpoint to stack content and relax the fixed info width.
.package-edit__row { display: flex; flex: 1 1 0; gap: 5rem; align-items: flex-start; justify-content: space-between; } .package-edit__row-content { display: flex; flex: 1 1 0; gap: var(--gap-md); } + +@media (width <= 48rem) { + .package-edit__row { + flex-direction: column; + gap: var(--gap-lg); + } + .package-edit__row-content { + flex-direction: column; + } +}
41-49: Fixed 21rem info sidebar can cause overflow on small screensAllow the info block to shrink on narrow viewports while keeping the intended width on larger screens.
.package-edit__info { display: flex; flex-direction: column; gap: 0.5rem; align-items: flex-start; align-self: stretch; - width: 21rem; + width: 21rem; + max-width: 100%; }apps/cyberstorm-remix/app/p/packageEdit.tsx (1)
407-409: Implement Cancel button functionalityThe Cancel button currently has no onClick handler. Consider adding navigation back to the package page or resetting form state.
-<NewButton csVariant="secondary" csSize="big"> +<NewButton + csVariant="secondary" + csSize="big" + primitiveType="cyberstormLink" + linkId="Package" + community={community.identifier} + namespace={listing.namespace} + package={listing.name} +> Cancel </NewButton>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
apps/cyberstorm-remix/app/c/Community.css(1 hunks)apps/cyberstorm-remix/app/c/community.tsx(2 hunks)apps/cyberstorm-remix/app/p/packageEdit.css(1 hunks)apps/cyberstorm-remix/app/p/packageEdit.tsx(1 hunks)apps/cyberstorm-remix/app/p/packageListing.css(2 hunks)apps/cyberstorm-remix/app/p/packageListing.tsx(8 hunks)apps/cyberstorm-remix/app/routes.ts(1 hunks)apps/cyberstorm-remix/app/styles/layout.css(0 hunks)apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts(1 hunks)apps/cyberstorm-storybook/LinkLibrary.tsx(2 hunks)packages/cyberstorm/src/components/Links/LinkingProvider.tsx(2 hunks)packages/cyberstorm/src/components/Links/Links.tsx(1 hunks)packages/cyberstorm/src/newComponents/BreadCrumbs/BreadCrumbs.css(1 hunks)packages/dapper-ts/src/index.ts(3 hunks)packages/dapper-ts/src/methods/package.ts(2 hunks)packages/dapper/src/dapper.ts(1 hunks)packages/dapper/src/types/methods.ts(2 hunks)packages/dapper/src/types/package.ts(1 hunks)packages/thunderstore-api/src/get/package.ts(1 hunks)packages/thunderstore-api/src/index.ts(1 hunks)packages/thunderstore-api/src/post/package.ts(3 hunks)packages/thunderstore-api/src/post/packageListing.ts(1 hunks)packages/thunderstore-api/src/schemas/objectSchemas.ts(1 hunks)packages/thunderstore-api/src/schemas/requestSchemas.ts(2 hunks)packages/thunderstore-api/src/schemas/responseSchemas.ts(4 hunks)
💤 Files with no reviewable changes (1)
- apps/cyberstorm-remix/app/styles/layout.css
✅ Files skipped from review due to trivial changes (2)
- packages/cyberstorm/src/components/Links/Links.tsx
- packages/cyberstorm/src/newComponents/BreadCrumbs/BreadCrumbs.css
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/thunderstore-api/src/index.ts
- packages/dapper/src/dapper.ts
- packages/cyberstorm/src/components/Links/LinkingProvider.tsx
- packages/thunderstore-api/src/schemas/objectSchemas.ts
- packages/dapper-ts/src/methods/package.ts
- packages/thunderstore-api/src/get/package.ts
- packages/dapper/src/types/package.ts
- packages/dapper-ts/src/index.ts
- apps/cyberstorm-remix/app/routes.ts
- packages/dapper/src/types/methods.ts
- apps/cyberstorm-storybook/LinkLibrary.tsx
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/thunderstore-api/src/post/package.ts (4)
packages/thunderstore-api/src/index.ts (1)
ApiEndpointProps(9-15)packages/thunderstore-api/src/schemas/requestSchemas.ts (3)
PackageUnlistRequestParams(315-317)PackageUnlistRequestData(323-325)packageUnlistRequestDataSchema(319-321)packages/thunderstore-api/src/schemas/responseSchemas.ts (2)
PackageUnlistResponseData(231-233)packageUnlistResponseDataSchema(227-229)packages/thunderstore-api/src/apiFetch.ts (1)
apiFetch(59-114)
apps/cyberstorm-remix/app/p/packageEdit.tsx (8)
packages/dapper-ts/src/index.ts (1)
DapperTs(35-86)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)packages/dapper/src/types/package.ts (1)
PackageListing(4-18)apps/cyberstorm-remix/app/root.tsx (1)
OutletContextShape(65-70)packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
useToast(79-87)packages/thunderstore-api/src/post/package.ts (2)
packageDeprecate(50-75)packageUnlist(77-102)packages/thunderstore-api/src/post/packageListing.ts (1)
packageListingUpdate(19-44)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
useStrongForm(24-169)
apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx (2)
apps/cyberstorm-storybook/LinkLibrary.tsx (1)
Link(28-59)packages/cyberstorm/src/index.ts (1)
Link(49-49)
packages/thunderstore-api/src/schemas/responseSchemas.ts (1)
packages/thunderstore-api/src/schemas/objectSchemas.ts (1)
packagePermissionsSchema(196-199)
apps/cyberstorm-remix/app/p/packageListing.tsx (8)
apps/cyberstorm-remix/app/root.tsx (1)
clientLoader(82-94)packages/dapper-ts/src/index.ts (1)
DapperTs(35-86)packages/dapper-ts/src/methods/package.ts (1)
getPackagePermissions(157-184)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
useToast(79-87)packages/thunderstore-api/src/get/package.ts (1)
fetchPackagePermissions(9-27)packages/ts-api-react-actions/src/ApiAction.tsx (1)
ApiAction(18-52)packages/thunderstore-api/src/post/packageListing.ts (2)
packageListingReject(73-98)packageListingApprove(46-71)
apps/cyberstorm-remix/app/c/community.tsx (1)
apps/cyberstorm-remix/app/commonComponents/PageHeader/PageHeader.tsx (1)
PageHeader(17-81)
🪛 Biome (2.1.2)
apps/cyberstorm-remix/app/p/packageListing.css
[error] 49-49: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
align-items is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
⏰ 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: CodeQL
🔇 Additional comments (13)
apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
48-79: Ensureprops.refineris included in the effect dependencies or stabilizedThe
useEffectis currently only keyed onprops.inputs, so ifprops.refinerever changes identity (for example, defined inline), the effect won’t re-run and may call a stale function. To address this:• Add all props used inside the effect to the dependency array—at minimum
useEffect(() => { … }, [props.inputs, props.refiner, props.onRefineSuccess, props.onRefineError]);if those callbacks are guaranteed to be stable.
• Or require callers to wraprefiner(and any callback props) inuseCallbackso their identity never changes.Please review your
useStrongFormcall sites to see whether any inline or unstable refiners are being passed and update accordingly.packages/thunderstore-api/src/schemas/requestSchemas.ts (2)
308-326: New unlist request schemas look good
packageUnlistRequestParamsSchemaandpackageUnlistRequestDataSchemafollow existing conventions (community/namespace/package + simple command payload). No issues spotted.
611-620: Clarify Parameter Naming in Package Permissions Schema
The snake_cased properties (community_id,namespace_id,package_name) are used consistently at the fetch and call sites, but they diverge from other endpoints that expectcommunity,namespace, andpackage. This mismatch can lead to subtle bugs when wiring up new calls.• Add JSDoc on each field to specify whether it’s a slug, UUID, or human-readable name.
• Or introduce a small adapter in your fetch layer to normalize betweencommunity→community_id,namespace→namespace_id,package→package_name.Affected location:
- packages/thunderstore-api/src/schemas/requestSchemas.ts (packagePermissionsRequestParamsSchema, lines 612–615)
packages/thunderstore-api/src/post/packageListing.ts (1)
80-83: Reject endpoint integration verified — no stale/approve/references foundAll checks confirm the
packageListingRejectflow is self-contained and correctly aligned:
- The endpoint path is updated to
/reject/inpackages/thunderstore-api/src/post/packageListing.tsand no stray/approve/occurrences remain in the reject flow.responseSchemais explicitly set toundefined, so consumers won’t attempt to parse a response body.- The request schemas in
packages/thunderstore-api/src/schemas/requestSchemas.ts(lines 401–420) correctly define only the reject parameters and data.- The Remix app at
apps/cyberstorm-remix/app/p/packageListing.tsxusespackageListingRejectwithout expecting any returned payload.These changes look correct. Please ensure the backend implements the matching
/api/cyberstorm/listing/.../reject/endpoint and returns the appropriate status with no body.apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx (1)
85-91: Resolved: PackageEdit link correctly omits trailing slashConfirmed that the route is defined as
route("/c/:communityId/p/:namespaceId/:packageId/edit", "p/packageEdit.tsx")in apps/cyberstorm-remix/app/routes.ts, without a trailing slash. The LinkLibrary.tsx URL (
/c/${p.community}/p/${p.namespace}/${p.package}/edit) therefore matches exactly and requires no change.packages/thunderstore-api/src/post/package.ts (1)
85-96: Verify unlist endpoint path; it deviates from existing API conventionsOther endpoints here use
/api/cyberstorm/package/.../<action>/. Unlist uses/c/${community}/${namespace}/${package}/(no/apiand no/unlist/). If the backend expects the HTML page path, JSON parsing will fail; if it expects a dedicated API route, this path is wrong. Please confirm the correct endpoint and align with the existing pattern if applicable.Here’s a proposed change if the API route should match the others:
-const path = `/c/${params.community}/${params.namespace}/${params.package}/`; +const path = `/api/cyberstorm/package/${params.namespace}/${params.package}/unlist/`;If the current path is intentional (form-style POST with
unlistaction), consider adding a code comment documenting why this differs from other endpoints.packages/thunderstore-api/src/schemas/responseSchemas.ts (2)
235-243: Confirm UI handles the deprecation response messageI’ve verified that the API client now returns an object with a
message: stringfor the deprecate endpoint, so please ensure your UI layer is updated accordingly:
- In packages/thunderstore-api/src/schemas/responseSchemas.ts (lines 236–238),
packageDeprecateResponseDataSchemais defined as{ message: string }instead of a boolean.- In packages/thunderstore-api/src/post/package.ts (around line 73), the
responseSchemaforpackageDeprecateis set topackageDeprecateResponseDataSchema, wiring the new schema into the endpoint.- Next steps: review any UI code that consumes
packageDeprecate—it must access and displayresponse.messagerather than rely on truthiness.
245-252: Verify and update all consumers of categories after schema changeThe
packageListingUpdateResponseDataSchemanow returns categories as an array of{ slug: string; name: string }instead ofstring[]. Any code that treatscategoriesasstring[]—whether in your API client, form state, or UI components—will break. At a minimum, review and adjust the following call sites:• packages/dapper/src/types/methods.ts (around line 74):
–categories?: string[]
→ update to matchPackageCategory[]or map to/from slug as needed• packages/dapper-ts/src/methods/package.ts (around line 121):
– method signature still usescategories?: string[]
→ align with{ slug, name }[]or map immediately on receipt• apps/cyberstorm-remix/app/p/packageEdit.tsx (lines 178–187 & 192–196):
– initial form state and submit payload assumestring[]vialisting.categories.map(c => c.slug)
→ ensure you’re extractingslugfrom each object in the update response• apps/cyberstorm-remix/app/upload/upload.tsx (lines 279 & 316–318):
–data.categoriesis fed directly into form state
→ map objects to slugs before callingupdateFormFieldState• UI components rendering
categories(e.g. CardPackage.tsx at lines 56–59, PackageSearch and its helpers):
– verify they consumec.slug/c.namerather than treatingcas a string• Tests & object schemas (e.g.
packages/thunderstore-api/src/schemas/objectSchemas.ts, communityFilters tests):
– ensure any mocked or assertedcategoriespayloads now use object arraysPlease audit any other occurrences of
categoriesto confirm no lingeringstring[]assumptions remain.apps/cyberstorm-remix/app/p/packageListing.tsx (2)
797-933: LGTM! Well-structured modal componentThe ReviewPackageForm component is well-implemented with proper error handling, toast notifications, and clean separation of concerns. The ApiAction integration works correctly for both approve and reject flows.
534-795: Excellent UI restructuring with permission-driven behaviorThe new layout structure with hero background, Suspense/Await workflow for permissions, and comprehensive navigation tabs is well-executed. The permission-driven rendering ensures users only see actions they're authorized to perform.
apps/cyberstorm-remix/app/p/packageEdit.tsx (3)
126-165: Well-implemented API action handlersThe deprecateToggleAction and unlistAction implementations are excellent - proper error handling, user feedback through toasts, and automatic revalidation to update the UI state.
202-226: Excellent use of useStrongForm for typed form handlingThe integration of useStrongForm with proper TypeScript generics and comprehensive error handling is well-executed. The success/error toast integration provides good user feedback.
274-365: Permission-driven UI sections are well-structuredThe conditional rendering based on permissions (
can_unlist,can_manage_deprecation) properly gates sensitive operations. The UI clearly communicates the current state and consequences of actions.
| .community { | ||
| --nimbus-layout-content-max-width: 120rem; | ||
| } | ||
|
|
||
| .community__heading { | ||
| z-index: 1; | ||
| display: flex; | ||
| gap: var(--gap-xl); | ||
| align-items: center; | ||
| flex-direction: column; | ||
| gap: 1rem; | ||
| align-items: flex-start; | ||
| align-self: stretch; | ||
| padding: var(--space-8) 0 var(--space-20); | ||
| padding: 7.5rem 3rem 2rem; | ||
| } | ||
|
|
There was a problem hiding this comment.
Anchor the absolute hero background to the community container
Add position: relative (and consider a stacking context) so the absolutely positioned background is scoped to .community. This also helps ensure border radius and overflow apply as intended.
.community {
--nimbus-layout-content-max-width: 120rem;
z-index: 1;
+ position: relative;
display: flex;
flex-direction: column;
gap: 1rem;
align-items: flex-start;
align-self: stretch;
padding: 7.5rem 3rem 2rem;
}🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/c/Community.css around lines 2 to 13, the
.community container lacks positioning so any absolutely positioned hero
background isn't scoped to it; update the .community rule to include position:
relative to anchor the absolutely positioned background, and if you need to
enforce stacking and clipping also add an appropriate z-index (already present)
plus establish a stacking context with transform: translateZ(0) or isolation:
isolate and ensure border-radius/overflow settings are applied so the absolute
background is clipped as intended.
| .community__background { | ||
| position: absolute; | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: flex-start; | ||
| height: 20rem; | ||
| border-radius: var(--section-border-radius); | ||
| overflow: hidden; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Background container lacks sizing and stacking; can render unpredictably
As an absolutely positioned sibling, .community__background needs explicit inset or width to fill the header area, and a lower z-index so content sits above.
.community__background {
position: absolute;
display: flex;
flex-direction: column;
align-items: flex-start;
height: 20rem;
border-radius: var(--section-border-radius);
overflow: hidden;
+ inset: 0 auto auto 0; /* top/left pinned */
+ width: 100%;
+ z-index: 0; /* ensure it's behind content */
+ pointer-events: none; /* avoid intercepting clicks on links above */
}📝 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.
| .community__background { | |
| position: absolute; | |
| display: flex; | |
| flex-direction: column; | |
| align-items: flex-start; | |
| height: 20rem; | |
| border-radius: var(--section-border-radius); | |
| overflow: hidden; | |
| } | |
| .community__background { | |
| position: absolute; | |
| display: flex; | |
| flex-direction: column; | |
| align-items: flex-start; | |
| height: 20rem; | |
| border-radius: var(--section-border-radius); | |
| overflow: hidden; | |
| inset: 0 auto auto 0; /* top/left pinned */ | |
| width: 100%; | |
| z-index: 0; /* ensure it's behind content */ | |
| pointer-events: none; /* avoid intercepting clicks on links above */ | |
| } |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/c/Community.css around lines 14 to 22, the
absolutely positioned .community__background lacks explicit sizing/insets and
stacking context which can cause unpredictable rendering; update the rule to
provide explicit insets (e.g., inset: 0 or top/left/right with a fixed
height/width or width:100%) so it reliably fills the header area, and set a
lower z-index (e.g., z-index: 0) so foreground content can sit above it (ensure
foreground elements have higher z-index or position to remain interactive).
| .community__background-image { | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: flex-start; | ||
| background: var(--color-body-bg-color, #101028); | ||
|
|
||
| opacity: 0.4; | ||
| mix-blend-mode: luminosity; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Image styles on use flex/background; ensure full-cover behavior
display:flex and background on an img won’t achieve a cover background. Make the image fill and crop properly.
.community__background-image {
- display: flex;
- flex-direction: column;
- align-items: flex-start;
- background: var(--color-body-bg-color, #101028);
-
- opacity: 0.4;
- mix-blend-mode: luminosity;
+ display: block;
+ width: 100%;
+ height: 100%;
+ object-fit: cover;
+ opacity: 0.4;
+ mix-blend-mode: luminosity;
}📝 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.
| .community__background-image { | |
| display: flex; | |
| flex-direction: column; | |
| align-items: flex-start; | |
| background: var(--color-body-bg-color, #101028); | |
| opacity: 0.4; | |
| mix-blend-mode: luminosity; | |
| } | |
| .community__background-image { | |
| display: block; | |
| width: 100%; | |
| height: 100%; | |
| object-fit: cover; | |
| opacity: 0.4; | |
| mix-blend-mode: luminosity; | |
| } |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/c/Community.css around lines 24 to 32, the rules
apply display:flex and background to an <img> which won’t produce a full-cover
background; change the approach so the image fills and crops correctly: remove
flex and background properties from the img selector, ensure the element is
sized (width:100%; height:100% or fixed height on its container), set
display:block and object-fit:cover on the <img>, or alternatively move
background/background-size:cover to a wrapper div if you need CSS background
images; ensure the wrapper uses position/size to allow full-cover behavior.
| <> | ||
| <div className="community__background"> | ||
| {community.hero_image_url ? ( | ||
| <img | ||
| src={community.hero_image_url} | ||
| alt={community.name} | ||
| className="community__background-image" | ||
| /> | ||
| ) : null} | ||
| <div className="community__background-tint" /> | ||
| </div> | ||
| <div className="container container--y container--full layout__content community"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hero background is absolutely positioned without a positioned ancestor; likely misaligned/overflowing
The .community__background block is rendered as a sibling before the .community container, and CSS sets it to position: absolute without a positioned ancestor. This will position it relative to the nearest ancestor with non-static positioning (often the page), not the community container, breaking clipping, border-radius, and z-index expectations.
Two fixes (pick one):
- Move the background inside the
.communitycontainer as the first child and addposition: relativeto.community. - Wrap both background and content in a new positioned wrapper.
If you choose the first approach, apply this DOM change and the CSS changes suggested in Community.css review.
Apply this diff to move the background inside the container:
- <>
- <div className="community__background">
+ <>
+ <div className="container container--y container--full layout__content community">
+ <div className="community__background">
{community.hero_image_url ? (
<img
src={community.hero_image_url}
alt={community.name}
className="community__background-image"
/>
) : null}
<div className="community__background-tint" />
- </div>
- <div className="container container--y container--full layout__content community">
+ </div>
<NewBreadCrumbs>📝 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.
| <> | |
| <div className="community__background"> | |
| {community.hero_image_url ? ( | |
| <img | |
| src={community.hero_image_url} | |
| alt={community.name} | |
| className="community__background-image" | |
| /> | |
| ) : null} | |
| <div className="community__background-tint" /> | |
| </div> | |
| <div className="container container--y container--full layout__content community"> | |
| <> | |
| <div className="container container--y container--full layout__content community"> | |
| <div className="community__background"> | |
| {community.hero_image_url ? ( | |
| <img | |
| src={community.hero_image_url} | |
| alt={community.name} | |
| className="community__background-image" | |
| /> | |
| ) : null} | |
| <div className="community__background-tint" /> | |
| </div> | |
| <NewBreadCrumbs> |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/c/community.tsx around lines 179 to 190, the
.community__background is rendered as a sibling before the .community container
and is absolutely positioned without a positioned ancestor; move the background
markup inside the .community container as its first child and add position:
relative to the .community CSS so the absolutely positioned background is
clipped and stacked correctly; update Community.css as suggested in the review
(add position: relative to .community and ensure .community__background uses
absolute positioning with appropriate inset, overflow, and border-radius rules)
to complete the fix.
| community: await dapper.getCommunity(params.communityId), | ||
| communityFilters: await dapper.getCommunityFilters(params.communityId), | ||
| listing: await dapper.getPackageListingDetails( | ||
| params.communityId, | ||
| params.namespaceId, | ||
| params.packageId | ||
| ), | ||
| team: await dapper.getTeamDetails(params.namespaceId), | ||
| filters: await dapper.getCommunityFilters(params.communityId), | ||
| permissions: undefined, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate data fetching
Both communityFilters and filters are fetching the same data using dapper.getCommunityFilters(params.communityId).
return {
community: await dapper.getCommunity(params.communityId),
communityFilters: await dapper.getCommunityFilters(params.communityId),
listing: await dapper.getPackageListingDetails(
params.communityId,
params.namespaceId,
params.packageId
),
team: await dapper.getTeamDetails(params.namespaceId),
- filters: await dapper.getCommunityFilters(params.communityId),
permissions: undefined,
};Update the component to use communityFilters instead of filters.
📝 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.
| community: await dapper.getCommunity(params.communityId), | |
| communityFilters: await dapper.getCommunityFilters(params.communityId), | |
| listing: await dapper.getPackageListingDetails( | |
| params.communityId, | |
| params.namespaceId, | |
| params.packageId | |
| ), | |
| team: await dapper.getTeamDetails(params.namespaceId), | |
| filters: await dapper.getCommunityFilters(params.communityId), | |
| permissions: undefined, | |
| community: await dapper.getCommunity(params.communityId), | |
| communityFilters: await dapper.getCommunityFilters(params.communityId), | |
| listing: await dapper.getPackageListingDetails( | |
| params.communityId, | |
| params.namespaceId, | |
| params.packageId | |
| ), | |
| team: await dapper.getTeamDetails(params.namespaceId), | |
| permissions: undefined, |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/packageEdit.tsx around lines 52 to 61, the code
fetches community filters twice (communityFilters and filters) via
dapper.getCommunityFilters(params.communityId); remove the duplicate fetch by
deleting the filters property from the returned object, and update any component
references to use communityFilters instead of filters so the component reads
from communityFilters throughout.
| const { community, listing, filters, permissions } = useLoaderData< | ||
| typeof loader | typeof clientLoader | ||
| >(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update destructuring to match loader changes
After removing the duplicate filters field, update the destructuring to use communityFilters.
-const { community, listing, filters, permissions } = useLoaderData<
+const { community, listing, communityFilters, permissions } = useLoaderData<
typeof loader | typeof clientLoader
>();And update the references throughout the component:
-options={filters.package_categories.map((category) => ({
+options={communityFilters.package_categories.map((category) => ({- filters.package_categories.find(
+ communityFilters.package_categories.find(📝 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.
| const { community, listing, filters, permissions } = useLoaderData< | |
| typeof loader | typeof clientLoader | |
| >(); | |
| const { community, listing, communityFilters, permissions } = useLoaderData< | |
| typeof loader | typeof clientLoader | |
| >(); |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/packageEdit.tsx around lines 117 to 119, the
loader no longer returns a duplicate filters field so the destructuring should
be updated to pull communityFilters instead of filters; change "const {
community, listing, filters, permissions } = useLoaderData<...>()" to include
communityFilters and then replace all uses of "filters" in this component with
"communityFilters" (or adjust variable names where appropriate) to match the
updated loader shape.
| reviewStatusColor={"orange"} | ||
| reviewStatus={"Skibidied"} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded admin URLs should be dynamic
The hardcoded admin URLs (lines 318 and 336) contain fixed IDs that won't work for different packages. These should be dynamically generated based on the actual package data.
- href={`${
- import.meta.env.VITE_SITE_URL
- }/djangoadmin/community/packagelisting/206/change/`}
+ href={`${
+ import.meta.env.VITE_SITE_URL
+ }/djangoadmin/community/packagelisting/${packagePermissions.package.listing_id}/change/`}Apply similar fix to line 336 for the package admin URL.
📝 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.
| reviewStatusColor={"orange"} | |
| reviewStatus={"Skibidied"} | |
| href={`${ | |
| import.meta.env.VITE_SITE_URL | |
| }/djangoadmin/community/packagelisting/${packagePermissions.package.listing_id}/change/`} |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/packageListing.tsx around lines 305-306 (and also
update the other occurrences at ~318 and ~336), the admin URLs are hardcoded
with fixed package IDs; replace those literal IDs with the package's actual id
from props/state (e.g., package.id or pkg.id) so the link is generated
dynamically, and construct the path using the app's router convention (use a
template string or URL helper) for both admin links so they point to the current
package instead of a fixed ID.
4a5e0d8 to
f2c5ab5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
apps/cyberstorm-remix/app/c/community.tsx (1)
179-256: Hero background is absolutely positioned without a positioned ancestor; move inside the container for correct clipping/stackingThis mirrors a prior comment on this file: rendering
.community__backgroundas a sibling causes it to position relative to the page, not the.communitysection, which can break border-radius, overflow, and z-index.Apply this DOM reorder so the background is the first child of the
.communitycontainer:- <> - <div className="community__background"> + <> + <div className="container container--y container--full layout__content community"> + <div className="community__background"> {community.hero_image_url ? ( <img src={community.hero_image_url} alt={community.name} className="community__background-image" /> ) : null} - <div className="community__background-tint" /> - </div> - <div className="container container--y container--full layout__content community"> + <div className="community__background-tint" /> + </div> <NewBreadCrumbs> ... </NewBreadCrumbs> <PageHeader ...> {community.name} </PageHeader> <PackageSearch ... /> - </div> + </div> </>Ensure corresponding CSS sets
.community { position: relative; overflow: hidden; }and.community__background { position: absolute; inset: 0; }so the background is clipped by the container.apps/cyberstorm-remix/app/p/packageListing.tsx (5)
75-76: Stop deep-importing from dapper internals; use the public client APIImporting from "@thunderstore/dapper-ts/src/..." will break with the published package. Remove the deep import and rely on the instance method/type on DapperTs.
Apply this diff to remove the deep import:
- import { getPackagePermissions } from "@thunderstore/dapper-ts/src/methods/package";Then, type usages should reference the public instance method (see comments on Lines 175-190 and 292-293).
166-172: Guard missing session tools and avoid optional chaining in Dapper configtools may be undefined; you call tools.getSessionCurrentUser() below, which will throw. Also, don’t pass possibly undefined config values into DapperTs.
- const tools = getSessionTools(context); + const tools = getSessionTools(context); + if (!tools) { + throw new Error("Session tools not found in context"); + } const dapper = new DapperTs(() => { - return { - apiHost: tools?.getConfig().apiHost, - sessionId: tools?.getConfig().sessionId, - }; + const cfg = tools.getConfig(); + return { + apiHost: cfg.apiHost, + sessionId: cfg.sessionId, + }; });
175-190: Remove Promise.withResolvers; assign the returned promise directlyPromise.withResolvers has limited support and is unnecessary here. Assign the promise from dapper.getPackagePermissions when logged in.
- // We do some trickery right here to prevent unnecessary request when the user is not logged in - let permissionsPromise = undefined; + // Prevent unnecessary request when the user is not logged in + let permissionsPromise: + | ReturnType<DapperTs["getPackagePermissions"]> + | undefined; const cu = await tools.getSessionCurrentUser(); if (cu.username) { - const wrapperPromise = - Promise.withResolvers< - Awaited<ReturnType<typeof getPackagePermissions>> - >(); - dapper - .getPackagePermissions( - params.communityId, - params.namespaceId, - params.packageId - ) - .then(wrapperPromise.resolve, wrapperPromise.reject); - permissionsPromise = wrapperPromise.promise; + permissionsPromise = dapper.getPackagePermissions( + params.communityId, + params.namespaceId, + params.packageId + ); }
26-34: Remove unused value import; fix code-scanning false positivefetchPackagePermissions is only referenced in a type position later, which keeps a live value import and triggers “defined but never used.” After switching to the public DapperTs method type, drop this import.
import { ApiError, - fetchPackagePermissions, packageListingApprove, packageListingReject, packageListingReport, PackageListingReportRequestData, RequestConfig, } from "@thunderstore/thunderstore-api";
292-293: Type via the public DapperTs API, not thunderstore-api internalsUse the instance method’s return type to avoid importing fetchPackagePermissions just for types.
- function managementTools( - packagePermissions: Awaited<ReturnType<typeof fetchPackagePermissions>> - ) { + function managementTools( + packagePermissions: Awaited<ReturnType<DapperTs["getPackagePermissions"]>> + ) {apps/cyberstorm-remix/app/p/packageEdit.tsx (4)
60-62: Remove duplicate communityFilters fetch in the server loaderfilters duplicates communityFilters. Drop filters here.
return { community: await dapper.getCommunity(params.communityId), communityFilters: await dapper.getCommunityFilters(params.communityId), listing: await dapper.getPackageListingDetails( params.communityId, params.namespaceId, params.packageId ), team: await dapper.getTeamDetails(params.namespaceId), - filters: await dapper.getCommunityFilters(params.communityId), permissions: undefined, };
79-85: Guard missing session tools and avoid optional chaining in Dapper configSame issue as listing page: ensure tools exists and build cfg once.
- const tools = getSessionTools(context); + const tools = getSessionTools(context); + if (!tools) { + throw new Error("Session tools not found in context"); + } const dapper = new DapperTs(() => { - return { - apiHost: tools?.getConfig().apiHost, - sessionId: tools?.getConfig().sessionId, - }; + const cfg = tools.getConfig(); + return { + apiHost: cfg.apiHost, + sessionId: cfg.sessionId, + }; });
106-107: Remove duplicate communityFilters fetch in the client loaderfilters duplicates communityFilters here too.
return { community: await dapper.getCommunity(params.communityId), communityFilters: await dapper.getCommunityFilters(params.communityId), listing: await dapper.getPackageListingDetails( params.communityId, params.namespaceId, params.packageId ), team: await dapper.getTeamDetails(params.namespaceId), - filters: await dapper.getCommunityFilters(params.communityId), permissions: permissions, };
384-401: Switch usages from filters to communityFiltersThese still reference the removed filters field.
- options={filters.package_categories.map((category) => ({ + options={communityFilters.package_categories.map((category) => ({ value: category.slug, label: category.name, }))} ... - value={formInputs.categories.map((categoryId) => ({ + value={formInputs.categories.map((categoryId) => ({ value: categoryId, label: - filters.package_categories.find( + communityFilters.package_categories.find( (c) => c.slug === categoryId )?.name || "", }))}apps/cyberstorm-remix/app/p/packageListing.css (1)
48-53: Fix duplicate CSS property (align-items) to satisfy linterThere are two align-items declarations; remove the redundant one to pass Biome’s noDuplicateProperties.
.package-listing__package-section { position: relative; display: flex; flex: 1 0 0; flex-direction: column; gap: 8px; - align-items: center; align-items: flex-start; align-self: stretch; justify-content: flex-start; padding: 7.5rem 3rem 2rem; }
🧹 Nitpick comments (15)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Profile/Profile.tsx (4)
14-14: Keep form state in sync after revalidationBecause
useReducer’s initial state runs only once, a post-saverevalidator.revalidate()won’t update the input if the server normalizes the link. Sync whenteam.donation_linkchanges.Apply:
-import { useReducer } from "react"; +import { useEffect, useReducer } from "react";} return ( + useEffect(() => { + updateFormFieldState({ + field: "donation_link", + value: team.donation_link ?? "", + }); + }, [team.donation_link]), <div className="settings-items team-profile">Alternative: only update if not dirty (if
useStrongFormexposesisDirty/isSubmitting, guard accordingly).Also applies to: 112-113
121-135: Small a11y win: associate label with inputThe visual label is a
span, so screen readers don’t connect it with the input. Use alabelwithhtmlForand set anidon the input.Apply:
- <span className="team-profile__label">URL</span> + <label className="team-profile__label" htmlFor="donationLinkInput">URL</label> <NewTextInput + id="donationLinkInput" name={"donation_link"} placeholder={"https://"} value={formInputs.donation_link}
137-142: Avoid no-op requests by disabling Save when unchangedDisable the button if the value hasn’t changed from the loaded team data.
Apply:
<NewButton rootClasses="team-profile__save" onClick={strongForm.submit} + disabled={formInputs.donation_link === (team.donation_link ?? "")} > Save changes </NewButton>If
NewButtonsupports a loading state anduseStrongFormexposesisSubmitting, considerloading={strongForm.isSubmitting}as well.
67-69: Prefer nullish coalescing and trimdonation_linkbefore sending
- Use
team.donation_link ?? ""instead ofteam.donation_link || ""to avoid accidentally overwriting legitimate falsy values- In your submission handler, always call
.trim()ondata.donation_linkto strip extra whitespace. The API’steamDetailsEditRequestDataSchemarequires a non-empty string (min length 1), so empty values will be rejected sed- If you need to clear the link, note that this endpoint currently does not accept
nullor""(empty string), so you’ll either have to omit the field entirely or update the API to allow nullablesApply:
- const [formInputs, updateFormFieldState] = useReducer(formFieldUpdateAction, { - donation_link: team.donation_link || "", - }); + const [formInputs, updateFormFieldState] = useReducer(formFieldUpdateAction, { + donation_link: team.donation_link ?? "", + });async function submitor(data: typeof formInputs): Promise<SubmitorOutput> { - return await teamDetailsEdit({ + const trimmed = data.donation_link.trim(); + return await teamDetailsEdit({ config: outletContext.requestConfig, params: { teamIdentifier: team.name }, - data: { donation_link: data.donation_link }, + data: { donation_link: trimmed }, queryParams: {}, }); }apps/cyberstorm-remix/app/communities/communities.tsx (1)
171-175: Avoid unsafeascast on NewSelect onChange; add a type guard fallbackCasting
val as SortOptionstrusts unchecked input from the select. Safer: validate against the enum and fall back to a known value to prevent invalid URL state or API calls.Apply this diff within the changed lines:
- onChange={(val) => changeOrder(val as SortOptions)} + onChange={(val) => changeOrder(isSortOption(val) ? val : SortOptions.Popular)}Add this helper near the top of the file (outside the selected lines):
function isSortOption(v: unknown): v is SortOptions { return typeof v === "string" && (Object.values(SortOptions) as string[]).includes(v); }Optional: also sanitize the
valueprop similarly so the UI never renders an unknown selection:const orderParam = searchParams.get("order"); const selectedOrder = isSortOption(orderParam) ? orderParam : SortOptions.Popular;packages/cyberstorm/src/newComponents/Switch/Switch.css (1)
30-32: Nesting selector may not compile depending on tooling; prefer explicit.switch__knob:hover
&:hoverrequires CSS Nesting support in the build or browser. If the pipeline lacks a nesting plugin, this block will be dropped silently.Apply this diff to remove the nested selector:
- &:hover { - cursor: pointer; - }Then add this rule (outside the selected lines, within the same
@layer componentsblock) to keep behavior without requiring nesting:.switch__knob:hover { cursor: pointer; }packages/cyberstorm/src/newComponents/Modal/Modal.tsx (2)
12-15: Makingtriggeroptional is reasonable; ensure non-trigger usage still has a reliable open/close mechanismWith
trigger?, consumers can control the modal externally. Please verify all no-trigger usages provide a button elsewhere withpopoverTarget={popoverId}so the modal is operable, and thatpopoverIdis always set (Frame likely depends on it).Optional dev guard to surface misconfiguration early:
export function Modal(props: Props) { - const { children, csVariant = "default", csSize = "medium", trigger } = props; + const { children, csVariant = "default", csSize = "medium", trigger } = props; + if (process.env.NODE_ENV !== "production" && !props.popoverId) { + console.warn("Modal: `popoverId` is required for popover control."); + }
18-19: Follow-up on TODO: make multiple instances safe via unique IDsThe “can’t reuse the same modal twice” symptom is usually duplicate
popoverIds. Consider generating a default ID viauseId()whenpopoverIdisn’t provided, or allowing a suffix prop to disambiguate. I can draft a patch if helpful.apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx (1)
369-373: Switching to absolutehrefs: verify domain shaping to avoid//settingsor missing schemeInterpolating
${domain}/settings/.../depends ondomainnot having a trailing slash and including a scheme. To avoid environment-specific regressions, normalizedomainor use URL joining.Example helper (outside these lines):
const join = (base: string, path: string) => new URL(path.replace(/^\/+/, ""), base.endsWith("/") ? base : base + "/").toString();Then:
href={join(domain, "settings/linked-accounts/")} href={join(domain, "settings/teams/")}If this change is part of a broader move away from
cyberstormLink, consider centralizing these routes alongside the newLinkLibraryto keep portability across environments.Also applies to: 383-387
apps/cyberstorm-remix/app/p/packageListing.tsx (3)
315-323: Replace placeholder review status and verify allowed tag variant“Skibidied” looks like a placeholder, and TagVariants may not include "orange". Prefer a real status and a known variant.
- reviewStatusColor={"orange"} - reviewStatus={"Skibidied"} + reviewStatusColor={"yellow"} + reviewStatus={"Pending review"}If an actual status is available from the API (e.g., listing.review_status), bind it here instead. Confirm TagVariants includes "yellow" (used elsewhere) and not "orange".
1076-1085: Nit: Rename form field id/name to “reason” for consistencyThe field is “reason” in state and types; using “role” is confusing.
- <NewSelect - name={"role"} + <NewSelect + name={"reason"} options={reportOptions} placeholder="Please select..." value={formInputs.reason} onChange={(value) => { updateFormFieldState({ field: "reason", value: value }); }} - id="role" + id="reason" />
930-969: Optional: Prevent double submissions for approve/rejectClicks trigger network calls without disabling the buttons. Consider a submitting flag to disable while in-flight.
apps/cyberstorm-remix/app/p/packageEdit.tsx (2)
122-125: Rename component to match file and update destructuring after filters removalUse a descriptive name and align with the loader shape.
-export default function PackageListing() { - const { community, listing, filters, permissions } = useLoaderData< +export default function PackageEdit() { + const { community, listing, communityFilters, permissions } = useLoaderData< typeof loader | typeof clientLoader >();
87-95: 403 on undefined permissions is OK, but consider a clearer path for anon usersgetPackagePermissions returns undefined on 401; your 403 for non-managers covers both not-logged-in and lacks-manage. If you want to distinguish anon vs. unauthorized, check undefined specifically and redirect to login instead.
apps/cyberstorm-remix/app/p/packageListing.css (1)
143-152: Optional: Prefer overflow-x: auto and hide WebKit scrollbars for consistencyscrollbar-width covers Firefox only. Consider auto + WebKit hiding for cross-browser polish.
.package-listing__package-content-section { display: flex; flex: 1 0 0; flex-direction: column; gap: var(--gap-xl); align-items: flex-start; padding-top: 1rem; - overflow-x: scroll; + overflow-x: auto; scrollbar-width: none; } + .package-listing__package-content-section::-webkit-scrollbar { + display: none; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (33)
apps/cyberstorm-remix/app/c/Community.css(1 hunks)apps/cyberstorm-remix/app/c/community.tsx(2 hunks)apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.css(0 hunks)apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx(2 hunks)apps/cyberstorm-remix/app/communities/communities.tsx(1 hunks)apps/cyberstorm-remix/app/p/packageEdit.css(1 hunks)apps/cyberstorm-remix/app/p/packageEdit.tsx(1 hunks)apps/cyberstorm-remix/app/p/packageListing.css(2 hunks)apps/cyberstorm-remix/app/p/packageListing.tsx(9 hunks)apps/cyberstorm-remix/app/root.tsx(0 hunks)apps/cyberstorm-remix/app/routes.ts(1 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/Profile/Profile.tsx(1 hunks)apps/cyberstorm-remix/app/styles/layout.css(0 hunks)apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts(1 hunks)apps/cyberstorm-storybook/LinkLibrary.tsx(2 hunks)packages/cyberstorm/src/components/Links/LinkingProvider.tsx(2 hunks)packages/cyberstorm/src/components/Links/Links.tsx(1 hunks)packages/cyberstorm/src/newComponents/BreadCrumbs/BreadCrumbs.css(1 hunks)packages/cyberstorm/src/newComponents/Modal/Modal.tsx(1 hunks)packages/cyberstorm/src/newComponents/Switch/Switch.css(1 hunks)packages/dapper-ts/src/index.ts(3 hunks)packages/dapper-ts/src/methods/package.ts(2 hunks)packages/dapper/src/dapper.ts(1 hunks)packages/dapper/src/types/methods.ts(2 hunks)packages/dapper/src/types/package.ts(1 hunks)packages/thunderstore-api/src/get/package.ts(1 hunks)packages/thunderstore-api/src/index.ts(1 hunks)packages/thunderstore-api/src/post/package.ts(3 hunks)packages/thunderstore-api/src/post/packageListing.ts(3 hunks)packages/thunderstore-api/src/schemas/objectSchemas.ts(1 hunks)packages/thunderstore-api/src/schemas/requestSchemas.ts(3 hunks)packages/thunderstore-api/src/schemas/responseSchemas.ts(4 hunks)
💤 Files with no reviewable changes (3)
- apps/cyberstorm-remix/app/root.tsx
- apps/cyberstorm-remix/app/styles/layout.css
- apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.css
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/dapper-ts/src/methods/package.ts
- packages/thunderstore-api/src/index.ts
- packages/dapper/src/dapper.ts
- apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts
- apps/cyberstorm-storybook/LinkLibrary.tsx
- apps/cyberstorm-remix/app/routes.ts
- packages/cyberstorm/src/components/Links/LinkingProvider.tsx
- packages/thunderstore-api/src/schemas/responseSchemas.ts
- packages/thunderstore-api/src/get/package.ts
- packages/cyberstorm/src/components/Links/Links.tsx
- packages/dapper/src/types/package.ts
- packages/dapper-ts/src/index.ts
- packages/cyberstorm/src/newComponents/BreadCrumbs/BreadCrumbs.css
- apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx
- packages/thunderstore-api/src/schemas/objectSchemas.ts
- packages/dapper/src/types/methods.ts
- apps/cyberstorm-remix/app/p/packageEdit.css
- packages/thunderstore-api/src/post/packageListing.ts
- packages/thunderstore-api/src/schemas/requestSchemas.ts
- apps/cyberstorm-remix/app/c/Community.css
- packages/thunderstore-api/src/post/package.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/cyberstorm-remix/app/p/packageEdit.tsx (7)
apps/cyberstorm-remix/app/p/packageListing.tsx (4)
meta(82-128)loader(130-160)clientLoader(163-213)PackageListing(217-834)apps/cyberstorm-remix/app/root.tsx (3)
meta(72-80)clientLoader(82-93)OutletContextShape(65-70)packages/dapper-ts/src/index.ts (1)
DapperTs(35-86)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
useToast(79-87)packages/thunderstore-api/src/post/package.ts (2)
packageDeprecate(50-75)packageUnlist(77-102)packages/thunderstore-api/src/post/packageListing.ts (1)
packageListingUpdate(22-47)
packages/cyberstorm/src/newComponents/Modal/Modal.tsx (1)
packages/cyberstorm-theme/src/components/Modal/Modal.ts (2)
ModalVariants(3-3)ModalSizes(7-7)
apps/cyberstorm-remix/app/c/community.tsx (1)
apps/cyberstorm-remix/app/commonComponents/PageHeader/PageHeader.tsx (1)
PageHeader(17-81)
apps/cyberstorm-remix/app/p/packageListing.tsx (10)
apps/cyberstorm-remix/app/root.tsx (1)
clientLoader(82-93)packages/dapper-ts/src/index.ts (1)
DapperTs(35-86)packages/dapper-ts/src/methods/package.ts (1)
getPackagePermissions(157-183)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
useToast(79-87)packages/thunderstore-api/src/get/package.ts (1)
fetchPackagePermissions(9-27)packages/thunderstore-api/src/index.ts (1)
RequestConfig(1-7)packages/ts-api-react-actions/src/ApiAction.tsx (1)
ApiAction(18-52)packages/thunderstore-api/src/post/packageListing.ts (3)
packageListingReject(76-101)packageListingApprove(49-74)packageListingReport(103-130)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
useStrongForm(24-169)
🪛 Biome (2.1.2)
apps/cyberstorm-remix/app/p/packageListing.css
[error] 49-49: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
align-items is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
⏰ 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: CodeQL
🔇 Additional comments (1)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Profile/Profile.tsx (1)
67-69: Prefilling with team data is a solid UX improvementUsing the loader-provided
team.donation_linkas the initial value avoids controlled/uncontrolled warnings and improves hydration consistency. Looks good.
| // TODO: Needs to check if package is available for the logged in user | ||
| // export async function clientLoader({ params }: LoaderFunctionArgs) { | ||
| // if (params.communityId && params.namespaceId && params.packageId) { | ||
| // try { | ||
| // const dapper = window.Dapper; | ||
| // return { | ||
| // community: await dapper.getCommunity(params.communityId), | ||
| // communityFilters: await dapper.getCommunityFilters(params.communityId), | ||
| // listing: await dapper.getPackageListingDetails( | ||
| // params.communityId, | ||
| // params.namespaceId, | ||
| // params.packageId | ||
| // ), | ||
| // team: await dapper.getTeamDetails(params.namespaceId), | ||
| // currentUser: await dapper.getCurrentUser(), | ||
| // }; | ||
| // } catch (error) { | ||
| // if (error instanceof ApiError) { | ||
| // throw new Response("Package not found", { status: 404 }); | ||
| // } else { | ||
| // // REMIX TODO: Add sentry | ||
| // throw error; | ||
| // } | ||
| // } | ||
| // } | ||
| // throw new Response("Package not found", { status: 404 }); | ||
| // } | ||
| export async function clientLoader({ params, context }: LoaderFunctionArgs) { | ||
| if (params.communityId && params.namespaceId && params.packageId) { | ||
| try { | ||
| const tools = getSessionTools(context); | ||
| const dapper = new DapperTs(() => { | ||
| return { | ||
| apiHost: tools?.getConfig().apiHost, | ||
| sessionId: tools?.getConfig().sessionId, | ||
| }; | ||
| }); | ||
|
|
||
| // We do some trickery right here to prevent unnecessary request when the user is not logged in | ||
| let permissionsPromise = undefined; | ||
| const cu = await tools.getSessionCurrentUser(); | ||
| if (cu.username) { | ||
| const wrapperPromise = | ||
| Promise.withResolvers< | ||
| Awaited<ReturnType<typeof getPackagePermissions>> | ||
| >(); | ||
| dapper | ||
| .getPackagePermissions( | ||
| params.communityId, | ||
| params.namespaceId, | ||
| params.packageId | ||
| ) | ||
| .then(wrapperPromise.resolve, wrapperPromise.reject); | ||
| permissionsPromise = wrapperPromise.promise; | ||
| } | ||
|
|
||
| return { | ||
| community: await dapper.getCommunity(params.communityId), | ||
| communityFilters: await dapper.getCommunityFilters(params.communityId), | ||
| listing: await dapper.getPackageListingDetails( | ||
| params.communityId, | ||
| params.namespaceId, | ||
| params.packageId | ||
| ), | ||
| team: await dapper.getTeamDetails(params.namespaceId), | ||
| permissions: permissionsPromise, | ||
| }; | ||
| } catch (error) { | ||
| if (error instanceof ApiError) { | ||
| throw new Response("Package not found", { status: 404 }); | ||
| } else { | ||
| // REMIX TODO: Add sentry | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
| throw new Response("Package not found", { status: 404 }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Repo-wide check to catch remaining deep imports and Promise.withResolvers
Run this to ensure no other deep imports of dapper internals or Promise.withResolvers usages remain.
🏁 Script executed:
#!/bin/bash
rg -nP "@thunderstore/dapper-ts/src/" -C2
rg -nP "\bPromise\.withResolvers\b" -C2Length of output: 3731
Refactor deep imports and eliminate Promise.withResolvers usages
The repo-wide grep confirms that there are still direct deep imports of internal modules from @thunderstore/dapper-ts/src/… and ad-hoc calls to Promise.withResolvers. These should be replaced with the publicly exported APIs and standard promise patterns to preserve encapsulation and maintainability.
Please address the following locations:
• apps/cyberstorm-remix/app/upload/upload.tsx (line 43)
– Replace
import { postPackageSubmissionMetadata } from "@thunderstore/dapper-ts/src/methods/package";
with the public entrypoint, e.g.:
import { postPackageSubmissionMetadata } from "@thunderstore/dapper-ts";
• apps/cyberstorm-remix/app/settings/teams/Teams.tsx (line 25)
– Replace
import { postTeamCreate } from "@thunderstore/dapper-ts/src/methods/team";
with the public API import from "@thunderstore/dapper-ts".
• apps/cyberstorm-remix/app/p/packageListing.tsx (line 75)
– Replace
import { getPackagePermissions } from "@thunderstore/dapper-ts/src/methods/package";
with the public export from "@thunderstore/dapper-ts".
• apps/cyberstorm-remix/app/p/tabs/Versions/Versions.tsx (line 18)
– Replace
import { versionsSchema } from "@thunderstore/dapper-ts/src/methods/package";
with the public schema import.
• packages/ts-uploader/src/uploaders/MultipartUpload.ts (around line 392)
– Refactor the Promise.withResolvers<…>() pattern. Consider using the standard
new Promise<…>((resolve, reject) => { … }) instead of accessing a non-standard helper.
• packages/graph-system/src/executor.ts (around line 50)
– Refactor the Promise.withResolvers() usage into a regular promise constructor.
By switching to the officially exported methods in @thunderstore/dapper-ts and standard promise constructions, we avoid brittle dependencies on internal paths and remove non-standard promise helpers.
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/packageListing.tsx around lines 162-213, the
loader uses a deep import and Promise.withResolvers; replace the deep import of
getPackagePermissions with the public export from "@thunderstore/dapper-ts" and
eliminate Promise.withResolvers by creating a standard Promise: when cu.username
is truthy assign permissionsPromise = new Promise((resolve, reject) =>
dapper.getPackagePermissions(params.communityId, params.namespaceId,
params.packageId).then(resolve, reject)); keep the rest of the logic and typings
intact and return that Promise as permissions.
Oksamies
force-pushed
the
08-18-add_package_management_tools
branch
from
f2c5ab5 to
3da1c4a
Compare
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 (2)
apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
111-121: Return value can be stale; avoid mixing async/await with then/catch.setState is async; returning submitOutput immediately after the chain can yield undefined or the previous value. Also mixing await with .then/.catch reduces clarity. Flatten to async/await and return a local variable.
Apply this diff to simplify control flow and fix the return value:
- try { - await props - .submitor(submissionData) - .then((output) => { - setSubmitOutput(output); - if (props.onSubmitSuccess) { - props.onSubmitSuccess(output); - } - }) - .catch((error) => { + try { + const output = await props.submitor(submissionData); + setSubmitOutput(output); + if (props.onSubmitSuccess) { + props.onSubmitSuccess(output); + } + return output; + } catch (error) { + try { if (error instanceof RequestBodyParseError) { setSubmitError( new Error( "Some of the field values are invalid" ) as SubmissionError ); setInputErrors(error.error.formErrors as InputErrors); } else if (error instanceof RequestQueryParamsParseError) { setSubmitError( new Error( "Some of the query parameters are invalid" ) as SubmissionError ); setInputErrors(error.error.formErrors as InputErrors); } else if (error instanceof ParseError) { setSubmitError( new Error( "Request succeeded, but the response was invalid" ) as SubmissionError ); setInputErrors(error.error.formErrors as InputErrors); - throw error; } else { throw error; } - }); - return submitOutput; - } catch (error) { + } finally { + // Fall through to the outer error handler below + } if (props.onSubmitError) { props.onSubmitError(error as SubmissionError); } throw error; } finally { setSubmitting(false); }Also applies to: 148-149
packages/dapper-fake/src/fakers/package.ts (1)
146-149: Bug: extra '}' in full_version_name.Trailing brace produces malformed version names like foo-bar-1.2.3}. Fix the template literal.
Apply this diff:
- full_version_name: `${namespace}-${name}-${version}}`, + full_version_name: `${namespace}-${name}-${version}`,
♻️ Duplicate comments (10)
apps/cyberstorm-remix/app/p/packageEdit.tsx (5)
52-62: Duplicate community filters fetch (wasted request and divergent state)You fetch community filters twice (
communityFiltersandfilters). Keep a single source of truth.Apply:
return { community: await dapper.getCommunity(params.communityId), communityFilters: await dapper.getCommunityFilters(params.communityId), listing: await dapper.getPackageListingDetails( params.communityId, params.namespaceId, params.packageId ), team: await dapper.getTeamDetails(params.namespaceId), - filters: await dapper.getCommunityFilters(params.communityId), permissions: undefined, };
79-85: Guard missing session tools to avoid runtime null-deref
getSessionTools(context)can be undefined. You call methods ontoolsunconditionally via optional chaining and later directly use it (e.g., permissions fetch). Add a hard guard and avoid optional chaining in config.Apply:
- const tools = getSessionTools(context); - const dapper = new DapperTs(() => { - return { - apiHost: tools?.getConfig().apiHost, - sessionId: tools?.getConfig().sessionId, - }; - }); + const tools = getSessionTools(context); + if (!tools) { + throw new Error("Session tools not found in context"); + } + const cfg = tools.getConfig(); + const dapper = new DapperTs(() => { + return { + apiHost: cfg.apiHost, + sessionId: cfg.sessionId, + }; + });
97-108: Duplicate community filters in clientLoaderSame duplication as the server loader. Remove the
filtersfield and rely oncommunityFilters.return { community: await dapper.getCommunity(params.communityId), communityFilters: await dapper.getCommunityFilters(params.communityId), listing: await dapper.getPackageListingDetails( params.communityId, params.namespaceId, params.packageId ), team: await dapper.getTeamDetails(params.namespaceId), - filters: await dapper.getCommunityFilters(params.communityId), permissions: permissions, };
123-125: Destructure and use communityFilters after de-duplicating filtersUpdate consumer code to match the loader shape.
-export default function PackageListing() { - const { community, listing, filters, permissions } = useLoaderData< +export default function PackageListing() { + const { community, listing, communityFilters, permissions } = useLoaderData< typeof loader | typeof clientLoader >();
384-401: Use communityFilters instead of filters throughout the formAlign the select’s options and value mapping to the single filters source.
- options={filters.package_categories.map((category) => ({ + options={communityFilters.package_categories.map((category) => ({ value: category.slug, label: category.name, }))} ... - filters.package_categories.find( + communityFilters.package_categories.find( (c) => c.slug === categoryId )?.name || "",apps/cyberstorm-remix/app/p/packageListing.css (1)
42-53: Remove duplicate align-items declaration (Biome error, last one wins anyway)There are two align-items rules; keep just the intended one.
.package-listing__package-section { position: relative; display: flex; flex: 1 0 0; flex-direction: column; gap: 8px; - align-items: center; align-items: flex-start; align-self: stretch; justify-content: flex-start; padding: 7.5rem 3rem 2rem; }apps/cyberstorm-remix/app/routes.ts (1)
30-30: New edit route: ensure the page’s loader enforces permissions server-sideThe route is correct. Please confirm apps/p/packageEdit.tsx’s server loader denies access (403) to users lacking can_manage, not only in clientLoader.
#!/bin/bash # Quick check for permission gating in packageEdit loader rg -nC2 'export async function loader' apps/cyberstorm-remix/app/p/packageEdit.tsx rg -nC2 '403|Forbidden|can_manage|permissions' apps/cyberstorm-remix/app/p/packageEdit.tsxapps/cyberstorm-remix/app/p/packageListing.tsx (3)
75-75: Avoid deep import from dapper internals
@thunderstore/dapper-ts/src/...is not part of the public API and will break with published packages. Use the instance method’s return type.-import { getPackagePermissions } from "@thunderstore/dapper-ts/src/methods/package";No replacement import needed; type through
DapperTs["getPackagePermissions"]below.
166-172: Guard missing session tools and stop passing undefined into Dapper configSame pattern as elsewhere: add a tools guard and remove optional chaining in config.
- const tools = getSessionTools(context); - const dapper = new DapperTs(() => { - return { - apiHost: tools?.getConfig().apiHost, - sessionId: tools?.getConfig().sessionId, - }; - }); + const tools = getSessionTools(context); + if (!tools) { + throw new Error("Session tools not found in context"); + } + const cfg = tools.getConfig(); + const dapper = new DapperTs(() => { + return { + apiHost: cfg.apiHost, + sessionId: cfg.sessionId, + }; + });
174-191: Remove Promise.withResolvers; assign the promise directly (broader compat, simpler code)
Promise.withResolversisn’t needed here. Assign the returned promise directly and type via the public Dapper API.- // We do some trickery right here to prevent unnecessary request when the user is not logged in - let permissionsPromise = undefined; + // Prevent unnecessary request when the user is not logged in + let permissionsPromise: + | ReturnType<DapperTs["getPackagePermissions"]> + | undefined = undefined; const cu = await tools.getSessionCurrentUser(); if (cu.username) { - const wrapperPromise = - Promise.withResolvers< - Awaited<ReturnType<typeof getPackagePermissions>> - >(); - dapper - .getPackagePermissions( - params.communityId, - params.namespaceId, - params.packageId - ) - .then(wrapperPromise.resolve, wrapperPromise.reject); - permissionsPromise = wrapperPromise.promise; + permissionsPromise = dapper.getPackagePermissions( + params.communityId, + params.namespaceId, + params.packageId + ); }
🧹 Nitpick comments (9)
packages/cyberstorm/src/newComponents/Modal/Modal.tsx (2)
12-12: Making trigger optional looks good; document usage without a trigger.When trigger is omitted, opening relies on external popoverTarget elements. Consider a short JSDoc noting this, and add a Storybook story demonstrating programmatic open.
18-18: Track the TODO via an issue.“The same modal can't be used in 2 places on the same page” sounds like a popoverId collision. Suggest filing an issue and planning a scoped fix (auto-unique IDs or scoping per mounting point).
I can draft an issue and a potential approach (ID namespace + context provider). Want me to proceed?
packages/dapper-fake/src/fakers/package.ts (1)
168-172: Optional: avoid Math.random for determinism under seeding.getFakePackageVersions uses Math.random, which ignores faker’s seed and breaks reproducibility. Prefer faker.number or faker.helpers.weightedArrayElement.
I can propose a deterministic replacement if you want to keep seeded outputs stable across runs.
apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx (1)
369-387: Switch from cyberstormLink to absolute hrefs: verify domain shape and consider centralizing these routesYou replaced internal linkId-based navigation with absolute links using the domain prop. This is fine short-term, but please:
- Ensure domain always includes protocol and no trailing slash, or normalize it to avoid accidental double slashes (“https://…//settings/…”).
- Consider routing these through LinkLibrary in the future to keep parity with app-wide route changes.
Apply this minimal guard if domain may include a trailing slash:
- href={`${domain}/settings/linked-accounts/`} + href={`${domain.replace(/\/+$/, "")}/settings/linked-accounts/`}And similarly below:
- href={`${domain}/settings/teams/`} + href={`${domain.replace(/\/+$/, "")}/settings/teams/`}Also applies to: 383-387
apps/cyberstorm-remix/app/p/packageEdit.tsx (1)
218-224: Revalidate after successful save to reflect updated categories immediatelyYou toast on success but don’t revalidate the page data. Add revalidation to keep UI state in sync.
onSubmitSuccess: () => { toast.addToast({ csVariant: "success", children: `Changes saved!`, duration: 4000, }); + revalidator.revalidate(); },apps/cyberstorm-remix/app/p/packageListing.css (1)
143-152: Prefer overflow-x: auto to avoid always-visible scrollbarsMinor UX nit: auto will show scrollbars only when needed.
- overflow-x: scroll; + overflow-x: auto;apps/cyberstorm-remix/app/p/packageListing.tsx (3)
290-297: Type the management tools param via public API types and replace placeholder stringsUse the public type for permissions and avoid placeholder UI (“Skibidied”, arbitrary color).
- function managementTools( - packagePermissions: Awaited<ReturnType<typeof fetchPackagePermissions>> - ) { + function managementTools( + packagePermissions: PackagePermissionsResponseData + ) {Later in the modal props, replace hardcoded values with meaningful ones (or derive from
packagePermissionsif such fields exist):- reviewStatusColor={"orange"} - reviewStatus={"Skibidied"} + reviewStatusColor={"yellow"} + reviewStatus={"Pending review"}If the API exposes a review status, wire it instead of hardcoding.
856-872: Close the review modal and/or revalidate on success for immediate feedbackThe approve/reject actions toast, but don’t close the modal or refresh state.
Consider adding:
- A call to revalidate (if available via context) or
- Triggering popoverTargetAction="hide" via a ref/prop
994-1011: Report form: props suggest hybrid ID sources — keep it consistentYou’ve commented out community/namespace/package props and rely solely on
id. If the production endpoint will switch to a different path, keep a single source of truth and remove dead props or reintroduce the full triplet once the endpoint stabilizes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (35)
apps/cyberstorm-remix/app/c/Community.css(1 hunks)apps/cyberstorm-remix/app/c/community.tsx(2 hunks)apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.css(0 hunks)apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx(2 hunks)apps/cyberstorm-remix/app/communities/communities.tsx(1 hunks)apps/cyberstorm-remix/app/p/packageEdit.css(1 hunks)apps/cyberstorm-remix/app/p/packageEdit.tsx(1 hunks)apps/cyberstorm-remix/app/p/packageListing.css(2 hunks)apps/cyberstorm-remix/app/p/packageListing.tsx(9 hunks)apps/cyberstorm-remix/app/root.tsx(0 hunks)apps/cyberstorm-remix/app/routes.ts(1 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/Profile/Profile.tsx(1 hunks)apps/cyberstorm-remix/app/styles/layout.css(0 hunks)apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts(1 hunks)apps/cyberstorm-storybook/LinkLibrary.tsx(2 hunks)packages/cyberstorm/src/components/Links/LinkingProvider.tsx(2 hunks)packages/cyberstorm/src/components/Links/Links.tsx(1 hunks)packages/cyberstorm/src/newComponents/BreadCrumbs/BreadCrumbs.css(1 hunks)packages/cyberstorm/src/newComponents/Modal/Modal.tsx(1 hunks)packages/cyberstorm/src/newComponents/Switch/Switch.css(1 hunks)packages/dapper-fake/src/fakers/package.ts(1 hunks)packages/dapper-fake/src/index.ts(2 hunks)packages/dapper-ts/src/index.ts(3 hunks)packages/dapper-ts/src/methods/package.ts(2 hunks)packages/dapper/src/dapper.ts(1 hunks)packages/dapper/src/types/methods.ts(2 hunks)packages/dapper/src/types/package.ts(1 hunks)packages/thunderstore-api/src/get/package.ts(1 hunks)packages/thunderstore-api/src/index.ts(1 hunks)packages/thunderstore-api/src/post/package.ts(3 hunks)packages/thunderstore-api/src/post/packageListing.ts(3 hunks)packages/thunderstore-api/src/schemas/objectSchemas.ts(1 hunks)packages/thunderstore-api/src/schemas/requestSchemas.ts(3 hunks)packages/thunderstore-api/src/schemas/responseSchemas.ts(4 hunks)
💤 Files with no reviewable changes (3)
- apps/cyberstorm-remix/app/styles/layout.css
- apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.css
- apps/cyberstorm-remix/app/root.tsx
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/thunderstore-api/src/index.ts
- apps/cyberstorm-remix/app/communities/communities.tsx
- packages/dapper-ts/src/methods/package.ts
- packages/thunderstore-api/src/schemas/requestSchemas.ts
- packages/thunderstore-api/src/get/package.ts
- packages/dapper/src/types/methods.ts
- packages/cyberstorm/src/components/Links/LinkingProvider.tsx
- apps/cyberstorm-storybook/LinkLibrary.tsx
- packages/thunderstore-api/src/schemas/objectSchemas.ts
- apps/cyberstorm-remix/app/c/Community.css
- packages/cyberstorm/src/components/Links/Links.tsx
- packages/cyberstorm/src/newComponents/BreadCrumbs/BreadCrumbs.css
- apps/cyberstorm-remix/app/p/packageEdit.css
- packages/dapper/src/types/package.ts
- apps/cyberstorm-remix/app/settings/teams/team/tabs/Profile/Profile.tsx
- packages/cyberstorm/src/newComponents/Switch/Switch.css
- apps/cyberstorm-remix/app/c/community.tsx
- apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx
- packages/dapper-ts/src/index.ts
- packages/thunderstore-api/src/post/package.ts
- packages/thunderstore-api/src/post/packageListing.ts
- packages/thunderstore-api/src/schemas/responseSchemas.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/dapper/src/dapper.ts (1)
packages/dapper/src/types/methods.ts (1)
GetPackagePermissions(63-67)
apps/cyberstorm-remix/app/p/packageEdit.tsx (8)
apps/cyberstorm-remix/app/p/packageListing.tsx (4)
meta(82-128)loader(130-160)clientLoader(163-213)PackageListing(217-834)apps/cyberstorm-remix/app/root.tsx (3)
meta(72-80)clientLoader(82-93)OutletContextShape(65-70)packages/dapper-ts/src/index.ts (1)
DapperTs(35-86)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
useToast(79-87)packages/thunderstore-api/src/post/package.ts (2)
packageDeprecate(50-75)packageUnlist(77-102)packages/thunderstore-api/src/post/packageListing.ts (1)
packageListingUpdate(22-47)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
useStrongForm(24-169)
packages/dapper-fake/src/fakers/package.ts (1)
packages/dapper-fake/src/fakers/utils.ts (1)
setSeed(74-77)
apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx (1)
.yarn/releases/yarn-1.19.0.cjs (1)
domain(100297-100297)
packages/cyberstorm/src/newComponents/Modal/Modal.tsx (1)
packages/cyberstorm-theme/src/components/Modal/Modal.ts (2)
ModalVariants(3-3)ModalSizes(7-7)
packages/dapper-fake/src/index.ts (1)
packages/dapper-fake/src/fakers/package.ts (1)
getFakePackagePermissions(91-124)
apps/cyberstorm-remix/app/p/packageListing.tsx (7)
apps/cyberstorm-remix/app/root.tsx (1)
clientLoader(82-93)packages/dapper-ts/src/methods/package.ts (1)
getPackagePermissions(157-183)packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
useToast(79-87)packages/thunderstore-api/src/get/package.ts (1)
fetchPackagePermissions(9-27)packages/ts-api-react-actions/src/ApiAction.tsx (1)
ApiAction(18-52)packages/thunderstore-api/src/post/packageListing.ts (3)
packageListingReject(76-101)packageListingApprove(49-74)packageListingReport(103-130)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
useStrongForm(24-169)
🪛 Biome (2.1.2)
apps/cyberstorm-remix/app/p/packageListing.css
[error] 49-49: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
align-items is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
⏰ 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). (3)
- GitHub Check: Test
- GitHub Check: Generate visual diffs
- GitHub Check: CodeQL
🔇 Additional comments (7)
packages/dapper/src/dapper.ts (1)
13-13: LGTM on adding the new interface member.Once the parameter order is standardized, this extension to DapperInterface looks consistent with adjacent getters.
apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (2)
137-144: Good change: bubble ParseError to the outer handler.Rethrowing the ParseError after setting submitError/inputErrors ensures onSubmitError and any error boundaries get invoked. Matches the rest of the error flow.
137-143: Verify ParseError shape before casting to InputErrors.Confirm that ParseError.error.formErrors is always present and shape-compatible with InputErrors; otherwise guard the cast.
Would you like a small type guard helper for ParseError that narrows the shape before assigning inputErrors?
packages/dapper-fake/src/fakers/package.ts (1)
91-125: The parameter order in the realgetPackagePermissions(communityId, namespaceId, packageName) matches the fake implementation’s (community, namespace, name), so there’s no mismatch—bridging wrappers aren’t needed. However, adding an explicit return type remains valuable to guard against future shape drift.To tighten typing here:
• Import the proper
PackagePermissionstype (verify its exact export path after the below search).
• Annotate the function’s return asPromise<PackagePermissions>.Once we have the correct import path, update
packages/dapper-fake/src/fakers/package.ts:export const getFakePackagePermissions = async ( community: string, namespace: string, name: string -) +): Promise<PackagePermissions> => { const seed = `${community}-${namespace}-${name}`; setSeed(seed); … };• Ensure you import
PackagePermissionsfrom its actual module (e.g.@thunderstore/dapper/types). [verify_import_path]packages/dapper-fake/src/index.ts (1)
12-14: Import addition looks good.Public API exposure of getPackagePermissions through the fake is consistent with DapperInterface.
apps/cyberstorm-remix/app/p/packageEdit.tsx (1)
87-96: I’ve queued scripts to inspectpackageEdit.tsx, locate any server-side loaders, and check for permission enforcement there. Once I have the outputs, I can confirm whether a server-side gating is missing and propose an appropriate patch.apps/cyberstorm-remix/app/p/packageListing.tsx (1)
560-585: Permissions Await pattern is soundUsing Suspense/Await to optionally resolve a permissions promise and render management tools is correct and won’t block SSR when permissions are undefined.
| import { | ||
| ApiError, | ||
| fetchPackagePermissions, | ||
| packageListingApprove, | ||
| packageListingReject, | ||
| packageListingReport, | ||
| PackageListingReportRequestData, | ||
| RequestConfig, | ||
| } from "@thunderstore/thunderstore-api"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import types as type-only; avoid pulling runtime code for typing
fetchPackagePermissions is only used for typing later. Import its response type instead as a type-only import to prevent bundling runtime code.
-import {
- ApiError,
- fetchPackagePermissions,
- packageListingApprove,
- packageListingReject,
- packageListingReport,
- PackageListingReportRequestData,
- RequestConfig,
-} from "@thunderstore/thunderstore-api";
+import {
+ ApiError,
+ packageListingApprove,
+ packageListingReject,
+ packageListingReport,
+} from "@thunderstore/thunderstore-api";
+import type {
+ PackageListingReportRequestData,
+ RequestConfig,
+ PackagePermissionsResponseData,
+} from "@thunderstore/thunderstore-api";Then update the managementTools signature to use PackagePermissionsResponseData (see comment below).
📝 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.
| import { | |
| ApiError, | |
| fetchPackagePermissions, | |
| packageListingApprove, | |
| packageListingReject, | |
| packageListingReport, | |
| PackageListingReportRequestData, | |
| RequestConfig, | |
| } from "@thunderstore/thunderstore-api"; | |
| import { | |
| ApiError, | |
| packageListingApprove, | |
| packageListingReject, | |
| packageListingReport, | |
| } from "@thunderstore/thunderstore-api"; | |
| import type { | |
| PackageListingReportRequestData, | |
| RequestConfig, | |
| PackagePermissionsResponseData, | |
| } from "@thunderstore/thunderstore-api"; |
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/packageListing.tsx around lines 26 to 34, the
current import pulls in fetchPackagePermissions (a runtime function) only to use
its response type; replace that with a type-only import for the response type
(PackagePermissionsResponseData) from "@thunderstore/thunderstore-api" and
remove fetchPackagePermissions from the runtime import list so no runtime code
is bundled, then update the managementTools signature to use
PackagePermissionsResponseData where it currently references the
fetchPackagePermissions type/return value.
| public getCommunityFilters = getFakeCommunityFilters; | ||
| public getCurrentUser = getFakeCurrentUser; | ||
| public getPackageChangelog = getFakeChangelog; | ||
| public getPackagePermissions = getFakePackagePermissions; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bridge the parameter order mismatch here to prevent swapped args at runtime.
Until the type is standardized, adapt the fake to the interface’s current order by reordering arguments in the assignment. This prevents subtle bugs in environments that call by position.
Apply this diff:
- public getPackagePermissions = getFakePackagePermissions;
+ // DapperInterface currently declares (namespaceId, communityId, packageName).
+ // The fake implementation expects (community, namespace, name).
+ public getPackagePermissions = (
+ namespaceId: string,
+ communityId: string,
+ packageName: string
+ ) => getFakePackagePermissions(communityId, namespaceId, packageName);Once the repo is standardized to (communityId, namespaceId, packageName), you can revert this adapter to a direct assignment.
🏁 Script executed:
#!/bin/bash
set -e
echo "🔍 Locating the DapperInterface definition"
rg -nP --glob '*.ts' 'interface\s+DapperInterface' -C3 .
echo "🔍 Inspecting getPackagePermissions in the interface"
rg -nP --glob '*.ts' 'getPackagePermissions\s*[:=]' -C3 .
echo "🔍 Checking the signature of getFakePackagePermissions"
rg -nP --glob '*.ts' 'function\s+getFakePackagePermissions' -C3 .Length of output: 3495
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "🔍 Printing the imports and context in index.ts"
sed -n '1,30p' packages/dapper-fake/src/index.ts
echo
echo "🔍 Locating getFakePackagePermissions definition in dapper-fake"
rg -nR 'getFakePackagePermissions' -C3 packages/dapper-fake/src
echo
echo "🔍 Locating methods.GetPackagePermissions definition in dapper"
rg -nR 'GetPackagePermissions' -C3 packages/dapper/src/typesLength of output: 1809
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "🔍 Listing the 'fakers' directory in dapper-fake"
ls packages/dapper-fake/src/fakers
echo ""
echo "🔍 Inspecting getFakePackagePermissions signature in fakers"
rg -n -C3 "getFakePackagePermissions" packages/dapper-fake/src/fakers
echo ""
echo "🔍 Listing the 'types' directory in dapper"
ls packages/dapper/src/types
echo ""
echo "🔍 Searching for GetPackagePermissions type in dapper/types"
rg -n -C3 "GetPackagePermissions" packages/dapper/src/typesLength of output: 1753
Adapt getPackagePermissions to match interface signature
We’ve confirmed that:
- DapperInterface (packages/dapper/src/types/methods.ts) defines
GetPackagePermissions = (namespaceId: string, communityId: string, packageName: string) => … - The fake implementation (packages/dapper-fake/src/fakers/package.ts) is declared as
getFakePackagePermissions = async (community: string, namespace: string, name: string) => …
To prevent swapped arguments at runtime, replace the direct assignment in packages/dapper-fake/src/index.ts (around line 31) with an adapter that reorders parameters:
- public getPackagePermissions = getFakePackagePermissions;
+ // Interface: (namespaceId, communityId, packageName)
+ // Fake: (community, namespace, name)
+ public getPackagePermissions = (
+ namespaceId: string,
+ communityId: string,
+ packageName: string
+ ) => getFakePackagePermissions(communityId, namespaceId, packageName);Once DapperInterface standardizes on (communityId, namespaceId, packageName), you can safely revert this back to:
public getPackagePermissions = getFakePackagePermissions;🤖 Prompt for AI Agents
In packages/dapper-fake/src/index.ts around line 31, the direct assignment
public getPackagePermissions = getFakePackagePermissions swaps arguments at
runtime because the DapperInterface expects (namespaceId, communityId,
packageName) while the fake impl is (community, namespace, name); replace the
assignment with an adapter wrapper that accepts (namespaceId, communityId,
packageName) and calls getFakePackagePermissions(communityId, namespaceId,
packageName), returning/awaiting its result so signatures and behavior match the
interface.
| getPackageListings: methods.GetPackageListings; | ||
| getPackageReadme: methods.GetPackageReadme; | ||
| getPackageVersions: methods.GetPackageVersions; | ||
| getPackagePermissions: methods.GetPackagePermissions; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Standardize getPackagePermissions parameter order across layers (type vs implementation mismatch).
The interface addition is fine, but there’s a cross-repo mismatch in parameter order:
- types.GetPackagePermissions: (namespaceId, communityId, packageName)
- dapper-ts implementation and fakes (see other files in this PR): (communityId, namespaceId, packageName)
This will silently swap community/namespace at runtime in some environments. Please standardize on one order (recommend: communityId, namespaceId, packageName to match API path semantics) and align all definitions and call sites.
Two concrete fixes:
- Preferred: change the type definition to (communityId, namespaceId, packageName) and update call sites.
- Bridging: add small wrapper functions in implementations (fake/ts) to map the argument order until types are corrected.
Run to locate the current signature and call sites:
🏁 Script executed:
#!/bin/bash
# Where the type is declared
rg -nP --type=ts 'type\s+GetPackagePermissions\s*=\s*\('
# All implementations/call sites using "getPackagePermissions("
rg -nP --type=ts -C2 '\bgetPackagePermissions\s*\('Length of output: 1271
🏁 Script executed:
#!/bin/bash
# Extract the type definition for GetPackagePermissions
sed -n '55,75p' packages/dapper/src/types/methods.ts
# Extract the implementation signature of getPackagePermissions in dapper-ts
sed -n '150,170p' packages/dapper-ts/src/methods/package.tsLength of output: 1071
Standardize getPackagePermissions parameter order across layers
There’s a confirmed mismatch between the exported type in packages/dapper/src/types/methods.ts and the runtime implementation in packages/dapper-ts/src/methods/package.ts. At the type level (line 63) it’s declared as:
export type GetPackagePermissions = (
namespaceId: string,
communityId: string,
packageName: string
) => Promise<PackagePermissions | undefined>;But the actual function signature (around line 157) is:
export async function getPackagePermissions(
this: DapperTsInterface,
communityId: string,
namespaceId: string,
packageName: string
) { … }Calls in your apps pass (communityId, namespaceId, packageName)—matching the implementation—while TypeScript thinks the first argument is namespaceId, causing them to be swapped at runtime.
To fix:
- In
packages/dapper/src/types/methods.ts(line 63), updateGetPackagePermissionsto:export type GetPackagePermissions = ( communityId: string, namespaceId: string, packageName: string ) => Promise<PackagePermissions | undefined>;
- Review all call sites (e.g. in
apps/cyberstorm-remix/.../packageEdit.tsxandpackageListing.tsx) to ensure they still pass(communityId, namespaceId, packageName). - (Optional bridging) If you prefer not to break downstream code immediately, you can add a thin wrapper in the implementation that swaps args until callers/types are fully migrated.
This alignment ensures the declared type matches the runtime order and prevents silent swaps of community and namespace IDs.
🤖 Prompt for AI Agents
In packages/dapper/src/dapper.ts around line 13 the exported method type for
getPackagePermissions is declared with a different parameter order than the
runtime implementation; update the GetPackagePermissions type in
packages/dapper/src/types/methods.ts (line ~63) to accept (communityId: string,
namespaceId: string, packageName: string) so it matches the implementation, then
audit and adjust call sites (e.g., apps/cyberstorm-remix/.../packageEdit.tsx and
packageListing.tsx) to ensure they pass (communityId, namespaceId, packageName);
alternatively, if you need to avoid immediate downstream changes, add a thin
wrapper in the implementation that swaps the first two args and keep types in
sync for now.
Summary by CodeRabbit
New Features
Style
UX