Bugfix: Feedback step should be able to navigate to a data task #3645
Conversation
olemartinorg
added
kind/bug
📝 WalkthroughWalkthroughRemoves the type-based PresentationComponent API, centralizes receipt detection via useIsReceiptPage and FixWrongReceiptType, moves SearchParams into core/routing, adds navigation/query helpers (useIsNavigating, useWaitForQueries), refactors process/feedback and data-model handling to tuple form, and updates related imports/tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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. 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/features/propagateTraceWhenPdf/index.ts (2)
6-13: Fix cookie parsing typing and edge-cases.
Current code creates a{}and assigns dynamic keys (unsafe) and may create empty keys.-function getCookies(): { [key: string]: string } { - const cookie = {}; - document.cookie.split(';').forEach((el) => { - const split = el.split('='); - cookie[split[0].trim()] = split.slice(1).join('='); - }); - return cookie; -} +function getCookies(): Record<string, string> { + const cookies: Record<string, string> = {}; + if (!document.cookie) return cookies; + document.cookie.split(';').forEach((raw) => { + const el = raw.trim(); + if (!el) return; + const idx = el.indexOf('='); + if (idx === -1) return; + const key = el.slice(0, idx).trim(); + const value = el.slice(idx + 1); + if (!key) return; + cookies[key] = decodeURIComponent(value); + }); + return cookies; +}
21-46: Avoid multiple interceptor registrations.
IfpropagateTraceWhenPdf()runs more than once, interceptors stack up.- if (isPdf) { - const cookies = getCookies(); - axios.interceptors.request.use((config) => { + if (isPdf) { + const cookies = getCookies(); + if (pdfInterceptorId != null) return; + pdfInterceptorId = axios.interceptors.request.use((config) => { try { if (config.url?.startsWith(appPath) !== true) { return config; } // This header is caught in app-lib/backend and used to allow injection of traceparent/context - config.headers['X-Altinn-IsPdf'] = 'true'; + (config.headers ??= {})['X-Altinn-IsPdf'] = 'true'; const traceparent = cookies['altinn-telemetry-traceparent']; const tracestate = cookies['altinn-telemetry-tracestate']; if (traceparent) { - config.headers['traceparent'] = traceparent; + (config.headers ??= {})['traceparent'] = traceparent; } if (tracestate) { - config.headers['tracestate'] = tracestate; + (config.headers ??= {})['tracestate'] = tracestate; } return config; } catch (err) { console.error('Error configuring propagation of W3C trace for request', err); return config; } }); }Additional top-level addition (outside the selected range):
// At module scope let pdfInterceptorId: number | null = null;src/features/instance/useProcessQuery.ts (1)
66-71: Bug: wrong task lookup uses currentTask regardless of match.
You ignore the found task inprocessTasksand always returncurrentTask, which misclassifies other tasks.- return (taskId: string | undefined) => { - const task = - (processData?.processTasks?.find((t) => t.elementId === taskId) ?? processData?.currentTask?.elementId === taskId) - ? processData?.currentTask - : undefined; + return (taskId: string | undefined) => { + const task = + processData?.processTasks?.find((t) => t.elementId === taskId) ?? + (processData?.currentTask?.elementId === taskId ? processData?.currentTask : undefined);src/components/form/Form.tsx (1)
53-56: Bug: search param not removed due to returning stale objectYou mutate params but return the outer searchParams, so ?validate persists and can cause repeated validation/replace loops. Return a new instance (and prefer replace).
Apply:
- setSearchParams((params) => { - params.delete(SearchParams.Validate); - return searchParams; - }); + setSearchParams((prev) => { + const next = new URLSearchParams(prev); + next.delete(SearchParams.Validate); + return next; + }, { replace: true });src/hooks/useNavigatePage.ts (1)
248-252: Bug: URLSearchParams.has does not accept a value argument
has(name, 'true')is invalid and always checks only presence. You likely want to check for the value being 'true'.Apply this diff:
- const shouldExitSubform = options?.searchParams?.has(SearchParams.ExitSubform, 'true') ?? false; + const shouldExitSubform = options?.searchParams?.get(SearchParams.ExitSubform) === 'true';src/features/processEnd/feedback/Feedback.tsx (1)
60-71: Prevent overlapping refetches and handle errors in backoff loopCurrently, the next tick is scheduled immediately after triggering
callback(), which can cause overlapping requests if a fetch takes longer than the backoff delay. Also, unhandled promise rejections can leak to the console. Await the callback (with catch) before scheduling the next attempt.- setTimeout(() => { - if (!shouldContinue) { - return; - } - callback().then(); - attempts.current++; - shouldContinue && continueCalling(); - }, backoff); + setTimeout(() => { + if (!shouldContinue) { + return; + } + Promise.resolve() + .then(() => callback()) + .catch((err) => { + // Log once per failure; consider using a dedicated logger if available + console.error('Feedback polling failed', err); + }) + .finally(() => { + attempts.current++; + if (shouldContinue) { + continueCalling(); + } + }); + }, backoff);
🧹 Nitpick comments (18)
src/core/routing/types.ts (1)
1-7: Optional: use a const object + union type instead of a TS enumThis avoids enum runtime artifacts, plays nicer with ESM tree-shaking, and retains the same call-site API.
-export enum SearchParams { - FocusComponentId = 'focusComponentId', - FocusErrorBinding = 'focusErrorBinding', - ExitSubform = 'exitSubform', - Validate = 'validate', - Pdf = 'pdf', -} +export const SearchParams = { + FocusComponentId: 'focusComponentId', + FocusErrorBinding: 'focusErrorBinding', + ExitSubform: 'exitSubform', + Validate: 'validate', + Pdf: 'pdf', +} as const; +export type SearchParamKey = keyof typeof SearchParams;src/components/form/LinkToPotentialNode.tsx (1)
21-23: Tighten search extraction for robustness.
Safer handling when no query string and clearer intent.- const searchParams = typeof to === 'string' ? to.split('?').at(1) : to.search; + const searchParams = + typeof to === 'string' + ? (to.includes('?') ? to.slice(to.indexOf('?') + 1) : '') + : (to.search ?? '');test/e2e/integration/stateless-app/receipt.ts (2)
19-23: Make the manual back-navigation URL replace resilient.
Preserve query/hash and avoid brittle end-of-string assumptions.- cy.url().then((url) => cy.visit(url.replace(/\/Task_2\/1$/, '/Task_1'))); + cy.location('href').then((href) => + cy.visit(href.replace(/\/Task_2\/1(?=(?:\?|#|$))/, '/Task_1')), + );
55-63: Ditto for navigation away from receipt.
Use the same resilient pattern.- cy.url().then((url) => cy.visit(url.replace(/\/ProcessEnd$/, '/Task_4'))); + cy.location('href').then((href) => + cy.visit(href.replace(/\/ProcessEnd(?=(?:\?|#|$))/, '/Task_4')), + );src/features/instance/useProcessNext.tsx (1)
44-47: Remove duplicate hook call; reuse one reference.
useOnFormSubmitValidation()is called twice under two names.- const onFormSubmitValidation = useOnFormSubmitValidation(); + const runFormSubmitValidation = useOnFormSubmitValidation(); @@ - const onSubmitFormValidation = useOnFormSubmitValidation(); @@ - const hasErrors = await onFormSubmitValidation(); + const hasErrors = await runFormSubmitValidation(); @@ - const hasValidationErrors = await onSubmitFormValidation(true); + const hasValidationErrors = await runFormSubmitValidation(true);src/features/datamodel/utils.ts (2)
104-108: Strengthen typing: explicit return type and tuple alias for readabilityGive the function an explicit boolean return type and use a named, labeled tuple type instead of raw indices to avoid “magic numbers”.
-export function isDataTypeWritable( - dataType: string | undefined, - isStateless: boolean, - dataElements: (readonly [string, boolean])[], -) { +export function isDataTypeWritable( + dataType: string | undefined, + isStateless: boolean, + dataElements: readonly DataElementTuple[], +): boolean {Add this type once in the module (outside the selected range):
export type DataElementTuple = readonly [dataType: string, locked: boolean];
115-116: Minor clarity: handle undefined result explicitlyAvoid relying on coercion; return false when the element isn’t found, and keep the boolean semantics obvious.
- return !!dataElement && !dataElement[1]; + return dataElement ? !dataElement[1] : false;src/features/datamodel/DataModelsProvider.tsx (3)
159-163: Prefer isSuccess over isFetching; tighten select typing to dropas constUsing isSuccess better models “data is ready” than isFetching (which also toggles during background refetches). Also, provide a concrete return type for select to avoid the
as constassertion.- const { data: dataElements, isFetching } = useInstanceDataQuery({ + const { data: dataElements, isSuccess } = useInstanceDataQuery<readonly DataElementTuple[]>({ enabled: !isStateless, - select: (data) => data.data.map((element) => [element.dataType, element.locked] as const), + select: (data) => data.data.map(({ dataType, locked }) => [dataType, locked]) as const, });(Assumes
DataElementTuplefrom utils is exported; otherwise define a local alias.)
195-205: Avoid repeated linear scans by precomputing a SetMicro-optimization and clearer intent when checking for presence across many data types.
- if (!isStateless && !dataElements?.find(([dt]) => dt === dataType)) { + const types = new Set((dataElements ?? []).map(([dt]) => dt)); + if (!isStateless && !types.has(dataType)) { const error = new MissingDataElementException(dataType); window.logErrorOnce(error.message); continue; } @@ - if (isDataTypeWritable(dataType, isStateless, dataElements ?? [])) { + if (isDataTypeWritable(dataType, isStateless, dataElements ?? [])) { writableDataTypes.push(dataType); }Note: declare
typesonce before the loop if you keep using it multiple times.
209-209: Keep dependencies in sync with control flowIf you adopt
isSuccess, removeisFetchingfrom the deps and addisSuccess. Also,dataElementscan be omitted if you derive the Set inside the effect whenisSuccessis true.- }, [applicationMetadata, defaultDataType, isStateless, layouts, setDataTypes, dataElements, layoutSetId, isFetching]); + }, [applicationMetadata, defaultDataType, isStateless, layouts, setDataTypes, layoutSetId, isSuccess]);src/features/navigation/utils.ts (1)
22-27: Ensure boolean return value
pageGroups || taskGroups.lengthcan yield a non-boolean (object or number). Cast to boolean to keep types predictable.- return !isReceiptPage && (pageGroups || taskGroups.length); + return !isReceiptPage && !!(pageGroups || taskGroups.length > 0);src/components/form/Form.tsx (1)
239-258: Remove unused dependency and variableonFormSubmitValidation is declared but not used; also listed in deps causing needless effect runs.
- const onFormSubmitValidation = useOnFormSubmitValidation(); const exitSubform = useQueryKey(SearchParams.ExitSubform)?.toLocaleLowerCase() === 'true'; const validate = useQueryKey(SearchParams.Validate)?.toLocaleLowerCase() === 'true'; const navigate = useNavigate(); const searchStringRef = useAsRef(useLocation().search); @@ - }, [navigate, searchStringRef, exitSubform, validate, onFormSubmitValidation]); + }, [navigate, searchStringRef, exitSubform, validate]);src/core/routing/useIsReceiptPage.ts (1)
4-7: Receipt detection hook looks good; minor readability tweak optionalIf the list grows, a Set reads cleaner.
- return taskId === TaskKeys.CustomReceipt || taskId === TaskKeys.ProcessEnd; + return new Set([TaskKeys.CustomReceipt, TaskKeys.ProcessEnd]).has(taskId);src/hooks/useNavigatePage.ts (1)
235-240: Avoid emitting trailing “?” when no search paramsWhen stateless, the URL can end with a bare “?” if there are no params.
Apply this diff:
- if (isStatelessApp && orderRef.current[0] !== undefined && (!currentPageId || !isValidPageId(currentPageId))) { - navigate(`/${orderRef.current[0]}?${searchParamsRef.current}`, { replace: true }); - } + if (isStatelessApp && orderRef.current[0] !== undefined && (!currentPageId || !isValidPageId(currentPageId))) { + const q = searchParamsRef.current.toString(); + const qs = q ? `?${q}` : ''; + navigate(`/${orderRef.current[0]}${qs}`, { replace: true }); + }src/features/receipt/FixWrongReceiptType.tsx (1)
21-26: Prefer const expression for redirect targetMinor readability: compute
redirectToas a single const expression.Apply this diff:
- let redirectTo: undefined | TaskKeys = undefined; - if (taskId === TaskKeys.ProcessEnd && hasCustomReceipt) { - redirectTo = TaskKeys.CustomReceipt; - } else if (taskId === TaskKeys.CustomReceipt && !hasCustomReceipt) { - redirectTo = TaskKeys.ProcessEnd; - } + const redirectTo: TaskKeys | undefined = + taskId === TaskKeys.ProcessEnd && hasCustomReceipt + ? TaskKeys.CustomReceipt + : taskId === TaskKeys.CustomReceipt && !hasCustomReceipt + ? TaskKeys.ProcessEnd + : undefined;src/layout/GenericComponent.tsx (1)
230-233: Non-standard scroll behavior value
scrollIntoView({ behavior: 'instant' })is non-standard. Use'auto'(or'smooth') to avoid cross-browser issues.- !abortController.current.signal.aborted && div.scrollIntoView({ behavior: 'instant' }); + !abortController.current.signal.aborted && div.scrollIntoView({ behavior: 'auto' });src/components/presentation/Presentation.test.tsx (1)
91-97: Rename tests to match new hook-driven behavior (cosmetic)Titles still reference ProcessTaskType; assertions are now driven by
useIsReceiptPage. Rename for clarity.Example:
- “background is greyLight when not receipt page”
- “background is greenLight when receipt page”
Also applies to: 99-107
src/features/processEnd/feedback/Feedback.tsx (1)
55-58: Comment/backoff mismatchThe current formula reaches the 30s max delay around attempt ≈ 39 (starting from 0), not 20. Either adjust the formula or update the comment to reflect reality.
📜 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 (33)
src/App.tsx(3 hunks)src/components/form/Form.tsx(2 hunks)src/components/form/LinkToPotentialNode.tsx(1 hunks)src/components/presentation/Presentation.test.tsx(6 hunks)src/components/presentation/Presentation.tsx(5 hunks)src/components/wrappers/ProcessWrapper.tsx(3 hunks)src/core/loading/Loader.tsx(0 hunks)src/core/routing/types.ts(1 hunks)src/core/routing/useIsNavigating.ts(1 hunks)src/core/routing/useIsReceiptPage.ts(1 hunks)src/features/datamodel/DataModelsProvider.tsx(4 hunks)src/features/datamodel/utils.ts(1 hunks)src/features/expressions/expression-functions.ts(1 hunks)src/features/instance/useProcessNext.tsx(1 hunks)src/features/instance/useProcessQuery.ts(2 hunks)src/features/instantiate/selection/InstanceSelection.tsx(1 hunks)src/features/navigation/AppNavigation.tsx(2 hunks)src/features/navigation/utils.ts(1 hunks)src/features/pdf/PDFWrapper.test.tsx(1 hunks)src/features/processEnd/feedback/Feedback.tsx(3 hunks)src/features/propagateTraceWhenPdf/index.ts(1 hunks)src/features/receipt/FixWrongReceiptType.tsx(1 hunks)src/features/receipt/ReceiptContainer.tsx(3 hunks)src/hooks/navigation.ts(1 hunks)src/hooks/useIsPdf.ts(1 hunks)src/hooks/useNavigatePage.ts(2 hunks)src/layout/GenericComponent.tsx(2 hunks)src/layout/RepeatingGroup/EditContainer/RepeatingGroupEditContext.tsx(1 hunks)src/layout/RepeatingGroup/Providers/RepeatingGroupFocusContext.tsx(1 hunks)src/layout/Subform/SubformWrapper.tsx(1 hunks)src/layout/Tabs/Tabs.tsx(1 hunks)test/e2e/integration/stateless-app/feedback.ts(1 hunks)test/e2e/integration/stateless-app/receipt.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/core/loading/Loader.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/propagateTraceWhenPdf/index.tssrc/features/pdf/PDFWrapper.test.tsxsrc/features/instance/useProcessNext.tsxsrc/layout/Subform/SubformWrapper.tsxsrc/features/instantiate/selection/InstanceSelection.tsxsrc/features/receipt/FixWrongReceiptType.tsxsrc/core/routing/useIsReceiptPage.tssrc/hooks/useIsPdf.tssrc/layout/Tabs/Tabs.tsxsrc/layout/RepeatingGroup/EditContainer/RepeatingGroupEditContext.tsxsrc/features/navigation/utils.tstest/e2e/integration/stateless-app/receipt.tssrc/features/instance/useProcessQuery.tssrc/components/form/LinkToPotentialNode.tsxsrc/features/processEnd/feedback/Feedback.tsxsrc/core/routing/types.tssrc/core/routing/useIsNavigating.tstest/e2e/integration/stateless-app/feedback.tssrc/components/presentation/Presentation.test.tsxsrc/components/form/Form.tsxsrc/features/expressions/expression-functions.tssrc/features/receipt/ReceiptContainer.tsxsrc/layout/RepeatingGroup/Providers/RepeatingGroupFocusContext.tsxsrc/components/presentation/Presentation.tsxsrc/features/datamodel/DataModelsProvider.tsxsrc/layout/GenericComponent.tsxsrc/features/datamodel/utils.tssrc/hooks/navigation.tssrc/components/wrappers/ProcessWrapper.tsxsrc/App.tsxsrc/features/navigation/AppNavigation.tsxsrc/hooks/useNavigatePage.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/features/pdf/PDFWrapper.test.tsxsrc/components/presentation/Presentation.test.tsx
🧠 Learnings (2)
📚 Learning: 2025-08-22T13:53:28.252Z
Learnt from: CR
PR: Altinn/app-frontend-react#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.252Z
Learning: Applies to **/*.{ts,tsx} : For TanStack Query, use objects to manage query keys and functions, and centralize shared options via `queryOptions`
Applied to files:
src/features/instance/useProcessQuery.ts
📚 Learning: 2025-08-22T13:53:28.252Z
Learnt from: CR
PR: Altinn/app-frontend-react#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.252Z
Learning: Applies to **/*.test.{ts,tsx} : In tests, use `renderWithProviders` from `src/test/renderWithProviders.tsx` to supply required form layout context
Applied to files:
src/components/presentation/Presentation.test.tsxsrc/features/datamodel/DataModelsProvider.tsx
🧬 Code graph analysis (12)
src/features/pdf/PDFWrapper.test.tsx (1)
src/components/presentation/Presentation.tsx (1)
PresentationComponent(32-93)
src/layout/Subform/SubformWrapper.tsx (1)
src/components/presentation/Presentation.tsx (1)
PresentationComponent(32-93)
src/features/instantiate/selection/InstanceSelection.tsx (1)
src/components/presentation/Presentation.tsx (1)
PresentationComponent(32-93)
src/features/receipt/FixWrongReceiptType.tsx (5)
src/hooks/navigation.ts (1)
useNavigationParam(51-55)src/features/form/layoutSets/LayoutSetsProvider.tsx (1)
useLayoutSets(52-52)src/utils/formLayout.ts (1)
behavesLikeDataTask(8-14)src/hooks/useNavigatePage.ts (1)
useNavigateToTask(152-182)src/core/loading/Loader.tsx (1)
Loader(14-36)
src/core/routing/useIsReceiptPage.ts (1)
src/hooks/navigation.ts (1)
useNavigationParam(51-55)
src/features/processEnd/feedback/Feedback.tsx (3)
src/features/instance/useProcessQuery.ts (2)
useProcessQuery(23-26)useOptimisticallyUpdateProcess(99-106)src/hooks/useNavigatePage.ts (1)
useNavigateToTask(152-182)src/features/instance/InstanceContext.tsx (1)
useInstanceDataQuery(123-132)
test/e2e/integration/stateless-app/feedback.ts (1)
test/e2e/pageobjects/app-frontend.ts (1)
AppFrontend(3-371)
src/features/receipt/ReceiptContainer.tsx (3)
src/features/receipt/FixWrongReceiptType.tsx (1)
FixWrongReceiptType(15-39)src/components/presentation/Presentation.tsx (1)
PresentationComponent(32-93)src/features/form/FormContext.tsx (1)
FormProvider(43-86)
src/components/presentation/Presentation.tsx (7)
src/features/instance/InstanceContext.tsx (1)
useInstanceDataQuery(123-132)src/features/form/layout/UiConfigContext.tsx (1)
useUiConfigContext(44-44)src/features/navigation/utils.ts (1)
useHasGroupedNavigation(22-27)src/core/routing/useIsReceiptPage.ts (1)
useIsReceiptPage(4-7)src/features/language/Lang.tsx (1)
Lang(15-23)src/theme/altinnAppTheme.tsx (1)
AltinnPalette(2-25)src/features/form/layoutSettings/LayoutSettingsContext.tsx (1)
usePageSettings(131-140)
src/features/datamodel/DataModelsProvider.tsx (2)
src/features/instance/InstanceContext.tsx (1)
useInstanceDataQuery(123-132)src/features/datamodel/utils.ts (2)
MissingDataElementException(27-36)isDataTypeWritable(104-117)
src/layout/GenericComponent.tsx (1)
src/core/routing/useIsNavigating.ts (1)
useIsNavigating(3-7)
src/components/wrappers/ProcessWrapper.tsx (6)
src/hooks/useNavigatePage.ts (1)
useIsValidTaskId(184-202)src/features/instance/useProcessQuery.ts (2)
useGetTaskTypeById(61-89)useProcessQuery(23-26)src/features/form/FormContext.tsx (1)
FormProvider(43-86)src/features/receipt/ReceiptContainer.tsx (1)
CustomReceipt(92-112)src/features/instance/useProcessNext.tsx (1)
getProcessNextMutationKey(30-35)src/core/routing/useIsNavigating.ts (1)
useIsNavigating(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Install
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (34)
src/core/routing/types.ts (2)
1-7: Centralized SearchParams: LGTMGood move consolidating routing search keys into a single exported source.
1-7: All SearchParams imports have been migrated
No occurrences ofimport { SearchParams } from 'src/hooks/navigation'remain.src/layout/RepeatingGroup/Providers/RepeatingGroupFocusContext.tsx (1)
6-6: Import path switched to core/routing: LGTMConsistent with the centralization of SearchParams.
src/features/expressions/expression-functions.ts (1)
4-4: Import path switched to core/routing: LGTMNo behavior change; linkToComponent still builds URLs with FocusComponentId as before.
src/layout/RepeatingGroup/EditContainer/RepeatingGroupEditContext.tsx (1)
6-6: Import path switched to core/routing: LGTMMatches the new single source of truth for search params.
src/layout/Tabs/Tabs.tsx (1)
7-7: Import path switched to core/routing: LGTMKeeps tab auto-activation behavior intact when focusing a component via URL.
src/components/form/LinkToPotentialNode.tsx (1)
5-5: Import path migration looks correct.
Keeps routing params centralized in core/routing.src/features/propagateTraceWhenPdf/index.ts (1)
3-3: Import path migration looks correct.
Aligns with new routing/types location.src/features/instance/useProcessQuery.ts (1)
25-26: Good: queryOptions + useQuery pattern.
Matches our TanStack Query guideline for centralized keys/options.src/features/instance/useProcessNext.tsx (1)
92-92: Nice: parallel refetch cuts latency.
Refetching process and instance in parallel is a solid improvement.src/features/datamodel/DataModelsProvider.tsx (1)
293-295: Hook return shapes unchanged
useInstanceDataElementsstill returnsIData[](objects with.idand.dataType), andgetFirstDataElementIdcontinues to accept that array—no downstream updates required.src/features/pdf/PDFWrapper.test.tsx (1)
56-59: LGTM: aligns test with PresentationComponent APIRemoving the obsolete
typeprop matches the new receipt-driven Presentation logic.src/features/navigation/utils.ts (1)
13-13: LGTM: updated import pathImporting
useIsReceiptPagefrom the new routing location is consistent with the refactor.src/features/instantiate/selection/InstanceSelection.tsx (2)
48-51: LGTM: remove deprecatedtypepropPresentation is now receipt-aware; dropping the
typeprop here is correct.
1-340: Audit ProcessTaskType usage; PresentationComponent check passed
- No lingering
type=props found onPresentationComponent.ProcessTaskTypeis still referenced in multiple files (e.g.,src/types/index.ts,src/hooks/useNavigatePage.ts,src/layout/SigneeList/index.tsx, etc.)—verify and remove or refactor any obsolete occurrences.src/components/form/Form.tsx (2)
10-10: Centralize SearchParams import — OKImport moved to core routing. Consistent with the refactor.
47-48: Clarify allowed values for validate paramHere any truthy value triggers validation, while elsewhere you strictly compare to 'true'. Align semantics or document accepted values ('true' vs '1'/presence).
src/hooks/useIsPdf.ts (1)
1-2: Import path update — OKSearchParams now comes from core routing; hook behavior unchanged.
src/layout/Subform/SubformWrapper.tsx (1)
26-29: Updated PresentationComponent usage — OKRemoval of the type prop matches the new Presentation API.
src/features/navigation/AppNavigation.tsx (2)
7-7: useIsReceiptPage import path update — OKAligned with new core routing hook location; no logic change.
16-16: Retained useIsSubformPage from hooks — OKSplit sources are consistent.
src/hooks/useNavigatePage.ts (2)
5-5: Import move looks goodImporting SearchParams from core routing types is consistent with the refactor.
165-171: Receipt remapping in task navigation is solidGood normalization of receipt destinations to the actual configured receipt type.
src/features/receipt/ReceiptContainer.tsx (2)
84-88: Good: Centralized receipt-type correction wrapperWrapping DefaultReceipt with FixWrongReceiptType ensures correct redirection without duplicating route logic.
106-111: Good: Custom receipt guarded and wrappedGuarding missing data model and using the wrapper keeps navigation consistent while avoiding redirect loops.
src/App.tsx (5)
13-13: Route import simplification is correctUsing only DefaultReceipt aligns with the wrapper-based redirection.
36-36: Removal of type-prop from PresentationComponent is consistentMatches the move to receipt-page detection.
61-63: Receipt route wiring looks correctProcessEnd now renders DefaultReceipt (with fix wrapper), simplifying routing.
84-86: Presentation without type-prop in PDF flow is consistentGood alignment with Presentation changes.
60-74: Verify deep links to CustomReceipt still work as intendedSince CustomReceipt has no dedicated route, confirm that deep links to
/instance/:partyId/:guid/CustomReceiptstill render correctly (custom layout) or are redirected appropriately via FixWrongReceiptType or process logic.Steps:
- In an app with a custom receipt, open an instance at
/ProcessEndand ensure redirect to custom receipt occurs.- In an app without a custom receipt, navigate to
/CustomReceiptand verify redirect/fallback to/ProcessEnd.- Confirm browser history contains a single entry after redirects (replace = true).
src/hooks/navigation.ts (1)
4-5: Good: centralizing SearchParams typing improves safetySwitching
useQueryKeyto accept the sharedSearchParamstype removes stringly-typed keys and prevents typos. LGTM.Also applies to: 60-61
src/components/presentation/Presentation.tsx (3)
43-47: Receipt-aware presentation is correctUsing
useIsReceiptPage()to select header, background, and hide navigation on receipts is consistent with the new routing model.
73-78: Substatus only on receiptsConditionally rendering
AltinnSubstatusonly for receipts (and when a substatus exists) avoids confusing intermediate states.
84-85: Progress bar placement and simplification look goodMoving the
ProgressBarintoHeaderand rendering only whenshowProgressis true reads well and avoids special-casing in receipt views.Also applies to: 95-99
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/presentation/Presentation.test.tsx (1)
111-116: Remove legacytypeprop and import; they’re no longer part of the APITests still pass a
typeprop and importProcessTaskType, which conflicts with the stated refactor.Apply this diff:
-import { ProcessTaskType } from 'src/types'; @@ const allProps = { header: 'Header text', - type: ProcessTaskType.Unknown, ...props, };
♻️ Duplicate comments (5)
src/components/presentation/Presentation.test.tsx (1)
32-35: Restore property spies to avoid cross-test bleed — addressedAdding
jest.restoreAllMocks()fixes the teardown gap flagged earlier. Nice.src/features/processEnd/feedback/Feedback.tsx (2)
24-31: LGTM: ended-first check and data-task navigationPrioritizing
endedbefore task change prevents transient misnavigation and enables routing to non-EndEvent tasks (e.g., data tasks). This addresses the core bug.
13-14: Fix stale previousData capture and avoid unnecessary backoff resetsThe callback closes over
previousData, and including it in the dependency array restarts polling frequently. Track the last task id in a ref and compare against that inside a stable callback.- const { refetch: reFetchProcessData, data: previousData } = useProcessQuery(); + const { refetch: reFetchProcessData, data: previousData } = useProcessQuery(); + const previousTaskRef = useRef<string | undefined>(previousData?.currentTask?.elementId); + useEffect(() => { + previousTaskRef.current = previousData?.currentTask?.elementId; + }, [previousData?.currentTask?.elementId]); @@ - const callback = useCallback(async () => { + const callback = useCallback(async () => { const result = await reFetchProcessData(); let navigateTo: undefined | string; if (result.data?.ended) { navigateTo = TaskKeys.ProcessEnd; } else if ( result.data?.currentTask?.elementId && - result.data.currentTask.elementId !== previousData?.currentTask?.elementId + result.data.currentTask.elementId !== previousTaskRef.current ) { navigateTo = result.data.currentTask.elementId; } if (navigateTo && result.data) { optimisticallyUpdateProcess(result.data); await reFetchInstanceData(); navigateToTask(navigateTo); } - }, [ - navigateToTask, - optimisticallyUpdateProcess, - previousData?.currentTask?.elementId, - reFetchInstanceData, - reFetchProcessData, - ]); + }, [navigateToTask, optimisticallyUpdateProcess, reFetchInstanceData, reFetchProcessData]); @@ - useBackoff(callback); + useBackoff(callback);Also applies to: 21-44, 47-47
src/components/wrappers/ProcessWrapper.tsx (2)
71-94: Make process/next gating reactive with useIsMutatingUsing
queryClient.isMutatingsnapshots once and may miss updates; preferuseIsMutating.Apply this diff:
@@ - const processNextKey = getProcessNextMutationKey(); - const queryClient = useQueryClient(); - const isRunningProcessNext = queryClient.isMutating({ mutationKey: processNextKey }); + const processNextKey = getProcessNextMutationKey(); + const isRunningProcessNext = useIsMutating({ mutationKey: processNextKey }) > 0;And below:
@@ -import { useQueryClient } from '@tanstack/react-query'; -import type { QueryClient } from '@tanstack/react-query'; +import { useIsMutating, useQueryClient } from '@tanstack/react-query'; +import type { QueryClient } from '@tanstack/react-query';
181-193: Simplify and modernize useIsRunningProcessNextReplace stateful snapshot with
useIsMutatingto avoid stale state.Apply this diff:
-function useIsRunningProcessNext() { - const queryClient = useQueryClient(); - const [isMutating, setIsMutating] = useState<boolean | null>(null); - - // Intentionally wrapped in a useEffect() and saved as a state. If this happens, we'll seemingly be locked out - // with a <Loader /> forever, but when this happens, we also know we'll be re-rendered soon. This is only meant to - // block rendering when we're calling process/next. - useEffect(() => { - setIsMutating(isRunningProcessNext(queryClient)); - }, [queryClient]); - - return isMutating; -} +function useIsRunningProcessNext() { + const count = useIsMutating({ mutationKey: getProcessNextMutationKey() }); + return count > 0; +}
🧹 Nitpick comments (9)
src/components/presentation/Presentation.test.tsx (3)
37-50: Assert the query function is actually usedOptional: wrap
fetchReturnUrlin a mock and assert it was called; improves test intent and guards refactors.Apply this diff within this test:
it('should link to query parameter returnUrl if valid URL', async () => { const returnUrl = 'foo'; const mockedLocation = { ...realLocation, search: `?returnUrl=${returnUrl}` }; jest.spyOn(window, 'location', 'get').mockReturnValue(mockedLocation); - await render({}, { fetchReturnUrl: async () => returnUrl }); + const fetchReturnUrlMock = jest.fn().mockResolvedValue(returnUrl); + await render({}, { fetchReturnUrl: fetchReturnUrlMock }); const closeButton = screen.getByRole('link', { name: 'Tilbake', }); expect(closeButton).toHaveAttribute('href', returnUrl); + expect(fetchReturnUrlMock).toHaveBeenCalled(); });
92-98: Rename test to reflect hook semantics, notProcessTaskType.DataThe component no longer uses a
typeprop; the name should expressisReceiptPage=false.Apply this diff:
- it('the background color should be greyLight if type is "ProcessTaskType.Data"', async () => { + it('the background color should be greyLight when isReceiptPage=false', async () => {
100-108: Tighten semantics and scope the mock to this test
- Rename to reflect
isReceiptPage=true.- Prefer
mockReturnValueOnce(true)for tighter scope.Apply this diff:
- it('the background color should be lightGreen if type is "ProcessTaskType.Archived"', async () => { - mockUseIsReceiptPage.mockReturnValue(true); + it('the background color should be lightGreen when isReceiptPage=true', async () => { + mockUseIsReceiptPage.mockReturnValueOnce(true);src/features/instance/useProcessNext.tsx (1)
92-93: Parallel refetch is right; consider allSettled to avoid aborting on a single failureIf either refetch rejects,
Promise.allwill reject and skip invalidation/navigation work. UsingallSettledis a low-risk hardening.- await Promise.all([refetchProcessData(), reFetchInstanceData()]); + await Promise.allSettled([refetchProcessData(), reFetchInstanceData()]);src/features/processEnd/feedback/Feedback.tsx (1)
59-85: Backoff comments don’t match the math; also clear timers on unmount
- The code reaches 30s delay near attempt ~40, not 20. Fix the comment for accuracy.
- Clear the timeout on cleanup to avoid stray queued callbacks; you already guard with
shouldContinue, but clearing is cleaner.function useBackoff(callback: () => Promise<void>) { // The backoff algorithm is used to check the process data, and slow down the requests after a while. // At first, it starts off once a second (every 1000ms) for 10 seconds. - // After that, it slows down by one more second for every request. - // Once it reaches 20 attempts, it will reach the max delay of 30 seconds between each request. + // After that, it slows down by one more second for every request. + // It reaches the max delay of 30 seconds between each request after ~40 attempts with the current formula. const attempts = useRef(0); + const timeoutRef = useRef<number | undefined>(undefined); useEffect(() => { let shouldContinue = true; function continueCalling() { const backoff = attempts.current < 10 ? 1000 : Math.min(30000, 1000 + (attempts.current - 10) * 1000); - setTimeout(() => { + timeoutRef.current = window.setTimeout(() => { if (!shouldContinue) { return; } callback().then(); attempts.current++; shouldContinue && continueCalling(); }, backoff); } continueCalling(); return () => { shouldContinue = false; + if (timeoutRef.current !== undefined) { + clearTimeout(timeoutRef.current); + } }; }, [callback]); }src/features/receipt/FixWrongReceiptType.tsx (1)
29-33: Harden logging
window.logWarnOncemay be undefined in some environments; guard it and fall back toconsole.warn.See the logging changes included in the diff above.
src/features/receipt/ReceiptContainer.tsx (1)
101-110: Avoid duplicate useLanguage calls and use the existing langToolsMinor cleanup: you call
useLanguage()twice; reuselangToolsforlangAsString.Apply this diff:
@@ - const langTools = useLanguage(); + const langTools = useLanguage(); @@ - const { langAsString } = useLanguage(); @@ - <title>{`${getPageTitle(appName, langAsString('receipt.title'), appOwner)}`}</title> + <title>{`${getPageTitle(appName, langTools.langAsString('receipt.title'), appOwner)}`}</title>Also applies to: 176-179
src/components/wrappers/ProcessWrapper.tsx (2)
195-228: Return a strict boolean from useIsWrongTaskEnsure the hook returns
true|false, notnullor a truthy value; avoids ambiguous checks.Apply this diff:
- return isWrongTask && !isCurrentTask && !isNavigating; + return isWrongTask === true && !isCurrentTask && !isNavigating;
120-126: Optional: Prefer auto-navigation to current task over showing an error on Feedback/ConfirmTo better satisfy “Feedback step should navigate to a data task,” render Feedback/Confirm even if
isWrongTaskjust flipped and let them drive navigation; move theisWrongTaskguard below these branches or auto-navigate when wrong.Proposed reordering:
- if (isWrongTask) { - return ( - <PresentationComponent showNavigation={false}> - <NavigationError label='general.part_of_form_completed' /> - </PresentationComponent> - ); - } @@ if (taskType === ProcessTaskType.Feedback) { return ( <PresentationComponent> <Feedback /> </PresentationComponent> ); } + + if (isWrongTask) { + return ( + <PresentationComponent showNavigation={false}> + <NavigationError label='general.part_of_form_completed' /> + </PresentationComponent> + ); + }Also applies to: 136-142
📜 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 (7)
src/App.tsx(4 hunks)src/components/presentation/Presentation.test.tsx(6 hunks)src/components/wrappers/ProcessWrapper.tsx(3 hunks)src/features/instance/useProcessNext.tsx(2 hunks)src/features/processEnd/feedback/Feedback.tsx(3 hunks)src/features/receipt/FixWrongReceiptType.tsx(1 hunks)src/features/receipt/ReceiptContainer.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/instance/useProcessNext.tsxsrc/App.tsxsrc/features/receipt/FixWrongReceiptType.tsxsrc/components/wrappers/ProcessWrapper.tsxsrc/features/receipt/ReceiptContainer.tsxsrc/components/presentation/Presentation.test.tsxsrc/features/processEnd/feedback/Feedback.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/components/presentation/Presentation.test.tsx
🧠 Learnings (2)
📚 Learning: 2025-08-28T12:00:31.684Z
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3645
File: src/features/datamodel/DataModelsProvider.tsx:172-175
Timestamp: 2025-08-28T12:00:31.684Z
Learning: In DataModelsProvider.tsx, the effect that processes referenced data types must run for both stateless and non-stateless apps. The `isFetching` guard is appropriate because stateless apps don't fetch instance data (useInstanceDataQuery is disabled with `enabled: !isStateless`) but still need the data type processing to occur.
Applied to files:
src/features/instance/useProcessNext.tsx
📚 Learning: 2025-08-22T13:53:28.252Z
Learnt from: CR
PR: Altinn/app-frontend-react#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.252Z
Learning: Applies to **/*.test.{ts,tsx} : In tests, use `renderWithProviders` from `src/test/renderWithProviders.tsx` to supply required form layout context
Applied to files:
src/components/presentation/Presentation.test.tsx
🧬 Code graph analysis (4)
src/App.tsx (3)
src/components/presentation/Presentation.tsx (1)
PresentationComponent(32-93)src/features/receipt/FixWrongReceiptType.tsx (1)
FixWrongReceiptType(16-47)src/features/form/FormContext.tsx (1)
FormProvider(43-86)
src/features/receipt/FixWrongReceiptType.tsx (6)
src/hooks/navigation.ts (1)
useNavigationParam(51-55)src/features/form/layoutSets/LayoutSetsProvider.tsx (1)
useLayoutSets(52-52)src/utils/formLayout.ts (1)
behavesLikeDataTask(8-14)src/hooks/useNavigatePage.ts (1)
useNavigateToTask(152-182)src/features/datamodel/useBindingSchema.tsx (1)
useCurrentDataModelGuid(32-50)src/core/loading/Loader.tsx (1)
Loader(14-36)
src/features/receipt/ReceiptContainer.tsx (2)
src/features/receipt/FixWrongReceiptType.tsx (1)
FixWrongReceiptType(16-47)src/components/presentation/Presentation.tsx (1)
PresentationComponent(32-93)
src/features/processEnd/feedback/Feedback.tsx (3)
src/features/instance/useProcessQuery.ts (2)
useProcessQuery(23-26)useOptimisticallyUpdateProcess(99-106)src/hooks/useNavigatePage.ts (1)
useNavigateToTask(152-182)src/features/instance/InstanceContext.tsx (1)
useInstanceDataQuery(123-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
src/components/presentation/Presentation.test.tsx (5)
8-8: Switch to receipt-page hook aligns tests with new routing logicImporting and relying on
useIsReceiptPageis the right pivot away from the legacytypeprop.
18-23: Mock is set up correctly and typedGood use of a module factory and
jest.MockedFunctionforuseIsReceiptPage.
27-31: DefaultingisReceiptPageto false in beforeEach prevents leakageSolid baseline for each test.
59-60: Hook-driven default rendering path is correctRendering with no explicit
typeprop matches the new behavior.
75-76: Good: default path whenreturnUrlis missingKeeps behavior consistent and readable.
src/features/instance/useProcessNext.tsx (1)
38-38: Good: lazy instance refetch prevents extra observerSwitching to
useInstanceDataQuery({ enabled: false }).refetchavoids an unnecessary query observer and matches our prior learning for stateless vs. non-stateless flows.src/features/processEnd/feedback/Feedback.tsx (1)
19-20: Good: disable instance query and use only refetch
useInstanceDataQuery({ enabled: false }).refetchavoids an extra live observer on the feedback page while still letting you pull when needed.src/features/receipt/ReceiptContainer.tsx (1)
76-83: Good use of wrapper to normalize receipt routingWrapping DefaultReceipt with FixWrongReceiptType ensures correct target receipt without duplicating routing concerns elsewhere.
src/App.tsx (2)
61-65: Explicit ProcessEnd route rendering via DefaultReceipt looks correctDirectly routing ProcessEnd to DefaultReceipt aligns with the new receipt normalization.
66-76: Receipt guard applied to entire task flow is appropriateWrapping the task route with FixWrongReceiptType centralizes receipt redirection.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/processEnd/feedback/Feedback.tsx (1)
63-90: Prevent overlapping polls and handle errors; align backoff math with comments
- The current loop fires the next timeout without awaiting
callback(), so polls can overlap and spike load; re-schedule only after the promise settles.- Swallow rejections to keep polling alive.
- Fix the off-by-one in backoff (attempt 10 should move to 2s) and update comments to reflect actual behavior.
function useBackoff(callback: () => Promise<void>) { - // The backoff algorithm is used to check the process data, and slow down the requests after a while. - // At first, it starts off once a second (every 1000ms) for 10 seconds. - // After that, it slows down by one more second for every request. - // Once it reaches 20 attempts, it will reach the max delay of 30 seconds between each request. + // Poll with increasing delay to reduce load over time. + // - 1s delay for the first 10 attempts + // - then increase by 1s per attempt, up to a max of 30s const attempts = useRef(0); useEffect(() => { - let shouldContinue = true; - function continueCalling() { - const backoff = attempts.current < 10 ? 1000 : Math.min(30000, 1000 + (attempts.current - 10) * 1000); - setTimeout(() => { - if (!shouldContinue) { - return; - } - callback().then(); - attempts.current++; - shouldContinue && continueCalling(); - }, backoff); - } + let shouldContinue = true; + let timer: ReturnType<typeof setTimeout> | undefined; + const computeBackoff = () => + attempts.current < 10 ? 1000 : Math.min(30000, 1000 + (attempts.current - 9) * 1000); // 10th -> 2s + const scheduleNext = (delay: number) => { + timer = setTimeout(async () => { + if (!shouldContinue) return; + try { + await callback(); + } catch { + // keep polling even if one iteration fails + } finally { + attempts.current++; + if (shouldContinue) { + scheduleNext(computeBackoff()); + } + } + }, delay); + }; - continueCalling(); + scheduleNext(1000); return () => { shouldContinue = false; + if (timer) { + clearTimeout(timer); + } }; }, [callback]); }
♻️ Duplicate comments (3)
src/features/processEnd/feedback/Feedback.tsx (1)
13-20: Avoid stale previousData by comparing via a ref; drop deep dep to reduce effect churnCurrent code closes over
previousDataand re-memos on a deep property, which can still race under async polling. Track the latest value in a ref and read from it inside the callback. This also simplifies dependencies.@@ - const { refetch: reFetchProcessData, data: previousData } = useProcessQuery(); + const { refetch: reFetchProcessData, data: previousData } = useProcessQuery(); + const previousDataRef = useRef(previousData); + useEffect(() => { + previousDataRef.current = previousData; + }, [previousData]); @@ - result.data.currentTask.elementId !== previousData?.currentTask?.elementId + result.data.currentTask.elementId !== previousDataRef.current?.currentTask?.elementId @@ - optimisticallyUpdateProcess, - previousData?.currentTask?.elementId, + optimisticallyUpdateProcess, + // previousData is read from previousDataRef to avoid stale capturesAlso applies to: 31-35, 42-48
src/core/routing/useIsNavigating.ts (1)
1-10: Return actual router navigation state; drop brittle hash checkCurrent logic inverts idle and couples to window.location.hash. Use react-router’s state directly.
Apply this diff:
-import { useLocation, useNavigation } from 'react-router-dom'; +import { useNavigation } from 'react-router-dom'; export function useIsNavigating() { - const isIdle = useNavigation().state === 'idle'; - const location = useLocation(); - const expectedLocation = `${location.pathname}${location.search}`.replace(/^\//, ''); - const locationIsUpToDate = window.location.hash.endsWith(expectedLocation); - - return !locationIsUpToDate || !isIdle; + const { state } = useNavigation(); + return state !== 'idle'; // true while submitting/loading }src/components/wrappers/ProcessWrapper.tsx (1)
109-112: Archived→receipt loader is OK (navigation handled elsewhere)Acknowledging the intentional design per project guidance; no change needed.
🧹 Nitpick comments (2)
src/features/datamodel/DataModelsProvider.tsx (2)
173-175: Keep the isFetching guard; add an explanatory comment to prevent regressionsGiven instanceless apps disable the query, the guard must remain on isFetching (not success). Add a clarifying comment.
// Find all data types referenced in dataModelBindings in the layout useEffect(() => { - if (isFetching) { + // Important: For instanceless apps, useInstanceDataQuery is disabled. + // We still need to parse referenced data types in that case. + // Only bail while actively fetching for instance-based apps. + if (isFetching) { return; }
152-160: Avoid double useApplicationMetadata() call; derive isStateless from the existing variableUse the already-read applicationMetadata to prevent redundant context reads and ensure consistency if metadata updates.
const applicationMetadata = useApplicationMetadata(); @@ - const isStateless = useApplicationMetadata().isStatelessApp; + const isStateless = applicationMetadata.isStatelessApp;
📜 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 (5)
src/components/wrappers/ProcessWrapper.tsx(5 hunks)src/core/routing/useIsNavigating.ts(1 hunks)src/features/datamodel/DataModelsProvider.tsx(5 hunks)src/features/processEnd/feedback/Feedback.tsx(3 hunks)src/hooks/useWaitForQueries.ts(1 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/hooks/useWaitForQueries.tssrc/features/processEnd/feedback/Feedback.tsxsrc/components/wrappers/ProcessWrapper.tsxsrc/core/routing/useIsNavigating.tssrc/features/datamodel/DataModelsProvider.tsx
🧠 Learnings (5)
📚 Learning: 2025-08-28T12:25:27.289Z
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3645
File: src/components/wrappers/ProcessWrapper.tsx:107-110
Timestamp: 2025-08-28T12:25:27.289Z
Learning: In ProcessWrapper.tsx, when taskType === ProcessTaskType.Archived and taskId !== TaskKeys.CustomReceipt, the component shows a loader with reason 'redirect-to-receipt' but does not perform explicit navigation. This is intentional - other parts of the system handle the actual navigation closer to where user interactions occur.
Applied to files:
src/features/processEnd/feedback/Feedback.tsxsrc/components/wrappers/ProcessWrapper.tsx
📚 Learning: 2025-08-28T12:25:27.289Z
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3645
File: src/components/wrappers/ProcessWrapper.tsx:107-110
Timestamp: 2025-08-28T12:25:27.289Z
Learning: In the app-frontend-react codebase, the architecture favors navigation happening closer to user interactions or specific process state changes, rather than using universal effects that automatically redirect users. This is part of a design philosophy to avoid "free-standing effects that throw you around" and make navigation more predictable and contextual.
Applied to files:
src/components/wrappers/ProcessWrapper.tsx
📚 Learning: 2025-08-28T12:00:32.967Z
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3645
File: src/features/datamodel/DataModelsProvider.tsx:172-175
Timestamp: 2025-08-28T12:00:32.967Z
Learning: In DataModelsProvider.tsx, the effect that processes referenced data types must run for both stateless and non-stateless apps. The `isFetching` guard is appropriate because stateless apps don't fetch instance data (useInstanceDataQuery is disabled with `enabled: !isStateless`) but still need the data type processing to occur.
Applied to files:
src/features/datamodel/DataModelsProvider.tsx
📚 Learning: 2025-08-28T12:00:32.967Z
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3645
File: src/features/datamodel/DataModelsProvider.tsx:172-175
Timestamp: 2025-08-28T12:00:32.967Z
Learning: In the Altinn app-frontend-react codebase, "stateless" refers to "instanceless" applications. While stateless/instanceless apps don't have server-side instance data, they still require data models to be loaded and processed for local state management.
Applied to files:
src/features/datamodel/DataModelsProvider.tsx
📚 Learning: 2025-08-22T13:53:28.252Z
Learnt from: CR
PR: Altinn/app-frontend-react#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.252Z
Learning: Applies to **/*.test.{ts,tsx} : In tests, use `renderWithProviders` from `src/test/renderWithProviders.tsx` to supply required form layout context
Applied to files:
src/features/datamodel/DataModelsProvider.tsx
🧬 Code graph analysis (3)
src/features/processEnd/feedback/Feedback.tsx (3)
src/features/instance/useProcessQuery.ts (2)
useProcessQuery(23-26)useOptimisticallyUpdateProcess(99-106)src/hooks/useNavigatePage.ts (1)
useNavigateToTask(152-182)src/features/instance/InstanceContext.tsx (1)
useInstanceDataQuery(123-132)
src/components/wrappers/ProcessWrapper.tsx (4)
src/core/routing/useIsNavigating.ts (1)
useIsNavigating(3-10)src/hooks/useNavigatePage.ts (1)
useIsValidTaskId(184-202)src/features/instance/useProcessQuery.ts (2)
useGetTaskTypeById(61-89)useProcessQuery(23-26)src/hooks/useWaitForQueries.ts (1)
useWaitForQueries(11-31)
src/features/datamodel/DataModelsProvider.tsx (2)
src/features/instance/InstanceContext.tsx (1)
useInstanceDataQuery(123-132)src/features/datamodel/utils.ts (2)
MissingDataElementException(27-36)isDataTypeWritable(104-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
src/features/processEnd/feedback/Feedback.tsx (1)
21-36: Good: ended-first check prevents transient misnavigationPrioritizing
result.data.endedbefore task-change comparison is correct and avoids routing to a transient task after process end.src/features/datamodel/DataModelsProvider.tsx (5)
4-4: Good: Blocking UI on in-flight saves with useMutationStateImport looks right and is used to prevent racey initial writes in BlockUntilLoaded.
30-30: Good: Consolidated instance data access via InstanceContext hooksSwitching to useInstanceDataQuery/useInstanceDataElements simplifies data sourcing and typing across the file.
160-163: Tuple-select for dataElements is precise and lightweightThe enabled: !isStateless gate and as const tuple shape work well with downstream utils like isDataTypeWritable.
196-201: Correct: Only require instance data elements for stateful appsThe non-stateless check avoids false positives for instanceless flows; logErrorOnce keeps noise low.
204-206: Writable detection aligned with new dataElements shapeUsing isDataTypeWritable(dataType, isStateless, dataElements ?? []) matches the tuple refactor and preserves stateless semantics.
olemartinorg
force-pushed
the
bug/feedback-step
branch
from
b555580 to
5934d28
Compare
…e() instead. Moving things around to make tests/mocking work.
… same as for every other form page, and can be rendered out more easily in ProcessWrapper.tsx
… check and fix wrong navigation in FixWrongReceiptType.tsx.
… the current one' page shows up correctly
…out to a separate hook for re-use
…e data is not yet fetched again when we arrive at the DataModelsProvider. This causes it to use outdated instance data, marking the data model as read-only when it has been unlocked. This should fix the problem, and hopefully also solve the earlier bug with subforms.
…ithout flickering this message when navigating.
…t looking all that hard.
…/>, so it depended on having its own routing. Baking what's left into FixWrongReceiptType.tsx and removing everything that makes it look like CustomReceipt isn't just any other taskId
… it for refetching
…avigating you to the first page. Updating the hook to make it work without a current location as well.
…cause network takes time the navigation hadn't happened quite yet. Waiting for these to finish before showing this message to the user fixes things.
…) needs info about pages that are hidden, and so it needs to run expressions. In one case in the ttd/payment-test app, when navigating to process/next, the FormDataWrite provider is no longer provided when useNavigatePage() is called, so everything crashes. This uses the lax selector instead, making things not crash anymore.
olemartinorg
force-pushed
the
bug/feedback-step
branch
from
58cc7a0 to
d3d6e69
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/processEnd/feedback/Feedback.tsx (1)
63-90: Prevent overlapping polls, fix off‑by‑one backoff, handle errors, and clear timer on unmount.Current loop can schedule the next tick before the prior async finishes (parallel refetches), the delay is 1s for 11 ticks (off by one vs comment), errors are unhandled, and the timer isn’t cleared on cleanup.
-function useBackoff(callback: () => Promise<void>) { +function useBackoff(callback: () => Promise<void>) { // The backoff algorithm is used to check the process data, and slow down the requests after a while. // At first, it starts off once a second (every 1000ms) for 10 seconds. // After that, it slows down by one more second for every request. // Once it reaches 20 attempts, it will reach the max delay of 30 seconds between each request. - const attempts = useRef(0); + const attempts = useRef(0); + const timeoutIdRef = useRef<number | undefined>(undefined); @@ useEffect(() => { let shouldContinue = true; function continueCalling() { - const backoff = attempts.current < 10 ? 1000 : Math.min(30000, 1000 + (attempts.current - 10) * 1000); - setTimeout(() => { + // After 10 attempts at 1s, increase by 1s per attempt, capped at 30s. + const backoff = + attempts.current < 10 ? 1000 : Math.min(30000, 1000 + (attempts.current - 9) * 1000); + timeoutIdRef.current = window.setTimeout(() => { if (!shouldContinue) { return; } - callback().then(); - attempts.current++; - shouldContinue && continueCalling(); + void callback() + .catch(() => { + // swallow or add debug log; avoid unhandled rejections + }) + .finally(() => { + attempts.current++; + if (shouldContinue) { + continueCalling(); + } + }); - }, backoff); + }, backoff); } continueCalling(); return () => { shouldContinue = false; + if (timeoutIdRef.current !== undefined) { + clearTimeout(timeoutIdRef.current); + } }; }, [callback]); }
♻️ Duplicate comments (4)
src/components/wrappers/ProcessWrapper.tsx (2)
109-111: Archived task shows loader without explicit redirect — intentionalAcknowledging prior decision: redirect handled elsewhere; keeping this as a passive loader is by design.
197-231: Annotate hook return type (nit) and keep semantics explicitThe hook returns boolean | null; make it explicit.
-function useIsWrongTask(taskId: string | undefined) { +function useIsWrongTask(taskId: string | undefined): boolean | null {Note: prior discussions covered timeout cleanup/abort; not rehashing here.
src/features/processEnd/feedback/Feedback.tsx (2)
27-35: Ended-first navigation ordering is correct.Thanks for prioritizing
endedbefore task-change; this prevents transient misnavigation.
27-35: Decouple polling restarts from query cache churn; read previous process via ref.Using
previousData?.currentTask?.elementIdin deps causes the backoff loop to restart on every cache change. Keep a ref of the latest process to compare inside the callback and remove it from deps.- const { refetch: reFetchProcessData, data: previousData } = useProcessQuery(); + const { refetch: reFetchProcessData, data: previousData } = useProcessQuery(); + const previousDataRef = useRef(previousData); + useEffect(() => { + previousDataRef.current = previousData; + }, [previousData]); @@ - } else if ( - result.data.currentTask?.elementId && - result.data.currentTask.elementId !== previousData?.currentTask?.elementId - ) { + } else if ( + result.data.currentTask?.elementId && + result.data.currentTask.elementId !== previousDataRef.current?.currentTask?.elementId + ) { navigateTo = result.data.currentTask.elementId; } @@ }, [ navigateToTask, optimisticallyUpdateProcess, - previousData?.currentTask?.elementId, reFetchInstanceData, reFetchProcessData, ]);Also applies to: 42-48
🧹 Nitpick comments (8)
src/components/wrappers/ProcessWrapper.tsx (2)
78-81: Guard navigate while router is busy; normalize isMutating to booleanGood guard with isNavigating. Minor nit: coerce isMutating count to boolean for consistency with helpers.
- const isRunningProcessNext = queryClient.isMutating({ mutationKey: processNextKey }); + const isRunningProcessNext = queryClient.isMutating({ mutationKey: processNextKey }) > 0;Also applies to: 86-89
179-195: Add explicit return types for helpers (nit)Tiny clarity win; no behavior change.
-function isRunningProcessNext(queryClient: QueryClient) { +function isRunningProcessNext(queryClient: QueryClient): boolean { return queryClient.isMutating({ mutationKey: getProcessNextMutationKey() }) > 0; } -function useIsRunningProcessNext() { +function useIsRunningProcessNext(): boolean | null { const queryClient = useQueryClient(); const [isMutating, setIsMutating] = useState<boolean | null>(null);src/features/processEnd/feedback/Feedback.tsx (1)
13-20: Good: disabled instance query for explicit refetch; also disable focus-refetch.Nice switch to
enabled: false. Consider also disabling focus refetch to avoid accidental fetches.- const reFetchInstanceData = useInstanceDataQuery({ enabled: false }).refetch; + const reFetchInstanceData = useInstanceDataQuery({ enabled: false, refetchOnWindowFocus: false }).refetch;src/features/propagateTraceWhenPdf/index.ts (4)
6-13: Type the cookie map and harden parsing (avoid empty keys, prototype keys).Prevents TS index errors and edge cases when document.cookie is empty.
Apply:
-function getCookies(): { [key: string]: string } { - const cookie = {}; - document.cookie.split(';').forEach((el) => { - const split = el.split('='); - cookie[split[0].trim()] = split.slice(1).join('='); - }); - return cookie; -} +function getCookies(): Record<string, string> { + const cookies: Record<string, string> = Object.create(null); + const raw = typeof document !== 'undefined' ? document.cookie : ''; + if (!raw) return cookies; + raw.split(';').forEach((el) => { + if (!el) return; + const [rawKey, ...rest] = el.split('='); + const key = rawKey?.trim(); + if (!key) return; + cookies[key] = rest.join('='); + }); + return cookies; +}
15-20: Guard against SSR/non‑browser invocation.Avoids ReferenceErrors if called outside the browser (tests/SSR).
export function propagateTraceWhenPdf() { try { - const hash = window.location.hash; + if (typeof window === 'undefined' || typeof document === 'undefined') return; + const hash = window.location.hash; const search = hash.split('?')[1] ?? ''; const isPdf = new URLSearchParams(search).get(SearchParams.Pdf) === '1';
4-6: Prevent duplicate axios interceptors on repeated calls.Without gating/ejecting, repeated invocations can stack interceptors.
import { appPath } from 'src/utils/urls/appUrlHelper'; +let pdfInterceptorId: number | null = null; + function getCookies(): { [key: string]: string } {- if (isPdf) { + if (isPdf && pdfInterceptorId === null) { const cookies = getCookies(); - axios.interceptors.request.use((config) => { + pdfInterceptorId = axios.interceptors.request.use((config) => { try { if (config.url?.startsWith(appPath) !== true) { return config; }Optionally return a disposer from this function (or handle elsewhere) to eject when no longer needed:
// elsewhere when appropriate: // if (pdfInterceptorId !== null) { axios.interceptors.request.eject(pdfInterceptorId); pdfInterceptorId = null; }Also applies to: 21-46
29-39: Header assignment: be compatible with Axios v1 types.Use AxiosHeaders.set when available; otherwise fall back to plain object merge.
- // This header is caught in app-lib/backend and used to allow injection of traceparent/context - config.headers['X-Altinn-IsPdf'] = 'true'; - - const traceparent = cookies['altinn-telemetry-traceparent']; - const tracestate = cookies['altinn-telemetry-tracestate']; - if (traceparent) { - config.headers['traceparent'] = traceparent; - } - if (tracestate) { - config.headers['tracestate'] = tracestate; - } + // This header is caught in app-lib/backend and used to allow injection of traceparent/context + const traceparent = cookies['altinn-telemetry-traceparent']; + const tracestate = cookies['altinn-telemetry-tracestate']; + const h = config.headers as unknown; + if (h && typeof (h as { set?: Function }).set === 'function') { + (h as { set: (k: string, v: string) => void }).set('X-Altinn-IsPdf', 'true'); + if (traceparent) (h as { set: (k: string, v: string) => void }).set('traceparent', traceparent); + if (tracestate) (h as { set: (k: string, v: string) => void }).set('tracestate', tracestate); + } else { + config.headers = { + ...(config.headers ?? {}), + 'X-Altinn-IsPdf': 'true', + ...(traceparent ? { traceparent } : {}), + ...(tracestate ? { tracestate } : {}), + }; + }src/features/receipt/ReceiptContainer.tsx (1)
78-82: Redundant prop: PresentationComponent already hides navigation on receipt pages.PresentationComponent derives isReceipt internally and sets showNavigation to false for receipts. The explicit showNavigation={false} here is unnecessary.
Apply this diff if you keep this block:
- <PresentationComponent showNavigation={false}> + <PresentationComponent>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
src/App.tsx(4 hunks)src/components/form/Form.tsx(2 hunks)src/components/form/LinkToPotentialNode.tsx(1 hunks)src/components/presentation/Presentation.test.tsx(6 hunks)src/components/presentation/Presentation.tsx(5 hunks)src/components/wrappers/ProcessWrapper.tsx(5 hunks)src/core/loading/Loader.tsx(0 hunks)src/core/routing/types.ts(1 hunks)src/core/routing/useIsNavigating.ts(1 hunks)src/core/routing/useIsReceiptPage.ts(1 hunks)src/features/datamodel/DataModelsProvider.tsx(5 hunks)src/features/datamodel/utils.ts(1 hunks)src/features/expressions/expression-functions.ts(2 hunks)src/features/formData/FormDataWrite.tsx(2 hunks)src/features/instance/useProcessNext.tsx(2 hunks)src/features/instance/useProcessQuery.ts(2 hunks)src/features/instantiate/selection/InstanceSelection.tsx(1 hunks)src/features/navigation/AppNavigation.tsx(2 hunks)src/features/navigation/utils.ts(1 hunks)src/features/pdf/PDFWrapper.test.tsx(1 hunks)src/features/processEnd/feedback/Feedback.tsx(3 hunks)src/features/propagateTraceWhenPdf/index.ts(1 hunks)src/features/receipt/FixWrongReceiptType.tsx(1 hunks)src/features/receipt/ReceiptContainer.tsx(2 hunks)src/hooks/navigation.ts(1 hunks)src/hooks/useIsPdf.ts(1 hunks)src/hooks/useNavigatePage.ts(2 hunks)src/hooks/useWaitForQueries.ts(1 hunks)src/layout/GenericComponent.tsx(2 hunks)src/layout/RepeatingGroup/EditContainer/RepeatingGroupEditContext.tsx(1 hunks)src/layout/RepeatingGroup/Providers/RepeatingGroupFocusContext.tsx(1 hunks)src/layout/Subform/SubformWrapper.tsx(1 hunks)src/layout/Tabs/Tabs.tsx(1 hunks)src/layout/index.ts(2 hunks)src/utils/layout/useExpressionDataSources.ts(3 hunks)test/e2e/integration/stateless-app/feedback.ts(1 hunks)test/e2e/integration/stateless-app/receipt.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/core/loading/Loader.tsx
✅ Files skipped from review due to trivial changes (2)
- src/features/navigation/AppNavigation.tsx
- src/layout/Tabs/Tabs.tsx
🚧 Files skipped from review as they are similar to previous changes (30)
- src/core/routing/types.ts
- src/components/form/LinkToPotentialNode.tsx
- src/layout/RepeatingGroup/Providers/RepeatingGroupFocusContext.tsx
- src/core/routing/useIsReceiptPage.ts
- src/features/expressions/expression-functions.ts
- src/layout/RepeatingGroup/EditContainer/RepeatingGroupEditContext.tsx
- src/layout/Subform/SubformWrapper.tsx
- src/hooks/useIsPdf.ts
- src/features/navigation/utils.ts
- src/features/receipt/FixWrongReceiptType.tsx
- src/features/instance/useProcessNext.tsx
- src/hooks/useNavigatePage.ts
- src/core/routing/useIsNavigating.ts
- src/features/pdf/PDFWrapper.test.tsx
- src/layout/index.ts
- src/components/form/Form.tsx
- src/layout/GenericComponent.tsx
- src/components/presentation/Presentation.tsx
- src/utils/layout/useExpressionDataSources.ts
- src/components/presentation/Presentation.test.tsx
- test/e2e/integration/stateless-app/receipt.ts
- src/features/formData/FormDataWrite.tsx
- src/features/datamodel/utils.ts
- src/features/instance/useProcessQuery.ts
- src/hooks/navigation.ts
- src/features/instantiate/selection/InstanceSelection.tsx
- src/App.tsx
- src/hooks/useWaitForQueries.ts
- test/e2e/integration/stateless-app/feedback.ts
- src/features/datamodel/DataModelsProvider.tsx
🧰 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/propagateTraceWhenPdf/index.tssrc/features/receipt/ReceiptContainer.tsxsrc/features/processEnd/feedback/Feedback.tsxsrc/components/wrappers/ProcessWrapper.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3645
File: src/features/receipt/FixWrongReceiptType.tsx:16-34
Timestamp: 2025-09-03T14:23:33.606Z
Learning: In FixWrongReceiptType.tsx, brief ping-ponging between receipt routes during instance data loading is acceptable because the instance data is typically already fetched when this component runs. The author considers this a non-issue.
📚 Learning: 2025-09-03T14:23:33.606Z
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3645
File: src/features/receipt/FixWrongReceiptType.tsx:16-34
Timestamp: 2025-09-03T14:23:33.606Z
Learning: In FixWrongReceiptType.tsx, brief ping-ponging between receipt routes during instance data loading is acceptable because the instance data is typically already fetched when this component runs. The author considers this a non-issue.
Applied to files:
src/features/receipt/ReceiptContainer.tsxsrc/components/wrappers/ProcessWrapper.tsx
📚 Learning: 2025-08-28T12:25:27.289Z
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3645
File: src/components/wrappers/ProcessWrapper.tsx:107-110
Timestamp: 2025-08-28T12:25:27.289Z
Learning: In ProcessWrapper.tsx, when taskType === ProcessTaskType.Archived and taskId !== TaskKeys.CustomReceipt, the component shows a loader with reason 'redirect-to-receipt' but does not perform explicit navigation. This is intentional - other parts of the system handle the actual navigation closer to where user interactions occur.
Applied to files:
src/features/receipt/ReceiptContainer.tsxsrc/features/processEnd/feedback/Feedback.tsxsrc/components/wrappers/ProcessWrapper.tsx
📚 Learning: 2025-09-03T14:26:18.627Z
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3645
File: src/components/wrappers/ProcessWrapper.tsx:78-83
Timestamp: 2025-09-03T14:26:18.627Z
Learning: In ProcessWrapper.tsx, the useIsRunningProcessNext() hook intentionally uses a non-reactive pattern with queryClient.isMutating and local state instead of useIsMutating hook. This design choice is deliberate and should not be changed to a reactive pattern.
Applied to files:
src/features/processEnd/feedback/Feedback.tsxsrc/components/wrappers/ProcessWrapper.tsx
📚 Learning: 2025-08-28T12:25:27.289Z
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3645
File: src/components/wrappers/ProcessWrapper.tsx:107-110
Timestamp: 2025-08-28T12:25:27.289Z
Learning: In the app-frontend-react codebase, the architecture favors navigation happening closer to user interactions or specific process state changes, rather than using universal effects that automatically redirect users. This is part of a design philosophy to avoid "free-standing effects that throw you around" and make navigation more predictable and contextual.
Applied to files:
src/components/wrappers/ProcessWrapper.tsx
🧬 Code graph analysis (3)
src/features/receipt/ReceiptContainer.tsx (2)
src/features/receipt/FixWrongReceiptType.tsx (1)
FixWrongReceiptType(16-47)src/components/presentation/Presentation.tsx (1)
PresentationComponent(32-93)
src/features/processEnd/feedback/Feedback.tsx (3)
src/features/instance/useProcessQuery.ts (2)
useProcessQuery(23-26)useOptimisticallyUpdateProcess(99-106)src/hooks/useNavigatePage.ts (1)
useNavigateToTask(152-182)src/features/instance/InstanceContext.tsx (1)
useInstanceDataQuery(110-119)
src/components/wrappers/ProcessWrapper.tsx (4)
src/core/routing/useIsNavigating.ts (1)
useIsNavigating(3-10)src/hooks/useNavigatePage.ts (1)
useIsValidTaskId(184-202)src/features/instance/useProcessQuery.ts (2)
useGetTaskTypeById(61-89)useProcessQuery(23-26)src/hooks/useWaitForQueries.ts (1)
useWaitForQueries(11-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Install
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (6)
src/components/wrappers/ProcessWrapper.tsx (4)
6-6: Imports: type-only + routing/query hooks — LGTMClean type-only import and the added hooks match usage below.
Also applies to: 13-13, 24-24
100-106: Null-state gating to avoid flicker — LGTMDeferring render until both gates resolve is appropriate.
116-119: Disable navigation on error pages — LGTMshowNavigation={false} fits the flow for invalid/wrong task states.
Also applies to: 124-127
132-135: Wrap Confirm/Feedback in PresentationComponent — LGTMMatches the refactor to centralize receipt/state logic.
Also applies to: 140-143
src/features/processEnd/feedback/Feedback.tsx (1)
50-52: Separation of concerns looks good.Polling is isolated behind
useBackoff(callback); once navigation unmounts the page, cleanup halts polling. No action needed.src/features/propagateTraceWhenPdf/index.ts (1)
3-3: Import move looks correct — unable to verify repo-wide (rg skipped files).Repository search returned no files; cannot confirm there are no remaining imports of SearchParams. Run locally to verify:
rg -n --hidden -uu -S "\bSearchParams\b" -g '!node_modules/**' -C1
| import { useLanguage } from 'src/features/language/useLanguage'; | ||
| import { useInstanceOwnerParty } from 'src/features/party/PartiesProvider'; | ||
| import { getInstanceSender } from 'src/features/processEnd/confirm/helpers/returnConfirmSummaryObject'; | ||
| import { FixWrongReceiptType } from 'src/features/receipt/FixWrongReceiptType'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Avoid double-wrapping with FixWrongReceiptType (route-level already wraps).
Per the PR summary, App.tsx now wraps receipt routes with FixWrongReceiptType. Keeping this wrapper here duplicates the redirect effect and can cause redundant navigate calls/extra loader flashes. Prefer a single wrapping at the route level.
Apply this diff to remove the duplicate wrapper/import:
- import { FixWrongReceiptType } from 'src/features/receipt/FixWrongReceiptType';And in DefaultReceipt (see lines 78-82 change below):
- <FixWrongReceiptType>
- <PresentationComponent showNavigation={false}>
- <ReceiptContainer />
- </PresentationComponent>
- </FixWrongReceiptType>
+ <PresentationComponent>
+ <ReceiptContainer />
+ </PresentationComponent>Run to verify there isn’t another component-level wrapper remaining and that routes handle it:
🏁 Script executed:
#!/bin/bash
# Find all usages of FixWrongReceiptType to ensure it’s only applied at the route level.
rg -n -C2 --type=tsx --type=ts 'FixWrongReceiptType'
# Sanity check: find route definitions for receipt pages and confirm FixWrongReceiptType is used there.
rg -n -C3 --type=tsx 'Route|Routes|FixWrongReceiptType|CustomReceipt|ProcessEnd|receipt'Length of output: 55
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if command -v rg >/dev/null 2>&1; then
RG="rg -n --hidden -S"
else
RG="grep -RIn --exclude-dir=.git --binary-files=without-match"
fi
echo "=== 1) Occurrences of FixWrongReceiptType ==="
$RG 'FixWrongReceiptType' || true
echo
echo "=== 2) Import lines for FixWrongReceiptType ==="
$RG "import .*FixWrongReceiptType" || true
echo
echo "=== 3) Receipt / route related identifiers (Route|Routes|CustomReceipt|ProcessEnd|receipt|Receipt) ==="
$RG 'Route|Routes|CustomReceipt|ProcessEnd|receipt|Receipt' || true
echo
echo "=== 4) Show likely route/app files (App.tsx, *routes*.ts(x), *router*.tsx) (first 250 lines) ==="
find . -type f \( -iname 'App.tsx' -o -iname '*routes*.ts' -o -iname '*routes*.tsx' -o -iname '*router*.tsx' \) -print0 | xargs -0 -n1 -I{} sh -c 'echo "---- {} ----"; sed -n "1,250p" "{}"' || trueLength of output: 52910
Remove duplicated FixWrongReceiptType wrapper from DefaultReceipt (src/features/receipt/ReceiptContainer.tsx).
App.tsx already wraps receipt routes; ReceiptContainer still imports and wraps — remove the import and the inner wrapper (import at line ~19, wrapper at ~78–82) to avoid double redirects/extra loader flashes.
- import { FixWrongReceiptType } from 'src/features/receipt/FixWrongReceiptType';And in DefaultReceipt (replace):
- <FixWrongReceiptType>
- <PresentationComponent showNavigation={false}>
- <ReceiptContainer />
- </PresentationComponent>
- </FixWrongReceiptType>
+ <PresentationComponent>
+ <ReceiptContainer />
+ </PresentationComponent>🤖 Prompt for AI Agents
In src/features/receipt/ReceiptContainer.tsx around line 19 and lines ~78–82,
remove the duplicate import and inner FixWrongReceiptType wrapper: delete the
import statement for FixWrongReceiptType at the top and remove the JSX wrapper
that wraps DefaultReceipt (lines ~78–82) so DefaultReceipt is rendered directly;
ensure any now-unused import is removed from the file and update DefaultReceipt
to not include the inner wrapper.
Description
This got a bit out of hand, but the main purpose of this PR is to fix a bug that caused the Feedback task to not correctly load the next task unless it was going to the archive/receipt. I added a gateway to
ttd/stateless-appletting the test decide what happens after the feedback task, and you can choose for it to either fail (taking you back to try again) or continue (which will land you at the process end-event).This uncovered a few things, which I'm fixing here:
process/nexthad finished fetching instance data again before using old instance data inDataModelsProviderto conclude the data element was locked (but it is unlocked when going backwards again). Fixing this by not providingDataModelsProviderwhen the instance data is fetching.App.tsxhad a whole thing forCustomReceipt, but there's no real difference betweenCustomReceiptand any other data-task. Removing the duplication and adding<FixWrongReceiptType />which seemed to be the only thing differentiatingCustomReceiptfrom a normal form.PresentationComponentneeded to know if you're in a receipt or not, but passing this in as a prop had gotten very complex. Instead of passing it in as a prop, the component itself can justuseIsReceiptPage()to figure that out. Refactoring this, removing a prop.useProcessQuery()had, until now, an effect that would navigate you to the receipt page if the process has ended. This is what actually did the magic in theFeedbacktask, but it was quite unclear that this was what broke navigating to any other task type afterFeedback. Instead of just fixing things locally here, I re-engineered the whole thing. TheFeedbackcomponent now does the navigation after polling find you at another task, and the effect to navigate to the correct receipt type was moved intoFixWrongReceiptTypeinstead. This way everyone can read the process query without applying any effects.<ProcessWrapper />got in the way quite a few times. When I tried making any generic "if the process data says we're on a different task now, navigate us there" functionality, it would get in the way of the existing "you're looking at a previous task" error page that could be rendered from<ProcessWrapper />. To make sure I didn't introduce any more bugs, I added Cypress tests (forttd/stateless-app) that check:process/next, as there will be a tiny window where the URL has not caught up with the state in the process data query. Now a test will fail if we regress again.Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Bug Fixes
Performance
Tests