Skip to content

Commit ff46fdf

Browse files
olemartinorgOle Martin Handeland
andauthored
Minor performance optimizations for large repeating groups (#3606)
* Removing process() from query providers, it can already be covered by either select() in tanstack query or by wrapping the query function. * React compiler doesn't seem to memoize this data, so it needs manual 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. * There shouldn't really be a need for delayedSelector here, as we don't have to re-render these tools for them them to work correctly next time they're called. * Preventing useless re-renders by just passing the props instead of passing a new object * The other memo relies on this being memoized * No need to really return a new object if the values never changed * Processing this in the query function instead of selecting * Very strange, but createQueryContext() failed completely without the 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. * Found a case where a language change seems to unmount FormDataWrite entirely 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. * This mutation check was not really reactive, so it could end up locking 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). * Applying suggestion from coderabbit --------- Co-authored-by: Ole Martin Handeland <[email protected]>
1 parent 42b46cc commit ff46fdf

File tree

10 files changed

+124
-134
lines changed

10 files changed

+124
-134
lines changed

src/core/contexts/queryContext.tsx

Lines changed: 11 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@ import type { UseQueryResult } from '@tanstack/react-query';
55
import type { AxiosError } from 'axios';
66

77
import { createContext } from 'src/core/contexts/context';
8-
import { DisplayError as DefaultDisplayError } from 'src/core/errorHandling/DisplayError';
9-
import { Loader as DefaultLoader } from 'src/core/loading/Loader';
8+
import { DisplayError } from 'src/core/errorHandling/DisplayError';
9+
import { Loader } from 'src/core/loading/Loader';
1010
import type { LaxContextProps, StrictContextProps } from 'src/core/contexts/context';
1111

1212
type Err = Error | AxiosError;
@@ -18,17 +18,9 @@ type Query<Req extends boolean, QueryData> = () => Req extends true
1818

1919
type ContextProps<Ctx, Req extends boolean> = Req extends true ? StrictContextProps : LaxContextProps<Ctx>;
2020

21-
export type QueryContextProps<QueryData, Req extends boolean, ContextData = QueryData> = ContextProps<
22-
ContextData,
23-
Req
24-
> & {
21+
export type QueryContextProps<QueryData, Req extends boolean> = ContextProps<QueryData, Req> & {
2522
query: Query<Req, QueryData>;
26-
27-
process?: (data: QueryData) => ContextData;
2823
shouldDisplayError?: (error: Err) => boolean;
29-
30-
DisplayError?: React.ComponentType<{ error: Err }>;
31-
Loader?: React.ComponentType<{ reason: string }>;
3224
};
3325

3426
/**
@@ -38,26 +30,16 @@ export type QueryContextProps<QueryData, Req extends boolean, ContextData = Quer
3830
* Remember to call this through a delayedContext() call to prevent problems with cyclic imports.
3931
* @see delayedContext
4032
*/
41-
export function createQueryContext<QD, Req extends boolean, CD = QD>(props: QueryContextProps<QD, Req, CD>) {
42-
const {
43-
name,
44-
required,
45-
query,
46-
process = (i: QD) => i as unknown as CD,
47-
shouldDisplayError = () => true,
48-
DisplayError = DefaultDisplayError,
49-
Loader = DefaultLoader,
50-
...rest
51-
} = props;
33+
export function createQueryContext<QD, Req extends boolean>(props: QueryContextProps<QD, Req>) {
34+
const { name, required, query: useQuery, shouldDisplayError = () => true, ...rest } = props;
5235
// eslint-disable-next-line @typescript-eslint/no-explicit-any
53-
const { Provider, useCtx, useLaxCtx, useHasProvider } = createContext<CD>({ name, required, ...(rest as any) });
54-
const defaultValue = ('default' in rest ? rest.default : undefined) as CD;
36+
const { Provider, useCtx, useLaxCtx, useHasProvider } = createContext<QD>({ name, required, ...(rest as any) });
37+
const defaultValue = ('default' in rest ? rest.default : undefined) as QD;
5538

56-
const WrappingProvider = ({ children }: PropsWithChildren) => {
57-
const { data, isPending, error, ...rest } = query();
39+
function WrappingProvider({ children }: PropsWithChildren) {
40+
const { data, isPending, error, ...rest } = useQuery();
5841
const enabled = 'enabled' in rest && !required ? rest.enabled : true;
59-
60-
const value = useMemo(() => (typeof data !== 'undefined' ? process(data) : undefined), [data]);
42+
const value = useMemo(() => data, [data]);
6143

6244
if (enabled && isPending) {
6345
return <Loader reason={`query-${name}`} />;
@@ -68,7 +50,7 @@ export function createQueryContext<QD, Req extends boolean, CD = QD>(props: Quer
6850
}
6951

7052
return <Provider value={enabled ? (value ?? defaultValue) : defaultValue}>{children}</Provider>;
71-
};
53+
}
7254

7355
return {
7456
Provider: WrappingProvider,

src/features/datamodel/DataModelsProvider.tsx

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
import React, { useCallback, useEffect, useMemo } from 'react';
1+
import React, { useCallback, useEffect, useMemo, useRef } from 'react';
22
import type { PropsWithChildren } from 'react';
33

4-
import { useQueryClient } from '@tanstack/react-query';
4+
import { useMutationState, useQueryClient } from '@tanstack/react-query';
55
import deepEqual from 'fast-deep-equal';
66
import { createStore } from 'zustand';
77
import type { JSONSchema7 } from 'json-schema';
@@ -257,6 +257,18 @@ function BlockUntilLoaded({ children }: PropsWithChildren) {
257257
const actualCurrentTask = useCurrentLayoutSetId();
258258
const isPDF = useIsPdf();
259259

260+
const currentMutations = useMutationState({ filters: { status: 'pending', mutationKey: ['saveFormData'] } });
261+
const hasPassedMutationCheck = useRef(false);
262+
if (currentMutations.length > 0 && !hasPassedMutationCheck.current) {
263+
// FormDataWrite automatically saves unsaved changes on unmount. If something happens above us in the render tree
264+
// that causes FormDataWrite to be unmounted (forcing it to save) and re-mounts everything (including us), we
265+
// should wait for that previously started save to complete. Otherwise, we'd end up saving outdated initial data
266+
// and cause a 409 when patching later.
267+
return <Loader reason='save-form-data' />;
268+
}
269+
270+
hasPassedMutationCheck.current = true;
271+
260272
if (error) {
261273
// Error trying to fetch data, if missing rights we display relevant page
262274
if (isAxiosError(error) && error.response?.status === HttpStatusCodes.Forbidden) {

src/features/form/layout/LayoutsContext.tsx

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useEffect } from 'react';
1+
import { useEffect, useMemo } from 'react';
22

33
import { skipToken, useQuery } from '@tanstack/react-query';
44

@@ -57,15 +57,18 @@ function useLayoutQuery() {
5757
utils.error && window.logError('Fetching form layout failed:\n', utils.error);
5858
}, [utils.error]);
5959

60-
return utils.data
61-
? {
62-
...utils,
63-
data: {
64-
...utils.data,
65-
lookups: makeLayoutLookups(utils.data.layouts),
66-
},
67-
}
68-
: utils;
60+
const data = useMemo(() => {
61+
if (utils.data) {
62+
return {
63+
...utils.data,
64+
lookups: makeLayoutLookups(utils.data.layouts),
65+
};
66+
}
67+
68+
return utils.data;
69+
}, [utils.data]);
70+
71+
return { ...utils, data };
6972
}
7073
const { Provider, useCtx, useLaxCtx } = delayedContext(() =>
7174
createQueryContext({

src/features/form/layoutSets/LayoutSetsProvider.tsx

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,19 @@ export function useLayoutSetsQueryDef() {
1414
const { fetchLayoutSets } = useAppQueries();
1515
return {
1616
queryKey: ['fetchLayoutSets'],
17-
queryFn: fetchLayoutSets,
17+
queryFn: async () => {
18+
const layoutSets = await fetchLayoutSets();
19+
if (layoutSets?.uiSettings?.taskNavigation) {
20+
return {
21+
...layoutSets,
22+
uiSettings: {
23+
...layoutSets.uiSettings,
24+
taskNavigation: layoutSets.uiSettings.taskNavigation.map((g) => ({ ...g, id: uuidv4() })),
25+
},
26+
};
27+
}
28+
return layoutSets;
29+
},
1830
};
1931
}
2032

@@ -33,18 +45,6 @@ const { Provider, useCtx, useLaxCtx } = delayedContext(() =>
3345
name: 'LayoutSets',
3446
required: true,
3547
query: useLayoutSetsQuery,
36-
process: (layoutSets) => {
37-
if (layoutSets?.uiSettings?.taskNavigation) {
38-
return {
39-
...layoutSets,
40-
uiSettings: {
41-
...layoutSets.uiSettings,
42-
taskNavigation: layoutSets.uiSettings.taskNavigation.map((g) => ({ ...g, id: uuidv4() })),
43-
},
44-
};
45-
}
46-
return layoutSets;
47-
},
4848
}),
4949
);
5050

src/features/form/layoutSettings/LayoutSettingsContext.tsx

Lines changed: 45 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,11 @@ import type { QueryDefinition } from 'src/core/queries/usePrefetchQuery';
1414
import type { GlobalPageSettings, ILayoutSettings, NavigationPageGroup } from 'src/layout/common.generated';
1515

1616
// Also used for prefetching @see formPrefetcher.ts
17-
export function useLayoutSettingsQueryDef(layoutSetId?: string): QueryDefinition<ILayoutSettings | null> {
17+
export function useLayoutSettingsQueryDef(layoutSetId?: string): QueryDefinition<ProcessedLayoutSettings> {
1818
const { fetchLayoutSettings } = useAppQueries();
1919
return {
2020
queryKey: ['layoutSettings', layoutSetId],
21-
queryFn: () => (layoutSetId ? fetchLayoutSettings(layoutSetId) : null),
21+
queryFn: async () => processData(layoutSetId ? await fetchLayoutSettings(layoutSetId) : null),
2222
};
2323
}
2424

@@ -33,50 +33,53 @@ function useLayoutSettingsQuery() {
3333
return query;
3434
}
3535

36+
function processData(settings: ILayoutSettings | null): ProcessedLayoutSettings {
37+
if (!settings) {
38+
return {
39+
order: [],
40+
groups: [],
41+
pageSettings: {},
42+
pdfLayoutName: undefined,
43+
};
44+
}
45+
46+
if (!('order' in settings.pages) && !('groups' in settings.pages)) {
47+
const msg = 'Missing page order, specify one of `pages.order` or `pages.groups` in Settings.json';
48+
window.logError(msg);
49+
throw new Error(msg);
50+
}
51+
if ('order' in settings.pages && 'groups' in settings.pages) {
52+
const msg = 'Specify one of `pages.order` or `pages.groups` in Settings.json';
53+
window.logError(msg);
54+
throw new Error(msg);
55+
}
56+
57+
const order: string[] =
58+
'order' in settings.pages
59+
? settings.pages.order
60+
: settings.pages.groups.filter((group) => 'order' in group).flatMap((group) => group.order);
61+
62+
return {
63+
order,
64+
groups: 'groups' in settings.pages ? settings.pages.groups.map((g) => ({ ...g, id: uuidv4() })) : undefined,
65+
pageSettings: omitUndefined({
66+
autoSaveBehavior: settings.pages.autoSaveBehavior,
67+
expandedWidth: settings.pages.expandedWidth,
68+
hideCloseButton: settings.pages.hideCloseButton,
69+
showExpandWidthButton: settings.pages.showExpandWidthButton,
70+
showLanguageSelector: settings.pages.showLanguageSelector,
71+
showProgress: settings.pages.showProgress,
72+
taskNavigation: settings.pages.taskNavigation?.map((g) => ({ ...g, id: uuidv4() })),
73+
}),
74+
pdfLayoutName: settings.pages.pdfLayoutName,
75+
};
76+
}
77+
3678
const { Provider, useCtx, useLaxCtx } = delayedContext(() =>
37-
createQueryContext<ILayoutSettings | null, true, ProcessedLayoutSettings>({
79+
createQueryContext<ProcessedLayoutSettings, true>({
3880
name: 'LayoutSettings',
3981
required: true,
4082
query: useLayoutSettingsQuery,
41-
process: (settings) => {
42-
if (!settings) {
43-
return {
44-
order: [],
45-
groups: [],
46-
pageSettings: {},
47-
pdfLayoutName: undefined,
48-
};
49-
}
50-
51-
if (!('order' in settings.pages) && !('groups' in settings.pages)) {
52-
window.logError('Missing page order, specify one of `pages.order` or `pages.groups` in Settings.json');
53-
throw 'Missing page order, specify one of `pages.order` or `pages.groups` in Settings.json';
54-
}
55-
if ('order' in settings.pages && 'groups' in settings.pages) {
56-
window.logError('Both `pages.order` and `pages.groups` was set in Settings.json');
57-
throw 'Both `pages.order` and `pages.groups` was set in Settings.json';
58-
}
59-
60-
const order: string[] =
61-
'order' in settings.pages
62-
? settings.pages.order
63-
: settings.pages.groups.filter((group) => 'order' in group).flatMap((group) => group.order);
64-
65-
return {
66-
order,
67-
groups: 'groups' in settings.pages ? settings.pages.groups.map((g) => ({ ...g, id: uuidv4() })) : undefined,
68-
pageSettings: omitUndefined({
69-
autoSaveBehavior: settings.pages.autoSaveBehavior,
70-
expandedWidth: settings.pages.expandedWidth,
71-
hideCloseButton: settings.pages.hideCloseButton,
72-
showExpandWidthButton: settings.pages.showExpandWidthButton,
73-
showLanguageSelector: settings.pages.showLanguageSelector,
74-
showProgress: settings.pages.showProgress,
75-
taskNavigation: settings.pages.taskNavigation?.map((g) => ({ ...g, id: uuidv4() })),
76-
}),
77-
pdfLayoutName: settings.pages.pdfLayoutName,
78-
};
79-
},
8083
}),
8184
);
8285

src/features/language/textResources/TextResourcesProvider.tsx

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,10 @@ const useTextResourcesQuery = () => {
2222
const enabled = useIsCurrentLanguageResolved();
2323

2424
const utils = {
25-
...useQueryWithStaleData<ITextResourceResult, HttpClientError>({
25+
...useQueryWithStaleData<TextResourceMap, HttpClientError>({
2626
enabled,
2727
queryKey: ['fetchTextResources', selectedLanguage],
28-
queryFn: () => fetchTextResources(selectedLanguage),
28+
queryFn: async () => convertResult(await fetchTextResources(selectedLanguage)),
2929
}),
3030
enabled,
3131
};
@@ -38,12 +38,11 @@ const useTextResourcesQuery = () => {
3838
};
3939

4040
const { Provider, useCtx, useHasProvider } = delayedContext(() =>
41-
createQueryContext<ITextResourceResult, false, TextResourceMap>({
41+
createQueryContext<TextResourceMap, false>({
4242
name: 'TextResources',
4343
required: false,
4444
default: {},
4545
query: useTextResourcesQuery,
46-
process: convertResult,
4746
}),
4847
);
4948

src/layout/RepeatingGroup/Providers/RepeatingGroupContext.tsx

Lines changed: 7 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -357,26 +357,16 @@ export function RepeatingGroupProvider({ baseComponentId, children }: PropsWithC
357357

358358
export const useRepeatingGroupComponentId = () => ZStore.useSelector((state) => state.baseComponentId);
359359

360-
function useDelayedSelector() {
361-
return ZStore.useDelayedSelector({
362-
mode: 'innerSelector',
363-
makeArgs: (state) => [state],
364-
});
365-
}
366-
367360
function useMaybeValidateRow() {
361+
const store = ZStore.useStore();
368362
const baseComponentId = useRepeatingGroupComponentId();
369-
const delayedSelector = useDelayedSelector();
370363
const { validateOnSaveRow } = useExternalItem(baseComponentId, 'RepeatingGroup');
371364
const onGroupCloseValidation = useOnGroupCloseValidation();
372365
const getRows = RepGroupHooks.useGetFreshRowsWithButtons(baseComponentId);
373-
const getState = () => produceStateFromRows(getRows() ?? []);
374366

375367
return () => {
376-
const editingAll = delayedSelector((s) => s.editingAll, []);
377-
const editingId = delayedSelector((s) => s.editingId, []);
378-
const editingNone = delayedSelector((s) => s.editingNone, []);
379-
const index = getState().editableRows.find((row) => row.uuid === editingId)?.index;
368+
const { editingAll, editingId, editingNone } = store.getState();
369+
const index = produceStateFromRows(getRows() ?? []).editableRows.find((row) => row.uuid === editingId)?.index;
380370
if (!validateOnSaveRow || editingAll || editingNone || editingId === undefined || index === undefined) {
381371
return Promise.resolve(false);
382372
}
@@ -419,16 +409,16 @@ export const RepGroupContext = {
419409
return ZStore.useSelector((state) => (uuid ? state.deletingIds.includes(uuid) : false));
420410
},
421411
useToggleEditing() {
412+
const store = ZStore.useStore();
422413
const rawOpenForEditing = ZStore.useStaticSelector((state) => state.openForEditing);
423414
const rawCloseForEditing = ZStore.useStaticSelector((state) => state.closeForEditing);
424-
const delayedSelector = useDelayedSelector();
425415
const maybeValidateRow = useMaybeValidateRow();
426416

427417
return async (row: BaseRow) => {
428418
if (await maybeValidateRow()) {
429419
return;
430420
}
431-
const editingId = delayedSelector((s) => s.editingId, []);
421+
const editingId = store.getState().editingId;
432422
if (editingId === row.uuid) {
433423
rawCloseForEditing(row);
434424
} else {
@@ -481,20 +471,16 @@ export const RepGroupContext = {
481471
};
482472
},
483473
useChangePageToRow() {
474+
const store = ZStore.useStore();
484475
const baseComponentId = useRepeatingGroupComponentId();
485476
const rawChangePage = ZStore.useStaticSelector((state) => state.changePage);
486-
const delayedSelector = useDelayedSelector();
487477
const maybeValidateRow = useMaybeValidateRow();
488478

489479
const { pagination } = useExternalItem(baseComponentId, 'RepeatingGroup');
490480
const getRows = RepGroupHooks.useGetFreshRowsWithButtons(baseComponentId);
491481
const getState = () => produceStateFromRows(getRows() ?? []);
492482
const getPaginationState = () =>
493-
producePaginationState(
494-
delayedSelector((s) => s.currentPage, []),
495-
pagination,
496-
getState().visibleRows,
497-
);
483+
producePaginationState(store.getState().currentPage, pagination, getState().visibleRows);
498484

499485
return async (row: BaseRow) => {
500486
if (await maybeValidateRow()) {

0 commit comments

Comments
 (0)