Skip to content

replace hardcoded panel selection indices in CompareV2 metrics#13277

Open
pnaik1 wants to merge 1 commit intokubeflow:masterfrom
pnaik1:13270-issue
Open

replace hardcoded panel selection indices in CompareV2 metrics#13277
pnaik1 wants to merge 1 commit intokubeflow:masterfrom
pnaik1:13270-issue

Conversation

@pnaik1
Copy link
Copy Markdown
Contributor

@pnaik1 pnaik1 commented Apr 15, 2026

Closes: #13270

Summary

  • Replace hardcoded [0]/[1] panel indices in MetricsDropdown with a generic array-based selectedItems state
  • Extract a shared COMPARE_PANEL_COUNT constant from CompareUtils.ts to eliminate magic 2 literals in CompareV2
  • Add runId to SelectedItem and id to DropdownItem so artifact lookup uses stable run_id identity instead of display_name
  • Refactor getVerifiedTwoPanelSelection in CompareV2 to use Set-based lookups with run_id-first, display_name fallback
  • Add a middleCell CSS class in MetricsDropdown to correctly style any panel that is neither the first nor last
  • Add a new test covering runId-based artifact resolution in MetricsDropdown

Why

The previous implementation hardcoded panel positions as [0] and [1] throughout MetricsDropdown and
CompareV2, making it fragile and difficult to extend. Specifically:

  • MetricsDropdown maintained separate firstSelectedItem / secondSelectedItem state variables, meaning
    adding a third panel would require duplicating state and handler logic
  • getVerifiedTwoPanelSelection iterated run artifacts comparing by display_name, which is not a stable
    identifier — runs with the same name could resolve to the wrong artifact
  • areSelectedArtifactsEqual did not account for runId, so selection equality checks could produce false
    positives

This change makes the panel count driven by a single constant, loops over panels uniformly, and anchors
artifact resolution to run_id for correctness.

Verification

cd frontend && fnm exec --using .nvmrc -- npx prettier@3.8.1 --check
src/components/TwoLevelDropdown.tsx
src/components/viewers/MetricsDropdown.tsx
src/components/viewers/MetricsDropdown.test.tsx
src/lib/v2/CompareUtils.ts
src/pages/CompareV2.tsx

cd frontend && fnm exec --using .nvmrc -- npm run test:ui --
src/components/viewers/MetricsDropdown.test.tsx
src/pages/CompareV2.test.tsx

cd frontend && fnm exec --using .nvmrc -- npm run typecheck
cd frontend && fnm exec --using .nvmrc -- npm run lint

Checklist:

Copilot AI review requested due to automatic review settings April 15, 2026 09:48
@google-oss-prow
Copy link
Copy Markdown

Hi @pnaik1. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Copy Markdown
Contributor

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

Refactors CompareV2 metrics selection to remove hardcoded two-panel indexing and to resolve selected artifacts using stable run_id identity (with display_name fallback for older selections). This addresses #13270 by making panel-related logic more uniform and less fragile.

Changes:

  • Introduces a shared COMPARE_PANEL_COUNT constant and replaces magic 2 literals in CompareV2 initialization.
  • Migrates MetricsDropdown selection state to an array-driven approach (instead of separate first/second states) and adds a runtime guard for unexpected panel counts.
  • Adds runId/id plumbing through TwoLevelDropdown and updates artifact verification/equality to account for runId.

Reviewed changes

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

Show a summary per file
File Description
frontend/src/pages/CompareV2.tsx Uses COMPARE_PANEL_COUNT, adds runId to selection equality, and refactors selection verification to prefer run_id.
frontend/src/lib/v2/CompareUtils.ts Exports COMPARE_PANEL_COUNT to centralize compare panel sizing.
frontend/src/components/viewers/MetricsDropdown.tsx Reworks selection handling to be array-based, adds middleCell, and resolves artifacts via runId when available.
frontend/src/components/viewers/MetricsDropdown.test.tsx Updates/extends tests and adds a new test intended to cover runId-based resolution.
frontend/src/components/TwoLevelDropdown.tsx Extends dropdown selection payload to include an optional runId sourced from an optional item id.

Comment thread frontend/src/components/viewers/MetricsDropdown.test.tsx
Comment thread frontend/src/components/viewers/MetricsDropdown.tsx
Comment thread frontend/src/pages/CompareV2.tsx Outdated
@manaswinidas
Copy link
Copy Markdown
Contributor

/ok-to-test

@github-actions github-actions Bot added the ci-passed All CI tests on a pull request have passed label Apr 15, 2026
@github-actions github-actions Bot removed the ci-passed All CI tests on a pull request have passed label Apr 15, 2026
@pnaik1
Copy link
Copy Markdown
Contributor Author

pnaik1 commented Apr 15, 2026

Panel validation bug using display name (discovered during investigation)
getVerifiedTwoPanelSelection in CompareV2.tsx validated whether a selected panel's run was still in the active run list by matching on display_name. If two runs shared the same display name, the validation logic couldn't distinguish them — Panel could be incorrectly updated (Addressed in the PR)
Before:

Screen.Recording.2026-04-15.at.3.17.06.PM.mov

After:

Screen.Recording.2026-04-15.at.3.52.17.PM.mov

Copy link
Copy Markdown
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

Some frontend e2e tests are failing - can you look into them ?

@pnaik1
Copy link
Copy Markdown
Contributor Author

pnaik1 commented Apr 15, 2026

@manaswinidas I think this is unrelated to my changes, I believe more of infra issue

error

@manaswinidas
Copy link
Copy Markdown
Contributor

/retest

@github-actions github-actions Bot added the ci-passed All CI tests on a pull request have passed label Apr 15, 2026
@github-actions github-actions Bot removed the ci-passed All CI tests on a pull request have passed label Apr 16, 2026
@google-oss-prow
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from manaswinidas. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: pnaik1 <pnaik@redhat.com>
@pnaik1
Copy link
Copy Markdown
Contributor Author

pnaik1 commented Apr 16, 2026

/retest

@github-actions github-actions Bot added the ci-passed All CI tests on a pull request have passed label Apr 16, 2026
Copy link
Copy Markdown
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

/lgtm

but the branch has conflicts now that #13282 is merged

@google-oss-prow google-oss-prow Bot added the lgtm label Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-passed All CI tests on a pull request have passed lgtm ok-to-test size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

frontend: replace hardcoded [0]/[1] panel indices in CompareV2 metrics selection

3 participants