fix(lint): surface stylelint results and clear the backlog they hid - #286
Open
rlorenzo wants to merge 5 commits into
Open
fix(lint): surface stylelint results and clear the backlog they hid#286rlorenzo wants to merge 5 commits into
rlorenzo wants to merge 5 commits into
Conversation
Bundle ReportChanges will decrease total bundle size by 182 bytes (-0.01%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: viper-frontend-esmAssets Changed:
Files in
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
=======================================
Coverage 50.09% 50.09%
=======================================
Files 998 998
Lines 58370 58370
Branches 5858 5858
=======================================
Hits 29239 29239
Misses 28228 28228
Partials 903 903
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
Fixes the CSS lint pipeline so Stylelint findings are correctly surfaced (and do not exceed Windows command-line limits), then applies repo-wide style/accessibility remediations to bring Stylelint findings to zero.
Changes:
- Fix Stylelint JSON parsing to read results from
stderrwhenstdoutis empty, and batch file arguments to avoid Windows ENAMETOOLONG. - Adjust Stylelint a11y configuration tier (
strict→recommended) with documented rationale. - Apply stylelint-driven CSS updates across Vue SFCs (wrapping, modern color syntax, media query range syntax, reduced-motion override ordering) and restore visible focus indicators.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| VueApp/src/Effort/pages/StaffDashboard.vue | Restores robust keyboard focus indicators using the shared focus-ring pattern. |
| VueApp/src/Effort/pages/AuditList.vue | Replaces word-break with overflow-wrap for better wrapping behavior. |
| VueApp/src/CTS/pages/CompetenciesBundleReport.vue | Updates wrapping rules to overflow-wrap across table cells/columns. |
| VueApp/src/CTS/components/LevelSelect.vue | Modernizes color notation (space-separated rgb() with alpha). |
| VueApp/src/components/SortableList.vue | Ensures reduced-motion override comes after the animated rule so it wins. |
| VueApp/src/CMS/pages/ManageLinkCollections.vue | Refines long-text wrapping rules (remove/replace word-break). |
| VueApp/src/ClinicalScheduler/components/WeekHistoryContent.vue | Suppresses a known Stylelint false positive for Vue SFC v-bind() values. |
| VueApp/src/ClinicalScheduler/components/WeekCell.vue | Renames keyframes to kebab-case and updates the animation reference. |
| VueApp/src/ClinicalScheduler/components/ScheduleView.vue | Updates to media query range syntax for mobile breakpoint styling. |
| VueApp/src/assets/base.css | Removes unused dark-mode starter-template variables and media block. |
| stylelint.config.mjs | Switches a11y tier to recommended with detailed justification. |
| scripts/lint-staged-css.js | Batches Stylelint runs to avoid Windows command-line length limits. |
| scripts/lib/lint-staged-common.js | Fixes JSON parsing to accept Stylelint JSON output from stderr. |
Comment on lines
+121
to
+125
| // Status 2 means "violations found" - only warn if no violations were parsed | ||
| if (stylelintResult.status === 2) { | ||
| const jsonToCheck = stylelintResult.stdout.trim() || stylelintResult.stderr.trim() | ||
| const hasValidJson = jsonToCheck && jsonToCheck.startsWith("[") | ||
| if (!hasValidJson) { |
rlorenzo
force-pushed
the
fix/stylelint-output-parsing
branch
from
August 5, 2026 13:05
c07c6ca to
bbb97a9
Compare
- parseJsonOutput read only stdout, but stylelint writes its JSON report to stderr, so every result was discarded and the CSS check always reported success regardless of what it found - stylelint also received every path as an argument, so a whole-tree run exceeded the Windows ~8191 char command-line limit and failed outright - ESLint still writes to stdout, so the vue and cshtml checks are unaffected - VueApp/src now reports 164 issues that were previously invisible
- modern color notation, media range syntax and blank-line spacing, via stylelint --fix - word-break: break-word is a deprecated alias, so switched to overflow-wrap; one line was dropped instead, its block already setting overflow-wrap: anywhere - renamed the fadeToBackground keyframe to kebab-case, updating its one reference - suppressed declaration-property-value-no-unknown on Vue's v-bind() in CSS, which stylelint cannot resolve - corrected stylelint's reduced-motion autofix: it emitted the media query before the rule it overrides, so at equal specificity the animation always won and the override never applied - takes VueApp/src from 164 stylelint issues to 134, all of which are a11y rules needing design decisions
- strict adds nine rules beyond recommended which produced 132 findings here and no real defects: dark-theme demands, baseline-grid line heights, and display:none inside print and responsive blocks - both genuine WCAG failures found so far came from recommended rules - takes VueApp/src and the Razor CSS to zero findings
- both :focus states set outline: none and offered only a background tint or a 5% brightness shift in its place, leaving keyboard users without a visible focus indicator (WCAG 2.4.7) - adds the system focus ring on :focus-visible, matching the pattern in styles/base.css including the transparent outline that carries the indicator under Windows forced-colors
- the block flipped --color-* to dark values under prefers-color-scheme, implying dark-mode support the app does not have - removing it orphaned seven --vt-c-*-dark declarations, dropped too - base.css reaches only the scaffold main.ts entry; none of the seven area SPAs import it
rlorenzo
force-pushed
the
fix/stylelint-output-parsing
branch
from
August 5, 2026 16:16
bbb97a9 to
aa2984a
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
|
Contributor
📝 WalkthroughWalkthroughThe PR updates frontend CSS conventions, accessibility behavior, responsive syntax, motion handling, theme variables, and Stylelint processing. Stylelint now uses batched execution and parses reports from either output stream. ChangesFrontend styling updates
Stylelint workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant LintStagedCSS
participant Stylelint
participant parseJsonOutput
LintStagedCSS->>Stylelint: lint batches of up to 50 files
Stylelint-->>LintStagedCSS: return status and output
LintStagedCSS->>parseJsonOutput: parse cleaned stdout or stderr
parseJsonOutput-->>LintStagedCSS: return accumulated issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
VueApp/src/ClinicalScheduler/components/WeekCell.vue (1)
427-440: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the reduced-motion override win the cascade.
When reduced motion is enabled, the rule at Line [427]-[433] sets
animation: none. The base rule at Line [440] appears later with equal specificity. The renamed animation still runs.Move the reduced-motion block after the base rule. Apply the same ordering to the other reduced-motion transition overrides in this style block.
🤖 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/components/WeekCell.vue` around lines 427 - 440, Reorder the styles in WeekCell.vue so the base .week-cell__assignment-item--newly-added rule appears before its prefers-reduced-motion override, allowing animation: none to win the cascade. Apply the same ordering adjustment to all other reduced-motion transition overrides in this style block, without changing their declarations.
🤖 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/components/SortableList.vue`:
- Around line 318-323: Update the reduced-motion comment associated with the
`@media` prefers-reduced-motion rule in SortableList.vue to state that the
.sortable-row--moved animation is disabled, replacing the outdated claim that
the flash still plays. Leave the CSS rule unchanged.
In `@VueApp/src/Effort/pages/StaffDashboard.vue`:
- Around line 1267-1271: Replace the 2px outline width with 0.125rem in both
focus-visible rules, including .dept-row--clickable:focus-visible and the
corresponding rule near the second occurrence, while preserving the existing
box-shadow declarations.
---
Outside diff comments:
In `@VueApp/src/ClinicalScheduler/components/WeekCell.vue`:
- Around line 427-440: Reorder the styles in WeekCell.vue so the base
.week-cell__assignment-item--newly-added rule appears before its
prefers-reduced-motion override, allowing animation: none to win the cascade.
Apply the same ordering adjustment to all other reduced-motion transition
overrides in this style block, without changing their declarations.
🪄 Autofix
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 Plus
Run ID: 643c668a-1603-43eb-8991-9fd455628654
📒 Files selected for processing (13)
VueApp/src/CMS/pages/ManageLinkCollections.vueVueApp/src/CTS/components/LevelSelect.vueVueApp/src/CTS/pages/CompetenciesBundleReport.vueVueApp/src/ClinicalScheduler/components/ScheduleView.vueVueApp/src/ClinicalScheduler/components/WeekCell.vueVueApp/src/ClinicalScheduler/components/WeekHistoryContent.vueVueApp/src/Effort/pages/AuditList.vueVueApp/src/Effort/pages/StaffDashboard.vueVueApp/src/assets/base.cssVueApp/src/components/SortableList.vuescripts/lib/lint-staged-common.jsscripts/lint-staged-css.jsstylelint.config.mjs
💤 Files with no reviewable changes (1)
- VueApp/src/assets/base.css
Comment on lines
+318
to
+323
| /* Must follow the rule above: equal specificity, so the later declaration wins. */ | ||
| @media screen and (prefers-reduced-motion: reduce) { | ||
| .sortable-row--moved { | ||
| animation: none; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the reduced-motion comment.
The comment at Line [299]-[301] says the flash plays under reduced motion. The new rule at Line [320]-[322] disables .sortable-row--moved animation. Update the comment to describe the current behavior.
🤖 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/components/SortableList.vue` around lines 318 - 323, Update the
reduced-motion comment associated with the `@media` prefers-reduced-motion rule in
SortableList.vue to state that the .sortable-row--moved animation is disabled,
replacing the outdated claim that the flash still plays. Leave the CSS rule
unchanged.
Comment on lines
+1267
to
+1271
| .dept-row--clickable:focus-visible { | ||
| outline: 2px solid transparent; | ||
| box-shadow: | ||
| 0 0 0 0.1rem white, | ||
| 0 0 0 0.25rem var(--focus-ring-color); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use rem for the focus outline width.
The changed focus-ring declarations use 2px at Line [1268] and Line [1336]. Replace both values with 0.125rem to follow the Vue/CSS sizing guideline.
As per coding guidelines, use rem instead of px for sizing in Vue/CSS.
Proposed fix
- outline: 2px solid transparent;
+ outline: 0.125rem solid transparent;Also applies to: 1335-1339
🤖 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/StaffDashboard.vue` around lines 1267 - 1271, Replace
the 2px outline width with 0.125rem in both focus-visible rules, including
.dept-row--clickable:focus-visible and the corresponding rule near the second
occurrence, while preserving the existing box-shadow declarations.
Source: Coding guidelines
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
Started as a Windows-only "command line is too long" failure in the CSS lint step. Fixing that revealed the real problem: stylelint has been reporting nothing at all, repo-wide.
Takes
VueApp/srcfrom 164 stylelint findings to 0.Two bugs in the tooling
1. Every stylelint result was being discarded.
parseJsonOutputinscripts/lib/lint-staged-common.jsread onlystdout, but stylelint writes its JSON report tostderr. Withstdoutempty the function returned[]and the CSS check reported success no matter what it found. The text fallback did not catch it either, since that only fires on a JSON parse error rather than on empty output.ESLint writes to
stdout, so the vue and cshtml checks were unaffected and still report identically.2. Stylelint received every path as an argument. A whole-tree run passed 207 paths on one command line and blew past the Windows ~8191 character limit. Now batched in groups of 50, matching what
runOxfmtCheckalready did.Fixing only the second bug would have left stylelint just as blind, minus the crash. This was caught by injecting a deliberate
#FFFFFFviolation after the batching fix and finding it still went unreported.Remediation: 164 to 0
stylelint --fix)word-break: break-wordtooverflow-wrap(one dropped as redundant)v-bind()false positive suppressedstricttier torecommendedThe focus ring fix is a real accessibility bug
Two
:focusstates inStaffDashboard.vuesetoutline: noneand offered only a background tint or a 5% brightness shift in its place, leaving keyboard users with no visible focus indicator (WCAG 2.4.7). Both now carry the system focus ring on:focus-visible, matching the pattern instyles/base.cssincluding the transparent outline that keeps the indicator visible under Windows forced-colors.One autofix was wrong and was corrected
stylelint --fixgenerated a@media (prefers-reduced-motion: reduce)block before the rule it overrides. At equal specificity the later declaration wins, so the animation always won and the reduced-motion override never applied. It was also emitted at column 0. Moved after the rule, with a comment recording why order matters. Worth knowing that--fixoutput needs review rather than being assumed safe; it also wrote three bare-LF lines that had to be normalized back to CRLF.strict to recommended
The a11y plugin ships two tiers.
recommendedenables 3 rules,strictadds 9 more.Lining that up against what was actually found here: both genuine WCAG failures came from
recommendedrules (no-outline-none,media-prefers-reduced-motion). Every one of the 132 findings fromstrict-only rules turned out to be a design decision or a false positive:media-prefers-color-scheme(95) asks every colour rule for a dark counterpart. The app ships a single light theme whose pairings are verified against WCAG AA (see DESIGN.md), and Quasar themes via abody--darkclass rather than this media query, so even a proper dark mode likely would not satisfy it.no-display-none(13) flagsdisplay: noneinside@media printand responsive blocks, where it is correct.line-height-is-vertical-rhythmed(12) enforces a baseline grid. This is not WCAG 1.4.12, which requires text to stay readable when a user sets 1.5, not that authors ship it.font-size-is-readable(9) flags 13px on dense UI chrome, a DESIGN.md typography question.Switching tiers replaces a growing list of one-off rule disables with a single decision, and follows the maintainer's own tiering. The reasoning is recorded in a comment on the
extendsline.Verified the plugin is still enforcing rather than silently unloaded: an injected
outline: noneinside a<style>block is still caught bya11y/no-outline-none.Dead code
assets/base.csscarried a@media (prefers-color-scheme: dark)block from the Vue starter template, flipping--color-*to dark values and implying dark-mode support the app does not have. Removed, along with the seven--vt-c-*-darkdeclarations it orphaned.Note for a possible follow-up: that file is reached only by the scaffold
main.tsentry, whoseApp.vuestill renders the Vue logo placeholder. None of the seven area SPAs import it, and nothing inweb/Viewsreferences the main bundle, so the wholeindex.html->main.ts->App.vue->main.css->base.csschain looks unused. Removing it would also drop a Vite build target, so it is left out of this PR.Verification
VueApp/srcand the Razor CSS both at 0 findingsstylelint --fixhad rewrittenReviewer note
chore(lint): disable a11y/media-prefers-color-schemeis superseded by the later tier swap, which removed that one-off disable. Kept for history; happy to squash the two if preferred.