Skip to content

chore: Metric Views typegen#433

Open
atilafassina wants to merge 8 commits into
mainfrom
mv-typegen
Open

chore: Metric Views typegen#433
atilafassina wants to merge 8 commits into
mainfrom
mv-typegen

Conversation

@atilafassina

@atilafassina atilafassina commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

When config/queries/metric-views.json is present, the type generator runs DESCRIBE TABLE EXTENDED ... AS JSON per declared metric view and emits two artifacts:

  1. shared/appkit-types/metric.d.ts — the MetricRegistry module augmentation. Each entry carries typed measures/dimensions row fields plus measureKeys/dimensionKeys/timeGrains literal unions, the base the MeasureKey<K>/DimensionKey<K>/MetricRow<K>/TimeGrain<K> helpers derive from on the appkit-ui side.

  2. shared/appkit-types/metrics.metadata.json — the semantic-metadata bundle, entries shaped { measures, dimensions } (display names, format specs, descriptions, time-grain hints). Frontend-safe by construction: UC FQNs and execution lanes are deliberately excluded.

Note

Dormancy invariant: absent config means nothing executes — zero artifacts, zero logs, no fallback to any legacy filename. Apps that never adopt metric views see no change, which is what keeps merging this incrementally safe ahead of the runtime.

Output

The schema already exists in main, so it's not part of this PR.
This feature creates the artifact according to the defined schema.

config/queries/metric-views.json, entity-first metricViews map per the #429 metric-source schema:

{
  "metricViews": {
    "revenue": { "source": "main.finance.revenue_metrics" },
    "customer_metrics": { "source": "main.cs.customer_metrics", "executor": "user" }
  }
}

executor is app_service_principal (default) or user (per-user OBO); the internal sp/obo lane is derived at the parse boundary, so downstream code only ever sees lanes.

Failure semantics

  • Config absent → fully dormant.
  • Broken config (unparseable JSON, unknown fields, invalid keys/FQNs/executors) → throws loudly at build time.
  • Per-key DESCRIBE failures (typo'd view, permissions) → warn-and-continue, shipping empty measure/dimension allowlists for that key. This arms the runtime's fail-closed gate (next PR in the chain): an empty allowlist means the route 503s rather than passing unvalidated identifiers through.

Vite plugin grows mvOutFile/mvMetadataOutFile options, and the dev watcher regenerates on metric-views.json edits through the exact same single-flight regen flow as .sql files.

Adaptive DESCRIBE format per warehouse

Some warehouses have opposite requirements for DESCRIBE … AS JSON:

disposition + format ... ...
INLINE + JSON_ARRAY data_array merge_json_arrays
INLINE + ARROW_STREAM ❌ rejected ✅ attachment
EXTERNAL_LINKS + ARROW_STREAM ❌ not implemented

Manual Testing

  1. Auth a profile (if not already): databricks auth login -p DEFAULT
  2. Pick a warehouse you own. PRO/CLASSIC exercises the JSON_ARRAY path (the common case); type=REYDEN exercises the ARROW_STREAM
    fallback:
  databricks warehouses list -p DEFAULT -o json \
    | jq -r '.[] | select(.creator_name=="<your_account_email>") | "\(.id)\t\(.warehouse_type)\t\(.name)"'
  # → 1075664542a32710  PRO  revenue_arr_demo
  1. The metric views referenced in the config must exist in that workspace.

  2. Build the SDK (so the CLI runs the patched describeAdaptive)

pnpm --filter=@databricks/appkit build:package
  1. Create the test harness config
  mkdir -p /tmp/mv-typegen-test/config/queries
  cat > /tmp/mv-typegen-test/config/queries/metric-views.json <<'JSON'
  {
    "$schema": "https://databricks.github.io/appkit/schemas/metric-source.schema.json",
    "metricViews": {
      "revenue":   { "source": "appkit_demo.public.revenue_metrics" },
      "customers": { "source": "appkit_demo.public.customer_metrics", "executor": "user" }
    }
  }
  JSON

(Point source at metric views that exist in your workspace.)

  1. Run typegen against the warehouse
  DATABRICKS_CONFIG_PROFILE=DEFAULT DATABRICKS_WAREHOUSE_ID=1075...2710 \
    pnpm exec tsx packages/shared/src/cli/index.ts generate-types \
    /tmp/mv-typegen-test /tmp/mv-typegen-test/shared/appkit-types/analytics.d.ts \
    --no-cache --wait
  - --no-cache → forces a fresh DESCRIBE (actually hits the warehouse).
  - --wait → blocks until RUNNING (auto-starts a stopped serverless warehouse).
  • The explicit 2nd arg (outFile) keeps output under /tmp/... instead of polluting the worktree; warehouse ID comes from the env
    var.
  1. Verify (expected: success, no degrade)

(a) no format errors:
the run output should NOT contain INVALID_PARAMETER_VALUE / merge_json_arrays / "metric sync failed"

(b) real unions (not string) in the generated types:

  grep -E "measureKeys:|dimensionKeys:" /tmp/mv-typegen-test/shared/appkit-types/metric.d.ts
  #   expect e.g.  measureKeys: "mrr" | "arr" | "new_arr" | "churned_arr";

(c) cache shows not-degraded (cache lives under the cwd = worktree):

  jq '.metrics | to_entries[] | {key, degraded: .value.schema.degraded, retry: .value.retry}' \
    node_modules/.databricks/appkit/.appkit-types-cache.json

  #   expect degraded: null, retry: false  for each metric

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 build-time type generation for Unity Catalog Metric Views, emitting a MetricRegistry module augmentation plus a frontend-safe semantic metadata bundle when config/queries/metric-views.json is present.

Changes:

  • Introduces metric-view config parsing/validation + DESCRIBE-driven schema extraction, emitting metric.d.ts and metrics.metadata.json.
  • Extends the Vite typegen plugin with metric output options and watcher support for metric-views.json changes.
  • Adds extensive unit + snapshot coverage for metric registry generation and plugin option plumbing.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/appkit/src/type-generator/vite-plugin.ts Adds metric output options and triggers regeneration on metric-views.json edits.
packages/appkit/src/type-generator/index.ts Wires metric-view generation into generateFromEntryPoint and exports metric artifact constants/types.
packages/appkit/src/type-generator/metric-registry.ts Implements metric config resolution, DESCRIBE parsing, type/metadata emission, and sync failure reporting.
packages/appkit/src/type-generator/tests/vite-plugin.test.ts Tests watcher behavior for metric-views.json and option plumbing for metric outputs.
packages/appkit/src/type-generator/tests/index.test.ts Tests end-to-end emission/dormancy behavior for metric artifacts in generateFromEntryPoint.
packages/appkit/src/type-generator/tests/metric-registry.test.ts Adds comprehensive unit tests for config validation, extraction, time grains, and metadata formatting.
packages/appkit/src/type-generator/tests/snapshots/metric-registry.test.ts.snap Snapshot coverage for emitted metric.d.ts and metrics.metadata.json.

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

Comment thread packages/appkit/src/type-generator/index.ts Outdated
Comment thread packages/appkit/src/type-generator/metric-registry.ts Outdated
Comment thread packages/appkit/src/type-generator/metric-registry.ts Outdated
Comment thread packages/appkit/src/type-generator/metric-registry.ts Outdated
@atilafassina atilafassina marked this pull request as draft June 10, 2026 18:31
@atilafassina atilafassina changed the title feat(appkit): metric-registry type generation chore: Metric Views typegen Jun 10, 2026
atilafassina added a commit that referenced this pull request Jun 10, 2026
…c failure logs

Copilot review response (#433): (1) the metric emit block now gates DESCRIBEs
on warehouse state in non-blocking mode — one read-only status GET (never
starts a warehouse); when not RUNNING (or the probe fails) it skips all
DESCRIBEs and emits degraded artifacts (every configured key with empty
measures/dimensions) that the vite plugin's warehouse-watch regen (blocking
mode) refreshes once the warehouse is up. Blocking mode and injected
metricFetcher bypass the gate. (2) syncMetrics is now log-free: the three
internal per-failure warns are removed and the generateFromEntryPoint caller
owns surfacing failures exactly once.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
@atilafassina atilafassina force-pushed the mv-typegen branch 3 times, most recently from e66fc62 to e38d0f2 Compare June 17, 2026 15:19
Build-time type generator for UC Metric Views: reads
config/queries/metric-views.json, runs DESCRIBE TABLE EXTENDED per
declared view, and emits the MetricRegistry .d.ts augmentation
(metric.d.ts) plus the metrics.metadata.json semantic bundle.
Non-blocking-mode aware (degraded types when the warehouse is
unavailable), blocking-mode warehouse preflight, bounded-concurrency
DESCRIBEs, and a retry-driven describe cache with last-known-good
degradation.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Config caps (200 metric views, 255 chars per FQN segment, 767 total,
100 decimal places), reject metricViews:null, backtick-quote validated
FQN segments in DESCRIBE, null-prototype metadata bundle, exact-basename
watcher match, and locale-independent artifact key order. Cache: sticky
vs transient retry classification, pruning to the configured key set,
and structural validation of revived entries.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Failure-outcome helper, unified renderer block builders, currency-symbol
map, parallel-array removal, hoisted allowlist sets, and relocation of
the revival validator + cache-hash helper into cache.ts. The Vite plugin
now defers metric artifact defaults to the generator so plugin- and
CLI-driven runs agree under a custom outFile.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
SDK executeStatement returns ARROW_STREAM by default (rows in
result.attachment, data_array empty), so the metric and query DESCRIBE
fetchers silently degraded on warehouses that don't default to
JSON_ARRAY. Add normalizeResultRows (apache-arrow tableFromIPC) and
request ARROW_STREAM + INLINE in both fetchers; downstream parsers read
the populated data_array unchanged. Verified live against a real
warehouse: real measure/dimension unions, cache no longer degraded.

Hardening: refuse to emit partial types when a DESCRIBE result is
multi-chunk (next_chunk_* present) — fail loud rather than cache a
truncated schema; extract row values via the positional StructRow
iterator ([...row]) rather than Object.values, which reorders
integer-named columns.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
Split the ~1.4k-line metric-registry.ts into focused mv-registry/
modules (config, describe, metadata, render-types, sync, types).
Consumers import directly from the relevant submodule; there is no
aggregating barrel. The package's public type surface is unchanged —
type-generator/index.ts still re-exports the same metric types from
mv-registry/types. Behavior-preserving: 2962 tests green and the
dogfood live run still emits real metric types.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
The prior Arrow fix hardcoded INLINE+ARROW_STREAM for DESCRIBE, which only the
Reyden engine accepts. Standard DBSQL (PRO/CLASSIC) rejects that pairing and
requires JSON_ARRAY, so metric and query typegen broke on real warehouses. The
two engines have opposite requirements — no single hardcoded format works on
both.

describeAdaptive tries JSON_ARRAY first and falls back to ARROW_STREAM only when
the warehouse rejects the format (merge_json_arrays / disposition mismatch),
memoizing the accepted format per run. SQL errors, degrades, and connectivity
failures pass through unchanged. The Arrow decoder stays as the Reyden branch.

Covers the metric (describe.ts) and query (query-registry.ts) paths. Verified
live on revenue_arr_demo (PRO) and a type=REYDEN warehouse.

Co-authored-by: Isaac
Signed-off-by: Atila Fassina <atila@fassina.eu>
@atilafassina atilafassina marked this pull request as ready for review June 18, 2026 13:17
Comment on lines +83 to +86
export function isValidFqn(fqn: string): boolean {
return /^[a-zA-Z0-9_][a-zA-Z0-9_-]*\.[a-zA-Z0-9_][a-zA-Z0-9_-]*\.[a-zA-Z0-9_][a-zA-Z0-9_-]*$/.test(
fqn,
);

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.

Looks like this regex is more restrictive than the UC identifier naming limitations? https://docs.databricks.com/aws/en/sql/language-manual/sql-ref-names

Should we make this regex 100% accurate if based on it we might fail the production build?

@pkosiec

pkosiec commented Jun 19, 2026

Copy link
Copy Markdown
Member

Tried to post the comments separately but encountering some issues with Claude, so here's an aggregated version:

[blocking] comments

B1 — mv-registry/sync.ts:73-75 — classify DESCRIBE errors instead of always-transient

Comment:

Every fetcher() throw is hard-coded transient: true, so auth failures, bad warehouse
IDs, permission errors and the deterministic "multi-chunk (truncated)" throw
(statement-result.ts:32) all cache retry: true and re-describe forever, never failing
the build
— even in blocking/production mode. The query path does the opposite:
query-registry.ts:803-830 runs throws through isConnectivityError() and routes
non-connectivity failures to fatalErrors. Please mirror that here so a real
misconfiguration is surfaced (and pinned sticky) rather than silently shipping permissive
types. isConnectivityError currently lives unexported in query-registry.ts; extract it
(e.g. into statement-result.ts or a small errors.ts) and reuse it in both paths.

Sketch for the catch in describeOne:

} catch (err) {
  const reason = `DESCRIBE TABLE EXTENDED failed: ${(err as Error).message}`;
  // connectivity → transient (self-converges); everything else (auth, bad id,
  // truncated/multi-chunk, malformed request) → deterministic, must surface.
  return failedOutcome(index, entry, reason, isConnectivityError(err));
}

B2 — index.ts:246-255 & index.ts:456-459 — stop swallowing auth/timeout into "not running"

Comment (post on probeWarehouseState, 246-255):

The bare catch { return undefined } makes a 403 / expired token indistinguishable from a
transient not-running warehouse: the gate reads undefined, skips DESCRIBE, and emits
degraded retry: true types on every pass with no error ever logged. Mirror
query-registry.ts:633-642 — return undefined only for isConnectivityError, and let
auth/config errors propagate so the gate can fail or pin sticky.

Comment (post on the blocking-preflight catch {}, 456-459):

This empty catch swallows a timed-out waitUntilRunning and auth errors, then falls
through to syncMetrics against a not-ready warehouse (a ~5-min stall that still "succeeds").
Same fix as the query path: connectivity → degrade; auth / bad id / timeout → set
preflightFatalMessage. This and B1 are one root cause (error classification) — fix together.

B3 — statement-result.ts:125-133isFormatRejection misreads genuine errors

Comment:

The third clause m.includes("disposition") && m.includes("format") matches a broad class
of real Databricks errors (e.g. [INVALID_PARAMETER_VALUE] disposition must be one of INLINE…; format must be JSON_ARRAY…). A genuine error then gets retried under
ARROW_STREAM and its original diagnostic is discarded — and with B1 it loops forever.
Please tighten to the exact known rejection signatures and gate on a missing SQL
error_code, then add a test asserting a real SQL error is not treated as a format
rejection. Suggestion (confirm the exact standard-DBSQL string against a live PRO warehouse
before merging):

function isFormatRejection(
  message: string | undefined,
  errorCode?: string,
): boolean {
  if (!message) return false;
  // A statement that actually ran and produced a SQL error is never a format
  // rejection — only the request-shape rejections below qualify.
  if (errorCode && errorCode !== "INVALID_PARAMETER_VALUE") return false;
  const m = message.toLowerCase();
  return (
    m.includes("merge_json_arrays") || // Reyden rejects JSON_ARRAY on AS JSON
    m.includes("arrow_stream") // standard DBSQL rejects ARROW_STREAM + INLINE
  );
}

(Update both call sites to pass normalized.status.error?.error_code / the thrown error's
code.)


[should-fix] comments

S1 — packages/appkit/package.json:80apache-arrow shouldn't be a hard dep

Comment:

apache-arrow@21.1.0 is ~5.4 MB unpacked and is only import()-ed at build time, and only
when a warehouse returns an ARROW_STREAM attachment (Reyden). As a dependencies entry it
is force-installed on every @databricks/appkit consumer, including those with no metric
views / a JSON_ARRAY warehouse. Move it to optionalDependencies (the try/catch at
statement-result.ts:50-84 already degrades gracefully if it's absent). Pair this with S2 so
the degrade isn't silent.

// remove from "dependencies"; add:
"optionalDependencies": {
  "apache-arrow": "21.1.0"
}

S2 — statement-result.ts:80-84 — don't silently swallow Arrow decode failure

Comment:

The catch returns the response unchanged, routing into a "no rows" degrade with no signal —
so a corrupt payload or a missing apache-arrow (after S1) looks identical to an empty
result. Add a logger.warn here naming the cause so a Reyden user whose decode failed gets a
breadcrumb instead of mysteriously-empty metric types.

S3 — mv-registry/render-types.ts:47 — escape */ in the raw @sqlType JSDoc sink

Comment:

col.type is interpolated raw into the generated JSDoc, while every other warehouse-derived
string here is JSON.stringify'd. A */ in the type closes the comment early and lands
residual tokens as top-level declarations in metric.d.ts (build-integrity / output
injection; not RCE since .d.ts isn't executed). Same pre-existing sink at
query-registry.ts:297-298 — worth fixing there too.

        return `${indent}/** @sqlType ${col.type.replace(/\*\//g, "* /")}${grainComment} */
${indent}${JSON.stringify(col.name)}: ${tsTypeFor(col.type)}`;

S4 — packages/shared/src/schemas/metric-source.ts:42-74 — the "canonical" schema doesn't enforce what the build does

Comment:

The input caps (MAX_METRIC_VIEWS=200, MAX_FQN_LENGTH=767, segment 255) live only in
the imperative config.ts validator. But the rules are advertised in three places that are
meant to agree: (1) config.ts at build time, (2) this Zod schema — called the "canonical
contract for IDE/CI" — and (3) the published metric-source.schema.json that users get live
validation from via the $schema field. (2) and (3) have the regexes but not the caps, so
a 201-view (or 300-char-segment) config shows green in the editor and CI, then the build throws
"exceeds the maximum of 200" — the "canonical" contract was wrong. Two ways to fix; please pick:

(a) Mirror the caps here (and regenerate the JSON Schema) so the editor catches it:

source: z
  .string()
  .max(767)
  .refine((s) => s.split(".").every((seg) => seg.length <= 255), "each FQN segment ≤ 255 chars")
  .regex(/^[a-zA-Z0-9_][a-zA-Z0-9_-]*\.[a-zA-Z0-9_][a-zA-Z0-9_-]*\.[a-zA-Z0-9_][a-zA-Z0-9_-]*$/)
  // …
metricViews: z
  .record(metricKeySchema, metricEntrySchema)
  .refine((m) => Object.keys(m).length <= 200, "at most 200 metric views")
  .optional()

(z.record has no key-count limiter, hence the .refine; the per-segment .refine is only
needed if you want 255 enforced — 767-total alone doesn't bound a single segment. Then extend
the resolveMetricConfig ↔ schema parity test to cover the caps.)

(b) Or decide the caps are runtime-only defense and add a one-line comment in config.ts
saying they're intentionally not part of the published contract — no schema change.

(context7-verified for Zod 4: two-arg z.record(key, value), .refine() chains before
.optional(), z.string().max() valid; string-form refine message for version-safety.)

S5 — statement-result.ts:102-113errorMessageOf duplicates getErrorMessage

Comment:

errorMessageOf here is a line-for-line duplicate of getErrorMessage in
query-registry.ts:106-112 (same instanceof Error → object-messageString(error)
ladder), added new in this branch. Extract one copy into a shared internal util (e.g.
type-generator/errors.ts) and import it from both — this is also the natural home for the
isConnectivityError you'll need to share for B1.


[nit] comments (slop / staleness cleanup)

N1 — index.ts:52 — double-underscore typo MV__PREFLIGHT_WAIT_MAX_MS

Comment: nit: Rename artifact from the metric*mv* commit; the query twin is
PREFLIGHT_WAIT_MAX_MS. Single underscore, and update the two usages at 430 and 445.

const MV_PREFLIGHT_WAIT_MAX_MS = 300_000;

N2 — mv-registry/config.ts:34 — self-referential stale comment

Comment: nit: {@link MV_CONFIG_FILE} resolves to the current file, so this reads "no
fallback to the legacy metric-views.json filename". The original meaning was the legacy
metric.json.

 * additive — apps without metric-views.json must not be penalized). There is
 * deliberately no fallback to the legacy `metric.json` filename.

N3 — mv-registry/sync.ts:45@link to a non-exported const

Comment: nit: MV_CONFIG_FILE isn't exported from config.ts, so this link doesn't resolve.

 * Run schema synchronization for every entry in `metric-views.json`.

N4 — packages/shared/src/schemas/metric-source.ts:4 — schema header still says metric.json

Comment: nit: The file is metric-views.json; the header (and the metricViews description at
line 78) predate the rename.

 * Single source of truth for `metric-views.json`

N5 — mv-registry/config.ts:15-17 — annotate the magic numbers

Comment: nit: Reviewers asked what these mean; a one-liner each removes the question.

// Safety cap on declared metric views (typo / DoS guard — not a UC limit).
const MAX_METRIC_VIEWS = 200;
// Unity Catalog's maximum identifier length, per FQN segment.
const MAX_FQN_SEGMENT_LENGTH = 255;
// Longest possible 3-part FQN: 3 × 255-char segments + 2 dot separators.
const MAX_FQN_LENGTH = 767;

N6 — incomplete metric*mv* internal rename (single comment, list the spots)

Comment: nit: The directory is mv-registry/ and the public params are mvOutFile/
mvMetadataOutFile, but these internals still use the old prefix — please finish the rename
for consistency (leave the public Metric* types and METRIC_*_FILE exports alone unless
you want a deliberate breaking change):

  • index.ts: metricsSection (349), metricClient/getMetricClient (398-401),
    metricSchemas (610), metricFile (619), metricDeclarations (621)
  • sync.ts:35: METRIC_DESCRIBE_CONCURRENCYMV_DESCRIBE_CONCURRENCY
  • rename tests/metric-registry.test.ts (+ .snap) → mv-registry.test.ts (it imports only
    ../mv-registry/*; the old metric-registry.ts was deleted in 1941fc5)

N7 — mv-registry/render-types.ts:6 — the @todo unify with query-registry.ts is real debt

Comment: nit: tsTypeFor duplicates query-registry.ts:974 typeMap with divergent
coverage (tsTypeFor adds INTEGER/NUMERIC but silently drops DATE, TIMESTAMP,
ARRAY, MAP, STRUCT, VARIANT, spatial, VOID to string). Extract one shared
sqlTypeToTs() and add a parity test. (OK as a follow-up if tracked.)

N8 — cache.ts:61 — validator should accept unknown, not MetricCacheEntry

Comment: nit: isRevivableMetricCacheEntry exists to guard against corrupted cache JSON, but it
types its param as MetricCacheEntry (already-valid) and then re-widens with entry.schema as unknown inside. That's a contradiction — type it unknown so the type system expresses
"unverified input" and the internal cast disappears. Callers still pass MetricCacheEntry
(assignable to unknown), so it's a safe signature relaxation.

export function isRevivableMetricCacheEntry(entry: unknown): boolean {
  if (typeof entry !== "object" || entry === null) return false;
  const e = entry as Record<string, unknown>;
  if (typeof e.hash !== "string" || typeof e.retry !== "boolean") {
    return false;
  }
  const schema = e.schema;

(Adjust the remaining body to read through e/schema as unknown.)

N9 — index.ts:613-616?? emptyMetricSchema(entry) arm labelled // Unreachable

Comment: nit: A dead branch with an // Unreachable comment is a classic AI-slop tell — it
exists only to satisfy the type checker. If the invariant truly holds, assert it loudly
instead of silently emitting a degraded schema for a state that "can't happen":

      const metricSchemas = resolution.entries.map((entry) => {
        const schema =
          hitSchemas.get(entry.key) ?? describedByKey.get(entry.key);
        if (!schema) {
          throw new Error(
            `[appkit] unreachable: no schema for metric key "${entry.key}"`,
          );
        }
        return schema;
      });

(Trade-off worth a sentence in the reply: this turns a "can't happen" into a hard fail
rather than a silent degrade — appropriate for build tooling, but flag it so it's a
conscious choice.)

N10 — mv-registry/config.ts:109-113/** */ doc-comment inside a function body

Comment: nit: This /** … */ block sits inside resolveMetricConfig, not attached to a
declaration, so IDE hover / TypeDoc ignore it — and it's longer than any other inline comment
in the file. Convert to a plain // and trim:

  // Default to {} only when metricViews is genuinely absent; a `null` must fall
  // through to the type-check below and throw (the canonical Zod schema rejects null).
  const metricViews =
    config.metricViews === undefined ? {} : config.metricViews;

N11 — mv-registry/config.ts:19-27 & 70-78 — trim non-rendering / redundant JSDoc

Comment (compareKeys, 19-27): nit: @note isn't a standard TSDoc tag (silently dropped by
tooling). State why locale-independence matters instead — sort() without a comparator is
locale-sensitive, which would reorder keys across environments and invalidate cache hashes.
Comment (isValidMetricKey, 70-78): nit: the file path
packages/shared/src/schemas/metric-source.ts is repeated after the {@link} that already
points there — drop the redundant literal path.


[question] comments

Q1 — mv-registry/render-types.ts:161MetricRegistry has no base interface

Comment: Unlike QueryRegistry / ServingEndpointRegistry (both declared in
packages/appkit-ui/src/react/hooks/types.ts), MetricRegistry has no empty base in
@databricks/appkit-ui/react. The augmentation compiles, but the type is unreachable until a
consumer ships. Is the base interface (and the useMetricView hook) landing in a sibling PR?
If so, link it; if not, add the base declaration here.

Q2 — index.ts:690 — JSDoc references APIs that don't exist yet

Comment: This points apps at @databricks/appkit-ui/format's registerMetricsMetadata(),
but neither the export nor the function exists in appkit-ui today — following the doc gives a
module-resolution error. Confirm it's a forward-reference to a sibling PR (and link it), or
soften the JSDoc until it lands.

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