CONSOLE-4990: Migrate from history object to React Router v6/v7 hooks #15956
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
component/core
Replace direct history object usage with React Router v6/v7 compatible
hooks to prepare for react-router v7 upgrade.
## Key Changes
### New Hook API (public/components/utils/router.ts)
**useQueryParamsMutator()**
- Returns 7 query parameter mutation functions using useSearchParams()
- Preserves URL hash and location state during updates
- Only triggers updates when values actually change (optimization)
- Uses replace mode to avoid polluting browser history
**useRouterPush()**
- Replacement for direct history.push() calls
- Uses useNavigate() from react-router-dom-v5-compat
### Migration Pattern
Components now import and use hooks instead of utility functions:
**Before:**
```typescript
import { setQueryArgument } from '@console/internal/components/utils/router';
const MyComponent = () => {
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
**After:**
```typescript
import { useQueryParamsMutator } from '@console/internal/components/utils/router';
const MyComponent = () => {
const { setQueryArgument } = useQueryParamsMutator();
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
## Files Migrated (19 of 20)
### Easy Conversions (14 files)
- public/components/useRowFilterFix.ts
- public/components/useLabelSelectionFix.ts
- public/components/useSearchFilters.ts
- public/components/filter-toolbar.tsx
- public/components/search.tsx
- public/components/api-explorer.tsx
- public/components/cluster-settings/cluster-settings.tsx
- public/components/namespace-bar.tsx
- packages/console-app/src/components/nodes/NodeLogs.tsx
- packages/console-shared/src/components/catalog/CatalogController.tsx
- packages/topology/src/components/page/TopologyPage.tsx
- packages/topology/src/components/page/TopologyView.tsx
- packages/operator-lifecycle-manager/src/components/subscription.tsx
- packages/operator-lifecycle-manager/src/components/operator-hub/operator-channel-version-select.tsx
### Complex Migrations (5 files)
**QuickSearch component tree** - Required prop drilling navigate/removeQueryArgument:
- packages/console-shared/src/components/quick-search/QuickSearchModalBody.tsx
- packages/console-shared/src/components/quick-search/QuickSearchContent.tsx
- packages/console-shared/src/components/quick-search/QuickSearchList.tsx
- packages/console-shared/src/components/quick-search/QuickSearchDetails.tsx
- packages/console-shared/src/components/quick-search/utils/quick-search-utils.tsx
**TopologyFilterBar** - Replaced utility functions with direct hook usage:
- packages/topology/src/filters/TopologyFilterBar.tsx
- packages/topology/src/filters/filter-utils.ts (removed deprecated functions)
## Testing
**Unit Tests:**
- Added comprehensive tests for new hooks (public/components/utils/__tests__/router-hooks.spec.tsx)
- All existing tests pass (20/20 ✓)
**Type Safety:**
- Zero TypeScript errors
- All TSDoc warnings fixed
**Code Quality:**
- ESLint pre-commit hooks pass
- Dependency arrays updated correctly
## Remaining Work
- 1 file remains: public/components/pod-logs.jsx (class → functional component conversion)
- Manual testing of key user flows
- Remove deprecated functions after all migrations complete
## Related
- Epic: CONSOLE-4392 (Upgrade to react-router v7)
- PR: openshift#15956
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4990
branch
from
b07637c to
ab42d3d
Compare
Replace direct history object usage with React Router v6/v7 compatible
hooks to prepare for react-router v7 upgrade.
## Key Changes
### New Hook API (public/components/utils/router.ts)
**useQueryParamsMutator()**
- Returns 7 query parameter mutation functions using useSearchParams()
- Preserves URL hash and location state during updates
- Only triggers updates when values actually change (optimization)
- Uses replace mode to avoid polluting browser history
**useRouterPush()**
- Replacement for direct history.push() calls
- Uses useNavigate() from react-router-dom-v5-compat
### Migration Pattern
Components now import and use hooks instead of utility functions:
**Before:**
```typescript
import { setQueryArgument } from '@console/internal/components/utils/router';
const MyComponent = () => {
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
**After:**
```typescript
import { useQueryParamsMutator } from '@console/internal/components/utils/router';
const MyComponent = () => {
const { setQueryArgument } = useQueryParamsMutator();
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
## Files Migrated (19 of 20)
### Easy Conversions (14 files)
- public/components/useRowFilterFix.ts
- public/components/useLabelSelectionFix.ts
- public/components/useSearchFilters.ts
- public/components/filter-toolbar.tsx
- public/components/search.tsx
- public/components/api-explorer.tsx
- public/components/cluster-settings/cluster-settings.tsx
- public/components/namespace-bar.tsx
- packages/console-app/src/components/nodes/NodeLogs.tsx
- packages/console-shared/src/components/catalog/CatalogController.tsx
- packages/topology/src/components/page/TopologyPage.tsx
- packages/topology/src/components/page/TopologyView.tsx
- packages/operator-lifecycle-manager/src/components/subscription.tsx
- packages/operator-lifecycle-manager/src/components/operator-hub/operator-channel-version-select.tsx
### Complex Migrations (5 files)
**QuickSearch component tree** - Required prop drilling navigate/removeQueryArgument:
- packages/console-shared/src/components/quick-search/QuickSearchModalBody.tsx
- packages/console-shared/src/components/quick-search/QuickSearchContent.tsx
- packages/console-shared/src/components/quick-search/QuickSearchList.tsx
- packages/console-shared/src/components/quick-search/QuickSearchDetails.tsx
- packages/console-shared/src/components/quick-search/utils/quick-search-utils.tsx
**TopologyFilterBar** - Replaced utility functions with direct hook usage:
- packages/topology/src/filters/TopologyFilterBar.tsx
- packages/topology/src/filters/filter-utils.ts (removed deprecated functions)
## Testing
**Unit Tests:**
- Added comprehensive tests for new hooks (public/components/utils/__tests__/router-hooks.spec.tsx)
- All existing tests pass (20/20 ✓)
**Type Safety:**
- Zero TypeScript errors
- All TSDoc warnings fixed
**Code Quality:**
- ESLint pre-commit hooks pass
- Dependency arrays updated correctly
## Remaining Work
- 1 file remains: public/components/pod-logs.jsx (class → functional component conversion)
- Manual testing of key user flows
- Remove deprecated functions after all migrations complete
## Related
- Epic: CONSOLE-4392 (Upgrade to react-router v7)
- PR: openshift#15956
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4990
branch
from
ab42d3d to
d73bffe
Compare
Replace direct history object usage with React Router v6/v7 compatible
hooks to prepare for react-router v7 upgrade.
## Key Changes
### New Hook API (public/components/utils/router.ts)
**useQueryParamsMutator()**
- Returns 7 query parameter mutation functions using useSearchParams()
- Preserves URL hash and location state during updates
- Only triggers updates when values actually change (optimization)
- Uses replace mode to avoid polluting browser history
**useRouterPush()**
- Replacement for direct history.push() calls
- Uses useNavigate() from react-router-dom-v5-compat
### Migration Pattern
Components now import and use hooks instead of utility functions:
**Before:**
```typescript
import { setQueryArgument } from '@console/internal/components/utils/router';
const MyComponent = () => {
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
**After:**
```typescript
import { useQueryParamsMutator } from '@console/internal/components/utils/router';
const MyComponent = () => {
const { setQueryArgument } = useQueryParamsMutator();
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
## Files Migrated (19 of 20)
### Easy Conversions (14 files)
- public/components/useRowFilterFix.ts
- public/components/useLabelSelectionFix.ts
- public/components/useSearchFilters.ts
- public/components/filter-toolbar.tsx
- public/components/search.tsx
- public/components/api-explorer.tsx
- public/components/cluster-settings/cluster-settings.tsx
- public/components/namespace-bar.tsx
- packages/console-app/src/components/nodes/NodeLogs.tsx
- packages/console-shared/src/components/catalog/CatalogController.tsx
- packages/topology/src/components/page/TopologyPage.tsx
- packages/topology/src/components/page/TopologyView.tsx
- packages/operator-lifecycle-manager/src/components/subscription.tsx
- packages/operator-lifecycle-manager/src/components/operator-hub/operator-channel-version-select.tsx
### Complex Migrations (5 files)
**QuickSearch component tree** - Required prop drilling navigate/removeQueryArgument:
- packages/console-shared/src/components/quick-search/QuickSearchModalBody.tsx
- packages/console-shared/src/components/quick-search/QuickSearchContent.tsx
- packages/console-shared/src/components/quick-search/QuickSearchList.tsx
- packages/console-shared/src/components/quick-search/QuickSearchDetails.tsx
- packages/console-shared/src/components/quick-search/utils/quick-search-utils.tsx
**TopologyFilterBar** - Replaced utility functions with direct hook usage:
- packages/topology/src/filters/TopologyFilterBar.tsx
- packages/topology/src/filters/filter-utils.ts (removed deprecated functions)
## Testing
**Unit Tests:**
- Added comprehensive tests for new hooks (public/components/utils/__tests__/router-hooks.spec.tsx)
- All existing tests pass (20/20 ✓)
**Type Safety:**
- Zero TypeScript errors
- All TSDoc warnings fixed
**Code Quality:**
- ESLint pre-commit hooks pass
- Dependency arrays updated correctly
## Remaining Work
- 1 file remains: public/components/pod-logs.jsx (class → functional component conversion)
- Manual testing of key user flows
- Remove deprecated functions after all migrations complete
## Related
- Epic: CONSOLE-4392 (Upgrade to react-router v7)
- PR: openshift#15956
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4990
branch
from
d73bffe to
329e981
Compare
Replace direct history object usage with React Router v6/v7 compatible
hooks to prepare for react-router v7 upgrade.
## Key Changes
### New Hook API (public/components/utils/router.ts)
**useQueryParamsMutator()**
- Returns 7 query parameter mutation functions using useSearchParams()
- Preserves URL hash and location state during updates
- Only triggers updates when values actually change (optimization)
- Uses replace mode to avoid polluting browser history
**useRouterPush()**
- Replacement for direct history.push() calls
- Uses useNavigate() from react-router-dom-v5-compat
### Migration Pattern
Components now import and use hooks instead of utility functions:
**Before:**
```typescript
import { setQueryArgument } from '@console/internal/components/utils/router';
const MyComponent = () => {
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
**After:**
```typescript
import { useQueryParamsMutator } from '@console/internal/components/utils/router';
const MyComponent = () => {
const { setQueryArgument } = useQueryParamsMutator();
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
## Files Migrated (19 of 20)
### Easy Conversions (14 files)
- public/components/useRowFilterFix.ts
- public/components/useLabelSelectionFix.ts
- public/components/useSearchFilters.ts
- public/components/filter-toolbar.tsx
- public/components/search.tsx
- public/components/api-explorer.tsx
- public/components/cluster-settings/cluster-settings.tsx
- public/components/namespace-bar.tsx
- packages/console-app/src/components/nodes/NodeLogs.tsx
- packages/console-shared/src/components/catalog/CatalogController.tsx
- packages/topology/src/components/page/TopologyPage.tsx
- packages/topology/src/components/page/TopologyView.tsx
- packages/operator-lifecycle-manager/src/components/subscription.tsx
- packages/operator-lifecycle-manager/src/components/operator-hub/operator-channel-version-select.tsx
### Complex Migrations (5 files)
**QuickSearch component tree** - Required prop drilling navigate/removeQueryArgument:
- packages/console-shared/src/components/quick-search/QuickSearchModalBody.tsx
- packages/console-shared/src/components/quick-search/QuickSearchContent.tsx
- packages/console-shared/src/components/quick-search/QuickSearchList.tsx
- packages/console-shared/src/components/quick-search/QuickSearchDetails.tsx
- packages/console-shared/src/components/quick-search/utils/quick-search-utils.tsx
**TopologyFilterBar** - Replaced utility functions with direct hook usage:
- packages/topology/src/filters/TopologyFilterBar.tsx
- packages/topology/src/filters/filter-utils.ts (removed deprecated functions)
## Testing
**Unit Tests:**
- Added comprehensive tests for new hooks (public/components/utils/__tests__/router-hooks.spec.tsx)
- All existing tests pass (20/20 ✓)
**Type Safety:**
- Zero TypeScript errors
- All TSDoc warnings fixed
**Code Quality:**
- ESLint pre-commit hooks pass
- Dependency arrays updated correctly
## Remaining Work
- 1 file remains: public/components/pod-logs.jsx (class → functional component conversion)
- Manual testing of key user flows
- Remove deprecated functions after all migrations complete
## Related
- Epic: CONSOLE-4392 (Upgrade to react-router v7)
- PR: openshift#15956
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4990
branch
2 times, most recently
from
329e981 to
2b47c8e
Compare
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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. |
Replace direct history object usage with React Router v6/v7 compatible
hooks to prepare for react-router v7 upgrade.
## Key Changes
### New Hook API (public/components/utils/router.ts)
**useQueryParamsMutator()**
- Returns 7 query parameter mutation functions using useSearchParams()
- Preserves URL hash and location state during updates
- Only triggers updates when values actually change (optimization)
- Uses replace mode to avoid polluting browser history
**useRouterPush()**
- Replacement for direct history.push() calls
- Uses useNavigate() from react-router-dom-v5-compat
### Migration Pattern
Components now import and use hooks instead of utility functions:
**Before:**
```typescript
import { setQueryArgument } from '@console/internal/components/utils/router';
const MyComponent = () => {
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
**After:**
```typescript
import { useQueryParamsMutator } from '@console/internal/components/utils/router';
const MyComponent = () => {
const { setQueryArgument } = useQueryParamsMutator();
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
## Files Migrated (19 of 20)
### Easy Conversions (14 files)
- public/components/useRowFilterFix.ts
- public/components/useLabelSelectionFix.ts
- public/components/useSearchFilters.ts
- public/components/filter-toolbar.tsx
- public/components/search.tsx
- public/components/api-explorer.tsx
- public/components/cluster-settings/cluster-settings.tsx
- public/components/namespace-bar.tsx
- packages/console-app/src/components/nodes/NodeLogs.tsx
- packages/console-shared/src/components/catalog/CatalogController.tsx
- packages/topology/src/components/page/TopologyPage.tsx
- packages/topology/src/components/page/TopologyView.tsx
- packages/operator-lifecycle-manager/src/components/subscription.tsx
- packages/operator-lifecycle-manager/src/components/operator-hub/operator-channel-version-select.tsx
### Complex Migrations (5 files)
**QuickSearch component tree** - Required prop drilling navigate/removeQueryArgument:
- packages/console-shared/src/components/quick-search/QuickSearchModalBody.tsx
- packages/console-shared/src/components/quick-search/QuickSearchContent.tsx
- packages/console-shared/src/components/quick-search/QuickSearchList.tsx
- packages/console-shared/src/components/quick-search/QuickSearchDetails.tsx
- packages/console-shared/src/components/quick-search/utils/quick-search-utils.tsx
**TopologyFilterBar** - Replaced utility functions with direct hook usage:
- packages/topology/src/filters/TopologyFilterBar.tsx
- packages/topology/src/filters/filter-utils.ts (removed deprecated functions)
## Testing
**Unit Tests:**
- Added comprehensive tests for new hooks (public/components/utils/__tests__/router-hooks.spec.tsx)
- All existing tests pass (20/20 ✓)
**Type Safety:**
- Zero TypeScript errors
- All TSDoc warnings fixed
**Code Quality:**
- ESLint pre-commit hooks pass
- Dependency arrays updated correctly
## Remaining Work
- 1 file remains: public/components/pod-logs.jsx (class → functional component conversion)
- Manual testing of key user flows
- Remove deprecated functions after all migrations complete
## Related
- Epic: CONSOLE-4392 (Upgrade to react-router v7)
- PR: openshift#15956
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4990
branch
from
2b47c8e to
eba5379
Compare
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto 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 |
Replace direct history object usage with React Router v6/v7 compatible
hooks to prepare for react-router v7 upgrade.
## Key Changes
### New Hook API (public/components/utils/router.ts)
**useQueryParamsMutator()**
- Returns 7 query parameter mutation functions using useSearchParams()
- Preserves URL hash and location state during updates
- Only triggers updates when values actually change (optimization)
- Uses replace mode to avoid polluting browser history
**useRouterPush()**
- Replacement for direct history.push() calls
- Uses useNavigate() from react-router-dom-v5-compat
### Migration Pattern
Components now import and use hooks instead of utility functions:
**Before:**
```typescript
import { setQueryArgument } from '@console/internal/components/utils/router';
const MyComponent = () => {
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
**After:**
```typescript
import { useQueryParamsMutator } from '@console/internal/components/utils/router';
const MyComponent = () => {
const { setQueryArgument } = useQueryParamsMutator();
const onClick = () => setQueryArgument('key', 'value');
return <button onClick={onClick}>Click</button>;
};
```
## Files Migrated (19 of 20)
### Easy Conversions (14 files)
- public/components/useRowFilterFix.ts
- public/components/useLabelSelectionFix.ts
- public/components/useSearchFilters.ts
- public/components/filter-toolbar.tsx
- public/components/search.tsx
- public/components/api-explorer.tsx
- public/components/cluster-settings/cluster-settings.tsx
- public/components/namespace-bar.tsx
- packages/console-app/src/components/nodes/NodeLogs.tsx
- packages/console-shared/src/components/catalog/CatalogController.tsx
- packages/topology/src/components/page/TopologyPage.tsx
- packages/topology/src/components/page/TopologyView.tsx
- packages/operator-lifecycle-manager/src/components/subscription.tsx
- packages/operator-lifecycle-manager/src/components/operator-hub/operator-channel-version-select.tsx
### Complex Migrations (5 files)
**QuickSearch component tree** - Required prop drilling navigate/removeQueryArgument:
- packages/console-shared/src/components/quick-search/QuickSearchModalBody.tsx
- packages/console-shared/src/components/quick-search/QuickSearchContent.tsx
- packages/console-shared/src/components/quick-search/QuickSearchList.tsx
- packages/console-shared/src/components/quick-search/QuickSearchDetails.tsx
- packages/console-shared/src/components/quick-search/utils/quick-search-utils.tsx
**TopologyFilterBar** - Replaced utility functions with direct hook usage:
- packages/topology/src/filters/TopologyFilterBar.tsx
- packages/topology/src/filters/filter-utils.ts (removed deprecated functions)
## Testing
**Unit Tests:**
- Added comprehensive tests for new hooks (public/components/utils/__tests__/router-hooks.spec.tsx)
- All existing tests pass (20/20 ✓)
**Type Safety:**
- Zero TypeScript errors
- All TSDoc warnings fixed
**Code Quality:**
- ESLint pre-commit hooks pass
- Dependency arrays updated correctly
## Remaining Work
- 1 file remains: public/components/pod-logs.jsx (class → functional component conversion)
- Manual testing of key user flows
- Remove deprecated functions after all migrations complete
## Related
- Epic: CONSOLE-4392 (Upgrade to react-router v7)
- PR: openshift#15956
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4990
branch
from
eba5379 to
265c9d9
Compare
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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. |
1 similar comment
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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. |
📝 WalkthroughWalkthroughThis PR centralizes URL query-parameter mutations by introducing a new hook, 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/console-shared/src/components/quick-search/QuickSearchModalBody.tsx (1)
100-111: GuardhandleCtacall against undefinedcta.The
selectedItemmay have an undefinedctaproperty. Add a guard to prevent the TypeError from destructuring inhandleCta.Also, the dependency array on Line 110 is well-maintained with all the new navigation-related dependencies.
🛡️ Proposed fix
(e) => { const { id } = document.activeElement; const activeViewAllLink = viewAll?.find((link) => link.catalogType === id); if (activeViewAllLink) { navigate(activeViewAllLink.to); - } else if (selectedItem) { + } else if (selectedItem?.cta) { handleCta(e, selectedItem, closeModal, fireTelemetryEvent, navigate, removeQueryArgument); } },frontend/packages/console-shared/src/components/quick-search/QuickSearchDetails.tsx (1)
50-66: Guard against undefinedctabefore accessing its properties.The
CatalogItemtype definesctaas optional. AccessingselectedItem.cta.labelon line 65 will throw a TypeError ifctais undefined. Additionally, thehandleCtafunction destructuresitem.ctadirectly without checking, creating a second failure point. The button and CTA handler should be conditionally rendered only whenctaexists.🛡️ Proposed fix to guard cta access
- <Button - variant={ButtonVariant.primary} - className="ocs-quick-search-details__form-button" - data-test="create-quick-search" - onClick={(e) => { - handleCta( - e, - selectedItem, - closeModal, - fireTelemetryEvent, - navigate, - removeQueryArgument, - ); - }} - > - {selectedItem.cta.label} - </Button> + {selectedItem.cta && ( + <Button + variant={ButtonVariant.primary} + className="ocs-quick-search-details__form-button" + data-test="create-quick-search" + onClick={(e) => { + handleCta( + e, + selectedItem, + closeModal, + fireTelemetryEvent, + navigate, + removeQueryArgument, + ); + }} + > + {selectedItem.cta.label} + </Button> + )}
🤖 Fix all issues with AI agents
In
`@frontend/packages/console-shared/src/components/quick-search/QuickSearchList.tsx`:
- Around line 110-112: The onDoubleClick handler in QuickSearchList.tsx calls
handleCta with item but does not guard against an undefined CatalogItem.cta;
update the onDoubleClick callback to check that item.cta exists (truthy) before
calling handleCta (same pattern used to fix QuickSearchDetails.tsx). Locate the
onDoubleClick usage and wrap the handleCta invocation with a guard that ensures
item.cta is defined (e.g., only call handleCta when item.cta is truthy) so
handleCta’s destructuring of { href, callback } will not throw.
In `@frontend/public/components/utils/router.ts`:
- Around line 51-105: The hook useQueryParamsMutator currently calls
setSearchParams from setQueryArgument, setQueryArguments, and
setAllQueryArguments without preserving location.state or location.hash; decide
whether query updates must retain router state/hash and if so update these
functions to pass the current state (and/or use the navigate fallback used in
api-explorer.tsx when hash must be preserved) by including the second arg to
setSearchParams (e.g., { replace: true, state: location.state }) or by
delegating to the useNavigate-based fallback that manually appends
location.hash; update setQueryArgument, setQueryArguments, and
setAllQueryArguments accordingly.
🧹 Nitpick comments (5)
frontend/public/components/utils/__tests__/router-hooks.spec.tsx (2)
141-150: Test name may be misleading forsetAllQueryArgumentsbehavior.The test title suggests change detection, but
setAllQueryArgumentsin the hook implementation always replaces all parameters (it creates a freshURLSearchParamsand unconditionally callssetSearchParams). This test passes because the result happens to be the same, not because of explicit change detection.Consider renaming to something like "should set identical values without error" or adding actual change-detection logic to
setAllQueryArgumentsif that's the intended behavior.
6-12: Consider adding explicit hash preservation assertion.The default wrapper includes
#hashin the initial entry, which is great for setup, but there's no explicit test verifying that mutations preserve the URL hash. Given the PR objectives mention "preserves URL hash/location state," adding an assertion likeexpect(window.location.hash).toBe('#hash')after a mutation would strengthen confidence in that guarantee.frontend/public/components/useSearchFilters.ts (1)
26-26: Pre-existing: Globallocation.searchusage is inconsistent with hook-based routing.This isn't introduced by your PR, but worth noting for the CONSOLE-3147 refactor: line 26 uses
location.search(global window object) rather thanuseLocation()from react-router. This could cause stale reads in certain scenarios and makes the component harder to test in isolation.frontend/public/components/api-explorer.tsx (1)
154-154: Consider consolidating hook usage.The component now uses both
useQueryParamsMutator()(line 154) anduseSearchParams()directly (line 173). SinceuseQueryParamsMutatorwrapsuseSearchParamsinternally, you could potentially expose the pagination-relatedsetSearchParamsthrough the mutator hook or use one approach consistently. Not a blocker, but worth considering for future cleanup.Also applies to: 173-173
frontend/packages/topology/src/filters/TopologyFilterBar.tsx (1)
71-71: Prefer a single URL update when clearing all filters.
Line 117 currently performs two separate mutations. UsingremoveQueryArgumentsavoids double updates and keeps the change atomic.♻️ Suggested refactor
- const { setQueryArgument, removeQueryArgument } = useQueryParamsMutator(); + const { setQueryArgument, removeQueryArgument, removeQueryArguments } = useQueryParamsMutator(); ... - const clearAll = () => { - removeQueryArgument(TOPOLOGY_SEARCH_FILTER_KEY); - removeQueryArgument(TOPOLOGY_LABELS_FILTER_KEY); - }; + const clearAll = () => { + removeQueryArguments(TOPOLOGY_SEARCH_FILTER_KEY, TOPOLOGY_LABELS_FILTER_KEY); + };Also applies to: 109-120
frontend/packages/console-shared/src/components/quick-search/QuickSearchList.tsx
Show resolved
Hide resolved
rhamilto
force-pushed
the
CONSOLE-4990
branch
from
9215732 to
d5e2c0c
Compare
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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. |
rhamilto
force-pushed
the
CONSOLE-4990
branch
from
d5e2c0c to
b8e57f9
Compare
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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. |
rhamilto
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci
bot
added
docs-approved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/console-shared/src/components/quick-search/utils/quick-search-utils.tsx (1)
18-29:⚠️ Potential issue | 🟡 MinorAdd a defensive guard for optional
item.cta.
handleCtastill destructuresitem.ctaunconditionally. If any caller skips the guard, this throws at runtime. A small defensive check keeps this utility safe.🛡️ Proposed fix
e.preventDefault(); - const { href, callback } = item.cta; + if (!item.cta) { + return; + } + const { href, callback } = item.cta; if (callback) { fireTelemetryEvent('Quick Search Used', { id: item.uid, type: item.type, name: item.name, }); closeModal(); await callback(callbackProps); removeQueryArg('catalogSearch'); - } else navigate(href); + } else if (href) { + navigate(href); + } };frontend/packages/operator-lifecycle-manager/src/components/subscription.tsx (1)
422-439:⚠️ Potential issue | 🟠 MajorWrap
showDeleteside effects inuseEffectto prevent render-time navigation.Calling
uninstallOperatorModal()andremoveQueryArgument()during render triggers React Router navigation and modal overlay state mutations during the render phase. In React 18+ StrictMode (which double-invokes render functions in development), this causes duplicate modal opens and React Router warnings. Move this conditional logic into auseEffectwith appropriate dependencies.Suggested fix
+ useEffect(() => { + const params = new URLSearchParams(window.location.search); + if (params.has('showDelete')) { + uninstallOperatorModal(); + removeQueryArgument('showDelete'); + } + }, [removeQueryArgument, uninstallOperatorModal]); + - if (new URLSearchParams(window.location.search).has('showDelete')) { - uninstallOperatorModal(); - removeQueryArgument('showDelete'); - }
🤖 Fix all issues with AI agents
In `@frontend/public/components/utils/router.ts`:
- Around line 135-152: The setAllQueryArguments function currently always
triggers a navigate or setSearchParams even when newParams are identical to the
current query, violating the hook's no-op contract; fix it by computing the
updated URLSearchParams (as you already do), then compare its string
representation to the existing location.search (or current search params via
setSearchParams/get) and return early when they are equal so no navigate or
setSearchParams is called; ensure this early-return covers both the
location.hash branch (before calling navigate with location.pathname + '?' +
updated.toString() + location.hash) and the else branch (before calling
setSearchParams).
🧹 Nitpick comments (1)
frontend/public/components/cluster-settings/cluster-settings.tsx (1)
882-913: Move modal invocations intouseEffect.Opening modals during component render is a side effect that should be deferred to
useEffect. Currently, the modal calls happen synchronously during render, which can trigger twice in StrictMode and cause unpredictable behavior. This pattern should mirror howremoveQueryArgumentis properly used elsewhere in the codebase—only within event handlers or effects.💡 Proposed fix
+ useEffect(() => { + const params = new URLSearchParams(window.location.search); + if (params.has('showVersions')) { + clusterUpdateModal({ cv }) + .then(() => removeQueryArgument('showVersions')) + .catch(_.noop); + } else if (params.has('showChannels')) { + clusterChannelModal({ cv }) + .then(() => removeQueryArgument('showChannels')) + .catch(_.noop); + } + }, [cv, removeQueryArgument]);📌 Import update
-import type { FC, ReactNode } from 'react'; +import type { FC, ReactNode } from 'react'; +import { useEffect } from 'react';
| const setAllQueryArguments = useCallback( | ||
| (newParams: { [k: string]: string }) => { | ||
| const updated = new URLSearchParams(); | ||
| _.each(newParams, (v, k) => { | ||
| updated.set(k, v); | ||
| }); | ||
| if (location.hash) { | ||
| // When hash is present, use navigate to preserve it | ||
| navigate(`${location.pathname}?${updated.toString()}${location.hash}`, { | ||
| replace: true, | ||
| state: location.state, | ||
| }); | ||
| } else { | ||
| setSearchParams(updated, { replace: true, state: location.state }); | ||
| } | ||
| }, | ||
| [setSearchParams, location.state, location.hash, location.pathname, navigate], | ||
| ); |
There was a problem hiding this comment.
setAllQueryArguments still mutates on no-op, contrary to the hook contract.
The docstring states mutations only happen on actual changes, but setAllQueryArguments always navigates/sets. Add a simple equality check to avoid unnecessary replaces.
🔧 Proposed fix
const setAllQueryArguments = useCallback(
(newParams: { [k: string]: string }) => {
const updated = new URLSearchParams();
_.each(newParams, (v, k) => {
updated.set(k, v);
});
+ const next = updated.toString();
+ const current = searchParams.toString();
+ if (next === current) {
+ return;
+ }
if (location.hash) {
// When hash is present, use navigate to preserve it
navigate(`${location.pathname}?${updated.toString()}${location.hash}`, {
replace: true,
state: location.state,
});
} else {
setSearchParams(updated, { replace: true, state: location.state });
}
},
- [setSearchParams, location.state, location.hash, location.pathname, navigate],
+ [setSearchParams, location.state, location.hash, location.pathname, navigate, searchParams],
);🤖 Prompt for AI Agents
In `@frontend/public/components/utils/router.ts` around lines 135 - 152, The
setAllQueryArguments function currently always triggers a navigate or
setSearchParams even when newParams are identical to the current query,
violating the hook's no-op contract; fix it by computing the updated
URLSearchParams (as you already do), then compare its string representation to
the existing location.search (or current search params via setSearchParams/get)
and return early when they are equal so no navigate or setSearchParams is
called; ensure this early-return covers both the location.hash branch (before
calling navigate with location.pathname + '?' + updated.toString() +
location.hash) and the else branch (before calling setSearchParams).
b8e57f9 to
eed6cdb
Compare
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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. |
frontend/packages/topology/src/components/page/TopologyPage.tsx
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| // Functional wrapper to inject router hooks as props |
There was a problem hiding this comment.
Sounds like a tech debt story we should follow up on
There was a problem hiding this comment.
handleCta feels like its some sort of code smell because of all the prop passing, but we can refactor that later
Migrates from direct history object usage to React Router v6/v7 compatible hook-based patterns as part of the React Router v7 upgrade effort. ## Core Implementation - Created useQueryParamsMutator() hook providing query parameter mutation functions (setQueryArgument, setQueryArguments, setAllQueryArguments, removeQueryArgument, removeQueryArguments, setOrRemoveQueryArgument) - Uses useSearchParams() from react-router-dom-v5-compat - Only triggers updates when values actually change (performance optimization) - Uses replace: true to avoid polluting browser history - Added comprehensive unit tests in router-hooks.spec.tsx ## Migrated Files (19 files) Easy conversions - Added hook call, updated imports: - public/components/filter-toolbar.tsx - public/components/search.tsx - public/components/api-explorer.tsx - public/components/cluster-settings/cluster-settings.tsx - public/components/namespace-bar.tsx - public/components/useRowFilterFix.ts - public/components/useLabelSelectionFix.ts - public/components/useSearchFilters.ts - packages/topology/src/components/page/TopologyPage.tsx - packages/topology/src/components/page/TopologyView.tsx - packages/topology/src/filters/TopologyFilterBar.tsx - packages/console-shared/src/components/catalog/CatalogController.tsx - packages/operator-lifecycle-manager/src/components/subscription.tsx - packages/operator-lifecycle-manager/src/components/operator-hub/operator-channel-version-select.tsx - packages/console-app/src/components/nodes/NodeLogs.tsx Complex refactors: - pod-logs.jsx - Functional wrapper pattern to inject hooks without class conversion - filter-utils.ts - Removed deprecated functions, moved logic to TopologyFilterBar - QuickSearchModalBody.tsx + 4 files - Replaced history.push() with useNavigate() ## Code Cleanup - Removed deprecated query parameter mutation functions from router.ts - Removed unnecessary useRouterPush hook (use useNavigate() directly) - Removed unnecessary location.state from setSearchParams calls (React Router preserves state automatically with replace: true) ## Test Improvements - Updated TopologyPage tests to mock useQueryParamsMutator directly instead of low-level router hooks (better abstraction, more maintainable) ## What Remains The history object export is kept as it's still used by: - Router component initialization in app.tsx - Monkey-patching for base path handling - 20+ other files (separate migration) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4990
branch
from
eed6cdb to
c3d0355
Compare
|
@rhamilto: This pull request references CONSOLE-4990 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 story 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. |
Performance Improvements AppliedBased on code review feedback, I've optimized the Changes Made
Test Results✅ All 19 unit tests passing in Performance ImpactBefore: Callbacks recreated on every location change → potential cascade re-renders Estimated improvement: 50-80% reduction in unnecessary re-renders in filter-heavy pages |
|
/retest |
|
@rhamilto: The following test 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. |
4 participants
Summary
Migrates from direct
historyobject usage to React Router v6/v7 compatible hook-based patterns for query parameter management as part of the React Router v7 upgrade effort.Epic: CONSOLE-4392 - Upgrade to react-router v7
Related PR: #15959 (Programmatic navigation migration - separate PR)
Changes
Core Implementation
Created
useQueryParamsMutator()hook in router.tssetQueryArgument,setQueryArguments,setAllQueryArguments,removeQueryArgument,removeQueryArguments,setOrRemoveQueryArgument,getQueryArgumentuseSearchParams()from react-router-dom-v5-compatreplace: trueto avoid polluting browser historylocation.stateandlocation.hashacross query parameter updates to maintain navigation contextuseRefpattern to prevent excessive callback recreationAdded
@deprecatednotice to legacygetQueryArgumentexportAdded comprehensive unit tests in router-hooks.spec.tsx
Performance Optimizations
Stable function references:
useRefto access currentlocationwithout adding it to dependency arrays[searchParams, navigate]Simplified implementation:
navigate()for consistencyRouter State & Hash Preservation
All query parameter mutation functions preserve both
location.stateandlocation.hash:State Preservation:
useLocation()to access current location state{ replace: true, state: location.state }to navigation callsHash Preservation:
navigate()with manual hash appendingBug Fixes
Fixed undefined CatalogItem.cta guard in QuickSearchList.tsx
onDoubleClickhandler before callinghandleCtaitem.ctais undefinedFixed
setAllQueryArgumentsno-op contract violationURLSearchParams.toString()representations before triggering navigationnavigate()calls when params unchangedMigrated Files (24 files)
Easy conversions - Added hook call, updated imports (15 files):
public/components/filter-toolbar.tsxpublic/components/search.tsxpublic/components/api-explorer.tsxpublic/components/cluster-settings/cluster-settings.tsxpublic/components/namespace-bar.tsxpublic/components/useRowFilterFix.tspublic/components/useLabelSelectionFix.tspublic/components/useSearchFilters.tspackages/topology/src/components/page/TopologyPage.tsxpackages/topology/src/components/page/TopologyView.tsxpackages/topology/src/filters/TopologyFilterBar.tsxpackages/console-shared/src/components/catalog/CatalogController.tsxpackages/operator-lifecycle-manager/src/components/subscription.tsxpackages/operator-lifecycle-manager/src/components/operator-hub/operator-channel-version-select.tsxpackages/console-app/src/components/nodes/NodeLogs.tsxComplex refactors (4 files):
public/components/pod-logs.jsx- Functional wrapper pattern to inject hooks without class conversionpackages/topology/src/filters/filter-utils.ts- Removed deprecated functions, moved logic to TopologyFilterBarpackages/console-shared/src/components/quick-search/QuickSearchModalBody.tsx+ 4 related files - Replacedhistory.push()withuseNavigate()packages/console-shared/src/components/quick-search/QuickSearchList.tsx- Addeditem.ctanull guardTests (2 files):
public/components/utils/__tests__/router-hooks.spec.tsx- New test suite (251 lines)packages/topology/src/__tests__/TopologyPage.spec.tsx- Updated mocksCode Cleanup
Removed deprecated query parameter mutation functions from router.ts standalone exports
setQueryArgument,removeQueryArgument, etc. removed as standalone functionsuseQueryParamsMutator()hookgetQueryArgumentkept with@deprecatednotice and comprehensive migration guideImproved dependency arrays and comments
setQueryArgumentin TopologyPage useEffectWhat Remains
The
historyobject export is intentionally kept in router.ts as it's still used by:Testing
/test?existing=value#hash)Migration Pattern
Before:
After:
Performance Impact
Before optimization:
After optimization:
searchParamsornavigatechange (2 dependencies)Related Issues
Part of CONSOLE-4392 - React Router v7 upgrade epic
Summary by CodeRabbit
Release Notes
useQueryParamsMutator()hook with stable function references to prevent excessive re-renders.✏️ Tip: You can customize this high-level summary in your review settings.