Deamon #137
Deamon #137
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several significant changes across multiple components related to exam functionality. A new Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
src/pages/candidate/ExamMcqResults.tsx (1)
214-217: Provide user feedback upon grade submissionCurrently, success or error messages are logged to the console. For better user experience, display feedback using UI notifications.
Proposed Solution:
Use Ant Design's
messagecomponent:if (result.success) { - console.log('Grade submitted successfully'); + message.success('Grade submitted successfully'); } else { - console.error('Error submitting grade'); + message.error('Error submitting grade'); }src/components/Exam/PreventBackButton.tsx (2)
5-9: HandlepreventDefaultandpushStatecarefully to avoid unexpected behaviorWhile preventing back navigation, modifying the history stack can lead to a cluttered history and may confuse users if not handled properly.
Proposed Solution:
Consider using
event.preventDefault()andevent.stopImmediatePropagation()without pushing new states repeatedly:const preventBack = (event: any) => { event.preventDefault(); + event.stopImmediatePropagation(); - window.history.pushState(null, document.title, window.location.href); };Alternatively, ensure that
pushStateis not called excessively by checking the history state.
26-26: Remove default export to avoid ambiguityHaving both named and default exports of the same component can cause confusion and import issues.
Proposed Solution:
Remove the default export if it's not necessary:
- export default PreventBackButton;src/pages/candidate/CandidateDashboard.tsx (2)
47-55: Handle API errors and loading states appropriatelyEnsure that error handling provides feedback to the user and that loading states are managed to prevent multiple API calls.
Proposed Solution:
Consider adding a loading state and displaying error messages to the user if the API call fails.
78-79: Pass state safely when navigatingEnsure that sensitive information is not passed via the
stateobject when navigating, as it may not persist across page reloads.Proposed Solution:
Store necessary data in a more persistent storage like
sessionStorageor use URL parameters.navigate('/candidate/exam/view', { state: { id: examId, name: examName, type: examType }, });src/pages/candidate/ExamSummary.tsx (2)
Line range hint
67-71: Replace mock implementation with actual session checkThe
checkActiveSessionfunction uses a mock implementation instead of making an actual API call.Consider implementing the actual session check:
const checkActiveSession = async () => { try { - // Mock the response for active session check - const response = { data: { active: false } }; // Mocking as no API exists + const token = sessionStorage.getItem('accessToken'); + if (!token) { + setSessionExists(false); + return; + } + + const examId = id; + const response = await axios.get( + `http://localhost:8080/api/v1/exam/check-session/${examId}`, + { headers: { 'Authorization': `Bearer ${token}` } } + ); setSessionExists(response.data.active); } catch (error) { console.error('Error checking active session:', error); setSessionExists(false); } };
Line range hint
51-55: Address security concerns in API implementationSeveral security considerations need to be addressed:
- API URLs are hardcoded
- No CSRF protection in POST requests
- Sensitive exam data stored in sessionStorage could be accessed by other tabs
Consider these security enhancements:
- Move API URLs to environment configuration
- Add CSRF protection to POST requests
- Consider using more secure storage methods for sensitive data
Example implementation:
+ import { API_BASE_URL } from '../config'; + import { getCsrfToken } from '../utils/security'; - const url = `http://localhost:8080/api/v1/candidate/exams/${id}`; + const url = `${API_BASE_URL}/api/v1/candidate/exams/${id}`; const response = await axios.post( 'http://localhost:8080/api/v1/exam/start', { examId: id }, - { headers: { 'Authorization': `Bearer ${token}` } } + { + headers: { + 'Authorization': `Bearer ${token}`, + 'X-CSRF-Token': getCsrfToken(), + } + } );Also applies to: 104-110
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/components/Exam/PreventBackButton.tsx(1 hunks)src/components/Exam/TimeContainer.tsx(4 hunks)src/components/Exam/index.ts(1 hunks)src/pages/candidate/CandidateDashboard.tsx(2 hunks)src/pages/candidate/ExamMcqResults.tsx(9 hunks)src/pages/candidate/ExamSummary.tsx(1 hunks)src/pages/candidate/ExamView.tsx(2 hunks)
🔇 Additional comments (5)
src/pages/candidate/ExamMcqResults.tsx (1)
Line range hint 342-404: Ensure data is loaded before rendering exam details
There is a risk of examDetails or examResults being undefined, which can cause runtime errors.
Proposed Solution:
Add checks before mapping over examDetails:
- {!loading && examDetails.map((question, index) => {
+ {!loading && examDetails && examResults && examDetails.map((question, index) => {src/components/Exam/index.ts (1)
8-8: Exported component is correctly added
The PreventBackButton component is properly exported and will be available for use in other parts of the application.
src/components/Exam/TimeContainer.tsx (2)
105-105: Disable submit button appropriately
The submit button is disabled when remainingTimeInSeconds <= 0. Ensure this logic aligns with when the exam should be submitted.
Proposed Solution:
Verify that the button disabling logic matches the exam submission criteria.
68-72:
Ensure handleSubmitExam is not called multiple times due to rapid state changes
The useEffect dependency array is missing remainingTimeInSeconds, which may cause handleSubmitExam to be called multiple times.
Proposed Solution:
Include remainingTimeInSeconds in the dependency array:
useEffect(() => {
if (remainingTimeInSeconds <= 0) {
handleSubmitExam();
}
- }, []);
+ }, [remainingTimeInSeconds]);Likely invalid or redundant comment.
src/pages/candidate/CandidateDashboard.tsx (1)
33-36:
Avoid early returns in useEffect without cleanup
Returning early inside a useEffect hook can prevent cleanup functions from running.
Proposed Solution:
Refactor the code to avoid early returns:
if (!userString || !token) {
console.error("User information or token not found in session storage.");
- return;
+ return;
}However, since useEffect cleanup functions are not affected here, and there is no cleanup, this might be acceptable. Verify whether early returns are acceptable in this context.
|
|
||
| interface UserAnswer { | ||
| questionId: number; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define a specific interface for gradingScheme instead of using any[]
Using any[] for the gradingScheme state can lead to type safety issues and make the code harder to maintain. Defining a specific interface improves code readability and type safety.
Proposed Solution:
Define an interface for the grading scheme:
interface GradingScheme {
minMarks: number;
maxMarks: number;
grade: string;
}Update the state declaration:
- const [gradingScheme, setGradingScheme] = useState<any[]>([]);
+ const [gradingScheme, setGradingScheme] = useState<GradingScheme[]>([]);| console.error("User information or token not found in session storage."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Avoid returning early without rendering in a React component
Returning early from a React functional component without rendering can cause unexpected behavior. Instead, consider rendering a fallback UI or redirecting the user.
Proposed Solution:
Use conditional rendering or navigate the user appropriately:
if (!userString || !token) {
console.error("User information or token not found in session storage.");
- return;
+ navigate('/login');
+ return null;
}
if (!candidateId) {
console.error("Candidate ID not found in user object.");
- return;
+ navigate('/login');
+ return null;
}Also applies to: 60-63
| let totalMarks = 0; | ||
| let correctAnswersMarks = 0; | ||
| let skippedQuestions = 0; | ||
|
|
||
| // Assuming each correct answer is worth 10 marks | ||
| const marksPerAnswer = 10; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent variables for marks calculation
The value 10 is hardcoded in multiple places for marks calculation. This can cause inconsistencies if the marking scheme changes. Use the marksPerAnswer variable consistently.
Proposed Solution:
Use marksPerAnswer instead of hardcoded values:
// Define marksPerAnswer at the top if not already defined
const marksPerAnswer = 10;
// Update calculations
- let correctAnswersMarks = 0;
+ let correctAnswersMarks = 0;
...
if (selectedOption.optionId === correctOption.optionId) {
correctAnswersCount += 1;
- correctAnswersMarks += marksPerAnswer;
+ correctAnswersMarks += marksPerAnswer;
}
...
const percentage = totalMarks > 0 ? Math.round((correctAnswersMarks / totalMarks) * 100) : 0;
...
Scored {correctAnswersMarks} out of {totalMarks}Also applies to: 151-151, 190-190, 291-291
| `http://localhost:8080/api/v1/grade/1/grading-scheme`, | ||
| { |
There was a problem hiding this comment.
Avoid hardcoding IDs in API endpoints
The API call uses a hardcoded ID (1) for the grading scheme. This can lead to errors if the grading scheme ID varies. Use a dynamic ID based on the context, such as examId or another relevant identifier.
Proposed Solution:
Replace the hardcoded ID with a dynamic value:
const gradingResponse = await fetch(
- `http://localhost:8080/api/v1/grade/1/grading-scheme`,
+ `http://localhost:8080/api/v1/grade/${examId}/grading-scheme`,
{
method: 'GET',
headers: {
Authorization: `Bearer ${token}`,
},
}
);Committable suggestion skipped: line range outside the PR's diff.
| const onFullscreenChange = () => { | ||
| if (!document.fullscreenElement) { | ||
| console.log('Exited fullscreen'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Enhance fullscreen exit handling for exam integrity
Currently, the code only logs when fullscreen is exited. For exam integrity, consider warning the user or auto-requesting fullscreen again.
Consider this enhancement:
const onFullscreenChange = () => {
if (!document.fullscreenElement) {
- console.log('Exited fullscreen');
+ message.warning('Fullscreen mode is required for the exam. Re-entering fullscreen...');
+ // Give a short delay before re-requesting fullscreen
+ setTimeout(requestFullscreen, 1000);
+
+ // Optional: Log attempt to exit fullscreen for monitoring
+ const sessionId = sessionStorage.getItem('sessionId');
+ axios.post(
+ 'http://localhost:8080/api/v1/exam/log-fullscreen-exit',
+ { sessionId },
+ { headers: { Authorization: `Bearer ${token}` } }
+ ).catch(console.error);
}
};Committable suggestion skipped: line range outside the PR's diff.
| const requestFullscreen = () => { | ||
| const elem = document.documentElement; | ||
| if (elem.requestFullscreen) { | ||
| elem.requestFullscreen(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Enhance fullscreen implementation for better exam security
The current fullscreen implementation has several security gaps:
- No handling of browser prefixes for fullscreen API
- No error handling for fullscreen request
- No prevention or warning when user exits fullscreen mode
Consider this enhanced implementation:
const requestFullscreen = () => {
const elem = document.documentElement;
- if (elem.requestFullscreen) {
+ try {
+ if (elem.requestFullscreen) {
elem.requestFullscreen();
+ } else if (elem.webkitRequestFullscreen) {
+ elem.webkitRequestFullscreen();
+ } else if (elem.msRequestFullscreen) {
+ elem.msRequestFullscreen();
+ } else if (elem.mozRequestFullscreen) {
+ elem.mozRequestFullscreen();
+ } else {
+ throw new Error('Fullscreen API not supported');
}
+ } catch (error) {
+ console.error('Failed to enter fullscreen:', error);
+ message.error('Fullscreen mode is required for the exam. Please use a supported browser.');
+ }
};📝 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.
| const requestFullscreen = () => { | |
| const elem = document.documentElement; | |
| if (elem.requestFullscreen) { | |
| elem.requestFullscreen(); | |
| } | |
| }; | |
| const requestFullscreen = () => { | |
| const elem = document.documentElement; | |
| try { | |
| if (elem.requestFullscreen) { | |
| elem.requestFullscreen(); | |
| } else if (elem.webkitRequestFullscreen) { | |
| elem.webkitRequestFullscreen(); | |
| } else if (elem.msRequestFullscreen) { | |
| elem.msRequestFullscreen(); | |
| } else if (elem.mozRequestFullscreen) { | |
| elem.mozRequestFullscreen(); | |
| } else { | |
| throw new Error('Fullscreen API not supported'); | |
| } | |
| } catch (error) { | |
| console.error('Failed to enter fullscreen:', error); | |
| message.error('Fullscreen mode is required for the exam. Please use a supported browser.'); | |
| } | |
| }; |
| const sessionId = response.data.sessionId; | ||
| console.log('sessionId:', response.data.endTime); | ||
| sessionStorage.setItem('sessionId', sessionId); | ||
| sessionStorage.setItem('endTime', response.data.endTime); | ||
| sessionStorage.setItem('examType', examData?.examType || ''); | ||
| sessionStorage.setItem('examId', id); | ||
| sessionStorage.setItem('examName', examName); |
There was a problem hiding this comment.
Fix incorrect logging and add response validation
There are several issues in the session management code:
- Console.log is logging endTime but labeled as sessionId
- No validation of response data before storage
- Missing error handling for invalid response data
Consider this enhanced implementation:
- const sessionId = response.data.sessionId;
- console.log('sessionId:', response.data.endTime);
- sessionStorage.setItem('sessionId', sessionId);
- sessionStorage.setItem('endTime', response.data.endTime);
- sessionStorage.setItem('examType', examData?.examType || '');
- sessionStorage.setItem('examId', id);
- sessionStorage.setItem('examName', examName);
+ // Validate required response data
+ const { sessionId, endTime } = response.data;
+ if (!sessionId || !endTime) {
+ throw new Error('Invalid response: missing required session data');
+ }
+
+ try {
+ // Store session data
+ sessionStorage.setItem('sessionId', sessionId);
+ sessionStorage.setItem('endTime', endTime);
+ sessionStorage.setItem('examType', examData?.examType || '');
+ sessionStorage.setItem('examId', id);
+ sessionStorage.setItem('examName', examName);
+ } catch (error) {
+ console.error('Failed to store session data:', error);
+ // Cleanup any partially stored data
+ ['sessionId', 'endTime', 'examType', 'examId', 'examName'].forEach(key =>
+ sessionStorage.removeItem(key)
+ );
+ throw new Error('Failed to store session data');
+ }📝 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.
| const sessionId = response.data.sessionId; | |
| console.log('sessionId:', response.data.endTime); | |
| sessionStorage.setItem('sessionId', sessionId); | |
| sessionStorage.setItem('endTime', response.data.endTime); | |
| sessionStorage.setItem('examType', examData?.examType || ''); | |
| sessionStorage.setItem('examId', id); | |
| sessionStorage.setItem('examName', examName); | |
| // Validate required response data | |
| const { sessionId, endTime } = response.data; | |
| if (!sessionId || !endTime) { | |
| throw new Error('Invalid response: missing required session data'); | |
| } | |
| try { | |
| // Store session data | |
| sessionStorage.setItem('sessionId', sessionId); | |
| sessionStorage.setItem('endTime', endTime); | |
| sessionStorage.setItem('examType', examData?.examType || ''); | |
| sessionStorage.setItem('examId', id); | |
| sessionStorage.setItem('examName', examName); | |
| } catch (error) { | |
| console.error('Failed to store session data:', error); | |
| // Cleanup any partially stored data | |
| ['sessionId', 'endTime', 'examType', 'examId', 'examName'].forEach(key => | |
| sessionStorage.removeItem(key) | |
| ); | |
| throw new Error('Failed to store session data'); | |
| } |
Summary by CodeRabbit
Release Notes
New Features
PreventBackButtoncomponent to prevent users from navigating back in browser history.TimeContainer.CandidateDashboardwith real-time checks for ongoing exam sessions.ExamMcqResults.ExamSummaryto better manage session data and error handling.ExamView.Bug Fixes