Modal system: break import cycles — component map moves to the shell

2026-08-08 02:02:33 +00:00 · 2026-06-12 19:45:50 +03:00
parent 91b6e102eb
commit 538882b342
5 changed files with 49 additions and 43 deletions
@@ -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
          </button>
@@ -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 (`<Body />` from the registry), so there is no
- * `name === 'datasets' && <DatasetsModal/>` 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' && <DatasetsModal/>` 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<ModalName, ComponentType> = {
+  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
@@ -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<Record<ModalName, ModalConfig>> = {
+const MODAL_REGISTRY: Partial<Record<ModalName, ModalConfig>> = {
  // 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<Record<ModalName, ModalConfig>> = {
  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<Record<ModalName, ModalConfig>> = {
  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<Record<ModalName, ModalConfig>> = {
  themeBuilder: {
    name: 'themeBuilder',
    title: 'Theme Builder',
-    component: ThemeBuilderModal,
    dismissOnBackdrop: false,
    init: () => {
      const store = useCustomThemeStore.getState();
@@ -124,12 +115,10 @@ export const MODAL_REGISTRY: Partial<Record<ModalName, ModalConfig>> = {
  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;
@@ -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<ChartBuilderState>((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;
  },