Repository navigation
fix(dashboards): preserve multi-line SQL when replaying a search - #3382
Conversation
🦋 Changeset detectedLatest commit: 66bb252 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
[Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryDashboard replay and drill-down links now use the same encoding helpers as the search page.
|
E2E Test Results✅ All tests passed • 438 passed • 1 skipped • 1363s
Tests ran across 4 shards in parallel. |
a19312d to
dd431a8
Compare
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
dd431a8 to
563561b
Compare
PR Review
1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor None could be anchored to changed lines; they are listed below. Findings outside the changed lines1 minor
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([ |
There was a problem hiding this comment.
🔵 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
Deep ReviewScope: ✅ No critical issues found. The core change is correct. The round-trip was traced end to end: 🔵 P3 nitpicks (4)
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:
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 |
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:
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:
On this branch:
How to test on Vercel preview
Preview routes: /dashboards
Steps:
References