Skip to content

fix: add loading spinners when loading panels and overviews - #3604

Merged
kodiakhq[bot] merged 2 commits into
mainfrom
aaron/panel-loading-spinners
Nov 10, 2026
Merged

kodiakhq[bot] merged 2 commits into
mainfrom
aaron/panel-loading-spinners

Conversation

@knudtty

@knudtty knudtty commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

Summary

When loading a row or trace, all you'd see is some variation of "no data found" when in fact the query is in flight. This PR adds loading spinners to accurately reflect the loading state.

Screenshots or video

Before

image image

After

Notice the loading spinners.
image
image

References

  • Linear Issue: Closes HDX-5984

@knudtty
knudtty requested a review from a team as a code owner November 10, 2026 16:25
@vercel

vercel Bot commented Nov 10, 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 Nov 10, 2026 4:46pm UTC
hyperdx-storybook Ready Ready Preview Nov 10, 2026 4:46pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2f547b5

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

@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.

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/__tests__/DBRowDataPanel.rowQuery.test.tsx:118 — New test only covers the pending state of RowDataPanel; the loader in RowOverviewPanel is never tested → The test uses a promise that never resolves, so it would still pass if isLoading stayed true forever. Add a case that resolves the row and checks that 'Loading row' disappears and DBRowJsonViewer gets the row. Also add a pending-state test for RowOverviewPanel. The existing DBRowOverviewPanel.*.test.tsx mocks of ../DBRowDataPanel don't export RowPanelLoader or return isLoading, so the new branch in DBRowOverviewPanel.tsx:301 has no coverage.

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

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Nov 10, 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: 2
  • Production lines changed: 37 (+ 15 in test files, excluded from tier calculation)
  • Branch: aaron/panel-loading-spinners
  • Author: knudtty

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

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

Scope: PR #3604 — adds loading spinners to row/trace detail panels (RowDataPanel, RowOverviewPanel) so an in-flight row lookup shows a <RowPanelLoader/> instead of an "Empty" placeholder. 3 source files + 1 changeset, 58 insertions.

Intent: Replace the misleading "no data" state shown during an in-flight useRowData query with an accurate loading indicator.

✅ No critical issues found. The async paths were traced directly: useRowData's custom isLoading resolves to false when rowId is null (both underlying queries are disabled; react-query v5 reports isLoading=false for disabled queries), so there is no permanent-spinner path; both early returns sit after all hooks, so rules-of-hooks hold; and rowLookupStaleTime: Infinity for found rows means a background refetch never blanks loaded content behind a spinner.

🟡 P2 — recommended

  • packages/app/src/components/DBRowOverviewPanel.tsx:301 — the new if (isLoading) return <RowPanelLoader/> branch added to RowOverviewPanel has no test; all existing DBRowOverviewPanel.*.test.tsx stub useRowData without isLoading, so the loader path this PR introduces is never exercised (only RowDataPanel's equivalent branch is covered).
    • Fix: Add a test that mocks useRowData to return isLoading: true and asserts findByLabelText('Loading row') renders while the header/subpanels do not, mirroring the RowDataPanel test.
    • testing, correctness, kieran-typescript, julik-frontend-races
🔵 P3 nitpicks (2)
  • packages/app/src/components/DBRowDataPanel.tsx:482 — data-testid is applied to both the loader render path and the loaded-content path, so a test keyed only on the test id cannot distinguish loading from loaded (the aria-label="Loading row" on the Loader is what currently disambiguates).
    • Fix: Have consumers assert on content or the aria-label rather than the shared test id when they mean "content rendered."
  • packages/app/src/components/DBRowDataPanel.tsx:434 — RowPanelLoader, a presentational component, is exported from a module that is otherwise the useRowData data hook and SQL/meta helpers, deepening the file's mixed responsibility.
    • Fix: Co-locate the loader with the other panel UI components or a shared panel-primitives module; acceptable as-is since RowDataPanel already lives in this file.
    • kieran-typescript, maintainability

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

Testing gaps:

  • No assertion that rowId == null leaves the panel in the empty state rather than spinning (behavior is verified-safe but unlocked only implicitly).
  • No single-mount loader→content transition assertion (loader removed once data settles).
  • No test exercising the isBoundedEmpty && queryResult.isPending guard — the line that prevents a one-render content/empty flash when the fallback query activates.

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

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 464 passed • 1 skipped • 1227s

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

Tests ran across 4 shards in parallel.

View full report →

@teeohhem
teeohhem requested a review from elizabetdev November 10, 2026 16:45
@kodiakhq
kodiakhq Bot merged commit 82b2104 into main Nov 10, 2026
28 checks passed
@kodiakhq
kodiakhq Bot deleted the aaron/panel-loading-spinners branch November 10, 2026 16:53
Moutia-Ben-Yahia pushed a commit to HotOpenSourcing/ClickStack that referenced this pull request Nov 10, 2026
Moutia-Ben-Yahia pushed a commit to HotOpenSourcing/ClickStack that referenced this pull request Nov 11, 2026
Moutia-Ben-Yahia pushed a commit to HotOpenSourcing/ClickStack that referenced this pull request Nov 11, 2026
Moutia-Ben-Yahia pushed a commit to HotOpenSourcing/ClickStack that referenced this pull request Nov 11, 2026

This branch was successfully deployed

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