-
Notifications
You must be signed in to change notification settings - Fork 131
Add History Pane #10832
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
base: main
Are you sure you want to change the base?
Add History Pane #10832
Conversation
|
E2E Tests 🚀 |
|
Is deleting entries working for you? I found that deleting entries wasn't working for me. I tested against entries for |
|
Noticed a strange resizing issue where the scroll bar is shifting! Screen.Recording.2025-12-02.at.4.39.12.PM.mov |
It is! I think the disposable leaks you're seeing are unrelated (just pushed a fix for those). Can you share more about the steps you're taking and what you're seeing? |
Are you supposed to be able to delete stuff that falls under the "Older" grouping? I just tested things again and noticed that it works for stuff under "Today". Here are the steps:
Screen.Recording.2025-12-02.at.5.09.39.PM.mov |
This happens because we're using react-window, which requires that we supply an explicit width (and will only render in the rectangle supplied). We debounce the width we pass to react-window because asking it to redraw every time the size changes results in some pretty bad flickering artifacts that are worse than the behavior you see here. I think it's worth the tradeoff to get virtualized scrolling since these histories can get pretty long! I toned down the debouncer in 275746a; LMK if you think it still feels too janky and I'll see if there are any other tricks we can use here. |
You are! But I've done so many state clears that I didn't have anything in that grouping, which is why I couldn't reproduce this. Thanks, I'll take a look! |
dhruvisompura
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.
Not done with my review and will continue it on the following day, but here's my feedback so far!
| } | ||
| }} | ||
| > | ||
| <div className="history-entry-content"> |
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.
super minor, but using double quotes for classnames normally throws an eslint error. These should be single quotes. There's a few places with this issue!
| <div className="history-entry-content"> | |
| <div className='history-entry-content'> |
| {/* Show "... N more lines" above if smart excerpt hides lines above */} | ||
| {smartExcerpt && smartExcerpt.hiddenAbove > 0 && ( | ||
| <div className="history-entry-line-indicator"> | ||
| ... {smartExcerpt.hiddenAbove} more lines |
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.
This string should probably be localized!
| // Input key down handler. | ||
| const inputKeyDownHandler = (e: React.KeyboardEvent<HTMLInputElement>) => { | ||
| if (e.key === 'Escape' && filterText !== '') { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| buttonClearClickHandler(); | ||
| } | ||
| }; | ||
|
|
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.
This was a nice addition and we get it for free in the data explorer and variables pane!
| onMouseDown={(e) => { | ||
| // Use onMouseDown instead of onClick to ensure selection happens before focus events | ||
| // This prevents the two-click issue when the panel is unfocused | ||
| if (e.button === 0) { // Only handle left clicks | ||
| e.preventDefault(); // Prevent default focus behavior that steals focus | ||
| onSelect(); | ||
| } | ||
| }} |
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.
Do you have a video of this "two-click" issue? I'm trying to understand what may have been happening. Generally, an onClick handler would be what we'd want to use. Having to use onMouseDown makes me think there is a greater focus/selection bug.
| const styleWithoutHeight = { ...style, height: 'auto' }; | ||
|
|
||
| return ( | ||
| <div |
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 know we have divs with click elements in quite a number of places but an unstyled button element would be a much better approach here. We get accessibility and keyboard navigation for free which means a lot less code and semantically divs aren't supposed to be used for interactive elements.
This does make me think a reusable unstyled button element would be really useful for most of our panes!
| return ( | ||
| <div className="history-separator" style={customStyle}> | ||
| <div className="history-separator-content"> | ||
| <span className="history-separator-label">{label}</span> | ||
| </div> | ||
| </div> | ||
| ); |
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.
Just a thought on a future improvement: while playing with this, I really wanted to be able to click this separator so I could collapse the groupings since I had a lot of OLDER entries and didn't want to scroll all the way to the bottom to see the history entries from TODAY.
| {stickyHeaderLabel && listItems.length > 0 && ( | ||
| <div className='history-sticky-header'> | ||
| <div className='history-separator-content'> | ||
| <span className='history-separator-label'>{stickyHeaderLabel}</span> | ||
| </div> | ||
| </div> | ||
| )} |
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 think we could just use the historySeparator component and pass in the history-sticky-header class as a prop.
| ); | ||
| } | ||
| // Create and append the Positron history container | ||
| this._positronHistoryContainer = DOM.$('.positron-history-container'); |
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.
This container doesn't look to be extending the full height of the parent pane. We need a height: 100% on this element like we have for our other containers.
|
Just tested the delete functionality again, and noticed that deleting a single older entry still not working as expected. I tried to delete the only entry I had under "OLDER". That didn't seem to work and I noticed an entry below ended up getting deleted. 😭 Out of curiosity, is there an easy way to mock out a ton of history for the panel that is older for testing? I was hoping to dig into why that was happening but I can't reliably reproduce without older entries (anything not from "Today") Screen.Recording.2025-12-04.at.9.49.14.AM.mov |
I just pulled down the changes and gave them a test run. I didn't notice a visible improvement with the debounce. It's still very noticeable that the vertical scrollbar is trying to run away 😂. Screen.Recording.2025-12-04.at.9.46.56.AM.mov |
|
I definitely think there is something wonky going on with the history container and getting it to take up the full height of the panel in the secondary side bar...the scrollbar isn't extending the full height and its causing some issues when you try and scroll to the bottom to see the last item. Screen.Recording.2025-12-04.at.10.15.07.AM.movScreen.Recording.2025-12-04.at.9.46.56.AM.copy.mov |

A little holiday week project to add a history pane to Positron:
Some placeholder services had already been created this early in Positron's lifecycle, and we already save inputs for navigation/search in the console, so this mostly just connects wires and adds UX on top of existing services.
Features:
Addresses #5484.
Release Notes
New Features
Bug Fixes
QA Notes