Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
import { useIsGraderOrInstructor, useIsInstructor } from "@/hooks/useClassProfiles";
import { Box, Button, Flex, Heading, HStack, VStack } from "@chakra-ui/react";
import { Select } from "chakra-react-select";
import { hasRubricUnsavedChangesFlag, RUBRIC_UNSAVED_CHANGES_WARNING_MESSAGE } from "@/lib/rubricUnsavedChanges";
import NextLink from "next/link";
import { useParams, usePathname, useRouter } from "next/navigation";
import React, { useState } from "react";
import React, { useCallback, useState } from "react";
import {
FaCalendar,
FaChartBar,
Expand Down Expand Up @@ -106,6 +107,15 @@ export function ManageAssignmentNav({
const pathname = usePathname();
const [selectedPage, setSelectedPage] = useState<string>("");
const router = useRouter();
const confirmRubricNavigation = useCallback(
(nextHref: string) => {
if (!pathname.includes("/rubric")) return true;
if (pathname === nextHref) return true;
if (!hasRubricUnsavedChangesFlag(String(assignment_id))) return true;
return window.confirm(RUBRIC_UNSAVED_CHANGES_WARNING_MESSAGE);
},
[assignment_id, pathname]
);

return (
<>
Expand Down Expand Up @@ -156,6 +166,7 @@ export function ManageAssignmentNav({
<Select
onChange={(e) => {
if (e) {
if (!confirmRubricNavigation(e.value)) return;
setSelectedPage(e.value);
router.replace(e.value);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,12 @@ import {
import { useCreate, useDataProvider, useDelete, useInvalidate, useUpdate } from "@refinedev/core";
import { configureMonacoYaml } from "monaco-yaml";
import dynamic from "next/dynamic";
import { memo, useCallback, useEffect, useMemo, useRef, useState } from "react";
import { memo, useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from "react";
import {
clearRubricUnsavedChangesFlag,
RUBRIC_UNSAVED_CHANGES_WARNING_MESSAGE,
setRubricUnsavedChangesFlag
} from "@/lib/rubricUnsavedChanges";

// Dynamic import of Monaco Editor to reduce build memory usage
const Editor = dynamic(() => import("@monaco-editor/react").then((mod) => mod.default), {
Expand Down Expand Up @@ -532,6 +537,23 @@ function InnerRubricPage() {
{} as Record<string, boolean>
)
);
const hasAnyUnsavedChanges = useMemo(
() => hasUnsavedChanges || Object.values(unsavedStatusPerTab).some(Boolean),
[hasUnsavedChanges, unsavedStatusPerTab]
);
const hasAnyUnsavedChangesRef = useRef(hasAnyUnsavedChanges);
const shouldSkipNextPopStateWarningRef = useRef(false);
const rubricPageRootRef = useRef<HTMLDivElement>(null);
const isRubricPageInstanceVisible = useCallback(() => {
const rootElement = rubricPageRootRef.current;
if (!rootElement) return true;
return rootElement.getClientRects().length > 0;
}, []);
Comment on lines +547 to +551
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Visibility ownership handoff is not reactive to breakpoint-only visibility changes.

This effect won’t re-run when the instance switches hidden/visible via CSS alone, so the session flag owner can stay stale until some other dependency changes.

Suggested direction
+const [isRubricPageVisible, setIsRubricPageVisible] = useState(true);
+
+useEffect(() => {
+  const updateVisibility = () => {
+    const rootElement = rubricPageRootRef.current;
+    setIsRubricPageVisible(!rootElement || rootElement.getClientRects().length > 0);
+  };
+  updateVisibility();
+  window.addEventListener("resize", updateVisibility);
+  return () => window.removeEventListener("resize", updateVisibility);
+}, []);
+
 useLayoutEffect(() => {
   if (!assignment_id) return;
-  if (!isRubricPageInstanceVisible()) return;
+  if (!isRubricPageVisible) return;
 
   setRubricUnsavedChangesFlag(assignment_id, hasAnyUnsavedChanges);
   return () => {
     clearRubricUnsavedChangesFlag(assignment_id);
   };
-}, [assignment_id, hasAnyUnsavedChanges, isRubricPageInstanceVisible]);
+}, [assignment_id, hasAnyUnsavedChanges, isRubricPageVisible]);

Also applies to: 901-909

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/course/`[course_id]/manage/assignments/[assignment_id]/rubric/page.tsx
around lines 547 - 551, The isRubricPageInstanceVisible useCallback (referencing
rubricPageRootRef) only checks getClientRects once and won’t detect visibility
toggled purely by CSS; replace or augment this with a reactive visibility
observer (e.g., IntersectionObserver or ResizeObserver) tied to the same root
ref so visibility changes fire updates immediately, and update the code that
uses isRubricPageInstanceVisible (and the related logic at the other occurrence
around lines 901-909) to subscribe/unsubscribe the observer lifecycle and call
the same handler that sets the session flag owner whenever the observer reports
visibility changes.


useLayoutEffect(() => {
hasAnyUnsavedChangesRef.current = hasAnyUnsavedChanges;
}, [hasAnyUnsavedChanges]);

const [stashedEditorStates, setStashedEditorStates] = useState<
Record<
string,
Expand Down Expand Up @@ -876,6 +898,81 @@ function InnerRubricPage() {
}
}, [value, initialActiveRubricSnapshot, activeReviewRound, assignment_id]);

useLayoutEffect(() => {
if (!assignment_id) return;
if (!isRubricPageInstanceVisible()) return;

setRubricUnsavedChangesFlag(assignment_id, hasAnyUnsavedChanges);
return () => {
clearRubricUnsavedChangesFlag(assignment_id);
};
}, [assignment_id, hasAnyUnsavedChanges, isRubricPageInstanceVisible]);

useEffect(() => {
const handleBeforeUnload = (event: BeforeUnloadEvent) => {
if (!isRubricPageInstanceVisible()) return;
if (!hasAnyUnsavedChangesRef.current) return;
event.preventDefault();
event.returnValue = "";
};

const handlePopState = () => {
if (!isRubricPageInstanceVisible()) return;
if (shouldSkipNextPopStateWarningRef.current) {
shouldSkipNextPopStateWarningRef.current = false;
return;
}
if (!hasAnyUnsavedChangesRef.current) return;

const shouldLeave = window.confirm(RUBRIC_UNSAVED_CHANGES_WARNING_MESSAGE);
if (shouldLeave) return;

shouldSkipNextPopStateWarningRef.current = true;
window.history.go(1);
};

const handleDocumentClick = (event: MouseEvent) => {
if (!isRubricPageInstanceVisible()) return;
if (!hasAnyUnsavedChangesRef.current || event.defaultPrevented) return;
if (event.button !== 0 || event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) return;

const target = event.target;
if (!(target instanceof Element)) return;

const anchor = target.closest("a[href]");
if (!(anchor instanceof HTMLAnchorElement)) return;
if (anchor.target === "_blank" || anchor.hasAttribute("download")) return;

const href = anchor.getAttribute("href");
if (!href) return;

const currentUrl = new URL(window.location.href);
const nextUrl = new URL(href, window.location.href);
const isSameDocumentLocation =
currentUrl.origin === nextUrl.origin &&
currentUrl.pathname === nextUrl.pathname &&
currentUrl.search === nextUrl.search;

if (isSameDocumentLocation) return;

const shouldLeave = window.confirm(RUBRIC_UNSAVED_CHANGES_WARNING_MESSAGE);
if (shouldLeave) return;

event.preventDefault();
event.stopPropagation();
};

window.addEventListener("beforeunload", handleBeforeUnload);
window.addEventListener("popstate", handlePopState);
document.addEventListener("click", handleDocumentClick, true);

return () => {
window.removeEventListener("beforeunload", handleBeforeUnload);
window.removeEventListener("popstate", handlePopState);
document.removeEventListener("click", handleDocumentClick, true);
};
}, [isRubricPageInstanceVisible]);

const updatePartIfChanged = useCallback(
async (part: HydratedRubricPart, existingPart: HydratedRubricPart) => {
if (part.id !== existingPart.id) {
Expand Down Expand Up @@ -1302,7 +1399,7 @@ function InnerRubricPage() {
}

return (
<Flex w="100%" minW="0" direction="column">
<Flex ref={rubricPageRootRef} w="100%" minW="0" direction="column">
<HStack w="100%" mt={2} mb={2} justifyContent="space-between" pr={2}>
<Toaster />
<VStack align="start">
Expand Down
36 changes: 36 additions & 0 deletions lib/rubricUnsavedChanges.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
export const RUBRIC_UNSAVED_CHANGES_WARNING_MESSAGE =
"You have unsaved rubric changes. Leave this page without saving?";

const STORAGE_KEY_PREFIX = "pawtograder:rubric-unsaved-changes";

function getStorageKeyOrNull(assignmentId: string | number): string | null {
const normalizedAssignmentId = String(assignmentId).trim();
if (!normalizedAssignmentId) return null;
return `${STORAGE_KEY_PREFIX}:${normalizedAssignmentId}`;
}

export function setRubricUnsavedChangesFlag(assignmentId: string | number, hasUnsavedChanges: boolean): void {
if (typeof window === "undefined") return;
const storageKey = getStorageKeyOrNull(assignmentId);
if (!storageKey) return;

if (hasUnsavedChanges) {
window.sessionStorage.setItem(storageKey, "true");
return;
}
window.sessionStorage.removeItem(storageKey);
}
Comment on lines +12 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n lib/rubricUnsavedChanges.ts

Repository: pawtograder/platform

Length of output: 1696


🌐 Web query:

sessionStorage throws error Safari private mode restricted browser contexts

💡 Result:

In Safari, sessionStorage/localStorage can throw at access time (not just on setItem) in “restricted browser contexts” such as Private Browsing in older Safari/WebKit behavior (quota effectively 0, causing QuotaExceededError) or when the user has privacy settings like “Block all cookies” enabled (can raise SecurityError: The operation is insecure). [1][2][3]

What to do

  1. Never touch sessionStorage without a try/catch (even reading window.sessionStorage can throw in some Safari setups). [3]
  2. Feature-test by writing+removing a key, and fall back to in-memory (or cookies/server) if it fails. [2][3]
export function storageAvailable(kind = "sessionStorage") {
  try {
    const s = window[kind];              // may throw in Safari
    const k = "__storage_test__";
    s.setItem(k, "1");                   // may throw QuotaExceededError (quota=0)
    s.removeItem(k);
    return true;
  } catch {
    return false;
  }
}

// usage
const mem = new Map();
export const store = {
  get(key) {
    if (storageAvailable("sessionStorage")) return sessionStorage.getItem(key);
    return mem.get(key) ?? null;
  },
  set(key, val) {
    if (storageAvailable("sessionStorage")) sessionStorage.setItem(key, val);
    else mem.set(key, String(val));
  }
};

If you tell me the exact Safari version + platform (macOS/iOS) + the thrown error text (SecurityError vs QuotaExceededError) and whether this is happening inside an iframe/WKWebView, I can narrow it to the specific Safari restriction you’re hitting.

Sources: [1][2][3]


🏁 Script executed:

rg "sessionStorage" --type ts --type tsx --type js --type jsx

Repository: pawtograder/platform

Length of output: 91


🏁 Script executed:

rg "sessionStorage" --type ts --type js

Repository: pawtograder/platform

Length of output: 2002


Harden storage access to avoid runtime crashes in restricted browser storage contexts.

Direct window.sessionStorage calls can throw in Safari (private browsing, privacy settings) and other restricted browser contexts, breaking navigation and save flows. Wrap storage access in a safe accessor with try-catch and no-op when unavailable.

Proposed fix
+function getSessionStorageOrNull(): Storage | null {
+  if (typeof window === "undefined") return null;
+  try {
+    return window.sessionStorage;
+  } catch {
+    return null;
+  }
+}
+
 export function setRubricUnsavedChangesFlag(assignmentId: string | number, hasUnsavedChanges: boolean): void {
-  if (typeof window === "undefined") return;
+  const storage = getSessionStorageOrNull();
+  if (!storage) return;
   const storageKey = getStorageKeyOrNull(assignmentId);
   if (!storageKey) return;
 
   if (hasUnsavedChanges) {
-    window.sessionStorage.setItem(storageKey, "true");
+    storage.setItem(storageKey, "true");
     return;
   }
-  window.sessionStorage.removeItem(storageKey);
+  storage.removeItem(storageKey);
 }
 
 export function hasRubricUnsavedChangesFlag(assignmentId: string | number): boolean {
-  if (typeof window === "undefined") return false;
+  const storage = getSessionStorageOrNull();
+  if (!storage) return false;
   const storageKey = getStorageKeyOrNull(assignmentId);
   if (!storageKey) return false;
-  return window.sessionStorage.getItem(storageKey) === "true";
+  return storage.getItem(storageKey) === "true";
 }
 
 export function clearRubricUnsavedChangesFlag(assignmentId: string | number): void {
-  if (typeof window === "undefined") return;
+  const storage = getSessionStorageOrNull();
+  if (!storage) return;
   const storageKey = getStorageKeyOrNull(assignmentId);
   if (!storageKey) return;
-  window.sessionStorage.removeItem(storageKey);
+  storage.removeItem(storageKey);
 }

Also applies to: 24-29, 31-36

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/rubricUnsavedChanges.ts` around lines 12 - 22, The code directly uses
window.sessionStorage in setRubricUnsavedChangesFlag which can throw in
restricted browser contexts; create a small safe accessor utility (e.g.,
safeSessionStorage with methods getItem(key): string|null, setItem(key, val):
void, removeItem(key): void) that wraps window.sessionStorage calls in try/catch
and no-ops/returns null on error, then replace direct calls in
setRubricUnsavedChangesFlag (and the other rubric storage functions referenced
in the comment) to use safeSessionStorage.setItem / getItem / removeItem so
failures won't propagate.


export function hasRubricUnsavedChangesFlag(assignmentId: string | number): boolean {
if (typeof window === "undefined") return false;
const storageKey = getStorageKeyOrNull(assignmentId);
if (!storageKey) return false;
return window.sessionStorage.getItem(storageKey) === "true";
}

export function clearRubricUnsavedChangesFlag(assignmentId: string | number): void {
if (typeof window === "undefined") return;
const storageKey = getStorageKeyOrNull(assignmentId);
if (!storageKey) return;
window.sessionStorage.removeItem(storageKey);
}
35 changes: 35 additions & 0 deletions tests/unit/rubric-unsaved-changes.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import {
clearRubricUnsavedChangesFlag,
hasRubricUnsavedChangesFlag,
setRubricUnsavedChangesFlag
} from "@/lib/rubricUnsavedChanges";

describe("rubric unsaved changes session state", () => {
const assignmentId = "123";

beforeEach(() => {
window.sessionStorage.clear();
});

it("sets and reads unsaved state", () => {
setRubricUnsavedChangesFlag(assignmentId, true);
expect(hasRubricUnsavedChangesFlag(assignmentId)).toBe(true);
});

it("clears unsaved state when set false", () => {
setRubricUnsavedChangesFlag(assignmentId, true);
setRubricUnsavedChangesFlag(assignmentId, false);
expect(hasRubricUnsavedChangesFlag(assignmentId)).toBe(false);
});

it("clears unsaved state explicitly", () => {
setRubricUnsavedChangesFlag(assignmentId, true);
clearRubricUnsavedChangesFlag(assignmentId);
expect(hasRubricUnsavedChangesFlag(assignmentId)).toBe(false);
});

it("ignores empty assignment ids", () => {
setRubricUnsavedChangesFlag("", true);
expect(hasRubricUnsavedChangesFlag("")).toBe(false);
});
});
Loading