Improve loading and error handling in ReadyForPrint #3840
Conversation
📝 WalkthroughWalkthroughAdds local loading and error components (Spinner, LoadingEmpty, LoadingWrapper, AltinnContentLoader, FatalError, FatalErrorEmpty), migrates many Spinner imports to the local Spinner, replaces the old molecules AltinnContentLoader usage with the new variant-based loader, and refactors DOM detection from class-based to attribute-based for print gating. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Areas to focus review on:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
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 |
bjosttveit
added
kind/bug
bjosttveit
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
src/app-components/Button/Button.tsx(1 hunks)src/app-components/Table/Table.tsx(1 hunks)src/app-components/error/FatalError/FatalError.tsx(1 hunks)src/app-components/error/FatalErrorEmpty/FatalErrorEmpty.tsx(1 hunks)src/app-components/loading/AltinnContentLoader/AltinnContentIconFormData.tsx(1 hunks)src/app-components/loading/AltinnContentLoader/AltinnContentIconReceipt.tsx(1 hunks)src/app-components/loading/AltinnContentLoader/AltinnContentLoader.test.tsx(1 hunks)src/app-components/loading/AltinnContentLoader/AltinnContentLoader.tsx(1 hunks)src/app-components/loading/LoadingEmpty/LoadingEmpty.tsx(1 hunks)src/app-components/loading/Spinner/Spinner.tsx(1 hunks)src/components/AltinnSpinner.tsx(1 hunks)src/components/PDFGeneratorPreview/PDFGeneratorPreview.tsx(1 hunks)src/components/ReadyForPrint.tsx(5 hunks)src/components/altinnError.tsx(3 hunks)src/components/molecules/AltinnContentLoader.test.tsx(0 hunks)src/components/molecules/AltinnContentLoader.tsx(0 hunks)src/components/presentation/BackNavigationButton.tsx(1 hunks)src/core/loading/Loader.tsx(2 hunks)src/core/loading/LoadingContext.tsx(1 hunks)src/core/ui/RenderStart.tsx(2 hunks)src/features/devtools/components/VersionSwitcher/VersionSwitcher.tsx(1 hunks)src/features/navigation/components/Page.tsx(1 hunks)src/features/navigation/components/PageGroup.tsx(1 hunks)src/features/options/useGetOptions.ts(1 hunks)src/features/pdf/PdfFromLayout.tsx(2 hunks)src/features/process/confirm/containers/Confirm.tsx(2 hunks)src/features/receipt/ReceiptContainer.tsx(2 hunks)src/layout/GenericComponent.tsx(2 hunks)src/layout/ImageUpload/ImageCanvas/ImagePreview.tsx(1 hunks)src/layout/NavigationBar/NavigationBarComponent.tsx(1 hunks)src/layout/Option/OptionComponent.tsx(2 hunks)src/layout/SigneeList/SigneeListError.tsx(4 hunks)src/layout/SigneeList/SigneeListSummary.tsx(2 hunks)src/layout/SigningActions/PanelAwaitingCurrentUserSignature.tsx(1 hunks)src/layout/SigningActions/SigningActionsComponent.tsx(2 hunks)src/layout/SigningDocumentList/SigningDocumentListError.tsx(4 hunks)src/layout/Subform/SubformComponent.tsx(2 hunks)src/layout/Subform/Summary/SubformSummaryComponent.tsx(2 hunks)src/layout/Subform/Summary/SubformSummaryComponent2.tsx(2 hunks)src/layout/Subform/Summary/SubformSummaryTable.tsx(2 hunks)test/e2e/integration/subform-test/pdf.ts(2 hunks)
💤 Files with no reviewable changes (2)
- src/components/molecules/AltinnContentLoader.tsx
- src/components/molecules/AltinnContentLoader.test.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/app-components/loading/LoadingEmpty/LoadingEmpty.tsxsrc/app-components/error/FatalErrorEmpty/FatalErrorEmpty.tsxsrc/layout/GenericComponent.tsxsrc/app-components/loading/AltinnContentLoader/AltinnContentLoader.test.tsxsrc/app-components/error/FatalError/FatalError.tsxsrc/layout/SigningActions/SigningActionsComponent.tsxsrc/features/devtools/components/VersionSwitcher/VersionSwitcher.tsxsrc/app-components/Table/Table.tsxsrc/app-components/loading/AltinnContentLoader/AltinnContentLoader.tsxsrc/features/pdf/PdfFromLayout.tsxsrc/layout/SigneeList/SigneeListError.tsxsrc/app-components/loading/AltinnContentLoader/AltinnContentIconFormData.tsxsrc/components/PDFGeneratorPreview/PDFGeneratorPreview.tsxsrc/app-components/Button/Button.tsxsrc/layout/Subform/Summary/SubformSummaryComponent2.tsxsrc/layout/Subform/SubformComponent.tsxsrc/layout/Option/OptionComponent.tsxsrc/app-components/loading/AltinnContentLoader/AltinnContentIconReceipt.tsxsrc/features/process/confirm/containers/Confirm.tsxsrc/components/AltinnSpinner.tsxsrc/components/presentation/BackNavigationButton.tsxsrc/features/receipt/ReceiptContainer.tsxsrc/layout/SigningDocumentList/SigningDocumentListError.tsxsrc/layout/SigningActions/PanelAwaitingCurrentUserSignature.tsxsrc/layout/NavigationBar/NavigationBarComponent.tsxsrc/layout/Subform/Summary/SubformSummaryComponent.tsxsrc/features/navigation/components/Page.tsxsrc/layout/SigneeList/SigneeListSummary.tsxsrc/components/altinnError.tsxsrc/features/navigation/components/PageGroup.tsxsrc/layout/Subform/Summary/SubformSummaryTable.tsxsrc/core/loading/LoadingContext.tsxsrc/app-components/loading/Spinner/Spinner.tsxsrc/core/ui/RenderStart.tsxsrc/features/options/useGetOptions.tstest/e2e/integration/subform-test/pdf.tssrc/core/loading/Loader.tsxsrc/layout/ImageUpload/ImageCanvas/ImagePreview.tsxsrc/components/ReadyForPrint.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/app-components/loading/AltinnContentLoader/AltinnContentLoader.test.tsx
🧠 Learnings (8)
📚 Learning: 2025-08-22T13:53:28.252Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.252Z
Learning: Applies to src/layout/*/{config.ts,Component.tsx,index.tsx,config.generated.ts} : Layout components must follow the standardized structure: `config.ts`, `Component.tsx`, `index.tsx`, and include generated types in `config.generated.ts`
Applied to files:
src/layout/GenericComponent.tsx
📚 Learning: 2025-08-22T13:53:28.252Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 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/app-components/loading/AltinnContentLoader/AltinnContentLoader.test.tsxsrc/layout/Subform/Summary/SubformSummaryComponent2.tsxtest/e2e/integration/subform-test/pdf.ts
📚 Learning: 2025-08-28T12:25:27.289Z
Learnt from: olemartinorg
Repo: Altinn/app-frontend-react PR: 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/layout/SigningActions/SigningActionsComponent.tsxsrc/layout/Subform/SubformComponent.tsxsrc/components/presentation/BackNavigationButton.tsxsrc/layout/SigningActions/PanelAwaitingCurrentUserSignature.tsxsrc/layout/Subform/Summary/SubformSummaryTable.tsxsrc/core/ui/RenderStart.tsx
📚 Learning: 2025-08-28T12:00:32.967Z
Learnt from: olemartinorg
Repo: Altinn/app-frontend-react PR: 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/layout/Option/OptionComponent.tsxsrc/features/navigation/components/PageGroup.tsxsrc/components/ReadyForPrint.tsx
📚 Learning: 2025-09-03T14:23:33.606Z
Learnt from: olemartinorg
Repo: Altinn/app-frontend-react PR: 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/layout/Option/OptionComponent.tsxsrc/app-components/loading/AltinnContentLoader/AltinnContentIconReceipt.tsxsrc/features/process/confirm/containers/Confirm.tsxsrc/features/receipt/ReceiptContainer.tsx
📚 Learning: 2025-08-28T12:00:32.967Z
Learnt from: olemartinorg
Repo: Altinn/app-frontend-react PR: 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/process/confirm/containers/Confirm.tsx
📚 Learning: 2025-08-28T12:25:27.289Z
Learnt from: olemartinorg
Repo: Altinn/app-frontend-react PR: 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.tsx
📚 Learning: 2025-08-22T13:53:28.252Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.252Z
Learning: Applies to src/features/*/Provider.tsx : Place feature-specific context providers as `Provider.tsx` within each feature directory under `src/features/`
Applied to files:
src/core/loading/LoadingContext.tsx
🧬 Code graph analysis (16)
src/layout/GenericComponent.tsx (1)
src/app-components/error/FatalErrorEmpty/FatalErrorEmpty.tsx (1)
FatalErrorEmpty(6-13)
src/app-components/loading/AltinnContentLoader/AltinnContentLoader.test.tsx (1)
src/app-components/loading/AltinnContentLoader/AltinnContentLoader.tsx (2)
IAltinnContentLoaderProps(10-17)AltinnContentLoader(38-58)
src/layout/SigningActions/SigningActionsComponent.tsx (2)
src/app-components/error/FatalError/FatalError.tsx (1)
FatalError(7-16)src/layout/SigningActions/PanelSigning.tsx (1)
SigningPanel(24-61)
src/app-components/loading/AltinnContentLoader/AltinnContentLoader.tsx (3)
src/app-components/loading/AltinnContentLoader/AltinnContentIconFormData.tsx (1)
AltinnContentIconFormData(3-160)src/app-components/loading/AltinnContentLoader/AltinnContentIconReceipt.tsx (1)
AltinnContentIconReceipt(3-94)src/app-components/loading/AltinnContentLoader/AltinnContentIcon.tsx (1)
AltinnContentIcon(3-83)
src/features/pdf/PdfFromLayout.tsx (1)
src/app-components/loading/LoadingEmpty/LoadingEmpty.tsx (1)
LoadingEmpty(6-13)
src/layout/SigneeList/SigneeListError.tsx (2)
src/app-components/error/FatalError/FatalError.tsx (1)
FatalError(7-16)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/Subform/Summary/SubformSummaryComponent2.tsx (2)
src/core/loading/Loader.tsx (1)
Loader(13-35)src/core/errorHandling/DisplayError.tsx (1)
DisplayError(18-32)
src/layout/Subform/SubformComponent.tsx (2)
src/app-components/error/FatalError/FatalError.tsx (1)
FatalError(7-16)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/Option/OptionComponent.tsx (1)
src/app-components/loading/LoadingEmpty/LoadingEmpty.tsx (1)
LoadingEmpty(6-13)
src/layout/SigningDocumentList/SigningDocumentListError.tsx (2)
src/app-components/error/FatalError/FatalError.tsx (1)
FatalError(7-16)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/Subform/Summary/SubformSummaryComponent.tsx (2)
src/app-components/error/FatalError/FatalError.tsx (1)
FatalError(7-16)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/SigneeList/SigneeListSummary.tsx (1)
src/app-components/error/FatalError/FatalError.tsx (1)
FatalError(7-16)
src/components/altinnError.tsx (1)
src/app-components/error/FatalError/FatalError.tsx (1)
FatalError(7-16)
src/layout/Subform/Summary/SubformSummaryTable.tsx (2)
src/app-components/error/FatalError/FatalError.tsx (1)
FatalError(7-16)src/features/language/Lang.tsx (1)
Lang(15-23)
src/core/ui/RenderStart.tsx (1)
src/components/ReadyForPrint.tsx (2)
useHasElementsByAttribute(79-104)loadingAttribute(7-7)
test/e2e/integration/subform-test/pdf.ts (1)
src/types/shared.ts (1)
IInstance(70-89)
⏰ 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). (1)
- GitHub Check: Install
🔇 Additional comments (50)
src/features/options/useGetOptions.ts (1)
258-258: Clarify the TODO comment's intent and action item.The TODO is phrased as a question ("we just log and PDF generation will still succeed?") rather than specifying the desired behavior or action. Given the PR objective to improve error handling, clarify whether:
- PDF generation should fail if options fail to fetch?
- Additional error UI should be shown (as suggested by broader PR changes)?
- Current logging behavior is intentional but needs documentation?
Regarding the AI summary claiming "two identical TODO comments"—only one TODO appears in the provided code at line 258. Verify whether a second TODO was intended or if this is an inconsistency in the summary.
src/app-components/loading/AltinnContentLoader/AltinnContentIconFormData.tsx (1)
7-7: LGTM! Test identifier added.The
data-testidattribute enables precise targeting of this icon variant in tests without affecting runtime behavior.src/core/ui/RenderStart.tsx (1)
5-5: LGTM! Clean migration to attribute-based loading detection.The shift from class-based to attribute-based detection is consistent with the broader refactoring in
ReadyForPrint.tsx, improving the reliability of loading state detection.Also applies to: 35-35
src/components/ReadyForPrint.tsx (3)
7-8: LGTM! Attribute-based detection improves reliability.The shift from
loadingClassNametoloadingAttributeand the addition oferrorAttributeprovides more robust detection of loading and error states via data attributes rather than CSS classes.
28-29: Excellent addition of error state detection.The dual detection of both
hasLoadersandhasErrorsensures that the PDF indicator correctly hides when fatal errors occur, preventing premature PDF generation.Also applies to: 44-44
79-104: Well-refactored hook with proper MutationObserver configuration.The
useHasElementsByAttributehook correctly:
- Initializes state with a document query
- Configures MutationObserver to watch for attribute changes on the specified attribute
- Updates dependencies appropriately
src/components/presentation/BackNavigationButton.tsx (1)
8-8: LGTM! Spinner migration to local component.Part of the project-wide migration from design system Spinner to a local implementation. No behavioral changes.
src/features/devtools/components/VersionSwitcher/VersionSwitcher.tsx (1)
3-3: LGTM! Spinner import migration.Consistent with the project-wide migration to local Spinner component. No behavioral changes.
Also applies to: 8-8
src/layout/SigneeList/SigneeListSummary.tsx (1)
9-9: Excellent integration with attribute-based error detection.Wrapping the error state in
FatalErroradds thedata-fatal-errorattribute, enablingReadyForPrintto detect the error condition and prevent premature PDF generation. This aligns with the broader error handling pattern introduced in this PR.Also applies to: 49-59
src/layout/SigningActions/PanelAwaitingCurrentUserSignature.tsx (1)
4-4: LGTM! Spinner import migration.Consistent with the project-wide Spinner migration. No behavioral changes.
Also applies to: 7-7
src/layout/NavigationBar/NavigationBarComponent.tsx (1)
7-7: LGTM!The Spinner import has been successfully migrated to the local implementation, aligning with the PR's goal to centralize loading components.
src/features/navigation/components/Page.tsx (1)
6-6: LGTM!The Spinner import has been successfully migrated to the local implementation.
src/components/AltinnSpinner.tsx (1)
7-7: LGTM!The Spinner import has been successfully migrated to the local implementation.
src/app-components/loading/AltinnContentLoader/AltinnContentIconReceipt.tsx (1)
7-7: LGTM!Adding the
data-testidattribute enables targeted testing for the receipt icon variant, improving test coverage and maintainability.src/app-components/Table/Table.tsx (1)
10-10: LGTM!The Spinner import has been successfully migrated to the local implementation.
src/layout/Option/OptionComponent.tsx (1)
6-6: Improved loading state detection.Replacing the
nullreturn with<LoadingEmpty />provides a DOM-based signal (data-loadingattribute) that enables external processes to detect loading states, which aligns with the PR's objective to improve loading handling for the readyforprint feature.Also applies to: 62-64
src/layout/Subform/Summary/SubformSummaryComponent2.tsx (2)
8-9: LGTM!Added necessary imports for improved error and loading state handling in subform summaries.
96-102: Excellent error and loading state handling.The explicit handling of loading and error states prevents rendering incomplete or invalid data when subform data is being fetched or has encountered an error. This significantly improves the user experience and data integrity.
src/layout/GenericComponent.tsx (1)
7-7: Improved error state detection in production.Replacing the
nullreturn with<FatalErrorEmpty />provides a DOM-based signal (data-fatal-errorattribute) that enables external processes to detect unrecoverable errors in production, preventing PDF generation when components have fatal configuration errors. This aligns with the PR's objective to improve error handling for the readyforprint feature.Also applies to: 194-194
src/features/navigation/components/PageGroup.tsx (1)
6-6: LGTM – Clean import path update.The Spinner import has been successfully migrated to the local component path, consistent with the broader refactoring effort to centralize loading components.
src/components/PDFGeneratorPreview/PDFGeneratorPreview.tsx (1)
3-7: LGTM – Clean import migration.The Spinner import has been cleanly migrated from the design system package to the local component implementation, maintaining all existing functionality.
src/layout/ImageUpload/ImageCanvas/ImagePreview.tsx (1)
3-3: LGTM – Import path updated correctly.The Spinner component import has been successfully updated to use the local implementation.
src/layout/Subform/Summary/SubformSummaryComponent.tsx (2)
4-5: LGTM – Imports updated for centralized error/loading components.The imports have been correctly updated to use the local FatalError and Spinner implementations.
61-65: LGTM – Error handling improved with FatalError wrapper.Wrapping the error message in FatalError adds the
data-fatal-errorattribute, which signals to the PDF generation system that an unrecoverable error has occurred and prevents incomplete PDF generation.src/components/altinnError.tsx (2)
5-5: LGTM – FatalError import added.
33-79: LGTM – Root element correctly wrapped with FatalError.The AltinnError component has been properly updated to use FatalError as the root wrapper, which adds the
data-fatal-errorattribute for PDF generation prevention. All props are correctly preserved through spreading.src/app-components/error/FatalErrorEmpty/FatalErrorEmpty.tsx (1)
1-13: LGTM – Well-designed utility component for error signaling.This component provides a clean, focused solution for signaling unrecoverable errors to the PDF generation system without rendering visible content. The inline style is appropriate for this specific use case, and the documentation clearly explains the purpose of the
data-fatal-errorattribute.src/app-components/loading/LoadingEmpty/LoadingEmpty.tsx (1)
1-13: LGTM – Well-designed utility component for loading state signaling.This component provides a clean solution for signaling pending/loading states to the PDF generation system without rendering visible UI. The design mirrors FatalErrorEmpty and the documentation clearly explains the purpose of the
data-loadingattribute.src/layout/Subform/SubformComponent.tsx (2)
9-11: LGTM – Imports updated correctly.The imports have been properly updated to use the local FatalError and Spinner implementations, consistent with the broader refactoring effort.
242-244: LGTM – Error handling improved with FatalError wrapper.The error message is now properly wrapped in FatalError, adding the
data-fatal-errorattribute to signal unrecoverable errors and prevent incomplete PDF generation. This change is consistent with the error handling pattern applied throughout the codebase.src/app-components/Button/Button.tsx (1)
7-7: LGTM: Spinner import centralized.The import path change centralizes the Spinner component to a local implementation while preserving existing functionality.
src/features/pdf/PdfFromLayout.tsx (1)
8-8: LGTM: LoadingEmpty signals loading state.The change from BlockPrint to LoadingEmpty aligns with the PR's goal of using attribute-based detection (
data-loading) for loading states.Also applies to: 65-65
src/core/loading/LoadingContext.tsx (1)
16-18: LGTM: Cleaner Provider implementation.Removing the UI rendering side-effect from the context provider improves separation of concerns.
src/layout/SigningActions/SigningActionsComponent.tsx (2)
4-5: LGTM: Imports updated to centralized components.The import changes align with the PR's refactoring to centralize loading and error components.
61-69: LGTM: FatalError wrapper appropriately signals unrecoverable error.The FatalError wrapper correctly signals that PDF generation should not proceed when the signee list API fails.
src/app-components/error/FatalError/FatalError.tsx (1)
1-16: LGTM: Well-designed error wrapper component.The FatalError component provides a clean abstraction for signaling unrecoverable errors. The JSDoc clearly explains its purpose in preventing PDF generation.
src/layout/SigningDocumentList/SigningDocumentListError.tsx (1)
21-32: LGTM: Consistent FatalError wrapping for all error branches.All error states are appropriately wrapped with FatalError to signal that PDF generation should not proceed.
Also applies to: 42-46, 50-54
src/layout/SigneeList/SigneeListError.tsx (1)
26-37: LGTM: Consistent FatalError wrapping for all error branches.All error states are appropriately wrapped with FatalError, maintaining consistency with other error components in the codebase.
Also applies to: 46-50, 54-58
src/layout/Subform/Summary/SubformSummaryTable.tsx (2)
4-9: LGTM! Import consolidation aligns with readyforprint improvements.The migration to local wrapper components (Spinner, FatalError) is consistent with the PR's goal of adding data attributes for PDF generation detection.
69-71: LGTM! Error handling wrapper properly applied.The FatalError component correctly wraps the error message, adding the
data-fatal-errorattribute for PDF generation detection.src/app-components/loading/Spinner/Spinner.tsx (1)
1-15: LGTM! Clean wrapper with proper type inference.The component correctly wraps the design system spinner, adds the
data-loadingattribute for PDF generation detection, and uses type inference to avoid duplication.src/features/receipt/ReceiptContainer.tsx (2)
6-6: LGTM! Import path updated to centralized location.
164-169: LGTM! Migration to prop-based variant API.The change from child component pattern to
variant='receipt'prop aligns with the new AltinnContentLoader API, simplifying the usage while maintaining the same visual behavior.src/features/process/confirm/containers/Confirm.tsx (2)
3-3: LGTM! Import path updated to centralized location.
23-28: LGTM! Consistent migration to prop-based variant API.The change to
variant='receipt'matches the pattern used throughout the PR, maintaining consistency across the codebase.src/core/loading/Loader.tsx (2)
3-3: LGTM! Import path updated to centralized location.
38-44: LGTM! Migration to prop-based variant API.The use of
variant='form'correctly replaces the previous child component pattern, maintaining the form-specific loading icon while simplifying the API.src/app-components/loading/AltinnContentLoader/AltinnContentLoader.tsx (3)
8-17: LGTM! Well-defined type interfaces.The type definitions are clear and properly scoped. The LoaderVariant type and IAltinnContentLoaderProps interface provide good type safety without using
any.
23-33: LGTM! Robust variant handling with proper defaults.The switch statement correctly maps each variant to its corresponding icon component and includes a sensible default case.
38-58: LGTM! Clean component implementation with debugging support.The component properly:
- Adds
data-loadingfor PDF generation detection- Provides debugging attributes (
data-reason,data-details)- Sets sensible defaults for width and height
- Forwards all props correctly
src/app-components/loading/AltinnContentLoader/AltinnContentLoader.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/e2e/integration/subform-test/pdf.ts (2)
119-119: Throw an Error object instead of a string.Best practice is to throw an
Errorobject rather than a string literal for better stack traces and consistency.Apply this diff:
- throw 'Could not find data element to block'; + throw new Error('Could not find data element to block');
128-128: Remove commented-out code.The commented-out
req.destroy()line should be removed for cleaner test code.Apply this diff:
if (req.url.includes(data.dataElementIdToBlock)) { - // req.destroy(); req.reply({ statusCode: 404, body: 'Not Found' }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app-components/loading/AltinnContentLoader/AltinnContentLoader.test.tsx(1 hunks)test/e2e/integration/subform-test/pdf.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app-components/loading/AltinnContentLoader/AltinnContentLoader.test.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:
test/e2e/integration/subform-test/pdf.ts
🧠 Learnings (3)
📚 Learning: 2025-09-03T14:23:33.606Z
Learnt from: olemartinorg
Repo: Altinn/app-frontend-react PR: 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:
test/e2e/integration/subform-test/pdf.ts
📚 Learning: 2025-09-05T12:57:07.200Z
Learnt from: olemartinorg
Repo: Altinn/app-frontend-react PR: 3673
File: src/utils/layout/plugins/claimRepeatingChildren.ts:8-27
Timestamp: 2025-09-05T12:57:07.200Z
Learning: In the app-frontend-react codebase, the maintainer prefers throwing errors for invalid multiPage ID formats in claimRepeatingChildren rather than logging and continuing processing. This indicates a preference for strict validation that halts on malformed input.
Applied to files:
test/e2e/integration/subform-test/pdf.ts
📚 Learning: 2025-08-22T13:53:28.252Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 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:
test/e2e/integration/subform-test/pdf.ts
🧬 Code graph analysis (1)
test/e2e/integration/subform-test/pdf.ts (2)
src/types/shared.ts (1)
IInstance(70-89)src/utils/instanceIdRegExp.ts (1)
getInstanceIdRegExp(3-18)
⏰ 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: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (5)
test/e2e/integration/subform-test/pdf.ts (5)
3-6: LGTM! Imports are appropriate for the new test.The added imports (
getInstanceIdRegExpandIInstance) are properly used in the new error handling test case.
105-110: Well-structured test setup.The test name clearly communicates intent, and the waiting strategy appropriately ensures the page and data are ready before intercepting requests.
133-141: Solid URL manipulation and navigation logic.The code correctly extracts the instance ID using the provided utility function and constructs the PDF URL appropriately. The reload strategy effectively triggers the error scenario.
143-149: Error state assertions are correct.The test appropriately verifies both the presence of the error heading and the absence of the
#readyForPrintelement when a subform fails to load.
151-156: Excellent recovery validation.The test effectively confirms recovery by reloading (after intercepts are exhausted) and validating that the PDF view renders correctly with
#readyForPrintvisible and no error message present. This provides comprehensive coverage of the error handling and recovery flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/features/language/LanguageProvider.tsx (1)
51-51: Clarify whether this TODO should be resolved in this PR.Since this PR specifically addresses error handling improvements in ReadyForPrint (labeled
kind/bug), it's unclear whether this TODO represents:
- A known limitation being flagged for future work, or
- An open question that should be resolved in this PR
Currently, language fetch errors are only logged (line 54) and don't prevent PDF generation. If the app languages fail to load, the PDF will render using fallback language logic, which may not match user expectations.
Please clarify:
- Should this decision be made and implemented in this PR?
- If deferring, should a tracking issue be created?
If you'd like to implement error handling now, I can help propose a solution that either:
- Blocks PDF generation on language fetch errors, or
- Shows a warning/degraded state indicator in the PDF when languages couldn't be loaded
src/app-components/loading/LoadingWrapper/LoadingWrapper.tsx (1)
1-1: Remove unused React import.With React 19's JSX transform, the
Reactimport is no longer necessary when you're not using React APIs directly. Consider removing it for cleaner code.-import React from 'react'; import type { HTMLAttributes, PropsWithChildren } from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app-components/loading/LoadingWrapper/LoadingWrapper.tsx(1 hunks)src/features/language/LanguageProvider.tsx(1 hunks)src/layout/SigneeList/SigneeListSummary.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/layout/SigneeList/SigneeListSummary.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/app-components/loading/LoadingWrapper/LoadingWrapper.tsxsrc/features/language/LanguageProvider.tsx
⏰ 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: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
src/app-components/loading/LoadingWrapper/LoadingWrapper.tsx (1)
7-15: Verify thatdata-loadingcan be overridden by props.The
data-loadingattribute is placed before{...props}, which allows callers to override it by passingdata-loading={false}ordata-loading={undefined}in props. Given that this component's purpose is to signal a loading state for PDF print gating, allowing the attribute to be overridden seems unintentional and could lead to bugs.If
data-loadingshould always be present when using this wrapper, consider moving the spread before the attribute to prevent overrides.Apply this diff if the attribute should not be overridable:
export function LoadingWrapper({ children, ...props }: PropsWithChildren<HTMLAttributes<HTMLDivElement>>) { return ( <div - data-loading {...props} + data-loading > {children} </div> ); }
|
|
I tested blocking application languages, and it seems monstly fine honestly, the PDF generator should always specify the language explicitly through a query param, and the logic currently will just use that even if the allowed languages have not loaded. So not really a problem right now. However, if I block an options fetch it looks like it retries a bunch of times but eventually just shows the option value (not label). This does not block print from occuring though, but probably blocks while the query is fetching through the 
|
|
Most other important queries failing results in unknown error, and now, these queries will also cause failure for readyForPrint if they occur within a subform context. The mehcanism previously was that when unknown error is rendered, the readyForPrint element is not rendered at all, but this broke down when we added subforms as an independent subtree next to (not above) readyForPrint, which could fail independently. Hence, why checking for errors anywhere in the DOM is now necessary |
* improve loading and error handling in readyforprint * fix test description * strict equality * use fatal error in dev as well * use loading attribute in signeelistsummary * add todo * clarify comments * remove comment
|
✅ Automatic backport successful! A backport PR has been automatically created for the A new release branch The cherry-pick was clean with no conflicts. Please review the backport PR when it appears. |
* improve loading and error handling in readyforprint * fix test description * strict equality * use fatal error in dev as well * use loading attribute in signeelistsummary * add todo * clarify comments * remove comment
2 participants
Description
Remaining potential issues identified (there may be more I haven't considered), added TODOs:
app-frontend-react/src/features/language/LanguageProvider.tsx
Line 51 in 45fb282
app-frontend-react/src/features/options/useGetOptions.ts
Line 258 in 45fb282
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Refactor
Tests