OU-1175: OCP 4.22 Breaking Changes#863
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PeterYurkovich 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 |
openshift-ci
bot
added
the
approved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced legacy router/query utilities with React Router v7 and a new adapter, added an ESLint flat config, applied a dynamic-plugin-sdk patch, updated TypeScript typings and React 18 entry/usage, migrated many routing/query usages, and added build/docker/Makefile steps to apply frontend patches. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
/retest |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
PeterYurkovich
force-pushed
the
breaking-changes
branch
2 times, most recently
from
cea86b9 to
f027d44
Compare
PeterYurkovich
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@PeterYurkovich: This pull request references OU-1175 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@PeterYurkovich: This pull request references OU-1175 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
needs-rebase
PeterYurkovich
force-pushed
the
breaking-changes
branch
from
2d128c8 to
7635ba3
Compare
|
@PeterYurkovich: The following tests failed, say
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. |
PeterYurkovich
force-pushed
the
breaking-changes
branch
from
47d1f33 to
e2b0d61
Compare
openshift-ci
bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
web/src/components/MetricsPage.tsx (1)
1101-1119:⚠️ Potential issue | 🟠 MajorKeep the URL as a source of truth after the initial load.
After Line 1103, later
query{i}search-param changes are ignored, but Lines 1139-1144 still mirror Redux back into the URL. Browser back/forward to a different metrics query will be immediately reverted to the stale in-memory queries, which makes bookmarked/shared URLs unreliable.Also applies to: 1135-1145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/MetricsPage.tsx` around lines 1101 - 1119, The current useEffect blocks updates from URL after the first render (isFirstRender) so later changes to query{i} params are ignored and then Redux is mirrored back into the URL (lines that mirror Redux into URL), causing URL navigations to be reverted; modify sync logic so the URL remains the source of truth: replace the one-time isFirstRender-only apply (useEffect that calls queryBrowserDeleteAllQueries, queryBrowserPatchQuery, setFirstRenderFalse) with a sync that runs whenever queryParams changes — parse all query{i} params and dispatch patch/delete actions to reconcile Redux to the URL (use queryBrowserDeleteAllQueries and queryBrowserPatchQuery), and add a small origin flag or compare previous vs current params to avoid creating a feedback loop with the existing Redux->URL mirror logic (i.e., only update Redux when params differ). Ensure unique symbols referenced: useEffect (initialization block), isFirstRender, dispatch, queryBrowserDeleteAllQueries, queryBrowserPatchQuery, setFirstRenderFalse, and the Redux->URL mirror should skip updates when the change originated from URL parsing.web/src/components/dashboards/legacy/useLegacyDashboards.ts (1)
126-150:⚠️ Potential issue | 🟠 MajorPreserve the current query params when switching legacy dashboards.
Line 126 creates a fresh
URLSearchParams, so Line 133 dropsrefresh,endTime,timeRange, and templating params. Line 147 and Line 150 then read from that empty object and always fall back to defaults.Proposed fix
- const params = new URLSearchParams(); + const params = new URLSearchParams(queryParams); const url = getLegacyDashboardsUrl(perspective, newBoard, namespace); if (newBoard !== urlBoard || forceRefresh) { - if (!params.has(QueryParams.Dashboard) || params.get(QueryParams.Dashboard) !== newBoard) { + if (forceRefresh || params.get(QueryParams.Dashboard) !== newBoard) { params.set(QueryParams.Dashboard, newBoard); navigate(`${url}?${params.toString()}`, { replace: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/legacy/useLegacyDashboards.ts` around lines 126 - 150, The code creates an empty URLSearchParams (const params = new URLSearchParams()) which drops existing query params when switching dashboards; change it to initialize params from the existing query params (e.g., new URLSearchParams(queryParams.toString() or current location.search) so refreshInterval, endTime, timeRange and templating params are preserved. Ensure getLegacyDashboardsUrl and the navigate call still update only QueryParams.Dashboard while leaving other keys intact, and then continue to read refreshInterval, params.get(QueryParams.EndTime) and params.get(QueryParams.TimeRange) (used by dashboardsSetPollInterval, dashboardsSetEndTime, dashboardsSetTimespan) from the preserved params.web/src/components/dashboards/perses/hooks/useDashboardsData.ts (2)
95-118:⚠️ Potential issue | 🟠 MajorAll-namespaces mode makes dashboard selection ambiguous.
Line 96 now exposes dashboards from every project, but the selection is still keyed by
nameonly. Dashboard names are project-scoped, so duplicate names across projects will resolve to the first match and derive the wrongprojectToUse. Downstream, the existingnewBoard !== dashboardNameguard also means same-name dashboards in different projects will not navigate at all. The selector value needs to carry bothprojectandname.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/hooks/useDashboardsData.ts` around lines 95 - 118, The all-namespaces mode exposes dashboards from all projects but selection is still by name only (in useMemo activeProjectDashboardsMetadata and in changeBoard), causing ambiguous/incorrect matches; update the selector to carry both project and name (e.g., a composite string "project/name" or an object) so changeBoard can unambiguously resolve the target dashboard: when building options (activeProjectDashboardsMetadata) emit the composite identifier, update changeBoard to parse that composite into project and name, find the dashboard by matching both combinedDashboardsMetadata items' project and name (not just name), compute projectToUse from the parsed project if activeProject === ALL_NAMESPACES_KEY, and replace the newBoard !== dashboardName guard with a comparison against the composite identifier to ensure proper navigation handling.
43-92:⚠️ Potential issue | 🟠 MajorMove state updates out of
useMemo; calling setters violates render-phase purity.Lines 82–83 call
setPreviousDashboardsandsetPreviousMetadatainside theuseMemocallback. State setters schedule updates during the render phase, causing extra renders and violating the purity contract ofuseMemo. This can trigger render-loop detection in StrictMode.Use
useRefto track previous values, or move state updates to auseEffecthook that runs after the memo computation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/hooks/useDashboardsData.ts` around lines 43 - 92, The memoized computed value combinedDashboardsMetadata currently calls state setters setPreviousDashboards and setPreviousMetadata inside the useMemo callback (violating render-phase purity); refactor by keeping the pure computation in useMemo (compute newMetadata and dashboardsChanged using persesDashboards, prevDashboards, prevMetadata) and move the side-effectful updates into a useEffect that depends on persesDashboards and combinedInitialLoad (or use refs: replace prevDashboards/prevMetadata state with useRef to track previous values and update those refs inside useEffect), ensuring combinedDashboardsMetadata remains read-only inside useMemo and all calls to setPreviousDashboards/setPreviousMetadata occur in the effect or by mutating refs.
🧹 Nitpick comments (1)
Dockerfile.mcp (1)
9-9: Consider copying the fullweb/patches/directory for maintainability.Copying only one patch file means new patch-package files won’t be applied in this build until this Dockerfile is edited too.
Suggested change
-COPY web/patches/@openshift-console+dynamic-plugin-sdk+4.22.0-prerelease.2.patch web/patches/ +COPY web/patches/ web/patches/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.mcp` at line 9, The Dockerfile currently copies a single patch file named "@openshift-console+dynamic-plugin-sdk+4.22.0-prerelease.2.patch" (COPY web/patches/@openshift-console+dynamic-plugin-sdk+4.22.0-prerelease.2.patch web/patches/), which requires editing the Dockerfile each time a new patch is added; change this to copy the entire patches directory (e.g., COPY web/patches/ web/patches/) so any new files in web/patches are automatically included in the build and avoid future Dockerfile edits when adding patch-package files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/dashboards/legacy/custom-time-range-modal.tsx`:
- Line 52: The current call to setQueryParams replaces the entire query string
and drops unrelated params; update the call in custom-time-range-modal (the
function handling time range updates around where setQueryParams is used) to use
the updater form: pass a callback that receives existing URLSearchParams, clone
or modify it (new URLSearchParams(prev)), set/replace only the time-range keys
(e.g., "start","end" or whatever keys you use), and return the updated
URLSearchParams so other params (Dashboard, RefreshInterval, Units) are
preserved; apply the same pattern to legacy-dashboard.tsx handleZoom to merge
rather than overwrite query params.
In `@web/src/components/dashboards/legacy/useOpenshiftProject.ts`:
- Around line 36-40: The patch call can set the namespace variable to undefined
because routeNamespace is optional; update the condition around dispatching
dashboardsPatchVariable('namespace', { value: routeNamespace }) to ensure
routeNamespace is defined/non-empty before calling it (e.g., add an explicit
check for routeNamespace !== undefined/null/''), so you never dispatch a {
value: undefined } and thus never accidentally clear the dashboard variable;
reference the existing variableNamespace and routeNamespace checks and the
dashboardsPatchVariable('namespace', {...}) call when making this change.
In `@web/src/components/MetricsPage.tsx`:
- Around line 1139-1145: The URLSearchParams created from queryParams preserves
old queryN keys; before re-adding the current queries in the effect that uses
newParams, remove any stale keys matching the pattern /^query\d+$/ so deleted
queries don't remain in the URL. In the effect where newParams is created
(variables: newParams, queryStrings, setQueryParams, customDataSourceName),
iterate newParams.keys() and call newParams.delete(key) for keys matching that
regex, then set the current queryN entries and datasource as you already do, and
call setQueryParams(newParams).
---
Outside diff comments:
In `@web/src/components/dashboards/legacy/useLegacyDashboards.ts`:
- Around line 126-150: The code creates an empty URLSearchParams (const params =
new URLSearchParams()) which drops existing query params when switching
dashboards; change it to initialize params from the existing query params (e.g.,
new URLSearchParams(queryParams.toString() or current location.search) so
refreshInterval, endTime, timeRange and templating params are preserved. Ensure
getLegacyDashboardsUrl and the navigate call still update only
QueryParams.Dashboard while leaving other keys intact, and then continue to read
refreshInterval, params.get(QueryParams.EndTime) and
params.get(QueryParams.TimeRange) (used by dashboardsSetPollInterval,
dashboardsSetEndTime, dashboardsSetTimespan) from the preserved params.
In `@web/src/components/dashboards/perses/hooks/useDashboardsData.ts`:
- Around line 95-118: The all-namespaces mode exposes dashboards from all
projects but selection is still by name only (in useMemo
activeProjectDashboardsMetadata and in changeBoard), causing ambiguous/incorrect
matches; update the selector to carry both project and name (e.g., a composite
string "project/name" or an object) so changeBoard can unambiguously resolve the
target dashboard: when building options (activeProjectDashboardsMetadata) emit
the composite identifier, update changeBoard to parse that composite into
project and name, find the dashboard by matching both combinedDashboardsMetadata
items' project and name (not just name), compute projectToUse from the parsed
project if activeProject === ALL_NAMESPACES_KEY, and replace the newBoard !==
dashboardName guard with a comparison against the composite identifier to ensure
proper navigation handling.
- Around line 43-92: The memoized computed value combinedDashboardsMetadata
currently calls state setters setPreviousDashboards and setPreviousMetadata
inside the useMemo callback (violating render-phase purity); refactor by keeping
the pure computation in useMemo (compute newMetadata and dashboardsChanged using
persesDashboards, prevDashboards, prevMetadata) and move the side-effectful
updates into a useEffect that depends on persesDashboards and
combinedInitialLoad (or use refs: replace prevDashboards/prevMetadata state with
useRef to track previous values and update those refs inside useEffect),
ensuring combinedDashboardsMetadata remains read-only inside useMemo and all
calls to setPreviousDashboards/setPreviousMetadata occur in the effect or by
mutating refs.
In `@web/src/components/MetricsPage.tsx`:
- Around line 1101-1119: The current useEffect blocks updates from URL after the
first render (isFirstRender) so later changes to query{i} params are ignored and
then Redux is mirrored back into the URL (lines that mirror Redux into URL),
causing URL navigations to be reverted; modify sync logic so the URL remains the
source of truth: replace the one-time isFirstRender-only apply (useEffect that
calls queryBrowserDeleteAllQueries, queryBrowserPatchQuery, setFirstRenderFalse)
with a sync that runs whenever queryParams changes — parse all query{i} params
and dispatch patch/delete actions to reconcile Redux to the URL (use
queryBrowserDeleteAllQueries and queryBrowserPatchQuery), and add a small origin
flag or compare previous vs current params to avoid creating a feedback loop
with the existing Redux->URL mirror logic (i.e., only update Redux when params
differ). Ensure unique symbols referenced: useEffect (initialization block),
isFirstRender, dispatch, queryBrowserDeleteAllQueries, queryBrowserPatchQuery,
setFirstRenderFalse, and the Redux->URL mirror should skip updates when the
change originated from URL parsing.
---
Nitpick comments:
In `@Dockerfile.mcp`:
- Line 9: The Dockerfile currently copies a single patch file named
"@openshift-console+dynamic-plugin-sdk+4.22.0-prerelease.2.patch" (COPY
web/patches/@openshift-console+dynamic-plugin-sdk+4.22.0-prerelease.2.patch
web/patches/), which requires editing the Dockerfile each time a new patch is
added; change this to copy the entire patches directory (e.g., COPY web/patches/
web/patches/) so any new files in web/patches are automatically included in the
build and avoid future Dockerfile edits when adding patch-package files.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8a2912e9-1432-4cfc-98ff-6032ff561b76
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (85)
DockerfileDockerfile.artDockerfile.devDockerfile.dev-mcpDockerfile.konfluxDockerfile.mcpDockerfile.testMakefiledevspace.yamlweb/eslint.config.tsweb/locales/en/plugin__monitoring-plugin.jsonweb/package.jsonweb/patches/@openshift-console+dynamic-plugin-sdk+4.22.0-prerelease.2.patchweb/src/components/Incidents/IncidentsDetailsRowTable.tsxweb/src/components/Incidents/IncidentsPage.tsxweb/src/components/Incidents/IncidentsTable.tsxweb/src/components/Incidents/ToolbarItemFilter.tsxweb/src/components/Incidents/utils.tsweb/src/components/MetricsPage.tsxweb/src/components/alerting/AlertDetail/SilencedByTable.tsxweb/src/components/alerting/AlertList/AggregateAlertTableRow.tsxweb/src/components/alerting/AlertList/AlertTableRow.tsxweb/src/components/alerting/AlertRulesDetailsPage.tsxweb/src/components/alerting/AlertRulesPage.tsxweb/src/components/alerting/AlertUtils.tsxweb/src/components/alerting/AlertingPage.tsxweb/src/components/alerting/AlertsDetailsPage.tsxweb/src/components/alerting/SilenceCreatePage.tsxweb/src/components/alerting/SilenceEditPage.tsxweb/src/components/alerting/SilenceForm.tsxweb/src/components/alerting/SilencesDetailsPage.tsxweb/src/components/alerting/SilencesPage.tsxweb/src/components/alerting/SilencesUtils.tsxweb/src/components/alerting/useSelectedFilters.tsweb/src/components/console/console-shared/error/error-boundary.tsxweb/src/components/console/console-shared/src/components/loading/LoadError.tsxweb/src/components/console/console-shared/src/components/loading/LoadingBox.tsxweb/src/components/console/console-shared/src/components/query-browser/QueryBrowserTooltip.tsxweb/src/components/console/graphs/bar.tsxweb/src/components/console/graphs/promethues-graph.tsxweb/src/components/console/module/k8s/label-selector.jsweb/src/components/console/utils/button-bar.jsxweb/src/components/console/utils/link.tsxweb/src/components/console/utils/poll-hook.tsweb/src/components/console/utils/ref-width-hook.tsweb/src/components/console/utils/router.tsweb/src/components/console/utils/single-typeahead-dropdown.tsxweb/src/components/dashboards/legacy/bar-chart.tsxweb/src/components/dashboards/legacy/custom-time-range-modal.tsxweb/src/components/dashboards/legacy/legacy-dashboard-page.tsxweb/src/components/dashboards/legacy/legacy-dashboard.tsxweb/src/components/dashboards/legacy/legacy-variable-dropdowns.tsxweb/src/components/dashboards/legacy/time-dropdowns.tsxweb/src/components/dashboards/legacy/useLegacyDashboards.tsweb/src/components/dashboards/legacy/useOpenshiftProject.tsweb/src/components/dashboards/perses/dashboard-action-modals.tsxweb/src/components/dashboards/perses/dashboard-action-validations.tsweb/src/components/dashboards/perses/dashboard-app.tsxweb/src/components/dashboards/perses/dashboard-create-dialog.tsxweb/src/components/dashboards/perses/dashboard-frame.tsxweb/src/components/dashboards/perses/dashboard-header.tsxweb/src/components/dashboards/perses/dashboard-import-dialog.tsxweb/src/components/dashboards/perses/dashboard-list-frame.tsxweb/src/components/dashboards/perses/dashboard-list-page.tsxweb/src/components/dashboards/perses/dashboard-list.tsxweb/src/components/dashboards/perses/dashboard-page.tsxweb/src/components/dashboards/perses/datasource-api.tsweb/src/components/dashboards/perses/hooks/useDashboardsData.tsweb/src/components/dashboards/perses/project/ProjectBar.tsxweb/src/components/dashboards/perses/project/ProjectDropdown.tsxweb/src/components/dashboards/perses/project/useActiveProject.tsxweb/src/components/hooks/useMonitoringNamespace.tsweb/src/components/query-browser.tsxweb/src/components/redirects/prometheus-redirect-page.tsxweb/src/components/table-pagination.tsxweb/src/components/targets-page.tsxweb/src/contexts/MonitoringContext.tsxweb/src/e2e-tests-app.tsxweb/src/hooks/useAlerts.tsweb/src/index.d.tsweb/src/react-router-7-adapter.tsweb/src/store/actions.tsweb/src/store/thunks.tsweb/tsconfig.jsonweb/webpack.config.ts
💤 Files with no reviewable changes (6)
- web/src/components/dashboards/perses/dashboard-action-validations.ts
- web/src/components/console/utils/poll-hook.ts
- web/src/components/console/console-shared/src/components/query-browser/QueryBrowserTooltip.tsx
- web/src/components/console/utils/button-bar.jsx
- Dockerfile.konflux
- web/src/components/console/utils/router.ts
✅ Files skipped from review due to trivial changes (29)
- Dockerfile.art
- Dockerfile.dev-mcp
- Dockerfile.dev
- web/src/components/dashboards/legacy/bar-chart.tsx
- web/locales/en/plugin__monitoring-plugin.json
- web/src/components/dashboards/perses/dashboard-header.tsx
- web/src/components/redirects/prometheus-redirect-page.tsx
- web/src/components/console/utils/single-typeahead-dropdown.tsx
- web/src/components/console/module/k8s/label-selector.js
- web/src/components/alerting/AlertingPage.tsx
- web/src/index.d.ts
- web/src/components/Incidents/ToolbarItemFilter.tsx
- web/src/components/alerting/AlertList/AlertTableRow.tsx
- web/tsconfig.json
- web/src/components/dashboards/perses/dashboard-app.tsx
- Dockerfile
- web/src/components/alerting/SilenceForm.tsx
- web/src/components/alerting/AlertList/AggregateAlertTableRow.tsx
- web/src/components/dashboards/perses/dashboard-create-dialog.tsx
- web/src/components/dashboards/legacy/legacy-dashboard.tsx
- web/src/components/console/console-shared/src/components/loading/LoadingBox.tsx
- web/src/components/alerting/AlertsDetailsPage.tsx
- web/src/components/dashboards/perses/datasource-api.ts
- web/src/components/alerting/useSelectedFilters.ts
- web/src/e2e-tests-app.tsx
- web/src/components/alerting/SilencesPage.tsx
- web/src/components/Incidents/utils.ts
- web/src/components/console/console-shared/error/error-boundary.tsx
- web/src/components/console/graphs/bar.tsx
🚧 Files skipped from review as they are similar to previous changes (32)
- web/src/components/targets-page.tsx
- web/src/components/hooks/useMonitoringNamespace.ts
- web/src/components/alerting/AlertRulesDetailsPage.tsx
- Dockerfile.test
- web/src/components/dashboards/legacy/time-dropdowns.tsx
- web/src/components/alerting/SilenceEditPage.tsx
- web/src/components/dashboards/perses/dashboard-import-dialog.tsx
- web/src/components/Incidents/IncidentsDetailsRowTable.tsx
- web/src/components/alerting/AlertRulesPage.tsx
- web/src/components/console/utils/link.tsx
- web/src/components/dashboards/perses/dashboard-action-modals.tsx
- web/src/components/alerting/SilenceCreatePage.tsx
- web/src/store/thunks.ts
- web/src/components/alerting/SilencesUtils.tsx
- web/src/components/dashboards/perses/project/ProjectDropdown.tsx
- web/src/components/Incidents/IncidentsPage.tsx
- web/src/components/console/utils/ref-width-hook.ts
- devspace.yaml
- web/src/components/dashboards/perses/dashboard-page.tsx
- web/src/components/dashboards/legacy/legacy-dashboard-page.tsx
- web/src/store/actions.ts
- web/patches/@openshift-console+dynamic-plugin-sdk+4.22.0-prerelease.2.patch
- web/src/components/dashboards/legacy/legacy-variable-dropdowns.tsx
- web/src/react-router-7-adapter.ts
- web/src/contexts/MonitoringContext.tsx
- web/src/components/console/console-shared/src/components/loading/LoadError.tsx
- web/src/components/table-pagination.tsx
- web/src/components/dashboards/perses/project/useActiveProject.tsx
- web/src/components/console/graphs/promethues-graph.tsx
- web/src/components/query-browser.tsx
- web/src/hooks/useAlerts.ts
- web/package.json
| const { t } = useTranslation(process.env.I18N_NAMESPACE); | ||
| const [, setEndTime] = useQueryParam(QueryParams.EndTime, NumberParam); | ||
| const [, setTimeRange] = useQueryParam(QueryParams.TimeRange, NumberParam); | ||
| const [, setQueryParams] = useSearchParams(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== QueryParams definition(s) =="
fd -i 'query-params\.(ts|tsx)$' web/src/components --exec sed -n '1,220p' {}
echo
echo "== QueryParams usages in legacy dashboards =="
rg -n --type=ts --type=tsx -C2 '\bQueryParams\.' web/src/components/dashboards/legacy
echo
echo "== setQueryParams call sites in legacy dashboards =="
rg -n --type=tsx -C3 '\bsetQueryParams\s*\(' web/src/components/dashboards/legacyRepository: openshift/monitoring-plugin
Length of output: 661
🏁 Script executed:
# First, confirm the file exists and read the relevant sections
cat -n web/src/components/dashboards/legacy/custom-time-range-modal.tsx | head -80Repository: openshift/monitoring-plugin
Length of output: 3175
🏁 Script executed:
# Check file structure and imports to understand the code flow
head -30 web/src/components/dashboards/legacy/custom-time-range-modal.tsxRepository: openshift/monitoring-plugin
Length of output: 925
🏁 Script executed:
# Find other usages of the QueryParams enum to see what other parameters are being used
rg "QueryParams\." web/src/components/dashboards/legacy --max-count=20Repository: openshift/monitoring-plugin
Length of output: 2174
🏁 Script executed:
# Find other uses of setSearchParams or setQueryParams in the codebase to see patterns
rg "setSearchParams|setQueryParams" web/src --max-count=30Repository: openshift/monitoring-plugin
Length of output: 958
🏁 Script executed:
# Check how Dashboard and other params are set in legacy-dashboard.tsx
cat -n web/src/components/dashboards/legacy/legacy-dashboard.tsx | grep -A5 -B5 "setQueryParams\|setSearchParams" | head -60Repository: openshift/monitoring-plugin
Length of output: 1103
🏁 Script executed:
# Check how MetricsPage.tsx uses setQueryParams - it seems to use a different pattern
cat -n web/src/components/MetricsPage.tsx | grep -B10 -A10 "setQueryParams"Repository: openshift/monitoring-plugin
Length of output: 1851
🏁 Script executed:
# Let's see the full context of how Dashboard param is handled in legacy dashboards
rg "QueryParams.Dashboard" web/src/components/dashboards/legacy -B3 -A3Repository: openshift/monitoring-plugin
Length of output: 1630
🏁 Script executed:
# Check if there's a difference in how setSearchParams is being used in perses vs legacy
cat -n web/src/components/dashboards/perses/dashboard-list.tsx | grep -B5 -A5 "setSearchParams"Repository: openshift/monitoring-plugin
Length of output: 862
🏁 Script executed:
# Let's check if there's any documentation or type definition for setSearchParams behavior
rg "setSearchParams.*\[" web/src -A2 -B2 | head -50Repository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Check if the tuple array format is used elsewhere successfully or if it's a pattern issue
fd -e ts -e tsx web/src/components | xargs grep -l "setSearchParams\|setQueryParams" | xargs grep -A2 "setQueryParams\|setSearchParams" | grep "\[\[" | head -20Repository: openshift/monitoring-plugin
Length of output: 448
🏁 Script executed:
# Let's understand the full context by checking if Dashboard param is usually present during time range changes
cat -n web/src/components/dashboards/legacy/legacy-dashboard-page.tsx | head -100Repository: openshift/monitoring-plugin
Length of output: 3990
🏁 Script executed:
# Let's check what react-router version is being used
cat package.json | grep -i "react-router"Repository: openshift/monitoring-plugin
Length of output: 115
🏁 Script executed:
# Look at how other parts of the codebase use setSearchParams with the merge pattern
cat -n web/src/components/MetricsPage.tsx | sed -n '1089,1145p'Repository: openshift/monitoring-plugin
Length of output: 2514
🏁 Script executed:
# Check if there are tests or other evidence of what happens with setQueryParams in the tuple format
rg "setQueryParams.*\[\[" web/src -B5 -A5Repository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Check if package.json is at the repo root
find . -maxdepth 2 -name "package.json" -type fRepository: openshift/monitoring-plugin
Length of output: 89
🏁 Script executed:
# Let's check the complete flow to understand when the modal is called and what params would be present
cat -n web/src/components/dashboards/legacy/legacy-dashboard.tsx | head -100Repository: openshift/monitoring-plugin
Length of output: 3964
🏁 Script executed:
# Check how the custom time range modal is invoked
rg "CustomTimeRangeModal" web/src/components/dashboards/legacy -B5 -A5Repository: openshift/monitoring-plugin
Length of output: 4177
🏁 Script executed:
# Check the react-router version to understand setSearchParams behavior
cat web/package.json | grep -A2 -B2 "react-router"Repository: openshift/monitoring-plugin
Length of output: 296
🏁 Script executed:
# Let's see the complete time-dropdowns.tsx to understand when the modal is called and what params are already in URL
cat -n web/src/components/dashboards/legacy/time-dropdowns.tsx | head -120Repository: openshift/monitoring-plugin
Length of output: 5872
🏁 Script executed:
# Check useLegacyDashboards to see how Dashboard param is set initially
cat -n web/src/components/dashboards/legacy/useLegacyDashboards.ts | sed -n '1,100p'Repository: openshift/monitoring-plugin
Length of output: 4681
🏁 Script executed:
# Let's verify the exact behavior by checking if there are any test files that might show this issue
find web/src -name "*.test.*" -o -name "*.spec.*" | grep -i "custom.*time\|time.*range"Repository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Check the complete pattern in legacy-dashboard.tsx to see how it uses setQueryParams
cat -n web/src/components/dashboards/legacy/legacy-dashboard.tsx | sed -n '245,260p'Repository: openshift/monitoring-plugin
Length of output: 662
🏁 Script executed:
# Let's verify the URL structure by checking how the dashboard page is constructed
cat -n web/src/components/dashboards/legacy/useLegacyDashboards.ts | sed -n '100,180p'Repository: openshift/monitoring-plugin
Length of output: 3237
🏁 Script executed:
# Let's verify if the URLSearchParams pattern shown in the fix would work by checking react-router docs/types
# Let's also check if there's a utilities file that might have type definitions
rg "setSearchParams\|setQueryParams" web/src -A3 | grep -i "URLSearchParams\|URLSearchParams\|Partial"Repository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Let's look for any existing patterns where params are conditionally updated in other components
rg "new URLSearchParams" web/src/components -B2 -A5 | head -80Repository: openshift/monitoring-plugin
Length of output: 6108
🏁 Script executed:
# Let's check the exact pattern used in the perses dashboard for setting params
cat -n web/src/components/dashboards/perses/hooks/useDashboardsData.ts | sed -n '1,100p'Repository: openshift/monitoring-plugin
Length of output: 4688
🏁 Script executed:
# Let's look for the type signatures or how setSearchParams is used in other successful patterns
rg "setSearchParams.*prev" web/src -B2 -A5Repository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Let's verify the MetricsPage pattern more closely - does it actually work?
cat -n web/src/components/MetricsPage.tsx | sed -n '1134,1146p'Repository: openshift/monitoring-plugin
Length of output: 636
🏁 Script executed:
# Let's verify the react-router documentation behavior by checking if there are any comments or type definitions in the codebase
rg "setSearchParams.*function\|setSearchParams.*type" web/src -B2 -A3Repository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Let's check if the tuple array format is intentional or a mistake by looking at git history or any related issues
git log --oneline web/src/components/dashboards/legacy/custom-time-range-modal.tsx | head -10Repository: openshift/monitoring-plugin
Length of output: 110
🏁 Script executed:
# Check if there are any other places where the tuple array is used vs URLSearchParams pattern
rg "setQueryParams.*\[\[|setSearchParams.*\[\[" web/src -cRepository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Let's verify the exact react-router setSearchParams signature to understand tuple array behavior
# Check if there are any TypeScript type definitions or interfaces in the codebase
rg "interface.*SearchParams|type.*SearchParams" web/src -B2 -A5Repository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Let's verify by checking the actual usage pattern in MetricsPage more carefully - compare to the tuple array pattern
# Look at the exact difference between how setQueryParams is called in different places
cat -n web/src/components/dashboards/legacy/custom-time-range-modal.tsx | sed -n '65,77p'Repository: openshift/monitoring-plugin
Length of output: 636
🏁 Script executed:
# Let's check legacy-dashboard.tsx handleZoom to see if it has the same pattern
cat -n web/src/components/dashboards/legacy/legacy-dashboard.tsx | sed -n '251,256p'Repository: openshift/monitoring-plugin
Length of output: 311
🌐 Web query:
React Router v7 setSearchParams tuple array replace vs merge behavior
💡 Result:
In React Router v7, useSearchParams returns a tuple [searchParams, setSearchParams] where searchParams is a URLSearchParams object and setSearchParams updates the URL's search parameters, triggering navigation. setSearchParams accepts URLSearchParamsInit, which includes: - string (e.g., "?tab=1") - object (e.g., { tab: "1" } or { brand: ["nike", "reebok"] } for arrays) - array of [key, value] tuples (e.g., [["tab", "1"]]) - URLSearchParams object - updater function: (prev: URLSearchParams) => URLSearchParamsInit Direct arguments (string, object, array, URLSearchParams) REPLACE all existing search params with the new set. The updater function provides the current searchParams for modification (e.g., prev.set("tab", "2"); return prev;), enabling MERGE-like behavior by selectively updating while preserving others. Mutate with .set, .append, .delete, etc., then return the modified instance. Note: searchParams is stable but mutable; always call setSearchParams to sync with URL. Unlike React.useState, updater callbacks do not queue or batch across multiple setSearchParams calls in the same tick—each receives the current state independently, not pending updates (intentional since v7.7.0 to avoid internal state desync). Optionally pass NavigateOptions as second arg (e.g., setSearchParams(newParams, { replace: true }) to replace history entry instead of push; { preventScrollReset: true }). Examples: const [searchParams, setSearchParams] = useSearchParams; // Replace all setSearchParams({ tab: "2" }); // ?tab=2 (drops prior params) // Merge via updater setSearchParams(prev => { prev.set("tab", "2"); return prev; }); // Preserves other params, updates tab // Array tuple append (merge) setSearchParams(prev => [...prev.entries, ["new", "value"]]); // With replace setSearchParams(prev => { prev.set("tab", "2"); return prev; }, { replace: true }); Official docs confirm v7 behavior unchanged from v6 in this regard (v7.13+).
Citations:
- 1: https://reactrouter.com/7.13.1/api/hooks/useSearchParams
- 2: https://api.reactrouter.com/v7/types/react-router.SetURLSearchParams.html
- 3: https://reactrouter.com/api/hooks/useSearchParams
- 4: https://reactrouter.com/en/6.8.0/hooks/use-search-params
Preserve existing URL params when updating time range
On lines 71-74, setQueryParams with a tuple array replaces the entire query string instead of merging with existing params. This silently drops unrelated query parameters (like Dashboard, RefreshInterval, Units) that may already be in the URL, causing state loss in the dashboard.
Use the updater form with URLSearchParams to preserve other params:
Suggested fix
- setQueryParams([
- [QueryParams.EndTime, to.toString()],
- [QueryParams.TimeRange, (to - from).toString()],
- ]);
+ setQueryParams((prev) => {
+ const next = new URLSearchParams(prev);
+ next.set(QueryParams.EndTime, to.toString());
+ next.set(QueryParams.TimeRange, (to - from).toString());
+ return next;
+ });Also applies to: legacy-dashboard.tsx handleZoom (lines 251-256)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboards/legacy/custom-time-range-modal.tsx` at line 52,
The current call to setQueryParams replaces the entire query string and drops
unrelated params; update the call in custom-time-range-modal (the function
handling time range updates around where setQueryParams is used) to use the
updater form: pass a callback that receives existing URLSearchParams, clone or
modify it (new URLSearchParams(prev)), set/replace only the time-range keys
(e.g., "start","end" or whatever keys you use), and return the updated
URLSearchParams so other params (Dashboard, RefreshInterval, Units) are
preserved; apply the same pattern to legacy-dashboard.tsx handleZoom to merge
rather than overwrite query params.
| if (variableNamespace && variableNamespace !== routeNamespace) { | ||
| dispatch( | ||
| dashboardsPatchVariable('namespace', { | ||
| // Dashboards space variable shouldn't use the ALL_NAMESPACES_KEY | ||
| value: routeNamespace, |
There was a problem hiding this comment.
Guard against patching namespace to undefined.
routeNamespace is optional here. When it is missing, Line 36 still passes for any non-empty variableNamespace, and Line 40 dispatches { value: undefined }, which can clear the dashboard variable unexpectedly.
Proposed fix
- if (variableNamespace && variableNamespace !== routeNamespace) {
+ if (routeNamespace && variableNamespace && variableNamespace !== routeNamespace) {
dispatch(
dashboardsPatchVariable('namespace', {
// Dashboards space variable shouldn't use the ALL_NAMESPACES_KEY
value: routeNamespace,📝 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.
| if (variableNamespace && variableNamespace !== routeNamespace) { | |
| dispatch( | |
| dashboardsPatchVariable('namespace', { | |
| // Dashboards space variable shouldn't use the ALL_NAMESPACES_KEY | |
| value: routeNamespace, | |
| if (routeNamespace && variableNamespace && variableNamespace !== routeNamespace) { | |
| dispatch( | |
| dashboardsPatchVariable('namespace', { | |
| // Dashboards space variable shouldn't use the ALL_NAMESPACES_KEY | |
| value: routeNamespace, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboards/legacy/useOpenshiftProject.ts` around lines 36
- 40, The patch call can set the namespace variable to undefined because
routeNamespace is optional; update the condition around dispatching
dashboardsPatchVariable('namespace', { value: routeNamespace }) to ensure
routeNamespace is defined/non-empty before calling it (e.g., add an explicit
check for routeNamespace !== undefined/null/''), so you never dispatch a {
value: undefined } and thus never accidentally clear the dashboard variable;
reference the existing variableNamespace and routeNamespace checks and the
dashboardsPatchVariable('namespace', {...}) call when making this change.
| const newParams = new URLSearchParams(queryParams); | ||
| queryStrings.forEach((query, i) => newParams.set(`query${i}`, query || '')); | ||
| if (customDataSourceName) { | ||
| newParams.datasource = customDataSourceName; | ||
| newParams.set('datasource', customDataSourceName); | ||
| } | ||
| setAllQueryArguments(newParams); | ||
| }, [queryStrings, customDataSourceName]); | ||
| setQueryParams(newParams); | ||
| }, [queryStrings, customDataSourceName, isFirstRender, queryParams]); |
There was a problem hiding this comment.
Clear stale queryN params before serializing the current query list.
new URLSearchParams(queryParams) preserves removed queries. After a delete, old query1/query2 keys stay in the address bar, and a refresh restores them because the bootstrap loop on Line 1107 still finds those keys.
Suggested fix
useEffect(() => {
if (isFirstRender) {
return;
}
const newParams = new URLSearchParams(queryParams);
+ Array.from(newParams.keys())
+ .filter((key) => /^query\d+$/.test(key))
+ .forEach((key) => newParams.delete(key));
queryStrings.forEach((query, i) => newParams.set(`query${i}`, query || ''));
if (customDataSourceName) {
newParams.set('datasource', customDataSourceName);
}
setQueryParams(newParams);
}, [queryStrings, customDataSourceName, isFirstRender, queryParams]);📝 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.
| const newParams = new URLSearchParams(queryParams); | |
| queryStrings.forEach((query, i) => newParams.set(`query${i}`, query || '')); | |
| if (customDataSourceName) { | |
| newParams.datasource = customDataSourceName; | |
| newParams.set('datasource', customDataSourceName); | |
| } | |
| setAllQueryArguments(newParams); | |
| }, [queryStrings, customDataSourceName]); | |
| setQueryParams(newParams); | |
| }, [queryStrings, customDataSourceName, isFirstRender, queryParams]); | |
| const newParams = new URLSearchParams(queryParams); | |
| Array.from(newParams.keys()) | |
| .filter((key) => /^query\d+$/.test(key)) | |
| .forEach((key) => newParams.delete(key)); | |
| queryStrings.forEach((query, i) => newParams.set(`query${i}`, query || '')); | |
| if (customDataSourceName) { | |
| newParams.set('datasource', customDataSourceName); | |
| } | |
| setQueryParams(newParams); | |
| }, [queryStrings, customDataSourceName, isFirstRender, queryParams]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/MetricsPage.tsx` around lines 1139 - 1145, The
URLSearchParams created from queryParams preserves old queryN keys; before
re-adding the current queries in the effect that uses newParams, remove any
stale keys matching the pattern /^query\d+$/ so deleted queries don't remain in
the URL. In the effect where newParams is created (variables: newParams,
queryStrings, setQueryParams, customDataSourceName), iterate newParams.keys()
and call newParams.delete(key) for keys matching that regex, then set the
current queryN entries and datasource as you already do, and call
setQueryParams(newParams).
2 participants
This PR looks to update the monitoring-plugin to handle the 4.22 breaking changes in the console, comprising of shared package updates. The following package updates are the primary focus of this PR:
Due to an issue with
@openshift-console/dynamic-plugin-sdk@4.22-prerelease.2thepatch-packagemodule has been added with a single patch file to adjust one of the shared modules to enable building. This package and the patch file should be removed when the offending function has been fixed in a future@openshift-console/dynamic-plugin-sdkrelease.There should be no visible changes from this update