Fixing broken linkToComponent with openByDefault: first
#3921
Conversation
…e about to focus (possibly somewhere else) + regression test
olemartinorg
added
kind/bug
📝 WalkthroughWalkthroughThe changes add a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.tsx (1)
95-116: Well-implemented helper hook.The logic correctly identifies whether the focus target is a descendant of the repeating group by traversing up the component parent chain. The hook properly calls React hooks unconditionally before any conditional returns.
Minor naming note: The function uses "Focussing" (British spelling) while other parts of the codebase might use "Focusing" (American spelling). Consider standardizing if there's a preference in the project.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snapshots.js(1 hunks)src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.tsx(4 hunks)test/e2e/integration/frontend-test/group.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganytype or type casting (as type) in TypeScript code; improve typing by avoiding casts andanys when refactoring
Use objects for managing query keys and functions, andqueryOptionsfor sharing TanStack Query patterns across the system for central management
Files:
src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.tsxtest/e2e/integration/frontend-test/group.ts
{**/*.module.css,**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and leverage Digdir Design System components when possible
Files:
src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.tsxtest/e2e/integration/frontend-test/group.ts
🧠 Learnings (3)
📚 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/layout/RepeatingGroup/Providers/OpenByDefaultProvider.tsx
📚 Learning: 2025-11-25T12:53:54.399Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:53:54.399Z
Learning: Applies to **/*.test.{ts,tsx} : Use `renderWithProviders` from `src/test/renderWithProviders.tsx` when testing components that require form layout context
Applied to files:
src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.tsx
📚 Learning: 2025-11-25T12:53:54.399Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:53:54.399Z
Learning: Reduce dependency on React Context and migrate state management towards React Query patterns instead of Zustand wrappers
Applied to files:
src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.tsx
🧬 Code graph analysis (1)
src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.tsx (3)
src/hooks/useAsRef.ts (1)
useAsRef(12-16)src/features/form/layout/LayoutsContext.tsx (1)
useLayoutLookups(102-102)src/utils/splitDashedKey.ts (1)
splitDashedKey(18-48)
⏰ 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: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (4)
snapshots.js (1)
254-254: LGTM!The new
useBothRepGroupsfield aligns the snapshot data with the updated test scenario ingroup.ts, where this field is used to toggle the radio button that exercises thelinkToComponentnavigation behavior.src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.tsx (2)
46-47: LGTM!Using
useAsRefhere ensures the ref identity remains stable (preventing unnecessary effect re-runs) while still providing access to the latest computed value via.currentwhen the effect executes.
74-84: LGTM!The added condition
!isFocusingChildRef.currentcorrectly preventsopenByDefaultfrom interfering when the user is navigating to a specific child component vialinkToComponent. This is the core fix for the reported bug.test/e2e/integration/frontend-test/group.ts (1)
424-487: Excellent regression test coverage.This test comprehensively validates the fix by:
- Verifying
openByDefault: 'first'works initially- Testing that
linkToComponentnavigation to a child component bypasses theopenByDefaultbehavior (Lines 462-469)- Confirming that
openByDefaultstill triggers when focus moves outside the group (Lines 475-483)- Ensuring no layout shift affects the focused element (Line 486)
The test effectively captures the bug scenario and validates the expected behavior in both directions.
|
1 participant
Description
🚧
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.