fix: rely on component type instead of its ID when display override config #14545
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes to the Summary2 component's type definitions and prop handling across multiple files. The modifications primarily focus on renaming 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
lassopicasso
changed the title
lassopicasso
added
the
skip-manual-testing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx (1)
25-27: LGTM! Improved component type checking.The implementation now correctly relies on the component's type rather than its ID for determining display behavior. This change makes the logic more robust when component IDs change.
- const checkboxOrMultipleselect = - selectedComponentType?.includes(ComponentType.MultipleSelect) || - selectedComponentType?.includes(ComponentType.Checkboxes); + const checkboxOrMultipleselect = [ + ComponentType.MultipleSelect, + ComponentType.Checkboxes + ].includes(selectedComponentType);Consider this minor optimization to make the type checking more concise and efficient.
Also applies to: 30-31
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/targetUtils.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (7)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/targetUtils.ts (3)
32-35: LGTM! Clear separation of page and component types.The introduction of separate
TargetPagePropsandTargetComponentPropstypes provides a clear distinction between page and component references, with components explicitly including their type information.Also applies to: 37-39
66-66: LGTM! Component type is now included in options.The
getComponentOptionsfunction now includes the component type in its return value, which is crucial for the new type-based display logic.Also applies to: 76-76
Line range hint
80-86: LGTM! Consistent type usage for page options.The
getPageOptionsfunction now correctly uses the renamedTargetPagePropstype, maintaining consistency with the new type system.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
20-20: LGTM! Updated type imports and props.The component now correctly uses
TargetComponentPropstype for component options, ensuring type information is available.Also applies to: 26-26
79-83: LGTM! Component options properly passed to child.The
Summary2OverrideDisplayTypecomponent now receives the necessary component options for type-based display logic.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx (2)
11-11: LGTM! Added necessary type imports and props.The component correctly imports and uses
TargetComponentPropstype for the new componentOptions prop.Also applies to: 15-15
37-41: LGTM! Handler maintains functionality.The
handleCustomTypeChangefunction maintains its functionality while working with the new type-based approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (2)
155-182: Consider consolidating duplicate test logic.The tests for overriding display type for multiple select and checkbox components contain similar logic. Consider using
it.eachto reduce duplication, similar to how you've done it in the earlier test.- it('should be able to override display type when component type is multiple select', async () => { - const user = userEvent.setup(); - render({ overrides: [{ componentId: multipleSelectId }] }); - - await user.click(overrideCollapsedButton(1)); - const optionStringType = screen.getByRole('option', { - name: textMock('ux_editor.component_properties.summary.override.display_type.string'), - }); - await user.selectOptions(overrideTypeSelect(), optionStringType); - - expect(defaultProps.onChange).toHaveBeenCalledWith( - expect.arrayContaining([{ componentId: multipleSelectId, displayType: 'string' }]), - ); - }); - - it('should be able to override display type when component type is checkbox', async () => { - const user = userEvent.setup(); - render({ overrides: [{ componentId: checkBoxId }] }); - await user.click(overrideCollapsedButton(1)); - const optionStringType = screen.getByRole('option', { - name: textMock('ux_editor.component_properties.summary.override.display_type.string'), - }); - await user.selectOptions(overrideTypeSelect(), optionStringType); - - expect(defaultProps.onChange).toHaveBeenCalledWith( - expect.arrayContaining([{ componentId: checkBoxId, displayType: 'string' }]), - ); - }); + it.each([ + ['multiple select', multipleSelectId], + ['checkbox', checkBoxId], + ])('should be able to override display type when component type is %s', async (_, componentId) => { + const user = userEvent.setup(); + render({ overrides: [{ componentId }] }); + + await user.click(overrideCollapsedButton(1)); + const optionStringType = screen.getByRole('option', { + name: textMock('ux_editor.component_properties.summary.override.display_type.string'), + }); + await user.selectOptions(overrideTypeSelect(), optionStringType); + + expect(defaultProps.onChange).toHaveBeenCalledWith( + expect.arrayContaining([{ componentId, displayType: 'string' }]), + ); + });
270-277: Enhance error reporting in the helper function.The
renderedTypeOptionsfunction could provide better feedback when assertions fail. Consider returning an object with both the result and any missing options.const renderedTypeOptions = () => { const expectedOptions = ['list', 'string', 'not_set'].map((type) => textMock(`ux_editor.component_properties.summary.override.display_type.${type}`), ); const renderedOptions = screen.getAllByRole('option').map((option) => option.textContent); - return expectedOptions.every((option) => renderedOptions.includes(option)); + const missingOptions = expectedOptions.filter((option) => !renderedOptions.includes(option)); + return { + isComplete: missingOptions.length === 0, + missingOptions, + toString: () => missingOptions.length === 0 ? 'All options present' : `Missing options: ${missingOptions.join(', ')}`, + }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx(10 hunks)frontend/packages/ux-editor/src/testing/layoutMock.ts(2 hunks)
🔇 Additional comments (2)
frontend/packages/ux-editor/src/testing/layoutMock.ts (1)
56-64: LGTM! Well-structured mock data.The new multiple select mock component follows the established patterns and maintains consistency with other mock components in the file.
Also applies to: 84-84
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
46-57: LGTM! Great use of parameterized testing.The use of
it.eachreduces code duplication and makes the tests more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
152-179: Consider consolidating duplicate tests using parameterized testing.The tests for multiple select and checkbox display type override are nearly identical. Consider refactoring them using
it.eachsimilar to the type options test to reduce code duplication.- it('should be able to override display type when component type is multiple select', async () => { - const user = userEvent.setup(); - render({ overrides: [{ componentId: multipleSelectId }] }); - - await user.click(overrideCollapsedButton(1)); - const optionStringType = screen.getByRole('option', { - name: textMock('ux_editor.component_properties.summary.override.display_type.string'), - }); - await user.selectOptions(overrideTypeSelect(), optionStringType); - - expect(defaultProps.onChange).toHaveBeenCalledWith( - expect.arrayContaining([{ componentId: multipleSelectId, displayType: 'string' }]), - ); - }); - - it('should be able to override display type when component type is checkbox', async () => { - const user = userEvent.setup(); - render({ overrides: [{ componentId: checkBoxId }] }); - await user.click(overrideCollapsedButton(1)); - const optionStringType = screen.getByRole('option', { - name: textMock('ux_editor.component_properties.summary.override.display_type.string'), - }); - await user.selectOptions(overrideTypeSelect(), optionStringType); - - expect(defaultProps.onChange).toHaveBeenCalledWith( - expect.arrayContaining([{ componentId: checkBoxId, displayType: 'string' }]), - ); - }); + it.each([ + ['multiple select', multipleSelectId], + ['checkbox', checkBoxId], + ])('should be able to override display type when component type is %s', async (_, componentId) => { + const user = userEvent.setup(); + render({ overrides: [{ componentId }] }); + + await user.click(overrideCollapsedButton(1)); + const optionStringType = screen.getByRole('option', { + name: textMock('ux_editor.component_properties.summary.override.display_type.string'), + }); + await user.selectOptions(overrideTypeSelect(), optionStringType); + + expect(defaultProps.onChange).toHaveBeenCalledWith( + expect.arrayContaining([{ componentId, displayType: 'string' }]), + ); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (4)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (4)
11-12: LGTM! Clear mock component setup.The mock components and their corresponding ID constants are well-organized and properly imported.
Also applies to: 20-21
46-55: Well-structured parameterized test!Good use of
it.eachto test type options rendering for both component types, reducing code duplication while maintaining clear test cases.
Line range hint
57-65: LGTM! Good negative test case.Properly verifies that type selector is not rendered for unsupported component types using appropriate query methods.
262-274: LGTM! Well-structured helper functions.The helper functions are focused, well-named, and use appropriate testing library queries. The
renderedTypeOptionsfunction provides a clean way to verify all expected options.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14545 +/- ##
=======================================
Coverage 95.71% 95.71%
=======================================
Files 1904 1904
Lines 24801 24803 +2
Branches 2840 2840
=======================================
+ Hits 23738 23740 +2
Misses 802 802
Partials 261 261 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (4)
Line range hint
64-72: Consider enhancing the negative test case.While the test correctly verifies that the type selector doesn't appear for unsupported components, consider:
- Using a more descriptive component ID (e.g.,
textComponentId) instead ofcomponent1IdMock- Adding more component types to verify (e.g., textfield, radio, etc.)
79-79: Consider extracting test constants and helper functions.The repeated use of
overrideCollapsedButton(1)across multiple tests suggests an opportunity for improvement:
- Extract the override index as a named constant (e.g.,
FIRST_OVERRIDE_INDEX = 1)- Create a helper function that combines common operations (e.g.,
expandFirstOverride())Example:
const FIRST_OVERRIDE_INDEX = 1; const expandOverride = async (index: number) => { await userEvent.click(overrideCollapsedButton(index)); };Also applies to: 94-94, 110-110, 128-128, 145-145
159-185: Consider parameterizing display type override tests.The tests for checkbox and multiple select components have similar structure. Consider:
- Using
it.eachto test both component types- Testing all available display types, not just 'string'
- Extracting the option selection logic into a helper function
Example:
it.each([ { componentId: checkBoxId, type: 'checkbox' }, { componentId: multipleSelectId, type: 'multipleSelect' } ])('should override display type for $type component', async ({ componentId }) => { const user = userEvent.setup(); render({ overrides: [{ componentId }] }); await user.click(overrideCollapsedButton(1)); for (const displayType of ['string', 'list']) { const option = screen.getByRole('option', { name: textMock(`ux_editor.component_properties.summary.override.display_type.${displayType}`), }); await user.selectOptions(overrideTypeSelect(), option); expect(defaultProps.onChange).toHaveBeenCalledWith( expect.arrayContaining([{ componentId, displayType }]), ); } });
269-281: Consider making helper functions more flexible.The helper functions are well-structured, but
renderedTypeOptionscould be improved:
- Accept expected types as a parameter instead of hardcoding them
- Return the actual options for more detailed assertions
- Add JSDoc comments to document the purpose and usage
Example:
/** * Verifies that the rendered type options match the expected options. * @param expectedTypes - Array of expected type keys (e.g., ['list', 'string']) * @returns Object containing actual options and a match result */ const getRenderedTypeOptions = (expectedTypes: string[] = ['list', 'string', 'not_set']) => { const expectedOptions = expectedTypes.map((type) => textMock(`ux_editor.component_properties.summary.override.display_type.${type}`) ); const renderedOptions = screen.getAllByRole('option').map((option) => option.textContent); return { renderedOptions, allExpectedOptionsPresent: expectedOptions.every((option) => renderedOptions.includes(option)) }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx(11 hunks)frontend/packages/ux-editor/src/testing/layoutMock.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/ux-editor/src/testing/layoutMock.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Testing
🔇 Additional comments (3)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (3)
11-28: LGTM! Clean test data setup.The mock setup is well-structured and follows good testing practices by reusing and extending existing mock objects.
53-62: Great use of parameterized testing!The use of
it.eachreduces code duplication and makes the test cases more maintainable. This approach makes it easy to add more component types in the future.
291-291: LGTM! Necessary update to support new tests.The render function is correctly updated to use the new layout mock with multiple select component.
Description
This PR fix the override config for checkbox and multiple select components when parts of the config is not displayed after editing the component's id.
There were some incorrect tests in
Summary2Overridethat passed for reasons unrelated to what was actually being tested. I also removed some redundant tests and refactored others.Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Summary by CodeRabbit
Type Updates
TargetComponentPropstype with enhanced component informationTargetPropstoTargetPagePropsComponent Enhancements
Summary2OverrideDisplayTypeto includecomponentOptionspropSummary2OverrideEntryto utilize newcomponentOptionspropUtility Function Updates
getComponentOptionsandgetPageOptionsto return more detailed component informationTesting Improvements
Summary2Override