Open
Conversation
Co-authored-by: Jonathan Bell <jon@jonbell.net>
|
Cursor Agent can help with this pull request. Just |
Contributor
WalkthroughThis PR adds unsaved-changes detection and navigation guards for rubric editing. It introduces a new sessionStorage-based utility module to track per-assignment rubric changes, integrates comprehensive lifecycle hooks and browser navigation guards into the rubric page, and extends the assignment navigation component with a confirmation guard before allowing rubric section navigation. Changes
Sequence DiagramsequenceDiagram
participant User
participant RubricPage as Rubric Page
participant Storage as SessionStorage
participant Browser as Browser Navigation
User->>RubricPage: Edit rubric content
RubricPage->>RubricPage: Detect unsaved changes
RubricPage->>Storage: setRubricUnsavedChangesFlag(true)
Storage-->>RubricPage: Flag stored
User->>Browser: Navigate away (back, link, refresh)
Browser->>RubricPage: Trigger beforeunload/popstate
RubricPage->>Storage: hasRubricUnsavedChangesFlag?
Storage-->>RubricPage: true (unsaved exists)
RubricPage->>User: Show warning confirmation dialog
alt User confirms exit
User->>RubricPage: Confirm navigation
RubricPage->>Storage: clearRubricUnsavedChangesFlag()
RubricPage->>Browser: Allow navigation
else User cancels
User->>RubricPage: Cancel
RubricPage->>Browser: Prevent navigation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/rubric-unsaved-changes.test.ts (1)
31-34: Consider adding a whitespace-only assignment ID test.The production helper trims IDs, so
" "should be asserted as invalid too.Suggested test addition
it("ignores empty assignment ids", () => { setRubricUnsavedChangesFlag("", true); expect(hasRubricUnsavedChangesFlag("")).toBe(false); }); + + it("ignores whitespace-only assignment ids", () => { + setRubricUnsavedChangesFlag(" ", true); + expect(hasRubricUnsavedChangesFlag(" ")).toBe(false); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/rubric-unsaved-changes.test.ts` around lines 31 - 34, Add a test case asserting that whitespace-only assignment IDs are treated as invalid: call setRubricUnsavedChangesFlag with a string like " " and then assert hasRubricUnsavedChangesFlag(" ") returns false; this mirrors production trimming behavior and should be added alongside the existing empty-id test referencing setRubricUnsavedChangesFlag and hasRubricUnsavedChangesFlag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/course/`[course_id]/manage/assignments/[assignment_id]/rubric/page.tsx:
- Around line 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.
In `@lib/rubricUnsavedChanges.ts`:
- Around line 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.
---
Nitpick comments:
In `@tests/unit/rubric-unsaved-changes.test.ts`:
- Around line 31-34: Add a test case asserting that whitespace-only assignment
IDs are treated as invalid: call setRubricUnsavedChangesFlag with a string like
" " and then assert hasRubricUnsavedChangesFlag(" ") returns false; this
mirrors production trimming behavior and should be added alongside the existing
empty-id test referencing setRubricUnsavedChangesFlag and
hasRubricUnsavedChangesFlag.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/course/[course_id]/manage/assignments/[assignment_id]/ManageAssignmentNav.tsxapp/course/[course_id]/manage/assignments/[assignment_id]/rubric/page.tsxlib/rubricUnsavedChanges.tstests/unit/rubric-unsaved-changes.test.ts
Comment on lines
+547
to
+551
| const isRubricPageInstanceVisible = useCallback(() => { | ||
| const rootElement = rubricPageRootRef.current; | ||
| if (!rootElement) return true; | ||
| return rootElement.getClientRects().length > 0; | ||
| }, []); |
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+12
to
+22
| 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); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/rubricUnsavedChanges.tsRepository: 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
- Never touch
sessionStoragewithout atry/catch(even readingwindow.sessionStoragecan throw in some Safari setups). [3] - 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 jsxRepository: pawtograder/platform
Length of output: 91
🏁 Script executed:
rg "sessionStorage" --type ts --type jsRepository: 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a navigation-leave warning to the rubric editor to prevent loss of unsaved changes.
The implementation addresses a timing bug where the warning could still fire immediately after saving. This was resolved by ensuring only the currently visible rubric instance manages global listeners and session synchronization, preventing stale state from triggering false warnings.
Summary by CodeRabbit
New Features
Tests