feat: summary2 filter override components #14470
Conversation
📝 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
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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 @@
## main #14470 +/- ##
==========================================
- Coverage 95.70% 95.70% -0.01%
==========================================
Files 1902 1902
Lines 24755 24771 +16
Branches 2833 2839 +6
==========================================
+ Hits 23692 23707 +15
- Misses 802 803 +1
Partials 261 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
linting
Jondyr
force-pushed
the
21-01-summary2-filter-override-components
branch
from
31fee1d to
a27a6e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/targetUtils.ts (1)
43-52:⚠️ Potential issueAdd null checks and improve error handling.
The function needs additional safety checks:
- The
findoperation could return undefined (same issue as previously noted)- Add validation for required properties
export const getTargetLayoutSetName = ({ target, layoutSets, selectedFormLayoutSetName, }: getTargetLayoutSetNameProps): string => { + if (!layoutSets?.sets) { + return selectedFormLayoutSetName; + } const layoutSetName = target?.taskId - ? layoutSets.sets.find((layoutSet) => layoutSet.task?.id === target.taskId).id + ? layoutSets.sets.find((layoutSet) => layoutSet.task?.id === target.taskId)?.id ?? selectedFormLayoutSetName : selectedFormLayoutSetName; return layoutSetName; };
🧹 Nitpick comments (6)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
Line range hint
1-415: Consider adding error handling test cases.While the current test coverage is good for the happy path, consider adding test cases for error scenarios:
- Test invalid query responses:
it('should handle invalid layout sets data', async () => { const queryClient = createQueryClientMock(); queryClient.setQueryData([QueryKey.LayoutSetsExtended, org, app], null); render({ target: { type: 'layoutSet' } }); // Assert error handling behavior });
- Test component behavior when target type doesn't match available options:
it('should handle mismatched target type', async () => { render({ target: { type: 'invalid' } }); // Assert appropriate error message or fallback behavior });frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (3)
26-26: Good architectural change moving to prop-based injection!Moving from internal derivation to prop-based injection of
componentOptionsimproves:
- Reusability: The component is more flexible and can be used in different contexts
- Testability: Easier to test with mock data
- Performance: Avoids redundant computations if the options are already available in the parent
43-43: Consider removing unnecessary null check.Since
componentOptionsis a required prop in the type definition (not marked as optional with?), TypeScript ensures it will always be provided. This null check is redundant and can be safely removed.- if (!componentOptions) return null;
Line range hint
46-49: Consider memoizing the component lookup.The component lookup could be memoized using
useMemoto avoid unnecessary recalculations on re-renders, especially ifcomponentOptionsis a large array.+ const selectedComponent = React.useMemo( + () => componentOptions.find((comp) => comp.id === override.componentId), + [componentOptions, override.componentId] + ); if (!open) { - const componentNameType = componentOptions.find( - (comp) => comp.id === override.componentId, - )?.description; + const componentNameType = selectedComponent?.description;frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (2)
26-26: Consider memoizing componentOptions to prevent unnecessary recalculations.The componentOptions are recalculated on every render. Consider using useMemo to optimize performance.
- const componentOptions = useTargetComponentOptions(target); + const componentOptions = React.useMemo( + () => useTargetComponentOptions(target), + [target] + );
83-108: Enhance type safety and add input validation.The hook implementation looks good but could benefit from some improvements:
- Replace
any[]with a more specific return type- Add input validation for the target parameter
Consider applying these improvements:
-const useTargetComponentOptions = (target: Summary2TargetConfig): any[] => { +interface ComponentOption { + id: string; + title: string; +} + +const useTargetComponentOptions = ( + target: Summary2TargetConfig | undefined +): ComponentOption[] => { + if (!target) return []; const { org, app } = useStudioEnvironmentParams(); const { data: layoutSets } = useLayoutSetsExtendedQuery(org, app);
📜 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)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/packages/shared/src/types/api/dto/LayoutSetModel.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2ComponentReferenceSelector.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Component.test.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Component.tsx
- frontend/language/src/nb.json
🧰 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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (14)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (4)
15-15: LGTM!The import of
layoutSetsExtendedMockis correctly added to support the new target-based component filtering functionality.
78-78: LGTM!The test case is properly updated to include the target configuration, testing the new layout set filtering functionality.
398-401: Add test cases for different target configurations.While the default props are correctly typed and initialized, consider adding test cases for different target configurations to ensure the component behaves correctly with various target types.
Consider adding tests for:
- Empty target (current case)
- Target with type 'layoutSet'
- Target with invalid type
408-408: LGTM!The render function is correctly updated to include the necessary layout sets data for testing target-based filtering.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
20-20: LGTM! Clean type-only import.The import is correctly using the type-only import syntax for better tree-shaking.
36-36: LGTM! Clean prop destructuring.The
componentOptionsprop is correctly destructured alongside other props.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/targetUtils.ts (4)
1-8: LGTM! Import statements are well-organized.The new imports for
Summary2TargetConfigandLayoutSetsModelare properly added and organized alongside existing imports.
Line range hint
9-31: LGTM! Comprehensive list of excluded components.The excluded components list appropriately filters out UI components that wouldn't make sense in a summary view.
32-41: LGTM! Well-structured type definitions.The new types
TargetPropsandgetTargetLayoutSetNamePropsare clean, reusable, and properly documented.
54-57: Verify handling of the union type in implementation.The
formLayoutsDatatype has been expanded to accept eitherIFormLayoutsorIInternalLayout[]. Please ensure that the implementation ingetComponentOptionsproperly handles both types.✅ Verification successful
Implementation correctly handles both types
The code implementation safely handles both
IFormLayoutsandIInternalLayout[]types:
- When used with
IFormLayouts,Object.values()converts the object to an array- When used with
IInternalLayout[], the array is used directly- Both cases work correctly with the subsequent
flatMapoperation🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of getComponentOptions to verify proper type handling ast-grep --pattern 'getComponentOptions({ $$$formLayoutsData: $_$$$})'Length of output: 72
Script:
#!/bin/bash # Find the getComponentOptions function implementation ast-grep --pattern 'getComponentOptions = ({ $$$formLayoutsData$$$, $_$$$}) => $_' # Also search for any file containing getComponentOptions rg "getComponentOptions" -A 5Length of output: 8352
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx (1)
37-37: LGTM! Clearing overrides when target changes prevents invalid configurations.The change ensures that component overrides are reset when the target changes, which is the correct behavior as different targets may have different valid override options.
Let's verify that this change doesn't cause any regressions:
✅ Verification successful
Clearing overrides on target change is the correct implementation
The code shows that:
- Target selection is a required main configuration
- Overrides are properly cleared when target changes to prevent invalid configurations
- Changes are properly propagated through the component hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests covering the target change behavior rg -l "handleTargetChange|target.*change" --type ts --type tsxLength of output: 92
Script:
#!/bin/bash # Search for test files and related components rg -l "handleTargetChange|targetChange" --type ts # Search for override-related patterns in the component rg -l "overrides.*\[\]|component\.overrides" --type ts # Look for test files specifically fd "test|spec" -e tsLength of output: 26097
Script:
#!/bin/bash # Check the implementation and test coverage rg "handleTargetChange" -A 5 -B 5 frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx # Look for related tests rg "target.*change.*override" frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Override.test.tsxLength of output: 1422
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (3)
2-14: LGTM! Well-organized imports with clear dependencies.The new imports are logically grouped and necessary for the target-based filtering functionality.
17-19: LGTM! Clear type definition with required target prop.The Summary2OverrideProps interface is well-defined with the new target property.
67-67: Verify integration with Summary2OverrideEntry component.The componentOptions are correctly passed to Summary2OverrideEntry, but let's verify the integration:
✅ Verification successful
✓ Integration with Summary2OverrideEntry is properly implemented
The componentOptions prop is correctly:
- Validated with null checks
- Used for component lookup and display
- Passed to Summary2ComponentReferenceSelector for selection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Summary2OverrideEntry's handling of componentOptions ast-grep --pattern 'interface $interface { $$$ componentOptions: $_ $$$ }' # Check for any existing tests covering this integration rg -l "componentOptions.*Summary2OverrideEntry" --type testLength of output: 173
Script:
#!/bin/bash # Find Summary2OverrideEntry component definition and usage ast-grep --pattern 'const Summary2OverrideEntry = $_' # Find interface or type definitions related to componentOptions ast-grep --pattern 'type $_ = { $$$ componentOptions?: $_ $$$ }' # Look for test files with proper extensions rg -l "componentOptions.*Summary2OverrideEntry" -g "*.test.tsx" -g "*.test.ts" -g "*.spec.tsx" -g "*.spec.ts"Length of output: 13593
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 inLayoutSetModel.Summary2Overridecomponent to support target configurations.Component Enhancements
Summary2Overridecomponent with target-specific logic.Testing