Fix issue with broken RemoveHiddenData when a whole page is hidden#1721
Merged
Fix issue with broken RemoveHiddenData when a whole page is hidden#1721
Conversation
The root cause was that there is no `removeWhenHidden` property on pages, so the `PageComponent` was initialized with `null` in that field. ExpressionValue.ToBoolLoose() copies expression semantics that treat `null` as `false`, so in `BaseComponent.ShouldRemoveWhenHidden` we now check for null, because for that property null and undefined must be treated as true.
ivarne
added
bugfix
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Altinn.App.Core/Models/Layout/Components/Base/BaseComponent.cs (1)
108-114: HandleJsonValueKind.Undefinedexplicitly in the default branch.Given the new
Expression.Undefinedpath, explicitly includingUndefinedhere makes behavior unambiguous and independent ofToBoolLoose()semantics.Proposed diff
- if (removeWhenHidden.ValueKind == JsonValueKind.Null) + if (removeWhenHidden.ValueKind is JsonValueKind.Null or JsonValueKind.Undefined) { // ToBoolLoose returns false when the value is null, but we want to return the default value in that case, so we check for null before calling ToBoolLoose() return defaultShouldRemove; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Models/Layout/Components/Base/BaseComponent.cs` around lines 108 - 114, The current default branch in BaseComponent.cs treats only JsonValueKind.Null separately but doesn't handle JsonValueKind.Undefined introduced by Expression.Undefined, so update the check around removeWhenHidden in the method (referencing removeWhenHidden, ToBoolLoose(), and the BaseComponent class) to explicitly treat both JsonValueKind.Null and JsonValueKind.Undefined as the default case (return defaultShouldRemove) before calling ToBoolLoose(), ensuring behavior is deterministic and not relying on ToBoolLoose() semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.cs`:
- Around line 475-476: Replace FluentAssertions usage in the test assertions
with xUnit asserts: change the two calls on dataString (the lines using
dataString.Should().Contain("<hiddenPage>...") and
dataString.Should().Contain("<hiddenPageNotRemove>...")) to use Assert.Contains
for strings that must be present and Assert.DoesNotContain for strings that must
not be present; also update the similar pair around lines 490-491 accordingly so
all four assertions follow the repo convention (use Assert.Contains /
Assert.DoesNotContain with the same expected substring and actual dataString).
---
Nitpick comments:
In `@src/Altinn.App.Core/Models/Layout/Components/Base/BaseComponent.cs`:
- Around line 108-114: The current default branch in BaseComponent.cs treats
only JsonValueKind.Null separately but doesn't handle JsonValueKind.Undefined
introduced by Expression.Undefined, so update the check around removeWhenHidden
in the method (referencing removeWhenHidden, ToBoolLoose(), and the
BaseComponent class) to explicitly treat both JsonValueKind.Null and
JsonValueKind.Undefined as the default case (return defaultShouldRemove) before
calling ToBoolLoose(), ensuring behavior is deterministic and not relying on
ToBoolLoose() semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 148bd19f-ff23-4f60-9243-6bcde4c71d03
📒 Files selected for processing (8)
src/Altinn.App.Core/Models/Expressions/Expression.cssrc/Altinn.App.Core/Models/Layout/Components/Base/BaseComponent.cssrc/Altinn.App.Core/Models/Layout/Components/PageComponent.cstest/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.cstest/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.cstest/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/ui/default/Settings.jsontest/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/ui/default/layouts/hidden_page.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
There was a problem hiding this comment.
Pull request overview
Fixes hidden-data removal behavior when an entire page is hidden by ensuring removeWhenHidden defaults correctly for pages and by adding regression coverage.
Changes:
- Adjust page component initialization to use an “undefined”
removeWhenHiddenvalue (instead ofnull) so default removal semantics apply. - Update
BaseComponent.ShouldRemoveWhenHiddento treatnullas “use default” (true) rather than as false. - Add an API test app layout + extended controller test assertions to cover hidden whole-page scenarios.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt | Updates public API snapshot to include Expression.Undefined. |
| test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/ui/default/layouts/hidden_page.json | Adds a hidden page layout to reproduce/cover whole-page hidden removal behavior. |
| test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/ui/default/Settings.json | Adds the new hidden page to the page order for the test app. |
| test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.cs | Extends the test model with fields bound to the new hidden page components. |
| test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.cs | Extends the existing hidden-data removal test to include hidden-page fields. |
| src/Altinn.App.Core/Models/Layout/Components/PageComponent.cs | Sets page RemoveWhenHidden to Expression.Undefined and minor refactor for child list creation. |
| src/Altinn.App.Core/Models/Layout/Components/Base/BaseComponent.cs | Updates ShouldRemoveWhenHidden to treat null as “default true”. |
| src/Altinn.App.Core/Models/Expressions/Expression.cs | Adds Expression.Undefined convenience literal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
HauklandJ
approved these changes
Apr 21, 2026
|
|
✅ Automatic backport successful! A backport PR has been automatically created for the The release branch The cherry-pick was clean with no conflicts. Please review the backport PR when it appears. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The root cause was that there is no
removeWhenHiddenproperty on pages, so thePageComponentwas initialized withnullin that field.ExpressionValue.ToBoolLoose() copies expression semantics that treat
nullasfalse, so inBaseComponent.ShouldRemoveWhenHiddenwe now check for null, because for that property null and undefined must be treated as true.Summary by CodeRabbit
Bug Fixes
New Features
Tests
API