Skip to content

feat: Support source name deeplinks on additional pages - #2758

Merged
kodiakhq[bot] merged 5 commits into
mainfrom
drew/extend-source-name-param-support
Jul 31, 2026
Merged

feat: Support source name deeplinks on additional pages#2758
kodiakhq[bot] merged 5 commits into
mainfrom
drew/extend-source-name-param-support

Conversation

@pulpdrew

@pulpdrew pulpdrew commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR extends the functionality added in #2746 (accepting source names in the source URL param) to additional pages: chart explorer, service map, services dashboard, K8s dashboard, sessions).

To prevent render loops when syncing between form state and URL state (in particular when navigating away from a page), I've upgrade React to get access to useEffectEvent. The syncing effects on the changed pages are now triggered on either param changes or form changes, not both.

Screenshots or video

Screen.Recording.2026-07-31.at.8.10.47.AM.mov

How to test on Vercel preview

Try navigating to the following pages using source names for their source-related URL params:

  • chart explorer (config.source)
  • service map (source)
  • services dashboard (source)
  • K8s dashboard (logSource, metricSource)
  • sessions (sessionSource)

References

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

@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Jul 31, 2026 6:42pm
hyperdx-storybook Ready Ready Preview Jul 31, 2026 6:42pm

Request Review

@changeset-bot

changeset-bot Bot commented Jul 30, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5f40fb6

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

@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Extends source-name deep links across additional observability pages.

  • Resolves source names to source IDs for chart explorer, service map, services, Kubernetes, and sessions pages.
  • Revises form-to-URL synchronization to canonicalize resolved IDs while avoiding navigation-time update loops.
  • Upgrades React type definitions for useEffectEvent and adds unit and end-to-end coverage.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/app/src/DBChartPage.tsx Resolves the source embedded in chart configuration before passing it to the chart editor.
packages/app/src/DBServiceMapPage.tsx Adds trace-source name resolution and guarded URL canonicalization for the service map.
packages/app/src/KubernetesDashboardPage.tsx Resolves log and metric source names before correlation and distinguishes form catch-up from user selection.
packages/app/src/ServicesDashboardPage/ServicesDashboardPage.tsx Resolves trace-source names and revises synchronization between effective configuration, form state, and URL parameters.
packages/app/src/SessionsPage.tsx Resolves session-source names and synchronizes the selected source ID back to the URL.
packages/app/tests/e2e/features/source-name-deeplink.spec.ts Adds end-to-end coverage for source-name deep links, canonicalization, missing sources, and cross-page transitions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  URL[Source name in URL] --> Resolver[Resolve source by name or ID]
  Resolver --> ID[Canonical source ID]
  ID --> Form[Page form state]
  Form --> Query[Dashboard or chart query]
  ID --> CanonicalURL[Canonicalized URL parameter]
Loading

Reviews (4): Last reviewed commit: "Merge branch 'main' into drew/extend-sou..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 266 passed • 1 skipped • 1084s

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

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew
pulpdrew force-pushed the drew/extend-source-name-param-support branch from a0de344 to 93bc47d Compare July 31, 2026 12:47
@pulpdrew
pulpdrew marked this pull request as ready for review July 31, 2026 12:52
@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Jul 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 290 production lines changed (Tier 2 max: < 250)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 8
  • Production lines changed: 290 (+ 483 in test files, excluded from tier calculation)
  • Branch: drew/extend-source-name-param-support
  • Author: pulpdrew

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

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

Scope note: git, grep, and network access were all unavailable in this environment (bwrap sandbox-init failure affecting every shell, including sub-agents). The reviewers therefore worked from the current file state at 93bc47d5 rather than a git diff against e231d72e. Findings below are anchored to lines that were read directly; attribution of a line to this PR versus pre-existing code is inferred from code comments and commit subjects, so the two pre-existing? judgements marked inline are less certain than the code facts.

🔴 P0/P1 — must fix

  • packages/app/src/ServicesDashboardPage/ServicesDashboardPage.tsx:228 — The guard if (sourceId && sourceId !== appliedConfigParams.source) return; blocks every URL→form sync once the form holds a value, so an externally-changed ?source= moves the charts but never the picker, and the two never reconverge.
    • Fix: Track the last value this effect itself wrote (via a ref) instead of comparing sourceId against the raw param, or drive the field with useForm({ values: … }) as DBServiceMapPage.tsx:123 does, so an external param change always wins.
    • julik-frontend-races, adversarial

Verified by direct read: mounted at ?source=B, the user picks C (auto-submit writes ?source=C), then presses Back to ?source=B. effectiveSourceId becomes B while sourceId stays C; the second guard returns because C !== B, so setValue never runs and syncSourceParam no-ops because effectiveSource === appliedConfigParams.source. appliedConfig.source (B) feeds the HTTP/Database/Errors tabs while sourceId (C) feeds SourceSelectControlled, ServiceSelectControlled, useSource, usePresetDashboardFilters, and both side panels. No notification fires, because the param resolved cleanly.

🟡 P2 — recommended

  • packages/app/src/utils/sourceParams.ts:66paramValue.toLowerCase() runs after only a == null || === '' check, so a non-string value throws a TypeError during render and blanks the page.

    • Fix: Replace the emptiness check at line 63 with if (typeof paramValue !== 'string' || paramValue === '') return { status: 'empty' };.
    • adversarial, security

    Reachable only from DBChartPage.tsx:232, which feeds rawChartConfig.source straight in; config is parsed by nuqs parseAsJson<SavedChartConfig>() (nuqs pinned 1.17.0), which does a bare JSON.parse with no runtime validator. The five parseAsString call sites can only yield string | null and are safe. The throw is deferred until useSources() settles, because sources == null short-circuits to pending first.

  • packages/app/src/hooks/useResolvedSourceParam.ts:65 — An unresolved param triggers a one-time toast, then the page falls back to a default source and the sync effect overwrites the param with that default's ID, so a reload resolves cleanly and the warning never reappears.

    • Fix: Expose the resolution status from the hook and gate each page's sync effect on status === 'resolved' so a failed value stays in the URL, and reword the message to state which source is being shown instead.
    • correctness, adversarial, agent-native

    Chain: resolveSourceParam returns not-foundDBServiceMapPage.tsx:121 (paramSource ?? defaultSource), ServicesDashboardPage.tsx:197, or KubernetesDashboardPage.tsx:1069 substitutes a default → the sync effect at DBServiceMapPage.tsx:145 / ServicesDashboardPage.tsx:267 / KubernetesDashboardPage.tsx:1102 writes it over the param. The message says "pick a source to continue" though a source has already been picked and rendered. SessionsPage and DBSearchPage preserve the param instead, so the five pages disagree.

  • packages/app/src/ServicesDashboardPage/ServicesDashboardPage.tsx:165resolveSourceParam resolves disabled sources but getEffectiveTraceSourceId filters !s.disabled, so a disabled source reports resolved (no warning), then gets silently replaced by an unrelated source whose ID is written to the URL.

    • Fix: Align the two policies — either exclude disabled sources from name resolution in sourceParams.ts:96 so the hook reports not-found, or accept an explicitly-named disabled source here as DBServiceMapPage.tsx:121 does.
    • adversarial, correctness

    This is the one fallback path with zero user-visible signal: useResolvedSourceParam.ts:46 only warns on not-found/wrong-kind, and a resolved-but-disabled source is neither.

  • packages/app/src/utils/sourceParams.ts:63 — The param is never trimmed, so a hand-authored link with a trailing space fails all three comparisons and falls into the silent-default-swap path.

    • Fix: Normalize once at the top with const trimmed = paramValue.trim(), return empty when it is blank, and match ID and both name comparisons against trimmed.
    • correctness, adversarial

    Names are matched with === and toLowerCase() only (lines 71, 74–81), so case is tolerated but whitespace is not — and hand-authored links are exactly where stray padding appears.

  • packages/app/src/DBServiceMapPage.tsx:145 — No unmount or route-change guard remains after useRouteChangeState was dropped; useEffectEvent addresses stale closures, not navigation timing, so the effect can still call setSourceId during an in-flight route transition.

    • Fix: Add a routeChangeStart-driven ref flag and early-return from each useEffectEvent callback while it is set, covering DBServiceMapPage.tsx:140, ServicesDashboardPage.tsx:262, and SessionsPage.tsx:282.
    • julik-frontend-races, correctness

    The suspected chain — nuqs reports the destination route's absent source while the outgoing page is still mounted → paramSource undefined → source falls back to the default → watchedSource changes → the effect writes ?source=<default> onto the new URL — is code-verifiable up to the nuqs/Next transition-batching step, which could not be confirmed here. Graded P2 rather than P1 for that reason.

  • packages/app/src/ServicesDashboardPage/ServicesDashboardPage.tsx:209useForm uses defaultValues (captured once at mount) with nothing resyncing where/service/whereLanguage, while onSubmit writes the whole form snapshot, so an unrelated source or service change resurrects a filter the user navigated away from.

    • Fix: Switch this form to values: so param changes flow back in, or narrow onSubmit at line 293 to write only the field that actually changed.
    • adversarial
  • packages/app/src/DBServiceMapPage.tsx:140 — The "only write a truthy, changed source" invariant is reimplemented four different ways across the migrated pages, two with useEffectEvent and two with hand-rolled refs.

    • Fix: Extract one useSyncSourceParam(formValue, paramValue, setParam) hook and call it from DBServiceMapPage.tsx:140, SessionsPage.tsx:282, ServicesDashboardPage.tsx:262, and KubernetesDashboardPage.tsx:1102.
    • maintainability, kieran-typescript
  • packages/app/src/SessionsPage.tsx:282resolveSourceParam and useResolvedSourceParam are well unit-tested, but none of the five per-page sync effects — the behavioural change itself — has any test.

    • Fix: Add render-level tests asserting the param is canonicalized exactly once, that nothing is written while sources is still undefined, and that no write occurs during a route change.
    • testing, maintainability

    packages/app/src/__tests__/KubernetesDashboardPage.test.ts and ServicesDashboardPage.test.ts exist but cover only the extracted pure helpers (resolveSourceIds, getEffectiveTraceSourceId, buildInFilterCondition); there is no test file for SessionsPage, DBServiceMapPage, or DBChartPage.

🔵 P3 nitpicks (9)
  • packages/app/src/utils/sourceParams.ts:106ambiguousMatchCount reports pool.length, which excludes disabled same-named sources, so the toast states a number lower than the sources actually sharing the name.

    • Fix: Report nameMatches.length, or reword the message to say "usable sources".
  • packages/app/src/KubernetesDashboardPage.tsx:1061 — Two hook instances with different kinds share one message template, so a swapped logSource/metricSource yields "is a metric source, which this page can't show" on a page that is charting metrics; identical unresolved values on both params also collapse to one toast via the shared notification id.

    • Fix: Accept a param label in the hook and namespace both the message and the notification id by param name.
  • packages/app/src/SessionsPage.tsx:293 — The auto-select effect is keyed on the raw appliedConfig.sessionSource rather than the resolved source, so an unresolvable param leaves it non-empty, no source is ever selected, and the first-run setup instructions render even though session sources exist.

    • Fix: Key the guard on the resolved source (if (sources?.length && !paramSource)).
  • packages/app/src/DBChartPage.tsx:236paramSource?.id ?? '' collapses pending and not-found, so config.source is blank for the whole source-list loading window.

    • Fix: Surface status from the hook and return rawChartConfig unchanged while it is pending.
  • packages/app/src/DBChartPage.tsx:232 — This is the only migrated page that never writes the resolved ID back into its URL param, so a name-based chart link keeps the name indefinitely.

    • Fix: Either add the write-back or document next to the memo why this page deliberately resolves in-memory only.
  • packages/app/src/hooks/useResolvedSourceParam.ts:22 — The Extract<TSource, { kind: K }> overload is not proven by the implementation, which is generic only over resolveSourceParam's own T; the narrowing holds only because of a runtime filter in another file.

    • Fix: Make resolveSourceParam generic in K and return Extract<T, { kind: K }>, or note the manual invariant at the overload.
  • packages/app/src/hooks/useResolvedSourceParam.ts:38 — The five-member SourceParamResolution union is consumed by independent status === … ternaries, so a new variant would compile and silently resolve to "nothing to show".

    • Fix: Replace the ternaries with a switch ending in an assertNever(resolution) default.
  • packages/app/src/hooks/useResolvedSourceParam.ts:64 — Resolution failure is signalled only by an auto-dismissing toast while the page renders a fully populated fallback, which is indistinguishable from success to a headless or screenshot-driven consumer.

    • Fix: Add a durable element with a stable data-testid alongside the toast.
  • packages/app/src/utils/sourceParams.ts:71 — An ID match returns immediately without checking whether another source matches the same string by name, so that collision alone produces no ambiguity signal.

    • Fix: When matchedById hits, also look for exact name matches and set ambiguousMatchCount so the user is told the value was read as an ID.

Reviewers (9): correctness, julik-frontend-races, adversarial, kieran-typescript, testing, maintainability, security, project-standards, agent-native.

Testing gaps:

  • No test asserts what happens to the URL after an unresolved param — the five pages currently disagree (service map / services dashboard / K8s overwrite it; sessions and search preserve it), so whichever behaviour is intended is unverified on at least three pages.
  • No test resolves a disabled source by name or ID; resolveSourceParam accepts it, getEffectiveTraceSourceId rejects it, and DBServiceMapPage renders it — three behaviours for one input.
  • No test covers a non-string config.source reaching resolveSourceParam through parseAsJson.
  • No test drives an external ?source= change after mount (client-side nav or Back), which is the only way to reproduce the P1.
  • No test covers the K8s page with logSource/metricSource swapped, or whitespace-padded / "undefined" / "null" param values.

Cleared / not reported: security found no XSS (params reach only Mantine text children, no HTML sink), no authorization bypass (resolution is in-memory equality against the already team-scoped useSources() response), no query injection, and no exploitable notification-id hijack. A sub-agent flagged possible useEffectEvent unavailability — dismissed: packages/app/package.json:80 pins react: ^19.2.3, which cannot resolve below the version that ships it, and eslint-plugin-react-hooks: ^7.0.1 understands the hook. Two pre-existing 300-line file-size violations were dropped as noise.

Unverified: AGENTS.md requires a changeset for user-facing @hyperdx/app changes; whether one was added could not be checked, since listing .changeset/ requires the unavailable shell.

@pulpdrew

Copy link
Copy Markdown
Contributor Author

packages/app/src/ServicesDashboardPage/ServicesDashboardPage.tsx:228 — The guard if (sourceId && sourceId !== appliedConfigParams.source) return; blocks every URL→form sync once the form holds a value, so an externally-changed ?source= moves the charts but never the picker, and the two never reconverge.

This is not something that's reachable in practice since nothing on the page changes the URL directly. Back/Forward navigation and manual address box changes still trigger a change in the form state.

@pulpdrew
pulpdrew requested review from a team, fleon and teeohhem and removed request for a team and fleon July 31, 2026 13:29

@teeohhem teeohhem left a comment

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.

Tested and works as expected!

@kodiakhq
kodiakhq Bot merged commit cacdfe9 into main Jul 31, 2026
27 checks passed
@kodiakhq
kodiakhq Bot deleted the drew/extend-source-name-param-support branch July 31, 2026 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants