diff --git a/.changeset/tile-editor-drawer.md b/.changeset/tile-editor-drawer.md new file mode 100644 index 0000000000..d7f9e2d646 --- /dev/null +++ b/.changeset/tile-editor-drawer.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/app': patch +--- + +feat: Move dashboard tile creation/editing from a modal into a right-side drawer. Display Settings (including the heatmap variant) now dock as a full-height side panel next to the editor/preview instead of stacking a second drawer, so the tile stays visible while its display options are changed and a single Esc press just closes the panel rather than closing everything at once. diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index 5fc4cec398..8b0abcd02d 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -63,11 +63,11 @@ import { Box, Breadcrumbs, Button, + Drawer, Flex, Group, Indicator, Menu, - Modal, Paper, Popover, Portal, @@ -1418,7 +1418,7 @@ const Tile = forwardRef( }, ); -const EditTileModal = ({ +const EditTileDrawer = ({ dashboardId, chart, onClose, @@ -1434,9 +1434,12 @@ const EditTileModal = ({ onSave: (chart: Tile) => void; }) => { const contextZIndex = useZIndex(); - const modalZIndex = contextZIndex + 10; + const drawerZIndex = contextZIndex + 10; const confirm = useConfirm(); const [hasUnsavedChanges, setHasUnsavedChanges] = useState(false); + // While a settings side panel is docked, hand Esc over to the panel so it + // closes only the panel instead of dismissing the whole tile editor. + const [settingsPanelOpen, setSettingsPanelOpen] = useState(false); // Reset dirty state only when a *different* tile is opened, not on every // chart-object reference change (onSubmit recreates the chart object with @@ -1469,18 +1472,23 @@ const EditTileModal = ({ }, [confirm, isSaving, hasUnsavedChanges, onClose]); return ( - {chart != null && ( - - {/* Isolate chart cross-syncing to this edit modal: the preview chart + + {/* Isolate chart cross-syncing to this edit drawer: the preview chart must not drive shadow tooltips on the dashboard tiles behind it. */} )} - + ); }; @@ -2918,7 +2927,7 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { {!isKioskMode && } {!isKioskMode && ( - { diff --git a/packages/app/src/components/ChartDisplaySettingsDrawer.tsx b/packages/app/src/components/ChartDisplaySettingsDrawer.tsx index ba38dac8d1..3ad5b1859b 100644 --- a/packages/app/src/components/ChartDisplaySettingsDrawer.tsx +++ b/packages/app/src/components/ChartDisplaySettingsDrawer.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useMemo } from 'react'; +import { useCallback, useEffect, useMemo, useRef } from 'react'; import { Controller, useForm, useWatch } from 'react-hook-form'; import { ChartConfigWithDateRange, @@ -17,6 +17,7 @@ import { Stack, Text, } from '@mantine/core'; +import { IconAlertTriangle } from '@tabler/icons-react'; import { shouldFillNullsWithZero } from '@/ChartUtils'; import { DEFAULT_SERIES_LIMIT } from '@/defaults'; @@ -32,6 +33,7 @@ import { import { ColorSwatchInput } from './ColorSwatchInput'; import { CheckBoxControlled } from './InputControlled'; import { DEFAULT_NUMBER_FORMAT, NumberFormatForm } from './NumberFormat'; +import SettingsSidePanel from './SettingsSidePanel'; export type ChartConfigDisplaySettings = Pick< ChartConfigWithDateRange, @@ -76,6 +78,14 @@ interface ChartDisplaySettingsDrawerProps { onChange: (settings: ChartConfigDisplaySettings, isDirty: boolean) => void; onClose: () => void; isPerSeriesNumberFormatAllowed?: boolean; + /** + * Render the settings as an in-place side panel (no Drawer overlay) instead + * of a nested drawer. Used inside the tile editor drawer so opening Display + * Settings docks a panel beside the editor/preview rather than stacking a + * second drawer on top (which made a single Esc press close both) — and keeps + * the tile visible while its display options are changed. + */ + asPanel?: boolean; } function applyDefaultSettings( @@ -115,6 +125,7 @@ export default function ChartDisplaySettingsDrawer({ onClose, previousDateRange, isPerSeriesNumberFormatAllowed = false, + asPanel = false, }: ChartDisplaySettingsDrawerProps) { const appliedDefaults = useMemo( () => applyDefaultSettings(settings, defaultNumberFormat), @@ -131,9 +142,20 @@ export default function ChartDisplaySettingsDrawer({ defaultValues: appliedDefaults, }); + // Reset only on the closed→open transition so every dismiss path — Apply, + // Cancel, Esc, or a tab change that closes the panel via the bare disclosure — + // behaves like cancel: abandoned edits never linger to be written by the next + // Apply. This is edge-triggered on `opened` (via a ref) rather than on + // `appliedDefaults` identity: `defaultNumberFormat` is a fresh object literal + // on every render, so a level-triggered reset would fire on each keystroke in + // the main form while the panel is open, silently discarding unapplied edits. + const wasOpenedRef = useRef(false); useEffect(() => { - reset(appliedDefaults); - }, [appliedDefaults, reset]); + if (opened && !wasOpenedRef.current) { + reset(appliedDefaults); + } + wasOpenedRef.current = opened; + }, [opened, appliedDefaults, reset]); const fillNulls = useWatch({ control, name: 'fillNulls' }); const isFillNullsEnabled = shouldFillNullsWithZero(fillNulls); @@ -213,83 +235,50 @@ export default function ChartDisplaySettingsDrawer({ const showBackgroundChart = displayType === DisplayType.Number; const isBackgroundChartDisabled = configType === 'sql'; - return ( - - - {isTimeChart && ( - <> - - - { - setValue('fillNulls', e.currentTarget.checked ? 0 : false); - }} - /> - - - ( - - {' - '} - ) - - ) - } - /> - + {isTimeChart && ( + <> + + + { + setValue('fillNulls', e.currentTarget.checked ? 0 : false); + }} /> - {showSeriesLimit && ( - - ( - - onChange(v === '' || v == null ? null : Number(v)) - } - /> - )} - /> - - )} - - - )} - - {showCategoricalLimit && ( - <> + + + ( + + {' - '} + ) + + ) + } + /> + + {showSeriesLimit && ( - - - )} + )} + + + )} - {showTableOptions && ( - <> - {showGroupByColumnsOnLeft && ( - - )} + {showCategoricalLimit && ( + <> + + ( + + onChange(v === '' || v == null ? null : Number(v)) + } + /> + )} + /> + + + + )} + + {showTableOptions && ( + <> + {showGroupByColumnsOnLeft && ( - - - )} + )} + + + + )} - {showTileColor && ( - <> - - - Color - - ( - - )} - /> - - - ( - - )} - /> - - - - )} - - {showBackgroundChart && ( - <> + {showTileColor && ( + <> + + + Color + ( - )} /> - - - )} + + + ( + + )} + /> + + + + )} + + {showBackgroundChart && ( + <> + ( + + )} + /> + + + )} - - - Format may be overridden on individual series. - - - ) : undefined - } - /> - - - - - - + } + > + + Format may be overridden on individual series. + + + ) : undefined + } + /> + + + + + + + ); + + // Panel mode: dock the settings as a full-height side panel beside the editor + // instead of stacking a second drawer, so the tile/preview stays visible + // while its display options are changed. + if (asPanel) { + if (!opened) return null; + return ( + + {content} + + ); + } + + return ( + + {content} ); } diff --git a/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx b/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx index 625a61025a..de67a82d65 100644 --- a/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx @@ -29,7 +29,11 @@ import { Text, Textarea, } from '@mantine/core'; -import { useDisclosure, usePrevious } from '@mantine/hooks'; +import { + useDisclosure, + useIsomorphicEffect, + usePrevious, +} from '@mantine/hooks'; import { notifications } from '@mantine/notifications'; import { IconBracketsContain, @@ -108,6 +112,11 @@ type EditTimeChartFormProps = { 'data-testid'?: string; submitRef?: React.MutableRefObject<(() => void) | undefined>; isDashboardForm?: boolean; + /** + * Notifies the containing drawer whether a settings side panel is docked, so + * it can disable its own Esc-to-close and let the panel own Esc instead. + */ + onSettingsPanelOpenChange?: (open: boolean) => void; autoRun?: boolean; }; @@ -146,6 +155,7 @@ export default function EditTimeChartForm({ 'data-testid': dataTestId, submitRef, isDashboardForm = false, + onSettingsPanelOpenChange, autoRun = false, }: EditTimeChartFormProps) { const formValue: ChartEditorFormState = useMemo( @@ -346,6 +356,112 @@ export default function EditTimeChartForm({ { open: openHeatmapSettings, close: closeHeatmapSettings }, ] = useDisclosure(false); + // Need to force a rerender on change as the modal will not be mounted when initially rendered + const [parentRef, setParentRef] = useState(null); + + // On a dashboard the editor lives inside a Drawer, so opening a settings + // view (regular Display Settings or the heatmap variant) docks a side panel + // next to the editor/preview instead of stacking a second drawer. On the + // Chart Explorer page the editor is not in a drawer, so they keep their own + // overlay drawer. + const showSettingsPanel = + isDashboardForm && (displaySettingsOpened || heatmapSettingsOpened); + + // Tell the containing drawer to hand Esc over to the docked panel: while a + // panel is open the drawer disables its own Esc-to-close (see EditTileDrawer), + // so Esc closes just the panel and can never dismiss the whole tile editor. + // + // This MUST be a layout effect, not a passive effect. Mantine's Drawer reads + // `closeOnEscape` live via a window capture-phase keydown listener. If we + // notified the parent from a passive effect it would flip `closeOnEscape` + // only after the browser paints the just-opened panel, leaving a window in + // which Esc (pressed right after opening) still reaches Mantine as `true` and + // dismisses the entire tile editor. A layout effect flushes the parent state + // update synchronously before paint, so `closeOnEscape` is already `false` + // before the panel is ever visible to press Esc against. + useIsomorphicEffect(() => { + onSettingsPanelOpenChange?.(showSettingsPanel); + // Reset the parent's flag if the form unmounts while a panel is open (e.g. + // the drawer closes with settings still docked). Otherwise the drawer keeps + // `settingsPanelOpen === true` and its Esc-to-close stays disabled for the + // next tile it hosts. + return () => onSettingsPanelOpenChange?.(false); + }, [showSettingsPanel, onSettingsPanelOpenChange]); + + // The panel owns Esc: while it's docked, Esc closes the panel. The drawer's + // own Esc is disabled above, so there's no double-close and no need to stop + // propagation. Bail out when Esc originated inside a nested overlay that + // consumes it: a popover dropdown, a select listbox, or an OPEN combobox (a + // Mantine Select keeps DOM focus on its input via aria-activedescendant, so + // match the input itself). The combobox arms are scoped to combobox-like + // roles — `[role="combobox"]` / `[aria-haspopup="listbox"]` with + // `aria-expanded="true"` — rather than a bare `[aria-expanded="true"]`: other + // expandable widgets (e.g. an `Accordion.Control` in the preview column) also + // carry `aria-expanded`, and matching those would swallow Esc and leave the + // panel stuck open with the UI reading as frozen. A closed combobox exposes + // `aria-expanded="false"` and correctly lets Esc fall through to close the + // panel. A focused CodeMirror editor is intentionally NOT exempt here — it + // calls preventDefault only when it actually consumes Esc (e.g. closing an + // open autocomplete), which the defaultPrevented guard above already covers; + // exempting it wholesale would swallow Esc and leave the panel open when no + // completion is showing. + // Attach as a layout effect (pre-paint) for the same reason as the notifier + // above: the panel must own Esc from the very first frame it is visible, so + // an Esc pressed immediately after opening closes the panel rather than + // doing nothing (or racing the drawer's own listener). + useIsomorphicEffect(() => { + if (!showSettingsPanel) return; + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key !== 'Escape' || e.defaultPrevented) return; + const target = e.target; + if ( + target instanceof HTMLElement && + target.closest( + '.mantine-Popover-dropdown, [role="listbox"], [role="combobox"][aria-expanded="true"], [aria-haspopup="listbox"][aria-expanded="true"]', + ) + ) { + return; + } + // Mark the event handled so any later listener (e.g. the containing + // drawer's, should its `closeOnEscape` not yet have flipped) bails on + // the `defaultPrevented` guard instead of tearing down the editor. + e.preventDefault(); + if (displaySettingsOpened) closeDisplaySettings(); + if (heatmapSettingsOpened) closeHeatmapSettings(); + }; + window.addEventListener('keydown', handleKeyDown); + return () => window.removeEventListener('keydown', handleKeyDown); + }, [ + showSettingsPanel, + displaySettingsOpened, + heatmapSettingsOpened, + closeDisplaySettings, + closeHeatmapSettings, + ]); + + // Whether the current tab has a regular Display Settings view. Search, + // Patterns and Markdown have none; Heatmap uses its own variant instead. + const displayTypeSupportsDisplaySettings = + displayType !== DisplayType.Search && + displayType !== DisplayType.EventPatterns && + displayType !== DisplayType.Markdown && + displayType !== DisplayType.Heatmap; + + // On tab change, close any settings panel that doesn't belong to the new tab + // so a stale panel (e.g. Number's Display Settings) is never shown on a tab + // without one, or the wrong variant (heatmap vs. regular) lingers. + useEffect(() => { + if (displayType === previousDisplayType) return; + if (!displayTypeSupportsDisplaySettings) closeDisplaySettings(); + if (displayType !== DisplayType.Heatmap) closeHeatmapSettings(); + }, [ + displayType, + previousDisplayType, + displayTypeSupportsDisplaySettings, + closeDisplaySettings, + closeHeatmapSettings, + ]); + // Only update this on submit, otherwise we'll have issues // with using the source value from the last submit // (ex. ignoring local custom source updates) @@ -616,9 +732,6 @@ export default function EditTimeChartForm({ const previousDateRange = getPreviousDateRange(dateRange); - // Need to force a rerender on change as the modal will not be mounted when initially rendered - const [parentRef, setParentRef] = useState(null); - const handleUpdateDisplaySettings = useCallback( ( { @@ -709,8 +822,22 @@ export default function EditTimeChartForm({ ); return ( -
- +
+ {/* Full-width display-type tab bar. Kept above the editor + Display + Settings row so the tabs never wrap when the panel is open. */} + @@ -781,166 +907,198 @@ export default function EditTimeChartForm({ )} /> - - - Chart Name - - - {isRawSqlDisplayType(displayType) && ( - ( - + {/* Editor + Display Settings side by side, below the tab bar, so the + tile/preview stays visible while its display options are changed. + In the dashboard drawer the row fills the remaining height: the + editor column scrolls on its own and the settings dock full-height + beside it. On Chart Explorer the settings return null and fall back + to the overlay Drawer. */} + + + + + + Chart Name + + + {isRawSqlDisplayType(displayType) && ( + ( + + )} /> )} - /> - )} - - - {activeTab === 'markdown' ? ( -
-