PR 5: refactor(effort): component extraction + report context centralization#178
Open
rlorenzo wants to merge 14 commits into
Open
PR 5: refactor(effort): component extraction + report context centralization#178rlorenzo wants to merge 14 commits into
rlorenzo wants to merge 14 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughPR extracts reusable Vue component primitives (\ ChangesVue Component Extraction & Refactoring
Backend Service Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
5 tasks
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
eee8d10 to
8f5ffd2
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
ebe32ec to
75abeb2
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
8f5ffd2 to
9cad656
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
4 times, most recently
from
d26c5d8 to
8cbfb69
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
9cad656 to
d14e7b9
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/effort-component-extraction
branch
from
d14e7b9 to
94b732b
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
a0c55c4 to
d8a3735
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
94b732b to
61ee064
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
d8a3735 to
ddd64e7
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
61ee064 to
f53d1a9
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
ddd64e7 to
66fe537
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
f53d1a9 to
9518c20
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview 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: 6
🤖 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 `@VueApp/src/Effort/components/AsyncOperationDialog.vue`:
- Line 45: Remove the inline style attributes on the QCard and the element at
the top-right: replace the :style binding on the q-card (the element using
`:style="`width: 100%; max-width: ${maxWidth}; position: relative`"`) with a
class (e.g., `async-op-card`) and move the top-right element's inline z-index
style into a class (e.g., `absolute-top-right`); then add those CSS rules in the
component's <style scoped> block (set width/ max-width/position rules for
`.async-op-card` and z-index/positioning for `.absolute-top-right`) and update
the template to use the new class names or Quasar utility classes instead of
inline styles.
In `@VueApp/src/Effort/components/ClinicalImportDialog.vue`:
- Around line 151-156: The Confirm Import QBtn in ClinicalImportDialog.vue (the
<q-btn> with label="Confirm Import", color="info", :disable="totalChanges === 0
|| isCommitting", `@click`="confirmImport") needs the text-color="dark" attribute
added to satisfy the UI guidelines and ensure sufficient contrast on the info
background; update that <q-btn> to include text-color="dark" (and mirror the
same change for any other info/warning QBtn instances in this component if
present).
In `@VueApp/src/Effort/components/EffortReportPage.vue`:
- Around line 20-21: Replace the callback-prop pattern with Vue events: remove
the onPdfExport and onExcelExport props from ExportToolbar usage and instead
have ExportToolbar emit "export-pdf" and "export-excel"; update
EffortReportPage.vue to listen for those emits (e.g., <ExportToolbar
`@export-pdf`="handlePdfExport" `@export-excel`="handleExcelExport">) and rework any
prop passthrough so EffortReportPage emits its own events
(this.$emit('export-pdf') / this.$emit('export-excel') or calls local handlers)
rather than passing functions down; ensure you update the component's emits
option and any parent pages to use `@export-pdf` and `@export-excel` instead of
:on-pdf-export/:on-excel-export.
In `@VueApp/src/Effort/components/HarvestDialog.vue`:
- Around line 198-205: Replace the raw q-badge usage inside each
body-cell-status template with the project's StatusBadge component so accessible
text-color logic is preserved: for each template (e.g., the one using
getStatusColor(slotProps.value)) render StatusBadge and pass the status value
and the color computed by getStatusColor(slotProps.value) instead of q-badge,
and restore/ensure the StatusBadge import at the top of the file; apply the same
replacement to all eight body-cell-status sites referenced so they consistently
use StatusBadge rather than q-badge.
In `@VueApp/src/Effort/components/PercentAssignmentFormFields.vue`:
- Around line 67-80: The q-select for Unit (v-model="form.unitId") is marked
required via label "Unit *" and the rule requiredRule('Unit') but still includes
the clearable prop; remove the clearable attribute from the q-select element in
PercentAssignmentFormFields.vue so users cannot clear the required Unit field
and validation remains consistent.
In `@VueApp/src/Effort/components/TermTable.vue`:
- Line 18: The title is rendered twice in TermTable.vue (the <h2 class="text-h6
q-mb-sm q-mt-none">{{ title }}</h2> and the separate title inside the lt-sm
section), causing duplicate display on mobile; fix by either adding the
responsive class gt-xs to the existing <h2> so it hides on extra-small screens,
or remove the duplicate subtitle inside the lt-sm block so the single <h2>
remains; update the element that currently prints {{ title }} inside the lt-sm
section (or add gt-xs to the <h2>) accordingly to ensure only one title is shown
on mobile.
🪄 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: 01234b4d-9fac-4c89-90f3-55c6ec94e220
📒 Files selected for processing (43)
VueApp/src/Effort/components/AddCourseEffortDialog.vueVueApp/src/Effort/components/AsyncOperationDialog.vueVueApp/src/Effort/components/ClinicalImportDialog.vueVueApp/src/Effort/components/DialogSubmitActions.vueVueApp/src/Effort/components/EffortRecordAddDialog.vueVueApp/src/Effort/components/EffortRecordEditDialog.vueVueApp/src/Effort/components/EffortRecordSharedFields.vueVueApp/src/Effort/components/EffortReportPage.vueVueApp/src/Effort/components/HarvestDialog.vueVueApp/src/Effort/components/InstructorPageShell.vueVueApp/src/Effort/components/PercentAssignmentAddDialog.vueVueApp/src/Effort/components/PercentAssignmentEditDialog.vueVueApp/src/Effort/components/PercentAssignmentFormFields.vueVueApp/src/Effort/components/PercentRolloverDialog.vueVueApp/src/Effort/components/TermTable.vueVueApp/src/Effort/composables/use-percentage-form.tsVueApp/src/Effort/pages/DeptSummary.vueVueApp/src/Effort/pages/EvalDetail.vueVueApp/src/Effort/pages/EvalSummary.vueVueApp/src/Effort/pages/InstructorDetail.vueVueApp/src/Effort/pages/InstructorEdit.vueVueApp/src/Effort/pages/MeritAverage.vueVueApp/src/Effort/pages/MeritDetail.vueVueApp/src/Effort/pages/MeritSummary.vueVueApp/src/Effort/pages/SchoolSummary.vueVueApp/src/Effort/pages/TeachingActivityGrouped.vueVueApp/src/Effort/pages/TeachingActivityIndividual.vueVueApp/src/Effort/pages/TermSelection.vueVueApp/src/Students/EmergencyContact/components/EmergencyContactPageShell.vueVueApp/src/Students/EmergencyContact/pages/EmergencyContactForm.vueVueApp/src/Students/EmergencyContact/pages/EmergencyContactView.vuetest/Effort/BaseReportServiceTests.cstest/Effort/ExcelGenerationTests.csweb/Areas/Effort/Services/BaseReportService.csweb/Areas/Effort/Services/ClinicalEffortService.csweb/Areas/Effort/Services/ClinicalScheduleService.csweb/Areas/Effort/Services/DeptSummaryService.csweb/Areas/Effort/Services/EvaluationReportService.csweb/Areas/Effort/Services/MeritMultiYearService.csweb/Areas/Effort/Services/MeritReportService.csweb/Areas/Effort/Services/MeritSummaryService.csweb/Areas/Effort/Services/SchoolSummaryService.csweb/Areas/Effort/Services/TeachingActivityService.cs
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
66fe537 to
d09da71
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
9518c20 to
53f2c8a
Compare
Bundle ReportChanges will decrease total bundle size by 7.12kB (-0.33%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: viper-frontend-esmAssets Changed:
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
+ Coverage 43.02% 43.05% +0.02%
==========================================
Files 881 881
Lines 51437 51407 -30
Branches 4812 4810 -2
==========================================
Hits 22131 22131
+ Misses 28780 28750 -30
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. |
Hoist breadcrumb, loading spinner, and not-found banner from EmergencyContactForm.vue and EmergencyContactView.vue into a new EmergencyContactPageShell.vue. Each page now owns only its h1 and body content. Pure template refactor; no runtime change.
AddCourseEffortDialog and EffortRecordEditDialog shared a Cancel + primary-action q-card-actions block with an identical #loading slot. Hoist into DialogSubmitActions.vue parameterized by submitLabel and isSaving. Other Effort dialogs still inline the pattern; migrate as they are touched.
PercentAssignmentAddDialog and PercentAssignmentEditDialog shared a 165-line q-select/q-input/q-checkbox form body. Hoist into PercentAssignmentFormFields.vue, v-modeled on the form state the usePercentageForm composable already centralized. Each dialog now owns only its title, save logic, banner variants, and footer. Also migrate both footers to DialogSubmitActions. PercentageFormState and TypeOption are re-exported from the composable so the new component can type its props.
Consolidates the h1 + ReportFilterForm + loading spinner + ReportLayout header/ExportToolbar + empty-state boilerplate that was duplicated across all 9 standard Effort report pages into a single wrapper, so individual pages only define their table content.
Share the Role / Effort Value / Notes / warning + error banner block between EffortRecordAddDialog and EffortRecordEditDialog, and route the Add dialog's footer through the existing DialogSubmitActions component.
Share the Instructors breadcrumb, loading spinner, and error banner between InstructorDetail and InstructorEdit via a slot-based shell.
Share the dialog scaffold (close affordance, title/subtitle, loading spinner, progress bar, and error state) between HarvestDialog, PercentRolloverDialog, and ClinicalImportDialog. Per-dialog preview bodies and action buttons stay in the consumers.
Share the desktop table + mobile list rendering across unopened / open / closed term sections instead of inlining three near-identical q-table blocks.
Move ITermService injection into BaseReportService and add shared helpers (LoadSingleTermContextAsync, LoadYearlyReportContextAsync, ExtractDistinctEffortTypes) so DeptSummary, SchoolSummary, and TeachingActivity services no longer hand-roll the same row + clinical-faculty + term-name plumbing.
…tService Replace four near-identical N5..N1 weighted-average + divide-by-zero guards in the summary and detail builders with a single helper.
The InstructorPageShell extraction (refactor 4601108) replaced the StatusBanner import in InstructorEdit.vue, but the template still references StatusBanner three times for save/error feedback. Vue logged "Failed to resolve component: StatusBanner" warnings and rendered those banners blank.
Carry-over from the analyzer-driven cleanup batch (PR 3). The materialization fix targets the helper extracted in d12711e — that helper doesn't exist in PR 3's base, so the fix moves here.
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
53f2c8a to
ee16b44
Compare
CA1508 (commit b9889d8) removed `(object?) ?? DBNull.Value` guards on @PersonId/@ROLE in raw SqlCommand calls, treating them as dead null-checks. For nullable value types, AddWithValue(null) sends no value at all rather than DBNull, so the eval summary/detail and merit detail endpoints returned 500 ("expects the parameter '@personid', which was not supplied") whenever the UI omitted the filter. Switch to `personId.HasValue ? personId.Value : DBNull.Value` (already used at MeritReportService:559) so CA1508 stays quiet.
There was a problem hiding this comment.
Pull request overview
Part 5 of a 6-PR stack driven by jscpd/fallow audits. Extracts repeated Vue/Quasar UI shells in the Effort and Emergency Contact areas and centralizes report-context loading in a shared C# BaseReportService. Also bundles two correctness fixes that pair with the extractions (restored StatusBanner import in InstructorEdit; restored DBNull guards on @PersonId/@Role raw-SQL parameters after CA1508 wrongly stripped them in PR #176).
Changes:
- Extract reusable Vue components:
EffortReportPage,AsyncOperationDialog,InstructorPageShell,TermTable,EmergencyContactPageShell,DialogSubmitActions,PercentAssignmentFormFields,EffortRecordSharedFields. - Centralize report context in
BaseReportService(LoadSingleTermContextAsync,LoadYearlyReportContextAsync,ExtractDistinctEffortTypes); update all subclasses + tests to passITermServicethrough the base. ExtractCalculateWeightedAverageinEvaluationReportService. - Restore
personId.HasValue ? personId.Value : DBNull.Valueand equivalent role guards inMeritReportService/EvaluationReportServiceraw SQL.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/Areas/Effort/Services/BaseReportService.cs | Adds ITermService dependency and centralized context-loading helpers. |
| web/Areas/Effort/Services/TeachingActivityService.cs | Drops local ITermService field; consumes base helpers. |
| web/Areas/Effort/Services/DeptSummaryService.cs | Uses shared yearly context + ExtractDistinctEffortTypes. |
| web/Areas/Effort/Services/SchoolSummaryService.cs | Uses shared yearly context + ExtractDistinctEffortTypes. |
| web/Areas/Effort/Services/ClinicalEffortService.cs | Constructor updated to pass ITermService to base. |
| web/Areas/Effort/Services/ClinicalScheduleService.cs | Constructor updated to pass ITermService to base. |
| web/Areas/Effort/Services/MeritReportService.cs | Restores nullable DBNull guards; base constructor wiring. |
| web/Areas/Effort/Services/MeritMultiYearService.cs | Constructor updated to pass ITermService to base. |
| web/Areas/Effort/Services/MeritSummaryService.cs | Constructor updated to pass ITermService to base. |
| web/Areas/Effort/Services/EvaluationReportService.cs | Adds CalculateWeightedAverage; restores DBNull guards. |
| test/Effort/BaseReportServiceTests.cs | Test fixture updated for new base ctor signature. |
| test/Effort/ExcelGenerationTests.cs | Updated to pass null! for new ITermService param. |
| VueApp/src/Effort/components/EffortReportPage.vue | New shared report-page shell. |
| VueApp/src/Effort/components/AsyncOperationDialog.vue | New preview/commit dialog shell. |
| VueApp/src/Effort/components/InstructorPageShell.vue | Breadcrumbs + loading/error states for instructor pages. |
| VueApp/src/Effort/components/TermTable.vue | Desktop+mobile term-selection rows. |
| VueApp/src/Effort/components/DialogSubmitActions.vue | Standard Cancel + submit cluster. |
| VueApp/src/Effort/components/PercentAssignmentFormFields.vue | Shared percent-assignment field cluster. |
| VueApp/src/Effort/components/EffortRecordSharedFields.vue | Role/value/notes/banner cluster. |
| VueApp/src/Effort/components/PercentRolloverDialog.vue | Refactored onto AsyncOperationDialog; swaps brand bg classes. |
| VueApp/src/Effort/components/ClinicalImportDialog.vue | Refactored onto AsyncOperationDialog. |
| VueApp/src/Effort/components/HarvestDialog.vue | Refactored onto AsyncOperationDialog. |
| VueApp/src/Effort/pages/Report/SchoolSummary/MeritDetail/etc. | Converted to use EffortReportPage. |
| VueApp/src/Effort/pages/InstructorEdit.vue / InstructorDetail.vue | Use InstructorPageShell; restore StatusBanner import. |
| VueApp/src/Students/EmergencyContact/components/EmergencyContactPageShell.vue | New shared page shell for EC pages. |
- Drop the three q-expansion-item background tints in PercentRolloverDialog; section differentiation now comes from the header icon + label alone, resolving a brand-color regression flagged in PR review. - TermManagement: hide-bottom-space on the boundary-year q-input so the reserved validation-message area no longer pushes the Reset / Preview Rollover buttons out of vertical alignment. - Swap the inline "changing the year is unusual" caption for a StatusBanner type="warning" live="assertive" per project banner guidance.
Contributor
Author
|
@bsedwards This branch is on TEST. This branch results in a net -600 lines of code due to deduplication and fixes some regressions in a previous fix. |
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 5 of 6. Stacks on top of PR #177.
Effort-area refactors driven by jscpd (duplication detection): extract repeated UI shells, dialog scaffolding, and form-field clusters into reusable components. Plus a small report-service consolidation in C#. Also folds in the
EmergencyContactshared page shell because it pairs naturally with the Effort UI patterns.What's in this PR
Vue/UI extractions
EmergencyContactPageShell: shared page shell for emergency contact formsDialogSubmitActions: submit/cancel button cluster used across multiple dialogsPercentAssignmentFormFields: shared form-field clusterEffortReportPage: page-layout shell for report screensEffortRecordSharedFields: duplicate field cluster across record dialogsInstructorPageShell: breadcrumbs + loading/error states for instructor pagesAsyncOperationDialog: preview-dialog shellTermTable: term-selection rows componentC# / report services
BaseReportServicecentralizes report context loading; per-report services now consume it instead of each loading the term/context independentlyCalculateWeightedAverageextracted inEvaluationReportServiceto replace four near-identical N5..N1 weighted-average + divide-by-zero guardsBundled fixes
restore StatusBanner import in InstructorEdit: patches theInstructorPageShellextraction, which dropped a still-referenced import.materialize CalculateWeightedAverage rows once: carry-over from PR 3'smaterialize IEnumerablebatch, bundled here because the helper it targets is introduced in this PR.restore DBNull guards for nullable SQL params: PR 3's CA1508 cleanup wrongly stripped(object?) ?? DBNull.Valuefromint?SqlCommandparams, returning 500 on eval summary/detail and merit detail when no faculty/role filter was set. Restored asHasValue ? Value : DBNull.Valueto keep CA1508 quiet.polish Percent Rollover preview UI: three smoke-test fixes. Dropped the three expansion-section background tints (Copilot flagged a brand-color regression in the markup re-flatten). Addedhide-bottom-spaceon the boundary-yearq-inputso Reset / Preview Rollover stay vertically centered. Swapped the inline "changing the year is unusual" caption forStatusBanner type="warning" live="assertive".Commits
refactor(emergency-contact): extract shared page shellrefactor(effort): extract DialogSubmitActions componentrefactor(effort): extract PercentAssignmentFormFieldsrefactor(effort): extract EffortReportPage layout shellrefactor(effort): extract EffortRecordSharedFields from dialogsrefactor(effort): extract InstructorPageShell for breadcrumbs and statesrefactor(effort): extract AsyncOperationDialog shell for preview dialogsrefactor(effort): extract TermTable for term selection rowsrefactor(effort): centralize report context loading in BaseReportServicerefactor(effort): extract CalculateWeightedAverage in EvaluationReportServicefix(effort): restore StatusBanner import in InstructorEditchore(quality): materialize CalculateWeightedAverage rows oncefix(effort): restore DBNull guards for nullable SQL paramsfix(effort): polish Percent Rollover preview UIPR stack
Test plan
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements