Skip to content

Fix webpack-dev-server static config: default to false, fix YAML passthrough - #1032

Merged
justin808 merged 10 commits into
mainfrom
ihabadham/fix/static-config-bugs
May 21, 2026
Merged

Fix webpack-dev-server static config: default to false, fix YAML passthrough#1032
justin808 merged 10 commits into
mainfrom
ihabadham/fix/static-config-bugs

Conversation

@ihabadham

@ihabadham ihabadham commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes three bugs in Shakapacker's webpack-dev-server static configuration that cause unnecessary inotify watches and ENOSPC crashes on systems with limited headroom.

  • Default static from a misconfigured object to false — Rails serves public/ via ActionDispatch::Static, webpack-dev-server doesn't need to
  • Fix if (devServerYamlConfig.static) truthy check so static: false in shakapacker.yml is no longer silently ignored
  • Remove static.watch from the default install template — it was a v3→v4 migration artifact that caused webpack-dev-server to watch public/ via chokidar
  • Pass through all valid webpack-dev-server static values (true, strings, arrays, objects) from YAML — if a user explicitly overrides the default, we respect it

Fixes #1031

Details

The old default set static.publicPath to a filesystem path (config.outputPath) instead of a URL path, and never set static.directory, causing webpack-dev-server to default to watching the entire public/ directory. This has been present since v6.0.0-rc.7 but was harmless on most systems. On systems near their inotify watch limit (e.g., Fedora with auto-calculated limits + watch-heavy tools like Warp terminal), it triggers ENOSPC: System limit for number of file watchers reached.

Setting static: false is safe because:

  • Shakapacker::DevServerProxy (hardwired in the railtie) proxies /packs/* to webpack-dev-server — the browser never accesses webpack-dev-server directly
  • Webpack bundles are served through devMiddleware, which is independent of static
  • ActionDispatch::Static serves public/ files in Rails

Users who explicitly set static in shakapacker.yml (to true, an object, a string path, etc.) will have their value passed through to webpack-dev-server as-is. Bare static: (YAML null) is treated the same as unset — the default false applies.

Test plan

  • Added 11 new tests in test/package/webpackDevServerConfig.test.js covering default behavior, false/true/string/object/array passthrough, and existing key mapping
  • All 13 tests in the suite pass (11 new + 2 existing middleware-hook warning tests)
  • yarn lint — 0 errors
  • yarn type-check — clean
  • Manual integration test against spec/dummy — wrote a harness that swaps the dummy app's shakapacker-webpack.yml and runs createDevServerConfig() against each scenario:
Scenario Result
No static key in YAML static: false (new default)
static: false (regression) static: false (previously silently ignored)
static: true static: true (passed through)
static: /custom/static "/custom/static" (string passed through)
static: { directory, watch.ignored } object passed through verbatim
static: [/path1, /path2] array preserved
static: ~ (YAML null) falls back to false

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes

  • Fixed webpack-dev-server static configuration to correctly default to false (previously defaulted to an invalid value that caused unintended watching of public/)
  • Corrected handling of static: false setting from configuration files to be properly respected instead of being silently ignored
  • Removed unnecessary watch configuration leftover from previous version migration

Review Change Stack

@coderabbitai

coderabbitai Bot commented Mar 30, 2026

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Defaults and YAML handling for webpack-dev-server static were corrected: default static is now false, explicit static: false in shakapacker.yml is honored, static.watch was removed from the default template, and tests were added to validate passthrough and key mappings.

Changes

webpack-dev-server static config fix

Layer / File(s) Summary
Changelog and default YAML
CHANGELOG.md, lib/install/config/shakapacker.yml
Adds changelog entry; removes the dev_server.static.watch.ignored entry from the development shakapacker.yml template (removes default static.watch).
Dev server config implementation
package/webpackDevServerConfig.ts
Adjusts ./config destructuring to use publicPath, widens WebpackDevServerConfig.static to `boolean
Tests
test/package/webpackDevServerConfig.test.js
Adds Jest setup and tests validating default static (false), YAML static passthrough for false/true/string/object/array, devMiddleware.publicPath, HMR (hot/hmr) and liveReload mapping, snake_case → camelCase key mapping, and client passthrough.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 I hopped through configs, sniffed the trail,
Where watchers peered in public vale,
I whispered "false" and cleared the way,
Now builds breathe easy, light of day,
A tiny carrot cheers the fix 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and concisely describes the main changes: fixing webpack-dev-server static configuration defaults and YAML passthrough behavior.
Linked Issues check ✅ Passed All linked issue #1031 objectives are met: default static is changed to false, static: false YAML is now honored, static.watch is removed from template, and all valid static values are passed through unchanged.
Out of Scope Changes check ✅ Passed All changes directly address the linked issues: CHANGELOG updates, template configuration removal, core config logic fixes, and comprehensive test coverage. No unrelated modifications detected.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ihabadham/fix/static-config-bugs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes three real bugs in Shakapacker's webpack-dev-server static configuration that could cause ENOSPC crashes on inotify-constrained systems. The core changes are: (1) defaulting static to false instead of a misconfigured object that triggered unnecessary public/ directory watching, (2) fixing the truthy-check guard so static: false in shakapacker.yml is no longer silently ignored, and (3) removing the stale static.watch block from the install template. The fix is well-motivated and the 8 new tests cover the main behavioral paths cleanly.

Key findings:

  • The CHANGELOG entry uses a placeholder [PR #XXXX] that should be updated to [PR #1032] before merge.
  • The new YAML passthrough logic only handles false and plain objects. Truthy non-object values — static: true, a string path, or an array — fall through both branches silently and leave config.static = false, which is the opposite of what a user specifying those values would expect. The WebpackDevServerConfig TypeScript type doesn't model these cases either, so the fix is incomplete for the full webpack-dev-server static API surface.

Confidence Score: 5/5

Safe to merge — the core bug fixes are correct and all remaining findings are P2 style/edge-case suggestions.

Both open findings are P2: the CHANGELOG placeholder is a trivial text fix, and the silent drop of non-object static values is an unlikely edge case that wasn't handled correctly in the old code either. The primary fix is sound, well-tested with 8 new tests, and addresses the real-world ENOSPC regression.

CHANGELOG.md (placeholder PR number) and package/webpackDevServerConfig.ts (incomplete passthrough for truthy non-object static values).

Important Files Changed

Filename Overview
package/webpackDevServerConfig.ts Defaults static to false, correctly handles false and object passthrough from YAML, but silently drops truthy non-object values (e.g. true, strings, arrays) leaving static: false with no warning.
test/package/webpackDevServerConfig.test.js New test file with 8 tests covering the default, false passthrough, object passthrough, devMiddleware.publicPath, hmr→hot mapping, liveReload default, snake_case→camelCase mapping, and client config passthrough — all using correct jest.resetModules() isolation pattern.
lib/install/config/shakapacker.yml Removes the static.watch.ignored block from the development section, eliminating the v3→v4 migration artifact that caused unnecessary inotify watches on public/.
CHANGELOG.md Adds a "Fixed" entry for the three bugs, but uses the placeholder #XXXX instead of the actual PR number #1032.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[createDevServerConfig called] --> B[Build base config\nstatic: false]
    B --> C{devServerYamlConfig.static\n!== undefined?}
    C -- No --> F[Keep static: false]
    C -- Yes --> D{static === false?}
    D -- Yes --> E[config.static = false]
    D -- No --> G{typeof static === 'object'\n&& static !== null?}
    G -- Yes --> H[config.static = static object]
    G -- No --> I[⚠️ Silently ignored\nconfig.static stays false\ne.g. static: true or string]
    E --> J[delete devServerYamlConfig.static]
    H --> J
    I --> J
    F --> K[Process remaining YAML keys\nsnake_case to camelCase]
    J --> K
    K --> L[Return WebpackDevServerConfig]
Loading

Reviews (1): Last reviewed commit: "docs: update changelog with PR number" | Re-trigger Greptile

Comment thread CHANGELOG.md Outdated
Comment thread package/webpackDevServerConfig.ts Outdated
Comment thread package/webpackDevServerConfig.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2645056893

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread package/webpackDevServerConfig.ts Outdated
Comment thread package/webpackDevServerConfig.ts Outdated
@claude

claude Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Review

The fix is well-motivated and the root-cause analysis is solid. Defaulting static to false, fixing the truthy-check regression, and cleaning up the stale template entry are all correct and safe changes. The new test suite is thorough for the cases it covers.

One correctness issue to address:

static: true in shakapacker.yml is now silently discarded — see the inline comment on webpackDevServerConfig.ts. webpack-dev-server accepts static: true as a valid option (enables its built-in static-serving defaults), so silently ignoring it is a behavioral regression. Adding an else passthrough branch (and a matching test) would close the gap.

Everything else looks good:

  • Removing outputPath import is correct since the default static object that used it is gone.
  • The !== undefined guard properly catches static: false where the old truthy check didn't.
  • Test isolation via jest.resetModules() + mutating exports before requiring the config module under test is consistent with the project's existing patterns (e.g., dev_server.test.js).
  • YAML template cleanup is appropriate — the static.watch.ignored block was indeed a v3→v4 migration artifact.

Comment thread test/package/webpackDevServerConfig.test.js
@claude

claude Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Review

The fix is well-motivated and the core change is correct: static: false is the right default for a Rails + Shakapacker setup where ActionDispatch::Static already serves public/ and DevServerProxy handles /packs/*. The truthy-check bug fix and template cleanup are also solid.

One real issue: static: true / string values are silently discarded

The new branch handles false and objects, but any other truthy value from YAML (webpack-dev-server accepts true, a string path, and arrays) falls through both inner conditionals, gets deleted, and silently leaves config.static = false. I've left an inline comment with a suggested fix (warn + keep false, or pass non-false primitives/arrays through).

A test covering that path would pin down the intended behavior — also noted inline.

Everything else looks good

  • Removing outputPath from the import is correct; it was never a valid publicPath value (filesystem path ≠ URL path).
  • The !== undefined guard properly handles the false passthrough that the old truthy check missed.
  • Test structure matches the existing dev_server.test.js pattern (chdirTestApp() at module scope + afterAll restore).
  • The jest.resetModules() + mutate-then-require pattern correctly exercises module-level require("./dev_server") in the production code.
  • CHANGELOG format and placement follow project conventions.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@package/webpackDevServerConfig.ts`:
- Around line 104-114: The file is missing a trailing newline and the
devServerYamlConfig.static handling only accounts for false and object types;
update the code to add a final newline and to set config.static when
devServerYamlConfig.static is a string or array (e.g., if typeof === "string"
assign the string, if Array.isArray assign the array cast to Array<string |
Record<string, unknown>>), keeping the existing false and object branches and
still deleting devServerYamlConfig.static afterwards; also update the
WebpackDevServerConfig.static type definition (the static property in the config
interface) to include string and Array<string | object> variants so the types
align with webpack-dev-server API.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5796f134-083b-442b-855a-214624d3508d

📥 Commits

Reviewing files that changed from the base of the PR and between 4cd5250 and 2645056.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • lib/install/config/shakapacker.yml
  • package/webpackDevServerConfig.ts
  • test/package/webpackDevServerConfig.test.js
💤 Files with no reviewable changes (1)
  • lib/install/config/shakapacker.yml

Comment thread package/webpackDevServerConfig.ts Outdated
@ihabadham
ihabadham force-pushed the ihabadham/fix/static-config-bugs branch from 2645056 to e7a1417 Compare March 30, 2026 16:24
Comment thread package/webpackDevServerConfig.ts Outdated
@claude

claude Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Review

This is a solid, focused fix for a real pain point. The root cause analysis is correct: the old static.publicPath was set to a filesystem path (outputPath) rather than a URL path, causing webpack-dev-server to default-watch public/, which is already owned by Rails' ActionDispatch::Static. Setting static: false is the right default for Shakapacker's proxy-based architecture.

What is good

  • The truthy-check fix (devServerYamlConfig.static!== undefined) correctly preserves an explicit static: false from YAML that was previously silently ignored.
  • Removing the stale static.watch.ignored template entry is clean housekeeping.
  • Tests are thorough and use the same module-mutation pattern as the rest of the test suite.
  • The outputPath/contentBase import is correctly removed along with its usage.

One issue to address

See the inline comment on lines 104–113 of webpackDevServerConfig.ts: static: true (and other valid webpack-dev-server values like strings/arrays) is silently dropped, leaving config.static at false. webpack-dev-server v4 accepts boolean | string | string[] | Static | Static[], so a user who explicitly opts back into static serving with static: true or static: '/custom' would find their setting quietly ignored. A comment explaining the intentional scope, or extending the guard to forward other truthy values, would prevent future confusion.

Minor nit

The test for "passes through static: false from YAML config" is technically redundant with the default test (the default is already false), though it is useful as explicit documentation of the YAML-passthrough contract. No action needed.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e7a1417458

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread lib/install/config/shakapacker.yml
@claude

claude Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Review

The fix is correct and well-motivated. The old default (static: { publicPath: contentBase }) was misconfigured: contentBase (outputPath) is a filesystem path not a URL path, and the missing directory field caused webpack-dev-server to fall back to watching the project root. Setting static: false is the right call given Shakapacker's proxy architecture.

Bug: unhandled truthy non-object static value

When static is set to true (or any other truthy scalar) in shakapacker.yml, neither branch in the new if / else if block matches and config.static is silently left as false. webpack-dev-server v4/v5 treats static: true as "use defaults" (serve from project root), so a user who had this value would get the opposite behavior with no warning. This is a behavior regression from the old code, which at least preserved its default when static was truthy. An else branch that warns and defaults to false, or explicitly passes true through, would address this.

Minor: one test does not cover the logic it implies

The test "passes through static: false from YAML config" sets devServer.static = false and asserts config.static === false, but since the default is already false the test would pass even if the YAML override branch was completely absent. A more effective version would verify that the static key is consumed from devServerYamlConfig (not leaked into the camelCase mapping loop), or check that static: false in YAML wins over a non-false starting state.

Everything else looks good

  • Removing outputPath / contentBase from the import is correct cleanup.
  • Changing the truthy check to !== undefined correctly handles static: false from YAML — that is the core bug fix.
  • Tests for the object passthrough, devMiddleware.publicPath, hmr to hot mapping, and snake_case conversion are solid.
  • Removing static.watch from the install template is correct.

Comment thread package/webpackDevServerConfig.ts Outdated
Comment thread test/package/webpackDevServerConfig.test.js
Comment thread package/webpackDevServerConfig.ts
Comment thread package/webpackDevServerConfig.ts
@claude

claude Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Review

The fix is well-motivated and the core logic is sound. The three bugs (wrong default, static: false silently ignored, stale template artifact) are all correctly addressed, and the new tests give good coverage of the key paths.

Two issues to address before merging:

1. TypeScript type doesn't cover true (or other passthrough values)

The WebpackDevServerConfig["static"] type is false | { ... }, but the else branch passes through any non-false, non-object value as-is — including true, which is tested. The as WebpackDevServerConfig["static"] cast in that branch silently lies to the type checker. Widening the type to boolean | { ... } (or to match the full webpack-dev-server signature) would make the types honest. See inline comment.

2. Off-by-one indentation in the else closing brace

A stray space gives the closing } 5-space indentation instead of 4. See inline comment.

Everything else looks good:

  • Removing outputPath/contentBase from the static default is the right call — it was a filesystem path being used where a URL was expected.
  • The !== undefined guard correctly catches static: false where the old truthy check didn't.
  • The delete devServerYamlConfig.static placement (outside the inner if-else but inside the outer guard) is correct.
  • Test setup with jest.resetModules() + mutating the required module before loading the config under test is the right pattern for this module structure.

@ihabadham
ihabadham force-pushed the ihabadham/fix/static-config-bugs branch from 3f975a9 to 2c4bedd Compare March 30, 2026 16:49

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2c4bedd784

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread package/webpackDevServerConfig.ts Outdated
@claude

claude Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Overall: solid fix for a real bug. The truthy-check correction and false default are correct. One type-safety issue to address: see inline comments.

Comment thread package/webpackDevServerConfig.ts
Comment thread package/webpackDevServerConfig.ts
Comment thread package/webpackDevServerConfig.ts
Comment thread test/package/webpackDevServerConfig.test.js
@claude

claude Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Review

The fix is correct and well-motivated. Defaulting static: false prevents webpack-dev-server from inotify-watching public/, and the truthy-check bug (if (devServerYamlConfig.static)) silently eating static: false from YAML is a real regression worth fixing. The test coverage for the new behaviour is good.

A few nits/issues to address:


1. Redundant static === false branch (minor)

config.static is initialised to false on the line above, so the === false arm just re-assigns the same value. The only meaningful side-effect is the delete devServerYamlConfig.static at the bottom of the outer block, which runs regardless of which branch is taken. The branch can be removed entirely without changing behaviour.


2. Unnecessary !== null guard (minor)

YAML static: false is deserialised to the JS boolean false, never to null. The only way to get null here is static: ~ / bare static: in the YAML, which isn't a meaningful webpack-dev-server value. The check !== undefined alone is sufficient.


3. Array passthrough has an incorrect TypeScript cast (worth fixing)

webpack-dev-server accepts static as an array (Array<string | Static>) — the type comment on DevServerConfig.static already documents this. If a user configures:

static:
  - "/path1"
  - "/path2"

…then typeof [] === "object" is true, so the object branch is taken and the value is cast to Record<string, unknown>. The runtime value flowing through is fine, but the TypeScript cast is wrong. An Array.isArray guard would fix the type fidelity:

config.static = Array.isArray(devServerYamlConfig.static)
  ? (devServerYamlConfig.static as string[])
  : (devServerYamlConfig.static as Record<string, unknown>)

4. Missing test for string and array passthrough (minor)

There is no test exercising static: "/path" (string) or static: ["/p1", "/p2"] (array) — both valid webpack-dev-server values handled by the else/object branches. Worth adding at least the string case.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0083f34f51

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread lib/install/config/shakapacker.yml
@justin808
justin808 force-pushed the ihabadham/fix/static-config-bugs branch from 6230078 to d809bbe Compare May 21, 2026 09:12

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
test/package/webpackDevServerConfig.test.js (1)

117-125: ⚡ Quick win

Add an explicit regression test for static: null (YAML bare key).

This behavior is part of the intended contract; a focused test will prevent regressions where null accidentally overrides defaults.

Suggested test case
   test("passes through static array from YAML config", () => {
     const devServer = require("../../package/dev_server")
     devServer.static = ["/path1", "/path2"]

     const createDevServerConfig = require("../../package/webpackDevServerConfig")
     const config = createDevServerConfig()

     expect(config.static).toStrictEqual(["/path1", "/path2"])
   })
+
+  test("treats static: null as unset and keeps default", () => {
+    const devServer = require("../../package/dev_server")
+    devServer.static = null
+
+    const createDevServerConfig = require("../../package/webpackDevServerConfig")
+    const config = createDevServerConfig()
+
+    expect(config.static).toBe(false)
+  })
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/package/webpackDevServerConfig.test.js` around lines 117 - 125, Add a
focused regression test in test/package/webpackDevServerConfig.test.js that
verifies a YAML bare key producing static: null does not override defaults:
require("../../package/dev_server"), set devServer.static = null, call
createDevServerConfig (require("../../package/webpackDevServerConfig")) and
assert the returned config.static equals the expected default (i.e., the same
value used when devServer.static is undefined) so null does not replace the
default static configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Line 65: Remove the duplicate "### Fixed" heading inside the v10.0.0 release
block: locate both occurrences of the "### Fixed" heading within that release
section and delete the second one, then move any bullet(s) under the removed
heading so all "Fixed" items are listed together under the single remaining "###
Fixed" heading.

---

Nitpick comments:
In `@test/package/webpackDevServerConfig.test.js`:
- Around line 117-125: Add a focused regression test in
test/package/webpackDevServerConfig.test.js that verifies a YAML bare key
producing static: null does not override defaults:
require("../../package/dev_server"), set devServer.static = null, call
createDevServerConfig (require("../../package/webpackDevServerConfig")) and
assert the returned config.static equals the expected default (i.e., the same
value used when devServer.static is undefined) so null does not replace the
default static configuration.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 37e2d069-8053-4988-bc28-fb99a8b84a0d

📥 Commits

Reviewing files that changed from the base of the PR and between 6230078 and d809bbe.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • lib/install/config/shakapacker.yml
  • package/webpackDevServerConfig.ts
  • test/package/webpackDevServerConfig.test.js
💤 Files with no reviewable changes (1)
  • lib/install/config/shakapacker.yml

Comment thread CHANGELOG.md Outdated
@justin808
justin808 force-pushed the ihabadham/fix/static-config-bugs branch from d809bbe to 7658135 Compare May 21, 2026 19:12
@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Code Review

This is a well-motivated fix for a real user-facing issue (#1031). The root cause analysis in the PR description is accurate, the YAML passthrough matrix is thorough, and having 11 new tests is great. A few things to address before merging:


Must-Fix

1. CHANGELOG entry is under [v10.0.0] (a released version) and creates a duplicate ### Fixed header

The entry was inserted just before ## [v9.7.0], which puts it at the end of the [v10.0.0] block. That block is already published. The fix belongs under [Unreleased] (or under [v10.1.0-rc.0] if this is being included there). Additionally, the insertion added a second ### Fixed section within [v10.0.0]; the existing section at the top of that block already had the compression-webpack-plugin entry — the new bullet should merge into that section rather than open a new one.


Code Quality

2. The static type-narrowing if-else is unnecessary (webpackDevServerConfig.ts:122–138)

All four branches do the same thing: assign the value to config.static. The Array/object/else branching exists only to satisfy TypeScript's casts, but since devServerYamlConfig.static is typed as unknown, a single cast to the full union type is sufficient and compiles cleanly:

if (devServerYamlConfig.static != null) {
  config.static = devServerYamlConfig.static as WebpackDevServerConfig["static"]
  delete devServerYamlConfig.static
}

(!= null catches both null and undefined, matching the existing two-condition guard.)

3. New tests use direct module mutation; existing tests use jest.doMock()

The two existing tests in the file use jest.isolateModules + jest.doMock() for module isolation. The 11 new tests instead require the live module and mutate its exports (e.g. devServer.static = false). This works today but is fragile — it relies on the mutation being visible to the module-level assignment in webpackDevServerConfig.ts because jest.resetModules() clears the cache before each test. The pattern is also inconsistent with the file's own style. Consider using jest.doMock() instead, or at minimum add a comment explaining the approach.

4. Missing test for static: null (YAML ~)

The PR description calls out the static: ~ case as being treated the same as unset, but there is no test for it. Add one:

test("treats static: null (YAML ~) as unset, falling back to false", () => {
  const devServer = require("../../package/dev_server")
  devServer.static = null

  const createDevServerConfig = require("../../package/webpackDevServerConfig")
  const config = createDevServerConfig()

  expect(config.static).toBe(false)
})

Positives

  • The core bug fix is correct: static: false as default + explicit !== undefined && !== null guard instead of the truthy check is exactly right.
  • Removing the static.watch v3→v4 migration artifact from the YAML template is a clean improvement.
  • The type definition for WebpackDevServerConfig["static"] now correctly reflects all valid webpack-dev-server values.
  • Removing the unused outputPath: contentBase destructuring is good hygiene.

Comment thread CHANGELOG.md
Comment on lines +122 to 139
if (
devServerYamlConfig.static !== undefined &&
devServerYamlConfig.static !== null
) {
if (devServerYamlConfig.static === false) {
config.static = false
} else if (Array.isArray(devServerYamlConfig.static)) {
config.static = devServerYamlConfig.static as Array<
string | Record<string, unknown>
>
} else if (typeof devServerYamlConfig.static === "object") {
config.static = devServerYamlConfig.static as Record<string, unknown>
} else {
config.static =
devServerYamlConfig.static as WebpackDevServerConfig["static"]
}
delete devServerYamlConfig.static
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All four branches here do the same thing — assign the value to config.static — with different casts for TypeScript. Since devServerYamlConfig.static is typed as unknown (see types.ts), a single cast to the full union type covers every case. The Array.isArray / typeof === "object" guards add runtime branching without changing the runtime result. Consider simplifying:

Suggested change
if (
devServerYamlConfig.static !== undefined &&
devServerYamlConfig.static !== null
) {
if (devServerYamlConfig.static === false) {
config.static = false
} else if (Array.isArray(devServerYamlConfig.static)) {
config.static = devServerYamlConfig.static as Array<
string | Record<string, unknown>
>
} else if (typeof devServerYamlConfig.static === "object") {
config.static = devServerYamlConfig.static as Record<string, unknown>
} else {
config.static =
devServerYamlConfig.static as WebpackDevServerConfig["static"]
}
delete devServerYamlConfig.static
}
if (devServerYamlConfig.static != null) {
config.static = devServerYamlConfig.static as WebpackDevServerConfig["static"]
delete devServerYamlConfig.static
}

!= null catches both null and undefined, which matches the intent of the existing two-condition guard.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Acknowledged — a single cast to WebpackDevServerConfig["static"] would compress fine. The current four-branch shape was reached deliberately over earlier review rounds (the "honest TypeScript cast" thread) to make the runtime cases explicit at the point of assignment rather than hide them behind one wide cast. Same overall behavior either way; leaving the explicit form for now unless @ihabadham wants to revisit.

Comment on lines +74 to +82
test("passes through static: false from YAML config", () => {
const devServer = require("../../package/dev_server")
devServer.static = false

const createDevServerConfig = require("../../package/webpackDevServerConfig")
const config = createDevServerConfig()

expect(config.static).toBe(false)
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two concerns with the testing approach used in the new tests:

Pattern inconsistency: The two existing tests in this file use jest.isolateModules + jest.doMock() for full module isolation. These new tests instead require the live module and mutate its exports directly (devServer.static = false). This works because jest.resetModules() in beforeEach clears the cache so webpackDevServerConfig picks up the mutation when it's required next, but it's fragile and inconsistent with the file's established style.

This specific test: The result (config.static === false) is identical to the default behavior, so the test would pass even if the explicit-false passthrough branch were removed. It still has documentation value and would catch a regression if the default ever changed, but it's weaker than it appears. A comment explaining what regression it guards against would help future readers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Considered both points:

  1. Pattern: jest.isolateModules + jest.doMock is needed when several modules must be mocked together (the middleware-hook tests mock both dev_server and config). For the static tests we mutate a single property on devServer and jest.resetModules() in beforeEach already gives a clean require cache between tests. Wrapping each one in isolateModules would add boilerplate without changing isolation.
  2. Coverage of the explicit-false test: It's intentionally a regression guard against the default ever flipping back to a truthy value. The test name documents the intent. Leaving as-is.

Comment thread test/package/webpackDevServerConfig.test.js
@justin808 justin808 self-assigned this May 21, 2026
ihabadham and others added 10 commits May 21, 2026 10:26
Documents expected behavior for static config: default to false,
pass through YAML static: false, and pass through static objects.
Some tests fail against current code — fixes follow in next commits.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three bugs fixed:

1. Default static changed from { publicPath: contentBase } to false.
   The old default set static.publicPath to a filesystem path (config.outputPath)
   instead of a URL path, and never set static.directory, causing
   webpack-dev-server to watch the entire public/ directory unnecessarily.
   In Rails apps, static file serving is handled by ActionDispatch::Static,
   not webpack-dev-server.

2. The truthy check 'if (devServerYamlConfig.static)' now uses
   'if (devServerYamlConfig.static !== undefined)' so that setting
   static: false in shakapacker.yml is no longer silently ignored.

3. When a user provides a static object in YAML, it is passed through
   directly instead of being merged with the (now-removed) buggy base.

Also removes static.watch from the default shakapacker.yml template,
since static is now false by default and the watch config was only
needed to work around the old buggy static default.

Fixes #1031

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… them

If a user sets static: true, a string, or any other value in
shakapacker.yml, pass it through to webpack-dev-server as-is
instead of silently discarding it.
- Type now accepts boolean | string | string[] | object (matching
  webpack-dev-server's actual schema) instead of just false | object
- Bare `static:` in YAML (parses as null via js-yaml) is now
  treated the same as undefined — skipped, default false sticks
…pt cast

Previously, an array `static: ["/p1", "/p2"]` in shakapacker.yml entered
the `typeof === "object"` branch and was cast to `Record<string, unknown>`,
which is a type lie. Add an explicit `Array.isArray` branch and widen the
type union accordingly. Behavior unchanged at runtime.
Addresses PR review:
- CHANGELOG entry was incorrectly placed under released [v10.0.0] with a
  duplicate ### Fixed heading. Moved it to [Unreleased].
- Added explicit test that static: null (YAML bare `static:` / `static: ~`)
  falls back to the default false, matching the behavior promised in the PR
  description.
@justin808
justin808 force-pushed the ihabadham/fix/static-config-bugs branch from b23d5bc to 22e7150 Compare May 21, 2026 20:27
@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Code Review

Overall: This is a well-motivated fix for a real bug. The root cause analysis in the PR description is accurate — the old default was a misconfigured object that caused webpack-dev-server to watch public/ unnecessarily, and the truthy-check bug meant static: false was silently ignored. The fix is correct and the test coverage is solid.

A few things to address:


1. Dispatch chain in webpackDevServerConfig.ts is over-engineered (lines 126–137)

Every branch of the if/else if/else block does the same thing at runtime: assign devServerYamlConfig.static to config.static. TypeScript type casts (as) are erased at compile time — they have no runtime effect. So the entire chain is equivalent to a single assignment:

config.static = devServerYamlConfig.static as WebpackDevServerConfig["static"]
delete devServerYamlConfig.static

The original motivation (from the commit message) was to give an "honest TypeScript cast" for arrays vs. objects — but since WebpackDevServerConfig["static"] already is a union of boolean | string | Array<...> | object, there's no cast dishonesty to avoid. The only functionally distinct case is === false needing to run the block at all, which is already handled by the outer guard (!== undefined && !== null).


2. Inline test comment violates project style

test/package/webpackDevServerConfig.test.js line 156 has:

// Test app has hmr: true, so liveReload should default to false

Per the project's coding-style rules, comments should only explain a non-obvious WHY — not narrate what the test expects. This also leaks a fragile coupling to the test app's YAML config: if someone changes hmr in the test app, they'd have to hunt down this comment to understand why the assertion started failing. Consider removing the comment or expressing the invariant in the test logic itself (e.g., by setting hmr explicitly rather than relying on the test app's value).


3. Test isolation pattern inconsistency (minor)

The two existing tests at the top of the file use jest.isolateModules() + jest.doMock() for full isolation. The 11 new tests use a different pattern: require dev_server directly and mutate its export. This works correctly (because jest.resetModules() in beforeEach ensures a fresh module load per test), but the inconsistency within one file is noticeable. It also means the new tests silently depend on the test app's real dev_server YAML for non-static fields (e.g., hmr: true for the liveReload test). Not a bug, but worth being aware of.


4. DevServerConfig.static type remains unknown (minor)

package/types.ts line 91 still has static?: unknown. Now that the code handles the full shape (boolean | string | string[] | object), this is a good opportunity to tighten the type to match WebpackDevServerConfig["static"]. Not blocking, but would eliminate the runtime-cast reliance.


Summary: Issue 1 is the main code-quality concern. Issues 2–4 are minor polish items. The actual bug fix logic and the test coverage are both correct.

Comment on lines +126 to 138
if (devServerYamlConfig.static === false) {
config.static = false
} else if (Array.isArray(devServerYamlConfig.static)) {
config.static = devServerYamlConfig.static as Array<
string | Record<string, unknown>
>
} else if (typeof devServerYamlConfig.static === "object") {
config.static = devServerYamlConfig.static as Record<string, unknown>
} else {
config.static =
devServerYamlConfig.static as WebpackDevServerConfig["static"]
}
delete devServerYamlConfig.static

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All four branches of this dispatch do the same thing at runtime: assign devServerYamlConfig.static to config.static. TypeScript as casts are compile-time only — they have no effect at runtime, so the Array.isArray / typeof === "object" / else split is pure ceremony.

Since WebpackDevServerConfig["static"] is already a union that covers boolean | string | Array<...> | object, a single cast is both accurate and simpler:

Suggested change
if (devServerYamlConfig.static === false) {
config.static = false
} else if (Array.isArray(devServerYamlConfig.static)) {
config.static = devServerYamlConfig.static as Array<
string | Record<string, unknown>
>
} else if (typeof devServerYamlConfig.static === "object") {
config.static = devServerYamlConfig.static as Record<string, unknown>
} else {
config.static =
devServerYamlConfig.static as WebpackDevServerConfig["static"]
}
delete devServerYamlConfig.static
config.static = devServerYamlConfig.static as WebpackDevServerConfig["static"]
delete devServerYamlConfig.static

const createDevServerConfig = require("../../package/webpackDevServerConfig")
const config = createDevServerConfig()

// Test app has hmr: true, so liveReload should default to false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment narrates what the test expects rather than explaining a non-obvious constraint — the project style guide asks us to omit such comments. It also creates a fragile implicit dependency on the test app's YAML (hmr: true): a future change to that config would break this assertion with no obvious connection between the symptom and the cause.

Consider making the dependency explicit by setting hmr in the test itself (similarly to how other tests mutate devServer.static):

Suggested change
// Test app has hmr: true, so liveReload should default to false
test("defaults liveReload to inverse of hmr", () => {
const devServer = require("../../package/dev_server")
devServer.hmr = true
const createDevServerConfig = require("../../package/webpackDevServerConfig")
const config = createDevServerConfig()
expect(config.liveReload).toBe(false)
})

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22e71501f4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 165 to 167
# port than your Rails server and the browser blocks cross-origin asset requests):
# headers:
# "Access-Control-Allow-Origin": "*"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep static key in generated dev_server config

Removing dev_server.static from the install template makes SHAKAPACKER_DEV_SERVER_STATIC=... impossible to apply in newly generated apps, because package/dev_server.ts only reads env overrides for keys already present in YAML (Object.keys(devServerConfig)). In this same change, webpackDevServerConfig now correctly passes through boolean/string/array/object static values, so a template default of static: false would preserve the new default while still allowing environment-only toggles; with the key deleted, operators cannot re-enable static serving via env without editing YAML.

Useful? React with 👍 / 👎.

@justin808
justin808 merged commit ed6c5a7 into main May 21, 2026
62 checks passed
@justin808
justin808 deleted the ihabadham/fix/static-config-bugs branch May 21, 2026 22:12
justin808 added a commit that referenced this pull request May 21, 2026
## Summary

Stamps the `v10.1.0-rc.1` header and collapses `v10.1.0-rc.0` per the
project's RC collapse convention.

- Renamed `## [v10.1.0-rc.0] - May 20, 2026` → `## [v10.1.0-rc.1] - May
21, 2026`
- Moved the post-rc.0 `Unreleased` Fixed entry for PR #1032
(webpack-dev-server `static` config) into the merged `### Fixed` section
- Dropped the PR #1120 entry (`shakapacker:check_node` regression fix) —
it only fixes a bug introduced by PR #1110 that shipped solely in
`v10.1.0-rc.0`, so users going from `v10.0.0` → `v10.1.0-rc.1` never see
it
- Updated `[Unreleased]` and `[v10.1.0-rc.1]` diff links

## Test plan

- [ ] CI passes
- [ ] Maintainer confirms the rc.0 → rc.1 collapse looks right

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Low risk documentation-only change that updates release notes and
comparison links without affecting runtime code.
> 
> **Overview**
> **Updates `CHANGELOG.md` for the `v10.1.0-rc.1` release.**
> 
> Renames the `v10.1.0-rc.0` section to `v10.1.0-rc.1` (with the new
date), moves the webpack-dev-server `static` fix (PR #1032) from
*Unreleased* into the release’s *Fixed* section, removes the rc.0-only
`shakapacker:check_node` regression note (PR #1120), and updates the
bottom compare links to point at `v10.1.0-rc.1`.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
eaf83d5. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

webpack-dev-server watches public/ unnecessarily due to static config bugs

2 participants