Feat/16446 navigation validation#4016
Merged
walldenfilippa merged 17 commits intomainfrom Apr 7, 2026
Merged
Conversation
Co-authored-by: walldenfilippa <filippa.walden@digdir.no>
…and validateOnPrevious/Backward with preventNavigation prop for validationOnNavigation
* add basic logic for preventing navigation * change wording
* added PageValidation on layoutSets and layoutSettings --------- Co-authored-by: walldenfilippa <filippa.walden@digdir.no>
* refactor: navigation validation backward and improve page validation hooks * refactor preventing navigation to use simplified config --------- Co-authored-by: walldenfilippa <filippa.walden@digdir.no> Co-authored-by: Magnus Revheim Martinsen <mrmartinsen.96@gmail.com>
* added prop expandedByDefault to groups for sidenavigation * added prop expandedByDefault to subform sidenavigation --------- Co-authored-by: walldenfilippa <filippa.walden@digdir.no>
Contributor
📝 WalkthroughWalkthroughAdds configurable page-level validation on navigation and a per-group expanded-by-default option; propagates these settings through layout/context hooks; prevents forward sidebar navigation when intermediate pages require validation and have errors; updates navigation components, hooks, and tests to enforce and respect this behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 |
Magnusrm
added
backport-ignore
…4027) * feat: enhance navigation validation by integrating validation checks in navigation components * removed one hook and added useGetNavigationIsPrevented on pageGroup aswell * temporary notes * remove temporary note * update tests --------- Co-authored-by: walldenfilippa <filippa.walden@digdir.no>
Contributor
|
/publish |
Contributor
PR release:
|
…ed validationOnNavigation to GlobalPageSettings (#4064) Co-authored-by: walldenfilippa <filippa.walden@digdir.no>
Contributor
|
/publish |
Contributor
PR release:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/features/navigation/components/PageGroup.tsx (1)
81-102:⚠️ Potential issue | 🔴 CriticalSingle-page groups still bypass page-leave validation.
Line 98 still does a raw
navigateToPage(page). The regular page entries now go throughmaybeSaveOnPageChange()andonPageNavigationValidation(...)first insrc/features/navigation/components/Page.tsx, so anyNavigationPageGroupSinglecan still be used to step forward without triggering the new sidebar validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/navigation/components/PageGroup.tsx` around lines 81 - 102, The single-page group button currently calls navigateToPage(page) directly (in the onClick inside PageGroup.tsx), bypassing the new page-leave validation; update the onClick handler (the performProcess async block) to replicate the same flow used in Page.tsx by first invoking maybeSaveOnPageChange() and onPageNavigationValidation(...) and only calling navigateToPage(page) if validation succeeds, preserving the existing performProcess wrapper and calling onNavigate() after successful navigation; reference the existing symbols navigateToPage, maybeSaveOnPageChange, onPageNavigationValidation, performProcess, and onNavigate to locate and implement the change.src/features/navigation/components/SubformsForPage.tsx (1)
68-91:⚠️ Potential issue | 🟠 Major
expandedByDefaultnow turns this into an always-open, unlabeled section.
PageGroupMultipleuses the same prop only to seedisOpen, but here it removes the toggle entirely. That means users can never collapse the subform section again, and Line 90 keepsaria-labelledby={buttonId}even when no element with thatidis rendered.Suggested shape
- {!expandedByDefault && ( - <button - id={buttonId} - aria-expanded={isOpen} - aria-owns={listId} - onClick={() => setIsOpen((o) => !o)} - className={cn(classes.subformExpandButton, 'fds-focus')} - > + <button + id={buttonId} + aria-expanded={isOpen} + aria-controls={listId} + onClick={() => setIsOpen((o) => !o)} + className={cn(classes.subformExpandButton, 'fds-focus')} + > <span className={classes.subformGroupName}> <Lang id={title} /> ({dataElements.length}) </span> <ChevronDownIcon aria-hidden className={cn(classes.subformExpandChevron, { [classes.subformExpandChevronOpen]: isOpen })} /> - </button> - )} + </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/navigation/components/SubformsForPage.tsx` around lines 68 - 91, The component currently removes the toggle button when expandedByDefault is true, which breaks collapsing and leaves aria-labelledby pointing at a non-existent buttonId; change the logic so expandedByDefault only seeds the initial isOpen state (like PageGroupMultiple) but still renders the toggle button tied to isOpen and setIsOpen (i.e., always render the button that uses buttonId and controls listId), and if you must hide a visual toggle use a visually-hidden class rather than omitting the element so aria-labelledby remains valid; ensure aria-expanded, aria-owns, and aria-labelledby remain consistent with the rendered button.
🧹 Nitpick comments (2)
src/layout/NavigationButtons/NavigationButtonsComponent.test.tsx (1)
139-141: Avoid fixed sleeps in these assertions.These tests are waiting on timing, not on the state transition they care about, so they'll get flaky as soon as navigation/validation takes longer than 100ms on CI. Please switch to a real async signal here (
waitFor,waitForElementToBeRemoved, or a mocked navigation assertion) instead of hard-coded delays.Also applies to: 154-156, 168-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/layout/NavigationButtons/NavigationButtonsComponent.test.tsx` around lines 139 - 141, Replace the hard-coded 100ms sleeps after userEvent.click(screen.getByText('next')) with a proper async wait that observes the actual UI change or navigation; e.g., use waitFor or waitForElementToBeRemoved to wait for the next button to disappear or for the expected element/state to appear (replace the new Promise(setTimeout...) call in the test at userEvent.click(...) with await waitFor(() => /* assertion for expected state */) or await waitForElementToBeRemoved(() => screen.getByText('next'))); apply the same change to the other occurrences around the tests referenced (the similar blocks at the other two locations).src/hooks/usePageValidation.ts (1)
5-20: Remove the redundant cast.
layoutCollection[pageKey]?.data?.validationOnNavigationalready comes from the generated layout type, soas ILayoutFile['data']['validationOnNavigation']only masks type drift and adds an import you do not need.Suggested cleanup
-import type { ILayoutFile, PageValidation } from 'src/layout/common.generated'; +import type { PageValidation } from 'src/layout/common.generated'; @@ - const pageValidation = currentPageLayout?.data - ?.validationOnNavigation as ILayoutFile['data']['validationOnNavigation']; + const pageValidation = currentPageLayout?.data?.validationOnNavigation;As per coding guidelines, "Avoid using
anyor type casting (as type) in TypeScript; instead, improve typing by removing such casts andanys to maintain proper type safety".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/usePageValidation.ts` around lines 5 - 20, The code in useEffectivePageValidation is using an unnecessary cast on layoutCollection[pageKey]?.data?.validationOnNavigation; remove the trailing "as ILayoutFile['data']['validationOnNavigation']" from the pageValidation assignment so the value is inferred from the generated types, and then delete the now-unused import of ILayoutFile at the top; keep the rest of useEffectivePageValidation, layoutCollection, and effectivePageValidation logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/navigation/utils.ts`:
- Around line 157-195: The current useGetNavigationIsPrevented implementation
scans only intermediate pages for validation errors and can allow skipping pages
that the sidebar marks incomplete; instead derive the allowed forward target
using the same completion logic the menu uses (useValidationsForPages or the
helper that computes page completion), e.g. call useValidationsForPages()/the
completion predicate over order starting at currentPageId to compute the
furthestAllowedIndex (or allowedPageKey) and then prevent navigation if
targetIndex > furthestAllowedIndex; update useGetNavigationIsPrevented
(referencing useGetNavigationIsPrevented, order, currentPageId, targetPageKey,
and useValidationsForPages) to use that computed allowed boundary rather than
the simple slice-check.
---
Outside diff comments:
In `@src/features/navigation/components/PageGroup.tsx`:
- Around line 81-102: The single-page group button currently calls
navigateToPage(page) directly (in the onClick inside PageGroup.tsx), bypassing
the new page-leave validation; update the onClick handler (the performProcess
async block) to replicate the same flow used in Page.tsx by first invoking
maybeSaveOnPageChange() and onPageNavigationValidation(...) and only calling
navigateToPage(page) if validation succeeds, preserving the existing
performProcess wrapper and calling onNavigate() after successful navigation;
reference the existing symbols navigateToPage, maybeSaveOnPageChange,
onPageNavigationValidation, performProcess, and onNavigate to locate and
implement the change.
In `@src/features/navigation/components/SubformsForPage.tsx`:
- Around line 68-91: The component currently removes the toggle button when
expandedByDefault is true, which breaks collapsing and leaves aria-labelledby
pointing at a non-existent buttonId; change the logic so expandedByDefault only
seeds the initial isOpen state (like PageGroupMultiple) but still renders the
toggle button tied to isOpen and setIsOpen (i.e., always render the button that
uses buttonId and controls listId), and if you must hide a visual toggle use a
visually-hidden class rather than omitting the element so aria-labelledby
remains valid; ensure aria-expanded, aria-owns, and aria-labelledby remain
consistent with the rendered button.
---
Nitpick comments:
In `@src/hooks/usePageValidation.ts`:
- Around line 5-20: The code in useEffectivePageValidation is using an
unnecessary cast on layoutCollection[pageKey]?.data?.validationOnNavigation;
remove the trailing "as ILayoutFile['data']['validationOnNavigation']" from the
pageValidation assignment so the value is inferred from the generated types, and
then delete the now-unused import of ILayoutFile at the top; keep the rest of
useEffectivePageValidation, layoutCollection, and effectivePageValidation logic
unchanged.
In `@src/layout/NavigationButtons/NavigationButtonsComponent.test.tsx`:
- Around line 139-141: Replace the hard-coded 100ms sleeps after
userEvent.click(screen.getByText('next')) with a proper async wait that observes
the actual UI change or navigation; e.g., use waitFor or
waitForElementToBeRemoved to wait for the next button to disappear or for the
expected element/state to appear (replace the new Promise(setTimeout...) call in
the test at userEvent.click(...) with await waitFor(() => /* assertion for
expected state */) or await waitForElementToBeRemoved(() =>
screen.getByText('next'))); apply the same change to the other occurrences
around the tests referenced (the similar blocks at the other two locations).
🪄 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: 0d5329cb-1eca-40b1-beae-85acd3754b3f
📒 Files selected for processing (14)
src/codegen/Common.tssrc/features/form/layout/LayoutsContext.tsxsrc/features/form/layoutSettings/LayoutSettingsContext.tsxsrc/features/navigation/components/Page.tsxsrc/features/navigation/components/PageGroup.tsxsrc/features/navigation/components/SubformsForPage.tsxsrc/features/navigation/utils.tssrc/features/validation/callbacks/onPageNavigationValidation.tssrc/hooks/usePageValidation.tssrc/layout/CustomButton/CustomButtonComponent.tsxsrc/layout/NavigationBar/NavigationBarComponent.tsxsrc/layout/NavigationButtons/NavigationButtonsComponent.test.tsxsrc/layout/NavigationButtons/NavigationButtonsComponent.tsxsrc/types/index.ts
* fix comments in feat/16446-navigation-validation * fixed tests in NavigationButtons tests * refactor: implement navigation with validation logic in Page and PageGroup components --------- Co-authored-by: walldenfilippa <filippa.walden@digdir.no>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/features/navigation/useNavigateToPageWithValidation.ts (1)
12-37: Consider wrapping the returned function inuseCallback.The async function is recreated on every render, which may cause unnecessary re-renders in consuming components that use it as a dependency or pass it to child components. Since the function closes over values from other hooks, wrapping it in
useCallbackwith appropriate dependencies would provide a stable reference.♻️ Suggested refactor
+import { useCallback } from 'react'; import { useOnPageNavigationValidation } from 'src/features/validation/callbacks/onPageNavigationValidation'; import { useNavigationParam } from 'src/hooks/navigation'; import { useNavigatePage } from 'src/hooks/useNavigatePage'; import { useEffectivePageValidation } from 'src/hooks/usePageValidation'; export function useNavigateToPageWithValidation() { const currentPageId = useNavigationParam('pageKey'); const { navigateToPage, order, maybeSaveOnPageChange } = useNavigatePage(); const onPageNavigationValidation = useOnPageNavigationValidation(); const { getPageValidation } = useEffectivePageValidation(currentPageId ?? ''); - return async (targetPage: string, onNavigate?: () => void) => { + return useCallback(async (targetPage: string, onNavigate?: () => void) => { if (!currentPageId || targetPage === currentPageId) { return; } const currentIndex = order.indexOf(currentPageId); const targetIndex = order.indexOf(targetPage); if (currentIndex === -1 || targetIndex === -1) { return; } const isForward = targetIndex > currentIndex; const validationOnNavigation = getPageValidation(); await maybeSaveOnPageChange(); if (isForward && validationOnNavigation) { const hasValidationErrors = await onPageNavigationValidation(currentPageId, validationOnNavigation); if (hasValidationErrors) { return; } } await navigateToPage(targetPage); onNavigate?.(); - }; + }, [currentPageId, order, getPageValidation, maybeSaveOnPageChange, onPageNavigationValidation, navigateToPage]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/navigation/useNavigateToPageWithValidation.ts` around lines 12 - 37, Wrap the async function returned by useNavigateToPageWithValidation in React's useCallback to stabilize its identity; specifically, change the anonymous returned function to a useCallback that depends on the external values it closes over (currentPageId, order, getPageValidation, maybeSaveOnPageChange, onPageNavigationValidation, navigateToPage) so consumers don't receive a new function each render and to avoid unnecessary re-renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/features/navigation/useNavigateToPageWithValidation.ts`:
- Around line 12-37: Wrap the async function returned by
useNavigateToPageWithValidation in React's useCallback to stabilize its
identity; specifically, change the anonymous returned function to a useCallback
that depends on the external values it closes over (currentPageId, order,
getPageValidation, maybeSaveOnPageChange, onPageNavigationValidation,
navigateToPage) so consumers don't receive a new function each render and to
avoid unnecessary re-renders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5dbe5a0f-9672-4ba4-892b-096e6437fc89
📒 Files selected for processing (6)
src/features/navigation/components/Page.tsxsrc/features/navigation/components/PageGroup.tsxsrc/features/navigation/components/SubformsForPage.tsxsrc/features/navigation/useNavigateToPageWithValidation.tssrc/hooks/usePageValidation.tssrc/layout/NavigationButtons/NavigationButtonsComponent.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/layout/NavigationButtons/NavigationButtonsComponent.test.tsx
- src/hooks/usePageValidation.ts
|
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
3 participants
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.
Description
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit