Conversation
`preloadEntries(modules)` walks every `lazy:` entry on a module set and primes the resolver's per-entry cache. After it resolves, the existing synchronous-thenable fast path in `resolveEntryComponent` (proven by `packages/react/src/resolve-entry.test.tsx:123-150`) takes over — ModuleTab/JourneyOutlet commit the resolved component on the first render with no Suspense fallback flash, so tests no longer need `waitFor` / `act` choreography. - New `preloadEntries(modules)` in `@modular-react/testing` - Re-export `preloadEntry` from testing for a single import surface - Unit tests cover dedupe, idempotence, eager skipping, rejection propagation, and the post-preload synchronous-thenable behavior - Integration test in `module-tab.test.tsx` proves zero fallback mounts and synchronous render after `preloadEntries` - README sections in `@modular-react/testing` and a pointer from the `@modular-react/react` README https://claude.ai/code/session_0166HdHFxs9MBEWdxybXhSkM
These three packages declared `private: false` and shipped publish-ready manifests (`main`, `files`, `prepublishOnly`), but the publish workflow filtered them out. Add path filters and PKG_MAP entries so a labelled PR that touches them triggers a release. Catalog's filter includes both `src/**` and `spa-src/**` because the package's published `files` are `dist` + `dist-spa` and the SPA bundle is built from `spa-src`. https://claude.ai/code/session_0166HdHFxs9MBEWdxybXhSkM
Self-review pass on the previous commit.
- Drop the `as { lazy?: unknown }` cast and the redundant
`as ModuleEntryPoint<any>[]` in favor of a direct `"lazy" in entry`
narrow on the EagerModuleEntryPoint | LazyModuleEntryPoint union.
- Rename the loop variable from `module` to `mod` to avoid shadowing.
- Make README/JSDoc imports consistent: both examples now use
`@modular-react/testing` (the unified import surface) and the second
example notes explicitly that `preloadEntry` is re-exported here.
- Add a sanity test asserting the re-export is verbatim
(`preloadEntry === preloadEntryFromReact`) so a future refactor can't
silently break the unified import surface.
- Add a comment on the Promise.all line documenting why per-promise
rejection handlers are safe (Promise.all attaches one to each input,
so a single rejection won't leave the others as unhandled).
https://claude.ai/code/session_0166HdHFxs9MBEWdxybXhSkM
The README claims `preloadEntries` honors `vi.mock` because vitest hoists
mock factories above all imports, but no test actually exercised the
dynamic-import path through a mocked module. Add one.
- New `preload-entries.fixture.ts` next to the test (a tiny default
export with `displayName: "real"`).
- Test hoists `vi.mock(...)` returning a mock with `displayName:
"mocked"`, then asserts the cached resolved module after
`preloadEntries` is the mocked one. Reads the cached value through
the synchronous-thenable fast path so no await/DOM is needed.
- tsconfig `exclude` now covers `*.fixture.*` so the fixture stays out
of `dist`. Verified build still produces `index.{js,d.ts}` only.
https://claude.ai/code/session_0166HdHFxs9MBEWdxybXhSkM
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
📝 WalkthroughWalkthroughThis PR introduces ChangesPreload Entries Eager-Resolution Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/testing/src/preload-entries.test.ts (1)
88-94: ⚡ Quick winConsider adding a multi-importer partial-failure case.
The current test exercises a single-importer rejection.
Promise.allsemantics guarantee that a rejection from any one entry propagates, but a test with two entries (one resolving, one rejecting) would also confirm that the resolving importer's promise does not produce an unhandled-rejection warning (the point of the comment inpreload-entries.tslines 49–52). This is a small coverage gap.🧪 Suggested additional assertion in the rejection test
it("propagates importer rejections", async () => { const failure = new Error("chunk load failed"); + const success = vi.fn(() => Promise.resolve({ default: Stub })); const importer = vi.fn(() => Promise.reject(failure)); - const mod = module_("m", { e: lazyEntry(importer) }); + const mod = module_("m", { + e: lazyEntry(importer), + ok: lazyEntry(success), + }); await expect(preloadEntries([mod])).rejects.toBe(failure); + // The resolved importer's promise is handled by Promise.all, so no + // unhandled-rejection warning is emitted from the 'ok' entry. });🤖 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/testing/src/preload-entries.test.ts` around lines 88 - 94, Add a second importer to the existing rejection test so one importer resolves and the other rejects to validate the partial-failure path: create importerA = vi.fn(() => Promise.resolve({})) and importerB = vi.fn(() => Promise.reject(failure)), use module_("m1", { e: lazyEntry(importerA) }) and module_("m2", { e: lazyEntry(importerB) }) and call preloadEntries([mod1, mod2]); assert the returned promise rejects with the same failure (await expect(...).rejects.toBe(failure)) and also assert importerA was called to ensure the resolving importer's promise was observed (preventing unhandled-rejection warnings).
🤖 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/testing/src/preload-entries.test.ts`:
- Around line 88-94: Add a second importer to the existing rejection test so one
importer resolves and the other rejects to validate the partial-failure path:
create importerA = vi.fn(() => Promise.resolve({})) and importerB = vi.fn(() =>
Promise.reject(failure)), use module_("m1", { e: lazyEntry(importerA) }) and
module_("m2", { e: lazyEntry(importerB) }) and call preloadEntries([mod1,
mod2]); assert the returned promise rejects with the same failure (await
expect(...).rejects.toBe(failure)) and also assert importerA was called to
ensure the resolving importer's promise was observed (preventing
unhandled-rejection warnings).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2562f9a0-c727-421d-a933-728d85396592
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.github/workflows/publish.ymlpackages/journeys/package.jsonpackages/journeys/src/module-tab.test.tsxpackages/react/README.mdpackages/testing/README.mdpackages/testing/package.jsonpackages/testing/src/index.tspackages/testing/src/preload-entries.fixture.tspackages/testing/src/preload-entries.test.tspackages/testing/src/preload-entries.tspackages/testing/tsconfig.jsonpackages/testing/vite.config.ts
Imports were re-flowed to single-line per the formatter's defaults.
The version-bump bot promoted @modular-react/core to 2.0.0 in commit ba105a5, but did not update the cross-references that still pinned core to ^1.2.0 / ^1.0.0 across the workspace. pnpm therefore resolved the core dep to the registry's 1.8.1 (the highest 1.x match), which lacks LazyModuleEntryPoint and EagerModuleEntryPoint, breaking typecheck for @modular-react/react and every example using `lazy:` entries. Switch all internal cross-package refs (in `dependencies` and `devDependencies`) to `workspace:*` so pnpm always resolves to the in-repo version. `peerDependencies` are intentionally left alone — the published consumer contract is a separate maintainer decision (and the ^1.2.0 peer pin on react@2.0.0 is itself wrong, but out of scope here). `pnpm publish` (already used in publish.yml) rewrites `workspace:*` to a real version range in the published tarball. For the publishable packages here, all `workspace:*` additions land in `devDependencies` which are stripped from the tarball entirely, so the rewrite is a non-issue for these changes. Verified: `pnpm lint`, `pnpm turbo run typecheck --filter='./examples/**'`, and `pnpm turbo run test --filter='./packages/*'` all pass after the update.
Addresses CodeRabbit nitpick on PR #32. The previous rejection test only exercised a single-importer rejection, which `Promise.all` would propagate unconditionally. The interesting property to verify is the comment in `preload-entries.ts:49-52`: that a mixed resolve/reject lineup does NOT leak the resolving sibling's promise as an unhandledRejection. The new test: - mixes one resolving and one rejecting importer in the same module - registers a process-level `unhandledRejection` listener for the duration of the assertion - drains microtasks via `setImmediate` so any pending notifications fire - asserts `leaks === []` after restoring the listener This gives a concrete behavioral check of the structural guarantee, not just the happy-path rejection propagation.
|
@coderabbitai re: the partial-failure nitpick on Went a bit beyond the suggestion: instead of just adding a sibling resolving importer, I rewrote the rejection test to mix a resolving and a rejecting importer in the same module and registered a process-level Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
| @@ -44,6 +45,7 @@ | |||
| }, | |||
| "peerDependencies": { | |||
| "@modular-react/core": "^1.2.0", | |||
There was a problem hiding this comment.
I believe the react's resolver needs core 2.x.
| "@modular-react/core": "^1.2.0", | |
| "@modular-react/core": "^2.0.0", |
There was a problem hiding this comment.
Good catch — applied in 1a12eaf. The ^1.2.0 claim was inconsistent with the rest of the manifest: testing peers on @modular-react/react@^2.0.0, which itself imports EagerModuleEntryPoint / LazyModuleEntryPoint from core (only exported by core 2.x), so the effective minimum was always 2.x.
Heads-up: the same inconsistency exists in packages/react/package.json (peerDependencies."@modular-react/core": "^1.2.0" while the source imports core 2.x types). I left that alone in this PR to keep scope tight, but happy to fix it here if you'd like — let me know.
Generated by Claude Code
Address @diogomiguel's review comment on PR #32. The testing package's peer on `@modular-react/core` was still `^1.2.0` even though it also peers on `@modular-react/react@^2.0.0`, which itself imports `EagerModuleEntryPoint` / `LazyModuleEntryPoint` from core (only exported by core 2.x). The `^1.2.0` claim was incompatible with what the package actually requires at runtime.
3 participants
preloadEntries(modules)walks everylazy:entry on a module set andprimes the resolver's per-entry cache. After it resolves, the existing
synchronous-thenable fast path in
resolveEntryComponent(proven bypackages/react/src/resolve-entry.test.tsx:123-150) takes over —ModuleTab/JourneyOutlet commit the resolved component on the first
render with no Suspense fallback flash, so tests no longer need
waitFor/actchoreography.preloadEntries(modules)in@modular-react/testingpreloadEntryfrom testing for a single import surfacepropagation, and the post-preload synchronous-thenable behavior
module-tab.test.tsxproves zero fallbackmounts and synchronous render after
preloadEntries@modular-react/testingand a pointer from the@modular-react/reactREADMEhttps://claude.ai/code/session_0166HdHFxs9MBEWdxybXhSkM
Summary by CodeRabbit
Release Notes
New Features
preloadEntriesandpreloadEntryutilities to the testing package for synchronously resolving lazy-loaded module entries during tests, eliminating Suspense fallback flashes.Documentation