fix(app): sort builder tables by output identifiers - #2981
Conversation
🦋 Changeset detectedLatest commit: 3aa6af4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
@fallintoplace is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThe PR fixes builder-table sorting by treating returned column names as quoted ClickHouse output identifiers and updates persisted sorting-state parsing to support quoted names containing spaces.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/app/src/components/DBTableChart.tsx | Builder columns now use escaped, quoted output identifiers while raw SQL and PromQL columns remain unchanged. |
| packages/app/src/utils.ts | The order-by parser now restores quoted identifiers containing spaces while retaining validation for unquoted identifiers. |
| packages/app/src/components/tests/DBTableChart.test.tsx | Adds typed fixtures and regression coverage for builder output identifiers, escaping, and generated server-side ordering. |
| packages/app/src/tests/utils.test.ts | Adds parsing and round-trip coverage for quoted sorting identifiers containing spaces. |
Reviews (2): Last reviewed commit: "fix(app): preserve quoted table sort sta..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Builder sorting runs against the final result, so every returned key | ||
| // must be treated as an output identifier rather than an expression. | ||
| id: isBuilderChartConfig(queriedConfig) |
There was a problem hiding this comment.
When a builder output name contains spaces, such as error rate, its quoted ID is serialized as "error rate" DESC, but the existing parser splits on every space and rejects the resulting three tokens. After the chart remounts or reloads, the persisted ordering is not restored and the sort indicator and selected ordering are lost.
| direction: 'DESC' as const, | ||
| }, | ||
| }, | ||
| fetchNextPage: jest.fn(), |
There was a problem hiding this comment.
The two added query mocks use as any, so changes to the query result contract will not be checked against these fixtures. Please give these mock return values their proper type so missing or malformed fields remain visible to TypeScript.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Deep Review✅ No critical issues found. The core change is sound: builder-table sort IDs are now quoted ClickHouse identifiers, escaping is applied backslash-first then double-quote-doubling ( 🔵 P3 nitpicks (3)
Reviewers (7): correctness, security, testing, maintainability, kieran-typescript, project-standards, previous-comments. Testing gaps: No integration-level test that a persisted quoted builder sort (spaces/special chars) round-trips through the ChartPreviewPanel order-by string back onto the matching column id. |
…gate (#3024) ## Summary `claude-code-review.yml` has been disabled since **2026-05-20**, so this is a rebuild rather than a tweak. It replaces the prompt, moves it into a versioned file, and changes how findings reach the PR. **The prompt was selected by measurement, not taste.** I built an offline harness that scores reviewer prompts against a frozen dataset of **122 hyperdx PRs where a human left a substantive inline review comment**, using those comments as ground truth. On the held-out test split (Opus, 49 PRs / 87 gold items): | prompt | recall | cost/PR | agents | | --- | --- | --- | --- | | lifted-budget variant of the old prompt | 31% | $2.86 | 1 | | **this prompt** | **40%** | **$3.11** | **1** | | `deep-review` (currently in production) | ~22–33% | — | 6–13 | On the larger shared slice this prompt and `deep-review` are within noise of each other, so **the claim is parity-or-better at one agent instead of six-to-thirteen — not a 4× improvement.** I'd rather understate that than have it quoted back later. ### What changes for reviewers - **Findings post as inline comments on the changed lines.** 82% of findings anchor to a diff line (measured); the rest reference files the diff never touches and fall back to the sticky summary rather than being dropped — that class is a large share of the useful output. - **One sticky summary**, updated in place, with counts and anything unanchored. - **Repeat comments are suppressed across pushes** via a hidden per-finding fingerprint keyed on path + normalized title, so a reworded body doesn't repost. - **Minor findings are folded behind `<details>`, not filtered.** On the dataset, minor-severity findings carry **29–57% of everything a human independently flagged** — filtering them would cost roughly half the recall. Severity is used for ordering only. ### Cost and correctness controls - **Review gate.** Hashes the effective diff (`merge-base..HEAD`) and the prompt; skips when both are unchanged. `synchronize` fires on every "Update branch" merge, which advances the merge-base while leaving the diff byte-identical — without this, a large share of runs are pure waste at ~$3–5/PR. Same idea as `deep-review.yml`. - **Fail-open.** The state marker is only stamped when the run produced parseable output; otherwise the next push retries instead of the gate caching a zero-finding review against that diff forever. - **`concurrency` + cancel-in-progress**, so two quick pushes can't race the sticky comment or pay twice. - **Model pinned to `opus`.** This prompt gained ~13 points from Opus where the old one gained ~5 — the extra passes only pay off at that tier, and an action-default change shouldn't silently swap the reviewer. ### Tool grants Follows the pattern already set in `deep-review.yml`: a read-only `git`/`gh` prefix allowlist. Withheld deliberately, since this job checks out fork-authored code and holds secrets: - **`gh api`** — accepts `--method POST`, and prefix allowlists can't constrain flags (your own note in `deep-review.yml`). Prior review threads are materialized in trusted shell instead, fenced and capped, and the prompt instructs the reviewer not to re-report them. - **bare `Bash`** — would be arbitrary code execution on PR-author-controlled build scripts next to `ANTHROPIC_API_KEY`. - **`WebFetch`/`WebSearch`/`curl`/`wget`** — data egress from a job holding secrets. ### Testing `.github/scripts/review-comments.cjs` holds the comment-routing logic so it's testable without triggering a PR event. 11 tests, run by this workflow before the review step (same pattern as `pr-triage.yml`). They pin the two silent failure modes — a wrong line map, and a fingerprint that reposts — plus the fail-open contract, so nobody "tidies" the marker format and quietly disables the gate. ### Dry run on live PRs Ran against #2992, #2981, #2980 (none in the eval set) without posting: 17 findings, 14 inline, 3 in summary, $10.86. Full output on request. The standout was a `critical` on #2981 that neither `greptile` nor `deep-review` reported — quoting every builder column makes `ORDER BY "count()"` unresolvable for unaliased projections. ## Merging this does not turn it on The workflow is `disabled_manually`; merging the file doesn't change that. Enabling is a separate, deliberate step: ``` gh api -X PUT repos/hyperdxio/hyperdx/actions/workflows/claude-code-review.yml/enable ``` I'd suggest enabling it **alongside** `deep-review` for a couple of weeks (two comments per PR, distinct markers) and comparing on live PRs before deciding whether this replaces it. If it does, expect a net spend *decrease* — one agent instead of 6–13. ## Known gaps, stated plainly - **Two changes here are unevaluated.** The harness measured the prompt with the diff *inlined* and with `Read/Grep/Glob` only. CI reads the diff from a file, and grants read-only `git`/`gh` plus materialized prior comments. Those should help, but the 40% figure doesn't cover them. Prior comments in particular are *unevaluable* in the harness — on the eval PRs those comments **are** the gold set. - **Cost may run above $3.11/PR.** The dry run averaged $3.62 on mid-sized PRs. If the bill looks wrong, the natural lever is gating on the existing `review/tier-N` labels and only running this on tier-3/4. - **One measured gap.** On the dry-run PRs, both `greptile` and `deep-review` independently caught failures involving *data persisted before the change* (stale saved sort strings) and this prompt caught neither. It has no pass for that class. That's the next thing I'd add. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
2 participants
What changed
Builder table sorting now uses the returned column name as a quoted ClickHouse identifier.
This fixes sorting for:
avg(metric.total)Also:
Why
The composed metric query sorts in the outer result scope. The table was sending generated names as expressions, so ClickHouse could not resolve them.
The sort state parser also split on every space, so a quoted name like
"error rate" DESCwas lost after reload.Tests
yarn workspace @hyperdx/app jest --runInBand src/__tests__/utils.test.ts src/components/__tests__/DBTableChart.test.tsxyarn workspace @hyperdx/app tsc --noEmityarn lint:fix