Minor performance optimizations for large repeating groups #3606
Conversation
olemartinorg
added
kind/other
|
/publish |
PR release:
|
olemartinorg
force-pushed
the
refactor/performance-fixes2
branch
2 times, most recently
from
8862d98 to
b83a7f7
Compare
olemartinorg
changed the base branch from
main
to
chore/navigate-to-component-fix
📝 WalkthroughWalkthroughRefactors QueryContext to remove processing and UI overrides, returning raw query data. Moves per-provider data transformations into queryFns and adjusted typings. Adds a mutation-based loading gate in DataModelsProvider. Refactors RepeatingGroup internals to read directly from the Zustand store, changes RowToDisplay props, and tightens memoization in several hooks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
… either select() in tanstack query or by wrapping the query function.
…memoization. In ssb/ra0485-01 each click on the 'add new row' button seems to re-render this and re-render the entire nodes context.
…t have to re-render these tools for them them to work correctly next time they're called.
…ssing a new object
…useMemo() call that was here before. Leaving it in seems to fix things, but I suspect there is a react-compiler issue at the core of this. I tried opting out of react-compiler, but that didn't work as expected either.
…ntirely and clear all state. Saving is trigged on unmount, but that saving isn't always done when re-mounting again, so DataModelsProvider would end up getting outdated initial form data for the next render.
…ng the entire application if the component never re-renders. Also, if you're unlucky you'll suddenly get the loader again during normal form filling because this re-rendered and suddenly found a (normal) mutation was in progress. Fixing this seems to make both tests stable again: - components.ts 'should be possible to change language back and forth and reflect the change in the UI', which was the initially flaky one that made me write this code - validation.ts 'Required field validation should be visible on submit, not on blur', which now failed sometimes because you can't blur a field that doesn't have focus (and it didn't have focus any more because the loader flashed by for a short while when mutating).
olemartinorg
force-pushed
the
refactor/performance-fixes2
branch
from
b15be75 to
b1a2275
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/form/layoutSettings/LayoutSettingsContext.tsx (1)
84-91: Type-safe omitUndefined implementation (fixes potential TS errors in strict mode).The current reduce initializer is
{}, which is not typed; indexing into it with arbitrary keys will error under stricter TS settings.Apply this diff:
-function omitUndefined<T extends { [K: string]: unknown }>(obj: T): Partial<T> { - return Object.keys(obj).reduce((newObj, key) => { - if (obj[key] !== undefined) { - newObj[key] = obj[key]; - } - return newObj; - }, {}); -} +function omitUndefined<T extends Record<string, unknown>>(obj: T): Partial<T> { + const result: Partial<T> = {}; + (Object.keys(obj) as Array<keyof T>).forEach((key) => { + if (obj[key] !== undefined) { + result[key] = obj[key]; + } + }); + return result; +}
🧹 Nitpick comments (9)
src/layout/RepeatingGroup/utils.ts (1)
149-176: useMemoDeepEqual here should cut re-renders; ensure the returned object stays immutable.This change will stabilize the reference when the computed payload is deep-equal across renders. One caveat: if a consumer mutates the returned object, future deep-equality checks may become unreliable. If you’ve had accidental mutations in this area before, consider freezing the result in dev to catch it early.
Optional dev-only guard you could wrap behind a DEV flag:
return { ...row, hidden: evalBool({ expr: hiddenRow, ...baseProps }) ?? false, textResourceBindings: trb ? { edit_button_close: evalString({ expr: trb.edit_button_close, ...baseProps }), edit_button_open: evalString({ expr: trb.edit_button_open, ...baseProps }), save_and_next_button: evalString({ expr: trb.save_and_next_button, ...baseProps }), save_button: evalString({ expr: trb.save_button, ...baseProps }), } : undefined, edit: edit ? { alertOnDelete: evalBool({ expr: edit.alertOnDelete, ...baseProps }), editButton: evalBool({ expr: edit.editButton, ...baseProps, defaultValue: true }), deleteButton: evalBool({ expr: edit.deleteButton, ...baseProps, defaultValue: true }), saveAndNextButton: evalBool({ expr: edit.saveAndNextButton, ...baseProps }), saveButton: evalBool({ expr: edit.saveButton, ...baseProps, defaultValue: true }), } : undefined, }; + // if (__DEV__) Object.freeze(result);src/features/form/layout/LayoutsContext.tsx (1)
60-72: Memoizing lookups off the query result is solid; minor dependency/type tidy-ups recommended.
- The lookups only depend on layouts, so using utils.data?.layouts as the dependency avoids unnecessary recompute if other fields change.
- Consider renaming data to dataWithLookups for clarity in the return expression.
- Today LayoutContextValue doesn’t include lookups, yet the context value returned from useLayoutQuery now does. TS will infer the wider type via createQueryContext, but making lookups explicit in LayoutContextValue improves consistency.
Apply within this hunk:
- const data = useMemo(() => { - if (utils.data) { - return { - ...utils.data, - lookups: makeLayoutLookups(utils.data.layouts), - }; - } - - return utils.data; - }, [utils.data]); - - return { ...utils, data }; + const dataWithLookups = useMemo(() => { + const layouts = utils.data?.layouts; + if (!layouts) { + return utils.data; + } + return { ...utils.data, lookups: makeLayoutLookups(layouts) }; + }, [utils.data?.layouts]); + + return { ...utils, data: dataWithLookups };Outside this hunk (for typing clarity), you can update the interface:
// near line 24 import type { LayoutLookups } from 'src/features/form/layout/makeLayoutLookups'; export interface LayoutContextValue { layouts: ILayouts; hiddenLayoutsExpressions: IHiddenLayoutsExternal; expandedWidthLayouts: IExpandedWidthLayouts; lookups: LayoutLookups; // new }src/features/datamodel/DataModelsProvider.tsx (1)
260-271: One-time pending-save gate prevents 409s after unmount-triggered autosave — confirmed. All occurrences of the['saveFormData']mutationKey inFormDataWrite.tsx(lines 124, 177, 352, 603) andDataModelsProvider.tsx(line 260) align perfectly.Nitpick (optional): if
FormDataWritecan ever unmount without unmounting its parent (e.g.BlockUntilLoaded), this one-shot guard won’t catch a subsequent pending save on the same mount. Consider resettinghasPassedMutationCheck.currentwhile initial data/schema loading is still in progress to cover that scenario.src/layout/RepeatingGroup/Providers/RepeatingGroupContext.tsx (1)
474-484: Pagination page calculation uses current store page — minor micro-optimization possible.Current approach is correct. If you want to shave a read, cache store.getState() in a local variable in getPaginationState; impact is negligible, so up to you.
- const getPaginationState = () => - producePaginationState(store.getState().currentPage, pagination, getState().visibleRows); + const getPaginationState = () => { + const { currentPage } = store.getState(); + return producePaginationState(currentPage, pagination, getState().visibleRows); + };src/core/contexts/queryContext.tsx (1)
39-54: Minor readability nits: avoid shadowing and unnecessary memo.
- The variable name rest is used both for props (outer scope) and for the query result spread (inner scope). Rename the inner one to queryRest to avoid confusion.
- value = useMemo(() => data, [data]) doesn’t add value here; passing data directly is equivalent.
- function WrappingProvider({ children }: PropsWithChildren) { - const { data, isPending, error, ...rest } = useQuery(); - const enabled = 'enabled' in rest && !required ? rest.enabled : true; - const value = useMemo(() => data, [data]); + function WrappingProvider({ children }: PropsWithChildren) { + const { data, isPending, error, ...queryRest } = useQuery(); + const enabled = 'enabled' in queryRest && !required ? queryRest.enabled : true; @@ - return <Provider value={enabled ? (value ?? defaultValue) : defaultValue}>{children}</Provider>; + return <Provider value={enabled ? (data ?? defaultValue) : defaultValue}>{children}</Provider>;src/features/form/layoutSets/LayoutSetsProvider.tsx (1)
17-29: Avoid random ids in queryFn to prevent remounts on refetch; prefer deterministic ids (preserve-existing or index-based).Generating new UUIDs on every fetch will change React keys for taskNavigation items and can cause unnecessary unmount/mount cycles and lost local state, undermining the performance goal of this PR. Use stable, deterministic ids instead and only synthesize an id when one is missing.
Apply this diff to make ids deterministic and to preserve any existing id:
- queryFn: async () => { - const layoutSets = await fetchLayoutSets(); - if (layoutSets?.uiSettings?.taskNavigation) { - return { - ...layoutSets, - uiSettings: { - ...layoutSets.uiSettings, - taskNavigation: layoutSets.uiSettings.taskNavigation.map((g) => ({ ...g, id: uuidv4() })), - }, - }; - } - return layoutSets; - }, + queryFn: async () => { + const layoutSets = await fetchLayoutSets(); + if (layoutSets?.uiSettings?.taskNavigation) { + return { + ...layoutSets, + uiSettings: { + ...layoutSets.uiSettings, + taskNavigation: layoutSets.uiSettings.taskNavigation.map((g, i) => + // Preserve any existing id; otherwise generate a stable one + (('id' in g && g.id) ? g : { ...g, id: String(i) }) + ), + }, + }; + } + return layoutSets; + },Additionally remove the now-unused uuid import:
-import { v4 as uuidv4 } from 'uuid';Optional follow-up: the same id-injection pattern appears in LayoutSettingsContext.tsx; consider centralizing it in a small helper to avoid drift.
src/features/language/textResources/TextResourcesProvider.tsx (1)
41-46: Optional: consider centralizing common TanStack Query options.If you’re adopting a shared queryOptions helper elsewhere (per repo guidelines), you could wrap the key/fn here too for consistency. No functional change required.
src/layout/RepeatingGroup/useTableComponentIds.ts (2)
15-26: Defensive optional chaining to avoid potential crashes if children is missing.If component.children were ever undefined (misconfig, partial load), calling .map would throw. Optional chain safely falls back to emptyArray (already in place).
Apply this diff:
- component.children.map((id) => { + component.children?.map((id) => { if (multiPage) { const [, childId] = id.split(':', 2); return layoutLookups.getComponent(childId); } return layoutLookups.getComponent(id); - }) ?? emptyArray, + }) ?? emptyArray,
37-44: Micro-optimization: avoid O(n^2) sort by precomputing header indexes.When tableHeaders is present, using indexOf in the comparator repeatedly is O(n^2). Precompute a map once to keep sort O(n log n).
Apply this diff:
- if (tableHeaders) { - ids.sort((a, b) => { - const aIndex = tableHeaders.indexOf(a); - const bIndex = tableHeaders.indexOf(b); - return aIndex - bIndex; - }); - } + if (tableHeaders) { + const indexMap = new Map(tableHeaders.map((h, i) => [h, i] as const)); + ids.sort((a, b) => (indexMap.get(a)! - indexMap.get(b)!)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/core/contexts/queryContext.tsx(4 hunks)src/features/datamodel/DataModelsProvider.tsx(2 hunks)src/features/form/layout/LayoutsContext.tsx(2 hunks)src/features/form/layoutSets/LayoutSetsProvider.tsx(1 hunks)src/features/form/layoutSettings/LayoutSettingsContext.tsx(2 hunks)src/features/language/textResources/TextResourcesProvider.tsx(2 hunks)src/layout/RepeatingGroup/Providers/RepeatingGroupContext.tsx(3 hunks)src/layout/RepeatingGroup/Table/RepeatingGroupTable.tsx(2 hunks)src/layout/RepeatingGroup/useTableComponentIds.ts(1 hunks)src/layout/RepeatingGroup/utils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/RepeatingGroup/utils.tssrc/layout/RepeatingGroup/useTableComponentIds.tssrc/features/form/layout/LayoutsContext.tsxsrc/features/form/layoutSets/LayoutSetsProvider.tsxsrc/features/language/textResources/TextResourcesProvider.tsxsrc/layout/RepeatingGroup/Providers/RepeatingGroupContext.tsxsrc/features/datamodel/DataModelsProvider.tsxsrc/layout/RepeatingGroup/Table/RepeatingGroupTable.tsxsrc/core/contexts/queryContext.tsxsrc/features/form/layoutSettings/LayoutSettingsContext.tsx
🧬 Code graph analysis (10)
src/layout/RepeatingGroup/utils.ts (1)
src/hooks/useStateDeepEqual.ts (1)
useMemoDeepEqual(27-41)
src/layout/RepeatingGroup/useTableComponentIds.ts (1)
src/features/expressions/expression-functions.ts (1)
component(426-462)
src/features/form/layout/LayoutsContext.tsx (1)
src/features/form/layout/makeLayoutLookups.ts (1)
makeLayoutLookups(115-167)
src/features/form/layoutSets/LayoutSetsProvider.tsx (1)
src/queries/queries.ts (1)
fetchLayoutSets(240-240)
src/features/language/textResources/TextResourcesProvider.tsx (4)
src/core/queries/useQueryWithStaleData.ts (1)
useQueryWithStaleData(13-39)src/utils/network/sharedNetworking.ts (1)
HttpClientError(5-5)src/queries/queries.ts (1)
fetchTextResources(285-286)src/core/contexts/queryContext.tsx (1)
createQueryContext(33-61)
src/layout/RepeatingGroup/Providers/RepeatingGroupContext.tsx (4)
src/utils/layout/hooks.ts (1)
useExternalItem(16-22)src/features/validation/callbacks/onGroupCloseValidation.ts (1)
useOnGroupCloseValidation(15-57)src/layout/RepeatingGroup/utils.ts (1)
RepGroupHooks(63-231)src/utils/layout/types.ts (1)
BaseRow(12-15)
src/features/datamodel/DataModelsProvider.tsx (1)
src/core/loading/Loader.tsx (1)
Loader(15-38)
src/layout/RepeatingGroup/Table/RepeatingGroupTable.tsx (2)
src/layout/layout.ts (1)
IDataModelBindings(61-64)src/utils/layout/types.ts (1)
BaseRow(12-15)
src/core/contexts/queryContext.tsx (1)
src/core/contexts/context.tsx (1)
createContext(45-89)
src/features/form/layoutSettings/LayoutSettingsContext.tsx (5)
src/core/queries/usePrefetchQuery.ts (1)
QueryDefinition(4-4)src/core/contexts/AppQueriesProvider.tsx (1)
useAppQueries(58-58)src/queries/queries.ts (1)
fetchLayoutSettings(244-245)src/core/contexts/delayedContext.tsx (1)
delayedContext(11-38)src/core/contexts/queryContext.tsx (1)
createQueryContext(33-61)
🔇 Additional comments (8)
src/layout/RepeatingGroup/utils.ts (1)
7-7: Good call switching to deep-equality memoization.Importing useMemoDeepEqual is appropriate here given the heavy, mostly-stable object you return per row.
src/layout/RepeatingGroup/Providers/RepeatingGroupContext.tsx (2)
360-375: Replacing delayed selectors with store.getState() at call time fixes stale reads.Grabbing editingAll/editingId/editingNone and computing index from fresh rows inside the returned function ensures validation runs against the latest state.
412-427: Toggling edit state now reads the live editingId — good.Using store.getState() avoids capturing a stale editingId in the handler closure.
src/core/contexts/queryContext.tsx (2)
21-24: Type surface is cleaner; default shouldDisplayError hook is a nice touch.No issues here.
33-38: API simplification confirmed – no call sites pass removed props
Ran the provided ripgrep checks across the repository and found zero occurrences ofprocess,DisplayError,LoaderincreateQueryContextcalls, nor any old three-generic-parameter usages. The API change is safe to approve.src/features/language/textResources/TextResourcesProvider.tsx (1)
25-29: LGTM: moving transformation into queryFn aligns with the new QueryContext API and keeps types precise.The queryKey includes the selectedLanguage, the conversion is tight, and useQueryWithStaleData preserves UX during transitions. No issues spotted.
src/features/form/layoutSettings/LayoutSettingsContext.tsx (1)
21-22: LGTM: processing moved to queryFn with a typed ProcessedLayoutSettings result.The queryKey scopes to layoutSetId and the processing path is single-sourced via processData.
src/layout/RepeatingGroup/Table/RepeatingGroupTable.tsx (1)
150-152: LGTM: passing only index/uuid reduces prop churn and child re-renders.This narrows RowToDisplay’s props to the minimal stable surface, which should cut unnecessary renders and aligns with the PR goal.
Also applies to: 174-176
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/features/form/layoutSettings/LayoutSettingsContext.tsx (2)
46-55: Throwing Error objects: LGTM, resolves earlier concern.Switching from string throws to Error preserves stack traces and matches createQueryContext’s error handling.
62-75: Stabilize generated ids to prevent remounts on refetch (perf).Recomputing uuidv4() on each refetch assigns new keys to groups/taskNavigation, causing remounts and extra work downstream. Preserving an existing id and synthesizing a deterministic fallback (e.g., index) keeps keys stable across refetches and aligns with the PR’s performance objectives.
Apply this diff:
return { order, - groups: 'groups' in settings.pages ? settings.pages.groups.map((g) => ({ ...g, id: uuidv4() })) : undefined, + groups: + 'groups' in settings.pages + ? settings.pages.groups.map((g, i) => + (('id' in g && g.id) ? g : { ...g, id: String(i) }) + ) + : undefined, pageSettings: omitUndefined({ autoSaveBehavior: settings.pages.autoSaveBehavior, expandedWidth: settings.pages.expandedWidth, hideCloseButton: settings.pages.hideCloseButton, showExpandWidthButton: settings.pages.showExpandWidthButton, showLanguageSelector: settings.pages.showLanguageSelector, showProgress: settings.pages.showProgress, - taskNavigation: settings.pages.taskNavigation?.map((g) => ({ ...g, id: uuidv4() })), + taskNavigation: settings.pages.taskNavigation?.map((g, i) => + (('id' in g && g.id) ? g : { ...g, id: String(i) }) + ), }), pdfLayoutName: settings.pages.pdfLayoutName, };If you adopt this, you can remove the uuid import at the top of the file as a follow-up.
🧹 Nitpick comments (2)
src/features/form/layoutSettings/LayoutSettingsContext.tsx (2)
17-23: Good shift: processing moved into queryFn; consider queryOptions for consistency.This aligns with the PR’s goal (do work once in the query pipeline). If your codebase uses TanStack’s queryOptions to centralize defaults (staleTime/gcTime/select/meta), consider wrapping these options to keep behavior consistent with other queries.
Apply this diff if you want to standardize on queryOptions:
-import { useQuery } from '@tanstack/react-query'; +import { useQuery, queryOptions } from '@tanstack/react-query'; @@ -export function useLayoutSettingsQueryDef(layoutSetId?: string): QueryDefinition<ProcessedLayoutSettings> { +export function useLayoutSettingsQueryDef(layoutSetId?: string): QueryDefinition<ProcessedLayoutSettings> { const { fetchLayoutSettings } = useAppQueries(); - return { - queryKey: ['layoutSettings', layoutSetId], - queryFn: async () => processData(layoutSetId ? await fetchLayoutSettings(layoutSetId) : null), - }; + return queryOptions({ + queryKey: ['layoutSettings', layoutSetId] as const, + queryFn: async () => processData(layoutSetId ? await fetchLayoutSettings(layoutSetId) : null), + }); }
36-44: Prefer returning groups: undefined instead of [] when settings are missing.ProcessedLayoutSettings declares groups as optional. Returning [] here but undefined elsewhere makes consumers handle two shapes for “no groups”. Using undefined consistently typically avoids unnecessary renders and guards like arr.length === 0.
Proposed change:
if (!settings) { return { order: [], - groups: [], + groups: undefined, pageSettings: {}, pdfLayoutName: undefined, }; }Please verify no downstream code relies on [] specifically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/features/form/layoutSettings/LayoutSettingsContext.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/features/form/layoutSettings/LayoutSettingsContext.tsx
🧬 Code graph analysis (1)
src/features/form/layoutSettings/LayoutSettingsContext.tsx (5)
src/core/queries/usePrefetchQuery.ts (1)
QueryDefinition(4-4)src/core/contexts/AppQueriesProvider.tsx (1)
useAppQueries(58-58)src/queries/queries.ts (1)
fetchLayoutSettings(244-245)src/core/contexts/delayedContext.tsx (1)
delayedContext(11-38)src/core/contexts/queryContext.tsx (1)
createQueryContext(33-61)
🔇 Additional comments (2)
src/features/form/layoutSettings/LayoutSettingsContext.tsx (2)
57-61: Confirm behavior: groups without “order” are silently ignored when deriving page order.The flatten will drop any group lacking an order array. If that’s intended, all good. If not, consider logging a warning (or appending those pages in declared order).
79-84: Context wiring: LGTM.Typed createQueryContext<ProcessedLayoutSettings, true> and deferring transformation to the query layer is consistent with the new pattern and reduces Provider churn.
|
Description
process()fromcreateQueryContext(), as this can be covered by functionality already in tanstack query, or the code could simply be added to the query function instead. This caused spooky stuff to happen, as I initially removed theuseMemo()from the wrapper provider insidecreateQueryContext(), and I think that caused react compiler to optimize a bit too much. With auseMemo()it works fine, but without it the app would never load.delayedSelectorin some utility functions forRepeatingGroup. These functions does not really need to be re-created every time some state changes - they can just get the fresh state every call. There's still an issue inRepGroupHooks.useGetFreshRowsWithButtons()which also could just get fresh state instead of re-rendering after it has been called, but that would mean I'd have to create a non-reactive variant of expression data sources - which will be much easier after we move to one big store.RepeatingGroupTable.tsxbecause an object was passed instead of just theindexanduuidproperties that rarely changes.useTableComponentIds(), as a dependency touseMemo()was not itself memoized.useRowWithExpressions(). The result rarely changes, but since data sources may force a re-render when a new row is added, this memo would be re-generated a bit often, and would cause more work to be done downstream.These are all minor stuff, but in sum they further boost the performance when testing the
ssb/ra0485-01app. Along with pagination in that repeating group, I can just about keep working with ~370 rows.Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores