Feat/UI cleanup#116
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR removes several dashboard UI components and the dashboard catalog, migrates remaining dashboard components to use the observability catalog, refactors renderer/data plumbing to use typed Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer
participant ElementRenderer
participant DataSourceElement
participant useKopaiData
participant KopaiClient
User->>Renderer: render catalog elements (may pass acceptsDataFromByType)
Renderer->>ElementRenderer: render element tree
ElementRenderer->>DataSourceElement: encounter element with dataSource
DataSourceElement->>Renderer: check dataSource.method ∈ acceptsDataFrom
alt method accepted
DataSourceElement->>useKopaiData: fetch(method, params, signal)
useKopaiData->>KopaiClient: call API (searchMetricsPage / searchAggregatedMetrics / ...)
KopaiClient-->>useKopaiData: response
useKopaiData-->>DataSourceElement: { response, isLoading, error }
DataSourceElement->>ElementRenderer: render child with response prop
else method rejected
DataSourceElement->>ElementRenderer: render "dataSource method not accepted" (no hasData)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 4
🧹 Nitpick comments (2)
packages/ui/src/hooks/use-kopai-data.test.ts (1)
156-184: Strengthen this test by asserting the exact aggregated call payload.Current checks prove method selection, but not argument correctness (Line 182). A wrong params mapping could still pass.
Proposed test hardening
expect(result.current.data).toEqual(mockData); - expect(mockClient.searchAggregatedMetrics).toHaveBeenCalled(); + expect(mockClient.searchAggregatedMetrics).toHaveBeenCalledWith( + { + metricType: "Sum", + metricName: "kopai.ingestion.bytes", + aggregate: "sum", + groupBy: ["signal"], + }, + expect.objectContaining({ signal: expect.any(AbortSignal) }) + ); + expect(mockClient.searchAggregatedMetrics).toHaveBeenCalledTimes(1); expect(mockClient.searchMetricsPage).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/hooks/use-kopai-data.test.ts` around lines 156 - 184, The test currently verifies that useKopaiData chooses searchAggregatedMetrics but not that it passes the correct parameters; update the assertion to check mockClient.searchAggregatedMetrics was called with the exact mapped payload derived from the dataSource.params (e.g., metricType, metricName, aggregate, groupBy and any expected defaults like cursor/page size), referencing the test's dataSource object and the useKopaiData hook to construct the expected call, and replace or augment the expect(mockClient.searchAggregatedMetrics).toHaveBeenCalled() with an expect(mockClient.searchAggregatedMetrics).toHaveBeenCalledWith(<expectedPayload>) assertion to ensure correct argument mapping.packages/ui/src/lib/observability-catalog.ts (1)
119-151: Consider encoding metric-type compatibility alongside the method.
searchMetricsPageis still broad here, soMetricTimeSeries,MetricHistogram, andMetricTablerely on descriptions to avoid invalid pairings. If this metadata is now part of the contract, adding something likeacceptsMetricTypeswould make those constraints enforceable instead of implicit.Based on learnings: When using Metrics search, the --type flag is required and must be one of: Gauge, Sum, Histogram, ExponentialHistogram, Summary
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/lib/observability-catalog.ts` around lines 119 - 151, Add explicit metric-type compatibility metadata alongside acceptsDataFrom for components like MetricTimeSeries, MetricHistogram, MetricTable, and MetricStat by introducing an acceptsMetricTypes property (an array of literal metric type strings) and populate it with the allowed values from the metrics search --type flag (e.g., "Gauge", "Sum", "Histogram", "ExponentialHistogram", "Summary"); update each component object (MetricTimeSeries, MetricHistogram, MetricTable, MetricStat) to include the appropriate subset of these strings so the contract enforces valid pairings rather than relying on descriptions.
🤖 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/ui/src/lib/component-catalog.ts`:
- Line 65: The acceptsDataFrom schema currently allows any string; change it to
validate only known data-source method names (e.g., use
z.array(z.enum([...knownNames])) or z.array(z.nativeEnum(DataSourceMethod)) or
z.array(z.string().refine(s => s in DataSourceMethodMap)) so typos fail
validation. Locate acceptsDataFrom in component-catalog.ts and replace
z.array(z.string()).readonly().optional() with a schema that sources its allowed
values from the canonical list/type of data-source methods used by InferData
(e.g., DataSourceMethod, DataSourceMethodNames, or the DataSourceMethodMap) so
the compiler and runtime validation keep InferData<CD> typed correctly.
In `@packages/ui/src/lib/generate-prompt-instructions.ts`:
- Around line 178-180: The current fallback string advertises dataSource support
by default by returning "Accepts dataSource: yes" when def.acceptsDataFrom is
missing; update the logic that builds this string (the expression using
def.acceptsDataFrom?.length) to only claim support when def.acceptsDataFrom
exists and has entries, and use a negative fallback such as "Accepts dataSource:
no" for the else path so leaf components without acceptsDataFrom are not
advertised as supporting data binding.
- Around line 62-66: The reqStr logic treats required-but-nullable props as
optional; update the classification for isNullable/isRequired so all four states
are expressed: compute isNullable = typeStr.endsWith("| null") and isRequired =
required.has(key) (do not negate with isNullable), then set reqStr to exactly
one of "(required)", "(required, may be null)", "(optional)", or "(optional, may
be null)" based on isRequired and isNullable (use the existing symbols
isNullable, isRequired, reqStr, typeStr, required.has(key) to locate and change
the logic).
In `@packages/ui/src/lib/renderer.tsx`:
- Around line 47-58: The renderer currently trusts runtime element.dataSource
and injects its response into components, bypassing the compile-time helpers
(SDKResponseFor, InferData and component acceptsDataFrom); add a runtime check
in the rendering path (where element is processed before creating response) that
compares element.dataSource.method against
catalog.components[element.type]?.acceptsDataFrom (or treats missing
acceptsDataFrom as empty) and throw or return a clear failure if the method is
not included; ensure this check runs before invoking KopaiClient methods and
before setting the response passed to the component so the runtime tree cannot
supply an incompatible dataSource.
---
Nitpick comments:
In `@packages/ui/src/hooks/use-kopai-data.test.ts`:
- Around line 156-184: The test currently verifies that useKopaiData chooses
searchAggregatedMetrics but not that it passes the correct parameters; update
the assertion to check mockClient.searchAggregatedMetrics was called with the
exact mapped payload derived from the dataSource.params (e.g., metricType,
metricName, aggregate, groupBy and any expected defaults like cursor/page size),
referencing the test's dataSource object and the useKopaiData hook to construct
the expected call, and replace or augment the
expect(mockClient.searchAggregatedMetrics).toHaveBeenCalled() with an
expect(mockClient.searchAggregatedMetrics).toHaveBeenCalledWith(<expectedPayload>)
assertion to ensure correct argument mapping.
In `@packages/ui/src/lib/observability-catalog.ts`:
- Around line 119-151: Add explicit metric-type compatibility metadata alongside
acceptsDataFrom for components like MetricTimeSeries, MetricHistogram,
MetricTable, and MetricStat by introducing an acceptsMetricTypes property (an
array of literal metric type strings) and populate it with the allowed values
from the metrics search --type flag (e.g., "Gauge", "Sum", "Histogram",
"ExponentialHistogram", "Summary"); update each component object
(MetricTimeSeries, MetricHistogram, MetricTable, MetricStat) to include the
appropriate subset of these strings so the contract enforces valid pairings
rather than relying on descriptions.
🪄 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: 0422c133-d3f1-40be-b520-2111318dbc23
⛔ Files ignored due to path filters (1)
packages/ui/src/lib/__snapshots__/generate-prompt-instructions.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (47)
packages/ui/src/components/dashboard/Badge/index.tsxpackages/ui/src/components/dashboard/Button/Button.stories.tsxpackages/ui/src/components/dashboard/Button/index.tsxpackages/ui/src/components/dashboard/Card/index.tsxpackages/ui/src/components/dashboard/Chart/Chart.stories.tsxpackages/ui/src/components/dashboard/Chart/index.tsxpackages/ui/src/components/dashboard/DatePicker/DatePicker.stories.tsxpackages/ui/src/components/dashboard/DatePicker/index.tsxpackages/ui/src/components/dashboard/Divider/index.tsxpackages/ui/src/components/dashboard/Empty/index.tsxpackages/ui/src/components/dashboard/Grid/index.tsxpackages/ui/src/components/dashboard/Heading/index.tsxpackages/ui/src/components/dashboard/List/List.stories.tsxpackages/ui/src/components/dashboard/List/index.tsxpackages/ui/src/components/dashboard/Metric/Metric.stories.tsxpackages/ui/src/components/dashboard/Metric/index.tsxpackages/ui/src/components/dashboard/Stack/index.tsxpackages/ui/src/components/dashboard/Table/Table.stories.tsxpackages/ui/src/components/dashboard/Table/index.tsxpackages/ui/src/components/dashboard/Text/index.tsxpackages/ui/src/components/dashboard/index.tspackages/ui/src/components/index.tspackages/ui/src/components/observability/LogTimeline/LogRow.tsxpackages/ui/src/components/observability/TraceTimeline/FlamegraphView.tsxpackages/ui/src/components/observability/TraceTimeline/SpanRow.tsxpackages/ui/src/components/observability/TraceTimeline/index.tsxpackages/ui/src/components/observability/renderers/NoDataSource.tsxpackages/ui/src/components/observability/renderers/OtelLogTimeline.tsxpackages/ui/src/components/observability/renderers/OtelMetricDiscovery.tsxpackages/ui/src/components/observability/renderers/OtelMetricHistogram.tsxpackages/ui/src/components/observability/renderers/OtelMetricStat.tsxpackages/ui/src/components/observability/renderers/OtelMetricTable.tsxpackages/ui/src/components/observability/renderers/OtelMetricTimeSeries.tsxpackages/ui/src/components/observability/renderers/OtelTraceDetail.tsxpackages/ui/src/components/observability/utils/flatten-tree.tspackages/ui/src/components/observability/utils/lttb.tspackages/ui/src/hooks/use-kopai-data.test.tspackages/ui/src/hooks/use-kopai-data.tspackages/ui/src/index.tspackages/ui/src/lib/catalog.tspackages/ui/src/lib/component-catalog.tspackages/ui/src/lib/generate-prompt-instructions.tspackages/ui/src/lib/observability-catalog.tspackages/ui/src/lib/renderer.test.tsxpackages/ui/src/lib/renderer.tsxpackages/ui/src/pages/observability.tsxskills/create-dashboard/rules/workflow.md
💤 Files with no reviewable changes (20)
- packages/ui/src/components/observability/TraceTimeline/SpanRow.tsx
- packages/ui/src/components/observability/TraceTimeline/index.tsx
- packages/ui/src/components/observability/utils/flatten-tree.ts
- packages/ui/src/components/dashboard/List/index.tsx
- packages/ui/src/components/dashboard/DatePicker/index.tsx
- packages/ui/src/components/dashboard/Chart/index.tsx
- packages/ui/src/components/dashboard/DatePicker/DatePicker.stories.tsx
- packages/ui/src/lib/catalog.ts
- packages/ui/src/components/dashboard/Button/Button.stories.tsx
- packages/ui/src/components/dashboard/Metric/Metric.stories.tsx
- packages/ui/src/components/dashboard/Table/Table.stories.tsx
- packages/ui/src/components/dashboard/List/List.stories.tsx
- packages/ui/src/components/dashboard/Button/index.tsx
- packages/ui/src/components/dashboard/Metric/index.tsx
- packages/ui/src/components/observability/utils/lttb.ts
- packages/ui/src/components/dashboard/Table/index.tsx
- packages/ui/src/components/dashboard/Chart/Chart.stories.tsx
- packages/ui/src/components/dashboard/index.ts
- packages/ui/src/components/index.ts
- packages/ui/src/index.ts
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 `@packages/ui/src/lib/generate-prompt-instructions.ts`:
- Around line 182-184: The generated instructions claim per-component
acceptsDataFrom methods (uses def.acceptsDataFrom) but buildExampleElements
still hardcodes searchTracesPage for every leaf, causing contradictions for
components like MetricStat and MetricDiscovery; update buildExampleElements to
consult each component's acceptsDataFrom (or def.acceptsDataFrom) and pick an
example dataSource method that actually appears in that list (fall back to a
generic/none option if empty) instead of always using searchTracesPage so
example trees match the accepted-method docs.
In `@packages/ui/src/lib/renderer.tsx`:
- Around line 324-329: The new default of acceptsDataFromByType = {} causes the
exported Renderer to reject every data-backed element; change the parameter to
be optional (default undefined) and update the guard that checks
acceptsDataFrom?.includes(...) to only run validation when acceptsDataFromByType
is provided, so omitting the map means "skip validation" and preserves existing
behavior for Renderer (leave createRendererFromCatalog consumers able to pass
explicit metadata if they want strict checks); reference acceptsDataFromByType,
Renderer, useKopaiData, and createRendererFromCatalog when making this change.
🪄 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: 75c3a471-48e4-4c34-be53-5e1cda5d10bb
⛔ Files ignored due to path filters (1)
packages/ui/src/lib/__snapshots__/generate-prompt-instructions.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
packages/ui/src/hooks/use-kopai-data.test.tspackages/ui/src/lib/generate-prompt-instructions.tspackages/ui/src/lib/renderer.test.tsxpackages/ui/src/lib/renderer.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/hooks/use-kopai-data.test.ts
- packages/ui/src/lib/renderer.test.tsx
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 `@packages/ui/src/lib/component-catalog.ts`:
- Around line 56-63: The current cast widens the mapped array to string, losing
literal types so DataSourceMethod collapses to string; fix by preserving literal
tuple when calling z.enum — e.g. call z.enum(dataSourceSchema.options.map(o =>
o.shape.method.value) as const) (or otherwise construct a readonly tuple of
those literal values) so z.infer<typeof dataSourceMethodSchema> yields the
literal union; update usages like DataSourceMethod and acceptsDataFrom to rely
on the restored literal union.
🪄 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: 1a6aec86-ba3f-424e-b0c7-7060b6c0631c
⛔ Files ignored due to path filters (1)
packages/ui/src/lib/__snapshots__/generate-prompt-instructions.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
packages/ui/src/lib/component-catalog.ts
| "Component description to be displayed by the prompt generator" | ||
| ), | ||
| props: z.unknown(), | ||
| acceptsDataFrom: z.array(dataSourceMethodSchema).readonly().optional(), |
There was a problem hiding this comment.
Enforce acceptsDataFrom in the generated element schemas.
Right now this is metadata only. createCatalog() still uses dataSource: dataSourceSchema.optional() on Lines 151-152 for every component, so a component that omits acceptsDataFrom can still validate with any dataSource, and a component that lists a subset is not narrowed to it. Invalid trees will pass uiTreeSchema and fail later in the renderer.
This was referenced Apr 16, 2026
Merged
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
Refactor
responseand enforces accepted data-source methods.Documentation