better error handling and bugfixes for exam #7844
Conversation
WalkthroughThis pull request introduces several changes across multiple files. In the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint (1.23.1)
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. 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 s 8000 pecific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -27,7 +27,7 @@ | |||
| type="button" | |||
| class="btn btn-lg btn-success" | |||
| value="Mark as Complete" | |||
| @click="submit" | |||
| @click="() => submit(false)" | |||
There was a problem hiding this comment.
why ? i think "submit" is same as "() => submit(false)"
There was a problem hiding this comment.
no, it sends PointerEvent when it's empty. JS thinks of it as true because we don't actually check true boolean value
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
app/views/exams/components/ExamLevel.vue (1)
29-29: Consider URL encoding query parametersThe URL construction should encode query parameters to handle special characters safely.
Here's a safer implementation using
encodeURIComponent:- return `/play/level/${this.level.slug}?course=${this.level.courseId}&course-instance=${this.level.instanceId}&codeLanguage=${this.language}` + return `/play/level/${encodeURIComponent(this.level.slug)}?course=${encodeURIComponent(this.level.courseId)}&course-instance=${encodeURIComponent(this.level.instanceId)}&codeLanguage=${encodeURIComponent(this.language)}`app/views/exams/StartPage.vue (1)
119-121: Add a comment explaining the language persistenceConsider adding a comment to explain why we're restoring the user's previous language selection.
+ // Restore user's previously selected programming language if they're returning to the exam if (this.userExam?.codeLanguage) { this.codeLanguage = this.userExam.codeLanguage }app/views/exams/ProgressPage.vue (7)
30-30: Avoid using arrow functions directly in template event handlersUsing arrow functions in template event handlers creates a new function on every render, which can impact performance. Consider defining a method instead.
Apply this diff to refactor the event handler:
-<input - type="button" - class="btn btn-lg btn-success" - value="Mark as Complete" - @click="() => submit(false)" -> +<input + type="button" + class="btn btn-lg btn-success" + value="Mark as Complete" + @click="submitExam" +>And add the
submitExammethod:methods: { // ...existing methods + submitExam() { + this.submit(false); + }, // ...remaining methods }
39-39: Use ES6 module imports instead of CommonJSrequireIn Vue single-file components, it's a best practice to use ES6
importstatements rather than CommonJSrequirecalls for consistency and tree-shaking benefits.Apply this diff to update the import statement:
-const courseInstancesApi = require('../../core/api/course-instances') +import courseInstancesApi from '../../core/api/course-instances'
54-54: InitializecourseInstanceMapas an empty objectInitializing
courseInstanceMapasnullrequires additional null checks later. By initializing it as an empty object, you can simplify your code.Apply this diff:
- courseInstanceMap: null, + courseInstanceMap: {},
66-68: Simplify null checks in theproblemscomputed propertyWith
courseInstanceMapinitialized as an empty object, the null check can be simplified.Apply this diff:
- if (!this.exam || !this.courseInstanceMap) { + if (!this.exam) {
73-82: Handle missing course instances appropriatelyWhen
instanceIdis not found, an error notification is displayed, but the loop continues. Consider whether you should halt the loop or handle the missing instance differently to prevent potential issues downstream.Optionally, you can modify the code to skip processing if the instance is not found:
- return + // Optionally, halt further processing or handle differently + // For example, skip adding this level + continue
139-141: Replace nativeconfirmwith a custom modal dialog for better UXUsing the native
confirm()function can disrupt the user experience. Consider using a custom modal dialog component to provide a consistent look and feel.For example, you can use a modal component:
// Add a data property to control modal visibility data() { return { // ...existing data properties showSubmitModal: false, } }, // Modify the submit method async submit (expires) { if (!expires) { this.showSubmitModal = true return } // ...existing submit logic }, // Add methods to handle modal actions methods: { // ...existing methods confirmSubmit() { this.showSubmitModal = false this.submit(true) }, cancelSubmit() { this.showSubmitModal = false }, }, // In your template, include the modal component <modal v-if="showSubmitModal"> <!-- Modal content --> </modal>
157-158: Remove unused methodgetCourseInstanceThe method
getCourseInstance(courseId)is defined but not used in the provided code. Removing unused code improves maintainability and readability.Apply this diff:
- getCourseInstance (courseId) { - return this.courseInstanceMap[courseId] - },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/schemas/models/exam.schema.js(1 hunks)app/schemas/models/user_exam.schema.js(1 hunks)app/views/exams/ProgressPage.vue(6 hunks)app/views/exams/StartPage.vue(3 hunks)app/views/exams/components/ExamLevel.vue(1 hunks)
🔇 Additional comments (9)
app/views/exams/components/ExamLevel.vue (1)
29-29: Verify instanceId usage across the codebase
The AI summary mentions that instanceId was removed from the problems array in the exam schema. Let's verify that we're using the correct property.
✅ Verification successful
Let me try a different approach to verify the instanceId usage.
Let me try one more search with a different approach to get more context about the instanceId usage.
Based on the extensive search results, I can now provide a definitive response about the instanceId usage.
The instanceId property is being used correctly in ExamLevel.vue
The search results show that instanceId is consistently used across the codebase as a property to identify course instances, particularly in URL construction for course navigation. The usage in ExamLevel.vue follows this pattern by including it as a query parameter course-instance=${this.level.instanceId} which is the standard format used throughout the codebase.
Some key evidence:
- The exam schema still includes instanceId in its properties
- Multiple components and views use the same pattern of passing instanceId as
course-instancein URLs - The property is used consistently in both exam-related files and the broader codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how instanceId is used in exam-related files
echo "Checking instanceId usage in exam schema and related files..."
rg -A 5 "instanceId" --type vue --type js app/views/exams app/schemas/models
# Check if there are any recent changes to instanceId handling
echo "Checking recent changes to instanceId in exam components..."
ast-grep --pattern 'instanceId'
Length of output: 398
Script:
#!/bin/bash
# Check instanceId usage in exam-related files with ripgrep
echo "Checking instanceId usage in Vue files..."
rg -t js -t vue "instanceId" app/views/exams/
# Check exam schema definition
echo -e "\nChecking exam schema definition..."
rg -t js "instanceId" app/schemas/
Length of output: 389
Script:
#!/bin/bash
# Search for instanceId in all JavaScript and Vue files, ignoring case and showing context
echo "Searching for instanceId usage with context..."
rg -i "instanceId" -g "*.{js,vue}" -B 2 -A 2
# Get the exam schema content to understand the structure
echo -e "\nExam schema content:"
cat app/schemas/models/exam.schema.js
Length of output: 102423
app/schemas/models/user_exam.schema.js (2)
19-19: Please clarify validation requirements for classroomId
The property addition looks good, but we should specify whether this field is required for new user exams. Consider adding validation requirements to match the business logic.
Let's check how classroomId is used in related schemas:
✅ Verification successful
Optional classroomId field aligns with existing exam schema pattern
The classroomId field in user_exam.schema.js correctly follows the pattern established in exam.schema.js where classroomId is also defined as an optional field. This is consistent with the broader classroom integration where exams can exist both within and outside of classroom contexts.
- In
exam.schema.js,classroomIdis defined asc.objectId()without required validation - The field is part of the
languageClassroomcontext, indicating it's an optional classroom association - Other classroom-related schemas show that classroom associations are typically optional for flexibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for classroom-related validations in other schema files
rg -g "*.schema.js" "classroomId.*required|classroom.*schema" app/schemas/
Length of output: 76
Script:
#!/bin/bash
# Let's try a broader search for classroom-related schemas and fields
rg -g "*.schema.js" -i "classroom" app/schemas/
# Also search for similar objectId patterns to understand validation requirements
rg -g "*.schema.js" "c\.objectId.*required" app/schemas/
Length of output: 3715
19-19: Verify breaking changes and migration strategy
This schema change could affect existing user exams. Please ensure:
- Existing queries handle nullable classroomId
- A migration strategy exists for existing records
Let's check for existing queries that might break:
✅ Verification successful
Schema change appears safe to implement
The classroomId field is being added as an optional field (not required) in the UserExam schema, which means:
- Existing records without this field will remain valid
- The schema doesn't enforce non-null constraints
- Current usage in the codebase (from ProgressPage.vue) properly checks for existence before accessing
userExam.classroomId
No migration is needed since:
- The field is optional in the schema
- Existing queries will continue to work as the field is not required
- New records can include the field while old records remain valid
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UserExam queries that might need updates
rg "UserExam.*find|find.*UserExam" app/
Length of output: 41
Script:
#!/bin/bash
# Let's try a broader search for UserExam usage patterns
rg -i "userexam" app/ -A 2
# Also search for database operations that might reference this model
rg -i "db.*userexam|userexam.*db" app/ -A 2
# Check if there are any migration scripts
fd migration app/
# Look for model definition to understand current schema
fd -g "*user_exam*" app/
Length of output: 7563
Script:
#!/bin/bash
# Let's examine the current schema structure
cat app/schemas/models/user_exam.schema.js
# Check if classroomId is used in queries
rg "classroomId.*userexam|userexam.*classroomId" app/ -A 2
# Look for any database-related files that might contain migrations
fd -g "*database*" app/
fd -g "*migration*" app/
Length of output: 1186
app/schemas/models/exam.schema.js (1)
Line range hint 1-66: Verify the impact of removing instanceId
The removal of instanceId from the problems array could impact existing code that depends on this field.
Let's verify the usage and ensure there are no remaining references:
app/views/exams/StartPage.vue (5)
45-50: LGTM: Loading indicator implementation
Good addition of a loading indicator to provide visual feedback during exam start operation.
74-74: LGTM: Loading state management
Appropriate addition of loading state to manage async operations.
110-115: LGTM: Improved computed properties
Good improvements:
- More descriptive naming with
hasArchivedExam - Clear separation of concerns with
isExamEnded
161-161: LGTM: Consistent use of hasArchivedExam
Good update to use the renamed computed property, maintaining consistency.
128-153: Verify exam state transitions
Please verify these edge cases:
- User rapidly clicking start button (potential race condition)
- Network disconnection during exam start
- Browser navigation during exam start
Let's check for existing error handling patterns in the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about the loading state handling and navigation patterns in the exam flow:
Loading state and navigation are properly handled
The code already implements proper safeguards against the mentioned edge cases:
-
Race condition prevention:
- Loading state is properly set before async operations and cleared in both success/error paths
- The loading flag prevents multiple concurrent starts as the button would be disabled
-
Network/error handling:
- Comprehensive try-catch block with error display
- Loading state is properly reset in catch block
- Generic error fallback for undefined error messages
-
Navigation safety:
- Navigation only occurs after successful exam start
- Clear navigation pattern between start/progress/end states
- State checks prevent invalid transitions (e.g., ended exam check)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar async operations with loading states
rg -g '*.vue' 'loading\s*=\s*true' -A 10 -B 2
# Look for other exam state transition handling
rg -g '*.vue' 'navigate.*progress|navigate.*end' -A 5
Length of output: 11983
| @@ -26,7 +26,7 @@ export default { | |||
| }, | |||
| computed: { | |||
| levelLink () { | |||
| return `/play/level/${this.level.slug}?course=${this.level.courseId}&codeLanguage=${this.language}` | |||
| return `/play/level/${this.level.slug}?course=${this.level.courseId}&course-instance=${this.level.instanceId}&codeLanguage=${this.language}` | |||
There was a problem hiding this comment.
Add null check for instanceId to prevent invalid URLs
The levelLink computed property assumes this.level.instanceId will always be defined. Given that this PR focuses on error handling, we should add a safeguard.
Here's a safer implementation:
- return `/play/level/${this.level.slug}?course=${this.level.courseId}&course-instance=${this.level.instanceId}&codeLanguage=${this.language}`
+ if (!this.level.instanceId) {
+ console.error('Missing instanceId for level:', this.level)
+ return '#'
+ }
+ return `/play/level/${this.level.slug}?course=${this.level.courseId}&course-instance=${this.level.instanceId}&codeLanguage=${this.language}`📝 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.
| return `/play/level/${this.level.slug}?course=${this.level.courseId}&course-instance=${this.level.instanceId}&codeLanguage=${this.language}` | |
| if (!this.level.instanceId) { | |
| console.error('Missing instanceId for level:', this.level) | |
| 8000 | return '#' |
| } | |
| return `/play/level/${this.level.slug}?course=${this.level.courseId}&course-instance=${this.level.instanceId}&codeLanguage=${this.language}` |
| languageClassroom: { | ||
| type: 'array', | ||
| items: { | ||
| type: 'object', | ||
| properties: { | ||
| language: { | ||
| type: 'string', | ||
| }, | ||
| classroomId: c.objectId(), | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and documentation for languageClassroom property
The new languageClassroom property needs additional constraints and documentation:
- Missing field descriptions
- No validation for language values
- No required fields specified
- No uniqueness constraints
Apply this diff to improve the schema:
languageClassroom: {
type: 'array',
+ description: 'Mapping of programming languages to their respective classrooms',
items: {
type: 'object',
+ required: ['language', 'classroomId'],
+ additionalProperties: false,
properties: {
language: {
type: 'string',
+ description: 'Programming language for this classroom',
+ enum: ['python', 'javascript', 'cpp', 'java'], // adjust based on supported languages
},
classroomId: c.objectId({
+ description: 'ID of the classroom for this language',
+ links: [{ rel: 'db', href: '/db/classroom/{($)}' }]
+ }),
},
},
+ uniqueItems: true,
},📝 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.
| languageClassroom: { | |
| type: 'array', | |
| items: { | |
| type: 'object', | |
| properties: { | |
| language: { | |
| type: 'string', | |
| }, | |
| classroomId: c.objectId(), | |
| }, | |
| }, | |
| }, | |
| languageClassroom: { | |
| type: 'array', | |
| description: 'Mapping of programming languages to their respective classrooms', | |
| items: { | |
| type: 'object', | |
| required: ['language', 'classroomId'], | |
| additionalProperties: false, | |
| properties: { | |
| language: { | |
| type: 'string', | |
| description: 'Programming language for this classroom', | |
| enum: ['python', 'javascript', 'cpp', 'java'], | |
| }, | |
| classroomId: c.objectId({ | |
| description: 'ID of the classroom for this language', | |
| 8000 links: [{ rel: 'db', href: '/db/classroom/{($)}' }] | |
| }), | |
| }, | |
| }, | |
| uniqueItems: true, | |
| }, |
| this.loading = true | ||
| if (this.isExamEnded && !this.hasArchivedExam) { | ||
| noty({ | ||
| text: 'Exam has ended', | ||
| type: 'error', | ||
| timeout: 5000, | ||
| }) | ||
| this.loading = false | ||
| return | ||
| } | ||
| try { | ||
| if (this.isNewUser) { | ||
| await this.startExam({ examId: this.examId, codeLanguage: this.codeLanguage }) | ||
| } else if (this.hasArchivedExam) { | ||
| await this.startExam({ examId: this.examId, codeLanguage: this.codeLanguage, duration: this.userExam.extraDuration }) | ||
| } | ||
| } catch (err) { | ||
| noty({ | ||
| text: err?.message || 'Start exam failed', | ||
| type: 'error', | ||
| timeout: 5000, | ||
| }) | ||
| this.loading = false | ||
| return | ||
| } | ||
| this.loading = false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling robustness
While the error handling is good, it can be made more robust and DRY.
Consider these improvements:
async localStartExam () {
this.loading = true
if (this.isExamEnded && !this.hasArchivedExam) {
noty({
- text: 'Exam has ended',
+ text: this.$t('exams.exam_ended'), // Use i18n for consistency
type: 'error',
timeout: 5000,
})
this.loading = false
return
}
try {
if (this.isNewUser) {
await this.startExam({ examId: this.examId, codeLanguage: this.codeLanguage })
} else if (this.hasArchivedExam) {
await this.startExam({ examId: this.examId, codeLanguage: this.codeLanguage, duration: this.userExam.extraDuration })
}
- } catch (err) {
+ } catch (error) {
+ // Log error for debugging
+ console.error('Failed to start exam:', error)
noty({
- text: err?.message || 'Start exam failed',
+ text: this.$t('exams.start_failed', { error: error?.message || 'Unknown error' }),
type: 'error',
timeout: 5000,
})
- this.loading = false
return
+ } finally {
+ this.loading = false
}
- this.loading = false
application.router.navigate(window.location.pathname.replace(/start$/, 'progress'), { trigger: true })
}📝 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.
| this.loading = true | |
| if (this.isExamEnded && !this.hasArchivedExam) { | |
| noty({ | |
| text: 'Exam has ended', | |
| type: 'error', | |
| timeout: 5000, | |
| }) | |
| this.loading = false | |
| return | |
| } | |
| try { | |
| if (this.isNewUser) { | |
| await this.startExam({ examId: this.examId, codeLanguage: this.codeLanguage }) | |
| } else if (this.hasArchivedExam) { | |
| await this.startExam({ examId: this.examId, codeLanguage: this.codeLanguage, duration: this.userExam.extraDuration }) | |
| } | |
| } catch (err) { | |
| noty({ | |
| text: err?.message || 'Start exam failed', | |
| type: 'error', | |
| timeout: 5000, | |
| }) | |
| this.loading = false | |
| return | |
| } | |
| this.loading = false | |
| this.loading = true | |
| if (this.isExamEnded && !this.hasArchivedExam) { | |
| noty({ | |
| text: this.$t('exams.exam_ended'), // Use i18n for consistency | |
| type: 'error', | |
| timeout: 5000, | |
| }) | |
| this.loading = false | |
| return | |
| } | |
| try { | |
| if (this.isNewUser) { | |
| await this.startExam({ examId: this.examId, codeLanguage: this.codeLanguage }) | |
| } else if (this.hasArchivedExam) { | |
| await this.startExam({ examId: this.examId, codeLanguage: this.codeLanguage, duration: this.userExam.extraDuration }) | |
| } | |
| } catch (error) { | |
| // Log error for debugging | |
| console.error('Failed to start exam:', error) | |
| noty({ | |
| text: this.$t('exams.start_failed', { error: error?.message || 'Unknown error' }), | |
| type: 'error', | |
| timeout: 5000, | |
| }) | |
| return | |
| } finally { | |
| this.loading = false | |
| } | |
| application.router.navigate(window.location.pathname.replace(/start$/, 'progress'), { trigger: true }) |
| const courseInstances = await courseInstancesApi.fetchByClassroom(this.userExam.classroomId) | ||
| this.courseInstanceMap = courseInstances.reduce((acc, courseInstance) => { | ||
| acc[courseInstance.courseID] = courseInstance._id | ||
| return acc | ||
| }, {}) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for API calls in fetchCourseInstanceMap
If the API call fails or returns unexpected data, courseInstanceMap may not be properly populated, leading to errors elsewhere. Implement error handling to manage this scenario gracefully.
Apply this diff:
async fetchCourseInstanceMap () {
+ try {
const courseInstances = await courseInstancesApi.fetchByClassroom(this.userExam.classroomId)
this.courseInstanceMap = courseInstances.reduce((acc, courseInstance) => {
acc[courseInstance.courseID] = courseInstance._id
return acc
}, {})
+ } catch (error) {
+ noty({
+ text: 'Failed to fetch course instances',
+ type: 'error',
+ timeout: 5000,
+ })
+ }
}📝 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 courseInstances = await courseInstancesApi.fetchByClassroom(this.userExam.classroomId) | |
| this.courseInstanceMap = courseInstances.reduce((acc, courseInstance) => { | |
| acc[courseInstance.courseID] = courseInstance._id | |
| return acc | |
| }, {}) | |
| }, | |
| async fetchCourseInstanceMap () { | |
| try { | |
| const courseInstances = await courseInstancesApi.fetchByClassroom(this.userExam.classroomId) | |
| this.courseInstanceMap = courseInstances.reduce((acc, courseInstance) => { | |
| acc[courseInstance.courseID] = courseInstance._id | |
| return acc | |
| }, {}) | |
| } catch (error) { | |
| noty({ | |
| text: 'Failed to fetch course instances', | |
| type: 'error', | |
| timeout: 5000, | |
| }) | |
| } | |
| }, |
| async mounted () { | ||
| if (this.userExam?.submitted) { | ||
| noty({ | ||
| text: 'Exam has ended', | ||
| type: 'error', | ||
| timeout: 5000, | ||
| }) | ||
| return | ||
| } | ||
| await this.fetchCourseInstanceMap() |
There was a problem hiding this comment.
Avoid using async directly on lifecycle hooks
Vue does not natively support async on lifecycle hooks like mounted(). This can cause unexpected behavior. Instead, use an immediately invoked async function inside the lifecycle hook.
Apply this diff to refactor the mounted() hook:
- async mounted () {
+ mounted () {
+ (async () => {
if (this.userExam?.submitted) {
noty({
text: 'Exam has ended',
type: 'error',
timeout: 5000,
})
return
}
await this.fetchCourseInstanceMap()
this.counter()
const * 1000
this.counterInterval = setInterval(this.counter, oneMin)
+ })()
},Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
languageClassroomproperty to the Exam Schema for improved exam organization.classroomIdproperty to the User Exam Schema to link exams to specific classrooms.ProgressPagewith improved error handling and submission logic.StartPageto prevent multiple exam submissions and improve user feedback.Bug Fixes
Refactor