Separate and organize root.tsx to multiple files #1550
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. WalkthroughReworked root layout to centralize breadcrumbs and ads, augmented global/window typings and outlet context, added hydration-aware initializers for Nitro Ads and beta switch script, and introduced small providers/utilities. New components: NimbusBreadcrumbs, Ads/AdsInit, BetaButtonInit, TooltipProvider, plus breadcrumb helper utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser
participant RemixApp as Remix App
participant Root as root.tsx
participant AdsInit as AdsInit
participant Nitro as nitroAds (window)
participant Beta as BetaButtonInit
participant Router as Router (useMatches)
participant Breadcrumbs as NimbusBreadcrumbs
User->>Browser: Navigate
Browser->>RemixApp: Request + SSR
RemixApp->>Root: loader provides public env/flags
Note over Root: clientLoader.hydrate = true
Browser-->>Root: Hydration
par UI composition
Root->>AdsInit: Mount (if shouldShowAds)
Root->>Beta: Mount
Root->>Router: useMatches()
Router-->>Breadcrumbs: matches
and
Root->>Breadcrumbs: Render
end
AdsInit->>Browser: Inject Nitro Ads script (once)
AdsInit->>Nitro: createAd(...) for containerIds
Beta->>Browser: Inject beta-switch.js (once)
Note over Root,Nitro: Ads render via <Ads/> with AdContainer shells
sequenceDiagram
autonumber
participant Router as Router (useMatches)
participant Breadcrumbs as NimbusBreadcrumbs
participant Helpers as Helpers (community/package)
Router-->>Breadcrumbs: matches[]
Breadcrumbs->>Helpers: getCommunityBreadcrumb(match, isNotLast)
Helpers-->>Breadcrumbs: Element or null
Breadcrumbs->>Helpers: getPackageListingBreadcrumb(listing, edit, dependants)
Helpers-->>Breadcrumbs: Fragment or null
Breadcrumbs-->>Router: Breadcrumb trail (links/spans)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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}
| // 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?
| captureRemixErrorBoundaryError(error); | ||
| } else if (import.meta.env.DEV) { | ||
| // In development mode, we log the error to the console | ||
| console.log(error); |
There was a problem hiding this comment.
Nitpick: console.error is handy for logging errors.
| @@ -0,0 +1,64 @@ | |||
| import { isRecord, NewBreadCrumbsLink } from "@thunderstore/cyberstorm"; | |||
There was a problem hiding this comment.
Commenting here as I can't comment on an empty file: Should breadcrumbs.tsx contain something or be deleted?
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.
Summary by CodeRabbit
New Features
Refactor