-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: update the auto-complete API reading logic #7254
base: main
Are you sure you want to change the base?
fix: update the auto-complete API reading logic #7254
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 everything up to 5770e9d in 2 minutes and 51 seconds
More details
- Looked at
116
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. frontend/src/constants/queryBuilder.ts:422
- Draft comment:
Good use of a mapping constant. Ensure consistency by using design tokens if applicable. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. frontend/src/pages/MessagingQueues/MQGraph/useGetAllConfigOptions.ts:37
- Draft comment:
Consider using the DATA_TYPE_VS_ATTRIBUTE_VALUES_KEY mapping instead of Object.values to improve clarity and consistency. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. frontend/src/pages/TracesExplorer/Filter/filterUtils.ts:331
- Draft comment:
Good update using DATA_TYPE_VS_ATTRIBUTE_VALUES_KEY; ensure the cast on keyData.dataType is safe. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. frontend/src/hooks/queryBuilder/useFetchKeysAndValues.ts:240
- Draft comment:
The specific setResults call using DATA_TYPE_VS_ATTRIBUTE_VALUES_KEY is immediately overwritten by a generic setResults call. Use an else clause so the specialized results aren’t lost. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/src/pages/MessagingQueues/MQGraph/useGetAllConfigOptions.ts:37
- Draft comment:
Using a hardcoded 'stringAttributeValues' here; ensure this is intentional. For consistency, consider using the DATA_TYPE_VS_ATTRIBUTE_VALUES_KEY mapping if multiple data types are supported. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. frontend/src/pages/TracesExplorer/Filter/filterUtils.ts:332
- Draft comment:
Casting keyData.dataType as Partial may hide potential type issues if keyData.dataType is missing or unexpected. Ensure it’s always valid before using the mapping. - 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 comment suggests there could be type issues, but examining the code shows keyData.dataType is already strictly typed through the traceFilterKeys const object. The cast seems unnecessary but not harmful. The comment is speculative about potential issues rather than pointing out a concrete problem.
I could be wrong about the type safety - there might be places where traceFilterKeys is modified at runtime. The DataTypes enum might be extended elsewhere.
The traceFilterKeys is defined as a const object with explicit types, and TypeScript would catch any type mismatches at compile time. Runtime modifications would be caught by TypeScript's type system.
The comment should be deleted as it's speculative about potential issues rather than identifying a concrete problem, and the existing type system already provides the safety being discussed.
7. frontend/src/pages/TracesExplorer/Filter/filterUtils.ts:346
- Draft comment:
Calling setFetching during render (in the if(!value) branch) is a side effect. Move this logic into a useEffect to avoid state updates during render. - 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_8cC8wXTKjrCW4fqV
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.
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 cc2fa31 in 1 minute and 8 seconds
More details
- Looked at
12
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/hooks/queryBuilder/useFetchKeysAndValues.ts:245
- Draft comment:
Adding the early return after setting results prevents the fallback logic from running. This ensures that when payload is recognized as an attribute value response, the state is updated only once. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining the effect of an early return in the code. It doesn't suggest any changes or ask for confirmation of intent, which violates the rules.
2. frontend/src/hooks/queryBuilder/useFetchKeysAndValues.ts:245
- Draft comment:
Early return here prevents the fallback logic from running when the payload matches the expected IAttributeValuesResponse structure. Consider adding an inline comment to clarify why this early exit is necessary. - 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 early return's purpose is fairly clear from the context - it's part of a type-specific handling branch. The code is already well-structured with the isAttributeValuesResponse type guard. Adding a comment here would be stating the obvious. The logic flow is straightforward enough that a developer can understand it without additional documentation.
I could be underestimating the complexity for future maintainers. Some developers might find the dual-path logic confusing without explicit documentation.
The code structure itself documents the intent - there's a clear type check and type-specific handling followed by fallback logic. Additional comments would be redundant.
This comment should be deleted as it requests documentation for something that is already clear from the code structure and context.
Workflow ID: wflow_brH6YuQzraJzH3Ev
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@ahmadshaheer not handled here signoz/frontend/src/components/CeleryTask/CeleryTaskConfigOptions/useGetCeleryFilters.ts Line 71 in 239ea9d
signoz/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx Lines 79 to 84 in 239ea9d
Is there a better way to identify where |
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. Incremental review on 8f38862 in 1 minute and 36 seconds
More details
- Looked at
41
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/hooks/queryBuilder/useFetchKeysAndValues.ts:172
- Draft comment:
The new type guard function 'isMetricsListFilterValuesData' is useful. Ensure that its naming and behavior stay consistent with similar guard functions in this file. Also, consider adding a null/undefined check explicitly if payload might be null. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
2. frontend/src/hooks/queryBuilder/useFetchKeysAndValues.ts:257
- Draft comment:
The additional check for metrics explorer payload (isMetricsListFilterValuesData) is clear. Verify that the ordering of type guard checks is intentional so that payloads for metrics explorer aren’t misinterpreted as attribute values responses. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
3. frontend/src/hooks/queryBuilder/useFetchKeysAndValues.ts:250
- Draft comment:
The payload processing now prioritizes the attribute values check over metrics explorer even when isMetricsExplorer is true. Verify that this ordering is intended, as overlapping payload structures may lead to the metrics branch being skipped. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify the intention behind a change in logic. It points out a potential issue with the ordering of operations, which could lead to unintended behavior. However, it does not provide a specific suggestion or ask for a test to be written. According to the rules, comments should not ask the author to confirm their intention or verify behavior. Therefore, this comment should be removed.
4. frontend/src/hooks/queryBuilder/useFetchKeysAndValues.ts:250
- Draft comment:
Note that the fallback behavior from the previous version (using Object.values to extract a truthy value) has been removed. Confirm if this strict type-based approach covers all expected payload cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to confirm if the new approach covers all expected cases, which violates the rule against asking for confirmation of intention or behavior. It doesn't provide a specific suggestion or point out a clear issue.
Workflow ID: wflow_QMkJnNoZIUu2xxNN
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.
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. Incremental review on 788b11f in 1 minute and 32 seconds
More details
- Looked at
31
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/hooks/queryBuilder/useFetchKeysAndValues.ts:162
- Draft comment:
The updated type guard for IAttributeValuesResponse now strictly checks for arrays or null values, which improves type safety. However, consider adding an early check to ensure that 'payload' is a non-null object before accessing its properties to avoid unexpected runtime errors. - 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/hooks/queryBuilder/useFetchKeysAndValues.ts:175
- Draft comment:
In the isMetricsListFilterValuesData guard, the previous check for a truthy payload was removed. This may lead to runtime errors if a non-object is passed. Consider reintroducing a payload existence check (e.g., 'if (!payload) return false') to ensure robustness. - 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 'in' operator in JavaScript/TypeScript will return false if the left operand is null/undefined, so there's no risk of runtime errors from that. The Array.isArray check will also safely return false for null/undefined. The previous check was actually redundant since the 'in' operator already handles null/undefined cases safely. The simpler version is actually better code.
I could be wrong about the exact behavior of the 'in' operator with null/undefined in all JavaScript environments. There might be edge cases I'm not considering.
The 'in' operator behavior with null/undefined is well-defined in the ECMAScript specification and will consistently return false, making the extra check unnecessary.
The comment should be deleted because the suggested change would add unnecessary code - the current implementation is already safe and more concise.
Workflow ID: wflow_Eu1hHB3hph3nq3NX
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.
788b11f
to
dea9fca
Compare
Done, @srikanthccv, I made the changes. |
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 dea9fca in 10 minutes and 43 seconds
More details
- Looked at
25
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/hooks/queryBuilder/useFetchKeysAndValues.ts:166
- Draft comment:
New type guard checks payload existence and proper array/null conditions. Confirm that allowing null for attribute values (e.g. stringAttributeValues === null) is intended, as this differs from simply checking key existence. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/src/hooks/queryBuilder/useFetchKeysAndValues.ts:164
- Draft comment:
The updated type guard now explicitly checks for array or null on each attribute property. Consider also verifying that 'payload' is a plain object (e.g. usingtypeof payload === 'object' && payload !== null
) to prevent unintended truthy non-object values from passing the check. Also, ensure this stricter logic aligns with your API specs (e.g. if valid responses might sometimes use undefined instead of null). - 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 suggestion to add an explicit object type check is technically valid and would make the type guard more robust. However, the currentpayload &&
check combined with property access would catch most issues. The speculation about API specs and undefined is not helpful. The change is minor and the current code would work correctly in practice.
The suggestion is technically correct but may be overly cautious. The current code would fail appropriately in most edge cases anyway.
While the suggestion would add more type safety, the current implementation is sufficient for real-world usage and the benefit is minimal.
The comment should be deleted as it suggests a minor improvement that isn't clearly necessary, and includes speculative concerns about API specs.
Workflow ID: wflow_hA7qhtYYYPISWFYq
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Related Issues / PR's
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Update attribute value fetching logic across components to use data type mapping for correct attribute key selection.
useGetAllFilters
inuseGetCeleryFilters.ts
to useDATA_TYPE_VS_ATTRIBUTE_VALUES_KEY
for fetching attribute values based on data type.CheckboxFilter
inCheckbox.tsx
to useDATA_TYPE_VS_ATTRIBUTE_VALUES_KEY
for determining attribute values.QueryBuilderSearchV2
inQueryBuilderSearchV2.tsx
to useDATA_TYPE_VS_ATTRIBUTE_VALUES_KEY
for fetching attribute values.DATA_TYPE_VS_ATTRIBUTE_VALUES_KEY
toqueryBuilder.ts
to mapDataTypes
to attribute value keys.useFetchKeysAndValues
to useDATA_TYPE_VS_ATTRIBUTE_VALUES_KEY
for fetching attribute values.useGetAllConfigOptions
to useDATA_TYPE_VS_ATTRIBUTE_VALUES_KEY
for fetching attribute values.useGetAggregateValues
infilterUtils.ts
to useDATA_TYPE_VS_ATTRIBUTE_VALUES_KEY
for fetching attribute values.This description was created by
for dea9fca. It will automatically update as commits are pushed.