fix(app): full reload risky ui hmr edits#593
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Vite plugin and helper module that detect UI-source changes with high importer fanout and force a full page reload (via dev server WS) instead of incremental HMR to avoid context-provider breakage during large UI edits. ChangesUI HMR Guard for High-Fanout Modules
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Vite HMR
participant ui-hmr-guard Plugin
participant Detection Function
participant Dev Server WS
participant Browser
Developer->>Vite HMR: Edit packages/ui/src/components/icon.tsx (bulk change)
Vite HMR->>ui-hmr-guard Plugin: handleHotUpdate({file, modules})
ui-hmr-guard Plugin->>Detection Function: shouldForceFullReloadForUiHmr({file, modules})
Detection Function-->>ui-hmr-guard Plugin: true (high importer fanout)
ui-hmr-guard Plugin->>Dev Server WS: send({type: "full-reload", path: "*"})
ui-hmr-guard Plugin-->>Vite HMR: return []
Dev Server WS->>Browser: full-reload message
Browser->>Browser: Full page reload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app/vite.hmr.test.ts (1)
27-50: ⚡ Quick winAdd an explicit threshold boundary test (
== 30).You test below and above behavior, but not the exact boundary. Adding one assertion at the threshold will lock in intended
>=semantics and guard against off-by-one regressions.Proposed test addition
+ test("forces full reload at the exact threshold boundary", () => { + const leaf = module() + leaf.importers = new Set(Array.from({ length: 30 }, () => module())) + + expect( + shouldForceFullReloadForUiHmr({ + file: "/repo/packages/ui/src/components/icon.tsx", + modules: [leaf], + }), + ).toBe(true) + })🤖 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/app/vite.hmr.test.ts` around lines 27 - 50, Add a boundary test for shouldForceFullReloadForUiHmr to cover the exact threshold (30): create a test named like "forces a full reload at high-fanout boundary (== 30)" that builds a leaf module, sets leaf.importers = new Set(Array.from({ length: 30 }, () => module())), and expects shouldForceFullReloadForUiHmr({ file: "/repo/packages/ui/src/components/icon.tsx", modules: [leaf] }) toBe(true) to lock in >=30 semantics and prevent off-by-one regressions.
🤖 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/app/vite.hmr.test.ts`:
- Around line 27-50: Add a boundary test for shouldForceFullReloadForUiHmr to
cover the exact threshold (30): create a test named like "forces a full reload
at high-fanout boundary (== 30)" that builds a leaf module, sets leaf.importers
= new Set(Array.from({ length: 30 }, () => module())), and expects
shouldForceFullReloadForUiHmr({ file:
"/repo/packages/ui/src/components/icon.tsx", modules: [leaf] }) toBe(true) to
lock in >=30 semantics and prevent off-by-one regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 090046c0-994c-4157-944e-687e87556ef1
📒 Files selected for processing (3)
packages/app/vite.hmr.jspackages/app/vite.hmr.test.tspackages/app/vite.js
There was a problem hiding this comment.
Code Review
This pull request introduces a Vite HMR guard to manage high-fanout UI module edits by triggering a full reload when the number of unique importers exceeds a specific threshold. This implementation includes a new utility for counting transitive importers and a Vite plugin to intercept hot updates, serving as a workaround for upstream Solid/Vite HMR issues. Comprehensive unit tests for the new logic have also been added. I have no feedback to provide.
Summary
Add a narrow Vite dev-only guard in
packages/appso high-fanout hot updates frompackages/ui/src/*trigger a full reload instead of letting Solid HMR keep a broken provider tree alive:Why
The root cause is not in PawWork's providers themselves. The failure mode is an upstream Solid/Vite HMR edge case: after a large hot update to a high-fanout workspace UI module, the app can keep rendering with a stale provider graph and throw errors such as
Layout context must be used within a context provider.This PR does not fix that upstream HMR bug. It adds a PawWork-side workaround: when a
packages/ui/src/*edit fans out broadly enough to hit that risky path, desktop dev stops trying to apply HMR and falls back to a full reload instead.The linked upstream issues are related evidence, not a claim that either issue is this exact repro:
solidjs/vite-plugin-solid#80shows a real provider/context breakage across a library boundary in devsolidjs/vite-plugin-solid#106shows another case where Solid/Vite HMR corruption was avoided by falling back to a full reloadRelated Issue
Closes #189.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
packages/app/vite.js: the new dev-only plugin forces a full reload instead of HMR for risky workspace UI updatespackages/app/vite.hmr.js: the guard stays narrow by checking both source path and importer fanout before switching away from HMRpackages/app/vite.hmr.test.ts: the workaround logic is covered without changing production code paths or app runtime behaviorRisk Notes
This is a workaround, not a root fix:
packages/ui/src/*will now do a full reload instead of preserving in-place HMR statepackages/appstarts aroundicon(39 imports),button(34), andtoast(31); if false positives show up in day-to-day dev, this threshold should be retuned empiricallyHow To Verify
Screenshots or Recordings
None. Dev-only HMR workaround.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Tests
Chores