Feat/dynamic metrics dashboard#77
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Dynamic Dashboard feature: new dashboard schemas and datasource interface, SQLite-backed DashboardDbDatasource, API routes and error handling, SDK/CLI support, React DynamicDashboard renderer and validators, tests, docs, and wiring across app/server to expose dashboard functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as App (React)
participant SDK as KopaiClient
participant API as Fastify API
participant DB as SQLite DB
participant Renderer as Catalog Renderer
Client->>SDK: POST /dashboards (createDashboard)
SDK->>API: POST /dashboards
API->>DB: createDashboard(options)
DB->>DB: generate id, createdAt, ui_tree_version_major
DB-->>API: Dashboard
API-->>SDK: 201 + Dashboard
SDK-->>Client: resolve Dashboard
Client->>SDK: GET /dashboards/:id
SDK->>API: GET /dashboards/:id
API->>DB: getDashboard(id)
DB-->>API: Dashboard (or not-found)
API-->>SDK: 200|404
SDK-->>Client: Dashboard|error
Client->>Renderer: render(uiTree)
Renderer->>SDK: request metrics/logs/traces via SDK
SDK->>API: POST /signals/* (search)
API->>Client: data
Renderer-->>Client: populated UI components
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
packages/sqlite-datasource/src/sqlite-dashboards-ddl.ts (1)
1-15: Use a single source of truth for dashboards DDL.This SQL duplicates
packages/sqlite-datasource/src/sqlite-dashboards-ddl.sql(Line 4-16), which increases schema drift risk. Consider generating one artifact from the other (or loading one canonical definition).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqlite-datasource/src/sqlite-dashboards-ddl.ts` around lines 1 - 15, The ddl constant in sqlite-dashboards-ddl.ts duplicates the canonical SQL in sqlite-dashboards-ddl.sql; replace the hard-coded ddl string with a single-source approach by loading or importing the .sql file (or generating the .ts from the .sql at build time) so only sqlite-dashboards-ddl.sql contains the schema. Update the sqlite-dashboards-ddl.ts to read the SQL from sqlite-dashboards-ddl.sql (e.g., require/fs read or build-time embed) and remove the duplicated SQL literal, keeping the exported symbol name ddl unchanged so callers of ddl continue to work.packages/core/src/dynamic-dashboard-datasource.ts (1)
15-15: Harden dashboard name validation at the schema boundary.Line 15 and Line 30 allow empty/whitespace-only names. Normalizing and requiring at least one visible character will prevent low-quality records.
Proposed fix
- name: z.string(), + name: z.string().trim().min(1), ... - name: z.string(), + name: z.string().trim().min(1),Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/dynamic-dashboard-datasource.ts` at line 15, Replace the loose "name: z.string()" schema entries with a trimmed-and-validated variant so names cannot be empty or whitespace-only: apply a trim transform and a non-whitespace check (e.g. use z.string().transform(s => s.trim()).refine(s => s.length > 0, { message: 'name must contain at least one non-whitespace character' })) for the "name" field wherever "z.string()" is used (both occurrences shown), ensuring normalization and a minimum visible-character requirement at the schema boundary.packages/api/src/index.ts (1)
34-44: Consider extracting shared API plugin bootstrap to avoid drift.
setValidatorCompiler,setSerializerCompiler, andsetErrorHandlerare duplicated across route plugins; centralizing this keeps behavior consistent as API setup evolves.Refactor sketch
+function configureApiPlugin(fastify: Parameters<FastifyPluginAsyncZod>[0]) { + fastify.setValidatorCompiler(validatorCompiler); + fastify.setSerializerCompiler(serializerCompiler); + fastify.setErrorHandler(errorHandler); +} + export const dashboardsRoutes: FastifyPluginAsyncZod<{ dynamicDashboardDatasource: dashboardDatasource.DynamicDashboardDatasource; }> = async function (fastify, opts) { - fastify.setValidatorCompiler(validatorCompiler); - fastify.setSerializerCompiler(serializerCompiler); - fastify.setErrorHandler(errorHandler); + configureApiPlugin(fastify); fastify.register(_dashboardsRoutes, { dynamicDashboardDatasource: opts.dynamicDashboardDatasource, }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/index.ts` around lines 34 - 44, Extract the repeated Fastify bootstrap calls into a single reusable helper and use it inside dashboardsRoutes: move the calls to setValidatorCompiler, setSerializerCompiler, and setErrorHandler into a shared function or small Fastify plugin (e.g., applyApiBootstrap or apiBootstrapPlugin) and invoke that from dashboardsRoutes before registering _dashboardsRoutes; update dashboardsRoutes to call the helper instead of duplicating the three set* calls so other route modules can reuse the same bootstrap logic and avoid drift.packages/api/src/routes/error-handler.ts (1)
42-49: Downgrade expected dashboard misses from error-level logging.
DashboardNotFoundErroris a normal not-found path; logging it aserrorbefore classification creates noisy signals and can skew alerts.Suggested adjustment
- request.log.error(error); if (error instanceof DashboardNotFoundError) { + request.log.warn({ err: error }, "Dashboard not found"); return reply.status(404).send({ type: "https://docs.kopai.app/errors/dashboard-not-found", status: 404, title: "Dashboard not found", detail: error.message, } satisfies SignalsApiErrorResponse); } + request.log.error(error);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/error-handler.ts` around lines 42 - 49, This handler treats DashboardNotFoundError as a normal 404 but earlier code logs it at error level; change logging for DashboardNotFoundError to a non-error level (info or debug) and ensure the response path (the block returning reply.status(404).send({...} satisfies SignalsApiErrorResponse)) does not trigger error-level logging. Locate DashboardNotFoundError handling and replace or remove any logger.error calls that run for this case (use logger.info/logger.debug or skip logging), keeping the return of the 404 payload intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/dashboards.test.ts`:
- Around line 186-201: The test currently mocks getDashboardSpy with
TestApiError which is a generic/internal error; replace that mock with the
explicit not-found error used by the service layer (e.g., NotFoundError or
DashboardNotFoundError) so the "returns 404 when dashboard not found" spec uses
the correct not-found signal, and add a separate test that mocks getDashboardSpy
to reject with TestApiError and asserts the response statusCode is 500 and the
payload indicates an internal error; update references to getDashboardSpy,
TestApiError, and the test titles accordingly.
In `@packages/api/src/routes/dashboards.ts`:
- Around line 46-56: The catch block in the GET handler for dashboards wrongly
maps every error from opts.dynamicDashboardDatasource.getDashboard to
DashboardNotFoundError; change it to only translate true not-found conditions
(e.g., when the datasource returns null/undefined or throws a specific
NotFound/EntityNotFound error) into DashboardNotFoundError and otherwise rethrow
the original error (or wrap it as a 5xx/internal error). Locate the call to
opts.dynamicDashboardDatasource.getDashboard and the catch that currently throws
DashboardNotFoundError and implement conditional logic that checks for the
datasource's not-found signal (error instanceof SomeNotFoundError or result ==
null) before throwing DashboardNotFoundError; for all other errors, propagate
(throw) them unchanged so they surface as 5xx.
In `@packages/core/src/dynamic-dashboard-datasource.ts`:
- Around line 44-45: The uiTreeVersion and uiTreeVersionCompatible fields are
currently declared as z.string().optional() but should use the existing
semverSchema to enforce SemVer consistency; update their definitions to use
semverSchema.optional() (or semverSchema.nullable().optional() if nulls are
expected) instead of z.string().optional() so validation matches the create flow
that references semverSchema.
In `@packages/sqlite-datasource/src/dashboard-datasource.ts`:
- Around line 25-28: The extractMajor function currently returns 0 for invalid
semver which silently changes uiTreeVersionCompatible behavior; change
extractMajor to validate the input and throw a descriptive error when the semver
string doesn't match the expected pattern (or use a semver parsing lib) instead
of returning 0, and update any callers (e.g., locations computing
uiTreeVersionCompatible) to handle or propagate that error appropriately so
invalid version input fails fast.
In `@packages/ui/src/components/observability/renderers/OtelTraceDetail.tsx`:
- Around line 26-29: The TraceDetail instantiation in OtelTraceDetail.tsx is
using placeholder props (service="", traceId="", onBack={() => {}}) which
produces a non-functional breadcrumb and empty trace label; replace these with
the actual service and trace id values from the OtelTraceDetail component
state/props (e.g., the variables/props that hold the current service and
traceId) and provide a real onBack handler that navigates back or closes the
detail (or remove TraceDetail rendering until those values exist). Update the
TraceDetail props so service and traceId are full, non-empty strings and onBack
calls the proper navigation function rather than a no-op.
In `@packages/ui/src/pages/observability.tsx`:
- Around line 57-72: The URLState now includes dashboardId but pushURLState
still omits it; update the pushURLState function to read and preserve the
dashboardId property when constructing URLSearchParams (and when
merging/updating fields), so any navigation or state push includes dashboardId
in the query string; ensure pushURLState accepts/handles URLState (or pulls
dashboardId from the current state) and appends params.set("dashboardId",
dashboardId) only when dashboardId is non-null/defined to avoid clearing it
unintentionally.
- Around line 727-730: The code currently trusts data.uiTree and calls
setState({ loading: false, error: null, tree: data.uiTree }) directly; add
schema validation for the uiTree payload (e.g., create or import a UiTree
schema/validator such as validateUiTree or a Zod schema for METRICS_TREE shape),
run validation on data.uiTree before calling setState, and if validation fails
setState({ loading: false, error: <validation error>, tree: null }) (and
optionally log the error) so the renderer never receives an unvalidated tree;
update the promise handler around the existing .then block that references
data.uiTree and METRICS_TREE to perform this validation step and short-circuit
on invalid payloads.
---
Nitpick comments:
In `@packages/api/src/index.ts`:
- Around line 34-44: Extract the repeated Fastify bootstrap calls into a single
reusable helper and use it inside dashboardsRoutes: move the calls to
setValidatorCompiler, setSerializerCompiler, and setErrorHandler into a shared
function or small Fastify plugin (e.g., applyApiBootstrap or apiBootstrapPlugin)
and invoke that from dashboardsRoutes before registering _dashboardsRoutes;
update dashboardsRoutes to call the helper instead of duplicating the three set*
calls so other route modules can reuse the same bootstrap logic and avoid drift.
In `@packages/api/src/routes/error-handler.ts`:
- Around line 42-49: This handler treats DashboardNotFoundError as a normal 404
but earlier code logs it at error level; change logging for
DashboardNotFoundError to a non-error level (info or debug) and ensure the
response path (the block returning reply.status(404).send({...} satisfies
SignalsApiErrorResponse)) does not trigger error-level logging. Locate
DashboardNotFoundError handling and replace or remove any logger.error calls
that run for this case (use logger.info/logger.debug or skip logging), keeping
the return of the 404 payload intact.
In `@packages/core/src/dynamic-dashboard-datasource.ts`:
- Line 15: Replace the loose "name: z.string()" schema entries with a
trimmed-and-validated variant so names cannot be empty or whitespace-only: apply
a trim transform and a non-whitespace check (e.g. use z.string().transform(s =>
s.trim()).refine(s => s.length > 0, { message: 'name must contain at least one
non-whitespace character' })) for the "name" field wherever "z.string()" is used
(both occurrences shown), ensuring normalization and a minimum visible-character
requirement at the schema boundary.
In `@packages/sqlite-datasource/src/sqlite-dashboards-ddl.ts`:
- Around line 1-15: The ddl constant in sqlite-dashboards-ddl.ts duplicates the
canonical SQL in sqlite-dashboards-ddl.sql; replace the hard-coded ddl string
with a single-source approach by loading or importing the .sql file (or
generating the .ts from the .sql at build time) so only
sqlite-dashboards-ddl.sql contains the schema. Update the
sqlite-dashboards-ddl.ts to read the SQL from sqlite-dashboards-ddl.sql (e.g.,
require/fs read or build-time embed) and remove the duplicated SQL literal,
keeping the exported symbol name ddl unchanged so callers of ddl continue to
work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 582f129e-c11f-4c7c-aacf-3c61fbcaec9a
📒 Files selected for processing (25)
packages/api/src/dashboards.test.tspackages/api/src/index.tspackages/api/src/routes/dashboards.tspackages/api/src/routes/error-handler.tspackages/api/src/routes/errors.tspackages/app/src/routes/index.tspackages/app/src/server.tspackages/core/src/dynamic-dashboard-datasource.tspackages/core/src/index.tspackages/sqlite-datasource/src/dashboard-datasource.tspackages/sqlite-datasource/src/datasource-dashboards.test.tspackages/sqlite-datasource/src/db-types.tspackages/sqlite-datasource/src/index.tspackages/sqlite-datasource/src/initialize-database.tspackages/sqlite-datasource/src/sqlite-dashboards-ddl.sqlpackages/sqlite-datasource/src/sqlite-dashboards-ddl.tspackages/ui/src/components/observability/DynamicDashboard/DynamicDashboard.test.tsxpackages/ui/src/components/observability/DynamicDashboard/index.tsxpackages/ui/src/components/observability/index.tspackages/ui/src/components/observability/renderers/OtelLogTimeline.tsxpackages/ui/src/components/observability/renderers/OtelTraceDetail.tsxpackages/ui/src/components/observability/renderers/index.tspackages/ui/src/lib/dashboard-datasource.tspackages/ui/src/lib/renderer.tsxpackages/ui/src/pages/observability.tsx
💤 Files with no reviewable changes (1)
- packages/ui/src/lib/dashboard-datasource.ts
Comment on lines
+26
to
+29
| service="" | ||
| traceId="" | ||
| onBack={() => {}} | ||
| /> |
There was a problem hiding this comment.
Avoid placeholder breadcrumb/navigation props in TraceDetail.
Line 26, Line 27, and Line 28 pass service="", traceId="", and a no-op onBack. In packages/ui/src/components/observability/TraceDetail/index.tsx (Line 28-42), this renders a clickable breadcrumb with no action and an empty trace label (...), which is a broken interaction in the detail view.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/observability/renderers/OtelTraceDetail.tsx`
around lines 26 - 29, The TraceDetail instantiation in OtelTraceDetail.tsx is
using placeholder props (service="", traceId="", onBack={() => {}}) which
produces a non-functional breadcrumb and empty trace label; replace these with
the actual service and trace id values from the OtelTraceDetail component
state/props (e.g., the variables/props that hold the current service and
traceId) and provide a real onBack handler that navigates back or closes the
detail (or remove TraceDetail rendering until those values exist). Update the
TraceDetail props so service and traceId are full, non-empty strings and onBack
calls the proper navigation function rather than a no-op.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/ui/src/pages/observability.test.tsx (1)
48-121: Consider adding a test for the fallback behavior.The test suite covers valid and invalid
uiTreescenarios but doesn't test the case whendashboardIdis absent (no query parameter), which should render with the staticMETRICS_TREEfallback.Example test case
it("renders fallback METRICS_TREE when no dashboardId is provided", async () => { setURL("?tab=metrics"); render( createElement(ObservabilityPage, { client: mockClient as unknown as KopaiClient, }) ); await waitFor(() => { expect(screen.getByText("Metrics")).toBeTruthy(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/pages/observability.test.tsx` around lines 48 - 121, Add a test that verifies the fallback to the static METRICS_TREE when no dashboardId query param is present: use the existing setURL("?tab=metrics") helper, render ObservabilityPage with the mockClient (as in other tests), and await assertion that a known METRICS_TREE label (e.g., "Metrics") is rendered; place this new it(...) alongside the other useDashboardTree validation tests so it exercises the fallback branch in the component that reads the URL and uses METRICS_TREE.packages/ui/src/pages/observability.tsx (1)
666-666: Consider making the API base configurable.The hardcoded
/dashboardspath assumes the API is co-located. This works but may limit flexibility if the API needs to be hosted elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/pages/observability.tsx` at line 666, DASHBOARDS_API_BASE is hardcoded to "/dashboards"; make it configurable by replacing the constant with a value read from a configurable source (e.g., environment variable like NEXT_PUBLIC_DASHBOARDS_API_BASE, a runtime window/global config, or an app config module) while keeping "/dashboards" as the default fallback; update any consumers that import or reference DASHBOARDS_API_BASE in this file (observability.tsx) to use the new configurable export so the API base can point to an external host when needed.packages/sqlite-datasource/src/dashboard-datasource.ts (1)
217-221: Consider a composite index for the(created_at, id)page key.This path orders and paginates on both columns, but
packages/sqlite-datasource/src/sqlite-dashboards-ddl.ts:1-15only createsidx_dashboards_created_atoncreated_at. As the table grows, SQLite will still do extra work for theidtiebreaker.Suggested follow-up in `packages/sqlite-datasource/src/sqlite-dashboards-ddl.ts`
CREATE INDEX IF NOT EXISTS idx_dashboards_created_at ON dashboards(created_at); +CREATE INDEX IF NOT EXISTS idx_dashboards_created_at_id +ON dashboards(created_at, id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqlite-datasource/src/dashboard-datasource.ts` around lines 217 - 221, The query in dashboard-datasource.ts orders by created_at then id (see the chained .orderBy("created_at", ...) and .orderBy("id", ...)) but the DDL only defines idx_dashboards_created_at on created_at; add a composite index (e.g., idx_dashboards_created_at_id) on (created_at, id) in sqlite-dashboards-ddl.ts so SQLite can use a single index for the ordered pagination; update the DDL to create the composite index (and optionally remove or keep the single-column index) and bump any migration/version metadata as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sqlite-datasource/src/dashboard-datasource.ts`:
- Around line 184-214: The cursor parsing is too lax (only checks for "|") so
malformed cursors like "zzz|" or invalid timestamps can silently drive the
query; in the block that reads filter.cursor (around variables filter.cursor,
cursorCreatedAt, cursorId, and SqliteDatasourceQueryError) validate both parts:
ensure cursorId is non-empty and cursorCreatedAt is a valid ISO datetime (e.g.,
Date.parse(cursorCreatedAt) is finite or matching an ISO regex), and if
validation fails throw SqliteDatasourceQueryError with a clear message; do this
validation before building the page predicate used with query and sortOrder so
invalid cursors fail fast.
- Around line 57-90: The catch in createDashboard currently wraps all errors as
a new SqliteDatasourceQueryError, hiding pre-existing SqliteDatasourceQueryError
instances (e.g., thrown by extractMajor); change the catch to rethrow the
original error when it is already a SqliteDatasourceQueryError (if (error
instanceof SqliteDatasourceQueryError) throw error) and only wrap other error
types into the new SqliteDatasourceQueryError("Failed to create dashboard", {
cause: error }); this preserves original error semantics from extractMajor and
keeps behavior consistent with other methods.
---
Nitpick comments:
In `@packages/sqlite-datasource/src/dashboard-datasource.ts`:
- Around line 217-221: The query in dashboard-datasource.ts orders by created_at
then id (see the chained .orderBy("created_at", ...) and .orderBy("id", ...))
but the DDL only defines idx_dashboards_created_at on created_at; add a
composite index (e.g., idx_dashboards_created_at_id) on (created_at, id) in
sqlite-dashboards-ddl.ts so SQLite can use a single index for the ordered
pagination; update the DDL to create the composite index (and optionally remove
or keep the single-column index) and bump any migration/version metadata as
needed.
In `@packages/ui/src/pages/observability.test.tsx`:
- Around line 48-121: Add a test that verifies the fallback to the static
METRICS_TREE when no dashboardId query param is present: use the existing
setURL("?tab=metrics") helper, render ObservabilityPage with the mockClient (as
in other tests), and await assertion that a known METRICS_TREE label (e.g.,
"Metrics") is rendered; place this new it(...) alongside the other
useDashboardTree validation tests so it exercises the fallback branch in the
component that reads the URL and uses METRICS_TREE.
In `@packages/ui/src/pages/observability.tsx`:
- Line 666: DASHBOARDS_API_BASE is hardcoded to "/dashboards"; make it
configurable by replacing the constant with a value read from a configurable
source (e.g., environment variable like NEXT_PUBLIC_DASHBOARDS_API_BASE, a
runtime window/global config, or an app config module) while keeping
"/dashboards" as the default fallback; update any consumers that import or
reference DASHBOARDS_API_BASE in this file (observability.tsx) to use the new
configurable export so the API base can point to an external host when needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 468a83d0-8afe-4fd7-8cdb-4c4dceec0734
📒 Files selected for processing (9)
packages/api/src/dashboards.test.tspackages/api/src/routes/dashboards.tspackages/api/src/routes/error-handler.tspackages/core/src/dynamic-dashboard-datasource.tspackages/sqlite-datasource/src/dashboard-datasource.tspackages/sqlite-datasource/src/datasource-dashboards.test.tspackages/ui/src/components/observability/renderers/OtelTraceDetail.tsxpackages/ui/src/pages/observability.test.tsxpackages/ui/src/pages/observability.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/api/src/routes/dashboards.ts
- packages/core/src/dynamic-dashboard-datasource.ts
- packages/api/src/routes/error-handler.ts
- packages/api/src/dashboards.test.ts
- packages/sqlite-datasource/src/datasource-dashboards.test.ts
Comment on lines
+57
to
+90
| try { | ||
| const id = randomUUID(); | ||
| const createdAt = new Date().toISOString(); | ||
| const major = extractMajor(options.uiTreeVersion); | ||
|
|
||
| const { sql, parameters } = queryBuilder | ||
| .insertInto("dashboards") | ||
| .values({ | ||
| id, | ||
| name: options.name, | ||
| created_at: createdAt, | ||
| metadata: JSON.stringify(options.metadata ?? {}), | ||
| ui_tree_version: options.uiTreeVersion, | ||
| ui_tree_version_major: major, | ||
| ui_tree: JSON.stringify(options.uiTree ?? {}), | ||
| }) | ||
| .compile(); | ||
|
|
||
| this.sqliteConnection | ||
| .prepare(sql) | ||
| .run(...(parameters as (string | number | bigint | null)[])); | ||
|
|
||
| return { | ||
| id, | ||
| name: options.name, | ||
| createdAt, | ||
| metadata: options.metadata ?? {}, | ||
| uiTreeVersion: options.uiTreeVersion, | ||
| uiTree: options.uiTree ?? {}, | ||
| }; | ||
| } catch (error) { | ||
| throw new SqliteDatasourceQueryError("Failed to create dashboard", { | ||
| cause: error, | ||
| }); |
There was a problem hiding this comment.
Preserve known datasource errors in createDashboard.
extractMajor() can already throw SqliteDatasourceQueryError, but this catch wraps it into a generic "Failed to create dashboard". That makes invalid uiTreeVersion input look like an insert failure and is inconsistent with the other methods in this class.
Suggested fix
} catch (error) {
+ if (error instanceof SqliteDatasourceQueryError) throw error;
throw new SqliteDatasourceQueryError("Failed to create dashboard", {
cause: error,
});
}📝 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
| try { | |
| const id = randomUUID(); | |
| const createdAt = new Date().toISOString(); | |
| const major = extractMajor(options.uiTreeVersion); | |
| const { sql, parameters } = queryBuilder | |
| .insertInto("dashboards") | |
| .values({ | |
| id, | |
| name: options.name, | |
| created_at: createdAt, | |
| metadata: JSON.stringify(options.metadata ?? {}), | |
| ui_tree_version: options.uiTreeVersion, | |
| ui_tree_version_major: major, | |
| ui_tree: JSON.stringify(options.uiTree ?? {}), | |
| }) | |
| .compile(); | |
| this.sqliteConnection | |
| .prepare(sql) | |
| .run(...(parameters as (string | number | bigint | null)[])); | |
| return { | |
| id, | |
| name: options.name, | |
| createdAt, | |
| metadata: options.metadata ?? {}, | |
| uiTreeVersion: options.uiTreeVersion, | |
| uiTree: options.uiTree ?? {}, | |
| }; | |
| } catch (error) { | |
| throw new SqliteDatasourceQueryError("Failed to create dashboard", { | |
| cause: error, | |
| }); | |
| try { | |
| const id = randomUUID(); | |
| const createdAt = new Date().toISOString(); | |
| const major = extractMajor(options.uiTreeVersion); | |
| const { sql, parameters } = queryBuilder | |
| .insertInto("dashboards") | |
| .values({ | |
| id, | |
| name: options.name, | |
| created_at: createdAt, | |
| metadata: JSON.stringify(options.metadata ?? {}), | |
| ui_tree_version: options.uiTreeVersion, | |
| ui_tree_version_major: major, | |
| ui_tree: JSON.stringify(options.uiTree ?? {}), | |
| }) | |
| .compile(); | |
| this.sqliteConnection | |
| .prepare(sql) | |
| .run(...(parameters as (string | number | bigint | null)[])); | |
| return { | |
| id, | |
| name: options.name, | |
| createdAt, | |
| metadata: options.metadata ?? {}, | |
| uiTreeVersion: options.uiTreeVersion, | |
| uiTree: options.uiTree ?? {}, | |
| }; | |
| } catch (error) { | |
| if (error instanceof SqliteDatasourceQueryError) throw error; | |
| throw new SqliteDatasourceQueryError("Failed to create dashboard", { | |
| cause: error, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sqlite-datasource/src/dashboard-datasource.ts` around lines 57 - 90,
The catch in createDashboard currently wraps all errors as a new
SqliteDatasourceQueryError, hiding pre-existing SqliteDatasourceQueryError
instances (e.g., thrown by extractMajor); change the catch to rethrow the
original error when it is already a SqliteDatasourceQueryError (if (error
instanceof SqliteDatasourceQueryError) throw error) and only wrap other error
types into the new SqliteDatasourceQueryError("Failed to create dashboard", {
cause: error }); this preserves original error semantics from extractMajor and
keeps behavior consistent with other methods.
- change base URL so that it does not contain /signals
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/AGENTS.md (1)
15-25:⚠️ Potential issue | 🟡 MinorUpdate the
.kopaircexample to the new base URL format.The default URL now points at the server root, but the sample config still uses
.../signals. Copying that example will send current CLI requests to the wrong path.Based on learnings, "Reference CLI documentation in packages/cli/AGENTS.md for full CLI reference".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/AGENTS.md` around lines 15 - 25, Update the .kopairc example so the "url" uses the new base-server format (root URL) instead of including the "/signals" path; change the sample "url" value to match the default base (e.g. the same host as the documented Default: http://localhost:8000) and keep the "token" key as-is, and update any accompanying sentence that references the example to point readers to the CLI docs in packages/cli/AGENTS.md for full reference.
🧹 Nitpick comments (1)
packages/api/src/routes/dashboards.ts (1)
76-85: String matching for "not found" is fragile.The current approach (
msg.includes("not found")) may miss variations like "Dashboard does not exist", "No dashboard with ID...", or localized error messages. Consider having the datasource throw a typed error (e.g.,DashboardNotFoundErrorfrom core) or returnnullfor missing dashboards, which provides a more reliable contract.💡 Alternative: Return-null pattern
If the datasource can return
nullfor missing dashboards:handler: async (req, res) => { - try { - const result = await opts.dynamicDashboardDatasource.getDashboard({ - id: req.params.dashboardId, - requestContext: req.requestContext, - }); - res.send(result); - } catch (error) { - const msg = error instanceof Error ? error.message.toLowerCase() : ""; - if (msg.includes("not found")) { - throw new DashboardNotFoundError( - `Dashboard not found: ${req.params.dashboardId}`, - { cause: error } - ); - } - throw error; - } + const result = await opts.dynamicDashboardDatasource.getDashboard({ + id: req.params.dashboardId, + requestContext: req.requestContext, + }); + if (!result) { + throw new DashboardNotFoundError( + `Dashboard not found: ${req.params.dashboardId}` + ); + } + res.send(result); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/dashboards.ts` around lines 76 - 85, Replace fragile string matching in the catch block with a typed-or-null contract: update the datasource method that fetches dashboards (e.g., getDashboardById / repository method used in the route) to either return null when a dashboard is missing or throw a core DashboardNotFoundError, and then change the error handling in the route’s try/catch to detect that contract by checking for instance-of DashboardNotFoundError or a null return and then throw the DashboardNotFoundError (with the original error as cause when present); ensure you reference the DashboardNotFoundError class used elsewhere so matching is type-safe rather than using msg.includes("not found").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/client.ts`:
- Around line 27-33: The resolved URL may include a legacy "/signals" path or a
trailing slash; update resolveConnectionOpts to normalize the final url (the
value chosen from opts.url ?? fileConfig.url ?? DEFAULT_URL) by stripping a
trailing "/signals" segment if present and then removing any trailing slash
before returning ConnectionOpts; keep token resolution as-is and use symbols
resolveConnectionOpts, loadConfig, ClientOptions, ConnectionOpts, and
DEFAULT_URL to locate and modify the logic.
In `@packages/cli/src/commands/dashboards.ts`:
- Around line 12-21: The dashboards schema subcommand ignores the provided
--timeout because the fetch call in the withConnectionOptions(...) action (the
block using resolveConnectionOpts and calling fetch(`${url}/dashboards/schema`,
{ headers })) never uses a timeout/AbortSignal; update the action to read the
timeout value from opts (or from resolveConnectionOpts if it returns it), create
an AbortController, schedule a setTimeout to call controller.abort() after the
timeout, pass controller.signal into the fetch options alongside headers, and
clear the timeout on success/failure so the command honors --timeout and won’t
hang indefinitely.
In `@packages/sdk/src/client.ts`:
- Around line 89-90: Normalize the baseUrl once in the KopaiClient constructor
to strip a trailing "/signals" (and optional trailing slash) so subsequent
endpoint constructions like the request calls using
`${this.baseUrl}/signals/...` do not produce duplicate "/signals/signals/...";
update the constructor to set this.baseUrl to the cleaned value (only if
present) and leave all existing methods (those building URLs for traces, events,
etc.) unchanged so they continue to append "/signals/..." safely.
---
Outside diff comments:
In `@packages/cli/AGENTS.md`:
- Around line 15-25: Update the .kopairc example so the "url" uses the new
base-server format (root URL) instead of including the "/signals" path; change
the sample "url" value to match the default base (e.g. the same host as the
documented Default: http://localhost:8000) and keep the "token" key as-is, and
update any accompanying sentence that references the example to point readers to
the CLI docs in packages/cli/AGENTS.md for full reference.
---
Nitpick comments:
In `@packages/api/src/routes/dashboards.ts`:
- Around line 76-85: Replace fragile string matching in the catch block with a
typed-or-null contract: update the datasource method that fetches dashboards
(e.g., getDashboardById / repository method used in the route) to either return
null when a dashboard is missing or throw a core DashboardNotFoundError, and
then change the error handling in the route’s try/catch to detect that contract
by checking for instance-of DashboardNotFoundError or a null return and then
throw the DashboardNotFoundError (with the original error as cause when
present); ensure you reference the DashboardNotFoundError class used elsewhere
so matching is type-safe rather than using msg.includes("not found").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59f0d6f9-df86-40aa-8ba2-68cb5cfa9f8f
📒 Files selected for processing (21)
.changeset/remove-signals-from-base-url.mdexamples/clickhouse-observability-backend/README.mdpackages/api/src/index.tspackages/api/src/routes/dashboards.tspackages/app/src/routes/index.tspackages/cli/AGENTS.mdpackages/cli/README.mdpackages/cli/src/client.tspackages/cli/src/commands/dashboards.tspackages/cli/src/commands/login.test.tspackages/cli/src/commands/whoami.test.tspackages/cli/src/config.tspackages/cli/src/index.tspackages/sdk/src/client.test.tspackages/sdk/src/client.tspackages/sdk/src/mocks/handlers.tspackages/ui/README.mdpackages/ui/src/index.tspackages/ui/src/lib/generate-prompt-instructions.tspackages/ui/src/pages/observability.tsxskills/create-dashboard
✅ Files skipped from review due to trivial changes (2)
- skills/create-dashboard
- packages/cli/README.md
Comment on lines
+12
to
+21
| withConnectionOptions( | ||
| dashboards | ||
| .command("schema") | ||
| .description("Print UI tree schema for AI agents") | ||
| ).action(async (opts: ClientOptions) => { | ||
| try { | ||
| const { url, token } = resolveConnectionOpts(opts); | ||
| const headers: Record<string, string> = {}; | ||
| if (token) headers["Authorization"] = `Bearer ${token}`; | ||
| const response = await fetch(`${url}/dashboards/schema`, { headers }); |
There was a problem hiding this comment.
Honor --timeout for dashboards schema.
This subcommand exposes --timeout via withConnectionOptions, but the raw fetch call ignores it. That makes the flag ineffective and allows the command to hang indefinitely on a slow or stuck server.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/dashboards.ts` around lines 12 - 21, The dashboards
schema subcommand ignores the provided --timeout because the fetch call in the
withConnectionOptions(...) action (the block using resolveConnectionOpts and
calling fetch(`${url}/dashboards/schema`, { headers })) never uses a
timeout/AbortSignal; update the action to read the timeout value from opts (or
from resolveConnectionOpts if it returns it), create an AbortController,
schedule a setTimeout to call controller.abort() after the timeout, pass
controller.signal into the fetch options alongside headers, and clear the
timeout on success/failure so the command honors --timeout and won’t hang
indefinitely.
Comment on lines
+89
to
90
| return request(`${this.baseUrl}/signals/traces/${traceId}`, schema, { | ||
| method: "GET", |
There was a problem hiding this comment.
Preserve compatibility for baseUrl values that already include /signals.
These path updates silently change the public KopaiClient contract. Callers still passing https://host/signals will now request /signals/signals/... and start failing after upgrade. Normalize a trailing /signals once in the constructor before building endpoint URLs.
🔧 Suggested compatibility fix
constructor(options: KopaiClientOptions) {
- this.baseUrl = options.baseUrl.replace(/\/$/, ""); // Remove trailing slash
+ this.baseUrl = options.baseUrl
+ .replace(/\/signals\/?$/, "")
+ .replace(/\/$/, ""); // Remove trailing slash
this.fetchFn = options.fetch ?? fetch;
this.defaultTimeout = options.timeout ?? DEFAULT_TIMEOUT;Also applies to: 125-136, 166-173, 203-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk/src/client.ts` around lines 89 - 90, Normalize the baseUrl once
in the KopaiClient constructor to strip a trailing "/signals" (and optional
trailing slash) so subsequent endpoint constructions like the request calls
using `${this.baseUrl}/signals/...` do not produce duplicate
"/signals/signals/..."; update the constructor to set this.baseUrl to the
cleaned value (only if present) and leave all existing methods (those building
URLs for traces, events, etc.) unchanged so they continue to append
"/signals/..." safely.
- use --tree-version instead of --version to prevent confusion
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/README.md (1)
55-59:⚠️ Potential issue | 🟡 MinorConfig file example may be inconsistent with new URL handling.
The example shows
"url": "https://your-kopai-server.com/signals"with the/signalssuffix. Based on the context snippets, the SDK now expects the base URL without/signals(e.g.,http://localhost:8000), and the client internally appends/signalsfor signal-related endpoints while using/dashboardsdirectly for dashboard endpoints.Consider updating the example to reflect the new URL convention:
{ - "url": "https://your-kopai-server.com/signals", + "url": "https://your-kopai-server.com", "token": "your-token" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/README.md` around lines 55 - 59, The README.json example uses "url": "https://your-kopai-server.com/signals" which is inconsistent with the SDK's new URL handling; update the example to use the base server URL (e.g., "https://your-kopai-server.com" or "http://localhost:8000") for the "url" field so the client can append "/signals" or "/dashboards" internally and keep the "token" field unchanged.
🧹 Nitpick comments (4)
packages/ui/src/pages/observability.test.tsx (1)
73-94: Consider restoring the fetch spy after each test.The
vi.spyOn(globalThis, "fetch")creates a spy that persists beyond the test. Whilevi.clearAllMocks()clears call counts, it doesn't restore the original implementation. If other tests in the suite (or future tests) depend on real fetch behavior, this could cause issues.♻️ Suggested fix: restore mocks in afterEach
afterEach(() => { + vi.restoreAllMocks(); window.history.replaceState( null, "", window.location.pathname + originalLocation ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/pages/observability.test.tsx` around lines 73 - 94, The test creates a persistent spy via vi.spyOn(globalThis, "fetch") which isn't restored and can leak into other tests; update the test file to restore the spy after each test by either capturing the spy returned from vi.spyOn and calling mockRestore() in an afterEach block or calling vi.restoreAllMocks() / vi.resetAllMocks() in afterEach to restore globalThis.fetch to its original implementation (the spy is created near the "vi.spyOn(globalThis, 'fetch')" call in the observability.test.tsx that exercises ObservabilityPage).packages/cli/src/commands/dashboards.ts (3)
67-68: Add error handling for invalid JSON input.If stdin contains invalid JSON,
JSON.parsewill throw aSyntaxError. Consider catching this specifically and providing a clearer error message or using exit code 2 (invalid arguments).🛡️ Suggested improvement
const raw = await readStdin(); - const body = JSON.parse(raw) as Record<string, unknown>; + let body: Record<string, unknown>; + try { + body = JSON.parse(raw) as Record<string, unknown>; + } catch (e) { + outputError(new Error('Invalid JSON input from stdin'), isJson); + process.exit(2); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/dashboards.ts` around lines 67 - 68, The code calls JSON.parse(raw) without handling parse errors; wrap the parse in a try/catch around the readStdin() result in the relevant command handler (where readStdin() and const body are used), catch SyntaxError specifically and print a clear error to stderr (e.g., "Invalid JSON input: <error.message>") and exit with code 2 (or return a CLI error with exit code 2), otherwise rethrow other errors; ensure the parsed value still assigned to the existing body variable on success.
43-46: Consider differentiating exit codes based on error type.Per the CLI exit code convention, config errors (e.g., missing URL) should exit with code 3, while API/runtime errors exit with code 1. Currently, all errors exit with code 1.
🛠️ Suggested approach
} catch (err) { + const isConfigError = err instanceof Error && + (err.message.includes('ECONNREFUSED') || err.message.includes('Invalid URL')); outputError(err, false); - process.exit(1); + process.exit(isConfigError ? 3 : 1); }As per coding guidelines: "Kopai CLI exit codes must follow the standard convention: 0 for success, 1 for API/runtime errors, 2 for invalid arguments, 3 for config errors (missing url)"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/dashboards.ts` around lines 43 - 46, The catch block in the dashboards command currently always calls process.exit(1); change it to map error types to the correct CLI exit codes: detect config errors (e.g., missing URL) and exit with code 3, while leaving API/runtime errors as code 1; implement detection by checking for a dedicated ConfigError class (err instanceof ConfigError) and/or err.name === 'ConfigError' and as a fallback matching known messages like 'missing url'; still call outputError(err, false) before exiting and use process.exit(<code>) so existing symbols (outputError, process.exit, the dashboards command) are used.
79-82: Consider differentiating exit codes for thecreatesubcommand as well.Same as the
schemasubcommand - config-related errors (connection refused, invalid URL) should exit with code 3 rather than 1.As per coding guidelines: "Kopai CLI exit codes must follow the standard convention: 0 for success, 1 for API/runtime errors, 2 for invalid arguments, 3 for config errors (missing url)"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/dashboards.ts` around lines 79 - 82, The catch block for the `create` subcommand currently always calls process.exit(1) after outputError(err, isJson); update it to mirror the `schema` subcommand behavior by detecting config-related errors (e.g., connection refused, invalid URL) and exiting with code 3 instead of 1: add a small predicate (or reuse an existing helper) to check err codes/names/messages (e.g., ECONNREFUSED, ENOTFOUND, INVALID_URL or message contains "invalid url") and call process.exit(3) when that predicate is true; otherwise keep process.exit(1). Ensure you update the catch in the same function handling the `create` command (referencing outputError and isJson) so only config errors map to exit code 3.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/cli/README.md`:
- Around line 55-59: The README.json example uses "url":
"https://your-kopai-server.com/signals" which is inconsistent with the SDK's new
URL handling; update the example to use the base server URL (e.g.,
"https://your-kopai-server.com" or "http://localhost:8000") for the "url" field
so the client can append "/signals" or "/dashboards" internally and keep the
"token" field unchanged.
---
Nitpick comments:
In `@packages/cli/src/commands/dashboards.ts`:
- Around line 67-68: The code calls JSON.parse(raw) without handling parse
errors; wrap the parse in a try/catch around the readStdin() result in the
relevant command handler (where readStdin() and const body are used), catch
SyntaxError specifically and print a clear error to stderr (e.g., "Invalid JSON
input: <error.message>") and exit with code 2 (or return a CLI error with exit
code 2), otherwise rethrow other errors; ensure the parsed value still assigned
to the existing body variable on success.
- Around line 43-46: The catch block in the dashboards command currently always
calls process.exit(1); change it to map error types to the correct CLI exit
codes: detect config errors (e.g., missing URL) and exit with code 3, while
leaving API/runtime errors as code 1; implement detection by checking for a
dedicated ConfigError class (err instanceof ConfigError) and/or err.name ===
'ConfigError' and as a fallback matching known messages like 'missing url';
still call outputError(err, false) before exiting and use process.exit(<code>)
so existing symbols (outputError, process.exit, the dashboards command) are
used.
- Around line 79-82: The catch block for the `create` subcommand currently
always calls process.exit(1) after outputError(err, isJson); update it to mirror
the `schema` subcommand behavior by detecting config-related errors (e.g.,
connection refused, invalid URL) and exiting with code 3 instead of 1: add a
small predicate (or reuse an existing helper) to check err codes/names/messages
(e.g., ECONNREFUSED, ENOTFOUND, INVALID_URL or message contains "invalid url")
and call process.exit(3) when that predicate is true; otherwise keep
process.exit(1). Ensure you update the catch in the same function handling the
`create` command (referencing outputError and isJson) so only config errors map
to exit code 3.
In `@packages/ui/src/pages/observability.test.tsx`:
- Around line 73-94: The test creates a persistent spy via vi.spyOn(globalThis,
"fetch") which isn't restored and can leak into other tests; update the test
file to restore the spy after each test by either capturing the spy returned
from vi.spyOn and calling mockRestore() in an afterEach block or calling
vi.restoreAllMocks() / vi.resetAllMocks() in afterEach to restore
globalThis.fetch to its original implementation (the spy is created near the
"vi.spyOn(globalThis, 'fetch')" call in the observability.test.tsx that
exercises ObservabilityPage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 789ed3e5-3fdc-4ad7-819c-a8fc79a2ec19
📒 Files selected for processing (12)
packages/cli/AGENTS.mdpackages/cli/README.mdpackages/cli/src/commands/dashboards.tspackages/sdk/src/client.test.tspackages/sdk/src/client.tspackages/sdk/src/index.tspackages/sdk/src/mocks/handlers.tspackages/sdk/src/types.tspackages/ui/src/components/observability/DynamicDashboard/DynamicDashboard.test.tsxpackages/ui/src/pages/observability.test.tsxskills/create-dashboard/SKILL.mdskills/create-dashboard/rules/workflow.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/AGENTS.md
- packages/ui/src/components/observability/DynamicDashboard/DynamicDashboard.test.tsx
- packages/sdk/src/client.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/ui/src/components/observability/__fixtures__/metrics.ts (1)
272-288: Non-deterministic fixture values may cause flaky snapshots.
Math.random()on line 281 produces different values on each run, which could cause snapshot test failures or inconsistent visual regression tests if these fixtures are used for snapshot comparisons.♻️ Suggested fix using deterministic pseudo-random values
export const mockMultiAttributeRows: OtelMetricsRow[] = cpuTimeStates.flatMap( (state, si) => Array.from({ length: 10 }, (_, i) => ({ MetricType: "Gauge" as const, MetricName: "process.cpu.time", MetricDescription: "CPU time by state", MetricUnit: "s", TimeUnix: ts(i * INTERVAL_MS), StartTimeUnix: ts(0), - Value: cpuTimeBase[si]! + Math.random() * 0.5, + Value: cpuTimeBase[si]! + ((si * 10 + i) % 5) * 0.1, ServiceName: "api-gateway", Attributes: { state, "cpu.mode": si < 2 ? "main" : "children" }, ResourceAttributes: { "service.name": "api-gateway" }, ScopeName: "opentelemetry.instrumentation.process", ScopeVersion: "0.44.0", })) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/__fixtures__/metrics.ts` around lines 272 - 288, The fixture mockMultiAttributeRows uses Math.random() (in the Value field) which makes snapshots flaky; replace the non-deterministic random with a deterministic source (e.g., a seeded pseudo-random generator or a fixed array of offsets) so tests always get the same values. Locate the mockMultiAttributeRows construction and change Value: cpuTimeBase[si]! + Math.random() * 0.5 to derive the offset from a reproducible function/seed (tied to i and si) or a predefined offsets array generated once, ensuring cpuTimeStates and cpuTimeBase usage remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/dashboards.ts`:
- Around line 63-91: The create subcommand currently exits with code 1 for all
errors; modify error handling to use a shared helper (e.g., handleCommandError)
that calls outputError(err, isJson) and then exits with code 3 for
config-related errors (detectable by checking err instanceof Error &&
/\burl\b/i.test(err.message) or other config error patterns) otherwise exiting
with code 1; update the action callback around createClient(opts) and the outer
catch to invoke handleCommandError(err, isJson) instead of directly calling
outputError and process.exit so createClient, client.createDashboard, and
related failures produce the correct exit code.
- Around line 43-46: The catch block in dashboards.ts currently treats all
errors the same and exits with code 1; modify it to distinguish configuration
errors (from resolveConnectionOpts or a dedicated ConfigError) from runtime/API
errors by detecting the specific error type or a property (e.g., instanceof
ConfigError or err.code === 'MISSING_URL') and call process.exit(3) for config
errors and process.exit(1) for others; ensure outputError(err, false) is still
called before exiting so logs remain intact.
---
Nitpick comments:
In `@packages/ui/src/components/observability/__fixtures__/metrics.ts`:
- Around line 272-288: The fixture mockMultiAttributeRows uses Math.random() (in
the Value field) which makes snapshots flaky; replace the non-deterministic
random with a deterministic source (e.g., a seeded pseudo-random generator or a
fixed array of offsets) so tests always get the same values. Locate the
mockMultiAttributeRows construction and change Value: cpuTimeBase[si]! +
Math.random() * 0.5 to derive the offset from a reproducible function/seed (tied
to i and si) or a predefined offsets array generated once, ensuring
cpuTimeStates and cpuTimeBase usage remains consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6678a49-46bb-49ab-b117-7f77dc80fac5
📒 Files selected for processing (17)
packages/cli/README.mdpackages/cli/src/commands/dashboards.tspackages/ui/src/components/observability/MetricHistogram/MetricHistogram.stories.tsxpackages/ui/src/components/observability/MetricHistogram/index.tsxpackages/ui/src/components/observability/MetricStat/MetricStat.stories.tsxpackages/ui/src/components/observability/MetricTimeSeries/MetricTimeSeries.stories.tsxpackages/ui/src/components/observability/MetricTimeSeries/index.tsxpackages/ui/src/components/observability/__fixtures__/metrics.tspackages/ui/src/components/observability/renderers/OtelMetricHistogram.tsxpackages/ui/src/components/observability/renderers/OtelMetricStat.tsxpackages/ui/src/components/observability/renderers/OtelMetricTimeSeries.tsxpackages/ui/src/components/observability/utils/attributes.tspackages/ui/src/components/observability/utils/units.test.tspackages/ui/src/components/observability/utils/units.tspackages/ui/src/lib/observability-catalog.tspackages/ui/src/pages/observability.test.tsxskills/create-dashboard/rules/workflow.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/README.md
- packages/ui/src/pages/observability.test.tsx
Comment on lines
+43
to
+46
| } catch (err) { | ||
| outputError(err, false); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Exit code should distinguish config errors from API/runtime errors.
Per coding guidelines, config errors (e.g., missing URL from resolveConnectionOpts) should use exit code 3, not 1. Currently, all errors exit with code 1.
🛠️ Proposed fix
} catch (err) {
+ // Config errors (e.g., missing URL) should exit with code 3
+ if (err instanceof Error && err.message.includes('url')) {
+ outputError(err, false);
+ process.exit(3);
+ }
outputError(err, isJson);
process.exit(1);
}Alternatively, consider having resolveConnectionOpts or a dedicated error type that can be checked for config-specific errors to properly route to exit code 3.
As per coding guidelines: "3 for config errors (missing url)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/dashboards.ts` around lines 43 - 46, The catch
block in dashboards.ts currently treats all errors the same and exits with code
1; modify it to distinguish configuration errors (from resolveConnectionOpts or
a dedicated ConfigError) from runtime/API errors by detecting the specific error
type or a property (e.g., instanceof ConfigError or err.code === 'MISSING_URL')
and call process.exit(3) for config errors and process.exit(1) for others;
ensure outputError(err, false) is still called before exiting so logs remain
intact.
Comment on lines
+63
to
+91
| ).action(async (opts: DashboardCreateOptions) => { | ||
| const isJson = opts.json ?? false; | ||
| try { | ||
| const client = createClient(opts); | ||
| const raw = await readStdin(); | ||
| let body: Record<string, unknown>; | ||
| try { | ||
| body = JSON.parse(raw) as Record<string, unknown>; | ||
| } catch (e) { | ||
| if (e instanceof SyntaxError) { | ||
| process.stderr.write(`Invalid JSON input: ${e.message}\n`); | ||
| process.exit(2); | ||
| } | ||
| throw e; | ||
| } | ||
|
|
||
| const result = await client.createDashboard({ | ||
| name: opts.name, | ||
| uiTreeVersion: opts.treeVersion, | ||
| uiTree: (body.uiTree ?? body) as Record<string, unknown>, | ||
| metadata: body.metadata as Record<string, unknown> | undefined, | ||
| }); | ||
|
|
||
| const format = detectFormat(opts.json, opts.table); | ||
| output(result, { format }); | ||
| } catch (err) { | ||
| outputError(err, isJson); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Create subcommand also needs exit code 3 for config errors.
Same issue as the schema subcommand: if createClient(opts) throws due to missing URL configuration, the error is caught and exits with code 1 instead of the required code 3 for config errors.
🛠️ Proposed fix
Consider extracting a shared error-handling helper that checks for config-related errors:
function handleCommandError(err: unknown, isJson: boolean): never {
outputError(err, isJson);
// Check if this is a config error (e.g., missing URL)
if (err instanceof Error && /\burl\b/i.test(err.message)) {
process.exit(3);
}
process.exit(1);
}Then use it in both subcommands:
} catch (err) {
- outputError(err, isJson);
- process.exit(1);
+ handleCommandError(err, isJson);
}As per coding guidelines: "3 for config errors (missing url)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/dashboards.ts` around lines 63 - 91, The create
subcommand currently exits with code 1 for all errors; modify error handling to
use a shared helper (e.g., handleCommandError) that calls outputError(err,
isJson) and then exits with code 3 for config-related errors (detectable by
checking err instanceof Error && /\burl\b/i.test(err.message) or other config
error patterns) otherwise exiting with code 1; update the action callback around
createClient(opts) and the outer catch to invoke handleCommandError(err, isJson)
instead of directly calling outputError and process.exit so createClient,
client.createDashboard, and related failures produce the correct exit code.
This was referenced Mar 9, 2026
This was referenced Apr 7, 2026
Merged
Open
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
New Features
API Changes
Documentation