Skip to content

feat(dashboards): move tile editor to a drawer with docked settings panel - #2721

Open
elizabetdev wants to merge 16 commits into
mainfrom
elizabet/tile-editor-drawer
Open

feat(dashboards): move tile editor to a drawer with docked settings panel#2721
elizabetdev wants to merge 16 commits into
mainfrom
elizabet/tile-editor-drawer

Conversation

@elizabetdev

@elizabetdev elizabetdev commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Moves dashboard tile creation/editing from a centered modal into a right-side drawer, and reworks Display Settings so it no longer stacks a second drawer on top of the editor.

Closes HDX-4897

Why

Previously the tile editor was a Modal and Display Settings opened as a separate Drawer on top of it. With two stacked overlays, a single Esc press closed both at once, and the nested drawer-on-drawer felt wrong. This makes the editor a single drawer with settings docked inside it.

What changed

  • Modal → Drawer. EditTileModal is now EditTileDrawer (right-side, 90%, full-height flex body). Unsaved-changes confirm, z-index/ZIndexContext, and IsolatedChartSyncProvider behavior are preserved.
  • Display Settings as a docked side panel. Instead of a nested drawer, Display Settings render as a full-height panel beside the editor/preview, so the tile stays visible while you change its options (e.g. applying a color on a Number tile). A single Esc now closes just the panel, not the whole editor.
  • Heatmap settings too. The heatmap "Display Settings" (HeatmapSettingsDrawer) uses the same docked panel via a new shared SettingsSidePanel shell. Its use on the Search page (DBSearchHeatmapChart) is unchanged (still a Drawer, asPanel is opt-in).
  • Full-width tab bar. The display-type tabs sit in a full-width bar above the editor+panel row, so they never wrap when the panel is open. The panel's border-left meets the tab underline cleanly (semantic --color-border token).
  • No stale panels. Switching to a tab without settings (Search / Patterns / Markdown) or to the other variant (heatmap ↔ regular) closes the non-applicable panel.
  • Chart Explorer is unaffected — all the drawer/panel/full-height behavior is gated behind isDashboardForm; there the editor still uses the overlay Drawer.

Screenshots

Before

My-Dashboard-–-ClickStack-07-23-2026_07_28_PM

After

My-Dashboard-–-ClickStack-07-23-2026_07_24_PM

Testing

  • make ci-lint equivalent: yarn lint:fix + yarn tsc --noEmit clean.
  • Unit: ChartDisplaySettingsDrawer, DBSearchHeatmapChart, DBEditTimeChartForm suites pass.

Changeset

.changeset/tile-editor-drawer.md@hyperdx/app patch.

Made with Cursor

…anel

Convert the dashboard tile create/edit modal into a right-side drawer.
Display Settings (and the heatmap variant) now dock as a full-height side
panel beside the editor/preview instead of stacking a second drawer, so the
tile stays visible while its options change and a single Esc closes only the
panel. The display-type tabs sit in a full-width bar above the editor+panel
row so they never wrap, and switching to a tab without settings (Search /
Patterns / Markdown) or the other variant closes the non-applicable panel.

Co-authored-by: Cursor <cursoragent@cursor.com>
@changeset-bot

changeset-bot Bot commented Jul 23, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0413fc1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel

vercel Bot commented Jul 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jul 28, 2026 12:01pm
hyperdx-storybook Ready Ready Preview, Comment Jul 28, 2026 12:01pm

Request Review

@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Jul 23, 2026
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Large diff: 1096 production lines changed (threshold: 1000)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 6
  • Production lines changed: 1096 (+ 72 in test files, excluded from tier calculation)
  • Branch: elizabet/tile-editor-drawer
  • Author: elizabetdev

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Moves dashboard tile editing into a right-side drawer with docked display and heatmap settings panels.

  • Adds coordinated Escape handling between the settings panel and containing tile-editor drawer.
  • Introduces a shared settings side-panel shell and preserves overlay drawers outside dashboard forms.
  • Updates display-settings state reset behavior, layout, tests, E2E helpers, and the app changeset.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains within the scope of the previous Escape-handling threads.

Important Files Changed

Filename Overview
packages/app/src/DBDashboardPage.tsx Replaces the tile editor modal with a drawer and coordinates its Escape behavior with docked settings panels.
packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx Adds the docked settings layout, panel lifecycle handling, and nested-overlay-aware Escape behavior; no additional eligible blocking failure was identified.
packages/app/src/components/ChartDisplaySettingsDrawer.tsx Adds in-place panel rendering and resets abandoned edits when the settings form is reopened.
packages/app/src/components/HeatmapSettingsDrawer.tsx Adds opt-in docked-panel rendering while retaining the existing overlay-drawer behavior elsewhere.
packages/app/src/components/SettingsSidePanel.tsx Introduces the shared full-height shell for docked settings content.

Reviews (13): Last reviewed commit: "fix(dashboards): drive number format E2E..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 242 passed • 1 skipped • 783s

Status Count
✅ Passed 242
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

<!-- deep-review -->

Deep Review

🔴 P0/P1 -- must fix

  • packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx:374 -- The Escape bail-out selector includes [role="dialog"], which Element.closest() matches on the enclosing Mantine Drawer content that wraps the whole editor, so the capture handler returns before e.stopPropagation() on every keypress and the entire tile editor closes instead of just the settings panel.
    • Fix: Bail only when the matched [role="dialog"] is not the editor's own Drawer content (compare against a ref/parentRef.closest(...)), and drive the drawer's Escape explicitly via closeOnEscape={false} plus an owned handler, since stopPropagation() cannot defeat Mantine's own window-capture Escape listener documented at packages/app/tests/e2e/utils/locators.ts:13-19.
    • correctness, adversarial, julik-frontend-races

🟡 P2 -- recommended

  • packages/app/src/components/ChartDisplaySettingsDrawer.tsx:148 -- The reset effect re-runs reset(appliedDefaults) on every appliedDefaults identity change while opened is true, and getTraceDurationNumberFormat (packages/app/src/source.ts:517-522) returns a fresh object literal per call, so editing a series in the now-interactive editor column silently discards un-applied panel edits.

    • Fix: Gate the reset on the closed→open transition using a previous-opened ref, and memoize autoDetectedNumberFormat on its primitive inputs so it stops changing identity per keystroke.
    • correctness, kieran-typescript, adversarial
  • packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx:400 -- The tab-change cleanup only closes the panel when the new display type has no settings view, so a Line→Table switch leaves un-applied Line-only values (alignDateRangeToGranularity, seriesLimit) in the retained form, and applyChanges writes the whole form on the next Apply.

    • Fix: Close or force a reset of the settings form on every display-type change while the panel is open, not only when the new type lacks a settings view.
    • adversarial
  • packages/app/src/components/SettingsSidePanel.tsx:30 -- The panel is a bare Box with no role, no accessible name, and no focus return, dropping the role="dialog" + "Display Settings" name that the Drawer it replaces exposed and that ChartEditorComponent.ts:540 still relies on for the Chart Explorer variant.

    • Fix: Add role="region" with aria-label={title} (or aria-labelledby wired to the header), and restore focus to the triggering control when the panel unmounts.
    • agent-native, adversarial
  • packages/app/tests/e2e/components/ChartEditorComponent.ts:537 -- displaySettingsContainer matches only display-settings-panel or role=dialog[name="Display Settings"], but the docked heatmap variant renders data-testid="heatmap-settings-panel" with no dialog role, so the shared helpers cannot reach heatmap settings in dashboard mode and applyDisplaySettings's waitFor({ state: 'hidden' }) passes vacuously against zero matches.

    • Fix: Add .or(this.page.getByTestId('heatmap-settings-panel')) to the union, or expose a separate heatmapSettingsContainer getter and select the variant explicitly.
    • testing, adversarial
  • packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx:366 -- The capture-phase Escape handler that is the PR's central behavior has no unit or e2e coverage: DBEditTimeChartForm.test.tsx never passes isDashboardForm, and the two dashboard.spec.ts Escape tests press Escape only when no panel is open.

    • Fix: Add an e2e case that opens the tile editor and Display Settings, presses Escape once, and asserts the panel is hidden while chart-name-input stays visible.
    • testing, correctness, adversarial, julik-frontend-races
🔵 P3 nitpicks (4)
  • packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx:377 -- For genuinely nested widgets the guard returns without calling stopPropagation(), so an Escape dismissing a CodeMirror autocomplete inside the docked heatmap panel still reaches the Drawer and tears down the editor.

    • Fix: Call e.stopPropagation() for nested-widget targets too while skipping the panel-close calls, so the surrounding Drawer never sees the key.
  • packages/app/src/components/ChartDisplaySettingsDrawer.tsx:428 -- The asPanel shell fork, the reset-on-open effect, and handleClose are duplicated near-verbatim in HeatmapSettingsDrawer.tsx:147, including an identical comment block, so any fix must land twice.

    • Fix: Extract a shared SettingsShell wrapper plus a useResetOnOpen hook and have both components call them.
  • packages/app/src/components/SettingsSidePanel.tsx:32 -- The container uses a raw style object with an unnamed width: 340, against the guidance in agent_docs/code_style.md:34 to prefer individual Mantine style props.

    • Fix: Use Mantine props (w, h, display) and hoist the width into a named constant, keeping only the var(--color-border) borders as inline style.
  • packages/app/tests/e2e/components/ChartEditorComponent.ts:630 -- getPreviewTableHeaders and getPreviewTableCellTexts scope to the third-party .mantine-Drawer-body class, which does not exist in Chart Explorer mode and can also match a concurrently open Row Click or Series Display Settings drawer.

    • Fix: Add a first-party data-testid to the editor's scroll container and scope both helpers to it.

Reviewers (9): correctness, adversarial, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, agent-native, learnings-researcher.

Testing gaps:

  • No coverage for Escape originating inside the docked panel's CodeMirror editors or a Mantine Select/Popover while the panel is open — the exact targets the guard whitelists.
  • No coverage for editing the chart builder while the settings panel is docked open, the interaction the docked layout newly enables.
  • No test switches display types with the panel open to assert only currently visible options are persisted on Apply.
  • The asPanel branch of HeatmapSettingsDrawer is unexercised: DBSearchHeatmapChart.test.tsx:51 mocks the component wholesale, and no dashboard e2e covers Heatmap tiles.
  • No test asserts where focus lands after the panel unmounts, or that the editor still responds to Escape afterwards.

Note: bash/git were unavailable in this environment, so reviewers worked from the working-tree files rather than a computed diff; new-vs-pre-existing attribution was inferred from code structure and may be imprecise for the three large pre-existing files.

The dashboard tile editor was converted from a Modal to a Drawer, with
Display Settings now rendered as a docked side panel instead of a nested
Drawer. Update the shared ChartEditorComponent page object to match:

- Preview table helpers now target .mantine-Drawer-body instead of
  .mantine-Modal-body
- Display Settings helpers match either the dashboard side panel
  (data-testid) or the Chart Explorer drawer (role=dialog) so both
  contexts work

Co-authored-by: Cursor <cursoragent@cursor.com>
setGroupByColumnsOnLeft still targeted the Display Settings dialog
directly, which times out in the dashboard where Display Settings is now
a docked side panel. Route it through the shared displaySettingsContainer
locator so it matches either the panel or the drawer.

Co-authored-by: Cursor <cursoragent@cursor.com>
…p swallowing nested Esc

The tile editor's Esc interceptor and tab-change cleanup close the
settings panels via the bare useDisclosure close(), bypassing each
child's handleClose reset. Abandoned sub-form edits therefore stayed
latched and were written into the tile config by the next Apply. Reset
the sub-form on the closed→open transition in both ChartDisplaySettingsDrawer
and HeatmapSettingsDrawer so every dismiss path behaves like cancel.

Also stop the capture-phase Esc handler from calling stopPropagation on
every Escape: bail out when the key originated inside a nested overlay
(popover dropdown, code editor, select listbox, dialog) or was already
handled, so those widgets can consume their own Esc instead of having it
swallowed before they ever see it.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions github-actions Bot added review/tier-4 Critical — deep review + domain expert sign-off and removed review/tier-3 Standard — full human review required labels Jul 27, 2026
Comment thread packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx Outdated
The tile editor's own Drawer has role="dialog", so target.closest() matched
it and the handler bailed before stopping propagation, closing the whole
editor on Esc. Ignore a match that wraps the form (the containing drawer) and
only bail for overlays genuinely nested inside the panel.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx Outdated
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Deep Review

⚠️ Review did not run — environment blocker. The multi-agent pipeline was invoked but halted at scope detection (Stage 1). No reviewer sub-agents were dispatched, so this comment reports zero findings, and that is not a clean bill of health.

Why it halted

The review requires the PR diff as its primary input. No tool available in this session could retrieve it:

Capability Result
Bash (sandboxed) Fails on every command, including echo test and pwd: bwrap: Can't create file at /home/.mcp.json: Permission denied
Bash (sandbox disabled) Identical bwrap failure; other variants returned This command requires approval, ungrantable non-interactively
Bash via sub-agent Identical bwrap failure — not scoped to the orchestrator process
Glob Not available in this session; its own error directs to find via Bash, which is broken
Grep Not available in this session
WebFetch Claude requested permissions to use WebFetch, but you haven't granted it yet — blocked the fallback to the public .diff URL
Read Works, but exact paths only; directories return EISDIR, so the changed-file set cannot be enumerated

Because git is reachable only through Bash, neither git diff nor gh pr diff could run, and the base:0e28094941b7b685e439f7fd159444b505660b57 fast path could not be exercised.

Why no degraded review was substituted

Read did confirm several feature files exist — packages/app/src/components/SettingsSidePanel.tsx, packages/app/src/components/ChartDisplaySettingsDrawer.tsx, packages/app/src/components/HeatmapSettingsDrawer.tsx, a DBEditTimeChartForm entry under packages/app/src/components/, and .changeset/tile-editor-drawer.md. A whole-file review of these was deliberately not performed:

  • Unknown coverage. Those paths were found by guessing, not enumeration. The actual changed-file set is unknown, so any review would silently cover an arbitrary subset while appearing complete.
  • No new-vs-existing attribution. Without a diff, new code cannot be distinguished from pre-existing code. Both the severity rubric (breakage introduced by this diff) and the pipeline's pre_existing flag depend on that distinction.
  • False-positive risk. Flagging long-standing code as newly introduced is the exact failure mode the pipeline's Quality Gates prohibit.

Findings were also not derived from the PR title or description, which are advisory context for intent only and not a substitute for reading the code.

Unblocking

Any one of these is sufficient to get a full review:

  1. Fix HOME — the /home/.mcp.json path implies HOME=/home instead of /home/runner, which is what breaks bwrap sandbox setup. This is the root cause and restores all shell-dependent tooling.
  2. Grant WebFetch — allows fetching the diff for this public repository over HTTP without a shell.
  3. Pre-materialize the diff to a file at a known absolute path and pass that path, since Read is functional.

Reviewers (0): none dispatched — halted at scope detection before Stage 4 fan-out.

Testing gaps: not assessed; requires diff access.

…lied edits

Disable the tile Drawer's Esc-to-close while a settings panel is docked and
let the panel own Esc, so Esc closes only the panel — even from inside the
code editor — instead of dismissing the whole tile editor. Also edge-trigger
the display-settings reset on the closed->open transition (via a ref) rather
than on appliedDefaults identity, so editing a series in the main form no
longer wipes the panel's unapplied edits and dirty state.

Co-authored-by: Cursor <cursoragent@cursor.com>
elizabetdev and others added 2 commits July 28, 2026 11:53
Ref identifiers must end in "Ref"; the unsuffixed name added one warning that
tripped the app package's max-warnings cap and failed CI lint.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx Outdated
elizabetdev and others added 2 commits July 28, 2026 11:57
…anel

A Mantine Select keeps DOM focus on its input via aria-activedescendant, so
neither [role="listbox"] nor .mantine-Popover-dropdown matches the Esc target
and the whole panel closed instead of the open dropdown. Also bail when the
target is inside an expanded combobox ([aria-expanded="true"]).

Co-authored-by: Cursor <cursoragent@cursor.com>
…column

Cap the panel at min(340px, 45%) so it yields on narrow viewports inside the
90% tile drawer, and give the editor column a 320px minWidth floor when a panel
can dock beside it (kept at 0 on Chart Explorer).

Co-authored-by: Cursor <cursoragent@cursor.com>
Dropped the blanket .cm-editor exemption from the settings-panel Esc handler.
CodeMirror only calls preventDefault when it actually consumes Esc (e.g.
closing an open autocomplete), which the defaultPrevented guard already
handles; exempting it wholesale left Esc doing nothing — and the panel open —
whenever a SQL editor was focused without a completion showing.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx Outdated
…close the panel

Native <select> exposes no open/closed state to JS, so the docked settings
panel's window-level Escape handler could not tell whether Esc dismissed the
select popup or should close the panel. Chrome/Safari fire no keydown while a
native select popup is open (so the panel stayed open) but Firefox/IE do (so
the panel closed, discarding unapplied settings); exempting native <select>
wholesale then broke closing the panel when a closed select had focus.

Convert the number-format controls to Mantine Select, whose open state is
visible via aria-expanded/role="listbox" (already matched by the handler) and
which does not swallow Esc when closed, and drop the native-select exemption.

Co-authored-by: Cursor <cursoragent@cursor.com>
- Remove now-unused no-unsafe-type-assertion eslint-disable in NumberFormat
  (Mantine Select's onChange value no longer trips the rule), which had pushed
  app warnings to 741 over the --max-warnings 740 lint budget.
- Update ChartDisplaySettingsDrawer test to drive the output format via the
  Mantine combobox (click + option) instead of native selectOptions, and mock
  HTMLElement.prototype.scrollIntoView as the other Combobox tests do.

Co-authored-by: Cursor <cursoragent@cursor.com>
The chart-wide/per-series number format E2E helper still used the native
selectOption API against getByLabel('Output format'), which now resolves to
both the Mantine Select input and its listbox (strict-mode violation). Open the
combobox and click the option, matching the pattern used by the other Select
helpers in this component.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant