Skip to content

Fix crash when clicking back on room requests #381

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 5, 2025
Merged

Fix crash when clicking back on room requests #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3f8ef8f
Fix crash when clicking back on room requests
devksingh4 Nov 5, 2025
bffb9da
Add an error boundary for react errors
devksingh4 Nov 5, 2025
55237aa
Memoize semester options
devksingh4 Nov 5, 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
280 changes: 129 additions & 151 deletions src/ui/Router.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
import React, { useState, useEffect, ReactNode } from "react";
import React from "react";
import {
createBrowserRouter,
Navigate,
RouterProvider,
useLocation,
} from "react-router-dom";
import { AcmAppShell } from "./components/AppShell";
import { useAuth } from "./components/AuthContext";
import AuthCallback from "./components/AuthContext/AuthCallbackHandler.page";
import { Error404Page } from "./pages/Error404.page";
import { Error500Page } from "./pages/Error500.page";
import { ErrorPage } from "./pages/Error.page";
import { HomePage } from "./pages/Home.page";
import { LoginPage } from "./pages/Login.page";
import { LogoutPage } from "./pages/Logout.page";
Expand Down Expand Up @@ -96,165 +95,148 @@ const commonRoutes = [
];

const profileRouter = createBrowserRouter([
...commonRoutes,
{
path: "/profile",
element: <ManageProfilePage />,
},
{
path: "*",
element: <ProfileRediect />,
path: "/",
errorElement: <ErrorPage />,
children: [
...commonRoutes,
{
path: "/profile",
element: <ManageProfilePage />,
},
{
path: "*",
element: <ProfileRediect />,
},
],
},
]);

const unauthenticatedRouter = createBrowserRouter([
...commonRoutes,
{
path: "/",
element: <Navigate to="/login" replace />,
},
{
path: "/login",
element: <LoginPage />,
},
{
path: "*",
element: <LoginRedirect />,
errorElement: <ErrorPage />,
children: [
...commonRoutes,
{
path: "/",
element: <Navigate to="/login" replace />,
},
{
path: "/login",
element: <LoginPage />,
},
{
path: "*",
element: <LoginRedirect />,
},
],
},
]);

const authenticatedRouter = createBrowserRouter([
...commonRoutes,
{
path: "/",
element: <Navigate to="/home" replace />,
},
{
path: "/login",
element: <LoginPage />,
},
{
path: "/logout",
element: <LogoutPage />,
},
{
path: "/profile",
element: <ManageProfilePage />,
},
{
path: "/home",
element: <HomePage />,
},
{
path: "/events/add",
element: <ManageEventPage />,
},
{
path: "/events/edit/:eventId",
element: <ManageEventPage />,
},
{
path: "/events/manage",
element: <ViewEventsPage />,
},
{
path: "/linkry",
element: <LinkShortener />,
},
{
path: "/linkry/add",
element: <ManageLinkPage />,
},
{
path: "/linkry/edit/:slug",
element: <ManageLinkPage />,
},
{
path: "/tickets/scan",
element: <ScanTicketsPage />,
},
{
path: "/tickets",
element: <SelectTicketsPage />,
},
{
path: "/iam",
element: <ManageIamPage />,
},
{
path: "/membershipLists",
element: <ManageExternalMembershipPage />,
},
{
path: "/tickets/manage/:eventId",
element: <ViewTicketsPage />,
},
{
path: "/stripe",
element: <ManageStripeLinksPage />,
},
{
path: "/roomRequests",
element: <ManageRoomRequestsPage />,
},
{
path: "/roomRequests/:semesterId/:requestId",
element: <ViewRoomRequest />,
},
{
path: "/logs",
element: <ViewLogsPage />,
},
{
path: "/apiKeys",
element: <ManageApiKeysPage />,
},
{
path: "/orgInfo",
element: <OrgInfoPage />,
},
// Catch-all route for authenticated users shows 404 page
{
path: "*",
element: <Error404Page />,
errorElement: <ErrorPage />,
children: [
...commonRoutes,
{
path: "/",
element: <Navigate to="/home" replace />,
},
{
path: "/login",
element: <LoginPage />,
},
{
path: "/logout",
element: <LogoutPage />,
},
{
path: "/profile",
element: <ManageProfilePage />,
},
{
path: "/home",
element: <HomePage />,
},
{
path: "/events/add",
element: <ManageEventPage />,
},
{
path: "/events/edit/:eventId",
element: <ManageEventPage />,
},
{
path: "/events/manage",
element: <ViewEventsPage />,
},
{
path: "/linkry",
element: <LinkShortener />,
},
{
path: "/linkry/add",
element: <ManageLinkPage />,
},
{
path: "/linkry/edit/:slug",
element: <ManageLinkPage />,
},
{
path: "/tickets/scan",
element: <ScanTicketsPage />,
},
{
path: "/tickets",
element: <SelectTicketsPage />,
},
{
path: "/iam",
element: <ManageIamPage />,
},
{
path: "/membershipLists",
element: <ManageExternalMembershipPage />,
},
{
path: "/tickets/manage/:eventId",
element: <ViewTicketsPage />,
},
{
path: "/stripe",
element: <ManageStripeLinksPage />,
},
{
path: "/roomRequests",
element: <ManageRoomRequestsPage />,
},
{
path: "/roomRequests/:semesterId/:requestId",
element: <ViewRoomRequest />,
},
{
path: "/logs",
element: <ViewLogsPage />,
},
{
path: "/apiKeys",
element: <ManageApiKeysPage />,
},
{
path: "/orgInfo",
element: <OrgInfoPage />,
},
// Catch-all route for authenticated users shows 404 page
{
path: "*",
element: <Error404Page />,
},
],
},
]);

interface ErrorBoundaryProps {
children: ReactNode;
}

const ErrorBoundary: React.FC<ErrorBoundaryProps> = ({ children }) => {
const [hasError, setHasError] = useState(false);
const [error, setError] = useState<Error | null>(null);
const { isLoggedIn } = useAuth();

const onError = (errorObj: Error) => {
setHasError(true);
setError(errorObj);
};

useEffect(() => {
const errorHandler = (event: ErrorEvent) => {
onError(event.error);
};

window.addEventListener("error", errorHandler);
return () => {
window.removeEventListener("error", errorHandler);
};
}, []);

if (hasError && error) {
if (error.message === "404") {
return isLoggedIn ? <Error404Page /> : <LoginRedirect />;
}
return <Error500Page />;
}

return <>{children}</>;
};

export const Router: React.FC = () => {
const { isLoggedIn } = useAuth();
const router = isLoggedIn
Expand All @@ -263,9 +245,5 @@ export const Router: React.FC = () => {
? profileRouter
: unauthenticatedRouter;

return (
<ErrorBoundary>
<RouterProvider router={router} />
</ErrorBoundary>
);
return <RouterProvider router={router} />;
};
35 changes: 35 additions & 0 deletions src/ui/pages/Error.page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import React from "react";
import { useRouteError } from "react-router-dom";
import { Container, Title, Text, Card, Accordion } from "@mantine/core";
import { AcmAppShell } from "@ui/components/AppShell";

export const ErrorPage: React.FC = () => {
const error = useRouteError() as Error;
Comment on lines +6 to +7
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 5, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Unsafe type assertion can cause runtime errors.

useRouteError() returns unknown and can be an Error, Response, or any thrown value. The as Error assertion is unsafe and will fail at line 25 when accessing .stack or .message if the error is a Response (e.g., from a loader/action) or another type.

Apply this diff to handle different error types safely:

-export const ErrorPage: React.FC = () => {
-  const error = useRouteError() as Error;
+export const ErrorPage: React.FC = () => {
+  const routeError = useRouteError();
+  
+  let errorMessage: string;
+  let errorStack: string | undefined;
+  
+  if (routeError instanceof Error) {
+    errorMessage = routeError.message;
+    errorStack = routeError.stack;
+  } else if (typeof routeError === 'string') {
+    errorMessage = routeError;
+  } else if (routeError && typeof routeError === 'object' && 'statusText' in routeError) {
+    // Handle Response objects
+    errorMessage = (routeError as { statusText?: string; status?: number }).statusText || 'Unknown error';
+  } else {
+    errorMessage = 'An unknown error occurred';
+  }

Then update line 25 to use the safe variables:

-                    <pre>{error.stack || error.message}</pre>
+                    <pre>{errorStack || errorMessage}</pre>

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/ui/pages/Error.page.tsx around lines 6-7, the code unsafely asserts
useRouteError() as Error which can be Response, string, or other thrown values
and will crash when accessing .message or .stack; change to narrow the unknown
safely by detecting types: if error is an instance of Response handle
status/text, if it's an instance of Error extract message and stack, if it's a
string use it as the message, otherwise provide a generic message; assign safe
variables (e.g., const message = ...; const stack = ...;) and use those
variables at line 25 instead of directly using error.message/error.stack.


return (
<AcmAppShell>
<Container>
<Card shadow="sm" p="lg" radius="md" withBorder>
<Title order={2}>Oops! Something went wrong.</Title>
<Text size="lg" c="dimmed" mt="md">
We're sorry, but an unexpected error has occurred. Please contact
support if this error persists.
</Text>

<Accordion mt="md">
<Accordion.Item value="error-details">
<Accordion.Control>Error Details</Accordion.Control>
<Accordion.Panel>
<Card withBorder p="md" mt="md">
<Text c="red">
<pre>{error.stack || error.message}</pre>
</Text>
</Card>
</Accordion.Panel>
</Accordion.Item>
</Accordion>
</Card>
</Container>
</AcmAppShell>
);
};
Loading
Loading