-
Notifications
You must be signed in to change notification settings - Fork 174
Add exam mode #3106
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.
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
base: master
Are you sure you want to change the base?
Add exam mode #3106
Conversation
…ausing spinner to show
…s to Playground.tsx for GitHub and Google Drive buttons
RichDom2185
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor nits :), thanks!
…n not an official course, remove exam mode toggle when creating new course
…om navbar getting cropped
…nto pr/kah-seng/3106
…nto pr/kah-seng/3106
…nto pr/kah-seng/3106
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive exam mode functionality to Source Academy, allowing instructors to enable a restricted environment for students during assessments. The implementation includes admin controls for enabling/previewing exam mode, UI restrictions (hiding remote execution, file sharing, and course switching), security features (dev tools detection, tab switching monitoring), and a pause/resume mechanism with authentication.
Key Changes
- Added exam mode toggle and preview functionality in admin panel with resume code authentication
- Implemented client-side security features: dev tools blocking, context menu prevention, and tab focus monitoring
- Added documentation tab with embedded SICP content for assessments
- Created pause overlay system with resume code validation
Reviewed Changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/commons/application/Application.tsx |
Implements dev tools detection, tab switching monitoring, and pause overlay logic |
src/pages/academy/adminPanel/subcomponents/CourseConfigPanel.tsx |
Adds exam mode toggle, resume code input, and preview mode button |
src/pages/academy/adminPanel/AdminPanel.tsx |
Validates resume code on submit and updates course config |
src/commons/sideContent/content/SideContentDocumentation.tsx |
New documentation tab component with iframe-based content |
src/commons/application/types/SessionTypes.ts |
Extends types with exam mode fields (enableExamMode, resumeCode, isPaused) |
src/commons/sagas/RequestsSaga.ts |
Adds API endpoints for resume code validation and user pause/focus tracking |
src/commons/sagas/BackendSaga.ts |
Implements saga handlers for exam mode API calls |
src/pages/playground/Playground.tsx |
Conditionally hides UI elements when exam mode is active |
src/pages/academy/Academy.tsx |
Prevents course switching when exam mode is enabled |
src/commons/dropdown/Dropdown*.tsx |
Disables course creation/switching dropdowns in exam mode |
src/commons/pauseAcademyOverlay/PauseAcademyOverlay.tsx |
New pause overlay component with resume code input |
src/styles/*.scss |
Styling for documentation tabs and pause overlay |
| Test files | Updates mock data with new exam mode fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
@sentry review |
| // Detect when Source Academy tab's content are hidden (e.g., user changes tab while Source Academy is active) | ||
| document.addEventListener('visibilitychange', _ => { | ||
| if (document.visibilityState === 'hidden') { | ||
| dispatch(SessionActions.reportFocusLost()); | ||
| } else { | ||
| showDangerMessage('Source Academy was out of focus.', 5000); | ||
| dispatch(SessionActions.reportFocusRegain()); | ||
| } | ||
| }); | ||
| } | ||
| }, [dispatch, enableExamMode, isPaused, hasSentPauseUserRequest, role, isPreviewExamMode]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The useEffect hook adds event listeners without cleanup, causing accumulation and memory growth on dependency changes.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The useEffect hook at src/commons/application/Application.tsx:171~223 adds contextmenu, keydown, and visibilitychange event listeners to the document. The effect's dependency array includes dispatch, enableExamMode, isPaused, hasSentPauseUserRequest, role, and isPreviewExamMode. When any of these dependencies change, the effect re-runs, adding new listeners without removing previously added ones. This leads to an unbounded accumulation of event handlers, causing performance degradation and increased memory usage over time.
💡 Suggested Fix
Add a cleanup function to the useEffect hook that removes the contextmenu, keydown, and visibilitychange event listeners when the component unmounts or dependencies change.
🤖 Prompt for AI Agent
Fix this bug. In src/commons/application/Application.tsx at lines 171-223: The
`useEffect` hook at `src/commons/application/Application.tsx:171~223` adds
`contextmenu`, `keydown`, and `visibilitychange` event listeners to the document. The
effect's dependency array includes `dispatch`, `enableExamMode`, `isPaused`,
`hasSentPauseUserRequest`, `role`, and `isPreviewExamMode`. When any of these
dependencies change, the effect re-runs, adding new listeners without removing
previously added ones. This leads to an unbounded accumulation of event handlers,
causing performance degradation and increased memory usage over time.
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| // Effect for dev tools blocking/detection when exam mode enabled | ||
| React.useEffect(() => { | ||
| if (role !== Role.Student && !isPreviewExamMode) { | ||
| return; | ||
| } | ||
|
|
||
| const showPauseAcademyOverlay = (reason: string) => { | ||
| setPauseAcademy(true); | ||
| setPauseAcademyReason(reason); | ||
| if (hasSentPauseUserRequest.current === false) { | ||
| dispatch(SessionActions.pauseUser()); | ||
| hasSentPauseUserRequest.current = true; | ||
| } | ||
| }; | ||
|
|
||
| if (enableExamMode || isPreviewExamMode) { | ||
| dispatch(SessionActions.reportFocusRegain()); | ||
|
|
||
| if (isPaused !== undefined && isPaused) { | ||
| showPauseAcademyOverlay('Browser was refreshed when Source Academy was paused'); | ||
| } else { | ||
| hasSentPauseUserRequest.current = false; | ||
| } | ||
|
|
||
| // Disable/Detect dev tools | ||
| disableDevtool({ | ||
| ondevtoolopen: () => { | ||
| showPauseAcademyOverlay('Developer tools detected'); | ||
| } | ||
| }); | ||
|
|
||
| document.addEventListener('contextmenu', event => event.preventDefault()); | ||
| document.addEventListener('keydown', event => { | ||
| if ( | ||
| event.key === 'F12' || | ||
| ((event.key === 'I' || event.key === 'J' || event.key === 'C') && | ||
| event.ctrlKey && | ||
| event.shiftKey) | ||
| ) { | ||
| event.preventDefault(); | ||
| } | ||
| }); | ||
|
|
||
| // Detect when Source Academy tab's content are hidden (e.g., user changes tab while Source Academy is active) | ||
| document.addEventListener('visibilitychange', _ => { | ||
| if (document.visibilityState === 'hidden') { | ||
| dispatch(SessionActions.reportFocusLost()); | ||
| } else { | ||
| showDangerMessage('Source Academy was out of focus.', 5000); | ||
| dispatch(SessionActions.reportFocusRegain()); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dev tools detection effect (lines 169-221) adds event listeners to the document without ever removing them. This causes memory leaks when the component unmounts or when the effect re-runs. Each time the effect runs, new listeners are added without removing the old ones, potentially accumulating dozens of event listeners. Add a cleanup function that removes all listeners, or better yet, use AbortController or WeakMaps to track and manage listeners.
Severity: HIGH
🤖 Prompt for AI Agent
Fix this code. In src/commons/application/Application.tsx#L169-L221: The dev tools
detection effect (lines 169-221) adds event listeners to the document without ever
removing them. This causes memory leaks when the component unmounts or when the effect
re-runs. Each time the effect runs, new listeners are added without removing the old
ones, potentially accumulating dozens of event listeners. Add a cleanup function that
removes all listeners, or better yet, use AbortController or WeakMaps to track and
manage listeners.
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| type SearchData = { | ||
| indexTrie: TrieNode; | ||
| textTrie: TrieNode; | ||
| idToContentMap: Record<string, string>; | ||
| }; | ||
|
|
||
| const fetchSearchData = () => { | ||
| const xhr = new XMLHttpRequest(); | ||
| const url = Constants.sicpBackendUrl + 'json/rewritedSearchData.json'; | ||
| xhr.open('GET', url, false); //sync download | ||
| xhr.send(); | ||
| if (xhr.status !== 200) { | ||
| alert('Unable to get rewrited search data. Error code = ' + xhr.status + ' url is ' + url); | ||
| throw new Error('Unable to get search data. Error code = ' + xhr.status + ' url is ' + url); | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fetchSearchData() function uses synchronous XMLHttpRequest (line 93: xhr.open('GET', url, false)). This blocks the entire UI thread while the request completes, which can cause the application to become unresponsive. This is a significant performance anti-pattern. Replace this with an asynchronous fetch API and move the data fetching to a useEffect hook, or use a proper data loading library. The synchronous XHR also prevents proper error handling and timeout mechanisms.
Severity: HIGH
🤖 Prompt for AI Agent
Fix this code. In src/commons/documentation/SicpNavigationBar.tsx#L89-L104: The
`fetchSearchData()` function uses synchronous XMLHttpRequest (line 93: `xhr.open('GET',
url, false)`). This blocks the entire UI thread while the request completes, which can
cause the application to become unresponsive. This is a significant performance
anti-pattern. Replace this with an asynchronous fetch API and move the data fetching to
a useEffect hook, or use a proper data loading library. The synchronous XHR also
prevents proper error handling and timeout mechanisms.
Did we get this right? 👍 / 👎 to inform future reviews.
| setActivePage(pages[index]); | ||
| }; | ||
|
|
||
| const handleDocsHome = useCallback(() => { | ||
| if (sicpHomeCallbackFn !== null && activePage.src === 'https://sicp.sourceacademy.org') { | ||
| sicpHomeCallbackFn(); | ||
| } | ||
|
|
||
| if (activeIframeRef.current !== null) { | ||
| activeIframeRef.current.src = activePage.src; | ||
| } | ||
| }, [activePage.src]); | ||
|
|
||
| const sicpHomeCallbackSetter = (fn: () => void) => { | ||
| sicpHomeCallbackFn = fn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pages array is being mutated by pushing a new SICP JS page (line 76) outside of state management. This violates React's principles - the pages array is created fresh on each render, so the SICP JS page will be added repeatedly. Move the SICP JS page into a proper useState hook or create the pages array within useMemo to ensure it's handled correctly during re-renders.
Severity: MEDIUM
🤖 Prompt for AI Agent
Fix this code. In src/commons/sideContent/content/SideContentDocumentation.tsx#L62-L76:
The `pages` array is being mutated by pushing a new SICP JS page (line 76) outside of
state management. This violates React's principles - the `pages` array is created fresh
on each render, so the SICP JS page will be added repeatedly. Move the SICP JS page into
a proper useState hook or create the pages array within useMemo to ensure it's handled
correctly during re-renders.
Did we get this right? 👍 / 👎 to inform future reviews.
| onChange={e => setResumeCode((e.target as HTMLInputElement).value)} | ||
| /> | ||
| </FormGroup> | ||
| <Button text="Submit" onClick={() => props.onSubmit(resumeCode)} /> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The InputGroup component receives an onChange handler that casts the target to HTMLInputElement, but defaultValue should be value with the state variable. Using defaultValue creates an uncontrolled component, which means the input won't properly sync with the state. Change defaultValue="" to value={resumeCode} to make this a controlled component. Additionally, consider adding a maxLength attribute to prevent excessively long resume codes.
Severity: MEDIUM
🤖 Prompt for AI Agent
Fix this code. In src/commons/pauseAcademyOverlay/PauseAcademyOverlay.tsx#L23-L27: The
`InputGroup` component receives an `onChange` handler that casts the target to
`HTMLInputElement`, but `defaultValue` should be `value` with the state variable. Using
`defaultValue` creates an uncontrolled component, which means the input won't properly
sync with the state. Change `defaultValue=""` to `value={resumeCode}` to make this a
controlled component. Additionally, consider adding a `maxLength` attribute to prevent
excessively long resume codes.
Did we get this right? 👍 / 👎 to inform future reviews.
| Tree.nodeFromPath(path, newState).isExpanded = true; | ||
| setSidebarContent(newState); | ||
| }; | ||
|
|
||
| const handleNodeCollapse = (_node: TreeNodeInfo, path: integer[]) => { | ||
| const newState = cloneDeep(sidebarContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type annotation integer[] on lines 19 and 24 is not a valid TypeScript type. It should be number[]. This will cause TypeScript compilation errors. Replace integer with number.
Severity: CRITICAL
💡 Suggested Fix
| Tree.nodeFromPath(path, newState).isExpanded = true; | |
| setSidebarContent(newState); | |
| }; | |
| const handleNodeCollapse = (_node: TreeNodeInfo, path: integer[]) => { | |
| const newState = cloneDeep(sidebarContent); | |
| const handleNodeExpand = (_node: TreeNodeInfo, path: number[]) => { |
Did we get this right? 👍 / 👎 to inform future reviews.
| // Effect for dev tools blocking/detection when exam mode enabled | ||
| React.useEffect(() => { | ||
| if (role !== Role.Student && !isPreviewExamMode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exam mode check on line 170 uses role !== Role.Student && !isPreviewExamMode which returns early. This means non-students won't set up dev tools detection unless in preview mode. The logic seems backwards - you probably want to trigger detection for students OR when in preview mode. The condition should likely be if (role === Role.Student || isPreviewExamMode) before setting up the detection. Please verify the intended behavior.
Severity: MEDIUM
🤖 Prompt for AI Agent
Fix this code. In src/commons/application/Application.tsx#L170-L172: The exam mode check
on line 170 uses `role !== Role.Student && !isPreviewExamMode` which returns early. This
means non-students won't set up dev tools detection unless in preview mode. The logic
seems backwards - you probably want to trigger detection for students OR when in preview
mode. The condition should likely be `if (role === Role.Student || isPreviewExamMode)`
before setting up the detection. Please verify the intended behavior.
Did we get this right? 👍 / 👎 to inform future reviews.
Description
Type of change
How to test
Checklist