Scheduler: refactor workspaces module (TS): part 2#33864
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the Scheduler workspace TS refactor by moving away from legacy m_* modules and tightening typing across workspace navigation/selection and layout helper utilities.
Changes:
- Replaced legacy workspace imports (
m_*) with refactored modules (e.g.work_space_vertical,helpers/position_helper, selection controller/state). - Improved TypeScript typings across workspace render options, cell selection flow, and view-data provider APIs.
- Introduced/updated helper and model types (e.g.
CellPosition) to make workspace interactions more explicit.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/js/__internal/scheduler/workspaces/work_space_week.ts | Updates week workspace to use refactored vertical workspace module. |
| packages/devextreme/js/__internal/scheduler/workspaces/work_space_day.ts | Updates day workspace to use refactored vertical workspace module. |
| packages/devextreme/js/__internal/scheduler/workspaces/work_space_vertical.ts | Adds explicit return/parameter types for render options and formatting. |
| packages/devextreme/js/__internal/scheduler/workspaces/work_space_month.ts | Adjusts month cell width calculation typing/return behavior. |
| packages/devextreme/js/__internal/scheduler/workspaces/view_model/m_view_data_provider.ts | Tightens return type for findCellPositionInMap and imports shared types. |
| packages/devextreme/js/__internal/scheduler/workspaces/view_model/m_types.ts | Adds shared position type(s) used by view model / selection flow. |
| packages/devextreme/js/__internal/scheduler/workspaces/view_model/m_grouped_data_map_provider.ts | Updates typings for findCellPositionInMap to align with refactor. |
| packages/devextreme/js/__internal/scheduler/workspaces/m_work_space.ts | Switches to refactored helpers/controllers and updates selection logic integration. |
| packages/devextreme/js/__internal/scheduler/workspaces/helpers/position_helper.ts | Refactors/strongly-types position helper utilities and strategies. |
| packages/devextreme/js/__internal/scheduler/workspaces/cells_selection_state.ts | Adds stronger typing for focused/selected cell state handling. |
| packages/devextreme/js/__internal/scheduler/workspaces/cells_selection_controller.ts | Adds typed navigation logic for keyboard-based cell selection. |
Comments suppressed due to low confidence (5)
packages/devextreme/js/__internal/scheduler/workspaces/helpers/position_helper.ts:162
positionis anumber(result ofgetMaxAllowedPosition), but it is being cast tonumber[]and indexed. This makescurrentPositionalwaysundefinedat runtime, so the correction branch never runs and group width calculations can be wrong.
packages/devextreme/js/__internal/scheduler/workspaces/cells_selection_controller.ts:65- After making
focusedCellPositionoptional,handleArrowClickshould guard before using it; otherwise the method will still throw whenfocusedCellPositionis missing.
packages/devextreme/js/__internal/scheduler/workspaces/helpers/position_helper.ts:49 getCellSizecan be called when DOM metadata contains placeholder objects (e.g.[[{}]]when there is no window / no cells). In that case,cellSize.width/heightwill beundefinedat runtime, but the function returns them as numbers, which can propagate NaN into downstream layout calculations.
packages/devextreme/js/__internal/scheduler/workspaces/cells_selection_controller.ts:18focusedCellPositioncomes fromViewDataProvider.findCellPositionInMap, which can returnundefined. Making it optional here avoids forcing callers to assert non-null, and pairs with a runtime guard inhandleArrowClick.
packages/devextreme/js/__internal/scheduler/workspaces/cells_selection_controller.ts:127- If
focusedCellPositionis allowed to beundefined(e.g. when the view data map can’t resolve a position), this destructuring will throw. Either guard earlier (as inhandleArrowClick) and use a non-null assertion here, or add a local fallback/guard in this method too.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
packages/devextreme/js/__internal/scheduler/workspaces/helpers/position_helper.ts:49
getCellSizecan returnundefineddimensions whengetDOMElementsMetaData()provides placeholder objects like[[{}]](e.g., no DOM / no cells). That propagates asNaNin width/height calculations. Please default missingwidth/heightto0.
packages/devextreme/js/__internal/scheduler/workspaces/helpers/position_helper.ts:104getAllDayHeightcan returnundefinedwhen DOM meta arrays contain placeholder objects (e.g.,{}fromgetAllDayPanelDOMElementsInfo()/getDateTableDOMElementsInfo()), which then breaks calculations relying on numeric heights. Please normalize missing heights to0.
packages/devextreme/js/__internal/scheduler/workspaces/helpers/position_helper.ts:163getGroupWidthtreats the result ofgetMaxAllowedPosition(...)as an array (position[groupIndex]), butgetMaxAllowedPositionreturns a single number. This makes the correction block effectively dead and can return incorrect group widths (especially in RTL). Compute the current/prev/next positions by callinggetMaxAllowedPositionfor the needed group indices instead of indexing into a scalar.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/devextreme/js/__internal/scheduler/workspaces/cells_selection_state.ts:77
setSelectedCellsmay callviewDataProvider.getCellsBetweenwithfirstCellbeingnullwhenfirstCellCoordinatesis omitted (e.g. multi-selection starts beforefirstSelectedCellis initialized). The currentfirstCell as ViewCellDatacast hides this and can cause a runtime exception insidegetCellsBetween(it dereferencesfirst.startDate). Prefer a safe fallback tolastCellwhenfirstCellis not set, and store the resolved value back intofirstSelectedCell.
packages/devextreme/js/__internal/scheduler/workspaces/helpers/position_helper.ts:9DOMMetaDatais typed asDOMRect[][]/DOMRect[], but the metadata produced bySchedulerWorkSpace.addCellMetaDatais a plain object with only{ left, top, width, height }. UsingDOMRecthere is misleading and suggests additional fields (e.g.right,bottom,x,y) exist. Consider introducing a dedicated cell-rect type matching the actual shape.
| @@ -83,50 +142,33 @@ export const getGroupWidth = (groupIndex, viewDataProvider, options) => { | |||
| result = groupLength * cellWidth; | |||
| } | |||
|
|
|||
| const position = getMaxAllowedPosition( | |||
There was a problem hiding this comment.
Dead code. getMaxAllowedPosition always returned a number, not an array. In following lines the code tried to access the value from getMaxAllowedPosition as array, which is nonsense
| ) | ||
| : this.firstSelectedCell; | ||
| const lastCell = viewDataProvider.getCellData(lastRowIndex, lastColumnIndex, isLastCellAllDay); | ||
| : (this.firstSelectedCell ?? lastCell); |
There was a problem hiding this comment.
Added a fallback for firstSelectedCell, because it can be undefined
| const dateTableCells = this.getAllCells(false); | ||
| if (!dateTableCells.length || !hasWindow()) { | ||
| return [[{}]]; | ||
| return [[{ |
There was a problem hiding this comment.
Changed for type compatibility reasons and to remove useless type guards. Should not affect anything
| this.positionHelper = new PositionHelper({ | ||
| key: this.option('key'), | ||
| viewDataProvider: this.viewDataProvider, | ||
| viewStartDayHour: this.option('startDayHour'), |
There was a problem hiding this comment.
Those props were not actually being used
# Conflicts: # packages/devextreme/js/__internal/scheduler/workspaces/view_model/m_grouped_data_map_provider.ts
No description provided.