8000 feat: added the ability to select tags for multiple tags by Gmin2 · Pull Request #2254 · comet-ml/opik · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: added the ability to select tags for multiple tags #2254

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: added the ability to select tags for multiple tags #2254

wants to merge 4 commits into from

Conversation

Gmin2
Copy link
Contributor
@Gmin2 Gmin2 commented May 25, 2025 •
edited
Loading

Details

Added the ability to Tag multiple traces at once within a Project

Issues

Resolves #1009

Testing

Screencast.from.2025-05-25.16-52-38.mp4

/claim #1009

@Gmin2 Gmin2 requested a review from a team as a code owner May 25, 2025 11:23
This was referenced May 25, 2025
Open
Open
@vincentkoc
Copy link
Collaborator

@Gmin2 will review as soon as possible. Thanks for your contribution Can you verbally agree to the CLA.md with "i accept the CLA" in a comment please.

@Gmin2
Copy link
Contributor Author
Gmin2 commented May 25, 2025

i accept the CLA

@vincentkoc
Copy link
Collaborator

@Gmin2 thanks. Changes look good, thanks for the video also. Will get one of the product engineers to take a look and review over the next few days.

aadereiko
aadereiko requested changes May 27, 2025
View reviewed changes
Copy link
Collaborator
@aadereiko aadereiko left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! The changes look good. There's just one thing I'd like to change: the way the success message is displayed.

@aadereiko aadereiko requested a review from Copilot May 27, 2025 08:24
Copilot
Copilot AI reviewed May 27, 2025
View reviewed changes
Copy link
Contributor
@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces the ability to add tags to multiple traces (or spans) within a project in one go. Key changes include:

  • Adding a clear row selection callback in TracesSpansTab to reset selections.
  • Integrating an AddTagDialog component in the TracesActionsPanel with a new optional onClearSelection prop.
  • Creating the AddTagDialog component to handle batch tag additions via individual mutation calls.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
apps/opik-frontend/src/components/pages/TracesPage/TracesSpansTab/TracesSpansTab.tsx Added a clear selection callback and passed it to child export component.
apps/opik-frontend/src/components/pages/TracesPage/TracesSpansTab/TracesActionsPanel.tsx Updated import order, added AddTagDialog integration, and included an onClearSelection prop.
apps/opik-frontend/src/components/pages-shared/traces/AddTagDialog/AddTagDialog.tsx Introduced a new dialog component for adding tags to multiple rows with individual mutation calls.

@Gmin2 Gmin2 force-pushed the multi-trace-tag branch from 341e5b0 to cb1a09d Compare May 27, 2025 08:34
aadereiko
aadereiko requested changes May 27, 2025
View reviewed changes
Copy link
Collaborator
@aadereiko aadereiko left a comment

Choose a reason for hiding this comment

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

We've discussed it with our FE team and decided that we need to do updates of traces and spans in batches here. So, it will likely require the BE work as well. Will you consider doing it?

@Gmin2
Copy link
Contributor Author
Gmin2 commented May 27, 2025

We've discussed it with our FE team and decided that we need to do updates of traces and spans in batches here. So, it will likely require the BE work as well. Will you consider doing it?

yes, can yu share how you have plan to do that?

@aadereiko
Copy link
Collaborator

@Gmin2

yes, can yu share how you have plan to do that?

So, implementing the back-end part to support batch updates should also be included in this PR and should be done by the person seeking the bounty. Could you implement the back-end part?

@vincentkoc vincentkoc requested a review from aadereiko May 30, 2025 08:22
@aadereiko
Copy link
Collaborator

@Gmin2
We've discussed this PR within our team and realized that the backend changes were not part of the initial requirements. We'll handle those improvements ourselves later. On your side, you can simply add a limit of up to 10 traces.

Also, you have some eslint issue:

Error:    49:29  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
Error:    68:13  error  Insert `,`                                prettier/prettier
Error:    85:[13](https://github.com/comet-ml/opik/actions/runs/15342488982/job/43181043429?pr=2254#step:5:14)  error  Insert `,`                                prettier/prettier
Error:   105:15  error  'error' is defined but never used         @typescript-eslint/no-unused-vars
Error:   [14](https://github.com/comet-ml/opik/actions/runs/15342488982/job/43181043429?pr=2254#step:5:15)2:29  error  Insert `⏎`                                prettier/prettier

Please, before submitting a PR, run npm run lint from opik-frontend repo to see what stylistic error you need to fix :)

@Gmin2 Gmin2 force-pushed the multi-trace-tag branch from fdba3f4 to 1f68589 Compare June 3, 2025 10:16
@Gmin2
Copy link
Contributor Author
Gmin2 commented Jun 3, 2025

Resolved the linting issues !

@Gmin2
Copy link
Contributor Author
Gmin2 commented Jun 11, 2025

can i get approved on this?

8000

@aadereiko
Copy link
Collaborator

hey @Gmin2 , could you please add limits of adding tags to up to 10 entities and we can approve?

@Gmin2 Gmin2 force-pushed the multi-trace-tag branch from b491d85 to c8ad281 Compare June 13, 2025 05:47
@Gmin2
Copy link
Contributor Author
Gmin2 commented Jun 13, 2025

hey @Gmin2 , could you please add limits of adding tags to up to 10 entities and we can approve?

hey @aadereiko just done that could you review it once !

@aadereiko
Copy link
Collaborator

@Gmin2
we'll add error handling on our end and merge this PR soon, thanks! I guess your work here is done
FYI @vincentkoc

@Gmin2
Copy link
Contributor Author
Gmin2 commented Jun 18, 2025

@Gmin2 we'll add error handling on our end and merge this PR soon, thanks! I guess your work here is done FYI @vincentkoc

cc @vincentkoc

@vincentkoc
Copy link
Collaborator

@Gmin2 yes I have seen this. Wont be long once the team make the additional changes and this PR is merged you will be paid out. Easier to keep to this approach, but as discussed consider this one locked in. If any delays on our side I will process the bounty manually.

@aadereiko aadereiko self-assigned this Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@aadereiko aadereiko Awaiting requested review from aadereiko

Requested changes must be addressed to merge this pull request.

Assignees

@aadereiko aadereiko

Labels
🙋 Bounty claim
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR]: UI - Ability to Tag multiple traces at once within a Project
3 participants
@Gmin2 @vincentkoc @aadereiko
0