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:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Shell
participant JourneyRuntime
participant Persistence
participant JourneyOutlet
participant ModuleTab
User->>Shell: Click "Start Onboarding"
Shell->>JourneyRuntime: start(journeyHandle, input)
JourneyRuntime->>Persistence: load(key)
alt persisted active blob
Persistence-->>JourneyRuntime: blob (status: active)
JourneyRuntime->>JourneyRuntime: hydrate(blob)
else no blob
JourneyRuntime->>JourneyRuntime: create instance (status: active)
JourneyRuntime->>Persistence: save(serialized)
end
JourneyRuntime-->>Shell: { instanceId }
Shell->>JourneyOutlet: mount(instanceId)
JourneyOutlet->>JourneyRuntime: subscribe(instanceId)
JourneyRuntime-->>JourneyOutlet: currentStep (moduleId, entry, input)
JourneyOutlet->>ModuleTab: render module entry with input
User->>ModuleTab: interact -> exit(name, payload)
ModuleTab->>JourneyRuntime: exit(instanceId, name, payload)
JourneyRuntime->>JourneyRuntime: apply transition, update state/history
JourneyRuntime->>Persistence: save(updated)
JourneyRuntime-->>JourneyOutlet: notify subscribers (new step / terminal)
alt terminal
JourneyRuntime->>Persistence: remove(key)
JourneyOutlet->>Shell: onFinished(terminalOutcome)
end
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/workspace-patterns.md (1)
489-500:⚠️ Potential issue | 🟡 MinorExample still calls the newly-deprecated
openModuleTab.You just annotated
openModuleTabwith@deprecated(line 467) and introducedopenTab({ kind: 'module', ... })as the replacement, but theInvoiceActionsexample below still demonstratesworkspace.openModuleTab('payments'). Readers hitting this section after reading the interface change will get mixed signals. Update the example (and keep one short aside notingopenModuleTabis still accepted for back-compat) so the doc's recommended call path is consistent.📝 Suggested doc fix
function InvoiceActions({ invoiceId }: { invoiceId: string }) { const workspace = useService('workspace') return ( - <button onClick={() => workspace.openModuleTab('payments')}> + <button onClick={() => workspace.openTab({ kind: 'module', id: 'payments' })}> Pay Invoice </button> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/workspace-patterns.md` around lines 489 - 500, Update the InvoiceActions example to use the new API: replace the deprecated workspace.openModuleTab('payments') call with workspace.openTab({ kind: 'module', id: 'payments' }) in the InvoiceActions component that uses useService('workspace'); also add one short aside sentence below the example stating that openModuleTab is still accepted for back-compat but deprecated, referencing the openModuleTab and openTab symbols so readers see the recommended call path.
🧹 Nitpick comments (39)
packages/tanstack-router-runtime/package.json (1)
49-58: Consider marking@modular-react/journeysas an optional peer dependency.The PR states journeys is an additive, opt-in layer with no impact on apps that don't use it, but adding it as a required
peerDependencywill emit install warnings/errors for all existing consumers of@tanstack-react-modules/runtimewho don't adopt journeys. Consider declaring it optional viapeerDependenciesMeta.♻️ Proposed change
"peerDependencies": { "@modular-react/core": "^1.2.0", "@modular-react/journeys": "^0.1.0", "@modular-react/react": "^1.2.0", ... + }, + "peerDependenciesMeta": { + "@modular-react/journeys": { "optional": true } }Also note:
@modular-react/journeysat^0.1.0is a 0.x range — per semver, any minor bump (0.2.0) will break this constraint. Confirm this is the intended floor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tanstack-router-runtime/package.json` around lines 49 - 58, Update package metadata to make `@modular-react/journeys` optional: in packages/tanstack-router-runtime/package.json, add a peerDependenciesMeta entry for "@modular-react/journeys" with {"optional": true} and keep it listed under peerDependencies so consumers that use it still get the peer check; also verify the intended minimum semver for "@modular-react/journeys" (currently "^0.1.0" is a 0.x range) and adjust the version spec if you want a less restrictive floor.packages/react-router-testing/vite.config.ts (1)
13-27: Externalize subpaths with a regex to avoid missing future entry points.Listing
@modular-react/journeysand@modular-react/journeys/testingexplicitly works, but if the package adds another subpath export later (e.g.,/react) it will silently get bundled. A pattern like/^@modular-react\/journeys(\/.*)?$/is more future-proof. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router-testing/vite.config.ts` around lines 13 - 27, Replace the literal entries for "@modular-react/journeys" and "@modular-react/journeys/testing" in the Vite config's external array with a regex that matches the package and any subpaths (e.g., /^@modular-react\/journeys(\/.*)?$/) so future subpath exports aren't accidentally bundled; update the external array where those strings appear in vite.config.ts (the external: [...] declaration) to include this regex instead of the two explicit entries.examples/tanstack-router/customer-onboarding-journey/shell/index.html (1)
7-41: Consider extracting shared shell CSS to avoid drift.This style block appears duplicated across example shells. Moving it to a shared stylesheet would reduce maintenance overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/shell/index.html` around lines 7 - 41, Extract the repeated shell CSS (the <style> block defining selectors *, *::before, *::after, body, button, button:hover, button:disabled, and code) into a shared stylesheet and replace the inline block in this file with a <link rel="stylesheet"> reference to that shared file; ensure the shared stylesheet preserves the exact rules and that any example shells update their HTML to import the same shared CSS so styles don't drift.examples/react-router/customer-onboarding-journey/shell/src/stores/workspace-tabs.ts (1)
30-30: Narrow the return type offindJourneyTabByInstancetoJourneyTab | null.The implementation filters by
t.kind === "journey", so the result is always aJourneyTab(ornull), never aModuleTab. Typing it asTab | nullforces callers to re-narrow. Since this is the public contract of the store, downstream consumers (e.g.workspace-actions.ts) benefit from the precise type.Note: the same fix applies to the duplicated file at
examples/tanstack-router/customer-onboarding-journey/shell/src/stores/workspace-tabs.ts.♻️ Proposed type narrowing
- readonly findJourneyTabByInstance: (instanceId: string) => Tab | null; + readonly findJourneyTabByInstance: (instanceId: string) => JourneyTab | null;findJourneyTabByInstance: (instanceId) => { const { tabs } = get(); - return tabs.find((t) => t.kind === "journey" && t.instanceId === instanceId) ?? null; + return ( + tabs.find( + (t): t is JourneyTab => t.kind === "journey" && t.instanceId === instanceId, + ) ?? null + ); },Also applies to: 91-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react-router/customer-onboarding-journey/shell/src/stores/workspace-tabs.ts` at line 30, The method findJourneyTabByInstance currently returns Tab | null but its implementation filters t.kind === "journey", so change its signature to return JourneyTab | null; update the declaration of readonly findJourneyTabByInstance to use JourneyTab | null (in workspace-tabs.ts) and mirror the same change in the duplicate file in the tanstack-router example so callers no longer need to re-narrow the result.examples/tanstack-router/customer-onboarding-journey/app-shared/src/index.ts (2)
26-29: Nit:readonly commands: readonly never[]is an awkward way to express "no slots".
never[]is fine but the intent ("modules contribute no slot items") reads more naturally as an empty tuple literal type, which also prevents any accidental future push of a non-neveritem from a structurally-compatible value.-export interface AppSlots { - // Intentionally empty — this example renders exclusively via journey tabs. - readonly commands: readonly never[]; -} +export interface AppSlots { + // Intentionally empty — this example renders exclusively via journey tabs. + readonly commands: readonly []; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/app-shared/src/index.ts` around lines 26 - 29, Replace the awkward "readonly commands: readonly never[]" in the AppSlots interface with an empty tuple literal type to express “no slots” more clearly: update the AppSlots interface property commands to use a readonly empty tuple (i.e., the empty tuple literal type) so it communicates intent and prevents accidental pushes; adjust any callers if they relied on the previous never[] shape.
58-63: Make the deprecatedopenModuleTaboptional to allow shells to stop providing it.
openModuleTabis JSDoc-deprecated in favor ofopenTab({ kind: 'module', ... })but remains a required member ofWorkspaceActions. No active callers ofopenModuleTabexist in the codebase—both shell implementations already delegate it toopenTab. Making it optional would allow shells to drop the legacy method at their own pace while maintaining backward compatibility.♻️ Suggested change
export interface WorkspaceActions { /** `@deprecated` Use `openTab({ kind: 'module', id, input })` instead. */ - readonly openModuleTab: (moduleId: string, input?: unknown) => OpenTabResult; + readonly openModuleTab?: (moduleId: string, input?: unknown) => OpenTabResult; readonly openTab: (spec: OpenTabSpec) => OpenTabResult; readonly closeTab: (tabId: string) => void; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/app-shared/src/index.ts` around lines 58 - 63, The WorkspaceActions interface currently requires the deprecated method openModuleTab; change its signature to be optional so shells can omit it: update the interface definition for WorkspaceActions to make openModuleTab?: (moduleId: string, input?: unknown) => OpenTabResult (keeping the JSDoc deprecation comment), and ensure no other call sites rely on it by preferring openTab(spec) where spec.kind === 'module'—leave openTab and closeTab unchanged.examples/tanstack-router/customer-onboarding-journey/shell/src/services/workspace-actions.ts (1)
18-21: Module-levelnextTabIdis shared across allcreateWorkspaceActionsinstances.Minor: if the shell ever instantiates multiple workspaces (e.g., tests, multi-root), they share a monotonic counter. Fine for a single-shell example, but consider moving the counter inside
createWorkspaceActionsfor isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/shell/src/services/workspace-actions.ts` around lines 18 - 21, The module-level counter nextTabId and mintTabId create a shared monotonic ID across all createWorkspaceActions instances; make the counter instance-scoped by moving nextTabId into createWorkspaceActions and either recreate mintTabId as a closure inside createWorkspaceActions or change mintTabId to accept the instance counter, so each createWorkspaceActions call gets its own counter (refer to nextTabId, mintTabId, and createWorkspaceActions to locate the code).packages/tanstack-router-testing/src/render-journey.test.tsx (1)
46-58: Optional: assert the initial entry renders before acting.Currently the test jumps straight from
status === "active"to clickingconfirm-it. Addingexpect(getByText("confirm-it")).toBeTruthy()(or similar) before theact()would catch regressions where the outlet mounts but fails to render the initial module entry, which is otherwise masked by the latergetByTextthrowing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tanstack-router-testing/src/render-journey.test.tsx` around lines 46 - 58, Add an assertion that the initial entry is rendered before interacting: after calling renderJourney(journey, ...) and after confirming runtime.getInstance(instanceId)?.status is "active", assert that getByText("confirm-it") (or an equivalent query for the initial outlet content) is present/truthy before calling act() and clicking it; this ensures renderJourney, the outlet mount, and the initial module entry (referenced by getByText("confirm-it")) are actually rendered prior to driving the transition with act() and clicking.packages/journeys/src/module-tab.test.tsx (2)
102-107: Brittle: assertion depends on entry iteration order.
getByText(/review, other/)will fail if the notice ever lists keys alphabetically or in a different order. If the implementation guarantees insertion order, consider asserting both names independently to decouple from formatting:♻️ Proposed tweak
- expect(getByText(/exposes multiple entries/)).toBeTruthy(); - expect(getByText(/review, other/)).toBeTruthy(); + const notice = getByText(/exposes multiple entries/); + expect(notice.textContent).toMatch(/\breview\b/); + expect(notice.textContent).toMatch(/\bother\b/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/module-tab.test.tsx` around lines 102 - 107, The test for ModuleTab is brittle because it asserts a specific entry order using getByText(/review, other/); change the assertion to check for each entry name independently (e.g., assert that both "review" and "other" are present via separate getByText calls or use a regex that accepts either order) so it doesn't rely on insertion ordering of the module entries referenced by multiMod and ModuleTab.
73-84: Nit: test title vs. scenario.The title says "when no entry matches," but the test exercises a module that has no
entryPointsat all (and passes noentryprop). Consider renaming to e.g."falls back to module.component when the module declares no entries"to avoid implying the lookup-miss path (which is actually covered by theentry="ghost"test below).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/module-tab.test.tsx` around lines 73 - 84, Rename the test title to accurately reflect the scenario: update the it(...) description for the test that renders Legacy/legacyMod with ModuleTab to something like "falls back to module.component when the module declares no entries" (references: ModuleTab, defineModule, legacyMod, Legacy) so it doesn't imply an entry lookup miss.packages/journeys/vite.config.ts (1)
14-22: Optional: make the@modular-react/*external match future-proof.The current list only externalizes exact package ids. If any source later imports a subpath (e.g.
@modular-react/core/testing), Rollup would try to bundle it. A function form avoids that surprise.♻️ Proposed refactor
rollupOptions: { - external: [ - "react", - "react/jsx-runtime", - "react-dom", - "@modular-react/core", - "@modular-react/react", - ], + external: (id) => + id === "react" || + id === "react-dom" || + id.startsWith("react/") || + id.startsWith("@modular-react/"), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/vite.config.ts` around lines 14 - 22, The external array in rollupOptions only matches exact package IDs so subpath imports like `@modular-react/core/testing` would be bundled; update rollupOptions.external to a function (or use a regex) that returns true for exact matches ("react", "react/jsx-runtime", "react-dom") and also for any id that startsWith or matches the prefix "@modular-react/" so all subpaths (e.g. "@modular-react/core/testing") are treated external; locate rollupOptions and replace the external array with a function that checks these conditions.packages/journeys/src/simulate-journey.test.ts (1)
10-10: Nit: avoid shadowingmoduleas a local identifier.
moduleis a well-known CJS global and reads awkwardly in ESM files; some lint configs (no-shadow-restricted-names,no-restricted-globals) will flag it. Consider renaming to e.g.menuModule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/simulate-journey.test.ts` at line 10, The test declares a local variable named "module" via the call to defineModule which shadows the CommonJS global and may trigger lint rules; rename that local identifier (e.g., to "menuModule" or "testModule") wherever it is declared and used in this file (the defineModule call and any references to its properties/methods) to avoid shadowing and satisfy rules like no-shadow-restricted-names/no-restricted-globals.packages/react-router-testing/src/render-journey.test.tsx (1)
46-58: Consider usingfireEvent.clickoruserEventinstead of native.click().Calling
.click()directly on the DOM node works but bypasses Testing Library's event system.fireEvent.click(getByText("confirm-it"))(oruserEvent.click) is the idiomatic pattern and auto-wraps inact, making the explicitact()here unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router-testing/src/render-journey.test.tsx` around lines 46 - 58, Replace the direct DOM .click() inside act with Testing Library's event helper: in the test that calls renderJourney (symbols: renderJourney, runtime, instanceId, getByText), remove the explicit act wrapper and invoke fireEvent.click(getByText("confirm-it")) (or userEvent.click) so the click is dispatched via Testing Library's event system and auto-wrapped; keep the rest of the assertions unchanged.examples/react-router/customer-onboarding-journey/shell/src/persistence.ts (1)
23-33: Consider validating shape, not just JSON-parse-ability.
loadonly guards against JSON parse errors; a JSON-valid-but-wrong-shape blob (e.g. from an older schema) still reaches the runtime and can blow up on hydration. For a demo this is probably fine, but a lighttypeof blob === "object" && blob && "status" in blobcheck (andremoveon failure, matching your JSON-parse branch) would make reload-recovery more robust across schema drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react-router/customer-onboarding-journey/shell/src/persistence.ts` around lines 23 - 33, In the load function (load: (key: string): SerializedJourney<OnboardingState> | null), after JSON.parse succeeds, validate the parsed blob’s shape before returning: ensure the value is a non-null object and contains expected keys (e.g., check typeof parsed === "object" && parsed !== null && "status" in parsed or other required props from SerializedJourney/OnboardingState); if the shape check fails, call localStorage.removeItem(key) and return null to match the parse-error branch so malformed or old-schema data won’t be returned.examples/tanstack-router/customer-onboarding-journey/shell/src/components/Home.tsx (1)
6-17: UnusedtabsStoreprop.
HomePropsdeclarestabsStore, butHomeonly destructuresworkspace. Either drop the prop from the interface or consume it (and remove the prop from callers).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/shell/src/components/Home.tsx` around lines 6 - 17, HomeProps declares tabsStore but the Home component never uses it; either remove tabsStore from the HomeProps interface or actually consume it in the Home component. To fix, update the HomeProps type (remove the readonly tabsStore: StoreApi<WorkspaceTabsState> field) and remove that prop from any Home call sites/JSX where passed, or alternatively change the Home signature to accept and destructure tabsStore (function Home({ workspace, tabsStore }: HomeProps)) and use tabsStore inside the component; reference the HomeProps interface, the tabsStore symbol, and the Home component when making the change.examples/tanstack-router/customer-onboarding-journey/shell/src/persistence.ts (1)
56-61: Consider cleaning up corrupt entries here too.
loadremoves invalid JSON from storage on parse failure, buthasPersistedJourneysilently returnsfalseand leaves the corrupt entry behind. Minor inconsistency — optional to mirror the cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/shell/src/persistence.ts` around lines 56 - 61, The hasPersistedJourney function currently swallows JSON parse errors and returns false without removing the corrupt blob; update its catch block to delete the stored entry (using the same storage API used by load) when JSON.parse throws, then return false. Locate hasPersistedJourney in persistence.ts and mirror the cleanup logic from load: on parse failure call the storage removal method (e.g., removeItem or delete for the same key) to clear the corrupt entry before returning false.examples/react-router/customer-onboarding-journey/shell/src/components/TabStrip.tsx (1)
27-38: Minor a11y: consideraria-current="page"on the active tab/Home button.The active tab is indicated only via color. Adding
aria-currentcommunicates selection to assistive tech.Also applies to: 57-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react-router/customer-onboarding-journey/shell/src/components/TabStrip.tsx` around lines 27 - 38, Add aria-current="page" to the Home button and to each tab button when they are the active tab so assistive tech knows which tab is selected; specifically update the JSX in TabStrip.tsx (the Home button that calls tabsStore.getState().activateTab(null) and the other tab button render block around lines 57-70) to conditionally include aria-current="page" when state.activeTabId === null for Home or when state.activeTabId === tab.id for each tab.examples/react-router/customer-onboarding-journey/app-shared/package.json (1)
16-19: Considerworkspace:*for intra-repo dependencies.In a pnpm monorepo, referencing
@modular-react/coreand@react-router-modules/coreby semver range may resolve to a registry version (or fail to resolve) instead of the local workspace package. Example packages typically use"workspace:*".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react-router/customer-onboarding-journey/app-shared/package.json` around lines 16 - 19, The dependency entries in package.json currently use semver ranges which can pull from the registry; change the intra-repo dependency declarations for "@modular-react/core" and "@react-router-modules/core" in the "dependencies" section to use "workspace:*" so pnpm resolves the local workspace packages (update the two dependency values in package.json accordingly).packages/journeys/src/validation.test.ts (1)
125-134: Consider asserting on specific issues.
expect(issues.length).toBeGreaterThanOrEqual(1)would still pass if validation surfaced an unrelated issue. Asserting on the issue codes/messages for the missingidandinitialStatefields would better pin the behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/validation.test.ts` around lines 125 - 134, The test "reports missing required fields" should assert specific validation issues instead of only checking length; update the test that calls validateJourneyDefinition(bad) to look for issue objects that indicate the missing id and missing initialState (e.g., check issues contains entries whose path/name equals "id" and "initialState" or whose message/code references those fields) so the test fails if unrelated issues are reported; locate the test in validation.test.ts and replace the expect(issues.length).toBeGreaterThanOrEqual(1) with assertions that inspect issues[].path or issues[].message (or issues[].code) for "id" and "initialState".packages/journeys/src/provider.tsx (1)
41-45: Memoize the context value.
valueis a fresh object on every render, so every descendant that subscribes viauseJourneyContextre-renders wheneverJourneyProvider's parent re-renders — even whenruntimeandonModuleExitare stable. Wrap inuseMemo.Proposed fix
-import { createContext, createElement, useContext } from "react"; +import { createContext, createElement, useContext, useMemo } from "react"; @@ export function JourneyProvider(props: JourneyProviderProps): ReactNode { const { runtime, onModuleExit, children } = props; - const value: JourneyProviderValue = { runtime, onModuleExit }; + const value = useMemo<JourneyProviderValue>( + () => ({ runtime, onModuleExit }), + [runtime, onModuleExit], + ); return createElement(JourneyContext.Provider, { value }, children); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/provider.tsx` around lines 41 - 45, The JourneyProvider creates a new value object each render causing unnecessary re-renders; memoize the context value by wrapping the { runtime, onModuleExit } object in React.useMemo and pass that memoized value to JourneyContext.Provider inside the JourneyProvider function (keep references to JourneyProvider, value, runtime, onModuleExit, and JourneyContext.Provider) so the context only updates when runtime or onModuleExit change.examples/react-router/customer-onboarding-journey/modules/billing/src/CollectPayment.tsx (1)
9-11: Nit: call out thatMath.random()is illustrative only.This is obviously an example component, but since "PAY-…" references are the kind of thing that gets copy-pasted into real billing code, it may be worth a one-line comment noting that a real integration should source the reference from the payment processor (or at minimum
crypto.randomUUID()), notMath.random().toString(36). No functional change needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react-router/customer-onboarding-journey/modules/billing/src/CollectPayment.tsx` around lines 9 - 11, Add a one-line comment above the makeReference function explaining that using Math.random() is only illustrative and should not be used in production billing code; suggest sourcing a reference from the payment processor or using a stronger identifier like crypto.randomUUID() instead, and reference the function name makeReference so reviewers can locate it quickly.packages/journeys/src/outlet.test.tsx (2)
1-11: Nit:afterEachhook is interleaved with imports.The
afterEach(cleanup)sits between the testing-library import and the module-under-test imports. Works fine because ESM imports are hoisted, but it reads oddly — consider moving it below all imports for clarity.Proposed reorder
import { act, cleanup, render } from "@testing-library/react"; import { afterEach, describe, expect, it, vi } from "vitest"; - -afterEach(() => { - cleanup(); -}); import { defineEntry, defineExit, defineModule, schema } from "@modular-react/core"; import type { ModuleEntryProps } from "@modular-react/core"; import { defineJourney } from "./define-journey.js"; import { createJourneyRuntime } from "./runtime.js"; import { JourneyOutlet } from "./outlet.js"; + +afterEach(() => { + cleanup(); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/outlet.test.tsx` around lines 1 - 11, Move the afterEach(() => { cleanup(); }) hook so it appears after all imports (e.g., after imports of defineEntry/defineExit/defineModule/schema, ModuleEntryProps, defineJourney, createJourneyRuntime, and JourneyOutlet) to avoid interleaving test setup with module imports; locate the current afterEach usage and relocate it below the import block so all imports are grouped together and then test lifecycle hooks follow.
216-250: Loading-fallback test settles the promise but never asserts the post-load UI.After
resolveLoad(null)the test awaits two microtasks but never re-queries the DOM to confirm the loading fallback is removed or the real step rendered. That leaves the "transitions out of loading" path completely uncovered — a regression that keepsloadingFallbackdisplayed forever would still pass this test.Suggested follow-up assertion
expect(getByText("please wait")).toBeTruthy(); resolveLoad(null); await Promise.resolve(); await Promise.resolve(); + // After the persistence load resolves, the loading fallback should be gone + // and the real step should render. + expect(rt.getInstance(id)!.status).toBe("active");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/outlet.test.tsx` around lines 216 - 250, The test currently resolves the persistence load promise (resolveLoad(null)) but never asserts the post-load UI; update the test around the JourneyOutlet render to wait for the UI after resolving the load: after resolveLoad(null) await the DOM update using an async matcher (e.g. await findByText or waitFor) to assert that the loadingFallback text "please wait" is removed and the expected step content from the journey is present; locate this change in the test that uses createJourneyRuntime, JourneyOutlet, resolveLoad and getByText and add the appropriate post-resolution assertions.packages/react-router-runtime/src/registry.ts (1)
319-349: Consider makingmoduleDescriptorsimmutable before handing it to the runtime.
moduleDescriptorsis a plain mutable record that gets passed intocreateJourneyRuntimeand also surfaced on the resolved manifest (lines 338, 452, 521). If the runtime (or a consumer) later adds or swaps descriptors at runtime, you'd silently mutate the assembly record shared with the manifest. A shallowObject.freezehere would surface such misuse immediately without any real cost.Optional hardening
const moduleDescriptors: Record<string, ModuleDescriptor<any, any, any, any>> = {}; for (const mod of modules) moduleDescriptors[mod.id] = mod as ModuleDescriptor<any, any, any, any>; + Object.freeze(moduleDescriptors);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router-runtime/src/registry.ts` around lines 319 - 349, moduleDescriptors is a mutable plain object that gets passed into createJourneyRuntime and returned on the manifest; make it immutable to prevent later accidental mutation by calling Object.freeze(moduleDescriptors) (and optionally Object.freeze(...) each descriptor value) immediately after populating it and before passing it to createJourneyRuntime and returning it, so the runtime or consumers will get a frozen/shallow-immutable moduleDescriptors and mutations will throw rather than silently corrupt the manifest.packages/core/src/entry-exit.ts (1)
81-87: Minor: simplify the exit-shape predicate.
exit !== undefined && (exit === null || typeof exit !== "object")is correct but reads awkwardly. Sincetypeof null === "object", the condition can fold into a single expression.Optional simplification
if (exitPoints) { for (const [name, exit] of Object.entries(exitPoints)) { - if (exit !== undefined && (exit === null || typeof exit !== "object")) { + if (exit !== undefined && (exit === null || typeof exit !== "object")) { + issues.push(`exit "${name}" must be an object or undefined`); + } + // Equivalent, slightly tighter: + // if (exit !== undefined && (typeof exit !== "object" || exit === null)) { ... } - issues.push(`exit "${name}" must be an object or undefined`); - } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/entry-exit.ts` around lines 81 - 87, The predicate for validating exit shapes is awkward; replace the complex condition in the loop over exitPoints with a clearer expression such as testing for explicit null or non-object values: use exit === null || (exit !== undefined && typeof exit !== "object") where the current code checks exit !== undefined && (exit === null || typeof exit !== "object"); update the check inside the for (const [name, exit] of Object.entries(exitPoints)) loop so issues.push still runs when exit is null or any non-object (but not when undefined).packages/tanstack-router-runtime/src/registry.ts (1)
366-377: Consider rejecting duplicate journey registrations.
registerJourneyvalidates the structural shape but doesn't check for a duplicate(id, version)already present injourneys[]. A host that accidentally imports/registers the same journey twice will end up with both entries, and downstreamvalidateJourneyContracts/createJourneyRuntimewill see a list with duplicates. The module path guards this viavalidateNoDuplicateIds; a symmetric check here would surface the mistake loudly at registration time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tanstack-router-runtime/src/registry.ts` around lines 366 - 377, registerJourney currently validates structure but allows duplicate registrations; update registerJourney to check journeys[] for an existing entry with the same definition.id and definition.version before pushing. If a duplicate (same id and version) is found, throw a descriptive error (e.g., JourneyValidationError or a new DuplicateJourneyError) identifying the id/version so registration fails fast; keep the existing validateJourneyDefinition check and only push when no duplicate is present. Use the symbols registerJourney, journeys, and AnyJourneyDefinition.definition.id/version to locate and implement the check.examples/react-router/customer-onboarding-journey/shell/src/services/workspace-actions.ts (1)
14-17: Module-scopednextTabIdpersists across factory calls.
nextTabIdlives at module scope, so every call tocreateWorkspaceActions()shares the same counter. In an example app this is fine, but if this pattern gets copy-pasted into a real host (or used from tests that instantiate multiple registries), IDs can collide with another workspace's assumptions or leak across test cases. Moving it insidecreateWorkspaceActionswould make each workspace self-contained.♻️ Proposed scope change
-let nextTabId = 1; -function mintTabId(kind: "module" | "journey", key: string): string { - return `${kind}:${key}:${nextTabId++}`; -} - /** * Wire the workspace actions. The journey runtime reference is filled in * after `registry.resolve()` (which is what creates the runtime), so we * accept a mutable box instead of a concrete runtime. */ export function createWorkspaceActions( tabsStore: StoreApi<WorkspaceTabsState>, runtimeRef: RuntimeRef, ): WorkspaceActions { + let nextTabId = 1; + const mintTabId = (kind: "module" | "journey", key: string): string => + `${kind}:${key}:${nextTabId++}`; + function openTab(spec: OpenTabSpec): OpenTabResult {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react-router/customer-onboarding-journey/shell/src/services/workspace-actions.ts` around lines 14 - 17, The module-level counter nextTabId and mintTabId should be moved inside createWorkspaceActions so each workspace gets its own counter; update createWorkspaceActions to declare let nextTabId = 1 and define mintTabId(kind: "module" | "journey", key: string): string within that function so mintTabId closes over the per-instance counter, and replace external uses of the top-level mintTabId/nextTabId with the new instance-scoped mintTabId returned or used by functions like createWorkspaceActions.packages/core/src/types.ts (1)
203-291: LGTM — entry/exit type graph is cohesive and well-documented.
ExitFn+ExitOutputArgcorrectly make the payload argument optional for void exits while keeping it required for schemas with a declared output, and theallowBacktri-state onModuleEntryPointlines up with the rollback behavior tested in the runtime suite.Optional nit:
TExits extends ExitPointMap = {}works but reads a bit oddly — the{}default is valid only becauseExitPointMapusesRecord<string, …>. UsingExitPointMapas the default would be marginally clearer:Optional polish
-export interface ModuleEntryProps<TInput, TExits extends ExitPointMap = {}> { +export interface ModuleEntryProps<TInput, TExits extends ExitPointMap = ExitPointMap> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/types.ts` around lines 203 - 291, The default generic for TExits is currently an empty object (TExits extends ExitPointMap = {}) which relies on ExitPointMap being a Record and is slightly unclear; change the default to use ExitPointMap directly so it reads TExits extends ExitPointMap = ExitPointMap. Update the generic default in ModuleEntryProps<TInput, TExits extends ExitPointMap = ExitPointMap> and the related ExitFn<TExits extends ExitPointMap = ...> signature(s) to use ExitPointMap as the default to make intent clearer.packages/journeys/src/runtime.test.ts (2)
439-443: Comment mismatches the test.The comment describes "three transitions rapid-fire," but the test only fires a single
cancelledexit (plus the implicit start). Tightening this avoids future confusion when someone tries to reconcile the comment with the assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.test.ts` around lines 439 - 443, The test comment incorrectly says "three transitions rapid-fire" while the code only triggers an implicit "start" and a single explicit exit("cancelled") via internals.__bindStepCallbacks(internals.__getRecord(id)!, reg).exit("cancelled"); update the comment to describe that the test fires a single cancelled transition (after the implicit start) and asserts the runtime coalesces saves so there is at most one in-flight save, or remove the three-transitions wording so the comment matches the actual behavior being tested.
550-550: Use modernvi.fngeneric syntax for vitest 4.1.0.Line 550 uses the legacy pre-1.0
vi.fn<[Args], Return>generic form. Vitest 4.1.0 and later expectvi.fn<FunctionType>()syntax for full type safety:♻️ Proposed fix
- const remove = vi.fn<[string], void>(); + const remove = vi.fn<(k: string) => void>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.test.ts` at line 550, The test mock uses the old vi.fn generic form; update the declaration for the mock named remove to use the modern vitest 4.1+ generic signature (a function type) instead of vi.fn<[string], void>(); locate the remove constant in runtime.test.ts and replace its type parameter with a function type (e.g., a single-argument function taking string and returning void) by using vi.fn< (arg: string) => void >() so the mock is fully type-safe under the new API.packages/react-router-testing/src/render-journey.tsx (1)
38-92: Extract sharedrenderJourneyimplementation into a router-agnostic factory.The function implementations in this file and
packages/tanstack-router-testing/src/render-journey.tsxare identical except for the@react-router-modules/corevs@tanstack-react-modules/coreimport. The interface definitions, themoduleDescriptors/moduleEntriesloop, runtime creation, and render tree wiring are duplicated verbatim.Consider creating a factory in
@modular-react/journeys/testing(or internal utilities) that accepts{ separateDeps, SharedDependenciesContext }and returns the configured renderer, letting each router package provide only the package-specific wiring. This eliminates drift and makes future changes safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router-testing/src/render-journey.tsx` around lines 38 - 92, The renderJourney implementation is duplicated across router packages; extract the shared logic (the dependency separation using separateDeps, the moduleDescriptors/moduleEntries loop, createJourneyRuntime call, runtime.start, and the render tree wiring around JourneyOutlet) into a router-agnostic factory in the testing utilities (e.g., a function that accepts { separateDeps, SharedDependenciesContext } and returns a configured renderJourney). Replace the local renderJourney in both packages to call that factory and only keep router-specific bits (the router-specific SharedDependenciesContext import and any package-specific types), ensuring the factory exposes a generic function signature compatible with renderJourney’s generics and returns { runtime, instanceId } merged with the testing-library result; update references to moduleDescriptors, moduleEntries, createJourneyRuntime, and render usage to use the centralized implementation.packages/journeys/src/types.ts (1)
196-214:SerializedJourney.statusdrops"loading"— confirm intent and document.
JourneyStatusincludes"loading"butSerializedJourney.statusnarrows to"active" | "completed" | "aborted". The runtime'sserialize()coercesloading → activeat line 315 ofruntime.ts. Worth a short doc comment here pointing to that coercion so future hydrate/migration authors don't assume aloadingblob is possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/types.ts` around lines 196 - 214, SerializedJourney.status narrows possible values to "active" | "completed" | "aborted" but JourneyStatus includes "loading", and runtime.serialize() coerces "loading" → "active"; add a short doc comment on the SerializedJourney.status property explaining that "loading" is intentionally omitted because runtime.serialize() converts loading into active (see serialize() in runtime.ts) so serialized blobs will never contain "loading", to help future hydrate/migration authors avoid assuming a "loading" serialized state.packages/journeys/src/runtime.ts (3)
508-512:entryAllowBackModeForStepis a single-line re-export ofentryAllowBackMode.Only referenced once (line 454). Consider inlining or removing the wrapper.
♻️ Proposed cleanup
- const nextMode = entryAllowBackModeForStep(nextStep); + const nextMode = entryAllowBackMode(nextStep); ... - function entryAllowBackModeForStep( - step: JourneyStep | null, - ): "preserve-state" | "rollback" | false { - return entryAllowBackMode(step); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.ts` around lines 508 - 512, The function entryAllowBackModeForStep is a trivial one-line wrapper around entryAllowBackMode and is only used once; remove entryAllowBackModeForStep and update its sole call site to call entryAllowBackMode(step) directly (or if you prefer to keep the alias, export entryAllowBackMode under the desired name and delete the wrapper), ensuring imports/exports are adjusted accordingly and any TypeScript types remain compatible.
125-135: Listener iteration is not safe against listeners that unsubscribe during notify.
record.listeners.forEach-style iteration over aSetwhile a listener's callback callssubscribe's returned unsubscribe (which doeslisteners.delete) is permitted by spec, but a listener that both unsubscribes itself and the last-visited sibling can skip a notification depending on insertion order. For auseSyncExternalStore-driven world this rarely bites, but snapshotting the listener set before iteration is a cheap safety net.♻️ Proposed hardening
- for (const listener of record.listeners) { + for (const listener of [...record.listeners]) { try { listener();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.ts` around lines 125 - 135, The notify function mutates record.listeners while iterating, which can cause some listeners to be skipped if callbacks unsubscribe others; modify notify (operating on InstanceRecord and its record.listeners) to take a stable snapshot of the listeners (e.g., Array.from(record.listeners) or spread into a new array) before the for-loop, then iterate that snapshot and call each listener inside the try/catch so subscriptions/unsubscriptions during notification won’t affect which callbacks are invoked.
779-802:probeLoad'sregparameter is unused.The function body only uses
persistenceandkey. Dropping the unused parameter tightens the API a bit and quiets future lint tightening.♻️ Proposed cleanup
- function probeLoad( - reg: RegisteredJourney, - persistence: JourneyPersistence<unknown>, - key: string, - ): + function probeLoad( + persistence: JourneyPersistence<unknown>, + key: string, + ):(and update the two call sites at lines 846 / around the async branch).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.ts` around lines 779 - 802, The probeLoad function currently declares an unused first parameter reg (type RegisteredJourney); remove that parameter from the probeLoad signature and any related type mention so the function becomes probeLoad(persistence: JourneyPersistence<unknown>, key: string) with the same return union, and update both call sites that currently pass a RegisteredJourney to call probeLoad(persistence, key) instead of probeLoad(reg, persistence, key); also remove any now-unused RegisteredJourney imports and ensure the existing persistence.load usage and error handling are kept unchanged.examples/react-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.ts (1)
147-152:onHydratethrowing is handled, but consider returningnull-equivalent for smoother migration story.
migrateBlobcatches the thrown error and falls through tostartFresh(silently, unlessdebugis on) — so the user's onboarding progress is lost on version bumps with no telemetry. If that's the intent for this example, fine; otherwise, log or emit a hydration event so the shell can surface "your session expired due to an upgrade" rather than silently restarting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.ts` around lines 147 - 152, The onHydrate handler currently throws for unknown blob.version which makes migrateBlob fall back to startFresh silently; change onHydrate (in this file) to return a null-equivalent (e.g., null) instead of throwing when blob.version !== "1.0.0", and also emit or log a hydration/fallback event (or call the existing telemetry/log function) so the shell can surface "session expired due to upgrade" rather than silently losing progress; adjust callers that expect a blob to handle a null return and invoke startFresh only after logging/emitting the event.packages/tanstack-router-runtime/src/types.ts (1)
121-134: Consider extracting theonModuleExitevent shape into a named type.The same
{ moduleId, entry, exit, output, tabId? }event is inlined in bothResolveManifestOptionsandResolvedManifest(and implicitly inApplicationManifestcallers). Extracting aModuleExitEventtype would keep the two signatures in lockstep and also give consumers a name to import for typing their callbacks.♻️ Proposed refactor
+export interface ModuleExitEvent { + readonly moduleId: string; + readonly entry: string; + readonly exit: string; + readonly output: unknown; + readonly tabId?: string; +} + export interface ResolveManifestOptions<...> { ... - onModuleExit?: (event: { - readonly moduleId: string; - readonly entry: string; - readonly exit: string; - readonly output: unknown; - readonly tabId?: string; - }) => void; + onModuleExit?: (event: ModuleExitEvent) => void; }Also applies to: 221-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tanstack-router-runtime/src/types.ts` around lines 121 - 134, Extract the inline event shape used by onModuleExit into a named type (e.g., ModuleExitEvent) and replace the three inline occurrences with that type so signatures stay consistent: create export type ModuleExitEvent = { readonly moduleId: string; readonly entry: string; readonly exit: string; readonly output: unknown; readonly tabId?: string }; then update the onModuleExit property signature in the definitions that reference it (onModuleExit?: (event: ModuleExitEvent) => void) inside ResolveManifestOptions, ResolvedManifest, and any ApplicationManifest callers so they all share the same named type.packages/journeys/src/outlet.tsx (2)
170-178: Nit:children: nullin props alongside a third-arg child is redundant.
createElement's third argument overridesprops.children, sochildren: nullexists only to satisfy theStepErrorBoundaryProps.children: ReactNoderequired type. Markingchildrenoptional on the internal interface would drop the noise here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/outlet.tsx` around lines 170 - 178, The props object passed to createElement includes a redundant children: null because the third argument provides the actual child; update the StepErrorBoundaryProps interface to make children optional (e.g., children?: ReactNode) so callers like the createElement(StepErrorBoundary, { moduleId: step.moduleId, onError: handleError, key: stepKey }, ...) no longer need to pass children: null; change the StepErrorBoundaryProps type definition and any consumers that relied on children being required (referencing StepErrorBoundaryProps and the StepErrorBoundary component) accordingly.
56-73: Explicitruntimecan still beundefinedwithout a provider.If a caller passes
runtime={someMaybeUndefined}plus no provider is mounted, bothruntimePropandcontext?.runtimecan beundefined, and the thrown error says "either pass runtime or mount a provider" — which the author did attempt. Message is fine, but consider also narrowing the prop type toJourneyRuntime(required) when the caller opts into explicit mode. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/outlet.tsx` around lines 56 - 73, The runtime can still be undefined when a caller passes runtime={maybeUndefined} without a provider; to fix this either make the prop required at the type level or add an explicit runtime-prop presence check: update the JourneyOutletProps runtime type to be a non-optional JourneyRuntime (remove undefined from the type) so callers cannot compile with runtime={undefined}, or (if you prefer runtime remain optional) in JourneyOutlet (use runtimeProp and context via useJourneyContext) detect when props has an explicit runtime prop but it is undefined (e.g., check Object.prototype.hasOwnProperty.call(props, 'runtime') && runtimeProp === undefined) and throw a clearer error message indicating the caller passed an explicit undefined runtime; refer to JourneyOutlet, runtimeProp, JourneyOutletProps and useJourneyContext when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/journeys/src/outlet.tsx (2)
174-182: Nit:children: nullin the props object is dead — the explicit child overrides it.
React.createElement(type, config, ...children)ignoresconfig.childrenwhen...childrenis also supplied, so thechildren: nullentry never takes effect. Minor, but it could mislead a reader into thinking both are passed or that the boundary intentionally nulls children. Drop thechildren: nullkey from the props object.Proposed cleanup
return createElement( StepErrorBoundary, - { moduleId: step.moduleId, onError: handleError, key: stepKey, children: null }, + { moduleId: step.moduleId, onError: handleError, key: stepKey }, createElement(StepComponent, { input: step.input, exit, goBack, }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/outlet.tsx` around lines 174 - 182, The props object passed to React.createElement for StepErrorBoundary includes a redundant "children: null" that is ignored because an explicit child element is supplied; update the createElement call that constructs StepErrorBoundary (the one using StepErrorBoundary, { moduleId: step.moduleId, onError: handleError, key: stepKey, children: null }, createElement(StepComponent, {...})) by removing the children: null entry so the props only contain moduleId, onError, and key (leave the inner createElement(StepComponent, { input: step.input, exit, goBack }) as the explicit child).
148-169: Consider exposingincrementRetry()andgetRetryCount()accessors onJourneyRuntimeInternalsfor explicit retry budget management.The outlet directly mutates
record.retryCountalongside the runtime's internal writes (initialization at line 706 in runtime.ts, reset at line 739). WhileretryCountis mutable onInstanceRecord, this creates hidden coupling between the React layer and the runtime's internal state. The comment explaining the design (survives stepToken bumps mid-retry) is good, but the pattern of direct field access across modules makes it harder to evolve this contract later. Exposing dedicated accessor methods would make the coupling explicit and discoverable, improving maintainability without changing functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/outlet.tsx` around lines 148 - 169, Replace direct reads/writes of record.retryCount from the React boundary with explicit runtime accessors: add getRetryCount(instanceId) and incrementRetry(instanceId) (or similarly named methods) to the JourneyRuntimeInternals API and implement them to operate on the InstanceRecord.retryCount; then update handleError in outlet.tsx to call runtime.getRetryCount(instanceId) to compare with retryLimit and runtime.incrementRetry(instanceId) instead of mutating record.retryCount, and update any runtime initialization/reset code that currently touches retryCount directly to use the new accessors so the retry budget is only managed via the runtime API.packages/journeys/src/runtime.test.ts (1)
434-467: Test name slightly oversells what it verifies.The name says "coalesces rapid saves so there is at most one in flight", but the body fires only a single
cancelledexit on top of the start-time save. Sincecancelledis terminal, the subsequent scheduling hitsremovePersisted(deferred bypendingRemove), not the non-terminal "pending save coalesces" branch inschedulePersist. Consider either renaming to reflect what's actually asserted (e.g. "terminal transition while a save is in-flight doesn't spawn a second save") or adding a non-terminal intermediate transition to exercisependingSaveoverwrite semantics. Coverage of the rejected-save coalesce path is already handled at lines 804-844.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.test.ts` around lines 434 - 467, The test title overstates behavior: it only fires a terminal exit("cancelled") on top of the start-time save, so the runtime takes the removePersisted/pendingRemove path instead of exercising schedulePersist's non-terminal pendingSave overwrite/coalesce logic; update the test to either rename it to something like "terminal transition while a save is in-flight doesn't spawn a second save" or change the sequence of transitions invoked via internals.__bindStepCallbacks(...).exit(...) to include a non-terminal transition (e.g., trigger a non-terminal state transition before the final terminal one) so schedulePersist's pendingSave overwrite path (referenced by schedulePersist, pendingSave, pendingRemove, and removePersisted) is actually executed and asserted.packages/journeys/src/types.ts (1)
244-262: Minor:JourneyRegisterOptions.onTransitiondrops the generics that the definition-level hook carries.
JourneyDefinition.onTransitionis(ev: TransitionEvent<TModules, TState>) => void, but the registration-level hook here is(ev: TransitionEvent) => void(defaults). Shell-level analytics lose the typedstate/history/step narrowing that journey authors get for free. Consider parameterizing:Proposed typing
export interface JourneyRegisterOptions<TState = unknown> { - onTransition?: (ev: TransitionEvent) => void; + onTransition?: (ev: TransitionEvent<ModuleTypeMap, TState>) => void; persistence?: JourneyPersistence<TState>;Not a blocker — shells typically treat registration hooks as type-erased — but it's cheap to recover.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/types.ts` around lines 244 - 262, JourneyRegisterOptions currently loses generic typing for the onTransition hook; update the type parameters and hook signature so registration preserves module+state types: add a TModules generic (e.g. JourneyRegisterOptions<TState = unknown, TModules = unknown>) and change onTransition to accept a typed TransitionEvent (onTransition?: (ev: TransitionEvent<TModules, TState>) => void) so it matches JourneyDefinition.onTransition and preserves state/history/step narrowing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/journeys/src/runtime.test.ts`:
- Around line 567-591: Replace the deprecated Vitest 1 signature vi.fn<[string],
void>() with the new single function-type form; in this test change the mock
declaration to vi.fn<(k: string) => void>() (or vi.fn<(arg: string) => void>())
so the remove mock has the correct typed function signature referenced in the
test (the remove constant and its expectation remain unchanged).
---
Nitpick comments:
In `@packages/journeys/src/outlet.tsx`:
- Around line 174-182: The props object passed to React.createElement for
StepErrorBoundary includes a redundant "children: null" that is ignored because
an explicit child element is supplied; update the createElement call that
constructs StepErrorBoundary (the one using StepErrorBoundary, { moduleId:
step.moduleId, onError: handleError, key: stepKey, children: null },
createElement(StepComponent, {...})) by removing the children: null entry so the
props only contain moduleId, onError, and key (leave the inner
createElement(StepComponent, { input: step.input, exit, goBack }) as the
explicit child).
- Around line 148-169: Replace direct reads/writes of record.retryCount from the
React boundary with explicit runtime accessors: add getRetryCount(instanceId)
and incrementRetry(instanceId) (or similarly named methods) to the
JourneyRuntimeInternals API and implement them to operate on the
InstanceRecord.retryCount; then update handleError in outlet.tsx to call
runtime.getRetryCount(instanceId) to compare with retryLimit and
runtime.incrementRetry(instanceId) instead of mutating record.retryCount, and
update any runtime initialization/reset code that currently touches retryCount
directly to use the new accessors so the retry budget is only managed via the
runtime API.
In `@packages/journeys/src/runtime.test.ts`:
- Around line 434-467: The test title overstates behavior: it only fires a
terminal exit("cancelled") on top of the start-time save, so the runtime takes
the removePersisted/pendingRemove path instead of exercising schedulePersist's
non-terminal pendingSave overwrite/coalesce logic; update the test to either
rename it to something like "terminal transition while a save is in-flight
doesn't spawn a second save" or change the sequence of transitions invoked via
internals.__bindStepCallbacks(...).exit(...) to include a non-terminal
transition (e.g., trigger a non-terminal state transition before the final
terminal one) so schedulePersist's pendingSave overwrite path (referenced by
schedulePersist, pendingSave, pendingRemove, and removePersisted) is actually
executed and asserted.
In `@packages/journeys/src/types.ts`:
- Around line 244-262: JourneyRegisterOptions currently loses generic typing for
the onTransition hook; update the type parameters and hook signature so
registration preserves module+state types: add a TModules generic (e.g.
JourneyRegisterOptions<TState = unknown, TModules = unknown>) and change
onTransition to accept a typed TransitionEvent (onTransition?: (ev:
TransitionEvent<TModules, TState>) => void) so it matches
JourneyDefinition.onTransition and preserves state/history/step narrowing.
🪄 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: 193da361-b4a1-4485-aaa3-f78f15018e29
📒 Files selected for processing (19)
examples/react-router/customer-onboarding-journey/modules/billing/src/StartTrial.tsxexamples/react-router/customer-onboarding-journey/shell/src/main.tsxexamples/react-router/customer-onboarding-journey/shell/src/stores/workspace-tabs.tsexamples/tanstack-router/customer-onboarding-journey/README.mdexamples/tanstack-router/customer-onboarding-journey/modules/billing/src/StartTrial.tsxexamples/tanstack-router/customer-onboarding-journey/shell/package.jsonexamples/tanstack-router/customer-onboarding-journey/shell/src/components/Shell.tsxexamples/tanstack-router/customer-onboarding-journey/shell/src/main.tsxexamples/tanstack-router/customer-onboarding-journey/shell/src/stores/workspace-tabs.tspackages/journeys/README.mdpackages/journeys/src/module-tab.test.tsxpackages/journeys/src/module-tab.tsxpackages/journeys/src/outlet.test.tsxpackages/journeys/src/outlet.tsxpackages/journeys/src/provider.test.tsxpackages/journeys/src/runtime.test.tspackages/journeys/src/runtime.tspackages/journeys/src/simulate-journey.tspackages/journeys/src/types.ts
✅ Files skipped from review due to trivial changes (3)
- examples/tanstack-router/customer-onboarding-journey/shell/package.json
- examples/tanstack-router/customer-onboarding-journey/README.md
- examples/tanstack-router/customer-onboarding-journey/shell/src/stores/workspace-tabs.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- examples/react-router/customer-onboarding-journey/shell/src/stores/workspace-tabs.ts
- examples/tanstack-router/customer-onboarding-journey/shell/src/components/Shell.tsx
- examples/tanstack-router/customer-onboarding-journey/modules/billing/src/StartTrial.tsx
- packages/journeys/src/module-tab.tsx
- examples/react-router/customer-onboarding-journey/shell/src/main.tsx
- packages/journeys/src/outlet.test.tsx
- packages/journeys/src/runtime.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/journeys/src/runtime.test.ts (3)
434-467: Test gap: coalesced save is never observed to actually flush.The test only asserts
saves.length === 1while the first save is paused, then releases it and awaits microtasks without re-checking. If the coalescing logic regresses such that the second save is dropped entirely (e.g., pending flag cleared prematurely), this test would still pass. Add a post-release assertion that the coalesced save ran to lock in the intended behavior.♻️ Proposed tightening
resolveFirst(); await Promise.resolve(); await Promise.resolve(); + // Coalesced terminal save must flush once the first save settles. + expect(saves.length).toBe(2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.test.ts` around lines 434 - 467, The test currently only asserts saves.length === 1 while the first save is paused and never verifies the coalesced save actually flushed; after calling resolveFirst() and awaiting microtasks (the existing await Promise.resolve() lines), add a follow-up assertion that the coalesced save ran (e.g., expect(saves.length).toBe(2) or assert the final saved timestamp changed) so the test ensures the second/coalesced save was executed; update the test around the resolveFirst()/await Promise.resolve() section in the "coalesces rapid saves so there is at most one in flight" case to perform this post-release check using the existing saves array and resolveFirst/firstDone helpers.
668-692: Unnecessaryasyncon a fully synchronous test.The test body contains no
await. Dropasyncto avoid implying an async contract.✏️ Proposed edit
- it("keyFor collisions across journeys do not alias onto the same instance", async () => { + it("keyFor collisions across journeys do not alias onto the same instance", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.test.ts` around lines 668 - 692, The test "keyFor collisions across journeys do not alias onto the same instance" is declared async but contains no await; remove the unnecessary async keyword from the it(...) callback to avoid implying an async contract—edit the test function where it's defined (the it(...) block for that description in runtime.test.ts) and change the callback from async () => { ... } to () => { ... } so the test runs synchronously without an unused async wrapper.
457-461: Comment claims "three transitions" but only two occur.The sequence is
start+cancelled(two saves), not three. Small wording cleanup.✏️ Proposed edit
- // Fire three transitions rapid-fire — cancelled completes. Sequence: - // start (save queued), cancelled (save queued & coalesces on top of the - // first in-flight save). The runtime must have at most one save in - // flight at a time. + // Sequence: start (save queued), cancelled (save queued & coalesces + // on top of the first in-flight save). The runtime must have at most + // one save in flight at a time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.test.ts` around lines 457 - 461, The comment incorrectly says "Fire three transitions rapid-fire" but the code only triggers two transitions (start then cancelled); update the comment near internals.__bindStepCallbacks(internals.__getRecord(id)!, reg).exit("cancelled") to describe two transitions (e.g., "Fire two transitions rapid‑fire — cancelled completes" or similar), and adjust the following sentence to reflect "two saves" instead of "three" so the description matches the sequence produced by __bindStepCallbacks and __getRecord exit("cancelled").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/journeys/src/runtime.test.ts`:
- Around line 716-739: The current assertion in the test for hydrate() is too
broad (matching /version mismatch/) and masks the real cause when onHydrate
throws; update the expectation to assert the wrapped error more precisely—for
example call rt.hydrate("collect", blob) inside a throws assertion that either
checks the thrown value is an instance of JourneyHydrationError or that the
error message (or error.cause message) contains the original "migration bailed"
string coming from onHydrate; locate the test block using onHydrate, rt.hydrate,
and migrateBlob and replace the expect(...).toThrow(/version mismatch/) with an
assertion matching the specific "migration bailed" substring or asserting
JourneyHydrationError + cause contains that message.
---
Nitpick comments:
In `@packages/journeys/src/runtime.test.ts`:
- Around line 434-467: The test currently only asserts saves.length === 1 while
the first save is paused and never verifies the coalesced save actually flushed;
after calling resolveFirst() and awaiting microtasks (the existing await
Promise.resolve() lines), add a follow-up assertion that the coalesced save ran
(e.g., expect(saves.length).toBe(2) or assert the final saved timestamp changed)
so the test ensures the second/coalesced save was executed; update the test
around the resolveFirst()/await Promise.resolve() section in the "coalesces
rapid saves so there is at most one in flight" case to perform this post-release
check using the existing saves array and resolveFirst/firstDone helpers.
- Around line 668-692: The test "keyFor collisions across journeys do not alias
onto the same instance" is declared async but contains no await; remove the
unnecessary async keyword from the it(...) callback to avoid implying an async
contract—edit the test function where it's defined (the it(...) block for that
description in runtime.test.ts) and change the callback from async () => { ... }
to () => { ... } so the test runs synchronously without an unused async wrapper.
- Around line 457-461: The comment incorrectly says "Fire three transitions
rapid-fire" but the code only triggers two transitions (start then cancelled);
update the comment near
internals.__bindStepCallbacks(internals.__getRecord(id)!, reg).exit("cancelled")
to describe two transitions (e.g., "Fire two transitions rapid‑fire — cancelled
completes" or similar), and adjust the following sentence to reflect "two saves"
instead of "three" so the description matches the sequence produced by
__bindStepCallbacks and __getRecord exit("cancelled").
🪄 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: 5129c09b-4022-4d28-a970-8c8b65829414
📒 Files selected for processing (2)
packages/journeys/src/outlet.test.tsxpackages/journeys/src/runtime.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/journeys/src/outlet.test.tsx
|
@kibertoad ran into a runtime error:
Renders the app unusable: Fix is after: & |
Add a lightweight handle token (`JourneyHandle`, `defineJourneyHandle`) so modules and shells open journeys with typed `input` without importing the journey's runtime code. `JourneyRuntime.start` gains a handle-first overload; the string-id form still works. - core: `JourneyHandleRef` + the handle overload on `JourneyRuntime.start` - journeys: `handle.ts` / `handle.test.ts`, `start` impl accepts either form, re-exports the handle from the package index - examples: journeys export `customerOnboardingHandle`; Home screens use the handle at `runtime.start(...)` call sites Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend `JourneyRegisterOptions` with shell-level hooks so analytics and telemetry live at registration rather than in journey definitions: - `onComplete` / `onAbort` fire sequentially after the definition hook (both run). Matches the existing `onTransition` layering. - `onAbandon` overrides the definition's handler when set — only one can drive the transition; fallback to definition when absent. - `onHydrate` layers on top of the definition's. Either hook counts as explicit migration (skips the version-mismatch check). - `onError` observes every step-component throw and every transition handler throw. Observation-only; the outlet's `onStepError` policy still decides retry / ignore / abort. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce `createTestHarness(runtime)` on `@modular-react/journeys/testing` as the public way to drive a runtime in tests — `fireExit`, `goBack`, `inspect`. Replaces direct use of the `__`-prefixed runtime internals by external callers. - `testing.ts` rewritten from stub to harness implementation. - `simulate-journey.ts` migrated onto the harness; public `JourneySimulator` contract unchanged. - `getInternals` dropped from the package's public index. The symbol still lives on `./runtime.js` for in-package callers (the outlet, the harness itself) until the in-package tests migrate; that's the next incremental step, not blocking. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the hardcoded red divs in `<JourneyOutlet>` with two optional
component props. Omitting them preserves today's visuals.
- `notFoundComponent`: rendered when the current step points at an
unregistered module/entry. Receives `{ moduleId, entry }`.
- `errorComponent`: rendered when a step component throws. Receives
`{ moduleId, error }`, giving shells the raw error for their own
reporting. Threaded through `StepErrorBoundary`.
Defaults extracted as `DefaultNotFound` / `DefaultError`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Export `isTerminal(instance)` from `@modular-react/core` for the one predicate that captures a non-trivial disjunction over `JourneyStatus` (`completed` or `aborted`). `isActive` and `isLoading` would wrap a single string comparison on a typed union and are intentionally not shipped — the JSDoc says so. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-router-runtime/src/registry.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorPipeline:
oxfmtformatting failure reported in CI.Please rerun the formatter and commit the result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router-runtime/src/registry.ts` at line 1, The file packages/react-router-runtime/src/registry.ts failed the oxfmt CI check; re-run the oxfmt formatter locally and commit the updated file so the import line (import { createBrowserRouter, createMemoryRouter } from "react-router";) and the rest of registry.ts are properly formatted; ensure you run the same oxfmt command used by CI and add/commit the formatted changes before pushing.
♻️ Duplicate comments (1)
packages/journeys/src/runtime.ts (1)
1044-1048:⚠️ Potential issue | 🟡 Minor
hydrate()leaks a half-built instance whenhydrateIntothrows.
instances.set(instanceId, record)happens beforehydrateInto(...), which can throwJourneyHydrationErrorforrollbackSnapshots/historylength mismatches (line 774‑777). When it throws, the error propagates to the caller, but astatus: "loading"record with no listeners is left behind in the map — it surfaces inlistInstances()/getInstance(id)forever, andforget(id)refuses to drop it because it's not terminal. Thestart()paths already handle this (sync path at 980‑988 rolls back, async path at 937‑945 via theif (record.status !== "loading") returnguard +startFreshfallback), sohydrate()is the odd one out.🛡️ Proposed fix
const record = createRecord(reg, instanceId, null, migrated.blob.state); instances.set(instanceId, record); - hydrateInto(record, migrated.blob); - notify(record); - return instanceId; + try { + hydrateInto(record, migrated.blob); + } catch (err) { + instances.delete(instanceId); + throw err; + } + notify(record); + return instanceId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.ts` around lines 1044 - 1048, hydrate() currently inserts a partially-built record into the instances map before calling hydrateInto, which can throw (e.g., JourneyHydrationError on rollbackSnapshots/history mismatch) and leaves a non-terminal "loading" record behind; fix by ensuring the instance is not retained on failure — either move instances.set(instanceId, record) until after hydrateInto/notify complete or wrap hydrateInto(...) and notify(...) in a try/catch that calls instances.delete(instanceId) before rethrowing the error (referencing hydrate(), hydrateInto, createRecord, instances, notify, and the JourneyHydrationError/rollbackSnapshots failure path).
🧹 Nitpick comments (22)
examples/react-router/customer-onboarding-journey/shell/e2e/smoke.spec.ts (1)
64-64: Consider replacing the hard wait with an event-based settle.
waitForTimeout(250)is flaky-by-design: if a runtime error surfaces at 260ms (e.g. a lazy module import resolving late) the test passes despite the regression.await page.waitForLoadState("networkidle")(or an explicitexpect(...)on the first step's content) gives a deterministic flush point while still letting microtask errors propagate. Same note applies to the tanstack sibling spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react-router/customer-onboarding-journey/shell/e2e/smoke.spec.ts` at line 64, Replace the flaky hard wait (the call to page.waitForTimeout(250)) with an event-based settle so microtask/runtime errors reliably surface; specifically, use an explicit load/network/DOM assertion such as awaiting page.waitForLoadState("networkidle") or asserting the first step's content with an expect(...) after navigation in the smoke.spec.ts test (locate the page.waitForTimeout call) to provide a deterministic flush point instead of the fixed 250ms pause..github/workflows/ci.yml (1)
124-200: Consider lifting theany_changedguard to job level to avoid per-step repetition.Seven steps each carry
if: steps.changes.outputs.any_changed == 'true'. A new step added without the guard will run on every PR, including ones that shouldn't trigger e2e at all — a silent regression of the gating logic. Two cleaner shapes:
- Move the change-detection to a dedicated
detect-e2ejob and gateexamples-e2ewith a single job-levelif:.- Keep detection inline but make it a hard exit, e.g. an early step that sets a job-level output and subsequent steps check
needs.Option (1) also avoids running
tj-actions/changed-filesonce per matrix leg. Non-blocking, but worth doing while the blast radius is small.Sketch of option 1
examples-e2e-detect: runs-on: ubuntu-latest outputs: rr_changed: ${{ steps.rr.outputs.any_changed }} tsr_changed: ${{ steps.tsr.outputs.any_changed }} steps: - uses: actions/checkout@v6 - id: rr uses: tj-actions/changed-files@7dee1b0c1557f278e5c7dc244927139d78c0e22a with: files: | examples/react-router/customer-onboarding-journey/** packages/react-router-runtime/** packages/react-router-core/** packages/journeys/** packages/react/** packages/core/** .github/workflows/ci.yml files_ignore: | **/*.md **/*.test.ts **/*.test.tsx # ...similar `tsr` step... examples-e2e: needs: [examples-e2e-detect] if: needs.examples-e2e-detect.outputs.rr_changed == 'true' || needs.examples-e2e-detect.outputs.tsr_changed == 'true' strategy: fail-fast: false matrix: include: [...] steps: # no per-step `if:` needed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 124 - 200, The job currently repeats per-step guards using steps.changes.outputs.any_changed inside the examples-e2e job; instead create a dedicated detect job (e.g., examples-e2e-detect) that runs tj-actions/changed-files once and exposes outputs (like rr_changed/tsr_changed), then change the examples-e2e job to depend on that job (needs: examples-e2e-detect) and add a single job-level if that checks the detect outputs (e.g., if: needs.examples-e2e-detect.outputs.rr_changed == 'true' || needs.examples-e2e-detect.outputs.tsr_changed == 'true'), removing the per-step if: steps.changes.outputs.any_changed == 'true' guards so steps run only when the detection indicates changes.examples/react-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.ts (1)
30-32: Extract the journey version to a single constant to prevent hydrate drift.The literal
"1.0.0"is declared at line 32 (journeyversion) and independently checked at line 148 (blob.version !== "1.0.0"). When the journey version is bumped, it's easy to update one and forget the other — the result is either a silent acceptance of stale blobs under a new version or a spurious hydrate failure after a coordinated bump. Since referencingcustomerOnboardingJourney.versioninside its own definition would be circular, extract a const and use it in both places.♻️ Proposed refactor
+const CUSTOMER_ONBOARDING_VERSION = "1.0.0"; + export const customerOnboardingJourney = defineJourney<OnboardingModules, OnboardingState>()({ id: "customer-onboarding", - version: "1.0.0", + version: CUSTOMER_ONBOARDING_VERSION, meta: { @@ onHydrate: (blob) => { - if (blob.version !== "1.0.0") { + if (blob.version !== CUSTOMER_ONBOARDING_VERSION) { throw new Error(`Unknown customer-onboarding journey version: ${blob.version}`); } return blob; }, });Also applies to: 147-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.ts` around lines 30 - 32, Extract the journey version string into a single constant (e.g., const CUSTOMER_ONBOARDING_JOURNEY_VERSION = "1.0.0") and use that constant both when constructing the journey with defineJourney (replace the literal in customerOnboardingJourney's version) and when checking blobs (replace the literal in the blob.version !== ... comparison); do not reference customerOnboardingJourney.version inside its own definition to avoid a circular reference.packages/journeys/src/handle.test.ts (2)
67-77: Minor:as nevercasts in the persistence adapter.Using
as neveron the return ofloadand on the wholepersistenceobject bypasses type checking — if the adapter shape drifts fromJourneyPersistence<TState>, this test won't notice. Annotating the adapter with the real type costs nothing and gives the test the same coverage the production code gets.♻️ Proposed fix
- const store = new Map<string, unknown>(); - const persistence = { - keyFor: ({ input }: { input: unknown }) => `demo:${(input as Input).id}`, - load: (key: string) => (store.get(key) ?? null) as never, - save: (key: string, blob: unknown) => void store.set(key, blob), - remove: (key: string) => void store.delete(key), - }; - const rt = createJourneyRuntime( - [{ definition: journey, options: { persistence: persistence as never } }], - { modules: { m: mod }, debug: false }, - ); + const store = new Map<string, SerializedJourney<{ id: string }>>(); + const persistence: JourneyPersistence<{ id: string }> = { + keyFor: ({ input }) => `demo:${(input as Input).id}`, + load: (key) => store.get(key) ?? null, + save: (key, blob) => void store.set(key, blob), + remove: (key) => void store.delete(key), + }; + const rt = createJourneyRuntime( + [{ definition: journey, options: { persistence } }], + { modules: { m: mod }, debug: false }, + );(Requires importing
JourneyPersistenceandSerializedJourneyfrom@modular-react/coreor./types.js.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/handle.test.ts` around lines 67 - 77, The test uses unsafe casts "as never" on the persistence adapter (properties load/save/remove and the whole persistence object) which hides type mismatches; replace the casts by typing the adapter with the proper interface (e.g., JourneyPersistence<SerializedJourney> or the correct JourneyPersistence<TState> type) so the persistence constant and its load/save/remove signatures are type-checked; import JourneyPersistence and SerializedJourney (or the appropriate types) and annotate the persistence variable accordingly before passing it into createJourneyRuntime.
84-96: Test relies ongetInternals, which the PR is migrating off the public API.Per the PR's commit summary,
createTestHarness(runtime)is the new public test harness andgetInternalswas removed from the public API. This third test asserts an implementation detail (internals.__getRegistered("demo")) that the public surface no longer exposes, so it will become brittle ifgetInternalsmoves further behind a wall. Consider asserting behavior throughrt.listDefinitions()(or the new harness) instead — the observable claim ("handle resolves the registered definition the same way a string id does") is already fully exercised by the persistence idempotency test above.♻️ Proposed replacement
- it("surfaces the handle's id in the step that hydrates the journey", () => { - // Smoke test: the runtime's internals resolve the registered definition - // via the same path whether we pass a string or a handle. - const rt = createJourneyRuntime([{ definition: journey, options: undefined }], { - modules: { m: mod }, - debug: false, - }); - const handle = defineJourneyHandle(journey); - const id = rt.start(handle, { id: "x" }); - const internals = getInternals(rt); - expect(internals.__getRegistered("demo")).toBeDefined(); - expect(rt.getInstance(id)!.journeyId).toBe("demo"); - }); + it("resolves to the same registered definition as the string-id form", () => { + const rt = createJourneyRuntime([{ definition: journey, options: undefined }], { + modules: { m: mod }, + debug: false, + }); + const handle = defineJourneyHandle(journey); + const id = rt.start(handle, { id: "x" }); + expect(rt.listDefinitions().some((d) => d.id === "demo")).toBe(true); + expect(rt.getInstance(id)!.journeyId).toBe("demo"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/handle.test.ts` around lines 84 - 96, The test currently depends on the removed public helper getInternals; update it to assert via the public test harness/API instead: stop calling getInternals and replace the internal assertion with a check using createTestHarness(createJourneyRuntime(...)) or rt.listDefinitions() and the existing public methods (e.g., defineJourneyHandle, rt.start, rt.getInstance) to verify that the handle resolves to the same journey id ("demo"); ensure the test still creates the same runtime (createJourneyRuntime) and handle (defineJourneyHandle) but uses the public listing/harness API rather than internals.__getRegistered.packages/journeys/src/handle.ts (1)
23-29:defineJourneyHandlereturnsJourneyHandle<string, TInput>rather than narrowingTIdto the literal id.As written, consumers lose the literal-type information about which journey the handle refers to —
handle.idtypes as plainstring. Threading aTId extends stringgeneric throughJourneyDefinitionanddefineJourneywould let callers getJourneyHandle<"customer-onboarding", Input>back, enabling typed lookups / registries. Not blocking for the current example, but worth considering before the API surface is fully public.♻️ Sketch (requires corresponding widening of `JourneyDefinition` and `defineJourney`)
export function defineJourneyHandle< + TId extends string, TModules extends ModuleTypeMap, TState, TInput, ->(def: JourneyDefinition<TModules, TState, TInput>): JourneyHandle<string, TInput> { - return { id: def.id }; +>(def: JourneyDefinition<TModules, TState, TInput> & { readonly id: TId }): JourneyHandle<TId, TInput> { + return { id: def.id }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/handle.ts` around lines 23 - 29, defineJourneyHandle currently widens the id to string; change its generic to carry a literal id type (e.g. add TId extends string) and return JourneyHandle<TId, TInput> instead of JourneyHandle<string, TInput>, and adjust the JourneyDefinition type parameterization (and any defineJourney helper) to expose that TId so the compiler can infer the literal type from def.id; specifically update defineJourneyHandle signature to accept JourneyDefinition<TModules, TState, TInput, TId> (or equivalent) and return { id: def.id } typed as JourneyHandle<TId, TInput> so callers preserve the literal id type.packages/core/src/journey-contracts.ts (2)
115-121: Potential naming collision:allowBacklives in the same object as exit-name keys.
EntryTransitionsis built as a mapped type overExitNamesOf<TMod>intersected with{ readonly allowBack?: boolean }. If a module ever declares an exit namedallowBack, the type of that key becomesboolean & ((ctx) => TransitionResult)— effectively unusable and confusing at the call site. Consider either reserving the name explicitly in documentation, renaming the metadata key (e.g.$allowBack), or nesting transitions under a dedicated key ({ transitions: {...}, allowBack?: boolean }) to keep the namespaces fully separate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/journey-contracts.ts` around lines 115 - 121, EntryTransitions currently merges mapped exit keys (ExitNamesOf<TMod>) with a metadata key allowBack, causing a type collision if an exit name is "allowBack"; update the type to separate metadata from transitions to avoid name clashes by replacing EntryTransitions with a shape like { transitions: { [X in ExitNamesOf<TMod>]?: (ctx: ExitCtx<TState, ExitOutputOf<TMod, X>, EntryInputOf<TMod, TEntry>>) => TransitionResult<TModules, TState> }, allowBack?: boolean } so the mapped keys live under transitions and allowBack remains a distinct optional property; update any code that consumes EntryTransitions/EntryTransitions.* to access the nested transitions and the allowBack flag accordingly.
259-290: Overload ordering forstartlooks correct, but verify handle/string disambiguation at call sites.The handle-first overload takes
JourneyHandleRef<TId, TInput>(an object); the string-id overload takesstring. TS overload resolution is top-down, so a string argument will correctly skip the handle overload and bind to the second. However, if anyone callsrt.start({ id: "demo" }, input)with a plain literal that matchesJourneyHandleRefstructurally but was intended as a string-id path,TInputon that call site will be inferred asunknown(since the handle has no phantom marker) — silently widening the input check. Consider brandingJourneyHandleRefwith a unique symbol or tag to prevent accidental structural matches from erasing input typing.♻️ Proposed brand
-export interface JourneyHandleRef<TId extends string = string, TInput = unknown> { - readonly id: TId; - readonly __input?: TInput; -} +declare const journeyHandleBrand: unique symbol; +export interface JourneyHandleRef<TId extends string = string, TInput = unknown> { + readonly id: TId; + readonly __input?: TInput; + readonly [journeyHandleBrand]?: true; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/journey-contracts.ts` around lines 259 - 290, The overload ambiguity comes from JourneyHandleRef being structurally compatible with plain objects, which can cause accidental matches to the handle-first start<TId,TInput>(handle, input) overload and widen TInput; fix this by branding JourneyHandleRef<TId, TInput> with a unique nominal tag (e.g. a private/unique symbol property or a readonly phantom field) so plain object literals won't structurally match it, update the JourneyHandleRef type declaration to include that brand, and update any handle factory/constructor functions to produce values with the brand (and adjust places that construct handles manually) so runtime call sites using rt.start(...) keep correct overload resolution and input typing.packages/journeys/src/types.ts (1)
94-101:onAbandonat registration level cannot return a type-checkednextstep.
JourneyRegisterOptions.onAbandonis parameterized only byTState, soTransitionResult<ModuleTypeMap, TState>is evaluated against the baseModuleTypeMaprather than the specific journey's module map. A handler that returns{ next }will effectively have an untyped step spec. In practice, shell-level abandon handlers almost always return{ abort }or{ complete }, so this is likely fine — but worth documenting the limitation so authors don't reach for{ next }here expecting typed entry inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/types.ts` around lines 94 - 101, The onAbandon handler is currently typed against the global ModuleTypeMap which prevents returning a type-checked `{ next }` for a specific journey; update the JourneyRegisterOptions generic so it carries the journey-specific module map (e.g., add a generic parameter like M extends ModuleTypeMap) and change the onAbandon signature to use AbandonCtx<M, TState> => TransitionResult<M, TState> (ensure any uses of AbandonCtx, TransitionResult, and the JourneyRegisterOptions type are updated to propagate the same M generic) so onAbandon can return journey-typed next steps instead of the base ModuleTypeMap.packages/tanstack-router-runtime/src/registry.ts (2)
443-447: Manifest'sjourneysfield is implicitly coupled to a plugin named exactly"journeys".Both
resolve()andresolveManifest()readassembly.extensions.journeysto populatemanifest.journeys. If the journeys plugin is ever registered under a differentname(or a user swaps it for an alternate implementation with a different name),manifest.journeyswill silently beundefinedeven thoughmanifest.extensions["other-name"]holds the runtime. Consider either documenting"journeys"as a reserved plugin name or exposing a well-known symbol/constant that the plugin and registry both import from core.Also applies to: 507-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tanstack-router-runtime/src/registry.ts` around lines 443 - 447, The code currently hardcodes the "journeys" extension key (e.g. reading assembly.extensions.journeys in resolve() and resolveManifest() to set manifest.journeys), which couples the feature to a literal plugin name; export a single well-known constant/symbol (e.g. JOURNEYS_EXTENSION or a Symbol('journeys')) from core and use that symbol when registering and when reading extensions so both plugin implementations and the registry reference the same identifier; update all occurrences (assembly.extensions.journeys, manifest.journeys assignment sites, and the other spots mentioned around lines 507–511) to use that exported constant/symbol so the manifest correctly picks up the runtime regardless of plugin.name.
246-248:plugin.validate?.runs insidebuildAssembly, soresolveManifest()retries will call it again.
resolveManifest()is documented as "commit to first-call semantics but allow retry after a failedbuildAssembly" (see the comments aroundfirstCallCompleted). If a plugin'svalidatehas observable side-effects (logging, registering, throwing non-idempotently), a first-call failure followed by a retry will double-invoke it. TheonRegisterhook above is guarded byonRegisterRanfor exactly this reason — consider the same guard forvalidate, or document thatvalidatemust be a pure predicate with no side-effects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tanstack-router-runtime/src/registry.ts` around lines 246 - 248, The plugin.validate call inside buildAssembly can be invoked twice when resolveManifest retries; add a guard similar to onRegister/onRegisterRan to ensure validate is only run once per plugin (or once globally) on first successful invocation: introduce a boolean like validateRan (or a per-plugin flag) and wrap plugin.validate?.({ modules }) with if (!validateRan) { validateRan = true; plugin.validate?.({ modules }) } so retries won't re-run non-idempotent validate logic; reference buildAssembly, resolveManifest, plugin.validate, firstCallCompleted, onRegister and onRegisterRan when locating where to add the guard.examples/tanstack-router/customer-onboarding-journey/app-shared/src/index.ts (1)
26-29: Thereadonly never[]pattern is valid but unusually explicit.
readonly never[]satisfies the framework'sSlotMapOf<T>constraint (which requires each slot to be areadonly unknown[]), and the intent is already documented in the comment. However, this pattern is unique in the codebase — all otherAppSlotsdeclarations assign concrete array types. For consistency and simplicity, consider using an empty interface instead:export interface AppSlots { // Intentionally empty — this example renders exclusively via journey tabs. }An empty interface is equally valid, more idiomatic, and communicates the same intent without the unusual type syntax.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/app-shared/src/index.ts` around lines 26 - 29, The AppSlots interface uses the unusual readonly never[] pattern; update the declaration of AppSlots (the interface named AppSlots) to be an empty interface (remove the readonly never[] property and keep the comment) so it remains compatible with SlotMapOf<T> while matching the project's idiomatic style and other AppSlots declarations.packages/journeys/src/outlet.test.tsx (1)
337-341: Tighten the retry-count assertion.
retryLimit={1}with an initial render + one retry before abort meansonStepErrorfires exactly twice (initial throw+retry throw). The loose>= 2assertion will mask regressions that burst retries beyond the limit (e.g. off-by-one inrecord.retryCountaccounting). Prefer an exact count so the retry-budget invariant stays locked down.♻️ Proposed fix
- expect(onStepError.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(onStepError).toHaveBeenCalledTimes(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/outlet.test.tsx` around lines 337 - 341, The test currently asserts expect(onStepError.mock.calls.length).toBeGreaterThanOrEqual(2) which is too loose for retryLimit={1}; change this to assert the exact number of expected error callbacks (expect(onStepError.mock.calls.length).toBe(2)) so that the sequence (initial throw + one retry throw -> aborted) is enforced; update the assertion near the block that checks rt.getInstance(id)!.status === "aborted" and after restoreError.mockRestore() to validate exactly two onStepError invocations.examples/tanstack-router/customer-onboarding-journey/shell/src/components/Home.tsx (1)
52-52: UsecustomerOnboardingHandle.idfor consistency.Line 35 keys the tab by
customerOnboardingHandle.id, but Line 52 hardcodes"customer-onboarding"for the persistence lookup. If the handle'sidis ever renamed, the "Resume" label will silently desync from the actual tab/persistence key.♻️ Proposed fix
- const resuming = hasPersistedJourney("customer-onboarding", customer.id); + const resuming = hasPersistedJourney(customerOnboardingHandle.id, customer.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/shell/src/components/Home.tsx` at line 52, The persistence lookup uses a hardcoded key "customer-onboarding" causing a desync; update the call to hasPersistedJourney to use the same dynamic key as the tab by passing customerOnboardingHandle.id (i.e., change hasPersistedJourney("customer-onboarding", customer.id) to hasPersistedJourney(customerOnboardingHandle.id, customer.id)) so the resume label and persisted state remain consistent with the tab key.packages/journeys/src/register-options.test.ts (1)
4-4: PrefercreateTestHarnessover rawgetInternalsin tests.The PR introduces
createTestHarnessas the public test harness and migratessimulateJourneyoff internals. This file still reaches into__getRecord,__getRegistered, and__bindStepCallbacksviagetInternals, which duplicates what the harness already exposes viafireExit(and makes these tests sensitive to internal-shape changes the harness is supposed to insulate callers from).♻️ Proposed refactor using the harness
-import { createJourneyRuntime, getInternals } from "./runtime.js"; +import { createJourneyRuntime } from "./runtime.js"; +import { createTestHarness } from "./testing.js"; @@ -function driveExit(rt: ReturnType<typeof createJourneyRuntime>, id: string, exit: string, output?: unknown) { - const internals = getInternals(rt); - const rec = internals.__getRecord(id)!; - const reg = internals.__getRegistered("j")!; - internals.__bindStepCallbacks(rec, reg).exit(exit, output); -} +function driveExit(rt: ReturnType<typeof createJourneyRuntime>, id: string, exit: string, output?: unknown) { + createTestHarness(rt).fireExit(id, exit, output); +}Also applies to: 57-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/register-options.test.ts` at line 4, Tests are reaching into internals via getInternals to call __getRecord, __getRegistered, and __bindStepCallbacks; replace those calls with the public createTestHarness API (import createTestHarness from runtime.js) and use the harness methods (e.g., fireExit and whatever harness exposes for inspecting records/registered steps and binding callbacks) so tests no longer depend on internal shapes—update all usages in this file (including the block around lines 57-62) to construct a harness and call its public helpers instead of getInternals/__* methods.packages/journeys/README.md (1)
1327-1331:createTestHarnessis missing from the testing exports table.The PR objectives and
index.tscallouts both highlightcreateTestHarness(runtime)as the public test harness, yet the@modular-react/journeys/testingtable only listssimulateJourney/JourneySimulator. Add it so the "migrate offgetInternals" guidance inindex.tshas a landing spot in the docs.Proposed addition
| Export | Purpose | | ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `simulateJourney` | Headless simulator: fires exits / goBack, exposes `step` / `currentStep` (throws if terminal) / `state` / `history` / `status` / `transitions` / `terminalPayload` / `serialize()`, no React. | | `JourneySimulator` | Type for the object returned by `simulateJourney`. | +| `createTestHarness` | Public test harness (`fireExit`, `goBack`, `inspect`) bound to a runtime — replacement for the former `getInternals` access path. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/README.md` around lines 1327 - 1331, Add the missing testing export entry for createTestHarness to the `@modular-react/journeys/testing` exports table in README.md: update the table row list that currently documents simulateJourney and JourneySimulator to also include a `createTestHarness` entry and a short Purpose description linking it to the public test harness (e.g., "Public test harness for runtime-based testing; migration target for getInternals"), ensuring the symbol name `createTestHarness` is used exactly and that it aligns with the index.ts callouts referencing createTestHarness(runtime).packages/react-router-runtime/src/registry.ts (1)
389-409: Consider validating thatuse()isn't re-entered duringextend().
use()pushes toplugins(line 397) and then callsplugin.extend(...)(line 399). If a plugin'sextendwere ever written to callregistry.use(anotherPlugin)(legitimate composition), the duplicate-name guard would work but the nested plugin'sextendwould run first and its methods could be overwritten or lost depending on order. Todayextendcontexts only exposemarkDirty, so this is theoretical — but if you plan to broadenPluginExtendCtxlater, adding a re-entrancy guard (or makingplugins.pushhappen afterextend) would save a future debugging session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router-runtime/src/registry.ts` around lines 389 - 409, The use() flow can be re-entered during plugin.extend which pushes plugin into plugins before calling plugin.extend; to prevent nested registration from running in an inconsistent state either (A) move seenPluginNames.add(plugin.name) and plugins.push(plugin) to after calling plugin.extend({ markDirty: () => {} }) so the plugin is only considered registered once its extension completes, or (B) add a reentrancy guard flag around use() (e.g., a local inExtend boolean checked/set before calling plugin.extend and cleared after) to reject or queue nested registry.use calls; update checks that reference seenPluginNames, plugins, plugin.extend, PluginExtendCtx/markDirty and registry accordingly.packages/tanstack-router-runtime/src/types.ts (1)
140-249: Extract theonModuleExitevent payload to a shared type.The same
{ moduleId; entry; exit; output; tabId? }shape is inlined inResolveManifestOptions.onModuleExit(140‑146),ResolvedManifest.onModuleExit(243‑249), and — per the plugin file —JourneysPluginOptions.onModuleExitinpackages/journeys/src/plugin.tsx. Any future addition (e.g.timestamp,reason) has to be synchronized across three spots without TypeScript catching the drift. Lifting this into a single exported alias (ideally reused from@modular-react/journeys'ModuleTabExitEvent) keeps them in lockstep.Sketch
+import type { ModuleTabExitEvent } from "@modular-react/journeys"; … - onModuleExit?: (event: { - readonly moduleId: string; - readonly entry: string; - readonly exit: string; - readonly output: unknown; - readonly tabId?: string; - }) => void; + onModuleExit?: (event: ModuleTabExitEvent) => void;If
@modular-react/journeysis a peer/optional dep, hoist the alias to@modular-react/coreinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tanstack-router-runtime/src/types.ts` around lines 140 - 249, Create and export a single shared type for the module-exit payload (e.g. ModuleExitEvent or reuse ModuleTabExitEvent) from a common place (preferably `@modular-react/core` or the existing shared types file) and replace the inline object types currently used in ResolveManifestOptions.onModuleExit, ResolvedManifest.onModuleExit, and JourneysPluginOptions.onModuleExit with that exported alias; preserve all fields (moduleId, entry, exit, output, optional tabId) and update imports where those interfaces reference the inline shape so all three call sites use the single shared type.examples/tanstack-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.ts (2)
147-152:onHydrateis brittle against any futureversionbump.Exact-string matching against
"1.0.0"means the first time you ship1.0.1(a no-op patch, a typo fix, anything), every persisted blob will be discarded via the runtime'sdiscardBlob+startFreshpath — customers mid-onboarding on reload will silently lose state. Even if that's the desired behaviour today, the example is what new adopters copy-paste, so it's worth teaching the right pattern up front: either whitelist known older versions with migrations, or compare bymajor.minorand let patches pass through.Suggested pattern
onHydrate: (blob) => { - if (blob.version !== "1.0.0") { - throw new Error(`Unknown customer-onboarding journey version: ${blob.version}`); - } - return blob; + const [major, minor] = blob.version.split("."); + if (major !== "1" || minor !== "0") { + throw new Error(`Unsupported customer-onboarding journey version: ${blob.version}`); + } + return blob; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.ts` around lines 147 - 152, The onHydrate handler currently rejects any blob whose blob.version !== "1.0.0", which will discard persisted state on patch-level version bumps; update onHydrate (the function named onHydrate that reads blob.version) to accept compatible patch versions instead of exact-string matching — e.g., parse blob.version and compare only major and minor (or implement a whitelist/migrations table keyed by version) and return the blob or run migrations for supported versions rather than throwing, ensuring you still throw for truly unknown/unsupported major versions.
143-145:onAbandonembeds the entire journeystatein the abort payload.
stateincludescustomerId(and any future PII you stuff intoOnboardingState). Since the abort payload is serialized intoterminalPayloadand typically flows through analytics /onAborthooks / persisted terminal blobs, this can unintentionally expand the PII surface for every abandoned journey. Consider scoping what's captured:- onAbandon: ({ step, state }) => ({ - abort: { reason: "abandoned", at: step?.moduleId, state }, - }), + onAbandon: ({ step, state }) => ({ + abort: { + reason: "abandoned", + at: step?.moduleId, + customerId: state.customerId, // whatever you actually need downstream + selectedPlanId: state.selectedPlan?.id ?? null, + }, + }),Same caveat applies to the React Router copy of this example — worth aligning both.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.ts` around lines 143 - 145, The current onAbandon handler embeds the full journey state (including customerId/PII) into the abort payload; update the onAbandon implementation so abort contains only a minimal, non-PII subset (e.g., reason and at: step?.moduleId or other non-identifying metadata) or a sanitized summary produced by a helper like buildSafeAbortPayload(state), ensuring you do not include customerId or any PII from OnboardingState in the terminalPayload/abort object.packages/react-router-runtime/src/types.ts (1)
141-147: Consider extracting theonModuleExitevent shape to a named type.The event payload
{ moduleId, entry, exit, output, tabId? }is declared inline both here and at Lines 245-251, and the plugin wiring inpackages/journeys/src/plugin.tsx(JourneyProvider/onModuleExit) has to match it by hand. A sharedModuleExitEventtype would keep the three sites from drifting.Also applies to: 245-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router-runtime/src/types.ts` around lines 141 - 147, Extract the inline event payload into a named exported type (e.g. ModuleExitEvent) and use it for the onModuleExit callback signature instead of the inline object; specifically, declare export type ModuleExitEvent = { readonly moduleId: string; readonly entry: string; readonly exit: string; readonly output: unknown; readonly tabId?: string } in the same module as the current inline declaration, update the onModuleExit?: (event: ModuleExitEvent) => void signature where it is currently inline, and update the places that manually recreate the shape (including the JourneyProvider / onModuleExit wiring) to import and reference ModuleExitEvent so all three sites share the same type.packages/journeys/src/runtime.test.ts (1)
458-468: Comment says "three transitions" but the test only drives two saves; strengthen the post-release assertion.The inline comment describes a "three transitions rapid-fire" sequence, but the body only fires
start(implicit) +cancelled— two saves, not three. More importantly, afterresolveFirst()the test drains the microtask queue but never re-assertssaves.length, so it doesn't actually prove the coalesced terminal save eventually flushed (it only proves the second save was queued and not started while the first was paused). Consider pinningsaves.length === 2after the drain to lock in the coalesce-then-flush behavior.♻️ Proposed tweak
- // Fire three transitions rapid-fire — cancelled completes. Sequence: - // start (save queued), cancelled (save queued & coalesces on top of the + // Sequence: start (save queued), cancelled (save queued & coalesces on top of the // first in-flight save). The runtime must have at most one save in // flight at a time. internals.__bindStepCallbacks(internals.__getRecord(id)!, reg).exit("cancelled"); // First save is paused; the terminal save must have coalesced. expect(saves.length).toBe(1); resolveFirst(); await Promise.resolve(); await Promise.resolve(); + // The coalesced terminal save flushes once the first one settles. + expect(saves.length).toBe(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.test.ts` around lines 458 - 468, The test's comment and assertions are inconsistent: it claims "three transitions rapid-fire" but only triggers start + cancelled via internals.__bindStepCallbacks(internals.__getRecord(id)!, reg).exit("cancelled"), and after resolveFirst() the test never re-checks saves to confirm the terminal save flushed. Fix by either actually firing the third transition (call exit("...") a third time on the same record/registration) or, more simply, update the assertions after resolveFirst() and the microtask drains to assert saves.length === 2 (referencing saves and resolveFirst) and update the inline comment to match the two-transition scenario so the test proves the coalesced terminal save eventually flushed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad43eda8-f428-457e-a4ad-cd6d79eb20db
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (47)
.github/workflows/ci.yml.gitignoreexamples/react-router/customer-onboarding-journey/README.mdexamples/react-router/customer-onboarding-journey/app-shared/src/index.tsexamples/react-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.tsexamples/react-router/customer-onboarding-journey/shell/e2e/smoke.spec.tsexamples/react-router/customer-onboarding-journey/shell/package.jsonexamples/react-router/customer-onboarding-journey/shell/playwright.config.tsexamples/react-router/customer-onboarding-journey/shell/src/components/Home.tsxexamples/react-router/customer-onboarding-journey/shell/src/main.tsxexamples/react-router/customer-onboarding-journey/shell/src/services/workspace-actions.tsexamples/tanstack-router/customer-onboarding-journey/README.mdexamples/tanstack-router/customer-onboarding-journey/app-shared/src/index.tsexamples/tanstack-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.tsexamples/tanstack-router/customer-onboarding-journey/shell/e2e/smoke.spec.tsexamples/tanstack-router/customer-onboarding-journey/shell/package.jsonexamples/tanstack-router/customer-onboarding-journey/shell/playwright.config.tsexamples/tanstack-router/customer-onboarding-journey/shell/src/components/Home.tsxexamples/tanstack-router/customer-onboarding-journey/shell/src/components/Shell.tsxexamples/tanstack-router/customer-onboarding-journey/shell/src/main.tsxexamples/tanstack-router/customer-onboarding-journey/shell/src/services/workspace-actions.tspackages/core/src/index.tspackages/core/src/journey-contracts.tspackages/core/src/plugin.tspackages/journeys/README.mdpackages/journeys/src/handle.test.tspackages/journeys/src/handle.tspackages/journeys/src/index.tspackages/journeys/src/outlet.test.tsxpackages/journeys/src/outlet.tsxpackages/journeys/src/plugin.tsxpackages/journeys/src/register-options.test.tspackages/journeys/src/runtime.test.tspackages/journeys/src/runtime.tspackages/journeys/src/simulate-journey.test.tspackages/journeys/src/simulate-journey.tspackages/journeys/src/testing.test.tspackages/journeys/src/testing.tspackages/journeys/src/types.tspackages/react-router-runtime/src/registry-journeys.test.tspackages/react-router-runtime/src/registry.tspackages/react-router-runtime/src/types.tspackages/react-router-runtime/vite.config.tspackages/tanstack-router-runtime/src/registry-journeys.test.tspackages/tanstack-router-runtime/src/registry.tspackages/tanstack-router-runtime/src/types.tspackages/tanstack-router-runtime/vite.config.ts
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- packages/react-router-runtime/vite.config.ts
- examples/tanstack-router/customer-onboarding-journey/shell/package.json
- examples/react-router/customer-onboarding-journey/shell/package.json
- examples/tanstack-router/customer-onboarding-journey/shell/playwright.config.ts
- examples/tanstack-router/customer-onboarding-journey/README.md
- examples/react-router/customer-onboarding-journey/app-shared/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- examples/tanstack-router/customer-onboarding-journey/shell/src/components/Shell.tsx
- examples/tanstack-router/customer-onboarding-journey/shell/src/main.tsx
- packages/journeys/src/simulate-journey.test.ts
- examples/tanstack-router/customer-onboarding-journey/shell/src/services/workspace-actions.ts
- examples/react-router/customer-onboarding-journey/shell/src/components/Home.tsx
- examples/react-router/customer-onboarding-journey/shell/src/main.tsx
- packages/core/src/index.ts
| page.on("console", (msg: ConsoleMessage) => { | ||
| if (msg.type() !== "error") return; | ||
| const text = msg.text(); | ||
| // React's dev build yells loudly about DOM hydration warnings in StrictMode; | ||
| // keep only real failures. Extend if the example adds known-noisy logs. | ||
| consoleErrors.push(text); | ||
| }); |
There was a problem hiding this comment.
Comment doesn't match the code — nothing is actually filtered.
The "keep only real failures" / "Extend if the example adds known-noisy logs" wording implies a denylist that isn't there. Either drop the comment or push the filtering into the handler so future noise has an obvious hook.
Proposed fix
page.on("console", (msg: ConsoleMessage) => {
if (msg.type() !== "error") return;
- const text = msg.text();
- // React's dev build yells loudly about DOM hydration warnings in StrictMode;
- // keep only real failures. Extend if the example adds known-noisy logs.
- consoleErrors.push(text);
+ consoleErrors.push(msg.text());
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| page.on("console", (msg: ConsoleMessage) => { | |
| if (msg.type() !== "error") return; | |
| const text = msg.text(); | |
| // React's dev build yells loudly about DOM hydration warnings in StrictMode; | |
| // keep only real failures. Extend if the example adds known-noisy logs. | |
| consoleErrors.push(text); | |
| }); | |
| page.on("console", (msg: ConsoleMessage) => { | |
| if (msg.type() !== "error") return; | |
| consoleErrors.push(msg.text()); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/react-router/customer-onboarding-journey/shell/e2e/smoke.spec.ts`
around lines 20 - 26, The comment claims noisy console messages are filtered but
the handler for page.on("console", (msg: ConsoleMessage) => { ... }) currently
pushes every error into consoleErrors; update the handler to actually filter
known-noisy messages (or remove the misleading comment). For a fix, implement a
denylist predicate inside the page.on callback (e.g., check msg.text() for
hydration/StrictMode patterns like /hydrate|Warning.*Did not expect/ or any
example-specific noisy strings) and only push to consoleErrors when the message
does not match the denylist; reference the page.on("console", (msg:
ConsoleMessage) => { ... }), the text variable, and the consoleErrors array when
making the change.
There was a problem hiding this comment.
Already addressed - the misleading comment was removed; the handler now just pushes every console.error without pretending to filter.
There was a problem hiding this comment.
Rate Limit Exceeded
@kibertoad have exceeded the limit for the number of chat messages per hour. Please wait 57 minutes and 13 seconds before sending another message.
| @@ -0,0 +1,63 @@ | |||
| import type { StoreApi } from "zustand/vanilla"; | |||
There was a problem hiding this comment.
Fix oxfmt formatting violation flagged by CI.
The CI pipeline reports an oxfmt formatting issue on this line. Please run the repo's formatter (e.g. pnpm format / oxfmt) to normalize the imports block before merge — the likely culprit is the ordering of the zustand/vanilla type import relative to the @example-onboarding/app-shared import group.
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: Formatting issue detected by oxfmt in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@examples/react-router/customer-onboarding-journey/shell/src/services/workspace-actions.ts`
at line 1, The import ordering in workspace-actions.ts is violating oxfmt rules
due to the type import "StoreApi" from "zustand/vanilla" being out of the
expected group order; reorder the imports so external/shared package groups
(e.g. `@example-onboarding/app-shared`) come in the correct group before or after
the zustand/vanilla type import per repo convention, then run the project
formatter (pnpm format or oxfmt) to normalize the file and commit the formatted
change; locate the import for StoreApi in workspace-actions.ts to adjust
ordering and reformat.
There was a problem hiding this comment.
Already addressed - file now passes oxfmt (type imports grouped correctly).
There was a problem hiding this comment.
Rate Limit Exceeded
@kibertoad have exceeded the limit for the number of chat messages per hour. Please wait 57 minutes and 13 seconds before sending another message.
| @@ -0,0 +1,87 @@ | |||
| import { describe, expect, it } from "vitest"; | |||
| import { defineEntry, defineExit, defineModule, schema } from "@modular-react/core"; | |||
| import { defineJourney, journeysPlugin } from "@modular-react/journeys"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect vite.config.ts in both runtime packages and confirm whether
# "@modular-react/journeys" is declared alongside the other `@modular-react/`* deps.
fd -H -t f 'vite.config.ts' packages/tanstack-router-runtime packages/react-router-runtime --exec sh -c 'echo "=== $0 ==="; cat "$0"'
# Also confirm package.json dependencies for each runtime package.
fd -H -t f 'package.json' packages/tanstack-router-runtime packages/react-router-runtime --exec sh -c 'echo "=== $0 ==="; jq "{dependencies, peerDependencies, devDependencies}" "$0"'Repository: kibertoad/modular-react
Length of output: 3101
Test cannot run — @modular-react/journeys is not resolvable from this package.
CI fails with Failed to resolve import "@modular-react/journeys". The vite.config.ts files are already correct (both packages include @modular-react/journeys in rollupOptions.external), but the actual issue is that @modular-react/journeys is missing from package.json in both packages/tanstack-router-runtime and packages/react-router-runtime. Add it to devDependencies (or peerDependencies) in both package.json files so Vitest can resolve the import at test time.
🧰 Tools
🪛 GitHub Actions: CI
[error] 3-3: Vitest failed to resolve import "@modular-react/journeys" from "src/registry-journeys.test.ts" (vite:import-analysis). Error: Failed to resolve import "@modular-react/journeys" from "src/registry-journeys.test.ts". Does the file exist?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tanstack-router-runtime/src/registry-journeys.test.ts` at line 3,
The test imports defineJourney and journeysPlugin from "@modular-react/journeys"
but that module is not declared in the package manifests, so Vitest can't
resolve it; add "@modular-react/journeys" to the package.json deps for the
tanstack-router-runtime and react-router-runtime packages (prefer
devDependencies or peerDependencies per repo policy) so the import used by
registry-journeys.test.ts (defineJourney, journeysPlugin) can be resolved at
test time, then reinstall node modules and re-run the tests.
There was a problem hiding this comment.
Already addressed - @modular-react/journeys is in devDependencies (workspace:*) in both packages/react-router-runtime/package.json and packages/tanstack-router-runtime/package.json.
There was a problem hiding this comment.
Rate Limit Exceeded
@kibertoad have exceeded the limit for the number of chat messages per hour. Please wait 57 minutes and 11 seconds before sending another message.
|
@mmurakaru thank you, fixed and added e2e test to capture such issues in the future |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-router-runtime/src/registry.ts (1)
495-519:⚠️ Potential issue | 🟡 MinorMinor: plugin
onResolveis re-invoked on every retry, leaking the previous runtime.If the first
resolveManifest()call throws insidebuildAssembly(e.g., a pluginvalidatehook failure on a later plugin after earlier plugins already ranonResolvein the previous iteration, or a retry after a transient authoring error),cachedManifeststaysnull,firstCallCompletedistrue, and the next call falls through tobuildAssemblyagain. That re-runsplugin.onResolveand constructs a freshJourneyRuntime; any listeners/timers installed by the earlier runtime (if a plugin eagerly sets anything up on construction) are silently orphaned.For the journeys plugin today this is benign because the runtime is passive until
start()is called, but other future plugins may not be. Worth either memoizing theextensionsmap across retries or documenting inRegistryPlugin.onResolvethat it may be invoked more than once per registry on recovery paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router-runtime/src/registry.ts` around lines 495 - 519, The buildAssembly/plugins onResolve path can run multiple times on retries and leak runtimes; to fix, memoize the produced extensions/runtime across the first attempt and reuse them on subsequent retries: introduce a capturedExtensions (or capturedRuntime) variable alongside capturedOptions/firstCallCompleted and, when buildAssembly() is first invoked, store its extensions/runtime result there before any later steps that may throw; on subsequent calls (when firstCallCompleted is true and cachedManifest is still null) reuse capturedExtensions instead of re-invoking plugin.onResolve or reconstructing a new JourneyRuntime; update resolveManifest() to consult capturedExtensions before calling buildAssembly() and keep existing behavior for cachedManifest and capturedOptions.
🧹 Nitpick comments (7)
examples/tanstack-router/customer-onboarding-journey/shell/e2e/smoke.spec.ts (1)
49-53: Replace the fixedwaitForTimeout(250)with a deterministic wait.Hard waits are flaky and discouraged by Playwright best practices; they can mask late-arriving errors on slow CI and add needless latency on fast runs. Since the intent is to let
<JourneyOutlet>finish mounting so any delayedpageerror/console.errorsurfaces, prefer waiting on a concrete post-mount signal (e.g., the first step heading already asserted in the later test, orpage.waitForLoadState("networkidle")).♻️ Proposed refactor
await expect(page.getByRole("button", { name: /^Onboard · Alice Martin/ })).toBeVisible(); - await page.waitForTimeout(250); + await expect(page.getByRole("heading", { name: /Profile · /i })).toBeVisible(); assertNoErrors(errors);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router/customer-onboarding-journey/shell/e2e/smoke.spec.ts` around lines 49 - 53, The hard-coded waitForTimeout(250) should be replaced with a deterministic wait: remove the waitForTimeout call and instead wait for a concrete post-mount signal (for example use page.waitForLoadState("networkidle") or wait for the JourneyOutlet's first-step heading / the same element asserted later) so any delayed pageerror/console.error surfaces before assertNoErrors(errors) runs; update the test around the existing getByRole("button", { name: /^Onboard · Alice Martin/ }) and assertNoErrors(errors) to wait on that reliable signal (e.g., page.waitForLoadState or the first step heading selector) rather than a fixed timeout.examples/react-router/customer-onboarding-journey/shell/e2e/smoke.spec.ts (1)
64-64: Prefer a deterministic wait overwaitForTimeout(250).Fixed sleeps are flaky on slow CI — 250 ms may be too short under load, and too long locally. Since the intent is "let queued errors flush," an explicit wait on a post-mount signal (e.g., the step content you already assert, or
page.waitForLoadState('networkidle')) gives the same guarantee without the magic number.♻️ Possible replacement
- await expect(page.getByRole("button", { name: /^Onboard · Alice Martin/ })).toBeVisible(); - await page.waitForTimeout(250); // let microtask-queued errors flush + await expect(page.getByRole("button", { name: /^Onboard · Alice Martin/ })).toBeVisible(); + // Any error thrown during mount surfaces synchronously with the outlet's + // first paint; once the tab button is visible the outlet has rendered. + await page.waitForLoadState("networkidle");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/react-router/customer-onboarding-journey/shell/e2e/smoke.spec.ts` at line 64, Replace the brittle fixed sleep call page.waitForTimeout(250) with a deterministic wait that waits for the post-mount signal you already assert (e.g., wait for the step content selector) or use page.waitForLoadState('networkidle'); update the test around the same location where page.waitForTimeout(250) is used in smoke.spec.ts so the test explicitly waits for the element or load state that indicates the app has finished mounting instead of sleeping for 250ms.packages/core/src/journey-contracts.ts (1)
115-121: Minor:allowBackkey could collide with an exit literally named"allowBack".
EntryTransitionsintersects a mapped type overExitNamesOf<TMod>with{ readonly allowBack?: boolean }. A module that (perversely) defines an exit calledallowBackwould end up with two differently-typed fields at the same key, which TS collapses tonever. Not worth guarding in code, but worth a sentence in the JSDoc — "allowBackis reserved on the transition map and must not be a declared exit name" — so a future author gets a docs hit rather than a confusingneverin tooltips.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/journey-contracts.ts` around lines 115 - 121, Add a short JSDoc note on the EntryTransitions type explaining that the key "allowBack" is reserved and must not be used as an exit name; specifically, update the JSDoc for the EntryTransitions<TModules, TState, TMod, TEntry> type to say that allowBack is a reserved key on the transition map and declaring an exit named "allowBack" will collide with this field and produce confusing types. This references the EntryTransitions type and the allowBack property so future authors see the reservation in tooltips/docs.packages/journeys/src/runtime.ts (2)
81-82: Dead code:ASYNC_LOAD_PENDINGsymbol is declared but never returned.
probeLoad's return type unions inAsyncLoadPending, but the function body (lines 810-822) only ever returnsnull, a resolved blob, or aPromise. The sentinel is never produced or consumed anywhere. Drop both the symbol and the type —probeLoad's return simplifies toSerializedJourney<unknown> | null | Promise<SerializedJourney<unknown> | null>.🧹 Proposed cleanup
-const ASYNC_LOAD_PENDING = Symbol("asyncLoadPending"); -type AsyncLoadPending = typeof ASYNC_LOAD_PENDING; - /**function probeLoad( reg: RegisteredJourney, persistence: JourneyPersistence<unknown>, key: string, ): | SerializedJourney<unknown> | null - | AsyncLoadPending | Promise<SerializedJourney<unknown> | null> {Also applies to: 800-823
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.ts` around lines 81 - 82, Remove the unused sentinel by deleting the ASYNC_LOAD_PENDING symbol and the AsyncLoadPending type, and update probeLoad's declared return type to SerializedJourney<unknown> | null | Promise<SerializedJourney<unknown> | null>; then simplify any internal logic or type annotations in probeLoad to stop producing/handling ASYNC_LOAD_PENDING (search for ASYNC_LOAD_PENDING, AsyncLoadPending, and probeLoad to find all occurrences) so the function only returns null, a SerializedJourney, or a Promise thereof.
429-526: Edge case: transition handler returning a shape withoutnext/complete/abortsilently no-ops.If a handler (via
as anyescape or JS caller) returns{}or{ state: … }only, lines 441-443 may mutate state but none of the threein resultbranches fires — yet lines 519-525 still callschedulePersist/removePersistedandnotify. The instance stays on the same step with no history entry, listeners get woken up, and persistence captures the new state.The TS type precludes this in well-typed code, but a defensive warning in dev mode (symmetric to the "returned a Promise" guard at lines 590-603) would surface accidental
{ state }-only returns during refactors rather than leaving them as invisible state mutations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/runtime.ts` around lines 429 - 526, applyTransition currently allows a transition handler to return an object that mutates state but has no "next", "complete", or "abort" keys (e.g. {} or {state:...}) which silently no-ops; add a dev-only warning after the existing branch handling that checks if none of "next"/"complete"/"abort" are in result and, when debug is true, console.warn with context (include previousStep?.moduleId and previousStep?.entry and any identifying info from reg) describing that the transition returned an unexpected shape and no navigation occurred; keep behavior unchanged otherwise (just warn) so callers can spot accidental returns similar to the existing Promise-return guard.packages/journeys/README.md (1)
1336-1342:JourneyRegisterOptionsdocumented shape is incomplete.The runtime in
packages/journeys/src/runtime.tsreadsreg.options?.onAbandon,onComplete,onAbort,onError, andonHydratein addition to the three listed here (onTransition,persistence,maxHistory). The README's surrounding prose in "Observation hooks" actually describes most of these, but the formal interface block undersells the option surface — a reader copy/pasting it as "the" type will miss half the registration-level hooks that the rest of the document references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/README.md` around lines 1336 - 1342, The documented JourneyRegisterOptions interface is missing several optional hook properties that the runtime reads (reg.options?.onAbandon, onComplete, onAbort, onError, onHydrate); update the README's JourneyRegisterOptions declaration to include these optional callbacks (preserving the existing generic TState and the existing onTransition, persistence, maxHistory) so the documented type matches the runtime behavior and the "Observation hooks" prose.packages/journeys/src/validation.ts (1)
107-115: Nit: inconsistent handling of missingdef.idin issue messages.Line 109 uses
def.id ?? "(unknown)"but lines 111, 113, and 115 interpolatedef.iddirectly, producingjourney "undefined" ...messages when id is missing. Minor cosmetic inconsistency.✂️ Proposed tweak
- if (typeof def.initialState !== "function") - issues.push(`journey "${def.id}" must declare initialState as a function`); - if (typeof def.start !== "function") - issues.push(`journey "${def.id}" must declare start as a function`); - if (!def.transitions || typeof def.transitions !== "object") - issues.push(`journey "${def.id}" must declare transitions`); + const idLabel = def.id ?? "(unknown)"; + if (typeof def.initialState !== "function") + issues.push(`journey "${idLabel}" must declare initialState as a function`); + if (typeof def.start !== "function") + issues.push(`journey "${idLabel}" must declare start as a function`); + if (!def.transitions || typeof def.transitions !== "object") + issues.push(`journey "${idLabel}" must declare transitions`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journeys/src/validation.ts` around lines 107 - 115, The issue is inconsistent handling of a missing def.id in validation messages causing "journey \"undefined\"" to appear; fix by normalizing the id used in messages—compute a local identifier like const jid = def.id ?? "(unknown)" (or use def.id ?? "(unknown)" inline) and replace occurrences in the initialState, start, and transitions error pushes so all messages use jid instead of def.id directly; update the checks around typeof def.initialState, typeof def.start, and the def.transitions check to use this normalized jid when constructing the strings pushed to issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/journeys/src/runtime.ts`:
- Around line 1042-1046: hydrate() currently sets instances.set(instanceId,
record) before calling hydrateInto(record, migrated.blob), which leaks the
half-built record if hydrateInto throws; wrap the hydrateInto call in a
try/catch and on error remove the placeholder exactly as the sync-start path
does: instances.delete(instanceId), remove any keyIndex entry that was added for
this instance (e.g., keyIndex.delete(record.key || instanceKey)), and
delete/cleanup the migrated.blob entry that was stored for this id (mirror the
blob cleanup used in the sync-start error path), then rethrow the original error
so callers still receive the JourneyHydrationError. Ensure notify(record) and
the return only happen after successful hydration.
In `@packages/journeys/src/validation.ts`:
- Around line 62-95: The transitions traversal in
validateJourneyContracts/validateJourneyDefinition assumes nested objects and
will throw on null/non-object entries; add defensive guards before iterating:
when reading transitions, check each perModule is an object (non-null) and push
an issue like `journey "${def.id}" has malformed transitions for "${moduleId}"`
if not, skip iterating; likewise check each perEntry is an object before calling
Object.keys or accessing perEntry.allowBack and push a similar issue for
`moduleId.entryName`; update the loops around Object.entries(perModule),
Object.keys(perEntry), and perEntry.allowBack checks (referencing transitions,
perModule, perEntry, entry.allowBack) to skip/record malformed entries instead
of letting a TypeError propagate so all issues accumulate.
In `@packages/react-router-runtime/src/registry.ts`:
- Around line 475-481: The current assertions cast the manifest's extensions to
Record<string, unknown> causing the conditional type for journeys to resolve to
unknown; update the two incorrect casts so that where you currently assert
ApplicationManifest<..., Record<string, unknown>> and ResolvedManifest<...,
Record<string, unknown>> you instead use the actual plugin runtimes type
(PluginRuntimesOf<TPlugins> or the corresponding generic
TExtensions/PluginRuntimes type parameter) so that the conditional type for
journeys evaluates correctly to the inferred type or never; specifically change
the assertions that build the returned object (the properties moduleDescriptors,
extensions, journeys) inside resolve() and resolveManifest() to use
PluginRuntimesOf<TPlugins> (or the generic runtime type) for the extensions
generic to restore proper compile-time guarding when the journeys plugin is
absent.
---
Outside diff comments:
In `@packages/react-router-runtime/src/registry.ts`:
- Around line 495-519: The buildAssembly/plugins onResolve path can run multiple
times on retries and leak runtimes; to fix, memoize the produced
extensions/runtime across the first attempt and reuse them on subsequent
retries: introduce a capturedExtensions (or capturedRuntime) variable alongside
capturedOptions/firstCallCompleted and, when buildAssembly() is first invoked,
store its extensions/runtime result there before any later steps that may throw;
on subsequent calls (when firstCallCompleted is true and cachedManifest is still
null) reuse capturedExtensions instead of re-invoking plugin.onResolve or
reconstructing a new JourneyRuntime; update resolveManifest() to consult
capturedExtensions before calling buildAssembly() and keep existing behavior for
cachedManifest and capturedOptions.
---
Nitpick comments:
In `@examples/react-router/customer-onboarding-journey/shell/e2e/smoke.spec.ts`:
- Line 64: Replace the brittle fixed sleep call page.waitForTimeout(250) with a
deterministic wait that waits for the post-mount signal you already assert
(e.g., wait for the step content selector) or use
page.waitForLoadState('networkidle'); update the test around the same location
where page.waitForTimeout(250) is used in smoke.spec.ts so the test explicitly
waits for the element or load state that indicates the app has finished mounting
instead of sleeping for 250ms.
In
`@examples/tanstack-router/customer-onboarding-journey/shell/e2e/smoke.spec.ts`:
- Around line 49-53: The hard-coded waitForTimeout(250) should be replaced with
a deterministic wait: remove the waitForTimeout call and instead wait for a
concrete post-mount signal (for example use page.waitForLoadState("networkidle")
or wait for the JourneyOutlet's first-step heading / the same element asserted
later) so any delayed pageerror/console.error surfaces before
assertNoErrors(errors) runs; update the test around the existing
getByRole("button", { name: /^Onboard · Alice Martin/ }) and
assertNoErrors(errors) to wait on that reliable signal (e.g.,
page.waitForLoadState or the first step heading selector) rather than a fixed
timeout.
In `@packages/core/src/journey-contracts.ts`:
- Around line 115-121: Add a short JSDoc note on the EntryTransitions type
explaining that the key "allowBack" is reserved and must not be used as an exit
name; specifically, update the JSDoc for the EntryTransitions<TModules, TState,
TMod, TEntry> type to say that allowBack is a reserved key on the transition map
and declaring an exit named "allowBack" will collide with this field and produce
confusing types. This references the EntryTransitions type and the allowBack
property so future authors see the reservation in tooltips/docs.
In `@packages/journeys/README.md`:
- Around line 1336-1342: The documented JourneyRegisterOptions interface is
missing several optional hook properties that the runtime reads
(reg.options?.onAbandon, onComplete, onAbort, onError, onHydrate); update the
README's JourneyRegisterOptions declaration to include these optional callbacks
(preserving the existing generic TState and the existing onTransition,
persistence, maxHistory) so the documented type matches the runtime behavior and
the "Observation hooks" prose.
In `@packages/journeys/src/runtime.ts`:
- Around line 81-82: Remove the unused sentinel by deleting the
ASYNC_LOAD_PENDING symbol and the AsyncLoadPending type, and update probeLoad's
declared return type to SerializedJourney<unknown> | null |
Promise<SerializedJourney<unknown> | null>; then simplify any internal logic or
type annotations in probeLoad to stop producing/handling ASYNC_LOAD_PENDING
(search for ASYNC_LOAD_PENDING, AsyncLoadPending, and probeLoad to find all
occurrences) so the function only returns null, a SerializedJourney, or a
Promise thereof.
- Around line 429-526: applyTransition currently allows a transition handler to
return an object that mutates state but has no "next", "complete", or "abort"
keys (e.g. {} or {state:...}) which silently no-ops; add a dev-only warning
after the existing branch handling that checks if none of
"next"/"complete"/"abort" are in result and, when debug is true, console.warn
with context (include previousStep?.moduleId and previousStep?.entry and any
identifying info from reg) describing that the transition returned an unexpected
shape and no navigation occurred; keep behavior unchanged otherwise (just warn)
so callers can spot accidental returns similar to the existing Promise-return
guard.
In `@packages/journeys/src/validation.ts`:
- Around line 107-115: The issue is inconsistent handling of a missing def.id in
validation messages causing "journey \"undefined\"" to appear; fix by
normalizing the id used in messages—compute a local identifier like const jid =
def.id ?? "(unknown)" (or use def.id ?? "(unknown)" inline) and replace
occurrences in the initialState, start, and transitions error pushes so all
messages use jid instead of def.id directly; update the checks around typeof
def.initialState, typeof def.start, and the def.transitions check to use this
normalized jid when constructing the strings pushed to issues.
🪄 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: 2311000b-2d9b-4946-bf77-c6f2d9a94ac1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
examples/react-router/customer-onboarding-journey/shell/e2e/smoke.spec.tsexamples/react-router/customer-onboarding-journey/shell/src/components/Home.tsxexamples/react-router/customer-onboarding-journey/shell/src/main.tsxexamples/react-router/customer-onboarding-journey/shell/src/services/workspace-actions.tsexamples/tanstack-router/customer-onboarding-journey/shell/e2e/smoke.spec.tsexamples/tanstack-router/customer-onboarding-journey/shell/src/components/Home.tsxexamples/tanstack-router/customer-onboarding-journey/shell/src/main.tsxexamples/tanstack-router/customer-onboarding-journey/shell/src/services/workspace-actions.tspackages/core/src/journey-contracts.tspackages/journeys/README.mdpackages/journeys/src/handle.test-d.tspackages/journeys/src/handle.tspackages/journeys/src/index.tspackages/journeys/src/outlet.test.tsxpackages/journeys/src/plugin.tsxpackages/journeys/src/register-options.test.tspackages/journeys/src/runtime.tspackages/journeys/src/testing.test.tspackages/journeys/src/testing.tspackages/journeys/src/types.tspackages/journeys/src/validation.tspackages/react-router-runtime/package.jsonpackages/react-router-runtime/src/registry.tspackages/tanstack-router-runtime/package.jsonpackages/tanstack-router-runtime/src/registry.ts
✅ Files skipped from review due to trivial changes (5)
- packages/react-router-runtime/package.json
- packages/tanstack-router-runtime/package.json
- packages/journeys/src/handle.ts
- examples/tanstack-router/customer-onboarding-journey/shell/src/services/workspace-actions.ts
- packages/journeys/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/tanstack-router/customer-onboarding-journey/shell/src/components/Home.tsx
- packages/journeys/src/register-options.test.ts
- examples/react-router/customer-onboarding-journey/shell/src/main.tsx
- packages/journeys/src/outlet.test.tsx
- packages/journeys/src/testing.ts
- examples/react-router/customer-onboarding-journey/shell/src/services/workspace-actions.ts
| const record = createRecord(reg, instanceId, null, migrated.blob.state); | ||
| instances.set(instanceId, record); | ||
| hydrateInto(record, migrated.blob); | ||
| notify(record); | ||
| return instanceId; |
There was a problem hiding this comment.
hydrate() leaks a half-built record if hydrateInto throws.
instances.set(instanceId, record) happens before hydrateInto(record, migrated.blob). If hydrateInto throws (e.g., the rollbackSnapshots/history length mismatch check at lines 772-776), the caller gets the JourneyHydrationError but the just-createRecord-ed placeholder (status "loading", init state, no history) is already parked in instances. Subsequent getInstance(id) returns this partial record, hydrate() with the same id throws the "already in memory" guard, and forget(id) is a no-op (status ≠ terminal) — the record is effectively leaked until the caller knows to call end(id) first.
The sync-start path at lines 971-986 already handles this with a try/catch that cleans up instances/keyIndex/blob. Mirror that here.
🛡️ Proposed fix
const record = createRecord(reg, instanceId, null, migrated.blob.state);
instances.set(instanceId, record);
- hydrateInto(record, migrated.blob);
- notify(record);
- return instanceId;
+ try {
+ hydrateInto(record, migrated.blob);
+ } catch (err) {
+ instances.delete(instanceId);
+ throw err;
+ }
+ notify(record);
+ return instanceId;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const record = createRecord(reg, instanceId, null, migrated.blob.state); | |
| instances.set(instanceId, record); | |
| hydrateInto(record, migrated.blob); | |
| notify(record); | |
| return instanceId; | |
| const record = createRecord(reg, instanceId, null, migrated.blob.state); | |
| instances.set(instanceId, record); | |
| try { | |
| hydrateInto(record, migrated.blob); | |
| } catch (err) { | |
| instances.delete(instanceId); | |
| throw err; | |
| } | |
| notify(record); | |
| return instanceId; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/journeys/src/runtime.ts` around lines 1042 - 1046, hydrate()
currently sets instances.set(instanceId, record) before calling
hydrateInto(record, migrated.blob), which leaks the half-built record if
hydrateInto throws; wrap the hydrateInto call in a try/catch and on error remove
the placeholder exactly as the sync-start path does:
instances.delete(instanceId), remove any keyIndex entry that was added for this
instance (e.g., keyIndex.delete(record.key || instanceKey)), and delete/cleanup
the migrated.blob entry that was stored for this id (mirror the blob cleanup
used in the sync-start error path), then rethrow the original error so callers
still receive the JourneyHydrationError. Ensure notify(record) and the return
only happen after successful hydration.
There was a problem hiding this comment.
Addressed - runtime.ts:1044 now wraps hydrateInto in try/catch and calls instances.delete(instanceId) on failure before rethrowing, mirroring the sync-start cleanup path. keyIndex / blob don't need cleanup here because explicit hydrate() is intentionally persistence-unlinked (no key is set at createRecord).
There was a problem hiding this comment.
Rate Limit Exceeded
@kibertoad have exceeded the limit for the number of chat messages per hour. Please wait 57 minutes and 10 seconds before sending another message.
| moduleDescriptors: assembly.moduleDescriptors, | ||
| extensions: assembly.extensions, | ||
| journeys: assembly.extensions.journeys as ApplicationManifest< | ||
| TSlots, | ||
| TNavItem, | ||
| Record<string, unknown> | ||
| >["journeys"], |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how manifest.journeys and manifest.extensions.journeys are typed
fd 'types.ts' packages/react-router-runtime packages/tanstack-router-runtime --exec rg -n -C3 'journeys|extensions' {}Repository: kibertoad/modular-react
Length of output: 3414
🏁 Script executed:
# First, let's look at the exact code in registry.ts at the mentioned lines
cat -n packages/react-router-runtime/src/registry.ts | sed -n '470,485p'Repository: kibertoad/modular-react
Length of output: 647
🏁 Script executed:
# Also check lines 549-555
cat -n packages/react-router-runtime/src/registry.ts | sed -n '545,560p'Repository: kibertoad/modular-react
Length of output: 706
🏁 Script executed:
# Get the full type definitions from types.ts for both ApplicationManifest and ResolvedApplicationManifest
cat -n packages/react-router-runtime/src/types.ts | sed -n '75,95p'Repository: kibertoad/modular-react
Length of output: 1105
🏁 Script executed:
# Check the second occurrence in types.ts
cat -n packages/react-router-runtime/src/types.ts | sed -n '225,245p'Repository: kibertoad/modular-react
Length of output: 958
🏁 Script executed:
# Find where assembly is defined and what its type is
rg -n "assembly\s*=" packages/react-router-runtime/src/registry.ts | head -20Repository: kibertoad/modular-react
Length of output: 153
🏁 Script executed:
# Look for the function signature containing these type parameters
sed -n '450,485p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 1468
🏁 Script executed:
# Check what TExtensions is and how it flows through the function
rg -B 20 "journeys: assembly.extensions.journeys as ApplicationManifest" packages/react-router-runtime/src/registry.ts | head -40Repository: kibertoad/modular-react
Length of output: 840
🏁 Script executed:
# Let's see the full function signature for context
sed -n '430,485p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 2321
🏁 Script executed:
# Find the function signatures - look for the start of the functions containing these return statements
sed -n '400,485p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 3676
🏁 Script executed:
# Look even further back for function declarations
sed -n '350,450p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 4404
🏁 Script executed:
# Search for buildAssembly function definition
rg -n "function buildAssembly|const buildAssembly" packages/react-router-runtime/src/registry.tsRepository: kibertoad/modular-react
Length of output: 107
🏁 Script executed:
# Check the function around line 350-400 more carefully with wider context
sed -n '340,400p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 2538
🏁 Script executed:
# Look at the buildAssembly function definition and return type
sed -n '249,330p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 4040
🏁 Script executed:
# Check what the extensions variable is in buildAssembly
rg -n "extensions" packages/react-router-runtime/src/registry.ts | grep -E "249:|330:|extensions ="Repository: kibertoad/modular-react
Length of output: 49
🏁 Script executed:
# Let's see how extensions is built - look for where it's assigned
sed -n '249,350p' packages/react-router-runtime/src/registry.ts | cat -n | tail -50Repository: kibertoad/modular-react
Length of output: 2158
🏁 Script executed:
# Let's see the complete function signatures and return types for both resolve functions
rg -n "resolve\(" packages/react-router-runtime/src/registry.ts | head -5Repository: kibertoad/modular-react
Length of output: 381
🏁 Script executed:
# Check the first resolve function signature more completely
sed -n '405,430p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 1219
🏁 Script executed:
# Check the resolveManifest function signature
sed -n '480,510p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 1654
🏁 Script executed:
# Let's check what CommonAssembly type is and if it has the extensions property typed
rg -n "type CommonAssembly|interface CommonAssembly" packages/react-router-runtime/src/Repository: kibertoad/modular-react
Length of output: 143
🏁 Script executed:
# Get the CommonAssembly interface definition
sed -n '173,220p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 2678
🏁 Script executed:
# Now let's understand the real issue - check if extensions is actually typed as Record<string, unknown> in CommonAssembly
rg -A 20 "interface CommonAssembly" packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 1035
🏁 Script executed:
# Let me verify the actual type behavior - check what happens when the journeys plugin is used vs not used
rg -n "journeysPlugin" packages/react-router-runtime/src/ --type tsRepository: kibertoad/modular-react
Length of output: 956
🏁 Script executed:
# Look for how journeys plugin extends the extensions
rg -n "journeys" packages/react-router-runtime/src/ -A 2 -B 2 | head -50Repository: kibertoad/modular-react
Length of output: 3906
🏁 Script executed:
# Check if there's a way to understand how TExtensions should flow through the type system
rg -n "TExtensions" packages/react-router-runtime/src/types.tsRepository: kibertoad/modular-react
Length of output: 615
🏁 Script executed:
# Let me check what the actual TExtensions type is in the resolve and resolveManifest functions
# Look at the full function signature including generics
sed -n '405,425p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 992
🏁 Script executed:
# Check the resolveManifest function signature completely
sed -n '485,510p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 1493
🏁 Script executed:
# Find where these functions are defined in relation to the plugin tracking
sed -n '40,100p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 3160
🏁 Script executed:
# This is the key! Let me find PluginRuntimesOf which should track the plugin types
rg -n "PluginRuntimesOf" packages/react-router-runtime/src/Repository: kibertoad/modular-react
Length of output: 376
🏁 Script executed:
# Now I understand the issue - the resolve function signature promises PluginRuntimesOf<TPlugins>
# But the implementation hard-codes Record<string, unknown>
# Let me verify by looking at how resolve is actually typed vs implemented
sed -n '56,59p' packages/react-router-runtime/src/registry.tsRepository: kibertoad/modular-react
Length of output: 298
🏁 Script executed:
# And check the actual implementation return type annotation
sed -n '418,420p' packages/react-router-runtime/src/registry.tsRepository: kibertoad/modular-react
Length of output: 213
🏁 Script executed:
# Find PluginRuntimesOf definition
rg -n "PluginRuntimesOf|type PluginRuntimesOf" packages/react-router-runtime/src/Repository: kibertoad/modular-react
Length of output: 376
🏁 Script executed:
# Check the exact line numbers for both the interface declaration and implementation
echo "=== Interface declaration (should use PluginRuntimesOf<TPlugins>) ===" && \
sed -n '97p' packages/react-router-runtime/src/registry.ts && \
echo -e "\n=== Implementation return type (currently uses Record<string, unknown>) ===" && \
sed -n '418,420p' packages/react-router-runtime/src/registry.ts && \
echo -e "\n=== Same for resolveManifest interface ===" && \
sed -n '111p' packages/react-router-runtime/src/registry.ts && \
echo -e "\n=== resolveManifest implementation ===" && \
sed -n '493,495p' packages/react-router-runtime/src/registry.tsRepository: kibertoad/modular-react
Length of output: 626
🏁 Script executed:
# Let me get the resolveManifest implementation return type correctly
sed -n '493,495p' packages/react-router-runtime/src/registry.tsRepository: kibertoad/modular-react
Length of output: 108
🏁 Script executed:
# Find where resolveManifest is implemented in the registry object
rg -n "resolveManifest\(" packages/react-router-runtime/src/registry.ts | grep -v "//"Repository: kibertoad/modular-react
Length of output: 968
🏁 Script executed:
# Get the exact resolveManifest implementation return type
sed -n '528,532p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 310
🏁 Script executed:
# Get the resolveManifest function signature (line 486-488)
sed -n '486,490p' packages/react-router-runtime/src/registry.ts | cat -nRepository: kibertoad/modular-react
Length of output: 318
Fix type assertion to use actual plugin runtimes instead of Record<string, unknown>.
The interface declarations promise PluginRuntimesOf<TPlugins> for both resolve() and resolveManifest(), but the implementations incorrectly assert the type as ApplicationManifest<..., Record<string, unknown>> and ResolvedManifest<..., Record<string, unknown>>. This breaks the conditional type safety on journeys.
When TExtensions = Record<string, unknown>, the conditional type TExtensions extends { journeys: infer R } ? R : never always evaluates true, returning unknown instead of never. This means the convenience alias silently accepts manifest.journeys access regardless of whether the journeys plugin was registered, causing runtime crashes when accessing undefined.
Change the type assertions at lines 477–481 and 551–555 to use PluginRuntimesOf<TPlugins> (or the corresponding generic parameter) so the conditional type properly returns never when the plugin isn't loaded, forcing compile-time guards at call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-router-runtime/src/registry.ts` around lines 475 - 481, The
current assertions cast the manifest's extensions to Record<string, unknown>
causing the conditional type for journeys to resolve to unknown; update the two
incorrect casts so that where you currently assert ApplicationManifest<...,
Record<string, unknown>> and ResolvedManifest<..., Record<string, unknown>> you
instead use the actual plugin runtimes type (PluginRuntimesOf<TPlugins> or the
corresponding generic TExtensions/PluginRuntimes type parameter) so that the
conditional type for journeys evaluates correctly to the inferred type or never;
specifically change the assertions that build the returned object (the
properties moduleDescriptors, extensions, journeys) inside resolve() and
resolveManifest() to use PluginRuntimesOf<TPlugins> (or the generic runtime
type) for the extensions generic to restore proper compile-time guarding when
the journeys plugin is absent.
There was a problem hiding this comment.
Intentional design - the private createRegistry implementation doesn't thread TPlugins through, so the internal return types use Record<string, unknown> and the journeys field is as never. The public surface's correct typing comes from the outer as unknown as ModuleRegistry<..., readonly []> cast at registry.ts:587 - consumers see PluginRuntimesOf<TPlugins> from ModuleRegistry, so the conditional TExtensions extends { journeys: infer R } ? R : never resolves correctly at call sites. Inline comments at registry.ts:500-506 and :576-577 describe this split.
There was a problem hiding this comment.
Rate Limit Exceeded
@kibertoad have exceeded the limit for the number of chat messages per hour. Please wait 57 minutes and 8 seconds before sending another message.
- redact customerId from onAbandon abort payload (RR + TSR examples) — full state contained user-identifying fields that would leak into analytics sinks by default - give persisted tabs collision-free ids via crypto.randomUUID — the old module-level counter reset to 1 on reload and clobbered persisted tab ids - invoke vite through pnpm exec in playwright configs and fix the misleading "after pnpm build" comment (it's actually the dev server) - drop a stale filtering comment in the RR smoke spec that described a denylist that didn't exist - add a `text` language hint to the RR example README code fence Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Runtime
- runtime.hydrate(): clean up the half-built placeholder if hydrateInto
throws, mirroring the sync-start path. Without this, a retry hit the
"already in memory" guard and forget() was a no-op (non-terminal
status), effectively leaking the record.
- bindStepCallbacks: implement the JSDoc-documented fallback on
`JourneyRuntimeOptions.modules` — when a runtime is built without a
module descriptor but the journey transition declares allowBack: true,
treat the mode as 'preserve-state' so goBack stays wired. Makes the
headless simulator's goBack work out of the box (README example) and
matches what the public docs already promise.
- outlet: replace `React.ComponentType<any>` with the already-imported
`ComponentType<any>` — React namespace isn't imported in this file.
Validation
- validateJourneyContracts now guards null/non-object inner values in
the transitions map so malformed `{ transitions: { foo: null } }`
produces an issue instead of a TypeError that short-circuits the
accumulate-all-issues contract.
Testing
- createTestHarness.goBack now throws a readable error when back is
unavailable instead of silently no-oping. The previous optional chain
let `goBack walks back…` style tests "pass" when wiring was broken.
- README: drop the stale getInternals entry (not exported) and add
`createTestHarness` / `JourneyTestHarness` to the testing table.
RR + TSR runtime registries
- `plugin.onResolve({ debug })` now mirrors the journeys runtime's own
environment-based default (NODE_ENV !== "production") instead of
hardcoded false, so the journeys plugin emits verbose dev logs in
development without `journeysPlugin({ debug: true })` per site.
- `journeys: assembly.extensions.journeys as never` at the extension
cast point, with a comment explaining that the public `.journeys`
type comes from `PluginRuntimesOf<TPlugins>` via the outer registry
cast — the previous `as Record<string, unknown>` shape was misleading
and (incorrectly) implied a concrete shape at the impl site.
- Add a comment on `plugin.extend({ markDirty: () => {} })` clarifying
that markDirty is reserved by the plugin contract for future
reactivity support; today it's an intentional no-op, not missed
wiring.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary
Introduces Journeys - an optional layer that composes existing modules into typed, serializable workflows with shared state. Modules stay journey-unaware; a journey owns transitions and state; the shell just mounts inside whatever container it already uses (tab, route, modal).
Zero impact on apps that don't use it. Everything is additive.
What's in the branch
New package
serialized-per-instance save queue with coalescing, pluggable persistence (keyFor / load / save / remove), idempotent
start()
Core additions (@modular-react/core)
vocabulary from typeof someModule
Router-runtime integration (both React Router and TanStack Router)
Docs
Example project
Notes for reviewers
wires localStorage.
instances. Edge cases documented in docs/journeys.md §12.
Summary by CodeRabbit
New Features
Documentation
Examples
Tests / CI