Address fallow findings: bump to 2.90.0, remove dead code, de-duplicate Effort dialogs#228
Open
rlorenzo wants to merge 10 commits into
Open
Address fallow findings: bump to 2.90.0, remove dead code, de-duplicate Effort dialogs#228rlorenzo wants to merge 10 commits into
rlorenzo wants to merge 10 commits into
Conversation
Bundle ReportChanges will decrease total bundle size by 5.1kB (-0.24%) ⬇️. 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #228 +/- ##
==========================================
+ Coverage 44.51% 44.55% +0.03%
==========================================
Files 895 895
Lines 51760 51699 -61
Branches 4827 4821 -6
==========================================
- Hits 23041 23034 -7
+ Misses 28149 28095 -54
Partials 570 570
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughWalkthroughThree independent workstreams: (1) Fallow CI now generates a complexity baseline from the base branch and passes it to ChangesEffort Module: Shared Utilities and Dialog Refactoring
ClinicalScheduler: Dead Code and Type Removal
Fallow CI Tooling: Baseline Comparison
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over Parent,EffortDialogShell: Dialog open/validate flow
Parent->>EffortDialogShell: modelValue=true, title, submitLabel, isSaving
EffortDialogShell->>QForm: resetValidation() on open
Parent->>EffortDialogShell: `@submit` triggered by user
EffortDialogShell->>QForm: validate(true)
QForm-->>EffortDialogShell: boolean result
EffortDialogShell-->>Parent: emit submit (if valid)
end
rect rgba(144, 238, 144, 0.5)
note over Parent,usePercentageDialog: Percentage dialog state composition
Parent->>usePercentageDialog: percentAssignTypes, units
usePercentageDialog->>usePercentageForm: assemble form + fieldProps
usePercentageDialog->>useUnsavedChanges: setInitialState, confirmClose
usePercentageDialog-->>Parent: form, fieldProps, isSaving, errorMessage, warningMessage, pendingWarningConfirm
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/code-quality.yml (1)
15-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit least-privilege
GITHUB_TOKENpermissions for this workflow.This workflow currently relies on default token permissions. Add explicit read-only permissions at workflow scope to prevent unintended write-capable defaults in PR runs.
Suggested patch
name: Code Quality on: pull_request: branches: [main] + +permissions: + contents: read🤖 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 @.github/workflows/code-quality.yml around lines 15 - 19, The workflow lacks explicit GITHUB_TOKEN permissions, which relies on potentially unsafe defaults. Add a permissions block at the workflow level (before the jobs section) or at the fallow-regression job level to explicitly set least-privilege access. Configure the permissions to grant only read access to contents (content: read) and set other permissions to none to prevent unintended write capabilities during PR runs.Source: Linters/SAST tools
VueApp/src/Effort/components/PercentAssignmentEditDialog.vue (1)
152-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the update call with
try/catch/finallyto avoid stuck saving state.Line 152 awaits
percentageService.updatePercentage(...)outside atry/catch/finally. A rejection skips error messaging and can leaveisSavingtrue.Suggested fix
async function savePercentage() { if (!props.percentage) return const valid = await shell.value?.validate() if (!valid) return isSaving.value = true errorMessage.value = "" warningMessage.value = "" - - const startDate = formatToIsoDate(form.value.startMonth!, form.value.startYear!) - const endDate = - form.value.endMonth && form.value.endYear ? formatToIsoDate(form.value.endMonth, form.value.endYear) : null - - const request: UpdatePercentageRequest = { - percentAssignTypeId: form.value.percentAssignTypeId!, - unitId: form.value.unitId!, - modifier: form.value.modifier, - comment: form.value.comment || null, - percentageValue: form.value.percentageValue, - startDate, - endDate, - compensated: form.value.compensated, - } - - const result = await percentageService.updatePercentage(props.percentage.id, request) - isSaving.value = false + try { + const startDate = formatToIsoDate(form.value.startMonth!, form.value.startYear!) + const endDate = + form.value.endMonth && form.value.endYear ? formatToIsoDate(form.value.endMonth, form.value.endYear) : null + + const request: UpdatePercentageRequest = { + percentAssignTypeId: form.value.percentAssignTypeId!, + unitId: form.value.unitId!, + modifier: form.value.modifier, + comment: form.value.comment || null, + percentageValue: form.value.percentageValue, + startDate, + endDate, + compensated: form.value.compensated, + } + + const result = await percentageService.updatePercentage(props.percentage.id, request) - if (!result.success) { - errorMessage.value = result.error ?? "Failed to update percentage assignment" - return - } + if (!result.success) { + errorMessage.value = result.error ?? "Failed to update percentage assignment" + return + } - // Warnings returned after update are informational - record is already saved - if (result.warnings && result.warnings.length > 0 && !pendingWarningConfirm.value) { - warningMessage.value = result.warnings.join("; ") - pendingWarningConfirm.value = true - savedResult.value = result.result ?? null - return - } + // Warnings returned after update are informational - record is already saved + if (result.warnings && result.warnings.length > 0 && !pendingWarningConfirm.value) { + warningMessage.value = result.warnings.join("; ") + pendingWarningConfirm.value = true + savedResult.value = result.result ?? null + return + } - emit("update:modelValue", false) - emit("saved", result.result!) + emit("update:modelValue", false) + emit("saved", result.result!) + } catch (err) { + errorMessage.value = err instanceof Error ? err.message : "Failed to update percentage assignment" + } finally { + isSaving.value = false + } }🤖 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 `@VueApp/src/Effort/components/PercentAssignmentEditDialog.vue` around lines 152 - 154, The updatePercentage method call on line 152 is not wrapped in a try/catch/finally block, which means if the request fails, the isSaving.value flag will remain true, leaving the UI in a stuck saving state. Wrap the percentageService.updatePercentage call in a try block, add a catch block to handle any errors and display an appropriate error message to the user, and move the isSaving.value = false statement to a finally block to ensure it always executes regardless of success or failure.
🤖 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/EffortDialogShell.vue`:
- Line 8: The q-card element on line 8 uses an inline :style binding with px
units for width and maxWidth, which violates the frontend styling guidelines.
Remove the inline :style attribute from the q-card component and instead define
the width and max-width styling either through a CSS class that uses rem units
or through appropriate Quasar utility classes. Ensure the styling follows the
coding guideline of using rem for sizing and avoiding inline styles.
---
Outside diff comments:
In @.github/workflows/code-quality.yml:
- Around line 15-19: The workflow lacks explicit GITHUB_TOKEN permissions, which
relies on potentially unsafe defaults. Add a permissions block at the workflow
level (before the jobs section) or at the fallow-regression job level to
explicitly set least-privilege access. Configure the permissions to grant only
read access to contents (content: read) and set other permissions to none to
prevent unintended write capabilities during PR runs.
In `@VueApp/src/Effort/components/PercentAssignmentEditDialog.vue`:
- Around line 152-154: The updatePercentage method call on line 152 is not
wrapped in a try/catch/finally block, which means if the request fails, the
isSaving.value flag will remain true, leaving the UI in a stuck saving state.
Wrap the percentageService.updatePercentage call in a try block, add a catch
block to handle any errors and display an appropriate error message to the user,
and move the isSaving.value = false statement to a finally block to ensure it
always executes regardless of success or failure.
🪄 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: 580f55be-765b-4771-ab40-87670e9327de
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (25)
.github/workflows/code-quality.ymlVueApp/src/ClinicalScheduler/components/ScheduleView.vueVueApp/src/ClinicalScheduler/services/clinician-service.tsVueApp/src/ClinicalScheduler/services/instructor-schedule-service.tsVueApp/src/ClinicalScheduler/services/page-data-service.tsVueApp/src/ClinicalScheduler/services/permission-service.tsVueApp/src/ClinicalScheduler/services/rotation-service.tsVueApp/src/ClinicalScheduler/types/rotation-types.tsVueApp/src/ClinicalScheduler/utils/confirmation-dialog.tsVueApp/src/ClinicalScheduler/utils/schedule-update-helpers.tsVueApp/src/Effort/components/AddCourseEffortDialog.vueVueApp/src/Effort/components/CourseLinkDialog.vueVueApp/src/Effort/components/EditEvaluationDialog.vueVueApp/src/Effort/components/EffortDialogShell.vueVueApp/src/Effort/components/EffortRecordAddDialog.vueVueApp/src/Effort/components/EffortRecordEditDialog.vueVueApp/src/Effort/components/PercentAssignmentAddDialog.vueVueApp/src/Effort/components/PercentAssignmentEditDialog.vueVueApp/src/Effort/composables/use-effort-label.tsVueApp/src/Effort/composables/use-percentage-dialog.tsVueApp/src/Effort/pages/CourseDetail.vueVueApp/src/Effort/utils/course-relationships.tsVueApp/src/Effort/utils/grouped-options.tspackage.jsonscripts/audit-fallow.js
💤 Files with no reviewable changes (5)
- VueApp/src/ClinicalScheduler/utils/schedule-update-helpers.ts
- VueApp/src/ClinicalScheduler/services/permission-service.ts
- VueApp/src/ClinicalScheduler/services/page-data-service.ts
- VueApp/src/ClinicalScheduler/services/instructor-schedule-service.ts
- VueApp/src/ClinicalScheduler/services/clinician-service.ts
rlorenzo
added a commit
that referenced
this pull request
Jun 17, 2026
DESIGN.md requires rem and forbids inline styles; EffortDialogShell used an inline px max-width. Switch to the existing dialog-card-sm class (31.25rem / 500px) and drop the maxWidth prop, standardizing the effort dialogs on the design-system size. Addresses CodeRabbit review on #228.
There was a problem hiding this comment.
Pull request overview
This PR addresses findings surfaced by fallow by updating the toolchain, removing verified-dead ClinicalScheduler exports/methods/types, and de-duplicating several Effort dialog implementations via shared utilities/composables/components.
Changes:
- Updated
fallowto^2.90.0, fixed theaudit-fallowscript to run all analyses, and improved CI regression gating by baseliningfallow healthagainst the base branch. - Introduced shared Effort dialog building blocks (
EffortDialogShell,usePercentageDialog,useEffortLabel, and shared utils) and refactored multiple dialogs/pages to use them. - Removed unused ClinicalScheduler exports/types and uncalled service methods identified as dead code.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| VueApp/src/Effort/utils/grouped-options.ts | Adds a reusable grouped-options typeahead filter used by grouped q-select option lists. |
| VueApp/src/Effort/utils/course-relationships.ts | Centralizes Course Relationship table column definitions and the “confirm remove link” dialog. |
| VueApp/src/Effort/pages/CourseDetail.vue | Uses shared course-relationship columns and confirm dialog helper to reduce duplication. |
| VueApp/src/Effort/composables/use-percentage-dialog.ts | Adds a shared composable bundling percentage-form state + unsaved-changes + save/error/warn state. |
| VueApp/src/Effort/composables/use-effort-label.ts | Centralizes “Hours vs Weeks” effort label logic with an optional required asterisk. |
| VueApp/src/Effort/components/PercentAssignmentEditDialog.vue | Refactors to EffortDialogShell + usePercentageDialog to reduce duplicated dialog scaffolding. |
| VueApp/src/Effort/components/PercentAssignmentAddDialog.vue | Refactors to EffortDialogShell + usePercentageDialog to reduce duplicated dialog scaffolding. |
| VueApp/src/Effort/components/EffortRecordEditDialog.vue | Refactors to EffortDialogShell and shared effort-label logic. |
| VueApp/src/Effort/components/EffortRecordAddDialog.vue | Refactors to EffortDialogShell, uses shared grouped-options filtering, and shared effort-label logic. |
| VueApp/src/Effort/components/EffortDialogShell.vue | Introduces a shared Quasar dialog “chrome” wrapper that exposes validate/resetValidation. |
| VueApp/src/Effort/components/EditEvaluationDialog.vue | Deduplicates rating stats computation and cleans up QForm import to be type-only. |
| VueApp/src/Effort/components/CourseLinkDialog.vue | Uses shared course-relationship columns and confirm dialog helper to reduce duplication. |
| VueApp/src/Effort/components/AddCourseEffortDialog.vue | Uses shared grouped-options filtering and shared effort-label logic to reduce duplication. |
| VueApp/src/ClinicalScheduler/utils/schedule-update-helpers.ts | Removes unused exports while keeping internal helpers used by exported functions. |
| VueApp/src/ClinicalScheduler/utils/confirmation-dialog.ts | Removes an unused exported helper, leaving only the public bulk-delete confirmation API. |
| VueApp/src/ClinicalScheduler/types/rotation-types.ts | Removes unused interfaces/types from exported surface area. |
| VueApp/src/ClinicalScheduler/services/rotation-service.ts | Removes unused service methods and associated type imports. |
| VueApp/src/ClinicalScheduler/services/permission-service.ts | Removes unused batch/utility methods from the service class. |
| VueApp/src/ClinicalScheduler/services/page-data-service.ts | Removes an unused getAvailableYears method. |
| VueApp/src/ClinicalScheduler/services/instructor-schedule-service.ts | Removes unused audit-history methods and an orphaned type. |
| VueApp/src/ClinicalScheduler/services/clinician-service.ts | Removes unused search/rotation methods and associated interfaces. |
| VueApp/src/ClinicalScheduler/components/ScheduleView.vue | Narrows re-exported types to only those consumed externally. |
| scripts/audit-fallow.js | Fixes audit script to run dead-code, dupes, and health, aggregating exit codes instead of aborting early. |
| package.json | Bumps fallow from ^2.38.0 to ^2.90.0. |
| package-lock.json | Updates lockfile entries for fallow and platform CLI optional deps to 2.90.0. |
| .github/workflows/code-quality.yml | Adds a base-branch fallow health baseline and wires it into fallow audit via --health-baseline. |
Latest fallow release at least 7 days old as of the bump; pins the lockfile to 2.90.0 (was 2.82.0, range was a stale ^2.38.0).
dead-code exits non-zero whenever it finds anything, which aborted the script before dupes and health ran. Run all three unconditionally and aggregate exit codes, mirroring scripts/audit.js.
Make internally-used helpers module-private (getRotationDetails, clearOtherPrimaries, showConfirmationDialog) and remove the WeekItem/ViewMode type re-exports from ScheduleView that have no importers.
Delete 12 uncalled static methods across the ClinicalScheduler service layer (permission, page-data, instructor-schedule, clinician, rotation), then remove the types and imports they orphaned: AuditEntry, ClinicianRotationSummary, RotationSummary, and the now-unused Service and ApiResult imports. Also drops pre-existing dead interfaces that lint surfaced once these files were touched (ClinicianRotationItem, InstructorScheduleItem, WeekItem).
Add EffortDialogShell (dialog chrome: header, form wrapper, submit actions) and usePercentageDialog (form composable + save/error state). PercentAssignmentAddDialog and PercentAssignmentEditDialog now compose them, removing the duplicated template chrome and script wiring that fallow and jscpd flagged.
EffortRecordAddDialog and EffortRecordEditDialog now use the shared EffortDialogShell, removing the duplicated header/form/actions chrome. Extract a useEffortLabel composable and a filterGroupedOptions util shared by the three effort dialogs, removing the duplicated effort-label computed and grouped-select type-ahead filter.
Extract the child course-relationship table columns and the remove-link confirmation into a shared course-relationships util. CourseLinkDialog and CourseDetail now reuse them instead of each defining identical QTableColumn arrays and confirm-delete handlers.
Extract a single ratingStats helper for the weighted mean/SD/total so totalRespondents, computedMean, and computedSd no longer repeat the rating-count destructuring and weighted-sum calculation.
`fallow audit --gate new` re-attributes pre-existing complexity findings as "new" when files are substantially refactored — moves/rewrites defeat its base-snapshot matching, so untouched hotspots fail the gate. Generate a complexity (health) baseline from main's tree (same worktree approach as the jscpd-regression jobs) and pass --health-baseline so only genuinely-new complexity affects the verdict. Duplication stays gated by jscpd-regression; fallow's own duplication verdict is warn-only.
DESIGN.md requires rem and forbids inline styles; EffortDialogShell used an inline px max-width. Switch to the existing dialog-card-sm class (31.25rem / 500px) and drop the maxWidth prop, standardizing the effort dialogs on the design-system size. Addresses CodeRabbit review on #228.
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.
Cleans up the issues surfaced by
fallow(and fixes the audit tooling), in reviewable commits.Tooling
fallow^2.38.0→^2.90.0(latest release ≥ 7 days old at the time).npm run audit:fallownow runs all three analyses. It was aborting afterdead-code(which exits non-zero on findings), sodupes/healthnever ran. Now runs all and aggregates exit codes, mirroringscripts/audit.js.Dead code (verified each finding)
AuditEntry,ClinicianRotationSummary,RotationSummary,Service/ApiResultimports) and a few pre-existing dead interfaces lint surfaced once the files were touched.SCHEDULE_LABELS,SCHEDULE_MESSAGES,percentRule(used only in Vue<template>bindings, which this fallow version doesn't trace) andUserInfo(removing it breaks thevue-tscbuild).Duplication (Effort dialog families)
EffortDialogShell(dialog chrome) +usePercentageDialog,useEffortLabelcomposables +filterGroupedOptions/course-relationshipsutils + a localratingStatshelper.Testing
Hours→Weekslabel, ad-hoc eval computed Mean/SD/N, and course-relationship link/remove (created + removed a temp link; no data left behind).Out of scope (follow-ups)
HarvestDialog, layouts) — pre-existing; the CI jscpd gate is diff-scoped so they don't block.Note — relationship to the Clinical Scheduler audit trail
These removed audit items are safe to delete — the new Clinical Scheduler audit trail does not depend on them:
AuditLogService/ScheduleAuditController, not the removed frontend stubs.getRotationWeekAuditpointed at an endpoint that doesn't exist (non-functional);getAuditHistoryand theAuditEntrytype were genuinely unused.GetRotationWeekAuditHistoryAsync,GetInstructorScheduleAuditHistoryAsync) are untouched here and remain available for the planned per-week inline audit history (Phase 2), which will be built fresh on the same new audit service.