Skip to content

Show render tag warnings alongside query results#889

Merged
kylenesbit merged 1 commit into
mainfrom
monty/surface-render-warnings
Jul 20, 2026
Merged

Show render tag warnings alongside query results#889
kylenesbit merged 1 commit into
mainfrom
monty/surface-render-warnings

Conversation

@mlennie

@mlennie mlennie commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

What

Every query response already carries renderLogs — the render tag validation findings for that result — and has since #635. It's in api-doc.yaml, in all four generated clients, and the MCP execute_query tool already reports it, so an AI agent gets told when a tag is wrong. Nothing in the UI ever read it, so a human got a broken chart and no explanation. The only way to find out was the network tab, which is exactly how this surfaced: a user hit a blank chart and had to open devtools to discover the renderer had been telling us the whole time.

This surfaces those findings as a non-blocking note over the result.

Above: a bad tag renders a blank chart, now with the reason attached. Below it, a clean query, no note.

Design: annotate, never withhold

Severity describes the tag defect, not whether the chart drew, and it fails to predict the render in both directions. The case that prompted this was severity: "warn" and produced a blank chart. Meanwhile a benign warning can accompany a perfectly good chart, and error-severity findings coexist with a drawn result — model.ts notes they affect "only how a field renders, never whether the model compiles or a query runs".

So any rule that used severity to withhold a result would be guessing, and wrong either way: it would blank charts the renderer is actively drawing, replacing a visible result with nothing. Annotating is wrong in neither direction. A blank chart plus a note is recoverable — the user reads it and fixes the tag. A withheld good chart is not.

This also keeps faith with #861, which deliberately made render findings non-fatal after the fatal treatment caused package-load failures and a control-plane retry loop. debug and info are dropped; only warn and error are worth a user's attention.

Shape

ResultContainer takes an optional renderLogs?: LogMessage[]. summarizeRenderLogs (a pure helper, unit tested) filters to warn/error entries that have a message, dedupes them, and picks the worst severity — every LogMessage field is optional in the generated client, so entries without a severity or message are dropped rather than rendered as an empty tooltip.

The note is an overlay, not a sibling. ResultContainer measures renderedHeight and feeds it to RenderedResult, so a block-level element would change the height it just measured and shift every embedding surface.

It's a button rather than a bare icon for two reasons. MUI's SvgIcon renders focusable="false" and aria-hidden="true", so a bare icon can't take focus and never reaches the accessibility tree — the tooltip is the only channel for this message, so a keyboard or screen-reader user would get nothing at all. And it carries the same translucent backdrop as the sibling overlays in ModelCell and NotebookCell; the comment at NotebookCell.tsx:463-470 records why ("the white-on-white made it invisible").

QueryResult, ModelCell and ResultsDialog execute queries and pass their findings through. NotebookCell and MutableCell render a stored cell.result from executeNotebookCell, whose NotebookCellResult schema has no renderLogs, so they have nothing to pass and the prop stays optional.

Not covered

  • Notebook cells get no findings at all, because the notebook endpoint never returns them. Closing that means adding renderLogs to NotebookCellResult and regenerating the clients — a bigger change, happy to do it separately if you want it.
  • The query builder preview (SourcesExplorer) renders through @malloydata/malloy-explorer's own ResultPanel rather than ResultContainer, so this doesn't reach it.

Tests

Six unit tests on the summarizer (bun test, 70 pass), covering the real warning payload, worst-severity selection, dropping debug/info, dropping entries with missing optional fields, and dedupe.

Verified live against a package with a deliberately bad tag: the wire returned severity: "warn", "Unknown render tag 'viz.stack.y' on field 'root'"; the note appeared with that exact text; a clean query in the same model produced no note.

Two traps for anyone adding a Playwright test later. svg[data-testid="InfoOutlinedIcon"] matches nothing in a production build, since @mui/icons-material only emits data-testid when NODE_ENV !== "production" — locate by role and accessible name instead. And assert keyboard reachability with a real keyboard.press("Tab"), not locator.focus(): MUI's Tooltip gates its focus listener on :focus-visible, which programmatic focus doesn't set, so focus() looks like a failure when the behaviour is correct.

Every query response already carries `renderLogs`, the render tag validation
findings for that result, and the MCP execute_query tool already reports them.
Nothing in the UI read them, so a misconfigured tag produced a broken chart
with no explanation: the only way to find out was the network tab.

Surface them as a non-blocking note over the result. Severity describes the tag
defect rather than whether the chart drew, so a finding is never used to
withhold a result: an error-severity tag still renders, and a warning can still
blank a chart. Annotating is correct in both directions; withholding is not.

The note is a button rather than a bare icon so the message is reachable by
keyboard and exposed to a screen reader, and carries the same translucent
backdrop as the sibling overlays in ModelCell and NotebookCell.

Callers that execute a query pass their findings through. Notebook and workbook
cells render a stored result whose response has no renderLogs, so they have
none to pass and the prop stays optional.

Signed-off-by: Monty Lennie <[email protected]>

@kylenesbit kylenesbit 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.

Approve. The annotate-don't-withhold decision is correct given severity describes the tag defect and doesn't predict whether the chart drew. The overlay avoids the measured-height feedback loop, summarizeRenderLogs is a pure and well-tested helper, and the prop plumbing is complete with honest scoping (Notebook/Workbook cells correctly omitted since their result has no renderLogs).

Optional nits only, none blocking:

  • Consider MUI's Warning/Error icons instead of a colored InfoOutlined glyph (also more consistent with the large-result overlay in this same file).
  • summarizeRenderLogs runs before the early returns; moving it below (or useMemo) is marginally cleaner.
  • Confirm bun:test matches the SDK's test-runner convention.

@kylenesbit
kylenesbit merged commit bea8741 into main Jul 20, 2026
14 checks passed
@kylenesbit
kylenesbit deleted the monty/surface-render-warnings branch July 20, 2026 02:10
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.

2 participants