#42121 blank page when gantt was resize to minimum - #116
#42121 blank page when gantt was resize to minimum#116daniela-mateescu wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses blank page issues when resizing the gantt chart to its minimum width by preventing division-by-zero errors across several components and optimizing state updates to avoid redundant renders. The reviewer identified several critical improvement opportunities: returning zero in pixelsPerMillisecond may still cause downstream division-by-zero issues in getTimeAtPixel; the optional onScroll prop in Scrollbar should be guarded to prevent runtime errors; this.state.resolution in timebar.js needs a safety fallback; and side effects inside getBottomBar should be decoupled from componentDidUpdate to prevent unnecessary updates.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function pixelsPerMillisecond(vis_start, vis_end, total_width) { | ||
| const start_end_ms = vis_end.diff(vis_start, 'milliseconds'); | ||
| return total_width / start_end_ms; | ||
| return start_end_ms === 0 ? 0 : total_width / start_end_ms; |
There was a problem hiding this comment.
While returning 0 prevents division by zero inside pixelsPerMillisecond, it introduces a division by zero in getTimeAtPixel (line 56), where pixel_location is divided by the result of pixelsPerMillisecond. If pixelsPerMillisecond returns 0, getTimeAtPixel will calculate min_offset as Infinity or NaN, leading to an invalid Moment object and potential runtime errors or blank pages when the mouse is moved.
| const unit_per_px = this.state.scrollbarSize === 0 ? 0 : this.props.pageSize / this.state.scrollbarSize; | ||
| const scrollPositionInPixels = this.props.direction == Direction.HORIZONTAL ? this._outterDiv.scrollLeft : this._outterDiv.scrollTop; | ||
| this.props.onScroll(this.props.minScrollPosition + unit_per_px * scrollPositionInPixels); |
There was a problem hiding this comment.
The onScroll prop is optional in ScrollbarProperties (onScroll?: (scrollPosition: number) => void;), but it is called here without a safety guard. If a parent component renders Scrollbar without providing onScroll, this will throw a runtime TypeError. Please add a safety check before calling this.props.onScroll.
const unit_per_px = this.state.scrollbarSize === 0 ? 0 : this.props.pageSize / this.state.scrollbarSize;
const scrollPositionInPixels = this.props.direction == Direction.HORIZONTAL ? this._outterDiv.scrollLeft : this._outterDiv.scrollTop;
if (this.props.onScroll) {
this.props.onScroll(this.props.minScrollPosition + unit_per_px * scrollPositionInPixels);
}
| setResolutionIfNeeded(resolution) { | ||
| if (this.state.resolution.top !== resolution.top || this.state.resolution.bottom !== resolution.bottom) { | ||
| this.setState({resolution}); | ||
| } | ||
| } |
There was a problem hiding this comment.
To ensure defensive programming, it is safer to guard against this.state.resolution being undefined or null before accessing its properties. Using a fallback object prevents potential runtime TypeErrors.
| setResolutionIfNeeded(resolution) { | |
| if (this.state.resolution.top !== resolution.top || this.state.resolution.bottom !== resolution.bottom) { | |
| this.setState({resolution}); | |
| } | |
| } | |
| setResolutionIfNeeded(resolution) { | |
| const currentResolution = this.state.resolution || {}; | |
| if (currentResolution.top !== resolution.top || currentResolution.bottom !== resolution.bottom) { | |
| this.setState({resolution}); | |
| } | |
| } |
| const bottomBarComponent = this.getBottomBar(); | ||
| const topBarComponent = this.getTopBar(); |
There was a problem hiding this comment.
Calling this.getBottomBar() inside componentDidUpdate triggers this.props.setVerticalGridLines(bottomBar) on every update, even if bottomBarComponent hasn't changed. Since getBottomBar has this side effect, it is recommended to separate the side effect (updating vertical grid lines) from the getter method, or only call it when the bottom bar component actually changes.
There was a problem hiding this comment.
Are conditie inainte de a fi apelat
…en-gantt-was-resize-to-minimum
Proposed Change:
What is your change
Change type
Put an
xin the boxes that applyChecklist