feat: notebook-level auto-refresh interval with per-cell overrides

[emrberk]

Adds a notebook-level auto-refresh default for draw-mode charts that individual cells inherit and can override.

What's changed

• Notebook default — a new toolbar control sets one auto-refresh cadence (Auto / Off / 1s / 5s / 10s / 30s / 1m) for the whole notebook. Any chart without its own setting inherits it.
• Per-cell overrides — each chart's auto-refresh dropdown gains a "Notebook default" (inherit) option alongside the fixed choices. A cell that overrides the default is marked with a dot, and its override always wins over the notebook default.
• Refresh all charts — one click refreshes every chart in the notebook, superseding any in-flight background poll.
• Reset cell overrides — clears every per-cell override so all charts fall back to the notebook default.

AI / MCP

• New set_notebook_autorefresh tool to set the notebook default.
• set_cell_autorefresh now accepts null to clear a cell's override (inherit the default).
• apply_notebook_state gains an auto_refresh_default field, and the notebook snapshot reports the effective default to the agent.

Migration

• Existing notebooks are migrated on load: the notebook default is inferred from the charts' current cadences and redundant per-cell values are dropped. This is lossless — no chart changes how it actually polls.

Tandem PR: questdb/mcp-bridge#2

🤖 Generated with Claude Code

[emrberk]

Review: PR #595 — feat: notebook-level auto-refresh interval with per-cell overrides

Level 3 — full mission-critical pass: all 13 agent mandates across 10 parallel agents + a fresh-context adversarial pass + per-finding verification.
Reviewed at PR head 0e34b14 (working tree checked out to head for verification).
Repo: questdb/ui · Branch: feat/notebook-level-refresh-interval → main

---

PR conventions

• Title is valid Conventional Commits (feat: …) and states real end-user impact.
• Body is clear and structured (What's changed / AI-MCP / Migration / Tandem PR).
• No description-verb or implementation-leak issues.

Quality gate (run at head 0e34b14)

Check	Result
yarn typecheck	✅ pass
yarn lint	✅ pass
yarn test:unit	✅ pass (1116 tests)
yarn build	✅ pass

No quality-check findings.

On the prior review: the earlier PR comment (head 3971667) flagged a jitter/refresh-all race (#1) and a phantom override count (#2). Commit 5121dd9 fixes both, with tests — both fixes verified correct. Finding #1 below is the residual of that jitter fix, not the original bug.

---

Issues

All findings are Minor — there are no Critical or Moderate defects. All are in-diff.

#	Issue	Category	Severity	Location
1	Refresh-all click stranded on visible flip-to-Off mid-jitter	Async / Query-integrity	Minor	in-diff
2	autoRefreshDefault? optional breaks prop-chain contract	Structure & types	Minor	in-diff (latent)
3	Disabled "Refresh charts" button gives no tooltip/reason	Accessibility & UX	Minor	in-diff
4	Ragged left edge: reset item vs radio options	Styling & theming	Minor (borderline)	in-diff
5	Reset clears/logs more overrides than the toolbar discloses	State & context	Minor (cosmetic)	in-diff
6	New UI/store/snapshot wiring untested at its real layer	Test coverage	Minor	in-diff
7	Inherit-row override dot has no screen-reader text	Accessibility	Minor (borderline)	in-diff

#1 — Refresh-all click silently stranded when a chart flips to Off during the start-jitter window (visible cell)

Location: chartRefreshEngine.ts:572–579 (the updatePoll re-arm from 5121dd9) + :356 (resume, the sole redeemer).

Impact: the explicit "Refresh all charts" gesture does nothing for a chart if that chart's effective cadence flips to Off within the 0–300 ms poll-restart jitter while the cell stays visible. pendingManualRefresh is read only in resume(), and resume() fires only on a visibility transition (:188 tab-show, :337 reveal) — never for a cell that never hides — so the parked click is never redeemed. The chart shows the pre-click frame with no spinner and no error. Bounded: no wrong or lost persisted data; self-heals on the next scroll-away/back, tab switch, or another click.

Steps to reproduce:

1. A visible chart inheriting a polling default (e.g. 30s), settled.
2. Click Refresh all charts (poll restarts, sleeping in jitter, inFlight still null).
3. Within ~300 ms set the notebook default (or that cell's override) to Off.
4. Leave the cell visible — it never refetches.

Reachability is low: refresh-all is a user-only gesture (no AI tool drives it), and a human can't realistically open the dropdown and pick "Off" within 300 ms of the refresh click; the only sub-300 ms path is a coincidental AI set_*_autorefresh(false) landing in that exact window. Hence Minor (the async agent argued Medium; the query/data agent independently rated Minor, which the reachability supports). It is the one item genuinely worth fixing before merge.

Suggested fix: in the re-arm, redeem immediately when the cell is still live instead of parking it — mirroring forceRefresh's own non-poll branch:

if (entry.manualRefreshInFlight && !entry.inFlight) {
  if (entry.visible && !this.documentHidden) {
    entry.manualRefreshInFlight = false
    void this.fetchOnce(entry)          // Off-flip while visible: honor the click now
  } else {
    entry.manualRefreshInFlight = false
    entry.pendingManualRefresh = true   // hide/tab-switch: park for resume()
  }
}

#2 — autoRefreshDefault? optional on two links of the prop chain

Location: cells/CellToolbar.tsx:89, cells/CellDragHeader.tsx:61 type it autoRefreshDefault?: AutoRefresh (optional) while Cell, CellWideActions, CellRefreshButton, and index.tsx CellViewProps require AutoRefresh | undefined.

Impact: currently inert — the only omitting caller is MarkdownCell.tsx:372, and markdown cells resolve to view "none", so the value is never used (verified). It is a latent footgun: a future draw-capable caller that forgets the prop compiles cleanly and silently resolves to adaptive, ignoring an Off/interval notebook default with no compile error.

Steps to reproduce: N/A at runtime today — latent type-safety gap.

Suggested fix: make the prop required (autoRefreshDefault: AutoRefresh | undefined) on CellToolbar and CellDragHeader to match the rest of the chain, and have MarkdownCell pass undefined explicitly.

#3 — Disabled "Refresh charts" button gives no tooltip/reason

Location: NotebookRefreshControl.tsx:78–88.

Impact: when the notebook has 0 draw cells the refresh-all SplitSide is disabled inside a bare <Tooltip>. Native disabled buttons swallow pointer events, so the "Refresh charts" tooltip never opens, the button drops out of tab order, and SR users get "unavailable" with no rationale — exactly when an icon-only button most needs explaining. The design system already solves this with Button.disabledTooltip (used in VariablesPopover) or the TooltipButton span-wrap used elsewhere in this same toolbar.

Steps to reproduce:

1. Open a notebook with no draw/chart cells.
2. Hover the (disabled) refresh-all button → nothing explains why it's disabled.

Suggested fix: use Button's disabledTooltip prop (e.g. "No charts to refresh"), or wrap like NotebookToolbar's TooltipButton. (Pre-existing twin at CellRefreshButton.tsx:71, out-of-diff.)

#4 — Ragged left edge: reset item vs radio options

Location: NotebookRefreshControl.tsx:116–128 + components/DropdownMenu/index.tsx:36–54.

Impact: radio-option labels sit at ~2.8 rem (0.8 pad + 1.4 rem indicator slot + gap); the plain "Reset cell overrides" Item puts its text at ~2.0 rem, so it hangs ~0.8 rem left of the option labels above it — a ragged text edge under the divider. Borderline (a command item below a divider reading out from a radio group is a common menu convention).

Steps to reproduce:

1. Set a per-cell override so the notebook dropdown shows the reset item.
2. Open the notebook auto-refresh dropdown → the reset row's text isn't aligned with the option rows above it.

Suggested fix: lead the reset Item with a matching 1.4 rem centered slot (reuse the RadioItemIndicator width), or accept as intentional.

#5 — Reset clears/logs more overrides than the toolbar discloses

Location: NotebookRefreshControl.tsx:44 (uses countActiveAutoRefreshOverrides, draw-only) vs NotebookProvider.tsx:743–751 (resetAutoRefreshOverrides clears + trackEvent-logs countAutoRefreshOverrides, all modes).

Impact: with a dormant autoRefresh key on a run-mode cell (a chart override set, then the cell toggled to Run — cellModeChangePatch doesn't strip it), "Reset cell overrides" silently clears one more setting than the "N cells currently override" subtitle discloses, and the telemetry count exceeds the shown count. Cosmetic/telemetry only — no wrong persisted state results; the all-modes clear is intended cleanup so a dormant key can't resurrect on a later draw-switch.

Steps to reproduce:

1. Create a draw/chart cell, set its interval to 5s (creates an override).
2. Toggle that cell to Run mode (the key becomes dormant).
3. Add another draw cell with an override so the reset item appears; click Reset → both keys clear, but the subtitle only counted the visible one.

Suggested fix (optional): report the same count for the guard, subtitle, and event (or reword the subtitle to acknowledge dormant keys).

#6 — New UI/store/snapshot wiring untested at its real layer

Location: NotebookRefreshControl.tsx, useCellsStore.ts:91–102, utils/ai/notebookSnapshot.ts:137.

Impact: the logic primitives (migration, engine refreshAll, resolve/count/clear, MCP tools, digest, import round-trip) are thoroughly unit-tested. Untested:

• Any test exercising NotebookRefreshControl (refresh-all click, default-select no-op guard + emitUserAction, reset, disabled state).
• useCellsStore.setCellRefresh(undefined) → key deleted (guaranteed only on the AI/transitions path, not the store hook the cell dropdown actually uses; a regression to updateCell(id, { autoRefresh: undefined }) would store the key and pass every existing test).
• notebookSnapshot.buildCell's inherit-omission branch (only the override→"5s" case is asserted; a regression to ?? true would pin inheriting cells as explicit true to the agent, which round-trips them into phantom overrides).

There is no in-repo E2E for the feature — e2e/tests/ has none (grep for autorefresh|refresh-all|NotebookRefresh = 0 hits), and the e2e/questdb submodule bump is the server repo (for bridge 0.3.0), not the specs. RTL is non-idiomatic here (84 .test.ts vs 1 .test.tsx), so the UI is meant to be covered by Cypress — and isn't.

Steps to reproduce: N/A (coverage observation).

Suggested fix: add an E2E spec for the toolbar control (disabled state, default change repolls inheriting cells, reset clears overrides) + a 3-line useCellsStore unit test asserting setCellRefresh(id, undefined) yields "autoRefresh" in cell === false; extend the snapshot test with an inheriting draw cell asserting auto_refresh is omitted.

#7 — Inherit-row override dot has no screen-reader text

Location: cells/AutoRefreshOptions.tsx:42 (OverrideDot on the "Notebook default" radio row).

Impact: on the inherit row, the pink dot is the only signal that "this cell currently overrides — pick this to reset," and it's a bare decorative <span> invisible to screen readers. Low impact — aria-checked still conveys the essential state (inherit not being checked already implies an override).

Steps to reproduce:

1. With a cell override active, open the cell's auto-refresh dropdown with a screen reader.
2. The "Notebook default" row announces no indication that an override is currently in effect.

Suggested fix (optional): add visually-hidden text to the inherit label when value !== undefined.

---

False-positives / dismissed concerns

Verified not to be defects (listed so the reasoning can be checked):

Category	Concern (raised by brief/agent)	Why it's dismissed
Styling / cross-context	Shared DropdownMenu.RadioItem's new always-rendered 1.4 rem checkmark slot regresses other menus	NIL blast radius — grep confirms the only repo-wide consumer of RadioItem/RadioGroup is AutoRefreshOptions.tsx. No other menu is touched. (This was the brief's flagged top-priority concern.) Net a11y improvement — the old checked-state background equalled the highlight background, making selected vs keyboard-focused indistinguishable.
Persistence	migrateLegacyAutoRefresh could change what a chart polls at, or a Dexie schema bump is needed	Provably lossless (cell.autoRefresh ?? true mirrors the old effective cadence in every branch — the only way the default is non-true is when no draw cell is undefined), idempotent, no deliberate-override drift (guarded on settings.autoRefreshDefault !== undefined). autoRefreshDefault is a nested optional field in an existing structured-clone blob — no version() bump needed.
Persistence	Downgrade / cross-version cadence drift	Not a defect: a strictly-older app ignores autoRefreshDefault and shows consolidated cells at its own default, but the field is retained in storage (never sanitized out), so the new app restores the exact cadence. No PR change required.
Structure	null handled 3 different ways across the auto-refresh tool entry points	Each is internally consistent (schema + validator + description agree): set_cell_autorefresh null=clear, set_notebook_autorefresh rejects null (a default has nothing to inherit), apply_notebook_state null=preserve. A nit at most — a shared validator would reduce future drift risk.
Cross-context	shared-definitions.json adds auto_refresh_default to apply_notebook_state's required[]	Schema-breaking in isolation, but coordinated with the 0.2.0 → 0.3.0 bridge bump and consistent with the other all-required/null-preserves fields; the in-app handler reads it as optional and tolerates absence. SAFE.
Async	The strand (#1) is Medium/blocking	Corrected to Minor — refresh-all is user-only, the trigger needs a sub-300 ms flip-to-Off with no subsequent visibility change, and it self-heals with no wrong/lost data.

---

Summary

Verdict: Approve — no blockers. A careful, unusually well-tested change; the false / undefined / inheritance / race edge cases that are the natural failure points here are handled correctly throughout.

• Regressions / tradeoffs: the only genuine new logic gap is wip test for error range #1 — the 5121dd9 jitter-window fix is incomplete for the specific "cadence flips to Off while the cell stays visible" sub-case, which silently no-ops an explicit refresh-all click. It's Minor (self-healing, no wrong data, low reachability) but is the one item worth fixing before merge; the fix is a clean few lines mirroring the existing non-poll branch. The inheritance model, the Dexie load-migration (lossless/idempotent, no schema bump), the manual-refresh state machine, and the AI/MCP tool surface are all sound.
• Findings verified vs dropped: 7 findings verified (all Minor), ~6 concerns dropped as false-positives / non-issues / by-design. Duplicate reports were merged — the strand was found independently by the async and query/data agents; the disabled-tooltip by the styling and a11y mandates; the optional-prop by four agents.
• In-diff vs out-of-diff: 7 in-diff, 0 out-of-diff. Zero out-of-diff is a genuine result, not a cross-context underrun: the undefined = inherit contract was threaded to every consumer, the one shared component (RadioItem) has a single consumer, and the cross-context agent's full per-callsite sweep (.autoRefresh readers, settings clone/spread, setCellRefresh, MCP paths, resolveAutoRefresh argument order, duplicateCellAt) plus independent greps returned every callsite SAFE with reachability traced.
• Clean domains: React correctness/hooks, State & context architecture, Performance & rendering, Persistence & migrations, Browser compatibility & security, and Cross-context caller impact all came back clean after verification.