From 71b874978b605a3e2fd2b1a118156804e98ad6d2 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:55:58 -0400 Subject: [PATCH 1/6] feat(frontend): draw every dashboard with the kit's metric tier, and refuse re-implementing it (#1041) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@codefly-dev/ui/dashboard` shipped two tiers. The host's own operations pages used the metric one; a solution's dashboard got the older basic one — twice over, because `src/solutions/SolutionDashboard.tsx` carried its own copy of the widget-to-chart dispatch, the responsive grid, the card shell and the empty state beside the kit's ``, and both copies drew with `charts.tsx`. So the same declaration rendered plainer inside the host than beside it, and there were two renderers of one thing to keep in step. **A fix at the owner.** `` now paints with the metric tier, contract unchanged (a resolved `DashboardView` in; no fetching, no host context, no app imports). A scalar widget is a `StatTile` — the series total in the widget's format and unit, a sparkline and a delta against the previous bucket when the metric is bucketed over time, and a `MetricStateBadge` from `series.coverage`. A series widget is the kit's metric line/area/bar chart over a `ChartSeries`, which brings the axes, the crosshair readout, the legend and the visually-hidden data table that is the chart's screen-reader representation. A withheld total renders as a dash and never as `0`: `Metric.value` now admits `null` so that is a property of the primitive rather than of a state mapping, and a new `no_total` state says which of partial / empty / no-total withheld it. A series with no points reads as no data, not as a zero, because the aggregate omits a bucket instead of emitting an empty one. A sparkline and a bucket delta are drawn only for a time-bucketed series — a line through counts grouped by event type would assert an order the data does not have. `SolutionDashboard.tsx` now mounts that renderer and draws nothing itself. It keeps exactly what is the host's, composed through four new presentational slots: the + menu as header `actions`, the drag-to-reorder `SortableGrid` as `layout`, the per-widget audit query as `renderWidget`, and its own `empty` line. The queries stay one per widget — a widget whose metric errors still fails alone — which is why the kit also exports `DashboardWidget`, the same renderer for one widget: React fixes the number of hooks a component may call, so a varying set of per-widget queries cannot live in a renderer that takes the whole view. Its 16 existing tests still pass; two assertions and one helper moved onto the markup the metric tier produces, each with the reason in place. **A gate**, in the shape the kit's other consumer already uses: `src/components/__tests__/kit.test.ts`. A declaration taking a name `@codefly-dev/ui` exports must render the kit's component of that name or carry a reviewed entry saying what the two things are; `class-variance-authority` is no longer a dependency of this app (nothing imported it), so a kit-shaped primitive cannot be built here at all; and the two chart surfaces stay re-exports. Module-local declarations count, not only exported ones — that is how both duplicates in this tree arrived, and a gate matching only exports walked past both. Nine fixtures cover must-catch, must-not-catch, and the same duplication reached another way. Docs: `module/AUDIT_METRICS.md` gains "How a resolved series is drawn" — the tier, and a table of what each widget and each coverage state renders, which is what downstream consumers were guessing at; `module/services/frontend/AGENTS.md` says what the solution dashboard owns and what the kit draws; the kit's README names the tier and the slots. Refs #1041 Co-Authored-By: Claude Opus 5 (1M context) --- module/AUDIT_METRICS.md | 33 +- module/services/frontend/AGENTS.md | 34 ++ .../services/frontend/code/package-lock.json | 1 - module/services/frontend/code/package.json | 1 - .../code/packages/codefly-ui/README.md | 11 +- .../dashboard/__tests__/dashboard.test.tsx | 474 ++++++++++++++++-- .../src/dashboard/__tests__/types.test.ts | 10 +- .../codefly-ui/src/dashboard/dashboard.tsx | 382 ++++++++++++-- .../codefly-ui/src/dashboard/index.ts | 8 +- .../codefly-ui/src/dashboard/metric-state.tsx | 6 + .../codefly-ui/src/dashboard/metric-tiles.tsx | 61 ++- .../codefly-ui/src/dashboard/types.ts | 28 +- .../code/src/components/__tests__/kit.test.ts | 406 +++++++++++++++ .../webhooks/ui/webhook-deliveries-panel.tsx | 9 +- .../code/src/solutions/SolutionDashboard.tsx | 354 ++++++------- .../__tests__/SolutionDashboard.test.tsx | 28 +- 16 files changed, 1528 insertions(+), 318 deletions(-) create mode 100644 module/services/frontend/code/src/components/__tests__/kit.test.ts diff --git a/module/AUDIT_METRICS.md b/module/AUDIT_METRICS.md index 473dec252..44c0a2ec4 100644 --- a/module/AUDIT_METRICS.md +++ b/module/AUDIT_METRICS.md @@ -194,7 +194,8 @@ never the SDK binding input. Regenerate the export before generating the SDK. ## Unknown and partial results -Empty queries have no points and render **No data yet**. Numeric sums with no +Empty queries have no points and render as no data, in the form the widget's +visualization has for it (see *How a resolved series is drawn*). Numeric sums with no numeric observations omit their value, just like averages/percentiles; an emitted zero stays zero. `samples[alias]` reports observations before reduction (so repeated logical IDs are not mistaken for missing telemetry). Fewer samples than @@ -212,6 +213,36 @@ The host DSL also withholds totals after top-N truncation. Authorization/RPC errors remain errors, never empty successful results. Existing consumers of the SDK must handle `total: null` when adopting this contract. +## How a resolved series is drawn + +Every dashboard built from these series — the host's own pages, a solution's +declared dashboard, and a solution remote that mounts the kit itself — is drawn +by one renderer, `` from `@codefly-dev/ui/dashboard`, with the kit's +**metric tier**. There is no second, plainer rendering: a consuming solution +does not have to ask which half of the kit its dashboard will get. + +| Widget | Drawn as | Empty series | `coverage: "partial"` | `total: null` | +| --- | --- | --- | --- | --- | +| `number` | `StatTile`: the series total, in the widget's `format` and `unit`; a sparkline and a delta against the previous bucket when the metric is bucketed over **time** | `—` with a **No data** badge, never `0` | `—` with a **Partial data** badge | `—` with a **No total** badge when coverage is complete | +| `line` / `area` / `bar` | the kit's metric chart over a `ChartSeries` — axes from the appearance palette, a crosshair readout, a legend for two or more series, and every exact value in a visually-hidden data table | the chart's own **No data** plot, plus a **No data** badge on the card | the observed points still plot, with a **Partial data** badge on the card | nothing: a series widget shows no total | +| `table` | key/value rows | **No data yet.** | a **Partial data** badge on the card | — | + +Two of those cells are the contract, not presentation. A `null` total renders as +a dash and never as `0`, because "cannot say" and "nothing happened" are +different answers and only one of them is in the data; and a series with no +points is no data rather than a zero, because the aggregate omits a bucket +instead of emitting an empty one. A sparkline and a bucket delta are drawn +**only** for a time-bucketed series: a line through counts grouped by event type +or region would assert an order, and a delta between two of them a change over +time, that a categorical grouping does not have. + +A host composes its own features around that renderer — ``'s slots +take header actions, a layout (the host's solution dashboard passes a +drag-to-reorder grid), a per-widget renderer for a host that resolves each +widget in its own query, and an empty state. A consumer re-implementing any part +of the drawing is refused by a gate: `src/components/__tests__/kit.test.ts` in +the frontend. + ## Independent validation Run `go test -race ./pkg/infra/auditmetricstest` from accounts/code for pure diff --git a/module/services/frontend/AGENTS.md b/module/services/frontend/AGENTS.md index 00759fb49..bf0ba54e6 100644 --- a/module/services/frontend/AGENTS.md +++ b/module/services/frontend/AGENTS.md @@ -115,6 +115,40 @@ whether `/s/{id}` renders, and it stays installation-blind. observation speaks for the present is the host's to decide, and a copy of that window here would drift from it. +## Drawing a solution's declared dashboard + +`src/solutions/SolutionDashboard.tsx` **mounts** the shared kit's +`` (`@codefly-dev/ui/dashboard`); it draws nothing itself. Which +tier each widget and each coverage state is drawn with is the module's +[AUDIT_METRICS.md](../../AUDIT_METRICS.md) ("How a resolved series is drawn") — +one renderer for the host's pages, a declared solution dashboard and a solution +remote alike. + +What this file owns is only what is the host's, composed through that +renderer's slots: + +- **the audit queries**, one per widget, so a widget whose metric errors fails + alone instead of blanking its siblings. Each lives in its own component + (`SolutionWidget`) that resolves the series and then mounts the kit's + `DashboardWidget`, because React fixes the number of hooks a component may + call and a renderer taking the whole view cannot hold a varying number of + queries; +- **the viewer's arrangement** — reorder by drag or arrow keys, remove, add + back, saved per viewer and organization (`dashboard-layout.ts`), handed to + the renderer's `layout` slot as the kit's `SortableGrid`; +- **the ⓘ** that says what a tile counts, from which audit events, over what + range and who caused the newest ones (`metric-info.ts`, the principal + directory, the recent-events drill-down), handed to each widget as its + header actions. + +Re-implementing any part of the drawing is refused by +`src/components/__tests__/kit.test.ts`: a declaration taking a name +`@codefly-dev/ui` exports must render the kit's component of that name or carry +a reviewed entry, `class-variance-authority` is not a dependency of this app so +a kit-shaped primitive cannot be built here, and the two chart surfaces +(`src/components/charts/chart.tsx`, +`src/features/dashboard/ui/charts/line-chart.tsx`) stay re-exports. + ## Loading a remote, and its CSP `/s/[solutionId]` (`src/app/(dashboard)/s/[solutionId]/page.tsx`) loads the diff --git a/module/services/frontend/code/package-lock.json b/module/services/frontend/code/package-lock.json index b10edc9c7..46b1d9462 100644 --- a/module/services/frontend/code/package-lock.json +++ b/module/services/frontend/code/package-lock.json @@ -34,7 +34,6 @@ "@simplewebauthn/browser": "^13.3.0", "@tanstack/react-query": "^5.60.0", "@tanstack/react-table": "^8.21.3", - "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "codefly": "https://github.com/codefly-dev/sdk-js/archive/9f3bb78682a0c2b9a70631d35c2ae0094c281ac5.tar.gz", "js-yaml": "^4.3.2", diff --git a/module/services/frontend/code/package.json b/module/services/frontend/code/package.json index 7f3f13676..47289638d 100644 --- a/module/services/frontend/code/package.json +++ b/module/services/frontend/code/package.json @@ -57,7 +57,6 @@ "@simplewebauthn/browser": "^13.3.0", "@tanstack/react-query": "^5.60.0", "@tanstack/react-table": "^8.21.3", - "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "codefly": "https://github.com/codefly-dev/sdk-js/archive/9f3bb78682a0c2b9a70631d35c2ae0094c281ac5.tar.gz", "js-yaml": "^4.3.2", diff --git a/module/services/frontend/code/packages/codefly-ui/README.md b/module/services/frontend/code/packages/codefly-ui/README.md index 070eee474..95543fdca 100644 --- a/module/services/frontend/code/packages/codefly-ui/README.md +++ b/module/services/frontend/code/packages/codefly-ui/README.md @@ -36,6 +36,15 @@ and the v1 subset — is [CATALOG.md](./CATALOG.md). (`Banner`, `EmptyState`, `ErrorState`) and overlays (`Dialog`, `AlertDialog`, `Notice`, `Tooltip`, `DropdownMenu`); `dashboard` is ``, charts, `fromDashboardData`; `chat` is ``. React only: no plugin runtime, no host context. + `` paints with the **metric tier** — `StatTile`/`KPIRow` for scalar + widgets, `metric-chart`'s line/area/bar over `ChartSeries` for series ones — + which is the tier the host's own operations pages use, so a solution's + dashboard and the host's own are the same picture. `DashboardWidget` is that + renderer for one widget, exported for a consumer that resolves each widget in + its own query; `slots` take header actions, a layout, a per-widget renderer + and an empty state, so a consumer composes its own features around the + renderer rather than beside it. What each widget and each coverage state + draws is tabulated in the module's `AUDIT_METRICS.md`. This is the surface a solution fe-remote consumes. `` is fed by `@codefly-dev/saas-sdk`'s `useChatStream` — the hook owns the SSE/WS transport, the component stays pure, the same split as `runDashboard` → ``. @@ -106,7 +115,7 @@ solution measured where it had to reach past the kit. | `@codefly-dev/ui/plugin-host/ui` | Client UI adapters (`PluginErrorBoundary`) | | `@codefly-dev/ui/skin` | `resolveSkin`, skin types | | `@codefly-dev/ui/layout` | `Card`/`Section`/`Tabs` + shadcn primitives (React-only) | -| `@codefly-dev/ui/dashboard` | `Dashboard`, charts, `fromDashboardData` (React-only) | +| `@codefly-dev/ui/dashboard` | `Dashboard`, `DashboardWidget`, metric tiles + charts, `fromDashboardData` (React-only) | | `@codefly-dev/ui/chat` | `Chat` (React-only) | | `@codefly-dev/ui/content` | `Content`, `Markdown`, `JsonView`, `CodeBlock`, `TextBlock` (React-only) | | `@codefly-dev/ui/board` | `Board` (React-only) | diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx index cd9ffb4a2..d305553f5 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx @@ -1,77 +1,449 @@ // @vitest-environment happy-dom -import { cleanup, render, screen } from "@testing-library/react"; +import { cleanup, render, screen, within } from "@testing-library/react"; import { afterEach, describe, expect, it } from "vitest"; import { Dashboard } from "../dashboard.js"; -import type { DashboardView } from "../types.js"; +import type { + DashboardView, + DashboardWidgetView, + WidgetSeries, +} from "../types.js"; afterEach(cleanup); -const emptySeries = { points: [], total: 0 }; +// Two daily buckets: a time-bucketed series, so it earns a trend. +const daily: WidgetSeries = { + points: [ + { key: "2026-09-01T00:00:00Z", value: 3 }, + { key: "2026-09-02T00:00:00Z", value: 5 }, + ], + total: 8, +}; + +// The same counts grouped by a category: an ordered line through these would +// assert a direction the data does not have. +const byCategory: WidgetSeries = { + points: [ + { key: "identity", value: 3 }, + { key: "security", value: 5 }, + ], + total: 8, +}; + +const empty: WidgetSeries = { points: [], total: 0, coverage: "empty" }; + +function view(...widgets: DashboardWidgetView[]): DashboardView { + return { widgets }; +} + +function show(...widgets: DashboardWidgetView[]) { + return render(); +} describe("Dashboard", () => { - it("renders its header through Section and each widget through Card", () => { - const data: DashboardView = { - title: "Traffic", - description: "last 7 days", - widgets: [ - { - id: "w1", - title: "Visits", - visualization: "number", - series: emptySeries, - }, - ], - }; - const { container } = render(); - - // Section owns the header markup — its
root and heading classes. - const section = container.querySelector("section"); - expect(section).not.toBeNull(); + it("renders its header through Section and each series widget through Card", () => { + const { container } = render( + , + ); + + // Section owns the header markup — its
root and heading. + expect(container.querySelector("section")).not.toBeNull(); expect( screen.getByRole("heading", { name: "Traffic", level: 2 }), ).toBeTruthy(); expect(screen.getByText("last 7 days")).toBeTruthy(); - // Card owns the surface — one card per widget, painted by the primitive. - const card = container.querySelector( - ".rounded-lg.border.bg-card.p-4.text-card-foreground.shadow-sm", - ); - expect(card).not.toBeNull(); + // Card owns the surface — one card per series widget, painted by the + // primitive, with the widget's title as its heading. + expect( + container.querySelector( + ".rounded-lg.border.bg-card.p-4.text-card-foreground.shadow-sm", + ), + ).not.toBeNull(); expect( screen.getByRole("heading", { name: "Visits", level: 3 }), ).toBeTruthy(); }); it("scopes the accent override onto the Section root", () => { - const data: DashboardView = { - accent: "hotpink", - widgets: [{ id: "w1", visualization: "number", series: emptySeries }], - }; - const { container } = render(); + const { container } = render( + , + ); const section = container.querySelector("section") as HTMLElement; expect(section.style.getPropertyValue("--primary")).toBe("hotpink"); }); + + it("says so, rather than drawing an empty grid, when the view has no widgets", () => { + show(); + expect(screen.getByText("No widgets.")).toBeTruthy(); + }); }); -it("preserves partial telemetry and unavailable totals from the SDK view", () => { - render( - { + it("draws a metric chart with an accessible data table", () => { + const { container } = render( + , + ); + const table = screen.getByRole("table", { name: "Events" }); + expect(within(table).getByRole("row", { name: /identity/ })).toBeTruthy(); + expect(within(table).getByText("3")).toBeTruthy(); + expect(within(table).getByText("5")).toBeTruthy(); + expect(container.querySelector("svg")?.getAttribute("aria-hidden")).toBe( + "true", + ); + }); + + it("draws the chart's own empty state instead of an axis with nothing on it", () => { + const { container } = render( + , + ); + expect( + container.querySelector("figure")?.getAttribute("aria-label"), + ).toBe("Events: no data"); + expect(container.querySelector("svg")).toBeNull(); + // Said twice on purpose, and in two registers: the card badges the + // coverage beside its title, and the plot says it where the geometry + // would have been. + expect(screen.getAllByText("No data")).toHaveLength(2); + }); + + it("badges a partial series on the card, from `series.coverage`", () => { + render( + , + ); + expect(screen.getByText("Partial data")).toBeTruthy(); + // A partial series keeps its observed points: the chart still draws. + expect(screen.getByRole("table", { name: "Events" })).toBeTruthy(); + }); + }, +); + +describe("a number widget", () => { + it("is a stat tile showing the series total in the widget's format", () => { + show({ + id: "w", + title: "Revenue", + visualization: "number", + series: { points: [{ key: "all", value: 4_200_000 }], total: 4_200_000 }, + format: "currency", + }); + expect(screen.getByText("Revenue")).toBeTruthy(); + expect(screen.getByText("$4.2M")).toBeTruthy(); + }); + + it("appends the widget's unit", () => { + show({ + id: "w", + title: "Throughput", + visualization: "number", + series: { points: [{ key: "all", value: 42 }], total: 42 }, + unit: "req/s", + }); + expect(screen.getByText("req/s")).toBeTruthy(); + }); + + it("sparklines a time-bucketed metric and deltas its last bucket", () => { + const { container } = show({ + id: "w", + title: "Logins", + visualization: "number", + series: daily, + }); + expect(screen.getByText("8")).toBeTruthy(); + expect(container.querySelector("svg")).not.toBeNull(); + // 3 → 5 between the last two buckets. + expect(screen.getByText(/66\.7%/).textContent).toContain( + "vs previous bucket", + ); + }); + + it("draws neither a trend nor a delta for a metric grouped by category", () => { + // A sparkline over "identity, security" would read as a direction, and a + // delta between them as a change over time. Neither is true of a + // categorical grouping, whose order is the aggregate's, not time's. + const { container } = show({ + id: "w", + title: "Logins", + visualization: "number", + series: byCategory, + }); + expect(screen.getByText("8")).toBeTruthy(); + expect(container.querySelector("svg")).toBeNull(); + expect(screen.queryByText(/%/)).toBeNull(); + }); + + it("gives no delta for a rise from zero, which is not a percentage", () => { + show({ + id: "w", + visualization: "number", + series: { + points: [ + { key: "2026-09-01T00:00:00Z", value: 0 }, + { key: "2026-09-02T00:00:00Z", value: 5 }, ], - }} - />, - ); - expect(screen.getByRole("status").textContent).toBe("Partial telemetry"); - expect(screen.getByText("Total unavailable")).toBeTruthy(); - expect(screen.queryByText("0")).toBeNull(); + total: 5, + }, + }); + expect(screen.queryByText(/%/)).toBeNull(); + }); + + it("shows a dash and says no total, never a 0, when the total is withheld", () => { + // `total: null` means the pipeline cannot name one number — a distinct + // count or an average across groups. Rendering it as 0 would report + // "nothing happened" where the answer is "cannot say". + show({ + id: "w", + title: "Distinct actors", + visualization: "number", + series: { ...byCategory, total: null }, + }); + expect(screen.getByText("—")).toBeTruthy(); + expect(screen.getByText("No total")).toBeTruthy(); + expect(screen.queryByText("0")).toBeNull(); + }); + + it("badges a partial series, which is why its total is withheld", () => { + show({ + id: "w", + title: "Logins", + visualization: "number", + series: { ...byCategory, total: null, coverage: "partial" }, + }); + expect(screen.getByText("Partial data")).toBeTruthy(); + expect(screen.getByText("—")).toBeTruthy(); + expect(screen.queryByText("No total")).toBeNull(); + }); + + it("reads an empty series as no data, not as a zero", () => { + // The audit aggregate omits a bucket rather than emitting a zero one, so + // a series with no points means nothing matched. + show({ id: "w", title: "Logins", visualization: "number", series: empty }); + expect(screen.getByText("No data")).toBeTruthy(); + expect(screen.getByText("—")).toBeTruthy(); + expect(screen.queryByText("0")).toBeNull(); + }); + + it("keeps a real zero a zero", () => { + show({ + id: "w", + title: "Failures", + visualization: "number", + series: { points: [{ key: "all", value: 0 }], total: 0 }, + }); + expect(screen.getByText("0")).toBeTruthy(); + expect(screen.queryByText("—")).toBeNull(); + }); +}); + +describe("a table widget", () => { + it("lists each point's key and value", () => { + show({ + id: "w", + title: "By type", + visualization: "table", + series: byCategory, + }); + const table = screen.getByRole("table", { name: "By type" }); + expect(within(table).getByText("identity")).toBeTruthy(); + expect(within(table).getByText("5")).toBeTruthy(); + }); + + it("says there is nothing yet for an empty series", () => { + show({ id: "w", title: "By type", visualization: "table", series: empty }); + expect(screen.getByText("No data yet.")).toBeTruthy(); + }); +}); + +describe("a widget still being resolved, or that failed", () => { + it("waits on a null series rather than drawing it as empty", () => { + const { container } = show( + { id: "chart", title: "Logins", visualization: "line", series: null }, + { id: "tile", title: "Total", visualization: "number", series: null }, + ); + expect(container.querySelectorAll('[data-slot="skeleton"]')).toHaveLength( + 2, + ); + expect(screen.queryByText("No data")).toBeNull(); + }); + + it("says a failed widget failed, and does not wait forever on its missing series", () => { + // A failed resolution leaves no series behind, so a renderer that tested + // for the series first would draw every failure as an endless wait. + const { container } = show( + { + id: "chart", + title: "Logins", + visualization: "line", + series: null, + failed: true, + }, + { + id: "tile", + title: "Total", + visualization: "number", + series: null, + failed: true, + }, + ); + expect(screen.getByText("Unable to load.")).toBeTruthy(); + expect(screen.getByText("Provider unavailable")).toBeTruthy(); + expect(container.querySelector('[data-slot="skeleton"]')).toBeNull(); + }); +}); + +describe("the default layout", () => { + it("gathers adjacent scalars into one KPI row and keeps declared order", () => { + const { container } = show( + { id: "a", title: "A", visualization: "number", series: daily }, + { id: "b", title: "B", visualization: "number", series: daily }, + { id: "c", title: "C", visualization: "line", series: daily }, + { id: "d", title: "D", visualization: "number", series: daily }, + ); + // Two tiles, then a chart, then a tile: three runs, the KPI row's own + // four-up geometry only around the adjacent pair. + const rows = [ + ...container.querySelectorAll('section > div > div[class*="grid"]'), + ]; + expect(rows).toHaveLength(3); + expect(rows[0].className).toContain("lg:grid-cols-4"); + expect(rows[0].children).toHaveLength(2); + expect(rows[1].className).toContain("sm:grid-cols-2"); + expect(rows[2].children).toHaveLength(1); + expect( + [...container.querySelectorAll('[data-slot="metric-label"], h3')].map( + (node) => node.textContent, + ), + ).toEqual(["A", "B", "C", "D"]); + }); + + it("stacks every widget in a row of its own for a stack layout", () => { + const { container } = render( + , + ); + expect(container.querySelector("section > div")?.className).toContain( + "flex flex-col", + ); + expect(container.querySelector('[class*="grid-cols-4"]')).toBeNull(); + }); +}); + +// The seams a host composes its own features through. They are what let the +// host's solution dashboard keep its reorderable grid, its + menu and its +// per-widget queries while this renderer still does the drawing. +describe("slots", () => { + const widgets: DashboardWidgetView[] = [ + { id: "a", title: "A", visualization: "line", series: daily }, + { id: "b", title: "B", visualization: "number", series: daily }, + ]; + + it("puts actions in the dashboard's own header", () => { + render( + Add }} + />, + ); + expect(screen.getByRole("button", { name: "Add" })).toBeTruthy(); + }); + + it("hands every widget to renderWidget, in the view's order", () => { + render( +
  • {widget.id}
    • }} + />, + ); + expect( + screen.getAllByRole("listitem").map((item) => item.textContent), + ).toEqual(["a", "b"]); + }); + + it("hands the tiles, each with its widget, to a layout of the host's own", () => { + render( + ( +
      + {tiles.map((tile) => ( +
    • + {tile.node} +
      • + ))} +
    + ), + }} + />, + ); + const items = screen.getAllByRole("listitem"); + expect(items.map((item) => item.dataset.visualization)).toEqual([ + "line", + "number", + ]); + // The nodes are the kit's own cards: the chart's data table is in them. + expect(within(items[0]).getByRole("table", { name: "A" })).toBeTruthy(); + }); + + it("shows the host's own empty state in place of the layout", () => { + render( + Add one back with +.

    }} + />, + ); + expect(screen.getByText("Add one back with +.")).toBeTruthy(); + expect(screen.queryByText("No widgets.")).toBeNull(); + }); }); diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/types.test.ts b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/types.test.ts index 02f7d839d..397e29328 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/types.test.ts +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/types.test.ts @@ -25,8 +25,14 @@ describe("fromDashboardData", () => { expect(view.columns).toBe(3); expect(view.accent).toBe("#7c3aed"); expect(view.widgets).toHaveLength(1); - expect(view.widgets[0]).toMatchObject({ id: "trend", visualization: "line" }); - expect(view.widgets[0].series.total).toBe(3); + expect(view.widgets[0]).toMatchObject({ + id: "trend", + visualization: "line", + }); + // `fromDashboardData` bridges an ALREADY-resolved result, so the series + // it produces is never the `null` the view model allows for a widget + // still being resolved. + expect(view.widgets[0].series?.total).toBe(3); }); it("defaults layout to grid when the source omits it", () => { diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx index 978723a30..63d8f5fc6 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx @@ -6,13 +6,35 @@ // solution's Module-Federation remote render identical dashboards from the same // package instance. Data resolution (metric → audit query) is the job of // `@codefly-dev/saas-sdk`'s `runDashboard`; use `fromDashboardData` to bridge. +// +// It paints with the kit's METRIC tier — `StatTile`/`KPIRow` for scalars, +// `metric-chart`'s line/area/bar over `ChartSeries` for series — which is the +// tier the host's own operations pages are drawn with. Before, this renderer +// and the host's solution dashboard both used the older `charts.tsx` tier, so a +// solution's Dashboard tab read plainer than the host's own pages from the same +// kit, and there were two renderers to keep in step rather than one. import type * as React from "react"; +import { Fragment, type ReactNode } from "react"; import { Card } from "../layout/card.js"; import { Section } from "../layout/page.js"; -import { AreaChart, BarList, LineChart, StatChart } from "./charts.js"; +import { Skeleton } from "../layout/skeleton.js"; import { cn } from "./cn.js"; -import type { DashboardView, DashboardWidgetView } from "./types.js"; +import { parseTimeKey } from "./format.js"; +import { + AreaChart as MetricAreaChart, + BarChart as MetricBarChart, + LineChart as MetricLineChart, +} from "./metric-chart.js"; +import type { ChartSeries } from "./metric-geometry.js"; +import { type MetricState, MetricStateBadge } from "./metric-state.js"; +import { KPIRow, type Metric, StatTile } from "./metric-tiles.js"; +import type { + DashboardView, + DashboardWidgetView, + SeriesPoint, + WidgetSeries, +} from "./types.js"; // Column-span utility classes, matching the responsive grid below so a spanning // card widens in step with it. A span is clamped to the grid's column count by @@ -31,33 +53,129 @@ const GRID_COLS: Record<1 | 2 | 3 | 4, string> = { 4: "grid-cols-1 sm:grid-cols-2 lg:grid-cols-4", }; -function WidgetBody({ widget }: { widget: DashboardWidgetView }) { - const { series, visualization } = widget; - if (series.points.length === 0) { - return
    No data yet.
    ; +/** The widget's own title, or its id when the declaration gave none. */ +function widgetTitle(widget: DashboardWidgetView): string { + return widget.title ?? widget.id; +} + +/** + * Whether the series is bucketed over time, so its points are a trend rather + * than a set of categories. Only a time-bucketed series earns a sparkline and + * a bucket-over-bucket delta: drawing a line through counts grouped by event + * type or region would assert an order and a direction the data does not have. + */ +function isTimeBucketed(points: SeriesPoint[]): boolean { + return points.length > 1 && points.every((p) => parseTimeKey(p.key) !== null); +} + +/** + * The badge a resolved series earns, or `undefined` when it is simply ready. + * + * `partial` outranks a withheld total because it is the *reason* the total is + * withheld (a partial series keeps its observed points and gives up its total), + * and a reader who sees "No total" alone learns less than one who sees + * "Partial data". An empty series is `no_data`, never a zero: the audit + * aggregate omits a bucket rather than emitting one, so no points means nothing + * matched. + */ +function seriesState(series: WidgetSeries): MetricState | undefined { + if (series.coverage === "partial") return "partial"; + if (series.coverage === "empty" || series.points.length === 0) { + return "no_data"; } - switch (visualization) { + if (series.total === null) return "no_total"; + return undefined; +} + +/** + * Change from the second-to-last bucket to the last, as the signed fraction + * `StatTile` renders. A change from zero is not a percentage, so it is no + * delta at all rather than an infinite one. + */ +function lastBucketDelta(points: SeriesPoint[]): number | undefined { + if (points.length < 2) return undefined; + const previous = points[points.length - 2].value; + const latest = points[points.length - 1].value; + if (previous === 0) return undefined; + return (latest - previous) / Math.abs(previous); +} + +/** A resolved scalar widget as the `Metric` a tile binds to. */ +function widgetMetric( + widget: DashboardWidgetView, + series: WidgetSeries, +): Metric { + const trend = isTimeBucketed(series.points); + const delta = trend ? lastBucketDelta(series.points) : undefined; + return { + id: widget.id, + label: widgetTitle(widget), + // A withheld total is passed through as null, so the tile shows a dash + // and its badge says what withheld it. Coercing it to 0 here would + // report "nothing happened" where the pipeline said "cannot say". + value: series.total, + format: widget.format, + unit: widget.unit, + higherIsBetter: widget.higherIsBetter, + series: trend ? series.points.map((p) => p.value) : undefined, + delta, + // Only named when there is a delta: `MetricDelta` reads a label with no + // delta as "no data
    ); diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx index 6ed791030..e3bb165cd 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx @@ -260,8 +260,12 @@ export function StatTile({ {...selectable.props} >
    + {/* Truncated because the header also carries a badge and, for a + caller that owns the tile, its controls — with the full text + in `title`, as BarList does, so clipping it does not lose it. */} {metric.label} @@ -355,11 +359,10 @@ export function KPIRow({ metrics, children, className, -}: { - metrics?: Metric[]; - children?: ReactNode; - className?: string; -}) { +}: { className?: string | false } & ( + | { metrics: Metric[]; children?: never } + | { children: ReactNode; metrics?: never } +)) { return (
    {children ?? diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/types.ts b/module/services/frontend/code/packages/codefly-ui/src/dashboard/types.ts index 25acfe944..05512ecb6 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/types.ts +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/types.ts @@ -49,7 +49,15 @@ export interface DashboardWidgetView { failed?: boolean; /** Grid columns this widget spans (1–4); ignored in a stack. */ span?: 1 | 2 | 3 | 4; - /** How a scalar widget's value is rendered. Defaults to a plain number. */ + /** + * How a scalar widget's value is rendered. Defaults to a plain number. + * + * `"percent"` takes a value that is ALREADY a percentage: 45 renders "45%". + * It does not take a share — 0.4 renders "0.4%", not "40%". A producer whose + * own `percent` means a 0-to-1 share must scale it before it reaches here, + * because this field is bound structurally (see `fromDashboardData`) and + * nothing on the wire can tell the two conventions apart. + */ format?: MetricFormat; /** Suffix after a scalar widget's value, e.g. "req/s". */ unit?: string; diff --git a/module/services/frontend/code/packages/codefly-ui/src/skin/preview.generated.css b/module/services/frontend/code/packages/codefly-ui/src/skin/preview.generated.css index 79c8e1603..20b57248a 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/skin/preview.generated.css +++ b/module/services/frontend/code/packages/codefly-ui/src/skin/preview.generated.css @@ -4438,6 +4438,10 @@ max-width: var(--container-sm); } + .sm\:grid-cols-1 { + grid-template-columns: repeat(1, minmax(0, 1fr)); + } + .sm\:grid-cols-2 { grid-template-columns: repeat(2, minmax(0, 1fr)); } @@ -4553,6 +4557,14 @@ grid-column: span 4 / span 4; } + .lg\:grid-cols-1 { + grid-template-columns: repeat(1, minmax(0, 1fr)); + } + + .lg\:grid-cols-2 { + grid-template-columns: repeat(2, minmax(0, 1fr)); + } + .lg\:grid-cols-3 { grid-template-columns: repeat(3, minmax(0, 1fr)); } diff --git a/module/services/frontend/code/packages/codefly-ui/stories/metrics.stories.tsx b/module/services/frontend/code/packages/codefly-ui/stories/metrics.stories.tsx index f5066f219..70cbd65e5 100644 --- a/module/services/frontend/code/packages/codefly-ui/stories/metrics.stories.tsx +++ b/module/services/frontend/code/packages/codefly-ui/stories/metrics.stories.tsx @@ -1,12 +1,12 @@ -import { TrendLineChart } from "../src/dashboard/index.js"; import { - MetricCard, - StatTile, KPIRow, MetricAreaChart, MetricBarChart, + MetricCard, MetricLineChart, type MetricState, + StatTile, + TrendLineChart, } from "../src/dashboard/index.js"; export default { title: "Shared UI/Metrics" }; @@ -56,6 +56,7 @@ export const Availability = { "stale", "provider_unavailable", "not_configured", + "no_total", ] satisfies MetricState[] ).map((state) => ( diff --git a/module/services/frontend/code/src/components/__tests__/kit.test.ts b/module/services/frontend/code/src/components/__tests__/kit.test.ts index 0278abef2..5e968d620 100644 --- a/module/services/frontend/code/src/components/__tests__/kit.test.ts +++ b/module/services/frontend/code/src/components/__tests__/kit.test.ts @@ -103,6 +103,34 @@ function walk(dir: string, out: string[] = []): string[] { return out; } +/** + * `export *` clauses in the kit's entry points, by the module they name. + * + * The export scan below reads `export { … }` blocks, so a star re-export is + * invisible to it: the kit's root and plugin-host entry points use one, and + * four capitalised names reach consumers only that way. None collides with a + * host declaration today, but a partial miss is indistinguishable from success + * — the `kit.size` sanity check passes at 309 whether those four are in it or + * not — so the gate refuses a star it cannot resolve rather than quietly + * covering less than it claims. + */ +function starReExports( + entryPoints: { path: string; source: string }[], +): string[] { + const unresolved: string[] = []; + for (const { path, source } of entryPoints) { + for (const star of source.matchAll( + /export\s+\*\s+from\s*["']([^"']+)["']/g, + )) { + // A star onto another of the kit's own entry points resolves: that + // file is itself in this list and its named exports are read there. + if (star[1].startsWith("./") || star[1].startsWith("../")) continue; + unresolved.push(`${path} → ${star[1]}`); + } + } + return unresolved; +} + /** Every capitalised name the kit's entry points export. */ function kitExports(entryPoints: string[]): Set { const names = new Set(); @@ -124,28 +152,95 @@ function kitExports(entryPoints: string[]): Set { } /** - * The names a file imports FROM the kit, under the kit's own spelling: the `X` - * of `import { X as KitX } from "@codefly-dev/ui/layout"`. A wrapper is - * recognised by this, not by its alias, so renaming the alias cannot turn a - * re-implementation into a wrapper. + * Modules of this app that exist only to re-export the kit — the host's own + * import surface. `src/shared/ui/index.ts` says so in its first line ("Host + * convenience imports; presentation is implemented by the shared kit"), and + * most of the app reaches the kit through it rather than through + * `@codefly-dev/ui` directly. + * + * They are kit sources for the purposes of the rule below. Without this, the + * gate's verdict depended on which path a wrapper imported through: the same + * `Badge` wrapper passed when it imported `@codefly-dev/ui/layout` and failed + * when it imported the identical component from `@/shared/ui`. A gate that + * rejects the house style is one that gets an exception entry per correct file, + * and then gets deleted. + */ +const KIT_SURFACES = [ + "src/shared/ui/index.ts", + "src/components/charts/chart.tsx", + "src/components/charts/index.ts", + "src/components/sparkline.tsx", + "src/components/metric-state.tsx", +]; + +const KIT_SPECIFIER = /^@codefly-dev\/(?:ui|saas-ui)(?:\/|$)/; + +/** + * The named bindings an `import`/`export … from` clause carries, as + * published-name → exporter's-own-name. The two differ under an alias, and + * which one a check wants is the whole question: a wrapper is recognised by the + * exporter's name, so renaming the alias cannot turn a re-implementation into + * a wrapper. */ -function kitImports(source: string): Set { - const imported = new Set(); - const from = - /import\s+(?:type\s+)?\{([^}]*)\}\s*from\s*["']@codefly-dev\/(?:ui|saas-ui)(?:\/[\w./-]+)?["']/g; - for (const block of source.matchAll(from)) { +function namedBindings( + source: string, + isWanted: (specifier: string) => boolean, +): Map { + const bindings = new Map(); + const clause = + /(?:import|export)\s+(?:type\s+)?\{([^}]*)\}\s*from\s*["']([^"']+)["']/g; + for (const block of source.matchAll(clause)) { + if (!isWanted(block[2])) continue; for (const raw of block[1].split(",")) { - const name = raw + const parts = raw .trim() .replace(/^type\s+/, "") - .split(/\s+as\s+/)[0] - .trim(); - if (name) imported.add(name); + .split(/\s+as\s+/) + .map((part) => part.trim()); + const exported = parts[0]; + if (exported) bindings.set(parts[1] ?? exported, exported); } } - return imported; + return bindings; +} + +/** + * The names a file imports FROM the kit, under the kit's own spelling: the `X` + * of `import { X as KitX } from "@codefly-dev/ui/layout"`. A wrapper is + * recognised by this, not by its alias, so renaming the alias cannot turn a + * re-implementation into a wrapper. + * + * An import through a host surface counts too, under the name THAT surface + * publishes — which is not always the kit's: `src/shared/ui` re-exports + * `CardRoot` as `Card`, so importing `Card` from there composes `CardRoot` and + * a declaration named `Card` beside it is still a second `Card`, exactly as if + * it had imported `CardRoot` directly. + */ +function kitImports(source: string, surface: Map): Set { + const composed = new Set(); + for (const kitName of namedBindings(source, (specifier) => + KIT_SPECIFIER.test(specifier), + ).values()) { + composed.add(kitName); + } + // Through a host surface, the name imported is the one THAT file publishes, + // so it is resolved back to the kit's own. `src/shared/ui` re-exports + // `CardRoot` as `Card`, so importing `Card` from there composes `CardRoot` + // — and a declaration named `Card` beside it is still a second `Card`, + // exactly as if it had imported `CardRoot` directly. Resolving the alias is + // what keeps those two spellings of one import judged the same way. + for (const published of namedBindings(source, (specifier) => + HOST_SURFACE_SPECIFIER.test(specifier), + ).values()) { + const kitName = surface.get(published); + if (kitName) composed.add(kitName); + } + return composed; } +const HOST_SURFACE_SPECIFIER = + /^@\/(?:shared\/ui|components\/charts|components\/sparkline|components\/metric-state)(?:\/|$)/; + /** * A component declaration, exported or module-local, at any indentation — a * duplicate nested inside another component is still a duplicate, and the @@ -180,10 +275,13 @@ function findOffences( files: { path: string; source: string }[], kit: Set, allowed: Record = ALLOWED, + /** Published name → kit name for the host's own surfaces; see {@link KIT_SURFACES}. */ + surface: Map = new Map(), ): Offence[] { const offences: Offence[] = []; for (const { path, source } of files) { - const composed = kitImports(source); + if (KIT_SURFACES.includes(path)) continue; + const composed = kitImports(source, surface); const routeFile = ROUTE_CONVENTION.test(path); const seen = new Set(); for (const declaration of source.matchAll(DECLARATION)) { @@ -212,11 +310,31 @@ function appSources(): { path: string; source: string }[] { })); } -function kitNames(): Set { - const entries = walk(join(appRoot, "packages/codefly-ui/src")) +function kitEntryPoints(): { path: string; source: string }[] { + return walk(join(appRoot, "packages/codefly-ui/src")) .filter((file) => /[/\\]index\.ts$/.test(file)) - .map((file) => readFileSync(file, "utf8")); - return kitExports(entries); + .map((file) => ({ + path: relative(appRoot, file).replaceAll("\\", "/"), + source: readFileSync(file, "utf8"), + })); +} + +function kitNames(): Set { + return kitExports(kitEntryPoints().map((entry) => entry.source)); +} + +/** Each host surface's published name → the kit name behind it. */ +function hostSurface(): Map { + const surface = new Map(); + for (const path of KIT_SURFACES) { + const source = readFileSync(join(appRoot, path), "utf8"); + for (const [published, kitName] of namedBindings(source, (specifier) => + KIT_SPECIFIER.test(specifier), + )) { + surface.set(published, kitName); + } + } + return surface; } describe("the host app composes the kit rather than re-implementing it", () => { @@ -227,13 +345,26 @@ describe("the host app composes the kit rather than re-implementing it", () => { "the kit's entry points were not readable, so this gate proves nothing", ).toBeGreaterThan(100); - const offences = findOffences(appSources(), kit).map( + const offences = findOffences( + appSources(), + kit, + ALLOWED, + hostSurface(), + ).map( ({ name, path }) => `${name} in ${path} — @codefly-dev/ui already exports it. Compose the kit's component, or add "${name} in ${path}" to ALLOWED with what the two things are and why neither can stand in for the other.`, ); expect(offences).toEqual([]); }); + it("reads every name the kit exports, star re-exports included", () => { + // The gate covers what it can parse. A star onto a package outside the + // kit's own tree is not parsed, so it is refused rather than missed. + expect(starReExports(kitEntryPoints())).toEqual([ + "packages/codefly-ui/src/plugin-host/index.ts → @codefly-dev/saas-plugin-react", + ]); + }); + it("cannot build a kit-shaped primitive: the library for building them is not a dependency", () => { // The strongest leg, because it is a fact rather than a convention: `cva` // is how the kit composes its variants, and it left this package's @@ -285,9 +416,16 @@ describe("the host app composes the kit rather than re-implementing it", () => { // another route. Each fixture is a source that is not in the tree, so these // hold whatever the tree happens to contain. describe("the detector", () => { - const kit = new Set(["Card", "CardRoot", "StatTile", "Page"]); + const kit = new Set(["Card", "CardRoot", "StatTile", "Page", "Badge"]); + // What the host's surfaces publish, as `src/shared/ui` really does it: one + // name straight through, one renaming a kit primitive to another kit + // component's name. + const surface = new Map([ + ["Badge", "Badge"], + ["Card", "CardRoot"], + ]); const detect = (path: string, source: string) => - findOffences([{ path, source }], kit, {}).map( + findOffences([{ path, source }], kit, {}, surface).map( (o) => `${o.name} in ${o.path}`, ); @@ -395,6 +533,48 @@ describe("the detector", () => { ).toEqual(["Card in src/app/admin/users/page.tsx"]); }); + it("lets a wrapper compose the kit through the host's own import surface", () => { + // The path a wrapper imports through must not change the verdict. This + // is the same component, and the same wrapper, as the kit-direct case + // above; `@/shared/ui` publishes `Badge` straight from the kit. + expect( + detect( + "src/features/x/ui/badge.tsx", + `import { Badge as KitBadge } from "@/shared/ui"; + export function Badge(props: ComponentProps) { + return ; + }`, + ), + ).toEqual([]); + }); + + it("resolves the surface's alias, so one import is judged one way either way", () => { + // `@/shared/ui` publishes the kit's `CardRoot` under the name `Card`, so + // importing `Card` from there composes `CardRoot` — and a declaration + // named `Card` is a second `Card`, exactly as the kit-direct form of + // this is caught above. Without resolving the alias the two spellings + // of one import would get opposite verdicts. + expect( + detect( + "src/features/x/ui/card.tsx", + `import { Card as HostCard } from "@/shared/ui"; + export function Card({ title }: { title: string }) { + return {title}; + }`, + ), + ).toEqual(["Card in src/features/x/ui/card.tsx"]); + }); + + it("does not read the host's surfaces themselves as duplicates", () => { + // A surface file re-exports under a kit name by construction. + expect( + detect( + "src/shared/ui/index.ts", + `export { CardRoot as Card, Badge } from "@codefly-dev/ui/layout";`, + ), + ).toEqual([]); + }); + it("ignores a name the kit does not export", () => { expect( detect( From b82cc700bec2aa37b06769cf39fdcc1958fe3c66 Mon Sep 17 00:00:00 2001 From: Codefly Automation <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 11 Oct 2026 09:25:10 -0400 Subject: [PATCH 3/6] fix(dashboard): a refused read keeps no values, and the duplication gate reads the AST (#1041) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent review of this branch at 998e17d5 found one blocker, two majors and two minors. Each is fixed at its cause and mutation-checked: the mutation that re-creates the defect is shown to red a test. **BLOCKER — a permission-denied refetch kept the audit values on screen.** `SolutionWidget` forwarded cached data with a failure flag for every query error, so a viewer whose audit-read authority had just been revoked went on reading the counts, now labelled "Stale". Before this branch the host's `isError` branch withheld them, so this was a regression introduced by the retain-on-failure fix — the right answer for a transient failure, the wrong one for a refusal. Fixed with the policy that already exists for it: `readOutcome` classifies the error and `mayKeepRetainedRows` decides whether the series survives, exactly as the admin tables do. `src/shared/lib/read-outcome.ts` documents this trap in its own words — TanStack Query hands back the last successful data when a refetch rejects — which is what makes it the owner rather than a new policy here. The kit's `failed?: boolean` becomes `failure?: "failed" | "refused"`, because a boolean cannot carry the distinction the blocker is about. `MetricState` gains `forbidden` ("No access"), and the renderer answers a refusal before it looks at the series in every branch, so a caller that forwards retained values by mistake still cannot get them drawn. Reordering those branches to test the series first — the version that reads more naturally — reds the test. Regression: a success followed by a 403 discards the values and says "No access"; the same success followed by a 503 keeps them and says "Stale"; both assert the Connect code they classified on. **MAJOR — the duplication gate permitted re-implementations and rejected real composition.** It is rebuilt on the TypeScript AST and now checks composition *within the declaration*, because a file-wide import proved nothing: an unrelated `export { Card as KitCard } from "@codefly-dev/ui/layout"` exempted a hand-rolled `Card` beside it, `export { LocalPanel as Card }` published a copy under a kit name unseen, a namespace or relative import of a host surface was rejected, and the root route's conventional `Page` was rejected because the exemption only matched nested routes. All now have fixtures. The fixture that mattered needed a new leg. The renderer this gate exists to prevent — the one #1041 deleted — declares `WidgetBody`, `WidgetValues` and `WidgetCard`, and not one of them is a name the kit exports, so no name-based rule could ever have caught it. A host file may now draw with at most ONE of the kit's dashboard marks; reaching for several is a widget dispatch, which is ``'s job. The fixture reads the real deleted source out of `8590d329` rather than a paraphrase. Two live files draw with more than one, both already named in ALLOWED for the same reason; they are listed in DISPATCHERS with what would retire each, and removing one reds the gate. **MAJOR — a failed refresh turned a known-empty scalar into a reported zero.** Replacing the coverage state with `stale` also changed whether the value counted as meaningful, so `{points: [], total: 0, coverage: "empty"}` went from "— / No data" to "0 / Stale". Availability is now read off the series and freshness off the badge: they are different questions about one tile and are answered apart. A partial series keeps its withheld total dashed through a failed refresh too. **MINOR — the chart migration had broken the public `accent` option.** The tier change moved where a mark's colour comes from: a metric chart paints its first series `var(--chart-1)`, while the basic tier used `--primary`. The accent now sets both, asserted on the rendered mark rather than on the variable. **MINOR — a titled scalar had lost its heading.** It was a `Card` with an `

    `; a `StatTile`'s label is a span, so the title left the document outline. `StatTile` gains `labelAs`, so the title is a heading again through the shared primitive rather than a restored host card. Refs #1041 Co-Authored-By: Claude Opus 5 (1M context) --- .../src/dashboard/__tests__/atoms.test.tsx | 27 +- .../src/dashboard/__tests__/charts.test.tsx | 13 +- .../dashboard/__tests__/dashboard.test.tsx | 111 +++- .../src/dashboard/__tests__/geometry.test.ts | 15 +- .../codefly-ui/src/dashboard/charts.tsx | 95 +++- .../codefly-ui/src/dashboard/dashboard.tsx | 108 +++- .../codefly-ui/src/dashboard/format.ts | 13 +- .../codefly-ui/src/dashboard/geometry.ts | 54 +- .../codefly-ui/src/dashboard/index.ts | 50 +- .../codefly-ui/src/dashboard/metric-state.tsx | 7 + .../codefly-ui/src/dashboard/metric-tiles.tsx | 14 +- .../codefly-ui/src/dashboard/types.ts | 23 +- .../code/src/components/__tests__/kit.test.ts | 480 ++++++++++++++---- .../code/src/solutions/SolutionDashboard.tsx | 30 +- .../__tests__/SolutionDashboard.test.tsx | 57 ++- 15 files changed, 882 insertions(+), 215 deletions(-) diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/atoms.test.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/atoms.test.tsx index fb6d0aeb2..36d76d9f8 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/atoms.test.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/atoms.test.tsx @@ -2,18 +2,30 @@ import { describe, expect, it } from "vitest"; import { Axis, Gridline } from "../atoms.js"; import { linearScale, sparkPlot } from "../geometry.js"; -type ElementLike = { type?: unknown; props?: { children?: unknown } & Record }; +type ElementLike = { + type?: unknown; + props?: { children?: unknown } & Record; +}; // Collect every element of a given SVG tag (a rendered component is a plain // object tree, so no DOM is needed) with its props and text child. Recurses into // child arrays — an axis holds one array of y labels and one of x labels. -function byTag(node: unknown, tag: string): { props: Record; text: string }[] { +function byTag( + node: unknown, + tag: string, +): { props: Record; text: string }[] { if (Array.isArray(node)) return node.flatMap((child) => byTag(child, tag)); if (!node || typeof node !== "object") return []; const el = node as ElementLike; const self = el.type === tag && el.props - ? [{ props: el.props, text: typeof el.props.children === "string" ? el.props.children : "" }] + ? [ + { + props: el.props, + text: + typeof el.props.children === "string" ? el.props.children : "", + }, + ] : []; return self.concat(byTag(el.props?.children, tag)); } @@ -34,13 +46,18 @@ describe("Gridline", () => { describe("Axis", () => { it("labels each y tick with the formatted value", () => { const scale = linearScale(0, 10, plot.bottom, plot.top); - const labels = byTag(Axis({ plot, y: { ticks: [0, 5, 10], scale } }), "text").map((t) => t.text); + const labels = byTag( + Axis({ plot, y: { ticks: [0, 5, 10], scale } }), + "text", + ).map((t) => t.text); expect(labels).toEqual(["0", "5", "10"]); }); it("time-formats ISO bucket keys on the x axis", () => { const keys = ["2026-09-01T00:00:00+00", "2026-09-02T00:00:00+00"]; - const labels = byTag(Axis({ plot, x: { keys } }), "text").map((t) => t.text); + const labels = byTag(Axis({ plot, x: { keys } }), "text").map( + (t) => t.text, + ); expect(labels).toEqual(["Sep 1", "Sep 2"]); }); diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/charts.test.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/charts.test.tsx index 600a9200d..42a18a171 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/charts.test.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/charts.test.tsx @@ -3,7 +3,10 @@ import { Axis, Svg } from "../atoms.js"; import { AreaChart, LineChart, StatChart } from "../charts.js"; import type { SeriesPoint } from "../types.js"; -type ElementLike = { type?: unknown; props?: { children?: unknown } & Record }; +type ElementLike = { + type?: unknown; + props?: { children?: unknown } & Record; +}; // Walk a React element tree (no DOM needed — a rendered component is a plain // object) collecting every 's props, so a test can assert what color the @@ -36,7 +39,9 @@ const pts: SeriesPoint[] = [ // a text-color class (and the Dashboard accent behind `text-primary`) theme it. describe("chart color inherits via currentColor", () => { it("LineChart strokes with currentColor", () => { - const strokes = pathProps(LineChart({ points: pts })).filter((p) => p.stroke && p.stroke !== "none"); + const strokes = pathProps(LineChart({ points: pts })).filter( + (p) => p.stroke && p.stroke !== "none", + ); expect(strokes.length).toBeGreaterThan(0); for (const p of strokes) expect(p.stroke).toBe("currentColor"); }); @@ -69,7 +74,9 @@ describe("axes gating", () => { expect(tree.props.stretch).toBe(false); expect(byType(tree, Axis)).toHaveLength(1); // The data path still colors via currentColor, not a hardcoded hue. - const strokes = pathProps(tree).filter((p) => p.stroke && p.stroke !== "none"); + const strokes = pathProps(tree).filter( + (p) => p.stroke && p.stroke !== "none", + ); expect(strokes.length).toBeGreaterThan(0); for (const p of strokes) expect(p.stroke).toBe("currentColor"); }); diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx index 26545a52a..b929ff9b7 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx @@ -72,6 +72,45 @@ describe("Dashboard", () => { ).toBeTruthy(); }); + it("reaches the token the metric charts actually paint with its accent", () => { + // The tier change moved where a mark's colour comes from: a metric + // chart colours its first series `var(--chart-1)`, not `--primary`, + // so an accent that set only `--primary` stopped colouring the charts + // it exists for. Asserted on the rendered mark, not just the variable. + const { container } = render( + , + ); + expect( + container.querySelector('path[stroke="var(--chart-1)"]'), + ).not.toBeNull(); + const section = container.querySelector("section") as HTMLElement; + expect(section.style.getPropertyValue("--chart-1")).toBe("hotpink"); + expect(section.style.getPropertyValue("--primary")).toBe("hotpink"); + }); + + it("keeps a titled scalar reachable by heading navigation", () => { + // It was a Card with an

    ; a StatTile's label is a span, so the + // title left the document outline when the tier changed. + render( + , + ); + expect( + screen.getByRole("heading", { name: "Total events", level: 3 }), + ).toBeTruthy(); + }); + it("scopes the accent override onto the Section root", () => { const { container } = render( { title: "Logins", visualization: "line", series: byCategory, - failed: true, + failure: "failed", }, { id: "tile", title: "Total", visualization: "number", series: daily, - failed: true, + failure: "failed", }, ); expect(screen.queryByText("Unable to load.")).toBeNull(); @@ -400,6 +439,70 @@ describe("a widget still being resolved, or that failed", () => { expect(container.querySelector('[data-slot="skeleton"]')).toBeNull(); }); + it("keeps an empty series valueless through a failed refresh", () => { + // The badge is one slot and freshness takes it, so deriving the value's + // presence from the state turned a known-empty scalar into a reported + // zero the moment its refresh failed. + const widget: DashboardWidgetView = { + id: "total", + visualization: "number", + series: empty, + }; + const { rerender } = render(); + expect(screen.getByText("—")).toBeTruthy(); + expect(screen.getByText("No data")).toBeTruthy(); + + rerender(); + expect(screen.getByText("Stale")).toBeTruthy(); + expect(screen.getByText("—")).toBeTruthy(); + expect(screen.queryByText("0")).toBeNull(); + }); + + it("keeps a partial series identifiable through a failed refresh", () => { + show({ + id: "w", + title: "Logins", + visualization: "number", + series: { ...byCategory, total: null, coverage: "partial" }, + failure: "failed", + }); + // Freshness takes the badge, but the withheld total still reads as one. + expect(screen.getByText("Stale")).toBeTruthy(); + expect(screen.getByText("—")).toBeTruthy(); + expect(screen.queryByText("8")).toBeNull(); + }); + + it("discards a series the caller retained on a refused read", () => { + // A number this viewer may not see is a WRONG answer, not an old one. + // The host is expected to withhold it; the renderer that owns what a + // refusal looks like fails closed too, so a caller that forwards + // retained values by mistake cannot show them through this. + const { container } = show( + { + id: "chart", + title: "Logins", + visualization: "line", + series: byCategory, + failure: "refused", + }, + { + id: "tile", + title: "Total", + visualization: "number", + series: daily, + failure: "refused", + }, + ); + expect(screen.queryByText("8")).toBeNull(); + expect(container.querySelector("svg")).toBeNull(); + expect(screen.queryByRole("table", { name: "Logins" })).toBeNull(); + expect(screen.getAllByText("No access")).toHaveLength(2); + expect( + screen.getByText("You don't have permission to see this."), + ).toBeTruthy(); + expect(screen.queryByText("Unable to load.")).toBeNull(); + }); + it("says a failed widget failed, and does not wait forever on its missing series", () => { // A failed resolution leaves no series behind, so a renderer that tested // for the series first would draw every failure as an endless wait. @@ -409,14 +512,14 @@ describe("a widget still being resolved, or that failed", () => { title: "Logins", visualization: "line", series: null, - failed: true, + failure: "failed", }, { id: "tile", title: "Total", visualization: "number", series: null, - failed: true, + failure: "failed", }, ); expect(screen.getByText("Unable to load.")).toBeTruthy(); diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/geometry.test.ts b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/geometry.test.ts index ff04c06ff..de9854fbb 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/geometry.test.ts +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/geometry.test.ts @@ -1,5 +1,12 @@ import { describe, expect, it } from "vitest"; -import { areaPath, linearScale, linePath, niceTicks, scaleX, sparkPlot } from "../geometry.js"; +import { + areaPath, + linearScale, + linePath, + niceTicks, + scaleX, + sparkPlot, +} from "../geometry.js"; import type { SeriesPoint } from "../types.js"; // Path coordinates are emitted as "M x y L x y …"; pull them out as alternating @@ -12,7 +19,8 @@ function coords(d: string): { xs: number[]; ys: number[] } { }; } -const pts = (...values: number[]): SeriesPoint[] => values.map((value, i) => ({ key: String(i), value })); +const pts = (...values: number[]): SeriesPoint[] => + values.map((value, i) => ({ key: String(i), value })); describe("linePath", () => { // Regression: scaleY used to close over the constant full viewBox height, so a @@ -118,7 +126,8 @@ describe("niceTicks", () => { const ticks = niceTicks(0, 95); expect(ticks[0]).toBe(0); expect(ticks[ticks.length - 1]).toBeGreaterThanOrEqual(95); - for (let i = 1; i < ticks.length; i++) expect(ticks[i]).toBeGreaterThan(ticks[i - 1]); + for (let i = 1; i < ticks.length; i++) + expect(ticks[i]).toBeGreaterThan(ticks[i - 1]); }); // A degenerate span (all-zero series, or a single point anchored at its own diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/charts.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/charts.tsx index 003f3267e..325b130fe 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/charts.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/charts.tsx @@ -14,17 +14,17 @@ import type * as React from "react"; import { Axis, Gridline, Svg } from "./atoms.js"; import { cn } from "./cn.js"; import { - areaPath, AXIS_MARGIN, + areaPath, linearScale, linePath, niceTicks, PAD, plot, sparkPlot, - valueDomain, VIEW_H, VIEW_W, + valueDomain, } from "./geometry.js"; import type { SeriesPoint } from "./types.js"; @@ -66,8 +66,21 @@ function cartesian({ const p = sparkPlot(height); return ( - {fill && } - + {fill && ( + + )} + ); } @@ -86,10 +99,28 @@ function cartesian({ const singleY = points.length === 1 ? y(points[0].value) : undefined; return ( - + {showY && } - {fill && } - + {fill && ( + + )} + {points.map((p) => (
    - + {format ? format(p.key) : p.key} - {p.value.toLocaleString()} + + {p.value.toLocaleString()} +
    ))}

    @@ -154,7 +217,9 @@ export function StatChart({ const style: React.CSSProperties = { color: "var(--primary)" }; return (
    - {total.toLocaleString()} + + {total.toLocaleString()} + {/* A trend needs two points. One point (a single all-time bucket, say) would draw as a flat line that reads as "no change". */} {points.length >= 2 && ( diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx index 250dd39cd..9c369e100 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx @@ -130,6 +130,18 @@ function lastBucketDelta(trend: number[]): number | undefined { return (latest - previous) / Math.abs(previous); } +/** + * Whether this series has a single number to show. An empty result and a + * withheld total are both "no number"; an emitted zero is a number. + */ +function hasScalar(series: WidgetSeries): boolean { + return ( + series.total !== null && + series.points.length > 0 && + series.coverage !== "empty" + ); +} + /** A resolved scalar widget as the `Metric` a tile binds to. */ function widgetMetric( widget: DashboardWidgetView, @@ -140,10 +152,12 @@ function widgetMetric( return { id: widget.id, label: widgetTitle(widget), - // A withheld total is passed through as null, so the tile shows a dash - // and its badge says what withheld it. Coercing it to 0 here would - // report "nothing happened" where the pipeline said "cannot say". - value: series.total, + // Whether a number exists is read off the SERIES, never off the badge. + // The badge is one slot and freshness can take it (a failed refresh + // says "Stale"), so deriving the value's presence from the state made + // an empty series render its 0 the moment it also went stale. The two + // are different questions about the same tile and are answered apart. + value: hasScalar(series) ? series.total : null, format: widget.format, unit: widget.unit, higherIsBetter: widget.higherIsBetter, @@ -258,37 +272,62 @@ export function DashboardWidget({ actions, className, }: DashboardWidgetProps) { - const { series } = widget; const title = widgetTitle(widget); - // Three states, not two. A failure with nothing behind it is a failure; a - // failure with a series still in hand is a refresh that did not land, and - // blanking the tile would throw away values that are still the best - // available. `stale` is the kit's own word for that, and it outranks the - // coverage badge because it is the more actionable of the two. - const lost = widget.failed === true && series === null; - const stale = widget.failed === true && series !== null; + // A refusal is answered BEFORE the series is looked at, in every branch + // below. The host classifies its own transport and is expected to withhold + // the values too, but a number this viewer may not see is a wrong answer + // rather than an old one, so the renderer that owns what a refusal looks + // like fails closed as well: a caller that forwards retained values by + // mistake cannot get them drawn through this. Testing the series first + // here — the ordering that reads more naturally — is exactly what would + // put them back on screen. + const refused = widget.failure === "refused"; + const { series } = widget; + // Otherwise three outcomes, not two. A failure with nothing behind it is a + // failure; a failure with a series still in hand is a refresh that did not + // land, and blanking the tile would throw away values that are still the + // best available. `stale` is the kit's own word for that. + const lost = widget.failure === "failed" && series === null; + const stale = widget.failure === "failed" && series !== null; if (widget.visualization === "number") { - const metric: Metric = lost - ? { - id: widget.id, - label: title, - value: null, - state: "provider_unavailable", - } - : series === null - ? { id: widget.id, label: title, value: null, state: "loading" } - : { ...widgetMetric(widget, series), ...(stale && { state: "stale" }) }; - return ; + const metric: Metric = refused + ? { id: widget.id, label: title, value: null, state: "forbidden" } + : lost + ? { + id: widget.id, + label: title, + value: null, + state: "provider_unavailable", + } + : series === null + ? { id: widget.id, label: title, value: null, state: "loading" } + : { + ...widgetMetric(widget, series), + ...(stale && { state: "stale" }), + }; + return ( + + ); } // A chart or table card carries its coverage badge in the card's own // description slot — the line under the title that says what this card is — // so a reader meets "Partial data" where they meet the title, as they do on // a tile, instead of as a sentence pushed above the geometry. - const state: MetricState | undefined = - series === null ? undefined : stale ? "stale" : seriesState(series); + const state: MetricState | undefined = refused + ? "forbidden" + : series === null + ? undefined + : stale + ? "stale" + : seriesState(series); return ( - {lost ? ( + {refused ? ( +

    + You don't have permission to see this. +

    + ) : lost ? (

    Unable to load.

    ) : series === null ? ( @@ -440,8 +483,19 @@ export function Dashboard({ slots?: DashboardSlots; }) { const columns = data.columns ?? 2; + // The accent overrides BOTH tokens the marks paint from, because the tier + // change moved where the colour comes from: the metric charts colour a + // series from the categorical palette (`chartSeriesColor(0)` is + // `--chart-1`), while `BarList` and the sparkline still use `--primary`. + // Setting only `--primary`, as the basic tier needed, left a declared + // accent with no effect on the charts it exists to colour. A dashboard's + // accent is its FIRST series' colour; a chart with more keeps the rest of + // the palette so two series never collapse to one hue. const style = data.accent - ? ({ "--primary": data.accent } as React.CSSProperties) + ? ({ + "--primary": data.accent, + "--chart-1": data.accent, + } as React.CSSProperties) : undefined; const isGrid = (data.layout ?? "grid") === "grid"; diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/format.ts b/module/services/frontend/code/packages/codefly-ui/src/dashboard/format.ts index 3b350a6f3..08f5d6e05 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/format.ts +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/format.ts @@ -7,7 +7,10 @@ // thousands. Bounding *significant* digits (not fraction digits) is what keeps a // small fractional metric legible: `avg`/`ratio`/`percentile` series can tick in // the hundredths, and a fraction-digit cap would round every such tick to "0". -const valueFormat = new Intl.NumberFormat(undefined, { notation: "compact", maximumSignificantDigits: 3 }); +const valueFormat = new Intl.NumberFormat(undefined, { + notation: "compact", + maximumSignificantDigits: 3, +}); export function formatAxisValue(value: number): string { return valueFormat.format(value); @@ -16,7 +19,8 @@ export function formatAxisValue(value: number): string { // Series keys that are time buckets arrive as ISO timestamps from the audit RPC // (`YYYY-MM-DDThh:mm:ss±hh`); anything else is a plain category key. Gate on the // ISO shape so a non-date key ("US East", "42") is never coerced through Date. -const ISO_TIME = /^\d{4}-\d{2}-\d{2}([T ]\d{2}:\d{2}(:\d{2})?)?([+-]\d{2}(:?\d{2})?|Z)?$/; +const ISO_TIME = + /^\d{4}-\d{2}-\d{2}([T ]\d{2}:\d{2}(:\d{2})?)?([+-]\d{2}(:?\d{2})?|Z)?$/; // Postgres' `OF` offset emits a bare-hour zone ("+00", "-05") that Date.parse // rejects; pad it to the "+00:00" the parser accepts. A "+0000" form gets its @@ -24,7 +28,10 @@ const ISO_TIME = /^\d{4}-\d{2}-\d{2}([T ]\d{2}:\d{2}(:\d{2})?)?([+-]\d{2}(:?\d{2 // date-only "2026-12-25" would be misread as an offset and mangled. function normalizeOffset(iso: string): string { if (!iso.includes(":")) return iso; - return iso.replace(/([+-]\d{2})(\d{2})?$/, (_m, hh: string, mm?: string) => `${hh}:${mm ?? "00"}`); + return iso.replace( + /([+-]\d{2})(\d{2})?$/, + (_m, hh: string, mm?: string) => `${hh}:${mm ?? "00"}`, + ); } // Time buckets are day/week/month grains, so a key at exact UTC midnight reads as diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/geometry.ts b/module/services/frontend/code/packages/codefly-ui/src/dashboard/geometry.ts index fa3c53510..f60292a9d 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/geometry.ts +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/geometry.ts @@ -29,14 +29,29 @@ export interface Plot { // The axis-less sparkline gutter: a hair of padding on every side so a // non-scaling stroke isn't clipped at the frame, matching the pre-axis charts. -export const SPARK_MARGIN: Margins = { top: PAD, right: PAD, bottom: PAD, left: PAD }; +export const SPARK_MARGIN: Margins = { + top: PAD, + right: PAD, + bottom: PAD, + left: PAD, +}; // Extra left/bottom gutter an axed chart needs so the y value labels and the x // time labels have room outside the plot; top/right stay tight. -export const AXIS_MARGIN: Margins = { top: PAD, right: PAD, bottom: 22, left: 34 }; +export const AXIS_MARGIN: Margins = { + top: PAD, + right: PAD, + bottom: 22, + left: 34, +}; export function plot(width: number, height: number, m: Margins): Plot { - return { left: m.left, right: width - m.right, top: m.top, bottom: height - m.bottom }; + return { + left: m.left, + right: width - m.right, + top: m.top, + bottom: height - m.bottom, + }; } /** The full-width sparkline plot at a given height. */ @@ -47,7 +62,12 @@ export function sparkPlot(height: number): Plot { // The Scale atom: a pure linear map from a domain onto a pixel range. A // zero-width domain (a flat series) collapses to the range start instead of // dividing by zero, so it draws one line rather than NaNs. -export function linearScale(d0: number, d1: number, r0: number, r1: number): (v: number) => number { +export function linearScale( + d0: number, + d1: number, + r0: number, + r1: number, +): (v: number) => number { const span = d1 - d0 || 1; return (v) => r0 + ((v - d0) / span) * (r1 - r0); } @@ -96,7 +116,8 @@ export function niceTicks(min: number, max: number, count = 4): number[] { const end = Math.ceil(max / step) * step; const ticks: number[] = []; // `+ step / 2` absorbs float drift so the final tick isn't dropped. - for (let v = start; v <= end + step / 2; v += step) ticks.push(Number(v.toFixed(6))); + for (let v = start; v <= end + step / 2; v += step) + ticks.push(Number(v.toFixed(6))); return ticks; } @@ -111,7 +132,10 @@ export function niceTicks(min: number, max: number, count = 4): number[] { export function linePath( points: SeriesPoint[], p: Plot, - y = scaleY(points.map((pt) => pt.value), p), + y = scaleY( + points.map((pt) => pt.value), + p, + ), singleY = (p.top + p.bottom) / 2, ): string { if (points.length === 0) return ""; @@ -120,7 +144,10 @@ export function linePath( return `M ${p.left.toFixed(1)} ${yy} L ${p.right.toFixed(1)} ${yy}`; } return points - .map((pt, i) => `${i === 0 ? "M" : "L"} ${scaleX(i, points.length, p).toFixed(1)} ${y(pt.value).toFixed(1)}`) + .map( + (pt, i) => + `${i === 0 ? "M" : "L"} ${scaleX(i, points.length, p).toFixed(1)} ${y(pt.value).toFixed(1)}`, + ) .join(" "); } @@ -130,12 +157,19 @@ export function linePath( export function areaPath( points: SeriesPoint[], p: Plot, - y = scaleY(points.map((pt) => pt.value), p), + y = scaleY( + points.map((pt) => pt.value), + p, + ), singleY = (p.top + p.bottom) / 2, ): string { if (points.length === 0) return ""; const baseline = p.bottom.toFixed(1); - const left = (points.length === 1 ? p.left : scaleX(0, points.length, p)).toFixed(1); - const right = (points.length === 1 ? p.right : scaleX(points.length - 1, points.length, p)).toFixed(1); + const left = ( + points.length === 1 ? p.left : scaleX(0, points.length, p) + ).toFixed(1); + const right = ( + points.length === 1 ? p.right : scaleX(points.length - 1, points.length, p) + ).toFixed(1); return `${linePath(points, p, y, singleY)} L ${right} ${baseline} L ${left} ${baseline} Z`; } diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/index.ts b/module/services/frontend/code/packages/codefly-ui/src/dashboard/index.ts index b0612c06f..193fd4475 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/index.ts +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/index.ts @@ -28,54 +28,50 @@ export { scaleX, scaleY, } from "./geometry.js"; -export { - type DashboardLayoutKind, - type DashboardView, - type DashboardWidgetView, - fromDashboardData, - type SeriesPoint, - type WidgetSeries, - type WidgetVisualization, -} from "./types.js"; - export { AreaChart as MetricAreaChart, - LineChart as MetricLineChart, BarChart as MetricBarChart, chartSeriesColor, + LineChart as MetricLineChart, type MetricChartProps, } from "./metric-chart.js"; export { + axisPositions, type ChartDatum, type ChartSeries, - type ResolvedSeries, type Point, - unionLabels, + type ResolvedSeries, resolveSeries, - stackSeries, - stackedExtent, type StackedSeries, + stackedExtent, + stackSeries, + unionLabels, valuesExtent, - axisPositions, } from "./metric-geometry.js"; - -export { Sparkline } from "./sparkline.js"; -export { SortableGrid, type SortableGridProps } from "./sortable-grid.js"; - export { - MetricProvenance, - MetricStateBadge, assertSampleModeAllowed, + MetricProvenance, type MetricState, + MetricStateBadge, } from "./metric-state.js"; - export { - MetricCard, - StatTile, - KPIRow, formatMetricValue, + KPIRow, type Metric, + MetricCard, type MetricFormat, + StatTile, } from "./metric-tiles.js"; - +export { SortableGrid, type SortableGridProps } from "./sortable-grid.js"; +export { Sparkline } from "./sparkline.js"; export { TrendLineChart } from "./trend-line-chart.js"; +export { + type DashboardLayoutKind, + type DashboardView, + type DashboardWidgetView, + fromDashboardData, + type SeriesPoint, + type WidgetReadFailure, + type WidgetSeries, + type WidgetVisualization, +} from "./types.js"; diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-state.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-state.tsx index bd77e15af..9d3943a7f 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-state.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-state.tsx @@ -8,6 +8,12 @@ export type MetricState = | "provider_unavailable" | "not_configured" | "sample" + // The read was refused: the viewer is not allowed to see this number. + // Distinct from every other state here, which are all about the data — + // this one is about authority, and it is the only one where showing a + // value that was fetched before would be a wrong answer rather than a + // stale one. + | "forbidden" // The series resolved, but no single number summarises it: a distinct // count, an average, a percentile or a ratio has no scalar total across // several groups, and summing or averaging those summaries would @@ -24,6 +30,7 @@ const stateLabels: Record, string> = { not_configured: "Not configured", sample: "Sample data", no_total: "No total", + forbidden: "No access", }; export function assertSampleModeAllowed( diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx index e3bb165cd..e02968b57 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx @@ -77,6 +77,7 @@ const valuelessStates: MetricState[] = [ "not_configured", "provider_unavailable", "no_total", + "forbidden", ]; export function formatMetricValue( @@ -247,12 +248,21 @@ function selectableCard(metric: Metric): { export function StatTile({ metric, actions, + labelAs = "span", className, }: { metric: Metric; actions?: ReactNode; + /** + * The element the label is drawn as. A tile standing in for a titled + * region — a dashboard widget — passes `"h3"` so the title stays in the + * document outline and reachable by heading navigation, which a card with + * a heading gives for free and a tile otherwise takes away. + */ + labelAs?: "span" | "h3"; className?: string; }) { + const Label = labelAs; const selectable = selectableCard(metric); return ( {metric.label} - + {metric.state && } {actions != null && (
    {actions}
    diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/types.ts b/module/services/frontend/code/packages/codefly-ui/src/dashboard/types.ts index 05512ecb6..9d1b1c7ec 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/types.ts +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/types.ts @@ -10,6 +10,13 @@ import type { MetricFormat } from "./metric-tiles.js"; +/** + * How a widget's read ended when it did not succeed. Presentational: a caller + * classifies its own transport (the host app maps its `ReadOutcome` onto this) + * and the renderer only decides what to draw. + */ +export type WidgetReadFailure = "failed" | "refused"; + /** How a widget draws its series. */ export type WidgetVisualization = "line" | "bar" | "area" | "number" | "table"; @@ -42,11 +49,19 @@ export interface DashboardWidgetView { */ series: WidgetSeries | null; /** - * The series could not be resolved — an error, not an empty result. A - * caller that resolves each widget separately sets it per widget, so one - * failure says so in its own card instead of blanking its siblings. + * How the read ended, when it did not succeed. A caller that resolves each + * widget separately sets it per widget, so one failure says so in its own + * card instead of blanking its siblings. + * + * The two are not a severity ordering, they are different questions. + * `"failed"` says the read did not complete and says nothing about + * authority, so a series already in hand is still the best answer and is + * drawn as stale. `"refused"` says this viewer is not allowed to see the + * number, which makes a previously fetched one a WRONG answer rather than + * an old one — so the renderer discards it, whatever the caller passed. + * A boolean cannot express that difference, which is why this is not one. */ - failed?: boolean; + failure?: WidgetReadFailure; /** Grid columns this widget spans (1–4); ignored in a stack. */ span?: 1 | 2 | 3 | 4; /** diff --git a/module/services/frontend/code/src/components/__tests__/kit.test.ts b/module/services/frontend/code/src/components/__tests__/kit.test.ts index 5e968d620..16125b251 100644 --- a/module/services/frontend/code/src/components/__tests__/kit.test.ts +++ b/module/services/frontend/code/src/components/__tests__/kit.test.ts @@ -1,6 +1,7 @@ import { readdirSync, readFileSync } from "node:fs"; import { dirname, join, relative } from "node:path"; import { fileURLToPath } from "node:url"; +import ts from "typescript"; import { describe, expect, it } from "vitest"; import packageJson from "../../../package.json" with { type: "json" }; @@ -176,126 +177,185 @@ const KIT_SURFACES = [ const KIT_SPECIFIER = /^@codefly-dev\/(?:ui|saas-ui)(?:\/|$)/; /** - * The named bindings an `import`/`export … from` clause carries, as - * published-name → exporter's-own-name. The two differ under an alias, and - * which one a check wants is the whole question: a wrapper is recognised by the - * exporter's name, so renaming the alias cannot turn a re-implementation into - * a wrapper. + * Whether a module specifier names one of the host's kit surfaces, by alias + * (`@/shared/ui`) or by a relative path that lands on the same file. Both + * spellings reach the same module, so judging only the alias rejected a real + * wrapper for how it spelled its import. */ -function namedBindings( - source: string, - isWanted: (specifier: string) => boolean, +function isHostSurface(specifier: string, fromPath: string): boolean { + const resolved = specifier.startsWith("@/") + ? `src/${specifier.slice(2)}` + : specifier.startsWith(".") + ? join(dirname(fromPath), specifier).replaceAll("\\", "/") + : null; + if (resolved === null) return false; + return KIT_SURFACES.some( + (surface) => surface === resolved || surface.startsWith(`${resolved}/`), + ); +} + +/** + * The kit components a file has a binding for, as local-name → kit-name, with + * a namespace import recorded as `*`. + * + * Resolved from the AST rather than by regex, because a declaration's own body + * has to be checked against these bindings: file-wide import presence proved + * nothing. An unrelated `export { Card as KitCard } from "@codefly-dev/ui"` + * was enough to exempt a hand-rolled `Card` sitting beside it, which is the + * shape a duplicate actually arrives in — somebody re-exports the kit's thing + * for a neighbour and writes their own in the same file. + */ +function kitBindings( + file: ts.SourceFile, + path: string, + surface: Map, ): Map { const bindings = new Map(); - const clause = - /(?:import|export)\s+(?:type\s+)?\{([^}]*)\}\s*from\s*["']([^"']+)["']/g; - for (const block of source.matchAll(clause)) { - if (!isWanted(block[2])) continue; - for (const raw of block[1].split(",")) { - const parts = raw - .trim() - .replace(/^type\s+/, "") - .split(/\s+as\s+/) - .map((part) => part.trim()); - const exported = parts[0]; - if (exported) bindings.set(parts[1] ?? exported, exported); + for (const statement of file.statements) { + if (!ts.isImportDeclaration(statement)) continue; + if (!ts.isStringLiteral(statement.moduleSpecifier)) continue; + const specifier = statement.moduleSpecifier.text; + const kit = KIT_SPECIFIER.test(specifier); + const host = isHostSurface(specifier, path); + if (!kit && !host) continue; + const clause = statement.importClause; + if (!clause) continue; + // `import * as UI from "@/shared/ui"` — every member access on it is a + // kit component, so the namespace itself is the binding. + if (clause.namedBindings && ts.isNamespaceImport(clause.namedBindings)) { + bindings.set(clause.namedBindings.name.text, "*"); + continue; + } + if (!clause.namedBindings || !ts.isNamedImports(clause.namedBindings)) { + continue; + } + for (const element of clause.namedBindings.elements) { + const published = (element.propertyName ?? element.name).text; + // Through a host surface the published name is that file's, so it is + // resolved back to the kit's own: `src/shared/ui` re-exports + // `CardRoot` as `Card`, so importing `Card` from there composes + // `CardRoot`, and a declaration named `Card` beside it is still a + // second `Card`. + const kitName = kit ? published : surface.get(published); + if (kitName) bindings.set(element.name.text, kitName); } } return bindings; } /** - * The names a file imports FROM the kit, under the kit's own spelling: the `X` - * of `import { X as KitX } from "@codefly-dev/ui/layout"`. A wrapper is - * recognised by this, not by its alias, so renaming the alias cannot turn a - * re-implementation into a wrapper. + * Every component this file declares, by the name it is PUBLISHED under, with + * the declaration's own node. * - * An import through a host surface counts too, under the name THAT surface - * publishes — which is not always the kit's: `src/shared/ui` re-exports - * `CardRoot` as `Card`, so importing `Card` from there composes `CardRoot` and - * a declaration named `Card` beside it is still a second `Card`, exactly as if - * it had imported `CardRoot` directly. + * The published name is what matters, and it is not always the declared one: + * `const LocalPanel = …; export { LocalPanel as Card }` ships a `Card`, and + * reading only declaration names let that straight through. */ -function kitImports(source: string, surface: Map): Set { - const composed = new Set(); - for (const kitName of namedBindings(source, (specifier) => - KIT_SPECIFIER.test(specifier), - ).values()) { - composed.add(kitName); +function declarations(file: ts.SourceFile): Map { + const declared = new Map(); + const published = new Map(); + const visit = (node: ts.Node) => { + if (ts.isFunctionDeclaration(node) && node.name) { + declared.set(node.name.text, node); + } else if (ts.isClassDeclaration(node) && node.name) { + declared.set(node.name.text, node); + } else if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name)) { + declared.set(node.name.text, node); + } + ts.forEachChild(node, visit); + }; + visit(file); + // `export { Local as Published }` with no `from`: a local declaration + // published under another name. + for (const statement of file.statements) { + if (!ts.isExportDeclaration(statement) || statement.moduleSpecifier) { + continue; + } + const clause = statement.exportClause; + if (!clause || !ts.isNamedExports(clause)) continue; + for (const element of clause.elements) { + const local = (element.propertyName ?? element.name).text; + const node = declared.get(local); + if (node) published.set(element.name.text, node); + } } - // Through a host surface, the name imported is the one THAT file publishes, - // so it is resolved back to the kit's own. `src/shared/ui` re-exports - // `CardRoot` as `Card`, so importing `Card` from there composes `CardRoot` - // — and a declaration named `Card` beside it is still a second `Card`, - // exactly as if it had imported `CardRoot` directly. Resolving the alias is - // what keeps those two spellings of one import judged the same way. - for (const published of namedBindings(source, (specifier) => - HOST_SURFACE_SPECIFIER.test(specifier), - ).values()) { - const kitName = surface.get(published); - if (kitName) composed.add(kitName); + const all = new Map(); + for (const [name, node] of declared) { + if (/^[A-Z][A-Za-z0-9]*$/.test(name)) all.set(name, node); } - return composed; + for (const [name, node] of published) { + if (/^[A-Z][A-Za-z0-9]*$/.test(name)) all.set(name, node); + } + return all; } -const HOST_SURFACE_SPECIFIER = - /^@\/(?:shared\/ui|components\/charts|components\/sparkline|components\/metric-state)(?:\/|$)/; - -/** - * A component declaration, exported or module-local, at any indentation — a - * duplicate nested inside another component is still a duplicate, and the - * commonest hiding place for one. - */ -const DECLARATION = - /^[ \t]*(?:export\s+)?(?:default\s+)?(?:async\s+)?(?:function|const|let|class)\s+([A-Z][A-Za-z0-9]*)/gm; +/** Whether this declaration's own body renders the kit component `kitName`. */ +function composesWithin( + node: ts.Node, + kitName: string, + bindings: Map, +): boolean { + let found = false; + const visit = (child: ts.Node) => { + if (found) return; + if (ts.isIdentifier(child) && bindings.get(child.text) === kitName) { + found = true; + return; + } + // `UI.Badge` off a namespace import. + if ( + ts.isPropertyAccessExpression(child) && + ts.isIdentifier(child.expression) && + bindings.get(child.expression.text) === "*" && + child.name.text === kitName + ) { + found = true; + return; + } + ts.forEachChild(child, visit); + }; + ts.forEachChild(node, visit); + return found; +} /** - * Files whose default export's NAME is fixed by the framework, not chosen: - * Next's route conventions make every route component a `Page`, every nested - * shell a `Layout`, and so on, and the kit exports primitives called `Page` and - * `Layout`. A route component is not a second copy of either — it is the thing - * that mounts them — so its default export is not read as a declaration taking - * the kit's name. A NAMED declaration in such a file still is. + * The kit's dashboard marks. A host surface may compose ONE of these — a page + * drawing a chart — but a file that reaches for several is dispatching over a + * widget's visualization, which is ``'s whole job. + * + * This is the leg that catches a duplicate under a NEW name, which no + * name-based rule can: the renderer #1041 deleted declared `WidgetBody`, + * `WidgetValues` and `WidgetCard` — not one of them a name the kit exports — + * and so produced zero offences from the legs above while being the exact + * duplication this file exists to prevent. */ -const ROUTE_CONVENTION = - /^src\/app\/.*\/(page|layout|template|default|error|global-error|loading|not-found|forbidden|unauthorized)\.tsx?$/; -const DEFAULT_EXPORT = /^[ \t]*export\s+default\s/; - -interface Offence { - name: string; - path: string; -} +const DASHBOARD_MARKS = new Set([ + "LineChart", + "AreaChart", + "BarList", + "StatChart", + "TrendLineChart", + "MetricLineChart", + "MetricAreaChart", + "MetricBarChart", + "StatTile", + "MetricCard", + "KPIRow", + "Sparkline", +]); /** - * Declarations in these files that take a kit export's name without composing - * the kit's component of that name. Pure, so the fixtures below can exercise it - * on sources that are not in the tree. + * Files that draw with more than one of the kit's marks and may keep doing so, + * with what each is and what would retire it. They are the host's older + * dashboard renderers, the same ones {@link ALLOWED} names; this list exists so + * a NEW one cannot join them quietly. */ -function findOffences( - files: { path: string; source: string }[], - kit: Set, - allowed: Record = ALLOWED, - /** Published name → kit name for the host's own surfaces; see {@link KIT_SURFACES}. */ - surface: Map = new Map(), -): Offence[] { - const offences: Offence[] = []; - for (const { path, source } of files) { - if (KIT_SURFACES.includes(path)) continue; - const composed = kitImports(source, surface); - const routeFile = ROUTE_CONVENTION.test(path); - const seen = new Set(); - for (const declaration of source.matchAll(DECLARATION)) { - const name = declaration[1]; - if (!kit.has(name) || seen.has(name)) continue; - if (routeFile && DEFAULT_EXPORT.test(declaration[0])) continue; - seen.add(name); - if (composed.has(name)) continue; - if (`${name} in ${path}` in allowed) continue; - offences.push({ name, path }); - } - } - return offences; -} +const DISPATCHERS: Record = { + "src/components/dashboard.tsx": + "the widget-slot renderer: a union of host-supplied widgets, each with its own loading/error/empty state. Goes when the audit page's widgets become a DashboardView and it mounts the kit's renderer.", + "src/features/dashboard/ui/metric-card.tsx": + "the dashboard DSL's card: it resolves one declared metric through useMetric and then draws whichever chart the declaration asked for. Goes with the DSL resolving outside the card.", +}; function appSources(): { path: string; source: string }[] { return walk(join(appRoot, "src")) @@ -327,16 +387,131 @@ function kitNames(): Set { function hostSurface(): Map { const surface = new Map(); for (const path of KIT_SURFACES) { - const source = readFileSync(join(appRoot, path), "utf8"); - for (const [published, kitName] of namedBindings(source, (specifier) => - KIT_SPECIFIER.test(specifier), - )) { - surface.set(published, kitName); + const file = parse(path, readFileSync(join(appRoot, path), "utf8")); + for (const statement of file.statements) { + if ( + !ts.isExportDeclaration(statement) && + !ts.isImportDeclaration(statement) + ) { + continue; + } + const specifier = statement.moduleSpecifier; + if (!specifier || !ts.isStringLiteral(specifier)) continue; + if (!KIT_SPECIFIER.test(specifier.text)) continue; + const clause = ts.isExportDeclaration(statement) + ? statement.exportClause + : statement.importClause?.namedBindings; + if (!clause) continue; + if (ts.isNamedExports(clause) || ts.isNamedImports(clause)) { + for (const element of clause.elements) { + surface.set( + element.name.text, + (element.propertyName ?? element.name).text, + ); + } + } } } return surface; } +function parse(path: string, source: string): ts.SourceFile { + return ts.createSourceFile( + path, + source, + ts.ScriptTarget.Latest, + true, + ts.ScriptKind.TSX, + ); +} + +interface Offence { + name: string; + path: string; + /** Why it is an offence: a name the kit owns, or a widget dispatch. */ + kind: "name" | "dispatch"; +} + +/** + * Files whose default export's NAME is fixed by the framework, not chosen: + * Next's route conventions make every route component a `Page`, every nested + * shell a `Layout`, and so on, and the kit exports primitives called `Page` and + * `Layout`. A route component is not a second copy of either — it is the thing + * that mounts them — so its default export is not read as a declaration taking + * the kit's name. A NAMED declaration in such a file still is. + * + * The root route (`src/app/page.tsx`) counts: it is as conventional as a nested + * one, and requiring a directory between `app/` and `page.tsx` rejected it. + */ +const ROUTE_CONVENTION = + /^src\/app\/(?:.*\/)?(page|layout|template|default|error|global-error|loading|not-found|forbidden|unauthorized)\.tsx?$/; + +/** The names a file's default export declares, to exempt a route component. */ +function defaultExportNames(file: ts.SourceFile): Set { + const names = new Set(); + for (const statement of file.statements) { + const isDefault = ts + .getModifiers(statement as ts.HasModifiers) + ?.some((modifier) => modifier.kind === ts.SyntaxKind.DefaultKeyword); + if (!isDefault) continue; + if ( + (ts.isFunctionDeclaration(statement) || + ts.isClassDeclaration(statement)) && + statement.name + ) { + names.add(statement.name.text); + } + } + return names; +} + +/** + * Declarations that take a kit export's name without composing the kit's + * component of that name WITHIN their own body, plus files that dispatch over + * more than one of the kit's dashboard marks. Pure, so the fixtures below can + * exercise it on sources that are not in the tree. + */ +function findOffences( + files: { path: string; source: string }[], + kit: Set, + allowed: Record = ALLOWED, + /** Published name → kit name for the host's own surfaces. */ + surface: Map = new Map(), + dispatchers: Record = DISPATCHERS, +): Offence[] { + const offences: Offence[] = []; + for (const { path, source } of files) { + if (KIT_SURFACES.includes(path)) continue; + const file = parse(path, source); + const bindings = kitBindings(file, path, surface); + const routeDefaults = ROUTE_CONVENTION.test(path) + ? defaultExportNames(file) + : new Set(); + + for (const [name, node] of declarations(file)) { + if (!kit.has(name) || routeDefaults.has(name)) continue; + if (composesWithin(node, name, bindings)) continue; + if (`${name} in ${path}` in allowed) continue; + offences.push({ name, path, kind: "name" }); + } + + if (!(path in dispatchers)) { + const marks = new Set(); + for (const kitName of bindings.values()) { + if (DASHBOARD_MARKS.has(kitName)) marks.add(kitName); + } + if (marks.size > 1) { + offences.push({ + name: [...marks].sort().join("+"), + path, + kind: "dispatch", + }); + } + } + } + return offences; +} + describe("the host app composes the kit rather than re-implementing it", () => { it("declares no component the kit already exports", () => { const kit = kitNames(); @@ -350,9 +525,10 @@ describe("the host app composes the kit rather than re-implementing it", () => { kit, ALLOWED, hostSurface(), - ).map( - ({ name, path }) => - `${name} in ${path} — @codefly-dev/ui already exports it. Compose the kit's component, or add "${name} in ${path}" to ALLOWED with what the two things are and why neither can stand in for the other.`, + ).map(({ name, path, kind }) => + kind === "name" + ? `${name} in ${path} — @codefly-dev/ui already exports it. Compose the kit's component, or add "${name} in ${path}" to ALLOWED with what the two things are and why neither can stand in for the other.` + : `${path} draws with ${name} — more than one of the kit's dashboard marks, which is a widget dispatch and 's job. Mount or , or add ${path} to DISPATCHERS with what it is and what would retire it.`, ); expect(offences).toEqual([]); }); @@ -425,7 +601,7 @@ describe("the detector", () => { ["Card", "CardRoot"], ]); const detect = (path: string, source: string) => - findOffences([{ path, source }], kit, {}, surface).map( + findOffences([{ path, source }], kit, {}, surface, {}).map( (o) => `${o.name} in ${o.path}`, ); @@ -575,6 +751,94 @@ describe("the detector", () => { ).toEqual([]); }); + it("catches a same-name copy sitting beside an unrelated kit re-export", () => { + // The shape a duplicate actually arrives in: somebody re-exports the + // kit's component for a neighbour and writes their own beside it. A + // file-wide import check read that re-export as proof of composition + // and exempted the copy. + expect( + detect( + "src/features/x/ui/card.tsx", + `export { Card as KitCard } from "@codefly-dev/ui/layout"; +export const Card = () =>
    copy
    ;`, + ), + ).toEqual(["Card in src/features/x/ui/card.tsx"]); + }); + + it("catches an implementation published under a kit name by an export alias", () => { + // The declared name is not the shipped name. Reading only declarations + // let this straight through. + expect( + detect( + "src/features/x/ui/panel.tsx", + `const LocalPanel = () =>
    copy
    ; +export { LocalPanel as Card };`, + ), + ).toEqual(["Card in src/features/x/ui/panel.tsx"]); + }); + + it("allows a wrapper that reaches the kit through a namespace import", () => { + expect( + detect( + "src/features/x/ui/badge.tsx", + `import * as UI from "@/shared/ui"; +export const Badge = (props) => ;`, + ), + ).toEqual([]); + }); + + it("allows a wrapper that reaches a host surface by relative path", () => { + // `@/shared/ui` and `../../../shared/ui` are the same module; judging + // only the alias rejected a real wrapper for how it spelled its import. + expect( + detect( + "src/features/x/ui/badge.tsx", + `import { Badge as KitBadge } from "../../../shared/ui"; +export const Badge = (props) => ;`, + ), + ).toEqual([]); + }); + + it("applies the Next route exception to the ROOT route, not only nested ones", () => { + expect( + detect( + "src/app/page.tsx", + "export default function Page() { return
    ; }", + ), + ).toEqual([]); + }); + + it("rejects the exact renderer this gate was written to prevent", async () => { + // The case that shows the name legs are not enough on their own: the + // renderer #1041 deleted declared WidgetBody, WidgetValues and + // WidgetCard — not one of them a name the kit exports — so every + // name-based leg passed it while it was the duplication itself. The + // dispatch leg is what catches it, and this reads the real deleted + // source rather than a paraphrase of it. + const { execFileSync } = await import("node:child_process"); + const deleted = execFileSync( + "git", + [ + "show", + "8590d329:module/services/frontend/code/src/solutions/SolutionDashboard.tsx", + ], + { encoding: "utf8", cwd: appRoot }, + ); + expect( + detect("src/solutions/SolutionDashboard.tsx", deleted).length, + ).toBeGreaterThan(0); + }); + + it("lets a page draw ONE kit mark, which is composition and not a dispatch", () => { + expect( + detect( + "src/features/x/ui/page.tsx", + `import { MetricLineChart } from "@codefly-dev/ui/dashboard"; +export const Trend = () => ;`, + ), + ).toEqual([]); + }); + it("ignores a name the kit does not export", () => { expect( detect( diff --git a/module/services/frontend/code/src/solutions/SolutionDashboard.tsx b/module/services/frontend/code/src/solutions/SolutionDashboard.tsx index 28d86015e..131557e92 100644 --- a/module/services/frontend/code/src/solutions/SolutionDashboard.tsx +++ b/module/services/frontend/code/src/solutions/SolutionDashboard.tsx @@ -18,6 +18,7 @@ import { DashboardWidget, type DashboardWidgetView, SortableGrid, + type WidgetReadFailure, } from "@codefly-dev/ui/dashboard"; import { useQuery } from "@tanstack/react-query"; import { GripVertical, Info, Plus, X } from "lucide-react"; @@ -31,6 +32,11 @@ import { import { scopedDashboardDraftKey } from "@/features/dashboard/service/use-dashboard-authoring"; import { useAuth } from "@/lib/auth"; import { apiTransport } from "@/lib/connect/transport"; +import { + mayKeepRetainedRows, + type ReadOutcome, + readOutcome, +} from "@/shared/lib/read-outcome"; import { Button, DropdownMenu, @@ -120,7 +126,7 @@ function SolutionWidget({ // declaration refetches instead of serving another graph's cached series that // happens to share the same solution/dashboard/widget/org ids. Disabled until // the org resolves so the pre-org window reads as loading, never as empty. - const { data, isError } = useQuery({ + const { data, isError, error } = useQuery({ queryKey: [ "solution-widget", solutionId, @@ -139,18 +145,36 @@ function SolutionWidget({ enabled: orgId !== "", }); + // A refused read does not get to keep the numbers it fetched before. + // TanStack Query hands back the last successful data when a refetch + // rejects, so forwarding it on every error would leave a viewer whose + // audit-read authority was revoked still looking at the counts — the same + // wrong answer `mayKeepRetainedRows` exists to make unwritable, arrived at + // from the dashboard instead of from a table. A transient failure says + // nothing about authority, so those values stay and read as stale. + const outcome = readOutcome(isError, error); + const retained = mayKeepRetainedRows(outcome) ? (data?.series ?? null) : null; + return ( ); } +// The host's read classification as the renderer's presentational one. A read +// that succeeded is no failure at all, which is what `undefined` says. +const WIDGET_FAILURE: Record = { + empty: undefined, + forbidden: "refused", + failed: "failed", +}; + // A declared widget as the renderer's view model, with no series yet. The // series is bound by `SolutionWidget`, the one place that has resolved it; a // `null` series is the honest value here and the renderer draws it as a wait diff --git a/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx b/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx index 941dd350d..4c1010f61 100644 --- a/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx +++ b/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx @@ -1,5 +1,12 @@ import type { DataGraph } from "@codefly/saas-plugin-manifest"; -import { cleanup, fireEvent, screen, within } from "@testing-library/react"; +import { Code, ConnectError } from "@connectrpc/connect"; +import { + act, + cleanup, + fireEvent, + screen, + within, +} from "@testing-library/react"; import { HttpResponse, http } from "msw"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { renderInApp, rpc } from "@/test/container"; @@ -210,6 +217,54 @@ describe("SolutionDashboards", () => { expect(await screen.findByText("Unable to load.")).toBeTruthy(); }); + // The sequence that matters: a read that SUCCEEDED, then one that did not. + // TanStack Query hands back the last successful data when a refetch + // rejects, so a cold failure cannot show this — there is nothing retained + // to leak. + describe.each([ + [403, "permission_denied", Code.PermissionDenied], + [503, "unavailable", Code.Unavailable], + ] as const)("a %d on a refetch", (status, code, expected) => { + const refused = status === 403; + + it(`${refused ? "discards" : "keeps"} the values already on screen`, async () => { + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ buckets: [{ key: "2026-09-01", count: "37" }] }), + ), + ); + const { client } = renderDashboards(); + expect(await screen.findAllByText("37")).not.toHaveLength(0); + + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ code, message: "read rejected" }, { status }), + ), + ); + await act(async () => { + await client.refetchQueries({ queryKey: ["solution-widget"] }); + }); + + const query = client + .getQueryCache() + .find({ queryKey: ["solution-widget"], exact: false }); + expect(ConnectError.from(query?.state.error).code).toBe(expected); + + if (refused) { + // A number this viewer is no longer allowed to read is a wrong + // answer, not a stale one. + expect(await screen.findAllByText("No access")).not.toHaveLength(0); + expect(screen.queryAllByText("37")).toHaveLength(0); + expect(screen.queryByText("Stale")).toBeNull(); + } else { + // A transient failure says nothing about authority, so the + // values stay and read as out of date. + expect(await screen.findAllByText("Stale")).not.toHaveLength(0); + expect(screen.getAllByText("37").length).toBeGreaterThan(0); + } + }); + }); + it("reads as loading until the org context resolves, never as empty", () => { authState.organizationId = undefined; From 7c0aa0be28d4dc706921156cb2f14b010f58d0ca Mon Sep 17 00:00:00 2001 From: Codefly Automation <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 11 Oct 2026 10:45:21 -0400 Subject: [PATCH 4/6] fix(dashboard): a refusal latches, and the gate resolves what a name refers to (#1041) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The re-review of the previous commit found one blocker, three majors and one minor. Each is fixed at its cause, and each mechanism is mutation-checked separately: the change that re-creates the defect is shown to red a test. **BLOCKER — refused values came back on the next ordinary failure.** Hiding the retained snapshot for the refusal itself was not enough, and that was the bug: the snapshot outlived the refusal, so the NEXT error made it eligible again and handed the refused values straight back, with no authorized read in between. `200 → 403 → 503` and `200 → 403 → dropped connection` both restored the counts after "No access". A refusal now latches: the values stay ineligible until a successful read replaces them. The latch lives in the query cache beside the snapshot it guards, for two reasons — it outlives this component, where a ref or a piece of state forgets on remount and the snapshot does not, and it expires with the cache that holds the values, so neither can outlast the other. Ten regressions, five visualizations times two failure modes, plus the control where an authorized read lifts it. The first version of that test was green for the wrong reason: a rejected refetch reaches the component a tick after `refetchQueries` resolves, so asserting immediately passed with the latch deleted. The wait is now a named helper that says why it is there, and the mutation fails exactly the ten cases. **MAJOR — the gate still read references as composition.** It resolved bindings but not what they refer to. Only rendered JSX counts now, so a parameter typed `ComponentProps` no longer exempts a `Card` whose body is a bare `
    `. Namespace imports resolve through their members, including a host surface's, where `UI.Card` is the kit's `CardRoot`. Local aliases are chased to a fixed point. Names the declaration binds itself shadow the import. All three default-export forms resolve, so `export default Page` and `export { Page as default }` are the conventional route components they look like. An aliasing cross-file re-export is caught, while a same-name barrel re-export is not: that declaration was judged at its own site, and flagging it again would demand a second exception for one component. The dispatch leg now tests for BRANCHING rather than counting marks per file. Counting rejected a page that composed a KPI row above a chart, and was escaped by a namespace import or a local alias. A surface may draw several marks; what it may not do is choose between them by a widget's kind, which is the dispatch `` owns. The deleted renderer is still caught, and now also when its imports are rewritten to a namespace or to local aliases. **MAJOR — the suite required this repository's history.** The deleted-renderer fixture ran `git show`, which fails wherever the module is composed rather than cloned: a consuming repository carries the source tree, not these commits, and full-history CI is structurally unable to see that. The renderer is committed as a byte fixture instead, with a README on why it is `.txt` — as `.tsx` it would be typechecked against today's imports and counted by `ui-inventory.json`. **MAJOR — `forbidden` withheld the value but not what was derived from it.** `StatTile` and `MetricCard` drew the supplied delta and trend beside "No access", publishing the same figure twice over in the two forms hardest to notice. Everything value-derived now goes with the value. The renderer avoided this by building a fresh metric; a direct consumer of the exported primitives did not, which is where the regressions sit. **MINOR — the accent still missed scalar trends.** A sparkline strokes `currentColor` under a muted utility, so neither override reached it and one dashboard was drawn in two palettes. `StatTile` takes a `trend` option that colours it from the chart token, as a token rather than a utility class so a themed subtree's accent reaches it; `Sparkline` accepts a `style` for that. Refs #1041 Co-Authored-By: Claude Opus 5 (1M context) --- .../dashboard/__tests__/dashboard.test.tsx | 23 + .../codefly-ui/src/dashboard/dashboard.tsx | 1 + .../codefly-ui/src/dashboard/metric-tiles.tsx | 64 +- .../codefly-ui/src/dashboard/sparkline.tsx | 10 + .../components/__tests__/fixtures/README.md | 17 + .../fixtures/deleted-solution-dashboard.txt | 695 ++++++++++++++++++ .../code/src/components/__tests__/kit.test.ts | 601 ++++++++++++--- .../code/src/shared/ui/metric-tiles.test.tsx | 43 ++ .../code/src/solutions/SolutionDashboard.tsx | 50 +- .../__tests__/SolutionDashboard.test.tsx | 95 +++ 10 files changed, 1463 insertions(+), 136 deletions(-) create mode 100644 module/services/frontend/code/src/components/__tests__/fixtures/README.md create mode 100644 module/services/frontend/code/src/components/__tests__/fixtures/deleted-solution-dashboard.txt diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx index b929ff9b7..981a3be7b 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/__tests__/dashboard.test.tsx @@ -93,6 +93,29 @@ describe("Dashboard", () => { expect(section.style.getPropertyValue("--primary")).toBe("hotpink"); }); + it("reaches a scalar's own trend with the accent, not just the charts", () => { + // A declared accent that coloured the line charts but not the + // sparklines beside them drew one dashboard in two palettes. + const { container } = render( + , + ); + // The delta draws a lucide arrow, so the sparkline is not the first svg. + const [trend] = [...container.querySelectorAll("svg")].filter( + (svg) => !svg.classList.contains("lucide"), + ); + expect((trend as SVGElement).style.color).toBe("var(--chart-1)"); + expect( + ( + container.querySelector("section") as HTMLElement + ).style.getPropertyValue("--chart-1"), + ).toBe("hotpink"); + }); + it("keeps a titled scalar reachable by heading navigation", () => { // It was a Card with an

    ; a StatTile's label is a span, so the // title left the document outline when the tier changed. diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx index 9c369e100..91ede8d7e 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/dashboard.tsx @@ -312,6 +312,7 @@ export function DashboardWidget({ metric={metric} actions={actions} labelAs={widget.title != null ? "h3" : "span"} + trend="accent" className={className} /> ); diff --git a/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx b/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx index e02968b57..1de676fdb 100644 --- a/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx +++ b/module/services/frontend/code/packages/codefly-ui/src/dashboard/metric-tiles.tsx @@ -203,6 +203,20 @@ function hasTrend(series: number[] | undefined): series is number[] { return !!series && series.length > 1; } +/** + * Whether this metric's value may be derived from at all. + * + * `forbidden` is not a state of the data, it is a statement about the reader's + * authority, so everything computed FROM the value goes with the value: a tile + * that dashed its number and still drew "+170%" beside a trend line would have + * published the same figure twice over, in the two forms that are hardest to + * notice. Every other state here is about the data — an average legitimately + * has no total and a meaningful delta — so only this one withholds. + */ +function withholdsValue(metric: Metric): boolean { + return metric.state === "forbidden"; +} + // The card props that turn a metric into a control. Both StatTile and // MetricCard apply these, because `onSelect` lives on the Metric rather than on // one component's props: a renderer that ignored it would silently drop the @@ -245,10 +259,18 @@ function selectableCard(metric: Metric): { * owns the tile as well as the number — a dashboard whose viewer can drag, * remove or interrogate each tile. Without it the tile is purely presentational. */ +/** The colour a dashboard's accent reaches the first chart series through. */ +const ACCENT_TREND = "var(--chart-1)"; + +function trendTone(trend: "muted" | "accent"): string | undefined { + return trend === "muted" ? "text-muted-foreground/70" : undefined; +} + export function StatTile({ metric, actions, labelAs = "span", + trend = "muted", className, }: { metric: Metric; @@ -260,6 +282,14 @@ export function StatTile({ * a heading gives for free and a tile otherwise takes away. */ labelAs?: "span" | "h3"; + /** + * How the trend sparkline is coloured. `"muted"` is a tile reading beside + * other content; `"accent"` takes the dashboard's own accent, which a + * scalar widget wants for the same reason its charts do — a declared + * accent that coloured the line charts and not the sparklines beside them + * left one dashboard drawn in two palettes. + */ + trend?: "muted" | "accent"; className?: string; }) { const Label = labelAs; @@ -288,16 +318,22 @@ export function StatTile({
    - + {!withholdsValue(metric) && ( + + )}
    - {hasTrend(metric.series) && ( + {!withholdsValue(metric) && hasTrend(metric.series) && ( )}
    @@ -332,13 +368,15 @@ export function MetricCard({
    - + {!withholdsValue(metric) && ( + + )}
    - {hasTrend(metric.series) && ( + {!withholdsValue(metric) && hasTrend(metric.series) && ( ); @@ -81,6 +90,7 @@ export function Sparkline({ height={height} viewBox={`0 0 ${width} ${height}`} className={className} + style={style} aria-hidden > diff --git a/module/services/frontend/code/src/components/__tests__/fixtures/README.md b/module/services/frontend/code/src/components/__tests__/fixtures/README.md new file mode 100644 index 000000000..ddb89f971 --- /dev/null +++ b/module/services/frontend/code/src/components/__tests__/fixtures/README.md @@ -0,0 +1,17 @@ +# Fixtures + +`deleted-solution-dashboard.txt` is `src/solutions/SolutionDashboard.tsx` as it +stood at `8590d329`, before #1041 replaced its hand-rolled widget dispatch with +a mount of the kit's renderer. The duplication gate asserts that it would be +refused today, so the rule is pinned against the real duplication rather than a +paraphrase of it. + +It is committed rather than read with `git show` because the suite ships inside +an immutable module package: a consuming repository carries this source tree and +not this repository's history, so a fixture that reaches for an upstream commit +fails there with `fatal: not a git repository` while passing in full-history CI +— a portability failure that CI is structurally unable to see. + +The `.txt` extension is deliberate. The file is a byte fixture, not a module: +as `.tsx` it would be typechecked against today's imports, linted, and counted +by `ui-inventory.json`. diff --git a/module/services/frontend/code/src/components/__tests__/fixtures/deleted-solution-dashboard.txt b/module/services/frontend/code/src/components/__tests__/fixtures/deleted-solution-dashboard.txt new file mode 100644 index 000000000..526feeade --- /dev/null +++ b/module/services/frontend/code/src/components/__tests__/fixtures/deleted-solution-dashboard.txt @@ -0,0 +1,695 @@ +"use client"; + +import type { + Dashboard, + DataGraph, + MetricWidget, +} from "@codefly/saas-plugin-manifest"; +import { + createSaasClient, + type ResolvedWidget, + runDashboard, +} from "@codefly-dev/saas-sdk"; +// Charts come from the shared kit, not host-internal components: the same +// primitives a solution's own remote would render with, so host-rendered and +// solution-rendered dashboards look identical and there is one charting +// implementation to maintain. +import { + AreaChart, + BarList, + LineChart, + SortableGrid, + StatChart, +} from "@codefly-dev/ui/dashboard"; +import { useQuery } from "@tanstack/react-query"; +import { GripVertical, Info, Plus, X } from "lucide-react"; +import { type ReactNode, useState, useSyncExternalStore } from "react"; +import { flushSync } from "react-dom"; +import { resolveActor } from "@/features/audit/model/transforms"; +import { + useAuditEventTypes, + usePrincipalDirectory, +} from "@/features/audit/service/queries"; +import { scopedDashboardDraftKey } from "@/features/dashboard/service/use-dashboard-authoring"; +import { useAuth } from "@/lib/auth"; +import { apiTransport } from "@/lib/connect/transport"; +import { + Button, + Card, + CardContent, + CardHeader, + CardTitle, + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuTrigger, + Popover, + PopoverContent, + PopoverTitle, + PopoverTrigger, + Skeleton, +} from "@/shared/ui"; +import { + addableTiles, + addTile, + layoutKey, + moveBy, + parseLayout, + readSavedLayout, + removeTile, + serializeLayout, + swapTiles, + tileWidget, + writeSavedLayout, +} from "./dashboard-layout"; +import { + ACTIVITY_DASHBOARD, + activityGraph, + activitySummary, + describeMetric, + formatDay, + formatMoment, + newestEvents, + recentEventsQueries, +} from "./metric-info"; + +// One audit-backed SDK client for every solution dashboard, built on the host's +// shared Connect transport so a solution's declared graph resolves through the +// same auth'd, org-scoped gateway path the rest of the app uses. The host owns +// the client, the bearer token, and the org scope; the solution supplies only +// the DataGraph — it can never widen a query past the viewer's own org. +const sdk = createSaasClient(apiTransport); + +// Dashboard id of the throwaway single-widget graph each card resolves. The card +// resolves its own widget in isolation, so this id is never surfaced. +const SINGLE_WIDGET_DASHBOARD = "widget"; + +// A graph carrying every event and metric but exactly one widget, so runDashboard +// resolves only that widget's metric (and its derived inputs). Resolving each +// widget through its own call is what isolates failures: a widget whose metric's +// audit query errors fails alone, instead of blanking every sibling that shares +// the dashboard's single batched resolution. +function singleWidgetGraph(graph: DataGraph, widget: MetricWidget): DataGraph { + return { + events: graph.events, + metrics: graph.metrics, + dashboards: [ + { id: SINGLE_WIDGET_DASHBOARD, layout: "grid", widgets: [widget] }, + ], + }; +} + +// Render a resolved widget's series in the shape its visualization asks for. A +// series with no points is "no data yet" for every kind — the audit RPC omits a +// bucket rather than emitting a zero, so an empty series means nothing matched, +// not a real zero worth plotting. +function WidgetBody({ widget }: { widget: ResolvedWidget }) { + return ( + <> + {widget.series.coverage === "partial" && ( +

    + Partial telemetry +

    + )} + + + ); +} + +function WidgetValues({ widget }: { widget: ResolvedWidget }) { + const { series, visualization } = widget; + if (series.points.length === 0) { + return ( +

    + {series.coverage === "partial" + ? "Telemetry unavailable or incomplete." + : "No data yet."} +

    + ); + } + switch (visualization) { + case "line": + return ( + + ); + case "area": + return ( + + ); + case "bar": + return ; + case "number": + return series.total === null ? ( +

    + {series.coverage === "partial" + ? "Incomplete telemetry; total unavailable." + : "Total unavailable across groups."} +

    + ) : ( + + ); + case "table": + return ( + + + {series.points.map((point) => ( + + + + + ))} + +
    {point.key} + {point.value.toLocaleString()} +
    + ); + default: { + // Compile-time exhaustiveness: a new WidgetVisualization must be + // handled here or this assignment fails to type-check. + const _exhaustive: never = visualization; + return _exhaustive; + } + } +} + +function WidgetCard({ + graph, + widget, + solutionId, + dashboardId, + orgId, + grip, + info, + remove, +}: { + graph: DataGraph; + widget: MetricWidget; + solutionId: string; + dashboardId: string; + orgId: string; + grip: ReactNode; + info: ReactNode; + remove: ReactNode; +}) { + // The graph is part of the key so a solution that redeploys with a changed + // declaration refetches instead of serving another graph's cached series that + // happens to share the same solution/dashboard/widget/org ids. Disabled until + // the org resolves so the pre-org window reads as loading, never as empty. + const { data, isPending, isError } = useQuery({ + queryKey: [ + "solution-widget", + solutionId, + dashboardId, + widget.id, + orgId, + graph, + ], + queryFn: () => + runDashboard( + sdk.audit, + singleWidgetGraph(graph, widget), + SINGLE_WIDGET_DASHBOARD, + { orgId }, + ).then((resolved) => resolved.widgets[0]), + enabled: orgId !== "", + }); + + return ( + + + {grip} + + {widget.title ?? widget.metric} + + {info} + {remove} + + + {isError ? ( +

    Unable to load.

    + ) : isPending || !data ? ( + + ) : ( + + )} +     +     + ); +} + +// Reading `window.localStorage` can itself throw: blocked site data or a +// sandboxed frame raise a SecurityError from the getter, before any getItem. No +// storage means every viewer sees the declared default. +function browserStorage(): Storage | null { + if (typeof window === "undefined") return null; + try { + return window.localStorage; + } catch { + return null; + } +} + +// The saved layout is read from storage on each render, so there is nothing to +// subscribe to; a change made on this page is held in state instead. +function noSubscription() { + return () => {}; +} + +/** + * One viewer's layout of one dashboard, kept in this browser. The server + * cannot know what the browser saved, so it renders the declared default and + * React swaps in the saved layout after hydration; `useSyncExternalStore` is + * what makes that a defined re-render rather than a hydration mismatch. + * + * The layout is saved only on the viewer's own change, so a viewer who never + * customizes keeps following the declared default. + * + * STOPGAP (labelled): browser storage is an interim home, so a layout does not + * follow the viewer to another browser or device. ADR 0007 puts per-user + * dashboard preferences in a composed `user_settings` field keyed by dashboard + * id; that field and its typed settings-catalog entry do not exist yet (no + * tracking issue yet). When they land, this hook's read and write move there + * and nothing else changes. + */ +function useDashboardLayout( + storageKey: string, + dashboard: Dashboard, +): [string[], (next: readonly string[]) => void] { + const stored = useSyncExternalStore( + noSubscription, + () => readSavedLayout(browserStorage(), storageKey), + () => null, + ); + // The viewer's latest change on this page. It holds even when storage + // refuses the write; it just will not outlive a reload. + const [edited, setEdited] = useState<{ key: string; raw: string } | null>( + null, + ); + const layout = parseLayout( + edited?.key === storageKey ? edited.raw : stored, + dashboard, + ); + const save = (next: readonly string[]) => { + const raw = serializeLayout(next, dashboard); + setEdited({ key: storageKey, raw }); + writeSavedLayout(browserStorage(), storageKey, raw); + }; + return [layout, save]; +} + +const ARROW_STEP: Partial> = { + ArrowUp: -1, + ArrowLeft: -1, + ArrowDown: 1, + ArrowRight: 1, +}; + +// How many of the newest events the ⓘ lists. +const RECENT_EVENTS = 5; + +// Where a tile's number comes from: what it counts and from which audit +// events, read from the solution's declaration; how many events there are and +// the days they span, from per-day counts; and the newest events themselves, +// from the audit log. Nothing is asked of the audit trail until the viewer +// opens it. +function MetricInfo({ + graph, + widget, + orgId, +}: { + graph: DataGraph; + widget: MetricWidget; + orgId: string; +}) { + const [open, setOpen] = useState(false); + const title = widget.title ?? widget.metric; + const { counts, sources, note } = describeMetric( + graph, + widget.metric, + widget.visualization, + ); + const { data: registered } = useAuditEventTypes({ enabled: open }); + const activity = useQuery({ + queryKey: ["solution-metric-activity", widget.metric, orgId, graph], + queryFn: () => + runDashboard( + sdk.audit, + activityGraph(graph, widget.metric), + ACTIVITY_DASHBOARD, + { orgId }, + ).then((resolved) => + activitySummary(resolved.widgets.map((w) => w.series)), + ), + enabled: open && orgId !== "", + }); + const searches = recentEventsQueries(graph, widget.metric); + const recent = useQuery({ + queryKey: ["solution-metric-recent", widget.metric, orgId, graph], + queryFn: async () => + newestEvents( + await Promise.all( + (searches ?? []).map((search) => + sdk.audit + .queryAuditLog({ orgId, pageSize: RECENT_EVENTS, ...search }) + .then((response) => response.events), + ), + ), + RECENT_EVENTS, + ), + enabled: open && orgId !== "" && searches !== null, + }); + const { directory } = usePrincipalDirectory( + orgId, + recent.data?.map((event) => event.actorId) ?? [], + ); + const who = (actorId: string) => resolveActor(actorId, directory).label; + + const summary = activity.data; + const counted = activity.isPending + ? "Loading…" + : activity.isError + ? "Unavailable" + : null; + const dayFreshness = + counted ?? + (summary ? `Latest event on ${formatDay(summary.last)}` : "No events yet"); + // A metric the audit log search cannot narrow like the metric does (by + // collection) keeps the day its per-day counts give. + const newest = recent.data?.[0]; + const freshness = + searches === null || recent.isError + ? dayFreshness + : recent.isPending + ? "Loading…" + : newest + ? `${formatMoment(newest.at)}, by ${who(newest.actorId)}` + : "No events yet"; + + return ( + + + } + > + + + + {title} +
    +
    Counts
    +
    {counts}
    +
    Counted from
    +
    + {sources.map(({ type, declared }) => { + // The audit service's type list holds only the platform's own + // types, so a type a module registers falls back to what the + // solution's declaration says about it. + const description = + registered?.find((t) => t.name === type)?.description || + declared; + return ( +

    + {description && {description}} + + {type} + +

    + ); + })} +
    +
    Scope
    +
    Your organization
    +
    Period
    +
    All time
    +
    Based on
    +
    + {counted ?? + (summary + ? `${summary.events.toLocaleString()} ${summary.events === 1 ? "event" : "events"}` + : "No events yet")} +
    +
    Data range
    +
    + {counted ?? + (summary + ? formatDay(summary.first) === formatDay(summary.last) + ? formatDay(summary.first) + : `${formatDay(summary.first)} – ${formatDay(summary.last)}` + : "No events yet")} +
    +
    Freshness
    +
    {freshness}
    + {recent.data && recent.data.length > 0 && ( + <> +
    Recent events
    +
    +
      + {recent.data.map((event) => ( +
    • + {formatMoment(event.at, { year: false })} ·{" "} + {who(event.actorId)} +
      • + ))} +
    +
    + + )} + {note && ( + <> +
    Note
    +
    {note}
    + + )} +
    +     +     + ); +} + +// Lists what the viewer can put back on the dashboard: the declared widgets +// they removed. A preference never adds a metric the dashboard does not draw +// (ADR 0007). +function AddTileMenu({ + label, + addable, + onAdd, +}: { + label: string; + addable: { id: string; label: string }[]; + onAdd: (tileId: string) => void; +}) { + return ( + + + } + > + + + + {addable.length === 0 ? ( + + Every widget is on this dashboard + + ) : ( + addable.map((tile) => ( + onAdd(tile.id)}> + {tile.label} + + )) + )} + + + ); +} + +// A dashboard the viewer can arrange: drag a tile to reorder (or focus its grip +// and use the arrow keys), × to remove one, + to add one back. +function SolutionDashboard({ + graph, + dashboard, + solutionId, +}: { + graph: DataGraph; + dashboard: Dashboard; + solutionId: string; +}) { + const { user, organizationId } = useAuth(); + const orgId = organizationId ?? ""; + const storageKey = scopedDashboardDraftKey( + layoutKey(solutionId, dashboard.id), + { organizationId, userId: user?.id }, + ); + const [layout, save] = useDashboardLayout(storageKey, dashboard); + const shown = layout.flatMap((tileId) => { + const widget = tileWidget(dashboard, tileId); + return widget ? [{ tileId, widget }] : []; + }); + const widgets = new Map(shown.map(({ tileId, widget }) => [tileId, widget])); + const titleOf = (tileId: string) => { + const widget = widgets.get(tileId); + return widget?.title ?? widget?.metric ?? tileId; + }; + const renderTile = (tileId: string) => { + const widget = widgets.get(tileId); + if (!widget) return null; + const title = titleOf(tileId); + return ( + { + const step = ARROW_STEP[event.key]; + if (step === undefined) return; + event.preventDefault(); + const grip = event.currentTarget; + // Moving a tile can move its DOM node, which drops focus; + // commit first so the grip can take focus back. + flushSync(() => save(moveBy(layout, tileId, step))); + grip.focus(); + }} + > + + + } + info={} + remove={ + + } + /> + ); + }; + // The floating copy while a tile is dragged: the same card, but with no + // info popover (so no second panel or audit query exists mid-drag) and no + // controls a keyboard or screen reader could reach. The icons stay so the + // copy is drawn the same size as the tile it follows. + const renderDragged = (tileId: string) => { + const widget = widgets.get(tileId); + if (!widget) return null; + const glyph = (icon: ReactNode) => ( + + ); + return ( + + ); + }; + + return ( +
    +
    + {dashboard.title && ( +

    + {dashboard.title} +

    + )} + save(addTile(layout, tileId))} + /> +
    + {shown.length === 0 ? ( +

    + No widgets are shown. Add one back with the + button. +

    + ) : ( + // The kit's SortableGrid rather than its Grid/Stack: the tiles are + // dragged onto each other to swap. The classes are those Grid + // cols={2} and Stack draw with. + tileId)} + onSwap={(dragged, target) => save(swapTiles(layout, dragged, target))} + itemLabel={titleOf} + renderItem={renderTile} + renderOverlay={renderDragged} + className={ + dashboard.layout === "stack" + ? "flex flex-col gap-4" + : "grid grid-cols-1 gap-4 sm:grid-cols-2" + } + /> + )} +
    + ); +} + +/** + * Renders every dashboard a registered solution declares in its data-graph slot. + * Each widget resolves against the audit trail through its own query, so one + * widget's failure never blanks its siblings. A solution ships only the + * declaration; all charting lives here in the host. Each viewer arranges each + * dashboard for themselves, starting from the declared widgets in declared + * order. + */ +export function SolutionDashboards({ + graph, + solutionId, +}: { + graph: DataGraph; + solutionId: string; +}) { + return ( +
    + {graph.dashboards.map((dashboard) => ( + + ))} +
    + ); +} diff --git a/module/services/frontend/code/src/components/__tests__/kit.test.ts b/module/services/frontend/code/src/components/__tests__/kit.test.ts index 16125b251..9000a3814 100644 --- a/module/services/frontend/code/src/components/__tests__/kit.test.ts +++ b/module/services/frontend/code/src/components/__tests__/kit.test.ts @@ -195,69 +195,180 @@ function isHostSurface(specifier: string, fromPath: string): boolean { } /** - * The kit components a file has a binding for, as local-name → kit-name, with - * a namespace import recorded as `*`. + * What a local name in a file refers to. * - * Resolved from the AST rather than by regex, because a declaration's own body - * has to be checked against these bindings: file-wide import presence proved - * nothing. An unrelated `export { Card as KitCard } from "@codefly-dev/ui"` - * was enough to exempt a hand-rolled `Card` sitting beside it, which is the - * shape a duplicate actually arrives in — somebody re-exports the kit's thing - * for a neighbour and writes their own in the same file. + * `{ kit }` is a kit component, under the kit's OWN name whatever the local + * spelling. `{ namespace }` is a whole module imported as one object, so + * `` has to be resolved through it at the use site. + */ +type Binding = + | { kind: "kit"; kit: string } + | { kind: "namespace"; surface: boolean }; + +/** + * Every local name in a file that refers to the kit, resolved through named, + * aliased, default and namespace imports, and then through local aliases + * (`const Local = KitCard`). + * + * A host surface's names are mapped back to the kit's own: `src/shared/ui` + * re-exports `CardRoot` as `Card`, so importing or qualifying `Card` from + * there composes `CardRoot`, and a declaration named `Card` beside it is still + * a second `Card`. */ function kitBindings( file: ts.SourceFile, path: string, surface: Map, -): Map { - const bindings = new Map(); +): Map { + const bindings = new Map(); + const named = (published: string, fromKit: boolean): string | undefined => + fromKit ? published : surface.get(published); + for (const statement of file.statements) { if (!ts.isImportDeclaration(statement)) continue; if (!ts.isStringLiteral(statement.moduleSpecifier)) continue; const specifier = statement.moduleSpecifier.text; - const kit = KIT_SPECIFIER.test(specifier); - const host = isHostSurface(specifier, path); - if (!kit && !host) continue; + const fromKit = KIT_SPECIFIER.test(specifier); + const fromSurface = isHostSurface(specifier, path); + if (!fromKit && !fromSurface) continue; const clause = statement.importClause; - if (!clause) continue; - // `import * as UI from "@/shared/ui"` — every member access on it is a - // kit component, so the namespace itself is the binding. - if (clause.namedBindings && ts.isNamespaceImport(clause.namedBindings)) { - bindings.set(clause.namedBindings.name.text, "*"); - continue; - } - if (!clause.namedBindings || !ts.isNamedImports(clause.namedBindings)) { + // A type-only import renders nothing, so it is no evidence either way. + if (!clause || clause.isTypeOnly) continue; + const { namedBindings } = clause; + if (namedBindings && ts.isNamespaceImport(namedBindings)) { + bindings.set(namedBindings.name.text, { + kind: "namespace", + surface: fromSurface, + }); continue; } - for (const element of clause.namedBindings.elements) { - const published = (element.propertyName ?? element.name).text; - // Through a host surface the published name is that file's, so it is - // resolved back to the kit's own: `src/shared/ui` re-exports - // `CardRoot` as `Card`, so importing `Card` from there composes - // `CardRoot`, and a declaration named `Card` beside it is still a - // second `Card`. - const kitName = kit ? published : surface.get(published); - if (kitName) bindings.set(element.name.text, kitName); + if (namedBindings && ts.isNamedImports(namedBindings)) { + for (const element of namedBindings.elements) { + if (element.isTypeOnly) continue; + const kit = named((element.propertyName ?? element.name).text, fromKit); + if (kit) bindings.set(element.name.text, { kind: "kit", kit }); + } } } + + // Local aliases of an import: `const Local = KitCard`. Chased to a fixed + // point so a chain of them resolves, which is how a wrapper that reads + // more nicely still counts as composition. + for (let pass = 0; pass < 4; pass += 1) { + let changed = false; + const visit = (node: ts.Node) => { + if ( + ts.isVariableDeclaration(node) && + ts.isIdentifier(node.name) && + node.initializer && + ts.isIdentifier(node.initializer) && + !bindings.has(node.name.text) + ) { + const target = bindings.get(node.initializer.text); + if (target) { + bindings.set(node.name.text, target); + changed = true; + } + } + ts.forEachChild(node, visit); + }; + visit(file); + if (!changed) break; + } return bindings; } +/** + * The kit component a JSX tag renders, or undefined when it renders something + * else. Only JSX counts: a parameter typed `ComponentProps` + * mentions the kit and draws nothing, and reading plain identifiers let exactly + * that exempt a `Card` whose body was a bare `
    `. + * + * `shadowed` are names the enclosing declaration binds itself, which outrank + * the import — `const KitCard = () =>
    ` inside the body is not the kit's. + */ +function renderedKitComponent( + tag: ts.JsxTagNameExpression, + bindings: Map, + surface: Map, + shadowed: Set, +): string | undefined { + if (ts.isIdentifier(tag)) { + if (shadowed.has(tag.text)) return undefined; + const binding = bindings.get(tag.text); + return binding?.kind === "kit" ? binding.kit : undefined; + } + if (ts.isPropertyAccessExpression(tag) && ts.isIdentifier(tag.expression)) { + if (shadowed.has(tag.expression.text)) return undefined; + const binding = bindings.get(tag.expression.text); + if (binding?.kind !== "namespace") return undefined; + const member = tag.name.text; + // Through a surface namespace the member is that file's published + // name, so it resolves to the kit name behind it. + return binding.surface ? surface.get(member) : member; + } + return undefined; +} + +/** Every name declared inside `node` — the ones that shadow an import. */ +function locallyBound(node: ts.Node): Set { + const names = new Set(); + const visit = (child: ts.Node) => { + if ( + (ts.isVariableDeclaration(child) || + ts.isFunctionDeclaration(child) || + ts.isClassDeclaration(child) || + ts.isParameter(child)) && + child.name && + ts.isIdentifier(child.name) + ) { + names.add(child.name.text); + } + ts.forEachChild(child, visit); + }; + ts.forEachChild(node, visit); + return names; +} + +/** The kit components this declaration RENDERS, by kit name. */ +function renderedWithin( + node: ts.Node, + bindings: Map, + surface: Map, +): Set { + const shadowed = locallyBound(node); + const rendered = new Set(); + const visit = (child: ts.Node) => { + if (ts.isJsxOpeningElement(child) || ts.isJsxSelfClosingElement(child)) { + const kit = renderedKitComponent( + child.tagName, + bindings, + surface, + shadowed, + ); + if (kit) rendered.add(kit); + } + ts.forEachChild(child, visit); + }; + ts.forEachChild(node, visit); + return rendered; +} + /** * Every component this file declares, by the name it is PUBLISHED under, with * the declaration's own node. * - * The published name is what matters, and it is not always the declared one: - * `const LocalPanel = …; export { LocalPanel as Card }` ships a `Card`, and - * reading only declaration names let that straight through. + * The published name is not always the declared one: `const LocalPanel = …; + * export { LocalPanel as Card }` ships a `Card`, and reading only declaration + * names let that straight through. */ function declarations(file: ts.SourceFile): Map { const declared = new Map(); - const published = new Map(); const visit = (node: ts.Node) => { - if (ts.isFunctionDeclaration(node) && node.name) { - declared.set(node.name.text, node); - } else if (ts.isClassDeclaration(node) && node.name) { + if ( + (ts.isFunctionDeclaration(node) || ts.isClassDeclaration(node)) && + node.name + ) { declared.set(node.name.text, node); } else if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name)) { declared.set(node.name.text, node); @@ -265,8 +376,12 @@ function declarations(file: ts.SourceFile): Map { ts.forEachChild(node, visit); }; visit(file); - // `export { Local as Published }` with no `from`: a local declaration - // published under another name. + + const all = new Map(); + for (const [name, node] of declared) { + if (/^[A-Z][A-Za-z0-9]*$/.test(name)) all.set(name, node); + } + // `export { Local as Published }` with no `from`. for (const statement of file.statements) { if (!ts.isExportDeclaration(statement) || statement.moduleSpecifier) { continue; @@ -276,58 +391,28 @@ function declarations(file: ts.SourceFile): Map { for (const element of clause.elements) { const local = (element.propertyName ?? element.name).text; const node = declared.get(local); - if (node) published.set(element.name.text, node); + const published = element.name.text; + if (node && /^[A-Z][A-Za-z0-9]*$/.test(published)) { + all.set(published, node); + } } } - const all = new Map(); - for (const [name, node] of declared) { - if (/^[A-Z][A-Za-z0-9]*$/.test(name)) all.set(name, node); - } - for (const [name, node] of published) { - if (/^[A-Z][A-Za-z0-9]*$/.test(name)) all.set(name, node); - } return all; } -/** Whether this declaration's own body renders the kit component `kitName`. */ -function composesWithin( - node: ts.Node, - kitName: string, - bindings: Map, -): boolean { - let found = false; - const visit = (child: ts.Node) => { - if (found) return; - if (ts.isIdentifier(child) && bindings.get(child.text) === kitName) { - found = true; - return; - } - // `UI.Badge` off a namespace import. - if ( - ts.isPropertyAccessExpression(child) && - ts.isIdentifier(child.expression) && - bindings.get(child.expression.text) === "*" && - child.name.text === kitName - ) { - found = true; - return; - } - ts.forEachChild(child, visit); - }; - ts.forEachChild(node, visit); - return found; -} - /** - * The kit's dashboard marks. A host surface may compose ONE of these — a page - * drawing a chart — but a file that reaches for several is dispatching over a - * widget's visualization, which is ``'s whole job. + * The kit's dashboard marks. A surface may compose several of these on one + * page — a KPI row above a chart is a page, not a renderer. What it may not do + * is BRANCH between them: choosing a mark by a widget's kind is a dispatch over + * visualizations, and that is ``'s whole job. * - * This is the leg that catches a duplicate under a NEW name, which no - * name-based rule can: the renderer #1041 deleted declared `WidgetBody`, - * `WidgetValues` and `WidgetCard` — not one of them a name the kit exports — - * and so produced zero offences from the legs above while being the exact - * duplication this file exists to prevent. + * Branching is the test, not the count, because counting file-wide imports + * caught a page that composed two marks side by side and could be escaped by a + * namespace import or a local alias. This is also the only leg that catches a + * duplicate under a NEW name: the renderer #1041 deleted declared + * `WidgetBody`, `WidgetValues` and `WidgetCard` — not one of them a name the + * kit exports — so every name-based leg passed it while it was the duplication + * itself. */ const DASHBOARD_MARKS = new Set([ "LineChart", @@ -345,11 +430,67 @@ const DASHBOARD_MARKS = new Set([ ]); /** - * Files that draw with more than one of the kit's marks and may keep doing so, - * with what each is and what would retire it. They are the host's older - * dashboard renderers, the same ones {@link ALLOWED} names; this list exists so - * a NEW one cannot join them quietly. + * Whether this declaration picks between dashboard marks by branch: two + * different marks reachable from alternative arms of one conditional, if, or + * switch. */ +function dispatchesOverMarks( + node: ts.Node, + bindings: Map, + surface: Map, +): string[] { + const marksOf = (branch: ts.Node | undefined): Set => { + if (!branch) return new Set(); + // A branch is often the element itself (`cond ? : `), and + // `renderedWithin` walks a node's children rather than the node, so + // the branch's own tag is read here. + const rendered = renderedWithin(branch, bindings, surface); + if (ts.isJsxSelfClosingElement(branch) || ts.isJsxOpeningElement(branch)) { + const kit = renderedKitComponent( + branch.tagName, + bindings, + surface, + locallyBound(branch), + ); + if (kit) rendered.add(kit); + } + if (ts.isJsxElement(branch)) { + const kit = renderedKitComponent( + branch.openingElement.tagName, + bindings, + surface, + locallyBound(branch), + ); + if (kit) rendered.add(kit); + } + return new Set([...rendered].filter((mark) => DASHBOARD_MARKS.has(mark))); + }; + const found = new Set(); + const consider = (branches: (ts.Node | undefined)[]) => { + const perBranch = branches.map(marksOf); + const union = new Set(perBranch.flatMap((marks) => [...marks])); + // Two distinct marks, reached through different arms. + if ( + union.size > 1 && + perBranch.filter((marks) => marks.size > 0).length > 1 + ) { + for (const mark of union) found.add(mark); + } + }; + const visit = (child: ts.Node) => { + if (ts.isConditionalExpression(child)) { + consider([child.whenTrue, child.whenFalse]); + } else if (ts.isIfStatement(child)) { + consider([child.thenStatement, child.elseStatement]); + } else if (ts.isSwitchStatement(child)) { + consider([...child.caseBlock.clauses]); + } + ts.forEachChild(child, visit); + }; + visit(node); + return [...found].sort(); +} + const DISPATCHERS: Record = { "src/components/dashboard.tsx": "the widget-slot renderer: a union of host-supplied widgets, each with its own loading/error/empty state. Goes when the audit page's widgets become a DashboardView and it mounts the kit's renderer.", @@ -446,10 +587,32 @@ interface Offence { const ROUTE_CONVENTION = /^src\/app\/(?:.*\/)?(page|layout|template|default|error|global-error|loading|not-found|forbidden|unauthorized)\.tsx?$/; -/** The names a file's default export declares, to exempt a route component. */ +/** + * The names a file's default export resolves to, in every form a route + * component is written: `export default function Page`, `export default Page`, + * and `export { Page as default }`. Resolving only the first rejected two + * conventional pages. + */ function defaultExportNames(file: ts.SourceFile): Set { const names = new Set(); for (const statement of file.statements) { + if (ts.isExportAssignment(statement)) { + if (ts.isIdentifier(statement.expression)) { + names.add(statement.expression.text); + } + continue; + } + if (ts.isExportDeclaration(statement) && !statement.moduleSpecifier) { + const clause = statement.exportClause; + if (clause && ts.isNamedExports(clause)) { + for (const element of clause.elements) { + if (element.name.text === "default" && element.propertyName) { + names.add(element.propertyName.text); + } + } + } + continue; + } const isDefault = ts .getModifiers(statement as ts.HasModifiers) ?.some((modifier) => modifier.kind === ts.SyntaxKind.DefaultKeyword); @@ -466,10 +629,59 @@ function defaultExportNames(file: ts.SourceFile): Set { } /** - * Declarations that take a kit export's name without composing the kit's - * component of that name WITHIN their own body, plus files that dispatch over - * more than one of the kit's dashboard marks. Pure, so the fixtures below can - * exercise it on sources that are not in the tree. + * Names a file publishes that are declared in ANOTHER file of the set: + * `export { LocalPanel as Card } from "./local"`. Without this, splitting the + * copy and its kit-named export across two files hid it from both. + */ +function reExportedLocally( + files: { path: string; source: string }[], +): { name: string; path: string }[] { + const declaredIn = new Map>(); + for (const { path, source } of files) { + declaredIn.set(path, new Set(declarations(parse(path, source)).keys())); + } + const published: { name: string; path: string }[] = []; + for (const { path, source } of files) { + const file = parse(path, source); + for (const statement of file.statements) { + if (!ts.isExportDeclaration(statement) || !statement.moduleSpecifier) { + continue; + } + if (!ts.isStringLiteral(statement.moduleSpecifier)) continue; + const specifier = statement.moduleSpecifier.text; + if (!specifier.startsWith(".")) continue; + const target = join(dirname(path), specifier).replaceAll("\\", "/"); + const names = [...declaredIn.entries()].find( + ([other]) => + other === target || + other.startsWith(`${target}.`) || + other.startsWith(`${target}/`), + )?.[1]; + if (!names) continue; + const clause = statement.exportClause; + if (!clause || !ts.isNamedExports(clause)) continue; + for (const element of clause.elements) { + const local = (element.propertyName ?? element.name).text; + const name = element.name.text; + // Only an ALIASING re-export publishes a name the declaration + // does not already carry. `export { Dashboard } from "./ui"` is + // a barrel: that declaration was judged at its own site, and + // flagging it again would demand a second exception for one + // component. `export { LocalPanel as Card }` is the other + // thing entirely — a kit name appearing where no declaration + // answers for it. + if (local !== name && names.has(local)) published.push({ name, path }); + } + } + } + return published; +} + +/** + * Declarations that take a kit export's name without RENDERING the kit's + * component of that name, plus declarations that branch between the kit's + * dashboard marks. Pure, so the fixtures below can exercise it on sources that + * are not in the tree. */ function findOffences( files: { path: string; source: string }[], @@ -480,8 +692,9 @@ function findOffences( dispatchers: Record = DISPATCHERS, ): Offence[] { const offences: Offence[] = []; - for (const { path, source } of files) { - if (KIT_SURFACES.includes(path)) continue; + const scanned = files.filter(({ path }) => !KIT_SURFACES.includes(path)); + + for (const { path, source } of scanned) { const file = parse(path, source); const bindings = kitBindings(file, path, surface); const routeDefaults = ROUTE_CONVENTION.test(path) @@ -490,25 +703,28 @@ function findOffences( for (const [name, node] of declarations(file)) { if (!kit.has(name) || routeDefaults.has(name)) continue; - if (composesWithin(node, name, bindings)) continue; + if (renderedWithin(node, bindings, surface).has(name)) continue; if (`${name} in ${path}` in allowed) continue; offences.push({ name, path, kind: "name" }); } if (!(path in dispatchers)) { - const marks = new Set(); - for (const kitName of bindings.values()) { - if (DASHBOARD_MARKS.has(kitName)) marks.add(kitName); - } - if (marks.size > 1) { - offences.push({ - name: [...marks].sort().join("+"), - path, - kind: "dispatch", - }); + for (const [, node] of declarations(file)) { + const marks = dispatchesOverMarks(node, bindings, surface); + if (marks.length > 1) { + offences.push({ name: marks.join("+"), path, kind: "dispatch" }); + break; + } } } } + + for (const { name, path } of reExportedLocally(scanned)) { + if (!kit.has(name)) continue; + if (`${name} in ${path}` in allowed) continue; + if (offences.some((o) => o.name === name && o.path === path)) continue; + offences.push({ name, path, kind: "name" }); + } return offences; } @@ -600,6 +816,14 @@ describe("the detector", () => { ["Badge", "Badge"], ["Card", "CardRoot"], ]); + const deletedRenderer = () => + readFileSync( + join( + appRoot, + "src/components/__tests__/fixtures/deleted-solution-dashboard.txt", + ), + "utf8", + ); const detect = (path: string, source: string) => findOffences([{ path, source }], kit, {}, surface, {}).map( (o) => `${o.name} in ${o.path}`, @@ -808,24 +1032,163 @@ export const Badge = (props) => ;`, ).toEqual([]); }); - it("rejects the exact renderer this gate was written to prevent", async () => { + it("rejects the exact renderer this gate was written to prevent", () => { // The case that shows the name legs are not enough on their own: the // renderer #1041 deleted declared WidgetBody, WidgetValues and // WidgetCard — not one of them a name the kit exports — so every // name-based leg passed it while it was the duplication itself. The - // dispatch leg is what catches it, and this reads the real deleted - // source rather than a paraphrase of it. - const { execFileSync } = await import("node:child_process"); - const deleted = execFileSync( - "git", - [ - "show", - "8590d329:module/services/frontend/code/src/solutions/SolutionDashboard.tsx", - ], - { encoding: "utf8", cwd: appRoot }, - ); + // dispatch leg is what catches it. + // + // Read from a committed fixture, not with `git show`: this suite ships + // inside an immutable module package, where a consuming repository + // carries the source tree and not this repository's history, so a + // fixture reaching for an upstream commit fails there while passing in + // full-history CI. + expect( + detect("src/solutions/SolutionDashboard.tsx", deletedRenderer()).length, + ).toBeGreaterThan(0); + }); + + it("still rejects that renderer when its imports become a namespace", () => { + // Changing only how the marks are imported, and qualifying the JSX, + // must not change the verdict: a gate that reads local names rather + // than what they resolve to is escaped by one edit. + const named = + /import \{\s*AreaChart,\s*BarList,\s*LineChart,\s*SortableGrid,\s*StatChart,\s*\} from "@codefly-dev\/ui\/dashboard";/; + const original = deletedRenderer(); + expect(named.test(original)).toBe(true); + const namespaced = original + .replace(named, 'import * as Charts from "@codefly-dev/ui/dashboard";') + .replace( + /<(\/?)(AreaChart|BarList|LineChart|SortableGrid|StatChart)\b/g, + "<$1Charts.$2", + ); + expect( + detect("src/solutions/SolutionDashboard.tsx", namespaced).length, + ).toBeGreaterThan(0); + }); + + it("rejects that renderer when its marks are local aliases", () => { + expect( + detect( + "src/features/x/ui/widget.tsx", + `import { LineChart as L, AreaChart as A } from "@codefly-dev/ui/dashboard"; +export function WidgetBody({ kind }) { + return kind === "line" ? : ; +}`, + ).length, + ).toBeGreaterThan(0); + }); + + it("does not read a type reference as composition", () => { + // A parameter typed ComponentProps mentions the kit and + // draws nothing; reading plain identifiers exempted a Card whose body + // was a bare
    . + expect( + detect( + "src/features/x/ui/card.tsx", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export const Card = (props: React.ComponentProps) => ( +
    {props.children}
    +);`, + ), + ).toEqual(["Card in src/features/x/ui/card.tsx"]); + }); + + it("does not let a local shadow stand in for the kit's component", () => { + expect( + detect( + "src/features/x/ui/card.tsx", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card() { + const KitCard = () =>
    ; + return ; +}`, + ), + ).toEqual(["Card in src/features/x/ui/card.tsx"]); + }); + + it("resolves a surface namespace member to the kit name behind it", () => { + // `@/shared/ui` publishes CardRoot as Card, so composes + // CardRoot and a declaration named Card is still a second Card. + expect( + detect( + "src/features/x/ui/card.tsx", + `import * as UI from "@/shared/ui"; +export const Card = () =>

    copy

    ;`, + ), + ).toEqual(["Card in src/features/x/ui/card.tsx"]); + }); + + it("follows a local alias of a kit import", () => { + expect( + detect( + "src/features/x/ui/card.tsx", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +const Local = KitCard; +export const Card = (props) => ;`, + ), + ).toEqual([]); + }); + + it("allows a wrapper that renders through a kit namespace", () => { + expect( + detect( + "src/features/x/ui/badge.tsx", + `import * as UI from "@codefly-dev/ui/layout"; +export default function Badge(props) { return ; }`, + ), + ).toEqual([]); + }); + + it.each([ + [ + "a default identifier", + "const Page = () =>
    ; export default Page;", + ], + [ + "an aliased default", + "const Page = () =>
    ; export { Page as default };", + ], + ])("applies the route exception to %s", (_name, source) => { + expect(detect("src/app/page.tsx", source)).toEqual([]); + }); + + it("does not read two marks composed side by side as a dispatch", () => { + // A KPI row above a chart is a page. What a page may not do is BRANCH + // between marks, which is the dispatch owns. + expect( + detect( + "src/features/x/ui/overview.tsx", + `import { KPIRow, MetricLineChart } from "@codefly-dev/ui/dashboard"; +export const Overview = () => ( + <> + + + +);`, + ), + ).toEqual([]); + }); + + it("catches a copy split across two files by an aliasing re-export", () => { expect( - detect("src/solutions/SolutionDashboard.tsx", deleted).length, + findOffences( + [ + { + path: "src/features/x/ui/local.tsx", + source: "export const LocalPanel = () =>
    copy
    ;", + }, + { + path: "src/features/x/ui/index.ts", + source: 'export { LocalPanel as Card } from "./local";', + }, + ], + kit, + {}, + surface, + {}, + ).length, ).toBeGreaterThan(0); }); diff --git a/module/services/frontend/code/src/shared/ui/metric-tiles.test.tsx b/module/services/frontend/code/src/shared/ui/metric-tiles.test.tsx index f64721c54..56b00c0eb 100644 --- a/module/services/frontend/code/src/shared/ui/metric-tiles.test.tsx +++ b/module/services/frontend/code/src/shared/ui/metric-tiles.test.tsx @@ -95,6 +95,49 @@ describe("StatTile", () => { expect(screen.getByText("No data")).toBeTruthy(); }); + it.each([ + ["StatTile", StatTile], + ["MetricCard", MetricCard], + ])( + "%s withholds everything derived from a forbidden value, not just the value", + (_name, Tile) => { + // "No access" beside "+170%" and a trend line publishes the figure + // twice over, in the two forms hardest to notice. The renderer + // builds a fresh metric and avoids this; a direct consumer of the + // exported primitive does not. + const { container } = render( + , + ); + expect(screen.getByText("No access")).toBeTruthy(); + expect(screen.getByText("—")).toBeTruthy(); + expect(screen.queryByText("37")).toBeNull(); + expect(screen.queryByText(/170%/)).toBeNull(); + expect(container.querySelector("svg")).toBeNull(); + }, + ); + + it("colours its trend from the chart token when asked, so an accent reaches it", () => { + // The sparkline strokes currentColor under a muted utility, so neither + // of a dashboard's accent overrides could reach it. + const { container } = render( + , + ); + const svg = container.querySelector("svg") as SVGElement; + expect(svg.style.color).toBe("var(--chart-1)"); + expect(svg.getAttribute("class")).not.toContain("text-muted-foreground"); + }); + it("shows a skeleton instead of a value while loading", () => { const { container } = render( , diff --git a/module/services/frontend/code/src/solutions/SolutionDashboard.tsx b/module/services/frontend/code/src/solutions/SolutionDashboard.tsx index 131557e92..8cc177cb2 100644 --- a/module/services/frontend/code/src/solutions/SolutionDashboard.tsx +++ b/module/services/frontend/code/src/solutions/SolutionDashboard.tsx @@ -20,9 +20,14 @@ import { SortableGrid, type WidgetReadFailure, } from "@codefly-dev/ui/dashboard"; -import { useQuery } from "@tanstack/react-query"; +import { useQuery, useQueryClient } from "@tanstack/react-query"; import { GripVertical, Info, Plus, X } from "lucide-react"; -import { type ReactNode, useState, useSyncExternalStore } from "react"; +import { + type ReactNode, + useEffect, + useState, + useSyncExternalStore, +} from "react"; import { flushSync } from "react-dom"; import { resolveActor } from "@/features/audit/model/transforms"; import { @@ -126,6 +131,7 @@ function SolutionWidget({ // declaration refetches instead of serving another graph's cached series that // happens to share the same solution/dashboard/widget/org ids. Disabled until // the org resolves so the pre-org window reads as loading, never as empty. + const client = useQueryClient(); const { data, isError, error } = useQuery({ queryKey: [ "solution-widget", @@ -152,21 +158,57 @@ function SolutionWidget({ // wrong answer `mayKeepRetainedRows` exists to make unwritable, arrived at // from the dashboard instead of from a table. A transient failure says // nothing about authority, so those values stay and read as stale. + // + // Hiding the snapshot for the refusal alone is not enough, and that was + // this component's bug: the cached snapshot survived the refusal, so the + // NEXT error — a 503, a dropped connection — made it eligible again and + // handed the refused values straight back, with no authorized read in + // between. A refusal therefore latches: the snapshot stays ineligible + // until a successful read replaces it. const outcome = readOutcome(isError, error); - const retained = mayKeepRetainedRows(outcome) ? (data?.series ?? null) : null; + const authorized = !isError && data !== undefined; + // The latch lives in the query cache, beside the snapshot it guards, for + // two reasons: it outlives this component (a latch in state or a ref + // forgets on remount, and the snapshot does not), and it expires with the + // cache that holds the values, so neither can outlast the other. + const latchKey = refusedLatchKey(solutionId, dashboardId, widget.id, orgId); + const latched = client.getQueryData(latchKey) === true; + const widgetId = widget.id; + useEffect(() => { + const key = refusedLatchKey(solutionId, dashboardId, widgetId, orgId); + if (outcome === "forbidden") client.setQueryData(key, true); + else if (authorized) client.setQueryData(key, false); + }, [client, outcome, authorized, solutionId, dashboardId, widgetId, orgId]); + + const refused = outcome === "forbidden" || (!authorized && latched); + const retained = + !refused && mayKeepRetainedRows(outcome) ? (data?.series ?? null) : null; return ( ); } +// Where a widget's refusal latch is kept in the query cache. Not a read: it +// holds a boolean this component writes, under its own prefix so it can never +// be mistaken for a resolved widget. Built by a function rather than inline so +// the effect below depends on its parts and not on a fresh array each render. +function refusedLatchKey( + solutionId: string, + dashboardId: string, + widgetId: string, + orgId: string, +): readonly string[] { + return ["solution-widget-refused", solutionId, dashboardId, widgetId, orgId]; +} + // The host's read classification as the renderer's presentational one. A read // that succeeded is no failure at all, which is what `undefined` says. const WIDGET_FAILURE: Record = { diff --git a/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx b/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx index 4c1010f61..d9faf28bc 100644 --- a/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx +++ b/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx @@ -121,6 +121,11 @@ function aggregateHandler( ); } +// A rejected refetch reaches the component a tick after `refetchQueries` +// resolves. Without this wait an assertion can run while the previous state is +// still rendered, which makes the retention tests pass whatever the code does. +const settle = () => new Promise((resolve) => setTimeout(resolve, 20)); + const DEFAULT_ORDER = ["Logins over time", "Top event types", "Total logins"]; // The tile titles, in the order the dashboard shows them. The kit's renderer @@ -265,6 +270,96 @@ describe("SolutionDashboards", () => { }); }); + // The longer sequence, and the one that matters most: a refusal followed by + // an ordinary failure, with no authorized read in between. The cached + // snapshot outlives the refusal, so unless the refusal LATCHES, the next + // error makes those values eligible again and hands them straight back. + describe.each(["line", "area", "bar", "number", "table"] as const)( + "a refused %s widget", + (visualization) => { + const oneWidget: DataGraph = { + ...graph, + dashboards: [ + { + ...graph.dashboards[0], + widgets: [{ ...graph.dashboards[0].widgets[0], visualization }], + }, + ], + }; + + it.each([ + [ + "a 503", + () => HttpResponse.json({ code: "unavailable" }, { status: 503 }), + ], + ["a dropped connection", () => HttpResponse.error()], + ])("does not resurrect its values on %s", async (_name, fail) => { + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ + buckets: [{ key: "2026-09-01", count: "37" }], + }), + ), + ); + const { client } = renderInApp( + , + ); + expect(await screen.findAllByText("37")).not.toHaveLength(0); + + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ code: "permission_denied" }, { status: 403 }), + ), + ); + await act(async () => { + await client.refetchQueries({ queryKey: ["solution-widget"] }); + await settle(); + }); + expect(await screen.findByText("No access")).toBeTruthy(); + expect(screen.queryAllByText("37")).toHaveLength(0); + + server.use(http.post(rpc("AuditService", "AggregateAuditLog"), fail)); + await act(async () => { + await client.refetchQueries({ queryKey: ["solution-widget"] }); + await settle(); + }); + expect(screen.queryAllByText("37")).toHaveLength(0); + }); + }, + ); + + it("lets a successful authorized read lift the refusal", async () => { + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ buckets: [{ key: "2026-09-01", count: "37" }] }), + ), + ); + const { client } = renderDashboards(); + expect(await screen.findAllByText("37")).not.toHaveLength(0); + + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ code: "permission_denied" }, { status: 403 }), + ), + ); + await act(async () => { + await client.refetchQueries({ queryKey: ["solution-widget"] }); + }); + expect(await screen.findAllByText("No access")).not.toHaveLength(0); + + // Authority restored: the latch lifts and the NEW values are drawn. + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ buckets: [{ key: "2026-09-01", count: "42" }] }), + ), + ); + await act(async () => { + await client.refetchQueries({ queryKey: ["solution-widget"] }); + }); + expect(await screen.findAllByText("42")).not.toHaveLength(0); + expect(screen.queryByText("No access")).toBeNull(); + }); + it("reads as loading until the org context resolves, never as empty", () => { authState.organizationId = undefined; From 037c8aad5190ed3d1d11a570a3a15a3f67b394cb Mon Sep 17 00:00:00 2001 From: Codefly Automation <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 11 Oct 2026 11:26:56 -0400 Subject: [PATCH 5/6] fix(dashboard): a refusal deletes the snapshots, and the gate asks the compiler (#1041) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round three of the independent review found one blocker and one major, and prescribed the shape of both fixes. Both prescriptions were better than what they replaced, and both of mine had the same underlying fault: a rule enforced by a second thing kept alongside the first, rather than by removing what the rule is about. **BLOCKER — the refusal latch could expire, or be cleared without a new grant.** The latch and the values it guarded were separate cache entries with separate lifetimes. The latch had no observer, so it was garbage-collected on its own five-minute clock while the observed values stayed, and `200 → 403 → wait → 503` handed the refused numbers back on all five visualizations. Worse, `!isError && data !== undefined` read cached availability as a successful read, so a cached success for another revision of the same graph — a different dashboard title is enough — cleared the latch for all of them. There is no latch now. A refusal clears the snapshots themselves, for every cached revision of that widget: the graph is the last part of the query key, so a declaration that differs only in a title has its own entry holding the same numbers. Nothing restorable is left, so there is no second thing to keep in step, and only a read that succeeds can put values back. `resetQueries`, not `removeQueries`: removing destroys the query the component is observing, which leaves it with nothing to refetch and no way back even once authority returns — the recovery control catches that. A reset discards the data and keeps the query. Three regressions: past the default gcTime on the cache's own clock, a remount into the same client, and the cross-revision sequence. **MAJOR — the gate still misjudged composition and dispatch.** Nine fixtures, in both directions: a guard-return dispatch and a destructured shadow accepted, an unused nested wrapper counted as composition, a two-hop barrel hiding a copy; and function-local aliases, namespace-member aliases, nested shadows that never reach the outer JSX, layout branches, and a genuine wrapper re-exported under the kit's name all rejected. Every one of those is the same mistake — re-deriving by hand what the compiler already knows. The detector now builds a `ts.Program` over the sources under test and asks the checker what an identifier refers to. Relative specifiers resolve between those files; the kit deliberately does not, because a kit binding is evidenced by the import declaration in the source and needs no installed package, so no resolution failure can weaken the rule. Scope, destructuring, alias chains and export chains of any length all follow from that one question. Composition is judged only on JSX the component RETURNS, so an unused nested wrapper and a render prop stop counting. Dispatch is two return paths that render DIFFERENT marks, which is what separates a layout drawing the same pair in two wrappers from a renderer choosing a line or an area by a widget's kind; the ternary and guard-return spellings are both return paths and need no rule of their own. One consequence worth naming: an exception is recorded where a component is DECLARED, not where it is published, so a barrel re-exporting the host's own `Dashboard` no longer needs a second entry for one component. `ALLOWED` and `DISPATCHERS` are unchanged in content — no allowlist grew to make this pass. Six mutations, each shown to red a test: whole-declaration scanning instead of return paths, flattened paths, no local-alias following, no export-chain following, descending into nested functions, and for the blocker, no reset and a reset narrowed to one revision. Refs #1041 Co-Authored-By: Claude Opus 5 (1M context) --- .../code/src/components/__tests__/kit.test.ts | 812 +++++++++++------- .../code/src/solutions/SolutionDashboard.tsx | 61 +- .../__tests__/SolutionDashboard.test.tsx | 135 +++ .../frontend/code/src/test/container.tsx | 15 +- 4 files changed, 665 insertions(+), 358 deletions(-) diff --git a/module/services/frontend/code/src/components/__tests__/kit.test.ts b/module/services/frontend/code/src/components/__tests__/kit.test.ts index 9000a3814..cd91e340c 100644 --- a/module/services/frontend/code/src/components/__tests__/kit.test.ts +++ b/module/services/frontend/code/src/components/__tests__/kit.test.ts @@ -201,203 +201,327 @@ function isHostSurface(specifier: string, fromPath: string): boolean { * spelling. `{ namespace }` is a whole module imported as one object, so * `` has to be resolved through it at the use site. */ -type Binding = - | { kind: "kit"; kit: string } - | { kind: "namespace"; surface: boolean }; +/** + * A program over just these sources, so a symbol can be resolved the way the + * compiler resolves it. + * + * Hand-rolled scope analysis is what kept misjudging this: it collected names + * across scopes (so a nested parameter shadowed an outer tag), missed + * destructured bindings (so a parameter did NOT shadow when it should have), + * and could not follow an export chain at all. The checker answers all of + * those from one question — what does this identifier refer to — so the rules + * below ask it instead of re-deriving it. + * + * Only these files are in the program: relative specifiers resolve between + * them and everything else (`react`, the kit itself) stays unresolved on + * purpose. A kit binding is recognised from the import declaration in the + * source, which is present whether or not the package is installed, so the + * gate needs no `node_modules` and cannot be weakened by a resolution failure. + */ +function programOver(files: { path: string; source: string }[]): { + program: ts.Program; + checker: ts.TypeChecker; + sources: Map; +} { + const sources = new Map( + files.map(({ path, source }) => [path, parse(path, source)]), + ); + const host: ts.CompilerHost = { + fileExists: (name) => sources.has(name), + readFile: (name) => sources.get(name)?.text, + getSourceFile: (name) => sources.get(name), + getDefaultLibFileName: () => "lib.d.ts", + writeFile: () => {}, + getCurrentDirectory: () => "", + getCanonicalFileName: (name) => name, + useCaseSensitiveFileNames: () => true, + getNewLine: () => "\n", + resolveModuleNames: (names, containingFile) => + names.map((name) => { + if (!name.startsWith(".")) return undefined; + const base = join(dirname(containingFile), name).replaceAll("\\", "/"); + for (const candidate of [ + base, + `${base}.tsx`, + `${base}.ts`, + `${base}/index.tsx`, + `${base}/index.ts`, + ]) { + if (sources.has(candidate)) { + return { + resolvedFileName: candidate, + extension: candidate.endsWith(".tsx") + ? ts.Extension.Tsx + : ts.Extension.Ts, + }; + } + } + return undefined; + }), + }; + const program = ts.createProgram( + [...sources.keys()], + { + jsx: ts.JsxEmit.Preserve, + noLib: true, + noResolve: false, + allowJs: false, + target: ts.ScriptTarget.Latest, + module: ts.ModuleKind.ESNext, + }, + host, + ); + return { program, checker: program.getTypeChecker(), sources }; +} + +/** The import declaration a symbol's declaration sits under, if any. */ +function importOf(declaration: ts.Node): ts.ImportDeclaration | undefined { + for (let node: ts.Node | undefined = declaration; node; node = node.parent) { + if (ts.isImportDeclaration(node)) return node; + } + return undefined; +} /** - * Every local name in a file that refers to the kit, resolved through named, - * aliased, default and namespace imports, and then through local aliases - * (`const Local = KitCard`). + * The kit component an identifier refers to, by the kit's OWN name, or + * `"*namespace"` for a whole kit module, or undefined for anything else — + * including anything the checker cannot resolve, which is the conservative + * answer: unresolved is not composition. * - * A host surface's names are mapped back to the kit's own: `src/shared/ui` - * re-exports `CardRoot` as `Card`, so importing or qualifying `Card` from - * there composes `CardRoot`, and a declaration named `Card` beside it is still - * a second `Card`. + * Follows what the compiler follows: import aliases, local variable aliases + * (`const Local = KitCard`, `const Local = UI.Card`) and export chains of any + * length. */ -function kitBindings( - file: ts.SourceFile, - path: string, +function kitComponentOf( + node: ts.Expression | ts.JsxTagNameExpression, + checker: ts.TypeChecker, surface: Map, -): Map { - const bindings = new Map(); - const named = (published: string, fromKit: boolean): string | undefined => - fromKit ? published : surface.get(published); - - for (const statement of file.statements) { - if (!ts.isImportDeclaration(statement)) continue; - if (!ts.isStringLiteral(statement.moduleSpecifier)) continue; - const specifier = statement.moduleSpecifier.text; - const fromKit = KIT_SPECIFIER.test(specifier); - const fromSurface = isHostSurface(specifier, path); - if (!fromKit && !fromSurface) continue; - const clause = statement.importClause; - // A type-only import renders nothing, so it is no evidence either way. - if (!clause || clause.isTypeOnly) continue; - const { namedBindings } = clause; - if (namedBindings && ts.isNamespaceImport(namedBindings)) { - bindings.set(namedBindings.name.text, { - kind: "namespace", - surface: fromSurface, - }); - continue; - } - if (namedBindings && ts.isNamedImports(namedBindings)) { - for (const element of namedBindings.elements) { - if (element.isTypeOnly) continue; - const kit = named((element.propertyName ?? element.name).text, fromKit); - if (kit) bindings.set(element.name.text, { kind: "kit", kit }); - } - } + seen = new Set(), +): string | undefined { + if (seen.has(node)) return undefined; + seen.add(node); + + // `UI.Card` — resolve the namespace, then the member through the surface. + if (ts.isPropertyAccessExpression(node)) { + const base = kitComponentOf(node.expression, checker, surface, seen); + if (base === KIT_NAMESPACE) return node.name.text; + if (base === SURFACE_NAMESPACE) return surface.get(node.name.text); + return undefined; } - - // Local aliases of an import: `const Local = KitCard`. Chased to a fixed - // point so a chain of them resolves, which is how a wrapper that reads - // more nicely still counts as composition. - for (let pass = 0; pass < 4; pass += 1) { - let changed = false; - const visit = (node: ts.Node) => { + if (!ts.isIdentifier(node)) return undefined; + + let symbol = checker.getSymbolAtLocation(node); + if (!symbol) return undefined; + // Through import and export aliases, however many hops. + for (let hop = 0; hop < 8 && symbol.flags & ts.SymbolFlags.Alias; hop += 1) { + const target = checker.getImmediateAliasedSymbol(symbol); + const declaration = symbol.declarations?.[0]; + // An import of a kit module is where the chain ends: the module is not + // in the program, so there is nothing further to resolve, and the + // import declaration in the source is the evidence. + if (declaration) { + const importDeclaration = importOf(declaration); if ( - ts.isVariableDeclaration(node) && - ts.isIdentifier(node.name) && - node.initializer && - ts.isIdentifier(node.initializer) && - !bindings.has(node.name.text) + importDeclaration && + ts.isStringLiteral(importDeclaration.moduleSpecifier) ) { - const target = bindings.get(node.initializer.text); - if (target) { - bindings.set(node.name.text, target); - changed = true; + const specifier = importDeclaration.moduleSpecifier.text; + const fromKit = KIT_SPECIFIER.test(specifier); + const fromSurface = isHostSurface( + specifier, + importDeclaration.getSourceFile().fileName, + ); + if (fromKit || fromSurface) { + if (ts.isNamespaceImport(declaration)) { + return fromKit ? KIT_NAMESPACE : SURFACE_NAMESPACE; + } + if (ts.isImportSpecifier(declaration)) { + if (declaration.isTypeOnly) return undefined; + const published = (declaration.propertyName ?? declaration.name) + .text; + return fromKit ? published : surface.get(published); + } + return undefined; } } - ts.forEachChild(node, visit); - }; - visit(file); - if (!changed) break; + } + if (!target) break; + symbol = target; + } + + // A local alias: follow its initializer. + const declaration = symbol.declarations?.[0]; + if ( + declaration && + ts.isVariableDeclaration(declaration) && + declaration.initializer + ) { + return kitComponentOf(declaration.initializer, checker, surface, seen); } - return bindings; + return undefined; } +const KIT_NAMESPACE = "*kit"; +const SURFACE_NAMESPACE = "*surface"; + /** - * The kit component a JSX tag renders, or undefined when it renders something - * else. Only JSX counts: a parameter typed `ComponentProps` - * mentions the kit and draws nothing, and reading plain identifiers let exactly - * that exempt a `Card` whose body was a bare `
    `. + * The kit components a declaration RETURNS, as the sets reachable by each + * distinct return path. * - * `shadowed` are names the enclosing declaration binds itself, which outrank - * the import — `const KitCard = () =>
    ` inside the body is not the kit's. + * Two things this must not do. It must not count JSX in a nested function — + * an unused local wrapper is not what this component renders. And it must not + * flatten the paths, because the difference between a page and a dispatch is + * whether the arms render DIFFERENT marks: a layout that draws the same pair + * inside a `
    ` or a `
    ` is one picture twice, while choosing a + * line or an area by a widget's kind is a dispatch. */ -function renderedKitComponent( - tag: ts.JsxTagNameExpression, - bindings: Map, +function returnedPaths( + declaration: ts.Node, + checker: ts.TypeChecker, surface: Map, - shadowed: Set, -): string | undefined { - if (ts.isIdentifier(tag)) { - if (shadowed.has(tag.text)) return undefined; - const binding = bindings.get(tag.text); - return binding?.kind === "kit" ? binding.kit : undefined; - } - if (ts.isPropertyAccessExpression(tag) && ts.isIdentifier(tag.expression)) { - if (shadowed.has(tag.expression.text)) return undefined; - const binding = bindings.get(tag.expression.text); - if (binding?.kind !== "namespace") return undefined; - const member = tag.name.text; - // Through a surface namespace the member is that file's published - // name, so it resolves to the kit name behind it. - return binding.surface ? surface.get(member) : member; - } - return undefined; -} +): Set[] { + const body = functionBodyOf(declaration); + if (!body) return []; -/** Every name declared inside `node` — the ones that shadow an import. */ -function locallyBound(node: ts.Node): Set { - const names = new Set(); - const visit = (child: ts.Node) => { - if ( - (ts.isVariableDeclaration(child) || - ts.isFunctionDeclaration(child) || - ts.isClassDeclaration(child) || - ts.isParameter(child)) && - child.name && - ts.isIdentifier(child.name) - ) { - names.add(child.name.text); + const marksIn = (root: ts.Node): Set => { + const found = new Set(); + const visit = (node: ts.Node) => { + // A nested function is not what this component returns. + if (isFunctionLike(node) && node !== root) return; + if (ts.isJsxOpeningElement(node) || ts.isJsxSelfClosingElement(node)) { + const kit = kitComponentOf(node.tagName, checker, surface); + if (kit && !kit.startsWith("*")) found.add(kit); + } + ts.forEachChild(node, visit); + }; + visit(root); + return found; + }; + + // Each return path separately: every `return` in this body (not in a + // nested function), and each arm of a conditional inside one. + const paths: Set[] = []; + const addExpression = (expression: ts.Expression) => { + if (ts.isConditionalExpression(expression)) { + addExpression(expression.whenTrue); + addExpression(expression.whenFalse); + return; + } + if (ts.isParenthesizedExpression(expression)) { + addExpression(expression.expression); + return; } - ts.forEachChild(child, visit); + paths.push(marksIn(expression)); }; - ts.forEachChild(node, visit); - return names; + if (!ts.isBlock(body)) { + // A concise arrow body is the returned expression. + addExpression(body as ts.Expression); + return paths; + } + const visit = (node: ts.Node) => { + if (isFunctionLike(node) && node !== body) return; + if (ts.isReturnStatement(node) && node.expression) { + addExpression(node.expression); + return; + } + ts.forEachChild(node, visit); + }; + visit(body); + return paths; } -/** The kit components this declaration RENDERS, by kit name. */ -function renderedWithin( - node: ts.Node, - bindings: Map, +function isFunctionLike(node: ts.Node): boolean { + return ( + ts.isFunctionDeclaration(node) || + ts.isFunctionExpression(node) || + ts.isArrowFunction(node) || + ts.isMethodDeclaration(node) + ); +} + +/** The body of the function a declaration is, if it is one. */ +function functionBodyOf(declaration: ts.Node): ts.Node | undefined { + if (ts.isFunctionDeclaration(declaration)) return declaration.body; + if ( + ts.isVariableDeclaration(declaration) && + declaration.initializer && + isFunctionLike(declaration.initializer) + ) { + return (declaration.initializer as ts.ArrowFunction).body; + } + if (isFunctionLike(declaration)) { + return (declaration as ts.ArrowFunction).body; + } + return undefined; +} + +/** Every kit component this declaration returns, flattened. */ +function returnedKitComponents( + declaration: ts.Node, + checker: ts.TypeChecker, surface: Map, ): Set { - const shadowed = locallyBound(node); - const rendered = new Set(); - const visit = (child: ts.Node) => { - if (ts.isJsxOpeningElement(child) || ts.isJsxSelfClosingElement(child)) { - const kit = renderedKitComponent( - child.tagName, - bindings, - surface, - shadowed, - ); - if (kit) rendered.add(kit); - } - ts.forEachChild(child, visit); - }; - ts.forEachChild(node, visit); - return rendered; + return new Set( + returnedPaths(declaration, checker, surface).flatMap((path) => [...path]), + ); } /** - * Every component this file declares, by the name it is PUBLISHED under, with - * the declaration's own node. - * - * The published name is not always the declared one: `const LocalPanel = …; - * export { LocalPanel as Card }` ships a `Card`, and reading only declaration - * names let that straight through. + * Every component a file publishes, by published name, with the declaration + * that implements it — following an export chain of any length into whichever + * file actually declares it, so splitting a copy from its kit-named export + * hides nothing and a genuine wrapper re-exported under the kit's name is not + * mistaken for one. */ -function declarations(file: ts.SourceFile): Map { - const declared = new Map(); +function publishedComponents( + file: ts.SourceFile, + checker: ts.TypeChecker, +): Map { + const published = new Map(); + const implementation = ( + symbol: ts.Symbol | undefined, + ): ts.Node | undefined => { + let resolved = symbol; + for (let hop = 0; resolved && hop < 8; hop += 1) { + const declaration = resolved.declarations?.[0]; + if (!declaration) return undefined; + if ( + ts.isFunctionDeclaration(declaration) || + ts.isClassDeclaration(declaration) || + ts.isVariableDeclaration(declaration) + ) { + return declaration; + } + if (!(resolved.flags & ts.SymbolFlags.Alias)) return undefined; + resolved = checker.getImmediateAliasedSymbol(resolved); + } + return undefined; + }; + + const moduleSymbol = checker.getSymbolAtLocation(file); + if (moduleSymbol) { + for (const exported of checker.getExportsOfModule(moduleSymbol)) { + if (!/^[A-Z][A-Za-z0-9]*$/.test(exported.name)) continue; + const node = implementation(exported); + if (node) published.set(exported.name, node); + } + } + // Declarations the file does not export are still declarations here. const visit = (node: ts.Node) => { - if ( + const name = (ts.isFunctionDeclaration(node) || ts.isClassDeclaration(node)) && node.name - ) { - declared.set(node.name.text, node); - } else if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name)) { - declared.set(node.name.text, node); + ? node.name.text + : ts.isVariableDeclaration(node) && ts.isIdentifier(node.name) + ? node.name.text + : undefined; + if (name && /^[A-Z][A-Za-z0-9]*$/.test(name) && !published.has(name)) { + published.set(name, node); } ts.forEachChild(node, visit); }; visit(file); - - const all = new Map(); - for (const [name, node] of declared) { - if (/^[A-Z][A-Za-z0-9]*$/.test(name)) all.set(name, node); - } - // `export { Local as Published }` with no `from`. - for (const statement of file.statements) { - if (!ts.isExportDeclaration(statement) || statement.moduleSpecifier) { - continue; - } - const clause = statement.exportClause; - if (!clause || !ts.isNamedExports(clause)) continue; - for (const element of clause.elements) { - const local = (element.propertyName ?? element.name).text; - const node = declared.get(local); - const published = element.name.text; - if (node && /^[A-Z][A-Za-z0-9]*$/.test(published)) { - all.set(published, node); - } - } - } - return all; + return published; } /** @@ -430,65 +554,29 @@ const DASHBOARD_MARKS = new Set([ ]); /** - * Whether this declaration picks between dashboard marks by branch: two - * different marks reachable from alternative arms of one conditional, if, or - * switch. + * Whether this declaration chooses between dashboard marks — two return paths + * that render DIFFERENT sets of them. + * + * Differing is the test, not branching and not counting. A layout that returns + * the same pair inside a `
    ` or a `
    ` has two paths and one + * picture, and must pass; a component that returns a line for one kind and an + * area for another has two pictures, and is the dispatch `` owns. + * Both the `cond ? … : …` and the guard-return spellings are return paths, so + * neither needs its own rule. */ -function dispatchesOverMarks( - node: ts.Node, - bindings: Map, +function dispatchedMarks( + declaration: ts.Node, + checker: ts.TypeChecker, surface: Map, ): string[] { - const marksOf = (branch: ts.Node | undefined): Set => { - if (!branch) return new Set(); - // A branch is often the element itself (`cond ? : `), and - // `renderedWithin` walks a node's children rather than the node, so - // the branch's own tag is read here. - const rendered = renderedWithin(branch, bindings, surface); - if (ts.isJsxSelfClosingElement(branch) || ts.isJsxOpeningElement(branch)) { - const kit = renderedKitComponent( - branch.tagName, - bindings, - surface, - locallyBound(branch), - ); - if (kit) rendered.add(kit); - } - if (ts.isJsxElement(branch)) { - const kit = renderedKitComponent( - branch.openingElement.tagName, - bindings, - surface, - locallyBound(branch), - ); - if (kit) rendered.add(kit); - } - return new Set([...rendered].filter((mark) => DASHBOARD_MARKS.has(mark))); - }; - const found = new Set(); - const consider = (branches: (ts.Node | undefined)[]) => { - const perBranch = branches.map(marksOf); - const union = new Set(perBranch.flatMap((marks) => [...marks])); - // Two distinct marks, reached through different arms. - if ( - union.size > 1 && - perBranch.filter((marks) => marks.size > 0).length > 1 - ) { - for (const mark of union) found.add(mark); - } - }; - const visit = (child: ts.Node) => { - if (ts.isConditionalExpression(child)) { - consider([child.whenTrue, child.whenFalse]); - } else if (ts.isIfStatement(child)) { - consider([child.thenStatement, child.elseStatement]); - } else if (ts.isSwitchStatement(child)) { - consider([...child.caseBlock.clauses]); - } - ts.forEachChild(child, visit); - }; - visit(node); - return [...found].sort(); + const paths = returnedPaths(declaration, checker, surface) + .map( + (path) => new Set([...path].filter((mark) => DASHBOARD_MARKS.has(mark))), + ) + .filter((path) => path.size > 0); + const shapes = new Set(paths.map((path) => [...path].sort().join("+"))); + if (shapes.size < 2) return []; + return [...new Set(paths.flatMap((path) => [...path]))].sort(); } const DISPATCHERS: Record = { @@ -587,99 +675,46 @@ interface Offence { const ROUTE_CONVENTION = /^src\/app\/(?:.*\/)?(page|layout|template|default|error|global-error|loading|not-found|forbidden|unauthorized)\.tsx?$/; -/** - * The names a file's default export resolves to, in every form a route - * component is written: `export default function Page`, `export default Page`, - * and `export { Page as default }`. Resolving only the first rejected two - * conventional pages. - */ -function defaultExportNames(file: ts.SourceFile): Set { +/** What a file's default export resolves to, in every spelling of it. */ +function defaultExportNames( + file: ts.SourceFile, + checker: ts.TypeChecker, +): Set { const names = new Set(); - for (const statement of file.statements) { - if (ts.isExportAssignment(statement)) { - if (ts.isIdentifier(statement.expression)) { - names.add(statement.expression.text); + const moduleSymbol = checker.getSymbolAtLocation(file); + const exported = moduleSymbol + ? checker + .getExportsOfModule(moduleSymbol) + .find((symbol) => symbol.name === "default") + : undefined; + let symbol = exported; + for (let hop = 0; symbol && hop < 8; hop += 1) { + for (const declaration of symbol.declarations ?? []) { + if ( + (ts.isFunctionDeclaration(declaration) || + ts.isClassDeclaration(declaration) || + ts.isVariableDeclaration(declaration)) && + declaration.name && + ts.isIdentifier(declaration.name) + ) { + names.add(declaration.name.text); } - continue; - } - if (ts.isExportDeclaration(statement) && !statement.moduleSpecifier) { - const clause = statement.exportClause; - if (clause && ts.isNamedExports(clause)) { - for (const element of clause.elements) { - if (element.name.text === "default" && element.propertyName) { - names.add(element.propertyName.text); - } - } + if ( + ts.isExportAssignment(declaration) && + ts.isIdentifier(declaration.expression) + ) { + names.add(declaration.expression.text); } - continue; - } - const isDefault = ts - .getModifiers(statement as ts.HasModifiers) - ?.some((modifier) => modifier.kind === ts.SyntaxKind.DefaultKeyword); - if (!isDefault) continue; - if ( - (ts.isFunctionDeclaration(statement) || - ts.isClassDeclaration(statement)) && - statement.name - ) { - names.add(statement.name.text); } + if (!(symbol.flags & ts.SymbolFlags.Alias)) break; + symbol = checker.getImmediateAliasedSymbol(symbol); } return names; } /** - * Names a file publishes that are declared in ANOTHER file of the set: - * `export { LocalPanel as Card } from "./local"`. Without this, splitting the - * copy and its kit-named export across two files hid it from both. - */ -function reExportedLocally( - files: { path: string; source: string }[], -): { name: string; path: string }[] { - const declaredIn = new Map>(); - for (const { path, source } of files) { - declaredIn.set(path, new Set(declarations(parse(path, source)).keys())); - } - const published: { name: string; path: string }[] = []; - for (const { path, source } of files) { - const file = parse(path, source); - for (const statement of file.statements) { - if (!ts.isExportDeclaration(statement) || !statement.moduleSpecifier) { - continue; - } - if (!ts.isStringLiteral(statement.moduleSpecifier)) continue; - const specifier = statement.moduleSpecifier.text; - if (!specifier.startsWith(".")) continue; - const target = join(dirname(path), specifier).replaceAll("\\", "/"); - const names = [...declaredIn.entries()].find( - ([other]) => - other === target || - other.startsWith(`${target}.`) || - other.startsWith(`${target}/`), - )?.[1]; - if (!names) continue; - const clause = statement.exportClause; - if (!clause || !ts.isNamedExports(clause)) continue; - for (const element of clause.elements) { - const local = (element.propertyName ?? element.name).text; - const name = element.name.text; - // Only an ALIASING re-export publishes a name the declaration - // does not already carry. `export { Dashboard } from "./ui"` is - // a barrel: that declaration was judged at its own site, and - // flagging it again would demand a second exception for one - // component. `export { LocalPanel as Card }` is the other - // thing entirely — a kit name appearing where no declaration - // answers for it. - if (local !== name && names.has(local)) published.push({ name, path }); - } - } - } - return published; -} - -/** - * Declarations that take a kit export's name without RENDERING the kit's - * component of that name, plus declarations that branch between the kit's + * Declarations that take a kit export's name without RETURNING the kit's + * component of that name, plus declarations that choose between the kit's * dashboard marks. Pure, so the fixtures below can exercise it on sources that * are not in the tree. */ @@ -691,40 +726,40 @@ function findOffences( surface: Map = new Map(), dispatchers: Record = DISPATCHERS, ): Offence[] { + const { checker, sources } = programOver(files); const offences: Offence[] = []; - const scanned = files.filter(({ path }) => !KIT_SURFACES.includes(path)); - for (const { path, source } of scanned) { - const file = parse(path, source); - const bindings = kitBindings(file, path, surface); + for (const [path, file] of sources) { + if (KIT_SURFACES.includes(path)) continue; const routeDefaults = ROUTE_CONVENTION.test(path) - ? defaultExportNames(file) + ? defaultExportNames(file, checker) : new Set(); + const components = publishedComponents(file, checker); - for (const [name, node] of declarations(file)) { + for (const [name, declaration] of components) { if (!kit.has(name) || routeDefaults.has(name)) continue; - if (renderedWithin(node, bindings, surface).has(name)) continue; + if (returnedKitComponents(declaration, checker, surface).has(name)) { + continue; + } + // An exception is recorded where the component is DECLARED, so a + // barrel that re-exports it does not need a second one for the + // same component. The published path is checked too, for a name a + // file publishes without declaring anywhere in the set. + const declaredIn = declaration.getSourceFile().fileName; if (`${name} in ${path}` in allowed) continue; + if (`${name} in ${declaredIn}` in allowed) continue; offences.push({ name, path, kind: "name" }); } - if (!(path in dispatchers)) { - for (const [, node] of declarations(file)) { - const marks = dispatchesOverMarks(node, bindings, surface); - if (marks.length > 1) { - offences.push({ name: marks.join("+"), path, kind: "dispatch" }); - break; - } + if (path in dispatchers) continue; + for (const [, declaration] of components) { + const marks = dispatchedMarks(declaration, checker, surface); + if (marks.length > 1) { + offences.push({ name: marks.join("+"), path, kind: "dispatch" }); + break; } } } - - for (const { name, path } of reExportedLocally(scanned)) { - if (!kit.has(name)) continue; - if (`${name} in ${path}` in allowed) continue; - if (offences.some((o) => o.name === name && o.path === path)) continue; - offences.push({ name, path, kind: "name" }); - } return offences; } @@ -1202,6 +1237,137 @@ export const Trend = () => ;`, ).toEqual([]); }); + it.each([ + [ + "a guard-return dispatch", + `import { LineChart as L, AreaChart as A } from "@codefly-dev/ui/dashboard"; +export function WidgetBody({ kind }) { + if (kind === "line") return ; + return ; +}`, + true, + ], + [ + "a destructured parameter shadowing the import", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card({ KitCard }) { return ; }`, + true, + ], + [ + "an unused nested wrapper beside a plain return", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card() { + const Unused = () => ; + return
    copy
    ; +}`, + true, + ], + [ + "a function-local alias of the kit component", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card(props) { + const Local = KitCard; + return ; +}`, + false, + ], + [ + "an alias of a namespace member", + `import * as UI from "@codefly-dev/ui/layout"; +const Local = UI.Card; +export const Card = (props) => ;`, + false, + ], + [ + "a nested shadow that does not reach the outer JSX", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card(props) { + const helper = (KitCard) => KitCard; + return ; +}`, + false, + ], + [ + "two layout branches drawing the same pair of marks", + `import { KPIRow, MetricLineChart } from "@codefly-dev/ui/dashboard"; +export function Overview({ compact }) { + return compact ? ( +
    + ) : ( +
    + ); +}`, + false, + ], + ])("%s", (_name, source, rejected) => { + expect(detect("src/features/x/ui/example.tsx", source).length > 0).toBe( + rejected, + ); + }); + + it("does not count kit JSX inside a callback the component passes down", () => { + // A render prop is not what this component returns, and the skip that + // keeps a nested function out of a returned expression is what says so. + expect( + detect( + "src/features/x/ui/card.tsx", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card() { + return } />; +}`, + ), + ).toEqual(["Card in src/features/x/ui/card.tsx"]); + }); + + it("accepts a genuine wrapper re-exported under the kit's name", () => { + // Its implementation composes the kit, which is the question — the name + // it is published under is not evidence either way. + expect( + findOffences( + [ + { + path: "src/features/x/ui/local.tsx", + source: `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export const LocalPanel = (props) => ;`, + }, + { + path: "src/features/x/ui/index.ts", + source: 'export { LocalPanel as Card } from "./local";', + }, + ], + kit, + {}, + surface, + {}, + ), + ).toEqual([]); + }); + + it("catches a copy published under a kit name two hops away", () => { + expect( + findOffences( + [ + { + path: "src/features/x/ui/local.tsx", + source: "export const LocalPanel = () =>
    copy
    ;", + }, + { + path: "src/features/x/ui/bridge.ts", + source: 'export { LocalPanel } from "./local";', + }, + { + path: "src/features/x/ui/index.ts", + source: 'export { LocalPanel as Card } from "./bridge";', + }, + ], + kit, + {}, + surface, + {}, + ).length, + ).toBeGreaterThan(0); + }); + it("ignores a name the kit does not export", () => { expect( detect( diff --git a/module/services/frontend/code/src/solutions/SolutionDashboard.tsx b/module/services/frontend/code/src/solutions/SolutionDashboard.tsx index 8cc177cb2..f7ec7c040 100644 --- a/module/services/frontend/code/src/solutions/SolutionDashboard.tsx +++ b/module/services/frontend/code/src/solutions/SolutionDashboard.tsx @@ -159,28 +159,40 @@ function SolutionWidget({ // from the dashboard instead of from a table. A transient failure says // nothing about authority, so those values stay and read as stale. // - // Hiding the snapshot for the refusal alone is not enough, and that was - // this component's bug: the cached snapshot survived the refusal, so the - // NEXT error — a 503, a dropped connection — made it eligible again and - // handed the refused values straight back, with no authorized read in - // between. A refusal therefore latches: the snapshot stays ineligible - // until a successful read replaces it. + // A refusal DELETES the snapshots rather than marking them ineligible. + // Two earlier attempts marked instead, and both leaked: hiding the values + // for the refusal alone let the next transient error hand them back, and a + // latch beside them in the cache was a second entry with its own lifetime + // — garbage-collected on its own schedule, and cleared by a cached success + // for another revision of the graph, neither of which is a fresh grant. + // Nothing restorable is left here, so there is no second thing to keep in + // step: only a later successful read can put values back. const outcome = readOutcome(isError, error); - const authorized = !isError && data !== undefined; - // The latch lives in the query cache, beside the snapshot it guards, for - // two reasons: it outlives this component (a latch in state or a ref - // forgets on remount, and the snapshot does not), and it expires with the - // cache that holds the values, so neither can outlast the other. - const latchKey = refusedLatchKey(solutionId, dashboardId, widget.id, orgId); - const latched = client.getQueryData(latchKey) === true; const widgetId = widget.id; useEffect(() => { - const key = refusedLatchKey(solutionId, dashboardId, widgetId, orgId); - if (outcome === "forbidden") client.setQueryData(key, true); - else if (authorized) client.setQueryData(key, false); - }, [client, outcome, authorized, solutionId, dashboardId, widgetId, orgId]); + if (outcome !== "forbidden") return; + // Every cached revision of this widget's series, not just the one this + // render resolved: the graph is the last part of the query key, so a + // dashboard whose declaration has changed since — a different title is + // enough — has its own entry holding the same refused numbers. + // + // Reset, not remove: removing destroys the query this component is + // observing, which leaves it with nothing to refetch and no way back + // even once authority returns. A reset discards the data and keeps the + // query, so the only route to values on screen is a read that succeeds. + client.resetQueries({ + queryKey: ["solution-widget", solutionId, dashboardId, widgetId, orgId], + exact: false, + }); + }, [client, outcome, solutionId, dashboardId, widgetId, orgId]); - const refused = outcome === "forbidden" || (!authorized && latched); + const refused = outcome === "forbidden"; + // `!refused` covers the one render between the refusal arriving and the + // effect above clearing the cache, where `data` is still the old success. + // No test distinguishes it, and that is worth saying rather than implying: + // the renderer answers a refusal before it reads the series, so nothing + // reaches the screen through this window either way. It is the host's half + // of one rule stated in two places, not the mechanism. const retained = !refused && mayKeepRetainedRows(outcome) ? (data?.series ?? null) : null; @@ -196,19 +208,6 @@ function SolutionWidget({ ); } -// Where a widget's refusal latch is kept in the query cache. Not a read: it -// holds a boolean this component writes, under its own prefix so it can never -// be mistaken for a resolved widget. Built by a function rather than inline so -// the effect below depends on its parts and not on a fresh array each render. -function refusedLatchKey( - solutionId: string, - dashboardId: string, - widgetId: string, - orgId: string, -): readonly string[] { - return ["solution-widget-refused", solutionId, dashboardId, widgetId, orgId]; -} - // The host's read classification as the renderer's presentational one. A read // that succeeded is no failure at all, which is what `undefined` says. const WIDGET_FAILURE: Record = { diff --git a/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx b/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx index d9faf28bc..a26c53560 100644 --- a/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx +++ b/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx @@ -328,6 +328,141 @@ describe("SolutionDashboards", () => { }, ); + it("does not resurrect them once the caches have had time to expire", async () => { + // The shape this replaced marked the snapshot ineligible from a second + // cache entry, which had its own garbage-collection clock: five + // minutes later the mark was gone and the values were not, so a retry + // handed them back. Deleting the snapshot leaves nothing with a + // lifetime of its own to fall out of step. + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ buckets: [{ key: "2026-09-01", count: "37" }] }), + ), + ); + const { client } = renderDashboards(); + expect(await screen.findAllByText("37")).not.toHaveLength(0); + + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ code: "permission_denied" }, { status: 403 }), + ), + ); + await act(async () => { + await client.refetchQueries({ queryKey: ["solution-widget"] }); + await settle(); + }); + expect(await screen.findAllByText("No access")).not.toHaveLength(0); + + // Past the default five-minute gcTime, on the cache's own clock. + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + try { + await act(async () => { + await vi.advanceTimersByTimeAsync(300_001); + }); + } finally { + vi.useRealTimers(); + } + + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ code: "unavailable" }, { status: 503 }), + ), + ); + await act(async () => { + await client.refetchQueries({ queryKey: ["solution-widget"] }); + await settle(); + }); + expect(screen.queryAllByText("37")).toHaveLength(0); + }); + + it("does not resurrect them on a remount", async () => { + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ buckets: [{ key: "2026-09-01", count: "37" }] }), + ), + ); + const { client, unmount } = renderDashboards(); + expect(await screen.findAllByText("37")).not.toHaveLength(0); + + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ code: "permission_denied" }, { status: 403 }), + ), + ); + await act(async () => { + await client.refetchQueries({ queryKey: ["solution-widget"] }); + await settle(); + }); + expect(await screen.findAllByText("No access")).not.toHaveLength(0); + + unmount(); + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ code: "unavailable" }, { status: 503 }), + ), + ); + renderInApp( + , + client, + ); + await act(async () => { + await settle(); + }); + expect(screen.queryAllByText("37")).toHaveLength(0); + }); + + it("does not leave the same numbers behind under another graph revision", async () => { + // The graph is the last part of the query key, so a declaration that + // differs only in a dashboard title has its own cache entry holding + // the same numbers. A refusal has to reach every revision of them. + const renamed: DataGraph = { + ...graph, + dashboards: [{ ...graph.dashboards[0], title: "Activity (renamed)" }], + }; + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ buckets: [{ key: "2026-09-01", count: "37" }] }), + ), + ); + const { client, unmount } = renderDashboards(); + expect(await screen.findAllByText("37")).not.toHaveLength(0); + unmount(); + + // A second revision, cached under its own key. + const second = renderInApp( + , + client, + ); + expect(await screen.findAllByText("37")).not.toHaveLength(0); + + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ code: "permission_denied" }, { status: 403 }), + ), + ); + await act(async () => { + await client.refetchQueries({ queryKey: ["solution-widget"] }); + await settle(); + }); + expect(await screen.findAllByText("No access")).not.toHaveLength(0); + second.unmount(); + + // Back to the first revision, whose refresh fails transiently. + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ code: "unavailable" }, { status: 503 }), + ), + ); + renderInApp( + , + client, + ); + await act(async () => { + await settle(); + }); + expect(screen.queryAllByText("37")).toHaveLength(0); + }); + it("lets a successful authorized read lift the refusal", async () => { server.use( http.post(rpc("AuditService", "AggregateAuditLog"), () => diff --git a/module/services/frontend/code/src/test/container.tsx b/module/services/frontend/code/src/test/container.tsx index af7459594..c7caa3e83 100644 --- a/module/services/frontend/code/src/test/container.tsx +++ b/module/services/frontend/code/src/test/container.tsx @@ -10,10 +10,17 @@ export function rpc(service: string, method: string): string { // Render an admin feature container with the single provider its data hooks // need. retry is off so a rejected ancillary query surfaces at once rather // than stalling the test. -export function renderInApp(ui: React.ReactElement) { - const client = new QueryClient({ - defaultOptions: { queries: { retry: false } }, - }); +export function renderInApp(ui: React.ReactElement, reuse?: QueryClient) { + // `reuse` mounts into an EXISTING cache, which is what a remount or a + // second revision of the same declaration actually is: the component goes + // away and the cached answers do not. A test that built a fresh client + // could not tell a snapshot that was deleted from one that was never + // there. + const client = + reuse ?? + new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); // The client comes back alongside the render result so a test can drive a // SECOND read of the same query — the success-then-refused sequence, which is // the one that proves a denial is not hidden behind rows already on screen. From 9fe2b363713f1ac76f1d5cdf3efed61ed8385164 Mon Sep 17 00:00:00 2001 From: Codefly Automation <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:16:42 -0400 Subject: [PATCH 6/6] fix(dashboard): the read deletes what a refusal may not keep, and the gate refuses what it cannot read (#1041) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A refused read is the one thing that happens exactly once per refusal, whoever is watching. Moving the deletion of the retained snapshots into the query function — where `readOutcome` classifies the failure — is what makes the rule hold for a refusal that arrives after the viewer switched dashboards or after the page unmounted, neither of which runs an effect. The values go for every cached revision of that widget's source; the query, its error and its retry policy are untouched, so a standing 403 settles in one request instead of restarting itself. The duplication gate stops modelling more of JavaScript and starts refusing what it cannot resolve. A kit name published from a module it cannot read is an offence rather than a skip, export chains are followed to their end with cycle detection instead of a hop cap, the app's own `@/` alias resolves, and what a component RETURNS is read by expanding the alternatives of its returned expression — conditionals wherever they sit, casts and parentheses unwrapped, a comma's right operand alone, unreachable returns dropped, attributes not counted. A dispatch is two alternatives whose marks are each non-empty and neither contained in the other. Its limits are now written in its doc comment. The kit moves to 0.12.7: main's release cut v0.0.94 while this branch was open, which published 0.12.6, and these bytes moved. Additive is a patch — a 0.x minor breaks every remote pinned to ^0.12. Co-Authored-By: Claude Opus 5 (1M context) --- module/AUDIT_METRICS.md | 8 + module/services/frontend/AGENTS.md | 15 + .../services/frontend/code/package-lock.json | 8 +- module/services/frontend/code/package.json | 4 +- .../code/packages/codefly-ui/package.json | 2 +- .../code/packages/saas-ui/package.json | 2 +- .../code/src/components/__tests__/kit.test.ts | 872 ++++++++++++++---- .../code/src/solutions/SolutionDashboard.tsx | 147 +-- .../__tests__/SolutionDashboard.test.tsx | 178 +++- .../frontend/code/src/test/container.tsx | 14 +- 10 files changed, 989 insertions(+), 261 deletions(-) diff --git a/module/AUDIT_METRICS.md b/module/AUDIT_METRICS.md index 1e0b9984d..02fdd4225 100644 --- a/module/AUDIT_METRICS.md +++ b/module/AUDIT_METRICS.md @@ -253,6 +253,14 @@ that series and carries a **Stale** badge instead, because values that are old are still better than a blank card. Only a failure with nothing behind it reads as "Unable to load." +A widget whose resolution was **refused** is the other case, and it keeps +nothing. "No access" is drawn before the series is read, and the host that +resolved the widget deletes the values it had cached — every revision of that +widget's declaration, at the read that was refused rather than in the component +displaying it, so a refusal arriving after the viewer moved on still lands. +Numbers this viewer is no longer allowed to read are a wrong answer, not a stale +one, and only a read that succeeds puts any back. + A host composes its own features around that renderer — ``'s slots take header actions, a layout (the host's solution dashboard passes a drag-to-reorder grid), a per-widget renderer for a host that resolves each diff --git a/module/services/frontend/AGENTS.md b/module/services/frontend/AGENTS.md index bf0ba54e6..0a257e52b 100644 --- a/module/services/frontend/AGENTS.md +++ b/module/services/frontend/AGENTS.md @@ -252,3 +252,18 @@ failures part company here, which is what `mayKeepRetainedRows` and those very rows, so they go; a **transient failure** is not, so they stay with a note that they may be out of date. Blanking a good table because one request did not come back is worse than the staleness. + +**Where a refusal acts is the read, not the component.** Withholding rows at +render answers for the error in hand and leaves the rows in the cache, so the +next transient failure serves them again — authority is a property of the read, +and so is what the read may leave behind. A refused read therefore deletes the +retained snapshots for every cached key of what it was reading, in the query +function where `readOutcome` classifies it, before the rejection reaches any +component. `src/solutions/SolutionDashboard.tsx` is the worked example: one +widget, several cached revisions of its declaration, and a refusal that may +arrive after the viewer has navigated away or the page has unmounted, where no +effect of theirs is left to run. Delete the data and nothing else: removing the +entry cancels the read reporting the refusal and strands recovery, clearing it +restarts every active read of it so a standing 403 never settles, and +`setQueryData(key, undefined)` is a documented no-op. Recovery is then exactly +one thing — a read that succeeds. diff --git a/module/services/frontend/code/package-lock.json b/module/services/frontend/code/package-lock.json index 3af8fa84e..1cc6a8c4d 100644 --- a/module/services/frontend/code/package-lock.json +++ b/module/services/frontend/code/package-lock.json @@ -16,8 +16,8 @@ "@codefly-dev/saas-plugin-contract": "2.4.2", "@codefly-dev/saas-plugin-react": "0.4.3", "@codefly-dev/saas-sdk": "0.3.23", - "@codefly-dev/saas-ui": "0.12.6", - "@codefly-dev/ui": "0.12.6", + "@codefly-dev/saas-ui": "0.12.7", + "@codefly-dev/ui": "0.12.7", "@codefly/saas-plugin-manifest": "0.1.1", "@codefly/saas-settings": "0.1.0", "@connectrpc/connect": "^2.1.1", @@ -15233,7 +15233,7 @@ }, "packages/codefly-ui": { "name": "@codefly-dev/ui", - "version": "0.12.6", + "version": "0.12.7", "license": "Apache-2.0", "dependencies": { "@base-ui/react": "^1.3.0", @@ -15381,7 +15381,7 @@ }, "packages/saas-ui": { "name": "@codefly-dev/saas-ui", - "version": "0.12.6", + "version": "0.12.7", "license": "Apache-2.0", "devDependencies": { "@bufbuild/protobuf": "^2.11.0", diff --git a/module/services/frontend/code/package.json b/module/services/frontend/code/package.json index fbebc2da1..ca15dbe96 100644 --- a/module/services/frontend/code/package.json +++ b/module/services/frontend/code/package.json @@ -39,8 +39,8 @@ "@codefly-dev/saas-plugin-contract": "2.4.2", "@codefly-dev/saas-plugin-react": "0.4.3", "@codefly-dev/saas-sdk": "0.3.23", - "@codefly-dev/saas-ui": "0.12.6", - "@codefly-dev/ui": "0.12.6", + "@codefly-dev/saas-ui": "0.12.7", + "@codefly-dev/ui": "0.12.7", "@codefly/saas-plugin-manifest": "0.1.1", "@codefly/saas-settings": "0.1.0", "@connectrpc/connect": "^2.1.1", diff --git a/module/services/frontend/code/packages/codefly-ui/package.json b/module/services/frontend/code/packages/codefly-ui/package.json index 18f4fc37c..056f89478 100644 --- a/module/services/frontend/code/packages/codefly-ui/package.json +++ b/module/services/frontend/code/packages/codefly-ui/package.json @@ -1,6 +1,6 @@ { "name": "@codefly-dev/ui", - "version": "0.12.6", + "version": "0.12.7", "description": "The shared, versioned Codefly SaaS frontend kit: the plugin host and the tokens-as-data skin mechanism, standalone so the host and Module-Federation remotes import and dedupe one copy.", "repository": { "type": "git", diff --git a/module/services/frontend/code/packages/saas-ui/package.json b/module/services/frontend/code/packages/saas-ui/package.json index 505377c0c..d62870fe0 100644 --- a/module/services/frontend/code/packages/saas-ui/package.json +++ b/module/services/frontend/code/packages/saas-ui/package.json @@ -1,6 +1,6 @@ { "name": "@codefly-dev/saas-ui", - "version": "0.12.6", + "version": "0.12.7", "description": "Reusable SaaS-domain frontend components (datasources, …) the portal and solutions both import, built on @codefly-dev/ui primitives and driven by an injected typed client.", "repository": { "type": "git", diff --git a/module/services/frontend/code/src/components/__tests__/kit.test.ts b/module/services/frontend/code/src/components/__tests__/kit.test.ts index cd91e340c..755cc806a 100644 --- a/module/services/frontend/code/src/components/__tests__/kit.test.ts +++ b/module/services/frontend/code/src/components/__tests__/kit.test.ts @@ -37,11 +37,34 @@ import packageJson from "../../../package.json" with { type: "json" }; * `export … from "@codefly-dev/ui/dashboard"`. Issue #1041 asked what they * were; they are already right, and this keeps them so. * - * What the gate cannot do, said plainly: a duplicate under a NEW name, built by - * hand, passes all three legs. {@link ALLOWED} is the backstop for that — a - * label a reviewer reads and asks about, not a detector. Classifying files by - * their names instead would fail on the next file anyone adds, and a gate that - * cries wolf is a gate that gets deleted. + * It resolves names with the TypeScript checker, so every spelling that reaches + * the same component reaches the same verdict, and it reads what a component + * RETURNS rather than what a file mentions. Where it cannot resolve a name the + * kit owns — published from a third-party module, or from a chain whose end it + * cannot read — it refuses rather than passing: "could not tell" is not a + * verdict it is allowed to reach, and four rounds of adversarial fixtures each + * found the next way through the version that skipped instead. + * + * WHAT IT CANNOT SEE. This is a tripwire for the common shape, not a proof, and + * these are limits rather than bugs — each is a place where deciding would mean + * following the program further than reading one component's returned + * expression: + * + * - **A duplicate under a NEW name**, built by hand, passes every leg. + * {@link ALLOWED} and {@link DISPATCHERS} are the backstop — labels a + * reviewer reads and asks about, not detectors. + * - **Kit JSX handed to another component through an attribute.** What the + * recipient does with a prop is beyond this; a kit element in one is no + * evidence that this component renders it. Children are not attributes and + * do count. + * - **Anything behind a call.** A mark rendered by a helper function, or + * chosen from a lookup table (`MARKS[kind]`), is a dispatch this cannot + * read. The table is plain to a reviewer and invisible here. + * - **A dispatch split across components**, one visualization each, composed + * by a parent that only forwards. + * + * Classifying files by their names instead would fail on the next file anyone + * adds, and a gate that cries wolf is a gate that gets deleted. */ const here = dirname(fileURLToPath(import.meta.url)); @@ -238,8 +261,14 @@ function programOver(files: { path: string; source: string }[]): { getNewLine: () => "\n", resolveModuleNames: (names, containingFile) => names.map((name) => { - if (!name.startsWith(".")) return undefined; - const base = join(dirname(containingFile), name).replaceAll("\\", "/"); + // The app's own two spellings of its own modules: a relative path + // and the `@/` alias its tsconfig maps to `src/`. Resolving only + // the first let a copy hide behind an export chain written in the + // second — the gate read the barrel and then stopped. + if (!name.startsWith(".") && !name.startsWith("@/")) return undefined; + const base = name.startsWith("@/") + ? `src/${name.slice(2)}` + : join(dirname(containingFile), name).replaceAll("\\", "/"); for (const candidate of [ base, `${base}.tsx`, @@ -274,23 +303,120 @@ function programOver(files: { path: string; source: string }[]): { return { program, checker: program.getTypeChecker(), sources }; } -/** The import declaration a symbol's declaration sits under, if any. */ -function importOf(declaration: ts.Node): ts.ImportDeclaration | undefined { +const KIT_NAMESPACE = "*kit"; +const SURFACE_NAMESPACE = "*surface"; + +/** + * An alias chain from a symbol to its end, every hop included. + * + * Cycle-detected, not hop-capped. A chain that is long is not a chain that is + * suspicious, and abandoning one at an arbitrary depth was itself an escape: + * nine re-exports in a row resolved to nothing and the copy at the end of them + * went unexamined. A cycle is the only thing that can stop this, and it stops + * it without a verdict. + */ +function aliasChain(symbol: ts.Symbol, checker: ts.TypeChecker): ts.Symbol[] { + const chain = [symbol]; + const seen = new Set([symbol]); + let current = symbol; + while (current.flags & ts.SymbolFlags.Alias) { + const next = checker.getImmediateAliasedSymbol(current); + if (!next || seen.has(next)) break; + seen.add(next); + chain.push(next); + current = next; + } + return chain; +} + +/** + * Where a declaration comes from, when it comes from another module. + * + * `kit` is the kit's own component under the kit's own name, however the local + * spelling renamed it, whether it arrived by `import` or by `export … from`. + * `namespace` is a whole kit or surface module, to be resolved at the use site. + * `external` is a module that is neither — including a third-party one, which + * is as much a second implementation of a kit name as a hand-written copy. + * `undefined` means the declaration is local, so the chain continues. + */ +type KitBinding = + | { kind: "kit"; name: string } + | { kind: "namespace"; marker: string } + | { kind: "type" } + | { kind: "external"; from: string }; + +function kitBindingOf( + declaration: ts.Node, + surface: Map, +): KitBinding | undefined { + let from: ts.ImportDeclaration | ts.ExportDeclaration | undefined; for (let node: ts.Node | undefined = declaration; node; node = node.parent) { - if (ts.isImportDeclaration(node)) return node; + if (ts.isImportDeclaration(node) || ts.isExportDeclaration(node)) { + from = node; + break; + } } - return undefined; + const specifier = from?.moduleSpecifier; + if (!from || !specifier || !ts.isStringLiteral(specifier)) return undefined; + if ( + (ts.isImportDeclaration(from) && from.importClause?.isTypeOnly) || + (ts.isExportDeclaration(from) && from.isTypeOnly) + ) { + return { kind: "type" }; + } + const fromKit = KIT_SPECIFIER.test(specifier.text); + const fromSurface = isHostSurface( + specifier.text, + from.getSourceFile().fileName, + ); + if (!fromKit && !fromSurface) { + return { kind: "external", from: specifier.text }; + } + if (ts.isNamespaceImport(declaration) || ts.isNamespaceExport(declaration)) { + return { + kind: "namespace", + marker: fromKit ? KIT_NAMESPACE : SURFACE_NAMESPACE, + }; + } + if (ts.isImportSpecifier(declaration) || ts.isExportSpecifier(declaration)) { + // A type is not a component. It cannot be rendered, it cannot be a + // second implementation of one, and treating a type-only binding as + // something unreadable would offend on every `export type` that shares + // a kit name. + if (declaration.isTypeOnly || typeOnlyClause(declaration)) { + return { kind: "type" }; + } + const published = (declaration.propertyName ?? declaration.name).text; + const name = fromKit ? published : surface.get(published); + return name + ? { kind: "kit", name } + : { kind: "external", from: specifier.text }; + } + return { kind: "external", from: specifier.text }; +} + +/** Whether the clause a specifier sits in is type-only. */ +function typeOnlyClause( + declaration: ts.ImportSpecifier | ts.ExportSpecifier, +): boolean { + const clause = declaration.parent.parent; + return ts.isImportClause(clause) + ? clause.isTypeOnly + : ts.isExportDeclaration(clause) && clause.isTypeOnly; } /** - * The kit component an identifier refers to, by the kit's OWN name, or - * `"*namespace"` for a whole kit module, or undefined for anything else — - * including anything the checker cannot resolve, which is the conservative + * The kit component an identifier refers to, by the kit's OWN name, or a + * namespace marker for a whole kit module, or undefined for anything else — + * including anything that cannot be resolved, which is the conservative * answer: unresolved is not composition. * - * Follows what the compiler follows: import aliases, local variable aliases - * (`const Local = KitCard`, `const Local = UI.Card`) and export chains of any - * length. + * The checker answers this, so every spelling that reaches the same component + * reaches the same verdict: an import alias, a local variable alias, a chain of + * re-exports through the app's own barrels, a member of a namespace import of + * either. Hand-rolled scope analysis is what kept misjudging it — it collected + * names across scopes, missed destructured bindings, and could not follow an + * export chain at all. */ function kitComponentOf( node: ts.Expression | ts.JsxTagNameExpression, @@ -301,127 +427,208 @@ function kitComponentOf( if (seen.has(node)) return undefined; seen.add(node); - // `UI.Card` — resolve the namespace, then the member through the surface. + // The whole expression first, which is what resolves `UI.Card` when `UI` is + // a namespace of one of the app's own barrels: the checker walks the barrel's + // export to the kit import behind it. + const symbol = checker.getSymbolAtLocation(node); + if (symbol) { + for (const hop of aliasChain(symbol, checker)) { + const declaration = hop.declarations?.[0]; + if (!declaration) continue; + const binding = kitBindingOf(declaration, surface); + if (binding?.kind === "kit") return binding.name; + if (binding?.kind === "namespace") return binding.marker; + // Another module of the app's own: the chain walks through it. Only + // its end can say the name is not the kit's, which is why nothing + // here stops at the first hop that is not. + if (binding) continue; + if (ts.isVariableDeclaration(declaration) && declaration.initializer) { + return kitComponentOf(declaration.initializer, checker, surface, seen); + } + } + } + // `UI.Card` where `UI` is the kit itself: the kit is not in the program, so + // there is no symbol for the member and the namespace is the evidence. if (ts.isPropertyAccessExpression(node)) { const base = kitComponentOf(node.expression, checker, surface, seen); if (base === KIT_NAMESPACE) return node.name.text; if (base === SURFACE_NAMESPACE) return surface.get(node.name.text); - return undefined; } - if (!ts.isIdentifier(node)) return undefined; - - let symbol = checker.getSymbolAtLocation(node); - if (!symbol) return undefined; - // Through import and export aliases, however many hops. - for (let hop = 0; hop < 8 && symbol.flags & ts.SymbolFlags.Alias; hop += 1) { - const target = checker.getImmediateAliasedSymbol(symbol); - const declaration = symbol.declarations?.[0]; - // An import of a kit module is where the chain ends: the module is not - // in the program, so there is nothing further to resolve, and the - // import declaration in the source is the evidence. - if (declaration) { - const importDeclaration = importOf(declaration); + return undefined; +} + +/** + * What a component renders, as the alternative pictures it can draw: one set of + * kit marks per branch the renderer can take. + * + * Expression forms, and nothing beyond them. A conditional is two + * alternatives wherever it sits — nested inside returned JSX included, which + * is where a dispatch is usually written. `&&` adds the empty alternative, + * because the mark may be absent. `||` and `??` are their two sides. A cast, + * a parenthesis and a non-null assertion are their inner expression. A comma + * expression is its RIGHT operand alone: the others are evaluated and thrown + * away. Children of an element render together, so they multiply out. + * + * Nothing else is descended into, which is how two things end up not counting + * as rendered — both limits rather than oversights. A function body is not what + * the expression holding it draws, so a render prop and a callback in children + * are values, not pictures. And JSX handed to another component through an + * attribute is a value passed too: what the recipient does with it is beyond + * anything this can see, so a kit element in a prop is no evidence that this + * component renders it. Children are not attributes and do count. + */ +function alternatives( + node: ts.Node, + checker: ts.TypeChecker, + surface: Map, + seen = new Set(), +): Set[] { + const NOTHING = [new Set()]; + if (seen.has(node)) return NOTHING; + seen.add(node); + const of = (next: ts.Node) => alternatives(next, checker, surface, seen); + + if ( + ts.isParenthesizedExpression(node) || + ts.isAsExpression(node) || + ts.isSatisfiesExpression(node) || + ts.isTypeAssertionExpression(node) || + ts.isNonNullExpression(node) + ) { + return of(node.expression); + } + if (ts.isJsxExpression(node)) { + return node.expression ? of(node.expression) : NOTHING; + } + if (ts.isConditionalExpression(node)) { + return distinct([...of(node.whenTrue), ...of(node.whenFalse)]); + } + if (ts.isBinaryExpression(node)) { + switch (node.operatorToken.kind) { + case ts.SyntaxKind.CommaToken: + return of(node.right); + case ts.SyntaxKind.AmpersandAmpersandToken: + return distinct([new Set(), ...of(node.right)]); + case ts.SyntaxKind.BarBarToken: + case ts.SyntaxKind.QuestionQuestionToken: + return distinct([...of(node.left), ...of(node.right)]); + default: + return NOTHING; + } + } + if (ts.isJsxElement(node)) { + return together([ + [markOf(node.openingElement.tagName, checker, surface)], + ...node.children.map(of), + ]); + } + if (ts.isJsxSelfClosingElement(node)) { + return [markOf(node.tagName, checker, surface)]; + } + if (ts.isJsxFragment(node)) { + return together(node.children.map(of)); + } + if (ts.isArrayLiteralExpression(node)) { + return together(node.elements.map(of)); + } + // A local name holding a picture: `const content = ; return content`. + if (ts.isIdentifier(node)) { + const symbol = checker.getSymbolAtLocation(node); + for (const hop of symbol ? aliasChain(symbol, checker) : []) { + const declaration = hop.declarations?.[0]; if ( - importDeclaration && - ts.isStringLiteral(importDeclaration.moduleSpecifier) + declaration && + ts.isVariableDeclaration(declaration) && + declaration.initializer ) { - const specifier = importDeclaration.moduleSpecifier.text; - const fromKit = KIT_SPECIFIER.test(specifier); - const fromSurface = isHostSurface( - specifier, - importDeclaration.getSourceFile().fileName, - ); - if (fromKit || fromSurface) { - if (ts.isNamespaceImport(declaration)) { - return fromKit ? KIT_NAMESPACE : SURFACE_NAMESPACE; - } - if (ts.isImportSpecifier(declaration)) { - if (declaration.isTypeOnly) return undefined; - const published = (declaration.propertyName ?? declaration.name) - .text; - return fromKit ? published : surface.get(published); - } - return undefined; - } + return of(declaration.initializer); } } - if (!target) break; - symbol = target; } + return NOTHING; +} - // A local alias: follow its initializer. - const declaration = symbol.declarations?.[0]; - if ( - declaration && - ts.isVariableDeclaration(declaration) && - declaration.initializer - ) { - return kitComponentOf(declaration.initializer, checker, surface, seen); +/** Whether a node is a function, whose body is not this component's picture. */ +function isFunctionLike(node: ts.Node): boolean { + return ( + ts.isFunctionDeclaration(node) || + ts.isFunctionExpression(node) || + ts.isArrowFunction(node) || + ts.isMethodDeclaration(node) + ); +} + +/** The kit mark a JSX tag draws, as a set of none or one. */ +function markOf( + tag: ts.JsxTagNameExpression, + checker: ts.TypeChecker, + surface: Map, +): Set { + const kit = kitComponentOf(tag, checker, surface); + return kit && !kit.startsWith("*") ? new Set([kit]) : new Set(); +} + +/** One alternative per distinct set of marks. */ +function distinct(paths: Set[]): Set[] { + const byShape = new Map>(); + for (const path of paths) { + byShape.set([...path].sort().join("+"), path); } - return undefined; + return [...byShape.values()]; } -const KIT_NAMESPACE = "*kit"; -const SURFACE_NAMESPACE = "*surface"; +/** + * Parts that all render: every combination of one alternative from each. Kept + * distinct at each step, so the count is bounded by the mark sets that can + * actually occur rather than by the size of the tree. + */ +function together(parts: Set[][]): Set[] { + let combined: Set[] = [new Set()]; + for (const part of parts) { + const next: Set[] = []; + for (const left of combined) { + for (const right of part) { + next.push(new Set([...left, ...right])); + } + } + combined = distinct(next); + } + return combined; +} /** - * The kit components a declaration RETURNS, as the sets reachable by each - * distinct return path. + * The alternatives a declaration's body returns. * - * Two things this must not do. It must not count JSX in a nested function — - * an unused local wrapper is not what this component renders. And it must not - * flatten the paths, because the difference between a page and a dispatch is - * whether the arms render DIFFERENT marks: a layout that draws the same pair - * inside a `
    ` or a `
    ` is one picture twice, while choosing a - * line or an area by a widget's kind is a dispatch. + * Every `return` in the body, plus a concise arrow body, which is one. A + * statement after a `return` or a `throw` in the same block is unreachable and + * is not one: a picture no execution reaches is not drawn. */ -function returnedPaths( +function returnedAlternatives( declaration: ts.Node, checker: ts.TypeChecker, surface: Map, ): Set[] { const body = functionBodyOf(declaration); if (!body) return []; - - const marksIn = (root: ts.Node): Set => { - const found = new Set(); - const visit = (node: ts.Node) => { - // A nested function is not what this component returns. - if (isFunctionLike(node) && node !== root) return; - if (ts.isJsxOpeningElement(node) || ts.isJsxSelfClosingElement(node)) { - const kit = kitComponentOf(node.tagName, checker, surface); - if (kit && !kit.startsWith("*")) found.add(kit); - } - ts.forEachChild(node, visit); - }; - visit(root); - return found; - }; - - // Each return path separately: every `return` in this body (not in a - // nested function), and each arm of a conditional inside one. - const paths: Set[] = []; - const addExpression = (expression: ts.Expression) => { - if (ts.isConditionalExpression(expression)) { - addExpression(expression.whenTrue); - addExpression(expression.whenFalse); - return; - } - if (ts.isParenthesizedExpression(expression)) { - addExpression(expression.expression); - return; - } - paths.push(marksIn(expression)); - }; if (!ts.isBlock(body)) { - // A concise arrow body is the returned expression. - addExpression(body as ts.Expression); - return paths; + return alternatives(body, checker, surface); } + const paths: Set[] = []; const visit = (node: ts.Node) => { if (isFunctionLike(node) && node !== body) return; - if (ts.isReturnStatement(node) && node.expression) { - addExpression(node.expression); + if (ts.isBlock(node) || ts.isCaseClause(node) || ts.isDefaultClause(node)) { + for (const statement of node.statements) { + visit(statement); + if (ts.isReturnStatement(statement) || ts.isThrowStatement(statement)) { + return; + } + } + return; + } + if (ts.isReturnStatement(node)) { + if (node.expression) { + paths.push(...alternatives(node.expression, checker, surface)); + } return; } ts.forEachChild(node, visit); @@ -430,12 +637,16 @@ function returnedPaths( return paths; } -function isFunctionLike(node: ts.Node): boolean { - return ( - ts.isFunctionDeclaration(node) || - ts.isFunctionExpression(node) || - ts.isArrowFunction(node) || - ts.isMethodDeclaration(node) +/** Every kit component this declaration returns, on any alternative. */ +function returnedKitComponents( + declaration: ts.Node, + checker: ts.TypeChecker, + surface: Map, +): Set { + return new Set( + returnedAlternatives(declaration, checker, surface).flatMap((path) => [ + ...path, + ]), ); } @@ -455,58 +666,88 @@ function functionBodyOf(declaration: ts.Node): ts.Node | undefined { return undefined; } -/** Every kit component this declaration returns, flattened. */ -function returnedKitComponents( - declaration: ts.Node, - checker: ts.TypeChecker, - surface: Map, -): Set { - return new Set( - returnedPaths(declaration, checker, surface).flatMap((path) => [...path]), - ); -} +/** + * What a published name resolves to. + * + * `declaration` is code in this set, to be examined. `kit` is the kit's own + * component, however renamed — a re-export ships the kit's component rather + * than a second one. `unresolved` is everything else, and it is an OFFENCE + * rather than a skip: a name the kit owns, published here from somewhere this + * cannot read, is a second implementation of it as surely as a hand-written + * copy, and "could not tell" was the hole that four rounds of fixtures each + * found the next way through. + */ +type Resolution = + | { kind: "declaration"; node: ts.Node } + | { kind: "kit"; name: string } + | { kind: "type" } + | { kind: "unresolved"; why: string }; /** - * Every component a file publishes, by published name, with the declaration - * that implements it — following an export chain of any length into whichever - * file actually declares it, so splitting a copy from its kit-named export - * hides nothing and a genuine wrapper re-exported under the kit's name is not - * mistaken for one. + * Every component a file publishes, by published name — following an export + * chain of any length into whichever file actually declares it, so splitting a + * copy from its kit-named export hides nothing and a genuine wrapper + * re-exported under the kit's name is not mistaken for one. Declarations the + * file does not export are still declarations here: a duplicate hidden as a + * file-local helper is the commonest way one arrives. */ function publishedComponents( file: ts.SourceFile, checker: ts.TypeChecker, -): Map { - const published = new Map(); - const implementation = ( - symbol: ts.Symbol | undefined, - ): ts.Node | undefined => { - let resolved = symbol; - for (let hop = 0; resolved && hop < 8; hop += 1) { - const declaration = resolved.declarations?.[0]; - if (!declaration) return undefined; + surface: Map, +): Map { + const published = new Map(); + const resolve = (symbol: ts.Symbol): Resolution => { + const chain = aliasChain(symbol, checker); + for (const [hop, resolved] of chain.entries()) { + const declarations = resolved.declarations ?? []; + const value = declarations.find( + (declaration) => + ts.isFunctionDeclaration(declaration) || + ts.isClassDeclaration(declaration) || + ts.isVariableDeclaration(declaration), + ); + if (value) return { kind: "declaration", node: value }; if ( - ts.isFunctionDeclaration(declaration) || - ts.isClassDeclaration(declaration) || - ts.isVariableDeclaration(declaration) + declarations.length > 0 && + declarations.every( + (declaration) => + ts.isTypeAliasDeclaration(declaration) || + ts.isInterfaceDeclaration(declaration), + ) ) { - return declaration; + return { kind: "type" }; + } + const binding = declarations[0] + ? kitBindingOf(declarations[0], surface) + : undefined; + if (binding?.kind === "kit" || binding?.kind === "namespace") { + return { + kind: "kit", + name: binding.kind === "kit" ? binding.name : binding.marker, + }; + } + if (binding?.kind === "type") return { kind: "type" }; + // An `external` hop in the middle is another of the app's own + // modules, which the chain walks through. At the END of the chain it + // is a module this cannot read, and a kit name coming out of one is + // a second implementation of it as much as a hand-written copy. + if (hop === chain.length - 1) { + return binding?.kind === "external" + ? { kind: "unresolved", why: `published from ${binding.from}` } + : { kind: "unresolved", why: "the chain reaches no declaration" }; } - if (!(resolved.flags & ts.SymbolFlags.Alias)) return undefined; - resolved = checker.getImmediateAliasedSymbol(resolved); } - return undefined; + return { kind: "unresolved", why: "the chain reaches no declaration" }; }; const moduleSymbol = checker.getSymbolAtLocation(file); if (moduleSymbol) { for (const exported of checker.getExportsOfModule(moduleSymbol)) { if (!/^[A-Z][A-Za-z0-9]*$/.test(exported.name)) continue; - const node = implementation(exported); - if (node) published.set(exported.name, node); + published.set(exported.name, resolve(exported)); } } - // Declarations the file does not export are still declarations here. const visit = (node: ts.Node) => { const name = (ts.isFunctionDeclaration(node) || ts.isClassDeclaration(node)) && @@ -516,7 +757,7 @@ function publishedComponents( ? node.name.text : undefined; if (name && /^[A-Z][A-Za-z0-9]*$/.test(name) && !published.has(name)) { - published.set(name, node); + published.set(name, { kind: "declaration", node }); } ts.forEachChild(node, visit); }; @@ -554,29 +795,42 @@ const DASHBOARD_MARKS = new Set([ ]); /** - * Whether this declaration chooses between dashboard marks — two return paths - * that render DIFFERENT sets of them. + * Whether this declaration CHOOSES between dashboard marks: two alternatives + * whose marks are each non-empty and neither contained in the other. * - * Differing is the test, not branching and not counting. A layout that returns - * the same pair inside a `
    ` or a `
    ` has two paths and one - * picture, and must pass; a component that returns a line for one kind and an - * area for another has two pictures, and is the dispatch `` owns. - * Both the `cond ? … : …` and the guard-return spellings are return paths, so - * neither needs its own rule. + * Containment is what separates the two shapes that look alike. A page that + * always draws a chart and sometimes adds a KPI row has `{chart}` and + * `{row, chart}` — one is the other plus an optional extra, and it is one + * picture with a variation. A renderer that draws a line for one kind and an + * area for another has `{line}` and `{area}` — neither is the other, and that + * is the dispatch over visualizations `` owns. + * + * Choosing is the test, not branching and not counting: counting file-wide + * imports flagged a page that composed two marks side by side, and comparing + * only top-level returns walked past the same dispatch written one line deeper, + * inside the JSX it returns. */ function dispatchedMarks( declaration: ts.Node, checker: ts.TypeChecker, surface: Map, ): string[] { - const paths = returnedPaths(declaration, checker, surface) - .map( + const paths = distinct( + returnedAlternatives(declaration, checker, surface).map( (path) => new Set([...path].filter((mark) => DASHBOARD_MARKS.has(mark))), - ) - .filter((path) => path.size > 0); - const shapes = new Set(paths.map((path) => [...path].sort().join("+"))); - if (shapes.size < 2) return []; - return [...new Set(paths.flatMap((path) => [...path]))].sort(); + ), + ).filter((path) => path.size > 0); + const within = (small: Set, large: Set) => + [...small].every((mark) => large.has(mark)); + for (const left of paths) { + for (const right of paths) { + if (left === right) continue; + if (!within(left, right) && !within(right, left)) { + return [...new Set([...left, ...right])].sort(); + } + } + } + return []; } const DISPATCHERS: Record = { @@ -657,8 +911,11 @@ function parse(path: string, source: string): ts.SourceFile { interface Offence { name: string; path: string; - /** Why it is an offence: a name the kit owns, or a widget dispatch. */ - kind: "name" | "dispatch"; + /** + * Why it is an offence: a name the kit owns, a widget dispatch, or a name + * the kit owns published from somewhere this cannot read. + */ + kind: "name" | "dispatch" | "unresolved"; } /** @@ -687,8 +944,7 @@ function defaultExportNames( .getExportsOfModule(moduleSymbol) .find((symbol) => symbol.name === "default") : undefined; - let symbol = exported; - for (let hop = 0; symbol && hop < 8; hop += 1) { + for (const symbol of exported ? aliasChain(exported, checker) : []) { for (const declaration of symbol.declarations ?? []) { if ( (ts.isFunctionDeclaration(declaration) || @@ -706,17 +962,15 @@ function defaultExportNames( names.add(declaration.expression.text); } } - if (!(symbol.flags & ts.SymbolFlags.Alias)) break; - symbol = checker.getImmediateAliasedSymbol(symbol); } return names; } /** * Declarations that take a kit export's name without RETURNING the kit's - * component of that name, plus declarations that choose between the kit's - * dashboard marks. Pure, so the fixtures below can exercise it on sources that - * are not in the tree. + * component of that name, names the kit owns that this cannot resolve at all, + * and declarations that choose between the kit's dashboard marks. Pure, so the + * fixtures below can exercise it on sources that are not in the tree. */ function findOffences( files: { path: string; source: string }[], @@ -734,26 +988,35 @@ function findOffences( const routeDefaults = ROUTE_CONVENTION.test(path) ? defaultExportNames(file, checker) : new Set(); - const components = publishedComponents(file, checker); + const components = publishedComponents(file, checker, surface); - for (const [name, declaration] of components) { + for (const [name, resolution] of components) { if (!kit.has(name) || routeDefaults.has(name)) continue; - if (returnedKitComponents(declaration, checker, surface).has(name)) { - continue; - } + if (resolution.kind === "kit" || resolution.kind === "type") continue; // An exception is recorded where the component is DECLARED, so a // barrel that re-exports it does not need a second one for the // same component. The published path is checked too, for a name a // file publishes without declaring anywhere in the set. - const declaredIn = declaration.getSourceFile().fileName; + const declaredIn = + resolution.kind === "declaration" + ? resolution.node.getSourceFile().fileName + : path; if (`${name} in ${path}` in allowed) continue; if (`${name} in ${declaredIn}` in allowed) continue; + if (resolution.kind === "unresolved") { + offences.push({ name, path, kind: "unresolved" }); + continue; + } + if (returnedKitComponents(resolution.node, checker, surface).has(name)) { + continue; + } offences.push({ name, path, kind: "name" }); } if (path in dispatchers) continue; - for (const [, declaration] of components) { - const marks = dispatchedMarks(declaration, checker, surface); + for (const [, resolution] of components) { + if (resolution.kind !== "declaration") continue; + const marks = dispatchedMarks(resolution.node, checker, surface); if (marks.length > 1) { offences.push({ name: marks.join("+"), path, kind: "dispatch" }); break; @@ -776,11 +1039,15 @@ describe("the host app composes the kit rather than re-implementing it", () => { kit, ALLOWED, hostSurface(), - ).map(({ name, path, kind }) => - kind === "name" - ? `${name} in ${path} — @codefly-dev/ui already exports it. Compose the kit's component, or add "${name} in ${path}" to ALLOWED with what the two things are and why neither can stand in for the other.` - : `${path} draws with ${name} — more than one of the kit's dashboard marks, which is a widget dispatch and 's job. Mount or , or add ${path} to DISPATCHERS with what it is and what would retire it.`, - ); + ).map(({ name, path, kind }) => { + if (kind === "name") { + return `${name} in ${path} — @codefly-dev/ui already exports it. Compose the kit's component, or add "${name} in ${path}" to ALLOWED with what the two things are and why neither can stand in for the other.`; + } + if (kind === "unresolved") { + return `${name} in ${path} — @codefly-dev/ui exports that name and this publishes it from somewhere this gate cannot read, so it cannot tell the kit's component from a second one. Publish the kit's own, or declare it here where it can be read.`; + } + return `${path} draws with ${name} — more than one of the kit's dashboard marks, which is a widget dispatch and 's job. Mount or , or add ${path} to DISPATCHERS with what it is and what would retire it.`; + }); expect(offences).toEqual([]); }); @@ -1368,6 +1635,235 @@ export const LocalPanel = (props) => ;`, ).toBeGreaterThan(0); }); + // What the gate must see through, and what it must not mistake for a copy. + // Each of these was found by an independent review of an earlier shape of + // this detector, and each named a rule rather than a trick: these say what + // the rule now is. + it.each([ + [ + "a dispatch written one line inside the JSX it returns", + `import { LineChart as L, AreaChart as A } from "@codefly-dev/ui/dashboard"; +export function WidgetBody({ kind }) { + return
    {kind === "line" ? : }
    ; +}`, + true, + ], + [ + "the same dispatch behind a cast", + `import { LineChart as L, AreaChart as A } from "@codefly-dev/ui/dashboard"; +export function WidgetBody({ kind }) { + return (kind === "line" ? : ) as React.ReactNode; +}`, + true, + ], + [ + "kit JSX in a comma operand, which is evaluated and discarded", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card() { return (,
    copy
    ); }`, + true, + ], + [ + "kit JSX in a return no execution reaches", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card() { + return
    copy
    ; + return ; +}`, + true, + ], + [ + "kit JSX handed to another component through a prop", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +function Ignore(props) { return
    copy
    ; } +export function Card() { return } />; }`, + true, + ], + [ + "a dispatch whose arms are fragments", + `import { LineChart as L, AreaChart as A } from "@codefly-dev/ui/dashboard"; +export function WidgetBody({ kind }) { + return kind === "line" ? <> : <>    ; +}`, + true, + ], + [ + "kit JSX returned by a nested function the component only declares", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card() { + function Unused() { return ; } + return
    copy
    ; +}`, + true, + ], + [ + "kit JSX in a callback the component puts in its children", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card() { return {() => }; }`, + true, + ], + [ + "returned kit JSX in an array, which React renders", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card(props) { return []; }`, + false, + ], + [ + "returned kit JSX held in a local first", + `import { Card as KitCard } from "@codefly-dev/ui/layout"; +export function Card(props) { + const content = ; + return content; +}`, + false, + ], + [ + "a mark drawn always beside one drawn only sometimes", + `import { KPIRow, MetricLineChart } from "@codefly-dev/ui/dashboard"; +export function Overview({ withRow }) { + return ( +
    + {withRow ? : null} + +
    + ); +}`, + false, + ], + ])("%s", (_name, source, rejected) => { + expect(detect("src/features/x/ui/example.tsx", source).length > 0).toBe( + rejected, + ); + }); + + it("catches a copy re-exported through the app's own path alias", () => { + // `@/…` and `./…` reach the same module. Resolving only the second read + // the barrel and stopped, so the copy at the end of it went unexamined. + expect( + findOffences( + [ + { + path: "src/features/x/ui/local.tsx", + source: "export const LocalPanel = () =>
    copy
    ;", + }, + { + path: "src/features/x/ui/index.ts", + source: + 'export { LocalPanel as Card } from "@/features/x/ui/local";', + }, + ], + kit, + {}, + surface, + {}, + ).map(({ name, kind }) => `${kind}:${name}`), + // `name`, not `unresolved`: the copy was READ. An alias the gate + // cannot follow would also be refused, but for not knowing, and that + // is a weaker answer than the one available here. + ).toEqual(["name:Card"]); + }); + + it("catches a copy however many re-exports stand between", () => { + // A chain is followed to its end, cycle-detected rather than cut off at + // a depth: ten hops is not more suspicious than two, and a gate that + // gave up at eight could be escaped by writing a ninth. + const sources = [ + { + path: "src/features/x/ui/local.tsx", + source: "export const LocalPanel = () =>
    copy
    ;", + }, + ]; + for (let hop = 0; hop < 9; hop += 1) { + sources.push({ + path: `src/features/x/ui/bridge${hop}.ts`, + source: `export { LocalPanel } from "./${hop ? `bridge${hop - 1}` : "local"}";`, + }); + } + sources.push({ + path: "src/features/x/ui/index.ts", + source: 'export { LocalPanel as Card } from "./bridge8";', + }); + expect( + findOffences(sources, kit, {}, surface, {}).map( + ({ name, kind }) => `${kind}:${name}`, + ), + ).toEqual(["name:Card"]); + }); + + it.each([ + [ + "a named import", + 'export { Card as Panel } from "@codefly-dev/ui/layout";', + 'import { Panel } from "./bridge";\nexport const Card = (props) => ;', + ], + [ + "a namespace import", + 'export { Card } from "@codefly-dev/ui/layout";', + 'import * as UI from "./bridge";\nexport const Card = (props) => ;', + ], + ])( + "accepts a wrapper reaching the kit through the app's own barrel by %s", + (_name, bridge, local) => { + // The barrel is the app's, the component behind it is the kit's. A + // gate that stopped at the barrel rejected a genuine wrapper for how + // it spelled its import. + expect( + findOffences( + [ + { path: "src/features/x/ui/bridge.ts", source: bridge }, + { path: "src/features/x/ui/local.tsx", source: local }, + ], + kit, + {}, + surface, + {}, + ), + ).toEqual([]); + }, + ); + + it("refuses a kit name published from outside anything it can read", () => { + // Not a hand-written copy and not the kit either: a second Card from a + // third-party package is the same two-renderers problem, and "could not + // tell" is not a verdict this gate is allowed to reach. + expect( + findOffences( + [ + { + path: "src/features/x/ui/index.ts", + source: 'export { Panel as Card } from "some-other-ui-kit";', + }, + ], + kit, + {}, + surface, + {}, + ).length, + ).toBeGreaterThan(0); + }); + + it("does not refuse a TYPE that shares a kit name", () => { + // A type cannot be rendered and cannot stand in for a component, so it + // is neither a copy nor something unreadable. + expect( + findOffences( + [ + { + path: "src/features/x/model/types.ts", + source: "export interface Card { id: string }", + }, + { + path: "src/features/x/ui/index.ts", + source: 'export type { Card } from "../model/types";', + }, + ], + kit, + {}, + surface, + {}, + ), + ).toEqual([]); + }); + it("ignores a name the kit does not export", () => { expect( detect( diff --git a/module/services/frontend/code/src/solutions/SolutionDashboard.tsx b/module/services/frontend/code/src/solutions/SolutionDashboard.tsx index f7ec7c040..330d45656 100644 --- a/module/services/frontend/code/src/solutions/SolutionDashboard.tsx +++ b/module/services/frontend/code/src/solutions/SolutionDashboard.tsx @@ -20,14 +20,13 @@ import { SortableGrid, type WidgetReadFailure, } from "@codefly-dev/ui/dashboard"; -import { useQuery, useQueryClient } from "@tanstack/react-query"; -import { GripVertical, Info, Plus, X } from "lucide-react"; import { - type ReactNode, - useEffect, - useState, - useSyncExternalStore, -} from "react"; + type QueryClient, + useQuery, + useQueryClient, +} from "@tanstack/react-query"; +import { GripVertical, Info, Plus, X } from "lucide-react"; +import { type ReactNode, useState, useSyncExternalStore } from "react"; import { flushSync } from "react-dom"; import { resolveActor } from "@/features/audit/model/transforms"; import { @@ -132,82 +131,110 @@ function SolutionWidget({ // happens to share the same solution/dashboard/widget/org ids. Disabled until // the org resolves so the pre-org window reads as loading, never as empty. const client = useQueryClient(); + const source = widgetSource({ solutionId, dashboardId, widget, orgId }); const { data, isError, error } = useQuery({ - queryKey: [ - "solution-widget", - solutionId, - dashboardId, - widget.id, - orgId, - graph, - ], + queryKey: [...source, graph], queryFn: () => runDashboard( sdk.audit, singleWidgetGraph(graph, widget), SINGLE_WIDGET_DASHBOARD, { orgId }, - ).then((resolved) => resolved.widgets[0]), + ) + .then((resolved) => resolved.widgets[0]) + .catch((cause: unknown) => { + // The read itself answers for what it leaves behind. A refused + // read deletes every snapshot of this widget's source before the + // rejection reaches anyone, so there is nothing retained for a + // later error to hand back and nothing to keep in step with it. + if (!mayKeepRetainedRows(readOutcome(true, cause))) { + discardSnapshots(client, source); + } + throw cause; + }), enabled: orgId !== "", }); - // A refused read does not get to keep the numbers it fetched before. - // TanStack Query hands back the last successful data when a refetch - // rejects, so forwarding it on every error would leave a viewer whose - // audit-read authority was revoked still looking at the counts — the same - // wrong answer `mayKeepRetainedRows` exists to make unwritable, arrived at - // from the dashboard instead of from a table. A transient failure says - // nothing about authority, so those values stay and read as stale. - // - // A refusal DELETES the snapshots rather than marking them ineligible. - // Two earlier attempts marked instead, and both leaked: hiding the values - // for the refusal alone let the next transient error hand them back, and a - // latch beside them in the cache was a second entry with its own lifetime - // — garbage-collected on its own schedule, and cleared by a cached success - // for another revision of the graph, neither of which is a fresh grant. - // Nothing restorable is left here, so there is no second thing to keep in - // step: only a later successful read can put values back. const outcome = readOutcome(isError, error); - const widgetId = widget.id; - useEffect(() => { - if (outcome !== "forbidden") return; - // Every cached revision of this widget's series, not just the one this - // render resolved: the graph is the last part of the query key, so a - // dashboard whose declaration has changed since — a different title is - // enough — has its own entry holding the same refused numbers. - // - // Reset, not remove: removing destroys the query this component is - // observing, which leaves it with nothing to refetch and no way back - // even once authority returns. A reset discards the data and keeps the - // query, so the only route to values on screen is a read that succeeds. - client.resetQueries({ - queryKey: ["solution-widget", solutionId, dashboardId, widgetId, orgId], - exact: false, - }); - }, [client, outcome, solutionId, dashboardId, widgetId, orgId]); - - const refused = outcome === "forbidden"; - // `!refused` covers the one render between the refusal arriving and the - // effect above clearing the cache, where `data` is still the old success. - // No test distinguishes it, and that is worth saying rather than implying: - // the renderer answers a refusal before it reads the series, so nothing - // reaches the screen through this window either way. It is the host's half - // of one rule stated in two places, not the mechanism. - const retained = - !refused && mayKeepRetainedRows(outcome) ? (data?.series ?? null) : null; + // Whatever the cache still holds. A refusal has already emptied it, which is + // why this reads the snapshot without asking whether the viewer may keep it: + // the question was put at the read boundary, where the answer could be acted + // on once for every revision instead of per mounted component. + const retained = data?.series ?? null; return ( ); } +/** + * Every cached revision of one widget's source — the query key without the + * declaration that produced it. + * + * A dashboard whose declaration has changed since keeps its own cache entry: a + * different title is enough for another key, holding the same numbers. So a + * rule about this widget's values is a rule about the whole prefix, never about + * the one entry a render happens to have resolved. + */ +function widgetSource({ + solutionId, + dashboardId, + widget, + orgId, +}: { + solutionId: string; + dashboardId: string; + widget: MetricWidget; + orgId: string; +}): readonly unknown[] { + return ["solution-widget", solutionId, dashboardId, widget.id, orgId]; +} + +/** + * Delete the retained snapshots under a widget's source: the values go, the + * queries and their states stay. + * + * This is what `mayKeepRetainedRows` says must happen to rows a refused read + * leaves behind, done where the refusal is classified rather than in a + * component. The difference is not tidiness. A component effect runs only for + * the revision that is mounted and only while it is: a refusal that arrives + * after the viewer switched dashboards, or after the page unmounted, left the + * numbers in the cache for the next render to serve. The read is the one thing + * that happens exactly once per refusal, whoever is watching. + * + * Writing the state is the whole of it, deliberately: + * + * - `removeQueries` destroys the entry, which silently cancels the very read + * that is reporting the refusal (`Query.destroy` → `cancel({silent:true})`) + * and leaves a mounted observer holding a detached query that no longer + * re-renders — the values stay on screen and recovery has nothing to + * refetch into. + * - `setQueryData(key, undefined)` cannot express this at all: the client + * returns early when the new data is `undefined`, so the call does nothing. + * - `resetQueries` clears the error with the data and refetches every active + * entry, so a standing refusal re-asks and re-clears without end. + * + * Setting `data` notifies the observers, cancels nothing, keeps the refusal, + * and starts no fetch. An entry left with no data renders as a wait and is + * stale, so the next read of it is a real read — which is the only way values + * come back. + */ +function discardSnapshots(client: QueryClient, source: readonly unknown[]) { + for (const query of client + .getQueryCache() + .findAll({ queryKey: source, exact: false })) { + if (query.state.data === undefined) continue; + query.setState({ data: undefined, dataUpdatedAt: 0 }); + } +} + // The host's read classification as the renderer's presentational one. A read // that succeeded is no failure at all, which is what `undefined` says. const WIDGET_FAILURE: Record = { diff --git a/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx b/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx index a26c53560..e1376b1f3 100644 --- a/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx +++ b/module/services/frontend/code/src/solutions/__tests__/SolutionDashboard.test.tsx @@ -271,9 +271,9 @@ describe("SolutionDashboards", () => { }); // The longer sequence, and the one that matters most: a refusal followed by - // an ordinary failure, with no authorized read in between. The cached - // snapshot outlives the refusal, so unless the refusal LATCHES, the next - // error makes those values eligible again and hands them straight back. + // an ordinary failure, with no authorized read in between. A snapshot that + // outlived the refusal would be eligible again at the next error, which + // hands those values straight back. describe.each(["line", "area", "bar", "number", "table"] as const)( "a refused %s widget", (visualization) => { @@ -482,7 +482,7 @@ describe("SolutionDashboards", () => { }); expect(await screen.findAllByText("No access")).not.toHaveLength(0); - // Authority restored: the latch lifts and the NEW values are drawn. + // Authority restored: a read succeeds and the NEW values are drawn. server.use( http.post(rpc("AuditService", "AggregateAuditLog"), () => HttpResponse.json({ buckets: [{ key: "2026-09-01", count: "42" }] }), @@ -495,6 +495,176 @@ describe("SolutionDashboards", () => { expect(screen.queryByText("No access")).toBeNull(); }); + // Everything above drives the refusal through a mounted component. These + // drive it where no mounted component can answer for it: the refusal is a + // property of the READ, and the read outlives whoever started it. + describe("a refusal with nobody watching it", () => { + // A handler that holds its request open until the test releases it, so a + // refusal can be in flight across an unmount or a change of dashboard. + function heldRefusal() { + let enter!: () => void; + let release!: () => void; + const entered = new Promise((resolve) => { + enter = resolve; + }); + const released = new Promise((resolve) => { + release = resolve; + }); + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), async () => { + enter(); + await released; + return HttpResponse.json( + { code: "permission_denied" }, + { status: 403 }, + ); + }), + ); + return { entered, release }; + } + + it.each([ + [ + "a 503", + () => HttpResponse.json({ code: "unavailable" }, { status: 503 }), + ], + ["a dropped connection", () => HttpResponse.error()], + ])( + "deletes the values when it completes after the page is gone, then %s", + async (_name, fail) => { + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ + buckets: [{ key: "2026-09-01", count: "37" }], + }), + ), + ); + const { client, unmount } = renderDashboards(); + expect(await screen.findAllByText("37")).not.toHaveLength(0); + + const { entered, release } = heldRefusal(); + const refusal = client.refetchQueries({ + queryKey: ["solution-widget"], + }); + await entered; + // The viewer leaves while the request is still open, so no effect + // of theirs is left to run when the answer comes back. + unmount(); + release(); + await refusal; + + // A retry nobody is watching either. It must not hand the values + // back to the next mount. + server.use(http.post(rpc("AuditService", "AggregateAuditLog"), fail)); + await client.refetchQueries({ queryKey: ["solution-widget"] }); + + renderInApp( + , + client, + ); + await act(async () => { + await settle(); + }); + expect(screen.queryAllByText("37")).toHaveLength(0); + }, + ); + + it("deletes the revision on screen when it lands on the one the viewer left", async () => { + // The refusal is for the revision the viewer has navigated away from; + // the one mounted is another revision of the same widget, with its own + // cached values on screen. Both are this widget's source, and nothing + // in React connects the refused read to the component drawing them. + const renamed: DataGraph = { + ...graph, + dashboards: [{ ...graph.dashboards[0], title: "Activity (renamed)" }], + }; + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ buckets: [{ key: "2026-09-01", count: "37" }] }), + ), + ); + const { client, rerender } = renderDashboards(); + expect(await screen.findAllByText("37")).not.toHaveLength(0); + + // The second revision reads its own values, under its own key. + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ buckets: [{ key: "2026-09-01", count: "42" }] }), + ), + ); + rerender(); + expect(await screen.findAllByText("42")).not.toHaveLength(0); + rerender(); + await act(async () => { + await settle(); + }); + + const { entered, release } = heldRefusal(); + const refusal = client.refetchQueries({ + queryKey: ["solution-widget"], + type: "active", + }); + await entered; + + // Mid-refusal the viewer switches back, where the other revision's + // values are cached and its refresh fails transiently: 42, stale. + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ code: "unavailable" }, { status: 503 }), + ), + ); + rerender(); + expect(await screen.findAllByText("Stale")).not.toHaveLength(0); + expect(screen.getAllByText("42").length).toBeGreaterThan(0); + + await act(async () => { + release(); + await refusal; + await settle(); + }); + expect(screen.queryAllByText("42")).toHaveLength(0); + }); + + it("asks once, rather than clearing its own error and asking again", async () => { + // Clearing the entry to drop its values restarts every active read of + // it, whose next answer is the same refusal: a standing 403 became an + // unbounded loop. Deleting the values touches neither the error nor + // the fetch, so the retry policy is the client's alone. + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), () => + HttpResponse.json({ buckets: [{ key: "2026-09-01", count: "37" }] }), + ), + ); + const { client } = renderDashboards(); + expect(await screen.findAllByText("37")).not.toHaveLength(0); + + let refusals = 0; + server.use( + http.post(rpc("AuditService", "AggregateAuditLog"), async () => { + refusals += 1; + // Answered a tick late, so the component sees the pending state + // in between — the window a self-restarting clear re-entered. + await new Promise((resolve) => setTimeout(resolve, 15)); + return HttpResponse.json( + { code: "permission_denied" }, + { status: 403 }, + ); + }), + ); + await act(async () => { + await client.refetchQueries({ queryKey: ["solution-widget"] }); + }); + expect(await screen.findAllByText("No access")).not.toHaveLength(0); + for (let round = 0; round < 8; round += 1) { + await act(async () => { + await settle(); + }); + } + // Three widgets, one refresh, `retry: false` in tests. + expect(refusals).toBe(3); + }); + }); + it("reads as loading until the org context resolves, never as empty", () => { authState.organizationId = undefined; diff --git a/module/services/frontend/code/src/test/container.tsx b/module/services/frontend/code/src/test/container.tsx index c7caa3e83..acf8a8e7e 100644 --- a/module/services/frontend/code/src/test/container.tsx +++ b/module/services/frontend/code/src/test/container.tsx @@ -25,8 +25,20 @@ export function renderInApp(ui: React.ReactElement, reuse?: QueryClient) { // SECOND read of the same query — the success-then-refused sequence, which is // the one that proves a denial is not hidden behind rows already on screen. // A cold denied read cannot show that, because there is nothing retained. + const mounted = render( + {ui}, + ); return { - ...render({ui}), + ...mounted, client, + // Re-render under the SAME provider. React Testing Library's own + // `rerender` replaces the whole tree with what it is given, so a test + // that passed the bare container would lose the client — which is the + // one thing a test of a second revision of the same declaration needs + // to keep. + rerender: (next: React.ReactElement) => + mounted.rerender( + {next}, + ), }; }