Application : Task 3 GPA Calculator OverHaul/Remade#511
Open
RishabhSinha2009 wants to merge 1 commit into
Open
Application : Task 3 GPA Calculator OverHaul/Remade#511RishabhSinha2009 wants to merge 1 commit into
RishabhSinha2009 wants to merge 1 commit into
Conversation
Contributor
WalkthroughThe GPA Calculator component underwent a major structural refactor, transitioning from a flat course array to a hierarchical year/semester/course model. GPA calculation logic was rewritten to support the new structure, persistence changed from a deep watcher to explicit save/load operations, and support for full-year linked courses was added with automatic input normalization and level detection. Changes
Sequence DiagramsequenceDiagram
participant User
participant Component as GPA Calculator
participant Calc as Calculation Engine
participant Storage as Local Storage
User->>Component: Update course (name/grade/level/weight)
Component->>Component: normalizeGrade() & normalizeLevel()
alt Full-Year Course?
Component->>Component: syncLinkedCourses()<br/>(update paired semester)
end
Component->>Calc: calculateAll()
Calc->>Calc: Iterate years→semesters→courses
Calc->>Calc: Recompute weighted/unweighted GPA
Calc->>Component: Return updated hierarchy
Component->>Component: Update derived fields<br/>(finalGrade, GPA values)
Component->>Storage: saveData() to SHS_PLANNER_DATA
Storage->>Storage: Persist with schema validation
Component->>User: UI re-renders with new values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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 (4)
src/views/GpaCalculator/GpaCalculator.vue (4)
466-496: Plain objects cast as Course won't be class instances.The loaded course data is a plain object cast with
as Course, not an actualCourseclass instance. Currently this works becauseCourseonly has properties, but if methods are added to the class later, they won't be available on loaded data.Additionally, lines 483-490 preserve
unweightedGPA,weightedGPA, andfinalGradefrom storage, butcalculateAll()at line 508 immediately recalculates and overwrites these values, making the preservation redundant.Consider using createCourse or Course constructor
- ? sem.courses.map((course: any) => - ({ - instanceId: - typeof course.instanceId === 'string' - ? course.instanceId - : this.createId(), - linkedId: - typeof course.linkedId === 'string' - ? course.linkedId - : null, - name: typeof course.name === 'string' ? course.name : '', - grade: this.normalizeGrade(course.grade), - level: this.normalizeLevel(course.level), - weight: - course.weight === 1.5 || course.weight === 1.0 - ? course.weight - : 1.0, - unweightedGPA: - typeof course.unweightedGPA === 'number' - ? course.unweightedGPA - : 4.0, - weightedGPA: - typeof course.weightedGPA === 'number' - ? course.weightedGPA - : 4.0, - finalGrade: - typeof course.finalGrade === 'string' - ? course.finalGrade - : 'A', - } as Course)) + ? sem.courses.map((course: any) => { + const c = this.createCourse( + typeof course.name === 'string' ? course.name : '', + typeof course.linkedId === 'string' ? course.linkedId : null + ); + c.instanceId = typeof course.instanceId === 'string' + ? course.instanceId + : c.instanceId; + c.grade = this.normalizeGrade(course.grade); + c.level = this.normalizeLevel(course.level); + c.weight = course.weight === 1.5 ? 1.5 : 1.0; + return c; + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/GpaCalculator/GpaCalculator.vue` around lines 466 - 496, The loaded course objects are plain objects cast as Course which won't provide class methods later and you also preserve unneeded fields (unweightedGPA, weightedGPA, finalGrade) that are immediately recomputed by calculateAll; instead, construct real Course instances via the existing factory/constructor (use createCourse(...) or new Course(...)) passing instanceId (generate with this.createId() when missing), linkedId, name, normalized grade/level (use this.normalizeGrade/normalizeLevel) and weight, and omit loading stored GPA/finalGrade values so calculateAll can recompute them; update the mapping that builds courses to return created Course instances rather than plain objects.
967-973: Hardcodedbackground: whitemay not support theming.Several places use hardcoded
white(lines 971, 1042, 1115) instead of CSS variables likevar(--background). If the application supports or plans to support dark mode or theming, consider using CSS variables for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/GpaCalculator/GpaCalculator.vue` around lines 967 - 973, The CSS uses hardcoded "background: white" (e.g., in the .year-block rule and two other selectors around the same area) which breaks theming; update those rules to use a CSS variable like "background: var(--background, white)" or a semantic token such as "var(--card-background, white)" so dark mode and custom themes inherit correctly—locate the declarations setting "background: white" (including the .year-block rule and the other selectors at the referenced spots) and replace the literal with the chosen var() fallback form.
385-388: Inconsistent case handling for "AP" detection.The AP detection uses the original-case
trimmedstring while other level keywords use theloweredstring. This means "ap chemistry" won't be detected, only "AP Chemistry".Consider using case-insensitive matching for consistency:
- if (/\bhonors\b/.test(lowered) || /\bhonor\b/.test(lowered) || /\bAP\b/.test(trimmed)) + if (/\bhonors\b/.test(lowered) || /\bhonor\b/.test(lowered) || /\bap\b/i.test(lowered))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/GpaCalculator/GpaCalculator.vue` around lines 385 - 388, The AP check is using the original-case variable `trimmed` while other checks use `lowered`, causing "ap chemistry" to miss; update the AP detection in the same if block that references `lowered` and `trimmed` to use a case-insensitive test (either test `lowered` for `\bap\b` or use a case-insensitive regex like `/\bAP\b/i`) so AP is detected regardless of case; modify the condition in the GpaCalculator component where `if (/\bhonors\b/.test(lowered) || /\bhonor\b/.test(lowered) || /\bAP\b/.test(trimmed))` appears to use one of these two consistent approaches.
174-177: Duplicate ID generation logic.The
Courseconstructor (line 176) andcreateId()method (line 294) use identical logic for generating IDs. Consider having the constructor callcreateId()or making ID generation a static method.Refactor suggestion
constructor(name = '', linkedId: string | null = null) { - this.instanceId = Math.random().toString(36).substring(2, 11); + this.instanceId = ''; // Will be set by createCourse this.linkedId = linkedId;Then in
createCourse:createCourse(name = '', linkedId: string | null = null): Course { - return new Course(name, linkedId); + const course = new Course(name, linkedId); + course.instanceId = this.createId(); + return course; }Also applies to: 292-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/GpaCalculator/GpaCalculator.vue` around lines 174 - 177, The Course constructor currently duplicates the ID generation logic found in createId(); refactor so ID generation is centralized: either have the constructor call the existing createId() method (e.g., replace the inline Math.random... with this.instanceId = this.createId()) or extract the logic into a static helper (e.g., Course.generateId()) and use that from both the constructor and createCourse; update createCourse to use the same centralized generator (createId() or the new static) and remove the duplicated Math.random... logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/views/GpaCalculator/GpaCalculator.vue`:
- Around line 767-800: removeCourse currently only removes the specified course,
leaving its full-year partner orphaned when course.linkedId is set; update
removeCourse to, after finding and splicing the target course from
semester.courses, check if course.linkedId exists and if so locate the linked
course by instanceId === course.linkedId across the other semester (the
companion semester in the same year), remove that linked course as well, and
then call renumberSemesterCourses for both semesters (use
renumberSemesterCourses(year, sIdx) and renumberSemesterCourses(year,
otherSIdx)) before calling calculateAll so numbering stays correct and no
orphaned linked courses remain; ensure you still handle missing year/semester
and indexes as current code does.
- Around line 36-38: The UI text inside the <div class="edit-note"> contains a
Unicode replacement character (�); open the template where the edit-note div is
rendered (GpaCalculator.vue) and replace the corrupted characters with the
intended punctuation (e.g., em-dash — or explicit bullets "•" or separate
sentences) so the sentence reads cleanly (e.g., "Course names are editable and
can automatically set credit level from the name — full year course is different
from a one semester course — matches Infinite Campus layout"); save the file as
UTF-8 to prevent reintroducing encoding corruption.
---
Nitpick comments:
In `@src/views/GpaCalculator/GpaCalculator.vue`:
- Around line 466-496: The loaded course objects are plain objects cast as
Course which won't provide class methods later and you also preserve unneeded
fields (unweightedGPA, weightedGPA, finalGrade) that are immediately recomputed
by calculateAll; instead, construct real Course instances via the existing
factory/constructor (use createCourse(...) or new Course(...)) passing
instanceId (generate with this.createId() when missing), linkedId, name,
normalized grade/level (use this.normalizeGrade/normalizeLevel) and weight, and
omit loading stored GPA/finalGrade values so calculateAll can recompute them;
update the mapping that builds courses to return created Course instances rather
than plain objects.
- Around line 967-973: The CSS uses hardcoded "background: white" (e.g., in the
.year-block rule and two other selectors around the same area) which breaks
theming; update those rules to use a CSS variable like "background:
var(--background, white)" or a semantic token such as "var(--card-background,
white)" so dark mode and custom themes inherit correctly—locate the declarations
setting "background: white" (including the .year-block rule and the other
selectors at the referenced spots) and replace the literal with the chosen var()
fallback form.
- Around line 385-388: The AP check is using the original-case variable
`trimmed` while other checks use `lowered`, causing "ap chemistry" to miss;
update the AP detection in the same if block that references `lowered` and
`trimmed` to use a case-insensitive test (either test `lowered` for `\bap\b` or
use a case-insensitive regex like `/\bAP\b/i`) so AP is detected regardless of
case; modify the condition in the GpaCalculator component where `if
(/\bhonors\b/.test(lowered) || /\bhonor\b/.test(lowered) ||
/\bAP\b/.test(trimmed))` appears to use one of these two consistent approaches.
- Around line 174-177: The Course constructor currently duplicates the ID
generation logic found in createId(); refactor so ID generation is centralized:
either have the constructor call the existing createId() method (e.g., replace
the inline Math.random... with this.instanceId = this.createId()) or extract the
logic into a static helper (e.g., Course.generateId()) and use that from both
the constructor and createCourse; update createCourse to use the same
centralized generator (createId() or the new static) and remove the duplicated
Math.random... logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b50215c5-57df-4983-aeb6-4d837199f63c
📒 Files selected for processing (1)
src/views/GpaCalculator/GpaCalculator.vue
Comment on lines
+36
to
+38
| <div class="edit-note"> | ||
| Course names are editable and can automatically set credit level from the name � Full year course is different from a one semester course � Matches Infinite Campus Layout | ||
| </div> |
Contributor
There was a problem hiding this comment.
Corrupted character in UI text.
Line 37 contains a Unicode replacement character (�) which likely indicates encoding corruption. The intended character (possibly an em-dash or bullet) was not preserved.
- Course names are editable and can automatically set credit level from the name � Full year course is different from a one semester course � Matches Infinite Campus Layout
+ Course names are editable and can automatically set credit level from the name • Full year course is different from a one semester course • Matches Infinite Campus Layout📝 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.
Suggested change
| <div class="edit-note"> | |
| Course names are editable and can automatically set credit level from the name � Full year course is different from a one semester course � Matches Infinite Campus Layout | |
| </div> | |
| <div class="edit-note"> | |
| Course names are editable and can automatically set credit level from the name • Full year course is different from a one semester course • Matches Infinite Campus Layout | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/GpaCalculator/GpaCalculator.vue` around lines 36 - 38, The UI text
inside the <div class="edit-note"> contains a Unicode replacement character (�);
open the template where the edit-note div is rendered (GpaCalculator.vue) and
replace the corrupted characters with the intended punctuation (e.g., em-dash —
or explicit bullets "•" or separate sentences) so the sentence reads cleanly
(e.g., "Course names are editable and can automatically set credit level from
the name — full year course is different from a one semester course — matches
Infinite Campus layout"); save the file as UTF-8 to prevent reintroducing
encoding corruption.
Comment on lines
+767
to
+800
| removeCourse(yIdx: number, sIdx: number, course: Course) | ||
| { | ||
| const year = this.years[yIdx]; | ||
|
|
||
| if (!year) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| const semester = year.semesters[sIdx]; | ||
|
|
||
| if (!semester) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| const index = semester.courses.findIndex( | ||
| (c) => c.instanceId === course.instanceId | ||
| ); | ||
|
|
||
| if (index === -1) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| semester.courses.splice(index, 1); | ||
|
|
||
| if (!course.linkedId) | ||
| { | ||
| this.renumberSemesterCourses(year, sIdx); | ||
| } | ||
|
|
||
| this.calculateAll(); | ||
| }, |
Contributor
There was a problem hiding this comment.
Removing a full-year course orphans its linked pair.
When a course with a linkedId is removed, the linked course in the other semester is not removed. This leaves an orphaned course with a linkedId pointing to nothing, breaking the full-year pairing behavior.
Consider also removing the linked course:
Proposed fix to remove linked course
semester.courses.splice(index, 1);
- if (!course.linkedId)
- {
- this.renumberSemesterCourses(year, sIdx);
- }
+ if (course.linkedId)
+ {
+ // Remove linked course from other semesters
+ year.semesters.forEach((sem, semIdx) => {
+ if (semIdx !== sIdx) {
+ const linkedIdx = sem.courses.findIndex(c => c.linkedId === course.linkedId);
+ if (linkedIdx !== -1) {
+ sem.courses.splice(linkedIdx, 1);
+ this.renumberSemesterCourses(year, semIdx);
+ }
+ }
+ });
+ }
+
+ this.renumberSemesterCourses(year, sIdx);
this.calculateAll();📝 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.
Suggested change
| removeCourse(yIdx: number, sIdx: number, course: Course) | |
| { | |
| const year = this.years[yIdx]; | |
| if (!year) | |
| { | |
| return; | |
| } | |
| const semester = year.semesters[sIdx]; | |
| if (!semester) | |
| { | |
| return; | |
| } | |
| const index = semester.courses.findIndex( | |
| (c) => c.instanceId === course.instanceId | |
| ); | |
| if (index === -1) | |
| { | |
| return; | |
| } | |
| semester.courses.splice(index, 1); | |
| if (!course.linkedId) | |
| { | |
| this.renumberSemesterCourses(year, sIdx); | |
| } | |
| this.calculateAll(); | |
| }, | |
| removeCourse(yIdx: number, sIdx: number, course: Course) | |
| { | |
| const year = this.years[yIdx]; | |
| if (!year) | |
| { | |
| return; | |
| } | |
| const semester = year.semesters[sIdx]; | |
| if (!semester) | |
| { | |
| return; | |
| } | |
| const index = semester.courses.findIndex( | |
| (c) => c.instanceId === course.instanceId | |
| ); | |
| if (index === -1) | |
| { | |
| return; | |
| } | |
| semester.courses.splice(index, 1); | |
| if (course.linkedId) | |
| { | |
| // Remove linked course from other semesters | |
| year.semesters.forEach((sem, semIdx) => { | |
| if (semIdx !== sIdx) { | |
| const linkedIdx = sem.courses.findIndex(c => c.linkedId === course.linkedId); | |
| if (linkedIdx !== -1) { | |
| sem.courses.splice(linkedIdx, 1); | |
| this.renumberSemesterCourses(year, semIdx); | |
| } | |
| } | |
| }); | |
| } | |
| this.renumberSemesterCourses(year, sIdx); | |
| this.calculateAll(); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/GpaCalculator/GpaCalculator.vue` around lines 767 - 800,
removeCourse currently only removes the specified course, leaving its full-year
partner orphaned when course.linkedId is set; update removeCourse to, after
finding and splicing the target course from semester.courses, check if
course.linkedId exists and if so locate the linked course by instanceId ===
course.linkedId across the other semester (the companion semester in the same
year), remove that linked course as well, and then call renumberSemesterCourses
for both semesters (use renumberSemesterCourses(year, sIdx) and
renumberSemesterCourses(year, otherSIdx)) before calling calculateAll so
numbering stays correct and no orphaned linked courses remain; ensure you still
handle missing year/semester and indexes as current code does.
Contributor
|
From application review: Open-Ended Task
Evaluation
|
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.
Proper Description is on my Application : rsinha27@students.d125.org.
Summary by CodeRabbit
New Features
Refactor