Fix crash when clicking back on room requests #381
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors RoomRequestLanding state/URL semester sync (adds memoization, tightens setSemester updates, removes Loader), reworks Router to use a centralized ErrorPage via router Changes
Sequence Diagram(s)sequenceDiagram
participant Component as RoomRequestLanding
participant URL as BrowserURL
participant State as LocalState
participant Select as SemesterSelect
Note over Component,URL: Semester sync on mount/update
Component->>URL: read semester param
URL-->>Component: semester (or none)
Component->>State: validate semester against nextSemesters
alt valid
Component->>State: set semester
else invalid or missing
Component->>State: set default semester (first nextSemester)
Component->>URL: push default semester to URL
end
Component->>Select: render with selected semester
Note over Select,Component: User changes selection
Select->>Component: onChange(newSemester)
Component->>Component: setSemester(newSemester)
alt newSemester non-null && different
Component->>State: update semester
Component->>URL: update URL param
else skip
end
sequenceDiagram
participant RouterOld as Router (old)
participant RouterNew as Router (new)
participant ErrorBoundary as ErrorBoundary
participant ErrorPage as ErrorPage
Note over RouterOld: Previous flow
RouterOld->>ErrorBoundary: render per-route ErrorBoundary wrapping routes
ErrorBoundary->>RouterOld: catch/render errors inline
Note over RouterNew: New flow
RouterNew->>ErrorPage: provide errorElement for routers
RouterNew->>Children: render child routes under errorElement
ErrorPage->>RouterNew: handle route errors via useRouteError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
Comment |
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/ui/pages/roomRequest/RoomRequestLanding.page.tsx(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Required Reviews
src/ui/pages/roomRequest/RoomRequestLanding.page.tsx
[error] 1-1: Requirement 'Base Requirement' is not satisfied by the existing reviews.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Unit Tests
- GitHub Check: Build Application
🔇 Additional comments (3)
src/ui/pages/roomRequest/RoomRequestLanding.page.tsx (3)
2-2: LGTM - Import cleanup consistent with loading state removal.The removal of
Loaderfrom imports is consistent with the simplified conditional rendering logic.
26-31: Verify null handling behavior.The function silently ignores
nullvalues. If theSelectcomponent allows users to clear their selection (returningnull), this will prevent deselection. Confirm this is the intended behavior.
93-110: LGTM - Cleaner conditional rendering.The simplified rendering logic is an improvement. The
Selectdirectly usessetSemesteras itsonChangehandler, andExistingRoomRequestsonly renders when a valid semester is selected. This eliminates the need for loading states and extra conditional nesting.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/ui/Router.tsx(3 hunks)src/ui/pages/Error.page.tsx(1 hunks)
🧰 Additional context used
🪛 ESLint
src/ui/pages/Error.page.tsx
[error] 1-1: Resolve error: EACCES: permission denied, open '/LKvPAfUvBj'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
src/ui/Router.tsx
[error] 1-1: Resolve error: EACCES: permission denied, open '/TIJlfcsNoU'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 8-8: Missing file extension for "./components/AuthContext"
(import/extensions)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Application
- GitHub Check: Run Unit Tests
🔇 Additional comments (4)
src/ui/Router.tsx (4)
97-113: LGTM!The profileRouter correctly uses the centralized
ErrorPageviaerrorElementand properly structures routes as nested children.
115-135: LGTM!The unauthenticatedRouter correctly uses the centralized
ErrorPageand properly handles unauthenticated user routing.
155-236: LGTM!The authenticated routes are well-organized and comprehensive, with proper 404 handling via the catch-all route.
240-249: LGTM!The main Router component correctly selects the appropriate router based on authentication state and cleanly renders the RouterProvider.
| export const ErrorPage: React.FC = () => { | ||
| const error = useRouteError() as Error; |
There was a problem hiding this comment.
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.
| ...commonRoutes, | ||
| { | ||
| path: "/", | ||
| element: <Navigate to="/home" replace />, | ||
| }, | ||
| { | ||
| path: "/login", | ||
| element: <LoginPage />, | ||
| }, | ||
| { | ||
| path: "/logout", | ||
| element: <LogoutPage />, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate /logout route.
The /logout route is already included in commonRoutes (lines 84-86) which are spread at line 142. The explicit definition at lines 152-154 is redundant.
Apply this diff to remove the duplicate:
...commonRoutes,
{
path: "/",
element: <Navigate to="/home" replace />,
},
{
path: "/login",
element: <LoginPage />,
},
- {
- path: "/logout",
- element: <LogoutPage />,
- },
{
path: "/profile",
element: <ManageProfilePage />,📝 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.
| ...commonRoutes, | |
| { | |
| path: "/", | |
| element: <Navigate to="/home" replace />, | |
| }, | |
| { | |
| path: "/login", | |
| element: <LoginPage />, | |
| }, | |
| { | |
| path: "/logout", | |
| element: <LogoutPage />, | |
| }, | |
| ...commonRoutes, | |
| { | |
| path: "/", | |
| element: <Navigate to="/home" replace />, | |
| }, | |
| { | |
| path: "/login", | |
| element: <LoginPage />, | |
| }, |
🤖 Prompt for AI Agents
In src/ui/Router.tsx around lines 142 to 154, there is a duplicate /logout
route: commonRoutes (spread at ~line 142) already includes /logout (lines
84-86), and an explicit /logout route is again defined at lines 152-154; remove
the explicit route block for path "/logout" (the object with path "/logout" and
element <LogoutPage />) so the router only relies on the entry provided by
commonRoutes, keeping all other route entries ("/", "/login", etc.) unchanged.
Summary by CodeRabbit
New Features
Bug Fixes