Skip to content

feat: query builder fixes and enhancement #8692

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

Merged
merged 6 commits into from
Aug 4, 2025

Conversation

SagarRajput-7
Copy link
Contributor

@SagarRajput-7 SagarRajput-7 commented Aug 4, 2025

📄 Summary


✅ Changes

  • Feature: Brief description
  • Bug fix: Brief description

🏷️ Required: Add Relevant Labels

⚠️ Manually add appropriate labels in the PR sidebar
Please select one or more labels (as applicable):

ex:

  • frontend
  • backend
  • devops
  • bug
  • enhancement
  • ui
  • test

👥 Reviewers

Tag the relevant teams for review:

  • frontend / backend / devops

🧪 How to Test

  1. ...
  2. ...
  3. ...

🔍 Related Issues

Closes #


📸 Screenshots / Screen Recording (if applicable / mandatory for UI related changes)


📋 Checklist

  • Dev Review
  • Test cases added (Unit/ Integration / E2E)
  • Manually tested the changes

👀 Notes for Reviewers


Important

Enhances query builder with function name normalization, improved aggregation handling, and UI updates for better user experience.

  • Function Name Normalization:
    • Introduces normalizeFunctionName in functionNameNormalizer.ts to handle case sensitivity between backend and frontend.
    • Updates all instances of function name handling to use normalizeFunctionName, including in QueryFunctions, Function, and QueryAggregationSelect components.
  • Aggregation Handling:
    • Modifies getColName and getColId in convertV5Response.ts to improve aggregation name handling.
    • Updates QueryBuilderV2Context to manage aggregation options per query.
    • Enhances MetricsAggregateSection and AggregatorFilter to support new aggregation logic.
  • UI Enhancements:
    • Updates ResizeTable and GridTableComponent to handle column widths and units more effectively.
    • Improves HavingFilter and Threshold components for better user interaction.
  • Testing:
    • Adds unit tests for normalizeFunctionName in functionNameNormalizer.test.ts.

This description was created by Ellipsis for c6d4a86. You can customize this summary. It will automatically update as commits are pushed.

@SagarRajput-7 SagarRajput-7 requested review from YounixM and a team as code owners August 4, 2025 05:38
@github-actions github-actions bot added bug Something isn't working enhancement New feature or request labels Aug 4, 2025
@SagarRajput-7 SagarRajput-7 added the safe-to-test Run CI tests for dependabot and external contributors label Aug 4, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to c6d4a86 in 1 minute and 50 seconds. Click for details.
  • Reviewed 1270 lines of code in 26 files
  • Skipped 0 files when reviewing.
  • Skipped posting 12 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/utils/functionNameNormalizer.ts:7
  • Draft comment:
    The normalization mapping looks clear and covers all expected cases. No issues found here.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. frontend/src/utils/functionNameNormalizer.test.ts:5
  • Draft comment:
    Tests for functionNameNormalizer are comprehensive and cover both string and numeric argument scenarios.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
3. frontend/src/container/QueryFunctions/Function.tsx:70
  • Draft comment:
    Avoid using inline styles (e.g., style={{ minWidth: '100px' }}) in React components. Consider moving such styles to an external stylesheet or using a styled-component for consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/container/QueryFunctions/QueryFunctions.tsx:185
  • Draft comment:
    Inline styling for icons/size (e.g., in the Plus button) is used. For maintainability and theming consistency, consider using CSS classes or styled-components instead of inline style properties.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. frontend/src/container/QueryBuilder/filters/AggregatorFilter/AggregatorFilter.tsx:155
  • Draft comment:
    Ensure that the inline style usage in this component (e.g., in the dropdown options) is minimized. Consider using external or styled CSS rules rather than inline style objects.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. frontend/src/container/NewWidget/RightContainer/Threshold/Threshold.tsx:144
  • Draft comment:
    Inline style objects (e.g., style={{ marginLeft: '10px' }} in Select components) are used. Per our guidelines, please externalize these styles to maintain consistency and easier theming.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. frontend/src/lib/createTableColumnsFromQuery.ts:712
  • Draft comment:
    The fallback logic in getColumnUnit is clear; however, consider adding comments to explain the new vs old syntax in detail to aid future maintainers.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50% None
8. frontend/src/lib/uPlotLib/utils/getSeriesData.ts:62
  • Draft comment:
    The dynamic generation of series configuration is well structured. Ensure that the handling of graph visibility states is robust when 'hiddenGraph' is provided.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50% None
9. frontend/src/lib/dashboard/getQueryResults.ts:207
  • Draft comment:
    The validation function for metric name in getQueryResults returns an empty response if metric name is missing. Confirm that this behavior is as expected to prevent API 400 errors.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50% None
10. frontend/src/container/QueryBuilder/components/QueryFunctions/Function.tsx:50
  • Draft comment:
    Typographical suggestion: The variable name 'debouncedhandleUpdateFunctionArgs' (line 50) might be intended to use camelCase. Consider renaming it to 'debouncedHandleUpdateFunctionArgs' for consistency.
  • Reason this comment was not posted:
    Comment was on unchanged code.
11. frontend/src/container/QueryBuilder/filters/AggregatorFilter/AggregatorFilter.tsx:42
  • Draft comment:
    Typo detected in the type name. The component file is named 'AggregatorFilter', so it's likely that 'AgregatorFilterProps' should be corrected to 'AggregatorFilterProps' (note the extra 'g').
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. frontend/src/utils/functionNameNormalizer.ts:18
  • Draft comment:
    Typo: The key 'absolut' might be a misspelling. Consider using 'absolute' for consistency with QueryFunctionsTypes.ABSOLUTE, unless this is intentional.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The file's purpose is to normalize function names from backend to frontend. The comment shows that 'absolut' maps to QueryFunctionsTypes.ABSOLUTE. Given the file's docstring, it's likely that 'absolut' is the actual backend function name that needs to be mapped to the frontend's ABSOLUTE type. The comment misunderstands the purpose of this mapping. I could be wrong about the backend's function naming - without seeing the backend code, I can't be 100% certain that 'absolut' is the correct backend name. However, given the clear documentation and pattern in the file that shows this is mapping from backend names to frontend names, and other similar mappings like 'timeshift' to 'TIME_SHIFT', it's very likely that 'absolut' is the correct backend name. The comment should be deleted as it misunderstands the purpose of this mapping file - the keys are meant to match backend function names, not frontend names.

Workflow ID: wflow_FZ8yMLWpo1e8HYHU

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@SagarRajput-7 SagarRajput-7 added safe-to-test Run CI tests for dependabot and external contributors and removed safe-to-test Run CI tests for dependabot and external contributors labels Aug 4, 2025
@SagarRajput-7 SagarRajput-7 requested a review from ahrefabhi August 4, 2025 06:21
@SagarRajput-7 SagarRajput-7 merged commit 46aaa4d into fix/query-builder Aug 4, 2025
2 checks passed
@SagarRajput-7 SagarRajput-7 deleted the qb-formating-fixes branch August 4, 2025 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request safe-to-test Run CI tests for dependabot and external contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants