feat(app): make View Trace action more prominent with a one-time nudge

[elizabetdev]

Summary

Users reported that the log side panel's View Trace action is easy to miss. Since traces moved out of a dedicated tab, the action rendered as low-emphasis inline text (variant="subtle", compact-xs) blending into the dimmed metadata row (timestamp, service, Trace ID).

This PR makes it noticeable and adds a gentle one-time nudge:

• Prominent button (ViewTraceCalloutButton): outlined (secondary) button, larger (compact-sm), right-aligned (ml="auto"), using the trace source icon (IconConnection, matching SOURCE_KIND_ICONS) plus a trailing arrow. Same data-testid / navigation behavior, so E2E helpers are unchanged.
• One-time popover pointing at the button the first time a user opens a log that has a correlated trace:
  • Dismissed only by an explicit acknowledgement — clicking Got it or the View Trace button — which persists under its own dedicated localStorage key (shared constant VIEW_TRACE_CALLOUT_DISMISSED_KEY), deliberately not in hdx-user-preferences (that's for real user settings).
  • Stays pinned otherwise: stray clicks and drawer resizes do not dismiss it, so an accidental interaction can't burn the message before it's read.
  • Does not intercept Escape — Escape keeps its normal side-panel behavior (owned by the panel-level hotkey), so the callout never fights it for the keypress. Nothing transient is stored, so the nudge simply reappears next time the panel opens on an eligible log until acknowledged.
  • Gated on the trace being resolved (same condition as the button's enabled state) so it never points at a disabled button.
  • role="status" / aria-live="polite" so screen readers announce it when it auto-opens.
• data-source-icons skill: documents the canonical Tabler icon per SourceKind (log/trace/session/metric/promql) to keep icon usage consistent going forward.
• E2E base-test.ts seeds the dismissed flag (via the shared key) so the auto-popover never interferes with side-panel specs.

Why

The subtle inline treatment didn't read as an action to users coming from the old Trace tab or the previous "View trace" link. Emphasizing the button (without reintroducing tab/push navigation inconsistency) plus a self-retiring, placement-neutral nudge addresses discoverability with minimal behavioral change.

Screenshot

Review feedback addressed

• P0/P1 — Escape ownership: the callout no longer sets closeOnEscape, so it can't collide with the pre-existing useHotkeys(['esc'], handlePanelBack); Escape behaves exactly as before.
• P1 — transient state leak: removed the transient viewTraceCalloutClosed state entirely, so a dismissal can no longer persist across row changes in the same panel instance.
• P2/P3 — structure & coverage: extracted the inline popover into a dedicated ViewTraceCalloutButton component (keeps DBRowSidePanel focused, makes the nudge testable); added a unit test suite (opened gate, already-dismissed, and both persist paths); shared the localStorage key via a dependency-free constant imported by both the component and the E2E fixture; added role/aria-live for screen readers; and used placement-neutral copy so brand-new users don't get a change announcement for a placement they never saw.

Test plan

• yarn workspace @hyperdx/app run ci:unit — DBRowSidePanel.* + new ViewTraceCalloutButton suites pass
• yarn lint:fix clean; pre-commit hooks (prettier/eslint/knip) pass
• E2E suite green on CI (side-panel navigation unaffected; callout seeded off)
• Manual: open a log with a trace → popover appears once trace resolves; Got it / View Trace dismiss permanently (hdx-view-trace-callout-dismissed: true); Escape performs normal panel back/close; resizing the drawer does not dismiss
• Manual: a log without trace context shows no button and no popover

[changeset-bot]

🦋 Changeset detected

Latest commit: 0a6d4bc

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]

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

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview	Aug 6, 2026 4:26pm
hyperdx-storybook	Ready	Preview	Aug 6, 2026 4:26pm

[github-actions]

🔵 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: 120 (+ 87 in test files, excluded from tier calculation)
• Branch: elizabet/prominent-view-trace-action
• Author: elizabetdev

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

[greptile-apps]

Greptile Summary

The PR makes the log side panel’s View Trace action more prominent and adds a persistent, one-time explanatory callout.

• Extracts the action and callout into ViewTraceCalloutButton.
• Persists explicit acknowledgement in localStorage through a shared key.
• Seeds the dismissed flag in the common E2E fixture to prevent interference with existing side-panel tests.
• Documents the canonical source-kind icon mapping and adds focused component tests.

Confidence Score: 4/5

The PR should not merge until Escape no longer navigates away from or closes the side panel while the callout is open.

The transient row-state leak has been removed, but the callout still opts out of Escape handling while the unchanged panel hotkey consumes Escape to pop navigation or close the panel, leaving the previously reported interaction defect outstanding.

Files Needing Attention: packages/app/src/components/ViewTraceCalloutButton.tsx, packages/app/src/components/DBRowSidePanel.tsx

Important Files Changed

Filename	Overview
packages/app/src/components/ViewTraceCalloutButton.tsx	Introduces the prominent trace button and persistent callout, but the previously reported Escape interaction remains reachable.
packages/app/src/components/DBRowSidePanel.tsx	Replaces the inline trace action with the extracted component while retaining the panel-level Escape navigation handler.
packages/app/src/components/tests/ViewTraceCalloutButton.test.tsx	Covers visibility gates and both explicit persistent-dismissal paths.
packages/app/tests/e2e/utils/base-test.ts	Seeds the shared dismissal key for E2E tests using a serializable init-script argument.

_{Reviews (10): Last reviewed commit: "fix(app): avoid unsafe type assertion in..." | Re-trigger Greptile}

[github-actions]

E2E Test Results

✅ All tests passed • 275 passed • 1 skipped • 1068s

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

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

Deep Review

🔴 P0/P1 — must fix

• packages/app/src/components/DBRowSidePanel.tsx:900 — The nudge's only non-committing dismissal is Escape, but the pre-existing document-level useHotkeys(['esc'], handlePanelBack) at line 379 fires on the same keypress, so Escape closes the whole drawer at the root of the trail (popOne() === 'none' → onClose()) or silently rewinds one navigation level while the callout stays open.
  • Fix: Give the callout ownership of Escape by gating the panel hotkey while the popover is open (useHotkeys(['esc'], handlePanelBack, { enabled: !calloutOpen })) and setting the transient flag from that branch instead of relying on closeOnEscape.
  • _{correctness, julik-frontend-races, adversarial, maintainability}

🟡 P2 — recommended

• packages/app/src/components/DBRowSidePanel.tsx:432 — viewTraceCalloutClosed is plain useState inside the Mantine Drawer, which has no keepMounted (line 1258), so the transient hide is destroyed on every close; combined with closeOnClickOutside={false} and no neutral close control in the dropdown, the only paths that clear the nudge are the two that commit, and it re-opens on every trace-linked log row indefinitely.
  • Fix: Add an explicit close affordance to Popover.Dropdown and lift the transient flag above the Drawer (or into sessionStorage) so a "not now" survives a close/reopen cycle.
  • _{adversarial, correctness, maintainability}
• packages/app/src/components/DBRowSidePanel.tsx:890 — The 260px position="bottom-end" dropdown is portalled at z-index 300 over a drawer at z-index 10 and opens itself only once the async useSource({id: traceSourceId}) query resolves, landing over DBRowSidePanelHeader and TabBar after the body has already rendered, where closeOnClickOutside={false} means a swallowed click neither reaches the covered control nor dismisses the card.
  • Fix: Anchor the dropdown so it cannot overlap the header/tab controls, or open it only on deliberate hover/focus of the button rather than on query resolution.
  • _{adversarial, agent-native, julik-frontend-races}
• packages/app/tests/e2e/utils/base-test.ts:44 — The shared test.extend({ page }) fixture seeds hdx-view-trace-callout-dismissed for every spec, and no unit test for this component exists, so the feature's central behavior — the four-condition opened gate, the transient-vs-persisted dismissal split, and the two independent persist paths — has zero automated coverage at any tier.
  • Fix: Add a unit test asserting the opened gate, "Got it"/View-Trace persistence, and Escape-is-transient, plus one E2E spec that clears the seed and exercises the nudge.
  • _{testing, project-standards, correctness, maintainability, kieran-typescript}
• packages/app/src/components/DBRowSidePanel.tsx:431 — The key 'hdx-view-trace-callout-dismissed' is an inline literal re-spelled independently in packages/app/tests/e2e/utils/base-test.ts:45, departing from the repo's own convention (STORAGE_KEY in useUserPreferences.tsx, QUERY_LOCAL_STORAGE in utils.ts, LAST_TAB_STORAGE_KEY in useSidePanelStack.ts), so a rename in either copy silently un-seeds the E2E suppression and re-arms the auto-popover across the side-panel specs.
  • Fix: Export a VIEW_TRACE_CALLOUT_DISMISSED_KEY constant and import it in both the component and the E2E fixture.
  • _{kieran-typescript, maintainability, agent-native, adversarial}
• packages/app/src/components/DBRowSidePanel.tsx:940 — Popover.Dropdown carries only data-testid with no role/aria-live/aria-describedby, and with trapFocus={false} plus default portalling and no focus move on open, a keyboard-only user has no deterministic tab path to "Got it" — the only control that permanently dismisses the nudge.
  • Fix: Add role="status"/aria-live="polite" (or role="dialog" with aria-labelledby) to the dropdown and move focus into it when it auto-opens.
  • _agent-native
• .changeset/ — No changeset accompanies this user-facing behavior change to @hyperdx/app, which AGENTS.md requires for any behavior-affecting change to a published package and which .changeset/config.json confirms is release-managed.
  • Fix: Add a .changeset/*.md entry describing the View Trace prominence and nudge change.
  • _{project-standards}

🔵 P3 nitpicks (6)

• packages/app/src/components/DBRowSidePanel.tsx:897 — The comments at lines 425-428 and 908-909 describe "click-away / Escape" as the transient-hide trigger, but closeOnClickOutside={false} on line 899 makes the click-away half unreachable, so onChange can only ever fire from closeOnEscape.
  • Fix: Reword both comments to name Escape as the sole transient trigger and state that click-away is deliberately inert.
  • _{julik-frontend-races, kieran-typescript}
• packages/app/src/components/DBRowSidePanel.tsx:943 — "View trace is easier to find now 🎉" is gated only on the absence of the localStorage key, which is also the state of every brand-new user, fresh browser profile, and incognito session, so first-time users get a change announcement for a placement they never saw.
  • Fix: Use placement-neutral copy such as "Jump to this log's full trace".
  • _correctness
• packages/app/src/components/DBRowSidePanel.tsx:921 — leftSection={<IconConnection size={14} />} duplicates the canonical trace-source icon already declared as [SourceKind.Trace]: <IconConnection size={16} /> in packages/app/src/components/sourceSelectUtils.tsx:13, at a different size and with no link between them.
  • Fix: Consume SOURCE_KIND_ICONS[SourceKind.Trace] (parameterizing the registry for size) instead of a second inline icon literal.
  • _{maintainability}
• packages/app/src/components/DBRowSidePanel.tsx:433 — useLocalStorage.setValue performs localStorage.setItem inside the setStoredValue updater (utils.ts:223-227) and only console.logs a throw, so for users with blocked or full site storage the dismissal never persists and the unclosable nudge returns on every reload.
  • Fix: Write to localStorage before calling the state setter so the failure is observable, and treat a failed persist as a session-scoped dismissal.
  • _adversarial
• packages/app/src/components/DBRowSidePanel.tsx:935 — showLogTraceActions (line 788) needs only traceId && traceSourceId, while disabled also needs traceSpanRowId, which is undefined when the linked trace source resolves neither traceIdExpression nor spanIdExpression — so those rows now render a prominent permanently-dead button where a subtle inline link used to sit.
  • Fix: Hide the button entirely when traceSpanRowId cannot resolve rather than rendering it disabled.
  • _{adversarial, correctness}
• packages/app/src/components/DBRowSidePanel.tsx:889 — ~70 lines of presentational popover JSX plus its state are added inline to a 1303-line file whose DBRowSidePanelInner already spans ~950 lines, against the AGENTS.md guidance to keep component files under 300 lines.
  • Fix: Extract a ViewTraceCalloutButton component taking disabled, opened, onView, and onDismiss so the nudge is independently testable.
  • _{kieran-typescript, maintainability, project-standards}

---

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

Testing gaps:

• No unit test file exists for this component, so the opened gate, the two persist paths, and the transient-vs-permanent split are entirely unverified.
• The E2E fixture change suppresses the callout suite-wide, so no spec can observe it opening, dismissing, or intercepting clicks on the header and tab bar.
• No test pins the Escape interaction between the panel-level hotkey and any Mantine popup opened inside the panel — the regression test for the P1 above does not exist.

Coverage note: Bash, Grep, and Glob were non-functional in this environment (bwrap sandbox failure), so the diff could not be retrieved via git; review was reconstructed from direct file reads at the PR head. Two consequences: the changed-file set could not be enumerated (the changeset finding rests on the reviewed scope, not a directory listing), and the new .claude/skills/data-source-icons/SKILL.md is absent from the working tree and was not reviewed. Verified as non-issues and dropped: variant="secondary"/variant="primary" are first-class custom Button variants registered in both brand themes; clicking the portalled dropdown does not trip the drawer's outside-click close (FLOATING_LAYER_SELECTOR matches mantine-Popover-dropdown); the opened and disabled gates are exact complements within a render; and the popover is not z-index-buried by the drawer.

[github-actions]

<!-- deep-review -->

Deep Review

🔴 P0/P1 — must fix

• packages/app/src/components/ViewTraceCalloutButton.tsx:45 — The auto-opened 260px dropdown is portalled to <body> at Mantine's popover z-index while the drawer sits far below it, so it paints over DBRowSidePanelHeader and the TabBar and, with closeOnClickOutside={false}, silently swallows every click that lands on it until the user finds Got it.
  • Fix: Let an outside click both dismiss the callout and fall through to the element beneath, or anchor the hint beside the button as a non-overlaying element instead of a popover covering the header and tab bar.
  • _{correctness, adversarial, frontend-races}

🟡 P2 — recommended

• packages/app/src/components/ViewTraceCalloutButton.tsx:46 — With closeOnEscape={false} the keypress falls through to the pre-existing useHotkeys(['esc'], handlePanelBack) at DBRowSidePanel.tsx:372, which pops to onClose() at the root of a log panel, so Escape destroys the user's panel while leaving the callout unacknowledged and it reopens on the next log.
  • Fix: Handle Escape on the popover to run the dismiss path and stop propagation while the callout is open, so the panel-level back/close hotkey only fires once the callout is gone.
  • _{adversarial, correctness}
• packages/app/src/components/ViewTraceCalloutButton.tsx:44 — trapFocus={false} plus Mantine's default portalling places the dropdown at the end of <body>, so Tab from the trigger continues into the drawer and reaching Got it requires traversing every other focusable element on the page, leaving activating View Trace as the only keyboard dismissal.
  • Fix: Render the dropdown non-portalled or move focus into it on open so the acknowledge control is next in tab order, and give it keyboard-operable dialog semantics.
  • _{correctness, adversarial}
• packages/app/src/components/ViewTraceCalloutButton.tsx:47 — opened is derived directly from the async disabled prop, which flips only when the independent useSource query at DBRowSidePanel.tsx:498 resolves, so the callout can appear an arbitrary time after the panel became interactive and land on top of whatever the user is already doing.
  • Fix: Latch opening to a single stable moment — the first render where the button is enabled and the user has not yet interacted with the panel — rather than mirroring the async prop.
  • _{frontend-races, adversarial, correctness}
• packages/app/src/components/DBRowSidePanel.tsx:719 — DBRowSidePanelInner is re-rendered rather than remounted on a row change and the row query carries no placeholderData, so isRowLoading flips true, this early return tears down the header subtree, and the callout remounts and reopens on every row the user pages through inside one open panel.
  • Fix: Give the row query placeholderData: prev => prev so an in-place row change no longer unmounts the header subtree, or hold the shown-once decision above the component so a remount cannot retrigger it.
• packages/app/src/components/ViewTraceCalloutButton.tsx:36 — dismiss routes through useLocalStorage, whose setItem runs inside the setStoredValue updater; React swallows the eager invocation's error and rethrows during render, so a QuotaExceededError or blocked-storage SecurityError escapes as a render-phase throw into the panel's error boundary instead of a failed nudge.
  • Fix: Perform the storage write in the click handler inside its own try/catch and keep the closed state in component state, so a storage failure degrades the callout rather than replacing the panel.
• packages/app/tests/e2e/utils/base-test.ts:46 — The shared fixture seeds the dismissal flag for every spec unconditionally and with no opt-out, so no browser-level test can ever observe the opened state, which is both the feature's only user-visible behavior and the only state that can overlay and intercept clicks.
  • Fix: Add one Playwright spec that clears the key in its own init script and asserts the callout opens, that the tab bar beneath it stays clickable, and that acknowledging it survives a reload.
  • _{adversarial, testing, kieran-typescript}
• packages/app/src/components/__tests__/ViewTraceCalloutButton.test.tsx:43 — The onView={jest.fn()} stub never unmounts the component, so this case cannot reach the production path where handleSourceStackPush removes the button in the same React batch, and it reads as coverage of a path it does not exercise.
  • Fix: Add a case that renders the component inside a parent whose onView synchronously unmounts it, then assert the key was still written.
  • _{testing, correctness, frontend-races}
• packages/app/src/components/__tests__/ViewTraceCalloutButton.test.tsx:11 — None of the five cases exercise the three deliberately non-default popover props (closeOnEscape, closeOnClickOutside, trapFocus) or the disabled true-to-false transition that is the only production path by which the callout opens.
  • Fix: Add cases asserting the callout survives an Escape keydown and an outside click, and one that rerenders from disabled true to false and asserts it then opens.
  • _{testing, correctness, frontend-races}

🔵 P3 nitpicks (6)

• packages/app/src/components/ViewTraceCalloutButton.tsx:66 — role="status"/aria-live="polite" collide with the role="dialog"/aria-labelledby/tabIndex={-1} Mantine applies to the dropdown, and the region enters the DOM already containing its text, so the intended announcement is unlikely to fire either way.
  • Fix: Drop role/aria-live from the dropdown and announce through a separately mounted live region whose text is written when the callout opens.
• packages/app/src/components/ViewTraceCalloutButton.tsx:9 — disabled: boolean and onView: () => void are independent, so a caller can pass a live callback alongside disabled, and the call site compensates by duplicating the same condition inside onView.
  • Fix: Replace the two props with a single optional onView callback and derive the disabled state from its absence.
• packages/app/tests/e2e/utils/base-test.ts:33 — Typing the init-script parameter as unknown[] erases positional information, so the new third element needs an unchecked as string and a future reordering of the argument array would pass a wrong runtime value as the storage key.
  • Fix: Type the parameter as the tuple [unknown[], unknown[], string] and remove the as string assertion.
• packages/app/tests/e2e/utils/base-test.ts:31 — The seed is registered with page.addInitScript, so any spec that creates a second page or follows a popup loads the app without the flag and gets the auto-opening callout back.
  • Fix: Register the init script on the browser context fixture so popups and additional pages inherit the seeded flags.
• packages/app/src/components/ViewTraceCalloutButton.tsx:55 — IconConnection is instantiated inline here, making a third copy of the trace kind-to-icon mapping alongside sourceSelectUtils.tsx:13 and the inline switch in SidePanelBreadcrumbs.tsx:28.
  • Fix: Source the icon from the shared SOURCE_KIND_ICONS map, exporting a size-parameterised helper from it if the 14px variant is needed.
• packages/app/src/components/__tests__/ViewTraceCalloutButton.test.tsx:49 — Both dismissal cases assert only the localStorage value and never that the dropdown left the DOM, so they would pass even if the opened wiring stopped responding to dismissed.
  • Fix: Assert queryByTestId('view-trace-callout') is absent after each dismissal click in addition to the stored value.

---

Reviewers (7): correctness, adversarial, testing, frontend-races, maintainability, kieran-typescript, project-standards.

Testing gaps:

• No browser-level coverage of the callout's opened state at all; the shared Playwright fixture seeds it dismissed for every spec with no per-spec override.
• The wiring added at DBRowSidePanel.tsx:767 and 868-883 (render gate, disabled derivation, onView guard) is untested — there is no DBRowSidePanel test file, and the onView guard duplicates the disabled condition so the two can diverge unnoticed.
• No test covers a failing localStorage write, a corrupt or non-boolean value stored under the key, or two instances syncing through the hook's customStorage event.
• No assertions anywhere for keyboard tab order into the dropdown or for how often the callout announces across remounts.
• Prior-review-comment verification did not run: shell and repo-search tooling were unavailable in this environment, so gh-based checks against earlier review threads were skipped.

[elizabetdev]

Re: the P0/P1 on ViewTraceCalloutButton.tsx (auto-opened dropdown overlaying the header/TabBar with closeOnClickOutside={false}):

This is intentional and kept by design. The callout is a one-time, one-line hint that only ever covers a small strip directly below the button, and it's dismissed deliberately via Got it or by clicking View Trace — after which it never reappears. We specifically do not want an outside click to dismiss it, because a stray click shouldn't burn the message before the user has read it. We accept that the covered controls are briefly non-interactive until the user acknowledges the hint (a single click).

I've documented this decision inline at the closeOnClickOutside={false} prop (commit 23f1294) so the trade-off is explicit for future readers. Happy to revisit if a maintainer feels strongly, but treating this as intentional rather than a defect.

[github-actions]

<!-- deep-review -->

Deep Review

✅ No critical issues found. Nothing in this diff breaks the happy path — the button renders, navigation is unchanged, and the dismissal persists on the normal click. The findings below are all about the popover: an auto-opening, portalled overlay that deliberately cannot be dismissed by outside click or Escape, whose only exit is a small "Got it" button.

🟡 P2 -- recommended

• packages/app/src/components/ViewTraceCalloutButton.tsx:54 -- opened is derived from disabled, which flips asynchronously when useSource resolves, so the 260px dropdown can appear after the panel is already interactive and silently swallow an in-flight click on the region it covers.
  • Fix: Gate the first auto-open on a cancelable delay after the disabled: true → false transition, or set pointerEvents: 'none' on the dropdown body so covered controls stay clickable.
  • _{correctness, julik-frontend-races, adversarial}
• packages/app/src/components/ViewTraceCalloutButton.tsx:53 -- Escape does not dismiss the callout; it reaches the pre-existing useHotkeys(['esc'], handlePanelBack) at DBRowSidePanel.tsx:372, which closes the panel and runs clearTrail() + clearTraceWaterfallSearchState(), and because nothing was persisted the nudge reappears on the next eligible log.
  • Fix: Add a component-local hiddenForThisMount flag set by a keydown handler on the dropdown so Escape hides the nudge without persisting and without destroying panel state.
  • _{adversarial, correctness}
• packages/app/src/components/ViewTraceCalloutButton.tsx:36 -- Dismissal persists only through useLocalStorage, whose localStorage.setItem runs inside the setStoredValue updater, and the button's own click unmounts this component in the same commit (showLogTraceActions goes falsy once the trace frame is pushed), so the write depends on React's eager-state path and a blocked or failing write leaves the overlay permanently open with a dead "Got it".
  • Fix: Mirror the dismissal in component state (opened={!disabled && !dismissed && !dismissedThisMount}) and write the key synchronously in the click handler before invoking onView.
  • _{correctness, testing, julik-frontend-races, adversarial}
• packages/app/src/components/ViewTraceCalloutButton.tsx:44 -- With trapFocus={false}, no focus move, and the dropdown portalled to the end of document.body, the "Got it" control is not reachable in bounded tab order, so the only cheap keyboard exit is activating View Trace, which navigates away from the row being inspected.
  • Fix: Render the hint inline in DOM order beside the button, or move focus to the dropdown on open and pair it with a non-destructive Escape handler.
  • _{adversarial, correctness}
• packages/app/src/components/__tests__/ViewTraceCalloutButton.test.tsx:11 -- The five cases cover only static disabled/dismissed states and the two dismiss paths; nothing asserts the callout survives an outside click, that Escape is left to the panel hotkey, that it opens on the disabled: true → false transition, or that dismissal persists when the component unmounts in the same commit — so reverting the non-default Popover props would pass CI.
  • Fix: Add cases that fire an outside mousedown and an Escape keydown and assert the dropdown remains, rerender with disabled flipping false, and use a wrapper whose onView unmounts the component while asserting the persisted value.
  • _{testing, correctness, maintainability, project-standards}
• packages/app/tests/e2e/utils/base-test.ts:46 -- Seeding the dismissal key for every spec that uses the shared fixture means the feature has zero end-to-end coverage and no test ever exercises the app in the state a first-time user sees, which is the only state this change is about.
  • Fix: Add one spec that clears the key before opening an eligible log and asserts the callout appears, survives an outside click, and never returns after acknowledgement.
  • _{testing, adversarial}

🔵 P3 nitpicks (8)

• packages/app/src/components/ViewTraceCalloutButton.tsx:52 -- On a fully-controlled Popover with no onChange/onClose, closeOnClickOutside, closeOnEscape, and trapFocus have nothing to call and are inert, yet the adjacent comment credits them for the pinning behavior that the derived opened actually provides.
  • Fix: Rewrite the comment to attribute the guarantee to opened being fully derived, and drop the redundant trapFocus={false} since it is already the Mantine default.
  • _{correctness, maintainability}
• packages/app/src/components/ViewTraceCalloutButton.tsx:32 -- dismissed is used as a boolean without coercion: a targeted removeItem from another tab makes useLocalStorage's storage handler assign null through a non-null assertion, reopening the nudge, while any truthy non-boolean under the key suppresses it permanently and silently.
  • Fix: Coerce at the boundary with const dismissed = stored === true;.
  • _adversarial
• packages/app/src/components/ViewTraceCalloutButton.tsx:39 -- The popover uses Mantine's default z-index rather than deriving from ZIndexContext, unlike every other layer in this panel, which computes contextZIndex + 10.
  • Fix: Pass zIndex={useZIndex() + 1} so the callout layers inside the drawer's stacking band.
  • _{correctness, adversarial}
• packages/app/src/components/ViewTraceCalloutButton.tsx:75 -- A role="status" / aria-live="polite" region inserted into the DOM together with its content is generally not announced by assistive technology, so the stated screen-reader affordance does not take effect.
  • Fix: Render a persistent empty live region and populate it when the callout opens, or drop the live-region attributes and manage focus instead.
  • _adversarial
• packages/app/src/components/DBRowSidePanel.tsx:870 -- The traceSourceData && traceSpanRowId eligibility check is written twice, once negated for disabled and once inside onView, so the two can drift.
  • Fix: Hoist const canViewTrace = Boolean(traceSourceData && traceSpanRowId); and use it in both places.
  • _{maintainability}
• packages/app/tests/e2e/utils/base-test.ts:33 -- The init-script argument is a positional unknown[] destructured with a calloutDismissedKey as string cast, so reordering the array silently writes the literal key "undefined" and reintroduces the overlay across the whole suite; repo standards also call for avoiding as.
  • Fix: Pass a named object ({ connections, sources, calloutDismissedKey }) with an explicit parameter type instead of a positional array.
  • _adversarial
• packages/app/tests/e2e/utils/base-test.ts:31 -- The seed is registered on the page fixture, so pages created via context.newPage() or opened as popups never receive it.
  • Fix: Override the context fixture and register the seed with context.addInitScript so all pages inherit it.
  • _adversarial
• packages/app/src/components/ViewTraceCalloutButton.tsx:51 -- The inline comment cites a PR number as the justification for the design decision, which carries no meaning without repo access and will not update if the decision is revisited.
  • Fix: Drop the PR reference and let the surrounding rationale stand on its own.
  • _{maintainability}

---

Reviewers (6): correctness, testing, maintainability, project-standards, julik-frontend-races, adversarial.

Testing gaps:

• No unit test covers the two guarantees the non-default Popover props exist for (survives outside click; does not intercept Escape).
• No test covers the disabled: true → false transition, which is the only way the callout ever opens in production.
• No test covers the dismiss-then-unmount lifecycle that occurs at the real call site, nor a localStorage.setItem that throws.
• No DBRowSidePanel-level test for the showLogTraceActions gate or the disabled derivation.
• No end-to-end coverage of the callout in any state, since the shared fixture pre-dismisses it globally.

_{Coverage note: this run had no shell, git, or grep/glob access (bwrap failed to initialize), so the diff was reconstructed by reading the changed files directly rather than from git diff. .claude/skills/data-source-icons/SKILL.md was absent from the working tree and could not be audited, and the changeset entry could not be enumerated — neither is claimed as missing. ce-agent-native-reviewer and ce-learnings-researcher were not dispatched because both require repo-wide search.}

[github-actions]

Deep Review

✅ No critical issues found. Nothing in this diff causes data loss, an auth bypass, injection, or a guaranteed crash, and the localStorage key, the secondary/primary Button variants, and the e2e fixture import all resolve correctly against the existing theme and test infrastructure. The findings below are interaction, accessibility, and coverage concerns.

🟡 P2 -- recommended

• packages/app/src/components/ViewTraceCalloutButton.tsx:54 -- opened is derived purely from disabled, which flips asynchronously when the useSource query for the trace source resolves, so a 260px dropdown materialises over the panel with no user gesture and — with closeOnClickOutside={false} and closeOnEscape={false} — cannot be cleared by any gesture other than the two buttons inside it.
  • Fix: Latch the open state to the first render in which the button mounts already enabled, or allow an outside click to close the popover without persisting dismissal so the nudge still reappears later.
  • _{ce-correctness-reviewer, ce-julik-frontend-races-reviewer}
• packages/app/src/components/ViewTraceCalloutButton.tsx:73 -- the role="status" / aria-live="polite" node is mounted together with its text because Popover.Dropdown only renders while opened and no keepMounted is set, and a live region that enters the accessibility tree already populated is generally not announced.
  • Fix: Add keepMounted to the Popover, or announce through a separate always-mounted visually-hidden live region whose text is populated when the callout becomes eligible.
  • _{ce-correctness-reviewer, ce-julik-frontend-races-reviewer}
• packages/app/src/components/ViewTraceCalloutButton.tsx:44 -- the dropdown auto-opens with trapFocus={false} and no focus move, and Mantine portals it to the end of document.body, so reaching Got it by keyboard means tabbing through the remainder of the drawer while Escape is already consumed by the panel-level useHotkeys(['esc'], handlePanelBack).
  • Fix: Move focus to the acknowledgement button when the callout opens, so keyboard users get a one-key dismissal path.
  • _{ce-julik-frontend-races-reviewer}
• packages/app/src/components/__tests__/ViewTraceCalloutButton.test.tsx:21 -- every case mounts with a fixed disabled value, so the true → false transition that is the only way the popover ever opens in production is untested.
  • Fix: Add a case that rerenders the same instance from disabled to disabled={false} and asserts the callout appears.
  • _{ce-testing-reviewer, ce-correctness-reviewer}
• packages/app/tests/e2e/utils/base-test.ts:46 -- the shared fixture seeds the dismissed key for every Playwright test, so no end-to-end run ever renders the callout, and the unit tests render under a bare MantineProvider without the app theme, leaving the feature with no coverage in a production-like theme and drawer context.
  • Fix: Add one e2e spec that clears the dismissed key and asserts the callout appears, that Got it dismisses without navigating, and that the dismissal survives a reload.
  • _{ce-testing-reviewer, ce-correctness-reviewer}

🔵 P3 nitpicks (5)

• packages/app/src/components/ViewTraceCalloutButton.tsx:39 -- the Popover sets no zIndex, so it takes Mantine's default of 300 while KeyboardShortcutsModal takes the default Modal zIndex of 200, letting the callout float above that modal and its overlay.
  • Fix: Thread a zIndex derived from useZIndex() into the Popover so it participates in the drawer's stacking scheme.
• packages/app/src/components/ViewTraceCalloutButton.tsx:75 -- overriding the dropdown role to status conflicts with Mantine's default withRoles wiring, which decorates the target with aria-haspopup, aria-expanded, and aria-controls pointing at that node.
  • Fix: Disable withRoles on the Popover, or drop the role override and announce through a dedicated live region instead.
• packages/app/src/components/ViewTraceCalloutButton.tsx:62 -- IconConnection is hardcoded here for the trace kind while SidePanelBreadcrumbs.tsx:29-30 keeps its own independent copy of the same mapping, and the accompanying markdown convention doc is not enforced by the compiler.
  • Fix: Export one SourceKind-keyed icon map and have both components consume it.
• packages/app/src/components/__tests__/ViewTraceCalloutButton.test.tsx:43 -- both dismissal cases assert only the localStorage write, never that the callout actually disappears.
  • Fix: Assert queryByTestId('view-trace-callout') is absent after each dismissal click.
• packages/app/src/components/ViewTraceCalloutButton.tsx:52 -- the deliberately non-dismissing closeOnClickOutside / closeOnEscape behavior is explained in a long comment but locked in by no test.
  • Fix: Add a case that clicks outside and presses Escape while the callout is open and asserts it remains mounted.

---

Reviewers (4): ce-correctness-reviewer, ce-testing-reviewer, ce-maintainability-reviewer, ce-julik-frontend-races-reviewer.

Coverage caveat: this sandbox had no working shell and no Grep/Glob, so scope was reconstructed by reading the changed files directly rather than from git diff. The reviewer fan-out was reduced accordingly, and two claims were dropped as unverifiable here: whether a .changeset/ entry exists for this user-facing @hyperdx/app change (required by AGENTS.md), and whether any DBRowSidePanel test covers the new call-site conditional. .claude/skills/data-source-icons/SKILL.md is absent from the working tree and was not reviewed.

Testing gaps:

• The async disabled: true → false transition — the sole production trigger for the popover — is not exercised at any level.
• Blanket e2e seeding of the dismissed key removes the only environment where the callout's real positioning, theming, and overlap behavior could be observed.
• No test covers keyboard-only dismissal or the intentional non-dismissing outside-click/Escape behavior.
• Unit tests run under a themeless MantineProvider, so a wrong variant name on the new button would not fail any test.