-
Couldn't load subscription status.
- Fork 653
Form: fix get item by path (T1311534) #31492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 25_2
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in the Form component's itemOption method when retrieving items by path where multiple items share the same name or caption (T1311534). The fix ensures path-based lookups correctly traverse nested groups and return the intended item rather than the first matching name.
Key changes:
- Fixed the
_getFieldPartsmethod to correctly reverse the path array after splitting - Added validation in
_getItemByFieldPathto ensure the path node matches before retrieving nested items - Changed return types from
falsetonullfor consistency with TypeScript conventions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.ui.widgets.form/form.tests.js | Added test case verifying itemOption retrieves correct items by path when duplicates exist |
| packages/devextreme/js/__internal/ui/form/form.utils.ts | Changed return type from false to null for consistency |
| packages/devextreme/js/__internal/ui/form/form.ts | Fixed path parsing logic and added path validation to prevent incorrect item matching |
b5480bc to
a1c242f
Compare
a1c242f to
6a6a1c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
6a6a1c7 to
ad9448a
Compare
ad9448a to
69fa561
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
| return { | ||
| fieldName, | ||
| fieldPath: resultPath.reverse(), | ||
| fieldPath, |
Copilot
AI
Oct 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _getFieldParts method incorrectly reverses the fieldPath. When splitting 'a.b.c', it becomes ['c', 'b', 'a'], then destructuring gives fieldName='c' and fieldPath=['b', 'a']. However, the return statement doesn't reverse fieldPath back, so it's returned as ['b', 'a'] instead of ['a', 'b']. The old implementation at line 1498 explicitly reversed the path with resultPath.reverse(). This bug will cause path traversal to fail when looking up nested items.
| fieldPath, | |
| fieldPath: fieldPath.reverse(), |
|
|
||
| const editorLabelText = 'Item 1'; | ||
|
|
||
| const resultLabelMode = formLabelMode; |
Copilot
AI
Oct 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test removed the editorLabelMode parameter when determining resultLabelMode (line 224). The original code at line 1551 in form.tests.js used editorLabelMode || formLabelMode, but the new code only uses formLabelMode. This changes the test logic and may cause incorrect assertions when editorLabelMode is specified but different from formLabelMode.
| const resultLabelMode = formLabelMode; | |
| const editorLabelMode = $form.dxForm('option', 'items')[0].label?.labelMode; | |
| const resultLabelMode = editorLabelMode || formLabelMode; |
No description provided.