Code analysis tooling + security tightening + dedupe pass#162
Closed
Code analysis tooling + security tightening + dedupe pass#162
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #162 +/- ##
==========================================
+ Coverage 43.40% 43.64% +0.24%
==========================================
Files 863 866 +3
Lines 50476 50208 -268
Branches 4724 4695 -29
==========================================
+ Hits 21909 21915 +6
+ Misses 28044 27770 -274
Partials 523 523
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
rlorenzo
changed the title
Bundle ReportChanges will decrease total bundle size by 24.26kB (-1.14%) ⬇️. 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
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- PR CI fails if fallow or jscpd issue counts exceed main's baseline; existing duplication is capped but not required to reach zero. - npm run lint + lint:staged run both tools as report-only warnings on in-scope files (VueApp/src for fallow, VueApp/src + web/Areas for jscpd); findings do not block commits yet. - VueApp/.fallowrc.json declares the 8 SPA entry points and VueApp/tsconfig.json mirrors paths from tsconfig.app.json so fallow resolves @/* imports — without these, fallow false-flags ~200 files as unused.
- 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.
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.
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.
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.
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.
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.
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.
The three repeated prototype rows now iterate over a single mock array, which is how the real implementation will pull the API response.
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.
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.
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.
The four public methods now delegate to private EncodeWithMap / DecodeWithMap routines parameterised by the encoding map, eliminating the byte-for-byte duplication between the UU and XX variants.
…tService Replace four near-identical N5..N1 weighted-average + divide-by-zero guards in the summary and detail builders with a single helper.
…ages Both wrappers now exit 1 when findings are present and the block-on- warnings flag is set, matching the lint-staged-ts.js convention. The default pre-commit run keeps them informational.
- Bump SonarAnalyzer.CSharp to 10.24.0 in both web and test (lockstep alignment; previously 10.22 / 10.21). - Enable CodeQL's security-and-quality query pack (the broadest tier), upgrading from the default ~70-query pack to the full ~280-query set.
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.
- Enable Roslyn maintainability rules CA1501/1502/1505/1507/1508/ 1510-1513/1514. CA1506 and CA1515 skipped — heavy false positives on ASP.NET Core controllers and DTOs. - ReSharper inspectcode added as resharper-pr-gate CI job. Only new findings at PR-touched lines vs origin/main fail, so pre-existing issues don't block.
- Bulk-applied via `jb cleanupcode --profile=OptimizeUsings` then `dotnet format`. Removes unused usings and sorts/groups remaining ones — compiler-equivalent, no behavior change. - Adds Viper.sln.DotSettings with OptimizeUsings, ShortenReferences, and RemoveRedundancies profiles for this and follow-up PRs. - Reproduction note: cleanupcode deadlocks on MSBuild's shared compiler server while the dev server runs; set DOTNET_USE_COMPILER_SERVER=0 to bypass.
- Bulk-applied via `jb cleanupcode --profile=ShortenReferences` then `dotnet format`. Replaces inline `Namespace.Type` with imported `Type` (or `using Alias = Namespace.Type;` for ambiguous names). Compiler-equivalent — no behavior change. - PhotoExportService.cs gets the largest single diff because it juggles four DocumentFormat.OpenXml sub-namespaces with conflicting type names (`Picture`, `Paragraph`, `Run`, etc.); aliases at the top replace the inline full qualifiers throughout.
- Drop dead `?.` and `!= null` guards in 10 files where the analyzer proves the value is non-null (DI-injected helpers, catch parameters, values guaranteed by preceding null-checks). - ViteProxyHelpers.cs: suppress CA1508 with `#pragma warning disable` on the inner check of a double-checked locking pattern. The outer guard may have raced; CA1508's dataflow analysis doesn't model thread interleaving, so this is a known false positive.
- Strip empty `<param name="X"></param>` tags whose corresponding parameter doesn't exist (or where the tag adds no doc value). Removes ~24 InvalidXmlDocComment findings. - Fix malformed XML: escape literal `<T>` as `<T>` in three doc comments (HttpHelper, IamApi, UinformService); promote two `// <summary>` typos in IamApi to `///`; demote orphan `///` block on Program.cs SetAwsCredentials (top-level local function doesn't accept doc comments) to plain comment. - Rename three orphan `<param>` tags to match real parameters (UserHelper.HasPermission, RAPSSecurityService.IsAllowedTo, VMACSExport.ExportToInstances). Partial-coverage cases (methods with descriptive `<param>` tags for some but not all parameters) intentionally left alone — their existing documentation has real value, and selectively adding empty tags or deleting good ones would be a net loss.
Fixes the 29 PossibleMultipleEnumeration findings flagged by ReSharper. Each call site enumerated an IEnumerable two-or-more times (e.g., `source.Any()` then `source.First()`, or `source.Sum(a)` then `source.Sum(b)`). Materialising once with `.ToList()` is semantically equivalent and avoids re-evaluating deferred queries. - Test files: cast assertion targets to lists so xUnit's `Assert.NotEmpty` + `Assert.Equal(N, x.Count())` doesn't iterate twice. - VueTableDefault.cs: `data`, `skipColumns`, `altColumnNames` are enumerated 3+ times per helper and 3 times across helpers from `InvokeAsync`. Materialise at both layers. - EvaluationPolicyService.IsRequiredFor*: `rotationWeeks` is enumerated by `.Any()`, `.FirstOrDefault(currentWeek)`, and `.FirstOrDefault(nextWeek)`. Materialise at top of method. - CMSContentController.GetContentBlockByFn: switch `Any()/First()` to `Count == 0` / indexer. - CliniciansController.FilterCliniciansByPermissions: rename param to `cliniciansSource`, materialise once into `clinicians`. - Smaller fixes in RotationsController, EvaluationReportService.
- Bulk-applied via `jb cleanupcode --profile=RemoveRedundancies` then `dotnet format`. Removes redundant argument default values, redundant member initializers, redundant `else` after `return`, redundant anonymous-type property names, and other rewrites in the ReSharper "Remove code redundancies" category — all compiler-equivalent, no behavior change. - Updates `Viper.sln.DotSettings`: the umbrella `<CSRemoveCodeRedundancies>` flag alone is a no-op. ReSharper also requires the (sparsely-documented) sibling `<RemoveCodeRedundancies>` flag plus per-rule sub-flags (`<CSRemoveRedundantArgumentDefaultValues>`, `<CSRemoveRedundantInitializers>`) to actually trigger the rewrites. - Fixes a minor follow-up: `CliniciansController.cs:569` was changed to `clinicians.Count` (List property) from `clinicians.Count()` (LINQ). The IEnumerable to List materialisation in `9823a4c9` made `clinicians` a List, which Sonar S2971 then flagged as preferring the property. No new lint rule violations introduced by this commit.
- `npm run audit` now includes the ReSharper inspectcode pass and takes several minutes; use the per-tool scripts (`audit:fallow`, `audit:dupes`, `audit:resharper`) for fast iteration.
- Real cleanups across CMS / ClinicalScheduler / Effort / RAPS / Students: drop dead null-checks Roslyn flow analysis can prove unreachable, collapse a duplicated return path, simplify the cache lookup that was capturing a never-assigned outer variable, fix the stale XML doc param tag left behind by the IEnumerable materialisation rename, remove a virtual call from a DTO constructor, and split a SqlCommand object initializer outside the using statement. - Replace anonymous-type byte[]? gymnastics in PhotoExportService with a named record so CodeQL stops flagging the casts as redundant. - Gate now supports `--exclude-rule` and ships defaults for rules that fire false positives on ASP.NET Core / EF surfaces (DTO accessors bound at runtime by JSON / MVC, record positional properties used via record pattern Equals, and the EF nav-property NRT contract that Roslyn intentionally distrusts because Include() can be missing).
The UserHelper.GetByLoginId cache populator doesn't need the ICacheEntry parameter; using `_` silences the new ReSharper UnusedParameter.Local finding the previous commit introduced when it collapsed the closure.
|
Important Review skippedToo many files! This PR contains 298 files, which is 148 over the limit of 150. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (298)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
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
3 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
Adds three static-analysis tools (fallow for Vue/TS dead code + duplication, jscpd for cross-language clones, and ReSharper inspectcode for C# inspections), enables a tranche of Roslyn maintainability rules, uses the combined findings to drive a multi-phase cleanup of the existing backlog, and tightens the existing security tooling. Net result: ~2,000 fewer lines of code, regression-gated CI for all three new tools, and CodeQL upgraded to its broadest query pack.
Tooling additions
npm run lint,lint:staged, andnpm run audit:fallowwhole-project runner. CI regression job fails PRs that increase any finding count.npm run lint,lint:staged, andnpm run audit:dupes. CI regression job fails PRs that raise the C# clone count.npm run audit:resharper(full HTML+SARIF),audit:resharper:pr(PR-diff gate), andaudit:resharper-staged(staged-files gate for local pre-commit checks). CIresharper-pr-gatefails PRs that introduce findings on PR-touched lines only — pre-existing issues never block.Both wrappers honor
LINT_BLOCK_ON_WARNINGS=true(matchinglint-staged-ts.js) so the default pre-commit run stays informational andnpm run lint:precommitblocks on findings.npm run auditruns all three whole-project scanners in sequence (fallow → jscpd → ReSharper); ReSharper adds several minutes, so use the per-tool scripts for fast iteration.Roslyn analyzer rules enabled
Turned on the maintainability rules that ship off-by-default in NetAnalyzers: CA1501 (excessive inheritance), CA1502 (cyclomatic complexity), CA1505 (low maintainability index), CA1507 (use
nameof), CA1508 (dead conditional code), CA1510–1513 (ArgumentNullException.ThrowIfNullfamily), and CA1514 (redundantSubstringlength). CA1506 (class coupling) and CA1515 (sealed-by-default) skipped — heavy false-positive rate on ASP.NET Core controllers and DTOs. The one CA1508 false positive (the inner check ofViteProxyHelpers' double-checked-locking pattern) is suppressed inline; the analyzer's dataflow doesn't model thread interleaving.Security tooling tightening
security-and-quality(~280 queries) for bothcsharpandjavascript.web/andtest/projects (previously drifted at 10.22 / 10.21).(Evaluated Semgrep / Opengrep during this branch; concluded they're redundant on top of CodeQL
security-and-qualityfor this codebase.)Code cleanup driven by the new tools
colors.ts256 → 31 lines).permissions/index.tsremoved.createSpaRouterfactory hoisted across 7 SPA routers.EmergencyContactPageShell.vueshared between form and view pages.DialogSubmitActions.vueandPercentAssignmentFormFields.vueextracted from the percent-assignment dialog pair..vuefiles moved from in-template@import url(...)to JS-side imports so Vite extracts shared CSS into one chunk and fallow stops flagging the files.EffortReportPage(-302 lines across 9 report pages),ViperLayoutchrome (EmulationBanner/FooterLinks/ViperBrandButton),EffortRecordSharedFields,InstructorPageShell,AsyncOperationDialog(Harvest / PercentRollover / ClinicalImport),TermTable,CourseStudentsv-for collapse.BaseReportService.LoadSingleTermContext/LoadYearlyReportContext,IScheduleEntity+ApplyScheduleFilters,StudentGroupService.BuildStudentPhotoListAsync+GetActiveRossIamIdsAsync,CodecsUU/XX collapse onto shared encode/decode helpers,EvaluationReportService.CalculateWeightedAverage.cleanupcodeprofiles:OptimizeUsings(sort + drop unused usings),ShortenReferences(drop redundantNamespace.Typequalifiers — largest hit inPhotoExportService.csfrom four conflicting OpenXml sub-namespaces),RemoveRedundancies(redundant default arguments, member initializers,else-after-return, anonymous-type property names). All compiler-equivalent.?./!= nullguards in 10 files where dataflow proves non-null.<param>tags removed; fixed three malformed<T>escapes and a few orphan///blocks.IEnumerableonce in 29 call sites enumerating two-or-more times (test assertions,VueTableDefault,EvaluationPolicyService, controllers).Viper.sln.DotSettingswith the three cleanup profiles. Documented gotcha: ReSharper's umbrella<CSRemoveCodeRedundancies>flag is a no-op without the sparsely-documented sibling<RemoveCodeRedundancies>plus per-rule sub-flags.