feat: Traces push live-refresh + page improvements - #206
Conversation
- Added multiple test cases to `spanTree.test.ts` to verify the behavior of `flattenTree` when `hideEngineRouting` is enabled. - Tests include scenarios for keeping worker calls visible while hiding engine dispatch wrappers, nesting worker rows per hop, and ensuring correct visibility when child calls share the same service. - Updated `spanTree.ts` with a new utility function `isHideableRoutingNode` to determine if a span should be hidden based on its name and parent context, improving the logic for rendering spans in the tree structure.
Add a fanout pump that subscribes to the agent::turn_end stream and, with a ~400ms trailing coalesce, pushes an empty ui::traces::changed signal to every all-sessions subscriber, reusing the existing FanoutState + ui::subscribe and the function_not_found eviction path. Add the console-side subscriber (useTracesLiveRefresh + a framework-free startTracesSubscription/makeTracesChangedHandler core) that invalidates the ['traces'] and ['traceGroups'] React Query caches on the signal, re-subscribes and re-syncs on WS reconnect, and re-syncs on tab-visible. This lets the Traces view refresh on real activity instead of a 3s polling timer. The Traces page wiring (dropping refetchInterval, calling the hook) lives in the page files and is tracked with the broader Traces-page changes.
Wire the Traces page to the ui::traces::changed push signal: drop the 3s refetchInterval from useTraceData/useTraceGroups, call useTracesLiveRefresh, and auto-pause live updates while a detail panel is open (restoring the prior pause intent on close, since polling is no longer the throttle). Also: Traces-view refinements plus extracted, unit-tested helpers (attributeText, minimapMarkers, percentOfTotal, preorder treeFlatten) with spanTree/traceListItem/traceTransform updates and component tweaks across FlameGraph, WaterfallChart, SpanOtelLogsTab, SpanErrorsTab, and the group/ detail panels.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR refactors the Traces feature to replace polling-based refresh with live signal-driven React Query invalidation, introduces defensive attribute handling, optimizes component rendering, and adds backend fanout infrastructure for broadcasting trace-change events across browsers. ChangesTraces Live-Refresh Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skill-check — worker0 verified, 13 skipped (no docs/).
Note 17 stale rendered artifact(s) detected on main, unrelated to this PR. This PR is fine; the drift was already there. A maintainer should open a chore PR to re-render these.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
console/web/src/pages/Traces/api/traces.ts (1)
174-176: ⚖️ Poor tradeoffFix: group-by isn’t ambiguous; only trace-tree detail loses the “exporter disabled” signal.
fetchTracesGroupByreturns{ groups: [] }onmemory exporter … not enabledandTracesGroupByResponsehas noexporterDisabled. However,Traces/index.tsxgates all group-by rendering behindhasOtelConfiguredderived from the flatfetchTracesresponse (exporterDisabled→ “no observability”), so the groups view won’t fall back to “no traces” when the exporter is disabled.
fetchTraceTreelikewise returns{ roots: [] }without anexporterDisabledflag; when tree detail is loaded, empty roots are treated as a generic empty/error state (“no span data available for this trace”), so the tree path can still lose the explicit “no observability” messaging. Consider adding an analogous flag toTraceTreeResponse(or wiring tree detail to the existinghasOtelConfiguredstate) instead of changingTracesGroupByResponse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/web/src/pages/Traces/api/traces.ts` around lines 174 - 176, The trace-tree path currently swallows the "memory exporter not enabled" signal by returning only { roots: [] } from fetchTraceTree; update the TraceTreeResponse type and the fetchTraceTree function to include an exporterDisabled: boolean and return exporterDisabled: true when isMemoryExporterNotEnabled(err) is true (i.e., change the error branch that currently returns { roots: [] } to return { roots: [], exporterDisabled: true }); also update any consumers (e.g., Traces/index.tsx or other components that read trace detail) to respect TraceTreeResponse.exporterDisabled the same way group-by uses exporterDisabled so the UI can show the explicit "no observability / exporter disabled" state instead of a generic empty-tree message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@console/web/src/pages/Traces/api/traces.test.ts`:
- Around line 8-11: The test's vi.mock factory is hoisted before `const call =
vi.fn()` so it closes over an uninitialized `call`; fix by declaring `call`
using `vi.hoisted` (e.g. `const call = vi.hoisted(() => vi.fn())`) so the mock
factory in the `getIiiClient` mock can access the initialized `call`; update the
`traces.test.ts` file so `call` is created with `vi.hoisted` and the existing
mock for `getIiiClient` continues to return an object that references `call`.
In `@console/web/src/pages/Traces/hooks/useTraceData.ts`:
- Around line 120-126: When a fetch returns no spans the code clears traceList
items and fingerprintRef but fails to clear pendingTracesRef, allowing stale
pending traces to be flushed later; update the empty-spans branch to also clear
pendingTracesRef.current (e.g., set to new Map() or empty array consistent with
its type) and remove the redundant setHasOtelConfigured(true) call inside
flushPendingTraces since useEffect already sets hasOtelConfigured based on
exporterDisabled; change references to pendingTracesRef and flushPendingTraces
accordingly so the hover/flush path no longer displays outdated traces.
---
Nitpick comments:
In `@console/web/src/pages/Traces/api/traces.ts`:
- Around line 174-176: The trace-tree path currently swallows the "memory
exporter not enabled" signal by returning only { roots: [] } from
fetchTraceTree; update the TraceTreeResponse type and the fetchTraceTree
function to include an exporterDisabled: boolean and return exporterDisabled:
true when isMemoryExporterNotEnabled(err) is true (i.e., change the error branch
that currently returns { roots: [] } to return { roots: [], exporterDisabled:
true }); also update any consumers (e.g., Traces/index.tsx or other components
that read trace detail) to respect TraceTreeResponse.exporterDisabled the same
way group-by uses exporterDisabled so the UI can show the explicit "no
observability / exporter disabled" state instead of a generic empty-tree
message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed514673-1f64-4c60-8e5b-8b773b7d6bd5
📒 Files selected for processing (32)
console/web/src/lib/devtools-stream.test.tsconsole/web/src/lib/devtools-stream.tsconsole/web/src/pages/Traces/api/traces.test.tsconsole/web/src/pages/Traces/api/traces.tsconsole/web/src/pages/Traces/components/FlameGraph.tsxconsole/web/src/pages/Traces/components/ServiceBreakdown.tsxconsole/web/src/pages/Traces/components/SessionDetailPanel.tsxconsole/web/src/pages/Traces/components/SpanErrorsTab.tsxconsole/web/src/pages/Traces/components/SpanOtelLogsTab.tsxconsole/web/src/pages/Traces/components/TraceFilters.tsxconsole/web/src/pages/Traces/components/TraceGroupsView.tsxconsole/web/src/pages/Traces/components/WaterfallChart.tsxconsole/web/src/pages/Traces/hooks/useTraceData.tsconsole/web/src/pages/Traces/hooks/useTraceGroups.tsconsole/web/src/pages/Traces/index.tsxconsole/web/src/pages/Traces/lib/attributeText.test.tsconsole/web/src/pages/Traces/lib/attributeText.tsconsole/web/src/pages/Traces/lib/minimapMarkers.test.tsconsole/web/src/pages/Traces/lib/minimapMarkers.tsconsole/web/src/pages/Traces/lib/percent.test.tsconsole/web/src/pages/Traces/lib/percent.tsconsole/web/src/pages/Traces/lib/spanTree.test.tsconsole/web/src/pages/Traces/lib/spanTree.tsconsole/web/src/pages/Traces/lib/traceListItem.test.tsconsole/web/src/pages/Traces/lib/traceListItem.tsconsole/web/src/pages/Traces/lib/traceTransform.test.tsconsole/web/src/pages/Traces/lib/traceTransform.tsconsole/web/src/pages/Traces/lib/treeFlatten.test.tsconsole/web/src/pages/Traces/lib/treeFlatten.tsharness/src/harness/fanout/index.tsharness/src/harness/fanout/traces-changed.tsharness/tests/harness/fanout/traces-changed.test.ts
| const call = vi.fn() | ||
| vi.mock('@/lib/iii-client', () => ({ | ||
| getIiiClient: vi.fn(async () => ({ call })), | ||
| })) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Vitest 4 throw an error when a vi.mock factory references a top-level variable not prefixed with "mock"?
💡 Result:
Yes, Vitest throws an error if a vi.mock factory references a variable defined outside of its scope [1][2][3]. This is because vi.mock is hoisted to the top of the file and executed before other module-level code [1][2][4]. The error message specifically states: "There was an error when mocking a module. If you are using 'vi.mock' factory, make sure there are no top level variables inside, since this call is hoisted to top of the file." [5][3]. Note that this requirement is not related to a variable naming convention (e.g., whether it is prefixed with "mock"); it applies to any variable defined outside the factory that is accessed within the factory's closure [2][6]. To resolve this issue, you have two primary options: 1. Use vi.hoisted(): This allows you to define variables that are also hoisted to the top of the file, making them available for use within the vi.mock factory [2][6][7]. 2. Use vi.doMock(): This method is not hoisted, allowing you to reference standard top-level variables. However, because it is not hoisted, it only affects subsequent dynamic imports, meaning you must be careful about the order of your imports [2][3].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/v4.1.4/docs/api/vi.md
- 2: https://vitest.dev/api/vi
- 3: https://blog.logrocket.com/advanced-guide-vitest-testing-mocking/
- 4: https://github.com/vitest-dev/vitest/blob/1a4705da/docs/guide/mocking.md
- 5: https://stackoverflow.com/questions/78144648/error-vitest-there-was-an-error-when-mocking-a-module-if-you-are-using-vi-m
- 6: https://mergify.com/blog/vitest-vimock-hoisting-traps/
- 7: https://www.getbook.com/en/book/vitest-3/api/vi
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File: console/web/src/pages/Traces/api/traces.test.ts (lines 1-60) =="
nl -ba console/web/src/pages/Traces/api/traces.test.ts | sed -n '1,60p'
echo
echo "== Look up Vitest version =="
if [ -f package.json ]; then
node -p "require('./package.json').devDependencies?.vitest || require('./package.json').dependencies?.vitest || 'no-vitest-in-root-package.json'"
else
echo "no root package.json"
fi
# Also check for any workspace/package.json that might define vitest
echo
echo "== Any package.json containing vitest in repo =="
fd package.json . -x sh -c 'f="$1"; if rg -n "\"vitest\"" "$f" >/dev/null 2>&1; then echo "$f"; rg -n "\"vitest\"" "$f" | head -n 5; fi' shRepository: iii-hq/workers
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="console/web/src/pages/Traces/api/traces.test.ts"
echo "== Inspect: $FILE =="
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE"
exit 1
fi
# Print with line numbers (no nl available)
awk '{printf "%5d:%s\n", NR, $0}' "$FILE" | sed -n '1,80p'
echo
echo "== Find Vitest version in repo (package.json files) =="
# List packages that mention vitest and print the resolved version field
for p in $(fd package.json .); do
if rg -n '"vitest"\s*:' "$p" >/dev/null 2>&1; then
echo "--- $p ---"
node -e "const p=require('path').resolve(process.argv[1]); const j=require(p); const v=(j.devDependencies&&j.devDependencies.vitest)|| (j.dependencies&&j.dependencies.vitest); console.log('vitest:', v||'(not found)')" "$p" || true
fi
doneRepository: iii-hq/workers
Length of output: 2487
🌐 Web query:
Vitest 4.1 vi.mock factory top-level variable name prefix "mock" escape hatch call mockCall vi.hoisted
💡 Result:
Vitest 4.1 behavior/answer: when you use vi.mock(path, factory), the vi.mock call is hoisted to the top of the file, so variables from outside the factory that you reference in the factory are not available the way you might expect. To safely create and access values used inside the mock factory, define them with vi.hoisted (and pass them through), or use vi.doMock if you truly need non-hoisted access. What you’re asking about (top-level variable name prefix “mock”, escape hatch “mockCall vi.hoisted”): - The documentation does not define any special variable name prefix (like “mock”) that affects hoisting. Hoisting is about where vi.mock is moved/executed relative to imports and module evaluation, not about naming conventions. Correct pattern (use vi.hoisted for shared values used by the vi.mock factory): - “The call to vi.mock is hoisted … It will always be executed before all imports. If you need to reference some variables outside of its scope, you can define them inside vi.hoisted and reference them inside vi.mock.” [1][2] - “This also means that you cannot use any variables inside the factory that are defined outside the factory.” [1][2] Why (mechanics): - vi.mock is hoisted “to top of the file” so it runs before imports resolve; because of that, closures/outer variables referenced by the factory may be in the temporal dead zone until initialized, or otherwise unavailable. The official guidance is to use vi.hoisted for values you need inside the factory. [1][3] If you need a non-hoisted escape hatch: - Use vi.doMock instead; it “isn’t hoisted … It works the same way but isn’t hoisted.” [1][2]
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/v4.1.4/docs/api/vi.md
- 2: https://vitest.dev/api/vi
- 3: https://mergify.com/blog/vitest-vimock-hoisting-traps/
🌐 Web query:
Vitest 4.1 syntax for vi.hoisted: const x = vi.hoisted(() => ...) example
💡 Result:
In Vitest 4.1, the vi.hoisted method is used to execute code before all static import statements and vi.mock calls [1][2]. Because static imports are hoisted to the top of the file in ES modules, standard code defined before them would normally execute after those imports [1][2]. vi.hoisted allows you to bypass this limitation—for example, to set up mocks or system configurations (like system time) that need to be in place before the module is imported [1][2][3]. The method returns the value returned by the factory function, which you can then use in your vi.mock factory to reference local variables [1][2]. ### Example Syntax Here is how to use vi.hoisted to create a mock function and reference it within a vi.mock call [1][2]: ts import { expect, vi } from 'vitest' import { originalMethod } from './path/to/module.js' // Define the mock using vi.hoisted const { mockedMethod } = vi.hoisted(() => { return { mockedMethod: vi.fn() } }) // Reference the hoisted mock inside vi.mock vi.mock('./path/to/module.js', () => { return { originalMethod: mockedMethod } }) // Use the mock in your tests mockedMethod.mockReturnValue(100) expect(originalMethod()).toBe(100) ### Key Considerations - Scope: vi.hoisted must be called at the top level of your module [4]. - Imports: You cannot access standard imported variables inside the vi.hoisted factory because they are not yet defined [1][2]. If you absolutely need to access another module, you must use a dynamic import() inside the factory [1][2]. - Asynchrony: vi.hoisted can be called asynchronously if needed, even if the environment does not support top-level await [1][2]. - Alternatives: It is generally discouraged to import modules inside vi.hoisted; if you need to perform initialization before tests run, consider executing that logic directly within the imported module itself [1][2].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/v4.1.4/docs/api/vi.md
- 2: https://vitest.dev/api/vi
- 3: Introduce
vi.hoistedto run code before imports andvi.mockvitest-dev/vitest#3228 - 4: https://github.com/vitest-dev/vitest/releases/tag/v4.1.0
Fix Vitest vi.mock hoisting issue in traces.test.ts
vi.mock is hoisted above const call = vi.fn(), so the mock factory closes over call before it’s initialized, which can cause Vitest’s hoisting/TZ errors. Create call with vi.hoisted so it’s available inside the factory.
🐛 Proposed fix
-const call = vi.fn()
+const call = vi.hoisted(() => vi.fn())
vi.mock('`@/lib/iii-client`', () => ({
getIiiClient: vi.fn(async () => ({ call })),
}))📝 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.
| const call = vi.fn() | |
| vi.mock('@/lib/iii-client', () => ({ | |
| getIiiClient: vi.fn(async () => ({ call })), | |
| })) | |
| const call = vi.hoisted(() => vi.fn()) | |
| vi.mock('`@/lib/iii-client`', () => ({ | |
| getIiiClient: vi.fn(async () => ({ call })), | |
| })) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@console/web/src/pages/Traces/api/traces.test.ts` around lines 8 - 11, The
test's vi.mock factory is hoisted before `const call = vi.fn()` so it closes
over an uninitialized `call`; fix by declaring `call` using `vi.hoisted` (e.g.
`const call = vi.hoisted(() => vi.fn())`) so the mock factory in the
`getIiiClient` mock can access the initialized `call`; update the
`traces.test.ts` file so `call` is created with `vi.hoisted` and the existing
mock for `getIiiClient` continues to return an object that references `call`.
| } else { | ||
| setTraceListItems([]) | ||
| setHasOtelConfigured(false) | ||
| // Reset the dedup state so a later non-empty fetch is detected as | ||
| // fresh (otherwise the fingerprint/new-trace diff would be stale). | ||
| fingerprintRef.current = '' | ||
| prevTraceIdsRef.current = new Set() | ||
| } |
There was a problem hiding this comment.
Clear pending traces when spans become empty.
When the fetch returns no spans, pendingTracesRef is not cleared. If the user was hovering during a previous non-empty fetch, those stale pending traces will be flushed when the user stops hovering, potentially showing outdated data that contradicts the latest empty response.
Additionally, setHasOtelConfigured(true) in flushPendingTraces (line 132) is redundant since the useEffect at line 93 already sets the correct value based on exporterDisabled.
Proposed fix
} else {
setTraceListItems([])
// Reset the dedup state so a later non-empty fetch is detected as
// fresh (otherwise the fingerprint/new-trace diff would be stale).
fingerprintRef.current = ''
prevTraceIdsRef.current = new Set()
+ pendingTracesRef.current = null
}And in flushPendingTraces:
const flushPendingTraces = () => {
if (pendingTracesRef.current) {
setTraceListItems(pendingTracesRef.current)
- setHasOtelConfigured(true)
pendingTracesRef.current = null
}
}📝 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.
| } else { | |
| setTraceListItems([]) | |
| setHasOtelConfigured(false) | |
| // Reset the dedup state so a later non-empty fetch is detected as | |
| // fresh (otherwise the fingerprint/new-trace diff would be stale). | |
| fingerprintRef.current = '' | |
| prevTraceIdsRef.current = new Set() | |
| } | |
| } else { | |
| setTraceListItems([]) | |
| // Reset the dedup state so a later non-empty fetch is detected as | |
| // fresh (otherwise the fingerprint/new-trace diff would be stale). | |
| fingerprintRef.current = '' | |
| prevTraceIdsRef.current = new Set() | |
| pendingTracesRef.current = null | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@console/web/src/pages/Traces/hooks/useTraceData.ts` around lines 120 - 126,
When a fetch returns no spans the code clears traceList items and fingerprintRef
but fails to clear pendingTracesRef, allowing stale pending traces to be flushed
later; update the empty-spans branch to also clear pendingTracesRef.current
(e.g., set to new Map() or empty array consistent with its type) and remove the
redundant setHasOtelConfigured(true) call inside flushPendingTraces since
useEffect already sets hasOtelConfigured based on exporterDisabled; change
references to pendingTracesRef and flushPendingTraces accordingly so the
hover/flush path no longer displays outdated traces.
1 participant
Summary
Replace the Traces page's 3-second polling with a push signal driven by harness activity, plus a round of Traces-page improvements. Observability-quality / UX change — no agent behavior change.
Push-based live refresh (replaces polling)
harness/src/harness/fanout/traces-changed.ts) subscribes to theagent::turn_endstream and, coalesced over ~400ms, pushes an emptyui::traces::changedsignal to all-sessions subscribers (reusing the existingFanoutState+ui::subscribe, withfunction_not_foundeviction).useTracesLiveRefreshhook (console/web/src/lib/devtools-stream.ts) that invalidates the['traces']/['traceGroups']query caches on the signal, and re-subscribes + re-syncs on WS reconnect and on tab-visible.useTraceData/useTraceGroupsdrop their 3srefetchInterval— freshness now comes from the push signal, connect-resync, and the manual Refresh button.Traces-page improvements
Extracted, unit-tested helpers (
spanTree,treeFlatten,minimapMarkers,percent,attributeText, plustraceListItem/traceTransformupdates), component refinements (FlameGraph,WaterfallChart,SpanOtelLogsTab,SpanErrorsTab, group/detail panels), and auto-pause of live updates while a detail panel is open (restoring the prior pause intent on close, since polling is no longer the throttle).Behavior / compatibility
Test plan
pnpm test, 479 tests) +pnpm typecheck.pnpm typecheck.engine::traces::listevery 3s.Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes