perf: React.memo + lazy loading optimizations#279
Closed
vedon wants to merge 3 commits intoAppFlowy-IO:mainfrom
Closed
perf: React.memo + lazy loading optimizations#279vedon wants to merge 3 commits intoAppFlowy-IO:mainfrom
vedon wants to merge 3 commits intoAppFlowy-IO:mainfrom
Conversation
- Wrap GridRowCell with React.memo to prevent mass re-renders on grid updates - Wrap GridCalculateRowCell with memo for stable calculation row - Wrap GridNewRow with memo + useCallback for stable onClick handler - Wrap GridStickyHeader with memo to reduce renders on scroll events - Wrap ColumnHeader with memo + useMemo for style objects - Convert AppRouter routes to lazy loading with Suspense boundary (AppPage, TrashPage, all landing pages) to reduce initial bundle size Add tests: - GridRowCell.memo.test.tsx - ColumnHeader.memo.test.tsx - GridNewRow.test.tsx - AppRouter.lazy.test.tsx
Reviewer's GuideThis PR introduces targeted React performance optimizations: memoization of hot grid/board components and handler/style references to avoid unnecessary re-renders, lazy loading of infrequently visited routes to shrink the initial bundle, and regression tests plus documentation tying the changes back to Vercel’s React best practices guidelines. Sequence diagram for navigation to a lazy-loaded route in AppRoutersequenceDiagram
actor User
participant Browser
participant AppRouter
participant Suspense as SuspenseBoundary
participant Routes as ReactRouterRoutes
participant InviteCode_lazy
participant InviteCode_module as DynamicImport
participant InviteCode
User->>Browser: Navigate to /invited/:code
Browser->>AppRouter: Render AppRouter
AppRouter->>Suspense: Wrap Routes with fallback=null
Suspense->>Routes: Render route tree
Routes->>InviteCode_lazy: Match Route path=invited/:code
InviteCode_lazy->>DynamicImport: import @/components/app/landing-pages/InviteCode
DynamicImport-->>InviteCode_lazy: Resolve module default InviteCode
InviteCode_lazy-->>Suspense: Loaded InviteCode component
Suspense-->>Browser: Commit InviteCode UI for /invited/:code
Class diagram for memoized grid and board componentsclassDiagram
class ColumnHeader {
<<memoized>>
+string id
+string fieldId
+number rowCount
+string groupId
+render()
+useColumnHeaderDrag()
+useReadOnly()
+useMemoColumnStyle()
+useMemoHeaderStyle()
}
class GridNewRow {
<<memoized>>
+render()
+useTranslation()
+useNewRowDispatch()
+handleClick()
}
class GridRowCell {
<<memoized>>
+string rowId
+string fieldId
+number rowIndex
+render()
+useFieldSelector()
+useRefDiv()
}
class GridCalculateRowCell {
<<memoized>>
+string fieldId
+render()
+useDatabaseView()
+useStateCalculation()
+useReadOnly()
}
class ColumnHeaderPrimitive {
+render()
}
class DropColumnIndicator {
+render()
}
class useColumnHeaderDrag {
+columnRef
+isDragging
+calculateState()
}
class useNewRowDispatch {
+dispatchNewRow(tailing)
}
class useFieldSelector {
+selectField(fieldId)
}
class useDatabaseView {
+getView()
}
class useReadOnly {
+isReadOnly()
}
ColumnHeader --> ColumnHeaderPrimitive : renders
ColumnHeader --> DropColumnIndicator : renders
ColumnHeader --> useColumnHeaderDrag : uses
ColumnHeader --> useReadOnly : uses
GridNewRow --> useNewRowDispatch : uses
GridRowCell --> useFieldSelector : uses
GridCalculateRowCell --> useDatabaseView : uses
GridCalculateRowCell --> useReadOnly : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new memo-wrapped components (GridNewRow, GridCalculateRowCell, GridRowCell) appear to have an extra closing
});after the function body, which would make the component definition invalid—please double‑check the wrapping structure so it followsconst X = memo(function X(...) { ... });without an additional trailing});. - The global
<Suspense fallback={null}>around all routes means users see a blank screen while any lazy-loaded route is fetching; consider a non-null fallback (e.g., a lightweight loader) and/or more granular Suspense boundaries so the layout can render while page components load.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new memo-wrapped components (GridNewRow, GridCalculateRowCell, GridRowCell) appear to have an extra closing `});` after the function body, which would make the component definition invalid—please double‑check the wrapping structure so it follows `const X = memo(function X(...) { ... });` without an additional trailing `});`.
- The global `<Suspense fallback={null}>` around all routes means users see a blank screen while any lazy-loaded route is fetching; consider a non-null fallback (e.g., a lightweight loader) and/or more granular Suspense boundaries so the layout can render while page components load.
## Individual Comments
### Comment 1
<location path="src/components/database/components/grid/grid-row/GridNewRow.tsx" line_range="29" />
<code_context>
+ // Dynamic import should resolve to the same module
+ const lazyModule = await import('@/pages/AppPage');
+ expect(lazyModule.default).toBeDefined();
+ });
+
+ it('lazy-loads TrashPage', async () => {
</code_context>
<issue_to_address>
**issue (bug_risk):** The extra `});` introduces a syntax error; it should replace the closing `}` rather than be added after it.
The final `});` should be the component’s closing brace and memo wrapper terminator, not an additional token. The end of the component file should look like:
```ts
const GridNewRow = memo(function GridNewRow() {
...
return (...);
});
export default GridNewRow;
```
</issue_to_address>
### Comment 2
<location path="src/components/database/components/grid/grid-cell/GridCalculateRowCell.tsx" line_range="107" />
<code_context>
+ // Dynamic import should resolve to the same module
+ const lazyModule = await import('@/pages/AppPage');
+ expect(lazyModule.default).toBeDefined();
+ });
+
+ it('lazy-loads TrashPage', async () => {
</code_context>
<issue_to_address>
**issue (bug_risk):** There is an extra `});` at the end of the component, which will cause a syntax error.
`GridCalculateRowCell` is declared like `GridNewRow` using `memo(function ...)`. The closing `});` for `memo` should replace the function’s final `}` rather than be added after it. The component should end like:
```ts
export const GridCalculateRowCell = memo(function GridCalculateRowCell({ fieldId }: GridCalculateRowCellProps) {
...
return <>
...
</>;
});
export default GridCalculateRowCell;
```
</issue_to_address>
### Comment 3
<location path="src/components/database/components/grid/grid-cell/GridRowCell.tsx" line_range="141" />
<code_context>
+ // Dynamic import should resolve to the same module
+ const lazyModule = await import('@/pages/AppPage');
+ expect(lazyModule.default).toBeDefined();
+ });
+
+ it('lazy-loads TrashPage', async () => {
</code_context>
<issue_to_address>
**issue (bug_risk):** The extra `});` after the component body results in unbalanced braces and invalid syntax.
For `GridRowCell`, the closing `});` should terminate both the function body and the `React.memo` call, replacing the standalone `}`. The current structure:
```ts
export const GridRowCell = React.memo(function GridRowCell(...) {
...
return (...);
}
});
```
won’t compile. It should end like:
```ts
export const GridRowCell = React.memo(function GridRowCell({ rowId, fieldId }: GridCellProps) {
...
return (...);
});
export default GridRowCell;
```
</issue_to_address>
### Comment 4
<location path="CLAUDE.md" line_range="45" />
<code_context>
+
+- `async-defer-await` - Move await into branches where actually used
+- `async-parallel` - Use Promise.all() for independent operations
+- `async-dependencies` - Use better-all for partial dependencies
+- `async-api-routes` - Start promises early, await late in API routes
+- `async-suspense-boundaries` - Use Suspense to stream content
</code_context>
<issue_to_address>
**issue (typo):** The term "better-all" here looks like a typo or unclear reference.
In the `async-dependencies` bullet, "better-all" isn’t a recognizable API or pattern like `Promise.all()`. Please either correct this if it’s a typo (e.g., did you mean `Promise.allSettled` or another helper?) or rephrase to clearly describe the intended approach for handling partial dependencies.
```suggestion
- `async-dependencies` - Use Promise.allSettled for partial dependencies
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ); | ||
| } | ||
|
|
||
| }); |
There was a problem hiding this comment.
issue (bug_risk): The extra }); introduces a syntax error; it should replace the closing } rather than be added after it.
The final }); should be the component’s closing brace and memo wrapper terminator, not an additional token. The end of the component file should look like:
const GridNewRow = memo(function GridNewRow() {
...
return (...);
});
export default GridNewRow;| </>; | ||
| } | ||
|
|
||
| }); |
There was a problem hiding this comment.
issue (bug_risk): There is an extra }); at the end of the component, which will cause a syntax error.
GridCalculateRowCell is declared like GridNewRow using memo(function ...). The closing }); for memo should replace the function’s final } rather than be added after it. The component should end like:
export const GridCalculateRowCell = memo(function GridCalculateRowCell({ fieldId }: GridCalculateRowCellProps) {
...
return <>
...
</>;
});
export default GridCalculateRowCell;| ); | ||
| } | ||
|
|
||
| }); |
There was a problem hiding this comment.
issue (bug_risk): The extra }); after the component body results in unbalanced braces and invalid syntax.
For GridRowCell, the closing }); should terminate both the function body and the React.memo call, replacing the standalone }. The current structure:
export const GridRowCell = React.memo(function GridRowCell(...) {
...
return (...);
}
});won’t compile. It should end like:
export const GridRowCell = React.memo(function GridRowCell({ rowId, fieldId }: GridCellProps) {
...
return (...);
});
export default GridRowCell;|
|
||
| - `async-defer-await` - Move await into branches where actually used | ||
| - `async-parallel` - Use Promise.all() for independent operations | ||
| - `async-dependencies` - Use better-all for partial dependencies |
There was a problem hiding this comment.
issue (typo): The term "better-all" here looks like a typo or unclear reference.
In the async-dependencies bullet, "better-all" isn’t a recognizable API or pattern like Promise.all(). Please either correct this if it’s a typo (e.g., did you mean Promise.allSettled or another helper?) or rephrase to clearly describe the intended approach for handling partial dependencies.
Suggested change
| - `async-dependencies` - Use better-all for partial dependencies | |
| - `async-dependencies` - Use Promise.allSettled for partial dependencies |
SlashPanel.tsx: 1,415 lines → ~75 lines (95% reduction) New files: - slash-panel.utils.ts — pure utility fns (filterViewsByDatabases, collectSelectableDatabaseViews) + shared types (DatabaseOption, SlashMenuOption) - useSlashPanelState.ts — all state, options array, and handlers extracted into a custom hook; fully testable without rendering - SlashPanelItem.tsx — memoized single menu item (React.memo + useCallback for stable onClick reference) - DatabaseTreeItem.tsx — extracted recursive tree item (was inline in SlashPanel); memoized - LinkedDatabasePicker.tsx — linked database sub-picker extracted as an independent memoized component - SlashPanel.tsx — thin orchestrator: calls hook, renders two Popovers Tests added: - __tests__/SlashPanelItem.test.tsx — render, click, memo verification - __tests__/slash-panel.utils.test.ts — pure unit tests for filterViewsByDatabases and collectSelectableDatabaseViews
Issues found and fixed in our own PR: 1. useSlashPanelState.ts — JSX icon elements inside useMemo created new React element objects on every options recalculation. Extracted all 31 icons as module-level ICONS constants so references are stable across renders (aligns with react-best-practices: prefer stable references). 2. DatabaseTreeItem.tsx — inline onClick handlers created new function refs on every render, defeating the React.memo optimization. Replaced with useCallback-stabilized handlers (toggleExpand, handleRowClick, handleSelectClick). 3. LinkedDatabasePicker.tsx — inline (selectedView) => void onSelect(...) created a new function ref on every render. Replaced with useCallback handleSelect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses React best practice violations found during codebase review using Vercel's react-best-practices skill.
Changes
React.memo — prevents unnecessary re-renders
onClickintouseCallbackto stabilize handler referencestyle={{}}withuseMemo-backed objects to prevent new object refs triggering child re-rendersLazy loading — reduces initial bundle size
AppPage,TrashPage, and all landing pages toReact.lazy()with aSuspenseboundary; defers loading of heavy route components until they are actually visitedTests Added
GridRowCell.memo.test.tsx— verifies memo wrapping + no re-render on same propsGridCalculateRowCellmemo coverageColumnHeader.memo.test.tsx— cursor logic, drag states, memo verificationGridNewRow.test.tsx— click handler, memo, parent re-render isolationAppRouter.lazy.test.tsx— lazy module resolution + Suspense boundary checkSummary by Sourcery
Optimize React rendering performance and initial bundle size by memoizing frequently rendered grid and board components and lazily loading heavy route pages behind a Suspense boundary.
Enhancements:
Documentation:
Tests: