feat(skills): optimize skills#90
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughExpanded observability skills/docs, added OTLP-over-HTTP guidance, introduced a root-cause "identify cause & present findings" workflow step that creates incident dashboards, hardened sqlite-datasource metric/JSON/null handling and types, and made the observability UI use an injectable Kopai client for dashboard retrieval with related UI/tests updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/otel-instrumentation/rules/setup-environment.md`:
- Around line 36-43: The "Default Protocol" column in the OTEL setup table is
inaccurate and should be removed or corrected; update the table in
skills/otel-instrumentation/rules/setup-environment.md by either removing the
"Default Protocol" column entirely or replacing each SDK row (referencing the
table rows for Node.js, Python, Go, Java, .NET, Rust) with protocol values
verified against each SDK/agent's official docs (e.g., .NET SDK, Python
zero-code auto-instrumentation, Go autoexport, Java agent 2.x) and add a brief
note that defaults are SDK/agent-specific and may change, or include links to
the authoritative config pages to avoid hardcoding potentially incorrect
defaults.
In `@skills/root-cause-analysis/rules/workflow-identify-cause.md`:
- Around line 41-43: The fenced code block containing the URL snippet
"<baseUrl>/?tab=metrics&dashboardId=<id>" should include a language specifier to
satisfy markdownlint MD040; update the triple-backtick fence in
workflow-identify-cause.md to use "text" (i.e., ```text) so the block remains
visually identical but clears the lint warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0067f751-4a5e-4992-a9b5-5fb252ee198f
📒 Files selected for processing (8)
skills/create-dashboard/SKILL.mdskills/create-dashboard/rules/workflow.mdskills/otel-instrumentation/SKILL.mdskills/otel-instrumentation/rules/_sections.mdskills/otel-instrumentation/rules/setup-environment.mdskills/root-cause-analysis/SKILL.mdskills/root-cause-analysis/rules/_sections.mdskills/root-cause-analysis/rules/workflow-identify-cause.md
Comment on lines
+36
to
+43
| | SDK | Default Protocol | Action Needed | | ||
| | ------- | ---------------- | ----------------------------------------------- | | ||
| | Node.js | HTTP | None | | ||
| | Python | HTTP | None | | ||
| | Go | gRPC | Set `OTEL_EXPORTER_OTLP_PROTOCOL=http/protobuf` | | ||
| | Java | gRPC | Set `OTEL_EXPORTER_OTLP_PROTOCOL=http/protobuf` | | ||
| | .NET | HTTP | None | | ||
| | Rust | HTTP | None | |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenTelemetry .NET SDK OTLP exporter default protocol
💡 Result:
In the OpenTelemetry .NET SDK, the OTLP exporter’s default transport protocol is gRPC (i.e., OtlpExportProtocol.Grpc). By default it sends to the OTLP/gRPC endpoint (commonly http://localhost:4317). [1], [2]
You can switch to HTTP/protobuf by setting OtlpExporterOptions.Protocol = OtlpExportProtocol.HttpProtobuf (or via OTEL_EXPORTER_OTLP_PROTOCOL=http/protobuf). [1], [2]
Sources:
[1] NuGet package docs for OpenTelemetry.Exporter.OpenTelemetryProtocol (default protocol) (nuget.org)
[2] OpenTelemetry .NET exporters documentation (default gRPC to localhost:4317) (opentelemetry.io)
Citations:
- 1: https://www.nuget.org/packages/opentelemetry.exporter.opentelemetryprotocol/?utm_source=openai
- 2: https://opentelemetry.io/docs/languages/net/exporters/?utm_source=openai
🌐 Web query:
OpenTelemetry OTLP exporter protocol defaults all SDKs
💡 Result:
Spec default (what SDKs should do)
- If
OTEL_EXPORTER_OTLP_PROTOCOL(or signal-specific variants) is not set, the OTLP exporter SHOULD default tohttp/protobuf(SDKs may choosegrpcfor backward-compat reasons). [1] - Standard default ports: OTLP/gRPC = 4317, OTLP/HTTP = 4318. [1]
What common SDKs actually default to (as documented)
- .NET SDK (library): defaults to gRPC (sending to
http://localhost:4317unless changed). [2] - .NET Automatic Instrumentation (zero-code): defaults to
http/protobuf. [3] - Java agent 2.x / OpenTelemetry Spring Boot starter: default is
http/protobuf. [4] - Java SDK (library): supports both; endpoint defaults differ by protocol (
4317for grpc,4318/v1/{signal}for http/protobuf), but the docs do not claim a single universal protocol default for all Java SDK setups. [4] - JavaScript: in practice, defaults depend on which exporter you instantiate; the OpenTelemetry JS docs show OTLP over
http/protobufwith a default URL ofhttp://localhost:4318/v1/metricswhenurlis omitted. [5] - Go: there isn’t one “OTLP exporter” with a single protocol default—there are distinct gRPC vs HTTP exporter packages (you pick one). [6]
Sources
[1] OpenTelemetry OTLP Exporter spec (protocol + ports): (opentelemetry.io)
[2] OpenTelemetry .NET exporters (default gRPC to http://localhost:4317): (opentelemetry.io)
[3] OpenTelemetry .NET zero-code config (auto-instrumentation defaults to http/protobuf): (opentelemetry.io)
[4] OpenTelemetry Java configuration (agent 2.x default http/protobuf; endpoint behavior by protocol): (opentelemetry.io)
[5] OpenTelemetry JS exporters (example default http://localhost:4318/v1/metrics): (opentelemetry.io)
[6] OpenTelemetry Go exporters (separate OTLP gRPC vs HTTP exporter packages): (opentelemetry.io)
Citations:
- 1: https://opentelemetry.io/docs/specs/otel/protocol/exporter/?utm_source=openai
- 2: https://opentelemetry.io/docs/languages/net/exporters/?utm_source=openai
- 3: https://opentelemetry.io/docs/zero-code/net/configuration/?utm_source=openai
- 4: https://opentelemetry.io/docs/languages/java/configuration/?utm_source=openai
- 5: https://opentelemetry.io/docs/languages/js/exporters/?utm_source=openai
- 6: https://opentelemetry.io/docs/languages/go/exporters/?utm_source=openai
🌐 Web query:
OpenTelemetry Python SDK OTLP default protocol
💡 Result:
- Python OTLP exporters are protocol-specific: you explicitly use either the gRPC exporter (
opentelemetry.exporter.otlp.proto.grpc...) or the HTTP/protobuf exporter (opentelemetry.exporter.otlp.proto.http...). [1] - For Python zero-code auto-instrumentation (
opentelemetry-instrument --traces_exporter otlp), theotlpoption defaults to OTLP/gRPC; to use HTTP/protobuf you set--exporter_otlp_protocol http/protobuf(or the equivalent env var). [2] - Spec note: the OTLP exporter specification says the default protocol should be
http/protobufunless an SDK keepsgrpcfor backward compatibility. [3]
Sources: [1] https://opentelemetry-python.readthedocs.io/en/latest/exporter/otlp/otlp.html, [2] https://opentelemetry.io/ro/docs/zero-code/python/configuration/, [3] https://opentelemetry.io/docs/specs/otel/protocol/exporter/
🌐 Web query:
OpenTelemetry Go SDK OTLP default protocol gRPC
💡 Result:
In the OpenTelemetry Go ecosystem, OTLP is not “gRPC by default” when you rely on the standard environment-variable based configuration (the autoexport module):
OTEL_EXPORTER_OTLP_PROTOCOLsupportshttp/protobufandgrpc, and its default ishttp/protobuf. Set it togrpcto use OTLP/gRPC. [1]- The OTLP exporter specification also states the default protocol should be
http/protobufunless an SDK has strong reasons to choosegrpc. [2]
If you configure exporters directly in code (without autoexport), you choose the protocol by which exporter you instantiate (e.g., otlptracegrpc vs otlptracehttp). [1]
Sources
[1] OpenTelemetry Go autoexport docs (pkg.go.dev) (pkg.go.dev)
[2] OpenTelemetry OTLP exporter specification (opentelemetry.io)
Citations:
- 1: https://pkg.go.dev/go.opentelemetry.io/contrib/exporters/autoexport
- 2: https://opentelemetry.io/docs/specs/otel/protocol/exporter/?utm_source=openai
Remove or correct the Default Protocol column—it contains multiple inaccuracies.
The table has unverified SDK defaults that contradict official documentation:
- .NET SDK defaults to gRPC (not HTTP per the table)
- Python zero-code auto-instrumentation defaults to gRPC (not HTTP)
- Go's autoexport defaults to http/protobuf (not gRPC)
- Java agent 2.x defaults to http/protobuf (not gRPC)
The OpenTelemetry spec says SDKs should default to http/protobuf but allow gRPC for backward compatibility—making each SDK's actual default SDK-dependent and prone to drift. Either remove this column or cross-reference each row against the upstream SDK's official configuration docs to prevent sending users down incorrect setup paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/otel-instrumentation/rules/setup-environment.md` around lines 36 - 43,
The "Default Protocol" column in the OTEL setup table is inaccurate and should
be removed or corrected; update the table in
skills/otel-instrumentation/rules/setup-environment.md by either removing the
"Default Protocol" column entirely or replacing each SDK row (referencing the
table rows for Node.js, Python, Go, Java, .NET, Rust) with protocol values
verified against each SDK/agent's official docs (e.g., .NET SDK, Python
zero-code auto-instrumentation, Go autoexport, Java agent 2.x) and add a brief
note that defaults are SDK/agent-specific and may change, or include links to
the authoritative config pages to avoid hardcoding potentially incorrect
defaults.
Comment on lines
+41
to
+43
| ``` | ||
| <baseUrl>/?tab=metrics&dashboardId=<id> | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced URL block.
Markdownlint is already flagging this block with MD040. Using text keeps the rendering the same and clears the warning.
Suggested fix
-```
+```text
<baseUrl>/?tab=metrics&dashboardId=<id></details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/root-cause-analysis/rules/workflow-identify-cause.md` around lines 41
- 43, The fenced code block containing the URL snippet
"<baseUrl>/?tab=metrics&dashboardId=<id>" should include a language specifier to
satisfy markdownlint MD040; update the triple-backtick fence in
workflow-identify-cause.md to use "text" (i.e., ```text) so the block remains
visually identical but clears the lint warning.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/sqlite-datasource/src/datasource-write.test.ts (1)
848-893: Consider pinning the read-path contract too.This change is user-visible through
getLogs(), but the test only verifies the raw SQLite row. A companion read assertion would catch regressions where storage stays""while the mapper starts returningundefinedornullagain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqlite-datasource/src/datasource-write.test.ts` around lines 848 - 893, The test currently only checks the raw SQLite row after calling ds.writeLogs(logsData) but doesn't assert the read-path; add a companion assertion that calls ds.getLogs(...) (or the relevant read method) to verify the public mapper returns the expected values (e.g., Timestamp, TraceId, SpanId, TraceFlags, SeverityText, SeverityNumber, Body, LogAttributes, ResourceAttributes, ServiceName, ScopeName, etc.) and that none of those fields are undefined/null and they equal the expected minimal values (empty string, 0, "{}" or 1000000000) so read-path regressions are caught; locate the test around the existing it("handles logs without optional fields") block and use ds.getLogs (or the public reader function in the test suite) to fetch and assert the mapped output.
🤖 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/db-datasource.ts`:
- Line 1486: The code currently coerces missing OTLP sum values to 0 on write
(in toHistogramRow, toExpHistogramRow, toSummaryRow) and only converts NULL back
on read with toNumber(row.Sum), which causes newly ingested omitted sums to
round-trip as 0; change the write logic to persist NULL when sum is omitted (use
dataPoint.sum === undefined ? null : dataPoint.sum or dataPoint.sum ?? null) in
toHistogramRow, toExpHistogramRow, and toSummaryRow, and ensure the read path
for Sum (e.g., where Sum is converted) treats NULL appropriately (use a
conversion that returns null/undefined rather than 0) so omitted sums remain
omitted.
- Around line 1349-1357: The deserialization currently only strips nulls but
still accepts arrays and objects into AttributeValue, violating the expected
simple type contract; when building the result Record in db-datasource.ts (the
parsed -> result loop), filter values to only allow primitives (typeof v ===
"string" || typeof v === "number" || typeof v === "boolean") and skip
arrays/objects (and nulls) so returned AttributeValue values conform to the
local schema and downstream zod validation; adjust the loop that iterates
Object.entries(parsed) (and any related anyValueToSimple usage) to perform this
primitive-type check before assigning to result.
In `@packages/ui/src/components/observability/MetricTimeSeries/index.tsx`:
- Around line 109-116: The code currently uses Count alone as the metric value
(variables row, sum, count, value), which is incorrect because Count is sample
cardinality; change the logic so you only set value when both Sum and Count are
present (use sum/count) and otherwise skip that row (do not set value or push a
point); if you need to preserve counts, emit a separate series for Count with an
appropriate MetricUnit instead of reusing the original metric's unit.
In `@skills/create-dashboard/rules/workflow.md`:
- Around line 57-65: The Component Compatibility docs incorrectly omit that
MetricTimeSeries also accepts ExponentialHistogram and Summary rows when
Sum/Count are present; update the text in the MetricTimeSeries bullet to state
it works with Sum, Gauge, Histogram, ExponentialHistogram and Summary (the
latter two only when Sum/Count are available), and add a parenthetical note
referencing the renderer behavior implemented in
packages/ui/src/components/observability/MetricTimeSeries/index.tsx (where
Sum/Count enables rendering for Summary/ExponentialHistogram).
---
Nitpick comments:
In `@packages/sqlite-datasource/src/datasource-write.test.ts`:
- Around line 848-893: The test currently only checks the raw SQLite row after
calling ds.writeLogs(logsData) but doesn't assert the read-path; add a companion
assertion that calls ds.getLogs(...) (or the relevant read method) to verify the
public mapper returns the expected values (e.g., Timestamp, TraceId, SpanId,
TraceFlags, SeverityText, SeverityNumber, Body, LogAttributes,
ResourceAttributes, ServiceName, ScopeName, etc.) and that none of those fields
are undefined/null and they equal the expected minimal values (empty string, 0,
"{}" or 1000000000) so read-path regressions are caught; locate the test around
the existing it("handles logs without optional fields") block and use ds.getLogs
(or the public reader function in the test suite) to fetch and assert the mapped
output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38a550d4-b030-4c43-80fe-edacd1b2fa1d
📒 Files selected for processing (5)
packages/sqlite-datasource/src/datasource-write.test.tspackages/sqlite-datasource/src/db-datasource.tspackages/ui/src/components/observability/MetricTimeSeries/index.tsxpackages/ui/src/components/observability/renderers/OtelLogTimeline.tsxskills/create-dashboard/rules/workflow.md
Comment on lines
+1349
to
+1357
| const parsed = JSON.parse(value); | ||
| if (typeof parsed !== "object" || parsed === null) return undefined; | ||
| // Strip null values — OTLP attributes with unrecognized AnyValue types | ||
| // were previously stored as null, which fails Zod attributeValue validation | ||
| const result: Record<string, AttributeValue> = {}; | ||
| for (const [k, v] of Object.entries(parsed)) { | ||
| if (v != null) result[k] = v as AttributeValue; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Filter non-primitive attribute values here too.
This only removes null; arrays and nested objects still get cast into AttributeValue even though the local contract is string | number | boolean. Because anyValueToSimple() can emit arrays/objects for arrayValue and kvlistValue, the read path can still return values that violate the declared type/schema.
Possible fix
- const parsed = JSON.parse(value);
- if (typeof parsed !== "object" || parsed === null) return undefined;
+ const parsed = JSON.parse(value);
+ if (
+ typeof parsed !== "object" ||
+ parsed === null ||
+ Array.isArray(parsed)
+ ) {
+ return undefined;
+ }
// Strip null values — OTLP attributes with unrecognized AnyValue types
// were previously stored as null, which fails Zod attributeValue validation
const result: Record<string, AttributeValue> = {};
for (const [k, v] of Object.entries(parsed)) {
- if (v != null) result[k] = v as AttributeValue;
+ if (
+ typeof v === "string" ||
+ typeof v === "number" ||
+ typeof v === "boolean"
+ ) {
+ result[k] = v;
+ }
}📝 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
| const parsed = JSON.parse(value); | |
| if (typeof parsed !== "object" || parsed === null) return undefined; | |
| // Strip null values — OTLP attributes with unrecognized AnyValue types | |
| // were previously stored as null, which fails Zod attributeValue validation | |
| const result: Record<string, AttributeValue> = {}; | |
| for (const [k, v] of Object.entries(parsed)) { | |
| if (v != null) result[k] = v as AttributeValue; | |
| } | |
| return result; | |
| const parsed = JSON.parse(value); | |
| if ( | |
| typeof parsed !== "object" || | |
| parsed === null || | |
| Array.isArray(parsed) | |
| ) { | |
| return undefined; | |
| } | |
| // Strip null values — OTLP attributes with unrecognized AnyValue types | |
| // were previously stored as null, which fails Zod attributeValue validation | |
| const result: Record<string, AttributeValue> = {}; | |
| for (const [k, v] of Object.entries(parsed)) { | |
| if ( | |
| typeof v === "string" || | |
| typeof v === "number" || | |
| typeof v === "boolean" | |
| ) { | |
| result[k] = v; | |
| } | |
| } | |
| return result; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sqlite-datasource/src/db-datasource.ts` around lines 1349 - 1357,
The deserialization currently only strips nulls but still accepts arrays and
objects into AttributeValue, violating the expected simple type contract; when
building the result Record in db-datasource.ts (the parsed -> result loop),
filter values to only allow primitives (typeof v === "string" || typeof v ===
"number" || typeof v === "boolean") and skip arrays/objects (and nulls) so
returned AttributeValue values conform to the local schema and downstream zod
validation; adjust the loop that iterates Object.entries(parsed) (and any
related anyValueToSimple usage) to perform this primitive-type check before
assigning to result.
Comment on lines
+57
to
+65
| ## Component Compatibility | ||
|
|
||
| - **MetricStat** — works with **Sum** and **Gauge** only. Does NOT work with Histogram (shows "--") | ||
| - **MetricTimeSeries** — works with **Sum**, **Gauge**, and **Histogram** (renders mean duration over time) | ||
| - **MetricHistogram** — works with **Histogram** and **ExponentialHistogram** only | ||
|
|
||
| When choosing components, always check the metric's `type` from `metrics discover` output. Mismatched types render empty or show "--". | ||
|
|
||
| **For Histogram metrics**: use `MetricHistogram` for distribution views, or `MetricTimeSeries` for trends over time (renders mean = Sum/Count). `MetricStat` is NOT compatible with Histogram. |
There was a problem hiding this comment.
Update the MetricTimeSeries compatibility docs.
The renderer also accepts ExponentialHistogram and Summary rows when Sum/Count is available in packages/ui/src/components/observability/MetricTimeSeries/index.tsx at Lines 104-117. Leaving them out here will steer users away from valid dashboard configs.
📝 Suggested doc update
-- **MetricTimeSeries** — works with **Sum**, **Gauge**, and **Histogram** (renders mean duration over time)
+- **MetricTimeSeries** — works with **Sum**, **Gauge**, **Histogram**, **ExponentialHistogram**, and **Summary** when `Sum` and `Count` are available (renders mean over time)
-**For Histogram metrics**: use `MetricHistogram` for distribution views, or `MetricTimeSeries` for trends over time (renders mean = Sum/Count). `MetricStat` is NOT compatible with Histogram.
+**For distribution metrics**: use `MetricHistogram` for distribution views, or `MetricTimeSeries` for trends over time (renders mean = `Sum/Count` when available). `MetricStat` is not compatible with Histogram, ExponentialHistogram, or Summary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/create-dashboard/rules/workflow.md` around lines 57 - 65, The
Component Compatibility docs incorrectly omit that MetricTimeSeries also accepts
ExponentialHistogram and Summary rows when Sum/Count are present; update the
text in the MetricTimeSeries bullet to state it works with Sum, Gauge,
Histogram, ExponentialHistogram and Summary (the latter two only when Sum/Count
are available), and add a parenthetical note referencing the renderer behavior
implemented in
packages/ui/src/components/observability/MetricTimeSeries/index.tsx (where
Sum/Count enables rendering for Summary/ExponentialHistogram).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/sqlite-datasource/src/datasource-write.test.ts (1)
895-896: Avoid theunknowncast to access read operations on a narrowly-typed fixture.The test fixture is typed as
WriteTelemetryDatasource, butcreateOptimizedDatasource()returnsOptimizedDatasource, which implementsTelemetryDatasource— a type that includes both write and read operations. The unsafe cast bypasses this and would only fail at runtime if the instance lackedgetLogs(). Type the fixture as the full intersection (WriteTelemetryDatasource & ReadTelemetryDatasource) or asOptimizedDatasourcedirectly to enableds.getLogs(...)without casting.Suggested change
- let ds: datasource.WriteTelemetryDatasource; + let ds: + & datasource.WriteTelemetryDatasource + & datasource.ReadTelemetryDatasource; … - const readDs = ds as unknown as datasource.ReadTelemetryDatasource; - const readResult = await readDs.getLogs({ limit: 10 }); + const readResult = await ds.getLogs({ limit: 10 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sqlite-datasource/src/datasource-write.test.ts` around lines 895 - 896, The test currently forces an unsafe cast using "ds as unknown as datasource.ReadTelemetryDatasource" to call getLogs; instead update the fixture typing so the ds variable is declared as a type that includes read operations (either OptimizedDatasource or the intersection WriteTelemetryDatasource & ReadTelemetryDatasource) returned by createOptimizedDatasource(), then call ds.getLogs({ limit: 10 }) directly. Locate the test where ds is created with createOptimizedDatasource() and change its declared type to OptimizedDatasource or WriteTelemetryDatasource & ReadTelemetryDatasource so getLogs is available without an unknown cast (refer to symbols createOptimizedDatasource, ds, WriteTelemetryDatasource, ReadTelemetryDatasource, OptimizedDatasource, and getLogs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/cute-pandas-cheer.md:
- Around line 2-6: The changeset .changeset/cute-pandas-cheer.md lists both
"@kopai/ui" and "@kopai/sqlite-datasource" but the description only documents a
UI change; update the changeset so the package bumps match the release note by
either removing "@kopai/sqlite-datasource" from the header if no sqlite-related
changes are included, or expand the description to explicitly mention the
sqlite-datasource change (what changed and why) so the release entry is
accurate; locate the header lines with "@kopai/ui" and
"@kopai/sqlite-datasource" and edit the file accordingly.
In `@packages/ui/src/pages/observability.test.tsx`:
- Around line 79-80: Add an assertion that the page forwards the correct
dashboardId to the API: after rendering/waiting in the "renders DynamicDashboard
when API returns a valid uiTree" test (and the similar one at 97-105), assert
that mockClient.getDashboard was called with the expected dashboard id (e.g.,
expect(mockClient.getDashboard).toHaveBeenCalledWith(expectedDashboardId));
locate the expected id from the test setup (route param, query, or constant used
to render the page) and use that value for expectedDashboardId so the test
verifies URL → mockClient.getDashboard wiring rather than only the mocked
response rendering.
---
Nitpick comments:
In `@packages/sqlite-datasource/src/datasource-write.test.ts`:
- Around line 895-896: The test currently forces an unsafe cast using "ds as
unknown as datasource.ReadTelemetryDatasource" to call getLogs; instead update
the fixture typing so the ds variable is declared as a type that includes read
operations (either OptimizedDatasource or the intersection
WriteTelemetryDatasource & ReadTelemetryDatasource) returned by
createOptimizedDatasource(), then call ds.getLogs({ limit: 10 }) directly.
Locate the test where ds is created with createOptimizedDatasource() and change
its declared type to OptimizedDatasource or WriteTelemetryDatasource &
ReadTelemetryDatasource so getLogs is available without an unknown cast (refer
to symbols createOptimizedDatasource, ds, WriteTelemetryDatasource,
ReadTelemetryDatasource, OptimizedDatasource, and getLogs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2c51952-5a55-4c39-898e-f3165b6805f7
📒 Files selected for processing (11)
.changeset/cute-pandas-cheer.md.changeset/fine-eyes-own.mdpackages/sqlite-datasource/src/datasource-write.test.tspackages/sqlite-datasource/src/db-datasource.tspackages/sqlite-datasource/src/db-types.tspackages/ui/src/components/observability/MetricTimeSeries/index.tsxpackages/ui/src/hooks/use-kopai-data.test.tspackages/ui/src/hooks/use-live-logs.test.tspackages/ui/src/pages/observability.test.tsxpackages/ui/src/pages/observability.tsxpackages/ui/src/providers/kopai-provider.tsx
Comment on lines
+2
to
+6
| "@kopai/ui": minor | ||
| "@kopai/sqlite-datasource": patch | ||
| --- | ||
|
|
||
| Use kopai SDK to get dashboard |
There was a problem hiding this comment.
Align the sqlite-datasource bump with the release note.
This changeset description is UI-only, but it also bumps @kopai/sqlite-datasource. That will generate a misleading release entry unless you either remove the sqlite package from this changeset or mention the sqlite-facing change here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/cute-pandas-cheer.md around lines 2 - 6, The changeset
.changeset/cute-pandas-cheer.md lists both "@kopai/ui" and
"@kopai/sqlite-datasource" but the description only documents a UI change;
update the changeset so the package bumps match the release note by either
removing "@kopai/sqlite-datasource" from the header if no sqlite-related changes
are included, or expand the description to explicitly mention the
sqlite-datasource change (what changed and why) so the release entry is
accurate; locate the header lines with "@kopai/ui" and
"@kopai/sqlite-datasource" and edit the file 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
New Features
Bug Fixes
Documentation
Tests