Skip to content

feat: use search v2 component for traces #7537

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Apr 6, 2025

Summary

  • Use search v2 component to:
    • Add support for multiple types for traces data source
    • Add span filtering dropdown for traces data source
    • Write test for span filtering dropdown visible for traces data source and hidden for the rest of data sources
    • Write tests for the (key, operator, value) flow of qb search v2
    • overall improvements

Related Issues / PR's

close #7160
close #7355

Screenshots

Before:

2025-04-06.12-31-14.mov

After:

2025-04-06.12-32-31.mov

Affected Areas and Manually Tested Areas

Traces explorer
✔ Suggestions
✔ Search
✔ Multiple types
✔ stage and run sends the correct filters
✔ Display the span scope dropdown

Dashboard (create / edit panel)
✔ Suggestions
✔ Search
✔ Multiple types
✔ stage and run sends the correct filters
✔ Display the span scope dropdown

Alert
✔ Suggestions
✔ Search
✔ Multiple types
✔ stage and run sends the correct filters
✔ Display the span scope dropdown


Important

Introduce QueryBuilderSearchV2 for trace data sources with span filtering and multiple data type support, replacing QueryBuilderSearch in specific contexts.

  • Behavior:
    • Replace QueryBuilderSearch with QueryBuilderSearchV2 for DataSource.TRACES in Query.tsx.
    • Add span filtering dropdown for DataSource.TRACES in QueryBuilderSearchV2.tsx.
    • Support multiple data types in QueryBuilderSearchV2.tsx.
  • Tests:
    • Add tests for span filtering dropdown visibility in QueryBuilderSearchV2.test.tsx.
    • Add tests for key-operator-value flow in QueryBuilderSearchV2.test.tsx.
    • Add tests for SpanScopeSelector in SpanScopeSelector.test.tsx.
  • Styles:
    • Update styles in QueryBuilderSearchV2.styles.scss for new dropdown and span scope selector.
  • Misc:
    • Move SpanScopeSelector to QueryBuilderSearchV2 directory and update imports.

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

@ahmadshaheer ahmadshaheer requested a review from YounixM as a code owner April 6, 2025 08:26
@github-actions github-actions bot added docs required enhancement New feature or request labels Apr 6, 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.

👍 Looks good to me! Reviewed everything up to 2d24a9e in 1 minute and 17 seconds

More details
  • Looked at 156 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 11 drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/components/Query/Query.tsx:455
  • Draft comment:
    Refactoring the condition to use [DataSource.LOGS, DataSource.TRACES].includes(query.dataSource) is concise; ensure downstream logic handles both data sources properly.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchDropdown.tsx:49
  • Draft comment:
    Using classnames with the modifier 'non-logs-data-source' is good; verify that the corresponding CSS class is defined in the stylesheet.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:1017
  • Draft comment:
    Rendering when the data source is TRACES is a neat extension; ensure the component styling uses design tokens instead of inline values.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. frontend/src/hooks/queryBuilder/useOptions.ts:169
  • Draft comment:
    Removing duplicate options by comparing only label and value seems appropriate; confirm that excluding dataType in the duplicate check is intentional.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:971
  • Draft comment:
    Avoid using inline styles. The inline style prop (selectStyle) is passed directly to the Select component. Consider moving these styles to a CSS class or using styled components to keep styling centralized.
  • 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/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:295
  • Draft comment:
    The string 'body' is hardcoded in multiple places (e.g. when setting a default attribute key). Consider extracting it into a constant to avoid duplication and potential typos in the future.
  • 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/hooks/queryBuilder/useOptions.ts:64
  • Draft comment:
    The parameters 'results' and 'result' are very similar in name, which can be confusing. Consider renaming them to more descriptive and distinct names to clarify their different roles.
  • 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.
8. frontend/src/hooks/queryBuilder/useOptions.ts:141
  • Draft comment:
    The effect conditionally calls setOptions(newOptions) only when newOptions is non-empty. This might prevent the options state from being cleared when no options are available. Consider updating the state unconditionally with newOptions.
  • 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.
9. frontend/src/container/QueryBuilder/components/Query/Query.tsx:145
  • Draft comment:
    Typo: The function name 'handleToggleCollapsQuery' is likely a misspelling. It should probably be 'handleToggleCollapseQuery' to correctly reflect its purpose.
  • 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.
10. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:389
  • Draft comment:
    Typo: In the comment on line 389, 'seperated' should be corrected to 'separated'.
  • 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.
11. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:827
  • Draft comment:
    Typo: In the comment on line 827, 'sycn' should be corrected to 'sync'.
  • 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_NIrF5VUBkndIX5A6


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

👍 Looks good to me! Incremental review on 7be90f4 in 1 minute and 8 seconds

More details
  • Looked at 43 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:19
  • Draft comment:
    Snapshot tests are useful but can lead to brittle tests. Ensure these snapshots are maintained and updated intentionally when UI changes.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:8
  • Draft comment:
    Test descriptions and conditions are clear, ensuring that 'span-scope-selector' is rendered only for TRACES data source.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:19
  • Draft comment:
    Using a full snapshot test here (container.toMatchSnapshot) may be brittle as minor UI changes could require frequent snapshot updates. Consider adding (or replacing with) more targeted assertions if full snapshot validation isn’t critical.
  • 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 test already has a specific assertion checking for the presence of the component using getByTestId. The snapshot test provides additional coverage for the full rendered output. Having both types of tests is actually a good practice - snapshots for catching unexpected changes and specific assertions for core functionality. The comment seems to ignore that targeted assertions are already present.
    The comment raises a valid general point about snapshot tests being brittle. Maybe there are more specific assertions we could add beyond just component presence.
    While snapshot brittleness is a real concern, this test already follows best practices by combining snapshot and targeted assertions. The comment doesn't provide specific suggestions for additional assertions.
    Delete the comment as it doesn't acknowledge the existing targeted assertions and doesn't provide specific actionable improvements.

Workflow ID: wflow_0frv1iDMQ6f4F0ZK


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

eKuG
eKuG previously approved these changes Apr 8, 2025
Copy link
Contributor

@eKuG eKuG left a comment

Choose a reason for hiding this comment

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

Functionality LGTM

sawhil
sawhil previously approved these changes Apr 8, 2025
Copy link
Member

@sawhil sawhil left a comment

Choose a reason for hiding this comment

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

Minor query. Rest everything looks fine.
Approving for now

@ahmadshaheer ahmadshaheer dismissed stale reviews from sawhil and eKuG via a79c8c8 April 8, 2025 14:10
@ahmadshaheer ahmadshaheer requested review from eKuG and sawhil April 8, 2025 14:11
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.

👍 Looks good to me! Incremental review on a79c8c8 in 2 minutes and 6 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:702
  • Draft comment:
    Using tagKey.trim().length > 0 instead of !isEmpty(tagKey) is clearer for checking non-empty strings. The change from using isEqual(val.key, tagKey) to val.key === tagKey is appropriate here since tagKey is a primitive string. Ensure that tagKey is always a string to avoid unexpected type coercions.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:702
  • Draft comment:
    Replacing lodash's isEmpty(tagKey) with tagKey.trim().length > 0 ensures that strings containing only whitespace are correctly treated as empty. Also, using strict equality (===) instead of isEqual for comparing tag keys is simpler and more performant when the keys are expected to be strings. Please confirm that tagKey is always a string and that this behavior matches the intended handling of whitespace.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
    The comment makes valid technical points about the changes:
  • Using trim().length > 0 is more correct for handling whitespace-only strings than isEmpty()
  • Using === is more performant than isEqual() for simple string comparison
    However, the comment ends by asking the author to confirm assumptions, which violates our rules about not asking for confirmations.
    The technical feedback is good, but the comment's request for confirmation makes it less actionable. Also, since the author made these improvements themselves, maybe they already know this.
    While the technical points are valid, the author has already implemented these improvements, so the comment is more explanatory than actionable. The request for confirmation should be removed.
    The comment should be deleted because it is primarily explaining changes the author already made and includes a non-actionable request for confirmation.
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:392
  • Draft comment:
    Typo: 'seperated' should be 'separated' in the comment describing comma separated values.
  • 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/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:835
  • Draft comment:
    Typo: 'sycn' should be corrected to 'sync' in the comment ensuring that tags stay in sync with the current query.
  • 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_XSlbo9FPt0FUEjLl


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

sawhil
sawhil previously approved these changes Apr 9, 2025
eKuG
eKuG previously approved these changes Apr 10, 2025
@ahmadshaheer ahmadshaheer dismissed stale reviews from eKuG and sawhil via 698b8d6 April 16, 2025 11:12
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.

❌ Changes requested. Incremental review on 698b8d6 in 1 minute and 44 seconds

More details
  • Looked at 193 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:123
  • Draft comment:
    Using 'await' with a synchronous function. 'selectOption' does not return a Promise, so 'await' is redundant. Consider removing 'await' for consistency.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The comment is technically correct - the await is unnecessary since selectOption is synchronous. However, this is a test file and the await doesn't cause any actual issues. The test still works correctly either way. This feels like a very minor nitpick that doesn't meaningfully improve code quality.
    The await could be intentional to make the test more readable and consistent with other async tests in the file. Line 157 uses await with screen.findByText() which is async.
    While consistency is important, using await with sync functions can be misleading and confusing to readers. However, this is such a minor issue that commenting on it creates more noise than value.
    Delete the comment. While technically correct, this is too minor of an issue to warrant a PR comment, especially in test code.
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:125
  • Draft comment:
    The test 'should add isRoot filter when selecting ROOT_SPANS' uses 'await selectOption(...)', but selectOption is defined synchronously. Consider removing the async/await if no asynchronous actions are needed.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The selectOption function itself is synchronous, but it's triggering UI updates that might need to settle. React Testing Library's fireEvent can sometimes need time for state updates to propagate. However, if we're just checking mockRedirectWithQueryBuilderData.toHaveBeenCalled(), that should be synchronous since it's just a mock function.
    I might be missing some async behavior in the underlying SpanScopeSelector component that necessitates the await. The component code isn't visible in the diff.
    Without seeing the SpanScopeSelector component code, I can't be 100% certain that the async/await is unnecessary. The author may have had a reason for making it async.
    Since I don't have strong evidence that the async/await is definitely wrong, and removing it could potentially break the test, I should not keep this comment.

Workflow ID: wflow_joKTSUpgo6TXTExc


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

❌ Changes requested. Incremental review on 253c1cf in 3 minutes and 39 seconds

More details
  • Looked at 67 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:281
  • Draft comment:
    Duplicate dependency in useMemo: 'attributeValues?.payload?.stringAttributeValues?.length' is listed twice. Consider removing the duplicate or verifying if a different property was intended.
  • Reason this comment was not posted:
    Marked as duplicate.
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:280
  • Draft comment:
    Duplicate dependency detected: 'attributeValues?.payload?.stringAttributeValues?.length' appears twice in the useMemo dependencies for 'attributeValuesPayload'. Please remove the duplicate to ensure accurate dependency tracking.
  • Reason this comment was not posted:
    Marked as duplicate.
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:713
  • Draft comment:
    Good refactoring: using the memoized 'attributeKeysPayload' instead of direct access improves consistency. However, since the dependency array only tracks the payload’s length, ensure that changes to the payload contents (without length change) are not missed.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The comment raises a valid concern - if the contents of attributeKeys change without the length changing, the memoized value won't update. However, this seems unlikely in practice since attributeKeys are typically added/removed rather than modified in place. The comment is speculative ("ensure that...") and asks for verification rather than pointing out a clear issue.
    The comment identifies a real edge case in the memoization logic. Changes to key properties without length changes could cause stale data.
    While technically correct, the comment violates the rules by being speculative and asking for verification. It doesn't point to a clear issue that needs fixing.
    The comment should be deleted because it is speculative and asks for verification rather than identifying a clear issue that needs to be fixed.
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:789
  • Draft comment:
    Consistent use of the memoized 'attributeValuesPayload' is a clear improvement. Verify that using only the lengths of the nested arrays as dependencies is sufficient, as content updates that don’t change the array lengths might not be captured.
  • 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 verify their implementation, which violates the rule against asking for confirmation or verification. It does not provide a specific suggestion or point out a clear issue with the code.
5. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:403
  • Draft comment:
    Typo: 'seperated' should be corrected to 'separated' in the comment describing comma separated values.
  • 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/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:847
  • Draft comment:
    Typo: 'sycn' should be corrected to 'sync' in the comment that talks about keeping tags in sync with the query.
  • 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_xBlh39zSOjKNzXlr


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

sawhil
sawhil previously approved these changes Apr 17, 2025
@ahmadshaheer ahmadshaheer force-pushed the feat/use-search-v2-component-for-traces branch from 253c1cf to 0e2bca1 Compare April 21, 2025 04:09
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 0e2bca1 in 47 seconds. Click for details.
  • Reviewed 86 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 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/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/QueryBuilderSearchV2.test.tsx:80
  • Draft comment:
    Avoid using a generic 'as any' here. Providing a more specific type for the QueryBuilderContext value will improve type safety in 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.
2. frontend/jest.config.ts:17
  • Draft comment:
    Ensure the addition of 'isolatedModules: true' is intended. This flag can speed up compilation by skipping type-checks, so verify its effect on test accuracy.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/QueryBuilderSearchV2.test.tsx:92
  • Draft comment:
    Good refactor: extracting duplicate aggregate keys data into a constant improves maintainability and reduces repetition.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/QueryBuilderSearchV2.test.tsx:115
  • Draft comment:
    Similarly, extracting the aggregate values mock data into a constant aids in clarity and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
5. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/QueryBuilderSearchV2.test.tsx:128
  • Draft comment:
    Consider removing the commented-out mock for 'useUrlQuery' if it's no longer in use to keep the test file clean.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_9rzpEGY0JfRIUGyE

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

@ahmadshaheer ahmadshaheer force-pushed the feat/use-search-v2-component-for-traces branch from 0e2bca1 to ac88da8 Compare April 21, 2025 04:14
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 ac88da8 in 36 seconds. Click for details.
  • Reviewed 33 lines of code in 1 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/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:40
  • Draft comment:
    Good improvement: using TagFilterItem instead of any[] for createQueryWithFilters enhances type safety.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:63
  • Draft comment:
    Consider replacing the use of 'as any' for the context value with a more specific type to improve type clarity.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:40
  • Draft comment:
    Good change – typing the filters parameter as TagFilterItem[] instead of any[] improves type safety.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:92
  • Draft comment:
    Nice update typing updatedQuery as Query in assertFilterAdded instead of any, which clarifies expected structure.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
5. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:127
  • Draft comment:
    The test uses 'await selectOption(...)' while selectOption is defined synchronously. Consider removing the unnecessary await for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
6. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:66
  • Draft comment:
    Consider properly typing the QueryBuilderContext value instead of using 'as any' to avoid ESLint disables and improve type safety in tests.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_SdcH9VRGHDI3Er5v

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

@ahmadshaheer ahmadshaheer requested review from sawhil and eKuG April 21, 2025 08:59
Copy link
Contributor

@eKuG eKuG left a comment

Choose a reason for hiding this comment

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

Functionality LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs required enhancement New feature or request
Projects
None yet
3 participants