Grids: retarget dataController.push method - #35123
Open
anna-shakhova wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new focus DataSourceController extender sets its “data pushed” flag after firing pushed, which can reintroduce the ordering bug when pushed subscribers trigger an earlier change cycle.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR retargets grid “push” handling from DataController to DataSourceController, moving both the imperative push(...) entry point and the pushed callback pipeline, and adjusts focus/selection/editing code accordingly. It also refactors the grid-core focus module export into a dedicated focus_module.ts and adds a focus-specific DataSourceController extender plus Jest coverage.
Changes:
- Moved
push(...)andpushedevent wiring fromDataControllertoDataSourceController, updating selection/editing and related tests/mocks. - Split
focusModuleintofocus_module.tsand introduced a focusDataSourceControllerextender to track/consume “data pushed” state. - Added Jest coverage for the new focus data-source extender and expanded
DataSourceControllertests.
File summaries
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataController.tests.js | Updated QUnit tests to listen to dataSourceController.pushed and validate disposal behavior. |
| packages/devextreme/testing/helpers/gridBaseMocks.js | Adjusted grid test mocks to include push/pushed on the data source adapter mock. |
| packages/devextreme/js/__internal/grids/tree_list/m_focus.ts | Retargeted focus module import to the new focus_module entry. |
| packages/devextreme/js/__internal/grids/grid_core/selection/m_selection.ts | Subscribed selection push-handling to dataSourceController.pushed instead of dataController.pushed. |
| packages/devextreme/js/__internal/grids/grid_core/focus/m_focus.ts | Refactored focus extenders to be exported individually; shifted “data pushed” consumption to DataSourceController. |
| packages/devextreme/js/__internal/grids/grid_core/focus/focus_module.ts | New focus module entry defining defaults/controllers/extenders, including the new dataSource extender. |
| packages/devextreme/js/__internal/grids/grid_core/focus/extenders/focus_data_source_controller.ts | New extender tracking whether a push occurred and exposing consumeDataPushed(). |
| packages/devextreme/js/__internal/grids/grid_core/focus/extenders/tests/focus_data_source_controller.integration.test.ts | New integration tests for focused-row behavior around push + subscriber-driven updates. |
| packages/devextreme/js/__internal/grids/grid_core/editing/m_editing.ts | Switched editing save refresh path from dataController.push to dataSourceController.push. |
| packages/devextreme/js/__internal/grids/grid_core/data_source/data_source_controller.ts | Added pushed callback and push(...) API; wired adapter pushed into controller-level callback. |
| packages/devextreme/js/__internal/grids/grid_core/data_source/tests/data_source_controller.test.ts | Added tests for pushed re-fire behavior and push(...) delegation. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts | Removed pushed callback and push(...) API and unsubscribed from adapter pushed. |
| packages/devextreme/js/__internal/grids/data_grid/focus/m_focus.ts | Retargeted focus module import to the new focus_module entry. |
Review details
Suppressed comments (1)
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataController.tests.js:246
- This test mutates
dataSourceController.dataPushedHandlerProxy, which is now an internalprivate readonlyimplementation detail. That makes the test brittle (e.g. if the controller switches to#privatefields) and it also stops exercising the real handler. Prefer asserting the observable behavior:DataSourceController.pushedshould stop firing after disposal.
this.dataSourceController.dataPushedHandlerProxy = dataPushedHandlerSpy;
this.dataController.setDataSource(dataSource);
dataSource = this.dataSourceController.getAdapter();
dataSource.load();
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
anna-shakhova
force-pushed
the
retarget_push_main
branch
from
September 9, 2026 13:19
45a3007 to
61d00b7
Compare
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The push/pushed retargeting is consistently applied across controllers and is backed by focused unit + integration test coverage and updated mocks.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.