refactor(journeys): extract framework-neutral @modular-frontend/journeys-engine - #55
Merged
Conversation
…eys-engine PR-02 of the Vue-support initiative. Split the framework-neutral guts of @modular-react/journeys into a new @modular-frontend/journeys-engine package so a future Vue binding can reuse them, mirroring the @modular-frontend/core extraction from #54. - New packages/journeys-engine (@modular-frontend/journeys-engine): the runtime, validation, persistence, authoring helpers (defineJourney/defineTransition/ selectModule), handles, simulate-journey, the test harness, and the full type surface, plus their non-React tests. Depends only on @modular-frontend/core (happy-dom is a dev dep for the storage-backed persistence tests). Two entries: index + /testing. - @modular-react/journeys keeps its React files (outlet, module-tab, provider, plugin, instance-hooks, use-journey-state, use-wait-for-exit, mount-adapter) and re-exports the engine, so its public export surface is unchanged. A thin testing.ts re-exports the engine's /testing entry, keeping the existing @modular-react/journeys/testing import path working. Two forced deviations from the tracker's PR-02 plan: - mount-adapter.ts stays in the binding, not the engine: createJourneyMountAdapter supplies Outlet: JourneyOutlet (a React component), so it is binding-specific glue over the neutral RuntimeMountAdapter seam rather than engine logic. - JourneyNavContribution.icon used the React.ComponentType namespace; it moves to the neutral UiComponent seam, matching how NavigationItem.icon was neutralized in #54. Source-compatible for authors (a React component still satisfies UiComponent). Error-message prefixes stay [@modular-react/journeys] / [@modular-react/journeys/ testing] on purpose: they name the package users import and point at real import paths, so the moved tests pass unmodified. Verification: engine 346 tests (26 files) + journeys 72 tests (8 files) = the pre-split total; @modular-react/compositions (the createJourneyMountAdapter consumer) 115 tests pass; full workspace typecheck (110 tasks) and pnpm lint clean. The only failing tests are the pre-existing Windows EPERM .test-output flake in the two router CLIs, unrelated to this change.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR extracts a new framework-neutral Changesjourneys-engine extraction and journeys package rewiring
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant TestHarness
participant JourneyRuntime
participant RuntimeInternals
Test->>TestHarness: fireExit(id, name, output)
TestHarness->>RuntimeInternals: __getRecord(id)
RuntimeInternals-->>TestHarness: instance record
TestHarness->>RuntimeInternals: __getRegistered(journey)
RuntimeInternals-->>TestHarness: registered journey
TestHarness->>RuntimeInternals: __bindStepCallbacks(...).exit
RuntimeInternals-->>JourneyRuntime: dispatch exit callback
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/journeys/src/index.ts (1)
18-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc comment references removed local file.
The comment says
getInternalsis "still exported from./runtime.jsfor internal use (the outlet, the test harness itself)", butruntime.tshas moved to@modular-frontend/journeys-engineand no longer exists in this package (confirmed byoutlet.tsxnow importinggetInternalsdirectly from@modular-frontend/journeys-engine). Update the comment to reference the new location to avoid confusing future readers.📝 Proposed comment fix
// `getInternals` intentionally omitted from the public surface — test code // that used to reach through it should migrate to `createTestHarness` in -// `@modular-react/journeys/testing`. The symbol is still exported from -// `./runtime.js` for internal use (the outlet, the test harness itself). +// `@modular-react/journeys/testing`. The symbol is still exported from +// `@modular-frontend/journeys-engine` for internal use (the outlet, the +// test harness itself).🤖 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/journeys/src/index.ts` around lines 18 - 25, The doc comment above the exports in `index.ts` still refers to `./runtime.js`, but that local module no longer exists. Update the comment to point to `@modular-frontend/journeys-engine` as the place where `getInternals` is now available for internal use, keeping the note aligned with the current `outlet.tsx` and `createJourneyRuntime` usage.packages/journeys-engine/src/testing.ts (1)
60-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated record/registration/loading guard into a shared helper.
fireExit,goBack, andgoForwardeach re-implement the samerecordOrThrow→__getRegisterednull-check →loading-status check sequence. Consolidating this into one internal helper (e.g., returning{ record, reg }or throwing with a caller-supplied operation label) would cut duplication and centralize future guard changes.♻️ Example consolidation
+ function activeRecordAndReg(id: InstanceId, opLabel: string) { + const record = recordOrThrow(id); + const reg = internals.__getRegistered(record.journeyId); + if (!reg) { + throw new Error( + `[`@modular-react/journeys/testing`] Journey "${record.journeyId}" is not registered with this runtime.`, + ); + } + if (record.status === "loading") { + throw new Error( + `[`@modular-react/journeys/testing`] ${opLabel} called on instance "${id}" while status=loading. ` + + `Await the runtime's async load probe before dispatching.`, + ); + } + return { record, reg }; + } + return { fireExit(id, name, output) { - const record = recordOrThrow(id); - const reg = internals.__getRegistered(record.journeyId); - if (!reg) { - throw new Error( - `[`@modular-react/journeys/testing`] Journey "${record.journeyId}" is not registered with this runtime.`, - ); - } - if (record.status === "loading") { - throw new Error( - `[`@modular-react/journeys/testing`] fireExit("${name}") called on instance "${id}" while status=loading. ` + - `Await the runtime's async load probe (typically \`await Promise.resolve()\` a few times, or expose a subscribe hook in your test) before dispatching exits.`, - ); - } + const { record, reg } = activeRecordAndReg(id, `fireExit("${name}")`); if (record.status !== "active") { throw new Error( `[`@modular-react/journeys/testing`] fireExit("${name}") called on terminal instance "${id}" (status=${record.status}).`, ); } internals.__bindStepCallbacks(record, reg).exit(name, output); },🤖 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/journeys-engine/src/testing.ts` around lines 60 - 142, Repeated record, registration, and loading checks are duplicated across fireExit, goBack, and goForward; extract that shared validation into one internal helper. Centralize the recordOrThrow call, __getRegistered lookup, and status=loading guard in a helper that returns the validated record/reg pair or throws with a caller-provided operation label, then have fireExit, goBack, and goForward use it so future guard changes only happen in one place.
🤖 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 `@docs/vue-support-tracker.md`:
- Around line 221-222: The PR-02 row in the status board is missing its pull
request reference even though it is marked done. Update the PR-02 entry in the
markdown table to include the PR link/reference, matching the convention used by
the PR-01 row and the board guidance, so the completed item is consistently
labeled in the tracker.
- Around line 39-46: The table currently reads as if both engine packages are
already completed, but `@modular-frontend/compositions-engine` is still planned;
update the wording in this section to clearly distinguish completed vs pending
extraction work. Adjust the table/header in the `Shared engine packages
extracted in Phase 0` block (or split into done and planned rows) so
`@modular-frontend/journeys-engine` and `@modular-frontend/compositions-engine`
reflect their actual status consistently.
In `@packages/journeys-engine/README.md`:
- Around line 22-23: The journeys-engine README currently lists
createJourneyMountAdapter as an exported mount adapter, but that symbol is not
shipped by this package. Remove the mount adapter bullet from the engine docs
and keep the remaining type-surface description intact, so the README only
advertises exports actually provided by journeys-engine.
---
Nitpick comments:
In `@packages/journeys-engine/src/testing.ts`:
- Around line 60-142: Repeated record, registration, and loading checks are
duplicated across fireExit, goBack, and goForward; extract that shared
validation into one internal helper. Centralize the recordOrThrow call,
__getRegistered lookup, and status=loading guard in a helper that returns the
validated record/reg pair or throws with a caller-provided operation label, then
have fireExit, goBack, and goForward use it so future guard changes only happen
in one place.
In `@packages/journeys/src/index.ts`:
- Around line 18-25: The doc comment above the exports in `index.ts` still
refers to `./runtime.js`, but that local module no longer exists. Update the
comment to point to `@modular-frontend/journeys-engine` as the place where
`getInternals` is now available for internal use, keeping the note aligned with
the current `outlet.tsx` and `createJourneyRuntime` usage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3771a5a2-afff-4ec4-ba40-8cf4a23b33b9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (57)
docs/vue-support-tracker.mdpackages/journeys-engine/README.mdpackages/journeys-engine/package.jsonpackages/journeys-engine/src/build-input.test.tspackages/journeys-engine/src/define-journey.tspackages/journeys-engine/src/define-transition.test-d.tspackages/journeys-engine/src/define-transition.test.tspackages/journeys-engine/src/define-transition.tspackages/journeys-engine/src/handle.test-d.tspackages/journeys-engine/src/handle.test.tspackages/journeys-engine/src/handle.tspackages/journeys-engine/src/index.tspackages/journeys-engine/src/invoke-cycle-safety.test.tspackages/journeys-engine/src/invoke.test.tspackages/journeys-engine/src/mount-kinds.test-d.tspackages/journeys-engine/src/persistence.test-d.tspackages/journeys-engine/src/persistence.test.tspackages/journeys-engine/src/persistence.tspackages/journeys-engine/src/register-options.test-d.tspackages/journeys-engine/src/register-options.test.tspackages/journeys-engine/src/runtime-go-back.test.tspackages/journeys-engine/src/runtime-go-forward.test.tspackages/journeys-engine/src/runtime-rewind-to.test.tspackages/journeys-engine/src/runtime.test-d.tspackages/journeys-engine/src/runtime.test.tspackages/journeys-engine/src/runtime.tspackages/journeys-engine/src/select-module.test-d.tspackages/journeys-engine/src/select-module.test.tspackages/journeys-engine/src/select-module.tspackages/journeys-engine/src/simulate-journey-invoke.test.tspackages/journeys-engine/src/simulate-journey.test-d.tspackages/journeys-engine/src/simulate-journey.test.tspackages/journeys-engine/src/simulate-journey.tspackages/journeys-engine/src/testing.test.tspackages/journeys-engine/src/testing.tspackages/journeys-engine/src/types.tspackages/journeys-engine/src/validation.test.tspackages/journeys-engine/src/validation.tspackages/journeys-engine/src/wildcard-transitions.test-d.tspackages/journeys-engine/src/wildcard-transitions.test.tspackages/journeys-engine/tsconfig.jsonpackages/journeys-engine/vite.config.tspackages/journeys-engine/vitest.config.tspackages/journeys/package.jsonpackages/journeys/src/index.tspackages/journeys/src/mount-adapter.tspackages/journeys/src/mount-kinds-runtime.test.tsxpackages/journeys/src/outlet-invoke.test.tsxpackages/journeys/src/outlet-preload.test.tsxpackages/journeys/src/outlet.test.tsxpackages/journeys/src/outlet.tsxpackages/journeys/src/plugin.tsxpackages/journeys/src/provider.test.tsxpackages/journeys/src/provider.tsxpackages/journeys/src/testing.tspackages/journeys/src/use-journey-state.test.tsxpackages/journeys/vite.config.ts
createJourneyMountAdapter stays in the @modular-react/journeys binding (it supplies the React JourneyOutlet), so the engine does not ship it. Listing it under "What's included" advertised an export this package does not provide.
- vue-support-tracker: soften the shared-engines header so it no longer reads as if compositions-engine is already extracted, and add the #55 reference to the PR-02 status-board row. - journeys/index.ts: point the getInternals note at @modular-frontend/journeys-engine; runtime.ts no longer lives in this package. - journeys-engine/testing.ts: extract the repeated record + registration + loading guard shared by fireExit/goBack/goForward into one activeRecordAndReg helper.
Owner
Author
|
Addressed the two nitpicks from the review as well (d24c763):
Verification after the changes: |
kibertoad
added a commit
that referenced
this pull request
Jul 6, 2026
…ompositions-engine (#56) PR-03 of the Vue-support initiative. Splits the framework-neutral guts of @modular-react/compositions into a new @modular-frontend/compositions-engine package so a future Vue binding can reuse them, mirroring the journeys-engine extraction (#55) and the frontend-core extraction (#54). What moves: the pure files (runtime.ts, stores.ts, validation.ts, define-composition.ts, types.ts) plus their non-.tsx tests. The engine depends only on @modular-frontend/core; happy-dom is a dev dep for the test environment. @modular-react/compositions keeps its React files (outlet.tsx, provider.tsx, plugin.tsx, hooks.ts) and all .tsx tests, and re-exports the engine so its public export surface is unchanged. One React reference had to be neutralized, matching #54/PR-02: CompositionZoneDescriptor.fallback used React.ComponentType; it moves to the neutral UiComponent seam from @modular-frontend/core. Source-compatible for authors (a React component still satisfies UiComponent). Error-message prefixes stay [@modular-react/compositions] on purpose — they name the package users import. Test counts preserved: 52 in the engine + 63 in the binding = the pre-split total of 115. Full workspace typecheck and lint clean. Claude-Session: https://claude.ai/code/session_01Sf7deW3iRQaaSjZuj3akTA Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Jul 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR-02 of the Vue-support initiative. Splits the framework-neutral guts of
@modular-react/journeysinto a new@modular-frontend/journeys-enginepackage so a future Vue binding can reuse them, mirroring the@modular-frontend/coreextraction in #54.What moves
New package
@modular-frontend/journeys-engine(v1.7.1, depends only on@modular-frontend/core;happy-domis a dev dep for the storage-backed persistence tests). It holds the runtime, validation, persistence, authoring helpers (defineJourney/defineTransition/selectModule), handles,simulate-journey, the test harness, and the full type surface, plus their non-React tests. Two build entries:index+/testing.@modular-react/journeyskeeps its React files (outlet,module-tab,provider,plugin,instance-hooks,use-journey-state,use-wait-for-exit,mount-adapter) and re-exports the engine, so its public export surface is unchanged. A thintesting.tsre-exports the engine's/testingentry, keeping the@modular-react/journeys/testingimport path working.Deviations from the tracker's PR-02 plan
Both forced by the code, and recorded in the tracker:
mount-adapter.tsstays in the binding, not the engine:createJourneyMountAdaptersuppliesOutlet: JourneyOutlet(a React component), so it is binding-specific glue over the neutralRuntimeMountAdapterseam rather than engine logic.JourneyNavContribution.iconused theReact.ComponentTypenamespace; it moves to the neutralUiComponentseam, matching howNavigationItem.iconwas neutralized in refactor(core): extract framework-neutral @modular-frontend/core #54. Source-compatible for authors (a React component still satisfiesUiComponent).Error-message prefixes stay
[@modular-react/journeys]/[@modular-react/journeys/testing]on purpose: they name the package users import and point at real import paths, so the moved tests pass unmodified.Verification
@modular-frontend/journeys-engine: 346 tests (26 files);@modular-react/journeys: 72 tests (8 files). 346 + 72 = the pre-split total.@modular-react/compositions(the onecreateJourneyMountAdapterconsumer): 115 tests pass.pnpm lintclean..test-outputflake in the two router CLIs, unrelated to this change.Also resolves decision D2 (engine scope =
@modular-frontend) and marks PR-01 done (landed in #54) in the tracker.Summary by CodeRabbit
New Features
Documentation
Refactor