PR 5: refactor(effort): component extraction + report context centralization#178
Open
rlorenzo wants to merge 12 commits into
Open
PR 5: refactor(effort): component extraction + report context centralization#178rlorenzo wants to merge 12 commits into
rlorenzo wants to merge 12 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.15kB (-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
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 51406 -31
Branches 4812 4807 -5
==========================================
Hits 22131 22131
+ Misses 28780 28749 -31
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
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.
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
fix(effort): restore StatusBanner import in InstructorEdit. TheInstructorPageShellextraction dropped aStatusBannerimport the template still referenced. Pairs with the extraction commit it patches.chore(quality): materialize CalculateWeightedAverage rows once. Carry-over from PR 3'smaterialize IEnumerablebatch. The helper that needed materialization is introduced in this PR, so the analyzer fix is bundled here rather than in PR 3.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 oncePR stack
Test plan
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements