Skip to content

feat(analytics): add useMetricView client experience - #488

Open
atilafassina wants to merge 62 commits into
mainfrom
mv-hook
Open

feat(analytics): add useMetricView client experience#488
atilafassina wants to merge 62 commits into
mainfrom
mv-hook

Conversation

@atilafassina

@atilafassina atilafassina commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Delivers the complete Metric Views client experience on top of #512: applications can query typed Metric Views, format results from catalog metadata, and build interactive cross-filtering charts without duplicating query or presentation logic.

Delivered

  • Add useMetricView with inferred selected-row types, metadata, loading/error state, cancellation, HMR refresh, and shared warehouse-readiness reporting.
  • Consolidate the Analytics SSE lifecycle used by useMetricView and useAnalyticsQuery.
  • Add framework-agnostic formatValue, formatLabel, toD3Format, and toMetricFilter utilities.
  • Add chart click and controlled-selection APIs (onDataClick and selected) for cross-filter interactions.
  • Add an end-to-end Metric Views playground covering metadata-driven labels, formatting, filters, charts, and table output.
  • Document hook usage, formatting, filters, and Plotly/ECharts integration.
  • Cover hook typing, SSE behavior, format/filter utilities, chart interactions, and the playground integration with focused tests.

Stacked on #512.

@github-actions

Copy link
Copy Markdown
Contributor

🔬  Run evals on this PR  ·  Go to Evals Monitor →

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

📦 Bundle size report

Compared against bundle-size-baseline.json (main).

@databricks/appkit

npm tarball (packed): 827 KB (-1.2 KB) — gzipped download (dist + bin; excludes release-only docs/NOTICE).

dist raw gzip
JS (runtime) 855 KB (-294 B) 298 KB (-235 B)
Type declarations 313 KB 108 KB (-9 B)
Source maps 1.7 MB (-5.6 KB) 557 KB (-1.5 KB)
Other 11 KB 3.7 KB
Total 2.8 MB (-5.9 KB) 967 KB (-1.7 KB)
Per-entry composition (own code — deps external (as shipped))
Entry Initial (gz) Lazy (gz) Total (gz) node_modules (min) Own code (min)
. 87 KB 2.5 KB 90 KB external 285 KB
./beta 48 KB 459 B 48 KB external 140 KB
./type-generator 20 KB 0 B 20 KB external 57 KB

Chunks:

Entry Chunk Load Size (gz)
. index.js initial 83 KB
. utils.js initial 4.0 KB
. remote-tunnel-manager.js lazy 2.5 KB
./beta beta.js initial 32 KB
./beta stream-manager.js initial 5.8 KB
./beta wide-event-emitter.js initial 3.2 KB
./beta databricks.js initial 3.0 KB
./beta configuration.js initial 2.1 KB
./beta service-context.js initial 1.3 KB
./beta client.js initial 431 B
./beta client-options.js initial 219 B
./beta supervisor-api.js lazy 194 B
./beta databricks.js lazy 142 B
./beta index.js lazy 123 B
./type-generator index.js initial 20 KB

@databricks/appkit-ui

npm tarball (packed): 339 KB (+2.0 KB) — gzipped download (dist + bin; excludes release-only docs/NOTICE).

dist raw gzip
JS (runtime) 387 KB (+2.2 KB) 130 KB (+603 B)
Type declarations 226 KB 82 KB
Source maps 744 KB (+4.8 KB) 245 KB (+1.3 KB)
CSS 16 KB 3.3 KB
Total 1.3 MB (+7.0 KB) 461 KB (+1.9 KB)
Per-entry composition (consumer bundle — deps bundled, peerDeps external)
Entry Initial (gz) Lazy (gz) Total (gz) node_modules (min) Own code (min)
./js 5.0 KB 49 KB 54 KB 208 KB 14 KB
./js/beta 20 B 0 B 20 B 0 B 0 B
./react 431 KB (+405 B) 49 KB 480 KB (+405 B) 1.3 MB 174 KB (+1.1 KB)
./react/beta 1.0 KB 0 B 1.0 KB 0 B 1.9 KB

Chunks:

Entry Chunk Load Size (gz)
./js index.js initial 4.9 KB
./js chunk initial 120 B
./js apache-arrow lazy 49 KB
./js/beta beta.js initial 20 B
./react index.js initial 429 KB
./react tslib initial 2.1 KB
./react apache-arrow lazy 49 KB
./react/beta beta.js initial 1.0 KB

@atilafassina atilafassina changed the title feat(analytics): metric-view runtime hook + format utilities + payload metadata (PR5) feat(analytics): useMetricViews Jul 23, 2026
@atilafassina
atilafassina force-pushed the mv-hook branch 2 times, most recently from 7456890 to 613d5cb Compare July 29, 2026 08:24
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

🤖 AppKit PR bot

🔬 Run evals

Start an eval for this PR from the evals-monitor app: Go to Evals Monitor →

📦 Try this PR's app template

Scaffolds a new app from this PR's SDK build. Run it in any folder (requires the GitHub CLI — gh auth login — and the Databricks CLI):

gh run download 31110646227 -R databricks/appkit -n appkit-template-0.55.0-pr.693f431-mv-hook-488 -D appkit-pr-488 \
  && unzip -o "appkit-pr-488/appkit-template-0.55.0-pr.693f431-mv-hook-488.zip" -d "appkit-pr-488" \
  && databricks apps init --template "appkit-pr-488"

The template pins @databricks/appkit and @databricks/appkit-ui to tarballs built from this branch, so the scaffolded app runs against this PR's code.

Freeze the three shared seams the metric-view hook/server/generator phases
compile against:
- S1: MetricColumnMeta + MetricViewsMetadata value types in packages/shared
- S2: optional per-column metadata on the SSE result message + makeResultMessage
- S3: base MetricRegistry, MetricKey, Infer* helpers, MetricFilter mirrors, and
  UseMetricViewOptions/UseMetricViewResult in appkit-ui hook types

Types only (plus a makeResultMessage passthrough); existing /query callers are
unchanged since metadata is optional.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
… metadata (PR5 1-4)

Implement the four parallel PR5 slices against the frozen phase-0 contracts:

- Generator (type-generator): emit metric-views.ts (was .d.ts) carrying both
  the erasable declare-module MetricRegistry augmentation and a runtime
  `export const metricViewsMetadata = {...} as const`. Header is a type-only
  import (no runtime side-effect import on the Node server). Rename propagated
  through METRIC_TYPES_FILE, mvOutFile, vite-plugin, CLI announce, and tests;
  generated .ts added to Biome ignore.
- Server (analytics plugin): accept an injected `metricViewsMetadata` config
  and stamp the responding metric's per-column slice (scoped to the requested
  measures/dimensions) into the SSE result payload. Metadata is response
  decoration — it never enters composeMetricCacheKey and never alters SQL.
- Hook (appkit-ui): `useMetricView(key, opts)` mirroring useAnalyticsQuery
  (SSE, abort-on-arg-change, autoStart), returning
  { data, loading, error, errorCode, metadata }.
- Formatters (appkit-ui js): pure, React-free, tree-shakeable formatValue /
  formatLabel / toD3Format taking the format spec / column metadata as args.

Also fix a pre-existing latent port collision: analytics.integration.test.ts
and server.integration.test.ts both hardcoded port 9879; under the added
metric-test weight they could bind concurrently in the shared vitest worker
pool, so an analytics request hit the server-plugin app and 404'd. Switch the
analytics integration test to an OS-assigned ephemeral port (port: 0), matching
the files plugin integration test.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
…se 5)

Integrate the metric-view runtime in dev-playground and document it:

- Regenerate the generated artifact as shared/appkit-types/metric-views.ts
  (delete the legacy .d.ts). Verified byte-for-byte identical to a live
  `generate-types` DESCRIBE against a real UC Metric View (warehouse
  dd43ee29fedd958d, dogfood): display_name/format/description genuinely flow
  from the UC YAML through typegen into the runtime metricViewsMetadata const.
- Inject the const server-side: analytics({ metricViewsMetadata }).
- Add a /metric-views demo route calling useMetricView("revenue", …) with
  timeGrain/timeDimension, rendering a chart + table whose labels and value
  formats come from the payload metadata (never hand-typed), degrading
  gracefully when metadata is absent.
- Docs: extend plugins/analytics.md with the useMetricView + format-utility +
  metricViewsMetadata injection story (Plotly + ECharts examples), and fix the
  stale metric-views.d.ts references in development/type-generation.md.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
… builder to /js

Move the twelve-operator MetricFilter grammar out of react/hooks/types.ts into a
canonical, framework-agnostic js/metric-filter/ module and add a toMetricFilter
builder that compiles a { dimension -> value(s) } shorthand into a MetricFilter
(scalar -> equals, array -> in, omit undefined/empty). react/hooks/types.ts now
re-exports the types so the /react public surface and UseMetricViewOptions.filter
are unchanged. Wire the dev-playground metric-views route's buildFilter onto
toMetricFilter, keeping only the app-specific cross-filter facet-exclusion local.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Adds two public chart props, inherited by every chart type via the factory:
- onDataClick?(datum: ChartClickDatum): fire-and-forget click callback. base.tsx
  builds a memoized internal echarts onEvents={{ click }} only when the handler is
  set (no idle listener), mapping raw params via the pure mapToDatum. Pointer-only
  (canvas) — documented to require a keyboard-accessible equivalent.
- selected?: string | string[]: controlled, name-based visual emphasis. base.tsx
  runs the pure applySelectionEmphasis transform over the built option so matching
  bar/pie-donut categories stay prominent and the rest dim; no-op when unset.

ChartClickDatum is the only new public (barrel) symbol; mapToDatum,
applySelectionEmphasis and SelectionEmphasisOptions are internal. echarts types
stay out of the public API (datum.raw is unknown). Phases 1 and 2 are committed
together so the producer helpers have their consumer (satisfies knip).

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
…egment charts

Wire the new appkit-ui chart props into the metric-views demo: the region BarChart
and segment DonutChart get onDataClick={(d) => setDimension(dim, d.name)} — reusing
the same setDimension the table row-click uses — and selected={selection[dim]} so the
clicked category is emphasized. LineChart, Table, and the existing (keyboard-accessible)
table row-click are unchanged.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
… review cleanup

Address adversarial-review findings on the useMetricView / metric-route branch:

- Type soundness: infer rows from the selected measure/dimension tuples
  (PickMetricRow) and correlate timeDimension/timeGrain to temporal dims only.
- Formatting: preserve every currency symbol the generator emits end-to-end and
  keep bigint precision (no Number() rounding).
- Cache correctness: stamp fresh per-column metadata AFTER the cached execute()
  so a cache hit never serves stale labels/formats after a redeploy.
- Charts: guard selected="" as a no-op, split [x,y] click tuples into x/y, and
  memoize onEvents on handler presence (no listener thrash per SSE tick).
- Typegen: sweep a stale sibling metric-views.d.ts on upgrade and reject a
  .d.ts mvOutFile.
- Drop the unused public notify export and the fake "Write back" demo; remove
  the dead autoStart option; align AnalyticsStreamMessage; tighten the biome
  ignore; add tests + comment cleanup.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Drop comments that restated adjacent JSDoc/functions:
- base.tsx: the inline-handler re-subscribe rationale lived in both the
  `interactive` and `onEvents` comments; keep it once at `onEvents` (where
  the subscription happens) and point the `interactive` note at it.
- use-metric-view.ts: result-branch comment re-explained metadata narrowing
  already documented on `asMetricMetadata`; defer to that doc.
- js/format/index.ts: call-site comment restated `currencyPrefix`'s own JSDoc.
- render-types.ts: convert `generateMetricTypeDeclarations`'s // block to
  /** */ so the exported function's rationale surfaces on IDE hover.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
useMetricView never implemented autoStart — the row was copied from
useAnalyticsQuery's options table. The hook's effect calls start()
unconditionally and UseMetricViewOptions has no such field.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
- Centralize analytics SSE parsing across React hooks
- Share metric filter types and runtime operator vocabulary
- Reuse metadata and label formatters; refresh size baseline

Signed-off-by: Atila Fassina <atila@fassina.eu>
- analytics.md: make the Plotly and ECharts chart-library references
  links to plotly.com/javascript and echarts.apache.org; minor wording
  tidy (hardcode).
- charts/types.ts: trim the ChartClickDatum doc comment.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Phase 1 of typegen-ci-resilient-describe. In blocking (`--wait`) mode the
type generator now suppresses the `.d.ts` write on ANY degraded result
(query `result: unknown` or degraded metric), leaving the committed types
untouched as the CI fallback of record, then throws as before. The prior
path wrote degraded (`unknown`) declarations first and threw after, which
clobbered good committed types on a fresh CI checkout — including via the
auth/timeout/bad-id/DELETED fatal-degrade path.

Non-blocking mode is unchanged (still writes degraded types for the
detached worker to refresh). Tests inverted to assert no-write-on-degrade
while preserving throw + behavioral assertions; adds coverage for the
query-side fatal-degrade clobber-prevention case.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Phase 2 of typegen-ci-resilient-describe. Adds a pure
classifyBlockingFailure(error) → "deterministic" | "environmental" to
type-generator/errors.ts, building on the existing getErrorStatus and
isConnectivityError helpers.

Deterministic (build must crash regardless of committed types): HTTP 404
(bad warehouse id) and 400 (malformed request), checked first and walked
through cause/AggregateError chains. Environmental (has-types gate applies
later): 401/403 auth, connectivity, DELETED/DELETING, wait-timeout, and any
unrecognized failure (the default). The auth status set is a one-line
change point for the auth-owning team. No behavior change to
isConnectivityError. Adds tests/errors.test.ts.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
…wait

Phase 3 of typegen-ci-resilient-describe (join point). Wires the
classifyBlockingFailure taxonomy into the reordered blocking write path so
`--wait` degrades gracefully on environmental failures instead of always
crashing:

- Deterministic failures (SQL syntax, HTTP 404/400) still crash the build.
- Environmental failures (401/403 auth, connectivity, DELETED/DELETING,
  wait-timeout, unrecognized) now flow through a has-types gate: if committed
  analytics/metric-views .d.ts exist, skip the (already-suppressed) write,
  emit one loud greppable stderr warning naming the coarse cause
  (auth blocked / warehouse unreachable / warehouse unavailable) + warehouse
  id, and exit 0 using the committed types as the fallback of record. If no
  committed types exist, crash with a generic 'run generate-types --wait
  locally' remedy.

Serving types are excluded from the gate (gitignored, degrade independently).
Non-blocking mode is unchanged. Threads deterministic-vs-environmental and a
coarse cause label out of the query + metric preflights. Adds gate-matrix
coverage: environmental+present (per cause) → warning+exit0, environmental+
absent → crash, deterministic (404/400/syntax) → crash regardless of types,
partial presence, serving-exclusion, and CI-safe (ANSI-free) warning output.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Phase 4 of typegen-ci-resilient-describe. Updates the type-generation docs
to describe the committed-types fallback and two-bucket failure taxonomy for
blocking (`--wait`) builds: committed .d.ts as the fallback of record,
--wait never overwriting good types with degraded ones, deterministic
failures (SQL syntax / 404 / 400) crashing vs. environmental failures
(auth / connectivity / deleted / timeout) gating on committed-type presence,
the loud stderr warning, and the run-locally remedy for a first build with no
committed types. Notes the metric-views-only edge case (empty analytics.d.ts
satisfies the gate). Refreshes the metric-view section to reference the same
taxonomy instead of the old always-fail framing.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
…anges

Wash of the typegen-ci-resilient-describe branch: strips loop-process
"Phase N" labels from comments, test names, and describe titles (keeping
the semantic text), removes an unnecessary comment / empty else-block /
useless default parameter, rewrites two stale+duplicated write-suppression
comments to match the actual behavior, and converts errors.test.ts's
`(error as any)` casts to the sibling `Object.assign(new Error(...), { ... })`
idiom. Comments, names, and test-setup style only — no logic or assertion
changes (537 tests still pass).

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Brings in main's revert of PR #502 (#509), which relocated the typegen
cache back to node_modules/.databricks. This branch was cut from the
#502 merge commit and inherited that surface without ever depending on
it, so the sync is a clean subtraction: cache-paths.ts, the committed
apps/dev-playground/.appkit/ fixtures, the cache-serialization tests,
and the warehouse-less-deploy docs all drop out, while the metric-view
runtime (useMetricView, formatters, metric-views.ts codegen) is
untouched.

bundle-size-baseline.json conflicted because both sides regenerated it;
resolved by taking main's, to be regenerated against the merged tree.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
The merge took main's baseline to resolve the conflict; this remeasures
against the post-merge build so the numbers reflect the actual tree.
`size:compare` now reports no change.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Delegating to formatLabel dropped the [^a-zA-Z0-9_-] strip the wrapper
previously applied. data-table passes a raw column.id and
defaultFilterColumn that never pass through SAFE_KEY_REGEX, so the label
echoed arbitrary input; the accompanying test had encoded that as
expected. Restore the strip and correct the expectation.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
The `interactive` flag already carries this explanation where it is
computed, and the ChartOptions prop owns the mechanism.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Signed-off-by: Atila Fassina <atila@fassina.eu>
The useMetricView result table and the format-utilities section both
referred to `MetricColumnMeta`, which is not exported anywhere. The hook
returns `Record<string, MetricViewColumnDisplay>`, so a reader importing
the documented name got a compile error.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
mapToDatum treated every array-valued datum as an [x, y] tuple. Heatmap
data items are [xIndex, yIndex, value] INDEX triples, so onDataClick
received the y *index* as `value` and lost the actual cell value; radar
items hold one value per indicator and were reduced to their second
component.

Branch on `params.seriesType` instead of the datum's shape:

- heatmap reports the cell value, and resolves the two indices back to
  their axis category labels (the raw index when labels are absent), so
  a caller gets the labels the user sees rather than positions. BaseChart
  passes the normalized axis data for this, held in a ref so the click
  subscription still does not re-attach when the data changes.
- radar reports `value: null` rather than an arbitrary component; the
  full vector stays reachable through `raw`.

Line/scatter tuples and scalar bar/pie data are unchanged.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
The JSON_ARRAY wire path delivers every numeric cell as a string (the SQL
connector copies data_array cells verbatim), so a BIGINT or large DECIMAL
measure reaches formatValue as an integer-shaped string. Coercing that
through Number() rounds it: formatValue("9007199254740993", "#,##0")
rendered 9,007,199,254,740,992.

Format oversized integer strings exactly via BigInt instead. This is also
what the existing bigint branch was written for — JSON.parse cannot yield
a bigint, so that branch had no reachable caller and the precision it
protects was lost on the path that actually runs. Both now share one
formatBigInt helper.

Only plain optionally-signed digit strings qualify; fractions and
exponents stay on the Number path, where float semantics are correct.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Selecting a NULL group key built a predicate that matched nothing. The
value was stringified on the way into the filter, so a NULL region
compiled to `region equals 'null'` — a literal string comparison against
a column that is NULL.

toMetricFilter now accepts `null` and compiles it to the grammar's
`notSet`, which the server already renders as IS NULL. Note the
asymmetry: `undefined` means no filter on that dimension, `null` means
filter to the rows where it IS NULL.

A guard in toMetricFilter alone is not enough, since the value was
already a string by the time it arrived, so the playground's producers
pass the real `null` through: table rows, the dropdown domains (NULL
sorted last, labelled "(none)" behind a sentinel because Radix rejects
empty item values), and the filter chips. Chart clicks needed it too —
normalizeChartData maps a NULL category to "", which would otherwise
have compiled to `equals ''`.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Tighten the doc comments across the metric-view client surface: drop
restated rationale, redundant per-field descriptions, and prose that
repeated what the signature already says. Comments only — no behavior,
type, or formatting changes.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
@atilafassina
atilafassina marked this pull request as ready for review August 6, 2026 14:11
@atilafassina
atilafassina requested a review from a team as a code owner August 6, 2026 14:11
Signed-off-by: Atila Fassina <atila@fassina.eu>

# Conflicts:
#	bundle-size-baseline.json

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds the Metric Views client experience to AppKit UI and shared packages: a new useMetricView React hook (SSE-driven, typed rows + metadata), shared SSE lifecycle handling with useAnalyticsQuery, plus framework-agnostic format/filter utilities and chart interaction APIs. Also wires generated Metric View metadata into the dev-playground server and provides an end-to-end playground page + docs + focused tests.

Changes:

  • Introduce shared Metric Filter AST + a toMetricFilter helper, and re-export these types consistently across shared, appkit, and appkit-ui.
  • Add useMetricView and consolidate analytics SSE parsing/handling for both metric-view and analytics-query hooks.
  • Add chart click + selection emphasis support and a dev-playground Metric Views page demonstrating cross-filtering with metadata-driven formatting/labels.

Reviewed changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/shared/src/metric-filter.ts New shared Metric Filter operator vocabulary + filter AST types.
packages/shared/src/index.ts Re-export metric-filter from shared barrel.
packages/appkit/src/plugins/analytics/types.ts Switch metric-filter types to shared source-of-truth; expand analytics SSE error typing.
packages/appkit/src/plugins/analytics/tests/types.test.ts Type-level test ensuring analytics re-exports shared metric-filter types.
packages/appkit/src/plugins/analytics/tests/analytics.integration.test.ts Use ephemeral port + wait-for-listening helper to reduce integration-test flakiness.
packages/appkit/src/plugins/analytics/mv/constants.ts Re-export filter-operator vocab/subsets from shared module.
packages/appkit-ui/src/react/lib/format.ts Delegate label humanization to JS formatter with input sanitization.
packages/appkit-ui/src/react/lib/format.test.ts Add coverage for label formatting + sanitization behavior.
packages/appkit-ui/src/react/hooks/use-metric-view.ts New useMetricView hook implementation using shared analytics SSE lifecycle.
packages/appkit-ui/src/react/hooks/use-analytics-query.ts Refactor to use shared SSE handlers (analytics-sse.ts).
packages/appkit-ui/src/react/hooks/types.ts Add Metric View registry-driven typing utilities + UseMetricView* types.
packages/appkit-ui/src/react/hooks/index.ts Export new Metric View-related types and useMetricView.
packages/appkit-ui/src/react/hooks/analytics-sse.ts New shared SSE parsing/handling helpers for analytics hooks.
packages/appkit-ui/src/react/hooks/tests/use-metric-view.types.test.ts Compile-time probe test for useMetricView type inference/correlation.
packages/appkit-ui/src/react/hooks/tests/use-metric-view.test.ts Behavioral tests for useMetricView (payload, results, errors, abort, warehouse status publishing).
packages/appkit-ui/src/react/hooks/tests/analytics-sse.test.ts Unit tests for shared analytics SSE parsing/handling helpers.
packages/appkit-ui/src/react/charts/utils.ts Re-export shared formatLabel; add click datum mapping helper for ECharts.
packages/appkit-ui/src/react/charts/types.ts Add chart click datum type + onDataClick + controlled selected props.
packages/appkit-ui/src/react/charts/options.ts Add interactive line clickability + selection emphasis transform for categorical series.
packages/appkit-ui/src/react/charts/index.ts Export new chart interaction/selection APIs.
packages/appkit-ui/src/react/charts/base.tsx Wire click handling + selection emphasis into base chart; stabilize event subscription.
packages/appkit-ui/src/react/charts/tests/utils.test.ts Add tests for click datum normalization.
packages/appkit-ui/src/react/charts/tests/options.test.ts Add tests for interactive line options + applySelectionEmphasis behavior.
packages/appkit-ui/src/js/metric-filter/index.ts New JS helper toMetricFilter + re-export shared metric-filter types.
packages/appkit-ui/src/js/metric-filter/index.test.ts Tests for toMetricFilter behavior and type re-exports.
packages/appkit-ui/src/js/index.ts Export new JS format + metric-filter modules.
packages/appkit-ui/src/js/format/index.ts New pure formatting utilities: formatValue, formatLabel, toD3Format (incl bigint-safe paths).
packages/appkit-ui/src/js/format/index.test.ts Tests for format utilities, including bigint precision and currency-prefix coverage.
docs/docs/plugins/analytics.md Document useMetricView, metadata injection, and formatting utilities with examples.
bundle-size-baseline.json Update bundle size baseline after adding new APIs/utilities.
biome.json Exclude generated metric-views artifact from Biome formatting/linting.
apps/dev-playground/shared/appkit-types/metric-views.ts Generated registry augmentation + new exported metricViewsMetadata constant.
apps/dev-playground/server/index.ts Wire generated metricViewsMetadata into analytics({ metricViewsMetadata }).
apps/dev-playground/client/src/routeTree.gen.ts Add route entry for Metric Views playground page.
apps/dev-playground/client/src/routes/metric-views.route.tsx New end-to-end Metric Views playground page demonstrating cross-filtering + metadata-driven formatting.
apps/dev-playground/client/src/lib/nav.ts Add navigation entry for Metric Views page.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apps/dev-playground/client/src/routes/metric-views.route.tsx
Comment thread packages/appkit-ui/src/react/hooks/use-metric-view.ts Outdated
Comment thread apps/dev-playground/client/src/routes/metric-views.route.tsx
Signed-off-by: Atila Fassina <atila@fassina.eu>
…r UI

Radix `Select` throws when an item value is the empty string, and
`toDimensionOptions` keeps `""` as a real value, so a dimension carrying
an empty string crashed the filter dropdown on render. Add an `EMPTY`
sentinel alongside `ALL`/`NONE` and decode it back to `""` so it still
compiles to an `equals ''` filter, distinct from the NULL group's
`IS NULL`.

`FilterBadge` also interpolated the raw selection, rendering "Region:
null" for a NULL group key while the active-filter chip on the same page
showed "(none)" for it. Route the badge through `toDisplayLabel` so both
read the same.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
The note said the memo "enforces referencial equality" — misspelled, and
backwards: serializing to a string is what lets `start`'s dependency
check compare the request by value instead of by object identity.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Signed-off-by: Atila Fassina <atila@fassina.eu>
`buildMetricSql` emitted `LIMIT n` with no `ORDER BY`. In Spark that is a
non-deterministic row sample, not "the top n": the engine returns whichever
rows it produced first, which varies with partitioning, parallelism and cache
state. Measured against a live warehouse, identical SQL returned 3 distinct
result sets across 4 runs, and a KPI card built on such a request could render
a different number run to run with nothing erroring.

Two changes:

- `orderBy?: Array<{ field, direction? }>` on the metric request. Multi-key,
  `ASC`/`DESC` as a closed enum. `field` must be one of the request's own
  measures or dimensions — validated in-request, never against the view's
  declared columns, so the documented no-name-allowlist security model is
  preserved. Entry order is semantic and is never sorted.
- When `limit` is set, the grouped dimensions not already named in `orderBy`
  are appended as tie-breakers. Under `GROUP BY ALL` the dimension tuple is
  unique per row, so the ordering becomes total and `LIMIT` is deterministic.
  Skipped when there are no dimensions (a pure aggregate is one row) or when
  `limit` is absent.

Measures are ordered by their SELECT alias: `ORDER BY MEASURE(`x`)` is rejected
by Spark with METRIC_VIEW_INVALID_MEASURE_FUNCTION_INPUT. Verified against a
live metric view, along with alias binding under `date_trunc`/`GROUP BY ALL`
and the 4-run determinism claim.

BREAKING CHANGE: a metric request that sets `limit` without `orderBy`
previously returned an arbitrary subset of rows and now returns a stable one.
The rows returned may therefore differ from before, and callers who want a
ranked top-N must pass `orderBy` explicitly — the automatic tie-breakers make
the result stable, not ranked.

Signed-off-by: Atila Fassina <atila@fassina.eu>
The exact-formatting path only recognised integer-shaped strings, so a
fractional value off the JSON_ARRAY wire fell through to `Number()` and lost
precision past ±2^53 — a cents-denominated total or a fixed-scale DECIMAL
rendered as a neighbouring value.

Parse plain decimal strings into a fixed-point `{coefficient, negative, scale}`
and format from that, so the value never passes through a JS number. Rounding
is half-away-from-zero, matching `Intl.NumberFormat`'s default. The percent
path now scales the coefficient by 100 exactly instead of refusing the input.
Exponent-form strings still take the float path, having no exact reading.

Signed-off-by: Atila Fassina <atila@fassina.eu>
…tricView

`MetricOrderBy` was exported so callers could name a hoisted `orderBy`
constant, but its `field` was `string` while `UseMetricViewOptions` required the
selected-field literal union — so the exported type could not be used for the
thing it was exported for. Hoisting is the pattern the metric-views route
follows, since the hook's payload memo compares by value.

Make `MetricOrderBy<Field extends string = string>` generic and instantiate it
in `UseMetricViewOptions` at the selected measures/dimensions, replacing the
inline structural duplicate that let the two definitions drift.

The default instantiation stays `field: string`, so the server's wire type
(`IAnalyticsMetricRequest`) is unchanged. A bare `MetricOrderBy[]` is still
rejected by the hook, deliberately: `MetricOrderBy<string>` cannot prove its
fields were selected, and accepting it would forfeit the compile-time guarantee
that you cannot order by an unselected column. Hook callers parameterize with
their selected fields; the JSDoc now says so and the type probe pins both the
accepted and rejected forms.

Signed-off-by: Atila Fassina <atila@fassina.eu>
The `Rule A:` / `Rule B:` labels in the metric request `superRefine` restated
their own `addIssue` messages, and the sibling rules in that same function carry
no such labels. The `renderOrderByClause` lead comment narrated the step rather
than the intent; the ordering-is-caller-significant point it carried is stated
where it is load-bearing, in the cache key.

Comments only — no behaviour change.

Signed-off-by: Atila Fassina <atila@fassina.eu>
`2226fde4` established that an empty-string dimension value is a real value
filtering on `equals ''`, distinct from a NULL group key's `IS NULL`, and gave
the filter dropdown an `EMPTY` sentinel to say so. The chart-click path still
collapsed the two: `fromChartName("")` returned `null`, so clicking an
empty-string category filtered for SQL NULL, and `toSelectionSet` drops `""`, so
that category could never render as selected.

The ambiguity starts upstream of the click — `ChartClickDatum.name` is a string,
and `mapToDatum` derives it from ECharts' own `p.name`, so a NULL group key and
a genuine `""` are already indistinguishable by the time a handler sees them.
Project the dimension into the sentinel space the dropdown already uses before
handing rows to the chart, and decode it back on click. `toSelectionSet`'s
documented "empty selection = no emphasis" guard is untouched: sentinels are
non-empty, so they pass through it.

A dimension with no filter at all stays `undefined` rather than being mapped to
the NONE sentinel, which would have emphasized the NULL bar whenever nothing
was selected.

Signed-off-by: Atila Fassina <atila@fassina.eu>
`formatValue` re-derived `isPercent`, `grouping`, `decimals` and `prefix` from
the format string on every call — two of those run a regex — and callers invoke
it per table cell, so a render redid the same parse rows x columns times.

Memoize the parse keyed by the spec string. The realistic key space is
metric-view catalog metadata, so it is tiny, but the function is public and
takes an arbitrary string: cap the map and stop inserting once full rather than
letting a caller that generates specs in a loop grow it without bound. A miss
past the cap still parses and returns the right value, so correctness never
depends on cache state.

`toD3Format` parsed the same spec separately and now shares the cache.
Per-value work (`parseExactDecimal`, thousands grouping) is unchanged — it
cannot be cached, since it depends on the value rather than the spec.

Signed-off-by: Atila Fassina <atila@fassina.eu>

@pkosiec pkosiec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: useMetricView client experience

Solid PR — the hook, SSE consolidation, and format/filter utilities are well-built. I verified the feature live in the dev-playground against the dogfood warehouse: labels, currency formatting, all filter paths (dropdowns, table-row, chart-click), multi-dimension filters, chip removal, and Clear all all work.

One thing is broken: the time-series line chart scrambles into spaghetti after any filter (details inline). Everything else is UX polish or API consistency.

Findings

  • 🔴 P1 — trend line chart renders unordered → scrambled (reproduced)
  • 🟠 P2 — every filter blanks all visuals to skeletons (flash)
  • 🟠 P2useMetricView doesn't return warehouseStatus (no cold-start UI)
  • nit × 6 — charts ignore metadata format (playground), empty-key throw, autoStart parity, selected scope, leftover console.log, dense comments

Third-party (not this PR, FYI): filtering spams the console with TypeError: Cannot read properties of undefined (reading 'disconnect') (×10 in one session). Traced to size-sensor (resizeObserver.js destroy()), a transitive dep of echarts-for-react — not our code. The rapid chart remount-on-filter (see the skeleton-flash finding) is what surfaces it. Fixing that flash likely quiets it; otherwise a separate ticket / dep note.

Non-blocking (COMMENT). Nothing needs to gate merge except your own call on the P1.

});

// ARR + MRR over time — the hero trend.
const trend = useMetricView("revenue", {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 P1 — trend line chart renders scrambled after any filter.

Reproduced live: pick a region/segment or click a table row → this line chart tangles into criss-crossing lines. It sometimes looks fine unfiltered — that's luck, not correctness.

Why:

  • This query has no orderBy, and the metric route only adds ORDER BY when limit is set. So rows come back in arbitrary order.
  • ECharts draws a line in data-array order, so unordered rows zig-zag.
  • The client's sortTimeSeriesAscending only helps if the rows are fully reversed, not just shuffled.

This is the documented happy path (useMetricView by time → LineChart), so worth fixing at more than one layer:

  • Here (must-fix): add orderBy: [{ field: "created_at", direction: "ASC" }] — one line, and you already have TABLE_ORDER_BY as the pattern.
  • Docs: note that time-series charts need orderBy on the time dimension.
  • Follow-up (pre-existing): make sortTimeSeriesAscending actually sort ascending, not just reverse a descending array — fixes every consumer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's how the graph looks like for me:
image
image

setLoading(true);
setError(null);
setErrorCode(null);
setData(null);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 P2 — every re-query blanks all visuals to skeletons.

start() calls setData(null) on every requery, so a cross-filter click tears down and re-skeletons all four visuals at once — a full-page flash over the warehouse round-trip. Cross-filter is the headline feature, so this is exactly where it should feel smooth.

Suggestion: stale-while-revalidate — keep the previous data during a refetch, show the skeleton only on first load. One caveat: clear data on a key change (old metric's rows are meaningless), but keep it on a filter/payload change.


useQueryHMR(key, start);

return { data, loading, error, errorCode, metadata };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 P2 — hook doesn't return warehouseStatus, so there's no cold-start affordance.

The hook publishes warehouse status to the provider but doesn't return it, unlike useAnalyticsQuery. Without a ResourceStatusProvider mounted (the playground has none), a cold warehouse is just a long blank skeleton with no "warehouse starting…" signal. Combined with the skeleton-flash above, cold starts feel broken.

Suggestion: return warehouseStatus for parity, or document that a provider is required for the cold-start UX.

unpublish: unpublishWarehouseStatus,
} = useAnalyticsWarehousePublisher(publisherId, key);

if (!key || key.trim().length === 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: empty key throws during render. If a metric key comes from a dropdown that's "" for a frame, this crashes the subtree instead of flowing through the existing error state. Consider setting error and skipping the request. Matches useAnalyticsQuery, so it may be intentional — low priority.

!region.error &&
region.data &&
region.data.length > 0 && (
<BarChart

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: charts don't apply the metadata number format. Seen live: the bar tooltip shows 849,732,624 while the table shows $849,732,624.00 — same number, same page. The table uses formatValue(row[col], metadata?.[col]?.format), but the chart gets only data.

toD3Format exists for exactly this. Since this file is the API reference, it'd be nice to show the metadata→chart wiring (thread the d3 specifier through options) so users discover it. Playground-only, so low priority.

timeDimension: Extract<D[number], InferTimeDimensionKeys<K>>;
};

export type UseMetricViewOptions<

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: useMetricView has no autoStart option, but useAnalyticsQuery does. The two hooks read as twins, so the asymmetry surprises. Consider adding autoStart (and maybe a returned refetch) for parity, or noting the omission in the docs.

* cross-filter or selection. Categorical charts (bar, pie/donut) show emphasis;
* other chart types ignore it.
*/
selected?: string | string[];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: selected emphasis only applies to bar/pie/donut; line/area/scatter/heatmap/radar silently ignore it. It's documented on the prop — just worth a line in the chart-integration docs so users don't wire selected to a line chart and wonder why nothing dims.

height={320}
showLegend
onDataClick={(datum) => {
console.log("[Metric Views] Line chart clicked", datum);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: leftover console.log in the reference app. Since this file is the copy-paste template, replace it with a real handler or a commented-out // TODO: cross-filter here showing the intended wiring (the other charts show the pattern).

// `timeDimension` only changes the SQL when `timeGrain` is set (see renderDimensionClause)
const timeDimensionPart =
input.timeGrain != null ? (input.timeDimension ?? "_") : "_";
// `orderBy` is NOT sorted (unlike measures/dimensions) because the sequence is

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comments are dense in a few spots — worth a trim pass.

The intent-comments are good, but some run long:

  • cache.ts here — 12 lines explaining one orderByPart ternary.
  • metric-filter/index.ts — ~54% comments.
  • use-metric-view.ts:98 and a few chart blocks are essay-length.

These read more like design docs than code comments. Suggestion: keep the "why" (e.g. "orderBy not sorted — sequence is semantic under LIMIT") in 1-2 lines and drop the worked-through reasoning. Leaner diff, same signal. Public-API JSDoc with examples is fine to keep.

@pkosiec pkosiec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: useMetricView client experience

Solid PR — the hook, SSE consolidation, and format/filter utilities are well-built. I verified the feature live in the dev-playground against the dogfood warehouse: labels, currency formatting, all filter paths (dropdowns, table-row, chart-click), multi-dimension filters, chip removal, and Clear all all work.

One thing is broken: the time-series line chart scrambles into spaghetti after any filter (details inline). Everything else is UX polish or API consistency.

Findings

  • 🔴 P1 — trend line chart renders unordered → scrambled (reproduced)
  • 🟠 P2 — every filter blanks all visuals to skeletons (flash)
  • 🟠 P2useMetricView doesn't return warehouseStatus (no cold-start UI)
  • nit × 6 — charts ignore metadata format (playground), empty-key throw, autoStart parity, selected scope, leftover console.log, dense comments

Third-party (not this PR, FYI): filtering spams the console with TypeError: Cannot read properties of undefined (reading 'disconnect') (×10 in one session). Traced to size-sensor (resizeObserver.js destroy()), a transitive dep of echarts-for-react — not our code. The rapid chart remount-on-filter (see the skeleton-flash finding) is what surfaces it. Fixing that flash likely quiets it; otherwise a separate ticket / dep note.

Non-blocking (COMMENT). Nothing needs to gate merge except your own call on the P1.

});

// ARR + MRR over time — the hero trend.
const trend = useMetricView("revenue", {

@pkosiec pkosiec Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 P1 — trend line chart intermittently renders as scrambled spaghetti.

Reproduced live (screenshots in thread). It's non-deterministic — a coin flip per cached result, not reliable on demand. That unpredictability is the bug.

Mechanism (all three confirmed):

  1. This query sets no orderBy, and the metric route only emits SQL ORDER BY when limit is set. So rows come back in arbitrary order — verified against the live route: created_at is non-monotonic for both filtered and unfiltered queries.
  2. ECharts draws a line in data-array order, so unordered rows zig-zag.
  3. The client's sortTimeSeriesAscending (charts/utils.ts:327) only sorts when first > last (first row's timestamp is later than the last row's). If that guard happens to fire, you get a full ascending sort → clean chart. If it doesn't, the shuffled rows render as-is → spaghetti. Whether it fires depends purely on the arbitrary arrival/caching order → ~coin flip.

Repro note: you can't force it from the UI reliably — flip a filter and reload a few times, or clear the metric cache, and the same query alternates clean vs scrambled as the cached row order changes.

This is the documented happy path (useMetricView by time → LineChart), so worth fixing at more than one layer:

  • Here (must-fix): add orderBy: [{ field: "created_at", direction: "ASC" }] — one line, and you already have TABLE_ORDER_BY as the pattern.
  • Docs: note that time-series charts need orderBy on the time dimension (the route doesn't order by default without limit).
  • Follow-up (pre-existing): sortTimeSeriesAscending should always sort ascending by x, not only when first > last. Right now a shuffled series that isn't fully descending is left untouched — fixing this hardens every consumer, not just this demo.

setLoading(true);
setError(null);
setErrorCode(null);
setData(null);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 P2 — every re-query blanks all visuals to skeletons.

start() calls setData(null) on every requery, so a cross-filter click tears down and re-skeletons all four visuals at once — a full-page flash over the warehouse round-trip. Cross-filter is the headline feature, so this is exactly where it should feel smooth.

Suggestion: stale-while-revalidate — keep the previous data during a refetch, show the skeleton only on first load. One caveat: clear data on a key change (old metric's rows are meaningless), but keep it on a filter/payload change.


useQueryHMR(key, start);

return { data, loading, error, errorCode, metadata };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 P2 — hook doesn't return warehouseStatus, so there's no cold-start affordance.

The hook publishes warehouse status to the provider but doesn't return it, unlike useAnalyticsQuery. Without a ResourceStatusProvider mounted (the playground has none), a cold warehouse is just a long blank skeleton with no "warehouse starting…" signal. Combined with the skeleton-flash above, cold starts feel broken.

Suggestion: return warehouseStatus for parity, or document that a provider is required for the cold-start UX.

unpublish: unpublishWarehouseStatus,
} = useAnalyticsWarehousePublisher(publisherId, key);

if (!key || key.trim().length === 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: empty key throws during render. If a metric key comes from a dropdown that's "" for a frame, this crashes the subtree instead of flowing through the existing error state. Consider setting error and skipping the request. Matches useAnalyticsQuery, so it may be intentional — low priority.

!region.error &&
region.data &&
region.data.length > 0 && (
<BarChart

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: charts don't apply the metadata number format. Seen live: the bar tooltip shows 849,732,624 while the table shows $849,732,624.00 — same number, same page. The table uses formatValue(row[col], metadata?.[col]?.format), but the chart gets only data.

toD3Format exists for exactly this. Since this file is the API reference, it'd be nice to show the metadata→chart wiring (thread the d3 specifier through options) so users discover it. Playground-only, so low priority.

timeDimension: Extract<D[number], InferTimeDimensionKeys<K>>;
};

export type UseMetricViewOptions<

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: useMetricView has no autoStart option, but useAnalyticsQuery does. The two hooks read as twins, so the asymmetry surprises. Consider adding autoStart (and maybe a returned refetch) for parity, or noting the omission in the docs.

* cross-filter or selection. Categorical charts (bar, pie/donut) show emphasis;
* other chart types ignore it.
*/
selected?: string | string[];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: selected emphasis only applies to bar/pie/donut; line/area/scatter/heatmap/radar silently ignore it. It's documented on the prop — just worth a line in the chart-integration docs so users don't wire selected to a line chart and wonder why nothing dims.

height={320}
showLegend
onDataClick={(datum) => {
console.log("[Metric Views] Line chart clicked", datum);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: leftover console.log in the reference app. Since this file is the copy-paste template, replace it with a real handler or a commented-out // TODO: cross-filter here showing the intended wiring (the other charts show the pattern).

// `timeDimension` only changes the SQL when `timeGrain` is set (see renderDimensionClause)
const timeDimensionPart =
input.timeGrain != null ? (input.timeDimension ?? "_") : "_";
// `orderBy` is NOT sorted (unlike measures/dimensions) because the sequence is

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comments are dense in a few spots — worth a trim pass.

The intent-comments are good, but some run long:

  • cache.ts here — 12 lines explaining one orderByPart ternary.
  • metric-filter/index.ts — ~54% comments.
  • use-metric-view.ts:98 and a few chart blocks are essay-length.

These read more like design docs than code comments. Suggestion: keep the "why" (e.g. "orderBy not sorted — sequence is semantic under LIMIT") in 1-2 lines and drop the worked-through reasoning. Leaner diff, same signal. Public-API JSDoc with examples is fine to keep.

Wash pass over the useMetricView branch:

- drop the orderBy-is-not-sorted rationale duplicated in analytics.ts;
  it stays whole at its canonical site in mv/cache.ts
- collapse the verbatim-duplicated `selected` JSDoc in BaseChartProps to
  a link to ChartBaseProps.selected
- remove a dangling review-finding reference from a test name
- bring back warehouseStartupTimeoutMs's description and
  autoStartWarehouse's cost-control rationale, which an earlier
  comment-trim pass had removed from the IDE hover

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Signed-off-by: Atila Fassina <atila@fassina.eu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants