Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add external routes to AppRoutes component #3569

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Gkrumbach07
Copy link
Member

@Gkrumbach07 Gkrumbach07 commented Dec 11, 2024

https://issues.redhat.com/browse/RHOAIENG-8476

Description

This PR adds a system to handle external URL redirects in a centralized way, with the first implementation handling Pipeline SDK URLs.

WIP bc waiting for backend to confirm some things but could probably merge before them

What's Added

  • External routes system under /external/*
  • Redirect handling with loading and error states
  • Pipeline SDK URL support for experiments and runs

How to Extend

  1. Create a new redirect component in frontend/src/pages/external/redirectComponents/
  2. Add your route to ExternalRoutes.tsx
  3. Use useRedirect hook to handle the redirect logic

Example:

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 (
	<ApplicationsPage
		loaded={loaded}
		empty={!!error}
		emptyStatePage={<RedirectErrorState fallbackUrl="/foo/bar"/>}
	/>
);

Error state when parsing pipelinesSdk url fails
ex) external/pipelinesSdk/gages-proj/#/some/unexpected/route
image

Custom not found page when incorrect
ex) external/someUndefinedRoute
image

How Has This Been Tested?

Test these URLs locally:
For experiments
http://localhost:4010/external/pipelinesSdk/your-namespace/#/experiments/details/your-experiment-id
For runs
http://localhost:4010/external/pipelinesSdk/your-namespace/#/runs/details/your-run-id

The URLs will redirect to their respective internal routes while handling loading and error states.

Test Impact

Added a test for the hook

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

@kaedward @kywalker-rh
Yall should look over wording and button styling for this.

  • wording for both error messages and buttons (the description text comes from the error message which could be updated to if you want, but basically is generic we couldn't not match the path)
  • where should the pipelines redirect one point to
    • they are given something like this when a run completes in the sdk
Experiment details: http://ml-pipeline-ui-kubeflow.apps.rmartine.dev.datahub.redhat.com/#/experiments/details/2b36adf0-cf2a-4c63-af8e-0d44c731b598
Run details: http://ml-pipeline-ui-kubeflow.apps.rmartine.dev.datahub.redhat.com/#/runs/details/f20096dd-669f-476f-ace3-c33fe7e0864a
  • should there be two buttons becuase we dont know if the user was trying to come from one or the other (pipelines or experiment)
  • where should the not found button point to if anywhere

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

- 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.
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Dec 11, 2024
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 98.38710% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.20%. Comparing base (9124c70) to head (459f214).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...ernal/redirectComponents/PipelinesSdkRedirects.tsx 95.65% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3569      +/-   ##
==========================================
- Coverage   85.42%   85.20%   -0.22%     
==========================================
  Files        1378     1385       +7     
  Lines       31428    31597     +169     
  Branches     8797     8831      +34     
==========================================
+ Hits        26846    26921      +75     
- Misses       4582     4676      +94     
Files with missing lines Coverage Δ
frontend/src/app/AppRoutes.tsx 94.33% <100.00%> (+0.10%) ⬆️
frontend/src/pages/external/ExternalRoutes.tsx 100.00% <100.00%> (ø)
frontend/src/pages/external/RedirectErrorState.tsx 100.00% <100.00%> (ø)
...al/redirectComponents/ExternalRedirectNotFound.tsx 100.00% <100.00%> (ø)
frontend/src/utilities/useRedirect.ts 100.00% <100.00%> (ø)
...ernal/redirectComponents/PipelinesSdkRedirects.tsx 95.65% <95.65%> (ø)

... and 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9124c70...459f214. Read the comment docs.

@Gkrumbach07 Gkrumbach07 removed the do-not-merge/work-in-progress This PR is in WIP state label Jan 6, 2025
@Gkrumbach07 Gkrumbach07 changed the title [WIP] Add external routes to AppRoutes component Add external routes to AppRoutes component Jan 6, 2025
Copy link
Contributor

openshift-ci bot commented Jan 6, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from gkrumbach07. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rimolive
Copy link

@Gkrumbach07 This change lgtm for the DSP since it somewhat follows the pattern used in Kubeflow Pipelines:

Experiment details: https://ds-pipeline-dspa-pipelinesapi1.apps.dspv2.osp.rh-ods.com//#/experiments/details/c8ce99bc-3545-4f0f-b491-950258a55fab

When this PR is done, I can work to change KFP SDK to add new parameters to customize the experiments and runs page refirects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants