Fix for pageBreak bug in repeating groups
#3746
Conversation
olemartinorg
added
kind/bug
📝 WalkthroughWalkthroughStops forwarding Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 1
🧹 Nitpick comments (1)
test/e2e/integration/frontend-test/pdf.ts (1)
233-269: Solid regression test for summary-based PDF with pageBreak; consider waiting for intercepted settingsTest is focused and validates the bug fix via page count. Optional: ensure the layout settings interception has applied before proceeding to reduce flakiness.
Apply this diff to wait for the intercepted settings response after navigation:
- cy.goto('group'); + cy.goto('group'); + cy.wait('@settings');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/layout/Summary/SummaryComponent.tsx(1 hunks)test/e2e/integration/frontend-test/pdf.ts(1 hunks)test/e2e/support/custom.ts(2 hunks)test/e2e/support/global.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/layout/Summary/SummaryComponent.tsxtest/e2e/support/custom.tstest/e2e/support/global.tstest/e2e/integration/frontend-test/pdf.ts
⏰ 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: Analyze (javascript)
- GitHub Check: Install
🔇 Additional comments (2)
src/layout/Summary/SummaryComponent.tsx (1)
192-195: Stop forwarding pageBreak/grid to inner summaries: correct fix for RepeatingGroup PDF regressionLGTM. Preventing inheritance of pageBreak and grid into RenderSummary resolves the per-field page breaks while still allowing the wrapper to apply page-level breaks via pageBreakStyles.
test/e2e/support/global.ts (1)
328-332: Good addition: Chainable typing for getPrintPageCountAPI typing and brief doc are clear and sufficient for consumers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/e2e/support/global.ts (2)
37-37: Document freeze semantics in TestPdfOptionsConsider adding a brief JSDoc for freeze (default true) explaining it stubs timers to “freeze” the page during PDF assertions.
330-339: New Chainable API: typings align with implementationsSignatures for expectPageBreaks and setFeatureToggle match the implementations in custom.ts. Minor: the comment says “approximate number of pages” but the assertion is strict equality; consider clarifying wording.
test/e2e/support/custom.ts (1)
690-707: Restore cy.clock() and print emulation when not returning to formWhen freeze is true, cy.clock() is installed but only restored inside the returnToForm branch later. If returnToForm is false, the stubbed timers (and print emulation) can leak and affect subsequent steps/tests.
Apply cleanup when not returning to form (add near the end of testPdf, just before the if (returnToForm) block):
// After the .then() that handles PDF assertions and optional snapshot: if (!returnToForm) { if (freeze) { cy.clock().invoke('restore'); } cy.setEmulatedMedia(); // reset to default (screen) cy.get('body').invoke('css', 'margin', ''); }This keeps existing behavior for returnToForm while preventing state leakage otherwise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/e2e/integration/frontend-test/pdf.ts(1 hunks)test/e2e/integration/multiple-datamodels-test/saving.ts(1 hunks)test/e2e/support/custom.ts(4 hunks)test/e2e/support/global.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/integration/frontend-test/pdf.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/integration/multiple-datamodels-test/saving.tstest/e2e/support/global.tstest/e2e/support/custom.ts
🧠 Learnings (1)
📚 Learning: 2025-09-25T14:35:11.396Z
Learnt from: olemartinorg
PR: Altinn/app-frontend-react#3746
File: test/e2e/support/custom.ts:993-1002
Timestamp: 2025-09-25T14:35:11.396Z
Learning: In Cypress custom commands defined with `Cypress.Commands.add()`, you don't need to explicitly return the Cypress chain from the command function. The last command executed in the chain automatically becomes the yielded value. For example, `cy.window().then((win) => { return someValue; })` will yield `someValue` even without a `return` statement at the command function level.
Applied to files:
test/e2e/support/custom.ts
🧬 Code graph analysis (2)
test/e2e/support/global.ts (1)
src/features/toggles.ts (1)
IFeatureToggles(29-29)
test/e2e/support/custom.ts (1)
src/features/toggles.ts (1)
IFeatureToggles(29-29)
⏰ 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). (2)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (6)
test/e2e/integration/multiple-datamodels-test/saving.ts (1)
14-14: Good move to typed feature togglesSwitching to cy.setFeatureToggle('saveOnBlur', false) improves readability and type safety. Looks correct here.
test/e2e/support/global.ts (1)
7-7: Type-safe toggle names import — LGTMImporting IFeatureToggles keeps the Chainable signature strict and in sync with app toggles.
test/e2e/support/custom.ts (4)
15-15: Importing IFeatureToggles — LGTMKeeps the command strongly typed against actual toggle keys.
640-640: Default freeze=true preserves previous behaviorSignature change is clear and backward compatible for existing tests.
998-1009: Page-break counter — LGTMSimple and effective assertion under print emulation. Note: counts both break-before and break-after set to page (as intended).
1011-1013: Feature toggle via cookie — LGTMCookie naming aligns with FEATURE_. Using boolean.toString() matches prior convention.
|
Description
This should fix #3745, which seemed to happen after refactoring in #3462
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
Bug Fixes
Tests
New Features