-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
chore: hide promql from panel type - pie #6140
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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! Reviewed everything up to 756ab45 in 17 seconds
More details
- Looked at
248
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/container/NewWidget/LeftContainer/QuerySection/index.tsx:118
- Draft comment:
Consider addingqCategory
to the dependency array ofuseCallback
forhandleQueryCategoryChange
to ensure it captures the latest value. - Reason this comment was not posted:
Confidence changes required:50%
The use ofuseCallback
forhandleQueryCategoryChange
is appropriate as it optimizes performance by memoizing the function. However, theuseCallback
hook is missing a dependency onqCategory
. This could lead to stale closures ifqCategory
changes between renders.
2. frontend/src/container/NewWidget/LeftContainer/QuerySection/index.tsx:214
- Draft comment:
Ensure that theuseEffect
hook accounts for all panel types that might not support certain query types. Consider usingPANEL_TYPE_TO_QUERY_TYPES
to dynamically determine unsupported query types. - Reason this comment was not posted:
Confidence changes required:50%
TheuseEffect
hook on line 214 is used to switch to the query builder if the query type is not supported. However, it does not account for all panel types that might not support certain query types. This could lead to unexpected behavior if new panel types are added in the future.
3. frontend/src/container/NewWidget/utils.ts:497
- Draft comment:
Ensure thatPANEL_TYPE_TO_QUERY_TYPES
is kept up-to-date with any changes to panel types or query types to avoid inconsistencies. - Reason this comment was not posted:
Confidence changes required:33%
ThePANEL_TYPE_TO_QUERY_TYPES
mapping is used to determine which query types are supported for each panel type. This is a good approach for maintainability and scalability. However, it is important to ensure that this mapping is kept up-to-date with any changes to panel types or query types.
4. frontend/src/container/NewWidget/LeftContainer/QuerySection/index.tsx:265
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_oEjLfL0GuSj1BNae
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
756ab45
to
122f34e
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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! Incremental review on 122f34e in 18 seconds
More details
- Looked at
31
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/NewWidget/utils.ts:520
- Draft comment:
The change to removeEQueryType.PROM
fromPANEL_TYPES.PIE
is consistent with the PR's intent to hide PROMQL for PIE panel types. - Reason this comment was not posted:
Confidence changes required:0%
The PR aims to hide the PROM query type for PIE panel types. The change in the PANEL_TYPE_TO_QUERY_TYPES mapping reflects this intent correctly.
2. frontend/src/container/NewWidget/LeftContainer/QuerySection/index.tsx:217
- Draft comment:
The condition to switch to QUERY_BUILDER for PIE panel types when the current query type is PROM is correctly implemented. - Reason this comment was not posted:
Confidence changes required:0%
The useEffect hook is updated to switch to QUERY_BUILDER if the selected graph is PIE and the current query type is PROM. This aligns with the PR's intent.
Workflow ID: wflow_4QrFBcSwtT4P7MhW
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Summary
Removed PromQL from Pie Panel type
Related Issues / PR's
Screenshots
Screen.Recording.2024-10-09.at.10.17.31.AM.mov
Affected Areas and Manually Tested Areas
Important
Remove PromQL support for pie panel type by updating query handling logic and mappings.
index.tsx
, switch to query builder ifPANEL_TYPES.PIE
andEQueryType.PROM
are selected.PANEL_TYPE_TO_QUERY_TYPES
inutils.ts
to excludeEQueryType.PROM
forPANEL_TYPES.PIE
.This description was created by for 122f34e. It will automatically update as commits are pushed.