fix: use summaryTitle when available for summary2 components #3834
Conversation
📝 WalkthroughWalkthroughThis pull request systematically updates summary and component files across the layout module to prioritize Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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 |
Jondyr
added
kind/bug
Jondyr
force-pushed
the
fix/use-summaryTitle-when-available-in-summary2
branch
2 times, most recently
from
49f30a1 to
e9de154
Compare
|
Jondyr
force-pushed
the
fix/use-summaryTitle-when-available-in-summary2
branch
from
e9de154 to
aea8703
Compare
Jondyr
force-pushed
the
fix/use-summaryTitle-when-available-in-summary2
branch
from
aea8703 to
28eb2e0
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: 2
🧹 Nitpick comments (2)
src/layout/MultipleSelect/MultipleSelectSummary.tsx (1)
38-38: Works correctly; consider extracting for consistency.The inline usage is functionally correct but differs from the pattern used in other components (e.g., CheckboxesSummary.tsx line 24, DropdownSummary.tsx line 20, RadioButtonsSummary.tsx line 20), which extract the title to a variable first.
For consistency with the rest of the codebase, consider this refactor:
const { textResourceBindings, required } = useItemWhenType(targetBaseComponentId, 'MultipleSelect'); const displayValues = useMultipleValuesForSummary(targetBaseComponentId); + const title = textResourceBindings?.summaryTitle || textResourceBindings?.title; return ( <SummaryFlex targetBaseId={targetBaseComponentId} content={ displayValues.length === 0 ? required ? SummaryContains.EmptyValueRequired : SummaryContains.EmptyValueNotRequired : SummaryContains.SomeUserContent } > <MultipleValueSummary - title={<Lang id={textResourceBindings?.summaryTitle || textResourceBindings?.title} />} + title={<Lang id={title} />} baseComponentId={targetBaseComponentId} displayValues={displayValues} showAsList={showAsList} isCompact={isCompact} emptyFieldText={overrides?.emptyFieldText} /> </SummaryFlex>src/layout/Group/GroupSummary.tsx (1)
61-62: Simplify redundant title variable.The new
titlevariable (line 61) already implements the fallback logicsummaryTitle || title, but line 90 still referencessummaryTitle ?? title. Sincetitlealready handles the fallback, you can simplify line 90 to just use thetitlevariable directly, eliminating the redundancy.Apply this diff:
- <Lang id={summaryTitle ?? title} /> + <Lang id={title} />Alternatively, if you prefer to keep the existing pattern, you can remove the new
titlevariable and keep line 90 as-is.Also applies to: 90-90
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/layout/Accordion/SummaryAccordion.tsx(2 hunks)src/layout/Address/AddressSummary/AddressSummary.tsx(2 hunks)src/layout/Checkboxes/CheckboxesSummary.tsx(1 hunks)src/layout/Date/DateSummary.tsx(1 hunks)src/layout/Datepicker/DatepickerSummary.tsx(1 hunks)src/layout/Dropdown/DropdownSummary.tsx(1 hunks)src/layout/FileUpload/AttachmentSummaryComponent2.tsx(3 hunks)src/layout/Grid/GridSummary.tsx(1 hunks)src/layout/Group/GroupSummary.tsx(1 hunks)src/layout/ImageUpload/ImageUploadSummary2/ImageUploadSummary2.tsx(1 hunks)src/layout/Input/InputSummary.tsx(1 hunks)src/layout/Likert/LikertComponent.tsx(1 hunks)src/layout/Likert/Summary2/LikertSummary.tsx(5 hunks)src/layout/List/ListComponent.tsx(1 hunks)src/layout/Map/Summary2/MapSummary.tsx(1 hunks)src/layout/MultipleSelect/MultipleSelectSummary.tsx(1 hunks)src/layout/Number/NumberSummary.tsx(1 hunks)src/layout/Option/OptionSummary.tsx(1 hunks)src/layout/OrganisationLookup/OrganisationLookupSummary.tsx(1 hunks)src/layout/Payment/PaymentSummary.tsx(1 hunks)src/layout/RadioButtons/RadioButtonsSummary.tsx(1 hunks)src/layout/RepeatingGroup/Summary2/RepeatingGroupSummary.tsx(1 hunks)src/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx(1 hunks)src/layout/SimpleTable/ApiTableSummary.tsx(1 hunks)src/layout/SimpleTable/SimpleTableSummary.tsx(1 hunks)src/layout/Text/TextSummary.tsx(1 hunks)src/layout/TextArea/TextAreaSummary.tsx(1 hunks)src/layout/TimePicker/TimePickerSummary.tsx(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/Option/OptionSummary.tsxsrc/layout/List/ListComponent.tsxsrc/layout/Date/DateSummary.tsxsrc/layout/RepeatingGroup/Summary2/RepeatingGroupSummary.tsxsrc/layout/TimePicker/TimePickerSummary.tsxsrc/layout/Text/TextSummary.tsxsrc/layout/ImageUpload/ImageUploadSummary2/ImageUploadSummary2.tsxsrc/layout/SimpleTable/ApiTableSummary.tsxsrc/layout/Datepicker/DatepickerSummary.tsxsrc/layout/Number/NumberSummary.tsxsrc/layout/Input/InputSummary.tsxsrc/layout/Likert/Summary2/LikertSummary.tsxsrc/layout/Map/Summary2/MapSummary.tsxsrc/layout/Address/AddressSummary/AddressSummary.tsxsrc/layout/Group/GroupSummary.tsxsrc/layout/Payment/PaymentSummary.tsxsrc/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsxsrc/layout/Likert/LikertComponent.tsxsrc/layout/Accordion/SummaryAccordion.tsxsrc/layout/OrganisationLookup/OrganisationLookupSummary.tsxsrc/layout/FileUpload/AttachmentSummaryComponent2.tsxsrc/layout/Checkboxes/CheckboxesSummary.tsxsrc/layout/SimpleTable/SimpleTableSummary.tsxsrc/layout/MultipleSelect/MultipleSelectSummary.tsxsrc/layout/RadioButtons/RadioButtonsSummary.tsxsrc/layout/Grid/GridSummary.tsxsrc/layout/Dropdown/DropdownSummary.tsxsrc/layout/TextArea/TextAreaSummary.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/Option/OptionSummary.tsxsrc/layout/List/ListComponent.tsxsrc/layout/Date/DateSummary.tsxsrc/layout/RepeatingGroup/Summary2/RepeatingGroupSummary.tsxsrc/layout/TimePicker/TimePickerSummary.tsxsrc/layout/Text/TextSummary.tsxsrc/layout/ImageUpload/ImageUploadSummary2/ImageUploadSummary2.tsxsrc/layout/SimpleTable/ApiTableSummary.tsxsrc/layout/Datepicker/DatepickerSummary.tsxsrc/layout/Number/NumberSummary.tsxsrc/layout/Input/InputSummary.tsxsrc/layout/Likert/Summary2/LikertSummary.tsxsrc/layout/Map/Summary2/MapSummary.tsxsrc/layout/Address/AddressSummary/AddressSummary.tsxsrc/layout/Group/GroupSummary.tsxsrc/layout/Payment/PaymentSummary.tsxsrc/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsxsrc/layout/Likert/LikertComponent.tsxsrc/layout/Accordion/SummaryAccordion.tsxsrc/layout/OrganisationLookup/OrganisationLookupSummary.tsxsrc/layout/FileUpload/AttachmentSummaryComponent2.tsxsrc/layout/Checkboxes/CheckboxesSummary.tsxsrc/layout/SimpleTable/SimpleTableSummary.tsxsrc/layout/MultipleSelect/MultipleSelectSummary.tsxsrc/layout/RadioButtons/RadioButtonsSummary.tsxsrc/layout/Grid/GridSummary.tsxsrc/layout/Dropdown/DropdownSummary.tsxsrc/layout/TextArea/TextAreaSummary.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-14T09:01:40.985Z
Learnt from: lassopicasso
Repo: Altinn/app-frontend-react PR: 3654
File: src/layout/ImageUpload/ImageUploadSummary2/ImageUploadSummary2.tsx:49-53
Timestamp: 2025-10-14T09:01:40.985Z
Learning: In the Altinn app-frontend-react codebase, when data integrity is guaranteed through business logic and parent components guard rendering (e.g., `storedImage ? <Component /> : undefined`), non-null assertions on guaranteed fields like `storedImage!.data?.filename` are acceptable and preferred over optional chaining with fallbacks.
Applied to files:
src/layout/ImageUpload/ImageUploadSummary2/ImageUploadSummary2.tsx
🧬 Code graph analysis (4)
src/layout/Likert/Summary2/LikertSummary.tsx (1)
src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/Payment/PaymentSummary.tsx (2)
src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/layout/Payment/PaymentReceiptDetails/PaymentReceiptDetails.tsx (1)
PaymentReceiptDetails(68-197)
src/layout/FileUpload/AttachmentSummaryComponent2.tsx (1)
src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)
src/layout/MultipleSelect/MultipleSelectSummary.tsx (1)
src/features/language/Lang.tsx (1)
Lang(15-23)
⏰ 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: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (27)
src/layout/RepeatingGroup/Summary2/RepeatingGroupSummary.tsx (1)
39-39: LGTM! Consistent use of summaryTitle with fallback.The change correctly implements the pattern of preferring
summaryTitlewhen available, falling back totitle. The derived title is used consistently throughout the component.src/layout/RadioButtons/RadioButtonsSummary.tsx (1)
20-20: LGTM! Title precedence implemented correctly.The change follows the established pattern and is used appropriately at line 34 with proper undefined handling.
src/layout/List/ListComponent.tsx (1)
111-111: LGTM! Consistent summaryTitle precedence.The change is correctly implemented and the derived
titlevariable is used consistently throughout the component at lines 189, 214, and 258.src/layout/Dropdown/DropdownSummary.tsx (1)
20-20: LGTM! Correct implementation of summaryTitle precedence.The change follows the established pattern and is used properly at line 34 with appropriate undefined handling.
src/layout/Checkboxes/CheckboxesSummary.tsx (1)
24-24: LGTM! Follows the consistent pattern.The change correctly prioritizes
summaryTitleovertitleand is used appropriately in the component.src/layout/Grid/GridSummary.tsx (1)
57-57: LGTM! Correct summaryTitle precedence for grid caption.The change properly implements the pattern and the derived title is used correctly at line 152 in the table caption.
src/layout/Input/InputSummary.tsx (1)
20-20: LGTM! Proper fallback pattern for summary title.The change correctly prioritizes
summaryTitlewhen available, falling back totitle. This aligns with the PR objective and maintains backward compatibility.src/layout/TextArea/TextAreaSummary.tsx (1)
20-20: LGTM! Consistent with the summaryTitle pattern.The implementation correctly uses
summaryTitlewith fallback totitle, matching the approach used across other summary components in this PR.src/layout/Date/DateSummary.tsx (1)
20-20: LGTM! Follows the established pattern.The title resolution correctly implements the
summaryTitlepreference with proper fallback, consistent with other components in this PR.src/layout/Map/Summary2/MapSummary.tsx (1)
30-30: LGTM! Correctly implements summaryTitle preference.The change aligns with the PR-wide pattern of prioritizing
summaryTitlewith a fallback totitle.src/layout/Text/TextSummary.tsx (1)
20-20: LGTM! Consistent implementation.The title resolution properly uses
summaryTitlewith fallback totitle, maintaining consistency across the codebase.src/layout/OrganisationLookup/OrganisationLookupSummary.tsx (1)
21-21: LGTM! Aligns with the summaryTitle pattern.The implementation correctly prioritizes
summaryTitleovertitle, consistent with the changes across this PR.src/layout/Payment/PaymentSummary.tsx (1)
17-17: LGTM! Correct use of summaryTitle with fallback.The fallback pattern
summaryTitle || titleis correctly implemented, aligning with the PR objective.src/layout/TimePicker/TimePickerSummary.tsx (1)
20-20: LGTM! Proper implementation of summaryTitle precedence.The change correctly uses
summaryTitlewhen available, falling back totitle, consistent with the pattern established across this PR.src/layout/RepeatingGroup/Summary2/RepeatingGroupTableSummary/RepeatingGroupTableSummary.tsx (1)
40-40: LGTM - Consistent title resolution pattern.The change correctly prioritizes
summaryTitlewhen available, falling back totitle. This aligns with the PR objective and follows the pattern used across all summary components.src/layout/Datepicker/DatepickerSummary.tsx (1)
20-20: LGTM - Title fallback implemented correctly.src/layout/SimpleTable/SimpleTableSummary.tsx (1)
23-23: LGTM - Consistent with the broader pattern.src/layout/ImageUpload/ImageUploadSummary2/ImageUploadSummary2.tsx (1)
19-19: LGTM - Title resolution follows the established pattern.src/layout/Address/AddressSummary/AddressSummary.tsx (1)
18-19: LGTM - Triple fallback pattern is appropriate here.The Address component correctly adds
summaryTitleto the destructuring and uses a three-level fallback (summaryTitle || title || 'address_component.address'). The default fallback to'address_component.address'ensures the address field always has a meaningful label, which is appropriate for this component.Also applies to: 42-42
src/layout/Option/OptionSummary.tsx (1)
20-20: LGTM - Follows the consistent pattern.src/layout/SimpleTable/ApiTableSummary.tsx (1)
22-22: LGTM - Consistent title resolution.src/layout/Number/NumberSummary.tsx (1)
20-20: LGTM - Final file follows the consistent pattern.All 8 files have been updated consistently to prioritize
summaryTitleovertitle. The implementation is clean and aligns with the PR objective.src/layout/Likert/Summary2/LikertSummary.tsx (2)
37-37: LGTM! Consistent implementation of summaryTitle fallback.The title derivation correctly prioritizes
summaryTitleovertitleand is used consistently across all Lang component instances in LikertSummary.Also applies to: 46-46, 69-69
112-112: LGTM! Consistent pattern in LikertRowSummary.The same summaryTitle fallback pattern is correctly applied in the nested LikertRowSummary component.
Also applies to: 124-124
src/layout/Accordion/SummaryAccordion.tsx (1)
36-36: LGTM! Clean summaryTitle fallback in both components.Both
SummaryAccordionComponentandSummaryAccordionComponent2correctly implement the summaryTitle fallback pattern with proper string conversion vialangAsString.Also applies to: 63-63
src/layout/FileUpload/AttachmentSummaryComponent2.tsx (2)
19-23: LGTM! Improved type safety with useItemWhenType.The switch from
useExternalItemtouseItemWhenTypewith a type guard provides better type narrowing and will throw early if an unexpected component type is encountered. The derivation ofhasTagfrom the component type is cleaner and more explicit.
37-37: LGTM! Consistent summaryTitle pattern and cleaner property access.The direct access to
component.minNumberOfAttachmentsand the summaryTitle fallback intextResourceBindingsalign with the PR objective and simplify the code.Also applies to: 51-53
|
|
||
| const indexedId = useIndexedId(baseComponentId); | ||
| const title = textResourceBindings?.title; | ||
| const title = textResourceBindings?.summaryTitle || textResourceBindings?.title; |
There was a problem hiding this comment.
Title derivation is correct, but update aria-labelledby check.
The change at line 32 correctly implements summaryTitle precedence. However, line 64 still references textResourceBindings?.title for the aria-labelledby attribute, which creates an accessibility bug.
Issue: If summaryTitle exists but title does not, the component will display a heading (line 51 uses the derived title), but aria-labelledby will be undefined because the check at line 64 evaluates textResourceBindings?.title.
Apply this diff to fix the accessibility issue:
<div
role='group'
className={classes.likertMobileGroup}
- aria-labelledby={textResourceBindings?.title ? getLabelId(indexedId) : undefined}
+ aria-labelledby={title ? getLabelId(indexedId) : undefined}
aria-describedby={textResourceBindings?.description ? getDescriptionId(indexedId) : undefined}
>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/layout/Likert/LikertComponent.tsx around lines 32 and 64, the component
now derives title as "textResourceBindings?.summaryTitle ||
textResourceBindings?.title" (line 32) but the aria-labelledby check at line 64
still tests textResourceBindings?.title, causing aria-labelledby to be omitted
when only summaryTitle is present; change the aria-labelledby condition to
reference the derived title (or test textResourceBindings?.summaryTitle ||
textResourceBindings?.title) so the attribute is set whenever the displayed
heading exists, ensuring accessibility.
| const title = useItemWhenType(targetBaseComponentId, 'Payment').textResourceBindings?.title; | ||
| const summaryTitle = useItemWhenType(targetBaseComponentId, 'Payment').textResourceBindings?.summaryTitle; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to eliminate duplicate hook calls.
Calling useItemWhenType twice with identical arguments is inefficient and violates React best practices. The hook internally calls multiple other hooks, so this causes unnecessary duplicate computations.
Apply this diff to call the hook once:
- const title = useItemWhenType(targetBaseComponentId, 'Payment').textResourceBindings?.title;
- const summaryTitle = useItemWhenType(targetBaseComponentId, 'Payment').textResourceBindings?.summaryTitle;
+ const { textResourceBindings } = useItemWhenType(targetBaseComponentId, 'Payment');
+ const title = textResourceBindings?.title;
+ const summaryTitle = textResourceBindings?.summaryTitle;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const title = useItemWhenType(targetBaseComponentId, 'Payment').textResourceBindings?.title; | |
| const summaryTitle = useItemWhenType(targetBaseComponentId, 'Payment').textResourceBindings?.summaryTitle; | |
| const { textResourceBindings } = useItemWhenType(targetBaseComponentId, 'Payment'); | |
| const title = textResourceBindings?.title; | |
| const summaryTitle = textResourceBindings?.summaryTitle; |
🤖 Prompt for AI Agents
In src/layout/Payment/PaymentSummary.tsx around lines 9 to 10, the code calls
useItemWhenType(targetBaseComponentId, 'Payment') twice which duplicates hook
work and is inefficient; replace the two calls by calling useItemWhenType once,
store its return in a local variable (e.g., paymentItem), then derive title and
summaryTitle from paymentItem?.textResourceBindings?.title and
paymentItem?.textResourceBindings?.summaryTitle respectively, preserving the
same optional chaining/null checks.
|
1 participant
Description
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.