-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(explore): Migrate traces to use query params for sorts #101127
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(explore): Migrate traces to use query params for sorts #101127
Conversation
}); | ||
|
||
act(() => setSortBys([{field: 'max(span.duration)', kind: 'desc'}])); | ||
act(() => setAggregateSortBys([{field: 'max(span.duration)', kind: 'desc'}])); |
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.
Bug: Aggregate Tests Use Incorrect Sort Method
Several aggregate mode tests incorrectly use setSortBys
to update sort parameters. After recent changes, setSortBys
now controls sample sorts, so these tests should instead call setAggregateSortBys
to update the correct state.
Additional Locations (3)
: writableQueryParams.sortBys?.map( | ||
sort => `${sort.kind === 'desc' ? '-' : ''}${sort.field}` | ||
) | ||
); |
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.
Bug: Query Parameters Handle Nulls and Undefines Differently
The sortBys
and aggregateSortBys
query parameter updates might handle null
and undefined
inputs inconsistently. While null
inputs explicitly result in null
, undefined
inputs would pass through optional chaining (?.map()
) and yield undefined
for updateNullableLocation
, potentially causing unexpected behavior.
Additional Locations (1)
const [sorts, setSorts] = | ||
mode === Mode.SAMPLES | ||
? [sampleSortBys, setSampleSortBys] | ||
: [aggregateSortBys, setAggregateSortBys]; |
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.
No description provided.