chore: centralize workspace yarn fix pipeline (RHIDP-14035) - #4568
Conversation
Add a shared fix script with CI-compatible --check mode, wire all workspaces to it, and exclude generated dist-dynamic output from global-header lint/format passes. Fixes: https://redhat.atlassian.net/browse/RHIDP-14035 Signed-off-by: Yi Cai <yicai@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
|
🤖 Finished Review · ✅ Success · Started 5:38 PM UTC · Completed 5:45 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.20 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4568 +/- ##
=========================================
Coverage 62.36% 62.36%
=========================================
Files 2607 2607
Lines 104739 104807 +68
Branches 29427 29477 +50
=========================================
+ Hits 65324 65368 +44
+ Misses 38857 37570 -1287
- Partials 558 1869 +1311
*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:
|
Review — ApproveClean, well-structured change that centralizes the workspace What was reviewed
Findings (low — non-blocking)1.
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
Exclude generated dynamic plugin bundles from prettier --write so yarn fix does not traverse dist-dynamic/dist-scalprum output. Signed-off-by: Yi Cai <yicai@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 7:43 PM UTC · Completed 7:59 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $5.30 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
Replace the existing heap limit when a workspace sets an explicit rhdhFix.nodeOptions override, and add tests for the CI conflict path. Signed-off-by: Yi Cai <yicai@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · ❌ Terminated · Started 2:51 AM UTC · Ended 3:04 AM UTC Commit: |
|
Superseded by updated review
| return [ | ||
| repoFixStep, | ||
| { | ||
| id: 'sort-package-json', |
There was a problem hiding this comment.
[low] logic-error
mergeNodeOptions drops non-heap-limit options from additional when overrideHeapLimit is true and existing contains a heap-limit flag. For example, mergeNodeOptions('--max-old-space-size=8192', '--max-old-space-size=16384 --inspect', { overrideHeapLimit: true }) returns '--max-old-space-size=16384', silently discarding '--inspect'. Not triggered by current usage but would surprise future workspace authors setting multi-option rhdhFix.nodeOptions.
Suggested fix: After the heap-limit replacement, strip the matched heap token from additional and append any remaining options to the result. Add a test case for this scenario.
| @@ -0,0 +1,329 @@ | |||
| /* | |||
There was a problem hiding this comment.
[low] naming-conventions
The .mjs extension diverges from every other script in scripts/ and scripts/ci/, which all use .js. Since the root package.json declares "type": "module", .js files are already treated as ESM, making .mjs redundant. This convention split propagates into ~20 workspace package.json files referencing ../../scripts/workspace-fix.mjs.
Suggested fix: Rename to workspace-fix.js and workspace-fix.test.js, then update all workspace package.json fix entries and the root test:workspace-fix script accordingly.
| } | ||
| return existing; | ||
| } | ||
| return `${existing} ${additional}`.trim(); |
There was a problem hiding this comment.
[low] error-handling-idiom
The Object.assign(new Error(...), { exitCode }) pattern is used in five places but no other script in the repo uses this idiom. The established pattern is to throw a plain Error and always process.exit(1) in the catch handler.
Suggested fix: Either adopt the simpler repo-wide pattern (throw plain Error, always process.exit(1)) or extract a small FixerError class to make the custom exit-code intent explicit and grep-able.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 2:51 AM UTC · Completed 3:04 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $4.92 |
1 participant
Summary
scripts/workspace-fix.mjs, a shared fix pipeline invoked byyarn fixfrom each workspace rootCONTRIBUTING.mdyarn fix --checkfor CI (runsbackstage-cli repo fix --checkonly)NODE_OPTIONS=--max-old-space-size=8192for memory-heavy fixers, matching CIdist-dynamic/dist-scalprumfrom global-header lint/format passes to avoid OOM on generated bundlesFixed
yarn fixcommand at repo rootStory: Create a single
yarn fixscript that auto-corrects fixable lint, format, and code issues with a deterministic fixer order and central extensibility.Implementation notes: CI runs
yarn fixper changed workspace (not from the monorepo root), so the shared logic lives inscripts/workspace-fix.mjsand each workspace'sfixscript delegates to it. Optional fixers (sort-package-json,markdownlint,knip) are skipped when not installed;knip --fixremains opt-in viarhdhFix.knipor--knip.Fixes: https://redhat.atlassian.net/browse/RHIDP-14035
Test plan
yarn test:workspace-fix(20 unit tests)yarn fix --checkinworkspaces/global-headeryarn fixinworkspaces/global-header(lint no longer traversesdist-dynamic)yarn fix --checkstep on changed workspacesMade with Cursor