Hide edit button in Summary when target not editable #3905
Conversation
JamalAlabdullah
added
kind/bug
📝 WalkthroughWalkthroughPer-row editability was introduced for Summary2 repeating groups: rendering of Edit buttons is gated by per-row expressions and an editable-child context; layout hooks compute editable child IDs; tests and an e2e case were added/updated to exercise editButton visibility. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
@JamalAlabdullah Maybe you should backport this? Since this is a bugfix, we can backport it and release it in a patch-release. Just switch to the backport label and it will be done mostly automatically. |
JamalAlabdullah
added
backport
|
@JamalAlabdullah noen tester feiler, kan du se på de? |
|
Tror disse testene løser seg selv hvis du merger fra main en gang, @JamalAlabdullah - det er en del som har blitt fikset der nå. |
lassopicasso
left a comment
lassopicasso
left a comment
There was a problem hiding this comment.
Jeg har testet den, ser bra ut. Edit knappen vises ikke i tabellen hvis man setter editButton: false. Men skal denne knappen også skjules hvis repeterende gruppe vises som gruppe i oppsummeringen? Den gjør den ikke nå.
La forresten merke til at skillelinjene ikke tar full bredde når edit-knapp ikke er tilstede. Er dette ønskelig?
I am not sure if we need this also? i just followed the description in the issue , I fixed the dividing lines width. 
|
lassopicasso
left a comment
lassopicasso
left a comment
There was a problem hiding this comment.
I am not sure if we need this also? i just followed the description in the issue , I fixed the dividing lines width.
Perhaps it is not necessary, do you have any thoughts about it @olemartinorg ? I approve this.
|
Hmm, I tested this now, and there are a lot of edit buttons showing up in On the flip side, even if you turn In the older Summary component we have the TL;DR: When thinking about it more, this is probably more complex than it seemed at first glance. The current fix might even break some configurations. @JamalAlabdullah I can take a look at this one for you, if you want! |
olemartinorg
changed the title
olemartinorg
force-pushed
the
16422-endre-knapp-vises-i-summary2-på-en-ikke-redigerbar-repeterende-gruppe
branch
from
fd81601 to
859c6ac
Compare
… be considered editable inside a repeating group. This works much better in the summary view now, and the test should cover most things
olemartinorg
force-pushed
the
16422-endre-knapp-vises-i-summary2-på-en-ikke-redigerbar-repeterende-gruppe
branch
from
859c6ac to
db26f3e
Compare
![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
🤖 Fix all issues with AI agents
In
@src/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx:
- Line 144: The hook is being called with an empty-string UUID fallback which
can produce an invalid lookup; instead call useRowWithExpressions with a
meaningful sentinel when row is missing, e.g. pass row ? { uuid: row.uuid } :
'first' so you don't supply '' — update the call to
RepGroupHooks.useRowWithExpressions(baseComponentId, row ? { uuid: row.uuid } :
'first') to avoid the empty-string UUID case while keeping hooks order stable.
🧹 Nitpick comments (3)
src/layout/RepeatingGroup/utils.ts (1)
266-274: Consider memoizing the returned array.The
filtercall creates a new array reference on every render, which could cause unnecessary re-renders in consuming components. Other hooks in this file (e.g.,useAllRowsWithHidden,useAllRowsWithButtons) useuseMemoto stabilize their return values.♻️ Suggested memoization
useEditableChildren(baseComponentId: string, rowWithExpressions: RepGroupRowWithExpressions | undefined): string[] { const childrenBaseIds = RepGroupHooks.useChildIds(baseComponentId); const layoutLookups = useLayoutLookups(); const component = layoutLookups.getComponent(baseComponentId, 'RepeatingGroup'); - return childrenBaseIds.filter((childId) => - isChildEditableCheck(childId, layoutLookups, component, rowWithExpressions), - ); + return useMemo( + () => + childrenBaseIds.filter((childId) => + isChildEditableCheck(childId, layoutLookups, component, rowWithExpressions), + ), + [childrenBaseIds, layoutLookups, component, rowWithExpressions], + ); },src/layout/RepeatingGroup/Summary2/RepGroupSummaryEditableContext.tsx (1)
19-21: Memoization depends on array reference stability.The
useMemodependency is[editableChildIds], which is an array. If the parent component passes a new array reference on each render (even with the same contents), this will create a newSetunnecessarily.However, looking at the usage in
RepeatingGroupSummary.tsx, theeditableChildIdscomes fromRepGroupHooks.useEditableChildren, which currently doesn't memoize its result (as noted in my earlier comment). If that hook is updated to useuseMemo, this concern is mitigated.Consider using a deep comparison hook like
useMemoDeepEqual(already imported in utils.ts) if array contents stability is important, or ensure the upstream hook memoizes its output.src/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx (1)
146-146: Potential duplicate IDs and O(n) lookup.The combination
[...ids, ...children]may contain duplicates since table IDs are typically a subset of children. Also,editableChildren.includes(id)is O(n) for each element.Consider using a Set for both deduplication and faster lookup:
♻️ Suggested optimization
- const editableIds = [...ids, ...children].filter((id) => editableChildren.includes(id)); + const editableSet = new Set(editableChildren); + const editableIds = [...new Set([...ids, ...children])].filter((id) => editableSet.has(id));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/layout/RepeatingGroup/Summary2/RepGroupSummaryEditableContext.tsxsrc/layout/RepeatingGroup/Summary2/RepeatingGroupSummary.tsxsrc/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.test.tsxsrc/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsxsrc/layout/RepeatingGroup/utils.tssrc/layout/Summary2/CommonSummaryComponents/EditButton.tsxtest/e2e/integration/component-library/repeating-group.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.test.tsx
🧰 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/utils.tstest/e2e/integration/component-library/repeating-group.tssrc/layout/RepeatingGroup/Summary2/RepeatingGroupSummary.tsxsrc/layout/RepeatingGroup/Summary2/RepGroupSummaryEditableContext.tsxsrc/layout/Summary2/CommonSummaryComponents/EditButton.tsxsrc/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx
{**/*.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/utils.tstest/e2e/integration/component-library/repeating-group.tssrc/layout/RepeatingGroup/Summary2/RepeatingGroupSummary.tsxsrc/layout/RepeatingGroup/Summary2/RepGroupSummaryEditableContext.tsxsrc/layout/Summary2/CommonSummaryComponents/EditButton.tsxsrc/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx
🧠 Learnings (1)
📚 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/utils.tssrc/layout/RepeatingGroup/Summary2/RepGroupSummaryEditableContext.tsx
🧬 Code graph analysis (3)
src/layout/RepeatingGroup/Summary2/RepGroupSummaryEditableContext.tsx (1)
src/core/contexts/context.tsx (1)
createContext(45-89)
src/layout/Summary2/CommonSummaryComponents/EditButton.tsx (3)
src/utils/layout/hidden.ts (2)
useIsHiddenMulti(82-136)useIsHidden(44-77)src/layout/Summary/EditButton.tsx (1)
EditButton(14-30)src/layout/RepeatingGroup/Summary2/RepGroupSummaryEditableContext.tsx (1)
useIsEditableInRepGroup(27-33)
src/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx (3)
src/layout/RepeatingGroup/useTableComponentIds.ts (1)
useTableComponentIds(10-48)src/layout/RepeatingGroup/utils.ts (1)
RepGroupHooks(97-275)src/layout/Summary2/CommonSummaryComponents/EditButton.tsx (1)
EditButtonFirstVisibleAndEditable(28-49)
⏰ 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: Install
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (9)
test/e2e/integration/component-library/repeating-group.ts (1)
189-265: Well-structured E2E test covering multiple scenarios.The test effectively covers the key scenarios:
- Initial state with edit buttons visible
- Hiding edit buttons via
edit.editButton = false- Re-enabling edit for specific components via
editInTable: true- Verifying
readOnlycomponents don't get edit buttonsThe use of
data-target-idselectors provides precise verification of which specific edit buttons are rendered.src/layout/RepeatingGroup/Summary2/RepeatingGroupSummary.tsx (2)
143-175: Good extraction of per-row rendering logic.The
RepGroupListRowcomponent cleanly encapsulates:
- Row expression computation
- Editable children calculation
- Context providers for data model location and editability
- Conditional divider rendering
This separation improves readability and ensures each row has its own editability context.
139-139: Type at line 139 may not match the actualIDataModelBindings<'RepeatingGroup'>structure.The component declares
dataModelBindings: { group: IDataModelReference }as required, but throughout the codebase,groupis consistently accessed with optional chaining (e.g.,?.group). This pattern appears inutils.ts,Summary.tsx, and test files, suggestinggroupis optional in the actual type. Ifgroupcan be undefined, the type should reflect this as{ group?: IDataModelReference }or the component should add a guard before usingdataModelBindings.groupat line 158.src/layout/RepeatingGroup/Summary2/RepGroupSummaryEditableContext.tsx (1)
27-33: Safe default behavior for context absence.Returning
truewhen no context is provided ensures backward compatibility - components outside repeating group contexts will remain editable by default. TheSet.has()provides efficient O(1) lookup.src/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx (1)
172-176: Correct gating logic for edit button visibility.The implementation correctly:
- Uses
key={editableIds.join(',')}to trigger re-renders when editability changes- Provides
editableIdsas the pool of potential targets- Sets
fallbacktoundefinedwheneditButtonisfalse, ensuring no edit button appears for non-editable rowssrc/layout/Summary2/CommonSummaryComponents/EditButton.tsx (3)
78-82: Hook call placement may cause unnecessary early returns.The
useIsEditableInRepGrouphook is called unconditionally (which is correct for React hooks), but the check is placed before other conditionals that might also returnnull(likeisReadOnly,overriddenDataElementId,pdfModeActive).This is fine for correctness, but if
useIsEditableInRepGroupis expensive, consider ordering checks from cheapest to most expensive. However, since it's just a context lookup with a Set check, the current placement is acceptable.
28-49: Clean implementation of visibility and editability gating.The component properly:
- Finds the first visible ID from the candidates
- Falls back to the provided fallback if all candidates are hidden
- Returns
nullwhen no valid target exists- Correctly sets
skipLastIdMutatorwhen using the fallbackThe rename from
EditButtonFirstVisibletoEditButtonFirstVisibleAndEditableaccurately reflects the new behavior where editability is considered.
116-116: Useful addition for testing and debugging.The
data-target-idattribute enables precise selection of edit buttons in tests (as seen in the E2E test usingcy.get('button[data-target-id="..."]')), making tests more robust and easier to debug.src/layout/RepeatingGroup/utils.ts (1)
84-86: No changes needed — the expression handling is correct.The
columnSettings?.hidden === truecheck at line 85 is appropriate. AlthoughtableColumns[column].hiddencan initially be an expression, it is evaluated to a boolean value insrc/layout/RepeatingGroup/index.tsx(line 56) viaevalBool(tableColumns[column].hidden, false)before the component is stored. By the timeisChildEditableCheckreceivesparentComponent, thehiddenfield is already a boolean, so the=== truecheck correctly distinguishes betweentrueandfalse/undefinedcases.
src/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx
Show resolved
Hide resolved
|
JamalAlabdullah
added
the
squad/utforming
3 participants
Description
closes: #16422
Changes:
Test:
Add this to
RepeatingGroupin json file after adding a few rows:Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.