Add Fix/Explain buttons to Quarto inline output errors#14735
Conversation
|
E2E Tests 🚀 Why these tags?
More on automatic tags from changed files. Warning Unrecognized tag(s) in the PR description, ignored: |
PETE's assessment 🧪Verdict: 🟢 Adequate -- Both new components ship focused Vitest suites that cover the payload contract (including the Explain-only bug fix) and the full gate matrix at the cheapest viable level. What changed
Tests in this PR
Existing coverageThe new tests are the primary coverage. Suggested additionsNone. The only untested surface is the thin view-zone glue in Deployment note (optional)The Quarto quick-fix renders through browser-only view-zone glue and no surface-split ( PETE (Positron Extreme Test Experiment) - LLM-based test-coverage advisor, in pilot. Triggers on PR open and on |
jmcphers
left a comment
jmcphers
left a comment
There was a problem hiding this comment.
A few problems I had when testing this:
The height is computed incorrectly when Fix/Explain are present and the error is restored from the cache (vs from a just-executed cell). Maybe we shouldn't show these affordances on errors loaded from the cache?
I also found that these buttons sometimes disregarded the error they were given and instead found problems elsewhere in the document. We might need some additional context naming the code that produced the error, and its location in the document, to help focus the LLM.
nstrayer
force-pushed
the
feature/13228-quarto-fix-explain
branch
2 times, most recently
from
2f05fc3 to
62f7098
Compare
hide-on-reload.movUpdated so that the buttons only run when the cell has been created in that session/window open. Also updated the prompts to be more direct with what should be done in the fix situation. |
jmcphers
left a comment
jmcphers
left a comment
There was a problem hiding this comment.
LGTM. A few nits, might be worth investigating before merge
After a fresh start, the first fix/explain button I press fails. It opens the Posit Assistant sidebar but this popup shows up and the error never goes to Assistant.
Second, Assistant is kind of bad at fixing Quarto problems. Try creating these two cells:
```{r}
library(ggplot2)
```
```{r}
ggplot(faithful, aes(x = eruptions, y = waiting)) +
geom_point()
```
Explain the error:
Assistant says:
In Quarto, each code chunk is typically executed in isolation unless they're part of the same rendering session. The issue is that the library(ggplot2) call in the preceding chunk loads the package into that chunk's execution context, but the functions from ggplot2 may not be available in the next chunk
This is wrong and I would have expected Assistant to tell me that the problem is that I've run the chunks out of order. Assistant also thinks this is true of R Markdown files ("each cell is run in isolation") and encourages me to "fix" the problem by combining cells.
Maybe it needs to learn more about interactive execution, but probably that's outside the scope of this PR!
|
I agree those are out of scope for this pr but I made followup issues in the assistant repo for them! |
Extract a shared AssistantErrorQuickFix component from the notebook quick-fix, then wrap it in a thin QuartoOutputQuickFix that gates on ai.enabled + posit-assistant.hasChatModels and attaches the failing chunk's code and location to the assistant payload. - Resolve cell context at render time (code, language, path, line range) - Suppress buttons on cache-restored errors (structural: the cache- restore path sets fromCurrentSession=false on the view zone) - aria-live='off' on the quick-fix container so async button mounts inside the output's role='log' region are not announced - Add findCellByPreviousId to the document model for cell-remap support - Explain prompt appends a constraint preventing agentic fixes
The stash-restore path (tab close + reopen in the same session) was keeping fromCurrentSession=true and resolving cell context, which caused buttons to reappear on errors that were no longer freshly executed. Treat stash restores the same as cache restores: suppress the buttons.
The view zone defaulted to showing buttons and required restore paths to explicitly suppress them. Flip the default: buttons are suppressed unless the live execution path calls enableQuickFix(context?). This removes one field, one setter, and the need for restore paths to opt out.
nstrayer
force-pushed
the
feature/13228-quarto-fix-explain
branch
from
08149a6 to
68960b8
Compare
2 participants
Closes #13228
This PR adds the same Fix/Explain buttons that the Positron Notebook Editor gets for cell errors to Quarto inline outputs. The buttons were extracted from the notebook quick-fix into a shared
AssistantErrorQuickFixcomponent, so both surfaces can share one implementation. This also let us fix a latent problem (Explain was fixing errors too) in one place for both.Agent generated notes 👇
Summary
The notebook's Fix/Explain implementation was tangled with notebook-only gating, so this PR splits it in two:
AssistantErrorQuickFix(shared, presentational): the split buttons, ANSI cleanup, error attachment, andposit-assistant.newChatwiring. No gating -- callers decide whether to render it.NotebookCellQuickFix(unchanged gates) and the newQuartoOutputQuickFix.Key decisions:
ai.enabledsetting (read live; unset counts as enabled) plus theposit-assistant.hasChatModelscontext key. It deliberately drops the notebook-only gates (positron.notebook.enabled,notebook.ai.enabled) -- Quarto inline output works regardless of whether the Positron notebook editor is enabled.posit-assistant.newChathas no mode argument today.)AssistantErrorQuickFixstays inpositronNotebookeven thoughpositronQuartoimports it. The tidy home would be thepositronAssistantcontrib, but that's on a deprecation path; a shared home can be created when the assistant migration needs one.`${outputId}::quick-fix`), so the existing disposal sweep tears it down on clear/re-execute/dispose.The console's error quick-fix still hand-rolls its own version (different UX: plain buttons, always continues the current chat); unifying it is deliberately out of scope.
In-Action
quarto-fix.mov
Release Notes
New Features
Bug Fixes
Validation Steps
@:quarto @:positron-notebooks @:posit-assistant
.qmdwith an erroring chunk (e.g.test/e2e/test-files/workspaces/quarto_inline_output/r_errors.qmd) and run the chunk.ai.enabled: false(or quit the assistant so no chat models are available) and re-run: no buttons should render.Unit coverage:
AssistantErrorQuickFix.vitest.tsx(chat payload contract) andQuartoOutputQuickFix.vitest.tsx(gate matrix).