Re-rendering performance improvements#1777
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request refactors four components to improve rendering efficiency. It extracts inline JSX into new memoized child components: Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1777 +/- ##
==========================================
- Coverage 14.53% 14.51% -0.02%
==========================================
Files 334 334
Lines 27394 27428 +34
Branches 854 854
==========================================
Hits 3981 3981
- Misses 23413 23447 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/commonComponents/PackageSearch/PackageSearch.tsx (1)
400-418:⚠️ Potential issue | 🟠 MajorInline callback defeats
CardPackagememoization.Since
CardPackageis now wrapped withmemo(), the inline arrow function forpackageLikeAction(lines 406-416) causes unnecessary re-renders on every parent update. Consider memoizing the like action callback.Proposed fix using useCallback
+ const handlePackageLike = useCallback( + (namespace: string, name: string) => { + if (likeAction) { + likeAction( + ratedPackages.includes(`${namespace}-${name}`), + namespace, + name, + Boolean(currentUser?.username) + ); + } + }, + [likeAction, ratedPackages, currentUser?.username] + );Then at the call site:
<CardPackage key={`${p.namespace}-${p.name}`} packageData={p} isLiked={ratedPackages.includes(`${p.namespace}-${p.name}`)} - packageLikeAction={() => { - if (likeAction) { - likeAction( - ratedPackages.includes(`${p.namespace}-${p.name}`), - p.namespace, - p.name, - Boolean(currentUser?.username) - ); - } - }} + packageLikeAction={() => handlePackageLike(p.namespace, p.name)} />Note: Even this still creates an inline arrow, but it's much lighter. For full optimization, consider changing
packageLikeActionsignature to acceptnamespaceandnameparams, allowing you to passhandlePackageLikedirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cyberstorm-remix/app/commonComponents/PackageSearch/PackageSearch.tsx` around lines 400 - 418, The inline arrow for packageLikeAction causes CardPackage memo to break; fix by creating a memoized handler using useCallback (e.g., handlePackageLike = useCallback((namespace, name) => { const isLiked = ratedPackages.includes(`${namespace}-${name}`); if (likeAction) likeAction(isLiked, namespace, name, Boolean(currentUser?.username)); }, [likeAction, ratedPackages, currentUser?.username])) and then pass packageLikeAction={handlePackageLike} to CardPackage (and update CardPackage to call packageLikeAction(namespace, name) if necessary) so no inline callbacks are created on each render.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cyberstorm/src/newComponents/Card/CardPackage/CardPackage.tsx`:
- Line 46: The prop passed to CardPackage from PackageSearch.tsx
(packageLikeAction) is created inline inside a map, causing a new function
reference each render and defeating memo(); wrap the callback in
React.useCallback in PackageSearch.tsx (import useCallback if missing) to return
a stable function reference and pass that to CardPackage instead of an inline
arrow, and ensure the useCallback dependency array includes only the minimal
reactive values used by the callback so the reference stays stable when
appropriate.
---
Outside diff comments:
In `@apps/cyberstorm-remix/app/commonComponents/PackageSearch/PackageSearch.tsx`:
- Around line 400-418: The inline arrow for packageLikeAction causes CardPackage
memo to break; fix by creating a memoized handler using useCallback (e.g.,
handlePackageLike = useCallback((namespace, name) => { const isLiked =
ratedPackages.includes(`${namespace}-${name}`); if (likeAction)
likeAction(isLiked, namespace, name, Boolean(currentUser?.username)); },
[likeAction, ratedPackages, currentUser?.username])) and then pass
packageLikeAction={handlePackageLike} to CardPackage (and update CardPackage to
call packageLikeAction(namespace, name) if necessary) so no inline callbacks are
created on each render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cd769e8-cf79-4747-8a16-0770fc39d124
📒 Files selected for processing (4)
apps/cyberstorm-remix/app/commonComponents/PackageSearch/PackageSearch.tsxapps/cyberstorm-remix/app/root.tsxpackages/cyberstorm/src/newComponents/AdContainer/AdContainer.tsxpackages/cyberstorm/src/newComponents/Card/CardPackage/CardPackage.tsx
| } | ||
|
|
||
| export function CardPackage(props: Props) { | ||
| export const CardPackage = memo(function CardPackage(props: Props) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all usages of CardPackage to check for inline function props
rg -n -A 10 '<CardPackage' --type ts --type tsx -g '!*.test.*' -g '!*.spec.*'Repository: thunderstore-io/thunderstore-ui
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Search for CardPackage component usage (corrected file type flags)
rg -n -A 10 '<CardPackage' --type ts --type jsxRepository: thunderstore-io/thunderstore-ui
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Also check CardPackage.tsx file directly to confirm memo wrapper exists
wc -l packages/cyberstorm/src/newComponents/Card/CardPackage/CardPackage.tsxRepository: thunderstore-io/thunderstore-ui
Length of output: 149
🏁 Script executed:
#!/bin/bash
# Check the actual CardPackage.tsx around line 46
head -n 50 packages/cyberstorm/src/newComponents/Card/CardPackage/CardPackage.tsx | tail -n 15Repository: thunderstore-io/thunderstore-ui
Length of output: 438
🏁 Script executed:
#!/bin/bash
# Search for CardPackage usage without file type flags
rg '<CardPackage' -A 10 --max-count 20Repository: thunderstore-io/thunderstore-ui
Length of output: 2425
🏁 Script executed:
#!/bin/bash
# Check PackageSearch.tsx specifically around lines mentioned in review (406-416)
head -n 420 packages/cyberstorm/src/newComponents/PackageSearch/PackageSearch.tsx | tail -n 30Repository: thunderstore-io/thunderstore-ui
Length of output: 203
🏁 Script executed:
#!/bin/bash
# Get full context of the CardPackage usage in PackageSearch
rg -B 5 -A 20 'packageLikeAction=.*={() =>' apps/cyberstorm-remix/app/commonComponents/PackageSearch/PackageSearch.tsxRepository: thunderstore-io/thunderstore-ui
Length of output: 206
🏁 Script executed:
#!/bin/bash
# Check if useCallback is used elsewhere in PackageSearch
rg 'useCallback' apps/cyberstorm-remix/app/commonComponents/PackageSearch/PackageSearch.tsxRepository: thunderstore-io/thunderstore-ui
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Search for all CardPackage usages to see if there are other call sites
rg -B 3 -A 15 '<CardPackage' --max-count 50Repository: thunderstore-io/thunderstore-ui
Length of output: 4070
Inline callback defeats memoization effectiveness.
The memo() wrapper is correctly applied, but in PackageSearch.tsx, packageLikeAction is passed as an inline arrow function inside the map loop. This creates a new reference on every render, causing CardPackage to re-render despite memoization.
Since the file already uses useCallback for other callbacks, wrap packageLikeAction in useCallback to stabilize the prop reference and realize the performance benefit of memoization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cyberstorm/src/newComponents/Card/CardPackage/CardPackage.tsx` at
line 46, The prop passed to CardPackage from PackageSearch.tsx
(packageLikeAction) is created inline inside a map, causing a new function
reference each render and defeating memo(); wrap the callback in
React.useCallback in PackageSearch.tsx (import useCallback if missing) to return
a stable function reference and pass that to CardPackage instead of an inline
arrow, and ensure the useCallback dependency array includes only the minimal
reactive values used by the callback so the reference stays stable when
appropriate.
Oksamies
force-pushed
the
03-24-re-rendering_performance_improvements
branch
from
ab7ef65 to
52e5f88
Compare
Oksamies
changed the base branch from
graphite-base/1777
to
04-02-refactor_replace_await_and_suspense_with_suspenseifpromise_globally
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| throw new Response("Community not found", { status: 404 }); | ||
| } | ||
|
|
||
| let cachedFilters: any = null; |
Check failure
Code scanning / ESLint
Disallow the `any` type Error
|
|
||
| let cachedFilters: any = null; | ||
| let cachedCommunityId: string | null = null; | ||
| let cachedSeo: any = null; |
Check failure
Code scanning / ESLint
Disallow the `any` type Error
There was a problem hiding this comment.
Pull request overview
This PR targets React re-rendering performance in the Cyberstorm UI and the cyberstorm-remix app by memoizing frequently-rendered components/blocks and reducing unnecessary recomputation during renders/hydration.
Changes:
- Wrap
CardPackageandAdContainerwithReact.memoto reduce re-renders. - Memoize expensive/stable JSX blocks and outlet context in
app/root.tsx, and extract the ads column into a memoized component. - Add a new search-param blob setter helper and introduce simple client-side caching for PackageSearch server-derived data; extend
PackageOrderto accept an optionalid.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cyberstorm/src/newComponents/Card/CardPackage/CardPackage.tsx | Memoize CardPackage component to reduce re-render frequency. |
| packages/cyberstorm/src/newComponents/AdContainer/AdContainer.tsx | Memoize AdContainer component to reduce re-render frequency. |
| apps/cyberstorm-remix/cyberstorm/utils/searchParamsUtils.ts | Introduce a new setter-based setParamsBlobValue helper and keep a legacy wrapper. |
| apps/cyberstorm-remix/app/root.tsx | Memoize navigation JSX and outlet context; extract ads column into AdsColumn; tweak tooltip delay behavior. |
| apps/cyberstorm-remix/app/commonComponents/PackageSearch/components/PackageOrder.tsx | Add optional id prop to allow customizing/selecting different instances. |
| apps/cyberstorm-remix/app/c/tabs/PackageSearch/PackageSearch.tsx | Cache filters/seo between client navigations to avoid repeated serverLoader() calls. |
| apps/cyberstorm-remix/app/c/community.tsx | Memoize header JSX and outlet context to reduce re-renders in nested routes. |
| export function setParamsBlobValue< | ||
| SearchParamsType, | ||
| K extends keyof SearchParamsType, | ||
| >(setter: React.Dispatch<React.SetStateAction<SearchParamsType>>, key: K) { | ||
| return (v: SearchParamsType[K]) => | ||
| setter((prevBlob) => ({ ...prevBlob, [key]: v })); | ||
| } | ||
|
|
||
| // Keep the old signature wrapper with `oldBlob` around for backward compatibility, | ||
| // although it should be refactored eventually. | ||
| export function setParamsBlobValueLegacy< | ||
| SearchParamsType, | ||
| K extends keyof SearchParamsType, | ||
| >(setter: (v: SearchParamsType) => void, oldBlob: SearchParamsType, key: K) { | ||
| return (v: SearchParamsType[K]) => setter({ ...oldBlob, [key]: v }); |
There was a problem hiding this comment.
setParamsBlobValue now takes (setter, key) and uses React.Dispatch/React.SetStateAction, but this file has no React type imports and existing call sites in the app currently call setParamsBlobValue(setter, oldBlob, key). As written this will fail typechecking/build; either restore the original signature under setParamsBlobValue (and export the new setter-based helper under a new name), or update all callers to use setParamsBlobValueLegacy and add the necessary react type imports.
| export function setParamsBlobValue< | |
| SearchParamsType, | |
| K extends keyof SearchParamsType, | |
| >(setter: React.Dispatch<React.SetStateAction<SearchParamsType>>, key: K) { | |
| return (v: SearchParamsType[K]) => | |
| setter((prevBlob) => ({ ...prevBlob, [key]: v })); | |
| } | |
| // Keep the old signature wrapper with `oldBlob` around for backward compatibility, | |
| // although it should be refactored eventually. | |
| export function setParamsBlobValueLegacy< | |
| SearchParamsType, | |
| K extends keyof SearchParamsType, | |
| >(setter: (v: SearchParamsType) => void, oldBlob: SearchParamsType, key: K) { | |
| return (v: SearchParamsType[K]) => setter({ ...oldBlob, [key]: v }); | |
| import type { Dispatch, SetStateAction } from 'react'; | |
| export function setParamsBlobValue< | |
| SearchParamsType, | |
| K extends keyof SearchParamsType, | |
| >(setter: (v: SearchParamsType) => void, oldBlob: SearchParamsType, key: K) { | |
| return (v: SearchParamsType[K]) => setter({ ...oldBlob, [key]: v }); | |
| } | |
| export function setParamsBlobValueFromStateSetter< | |
| SearchParamsType, | |
| K extends keyof SearchParamsType, | |
| >(setter: Dispatch<SetStateAction<SearchParamsType>>, key: K) { | |
| return (v: SearchParamsType[K]) => | |
| setter((prevBlob) => ({ ...prevBlob, [key]: v })); |
| let cachedFilters: any = null; | ||
| let cachedCommunityId: string | null = null; | ||
| let cachedSeo: any = null; |
There was a problem hiding this comment.
Module-scope caches are typed as any, which defeats type-safety and will likely trip @typescript-eslint/no-explicit-any (enabled via plugin:@typescript-eslint/recommended). Prefer typing these as the concrete loader return types (e.g. CommunityFilters | null and ReturnType<typeof createSeo> | null) so downstream code can rely on filters.sections/etc without implicit any.
| let cachedFilters: any = null; | |
| let cachedCommunityId: string | null = null; | |
| let cachedSeo: any = null; | |
| let cachedFilters: Awaited< | |
| ReturnType<DapperTs["getCommunityFilters"]> | |
| > | null = null; | |
| let cachedCommunityId: string | null = null; | |
| let cachedSeo: ReturnType<typeof createSeo> | null = null; |
| {adContainerIds.map((cid, k_i) => ( | ||
| <AdContainer key={k_i} containerId={cid} /> |
There was a problem hiding this comment.
key={k_i} uses the array index, which can cause unnecessary remounts if the list ever changes. Since adContainerIds is an array of stable IDs, prefer using key={cid} for stable reconciliation.
| {adContainerIds.map((cid, k_i) => ( | |
| <AdContainer key={k_i} containerId={cid} /> | |
| {adContainerIds.map((cid) => ( | |
| <AdContainer key={cid} containerId={cid} /> |
No description provided.