Skip to content

Replace run group text input with typeahead, clickable run group label#7176

Merged
openshift-merge-bot[bot] merged 9 commits intoopendatahub-io:mainfrom
nananosirova:RHOAIENG-57471-tentative
Apr 15, 2026
Merged

Replace run group text input with typeahead, clickable run group label#7176
openshift-merge-bot[bot] merged 9 commits intoopendatahub-io:mainfrom
nananosirova:RHOAIENG-57471-tentative

Conversation

@nananosirova
Copy link
Copy Markdown
Contributor

@nananosirova nananosirova commented Apr 10, 2026

Description

Replaces the free-text run group input on the create/duplicate run form with a typeahead selector dropdown, and makes run group labels clickable in run tables to filter by group.

Screen.Recording.2026-04-10.at.1.38.19.AM.mov
Screen.Recording.2026-04-10.at.1.39.45.AM.mov
image

How Has This Been Tested?

  1. Create or duplicate a run — confirm the "Run group" field is a typeahead dropdown that lists existing run groups, and the "Create run group" modal uses "run group" terminology
  2. In the active runs table, click a run group label — verify its filtered by that group
  3. In the archived runs table, click a run group label — verify it filters to the archived view for that group
  4. In compare runs or manage runs, click a run group label — verify it filters the in-page table by run group name
  5. In the schedules table, click a run group label — verify it navigates to the runs page filtered by that group

Test Impact

Tested manually.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)
  • The code follows our Best Practices (React coding standards, PatternFly usage, performance considerations)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

Summary by CodeRabbit

  • New Features

    • Run group names are now clickable in tables and lists; clicking updates filters or navigates to filtered views.
    • Run creation and toolbar filters use a selector control for run group selection instead of freeform text.
  • Bug Fixes

    • Run-group filtering now matches run group names by exact equality to avoid ambiguous results.
  • Style

    • UI wording updated from "experiment" to "run group" across labels, buttons, placeholders, and descriptions.
  • Tests

    • E2E and unit tests updated to use and assert the run group selector and exact-match behavior.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 10, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR renames UI copy from “experiment” to “run group” in multiple components, replaces free-text run-group inputs with an Experiment/Run-Group selector, exposes LEGACY_EXPERIMENT_FILTER_PARAM as an exported constant, adds an optional onRunGroupClick callback to pipeline run row components and wires it through table renderers, updates routing to set legacy experiment query params when run-group labels are clicked, and updates Cypress tests and page objects to use the selector-based run-group control.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Security and Quality Issues

  1. Stale React hook dependencies — Several memoized callbacks (navigation and click handlers) depend on values like namespace, runType, experiment, onRunGroupClick, searchParams, and navigate. Missing or incorrect dependency lists can cause stale closures and incorrect navigation/filtering. Action: audit all useCallback/useMemo usages and include all referenced variables. — CWE-665

  2. Unsanitized query param insertion — Code writes experiment.experiment_id directly into URLSearchParams and navigates. If IDs or names can contain unexpected characters, this may break routing or leak unsafe values. Action: validate/encode IDs and enforce expected format via typings or runtime guards before inserting into query strings. — CWE-20, CWE-116

  3. Implicit contract on propagated callback signature — onRunGroupClick is propagated across multiple components with a specific ExperimentKF shape. Signature drift or missing null checks can silently break behavior. Action: strengthen TypeScript signatures to require non-null experiment object where used and add unit tests for the callback propagation path. — CWE-665

  4. Test selector brittleness — Changing data-testid defaults and selector names risks flaky tests if any automation or external scripts still reference old IDs. Action: run full E2E suite, update all test fixtures, and consider maintaining backward-compatible test IDs or a deprecation shim. — CWE-20

  5. Missing input validation for selector-derived values — The selector now maps display_name into filter values; display names may be non-unique or contain edge characters. Action: ensure selectors return canonical unique identifiers for filtering where appropriate, or enforce exact-match resolution and handle ambiguity explicitly. — CWE-20

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately summarizes the main changes: replacing text input with typeahead and making run group labels clickable.
Description check ✅ Passed Description covers what was changed, includes testing steps, UI assets, and completed most self-checklist items; minor unchecked items do not preclude merge.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (5)
frontend/src/pages/pipelines/global/runs/const.ts (1)

3-3: Rename stale exported identifier to match current domain language.

Line 3 still exports experimentRunsPageDescription while the product term is now “run group.” Keep the string and symbol aligned (for example, runGroupRunsPageDescription) to avoid terminology drift across imports and future changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/pages/pipelines/global/runs/const.ts` at line 3, Rename the
exported identifier experimentRunsPageDescription to runGroupRunsPageDescription
to align with current product terminology; update the export name in
frontend/src/pages/pipelines/global/runs/const.ts and then update all
imports/usages that reference experimentRunsPageDescription to import
runGroupRunsPageDescription instead, leaving the string value unchanged to
preserve behavior.
frontend/src/pages/pipelines/global/experiments/compareRuns/ManageRunsTable.tsx (1)

67-72: Dependency array should reference the stable callback, not the object.

filterProps is a new object reference on every render (from usePipelineFilterSearchParams), causing handleRunGroupClick to be recreated unnecessarily. Use the specific callback instead.

Proposed fix
+  const { onFilterUpdate } = filterProps;
   const handleRunGroupClick = React.useCallback(
     (clickedExperiment: ExperimentKF) => {
-      filterProps.onFilterUpdate(FilterOptions.RUN_GROUP, clickedExperiment.display_name);
+      onFilterUpdate(FilterOptions.RUN_GROUP, clickedExperiment.display_name);
     },
-    [filterProps],
+    [onFilterUpdate],
   );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@frontend/src/pages/pipelines/global/experiments/compareRuns/ManageRunsTable.tsx`
around lines 67 - 72, handleRunGroupClick is being re-created every render
because its dependency array references the whole filterProps object; instead
depend on the stable callback. Replace the dependency [filterProps] with the
specific callback (e.g., [filterProps.onFilterUpdate]) or destructure const {
onFilterUpdate } = filterProps and use [onFilterUpdate] so handleRunGroupClick
only changes when the update function changes; keep the body calling
onFilterUpdate(FilterOptions.RUN_GROUP, clickedExperiment.display_name)
unchanged.
frontend/src/concepts/pipelines/content/compareRuns/CompareRunsRunList.tsx (1)

95-100: Same dependency array issue as ManageRunsTable.tsx.

filterToolbarProps is a new object reference each render. Reference the stable onFilterUpdate callback directly.

Proposed fix
+  const { onFilterUpdate } = filterToolbarProps;
   const handleRunGroupClick = React.useCallback(
     (clickedExperiment: ExperimentKF) => {
-      filterToolbarProps.onFilterUpdate(FilterOptions.RUN_GROUP, clickedExperiment.display_name);
+      onFilterUpdate(FilterOptions.RUN_GROUP, clickedExperiment.display_name);
     },
-    [filterToolbarProps],
+    [onFilterUpdate],
   );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/concepts/pipelines/content/compareRuns/CompareRunsRunList.tsx`
around lines 95 - 100, The callback handleRunGroupClick uses filterToolbarProps
in its dependency array but filterToolbarProps is a new object each render
causing unnecessary re-creations; change the dependency to the stable function
itself by referencing filterToolbarProps.onFilterUpdate (or directly
onFilterUpdate) instead of the whole filterToolbarProps object so
React.useCallback depends on the stable onFilterUpdate function; update the
dependency array to [filterToolbarProps.onFilterUpdate] (or [onFilterUpdate] if
you import/receive it directly) while keeping the body: call
onFilterUpdate(FilterOptions.RUN_GROUP, clickedExperiment.display_name) with
ExperimentKF as before.
frontend/src/concepts/pipelines/content/createRun/RunForm.tsx (1)

158-163: CharLimitHelperText is now vestigial for a selector.

With ActiveExperimentSelector, users select from existing run groups rather than typing free text. The character limit helper (line 163) displays length information that users cannot act upon—they're constrained to existing names. Consider removing it or replacing with simpler text since the limit is enforced at run group creation time, not selection time.

Proposed removal
           <ActiveExperimentSelector
             dataTestId="run-group-selector"
             selection={data.runGroup}
             onSelect={(experiment) => onValueChange('runGroup', experiment.display_name)}
           />
-          <CharLimitHelperText limit={NAME_CHARACTER_LIMIT} currentLength={data.runGroup.length} />
         </FormGroup>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/concepts/pipelines/content/createRun/RunForm.tsx` around lines
158 - 163, The CharLimitHelperText component is vestigial for the
ActiveExperimentSelector usage in RunForm: remove the CharLimitHelperText
instance (the JSX line rendering CharLimitHelperText) since users pick existing
run groups via ActiveExperimentSelector (prop selection={data.runGroup}) and
cannot edit length here; if you prefer keeping guidance, replace it with a
static hint string or small caption component (e.g., “Run group name is selected
from existing experiments”) adjacent to ActiveExperimentSelector, but do not
display the dynamic currentLength based on data.runGroup. Ensure any removed
references to NAME_CHARACTER_LIMIT or currentLength props are cleaned up in the
RunForm component.
frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow.tsx (1)

79-105: Replace these useMemo blocks with direct derivation + useCallback for the prop callback.

runGroupLink is a cheap string derivation, and handleRunGroupClick is a callback prop; using useMemo for both adds indirection without clear payoff.

Suggested refactor
-  const runGroupLink = React.useMemo(() => {
-    if (!experiment) {
-      return undefined;
-    }
-
-    return buildLegacyExperimentFilterUrl(
-      runType === PipelineRunType.ARCHIVED
-        ? globalArchivedPipelineRunsRoute(namespace)
-        : globalPipelineRunsRoute(namespace),
-      experiment.experiment_id,
-    );
-  }, [experiment, namespace, runType]);
-  const handleRunGroupClick = React.useMemo(() => {
-    if (!experiment) {
-      return undefined;
-    }
-
-    if (onRunGroupClick) {
-      return () => onRunGroupClick(experiment);
-    }
-
-    if (!runGroupLink) {
-      return undefined;
-    }
-
-    return () => navigate(runGroupLink);
-  }, [experiment, navigate, onRunGroupClick, runGroupLink]);
+  const runGroupLink = experiment
+    ? buildLegacyExperimentFilterUrl(
+        runType === PipelineRunType.ARCHIVED
+          ? globalArchivedPipelineRunsRoute(namespace)
+          : globalPipelineRunsRoute(namespace),
+        experiment.experiment_id,
+      )
+    : undefined;
+
+  const handleRunGroupClick = React.useCallback(() => {
+    if (!experiment) {
+      return;
+    }
+    if (onRunGroupClick) {
+      onRunGroupClick(experiment);
+      return;
+    }
+    if (runGroupLink) {
+      navigate(runGroupLink);
+    }
+  }, [experiment, onRunGroupClick, runGroupLink, navigate]);

As per coding guidelines, “Performance: avoid unnecessary useCallback/useMemo/useRef — React is performant by default. Only use useCallback when the function is passed as a prop, used as a useEffect dependency, or returned from a custom hook.”

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow.tsx`
around lines 79 - 105, The two React.useMemo usages are unnecessary: replace the
computed string runGroupLink with a plain derived const (compute runGroupLink
directly from experiment, namespace, runType using
buildLegacyExperimentFilterUrl and
globalPipelineRunsRoute/globalArchivedPipelineRunsRoute), and replace
handleRunGroupClick with a React.useCallback that depends on [experiment,
navigate, onRunGroupClick, runGroupLink] and returns the same handlers (call
onRunGroupClick(experiment) if provided, otherwise navigate(runGroupLink) if
available, else undefined); keep the same early-return/null checks for
experiment and runGroupLink and reference the existing symbols runGroupLink,
handleRunGroupClick, buildLegacyExperimentFilterUrl, navigate, onRunGroupClick,
experiment, namespace, runType, and PipelineRunType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRowExperiment.tsx`:
- Around line 14-47: The row renders a label that can look clickable even when
no handler exists because showClickableStyles uses the isClickable prop; change
the logic so clickability is derived from the resolved handler only: compute
handleClick (already in PipelineRunTableRowExperiment) and replace
showClickableStyles = isClickable || !!handleClick with showClickableStyles =
!!handleClick, and only pass isClickable: true and onClick: handleClick into the
Label props when handleClick is defined (do not use the incoming isClickable
flag to trigger clickable styling alone).

---

Nitpick comments:
In `@frontend/src/concepts/pipelines/content/compareRuns/CompareRunsRunList.tsx`:
- Around line 95-100: The callback handleRunGroupClick uses filterToolbarProps
in its dependency array but filterToolbarProps is a new object each render
causing unnecessary re-creations; change the dependency to the stable function
itself by referencing filterToolbarProps.onFilterUpdate (or directly
onFilterUpdate) instead of the whole filterToolbarProps object so
React.useCallback depends on the stable onFilterUpdate function; update the
dependency array to [filterToolbarProps.onFilterUpdate] (or [onFilterUpdate] if
you import/receive it directly) while keeping the body: call
onFilterUpdate(FilterOptions.RUN_GROUP, clickedExperiment.display_name) with
ExperimentKF as before.

In `@frontend/src/concepts/pipelines/content/createRun/RunForm.tsx`:
- Around line 158-163: The CharLimitHelperText component is vestigial for the
ActiveExperimentSelector usage in RunForm: remove the CharLimitHelperText
instance (the JSX line rendering CharLimitHelperText) since users pick existing
run groups via ActiveExperimentSelector (prop selection={data.runGroup}) and
cannot edit length here; if you prefer keeping guidance, replace it with a
static hint string or small caption component (e.g., “Run group name is selected
from existing experiments”) adjacent to ActiveExperimentSelector, but do not
display the dynamic currentLength based on data.runGroup. Ensure any removed
references to NAME_CHARACTER_LIMIT or currentLength props are cleaned up in the
RunForm component.

In
`@frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow.tsx`:
- Around line 79-105: The two React.useMemo usages are unnecessary: replace the
computed string runGroupLink with a plain derived const (compute runGroupLink
directly from experiment, namespace, runType using
buildLegacyExperimentFilterUrl and
globalPipelineRunsRoute/globalArchivedPipelineRunsRoute), and replace
handleRunGroupClick with a React.useCallback that depends on [experiment,
navigate, onRunGroupClick, runGroupLink] and returns the same handlers (call
onRunGroupClick(experiment) if provided, otherwise navigate(runGroupLink) if
available, else undefined); keep the same early-return/null checks for
experiment and runGroupLink and reference the existing symbols runGroupLink,
handleRunGroupClick, buildLegacyExperimentFilterUrl, navigate, onRunGroupClick,
experiment, namespace, runType, and PipelineRunType.

In
`@frontend/src/pages/pipelines/global/experiments/compareRuns/ManageRunsTable.tsx`:
- Around line 67-72: handleRunGroupClick is being re-created every render
because its dependency array references the whole filterProps object; instead
depend on the stable callback. Replace the dependency [filterProps] with the
specific callback (e.g., [filterProps.onFilterUpdate]) or destructure const {
onFilterUpdate } = filterProps and use [onFilterUpdate] so handleRunGroupClick
only changes when the update function changes; keep the body calling
onFilterUpdate(FilterOptions.RUN_GROUP, clickedExperiment.display_name)
unchanged.

In `@frontend/src/pages/pipelines/global/runs/const.ts`:
- Line 3: Rename the exported identifier experimentRunsPageDescription to
runGroupRunsPageDescription to align with current product terminology; update
the export name in frontend/src/pages/pipelines/global/runs/const.ts and then
update all imports/usages that reference experimentRunsPageDescription to import
runGroupRunsPageDescription instead, leaving the string value unchanged to
preserve behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 567df94a-f4f8-4d6c-a3df-bb66d7154234

📥 Commits

Reviewing files that changed from the base of the PR and between 26a2a73 and d8c56e4.

📒 Files selected for processing (11)
  • frontend/src/concepts/pipelines/content/compareRuns/CompareRunsRunList.tsx
  • frontend/src/concepts/pipelines/content/createRun/RunForm.tsx
  • frontend/src/concepts/pipelines/content/experiment/CreateExperimentModal.tsx
  • frontend/src/concepts/pipelines/content/experiment/ExperimentSelector.tsx
  • frontend/src/concepts/pipelines/content/experiment/columns.ts
  • frontend/src/concepts/pipelines/content/tables/pipelineRecurringRun/PipelineRecurringRunTableRow.tsx
  • frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow.tsx
  • frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRowExperiment.tsx
  • frontend/src/concepts/pipelines/content/tables/usePipelineFilter.ts
  • frontend/src/pages/pipelines/global/experiments/compareRuns/ManageRunsTable.tsx
  • frontend/src/pages/pipelines/global/runs/const.ts

@caponetto
Copy link
Copy Markdown
Contributor

Two questions:

  1. Should we bring back the selector to the filter too?
image
  1. Does it still make sense to have Description in this modal? I guess users will never see this information
image

@nananosirova
Copy link
Copy Markdown
Contributor Author

nananosirova commented Apr 10, 2026

Thanks @caponetto

  1. We could change it to a typeahead, but the previous version also had a regular text input there (3.3):
image
  1. I thought about it, at the end I decided to leave it cause you can still technically see it in a tooltip when you hover over it in the dropdown:
image

I don't feel too strongly about it though, I can remove it if you want.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 71.07438% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.90%. Comparing base (5b29bd6) to head (c2190d4).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...content/tables/pipelineRun/PipelineRunTableRow.tsx 20.00% 12 Missing ⚠️
...lines/content/experiment/CreateExperimentModal.tsx 30.00% 7 Missing ⚠️
...elineRecurringRun/PipelineRecurringRunTableRow.tsx 33.33% 6 Missing ⚠️
...pts/pipelines/apiHooks/usePipelineRecurringRuns.ts 83.33% 2 Missing ⚠️
...pelines/content/compareRuns/CompareRunsRunList.tsx 71.42% 2 Missing ⚠️
...ipelines/content/experiment/ExperimentSelector.tsx 50.00% 2 Missing ⚠️
...global/experiments/compareRuns/ManageRunsTable.tsx 50.00% 2 Missing ⚠️
...eRecurringRun/PipelineRecurringRunTableToolbar.tsx 66.66% 1 Missing ⚠️
...bles/pipelineRun/PipelineRunTableRowExperiment.tsx 66.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7176      +/-   ##
==========================================
+ Coverage   64.82%   64.90%   +0.08%     
==========================================
  Files        2442     2442              
  Lines       76011    76015       +4     
  Branches    19171    19171              
==========================================
+ Hits        49271    49339      +68     
+ Misses      26740    26676      -64     
Files with missing lines Coverage Δ
frontend/src/api/pipelines/custom.ts 95.40% <100.00%> (+3.18%) ⬆️
...c/concepts/pipelines/content/createRun/RunForm.tsx 98.14% <100.00%> (-0.04%) ⬇️
...c/concepts/pipelines/content/createRun/RunPage.tsx 97.14% <100.00%> (+0.08%) ⬆️
.../src/concepts/pipelines/content/createRun/const.ts 100.00% <ø> (ø)
...oncepts/pipelines/content/createRun/submitUtils.ts 90.36% <100.00%> (+4.27%) ⬆️
.../src/concepts/pipelines/content/createRun/types.ts 100.00% <ø> (ø)
...epts/pipelines/content/createRun/useRunFormData.ts 88.75% <100.00%> (+1.07%) ⬆️
.../src/concepts/pipelines/content/createRun/utils.ts 100.00% <100.00%> (ø)
...c/concepts/pipelines/content/experiment/columns.ts 66.66% <ø> (+33.33%) ⬆️
...ipelineSelector/CustomPipelineRunToolbarSelect.tsx 72.22% <ø> (+58.33%) ⬆️
... and 17 more

... and 20 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b29bd6...c2190d4. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rhods-ci-bot
Copy link
Copy Markdown

@nananosirova: The following test has Failed:

OCI Artifact Browser URL

View in Artifact Browser

Inspecting Test Artifacts Manually

To inspect your test artifacts manually, follow these steps:

  1. Install ORAS (see the ORAS installation guide).
  2. Download artifacts with the following commands:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:odh-pr-test-trainer-s6s28

@caponetto
Copy link
Copy Markdown
Contributor

Thanks for checking them @nananosirova

  1. NP in this case, I had the impression we also had the selector there.
  2. The popup reference is enough for me 😄 but let's revisit that with UX for the next version (cc @acrago)

/lgtm

@openshift-ci openshift-ci Bot added lgtm and removed lgtm labels Apr 10, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/cypress/cypress/pages/pipelines/pipelineFilterBar.ts`:
- Around line 18-21: selectRunGroupByName currently finds any <td> containing
the given name which can match substrings; change the selection to require an
exact match inside the run-group table by scoping the contains to the <td> cell
and using an exact-text match (for example by matching the whole cell text with
a ^name$ regex or an exact-contains option) on the element with test id
'run-group-selector-table-list' before clicking; update the selectRunGroupByName
method to use that exact-match strategy to avoid accidentally clicking
overlapping names.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 98dcb52b-ae47-4a14-bbd7-9e690ad5618d

📥 Commits

Reviewing files that changed from the base of the PR and between ac9cc4e and 694e8e5.

📒 Files selected for processing (9)
  • frontend/src/concepts/pipelines/content/pipelineSelector/CustomPipelineRunToolbarSelect.tsx
  • frontend/src/concepts/pipelines/content/tables/pipelineRecurringRun/PipelineRecurringRunTableRow.tsx
  • frontend/src/concepts/pipelines/content/tables/pipelineRecurringRun/PipelineRecurringRunTableToolbar.tsx
  • frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow.tsx
  • frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRowExperiment.tsx
  • frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableToolbarBase.tsx
  • frontend/src/concepts/pipelines/content/tables/usePipelineFilter.ts
  • packages/cypress/cypress/pages/pipelines/pipelineFilterBar.ts
  • packages/cypress/cypress/tests/mocked/pipelines/runs/pipelineRuns.cy.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • frontend/src/concepts/pipelines/content/tables/usePipelineFilter.ts
  • frontend/src/concepts/pipelines/content/tables/pipelineRecurringRun/PipelineRecurringRunTableRow.tsx
  • frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRowExperiment.tsx
  • frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow.tsx

Comment on lines +18 to +21
selectRunGroupByName(name: string) {
this.findRunGroupSelect().click();
cy.findByTestId('run-group-selector-table-list').find('td').contains(name).click();
return this;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Use exact text matching for run-group row selection.

Line 20 uses partial contains(name) across all <td> cells, which can click the wrong row when run-group names overlap.

Proposed fix
   selectRunGroupByName(name: string) {
     this.findRunGroupSelect().click();
-    cy.findByTestId('run-group-selector-table-list').find('td').contains(name).click();
+    const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+    cy.findByTestId('run-group-selector-table-list')
+      .contains('td', new RegExp(`^${escaped}$`))
+      .click();
     return this;
   }
As per coding guidelines, `packages/cypress/**/*.{ts,js}` tests should be resilient; selectors must avoid brittle matching patterns.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
selectRunGroupByName(name: string) {
this.findRunGroupSelect().click();
cy.findByTestId('run-group-selector-table-list').find('td').contains(name).click();
return this;
selectRunGroupByName(name: string) {
this.findRunGroupSelect().click();
const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
cy.findByTestId('run-group-selector-table-list')
.contains('td', new RegExp(`^${escaped}$`))
.click();
return this;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cypress/cypress/pages/pipelines/pipelineFilterBar.ts` around lines
18 - 21, selectRunGroupByName currently finds any <td> containing the given name
which can match substrings; change the selection to require an exact match
inside the run-group table by scoping the contains to the <td> cell and using an
exact-text match (for example by matching the whole cell text with a ^name$
regex or an exact-contains option) on the element with test id
'run-group-selector-table-list' before clicking; update the selectRunGroupByName
method to use that exact-match strategy to avoid accidentally clicking
overlapping names.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@frontend/src/concepts/pipelines/content/tables/usePipelineFilter.ts`:
- Around line 72-75: The current code returns undefined when no exact
experiment.display_name === runGroupName match is found, which silently disables
backend filtering while filterData[run_group] may still be set; change the logic
in usePipelineFilter.ts so that when const match = experiments.find((e) =>
e.display_name === runGroupName) is falsy you do not return undefined but
instead create a fallback predicate that uses the provided runGroupName (e.g.,
match by includes/startsWith on e.display_name or compare against
e.run_group/id) and return that predicate so the backend receives a filter
consistent with the UI; update the branch around match to build and return this
fallback predicate using the same symbol names (experiments, match,
runGroupName, filterData[run_group]) so URL params produce an actual filter.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: d6667be3-f96e-4a8f-899f-2f0210a8c1d3

📥 Commits

Reviewing files that changed from the base of the PR and between 694e8e5 and 13cebd0.

📒 Files selected for processing (2)
  • frontend/src/concepts/pipelines/content/tables/__tests__/usePipelineFilter.spec.tsx
  • frontend/src/concepts/pipelines/content/tables/usePipelineFilter.ts

Comment thread frontend/src/concepts/pipelines/content/tables/usePipelineFilter.ts Outdated
@nananosirova nananosirova force-pushed the RHOAIENG-57471-tentative branch from 13cebd0 to ef2c08b Compare April 10, 2026 20:01
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@frontend/src/concepts/pipelines/content/experiment/ExperimentSelector.tsx`:
- Around line 130-131: The duplicate-name check is currently using the
paginated/filtered slice via existingNames={experiments.map((e) =>
e.display_name)} in CreateExperimentModal and will miss names outside the
current selector state; fix by passing the unfiltered full-name list (e.g.,
add/derive fullExperimentNames from the parent store or prop that contains all
experiments and use existingNames={fullExperimentNames.map(e=>e.display_name)})
or move the validation to a server-backed exact-name check during the create
flow (ensure CreateExperimentModal calls the createExperiment API and surfaces a
precise "name already exists" error). Locate CreateExperimentModal, the
existingNames prop usage, and the experiments.map(...) call to update the source
to the full name list or add server-side duplicate-name validation in the create
API handler.

In `@frontend/src/concepts/pipelines/content/tables/usePipelineFilter.ts`:
- Line 26: The legacy experiment/run_group conflict must be normalized before
building filterData: in the usePipelineFilter logic (the code exporting
LEGACY_EXPERIMENT_FILTER_PARAM and the routines that read/write URL params and
compute filterData), ensure that when an experiment id
(LEGACY_EXPERIMENT_FILTER_PARAM) is being set or present you clear or remove the
run_group param so the new click flow wins; conversely, if run_group is being
intentionally preserved, map/translate run_group into the experiment key
consistently. Update the URL-param write/update path that currently prefers
run_group (the branch referenced around the run_group handling and filterData
construction) to either delete run_group when setting
LEGACY_EXPERIMENT_FILTER_PARAM or normalize the pair into a single canonical key
before building filterData.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 907f2d4c-61b7-41aa-9c95-66d7bf67a766

📥 Commits

Reviewing files that changed from the base of the PR and between 13cebd0 and ef2c08b.

📒 Files selected for processing (5)
  • frontend/src/concepts/pipelines/content/compareRuns/CompareRunsRunList.tsx
  • frontend/src/concepts/pipelines/content/experiment/CreateExperimentModal.tsx
  • frontend/src/concepts/pipelines/content/experiment/ExperimentSelector.tsx
  • frontend/src/concepts/pipelines/content/tables/__tests__/usePipelineFilter.spec.tsx
  • frontend/src/concepts/pipelines/content/tables/usePipelineFilter.ts
✅ Files skipped from review due to trivial changes (1)
  • frontend/src/concepts/pipelines/content/tables/tests/usePipelineFilter.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/concepts/pipelines/content/experiment/CreateExperimentModal.tsx

Comment thread frontend/src/concepts/pipelines/content/tables/usePipelineFilter.ts
@caponetto
Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm label Apr 10, 2026
@nananosirova nananosirova force-pushed the RHOAIENG-57471-tentative branch from ef2c08b to 6fb99ee Compare April 10, 2026 20:13
@openshift-ci openshift-ci Bot removed the lgtm label Apr 10, 2026
@caponetto
Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added lgtm and removed lgtm labels Apr 10, 2026
@nananosirova nananosirova force-pushed the RHOAIENG-57471-tentative branch from 9a7d290 to 2ee8e88 Compare April 14, 2026 21:04
@DaoDaoNoCode
Copy link
Copy Markdown
Member

Review — HEAD 2ee8e884c

Great feature overall. The latest commit (Restore experiment object model, fix status filters) is a meaningful improvement — storing { value: experiment_id, label: display_name } for RUN_GROUP (matching the PIPELINE_VERSION pattern), deleting LEGACY_EXPERIMENT_FILTER_PARAM before setting RUN_GROUP in navigation, and the status enum fix are all good changes. A few issues remain that need attention before merging.


🔴 Must fix

1. FormGroup lost fieldId — accessibility regression (RunForm.tsx)

// Before
<FormGroup label="Run group" fieldId="run-group" isRequired ...>

// After
<FormGroup label="Run group" aria-label="Run group" isRequired ...>

fieldId causes PatternFly to render <label for="...">, which associates the label with the toggle button inside ActiveExperimentSelector. Without it, the "Run group" label is not programmatically linked to the selector. Screen readers won't announce the field label when the toggle receives focus. Restore fieldId pointing at the toggle button's ID.


2. dataTestId removed from ActiveExperimentSelector in RunForm — breaks Cypress

The previous iteration had dataTestId="run-group-selector" on ActiveExperimentSelector. The latest commit removed it, so the selector falls back to its default experiment-selector. Meanwhile createRunPage.ts still creates new SearchSelector('run-group-selector') and fillRunGroup calls this.runGroupSelect.openAndSelectItem(value). The Cypress run-group-selector test ID no longer exists in the DOM — these tests will fail.

Fix: restore dataTestId="run-group-selector" on the ActiveExperimentSelector call in RunForm.tsx.


🟡 Should fix

3. useMemo returning functions — wrong primitive (PipelineRunTableRow, PipelineRecurringRunTableRow)

const handleRunGroupClick = React.useMemo(() => {
  if (!experiment) return undefined;
  if (onRunGroupClick) return () => onRunGroupClick(experiment);
  return () => { /* navigate */ };
}, [...]);

useMemo is for memoizing values; useCallback is for memoizing callbacks. The project's coding guidelines say to avoid unnecessary useMemo. The right pattern here is a plain const for the derived URL and useCallback for the handler:

const runGroupLink = experiment
  ? buildLegacyExperimentFilterUrl(
      runType === PipelineRunType.ARCHIVED
        ? globalArchivedPipelineRunsRoute(namespace)
        : globalPipelineRunsRoute(namespace),
      experiment.experiment_id,
    )
  : undefined;

const handleRunGroupClick = React.useCallback(() => {
  if (!experiment) return;
  if (onRunGroupClick) { onRunGroupClick(experiment); return; }
  if (runGroupLink) navigate(runGroupLink);
}, [experiment, onRunGroupClick, runGroupLink, navigate]);

Same fix applies to PipelineRecurringRunTableRow.


4. handleRunGroupClick in ManageRunsTable and CompareRunsRunList depend on the whole filterProps object

const handleRunGroupClick = React.useCallback(
  (clickedExperiment: ExperimentKF) => {
    filterProps.onFilterUpdate(FilterOptions.RUN_GROUP, { ... });
  },
  [filterProps], // ← new object every render
);

filterProps is a new object reference on every render, so handleRunGroupClick re-creates every render, which also forces rowRenderer in ManageRunsTable to re-create. Destructure the stable callback:

const { onFilterUpdate } = filterProps;
const handleRunGroupClick = React.useCallback(
  (clickedExperiment: ExperimentKF) => {
    onFilterUpdate(FilterOptions.RUN_GROUP, {
      value: clickedExperiment.experiment_id,
      label: clickedExperiment.display_name,
    });
  },
  [onFilterUpdate],
);

5. Cypress selectRunGroupByName — partial substring match

// pipelineFilterBar.ts
cy.findByTestId('run-group-selector-table-list').find('td').contains(name).click();

contains(name) matches any row whose text includes name. If run groups "Foo" and "Foo Bar" both exist, selecting "Foo" can click the wrong row. Use exact matching:

const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
cy.findByTestId('run-group-selector-table-list')
  .contains('td', new RegExp(`^${escaped}$`))
  .click();

🟢 Nitpicks

  • Stack/StackItem with a single child in RunForm.tsx — adds DOM nodes with no layout benefit. ActiveExperimentSelector can be a direct child of FormGroup.
  • isClickable prop in PipelineRunTableRowExperiment is redundant — showClickableStyles = isClickable || !!onClick evaluates identically to !!onClick since the parent always sets both or neither. Derive from !!onClick only and remove the isClickable prop.
  • "Loading..." label in usePipelineFilter.ts — when arriving via a filtered URL before experiments load, the toolbar chip shows "Loading..." as a user-visible string. Consider returning undefined until the label resolves.
  • experimentRunsPageDescription export name in const.ts still uses the old identifier while its value now says "run group". Renaming to runGroupRunsPageDescription would keep the symbol consistent with the value.

@nananosirova nananosirova force-pushed the RHOAIENG-57471-tentative branch from 2ee8e88 to bcebdb4 Compare April 15, 2026 12:19
@DaoDaoNoCode
Copy link
Copy Markdown
Member

Follow-up review — HEAD d21599a96

All the issues from the previous review have been addressed — useCallback fixes, onFilterUpdate destructuring, isClickable removal, Stack cleanup, exact-match Cypress selector, dataTestId restored, experimentRunsPageDescriptionrunGroupRunsPageDescription, and the "Loading..." label. Great cleanup.

One small issue remains with the fieldId fix:


fieldId still doesn't associate the label (RunForm.tsx)

<FormGroup
  label="Run group"
  fieldId="run-group-selector-toggle"
  ...
>
  <ActiveExperimentSelector dataTestId="run-group-selector" ... />

fieldId generates <label for="run-group-selector-toggle">. But looking at SearchSelector.tsx, the MenuToggle is rendered as:

<MenuToggle
  id={dataTestId}              // → id="run-group-selector"
  data-testid={`${dataTestId}-toggle`}  // → data-testid="run-group-selector-toggle"
  ...
>

The HTML id on the toggle is "run-group-selector" (the bare dataTestId), not "run-group-selector-toggle". So the label's for attribute still doesn't match anything in the DOM and the programmatic association is still broken.

Fix: change fieldId to match the toggle's actual id:

<FormGroup
  label="Run group"
  fieldId="run-group-selector"
  ...
>

Everything else looks good to me. 🚀

Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
@nananosirova nananosirova force-pushed the RHOAIENG-57471-tentative branch from 44b7da7 to c2190d4 Compare April 15, 2026 22:50
@DaoDaoNoCode
Copy link
Copy Markdown
Member

Final review — HEAD c2190d458

Everything from the previous rounds is addressed. The fieldId="run-group-selector" now correctly matches id={dataTestId} on the MenuToggle inside SearchSelector, so the label association is properly wired. All other issues are resolved too.

The latest commit (Fix pipeline version filter in schedule tab and tests) adds two additional fixes worth calling out:

usePipelineRecurringRuns.ts — client-side version filtering: The recurring runs API doesn't support pipeline_version_id as a query param, so version filtering is now done client-side after fetching. One thing to be aware of: when filtering by version, total_size is set to filteredRuns.length for the current page. If there are more matching runs on subsequent pages, the displayed total will undercount. This is an inherent trade-off of client-side filtering against a paged API — not a blocker, just worth a comment for future reference.

useRunFormData.ts — simplified useUpdateRunGroupFormData: The previous code cleared the experiment if its storage_state was ARCHIVED. That logic is removed. Since ActiveExperimentSelector only shows active experiments, users can't select an archived one through the UI. For duplicate runs, the original run's experiment (which may be archived) is pre-populated — the new code preserves it rather than clearing it, letting the user decide whether to change it. That's a reasonable UX choice, just making sure it's intentional.

Everything looks good. LGTM 🚀

Copy link
Copy Markdown
Member

@DaoDaoNoCode DaoDaoNoCode left a comment

Choose a reason for hiding this comment

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

/lgtm
Great work here!

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 15, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DaoDaoNoCode

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

The pull request process is described 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

@openshift-merge-bot openshift-merge-bot Bot merged commit 7e25239 into opendatahub-io:main Apr 15, 2026
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants