feat: summary2 filter override components #14470
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces changes across multiple frontend components related to target configuration and localization. The modifications primarily focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
area/ui-editor
Jondyr
force-pushed
the
21-01-summary2-filter-override-components
branch
from
e987773 to
aee4443
Compare
Jondyr
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 20-01-summary2-reorganize-config #14470 +/- ##
===================================================================
Coverage ? 95.68%
===================================================================
Files ? 1894
Lines ? 24655
Branches ? 2827
===================================================================
Hits ? 23592
Misses ? 802
Partials ? 261 ☔ View full report in Codecov by Sentry. |
Jondyr
force-pushed
the
20-01-summary2-reorganize-config
branch
from
429634d to
7373c87
Compare
Jondyr
force-pushed
the
21-01-summary2-filter-override-components
branch
from
aee4443 to
0192d05
Compare
Jondyr
added
team/studio-domain2
text/content
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (5)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
43-43: Enhance error handling for better developer experience.The null check is too simplistic and might make debugging difficult. Consider:
- Adding a console warning in development
- Rendering a fallback UI with an error message
- Adding runtime validation for empty arrays
- if (!componentOptions) return null; + if (!componentOptions?.length) { + if (process.env.NODE_ENV === 'development') { + console.warn('Summary2OverrideEntry: componentOptions is empty or undefined'); + } + return ( + <StudioAlert severity='warning'> + {t('ux_editor.component_properties.summary.override.no_options')} + </StudioAlert> + ); + }
Line range hint
46-49: Add validation for component existence.The find operation could return undefined if the component ID doesn't exist in the options. Consider adding validation and user feedback.
- const componentNameType = componentOptions.find( - (comp) => comp.id === override.componentId, - )?.description; + const component = componentOptions.find( + (comp) => comp.id === override.componentId, + ); + if (!component) { + if (process.env.NODE_ENV === 'development') { + console.warn(`Component with ID ${override.componentId} not found in options`); + } + return ( + <StudioAlert severity='warning'> + {t('ux_editor.component_properties.summary.override.component_not_found')} + </StudioAlert> + ); + } + const componentNameType = component.description;frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2ComponentReferenceSelector.tsx (1)
33-35: Consider adding aria-live for accessibility.The empty state message improves UX, but consider adding
aria-live="polite"to ensure screen readers announce when the list becomes empty.- <StudioCombobox.Empty> + <StudioCombobox.Empty aria-live="polite">frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/targetUtils.ts (1)
54-57: Consider adding type guard for formLayoutsData.The union type
IFormLayouts | IInternalLayout[]might cause type confusion. Consider adding a type guard to safely handle both types.const isFormLayouts = (data: IFormLayouts | IInternalLayout[]): data is IFormLayouts => { return !Array.isArray(data); };frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
83-83: Consider adding proper return type instead of any[].The hook's return type should be explicitly defined to improve type safety and documentation.
-const useTargetComponentOptions = (target: Summary2TargetConfig): any[] => { +type ComponentOption = { id: string; title: string }; +const useTargetComponentOptions = (target: Summary2TargetConfig): ComponentOption[] => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/language/src/nb.json(1 hunks)frontend/packages/shared/src/types/api/dto/LayoutSetModel.ts(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx(3 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx(3 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Component.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Component.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2ComponentReferenceSelector.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/targetUtils.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
Learnt from: Jondyr
PR: Altinn/altinn-studio#14379
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx:63-70
Timestamp: 2025-01-13T12:44:45.751Z
Learning: In the Altinn Studio codebase, when using StudioProperty.Button component, it's expected to pass `false` as the value prop when no meaningful value is available (e.g., when componentNameType is undefined). This is the intended behavior and should not be changed to handle undefined cases differently.
🔇 Additional comments (10)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (3)
20-20: LGTM! Clean type import addition.The import of
TargetPropstype is well-organized and follows the existing import structure.
Line range hint
23-30: LGTM! Props type is well-defined.The addition of
componentOptionsas a required prop is appropriate, ensuring type safety for the component options list.
Line range hint
32-40: LGTM! Clean parameter destructuring.The addition of
componentOptionsto the component parameters is consistent with the type definition.frontend/packages/shared/src/types/api/dto/LayoutSetModel.ts (1)
7-7: Verify the impact of makingtaskoptional.Making the
taskproperty optional is a breaking change that could affect type checking across the codebase. This change needs careful verification to ensure it doesn't introduce runtime errors in existing code that assumestaskis always defined.Let's verify the usage of
LayoutSetModel.taskacross the codebase:✅ Verification successful
Making
taskoptional aligns with existing code patternsThe codebase already handles the
taskproperty as potentially undefined through consistent use of optional chaining (?.). All access tolayoutSet.taskis properly guarded, making this type change safe and aligned with the existing implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct access to the task property of LayoutSetModel rg -t typescript "\.task\b" --glob "frontend/**/*.{ts,tsx}" # Search for LayoutSetModel type usage rg -t typescript "LayoutSetModel" --glob "frontend/**/*.{ts,tsx}"Length of output: 197
Script:
#!/bin/bash # Search for direct access to the task property rg "\.task\b" --glob "frontend/**/*.{ts,tsx}" -B 2 -A 2 # Search for LayoutSetModel type usage rg "LayoutSetModel" --glob "frontend/**/*.{ts,tsx}" -B 2 -A 2 # Search for type definitions and interfaces extending LayoutSetModel rg "extends.*LayoutSetModel|type.*=.*LayoutSetModel" --glob "frontend/**/*.{ts,tsx}" -B 2 -A 2Length of output: 5683
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Component.tsx (1)
33-33: LGTM! Props are correctly passed to Summary2Override.The addition of the
targetprop is well-typed and properly integrated with the component's existing props.frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx (1)
65-65: LGTM! Proper prop passing implementation.The target prop is correctly passed from the component to Summary2Override.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
83-108: LGTM! Well-structured hook implementation.The hook effectively filters component options based on target type, with proper error handling and edge cases covered.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Component.test.tsx (1)
195-195: LGTM! Proper test setup with mock data.The test environment is correctly configured with the new LayoutSetsExtended query data.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
398-402: LGTM! Comprehensive test coverage with proper typing.The test suite is well-maintained with:
- Properly typed props
- Complete mock data setup
- Comprehensive test scenarios
Also applies to: 408-408
frontend/language/src/nb.json (1)
1489-1491: Well-structured translations added!The new translation keys follow good practices:
- Clear and concise error messages in Norwegian
- Consistent with existing UI terminology
- Properly namespaced under component properties
...editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/targetUtils.ts
Show resolved
Hide resolved
Jondyr
force-pushed
the
20-01-summary2-reorganize-config
branch
from
7373c87 to
aa848ad
Compare
Filters by applicable components I.e. If target is a layoutset, show only components from that layoutset
Jondyr
force-pushed
the
21-01-summary2-filter-override-components
branch
from
0192d05 to
31fee1d
Compare
Description
Summary2 overrides now filter component options. It should now display only components relevant to the summary target.
summary2-override-components-filter.mp4
PR stack
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Localization
Type Changes
taskproperty optional inLayoutSetModelSummary2Overridecomponent to support target configurationsComponent Enhancements
Summary2Overridecomponent with target-specific logicTesting