[change] Rebuild webpack when there is change in organizations dir #425#1047
[change] Rebuild webpack when there is change in organizations dir #425#1047PRoIISHAAN wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughAdds watching and cleanup for organization configurations. setup.writeConfigurations now resets in-memory accumulators, preserves previous organization slugs from organizations.json, rebuilds configs, computes current slugs, and removes assets directories for slugs that no longer exist. webpack.config.js adds a recursive getAllFiles helper and an OrganizationsWatchPlugin that registers organization files as compilation dependencies and, in development watch runs, invokes setup.writeConfigurations() when organizations/ changes are detected. Sequence DiagramsequenceDiagram
participant Dev as Developer
participant FS as File System
participant Webpack as Webpack Watcher
participant Plugin as OrganizationsWatchPlugin
participant Setup as setup.writeConfigurations
participant Acc as Config Accumulators
Dev->>FS: Modify file in organizations/
FS->>Webpack: Notify watched change
Webpack->>Plugin: watchRun / afterCompile hooks
Plugin->>FS: getAllFiles(organizations/)
Plugin->>Webpack: add files as fileDependency/contextDependency
Plugin->>Setup: call writeConfigurations()
Setup->>Acc: reset clientConfigs, serverConfigs, organizations
Setup->>FS: read organization files and rebuild configs
Setup->>FS: compute current slugs
Setup->>FS: remove assets for slugs removed since previous run
Setup-->>Plugin: finished
Plugin->>Webpack: report completion or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
PRoIISHAAN
force-pushed
the
Rebuild-webpack
branch
from
aad815a to
9f0027c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/setup.js`:
- Around line 181-185: The reset of clientConfigs, serverConfigs and
organizations only clears in-memory state but does not remove stale asset
directories, so implement pruning of removed organization slugs: after
rebuilding organizations array (the same place where
clientConfigs.length/serverConfigs.length/organizations.length are reset),
compute the current set of slugs (e.g., organizations.map(o => o.slug)), then
list directories under both "client/assets" and "server/assets" and remove any
directory whose name is not in that slug set (use fs.readdir + fs.stat to detect
directories and fs.rm or fs.rmSync with recursive:true to delete); ensure you
catch and log errors and skip non-directory entries so regeneration safely
prunes stale asset folders.
In `@config/webpack.config.js`:
- Around line 21-30: The recursive file walker getAllFiles should be hardened
against transient ENOENT between readdirSync and statSync: wrap the
fs.statSync(fullPath) call in a try-catch inside getAllFiles and, on catch, if
err.code === 'ENOENT' simply skip that entry (do not rethrow) so watch mode
doesn't crash; for other errors rethrow or log as appropriate to avoid
swallowing real failures.
- Around line 68-75: The try/catch around setup.writeConfigurations in
OrganizationsWatchPlugin currently logs errors but always calls callback(),
allowing webpack to continue; change the catch to pass the error to the webpack
watcher by invoking callback(err) so failed setup aborts the rebuild.
Specifically, update the error handling for setup.writeConfigurations in the
watchRun hook to call callback(err) (and return) instead of callback(), ensuring
the plugin surfaces setup failures to webpack.
- Around line 60-64: The check only inspects comp.modifiedFiles so deletions
under organizationsDir (exposed via compiler.removedFiles in watch mode) are
missed; update the logic around changedFiles/anyOrgFileChanged to include
removed files as well (merge comp.removedFiles with comp.modifiedFiles into the
changedFiles set) and use that combined set when computing anyOrgFileChanged so
deletes trigger the same rerun logic; refer to changedFiles, comp.modifiedFiles,
comp.removedFiles, organizationsDir, and anyOrgFileChanged to locate and update
the code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
config/setup.jsconfig/webpack.config.js
📜 Review details
⏰ 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). (1)
- GitHub Check: Tests and Coverage
🧰 Additional context used
🧬 Code graph analysis (1)
config/setup.js (2)
client/reducers/organization.js (2)
organizations(10-17)organizations(10-17)internals/generators/utils/organizationExists.js (1)
organizations(4-6)
🔇 Additional comments (1)
config/webpack.config.js (1)
117-119: Good scope: plugin is development-only.Limiting
OrganizationsWatchPluginto development avoids unnecessary production overhead while solving local watch ergonomics.
There was a problem hiding this comment.
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 `@config/webpack.config.js`:
- Around line 88-89: The code calls callback() twice when anyOrgFileChanged is
false; locate the conditional handling that checks anyOrgFileChanged and remove
the duplicate invocation so callback() is called exactly once (ensure you call
callback() then immediately return, using the existing return callback() pattern
or a single callback() followed by return to avoid double-invocation); check the
surrounding function where anyOrgFileChanged and callback are used to confirm no
other code path still triggers a second callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a529be2-abec-4668-af8f-db9c6a072e24
📒 Files selected for processing (2)
config/setup.jsconfig/webpack.config.js
📜 Review details
⏰ 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). (1)
- GitHub Check: Tests and Coverage
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), catch blocks deliberately do NOT check `isComponentMounted` — console warnings from catch paths are considered harmless. Only real memory leaks (timers, event listeners) and "setState on unmounted" in success paths are fixed. The maintainer (nemesifier) explicitly wants to avoid over-engineering with defensive checks in every catch block.
Applied to files:
config/webpack.config.js
🔇 Additional comments (6)
config/setup.js (2)
181-190: LGTM! Idempotency fix correctly implemented.The code now properly:
- Captures previous slugs from
organizations.jsonbefore clearing state- Resets in-memory arrays to prevent duplicate accumulation on re-runs
This addresses the previous review concern about idempotency.
242-254: LGTM! Stale asset cleanup correctly implemented.The cleanup logic properly identifies removed organization slugs by comparing previous and current states, then removes their asset directories. Using
force: trueensures graceful handling if directories don't exist.config/webpack.config.js (4)
21-36: LGTM! ENOENT race condition properly handled.The
try-catcharoundfs.statSynccorrectly handles the TOCTOU race where files may be deleted betweenreaddirSyncandstatSync. Non-ENOENT errors are properly re-thrown.
67-74: LGTM! File deletion detection now included.The code correctly checks both
modifiedFilesandremovedFiles, addressing the previous concern about stale configs when organization files are deleted. Thepath.relativelogic accurately determines whether changed files are within the organizations directory.
79-86: LGTM! Setup errors now properly propagate to webpack.The error handling correctly passes errors to the callback via
callback(err), ensuring webpack fails the rebuild when setup encounters problems rather than continuing with stale configuration.
129-133: LGTM!The plugin is correctly registered only in development mode, which aligns with the PR objective of improving the development workflow without affecting production builds.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
CI Failure: Browser Test TimeoutHello @PRoIISHAAN, The CI pipeline failed because the browser tests timed out. Specifically, the script Explanation: The Remediation:
Since the failure is a timeout during a health check, and no specific code error is indicated, the root cause is likely environmental or related to the application's startup sequence rather than a direct bug in the tested code itself. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config/webpack.config.js (1)
22-35:⚠️ Potential issue | 🟡 MinorGuard
readdirSyncagainst the remaining ENOENT race.Line 23 can pass, then the directory can disappear before Line 24, causing watch mode to crash despite the
statSyncguard. WrapreaddirSynctoo and treatENOENTas an empty directory.Proposed fix
const getAllFiles = (dirPath, results = []) => { - if (!fs.existsSync(dirPath)) return results; - fs.readdirSync(dirPath).forEach((entry) => { + let entries; + try { + entries = fs.readdirSync(dirPath); + } catch (err) { + if (err.code === "ENOENT") return results; + throw err; + } + entries.forEach((entry) => { const fullPath = path.join(dirPath, entry); try { if (fs.statSync(fullPath).isDirectory()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack.config.js` around lines 22 - 35, The helper getAllFiles currently calls fs.readdirSync(dirPath) without guarding for a race where the directory is removed after existsSync passes; wrap the readdirSync call in a try/catch and treat an ENOENT from readdirSync as an empty directory (i.e., return results) while re-throwing other errors, keeping the existing catch that handles fs.statSync errors for individual entries unchanged; ensure to reference getAllFiles and the calls to fs.readdirSync and fs.statSync when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/webpack.config.js`:
- Around line 22-35: The helper getAllFiles currently calls
fs.readdirSync(dirPath) without guarding for a race where the directory is
removed after existsSync passes; wrap the readdirSync call in a try/catch and
treat an ENOENT from readdirSync as an empty directory (i.e., return results)
while re-throwing other errors, keeping the existing catch that handles
fs.statSync errors for individual entries unchanged; ensure to reference
getAllFiles and the calls to fs.readdirSync and fs.statSync when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2e69590-c360-4adc-9b8f-4abae4ce2f8a
📒 Files selected for processing (1)
config/webpack.config.js
📜 Review details
⏰ 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). (2)
- GitHub Check: QA-Checks
- GitHub Check: Tests and Coverage
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 1072
File: config/__tests__/es5-compliance.test.js:22-29
Timestamp: 2026-03-23T21:28:04.132Z
Learning: In openwisp/openwisp-wifi-login-pages, `config/setup.js` and `config/build-translations.js` are project-level scripts that must be executed with `cwd` set to the project root (CURRENT_WORKING_DIR). They read from and write to project-relative directories (organizations/, public/, client/locales/, etc.) and will fail if run from any other directory. In the ES5 compliance test (`config/__tests__/es5-compliance.test.js`), the `beforeAll()` call to these scripts via `execSync` with `cwd: CURRENT_WORKING_DIR` is intentional and correct. There is no race risk because this is the only test file that calls setup. `yarn setup` is also already run before tests in CI, making the execSync just a local safety net.
📚 Learning: 2026-03-23T21:28:04.132Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 1072
File: config/__tests__/es5-compliance.test.js:22-29
Timestamp: 2026-03-23T21:28:04.132Z
Learning: In openwisp/openwisp-wifi-login-pages, `config/setup.js` and `config/build-translations.js` are project-level scripts that must be executed with `cwd` set to the project root (CURRENT_WORKING_DIR). They read from and write to project-relative directories (organizations/, public/, client/locales/, etc.) and will fail if run from any other directory. In the ES5 compliance test (`config/__tests__/es5-compliance.test.js`), the `beforeAll()` call to these scripts via `execSync` with `cwd: CURRENT_WORKING_DIR` is intentional and correct. There is no race risk because this is the only test file that calls setup. `yarn setup` is also already run before tests in CI, making the execSync just a local safety net.
Applied to files:
config/webpack.config.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
config/webpack.config.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), catch blocks deliberately do NOT check `isComponentMounted` — console warnings from catch paths are considered harmless. Only real memory leaks (timers, event listeners) and "setState on unmounted" in success paths are fixed. The maintainer (nemesifier) explicitly wants to avoid over-engineering with defensive checks in every catch block.
Applied to files:
config/webpack.config.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Applied to files:
config/webpack.config.js
Test Failures in Browser TestsHello @PRoIISHAAN and @nemesifier,
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Sorry for my late response, I did some testing and although it works initially, it's not consistent nor reliable.
I tried changing several parameters of the default.yml org like the text in components.header.links, the main org name or footer text, it did work for the first change but then it stopped.
3 participants
Checklist
Reference to Existing Issue
Closes #425 .
Description of Changes
Add OrganizationsWatchPlugin to webpack.config.js that watches all files under organizations/ during development. When any file there changes, setup.writeConfigurations() is re-run automatically before the next rebuild, so developers always edit the source files instead of the copied counterparts.
Also fix writeConfigurations() in setup.js to clear the module-level arrays at the start of each call, preventing duplicate entries when the function is invoked multiple times in a single session.