refactor: extract the shared query model behind dashboards and alerts - #459
Open
Makisuo wants to merge 7 commits into
Open
refactor: extract the shared query model behind dashboards and alerts#459Makisuo wants to merge 7 commits into
Makisuo wants to merge 7 commits into
Conversation
…f package Dashboard widgets and alert rules both persist a query-builder draft, but the schema lived in `@maple/domain/http` — which `@maple/widgets` cannot import, since domain's `MapleApi` embeds the widget schemas and the dependency runs the other way. `packages/widgets/src/index.ts` named this exact blocker: > it stays in `@maple/domain/http` until it gets a leaf of its own. This is that leaf. `@maple/query-model` depends on `@maple/primitives` and `effect` only, so both halves can reach it. Moved in: the query-draft schemas, `TimeRangeSchema` (a neutral value type that alert previews, MCP and the explore pages all resolve), and the metric-type / data-source / signal-source literal unions that were declared twice. New: `QueryBuilderFormulaSchema` replacing three spellings of the same struct, `QueryComparisonSchema`, `QueryResultShape`, `QuerySetSchema` (queries + formulas + comparison — the value both surfaces will store), and one reducer table deriving both the widget spelling and the alert spelling. Two things deliberately NOT collapsed: - The editor-state draft interface in `@maple/query-engine/query-builder` keeps every field required while the stored schema keeps them optional. That split is the point: the builder always holds a populated draft, a stored one omits what the user never set. Merging them would make `whereClause` possibly- undefined at every read in the builder to buy nothing. - `SERIES_REDUCERS` and `ALERT_REDUCERS` stay distinct literal sets. Widgets persist "first", alert rules persist "identity", and they coincide only on a one-bucket window — merging them would rewrite stored values on both sides. Only the table they derive from is shared, which keeps the mapping total. Pure move: every previous export is re-exported from its old home, so no call site changes. 41/41 typecheck tasks pass.
…up keys and reducers Five separate spellings of three concepts, found while mapping what dashboards and alerts already share. Bucket sizing had four implementations, not the one the module header claims: - `packages/query-engine/src/datetime.ts` — canonical. - `apps/api/src/routes/v1/query-engine.http.ts` — a private ladder for raw-SQL `$__interval_s`, whose comment pointed at a web path deleted long ago. - `apps/web/.../query-builder-timeseries.ts` — an alias that just called the shared one under a second name. - `apps/mobile/lib/time-utils.ts` — a genuine copy. The api ladder differed from canonical in exactly two ways: a 30-point target and a 300s floor. `ComputeBucketSecondsOptions` now expresses the floor as `minBucketSeconds`, applied by filtering the ladder BEFORE picking rather than clamping after — clamping would round 120 up to 300 while still having chosen "nearest rung" against rungs the caller cannot use. The floor is load-bearing: a sub-5-minute `$__interval_s` produces exactly the scan the granularity was picked to avoid. Tests pin the deleted ladder's outputs. Mobile keeps its copy: `apps/mobile` has no `@maple/*` dependencies at all, and taking one to share a pure function would pull the query engine's module graph into a Metro bundle. Now says so. `alertWindowBucketSeconds` names the window-is-the-bucket rule that was spelled out at both `compileRulePlan` (which bakes it into the stored spec) and `prepareAlertEvaluation`. Those disagreeing would evaluate a different window than the rule was saved with. `toStorageGroupKey` names the engine-vs-storage group-key boundary. The engine emits `"all"` for an ungrouped result; storage, wire and UI spell it `"__total__"`, and an `alert_rule_states` row keyed `"all"` is invisible to every reader. The translation was open-coded at each site that needed it. Reducers were declared five times: the alert literal set, the widget literal set, a shadowing copy in `settings-fields.tsx` hand-maintained separately from the schema that validates it, and a fifth union in `widget-builder-shared.ts`. All now derive from one table. The two literal SETS stay distinct on purpose — widgets persist "first", rules persist "identity" — but a reducer added to one must now declare whether it has a counterpart in the other. 41/41 typecheck, 243 alert tests, 57 datetime tests pass.
…on v2 Groundwork for the v3 data-source union. Every backend consumer that reached into `dataSource.endpoint` / `dataSource.params` by hand now goes through accessors that read v2 AND v3 identically, so the version flip becomes a small diff instead of a wide one. `access.test.ts` states both shapes per case and asserts one result, so a drift between them fails there rather than at cutover. `dataSourceEndpoint` deliberately returns null for a typed v3 arm instead of synthesising `"custom_query_builder_timeseries"` — inventing a legacy name would quietly re-create the endpoint-string sniffing the union exists to remove. The one place a legacy name is still produced is the `inspect_chart_data` response label, which is a published MCP contract; it is named and isolated. Two fixes found on the way: `migrateToLatest` restamped a document from a NEWER build downward. Unknown versions read as 1, so a rollback would run a v3 document through the whole chain as though it were the oldest shape and then stamp it current. Decode fails either way, but stamped, the next writer persists the lie and the original version is gone. Failing to read is recoverable; corrupting is not. Now returned untouched. Wrote `interpolation-keys.test.ts` — the guard that `display.ts` has been citing by name for the load-bearing `listWhereClause` field, which did not exist. Interpolation picks its formatting by KEY NAME, so renaming that field compiles, passes every schema, and silently changes behaviour. The test pins the real failure shape: a renamed key stops dropping an All-selected clause and expands it to `environment = prd,stg` — a filter matching nothing, where the user asked for no filter at all. 41/41 typecheck; 91 widgets, 224 api mcp/dashboard, 39 query-engine tests pass.
`apps/web` imports `@maple/query-model` from nine files but never declared it. This passed locally and failed in CI because the two installs differ: a full `bun install` hoists every workspace package to the root `node_modules`, so the import resolves whether or not it is declared. CI's web lanes use `install-filters: "@maple/web"` (ci.yml), and a filtered install only links a package's DECLARED dependencies — so `apps/web/node_modules/@maple/query-model` did not exist and typecheck, build and test all failed on TS2307. Verified by running CI's own commands with the cache bypassed: `turbo typecheck|build|test --filter=@maple/web --force` — 3/3 tasks each, 89 test files / 781 tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Groundwork for making dashboard widgets and alert rules share one query model. This is phases 1–3 of a larger refactor; each commit is independently shippable and nothing here changes a persisted schema or a wire shape yet.
Why
Widgets and alert rules describe the same thing — a warehouse query plus how to draw it — but store it incompatibly:
{ endpoint: string, params: Record<string, unknown> }, whereendpointnames a web-side server function andparamsis untyped.queryBuilderDraftplus a server-compiledquerySpecJson.Both lower through the same shared code (
buildTimeseriesQuerySpec,resolveGroupBy). The incompatibility is purely at the storage/type boundary — and it is what forces the downstream duplication: "create an alert from this chart" needs a 287-line translator that sniffs endpoint strings and runtime-validates a bag, the metric-selection hook exists twice, and variable interpolation has to guess which strings are where-clauses by key name.What changed
1.
@maple/query-model— the leaf the codebase already asked for.packages/widgets/src/index.tsnamed this exact blocker:@maple/widgetssits below@maple/domain(domain'sMapleApiembeds the widget schemas), so a widget schema cannot reach up for the draft. The new package depends on@maple/primitives+effectonly, so both halves can import it. It holds the draft schemas,TimeRangeSchema, and a newQuerySetSchema(queries + formulas + comparison) — the value both surfaces will eventually store.Pure move: every previous export is re-exported from its old home, so no call site changed.
2. Collapsed duplicated backend plumbing.
Bucket sizing had four implementations, not the one the module header claims — the canonical one, a private ladder in the raw-SQL route whose comment pointed at a file deleted long ago, an alias that just called the shared one under a second name, and a copy in
apps/mobile. The private ladder differed in exactly two ways (a 30-point target, a 300s floor), soComputeBucketSecondsOptionsnow expresses the floor asminBucketSeconds. Tests pin the deleted ladder's outputs.Reducers were declared five times, including a copy in
settings-fields.tsxthat shadowed the shared set and was hand-maintained separately from the schema validating it. All now derive from one table. The two literal sets stay distinct on purpose — widgets persist"first", rules persist"identity", and they coincide only on a one-bucket window — but a reducer added to one must now declare whether it has a counterpart in the other.toStorageGroupKeynames the engine-vs-storage group-key boundary ("all"vs"__total__"), previously open-coded at each site. Analert_rule_statesrow keyed"all"is invisible to every reader.3. Version-agnostic data-source accessors.
Every backend consumer that reached into
dataSource.endpoint/.paramsby hand now goes through accessors reading both the current shape and the planned typed union.access.test.tsstates both shapes per case and asserts one result, so a drift fails there rather than at cutover. This is what keeps the eventual flip a small diff.Bugs fixed along the way
migrateToLatestrestamped documents from a newer build downward. Unknown versions read as version 1, so a rollback would run a newer document through the whole migration chain as the oldest shape and then stamp it current. Decode fails either way — but stamped, the next writer persists the lie and the original version is gone. Failing to read is recoverable; corrupting is not.display.tshas been citing by name. It points atparams/interpolation-keys.test.tsto protect the load-bearinglistWhereClausefield; that file did not exist. Interpolation picks formatting by key name, so renaming that field compiles, passes every schema, and silently changes behaviour. The test pins the real failure shape: a renamed key stops dropping an All-selected clause and expands it toenvironment = prd,stg— a filter matching nothing, where the user asked for no filter at all.For the reviewer
One finding that changes the plan for the follow-up PR.
apps/mobilefetches/api/dashboards/— the v1 API — and dispatches by readingdataSource.endpoint/.paramsdirectly. v1'sDashboardsListResponseembedsDashboardDocument, the alias bound to the current schema version, so bumping that version changes the v1 wire. Plans for the typed union protect the v2 wire (which declares its own shape and has a parity test); nothing protects v1.Mobile also has zero
@maple/*dependencies by design, so it will not fail to typecheck when a shared schema changes — it breaks at runtime, in a build that cannot be force-updated. The cutover must therefore encode back to the legacy{endpoint, params}shape on both v1 and v2, with parity tests on each. Nothing in this PR is affected.Two places I deliberately did not dedupe, because the apparent duplication is real distinction:
"", not absent); a stored one omits what the user never set. Merging them would makewhereClausepossibly-undefined at every read in the builder to buy nothing.apps/web/.../timeseries-utils.tslooks like a fifth bucket-sizing copy but already delegates to the canonical one, adding string parsing and invalid-input fallback. Left alone.apps/mobilekeeps its own copy ofcomputeBucketSecondsfor the dependency reason above; it now says so in a comment.Verification
bun typecheck— 41/41 tasks pass, on every commit.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.