Default layout and PDF support in service tasks #3731
Conversation
|
/publish |
📝 WalkthroughWalkthroughThis pull request implements a default layout and PDF generation system for service tasks. It introduces a new ServiceTask component, refactors PDF generation to support single-layout and multi-task modes, reorganizes file structure from processEnd to process, adds task-type-based routing, and includes comprehensive E2E testing and multi-language support. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This change involves substantial refactoring across multiple concerns: comprehensive PDF generation logic overhaul (PdfFromLayout.tsx), critical type system changes affecting public API surface (shared.ts refactoring of ITask/IProcess), dynamic component routing (PdfWrapper.tsx), new feature implementation (ServiceTask), file reorganization, and extensive test coverage. While many homogeneous import path fixes are present, the heterogeneous nature of logic changes, type definitions, and feature complexity demands separate reasoning for multiple areas. The interconnected nature of routing, task detection, PDF generation, and UI rendering increases interdependency concerns. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
PR release:
|
olemartinorg
added
kind/product-feature
olemartinorg
force-pushed
the
feat/service-task2
branch
from
5af29f0 to
b80bcc4
Compare
olemartinorg
force-pushed
the
feat/service-task2
branch
from
b80bcc4 to
54837ba
Compare
|
/publish |
PR release:
|
|
/publish |
PR release:
|
olemartinorg
force-pushed
the
feat/service-task2
branch
from
af49a2f to
9c0e510
Compare
…nd it seems the reason is because <RenderStart />, which actually sets the text resources in language data sources, wasn't being rendered.
…his folder name made no sense. They are both distinct process steps, however.
…we can return from the PDF
… URL builder function
olemartinorg
force-pushed
the
feat/service-task2
branch
from
9c0e510 to
38f3d77
Compare
|
/publish |
PR release:
|
|
olemartinorg
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
🧹 Nitpick comments (10)
src/features/pdf/PDFView.module.css (2)
32-35: Hide screen placeholder; rely on print-only visibilityZero-sized block works, but it’s cleaner to hide this element on screen and only show it in print. Either combine with
.onlyInPrintin markup or add a screen rule.Apply within this range:
.pageBreak { - height: 0; - width: 0; }And outside this range, add to the screen media block:
@media screen { .pageBreak { display: none; } }Alternatively, use both classes in JSX: className={cn(styles.onlyInPrint, styles.pageBreak)}. As per coding guidelines.
42-44: Add modern break-before with legacy fallback; verify placement to avoid blank first pageInclude
break-before: pagealongsidepage-break-beforefor spec-compliant behavior across engines. Ensure this element is not the very first printable child to avoid an initial blank page.Apply within this range:
.pageBreak { + break-before: page; /* modern */ page-break-before: always; /* legacy */ }Please verify usages that
.pageBreakisn’t inserted as the first node under the printable container.src/features/receipt/ReceiptContainer.tsx (3)
68-71: Defensive: avoid out-of-bounds when deriving ref number from GUID.Accessing index 4 assumes a UUID with five hyphen-separated parts. Use the last segment with a fallback.
Apply:
- obj[langTools.langAsString('receipt.ref_num')] = { - value: instanceGuid.split('-')[4], + obj[langTools.langAsString('receipt.ref_num')] = { + value: (() => { + const parts = instanceGuid.split('-'); + return parts[parts.length - 1] || instanceGuid; + })(),…and keep the rest unchanged.
130-143: Minor: guard memo oninstanceOwnerPartyto avoid calling helper withundefined.You later require
instanceOwnerPartybefore rendering, so also include it in the precondition here.Apply:
- const instanceMetaObject = useMemo(() => { - if (instanceOrg && instanceOwner && instanceGuid && lastChangedDateTime) { + const instanceMetaObject = useMemo(() => { + if (instanceOrg && instanceOwner && instanceOwnerParty && instanceGuid && lastChangedDateTime) { const sender = getInstanceSender(instanceOwnerParty ?? undefined); return getSummaryDataObject({ langTools, sender, instanceGuid, lastChangedDateTime, receiver, }); }Dependencies already include
instanceOwnerParty.
161-169: Improve loader reason to avoid '-undefined' and aid observability.When
requirementMissingis undefined butpdfDisplayAttachmentsis missing, reason becomesreceipt-missing-undefined. Make the reason explicit.Apply:
- const requirementMissing = getMissingRequirement(); + const requirementMissing = getMissingRequirement(); + const missingReason = requirementMissing ?? (!pdfDisplayAttachments ? 'pdfDisplayAttachments' : 'unknown'); ... - <AltinnContentLoader + <AltinnContentLoader width={705} height={561} - reason={`receipt-missing-${requirementMissing}`} + reason={`receipt-missing-${missingReason}`} >Also applies to: 163-173
src/features/pdf/PdfWrapper.tsx (1)
51-54: Guard devtools PDF preview when tasks param is missingIf previewPDF is enabled on a Service task without
?tasks=...,<PdfForServiceTask />throws. Consider a fallback (e.g., render<PdfFromLayout />or a clear error) to avoid unhandled errors in devtools preview.src/features/pdf/PdfFromLayout.tsx (4)
53-58: Throwing in render path: consider graceful handlingThrowing inside render will rely on an ErrorBoundary. Consider logging and ignoring the param in auto‑mode to avoid white‑screening:
- Log once and proceed with auto‑layout PDF.
- Or render a small message block in the PDF.
99-127: Multi‑task generation: de‑dup and validate taskIds; skip unknown tasksIf the query includes duplicates or taskIds not present in any layout set, users may see repeated or empty sections. De‑dup is handled if you adopt getTaskIdsFromParams, but unknown taskIds should be skipped with a log.
Apply this diff:
function AutoGeneratePdfFromTasks({ taskIds }: { taskIds: string[] }) { + // Optional: Skip unknown taskIds to avoid empty/incorrect sections + // If you prefer, move this into TaskSummaryWrapper or a small hook that checks layoutSets. + // window.logInfoOnce is assumed; use your logging util. + // const layoutSets = useLayoutSets(); + // const validTaskIds = taskIds.filter((id) => layoutSets.some((set) => set.tasks?.includes(id))); + // const toRender = validTaskIds.length ? validTaskIds : taskIds; return ( <DummyPresentation> <PdfWrapping> <div className={classes.instanceInfo}> <InstanceInformation elements={{ dateSent: true, sender: true, receiver: true, referenceNumber: true, }} /> </div> - {taskIds.map((taskId, idx) => ( + {taskIds.map((taskId, idx) => ( <TaskSummaryWrapper key={taskId} taskId={taskId} > {idx > 0 && <div className={classes.pageBreak} />} {/* Settings intentionally omitted, as this is new functionality and PDF settings are deprecated at this point. */} <AllPages pdfSettings={undefined} /> <AllSubformSummaryComponent2 /> </TaskSummaryWrapper> ))} </PdfWrapping> </DummyPresentation> ); }If you want me to wire the validation using useLayoutSets(), I can provide the exact code.
193-209: Guard against malformed pdfSettings.excludedPagesIf the backend returns a partial shape, calling includes on undefined will crash. Default to an empty array.
Apply this diff:
function AllPages({ pdfSettings }: { pdfSettings: IPdfFormat | undefined }) { - const order = usePageOrder(); + const order = usePageOrder(); + const excludedPages = pdfSettings?.excludedPages ?? []; return ( <> - {order - .filter((pageKey) => !pdfSettings?.excludedPages.includes(pageKey)) + {order + .filter((pageKey) => !excludedPages.includes(pageKey)) .map((pageKey) => ( <PdfForPage key={pageKey} pageKey={pageKey} pdfSettings={pdfSettings} /> ))} </> ); }
211-224: Guard against malformed pdfSettings.excludedComponents and stabilize depsSame robustness concern here; also simplifies the memo deps.
Apply this diff:
function PdfForPage({ pageKey, pdfSettings }: { pageKey: string; pdfSettings: IPdfFormat | undefined }) { const lookups = useLayoutLookups(); - const children = useMemo(() => { + const excludedComponents = pdfSettings?.excludedComponents ?? []; + const children = useMemo(() => { const topLevel = lookups.topLevelComponents[pageKey] ?? []; return topLevel.filter((baseId) => { const component = lookups.getComponent(baseId); const def = getComponentDef(component.type); return ( component.type !== 'Subform' && - !pdfSettings?.excludedComponents.includes(baseId) && + !excludedComponents.includes(baseId) && def.shouldRenderInAutomaticPDF(component as never) ); }); - }, [lookups, pageKey, pdfSettings?.excludedComponents]); + }, [lookups, pageKey, excludedComponents]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
src/App.tsx(2 hunks)src/components/presentation/Presentation.tsx(1 hunks)src/components/wrappers/ProcessWrapper.tsx(2 hunks)src/core/routing/types.ts(1 hunks)src/features/instance/useProcessQuery.ts(1 hunks)src/features/pdf/PDFView.module.css(1 hunks)src/features/pdf/PDFWrapper.test.tsx(2 hunks)src/features/pdf/PdfFromLayout.tsx(5 hunks)src/features/pdf/PdfWrapper.tsx(3 hunks)src/features/process/confirm/containers/Confirm.test.tsx(1 hunks)src/features/process/confirm/containers/Confirm.tsx(1 hunks)src/features/process/confirm/containers/ConfirmPage.test.tsx(1 hunks)src/features/process/confirm/containers/ConfirmPage.tsx(1 hunks)src/features/process/confirm/helpers/returnConfirmSummaryObject.test.ts(1 hunks)src/features/process/service/ServiceTask.module.css(1 hunks)src/features/process/service/ServiceTask.tsx(1 hunks)src/features/receipt/ReceiptContainer.tsx(1 hunks)src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)src/layout/FileUpload/AttachmentSummaryComponent2.tsx(1 hunks)src/layout/FileUpload/FileUploadTable/AttachmentFileName.tsx(1 hunks)src/layout/FileUpload/FileUploadTable/FileTable.tsx(1 hunks)src/layout/FileUpload/FileUploadTable/FileTableRow.tsx(1 hunks)src/layout/Grid/GridSummary.tsx(1 hunks)src/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx(1 hunks)src/layout/Subform/SubformWrapper.tsx(2 hunks)src/layout/Subform/Summary/SubformSummaryTable.tsx(1 hunks)src/layout/Summary2/CommonSummaryComponents/EditButton.tsx(1 hunks)src/types/index.ts(1 hunks)src/types/shared.ts(2 hunks)test/README.md(1 hunks)test/e2e/integration/service-task/service-task.ts(1 hunks)test/e2e/pageobjects/app-frontend.ts(1 hunks)test/e2e/support/custom.ts(2 hunks)test/e2e/support/global.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/core/routing/types.tssrc/types/index.tssrc/language/texts/nb.tssrc/layout/FileUpload/FileUploadTable/AttachmentFileName.tsxsrc/features/instance/useProcessQuery.tssrc/language/texts/en.tssrc/layout/Subform/Summary/SubformSummaryTable.tsxsrc/layout/FileUpload/AttachmentSummaryComponent2.tsxsrc/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsxsrc/components/presentation/Presentation.tsxtest/e2e/support/global.tssrc/layout/Summary2/CommonSummaryComponents/EditButton.tsxsrc/language/texts/nn.tssrc/features/process/confirm/containers/ConfirmPage.test.tsxtest/e2e/integration/service-task/service-task.tssrc/features/receipt/ReceiptContainer.tsxtest/e2e/support/custom.tssrc/features/process/service/ServiceTask.tsxsrc/components/wrappers/ProcessWrapper.tsxsrc/layout/FileUpload/FileUploadTable/FileTableRow.tsxsrc/features/pdf/PDFWrapper.test.tsxsrc/layout/FileUpload/FileUploadTable/FileTable.tsxsrc/types/shared.tssrc/features/process/confirm/containers/Confirm.tsxtest/e2e/pageobjects/app-frontend.tssrc/features/process/confirm/helpers/returnConfirmSummaryObject.test.tssrc/features/pdf/PdfWrapper.tsxsrc/layout/Subform/SubformWrapper.tsxsrc/features/pdf/PdfFromLayout.tsxsrc/features/process/confirm/containers/Confirm.test.tsxsrc/features/process/confirm/containers/ConfirmPage.tsxsrc/App.tsxsrc/layout/Grid/GridSummary.tsx
**/*.module.css
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and follow existing patterns in
*.module.cssfiles
Files:
src/features/pdf/PDFView.module.csssrc/features/process/service/ServiceTask.module.css
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/features/process/confirm/containers/ConfirmPage.test.tsxsrc/features/pdf/PDFWrapper.test.tsxsrc/features/process/confirm/helpers/returnConfirmSummaryObject.test.tssrc/features/process/confirm/containers/Confirm.test.tsx
🧬 Code graph analysis (9)
src/components/presentation/Presentation.tsx (1)
src/core/ui/RenderStart.tsx (1)
RenderStart(22-31)
test/e2e/integration/service-task/service-task.ts (1)
test/e2e/pageobjects/app-frontend.ts (1)
AppFrontend(3-376)
src/features/process/service/ServiceTask.tsx (6)
src/features/language/useLanguage.ts (1)
useLanguage(90-93)src/utils/getPageTitle.ts (1)
getPageTitle(1-10)src/features/language/Lang.tsx (1)
Lang(15-23)src/components/ReadyForPrint.tsx (1)
ReadyForPrint(21-52)src/features/instance/useProcessQuery.ts (1)
useIsAuthorized(28-35)src/features/instance/useProcessNext.tsx (1)
useProcessNext(38-136)
src/components/wrappers/ProcessWrapper.tsx (3)
src/features/pdf/PdfWrapper.tsx (1)
PdfWrapper(20-58)src/components/presentation/Presentation.tsx (1)
PresentationComponent(32-93)src/features/process/service/ServiceTask.tsx (1)
ServiceTask(15-55)
src/features/pdf/PDFWrapper.test.tsx (1)
src/features/pdf/PdfWrapper.tsx (1)
PdfWrapper(20-58)
src/features/pdf/PdfWrapper.tsx (5)
src/features/devtools/data/DevToolsStore.ts (1)
useDevToolsStore(57-57)src/hooks/useIsPdf.ts (1)
useIsPdf(7-9)src/hooks/navigation.ts (1)
useNavigationParam(51-55)src/features/instance/useProcessQuery.ts (1)
useGetTaskTypeById(61-93)src/features/pdf/PdfFromLayout.tsx (2)
PdfForServiceTask(87-97)PdfFromLayout(36-47)
src/layout/Subform/SubformWrapper.tsx (3)
src/features/pdf/PdfWrapper.tsx (1)
PdfWrapper(20-58)src/components/presentation/Presentation.tsx (1)
PresentationComponent(32-93)src/components/form/Form.tsx (1)
Form(39-43)
src/features/pdf/PdfFromLayout.tsx (8)
src/features/form/layoutSettings/LayoutSettingsContext.tsx (1)
usePdfLayoutName(113-113)src/features/pdf/usePdfFormatQuery.ts (1)
usePdfFormatQuery(34-46)src/components/ReadyForPrint.tsx (1)
BlockPrint(105-112)src/components/presentation/Presentation.tsx (1)
DummyPresentation(123-129)src/layout/Subform/Summary/SubformSummaryComponent2.tsx (1)
AllSubformSummaryComponent2(146-163)src/layout/Summary2/SummaryComponent2/TaskSummaryWrapper.tsx (1)
TaskSummaryWrapper(14-28)src/features/pdf/types.ts (1)
IPdfFormat(1-4)src/hooks/useNavigatePage.ts (1)
usePageOrder(65-69)
src/App.tsx (1)
src/features/pdf/PdfWrapper.tsx (1)
PdfWrapper(20-58)
🪛 LanguageTool
test/README.md
[grammar] ~41-~41: There might be a mistake here.
Context: ...y.git) - ttd/expression-validation-test - [ttd/frontend-test](https://dev.altinn.st...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...on-validation-test) - ttd/frontend-test - [ttd/multiple-datamodels-test](https://de...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...nd-test) - ttd/multiple-datamodels-test - [ttd/navigation-test-subform](https://dev...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...els-test) - ttd/navigation-test-subform - [ttd/payment-test](https://dev.altinn.stu...
(QB_NEW_EN)
[grammar] ~45-~45: There might be a mistake here.
Context: ...gation-test-subform) - ttd/payment-test - [ttd/service-task](https://altinn.studio/...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ...os/ttd/payment-test) - ttd/service-task - [ttd/signering-brukerstyrt](https://altin...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...rvice-task) - ttd/signering-brukerstyrt - [ttd/signing-test](https://dev.altinn.stu...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...gnering-brukerstyrt) - ttd/signing-test - [ttd/stateless-app](https://dev.altinn.st...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...s/ttd/signing-test) - ttd/stateless-app - [ttd/subform-test](https://dev.altinn.stu...
(QB_NEW_EN)
🔇 Additional comments (41)
test/e2e/support/global.ts (1)
23-23: Approve constrained string union for authenticationLevel
All existing usages pass valid literals ('0' | '1' | '2'); no further changes required.src/features/receipt/ReceiptContainer.tsx (1)
18-18: Import path update verified and safe.All checks passed:
- ✅ No remaining imports from old
processEnd/confirm/...path- ✅ New import path
src/features/process/confirm/helpers/returnConfirmSummaryObjectexists and is used consistently across the codebase (ReceiptContainer, ConfirmPage, tests)- ✅
getInstanceSendersignature unchanged:export function getInstanceSender(instanceOwnerParty?: IParty): stringThe refactoring is complete and introduces no broken references.
src/components/presentation/Presentation.tsx (1)
124-128: LGTM!The addition of
RenderStartwrapper inDummyPresentationensures consistent initialization of language data sources, navigation effects, and dev tools across both the main presentation flow and the dummy/loader path.src/language/texts/nb.ts (1)
509-514: LGTM!The new service task translation keys provide complete Norwegian Bokmål text for the service task error UI, including title, body text with placeholders, help text, and button labels.
test/e2e/support/custom.ts (2)
656-656: Capture hash for restoration.Good addition to capture the hash before PDF generation, enabling restoration of the exact navigation state after PDF rendering completes.
737-741: Efficient hash-based restoration instead of full page visit.The change from visiting a modified URL to directly setting
window.location.hashis more efficient and avoids unnecessary page reloads. This hash-based restoration preserves the navigation state while staying within the React app context.test/README.md (1)
41-50: LGTM!The documentation update adds new test app repositories (service-task, subformTest, signeringBrukerstyrt) and reorganizes the list for better clarity. The static analysis warnings about grammar are false positives for Markdown link syntax.
src/language/texts/nn.ts (1)
506-511: LGTM!Norwegian Nynorsk translations added for service task UI, maintaining consistency with the Norwegian Bokmål (nb.ts) and English (en.ts) translation files.
test/e2e/pageobjects/app-frontend.ts (1)
8-39: LGTM!The apps mapping now includes the new
serviceTaskentry and reorganizes existing entries with updated repository links. This enables E2E testing of the new service task functionality.test/e2e/integration/service-task/service-task.ts (3)
11-36: Comprehensive test coverage for service task with custom layout.The test properly validates:
- Custom error layout rendering
- Notification handling
- PDF generation with custom layout
- Retry flow to success
The use of
{ retries: 0 }is appropriate for PDF tests to catch bugs rather than hide flaky behavior.
38-81: Thorough validation of default service task layout.This test effectively covers the fallback scenario when no custom layout-set exists:
- Layout-set interception to simulate missing layout
- Default error UI rendering
- Multi-task PDF generation with query parameters
- Page break verification
The PDF URL construction using
SearchParamsis clean and maintainable.
84-143: Well-structured helper functions.The three helper functions provide good separation of concerns:
startAppAndFillToFailure: Handles form filling and navigation to failure stateassertAndDismissNotification: Validates notification display and dismissalgoBackAndAchieveSuccess: Covers the retry-to-success pathThis structure promotes test readability and maintainability.
src/types/index.ts (1)
27-27: LGTM!The addition of
ProcessTaskType.Serviceenables the new service task functionality. This enum value is properly integrated with the existing process task type system and is used throughout the PR for routing and component rendering decisions.src/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx (1)
12-12: LGTM! Import path updated to match renamed module.The import path correction from
PDFWrappertoPdfWrapperaligns with the project-wide renaming effort. No behavior changes.src/layout/Subform/SubformWrapper.tsx (1)
10-10: LGTM! Component rename applied consistently.The import and JSX usage correctly reflect the
PDFWrapper→PdfWrapperrename. Behavior remains unchanged.Also applies to: 25-29
src/layout/Subform/Summary/SubformSummaryTable.tsx (1)
14-14: LGTM! Import path updated to match renamed module.The import path correction aligns with the
PdfWrapperrename across the codebase.src/layout/FileUpload/FileUploadTable/AttachmentFileName.tsx (1)
9-9: LGTM! Import path updated to match renamed module.The import path correction aligns with the project-wide
PdfWrapperrename.src/core/routing/types.ts (1)
7-7: LGTM! New search parameter supports task-based PDF generation.The
PdfForTask = 'task'enum member enables the backend-configurable PDF generation feature for multiple tasks via query parameters (e.g.,?pdf=1&task=Task_1&task=Task_2), as described in the PR objectives.src/features/process/confirm/containers/Confirm.tsx (1)
9-9: LGTM! Import path updated for module reorganization.The import path change from
processEnd/confirmtoprocess/confirmreflects a module consolidation. The imported entity remains the same.src/features/process/confirm/containers/ConfirmPage.test.tsx (1)
11-11: LGTM! Test import aligned with module reorganization.The test import correctly references the new
process/confirmmodule path, maintaining consistency with the production code.src/layout/Grid/GridSummary.tsx (1)
12-12: LGTM! Import path updated to match renamed module.The import path correction aligns with the
PdfWrapperrename across the codebase.src/layout/Summary2/CommonSummaryComponents/EditButton.tsx (1)
10-10: LGTM! Import path casing standardized.The import path has been correctly updated from
PDFWrappertoPdfWrapper, aligning with the project-wide naming standardization for PDF-related components.src/layout/FileUpload/FileUploadTable/FileTable.tsx (1)
5-5: LGTM! Import path casing standardized.The import path has been correctly updated from
PDFWrappertoPdfWrapper, consistent with the project-wide refactoring.src/features/process/confirm/helpers/returnConfirmSummaryObject.test.ts (1)
2-2: LGTM! Module path reorganization.The import path has been correctly updated from
processEnd/confirmtoprocess/confirm, aligning with the broader module reorganization for process-related features.src/features/process/confirm/containers/Confirm.test.tsx (1)
7-7: LGTM! Module path reorganization.The import path has been correctly updated from
processEnd/confirmtoprocess/confirm, consistent with the broader module reorganization.src/features/process/confirm/containers/ConfirmPage.tsx (1)
11-11: LGTM! Module path reorganization.The import path has been correctly updated from
processEnd/confirmtoprocess/confirm, aligning with the module reorganization for process-related features.src/features/process/service/ServiceTask.module.css (1)
1-5: LGTM! CSS follows project conventions.The
.buttonsclass provides clean flexbox layout for the ServiceTask action buttons, using:
- CSS custom property
var(--button-margin-top)for consistent spacing- Modern
gapproperty for button spacing- CSS Modules for scoped styling
This aligns with the coding guidelines for component styling.
src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (1)
11-11: LGTM! Import path casing standardized.The import path has been correctly updated from
PDFWrappertoPdfWrapper, consistent with the project-wide naming standardization.src/layout/FileUpload/AttachmentSummaryComponent2.tsx (1)
8-8: LGTM! Import path casing standardized.The import path has been correctly updated from
PDFWrappertoPdfWrapper, completing the consistent naming across FileUpload components.src/features/instance/useProcessQuery.ts (1)
82-84: ServiceTask precedence looks correct; confirm backend elementType coverageReturning Service on elementType before altinnTaskType matches the "custom layout-set overrides default" intent. Please verify that elementType='ServiceTask' is reliably present across supported backend versions; otherwise fallback may misclassify.
src/features/pdf/PDFWrapper.test.tsx (1)
14-15: API rename to PdfWrapper is consistentImport and JSX updated; test intent unchanged. LGTM.
Also applies to: 55-60
src/language/texts/en.ts (1)
508-514: New service_task translations added correctlyKeys and placeholders match component usage. LGTM.
src/components/wrappers/ProcessWrapper.tsx (2)
20-24: Imports updated for ServiceTask and PdfWrapperImport adjustments align with new flow. LGTM.
148-156: Correct render path for Service tasksWrapping ServiceTask in PdfWrapper inside PresentationComponent matches the PDF/preview and UI patterns. LGTM.
src/App.tsx (1)
12-13: PdfWrapper rename and usage are consistentImport and wrapper around Presentation+Form look good. LGTM.
Also applies to: 86-91
src/features/pdf/PdfWrapper.tsx (1)
25-27: Dynamic PDF component selection by task type is correctSelecting PdfForServiceTask for Service tasks and PdfFromLayout otherwise matches the new flow. LGTM.
Also applies to: 41-45
src/types/shared.ts (2)
200-201: processTasks now typed as IProcessTask[]Good consolidation; enables access to elementType where available. LGTM.
240-247: Introduce IProcessTask with optional elementTypeThis supports ServiceTask detection and future backend versions. LGTM.
Please confirm that elementType is present in all environments you target; if not, keep altinnTaskType fallback paths exercised in tests.
src/features/pdf/PdfFromLayout.tsx (3)
36-47: PdfFromLayout: clear split between explicit layout vs auto-generationThe control flow and wrapping look good. Explicit pdfLayoutName takes precedence; otherwise falls back to auto-generation.
113-124: Page breaks between tasks: Verification completeConfirmed:
classes.pageBreakexists in the imported CSS module (src/features/pdf/PDFView.module.css)- CSS correctly defines
page-break-before: always;within@media printblock, ensuring hard page breaks in print media- Placement logic (break before each task after the first) is correct
The code is correct as written.
49-60: The original review comment is based on an incorrect assumption about the parameter names.The codebase definitively shows:
SearchParams.PdfForTaskis a single enum value mapped to'task'(not multiple variants)- The current code correctly uses
params.getAll(SearchParams.PdfForTask)to retrieve multiple task values via the standard?task=...&task=...URL pattern- No
'tasks'parameter exists anywhere in the codebase- The e2e test confirms tasks are passed as multiple
taskparameters, not as a separatetaskskeyThe suggested helper function
getTaskIdsFromParams()and its aliases are unnecessary; the current implementation is already correct and follows standard URLSearchParams conventions.Likely an incorrect or invalid review comment.
| @@ -0,0 +1,143 @@ | |||
| import 'cypress-iframe'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
Verify cypress-iframe import usage.
The cypress-iframe import is present but no iframe-related commands are used in the test. Consider removing this import if it's not needed, or document why it's required for test setup.
🏁 Script executed:
#!/bin/bash
# Check if cypress-iframe commands are used in this test file
rg -n "\.frameLoaded\(|\.iframeLoaded\(|\.enter\(|\.iframe\(" test/e2e/integration/service-task/service-task.tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# 1. Check if file exists and show the imports section
if [ -f "test/e2e/integration/service-task/service-task.ts" ]; then
echo "=== File exists. First 30 lines (imports section): ==="
head -30 test/e2e/integration/service-task/service-task.ts
echo ""
echo "=== Searching for any iframe-related patterns (case-insensitive, broader search): ==="
rg -in "iframe|frameloaded|iframeloaded|enter" test/e2e/integration/service-task/service-task.ts
else
echo "File not found at test/e2e/integration/service-task/service-task.ts"
fiLength of output: 1393
🏁 Script executed:
#!/bin/bash
# Check if AppFrontend or its dependencies use cypress-iframe
echo "=== Checking AppFrontend file for cypress-iframe usage: ==="
rg -in "cypress-iframe|frameloaded|iframeloaded" test/e2e/pageobjects/app-frontend.ts test/e2e/pageobjects/app-frontend.tsx 2>/dev/null || echo "AppFrontend file not found or no matches"
echo ""
echo "=== Searching all files in test/e2e/pageobjects for cypress-iframe: ==="
rg -in "cypress-iframe" test/e2e/pageobjects/ 2>/dev/null | head -20 || echo "No cypress-iframe usage in pageobjects"
echo ""
echo "=== Full line count of service-task.ts to understand file size: ==="
wc -l test/e2e/integration/service-task/service-task.tsLength of output: 296
Remove unused cypress-iframe import.
The import on line 1 is not used anywhere in this test file, in the AppFrontend page object, or in any pageobjects. Remove the import to keep dependencies clean.
🤖 Prompt for AI Agents
In test/e2e/integration/service-task/service-task.ts around line 1, there is an
unused import "import 'cypress-iframe';" which is not referenced anywhere in
this file or related page objects; remove that import statement on line 1, save
the file, run the e2e tests to confirm nothing breaks, and if no other files use
cypress-iframe consider removing the package from package.json/devDependencies.
1 participant
Description
This adds a default layout for a service-task, that will be displayed if the service task fails:
It also makes it possible for easy configuration on backend that triggers PDF generation with
?pdf=1&task=Task_1&task=Task_2which makes it possible to auto-generate a PDF for multiple previous tasks.Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
Release Notes
New Features
Tests
Documentation