feat: add main config section with header behind feature flag #14702
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new feature flag ( 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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14702 +/- ##
=======================================
Coverage 95.76% 95.76%
=======================================
Files 1914 1916 +2
Lines 24948 24961 +13
Branches 2857 2858 +1
=======================================
+ Hits 23891 23904 +13
Misses 798 798
Partials 259 259 ☔ View full report in Codecov by Sentry. |
lassopicasso
changed the title
lassopicasso
changed the title
![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 (7)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx (1)
16-23: Consider a future-proof fallback for new component types.
While thisswitchis clear and readable, you may need additional cases as your component library expands. It might be good to have a fallback that explicitly handles unknown types or logs a warning to help with debugging.frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/HeaderMainConfig.tsx (1)
7-9: Avoid usingPartialwhen props are already optional.
Sincechildrenis already optional (children?: React.ReactNode;), wrapping the prop type inPartialis redundant. Consider removingPartial<HeaderMainConfigProps>to simplify the prop signature.-export const HeaderMainConfig = ({ children }: Partial<HeaderMainConfigProps>): JSX.Element => { +export const HeaderMainConfig = ({ children }: HeaderMainConfigProps): JSX.Element => {frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.test.tsx (1)
29-43: Add more test cases for feature flag scenarios.While the current tests cover basic rendering scenarios, consider adding tests for:
- Feature flag disabled state
- Edge cases when switching feature flag state
- Different component types with feature flag
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/SpecificMainConfig/SummaryMainConfig.tsx (3)
19-21: Consider using useReducer for accordion state.The current implementation uses useState with a Record type. For more complex state management, useReducer might be clearer and more maintainable.
- const [accordionOpen, setAccordionOpen] = React.useState<Record<string, boolean>>({}); + const [accordionState, dispatch] = React.useReducer(accordionReducer, {});
23-34: Add error handling for component updates.The component change handlers should include error handling for invalid states and logging for debugging purposes.
const handleTargetChange = (updatedTarget: Summary2TargetConfig): void => { + try { const updatedComponent = { ...component } as FormItem<ComponentType.Summary2>; updatedComponent.target = updatedTarget; updatedComponent.overrides = []; handleComponentChange(updatedComponent); + } catch (error) { + console.error('Failed to update target:', error); + // Consider adding error notification + } };
41-60: Enhance accordion accessibility.Consider adding aria-labels and keyboard navigation improvements to the accordion component.
<Accordion.Item open={accordionOpen['summary2overrides'] === true}> <Accordion.Header + aria-label={t('ux_editor.component_properties.summary.override.aria_label')} onHeaderClick={() => setAccordionOpen((prev) => { return { ...prev, summary2overrides: !prev['summary2overrides'] }; }) } >frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/SpecificMainConfig/SummaryMainConfig.test.tsx (1)
32-38: Add assertions for initial state.The test should verify the initial closed state of the accordion before clicking.
it('should render summary2 config', async () => { const user = userEvent.setup(); render(summary2Component); + expect(screen.queryByRole('button', { name: /add_override/ })).not.toBeInTheDocument(); expect(summary2AccordionButton()).toBeInTheDocument(); await user.click(summary2AccordionButton()); expect(summary2AddOverrideButton()).toBeInTheDocument(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/packages/shared/src/utils/featureToggleUtils.ts(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx(2 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/HeaderMainConfig.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx(2 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/SpecificMainConfig/SummaryMainConfig.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/SpecificMainConfig/SummaryMainConfig.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/shared/src/utils/featureToggleUtils.ts
🔇 Additional comments (8)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx (1)
4-5: Modular imports for specialized components look great.
The extraction of specialized components (SummaryMainConfigvs.HeaderMainConfig) is a clean and maintainable approach.frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/HeaderMainConfig.tsx (1)
11-23: Nicely implemented translated heading structure.
This straightforward layout usingStudioHeading,RequiredIndicator, anduseTranslationeffectively localizes the heading.frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx (2)
14-14: Importing feature flag utilities is well-structured.
No concerns here. This aligns with standard usage of the feature toggle utility.
54-57:✅ Verification successful
Conditional rendering for feature flag usage looks good.
It’s a neat approach to guardComponentMainConfigbehindFeatureFlag.MainConfig.Would you like to verify all references to
FeatureFlag.MainConfigto ensure a consistent setup across environments? If so, you can run:
🏁 Script executed:
#!/bin/bash # Description: Locate all references to FeatureFlag.MainConfig in the repository for cross-check. ast-grep --pattern $'FeatureFlag.MainConfig'Length of output: 7575
Conditional rendering for FeatureFlag.MainConfig verified.
The usage infrontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx(lines 54–57) correctly guards theComponentMainConfigcomponent with a check onFeatureFlag.MainConfig. The corresponding test inComponentMainConfig.test.tsx(line 37) confirms that the feature flag is set up as expected. Although the ast-grep command returned several matches in backend SQL scripts, these appear unrelated to the frontend flag configuration. No further adjustments are necessary.frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.test.tsx (2)
24-27: LGTM! Good cleanup practice.The afterEach cleanup properly handles both Jest mocks and feature flags in localStorage, ensuring a clean state for each test.
46-54: LGTM! Well-structured test helper.The renderComponentMainConfig helper function improves test maintainability by centralizing the setup logic.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/SpecificMainConfig/SummaryMainConfig.tsx (1)
14-17: LGTM! Well-typed props interface.The SummaryMainConfigProps type ensures type safety for the component props.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/SpecificMainConfig/SummaryMainConfig.test.tsx (1)
68-78: LGTM! Well-organized helper functions.The helper functions improve test readability and maintenance by centralizing element queries.
...tor/src/components/Properties/PropertiesHeader/SpecificMainConfig/SummaryMainConfig.test.tsx
Show resolved
Hide resolved
Description
This PR introduces a new feature flag,
mainConfig, which controls the rendering of the main section. Currently, it renders only the main section header.Some refactoring has been done to simplify future implementations for this section while ensuring that the new summary component, which is already part of the main configuration section, remains unaffected.
It may look like I made more changes than I actually did. I tried using
git mvto move files without them appearing as entirely new changes, but after reusing some file names, it didn’t go as planned. Sorry about that to the reviewer! 😉Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Refactor
Tests