New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: New metrics name select #331
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested.
- Reviewed the entire pull request up to 2fe32cc
- Looked at
110
lines of code in2
files - Took 1 minute and 9 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
0
additional comments because they didn't meet confidence threshold of50%
.
Workflow ID: wflow_r3CjoCBArY3muBhE
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. We'll respond in a few minutes. Learn more here.
packages/app/src/ChartUtils.tsx
Outdated
/> | ||
); | ||
|
||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AsyncSelect component is still being returned at the end of the MetricNameSelect function. This seems redundant as the MSelect component is already being returned earlier in the function. Consider removing this redundant return statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested.
- Performed an incremental review on 86be50c
- Looked at
74
lines of code in1
files - Took 1 minute and 36 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
0
additional comments because they didn't meet confidence threshold of50%
.
Workflow ID: wflow_lmDl4EesNni31T7I
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. We'll respond in a few minutes. Learn more here.
@@ -2,7 +2,13 @@ import { useMemo, useRef, useState } from 'react'; | |||
import { add } from 'date-fns'; | |||
import Select from 'react-select'; | |||
import AsyncSelect from 'react-select/async'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import of AsyncSelect is no longer used and should be removed to keep the code clean.
packages/app/src/ChartUtils.tsx
Outdated
@@ -441,6 +447,7 @@ | |||
</> | |||
); | |||
} | |||
import { Autocomplete } from '@mantine/core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import of Autocomplete is not used and should be removed to keep the code clean.
<AsyncSelect | ||
isLoading={isLoading} | ||
isDisabled={isError} | ||
<MSelect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MSelect component should include a similar filtering functionality to the loadOptions prop of the AsyncSelect component it replaced. This will prevent potential performance issues if the options list is very large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me!
- Performed an incremental review on d4a8491
- Looked at
66
lines of code in1
files - Took 1 minute and 26 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. /packages/app/src/ChartUtils.tsx:474
:
- Assessed confidence :
80%
- Grade:
0%
- Comment:
The AsyncSelect component was replaced with MSelect. The AsyncSelect component was used to load options asynchronously, but the new MSelect component does not have this functionality. This could potentially lead to performance issues if there are a large number of options to load. - Reasoning:
The PR changes the AsyncSelect component to MSelect from mantine. The AsyncSelect component was used to load options asynchronously, but the new MSelect component does not have this functionality. This could potentially lead to performance issues if there are a large number of options to load.
Workflow ID: wflow_KEnV5aWKX2eJlX69
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
nice! i'm wondering if this was tested against how it performs on a few thousand or 10k items? iirc this was an easy component to overwhelm |
yes, just tested it with 10k items, works smooth. I set a limit to render only 100 items max at a time so it shouldn't overwhelm Screen.Recording.2024-03-03.at.10.12.08.AM.mov |
https://github.com/hyperdxio/hyperdx/assets/149748269/2023c7d3-be2e-400d-be88-6f31e2f59f1d ---- | <a href="https://ellipsis.dev" target="_blank"><img src="https://avatars.githubusercontent.com/u/80834858?s=400&u=31e596315b0d8f7465b3ee670f25cea677299c96&v=4" alt="Ellipsis" width="30px" height="30px"/></a> | 🚀 This PR description was created by [Ellipsis](https://www.ellipsis.dev) for commit d4a8491. | |--------|--------| ### Summary: This PR enhances the user interface and functionality by updating the `MetricNameSelect` function to use `MSelect` and modifying the `Drawer` and `Button` components. **Key points**: - Enhances the user interface and functionality of the application - Updates the `MetricNameSelect` function in `ChartUtils.tsx` to use `MSelect` instead of `AsyncSelect` - Modifies the `Drawer` and `Button` components in `SessionSidePanel.tsx` ---- Generated with ❤️ by [ellipsis.dev](https://www.ellipsis.dev)
Screen.Recording.2024-03-02.at.11.00.51.PM.mov
Summary:
This PR enhances the user interface and functionality by updating the
MetricNameSelect
function to useMSelect
and modifying theDrawer
andButton
components.Key points:
MetricNameSelect
function inChartUtils.tsx
to useMSelect
instead ofAsyncSelect
Drawer
andButton
components inSessionSidePanel.tsx
Generated with ❤️ by ellipsis.dev