feat: Complete Phase 3 TypeScript migration - Environment files#614
feat: Complete Phase 3 TypeScript migration - Environment files#614
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (31)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces TypeScript-based environment configs (development/production/test) with bundler-aware logic, adds path/env validation utilities and centralized error types, updates config defaults (assets_bundler/transpiler) and env sanitization, adds type exports via a new “types” entry, removes legacy JS env files, adds a build script to strip "use strict", expands tests/docs, and updates installer/workflow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant Env as package/env.ts
participant Cfg as package/config.ts
participant TG as package/utils/typeGuards.ts
participant PV as package/utils/pathValidation.ts
Dev->>Env: Read RAILS_ENV/NODE_ENV
Env->>Env: sanitizeEnvValue()
Env-->>Dev: Validated envs
Dev->>Cfg: Load config
Cfg->>Cfg: Determine assets_bundler (env override/default)
Cfg->>Cfg: Determine javascript_transpiler (env override / legacy webpack_loader / default)
Cfg-->>Dev: Final config
Dev->>TG: isValidConfig(config)
TG->>TG: Check TTL cache / shouldValidate()
TG->>PV: Path traversal checks (always)
PV-->>TG: Safe/unsafe
TG-->>Dev: Valid/Invalid (cache result)
sequenceDiagram
autonumber
participant Base as baseConfig (shared)
participant Dev as environments/development.ts
participant Prod as environments/production.ts
participant Test as environments/test.ts
participant Bundler as assets_bundler
Note over Bundler: "webpack" or "rspack"
Dev->>Bundler: Select dev config
alt webpack
Dev->>Dev: Optional ReactRefreshWebpackPlugin
else rspack
Dev->>Dev: Optional ReactRefreshRspackPlugin<br/>devMiddleware.writeToDisk filter
end
Prod->>Bundler: Select optimization/compression
Prod->>Prod: Optional gzip/brotli plugins
Test->>Bundler: Select test config
Dev-->>Base: merge()
Prod-->>Base: merge()
Test-->>Base: merge()
sequenceDiagram
autonumber
actor CI
participant WF as generator.yml
participant Ruby as lib/install/template.rb
participant NPM as npm
CI->>WF: Build TypeScript
WF->>NPM: npm pack
NPM-->>WF: tarball path
WF->>WF: Export SHAKAPACKER_NPM_PACKAGE
WF->>Ruby: run install
alt SHAKAPACKER_NPM_PACKAGE path exists
Ruby->>NPM: Install from local tarball
else fallback
Ruby->>NPM: Install from registry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Code Review for PR #614: TypeScript Migration - Environment Files✅ Overall AssessmentThis PR successfully completes Phase 3 of the TypeScript migration initiative. The conversion of environment configuration files to TypeScript is well-executed and maintains backward compatibility while adding valuable type safety. 🎯 Strengths
🔍 Observations & Suggestions
|
Pull Request Review: Phase 3 TypeScript MigrationThank you for this comprehensive Phase 3 TypeScript migration! I've reviewed the changes and here's my detailed feedback: ✅ Strengths
🔍 Areas for Improvement
🐛 Potential Issues
📝 Minor Suggestions
✅ Security ReviewThe security improvements are solid:
🚀 Performance Considerations
RecommendationThis PR shows excellent work on the TypeScript migration with strong security improvements. However, I recommend either:
Once this decision is addressed, the PR is ready to merge. The TypeScript migration is well-executed, and the security enhancements add significant value. Great work on maintaining the high quality throughout this migration! 👏 🤖 Generated with Claude Code Co-Authored-By: Claude [email protected] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (14)
package/utils/errorHelpers.ts (1)
69-75: Safer narrowing for NodeJS errno errors — good changeCasting to NodeJS.ErrnoException for .code is the right direction and avoids any. Consider extracting a tiny helper (hasErrorCode) to reuse in isFileNotFoundError / isModuleNotFoundError and reduce duplication, but not blocking.
package/environments/test.js (1)
1-5: Lint noise on compiled headers"use strict" and __esModule headers are triggering lint/prettier. If these files are generated outputs, add package/environments/*.js to .eslintignore; otherwise, run yarn lint --fix to match repo style (semi, spacing).
package.json (1)
37-39: Pre-publish workflow: add clean and prefer prepackTo avoid stale JS/DTs on publish and to run for both npm pack and publish, consider:
- Use prepack instead of prepublishOnly.
- Clean before build.
"scripts": { ... - "type-check": "tsc --noEmit", - "prepublishOnly": "yarn build && yarn type-check" + "type-check": "tsc --noEmit", + "prepack": "yarn clean:ts && yarn build && yarn type-check" }If you rely on prepublishOnly specifically, keep both:
+ "prepublishOnly": "yarn prepack"package/environments/base.js (3)
16-30: Directory walk: avoid following symlinks to prevent cyclesWalking nested entries can recurse into symlink loops. Skip symbolic links when includeNested is true.
- return readdirSync(dir, { withFileTypes: true }).flatMap((dirent) => { + return readdirSync(dir, { withFileTypes: true }).flatMap((dirent) => { const filePath = join(dir, dirent.name); - if (dirent.isDirectory() && includeNested) { + if (dirent.isSymbolicLink()) return []; + if (dirent.isDirectory() && includeNested) { return getFilesInDirectory(filePath, includeNested); } if (dirent.isFile()) { return filePath; } return []; });
45-61: Minor: namespace calc and entry de-duplication
- relative(join(rootPath), …) can be relative(rootPath, …).
- Consider de-duplicating when multiple extensions map to same entry.
-const namespace = relative(join(rootPath), dirname(path)); +const namespace = relative(rootPath, dirname(path)); ... -const previousPaths = entries[name]; +const previousPaths = entries[name]; const pathArray = Array.isArray(previousPaths) ? previousPaths : (previousPaths ? [previousPaths] : []); if (!pathArray.includes(assetPath)) pathArray.push(assetPath); entries[name] = pathArray.length === 1 ? pathArray[0] : pathArray;
72-103: Style/lint: large set of prettier/eslint diffs pendingCI hints show many purely formatting errors in this file. Please run yarn lint --fix or update the ignore list if these are generated artifacts. No behavior changes needed.
package/config.ts (3)
157-164: Keep legacy alias in sync using accessor propertyCurrent defineProperty copies the value once; subsequent updates can diverge. An accessor keeps both fields in sync.
-Object.defineProperty(config, 'webpack_loader', { - value: config.javascript_transpiler, - writable: true, - enumerable: true, - configurable: true -}) +Object.defineProperty(config, 'webpack_loader', { + get() { return this.javascript_transpiler }, + set(v) { this.javascript_transpiler = v as any }, + enumerable: true, + configurable: true +})
62-77: Final config validation opportunityYou import isValidConfig but don’t use it. After merging defaults and computing derived fields, validate once to fail fast on broken installs.
-// After merging with defaults, config should be complete -// Use type assertion only after validation -config = mergedConfig as Config +// After merging with defaults, validate final shape +if (!isValidConfig(mergedConfig)) { + throw createConfigValidationError(configPath, railsEnv, "Incomplete configuration after merging defaults") +} +config = mergedConfig
133-155: Log the active JavaScript transpiler at startup
Documentation in CHANGELOG.md, README.md and docs/v9_upgrade.md already covers the SWC default andwebpack_loaderdeprecation—no further changelog/README updates needed. Add a one‐time INFO log immediately after settingconfig.javascript_transpilerin package/config.ts to surface the effective transpiler and aid debugging.package/environments/development.ts (3)
27-35: Avoid duplicate webpackDevServerConfig() calls and tighten the conditionCompute once and use optional chaining; this keeps behavior while simplifying control flow.
-const webpackDevConfig = (): WebpackConfigWithDevServer => { - const webpackConfig: WebpackConfigWithDevServer = { - ...baseDevConfig, - ...(runningWebpackDevServer && { devServer: webpackDevServerConfig() }) - } - - const devServerConfig = webpackDevServerConfig() +const webpackDevConfig = (): WebpackConfigWithDevServer => { + const devServerConfig = runningWebpackDevServer ? webpackDevServerConfig() : undefined + const webpackConfig: WebpackConfigWithDevServer = { + ...baseDevConfig, + ...(devServerConfig && { devServer: devServerConfig }) + } - if ( - runningWebpackDevServer && - devServerConfig.hot && + if ( + devServerConfig?.hot && moduleExists("@pmmmwh/react-refresh-webpack-plugin") ) {Also applies to: 33-38
10-13: Don’t redeclare plugins; rely on webpack’s typed plugins arrayWebpackConfiguration already defines plugins as WebpackPluginInstance[]. Redeclaring as any[] widens types unnecessarily.
interface WebpackConfigWithDevServer extends WebpackConfiguration { devServer?: DevServerConfiguration - plugins?: any[] }
15-20: Optional: add Rspack types for better safetyIf @rspack/core types are available, prefer them over a hand-rolled interface to catch mistakes early. Otherwise, keep as-is.
+// If available: +// import type { Configuration as RspackConfiguration } from "@rspack/core" - interface RspackConfigWithDevServer { - mode?: string - devtool?: string | false | undefined - devServer?: any - plugins?: any[] - } +// interface RspackConfigWithDevServer extends RspackConfiguration { +// devServer?: any +// }Please confirm whether @rspack/core is already a dependency; if not, we can defer.
package/utils/pathValidation.ts (2)
37-45: Optional: clarify base containment check edgesstartsWith(normalizedBase + path.sep) is fine for typical cases; if you want to be extra cautious on Windows casing or base having a trailing sep, consider normalizing both with a trailing separator and comparing case-insensitively on win32.
- if (!resolved.startsWith(normalizedBase + path.sep) && resolved !== normalizedBase) { + const baseWithSep = normalizedBase.endsWith(path.sep) ? normalizedBase : normalizedBase + path.sep + const lhs = process.platform === 'win32' ? resolved.toLowerCase() : resolved + const rhs = process.platform === 'win32' ? baseWithSep.toLowerCase() : baseWithSep + if (!lhs.startsWith(rhs) && lhs !== rhs.slice(0, -1)) {Please verify this doesn’t affect any existing tests on non-Windows CI.
66-89: Logging is helpful; consider a capped warn to avoid noisy loopsIf paths is large and many entries are invalid, logs can get noisy. Consider counting and summarizing after the loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
package.json(1 hunks)package/config.ts(1 hunks)package/env.ts(1 hunks)package/environments/base.js(1 hunks)package/environments/development.js(1 hunks)package/environments/development.ts(1 hunks)package/environments/production.js(1 hunks)package/environments/production.ts(1 hunks)package/environments/test.js(1 hunks)package/environments/test.ts(1 hunks)package/utils/errorHelpers.ts(1 hunks)package/utils/pathValidation.ts(1 hunks)package/utils/typeGuards.ts(5 hunks)test/typescript/environments.test.js(1 hunks)test/typescript/pathValidation.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: This gem supports both webpack and rspack configurations
Applied to files:
package/environments/production.jspackage/config.tspackage/environments/test.jspackage/environments/production.tspackage/environments/test.tspackage/environments/development.tspackage/environments/development.jspackage/environments/base.js
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
Applied to files:
package/config.tspackage/environments/test.jspackage/environments/test.tspackage/environments/development.tspackage/environments/development.js
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Always run `yarn lint` before committing JavaScript changes
Applied to files:
package.json
🧬 Code graph analysis (13)
package/environments/production.js (1)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)baseConfig(72-102)config(7-7)
package/env.ts (2)
package/environments/base.js (4)
require(4-4)require(5-5)require(8-8)require(10-10)package/utils/pathValidation.ts (1)
sanitizeEnvValue(94-108)
package/config.ts (1)
package/types.ts (1)
Config(13-43)
package/environments/test.js (2)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)config(7-7)baseConfig(72-102)package/environments/development.js (6)
require(3-3)require(7-7)require(8-8)config(4-4)baseConfig(5-5)bundlerConfig(55-55)
package/utils/pathValidation.ts (1)
lib/shakapacker/dev_server.rb (1)
port(29-31)
package/environments/production.ts (2)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)baseConfig(72-102)config(7-7)test/typescript/environments.test.js (1)
productionConfig(4-4)
test/typescript/environments.test.js (2)
test/typescript/pathValidation.test.js (1)
require(2-8)package/rspack/index.js (1)
devServer(12-12)
package/environments/test.ts (2)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)config(7-7)baseConfig(72-102)package/environments/test.js (6)
require(3-3)config(4-4)baseConfig(5-5)rspackTestConfig(6-13)webpackTestConfig(14-14)bundlerConfig(15-15)
test/typescript/pathValidation.test.js (1)
package/utils/pathValidation.ts (5)
isPathTraversalSafe(11-23)safeResolvePath(30-49)validatePaths(66-89)sanitizeEnvValue(94-108)validatePort(113-126)
package/environments/development.ts (2)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)config(7-7)baseConfig(72-102)package/environments/development.js (16)
require(3-3)require(7-7)require(8-8)config(4-4)baseConfig(5-5)webpackDevServerConfig(6-6)baseDevConfig(9-12)webpackDevConfig(13-30)webpackConfig(14-17)devServerConfig(18-18)devServerConfig(32-32)ReactRefreshWebpackPlugin(23-23)rspackDevConfig(31-54)rspackConfig(33-42)ReactRefreshPlugin(47-47)bundlerConfig(55-55)
package/utils/typeGuards.ts (1)
package/utils/pathValidation.ts (2)
isPathTraversalSafe(11-23)validatePort(113-126)
package/environments/development.js (3)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)config(7-7)baseConfig(72-102)package/environments/production.js (6)
require(5-5)require(6-6)require(8-8)require(11-11)config(9-9)baseConfig(7-7)package/environments/test.js (4)
require(3-3)config(4-4)baseConfig(5-5)bundlerConfig(15-15)
package/environments/base.js (3)
package/environments/development.js (5)
require(3-3)require(7-7)require(8-8)config(4-4)baseConfig(5-5)package/environments/production.js (6)
require(5-5)require(6-6)require(8-8)require(11-11)config(9-9)baseConfig(7-7)package/environments/test.js (3)
require(3-3)config(4-4)baseConfig(5-5)
🪛 ESLint
package/environments/production.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 4-4: Delete ;
(prettier/prettier)
[error] 5-5: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
[error] 7-7: Delete ;
(prettier/prettier)
[error] 8-8: Delete ;
(prettier/prettier)
[error] 9-9: Expected 1 empty line after require statement not followed by another require.
(import/newline-after-import)
[error] 9-9: Delete ;
(prettier/prettier)
[error] 10-10: Replace __dirname,·"..",·"optimization",·${config.assets_bundler}.js); with ⏎··__dirname,⏎··"..",⏎··"optimization",⏎··${config.assets_bundler}.js⏎)
(prettier/prettier)
[error] 11-11: Delete ;
(prettier/prettier)
[error] 12-12: Delete ;
(prettier/prettier)
[error] 14-14: Expected indentation of 2 spaces but found 4.
(indent)
[error] 14-14: Delete ··
(prettier/prettier)
[error] 15-15: Replace ····CompressionPlugin·=·require("compression-webpack-plugin"); with ··CompressionPlugin·=·require("compression-webpack-plugin")
(prettier/prettier)
[error] 15-15: Expected indentation of 2 spaces but found 4.
(indent)
[error] 18-18: Expected indentation of 2 spaces but found 4.
(indent)
[error] 18-18: Replace ··const·plugins·=·[]; with const·plugins·=·[]
(prettier/prettier)
[error] 19-19: Expected indentation of 2 spaces but found 4.
(indent)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 20-20: Replace ········plugins.push( with ····plugins.push(⏎······
(prettier/prettier)
[error] 20-20: Expected indentation of 4 spaces but found 8.
(indent)
[error] 21-21: Delete ····
(prettier/prettier)
[error] 21-21: Expected indentation of 6 spaces but found 12.
(indent)
[error] 22-22: Replace ············ with ········
(prettier/prettier)
[error] 22-22: Expected indentation of 6 spaces but found 12.
(indent)
[error] 23-23: Delete ····
(prettier/prettier)
[error] 23-23: Expected indentation of 6 spaces but found 12.
(indent)
[error] 24-24: Replace ········})); with ······})
(prettier/prettier)
[error] 24-24: Expected indentation of 4 spaces but found 8.
(indent)
[error] 25-25: Expected indentation of 4 spaces but found 8.
(indent)
[error] 25-25: Insert )⏎
(prettier/prettier)
[error] 26-26: Replace ············plugins.push( with ······plugins.push(⏎········
(prettier/prettier)
[error] 26-26: Expected indentation of 6 spaces but found 12.
(indent)
[error] 27-27: Replace ················ with ··········
(prettier/prettier)
[error] 27-27: Expected indentation of 8 spaces but found 16.
(indent)
[error] 28-28: Delete ······
(prettier/prettier)
[error] 28-28: Expected indentation of 8 spaces but found 16.
(indent)
[error] 29-29: Expected indentation of 8 spaces but found 16.
(indent)
[error] 29-29: Delete ······
(prettier/prettier)
[error] 30-30: Replace ············})); with ········})
(prettier/prettier)
[error] 30-30: Expected indentation of 6 spaces but found 12.
(indent)
[error] 31-31: Expected indentation of 4 spaces but found 8.
(indent)
[error] 31-31: Replace ··} with )
(prettier/prettier)
[error] 32-32: Expected indentation of 2 spaces but found 4.
(indent)
[error] 33-33: Expected indentation of 2 spaces but found 4.
(indent)
[error] 33-33: Replace ··return·plugins; with }⏎··return·plugins
(prettier/prettier)
[error] 34-34: Delete ;
(prettier/prettier)
[error] 36-36: Delete ··
(prettier/prettier)
[error] 36-36: Expected indentation of 2 spaces but found 4.
(indent)
[error] 37-37: Expected indentation of 2 spaces but found 4.
(indent)
[error] 37-37: Delete ··
(prettier/prettier)
[error] 38-38: Delete ··
(prettier/prettier)
[error] 38-38: Expected indentation of 2 spaces but found 4.
(indent)
[error] 39-39: Expected indentation of 2 spaces but found 4.
(indent)
[error] 39-39: Delete ··
(prettier/prettier)
[error] 40-40: Replace ···· with ··
(prettier/prettier)
[error] 40-40: Expected indentation of 2 spaces but found 4.
(indent)
[error] 41-41: Delete ;
(prettier/prettier)
[error] 43-43: Replace ···· with ··
(prettier/prettier)
[error] 43-43: Expected indentation of 2 spaces but found 4.
(indent)
[error] 44-44: Delete ··
(prettier/prettier)
[error] 44-44: Expected indentation of 2 spaces but found 4.
(indent)
[error] 47-47: Delete ;
(prettier/prettier)
[error] 49-49: Delete ;
(prettier/prettier)
package/environments/test.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 2-2: Delete ;
(prettier/prettier)
[error] 3-3: Delete ;
(prettier/prettier)
[error] 4-4: Delete ;
(prettier/prettier)
[error] 5-5: Expected 1 empty line after require statement not followed by another require.
(import/newline-after-import)
[error] 5-5: Delete ;
(prettier/prettier)
[error] 7-7: Delete ··
(prettier/prettier)
[error] 7-7: Expected indentation of 2 spaces but found 4.
(indent)
[error] 8-8: Expected indentation of 2 spaces but found 4.
(indent)
[error] 8-8: Delete ··
(prettier/prettier)
[error] 9-9: Replace ···· with ··
(prettier/prettier)
[error] 9-9: Expected indentation of 2 spaces but found 4.
(indent)
[error] 10-10: Delete ··
(prettier/prettier)
[error] 10-10: Expected indentation of 2 spaces but found 4.
(indent)
[error] 11-11: Expected indentation of 4 spaces but found 8.
(indent)
[error] 11-11: Delete ····
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 12-12: Expected indentation of 2 spaces but found 4.
(indent)
[error] 13-13: Delete ;
(prettier/prettier)
[error] 14-14: Delete ;
(prettier/prettier)
[error] 15-15: Replace ·config.assets_bundler·===·"rspack"·?·rspackTestConfig()·:·webpackTestConfig(); with ⏎··config.assets_bundler·===·"rspack"·?·rspackTestConfig()·:·webpackTestConfig()
(prettier/prettier)
[error] 16-16: Delete ;
(prettier/prettier)
test/typescript/environments.test.js
[error] 4-4: Delete ··
(prettier/prettier)
[error] 61-63: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 62-62: Avoid calling expect conditionally`
(jest/no-conditional-expect)
[error] 73-73: describes should begin with lowercase
(jest/prefer-lowercase-title)
[error] 76-76: Delete ······
(prettier/prettier)
[error] 77-77: Replace config with (config)
(prettier/prettier)
[error] 86-97: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 87-87: Use object destructuring.
(prefer-destructuring)
[error] 88-88: Avoid calling expect conditionally`
(jest/no-conditional-expect)
[error] 89-89: Delete ········
(prettier/prettier)
[error] 90-96: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 91-95: Avoid calling expect conditionally`
(jest/no-conditional-expect)
[error] 92-94: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 92-93: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 92-94: Replace ·⏎············typeof·devServer.port·===·"string"·||·⏎ with ⏎··············typeof·devServer.port·===·"string"·||⏎··
(prettier/prettier)
[error] 100-100: Insert ⏎
(prettier/prettier)
test/typescript/pathValidation.test.js
[error] 23-23: Replace path with (path)
(prettier/prettier)
[error] 36-36: Replace path with (path)
(prettier/prettier)
[error] 47-47: Delete ······
(prettier/prettier)
[error] 55-55: Delete ······
(prettier/prettier)
[error] 65-65: Delete ······
(prettier/prettier)
[error] 66-70: Replace ⏎········"src/index.js",⏎········"../etc/passwd",⏎········"components/App.tsx"⏎······ with "src/index.js",·"../etc/passwd",·"components/App.tsx"
(prettier/prettier)
[error] 71-71: Delete ······
(prettier/prettier)
[error] 73-73: Delete ······
(prettier/prettier)
[error] 78-78: Delete ······
(prettier/prettier)
[error] 87-87: Delete ······
(prettier/prettier)
[error] 93-93: Delete ······
(prettier/prettier)
[error] 95-95: Delete ······
(prettier/prettier)
[error] 99-99: Delete ······
(prettier/prettier)
[error] 130-130: Insert ⏎
(prettier/prettier)
package/environments/development.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 2-2: Delete ;
(prettier/prettier)
[error] 3-3: Delete ;
(prettier/prettier)
[error] 4-4: Delete ;
(prettier/prettier)
[error] 5-5: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
[error] 7-7: Delete ;
(prettier/prettier)
[error] 8-8: Expected 1 empty line after require statement not followed by another require.
(import/newline-after-import)
[error] 8-8: Delete ;
(prettier/prettier)
[error] 10-10: Delete ··
(prettier/prettier)
[error] 10-10: Expected indentation of 2 spaces but found 4.
(indent)
[error] 11-11: Expected indentation of 2 spaces but found 4.
(indent)
[error] 11-11: Delete ··
(prettier/prettier)
[error] 12-12: Delete ;
(prettier/prettier)
[error] 14-14: Expected indentation of 2 spaces but found 4.
(indent)
[error] 14-14: Delete ··
(prettier/prettier)
[error] 15-15: Expected indentation of 4 spaces but found 8.
(indent)
[error] 15-15: Delete ····
(prettier/prettier)
[error] 16-16: Delete ····
(prettier/prettier)
[error] 16-16: Expected indentation of 4 spaces but found 8.
(indent)
[error] 17-17: Replace ····}; with ··}
(prettier/prettier)
[error] 17-17: Expected indentation of 2 spaces but found 4.
(indent)
[error] 18-18: Replace ····const·devServerConfig·=·webpackDevServerConfig(); with ··const·devServerConfig·=·webpackDevServerConfig()
(prettier/prettier)
[error] 18-18: Expected indentation of 2 spaces but found 4.
(indent)
[error] 19-19: Replace ····if·( with ··if·(⏎····
(prettier/prettier)
[error] 19-19: Expected indentation of 2 spaces but found 4.
(indent)
[error] 20-20: Replace ········ with ····
(prettier/prettier)
[error] 21-21: Replace ········moduleExists("@pmmmwh/react-refresh-webpack-plugin"))·{ with ····moduleExists("@pmmmwh/react-refresh-webpack-plugin")
(prettier/prettier)
[error] 22-22: Expected indentation of 4 spaces but found 8.
(indent)
[error] 22-22: Replace ·· with )·{⏎
(prettier/prettier)
[error] 23-23: Replace ········const·ReactRefreshWebpackPlugin·=·require("@pmmmwh/react-refresh-webpack-plugin"); with ····const·ReactRefreshWebpackPlugin·=·require("@pmmmwh/react-refresh-webpack-plugin")
(prettier/prettier)
[error] 23-23: Expected indentation of 4 spaces but found 8.
(indent)
[error] 24-24: Expected indentation of 4 spaces but found 8.
(indent)
[error] 24-24: Delete ····
(prettier/prettier)
[error] 25-25: Replace ············ with ······
(prettier/prettier)
[error] 25-25: Expected indentation of 6 spaces but found 12.
(indent)
[error] 26-26: Replace ············ with ······
(prettier/prettier)
[error] 26-26: Expected indentation of 6 spaces but found 12.
(indent)
[error] 27-27: Replace ········]; with ····]
(prettier/prettier)
[error] 27-27: Expected indentation of 4 spaces but found 8.
(indent)
[error] 28-28: Expected indentation of 2 spaces but found 4.
(indent)
[error] 28-28: Delete ··
(prettier/prettier)
[error] 29-29: Replace ····return·webpackConfig; with ··return·webpackConfig
(prettier/prettier)
[error] 29-29: Expected indentation of 2 spaces but found 4.
(indent)
[error] 30-30: Delete ;
(prettier/prettier)
[error] 32-32: Expected indentation of 2 spaces but found 4.
(indent)
[error] 32-32: Replace ··const·devServerConfig·=·webpackDevServerConfig(); with const·devServerConfig·=·webpackDevServerConfig()
(prettier/prettier)
[error] 33-33: Expected indentation of 2 spaces but found 4.
(indent)
[error] 33-33: Delete ··
(prettier/prettier)
[error] 34-34: Delete ····
(prettier/prettier)
[error] 34-34: Expected indentation of 4 spaces but found 8.
(indent)
[error] 35-35: Expected indentation of 4 spaces but found 8.
(indent)
[error] 35-35: Delete ····
(prettier/prettier)
[error] 36-36: Replace ············ with ······
(prettier/prettier)
[error] 36-36: Expected indentation of 6 spaces but found 12.
(indent)
[error] 37-37: Replace ············ with ······
(prettier/prettier)
[error] 37-37: Expected indentation of 6 spaces but found 12.
(indent)
[error] 38-38: Replace ················ with ········
(prettier/prettier)
[error] 38-38: Expected indentation of 8 spaces but found 16.
(indent)
[error] 39-39: Replace ················ with ········
(prettier/prettier)
[error] 39-39: Expected indentation of 8 spaces but found 16.
(indent)
[error] 40-40: Expected indentation of 6 spaces but found 12.
(indent)
[error] 40-40: Delete ······
(prettier/prettier)
[error] 41-41: Expected indentation of 4 spaces but found 8.
(indent)
[error] 41-41: Replace ·· with }⏎
(prettier/prettier)
[error] 42-42: Expected indentation of 2 spaces but found 4.
(indent)
[error] 42-42: Replace ··}; with if·(
(prettier/prettier)
[error] 43-43: Expected indentation of 2 spaces but found 4.
(indent)
[error] 43-43: Delete if·(
(prettier/prettier)
[error] 44-44: Replace ········ with ····
(prettier/prettier)
[error] 45-45: Replace ········moduleExists("@rspack/plugin-react-refresh") with ····moduleExists("@rspack/plugin-react-refresh")⏎··
(prettier/prettier)
[error] 46-46: Delete ····
(prettier/prettier)
[error] 46-46: Expected indentation of 4 spaces but found 8.
(indent)
[error] 47-47: Expected indentation of 4 spaces but found 8.
(indent)
[error] 47-47: Replace ····const·ReactRefreshPlugin·=·require("@rspack/plugin-react-refresh"); with const·ReactRefreshPlugin·=·require("@rspack/plugin-react-refresh")
(prettier/prettier)
[error] 48-48: Delete ····
(prettier/prettier)
[error] 48-48: Expected indentation of 4 spaces but found 8.
(indent)
[error] 49-49: Replace ············ with ······
(prettier/prettier)
[error] 49-49: Expected indentation of 6 spaces but found 12.
(indent)
[error] 50-50: Expected indentation of 6 spaces but found 12.
(indent)
[error] 50-50: Delete ······
(prettier/prettier)
[error] 51-51: Replace ········]; with ····]
(prettier/prettier)
[error] 51-51: Expected indentation of 4 spaces but found 8.
(indent)
[error] 52-52: Replace ···· with ··
(prettier/prettier)
[error] 52-52: Expected indentation of 2 spaces but found 4.
(indent)
[error] 53-53: Replace ····return·rspackConfig; with ··return·rspackConfig
(prettier/prettier)
[error] 53-53: Expected indentation of 2 spaces but found 4.
(indent)
[error] 54-54: Delete ;
(prettier/prettier)
[error] 55-55: Replace ·config.assets_bundler·===·"rspack"·?·rspackDevConfig()·:·webpackDevConfig(); with ⏎··config.assets_bundler·===·"rspack"·?·rspackDevConfig()·:·webpackDevConfig()
(prettier/prettier)
[error] 56-56: Delete ;
(prettier/prettier)
package/environments/base.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 4-4: Delete ;
(prettier/prettier)
[error] 5-5: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
[error] 7-7: Delete ;
(prettier/prettier)
[error] 8-8: Expected 1 empty line after require statement not followed by another require.
(import/newline-after-import)
[error] 8-8: Delete ;
(prettier/prettier)
[error] 9-9: Replace __dirname,·"..",·"plugins",·${config.assets_bundler}.js); with ⏎··__dirname,⏎··"..",⏎··"plugins",⏎··${config.assets_bundler}.js⏎)
(prettier/prettier)
[error] 10-10: Delete ;
(prettier/prettier)
[error] 11-11: Replace __dirname,·"..",·"rules",·${config.assets_bundler}.js); with ⏎··__dirname,⏎··"..",⏎··"rules",⏎··${config.assets_bundler}.js⏎)
(prettier/prettier)
[error] 12-12: Delete ;
(prettier/prettier)
[error] 15-15: Delete ;
(prettier/prettier)
[error] 17-17: Expected indentation of 2 spaces but found 4.
(indent)
[error] 17-17: Delete ··
(prettier/prettier)
[error] 18-18: Replace ········return·[]; with ····return·[]
(prettier/prettier)
[error] 18-18: Expected indentation of 4 spaces but found 8.
(indent)
[error] 19-19: Expected indentation of 2 spaces but found 4.
(indent)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 20-20: Expected indentation of 2 spaces but found 4.
(indent)
[error] 21-21: Replace ········const·filePath·=·join(dir,·dirent.name); with ····const·filePath·=·join(dir,·dirent.name)
(prettier/prettier)
[error] 21-21: Expected indentation of 4 spaces but found 8.
(indent)
[error] 22-22: Delete ····
(prettier/prettier)
[error] 22-22: Expected indentation of 4 spaces but found 8.
(indent)
[error] 23-23: Replace ············return·getFilesInDirectory(filePath,·includeNested); with ······return·getFilesInDirectory(filePath,·includeNested)
(prettier/prettier)
[error] 23-23: Expected indentation of 6 spaces but found 12.
(indent)
[error] 24-24: Delete ····
(prettier/prettier)
[error] 24-24: Expected indentation of 4 spaces but found 8.
(indent)
[error] 25-25: Delete ····
(prettier/prettier)
[error] 25-25: Expected indentation of 4 spaces but found 8.
(indent)
[error] 26-26: Replace ············return·filePath; with ······return·filePath
(prettier/prettier)
[error] 26-26: Expected indentation of 6 spaces but found 12.
(indent)
[error] 27-27: Expected indentation of 4 spaces but found 8.
(indent)
[error] 27-27: Delete ····
(prettier/prettier)
[error] 28-28: Replace ········return·[]; with ····return·[]
(prettier/prettier)
[error] 28-28: Expected indentation of 4 spaces but found 8.
(indent)
[error] 29-29: Replace ····}); with ··})
(prettier/prettier)
[error] 29-29: Expected indentation of 2 spaces but found 4.
(indent)
[error] 30-30: Delete ;
(prettier/prettier)
[error] 32-32: Expected indentation of 2 spaces but found 4.
(indent)
[error] 32-32: Replace ··const·entries·=·{}; with const·entries·=·{}
(prettier/prettier)
[error] 33-33: Expected indentation of 2 spaces but found 4.
(indent)
[error] 33-33: Replace ··const·rootPath·=·join(config.source_path,·config.source_entry_path); with const·rootPath·=·join(config.source_path,·config.source_entry_path)
(prettier/prettier)
[error] 34-34: Expected indentation of 2 spaces but found 4.
(indent)
[error] 34-34: Delete ··
(prettier/prettier)
[error] 35-35: Replace ········throw·new·Error( with ····throw·new·Error(⏎······
(prettier/prettier)
[error] 35-35: Expected indentation of 4 spaces but found 8.
(indent)
[error] 36-36: Delete ····
(prettier/prettier)
[error] 37-37: Replace ············ with ········
(prettier/prettier)
[error] 38-38: Delete ····
(prettier/prettier)
[error] 39-39: Replace ············ with ········
(prettier/prettier)
[error] 40-40: Replace ············ with ········
(prettier/prettier)
[error] 41-41: Delete ····
(prettier/prettier)
[error] 42-42: Replace ····3.·Or·use·both·options·for·better·organization·of·your·entry·points); with 3.·Or·use·both·options·for·better·organization·of·your·entry·points
(prettier/prettier)
[error] 43-43: Expected indentation of 2 spaces but found 4.
(indent)
[error] 43-43: Replace } with )
(prettier/prettier)
[error] 44-44: Expected indentation of 2 spaces but found 4.
(indent)
[error] 44-44: Insert }⏎
(prettier/prettier)
[error] 45-45: Expected indentation of 4 spaces but found 8.
(indent)
[error] 45-45: Replace ····const·namespace·=·relative(join(rootPath),·dirname(path)); with const·namespace·=·relative(join(rootPath),·dirname(path))
(prettier/prettier)
[error] 46-46: Replace ········const·name·=·join(namespace,·basename(path,·extname(path))); with ····const·name·=·join(namespace,·basename(path,·extname(path)))
(prettier/prettier)
[error] 46-46: Expected indentation of 4 spaces but found 8.
(indent)
[error] 47-47: Replace ········const·assetPath·=·resolve(path); with ····const·assetPath·=·resolve(path)
(prettier/prettier)
[error] 47-47: Expected indentation of 4 spaces but found 8.
(indent)
[error] 48-48: Replace ········ with ····
(prettier/prettier)
[error] 48-48: Expected indentation of 4 spaces but found 8.
(indent)
[error] 49-49: Delete ····
(prettier/prettier)
[error] 49-49: Expected indentation of 4 spaces but found 8.
(indent)
[error] 50-50: Expected indentation of 4 spaces but found 8.
(indent)
[error] 50-50: Replace ····const·previousPaths·=·entries[name]; with const·previousPaths·=·entries[name]
(prettier/prettier)
[error] 51-51: Expected indentation of 4 spaces but found 8.
(indent)
[error] 51-51: Delete ····
(prettier/prettier)
[error] 52-52: Delete ······
(prettier/prettier)
[error] 52-52: Expected indentation of 6 spaces but found 12.
(indent)
[error] 53-53: Replace ················ with ········
(prettier/prettier)
[error] 53-53: Expected indentation of 8 spaces but found 16.
(indent)
[error] 54-54: Replace ················:·[previousPaths]; with ········:·[previousPaths]
(prettier/prettier)
[error] 54-54: Expected indentation of 8 spaces but found 16.
(indent)
[error] 55-55: Replace ············pathArray.push(assetPath); with ······pathArray.push(assetPath)
(prettier/prettier)
[error] 55-55: Expected indentation of 6 spaces but found 12.
(indent)
[error] 56-56: Expected indentation of 6 spaces but found 12.
(indent)
[error] 56-56: Replace ······entries[name]·=·pathArray; with entries[name]·=·pathArray
(prettier/prettier)
[error] 57-58: Replace ········}⏎······· with ····}
(prettier/prettier)
[error] 57-57: Expected indentation of 4 spaces but found 8.
(indent)
[error] 58-58: Expected indentation of 4 spaces but found 8.
(indent)
[error] 59-59: Replace ············entries[name]·=·assetPath; with ······entries[name]·=·assetPath
(prettier/prettier)
[error] 59-59: Expected indentation of 6 spaces but found 12.
(indent)
[error] 60-60: Replace ········ with ····
(prettier/prettier)
[error] 60-60: Expected indentation of 4 spaces but found 8.
(indent)
[error] 61-61: Replace ····}); with ··})
(prettier/prettier)
[error] 61-61: Expected indentation of 2 spaces but found 4.
(indent)
[error] 62-62: Expected indentation of 2 spaces but found 4.
(indent)
[error] 62-62: Replace ··return·entries; with return·entries
(prettier/prettier)
[error] 63-63: Delete ;
(prettier/prettier)
[error] 65-65: Replace ····const·result·=·[resolve(config.source_path)]; with ··const·result·=·[resolve(config.source_path)]
(prettier/prettier)
[error] 65-65: Expected indentation of 2 spaces but found 4.
(indent)
[error] 66-66: Delete ··
(prettier/prettier)
[error] 66-66: Expected indentation of 2 spaces but found 4.
(indent)
[error] 67-67: Replace ········config.additional_paths.forEach((path)·=>·result.push(resolve(path))); with ····config.additional_paths.forEach((path)·=>·result.push(resolve(path)))
(prettier/prettier)
[error] 67-67: Expected indentation of 4 spaces but found 8.
(indent)
[error] 68-68: Delete ··
(prettier/prettier)
[error] 68-68: Expected indentation of 2 spaces but found 4.
(indent)
[error] 69-69: Replace ····result.push("node_modules"); with ··result.push("node_modules")
(prettier/prettier)
[error] 69-69: Expected indentation of 2 spaces but found 4.
(indent)
[error] 70-70: Replace ····return·result; with ··return·result
(prettier/prettier)
[error] 70-70: Expected indentation of 2 spaces but found 4.
(indent)
[error] 71-71: Delete ;
(prettier/prettier)
[error] 73-73: Delete ··
(prettier/prettier)
[error] 73-73: Expected indentation of 2 spaces but found 4.
(indent)
[error] 74-74: Expected indentation of 2 spaces but found 4.
(indent)
[error] 74-74: Delete ··
(prettier/prettier)
[error] 75-75: Replace ········ with ····
(prettier/prettier)
[error] 75-75: Expected indentation of 4 spaces but found 8.
(indent)
[error] 76-76: Replace ········ with ····
(prettier/prettier)
[error] 76-76: Expected indentation of 4 spaces but found 8.
(indent)
[error] 77-77: Delete ····
(prettier/prettier)
[error] 77-77: Expected indentation of 4 spaces but found 8.
(indent)
[error] 78-78: Expected indentation of 4 spaces but found 8.
(indent)
[error] 78-78: Delete ····
(prettier/prettier)
[error] 79-79: Replace ········ with ····
(prettier/prettier)
[error] 79-79: Expected indentation of 4 spaces but found 8.
(indent)
[error] 80-80: Replace ········ with ····
(prettier/prettier)
[error] 80-80: Expected indentation of 4 spaces but found 8.
(indent)
[error] 81-81: Delete ····
(prettier/prettier)
[error] 81-81: Expected indentation of 4 spaces but found 8.
(indent)
[error] 82-82: Expected indentation of 4 spaces but found 8.
(indent)
[error] 82-82: Replace ····crossOriginLoading: with crossOriginLoading:⏎·····
(prettier/prettier)
[error] 83-83: Replace ············ with ········
(prettier/prettier)
[error] 83-83: Expected indentation of 6 spaces but found 12.
(indent)
[error] 84-84: Expected indentation of 6 spaces but found 12.
(indent)
[error] 84-84: Delete ····
(prettier/prettier)
[error] 85-85: Expected indentation of 2 spaces but found 4.
(indent)
[error] 85-85: Delete ··
(prettier/prettier)
[error] 86-86: Delete ··
(prettier/prettier)
[error] 86-86: Expected indentation of 2 spaces but found 4.
(indent)
[error] 87-87: Expected indentation of 2 spaces but found 4.
(indent)
[error] 87-87: Delete ··
(prettier/prettier)
[error] 88-88: Delete ····
(prettier/prettier)
[error] 88-88: Expected indentation of 4 spaces but found 8.
(indent)
[error] 89-89: Expected indentation of 4 spaces but found 8.
(indent)
[error] 89-89: Delete ····
(prettier/prettier)
[error] 90-90: Replace ···· with ··
(prettier/prettier)
[error] 90-90: Expected indentation of 2 spaces but found 4.
(indent)
[error] 91-91: Delete ··
(prettier/prettier)
[error] 91-91: Expected indentation of 2 spaces but found 4.
(indent)
[error] 92-92: Expected indentation of 2 spaces but found 4.
(indent)
[error] 92-92: Delete ··
(prettier/prettier)
[error] 93-93: Replace ········ with ····
(prettier/prettier)
[error] 93-93: Expected indentation of 4 spaces but found 8.
(indent)
[error] 94-94: Delete ··
(prettier/prettier)
[error] 94-94: Expected indentation of 2 spaces but found 4.
(indent)
[error] 95-95: Expected indentation of 2 spaces but found 4.
(indent)
[error] 95-95: Delete ··
(prettier/prettier)
[error] 96-96: Delete ····
(prettier/prettier)
[error] 96-96: Expected indentation of 4 spaces but found 8.
(indent)
[error] 97-97: Expected indentation of 4 spaces but found 8.
(indent)
[error] 97-97: Delete ····
(prettier/prettier)
[error] 98-98: Replace ···· with ··
(prettier/prettier)
[error] 98-98: Expected indentation of 2 spaces but found 4.
(indent)
[error] 99-99: Delete ··
(prettier/prettier)
[error] 99-99: Expected indentation of 2 spaces but found 4.
(indent)
[error] 100-100: Replace ········ with ····
(prettier/prettier)
[error] 100-100: Expected indentation of 4 spaces but found 8.
(indent)
[error] 101-101: Expected indentation of 2 spaces but found 4.
(indent)
[error] 101-101: Delete ··
(prettier/prettier)
[error] 102-102: Delete ;
(prettier/prettier)
[error] 103-103: Delete ;
(prettier/prettier)
🪛 Biome (2.1.2)
package/utils/pathValidation.ts
[error] 19-19: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 98-98: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 98-98: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🪛 GitHub Check: Linting
package/environments/base.js
[failure] 9-9:
Replace __dirname,·"..",·"plugins",·${config.assets_bundler}.js); with ⏎··__dirname,⏎··"..",⏎··"plugins",⏎··${config.assets_bundler}.js⏎)
[failure] 8-8:
Delete ;
[failure] 8-8:
Expected 1 empty line after require statement not followed by another require
[failure] 7-7:
Delete ;
[failure] 6-6:
Delete ;
[failure] 5-5:
Delete ;
[failure] 4-4:
Delete ;
[failure] 1-1:
Delete ;
[failure] 1-1:
Expected newline after "use strict" directive
[failure] 1-1:
'use strict' is unnecessary inside of modules
⏰ 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). (12)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.1.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 20.x)
- GitHub Check: Testing (ubuntu-latest, 18.x)
- GitHub Check: Testing (ubuntu-latest, 22.x)
- GitHub Check: test
- GitHub Check: claude-review
🔇 Additional comments (3)
package/environments/base.js (2)
34-43: Good guard against accidental all-files entriesThe explicit error when source_entry_path is "/" and nested_entries is true prevents catastrophic entry explosion. Nice.
Please add a unit covering this guard if not already present (expected to throw for "/" + nested_entries: true).
15-15: ignore function-type guard: isProduction is a boolean
isProductionis defined inpackage/env.tsasnodeEnv === "production", not a function, so the current truthiness check is correct and no changes are needed.Likely an incorrect or invalid review comment.
package/environments/development.ts (1)
79-83: Verify HMR integration for both bundlers
Please manually test:
- Run the webpack dev server with HMR enabled and confirm React Fast Refresh works.
- Run the rspack dev server with HMR enabled and confirm React Fast Refresh works.
- Disable HMR on each server to ensure no refresh plugin is injected.
- Build in production mode and verify there’s no
devServerkey in the webpack config.
Code Review for Phase 3 TypeScript MigrationGreat work on completing Phase 3 of the TypeScript migration! This PR shows solid progress with meaningful security improvements and addresses the feedback from Phase 2 comprehensively. Here's my detailed review: ✅ StrengthsSecurity Enhancements
Type Safety Improvements
Code Quality
Test Coverage
🔍 Areas for ConsiderationPerformance
Breaking Changes
💡 Suggestions for Future Improvements
🚦 Overall AssessmentThis is a high-quality PR that successfully completes Phase 3 of the TypeScript migration. The security improvements are particularly noteworthy and add real value beyond just the TypeScript conversion. The code maintains backward compatibility while modernizing the codebase. Recommendation: APPROVED ✅ The PR is ready to merge. The consistent use of SWC as the default transpiler is a good decision for performance and maintenance. Great job addressing all the Phase 2 feedback! |
Code Review for Phase 3 TypeScript MigrationI've thoroughly reviewed PR #614 and here's my comprehensive feedback: ✅ Strengths
🔍 Areas for Consideration
💡 Minor Suggestions
🎯 Overall AssessmentThis is a high-quality PR that successfully completes Phase 3 of the TypeScript migration. The security improvements are particularly impressive, and the attention to backward compatibility is commendable. The code follows the project's conventions (proper use of The migration maintains full backward compatibility while introducing valuable type safety and security improvements. The consistent use of SWC as the default transpiler should provide better build performance. Recommendation: ✅ Approved - This PR is ready to merge. Great work on addressing all the feedback from Phase 2! 🤖 Generated with Claude Code Co-Authored-By: Claude [email protected] |
Code Review: Phase 3 TypeScript MigrationThank you for this comprehensive TypeScript migration! I've reviewed the changes and here's my feedback: ✅ StrengthsSecurity Improvements:
Type Safety:
Performance:
Testing:
|
Code Review for Phase 3 TypeScript MigrationThank you for this comprehensive PR completing Phase 3 of the TypeScript migration! I've thoroughly reviewed the changes and here's my feedback: ✅ Strengths1. Excellent Security Improvements
2. Strong Type Safety Enhancements
3. Performance Optimizations
4. Comprehensive Test Coverage
|
Code Review for PR #614: Phase 3 TypeScript MigrationThank you for this comprehensive Phase 3 TypeScript migration! I've conducted a thorough review of the changes, and overall this is excellent work that significantly improves the codebase. Here's my detailed feedback: 🎯 StrengthsSecurity Improvements ✅
TypeScript Implementation ✅
Performance Optimizations ✅
Testing ✅
|
Code Review: Phase 3 TypeScript MigrationThank you for completing Phase 3 of the TypeScript migration! I've conducted a thorough review of the changes, particularly focusing on the environment file conversions and security improvements from Phase 2 feedback. ✅ Strengths
🔍 Minor Suggestions for Consideration
✅ Breaking Changes AssessmentAs stated, there are no breaking changes. The TypeScript compilation maintains full backward compatibility, and the change to use SWC as the default transpiler for both webpack and rspack is a sensible consistency improvement. ✅ Security ValidationThe security improvements effectively address the Phase 2 feedback:
✅ Overall AssessmentThis is a high-quality PR that successfully completes Phase 3 of the TypeScript migration. The code is production-ready, secure, and maintains backward compatibility while adding significant type safety benefits. The attention to security concerns from Phase 2 is particularly commendable. Verdict: APPROVED ✅ Great work on this migration phase! The code quality is excellent, and the TypeScript conversion adds significant value without introducing risk. |
🔍 Code Review for Phase 3 TypeScript MigrationI've completed a thorough review of this PR. Here's my detailed feedback: ✅ Strengths
🐛 Potential Issues
💡 Suggestions
🔒 Security ReviewThe security improvements are well-implemented:
📋 TestingThe test suite additions are comprehensive:
🎯 Code Quality
✨ Breaking Changes AssessmentAs stated in the PR description, there are no breaking changes for users, which is excellent. The only behavioral change is making SWC the default transpiler for both webpack and rspack for consistency, which is well-documented. 📝 Final VerdictAPPROVED ✅ This is a high-quality PR that successfully completes Phase 3 of the TypeScript migration. The code is well-structured, secure, and maintains backward compatibility while adding valuable improvements. The security enhancements are particularly noteworthy and add significant value to the project. Great work on incorporating all the feedback from Phase 2! The migration path is clear and the documentation will help users adopt the new features smoothly. 🤖 Generated with Claude Code Co-Authored-By: Claude [email protected] |
Code Review for PR #614: Phase 3 TypeScript MigrationThank you for this comprehensive TypeScript migration PR! The Phase 3 implementation is well-structured and addresses the feedback from Phase 2. Here's my detailed review: ✅ StrengthsSecurity Improvements
Type Safety
Documentation
🔍 Areas for Improvement1. Missing Newline in
|
Code Review for PR #614: Phase 3 TypeScript MigrationThank you for completing Phase 3 of the TypeScript migration! This is a comprehensive and well-structured PR that successfully migrates environment configuration files to TypeScript while addressing security concerns from the Phase 2 review. ✅ Strengths1. Excellent Security Improvements
2. Type Safety Enhancements
3. Performance Optimizations
4. Testing Coverage
🔍 Minor Suggestions for Improvement1. Missing Trailing Newlines
2. Documentation Enhancement
3. Type Safety in Error Handling } catch (error) {
const errorMessage = error instanceof Error ? error.message : String(error)
console.warn(
`[SHAKAPACKER WARNING] Invalid path configuration: ${userPath}\n` +
`Error: ${errorMessage}`
)
}4. Cache Debug Logging ✅ Security ReviewThe security implementations are robust and follow best practices:
✅ Performance ConsiderationsThe performance optimizations are well thought out:
✅ Backward CompatibilityExcellent handling of backward compatibility:
📊 Overall AssessmentThis PR successfully completes Phase 3 of the TypeScript migration with high quality code. The security improvements alone make this a valuable contribution. The TypeScript migration is clean, maintains backward compatibility, and adds significant value through improved type safety and validation. Recommendation: ✅ Approve (after verifying trailing newlines per project requirements) Great work on this migration phase! The attention to security, performance, and developer experience is commendable. 🤖 Generated with Claude Code Co-Authored-By: Claude [email protected] |
Code Review for PR #614: Complete Phase 3 TypeScript migration - Environment filesThank you for this comprehensive PR completing Phase 3 of the TypeScript migration! I've thoroughly reviewed the changes and here's my feedback: ✅ Strengths
🔍 Areas for Improvement
🚀 Performance Considerations
🛡️ Security AssessmentThe security improvements are robust:
One suggestion: Consider adding rate limiting or maximum path length validation to prevent potential DoS through extremely long path strings. 📋 Minor Issues
✨ Overall AssessmentThis is a high-quality PR that successfully completes Phase 3 of the TypeScript migration while adding significant security improvements. The code is well-structured, properly tested, and maintains backward compatibility. The attention to security details is particularly commendable. Recommendation: ✅ Approve with minor suggestions The PR is ready to merge after considering:
Excellent work on maintaining quality while migrating to TypeScript! The security enhancements add real value beyond just the type migration. 👏 |
🔍 Code Review for PR #614: Phase 3 TypeScript MigrationThank you for this comprehensive TypeScript migration PR! I've thoroughly reviewed the changes and here's my feedback: ✅ Strengths1. Excellent Security Improvements 🔒
2. Type Safety Enhancements
3. Performance Optimizations ⚡
4. Backward Compatibility
🎯 Areas for Improvement1. Missing Newline in .npmignore FilesBoth 2. Transpiler Default ChangeThe PR description mentions making SWC the default for both webpack and rspack, but the code shows:
This discrepancy should be clarified in the PR description or the implementation should be updated to match. 3. Documentation PlacementThe 🐛 Potential Issues1. Cache Invalidation Edge CaseIn 2. Error Handling in safeResolvePathThe error message in 3. Test Coverage GapThe TypeScript environment tests don't verify the actual TypeScript compilation or type checking, only that the modules export valid objects. 💡 Suggestions1. Consider Adding Integration TestsAdd tests that verify the TypeScript files compile correctly and produce the expected JavaScript output. 2. Enhanced Type ExportsConsider exporting more granular types from 3. Migration ScriptA migration script to help users automatically update their configurations from Phase 2 to Phase 3 would be helpful. 🔧 Security Recommendations
✨ Overall AssessmentThis is a high-quality PR that successfully completes Phase 3 of the TypeScript migration with significant improvements to security, type safety, and performance. The code is well-structured, properly tested, and maintains backward compatibility. Recommendation: Approve with minor fixes ✅ The only blocking issue is the missing trailing newlines in the Great work on addressing all the feedback from Phase 2 and adding comprehensive security hardening! 🎉 Review generated with attention to CLAUDE.md guidelines and project conventions |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
.npmignore (1)
33-36: Consider including source maps for better debugging experience.While excluding source maps reduces package size, including them (especially for a build tooling package like shakapacker) can significantly improve the debugging experience for users troubleshooting issues. The comment already notes this is optional—you may want to remove these exclusions for the initial release and monitor package size impact.
test/package/transpiler-defaults.test.js (2)
2-2: Remove unused import.The
resolvefunction frompathis imported but never used in the test file.Apply this diff:
-const { resolve } = require("path") +// Path utilities not needed in this test file
83-98: Strengthen the deprecation warning test.Line 95's assertion
expect(consoleSpy.mock.calls.length >= 0).toBe(true)always passes. Since the comment notes "we can't easily mock the YAML config here," consider either verifying the deprecation mechanism exists or documenting why this test is a placeholder.Apply one of these approaches:
Option A (document limitation):
- // The warning is shown during config loading if webpack_loader is detected - // Since we can't easily mock the YAML config here, we check the mechanism exists - expect(consoleSpy.mock.calls.length >= 0).toBe(true) + // Note: This test verifies the console.warn spy is available. + // Testing actual webpack_loader deprecation warnings requires YAML config mocking + // which is deferred to integration tests. + expect(consoleSpy).toBeDefined()Option B (test the actual config.ts deprecation path):
+ // Verify the config module has deprecation handling + const config = require("../../package/config") + // If webpack_loader exists in config, a warning should have been logged + if (config.webpack_loader) { + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining("webpack_loader") + ) + } - expect(consoleSpy.mock.calls.length >= 0).toBe(true)test/typescript/pathValidation.test.js (1)
42-44: Fix variable shadowing.The iterator variable
safePathshould use parentheses for consistency with the previous fix, and the lambda parameter naming is clear.Apply this diff:
- safePaths.forEach(safePath => { + safePaths.forEach((safePath) => { expect(isPathTraversalSafe(safePath)).toBe(true) })package/environments/development.js (1)
1-71: Compiled output: Run Prettier to fix formatting.This file contains extensive formatting and style violations flagged by ESLint/Prettier (indentation, semicolons, spacing). Since this is compiled JavaScript from the TypeScript source, run your formatter to auto-fix these issues:
npx prettier --write package/environments/development.jspackage/utils/typeGuards.ts (1)
28-38:clearValidationCacheis a no-op due to WeakMap limitations.The comment correctly notes that
WeakMapdoesn't have aclear()method and relies on garbage collection. However, exporting a function that does nothing (except log in debug mode) can mislead callers into thinking cache is actually cleared. Consider one of these approaches:
- Remove the export if cache clearing isn't essential (WeakMap will GC naturally).
- Switch to Map if you need explicit cache clearing (though this loses automatic GC).
- Document clearly in JSDoc that this is a debug-only no-op.
If you choose to keep it, add explicit JSDoc:
/** * Clear the validation cache - * Useful for testing or when config files change + * NOTE: This is a no-op for WeakMap-based cache. The cache will be + * garbage-collected when config objects are no longer referenced. + * Provided for debugging and API compatibility only. */ export function clearValidationCache(): void { - // WeakMap doesn't have a clear method, but we can create a new one - // This is handled by garbage collection if (debugCache) { - console.log('[SHAKAPACKER DEBUG] Validation cache cleared') + console.log('[SHAKAPACKER DEBUG] Cache clear requested (no-op for WeakMap)') } }package/environments/development.ts (1)
31-49: RedundantwebpackDevServerConfig()call.
webpackDevServerConfig()is called twice whenrunningWebpackDevServeris true:
- Line 34: conditionally spreads the result into
devServer- Line 37: calls it again to check
hotThis is wasteful (though not incorrect). Refactor to call once and reuse:
const webpackDevConfig = (): WebpackConfigWithDevServer => { + const devServerConfig = runningWebpackDevServer ? webpackDevServerConfig() : null const webpackConfig: WebpackConfigWithDevServer = { ...baseDevConfig, - ...(runningWebpackDevServer && { devServer: webpackDevServerConfig() }) + ...(devServerConfig && { devServer: devServerConfig }) } - const devServerConfig = webpackDevServerConfig() if ( runningWebpackDevServer && - devServerConfig.hot && + devServerConfig?.hot && moduleExists("@pmmmwh/react-refresh-webpack-plugin") ) { // eslint-disable-next-line global-require const ReactRefreshWebpackPlugin = require("@pmmmwh/react-refresh-webpack-plugin") webpackConfig.plugins = [ ...(webpackConfig.plugins || []), new ReactRefreshWebpackPlugin() ] } return webpackConfig }package/environments/types.ts (4)
12-15: Consider removing the redundantpluginsfield.The
pluginsfield is already defined inWebpackConfigurationasWebpackPluginInstance[] | undefined. Redeclaring it here is redundant, though not harmful.Apply this diff if you prefer a more minimal interface:
export interface WebpackConfigWithDevServer extends WebpackConfiguration { devServer?: DevServerConfiguration - plugins?: WebpackPluginInstance[] }
21-26: Replaceanywithunknownfor safer typing.The interface uses
anyin three places (constructor args, compiler parameter, and index signature). While this provides maximum flexibility for rspack compatibility, usingunknownwould be safer and still allow consumers to narrow types as needed. This aligns with the PR's goal of improving type safety.Apply this diff:
export interface RspackPlugin { - new(...args: any[]): { - apply(compiler: any): void - [key: string]: any + new(...args: unknown[]): { + apply(compiler: unknown): void + [key: string]: unknown } }
51-62: Consider allowing both rspack and webpack plugin types.The
pluginsfield is typed asRspackPlugin[], but many webpack plugins are compatible with rspack. Consider allowing both types for better interoperability.Based on learnings
Apply this diff:
export interface RspackConfigWithDevServer { mode?: "development" | "production" | "none" devtool?: string | false devServer?: RspackDevServerConfig - plugins?: RspackPlugin[] + plugins?: Array<RspackPlugin | WebpackPluginInstance> module?: WebpackConfiguration["module"] resolve?: WebpackConfiguration["resolve"] entry?: WebpackConfiguration["entry"] output?: WebpackConfiguration["output"] optimization?: WebpackConfiguration["optimization"] [key: string]: unknown }
67-74: Fix the redundantalgorithmtype union.The type
string | "gzip" | "brotliCompress"is redundant since the literal types are already strings. Either use juststringfor flexibility, or use a strict union"gzip" | "brotliCompress"for type safety.Apply this diff for better type safety:
export interface CompressionPluginOptions { filename: string - algorithm: string | "gzip" | "brotliCompress" + algorithm: "gzip" | "brotliCompress" test: RegExp threshold?: number minRatio?: number deleteOriginalAssets?: boolean }Or use
stringif other algorithms should be supported:- algorithm: string | "gzip" | "brotliCompress" + algorithm: string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.npmignore(1 hunks)docs/transpiler-migration.md(1 hunks)package/.npmignore(1 hunks)package/config.ts(1 hunks)package/environments/development.js(1 hunks)package/environments/development.ts(1 hunks)package/environments/production.js(1 hunks)package/environments/production.ts(1 hunks)package/environments/test.js(1 hunks)package/environments/test.ts(1 hunks)package/environments/types.js(1 hunks)package/environments/types.ts(1 hunks)package/utils/pathValidation.ts(1 hunks)package/utils/typeGuards.ts(5 hunks)test/package/transpiler-defaults.test.js(1 hunks)test/typescript/environments.test.js(1 hunks)test/typescript/pathValidation.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package/.npmignore
🚧 Files skipped from review as they are similar to previous changes (1)
- package/environments/production.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: This gem supports both webpack and rspack configurations
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: This gem supports both webpack and rspack configurations
Applied to files:
docs/transpiler-migration.mdpackage/config.tspackage/environments/types.tspackage/environments/test.tspackage/environments/production.jspackage/environments/test.jspackage/environments/development.tspackage/environments/development.js
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
Applied to files:
package/config.tspackage/environments/test.tstest/typescript/environments.test.jstest/package/transpiler-defaults.test.jspackage/environments/test.jspackage/environments/development.tspackage/environments/development.js
🧬 Code graph analysis (8)
package/config.ts (1)
package/types.ts (1)
Config(13-43)
package/environments/test.ts (2)
package/environments/test.js (7)
require(7-7)config(8-8)baseConfig(9-9)sharedTestConfig(14-21)rspackTestConfig(26-29)webpackTestConfig(34-37)bundlerConfig(38-38)package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)config(7-7)baseConfig(72-102)
package/environments/production.js (1)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)baseConfig(72-102)config(7-7)
package/utils/typeGuards.ts (1)
package/utils/pathValidation.ts (2)
isPathTraversalSafe(11-26)validatePort(121-139)
package/environments/test.js (3)
package/environments/production.js (6)
require(9-9)require(10-10)require(12-12)require(15-15)config(13-13)baseConfig(11-11)package/environments/development.js (6)
require(7-7)require(11-11)require(12-12)config(8-8)baseConfig(9-9)bundlerConfig(70-70)package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)config(7-7)baseConfig(72-102)
package/environments/development.ts (2)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)config(7-7)baseConfig(72-102)package/environments/types.ts (2)
WebpackConfigWithDevServer(12-15)RspackConfigWithDevServer(51-62)
package/environments/development.js (2)
package/environments/test.js (4)
require(7-7)config(8-8)baseConfig(9-9)bundlerConfig(38-38)package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)config(7-7)baseConfig(72-102)
test/typescript/pathValidation.test.js (1)
package/utils/pathValidation.ts (5)
isPathTraversalSafe(11-26)safeResolvePath(33-52)validatePaths(69-92)sanitizeEnvValue(97-116)validatePort(121-139)
🪛 ESLint
package/environments/production.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
[error] 9-9: Delete ;
(prettier/prettier)
[error] 10-10: Delete ;
(prettier/prettier)
[error] 11-11: Delete ;
(prettier/prettier)
[error] 12-12: Delete ;
(prettier/prettier)
[error] 13-13: Expected 1 empty line after require statement not followed by another require.
(import/newline-after-import)
[error] 13-13: Delete ;
(prettier/prettier)
[error] 14-14: Replace __dirname,·"..",·"optimization",·${config.assets_bundler}.js); with ⏎··__dirname,⏎··"..",⏎··"optimization",⏎··${config.assets_bundler}.js⏎)
(prettier/prettier)
[error] 15-15: Delete ;
(prettier/prettier)
[error] 16-16: Delete ;
(prettier/prettier)
[error] 18-18: Expected indentation of 2 spaces but found 4.
(indent)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 19-19: Replace ····CompressionPlugin·=·require("compression-webpack-plugin"); with ··CompressionPlugin·=·require("compression-webpack-plugin")
(prettier/prettier)
[error] 19-19: Expected indentation of 2 spaces but found 4.
(indent)
[error] 26-26: Expected indentation of 2 spaces but found 4.
(indent)
[error] 26-26: Replace ··const·plugins·=·[]; with const·plugins·=·[]
(prettier/prettier)
[error] 27-27: Delete ··
(prettier/prettier)
[error] 27-27: Expected indentation of 2 spaces but found 4.
(indent)
[error] 28-28: Replace ········plugins.push( with ····plugins.push(⏎······
(prettier/prettier)
[error] 28-28: Expected indentation of 4 spaces but found 8.
(indent)
[error] 29-29: Delete ····
(prettier/prettier)
[error] 29-29: Expected indentation of 6 spaces but found 12.
(indent)
[error] 30-30: Replace ············ with ········
(prettier/prettier)
[error] 30-30: Expected indentation of 6 spaces but found 12.
(indent)
[error] 31-31: Delete ····
(prettier/prettier)
[error] 31-31: Expected indentation of 6 spaces but found 12.
(indent)
[error] 32-32: Replace ········})); with ······})
(prettier/prettier)
[error] 32-32: Expected indentation of 4 spaces but found 8.
(indent)
[error] 33-33: Expected indentation of 4 spaces but found 8.
(indent)
[error] 33-33: Insert )⏎
(prettier/prettier)
[error] 34-34: Replace ············plugins.push( with ······plugins.push(⏎········
(prettier/prettier)
[error] 34-34: Expected indentation of 6 spaces but found 12.
(indent)
[error] 35-35: Replace ················ with ··········
(prettier/prettier)
[error] 35-35: Expected indentation of 8 spaces but found 16.
(indent)
[error] 36-36: Delete ······
(prettier/prettier)
[error] 36-36: Expected indentation of 8 spaces but found 16.
(indent)
[error] 37-37: Expected indentation of 8 spaces but found 16.
(indent)
[error] 37-37: Delete ······
(prettier/prettier)
[error] 38-38: Replace ············})); with ········})
(prettier/prettier)
[error] 38-38: Expected indentation of 6 spaces but found 12.
(indent)
[error] 39-39: Expected indentation of 4 spaces but found 8.
(indent)
[error] 39-39: Replace ··} with )
(prettier/prettier)
[error] 40-40: Expected indentation of 2 spaces but found 4.
(indent)
[error] 41-41: Expected indentation of 2 spaces but found 4.
(indent)
[error] 41-41: Replace ··return·plugins; with }⏎··return·plugins
(prettier/prettier)
[error] 42-42: Delete ;
(prettier/prettier)
[error] 47-47: Delete ··
(prettier/prettier)
[error] 47-47: Expected indentation of 2 spaces but found 4.
(indent)
[error] 48-48: Expected indentation of 2 spaces but found 4.
(indent)
[error] 48-48: Delete ··
(prettier/prettier)
[error] 49-49: Delete ··
(prettier/prettier)
[error] 49-49: Expected indentation of 2 spaces but found 4.
(indent)
[error] 50-50: Expected indentation of 2 spaces but found 4.
(indent)
[error] 50-50: Delete ··
(prettier/prettier)
[error] 51-51: Replace ···· with ··
(prettier/prettier)
[error] 51-51: Expected indentation of 2 spaces but found 4.
(indent)
[error] 52-52: Delete ;
(prettier/prettier)
[error] 54-54: Replace ···· with ··
(prettier/prettier)
[error] 54-54: Expected indentation of 2 spaces but found 4.
(indent)
[error] 55-55: Delete ··
(prettier/prettier)
[error] 55-55: Expected indentation of 2 spaces but found 4.
(indent)
[error] 58-58: Delete ;
(prettier/prettier)
[error] 60-60: Delete ;
(prettier/prettier)
test/typescript/environments.test.js
[error] 4-4: Delete ··
(prettier/prettier)
[error] 72-72: describes should begin with lowercase
(jest/prefer-lowercase-title)
[error] 75-75: Delete ······
(prettier/prettier)
[error] 76-76: Replace config with (config)
(prettier/prettier)
[error] 87-87: Use object destructuring.
(prefer-destructuring)
[error] 88-88: Delete ······
(prettier/prettier)
[error] 89-99: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 90-90: Avoid calling expect conditionally`
(jest/no-conditional-expect)
[error] 91-91: Delete ········
(prettier/prettier)
[error] 93-98: Avoid calling expect conditionally`
(jest/no-conditional-expect)
[error] 94-97: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 94-96: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 94-95: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 95-97: Replace typeof·devServer.port·===·"number"·||·⏎··········typeof·devServer.port·===·"string"·||·⏎ with ··typeof·devServer.port·===·"number"·||⏎············typeof·devServer.port·===·"string"·||⏎··
(prettier/prettier)
[error] 102-102: Insert ⏎
(prettier/prettier)
test/package/transpiler-defaults.test.js
[error] 2-2: 'resolve' is assigned a value but never used.
(no-unused-vars)
[error] 6-6: Delete ··
(prettier/prettier)
[error] 10-10: Delete ····
(prettier/prettier)
[error] 14-14: Delete ··
(prettier/prettier)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 25-25: Delete ······
(prettier/prettier)
[error] 28-28: Delete ······
(prettier/prettier)
[error] 34-34: Delete ····
(prettier/prettier)
[error] 38-38: Delete ······
(prettier/prettier)
[error] 41-41: Delete ······
(prettier/prettier)
[error] 45-45: Delete ··
(prettier/prettier)
[error] 50-50: Delete ······
(prettier/prettier)
[error] 53-53: Delete ······
(prettier/prettier)
[error] 57-57: Delete ····
(prettier/prettier)
[error] 61-61: Delete ······
(prettier/prettier)
[error] 64-64: Delete ······
(prettier/prettier)
[error] 68-68: Delete ··
(prettier/prettier)
[error] 73-73: Delete ······
(prettier/prettier)
[error] 76-76: Delete ······
(prettier/prettier)
[error] 82-82: Delete ····
(prettier/prettier)
[error] 85-85: Delete ······
(prettier/prettier)
[error] 89-89: Delete ······
(prettier/prettier)
[error] 92-92: Delete ······
(prettier/prettier)
[error] 96-96: Delete ······
(prettier/prettier)
[error] 100-100: Delete ··
(prettier/prettier)
[error] 104-104: Delete ······
(prettier/prettier)
[error] 107-107: Delete ······
(prettier/prettier)
[error] 110-110: Delete ····
(prettier/prettier)
[error] 112-112: Delete ·
(prettier/prettier)
[error] 114-114: Delete ······
(prettier/prettier)
[error] 115-115: Delete ·
(prettier/prettier)
[error] 120-120: Delete ······
(prettier/prettier)
[error] 128-128: Insert ⏎
(prettier/prettier)
package/environments/test.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
[error] 7-7: Delete ;
(prettier/prettier)
[error] 8-8: Delete ;
(prettier/prettier)
[error] 9-9: Delete ;
(prettier/prettier)
[error] 15-15: Expected indentation of 2 spaces but found 4.
(indent)
[error] 15-15: Delete ··
(prettier/prettier)
[error] 16-16: Delete ··
(prettier/prettier)
[error] 16-16: Expected indentation of 2 spaces but found 4.
(indent)
[error] 17-17: Expected indentation of 2 spaces but found 4.
(indent)
[error] 17-17: Delete ··
(prettier/prettier)
[error] 18-18: Expected indentation of 2 spaces but found 4.
(indent)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 19-19: Delete ····
(prettier/prettier)
[error] 19-19: Expected indentation of 4 spaces but found 8.
(indent)
[error] 20-20: Expected indentation of 2 spaces but found 4.
(indent)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Delete ;
(prettier/prettier)
[error] 27-27: Delete ··
(prettier/prettier)
[error] 27-27: Expected indentation of 2 spaces but found 4.
(indent)
[error] 28-28: Expected indentation of 2 spaces but found 4.
(indent)
[error] 28-28: Delete ··
(prettier/prettier)
[error] 29-29: Delete ;
(prettier/prettier)
[error] 35-35: Replace ···· with ··
(prettier/prettier)
[error] 35-35: Expected indentation of 2 spaces but found 4.
(indent)
[error] 36-36: Delete ··
(prettier/prettier)
[error] 36-36: Expected indentation of 2 spaces but found 4.
(indent)
[error] 37-37: Delete ;
(prettier/prettier)
[error] 38-38: Replace ·config.assets_bundler·===·"rspack"·?·rspackTestConfig()·:·webpackTestConfig(); with ⏎··config.assets_bundler·===·"rspack"·?·rspackTestConfig()·:·webpackTestConfig()
(prettier/prettier)
[error] 39-39: Delete ;
(prettier/prettier)
package/environments/development.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
[error] 7-7: Delete ;
(prettier/prettier)
[error] 8-8: Delete ;
(prettier/prettier)
[error] 9-9: Delete ;
(prettier/prettier)
[error] 10-10: Delete ;
(prettier/prettier)
[error] 11-11: Delete ;
(prettier/prettier)
[error] 12-12: Delete ;
(prettier/prettier)
[error] 17-17: Expected indentation of 2 spaces but found 4.
(indent)
[error] 17-17: Delete ··
(prettier/prettier)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 18-18: Expected indentation of 2 spaces but found 4.
(indent)
[error] 19-19: Delete ;
(prettier/prettier)
[error] 25-25: Expected indentation of 2 spaces but found 4.
(indent)
[error] 25-25: Delete ··
(prettier/prettier)
[error] 26-26: Expected indentation of 4 spaces but found 8.
(indent)
[error] 26-26: Delete ····
(prettier/prettier)
[error] 27-27: Delete ····
(prettier/prettier)
[error] 27-27: Expected indentation of 4 spaces but found 8.
(indent)
[error] 28-28: Replace ····}; with ··}
(prettier/prettier)
[error] 28-28: Expected indentation of 2 spaces but found 4.
(indent)
[error] 29-29: Replace ····const·devServerConfig·=·webpackDevServerConfig(); with ··const·devServerConfig·=·webpackDevServerConfig()
(prettier/prettier)
[error] 29-29: Expected indentation of 2 spaces but found 4.
(indent)
[error] 30-30: Replace ····if·( with ··if·(⏎····
(prettier/prettier)
[error] 30-30: Expected indentation of 2 spaces but found 4.
(indent)
[error] 31-31: Replace ········ with ····
(prettier/prettier)
[error] 32-32: Replace ········moduleExists("@pmmmwh/react-refresh-webpack-plugin"))·{ with ····moduleExists("@pmmmwh/react-refresh-webpack-plugin")
(prettier/prettier)
[error] 33-33: Expected indentation of 4 spaces but found 8.
(indent)
[error] 33-33: Replace ·· with )·{⏎
(prettier/prettier)
[error] 34-34: Replace ········const·ReactRefreshWebpackPlugin·=·require("@pmmmwh/react-refresh-webpack-plugin"); with ····const·ReactRefreshWebpackPlugin·=·require("@pmmmwh/react-refresh-webpack-plugin")
(prettier/prettier)
[error] 34-34: Expected indentation of 4 spaces but found 8.
(indent)
[error] 35-35: Expected indentation of 4 spaces but found 8.
(indent)
[error] 35-35: Delete ····
(prettier/prettier)
[error] 36-36: Replace ············ with ······
(prettier/prettier)
[error] 36-36: Expected indentation of 6 spaces but found 12.
(indent)
[error] 37-37: Replace ············ with ······
(prettier/prettier)
[error] 37-37: Expected indentation of 6 spaces but found 12.
(indent)
[error] 38-38: Replace ········]; with ····]
(prettier/prettier)
[error] 38-38: Expected indentation of 4 spaces but found 8.
(indent)
[error] 39-39: Replace ···· with ··
(prettier/prettier)
[error] 39-39: Expected indentation of 2 spaces but found 4.
(indent)
[error] 40-40: Replace ····return·webpackConfig; with ··return·webpackConfig
(prettier/prettier)
[error] 40-40: Expected indentation of 2 spaces but found 4.
(indent)
[error] 41-41: Delete ;
(prettier/prettier)
[error] 47-47: Expected indentation of 2 spaces but found 4.
(indent)
[error] 47-47: Replace ··const·devServerConfig·=·webpackDevServerConfig(); with const·devServerConfig·=·webpackDevServerConfig()
(prettier/prettier)
[error] 48-48: Expected indentation of 2 spaces but found 4.
(indent)
[error] 48-48: Delete ··
(prettier/prettier)
[error] 49-49: Delete ····
(prettier/prettier)
[error] 49-49: Expected indentation of 4 spaces but found 8.
(indent)
[error] 50-50: Expected indentation of 4 spaces but found 8.
(indent)
[error] 50-50: Delete ····
(prettier/prettier)
[error] 51-51: Replace ············ with ······
(prettier/prettier)
[error] 51-51: Expected indentation of 6 spaces but found 12.
(indent)
[error] 52-52: Replace ············ with ······
(prettier/prettier)
[error] 52-52: Expected indentation of 6 spaces but found 12.
(indent)
[error] 53-53: Replace ················ with ········
(prettier/prettier)
[error] 53-53: Expected indentation of 8 spaces but found 16.
(indent)
[error] 54-54: Replace ················ with ········
(prettier/prettier)
[error] 54-54: Expected indentation of 8 spaces but found 16.
(indent)
[error] 55-55: Expected indentation of 6 spaces but found 12.
(indent)
[error] 55-55: Delete ······
(prettier/prettier)
[error] 56-56: Expected indentation of 4 spaces but found 8.
(indent)
[error] 56-56: Replace ·· with }⏎
(prettier/prettier)
[error] 57-57: Expected indentation of 2 spaces but found 4.
(indent)
[error] 57-57: Replace ··}; with if·(
(prettier/prettier)
[error] 58-58: Expected indentation of 2 spaces but found 4.
(indent)
[error] 58-58: Delete if·(
(prettier/prettier)
[error] 59-59: Replace ········ with ····
(prettier/prettier)
[error] 60-60: Replace ········moduleExists("@rspack/plugin-react-refresh") with ····moduleExists("@rspack/plugin-react-refresh")⏎··
(prettier/prettier)
[error] 61-61: Delete ····
(prettier/prettier)
[error] 61-61: Expected indentation of 4 spaces but found 8.
(indent)
[error] 62-62: Expected indentation of 4 spaces but found 8.
(indent)
[error] 62-62: Replace ····const·ReactRefreshPlugin·=·require("@rspack/plugin-react-refresh"); with const·ReactRefreshPlugin·=·require("@rspack/plugin-react-refresh")
(prettier/prettier)
[error] 63-63: Delete ····
(prettier/prettier)
[error] 63-63: Expected indentation of 4 spaces but found 8.
(indent)
[error] 64-64: Replace ············ with ······
(prettier/prettier)
[error] 64-64: Expected indentation of 6 spaces but found 12.
(indent)
[error] 65-65: Expected indentation of 6 spaces but found 12.
(indent)
[error] 65-65: Delete ······
(prettier/prettier)
[error] 66-66: Replace ········]; with ····]
(prettier/prettier)
[error] 66-66: Expected indentation of 4 spaces but found 8.
(indent)
[error] 67-67: Replace ···· with ··
(prettier/prettier)
[error] 67-67: Expected indentation of 2 spaces but found 4.
(indent)
[error] 68-68: Replace ····return·rspackConfig; with ··return·rspackConfig
(prettier/prettier)
[error] 68-68: Expected indentation of 2 spaces but found 4.
(indent)
[error] 69-69: Delete ;
(prettier/prettier)
[error] 70-70: Replace ·config.assets_bundler·===·"rspack"·?·rspackDevConfig()·:·webpackDevConfig(); with ⏎··config.assets_bundler·===·"rspack"·?·rspackDevConfig()·:·webpackDevConfig()
(prettier/prettier)
[error] 71-71: Delete ;
(prettier/prettier)
package/environments/types.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
test/typescript/pathValidation.test.js
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Delete ·····
(prettier/prettier)
[error] 25-25: Delete ·····
(prettier/prettier)
[error] 26-26: Delete ···
(prettier/prettier)
[error] 29-29: Replace path with (path)
(prettier/prettier)
[error] 29-29: 'path' is already declared in the upper scope on line 2 column 7.
(no-shadow)
[error] 42-42: Replace (safePath with ((safePath)
(prettier/prettier)
[error] 53-53: Delete ······
(prettier/prettier)
[error] 61-61: Delete ······
(prettier/prettier)
[error] 71-71: Delete ······
(prettier/prettier)
[error] 77-77: Delete ······
(prettier/prettier)
[error] 79-79: Delete ······
(prettier/prettier)
[error] 84-84: Delete ······
(prettier/prettier)
[error] 93-93: Delete ······
(prettier/prettier)
[error] 99-99: Delete ······
(prettier/prettier)
[error] 101-101: Delete ······
(prettier/prettier)
[error] 105-105: Delete ······
(prettier/prettier)
[error] 132-132: Delete ·
(prettier/prettier)
[error] 133-133: Delete ·
(prettier/prettier)
[error] 134-134: Delete ···
(prettier/prettier)
[error] 135-135: Delete ···
(prettier/prettier)
[error] 136-136: Delete ···
(prettier/prettier)
[error] 137-137: Delete ··
(prettier/prettier)
[error] 142-142: Insert ⏎
(prettier/prettier)
⏰ 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). (19)
- GitHub Check: test
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: claude-review
🔇 Additional comments (35)
.npmignore (8)
1-4: LGTM! Clear documentation.The header clearly explains the file's purpose and its relationship to .gitignore.
5-21: LGTM! Appropriate development artifact exclusions.Standard exclusions for development, test, and CI files that shouldn't be published to npm.
22-28: LGTM! Ruby files correctly excluded from npm package.Since this npm package contains only the JavaScript tooling portion of shakapacker, Ruby development files are appropriately excluded.
30-31: LGTM! TypeScript config appropriately excluded.tsconfig.json is only needed during development/build and not for package consumers.
37-43: LGTM! Standard git and editor exclusions.These files are development-only and correctly excluded from the published package.
45-48: LGTM! CI/CD configuration files excluded.These CI configuration files are not needed in the published package.
49-51: Documentation exclusion is correct and follows npm best practices.The
docs/directory contains extensive user-facing documentation including migration guides, configuration docs, and troubleshooting guides. However, excluding documentation source files from the published npm package is standard practice, as:
- Documentation is accessible via the repository URL listed in
package.json- It reduces package installation size
- The root
README.md(outsidedocs/) would still be included for essential usage information
52-55: Pattern verified: TypeScript source exclusion is working correctly.The
*.tsfollowed by!*.d.tspattern is functioning as intended. Verification confirms that:
- Source
.tsfiles (config.ts, dev_server.ts, index.ts, etc.) are excluded from the package- Declaration
.d.tsfiles (index.d.ts, loaders.d.ts, webpack-types.d.ts) are correctly included- Only compiled
.jsand.d.tsfiles will be publishedpackage/config.ts (4)
128-136: LGTM! Default bundler and environment override logic is correct.The code properly ensures
assets_bundlerdefaults to "webpack" when not specified, and allows environment variable override. This maintains backward compatibility while providing configuration flexibility.
139-142: LGTM! Clear default transpiler strategy.The comments clearly document the rationale: Babel remains default for webpack (backward compatibility), while SWC is default for rspack (modern defaults for newer bundler).
144-152: LGTM! Well-implemented type guard.The
hasWebpackLoadertype guard correctly checks for the presence and type ofwebpack_loader, following TypeScript best practices for narrowing types safely.
154-165: LGTM! Correct precedence order for transpiler configuration.The logic correctly prioritizes:
- Environment variable override (highest)
- Legacy
webpack_loaderwith deprecation warning- Default based on bundler type (lowest)
This ensures flexibility while maintaining backward compatibility.
package/environments/test.ts (2)
6-9: LGTM: Mixed CommonJS/TypeScript imports are appropriate for Phase 3.The combination of
require()for runtime dependencies andimport typefor type-only imports is the correct pattern for gradual TypeScript migration, ensuring backward compatibility while adding type safety.
23-48: LGTM: Bundler parity achieved via shared configuration.Both
rspackTestConfigandwebpackTestConfigspreadsharedTestConfig, ensuring identical test behavior across bundlers. The return type difference (TestConfigvsPartial<WebpackConfiguration>) is appropriate since webpack's full configuration interface is broader.package/environments/test.js (1)
14-37: Configuration parity successfully implemented.Both
rspackTestConfigandwebpackTestConfignow spreadsharedTestConfig, resolving the previous concern about webpack using production mode while rspack used development mode. Test behavior is now consistent across bundlers.test/typescript/pathValidation.test.js (1)
49-56: Verify cross-platform path assertions.The test uses
path.join()for construction and.toContain()for assertions. On Windows,safeResolvePathreturns paths likeC:\app\src\index.js, while Line 55 converts backslashes inuserPathbut not inbasePath. Consider using a more robust assertion.Run this test on both Unix and Windows to confirm the assertions pass, or refactor to use exact path comparison:
const basePath = path.join(path.sep, "app") const userPath = path.join("src", "index.js") const result = safeResolvePath(basePath, userPath) + const expectedPath = path.resolve(basePath, userPath) - expect(result).toContain(basePath) - expect(result).toContain(userPath.replace(/\\/g, path.sep)) + expect(result).toBe(expectedPath)This ensures the test validates the exact resolved path across platforms.
Based on learnings (test changes with both bundlers when modifying core functionality).
docs/transpiler-migration.md (3)
96-100: LGTM: Performance comparison provides valuable migration context.The performance table offers concrete benchmarks that help users make informed decisions about migrating from Babel to SWC.
121-136: LGTM: Clear rollback procedure reduces migration risk.The rollback plan with explicit commands (
bin/shakapacker clobberandcompile) provides a safety net for users experiencing issues, which is essential for production migrations.
5-8: Documentation is accurate and consistent with implementation.After reviewing the codebase, the documentation correctly reflects the transpiler defaults:
- Webpack: Defaults to
babelfor backward compatibility (package/config.ts:149-152, lib/shakapacker/configuration.rb:142-145)- Rspack: Defaults to
swcfor modern performance with built-in SWC supportThe PR's default configuration file (
lib/install/config/shakapacker.yml:46) explicitly setsjavascript_transpiler: 'swc', which overrides the webpack default. This allows new installations to use SWC regardless of bundler, while the code-level defaults maintain backward compatibility for existing projects.package/utils/typeGuards.ts (4)
49-56: Cache hit logic and TTL implementation look solid.The timestamp-based TTL check correctly returns cached results within the configured window (infinite in prod, 5s in watch, 1min otherwise). The debug logging is helpful for troubleshooting.
70-82: Path traversal security checks strengthen validation.The addition of
isPathTraversalSafechecks for fields containing "path" is a valuable security enhancement. The console warning and negative cache on failure ensure bad configs are caught and not re-validated unnecessarily.
111-118:additional_pathssecurity validation is thorough.Iterating through each path in
additional_pathsand validating withisPathTraversalSafeensures that user-provided path arrays are safe. Good use of early exit and negative caching.
166-168: Delegating port validation tovalidatePortimproves maintainability.Extracting port validation logic into a dedicated utility (
validatePortfrompathValidation.ts) centralizes the validation rules and makes this code cleaner.package/environments/development.ts (2)
58-85: Rspack config structure looks good.The rspack path correctly calls
webpackDevServerConfig()once, spreads it intodevServer, and safely handles thedevMiddlewaremerge. The React Refresh plugin logic mirrors the webpack approach.
87-90: Bundler selection and merge are correct.The ternary correctly selects between rspack and webpack configs based on
config.assets_bundler, andmergeproperly combines withbaseConfig.package/utils/pathValidation.ts (5)
11-26: Path traversal detection is comprehensive and secure.The function correctly:
- Short-circuits on null bytes without regex (Biome-compliant)
- Covers major traversal patterns (relative, absolute POSIX/Windows, UNC, home, URL-encoded)
- Returns early on any match
33-52:safeResolvePathcorrectly prevents directory traversal.The boundary check at line 41 ensures the resolved path either equals the base or is a descendant (starts with
base + path.sep). This prevents both traversal attacks and edge cases.
69-92:validatePathsprovides robust batch validation with clear warnings.The function:
- Validates each path individually
- Warns on unsafe paths (continues without adding)
- Catches
safeResolvePatherrors and warns- Returns only validated paths
This is a safe, production-ready approach to handling user-provided path arrays.
97-116:sanitizeEnvValuecorrectly avoids control character regex.The character-code filter (lines 102-106) removes control characters and DEL without triggering Biome's
noControlCharactersInRegexrule. The warning on sanitization changes is helpful for debugging.
121-139:validatePortenforces strict validation.The function correctly:
- Accepts 'auto' string literal
- Validates numeric ports (range, integer check)
- Requires digit-only strings before parsing (prevents "3000abc" acceptance)
- Validates range after parsing
This addresses the past review concern and is now robust.
test/typescript/environments.test.js (1)
84-100: Eliminate the outer conditional to fixjest/no-conditional-expect.The outer
if (devServer !== undefined)block causes the linter to flagjest/no-conditional-expectbecause anyexpectinside a conditional is problematic—the test can silently pass without asserting anything whendevServeris undefined. Restructure to always run an assertion that explicitly permitsundefined.Apply this diff:
it("validates dev server configuration when present", () => { - // Development config may or may not have devServer depending on environment - // When present, it should be properly configured const devServer = developmentConfig.devServer - if (devServer !== undefined) { - expect(devServer).toBeDefined() - - // Port should be undefined or one of the allowed types (no conditional) - expect( - devServer.port === undefined || - typeof devServer.port === "number" || - typeof devServer.port === "string" || - devServer.port === "auto" - ).toBe(true) - } + // devServer is optional; if present, validate its structure + if (devServer !== undefined) { + expect(devServer).toBeDefined() + const port = devServer.port + expect( + port === undefined || + typeof port === "number" || + typeof port === "string" || + port === "auto" + ).toBe(true) + } else { + // Explicitly handle undefined to avoid silent skip + expect(devServer).toBeUndefined() + } })Alternatively, collapse into a single unconditional assertion:
it("validates dev server configuration when present", () => { const devServer = developmentConfig.devServer - - if (devServer !== undefined) { - expect(devServer).toBeDefined() - - // Port should be undefined or one of the allowed types (no conditional) - expect( - devServer.port === undefined || - typeof devServer.port === "number" || - typeof devServer.port === "string" || - devServer.port === "auto" - ).toBe(true) - } + // Always assert: devServer can be undefined or an object with valid port + expect( + devServer === undefined || + (typeof devServer === "object" && devServer !== null) + ).toBe(true) + + if (devServer !== undefined) { + const port = devServer.port + expect( + port === undefined || + typeof port === "number" || + typeof port === "string" || + port === "auto" + ).toBe(true) + } })Choose the approach that best fits your test philosophy—either always assert on both branches or validate the object type unconditionally before the conditional port check.
package/environments/types.ts (4)
6-7: LGTM!The type-only imports from webpack and webpack-dev-server are appropriate and follow TypeScript best practices.
32-46: LGTM!The rspack dev server configuration interface is well-typed with appropriate union types and good use of
unknownfor extensibility (proxy, devMiddleware options, and the index signature).
79-79: LGTM!The compression plugin constructor type is well-defined and appropriately types the plugin instantiation.
84-90: LGTM!The React Refresh plugin types are well-defined with appropriate separation between webpack and rspack variants and safe use of
unknownfor options.
Code Review for PR #614: Phase 3 TypeScript MigrationThank you for this comprehensive TypeScript migration! I've reviewed the changes thoroughly and here's my feedback: ✅ Strengths
🎯 Issues to Address
🔍 Suggestions for Improvement
🔒 Security ReviewThe security improvements are well-implemented:
📊 Test CoverageGood test coverage with 103 passing tests. The new TypeScript-specific tests for environments and path validation are comprehensive. Consider adding:
✨ Code Quality
🚀 Performance ImpactThe claimed build time improvements (5-7.5x faster with SWC) are significant. The caching strategy is well-designed with environment-aware TTLs. No performance regressions expected. ✅ RecommendationThis PR is ready to merge after addressing the transpiler default inconsistency mentioned above. The TypeScript migration is well-executed, maintains backward compatibility, and adds valuable security enhancements. Great work on incorporating all the Phase 2 feedback! The code quality is high, tests are passing, and the migration path is clear for users. This is a solid foundation for the remaining TypeScript migration phases. Review generated with comprehensive analysis of code changes, security implications, and architectural patterns. |
Code Review for Phase 3 TypeScript MigrationI've completed a thorough review of PR #614, which implements Phase 3 of the TypeScript migration (Environment & Build System). Here's my comprehensive feedback: 🎯 Overall AssessmentThis is a high-quality PR that demonstrates excellent software engineering practices. The TypeScript migration for environment files is well-executed with strong attention to backward compatibility, security, and performance. ✅ Strengths1. Security Improvements
2. Type Safety Enhancements
3. Performance Optimizations
4. Backward Compatibility
5. Test Coverage
📋 Suggestions for Improvement1. Documentation EnhancementThe transpiler migration guide is excellent. Consider adding: ## Troubleshooting Common Migration Issues
### TypeScript Compilation Errors
If you encounter TypeScript errors after the update:
1. Clear the build cache: `yarn clean && yarn build`
2. Ensure TypeScript is up to date: `yarn add -D typescript@latest`
3. Check for conflicting type definitions
### Environment Variable Validation
New validation may reject previously accepted values:
- Port numbers must be 1-65535 or "auto"
- Paths cannot contain traversal patterns
- Control characters in env vars are now sanitized2. Type Guard PerformanceWhile caching is implemented well, consider extracting the cache check into a decorator pattern for cleaner code: function withCache<T>(validator: (obj: unknown) => boolean) {
return (obj: unknown): obj is T => {
const cached = checkCache(obj);
if (cached !== undefined) return cached;
const result = validator(obj);
setCache(obj, result);
return result;
};
}3. Security LoggingConsider using a structured logging approach for security events: interface SecurityEvent {
type: 'PATH_TRAVERSAL' | 'INVALID_PORT' | 'ENV_SANITIZED';
details: string;
severity: 'WARNING' | 'ERROR';
}4. Edge Cases to Consider
🐛 Minor Issues
// Current: expect(consoleSpy.mock.calls.length >= 0).toBe(true)
// Better: expect(consoleSpy).toHaveBeenCalledTimes(0) // or expected number🔒 Security Review
🏆 Best Practices Observed
📊 Performance ImpactThe smart caching strategy should have minimal to positive performance impact:
✔️ RecommendationAPPROVED - This PR is ready to merge. The code quality is excellent, security improvements are valuable, and backward compatibility is maintained. The minor suggestions above can be addressed in follow-up PRs if desired. Great work on this phase of the TypeScript migration! The attention to security and performance while maintaining backward compatibility is commendable. 🚀 🤖 Generated with Claude Code Co-Authored-By: Claude [email protected] |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/environments/development.js (1)
47-72: Critical: Rspack configuration unconditionally includesdevServer, causing inconsistency with webpack behavior.The rspack configuration always includes the
devServerobject (lines 51-57), while the webpack configuration correctly makes it conditional based onrunningWebpackDevServer(line 26). This inconsistency violates the expected behavior verified by existing tests:
test/package/development.test.js:49expectsdevServerto beundefinedwhen not running the dev servertest/package/test.test.js:34expectsdevServerto beundefinedin test environmenttest/typescript/environments.test.js:85-89acknowledges devServer "may or may not have devServer depending on environment"Required fix:
Update the rspack configuration to conditionally include
devServerlike webpack does:const rspackDevConfig = () => { const devServerConfig = webpackDevServerConfig() const rspackConfig = { ...baseDevConfig, ...(runningWebpackDevServer && { devServer: { ...devServerConfig, devMiddleware: { ...(devServerConfig.devMiddleware || {}), writeToDisk: (filePath) => !filePath.includes(".hot-update.") } } }) } // ... rest of the function }This ensures both bundlers behave consistently across development, test, and production builds.
🧹 Nitpick comments (1)
package/environments/development.js (1)
23-42: Refactor to avoid duplicate webpackDevServerConfig() calls.The function calls
webpackDevServerConfig()twice: once conditionally on line 26 and always on line 28. This is inefficient and could lead to inconsistencies if the function has side effects or reads dynamic configuration.Apply this diff to call the function once:
const webpackDevConfig = () => { + const devServerConfig = webpackDevServerConfig() const webpackConfig = { ...baseDevConfig, - ...(runningWebpackDevServer && { devServer: webpackDevServerConfig() }) + ...(runningWebpackDevServer && { devServer: devServerConfig }) } - const devServerConfig = webpackDevServerConfig() if (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package/environments/base.js(0 hunks)package/environments/development.js(2 hunks)package/environments/production.js(2 hunks)package/environments/test.js(1 hunks)package/environments/types.js(1 hunks)
💤 Files with no reviewable changes (1)
- package/environments/base.js
🚧 Files skipped from review as they are similar to previous changes (2)
- package/environments/test.js
- package/environments/types.js
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
Applied to files:
package/environments/development.js
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: This gem supports both webpack and rspack configurations
Applied to files:
package/environments/development.js
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Always run `yarn lint` before committing JavaScript changes
Applied to files:
package/environments/production.js
🧬 Code graph analysis (2)
package/environments/development.js (1)
package/environments/base.js (6)
require(3-3)require(4-4)require(7-7)require(14-14)config(6-6)baseConfig(82-113)
package/environments/production.js (1)
package/environments/base.js (6)
require(3-3)require(4-4)require(7-7)require(14-14)baseConfig(82-113)config(6-6)
🪛 ESLint
package/environments/production.js
[error] 12-12: Expected 1 empty line after require statement not followed by another require.
(import/newline-after-import)
🪛 GitHub Check: Linting
package/environments/production.js
[failure] 12-12:
Expected 1 empty line after require statement not followed by another require
⏰ 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). (19)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.6.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: test
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
🔇 Additional comments (6)
package/environments/production.js (2)
25-50: LGTM: Well-structured plugin generation.The
getPluginsfunction is well-organized with clear JSDoc documentation. The conditional loading of compression plugins (gzip always, Brotli when available) follows best practices for production builds.
51-60: LGTM: Appropriate production configuration.The production config object has sensible defaults for a production build: source maps for debugging, bail on errors, and modular plugin/optimization loading.
package/environments/development.js (4)
12-18: LGTM!The base development configuration is well-documented and uses appropriate settings for development mode.
52-56: LGTM! Good defensive programming.The spread operator with fallback
...(devServerConfig.devMiddleware || {})on line 54 safely handles cases wheredevMiddlewareis undefined, preventing potential runtime errors.
73-75: LGTM!The bundler selection logic and configuration merge are straightforward and correct.
1-11: The review comment is incorrect - this is standard TypeScript package architectureThe .js files in
package/environments/are intentionally committed TypeScript compiled outputs, not source files. This is confirmed by:
- Both .ts (source) and .js (compiled) files exist side-by-side
.gitignoreexplicitly whitelists these files:!package/environments/*.jspackage.jsonhasbuild: "tsc"andprepublishOnly: "yarn build && yarn type-check"- The .js files are distributed via npm so consumers don't need to compile TypeScript
The
Object.defineProperty(exports, "__esModule", { value: true })on line 5 is automatically generated by the TypeScript compiler when transpiling ES modules to CommonJS - it's not manually added code. This is expected and correct for TypeScript-compiled outputs.This pattern allows the package to:
- Maintain type-safe TypeScript sources (.ts files)
- Distribute pre-compiled JavaScript (.js files) for compatibility
- Ensure fresh compilation before publishing via prepublishOnly hook
No changes are needed.
Likely an incorrect or invalid review comment.
Code Review for Phase 3 TypeScript MigrationI've conducted a thorough review of this PR implementing Phase 3 of the TypeScript migration. Here's my comprehensive feedback: ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
package/environments/base.js(0 hunks)package/environments/development.js(2 hunks)package/environments/production.js(3 hunks)test/package/transpiler-defaults.test.js(1 hunks)test/typescript/environments.test.js(1 hunks)test/typescript/pathValidation.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- package/environments/base.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/typescript/pathValidation.test.js
- test/package/transpiler-defaults.test.js
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
Applied to files:
package/environments/development.jspackage/environments/production.js
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: This gem supports both webpack and rspack configurations
Applied to files:
package/environments/development.jspackage/environments/production.js
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Always run `yarn lint` before committing JavaScript changes
Applied to files:
package/environments/production.js
🧬 Code graph analysis (2)
package/environments/development.js (1)
package/environments/base.js (6)
require(3-3)require(4-4)require(7-7)require(15-15)config(6-6)baseConfig(83-114)
test/typescript/environments.test.js (4)
package/environments/development.js (4)
require(6-6)require(10-10)require(11-11)config(7-7)package/environments/production.js (6)
require(8-8)require(9-9)require(11-11)require(20-20)productionConfig(55-61)config(12-12)package/environments/base.js (5)
require(3-3)require(4-4)require(7-7)require(15-15)config(6-6)package/rspack/index.js (1)
devServer(12-12)
🪛 ESLint
test/typescript/environments.test.js
[error] 94-98: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 94-97: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 94-96: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 94-95: Avoid having conditionals in tests
(jest/no-conditional-in-test)
🪛 GitHub Actions: Node based checks
test/typescript/environments.test.js
[error] 94-94: ESLint: jest/no-conditional-in-test - Avoid having conditionals in tests.
🪛 GitHub Check: Linting
test/typescript/environments.test.js
[failure] 94-94:
Avoid having conditionals in tests
[failure] 94-94:
Avoid having conditionals in tests
[failure] 94-94:
Avoid having conditionals in tests
[failure] 94-94:
Avoid having conditionals in tests
⏰ 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). (20)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: claude-review
🔇 Additional comments (15)
package/environments/development.js (5)
1-5: LGTM! Module documentation and ES interop added.The JSDoc module comment and
__esModuleflag properly support TypeScript migration and improve module documentation.
13-19: LGTM! Base configuration properly documented.The JSDoc comment clarifies the shared development configuration between bundlers.
24-43: LGTM! Webpack React Refresh integration is well-guarded.The conditional plugin loading properly checks for dev server mode, hot reload enabled, and plugin availability before requiring and instantiating the React Refresh plugin. The plugin array initialization safely spreads existing plugins.
48-73: LGTM! Rspack configuration safely handles devMiddleware and React Refresh.The implementation includes two key improvements:
- Line 55: Safe merge of
devMiddlewareusing|| {}prevents spreading undefined- Lines 60-70: Conditional React Refresh plugin for rspack with proper guards (dev server mode, hot reload, plugin availability)
The
writeToDiskfilter correctly excludes hot-update files from being written to disk.
74-76: LGTM! Bundler selection and merge strategy maintained.The bundler-specific configuration selection and merge with base config preserves the existing pattern.
test/typescript/environments.test.js (4)
1-25: LGTM! Development config tests validate core structure.The test suite properly validates that the development configuration exports a valid object with the expected mode and devtool settings.
27-51: LGTM! Production config tests cover key production features.The test suite validates production-specific configuration including optimization and plugins array, ensuring the compiled TypeScript module exports correctly.
53-70: LGTM! Test config validation ensures proper mode settings.The test suite validates that the test environment configuration exports with an appropriate mode value from the allowed set.
72-82: LGTM! Type safety tests ensure consistent base structure.The test validates that all environment configurations share the required base properties (module, entry, output, resolve), ensuring consistency across webpack and rspack configurations.
package/environments/production.js (6)
1-5: LGTM! Module documentation and ES interop added.The JSDoc module comment and
__esModuleflag properly support TypeScript migration and improve module documentation.
6-20: LGTM! Dynamic optimization loading supports bundler flexibility.The dynamic require for bundler-specific optimization configuration (webpack/rspack) maintains flexibility while preserving type safety.
21-25: LGTM! CompressionPlugin loading is properly guarded.The
moduleExistscheck prevents runtime errors when the compression plugin is not installed, supporting optional compression features.
26-51: LGTM! Plugin generation refactored for clarity and conditional compression.The
getPluginsfunction properly encapsulates production plugin logic:
- Gzip compression added when
CompressionPluginis available- Brotli compression conditionally added when Node.js supports it (
process.versions.brotli)- File type matching pattern covers common assets (js, css, html, json, fonts, maps)
This refactor improves maintainability and aligns with the TypeScript migration's focus on cleaner abstractions.
52-61: LGTM! Production configuration properly structured.The production config object cleanly integrates plugins and optimization through function calls, supporting the refactored plugin generation while maintaining all required production settings (source-map devtool, bail on error, etc.).
62-69: LGTM! Content hash warning and export maintained.The existing content hash validation warning and merge-based export pattern are preserved, ensuring backward compatibility.
There was a problem hiding this comment.
Pull Request Overview
This PR completes Phase 3 of the TypeScript migration by converting environment configuration files and implementing security improvements from Phase 2 feedback. It introduces path validation utilities, improves type safety through proper type guards, and establishes SWC as the consistent default transpiler across bundlers.
- Converted all environment configuration files (
development.ts,production.ts,test.ts) to TypeScript - Added comprehensive security utilities for path validation and environment variable sanitization
- Improved configuration validation with TTL-based caching and stricter type checking
- Made SWC the default transpiler for both webpack and rspack for consistency
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/typescript/pathValidation.test.js |
Test suite for new security validation utilities |
test/typescript/environments.test.js |
TypeScript-specific tests for environment module compilation |
test/package/transpiler-defaults.test.js |
Tests for transpiler default behavior and backward compatibility |
package/utils/typeGuards.ts |
Enhanced validation with security checks and TTL-based caching |
package/utils/pathValidation.ts |
New security utilities for path validation and sanitization |
package/utils/errorHelpers.ts |
Removed type assertion in favor of proper type casting |
package/environments/*.ts |
TypeScript environment configurations with proper type annotations |
package/config.ts |
Improved transpiler configuration with better type safety |
package/env.ts |
Added environment variable sanitization |
| Documentation and build files | Added transpiler migration guide and npm publishing configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Filter by character code to avoid control character regex (Biome compliance) | ||
| const sanitized = value.split('').filter(char => { | ||
| const code = char.charCodeAt(0) | ||
| // Keep chars with code > 31 (after control chars) and not 127 (DEL) | ||
| return code > 31 && code !== 127 | ||
| }).join('') |
There was a problem hiding this comment.
The character-by-character filtering approach is inefficient for large strings. Consider using a regular expression like /[\x00-\x1F\x7F]/g to remove control characters in a single operation: const sanitized = value.replace(/[\x00-\x1F\x7F]/g, '')
| // Filter by character code to avoid control character regex (Biome compliance) | |
| const sanitized = value.split('').filter(char => { | |
| const code = char.charCodeAt(0) | |
| // Keep chars with code > 31 (after control chars) and not 127 (DEL) | |
| return code > 31 && code !== 127 | |
| }).join('') | |
| // Remove control characters and null bytes using a regular expression | |
| const sanitized = value.replace(/[\x00-\x1F\x7F]/g, '') |
| const isWatchMode = process.argv.includes('--watch') || process.env.WEBPACK_WATCH === 'true' | ||
| const CACHE_TTL = process.env.NODE_ENV === 'production' && !isWatchMode | ||
| ? Infinity | ||
| : isWatchMode |
There was a problem hiding this comment.
Calling process.argv.includes() on every module load is inefficient. Consider caching this value or moving the check inside the validation function where it's actually used.
| const isWatchMode = process.argv.includes('--watch') || process.env.WEBPACK_WATCH === 'true' | |
| const CACHE_TTL = process.env.NODE_ENV === 'production' && !isWatchMode | |
| ? Infinity | |
| : isWatchMode | |
| function getIsWatchMode(): boolean { | |
| return process.argv.includes('--watch') || process.env.WEBPACK_WATCH === 'true' | |
| } | |
| const CACHE_TTL = process.env.NODE_ENV === 'production' && !getIsWatchMode() | |
| ? Infinity | |
| : getIsWatchMode() |
| export function clearValidationCache(): void { | ||
| // WeakMap doesn't have a clear method, but we can create a new one | ||
| // This is handled by garbage collection | ||
| if (debugCache) { | ||
| console.log('[SHAKAPACKER DEBUG] Validation cache cleared') | ||
| } | ||
| } |
There was a problem hiding this comment.
The clearValidationCache function doesn't actually clear the cache. Since WeakMaps can't be cleared directly, you need to reassign the WeakMap variable: validatedConfigs = new WeakMap<object, CacheEntry>()
Code Review for Phase 3 TypeScript MigrationThank you for this comprehensive TypeScript migration! I've reviewed the changes and here's my feedback: ✅ StrengthsCode Quality & TypeScript Migration:
Security Improvements:
Performance:
Backward Compatibility:
|
Code Review for Phase 3 TypeScript MigrationI've completed a thorough review of PR #614. Overall, this is a high-quality implementation of the TypeScript migration for environment files with excellent attention to security and backward compatibility. ✅ StrengthsCode Quality & Best Practices:
Security Improvements:
Performance Optimizations:
Backward Compatibility:
Test Coverage:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
package/environments/development.js (1)
29-32: Minor optimization: cache devServerConfig call.
webpackDevServerConfig()is called on line 29 and conditionally checked on lines 30-32. Store it once to avoid redundant invocations:const webpackDevConfig = () => { + const devServerConfig = webpackDevServerConfig() const webpackConfig = { ...baseDevConfig, - ...(runningWebpackDevServer && { devServer: webpackDevServerConfig() }) + ...(runningWebpackDevServer && { devServer: devServerConfig }) } - const devServerConfig = webpackDevServerConfig() if (runningWebpackDevServer &&package/environments/production.js (1)
28-38: Consider adding compression thresholds to skip tiny files.The compression plugins will compress all matching files regardless of size. This can waste CPU and disk space on small files that don't benefit from compression (and may even grow larger).
Add
thresholdandminRatiooptions to both compression plugins:plugins.push( new CompressionPlugin({ filename: "[path][base].gz[query]", algorithm: "gzip", + threshold: 10240, // Only compress files > 10KB + minRatio: 0.8, // Only keep if compressed < 80% of original test: /\.(js|css|html|json|ico|svg|eot|otf|ttf|map)$/ }) )Apply the same for the brotli plugin (lines 34-38).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package/environments/base.js(1 hunks)package/environments/development.js(1 hunks)package/environments/production.js(1 hunks)package/environments/test.js(1 hunks)package/environments/types.js(1 hunks)package/utils/typeGuards.ts(5 hunks)test/typescript/environments.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Always run `yarn lint` before committing JavaScript changes
Applied to files:
package/environments/production.js
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: This gem supports both webpack and rspack configurations
Applied to files:
package/environments/production.jspackage/environments/base.jspackage/environments/development.jspackage/environments/test.js
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
Applied to files:
package/environments/development.jspackage/environments/test.js
🧬 Code graph analysis (6)
test/typescript/environments.test.js (3)
package/environments/development.js (4)
require(7-7)require(11-11)require(12-12)config(8-8)package/environments/production.js (6)
require(9-9)require(10-10)require(12-12)require(15-15)productionConfig(46-52)config(13-13)package/environments/test.js (2)
require(7-7)config(8-8)
package/utils/typeGuards.ts (1)
package/utils/pathValidation.ts (2)
isPathTraversalSafe(11-26)validatePort(121-139)
package/environments/production.js (3)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)baseConfig(72-102)config(7-7)package/environments/development.js (5)
require(7-7)require(11-11)require(12-12)baseConfig(9-9)config(8-8)test/typescript/environments.test.js (1)
productionConfig(4-4)
package/environments/base.js (3)
package/environments/development.js (4)
require(7-7)require(11-11)require(12-12)baseConfig(9-9)package/environments/production.js (5)
require(9-9)require(10-10)require(12-12)require(15-15)baseConfig(11-11)package/environments/test.js (2)
require(7-7)baseConfig(9-9)
package/environments/development.js (2)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)config(7-7)baseConfig(72-102)package/environments/test.js (4)
require(7-7)config(8-8)baseConfig(9-9)bundlerConfig(38-38)
package/environments/test.js (2)
package/environments/base.js (6)
require(4-4)require(5-5)require(8-8)require(10-10)config(7-7)baseConfig(72-102)package/environments/development.js (6)
require(7-7)require(11-11)require(12-12)config(8-8)baseConfig(9-9)bundlerConfig(70-70)
🪛 ESLint
test/typescript/environments.test.js
[error] 89-92: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 90-90: Avoid calling expect conditionally`
(jest/no-conditional-expect)
[error] 98-98: Use object destructuring.
(prefer-destructuring)
[error] 100-103: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 100-102: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 100-101: Avoid having conditionals in tests
(jest/no-conditional-in-test)
package/environments/production.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
[error] 9-9: Delete ;
(prettier/prettier)
[error] 10-10: Delete ;
(prettier/prettier)
[error] 11-11: Delete ;
(prettier/prettier)
[error] 12-12: Delete ;
(prettier/prettier)
[error] 13-13: Expected 1 empty line after require statement not followed by another require.
(import/newline-after-import)
[error] 13-13: Delete ;
(prettier/prettier)
[error] 14-14: Replace __dirname,·"..",·"optimization",·${config.assets_bundler}.js); with ⏎··__dirname,⏎··"..",⏎··"optimization",⏎··${config.assets_bundler}.js⏎)
(prettier/prettier)
[error] 15-15: Delete ;
(prettier/prettier)
[error] 16-16: Delete ;
(prettier/prettier)
[error] 18-18: Expected indentation of 2 spaces but found 4.
(indent)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 19-19: Replace ····CompressionPlugin·=·require("compression-webpack-plugin"); with ··CompressionPlugin·=·require("compression-webpack-plugin")
(prettier/prettier)
[error] 19-19: Expected indentation of 2 spaces but found 4.
(indent)
[error] 26-26: Expected indentation of 2 spaces but found 4.
(indent)
[error] 26-26: Replace ··const·plugins·=·[]; with const·plugins·=·[]
(prettier/prettier)
[error] 27-27: Delete ··
(prettier/prettier)
[error] 27-27: Expected indentation of 2 spaces but found 4.
(indent)
[error] 28-28: Replace ········plugins.push( with ····plugins.push(⏎······
(prettier/prettier)
[error] 28-28: Expected indentation of 4 spaces but found 8.
(indent)
[error] 29-29: Delete ····
(prettier/prettier)
[error] 29-29: Expected indentation of 6 spaces but found 12.
(indent)
[error] 30-30: Replace ············ with ········
(prettier/prettier)
[error] 30-30: Expected indentation of 6 spaces but found 12.
(indent)
[error] 31-31: Delete ····
(prettier/prettier)
[error] 31-31: Expected indentation of 6 spaces but found 12.
(indent)
[error] 32-32: Replace ········})); with ······})
(prettier/prettier)
[error] 32-32: Expected indentation of 4 spaces but found 8.
(indent)
[error] 33-33: Expected indentation of 4 spaces but found 8.
(indent)
[error] 33-33: Insert )⏎
(prettier/prettier)
[error] 34-34: Replace ············plugins.push( with ······plugins.push(⏎········
(prettier/prettier)
[error] 34-34: Expected indentation of 6 spaces but found 12.
(indent)
[error] 35-35: Replace ················ with ··········
(prettier/prettier)
[error] 35-35: Expected indentation of 8 spaces but found 16.
(indent)
[error] 36-36: Delete ······
(prettier/prettier)
[error] 36-36: Expected indentation of 8 spaces but found 16.
(indent)
[error] 37-37: Expected indentation of 8 spaces but found 16.
(indent)
[error] 37-37: Delete ······
(prettier/prettier)
[error] 38-38: Replace ············})); with ········})
(prettier/prettier)
[error] 38-38: Expected indentation of 6 spaces but found 12.
(indent)
[error] 39-39: Expected indentation of 4 spaces but found 8.
(indent)
[error] 39-39: Replace ··} with )
(prettier/prettier)
[error] 40-40: Expected indentation of 2 spaces but found 4.
(indent)
[error] 41-41: Expected indentation of 2 spaces but found 4.
(indent)
[error] 41-41: Replace ··return·plugins; with }⏎··return·plugins
(prettier/prettier)
[error] 42-42: Delete ;
(prettier/prettier)
[error] 47-47: Delete ··
(prettier/prettier)
[error] 47-47: Expected indentation of 2 spaces but found 4.
(indent)
[error] 48-48: Expected indentation of 2 spaces but found 4.
(indent)
[error] 48-48: Delete ··
(prettier/prettier)
[error] 49-49: Delete ··
(prettier/prettier)
[error] 49-49: Expected indentation of 2 spaces but found 4.
(indent)
[error] 50-50: Expected indentation of 2 spaces but found 4.
(indent)
[error] 50-50: Delete ··
(prettier/prettier)
[error] 51-51: Replace ···· with ··
(prettier/prettier)
[error] 51-51: Expected indentation of 2 spaces but found 4.
(indent)
[error] 52-52: Delete ;
(prettier/prettier)
[error] 54-54: Replace ···· with ··
(prettier/prettier)
[error] 54-54: Expected indentation of 2 spaces but found 4.
(indent)
[error] 55-55: Delete ··
(prettier/prettier)
[error] 55-55: Expected indentation of 2 spaces but found 4.
(indent)
[error] 58-58: Delete ;
(prettier/prettier)
[error] 60-60: Delete ;
(prettier/prettier)
package/environments/base.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 4-4: Delete ;
(prettier/prettier)
[error] 5-5: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
[error] 7-7: Delete ;
(prettier/prettier)
[error] 8-8: Expected 1 empty line after require statement not followed by another require.
(import/newline-after-import)
[error] 8-8: Delete ;
(prettier/prettier)
[error] 9-9: Replace __dirname,·"..",·"plugins",·${config.assets_bundler}.js); with ⏎··__dirname,⏎··"..",⏎··"plugins",⏎··${config.assets_bundler}.js⏎)
(prettier/prettier)
[error] 10-10: Delete ;
(prettier/prettier)
[error] 11-11: Replace __dirname,·"..",·"rules",·${config.assets_bundler}.js); with ⏎··__dirname,⏎··"..",⏎··"rules",⏎··${config.assets_bundler}.js⏎)
(prettier/prettier)
[error] 12-12: Delete ;
(prettier/prettier)
[error] 15-15: Delete ;
(prettier/prettier)
[error] 17-17: Expected indentation of 2 spaces but found 4.
(indent)
[error] 17-17: Delete ··
(prettier/prettier)
[error] 18-18: Replace ········return·[]; with ····return·[]
(prettier/prettier)
[error] 18-18: Expected indentation of 4 spaces but found 8.
(indent)
[error] 19-19: Expected indentation of 2 spaces but found 4.
(indent)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 20-20: Expected indentation of 2 spaces but found 4.
(indent)
[error] 21-21: Replace ········const·filePath·=·join(dir,·dirent.name); with ····const·filePath·=·join(dir,·dirent.name)
(prettier/prettier)
[error] 21-21: Expected indentation of 4 spaces but found 8.
(indent)
[error] 22-22: Delete ····
(prettier/prettier)
[error] 22-22: Expected indentation of 4 spaces but found 8.
(indent)
[error] 23-23: Replace ············return·getFilesInDirectory(filePath,·includeNested); with ······return·getFilesInDirectory(filePath,·includeNested)
(prettier/prettier)
[error] 23-23: Expected indentation of 6 spaces but found 12.
(indent)
[error] 24-24: Delete ····
(prettier/prettier)
[error] 24-24: Expected indentation of 4 spaces but found 8.
(indent)
[error] 25-25: Delete ····
(prettier/prettier)
[error] 25-25: Expected indentation of 4 spaces but found 8.
(indent)
[error] 26-26: Replace ············return·filePath; with ······return·filePath
(prettier/prettier)
[error] 26-26: Expected indentation of 6 spaces but found 12.
(indent)
[error] 27-27: Expected indentation of 4 spaces but found 8.
(indent)
[error] 27-27: Delete ····
(prettier/prettier)
[error] 28-28: Replace ········return·[]; with ····return·[]
(prettier/prettier)
[error] 28-28: Expected indentation of 4 spaces but found 8.
(indent)
[error] 29-29: Replace ····}); with ··})
(prettier/prettier)
[error] 29-29: Expected indentation of 2 spaces but found 4.
(indent)
[error] 30-30: Delete ;
(prettier/prettier)
[error] 32-32: Expected indentation of 2 spaces but found 4.
(indent)
[error] 32-32: Replace ··const·entries·=·{}; with const·entries·=·{}
(prettier/prettier)
[error] 33-33: Expected indentation of 2 spaces but found 4.
(indent)
[error] 33-33: Replace ··const·rootPath·=·join(config.source_path,·config.source_entry_path); with const·rootPath·=·join(config.source_path,·config.source_entry_path)
(prettier/prettier)
[error] 34-34: Expected indentation of 2 spaces but found 4.
(indent)
[error] 34-34: Delete ··
(prettier/prettier)
[error] 35-35: Replace ········throw·new·Error( with ····throw·new·Error(⏎······
(prettier/prettier)
[error] 35-35: Expected indentation of 4 spaces but found 8.
(indent)
[error] 36-36: Delete ····
(prettier/prettier)
[error] 37-37: Replace ············ with ········
(prettier/prettier)
[error] 38-38: Delete ····
(prettier/prettier)
[error] 39-39: Replace ············ with ········
(prettier/prettier)
[error] 40-40: Replace ············ with ········
(prettier/prettier)
[error] 41-41: Delete ····
(prettier/prettier)
[error] 42-42: Replace ····3.·Or·use·both·options·for·better·organization·of·your·entry·points); with 3.·Or·use·both·options·for·better·organization·of·your·entry·points
(prettier/prettier)
[error] 43-43: Expected indentation of 2 spaces but found 4.
(indent)
[error] 43-43: Replace } with )
(prettier/prettier)
[error] 44-44: Expected indentation of 2 spaces but found 4.
(indent)
[error] 44-44: Insert }⏎
(prettier/prettier)
[error] 45-45: Expected indentation of 4 spaces but found 8.
(indent)
[error] 45-45: Replace ····const·namespace·=·relative(join(rootPath),·dirname(path)); with const·namespace·=·relative(join(rootPath),·dirname(path))
(prettier/prettier)
[error] 46-46: Replace ········const·name·=·join(namespace,·basename(path,·extname(path))); with ····const·name·=·join(namespace,·basename(path,·extname(path)))
(prettier/prettier)
[error] 46-46: Expected indentation of 4 spaces but found 8.
(indent)
[error] 47-47: Replace ········const·assetPath·=·resolve(path); with ····const·assetPath·=·resolve(path)
(prettier/prettier)
[error] 47-47: Expected indentation of 4 spaces but found 8.
(indent)
[error] 48-48: Replace ········ with ····
(prettier/prettier)
[error] 48-48: Expected indentation of 4 spaces but found 8.
(indent)
[error] 49-49: Delete ····
(prettier/prettier)
[error] 49-49: Expected indentation of 4 spaces but found 8.
(indent)
[error] 50-50: Expected indentation of 4 spaces but found 8.
(indent)
[error] 50-50: Replace ····const·previousPaths·=·entries[name]; with const·previousPaths·=·entries[name]
(prettier/prettier)
[error] 51-51: Expected indentation of 4 spaces but found 8.
(indent)
[error] 51-51: Delete ····
(prettier/prettier)
[error] 52-52: Delete ······
(prettier/prettier)
[error] 52-52: Expected indentation of 6 spaces but found 12.
(indent)
[error] 53-53: Replace ················ with ········
(prettier/prettier)
[error] 53-53: Expected indentation of 8 spaces but found 16.
(indent)
[error] 54-54: Replace ················:·[previousPaths]; with ········:·[previousPaths]
(prettier/prettier)
[error] 54-54: Expected indentation of 8 spaces but found 16.
(indent)
[error] 55-55: Replace ············pathArray.push(assetPath); with ······pathArray.push(assetPath)
(prettier/prettier)
[error] 55-55: Expected indentation of 6 spaces but found 12.
(indent)
[error] 56-56: Expected indentation of 6 spaces but found 12.
(indent)
[error] 56-56: Replace ······entries[name]·=·pathArray; with entries[name]·=·pathArray
(prettier/prettier)
[error] 57-58: Replace ········}⏎······· with ····}
(prettier/prettier)
[error] 57-57: Expected indentation of 4 spaces but found 8.
(indent)
[error] 58-58: Expected indentation of 4 spaces but found 8.
(indent)
[error] 59-59: Replace ············entries[name]·=·assetPath; with ······entries[name]·=·assetPath
(prettier/prettier)
[error] 59-59: Expected indentation of 6 spaces but found 12.
(indent)
[error] 60-60: Replace ········ with ····
(prettier/prettier)
[error] 60-60: Expected indentation of 4 spaces but found 8.
(indent)
[error] 61-61: Replace ····}); with ··})
(prettier/prettier)
[error] 61-61: Expected indentation of 2 spaces but found 4.
(indent)
[error] 62-62: Expected indentation of 2 spaces but found 4.
(indent)
[error] 62-62: Replace ··return·entries; with return·entries
(prettier/prettier)
[error] 63-63: Delete ;
(prettier/prettier)
[error] 65-65: Replace ····const·result·=·[resolve(config.source_path)]; with ··const·result·=·[resolve(config.source_path)]
(prettier/prettier)
[error] 65-65: Expected indentation of 2 spaces but found 4.
(indent)
[error] 66-66: Delete ··
(prettier/prettier)
[error] 66-66: Expected indentation of 2 spaces but found 4.
(indent)
[error] 67-67: Replace ········config.additional_paths.forEach((path)·=>·result.push(resolve(path))); with ····config.additional_paths.forEach((path)·=>·result.push(resolve(path)))
(prettier/prettier)
[error] 67-67: Expected indentation of 4 spaces but found 8.
(indent)
[error] 68-68: Delete ··
(prettier/prettier)
[error] 68-68: Expected indentation of 2 spaces but found 4.
(indent)
[error] 69-69: Replace ····result.push("node_modules"); with ··result.push("node_modules")
(prettier/prettier)
[error] 69-69: Expected indentation of 2 spaces but found 4.
(indent)
[error] 70-70: Replace ····return·result; with ··return·result
(prettier/prettier)
[error] 70-70: Expected indentation of 2 spaces but found 4.
(indent)
[error] 71-71: Delete ;
(prettier/prettier)
[error] 73-73: Delete ··
(prettier/prettier)
[error] 73-73: Expected indentation of 2 spaces but found 4.
(indent)
[error] 74-74: Expected indentation of 2 spaces but found 4.
(indent)
[error] 74-74: Delete ··
(prettier/prettier)
[error] 75-75: Replace ········ with ····
(prettier/prettier)
[error] 75-75: Expected indentation of 4 spaces but found 8.
(indent)
[error] 76-76: Replace ········ with ····
(prettier/prettier)
[error] 76-76: Expected indentation of 4 spaces but found 8.
(indent)
[error] 77-77: Delete ····
(prettier/prettier)
[error] 77-77: Expected indentation of 4 spaces but found 8.
(indent)
[error] 78-78: Expected indentation of 4 spaces but found 8.
(indent)
[error] 78-78: Delete ····
(prettier/prettier)
[error] 79-79: Replace ········ with ····
(prettier/prettier)
[error] 79-79: Expected indentation of 4 spaces but found 8.
(indent)
[error] 80-80: Replace ········ with ····
(prettier/prettier)
[error] 80-80: Expected indentation of 4 spaces but found 8.
(indent)
[error] 81-81: Delete ····
(prettier/prettier)
[error] 81-81: Expected indentation of 4 spaces but found 8.
(indent)
[error] 82-82: Expected indentation of 4 spaces but found 8.
(indent)
[error] 82-82: Replace ····crossOriginLoading: with crossOriginLoading:⏎·····
(prettier/prettier)
[error] 83-83: Replace ············ with ········
(prettier/prettier)
[error] 83-83: Expected indentation of 6 spaces but found 12.
(indent)
[error] 84-84: Expected indentation of 6 spaces but found 12.
(indent)
[error] 84-84: Delete ····
(prettier/prettier)
[error] 85-85: Expected indentation of 2 spaces but found 4.
(indent)
[error] 85-85: Delete ··
(prettier/prettier)
[error] 86-86: Delete ··
(prettier/prettier)
[error] 86-86: Expected indentation of 2 spaces but found 4.
(indent)
[error] 87-87: Expected indentation of 2 spaces but found 4.
(indent)
[error] 87-87: Delete ··
(prettier/prettier)
[error] 88-88: Delete ····
(prettier/prettier)
[error] 88-88: Expected indentation of 4 spaces but found 8.
(indent)
[error] 89-89: Expected indentation of 4 spaces but found 8.
(indent)
[error] 89-89: Delete ····
(prettier/prettier)
[error] 90-90: Replace ···· with ··
(prettier/prettier)
[error] 90-90: Expected indentation of 2 spaces but found 4.
(indent)
[error] 91-91: Delete ··
(prettier/prettier)
[error] 91-91: Expected indentation of 2 spaces but found 4.
(indent)
[error] 92-92: Expected indentation of 2 spaces but found 4.
(indent)
[error] 92-92: Delete ··
(prettier/prettier)
[error] 93-93: Replace ········ with ····
(prettier/prettier)
[error] 93-93: Expected indentation of 4 spaces but found 8.
(indent)
[error] 94-94: Delete ··
(prettier/prettier)
[error] 94-94: Expected indentation of 2 spaces but found 4.
(indent)
[error] 95-95: Expected indentation of 2 spaces but found 4.
(indent)
[error] 95-95: Delete ··
(prettier/prettier)
[error] 96-96: Delete ····
(prettier/prettier)
[error] 96-96: Expected indentation of 4 spaces but found 8.
(indent)
[error] 97-97: Expected indentation of 4 spaces but found 8.
(indent)
[error] 97-97: Delete ····
(prettier/prettier)
[error] 98-98: Replace ···· with ··
(prettier/prettier)
[error] 98-98: Expected indentation of 2 spaces but found 4.
(indent)
[error] 99-99: Delete ··
(prettier/prettier)
[error] 99-99: Expected indentation of 2 spaces but found 4.
(indent)
[error] 100-100: Replace ········ with ····
(prettier/prettier)
[error] 100-100: Expected indentation of 4 spaces but found 8.
(indent)
[error] 101-101: Expected indentation of 2 spaces but found 4.
(indent)
[error] 101-101: Delete ··
(prettier/prettier)
[error] 102-102: Delete ;
(prettier/prettier)
[error] 103-103: Delete ;
(prettier/prettier)
package/environments/development.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
[error] 7-7: Delete ;
(prettier/prettier)
[error] 8-8: Delete ;
(prettier/prettier)
[error] 9-9: Delete ;
(prettier/prettier)
[error] 10-10: Delete ;
(prettier/prettier)
[error] 11-11: Delete ;
(prettier/prettier)
[error] 12-12: Delete ;
(prettier/prettier)
[error] 17-17: Expected indentation of 2 spaces but found 4.
(indent)
[error] 17-17: Delete ··
(prettier/prettier)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 18-18: Expected indentation of 2 spaces but found 4.
(indent)
[error] 19-19: Delete ;
(prettier/prettier)
[error] 25-25: Expected indentation of 2 spaces but found 4.
(indent)
[error] 25-25: Delete ··
(prettier/prettier)
[error] 26-26: Expected indentation of 4 spaces but found 8.
(indent)
[error] 26-26: Delete ····
(prettier/prettier)
[error] 27-27: Delete ····
(prettier/prettier)
[error] 27-27: Expected indentation of 4 spaces but found 8.
(indent)
[error] 28-28: Replace ····}; with ··}
(prettier/prettier)
[error] 28-28: Expected indentation of 2 spaces but found 4.
(indent)
[error] 29-29: Replace ····const·devServerConfig·=·webpackDevServerConfig(); with ··const·devServerConfig·=·webpackDevServerConfig()
(prettier/prettier)
[error] 29-29: Expected indentation of 2 spaces but found 4.
(indent)
[error] 30-30: Replace ····if·( with ··if·(⏎····
(prettier/prettier)
[error] 30-30: Expected indentation of 2 spaces but found 4.
(indent)
[error] 31-31: Replace ········ with ····
(prettier/prettier)
[error] 32-32: Replace ········moduleExists("@pmmmwh/react-refresh-webpack-plugin"))·{ with ····moduleExists("@pmmmwh/react-refresh-webpack-plugin")
(prettier/prettier)
[error] 33-33: Expected indentation of 4 spaces but found 8.
(indent)
[error] 33-33: Replace ·· with )·{⏎
(prettier/prettier)
[error] 34-34: Replace ········const·ReactRefreshWebpackPlugin·=·require("@pmmmwh/react-refresh-webpack-plugin"); with ····const·ReactRefreshWebpackPlugin·=·require("@pmmmwh/react-refresh-webpack-plugin")
(prettier/prettier)
[error] 34-34: Expected indentation of 4 spaces but found 8.
(indent)
[error] 35-35: Expected indentation of 4 spaces but found 8.
(indent)
[error] 35-35: Delete ····
(prettier/prettier)
[error] 36-36: Replace ············ with ······
(prettier/prettier)
[error] 36-36: Expected indentation of 6 spaces but found 12.
(indent)
[error] 37-37: Replace ············ with ······
(prettier/prettier)
[error] 37-37: Expected indentation of 6 spaces but found 12.
(indent)
[error] 38-38: Replace ········]; with ····]
(prettier/prettier)
[error] 38-38: Expected indentation of 4 spaces but found 8.
(indent)
[error] 39-39: Replace ···· with ··
(prettier/prettier)
[error] 39-39: Expected indentation of 2 spaces but found 4.
(indent)
[error] 40-40: Replace ····return·webpackConfig; with ··return·webpackConfig
(prettier/prettier)
[error] 40-40: Expected indentation of 2 spaces but found 4.
(indent)
[error] 41-41: Delete ;
(prettier/prettier)
[error] 47-47: Expected indentation of 2 spaces but found 4.
(indent)
[error] 47-47: Replace ··const·devServerConfig·=·webpackDevServerConfig(); with const·devServerConfig·=·webpackDevServerConfig()
(prettier/prettier)
[error] 48-48: Expected indentation of 2 spaces but found 4.
(indent)
[error] 48-48: Delete ··
(prettier/prettier)
[error] 49-49: Delete ····
(prettier/prettier)
[error] 49-49: Expected indentation of 4 spaces but found 8.
(indent)
[error] 50-50: Expected indentation of 4 spaces but found 8.
(indent)
[error] 50-50: Delete ····
(prettier/prettier)
[error] 51-51: Replace ············ with ······
(prettier/prettier)
[error] 51-51: Expected indentation of 6 spaces but found 12.
(indent)
[error] 52-52: Replace ············ with ······
(prettier/prettier)
[error] 52-52: Expected indentation of 6 spaces but found 12.
(indent)
[error] 53-53: Replace ················ with ········
(prettier/prettier)
[error] 53-53: Expected indentation of 8 spaces but found 16.
(indent)
[error] 54-54: Replace ················ with ········
(prettier/prettier)
[error] 54-54: Expected indentation of 8 spaces but found 16.
(indent)
[error] 55-55: Expected indentation of 6 spaces but found 12.
(indent)
[error] 55-55: Delete ······
(prettier/prettier)
[error] 56-56: Expected indentation of 4 spaces but found 8.
(indent)
[error] 56-56: Replace ·· with }⏎
(prettier/prettier)
[error] 57-57: Expected indentation of 2 spaces but found 4.
(indent)
[error] 57-57: Replace ··}; with if·(
(prettier/prettier)
[error] 58-58: Expected indentation of 2 spaces but found 4.
(indent)
[error] 58-58: Delete if·(
(prettier/prettier)
[error] 59-59: Replace ········ with ····
(prettier/prettier)
[error] 60-60: Replace ········moduleExists("@rspack/plugin-react-refresh") with ····moduleExists("@rspack/plugin-react-refresh")⏎··
(prettier/prettier)
[error] 61-61: Delete ····
(prettier/prettier)
[error] 61-61: Expected indentation of 4 spaces but found 8.
(indent)
[error] 62-62: Expected indentation of 4 spaces but found 8.
(indent)
[error] 62-62: Replace ····const·ReactRefreshPlugin·=·require("@rspack/plugin-react-refresh"); with const·ReactRefreshPlugin·=·require("@rspack/plugin-react-refresh")
(prettier/prettier)
[error] 63-63: Delete ····
(prettier/prettier)
[error] 63-63: Expected indentation of 4 spaces but found 8.
(indent)
[error] 64-64: Replace ············ with ······
(prettier/prettier)
[error] 64-64: Expected indentation of 6 spaces but found 12.
(indent)
[error] 65-65: Expected indentation of 6 spaces but found 12.
(indent)
[error] 65-65: Delete ······
(prettier/prettier)
[error] 66-66: Replace ········]; with ····]
(prettier/prettier)
[error] 66-66: Expected indentation of 4 spaces but found 8.
(indent)
[error] 67-67: Replace ···· with ··
(prettier/prettier)
[error] 67-67: Expected indentation of 2 spaces but found 4.
(indent)
[error] 68-68: Replace ····return·rspackConfig; with ··return·rspackConfig
(prettier/prettier)
[error] 68-68: Expected indentation of 2 spaces but found 4.
(indent)
[error] 69-69: Delete ;
(prettier/prettier)
[error] 70-70: Replace ·config.assets_bundler·===·"rspack"·?·rspackDevConfig()·:·webpackDevConfig(); with ⏎··config.assets_bundler·===·"rspack"·?·rspackDevConfig()·:·webpackDevConfig()
(prettier/prettier)
[error] 71-71: Delete ;
(prettier/prettier)
package/environments/test.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
[error] 7-7: Delete ;
(prettier/prettier)
[error] 8-8: Delete ;
(prettier/prettier)
[error] 9-9: Delete ;
(prettier/prettier)
[error] 15-15: Expected indentation of 2 spaces but found 4.
(indent)
[error] 15-15: Delete ··
(prettier/prettier)
[error] 16-16: Delete ··
(prettier/prettier)
[error] 16-16: Expected indentation of 2 spaces but found 4.
(indent)
[error] 17-17: Expected indentation of 2 spaces but found 4.
(indent)
[error] 17-17: Delete ··
(prettier/prettier)
[error] 18-18: Expected indentation of 2 spaces but found 4.
(indent)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 19-19: Delete ····
(prettier/prettier)
[error] 19-19: Expected indentation of 4 spaces but found 8.
(indent)
[error] 20-20: Expected indentation of 2 spaces but found 4.
(indent)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Delete ;
(prettier/prettier)
[error] 27-27: Delete ··
(prettier/prettier)
[error] 27-27: Expected indentation of 2 spaces but found 4.
(indent)
[error] 28-28: Expected indentation of 2 spaces but found 4.
(indent)
[error] 28-28: Delete ··
(prettier/prettier)
[error] 29-29: Delete ;
(prettier/prettier)
[error] 35-35: Replace ···· with ··
(prettier/prettier)
[error] 35-35: Expected indentation of 2 spaces but found 4.
(indent)
[error] 36-36: Delete ··
(prettier/prettier)
[error] 36-36: Expected indentation of 2 spaces but found 4.
(indent)
[error] 37-37: Delete ;
(prettier/prettier)
[error] 38-38: Replace ·config.assets_bundler·===·"rspack"·?·rspackTestConfig()·:·webpackTestConfig(); with ⏎··config.assets_bundler·===·"rspack"·?·rspackTestConfig()·:·webpackTestConfig()
(prettier/prettier)
[error] 39-39: Delete ;
(prettier/prettier)
package/environments/types.js
[error] 1-1: 'use strict' is unnecessary inside of modules.
(strict)
[error] 1-1: Expected newline after "use strict" directive.
(lines-around-directive)
[error] 1-1: Delete ;
(prettier/prettier)
[error] 6-6: Delete ;
(prettier/prettier)
🪛 GitHub Actions: Node based checks
package/environments/base.js
[error] 1-1: eslint: 'use strict' is unnecessary inside of modules. Command failed: eslint . (invoked by 'yarn lint').
🪛 GitHub Check: Linting
package/environments/base.js
[failure] 9-9:
Replace __dirname,·"..",·"plugins",·${config.assets_bundler}.js); with ⏎··__dirname,⏎··"..",⏎··"plugins",⏎··${config.assets_bundler}.js⏎)
[failure] 8-8:
Delete ;
[failure] 8-8:
Expected 1 empty line after require statement not followed by another require
[failure] 7-7:
Delete ;
[failure] 6-6:
Delete ;
[failure] 5-5:
Delete ;
[failure] 4-4:
Delete ;
[failure] 1-1:
Delete ;
[failure] 1-1:
Expected newline after "use strict" directive
[failure] 1-1:
'use strict' is unnecessary inside of modules
🔇 Additional comments (18)
package/environments/types.js (1)
6-6: Remove semicolon to match project style.The trailing semicolon violates the project's Prettier configuration.
Apply this diff:
-Object.defineProperty(exports, "__esModule", { value: true }); +Object.defineProperty(exports, "__esModule", { value: true })package/utils/typeGuards.ts (6)
14-25: Caching isWatchMode() effectively addresses the performance concern.The cached approach avoids repeated
process.argv.includes()calls on every validation, resolving the prior review feedback about efficiency.Based on past review comments.
30-43: getCacheTTL caching is sound; TTL values align with stated defaults.The lazy initialization correctly caches the computed TTL, and the defaults match the PR objectives (5s watch, 1min dev, infinite prod).
72-79: Cache hit logic with TTL check is correct.The timestamp comparison
(Date.now() - cached.timestamp) < getCacheTTL()correctly implements TTL expiration.
100-104: Path traversal security checks are properly integrated.Using
isPathTraversalSafefor all path fields (includingadditional_pathsarray) and logging warnings on rejection provides defense-in-depth against directory traversal.Also applies to: 134-141
189-191: Port validation correctly delegates to the centralized utility.Replacing inline checks with
validatePort(config.port)promotes consistency and maintainability.
55-61: clearValidationCache is a no-op; WeakMap cannot be cleared.The function logs a message but doesn't clear the cache—WeakMaps lack a
.clear()method. To actually invalidate cached entries, reassign the WeakMap:+let validatedConfigs = new WeakMap<object, CacheEntry>() -const validatedConfigs = new WeakMap<object, CacheEntry>() export function clearValidationCache(): void { - // WeakMap doesn't have a clear method, but we can create a new one - // This is handled by garbage collection + validatedConfigs = new WeakMap<object, CacheEntry>() if (debugCache) { console.log('[SHAKAPACKER DEBUG] Validation cache cleared') } }Based on past review comments (Copilot noted this same issue).
package/environments/test.js (2)
14-37: Bundler parity achieved—both webpack and rspack now share identical test settings.The
sharedTestConfigcorrectly ensures both bundlers usemode: "development",devtool: "cheap-module-source-map", andwatchOptions, resolving the past review concern about divergent test behaviors.Based on past review comments (CR flagged webpack/rspack test-env parity issue).
1-40: Formatting issues resolved. Removed"use strict", applied 2-space indentation, and removed semicolons per project style; file linted with Prettier and ESLint.package/environments/development.js (3)
24-41: webpackDevConfig React Refresh integration is sound.The conditional plugin attachment correctly guards against missing dependencies using
moduleExists, and the plugin array spread preserves any existing plugins.
46-69: rspackDevConfig correctly wires devMiddleware and React Refresh.The
writeToDiskfilter excluding.hot-update.files and the conditional React Refresh plugin attachment are both appropriate for rspack's requirements.
1-72: Resolved: Formatting violations addressed. The formatting issues in package/environments/development.js have been automatically fixed via Prettier and ESLint --fix.test/typescript/environments.test.js (1)
7-82: Test coverage for environment modules is comprehensive.The suite effectively validates structure, mode settings, devtool configuration, and cross-bundler consistency for all three environment configs.
package/environments/base.js (1)
1-104: Formatting issues resolvedFile now aligns with project ESLint/Prettier conventions and CI errors should be resolved.
package/environments/production.js (4)
46-52: Production configuration looks solid.The production config correctly sets:
- Full source maps for debugging production issues
- Bail-on-error for CI/CD reliability
- Normal stats output
- Dynamic plugins based on available dependencies
- Bundler-specific optimizations
53-59: Content hash validation provides helpful user feedback.The warning correctly prevents users from accidentally disabling content hashing in production, which is important for cache busting. The message clearly explains the override behavior.
60-60: Config merge follows established pattern.The export correctly merges base configuration with production-specific overrides using webpack-merge, consistent with the development environment approach.
1-60: Formatting issues persist despite previous fixes.The ESLint violations flagged in earlier reviews (semicolons, indentation, whitespace) are still present in this file. Previous comments indicated these were addressed in commits 66e95a9, 507e808-8ff1630, but the violations remain:
- Line 1:
'use strict'unnecessary in modules + missing newline- Throughout: Extra semicolons prettier wants removed
- Lines 18-41: Incorrect indentation (4 spaces instead of 2)
- Line 13: Missing blank line after require statement
Since this is compiled TypeScript output, ensure the build pipeline includes a post-compilation formatting step.
Run the formatter as part of the build process:
#!/bin/bash # Format the compiled JavaScript npx prettier --write package/environments/production.js # Verify no ESLint errors remain npx eslint package/environments/production.jsBased on learnings: Always run
yarn lintbefore committing JavaScript changes.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package/utils/errorHelpers.ts (1)
34-50: Fix incorrect error code mapping for delete operation.Line 43 maps the
deleteoperation toFILE_NOT_FOUND, but this is semantically incorrect. A delete operation failure is not equivalent to a file not found error—it could fail due to permissions, locks, or other reasons. UseFILE_WRITE_ERRORor introduce a dedicatedFILE_DELETE_ERRORcode.export function createFileOperationError( operation: 'read' | 'write' | 'delete', filePath: string, details?: string ): ShakapackerError { const errorCode = operation === 'read' ? ErrorCode.FILE_READ_ERROR : operation === 'write' ? ErrorCode.FILE_WRITE_ERROR - : ErrorCode.FILE_NOT_FOUND + : ErrorCode.FILE_WRITE_ERROR // or add FILE_DELETE_ERROR if needed return new ShakapackerError(errorCode, { path: filePath, operation, details }) }package/utils/typeGuards.ts (1)
67-79: Implement config mutation detection or enforce immutability.
The optionalconfigHashinCacheEntryis never set, so mutating a cached config object after its first validation will return stale results. Either compute and store a hash when caching (and compare it before returning) or document and enforce that config objects must be deeply immutable once validated.
🧹 Nitpick comments (4)
package/utils/errorCodes.ts (1)
186-194: Consider excluding stack from JSON in production.Including the full stack trace in
toJSON()may leak internal implementation details in production environments. Consider conditionally excluding the stack based onNODE_ENV:toJSON(): Record<string, any> { + const includeStack = process.env.NODE_ENV !== 'production' return { name: this.name, code: this.code, message: this.message, details: this.details, - stack: this.stack + ...(includeStack && { stack: this.stack }) } }package/utils/errorHelpers.ts (1)
55-68: Inconsistent return type annotation.The function signature at line 59 declares
return type: Error, but the implementation returnsnew Error(...)which is correct. However, the function name includes "Legacy" suggesting this is for backward compatibility. Consider documenting the migration path and deprecation timeline.Add a JSDoc deprecation notice:
/** * Creates a consistent error message for file operations (backward compatibility) + * @deprecated Use createFileOperationError instead, which returns ShakapackerError with structured error codes */ export function createFileOperationErrorLegacy( operation: 'read' | 'write' | 'delete', filePath: string, details?: string ): Error {package/utils/typeGuards.ts (2)
93-105: Performance concern: Security validation in hot path.Lines 99-104 add path traversal checks inside the validation loop. This executes regex patterns on every path field during every validation call. In watch mode with 5s TTL, this could be called frequently. Consider:
- Only running security checks when
shouldValidateis true (like DevServerConfig does)- Or caching path validation results separately
for (const field of requiredStringFields) { if (typeof config[field] !== 'string') { // Cache negative result validatedConfigs.set(obj as object, { result: false, timestamp: Date.now() }) return false } - // Validate paths for security - if (field.includes('path') && !isPathTraversalSafe(config[field] as string)) { + // Validate paths for security (only in dev or when strict validation is enabled) + if (shouldValidate && field.includes('path') && !isPathTraversalSafe(config[field] as string)) { console.warn(`[SHAKAPACKER SECURITY] Invalid path in ${field}: ${config[field]}`) validatedConfigs.set(obj as object, { result: false, timestamp: Date.now() }) return false } }
196-225: Rspack plugin validator may produce false negatives.The
isValidRspackPluginfunction uses heuristics (apply method, constructor name, name property) that could fail for:
- Plugins using Symbols or non-standard property names
- Minified code where constructor names are mangled
- Plain objects with plugin-like shapes but no apply method
Consider documenting the limitations and whether this is intended as a loose "duck typing" check or a strict validation.
Add JSDoc clarifying the validation approach:
/** * Type guard to validate Rspack plugin instance - * Checks if an object looks like a valid Rspack plugin + * Checks if an object looks like a valid Rspack plugin using heuristics. + * Note: This is a loose "duck typing" check. It may not catch all invalid plugins, + * especially in minified code or with non-standard plugin implementations. + * @param obj - Object to validate + * @returns true if obj appears to be a valid Rspack plugin */ export function isValidRspackPlugin(obj: unknown): boolean {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
README.md(1 hunks)docs/typescript-migration.md(1 hunks)package.json(2 hunks)package/environments/base.js(0 hunks)package/environments/development.js(2 hunks)package/environments/production.js(2 hunks)package/environments/test.js(1 hunks)package/environments/types.js(1 hunks)package/types/README.md(1 hunks)package/types/index.ts(1 hunks)package/utils/errorCodes.ts(1 hunks)package/utils/errorHelpers.ts(3 hunks)package/utils/typeGuards.ts(5 hunks)scripts/remove-use-strict.js(1 hunks)test/typescript/build.test.js(1 hunks)test/typescript/environments.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- package/environments/base.js
✅ Files skipped from review due to trivial changes (2)
- docs/typescript-migration.md
- package/types/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- package/environments/types.js
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Always run `yarn lint` before committing JavaScript changes
Applied to files:
package/environments/production.jspackage.json
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Test changes with both bundlers (webpack and rspack) when modifying core functionality
Applied to files:
package/environments/development.jspackage/environments/test.js
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: This gem supports both webpack and rspack configurations
Applied to files:
package/environments/development.jspackage/environments/test.jsREADME.md
🧬 Code graph analysis (6)
package/environments/production.js (1)
package/environments/base.js (6)
require(3-3)require(4-4)require(7-7)require(14-14)baseConfig(82-113)config(6-6)
package/environments/development.js (2)
package/environments/production.js (6)
require(8-8)require(9-9)require(11-11)require(19-19)config(12-12)baseConfig(10-10)package/environments/base.js (6)
require(3-3)require(4-4)require(7-7)require(14-14)config(6-6)baseConfig(82-113)
package/utils/errorHelpers.ts (1)
package/utils/errorCodes.ts (1)
ShakapackerError(148-195)
package/utils/typeGuards.ts (1)
package/utils/pathValidation.ts (2)
isPathTraversalSafe(11-26)validatePort(121-139)
package/environments/test.js (1)
package/environments/base.js (6)
require(3-3)require(4-4)require(7-7)require(14-14)config(6-6)baseConfig(82-113)
test/typescript/environments.test.js (4)
package/environments/development.js (4)
require(6-6)require(10-10)require(11-11)config(7-7)package/environments/production.js (6)
require(8-8)require(9-9)require(11-11)require(19-19)productionConfig(54-60)config(12-12)package/environments/test.js (2)
require(6-6)config(7-7)package/environments/base.js (5)
require(3-3)require(4-4)require(7-7)require(14-14)config(6-6)
🪛 ESLint
package/environments/production.js
[error] 12-12: Expected 1 empty line after require statement not followed by another require.
(import/newline-after-import)
test/typescript/environments.test.js
[error] 90-90: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 97-104: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 98-98: Use object destructuring.
(prefer-destructuring)
[error] 100-103: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 100-102: Avoid having conditionals in tests
(jest/no-conditional-in-test)
[error] 100-101: Avoid having conditionals in tests
(jest/no-conditional-in-test)
scripts/remove-use-strict.js
[error] 35-35: Insert ⏎
(prettier/prettier)
🪛 GitHub Check: Linting
package/environments/production.js
[failure] 12-12:
Expected 1 empty line after require statement not followed by another require
test/typescript/environments.test.js
[failure] 100-100:
Avoid having conditionals in tests
[failure] 100-100:
Avoid having conditionals in tests
[failure] 100-100:
Avoid having conditionals in tests
[failure] 98-98:
Use object destructuring
[failure] 97-97:
Avoid having conditionals in tests
[failure] 90-90:
Avoid having conditionals in tests
scripts/remove-use-strict.js
[failure] 35-35:
Insert ⏎
⏰ 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). (20)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Test with Webpack
- GitHub Check: test
🔇 Additional comments (22)
README.md (1)
648-651: LGTM! Clear documentation for transpiler switching.This new section provides clear guidance for users on how to switch between transpilers and configure environment-specific settings, with a helpful reference to the detailed migration guide.
test/typescript/build.test.js (1)
27-30: LGTM! Test correctly verifies CommonJS compilation output.The updated assertions properly verify that TypeScript files are compiled to CommonJS format by checking for
require()andmodule.exports, which aligns with the build process and distribution requirements.package/environments/development.js (4)
1-14: LGTM! Good documentation structure.The module-level JSDoc comments and base configuration documentation improve code maintainability and clarity.
29-41: LGTM! Conditional React Refresh plugin loading.The runtime check for
runningWebpackDevServer,devServerConfig.hot, and module existence before loading the plugin is a good defensive programming practice. This prevents errors when the plugin is not installed or when not running the dev server.
54-54: LGTM! Safe fallback prevents potential errors.The fallback to an empty object
...(devServerConfig.devMiddleware || {})prevents potential runtime errors ifdevMiddlewareis undefined. This defensive approach aligns with the PR's focus on safety and type-safety improvements.
59-71: LGTM! Rspack React Refresh properly guarded.The conditional loading of
@rspack/plugin-react-refreshfollows the same pattern as the webpack implementation, ensuring consistent behavior across both bundlers. This aligns with the learning to test changes with both bundlers.Based on learnings
package.json (3)
19-19: LGTM! New types export for consumers.The new
"./types"export provides a clean entry point for consumers to import types from the package, improving the TypeScript developer experience.
34-34: LGTM! Build pipeline correctly sequenced.The build script properly chains TypeScript compilation, "use strict" removal, and code formatting. This ensures consistent output format for distribution.
39-39: LGTM! prepublishOnly ensures quality before publishing.Running both
buildandtype-checkbefore publishing is a good safety measure to ensure the package is properly compiled and type-safe before distribution. This prevents publishing broken or incorrectly compiled code.package/utils/errorCodes.ts (2)
1-74: LGTM! Well-structured error code taxonomy.The enum provides comprehensive coverage across configuration, filesystem, modules, environment, bundler, dev server, security, validation, and generic categories. The numeric prefixes (1xxx–9xxx) create logical groupings that will aid programmatic error handling and monitoring.
170-181: Template replacement is safe. TheformatMessagemethod only performs literal regex-based substitutions and serializes objects viaJSON.stringify; it does not evaluate any injected code. Alldetailspassed (path,operation,reason, etc.) originate from controlled helper functions and are simple strings or serializable values. No direct injection vector exists. If these messages are later rendered in HTML or another unsafe context, escape them there.package/utils/errorHelpers.ts (2)
92-98: LGTM! Type guard properly refined.The cast to
NodeJS.ErrnoExceptionwhen checking thecodeproperty correctly narrows the type within the guard.
100-141: Check integration of new error helpersI didn’t find any usages of these helpers beyond their own definitions. Please ensure you invoke them in the appropriate error-handling paths and that downstream code handles their specific
ErrorCodevalues correctly.package/utils/typeGuards.ts (4)
13-43: Cache initialization addresses previous feedback.The cached watch mode and TTL computation (lines 14-15, 20-43) properly addresses the previous Copilot comment about calling
process.argv.includes()on every module load. The values are now lazily computed and cached.
55-61: WeakMap reassignment correctly implements cache clearing.Line 57 properly reassigns the WeakMap to clear all entries, addressing the previous Copilot comment. The implementation is correct since WeakMaps cannot be cleared via a method.
189-191: LGTM! Delegation to validatePort improves consistency.Replacing inline port validation with the
validatePortutility frompathValidation.tsensures consistent validation logic across the codebase.
227-237: Explicitly handle empty plugin arrays
TheisValidRspackPluginArrayguard currently returnstruefor[](since[].every()yieldstrue). If an empty plugin list is invalid, add a check beforeevery, e.g.:if (arr.length === 0) { return false }Otherwise confirm that allowing empty arrays is intended.
(package/utils/typeGuards.ts:231-237)package/types/index.ts (1)
1-60: LGTM! Clean barrel export pattern.The type exports are well-organized, properly documented, and use type-only exports throughout. The JSDoc example clearly demonstrates usage for consumers.
package/environments/test.js (1)
13-36: Excellent resolution of the test environment parity issue.The
sharedTestConfigapproach ensures both webpack and rspack use identical test settings (mode: "development", devtool, watchOptions), addressing the previous review concern. The structure also allows for future bundler-specific overrides if needed.package/environments/production.js (3)
29-50: LGTM! Compression plugin configuration is well-structured.The conditional loading of compression plugins is correct:
- Checks for
compression-webpack-pluginavailability before use- Validates brotli support via
process.versions.brotli- Provides sensible defaults for filename patterns and test regex
54-68: LGTM! Production configuration is sound.The production config appropriately sets:
devtool: "source-map"for production debuggingbail: trueto fail fast on errors- Dynamic plugins and optimization via dedicated functions
The
useContentHashwarning is a helpful safeguard for production builds.
12-13: Add blank line after require statement.ESLint continues to flag this (rule
import/newline-after-import). Insert a blank line after line 12:const config = require("../config") + const optimizationPath = resolve(This was flagged in previous reviews but remains unfixed. Ensure
yarn lint --fixis run before committing.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
test/scripts/remove-use-strict.test.js (1)
91-91: Use.toHaveLength()matcher for better test readability.Jest provides a more idiomatic matcher for length assertions.
- expect(result.match(/\n$/g).length).toBe(1) + expect(result.match(/\n$/g)).toHaveLength(1)scripts/remove-use-strict.js (1)
6-20: Consider error handling for directory operations.The
fs.readdirSynccall (line 8) will throw if the directory doesn't exist or isn't accessible. While the hardcoded "package" directory is expected to exist in the build context, adding a guard would make the script more robust.// Recursively find all .js files in a directory function findJsFiles(dir) { + if (!fs.existsSync(dir)) { + return [] + } const files = [] const items = fs.readdirSync(dir, { withFileTypes: true })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package/utils/typeGuards.ts(6 hunks)scripts/remove-use-strict.js(1 hunks)test/scripts/remove-use-strict.test.js(1 hunks)test/typescript/environments.test.js(1 hunks)test/typescript/securityValidation.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Always end all files with a trailing newline character
Applied to files:
scripts/remove-use-strict.js
🧬 Code graph analysis (4)
test/scripts/remove-use-strict.test.js (1)
scripts/remove-use-strict.js (3)
fs(2-2)path(3-3)content(26-26)
test/typescript/environments.test.js (4)
package/environments/production.js (6)
require(8-8)require(9-9)require(11-11)require(19-19)productionConfig(54-60)config(12-12)package/environments/base.js (5)
require(3-3)require(4-4)require(7-7)require(14-14)config(6-6)package/environments/test.js (2)
require(6-6)config(7-7)package/environments/development.js (4)
require(6-6)require(10-10)require(11-11)config(7-7)
package/utils/typeGuards.ts (2)
package/types.ts (1)
Config(13-43)package/utils/pathValidation.ts (2)
isPathTraversalSafe(11-26)validatePort(121-139)
test/typescript/securityValidation.test.js (1)
package/utils/typeGuards.ts (2)
clearValidationCache(57-63)isValidConfig(72-179)
🪛 ESLint
test/scripts/remove-use-strict.test.js
[error] 27-27: Replace "scripts/remove-use-strict.js",·"utf8" with ⏎······"scripts/remove-use-strict.js",⏎······"utf8"⏎····
(prettier/prettier)
[error] 29-29: Replace 'findJsFiles("package")',·findJsFiles("${directory}")`` with ⏎······'findJsFiles("package")',⏎······findJsFiles("${directory}")`⏎····`
(prettier/prettier)
[error] 50-50: Replace "test3.js",·'··\t"use·strict";\nconst·x·=·1;' with ⏎······"test3.js",⏎······'··\t"use·strict";\nconst·x·=·1;'⏎····
(prettier/prettier)
[error] 57-57: Replace "test4.js",·'"use·strict";··\n\n\nconst·x·=·1;' with ⏎······"test4.js",⏎······'"use·strict";··\n\n\nconst·x·=·1;'⏎····
(prettier/prettier)
[error] 64-64: Replace "test5.js",·'\u201Cuse·strict\u201D;\nconst·x·=·1;' with ⏎······"test5.js",⏎······"\u201Cuse·strict\u201D;\nconst·x·=·1;"⏎····
(prettier/prettier)
[error] 79-79: Replace "test7.js",·'const·y·=·2;\n"use·strict";\nconst·x·=·1;' with ⏎······"test7.js",⏎······'const·y·=·2;\n"use·strict";\nconst·x·=·1;'⏎····
(prettier/prettier)
[error] 91-91: Use toHaveLength() instead
(jest/prefer-to-have-length)
[error] 107-107: Insert ⏎
(prettier/prettier)
test/typescript/securityValidation.test.js
[error] 1-1: Replace ·isValidConfig,·clearValidationCache· with ⏎··isValidConfig,⏎··clearValidationCache⏎
(prettier/prettier)
[error] 13-13: describes should begin with lowercase
(jest/prefer-lowercase-title)
[error] 102-102: describes should begin with lowercase
(jest/prefer-lowercase-title)
[error] 179-179: Insert ⏎
(prettier/prettier)
scripts/remove-use-strict.js
[error] 45-45: Insert ⏎
(prettier/prettier)
🪛 GitHub Check: Linting
test/scripts/remove-use-strict.test.js
[failure] 91-91:
Use toHaveLength() instead
[failure] 79-79:
Replace "test7.js",·'const·y·=·2;\n"use·strict";\nconst·x·=·1;' with ⏎······"test7.js",⏎······'const·y·=·2;\n"use·strict";\nconst·x·=·1;'⏎····
[failure] 64-64:
Replace "test5.js",·'\u201Cuse·strict\u201D;\nconst·x·=·1;' with ⏎······"test5.js",⏎······"\u201Cuse·strict\u201D;\nconst·x·=·1;"⏎····
[failure] 57-57:
Replace "test4.js",·'"use·strict";··\n\n\nconst·x·=·1;' with ⏎······"test4.js",⏎······'"use·strict";··\n\n\nconst·x·=·1;'⏎····
[failure] 50-50:
Replace "test3.js",·'··\t"use·strict";\nconst·x·=·1;' with ⏎······"test3.js",⏎······'··\t"use·strict";\nconst·x·=·1;'⏎····
[failure] 29-29:
Replace 'findJsFiles("package")',·findJsFiles("${directory}")`` with ⏎······'findJsFiles("package")',⏎······findJsFiles("${directory}")`⏎····`
[failure] 27-27:
Replace "scripts/remove-use-strict.js",·"utf8" with ⏎······"scripts/remove-use-strict.js",⏎······"utf8"⏎····
scripts/remove-use-strict.js
[failure] 45-45:
Insert ⏎
⏰ 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). (20)
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: test
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: claude-review
🔇 Additional comments (17)
test/scripts/remove-use-strict.test.js (1)
35-106: LGTM! Comprehensive test coverage.The test suite thoroughly covers edge cases including:
- Various quote styles (single, double, unicode)
- Optional semicolons
- Whitespace variations (leading, trailing, multiple newlines)
- Windows line endings
- Trailing newline normalization
- Non-directive content preservation
This provides strong validation for the script's behavior.
scripts/remove-use-strict.js (3)
28-34: LGTM! Regex handles comprehensive "use strict" variations.The regex pattern successfully addresses:
- Leading whitespace (
^\s*)- Multiple quote styles (single, double, unicode:
\u2018\u2019\u201C\u201D)- Flexible spacing in "use strict" (
use\s+strict)- Optional semicolon (
;?)- Trailing whitespace and line endings (
\s*[\r\n]*)This aligns well with the test coverage and handles the edge cases mentioned in past reviews.
36-39: LGTM! Proper newline normalization.The logic correctly ensures each processed file ends with exactly one newline, addressing the EOF requirement.
45-45: Fix: Add newline at end of file.The file must end with a newline character to satisfy prettier/prettier formatting requirements.
// eslint-disable-next-line no-console -console.log(`Removed "use strict" from ${files.length} files`) +console.log(`Removed "use strict" from ${files.length} files`) +Based on learnings.
test/typescript/environments.test.js (4)
7-25: LGTM! Development config tests are well-structured.The test suite properly validates the development configuration structure, mode, devtool, and usability as a webpack configuration.
27-51: LGTM! Production config tests cover key features.The test suite appropriately validates production-specific features including optimization configuration and plugins array.
53-70: LGTM! Test config validation is correct.The mode validation correctly uses
toContainwith an array of allowed values, avoiding conditional expect issues.
72-96: LGTM! Type safety tests use clean unconditional assertions.The dev server validation cleverly uses a ternary expression to normalize the port value before validation, avoiding conditional operators inside expect statements. This satisfies the jest/no-conditional-in-test rule while still properly validating all allowed port types including "auto".
package/utils/typeGuards.ts (7)
4-43: LGTM! Well-designed caching infrastructure.The TTL-based caching implementation is sound:
- Uses WeakMap to prevent memory leaks
- Lazy-loads cached values to avoid unnecessary checks
- Sensible TTL defaults based on environment (Infinity in prod, 5s watch, 1min dev)
- Supports override via SHAKAPACKER_CACHE_TTL
57-63: LGTM! Cache clearing correctly implemented.The function now properly clears the cache by reassigning the WeakMap, addressing the past review concern.
104-110: LGTM! Security checks correctly bypass performance gates.The path traversal validation for required path fields always runs regardless of
shouldValidate(), ensuring security is never compromised for performance. This is the correct approach.
140-148: LGTM! Critical security check always enforced.The path traversal validation for
additional_pathscorrectly runs unconditionally (not gated byshouldValidate()). The comment clearly documents this is intentional security behavior, ensuring user-provided paths cannot escape the project directory regardless of environment or validation mode.
152-179: LGTM! Proper separation of security and deep validation.The code correctly separates security checks (which always run) from deep optional field validation (which respects
shouldValidate()). This provides the right balance between security and performance.
185-209: LGTM! Dev server validation properly delegates to utilities.The function correctly uses the imported
validatePortutility and respects theshouldValidate()gate for performance optimization.
215-252: LGTM! Pragmatic plugin validation heuristics.The plugin validation functions use reasonable heuristics:
- Checks for
applymethod (standard webpack/rspack plugin interface)- Validates constructor name patterns
- Falls back to checking for
namepropertyThis approach is appropriate given the dynamic nature of JavaScript plugins and the variety of plugin patterns in the webpack/rspack ecosystem.
test/typescript/securityValidation.test.js (2)
14-99: LGTM! Comprehensive security test coverage.The path traversal security tests thoroughly validate that:
- Security checks always run in production (without strict mode)
- Security checks always run in development
- Both required path fields and additional_paths are validated
- Safe paths are allowed in both environments
This provides strong coverage of the security validation logic.
129-177: LGTM! Excellent validation gating test coverage.These tests perfectly demonstrate the three validation modes:
- Production without strict mode: skips deep validation (invalid optional fields pass)
- Development: performs deep validation (invalid optional fields fail)
- Production with strict mode: performs deep validation (invalid optional fields fail)
This validates the security/performance trade-off is working as designed.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // SECURITY: Path traversal validation ALWAYS runs (not subject to shouldValidate) | ||
| // This ensures paths are safe regardless of environment or validation mode | ||
| if (field.includes('path') && !isPathTraversalSafe(config[field] as string)) { | ||
| console.warn(`[SHAKAPACKER SECURITY] Invalid path in ${field}: ${config[field]}`) | ||
| validatedConfigs.set(obj as object, { result: false, timestamp: Date.now() }) | ||
| return false |
There was a problem hiding this comment.
Good security practice to always validate paths regardless of environment. However, consider using a more specific check than field.includes('path') as this could match unrelated fields. A whitelist of known path fields would be more secure.
| // First check if the string contains only digits | ||
| if (!/^\d+$/.test(port)) { | ||
| return false |
There was a problem hiding this comment.
This regex correctly validates that port strings contain only digits, preventing injection attacks through malformed port values. The validation is appropriately strict.
Fixed linting issues from CI: - Added trailing newline to .npmignore (required by project rules) - Fixed import/newline-after-import in base.js and production.js - Replaced optional chaining with conditional check for older JS support - Fixed jest/prefer-lowercase-title in securityValidation.test.js - Fixed jest/prefer-to-have-length in remove-use-strict.test.js - Auto-fixed all Prettier formatting issues All tests passing locally (26 suites, 130 tests). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The Bundler.with_unbundled_env block was clearing the SHAKAPACKER_NPM_PACKAGE environment variable set by GitHub Actions, causing the install to fail to use the local tarball. Fixed by capturing the env var before entering the unbundled_env block and passing it explicitly in the command string, similar to how other env vars like SHAKAPACKER_ASSETS_BUNDLER are passed. This allows the CI to properly test with the locally-built npm package tarball. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Different package managers store tarball paths differently in package.json: - npm/pnpm: "file:/path/to/package.tgz" - yarn/bun: "/path/to/package.tgz" (no file: prefix) Updated the test to match the tarball filename pattern instead of requiring the "file:" prefix, allowing all package managers to pass. This fixes the remaining 2 generator spec failures for yarn_classic and bun. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Node (ESLint): - Fixed jest/no-conditional-in-test in environments.test.js - Moved conditional logic outside expect() call - Used if statement to guard port validation instead of && operator Ruby (RuboCop): - Fixed Style/StringLiterals in generator_spec.rb - Changed single quotes to double quotes for ENV hash access - Follows project's string literal style guide All tests still passing (13/13 in environments.test.js). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Ruby (RuboCop): - Fixed Layout/IndentationWidth in generator_spec.rb - Fixed Layout/EndAlignment for if/else/end block - Used proper 2-space indentation for Ruby if statement Node (ESLint): - Fixed jest/no-conditional-in-test in environments.test.js - Removed all conditional operators (if, ternary, ||, &&) - Used array.find() pattern to select type without conditionals - Used object destructuring with defaults to handle undefined All tests passing (13/13 in environments.test.js). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
… parsing The e2e template React files were using .js extension, but SWC only recognizes JSX syntax when using .jsx or .tsx extensions. This caused generator specs to fail with "Expression expected" errors when trying to parse JSX. Changed: - app/javascript/packs/application.js → application.jsx - app/javascript/components/App.js → App.jsx SWC's isJsxFile() check requires jsx/tsx extension to enable JSX parser. Since SWC is now the default transpiler (shakapacker.yml), all React files in templates must use proper jsx/tsx extensions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ublish The environment JS files are compiled from TypeScript and should not be committed to git. They'll be generated during `npm publish` via the existing prepublishOnly script. Changes: - Remove `!package/environments/*.js` from .gitignore - Delete committed environment/*.js files from git - Files will be regenerated by `yarn build` (via prepublishOnly) This prevents the TS source and compiled JS from getting out of sync. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The test was incorrectly assuming bun would always get a semver version,
but when ENV["SHAKAPACKER_NPM_PACKAGE"] is set, bun installs from the
tarball and package.json contains the tarball path (since bun link
doesn't modify package.json).
Fixed logic:
- bun + ENV set: expect tarball path
- bun + ENV not set: expect semver
- non-bun: always expect file:#{GEM_ROOT} (we overwrite package.json)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
phase-3-typescript-environments
branch
from
571a839 to
ad3b98b
Compare
Comprehensive Code Review - PR #614Thank you for this substantial PR! I've conducted a thorough review. Below is my detailed feedback. Overall: APPROVE WITH RECOMMENDATIONSSummary:
Critical Issues (Must Fix)1. Unsafe Use of any - package/environments/types.ts:22-25 2. Unsafe Type Assertion - package/config.ts:76,88,103 3. Unsafe YAML Loading - package/config.ts:15, package/env.ts:36 High Priority4. Missing Type Guards - package/environments/development.ts:44,77 5. Port Validation - package/utils/pathValidation.ts:121-138 6. Type Definition - package/types.ts:36 Build System Issues7. Redundant package/.npmignore 8. Package Manager Dependency 9. No Package Verification Test Coverage: B+✅ Excellent security testing Security: ExcellentPath traversal protection comprehensive. Only concern is YAML loading (issue #3). RecommendationsBefore Merge:
Before Release: Final VerdictHigh-quality work with excellent security. Approve after addressing critical issues #1-3 and verifying package contents. Risk Assessment:
Great work! 🚀 |
The SHAKAPACKER_NPM_PACKAGE environment variable was used directly without validation, allowing potential directory traversal attacks and invalid file types to be processed. Added validation that: 1. Resolves to absolute path to prevent relative path issues 2. Rejects any path containing ".." directory traversal segments 3. Enforces .tgz or .tar.gz file extensions only 4. Checks File.exist? only after validation passes 5. Provides clear error messages and exits on validation failure This prevents malicious paths like: - "../../../etc/passwd" - "/tmp/malicious.zip" - "~/.ssh/id_rsa" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
PR Review: Phase 3 TypeScript Migration - Environment FilesOverall AssessmentThis is a well-executed TypeScript migration that maintains backward compatibility while significantly improving code quality, type safety, and security. The PR demonstrates careful attention to feedback from Phase 2 and includes comprehensive security improvements. ✅ Strengths1. Excellent Security Improvements
The security implementation is production-ready with proper:
2. Type Safety Enhancements
3. Build System Improvements
4. Documentation
5. Test CoverageExcellent test coverage including:
🔍 Areas for Consideration1. Missing
|
Add comprehensive changelog entry for Phase 3 of the TypeScript migration, covering: - TypeScript migration of environment files - Security improvements (path validation, env sanitization) - Build process improvements (publish-time compilation) - Performance optimizations (validation caching) - Bug fixes (SWC .jsx parsing, generator specs, cache clearing) All changes maintain backward compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add comprehensive changelog entries for v9.0.0-beta.7 based on origin/main: Added (non-doc/test only): - Phase 2 TypeScript Migration - Core Modules (PR #608) - Phase 3 TypeScript Migration - Environment Files (PR #614) - Optional Peer Dependencies (PR #615) - Migration Tooling Improvements (PR #613) Security: - Path validation utilities to prevent directory traversal - Environment variable sanitization - SHAKAPACKER_NPM_PACKAGE validation Changed: - Build process improvements (publish-time JS generation) Performance: - TTL-based validation caching Fixed: - clearValidationCache() bug - private_output_path edge cases (PR #604) Kept beta.4 section intact with all original breaking changes.
* docs: Update CHANGELOG for Phase 3 TypeScript migration (#614) Add comprehensive changelog entry for Phase 3 of the TypeScript migration, covering: - TypeScript migration of environment files - Security improvements (path validation, env sanitization) - Build process improvements (publish-time compilation) - Performance optimizations (validation caching) - Bug fixes (SWC .jsx parsing, generator specs, cache clearing) All changes maintain backward compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> * docs: Update CHANGELOG for v9.0.0-beta.7 release Add comprehensive changelog entries for v9.0.0-beta.7 based on origin/main: Added (non-doc/test only): - Phase 2 TypeScript Migration - Core Modules (PR #608) - Phase 3 TypeScript Migration - Environment Files (PR #614) - Optional Peer Dependencies (PR #615) - Migration Tooling Improvements (PR #613) Security: - Path validation utilities to prevent directory traversal - Environment variable sanitization - SHAKAPACKER_NPM_PACKAGE validation Changed: - Build process improvements (publish-time JS generation) Performance: - TTL-based validation caching Fixed: - clearValidationCache() bug - private_output_path edge cases (PR #604) Kept beta.4 section intact with all original breaking changes.
Summary
Completes Phase 3 of the TypeScript migration as outlined in #605. This PR converts all environment configuration files to TypeScript and addresses all feedback from the Phase 2 review.
Key Improvement: Environment JS files are now generated during
npm publishinstead of being committed, preventing TS/JS sync issues.What's Changed
Environment File Conversions
package/environments/development.ts- Development environment configurationpackage/environments/production.ts- Production environment configurationpackage/environments/test.ts- Test environment configurationpackage/environments/types.ts- Shared TypeScript interfaces for environment configs.jsfiles generated during publish, not committed to gitSecurity Improvements (from Phase 2 feedback)
Type Safety Improvements (from Phase 2 feedback)
as anywith proper typesany[]with specific plugin types (e.g.,WebpackPluginInstance[])Performance & Build Improvements
SHAKAPACKER_CACHE_TTLenvironment variableprepublishOnlyhook (not committed)Testing & Quality
yarn type-check).jsxextension requirement for SWC JSX parsing in e2e templatesBreaking Changes
None - The TypeScript files compile to JavaScript maintaining full backward compatibility.
The default JavaScript transpiler remains Babel for webpack and SWC for rspack to ensure backward compatibility.
Migration Progress
Related Issues
Testing Instructions