WIP: Rework feature gate test filtering to cover component owned jobs#3731
WIP: Rework feature gate test filtering to cover component owned jobs#3731dgoodwin wants to merge 1 commit into
Conversation
- Centralizes the logic for how to find test results for a featuregate for re-use in the openshift/api verification job. - Feature Gate API now returns hateoas links clients can follow. - Includes a link to the tests tagged for the feature gate name. - Includes a link to the synthetic "openshift-tests should work" as an indicator the job fully passed, for any jobs tied to a Capability. - Replaces the hack for featuregates containing Install to automatically route to all install results and exclude actual tests for the FG. - Adds feature gate seed data and e2e tests.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
dgoodwin
changed the title
openshift-ci
Bot
added
approved
WalkthroughAdds a ChangesFeature Gate Links Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant FeatureGates as FeatureGates List
participant FeatureGateDetail
participant Server as sippyserver
Browser->>FeatureGates: Load feature gates list
FeatureGates->>Server: GET /api/feature_gates
Server-->>FeatureGates: gates with Links
Browser->>FeatureGateDetail: Navigate via links.ui_detail
FeatureGateDetail->>Server: GET feature gate by name/release
Server-->>FeatureGateDetail: gate + links
FeatureGateDetail->>Server: GET tests_by_annotation / tests_by_capability
Server-->>FeatureGateDetail: test results
FeatureGateDetail-->>Browser: Render tabs and DataGrid
🚥 Pre-merge checks | ✅ 18 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (18 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
sippy-ng/src/tests/FeatureGateDetail.js (2)
136-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing existing filter helpers instead of hand-building the payload.
This manually builds a filter object with
JSON.stringify+safeEncodeURIComponent. The repo providesfilterFor/single/multiple/multiple_orfor building filter payloads andpathForAPIWithFilterfor appending the encodedfilters=query param — reusing these would keep encoding consistent with the rest of the codebase and reduce duplicated logic.As per path instructions, "Build filter payloads with
filterFor(column, operator, value)plussingle()/multiple()/multiple_or()" and "usepathForAPIWithFilter(apiPath, filter)to append the encodedfilters=...query param".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sippy-ng/src/tests/FeatureGateDetail.js` around lines 136 - 154, The FeatureGateDetail test is manually constructing and encoding a filter payload instead of using the shared filter helpers. Update the fetch URL-building logic to create the filter with filterFor plus single/multiple/multiple_or as appropriate, and append it with pathForAPIWithFilter rather than JSON.stringify and safeEncodeURIComponent. Use the existing FeatureGateDetail fetch call and the API path construction there as the place to swap in the shared helpers so encoding stays consistent.Source: Path instructions
148-154: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
releaseisn't URL-encoded in the fetch URL.
releaseis concatenated directly into the query string withoutencodeURIComponent/safeEncodeURIComponent. Low risk given release names are typically simple version strings, but worth guarding for consistency with the rest of the file (which does encode the filter).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sippy-ng/src/tests/FeatureGateDetail.js` around lines 148 - 154, The fetch URL construction in FeatureGateDetail’s request builder concatenates release directly into the query string, unlike filterParam which is encoded. Update the URL assembly in the fetch call to pass release through encodeURIComponent or safeEncodeURIComponent before appending it, so the query string is consistently encoded and safe.pkg/sippyserver/server.go (1)
693-698: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUndocumented trailing
]anchor; alsoreleaseis interpolated unescaped into generated URLs.The
"FeatureGate:%s]"trick is non-obvious and has no comment explaining why the bracket is there — worth a one-line comment for future maintainers. Separately,releaseis embedded via plain%sintests_by_annotation,tests_by_capability, andui_detailwithouturl.QueryEscape/url.PathEscape(unlikefg.FeatureGate), which is inconsistent even though current risk is low since an invalidreleasevalue would simply fail to match any DB rows upstream.As per coding guidelines, "Keep comments minimal and helpful, and make them explain the 'why' rather than the 'what'."
♻️ Suggested tweak
+ // Trailing "]" anchors the match to the annotation's closing bracket so a shorter + // gate name can't match as a prefix of a longer, similarly-named gate. annotationFilter := url.QueryEscape(fmt.Sprintf( `{"items":[{"columnField":"name","operatorValue":"contains","value":"FeatureGate:%s]"}]}`, fg.FeatureGate)) fg.Links["tests_by_annotation"] = fmt.Sprintf( "%s/api/tests?release=%s&filter=%s", - baseAPIURL, release, annotationFilter) + baseAPIURL, url.QueryEscape(release), annotationFilter)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/sippyserver/server.go` around lines 693 - 698, Add a brief explanatory comment near the FeatureGate annotation filter in server.go to document why the trailing ] is intentionally included in the value, and make the generated URLs consistent by escaping release before interpolating it into tests_by_annotation, tests_by_capability, and ui_detail in the same code path that already uses url.QueryEscape for fg.FeatureGate.Source: Coding guidelines
test/e2e/util/e2erequest.go (1)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
nolintjustification for the new exported signature.The
#nosec/nolintcomment on theclient.Get(url)call says the URL is "constructed from test helper's hardcoded localhost base URL," butSippyGetAbsoluteis now also invoked directly with server-returned HATEOAS links (seefeature_gates_test.go), not justBuildE2EURLoutput. Low risk given this is e2e test code targeting the same server, but worth updating the comment so the suppression rationale stays accurate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/util/e2erequest.go` around lines 42 - 48, The suppression comment on SippyGetAbsolute’s client.Get(url) call is outdated because this exported helper can now receive direct HATEOAS links, not only BuildE2EURL output. Update the nolint/gosec justification in SippyGetAbsolute to describe the broader e2e test usage accurately, while keeping the same suppression on the http.Client request line.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/sippyserver/server.go`:
- Around line 690-710: The HATEOAS link builder in injectFeatureGateHATEOASLinks
is too narrow and can return incorrect test links. Update tests_by_annotation to
match both feature-gate tag forms, including OCPFeatureGate alongside
FeatureGate, and adjust tests_by_capability to use exact array membership
semantics (“has entry”) rather than substring-based contains so unrelated tests
are not matched by prefix collisions.
In `@sippy-ng/src/App.js`:
- Around line 262-275: The redirect logic in FeatureGateDetailWrapper is
updating the wrong path segment for the “latest” release case. When useParams()
returns release === 'latest', change the navigation in
RedirectLatestReleaseWrapper usage so it targets the full
/feature_gates/${defaultRelease}/${feature_gate} URL instead of a relative ../
redirect. Keep the existing release and feature_gate values from
FeatureGateDetailWrapper/FeatureGateDetail, but ensure the redirect writes the
release segment so the loop stops.
In `@sippy-ng/src/tests/FeatureGateDetail.js`:
- Around line 101-112: The sorting state in DataGrid is controlled but never
updated, so header clicks cannot change the order. Update the FeatureGateDetail
DataGrid setup to manage sort state with a component state value, and wire
`onSortModelChange` to persist user changes instead of always passing the fixed
`sortModel` literal. Keep the existing default sort for initial render, but make
sure `DataGrid` in `FeatureGateDetail` reflects the current sort model on
subsequent renders.
- Around line 204-213: SimpleBreadcrumbs is being passed a crumbs prop that it
ignores, so the Feature Gates back link never renders. Update the
FeatureGateDetail test/setup to use the supported previousPage prop on
SimpleBreadcrumbs instead of crumbs, keeping release and currentPage as-is so
the breadcrumb can show the Feature Gates link correctly.
---
Nitpick comments:
In `@pkg/sippyserver/server.go`:
- Around line 693-698: Add a brief explanatory comment near the FeatureGate
annotation filter in server.go to document why the trailing ] is intentionally
included in the value, and make the generated URLs consistent by escaping
release before interpolating it into tests_by_annotation, tests_by_capability,
and ui_detail in the same code path that already uses url.QueryEscape for
fg.FeatureGate.
In `@sippy-ng/src/tests/FeatureGateDetail.js`:
- Around line 136-154: The FeatureGateDetail test is manually constructing and
encoding a filter payload instead of using the shared filter helpers. Update the
fetch URL-building logic to create the filter with filterFor plus
single/multiple/multiple_or as appropriate, and append it with
pathForAPIWithFilter rather than JSON.stringify and safeEncodeURIComponent. Use
the existing FeatureGateDetail fetch call and the API path construction there as
the place to swap in the shared helpers so encoding stays consistent.
- Around line 148-154: The fetch URL construction in FeatureGateDetail’s request
builder concatenates release directly into the query string, unlike filterParam
which is encoded. Update the URL assembly in the fetch call to pass release
through encodeURIComponent or safeEncodeURIComponent before appending it, so the
query string is consistently encoded and safe.
In `@test/e2e/util/e2erequest.go`:
- Around line 42-48: The suppression comment on SippyGetAbsolute’s
client.Get(url) call is outdated because this exported helper can now receive
direct HATEOAS links, not only BuildE2EURL output. Update the nolint/gosec
justification in SippyGetAbsolute to describe the broader e2e test usage
accurately, while keeping the same suppression on the http.Client request line.
🪄 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)
Review profile: CHILL
Plan: Enterprise
Run ID: d8d39b25-4310-40ad-8d3d-fa26362b5e58
📒 Files selected for processing (8)
cmd/sippy/seed_data.gopkg/apis/api/types.gopkg/sippyserver/server.gosippy-ng/src/App.jssippy-ng/src/tests/FeatureGateDetail.jssippy-ng/src/tests/FeatureGates.jstest/e2e/feature_gates_test.gotest/e2e/util/e2erequest.go
| func injectFeatureGateHATEOASLinks(fg *apitype.FeatureGate, release, baseAPIURL, baseFrontendURL string) { | ||
| fg.Links = make(map[string]string, 3) | ||
|
|
||
| annotationFilter := url.QueryEscape(fmt.Sprintf( | ||
| `{"items":[{"columnField":"name","operatorValue":"contains","value":"FeatureGate:%s]"}]}`, | ||
| fg.FeatureGate)) | ||
| fg.Links["tests_by_annotation"] = fmt.Sprintf( | ||
| "%s/api/tests?release=%s&filter=%s", | ||
| baseAPIURL, release, annotationFilter) | ||
|
|
||
| capabilityFilter := url.QueryEscape(fmt.Sprintf( | ||
| `{"items":[{"columnField":"name","operatorValue":"contains","value":"openshift-tests should work"},{"columnField":"variants","operatorValue":"contains","value":"Capability:%s"}],"linkOperator":"and"}`, | ||
| fg.FeatureGate)) | ||
| fg.Links["tests_by_capability"] = fmt.Sprintf( | ||
| "%s/api/tests?release=%s&filter=%s", | ||
| baseAPIURL, release, capabilityFilter) | ||
|
|
||
| fg.Links["ui_detail"] = fmt.Sprintf( | ||
| "%s/feature_gates/%s/%s", | ||
| baseFrontendURL, release, url.PathEscape(fg.FeatureGate)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C5 '"variants"' --type=go pkg/filter pkg/db/query 2>/dev/null
rg -n -C5 'columnField.*variants|case "variants"' --type=goRepository: openshift/sippy
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and search for feature-gate link construction and filter handling.
ast-grep outline pkg/sippyserver/server.go --view expanded || true
rg -n -C4 'injectFeatureGateHATEOASLinks|tests_by_capability|tests_by_annotation|FeatureGate:' pkg/sippyserver/server.go
rg -n -C4 'columnField.*variants|variants.*contains|case "variants"|operatorValue.*contains' pkg --type=goRepository: openshift/sippy
Length of output: 18733
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any filter/query code that may define "contains" semantics.
rg -n -C4 'contains' pkg --type=go
rg -n -C4 '"variants"' pkg --type=go
rg -n -C4 'FeatureGate|feature gate|feature_gate' pkg --type=goRepository: openshift/sippy
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the filter implementation to determine how "contains" behaves for array fields.
ast-grep outline pkg/filter --view expanded || true
rg -n -C4 'func .*contains|Operator: "contains"|strings.Contains|Contains' pkg/filter --type=go
sed -n '1,260p' pkg/filter/filterable.goRepository: openshift/sippy
Length of output: 20233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how feature-gate annotations are represented and whether the trailing bracket is intentional.
rg -n -C3 'FeatureGate:' pkg --type=go
rg -n -C3 'FeatureGate.*\]' pkg --type=go
rg -n -C3 'annotations|annotation' pkg/apis pkg/filter pkg/sippyserver --type=go | head -n 200Repository: openshift/sippy
Length of output: 12971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the feature-gate response type to see how links and gate names are represented.
sed -n '1040,1105p' pkg/apis/api/types.goRepository: openshift/sippy
Length of output: 1594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for existing feature-gate names that might already share prefixes.
rg -n 'FeatureGate:' pkg --type=goRepository: openshift/sippy
Length of output: 562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the in-memory array filter semantics for the exact operator we should use here.
sed -n '640,660p' pkg/filter/filterable.goRepository: openshift/sippy
Length of output: 587
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the FeatureGate API type to see how its name/value fields are used.
sed -n '1070,1100p' pkg/apis/api/types.goRepository: openshift/sippy
Length of output: 713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for OCPFeatureGate usage to understand whether it should be included in the link filter.
rg -n 'OCPFeatureGate' .Repository: openshift/sippy
Length of output: 983
Include both feature-gate tag forms
tests_by_annotationonly matchesFeatureGate:%s], but the data also carriesOCPFeatureGate:%s]tags, so this link can miss valid tests.tests_by_capabilityshould use exact array membership (has entry) instead ofcontains; array filtering here is substring-based, so prefix collisions can pull in unrelated tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/sippyserver/server.go` around lines 690 - 710, The HATEOAS link builder
in injectFeatureGateHATEOASLinks is too narrow and can return incorrect test
links. Update tests_by_annotation to match both feature-gate tag forms,
including OCPFeatureGate alongside FeatureGate, and adjust tests_by_capability
to use exact array membership semantics (“has entry”) rather than
substring-based contains so unrelated tests are not matched by prefix
collisions.
| const FeatureGateDetailWrapper = () => { | ||
| let { release, feature_gate } = useParams() | ||
| if (release === 'latest') { | ||
| release = findFirstNonGARelease(React.useContext(ReleasesContext)) | ||
| } | ||
|
|
||
| return ( | ||
| <Navigate | ||
| to={pathForTestSubstringByVariant(release, 'FeatureGate:' + feature_gate)} | ||
| replace | ||
| return RedirectLatestReleaseWrapper( | ||
| <FeatureGateDetail | ||
| key={'fg-detail-' + release + '-' + feature_gate} | ||
| release={release} | ||
| featureGate={feature_gate} | ||
| /> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect other usages of RedirectLatestReleaseWrapper on multi-segment routes
# to confirm whether this relative-navigation pattern is already known/handled elsewhere.
rg -n -B3 -A15 'RedirectLatestReleaseWrapper' sippy-ng/src/App.jsRepository: openshift/sippy
Length of output: 5644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the feature gate routes and the wrapper implementation in context.
sed -n '150,310p' sippy-ng/src/App.js
printf '\n--- package version ---\n'
jq -r '.dependencies["react-router-dom"] // .devDependencies["react-router-dom"] // empty' sippy-ng/package.jsonRepository: openshift/sippy
Length of output: 4466
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the feature gate route definitions and any navigation helpers tied to them.
rg -n -A4 -B4 'FeatureGateDetailWrapper|FeatureGatesWrapper|feature_gate|feature-gates|feature_gates' sippy-ng/src/App.jsRepository: openshift/sippy
Length of output: 1476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the actual feature-gate route path in the router config.
rg -n -A3 -B3 'FeatureGateDetail|featureGate|feature-gates|feature_gates' sippy-ng/src/App.jsRepository: openshift/sippy
Length of output: 1486
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the router table around the feature gate routes.
rg -n -A8 -B8 'FeatureGateDetailWrapper|FeatureGatesWrapper|Route path=.*feature|<Route' sippy-ng/src/App.jsRepository: openshift/sippy
Length of output: 7265
🌐 Web query:
React Router v6 useNavigate relative: "path" "../" path segment resolution docs
💡 Result:
In React Router v6, the useNavigate hook (and Link component) provides the relative option to control how relative paths, such as "../", are resolved [1][2]. By default, React Router v6 uses relative: "route" [1]. In this mode, ".." navigates up one level in the route hierarchy, effectively traversing the nested route tree rather than the URL string segments [2][3]. When you specify relative: "path", React Router switches to path-based resolution [1]. In this mode, ".." behaves similarly to a command-line "cd" operation, removing exactly one segment from the current URL path [1][2]. Key points regarding this behavior: - Scope: The relative: "path" option only impacts how the relative path string is resolved [1]. It does not change the "starting" location, which is always the route where the hook is called (or where the Link is rendered) [1][2]. - Difference from standard URL resolution: Because React Router's default behavior is based on the route hierarchy, it can sometimes produce results that differ from standard browser link behavior if your route structure does not align perfectly with your URL segments [1][3]. - Usage Example: navigate("..", { relative: "path" }); If you require resolution against the literal current URL path regardless of the route hierarchy, the recommended approach is to use the current location and the browser's URL constructor [1][2]: let location = useLocation; new URL("..", window.origin + location.pathname);
Citations:
- 1: https://reactrouter.com/docs/en/en/v6/hooks/use-navigate
- 2: https://reactrouter.com/6.30.3/components/link
- 3: https://reactrouter.com/v6/start/overview
🌐 Web query:
React Router v6.30.3 useNavigate relative path dot segments resolvePath "../" feature_gate
💡 Result:
In React Router v6.30.3, the useNavigate hook supports relative path segments (such as "../") for navigation [1][2]. There is no "feature_gate" required or present to enable this base functionality, as it is a core feature of the library [1]. Key behavioral details for relative paths in v6.30.3: Default Behavior (Route Hierarchy): By default, navigation is relative to the route hierarchy [1]. Using ".." in the navigate function will move the user up one level in the defined route structure, not necessarily one segment in the URL path [1][2]. Path-Relative Routing: If you want ".." to resolve relative to the actual URL path segments (e.g., to navigate to a sibling route), you can opt into this behavior by passing the relative: "path" option to the navigate function [1][3]: navigate("..", { relative: "path" }); Important Considerations: 1. The relative: "path" option only impacts how the relative path is resolved; it does not change the starting location, which remains tied to the route hierarchy where the hook is called [1][4]. 2. Splat Paths: While there is no feature gate for general "../" functionality, React Router does include a future flag named future.v7_relativeSplatPath, which affects how relative paths (including "..") resolve when inside splat routes [3][5]. This flag was introduced to fix legacy, buggy behavior regarding splat path resolution and is recommended for preparing applications for v7, but it is not a gate for the core ability to use "../" in navigation [5]. resolvePath: The resolvePath utility function follows similar resolution logic and is used internally by React Router components and hooks (like useResolvedPath) to resolve relative paths [6][7][8].
Citations:
- 1: https://reactrouter.com/6.30.3/hooks/use-navigate
- 2: https://raw.githubusercontent.com/remix-run/react-router/refs/tags/react-router@6.30.3/docs/start/overview.md
- 3: https://reactrouter.com/6.28.0/hooks/use-navigate
- 4: https://reactrouter.com/6.30.4/hooks/use-navigate
- 5: https://reactrouter.com/6.30.4/hooks/use-resolved-path
- 6: https://reactrouter.com/6.28.0/utils/resolve-path
- 7: https://reactrouter.com/6.30.1/utils/resolve-path
- 8: https://raw.githubusercontent.com/remix-run/react-router/refs/tags/react-router@6.30.3/docs/utils/resolve-path.md
latest is rewritten in the wrong URL segment. navigate('../' + defaultRelease, { relative: 'path' }) from /feature_gates/:release/:feature_gate changes :feature_gate, not :release, so the URL stays on latest and the redirect keeps firing. Navigate to /feature_gates/${defaultRelease}/${feature_gate} instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sippy-ng/src/App.js` around lines 262 - 275, The redirect logic in
FeatureGateDetailWrapper is updating the wrong path segment for the “latest”
release case. When useParams() returns release === 'latest', change the
navigation in RedirectLatestReleaseWrapper usage so it targets the full
/feature_gates/${defaultRelease}/${feature_gate} URL instead of a relative ../
redirect. Keep the existing release and feature_gate values from
FeatureGateDetailWrapper/FeatureGateDetail, but ensure the redirect writes the
release segment so the loop stops.
| <DataGrid | ||
| loading={!isLoaded} | ||
| rows={rows} | ||
| columns={columns} | ||
| getRowId={(row) => row.name} | ||
| getRowHeight={() => 'auto'} | ||
| autoHeight={true} | ||
| rowsPerPageOptions={[10, 25, 50]} | ||
| pageSize={25} | ||
| sortModel={[{ field: 'current_pass_percentage', sort: 'asc' }]} | ||
| disableSelectionOnClick | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sorting is non-functional: sortModel is set but never updated.
DataGrid is given a fixed sortModel without onSortModelChange. Since sortModel makes the grid a controlled component for sorting, clicking column headers won't actually reorder rows — the prop will be reset back to the fixed value each render.
🔧 Proposed fix
+ const [sortModel, setSortModel] = React.useState([
+ { field: 'current_pass_percentage', sort: 'asc' },
+ ])
...
<DataGrid
loading={!isLoaded}
rows={rows}
columns={columns}
getRowId={(row) => row.name}
getRowHeight={() => 'auto'}
autoHeight={true}
rowsPerPageOptions={[10, 25, 50]}
pageSize={25}
- sortModel={[{ field: 'current_pass_percentage', sort: 'asc' }]}
+ sortModel={sortModel}
+ onSortModelChange={setSortModel}
disableSelectionOnClick
/>📝 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.
| <DataGrid | |
| loading={!isLoaded} | |
| rows={rows} | |
| columns={columns} | |
| getRowId={(row) => row.name} | |
| getRowHeight={() => 'auto'} | |
| autoHeight={true} | |
| rowsPerPageOptions={[10, 25, 50]} | |
| pageSize={25} | |
| sortModel={[{ field: 'current_pass_percentage', sort: 'asc' }]} | |
| disableSelectionOnClick | |
| /> | |
| const [sortModel, setSortModel] = React.useState([ | |
| { field: 'current_pass_percentage', sort: 'asc' }, | |
| ]) | |
| <DataGrid | |
| loading={!isLoaded} | |
| rows={rows} | |
| columns={columns} | |
| getRowId={(row) => row.name} | |
| getRowHeight={() => 'auto'} | |
| autoHeight={true} | |
| rowsPerPageOptions={[10, 25, 50]} | |
| pageSize={25} | |
| sortModel={sortModel} | |
| onSortModelChange={setSortModel} | |
| disableSelectionOnClick | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sippy-ng/src/tests/FeatureGateDetail.js` around lines 101 - 112, The sorting
state in DataGrid is controlled but never updated, so header clicks cannot
change the order. Update the FeatureGateDetail DataGrid setup to manage sort
state with a component state value, and wire `onSortModelChange` to persist user
changes instead of always passing the fixed `sortModel` literal. Keep the
existing default sort for initial render, but make sure `DataGrid` in
`FeatureGateDetail` reflects the current sort model on subsequent renders.
| <SimpleBreadcrumbs | ||
| release={release} | ||
| currentPage={featureGate} | ||
| crumbs={[ | ||
| { | ||
| text: 'Feature Gates', | ||
| link: `/feature_gates/${release}`, | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the referenced test file and breadcrumb component
git ls-files | rg 'sippy-ng/src/tests/FeatureGateDetail\.js|SimpleBreadcrumbs|Breadcrumbs' -n
# Show the relevant component definition and the usage site with line numbers
for f in $(git ls-files | rg 'SimpleBreadcrumbs|Breadcrumbs' | head -n 20); do
echo "### $f"
wc -l "$f"
done
echo
echo "### Usage site"
sed -n '180,240p' sippy-ng/src/tests/FeatureGateDetail.js
echo
echo "### Search for SimpleBreadcrumbs definitions/usages"
rg -n "function SimpleBreadcrumbs|const SimpleBreadcrumbs|class SimpleBreadcrumbs|<SimpleBreadcrumbs|previousPage=|crumbs=" sippy-ng/src -g '!**/node_modules/**'Repository: openshift/sippy
Length of output: 4379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' sippy-ng/src/components/SimpleBreadcrumbs.js
echo
echo "### Nearby examples that use the supported prop shape"
sed -n '380,420p' sippy-ng/src/jobs/JobAnalysis.js
echo
sed -n '160,190p' sippy-ng/src/tests/TestAnalysis.jsRepository: openshift/sippy
Length of output: 2619
Use previousPage for the Feature Gates breadcrumb link SimpleBreadcrumbs only renders release, previousPage, and currentPage, so crumbs is ignored and the back link never appears.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sippy-ng/src/tests/FeatureGateDetail.js` around lines 204 - 213,
SimpleBreadcrumbs is being passed a crumbs prop that it ignores, so the Feature
Gates back link never renders. Update the FeatureGateDetail test/setup to use
the supported previousPage prop on SimpleBreadcrumbs instead of crumbs, keeping
release and currentPage as-is so the breadcrumb can show the Feature Gates link
correctly.
|
Scheduling required tests: |
|
@dgoodwin: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
1 participant
for re-use in the openshift/api verification job.
indicator the job fully passed, for any jobs tied to a Capability.
route to all install results and exclude actual tests for the FG.
Summary by CodeRabbit
New Features
Bug Fixes