Skip to content

Be more discriminating in failure popups#1022

Open
jmcphers wants to merge 1 commit into
mainfrom
bugfix/run-cell-popup
Open

Be more discriminating in failure popups#1022
jmcphers wants to merge 1 commit into
mainfrom
bugfix/run-cell-popup

Conversation

@jmcphers

Copy link
Copy Markdown
Collaborator

This change prevents superfluous error notifications/toasts when running cells in Positron.

The issue was that we were not catching exceptions thrown by executeCode. It is appropriate for exceptions to propagate here when we can't execute the code for some structural reason, but most often the failure is caused by the code itself.

When a cell fails to execute (a runtime/syntax error, an interrupt, or the session exiting) Positron already reports it in the console. Previously the rejection from executeCode also propagated up to the command handler, which surfaced it a second time as a notification popup.

This change distinguishes the two failure modes via the execution observer's onFailed callback:

  • Runtime-reported failures (the code ran and errored): logged at debug level and swallowed so they aren't shown twice. Subsequent blocks are skipped since one failed.
  • Submission failures (e.g. no runtime registered for the language): logged as an error and re-thrown so the user is notified, since these aren't otherwise surfaced.

Fixes posit-dev/positron#9845

@jmcphers jmcphers requested a review from juliasilge June 22, 2026 23:13
@posit-snyk-bot

posit-snyk-bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@juliasilge juliasilge left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working great! 🎉

Image

Before you merge, can you please update the CHANGELOG with a note on this fix? You might need to merge from main so you get the CHANGELOG entry under the right version (we released yesterday).

// the code couldn't be submitted to the runtime at all (e.g.
// no runtime is registered for the language), which is a
// genuine problem worth reporting.
let runtimeFailure = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic depends on onFailed having already fired (synchronously setting runtimeFailure) by the time we reach this catch. The ExecutionObserver docs only guarantee that one of onCompleted/onFailed is called, not their ordering relative to the executeCode promise rejecting. If that ordering ever changed or raced, a runtime error would be misclassified as a submission failure and we'd surface the popup again. Just noting the dependency for future readers; the failure mode is only a graceful regression to the old behavior, so nothing to change here IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

quarto: errors from Run cell button generate pop up

3 participants