Conversation
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
📋 PR Linter Failed❌ Incomplete Issues Section. You must reference at least one GitHub issue ( |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
75394f1 to
d03d87f
Compare
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
d03d87f to
17b0102
Compare
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
| <div className="pt-4"> | ||
| <div className="mb-1 flex items-center justify-between"> | ||
| <h1 className="comet-title-l">Alerts</h1> | ||
| <h1 className="comet-title-xs">Alerts</h1> | ||
| {canUpdateAlerts && ( | ||
| <Button variant="default" size="xs" onClick={handleNewAlertClick}> | ||
| <Plus className="mr-1 size-4" /> | ||
| Create alert | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
Header block is duplicated across list pages with only title/button/onClick differing — should we extract it into a shared <PageSectionHeader title="..." action={{ label, icon, onClick }} />?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🌙 Nightly cleanup: The test environment for this PR ( |
1 similar comment
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
17b0102 to
84cb0b2
Compare
JetoPistola
left a comment
There was a problem hiding this comment.
Reviewed the v2 page-title / Create-button standardization. No correctness issues — just a few consistency nits flagged inline.
🤖 Review posted via /review-github-pr
| /> | ||
| <h1 className="comet-title-xs truncate break-words">Experiments</h1> | ||
| <Button variant="outline" size="xs" onClick={handleNewExperimentClick}> | ||
| <Info className="mr-1 size-3.5" /> |
There was a problem hiding this comment.
💡 suggestion | Consistency
Every other "Create" button standardized in this PR (Projects, Prompts, Dashboards, Annotation queues, Alerts, Online evaluation rules, Feedback definitions) uses the Plus icon. The Info icon here looks like a leftover from the previous button — switching to Plus would match the rest.
| <Info className="mr-1 size-3.5" /> | |
| <Plus className="mr-1 size-4" /> |
(also update the lucide-react import on line 3 to use Plus instead of Info)
🤖 Review posted via /review-github-pr
There was a problem hiding this comment.
Looks uglier with size-4, but a good comment :)
| return ( | ||
| <div className="pt-6"> | ||
| <div className="pt-4"> | ||
| <div className="mb-1 flex items-center justify-between"> |
There was a problem hiding this comment.
💡 suggestion | Consistency
This page kept mb-1 on the title row (and the ExplainerDescription block on line 343 below the title), while every other list page in this PR was migrated to mb-4 and had its explainer block removed (e.g. ProjectsPage, PromptsPage, DashboardsPage, AnnotationQueuesPage, OnlineEvaluationPage). Was this intentional, or should AlertsPage follow the same pattern?
🤖 Review posted via /review-github-pr
| {noData && canCreateDashboards && ( | ||
| <Button variant="link" onClick={handleNewDashboardClick}> | ||
| Create new dashboard | ||
| <Plus className="mr-2 size-4" /> |
There was a problem hiding this comment.
🧹 nit | Style
The header "Create dashboard" button on this page (and across the PR) uses mr-1 for the icon spacing — only the no-data variant here uses mr-2.
| <Plus className="mr-2 size-4" /> | |
| <Plus className="mr-1 size-4" /> |
🤖 Review posted via /review-github-pr
There was a problem hiding this comment.
Commit e0d06b0 addressed this comment by updating the icon spacing on the no-data “Create dashboard” button from mr-2 to mr-1, matching the rest of the header buttons.
| <h1 className="comet-title-l">Optimize a prompt</h1> | ||
| <h1 className="comet-title-xs">Optimize a prompt</h1> | ||
| <div className="flex gap-2"> | ||
| <Button variant="outline" size="sm" onClick={onCancel}> |
There was a problem hiding this comment.
🧹 nit | Style
Title moved to comet-title-xs but the adjacent Cancel/submit buttons still use size="sm". The new convention in this PR pairs the xs title with size="xs" action buttons.
🤖 Review posted via /review-github-pr
There was a problem hiding this comment.
That was required by design
| @@ -57,13 +57,13 @@ describe("TracesTabRedirect", () => { | |||
| it("redirects ?tab=insights to /insights", () => { | |||
There was a problem hiding this comment.
🧹 nit | Tests
Test descriptions on lines 57, 63, 93, and 158 still read "to /insights" but the assertions now check /dashboards. The it(...) strings should be updated so the test names match what they verify.
🤖 Review posted via /review-github-pr
There was a problem hiding this comment.
Commit e0d06b0 addressed this comment by updating the four it(...) descriptions to mention /dashboards, matching the assertions for those redirects.
| } | ||
|
|
||
| // Legacy ?view=dashboards → insights | ||
| // Legacy ?view=dashboards → dashboards |
There was a problem hiding this comment.
🧹 nit | Comment
With the route renamed, this reads as a tautology (?view=dashboards → dashboards). Consider dropping or clarifying intent, e.g. // Legacy ?view=dashboards param from the old Insights tab.
🤖 Review posted via /review-github-pr
There was a problem hiding this comment.
Commit e0d06b0 addressed this comment by removing the tautological comment so the code no longer contains the confusing remark about ?view=dashboards.
JetoPistola
left a comment
There was a problem hiding this comment.
LGTM — consistent sweep across v2 pages. Inline nits are non-blocking.
🤖 Approved via Claude Code
JetoPistola
left a comment
There was a problem hiding this comment.
Re-approving — addressed nits look good (mr-1 fix, test names, comment cleanup).
🤖 Approved via Claude Code
* [OPIK-5585]: update titles of v2; * eslint issues; * update workspace level components; * eslint issue; * update routing and titles; * conflicts resolve; * update imports; * resize create buttons; * align buttons; * fix pr reviews; --------- Co-authored-by: aadereiko <aliaksandr@comet.com>
Details
Standardize page titles and "Create new" button placement across all v2 pages, and rename InsightsPage to ProjectDashboardsPage.
Moved "Create new" buttons from the toolbar row to the page title section (right-aligned next to the title) on all list pages:
Removed
ExplainerDescription/ExplainerCalloutblocks below page titles to reduce vertical padding between the title and page content.Changed title style from
comet-title-ltocomet-title-xswhere applicable.InsightsPage → ProjectDashboardsPage rename
Renamed the project-scoped "Insights" page to "Dashboards" to align naming with the workspace-level Dashboards page:
InsightsPage/→ProjectDashboardsPage/InsightsPage.tsx,InsightsViewSelector.tsx,InsightsViewDialog.tsx,InsightsViewItems.tsx→ProjectDashboardsPage.tsx,ProjectDashboardViewSelector.tsx,ProjectDashboardViewDialog.tsx,ProjectDashboardViewItems.tsxInsights*prefixes renamed toProjectDashboard*insightsRoute→projectDashboardsRoute, URL path/insights→/dashboards, title "Insights" → "Dashboards"/dashboardsTracesTabRedirectupdated to redirect?tab=insights,?tab=metrics, and?view=dashboardsto/dashboardsAgentConfigurationTab
Added page title "Agent configuration" (
comet-title-xs) and removed the "Agent configuration" and "Version history" sub-headers.Files changed
Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
npm run lintnpm run typecheckDocumentation