fix(core): preserve lazy compilation HMR state#8091
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds cause-aware lazy-compilation HMR tracking to the server and client. Build invalidation causes are propagated into websocket messages, while the client maintains reducer-backed hash state and applies lazy-specific HMR options. Server and reducer tests cover provenance, reconnects, duplicate hashes, resets, and fail-closed reloads. End-to-end coverage verifies document-load counts, initial chunks, active lazy-entry updates, websocket reconnects, and stale-hash reloads. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4c1e93d51
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/core/src/server/socketServer.ts (1)
264-277: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDelay clearing lazy-compilation state until after
sendStats()
sendStats()reads bothlazyCompilationInvalidationTokensandcurrentHashLazyCompilation; clearing them before the socket loop makes the current build look like a normal rebuild, so the client misses thelazy-compilation-hashsignal. Move those clears to after the loop, while keeping the no-sockets path cleared.🤖 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 `@packages/core/src/server/socketServer.ts` around lines 264 - 277, In onBuildDone(), defer clearing lazyCompilationInvalidationTokens and currentHashLazyCompilation until after the sendStats() loop so the current build retains its lazy-compilation state; preserve clearing for the no-sockets path before returning.
🤖 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 `@packages/core/src/server/assets-middleware/index.ts`:
- Around line 119-133: Isolate the Rspack-specific lazy-compilation detection in
a compatibility helper, including access to the
Symbol.for('rspack.lazyCompilationCurrent') flag and modifiedFiles/removedFiles;
keep the helper version-locked to the supported `@rspack/core` range and have the
thisCompilation hook use that helper instead of accessing internals directly.
---
Outside diff comments:
In `@packages/core/src/server/socketServer.ts`:
- Around line 264-277: In onBuildDone(), defer clearing
lazyCompilationInvalidationTokens and currentHashLazyCompilation until after the
sendStats() loop so the current build retains its lazy-compilation state;
preserve clearing for the no-sockets path before returning.
🪄 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: 5008a20d-0a60-4db6-8cd6-6fd40af177de
📒 Files selected for processing (5)
e2e/cases/lazy-compilation/dynamic-import/index.test.tspackages/core/rslib.config.tspackages/core/src/client/hmr.tspackages/core/src/server/assets-middleware/index.tspackages/core/src/server/socketServer.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/tests/server.test.ts (1)
528-529: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicate
sendStatsdeclaration in the type cast.Line 529 is an exact duplicate of line 528 — the
sendStatsmethod signature appears twice in theinternalstype assertion. This is a copy-paste artifact with no functional effect but should be cleaned up.♻️ Proposed fix
const internals = socketServer as unknown as { lazyCompilationInvalidationTokens: Set<string>; socketsMap: Map<string, Set<never>>; sendStats(options: { token: string }): void; - sendStats(options: { token: string }): void; };🤖 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 `@packages/core/tests/server.test.ts` around lines 528 - 529, Remove the duplicate sendStats declaration from the internals type assertion in the server test, keeping a single sendStats(options: { token: string }): void signature alongside socketsMap.
🤖 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.
Nitpick comments:
In `@packages/core/tests/server.test.ts`:
- Around line 528-529: Remove the duplicate sendStats declaration from the
internals type assertion in the server test, keeping a single sendStats(options:
{ token: string }): void signature alongside socketsMap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88400665-0780-4103-aa3f-e36cc8eb31d7
📒 Files selected for processing (2)
packages/core/src/server/socketServer.tspackages/core/tests/server.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/server/socketServer.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/client/hmr.ts (1)
411-428: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between
hashandlazy-compilation-hashcases.The two branches differ only in the boolean passed to
rememberHash; could be merged into a single fallthrough case.♻️ Optional consolidation
case 'hash': - rememberHash(message.data, false); - - if (clearOverlay && shouldUpdate()) { - clearOverlay(); - } - break; case 'lazy-compilation-hash': - rememberHash(message.data, true); + rememberHash(message.data, message.type === 'lazy-compilation-hash'); if (clearOverlay && shouldUpdate()) { clearOverlay(); } break;🤖 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 `@packages/core/src/client/hmr.ts` around lines 411 - 428, Merge the `hash` and `lazy-compilation-hash` branches in `onMessage` into a shared case, preserving their distinct `rememberHash` boolean values while consolidating the duplicated `clearOverlay` and `shouldUpdate` logic. Keep both message types’ existing behavior unchanged.
🤖 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.
Nitpick comments:
In `@packages/core/src/client/hmr.ts`:
- Around line 411-428: Merge the `hash` and `lazy-compilation-hash` branches in
`onMessage` into a shared case, preserving their distinct `rememberHash` boolean
values while consolidating the duplicated `clearOverlay` and `shouldUpdate`
logic. Keep both message types’ existing behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71feeb31-7c0b-4205-a9e2-50e9ae588ec3
📒 Files selected for processing (2)
packages/core/src/client/hmr.tspackages/core/src/server/assets-middleware/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/server/assets-middleware/index.ts
Deploying rsbuild with
|
| Latest commit: |
e6040a9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2a8cc807.rsbuild-v2.pages.dev |
| Branch Preview URL: | https://codex-lazy-compilation-hmr-p.rsbuild-v2.pages.dev |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/tests/server.test.ts (1)
518-605: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage of the retention/override contract; consider one more case.
The three tests thoroughly verify same-hash retention and normal-overrides-lazy behavior, but none vary
stats.hashacrossonBuildDone()calls. Add a case where a build produces a new hash with no pending cause set, to explicitly assert that a prior'lazy'record doesn't leak forward onto an unrelated hash (the implementation already handles this correctly via thepreviousRecord?.hash === hashguard, but it isn't directly asserted).🤖 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 `@packages/core/tests/server.test.ts` around lines 518 - 605, Add a test alongside the existing provenance tests using createSocketServerHarness that records a lazy build, changes stats.hash to a new value, then calls onBuildDone without setting another invalidation cause. Assert connect() emits a normal hash message for the new hash, confirming the prior lazy provenance does not carry across hashes.
🤖 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.
Nitpick comments:
In `@packages/core/tests/server.test.ts`:
- Around line 518-605: Add a test alongside the existing provenance tests using
createSocketServerHarness that records a lazy build, changes stats.hash to a new
value, then calls onBuildDone without setting another invalidation cause. Assert
connect() emits a normal hash message for the new hash, confirming the prior
lazy provenance does not carry across hashes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89b0de67-dfb9-4b65-892a-b670f8d1e2c9
📒 Files selected for processing (14)
e2e/cases/lazy-compilation/add-initial-chunk/index.test.tse2e/cases/lazy-compilation/add-initial-chunk/rsbuild.config.tse2e/cases/lazy-compilation/hmr-lifecycle/index.test.tse2e/cases/lazy-compilation/hmr-lifecycle/rsbuild.config.tse2e/cases/lazy-compilation/hmr-lifecycle/src/page1/App.csse2e/cases/lazy-compilation/hmr-lifecycle/src/page1/App.jsxe2e/cases/lazy-compilation/hmr-lifecycle/src/page1/index.jsxe2e/cases/lazy-compilation/hmr-lifecycle/src/page2/index.jspackages/core/src/client/hmr.tspackages/core/src/client/hmrHashState.tspackages/core/src/server/assets-middleware/index.tspackages/core/src/server/socketServer.tspackages/core/tests/hmrHashState.test.tspackages/core/tests/server.test.ts
|
Added the suggested cross-hash provenance regression in 9f23a69. It records a lazy build at |
|
Paired validation found one necessary boundary: a proven lazy cycle may stay in-document only while the initial chunk set is stable. A newly introduced static initial module has no installed factory, so the existing safety reload must remain. Commit 49ec591 restores that guard, adds a focused server regression, and keeps dynamic-import/lifecycle HMR same-document. Focused server tests and exact lazy E2Es pass. |
1 participant
Summary
Preserves completed-build provenance per environment so Rsbuild distinguishes proven lazy activations from file-backed or ambiguous invalidations, including delayed clients, duplicate stats, reconnects, and errors. Normal invalidations dominate mixed cycles and ambiguous state fails closed to a document reload.
The client uses a pure immutable hash-state reducer and applies proven lazy updates with
ignoreUnacceptedplus Rspack'spreserveDisposedModuleFactoriesoption. Initial-chunk-set changes retain the existing safety reload because newly introduced static module factories cannot be installed by a hot update alone. The compatibility helper remains guarded byTODO(rspack#14753)until the minimum Rspack version contains the upstream API.Related links
Tests