Skip to content

Conversation

@olemartinorg
Copy link
Contributor

@olemartinorg olemartinorg commented Nov 3, 2025

Description

Replaces #3822.

There was a bug introduced in #3753, as navigation could still be blocked. This happened because a comparison in useWaitForValidation() was waiting until the last saved validation issues was set in ValidationContext. That's not what happens if the last thing to set incremental validations is something else, in this case updating attachment tags.

Related Issue(s)

  • closes #{issue number}

Verification/QA

  • Manual functionality testing
    • I have tested these changes manually (or, @cammiida has)
    • Creator of the original issue (or service owner) has been contacted for manual testing (or will be contacted when released in alpha)
    • No testing done/necessary
  • Automated tests
    • Unit test(s) have been added/updated
    • Cypress E2E test(s) have been added/updated
    • No automatic tests are needed here (no functional changes/additions)
    • I want someone to help me make some tests
  • UU/WCAG (follow these guidelines until we have our own)
    • I have tested with a screen reader/keyboard navigation/automated wcag validator
    • No testing done/necessary (no DOM/visual changes)
    • I want someone to help me perform accessibility testing
  • User documentation @ altinn-studio-docs
    • Has been added/updated
    • No functionality has been changed/added, so no documentation is needed
    • I will do that later/have created an issue
  • Support in Altinn Studio
    • Issue(s) created for support in Studio
    • This change/feature does not require any changes to Altinn Studio
  • Sprint board
    • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board
    • I don't have permissions to do that, please help me out
  • Labels
    • I have added a kind/* and backport* label to this PR for proper release notes grouping
    • I don't have permissions to add labels, please help me out

Summary by CodeRabbit

  • New Features

    • Added ability to manually set server-provided validation issues outside of the standard save/unlock flow, providing more granular control over validation state management.
    • Enhanced incremental validation with explicit control over when validation updates are committed to form data.
  • Tests

    • Updated test scaffolding to support new validation features.

Ole Martin Handeland added 2 commits November 3, 2025 12:05
@olemartinorg olemartinorg added kind/bug Something isn't working backport This PR should be cherry-picked onto older release branches labels Nov 3, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 3, 2025

📝 Walkthrough

Walkthrough

Adds a new method setLastValidationIssues to the form data infrastructure to allow manual setting of server-provided validation issues. Modifies useUpdateIncrementalValidations to accept an optional boolean parameter and conditionally update validation issues in form data. Updates test scaffolding accordingly.

Changes

Cohort / File(s) Summary
Form Data Layer
src/features/formData/FormDataWrite.tsx, src/features/formData/FormDataWriteStateMachine.tsx, src/features/formData/FormDataWriteProxies.tsx
Introduces new public hook and method setLastValidationIssues to set BackendValidationIssueGroups in form state, including proxy wrapper in default context.
Validation Update Hook
src/features/validation/backendValidation/useUpdateIncrementalValidations.ts, src/features/validation/backendValidation/BackendValidation.tsx
Adds optional boolean parameter setInFormData (default true) to useUpdateIncrementalValidations; uses new FD.useSetLastValidationIssues() hook; call site updated to pass false argument.
Test Scaffolding
src/test/renderWithProviders.tsx
Extends form data write proxies mock with new setLastValidationIssues proxy method.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Hook integration: Verify useSetLastValidationIssues correctly retrieves the setter from static selector store and is properly exposed in FormDataWriteProxies
  • State update logic: Confirm setLastValidationIssues in state machine correctly updates state.validationIssues without unintended side effects
  • Parameter propagation: Review how the setInFormData parameter flows through the validation hook and ensure backward compatibility with existing callers
  • Conditional update behavior: Validate the logic that conditionally invokes updateInFormData(lastSaveValidations) based on the new parameter

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title 'Fix for incremental validation updates when saving attachment tags' is clear and directly relates to the main change in the changeset. It accurately summarizes the bug fix that addresses the incremental validation issue when saving attachment tags, which is the core objective of this PR.
Description check ✅ Passed The pull request description is mostly complete and follows the template well. It provides a clear explanation of the bug fix, references the replaced PR (#3822) and the PR that introduced the bug (#3753), and includes comprehensive verification/QA checkboxes. However, the 'Related Issue(s)' section contains only a placeholder '#{issue number}' without an actual issue number, and the description lacks a concise non-technical summary suitable for release notes. Most critical sections are filled out appropriately.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/broken-navigation-saving-tags

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b39fcd4 and 18c8f37.

📒 Files selected for processing (6)
  • src/features/formData/FormDataWrite.tsx (1 hunks)
  • src/features/formData/FormDataWriteProxies.tsx (1 hunks)
  • src/features/formData/FormDataWriteStateMachine.tsx (2 hunks)
  • src/features/validation/backendValidation/BackendValidation.tsx (1 hunks)
  • src/features/validation/backendValidation/useUpdateIncrementalValidations.ts (4 hunks)
  • src/test/renderWithProviders.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: Avoid using any and 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 via queryOptions

Files:

  • src/features/validation/backendValidation/BackendValidation.tsx
  • src/features/validation/backendValidation/useUpdateIncrementalValidations.ts
  • src/features/formData/FormDataWrite.tsx
  • src/features/formData/FormDataWriteProxies.tsx
  • src/test/renderWithProviders.tsx
  • src/features/formData/FormDataWriteStateMachine.tsx
🧠 Learnings (2)
📚 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/features/validation/backendValidation/useUpdateIncrementalValidations.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:

  • src/test/renderWithProviders.tsx
🧬 Code graph analysis (3)
src/features/validation/backendValidation/BackendValidation.tsx (1)
src/features/validation/backendValidation/useUpdateIncrementalValidations.ts (1)
  • useUpdateIncrementalValidations (19-53)
src/features/validation/backendValidation/useUpdateIncrementalValidations.ts (1)
src/features/validation/validationContext.tsx (1)
  • Validation (320-375)
src/features/formData/FormDataWriteStateMachine.tsx (1)
src/features/validation/index.ts (1)
  • BackendValidationIssueGroups (99-101)
⏰ 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: Install
  • GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (11)
src/test/renderWithProviders.tsx (1)

200-200: LGTM! Test scaffolding correctly extended.

The new proxy method setLastValidationIssues is properly added to the test infrastructure, following the same pattern as existing proxies. This ensures tests can mock and track calls to the new form data method.

src/features/formData/FormDataWriteStateMachine.tsx (2)

211-211: LGTM! Clean interface extension.

The new method signature correctly extends FormDataMethods to allow manual setting of validation issues. The type BackendValidationIssueGroups | undefined appropriately matches the existing validationIssues state property.


609-612: LGTM! Straightforward implementation.

The implementation correctly updates state.validationIssues without side effects, which is appropriate for manually setting server-provided validation issues outside the normal save/unlock flow.

src/features/formData/FormDataWrite.tsx (1)

1163-1164: LGTM! Hook follows established patterns.

The new hook correctly exposes the setLastValidationIssues method via useStaticSelector, consistent with other setter hooks in the FD namespace. Placement next to useLastSaveValidationIssues maintains logical cohesion.

src/features/validation/backendValidation/BackendValidation.tsx (1)

21-21: LGTM! Correct parameter for initial validation context.

Passing false here prevents BackendValidation from updating form data validation issues, which is appropriate since this component handles initial validation setup. Incremental validations from other sources (like attachment tag saves) can independently control whether to update the last-saved validation issues in form data.

src/features/validation/backendValidation/useUpdateIncrementalValidations.ts (5)

6-6: LGTM! Correct import for new integration.

Importing FD to access useSetLastValidationIssues correctly establishes the dependency on form data infrastructure.


19-19: LGTM! Parameter provides necessary control.

The optional setInFormData parameter (default true) allows callers to control whether last-saved validation issues are updated in form data, which is the core of the bug fix. The default maintains backward compatibility.


23-23: LGTM! Correct hook usage.

The updateInFormData function is properly obtained from FD.useSetLastValidationIssues(), establishing the pathway to manually update form data validation issues.


38-41: LGTM! Conditional update correctly placed.

The conditional update of form data validation issues occurs before the deepEqual check, ensuring that the last-saved validation issues are tracked in form data even when the validations themselves haven't changed. This positioning is intentional and correct for the fix.


51-51: LGTM! Dependency array correctly updated.

The dependency array properly includes setInFormData and updateInFormData to ensure the callback remains stable and correct across renders.

src/features/formData/FormDataWriteProxies.tsx (1)

48-48: LGTM! Proxy correctly added to default context.

The new setLastValidationIssues proxy is properly integrated into the default FormDataWriteProxies context, following the established pattern. The default proxy implementation is appropriate for this method.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 3, 2025

Copy link
Contributor

@cammiida cammiida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@olemartinorg olemartinorg merged commit d682fc8 into main Nov 3, 2025
18 checks passed
@olemartinorg olemartinorg deleted the fix/broken-navigation-saving-tags branch November 3, 2025 11:40
github-actions bot pushed a commit that referenced this pull request Nov 3, 2025
…3827)

* Setting data in FormDataWrite so that waitForValidation() doesn't fail later after incremental validation updates arrive from tags saving

* Moving the code snippet so it always happens

---------

Co-authored-by: Ole Martin Handeland <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2025

Automatic backport successful!

A backport PR has been automatically created for the release/v4.22 release branch.

The release branch release/v4.22 already existed and was updated.

The cherry-pick was clean with no conflicts. Please review the backport PR when it appears.

olemartinorg added a commit that referenced this pull request Nov 3, 2025
…3827)

* Setting data in FormDataWrite so that waitForValidation() doesn't fail later after incremental validation updates arrive from tags saving

* Moving the code snippet so it always happens

---------

Co-authored-by: Ole Martin Handeland <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport This PR should be cherry-picked onto older release branches kind/bug Something isn't working

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants