-
Notifications
You must be signed in to change notification settings - Fork 130
Add a button to stop a terminal process from the Viewer pane #10625
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
|
E2E Tests 🚀 |
| "runtime", | ||
| "terminal" |
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 think of any other source we want to track, other than something running in the terminal and something running in the console?
| } | ||
|
|
||
| .preview-action-bar .action-bar-button-icon.interrupt::before { | ||
| color: var(--vscode-errorForeground); |
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.
Same as the button in the console
| // Send Ctrl+C (SIGINT) to the terminal | ||
| sourceTerminal.sendText('\x03', false); | ||
|
|
||
| // Poll after 3 seconds to check if the process actually stopped |
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 you use this polling to dismiss the button, it feels quite laggy unfortunately. I think we need to dismiss the button immediately but set up the polling to show an error, in case something goes wrong.
src/vs/workbench/contrib/positronPreview/browser/components/urlActionBars.tsx
Outdated
Show resolved
Hide resolved
| export interface IRuntimeClientEvent { | ||
| name: UiFrontendEvent; | ||
| data: any; | ||
| data: unknown; |
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 is from the 106 merge, with a new (?) no-explicit-any eslint rule.
| // --- Start Positron --- | ||
| 'src/positron-dts/**', | ||
| // --- End Positron --- |
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.
The 106 merge added a new (I think) no-explict-any eslint rule, but I don't think we should apply it to our API file, similar to how the VS Code API file is treated.
The test failures seem to be related to very slow app start up... 🤔 I noticed some new useEffects hooks in the code. Code that possibly be related and/or running on startup? I'll take a closer look at your new test on Monday! Figuring out the app lag is higher priority. 🫠 Also, I'd update your branch with |
jmcphers
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.
|
I did merge from
So seems like those failures are not related to my changes here. |
Yeah, that worked out pretty well in 3c36ebb, adding the same thing that would happen if you clicked the button that says "Clear the current URL". I do want to highlight that there is a bit of messiness right now around how many previews there can be, but if you are using the preview pane (where this button is), every time a new URL or HTML preview is opened, it clears all previous previews first anyway. I think this is a good option for now! |
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.
Test looks great! One small suggestion, instead of creating a separate test, I would suggest adding the interrupt-button checks to the end of the existing streamlit app test. That way we get the coverage without the extra setup/teardown cost.
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.
HA, I tried to do this and then remember why I had it in a separate test from before! 😆
- The first test runs the Streamlit app in the viewer, then pops it to the editor to run there
- My new test runs the Streamlit app in the viewer, then stops it with the new button
We do in fact need two separate tests here.
Yep, you are right, these are issues in |
|
All the tests (including my new one) in The failures I see here are the same as on
I'm going to go ahead and merge so we can keep moving forward. |

Addresses #7747
@:apps
@:viewer
This PR extends the UI comm for previewing a URL to optionally keep track of the "source" of where that URL may be coming from, for example, the terminal that may be running that process. If we have such a process ID, then we show a new button the Viewer pane UI that can be used to stop the process. It's the same button that we use on the console to stop running code there.
I tried a few hacky things that might allow us to show such a button without truly tracking the source, but it did NOT result in good behavior. We can do PRs to the Shiny extension and the Quarto extension so they use this new API and can also show buttons that will stop their processes as well.
We also can extend this to process that come from the language runtimes, but I did not do that here. Do people want two buttons in such a case? The button that would be running in the console and the button in the viewer? A little weird maybe? I say let's wait and see what folks think.
Release Notes
New Features
Bug Fixes
QA Notes
If you have an app supported by the builtin
positron-run-appextension, when you press the "play" button, you should see a new button pop up on the Viewer pane.Be aware that we do not have clearing set up in the Viewer pane, i.e. #6827, #9323, #5344. Adding the button here might make that feel more urgent.