Repository navigation
feat(claude-plugin): draw ClickStack query results as terminal charts - #3400
jordan-simonovski wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: e3db061 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
PR Review
5 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 5 minor None could be anchored to changed lines; they are listed below. Findings outside the changed lines5 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
 [Medium impact] The PR appears safe to merge, with the earlier non-blocking chart-label issue still open.
 |
E2E Test Results✅ All tests passed • 440 passed • 1 skipped • 1441s
Tests ran across 4 shards in parallel. |
Deep Review✅ No critical issues found. The two prior P1 review items are confirmed resolved in the current diff: 🟡 P2 — recommended
🔵 P3 nitpicks (9)
Reviewers (9): correctness, testing, maintainability, project-standards, security, reliability, adversarial, kieran-typescript, previous-comments. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
Address review feedback on #3400: - Follow clickstack_table turning multi-select number, pie and bar into a table, so no metric is dropped. - Stop inferring granularity from bucket gaps; empty buckets are filled only for an explicit granularity on an untrimmed result, so sparse or misaligned series no longer gain invented zeros. - Mark results the server trimmed with a note under the chart. - Take calledAt before the tool runs, as the server fixes its window then. - Ship hand-written hooks/chart.d.ts types (the package has no src/), held in step with the source by a compile-time test. - Reuse common-utils' time-series helpers and key mapped list elements.
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
|
Replies to the review findings outside the changed lines, addressed in afac3ed unless noted:
Trimmed results (those with the server's
Fixed; see the inline thread.
Inference is removed. Empty buckets are filled only for an explicit granularity on an untrimmed result.
It's now taken before
The time-series map now reuses
Now uses
Both lists are keyed.
Added tests for the multi-select upgrade and its single-select boundary, fill rules for inferred, trimmed and explicit granularity, the trimmed note line, and the |
| } | ||
| return ran; | ||
| }).catch(($, e, next) => next(e)); | ||
|
|
There was a problem hiding this comment.
🔵 minor — The tool.call .catch handler calls next(e) again, so a call that rejects can run twice
If await next(e) rejects (for example a transport error from the MCP server), the hook throws and .catch(($, e, next) => next(e)) runs the same ClickStack query a second time. The plugin's own bookkeeping already has a try/catch, so drop the outer .catch. If the API requires one, make it rethrow or return the original failure instead of calling next again.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
|
|
||
| const DEFAULT_WINDOW_MS = 15 * 60 * 1000; | ||
| // clickstack_table turns these into a table when given more than one select item. | ||
| const SINGLE_SERIES_SHAPES = ['number', 'pie', 'bar']; |
There was a problem hiding this comment.
🔵 minor — The server's shape auto-upgrade rule is copied by hand, giving it a second source of truth
SINGLE_SERIES_SHAPES and the select.length > 1 rewrite repeat the upgrade logic in packages/api/src/mcp/tools/query/table.ts:266-274. If the server rule changes, the plugin draws a chart type the server never returned. Have the tool response report the display type it actually used (for example a displayType field next to result) and read that. Otherwise, export the shape list and upgrade rule from one shared module and use it in both places.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| select, | ||
| groupBy: args.groupBy, | ||
| } as unknown as ChartConfigWithDateRange; | ||
|
|
There was a problem hiding this comment.
🔵 minor — as unknown as ChartConfigWithDateRange turns off type checking on the config handed to the renderer
This breaks the base-branch rule "Avoid as casts, use satisfies or type inference". It also hides that select holds raw MCP items, not internal select items, while renderTileContent passes them to isRatioChartConfig and resolveChartNumberFormats (packages/cli/src/shared/tileRender.ts:84,131). Build the object against a narrow Pick<…> of the fields the renderer reads, using satisfies, so a shape mismatch fails to compile.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| // Empty buckets are filled only when their alignment is known: an explicit | ||
| // granularity and every row present. Otherwise filling would invent zeros. | ||
| fillNulls: args.granularity && !isTrimmed(output) ? undefined : false, | ||
| select, |
There was a problem hiding this comment.
🔵 minor — The args.granularity half of the fillNulls condition never changes the output, and granularity is not inferred as the PR says
formatResponseForTimeChart already skips empty-bucket filling when granularity == null (packages/cli/src/shared/chartData.ts:359), so forcing fillNulls: false for a missing granularity does nothing. Reduce the condition to isTrimmed(output) ? false : undefined. Also, the PR description says granularity is "inferred from bucket spacing" when omitted, but no code does that. Either implement it or correct the description.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| ]; | ||
| // Built from a string so the ESC character isn't a literal control char in a regex. | ||
| const SGR = new RegExp(`${String.fromCharCode(27)}\\[([0-9;]*)m`, 'g'); | ||
|
|
There was a problem hiding this comment.
🔵 minor — The SGR regex and color-code table duplicate termchart's ANSI helpers
packages/cli/src/termchart/ansi.ts already exports ANSI_PATTERN (the same \x1b\[[0-9;]*m match, used by stripAnsi) and PLOT_COLORS (color name → SGR code). The base-branch DRY rule is marked REQUIRED. Export the pattern from termchart and derive the code → name map from PLOT_COLORS/chalk names, so the parser and the emitter can't drift apart.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| <Box flexDirection="column" key={`chart-${row.id}`}> | ||
| <Text> | ||
| <Text bold>{stripControlChars(row.tool.split('__').pop() ?? '')}</Text> | ||
| {typeof reason === 'string' && ( |
There was a problem hiding this comment.
🔵 minor — The header re-derives the tool suffix that chartedTool already computes
row.tool.split('__').pop() repeats the suffix logic in chartedTool (src/toTileResult.ts). Use chartedTool(row.tool) so the label and the matching rule always agree.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| isInterrupted: false, | ||
| }, | ||
| ], | ||
| isActive: false, |
There was a problem hiding this comment.
🔵 minor — No test covers a group with charted and uncharted calls, an expanded group, or eviction after 20 stored calls
The engine tests only mount single-call groups. Add cases for: a ToolGroup with one clickstack call and one other call (the engine row plus the chart should both render); isExpanded: true (should fall through to next); and the 21st call evicting the oldest stored chart. These are the branches in drawRows and the KEEP slice, and they also run outside CI (claude plugin test).
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
When Claude Code calls clickstack_timeseries or clickstack_table, the tool row now shows the result as a line, stacked bar, bar, pie, number or table chart instead of raw JSON, drawn by the hdx CLI's renderers. Claude still reads the full result. One /plugin install hyperdx-charts --marketplace hyperdxio/hyperdx asks for the HyperDX URL and a personal API access key (kept in secure storage) and connects the ClickStack MCP server, so there is no separate claude mcp add step. Chart text is stripped of control characters and bidi overrides, since labels carry user data that must not drive the terminal.
Address review feedback on #3400: - Follow clickstack_table turning multi-select number, pie and bar into a table, so no metric is dropped. - Stop inferring granularity from bucket gaps; empty buckets are filled only for an explicit granularity on an untrimmed result, so sparse or misaligned series no longer gain invented zeros. - Mark results the server trimmed with a note under the chart. - Take calledAt before the tool runs, as the server fixes its window then. - Ship hand-written hooks/chart.d.ts types (the package has no src/), held in step with the source by a compile-time test. - Reuse common-utils' time-series helpers and key mapped list elements.
afac3ed to
15cb6ae
Compare
| }, | ||
| "devDependencies": { | ||
| "@hyperdx/cli": "workspace:*", | ||
| "@hyperdx/common-utils": "^0.30.0", |
There was a problem hiding this comment.
🟠 major — @hyperdx/common-utils: ^0.30.0 doesn't match the version in the repo (0.31.0), and yarn.lock has no entry for it
Change the range to ^0.31.0 (the same range as packages/cli, api and app) and run yarn install again. On a 0.x version a caret range is pinned to its minor version, so ^0.30.0 will not accept 0.31.0 and yarn cannot link it to the workspace copy. yarn.lock only has @hyperdx/common-utils@npm:^0.31.0, @hyperdx/common-utils@workspace:packages/common-utils (around line 4988) and nothing for npm:^0.30.0, so yarn install --immutable fails. If install is allowed to update the lockfile, it pulls the published 0.30.x from npm instead, and jest/tsc then run against that stale copy while tsup bundles the workspace source.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| return ansi.split('\n').map(line => { | ||
| const segments: Segment[] = []; | ||
| let last = 0; | ||
| const push = (raw: string) => { | ||
| const text = stripControlChars(raw); |
There was a problem hiding this comment.
ansiLines cleans user text too late. A table cell or service name containing \n has already become an extra chart row when stripControlChars runs. Embedded color codes are also read as chart styles instead of removed. This breaks table rows and legends or changes their colors.
Clean user strings before passing them to renderTileContent, while preserving the renderer’s own line breaks and colors.
main moved @hyperdx/common-utils to 0.31.0, so yarn install could not resolve ^0.30.0.
Summary
Adds
@hyperdx/claude-plugin, a Claude Code plugin that shows ClickStack MCP results as terminal charts. Today aclickstack_timeseriesorclickstack_tablecall shows up in Claude Code as collapsed JSON, so the person in the session never sees the data Claude is reasoning about. With the plugin, that row becomes a line, stacked bar, bar, pie, number or table chart, drawn by the same renderer ashdx. Claude still reads the full result./plugin install hyperdx-charts --marketplace hyperdxio/hyperdx, asks for the HyperDX URL and a personal API access key and connects the ClickStack MCP server, so there is no separateclaude mcp addstep. The key is kept in Claude Code's secure storage..claude-plugin/marketplace.json, which points at the npm package. Changesets publishes it with the other packages.Key decisions
_meta, but Claude Code only passes it to plugins behind a feature flag that is off. The plugin rebuilds the chart from the tool's input (shape, granularity, time range) and its result JSON, which is everythingrenderTileContentneeds. A spec in_metawould be cleaner once that flag ships.tool.call. Claude Code draws MCP calls as tool groups that don't carry the result, so the plugin keeps the last 20 charted results in session state and draws from there.hooks/register.tsxitself and refuses hooks modules over 1 MiB. The renderer is bundled intohooks/chart.js, which is about 280 KB because the build reads common-utils' ESM source and drops unused code. Built from the CommonJS dist it was 5.5 MB.Background
Claude Code plugins ("mods") are hooks modules loaded into a session. A
ui.renderhook can replace how a transcript row is drawn, anduserConfigfields inplugin.jsonare prompted for at install and substituted into the bundled MCP server config. The plugin API is marked early access and may change between releases.Impact
@hyperdx/claude-plugin, published on the next release (0.1.0 via the changeset).MCP.mdpoints Claude Code users at the plugin as an alternative toclaude mcp add.How to reproduce
yarn workspace @hyperdx/claude-plugin buildclaude --plugin-dir packages/claude-plugin, then enter your HyperDX URL and personal API access key when asked.clickstack_timeseriesrow shows a line chart instead of JSON.How to test on Vercel preview
N/A, non-UI change.
References
Implementation detail
src/toTileResult.tsmaps the tool input and MCP result onto the CLI'sTileQueryResult: shape to display type, the requested window (or the server's 15-minute default), and granularity, inferred from bucket spacing when the call omitted it. Without a known bucket size it turns off zero-filling so it doesn't invent buckets.src/ansiLines.tsparses termchart's ANSI output into styledTextsegments. termchart runs at chalk level 1 here, so only basic SGR codes need handling.mcp__plugin_hyperdx-charts_clickstack__*names and a hand-addedmcp__clickstack__*both chart.src/__tests__/*.spec.tsrun in CI. Engine tests intest/*.test.tsxrun withclaude plugin test, since they need Claude Code's test kit.claude plugin validatechecks the manifest and hooks module. The hooks module and engine tests are typed against API declarations that Claude Code writes at load time, so they are excluded from the repo's tsc and eslint.