-
Notifications
You must be signed in to change notification settings - Fork 121
PositroNB: Restart kernel button #9898
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?
Conversation
E2E Tests 🚀 |
@seeM Based on your changes, what do you expect to happen in the scenarios 2-4? Cases
Outcomes
|
@seeM Awesome! This is looking good however I am concerned that we are running out of space in the nav bar and not grouping related actions in the same area. I put 2 mockups for exploration here to determine next steps: https://www.figma.com/design/z9EOP4qLLzR08PNxe4M0Ei/Positron-Notebooks---Design-Audit?node-id=305-2&t=8CHFBPPjEQfYYkiB-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.
Code itself looks great. I'll leave the design part to further discussions.
src/vs/workbench/contrib/runtimeNotebookKernel/browser/runtimeNotebookKernelActions.ts
Outdated
Show resolved
Hide resolved
ca6b680
to
8b66d2d
Compare
Thanks for the reviews! I've updated the UI based on your feedback:
|
@seeM looking great! Based on the design we made the other day with a new dropdown for the kernel, are you suggesting that whole dropdown gets handled in #9722 Where restart kernel and show console would be a submenu item in this dropdown? My understanding was that 9722 would just capture putting the kernel info in the new dropdown |
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 see any particular e2e tests that exercise the notebook kernel restart. I can def add a test for that. However, I was curious if there are any particular checks I should assert? Is there any kind of persistence that is expected or is everything wiped?
@midleman in terms of state, it should behave exactly as restarting a console session. Some ideas:
|
@rodrigosf672 IIUC whether the notebook is saved or not shouldn't affect restart behavior. Perhaps restarting while something is running should display a warning, and perhaps a warning should also be displayed based if data will be lost. Will start a discussion in Slack. |
FYI, added a ticket to create an e2e test for this feature: #10033 |
8b66d2d
to
202ebc4
Compare
202ebc4
to
fbd23f2
Compare
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.
Re-reviewed and all looks good to me! I like the new design!
color: grey; | ||
} | ||
/* Use this property to take padding into account when sizing (prevents overflow) */ | ||
box-sizing: border-box; |
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.
Crazy this isn't in a base css file reset.
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.
Thanks for all this work!
I marked as Request changes because of the runtime status icon bug which does effect the console session list too. The fix is a small one line change which I dropped as a code suggestion. Mostly looking great. Had a couple comments/questions but nothing majorly blocking once that status icon color bug is fixed!
const statusToIconColor: Record<RuntimeStatus, ColorIdentifier> = { | ||
[RuntimeStatus.Active]: POSITRON_CONSOLE_STATE_ICON_ACTIVE, | ||
[RuntimeStatus.Disconnected]: POSITRON_CONSOLE_STATE_ICON_DISCONNECTED, | ||
[RuntimeStatus.Idle]: POSITRON_CONSOLE_STATE_ICON_DISCONNECTED, |
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.
Looks like a small copy/paste error that is causing the icon to always be red for the notebook kernel status AND in the console tab list. Changing this to use the right css variable should fix that issue :)
[RuntimeStatus.Idle]: POSITRON_CONSOLE_STATE_ICON_DISCONNECTED, | |
[RuntimeStatus.Idle]: POSITRON_CONSOLE_STATE_ICON_IDLE, |
className={`codicon ${icon}`} | ||
style={{ color }} | ||
/> | ||
<RuntimeStatusIcon status={runtimeStatus} /> |
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.
Thanks for doing the work to make the runtime and runtime status icons into reusable components. This is so much nicer!
export const RuntimeStatusIcon = ({ status }: RuntimeStatusIconProps) => { | ||
const iconClass = statusToIconClass[status]; | ||
const iconColor = statusToIconColor[status]; | ||
const iconColorCss = asCssVariable(iconColor); |
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 didn't know about these handy helpers! This makes it so much easier to keep the css maintainable and find all the different uses of a theme color!
{/* TODO: Runtime name or session name? */} | ||
<p className='session-name'>{runtimeName}</p> |
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 the todo is in regards to what to display here (and not about renaming the css class), then I think runtime name makes a lot of sense!
The runtimeName
field should really be used to reflect the runtime and the sessionName
is used for arbitrary labels (and can change). For this use case, I think the user cares about the specific runtime. Another thing to note, but it looks like the session name for notebooks defaults to the name of the file which isn't really useful in the kernel status badge anyways.
We may want to rethink this if we decide to enable console.showNotebookConsoles
and fully flesh it out as a feature. In the console, we show the session name and not the runtime name (which makes sense there). We would probably want to think about how we can communicate that this kernel status badge is reflecting the same/similar information as the console pane.
/* TODO: Use margin? */ | ||
gap: 4px; |
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.
Using gap here makes sense over margin if the intention is to add equal spacing to the children elements of positron-notebook-kernel-status-badge
which I think is what you are trying to do.
If you were to replace this with margin, you'd need to add that CSS to the RuntimeStatusIcon
CSS file but I wouldn't recommend that because then we'd need to override the margin if used in other places that needs different gutter spacing.
I would probably change this to use column-gap
here as well to be extra specific about the spacing you are targeting:
/* TODO: Use margin? */ | |
gap: 4px; | |
column-gap: 4px; |
order: 30, | ||
title: { value: localize('showConsole', 'Show Console'), original: 'Show Console' }, | ||
id: MenuId.PositronNotebookKernelSubmenu, | ||
// group: 'kernel', |
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.
Can we just remove this line instead of commenting it out, or do we need it for some future work?
category: NOTEBOOK_ACTIONS_CATEGORY_POSITRON, | ||
title: localize2('positronNotebookActions.selectKernel', 'Select Positron Notebook Kernel'), | ||
icon: selectKernelIcon, | ||
title: localize2('positronNotebookActions.selectKernel', 'Change Kernel...'), |
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 nit, but I think renaming the localized string key here makes sense - not a blocker though!
title: localize2('positronNotebookActions.selectKernel', 'Change Kernel...'), | |
title: localize2('positronNotebookActions.changeKernel', 'Change Kernel...'), |
localize('positron.notebook.workingDirectoryChanged.title', 'Update working directory?'), | ||
localize( | ||
'positron.notebook.workingDirectoryChanged', | ||
// eslint-disable-next-line local/code-no-unexternalized-strings |
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.
Thanks for adding that. I was wondering about this error and if it was something we could ignore or if we needed to change this string.
Add the Restart Kernel button to Positron notebooks, addresses #9789.
Screen.Recording.2025-10-10.at.19.02.37.mov
Release Notes
New Features
Bug Fixes
QA Notes
@:notebooks