Feat/sdk query#143
Open
Vunovati wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete type-safe query SDK for KopaiQuery across traces, logs, and metrics signals. It adds Zod-validated wire schemas, a fluent TypeScript builder with compile-time row inference, Fastify query endpoints with error mapping, SQLite query translation and execution with pagination, SDK client integration, datasource implementations, and comprehensive test coverage. ChangesKopaiQuery Type-Safe SDK Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/sqlite-datasource/src/__fixtures__/insert-helpers.ts (1)
448-451: 💤 Low valuePotential
undefinedvalue when array lengths mismatch.If
opts.quantiles.length > opts.quantileValues.length, the mapping will produceundefinedvalues forvalue. Consider adding a length check or using a safer access pattern.🛡️ Suggested defensive fix
- const quantileValues = opts.quantiles.map((q, i) => ({ - quantile: q, - value: opts.quantileValues[i], - })); + const quantileValues = opts.quantiles.map((q, i) => ({ + quantile: q, + value: opts.quantileValues[i] ?? 0, + }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sqlite-datasource/src/__fixtures__/insert-helpers.ts` around lines 448 - 451, The mapping that builds quantileValues can produce undefined when opts.quantiles is longer than opts.quantileValues; update the logic in the quantileValues construction (the const quantileValues = opts.quantiles.map(...) block) to defensively handle mismatched lengths by either validating lengths up front and throwing a clear error, or by using a safe access pattern (e.g., defaulting value to null/0 or skipping entries when opts.quantileValues[i] is undefined) so no undefined values are produced; ensure any downstream consumers of quantileValues are adjusted to expect the chosen default or the validation error.packages/sqlite-datasource/src/kopai-query-translator.ts (1)
311-347: 💤 Low valueDocument the supported aggregation functions or verify completeness.
buildAggExprsupportscount,countDistinct,sum,avg,min,maxbut the SDK'stracesAggalso exposes quantile functions (p50,p75,p90,p95,p99,p999) andtopN. These will throwSqliteDatasourceNotImplementedErrorat runtime.This is acceptable if quantiles/topN are intentionally unsupported in SQLite, but consider adding a comment clarifying this limitation or updating the error message to list only the truly supported functions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sqlite-datasource/src/kopai-query-translator.ts` around lines 311 - 347, buildAggExpr currently implements only count, countDistinct, sum, avg, min, max and will throw SqliteDatasourceNotImplementedError for quantile functions (p50, p75, p90, p95, p99, p999) and topN exposed by tracesAgg; update buildAggExpr or its error/message to be explicit: either add a brief comment above buildAggExpr noting that quantiles and topN are intentionally unsupported on SQLite, or change the SqliteDatasourceNotImplementedError string in buildAggExpr to list only the supported functions (count, countDistinct, sum, avg, min, max) and mention that quantiles/topN are not available; reference symbols: buildAggExpr, tracesAgg, SqliteDatasourceNotImplementedError.packages/sdk/src/query/aggs.ts (1)
149-168: 💤 Low valueConsider adding
countandcountDistincttometricsAgg.Unlike
tracesAggandlogsAgg,metricsAggis missingcount()andcountDistinct(). If this is intentional (e.g., metrics queries don't support row counting), please add a brief comment. Otherwise, consider adding them for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/query/aggs.ts` around lines 149 - 168, metricsAgg is missing count() and countDistinct() unlike tracesAgg and logsAgg; add count and countDistinct entries to the exported metricsAgg constant (using the same helper implementations those other modules use—e.g., the count/countDistinct agg helpers or the same makeAgg wrapper used elsewhere) so they mirror tracesAgg/logsAgg signatures for MetricsColumnName (e.g., count: countAgg<MetricsColumnName>("count"), countDistinct: countDistinctAgg<MetricsColumnName>("countDistinct")), or if omission is intentional add a short inline comment above metricsAgg explaining why counts are unsupported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/kopai-query-zod.ts`:
- Around line 286-301: The schema docs for timeRangeSchema are inconsistent: the
`end` field is described as "(exclusive)" while the overall description calls it
a "Closed time range" and the translator uses `<=` (inclusive) at comparator in
kopai-query-translator.ts; update the `end` field description and the overall
.describe text on timeRangeSchema to state that both `start` and `end` are
inclusive (e.g., "end (inclusive)" and "Closed (inclusive) time range") so the
schema documentation matches the behavior in the translator (where comparisons
use <=).
In `@packages/sdk/src/client.ts`:
- Around line 93-96: The runtime schema executeResponseSchema must enforce the
ExecuteResult contract: replace the loose schema with two explicit schemas—one
for non-aggregated results (executeResponseSchemaNonAgg) and one for aggregated
results (executeResponseSchemaAgg); make cursor required (z.string().nullable())
on the non-agg schema (remove optional()) and change rows to an array of objects
(e.g., z.array(z.record(z.unknown())) or z.array(z.object({}).passthrough()) so
scalars/arrays are rejected). Update the code paths that currently force-cast
the schema (the site referenced in the comment where the schema is cast on use)
to validate against the correct schema based on IsAgg/isAggregated instead of
casting, returning a validation error if the non-agg response is missing cursor.
Ensure references: executeResponseSchema, executeResponseSchemaNonAgg,
executeResponseSchemaAgg, and the usage location that currently force-casts the
schema are updated accordingly.
In `@packages/sdk/src/query/builder.ts`:
- Around line 58-60: Select, groupBy and orderBy accept unconstrained ColumnRef
types causing cross-builder refs to compile; tighten types by threading the
builder's active signal type into the SelectValue/SelectMap alias and into
function signatures so ColumnRef is parameterized by the builder signal (e.g.
change SelectValue from ColumnRef<string, unknown, Kind> to ColumnRef<string,
unknown, SignalType> and update SelectMap, and the select, groupBy, orderBy, and
toQuery-related signatures to use that SignalType), ensuring all uses of
ColumnRef in this file (including the symbols SelectValue, SelectMap, select,
groupBy, orderBy, and toQuery) are constrained to the builder's signal generic.
In `@packages/sqlite-datasource/src/kopai-query-translator.ts`:
- Around line 402-420: The function signature for buildQuery incorrectly
requires an intersection type MetricsKopaiQuery & { signal: "metrics" } which is
never used because signal comes from SignalContext; remove the unnecessary
intersection and change the union arm to just MetricsKopaiQuery (i.e., make the
parameter type TracesKopaiQuery | LogsKopaiQuery | MetricsKopaiQuery), then
audit the body of buildQuery to replace any uses of query.signal with ctx.signal
and ensure any type-narrowing logic relies on ctx.signal or explicit
discriminants rather than the removed intersection.
In `@packages/ui-core/src/hooks/use-kopai-data.ts`:
- Around line 55-61: The switch in useKopaiData currently throws for the case
labels executeTracesQuery, executeLogsQuery, and executeMetricsQuery causing
valid DataSource.method values to fail at runtime; replace the throw with calls
to the appropriate execution path by wiring these cases to the existing
data-fetching helpers (e.g., call the same handler used for other query methods
or the module's executeQuery/executeDataSource function), passing the current
dataSource and any context/params the hook already provides; ensure you return
or await the handler result consistent with other branches so useKopaiData
returns the expected response shape instead of throwing.
In `@packages/ui/src/pages/observability.test.tsx`:
- Line 39: The test's mocked execute call (execute:
vi.fn().mockResolvedValue(...)) doesn't include the SDK's isAgg flag, which can
hide aggregation-specific logic; update the mockResolvedValue in the execute
mock to return the full response shape including isAgg (e.g., isAgg: false or
true depending on the scenario) so tests exercise both aggregation and
non-aggregation branches where code reads isAgg (locate the execute mock in
observability.test.tsx and adjust the mockResolvedValue payload accordingly).
---
Nitpick comments:
In `@packages/sdk/src/query/aggs.ts`:
- Around line 149-168: metricsAgg is missing count() and countDistinct() unlike
tracesAgg and logsAgg; add count and countDistinct entries to the exported
metricsAgg constant (using the same helper implementations those other modules
use—e.g., the count/countDistinct agg helpers or the same makeAgg wrapper used
elsewhere) so they mirror tracesAgg/logsAgg signatures for MetricsColumnName
(e.g., count: countAgg<MetricsColumnName>("count"), countDistinct:
countDistinctAgg<MetricsColumnName>("countDistinct")), or if omission is
intentional add a short inline comment above metricsAgg explaining why counts
are unsupported.
In `@packages/sqlite-datasource/src/__fixtures__/insert-helpers.ts`:
- Around line 448-451: The mapping that builds quantileValues can produce
undefined when opts.quantiles is longer than opts.quantileValues; update the
logic in the quantileValues construction (the const quantileValues =
opts.quantiles.map(...) block) to defensively handle mismatched lengths by
either validating lengths up front and throwing a clear error, or by using a
safe access pattern (e.g., defaulting value to null/0 or skipping entries when
opts.quantileValues[i] is undefined) so no undefined values are produced; ensure
any downstream consumers of quantileValues are adjusted to expect the chosen
default or the validation error.
In `@packages/sqlite-datasource/src/kopai-query-translator.ts`:
- Around line 311-347: buildAggExpr currently implements only count,
countDistinct, sum, avg, min, max and will throw
SqliteDatasourceNotImplementedError for quantile functions (p50, p75, p90, p95,
p99, p999) and topN exposed by tracesAgg; update buildAggExpr or its
error/message to be explicit: either add a brief comment above buildAggExpr
noting that quantiles and topN are intentionally unsupported on SQLite, or
change the SqliteDatasourceNotImplementedError string in buildAggExpr to list
only the supported functions (count, countDistinct, sum, avg, min, max) and
mention that quantiles/topN are not available; reference symbols: buildAggExpr,
tracesAgg, SqliteDatasourceNotImplementedError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5718529-e302-40a6-babc-7f6b03518ec2
⛔ Files ignored due to path filters (1)
packages/ui-core/src/lib/__snapshots__/generate-prompt-instructions.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (46)
packages/api/src/__test-helpers__/mock-query-executors.tspackages/api/src/index.test.tspackages/api/src/routes/__tests__/query.test.tspackages/api/src/routes/error-handler.tspackages/api/src/routes/logs.tspackages/api/src/routes/metrics.tspackages/api/src/routes/query-route.tspackages/api/src/routes/traces.tspackages/api/src/signals.test.tspackages/clickhouse-datasource/src/datasource.tspackages/core/package.jsonpackages/core/src/__tests__/kopai-query.test.tspackages/core/src/index.tspackages/core/src/kopai-query-zod.tspackages/core/src/logs-kopai-query-zod.tspackages/core/src/metrics-kopai-query-zod.tspackages/core/src/telemetry-datasource.tspackages/core/src/traces-kopai-query-zod.tspackages/sdk/src/__tests__/client.types.test.tspackages/sdk/src/client.tspackages/sdk/src/index.tspackages/sdk/src/query/__tests__/aggs.types.test.tspackages/sdk/src/query/__tests__/builder.types.test.tspackages/sdk/src/query/__tests__/columns.types.test.tspackages/sdk/src/query/__tests__/operators.types.test.tspackages/sdk/src/query/__tests__/serialize.test.tspackages/sdk/src/query/aggs.tspackages/sdk/src/query/builder.tspackages/sdk/src/query/columns.tspackages/sdk/src/query/index.tspackages/sdk/src/query/internal-shared.tspackages/sdk/src/query/operators.tspackages/sqlite-datasource/src/__fixtures__/insert-helpers.tspackages/sqlite-datasource/src/datasource-read.test.tspackages/sqlite-datasource/src/db-datasource.tspackages/sqlite-datasource/src/json-path.tspackages/sqlite-datasource/src/kopai-query-read.test.tspackages/sqlite-datasource/src/kopai-query-translator.test.tspackages/sqlite-datasource/src/kopai-query-translator.tspackages/sqlite-datasource/src/optimized-datasource.tspackages/sqlite-datasource/src/sqlite-datasource-error.tspackages/ui-core/src/hooks/use-kopai-data.tspackages/ui-core/src/lib/component-catalog.test.tspackages/ui-core/src/lib/component-catalog.tspackages/ui/src/components/observability/DynamicDashboard/DynamicDashboard.test.tsxpackages/ui/src/pages/observability.test.tsx
Comment on lines
+286
to
+301
| export const timeRangeSchema = z | ||
| .object({ | ||
| start: z | ||
| .string() | ||
| .describe( | ||
| "Range start (inclusive). UNIX Epoch time in nanoseconds, expressed as a string in JSON." | ||
| ), | ||
| end: z | ||
| .string() | ||
| .describe( | ||
| "Range end (exclusive). UNIX Epoch time in nanoseconds, expressed as a string in JSON." | ||
| ), | ||
| }) | ||
| .describe( | ||
| "Closed time range used to constrain a query. Both endpoints are nanosecond JSON strings." | ||
| ); |
There was a problem hiding this comment.
Documentation inconsistency: "end (exclusive)" vs "Closed time range".
Line 295 describes end as "(exclusive)" but line 300 describes the range as "Closed time range" (which conventionally means both endpoints are inclusive). The translator at kopai-query-translator.ts:476 uses <= (inclusive). Update the description to be consistent.
📝 Suggested fix
end: z
.string()
.describe(
- "Range end (exclusive). UNIX Epoch time in nanoseconds, expressed as a string in JSON."
+ "Range end (inclusive). UNIX Epoch time in nanoseconds, expressed as a string in JSON."
),
})
.describe(
"Closed time range used to constrain a query. Both endpoints are nanosecond JSON strings."
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const timeRangeSchema = z | |
| .object({ | |
| start: z | |
| .string() | |
| .describe( | |
| "Range start (inclusive). UNIX Epoch time in nanoseconds, expressed as a string in JSON." | |
| ), | |
| end: z | |
| .string() | |
| .describe( | |
| "Range end (exclusive). UNIX Epoch time in nanoseconds, expressed as a string in JSON." | |
| ), | |
| }) | |
| .describe( | |
| "Closed time range used to constrain a query. Both endpoints are nanosecond JSON strings." | |
| ); | |
| export const timeRangeSchema = z | |
| .object({ | |
| start: z | |
| .string() | |
| .describe( | |
| "Range start (inclusive). UNIX Epoch time in nanoseconds, expressed as a string in JSON." | |
| ), | |
| end: z | |
| .string() | |
| .describe( | |
| "Range end (inclusive). UNIX Epoch time in nanoseconds, expressed as a string in JSON." | |
| ), | |
| }) | |
| .describe( | |
| "Closed time range used to constrain a query. Both endpoints are nanosecond JSON strings." | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/kopai-query-zod.ts` around lines 286 - 301, The schema docs
for timeRangeSchema are inconsistent: the `end` field is described as
"(exclusive)" while the overall description calls it a "Closed time range" and
the translator uses `<=` (inclusive) at comparator in kopai-query-translator.ts;
update the `end` field description and the overall .describe text on
timeRangeSchema to state that both `start` and `end` are inclusive (e.g., "end
(inclusive)" and "Closed (inclusive) time range") so the schema documentation
matches the behavior in the translator (where comparisons use <=).
Comment on lines
+93
to
+96
| const executeResponseSchema = z.object({ | ||
| rows: z.array(z.unknown()), | ||
| cursor: z.string().nullable().optional(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify current execute response schema and cast:"
rg -n -C3 'const executeResponseSchema|cursor: z\.string\(\)\.nullable\(\)\.optional\(\)|ZodType<ExecuteResult' packages/sdk/src/client.ts
echo
echo "Verify runtime tests currently cover happy-path cursor only:"
rg -n -C3 'client\.execute runtime fetch contract|next-cursor|no cursor' packages/sdk/src/__tests__/client.types.test.tsRepository: kopai-app/kopai-mono
Length of output: 2042
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ExecuteResult type definition ==="
rg -n -A10 'type ExecuteResult|interface ExecuteResult' packages/sdk/src/client.ts
echo ""
echo "=== Check if cursor is optional/nullable in type ==="
rg -n 'cursor\?' packages/sdk/src/client.ts | head -20
echo ""
echo "=== See full test case for non-agg cursor handling ==="
sed -n '118,160p' packages/sdk/src/__tests__/client.types.test.ts
echo ""
echo "=== See full test case for agg response ==="
sed -n '173,200p' packages/sdk/src/__tests__/client.types.test.ts
echo ""
echo "=== Check if there are tests validating missing cursor rejection ==="
rg -n 'cursor.*undefined|missing.*cursor|cursor.*required' packages/sdk/src/__tests__/client.types.test.tsRepository: kopai-app/kopai-mono
Length of output: 566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full test file to see cursor handling ==="
wc -l packages/sdk/src/__tests__/client.types.test.ts
echo ""
echo "=== Test cases for non-agg and agg execute ==="
rg -n 'it\(.*execute|cursor.*next|no cursor' packages/sdk/src/__tests__/client.types.test.ts
echo ""
echo "=== Check if any test validates cursor rejection ==="
rg -n 'throws|throw|error|Error' packages/sdk/src/__tests__/client.types.test.ts | grep -i cursor
echo ""
echo "=== Verify rows type throughout the codebase ==="
rg -n 'rows.*z\.' packages/sdk/src/client.ts
echo ""
echo "=== Check ExecutableQuery to understand select structure ==="
rg -n -A15 'type ExecutableQuery' packages/sdk/src/client.ts | head -40Repository: kopai-app/kopai-mono
Length of output: 1166
Fix schema validation to enforce ExecuteResult type contract for non-aggregated queries.
Line 95 allows cursor to be optional and Line 469 force-casts the schema to mask the incompatibility. This allows non-aggregated responses to omit the cursor field, violating the type contract which declares cursor: string | null as required. Additionally, rows currently accepts any values (scalars, arrays, etc.) when they should be objects.
The type definition confirms this:
IsAgg = false: requires{ rows: R[]; cursor: string | null }IsAgg = true: requires{ rows: R[] }(no cursor)
Current tests only cover the happy path; none validate rejection of missing cursor on non-aggregated queries.
🔧 Proposed fix
const executeResponseSchema = z.object({
- rows: z.array(z.unknown()),
+ rows: z.array(z.record(z.string(), z.unknown())),
cursor: z.string().nullable().optional(),
});
@@
async execute<R, IsAgg extends boolean>(
q: ExecutableQuery<R, IsAgg>,
opts?: RequestOptions
): Promise<ExecuteResult<R, IsAgg>> {
- return request(
+ const res = await request(
`${this.baseUrl}/signals/${q.signal}/query`,
- executeResponseSchema as unknown as z.ZodType<ExecuteResult<R, IsAgg>>,
+ executeResponseSchema,
{
method: "POST",
body: q,
...opts,
baseHeaders: this.baseHeaders,
fetchFn: this.fetchFn,
defaultTimeout: this.defaultTimeout,
}
);
+
+ const isAgg = Object.values(
+ (q as { select: Record<string, { kind: string }> }).select
+ ).some((n) => n.kind === "agg");
+
+ if (!isAgg && !("cursor" in res)) {
+ throw new Error("Non-aggregated query response must include `cursor`.");
+ }
+
+ return (isAgg
+ ? { rows: res.rows as R[] }
+ : { rows: res.rows as R[], cursor: res.cursor ?? null }) as ExecuteResult<
+ R,
+ IsAgg
+ >;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/src/client.ts` around lines 93 - 96, The runtime schema
executeResponseSchema must enforce the ExecuteResult contract: replace the loose
schema with two explicit schemas—one for non-aggregated results
(executeResponseSchemaNonAgg) and one for aggregated results
(executeResponseSchemaAgg); make cursor required (z.string().nullable()) on the
non-agg schema (remove optional()) and change rows to an array of objects (e.g.,
z.array(z.record(z.unknown())) or z.array(z.object({}).passthrough()) so
scalars/arrays are rejected). Update the code paths that currently force-cast
the schema (the site referenced in the comment where the schema is cast on use)
to validate against the correct schema based on IsAgg/isAggregated instead of
casting, returning a validation error if the non-agg response is missing cursor.
Ensure references: executeResponseSchema, executeResponseSchemaNonAgg,
executeResponseSchemaAgg, and the usage location that currently force-casts the
schema are updated accordingly.
Comment on lines
+58
to
+60
| type SelectValue = ColumnRef<string, unknown, Kind> | AggExpr<unknown>; | ||
|
|
||
| type SelectMap = Record<string, SelectValue>; |
There was a problem hiding this comment.
Constrain column refs to the active builder signal.
select, groupBy, and orderBy currently accept generic ColumnRef values, so a traces builder can accidentally accept logs/metrics refs and only fail at toQuery() runtime validation (Line 134, Line 154, Line 163, Line 241, Line 246, Line 248). This breaks the “type-safe” contract at compile time.
🔧 Proposed type-level tightening
-type SelectValue = ColumnRef<string, unknown, Kind> | AggExpr<unknown>;
-type SelectMap = Record<string, SelectValue>;
+type ColumnFromCols<T> = T extends ColumnRef<string, unknown, Kind>
+ ? T
+ : T extends Record<string, unknown>
+ ? ColumnFromCols<T[keyof T]>
+ : never;
+
+type SelectValue<Cols> = ColumnFromCols<Cols> | AggExpr<unknown>;
+type SelectMap<Cols> = Record<string, SelectValue<Cols>>;
@@
-export class QueryBuilder<
+export class QueryBuilder<
S extends Signal,
// Cols carried as a generic for downstream method type inference.
Cols,
@@
- select<M extends SelectMap>(
+ select<M extends SelectMap<Cols>>(
map: M
): QueryBuilder<S, Cols, InferRow<M>, HasAgg<M>> {
@@
- groupBy(
- ...cols: ColumnRef<string, unknown, Kind>[]
+ groupBy(
+ ...cols: ColumnFromCols<Cols>[]
): QueryBuilder<S, Cols, Row, IsAgg> {
@@
orderBy(ob: {
- col: ColumnRef<string, unknown, Kind>;
+ col: ColumnFromCols<Cols>;
dir: "asc" | "desc";
}): QueryBuilder<S, Cols, Row, IsAgg> {
@@
type SignalEntry<S extends Signal, Cols> = Cols & {
- select<M extends SelectMap>(
+ select<M extends SelectMap<Cols>>(
map: M
): QueryBuilder<S, Cols, InferRow<M>, HasAgg<M>>;
@@
groupBy(
- ...cols: ColumnRef<string, unknown, Kind>[]
+ ...cols: ColumnFromCols<Cols>[]
): QueryBuilder<S, Cols, never, false>;
orderBy(ob: {
- col: ColumnRef<string, unknown, Kind>;
+ col: ColumnFromCols<Cols>;
dir: "asc" | "desc";
}): QueryBuilder<S, Cols, never, false>;
};Also applies to: 134-136, 154-166, 240-251
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/src/query/builder.ts` around lines 58 - 60, Select, groupBy and
orderBy accept unconstrained ColumnRef types causing cross-builder refs to
compile; tighten types by threading the builder's active signal type into the
SelectValue/SelectMap alias and into function signatures so ColumnRef is
parameterized by the builder signal (e.g. change SelectValue from
ColumnRef<string, unknown, Kind> to ColumnRef<string, unknown, SignalType> and
update SelectMap, and the select, groupBy, orderBy, and toQuery-related
signatures to use that SignalType), ensuring all uses of ColumnRef in this file
(including the symbols SelectValue, SelectMap, select, groupBy, orderBy, and
toQuery) are constrained to the builder's signal generic.
Comment on lines
+402
to
+420
| function buildQuery( | ||
| query: | ||
| | TracesKopaiQuery | ||
| | LogsKopaiQuery | ||
| | (MetricsKopaiQuery & { signal: "metrics" }), | ||
| ctx: SignalContext, | ||
| opts: BuildOpts | ||
| ): TranslateResult { | ||
| let qb: SelectQueryBuilder< | ||
| DB, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| any, | ||
| Record<string, unknown> | ||
| > = queryBuilder.selectFrom(opts.table) as unknown as SelectQueryBuilder< | ||
| DB, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| any, | ||
| Record<string, unknown> | ||
| >; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Unnecessary intersection type in function signature.
The type MetricsKopaiQuery & { signal: "metrics" } at line 406 is misleading. The signal field is provided via SignalContext, not on the query object itself. This intersection is never satisfied by actual callers.
🔧 Suggested fix
function buildQuery(
query:
| TracesKopaiQuery
| LogsKopaiQuery
- | (MetricsKopaiQuery & { signal: "metrics" }),
+ | MetricsKopaiQuery,
ctx: SignalContext,
opts: BuildOpts
): TranslateResult {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sqlite-datasource/src/kopai-query-translator.ts` around lines 402 -
420, The function signature for buildQuery incorrectly requires an intersection
type MetricsKopaiQuery & { signal: "metrics" } which is never used because
signal comes from SignalContext; remove the unnecessary intersection and change
the union arm to just MetricsKopaiQuery (i.e., make the parameter type
TracesKopaiQuery | LogsKopaiQuery | MetricsKopaiQuery), then audit the body of
buildQuery to replace any uses of query.signal with ctx.signal and ensure any
type-narrowing logic relies on ctx.signal or explicit discriminants rather than
the removed intersection.
Comment on lines
+55
to
+61
| case "executeTracesQuery": | ||
| case "executeLogsQuery": | ||
| case "executeMetricsQuery": | ||
| // Wiring deferred — schema-only landing. See followup in plan. | ||
| throw new Error( | ||
| `Method not yet wired in useKopaiData: ${dataSource.method}` | ||
| ); |
There was a problem hiding this comment.
Wire execute data sources instead of hard-failing.*
Line 55–61 makes schema-valid DataSource.method values always error at runtime. Since these methods are now accepted by the catalog/schema layer, this breaks valid dashboard configs in useKopaiData.
💡 Proposed fix
case "executeTracesQuery":
case "executeLogsQuery":
case "executeMetricsQuery":
- // Wiring deferred — schema-only landing. See followup in plan.
- throw new Error(
- `Method not yet wired in useKopaiData: ${dataSource.method}`
+ return client.execute(
+ dataSource.params.query as Parameters<typeof client.execute>[0],
+ { signal }
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui-core/src/hooks/use-kopai-data.ts` around lines 55 - 61, The
switch in useKopaiData currently throws for the case labels executeTracesQuery,
executeLogsQuery, and executeMetricsQuery causing valid DataSource.method values
to fail at runtime; replace the throw with calls to the appropriate execution
path by wiring these cases to the existing data-fetching helpers (e.g., call the
same handler used for other query methods or the module's
executeQuery/executeDataSource function), passing the current dataSource and any
context/params the hook already provides; ensure you return or await the handler
result consistent with other branches so useKopaiData returns the expected
response shape instead of throwing.
| searchTraceSummariesPage: vi | ||
| .fn() | ||
| .mockResolvedValue({ data: [], nextCursor: null }), | ||
| execute: vi.fn().mockResolvedValue({ rows: [], cursor: null }), |
There was a problem hiding this comment.
Align execute mock with the SDK response contract.
On Line 39, the mock omits isAgg. If code paths branch on aggregation mode, this mock can mask behavior and reduce test fidelity.
Suggested fix
- execute: vi.fn().mockResolvedValue({ rows: [], cursor: null }),
+ execute: vi
+ .fn()
+ .mockResolvedValue({ rows: [], cursor: null, isAgg: false }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| execute: vi.fn().mockResolvedValue({ rows: [], cursor: null }), | |
| execute: vi | |
| .fn() | |
| .mockResolvedValue({ rows: [], cursor: null, isAgg: false }), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/pages/observability.test.tsx` at line 39, The test's mocked
execute call (execute: vi.fn().mockResolvedValue(...)) doesn't include the SDK's
isAgg flag, which can hide aggregation-specific logic; update the
mockResolvedValue in the execute mock to return the full response shape
including isAgg (e.g., isAgg: false or true depending on the scenario) so tests
exercise both aggregation and non-aggregation branches where code reads isAgg
(locate the execute mock in observability.test.tsx and adjust the
mockResolvedValue payload accordingly).
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
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
/signals/{traces,logs,metrics}/queryendpoints supporting filters, aggregations, grouping, and pagination.client.execute()method for executing queries programmatically.Tests