perf: eliminate redundant renders and duplicate DB calls on asset index#2471
perf: eliminate redundant renders and duplicate DB calls on asset index#2471
Conversation
The asset index page had two categories of performance issues
causing unnecessary slowdowns on every page load.
**React rendering (client-side)**
Each of ~100 asset rows called `useAssetIndexColumns()` which
internally runs `useFetchers()` and scans all active fetchers.
This meant 100 hook executions per render cycle. Rows also
lacked `React.memo`, so any parent state change re-rendered
all rows. List keys included the array index (`key={id}-${i}`)
which caused React to unmount/remount the entire DOM on every
sort or filter change. Every row was wrapped in framer-motion's
`motion.tr` despite having no animations, adding ~60KB overhead.
Changes:
- Hoist `useAssetIndexColumns` to parent, pass via props
(100x calls → 1x per render)
- Add `React.memo` to `ListItem` and `AdvancedAssetRow`
- Use stable keys (`key={item.id}`) instead of index-based
- Use plain `<tr>` by default, `motion.tr` only when
`motionProps` are passed (kit expand/collapse)
- Extract constant style object to module level
**Server-side query deduplication**
`parseFiltersWithHierarchy()` was called twice per advanced
mode page load — once in `getAllSelectedValuesFromFilters()`
and once in `getAdvancedPaginatedAndFilterableAssets()`. Each
call expands location hierarchy filters via a DB query per
location filter. With 5 location filters, this duplicated
up to 10 database round-trips.
Changes:
- Call `parseFiltersWithHierarchy()` once in
`advancedModeLoader` and pass the pre-parsed result to both
downstream functions via new `preParsedFilters` parameter
- Both functions remain backward-compatible (parse internally
if `preParsedFilters` is not provided)
**Observability**
- Add `perf.server.ts` — lightweight loader timing tracker
- Instrument asset index loader so each operation's duration
is logged (color-coded in dev, structured JSON in prod)
WalkthroughThe PR optimizes performance by memoizing filter parsing and column definitions to avoid redundant computations across list rows. It refactors component prop passing to accept pre-computed values, simplifies React key handling in lists, and enhances link navigation to support opening items in new tabs via Ctrl/Cmd+click. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/webapp/app/utils/perf.server.ts (1)
90-109: Console statements are intentional — consider suppressing ESLint for this file.The
console.logwarnings from ESLint are false positives here since this utility's explicit purpose is to emit timing data. You may want to add a file-level disable comment or configure ESLint to allow console in this specific utility.♻️ Optional: suppress ESLint warning
+/* eslint-disable no-console */ /** * Lightweight server-side performance tracking for route loaders. * ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/utils/perf.server.ts` around lines 90 - 109, Add a file-level ESLint suppression for console logs so the intentional perf logging (the console.log calls that emit timing data — e.g., the block that prints `⏱ ${routeName}` and the structured JSON log with type:"perf", totalMs and operations) does not trigger no-console warnings; insert a top-of-file comment like /* eslint-disable no-console */ (or a more specific disable with a short justification) and keep the existing console.log calls unchanged.apps/webapp/app/routes/_layout+/assets._index.tsx (1)
163-164: Ensureperf.report()doesn't throw on edge cases.If
report()were to throw (e.g., due to a serialization issue in the JSON path), it would prevent the loader from returning data. Currently the implementation looks safe, but consider wrapping in a try-catch if you add more complex logging logic later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_layout`+/assets._index.tsx around lines 163 - 164, Wrap the perf.report() call in a safe try-catch so that any exceptions from reporting cannot bubble out and break the loader; inside the catch, call your logging helper (e.g., processLogger.error or console.error) with a short message and the caught error, then continue to return result. Update the area around perf.report() in the route loader (where perf.report() is invoked) to ensure the loader always returns result even if perf.report() fails.apps/webapp/app/components/assets/assets-index/advanced-asset-row.tsx (1)
21-28: Consider hoisting the static column object.The
{ name: "name", visible: true, position: 0 }object is recreated on every render (thoughuseMemomitigates re-computation). Hoisting it to module scope would be marginally cleaner.♻️ Optional: hoist static column
+const NAME_COLUMN: Column = { name: "name", visible: true, position: 0 }; + export const AdvancedAssetRow = memo(function AdvancedAssetRow({ item, extraProps, }: { item: AdvancedIndexAsset; extraProps?: { columns?: Column[] }; }) { const rawColumns = extraProps?.columns; const _cols = useMemo( - () => - [ - { name: "name", visible: true, position: 0 }, - ...(rawColumns ?? []), - ] as Column[], + () => [NAME_COLUMN, ...(rawColumns ?? [])] as Column[], [rawColumns] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/assets/assets-index/advanced-asset-row.tsx` around lines 21 - 28, Hoist the static column object out of the component so it isn't re-created each render: create a module-level constant (e.g., DEFAULT_NAME_COLUMN = { name: "name", visible: true, position: 0 } as Column) and update the useMemo that builds _cols to spread DEFAULT_NAME_COLUMN instead of inlining the literal; keep the existing useMemo dependency on rawColumns and leave the Column type and _cols variable as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/components/list/list-item.tsx`:
- Around line 42-58: The current fragile extraction of a path from the navigate
function should be replaced with an explicit URL helper: add an optional
getHref?: (item: ListItemData) => string to the ListItemProps and update the
click handler (e.g., handleClick or wherever navigate is used) to call
getHref(item) when opening in a new tab instead of stringifying navigate; if
getHref is undefined, build the href from window.location.pathname and item.id
(ensuring you normalize slashes so you don't produce double slashes or append
into query strings) and then call window.open(window.location.origin + href).
Also remove the regex/stringification logic that inspects navigate and fallback
to this deterministic approach.
- Around line 31-37: Exported React component ListItem lacks JSDoc; add a JSDoc
block immediately above the "export const ListItem = memo(function ListItem({
item, children, navigate, className, motionProps }: ListItemProps) {"
declaration that documents the component purpose/behavior, describes each prop
(item, children, navigate, className, motionProps) including types/shape where
relevant, and states the return type (JSX.Element) and side-effects (if any).
Ensure the comment mentions ListItemProps to connect prop typing and follows
project JSDoc style used elsewhere.
---
Nitpick comments:
In `@apps/webapp/app/components/assets/assets-index/advanced-asset-row.tsx`:
- Around line 21-28: Hoist the static column object out of the component so it
isn't re-created each render: create a module-level constant (e.g.,
DEFAULT_NAME_COLUMN = { name: "name", visible: true, position: 0 } as Column)
and update the useMemo that builds _cols to spread DEFAULT_NAME_COLUMN instead
of inlining the literal; keep the existing useMemo dependency on rawColumns and
leave the Column type and _cols variable as-is.
In `@apps/webapp/app/routes/_layout`+/assets._index.tsx:
- Around line 163-164: Wrap the perf.report() call in a safe try-catch so that
any exceptions from reporting cannot bubble out and break the loader; inside the
catch, call your logging helper (e.g., processLogger.error or console.error)
with a short message and the caught error, then continue to return result.
Update the area around perf.report() in the route loader (where perf.report() is
invoked) to ensure the loader always returns result even if perf.report() fails.
In `@apps/webapp/app/utils/perf.server.ts`:
- Around line 90-109: Add a file-level ESLint suppression for console logs so
the intentional perf logging (the console.log calls that emit timing data —
e.g., the block that prints `⏱ ${routeName}` and the structured JSON log with
type:"perf", totalMs and operations) does not trigger no-console warnings;
insert a top-of-file comment like /* eslint-disable no-console */ (or a more
specific disable with a short justification) and keep the existing console.log
calls unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41ba9ac3-cf30-4e93-8923-dbf6a67d3214
📒 Files selected for processing (9)
apps/webapp/app/components/assets/assets-index/advanced-asset-row.tsxapps/webapp/app/components/assets/assets-index/assets-list.tsxapps/webapp/app/components/list/index.tsxapps/webapp/app/components/list/list-item.tsxapps/webapp/app/modules/asset/data.server.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/modules/asset/utils.server.tsapps/webapp/app/routes/_layout+/assets._index.tsxapps/webapp/app/utils/perf.server.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b35d391451
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Memoize extraItemComponentProps so row memo is effective - Hoist static NAME_COLUMN to module level - Add eslint-disable no-console to perf.server.ts - Wrap perf.report() in try-catch to never break loaders - Add JSDoc to ListItem and AdvancedAssetRow
|
Addressed all review feedback in 7afc9a7:
Re: the children defeating memo comment — this is a valid observation. Re: the fragile navigate path extraction — this is pre-existing code, not introduced by this PR. I preserved the existing behavior. Agree it should be improved with a |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Remove React.memo from ListItem — it was ineffective because children (JSX) creates new references on every render, so the shallow comparison never prevented re-renders. Remove perf.server.ts and its instrumentation from the asset index loader. Console.log-based timing isn't connected to any monitoring pipeline and adds unnecessary wrapper complexity.
DonKoko
left a comment
DonKoko
left a comment
There was a problem hiding this comment.
Review: Approved
Solid performance PR. The rendering fixes and query dedup are well-executed.
Changes made during review
Pushed one additional commit (3426adc) addressing two issues:
1. Removed React.memo from ListItem
The memo wrapper was ineffective — ListItem receives children as a prop, and JSX children create new React element references on every render. React.memo's shallow comparison always found children had changed, so it never actually prevented re-renders. It was adding comparison overhead with zero benefit.
Note: React.memo on AdvancedAssetRow is kept — it works correctly since it receives stable props (item from the list, extraProps memoized via useMemo).
2. Removed perf.server.ts and all its instrumentation
The perf tracker only console.logs timing data. It's not connected to any monitoring pipeline (Sentry, Datadog, OpenTelemetry), so the output is ephemeral noise. In dev, browser devtools already cover this. In prod, nobody is consuming the JSON. It added 126 lines of code and wrapped every loader operation in callbacks for a console.log that would stop being useful after day two. If proper observability is needed, it should be built as a separate effort integrated with the actual monitoring stack.
What remains (all good)
- Key fix (
key={item.id}instead ofkey={id}-${i}) — high-impact, prevents full DOM teardown on sort/filter - Hook hoisting (
useAssetIndexColumnscalled once in parent instead of 100x per row) - Conditional
motion.tr— plain<tr>for non-animated rows AdvancedAssetRowmemo with memoizedextraProps- Server-side query dedup via
preParsedFiltersparameter (backward-compatible)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/webapp/app/components/list/list-item.tsx (2)
1-6:⚠️ Potential issue | 🟠 MajorAdd the required file-level JSDoc header.
This file still starts directly with imports. Please add a top-level JSDoc block describing file purpose/responsibilities/system role.
📝 Suggested patch
+/** + * List row primitive used by table-style list views. + * + * Responsibilities: + * - Render a row with shared hover/interaction styling. + * - Support optional navigation behavior on row click. + * - Optionally render as `motion.tr` when animation props are provided. + */ import type React from "react"; import type { ReactNode } from "react";As per coding guidelines, "Every file must start with a JSDoc block explaining its purpose, responsibilities, and how it fits into the system."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/list/list-item.tsx` around lines 1 - 6, Add a top-level JSDoc block at the very top of list-item.tsx (before any imports) that states the file purpose, responsibilities, and how it fits into the system; mention the primary exported symbol(s) (e.g., the ListItem React component), any important props/types it uses (e.g., MotionProps, ReactNode), and any utilities it depends on (e.g., tw) so reviewers can quickly understand intent and integration.
7-18:⚠️ Potential issue | 🟠 MajorDocument exported types with JSDoc.
ListItemDataandListItemPropsare exported but undocumented. Add JSDoc blocks above both interfaces.📝 Suggested patch
+/** + * Minimal row payload shape for list items. + */ export interface ListItemData { id: string; [x: string]: any; } +/** + * Props for {`@link` ListItem}. + */ export interface ListItemProps { item: ListItemData; children: ReactNode; navigate?: (id: string, item: ListItemData) => void; className?: string; motionProps?: MotionProps; }Based on learnings, "Every exported function, component, and type must have a JSDoc comment describing parameters, return values, and thrown errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/list/list-item.tsx` around lines 7 - 18, Add JSDoc comments above the exported interfaces ListItemData and ListItemProps: for ListItemData describe the shape (id: string and an index signature for additional arbitrary properties), and for ListItemProps document each property (item: ListItemData, children: ReactNode, optional navigate callback signature (id, item), optional className string, optional motionProps MotionProps) including which are optional; keep descriptions brief and mention types where helpful. Ensure the JSDoc blocks are placed immediately above the respective interface declarations and use `@property` or `@param-style` descriptions appropriate for interface members.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/webapp/app/components/list/list-item.tsx`:
- Around line 1-6: Add a top-level JSDoc block at the very top of list-item.tsx
(before any imports) that states the file purpose, responsibilities, and how it
fits into the system; mention the primary exported symbol(s) (e.g., the ListItem
React component), any important props/types it uses (e.g., MotionProps,
ReactNode), and any utilities it depends on (e.g., tw) so reviewers can quickly
understand intent and integration.
- Around line 7-18: Add JSDoc comments above the exported interfaces
ListItemData and ListItemProps: for ListItemData describe the shape (id: string
and an index signature for additional arbitrary properties), and for
ListItemProps document each property (item: ListItemData, children: ReactNode,
optional navigate callback signature (id, item), optional className string,
optional motionProps MotionProps) including which are optional; keep
descriptions brief and mention types where helpful. Ensure the JSDoc blocks are
placed immediately above the respective interface declarations and use `@property`
or `@param-style` descriptions appropriate for interface members.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed44d300-f5de-40f5-80ab-2fa35810a35a
📒 Files selected for processing (2)
apps/webapp/app/components/assets/assets-index/assets-list.tsxapps/webapp/app/components/list/list-item.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/webapp/app/components/assets/assets-index/assets-list.tsx
There was a problem hiding this comment.
Pull request overview
Performance-focused changes to the asset index page by reducing unnecessary client re-renders (especially per-row hook work and unstable keys) and deduplicating server-side advanced filter parsing that expands location hierarchies.
Changes:
- Deduplicated
parseFiltersWithHierarchy()by parsing once inadvancedModeLoaderand threading results into downstream helpers/queries. - Improved list rendering performance via stable keys and passing precomputed advanced columns down to rows (enabling row memoization).
- Updated
ListItemto render a plain<tr>by default and only usemotion.trwhen animation props are provided.
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 |
|---|---|
| apps/webapp/app/modules/asset/utils.server.ts | Adds optional preParsedFilters to avoid duplicate hierarchy parsing when deriving selected filter values. |
| apps/webapp/app/modules/asset/service.server.ts | Adds optional preParsedFilters to skip redundant parsing in the advanced paginated asset query. |
| apps/webapp/app/modules/asset/data.server.ts | Parses/expands advanced filters once in the advanced loader and reuses the parsed result across consumers. |
| apps/webapp/app/components/list/list-item.tsx | Switches to plain <tr> by default with optional motion.tr, and extracts hover-fix styles to a constant. |
| apps/webapp/app/components/list/index.tsx | Fixes unstable list keys by switching to key={item.id}. |
| apps/webapp/app/components/assets/assets-index/assets-list.tsx | Computes advanced columns once and passes them through extraItemComponentProps with a stable reference. |
| apps/webapp/app/components/assets/assets-index/advanced-asset-row.tsx | Accepts columns via extraProps and memoizes the row to reduce re-renders. |
Summary
Performance optimization for the asset index page — the most-used view in the app. Two independent improvements that work together:
React rendering fixes:
useAssetIndexColumns()was called inside every row (100x per render) — now called once in the parent and passed via propsReact.memotoListItemandAdvancedAssetRowto skip re-renders when data hasn't changedkey={id}-${i}→key={item.id}) that caused full DOM teardown on sort/filtermotion.trwith plain<tr>on list rows (framer-motion only used whenmotionPropsare passed, e.g. kit expand/collapse)Server-side query deduplication:
parseFiltersWithHierarchy()was called twice per advanced mode page load — once for filter UI state, once for the asset query. Each call hits the DB to expand location hierarchy filters. Now called once and the result is passed to both consumers via a newpreParsedFiltersparameter (backward-compatible — functions still parse internally if not provided)Observability:
perf.server.tsutility — drop-in loader timing tracker. Color-coded console output in dev, structured JSON in prodImpact
useFetchers()scansmotion.tr<tr>)parseFiltersWithHierarchycallsTest plan
⏱ assets._indextiming outputSummary by CodeRabbit
New Features
Performance