Skip to content

Conversation

@teeohhem
Copy link
Contributor

@teeohhem teeohhem commented Jan 9, 2026

Fixes: HDX-1717

Adds:

  • Save icon that will save the WHERE input filter, along with any filters applied to the dashboard via the custom filter functionality + ties it to the dashboard object itself. Future reloads of this dashboard will restore saved values.
image

@changeset-bot
Copy link

changeset-bot bot commented Jan 9, 2026

⚠️ No Changeset found

Latest commit: ba26bc6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jan 9, 2026

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

Project Deployment Review Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview, Comment Jan 9, 2026 7:51pm

@claude
Copy link

claude bot commented Jan 9, 2026

PR Review: Add saved query support to dashboards

Overall Implementation: The feature is well-implemented with comprehensive E2E tests.

Issues Found:

  • Race condition in initialization (DBDashboardPage.tsx:729-745) → The useRef<string>(undefined) should be useRef<string | undefined>(undefined) to properly type-check against dashboard.id. Currently TypeScript may not catch issues where dashboard.id could be compared against the wrong type.

  • ⚠️ Missing dependency optimization (DBDashboardPage.tsx:746-757) → The useEffect depends on router.query which changes on every render. Consider using router.query.where and router.query.filters specifically, or use useMemo to stabilize the dependency.

  • ⚠️ Potential data loss pattern (packages/api/src/routers/api/dashboards.ts:84) → The change from _.isNil to _.isUndefined is correct for allowing null to clear fields, but ensure all callers understand this new behavior. Document this in code comments.

  • 🔍 Minor: Missing null check (DBDashboardPage.tsx:759-791) → handleSaveQuery accesses rawFilterQueries?.length correctly, but should also check if it is an empty array before setting to null: rawFilterQueries?.length > 0 ? rawFilterQueries : null

Recommendations:

  1. Add inline comment at line 84 explaining the _.isUndefined change for future maintainers
  2. Consider extracting the saved query restoration logic into a custom hook for better testability
  3. E2E tests are excellent - consider adding unit tests for the handleSaveQuery callback logic

Status: Approve with minor suggestions for follow-up.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2026

E2E Test Results

2 tests failed • 61 passed • 4 skipped • 766s

Status Count
✅ Passed 61
❌ Failed 2
⚠️ Flaky 0
⏭️ Skipped 4

Tests ran across 4 shards in parallel.

View full report →

@elizabetdev
Copy link
Contributor

Hey @teeohhem,

I’d like to suggest moving the Save query and filters action into the global Dashboard ... menu.

Right now we have a mixed interaction model: the dashboard layout (tiles) autosaves, but query and filter inputs require a manual save via the floppy disk icon. This can be confusing.

Two main UX issues with the current placement:

  • Ambiguity: Seeing a save button near the inputs makes users question whether other changes (like moving tiles) were saved.
  • Mental model: Because the icon sits next to the query bar, it implies only the text input is saved, even though it also saves the filters below.

Moving this into the Dashboard ... menu would better separate:

  • Daily usage (running queries, adjusting filters)
  • Configuration (setting default views)

The menu could be state-based.

State 1: No defaults saved:

[ Menu]
----------------------------------
⬇️  Export Dashboard
⬆️  Import New Dashboard
----------------------------------
💾  Save Query & Filters as Default
----------------------------------
🗑️  Delete Dashboard              (danger text)

State 2: Defaults are active

[ Menu ]
----------------------------------
⬇️  Export Dashboard
⬆️  Import New Dashboard
----------------------------------
💾  Update Default Query & Filters
✖️  Remove Default Query & Filters (danger text)
----------------------------------
🗑️  Delete Dashboard              (danger text)

This is only a suggestion. Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants