feat: redesign notebook cell toolbar with view toggles and auto-refresh intervals

[emrberk]

Reworks the notebook cell experience and adds MCP bridge version awareness.

Cell toolbar & views

• Width-aware toolbar that adapts to the cell's width (compact → standard → expanded).
• Table ⇄ chart view toggle that transfers existing data between the grid and the chart without re-running the query.
• Inline cell rename; the cell name is now the single source of the chart title (migrated from the old chartConfig.name on load).
• Per-cell auto-refresh: adaptive, off, or a fixed interval (1s/5s/10s/30s/1m).

MCP bridge

• Detects a bridge version mismatch — a major mismatch blocks with an upgrade command, a minor one connects but warns — surfaced in the pairing popover, consent modal, and footer status pill.

Internal

• Shared dropdown/context menu styles + dropdown theme tokens; ctrlCmd/altOption centralized in utils/platform; useWidthObserver split out of useContainerWidth.

Adds unit tests for the new utilities (auto-refresh tokens, view/tier resolution, cell-height math, name migration, version-mismatch parsing).

Resolves:

• Pairing UI: show the expected @questdb/mcp-bridge version (install hint) and warn on version mismatch #573
• Notebook layout: responsive chart/grid/cell action bars (overflow menu) + sidebar collapse that keeps focus on the notebook #574 (part A)

Tandem PR: questdb/mcp-bridge#1

🤖 Generated with Claude Code

[emrberk]

Code Review — PR #581

feat: redesign notebook cell toolbar with view toggles and auto-refresh intervals

Reviewed at level 3 (all 13 review agents + Agent 13 fresh-context adversarial + per-finding verification + full quality gate).

Quality gate — all green: yarn typecheck, yarn lint, yarn test:unit, yarn build all pass (exit 0). Cross-context pass found zero broken callsites and tsc --noEmit is clean repo-wide.

---

Issues

#1 — Grid⇄chart transfer & poll orchestration is untested; no notebook e2e

• Category: Test review & coverage
• Severity: Moderate
• Location: in-diff — DrawCanvas/index.tsx, e2e/tests/
• Description: The riskiest new logic — mount-transfer, skipNextPollFetchRef/pollEnabledRef coordination, and mirrorToCellResult (the data moved between the grid and the chart) — lives inside the DrawCanvas component and is verified only at the predicate level (resultMatchesQueries, resultsEquivalent). vitest runs in node env (no jsdom), so the orchestration is currently untestable as written, and there are no notebook / cell-toolbar e2e specs at all. A regression in the transfer/skip logic (stale data, duplicate fetch, dropped tabs) would be invisible to CI.
• Steps to reproduce: N/A (coverage gap).
• Suggested fix: Extract the mount decision into a pure planMount(cellResult, snapshot, queries) → { action, skipPoll } util and unit-test it (matches the repo's "extract critical logic to utils" convention). Add an e2e that runs a grid cell, toggles to chart, asserts no new network request + same rows, toggles back, asserts the grid tabs match.

#2 — No cancel affordance for a running query in the compact tier

• Category: Code structure / UX
• Severity: Minor
• Location: in-diff — Cell.tsx:754
• Description: In the compact tier the header right slot is null, so CellRunDrawToggles (the only component carrying the Cancel button/spinner) is never rendered, and the "More actions" menu has no cancel item. A long-running query in a narrow (e.g. grid-mode) cell has no inline way to cancel — the user must first maximize the cell.
• Steps to reproduce:
  • Put a cell in a narrow column (compact tier, < 480px).
  • Run a slow query.
  • There is no Cancel button; cancelling requires maximizing the cell.
• Suggested fix: Surface a cancel control in the compact tier while isRunning (e.g. swap the maximize button for a cancel, or add a menu item).

#3 — Transient: draw-cell first paint drops the loading spinner; toggle shows active state mid-hydration

• Category: React correctness & hooks
• Severity: Minor
• Location: in-diff — DrawCanvas/index.tsx:487-505, Cell.tsx:294
• Description:
  • (a) EventBus has no replay. On a cell that mounts directly in draw mode, React runs the child DrawCanvas publish effects before the parent Cell's useChartLoading/useChartZoomed subscribe effects, so the initial loading: true is lost — the spinner is absent during the first fetch. It self-corrects on the terminal loading: false.
  • (b) While expectingResult is true but the snapshot hasn't hydrated yet (view === "none", runActive === true), the Run toggle shows its active state before the result is actually visible — a brief inconsistency that self-corrects once hydration completes.
  • Both are transient (sub-second) and cosmetic.
• Steps to reproduce: Reload a notebook whose cell is a draw cell → no spinner on the first fetch. (b) is only observable during the sub-second hydration window.
• Suggested fix: Subscribe in useLayoutEffect (or publish from one) so the parent listens before the child publishes; derive the toggle's active state so it isn't shown before cell.result is present.

#4 — Grid mirror loses the real error message and can diverge from a real run

• Category: Query execution & data integrity
• Severity: Minor
• Location: in-diff — DrawCanvas/index.tsx:190-198, DrawCanvas/index.tsx:305-313
• Description: When a chart fetch statement fails, errorExecResult substitutes a generic "Query failed", so switching chart→grid shows that instead of the server's message a real run would display. Separately, the mirror runs statements concurrently and mirrors every slot, whereas a real multi-statement run is sequential and marks post-error statements cancelled; and the snapshot-seed path (reload) mirrors only DQL-with-rows, dropping error/empty/DDL tabs the grid would otherwise show. All low-reachability and non-corrupting.
• Steps to reproduce: Draw a multi-statement cell where one statement transiently errors, then toggle to the table → see "Query failed" / a different tab set than a real run.
• Suggested fix: Carry the real error message through the mirror; optionally short-circuit slots after the first error to match run semantics.

#5 — Accessibility gaps in the new toolbar / version-mismatch UI

• Category: Accessibility & UX
• Severity: Minor
• Location: in-diff — CellViewToggle.tsx, CopyableCommand.tsx, PairPopover.tsx
• Description:
  • (a) The Table/Chart segmented control is two independent aria-pressed buttons in a plain <div> — no role="group"/radiogroup, so screen-reader users don't perceive "1 of 2 selected".
  • (b) The icon-only CopyButton in CopyableCommand relies on title only (no aria-label), and the <code> command isn't keyboard-selectable.
  • (c) The error/version StatusRow uses bare role="alert" while the connecting row uses the more reliable role="status" aria-live.
  • (d) Active toggle state is conveyed by a subtle background-color delta only (verify 3:1 UI-component contrast).
• Steps to reproduce: Navigate the cell toolbar / pairing popover with a screen reader / keyboard.
• Suggested fix: Add role="group" (or radiogroup) to the segmented control; add aria-label to the copy button and tabIndex={0} to <code>; add aria-live="assertive" to the alert rows; strengthen the active-state non-color cue.

#6 — Auto-refresh submenu trigger lacks a leading icon (menu misalignment)

• Category: Styling & theming
• Severity: Minor
• Location: in-diff — CellToolbar.tsx:340-354
• Description: The "Auto-refresh (…)" DropdownMenu.SubTrigger renders text with no MenuItemIcon, while sibling items (Refresh now, Chart settings) have leading 16px icons, so its label starts in the icon column and is left-misaligned.
• Steps to reproduce: Open a chart cell's "More actions" menu at the standard tier → the Auto-refresh submenu row is misaligned.
• Suggested fix: Give the SubTrigger a leading icon (e.g. ArrowClockwiseIcon) or an empty MenuItemIcon spacer for column alignment.

#7 — Minor code-quality: redundant cellId prop, dead inline branch, duplicated refresh routing, UI/AI draw-entry divergence

• Category: Code structure & readability
• Severity: Minor
• Location: in-diff — CellToolbar.tsx, CellDragHeader.tsx, notebookAIBridge.ts
• Description: CellToolbar/CellDragHeader take both cellId and cell (always cellId={cell.id}) — two props that must stay in sync. CellToolbar.inline is always passed inline, leaving the non-inline $inline styled branch dead. handleRefreshNow (CellToolbar) duplicates handleRefresh (CellRefreshButton) routing. And entering draw from the UI (handleDrawClick) no longer forces isViewMaximized = false, while the AI bridge setCellMode still does — so the same action yields a different layout depending on entry point (likely intended given the "view-maximize persists" design, but the two paths diverge).
• Steps to reproduce: N/A (code review).
• Suggested fix: Drop the redundant cellId prop (read cell.id); make inline required or remove the dead branch; share the refresh-routing helper; align the AI-path draw-entry with the UI behavior (or confirm the divergence is intended).

---

False-positives

Draft findings that were investigated and dismissed:

Category	Reported issue	Why it's not a bug
Query execution & data integrity	resultsEquivalent O(1) fast-path could suppress a needed chart update (show stale data)	The new fast-path block only ever does early return false (force-update); it never returns true or skips the authoritative O(rows×cols) walk. It can only make updates more frequent. Verified in drawCanvasUtils.ts:85-104.
Query execution & data integrity	resultMatchesQueries could transfer stale (edited-but-not-rerun) SQL data into the chart	It rejects on per-statement normalizeQueryText mismatch and count mismatch, and the transfer pipes through successResults (DQL-with-rows only). Stale/error/empty frames yield [] → falls back to fetch.
React correctness / Async	Cmd/Ctrl+Enter could run twice (window listener + Monaco action)	Mutually exclusive: the window handler bails when editorContainerRef.current?.contains(document.activeElement), true exactly when Monaco's own keybinding fires. Only the single focused/maximized cell registers a window listener.
State & context / Performance	mirrorCellResult persists / leaks chart data into saved notebook state	It uses hydrateCells (non-persisting by design), and stripCellResults strips result from the persist payload anyway. No churn, no leak.
Async, timers & cancellation	Double fetchAll on mount via the stale pollEnabledRef read	Only triggers when bufferId === undefined, unreachable in practice (notebook buffers always have numeric Dexie ids); on the real path an await lets the ref-sync effect run first, and the up-front abort/supersede prevents any stale-data corruption regardless.
Persistence & migrations	Missing migration for isChartMaximized → isViewMaximized on old persisted notebooks	Intentional, confirmed decision — losing a maximized-view boolean is cosmetic and there is deliberately no upgrade migration.
State & context	CHART_ZOOM has no unmount reset (could leave useChartZoomed stuck true)	Both consumers gate the reset-zoom control on view === "chart", only true while DrawCanvas is mounted; re-entering draw remounts and re-publishes the real isZoomed. The stale flag is always masked.
State & context	MCPBridgeProvider / NotebookActions context values could be unstable / miss versionMismatch	The provider useMemo deps include versionMismatch (and every field); the actions value is a stable proxy over liveActionsRef with auto-included new keys.
Browser compatibility & security	Major-version bridge mislabelled "minor" if it acks instead of closing 4004	Only reachable if the bridge violates its own contract (a corrupted/misbehaving bridge). The bridge is localhost/non-adversarial and validates its own tool args, so this is not a real-world issue.

---

Summary

Verdict: approve with minor follow-ups. This is a large (80-file) but well-engineered redesign — strong util extraction, proper discriminated unions, thorough unit tests for the pure logic, and a fully green quality gate. The cross-context pass confirmed every changed contract (the isChartMaximized → isViewMaximized / chartConfig.name → cell.name / autoRefresh widening / interface renames / new MCP tools) is consistent at all callsites, with no broken consumers.

• Nothing Critical or blocking. The one Moderate item is a coverage gap (wip test for error range #1: the riskiest grid⇄chart transfer/poll logic is untested and there are no notebook e2e specs). Everything else is Minor.
• Tradeoff to confirm: the shared menuStyles consolidation changes the menu hover-highlight from teal (tableSelection) to neutral gray (background) across all menus app-wide (schema context menu, every dropdown, help). Intended redesign — just confirm the design sign-off.
• Findings: 7 verified, 9 draft findings dropped as false positives.
• In-diff vs out-of-diff: 7 in-diff, 0 out-of-diff. The zero out-of-diff count is expected here, not an underrun signal — the cross-context pass ran a wide grep sweep and tsc is clean, confirming the mechanical renames were applied exhaustively and no unchanged consumer was left reading an old contract.