Skip to content

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Oct 5, 2025

  • Prioritize checked filter values to top of list
  • Add visual separator and count indicator when collapsed
  • Auto-open filters when they contain active query filters

πŸ“„ 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 https://github.com/SigNoz/engineering-pod/issues/2945


πŸ“Έ Screenshots / Screen Recording (if applicable / mandatory for UI related changes)

Before:

Screen.Recording.2025-10-05.184843.mp4

After:

Screen.Recording.2025-10-05.185307.mp4

πŸ“‹ Checklist

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

πŸ‘€ Notes for Reviewers


Important

Enhance Checkbox filter visibility by prioritizing checked items, adding visual separators, and auto-opening filters with active queries.

  • Behavior:
    • Prioritize checked filter values to the top in Checkbox.tsx.
    • Auto-open filters with active query filters in Checkbox.tsx.
    • Add visual separator between checked and unchecked items in Checkbox.tsx.
  • Styles:
    • Add .filter-separator style in Checkbox.styles.scss for visual separation.
  • Tests:
    • Add Checkbox.test.tsx to test auto-open behavior and checked item prioritization.

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

- Prioritize checked filter values to top of list
- Add visual separator and count indicator when collapsed
- Auto-open filters when they contain active query filters
@ahmadshaheer ahmadshaheer requested review from a team and YounixM as code owners October 5, 2025 14:34
@github-actions github-actions bot added the enhancement New feature or request label Oct 5, 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.

Caution

Changes requested ❌

Reviewed everything up to a62aae2 in 1 minute and 37 seconds. Click for details.
  • Reviewed 285 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 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/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:67
  • Draft comment:
    The PR description mentions resolving an API params issue, but no changes related to URL parameter handling or API calls are apparent in this diff. Please confirm that the API fix has been included elsewhere.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to confirm whether the API fix mentioned in the PR description is included in the changes. This violates the rule against asking the author to confirm their intention or to update the PR description. Therefore, this comment should be removed.
2. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:636
  • Draft comment:
    Avoid dynamically generating CSS class names from props (e.g. using filter.title) as this can lead to unpredictable styling if the value contains unexpected characters. Consider sanitizing or using predefined class names.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:317
  • Draft comment:
    Nested ternary expressions (e.g. for determining 'Only' vs 'All') harm readability. Consider refactoring this logic using clearer conditional statements.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_q707KpceG5PBWnGH

You can customize Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

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.

Caution

Changes requested ❌

Reviewed 75f280b in 1 minute and 32 seconds. Click for details.
  • Reviewed 248 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 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/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.test.tsx:110
  • Draft comment:
    The tests do not verify that URL parameters (e.g. the 'service' param) are actually passed to the API call as required by the issue. Please add tests or assertions to confirm that non-empty URL parameters are forwarded to the API endpoint.
  • 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% This is a new test file focused on UI component behavior. The tests are well structured and cover key UI interactions. The API call is mocked but used as a support mechanism, not the main focus. Testing API parameter handling would be more appropriate in an API integration test. The current tests appropriately focus on component-level behavior. The comment raises a valid point about API parameter testing. Perhaps there are requirements around URL parameter handling that I'm not aware of. Even if URL parameter handling is important, this component test file is not the right place for those tests. That would be better handled in an API integration test file. The comment should be deleted as it suggests adding API integration tests in a UI component test file, which would mix concerns and violate testing best practices.

Workflow ID: wflow_EGAnoFjpfErXOkPb

You can customize Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

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 9a2fe42 in 1 minute and 11 seconds. Click for details.
  • Reviewed 86 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 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/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:1
  • Draft comment:
    The PR description mentions an API call fix (passing URL params) but no changes to API parameter handling are evident. Ensure the API fix is implemented or update the description.
  • 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.
2. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:24
  • Draft comment:
    Explicitly importing Fragment (added here) enables use of keyed fragments, which improves list rendering. Good update.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply praises the update without offering any specific guidance or questions.
3. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:612
  • Draft comment:
    Using a keyed Fragment for wrapping each list item (with key={value}) is a good practice to prevent key warnings when rendering lists.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative and does not provide a specific suggestion or ask for a confirmation of intention. It simply states a best practice without addressing a specific issue in the code.
4. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:638
  • Draft comment:
    Dynamically constructing a class name using filter.title (e.g. ${filter.title} label-${value}) may lead to unpredictable CSS if the title contains spaces or special characters. Consider using a sanitized or fixed class name.
  • 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/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:570
  • Draft comment:
    Refactoring the count indicator markup by replacing an extra
    wrapper with a direct improves the DOM structure. This cleanup enhances clarity.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is purely informative, explaining the benefits of a change that was made. It doesn't suggest any action or raise any concerns about the code. According to the rules, purely informative comments should be removed.
6. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.test.tsx:192
  • Draft comment:
    Switching from document.querySelector to screen.getByTestId('filter-separator') is a good practice for more reliable and readable tests.
  • 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.

Workflow ID: wflow_4lHTEI4L8rBRijIy

You can customize Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

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 cecb810 in 1 minute and 45 seconds. Click for details.
  • Reviewed 107 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 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/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.styles.scss:31
  • Draft comment:
    The .filter-count style block has been removed. The PR description mentions adding a count indicator when collapsed, so please clarify if the count indicator feature is intentionally removed or if it needs to be restyled instead.
  • 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% This comment violates several rules. It's asking for clarification about intention ("please clarify if..."), which is explicitly against our rules. It's also referencing the PR description, which we're told not to do. The comment isn't pointing out a clear issue that needs fixing - it's just asking for explanation. Maybe the removal of the count indicator could break functionality if it's still being used in the component's logic? Even if there might be functionality implications, we're told to only comment on what we can see in the diff. We can't make assumptions about the component's logic without seeing it. The comment should be deleted as it violates our rules by asking for clarification and referencing the PR description rather than pointing out a clear issue that needs fixing.
2. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.test.tsx:148
  • Draft comment:
    The test case for showing the count indicator when the filter is collapsed (with active filters) has been removed. Since the PR description includes a count indicator, please confirm whether the indicator feature has been dropped intentionally or if the test and UI need to be updated accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to confirm their intention regarding the removal of a test case. It violates the rule against asking the author to confirm their intention or to ensure behavior is intended. Therefore, it should be removed.
3. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:274
  • Draft comment:
    The appliedFiltersCount hook and the corresponding rendering of the count indicator in the right-action section have been removed. The PR description mentions adding a count indicator when collapsed, so please clarify if this removal is intentional or if additional changes are pending.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to clarify their intention regarding the removal of a feature, which violates the rule against asking for confirmation of intention. It does not provide a specific code suggestion or request for a test, nor does it address any of the additional rules provided.
4. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:75
  • Draft comment:
    The auto‐open behavior based on active filters (using userToggleState and isSomeFilterPresentForCurrentAttribute) is implemented clearly and is validated in the tests. Good job on ensuring checked items are prioritized.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_cQtRL1tn2OVBat95

You can customize Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

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 9d188ff in 49 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 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/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.styles.scss:31
  • Draft comment:
    This change only removes extra whitespace before the .clear-all block. While the style update is harmless, the PR description also mentions an API call issue fix which isn’t reflected in this diff. Please ensure that the API call parameter bug and corresponding tests are addressed in the appropriate commits.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that the API call parameter bug and corresponding tests are addressed, which violates the rule against asking the author to ensure things are tested or verified. The comment also mentions the PR description, which is not allowed. Therefore, this comment should be removed.

Workflow ID: wflow_DMqDuDuEqZyVTFNr

You can customize Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

Copy link
Contributor

@aks07 aks07 left a comment

Choose a reason for hiding this comment

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

minor comment

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 e00625b in 49 seconds. Click for details.
  • Reviewed 23 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 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/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:68
  • Draft comment:
    The useMemo wrapping for isSomeFilterPresentForCurrentAttribute is a good optimization, and the dependency array appears correct. However, please ensure that the reference to currentQuery.builder.queryData is stable enough to benefit from memoization. Also, the PR description mentions fixing API call URL parameters, but this diff only shows a change in filter memoization. Confirm that the API call issue is handled elsewhere.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment starts by acknowledging a good optimization, which is informative. It then asks the author to ensure the stability of a reference, which is not allowed. Finally, it asks the author to confirm if an issue is handled elsewhere, which is also not allowed. Overall, the comment violates the rules by being informative and asking for confirmation.

Workflow ID: wflow_HfmOWfri9pERPAhy

You can customize Ellipsis by changing your verbosity settings, reacting with πŸ‘ or πŸ‘Ž, replying to comments, or adding code review rules.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants