Separate and organize root.tsx to multiple files #1550
Conversation
|
Warning Rate limit exceeded@Oksamies has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
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. WalkthroughThe pull request refactors the Remix app's root layout and breadcrumb architecture. It moves breadcrumb generation from per-page logic into centralized components (NimbusBreadcrumbs, getCommunityBreadcrumb, getPackageListingBreadcrumb, getPackageVersionBreadcrumbs), modularizes ad and tooltip functionality into separate components (Ads, AdsInit, TooltipProvider), and adds a BetaButtonInit component for script injection. The global Window interface is augmented with NIMBUS\_PUBLIC\_ENV, Dapper, and nitroAds. The OutletContextShape type is extended to include domain and dapper properties for downstream consumption. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Oksamies
force-pushed
the
09-23-separate_and_organize_root.tsx_to_multiple_files
branch
from
afadece to
d267286
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
apps/cyberstorm-remix/app/root.tsx
Outdated
| // REMIX TODO: https://remix.run/docs/en/main/route/links | ||
| // export const links: LinksFunction = () => [{ rel: "stylesheet", href: styles }]; | ||
|
|
||
| /* Declaring window types that are present, because of certain parts of the application */ |
There was a problem hiding this comment.
Minor: I find this comment confusing. Certain parts of the application what? Expect the types to be declared?
Question: would it make sense to define the nitroAds part in nitroAds.tsx to compartmentalize ads related stuff in that file?
There was a problem hiding this comment.
Improved the description 👍
Question to question; Would like to separate the Ads components even more? There are two components in that nitroAds.tsx file, which I think is alright, considering those do need to be initialized in different parts of the page (to prevent performance issues while our site is loading).
There was a problem hiding this comment.
👍🏻 on the comment/description.
As for nitroAds, I don't have strong opinion about 1 vs 2 files in this particular case.
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumbs.tsx
Show resolved
Hide resolved
Oksamies
force-pushed
the
09-23-separate_and_organize_root.tsx_to_multiple_files
branch
from
d267286 to
0977167
Compare
![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)
apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx (1)
2-2: Past review comment addressed: Import alias suggestion.A previous review suggested aliasing the import as
Provider as RadixTooltip. While this could improve clarity in files with multiple providers, the current direct import is acceptable in this isolated file context where there's no ambiguity.apps/cyberstorm-remix/app/root.tsx (2)
51-51: Comment clarity could be improved.The comment references "certain parts of the application" without specificity. Based on the past review comment, consider moving nitroAds type to
nitroAds.tsxor clarifying which parts of the application require these declarations.Consider this clearer comment:
-/* Declaring window types that are present, because of certain parts of the application */ +/* Global window type augmentations used throughout the app: + * - NIMBUS_PUBLIC_ENV: injected in Layout for client-side env access + * - Dapper: authentication client instantiated in App component + * - nitroAds: third-party ad service loaded by AdsInit component + */Alternatively, as suggested in the past review, consider moving the
nitroAdsinterface definition tonitroAds.tsxand re-exporting it for use here.
352-353: Use console.error for error logging.In development mode, errors should be logged using
console.errorrather thanconsole.logfor proper error categorization and tooling support.Apply this diff:
} else if (import.meta.env.DEV) { // In development mode, we log the error to the console - console.log(error); + console.error(error); }
🧹 Nitpick comments (11)
apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx (1)
4-10: Consider removing memo wrapper.The
memowrapper provides minimal benefit here since thechildrenprop typically has a new reference on each render, causing memo's shallow comparison to trigger re-renders anyway. The inner RadixProviderlikely handles its own optimization.Apply this diff if you'd like to simplify:
-export const TooltipProvider = memo(function TooltipProvider({ +export function TooltipProvider({ children, }: { children: ReactNode; -}) { +}) { return <Provider delayDuration={80}>{children}</Provider>; -}); +}apps/cyberstorm-remix/app/rootRoute/betaButton.tsx (1)
17-26: Consider checking for existing script tag.The current implementation may append duplicate script tags if the component mounts multiple times (e.g., during development with Fast Refresh). Consider checking if the script already exists:
if (typeof window !== "undefined") { + const existingScript = document.querySelector('script[src="/cyberstorm-static/scripts/beta-switch.js"]'); + if (existingScript) { + hasRun.current = true; + return; + } + const $script = document.createElement("script"); $script.src = "/cyberstorm-static/scripts/beta-switch.js"; $script.setAttribute("async", "true"); document.body.append($script); hasRun.current = true; return () => $script.remove(); }Note: The
typeof window !== "undefined"check on line 17 is redundant sinceuseEffectonly runs on the client.apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumb.tsx (1)
13-41: Consider guarding against multiple simultaneous parameters.The function will render multiple breadcrumb segments if more than one parameter is provided (e.g., both
packageEditPageandpackageDependantsPage). If these parameters are mutually exclusive (only one should be truthy), consider adding a guard or comment to clarify:Additionally, lines 18-29 and 30-41 contain duplicated JSX. Consider extracting a helper:
function renderPackageBreadcrumbLink(match: UIMatch | undefined) { if (!match) return null; return ( <NewBreadCrumbsLink primitiveType="cyberstormLink" linkId="Package" community={match.params.communityId} namespace={match.params.namespaceId} package={match.params.packageId} csVariant="cyber" > {match.params.packageId} </NewBreadCrumbsLink> ); }Then use:
{renderPackageBreadcrumbLink(packageEditPage)}and{renderPackageBreadcrumbLink(packageDependantsPage)}.apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumb.tsx (3)
9-14: Redundant null check after early return.Line 9 already returns
nullif!communityPage, making thecommunityPage &&check on line 12 redundant.Apply this diff to remove the redundant check:
export function getCommunityBreadcrumb( communityPage: UIMatch | undefined, isNotLast: boolean ) { if (!communityPage) return null; return ( <> - {communityPage && - isRecord(communityPage.data) && + {isRecord(communityPage.data) && Object.prototype.hasOwnProperty.call(communityPage.data, "community") ? (
27-39: Simplify type guards and property access.The nested
Object.prototype.hasOwnProperty.callchecks combined withtypeofchecks are verbose. Consider using optional chaining or refining types for cleaner code.Example refactor using optional chaining:
if (isRecord(resolvedValue)) { - label = - Object.prototype.hasOwnProperty.call(resolvedValue, "name") && - typeof resolvedValue.name === "string" - ? resolvedValue.name - : communityPage.params.communityId; - icon = - Object.prototype.hasOwnProperty.call( - resolvedValue, - "community_icon_url" - ) && typeof resolvedValue.community_icon_url === "string" ? ( - <img src={resolvedValue.community_icon_url} alt="" /> - ) : undefined; + label = typeof resolvedValue.name === "string" + ? resolvedValue.name + : communityPage.params.communityId; + icon = typeof resolvedValue.community_icon_url === "string" + ? <img src={resolvedValue.community_icon_url} alt="" /> + : undefined; }
51-56: Simplify nested span structure.The nested
<span><span>structure appears unnecessary for rendering the non-link breadcrumb item.Consider simplifying to:
) : ( - <span> - <span> - {icon} - {label} - </span> - </span> + <span> + {icon} + {label} + </span> );apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx (3)
7-7: Incorrect component description comment.The comment states "Layout component to provide common UI around all pages" but this is
NimbusBreadcrumbs, a breadcrumb-specific component, not a layout component.Apply this diff:
-/** Layout component to provide common UI around all pages */ +/** Breadcrumbs component that renders navigation trail based on current route matches */ export const NimbusBreadcrumbs = memo(function NimbusBreadcrumbs(props: {
12-53: Multiple sequential find operations on matches array.The component performs 14 separate
.find()operations on thematchesarray. While readable, this could be optimized with a single pass using a lookup map if performance becomes a concern.Current approach is clear and maintainable. Consider optimization only if profiling reveals performance issues:
const matchesMap = useMemo(() => { const map = new Map<string, UIMatch>(); for (const match of matches) { map.set(match.id, match); } return map; }, [matches]); const userSettingsPage = matchesMap.get("settings/user/Settings"); // ... etc
131-148: Complex nested ternary for Communities breadcrumb.The deeply nested ternary logic for determining whether to render a link or span is difficult to parse at a glance.
Consider extracting to a clearer conditional structure:
{/* Communities page */} - {communitiesPage || - communityPage || - packageDependantsPage || - packageTeamPage ? ( - communityPage || packageDependantsPage || packageTeamPage ? ( + {(communitiesPage || communityPage || packageDependantsPage || packageTeamPage) ? ( + (communityPage || packageDependantsPage || packageTeamPage) ? ( <NewBreadCrumbsLink primitiveType="cyberstormLink" linkId="Communities" csVariant="cyber" > Communities </NewBreadCrumbsLink> ) : ( <span> <span>Communities</span> </span> ) ) : null}Or better yet, extract to a helper variable:
const showCommunities = communitiesPage || communityPage || packageDependantsPage || packageTeamPage; const communityIsLink = communityPage || packageDependantsPage || packageTeamPage; // Then in JSX: {showCommunities ? ( communityIsLink ? ( <NewBreadCrumbsLink ...>Communities</NewBreadCrumbsLink> ) : ( <span><span>Communities</span></span> ) ) : null}apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx (1)
39-61: Redundant null checks inside forEach loop.The
nitroAdsandnitroAds.createAdchecks on lines 41-42 are redundant since the outerifon line 40 already verifies these conditions.Apply this diff to remove the redundant checks:
useEffect(() => { if (nitroAds !== undefined && nitroAds.createAd !== undefined) { adContainerIds.forEach((cid) => { - if (nitroAds !== undefined && nitroAds.createAd !== undefined) { - nitroAds.createAd(cid, { - demo: false, - format: "display", - refreshLimit: 0, - refreshTime: 30, - renderVisibleOnly: true, - refreshVisibleOnly: true, - sizes: [["300", "250"]], - report: { - enabled: true, - wording: "Report Ad", - position: "bottom-right", - }, - mediaQuery: "(min-width: 1475px) and (min-height: 400px)", - }); - } + nitroAds.createAd(cid, { + demo: false, + format: "display", + refreshLimit: 0, + refreshTime: 30, + renderVisibleOnly: true, + refreshVisibleOnly: true, + sizes: [["300", "250"]], + report: { + enabled: true, + wording: "Report Ad", + position: "bottom-right", + }, + mediaQuery: "(min-width: 1475px) and (min-height: 400px)", + }); }); } }, [nitroAds]);apps/cyberstorm-remix/app/root.tsx (1)
242-250: Simplify nested ternary for shouldShowAds.The deeply nested ternary operators make this logic difficult to read and maintain.
Consider extracting to a more readable structure:
- const shouldShowAds = location.pathname.startsWith("/teams") - ? false - : location.pathname.startsWith("/settings") - ? false - : location.pathname.startsWith("/package/create") - ? false - : location.pathname.startsWith("/tools") - ? false - : true; + const shouldShowAds = + !location.pathname.startsWith("/teams") && + !location.pathname.startsWith("/settings") && + !location.pathname.startsWith("/package/create") && + !location.pathname.startsWith("/tools");Or even more explicitly:
const adsHiddenPaths = ["/teams", "/settings", "/package/create", "/tools"]; const shouldShowAds = !adsHiddenPaths.some(path => location.pathname.startsWith(path));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/cyberstorm-remix/app/root.tsx(7 hunks)apps/cyberstorm-remix/app/rootRoute/betaButton.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumb.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumb.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx (1)
packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
Provider(52-77)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx (2)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumb.tsx (1)
getCommunityBreadcrumb(5-64)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumb.tsx (1)
getPackageListingBreadcrumb(4-44)
apps/cyberstorm-remix/app/root.tsx (2)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx (1)
NimbusBreadcrumbs(8-197)apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx (1)
Ads(66-78)
⏰ 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: Generate visual diffs
- GitHub Check: Test
- GitHub Check: CodeQL
🔇 Additional comments (6)
apps/cyberstorm-remix/app/rootRoute/betaButton.tsx (1)
15-16: Verify hydration guard logic.The guard condition
if ((!startsHydrated.current && isHydrated) || hasRun.current) return;prevents script execution when the component starts not-hydrated and becomes hydrated (typical SSR→hydration flow). This means the script only runs in client-only rendering scenarios.If the intent is to run the script after SSR hydration, the condition should likely be:
if (startsHydrated.current || hasRun.current) return;This would run the script once during the initial client mount after SSR, but not if already hydrated (client-only render with fast refresh).
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumb.tsx (1)
4-10: LGTM!Function signature and null-guard logic are clear and correct.
apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx (2)
21-23: Clarify hydration guard logic.The condition
if (!startsHydrated.current && isHydrated) return;is not immediately clear. This returns early (skipping script injection) when the component starts non-hydrated but becomes hydrated, which seems intentional to avoid double-loading. However, the logic could be clearer with a comment or variable name.Based on the comment on lines 18-20, this guard prevents double script injection in development StrictMode. However, the condition seems to say "skip if we started server-side and are now hydrated". Can you verify this is the intended behavior?
Consider adding an explanatory comment:
useEffect(() => { + // Skip script injection if component started on server and is now hydrating + // to avoid double-injection in StrictMode during development if (!startsHydrated.current && isHydrated) return;
73-73: Using array index as key is acceptable here.While using array indices as keys is generally discouraged in React, it's acceptable in this case since
adContainerIdsis a static array that won't be reordered or modified.apps/cyberstorm-remix/app/root.tsx (2)
173-174: Correct usage of clientLoader.hydrate flag.Setting
clientLoader.hydrate = trueensures the client loader runs on initial page load, which is appropriate for session validation and current user fetching.
302-309: Clean integration of refactored components.The integration of
NimbusBreadcrumbs,Ads, andAdsInitcomponents successfully separates concerns from the root layout while maintaining the same functionality.
Oksamies
force-pushed
the
09-23-separate_and_organize_root.tsx_to_multiple_files
branch
from
0977167 to
2086ab9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/cyberstorm-remix/app/root.tsx (2)
51-51: Clarify the comment.The comment is vague. Consider being more specific about which parts of the application require these types.
Per past review feedback, you might also consider moving the
nitroAdstype declaration tonitroAds.tsxto compartmentalize ad-related code.
352-353: Use console.error for error logging.In development mode, use
console.errorinstead ofconsole.logfor better visibility and proper error categorization in browser dev tools.Apply this diff:
- // In development mode, we log the error to the console - console.log(error); + // In development mode, we log the error to the console + console.error(error);
🧹 Nitpick comments (1)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumb.tsx (1)
14-38: Consider simplifying property access checks.The repeated use of
Object.prototype.hasOwnProperty.call()is verbose. Consider using optional chaining or a helper function for cleaner code.Example with optional chaining:
- label = - Object.prototype.hasOwnProperty.call(resolvedValue, "name") && - typeof resolvedValue.name === "string" - ? resolvedValue.name - : communityPage.params.communityId; + label = + typeof resolvedValue.name === "string" + ? resolvedValue.name + : communityPage.params.communityId; icon = - Object.prototype.hasOwnProperty.call( - resolvedValue, - "community_icon_url" - ) && typeof resolvedValue.community_icon_url === "string" ? ( + typeof resolvedValue.community_icon_url === "string" ? ( <img src={resolvedValue.community_icon_url} alt="" /> ) : undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/cyberstorm-remix/app/root.tsx(7 hunks)apps/cyberstorm-remix/app/rootRoute/betaButton.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumb.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumb.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx
- apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cyberstorm-remix/app/root.tsx (2)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx (1)
NimbusBreadcrumbs(8-197)apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx (1)
Ads(66-78)
⏰ 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: Test
🔇 Additional comments (4)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumb.tsx (1)
4-44: LGTM!The breadcrumb helper correctly handles the three package-related page types with appropriate conditional rendering. The logic is clear and consistent with the breadcrumb system.
apps/cyberstorm-remix/app/rootRoute/betaButton.tsx (1)
7-30: LGTM!The script injection pattern is consistent with
AdsInitand correctly handles hydration with appropriate guards against multiple executions.apps/cyberstorm-remix/app/root.tsx (2)
38-46: Well-structured refactoring.The extraction of breadcrumbs, ads, and initialization logic into separate modules improves maintainability. The centralized approach with
NimbusBreadcrumbs,Ads, andAdsInitis cleaner than per-page scaffolding.Also applies to: 302-302, 306-309
174-174: Verify clientLoader.hydrate behavior.Setting
clientLoader.hydrate = trueforces the client loader to run on initial page load. Ensure this doesn't cause unnecessary data fetching or performance issues, especially if the server loader already provides the needed data.
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumb.tsx
Outdated
Show resolved
Hide resolved
Oksamies
force-pushed
the
09-23-separate_and_organize_root.tsx_to_multiple_files
branch
2 times, most recently
from
92618d3 to
f05a10c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cyberstorm-remix/app/root.tsx (1)
290-298: Harden inline JSON against </script> parsing.Embed-safe the payload to prevent premature script termination.
- dangerouslySetInnerHTML={{ - __html: `window.NIMBUS_PUBLIC_ENV = ${JSON.stringify( - resolvedEnvVars - )}`, - }} + dangerouslySetInnerHTML={{ + __html: `window.NIMBUS_PUBLIC_ENV = ${JSON.stringify(resolvedEnvVars).replace(/</g, "\\u003C")}`, + }}
♻️ Duplicate comments (1)
apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx (1)
2-2: Keep explicit Radix alias for clarity (matches existing patterns).Prefer an alias to make the source clear and avoid generic “Provider”.
-import { Provider } from "@radix-ui/react-tooltip"; +import { Provider as RadixTooltipProvider } from "@radix-ui/react-tooltip"; … - return <Provider delayDuration={80}>{children}</Provider>; + return <RadixTooltipProvider delayDuration={80}>{children}</RadixTooltipProvider>;Also applies to: 9-9
🧹 Nitpick comments (8)
apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx (1)
4-10: Forward Radix Provider props; keep 80ms as default.This increases flexibility without new wrappers.
-export const TooltipProvider = memo(function TooltipProvider({ - children, -}: { - children: ReactNode; -}) { - return <Provider delayDuration={80}>{children}</Provider>; -}); +export const TooltipProvider = memo(function TooltipProvider( + { children, ...props }: { children: ReactNode } & React.ComponentProps<typeof Provider> +) { + return <Provider delayDuration={props.delayDuration ?? 80} {...props}>{children}</Provider>; +});apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumbs.tsx (1)
14-21: Optional: provide errorElement for Await.Gracefully show the communityId if loading fails.
- <Suspense + <Suspense fallback={ <span> <span>Loading...</span> </span> } > - <Await resolve={communityPage.data.community}> + <Await + resolve={communityPage.data.community} + errorElement={ + <span> + <span>{communityPage.params.communityId}</span> + </span> + } + >apps/cyberstorm-remix/app/root.tsx (2)
40-40: Import Toast from the package’s public API (avoid deep src path).Reduces coupling to internal paths.
-import Toast from "@thunderstore/cyberstorm/src/newComponents/Toast"; +import { Toast } from "@thunderstore/cyberstorm";Confirm the package re-exports Toast; if not, consider adding it to the library’s public API.
Also applies to: 301-301
250-259: Simplify ad-visibility logic.Cleaner and easier to extend.
- const shouldShowAds = location.pathname.startsWith("/teams") - ? false - : location.pathname.startsWith("/settings") - ? false - : location.pathname.startsWith("/package/create") - ? false - : location.pathname.startsWith("/tools") - ? false - : true; + const hidePrefixes = ["/teams", "/settings", "/package/create", "/tools"]; + const shouldShowAds = !hidePrefixes.some((p) => location.pathname.startsWith(p));apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumbs.tsx (1)
13-17: Trim redundant wrapper spans.Nested span>span adds DOM without benefit.
- {packageListingPage ? ( - <span> - <span>{packageListingPage.params.packageId}</span> - </span> - ) : null} + {packageListingPage ? <span>{packageListingPage.params.packageId}</span> : null} … - {packageDependantsPage ? ( - <NewBreadCrumbsLink + {packageDependantsPage ? ( + <NewBreadCrumbsLink primitiveType="cyberstormLink" linkId="Package" community={packageDependantsPage.params.communityId} namespace={packageDependantsPage.params.namespaceId} package={packageDependantsPage.params.packageId} csVariant="cyber" > {packageDependantsPage.params.packageId} </NewBreadCrumbsLink> ) : null}Also applies to: 30-41
apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx (3)
6-6: Update import after renaming getPackageVersionBreadcrumbs.Keep import path consistent with corrected filename.
-import { getPackageVersionBreadcrumbs } from "./breadcrumbs/getPackgeVersionBreadcrumbs"; +import { getPackageVersionBreadcrumbs } from "./breadcrumbs/getPackageVersionBreadcrumbs";
13-58: Pre-index matches by id to reduce repeated scans.Micro-optimization and readability improvement.
- const userSettingsPage = matches.find((m) => m.id === "settings/user/Settings"); + const byId = Object.fromEntries(matches.map((m) => [m.id, m])); + const userSettingsPage = byId["settings/user/Settings"]; // …apply likewise for all other lookups…
72-76: Remove unnecessary nested spans.Tightens markup without changing visuals.
Example:
- <span> - <span>Settings</span> - </span> + <span>Settings</span>Repeat for the other plain-text crumbs in this file.
Also applies to: 80-83, 130-134, 176-179, 181-184, 191-199, 201-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/cyberstorm-remix/app/root.tsx(7 hunks)apps/cyberstorm-remix/app/rootRoute/betaButton.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumbs.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumbs.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackgeVersionBreadcrumbs.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx
- apps/cyberstorm-remix/app/rootRoute/betaButton.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/cyberstorm-remix/app/root.tsx (2)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx (1)
NimbusBreadcrumbs(9-207)apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx (1)
Ads(64-76)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx (3)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumbs.tsx (1)
getCommunityBreadcrumb(5-63)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumbs.tsx (1)
getPackageListingBreadcrumb(4-44)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackgeVersionBreadcrumbs.tsx (1)
getPackageVersionBreadcrumbs(4-47)
apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx (1)
packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
Provider(52-77)
🪛 GitHub Actions: Visual diff
apps/cyberstorm-remix/app/root.tsx
[error] 1-1: Build failed: Could not resolve './+types/root' from 'app/root.tsx' (UNRESOLVED_IMPORT). Command: yarn workspace @thunderstore/cyberstorm-remix build
⏰ 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: Build
🔇 Additional comments (4)
apps/cyberstorm-remix/app/root.tsx (3)
87-93: Outlet context additions look good.The new domain and dapper entries are supplied in App’s context.
Ensure consumers use the updated OutletContextShape.
351-362: ErrorBoundary DEV logging via console.error — good.Matches the earlier feedback; production still captures to Sentry.
37-37: Fix CI: avoid generated “./+types/root” import.Use React Router’s public types; removes dependency on typegen in build.
-import { type Route } from "./+types/root"; +import type { ClientLoaderFunctionArgs } from "react-router"; … -export async function clientLoader({ request }: Route.ClientLoaderArgs) { +export async function clientLoader({ request }: ClientLoaderFunctionArgs) {Run build again after this change; it should resolve the “UNRESOLVED_IMPORT” error reported by the Visual diff workflow.
Also applies to: 127-128
apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx (1)
60-206: Centralized breadcrumbs — LGTM.Structure is clear and composes well with helpers.
Quick-check route ids to avoid typos:
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumbs.tsx
Show resolved
Hide resolved
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageVersionBreadcrumbs.tsx
Show resolved
Hide resolved
Oksamies
force-pushed
the
09-23-separate_and_organize_root.tsx_to_multiple_files
branch
from
f05a10c to
14323b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cyberstorm-remix/app/root.tsx (1)
142-142: Fix typo in error message."Enviroment" should be "Environment".
Apply this diff:
- "Enviroment variables did not load correctly, please hard refresh page" + "Environment variables did not load correctly, please hard refresh page"
🧹 Nitpick comments (4)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageVersionBreadcrumbs.tsx (1)
24-26: Consider simplifying the nested span structure.Multiple instances of
<span><span>{value}</span></span>could be reduced to single spans unless the outer wrapper is required for styling or CSS selectors.Apply this diff if the outer span isn't needed:
) : null} - <span> - <span>{packageVersionPage.params.packageVersion}</span> - </span> + <span>{packageVersionPage.params.packageVersion}</span> </>Also applies to lines 32-34, 35-37, and 38-42.
Also applies to: 32-42
apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx (1)
4-10: Consider if memo is beneficial for this simple wrapper.The
memooptimization may not provide value since thechildrenprop will change frequently, causing re-renders regardless. However, this is a minor optimization concern and functionally correct.apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx (1)
70-72: Use containerId as key instead of index.Using array indices as keys can cause issues if the array is reordered. Since
cidis already unique and stable, prefer it as the key.Apply this diff:
- {adContainerIds.map((cid, k_i) => ( - <AdContainer key={k_i} containerId={cid} /> + {adContainerIds.map((cid) => ( + <AdContainer key={cid} containerId={cid} /> ))}apps/cyberstorm-remix/app/root.tsx (1)
241-247: Remove commented code.Legacy breadcrumb logic is now replaced by
NimbusBreadcrumbs. Remove the commented code block—it's preserved in git history if needed.Apply this diff:
const location = useLocation(); - // const splitPath = location.pathname.split("/"); - // const isSubPath = splitPath.length > 4; - // const enableCommunitiesBreadCrumb = - // location.pathname === "/communities" || location.pathname.startsWith("/c/"); - // const isPackageListingSubPath = - // splitPath.length > 5 && splitPath[1] === "c" && splitPath[3] === "p"; const matches = useMatches();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/cyberstorm-remix/app/root.tsx(7 hunks)apps/cyberstorm-remix/app/rootRoute/betaButton.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumbs.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumbs.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageVersionBreadcrumbs.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx(1 hunks)apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx
- apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getPackageListingBreadcrumbs.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/app/root.tsx (2)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs.tsx (1)
NimbusBreadcrumbs(9-207)apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx (1)
Ads(64-76)
apps/cyberstorm-remix/app/rootRoute/tooltipProvider.tsx (1)
packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
Provider(52-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (5)
apps/cyberstorm-remix/app/rootRoute/breadcrumbs/getCommunityBreadcrumbs.tsx (1)
5-69: LGTM! Robust implementation with proper fallbacks.The function handles edge cases well with type guards, provides a label fallback to prevent empty breadcrumbs, and includes image optimization attributes.
apps/cyberstorm-remix/app/rootRoute/betaButton.tsx (1)
7-30: LGTM! Hydration guards work as intended.The ref-based hydration tracking prevents duplicate script injection during SSR-to-client hydration, only running when the component starts in a hydrated state. The cleanup on unmount is properly handled.
apps/cyberstorm-remix/app/rootRoute/nitroAds.tsx (1)
14-62: LGTM! Ad initialization properly coordinated.The two-effect pattern correctly separates script loading from ad creation, with proper hydration guards and cleanup. The redundant null checks mentioned in previous reviews have been addressed.
apps/cyberstorm-remix/app/root.tsx (2)
51-85: Excellent documentation of global types.The JSDoc comments clearly explain the purpose of each Window property and their usage context. This helps prevent confusion about the extended global surface.
354-382: LGTM! Error boundary handles environments correctly.Proper Sentry integration for production and
console.errorfor development logging. The inline comment clarifies the behavior.
Oksamies
force-pushed
the
09-23-separate_and_organize_root.tsx_to_multiple_files
branch
from
14323b9 to
d7b40f4
Compare
| } from "cyberstorm/security/publicEnvVariables"; | ||
| import { StorageManager } from "@thunderstore/ts-api-react/src/storage"; | ||
| import type { Route } from "./+types/root"; | ||
| import { type Route } from "./+types/root"; |
There was a problem hiding this comment.
When trying to run the branch locally, I get:
My IDE says this path refers to .react-router folder, which is currently .gitignored.
There was a problem hiding this comment.
AFAIK the types should be gitignored, as those are generated by the build/dev server. But I'll double check, as I'm confused why you'd get that issue.
2 participants
Summary by CodeRabbit
New Features
Refactor