Feat/jaeger like spans#97
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds trace-discovery and summary functionality end-to-end: new getServices/getOperations/getTraceSummaries APIs, query builders, ClickHouse/SQLite implementations and tests, SDK/client and provider surface expansions, and many UI trace-search, visualization, timeline, and comparison components wired into observability flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as "Browser UI"
participant SDK as "Kopai SDK"
participant API as "Server / API"
participant DS as "Datasource (ClickHouse/SQLite)"
participant DB as "DB / ClickHouse"
User->>UI: Open Trace Search / Submit filters
UI->>SDK: getServices / getOperations / searchTraceSummariesPage
SDK->>API: HTTP GET/POST /signals/...
API->>DS: call getServices/getOperations/getTraceSummaries
DS->>DB: execute built query (bindings)
DB-->>DS: stream rows + headers
DS-->>API: aggregated data + nextCursor
API-->>SDK: JSON response
SDK-->>UI: services/operations/summaries
UI->>User: render scatter/timeline/compare views
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)
📝 Coding Plan
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: 15
🧹 Nitpick comments (11)
packages/ui/src/components/observability/LogTimeline/LogFilter.tsx (1)
278-307: Consider adding an ESLint suppression comment for missing dependencies.The
valueandonChangeare intentionally omitted from the dependency array to prevent infinite loops, but this will triggerreact-hooks/exhaustive-depswarnings. Adding a suppression comment would document the intentional pattern:📝 Suggested documentation
+ // Sync debounced text values to parent. + // Intentionally omitting `value` and `onChange` from deps: + // - Including `value` would cause infinite loops (onChange updates value) + // - Effect reads latest `value` from closure when debounced values change + // eslint-disable-next-line react-hooks/exhaustive-deps useEffect(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/LogTimeline/LogFilter.tsx` around lines 278 - 307, The useEffect in LogFilter.tsx (function using isFirstRender.current) intentionally omits value and onChange from its dependency array which triggers react-hooks/exhaustive-deps; add an ESLint suppression comment above the useEffect (e.g., // eslint-disable-next-line react-hooks/exhaustive-deps) and include a short inline justification referencing why value and onChange are excluded (to prevent infinite loops) so reviewers and linters understand the intentional omission while leaving the effect dependencies as [dBodyContains, dTraceId, dSpanId, dLogAttrs, dResAttrs, dScopeAttrs].packages/ui/src/components/observability/TraceTimeline/TimelineBar.tsx (1)
45-52: Label may overflow when bar is near the right edge.When
leftPercent + widthPercentapproaches 100%, the duration label positioned at+ 4pxto the right would extend outside the container bounds. Depending on parent overflow settings, this could be clipped or cause layout issues.Consider checking if the label would overflow and conditionally positioning it to the left of the bar or omitting it in edge cases.
💡 Example approach
{!isWide && leftPercent + widthPercent < 90 && ( <span className="absolute top-1/2 -translate-y-1/2 text-[10px] font-mono text-muted-foreground whitespace-nowrap" style={{ left: `calc(${leftPercent + widthPercent}% + 4px)` }} > {durationLabel} </span> )}Or use a more sophisticated approach that measures available space or positions the label to the left when near the edge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/TraceTimeline/TimelineBar.tsx` around lines 45 - 52, The duration label positioned to the right of the bar can overflow when leftPercent + widthPercent is near 100%; update the rendering logic in TimelineBar (the span that uses leftPercent, widthPercent, durationLabel and isWide) to detect edge cases and avoid overflow by conditionally switching placement: if leftPercent + widthPercent is sufficiently below the right edge render the label to the right as currently done, otherwise render it to the left of the bar (calculate a left position using leftPercent minus an offset) or omit it for very narrow/edge cases; ensure the check and alternate placement use the same span styling (text-[10px] font-mono text-muted-foreground whitespace-nowrap) so appearance stays consistent.packages/ui/src/components/observability/ServiceList/shortcuts.ts (1)
3-4: Rename exported shortcut constant to match “Traces” terminology.
SERVICES_SHORTCUTSnow represents traces. Keeping the old symbol name increases semantic drift.♻️ Suggested rename
-export const SERVICES_SHORTCUTS: ShortcutGroup = { +export const TRACES_SHORTCUTS: ShortcutGroup = { name: "Traces", shortcuts: [{ keys: ["Backspace"], description: "Go back" }], };(Consumers should be updated in the same PR to keep naming consistent.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/ServiceList/shortcuts.ts` around lines 3 - 4, The exported constant SERVICES_SHORTCUTS should be renamed to reflect the “Traces” domain: update the symbol name SERVICES_SHORTCUTS to something like TRACES_SHORTCUTS (or TRACES_SHORTCUT_GROUP) in packages/ui/src/components/observability/ServiceList/shortcuts.ts and update all consumers/imports in this PR to use the new name; ensure the exported type ShortcutGroup and the object shape remain unchanged and run the build/tests to catch any remaining reference errors.packages/ui/src/components/KeyboardShortcuts/KeyboardShortcutsProvider.tsx (1)
73-76: RenameonNavigateServicestoonNavigateTracesfor API clarity.The shortcut is now explicitly for traces, but the callback name still says services. This is easy to misread during future changes.
♻️ Suggested rename in this file
interface KeyboardShortcutsProviderProps { children: ReactNode; - onNavigateServices: () => void; + onNavigateTraces: () => void; onNavigateLogs: () => void; onNavigateMetrics: () => void; } export function KeyboardShortcutsProvider({ children, - onNavigateServices, + onNavigateTraces, onNavigateLogs, onNavigateMetrics, }: KeyboardShortcutsProviderProps) { @@ if (e.shiftKey && e.key === "T") { e.preventDefault(); - onNavigateServices(); + onNavigateTraces(); return; } @@ - }, [isOpen, onNavigateServices, onNavigateLogs, onNavigateMetrics]); + }, [isOpen, onNavigateTraces, onNavigateLogs, onNavigateMetrics]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/KeyboardShortcuts/KeyboardShortcutsProvider.tsx` around lines 73 - 76, Rename the shortcut callback from onNavigateServices to onNavigateTraces across this component: update the prop/interface name, its usage in the KeyboardShortcutsProvider component (the handler invoked inside the keydown branch for Shift+T), and any places that pass the prop so types and calls remain consistent; ensure the function signature and all import/prop references are updated to onNavigateTraces to avoid broken references.packages/ui/src/components/observability/TraceSearch/ScatterPlot.tsx (1)
121-127: Keep the service color in the fill.Right now any error trace becomes solid red, so the chart loses its service encoding exactly where comparison matters most. Use the red stroke as the error accent and keep
getServiceColor(...)as the fill.🎨 Suggested change
<Cell key={i} - fill={ - point.hasError - ? "#ef4444" - : getServiceColor(point.serviceName) - } + fill={getServiceColor(point.serviceName)} stroke={point.hasError ? "#ef4444" : "none"} strokeWidth={point.hasError ? 2 : 0} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/TraceSearch/ScatterPlot.tsx` around lines 121 - 127, Currently error points overwrite the fill with solid red; change the logic in ScatterPlot (where fill/stroke are set) to always use getServiceColor(point.serviceName) for fill and use the red color ("#ef4444") only as the stroke when point.hasError is true (with strokeWidth 2), otherwise leave stroke absent/none and strokeWidth 0; update the expressions around fill, stroke, and strokeWidth that reference point.hasError and getServiceColor to reflect this behavior.packages/ui/src/hooks/use-kopai-data.test.ts (1)
22-24: Add direct coverage for the new datasource branches.These mocks keep the harness compiling, but there are still no assertions that
useKopaiDatadispatchesgetServices,getOperations, andsearchTraceSummariesPagecorrectly. A small trio of hook tests here would catch missing switch cases and bad argument wiring early.🤖 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 22 - 24, The test file is missing assertions that useKopaiData dispatches the new datasource actions; add tests that mount or render the hook and verify the mocked functions getServices, getOperations, and searchTraceSummariesPage are called with the expected arguments for each datasource branch. Specifically, write three small tests that invoke useKopaiData (or call the hook wrapper/helper you already use), trigger the code paths for services, operations, and trace summaries, and assert vi.fn() mocks for getServices, getOperations, and searchTraceSummariesPage were called once with the correct params and context (e.g., expected datasource id/filters), covering the switch/case branches in the hook.packages/ui/src/components/observability/TraceSearch/SortDropdown.tsx (1)
5-16: Narrow the sort prop to the supported literals.
value: stringandonChange(string)allow invalid sort keys through at compile time, though all current usages already match the fixed option set. Deriving the type fromSORT_OPTIONSwould catch invalid values earlier and keep the prop surface in sync with the implementation.🔧 Suggested typing change
-export interface SortDropdownProps { - value: string; - onChange: (sort: string) => void; -} - const SORT_OPTIONS = [ { value: "recent", label: "Most Recent" }, { value: "longest", label: "Longest First" }, { value: "shortest", label: "Shortest First" }, { value: "mostSpans", label: "Most Spans" }, { value: "leastSpans", label: "Least Spans" }, ] as const; + +export type TraceSort = (typeof SORT_OPTIONS)[number]["value"]; + +export interface SortDropdownProps { + value: TraceSort; + onChange: (sort: TraceSort) => void; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/TraceSearch/SortDropdown.tsx` around lines 5 - 16, SortDropdownProps currently accepts open strings which can allow invalid sort keys; derive a literal union from SORT_OPTIONS and use it for the prop types so the component accepts only supported keys. Create a type (e.g., SortKey = typeof SORT_OPTIONS[number]["value"]) and update SortDropdownProps to use value: SortKey and onChange: (sort: SortKey) => void so the prop surface stays in sync with SORT_OPTIONS and invalid values are caught at compile time; update any usages if needed to match the new SortKey type.packages/ui/src/components/observability/TraceSearch/SearchForm.tsx (1)
59-62: Consider resetting operation when service changes.The
useEffectsyncsservicefrominitialValues, but if operations are service-specific (fetched per service), the operation selection may become stale when the service changes.♻️ Proposed fix to reset operation on service change
// Sync service from URL-driven changes useEffect(() => { - if (initialValues?.service != null) setService(initialValues.service); + if (initialValues?.service != null) { + setService(initialValues.service); + setOperation(""); // Reset operation when service changes + } }, [initialValues?.service]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/TraceSearch/SearchForm.tsx` around lines 59 - 62, The useEffect that syncs service from initialValues can leave a stale operation selected when the service changes; inside the same effect (the one referencing initialValues?.service and calling setService) also clear the operation selection (e.g., call setOperation(null or undefined)) so operations specific to the previous service are reset; if you have operation-related state or options (operationOptions, setOperationOptions) clear those as well to ensure UI/fetches reflect the new service.packages/ui/src/components/observability/TraceTimeline/SpanSearch.tsx (1)
20-26: Consider adding keyboard navigation for better UX.Users typically expect Enter/Shift+Enter to navigate between search matches. This would improve the search experience.
♻️ Optional: Add keyboard navigation
<input type="text" placeholder="Find..." value={value} onChange={(e) => onChange(e.target.value)} + onKeyDown={(e) => { + if (e.key === "Enter" && matchCount > 0) { + e.shiftKey ? onPrev() : onNext(); + } + }} className="bg-muted text-foreground text-sm px-2 py-0.5 rounded border border-border outline-none focus:border-blue-500 w-48" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/TraceTimeline/SpanSearch.tsx` around lines 20 - 26, SpanSearch currently only updates the search value via onChange on the input; add keyboard navigation so Enter advances to the next match and Shift+Enter goes to the previous match. Update the SpanSearch component to accept two optional props (e.g., onNextMatch and onPrevMatch) and add an onKeyDown handler on the input that calls onNextMatch when e.key === "Enter" and !e.shiftKey, and calls onPrevMatch when e.key === "Enter" and e.shiftKey (prevent default to avoid form submits). Ensure the handler coexists with the existing onChange logic and references the input in SpanSearch so keyboard nav triggers match navigation in the parent.packages/ui/src/components/observability/TraceTimeline/SpanDetailInline.tsx (1)
29-39: Expose the disclosure state to assistive tech.These buttons toggle content, but without
aria-expandedscreen readers won't announce whether a section is open. Addingtype="button"here also avoids accidental form submits.Suggested fix
<button + type="button" + aria-expanded={open} className="flex items-center gap-1 text-xs font-medium text-foreground hover:text-blue-600 dark:hover:text-blue-400 py-1" onClick={(e) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/TraceTimeline/SpanDetailInline.tsx` around lines 29 - 39, The button in SpanDetailInline toggles disclosure but lacks accessibility attributes and button semantics; update the button element used in SpanDetailInline.tsx to include type="button" and aria-expanded={open} (and optionally aria-controls referencing the toggled region id if present) so assistive tech can announce the expanded/collapsed state; keep the existing onClick that calls setOpen and ensure the aria attribute references the same open state.packages/ui/src/pages/observability.tsx (1)
371-379: Limited key character set inparseLogfmtmay miss common attribute names.The regex
(\w+)=only matches keys containing[a-zA-Z0-9_]. OpenTelemetry attribute names commonly use dots (e.g.,http.method,service.name) and occasionally hyphens. These would not be parsed correctly.♻️ Suggested improvement
function parseLogfmt(str: string): Record<string, string> { const result: Record<string, string> = {}; - const re = /(\w+)=(?:"([^"]*)"|([\S]*))/g; + const re = /([\w.\-]+)=(?:"([^"]*)"|([\S]*))/g; let m: RegExpExecArray | null; while ((m = re.exec(str)) !== null) { result[m[1]!] = m[2] ?? m[3] ?? ""; } return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/pages/observability.tsx` around lines 371 - 379, The parseLogfmt function's key regex (\w+)= only matches letters/numbers/underscore and will miss common OpenTelemetry keys with dots or hyphens (e.g., http.method, service.name). Update the key-capturing pattern in parseLogfmt to allow dots and hyphens (for example use a character class like [A-Za-z0-9_.-]+ or equivalent) so keys with '.' and '-' are captured; keep the rest of the value parsing logic (the alternation for quoted vs unquoted values and assignment to result[m[1]]) unchanged.
🤖 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/clickhouse-datasource/src/datasource.ts`:
- Around line 314-320: The getServices method declares opts as optional but
immediately accesses opts.requestContext, causing a type-safety error; update
the signature to require opts (e.g. async getServices(opts: { requestContext:
unknown }): Promise<{ services: string[] }>) to match
getOperations/getTraceSummaries, or if you prefer minimal change, locally narrow
before use by doing something like const requestContext = opts!.requestContext
and then call assertClickHouseRequestContext(requestContext) and use
requestContext for getLogger and destructuring; ensure you update references of
database/username/password to use the non-optional requestContext variable and
keep assertClickHouseRequestContext(requestContext) as the validation point.
In `@packages/clickhouse-datasource/src/query-traces.ts`:
- Around line 135-136: The errorCount expression in query-traces.ts uses
countIf(StatusCode = 'STATUS_CODE_ERROR') but this datasource emits statuses
like UNSET, OK, ERROR; replace that condition so errorCount counts the actual
ERROR status (e.g., change to toUInt32(countIf(StatusCode = 'ERROR'))), or
broaden to include both forms if needed (e.g., countIf(StatusCode IN
('ERROR','STATUS_CODE_ERROR'))), ensuring the spanCount and errorCount fields
remain toUInt32 results.
In `@packages/core/src/data-filters-zod.ts`:
- Around line 281-287: The exported TraceSummariesFilter type currently uses
z.infer<typeof traceSummariesFilterSchema> which yields the parsed/output shape
(making fields with defaults like limit and sortOrder required); change the
export to use z.input<typeof traceSummariesFilterSchema> so the type reflects
the input contract callers provide (allowing omitted/defaulted fields) — update
the TraceSummariesFilter export to reference z.input and keep
traceSummariesFilterSchema unchanged.
In `@packages/sqlite-datasource/src/db-datasource.ts`:
- Around line 1039-1057: The JSON path for span/resource attribute filtering is
built by interpolating the attribute key into a SQL string literal (variables
jsonPath, spanClauses, spanFilterParams) which allows single-quote injection;
fix by using the same safe embedding approach as getTraces (use
kyselySql.lit(jsonPath) or an equivalent parameterized/escaped API) when
constructing the json_extract path instead of raw string interpolation, or
ensure you escape single quotes in the key before building jsonPath and then
pass the path via kyselySql.lit to the query so keys like "foo'bar" cannot break
the SQL.
In `@packages/sqlite-datasource/src/optimized-datasource.ts`:
- Around line 116-124: The OptimizedDatasource methods getServices and
getOperations must accept the optional requestContext parameter required by the
ReadTracesMetaDatasource interface and forward it to the underlying dbDatasource
so the context isn't dropped; update the getServices signature to include
(requestContext?) and call return this.dbDatasource.getServices(requestContext),
and update getOperations to include (filter, requestContext?) and call return
this.dbDatasource.getOperations(filter, requestContext), keeping return types
the same and using the same parameter names (getServices, getOperations,
dbDatasource) so the optimized implementation fully conforms to the interface.
In `@packages/ui/src/components/observability/TraceComparison/index.tsx`:
- Around line 115-135: The comparison UI is computing deltas using the
empty-array fallback (rowsA ?? [] / rowsB ?? []), which yields false
added/removed results when one fetch fails; update the logic that computes stats
(computeTraceStats) and diff (computeDiff) and the derived values durationDelta
and spanDelta to only run when both requests succeeded (i.e., !loadingA &&
!loadingB && !errorA && !errorB and rowsA && rowsB), and gate rendering of the
comparison header/structural diff on that same condition (otherwise show the
error/loading state instead of a zero-span comparison).
- Around line 45-46: Do not build composite keys by concatenating
row.ServiceName and row.SpanName with "::" because SpanName may contain "::";
instead use a safe structured key (e.g., JSON.stringify({service:
row.ServiceName ?? 'unknown', span: row.SpanName ?? ''}) or a
nested/map-of-maps) wherever the code currently creates `key =
\`${row.ServiceName ?? "unknown"}::${row.SpanName ?? ""}\`` (and the similar
constructions at the other noted sites). Update any code that parses such keys
(places that call split("::")) to consume the structured key format (parse the
JSON or use the nested map lookup) so distinct service/span pairs cannot be
merged. Ensure changes are applied to the `key` creation and any lookups/usages
at the other occurrences (lines referenced 68-70 and 265-267).
In `@packages/ui/src/components/observability/TraceSearch/DurationBar.tsx`:
- Around line 18-29: The current width calculation uses Math.max(widthPct, 1)
which makes zero-duration rows appear non-zero and allows ratios >100 to
overflow; update the logic in DurationBar where widthPct is used (derived from
durationMs and maxDurationMs) to first compute widthPct = maxDurationMs > 0 ?
(durationMs / maxDurationMs) * 100 : 0, then compute a clampedWidthPct such that
if durationMs <= 0 -> 0, otherwise clamp widthPct into the [1, 100] range (e.g.,
Math.min(Math.max(widthPct, 1), 100)) and use clampedWidthPct for the style
width. Ensure references to durationMs, maxDurationMs, and widthPct are used to
locate and replace the width expression.
In `@packages/ui/src/components/observability/TraceTimeline/FlamegraphView.tsx`:
- Around line 159-176: The <g> elements for each span are currently only
pointer-interactive; make them keyboard-accessible by adding tabIndex={0} and
appropriate keyboard handlers: add onKeyDown that calls handleClick(span) when
Enter or Space is pressed, add onFocus to setTooltip with a fallback position
and onBlur to clear it, and include an accessible name via aria-label (e.g.,
derived from span.operationName or span.spanId) so screen readers can identify
the node; update the elements referenced (key={span.spanId}, handleClick,
setTooltip) accordingly so keyboard users can select/zoom spans.
In `@packages/ui/src/components/observability/TraceTimeline/GraphView.tsx`:
- Around line 41-76: The layout BFS needs protection from service-graph cycles
(A→B→A) to avoid infinite re-enqueueing; modify the BFS that walks childServices
/ computes layers so it tracks seen services (or a seenLayer map that records
the lowest layer assigned) and only enqueues a child when it hasn't been seen
yet or when the new layer is strictly smaller than the recorded layer, ensuring
you never re-enqueue a node with an equal-or-greater layer; apply the same
visited/seen-layer guard where the second BFS-like pass occurs (around the code
referenced at lines ~120-130) so cycles don’t cause non-termination.
In `@packages/ui/src/components/observability/TraceTimeline/Minimap.tsx`:
- Around line 66-121: The current handleMouseDown installs window listeners
(handleMouseMove, handleMouseUp) but only removes them in handleMouseUp, causing
a leak if the component unmounts mid-drag; update the component to ensure those
listeners are removed on unmount by storing the handler functions in refs (or a
listenersRef) and adding a useEffect cleanup that removes window event listeners
(the same references added) and clears dragRef; specifically locate
handleMouseDown, handleMouseMove, handleMouseUp, dragRef and containerRef and
ensure the listeners added inside handleMouseDown are the exact function
references removed in the useEffect cleanup so no stray listeners remain after
unmount.
In `@packages/ui/src/components/observability/TraceTimeline/TimelineBar.tsx`:
- Around line 39-43: The durationLabel text always uses white which can fail
contrast for some HSL(hue,70%,50%) backgrounds; update TimelineBar to pick text
color dynamically by computing background luminance from the bar's HSL (use the
same hue used to render the bar) and then choose a high-contrast text color
(e.g., black vs white) or a WCAG-compliant shade; add a small helper like
isLightBackground(hue, sat=70, light=50) or getContrastTextForHSL(hue) and apply
its result to the span that renders {durationLabel} (referencing isWide,
durationLabel and the TimelineBar rendering path) so the text color adapts to
ensure accessible contrast.
In `@packages/ui/src/lib/component-catalog.ts`:
- Around line 32-36: The params schema for the getServices action currently uses
z.object({}).optional() which silently strips unknown keys; update the params
definition for the getServices case (the z.object under method:
z.literal("getServices")) to use z.object({}).strict().optional() so unexpected
keys are rejected while keeping the whole params object optional.
In `@vitest.config.ts`:
- Around line 6-10: The current vitest.config.ts replaces Vitest's default
excludes by setting exclude explicitly; instead merge the default excludes with
your custom patterns so defaults like "**/.git/**" are preserved—update the
exclude array in vitest.config.ts to spread Vitest's defaults (e.g., use
configDefaults.exclude or defaultExclude from Vitest) and then append your
custom patterns ("**/node_modules/**",
"**/packages/otel-testing-harness/examples/jest/**",
"**/packages/otel-testing-harness/examples/tap/**") so the final exclude is
[...defaultExcludes, /* your patterns */].
---
Nitpick comments:
In `@packages/ui/src/components/KeyboardShortcuts/KeyboardShortcutsProvider.tsx`:
- Around line 73-76: Rename the shortcut callback from onNavigateServices to
onNavigateTraces across this component: update the prop/interface name, its
usage in the KeyboardShortcutsProvider component (the handler invoked inside the
keydown branch for Shift+T), and any places that pass the prop so types and
calls remain consistent; ensure the function signature and all import/prop
references are updated to onNavigateTraces to avoid broken references.
In `@packages/ui/src/components/observability/LogTimeline/LogFilter.tsx`:
- Around line 278-307: The useEffect in LogFilter.tsx (function using
isFirstRender.current) intentionally omits value and onChange from its
dependency array which triggers react-hooks/exhaustive-deps; add an ESLint
suppression comment above the useEffect (e.g., // eslint-disable-next-line
react-hooks/exhaustive-deps) and include a short inline justification
referencing why value and onChange are excluded (to prevent infinite loops) so
reviewers and linters understand the intentional omission while leaving the
effect dependencies as [dBodyContains, dTraceId, dSpanId, dLogAttrs, dResAttrs,
dScopeAttrs].
In `@packages/ui/src/components/observability/ServiceList/shortcuts.ts`:
- Around line 3-4: The exported constant SERVICES_SHORTCUTS should be renamed to
reflect the “Traces” domain: update the symbol name SERVICES_SHORTCUTS to
something like TRACES_SHORTCUTS (or TRACES_SHORTCUT_GROUP) in
packages/ui/src/components/observability/ServiceList/shortcuts.ts and update all
consumers/imports in this PR to use the new name; ensure the exported type
ShortcutGroup and the object shape remain unchanged and run the build/tests to
catch any remaining reference errors.
In `@packages/ui/src/components/observability/TraceSearch/ScatterPlot.tsx`:
- Around line 121-127: Currently error points overwrite the fill with solid red;
change the logic in ScatterPlot (where fill/stroke are set) to always use
getServiceColor(point.serviceName) for fill and use the red color ("#ef4444")
only as the stroke when point.hasError is true (with strokeWidth 2), otherwise
leave stroke absent/none and strokeWidth 0; update the expressions around fill,
stroke, and strokeWidth that reference point.hasError and getServiceColor to
reflect this behavior.
In `@packages/ui/src/components/observability/TraceSearch/SearchForm.tsx`:
- Around line 59-62: The useEffect that syncs service from initialValues can
leave a stale operation selected when the service changes; inside the same
effect (the one referencing initialValues?.service and calling setService) also
clear the operation selection (e.g., call setOperation(null or undefined)) so
operations specific to the previous service are reset; if you have
operation-related state or options (operationOptions, setOperationOptions) clear
those as well to ensure UI/fetches reflect the new service.
In `@packages/ui/src/components/observability/TraceSearch/SortDropdown.tsx`:
- Around line 5-16: SortDropdownProps currently accepts open strings which can
allow invalid sort keys; derive a literal union from SORT_OPTIONS and use it for
the prop types so the component accepts only supported keys. Create a type
(e.g., SortKey = typeof SORT_OPTIONS[number]["value"]) and update
SortDropdownProps to use value: SortKey and onChange: (sort: SortKey) => void so
the prop surface stays in sync with SORT_OPTIONS and invalid values are caught
at compile time; update any usages if needed to match the new SortKey type.
In `@packages/ui/src/components/observability/TraceTimeline/SpanDetailInline.tsx`:
- Around line 29-39: The button in SpanDetailInline toggles disclosure but lacks
accessibility attributes and button semantics; update the button element used in
SpanDetailInline.tsx to include type="button" and aria-expanded={open} (and
optionally aria-controls referencing the toggled region id if present) so
assistive tech can announce the expanded/collapsed state; keep the existing
onClick that calls setOpen and ensure the aria attribute references the same
open state.
In `@packages/ui/src/components/observability/TraceTimeline/SpanSearch.tsx`:
- Around line 20-26: SpanSearch currently only updates the search value via
onChange on the input; add keyboard navigation so Enter advances to the next
match and Shift+Enter goes to the previous match. Update the SpanSearch
component to accept two optional props (e.g., onNextMatch and onPrevMatch) and
add an onKeyDown handler on the input that calls onNextMatch when e.key ===
"Enter" and !e.shiftKey, and calls onPrevMatch when e.key === "Enter" and
e.shiftKey (prevent default to avoid form submits). Ensure the handler coexists
with the existing onChange logic and references the input in SpanSearch so
keyboard nav triggers match navigation in the parent.
In `@packages/ui/src/components/observability/TraceTimeline/TimelineBar.tsx`:
- Around line 45-52: The duration label positioned to the right of the bar can
overflow when leftPercent + widthPercent is near 100%; update the rendering
logic in TimelineBar (the span that uses leftPercent, widthPercent,
durationLabel and isWide) to detect edge cases and avoid overflow by
conditionally switching placement: if leftPercent + widthPercent is sufficiently
below the right edge render the label to the right as currently done, otherwise
render it to the left of the bar (calculate a left position using leftPercent
minus an offset) or omit it for very narrow/edge cases; ensure the check and
alternate placement use the same span styling (text-[10px] font-mono
text-muted-foreground whitespace-nowrap) so appearance stays consistent.
In `@packages/ui/src/hooks/use-kopai-data.test.ts`:
- Around line 22-24: The test file is missing assertions that useKopaiData
dispatches the new datasource actions; add tests that mount or render the hook
and verify the mocked functions getServices, getOperations, and
searchTraceSummariesPage are called with the expected arguments for each
datasource branch. Specifically, write three small tests that invoke
useKopaiData (or call the hook wrapper/helper you already use), trigger the code
paths for services, operations, and trace summaries, and assert vi.fn() mocks
for getServices, getOperations, and searchTraceSummariesPage were called once
with the correct params and context (e.g., expected datasource id/filters),
covering the switch/case branches in the hook.
In `@packages/ui/src/pages/observability.tsx`:
- Around line 371-379: The parseLogfmt function's key regex (\w+)= only matches
letters/numbers/underscore and will miss common OpenTelemetry keys with dots or
hyphens (e.g., http.method, service.name). Update the key-capturing pattern in
parseLogfmt to allow dots and hyphens (for example use a character class like
[A-Za-z0-9_.-]+ or equivalent) so keys with '.' and '-' are captured; keep the
rest of the value parsing logic (the alternation for quoted vs unquoted values
and assignment to result[m[1]]) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91069853-3f9c-4bd7-b3a4-d1b9f4cffd3f
⛔ Files ignored due to path filters (2)
packages/ui/src/lib/__snapshots__/generate-prompt-instructions.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (55)
packages/api/src/index.test.tspackages/api/src/routes/traces.tspackages/api/src/signals.test.tspackages/clickhouse-datasource/src/datasource.test.tspackages/clickhouse-datasource/src/datasource.tspackages/clickhouse-datasource/src/e2e.integration.test.tspackages/clickhouse-datasource/src/query-traces.tspackages/core/src/data-filters-zod.tspackages/core/src/telemetry-datasource.tspackages/sdk/src/client.tspackages/sdk/src/index.tspackages/sdk/src/pagination.test.tspackages/sdk/src/types.tspackages/sqlite-datasource/src/datasource-read.test.tspackages/sqlite-datasource/src/db-datasource.tspackages/sqlite-datasource/src/optimized-datasource.tspackages/ui/src/components/KeyboardShortcuts/KeyboardShortcutsProvider.tsxpackages/ui/src/components/observability/DynamicDashboard/DynamicDashboard.test.tsxpackages/ui/src/components/observability/LogTimeline/LogFilter.tsxpackages/ui/src/components/observability/LogTimeline/index.tsxpackages/ui/src/components/observability/MetricHistogram/index.tsxpackages/ui/src/components/observability/ServiceList/shortcuts.tspackages/ui/src/components/observability/TraceComparison/index.tsxpackages/ui/src/components/observability/TraceDetail/TraceDetail.stories.tsxpackages/ui/src/components/observability/TraceDetail/index.tsxpackages/ui/src/components/observability/TraceSearch/DurationBar.tsxpackages/ui/src/components/observability/TraceSearch/ScatterPlot.tsxpackages/ui/src/components/observability/TraceSearch/SearchForm.tsxpackages/ui/src/components/observability/TraceSearch/SortDropdown.tsxpackages/ui/src/components/observability/TraceSearch/index.tsxpackages/ui/src/components/observability/TraceTimeline/DetailPane/EventsTab.tsxpackages/ui/src/components/observability/TraceTimeline/FlamegraphView.tsxpackages/ui/src/components/observability/TraceTimeline/GraphView.tsxpackages/ui/src/components/observability/TraceTimeline/Minimap.tsxpackages/ui/src/components/observability/TraceTimeline/SpanDetailInline.tsxpackages/ui/src/components/observability/TraceTimeline/SpanRow.tsxpackages/ui/src/components/observability/TraceTimeline/SpanSearch.tsxpackages/ui/src/components/observability/TraceTimeline/StatisticsView.tsxpackages/ui/src/components/observability/TraceTimeline/TimeRuler.tsxpackages/ui/src/components/observability/TraceTimeline/TimelineBar.tsxpackages/ui/src/components/observability/TraceTimeline/TraceHeader.tsxpackages/ui/src/components/observability/TraceTimeline/ViewTabs.tsxpackages/ui/src/components/observability/TraceTimeline/index.tsxpackages/ui/src/components/observability/index.tspackages/ui/src/components/observability/renderers/OtelTraceDetail.tsxpackages/ui/src/components/observability/utils/flatten-tree.tspackages/ui/src/components/observability/utils/time.tspackages/ui/src/hooks/use-kopai-data.test.tspackages/ui/src/hooks/use-kopai-data.tspackages/ui/src/hooks/use-live-logs.test.tspackages/ui/src/lib/component-catalog.tspackages/ui/src/pages/observability.test.tsxpackages/ui/src/pages/observability.tsxpackages/ui/src/providers/kopai-provider.tsxvitest.config.ts
💤 Files with no reviewable changes (2)
- packages/sdk/src/pagination.test.ts
- packages/ui/src/components/observability/TraceDetail/TraceDetail.stories.tsx
Comment on lines
+41
to
+76
| function walk(span: SpanNode, parentService?: string) { | ||
| const svc = span.serviceName; | ||
|
|
||
| const existing = nodeMap.get(svc); | ||
| if (existing) { | ||
| existing.spanCount++; | ||
| if (span.status === "ERROR") existing.errorCount++; | ||
| } else { | ||
| nodeMap.set(svc, { | ||
| spanCount: 1, | ||
| errorCount: span.status === "ERROR" ? 1 : 0, | ||
| }); | ||
| } | ||
|
|
||
| if (parentService && parentService !== svc) { | ||
| const key = `${parentService}→${svc}`; | ||
| const edge = edgeMap.get(key); | ||
| if (edge) { | ||
| edge.callCount++; | ||
| edge.totalDurationMs += span.durationMs; | ||
| } else { | ||
| edgeMap.set(key, { callCount: 1, totalDurationMs: span.durationMs }); | ||
| } | ||
| if (!childServices.has(parentService)) | ||
| childServices.set(parentService, new Set()); | ||
| childServices.get(parentService)!.add(svc); | ||
| } | ||
|
|
||
| for (const child of span.children) { | ||
| walk(child, svc); | ||
| } | ||
| } | ||
|
|
||
| for (const root of trace.rootSpans) { | ||
| walk(root); | ||
| } |
There was a problem hiding this comment.
Guard the layout BFS against service-graph cycles.
The span tree is acyclic, but collapsing it by serviceName can still produce A → B → A. With that shape, the curLayer + 1 > prev branch keeps re-enqueueing the same nodes forever and the graph view never finishes rendering.
Minimal safe fix
- if (prev === undefined || curLayer + 1 > prev) {
+ if (prev === undefined) {
layerOf.set(kid, curLayer + 1);
queue.push(kid);
}Also applies to: 120-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/observability/TraceTimeline/GraphView.tsx` around
lines 41 - 76, The layout BFS needs protection from service-graph cycles (A→B→A)
to avoid infinite re-enqueueing; modify the BFS that walks childServices /
computes layers so it tracks seen services (or a seenLayer map that records the
lowest layer assigned) and only enqueues a child when it hasn't been seen yet or
when the new layer is strictly smaller than the recorded layer, ensuring you
never re-enqueue a node with an equal-or-greater layer; apply the same
visited/seen-layer guard where the second BFS-like pass occurs (around the code
referenced at lines ~120-130) so cycles don’t cause non-termination.
Comment on lines
+39
to
+43
| {isWide && ( | ||
| <span className="text-[10px] font-mono text-white px-1 truncate"> | ||
| {durationLabel} | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what colors getSpanBarColor can return
ast-grep --pattern $'function getSpanBarColor($_) {
$$$
}'
# Also search for the color palette/values used
rg -n -C5 'getSpanBarColor|barColor|serviceColor' --type=ts packages/ui/src/components/observability/Repository: kopai-app/kopai-mono
Length of output: 12271
🏁 Script executed:
cat -n packages/ui/src/components/observability/utils/colors.tsRepository: kopai-app/kopai-mono
Length of output: 1025
Update text color based on bar background luminance to ensure accessible contrast.
The bar colors are generated using HSL(hue, 70%, 50%) where hue varies by service name. This means certain hues—particularly yellows and greens—will have insufficient contrast with white text (10px), failing WCAG AA standards.
Either use a text color that adapts to background luminance, or ensure all generated colors meet minimum contrast thresholds with white text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/observability/TraceTimeline/TimelineBar.tsx`
around lines 39 - 43, The durationLabel text always uses white which can fail
contrast for some HSL(hue,70%,50%) backgrounds; update TimelineBar to pick text
color dynamically by computing background luminance from the bar's HSL (use the
same hue used to render the bar) and then choose a high-contrast text color
(e.g., black vs white) or a WCAG-compliant shade; add a small helper like
isLightBackground(hue, sat=70, light=50) or getContrastTextForHSL(hue) and apply
its result to the span that renders {durationLabel} (referencing isWide,
durationLabel and the TimelineBar rendering path) so the text color adapts to
ensure accessible contrast.
Comment on lines
+32
to
+36
| z.object({ | ||
| method: z.literal("getServices"), | ||
| params: z.object({}).optional(), | ||
| refetchIntervalMs: z.number().optional(), | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Zod, what is the default unknown-key behavior of z.object({}), and how does .strict() change it?
💡 Result:
- Default behavior of
z.object({}): Zod object schemas strip (remove) unrecognized keys during parsing, soz.object({}).parse({ a: 1 })returns{}. [1][2] - With
.strict(): the schema forbids unknown keys and throws a validation error (e.g.unrecognized_keys) if any extra keys are present, soz.object({}).strict().parse({ a: 1 })fails. [1]
Sources: [1] Zod Objects guide, [2] Zod GitHub discussion quoting the docs.
🏁 Script executed:
# First, let's look at the actual file to see the context
cat -n packages/ui/src/lib/component-catalog.ts | head -60Repository: kopai-app/kopai-mono
Length of output: 2296
Use .strict() on getServices.params to reject unexpected keys.
z.object({}).optional() silently strips unrecognized keys (e.g., { serviceName: "foo" } passes validation as {}), hiding typos in config. Add .strict() to enforce that only defined fields are accepted:
z.object({
method: z.literal("getServices"),
- params: z.object({}).optional(),
+ params: z.object({}).strict().optional(),
refetchIntervalMs: z.number().optional(),
}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/lib/component-catalog.ts` around lines 32 - 36, The params
schema for the getServices action currently uses z.object({}).optional() which
silently strips unknown keys; update the params definition for the getServices
case (the z.object under method: z.literal("getServices")) to use
z.object({}).strict().optional() so unexpected keys are rejected while keeping
the whole params object optional.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
packages/ui/src/components/observability/TraceTimeline/GraphView.tsx (1)
113-135:⚠️ Potential issue | 🟠 MajorStop deepening layers on service cycles.
The enqueue cap avoids a hard loop, but
A → B → Astill keeps reassigning larger layer numbers until the cap is hit, so cyclic service graphs render with arbitrary depth. Once a service already has a layer, later back-edges should be ignored here (or SCCs should be collapsed before layout).Minimal safe fix
- const enqueueCount = new Map<string, number>(); - const maxEnqueue = nodeMap.size * 2; const queue: string[] = []; for (const r of roots) { layerOf.set(r, 0); queue.push(r); } @@ const curLayer = layerOf.get(cur)!; const kids = children.get(cur); if (!kids) continue; for (const kid of kids) { const prev = layerOf.get(kid); - const count = enqueueCount.get(kid) ?? 0; - if ((prev === undefined || curLayer + 1 > prev) && count < maxEnqueue) { + if (prev === undefined) { layerOf.set(kid, curLayer + 1); - enqueueCount.set(kid, count + 1); queue.push(kid); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/TraceTimeline/GraphView.tsx` around lines 113 - 135, The BFS currently reassigns larger layer numbers on back-edges causing cycles to deepen; fix the logic in the loop that processes children (references: layerOf, enqueueCount, maxEnqueue, queue, children) to ignore attempts to increase an already-set layer — only assign layerOf.set(kid, curLayer + 1) and enqueue the kid when layerOf.has(kid) is false (i.e., prev === undefined); keep the enqueueCount/maxEnqueue guard for unassigned nodes to prevent infinite loops and do not update layer or enqueue if prev is defined so cycles stop deepening.packages/clickhouse-datasource/src/query-traces.ts (1)
44-90:⚠️ Potential issue | 🟠 MajorBuild summaries from full traces, not filtered span subsets.
These predicates run before
GROUP BY TraceId, so a trace likeapi → dbfiltered withserviceName = 'db'is summarized as if it were rooted atdb. The same problem applies tospanNameand attribute filters, anddurationMin/durationMaxare currently being evaluated against individual span durations instead of the aggregated trace duration. Select candidateTraceIds with span-level filters first, then aggregate all spans for those traces and apply trace-level filters afterward.Also applies to: 127-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/clickhouse-datasource/src/query-traces.ts` around lines 44 - 90, The current WHERE predicates (e.g., checks on filter.serviceName, filter.spanName, filter.spanAttributes, filter.resourceAttributes and the durationMin/durationMax params) are applied before GROUP BY TraceId and thus filter individual spans instead of selecting full traces; instead, first run a span-level subquery that selects candidate TraceId values using span-level filters (serviceName, spanName, spanAttributes, resourceAttributes) and then use those TraceIds to fetch and aggregate all spans for each trace, applying trace-level filters (aggregate durationMin/durationMax and any trace-rooted predicates) after grouping; update the construction around the existing params and conditions logic in query-traces.ts so span-level param names (e.g., spanAttrKey{i}, spanAttrVal{i}, serviceName, spanName) are only used in the initial TraceId selection subquery and the GROUP BY TraceId + aggregation step operates over all spans for those TraceIds before enforcing durationMin/durationMax on the aggregated Duration per TraceId.
🧹 Nitpick comments (1)
packages/clickhouse-datasource/src/datasource.test.ts (1)
1300-1312: Exercise a multi-service trace in this filter test.All current
order-servicefixtures are single-service traces, so this still passes if the implementation strips non-matching spans before building the summary. A cross-service fixture here would let you assert that the filtered result keeps the original root/span-count/services payload intact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/clickhouse-datasource/src/datasource.test.ts` around lines 1300 - 1312, Update the "filters by serviceName" test for ds.getTraceSummaries to include a multi-service trace fixture (one trace containing spans from "order-service" and at least one other service) and assert that the returned summaries for traces containing "order-service" preserve the original trace payload (do not strip non-matching spans): after calling ds.getTraceSummaries({ serviceName: "order-service", ... }), assert that at least one returned summary corresponds to the multi-service fixture and verify its root/span count and services array are unchanged (e.g., check root.id or trace id, spanCount matches original, and services includes both "order-service" and the other service) rather than only checking that some service name exists.
🤖 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`:
- Around line 981-1010: The durationMin/durationMax filters are being applied to
span rows (s.Duration) instead of the trace-level window; update the query
building so duration filters use the trace's overall duration (computed from
t.End - t.Start or by comparing t.Start/t.End) against filter.durationMin and
filter.durationMax and push those conditions into the trace-level clauses/params
(the same place where otel_traces_trace_id_ts is queried) instead of into the
span subquery; specifically replace uses of s.Duration in the span subquery with
t.Start/t.End or a trace-duration expression and add the parameters to
traceIdClauses/traceIdParams (not the span params) so trace filtering works as
intended.
- Around line 1110-1141: The SQL aggregates (aggSql and svcSql) only treat
StatusCode = 'STATUS_CODE_ERROR' as an error, but traces are serialized using
statusCodeToString(), so errorCount and hasError are wrong; update the CASE WHEN
checks in aggSql (errorCount) and svcSql (hasError) to detect the same string(s)
produced by statusCodeToString() (or alternatively check the numeric enum values
if StatusCode is stored numerically) so the aggregate logic matches how
StatusCode is written; locate statusCodeToString and make the SQL match its
output (or add an OR to the CASE to accept both legacy 'STATUS_CODE_ERROR' and
the current serialized value).
In `@packages/ui/src/components/observability/TraceTimeline/Minimap.tsx`:
- Around line 53-58: The getFraction callback can produce NaN when the
container's rect.width is 0; update getFraction to guard against zero width by
checking rect.width (or using a truthy width) and returning 0 (or a safe
fraction) before dividing, and apply the same guard wherever you compute
(clientX - rect.left) / rect.width so onViewChange never receives NaN; locate
the getFraction function and the other calculation around the onViewChange call
and add a rect.width === 0 (or !rect.width) early return or fallback value.
- Around line 133-141: The click handler handleBackgroundClick currently
recenters on any click unless dragging, causing clicks on the viewport/handles
to also recenter; modify handleBackgroundClick to verify the event originated on
the background container by checking e.currentTarget === e.target (or otherwise
ensure the target is the container) before computing
frac/clampView/onViewChange; apply the same guard to the other similar click
handler used for the container so only true background clicks trigger
recentering while clicks on children/handles are ignored.
- Around line 163-168: The element with role="slider" lacks keyboard support;
make it operable by keyboard or change the role. Add tabIndex={0} to the slider
element and implement an onKeyDown handler that listens for ArrowLeft/ArrowRight
(and ArrowUp/ArrowDown optionally) and adjusts the viewport by calling the same
logic that updates viewStartPct / moves the minimap viewport (use the existing
state updater or pan/scroll handler used elsewhere in Minimap.tsx), call
preventDefault for handled keys, and ensure aria-valuenow reflects the updated
Math.round(viewStartPct); alternatively, if you do not want slider semantics,
remove role="slider" and use a non-interactive role.
---
Duplicate comments:
In `@packages/clickhouse-datasource/src/query-traces.ts`:
- Around line 44-90: The current WHERE predicates (e.g., checks on
filter.serviceName, filter.spanName, filter.spanAttributes,
filter.resourceAttributes and the durationMin/durationMax params) are applied
before GROUP BY TraceId and thus filter individual spans instead of selecting
full traces; instead, first run a span-level subquery that selects candidate
TraceId values using span-level filters (serviceName, spanName, spanAttributes,
resourceAttributes) and then use those TraceIds to fetch and aggregate all spans
for each trace, applying trace-level filters (aggregate durationMin/durationMax
and any trace-rooted predicates) after grouping; update the construction around
the existing params and conditions logic in query-traces.ts so span-level param
names (e.g., spanAttrKey{i}, spanAttrVal{i}, serviceName, spanName) are only
used in the initial TraceId selection subquery and the GROUP BY TraceId +
aggregation step operates over all spans for those TraceIds before enforcing
durationMin/durationMax on the aggregated Duration per TraceId.
In `@packages/ui/src/components/observability/TraceTimeline/GraphView.tsx`:
- Around line 113-135: The BFS currently reassigns larger layer numbers on
back-edges causing cycles to deepen; fix the logic in the loop that processes
children (references: layerOf, enqueueCount, maxEnqueue, queue, children) to
ignore attempts to increase an already-set layer — only assign layerOf.set(kid,
curLayer + 1) and enqueue the kid when layerOf.has(kid) is false (i.e., prev ===
undefined); keep the enqueueCount/maxEnqueue guard for unassigned nodes to
prevent infinite loops and do not update layer or enqueue if prev is defined so
cycles stop deepening.
---
Nitpick comments:
In `@packages/clickhouse-datasource/src/datasource.test.ts`:
- Around line 1300-1312: Update the "filters by serviceName" test for
ds.getTraceSummaries to include a multi-service trace fixture (one trace
containing spans from "order-service" and at least one other service) and assert
that the returned summaries for traces containing "order-service" preserve the
original trace payload (do not strip non-matching spans): after calling
ds.getTraceSummaries({ serviceName: "order-service", ... }), assert that at
least one returned summary corresponds to the multi-service fixture and verify
its root/span count and services array are unchanged (e.g., check root.id or
trace id, spanCount matches original, and services includes both "order-service"
and the other service) rather than only checking that some service name exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3241187-d613-4167-8907-74d21e16035b
📒 Files selected for processing (11)
.changeset/free-stamps-lie.mdpackages/api/src/routes/dashboards.tspackages/clickhouse-datasource/src/datasource.test.tspackages/clickhouse-datasource/src/datasource.tspackages/clickhouse-datasource/src/query-traces.tspackages/core/src/data-filters-zod.tspackages/sqlite-datasource/src/db-datasource.tspackages/ui/src/components/observability/TraceSearch/DurationBar.tsxpackages/ui/src/components/observability/TraceTimeline/GraphView.tsxpackages/ui/src/components/observability/TraceTimeline/Minimap.tsxvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/components/observability/TraceSearch/DurationBar.tsx
Comment on lines
+1110
to
+1141
| const aggSql = ` | ||
| SELECT | ||
| TraceId, | ||
| COALESCE(MIN(CASE WHEN ParentSpanId = '' THEN ServiceName END), MIN(ServiceName)) as rootServiceName, | ||
| COALESCE(MIN(CASE WHEN ParentSpanId = '' THEN SpanName END), MIN(SpanName)) as rootSpanName, | ||
| CAST(MIN(Timestamp) AS TEXT) as startTimeNs, | ||
| CAST(MAX(Timestamp + Duration) - MIN(Timestamp) AS TEXT) as durationNs, | ||
| COUNT(*) as spanCount, | ||
| SUM(CASE WHEN StatusCode = 'STATUS_CODE_ERROR' THEN 1 ELSE 0 END) as errorCount | ||
| FROM otel_traces | ||
| WHERE TraceId IN (${placeholders}) | ||
| GROUP BY TraceId | ||
| `; | ||
| const aggRows = this.sqliteConnection | ||
| .prepare(aggSql) | ||
| .all(...traceIds) as { | ||
| TraceId: string; | ||
| rootServiceName: string | null; | ||
| rootSpanName: string | null; | ||
| startTimeNs: string; | ||
| durationNs: string; | ||
| spanCount: number; | ||
| errorCount: number; | ||
| }[]; | ||
|
|
||
| // Step 3: Per-service breakdown (small result: ~traces × avg services) | ||
| const svcSql = ` | ||
| SELECT TraceId, ServiceName, COUNT(*) as cnt, | ||
| MAX(CASE WHEN StatusCode = 'STATUS_CODE_ERROR' THEN 1 ELSE 0 END) as hasError | ||
| FROM otel_traces | ||
| WHERE TraceId IN (${placeholders}) | ||
| GROUP BY TraceId, ServiceName |
There was a problem hiding this comment.
Count the status string this datasource actually writes.
This conflicts with statusCodeToString() on Lines 1285-1288: locally ingested traces are serialized with the enum string, but these aggregates only treat STATUS_CODE_ERROR as an error. That makes errorCount and per-service hasError stay zero for ordinary errored traces.
Suggested fix
- SUM(CASE WHEN StatusCode = 'STATUS_CODE_ERROR' THEN 1 ELSE 0 END) as errorCount
+ SUM(CASE WHEN StatusCode IN ('ERROR', 'STATUS_CODE_ERROR') THEN 1 ELSE 0 END) as errorCount
@@
- MAX(CASE WHEN StatusCode = 'STATUS_CODE_ERROR' THEN 1 ELSE 0 END) as hasError
+ MAX(CASE WHEN StatusCode IN ('ERROR', 'STATUS_CODE_ERROR') THEN 1 ELSE 0 END) as hasError📝 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 aggSql = ` | |
| SELECT | |
| TraceId, | |
| COALESCE(MIN(CASE WHEN ParentSpanId = '' THEN ServiceName END), MIN(ServiceName)) as rootServiceName, | |
| COALESCE(MIN(CASE WHEN ParentSpanId = '' THEN SpanName END), MIN(SpanName)) as rootSpanName, | |
| CAST(MIN(Timestamp) AS TEXT) as startTimeNs, | |
| CAST(MAX(Timestamp + Duration) - MIN(Timestamp) AS TEXT) as durationNs, | |
| COUNT(*) as spanCount, | |
| SUM(CASE WHEN StatusCode = 'STATUS_CODE_ERROR' THEN 1 ELSE 0 END) as errorCount | |
| FROM otel_traces | |
| WHERE TraceId IN (${placeholders}) | |
| GROUP BY TraceId | |
| `; | |
| const aggRows = this.sqliteConnection | |
| .prepare(aggSql) | |
| .all(...traceIds) as { | |
| TraceId: string; | |
| rootServiceName: string | null; | |
| rootSpanName: string | null; | |
| startTimeNs: string; | |
| durationNs: string; | |
| spanCount: number; | |
| errorCount: number; | |
| }[]; | |
| // Step 3: Per-service breakdown (small result: ~traces × avg services) | |
| const svcSql = ` | |
| SELECT TraceId, ServiceName, COUNT(*) as cnt, | |
| MAX(CASE WHEN StatusCode = 'STATUS_CODE_ERROR' THEN 1 ELSE 0 END) as hasError | |
| FROM otel_traces | |
| WHERE TraceId IN (${placeholders}) | |
| GROUP BY TraceId, ServiceName | |
| const aggSql = ` | |
| SELECT | |
| TraceId, | |
| COALESCE(MIN(CASE WHEN ParentSpanId = '' THEN ServiceName END), MIN(ServiceName)) as rootServiceName, | |
| COALESCE(MIN(CASE WHEN ParentSpanId = '' THEN SpanName END), MIN(SpanName)) as rootSpanName, | |
| CAST(MIN(Timestamp) AS TEXT) as startTimeNs, | |
| CAST(MAX(Timestamp + Duration) - MIN(Timestamp) AS TEXT) as durationNs, | |
| COUNT(*) as spanCount, | |
| SUM(CASE WHEN StatusCode IN ('ERROR', 'STATUS_CODE_ERROR') THEN 1 ELSE 0 END) as errorCount | |
| FROM otel_traces | |
| WHERE TraceId IN (${placeholders}) | |
| GROUP BY TraceId | |
| `; | |
| const aggRows = this.sqliteConnection | |
| .prepare(aggSql) | |
| .all(...traceIds) as { | |
| TraceId: string; | |
| rootServiceName: string | null; | |
| rootSpanName: string | null; | |
| startTimeNs: string; | |
| durationNs: string; | |
| spanCount: number; | |
| errorCount: number; | |
| }[]; | |
| // Step 3: Per-service breakdown (small result: ~traces × avg services) | |
| const svcSql = ` | |
| SELECT TraceId, ServiceName, COUNT(*) as cnt, | |
| MAX(CASE WHEN StatusCode IN ('ERROR', 'STATUS_CODE_ERROR') THEN 1 ELSE 0 END) as hasError | |
| FROM otel_traces | |
| WHERE TraceId IN (${placeholders}) | |
| GROUP BY TraceId, ServiceName |
🤖 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 1110 - 1141,
The SQL aggregates (aggSql and svcSql) only treat StatusCode =
'STATUS_CODE_ERROR' as an error, but traces are serialized using
statusCodeToString(), so errorCount and hasError are wrong; update the CASE WHEN
checks in aggSql (errorCount) and svcSql (hasError) to detect the same string(s)
produced by statusCodeToString() (or alternatively check the numeric enum values
if StatusCode is stored numerically) so the aggregate logic matches how
StatusCode is written; locate statusCodeToString and make the SQL match its
output (or add an OR to the CASE to accept both legacy 'STATUS_CODE_ERROR' and
the current serialized value).
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsx (1)
61-83: Avoid spread-based min/max over unbounded arraysLine 77/78 and Line 81/82 use
Math.min(...arr)/Math.max(...arr). For very large groups, this can become fragile and adds avoidable allocations. A single-pass aggregation is safer and faster.Proposed refactor
for (const [key, { spans, selfTimes }] of groups) { - const durations = spans.map((s) => s.durationMs); const count = spans.length; - const totalDuration = durations.reduce((a, b) => a + b, 0); - const selfTimeTotal = selfTimes.reduce((a, b) => a + b, 0); + let totalDuration = 0; + let minDuration = Infinity; + let maxDuration = -Infinity; + let selfTimeTotal = 0; + let selfTimeMin = Infinity; + let selfTimeMax = -Infinity; + + for (let i = 0; i < count; i++) { + const d = spans[i]!.durationMs; + const st = selfTimes[i]!; + totalDuration += d; + selfTimeTotal += st; + if (d < minDuration) minDuration = d; + if (d > maxDuration) maxDuration = d; + if (st < selfTimeMin) selfTimeMin = st; + if (st > selfTimeMax) selfTimeMax = st; + } @@ stats.push({ key, serviceName: firstSpan.serviceName, spanName: firstSpan.name, count, totalDuration, avgDuration: totalDuration / count, - minDuration: Math.min(...durations), - maxDuration: Math.max(...durations), + minDuration, + maxDuration, selfTimeTotal, selfTimeAvg: selfTimeTotal / count, - selfTimeMin: Math.min(...selfTimes), - selfTimeMax: Math.max(...selfTimes), + selfTimeMin, + selfTimeMax, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsx` around lines 61 - 83, The current aggregation in the groups loop uses spread-based Math.min/Math.max on durations and selfTimes (Math.min(...durations), Math.max(...durations), Math.min(...selfTimes), Math.max(...selfTimes)), which is fragile and allocates for large arrays; replace those spread calls by computing min/max in the same single-pass that computes totalDuration and selfTimeTotal (or a simple for loop over spans/selfTimes) so you accumulate totalDuration, selfTimeTotal, minDuration, maxDuration, selfTimeMin and selfTimeMax without creating extra arrays or using the spread operator; update the stats.push payload to use these precomputed min/max values (symbols: groups, spans, selfTimes, totalDuration, selfTimeTotal, minDuration, maxDuration, selfTimeMin, selfTimeMax).packages/ui/src/pages/observability.tsx (1)
484-493: Prefer the sharedTraceSummaryRowtype over a local copy.
packages/core/src/data-filters-zod.ts:294-305already defines this payload shape. Keeping a second interface here makes the page easy to drift out of sync with the validated API contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/pages/observability.tsx` around lines 484 - 493, Replace the local TraceSummaryRow interface with the shared type exported by the data-filters module: remove the duplicated interface declaration and import the exported TraceSummaryRow type (the one that defines the same payload shape in data-filters-zod) and use that everywhere this local interface was referenced; ensure any usages in this file (e.g., component props, map/filter results) are updated to the imported type so the page stays consistent with the validated API contract.
🤖 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/clickhouse-datasource/src/datasource.ts`:
- Around line 336-341: The code currently casts row.json() directly (e.g., in
the resultSet.stream() loop that fills services) which can crash if the payload
shape changes; update the row parsing in that loop and the other similar sites
(around the row.json() usages at the ranges you noted and before the iteration
over _serviceData) to validate the object shape before using it: check that
row.json() returns an object and that the ServiceName (or expected fields used
later) is a string (or other expected types), skip or log malformed rows, and
only push/assign validated values into services/_serviceData; use the same
guarded validation logic in the other locations (the block around lines 396-400
and 470-473) to prevent unchecked casts and downstream runtime failures.
In `@packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsx`:
- Around line 50-55: The composite key currently built as
`${span.serviceName}:${span.name}` can collide if either value contains ":";
change the key generation to a collision-safe encoding (e.g., use
JSON.stringify([span.serviceName, span.name]) or another unique
delimiter/escaping) wherever the key is created and consumed (the variable named
key, the groups Map population, and any row key generation based on that key) so
service/name pairs cannot merge accidentally; ensure all lookups and
Map.set/Map.get usages (including the code that reads row keys) use the new
encoding consistently.
- Around line 37-43: computeSelfTime currently subtracts the sum of
child.durationMs which double-counts overlapping children; update
computeSelfTime(span: SpanNode) to first build child intervals as
[child.startMs, child.endMs] clamped to the parent span bounds, merge
overlapping intervals to get total covered time, then subtract that
merged-interval total from span.durationMs and return Math.max(0, result). Use
span.children, child.startMs/child.endMs (or equivalent) and span.durationMs to
locate and implement the fix in computeSelfTime.
- Around line 166-173: The sortable column header currently uses an onClick on
the <th>, which is not keyboard-accessible; update the header rendering in
StatisticsView (the <th> that calls handleSort) to put an interactive <button>
inside the <th> (so keyboard and screen-reader users can activate sorting), add
scope="col" to the <th> for semantics, and set aria-sort on the <th> (or on the
button if you prefer) to "ascending", "descending" or "none" based on sortField
and sortAsc; ensure the button calls handleSort(col.field) and keeps the same
visible sort indicator using sortField and sortAsc.
In `@packages/ui/src/pages/observability.tsx`:
- Around line 506-547: The datasource builder always hardcodes params.sortOrder
= "DESC"; instead read the URL-provided sort (e.g. urlState.sort) and set
params.sortOrder = urlState.sort ?? "DESC" (ensuring the value matches expected
type/values like "ASC"/"DESC"), and add urlState.sort to the useMemo dependency
array so changes to ?sort update the DataSource; adjust any type assertions if
needed to accept the dynamic sort value referenced in the DataSource returned by
ds.
- Around line 94-106: The returned limit from readURLState (rawLimit → limit)
must be clamped to a safe range before being forwarded to TraceSearchView;
update the logic around rawLimit/limit to parse the value and then clamp it to a
minimum of 1 and a maximum (e.g., 1000) and return that clamped number (or null
if parse fails) so zeros, negatives, and huge values are rejected—modify the
limit assignment in readURLState to perform parseInt then Math.max(1,
Math.min(parsed, 1000)) (or equivalent) and return null when NaN.
- Around line 371-379: The parser's regex (\w+) only allows
letters/digits/underscore and thus drops valid OpenTelemetry tag characters
(e.g. dots), so update parseLogfmt to accept any non-space, non-equals key
token: replace the key capture group (\w+) with a more permissive pattern such
as ([^=\s]+) (or [A-Za-z0-9_.-]+ if you prefer stricter allowed chars) in the
regex used in parseLogfmt so keys like service.name or http.response.status_code
are preserved; keep the rest of the function logic the same.
---
Nitpick comments:
In `@packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsx`:
- Around line 61-83: The current aggregation in the groups loop uses
spread-based Math.min/Math.max on durations and selfTimes
(Math.min(...durations), Math.max(...durations), Math.min(...selfTimes),
Math.max(...selfTimes)), which is fragile and allocates for large arrays;
replace those spread calls by computing min/max in the same single-pass that
computes totalDuration and selfTimeTotal (or a simple for loop over
spans/selfTimes) so you accumulate totalDuration, selfTimeTotal, minDuration,
maxDuration, selfTimeMin and selfTimeMax without creating extra arrays or using
the spread operator; update the stats.push payload to use these precomputed
min/max values (symbols: groups, spans, selfTimes, totalDuration, selfTimeTotal,
minDuration, maxDuration, selfTimeMin, selfTimeMax).
In `@packages/ui/src/pages/observability.tsx`:
- Around line 484-493: Replace the local TraceSummaryRow interface with the
shared type exported by the data-filters module: remove the duplicated interface
declaration and import the exported TraceSummaryRow type (the one that defines
the same payload shape in data-filters-zod) and use that everywhere this local
interface was referenced; ensure any usages in this file (e.g., component props,
map/filter results) are updated to the imported type so the page stays
consistent with the validated API contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ddafc17-1766-4383-9b0b-ee53c03aff0d
📒 Files selected for processing (4)
packages/clickhouse-datasource/src/datasource.tspackages/ui/src/components/observability/TraceTimeline/GraphView.tsxpackages/ui/src/components/observability/TraceTimeline/StatisticsView.tsxpackages/ui/src/pages/observability.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/components/observability/TraceTimeline/GraphView.tsx
Comment on lines
+336
to
+341
| const services: string[] = []; | ||
| for await (const batch of resultSet.stream()) { | ||
| for (const row of batch) { | ||
| const json = row.json() as { ServiceName: string }; | ||
| services.push(json.ServiceName); | ||
| } |
There was a problem hiding this comment.
Validate ClickHouse row shapes instead of relying on unchecked casts.
These branches trust external JSON payloads with as assertions. A schema drift or malformed row can break downstream logic (especially Line 486 iteration over _serviceData) with non-obvious runtime failures.
Proposed hardening diff
@@
- const json = row.json() as { ServiceName: string };
- services.push(json.ServiceName);
+ const json = row.json() as Record<string, unknown>;
+ if (typeof json.ServiceName !== "string") {
+ throw new Error("Invalid ClickHouse row: ServiceName must be a string");
+ }
+ services.push(json.ServiceName);
@@
- const json = row.json() as { SpanName: string };
- operations.push(json.SpanName);
+ const json = row.json() as Record<string, unknown>;
+ if (typeof json.SpanName !== "string") {
+ throw new Error("Invalid ClickHouse row: SpanName must be a string");
+ }
+ operations.push(json.SpanName);
@@
- rawRows.push(row.json() as (typeof rawRows)[number]);
+ const json = row.json() as Record<string, unknown>;
+ if (
+ typeof json.TraceId !== "string" ||
+ typeof json.rootServiceName !== "string" ||
+ typeof json.rootSpanName !== "string" ||
+ typeof json.startTimeNs !== "string" ||
+ typeof json.durationNs !== "string" ||
+ typeof json.spanCount !== "number" ||
+ typeof json.errorCount !== "number" ||
+ !Array.isArray(json._serviceData)
+ ) {
+ throw new Error("Invalid ClickHouse row shape for trace summary");
+ }
+ rawRows.push(json as (typeof rawRows)[number]);Also applies to: 396-400, 470-473
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/clickhouse-datasource/src/datasource.ts` around lines 336 - 341, The
code currently casts row.json() directly (e.g., in the resultSet.stream() loop
that fills services) which can crash if the payload shape changes; update the
row parsing in that loop and the other similar sites (around the row.json()
usages at the ranges you noted and before the iteration over _serviceData) to
validate the object shape before using it: check that row.json() returns an
object and that the ServiceName (or expected fields used later) is a string (or
other expected types), skip or log malformed rows, and only push/assign
validated values into services/_serviceData; use the same guarded validation
logic in the other locations (the block around lines 396-400 and 470-473) to
prevent unchecked casts and downstream runtime failures.
Comment on lines
+37
to
+43
| function computeSelfTime(span: SpanNode): number { | ||
| const childrenTotal = span.children.reduce( | ||
| (sum, child) => sum + child.durationMs, | ||
| 0 | ||
| ); | ||
| return Math.max(0, span.durationMs - childrenTotal); | ||
| } |
There was a problem hiding this comment.
Self-time is incorrect for overlapping child spans
Line 38–42 subtracts the sum of child durations, which underestimates self-time when children overlap. Use merged child intervals (clamped to parent bounds) before subtraction.
Proposed fix
function computeSelfTime(span: SpanNode): number {
- const childrenTotal = span.children.reduce(
- (sum, child) => sum + child.durationMs,
- 0
- );
- return Math.max(0, span.durationMs - childrenTotal);
+ if (span.children.length === 0) return span.durationMs;
+
+ const parentStart = span.startTimeUnixMs;
+ const parentEnd = span.endTimeUnixMs;
+ const intervals = span.children
+ .map((child) => [
+ Math.max(parentStart, child.startTimeUnixMs),
+ Math.min(parentEnd, child.endTimeUnixMs),
+ ] as const)
+ .filter(([start, end]) => end > start)
+ .sort((a, b) => a[0] - b[0]);
+
+ let covered = 0;
+ let [curStart, curEnd] = intervals[0] ?? [parentStart, parentStart];
+ for (let i = 1; i < intervals.length; i++) {
+ const [start, end] = intervals[i]!;
+ if (start <= curEnd) {
+ curEnd = Math.max(curEnd, end);
+ } else {
+ covered += curEnd - curStart;
+ curStart = start;
+ curEnd = end;
+ }
+ }
+ covered += curEnd - curStart;
+
+ return Math.max(0, span.durationMs - covered);
}📝 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
| function computeSelfTime(span: SpanNode): number { | |
| const childrenTotal = span.children.reduce( | |
| (sum, child) => sum + child.durationMs, | |
| 0 | |
| ); | |
| return Math.max(0, span.durationMs - childrenTotal); | |
| } | |
| function computeSelfTime(span: SpanNode): number { | |
| if (span.children.length === 0) return span.durationMs; | |
| const parentStart = span.startTimeUnixMs; | |
| const parentEnd = span.endTimeUnixMs; | |
| const intervals = span.children | |
| .map((child) => [ | |
| Math.max(parentStart, child.startTimeUnixMs), | |
| Math.min(parentEnd, child.endTimeUnixMs), | |
| ] as const) | |
| .filter(([start, end]) => end > start) | |
| .sort((a, b) => a[0] - b[0]); | |
| let covered = 0; | |
| let [curStart, curEnd] = intervals[0] ?? [parentStart, parentStart]; | |
| for (let i = 1; i < intervals.length; i++) { | |
| const [start, end] = intervals[i]!; | |
| if (start <= curEnd) { | |
| curEnd = Math.max(curEnd, end); | |
| } else { | |
| covered += curEnd - curStart; | |
| curStart = start; | |
| curEnd = end; | |
| } | |
| } | |
| covered += curEnd - curStart; | |
| return Math.max(0, span.durationMs - covered); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsx`
around lines 37 - 43, computeSelfTime currently subtracts the sum of
child.durationMs which double-counts overlapping children; update
computeSelfTime(span: SpanNode) to first build child intervals as
[child.startMs, child.endMs] clamped to the parent span bounds, merge
overlapping intervals to get total covered time, then subtract that
merged-interval total from span.durationMs and return Math.max(0, result). Use
span.children, child.startMs/child.endMs (or equivalent) and span.durationMs to
locate and implement the fix in computeSelfTime.
Comment on lines
+50
to
+55
| const key = `${span.serviceName}:${span.name}`; | ||
| let group = groups.get(key); | ||
| if (!group) { | ||
| group = { spans: [], selfTimes: [] }; | ||
| groups.set(key, group); | ||
| } |
There was a problem hiding this comment.
Composite string key can collide across service/span names
Line 50 builds keys with service:name. If either value contains :, unrelated groups can merge and Line 180 row keys can collide. Use a collision-safe key format.
Proposed fix
- const key = `${span.serviceName}:${span.name}`;
+ const key = JSON.stringify([span.serviceName, span.name]);Also applies to: 180-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsx`
around lines 50 - 55, The composite key currently built as
`${span.serviceName}:${span.name}` can collide if either value contains ":";
change the key generation to a collision-safe encoding (e.g., use
JSON.stringify([span.serviceName, span.name]) or another unique
delimiter/escaping) wherever the key is created and consumed (the variable named
key, the groups Map population, and any row key generation based on that key) so
service/name pairs cannot merge accidentally; ensure all lookups and
Map.set/Map.get usages (including the code that reads row keys) use the new
encoding consistently.
Comment on lines
+166
to
+173
| <th | ||
| key={col.field} | ||
| className="px-3 py-2 text-left text-xs font-medium text-muted-foreground cursor-pointer select-none hover:text-foreground whitespace-nowrap" | ||
| onClick={() => handleSort(col.field)} | ||
| > | ||
| {col.label}{" "} | ||
| {sortField === col.field ? (sortAsc ? "▲" : "▼") : ""} | ||
| </th> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its line count
wc -l packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsxRepository: kopai-app/kopai-mono
Length of output: 141
🏁 Script executed:
# Read lines 160-180 to see the context and verify the code snippet
sed -n '160,180p' packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsxRepository: kopai-app/kopai-mono
Length of output: 828
🏁 Script executed:
# Search for the handleSort function to understand how sorting is implemented
rg -n "handleSort" packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsxRepository: kopai-app/kopai-mono
Length of output: 170
🏁 Script executed:
# Check if there are any keyboard handlers (onKeyDown, onKeyUp, onKeyPress) in the file
rg -n "onKey|ENTER|SPACE" packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsxRepository: kopai-app/kopai-mono
Length of output: 46
🏁 Script executed:
# Look at the full component including imports and state management to understand the context better
head -n 100 packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsxRepository: kopai-app/kopai-mono
Length of output: 2621
🏁 Script executed:
# Check for any accessibility-related imports or ARIA attributes in the file
rg -n "aria-|role=" packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsxRepository: kopai-app/kopai-mono
Length of output: 46
🏁 Script executed:
# Read more of the file to see the complete component and handleSort implementation
sed -n '140,180p' packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsxRepository: kopai-app/kopai-mono
Length of output: 1286
🏁 Script executed:
# Check for any keyboard event handler patterns in the codebase related to table headers
rg -n "onKeyDown\|onKeyPress" packages/ui/src/components/observability/TraceTimeline/ -A 2Repository: kopai-app/kopai-mono
Length of output: 46
🏁 Script executed:
# Look for similar sortable table patterns in the codebase to see how they handle accessibility
find packages/ui/src -type f -name "*.tsx" -o -name "*.ts" | head -20 | xargs grep -l "sortable\|sort.*header" 2>/dev/null | head -5Repository: kopai-app/kopai-mono
Length of output: 46
🏁 Script executed:
# Let me search for accessibility patterns or best practices documentation in the repo
find packages/ui -type f \( -name "*.md" -o -name "*.txt" \) | xargs grep -l "accessibility\|a11y\|WCAG" 2>/dev/null | head -5Repository: kopai-app/kopai-mono
Length of output: 46
🏁 Script executed:
# Check if there's a button component being used elsewhere in the codebase for similar patterns
rg -n "button.*onClick.*sort\|sort.*button" packages/ui/src -i | head -10Repository: kopai-app/kopai-mono
Length of output: 46
🏁 Script executed:
# Look at other table header implementations to see if they have accessibility features
rg -n "aria-sort\|scope=\"col\"" packages/ui/src -B 3 -A 3Repository: kopai-app/kopai-mono
Length of output: 46
🏁 Script executed:
# Search for button components or interactive elements in table headers to understand the repo's patterns
rg -n "<button" packages/ui/src/components | grep -i "table\|header" | head -10Repository: kopai-app/kopai-mono
Length of output: 251
🏁 Script executed:
# Check what version of React and any accessibility libraries are being used
cat packages/ui/package.json | grep -A 5 '"dependencies"'Repository: kopai-app/kopai-mono
Length of output: 253
Sortable headers must be keyboard-accessible
The <th> element at line 169 uses onClick only, making it inaccessible to keyboard and assistive technology users. Replace with a <button> inside the header, add aria-sort to indicate sort state, and include scope="col" for semantic clarity.
Proposed fix
<th
key={col.field}
- className="px-3 py-2 text-left text-xs font-medium text-muted-foreground cursor-pointer select-none hover:text-foreground whitespace-nowrap"
- onClick={() => handleSort(col.field)}
+ scope="col"
+ aria-sort={
+ sortField === col.field
+ ? sortAsc
+ ? "ascending"
+ : "descending"
+ : "none"
+ }
+ className="px-3 py-2 text-left text-xs font-medium text-muted-foreground whitespace-nowrap"
>
- {col.label}{" "}
- {sortField === col.field ? (sortAsc ? "▲" : "▼") : ""}
+ <button
+ type="button"
+ className="inline-flex items-center gap-1 cursor-pointer select-none hover:text-foreground"
+ onClick={() => handleSort(col.field)}
+ >
+ {col.label}
+ {sortField === col.field ? (sortAsc ? "▲" : "▼") : ""}
+ </button>
</th>📝 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
| <th | |
| key={col.field} | |
| className="px-3 py-2 text-left text-xs font-medium text-muted-foreground cursor-pointer select-none hover:text-foreground whitespace-nowrap" | |
| onClick={() => handleSort(col.field)} | |
| > | |
| {col.label}{" "} | |
| {sortField === col.field ? (sortAsc ? "▲" : "▼") : ""} | |
| </th> | |
| <th | |
| key={col.field} | |
| scope="col" | |
| aria-sort={ | |
| sortField === col.field | |
| ? sortAsc | |
| ? "ascending" | |
| : "descending" | |
| : "none" | |
| } | |
| className="px-3 py-2 text-left text-xs font-medium text-muted-foreground whitespace-nowrap" | |
| > | |
| <button | |
| type="button" | |
| className="inline-flex items-center gap-1 cursor-pointer select-none hover:text-foreground" | |
| onClick={() => handleSort(col.field)} | |
| > | |
| {col.label} | |
| {sortField === col.field ? (sortAsc ? "▲" : "▼") : ""} | |
| </button> | |
| </th> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/observability/TraceTimeline/StatisticsView.tsx`
around lines 166 - 173, The sortable column header currently uses an onClick on
the <th>, which is not keyboard-accessible; update the header rendering in
StatisticsView (the <th> that calls handleSort) to put an interactive <button>
inside the <th> (so keyboard and screen-reader users can activate sorting), add
scope="col" to the <th> for semantics, and set aria-sort on the <th> (or on the
button if you prefer) to "ascending", "descending" or "none" based on sortField
and sortAsc; ensure the button calls handleSort(col.field) and keeps the same
visible sort indicator using sortField and sortAsc.
Comment on lines
+94
to
+106
| const rawLimit = params.get("limit"); | ||
| const limit = rawLimit ? parseInt(rawLimit, 10) : null; | ||
| return { | ||
| tab, | ||
| service, | ||
| operation: params.get("operation"), | ||
| tags: params.get("tags"), | ||
| lookback: params.get("lookback"), | ||
| tsMin: params.get("tsMin"), | ||
| tsMax: params.get("tsMax"), | ||
| minDuration: params.get("minDuration"), | ||
| maxDuration: params.get("maxDuration"), | ||
| limit: limit !== null && !isNaN(limit) ? limit : null, |
There was a problem hiding this comment.
Clamp limit before it reaches trace-summary queries.
readURLState accepts any numeric ?limit= and TraceSearchView forwards it unchanged. 0, negatives, or very large values can turn a shared URL into either an empty page or an unexpectedly expensive query. The logs URL parser in this same file already bounds limit; traces should do the same.
🛡️ Proposed fix
const rawLimit = params.get("limit");
- const limit = rawLimit ? parseInt(rawLimit, 10) : null;
+ const parsedLimit = rawLimit ? Number.parseInt(rawLimit, 10) : NaN;
+ const limit =
+ Number.isFinite(parsedLimit) && parsedLimit >= 1 && parsedLimit <= 1000
+ ? parsedLimit
+ : null;
return {
tab,
service,
@@
- limit: limit !== null && !isNaN(limit) ? limit : null,
+ limit,📝 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 rawLimit = params.get("limit"); | |
| const limit = rawLimit ? parseInt(rawLimit, 10) : null; | |
| return { | |
| tab, | |
| service, | |
| operation: params.get("operation"), | |
| tags: params.get("tags"), | |
| lookback: params.get("lookback"), | |
| tsMin: params.get("tsMin"), | |
| tsMax: params.get("tsMax"), | |
| minDuration: params.get("minDuration"), | |
| maxDuration: params.get("maxDuration"), | |
| limit: limit !== null && !isNaN(limit) ? limit : null, | |
| const rawLimit = params.get("limit"); | |
| const parsedLimit = rawLimit ? Number.parseInt(rawLimit, 10) : NaN; | |
| const limit = | |
| Number.isFinite(parsedLimit) && parsedLimit >= 1 && parsedLimit <= 1000 | |
| ? parsedLimit | |
| : null; | |
| return { | |
| tab, | |
| service, | |
| operation: params.get("operation"), | |
| tags: params.get("tags"), | |
| lookback: params.get("lookback"), | |
| tsMin: params.get("tsMin"), | |
| tsMax: params.get("tsMax"), | |
| minDuration: params.get("minDuration"), | |
| maxDuration: params.get("maxDuration"), | |
| limit, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/pages/observability.tsx` around lines 94 - 106, The returned
limit from readURLState (rawLimit → limit) must be clamped to a safe range
before being forwarded to TraceSearchView; update the logic around
rawLimit/limit to parse the value and then clamp it to a minimum of 1 and a
maximum (e.g., 1000) and return that clamped number (or null if parse fails) so
zeros, negatives, and huge values are rejected—modify the limit assignment in
readURLState to perform parseInt then Math.max(1, Math.min(parsed, 1000)) (or
equivalent) and return null when NaN.
Comment on lines
+371
to
+379
| function parseLogfmt(str: string): Record<string, string> { | ||
| const result: Record<string, string> = {}; | ||
| const re = /(\w+)=(?:"([^"]*)"|([\S]*))/g; | ||
| let m: RegExpExecArray | null; | ||
| while ((m = re.exec(str)) !== null) { | ||
| const key = m[1]; | ||
| if (key) result[key] = m[2] ?? m[3] ?? ""; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
parseLogfmt rejects common OpenTelemetry tag keys.
\w+ only matches letters, digits, and _, so inputs like service.name=checkout or http.response.status_code=500 get truncated to the last token and the backend receives the wrong tag key. That makes tag-based trace search silently miss results.
🔎 Proposed fix
function parseLogfmt(str: string): Record<string, string> {
const result: Record<string, string> = {};
- const re = /(\w+)=(?:"([^"]*)"|([\S]*))/g;
+ const re = /([^\s=]+)=(?:"([^"]*)"|(\S*))/g;
let m: RegExpExecArray | null;
while ((m = re.exec(str)) !== null) {
const key = m[1];📝 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
| function parseLogfmt(str: string): Record<string, string> { | |
| const result: Record<string, string> = {}; | |
| const re = /(\w+)=(?:"([^"]*)"|([\S]*))/g; | |
| let m: RegExpExecArray | null; | |
| while ((m = re.exec(str)) !== null) { | |
| const key = m[1]; | |
| if (key) result[key] = m[2] ?? m[3] ?? ""; | |
| } | |
| return result; | |
| function parseLogfmt(str: string): Record<string, string> { | |
| const result: Record<string, string> = {}; | |
| const re = /([^\s=]+)=(?:"([^"]*)"|(\S*))/g; | |
| let m: RegExpExecArray | null; | |
| while ((m = re.exec(str)) !== null) { | |
| const key = m[1]; | |
| if (key) result[key] = m[2] ?? m[3] ?? ""; | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/pages/observability.tsx` around lines 371 - 379, The parser's
regex (\w+) only allows letters/digits/underscore and thus drops valid
OpenTelemetry tag characters (e.g. dots), so update parseLogfmt to accept any
non-space, non-equals key token: replace the key capture group (\w+) with a more
permissive pattern such as ([^=\s]+) (or [A-Za-z0-9_.-]+ if you prefer stricter
allowed chars) in the regex used in parseLogfmt so keys like service.name or
http.response.status_code are preserved; keep the rest of the function logic the
same.
Comment on lines
+506
to
+547
| const ds = useMemo<DataSource>(() => { | ||
| const params: Record<string, unknown> = { | ||
| limit: urlState.limit ?? 20, | ||
| sortOrder: "DESC" as const, | ||
| }; | ||
| if (service) params.serviceName = service; | ||
| if (urlState.operation) params.spanName = urlState.operation; | ||
| if (urlState.lookback) { | ||
| const ms = LOOKBACK_MS[urlState.lookback]; | ||
| if (ms) { | ||
| params.timestampMin = String((Date.now() - ms) * 1e6); | ||
| } | ||
| } | ||
| if (urlState.tsMin) params.timestampMin = urlState.tsMin; | ||
| if (urlState.tsMax) params.timestampMax = urlState.tsMax; | ||
| if (urlState.minDuration) { | ||
| const parsed = parseDuration(urlState.minDuration); | ||
| if (parsed) params.durationMin = parsed; | ||
| } | ||
| if (urlState.maxDuration) { | ||
| const parsed = parseDuration(urlState.maxDuration); | ||
| if (parsed) params.durationMax = parsed; | ||
| } | ||
| if (urlState.tags) { | ||
| const tagMap = parseLogfmt(urlState.tags); | ||
| if (Object.keys(tagMap).length > 0) params.tags = tagMap; | ||
| } | ||
| return { | ||
| method: "searchTraceSummariesPage", | ||
| params, | ||
| } as DataSource; | ||
| }, [ | ||
| service, | ||
| urlState.operation, | ||
| urlState.lookback, | ||
| urlState.tsMin, | ||
| urlState.tsMax, | ||
| urlState.minDuration, | ||
| urlState.maxDuration, | ||
| urlState.limit, | ||
| urlState.tags, | ||
| ]); |
There was a problem hiding this comment.
Honor sort when building the trace-summary datasource.
sort is read from the URL and written back out, but this query always sends "DESC". Any ?sort=ASC link will still fetch descending results.
↕️ Proposed fix
const ds = useMemo<DataSource>(() => {
+ const sortOrder = urlState.sort === "ASC" ? "ASC" : "DESC";
const params: Record<string, unknown> = {
limit: urlState.limit ?? 20,
- sortOrder: "DESC" as const,
+ sortOrder,
};
@@
urlState.maxDuration,
urlState.limit,
+ urlState.sort,
urlState.tags,
]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/pages/observability.tsx` around lines 506 - 547, The
datasource builder always hardcodes params.sortOrder = "DESC"; instead read the
URL-provided sort (e.g. urlState.sort) and set params.sortOrder = urlState.sort
?? "DESC" (ensuring the value matches expected type/values like "ASC"/"DESC"),
and add urlState.sort to the useMemo dependency array so changes to ?sort update
the DataSource; adjust any type assertions if needed to accept the dynamic sort
value referenced in the DataSource returned by ds.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/sqlite-datasource/src/db-datasource.ts (1)
1110-1141: Consider aligning status code string with ClickHouse implementation.The SQLite implementation checks
StatusCode = 'STATUS_CODE_ERROR'while the ClickHouse implementation usesStatusCode = 'ERROR'. Since SQLite controls both write (viastatusCodeToString) and read paths, this is internally consistent. However, for cross-datasource parity and defensive handling, consider matching both:StatusCode IN ('ERROR', 'STATUS_CODE_ERROR')🤖 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 1110 - 1141, The SQL checks for StatusCode currently compare only to 'STATUS_CODE_ERROR' in the aggSql and svcSql queries; update those CASE/MAX conditions (in the aggSql SUM(...) and svcSql MAX(...) expressions inside the otel_traces queries used by aggSql and svcSql) to treat both representations by using an IN check for 'ERROR' and 'STATUS_CODE_ERROR' (e.g., StatusCode IN ('ERROR','STATUS_CODE_ERROR')), and apply the same change to any other StatusCode comparisons in this file (look for statusCodeToString usage and any other CASE WHEN StatusCode = ... occurrences) so reads are tolerant of both formats.
🤖 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/clickhouse-datasource/src/query-traces.ts`:
- Around line 97-108: Trace duration parameters coming from TraceSummariesFilter
(durationMin/durationMax) are strings but are bound to ClickHouse as UInt64 via
params.durMin/params.durMax; convert and validate them to numeric (preferably
BigInt or Number depending on how ClickHouse client expects UInt64) before
assigning to params to avoid type/coercion errors. Locate the block that builds
havingConditions and sets params.durMin/params.durMax and replace the direct
assignment with parsing (e.g., parseInt/BigInt) plus a check that the parsed
value is a safe non-negative integer, and throw or skip the filter if invalid so
the ClickHouse binding receives a numeric UInt64 value. Ensure the same
conversion is applied for both durationMin and durationMax and preserve the
existing null/undefined checks.
In `@packages/ui/src/components/observability/TraceTimeline/GraphView.tsx`:
- Around line 56-57: The current edge key construction using string
interpolation `${parentService}→${svc}` (used before calling edgeMap.get) is
ambiguous when service names include the delimiter; change the key strategy to
an unambiguous encoder/structure: either build a deterministic composite key via
JSON.stringify([parentService, svc]) or switch edgeMap from a flat
Map<string,Edge> to a nested Map(parentService -> Map(svc -> Edge)); update all
places that create/lookup keys (the spots that build `${parentService}→${svc}`
and the later similar code) to use the new encoder or nested map API so
reconstruction and merges/splits remain correct.
- Around line 126-127: The dequeued node check uses a falsy check (if (!cur)
continue) which incorrectly treats valid values like empty string as missing;
update the conditional in the loop that processes queue.shift() so it only skips
when the result is undefined (e.g., check cur === undefined), leaving valid
empty-string service names intact; locate the code around the queue.shift() call
and the variable cur in GraphView.tsx to make this change.
---
Nitpick comments:
In `@packages/sqlite-datasource/src/db-datasource.ts`:
- Around line 1110-1141: The SQL checks for StatusCode currently compare only to
'STATUS_CODE_ERROR' in the aggSql and svcSql queries; update those CASE/MAX
conditions (in the aggSql SUM(...) and svcSql MAX(...) expressions inside the
otel_traces queries used by aggSql and svcSql) to treat both representations by
using an IN check for 'ERROR' and 'STATUS_CODE_ERROR' (e.g., StatusCode IN
('ERROR','STATUS_CODE_ERROR')), and apply the same change to any other
StatusCode comparisons in this file (look for statusCodeToString usage and any
other CASE WHEN StatusCode = ... occurrences) so reads are tolerant of both
formats.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 890573dc-9343-4e1c-b0b2-86f68b51671e
📒 Files selected for processing (5)
packages/clickhouse-datasource/src/datasource.test.tspackages/clickhouse-datasource/src/query-traces.tspackages/sqlite-datasource/src/db-datasource.tspackages/ui/src/components/observability/TraceTimeline/GraphView.tsxpackages/ui/src/components/observability/TraceTimeline/Minimap.tsx
Comment on lines
+97
to
+108
| if (filter.durationMin != null) { | ||
| havingConditions.push( | ||
| "dateDiff('nanosecond', min(Timestamp), max(Timestamp + toIntervalNanosecond(Duration))) >= {durMin:UInt64}" | ||
| ); | ||
| params.durMin = filter.durationMin; | ||
| } | ||
| if (filter.durationMax != null) { | ||
| havingConditions.push( | ||
| "dateDiff('nanosecond', min(Timestamp), max(Timestamp + toIntervalNanosecond(Duration))) <= {durMax:UInt64}" | ||
| ); | ||
| params.durMax = filter.durationMax; | ||
| } |
There was a problem hiding this comment.
Duration filter parameters should be numeric, not string.
The TraceSummariesFilter schema defines durationMin and durationMax as optional strings, but the ClickHouse parameter binding expects UInt64. Passing a string directly may cause type coercion issues or query failures.
🔧 Proposed fix
if (filter.durationMin != null) {
havingConditions.push(
"dateDiff('nanosecond', min(Timestamp), max(Timestamp + toIntervalNanosecond(Duration))) >= {durMin:UInt64}"
);
- params.durMin = filter.durationMin;
+ params.durMin = BigInt(filter.durationMin);
}
if (filter.durationMax != null) {
havingConditions.push(
"dateDiff('nanosecond', min(Timestamp), max(Timestamp + toIntervalNanosecond(Duration))) <= {durMax:UInt64}"
);
- params.durMax = filter.durationMax;
+ params.durMax = BigInt(filter.durationMax);
}📝 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
| if (filter.durationMin != null) { | |
| havingConditions.push( | |
| "dateDiff('nanosecond', min(Timestamp), max(Timestamp + toIntervalNanosecond(Duration))) >= {durMin:UInt64}" | |
| ); | |
| params.durMin = filter.durationMin; | |
| } | |
| if (filter.durationMax != null) { | |
| havingConditions.push( | |
| "dateDiff('nanosecond', min(Timestamp), max(Timestamp + toIntervalNanosecond(Duration))) <= {durMax:UInt64}" | |
| ); | |
| params.durMax = filter.durationMax; | |
| } | |
| if (filter.durationMin != null) { | |
| havingConditions.push( | |
| "dateDiff('nanosecond', min(Timestamp), max(Timestamp + toIntervalNanosecond(Duration))) >= {durMin:UInt64}" | |
| ); | |
| params.durMin = BigInt(filter.durationMin); | |
| } | |
| if (filter.durationMax != null) { | |
| havingConditions.push( | |
| "dateDiff('nanosecond', min(Timestamp), max(Timestamp + toIntervalNanosecond(Duration))) <= {durMax:UInt64}" | |
| ); | |
| params.durMax = BigInt(filter.durationMax); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/clickhouse-datasource/src/query-traces.ts` around lines 97 - 108,
Trace duration parameters coming from TraceSummariesFilter
(durationMin/durationMax) are strings but are bound to ClickHouse as UInt64 via
params.durMin/params.durMax; convert and validate them to numeric (preferably
BigInt or Number depending on how ClickHouse client expects UInt64) before
assigning to params to avoid type/coercion errors. Locate the block that builds
havingConditions and sets params.durMin/params.durMax and replace the direct
assignment with parsing (e.g., parseInt/BigInt) plus a check that the parsed
value is a safe non-negative integer, and throw or skip the filter if invalid so
the ClickHouse binding receives a numeric UInt64 value. Ensure the same
conversion is applied for both durationMin and durationMax and preserve the
existing null/undefined checks.
Comment on lines
+56
to
+57
| const key = `${parentService}→${svc}`; | ||
| const edge = edgeMap.get(key); |
There was a problem hiding this comment.
Edge key encoding is ambiguous when service names contain →.
Delimiter-based keys can corrupt from/to reconstruction and merge/split edges incorrectly.
Suggested fix
- const key = `${parentService}→${svc}`;
+ const key = JSON.stringify([parentService, svc]);
...
- for (const [key, meta] of edgeMap) {
- const [from, to] = key.split("→");
- if (from && to) edges.push({ from, to, ...meta });
- }
+ for (const [key, meta] of edgeMap) {
+ const [from, to] = JSON.parse(key) as [string, string];
+ edges.push({ from, to, ...meta });
+ }Also applies to: 80-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/observability/TraceTimeline/GraphView.tsx` around
lines 56 - 57, The current edge key construction using string interpolation
`${parentService}→${svc}` (used before calling edgeMap.get) is ambiguous when
service names include the delimiter; change the key strategy to an unambiguous
encoder/structure: either build a deterministic composite key via
JSON.stringify([parentService, svc]) or switch edgeMap from a flat
Map<string,Edge> to a nested Map(parentService -> Map(svc -> Edge)); update all
places that create/lookup keys (the spots that build `${parentService}→${svc}`
and the later similar code) to use the new encoder or nested map API so
reconstruction and merges/splits remain correct.
Comment on lines
+126
to
+127
| const cur = queue.shift(); | ||
| if (!cur) continue; |
There was a problem hiding this comment.
Use undefined check instead of a falsy check for dequeued node.
Line 127 treats "" as missing and skips processing, which can drop valid services with empty names.
Suggested fix
- const cur = queue.shift();
- if (!cur) continue;
+ const cur = queue.shift();
+ if (cur === undefined) continue;📝 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 cur = queue.shift(); | |
| if (!cur) continue; | |
| const cur = queue.shift(); | |
| if (cur === undefined) continue; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/observability/TraceTimeline/GraphView.tsx` around
lines 126 - 127, The dequeued node check uses a falsy check (if (!cur) continue)
which incorrectly treats valid values like empty string as missing; update the
conditional in the loop that processes queue.shift() so it only skips when the
result is undefined (e.g., check cur === undefined), leaving valid empty-string
service names intact; locate the code around the queue.shift() call and the
variable cur in GraphView.tsx to make this change.
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 / UX
Tests