App Groups - Test memo all the things app groups #311
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Unrelated to this PR, can we move the Apps*.tsx components into a |
…be a shared context tbh
There was a problem hiding this comment.
Pull Request Overview
This PR greatly improves rendering performance for app-related components by introducing memoization and stabilizing callbacks.
- Wrapped key components (
AppsHeader,AppsAdminActionGroup,AppsAccordionListGroup) and subcomponents inReact.memo - Used
useMemo,useCallback, and refs to prevent unnecessary recomputations and re-renders - Added guards in state setters to update only on actual data changes
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/pages/apps/components/AppsHeader.tsx | Wrapped header in React.memo; memoized hasActions and tagChips |
| src/pages/apps/components/AppsAdminActionGroup.tsx | Wrapped admin actions in React.memo; memoized data transforms and callbacks with refs |
| src/pages/apps/components/AppsAccordionListGroup.tsx | Refactored accordion into memoized subcomponents; optimized prop comparisons |
| src/pages/apps/Read.tsx | Memoized initial groups and stabilized toggle/search handlers with useCallback |
Comments suppressed due to low confidence (1)
src/pages/apps/components/AppsAdminActionGroup.tsx:17
- The default for
isExpandedwas changed fromtruetofalse, altering the initial expand behavior. If this wasn’t intentional, restore the previous default or explicitly pass the prop from the parent.
({currentUser, app, onSearchSubmit, onToggleExpand, isExpanded = false}) => {
| }} | ||
| /> | ||
| )); | ||
| }, [app.active_app_tags, navigate]); |
There was a problem hiding this comment.
The navigate dependency isn't used inside the useMemo for tagChips. Consider removing it to avoid needless invalidations.
| }, [app.active_app_tags, navigate]); | |
| }, [app.active_app_tags]); |
Copilot uses AI. Check for mistakes.
| </Stack> | ||
| ); | ||
| }, | ||
| (prevProps, nextProps) => { |
There was a problem hiding this comment.
[nitpick] The custom comparison in React.memo only checks member_list length and IDs; if other fields change, the memo might skip necessary updates. Consider deeper comparisons or adjusting props to avoid stale renders.
Copilot uses AI. Check for mistakes.
| const initialNonOwnerAppGroups = React.useMemo(() => { | ||
| return app?.active_non_owner_app_groups || []; | ||
| }, [app?.active_non_owner_app_groups]); | ||
|
|
||
| React.useEffect(() => { | ||
| setNonOwnerAppGroups(app?.active_non_owner_app_groups || []); | ||
| }, [data]); | ||
| setNonOwnerAppGroups(initialNonOwnerAppGroups); | ||
| }, [initialNonOwnerAppGroups]); |
There was a problem hiding this comment.
[nitpick] The useMemo for initialNonOwnerAppGroups may be unnecessary since it's only used once in the effect. You could depend directly on app.active_non_owner_app_groups in the effect to simplify.
Copilot uses AI. Check for mistakes.
The current structure was a compromise, I don't really want to revisit changes until we have a general rule for the whole frontend app |
No description provided.