feat(journeys,core): fix defineModule + derive step ordering/progress (feedback items 3 & 4) - #83
Merged
Merged
Conversation
… (feedback items 3 & 4)
Implements the next two items from docs/consumer-feedback-production-app.md.
Item 3 — defineModule usable by real apps (all five variants:
@modular-frontend/core + the four router cores):
- Infer a trailing TDescriptor from the argument and return it verbatim, so
entryPoints/exitPoints keep their literal keys instead of widening. typeof
someModule now drops into a journey TransitionMap/StepSpec with zero casts.
- Infer TNavItem from the navigation array (descriptor & { navigation?:
readonly TNavItem[] }), defaulting to NavigationItem only when absent, so
function-form `to: (ctx) => ...` type-checks with no generics while the
inferred-narrow item stays assignable to a NavigationItem-typed register().
Item 4 — derive step ordering + progress from the transition graph:
- resolveStepSequence(definition, options?) walks the static defineTransition
targets graph and returns the ordered step list (linear, or branch-selected).
- JourneyDefinition.steps: per-step { path, progressLabel } metadata, keyed and
type-checked against the real modules/entries (single source of truth).
- useJourneyProgress on React and Vue returns { index, total, label, steps } —
the stepCount that JourneyHost (item 2) deferred, now graph-derived.
Adds acceptance + unit + type tests across the touched packages; updates the
changelog and marks items 3 & 4 shipped in the tracker.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KThnW7wGDMovXu8JdV4q2E
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds literal-preserving ChangesModule type inference
Journey step resolution
React and Vue progress bindings
Documentation and CLI support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JourneyProvider
participant useJourneyProgress
participant resolveStepSequence
participant JourneyRuntime
JourneyProvider->>useJourneyProgress: provide runtime
useJourneyProgress->>resolveStepSequence: resolve definition and sequence options
resolveStepSequence-->>useJourneyProgress: resolved steps and metadata
useJourneyProgress->>JourneyRuntime: read instance snapshot
JourneyRuntime-->>useJourneyProgress: history and current step
useJourneyProgress-->>JourneyProvider: index, total, label, steps
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…dback-items-9jen4y
Follow-up to the item 3/4 PR, addressing code-review findings: - resolveStepSequence: match the `branch` resolver's return against the fork's declared `targets` by module+entry, so a ref that isn't a real target (or `undefined`) stops the walk instead of being followed blindly. - resolveStepSequence docs: note that `wildcard` transitions are not walked, and that the start step is derived by invoking `initialState`/`start` (which must be safe to call with the provided input). - useJourneyProgress (React + Vue): correct the `total` JSDoc — it is the best-effort statically-resolved spine length (partial on forks/unannotated/ maxSteps), always >= 1 for a derivable start, `null` only for an empty sequence — not the previously-documented "null when no instance / unannotated". Document that `index` (live) can reach/exceed `total` (static), and that `options.sequence` should be referentially stable. - define-module type-test comments: the function-form `to` fix infers `TNavItem` from the `navigation` array; it does not change the default to `NavigationItemBase`. Corrected the misleading comments. - Add a test for the branch-resolver-returns-foreign-ref case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqsgpXv75wiufKKkjTNLXz
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend-core/src/define-module.ts (1)
55-62: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep
TNavIteminferable for typed-shell calls. Zero-generic calls already infer fromnavigation, butdefineModule<AppDeps, AppSlots>(...)locksTNavItemtoNavigationItem, so function-formtostops type-checking unless callers spell the full nav item type. Rework the generic order/overload inpackages/frontend-core/src/define-module.tsandpackages/react-router-core/src/define-module.ts, and add a partial-generic type test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/frontend-core/src/define-module.ts` around lines 55 - 62, Keep TNavItem inferable when callers provide only partial generics to defineModule. Update the generic ordering or overloads in packages/frontend-core/src/define-module.ts and packages/react-router-core/src/define-module.ts so defineModule<AppDeps, AppSlots>(...) infers function-form navigation to types instead of defaulting TNavItem to NavigationItem; add a type test covering this partial-generic call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/journeys-engine/src/resolve-step-sequence.ts`:
- Around line 25-37: Update ResolveStepSequenceOptions and the
resolveStepSequence signature so non-void TInput requires either input or start,
while void-input journeys retain the optional input behavior. Preserve start’s
precedence over input, and add a type-level regression test proving
resolveStepSequence(definition) is rejected when initialState requires input.
---
Outside diff comments:
In `@packages/frontend-core/src/define-module.ts`:
- Around line 55-62: Keep TNavItem inferable when callers provide only partial
generics to defineModule. Update the generic ordering or overloads in
packages/frontend-core/src/define-module.ts and
packages/react-router-core/src/define-module.ts so defineModule<AppDeps,
AppSlots>(...) infers function-form navigation to types instead of defaulting
TNavItem to NavigationItem; add a type test covering this partial-generic call.
🪄 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: 00aa71cd-b653-41e2-bd61-3c4d3ba2b88d
📒 Files selected for processing (25)
CHANGELOG.mddocs/consumer-feedback-production-app.mdpackages/angular-router-core/src/define-module.test-d.tspackages/angular-router-core/src/define-module.tspackages/frontend-core/src/define-module.test-d.tspackages/frontend-core/src/define-module.tspackages/frontend-core/src/index.tspackages/frontend-core/src/journey-contracts.tspackages/journeys-engine/src/index.tspackages/journeys-engine/src/resolve-step-sequence.test-d.tspackages/journeys-engine/src/resolve-step-sequence.test.tspackages/journeys-engine/src/resolve-step-sequence.tspackages/journeys-engine/src/types.tspackages/journeys/src/index.tspackages/journeys/src/use-journey-progress.test.tsxpackages/journeys/src/use-journey-progress.tspackages/react-router-core/src/define-module.tspackages/tanstack-router-core/src/define-module.test-d.tspackages/tanstack-router-core/src/define-module.tspackages/tanstack-router-core/vitest.config.tspackages/vue-core/src/define-module.test-d.tspackages/vue-core/src/define-module.tspackages/vue-journeys/src/index.tspackages/vue-journeys/src/use-journey-progress.test.tspackages/vue-journeys/src/use-journey-progress.ts
… resolveStepSequence Address CodeRabbit review on PR #83. `resolveStepSequence(definition)` previously type-checked for journeys whose `initialState` needs an input, then called `initialState(undefined)` when `input` was omitted. Model the options so a non-void `TInput` requires either `input` (handed to the factories) or `start` (naming the first step, skipping them), while void-input journeys keep fully-optional options. The trailing options argument becomes required for non-void input via `StepSequenceOptionsArg`, so `resolveStepSequence(def)` is now a compile error precisely when it would have called `initialState(undefined)`. Propagate the same input-awareness through `useJourneyProgress` (React and Vue): `UseJourneyProgressOptions.sequence` is required (with `input`/`start`) for non-void journeys, keeping the forwarding to `resolveStepSequence` sound. Add type-level regression tests proving the bare non-void call, empty options, walk-only options, and wrong-typed `input` are all rejected, while void-input journeys and `input`/`start` forms are accepted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTynWaEhJz9FYF95seoPvH
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 32: Update the CHANGELOG wording in the resolveStepSequence description
by replacing “Requires annotated (`defineTransition`) handlers” with “Requires
handlers annotated with `defineTransition`,” without changing the surrounding
behavior or API details.
🪄 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: 6e64167f-3abf-496a-bff2-d28052c59d25
📒 Files selected for processing (6)
CHANGELOG.mdpackages/journeys-engine/src/index.tspackages/journeys-engine/src/resolve-step-sequence.test-d.tspackages/journeys-engine/src/resolve-step-sequence.tspackages/journeys/src/use-journey-progress.tspackages/vue-journeys/src/use-journey-progress.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/journeys-engine/src/index.ts
- packages/journeys/src/use-journey-progress.ts
- packages/journeys-engine/src/resolve-step-sequence.ts
Address CodeRabbit nit on PR #83: "Requires annotated (defineTransition) handlers" → "Requires handlers annotated with defineTransition". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTynWaEhJz9FYF95seoPvH
…inferred Address CodeRabbit outside-diff review on PR #83. With partial explicit generics, `defineModule<AppDeps, AppSlots>(descriptor)` locked `TNavItem` to its `NavigationItem` default (TypeScript can't partially infer a call's type arguments — spelling some forces the rest to their defaults), so function-form `to: (ctx) => ...` stopped type-checking unless the caller also spelled the full nav-item generic. That partial form is exactly what the scaffolder templates and core READMEs shipped. Add a curried overload — `defineModule<AppDeps, AppSlots>()(descriptor)` — to all five cores (frontend-core + the four router cores). The first, empty call pins the app-wide TSharedDependencies / TSlots (and optional TMeta); the second infers TNavItem + TDescriptor from the descriptor, so a typed shell fixes deps/slots while function-form `to` stays inferred. Same idiom as defineJourney. The direct `defineModule(descriptor)` (zero generics) and fully-explicit four-generic forms are unchanged. - Type tests: curried function-form/plain-string `to` in frontend-core; curried literal-preservation in tanstack-router-core; curried `ctx` narrowing in vue-core. - Migrate scaffolder templates (react-router / tanstack / vue), the five core READMEs, the getting-started / shell-patterns / framework-mode / remote- capability docs, and the example modules to the curried form; refresh CLI scaffolding snapshots. Full-repo typecheck (147/147) and the five core + three CLI test suites pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTynWaEhJz9FYF95seoPvH
Owner
Author
|
Addressed the outside-diff finding on Confirmed the report: Fix: added a curried overload — Added partial-generic type tests (frontend-core, tanstack-router-core, vue-core), and migrated the scaffolder templates, the five core READMEs, the guide/shell-pattern docs, and the example modules to the curried form (CLI snapshots refreshed). Full-repo Generated by Claude Code |
Documentation/examples/CLI review pass:
- README: correct the per-family version claims in Project status
(react-router-modules & tanstack-react-modules: core/runtime v2.x,
cli/testing v3.x; @modular-react core/react v2.x, testing v1.x), and drop
the stale "some examples declare library deps with semver ranges" clause —
every example pins `workspace:*`.
- docs/remote-capability-manifests.md: retarget five broken source links from
the non-existent `packages/core/src/{types,remote-manifest}.ts` to their real
home in `packages/frontend-core/src/`.
- docs/navigation.md link fix: promote "Journey-contributed nav" in the
journeys README to a heading so the existing `#journey-contributed-nav`
anchor resolves.
- examples/README: add the undocumented examples to the index tree and
descriptions — integration-setup-journey, journey-invoke (RR + TSR),
tanstack remote-capabilities, and the catalog demo portal.
- CLI: genericize the shared `create store` help text (was hardcoded
"Zustand", wrong for the Vue CLI whose store is `createStore`); add
@modular-vue/cli to cli-core's consumer list (README + package.json); add
the now-required `scaffold` field to cli-core's preset example; document the
`serve` subcommand in the catalog README's CLI section; refresh the catalog
package status marker (v0.2 → v1.x).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UTynWaEhJz9FYF95seoPvH
Follow-up usefulness pass on the documentation review: - README: add the experimental Angular family (@angular-router-modules/core, @modular-angular/angular) to Project status and a new "Angular (experimental)" Packages subsection — marked v0.1.x / core-only and explicitly not yet a peer of the router integrations, mirroring how the Nuxt integration is noted. - getting-started (React Router, TanStack Router): name the shipped journey features the journeys section previously glossed as "the runtime hooks" — <JourneyHost>/useJourneyHost, useJourneyProgress, and useJourneySync. - getting-started (Vue Router): note the matching Vue journey composables in the closing further-reading list. Verified every referenced export exists in @modular-react/journeys and @modular-vue/journeys. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTynWaEhJz9FYF95seoPvH
…cts-83-gjee83 # Conflicts: # README.md # docs/remote-capability-manifests.md
kibertoad
added a commit
that referenced
this pull request
Jul 19, 2026
…time.discard, template-idiomatic host outlet (#91) * feat(vue,journeys): downstream feedback — plugin-typed manifests, runtime.discard, template-idiomatic host outlet A second consumer feedback round (Vue + Nuxt), triaged critically: some items were already solved and only needed verification/docs; the rest are focused, backward-compatible changes. - installModularApp (@modular-vue/nuxt) and createModularApp (@modular-vue/runtime) no longer erase plugin-extension types. They inferred over ModuleRegistry<…, any> and returned ApplicationManifest<TSlots, TNavItem>, collapsing manifest.journeys to `unknown` and forcing a cast. They now infer the extension map from the registry's resolve() return (the plugin tuple hides behind PluginRuntimesOf and isn't invertible; the extensions ride plainly on ApplicationManifest's third arg), so a journeysPlugin() registry yields manifest.journeys: JourneyRuntime with no cast. Plugin-less callers unaffected. - New JourneyRuntime.discard(id, reason?): end(force) + forget in one call, so a Cancel button drops the persisted blob without re-deriving keyFor(input). Names the "throw it away" intent — the counterpart to a soft close that keeps the blob by not ending the instance. Runs onAbandon and forces the outcome terminal. - <JourneyHost> slot `outlet` is now a stable functional component (was a raw VNode), so the documented `<component :is="outlet" />` actually renders and patches — not remounts — across steps. Slot also exposes `runtime`, enabling the idiomatic `<JourneyOutlet :instance-id :runtime />` alternative. - Verified (no code change): inline `buildInput` entries authored directly inside defineModule (no defineEntry wrapper) now flow buildInput presence into typeof mod, so StepSpec.input goes optional — the behavior a consumer flagged to re-check post-#83. Locked in with regression tests. - Docs + nuxt-modal-journey example: close-vs-cancel contract with runtime.discard recipe, functional-component outlet, appProvides auto-threading (provideJourney- Runtime unnecessary in the plugin path), and the defineNuxtPlugin TS7022 annotation footgun. Example gains a "Cancel (discard)" button + e2e case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TuYsjBFX8hKizbY5Yep1Bm * refactor(vue,journeys): address review feedback — dedup InstallableRegistry, doc precision - Consolidate the duplicated `InstallableRegistry` helper type: export it once from `@modular-vue/runtime` and import it in `@modular-vue/nuxt` instead of re-declaring an identical copy. Drops the now-unused RegistryPlugin / ModuleRegistry / ResolveOptions imports from the Nuxt installer. - Clarify `JourneyRuntime.discard` docstring: the force-end cascades to (and removes the blob of) an active child, but `forget` drops only the target record — a cascaded child is left as a terminal record for normal terminal cleanup, exactly as a plain `end` leaves it. - Fix `keyFor(frameId)` → `keyFor(input)` in the nuxt-modal-journey example so it matches the canonical `persistence.keyFor({ journeyId, input })` signature used everywhere else. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Hy8mBdWa8GpEg5bmV8NZE --------- Co-authored-by: Claude <noreply@anthropic.com>
…dback-items-9jen4y # Conflicts: # examples/README.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/getting-started-vue-router.md (1)
26-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the introductory text for consistency.
This line updates the supported baseline to
vue-router ^5.0, but the introductory text on line 5 still assumes the user is familiar with "vue-router 4". Please update the prose on line 5 to maintain consistency with this new baseline.📝 Proposed fix for line 5
-assumes you already use (or are comfortable with) Vue 3 `<script setup>` and -vue-router 4, and want to split your app into self-contained feature modules. +assumes you already use (or are comfortable with) Vue 3 `<script setup>` and +vue-router 5, and want to split your app into self-contained feature modules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/getting-started-vue-router.md` at line 26, Update the introductory prose in the getting-started document to reference vue-router 5 instead of assuming familiarity with vue-router 4, keeping it consistent with the supported baseline stated in the dependency bullet.
🧹 Nitpick comments (1)
packages/tanstack-router-core/src/define-module.test-d.ts (1)
142-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate navigation array item.
There is a duplicate entry for the
"Plan"navigation item.♻️ Proposed fix
navigation: [ { label: "Plan", to: (ctx: { workspaceId: string }) => `/plan/${ctx.workspaceId}` }, - { label: "Plan", to: (ctx: { workspaceId: string }) => `/plan/${ctx.workspaceId}` }, ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tanstack-router-core/src/define-module.test-d.ts` around lines 142 - 144, Remove the duplicate "Plan" object from the navigation array in the affected define-module type test, leaving one entry with the existing label and to callback unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/getting-started-vue-router.md`:
- Line 26: Update the introductory prose in the getting-started document to
reference vue-router 5 instead of assuming familiarity with vue-router 4,
keeping it consistent with the supported baseline stated in the dependency
bullet.
---
Nitpick comments:
In `@packages/tanstack-router-core/src/define-module.test-d.ts`:
- Around line 142-144: Remove the duplicate "Plan" object from the navigation
array in the affected define-module type test, leaving one entry with the
existing label and to callback unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1704b63c-f70d-43e2-ba2d-736f3ebfabca
⛔ Files ignored due to path filters (3)
packages/react-router-cli/test/__snapshots__/cli.test.ts.snapis excluded by!**/*.snappackages/tanstack-router-cli/test/__snapshots__/cli.test.ts.snapis excluded by!**/*.snappackages/vue-cli/test/__snapshots__/cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (52)
CHANGELOG.mdREADME.mddocs/framework-mode-nuxt.mddocs/framework-mode-tanstack-router.mddocs/getting-started-react-router.mddocs/getting-started-tanstack-router.mddocs/getting-started-vue-router.mddocs/remote-capability-manifests.mddocs/shell-patterns-react-router.mddocs/shell-patterns-tanstack-router.mddocs/shell-patterns-vue-router.mddocs/shell-patterns.mddocs/sibling-modules-shared-screen.mdexamples/README.mdexamples/react-router/active-project-manifest/modules/integrations/src/index.tsexamples/react-router/customer-onboarding-journey/app-shared/src/index.tsexamples/react-router/integration-manager/modules/contentful/src/index.tsxexamples/react-router/integration-manager/modules/github/src/index.tsxexamples/react-router/integration-manager/modules/strapi/src/index.tsxexamples/react-router/remote-capabilities/modules/integrations/src/index.tsexamples/tanstack-router/customer-onboarding-journey/app-shared/src/index.tsexamples/tanstack-router/integration-manager/modules/contentful/src/index.tsxexamples/tanstack-router/integration-manager/modules/github/src/index.tsxexamples/tanstack-router/integration-manager/modules/strapi/src/index.tsxexamples/tanstack-router/remote-capabilities/modules/integration-catalog/src/index.tsexamples/vue/customer-onboarding-journey/app-shared/src/index.tsexamples/vue/integration-manager/modules/contentful/src/index.tsexamples/vue/integration-manager/modules/github/src/index.tsexamples/vue/integration-manager/modules/strapi/src/index.tspackages/angular-router-core/README.mdpackages/angular-router-core/src/define-module.tspackages/catalog/README.mdpackages/cli-core/README.mdpackages/cli-core/package.jsonpackages/cli-core/src/commands/create-store.tspackages/frontend-core/src/define-module.test-d.tspackages/frontend-core/src/define-module.tspackages/frontend-core/src/index.tspackages/frontend-core/src/journey-contracts.tspackages/journeys/README.mdpackages/react-router-cli/src/templates/module.tspackages/react-router-core/README.mdpackages/react-router-core/src/define-module.tspackages/tanstack-router-cli/src/templates/module.tspackages/tanstack-router-core/README.mdpackages/tanstack-router-core/src/define-module.test-d.tspackages/tanstack-router-core/src/define-module.tspackages/vue-cli/src/templates/module.tspackages/vue-core/README.mdpackages/vue-core/src/define-module.test-d.tspackages/vue-core/src/define-module.tspackages/vue-journeys/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/frontend-core/src/index.ts
- packages/vue-core/src/define-module.ts
- packages/vue-journeys/src/index.ts
- packages/react-router-core/src/define-module.ts
- packages/angular-router-core/src/define-module.ts
- packages/vue-core/src/define-module.test-d.ts
- packages/frontend-core/src/define-module.ts
casamitjana
reviewed
Jul 20, 2026
casamitjana
left a comment
casamitjana
left a comment
Collaborator
There was a problem hiding this comment.
Found four correctness issues in the journey progress and path behavior. Details inline.
Four correctness/typing fixes from PR review:
- resolveStepSequence: type `start`/`branch`/targets against the journey's
real (module, entry) vocabulary via a `TModules`-generic `StepSequenceRef`,
so a typo'd explicit start is a compile error instead of a fake step.
- resolveStepSequenceResult: new companion returning `{ steps, complete }`,
where `complete` is true only when the walk reaches a genuine end of the
flow (not a partial spine cut short by a fork, bare/wildcard step, invoke,
cycle, or maxSteps cap).
- useJourneyProgress (React + Vue): derive `total` from `complete` so a
partial spine yields `null` rather than a misleading "Step 2 of 1"; derive
`index` from the resolved-sequence position of the live step so it stays
correct under a maxHistory cap that trims `history` (fallback to
history.length only when the step is off the spine).
- stepPathFromDefinition: new opt-in helper building a `stepToPath` from a
definition's `steps[module][entry].path`, so a declared JourneyStepMeta.path
can actually drive the URL. Doc on `path` corrected to drop the false
"automatic override" claim.
Adds unit + type-level regression tests; updates CHANGELOG and READMEs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E8c6U5oHPaC9wWb6FFJxgt
casamitjana
reviewed
Jul 20, 2026
classifyStep only inspected exact transitions[module][entry], so a step with an exact `done -> complete` handler plus a wildcard `retry -> next` fall-through was marked complete: true. The runtime routes an exit with no exact handler through wildcardTransitions.byEntryAndExit then byExit, so firing that exit advances the flow and recreates "Step 2 of 1". Fold applicable wildcard handlers into terminal classification: when any wildcard that could fire from a step may advance (a bare handler, a forward step ref, or "invoke"), the step is no longer terminal, so resolveStepSequenceResult reports complete: false and useJourneyProgress surfaces total: null. Precedence (exact > byEntryAndExit > byExit) is respected, and a wildcard that only targets complete/abort still leaves a genuinely terminal step complete. Also correct the resolveStepSequence example doc to derive a "Step X of N" total from resolveStepSequenceResult gated on `complete`, not steps.length.
casamitjana
approved these changes
Jul 20, 2026
casamitjana
left a comment
casamitjana
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed the latest changes. All findings are addressed and CI is green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Implements the next two items from
docs/consumer-feedback-production-app.md(items 1 & 2 already shipped). Both are the friction points a real production consumer wrote glue around.Item 3 — make
defineModuleusable by real apps. The consumer abandoneddefineModuleentirely, carrying a copy-pasted apology in every module, because it (a) widened the literal shape ofentryPoints/exitPoints(breaking journey transition maps that referencetypeof someModule) and (b) defaulted its nav-item generic toNavigationItem, whosetois a plainstring, rejecting function-formto. Fixed in all five variants (@modular-frontend/core+ the four router cores):defineModulenow infers a trailingTDescriptorand returns it verbatim, soentryPoints/exitPointskeep their literal keys. The acceptance test is met:typeof someModuleis usable as aTModulesmember in a journeyTransitionMap/StepSpecwith zero casts, and exit handlers narrowoutputper exit.to—TNavItemis now inferred from thenavigationarray (descriptor & { navigation?: readonly TNavItem[] }), defaulting toNavigationItemonly when absent.to: (ctx) => …type-checks with no generics. Inference (rather than defaulting the slot to the wideNavigationItemBasebound) is deliberate: a fixed wide default made the returned descriptor unassignable to aNavigationItem-typedregister(), so the inferred-narrow item keeps registration working while still admitting the resolver form.Item 4 — derive step ordering and progress from the transition graph. The consumer encoded each flow twice: the transition map, plus a ~170-line hand-maintained ordered-step file (in three branch-variant copies) for URL segments and "Step X of N". Now derived from the graph:
resolveStepSequence(definition, options?)(@modular-frontend/journeys-engine) walks the staticdefineTransitiontargetsgraph and returns the ordered step list (linear, oroptions.branch-selected at forks). Requires annotated transitions; cycle-guarded; length-capped.JourneyDefinition.steps— per-step{ path, progressLabel }metadata, keyed and type-checked against the real modules/entries. Single source of truth beside the transitions.useJourneyProgress(instanceId, definition, options?)on React and Vue returns{ index, total, label, steps }—indexfrom the live instance, the rest fromresolveStepSequence. This is thestepCountthatJourneyHost(item 2) explicitly deferred to this item.How it was verified
pnpm typecheck— full repo, 145/145 tasks (including all example shells that consumedefineModule/register).pnpm test— full repo, 74/74 tasks. New tests:resolveStepSequenceunit + type tests (journeys-engine),useJourneyProgresscomponent tests (React + Vue), anddefineModuleacceptance type-tests provingtypeof modulein aTransitionMapwith zero casts (tanstack + frontend-core) — added avitest.config.tsto@tanstack-react-modules/coreso its.test-d.tsassertions run.oxlint .— exits clean (only pre-existing warnings);oxfmt --write .applied.CHANGELOG.mdupdated.Checklist
pnpm lintpasses (typecheck + oxfmt + oxlint).pnpm test).@modular-frontend/*(core + journeys-engine) and adds a React-binding capability (useJourneyProgress). The Vue family is updated in the same PR:@modular-vue/coregets the samedefineModulefix, and@modular-vue/journeysgetsuseJourneyProgress. No follow-up row needed.🤖 Generated with Claude Code
https://claude.ai/code/session_01KThnW7wGDMovXu8JdV4q2E
Generated by Claude Code
Summary by CodeRabbit
resolveStepSequence(definition, options?)(and result helpers) to compute an ordered, cycle-safe journey step sequence with fork/branch handling.JourneyDefinition.steps(path,progressLabel).useJourneyProgress()for React/Vue to expose{ index, total, label, steps }, plusstepPathFromDefinition.defineModuleTypeScript inference to preserve literal route keys and correctly infernavigation[].toresolver types.defineModule<...>()({ ... })call form.