Skip to content

Conversation

@brandon-pereira
Copy link
Member

Removes react-select dependency and replaces with Mantine select.

Most of the uses of this component weren't actually in use, so this ended up being pretty easy.

As suggested by @elizabetdev as part of the mantine / bootstrap cleanup

@changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2025

🦋 Changeset detected

Latest commit: 3e83926

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@hyperdx/app Patch
@hyperdx/api 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
Copy link

vercel bot commented Nov 14, 2025

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

Project Deployment Preview Comments Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview Comment Nov 14, 2025 8:19pm

@claude
Copy link

claude bot commented Nov 14, 2025

PR Review

Overall: Good cleanup, but one critical type issue

Critical Issues

  • GranularityPicker.tsx:61-63 - Type mismatch in onChange handler → Mantine Select onChange receives string | null, not string | undefined. The cast should handle null properly:
    onChange={v => onChange((v ?? undefined) as Granularity | 'auto' | undefined)}
    Should be:
    onChange={v => onChange(v as Granularity | 'auto' | null)}
    Or if undefined is required by the consumer:
    onChange={v => onChange((v || undefined) as Granularity | 'auto' | undefined)}

Positive Changes

  • ✅ Successfully removed react-select dependency (~185 lines removed from yarn.lock)
  • ✅ Replaced with Mantine Select (follows project pattern per CLAUDE.md)
  • ✅ Removed unused components (DBColumnMultiSelect, DSSelect, TableSelect)
  • ✅ Fixed typo in AppNav.module.scss (2x2px)
  • ✅ Improved e2e test reliability with pressSequentially and mode-specific assertions

Notes

  • The removal of DBColumnMultiSelect appears safe (no remaining imports found)
  • E2e test changes improve test stability by handling CodeMirror vs textarea differences properly

@github-actions
Copy link
Contributor

github-actions bot commented Nov 14, 2025

E2E Test Results

All tests passed • 39 passed • 3 skipped • 394s

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

View full report →

elizabetdev
elizabetdev previously approved these changes Nov 14, 2025
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested in Google Chrome.

I also have the same test failing on a different branch.

pulpdrew
pulpdrew previously approved these changes Nov 14, 2025
Copy link
Contributor

@pulpdrew pulpdrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with two minor nits

@elizabetdev
Copy link
Contributor

@brandon-pereira I merged the very big PR that removes bootstrap, and it was causing a conflict here. So I fixed it and merged main: ef1ed44.

@brandon-pereira
Copy link
Member Author

@elizabetdev @pulpdrew pushed some improvements, removed DBSelect and just called the Mantine Select.. also improved the flakey test so it should be better now :)

Copy link
Contributor

@pulpdrew pulpdrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the test fix!

@brandon-pereira brandon-pereira merged commit 44a6a08 into main Nov 14, 2025
10 checks passed
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.

4 participants