Skip to content

fix: show session expired modal instead of silent redirect to login #31

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

Open
wicky-zipstack wants to merge 1 commit into
base: main
Choose a base branch
from
+32 −20
Open

fix: show session expired modal instead of silent redirect to login #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 25 additions & 18 deletions frontend/src/auth/RequireAuth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,20 @@ import { Navigate, Outlet, useNavigate, useLocation } from "react-router-dom";
import { useSessionStore } from "../store/session-store";

const RequireAuth = () => {
const { showSessionExpiredModal, sessionDetails } = useSessionStore();
const {
showSessionExpiredModal,
setShowSessionExpiredModal,
sessionDetails,
} = useSessionStore();
const navigate = useNavigate();
const location = useLocation();

const isLoggedIn = !!sessionDetails?.user?.id;

const handleLoginRedirect = useCallback(() => {
setShowSessionExpiredModal(false);
navigate("/login");
}, [navigate]);
}, [navigate, setShowSessionExpiredModal]);

const modalFooter = useMemo(
() => [
Expand All @@ -24,31 +29,33 @@ const RequireAuth = () => {
[handleLoginRedirect]
);

if (!isLoggedIn) {
return <Navigate to="/login" />;
}

if (location.pathname === "/") {
return <Navigate to="/project/list" />;
}

return (
<>
<Outlet />
// Show session expired modal instead of silently redirecting
if (!isLoggedIn && showSessionExpiredModal) {
return (
<Modal
title="Session Expired"
open={showSessionExpiredModal}
open
centered
footer={modalFooter}
closable={false}
maskClosable={false}
>
<Typography>
Your session has expired. Please log in again to continue your
transformation with Visitran Ai.
Your session has expired. Please log in again to continue.
</Typography>
</Modal>
</>
);
);
}

if (!isLoggedIn) {
return <Navigate to="/login" />;
}

if (location.pathname === "/") {
return <Navigate to="/project/list" />;
}

return <Outlet />;
};

export { RequireAuth };
1 change: 1 addition & 0 deletions frontend/src/service/axios-service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ function useAxiosPrivate() {
if (error?.response?.status === 401) {
localStorage.removeItem("orgid");
localStorage.removeItem("session-storage");
useSessionStore.getState().setShowSessionExpiredModal(true);
setSessionDetails({});
return Promise.resolve({ suppressed: true });
}
Expand Down
8 changes: 6 additions & 2 deletions frontend/src/service/notification-service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { notification, Button } from "antd";
import ReactMarkdown from "react-markdown";
import remarkGfm from "remark-gfm";
import "./notification-border.css";
import { useSessionStore } from "../store/session-store";

const NotificationContext = createContext(null);

Expand Down Expand Up @@ -50,8 +51,11 @@ function NotificationProvider({ children }) {
let finalDescription = description;

if (type === "error") {
// Skip notification for 401 errors - session expiry modal handles this
if (error?.response?.status === 401) {
// Skip all notifications when session expired modal is showing
if (
error?.response?.status === 401 ||
useSessionStore.getState().showSessionExpiredModal
) {
return;
}
Comment on lines +54 to 60
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Apr 2, 2026

Choose a reason for hiding this comment

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

P2 Overly broad notification suppression

The added useSessionStore.getState().showSessionExpiredModal condition now suppresses all error notifications while the modal is visible — not just those originating from 401 responses. If any in-flight request (e.g., a concurrent 500 or network error) fails after the session-expired modal appears, its error toast will be silently swallowed.

The original intent (per the PR description) was to avoid duplicate toasts for the 401 that triggered the modal, but this check goes further and mutes all error feedback.

If the goal is only to avoid the duplicate 401 toast, the original condition (error?.response?.status === 401) already covered that. The extra clause only matters for non-401 errors that arrive while the modal is open. Consider scoping the suppression to 401 only, or adding a comment acknowledging the broader suppression is intentional:

Suggested change
// Skip all notifications when session expired modal is showing
if (
error?.response?.status === 401 ||
useSessionStore.getState().showSessionExpiredModal
) {
return;
}
if (
error?.response?.status === 401 ||
// Suppress all error toasts while the session-expired modal is shown,
// since any concurrent request failures are likely a consequence of
// the expired session and would only add noise.
useSessionStore.getState().showSessionExpiredModal
) {
return;
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: frontend/src/service/notification-service.js
Line: 54-60

Comment:
**Overly broad notification suppression**

The added `useSessionStore.getState().showSessionExpiredModal` condition now suppresses **all** error notifications while the modal is visible — not just those originating from 401 responses. If any in-flight request (e.g., a concurrent 500 or network error) fails after the session-expired modal appears, its error toast will be silently swallowed.

The original intent (per the PR description) was to avoid *duplicate* toasts for the 401 that triggered the modal, but this check goes further and mutes all error feedback.

If the goal is only to avoid the duplicate 401 toast, the original condition (`error?.response?.status === 401`) already covered that. The extra clause only matters for non-401 errors that arrive while the modal is open. Consider scoping the suppression to 401 only, or adding a comment acknowledging the broader suppression is intentional:

```suggestion
        if (
          error?.response?.status === 401 ||
          // Suppress all error toasts while the session-expired modal is shown,
          // since any concurrent request failures are likely a consequence of
          // the expired session and would only add noise.
          useSessionStore.getState().showSessionExpiredModal
        ) {
          return;
        }
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor Author

@wicky-zipstack wicky-zipstack Apr 2, 2026

Choose a reason for hiding this comment

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

Intentional — when the session expired modal is showing, the user is about to be redirected to login. Any concurrent errors (500s from other in-flight requests) are irrelevant at that point since the session is dead. Suppressing all toasts avoids a noisy screen full of stacked error notifications.


Expand Down
Loading