From bc1050ee6a5195859147754657b51d22c560a10e Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Fri, 6 Dec 2024 13:59:29 -0600 Subject: [PATCH 1/8] Add external routes to AppRoutes component --- frontend/src/app/AppRoutes.tsx | 3 + .../src/pages/external/ExternalRoutes.tsx | 12 ++ .../PipelinesSdkRedirects.tsx | 51 ++++++++ .../utilities/__tests__/useRedirect.spec.ts | 113 ++++++++++++++++++ frontend/src/utilities/useRedirect.ts | 59 +++++++++ 5 files changed, 238 insertions(+) create mode 100644 frontend/src/pages/external/ExternalRoutes.tsx create mode 100644 frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx create mode 100644 frontend/src/utilities/__tests__/useRedirect.spec.ts create mode 100644 frontend/src/utilities/useRedirect.ts diff --git a/frontend/src/app/AppRoutes.tsx b/frontend/src/app/AppRoutes.tsx index 1749385da5..a20b057f1d 100644 --- a/frontend/src/app/AppRoutes.tsx +++ b/frontend/src/app/AppRoutes.tsx @@ -76,6 +76,8 @@ const StorageClassesPage = React.lazy(() => import('../pages/storageClasses/Stor const ModelRegistryRoutes = React.lazy(() => import('../pages/modelRegistry/ModelRegistryRoutes')); +const ExternalRoutes = React.lazy(() => import('../pages/external/ExternalRoutes')); + const AppRoutes: React.FC = () => { const { isAdmin, isAllowed } = useUser(); const isJupyterEnabled = useCheckJupyterEnabled(); @@ -94,6 +96,7 @@ const AppRoutes: React.FC = () => { }> + } /> {isHomeAvailable ? ( <> } /> diff --git a/frontend/src/pages/external/ExternalRoutes.tsx b/frontend/src/pages/external/ExternalRoutes.tsx new file mode 100644 index 0000000000..6527c6a257 --- /dev/null +++ b/frontend/src/pages/external/ExternalRoutes.tsx @@ -0,0 +1,12 @@ +import React from 'react'; +import { Navigate, Route, Routes } from 'react-router-dom'; +import PipelinesSdkRedirect from './redirectComponents/PipelinesSdkRedirects'; + +const ExternalRoutes: React.FC = () => ( + + } /> + } /> + +); + +export default ExternalRoutes; diff --git a/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx b/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx new file mode 100644 index 0000000000..d2fe882904 --- /dev/null +++ b/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx @@ -0,0 +1,51 @@ +import React from 'react'; +import { useParams, useLocation } from 'react-router-dom'; +import { experimentRunsRoute, globalPipelineRunDetailsRoute } from '~/routes'; +import ApplicationsPage from '~/pages/ApplicationsPage'; +import { useRedirect } from '~/utilities/useRedirect'; + +/** + * Handles redirects from Pipeline SDK URLs to internal routes. + * + * Matches and redirects: + * - Experiment URL: /external/pipelinesSdk/{namespace}/#/experiments/details/{experimentId} + * - Run URL: /external/pipelinesSdk/{namespace}/#/runs/details/{runId} + */ +const PipelinesSdkRedirects: React.FC = () => { + const { namespace } = useParams<{ namespace: string }>(); + const location = useLocation(); + + const createRedirectPath = React.useCallback(() => { + if (!namespace) { + throw new Error('Missing namespace parameter'); + } + + // Extract experimentId from hash + const experimentMatch = location.hash.match(/\/experiments\/details\/([^/]+)$/); + if (experimentMatch) { + const experimentId = experimentMatch[1]; + return experimentRunsRoute(namespace, experimentId); + } + + // Extract runId from hash + const runMatch = location.hash.match(/\/runs\/details\/([^/]+)$/); + if (runMatch) { + const runId = runMatch[1]; + return globalPipelineRunDetailsRoute(namespace, runId); + } + + throw new Error('Invalid URL format'); + }, [namespace, location.hash]); + + const [redirect, { loaded }] = useRedirect(createRedirectPath); + + React.useEffect(() => { + redirect(); + }, [redirect]); + + return ( + + ); +}; + +export default PipelinesSdkRedirects; diff --git a/frontend/src/utilities/__tests__/useRedirect.spec.ts b/frontend/src/utilities/__tests__/useRedirect.spec.ts new file mode 100644 index 0000000000..da9ff26484 --- /dev/null +++ b/frontend/src/utilities/__tests__/useRedirect.spec.ts @@ -0,0 +1,113 @@ +import { testHook } from '~/__tests__/unit/testUtils/hooks'; +import { useRedirect } from '~/utilities/useRedirect'; + +const mockNavigate = jest.fn(); +jest.mock('react-router-dom', () => ({ + useNavigate: () => mockNavigate, +})); + +describe('useRedirect', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should handle successful redirect', async () => { + const createRedirectPath = jest.fn().mockReturnValue('/success-path'); + const onComplete = jest.fn(); + const renderResult = testHook(useRedirect)(createRedirectPath, { onComplete }); + + const [redirect, state] = renderResult.result.current; + expect(state.loaded).toBe(false); + expect(state.error).toBeUndefined(); + + await redirect(); + renderResult.rerender(createRedirectPath, { onComplete }); + + expect(createRedirectPath).toHaveBeenCalled(); + expect(mockNavigate).toHaveBeenCalledWith('/success-path', undefined); + expect(onComplete).toHaveBeenCalled(); + expect(renderResult.result.current[1].loaded).toBe(true); + expect(renderResult.result.current[1].error).toBeUndefined(); + }); + + it('should handle async redirect path creation', async () => { + const createRedirectPath = jest.fn().mockResolvedValue('/async-path'); + const renderResult = testHook(useRedirect)(createRedirectPath); + const [redirect] = renderResult.result.current; + + await redirect(); + renderResult.rerender(createRedirectPath); + + expect(createRedirectPath).toHaveBeenCalled(); + expect(mockNavigate).toHaveBeenCalledWith('/async-path', undefined); + expect(renderResult.result.current[1].loaded).toBe(true); + expect(renderResult.result.current[1].error).toBeUndefined(); + }); + + it('should handle redirect with navigation options', async () => { + const createRedirectPath = jest.fn().mockReturnValue('/path'); + const navigateOptions = { replace: true }; + const renderResult = testHook(useRedirect)(createRedirectPath, { navigateOptions }); + const [redirect] = renderResult.result.current; + + await redirect(); + renderResult.rerender(createRedirectPath, { navigateOptions }); + + expect(mockNavigate).toHaveBeenCalledWith('/path', navigateOptions); + }); + + it('should handle error when path is undefined', async () => { + const createRedirectPath = jest.fn().mockReturnValue(undefined); + const onError = jest.fn(); + const renderResult = testHook(useRedirect)(createRedirectPath, { onError }); + + const [redirect] = renderResult.result.current; + + await redirect(); + renderResult.rerender(createRedirectPath, { onError }); + + expect(onError).toHaveBeenCalledWith(expect.any(Error)); + expect(mockNavigate).toHaveBeenCalledWith('/not-found', { replace: true }); + expect(renderResult.result.current[1].loaded).toBe(true); + expect(renderResult.result.current[1].error).toBeInstanceOf(Error); + }); + + it('should handle error in path creation', async () => { + const error = new Error('Failed to create path'); + const createRedirectPath = jest.fn().mockRejectedValue(error); + const onError = jest.fn(); + const renderResult = testHook(useRedirect)(createRedirectPath, { onError }); + + const [redirect] = renderResult.result.current; + + await redirect(); + renderResult.rerender(createRedirectPath, { onError }); + + expect(onError).toHaveBeenCalledWith(error); + expect(mockNavigate).toHaveBeenCalledWith('/not-found', { replace: true }); + expect(renderResult.result.current[1].loaded).toBe(true); + expect(renderResult.result.current[1].error).toBe(error); + }); + + it('should not redirect to not-found when notFoundOnError is false', async () => { + const createRedirectPath = jest.fn().mockRejectedValue(new Error()); + const renderResult = testHook(useRedirect)(createRedirectPath); + + const [redirect] = renderResult.result.current; + + await redirect(false); + renderResult.rerender(createRedirectPath); + + expect(mockNavigate).not.toHaveBeenCalled(); + expect(renderResult.result.current[1].loaded).toBe(true); + expect(renderResult.result.current[1].error).toBeInstanceOf(Error); + }); + + it('should be stable', () => { + const createRedirectPath = jest.fn().mockReturnValue('/path'); + const renderResult = testHook(useRedirect)(createRedirectPath); + renderResult.rerender(createRedirectPath); + expect(renderResult).hookToBeStable([true, true]); + expect(renderResult).hookToHaveUpdateCount(2); + }); +}); diff --git a/frontend/src/utilities/useRedirect.ts b/frontend/src/utilities/useRedirect.ts new file mode 100644 index 0000000000..9579a7eb7f --- /dev/null +++ b/frontend/src/utilities/useRedirect.ts @@ -0,0 +1,59 @@ +import { NavigateOptions, useNavigate } from 'react-router-dom'; +import React from 'react'; + +export type RedirectState = { + loaded: boolean; + error?: Error; +}; + +export type RedirectOptions = { + /** Additional options for the navigate function */ + navigateOptions?: NavigateOptions; + /** Callback when redirect is complete */ + onComplete?: () => void; + /** Callback when redirect fails */ + onError?: (error: Error) => void; +}; + +/** + * Hook for managing redirects with loading states + * @param createRedirectPath Function that creates the redirect path, can be async for data fetching + * @param options Redirect options + * @returns Array of [redirect function, redirect state] + */ +export const useRedirect = ( + createRedirectPath: () => string | Promise | undefined, + options: RedirectOptions = {}, +): [(notFoundOnError?: boolean) => Promise, RedirectState] => { + const { navigateOptions, onComplete, onError } = options; + + const navigate = useNavigate(); + const [state, setState] = React.useState({ + loaded: false, + error: undefined, + }); + + const redirect = React.useCallback( + async (notFoundOnError = true) => { + try { + const path = await createRedirectPath(); + if (!path) { + throw new Error('No redirect path available'); + } + navigate(path, navigateOptions); + setState({ loaded: true, error: undefined }); + onComplete?.(); + } catch (e) { + const error = e instanceof Error ? e : new Error('Failed to redirect'); + setState({ loaded: true, error }); + onError?.(error); + if (notFoundOnError) { + navigate('/not-found', { replace: true }); + } + } + }, + [createRedirectPath, navigate, navigateOptions, onComplete, onError], + ); + + return [redirect, state]; +}; From 89c8d870cf57f97cd9b27f4f58ff09c64e4a7460 Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Wed, 11 Dec 2024 15:37:57 -0600 Subject: [PATCH 2/8] Refactor external routing and error handling components - Updated ExternalRoutes to use ExternalRedirectNotFound instead of Navigate for unmatched routes. - Introduced RedirectErrorState component to handle redirect errors with customizable actions and fallback options. - Enhanced PipelinesSdkRedirects to display RedirectErrorState on error, providing navigation options to users. - Added ExternalRedirectNotFound component to show a user-friendly message for non-existent external redirects. This improves user experience by providing clearer error states and navigation options during redirects. --- .../src/pages/external/ExternalRoutes.tsx | 5 +- .../src/pages/external/RedirectErrorState.tsx | 106 ++++++++++++++++++ .../ExternalRedirectNotFound.tsx | 20 ++++ .../PipelinesSdkRedirects.tsx | 28 ++++- frontend/src/utilities/useRedirect.ts | 70 ++++++++---- 5 files changed, 204 insertions(+), 25 deletions(-) create mode 100644 frontend/src/pages/external/RedirectErrorState.tsx create mode 100644 frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx diff --git a/frontend/src/pages/external/ExternalRoutes.tsx b/frontend/src/pages/external/ExternalRoutes.tsx index 6527c6a257..8e08cc5d63 100644 --- a/frontend/src/pages/external/ExternalRoutes.tsx +++ b/frontend/src/pages/external/ExternalRoutes.tsx @@ -1,11 +1,12 @@ import React from 'react'; -import { Navigate, Route, Routes } from 'react-router-dom'; +import { Route, Routes } from 'react-router-dom'; import PipelinesSdkRedirect from './redirectComponents/PipelinesSdkRedirects'; +import ExternalRedirectNotFound from './redirectComponents/ExternalRedirectNotFound'; const ExternalRoutes: React.FC = () => ( } /> - } /> + } /> ); diff --git a/frontend/src/pages/external/RedirectErrorState.tsx b/frontend/src/pages/external/RedirectErrorState.tsx new file mode 100644 index 0000000000..de5f10b1a2 --- /dev/null +++ b/frontend/src/pages/external/RedirectErrorState.tsx @@ -0,0 +1,106 @@ +import { + PageSection, + EmptyState, + EmptyStateVariant, + EmptyStateBody, + EmptyStateFooter, + EmptyStateActions, + Button, +} from '@patternfly/react-core'; +import { ExclamationCircleIcon } from '@patternfly/react-icons'; +import React from 'react'; +import { useNavigate } from 'react-router-dom'; +import { EitherOrNone } from '~/typeHelpers'; + +type RedirectErrorStateProps = { + title?: string; + errorMessage?: string; +} & EitherOrNone< + { + fallbackUrl: string; + fallbackText: string; + }, + { + actions: React.ReactNode | React.ReactNode[]; + } +>; + +/** + * A component that displays an error state with optional title, message and actions + * Used for showing redirect/navigation errors with fallback options + * + * Props for the RedirectErrorState component + * @property {string} [title] - Optional title text to display in the error state + * @property {string} [errorMessage] - Optional error message to display + * @property {string} [fallbackUrl] - URL to navigate to when fallback button is clicked + * @property {React.ReactNode | React.ReactNode[]} [actions] - Custom action buttons/elements to display + * + * Note: The component accepts either fallbackUrl OR actions prop, but not both. + * This is enforced by the EitherOrNone type helper. + * + * @example + * ```tsx + * // With fallback URL + * + * + * // With custom actions + * + * + * + * + * } + * /> + * ``` + */ + +const RedirectErrorState: React.FC = ({ + title, + errorMessage, + fallbackUrl, + fallbackText, + actions, +}) => { + const navigate = useNavigate(); + + return ( + + + {errorMessage && {errorMessage}} + {fallbackUrl && ( + + + + + + )} + {actions && ( + + {actions} + + )} + + + ); +}; + +export default RedirectErrorState; diff --git a/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx b/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx new file mode 100644 index 0000000000..fb8840dfd4 --- /dev/null +++ b/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx @@ -0,0 +1,20 @@ +import React from 'react'; +import ApplicationsPage from '~/pages/ApplicationsPage'; +import RedirectErrorState from '~/pages/external/RedirectErrorState'; + +const ExternalRedirectNotFound: React.FC = () => ( + + } + /> +); + +export default ExternalRedirectNotFound; diff --git a/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx b/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx index d2fe882904..aa5fa02b03 100644 --- a/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx +++ b/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx @@ -1,8 +1,10 @@ import React from 'react'; -import { useParams, useLocation } from 'react-router-dom'; +import { useParams, useLocation, useNavigate } from 'react-router-dom'; +import { Button } from '@patternfly/react-core'; import { experimentRunsRoute, globalPipelineRunDetailsRoute } from '~/routes'; import ApplicationsPage from '~/pages/ApplicationsPage'; import { useRedirect } from '~/utilities/useRedirect'; +import RedirectErrorState from '~/pages/external/RedirectErrorState'; /** * Handles redirects from Pipeline SDK URLs to internal routes. @@ -14,6 +16,7 @@ import { useRedirect } from '~/utilities/useRedirect'; const PipelinesSdkRedirects: React.FC = () => { const { namespace } = useParams<{ namespace: string }>(); const location = useLocation(); + const navigate = useNavigate(); const createRedirectPath = React.useCallback(() => { if (!namespace) { @@ -37,14 +40,33 @@ const PipelinesSdkRedirects: React.FC = () => { throw new Error('Invalid URL format'); }, [namespace, location.hash]); - const [redirect, { loaded }] = useRedirect(createRedirectPath); + const [redirect, { loaded, error }] = useRedirect(createRedirectPath); React.useEffect(() => { redirect(); }, [redirect]); return ( - + + + + + } + /> + } + /> ); }; diff --git a/frontend/src/utilities/useRedirect.ts b/frontend/src/utilities/useRedirect.ts index 9579a7eb7f..c180b646dd 100644 --- a/frontend/src/utilities/useRedirect.ts +++ b/frontend/src/utilities/useRedirect.ts @@ -20,6 +20,42 @@ export type RedirectOptions = { * @param createRedirectPath Function that creates the redirect path, can be async for data fetching * @param options Redirect options * @returns Array of [redirect function, redirect state] + * + * @example + * ```tsx + * const [redirect, state] = useRedirect(() => '/foo'); + * + * // With async path creation + * const [redirect, state] = useRedirect(async () => { + * const data = await fetchData(); + * return `/bar/${data.id}`; + * }); + * + * // With options + * const [redirect, state] = useRedirect(() => '/foobar', { + * navigateOptions: { replace: true }, + * onComplete: () => console.log('Redirected'), + * onError: (error) => console.error(error) + * }); + * + * // Usage + * const createRedirectPath = React.useCallback(() => '/some/path', []); + * + * const [redirect, { loaded, error }] = useRedirect(createRedirectPath); + * + * React.useEffect(() => { + * redirect(); + * }, [redirect]); + * + * + * return ( + * } + * /> + * ); + * ``` */ export const useRedirect = ( createRedirectPath: () => string | Promise | undefined, @@ -33,27 +69,21 @@ export const useRedirect = ( error: undefined, }); - const redirect = React.useCallback( - async (notFoundOnError = true) => { - try { - const path = await createRedirectPath(); - if (!path) { - throw new Error('No redirect path available'); - } - navigate(path, navigateOptions); - setState({ loaded: true, error: undefined }); - onComplete?.(); - } catch (e) { - const error = e instanceof Error ? e : new Error('Failed to redirect'); - setState({ loaded: true, error }); - onError?.(error); - if (notFoundOnError) { - navigate('/not-found', { replace: true }); - } + const redirect = React.useCallback(async () => { + try { + const path = await createRedirectPath(); + if (!path) { + throw new Error('No redirect path available'); } - }, - [createRedirectPath, navigate, navigateOptions, onComplete, onError], - ); + navigate(path, navigateOptions); + setState({ loaded: true, error: undefined }); + onComplete?.(); + } catch (e) { + const error = e instanceof Error ? e : new Error('Failed to redirect'); + setState({ loaded: true, error }); + onError?.(error); + } + }, [createRedirectPath, navigate, navigateOptions, onComplete, onError]); return [redirect, state]; }; From 92e5635a7fd5cca781a9d60ff68f75c29af48b3e Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Wed, 11 Dec 2024 16:00:27 -0600 Subject: [PATCH 3/8] fix tests --- frontend/src/utilities/__tests__/useRedirect.spec.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/frontend/src/utilities/__tests__/useRedirect.spec.ts b/frontend/src/utilities/__tests__/useRedirect.spec.ts index da9ff26484..895aa4dffd 100644 --- a/frontend/src/utilities/__tests__/useRedirect.spec.ts +++ b/frontend/src/utilities/__tests__/useRedirect.spec.ts @@ -67,7 +67,6 @@ describe('useRedirect', () => { renderResult.rerender(createRedirectPath, { onError }); expect(onError).toHaveBeenCalledWith(expect.any(Error)); - expect(mockNavigate).toHaveBeenCalledWith('/not-found', { replace: true }); expect(renderResult.result.current[1].loaded).toBe(true); expect(renderResult.result.current[1].error).toBeInstanceOf(Error); }); @@ -84,7 +83,6 @@ describe('useRedirect', () => { renderResult.rerender(createRedirectPath, { onError }); expect(onError).toHaveBeenCalledWith(error); - expect(mockNavigate).toHaveBeenCalledWith('/not-found', { replace: true }); expect(renderResult.result.current[1].loaded).toBe(true); expect(renderResult.result.current[1].error).toBe(error); }); From 459f214cc1abd90304a3f52a0fd72427a3af15d8 Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Thu, 12 Dec 2024 12:25:19 -0600 Subject: [PATCH 4/8] added cypress tests and microcopy --- .../cypress/cypress/pages/externalRedirect.ts | 32 +++++++++++++++ .../tests/mocked/externalRedirects.cy.ts | 41 +++++++++++++++++++ .../src/pages/external/RedirectErrorState.tsx | 2 +- .../ExternalRedirectNotFound.tsx | 6 +-- .../PipelinesSdkRedirects.tsx | 6 +-- 5 files changed, 80 insertions(+), 7 deletions(-) create mode 100644 frontend/src/__tests__/cypress/cypress/pages/externalRedirect.ts create mode 100644 frontend/src/__tests__/cypress/cypress/tests/mocked/externalRedirects.cy.ts diff --git a/frontend/src/__tests__/cypress/cypress/pages/externalRedirect.ts b/frontend/src/__tests__/cypress/cypress/pages/externalRedirect.ts new file mode 100644 index 0000000000..2c8b8c9a54 --- /dev/null +++ b/frontend/src/__tests__/cypress/cypress/pages/externalRedirect.ts @@ -0,0 +1,32 @@ +class ExternalRedirect { + visit(path: string) { + cy.visitWithLogin(path); + this.wait(); + } + + private wait() { + cy.findByTestId('redirect-error').should('not.exist'); + cy.testA11y(); + } + + findErrorState() { + return cy.findByTestId('redirect-error'); + } + + findHomeButton() { + return cy.findByRole('button', { name: 'Go to Home' }); + } +} + +class PipelinesSdkRedirect { + findPipelinesButton() { + return cy.findByRole('button', { name: 'Go to Pipelines' }); + } + + findExperimentsButton() { + return cy.findByRole('button', { name: 'Go to Experiments' }); + } +} + +export const externalRedirect = new ExternalRedirect(); +export const pipelinesSdkRedirect = new PipelinesSdkRedirect(); diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/externalRedirects.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/externalRedirects.cy.ts new file mode 100644 index 0000000000..3bba564f64 --- /dev/null +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/externalRedirects.cy.ts @@ -0,0 +1,41 @@ +import { + externalRedirect, + pipelinesSdkRedirect, +} from '~/__tests__/cypress/cypress/pages/externalRedirect'; + +describe('External Redirects', () => { + describe('Pipeline SDK Redirects', () => { + it('should redirect experiment URLs correctly', () => { + // Test experiment URL redirect + externalRedirect.visit('/external/pipelinesSdk/test-namespace/#/experiments/details/123'); + cy.url().should('include', '/experiments/test-namespace/123/runs'); + }); + + it('should redirect run URLs correctly', () => { + // Test run URL redirect + externalRedirect.visit('/external/pipelinesSdk/test-namespace/#/runs/details/456'); + cy.url().should('include', '/pipelineRuns/test-namespace/runs/456'); + }); + + it('should handle invalid URL format', () => { + externalRedirect.visit('/external/pipelinesSdk/test-namespace/#/invalid'); + externalRedirect.findErrorState().should('exist'); + pipelinesSdkRedirect.findPipelinesButton().should('exist'); + pipelinesSdkRedirect.findExperimentsButton().should('exist'); + }); + }); + + describe('External Redirect Not Found', () => { + it('should show not found page for invalid external routes', () => { + externalRedirect.visit('/external/invalid-path'); + externalRedirect.findErrorState().should('exist'); + externalRedirect.findHomeButton().should('exist'); + }); + + it('should allow navigation back to home', () => { + externalRedirect.visit('/external/invalid-path'); + externalRedirect.findHomeButton().click(); + cy.url().should('include', '/'); + }); + }); +}); diff --git a/frontend/src/pages/external/RedirectErrorState.tsx b/frontend/src/pages/external/RedirectErrorState.tsx index de5f10b1a2..bf944ce478 100644 --- a/frontend/src/pages/external/RedirectErrorState.tsx +++ b/frontend/src/pages/external/RedirectErrorState.tsx @@ -81,7 +81,7 @@ const RedirectErrorState: React.FC = ({ icon={ExclamationCircleIcon} titleText={title ?? 'Error redirecting'} variant={EmptyStateVariant.lg} - data-id="redirect-error" + data-testid="redirect-error" > {errorMessage && {errorMessage}} {fallbackUrl && ( diff --git a/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx b/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx index fb8840dfd4..db6c87a576 100644 --- a/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx +++ b/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx @@ -8,9 +8,9 @@ const ExternalRedirectNotFound: React.FC = () => ( empty emptyStatePage={ } diff --git a/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx b/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx index aa5fa02b03..0c77155806 100644 --- a/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx +++ b/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx @@ -37,7 +37,7 @@ const PipelinesSdkRedirects: React.FC = () => { return globalPipelineRunDetailsRoute(namespace, runId); } - throw new Error('Invalid URL format'); + throw new Error('The URL format is invalid.'); }, [namespace, location.hash]); const [redirect, { loaded, error }] = useRedirect(createRedirectPath); @@ -57,10 +57,10 @@ const PipelinesSdkRedirects: React.FC = () => { actions={ <> } From 82f7bb4b395efc3f56be1fe9b8f0f366d3f995b0 Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Mon, 6 Jan 2025 12:45:39 -0600 Subject: [PATCH 5/8] no longer return redirect func and small adjustments --- frontend/src/pages/ApplicationsPage.tsx | 6 +- .../src/pages/external/RedirectErrorState.tsx | 75 +++++-------------- .../ExternalRedirectNotFound.tsx | 40 ++++++---- .../PipelinesSdkRedirects.tsx | 13 ++-- frontend/src/utilities/useRedirect.ts | 70 ++++++++--------- 5 files changed, 92 insertions(+), 112 deletions(-) diff --git a/frontend/src/pages/ApplicationsPage.tsx b/frontend/src/pages/ApplicationsPage.tsx index 64db623390..c3ff3de4c1 100644 --- a/frontend/src/pages/ApplicationsPage.tsx +++ b/frontend/src/pages/ApplicationsPage.tsx @@ -21,6 +21,7 @@ type ApplicationsPageProps = { loaded: boolean; empty: boolean; loadError?: Error; + loadErrorPage?: React.ReactNode; children?: React.ReactNode; errorMessage?: string; emptyMessage?: string; @@ -41,6 +42,7 @@ const ApplicationsPage: React.FC = ({ loaded, empty, loadError, + loadErrorPage, children, errorMessage, emptyMessage, @@ -77,7 +79,7 @@ const ApplicationsPage: React.FC = ({ const renderContents = () => { if (loadError) { - return ( + return !loadErrorPage ? ( = ({ {loadError.message} + ) : ( + loadErrorPage ); } diff --git a/frontend/src/pages/external/RedirectErrorState.tsx b/frontend/src/pages/external/RedirectErrorState.tsx index bf944ce478..456f6e2e69 100644 --- a/frontend/src/pages/external/RedirectErrorState.tsx +++ b/frontend/src/pages/external/RedirectErrorState.tsx @@ -5,25 +5,15 @@ import { EmptyStateBody, EmptyStateFooter, EmptyStateActions, - Button, } from '@patternfly/react-core'; import { ExclamationCircleIcon } from '@patternfly/react-icons'; import React from 'react'; -import { useNavigate } from 'react-router-dom'; -import { EitherOrNone } from '~/typeHelpers'; type RedirectErrorStateProps = { title?: string; errorMessage?: string; -} & EitherOrNone< - { - fallbackUrl: string; - fallbackText: string; - }, - { - actions: React.ReactNode | React.ReactNode[]; - } ->; + actions?: React.ReactNode | React.ReactNode[]; +}; /** * A component that displays an error state with optional title, message and actions @@ -32,21 +22,11 @@ type RedirectErrorStateProps = { * Props for the RedirectErrorState component * @property {string} [title] - Optional title text to display in the error state * @property {string} [errorMessage] - Optional error message to display - * @property {string} [fallbackUrl] - URL to navigate to when fallback button is clicked * @property {React.ReactNode | React.ReactNode[]} [actions] - Custom action buttons/elements to display * - * Note: The component accepts either fallbackUrl OR actions prop, but not both. - * This is enforced by the EitherOrNone type helper. * * @example * ```tsx - * // With fallback URL - * - * * // With custom actions * = ({ title, errorMessage, - fallbackUrl, - fallbackText, actions, -}) => { - const navigate = useNavigate(); - - return ( - - - {errorMessage && {errorMessage}} - {fallbackUrl && ( - - - - - - )} - {actions && ( - - {actions} - - )} - - - ); -}; +}) => ( + + + {errorMessage && {errorMessage}} + {actions && ( + + {actions} + + )} + + +); export default RedirectErrorState; diff --git a/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx b/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx index db6c87a576..b7e5c86325 100644 --- a/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx +++ b/frontend/src/pages/external/redirectComponents/ExternalRedirectNotFound.tsx @@ -1,20 +1,32 @@ +import { Button } from '@patternfly/react-core'; import React from 'react'; +import { useNavigate } from 'react-router-dom'; import ApplicationsPage from '~/pages/ApplicationsPage'; import RedirectErrorState from '~/pages/external/RedirectErrorState'; -const ExternalRedirectNotFound: React.FC = () => ( - - } - /> -); +const ExternalRedirectNotFound: React.FC = () => { + const navigate = useNavigate(); + + return ( + + + + } + /> + } + /> + ); +}; export default ExternalRedirectNotFound; diff --git a/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx b/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx index 0c77155806..ce49fc8393 100644 --- a/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx +++ b/frontend/src/pages/external/redirectComponents/PipelinesSdkRedirects.tsx @@ -40,17 +40,14 @@ const PipelinesSdkRedirects: React.FC = () => { throw new Error('The URL format is invalid.'); }, [namespace, location.hash]); - const [redirect, { loaded, error }] = useRedirect(createRedirectPath); - - React.useEffect(() => { - redirect(); - }, [redirect]); + const { error } = useRedirect(createRedirectPath); return ( '/foo'); + * // Basic usage + * const { loaded, error } = useRedirect(() => '/foo'); * * // With async path creation - * const [redirect, state] = useRedirect(async () => { + * const { loaded, error } = useRedirect(async () => { * const data = await fetchData(); * return `/bar/${data.id}`; * }); * * // With options - * const [redirect, state] = useRedirect(() => '/foobar', { + * const { loaded, error } = useRedirect(() => '/foobar', { * navigateOptions: { replace: true }, * onComplete: () => console.log('Redirected'), * onError: (error) => console.error(error) * }); * - * // Usage + * // Usage in a component * const createRedirectPath = React.useCallback(() => '/some/path', []); * - * const [redirect, { loaded, error }] = useRedirect(createRedirectPath); - * - * React.useEffect(() => { - * redirect(); - * }, [redirect]); - * + * const { loaded, error } = useRedirect(createRedirectPath); * * return ( - * } - * /> + * navigate('/foo/bar')}>Go to Home} + * />} + * /> * ); * ``` */ export const useRedirect = ( - createRedirectPath: () => string | Promise | undefined, + createRedirectPath: () => string | Promise, options: RedirectOptions = {}, -): [(notFoundOnError?: boolean) => Promise, RedirectState] => { +): RedirectState => { const { navigateOptions, onComplete, onError } = options; - const navigate = useNavigate(); const [state, setState] = React.useState({ loaded: false, error: undefined, }); - const redirect = React.useCallback(async () => { - try { - const path = await createRedirectPath(); - if (!path) { - throw new Error('No redirect path available'); + React.useEffect(() => { + const performRedirect = async () => { + try { + setState({ loaded: false, error: undefined }); + const path = await createRedirectPath(); + navigate(path, navigateOptions); + setState({ loaded: true, error: undefined }); + onComplete?.(); + } catch (e) { + const error = e instanceof Error ? e : new Error('Failed to redirect'); + setState({ loaded: true, error }); + onError?.(error); } - navigate(path, navigateOptions); - setState({ loaded: true, error: undefined }); - onComplete?.(); - } catch (e) { - const error = e instanceof Error ? e : new Error('Failed to redirect'); - setState({ loaded: true, error }); - onError?.(error); - } + }; + + performRedirect(); }, [createRedirectPath, navigate, navigateOptions, onComplete, onError]); - return [redirect, state]; + return state; }; From 9462b08103a7bb3802adf1a8483bcf221886534a Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Mon, 6 Jan 2025 12:54:16 -0600 Subject: [PATCH 6/8] added docs --- docs/external-redirects.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 docs/external-redirects.md diff --git a/docs/external-redirects.md b/docs/external-redirects.md new file mode 100644 index 0000000000..c84f8b1ae4 --- /dev/null +++ b/docs/external-redirects.md @@ -0,0 +1,23 @@ +# External Redirects + +The supported external redirect paths in the application. + +## Supported Redirects + +### Pipeline SDK Redirects + +Redirecting from Pipeline SDK output URLs to internal dashboard routes. + +#### Supported URL Patterns + +1. **Experiment Details** + ``` + /external/pipelinesSdk/{namespace}/#/experiments/details/{experimentId} + ``` + Redirects to the internal experiment runs route for the specified experiment. + +2. **Run Details** + ``` + /external/pipelinesSdk/{namespace}/#/runs/details/{runId} + ``` + Redirects to the internal pipeline run details route for the specified run. From 113e8c256138f69e1860c5d138f4a5bff8aeda73 Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Mon, 6 Jan 2025 12:55:24 -0600 Subject: [PATCH 7/8] quick comment --- frontend/src/pages/external/ExternalRoutes.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frontend/src/pages/external/ExternalRoutes.tsx b/frontend/src/pages/external/ExternalRoutes.tsx index 8e08cc5d63..15fea35151 100644 --- a/frontend/src/pages/external/ExternalRoutes.tsx +++ b/frontend/src/pages/external/ExternalRoutes.tsx @@ -3,6 +3,9 @@ import { Route, Routes } from 'react-router-dom'; import PipelinesSdkRedirect from './redirectComponents/PipelinesSdkRedirects'; import ExternalRedirectNotFound from './redirectComponents/ExternalRedirectNotFound'; +/** + * Be sure to keep this file in sync with the documentation in `docs/external-redirects.md`. + */ const ExternalRoutes: React.FC = () => ( } /> From 58b3b4285ac47cc5b90dcf12f792a43b19164567 Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Tue, 7 Jan 2025 10:18:04 -0600 Subject: [PATCH 8/8] fix tests --- .../utilities/__tests__/useRedirect.spec.ts | 74 +++++++++++-------- 1 file changed, 43 insertions(+), 31 deletions(-) diff --git a/frontend/src/utilities/__tests__/useRedirect.spec.ts b/frontend/src/utilities/__tests__/useRedirect.spec.ts index 895aa4dffd..c2063abcaa 100644 --- a/frontend/src/utilities/__tests__/useRedirect.spec.ts +++ b/frontend/src/utilities/__tests__/useRedirect.spec.ts @@ -16,59 +16,66 @@ describe('useRedirect', () => { const onComplete = jest.fn(); const renderResult = testHook(useRedirect)(createRedirectPath, { onComplete }); - const [redirect, state] = renderResult.result.current; + let state = renderResult.result.current; expect(state.loaded).toBe(false); expect(state.error).toBeUndefined(); - await redirect(); - renderResult.rerender(createRedirectPath, { onComplete }); + await renderResult.waitForNextUpdate(); + state = renderResult.result.current; expect(createRedirectPath).toHaveBeenCalled(); expect(mockNavigate).toHaveBeenCalledWith('/success-path', undefined); expect(onComplete).toHaveBeenCalled(); - expect(renderResult.result.current[1].loaded).toBe(true); - expect(renderResult.result.current[1].error).toBeUndefined(); + expect(state.loaded).toBe(true); + expect(state.error).toBeUndefined(); }); it('should handle async redirect path creation', async () => { const createRedirectPath = jest.fn().mockResolvedValue('/async-path'); const renderResult = testHook(useRedirect)(createRedirectPath); - const [redirect] = renderResult.result.current; - await redirect(); - renderResult.rerender(createRedirectPath); + let state = renderResult.result.current; + expect(state.loaded).toBe(false); + expect(state.error).toBeUndefined(); + + await renderResult.waitForNextUpdate(); + state = renderResult.result.current; expect(createRedirectPath).toHaveBeenCalled(); expect(mockNavigate).toHaveBeenCalledWith('/async-path', undefined); - expect(renderResult.result.current[1].loaded).toBe(true); - expect(renderResult.result.current[1].error).toBeUndefined(); + expect(state.loaded).toBe(true); + expect(state.error).toBeUndefined(); }); it('should handle redirect with navigation options', async () => { const createRedirectPath = jest.fn().mockReturnValue('/path'); const navigateOptions = { replace: true }; const renderResult = testHook(useRedirect)(createRedirectPath, { navigateOptions }); - const [redirect] = renderResult.result.current; + let state = renderResult.result.current; + expect(state.loaded).toBe(false); + expect(state.error).toBeUndefined(); - await redirect(); - renderResult.rerender(createRedirectPath, { navigateOptions }); + await renderResult.waitForNextUpdate(); + state = renderResult.result.current; expect(mockNavigate).toHaveBeenCalledWith('/path', navigateOptions); }); it('should handle error when path is undefined', async () => { - const createRedirectPath = jest.fn().mockReturnValue(undefined); + const createRedirectPath = jest.fn().mockRejectedValue(new Error('No path available')); const onError = jest.fn(); const renderResult = testHook(useRedirect)(createRedirectPath, { onError }); - const [redirect] = renderResult.result.current; + let state = renderResult.result.current; + expect(state.loaded).toBe(false); + expect(state.error).toBeUndefined(); - await redirect(); - renderResult.rerender(createRedirectPath, { onError }); + await renderResult.waitForNextUpdate(); + state = renderResult.result.current; expect(onError).toHaveBeenCalledWith(expect.any(Error)); - expect(renderResult.result.current[1].loaded).toBe(true); - expect(renderResult.result.current[1].error).toBeInstanceOf(Error); + expect(state.loaded).toBe(true); + expect(state.error).toBeInstanceOf(Error); }); it('should handle error in path creation', async () => { @@ -77,35 +84,40 @@ describe('useRedirect', () => { const onError = jest.fn(); const renderResult = testHook(useRedirect)(createRedirectPath, { onError }); - const [redirect] = renderResult.result.current; + let state = renderResult.result.current; + expect(state.loaded).toBe(false); + expect(state.error).toBeUndefined(); - await redirect(); - renderResult.rerender(createRedirectPath, { onError }); + await renderResult.waitForNextUpdate(); + state = renderResult.result.current; expect(onError).toHaveBeenCalledWith(error); - expect(renderResult.result.current[1].loaded).toBe(true); - expect(renderResult.result.current[1].error).toBe(error); + expect(state.loaded).toBe(true); + expect(state.error).toBe(error); }); it('should not redirect to not-found when notFoundOnError is false', async () => { const createRedirectPath = jest.fn().mockRejectedValue(new Error()); const renderResult = testHook(useRedirect)(createRedirectPath); - const [redirect] = renderResult.result.current; + let state = renderResult.result.current; + expect(state.loaded).toBe(false); + expect(state.error).toBeUndefined(); - await redirect(false); - renderResult.rerender(createRedirectPath); + await renderResult.waitForNextUpdate(); + state = renderResult.result.current; expect(mockNavigate).not.toHaveBeenCalled(); - expect(renderResult.result.current[1].loaded).toBe(true); - expect(renderResult.result.current[1].error).toBeInstanceOf(Error); + + expect(state.loaded).toBe(true); + expect(state.error).toBeInstanceOf(Error); }); it('should be stable', () => { const createRedirectPath = jest.fn().mockReturnValue('/path'); const renderResult = testHook(useRedirect)(createRedirectPath); renderResult.rerender(createRedirectPath); - expect(renderResult).hookToBeStable([true, true]); - expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable({ loaded: true, error: undefined }); + expect(renderResult).hookToHaveUpdateCount(3); }); });