[WIP] CONSOLE-4990: Replace history object navigation with useNavigate hook #15959
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@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 |
openshift-ci
bot
added
component/dev-console
rhamilto
changed the title
frontend/packages/console-app/src/actions/providers/cronjob-provider.ts
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/dynamic-form/index.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/quick-search/QuickSearchContent.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/quick-search/QuickSearchDetails.tsx
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/edit-application/EditApplication.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-items.tsx
Outdated
Show resolved
Hide resolved
rhamilto
force-pushed
the
CONSOLE-4990-programmatic-navigation
branch
from
fab6845 to
4c78f4b
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
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
rhamilto
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
rhamilto
force-pushed
the
CONSOLE-4990-programmatic-navigation
branch
2 times, most recently
from
738897a to
b7a45bf
Compare
openshift-ci
bot
added
the
kind/i18n
rhamilto
changed the title
rhamilto
force-pushed
the
CONSOLE-4990-programmatic-navigation
branch
from
f466ba8 to
e896642
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. |
rhamilto
force-pushed
the
CONSOLE-4990-programmatic-navigation
branch
from
e896642 to
98e7956
Compare
|
@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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
frontend/packages/dev-console/src/components/buildconfig/__tests__/BuildConfigFormPage.spec.tsx (1)
95-101:⚠️ Potential issue | 🟡 Minor
findBy*queries requireawaitor should be replaced withgetBy*.The
findBy*queries (lines 97-101) return Promises and are designed for async scenarios. Withoutawait, these assertions fire-and-forget—the test can pass even if the elements never appear because the Promise rejection happens after the test completes.Since your mocks return synchronously,
getBy*is the appropriate choice here for immediate assertions. Alternatively, make the testasyncandawaiteach call.Proposed fix using synchronous queries
it('should fetch BuildConfig and render edit form when BuildConfig is loaded', () => { // ... setup ... const renderResult = renderWithProviders(<BuildConfigFormPage />); expect(renderResult.queryByText('Create BuildConfig')).toBeFalsy(); - renderResult.findByText('Edit BuildConfig'); - renderResult.findByText('Configure via:'); - renderResult.findByText('Form view'); - renderResult.findByText('YAML view'); - renderResult.findByRole('button', { name: /Submit/ }); + renderResult.getByText('Edit BuildConfig'); + renderResult.getByText('Configure via:'); + renderResult.getByText('Form view'); + renderResult.getByText('YAML view'); + renderResult.getByRole('button', { name: /Submit/ });frontend/packages/dev-console/src/utils/add-page-utils.ts (1)
44-45:⚠️ Potential issue | 🟠 MajorReturn type contract mismatch:
resolvedHrefcan returnnullbut declares return type asstring.Call sites lack consistent null guards:
AddCardItem.tsxline 63: Only checksif (href)but doesn't validatenamespace, allowingnullto pass tonavigateTo()when namespace is falsy.add-resources.tsxline 34–47:resolvedURLWithParams()passesresolvedHref()result directly to.indexOf()without null guards, riskingTypeErrorat runtime.Recommend either updating the return type annotation to
string | nulland adding null checks at all call sites, or ensuringresolvedHrefnever returnsnullby handling the falsy case differently.frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepositoryPage.tsx (1)
3-50:⚠️ Potential issue | 🟠 MajorRe-validate RBAC permissions when switching namespaces.
The namespace switch doesn't re-check user permissions (
useAccessReview) for the target namespace before navigating. The currentcanCreateHCRandcanCreatePHCRchecks fire once at mount; switching to a restricted namespace still renders the form, leading to failed server-side submissions.Additionally, extract
handleNamespaceChangeas a standalone function (following the pattern inProjectDetailsPageandMonitoringPage) for better testability and clarity. The silent no-op whenns === namespaceshould be intentional but is worth documenting why the conditional guards against unnecessary navigation.frontend/packages/console-app/src/components/user-preferences/UserPreferencePage.tsx (1)
13-127:⚠️ Potential issue | 🟠 MajorFix ref creation pattern in tab rendering loop—refs are being recreated on every render.
The navigation behavior using
{ replace: true }is correct and will keep the history stack clean. However, line 76 creates refs inside the reduce function within useMemo, causing new ref objects to be instantiated on every useMemo execution (controlled byactiveTabId,sortedUserPreferenceGroups,sortedUserPreferenceItems). This breaks ref stability and can cause focus issues, DOM reference loss, and unnecessary overhead.Move ref creation out of the reduce to either:
- Use
useRefto maintain a stable Map/object of refs keyed by group id- Or refactor the render to avoid refs if they're only used for scroll-into-view (which PatternFly Tabs can handle natively)
Additionally, the Tab component combines
hrefwithpreventDefault()+navigate()—consider using only the programmatic navigation or removing the preventDefault if you want to allow ctrl/cmd+click behavior.frontend/packages/console-app/src/components/pdb/PDBForm.tsx (1)
30-307:⚠️ Potential issue | 🟠 MajorPost-submit navigation should use
replace: trueto prevent users from returning to the incomplete form after successful creation.The
navigate(resourcePathFromModel(...))call at line 155 creates a new history entry. Add{ replace: true }as the second argument so users don't inadvertently land back on the form when navigating back from the resource details page.Also, review the useEffect dependency array (line 112): it includes
requirement(which is set within the effect itself) andonFormValuesChange(which itself depends ononChangeandexistingResource). This creates a circular dependency chain that could trigger unexpected re-renders. Narrow the dependencies to what actually needs to trigger the effect—likely justexistingResourceand the derivedinitialValuesFromK8sResourceresult.The cancel navigation with
navigate(-1)is appropriate, but ensure this form is always entered via standard push navigation (not replaced), otherwise the back button may exit the feature entirely.
🤖 Fix all issues with AI agents
In `@frontend/packages/console-app/src/actions/hooks/useCronJobActions.ts`:
- Around line 60-62: The JSDoc for useCronJobActions.ts currently states that
invalid actionCreators produce an empty `actions` array, but the code (the
reduce over `memoizedFilterActions` using `factory` and checking `typeof fn ===
'function'`) actually skips invalid creators and returns the valid ones; update
the JSDoc comment above the function to state that invalid creators are ignored
and the returned `actions` array contains only the valid actions (e.g.,
reference `memoizedFilterActions`, `factory`, and the reduce logic), or
alternatively change the implementation to instead detect any invalid creator
and return an empty array—pick one behavior and make the doc and code consistent
across the related block(s) around the reduce and lines 110-122.
In
`@frontend/packages/console-shared/src/components/quick-search/QuickSearchDetails.tsx`:
- Around line 63-66: selectedItem.cta is optional on CatalogItem so accessing
selectedItem.cta.label and destructuring item.cta in handleCta can throw; update
QuickSearchDetails (where the Button renders selectedItem.cta.label) to only
render the CTA Button if selectedItem.cta exists (e.g., wrap the Button in a
guard like if (selectedItem.cta) or use optional chaining), and update handleCta
in quick-search-utils.tsx to avoid direct destructuring of item.cta (use
optional chaining or default to an empty object before reading href/callback) so
code safely handles missing cta.
In
`@frontend/packages/dev-console/src/components/buildconfig/__tests__/BuildConfigFormPage.spec.tsx`:
- Around line 116-119: The test calls renderWithProviders(<BuildConfigFormPage
/>) and then uses renderResult.findByText('Error Loading'),
renderResult.findByText('Edit BuildConfig'), and
renderResult.findByText('Something went wrong') without awaiting them, so they
don't actually assert; update the assertions in BuildConfigFormPage.spec.tsx to
await each call to renderResult.findByText(...) (or replace with synchronous
queries like getByText if appropriate) so the promises resolve and the test
reliably verifies the presence of those texts; target the
renderResult.findByText calls related to BuildConfigFormPage and keep the same
expected strings.
In
`@frontend/packages/dev-console/src/components/jar-file-upload/jar-file-upload-utils.ts`:
- Around line 3-5: The handler's signature is wrong for the extension system:
change jarFileUploadHandler to remove the third parameter (navigate:
NavigateFunction) and instead return the navigation path as a string (e.g.
`/upload-jar/ns/${namespace}`) so callers can invoke navigate themselves; update
jarFileUploadHandler (and any direct imports) to export a function
jarFileUploadHandler(file: File, namespace: string): string and ensure the
extension caller (e.g. requiredFileExtension.properties.handler in
file-upload-context.ts) uses the returned string with navigate, or otherwise
perform navigation in the calling component.
In `@frontend/packages/knative-plugin/src/utils/create-eventsources-utils.ts`:
- Around line 361-364: The code calls
perspectiveData.properties.importRedirectURL() and immediately passes its result
(redirectURL) to navigate, which can throw if redirectURL is undefined or empty;
before calling navigate(redirectURL) in the try block, add a guard that checks
if redirectURL is a non-empty string (e.g., if (!redirectURL) {
processLogger?.warn(...) or console.warn(...) and return/skip navigation }), and
only call navigate when the guard passes so you avoid routing with falsy values
and log the skipped case for debugging.
🧹 Nitpick comments (16)
frontend/packages/dev-console/src/components/import/jar/useUploadJarFormToast.ts (1)
27-32: Consider adding a guard for missingBuildConfigModelin the response.Pre-existing code, but since the PR is adding null guards elsewhere during this migration: if
resp.find()returnsundefined(noBuildConfigModelin the response), Line 31 will throw aTypeErrorwhen accessingcreatedBuildConfig.metadata.name.🛡️ Proposed defensive guard
return useCallback( (resp) => { const createdBuildConfig = resp.find((d) => d.kind === BuildConfigModel.kind); + if (!createdBuildConfig) { + // eslint-disable-next-line no-console + console.warn('BuildConfig not found in response, skipping toast'); + return; + } const ownBuilds = getOwnedResources(createdBuildConfig, builds); const buildName = `${createdBuildConfig.metadata.name}-${ownBuilds.length + 1}`;frontend/packages/dev-console/src/components/import/ImportSamplePage.tsx (1)
112-128: Consider memoizinghandleSubmitfor consistency.While Formik is reasonably tolerant of unstable
onSubmitreferences, wrappinghandleSubmitinuseCallbackwould align with the memoization pattern already applied tohandleCanceland prevent potential unnecessary re-renders of form internals.♻️ Optional: Wrap handleSubmit in useCallback
- const handleSubmit = (values, actions) => { + const handleSubmit = useCallback((values, actions) => { const resourceActions = createOrUpdateResources( t, values, imageStream as K8sResourceKind, false, true, ).then(() => createOrUpdateResources(t, values, imageStream as K8sResourceKind)); return resourceActions .then(() => { navigate(`/topology/ns/${namespace}`); }) .catch((err) => { actions.setStatus({ submitError: err.message }); }); - }; + }, [t, imageStream, navigate, namespace]);frontend/packages/dev-console/src/components/import/DeployImage.tsx (1)
161-192: handleSubmit navigation is correct; consider memoization for consistency.The navigation logic on success correctly routes to the topology view with an optional
selectIdquery parameter. The error handling appropriately keeps the user on the form with a submit error status.Similar to
ImportSamplePage, you may optionally wraphandleSubmitinuseCallbackfor consistency with thehandleCancelpattern, though Formik handles unstable references reasonably well.♻️ Optional: Wrap handleSubmit in useCallback
- const handleSubmit = ( + const handleSubmit = useCallback(( values: DeployImageFormData, helpers: FormikHelpers<DeployImageFormData>, ) => { // ... existing implementation - }; + }, [postFormCallback, navigate]);frontend/public/components/QuickCreate.tsx (3)
177-195: Missing TypeScript prop types for exported component.The component props
{ namespace, className }lack type annotations. For an exported public component, explicit typing improves maintainability and catches misuse at compile time.📝 Suggested type annotation
+type QuickCreateButtonProps = { + namespace: string; + className?: string; +}; + -export const QuickCreateImportFromGit = ({ namespace, className }) => { +export const QuickCreateImportFromGit: FC<QuickCreateButtonProps> = ({ namespace, className }) => {
190-193: Consider using PatternFlyButtonfor consistent styling and accessibility.A plain
<button>works functionally, but PatternFly'sButtoncomponent provides consistent styling, focus states, and built-in accessibility attributes. If this is intentional (e.g., styled externally viaclassName), feel free to disregard.♻️ Optional PatternFly Button usage
+import { Button } from '@patternfly/react-core'; ... - <button type="button" onClick={handleClick} className={className}> + <Button variant="link" onClick={handleClick} className={className}> {t('public~Import from Git')} - </button> + </Button>
197-215: Same observations apply toQuickCreateContainerImages.This component mirrors
QuickCreateImportFromGit—same notes on TypeScript prop types and optional PatternFlyButtonusage apply here. TheuseCallbackimplementation with[navigate, namespace]dependencies is correct.📝 Suggested type annotation
-export const QuickCreateContainerImages = ({ namespace, className }) => { +export const QuickCreateContainerImages: FC<QuickCreateButtonProps> = ({ namespace, className }) => {frontend/packages/console-shared/src/components/error/error-boundary.tsx (1)
33-46: Consider adding explicit TypeScript types to class methods.The
constructorandcomponentDidCatchparameters lack explicit types. While inference handles this in most cases, explicit typing improves IDE support and maintainability—especially for error boundaries where proper error/errorInfo shapes matter.♻️ Proposed typing improvements
- constructor(props) { + constructor(props: ErrorBoundaryProps) { super(props); this.state = this.defaultState; } - componentDidCatch(error, errorInfo) { + componentDidCatch(error: Error, errorInfo: React.ErrorInfo) { this.setState({ hasError: true, error, errorInfo, });frontend/packages/dev-console/src/components/projects/details/ProjectDetailsPage.tsx (1)
115-130: Consider memoizing theonNamespaceChangecallback.The inline arrow function on line 124 creates a new reference on every render. While
NamespacedPagelikely doesn't rely on referential equality for this prop, wrapping it inuseCallbackaligns with the PR's stated pattern and prevents unnecessary re-renders ifNamespacedPageever becomes optimized withReact.memo.♻️ Optional: memoize the callback
export const ProjectDetailsPage: FC<MonitoringPageProps> = (props) => { const { t } = useTranslation(); const navigate = useNavigate(); + const handleNamespaceChangeCallback = React.useCallback( + (newNamespace: string) => handleNamespaceChange(newNamespace, navigate), + [navigate], + ); return ( <> <DocumentTitle>{t('devconsole~Project Details')}</DocumentTitle> <NamespacedPage hideApplications variant={NamespacedPageVariants.light} - onNamespaceChange={(newNamespace) => handleNamespaceChange(newNamespace, navigate)} + onNamespaceChange={handleNamespaceChangeCallback} > <PageContentsWithStartGuide {...props} /> </NamespacedPage> </> ); };frontend/packages/dev-console/src/components/projects/details/__tests__/ProjectDetailsPage.spec.tsx (2)
23-27: Stale mock:historyis no longer used by the component.The
history: { push: jest.fn() }mock is dead code since the component now usesuseNavigateinstead. Consider removing it to reduce test maintenance burden and avoid confusion.🧹 Remove stale history mock
jest.mock('@console/internal/components/utils', () => ({ - history: { push: jest.fn() }, useAccessReview: jest.fn(), Page: {}, }));
91-146: Consider adding a test for the namespace change navigation behavior.The existing tests verify rendering behavior but don't exercise
handleNamespaceChange. Since this is the core logic being migrated, a test confirming that selectingALL_NAMESPACES_KEYtriggers navigation toPROJECT_DETAILS_ALL_NS_PAGE_URIwould increase confidence in the migration.💡 Example test for namespace change navigation
it('should navigate to all-namespaces page when ALL_NAMESPACES_KEY is selected', () => { const mockNavigate = jest.fn(); (useNavigate as jest.Mock).mockReturnValue(mockNavigate); (useAccessReview as jest.Mock).mockReturnValue(true); (useParams as jest.Mock).mockReturnValue({ ns: 'test-project' }); // Update NamespacedPage mock to capture and invoke onNamespaceChange // Then verify: expect(mockNavigate).toHaveBeenCalledWith('/project-details/all-namespaces'); });This would require updating the
NamespacedPagemock to capture and invoke theonNamespaceChangecallback, but it would provide coverage for the migrated navigation logic.frontend/packages/dev-console/src/components/edit-application/EditApplication.tsx (1)
92-101: Consider returning thehandleRedirectpromise for proper error propagation.
handleRedirectis an async function (per the signature inimport-submit-utils.ts), but its promise isn't returned inside the.then()callback. This means ifhandleRedirectthrows (unlikely, but possible), the error won't propagate to your.catch()handler, and the promise chain will resolve before navigation completes.♻️ Suggested improvement
const handleSubmit = (values, actions) => { return updateResources(values) .then(() => { actions.setStatus({ submitError: '' }); - handleRedirect(namespace, perspective, perspectiveExtensions, navigate); + return handleRedirect(namespace, perspective, perspectiveExtensions, navigate); }) .catch((err) => { actions.setStatus({ submitError: err.message }); }); };frontend/packages/dev-console/src/components/import/ImportForm.tsx (2)
249-255: Consider awaitinghandleRedirectfor proper error propagation.The call signature looks correct and matches the updated
handleRedirectutility. However, sincehandleRedirectis an async function (perimport-submit-utils.ts:1003), calling it withoutawaitmeans any rejection from the redirect logic won't be caught by the.catch()block at line 257. This could result in an unhandled promise rejection if, for example,perspectiveData.properties.importRedirectURL()fails.🔧 Proposed fix to properly await handleRedirect
- handleRedirect( + await handleRedirect( projectName, perspective, perspectiveExtensions, navigate, redirectSearchParams, );
162-262: Optional: Consider memoizinghandleSubmitfor consistency.While Formik handles
onSubmitchanges gracefully, for consistency with thehandleCancelmemoization pattern you could wraphandleSubmitinuseCallback. Given the substantial dependency list (builders, projects, perspective, toast context, etc.), this is entirely optional and may not provide meaningful performance benefits for this form.frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (1)
278-301: Consider usingNavigateFunctiontype fromreact-router-dom-v5-compatfor better type fidelity.The inline type annotation
(to: string, options?: { replace?: boolean }) => voidworks for this use case but is a narrower subset of React Router's actualNavigateFunction. Using the library type directly improves IDE autocompletion, ensures compatibility with future navigate overloads, and keeps the codebase consistent with other migrated components.♻️ Proposed type improvement
+import type { NavigateFunction } from 'react-router-dom-v5-compat'; + -export const setURLParams = ( - params: URLSearchParams, - navigate: (to: string, options?: { replace?: boolean }) => void, -) => { +export const setURLParams = (params: URLSearchParams, navigate: NavigateFunction) => { const url = new URL(window.location.href); const searchParams = `?${params.toString()}${url.hash}`; navigate(`${url.pathname}${searchParams}`, { replace: true }); }; -export const updateURLParams = ( - paramName: string, - value: string | string[], - navigate: (to: string, options?: { replace?: boolean }) => void, -) => { +export const updateURLParams = ( + paramName: string, + value: string | string[], + navigate: NavigateFunction, +) => { const params = new URLSearchParams(window.location.search); // ... rest unchangedfrontend/packages/dev-console/src/utils/add-page-utils.ts (1)
39-42: Calle.preventDefault()beforenavigate(url)for defensive programming.Standard practice is to prevent the default browser behavior first, then perform the custom action. While the current order likely works in practice (React's synthetic event system batches these), reversing the order is more defensive and makes the intent clearer—especially if this handler is ever attached to an anchor element where the default action could theoretically fire before the navigation is scheduled.
♻️ Suggested reorder
export const navigateTo = (e: SyntheticEvent, url: string, navigate: NavigateFunction) => { + e.preventDefault(); navigate(url); - e.preventDefault(); };Additionally, consider whether a guard for falsy
urlvalues is warranted here, sinceresolvedHref(line 44-45) can returnnull. If callers pass the result directly without checking, you'd navigate to a null path. That said, if callers are expected to validate upstream, this may be fine as-is.frontend/packages/console-shared/src/components/quick-search/QuickSearchModalBody.tsx (1)
100-111:onEntercallback properly integrates navigation and query cleanup.The
handleCtacall signature matches the utility function definition (withcallbackPropsdefaulting to{}). Dependency array correctly includesnavigateandremoveQueryArgument.One minor observation: the
eparameter on Line 101 is untyped. Consider adding explicit typing for consistency with TypeScript best practices:- const onEnter = useCallback( - (e) => { + const onEnter = useCallback( + (e: KeyboardEvent) => {
| * @param {CronJobActionCreator[]} [filterActions] - Optional. If provided, the returned `actions` array will contain | ||
| * only the specified actions. If omitted, it will contain all CronJob actions. In case of invalid `actionCreators` | ||
| * returned `actions` are an empty array. |
There was a problem hiding this comment.
JSDoc contradicts implementation behavior for invalid creators.
The JSDoc states: "In case of invalid actionCreators returned actions are an empty array."
However, the implementation silently skips invalid creators and returns whatever valid ones were found:
return memoizedFilterActions.reduce<Action[]>((acc, creator) => {
const fn = factory[creator];
if (typeof fn === 'function') {
acc.push(fn());
}
return acc;
}, []);If filterActions = [CronJobActionCreator.StartJob, 'InvalidCreator' as any], this returns [StartJobAction], not [].
Either update the JSDoc to reflect the "skip invalid, return valid" behavior, or change the implementation to return [] when any creator is invalid (as suggested in a prior review). The current "skip invalid" approach is arguably more forgiving and practical—just align the docs.
📝 Proposed JSDoc fix (if keeping current behavior)
- * only the specified actions. If omitted, it will contain all CronJob actions. In case of invalid `actionCreators`
- * returned `actions` are an empty array.
+ * only the specified actions. If omitted, it will contain all CronJob actions. Invalid creators in the array
+ * are silently skipped; only valid creators produce actions.Also applies to: 110-122
🤖 Prompt for AI Agents
In `@frontend/packages/console-app/src/actions/hooks/useCronJobActions.ts` around
lines 60 - 62, The JSDoc for useCronJobActions.ts currently states that invalid
actionCreators produce an empty `actions` array, but the code (the reduce over
`memoizedFilterActions` using `factory` and checking `typeof fn === 'function'`)
actually skips invalid creators and returns the valid ones; update the JSDoc
comment above the function to state that invalid creators are ignored and the
returned `actions` array contains only the valid actions (e.g., reference
`memoizedFilterActions`, `factory`, and the reduce logic), or alternatively
change the implementation to instead detect any invalid creator and return an
empty array—pick one behavior and make the doc and code consistent across the
related block(s) around the reduce and lines 110-122.
| }} | ||
| > | ||
| {props.selectedItem.cta.label} | ||
| {selectedItem.cta.label} | ||
| </Button> |
There was a problem hiding this comment.
Potential null access on selectedItem.cta.
Per the CatalogItem type definition, cta is optional (cta?: { label: string; ... }). Accessing selectedItem.cta.label on Line 65 will throw a TypeError if cta is undefined.
Additionally, the handleCta utility (Lines 18-21 in quick-search-utils.tsx) destructures item.cta.href and item.cta.callback, which would also fail if cta is missing.
Consider adding a guard or conditionally rendering the CTA button:
🛡️ Proposed fix to guard against undefined cta
- <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>
+ >
+ {selectedItem.cta.label}
+ </Button>
+ )}🤖 Prompt for AI Agents
In
`@frontend/packages/console-shared/src/components/quick-search/QuickSearchDetails.tsx`
around lines 63 - 66, selectedItem.cta is optional on CatalogItem so accessing
selectedItem.cta.label and destructuring item.cta in handleCta can throw; update
QuickSearchDetails (where the Button renders selectedItem.cta.label) to only
render the CTA Button if selectedItem.cta exists (e.g., wrap the Button in a
guard like if (selectedItem.cta) or use optional chaining), and update handleCta
in quick-search-utils.tsx to avoid direct destructuring of item.cta (use
optional chaining or default to an empty object before reading href/callback) so
code safely handles missing cta.
There was a problem hiding this comment.
@rhamilto – Confirmed, we're already in PR #15959.
The null-safety concern is valid: cta is optional on CatalogItem, so both the button render (Line 65: selectedItem.cta.label) and the handleCta call can throw if cta is undefined.
Since you're already modifying this file for the navigation migration, would you like to add the guard now? The fix is straightforward—conditionally render the Button only when selectedItem.cta exists:
{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>
)}You'll also want to guard inside handleCta (quick-search-utils.tsx) before destructuring item.cta.href and item.cta.callback.
Alternatively, if you'd prefer to keep this PR focused on the navigation migration, I can open a follow-up issue to track the defensive null-check separately. Let me know your preference.
| const renderResult = renderWithProviders(<BuildConfigFormPage />); | ||
| renderResult.findByText('Error Loading'); | ||
| renderResult.findByText('Edit BuildConfig'); | ||
| renderResult.findByText('Something went wrong'); |
There was a problem hiding this comment.
Same findBy* without await issue here.
Lines 117-119 have the same problem—unawaited findBy* calls that won't actually assert anything meaningful. Apply the same fix as above.
Proposed fix
it('should render an error when the BuildConfig fetching fails', () => {
// ... setup ...
const renderResult = renderWithProviders(<BuildConfigFormPage />);
- renderResult.findByText('Error Loading');
- renderResult.findByText('Edit BuildConfig');
- renderResult.findByText('Something went wrong');
+ renderResult.getByText('Error Loading');
+ renderResult.getByText('Edit BuildConfig');
+ renderResult.getByText('Something went wrong');
});🤖 Prompt for AI Agents
In
`@frontend/packages/dev-console/src/components/buildconfig/__tests__/BuildConfigFormPage.spec.tsx`
around lines 116 - 119, The test calls renderWithProviders(<BuildConfigFormPage
/>) and then uses renderResult.findByText('Error Loading'),
renderResult.findByText('Edit BuildConfig'), and
renderResult.findByText('Something went wrong') without awaiting them, so they
don't actually assert; update the assertions in BuildConfigFormPage.spec.tsx to
await each call to renderResult.findByText(...) (or replace with synchronous
queries like getByText if appropriate) so the promises resolve and the test
reliably verifies the presence of those texts; target the
renderResult.findByText calls related to BuildConfigFormPage and keep the same
expected strings.
| export const jarFileUploadHandler = (file: File, namespace: string, navigate: NavigateFunction) => { | ||
| navigate(`/upload-jar/ns/${namespace}`); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of jarFileUploadHandler to understand the workflow
rg -n -C5 'jarFileUploadHandler' --type=ts --type=tsx -g '!*.spec.*' -g '!*.test.*'Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Find all usages of jarFileUploadHandler - simplified search
rg -n -C5 'jarFileUploadHandler' --type=ts -g '!*.spec.*' -g '!*.test.*'Repository: openshift/console
Length of output: 711
🏁 Script executed:
#!/bin/bash
# Search for imports and usage of jarFileUploadHandler
rg -n 'jarFileUploadHandler' --type=ts -g '!*.spec.*' -g '!*.test.*' -C3Repository: openshift/console
Length of output: 711
🏁 Script executed:
#!/bin/bash
# List files in the jar-file-upload directory
find frontend/packages/dev-console/src/components/jar-file-upload -type f -name '*.ts' -o -name '*.tsx' -o -name '*.js' -o -name '*.jsx' | head -20
echo "---"
# Search for jarFileUploadHandler without type restrictions
rg 'jarFileUploadHandler' frontend/packages/dev-console/src/components/jar-file-upload -n -C5Repository: openshift/console
Length of output: 801
🏁 Script executed:
#!/bin/bash
# Broader search without type or location restrictions
rg 'jarFileUploadHandler' -n --max-count=50Repository: openshift/console
Length of output: 374
🏁 Script executed:
#!/bin/bash
# Look at the console-extensions.json around line 574
sed -n '560,590p' frontend/packages/dev-console/console-extensions.json | cat -nRepository: openshift/console
Length of output: 1064
🏁 Script executed:
#!/bin/bash
# Search for console.file-upload references and documentation
rg 'console\.file-upload' -n -C3
echo "---"
# Also search for other file-upload handlers that might show the pattern
rg 'fileUpload' frontend/packages/dev-console/src -n -C2 | head -50Repository: openshift/console
Length of output: 9962
🏁 Script executed:
#!/bin/bash
# Read the file-upload extension definition
cat -n frontend/packages/console-dynamic-plugin-sdk/src/extensions/file-upload.tsRepository: openshift/console
Length of output: 800
🏁 Script executed:
#!/bin/bash
# Search for where file-upload handlers are invoked
rg 'FileUploadHandler|isFileUpload' frontend/packages/console-app/src/components/file-upload -n -C5 | head -100Repository: openshift/console
Length of output: 2798
🏁 Script executed:
#!/bin/bash
# Read more of file-upload-context.ts to see how handlers are invoked
cat -n frontend/packages/console-app/src/components/file-upload/file-upload-context.tsRepository: openshift/console
Length of output: 3130
Remove navigate parameter and implement navigation using a hook, or refactor handler contract.
This handler has a critical type mismatch. The extension system invokes file-upload handlers with exactly two arguments: (file: File, namespace: string). However, jarFileUploadHandler declares three parameters including navigate: NavigateFunction.
When the extension framework calls requiredFileExtension.properties.handler(f, namespace) (line 48 in file-upload-context.ts), the navigate parameter will be undefined, causing the function to crash when attempting to invoke it on line 4.
Additionally, the file parameter is unused. Either:
- Remove
navigateand obtain it via a hook if this becomes a component (but util functions shouldn't use hooks) - Restructure the handler to work within the extension's contract, deferring navigation/file handling to the calling component
- Pass the file via navigation state instead of receiving it as a parameter
🤖 Prompt for AI Agents
In
`@frontend/packages/dev-console/src/components/jar-file-upload/jar-file-upload-utils.ts`
around lines 3 - 5, The handler's signature is wrong for the extension system:
change jarFileUploadHandler to remove the third parameter (navigate:
NavigateFunction) and instead return the navigation path as a string (e.g.
`/upload-jar/ns/${namespace}`) so callers can invoke navigate themselves; update
jarFileUploadHandler (and any direct imports) to export a function
jarFileUploadHandler(file: File, namespace: string): string and ensure the
extension caller (e.g. requiredFileExtension.properties.handler in
file-upload-context.ts) uses the returned string with navigate, or otherwise
perform navigation in the calling component.
| try { | ||
| const redirectURL = (await perspectiveData.properties.importRedirectURL())(project); | ||
| navigate(redirectURL); | ||
| } catch (error) { |
There was a problem hiding this comment.
Guard against empty redirect URL before navigating.
importRedirectURL() can legitimately return undefined/'' for some perspectives; calling navigate with a falsy value can throw or route unexpectedly. Add a short guard and log.
🩹 Suggested fix
- const redirectURL = (await perspectiveData.properties.importRedirectURL())(project);
- navigate(redirectURL);
+ const redirectURL = (await perspectiveData.properties.importRedirectURL())(project);
+ if (!redirectURL) {
+ // eslint-disable-next-line no-console
+ console.warn(`Redirect URL not provided for perspective ${perspective}`);
+ return;
+ }
+ navigate(redirectURL);📝 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.
| try { | |
| const redirectURL = (await perspectiveData.properties.importRedirectURL())(project); | |
| navigate(redirectURL); | |
| } catch (error) { | |
| try { | |
| const redirectURL = (await perspectiveData.properties.importRedirectURL())(project); | |
| if (!redirectURL) { | |
| // eslint-disable-next-line no-console | |
| console.warn(`Redirect URL not provided for perspective ${perspective}`); | |
| return; | |
| } | |
| navigate(redirectURL); | |
| } catch (error) { |
🤖 Prompt for AI Agents
In `@frontend/packages/knative-plugin/src/utils/create-eventsources-utils.ts`
around lines 361 - 364, The code calls
perspectiveData.properties.importRedirectURL() and immediately passes its result
(redirectURL) to navigate, which can throw if redirectURL is undefined or empty;
before calling navigate(redirectURL) in the try block, add a guard that checks
if redirectURL is a non-empty string (e.g., if (!redirectURL) {
processLogger?.warn(...) or console.warn(...) and return/skip navigation }), and
only call navigate when the guard passes so you avoid routing with falsy values
and log the skipped case for debugging.
98e7956 to
f6ebc6e
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-programmatic-navigation
branch
2 times, most recently
from
46378ec to
108ad4b
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. |
openshift-ci
bot
added
component/sdk
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>
Replace history object usage with useNavigate hook in console-app package. Changes: - cronjob-factory.ts: Converted to dependency injection pattern - cronjob-provider.ts: Use navigate in action creator - ClusterConfigurationPage.tsx: Replace history.push with navigate - Lightspeed.tsx: Replace history.push with navigate - clone-pvc-modal.tsx: Replace history.push with navigate - restore-pvc-modal.tsx: Replace history.push with navigate - PDBForm.tsx: Replace history.push with navigate - UserPreferencePage.tsx: Replace history.push with navigate - create-volume-snapshot.tsx: Replace history.push with navigate All changes: - Replaced history.push(path) with navigate(path) - Added useNavigate() hook calls - Updated imports to use react-router-dom-v5-compat Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace history object usage with useNavigate hook in console-shared package. Changes: - ActionMenuItem.tsx: Replace history.push with navigate - CatalogTile.tsx: Replace history.push with navigate - CatalogView.tsx: Replace history.push with navigate - catalog-utils.tsx: Accept NavigateFunction as parameter - dynamic-form/index.tsx: Replace history.goBack with navigate(-1) - error-boundary.tsx: Use key prop pattern for location reset - DeleteResourceModal.tsx: Replace history.push with navigate - MultiTabListPage.tsx: Replace history.push with navigate Migration patterns: - Components: Added useNavigate() hook - Utilities: Parameter injection for NavigateFunction - Class components: Key prop for location-based reset Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace history object usage with useNavigate hook in dev-console package. Changes: - AddCardItem.tsx: Pass navigate to navigateTo utility - EditBuildConfig.tsx: Replace history.push with navigate - EditDeployment.tsx: Replace history.push with navigate - EditApplication.tsx: Replace history.goBack with navigate(-1) - AddHealthChecks.tsx: Replace history.push with navigate - AddHealthChecksForm.tsx: Replace history.goBack with navigate(-1) - HPAFormikForm.tsx: Replace history.goBack with navigate(-1) - DeployImage.tsx: Pass navigate to handleRedirect - ImportForm.tsx: Pass navigate to handleRedirect - ImportSamplePage.tsx: Pass navigate to handleRedirect - import-submit-utils.ts: Accept NavigateFunction parameter - UploadJar.tsx: Replace history.goBack with navigate(-1) - useUploadJarFormToast.ts: Accept navigate as parameter - AddServerlessFunction.tsx: Replace history.goBack with navigate(-1) - jar-file-upload-utils.ts: Accept navigate as parameter - MonitoringPage.tsx: Replace history.push with navigate - ProjectDetailsPage.tsx: Replace history.push with navigate - add-page-utils.ts: Accept navigate as parameter All utility functions updated to use dependency injection pattern. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace history object usage with useNavigate hook in knative-plugin package. Changes: - EventSink.tsx: Replace history.goBack with navigate(-1) - EventSource.tsx: Replace history.goBack with navigate(-1) - AddBroker.tsx: Replace history.goBack with navigate(-1) - AddChannel.tsx: Replace history.goBack with navigate(-1) - Subscribe.tsx: Replace history.push with navigate - FunctionDetailsPage.tsx: Replace history.push with navigate - CreateKnatifyPage.tsx: Replace history.goBack with navigate(-1) - create-eventsources-utils.ts: Accept NavigateFunction parameter All components updated to use useNavigate() hook. Utility function updated to use dependency injection pattern. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace history object usage with useNavigate hook in helm-plugin package. Changes: - HelmReleaseDetailsPage.tsx: Replace history.push with navigate - CreateHelmChartRepository.tsx: Replace history.goBack with navigate(-1) - CreateHelmChartRepositoryPage.tsx: Replace history.push with navigate - HelmInstallUpgradePage.tsx: Replace history.push/goBack with navigate - HelmReleaseRollbackPage.tsx: Replace history.push/goBack with navigate All components updated to use useNavigate() hook from react-router-dom-v5-compat. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace history object usage with useNavigate hook in remaining packages. Changes: - operator-lifecycle-manager/create-catalog-source.tsx: Replace history.goBack with navigate(-1) - operator-lifecycle-manager/operator-hub-items.tsx: Replace history.replace with navigate - topology/ExportViewLogButton.tsx: Replace history.push with navigate - shipwright-plugin/EditBuild.tsx: Replace history.push/goBack with navigate - metal3-plugin/AddBareMetalHostForm.tsx: Replace history.goBack with navigate(-1) - public/QuickCreate.tsx: Replace history.push with navigate - console-app/useCronJobActions.ts: Update JSDoc to correctly describe that invalid action creators are ignored (not that an empty array is returned) All components updated to use useNavigate() hook from react-router-dom-v5-compat. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4990-programmatic-navigation
branch
from
108ad4b to
c216ba1
Compare
|
/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. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
7 participants
Summary
Migrates the Console frontend from using the legacy
historyobject API to React Router v6/v7'suseNavigatehook. This is part of the ongoing effort to modernize the Console's routing implementation.Changes
This PR systematically migrates the codebase across multiple packages:
Key Pattern Changes
history.push(path)→navigate(path)history.goBack()→navigate(-1)history.replace(path)→navigate(path, { replace: true })Bug Fixes & Improvements
This PR includes several critical fixes and improvements discovered during the migration:
FileUploadHandlertype to returnstring | voidinstead ofvoid, allowing handlers to optionally return a navigation pathasync/awaitTest Updates
Updated test files to properly await async operations and use modern testing patterns:
Test Plan
Related Issues
🤖 Generated with Claude Code