Skip to content

fix(dashboards): preserve multi-line SQL when replaying a search - #3382

Merged
kodiakhq[bot] merged 2 commits into
mainfrom
cursor/fix-replay-search-multiline-sql-5161
Oct 19, 2026
Merged

kodiakhq[bot] merged 2 commits into
mainfrom
cursor/fix-replay-search-multiline-sql-5161

Conversation

@pulpdrew

@pulpdrew pulpdrew commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Multi-line SQL in a dashboard's "Replay search" or drill-down link lost its line breaks as soon as the search page updated its URL, for example on a time-range change. A query like ServiceName = 'x' and + newline + SeverityText = 'error' became ...andSeverityText... and failed with a ClickHouse syntax error.

Cause: buildEventsSearchUrl built the /search URL with a plain URLSearchParams, which encodes values once. The search page's where, select and filters parsers (parseAsStringEncoded / parseAsJsonEncoded) expect two encoding layers. With only one, the first URL write on the search page re-rendered where with a raw newline. nuqs doesn't escape newlines, so the browser stripped it, and nuqs then re-read the joined value into state. The same missing layer also corrupted literal %XX text in values (a%20b was read as a b).

Fix: buildEventsSearchUrl now encodes where, select and filters with the same serializer the parsers use. It lives in a new nuqs-free module, utils/urlParamEncoding.ts, which queryParsers.ts now uses too. This fixes the four dashboard and chart links that use this builder:

  • tile Replay search
  • the time chart View All Events drill-down
  • table row click-through
  • the heatmap event-deltas link

Old single-encoded links still parse. Other /search link builders (generateSearchUrl, copyRowUrl, service map, LLM dashboard) have the same problem and will be handled in a follow-up.

How to reproduce

Starting point: play.hyperdx.io

On main:

  1. Create a dashboard and add a line tile on a logs source.
  2. Set the where language to SQL and enter ServiceName != '' AND, then press Shift+Enter, then SeverityText != ''. The line break must be the only whitespace between AND and SeverityText. Save the tile.
  3. Hover the tile and click Replay search. Results load.
  4. Change the time range, e.g. to "Last 1 days".
  5. The where box shows a single line (...ANDSeverityText...) and the search fails with a syntax error.

On this branch:

  1. Repeat steps 1–4. The where box keeps both lines and results load.

How to test on Vercel preview

Preview routes: /dashboards

Steps:

  1. Create a new dashboard and add a line tile on the logs source.
  2. Set the where language to SQL and enter ServiceName != '' AND, press Shift+Enter, then enter SeverityText != ''. Save the tile.
  3. Hover the tile and click Replay search. In the new tab, wait for results.
  4. Open the time picker and select "Last 1 days".
  5. Verify the where editor shows two lines and the results table has rows, with no error.

References

  • Linear Issue: Closes HDX-5726
  • Related PRs:

@changeset-bot

changeset-bot Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 66bb252

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

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

@vercel

vercel Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
hyperdx-oss Ready Ready Preview Oct 19, 2026 2:47pm UTC
hyperdx-storybook Ready Ready Preview Oct 19, 2026 2:47pm UTC

Request Review

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Duplicate code analysis

No new duplicate code introduced by this PR.


jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 125 in total (1.69%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

@greptile-apps

greptile-apps Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

Dashboard replay and drill-down links now use the same encoding helpers as the search page.

  • Dashboard search links keep SQL line breaks after URL updates.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Dashboard query] --> B[Shared encoding helpers]
  B --> C[URLSearchParams adds outer encoding]
  C --> D[Search page reads URL]
  D --> E[Shared parsers remove inner encoding]
  E --> F[Original query and filters]
Loading

Reviews (6) · Last reviewed commit: "Merge branch 'main' into cursor/fix-repl..." · Reviewed by Greptile

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 438 passed • 1 skipped • 1363s

Status Count
✅ Passed 438
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 3
  • Production lines changed: 60 (+ 195 in test files, excluded from tier calculation)
  • Branch: cursor/fix-replay-search-multiline-sql-5161
  • Author: pulpdrew

To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@pulpdrew
pulpdrew force-pushed the cursor/fix-replay-search-multiline-sql-5161 branch from dd431a8 to 563561b Compare October 16, 2026 13:55
@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

  • 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
  • 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
  • Anything about surrounding code, or asking you to widen the change (hoist a helper,
    dedupe with another file, fix other call sites): never fix here, whatever the
    severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor

None could be anchored to changed lines; they are listed below.

Findings outside the changed lines

1 minor
  • 🔵 packages/app/src/components/TraceLogsPanel.tsx:130 — The new encoder still has an inline copy in TraceLogsPanel → TraceLogsPanel builds the same /search link and adds the parser's inner layer by hand with select: encodeURIComponent(...) and where: encodeURIComponent(traceWhere). This PR adds encodeUrlParamValue in utils/urlParamEncoding.ts to be the single serializer for these params, so switch these two lines to encodeUrlParamValue. Then a future change to the inner encoding updates every link builder, not just buildEventsSearchUrl and queryParsers.ts.

Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.

expect(url.searchParams.get('where')).toBe('service:web');
expect(JSON.parse(url.searchParams.get('filters') ?? '')).toEqual([
expect(readWhere(url)).toBe('service:web');
expect(readFilters(url)).toEqual([

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Tests copy the parser's decode by hand instead of running the real parsers

readWhere/readFilters here and the searchParams helper in eventsSearchUrl.test.ts re-implement the decode as decodeURIComponent/JSON.parse. If parseAsStringEncoded/parseAsJsonEncoded (queryParsers.ts) change their inner layer, these tests keep passing while real links break. Decode with parseAsStringEncoded.parse(...) and parseAsJsonEncoded<Filter[]>().parse(...) (or decodeUrlParamValue) so each test checks that the builder and the parser actually 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

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Deep Review

Scope: packages/app URL-param encoding fix — buildEventsSearchUrl now double-encodes where/select/filters via a new nuqs-free module utils/urlParamEncoding.ts shared with queryParsers.ts, so dashboard replay/drill-down links survive a /search-page URL rewrite without losing newlines. Base: 8b92b7ac.

✅ No critical issues found.

The core change is correct. The round-trip was traced end to end: encodeURIComponent (inner layer) + URLSearchParams.toString() (outer layer) matches exactly what parseAsStringEncoded/parseAsJsonEncoded decode on the /search page (DBSearchPage.tsx:902-906). Every call site of both builders (ChartUtils.tsx:1552, DBTimeChart.tsx:845, DBDashboardPage.tsx:290,920) navigates to /search, which reads these three params through the double-decoding parsers — no consumer single-decodes them, so nothing is corrupted. The queryParsers.ts extraction is behavior-preserving (byte-for-byte equivalent delegation), the metric-chart branch keeps select encoded consistently, and old single-encoded links are not newly regressed.

🔵 P3 nitpicks (4)
  • packages/app/src/utils/urlParamEncoding.ts:1 — the extracted module that now backs every encoded query param has no direct unit test; it is covered only transitively through queryParsers and the builder tests.
    • Fix: Add a small test asserting encode/decode symmetry and the decodeUrlParamValue malformed-URI fallback (raw-value return on a bad % sequence).
    • kieran-typescript, correctness
  • packages/app/src/ChartUtils.tsx:1388 — no test round-trips a value containing a literal percent sign (e.g. a SQL LIKE '%foo%' clause), which is the value class most sensitive to an encoding-layer mismatch; existing tests use only newlines/tabs.
    • Fix: Add a round-trip case whose where contains a literal % and assert it decodes back unchanged.
  • packages/app/src/ChartUtils.tsx:1404 — the metric-chart path (isMetricChart reset block followed by the eventTableSelect encode) has no test asserting select still survives when both apply.
    • Fix: Add a metric-chart case with eventTableSelect set and assert the recovered select.
  • packages/app/src/ChartUtils.tsx:1388 — sibling /search link builders (generateSearchUrl, copyRowUrl, service map, LLM dashboard) still single-encode and remain inconsistent with the parsers; a future edit could keep encode and decode out of lockstep.
    • Fix: Track the stated follow-up to converge those builders onto the shared urlParamEncoding helpers.

Reviewers (6): correctness, kieran-typescript, previous-comments, learnings-researcher, plus orchestrator-side adversarial and maintainability verification (call-site trace, backward-compat walk-through, changeset check).

Testing gaps:

  • New urlParamEncoding module is exercised only transitively — no direct unit test.
  • No round-trip coverage for literal % values or the metric-chart + eventTableSelect interaction.

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

@pulpdrew
pulpdrew requested a review from knudtty October 16, 2026 14:27
@kodiakhq
kodiakhq Bot merged commit e3f24b9 into main Oct 19, 2026
29 checks passed
@kodiakhq
kodiakhq Bot deleted the cursor/fix-replay-search-multiline-sql-5161 branch October 19, 2026 14:53

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — 66bb2529 Deployed Oct 19, 2026 by vercel[bot]
Preview – hyperdx-storybook — 66bb2529 Deployed Oct 19, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants