Skip to content

feat: Keep service map sampling factor in local storage - #3388

Merged
kodiakhq[bot] merged 1 commit into
mainfrom
drew/local-storage-sampling-factor
Oct 16, 2026
Merged

kodiakhq[bot] merged 1 commit into
mainfrom
drew/local-storage-sampling-factor

Conversation

@pulpdrew

Copy link
Copy Markdown
Contributor

Summary

Store the service map sampling rate in local storage, so that a user's selection persists on subsequent service map visits. The URL value still overrides the saved sampling rate, so deeplinks still keep their sampling rate.

Screenshots or video

How to reproduce

How to test on Vercel preview

  1. Visit the service map
  2. Set a non-default sampling rate
  3. Visit the page again without a sampling rate in the URL, notice that the saved rate is applied
  4. Visit the page with an explicit sampling rate in the URL, notice that it's applied

References

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

@vercel

vercel Bot commented Oct 16, 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 16, 2026 3:03pm UTC
hyperdx-storybook Ready Ready Preview Oct 16, 2026 3:03pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Oct 16, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7ad6c7e

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

@pulpdrew
pulpdrew marked this pull request as ready for review October 16, 2026 15:03
@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Oct 16, 2026
@github-actions

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: 1
  • Production lines changed: 30 (+ 53 in test files, excluded from tier calculation)
  • Branch: drew/local-storage-sampling-factor
  • 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.

@github-actions

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.70%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

Comment thread packages/app/src/DBServiceMapPage.tsx
@github-actions

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.

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

1 posted as inline comment(s) on the changed lines. 1 listed below.

Findings outside the changed lines

1 minor
  • 🔵 packages/app/tests/e2e/features/service-map-sampling.spec.ts:123 — The E2E spec never tests the new fallback for an invalid stored value → Add a case that seeds hdx-service-map-sampling-factor with a value outside the allowed set (e.g. JSON.stringify(3)) and asserts the label reads 'Sampling 10%'. Without it, someone could remove the SAMPLING_FACTORS.some(...) check in DBServiceMapPage.tsx and both tests would still pass.

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

@greptile-apps

greptile-apps Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] Service map page remembers sampling preference in local storage.

The PR appears safe to merge, with a non-blocking issue when sharing a map whose sampling rate came from storage.

Fix All in Claude CodeFindings

  1. P2 Shared links lose restored rates ▶

Summary

Remembers service-map sampling choices across visits.

  • The service map remembers your sampling choice between visits.

Reviews (1) · Last reviewed commit: "feat: Keep service map sampling factor i..." · Reviewed by Greptile

Comment thread packages/app/src/DBServiceMapPage.tsx
@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. The change is small, well-targeted, and the core intent (URL param wins, only slider changes persist) is correctly implemented and covered by the two new e2e cases. One recommended test gap and a few nits below.

🟡 P2 -- recommended

  • packages/app/tests/e2e/features/service-map-sampling.spec.ts:41 -- The new validation guard (SAMPLING_FACTORS.some(...) ? storedSamplingFactor : DEFAULT_SAMPLING_FACTOR) that falls back to the default on a corrupt or out-of-set stored value is never exercised; the spec only seeds the valid value 2.
    • Fix: Add a case that seeds an invalid stored value (e.g. localStorage.setItem(KEY, JSON.stringify(7))) and asserts the label renders Sampling 10%.
    • testing, correctness, maintainability, kieran-typescript
🔵 P3 nitpicks (3)
  • packages/app/src/DBServiceMapPage.tsx:228 -- The stored sampling factor is validated against SAMPLING_FACTORS, but the URL samplingFactorParam wins via ?? with no equivalent membership check, so a hand-crafted link like ?samplingFactor=7 yields an empty label and a slider index of -1 (below min={0}); this URL-path behavior largely predates the PR but the new guard makes the asymmetry worth closing.
    • Fix: Run both the URL param and the stored value through one shared membership check that falls back to DEFAULT_SAMPLING_FACTOR.
    • correctness, maintainability, kieran-typescript
  • packages/app/src/DBServiceMapPage.tsx:228 -- On a statically pre-rendered first paint the label and slider show DEFAULT_SAMPLING_FACTOR, then flip to the persisted value once useLocalStorage reads storage on hydration, causing a brief flash for users with a non-default saved rate.
    • Fix: Gate the sampling-dependent UI on a mounted flag so it renders the default until after hydration, consistent with how other useLocalStorage-derived UI is handled.
    • julik-frontend-races
  • packages/app/src/DBServiceMapPage.tsx:230 -- The factor.value === x lookup over SAMPLING_FACTORS is reimplemented three times (.some for validation, .find for the label, .findIndex for the slider), so editing the options list means keeping three call sites in sync.
    • Fix: Derive a single lookup helper from SAMPLING_FACTORS and reuse it for validation, label, and slider index.

Reviewers (9): correctness, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, agent-native, learnings-researcher, previous-comments.

Testing gaps:

  • Out-of-set URL samplingFactor param (empty label, slider index -1) is untested.
  • SSR/hydration with a non-default stored value (flash / mismatch) is untested.
  • Cross-tab useLocalStorage sync of the preference is untested.

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

@github-actions

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 428 passed • 1 skipped • 1633s

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

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew
pulpdrew requested review from a team and brandon-pereira and removed request for a team October 16, 2026 15:49
@kodiakhq
kodiakhq Bot merged commit 3ff761b into main Oct 16, 2026
40 of 42 checks passed
@kodiakhq
kodiakhq Bot deleted the drew/local-storage-sampling-factor branch October 16, 2026 16:13

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — 7ad6c7e2 Deployed Oct 16, 2026 by vercel[bot]
Preview – hyperdx-storybook — 7ad6c7e2 Deployed Oct 16, 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