From 538882b3420c21deb537d591acc11fe20b34ac22 Mon Sep 17 00:00:00 2001 From: Oleh Omelchenko Date: Fri, 12 Jun 2026 19:45:50 +0300 Subject: [PATCH] =?UTF-8?q?Modal=20system:=20break=20import=20cycles=20?= =?UTF-8?q?=E2=80=94=20component=20map=20moves=20to=20the=20shell?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/architecture/03-modal-system.md | 10 +++++--- src/app/components/ChartBuilderModal.tsx | 14 ++++------- src/app/components/ModalShell.tsx | 32 +++++++++++++++++++----- src/app/modals/modal-registry.ts | 30 ++++++---------------- src/app/stores/ChartBuilderStore.ts | 6 ++--- 5 files changed, 49 insertions(+), 43 deletions(-) diff --git a/docs/architecture/03-modal-system.md b/docs/architecture/03-modal-system.md index 6a4e81b..e70e318 100644 --- a/docs/architecture/03-modal-system.md +++ b/docs/architecture/03-modal-system.md @@ -101,9 +101,13 @@ export interface ModalConfig { > `hasError`/`getError`**: each modal renders its **own action row** inside its body (the > multi-view Datasets manager doesn't fit a single shell-level Save/Cancel), so validity is > each modal's own concern. The shipped `ModalConfig` keeps only `getState` (close-time -> unsaved-change detection) plus `init`/`isUrlNavigable`. The generic-footer sketch through -> the rest of this section is retained as the simpler pattern for a single-action modal — -> treat it as illustrative, not a description of current code. +> unsaved-change detection) plus `init`/`isUrlNavigable`. It also **omits `component`**: +> the name → component map lives in the shell (`components/ModalShell` → +> `MODAL_COMPONENTS`), its only consumer — a registry that imported components would close +> an import cycle (coordinator → registry → component → coordinator, since modal bodies +> call `closeModal`). The generic-footer sketch through the rest of this section is +> retained as the simpler pattern for a single-action modal — treat it as illustrative, +> not a description of current code. ### Example entries diff --git a/src/app/components/ChartBuilderModal.tsx b/src/app/components/ChartBuilderModal.tsx index 3c131c2..6783fa5 100644 --- a/src/app/components/ChartBuilderModal.tsx +++ b/src/app/components/ChartBuilderModal.tsx @@ -56,7 +56,7 @@ import { type TimeUnit, } from '@core/chart-builder'; import { referencedFields, validateExpression } from '@core/expr-validate'; -import { tabularRows } from '@core/dataset'; +import { cellText, tabularRows } from '@core/dataset'; import type { ColumnType } from '@core/type-inference'; import { DatasetNotFoundError, prepareSpecForRender } from '@core/rendering'; import { chartConfigFor } from '@core/vega-themes'; @@ -179,13 +179,6 @@ const PREVIEW_ROW_LIMIT = 50; */ const VEGA_EXPRESSION_DOCS_URL = 'https://vega.github.io/vega/docs/expressions/'; -/** One preview cell's text: blank for empty, the string as-is, else JSON. */ -function cellText(value: unknown): string { - if (value == null) return ''; - if (typeof value === 'string') return value; - return JSON.stringify(value); -} - /** A safe `datum` accessor for a column name (dot for identifiers, bracket otherwise). */ function datumRef(name: string): string { return /^[A-Za-z_$][\w$]*$/.test(name) ? `datum.${name}` : `datum[${JSON.stringify(name)}]`; @@ -1425,7 +1418,10 @@ export function ChartBuilderModal() { className={`${styles.action} ${styles.primary}`} disabled={!valid} aria-describedby={!valid ? 'cb-create-hint' : undefined} - onClick={() => runCreate()} + // The create is the user's confirmation — close with no discard prompt. + onClick={() => { + if (runCreate()) void closeModal(true); + }} > Create Snippet diff --git a/src/app/components/ModalShell.tsx b/src/app/components/ModalShell.tsx index 089f8db..c0fe642 100644 --- a/src/app/components/ModalShell.tsx +++ b/src/app/components/ModalShell.tsx @@ -2,23 +2,43 @@ * Modal shell (docs/architecture/03 → Layer 3). * * Renders exactly ONE modal — whichever `activeModal` names — inside a single - * reusable chrome: backdrop, header (title + close), and a focus trap. The - * modal's registered `component` fills the body. This is the only place a modal - * name maps to a view (`` from the registry), so there is no - * `name === 'datasets' && ` chain anywhere. + * reusable chrome: backdrop, header (title + close), and a focus trap. The body + * comes from `MODAL_COMPONENTS` below — the only place a modal name maps to a + * view, so there is no `name === 'datasets' && ` chain anywhere. + * The map lives here rather than in the registry so the registry (which the + * coordinator and URL sync import) never imports components — that edge would + * close an import cycle: coordinator → registry → component → coordinator. * * Dismissal is uniform for these passive feature modals: the close button, * Escape, or a backdrop click — never a click inside the body (which stops * propagation). Each modal owns its own action buttons; the shell stays generic. */ +import type { ComponentType } from 'react'; import { useAppStore } from '../stores/AppStore'; import { getModalConfig, getModalTitle } from '../modals/modal-registry'; +import type { ModalName } from '../modals/types'; import { closeModal } from '../modals/ModalCoordinator'; import { useFocusTrap } from '../hooks/useFocusTrap'; +import { AboutModal } from './AboutModal'; +import { ChartBuilderModal } from './ChartBuilderModal'; +import { DatasetsModal } from './DatasetsModal'; +import { DonateModal } from './DonateModal'; +import { ExtractModal } from './ExtractModal'; +import { ThemeBuilderModal } from './ThemeBuilderModal'; import { Icon } from './Icon'; import styles from './ModalShell.module.css'; +/** The body rendered for each modal name (metadata stays in the registry). */ +const MODAL_COMPONENTS: Record = { + datasets: DatasetsModal, + extract: ExtractModal, + chartBuilder: ChartBuilderModal, + themeBuilder: ThemeBuilderModal, + about: AboutModal, + donate: DonateModal, +}; + export function ModalShell() { const name = useAppStore((s) => s.activeModal); const config = getModalConfig(name); @@ -40,8 +60,8 @@ export function ModalShell() { isLarge ? '#modal-title' : undefined, ); - if (!config) return null; - const Body = config.component; + if (!config || !name) return null; + const Body = MODAL_COMPONENTS[name]; // Modals with in-progress work (the Chart Builder's config) opt out of // click-outside-to-close so an accidental backdrop click can't discard it; Escape diff --git a/src/app/modals/modal-registry.ts b/src/app/modals/modal-registry.ts index 8ddbfe4..e2bc510 100644 --- a/src/app/modals/modal-registry.ts +++ b/src/app/modals/modal-registry.ts @@ -1,9 +1,13 @@ /** * Modal registry (docs/architecture/03 → Layer 1). * - * One metadata entry per feature modal — the single source of truth the - * coordinator and shell read, so adding a modal is one entry plus its component - * rather than edits scattered across the shell, URL sync, and close logic. + * One **metadata** entry per feature modal — the single source of truth the + * coordinator, URL sync, and shell read, so adding a modal is one entry here, + * one row in the shell's `MODAL_COMPONENTS` map, and its component — rather than + * edits scattered across the shell, URL sync, and close logic. The component + * map lives in `components/ModalShell` (its only consumer), NOT here: a registry + * that imported components would close an import cycle (coordinator → registry → + * component → coordinator). * * Divergence from the arch sketch's optional generic footer: each modal renders * its OWN action row inside its body (the Datasets manager is multi-view, so a @@ -15,15 +19,8 @@ * see components/SettingsPopover). */ -import type { ComponentType } from 'react'; import type { ActiveModal, ModalName } from './types'; import { customThemeIdOf } from '@core/vega-themes'; -import { AboutModal } from '../components/AboutModal'; -import { ChartBuilderModal } from '../components/ChartBuilderModal'; -import { DatasetsModal } from '../components/DatasetsModal'; -import { DonateModal } from '../components/DonateModal'; -import { ExtractModal } from '../components/ExtractModal'; -import { ThemeBuilderModal } from '../components/ThemeBuilderModal'; import { useAppStore } from '../stores/AppStore'; import { useChartBuilderStore } from '../stores/ChartBuilderStore'; import { selectIsDraftDirty, useCustomThemeStore } from '../stores/CustomThemeStore'; @@ -34,8 +31,6 @@ export interface ModalConfig { name: ModalName; /** Header title (literal for now; an i18n key once strings are centralized). */ title: string; - /** The body rendered inside the shell. */ - component: ComponentType; /** Initialize transient state on open. `arg` carries an optional sub-target. */ init?: (arg?: string) => void; /** @@ -54,13 +49,12 @@ export interface ModalConfig { dismissOnBackdrop?: boolean; } -export const MODAL_REGISTRY: Partial> = { +const MODAL_REGISTRY: Partial> = { // Navigable, multi-view manager. The snapshot captures only the open create/edit // form, so browsing list↔detail never trips a false discard prompt. datasets: { name: 'datasets', title: 'Datasets', - component: DatasetsModal, isUrlNavigable: true, init: (datasetId) => useDatasetStore.getState().select(datasetId ? Number(datasetId) : null), getState: () => { @@ -73,7 +67,6 @@ export const MODAL_REGISTRY: Partial> = { extract: { name: 'extract', title: 'Extract to Dataset', - component: ExtractModal, init: () => useExtractStore.getState().init(), getState: () => ({ name: useExtractStore.getState().name }), }, @@ -86,7 +79,6 @@ export const MODAL_REGISTRY: Partial> = { chartBuilder: { name: 'chartBuilder', title: 'Chart Builder', - component: ChartBuilderModal, isUrlNavigable: true, dismissOnBackdrop: false, init: (datasetId) => useChartBuilderStore.getState().init(datasetId ? Number(datasetId) : null), @@ -100,7 +92,6 @@ export const MODAL_REGISTRY: Partial> = { themeBuilder: { name: 'themeBuilder', title: 'Theme Builder', - component: ThemeBuilderModal, dismissOnBackdrop: false, init: () => { const store = useCustomThemeStore.getState(); @@ -124,12 +115,10 @@ export const MODAL_REGISTRY: Partial> = { about: { name: 'about', title: 'About & Help', - component: AboutModal, }, donate: { name: 'donate', title: 'Donate', - component: DonateModal, }, }; @@ -137,6 +126,3 @@ export const getModalConfig = (name: ActiveModal): ModalConfig | undefined => name ? MODAL_REGISTRY[name] : undefined; export const getModalTitle = (name: ActiveModal): string => getModalConfig(name)?.title ?? ''; - -export const isUrlNavigable = (name: ActiveModal): boolean => - getModalConfig(name)?.isUrlNavigable ?? false; diff --git a/src/app/stores/ChartBuilderStore.ts b/src/app/stores/ChartBuilderStore.ts index 1c80dbe..eb7e760 100644 --- a/src/app/stores/ChartBuilderStore.ts +++ b/src/app/stores/ChartBuilderStore.ts @@ -48,7 +48,6 @@ import { type TimeUnit, } from '@core/chart-builder'; import type { ColumnType } from '@core/type-inference'; -import { closeModal } from '../modals/ModalCoordinator'; import { useDatasetStore } from './DatasetStore'; import { useSnippetStore } from './SnippetStore'; @@ -487,8 +486,9 @@ export const useChartBuilderStore = create((set, get) => ({ // No success toast: the new snippet immediately becomes active and opens in the // editor, so the result is visible — toasting it would be noise (contract 10 §1, - // "toast only what the user can't already see"). - void closeModal(true); // the create is the user's confirmation — no discard prompt + // "toast only what the user can't already see"). Closing the modal is the + // component's choreography (it owns the modal lifecycle; the store stays + // coordinator-free) — it closes on this returning true. get().reset(); return true; },