PR 4: refactor: dead-code removal + shared chrome extraction#177
PR 4: refactor: dead-code removal + shared chrome extraction#177rlorenzo wants to merge 6 commits intoquality/analyzer-bulk-cleanupfrom
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughLarge-scale refactoring consolidating router initialization into a shared factory ( ChangesShared Router Infrastructure & Layout Component Refactoring
ClinicalScheduler State Management Architecture
Module Export Surface Pruning
Asset Organization & Stylesheet Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
0d26c81 to
438134a
Compare
ebe32ec to
75abeb2
Compare
438134a to
11799f3
Compare
75abeb2 to
0c49b01
Compare
11799f3 to
2580a27
Compare
0c49b01 to
2b9ea83
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
VueApp/src/ClinicalScheduler/assets/schedule-shared.css (1)
18-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace
pxtypography sizes withremin the scoped heading rules.These changed heading rules still use
px, which violates the stylesheet guideline. Convert torem(e.g.,1rem,1.125rem) in all three scoped selectors.Suggested diff
.clinical-scheduler-container h3 { color: var(--ucdavis-black-80); - font-size: 16px; + font-size: 1rem; margin-top: 20px; margin-bottom: 10px; } `@media` (max-width: 768px) { .clinical-scheduler-container h2 { - font-size: 18px; + font-size: 1.125rem; } } `@media` (max-width: 480px) { .clinical-scheduler-container h2 { - font-size: 16px; + font-size: 1rem; } }As per coding guidelines,
**/*.{vue,css,scss}: "Userem(notpx) for spacing, sizing, and typography."Also applies to: 41-43, 96-98
🤖 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/ClinicalScheduler/assets/schedule-shared.css` around lines 18 - 23, Update the scoped heading rules to use rem units instead of px: in the .clinical-scheduler-container h3 rule change font-size: 16px → 1rem, margin-top: 20px → 1.25rem, and margin-bottom: 10px → 0.625rem; apply the same px→rem conversions to the other two scoped heading selectors mentioned in the review so all typography and spacing use rem (e.g., 16px→1rem, 18px→1.125rem, 20px→1.25rem).VueApp/src/Effort/router/index.ts (1)
13-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the Eval permission latch after successful loads.
This latch only clears on failure. After the first successful fetch, later session changes will keep awaiting the old resolved promise instead of refetching Eval permissions, so access can stay stale until a hard reload.
Suggested fix
async function loadEvalPermissions() { - const userStore = useUserStore() - const existingPermissions = userStore.userInfo?.permissions ?? [] - const { get } = useFetch() - const apiUrl = import.meta.env.VITE_API_URL - const evalPerms = await get(`${apiUrl}loggedInUser/permissions?prefix=SVMSecure.Eval`) - if (evalPerms.success && Array.isArray(evalPerms.result)) { - userStore.setPermissions([...existingPermissions, ...evalPerms.result]) - } else { - // Reset latch so the next navigation retries the fetch + try { + const userStore = useUserStore() + const existingPermissions = userStore.userInfo?.permissions ?? [] + const { get } = useFetch() + const apiUrl = import.meta.env.VITE_API_URL + const evalPerms = await get(`${apiUrl}loggedInUser/permissions?prefix=SVMSecure.Eval`) + if (evalPerms.success && Array.isArray(evalPerms.result)) { + userStore.setPermissions([...existingPermissions, ...evalPerms.result]) + } + } finally { evalPermissionsPromise = null } }🤖 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/router/index.ts` around lines 13 - 25, The loadEvalPermissions function leaves the global latch evalPermissionsPromise set after a successful fetch, causing later session changes to reuse the old resolved promise and never refetch Eval permissions; update loadEvalPermissions (and the success branch where userStore.setPermissions is called) to clear/reset evalPermissionsPromise (set it to null) after the successful update so subsequent navigations will trigger a fresh fetch for Eval permissions.
🤖 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/pages/DeptSummary.vue`:
- Line 168: Remove the empty <style scoped> block in DeptSummary.vue (the empty
<style scoped> at the end of the file) — simply delete that empty tag pair; also
apply the same trivial cleanup to the corresponding empty <style scoped> in
MeritSummary.vue so neither file contains an unused empty scoped style block.
In `@VueApp/src/Effort/pages/MeritSummary.vue`:
- Line 148: Remove the now-empty <style scoped> block in the MeritSummary.vue
component (the leftover <style scoped></style> after migrating imports to
report-tables.css); simply delete that empty style tag so the no-op compile step
is removed and nothing else in the component is changed.
In `@VueApp/src/layouts/EmulationBanner.vue`:
- Around line 9-26: Replace the raw q-banner with the app's StatusBanner
component: remove the <q-banner> usage and instead render <StatusBanner
type="warning"> with the emulation text (using userStore.userInfo.firstName and
userStore.userInfo.lastName) in the default slot and move the End Emulation
<q-btn> (using the existing clearEmulationHref, no-caps, dense,
color="secondary", class="text-white q-px-sm q-ml-md") into the `#action` slot;
ensure the v-if="userStore.isEmulating" is applied to the StatusBanner and drop
redundant props like dense/rounded/inline-actions since StatusBanner manages
styling and ARIA semantics internally.
---
Outside diff comments:
In `@VueApp/src/ClinicalScheduler/assets/schedule-shared.css`:
- Around line 18-23: Update the scoped heading rules to use rem units instead of
px: in the .clinical-scheduler-container h3 rule change font-size: 16px → 1rem,
margin-top: 20px → 1.25rem, and margin-bottom: 10px → 0.625rem; apply the same
px→rem conversions to the other two scoped heading selectors mentioned in the
review so all typography and spacing use rem (e.g., 16px→1rem, 18px→1.125rem,
20px→1.25rem).
In `@VueApp/src/Effort/router/index.ts`:
- Around line 13-25: The loadEvalPermissions function leaves the global latch
evalPermissionsPromise set after a successful fetch, causing later session
changes to reuse the old resolved promise and never refetch Eval permissions;
update loadEvalPermissions (and the success branch where
userStore.setPermissions is called) to clear/reset evalPermissionsPromise (set
it to null) after the successful update so subsequent navigations will trigger a
fresh fetch for Eval permissions.
🪄 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: 23d60fc1-4699-4438-a8a4-48c39393093d
📒 Files selected for processing (77)
VueApp/src/CAHFS/assets/cahfs.cssVueApp/src/CAHFS/router/index.tsVueApp/src/CMS/assets/cms.cssVueApp/src/CMS/router/index.tsVueApp/src/CTS/pages/CourseList.vueVueApp/src/CTS/pages/MyAssessmentCharts.vueVueApp/src/CTS/router/index.tsVueApp/src/CTS/types/index.tsVueApp/src/ClinicalScheduler/assets/schedule-shared.cssVueApp/src/ClinicalScheduler/components/ScheduleView.vueVueApp/src/ClinicalScheduler/components/WeekCell.vueVueApp/src/ClinicalScheduler/composables/use-schedule-normalization.tsVueApp/src/ClinicalScheduler/constants/permission-messages.tsVueApp/src/ClinicalScheduler/constants/schedule-config.tsVueApp/src/ClinicalScheduler/pages/ClinicianScheduleView.vueVueApp/src/ClinicalScheduler/pages/RotationScheduleView.vueVueApp/src/ClinicalScheduler/router/index.tsVueApp/src/ClinicalScheduler/services/clinician-service.tsVueApp/src/ClinicalScheduler/services/error-transformer.tsVueApp/src/ClinicalScheduler/services/instructor-schedule-service.tsVueApp/src/ClinicalScheduler/services/page-data-service.tsVueApp/src/ClinicalScheduler/services/permission-service.tsVueApp/src/ClinicalScheduler/stores/permissions-utils.tsVueApp/src/ClinicalScheduler/stores/permissions/index.tsVueApp/src/ClinicalScheduler/stores/schedule-actions.tsVueApp/src/ClinicalScheduler/stores/schedule-cache.tsVueApp/src/ClinicalScheduler/stores/schedule-state.tsVueApp/src/ClinicalScheduler/stores/schedule.tsVueApp/src/ClinicalScheduler/types/api-responses.tsVueApp/src/ClinicalScheduler/types/index.tsVueApp/src/ClinicalScheduler/types/rotation-types.tsVueApp/src/ClinicalScheduler/utils/schedule-update-helpers.tsVueApp/src/Computing/router/index.tsVueApp/src/Effort/components/InstructorPercentEditDialog.vueVueApp/src/Effort/composables/use-effort-type-columns.tsVueApp/src/Effort/composables/use-percentage-form.tsVueApp/src/Effort/pages/ClinicalEffort.vueVueApp/src/Effort/pages/DeptSummary.vueVueApp/src/Effort/pages/EvalDetail.vueVueApp/src/Effort/pages/EvalSummary.vueVueApp/src/Effort/pages/MeritAverage.vueVueApp/src/Effort/pages/MeritDetail.vueVueApp/src/Effort/pages/MeritSummary.vueVueApp/src/Effort/pages/MultiYearReport.vueVueApp/src/Effort/pages/SchoolSummary.vueVueApp/src/Effort/pages/TeachingActivityGrouped.vueVueApp/src/Effort/pages/TeachingActivityIndividual.vueVueApp/src/Effort/router/index.tsVueApp/src/Effort/services/harvest-service.tsVueApp/src/Effort/types/dashboard-types.tsVueApp/src/Effort/types/evaluation-types.tsVueApp/src/Effort/types/harvest-types.tsVueApp/src/Effort/types/index.tsVueApp/src/Effort/types/report-types-extended.tsVueApp/src/Effort/types/report-types.tsVueApp/src/Effort/types/verification-types.tsVueApp/src/Students/EmergencyContact/pages/EmergencyContactForm.vueVueApp/src/Students/components/PhotoGallery/PhotoGrid.vueVueApp/src/Students/components/PhotoGallery/PhotoList.vueVueApp/src/Students/router/index.tsVueApp/src/Students/services/photo-gallery-service.tsVueApp/src/components/icons/IconCommunity.vueVueApp/src/components/icons/IconDocumentation.vueVueApp/src/components/icons/IconEcosystem.vueVueApp/src/components/icons/IconSupport.vueVueApp/src/components/icons/IconTooling.vueVueApp/src/composables/DateFunctions.tsVueApp/src/composables/QuasarConfig.tsVueApp/src/composables/validation-error.tsVueApp/src/config/colors.tsVueApp/src/layouts/EmulationBanner.vueVueApp/src/layouts/FooterLinks.vueVueApp/src/layouts/ViperBrandButton.vueVueApp/src/layouts/ViperFooter.vueVueApp/src/layouts/ViperLayout.vueVueApp/src/layouts/ViperLayoutSimple.vueVueApp/src/shared/createSpaRouter.ts
💤 Files with no reviewable changes (25)
- VueApp/src/ClinicalScheduler/stores/schedule-state.ts
- VueApp/src/CTS/pages/MyAssessmentCharts.vue
- VueApp/src/CTS/pages/CourseList.vue
- VueApp/src/CTS/types/index.ts
- VueApp/src/components/icons/IconSupport.vue
- VueApp/src/Students/components/PhotoGallery/PhotoList.vue
- VueApp/src/ClinicalScheduler/stores/schedule-cache.ts
- VueApp/src/ClinicalScheduler/stores/schedule.ts
- VueApp/src/components/icons/IconTooling.vue
- VueApp/src/Students/components/PhotoGallery/PhotoGrid.vue
- VueApp/src/ClinicalScheduler/stores/schedule-actions.ts
- VueApp/src/ClinicalScheduler/stores/permissions/index.ts
- VueApp/src/Effort/composables/use-effort-type-columns.ts
- VueApp/src/ClinicalScheduler/types/index.ts
- VueApp/src/Effort/components/InstructorPercentEditDialog.vue
- VueApp/src/ClinicalScheduler/services/page-data-service.ts
- VueApp/src/Effort/types/evaluation-types.ts
- VueApp/src/components/icons/IconCommunity.vue
- VueApp/src/components/icons/IconDocumentation.vue
- VueApp/src/Effort/types/report-types-extended.ts
- VueApp/src/components/icons/IconEcosystem.vue
- VueApp/src/Effort/types/harvest-types.ts
- VueApp/src/Effort/types/verification-types.ts
- VueApp/src/Effort/types/report-types.ts
- VueApp/src/Effort/types/index.ts
| import ReportLayout from "../components/ReportLayout.vue" | ||
| import ReportDeptTabs from "../components/ReportDeptTabs.vue" | ||
| import type { DeptSummaryReport } from "../types" | ||
| import "../report-tables.css" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
LGTM — empty <style scoped> at line 191 can be deleted.
Same trivial cleanup as MeritSummary.vue.
Also applies to: 191-191
🤖 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/pages/DeptSummary.vue` at line 168, Remove the empty <style
scoped> block in DeptSummary.vue (the empty <style scoped> at the end of the
file) — simply delete that empty tag pair; also apply the same trivial cleanup
to the corresponding empty <style scoped> in MeritSummary.vue so neither file
contains an unused empty scoped style block.
| import ReportLayout from "../components/ReportLayout.vue" | ||
| import ReportDeptTabs from "../components/ReportDeptTabs.vue" | ||
| import type { MeritSummaryReport } from "../types" | ||
| import "../report-tables.css" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
LGTM — drop the empty <style scoped></style> while you're here.
The block at line 171 is now empty after the @import migration. Removing it shaves a no-op compile step.
Also applies to: 171-171
🤖 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/pages/MeritSummary.vue` at line 148, Remove the now-empty
<style scoped> block in the MeritSummary.vue component (the leftover <style
scoped></style> after migrating imports to report-tables.css); simply delete
that empty style tag so the no-op compile step is removed and nothing else in
the component is changed.
| <q-banner | ||
| v-if="userStore.isEmulating" | ||
| dense | ||
| rounded | ||
| inline-actions | ||
| class="bg-warning text-black q-ml-lg" | ||
| > | ||
| <strong>EMULATING:</strong> | ||
| {{ userStore.userInfo.firstName }} {{ userStore.userInfo.lastName }} | ||
| <q-btn | ||
| no-caps | ||
| dense | ||
| :href="clearEmulationHref" | ||
| color="secondary" | ||
| class="text-white q-px-sm q-ml-md" | ||
| >End Emulation</q-btn | ||
| > | ||
| </q-banner> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for StatusBanner component definition
fd -e vue -e ts -e tsx | xargs rg -l "StatusBanner" | head -20Repository: ucdavis/VIPER
Length of output: 1094
🏁 Script executed:
# Search for StatusBanner export/definition
rg "export.*StatusBanner|component.*StatusBanner" -t ts -t vue -t tsxRepository: ucdavis/VIPER
Length of output: 85
🏁 Script executed:
# Explore the directory structure to understand the project layout
ls -la VueApp/src/components/ | head -30Repository: ucdavis/VIPER
Length of output: 650
🏁 Script executed:
# Look for the StatusBanner component file
fd "StatusBanner" -t fRepository: ucdavis/VIPER
Length of output: 96
🏁 Script executed:
# Read the StatusBanner component
cat -n VueApp/src/components/StatusBanner.vueRepository: ucdavis/VIPER
Length of output: 4429
🏁 Script executed:
# Check if StatusBanner is imported in EmulationBanner.vue and see full context
cat -n VueApp/src/layouts/EmulationBanner.vueRepository: ucdavis/VIPER
Length of output: 964
Replace q-banner with StatusBanner for Vue SPA consistency.
Use <StatusBanner type="warning"> instead of raw q-banner. Move the emulation text into the default slot and the End Emulation button into the #action slot. StatusBanner handles rounded, inline-actions, ARIA semantics (role="status" with live="polite"), and warning styling internally.
🤖 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/layouts/EmulationBanner.vue` around lines 9 - 26, Replace the raw
q-banner with the app's StatusBanner component: remove the <q-banner> usage and
instead render <StatusBanner type="warning"> with the emulation text (using
userStore.userInfo.firstName and userStore.userInfo.lastName) in the default
slot and move the End Emulation <q-btn> (using the existing clearEmulationHref,
no-caps, dense, color="secondary", class="text-white q-px-sm q-ml-md") into the
`#action` slot; ensure the v-if="userStore.isEmulating" is applied to the
StatusBanner and drop redundant props like dense/rounded/inline-actions since
StatusBanner manages styling and ARIA semantics internally.
2580a27 to
6b500eb
Compare
aaada02 to
d26c5d8
Compare
6b500eb to
f9e9c1a
Compare
d26c5d8 to
8cbfb69
Compare
8cbfb69 to
0d62a4e
Compare
f9e9c1a to
55cbcab
Compare
0d62a4e to
a0c55c4
Compare
55cbcab to
7f36555
Compare
- Delete 13 unused files: Vite-starter icon components, an orphan footer layout, two abandoned CTS pages, a refactor- leftover percent-edit dialog, two unused photo-gallery variants, and CSS assets with zero imports. - Drop @quasar/extras and @pinia/testing from package.json after verifying zero usages. Future SVG-icon migration is tracked separately in PLAN-svg.md. - Remove unused exports (and the now-dead local declarations that supported them) from colors.ts, DateFunctions.ts, QuasarConfig.ts, photo-gallery-service.ts, use-percentage- form.ts, error-transformer.ts, permission-messages.ts, and schedule-config.ts. colors.ts in particular collapses from 256 lines to 31 after the dead palette plumbing is removed. - Fallow's `percentRule` and SCHEDULE_LABELS/SCHEDULE_MESSAGES findings are false positives (direct imports and dynamic imports fallow missed) — kept as-is.
- Delete schedule.ts + schedule-{actions,cache,state}.ts: a 549-line
state-management refactor landed in Sep 2025 ("Phase 4 - Frontend
state management refactoring") was never wired in. `useScheduleStore`
has zero importers anywhere in the codebase; RotationScheduleView and
ClinicianScheduleView talk to services directly.
- Delete stores/permissions/index.ts: shadowed by sibling
stores/permissions.ts (TypeScript resolves the .ts file before the
directory index), making it structurally unreachable.
- Drop 5 redundant named exports from use-schedule-normalization.ts
(normalizeSemesterWeeks, filterExcludedRotations, getAssignedRotationNames,
normalizeClinicianSchedule, normalizeRotationSchedule). All five are
only accessed via the useScheduleNormalization() composable; the
module-level duplicates had no importers. normalizeWeek and
normalizeScheduleSemesters stay exported — tests import them directly.
- Drop 42 dead re-exports from Effort/types/index.ts barrel: types passed through but no consumer imported them via the barrel. - Drop 15 types each from report-types.ts and report-types-extended.ts and 9 from harvest-types.ts: row/group intermediates used only as property types within the same module; now file-local. - Drop 8 service response types from api-responses.ts and parallel cleanups in instructor-schedule-service.ts, clinician-service.ts, ClinicalScheduler/types/index.ts, rotation-types.ts. - Drop singletons: PageInitialData, PermissionErrorType, PermissionsGetters, UpdateRotationParams, EffortColumnOptions, HarvestProgressEvent, DataHygieneSummaryDto, EvalCourseInfoDto, EmailFailure, ValidationErrors, 3 CTS types. - Fix BreadCrumb duplicate-export across ViperLayout.vue and ViperLayoutSimple.vue by making the local-only type file-private in both. - WeekAssignment, WeekItem, ViewMode dropped to file-local in their component files. - UserInfo stays exported — TypeScript requires it externally nameable because ProfilePic.vue's script-setup exposes store values typed with it.
Replace style-block @import url() with script-setup side-effect imports across 15 .vue files. Vite extracts shared CSS chunks instead of inlining the file into each consumer's stylesheet, and fallow/jscpd can now follow the dependency graph (the two CSS files no longer surface as "unused"). Behavior-preserving for report-tables.css and compact-form.css since their selectors are already class-prefixed. For schedule-shared.css, the bare h2/h3 element selectors are re-scoped under .clinical-scheduler-container so the rules keep their previous reach now that the file applies globally. Drop the redundant colors.css @import in WeekCell.vue — bootstrap-spa.ts already loads colors via styles/index.css for every SPA.
Seven SPA routers (CAHFS, CMS, CTS, ClinicalScheduler, Computing, Effort, Students) duplicated the same createRouter + VITE_VIPER_HOME + scroll-top + useRouteFocus setup. Hoist into src/shared/createSpaRouter.ts so each router only owns its bespoke beforeEach guard. No behavior change.
Extract EmulationBanner, FooterLinks, and ViperBrandButton from the ViperLayout/ViperLayoutSimple pair so both layouts render identical header badges, emulation indicator, and footer links from a single source.
a0c55c4 to
d8a3735
Compare
Summary
Part 4 of 6. Stacks on top of PR #176.
Cross-cutting refactors driven by fallow (dead-code detection) and jscpd (duplication detection). Net effect: ~2200 lines deleted, ~200 added.
What's in this PR
@importrules to JS-side imports for more deterministic Vite processing.createSpaRouterfactory: consolidates the duplicated router setup that each Vue SPA was open-coding.EmulationBanner,FooterLinks, andViperBrandButtonfrom the monolithicViperLayout.vueso layouts can share them without duplication.Commits
refactor: remove dead code surfaced by fallow (Phase 1)refactor(clinical-scheduler): drop orphaned store cluster (Phase 2)refactor: remove unused type exports (Phase 3)refactor: migrate CSS @import to JS-side imports (Phase 4c)refactor(router): extract createSpaRouter factoryrefactor(layouts): extract shared chrome components from ViperLayoutNote
The original branch had a
fix(effort): restore StatusBanner import in InstructorEdithere. That fix repairs a bug introduced by theInstructorPageShellextraction (which lives in PR 5), so on this base the bug doesn't exist. The fix is moved into PR 5 alongside the extraction it patches.PR stack
Test plan
createSpaRouter(route loads + navigation guard)EmulationBanner,FooterLinks,ViperBrandButtonrender correctly inViperLayoutandViperLayoutSimplenpm run audit:fallow— no regressionsSummary by CodeRabbit
New Features
Refactor