Skip to content

legacy-issue-fix#596

Open
nandap4790 wants to merge 2 commits intomasterfrom
legacy-issue-fix
Open

legacy-issue-fix#596
nandap4790 wants to merge 2 commits intomasterfrom
legacy-issue-fix

Conversation

@nandap4790
Copy link
Contributor

@nandap4790 nandap4790 commented Aug 21, 2025

Description

Please include the summary of the change made. Please include the required context here. Please include the issue reference for the fix and the dependencies reference for the change.

Fixes # (issue-reference)

Dependencies # (dependency-issue-reference)

Documentation # (link to the corresponding documentation changes)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test Case A
  • Test Case B

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Summary by CodeRabbit

  • New Features
    • Dev server now on port 8081, accessible from all hosts, with on-screen error overlays (warnings suppressed).
    • Production build improvements: compressed assets (gzip), stronger JS/CSS minification that strips debug/logging, vendor/code splitting, and optional bundle analysis.
  • Chores
    • Updated dev tooling and added performance/build scripts and compression/minification tooling.
    • Aligned test renderer version and adjusted asset host to the new dev port.

@coderabbitai
Copy link

coderabbitai bot commented Aug 21, 2025

Walkthrough

Updated publisher asset host to port 8081, added production-oriented webpack enhancements (compression, terser minimizer, splitChunks, optional bundle analyzer, image optimization, SpriteLoaderPlugin), merged new devServer settings (port 8081, host 0.0.0.0, allowedHosts all, overlay errors only), and modified devDependencies and npm scripts in package.json.

Changes

Cohort / File(s) Summary
Dev server & asset host
config/publisher.yml, webpack.config.js
Changed publisher asset_host from http://localhost:8080 to http://localhost:8081. Added/merged devServer settings: port: 8081, host: 0.0.0.0, allowedHosts: "all", client.overlay showing errors only.
Webpack production & plugins
webpack.config.js
Added plugin imports and usage: CompressionPlugin, TerserPlugin, optional BundleAnalyzerPlugin, and SpriteLoaderPlugin. Added production gzip compression (threshold 10KB, minRatio 0.8). Introduced optimization with splitChunks (vendor/common/react/lodash) and minimizer using TerserPlugin to drop console/debugger. Augmented asset/image handling to conditionally apply optimization in production. Merged enhanced rules/plugins into exported config.
Package metadata & scripts
package.json
Updated devDependencies: eslint-config-standard -> ^17.0.0, eslint-plugin-promise -> ^6.0.0, react-test-renderer -> ^16.14.0; added compression-webpack-plugin and terser-webpack-plugin. Added scripts: performance, performance:analyze, build:prod; minor check-node-npm trailing comma change.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Browser
  participant DevServer as Webpack Dev Server (WDS)
  participant Bundler as Webpack Bundler

  Note over DevServer: devServer: port=8081, host=0.0.0.0, allowedHosts=all<br/>client.overlay: errors=true, warnings=false

  Browser->>DevServer: Request assets at http://localhost:8081
  DevServer->>Bundler: Serve/compile assets (HMR)
  Bundler-->>DevServer: Build artifacts / stats
  DevServer-->>Browser: Serve bundled assets / HMR events
  alt ANALYZE_STATS=true
    Bundler->>BundleAnalyzer: Generate report
  end
  alt Production build
    Bundler->>TerserPlugin: Minify (drop console/debugger)
    Bundler->>CompressionPlugin: Emit gzipped assets (>10KB)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch legacy-issue-fix

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
webpack.config.js (2)

37-49: Parameterize port/host and optionally set CORS header for asset requests.

Helpful for local variations (e.g., port already in use) and cross-origin asset loading from the node app (often on 3000).

Suggested tweak:

   devServer: {
-    ...(webpackConfig.devServer || {}),
-    port: 8081,
-    host: '0.0.0.0',
+    ...(webpackConfig.devServer || {}),
+    port: Number(process.env.WEBPACK_DEV_SERVER_PORT || process.env.ASSET_PORT || 8081),
+    host: process.env.WEBPACK_DEV_SERVER_HOST || '0.0.0.0',
     allowedHosts: 'all',
+    headers: { 'Access-Control-Allow-Origin': '*' },
     client: {
       overlay: {
         errors: true,
         warnings: false,
       },
     },
   },

37-49: Remove duplicate overlay flags from package.json scripts

It looks like you’re specifying the same overlay behavior both in your webpack configuration and via CLI flags in your asset-server scripts. To keep a single source of truth and avoid drift:

• In package.json, update the asset-server and windows-asset-server scripts to remove the --client-overlay-errors and --no-client-overlay-warnings flags.
• Let webpack’s devServer.client.overlay settings (errors: true, warnings: false) drive the behavior.
• After making this change, run your dev server (npm run asset-server) to confirm the overlay still appears for errors and suppresses warnings as intended.

This will simplify maintenance and prevent these two configurations from getting out of sync.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1634ba7 and 8972a75.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (3)
  • config/publisher.yml (1 hunks)
  • package.json (2 hunks)
  • webpack.config.js (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: perf_prod_lighthouse_test
  • GitHub Check: stg_lighthouse_test
  • GitHub Check: build_and_test
🔇 Additional comments (1)
package.json (1)

62-62: react-test-renderer aligned to React 16.14.0 — LGTM.

The downgrade removes the React 17 mismatch and should stabilize snapshot/render tests.

@@ -1,4 +1,4 @@
asset_host: http://localhost:8080
asset_host: http://localhost:8081
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Port switch to 8081 looks right—please verify no stale 8080 references remain and that cross-device dev works.

This aligns with the new webpack devServer port. To avoid surprises:

  • Search for any lingering localhost:8080 references.
  • If you test from a phone/another machine, localhost won’t resolve; consider using your LAN IP or an env-driven host for dev.

Run:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Find stale :8080 references (excluding node_modules)
rg -nP 'localhost:8080|:8080\b' -g '!**/node_modules/**' -C2

# 2) Confirm the active asset_host points to 8081
rg -nP '^asset_host:\s*http://localhost:8081' -n config

# 3) Surface any other asset host config
rg -nP '\basset_host\b|\bASSET_HOST\b' -n

Length of output: 873


Synchronize asset server port references to 8081

We’ve confirmed the asset_host in config/publisher.yml is correctly set to http://localhost:8081, but there are still stale uses of port 8080 elsewhere. Please update the following so your local and cross-device workflows continue to work:

  • docker-compose.yml (lines 13–17)
    Currently mapping ["8080:8080"]. Change this to ["8081:8081"] (or your chosen host:container ports) so the asset server matching webpack’s devServer.port is exposed.
  • dev-docker/port-forward (line 3)
    Update the SSH tunnel from -L 8080:localhost:8080 to -L 8081:localhost:8081 (and any other forwarded ports) to mirror the new asset-server port.
  • run-prodish script (line 4)
    It runs sed -i "" 's/asset_host:.*$/asset_host:/' config/publisher.yml. Ensure any automation around resetting or templating the asset_host still works as intended now that the URL includes :8081.
  • README.md (line 22)
    Revise instructions around config/publisher.yml (e.g. “remove the asset_host”) to reflect that the default dev port is now 8081, and note any manual edits needed.
  • Cross-device access
    Remember that localhost won’t resolve on other devices. We recommend enabling an env-driven host (e.g. ASSET_HOST=http://192.168.x.y:8081) or documenting the LAN IP in your README for phone/tablet testing.

Once these updates are in place, rerun the grep checks to confirm no :8080 references remain and verify you can access the asset server both locally and from another machine.

🤖 Prompt for AI Agents
In config/publisher.yml line 1 the asset_host is set to http://localhost:8081
but there are stale 8080 references elsewhere; update docker-compose.yml (lines
13–17) to map ["8081:8081"] instead of ["8080:8080"], change
dev-docker/port-forward line 3 to forward -L 8081:localhost:8081 (and any other
forwarded 8080 ports), adjust run-prodish script line 4 so its sed pattern still
correctly matches and preserves the full URL including :8081 (or update the sed
expression to handle :PORT), and revise README.md line 22 to document the new
default dev port 8081 and recommend using an env-driven host or LAN IP for
cross-device testing; after changes run a grep for ":8080" to ensure no
remaining references and verify local and remote access to the asset server.

Comment on lines +46 to +51
"eslint-config-standard": "^17.0.0",
"eslint-plugin-import": "^2.25.2",
"eslint-plugin-jest": "^25.2.2",
"eslint-plugin-node": "^11.1.0",
"eslint-plugin-prettier": "^4.0.0",
"eslint-plugin-promise": "^5.1.1",
"eslint-plugin-promise": "^6.0.0",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

ESLint Standard v17 requires eslint-plugin-n — current setup will likely break linting.

Upgrading eslint-config-standard to 17 switches from eslint-plugin-node to eslint-plugin-n. Without installing eslint-plugin-n, ESLint typically errors with “Failed to load plugin 'n'…”.

Apply:

-    "eslint-config-standard": "^17.0.0",
+    "eslint-config-standard": "^17.0.0",
+    "eslint-plugin-n": "^16.7.0",

Then verify:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check presence in package.json
jq -r '.devDependencies["eslint-config-standard"], .devDependencies["eslint-plugin-n"]' package.json

# Validate ESLint can resolve the config/plugins (will print an error if misconfigured)
npx eslint --print-config start.js > /dev/null && echo "ESLint config OK for Standard 17"

Length of output: 761


Add eslint-plugin-n and remove deprecated eslint-plugin-node for ESLint Standard v17

To upgrade to eslint-config-standard@17, you must replace the old node plugin with the new n plugin. Without this change, linting will fail with “Failed to load plugin 'n'…” errors.

• In package.json devDependencies, remove “eslint-plugin-node” and add the latest compatible 16.x release of “eslint-plugin-n” (Standard v17 peers with eslint-plugin-n@^15.0.0 || ^16.0.0) (github.com, socket.dev)
• Verify linting completes without plugin errors by running ESLint across your project

Apply this diff:

   "eslint-config-standard": "^17.0.0",
   "eslint-plugin-import": "^2.25.2",
   "eslint-plugin-jest": "^25.2.2",
-  "eslint-plugin-node": "^11.1.0",
+  "eslint-plugin-n": "^16.6.2",
   "eslint-plugin-prettier": "^4.0.0",
   "eslint-plugin-promise": "^6.0.0",

Updated verification script:

#!/bin/bash
set -euo pipefail

# Confirm deps are correct
jq -r '.devDependencies["eslint-config-standard"], .devDependencies["eslint-plugin-n"], .devDependencies["eslint-plugin-node"]' package.json

# Run lint to ensure no missing-plugin errors
npx eslint . --max-warnings=0 && echo "✅ ESLint lint OK for Standard v17"
🤖 Prompt for AI Agents
In package.json around lines 46 to 51, the project still lists the deprecated
"eslint-plugin-node" which must be replaced with "eslint-plugin-n" to satisfy
eslint-config-standard@17; remove the "eslint-plugin-node" entry from
devDependencies and add "eslint-plugin-n" with a compatible 16.x peer (e.g.
"^16.0.0"), then run npm/yarn install and run ESLint across the repo to verify
there are no missing-plugin errors.

plugins: enhancedPlugins,
// Add explicit dev server configuration
devServer: {
...webpackConfig.devServer,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code rabbit suggesting this => + ...(webpackConfig.devServer || {}),

// Add explicit dev server configuration
devServer: {
...webpackConfig.devServer,
port: 8081,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, 8081 works only for you ,its 8080 for others nanda

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (1)
webpack.config.js (1)

56-69: Avoid magic index mutation and incorrect file-loader options (limit/fallback).

  • Overwriting enhancedRules[8] is brittle and likely to break if the base rules change.
  • limit and fallback are url-loader options, not file-loader options. They’re ignored and can confuse future readers.

Apply:

-const enhancedRules = insertIntoIndex(webpackModule.rules, 5, spriteRule);
-enhancedRules[8] = {
-  test: /\.(jpe?g|gif|png|woff|woff2|eot|ttf|wav|mp3|ico|mp4)$/,
-  loader: "file-loader",
-  options: {
-    context: "./app/assets",
-    name: "[name].[ext]",
-    // Add image optimization
-    ...(process.env.NODE_ENV === "production" && {
-      limit: 8192,
-      fallback: "file-loader",
-    })
-  },
-};
+const enhancedRules = insertIntoIndex(webpackModule.rules, 5, spriteRule);
+// Safely patch the existing asset rule instead of using a magic index
+const fileRuleIndex = enhancedRules.findIndex(
+  (r) =>
+    r &&
+    (
+      (r.loader && r.loader.includes("file-loader")) ||
+      (Array.isArray(r.use) && r.use.some((u) => (u.loader || u).includes && (u.loader || u).includes("file-loader"))) ||
+      r.type === "asset/resource"
+    )
+);
+if (fileRuleIndex !== -1) {
+  const base = enhancedRules[fileRuleIndex];
+  enhancedRules[fileRuleIndex] = {
+    ...base,
+    options: {
+      ...(base.options || {}),
+      context: "./app/assets",
+      name: process.env.NODE_ENV === "production" ? "[name].[contenthash:8].[ext]" : "[name].[ext]",
+      esModule: false
+    }
+  };
+}
♻️ Duplicate comments (2)
webpack.config.js (1)

126-131: Nice fix: safe spread of devServer base avoids runtime throws.

Using ...(webpackConfig.devServer || {}) resolves the earlier bot concern about spreading undefined.

package.json (1)

46-51: ESLint Standard v17 requires eslint-plugin-n; remove deprecated eslint-plugin-node.

Upgrading to eslint-config-standard@17 without adding eslint-plugin-n typically breaks lint with “Failed to load plugin 'n'…”. The repo still lists eslint-plugin-node.

Apply:

   "eslint-config-standard": "^17.0.0",
   "eslint-plugin-import": "^2.25.2",
   "eslint-plugin-jest": "^25.2.2",
-  "eslint-plugin-node": "^11.1.0",
+  "eslint-plugin-n": "^16.7.0",
   "eslint-plugin-prettier": "^4.0.0",
   "eslint-plugin-promise": "^6.0.0",

Verify:

#!/bin/bash
# Confirm deps and run a quick print-config
jq -r '.devDependencies["eslint-config-standard"], .devDependencies["eslint-plugin-n"], .devDependencies["eslint-plugin-node"]' package.json
npx eslint --print-config start.js >/dev/null && echo "ESLint config resolves plugins ✅"
🧹 Nitpick comments (3)
webpack.config.js (3)

8-16: Guard against missing plugins array.

insertIntoIndex calls slice on the array; if webpackConfig.plugins is undefined, this will throw. Use a safe fallback.

Apply:

-const { plugins, output, module: webpackModule } = webpackConfig;
+const { output, module: webpackModule } = webpackConfig;
+const basePlugins = Array.isArray(webpackConfig.plugins) ? webpackConfig.plugins : [];

-const enhancedPlugins = insertIntoIndex(plugins, 1, getSpritePlugin());
+const enhancedPlugins = insertIntoIndex(basePlugins, 1, getSpritePlugin());

18-38: Comment mismatch and scope.

The comment says “Bundle analyzer for development” but the code runs it only in production. Either move the ANALYZE_STATS block outside the production guard or update the comment.

Apply either:

  • Move if (process.env.ANALYZE_STATS) above the production guard; or
  • Change the comment to “Bundle analyzer (conditional)”.

126-137: Make devServer host/port configurable to avoid local conflicts (8080 vs 8081).

Hardcoding 8081 will break teams using 8080 or other ports. Respect env with sane defaults; keep the spread fallback you added.

Apply:

   devServer: {
     ...(webpackConfig.devServer || {}),
-    port: 8081,
-    host: '0.0.0.0',
-    allowedHosts: 'all',
+    port: Number(process.env.ASSET_SERVER_PORT || process.env.PORT || 8081),
+    host: process.env.ASSET_SERVER_HOST || '0.0.0.0',
+    allowedHosts: process.env.ASSET_ALLOWED_HOSTS || 'all',
     client: {
       overlay: {
         errors: true,
         warnings: false,
       },
     },
   },

Also ensure config/publisher.yml asset_host stays in sync (e.g., use env injection) so assets resolve correctly across environments.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8972a75 and 8149f23.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • package.json (4 hunks)
  • webpack.config.js (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: perf_prod_lighthouse_test
  • GitHub Check: stg_lighthouse_test
  • GitHub Check: build_and_test
🔇 Additional comments (4)
webpack.config.js (2)

47-47: Prefer contenthash for the generated sprite to improve cache-busting.

[ suggest_optional_refactor ]
Apply:

-        spriteFilename: process.env.NODE_ENV === "production" ? "svg-sprite-[hash].svg" : "svg-sprite.svg",
+        spriteFilename: process.env.NODE_ENV === "production" ? "svg-sprite-[contenthash:8].svg" : "svg-sprite.svg",

40-54: Verify exclusive .svg handling in webpack rules

We only found one .svg rule in webpack.config.js (the spriteRule at lines 40–54), and the file-loader rule at lines 56–69 explicitly targets other formats (jpe?g|gif|png|…) without .svg. However, since you’re importing a base config (@quintype/build/config/webpack), it may include its own .svg handling. Please confirm there are no other rules (in the base config) matching .svg to prevent conflicts or duplicate processing.

• Locations to review:
webpack.config.js, lines 40–54: the spriteRule
webpack.config.js, lines 56–69: the file-loader rule

• Suggested diff to safeguard the sprite rule against third-party SVGs:

 const spriteRule = {
   test: /\.svg$/,
+  exclude: /node_modules/,
   use: [
     {
       loader: "svg-sprite-loader",
       options: {
         extract: true,
         spriteFilename: process.env.NODE_ENV === "production"
           ? "svg-sprite-[hash].svg"
           : "svg-sprite.svg",
         esModule: false,
       },
     },
     "svg-transform-loader",
     "svgo-loader",
   ],
 };

If the base config includes any .svg matchers (e.g., file-loader or asset modules), ensure those rules exclude .svg (e.g., exclude: /\.svg$/) or adjust their test patterns accordingly.

package.json (2)

62-62: Version alignment LGTM.

react-test-renderer@16.14.0 matches react/react-dom@16.14.0.


73-74: ✅ Confirmed: Plugins compatible with Node and Webpack

compression-webpack-plugin@^10.0.0 and terser-webpack-plugin@^5.3.0 align with your declared webpack@^5.59.1 and engines.node "^16.14.2". Ensure your webpack.config.js only instantiates these plugins in production mode to avoid impacting dev build performance.

👍 Code changes approved.

Comment on lines +103 to +106
"check-node-npm": "check-node-version --node '>= 12.13.0' --npm '^6.14.0'",
"performance": "node scripts/performance-check.js",
"performance:analyze": "npm run analyze-stats && npm run performance",
"build:prod": "NODE_ENV=production npm run compile"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Align Node/NPM version check with engines and make build:prod cross-platform.

  • Pre-commit check enforces Node>=12 / npm ^6 while engines requires Node ^16.14.2 / npm ^8.5.0. This mismatch will let unsupported runtimes pass the hook.
  • On Windows, setting NODE_ENV inline fails without cross-env.

Apply:

-    "check-node-npm": "check-node-version --node '>= 12.13.0' --npm '^6.14.0'",
+    "check-node-npm": "check-node-version --node '>= 16.14.2' --npm '^8.5.0'",

Additionally, consider:

-    "build:prod": "NODE_ENV=production npm run compile"
+    "build:prod": "cross-env NODE_ENV=production npm run compile"

And add the devDependency:

"devDependencies": {
  "cross-env": "^7.0.3"
}
🤖 Prompt for AI Agents
In package.json around lines 103 to 106, the pre-commit check and build script
are out of sync with package.json engines and not cross-platform: update the
"check-node-npm" script to enforce the same Node and npm ranges as engines (Node
>=16.14.2 and npm >=8.5.0 / match the engines spec), change "build:prod" to use
cross-env for setting NODE_ENV so it works on Windows, and add cross-env as a
devDependency (or run npm install --save-dev cross-env) so the new script works
reliably.

Comment on lines +4 to +6
const { BundleAnalyzerPlugin } = require("webpack-bundle-analyzer");
const TerserPlugin = require("terser-webpack-plugin");
const CompressionPlugin = require("compression-webpack-plugin");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid top-level require of webpack-bundle-analyzer to prevent production build crashes when devDependencies are pruned.

Top-level require executes even when ANALYZE_STATS is unset. If your CI/production install skips devDependencies, require("webpack-bundle-analyzer") will throw. Load it only inside the ANALYZE_STATS branch.

Apply these diffs:

-const { BundleAnalyzerPlugin } = require("webpack-bundle-analyzer");
   // Bundle analyzer for development
-  if (process.env.ANALYZE_STATS) {
-    enhancedPlugins.push(
-      new BundleAnalyzerPlugin({
-        analyzerMode: "static",
-        openAnalyzer: false,
-        reportFilename: "bundle-analysis.html",
-      })
-    );
-  }
+  if (process.env.ANALYZE_STATS) {
+    // Lazy-load to avoid requiring devDeps when not analyzing
+    const { BundleAnalyzerPlugin } = require("webpack-bundle-analyzer");
+    enhancedPlugins.push(
+      new BundleAnalyzerPlugin({
+        analyzerMode: "static",
+        openAnalyzer: false,
+        reportFilename: "bundle-analysis.html",
+      })
+    );
+  }

Also applies to: 28-37

Comment on lines +71 to 118
// Enhanced optimization configuration
const optimization = {
...webpackConfig.optimization,
splitChunks: {
chunks: "all",
cacheGroups: {
vendor: {
test: /[\\/]node_modules[\\/]/,
name: "vendors",
chunks: "all",
priority: 10,
},
common: {
name: "common",
minChunks: 2,
chunks: "all",
priority: 5,
reuseExistingChunk: true,
},
// Separate React and React DOM
react: {
test: /[\\/]node_modules[\\/](react|react-dom)[\\/]/,
name: "react",
chunks: "all",
priority: 20,
},
// Separate lodash
lodash: {
test: /[\\/]node_modules[\\/]lodash[\\/]/,
name: "lodash",
chunks: "all",
priority: 15,
},
},
},
minimizer: [
new TerserPlugin({
terserOptions: {
compress: {
drop_console: process.env.NODE_ENV === "production",
drop_debugger: process.env.NODE_ENV === "production",
},
mangle: true,
},
extractComments: false,
}),
],
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Do not overwrite existing minimizers; scope Terser to production to avoid slow/dev-mangled builds.

Setting optimization.minimizer here replaces any base minimizers (e.g., CssMinimizerPlugin), potentially dropping CSS minification. Also, Terser runs in dev since this object is unconditional.

Apply:

-const optimization = {
-  ...webpackConfig.optimization,
-  splitChunks: { /* …unchanged… */ },
-  minimizer: [
-    new TerserPlugin({
-      terserOptions: {
-        compress: {
-          drop_console: process.env.NODE_ENV === "production",
-          drop_debugger: process.env.NODE_ENV === "production",
-        },
-        mangle: true,
-      },
-      extractComments: false,
-    }),
-  ],
-};
+const isProd = process.env.NODE_ENV === "production";
+const optimization = {
+  ...webpackConfig.optimization,
+  splitChunks: {
+    /* keep your cacheGroups as-is */
+    ...{
+      chunks: "all",
+      cacheGroups: { .../* vendor/common/react/lodash as above */ }
+    }
+  },
+  minimizer: [
+    // Preserve existing minimizers (e.g., CSS)
+    ...((webpackConfig.optimization && webpackConfig.optimization.minimizer) || []),
+    ...(isProd
+      ? [
+          new TerserPlugin({
+            terserOptions: {
+              compress: { drop_console: true, drop_debugger: true },
+              mangle: true
+            },
+            extractComments: false
+          })
+        ]
+      : [])
+  ]
+};

If the base config already provides a production Terser, consider only tweaking its options instead of adding another instance.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In webpack.config.js around lines 71 to 118 the new optimization.minimizer
unconditionally replaces any existing minimizers (losing CSS minimization) and
runs Terser in dev; instead, detect if process.env.NODE_ENV === "production" and
only add/configure Terser in that case, and when adding it merge with
webpackConfig.optimization.minimizer rather than overwriting so existing plugins
(e.g., CssMinimizerPlugin) are preserved; alternatively locate an existing
Terser instance in webpackConfig.optimization.minimizer and modify its
terserOptions rather than creating a second instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants