-
Notifications
You must be signed in to change notification settings - Fork 33
feat: DH-19382: Add actions and timing info on console history hover #2526
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
feat: DH-19382: Add actions and timing info on console history hover #2526
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 adds hover actions to console history items, including copy, rerun functionality, and a contextual help tooltip displaying execution timing information. The implementation supports both client-side execution time and server query time (when available from the backend).
- Adds hover actions (copy, rerun, info tooltip) to console history items
- Implements timing display functionality with support for millisecond and nanosecond conversions
- Extends console history data structures to include server timing information
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ConsoleHistoryTypes.tsx | Adds server timing fields to the action item interface |
| ConsoleHistoryItemTooltip.tsx | New component for displaying execution timing information in a contextual help tooltip |
| ConsoleHistoryItemTooltip.test.tsx | Test suite for the tooltip component and time formatting utility |
| ConsoleHistoryItemActions.tsx | New component containing copy, rerun, and info actions that appear on hover |
| ConsoleHistoryItem.tsx | Integrates action buttons and hover state management |
| ConsoleHistoryItem.scss | Styles for hover effects and action button positioning |
| ConsoleHistory.tsx | Passes command submit handler to history items |
| ConsoleHistory.scss | Additional tooltip styling |
| CommandHistoryStorage.ts | Extends storage interface to include server timing |
| Console.tsx | Updates console to handle and pass server timing data |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/console/src/console-history/ConsoleHistoryItemActions.tsx
Outdated
Show resolved
Hide resolved
packages/console/src/console-history/ConsoleHistoryItemTooltip.tsx
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2526 +/- ##
==========================================
+ Coverage 45.14% 45.23% +0.08%
==========================================
Files 769 771 +2
Lines 43511 43580 +69
Branches 11189 11026 -163
==========================================
+ Hits 19645 19713 +68
- Misses 23820 23851 +31
+ Partials 46 16 -30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| .console-history-actions { | ||
| z-index: 1000; |
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.
We shouldn't have the z-index so high. In bootstraps _variables.scss, they define a bunch of $zindex-* variables, and $zindex-dropdown is 1000, so this could in theory conflict with that. Probably should only set it to 100 at most.
That being said - why do we need to set this anyway?
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.
removed - this was just leftover from some experimentation I was doing with the hover behavior
| onMouseOver={() => this.setState({ isHovered: true })} | ||
| onMouseOut={() => this.setState({ isHovered: false })} |
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.
You shouldn't need this isHovered state at all - instead just key off the :hover CSS class.
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.
done
| padding: $spacer-1; | ||
| } | ||
|
|
||
| .console-history-item-command-hovered { |
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.
You shouldn't need to do this yourself, you can just use the :hover pseudo-class:
| .console-history-item-command-hovered { | |
| .console-history-item-command:hover { |
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.
I originally had it with the pseudo-class, but the behavior is buggy. When I click on the contextual help button, the hover stops, and the contextual help popup moves to the top left corner of the screen (seemingly because the contextual help button is gone). Is there a good way to use the pseudo class that keeps it hovered after clicking on the contextual 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.
Target the :hover, then when the tooltip is shown add another class that can also be targeted
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.
done
| const lineCount = command.split('\n').length; | ||
| const classes = ['console-history-actions']; | ||
|
|
||
| if (lineCount === 1) { | ||
| if (firstItem) { | ||
| // first single items are pushed down so that they are visible | ||
| // this should be higher priority than lastItem | ||
| classes.push('console-history-first-single-line'); | ||
| } else if (lastItem) { | ||
| // last single items are pushed up to prevent layout shifts | ||
| classes.push('console-history-last-single-line'); | ||
| } else { | ||
| classes.push('console-history-single-line'); | ||
| } | ||
| } else if (lineCount === 2) { | ||
| classes.push('console-history-two-lines'); | ||
| } | ||
| return classes.join(' '); | ||
| }; |
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.
I don't think you need the firstItem or lastItem properties at all - you can do this all with CSS!
Take a look at the :first-child and :last-child pseudo-classes: https://developer.mozilla.org/en-US/docs/Web/CSS/:first-child
Then in CSS instead of doing .console-history-last-single-line, you would do .console-command-result:first-child (or even .console-command-result:first-child .console-history-item-command if you want to select an inner element within that child).
Then we don't need to manually track the classes.
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.
That's a really cool feature - how should I properly target only first and last when there is one line of text?
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.
.console-command-result:first-child .console-history-actions-line-1
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.
done
packages/console/src/console-history/ConsoleHistoryItemActions.tsx
Outdated
Show resolved
Hide resolved
packages/console/src/console-history/ConsoleHistoryItemUtils.ts
Outdated
Show resolved
Hide resolved
| * @param conversion The conversion type ('ms' or 'ns') | ||
| * @returns A string representing the time difference, or null if invalid. | ||
| */ | ||
| export const getTimeString = ( |
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.
We should use this function from CommandHistoryItemTooltip.tsx as well for consistency.
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.
It seems like the CommandHistoryItemTooltip.tsx one has a couple key differences:
- If the delta is less than 1 second it just returns "<1s"
- Possibly has hours, minutes, and seconds, like "1h 3m 23s"
I don't want to remove that formatting completely without verifying how we want to unify this, as the formats are different enough where one isn't inherently better than the other I think. Thoughts?
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.
From the ticket DH-19382:
Label as “Execution time: 1.02s” - round to two decimals, and add appropriate units. Note time could be seconds to hours, so display units accordingly.
@dsmmcken if we had a time that was over an hour, what format should it display in?
1h 3m 23s(rounded to the second)1h 3m 23.02s(larger units split out, but seconds rounded to two decimal places)1.05h(round to two decimal places of the largest unit- ???
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.
if greater than 1 hour, no decimals: 1h 2m 32s
if less than an hour, 1 decimal: 2m 32.1s
if less than a minute, 2 decimals: 32.02s
I would say precision matter less the higher the scale goes.
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.
modified both to use new format
packages/console/src/console-history/ConsoleHistoryItemTooltip.tsx
Outdated
Show resolved
Hide resolved
mofojed
left a comment
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.
Whoops, meant to request changes for the classNames suggestion
|
Let's not merge until the API gets merged: deephaven/deephaven-core#7145 |
|
@jnumainville API is merged let's get this over the line |
Server side PR for DH-19382 Adds server side timing to `JsCommandResult` for query execution time Tested in deephaven/web-client-ui#2526
…7145) Server side PR for DH-19382 Adds server side timing to `JsCommandResult` for query execution time Tested in deephaven/web-client-ui#2526
Adds new actions when hovering over console history items: copy, rerun, and a contextual help with timing (from the same source as the extant time in our command history but formatted differently to meet spec) as well as new server timing.
Full support is dependent on adding server timing to console history from core. That PR is not up yet but just running this PR will just not show that time.