Add repository-wide safeguards to prevent Material UI v4 reintroduction - #4847
lokanandaprabhu wants to merge 10 commits into
Conversation
Changed Packages
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4847 +/- ##
=======================================
Coverage 63.65% 63.65%
=======================================
Files 2692 2692
Lines 107325 107325
Branches 30062 30061 -1
=======================================
Hits 68318 68318
+ Misses 37236 37232 -4
- Partials 1771 1775 +4
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 3:21 PM UTC · Completed 3:44 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.57 |
|
Risk Assessment: elevated (3/5) DetailsThis large-scale ESLint consolidation and Material UI v4 safeguard PR touches 41 files across 14 workspaces with a large blast radius, CI workflow changes, 3 protected paths, a dependency file update, and zero test coverage — all high Tier 1 signals — but the underlying files exhibit very low churn, no regression history, and minimal author contention in git history, yielding an elevated composite score of 3. |
ReviewFindingsHigh
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| return migratedWorkspaces.map(name => join(workspacesDir, name)); | ||
| } | ||
|
|
||
| async function reportMigratedWorkspaceDrift(migratedWorkspaces) { |
There was a problem hiding this comment.
[low] enforcement gap
The reportMigratedWorkspaceDrift function (lines 89-124) logs warnings when a migrated workspace directory or its eslint.frontend-shared.cjs file is missing, but does not contribute to the violations array or call process.exit(1). The CI check passes with only a console warning that may go unnoticed.
Suggested fix: Add drift entries to the violations array so they cause a CI failure, or emit a structured annotation (::warning::) visible in the GitHub Actions UI.
|
|
||
| // eslint-disable-next-line @backstage/no-relative-monorepo-imports -- workspace ESLint shared config | ||
| module.exports = require('../../eslint.frontend-shared.cjs')(__dirname); | ||
| module.exports = require('@backstage/cli/config/eslint-factory')(__dirname); |
There was a problem hiding this comment.
[low] defense-in-depth regression
The 9 remaining packages/app/.eslintrc.js files now call @backstage/cli/config/eslint-factory directly, bypassing the shared config that included materialUiMigrationEslintConfig with v4 import guards. Plugin packages retain the guards via the root config. Only app packages lose inline ESLint feedback; the CI script still catches violations.
| process.exit(1); | ||
| } | ||
|
|
||
| await main(); |
There was a problem hiding this comment.
[low] error-handling-idiom
Top-level await main() deviates from the established entry-point idiom used by sibling scripts in this directory, which use main().catch(error => { console.error(error.stack); process.exit(1); }). On Node.js 22/24 the practical impact is negligible but consistency is preferred.
Suggested fix: Replace await main(); with main().catch(error => { console.error(error.stack); process.exit(1); });.
| * Lockfiles are not scanned because many workspaces still resolve Material UI | ||
| * v4 transitively through Backstage dependencies. | ||
| * | ||
| * Copyright Red Hat, Inc. |
There was a problem hiding this comment.
[low] documentation-comment-format
Copyright attribution Copyright Red Hat, Inc. differs from the three existing scripts/ci/ scripts which carry Copyright The Backstage Authors. The new file is genuinely Red Hat-authored so the attribution may be correct, but inconsistency within the directory may cause confusion.
Suggested fix: Consider whether the project has a policy on copyright headers for new files and align accordingly.
its-mitesh-kumar
left a comment
its-mitesh-kumar
left a comment
There was a problem hiding this comment.
Several packages/app / packages/app-legacy missing .eslintrc.js entirely — now have they no ESLint config. Are we sure on it ?
|
@its-mitesh-kumar Thanks for catching this, I have restored them all. |
Centralize ESLint import restrictions and add CI validation for direct @material-ui/* dependencies so migrated workspaces cannot accidentally reintroduce deprecated MUI v4 packages. Co-authored-by: Cursor <cursoragent@cursor.com>
Disable Yarn lifecycle scripts in the verify-repo install step and address Sonar code smells in the verification script. Co-authored-by: Cursor <cursoragent@cursor.com>
Intentionally includes @material-ui/core to confirm CI lint fails. Revert after validation. Co-authored-by: Cursor <cursoragent@cursor.com>
Run full workspace lint in CI, extend verify-no-material-ui-v4.js to scan source imports in migrated workspaces, and restore the temporary MUI v4 test file to validate CI catches violations. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove before merge. Co-authored-by: Cursor <cursoragent@cursor.com>
Revert packages/app to default eslint-factory and remove shared ESLint from app-legacy to avoid legacy blast-radius failures. Restore repo lint --since origin/main while keeping the verify script as the MUI v4 policy gate. Co-authored-by: Cursor <cursoragent@cursor.com>
…t file. Replace implicit eslint.frontend-shared.cjs detection with material-ui-v4-policy.json as the single source of truth for migrated workspaces. Remove the temporary __ci-test-mui-v4__.ts file after confirming CI enforcement via the verify script. Co-authored-by: Cursor <cursoragent@cursor.com>
Fail CI on migrated-workspace policy drift, align script entry-point error handling with sibling CI scripts, and document repository-wide checks in the CI workflow README. Co-authored-by: Cursor <cursoragent@cursor.com>
Use try/await/catch so Sonar rule S7785 is satisfied while still logging stack traces and exiting non-zero on unexpected failures. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep existing license headers and frontend-shared wiring that already existed on main; the earlier scoping pass incorrectly removed them. Co-authored-by: Cursor <cursoragent@cursor.com>
398fb15 to
8acc471
Compare
|
4 participants
Summary
eslint.frontend-shared.cjsat the repo root to block@material-ui/*imports (and deprecated@mui/styles) with guidance to use@mui/*, Backstage UI, or Canon.eslint.frontend-shared.cjswrappers.packages/appwas reverted to defaulteslint-factory;packages/app-legacyhas no shared ESLint config (legacy quarantine).scripts/ci/verify-no-material-ui-v4.jsas the CI policy gate, backed byscripts/ci/material-ui-v4-policy.json(explicitmigratedWorkspaceslist + allowlist exceptions).verify-repo) job in CI andyarn verify:no-material-ui-v4for local validation.Enforcement model
verify-no-material-ui-v4.jsmaterial-ui-v4-policy.jsonpackage.jsondeps and source importsfrontend-sharedno-restricted-imports)repo lint --since origin/mainPolicy config (
scripts/ci/material-ui-v4-policy.json)migratedWorkspaces: explicit list of 13 UI-team migrated workspaces (single source of truth for verify scope)allowedPackageJsonPaths: temporary exceptions for workspaces still migrating (konflux, x2a, dcm, augment, cost-management, mcp-integrations, extensions app-legacy, theme mui4-test, etc.)allowedSourcePathPrefixes: allowlisted legacy source paths (e.g.extensions/packages/app-legacy/,theme/plugins/mui4-test/)The verify script also warns on policy drift (e.g. migrated workspace missing
eslint.frontend-shared.cjs).Scope
In scope (13 workspaces): adoption-insights, ai-integrations, app-defaults, bulk-import, extensions, global-header, homepage, intelligent-assistant, orchestrator, quickstart, scorecard, theme, translations
Out of scope (per epic owner): konflux, x2a, dcm, augment, cost-management, mcp-integrations, etc. — covered by allowlist, not in changed-workspace ESLint wiring.
Notes
@material-ui/*transitively through Backstage dependencies. Directpackage.jsonenforcement is in place; lockfile validation can be added once upstream removes transitive MUI v4 packages.eslint.frontend-shared.cjs=eslint-factory+ MUI v4 rules only. Revertingpackages/appremoved MUI extras, not other Backstage lint rules.verify-repois a repository-wide job bucket — more repo-level checks can be added as steps later.Test plan
yarn verify:no-material-ui-v4passes on a clean repo@material-ui/corein a migratedpackage.jsonfails the verify script@material-ui/coresource import fails ESLint on a wired frontend plugin (no-restricted-imports)__ci-test-mui-v4__.ts) — confirm verify script catches violationsapp-legacy/packages/app