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

STUD-300: Implement 404 "Page Not Found" error page for NEWM Studio #848

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
a8e3ae3
feat: Add NEWM Monster Percussion for 404 page
dmkirshon Jan 14, 2025
6c8ce62
feat: Create 404 Page Not Found template
dmkirshon Jan 14, 2025
35d9723
refactor: Sort package indexes
dmkirshon Jan 21, 2025
713cb6b
feat: Add PageNotFound to App routes
dmkirshon Jan 21, 2025
df4ace5
feat: Add PageNotFound to Home route
dmkirshon Jan 21, 2025
ca546dc
feat: Add PageNotFound to Library route
dmkirshon Jan 21, 2025
d34a028
feat: Add PageNotFound to Collaborator routes
dmkirshon Jan 21, 2025
061354a
feat: Add PageNotFound to UploadSong routes
dmkirshon Jan 21, 2025
2249592
feat: Add PageNotFound to EditSong route
dmkirshon Jan 27, 2025
12d3176
feat: Add PageNotFound to CreateProfile routes
dmkirshon Jan 27, 2025
47c911b
feat: Add PageNotFound to ForgotPassword routes
dmkirshon Jan 27, 2025
45aab55
feat: Add PageNotFound to SignUp routes
dmkirshon Jan 27, 2025
c86d8ee
feat: Add navigation to closest url path for 404
dmkirshon Jan 27, 2025
ff8cac7
feat: Add back collab id error redirect
dmkirshon Jan 28, 2025
8480f42
fix: Revert EditSong navigation functionality
dmkirshon Jan 28, 2025
b403b16
feat: Add redirect Urls for specific paths
dmkirshon Jan 28, 2025
1578e54
feat: Remove redundant page not found routes
dmkirshon Jan 28, 2025
bea8b65
refactor: Simplify CreateProfile by consolidation
dmkirshon Feb 3, 2025
f924237
refactor: Change top margin in subroutes for 404
dmkirshon Feb 7, 2025
185a893
refactor: Simplify UploadSong for 404 component
dmkirshon Feb 7, 2025
3086c3b
refactor: Simplify pages for 404 component use
dmkirshon Feb 7, 2025
8e24cad
doc: Revert useless change for sake of change
dmkirshon Feb 7, 2025
48cc2a9
refactor: Remove console logs that hold no value
dmkirshon Feb 7, 2025
3c0435d
Merge branch 'master' into STUD-300-Implement-404-Page-Note-Found-err…
dmkirshon Feb 7, 2025
a417f80
refactor: Import correct react router utils
dmkirshon Feb 7, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions apps/studio/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { GOOGLE_CLIENT_ID } from "@newm-web/env";
import {
LDProvider,
Maintenance,
PageNotFound,
UnsupportedBrowserBanner,
} from "@newm-web/components";
import {
Expand Down Expand Up @@ -121,6 +122,9 @@ const App = () => {
}
path="create-profile/*"
/>

{ /* Catch-all route for 404 Page Not Found */ }
<Route element={ <PageNotFound /> } path="*" />
</Routes>
</BrowserRouter>
</Background>
Expand Down
31 changes: 30 additions & 1 deletion apps/studio/src/pages/createProfile/CreateProfile.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { FunctionComponent } from "react";
import { FunctionComponent, useEffect, useState } from "react";
import { Box, Container, useTheme } from "@mui/material";
import { WizardForm } from "@newm-web/elements";
import * as Yup from "yup";
import { getUpdatedValues } from "@newm-web/utils";
import { useLocation } from "react-router-dom";
import { PageNotFound } from "@newm-web/components";
import Begin from "./Begin";
import SelectNickname from "./SelectNickname";
import SelectRole from "./SelectRole";
Expand All @@ -21,6 +23,9 @@ import {

const CreateProfile: FunctionComponent = () => {
const theme = useTheme();
const currentPathLocation = useLocation();
const [isValidPath, setIsValidPath] = useState(true);

const { data: roles = [] } = useGetRolesQuery();
const {
data: { firstName, lastName, role, location, nickname } = emptyProfile,
Expand Down Expand Up @@ -59,6 +64,30 @@ const CreateProfile: FunctionComponent = () => {
updateInitialProfile(updatedValues);
};

/**
* Checks if the current path is a valid path or requires a 404 page.
*/
useEffect(() => {
const validPaths = [
"/create-profile",
"/create-profile/what-is-your-first-name",
"/create-profile/what-is-your-last-name",
"/create-profile/what-should-we-call-you",
"/create-profile/what-is-your-role",
"/create-profile/what-is-your-location",
"/create-profile/complete",
];

const normalizePath = (path: string) => path.replace(/\/+$/, ""); // Remove trailing slashes
const currentPath = normalizePath(currentPathLocation.pathname);

setIsValidPath(validPaths.map(normalizePath).includes(currentPath));
}, [currentPathLocation.pathname]);

if (!isValidPath) {
return <PageNotFound />;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this useEffect? Couldn't we just use

if (!isValidPath) {
  return <PageNotFound />;
}

directly? It would make the code a bit cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to remove the useEffect and convert the valid path conditional into a function? I will play around with removing the useEffect and see how the rendering is affected by it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I supposed a benefit of having it in a useEffect call is that it would only run once. It could be fine to keep it there, on second thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the useEffect and went for the useMemo route, it cleaned up code and simplified the logic a bit as well. Copilot said there shouldn't be any large rerender performance issues from doing it this way. Let me know if you see any issues with the update on your end.


return (
<Box
sx={ {
Expand Down
23 changes: 22 additions & 1 deletion apps/studio/src/pages/forgotPassword/ForgotPassword.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import * as Yup from "yup";
import { Box, Container } from "@mui/material";
import { FunctionComponent } from "react";
import { FunctionComponent, useEffect, useState } from "react";
import { FormikHelpers, FormikValues } from "formik";
import theme from "@newm-web/theme";
import { WizardForm } from "@newm-web/elements";
import { useLocation } from "react-router";
import { PageNotFound } from "@newm-web/components";
import InitiateReset from "./InitiateReset";
import VerifyEmail from "./VerifyEmail";
import ResetPassword from "./ResetPassword";
Expand All @@ -13,6 +15,8 @@ import { ResponsiveNEWMLogo } from "../../components";

const ForgotPassword: FunctionComponent = () => {
const dispatch = useAppDispatch();
const location = useLocation();
const [isValidPath, setIsValidPath] = useState(true); // State to track path validity

const initialValues = {
authCode: "",
Expand All @@ -38,6 +42,23 @@ const ForgotPassword: FunctionComponent = () => {
setSubmitting(false);
};

useEffect(() => {
const validPaths = [
"/forgot-password",
"/forgot-password/verification",
"/forgot-password/reset",
];

const normalizePath = (path: string) => path.replace(/\/+$/, ""); // Remove trailing slashes
const currentPath = normalizePath(location.pathname);

setIsValidPath(validPaths.map(normalizePath).includes(currentPath));
}, [location.pathname]);

if (!isValidPath) {
return <PageNotFound />;
}

return (
<Container
maxWidth={ false }
Expand Down
3 changes: 3 additions & 0 deletions apps/studio/src/pages/home/Home.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { FunctionComponent, useState } from "react";
import { Navigate, Route, Routes, useNavigate } from "react-router-dom";
import { useTheme } from "@mui/material/styles";
import MenuIcon from "@mui/icons-material/Menu";
import { PageNotFound } from "@newm-web/components";
import SideBar from "./SideBar";
import UploadSong from "./uploadSong/UploadSong";
import Library from "./library/Library";
Expand Down Expand Up @@ -78,6 +79,8 @@ const Home: FunctionComponent = () => {
<Route element={ <Wallet /> } path="wallet" />
<Route element={ <Profile /> } path="profile" />
<Route element={ <Settings /> } path="settings" />

<Route element={ <PageNotFound /> } path="*" />
</Routes>
</Box>
</Box>
Expand Down
3 changes: 2 additions & 1 deletion apps/studio/src/pages/home/library/EditSong.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -257,10 +257,11 @@ const EditSong: FunctionComponent = () => {
* Ensure user is returned to first step on refresh since form
* contents are not persisted.
*
* TODO: remove this when form values are persisted on refresh
* TODO: remove the navigation when form values are persisted on refresh
*/
useEffect(() => {
navigate(`/home/library/edit-song/${songId}`, { replace: true });

// useNavigate doesn't return memoized function, including it
// as dependency will run this when navigation occurs. Exclude
// to only run on mount.
Expand Down
5 changes: 3 additions & 2 deletions apps/studio/src/pages/home/library/Library.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Container } from "@mui/material";
import { FunctionComponent } from "react";
import { Navigate, Route, Routes } from "react-router-dom";
import { Route, Routes } from "react-router-dom";
import { PageNotFound } from "@newm-web/components";
import EditSong from "./EditSong";
import Discography from "./Discography";
import ViewDetails from "./ViewDetails";
Expand All @@ -23,7 +24,7 @@ const Library: FunctionComponent = () => (
<Route element={ <EditSong /> } path="edit-song/:songId*" />
<Route element={ <ViewDetails /> } path="view-details/:songId" />

<Route element={ <Navigate to="/home/library" replace /> } path="*" />
<Route element={ <PageNotFound redirectUrl="/home/library" /> } path="*" />
</Routes>
</Container>
);
Expand Down
8 changes: 6 additions & 2 deletions apps/studio/src/pages/home/owners/Owners.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Container } from "@mui/material";
import { FunctionComponent } from "react";
import { Navigate, Route, Routes } from "react-router-dom";
import { Route, Routes } from "react-router-dom";
import { PageNotFound } from "@newm-web/components";
import Owner from "./Owner";
import OwnersList from "./OwnersList";

Expand All @@ -21,7 +22,10 @@ const Owners: FunctionComponent = () => (
<Route element={ <OwnersList /> } path="/" />
<Route element={ <Owner /> } path="/:userId" />

<Route element={ <Navigate to="/home/collaborators" replace /> } path="*" />
<Route
element={ <PageNotFound redirectUrl="/home/collaborators" /> }
path="*"
/>
</Routes>
</Container>
);
Expand Down
36 changes: 30 additions & 6 deletions apps/studio/src/pages/home/uploadSong/UploadSong.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { Box, Container, Typography } from "@mui/material";
import { WizardForm } from "@newm-web/elements";
import { FormikHelpers, FormikValues } from "formik";
import { FunctionComponent, useEffect } from "react";
import { useNavigate } from "react-router-dom";
import { FunctionComponent, useEffect, useState } from "react";
import { useLocation, useNavigate } from "react-router-dom";
import * as Yup from "yup";
import { PageNotFound } from "@newm-web/components";
import AdvancedSongDetails from "./AdvancedSongDetails";
import BasicSongDetails from "./BasicSongDetails";
import ConfirmAgreement from "./ConfirmAgreement";
Expand All @@ -28,6 +29,9 @@ export interface UploadSongFormValues extends UploadSongThunkRequest {

const UploadSong: FunctionComponent = () => {
const navigate = useNavigate();
const location = useLocation();

const [isValidPath, setIsValidPath] = useState(true);

const { data: genres = [] } = useGetGenresQuery();
const { data: roles = [] } = useGetRolesQuery();
Expand Down Expand Up @@ -139,17 +143,37 @@ const UploadSong: FunctionComponent = () => {

/**
* Ensure user is returned to first step on refresh since form
* contents are not persisted.
* contents are not persisted. If user navigates to an invalid
* path, redirect to 404 page.
*
* TODO: remove this when form values are persisted on refresh
* TODO: remove the navigation when form values are persisted on refresh
*/
useEffect(() => {
navigate("/home/upload-song", { replace: true });
const validPaths = [
"/home/upload-song",
"/home/upload-song/advanced-details",
"/home/upload-song/confirm",
];
Copy link
Contributor

@scandycuz scandycuz Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach could create some issues, as we'll need to make sure to always update this array when adding a new route, or it could result in the 404 component being incorrectly displayed. Instead, it would be better to declare the routes variable externally and then derive this array from them. E.g.

const routes = [{
  path: "advanced-details",
  ...
}]

const validRoutes = routes.map((route) =>  `/home/upload-song/${route.path}`)

...

if (!isValidPath) {
  return <PageNotFound redirectUrl="/home/upload-song" />;
}

return (
  <WizardForm
    routes={routes}
    ...

That way it only has to be updated in one place, when the routes are declared.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 404 redirect functionality could also be included in the WizardForm component, although I feel less strongly about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to consolidate the routes, will make those changes.

I was trying to keep to the highest level where the routing logic was made. WizardForm is used in multiple different contexts so it felt off to integrate any triggering there.

Copy link
Contributor

@scandycuz scandycuz Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't have a strong opinion on having the logic in the WizardForm. There are pros and cons to doing it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored the wizard routes and check for valid routes to mimic your example, it should be a little bit cleaner and should reduce future conflicts on updating the routes.


// Remove trailing slashes to compare paths
const normalizePath = (path: string) => path.replace(/\/+$/, "");
const currentPath = normalizePath(location.pathname);

if (!validPaths.includes(currentPath)) {
setIsValidPath(false);
} else {
setIsValidPath(true);
navigate("/home/upload-song", { replace: true });
}
// useNavigate doesn't return memoized function, including it
// as dependency will run this when navigation occurs. Exclude
// to only run on mount.
// eslint-disable-next-line
}, []);
}, [location.pathname]);

if (!isValidPath) {
return <PageNotFound redirectUrl="/home/upload-song" />;
}

const validations = {
agreesToCoverArtGuidelines: commonYupValidation.agreesToCoverArtGuidelines,
Expand Down
20 changes: 19 additions & 1 deletion apps/studio/src/pages/signUp/SignUp.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import * as Yup from "yup";
import { Box, useTheme } from "@mui/material";
import { FormikValues } from "formik";
import { FunctionComponent } from "react";
import { FunctionComponent, useEffect, useState } from "react";
import { WizardForm } from "@newm-web/elements";
import { useLocation } from "react-router";
import { PageNotFound } from "@newm-web/components";
import Verification from "./Verification";
import Welcome from "./Welcome";
import { commonYupValidation, useAppDispatch } from "../../common";
Expand All @@ -18,6 +20,9 @@ interface AccountValues {
const SignUp: FunctionComponent = () => {
const theme = useTheme();
const dispatch = useAppDispatch();
const location = useLocation();
const [isValidPath, setIsValidPath] = useState(true); // State to track path validity

const initialValues: AccountValues = {
authCode: "",
confirmPassword: "",
Expand Down Expand Up @@ -55,6 +60,19 @@ const SignUp: FunctionComponent = () => {
dispatch(createAccount({ authCode, confirmPassword, email, newPassword }));
};

useEffect(() => {
const validPaths = ["/sign-up", "/sign-up/verification"];

const normalizePath = (path: string) => path.replace(/\/+$/, ""); // Remove trailing slashes
const currentPath = normalizePath(location.pathname);

setIsValidPath(validPaths.map(normalizePath).includes(currentPath));
}, [location.pathname]);

if (!isValidPath) {
return <PageNotFound redirectUrl="/sign-up" />;
}

return (
<Box
sx={ {
Expand Down
20 changes: 11 additions & 9 deletions packages/assets/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import AddImage from "./lib/icons/AddImage";
import AddSong from "./lib/icons/AddSong";
import artistAgreementPreview from "./lib/images/artist-agreement-preview.jpg";
import bgImage from "./lib/images/bg-img.webp";
import Charli3Logo from "./lib/icons/Charli3Logo";
import CheckboxIcon from "./lib/icons/CheckboxIcon";
import CheckCircle from "./lib/icons/CheckCircle";
Expand All @@ -19,26 +17,29 @@ import ItunesLogo from "./lib/icons/ItunesLogo";
import NEWMLogo from "./lib/icons/NEWMLogo";
import NEWMLogoSmInverse from "./lib/icons/NEWMLogoSmInverse";
import NEWMMarketplaceLogo from "./lib/icons/NEWMMarketplaceLogo";
import { default as NEWMMonster } from "./lib/images/NEWMMonster.png";
import Owner from "./lib/icons/Owner";
import PeopleIcon from "./lib/icons/PeopleIcon";
import PlayButton from "./lib/icons/PlayButton";
import SelectedCheckboxIcon from "./lib/icons/SelectedCheckboxIcon";
import SoundWave from "./lib/icons/SoundWave";
import Suitcase from "./lib/icons/Suitcase";
import Search from "./lib/icons/Search";
import SelectedCheckboxIcon from "./lib/icons/SelectedCheckboxIcon";
import SoundCloudLogo from "./lib/icons/SoundCloudLogo";
import SoundWave from "./lib/icons/SoundWave";
import SpotifyLogo from "./lib/icons/SpotifyLogo";
import Suitcase from "./lib/icons/Suitcase";
import TimeCircleLine from "./lib/icons/TimeCircleLine";
import UnselectedCheckboxIcon from "./lib/icons/UnselectedCheckboxIcon";
import UploadIcon from "./lib/icons/UploadIcon";
import VerticalEllipsis from "./lib/icons/VerticalEllipsis";
import WalletIcon from "./lib/icons/WalletIcon";
import XLogo from "./lib/icons/XLogo";
import artistAgreementPreview from "./lib/images/artist-agreement-preview.jpg";
import bgImage from "./lib/images/bg-img.webp";
import { default as NEWMMonsterPercussion } from "./lib/images/NEWMMonsterPercussion.png";
import { default as NEWMMonsterRock } from "./lib/images/NEWMMonsterRock.png";

export {
AddSong,
AddImage,
AddSong,
artistAgreementPreview,
bgImage,
Charli3Logo,
Expand All @@ -58,12 +59,13 @@ export {
NEWMLogo,
NEWMLogoSmInverse,
NEWMMarketplaceLogo,
NEWMMonster,
NEWMMonsterPercussion,
NEWMMonsterRock,
Owner,
PeopleIcon,
PlayButton,
SelectedCheckboxIcon,
Search,
SelectedCheckboxIcon,
SoundCloudLogo,
SoundWave,
SpotifyLogo,
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading