-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix query splits timeline in preview UI to prevent blank screen #26920
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
Conversation
Reviewer's GuideRefactored the QuerySplitsTimeline component to exclude invalid NaN task times from min/max calculations and to guard timeline rendering with valid default start/end times, displaying an informational alert when data is insufficient. Class diagram for updated QuerySplitsTimeline component logicclassDiagram
class QuerySplitsTimeline {
+items: SplitTimelineItem[]
+groups: Group[]
+startTimes: number[]
+endTimes: number[]
+timelineStartTime: number | null
+timelineEndTime: number | null
+itemRenderer(item, itemContext, getItemProps)
}
class Timeline {
+groups: Group[]
+items: SplitTimelineItem[]
+defaultTimeStart: number
+defaultTimeEnd: number
+itemRenderer: TimelineItemRenderer
+minZoom: number
+itemHeightRatio: number
+itemTouchSendsClick: boolean
+canMove: boolean
+canResize: boolean
+stackItems: boolean
}
QuerySplitsTimeline --> Timeline : renders if valid times
QuerySplitsTimeline --> Alert : renders if missing times
class Alert {
+severity: string
+message: string
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider memoizing the filtered startTimes/endTimes and computed timelineStartTime/timelineEndTime with useMemo to avoid recalculating them on every render.
- To improve readability, extract the conditional Timeline vs Alert rendering block into its own child component or helper function.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider memoizing the filtered startTimes/endTimes and computed timelineStartTime/timelineEndTime with useMemo to avoid recalculating them on every render.
- To improve readability, extract the conditional Timeline vs Alert rendering block into its own child component or helper function.
## Individual Comments
### Comment 1
<location> `core/trino-web-ui/src/main/resources/webapp-preview/src/components/QuerySplitsTimeline.tsx:292-301` </location>
<code_context>
- stackItems
- // traditionalZoom
- />
+ {timelineStartTime && timelineEndTime ? (
+ <Timeline
+ groups={groups}
+ items={items}
+ defaultTimeStart={timelineStartTime}
+ defaultTimeEnd={timelineEndTime}
+ itemRenderer={itemRenderer}
+ minZoom={60 * 1000}
+ itemHeightRatio={0.65}
+ itemTouchSendsClick={false}
+ canMove={false}
+ canResize={false}
+ stackItems
+ // traditionalZoom
+ />
+ ) : (
+ <Box sx={{ width: '100%', mt: 1 }}>
+ <Alert severity="info">
</code_context>
<issue_to_address>
**issue:** Using truthy checks for timelineStartTime and timelineEndTime may skip valid zero values.
Explicitly check for null or undefined to ensure zero values are handled correctly.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Description
Fix an issue when blank screen appears in the preview UI Query Splits Timeline when split task hasn’t started.
Additional context and related issues
Sometimes this happens because the
<Timeline>
component requires validdefaultTimeStart
anddefaultTimeEnd
values, butNaN
is being selected as min/max when some tasks don’t have a start/end time.To fix:
NaN
values when calculating min/max task times.defaultTimeStart
anddefaultTimeEnd
are available before rendering the timeline.Alert
if the required time values are missing, instead of rendering an empty timeline that results a blank screenRelease notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: