-
Notifications
You must be signed in to change notification settings - Fork 95
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
add feature of auto filter when clicking on property #317
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.
👍 Looks good to me! Reviewed everything up to 9f6142c in 1 minute and 53 seconds
More details
- Looked at
392
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. frontend/components/ui/datatable-filter.tsx:71
- Draft comment:
CallinghandleApplyFilters
insideuseEffect
can cause an infinite loop if it updatesactiveFilters
. Consider refactoring to avoid this. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/components/traces/sessions-table.tsx:126
- Draft comment:
ThehandleAddFilter
function is repeated across multiple files. Consider refactoring it into a shared utility function to adhere to the DRY principle. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While code duplication is a valid concern, I cannot verify if this function is actually duplicated since I don't have access to other files. The comment makes an assumption about code in other files that I cannot validate. Following the rules, I should not keep comments that require more context to verify.
The function does look like a generic utility that could be reused, so the suggestion isn't completely unreasonable. Maybe there is duplication that would benefit from refactoring.
Without being able to verify the duplication claim, keeping this comment would be speculative. The rules clearly state we need strong evidence to keep a comment.
Delete the comment since we cannot verify the claim about code duplication across files without access to those files.
3. frontend/components/traces/spans-table.tsx:165
- Draft comment:
ThehandleAddFilter
function is repeated across multiple files. Consider refactoring it into a shared utility function to adhere to the DRY principle. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/components/traces/traces-table.tsx:176
- Draft comment:
ThehandleAddFilter
function is repeated across multiple files. Consider refactoring it into a shared utility function to adhere to the DRY principle. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/components/traces/sessions-table.tsx:126
-
Draft comment:
This filter management logic could be extracted to a shared utility function. -
function
handleAddFilter
(spans-table.tsx) -
Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_G3YnjwSVotPxwKeb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Hey @nidhi-wa can you record a video of the new functionality? I couldn't get this to work. Perhaps, you may need things like |
Also, we'd probably need some kind of visual indication that a cell is clickable, not sure exactly, but AWS for example uses dotted underline text |
Based on your suggestions I've made changes. Included a recording of functionality showcasing how it works in action . Let me know your thoughts. 20.01.2025_16.52.39_REC.mp4 |
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 overall. Please resolve the conflicts. I think, I've removed the swr fetcher bit from the traces table. Also @skull8888888 what do you think of the visuals? I need your input
@nidhi-wa Thank you for the PR! Looks good, although I would remove primary color and underline. |
@skull8888888 @dinmukhamedm Thank you for the feedback! Just to clarify, are you suggesting removing the primary color and underline entirely, or do you have an alternative styling recommendation in mind? Let me know so I can make adjustments accordingly! |
How about we remove the primary color and instead show the underline only when the user hovers over the cell? I think this approach could keep the design cleaner while still providing a clear visual for interactivity. Let me know your thoughts! |
@nidhi-wa yes, let's do underline on hover, but without primary color. |
@dinmukhamedm Done |
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.
LGTM
* add feature of auto filter when clicking on property (#317) * add filter when clicking on property * add visual Indication * improved auto filter and visual interaction * fix es-lint error * remove outdated nextjs guide from trace page placeholder (#350) * add spans, datasets count to project card (#347) * eval time progression (#210) * initial work to compare evals * remove unnecessary div * design --------- Co-authored-by: Din <[email protected]> * add migration for indexing datasets on projectId * feat: add spans, datasets count to project card * feat: add evals count, move stats to separate query * fix: linter * fix: small refactor * run migration, address own nits * feat: make stats per project * fix: fix prettier, fix clickhouse param in functions, fix spacings --------- Co-authored-by: Dinmukhamed Mailibay <[email protected]> Co-authored-by: skull8888888 <[email protected]> Co-authored-by: Din <[email protected]> Co-authored-by: Nidhi Singh <[email protected]> * fix: sqlx query in dataset fetching (#351) --------- Co-authored-by: Nidhi Singh <[email protected]> Co-authored-by: Olzhas Nurpeisov <[email protected]> Co-authored-by: skull8888888 <[email protected]>
This PR implements the Auto Filter feature, enabling users to apply filters automatically when clicking on cells in the Trace , Span and Session Tab.
#192
Important
Add auto-filter feature to
sessions-table.tsx
,spans-table.tsx
, andtraces-table.tsx
enabling filter application by clicking on table cells.sessions-table.tsx
,spans-table.tsx
, andtraces-table.tsx
allowing users to apply filters by clicking on table cells.id
,span_type
,name
,top_span_type
, andtop_span_name
.handleAddFilter
andhandleUpdateFilters
functions added to manage filter state insessions-table.tsx
,spans-table.tsx
, andtraces-table.tsx
.DataTableFilter
indatatable-filter.tsx
updated to handle active filters and apply them to URL search parameters.handleApplyFilters
function indatatable-filter.tsx
updates URL with new filters and resets pagination.This description was created by
for 9f6142c. It will automatically update as commits are pushed.