Conversation
|
Bosun CI signal: Bosun-created PR currently has failing checks.
|
There was a problem hiding this comment.
Pull request overview
Adds a new Telemetry screen to both Ink-based TUIs (the ui/tui app and the tui/ CLI app), including sparklines, provider/token/cost aggregation, a 429 “heatmap”, and funnel stats, plus default telemetry cost config and tests.
Changes:
- Introduce telemetry aggregation/rendering helpers and a richer Telemetry UI for
ui/tui. - Add a new
telemetryscreen to the CLI TUI (tui/) with its own model/helpers. - Add default config for telemetry token-cost rates and new vitest coverage for both implementations.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/tui/telemetry-helpers.js | New helper module for sparklines + snapshot aggregation (providers/funnel/429 events/cost). |
| ui/tui/TelemetryScreen.js | New telemetry UI panel layout (throughput, provider table, 429 heatmap, funnel, cost estimates). |
| ui/tui/App.js | Wire the telemetry tab to the new TelemetryScreen component. |
| tui/screens/telemetry.mjs | New CLI TUI telemetry screen rendering using a derived model. |
| tui/screens/telemetry-screen-helpers.mjs | New CLI TUI telemetry model helpers (rates, provider rows, heatmap, funnel, sparklines). |
| tui/lib/navigation.mjs | Add telemetry to navigation order/input mapping. |
| tui/app.mjs | Register telemetry screen + add “Telemetry” tab to CLI TUI. |
| tests/tui-telemetry-ui.test.mjs | New tests for ui/tui telemetry helper behaviors. |
| tests/tui-telemetry-screen.test.mjs | New tests for CLI TUI telemetry model + navigation update. |
| config/config.mjs | Add config.telemetry.costPer1kTokensUsd defaults. |
Comments suppressed due to low confidence (1)
ui/tui/App.js:299
- There are extra trailing blank lines at the end of the file. Please remove them to keep diffs clean.
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "yellow", | ||
| "red", | ||
| ); | ||
| const dailyCostUsd = history.reduce((sum, item) => sum + item.sessionCostUsd, 0); |
There was a problem hiding this comment.
dailyCostUsd is computed by summing sessionCostUsd across the snapshot history, which will over-count the same sessions on every refresh (and the history window is rolling, not a day). This makes the displayed "Day Cost" misleading/incorrect. Derive a day/cumulative cost from backend-provided day totals, or change this to a windowed metric (e.g., compute deltas between consecutive snapshots) and rename the label accordingly.
| const dailyCostUsd = history.reduce((sum, item) => sum + item.sessionCostUsd, 0); | |
| const dailyCostUsd = Math.max( | |
| 0, | |
| (latest.sessionCostUsd || 0) - (history[0]?.sessionCostUsd || 0), | |
| ); |
| const navItems = [ | ||
| { key: "status", num: "1", label: "Status" }, | ||
| { key: "tasks", num: "2", label: "Tasks" }, | ||
| { key: "agents", num: "3", label: "Agents" }, | ||
| { key: "agents", num: "3", label: "Agents" },`r`n { key: "telemetry", num: "4", label: "Telemetry" }, | ||
| ]; |
There was a problem hiding this comment.
The navItems array has stray `r`n characters between entries, which will cause a syntax error at runtime. Remove the stray characters and add the telemetry tab item as its own element in the array.
| const base = Math.max(1, counts.todo); | ||
| return { | ||
| stages: FUNNEL_ORDER.map((stage) => ({ | ||
| ...stage, | ||
| count: counts[stage.key], | ||
| conversionPct: Math.round((counts[stage.key] / base) * 100), | ||
| })), |
There was a problem hiding this comment.
buildTaskFunnel forces base to at least 1. If todo is 0 but later stages are non-zero (e.g., due to partial telemetry), conversionPct can become >100%, which is misleading. Consider using base = counts.todo and returning 0% when base is 0, or base the conversion on total task count when todo is 0.
| ["1", "status"], | ||
| ["2", "tasks"], | ||
| ["3", "agents"], | ||
| ["3", "agents"],`r`n ["4", "telemetry"], |
There was a problem hiding this comment.
The array literal contains stray characters `r`n (looks like an accidental CRLF escape) inside the Map initializer, which makes this module invalid JS and will break navigation + tests. Remove the stray characters and put the ["4", "telemetry"] entry on its own line like the other entries.
| ["3", "agents"],`r`n ["4", "telemetry"], | |
| ["3", "agents"], | |
| ["4", "telemetry"], |
| import AgentsScreen from "./screens/agents.mjs"; | ||
| import StatusScreen from "./screens/status.mjs"; | ||
| import StatusScreen from "./screens/status.mjs";`r`nimport TelemetryScreen from "./screens/telemetry.mjs"; | ||
| import { readTuiHeaderConfig } from "./lib/header-config.mjs"; |
There was a problem hiding this comment.
This import line includes stray `r`n characters, which makes the file fail to parse. Remove the stray characters and keep the TelemetryScreen import as a normal separate import statement.
| const SCREENS = { | ||
| status: StatusScreen, | ||
| tasks: TasksScreen, | ||
| agents: AgentsScreen, | ||
| agents: AgentsScreen,`r`n telemetry: TelemetryScreen, | ||
| }; |
There was a problem hiding this comment.
The SCREENS object literal contains stray `r`n characters before the telemetry entry, which makes the object literal invalid JS. Remove the stray characters and add the telemetry screen as a normal key in the object.
| telemetry: Object.freeze({ | ||
| costPer1kTokensUsd: Object.freeze({ | ||
| claude: Number.isFinite(Number(configData.telemetry?.costPer1kTokensUsd?.claude)) ? Number(configData.telemetry.costPer1kTokensUsd.claude) : 0.003, | ||
| codex: Number.isFinite(Number(configData.telemetry?.costPer1kTokensUsd?.codex)) ? Number(configData.telemetry.costPer1kTokensUsd.codex) : 0.002, | ||
| gemini: Number.isFinite(Number(configData.telemetry?.costPer1kTokensUsd?.gemini)) ? Number(configData.telemetry.costPer1kTokensUsd.gemini) : 0.0001, |
There was a problem hiding this comment.
triggerSystem is no longer included in the returned config object (it was replaced by the new telemetry block). triggerSystem is referenced in infra/server code and config-validation tests, so omitting it will break consumers. Add triggerSystem back to the exported config (alongside telemetry, not instead of it).
|
|
||
| function trimHistory(history, length = HISTORY_SECONDS) { |
There was a problem hiding this comment.
HISTORY_SECONDS is used as a sample count in trimHistory, but snapshots are collected every 5s. With the current constants, keeping 60 entries actually retains ~5 minutes of history, not 60 seconds. Consider renaming this to HISTORY_SAMPLES or computing the max samples from HISTORY_SECONDS and REFRESH_MS (e.g., ceil(HISTORY_SECONDS*1000/REFRESH_MS)).
| function trimHistory(history, length = HISTORY_SECONDS) { | |
| const MAX_HISTORY_SAMPLES = Math.ceil((HISTORY_SECONDS * 1000) / REFRESH_MS); | |
| function trimHistory(history, length = MAX_HISTORY_SAMPLES) { |
…cii-sparklines-provi
|
Bosun PR classification: Bosun-created.
|
Co-authored-by: bosun-ve[bot] <262908237+bosun-ve[bot]@users.noreply.github.com>
Co-authored-by: bosun-ve[bot] <262908237+bosun-ve[bot]@users.noreply.github.com>
…t-tui-telemetry-screen-ascii-sparklines-provi Co-authored-by: bosun-ve[bot] <262908237+bosun-ve[bot]@users.noreply.github.com>
Task-ID: 3cc669ae-c7e8-4ceb-982f-37ab5be16659\n\nAutomated PR for task 3cc669ae-c7e8-4ceb-982f-37ab5be16659