Adding PDF support in subforms #3637
Conversation
…sing layout lookups
…d custom PDFs work for a given subform element
olemartinorg
added
kind/product-feature
📝 WalkthroughWalkthroughUpdates PDF page child computation in PdfView2 to use memoized top-level components filtering. Wraps Subform forms with PDFWrapper. Adds new Cypress tests for subform PDFs, removes overlapping tests from an existing suite, and extends Cypress support to allow custom PDF URL building with a new buildUrl option and corresponding type updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
test/e2e/support/global.ts (1)
33-41: Expose and document buildUrl option; note minor type alignment with implementationAdding buildUrl?: (href: string) => string; is a clean, minimal API to support custom PDF URL rewrites. Please document it for future consumers, and consider aligning the testPdf implementation to avoid assigning a boolean default to snapshotName (see comment in custom.ts) since TestPdfOptions declares it as string.
Apply this diff to document the new option:
export interface TestPdfOptions { snapshotName?: string; beforeReload?: () => void; callback: () => void; returnToForm?: boolean; enableResponseFuzzing?: boolean; + /** + * Custom builder for the PDF URL used by cy.testPdf(). Receives window.location.href + * and must return a navigable URL that renders a PDF (for instance by adding '?pdf=1' + * or rewriting to a subform path). + */ buildUrl?: (href: string) => string; }src/layout/Subform/SubformWrapper.tsx (1)
10-10: Avoid Nested PDFWrapper in SubformFormIt looks like you’re wrapping subforms in a PDFWrapper even though the top‐level routes in App.tsx already wrap every Data‐entry view in a PDFWrapper. This will produce a nested PDFWrapper when a subform is rendered via the dynamic route.
Files involved:
- src/App.tsx
- line 87:
<PDFWrapper><Form /></PDFWrapper>- line 123:
<PDFWrapper><PresentationComponent type={ProcessTaskType.Data}><Form/></PresentationComponent></PDFWrapper>- src/layout/Subform/SubformWrapper.tsx
- lines 28–32: your new
<PDFWrapper>…</PDFWrapper>around the subform viewTo avoid double‐wrapping, either remove the inner PDFWrapper in SubformForm or conditionally render it only when no ancestor has already enabled PDF mode. For example:
export function SubformForm() { - return ( - <PDFWrapper> - <PresentationComponent type={ProcessTaskType.Data}> - <Form /> - </PresentationComponent> - </PDFWrapper> - ); + const pdfActive = useIsPdf() || useDevToolsStore(s => s.pdfPreview); + return pdfActive ? ( + <PresentationComponent type={ProcessTaskType.Data}> + <Form /> + </PresentationComponent> + ) : ( + <PDFWrapper> + <PresentationComponent type={ProcessTaskType.Data}> + <Form /> + </PresentationComponent> + </PDFWrapper> + ); }This way, the inner wrapper only kicks in if you’re not already in PDF mode.
src/features/pdf/PdfView2.tsx (1)
144-156: Defensive guard around component defs to avoid rare runtime crashesThe new topLevelComponents-based filtering looks solid and simplifies the logic. One edge case: getComponentDef(component.type) could theoretically return undefined (or lack shouldRenderInAutomaticPDF). Add a defensive guard with a sensible default to prevent hard-to-debug runtime errors if a new component type is introduced or mis-registered. Also precompute excludedComponents as a Set for clarity.
Apply this diff:
- 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) && - def.shouldRenderInAutomaticPDF(component as never) - ); - }); - }, [lookups, pageKey, pdfSettings?.excludedComponents]); + const children = useMemo(() => { + const topLevel = lookups.topLevelComponents[pageKey] ?? []; + const excluded = new Set(pdfSettings?.excludedComponents ?? []); + return topLevel.filter((baseId) => { + const component = lookups.getComponent(baseId); + if (component.type === 'Subform') { + return false; + } + if (excluded.has(baseId)) { + return false; + } + const def = getComponentDef(component.type); + const shouldRender = + def?.shouldRenderInAutomaticPDF + ? def.shouldRenderInAutomaticPDF(component as never) + : true; // Default to rendering if a component hasn't specified behavior + return shouldRender; + }); + }, [lookups, pageKey, pdfSettings?.excludedComponents]);test/e2e/support/custom.ts (2)
631-639: Align snapshotName typing with TestPdfOptionsThe options param is typed as TestPdfOptions where snapshotName?: string, but the implementation assigns a boolean default (false). This widens the runtime type and can confuse the compiler and readers. Default to undefined and rely on the truthiness check.
Apply this diff:
({ - snapshotName = false, + snapshotName, beforeReload, callback, returnToForm = false, enableResponseFuzzing = false, buildUrl = buildPdfUrl, }) => {
660-672: Minor: surface builder errors with more context (optional)If a custom buildUrl throws, the error will bubble during cy.window().then(). Optionally wrap and rethrow with the current href for better diagnostics.
Apply this diff:
- cy.window({ log: false }).then((win) => { - const visitUrl = buildUrl(win.location.href); + cy.window({ log: false }).then((win) => { + let visitUrl: string; + try { + visitUrl = buildUrl(win.location.href); + } catch (e) { + throw new Error(`testPdf: buildUrl failed for href='${win.location.href}': ${String(e)}`); + }test/e2e/integration/subform-test/pdf.ts (3)
57-76: Single-subform PDF URL builder is pragmatic; consider a more robust variantbuildUrlForSingleSubform currently asserts path shapes and then performs string replacements. To reduce brittleness across future navigation changes, consider deriving the base up to the current subform and replacing only the trailing page segment, or reuse getInstanceIdRegExp to isolate the part after the instance id before rewriting. Keeping the assertions is good, but a regex-based slice would be more robust. I can draft a URL-rewrite helper if desired.
78-85: Layout settings intercept: guard for missing pages (optional)If pages is missing in layout settings, body.pages.pdfLayoutName would throw. While unlikely in this app, consider a defensive set (e.g., ensure body.pages exists) to make the test self-explanatory on failure.
Apply this diff:
cy.intercept('GET', '**/api/layoutsettings/moped-subform', (req) => { req.on('response', (res) => { const body = JSON.parse(res.body) as ILayoutSettings; - body.pages.pdfLayoutName = 'moped-pdf'; // Forces PDF engine to use a tailor-made layout + body.pages = body.pages ?? {}; + body.pages.pdfLayoutName = 'moped-pdf'; // Forces PDF engine to use a tailor-made layout res.send(body); }); }).as('settings');
175-187: buildUrlForSingleSubform: add clearer failure messages and future-proofing (optional)Good to have explicit guards. Consider enhancing the error messages with the actual href to speed up debugging, and optionally using a regex slice anchored after the instance id to avoid coupling to specific page names ('utfylling').
Apply this diff:
function buildUrlForSingleSubform(href: string) { if (!href.includes('/utfylling/') && !href.includes('/whatever/')) { - // We replace this with 'whatever' to make sure we can still load the PDF whatever the main page - // name is. It should not matter inside this subform. - throw new Error('Expected URL to contain /utfylling/ but it was not found'); + // We replace this with 'whatever' to make sure we can still load the PDF whatever the main page + // name is. It should not matter inside this subform. + throw new Error(`Expected URL to contain /utfylling/ (or /whatever/); got: ${href}`); } if (!href.endsWith('/moped-utfylling')) { - throw new Error('Expected URL to end with /moped-utfylling but it was not found'); + throw new Error(`Expected URL to end with /moped-utfylling; got: ${href}`); } return href.replace('/utfylling/', '/whatever/').replace('/moped-utfylling', '/?pdf=1'); }
📜 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 (6)
src/features/pdf/PdfView2.tsx(2 hunks)src/layout/Subform/SubformWrapper.tsx(2 hunks)test/e2e/integration/subform-test/pdf.ts(1 hunks)test/e2e/integration/subform-test/subform.ts(0 hunks)test/e2e/support/custom.ts(3 hunks)test/e2e/support/global.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/e2e/integration/subform-test/subform.ts
🧰 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/support/global.tssrc/features/pdf/PdfView2.tsxsrc/layout/Subform/SubformWrapper.tsxtest/e2e/integration/subform-test/pdf.tstest/e2e/support/custom.ts
🧠 Learnings (1)
📚 Learning: 2025-08-22T13:53:28.201Z
Learnt from: CR
PR: Altinn/app-frontend-react#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.201Z
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/layout/Subform/SubformWrapper.tsx
🧬 Code graph analysis (4)
src/features/pdf/PdfView2.tsx (2)
src/features/expressions/expression-functions.ts (1)
component(426-462)src/layout/index.ts (1)
getComponentDef(34-42)
src/layout/Subform/SubformWrapper.tsx (3)
src/features/pdf/PDFWrapper.tsx (1)
PDFWrapper(17-50)src/components/presentation/Presentation.tsx (1)
PresentationComponent(34-99)src/components/form/Form.tsx (1)
Form(38-42)
test/e2e/integration/subform-test/pdf.ts (2)
test/e2e/pageobjects/app-frontend.ts (1)
AppFrontend(3-371)src/layout/layout.ts (1)
ILayoutCollection(85-85)
test/e2e/support/custom.ts (1)
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: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
test/e2e/integration/subform-test/pdf.ts (2)
32-53: End-to-end coverage for subform content in PDFs looks solidThe first PDF test validates both top-level and subform summaries and exercises return-to-form with a subsequent edit. Assertions are focused and resilient.
104-172: Custom PDF via Summary2 and overrides: great scenario coverageNice use of layoutsettings and layouts intercepts to validate Summary2 with subform table overrides. The assertions check headers and a subset of cell values—concise and effective.
|
/publish |
PR release:
|
Description
This adds PDF support for a single subform element. Most of these changes are about adding tests.
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit