PR 6: refactor: per-area small refactors (CTS, Scheduler, Students, CMS)#179
Open
rlorenzo wants to merge 4 commits into
Open
PR 6: refactor: per-area small refactors (CTS, Scheduler, Students, CMS)#179rlorenzo wants to merge 4 commits into
rlorenzo wants to merge 4 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds IScheduleEntity and ApplyScheduleFilters; refactors Codecs to parameterized EncodeWithMap/DecodeWithMap and adds UU tests; consolidates StudentGroupService photo/Ross IAM logic; converts CourseStudents.vue table to data-driven rendering. ChangesSchedule Filtering Abstraction
Codec Parameterization and Testing
CourseStudents Table Data-Driven Rendering
StudentGroupService Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
25b942b to
d0b2e7f
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
ebe32ec to
75abeb2
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
d0b2e7f to
0ea0117
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
5 times, most recently
from
d26c5d8 to
8cbfb69
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
0ea0117 to
0fa6909
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
2 times, most recently
from
0d62a4e to
a0c55c4
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
0fa6909 to
5cbd5f4
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
a0c55c4 to
d8a3735
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
5cbd5f4 to
8eb247a
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
d8a3735 to
ddd64e7
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
8eb247a to
a461dcd
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
ddd64e7 to
66fe537
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
a461dcd to
6d0722d
Compare
3 tasks
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
66fe537 to
d09da71
Compare
The three repeated prototype rows now iterate over a single mock array, which is how the real implementation will pull the API response.
Add an IScheduleEntity interface implemented by InstructorSchedule and StudentSchedule, then route both services through a generic ApplyScheduleFilters extension instead of duplicating the rotation / service / week / date filter clauses.
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
6d0722d to
9c30e16
Compare
Bundle ReportChanges will decrease total bundle size by 881 bytes (-0.04%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: viper-frontend-esmAssets Changed:
Files in
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 43.02% 43.46% +0.43%
==========================================
Files 881 882 +1
Lines 51436 51226 -210
Branches 4812 4779 -33
==========================================
+ Hits 22131 22264 +133
+ Misses 28779 28436 -343
Partials 526 526
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
9c30e16 to
743f4e5
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
743f4e5 to
b8152a2
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/Areas/Students/Services/StudentGroupService.cs (1)
407-433: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd
.AsNoTracking()to read-only query.Same issue. Insert
.AsNoTracking()before.OrderBy().⚡ Suggested fix
sg != null ? sg.StudentgrpV3grp : null); // Apply group filtering if specified if (!string.IsNullOrEmpty(groupType) && !string.IsNullOrEmpty(groupId)) { query = groupType.ToLower() switch { "eighths" => query.Where(s => s.EighthsGroup == groupId), "twentieths" => query.Where(s => s.TwentiethsGroup == groupId), "teams" => query.Where(s => s.TeamNumber == groupId), "v3specialty" => query.Where(s => s.V3SpecialtyGroup == groupId), _ => query }; } -var students = await query.OrderBy(s => s.LastName).ThenBy(s => s.FirstName).ToListAsync(); +var students = await query.AsNoTracking().OrderBy(s => s.LastName).ThenBy(s => s.FirstName).ToListAsync();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/Students/Services/StudentGroupService.cs` around lines 407 - 433, The LINQ query building a sequence of StudentBaseRecord is read-only and should use AsNoTracking to avoid EF change tracking overhead; update the code that constructs "query" (the projection creating new StudentBaseRecord in StudentGroupService) to call .AsNoTracking() on the IQueryable before applying .OrderBy(...).ThenBy(...)/ToListAsync(), preserving any groupType filtering logic (the switch) so call .AsNoTracking() on the final query instance just prior to ordering and materialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/CMS/CodecsTests.cs`:
- Line 4: The namespace declared as Test.CMS doesn't match the project's
expected root namespace; update the namespace declaration to Viper.test.CMS
(replace the Test.CMS namespace token) so the class/test types in this file use
the Viper.test.CMS namespace and satisfy the ReSharper/CI checks.
In `@web/Areas/Students/Services/StudentGroupService.cs`:
- Around line 765-772: The query against _sisContext.StudentDesignations in
StudentGroupService should be executed as a read-only query; insert
.AsNoTracking() immediately after _sisContext.StudentDesignations (before the
existing .Where(...) calls) so the EF Core change tracker is not used for this
read-only projection (retain the rest of the chain:
.Where(...).Select(...).Where(...).Distinct().ToListAsync()).
- Line 738: Remove the null-forgiving operator from student.MailId in
StudentGroupService.cs and handle nullable MailId properly: either filter the
students iteration to only include those with MailId != null before mapping, or
assign MailId = student.MailId (no !) and ensure the consumer (e.g., the
StudentPhoto model or downstream code) accepts a nullable MailId; locate the
mapping where MailId = student.MailId! and replace it with a safe nullable-aware
approach consistent with how MailId is handled elsewhere (see other assignments
around lines with StudentPhoto.MailId).
- Around line 113-126: The LINQ query building StudentBaseRecord instances is
read-only but currently tracks entities; update the query used in
StudentGroupService (the variable named query that projects to
StudentBaseRecord) to call .AsNoTracking() before the OrderBy/ThenBy and
ToListAsync so EF Core doesn't enable change tracking for this read-only
projection; locate the projection creating new StudentBaseRecord and insert
.AsNoTracking() on that IQueryable prior to .OrderBy(s => s.LastName).ThenBy(s
=> s.FirstName).ToListAsync().
- Around line 341-354: The query projection creating StudentBaseRecord from
queryBase should be treated as read-only—modify the LINQ chain so that the
IQueryable returned by the Select (the variable named query) uses AsNoTracking()
before applying OrderBy/ThenBy; locate the Select(...) producing
StudentBaseRecord in StudentGroupService (the variable query) and insert
.AsNoTracking() on that query before calling OrderBy(s => s.LastName).ThenBy(s
=> s.FirstName).ToListAsync().
---
Outside diff comments:
In `@web/Areas/Students/Services/StudentGroupService.cs`:
- Around line 407-433: The LINQ query building a sequence of StudentBaseRecord
is read-only and should use AsNoTracking to avoid EF change tracking overhead;
update the code that constructs "query" (the projection creating new
StudentBaseRecord in StudentGroupService) to call .AsNoTracking() on the
IQueryable before applying .OrderBy(...).ThenBy(...)/ToListAsync(), preserving
any groupType filtering logic (the switch) so call .AsNoTracking() on the final
query instance just prior to ordering and materialization.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d79bf74a-c6b5-4d4d-84dc-7f186950c9ab
📒 Files selected for processing (10)
VueApp/src/CTS/pages/CourseStudents.vuetest/CMS/CodecsTests.csweb/Areas/CMS/Data/Codecs.csweb/Areas/ClinicalScheduler/Services/InstructorScheduleService.csweb/Areas/ClinicalScheduler/Services/ScheduleQueryExtensions.csweb/Areas/ClinicalScheduler/Services/StudentScheduleService.csweb/Areas/Students/Services/StudentGroupService.csweb/Models/CTS/IScheduleEntity.csweb/Models/CTS/InstructorSchedule.csweb/Models/CTS/StudentSchedule.cs
rlorenzo
force-pushed
the
refactor/per-area-small
branch
2 times, most recently
from
ec53a80 to
bc09f90
Compare
There was a problem hiding this comment.
Pull request overview
This PR (part 6/6 of the refactor stack) applies several small, per-area dedup/refactors across CTS, Clinical Scheduler, Students, and CMS—primarily consolidating repeated logic and adding CMS UU codec test coverage.
Changes:
- Clinical Scheduler: introduce
IScheduleEntity+ScheduleQueryExtensionsto share schedule filter LINQ across instructor/student schedule queries. - Students: refactor
StudentGroupServiceto share common photo-building and Ross-ID lookup logic. - CMS: collapse UU encode/decode onto shared helpers, remove XX variants, and add
CodecsTests. - CTS Vue: replace repeated hard-coded
<tr>rows with a singlev-for.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/Models/CTS/StudentSchedule.cs | Implements IScheduleEntity to enable shared schedule filtering extensions. |
| web/Models/CTS/IScheduleEntity.cs | New interface defining common filterable schedule fields. |
| web/Models/CTS/InstructorSchedule.cs | Implements IScheduleEntity to enable shared schedule filtering extensions. |
| web/Areas/ClinicalScheduler/Services/ScheduleQueryExtensions.cs | New shared LINQ filter extension for schedule queries. |
| web/Areas/ClinicalScheduler/Services/StudentScheduleService.cs | Uses shared ApplyScheduleFilters instead of duplicated filter logic. |
| web/Areas/ClinicalScheduler/Services/InstructorScheduleService.cs | Uses shared ApplyScheduleFilters instead of duplicated filter logic. |
| web/Areas/Students/Services/StudentGroupService.cs | Dedups photo-building + Ross IAM ID fetching into helpers and applies AsNoTracking(). |
| web/Areas/CMS/Data/Codecs.cs | Refactors UU encode/decode to shared helpers and removes XX variants. |
| test/CMS/CodecsTests.cs | Adds automated coverage for UU encode/decode round-trip + canonical wire format vectors. |
| VueApp/src/CTS/pages/CourseStudents.vue | Replaces repeated hard-coded table rows with a v-for over mock data. |
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
bc09f90 to
d1b9d4c
Compare
Introduce the StudentBaseRecord projection shape, BuildStudentPhotoListAsync, and GetActiveRossIamIdsAsync so the by-class-level / by-group / by-course methods no longer hand-roll the same photo lookup, group-assignment formatting, and Ross-IamIds pre-query.
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
d1b9d4c to
b06de1c
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Collapse the UU encoder/decoder onto shared EncodeWithMap/DecodeWithMap helpers and drop the unused XX variants and their maps. ColdFusion only ever uses the default UU encoding, so XX was dead in both the legacy app and here. UUEncode has no caller yet but is kept for the CMS migration, where new files must store their AES key UU-encoded so legacy CF can decrypt them during the parallel-run period. Add CodecsTests covering UU round-trips, the canonical "Cat"/"#0V%T" wire format, empty input, and null-argument guards.
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
b06de1c to
77a1736
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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
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.
Summary
Part 6 of 6. Stacks on top of PR #177 (parallel to PR #178). The smallest PR in the stack: 10 files, 4 self-contained refactors.
Each commit is a small per-area dedup driven by jscpd findings on the original
code-anaylsisbranch.Commits
refactor(cts): collapse hardcoded CourseStudents rows into a v-for: replace 5 near-identical<tr>blocks with a singlev-for.refactor(scheduler): share schedule filter LINQ via IScheduleEntity: introduceIScheduleEntity+ScheduleQueryExtensionsso the by-clinician / by-rotation / by-week scheduling queries share their filter LINQ.refactor(students): consolidate StudentGroupService photo + Ross helpers: extractBuildStudentPhotoListAsync,FormatStudentDisplayName,FormatGroupAssignment,ResolvePhotoUrl, andGetActiveRossIamIdsAsyncso the by-class-level / by-group / by-course paths no longer hand-roll the same logic.refactor(cms): reduce Codecs to UU-only and cover with tests: collapse the UU encode/decode methods onto shared helpers (DecodeWithMap,EncodeWithMap), delete the unused XX variants and their maps (ColdFusion only ever uses the default UU encoding, so XX was dead in both the legacy app and here), and addCodecsTests.UUEncodehas no caller yet but is kept for the CMS migration, where new files must store their AES key UU-encoded so legacy CF can decrypt them during the parallel-run period.Conflict resolution notes
Two cherry-picks needed conflict resolution because PR 3's analyzer cleanup ran ahead of these refactors in the original chronology, and the analyzer fixes touched the pre-refactor code shape:
StudentGroupService.cs: took the refactor's structure, then re-applied PR 3's analyzer fixes to the new helper (SqlExceptionshort qualifier,!string.IsNullOrEmpty(id)overid != null,sealed record StudentBaseRecord).Codecs.cs: took the refactor's consolidated structure withStreamqualifiers shortened (the file's implicit-usings setup makesusing System.IO;unnecessary).Net effect: each file ends up at the same end-state as on the original
code-anaylsisbranch, exceptCodecs.cs. It was taken further after review: the unused XX variants and maps were deleted (leaving UU only) andCodecsTestswas added, so it intentionally diverges from the original branch shape (see the CMS commit above).PR stack
Test plan
CodecsTests, run in CI); XX path removed