fix: tablecolumns saving/deleting a column affects other columns #14561
Conversation
github-actions
bot
added
area/ui-editor
📝 WalkthroughWalkthroughThis pull request alters the UX editor's handling of column editing by replacing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 (
|
![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 (2)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (2)
62-63: Consider adding fallback text for missing text resources.The text resource handling could be improved by adding a more descriptive fallback when the text resource is not found.
- const textResourceValue = - getValueOfTitleId(tableColumn.headerContent, textResources) || tableColumn.headerContent; + const textResourceValue = + getValueOfTitleId(tableColumn.headerContent, textResources) || + tableColumn.headerContent || + t('ux_editor.properties_panel.subform_table_columns.untitled_column');
83-91: LGTM! Button behaviors are now more consistent.The changes improve the component's interaction model:
- Save button disabled state is simpler
- Close and delete actions are properly separated
However, there's a minor UX inconsistency:
- <StudioActionCloseButton + <StudioActionCloseButton variant='secondary' onClick={onClose} - title={t('general.save')} + title={t('general.close')} disabled={isSaveButtonDisabled} />The button uses a close icon but has "Save" as its title, which might confuse users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (8)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (2)
26-33: LGTM! Props interface changes improve clarity and follow React conventions.The changes to the props interface are well-thought-out:
- Renaming
sourceColumntotableColumnis more descriptive- Using
onChangeinstead ofonEditfollows standard React conventions- Adding
onCloseimproves separation of concerns
49-60: LGTM! Simplified state management improves maintainability.The
selectComponentfunction now directly updates the parent throughonChangeinstead of managing local state, which is a cleaner approach.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx (3)
11-18: LGTM! Props interface update follows React conventions.The change from
onEdittoonChangebetter aligns with React conventions and makes the component's API more intuitive.
39-48: LGTM! Clean integration with EditColumnElement.The props are passed consistently, and the component maintains a clear parent-child relationship.
33-35: Consider handling undefined textResources case.While the text key handling is cleaner, consider adding a null check for textResources to handle the case where the query hasn't completed yet.
✅ Verification successful
Review on Text Resources Handling – Consistent and Safe Pattern
The code’s use of optional chaining with a fallback value is applied consistently across the codebase. In
ColumnElement.tsx, iftextResourcesis undefined, the expression returns undefined and then falls back totableColumn.headerContent. While adding an explicit null check might improve clarity, the existing pattern safely handles the undefined case as seen in other modules (e.g., ineditSubformTableColumnsUtils.tsandPageConfigPanel.tsx).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if textResources is handled consistently across the codebase rg -A 2 'textResourceByLanguageAndIdSelector\('Length of output: 3348
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx (1)
70-70: LGTM! Consistent prop usage.The change to use
onChangemaintains consistency with the updated ColumnElement API.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (1)
42-80: LGTM! Comprehensive test coverage for the new onChange behavior.The tests thoroughly cover both component selection and manual content changes, ensuring the new
onChangebehavior works as expected.Also applies to: 82-120
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx (1)
94-96: LGTM! More precise test assertion.The updated assertion using the actual mock data makes the test more reliable and specific.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14561 +/- ##
==========================================
- Coverage 95.74% 95.74% -0.01%
==========================================
Files 1908 1908
Lines 24915 24885 -30
Branches 2848 2847 -1
==========================================
- Hits 23856 23825 -31
Misses 799 799
- Partials 260 261 +1 ☔ View full report in Codecov by Sentry. |
Jondyr
force-pushed
the
spr/main/2ef223ec
branch
from
9825a74 to
d01d109
Compare
Jondyr
force-pushed
the
spr/main/2ef223ec
branch
from
d01d109 to
ed17db3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (1)
62-66: Consider memoizingtextResourceValue.Since
textResourceValueis computed on every render and depends on external data, consider memoizing it usinguseMemoto optimize performance.- const textResourceValue = - getValueOfTitleId(tableColumn.headerContent, textResources) || tableColumn.headerContent; + const textResourceValue = useMemo( + () => getValueOfTitleId(tableColumn.headerContent, textResources) || tableColumn.headerContent, + [tableColumn.headerContent, textResources] + );frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (1)
82-120: Consider extracting repeated selectors.The test is comprehensive but could be improved by extracting repeated selectors into constants to reduce duplication and improve maintainability.
+const SELECTORS = { + columnTitleEdit: textMock('ux_editor.properties_panel.subform_table_columns.column_title_edit'), + columnTitleUnedit: textMock('ux_editor.properties_panel.subform_table_columns.column_title_unedit'), + saveButton: textMock('general.save'), +}; it('should call onEdit with updated header content on manual change', async () => { // ... await user.click( await screen.findByText( - textMock('ux_editor.properties_panel.subform_table_columns.column_title_unedit'), + SELECTORS.columnTitleUnedit, ), ); // ... similar changes for other selectors });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (5)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (3)
26-33: LGTM! Props follow React best practices.The prop changes improve clarity and follow React conventions:
- Renaming to
tableColumnbetter describes the prop's purpose- Using
onChangealigns with React's controlled component pattern- Adding
onCloseproperly separates concerns
49-60: LGTM! Function follows unidirectional data flow.The function is concise and properly updates the parent component through
onChange.
76-91: LGTM! Component properly handles user interactions.The changes correctly wire up the new callbacks and maintain consistency with the updated prop structure.
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (2)
28-40: LGTM! Test setup follows best practices.The changes improve test reliability:
- Using
beforeEachensures a clean state for each test- Props correctly reflect the component's new API
42-80: LGTM! Test case properly verifies component selection.The test effectively validates that selecting a component triggers the correct callback with the expected data.
There was a problem hiding this comment.
Great work! The code looks good. After testing, I have some thoughts on the current behavior.
The save button no longer serves its original purpose and now acts as a close button for edit mode. Since changes are saved automatically, I don’t strongly disagree with this behavior, but we should align on this as a general approach for future tasks. My initial reasoning for having a save button was that we have multiple form elements—selector and column title - such as in recommended next action when creating a new subform.
Saving on change introduces an issue: clearing the input immediately removes the column title. The logic relies on headerContent to display both the title and cell content, which was previously managed by state. This also makes the input field’s error message redundant. Watch video below.
Since this PR removes existing custom text resource keys, we should consider creating an issue to introduce a custom text key, with consistent design, later on. This would ensure consistency with other text configurations that use dedicated keys for multilingual support.
We could also discuss this with the rest of the team.
column-title.mp4
|
@lassopicasso Thanks for the feedback!
I get what you mean, we could definitely gain some consistency for buttons. I'm not sure if anything else than "save" really works here though, but it would be an interesting discussion.
Good catch, it seems the logic for deciding if a component was already chosen is if
Yes i agree, i believe we should have a design/UX issue for this first? If we do not want to just use the text-resource component. |
I’m unsure about this. In my opinion, this solution worsens the behavior when the title is cleared, as elements disappear without an error message (due to removing the state and saving changes regardless of the title being empty), so I don’t think it should be implemented as it is. If the use of After further testing and reflection, I’m also unsure if removing the logic for text resources is the best approach for handling the issue of saving/deleting columns. This concern stems from how I understand team apps' config, where There are also additional considerations from Studio's perspective:
Instead, I think we could temporarily show the user that a new text key has been created (e.g., "ID: someTextKey" below the title field, similar to how other text elements are designed) and then create a new issue to improve the design later on, ensuring consistency with other text elements in the config. I’ve asked @mlqn if he could take a second look at this PR. |
I agree that using the text resources here is the ideal solution. My original thought was to limit the scope of this PR to fix the bug and refactor/simplify the code, and then we could iterate, adding support for text resources. As in the existing state it was a bit limiting and confusing. But I understand that this maybe feels more of a side-step than a step forward on the user-side. So i'll update this PR with a solution to both the bug and add support for text resource editing. |
Jondyr
force-pushed
the
spr/main/2ef223ec
branch
2 times, most recently
from
75a5eae to
ea04d5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (2)
6-6: Useimport typefor type-only imports.According to the linting rule (
@typescript-eslint/consistent-type-imports), prefer usingimport type { TableColumn }instead ofimport { TableColumn }here, as it is only referenced as a type.Apply the following diff:
-import { TableColumn } from '../../types/TableColumn'; +import type { TableColumn } from '../../types/TableColumn';🧰 Tools
🪛 ESLint
[error] 6-6: All imports in the declaration are only used as types. Use
import type.(@typescript-eslint/consistent-type-imports)
🪛 GitHub Check: Typechecking and linting
[failure] 6-6:
All imports in the declaration are only used as types. Useimport type
42-42: Consider fallback handling fortableColumn.cellContent.query.If
cellContentcan ever be undefined or null, referencingcellContent.querymight cause an error.You could defensively access
cellContent?.queryor ensure the column definition always sets a default value.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx (1)
33-35: Hard-coded language might limit localization.The code forces the language
'nb'when selecting text resources. Consider using the current language fromuseTranslation()or a configuration-based approach for better localizability.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (1)
82-82: Rename button label to “Close” for clarity.Currently, the button label is pulled from
'general.save', but it only invokesonClose. Updating the translation key to something like'general.close'may prevent user confusion.- title={t('general.save')} + title={t('general.close')}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx(5 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx
🧰 Additional context used
🪛 ESLint
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx
[error] 6-6: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
🪛 GitHub Check: Typechecking and linting
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx
[failure] 6-6:
All imports in the declaration are only used as types. Use import type
⏰ 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 (9)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (1)
28-30: Ensure setting an emptyheaderContentis intended behavior.Currently, removing text resources resets
headerContentto an empty string. Verify that this will not cause undesired behavior, such as rendering blanks in the UI when the user clears the field.Would you like to confirm the downstream usage of
headerContentto ensure an empty string is always handled gracefully?frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx (1)
41-45: Exiting edit mode without a final callback is consistent with auto-saving logic.It’s clear that
onClosesimply toggles off the editing state without invoking an explicit save function. If automatic saving is the intended mechanism, this approach appears consistent.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (3)
25-29: Renaming props fromonEdittoonChangeand addingonCloseis well-structured.The new props better reflect the component’s use case and help clarify responsibilities.
50-56: Straightforward update logic.The procedure of constructing a new
TableColumnobject and callingonChangeis clear and maintains immutability.
61-61: Confirm disabling save button for empty header content.Enforcing a non-empty
headerContentfor saving may disallow intentionally empty column headers. If that’s acceptable, this check is fine. Otherwise, consider changing this condition.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (4)
19-20: LGTM! Props and mocks updated to support new text resource handling.The changes align with the PR objectives of simplifying state management and text resource handling.
Also applies to: 35-35
44-76: LGTM! Test case updated to verify new component selection behavior.The test properly validates that selecting a component updates both cell content and header content appropriately.
163-165: LGTM! Text resources mocking setup added.The mocking setup properly supports testing the new text resource handling functionality.
78-118: Add test cases for edge cases in header content handling.Based on the PR comments discussion, consider adding test cases to verify:
- Behavior when clearing input (mentioned as potentially confusing)
- Treatment of non-existent text keys as raw text
Example test case structure:
it('should treat non-existent text key as raw text', async () => { const onChangeMock = jest.fn(); const rawText = 'Custom Header'; // Test setup... // Enter raw text that's not a text key // Verify it's treated as raw text in onChange callback }); it('should handle empty header content appropriately', async () => { const onChangeMock = jest.fn(); // Test setup... // Clear the header content // Verify the expected behavior in onChange callback });
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (1)
6-6: Convert TableColumn import to type import.Use
import typefor type-only imports to improve build-time optimization.-import { TableColumn } from '../../types/TableColumn'; +import type { TableColumn } from '../../types/TableColumn';🧰 Tools
🪛 ESLint
[error] 6-6: All imports in the declaration are only used as types. Use
import type.(@typescript-eslint/consistent-type-imports)
🪛 GitHub Check: Typechecking and linting
[failure] 6-6:
All imports in the declaration are only used as types. Useimport typefrontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (1)
44-44: Update test description to match new behavior.The test description still mentions
onEditbut testsonChangebehavior.- it('should call onEdit with component values when selecting component', async () => { + it('should call onChange with component values when selecting component', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx(5 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx
🧰 Additional context used
🪛 ESLint
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx
[error] 6-6: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
🪛 GitHub Check: Typechecking and linting
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx
[failure] 6-6:
All imports in the declaration are only used as types. Use import type
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CodeQL
- GitHub Check: Testing
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (6)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (2)
10-14: LGTM! Props refactor improves state management.The removal of local state management (
title,setTitle) in favor of directtableColumnprop usage aligns with the PR objective of removing unnecessary state that could affect other columns.Also applies to: 16-20
26-36: LGTM! TextResource component improves text key handling.The TextResource component provides better handling of text keys and raw text, addressing the PR objective of eliminating dependence on custom text resource keys.
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (3)
24-31: LGTM! Props refactor improves clarity.The rename from
sourceColumntotableColumnand addition ofonCloseimproves the component's API clarity.
46-57: LGTM! Component selection logic simplified.The simplified component selection logic with direct
onChangecalls reduces state complexity.
80-85: Verify save button behavior change.The save button now acts as a close button with automatic saving. Based on PR comments, this change in behavior might need user experience consideration.
Consider either:
- Renaming the button to "Close" to match its behavior
- Adding a tooltip explaining that changes are saved automatically
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (1)
78-118: LGTM! Comprehensive test coverage for text key changes.The test thoroughly verifies the text key selection and update behavior, ensuring the new text resource handling works as expected.
Jondyr
force-pushed
the
spr/main/2ef223ec
branch
from
ea04d5a to
8085a5b
Compare
Jondyr
force-pushed
the
spr/main/2ef223ec
branch
from
8085a5b to
b0cee09
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (1)
26-36: Consider memoizing the callback functions.The
handleIdChangeandhandleRemoveTextResourcecallbacks are recreated on every render. Consider memoizing them withuseCallbackto optimize performance.+import { useCallback } from 'react'; export const EditColumnElementContent = ({ subformLayout, tableColumn, onChange, }: EditColumnElementContentProps) => { const { t } = useTranslation(); + + const handleIdChange = useCallback( + (newTextKey: string) => onChange({ ...tableColumn, headerContent: newTextKey }), + [onChange, tableColumn] + ); + + const handleRemoveTextResource = useCallback( + () => onChange({ ...tableColumn, headerContent: '' }), + [onChange, tableColumn] + ); return ( <> <div className={classes.textResource}> <TextResource label={t('ux_editor.properties_panel.subform_table_columns.column_title_edit')} textResourceId={tableColumn.headerContent} - handleIdChange={(newTextKey) => onChange({ ...tableColumn, headerContent: newTextKey })} - handleRemoveTextResource={() => onChange({ ...tableColumn, headerContent: '' })} + handleIdChange={handleIdChange} + handleRemoveTextResource={handleRemoveTextResource} generateIdOptions={{ layoutId: subformLayout, componentId: 'tableColumn', textResourceKey: generateRandomId(6), }} /> </div>frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx (2)
237-239: Remove unnecessary act() wrapper.The
act()wrapper is unnecessary here asuser.click()from@testing-library/user-eventalready wraps operations inact().- await act(async () => { - await user.click(dataModelBindingsSelect); - }); + await user.click(dataModelBindingsSelect);
249-253: Simplify test assertions.The test has two assertions for
onChangeMockwhere one would suffice. The first assertion checkingtoHaveBeenCalledTimes(2)is redundant since we're already verifying the exact call parameters.- expect(onChangeMock).toHaveBeenCalledTimes(2); expect(onChangeMock).toHaveBeenCalledWith({ headerContent: subformLayoutMock.component4.textResourceBindings.title, cellContent: { query: subformLayoutMock.component4.dataModelBindings.postPlace }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx(5 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx(7 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx(4 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx
⏰ 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/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (1)
10-14: LGTM! Props refactor improves component cohesion.The props interface has been streamlined to use a more cohesive
TableColumntype andonChangecallback, which better aligns with React's unidirectional data flow.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (2)
39-42: LGTM! Good test hygiene with beforeEach.The switch from
afterEachtobeforeEachfor clearing mocks is a good practice as it ensures a clean state before each test runs.
82-122: LGTM! Comprehensive test coverage for text key changes.The new test case thoroughly verifies the text key change functionality, including user interactions and final state validation.
Jondyr
force-pushed
the
spr/main/2ef223ec
branch
2 times, most recently
from
e50ddbf to
34cbe48
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx (3)
190-226: Consider adding negative test cases for text key changes.While the happy path is well tested, consider adding test cases for:
- Invalid text keys
- Empty text keys
- Cancellation of text key changes
255-290: Add assertion for intermediate state in delete operation.The test verifies the final state but doesn't check the intermediate state before deletion.
Add an assertion before the delete operation:
expect(onChangeMock).toHaveBeenNthCalledWith(1, expect.objectContaining({ headerContent: textKeyMock, }));
338-353: Consider adding type safety to renderEditColumnElement utility.While the function works correctly, consider adding type assertions for the mock data to catch potential type mismatches early:
- queryClient.setQueryData([QueryKey.TextResources, org, app], { - nb: [{ id: textKeyMock, value: textValueMock }], - }); + queryClient.setQueryData<TextResourcesResponse>([QueryKey.TextResources, org, app], { + nb: [{ id: textKeyMock, value: textValueMock }], + });frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (1)
42-42: Add null check for cellContent.The optional chaining suggests
cellContentmight be undefined. Consider adding a default value to prevent potential rendering issues.- values={{ cellContent: tableColumn.cellContent?.query }} + values={{ cellContent: tableColumn.cellContent?.query || '' }}frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (1)
49-64: Consider adding error handling for component selection.The component selection logic should handle cases where:
- The selected component is not found
- The component has no data model bindings
- The binding key is not found
const selectComponent = (values: string[]) => { const componentId = values[0]; setSelectedComponentId(componentId); const selectedComponent = availableComponents.find((comp) => comp.id === componentId); + if (!selectedComponent) { + console.warn(`Component with id ${componentId} not found`); + return; + } const bindingKey = Object.keys(selectedComponent.dataModelBindings)[0]; + if (!bindingKey) { + console.warn(`No data model bindings found for component ${componentId}`); + return; + } const binding = convertDataBindingToInternalFormat(selectedComponent, bindingKey); + if (!binding?.field) { + console.warn(`Invalid binding for component ${componentId}`); + return; + } onChange({ ...tableColumn, headerContent: selectedComponent.textResourceBindings?.title, cellContent: { query: binding.field }, }); };
🛑 Comments failed to post (1)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (1)
31-35:
⚠️ Potential issueAvoid generating random IDs during render.
Using
generateRandomIdduring render could lead to inconsistent text keys across re-renders. Consider moving the ID generation to a higher level where it's only generated once when creating a new column.- textResourceKey: generateRandomId(6), + textResourceKey: tableColumn.id || generateRandomId(6),📝 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.generateIdOptions={{ layoutId: subformLayout, componentId: 'tableColumn', textResourceKey: tableColumn.id || generateRandomId(6), }}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (1)
25-37: TextResource component implementation improves header content handling.The implementation correctly handles text resource management and provides immediate feedback through the
handleIdChangeandhandleRemoveTextResourcecallbacks.Consider adding error boundaries around the TextResource component to gracefully handle any text resource loading failures.
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx (1)
196-232: Test coverage added for text resource changes.Good test coverage for text resource changes, but consider adding error cases.
Consider adding test cases for:
- Invalid text resource keys
- Network failures during text resource loading
- Concurrent text resource updates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx(4 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx(8 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx(4 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css
🔇 Additional comments (7)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (1)
10-14: Props interface refactored to improve state management.The props interface has been simplified by removing individual state handlers in favor of a single
onChangecallback, which aligns with React's recommended patterns for state management.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (3)
25-32: Props interface updated for better clarity and control.The renaming of
sourceColumntotableColumnand addition ofonCloseimproves the component's API clarity.
76-76: Simplified save button disabled logic.The condition for disabling the save button has been simplified to essential checks, making the behavior more predictable.
Verify that this change doesn't introduce edge cases where the save button should be disabled but isn't:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other implementations of save button disabled logic rg -A 2 "isSaveButtonDisabled"Length of output: 2137
The save button disabling logic in EditColumnElement is working as expected.
The search confirms that the simplified check consistently follows similar patterns elsewhere (with adjustments per component requirements). No gaps were identified where the save button would erroneously remain enabled.
105-112: Save button now acts as close with automatic saving.The save button's behavior has changed to act as a close button while changes are saved automatically. This aligns with the PR objectives of improving column editing interactions.
Please verify this UX change with the design team to ensure it meets user expectations.
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (2)
42-78: Test coverage properly validates component selection behavior.The test cases effectively verify the component selection functionality and error states.
71-77: Negative test case ensures proper error handling.Good practice: Including a negative test case to verify that unavailable components are not rendered.
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx (1)
298-341: Comprehensive test coverage for data model binding updates.The test effectively verifies the multiple data model binding functionality.
Removes unecessary state, and dependence on custom text-key for custom labels commit-id:2ef223ec
Jondyr
force-pushed
the
spr/main/2ef223ec
branch
from
34cbe48 to
c67e706
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.test.ts (1)
60-159:🧩 Analysis chain
Verify test coverage after removal of title ID functionality.
The test suite no longer includes tests for title ID generation, which was removed as part of the changes mentioned in the PR objectives to eliminate dependence on custom text resource keys. Please ensure that:
- The removal of this functionality doesn't create gaps in test coverage
- The new approach of displaying raw text (when not resolving to an existing text key) is properly tested elsewhere
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to title ID functionality # and verify test coverage in related files # Search for any remaining references to title ID functionality echo "Searching for remaining title ID references..." rg -i "titleid|gettitleid" --type ts # Look for test files that might need coverage for the new raw text display functionality echo "Searching for related test files..." fd -e test.ts -e test.tsx -p "EditSubformTableColumns|ColumnElement"Length of output: 2452
Test Coverage Gap on Raw Text Fallback for Title Handling
- The test mocks in
editSubformTableColumnsUtils.test.tsstill include hard-coded title IDs (e.g.'mockTitleId'), but there are no explicit tests verifying that the new behavior—displaying raw text when no valid text resource key exists—is functioning as expected.- Additionally, a search across related test files (e.g. in the
ColumnElementtests) did not reveal any cases that confirm the correct fallback to raw text.- Please verify that the new raw text fallback logic is covered in the UI tests (or consider adding explicit tests to validate that behavior) so that the removal of the
getTitleIdForColumnfunctionality does not introduce gaps in test coverage.
🧹 Nitpick comments (5)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (1)
38-40: Consider using beforeAll for jest.clearAllMocks()The
beforeEachsetup could be moved tobeforeAllsinceclearAllMocksis idempotent and doesn't need to run before each test. This would slightly improve test performance.- beforeEach(() => { + beforeAll(() => { jest.clearAllMocks(); });frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (1)
66-72: Consider memoizing the handleBindingChange callback.The
handleBindingChangefunction is recreated on every render. Consider using useCallback to optimize performance.- const handleBindingChange = (field: string) => { + const handleBindingChange = useCallback((field: string) => { const updatedTableColumn = { ...tableColumn, cellContent: { query: field }, }; onChange(updatedTableColumn); - }; + }, [tableColumn, onChange]);frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx (2)
196-232: Consider adding error case test coverage.The test suite focuses on happy path scenarios. Consider adding test cases for error conditions such as:
- Invalid text keys
- Network failures
- Empty or malformed data
353-355: Consider testing with multiple languages.The text resources setup only includes 'nb' language. Consider adding test cases with multiple languages to ensure proper i18n handling.
queryClient.setQueryData([QueryKey.TextResources, org, app], { nb: [{ id: textKeyMock, value: textValueMock }], + en: [{ id: textKeyMock, value: 'English value' }], + de: [{ id: textKeyMock, value: 'German value' }], });frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (1)
1-14: LGTM! Consider making the onChange callback more type-safe.The refactored props and new imports align well with the PR objectives of simplifying text resource handling.
Consider using a more specific type for the onChange callback to only allow changes to relevant fields:
- onChange: (updatedTableColumn: TableColumn) => void; + onChange: (updatedTableColumn: Pick<TableColumn, 'headerContent'>) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx(4 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx(8 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx(4 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.test.ts(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.ts(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css
🔇 Additional comments (9)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (1)
42-78: LGTM! Test case properly validates component selection behavior.The test case correctly verifies that the
onChangecallback is invoked with the expected table column values when a component is selected.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (1)
25-32: Props interface looks good with improved naming.The props interface changes improve clarity by:
- Renaming
sourceColumntotableColumnfor better semantics- Replacing
onEditwithonChangefor consistency with React conventions- Adding
onClosefor explicit handling of the close actionfrontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx (1)
324-326: Good use of act for async state updates.The
actwrapper ensures that all state updates are processed before continuing with the test, preventing "act" warnings and race conditions.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.test.ts (3)
6-10: LGTM! Import statements follow TypeScript best practices.The use of
import typesyntax for type-only imports is a good practice as it helps reduce bundle size by ensuring types are removed during compilation.
13-58: LGTM! Test implementation follows best practices.The test suite demonstrates:
- Well-structured test cases with clear descriptions
- Comprehensive mock data
- Good coverage of edge cases
- Precise assertions
Also applies to: 61-127, 129-158
50-50: Track technical debt: Removeas anytype assertion.The TODO comment references issue #14441. Once that issue is resolved, this type assertion should be removed to maintain type safety.
Let's check the status of the referenced issue:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check the status of issue #14441 gh issue view 14441 --json title,state,bodyLength of output: 1276
Technical debt tracking remains necessary.
The GitHub issue #14441 is still open, confirming that the temporary
as anytype assertion is expected until the issue is resolved. Once it's closed, the type assertion should be removed to enhance type safety in the codebase.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (3)
16-22: LGTM! Clean implementation of the refactored component.The component signature changes align well with the PR objectives of simplifying state management.
39-46: Verify cell content query access safety.The optional chaining on
cellContent?.queryis good, but we should ensure proper fallback handling.- values={{ cellContent: tableColumn.cellContent?.query }} + values={{ cellContent: tableColumn.cellContent?.query ?? t('ux_editor.properties_panel.subform_table_columns.no_content') }}
23-38: Verify text resource key uniqueness and handling.A few concerns about the text resource handling:
- Random ID generation could potentially create duplicate keys
- Setting empty string for headerContent on remove might cause issues (per PR comments discussion)
Let's verify the uniqueness constraints and error handling:
Consider adding a prefix to ensure uniqueness:
- textResourceKey: generateRandomId(6), + textResourceKey: `col_${generateRandomId(6)}`,
...nts/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx
Show resolved
Hide resolved
|
@coderabbitai following the specification of conventional commits, suggest a title for this PR. |
|
Tip For best results, initiate chat on the files or code changes. @Jondyr Based on the changes in this PR, I suggest the following conventional commit title: This title follows the conventionalcommits specification by:
|
|
Tip For best results, initiate chat on the files or code changes. @Jondyr Based on the changes in this PR, I suggest the following conventional commit title: This title follows the conventional commits format:
|
Removes unecessary state, and dependence on custom text resource key for headerContent.
headerContentnow uses theTextResourcecomponent for editing and lookup of text resources.tablecolumns-text-resources.mp4
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
onEdittoonChangeand simplifying state management.Tests
Style