feat(react/future): add structuredStackflow API #649
Conversation
- Introduce structuredStackflow for hierarchical activity management - Support nested navigation with defineNavigation - Provide type-safe useFlow hook for structured activities - Include built-in route extraction helper createRoutesFromActivities
|
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a typed activity/navigation DSL and a structured stack navigation implementation for React: new factories to define activities and navigations, a structuredStackflow factory that builds a stack-based navigator with typed actions, data loaders with caching, plugin integration, and route generation utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant defineActivity as defineActivity<br/>(Factory)
participant defineNavigation as defineNavigation<br/>(Factory)
participant structuredStackflow as structuredStackflow<br/>(Factory)
participant Stack as Stack Component
participant CoreStore as CoreStore +<br/>Plugins
Dev->>defineActivity: defineActivity("screen1")
defineActivity-->>Dev: returns function
Dev->>defineActivity: pass ActivityDefinitionInput<br/>(component, route, loader, transition)
defineActivity-->>Dev: ActivityDefinitionOutput<"screen1", TParams>
Dev->>defineNavigation: defineNavigation("root")
defineNavigation-->>Dev: returns function
Dev->>defineNavigation: pass {activities, initialActivity}
defineNavigation-->>Dev: NavigationDefinitionOutput
Dev->>structuredStackflow: pass StructuredStackflowInput<br/>(activities, initialActivity, plugins)
structuredStackflow->>structuredStackflow: flattenActivities()<br/>createRoutesFromActivities()
structuredStackflow->>structuredStackflow: setup loaderDataCacheMap<br/>initialize CoreStore
structuredStackflow->>structuredStackflow: register plugins<br/>(including loader plugin)
structuredStackflow-->>Dev: StructuredStackflowOutput<br/>(Stack, actions, hooks)
Dev->>Stack: render Stack component<br/>with ConfigProvider, PluginsProvider, CoreProvider
Stack->>CoreStore: dispatch Initialized<br/>ActivityRegistered events
Stack->>Stack: render current activity<br/>via ActivityComponentMapProvider + DataLoaderProvider
Stack-->>Dev: rendered navigation UI
sequenceDiagram
participant User as User
participant TypedActions as TypedActions<br/>(actions.push/replace)
participant LoaderPlugin as Loader Plugin
participant DataCache as LoaderDataCacheMap
participant Renderer as Stack Renderer
User->>TypedActions: actions.push("nextScreen", params, options)
TypedActions->>LoaderPlugin: trigger loader plugin
LoaderPlugin->>DataCache: check cache by param equality
alt Cache Hit
DataCache-->>LoaderPlugin: cached data
else Cache Miss
LoaderPlugin->>LoaderPlugin: invoke loader(params, config)
LoaderPlugin->>DataCache: store result + timestamp
end
LoaderPlugin-->>TypedActions: data ready
TypedActions->>Renderer: dispatch state update<br/>(new activity, transition animation)
Renderer->>Renderer: render new activity component<br/>with loaded data
Renderer-->>User: activity displayed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Areas to review closely:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
integrations/react/src/future/defineActivity.ts (1)
50-79: Consider tighteningDestinationsMaptyping to reduceanyusage
DestinationsMapandisNavigationDefinitioncurrently rely onActivityDefinitionOutput<string, any> | NavigationDefinitionOutput<string, any>, which weakens type safety at the boundary. You could consider:
- Switching
anytounknownwhere possible, and- Threading more specific generics through
DestinationsMap/NavigationDefinitionOutputso downstream consumers don’t have to immediately cast back toany.This would keep the runtime behavior the same while providing stricter compile-time guarantees.
integrations/react/src/future/structuredStackflow.tsx (2)
171-239: Loader caching is solid; consider tightening typing and simplifying the error pathThe per-activity cache keyed by deep-equal
paramsand the “max age after resolution” behavior are well thought out. A couple of small improvements:
- Typing: instead of
(activityConfig.loader as any)?.loaderCacheMaxAge, consider extending the loader type:export type ActivityLoader<TParams> = (args: {...}) => unknown & { loaderCacheMaxAge?: number; };(or an intersection type) so
loaderCacheMaxAgeis discoverable and type-safe.
- Error handler: in
Promise.resolve(loaderData).then(clearCacheAfterMaxAge, (error) => { clearCache(); throw error; });the
throw erroronly affects the chained promise, not the originalloaderDatathat callers await. You can simply:Promise.resolve(loaderData).then( clearCacheAfterMaxAge, () => clearCache(), );Callers will still see the original rejection, and you avoid creating an extra unobserved rejected promise.
356-435: Stack initialization and store reuse are reasonable; clarify “initial” props semanticsThe
Stackcomponent’suseMemocalls with empty dependency arrays intentionally treat:
initialContext, andinitialLoaderDataas one-shot “initialization” values rather than reactive props, and the core store is created once (reusing a previous store in the browser via
makeRef). This is a good fit for the typical “configure once at app bootstrap” pattern.If you ever expect
initialContext/initialLoaderDatato change over the lifetime of the app (e.g., hot reconfiguration), they’ll currently be ignored after the first render; in that case you’d need to include them in the memo deps and decide how to reinitialize the store. For now, consider documenting these props as immutable boot-time inputs to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
integrations/react/src/future/defineActivity.ts(1 hunks)integrations/react/src/future/index.ts(1 hunks)integrations/react/src/future/structuredStackflow.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write source in TypeScript with strict typing enabled across the codebase
Files:
integrations/react/src/future/index.tsintegrations/react/src/future/structuredStackflow.tsxintegrations/react/src/future/defineActivity.ts
integrations/react/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use .tsx files for React components and JSX in the React integration package
Files:
integrations/react/src/future/structuredStackflow.tsx
🔇 Additional comments (4)
integrations/react/src/future/defineActivity.ts (1)
3-47: Activity/loader definitions anddefineActivitybuilder look consistent
ActivityRoute,ActivityLoader, and theActivityDefinitionInput/Outputshapes line up well with howstructuredStackflowlater consumes routes/loaders and components, anddefineActivitycorrectly preserves theTParamsgeneric and binds it toInternalActivityComponentType<TParams>. I don’t see correctness issues here.integrations/react/src/future/structuredStackflow.tsx (2)
94-141: Activity flattening and route extraction logic look correct and robust
flattenActivitiescorrectly recurses through nestedNavigationDefinitionOutputvalues, reusing a sharedvisitedNamesset to enforce global uniqueness bydef.nameand throwing a clear error on duplicates.createRoutesFromActivitiesmirrors this traversal to build a flat{ [activityName]: route }map, which should work well for plugins like history-sync. No issues spotted in the traversal or error handling.
281-334: Typed actions andpop/canGoBackbehavior look correct
createActionsrespects the typed activity name/params, generates freshactivityIds by default, and forwardsanimate: falseintoskipEnterActiveState. Thepopoverload is handled cleanly by normalizing arguments, and the loop that setsskipExitActiveStateonly for the first popped activity when animating matches typical UX.canGoBack’s check against activities in"enter-done"/"enter-active"also seems aligned with stack semantics.integrations/react/src/future/index.ts (1)
19-20: Re-exportingdefineActivityandstructuredStackflowfrom the future bundle makes senseSurfacing these two modules from the
futureindex aligns with the new structured navigation API and keeps the public entrypoint coherent. No issues here.
- AllActivityNames extracts def.name instead of map keys - GetActivityDefinition searches by name, not key - Added runtime validation for key/name mismatches Fixes type/runtime mismatch in activity resolution.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
integrations/react/src/future/structuredStackflow.tsx (1)
30-37: Type/runtime activity name alignment + uniqueness checks look correct nowThe combination of:
AllActivityNamesderiving names fromActivityDefinitionOutput<infer N, any>instead of map keys, andflattenActivitiesenforcingkey === def.nameand rejecting duplicatedef.namevalues across the entire hierarchyremoves the earlier risk of type-level names diverging from runtime registration and from what
loadData/activitiesConfigactually use. This ensuresinitialActivity/push/replacenames, route generation, and loader lookup all consistently operate ondefineActivity’snameparameter and that duplicates are caught eagerly.Also applies to: 91-128
🧹 Nitpick comments (3)
integrations/react/src/future/structuredStackflow.tsx (3)
131-145: ReuseflattenActivities(or its validations) increateRoutesFromActivities
createRoutesFromActivitieswalks the tree independently and only usesdef.name/def.route, so it doesn’t benefit from the key/name equality and duplicate-name checks inflattenActivities. If someone callscreateRoutesFromActivitieson a destinations tree before (or even without) creating a stack, duplicate activity names will silently result in last-write-wins route entries.Consider either:
- building routes from
flattenActivities(activities)(which already enforces invariants), e.g.:-export function createRoutesFromActivities( - activities: DestinationsMap, - routes: Record<string, any> = {}, -): Record<string, any> { - for (const [, def] of Object.entries(activities)) { - if (isNavigationDefinition(def)) { - createRoutesFromActivities(def.activities, routes); - } else { - if (def.route) { - routes[def.name] = def.route; - } - } - } - return routes; -} +export function createRoutesFromActivities( + activities: DestinationsMap, + routes: Record<string, any> = {}, +): Record<string, any> { + for (const def of flattenActivities(activities)) { + if (def.route) { + routes[def.name] = def.route; + } + } + return routes; +}or
- mirroring the duplicate-name detection logic locally.
This keeps the “unique names across the app” guarantee consistent for both the stack and route helpers.
162-173: Minor: simplifyactivitiesConfig/componentsMapconstruction
flattenedActivitiesalready materializes{ name, route, loader, component, ... }. When derivingactivitiesConfig, you re‑spread and then re‑assign the same properties:const activitiesConfig = flattenedActivities.map( ({ component, ...activity }) => ({ ...activity, name: activity.name, route: activity.route, loader: activity.loader, }), );This can be simplified to just drop the component and keep the rest as‑is:
- const activitiesConfig = flattenedActivities.map( - ({ component, ...activity }) => ({ - ...activity, - name: activity.name, - route: activity.route, - loader: activity.loader, - }), - ); + const activitiesConfig = flattenedActivities.map( + ({ component, ...activity }) => activity, + );
componentsMapis already cleanly derived fromflattenedActivities, so this keeps both paths straightforward and avoids redundant property copies.
360-369:initialContextmemo ignores prop changes – verify this is intentional
initialContextis computed fromprops.initialContext/props.initialLoaderDatabut memoized with an empty dependency array:const initialContext = useMemo( () => ({ ...props.initialContext, ...(props.initialLoaderData ? { initialLoaderData: props.initialLoaderData } : null), }), [], );This means subsequent prop updates to
initialContextorinitialLoaderDataare intentionally ignored after the first render. If the semantics are truly “initial-only” (similar to aninitialRoute), this is fine; otherwise, consider adding the props as dependencies or documenting that they’re only read once.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integrations/react/src/future/structuredStackflow.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write source in TypeScript with strict typing enabled across the codebase
Files:
integrations/react/src/future/structuredStackflow.tsx
integrations/react/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use .tsx files for React components and JSX in the React integration package
Files:
integrations/react/src/future/structuredStackflow.tsx
| const loaderDataCacheMap = new Map< | ||
| string, | ||
| { params: Record<string, unknown>; data: unknown }[] | ||
| >(); | ||
|
|
||
| const loadData = ( | ||
| activityName: string, | ||
| activityParams: Record<string, unknown>, | ||
| ) => { | ||
| const cache = loaderDataCacheMap.get(activityName); | ||
| const cacheEntry = cache?.find((entry) => | ||
| isEqual(entry.params, activityParams), | ||
| ); | ||
|
|
||
| if (cacheEntry) { | ||
| return cacheEntry.data; | ||
| } | ||
|
|
||
| const activityConfig = activitiesConfig.find( | ||
| (activity) => activity.name === activityName, | ||
| ); | ||
|
|
||
| if (!activityConfig) { | ||
| throw new Error(`Activity ${activityName} is not registered.`); | ||
| } | ||
|
|
||
| const loaderData = activityConfig.loader?.({ | ||
| params: activityParams, | ||
| config: { | ||
| activities: activitiesConfig, | ||
| transitionDuration, | ||
| }, | ||
| }); | ||
|
|
||
| const newCacheEntry = { | ||
| params: activityParams, | ||
| data: loaderData, | ||
| }; | ||
|
|
||
| if (cache) { | ||
| cache.push(newCacheEntry); | ||
| } else { | ||
| loaderDataCacheMap.set(activityName, [newCacheEntry]); | ||
| } | ||
|
|
||
| const clearCache = () => { | ||
| const cache = loaderDataCacheMap.get(activityName); | ||
| if (!cache) return; | ||
| loaderDataCacheMap.set( | ||
| activityName, | ||
| cache.filter((entry) => entry !== newCacheEntry), | ||
| ); | ||
| }; | ||
|
|
||
| const clearCacheAfterMaxAge = () => { | ||
| setTimeout( | ||
| clearCache, | ||
| (activityConfig.loader as any)?.loaderCacheMaxAge ?? | ||
| DEFAULT_LOADER_CACHE_MAX_AGE, | ||
| ); | ||
| }; | ||
|
|
||
| Promise.resolve(loaderData).then(clearCacheAfterMaxAge, (error) => { | ||
| clearCache(); | ||
| throw error; | ||
| }); | ||
|
|
||
| return loaderData; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid throw inside the Promise rejection handler in loadData (prevents unhandled rejections)
In loadData, this fragment:
Promise.resolve(loaderData).then(clearCacheAfterMaxAge, (error) => {
clearCache();
throw error;
});attaches a rejection handler to loaderData and then rethrows inside that handler. Since the promise returned from .then(...) is not observed anywhere, a rejected loader promise will typically:
- still reject as expected for its direct consumers, and
- also create a second, unhandled rejected promise from
.then(...)(the rethrow), which can show up as unhandled‑rejection noise in browsers/Node.
You can clear the cache on error without rethrowing here, letting the original loaderData promise carry the rejection to callers:
- Promise.resolve(loaderData).then(clearCacheAfterMaxAge, (error) => {
- clearCache();
- throw error;
- });
+ Promise.resolve(loaderData).then(
+ clearCacheAfterMaxAge,
+ () => {
+ clearCache();
+ },
+ );This preserves behavior for callers of loadData while avoiding extra unhandled rejections.
1 participant
Summary
This PR introduces the structuredStackflow API, a new way to define and manage activities hierarchically, similar to Jetpack Compose Navigation.
Motivation
As applications grow, managing a flat list of activities becomes difficult. This new API allows developers to group related activities (e.g.,
Auth,Settings) using defineNavigation, improving code organization and maintainability.Features
createRoutesFromActivitieshelper to easily integrate with plugins likehistorySyncPluginwithout manual route duplication.Usage Example