feat(react, plugin-history-sync): Default history setup option for rich deep link experiences #610
Conversation
🦋 Changeset detectedLatest commit: da7bcd1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Deploying stackflow-demo with
|
| Latest commit: |
4711c2e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://528d3abb.stackflow-demo.pages.dev |
| Branch Preview URL: | https://deep-link-default-nav-contex.stackflow-demo.pages.dev |
commit: |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
stackflow-docs | da7bcd1 | Commit Preview URL | Sep 05 2025, 07:21 AM |
|
Warning Rate limit exceeded@ENvironmentSet has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a StructuredActivityComponent system and exposes it via the React integration; introduces a route-level defaultHistory option and queue-driven insertion in the history-sync plugin; refactors demo Article into structured pieces, adds a loading spinner, and adjusts loader and type mappings across integrations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Stack as Stackflow (React)
participant Renderer as PluginRenderer
participant SAC as StructuredActivityComponent
participant Loader as Loader (future)
participant React as React (Suspense/ErrorBoundary)
App->>Stack: navigate to Activity
Stack->>Renderer: render Activity
alt Monolithic component
Renderer->>Renderer: render <Activity params />
else Structured activity
Renderer->>SAC: detect content/layout/loading/error
par Prefetch resolution
Stack->>Loader: before-route: resolve content()/_load
Loader-->>Stack: pause until ready (if needed)
end
Renderer->>React: wrap with Suspense (Loading)
Renderer->>React: wrap with ErrorBoundary (ErrorHandler)
Renderer->>SAC: get Content component
Renderer-->>App: render <Layout?><Content/><Layout?>
end
sequenceDiagram
autonumber
participant Browser as Browser URL
participant Plugin as historySyncPlugin
participant Actions as Stackflow Actions
participant Queue as DefaultHistory Queue
participant Stack as Stack
Browser->>Plugin: onInit(currentPath)
Plugin->>Plugin: resolve route + params
Plugin->>Plugin: derive defaultHistory[]
alt has defaultHistory
Plugin->>Actions: dispatch initial Pushed (first entry, backdated)
Plugin->>Queue: enqueue pushes for remaining entries/steps
end
note over Plugin,Queue: Checkpoint when stack idle triggers flush
Plugin->>Actions: onChanged() -> flush queued tasks
Actions->>Stack: push/stepPush per queued entries
rect rgb(247,247,255)
note right of Plugin: Non-default push/replace/pop clears queue
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
ENvironmentSet
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
161-166: Hash path extraction can return empty string; use a safer fallbackWhen the URL has no hash, location.hash.split("#")[1] yields an empty string, not undefined, so the current code returns "". Prefer stripping the leading # and fallback to "/".
- if (options.useHash) { - return location.hash.split("#")[1] ?? "/"; - } + if (options.useHash) { + const hashed = location.hash.replace(/^#/, ""); + return hashed || "/"; + }
🧹 Nitpick comments (10)
.changeset/new-pandas-return.md (1)
5-5: Polish wording for clarity and consistency (“deep‑link”, option name).More explicit and grammatically tighter phrasing helps when reading release notes.
-Default history setup option for rich deep link experiences +Add defaultHistory route option to pre-seed stack for deep‑link experiences.changeset/chubby-pianos-heal.md (1)
5-5: Fix grammar in changeset description.Use active/imperative voice for release notes.
-Make lazy activity components can be forced to render +Allow lazy activity components to be forced to renderdemo/src/activities/Article.placeholder.css.ts (1)
3-8: Consider adding width to ensure consistent centering across layouts.If the parent container doesn’t stretch horizontally by default, adding width improves robustness.
export const container = style({ display: "flex", alignItems: "center", justifyContent: "center", + width: "100%", height: "100%", });integrations/react/src/__internal__/LazyActivityComponentType.ts (1)
2-7: Type shape LGTM; simplify generic constraint for readability.The current constraint is effectively “object-like.” Using
objectcommunicates intent more clearly without changing behavior.-export type LazyActivityComponentType<T extends { [K in keyof T]: any } = {}> = +export type LazyActivityComponentType<T extends object = {}> = ComponentType<{ params: T; shouldRenderImmediately?: boolean }> & { _load?: () => Promise<{ default: StaticActivityComponentType<T> }>; };demo/src/components/LoadingSpinner.tsx (1)
5-7: Add ARIA semantics to the spinner for accessibilityExpose the loading state to assistive technologies. A minimal enhancement is to mark it as a status region and provide an accessible label.
Apply this diff:
-const LoadingSpinner: FC = () => { - return <div className={css.spinner} />; -}; +const LoadingSpinner: FC<{ ariaLabel?: string }> = ({ ariaLabel = "Loading" }) => { + return ( + <div + className={css.spinner} + role="status" + aria-live="polite" + aria-label={ariaLabel} + /> + ); +};demo/src/components/LoadingSpinner.css.ts (1)
11-14: Prefer design tokens over hardcoded colors for consistency and themingReplace hex colors with Seed Design tokens to align with the rest of the demo and enable theme coherence.
Apply this diff in the style block:
- border: "4px solid #ccc", - borderTop: "4px solid #333", + border: `4px solid ${vars.$semantic.color.divider3}`, + borderTop: `4px solid ${vars.$scale.color.gray900}`,And add the token import:
// at the top of the file import { vars } from "@seed-design/design-token";integrations/react/src/__internal__/PluginRenderer.tsx (1)
40-49: Minor readability refactor: cache lazy context before branchFunctionally correct. Caching the computed context removes repetition and clarifies the conditional.
Apply this diff:
- let output: React.ReactNode = - "_load" in Activity && - typeof (activity.context as any)?.lazyActivityComponentRenderContext === "object" ? ( - <Activity - {...(activity.context as any)?.lazyActivityComponentRenderContext} - params={activity.params} - /> - ) : ( - <Activity params={activity.params} /> - ); + const lazyCtx = (activity.context as any)?.lazyActivityComponentRenderContext; + let output: React.ReactNode = + "_load" in Activity && typeof lazyCtx === "object" ? ( + <Activity {...lazyCtx} params={activity.params} /> + ) : ( + <Activity params={activity.params} /> + );extensions/plugin-history-sync/src/historySyncPlugin.tsx (3)
186-197: Prefer using makeTemplate.parse for params instead of manual UrlPattern parsingYou parse pathParams via a new UrlPattern instead of the already-constructed template, then merge with search params. This duplicates logic and can diverge from decode behavior (template.parse applies route.decode). If defaultHistory expects decoded values, this may be incorrect.
- If defaultHistory should receive decoded params: replace this block by using template.parse(currentPath) and fall back to search params only when null.
- If defaultHistory should receive raw string params: keep as-is, but consider adding a code comment clarifying the expectation to avoid regressions.
Suggested change (decoded-params version):
- const pattern = new UrlPattern( - `${targetActivityRoute.path}/`, - options.urlPatternOptions, - ); - const url = pathToUrl(currentPath); - const pathParams = pattern.match(url.pathname); - const searchParams = urlSearchParamsToMap(url.searchParams); - const params = { - ...searchParams, - ...pathParams, - }; + const template = makeTemplate( + targetActivityRoute, + options.urlPatternOptions, + ); + const parsed = template.parse(currentPath); + const params = + parsed ?? + urlSearchParamsToMap(pathToUrl(currentPath).searchParams);
286-301: Typo: enoughtPastTime → enoughPastTimeMinor readability nit.
- const enoughtPastTime = new Date().getTime() - MINUTE; + const enoughPastTime = new Date().getTime() - MINUTE; ... - eventDate: enoughtPastTime, + eventDate: enoughPastTime,
613-631: Guard readability: prefer negative checks over === falseThe path-injection guard is correct, but readability suffers from using in with === false. Consider a more idiomatic check.
- if ( - !actionParams.activityContext || - "path" in actionParams.activityContext === false - ) { + if ( + !actionParams.activityContext || + !("path" in actionParams.activityContext) + ) {Same comment applies to onBeforeReplace.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (15)
.changeset/chubby-pianos-heal.md(1 hunks).changeset/new-pandas-return.md(1 hunks)demo/src/activities/Article.placeholder.css.ts(1 hunks)demo/src/activities/Article.placeholder.tsx(1 hunks)demo/src/components/LoadingSpinner.css.ts(1 hunks)demo/src/components/LoadingSpinner.tsx(1 hunks)demo/src/stackflow/Stack.tsx(1 hunks)demo/src/stackflow/stackflow.config.ts(1 hunks)extensions/plugin-history-sync/src/RouteLike.ts(1 hunks)extensions/plugin-history-sync/src/historySyncPlugin.tsx(14 hunks)integrations/react/src/__internal__/LazyActivityComponentType.ts(1 hunks)integrations/react/src/__internal__/PluginRenderer.tsx(1 hunks)integrations/react/src/future/lazy.ts(0 hunks)integrations/react/src/future/lazy.tsx(1 hunks)integrations/react/src/future/loader/loaderPlugin.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- integrations/react/src/future/lazy.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
integrations/react/src/__internal__/PluginRenderer.tsx (1)
core/src/Stack.ts (1)
Activity(28-43)
integrations/react/src/future/lazy.tsx (2)
integrations/react/src/__internal__/StaticActivityComponentType.ts (1)
StaticActivityComponentType(3-5)integrations/react/src/__internal__/LazyActivityComponentType.ts (1)
LazyActivityComponentType(4-7)
integrations/react/src/future/loader/loaderPlugin.tsx (1)
core/src/utils/makeActions.ts (2)
pause(100-113)resume(114-127)
demo/src/stackflow/Stack.tsx (1)
integrations/react/src/future/lazy.tsx (1)
lazy(9-43)
extensions/plugin-history-sync/src/RouteLike.ts (2)
config/src/RegisteredActivityName.ts (1)
RegisteredActivityName(3-6)config/src/RegisteredActivityParamTypes.ts (1)
RegisteredActivityParamTypes(4-8)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (4)
core/src/interfaces/StackflowActions.ts (1)
StackflowActions(15-65)extensions/plugin-history-sync/src/makeHistoryTaskQueue.ts (1)
makeHistoryTaskQueue(7-30)extensions/plugin-history-sync/src/makeTemplate.ts (3)
makeTemplate(48-110)pathToUrl(5-7)urlSearchParamsToMap(9-17)core/src/event-utils/makeEvent.ts (1)
makeEvent(5-19)
🪛 LanguageTool
.changeset/new-pandas-return.md
[style] ~5-~5: Consider a different adjective to strengthen your wording.
Context: ... Default history setup option for rich deep link experiences
(DEEP_PROFOUND)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: stackflow-docs
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
extensions/plugin-history-sync/src/RouteLike.ts (1)
15-24: ConfirmadditionalStepsshape matches the core step model.Typing
additionalStepsto the same activityK’s params is great if steps are constrained to the same activity. If steps can reference different activities, this will be too narrow and may need a discriminated union includingactivityNameper step.If steps can vary by activity, a possible alternative would be:
- Change
additionalStepsto an array ofHistoryEntry-like step items.- Or define a dedicated
HistoryStepunion mirroring the runtime shape.demo/src/stackflow/Stack.tsx (2)
14-17: LGTM: Placeholder integration with the new lazy API is correctUsing buildPlaceholder with the Article placeholder component aligns with the new lazy loader contract and should provide a smooth deep-link loading experience.
36-39: No action needed — loader gating and Suspense coverage verifiedQuick summary: loaderPlugin pauses navigation when a loader or lazy component is pending unless the activity context requests immediate rendering; historySyncPlugin sets shouldRenderImmediately for history-driven initial entries; PluginRenderer forwards that context to lazy activities; demo also wraps in a Suspense boundary.
Key locations:
- integrations/react/src/future/loader/loaderPlugin.tsx — pauses (gates) when loader/lazy promises exist unless activity.context.lazyActivityComponentRenderContext?.shouldRenderImmediately === true.
- integrations/react/src/future/lazy.tsx — wraps the lazy component in Suspense only when a placeholder exists AND shouldRenderImmediately is true; otherwise it renders the lazy component (relies on gating or an outer Suspense).
- integrations/react/src/internal/PluginRenderer.tsx — forwards lazyActivityComponentRenderContext into the Activity props when a component is lazy.
- integrations/react/src/future/stackflow.tsx — loaderPlugin is appended after user plugins (commented/ensured ordering).
- extensions/plugin-history-sync/src/historySyncPlugin.tsx — sets lazyActivityComponentRenderContext.shouldRenderImmediately = true for initial/history-based pushes.
- demo/src/stackflow/Stack.tsx — uses historySyncPlugin(...)
- demo/src/App.tsx — wraps in a Suspense boundary.
Conclusion: the codebase already provides both gating (loaderPlugin) and Suspense coverage (historySyncPlugin + demo-level Suspense) to avoid "suspended without fallback".
demo/src/activities/Article.placeholder.tsx (1)
7-17: LGTM: Placeholder comp correctly uses params and composes the spinnerStructure and usage of useActivityParams and LoadingSpinner look good.
demo/src/stackflow/stackflow.config.ts (1)
20-25: defaultHistory wiring looks correct; verify interplay with initialActivity and typesThe defaultHistory seeding to Main aligns with the new history plugin behavior. Two quick checks:
- Ensure the route’s defaultHistory function signature matches the plugin type (params are strings per UrlPattern parsing). You’re not using params, which is fine.
- With history-sync providing initial events, initialActivity may be effectively bypassed for deep links. Confirm the intended precedence and that you won’t end up with duplicated initial pushes in non-deeplink cases.
Would you like me to scan the repo to ensure all defaultHistory route definitions match the expected signature and to check the plugin registration order relative to initialActivity handling?
integrations/react/src/future/lazy.tsx (1)
30-39: Render lazy component within a Suspense boundary even without a placeholderIf there’s no placeholder or shouldRenderImmediately is false, you render the lazy component without Suspense. That relies on an upstream Suspense boundary; without it, React will throw at runtime. If a global Suspense boundary is not guaranteed, wrap with Suspense and use a minimal fallback.
Do you guarantee a Suspense boundary upstream for all lazy activity render paths (e.g., PluginRenderer)? If not, consider this change:
- if (placeholder && shouldRenderImmediately) { - return ( - <Suspense fallback={placeholder}> - <LazyLoadedActivityComponent params={params} /> - </Suspense> - ); - } - - return <LazyLoadedActivityComponent params={params} />; + const shouldShowPlaceholder = placeholder && shouldRenderImmediately; + return ( + <Suspense fallback={shouldShowPlaceholder ? placeholder : null}> + <LazyLoadedActivityComponent params={params} /> + </Suspense> + );extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
725-727: onChanged hook is wired in core — no change neededConfirmed: the core exposes and invokes the onChanged post-effect hook and your plugin’s onChanged will be called after state changes.
- core/src/interfaces/StackflowPlugin.ts — onChanged is declared on the plugin interface.
- core/src/produceEffects.ts — emits a "%SOMETHING_CHANGED%" post-effect when state output changes.
- core/src/utils/triggerPostEffectHooks.ts — dispatches plugin.onChanged?.({ actions, effect }) for that effect.
- extensions/plugin-history-sync/src/historySyncPlugin.tsx — your onChanged handler (defaultHistorySetupCheckpoint(actions)) is in the right place and will be invoked.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
integrations/react/src/future/lazy.tsx (4)
32-45: Always use the placeholder as Suspense fallback when providedCurrently, the placeholder is used as a fallback only when
shouldRenderImmediatelyis true. If a placeholder is provided, it’s generally useful as a Suspense fallback in all cases. This also simplifies the branching.If this matches the intended UX, consider:
- if (placeholder && shouldRenderImmediately) { - if (transitionState === "enter-active") { - return placeholder; - } - - return ( - <Suspense fallback={placeholder}> - <LazyLoadedActivityComponent params={params} /> - </Suspense> - ); - } - - return <LazyLoadedActivityComponent params={params} />; + if (placeholder) { + if (shouldRenderImmediately && transitionState === "enter-active") { + return placeholder; + } + return ( + <Suspense fallback={placeholder}> + <LazyLoadedActivityComponent params={params} /> + </Suspense> + ); + } + return <LazyLoadedActivityComponent params={params} />;If you intentionally want to rely on an upstream Suspense boundary when
shouldRenderImmediatelyis false, feel free to ignore this.
46-48: Set a displayName for better DevTools tracesSmall DX improvement: set a displayName so the wrapper shows up clearly in React DevTools and error stacks.
LazyActivityComponent._load = cachedLoad; + LazyActivityComponent.displayName = "LazyActivityComponent";
10-13: Tighten the generic constraint to avoidanyPrefer
unknownoveranyfor safer typing. If your internal types expect indexable params, this will still work while preventing accidental unsafe access.-export function lazy<T extends { [K in keyof T]: any } = {}>( +export function lazy<T extends Record<string, unknown> = Record<string, never>>(Note: If
StaticActivityComponentType<T>depends on{ [K in keyof T]: any }specifically, you may need to adjust the companion type to acceptunknown.
25-31: Consider passing params to the placeholder builder (future enhancement)Placeholders often benefit from route params (e.g., sizing, skeleton variants). Exposing them is a small ergonomic win and keeps placeholders in sync with the activity’s params.
Proposed API:
- Change config type:
- buildPlaceholder?: (params: T) => ReactNode
- And call as:
- const placeholder = config?.buildPlaceholder?.(params);
If you’d like, I can open a follow-up PR with the type and call-site updates across the demo and internals.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
integrations/react/src/future/lazy.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build all packages
- GitHub Check: Create PR or release packages
- GitHub Check: Check whether the written test passes normally
- GitHub Check: Check the TypeScript typings
- GitHub Check: Workers Builds: stackflow-docs
🔇 Additional comments (2)
integrations/react/src/future/lazy.tsx (2)
46-46: Confirm _load typing matches LazyActivityComponentTypeEnsure
LazyActivityComponentType<T>declares_load: () => Promise<{ default: StaticActivityComponentType<T> }>(or theLoadedModulealias). This prevents property-assignment errors and improves API discoverability for callers who want to preload.
10-10: API shape and overall direction look solidGenerics, cached dynamic import, and the wrapper with
shouldRenderImmediatelyare coherent and align with the PR’s goals.
|
@coderabbitai perform full review |
|
@ENvironmentSet I'll perform a comprehensive review of this PR which introduces default history setup options and structured activity components for enhanced deep link experiences. This is a substantial feature addition that touches multiple core areas of the Stackflow framework. The changes include:
Key areas I'll focus on during the review:
The feature appears well-architected with clear separation of concerns and should significantly improve the deep linking experience in Stackflow applications. ✅ Actions performedFull review triggered. |
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)
integrations/react/src/stable/stackflow.tsx (1)
311-319: Bug:replace()generates two different activityIds when none is provided.
makeActivityId()is called twice; the ID used for the action may differ from the one you return. Use the computedactivityIdfor both.Apply this diff:
replace(activityName, activityParams, options) { - const activityId = options?.activityId ?? makeActivityId(); + const activityId = options?.activityId ?? makeActivityId(); getCoreStore()?.actions.replace({ - activityId: options?.activityId ?? makeActivityId(), + activityId, activityName, activityParams, skipEnterActiveState: parseActionOptions(options).skipActiveState, }); return { activityId, }; },
♻️ Duplicate comments (7)
.changeset/chubby-pianos-heal.md (1)
7-8: Fix grammar and capitalization in the public changeset entryTighten the phrasing and use the correct brand capitalization.
-Structured activity components are components modeling activity view while exposing major features of an activity. -Allowing developers easily customize user experience of an activity view, it even allow stackflow to perform various kinds of optimizations. +Structured activity components model an activity view while exposing its major features. +They allow developers to easily customize the user experience of an activity view and enable Stackflow to perform various optimizations.extensions/plugin-history-sync/src/RouteLike.ts (2)
26-30: Follow through the generic rename inRouteLike.Keep the naming consistent to avoid confusion with React’s
ComponentType.-export type RouteLike<ComponentType> = +export type RouteLike<TComponent> = | string | string[] - | Route<ComponentType> - | Route<ComponentType>[]; + | Route<TComponent> + | Route<TComponent>[];
7-13: MakedefaultHistoryreceive decoded params; also rename the generic for clarity.This keeps
defaultHistoryaligned withdecode()and avoids forcing consumers to handle raw strings when typed params are available.Apply this diff:
-export type Route<ComponentType> = { +export type Route<TComponent> = { path: string; decode?: ( params: Record<string, string>, - ) => ComponentType extends ActivityComponentType<infer U> ? U : {}; - defaultHistory?: (params: Record<string, string>) => HistoryEntry[]; + ) => TComponent extends ActivityComponentType<infer U> ? U : {}; + defaultHistory?: ( + params: TComponent extends ActivityComponentType<infer U> ? U : {}, + ) => HistoryEntry[]; };And update the union below accordingly (see next comment).
After updating the types, ensure the plugin calls
defaultHistorywith decoded params (or parsed fallback). If needed, I can propose the corresponding changes inhistorySyncPlugin.tsx.demo/src/stackflow/Stack.tsx (1)
5-15: Confirm Article component shape (structured vs monolithic) and prop contractsYou’re now passing Article directly in the components map. If Article is a structured activity object, ensure its loading/layout components accept the required params shape; if it’s monolithic, previous concerns are resolved.
If Article is structured, and ArticleLoading/ArticleLayout don’t accept params, wrap them or add a params prop. Example adapter:
- Article, + Article: { + content: Article.content, + loading: ({ params }) => <ArticleLoading />, + layout: ({ params, children }) => <ArticleLayout>{children}</ArticleLayout>, + // error: ... + },Also note: removing lazy-loading for Article trades off code-splitting for simplicity in the demo; that’s fine for the demo, just flagging the change in loading behavior.
integrations/react/src/future/loader/loaderPlugin.tsx (1)
121-129: Fix wait gating and always attach handlers to avoid unhandled rejectionsTwo issues:
- Pausing logic: Using OR with the loading check forces pause even when a Loading UI exists. Use AND so we only block when we must.
- Handlers: When shouldRenderImmediately === true and a Loading UI exists, you skip attaching handlers entirely, risking unhandled rejections and losing error logs.
Apply:
- const lazyComponentPromise = - "_load" in contentComponent ? contentComponent._load?.() : undefined; - const shouldRenderImmediately = (activityContext as any) - ?.lazyActivityComponentRenderContext?.shouldRenderImmediately; - - if ( - (loaderDataPromise || lazyComponentPromise) && - (shouldRenderImmediately !== true || - "loading" in matchActivityComponent === false) - ) { - pause(); - - Promise.allSettled([loaderDataPromise, lazyComponentPromise]) - .then(([loaderDataPromiseResult, lazyComponentPromiseResult]) => { - printLoaderDataPromiseError({ - promiseResult: loaderDataPromiseResult, - activityName: matchActivity.name, - }); - printLazyComponentPromiseError({ - promiseResult: lazyComponentPromiseResult, - activityName: matchActivity.name, - }); - }) - .finally(() => { - resume(); - }); - } + const lazyComponentPromise = + "_load" in contentComponent ? contentComponent._load?.() : undefined; + const shouldRenderImmediately = + (activityContext as any)?.lazyActivityComponentRenderContext + ?.shouldRenderImmediately; + + // Always attach handlers for error visibility, irrespective of waiting. + const settled = Promise.allSettled([ + loaderDataPromise, + lazyComponentPromise, + ]).then(([loaderDataPromiseResult, lazyComponentPromiseResult]) => { + printLoaderDataPromiseError({ + promiseResult: loaderDataPromiseResult, + activityName: matchActivity.name, + }); + printLazyComponentPromiseError({ + promiseResult: lazyComponentPromiseResult, + activityName: matchActivity.name, + }); + }); + + // Only pause when we should wait AND there is no component-level Loading UI. + const shouldWait = + (loaderDataPromise || lazyComponentPromise) && + shouldRenderImmediately !== true && + ("loading" in matchActivityComponent === false); + + if (shouldWait) { + pause(); + settled.finally(() => { + resume(); + }); + }Also applies to: 132-146
integrations/react/src/__internal__/PluginRenderer.tsx (1)
100-106: Remount ErrorBoundary on param changes to clear stale error stateWithout a key tied to params, an error state can persist across param-only updates.
- ErrorComponent ? ( - <StructuredActivityComponentErrorBoundary + ErrorComponent ? ( + <StructuredActivityComponentErrorBoundary + key={JSON.stringify(params)} renderFallback={(err, reset) => ( <ErrorComponent params={params} error={err} reset={reset} /> )} > {node} </StructuredActivityComponentErrorBoundary> ) : ( node ),extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
186-196: Fix brittle pattern and null-spread crash when building defaultHistory params
- Appending a trailing slash to the UrlPattern is brittle; it can cause mismatches for paths that do not end with “/”.
UrlPattern.match()can returnnull. Spreading...pathParamswill throw in native object spread semantics.Use the route path as-is and guard the match with a fallback to
{}.Apply this diff:
- const pattern = new UrlPattern( - `${targetActivityRoute.path}/`, - options.urlPatternOptions, - ); + const pattern = new UrlPattern( + targetActivityRoute.path, + options.urlPatternOptions, + ); const url = pathToUrl(currentPath); - const pathParams = pattern.match(url.pathname); + const pathParams = pattern.match(url.pathname) ?? {}; const searchParams = urlSearchParamsToMap(url.searchParams); const params = { ...searchParams, ...pathParams, };Optional: You can also reuse the earlier
makeTemplate(...).parse(currentPath)result to avoid a second matcher.
🧹 Nitpick comments (23)
.changeset/gold-groups-own.md (1)
5-5: Polish the description for consistency and clarityConsider tightening the language and using backticks for the package name.
-Sync with type constraint changes in stackflow/react +Sync with type-constraint changes in `@stackflow/react`.demo/src/activities/Article/Article.loading.css.ts (1)
3-9: Prefer viewport-safe vertical centering (mobile-safe)
height: "100%"centers only if a parent provides explicit height. To ensure vertical centering across pages and mobile browsers (iOS dynamic viewport), add a viewport-constrained minHeight.export const container = style({ display: "flex", alignItems: "center", justifyContent: "center", - height: "100%", + height: "100%", + // Ensure full-viewport centering even when no parent height is set + minHeight: "100dvh", width: "100%", });demo/src/components/LoadingSpinner.css.ts (1)
15-15: Respect prefers-reduced-motion for accessibilityDisable or reduce animation for users who prefer reduced motion.
export const spinner = style({ width: 48, height: 48, border: `4px solid ${vars.$scale.color.gray600}`, borderTop: `4px solid ${vars.$scale.color.gray900}`, borderRadius: "50%", - animation: `${spin} 1s linear infinite`, + animation: `${spin} 1s linear infinite`, + "@media": { + "(prefers-reduced-motion: reduce)": { + animation: "none", + }, + }, });demo/src/components/LoadingSpinner.tsx (1)
5-7: Add basic ARIA for screen readers and allow className passthroughExpose a label and className prop, and add role/aria-live for assistive tech. Keeps defaults simple while enabling composition.
-const LoadingSpinner: FC = () => { - return <div className={css.spinner} />; -}; +const LoadingSpinner: FC<{ label?: string; className?: string }> = ({ + label = "Loading…", + className, +}) => { + return ( + <div + role="status" + aria-live="polite" + aria-label={label} + className={[css.spinner, className].filter(Boolean).join(" ")} + /> + ); +};integrations/react/src/stable/stackflow.tsx (1)
262-262: Mutation ofactivityComponentMapwon’t trigger context updates post-mount (by design).This is fine since
addActivity()is disallowed after<Stack />is rendered. If this policy ever changes, remember that mutating the same object reference won’t propagate via context and you’d need to replace the map object to trigger updates.integrations/react/src/__internal__/StaticActivityComponentType.ts (1)
3-4: Prefer a clearer object constraint for params.
T extends {}is permissive but a bit opaque. Consider a more explicit constraint for readability and intent.-export type StaticActivityComponentType<T extends {} = {}> = - React.ComponentType<{ params: T }>; +export type StaticActivityComponentType<T extends Record<string, unknown> = {}> = + React.ComponentType<{ params: T }>;extensions/plugin-preload/src/pluginPreload.tsx (1)
14-15: Avoid self-referentialinfer U extends { [key in keyof U]: ... }constraint; use a safer two-step check.Self-referential constraints can be brittle and sometimes lead to hard-to-debug inference issues. A safer pattern is to infer
Ufirst, then narrow with a conditional.Apply this diff:
- [key in Extract<keyof T, string>]?: T[key] extends ActivityComponentType< - infer U extends { [key in keyof U]: string | undefined } - > - ? Loader<U> - : Loader<{}>; + [key in Extract<keyof T, string>]?: T[key] extends ActivityComponentType<infer U> + ? U extends Record<string, string | undefined> + ? Loader<U> + : Loader<{}> + : Loader<{}>;Please run your local typecheck to ensure this narrows as intended across monolithic and structured activity types.
.changeset/new-pandas-return.md (1)
5-5: Tighten changeset copy (grammar and clarity)Use a definite article and hyphenate “deep‑link” for clarity.
-Add `defaultHistory` route option to pre-seed stack for better deep link experiences +Add a `defaultHistory` route option to pre-seed the navigation stack for richer deep‑link experiencesdemo/src/activities/Article/Article.content.css.ts (1)
4-4: Consider path aliases to reduce brittle relative importsAs these files move deeper,
../../chains can get noisy and fragile. Consider setting up a TS path alias (e.g.,@demo/styles) and updating the bundler to match.If you want, I can propose a tsconfig and Vite/Next config patch.
demo/src/activities/Article/Article.loading.tsx (1)
5-11: Minor a11y check for loading UI.If LoadingSpinner doesn’t already expose appropriate ARIA (e.g., role="status" and an accessible label), consider adding it at the container or spinner to ensure screen readers announce progress.
integrations/react/src/future/usePrepare.ts (3)
37-41: Guard against undefinedcontentin misconfigured maps.If a structured entry has
contentdefined but set toundefinedat runtime (misconfiguration), the current code will blow up when checking_load. A small nullish fallback makes this more robust without changing semantics.Apply:
- const contentComponent = - "content" in activityComponentMap[activityName] - ? activityComponentMap[activityName].content - : activityComponentMap[activityName]; + const contentComponent = + "content" in activityComponentMap[activityName] && + activityComponentMap[activityName].content + ? activityComponentMap[activityName].content + : activityComponentMap[activityName];
42-46: Tighten lazy prefetch guard; avoid queuing a resolvedundefined.Using
"in"checks truthiness of the property, but you still call._load?.()which can enqueue a resolvedundefined. Prefer verifying it’s a function before invoking.Apply:
- if ("_load" in contentComponent) { - const lazyComponent = contentComponent; - - prefetchTasks.push(Promise.resolve(lazyComponent._load?.())); - } + if ( + contentComponent && + typeof (contentComponent as any)._load === "function" + ) { + prefetchTasks.push((contentComponent as any)._load()); + }
48-49: Consider swallowing prefetch errors to not block navigation.If a loader or lazy prefetch fails,
preparewill reject. Depending on UX, you may want to log and continue, letting runtime rendering handle errors.Option:
- await Promise.all(prefetchTasks); + // Prefer not to block navigation on prefetch failures + await Promise.allSettled(prefetchTasks);If the intended contract is to fail fast, ignore this.
integrations/react/src/__internal__/LazyActivityComponentType.ts (1)
3-6: Generic boundT extends {}admits primitives; considerobject.If
Trepresents props, allowing primitives (e.g.,string) viaT extends {}is likely unintended. Usingobject(or a record) tightens the constraint while keeping inference friendly.Minimal change:
-export type LazyActivityComponentType<T extends {} = {}> = +export type LazyActivityComponentType<T extends object = {}> =If other types in this PR have standardised on
{}, keeping it for consistency is fine; just ensure no call sites rely on primitives forT.demo/src/activities/Article/Article.tsx (1)
14-18: Consider adding an error component for resiliency.Since the structured type supports it, wiring an error boundary component (e.g., ArticleError) will improve UX for loader/content failures.
If you have (or want) an error component:
export const Article: ActivityComponentType<"Article"> = { content: lazy(() => import("./Article.content")), layout: ArticleLayout, loading: ArticleLoading, + // Optional but recommended + // error: ArticleError, };integrations/react/src/future/ActivityLoadingComponentType.ts (1)
1-9: Align public “future” types on React type exposure policyThis type directly imports React’s ComponentType, while ActivityComponentType was refactored to avoid exposing React types at the public surface via an internal alias. Consider aligning on a single approach (either consistently expose React types or consistently hide them behind internal aliases) to avoid API inconsistency and reduce future churn.
Would you like me to draft a small internal alias (e.g., an internal LoadingComponentType) and update the public type to reference it?
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (1)
4-9: Tighten generic constraint and verify content type choice
- Prefer P extends Record<string, unknown> over P extends {} for clearer intent and better soundness.
- Double-check that content: MonolithicActivityComponentType
is intentional. If content is meant to be a “content-only” component (as opposed to a full monolithic activity), consider introducing/using a dedicated ActivityContentComponentType to avoid over-permissive props at the type level.
Apply this small constraint improvement:
-export interface StructuredActivityComponentType<P extends {}> { +export interface StructuredActivityComponentType<P extends Record<string, unknown>> {integrations/react/src/future/stackflow.tsx (1)
36-45: Make StackflowInput components generic match the stricter function genericStackflowInput currently uses ActivityComponentType, while the stackflow() function’s R generic is already tighter with ActivityComponentType<InferActivityParams>. Aligning StackflowInput improves type safety when the type is used standalone.
Apply:
export type StackflowInput< T extends ActivityDefinition<RegisteredActivityName>, R extends { - [activityName in RegisteredActivityName]: ActivityComponentType<any>; + [activityName in RegisteredActivityName]: ActivityComponentType< + InferActivityParams<activityName> + >; }, > = { config: Config<T>; components: R; plugins?: Array<StackflowPluginsEntry>; };demo/src/activities/Article/Article.content.tsx (2)
16-21: Add alt attribute to image for accessibilityThe hero image lacks an alt attribute. Use the article title or an empty alt if decorative.
<LazyLoadImage src={imageUrl} effect="opacity" width="100%" height="100%" + alt={title} />
36-36: Remove stray trailing space in section titleMinor polish.
- <div className={css.sectionTitle}>Other Items by Emila </div> + <div className={css.sectionTitle}>Other Items by Emila</div>integrations/react/src/__internal__/PluginRenderer.tsx (1)
38-43: Type consistency nit: prefer ReactNode (already imported) over React.ReactNodeKeeps style consistent within the file.
- let output: React.ReactNode = + let output: ReactNode = "content" in Activity ? ( renderStructuredActivityComponent(Activity, activity.params) ) : ( <Activity params={activity.params} /> );extensions/plugin-history-sync/src/historySyncPlugin.tsx (2)
98-114: Avoid lingering entity set by clearing it when the queue drainsWhen the last pending task is shifted and the queue becomes null, also clear
defaultHistoryEntryEntities. This prevents unnecessary memory retention and avoids any future false positives if more default-history work is ever scheduled.Apply this diff:
const nextTask = pendingDefaultHistoryEntryInsertionTasks.shift(); if (pendingDefaultHistoryEntryInsertionTasks.length === 0) { pendingDefaultHistoryEntryInsertionTasks = null; + defaultHistoryEntryEntities.clear(); } if (nextTask) { nextTask(actions); }
616-635: Simplify guard for path injection into activityContextThe current check uses the
inoperator; prefer a straightforward nullish check for readability and to avoid prototype-chain quirks.Apply these diffs:
- if ( - !actionParams.activityContext || - "path" in actionParams.activityContext === false - ) { + if (!actionParams.activityContext?.path) { const match = activityRoutes.find( (r) => r.activityName === actionParams.activityName, )!; const template = makeTemplate(match, options.urlPatternOptions); const path = template.fill(actionParams.activityParams); overrideActionParams({ ...actionParams, activityContext: { ...actionParams.activityContext, path, }, }); }- if ( - !actionParams.activityContext || - "path" in actionParams.activityContext === false - ) { + if (!actionParams.activityContext?.path) { const match = activityRoutes.find( (r) => r.activityName === actionParams.activityName, )!; const template = makeTemplate(match, options.urlPatternOptions); const path = template.fill(actionParams.activityParams); overrideActionParams({ ...actionParams, activityContext: { ...actionParams.activityContext, path, }, }); }Also applies to: 640-657
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- 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 (38)
.changeset/chubby-pianos-heal.md(1 hunks).changeset/gold-groups-own.md(1 hunks).changeset/new-pandas-return.md(1 hunks)demo/src/activities/Article.tsx(0 hunks)demo/src/activities/Article/Article.content.css.ts(1 hunks)demo/src/activities/Article/Article.content.tsx(1 hunks)demo/src/activities/Article/Article.layout.tsx(1 hunks)demo/src/activities/Article/Article.loading.css.ts(1 hunks)demo/src/activities/Article/Article.loading.tsx(1 hunks)demo/src/activities/Article/Article.tsx(1 hunks)demo/src/activities/Main/Main.css.ts(1 hunks)demo/src/activities/Main/Main.tsx(1 hunks)demo/src/components/LoadingSpinner.css.ts(1 hunks)demo/src/components/LoadingSpinner.tsx(1 hunks)demo/src/stackflow/Stack.tsx(1 hunks)demo/src/stackflow/stackflow.config.ts(2 hunks)demo/src/stackflow/stackflow.docs.ts(1 hunks)extensions/plugin-history-sync/src/RouteLike.ts(1 hunks)extensions/plugin-history-sync/src/historySyncPlugin.tsx(13 hunks)extensions/plugin-preload/src/pluginPreload.tsx(1 hunks)integrations/react/src/__internal__/ActivityComponentType.ts(1 hunks)integrations/react/src/__internal__/LazyActivityComponentType.ts(1 hunks)integrations/react/src/__internal__/MainRenderer.tsx(0 hunks)integrations/react/src/__internal__/MonolithicActivityComponentType.ts(1 hunks)integrations/react/src/__internal__/PluginRenderer.tsx(3 hunks)integrations/react/src/__internal__/StaticActivityComponentType.ts(1 hunks)integrations/react/src/__internal__/StructuredActivityComponentType.tsx(1 hunks)integrations/react/src/future/ActivityComponentType.tsx(1 hunks)integrations/react/src/future/ActivityContentComponentType.ts(1 hunks)integrations/react/src/future/ActivityErrorComponentType.ts(1 hunks)integrations/react/src/future/ActivityLayoutComponentType.ts(1 hunks)integrations/react/src/future/ActivityLoadingComponentType.ts(1 hunks)integrations/react/src/future/index.ts(1 hunks)integrations/react/src/future/lazy.tsx(1 hunks)integrations/react/src/future/loader/loaderPlugin.tsx(2 hunks)integrations/react/src/future/stackflow.tsx(2 hunks)integrations/react/src/future/usePrepare.ts(1 hunks)integrations/react/src/stable/stackflow.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- integrations/react/src/internal/MainRenderer.tsx
- demo/src/activities/Article.tsx
🧰 Additional context used
🪛 LanguageTool
.changeset/chubby-pianos-heal.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...an activity. Allowing developers easily customize user experience of an activity view, it...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ... perform various kinds of optimizations.
(QB_NEW_EN)
.changeset/new-pandas-return.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...dd defaultHistory route option to pre-seed stack for better deep link experiences ...
(QB_NEW_EN)
[style] ~5-~5: Consider a different adjective to strengthen your wording.
Context: ...ute option to pre-seed stack for better deep link experiences
(DEEP_PROFOUND)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: stackflow-docs
🔇 Additional comments (30)
demo/src/components/LoadingSpinner.css.ts (1)
9-16: LGTM on size typing; numeric lengths are valid in vanilla-extractUsing numeric values for width/height is correct here (vanilla-extract appends px for length properties). The animation and color tokens are also appropriate.
demo/src/components/LoadingSpinner.tsx (1)
1-9: Component structure is clean and minimalThe zero-dependency implementation and default export are straightforward and easy to consume.
integrations/react/src/stable/stackflow.tsx (1)
138-141: Dropping memo() around activity components: verify render/perf implications and structured-type compatibility.Storing raw components (or structured entries) directly in
activityComponentMapaligns with the new flexible component shapes. However, removingmemo()may increase re-renders for monolithic components when used by consumers that rely on referential stability.
- Please confirm that consumers don’t rely on memoized activity components for perf.
- Ensure MainRenderer (and any consumers) treat these as relatively stable and don’t recreate elements in render loops unnecessarily.
If helpful, I can draft a lightweight micro-benchmark or profiling recipe to compare before/after perf characteristics in the demo app.
extensions/plugin-history-sync/src/RouteLike.ts (1)
15-24: Confirm step params shape foradditionalSteps.
additionalSteps.stepParamsis typed asRegisteredActivityParamTypes[K]. If step params have a distinct registered type (often they do), consider introducing a dedicatedRegisteredStepParamTypesmapping instead.If steps are guaranteed to mirror their activity’s params, this is fine; otherwise, please factor a separate step params registry.
demo/src/activities/Main/Main.css.ts (1)
3-3: Unable to locate exportfin the styles moduleI searched
demo/src/styles/**for anyexport const forexport { f }declarations and found none. Please verify wherefis defined and update this import (and any others) accordingly.• File to update:
demo/src/activities/Main/Main.css.ts(line 3)
• Ensure the target module exports a namedf(or adjust the path to the file that does).changeset/new-pandas-return.md (1)
1-3: Changeset metadata looks correctPackage and bump type are valid for introducing a new route option.
demo/src/activities/Article/Article.content.css.ts (1)
4-4: Import path update LGTMPath change to the centralized styles barrel is consistent with the repo’s restructuring. All usages of
f.*in this file remain valid.demo/src/activities/Main/Main.tsx (1)
4-9: Import path realignment is consistent and safeOnly path relocations; no behavioral changes. This should tree-shake identically and keeps the component wiring intact.
demo/src/stackflow/stackflow.docs.ts (1)
5-6: Export shapes confirmed
Article is correctly exported as a named export and Main remains a default export—no further changes needed.integrations/react/src/future/lazy.tsx (1)
23-28: Rename to LazyActivityComponent is clear and consistentThe naming aligns with the surrounding types and keeps
_loadattached for prefetch. No behavior change introduced.demo/src/activities/Article/Article.loading.tsx (1)
3-3: Import alias validated – no changes neededI’ve confirmed that
demo/src/activities/Article/Article.loading.css.tsexists and that importing it via./Article.loading.cssis correct under the project’s vanilla-extract setup.integrations/react/src/future/ActivityErrorComponentType.ts (1)
7-13: Type looks solid and aligned with error boundary patterns.Good use of
unknownforerrorand explicitresetcallback. No issues.integrations/react/src/__internal__/MonolithicActivityComponentType.ts (1)
4-6: Clear consolidation of static and lazy component types.This alias simplifies downstream typing and pairs well with the structured component shape introduced elsewhere. Looks good.
integrations/react/src/future/index.ts (1)
6-9: Re-exports look good and clarify the public future API surface.Adding these typed component re-exports improves DX and keeps the surface cohesive.
demo/src/activities/Article/Article.tsx (2)
5-12: Module augmentation matches route decode shape.Param types for Article are well-defined (articleId: number; title?: string) and align with the stack config’s decode.
15-15: Default export shape verified for ArticleContentI’ve confirmed that:
demo/src/activities/Article/Article.content.tsxexportsArticleContentas its default.ArticleContentis a React functional component and thus satisfies theComponentType<{ params: … }>requirement ofActivityContentComponentType<"Article">.No further changes are needed here.
demo/src/stackflow/stackflow.config.ts (2)
2-3: Loaders correctly exported – no changes needed.Verified that
demo/src/activities/Article/Article.loader.tsexportsasync function articleLoaderdemo/src/activities/Main/Main.loader.tsexportsfunction mainLoaderNo further action required.
20-25: defaultHistory only applies to the Article route—no duplicate Main
I’ve checked extensions/plugin-history-sync/src/historySyncPlugin.tsx and confirmed that it callstargetActivityRoute.defaultHistory(which is only defined on the Article route in demo/src/stackflow/stackflow.config.ts). Since the Main route has nodefaultHistory, starting atinitialActivity: "Main"won’t reinsert another Main entry. No changes needed here.integrations/react/src/future/ActivityContentComponentType.ts (1)
1-11: Type alias is correct and consistent with the structured activity pattern.Generic constraints and params inference look good.
integrations/react/src/future/ActivityComponentType.tsx (1)
5-8: LGTM: Public type now correctly delegates to internal aliasGood move to decouple the public API from React types by routing through the internal alias while preserving per-activity param inference.
integrations/react/src/future/stackflow.tsx (1)
58-61: LGTM: Per-activity param inference on components mapUsing ActivityComponentType<InferActivityParams> tightens the public typing and prevents accidental param mismatches per activity.
demo/src/activities/Article/Article.layout.tsx (1)
5-12: LGTM: clean layout wrapper with correct typingReads the title via useActivityParams and passes it to Layout; ReactNode typing is correct. DisplayName set appropriately.
integrations/react/src/__internal__/ActivityComponentType.ts (1)
4-6: LGTM: unified activity component alias covers monolithic and structured variantsThe union alias is clearer and keeps call sites decoupled from implementation specifics.
integrations/react/src/future/loader/loaderPlugin.tsx (1)
105-108: Content-component resolution is correctDeferring to structuredActivityComponent.content when available aligns the loader with the structured model.
integrations/react/src/__internal__/PluginRenderer.tsx (1)
91-111: Wrapper order is correct (Layout(ErrorBoundary(Suspense(Content))))This ordering properly catches lazy-load errors and shows loading fallbacks. Nice.
extensions/plugin-history-sync/src/historySyncPlugin.tsx (5)
286-305: Good fix: pre-register initial activity id for default history sequencingRegistering
initialActivityIdindefaultHistoryEntryEntitiesand reusing it for the initial Pushed event resolves the sequencing bug where the initial push cleared pending default-history tasks.
312-315: LGTM: query params fallback for routes without defaultHistoryUsing
template.parse(currentPath) ?? urlSearchParamsToMap(...)avoids null-spread and preserves query params when path variables don’t match. Consistent with the intended deep-link behavior.
488-495: LGTM: cancel default-history queue on non-default pushClearing pending tasks when a pushed activity id isn’t part of the default-history set prevents stale preseed tasks from polluting user-driven navigation.
520-525: LGTM: cancel default-history queue on non-default step pushSame reasoning as activities; protects the stack from unintended step insertions once the user interacts.
606-615: LGTM: cleanup on pop eventsClearing pending default-history tasks on
onPopped/onStepPoppedaligns with user-driven back navigation and avoids inconsistent stack state.
|
@CodeRabbit write me a detailed PR summary |
|
Tip For best results, initiate chat on the files or code changes. @ENvironmentSet Here's a detailed summary of PR #610: 🎯 OverviewThis PR introduces default history setup options and structured activity components to enable richer deep link experiences in Stackflow applications. It's a substantial architectural enhancement that modernizes the component system while maintaining backward compatibility. 🏗️ Major Architectural Changes1. Structured Activity Components
2. Default History Feature
🔧 Type System ImprovementsEnhanced Type Constraints
Improved Type Safety
📦 Package-Specific Changes@stackflow/react (Minor)
@stackflow/plugin-history-sync (Minor)
@stackflow/plugin-preload (Patch)
🎨 Demo Application TransformationRestructured Architecture
Enhanced User Experience
Path Updates
🔄 Migration & CompatibilityNon-Breaking Changes
Opt-in Enhancements
🎯 Key Benefits
📊 Impact Assessment
This PR significantly enhances Stackflow's capabilities for building rich, navigable applications while maintaining the framework's ease of use and type safety. |
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)
integrations/react/src/future/loader/loaderPlugin.tsx (1)
105-112: Guard the "in" operator: potential runtime TypeError when component is missingYou’re using "content" in matchActivityComponent before verifying that matchActivityComponent exists. If the component isn’t registered, this throws at runtime. Move the null-check before the "in" check.
Apply this diff to reorder the guard:
- const matchActivityComponent = input.components[activityName as T["name"]]; - const contentComponent = - "content" in matchActivityComponent - ? matchActivityComponent.content - : matchActivityComponent; - - if (!matchActivity || !matchActivityComponent) { - return; - } + const matchActivityComponent = input.components[activityName as T["name"]]; + + if (!matchActivity || !matchActivityComponent) { + return; + } + + const contentComponent = + "content" in matchActivityComponent + ? matchActivityComponent.content + : matchActivityComponent;
♻️ Duplicate comments (2)
integrations/react/src/future/loader/loaderPlugin.tsx (1)
125-145: Unhandled rejections when shouldRenderImmediately === true; attach handlers regardless of waitingWhen shouldRenderImmediately is true, no handlers are attached to loaderDataPromise/lazyComponentPromise. Rejections will surface as unhandled and you lose error logging. Always attach allSettled handlers; only pause/resume when you actually need to wait.
Apply this diff to always attach handlers while preserving non-blocking behavior:
- if ( - (loaderDataPromise || lazyComponentPromise) && - shouldRenderImmediately !== true - ) { - pause(); - - Promise.allSettled([loaderDataPromise, lazyComponentPromise]) - .then(([loaderDataPromiseResult, lazyComponentPromiseResult]) => { - printLoaderDataPromiseError({ - promiseResult: loaderDataPromiseResult, - activityName: matchActivity.name, - }); - printLazyComponentPromiseError({ - promiseResult: lazyComponentPromiseResult, - activityName: matchActivity.name, - }); - }) - .finally(() => { - resume(); - }); - } + if (loaderDataPromise || lazyComponentPromise) { + const shouldWait = shouldRenderImmediately !== true; + const settled = Promise.allSettled([ + loaderDataPromise, + lazyComponentPromise, + ]).then(([loaderDataPromiseResult, lazyComponentPromiseResult]) => { + printLoaderDataPromiseError({ + promiseResult: loaderDataPromiseResult, + activityName: matchActivity.name, + }); + printLazyComponentPromiseError({ + promiseResult: lazyComponentPromiseResult, + activityName: matchActivity.name, + }); + }); + + if (shouldWait) { + pause(); + settled.finally(() => { + resume(); + }); + } + }Nit (optional): filter undefined inputs to Promise.allSettled to avoid creating extraneous fulfilled entries:
const promises = [loaderDataPromise, lazyComponentPromise].filter( Boolean, ) as Promise<unknown>[]; const settled = Promise.allSettled(promises)...extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
186-196: Remove brittle trailing slash and guard against null pattern match.Appending a trailing slash to the UrlPattern can cause mismatches for paths without trailing slashes. Also, UrlPattern.match() can return null; spreading null into params will throw. Fix both.
Apply this diff:
- const pattern = new UrlPattern( - `${targetActivityRoute.path}/`, - options.urlPatternOptions, - ); + const pattern = new UrlPattern( + targetActivityRoute.path, + options.urlPatternOptions, + ); const url = pathToUrl(currentPath); - const pathParams = pattern.match(url.pathname); + const pathParams = pattern.match(url.pathname) ?? {}; const searchParams = urlSearchParamsToMap(url.searchParams); const params = { ...searchParams, ...pathParams, };Optional: to avoid dual parsing, reuse makeTemplate(targetActivityRoute, ...).parse(currentPath) and merge with search params, as you already do later at Lines 260-266.
🧹 Nitpick comments (6)
integrations/react/src/future/loader/loaderPlugin.tsx (1)
121-124: Avoid any-cast for shouldRenderImmediately; add a minimal typed guardDirect any-casts reduce type safety. Introduce a tiny local type or a type guard for lazyActivityComponentRenderContext to keep inference intact without widening.
Example:
type LazyActivityComponentRenderContext = { shouldRenderImmediately?: boolean; }; const shouldRenderImmediately = (activityContext as { lazyActivityComponentRenderContext?: LazyActivityComponentRenderContext }) ?.lazyActivityComponentRenderContext?.shouldRenderImmediately;extensions/plugin-history-sync/src/historySyncPlugin.tsx (5)
98-115: Drain the default-history queue while idle and schedule retries to avoid stalls.As written, only one task executes per checkpoint and progress relies on onChanged frequency. If onChanged isn’t dispatched for some steps (or transitions stay mostly idle), tasks can stall.
Apply this diff to drain multiple tasks while idle and schedule a retry when still pending:
function defaultHistorySetupCheckpoint(actions: StackflowActions) { - if (!pendingDefaultHistoryEntryInsertionTasks) return; - - const stack = actions.getStack(); - - if (stack.globalTransitionState !== "idle") return; - - const nextTask = - pendingDefaultHistoryEntryInsertionTasks.shift(); - - if (pendingDefaultHistoryEntryInsertionTasks.length === 0) { - pendingDefaultHistoryEntryInsertionTasks = null; - } - - if (nextTask) { - nextTask(actions); - } + if (!pendingDefaultHistoryEntryInsertionTasks) return; + // Drain as many tasks as we can while the stack is idle. + while ( + pendingDefaultHistoryEntryInsertionTasks && + pendingDefaultHistoryEntryInsertionTasks.length > 0 && + actions.getStack().globalTransitionState === "idle" + ) { + const nextTask = + pendingDefaultHistoryEntryInsertionTasks.shift(); + if (nextTask) nextTask(actions); + } + if ( + pendingDefaultHistoryEntryInsertionTasks && + pendingDefaultHistoryEntryInsertionTasks.length === 0 + ) { + pendingDefaultHistoryEntryInsertionTasks = null; + } else if (pendingDefaultHistoryEntryInsertionTasks) { + // Ensure forward progress even if onChanged is sparse. + requestHistoryTick(() => defaultHistorySetupCheckpoint(actions)); + } }
490-496: Abort policy on unexpected pushes looks correct; consider resilience.Clearing pending tasks when an unrecognized push occurs is a sensible abort. If you encounter environments where onChanged isn’t reliably dispatched after these pushes, couple this with a scheduled checkpoint (see earlier suggestion) to ensure the queue progresses.
521-527: Mirrored abort policy for step pushes is consistent.Same observation as onPushed; optional scheduled checkpoint can help in sparse onChanged scenarios.
618-635: Nit: simplify the path-presence check for readability.Current condition relies on operator precedence. Make it explicit for maintainability.
Apply this diff:
- if ( - !actionParams.activityContext || - "path" in actionParams.activityContext === false - ) { + if ( + !actionParams.activityContext || + !("path" in actionParams.activityContext) + ) {
641-658: Nit: same readability improvement here.Apply this diff:
- if ( - !actionParams.activityContext || - "path" in actionParams.activityContext === false - ) { + if ( + !actionParams.activityContext || + !("path" in actionParams.activityContext) + ) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- 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)
extensions/plugin-history-sync/src/historySyncPlugin.tsx(13 hunks)integrations/react/src/future/ActivityLayoutComponentType.ts(1 hunks)integrations/react/src/future/loader/loaderPlugin.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integrations/react/src/future/ActivityLayoutComponentType.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: stackflow-docs
🔇 Additional comments (12)
integrations/react/src/future/loader/loaderPlugin.tsx (1)
121-124: Good: preloading the content component (when structured) vs. monolithicUsing contentComponent as the lazy target ensures you prefetch the actual content for structured activities. This aligns with the structured component design and avoids loading non-content wrappers.
extensions/plugin-history-sync/src/historySyncPlugin.tsx (11)
6-11: Imports look good; type import for StackflowActions is appropriate.Bringing StackflowActions in as a type keeps runtime bundle lean. Adding UrlPattern directly here is fine, but ensure the package declares url-pattern as a dependency for this package.
If not already present, add "url-pattern" to extensions/plugin-history-sync/package.json dependencies.
91-120: Queue + entity set design is solid; consider small ergonomics.The queue (pendingDefaultHistoryEntryInsertionTasks) and entity set (defaultHistoryEntryEntities) are a good basis for deterministic default-history seeding. clearPendingDefaultHistoryEntryInsertionTasks correctly resets both.
153-166: Initial path resolution is correct and SSR-aware.Prefers initialContext.req.path in SSR, then hash-based path, then pathname+search. Good coverage.
169-185: Route resolution with fallback is clear and robust.Using makeTemplate.parse for match and falling back to the configured activity name is straightforward and keeps behavior predictable.
287-306: Great: initial default-history activity id is pre-registered and reused.This addresses the earlier sequencing/race where onPushed would clear pending tasks because the initial id wasn’t recognized.
309-316: Good null-guard with search param fallback for activity params.This ensures fallback activities still receive query params when template parsing returns null.
346-356: Replace state tick on init is correct and safely silenced.requestHistoryTick around replaceState with silentFlag prevents feedback loops. Solid.
487-488: Checkpoint trigger after listener wiring is a good kick-off.This ensures pending default-history tasks start processing without waiting for the first onChanged.
553-556: Clearing pending tasks on replace/pop is a safe cleanup.This prevents stale default-history insertions after user-driven navigation changes.
Also applies to: 580-583, 607-616
730-732: onChanged hook is put to good use.This keeps default-history processing responsive to state changes.
200-245: Verify whether activityContext.path should reflect each default-history entry’s own route.Right now, all queued pushes during default-history seeding use activityContext.path = currentPath. If other parts of the system rely on activityContext.path for route reconstruction or analytics, you may want to fill it per-entry using that entry’s route template and params.
I can draft a follow-up that, for each entry, resolves its route and fills the path accordingly. Do you want that change?
Also applies to: 260-284
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (2)
288-299: Good fix: Seed initialActivityId in default-history entity set.This resolves the race where the first onPushed could otherwise clear queued tasks. Thanks for addressing the earlier sequencing bug.
186-196: Fix null-spread crash and brittle trailing-slash match; reuse makeTemplate for params.
UrlPattern.match()returns null on mismatch. Spreading...pathParamswhen it's null will throw at runtime. The hard-coded trailing slash also makes matching brittle. Build params via makeTemplate.parse and guard null; merge query params explicitly.Apply this diff:
- const pattern = new UrlPattern( - `${targetActivityRoute.path}/`, - options.urlPatternOptions, - ); - const url = pathToUrl(currentPath); - const pathParams = pattern.match(url.pathname); - const searchParams = urlSearchParamsToMap(url.searchParams); - const params = { - ...searchParams, - ...pathParams, - }; + const url = pathToUrl(currentPath); + const paramsTemplate = makeTemplate( + targetActivityRoute, + options.urlPatternOptions, + ); + const pathParams = paramsTemplate.parse(currentPath) ?? {}; + const searchParams = urlSearchParamsToMap(url.searchParams); + const params = { + ...searchParams, + ...pathParams, + };
🧹 Nitpick comments (2)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (2)
205-213: Remove non-null assertion on additionalSteps; iterate safely.Even though the enclosing length check makes it practically safe today, the
!is unnecessary and brittle. Use a null-safe iteration, consistent with your later code.- } of initialHistoryEntry.additionalSteps!) { + } of initialHistoryEntry.additionalSteps ?? []) {
98-114: Optionally chain checkpoints to reduce reliance on onChanged cadence.Today you process one task per checkpoint and rely on onChanged to drive the next. To improve resilience when onChanged is sparse, immediately schedule another checkpoint after running a task.
if (nextTask) { - nextTask(actions); + nextTask(actions); + // Process the next task if we're still idle. + defaultHistorySetupCheckpoint(actions); }If you want to avoid immediate recursion, wrap the follow-up call with requestHistoryTick.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- 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)
extensions/plugin-history-sync/src/historySyncPlugin.tsx(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Workers Builds: stackflow-docs
- GitHub Check: Check the status of Changesets
- GitHub Check: Check the TypeScript typings
- GitHub Check: Build all packages
- GitHub Check: Create PR or release packages
- GitHub Check: Check whether the written test passes normally
🔇 Additional comments (5)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (5)
260-285: Good: Fallback to query params when parse returns null.This null-safe merge prevents crashes for fallback routes and preserves query params for deep links. Nicely handled.
488-489: LGTM: Checkpoint triggers wired at init and onChanged.These hooks ensure the default-history queue makes progress as the stack transitions.
Also applies to: 731-733
491-497: LGTM: Defensive clearing of pending default-history tasks.Clearing on mismatched pushes, steps, replacements, and pops prevents default-history invariants from leaking into user-driven navigation.
Also applies to: 522-528, 554-557, 581-584, 608-617
619-637: LGTM: Fill missing activityContext.path on push/replace.The conditional override preserves explicit paths and ensures history state stays aligned with route params when omitted.
Also applies to: 642-659
293-299: Guard possible undefined activityParams in initial default-history event.If a route’s defaultHistory omits activityParams for the initial entry, spreading will throw. Make it null-safe.
makeEvent("Pushed", { activityId: initialActivityId, activityName: initialHistoryEntry.activityName, activityParams: { - ...initialHistoryEntry.activityParams, + ...(initialHistoryEntry.activityParams ?? {}), }, eventDate: enoughPastTime, activityContext: {Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (2)
288-295: LGTM: initial activity id is recorded before the initial Pushed eventRegistering
initialActivityIdindefaultHistoryEntryEntitiesprevents premature clearing of pending default-history tasks. This resolves the sequencing race noted earlier.
246-247: LGTM: safely iterating additionalSteps for subsequent entriesThe
additionalSteps ?? []guard avoids runtime errors when steps are absent.
🧹 Nitpick comments (2)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (2)
98-114: Make the checkpoint self-progressing to avoid stalling when onChanged is sparseToday, one task is executed per checkpoint and we rely on
onChanged/external triggers to continue. To make progress more robust (e.g., when intermediate events don’t fireonChangedreliably), re-schedule the next checkpoint after running a task:function defaultHistorySetupCheckpoint(actions: StackflowActions) { if (!pendingDefaultHistoryEntryInsertionTasks) return; const stack = actions.getStack(); if (stack.globalTransitionState !== "idle") return; const nextTask = pendingDefaultHistoryEntryInsertionTasks.shift(); if (pendingDefaultHistoryEntryInsertionTasks.length === 0) { pendingDefaultHistoryEntryInsertionTasks = null; } if (nextTask) { nextTask(actions); + // Attempt another checkpoint on the next history tick. + // If the stack isn't idle yet, the guard above will prevent progress until it is. + requestHistoryTick(() => defaultHistorySetupCheckpoint(actions)); } }This keeps default-history insertion moving without waiting on incidental lifecycle calls.
94-94: Widen entity-id type for safety
defaultHistoryEntryEntitiesstores ids fromid(),activity.id, andstep.id. If these are not strictlystring(some stacks use numeric ids), theSet<string>type can become misleading. Prefer typing by generator return type:- const defaultHistoryEntryEntities: Set<string> = new Set(); + const defaultHistoryEntryEntities: Set<ReturnType<typeof id>> = new Set();This keeps the set compatible even if the id type changes across versions.
If you prefer explicit types, verify the actual id type in your setup by checking
typeof id()at runtime or referencing the exported type in@stackflow/core.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- 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)
extensions/plugin-history-sync/src/historySyncPlugin.tsx(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build all packages
- GitHub Check: Check whether the written test passes normally
- GitHub Check: Check the TypeScript typings
- GitHub Check: Create PR or release packages
- GitHub Check: Workers Builds: stackflow-docs
🔇 Additional comments (4)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (4)
260-285: LGTM: fallback query-params are preserved for the synthetic pushGood change to guard
template.parse(currentPath)and fall back tourlSearchParamsso deep-linked query params are not dropped on the final synthetic push.
618-637: Path autofill is correct; preserves explicit pathsConditionally filling
activityContext.pathonly when not provided maintains caller intent and keeps history URLs consistent with params.
296-298: Guard against undefined/null activityParams in initial default-history pushIf a route author omits
activityParamson the firstdefaultHistoryentry, spreading it will throw. Add a nullish fallback:- activityParams: { - ...initialHistoryEntry.activityParams, - }, + activityParams: { + ...(initialHistoryEntry.activityParams ?? {}), + },Likely an incorrect or invalid review comment.
186-196: The initial assumption that spreadingnullcrashes in JavaScript is incorrect: object spread ofnullorundefinedsimply results in no properties and does not throw (ES2018 spec) — so a runtime guard forpathParamsisn’t required to prevent an exception. However, reusing an existingmakeTemplate–based parser to avoid duplicated URL logic remains valid ifmakeTemplateis available and intended for this route’s parsing.Next steps:
• Verify that
makeTemplateis imported and returns a parser compatible withtemplate.parse(path).
• IfmakeTemplatealready handles both matching and parameter extraction, refactor to use it exclusively.
• Otherwise, drop the incorrect null-guard suggestion and simply null-coalesce the result ofpattern.match()to avoid undefined merging.Rewrite summary:
Use
makeTemplateparser if available; remove incorrect null-spread guard
- Confirm and import
makeTemplatefrom the module that provides it.- Replace the dual
new UrlPattern+pattern.match()calls with a singletemplate.parse(currentPath)if supported.- Remove the “remove UrlPattern import” only if
UrlPatternis no longer used anywhere else in this file.- Otherwise, keep the existing code and simply default
pathParamsto{}:const pattern = new UrlPattern( `${targetActivityRoute.path}(/)`, options.urlPatternOptions, ); const url = pathToUrl(currentPath); const pathParams = pattern.match(url.pathname) ?? {}; const searchParams = urlSearchParamsToMap(url.searchParams); const params = { ...searchParams, ...pathParams, };This approach corrects the null guard without introducing incorrect assumptions about crashes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
186-196: Fix null spread crash when building params for defaultHistory.
pattern.match(url.pathname)can return null for fallback routes; spreading null into an object throws at runtime. Reuse makeTemplate and guard with a search-params fallback.- const pattern = new UrlPattern( - `${targetActivityRoute.path}(/)`, - options.urlPatternOptions, - ); - const url = pathToUrl(currentPath); - const pathParams = pattern.match(url.pathname); - const searchParams = urlSearchParamsToMap(url.searchParams); - const params = { - ...searchParams, - ...pathParams, - }; + const templateForParams = makeTemplate( + targetActivityRoute, + options.urlPatternOptions, + ); + const params = + templateForParams.parse(currentPath) ?? + urlSearchParamsToMap(pathToUrl(currentPath).searchParams);integrations/react/src/future/loader/loaderPlugin.tsx (1)
124-148: Always attach handlers to loader/lazy promises; pause/resume only when waitingUnhandled rejections can occur when rendering immediately with a component-level loading UI (no handlers are attached). Attach
allSettledunconditionally and gatepause/resumebyshouldWait. This also preserves your corrected"loading" in matchActivityComponentcheck.Apply:
- const shouldRenderImmediately = (activityContext as any) - ?.lazyActivityComponentRenderContext?.shouldRenderImmediately; - - if ( - (loaderDataPromise || lazyComponentPromise) && - (shouldRenderImmediately !== true || - "loading" in matchActivityComponent === false) - ) { - pause(); - - Promise.allSettled([loaderDataPromise, lazyComponentPromise]) - .then(([loaderDataPromiseResult, lazyComponentPromiseResult]) => { - printLoaderDataPromiseError({ - promiseResult: loaderDataPromiseResult, - activityName: matchActivity.name, - }); - printLazyComponentPromiseError({ - promiseResult: lazyComponentPromiseResult, - activityName: matchActivity.name, - }); - }) - .finally(() => { - resume(); - }); - } + const shouldRenderImmediately = (activityContext as any) + ?.lazyActivityComponentRenderContext?.shouldRenderImmediately; + + const shouldWait = + (loaderDataPromise || lazyComponentPromise) && + (shouldRenderImmediately !== true || + "loading" in matchActivityComponent === false); + + const settled = Promise.allSettled([ + loaderDataPromise, + lazyComponentPromise, + ]).then(([loaderDataPromiseResult, lazyComponentPromiseResult]) => { + printLoaderDataPromiseError({ + promiseResult: loaderDataPromiseResult, + activityName: matchActivity.name, + }); + printLazyComponentPromiseError({ + promiseResult: lazyComponentPromiseResult, + activityName: matchActivity.name, + }); + }); + + if (shouldWait) { + pause(); + settled.finally(() => { + resume(); + }); + }integrations/react/src/__internal__/PluginRenderer.tsx (1)
102-110: Remount ErrorBoundary on param changes to clear stale error stateWithout a key, an error from prior params can persist after navigation. Add a stable key derived from params.
- errorHandler?.component ? ( - <StructuredActivityComponentErrorBoundary + errorHandler?.component ? ( + <StructuredActivityComponentErrorBoundary + key={JSON.stringify(params)} renderFallback={(err, reset) => ( <errorHandler.component params={params} error={err} reset={reset} /> )} > {node} </StructuredActivityComponentErrorBoundary> ) : ( node ),
🧹 Nitpick comments (10)
demo/src/activities/Article/Article.layout.tsx (1)
5-5: Guard against undefined title to avoid an empty app bar label.If
titleis optional (as per the Register augmentation), pass a fallback to prevent rendering “undefined”.Apply this diff:
- return <Layout appBar={{ title }}>{children}</Layout>; + return <Layout appBar={{ title: title ?? "Article" }}>{children}</Layout>;If
Layoutalready acceptstitle?: string, feel free to skip.demo/src/activities/Article/Article.tsx (1)
14-18: Add an error handler to surface content-load failures gracefully.Optional but improves UX and observability when
Article.contentfails or throws.Apply this diff:
export const Article = structuredActivityComponent<"Article">({ content: () => import("./Article.content"), layout: ArticleLayout, loading: ArticleLoading, + errorHandler: ({ error }) => ( + <div role="alert" style={{ padding: 16 }}> + Failed to load article. + </div> + ), });extensions/plugin-history-sync/src/historySyncPlugin.tsx (3)
11-11: Remove UrlPattern import (no longer needed after the fix).Keeps bundle lean and avoids unused import warnings.
-import UrlPattern from "url-pattern";
205-213: Avoid non-null assertion on additionalSteps.Guard directly in the loop to drop the
!and simplify.- for (const { - stepParams, - hasZIndex, - } of initialHistoryEntry.additionalSteps!) { + for (const { stepParams, hasZIndex } of (initialHistoryEntry.additionalSteps ?? [])) {
678-694: Double back calls on replace: confirm intent.Each iteration issues two history.back() calls (one conditional, one unconditional). If intentional to collapse both activity and step states, add a short comment; otherwise this may overshoot.
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (2)
60-82: Align lazy error semantics with existing lazy wrapper (optional).React.lazy caches rejections, making retries harder. Consider reusing your project’s lazy wrapper (future/lazy.tsx) semantics for reset-on-error consistency, or memoize the loader similarly.
35-43: Tighten brand check.Also verify the branded flag is true, not just present.
- return ( - value !== null && - typeof value === "object" && - STRUCTURED_ACTIVITY_COMPONENT_TYPE in value - ); + return ( + value !== null && + typeof value === "object" && + (value as any)[STRUCTURED_ACTIVITY_COMPONENT_TYPE] === true + );integrations/react/src/future/loader/loaderPlugin.tsx (1)
111-116: Guard synchronous loader throws
loadDatamay throw synchronously, which would bypass your promise error logging and break routing. Wrap with try/catch and log consistently.- const loaderData = - matchActivity.loader && loadData(activityName, activityParams); + let loaderData: unknown; + try { + loaderData = + matchActivity.loader && loadData(activityName, activityParams); + } catch (err) { + console.error(err); + console.error( + `The above error occurred in the "${matchActivity.name}" activity loader (sync throw).`, + ); + loaderData = undefined; + }integrations/react/src/__internal__/PluginRenderer.tsx (2)
127-158: Optional: log caught errors for visibilityConsider adding
componentDidCatchto surface errors that reach the boundary (helps debugging beyond the loader logs).class StructuredActivityComponentErrorBoundary extends Component< @@ static getDerivedStateFromError(error: unknown): { hasError: true; error: unknown; } { return { hasError: true, error }; } + + componentDidCatch(error: unknown) { + // Keep lightweight; defer advanced reporting to host app + console.error(error); + } }
85-91: Tighten typings for params
params: {}erases safety. If generic threading is difficult here, preferRecord<string, unknown>to reflect runtime shape.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- 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 (10)
demo/src/activities/Article/Article.content.tsx(1 hunks)demo/src/activities/Article/Article.layout.tsx(1 hunks)demo/src/activities/Article/Article.loading.tsx(1 hunks)demo/src/activities/Article/Article.tsx(1 hunks)extensions/plugin-history-sync/src/historySyncPlugin.tsx(13 hunks)integrations/react/src/__internal__/PluginRenderer.tsx(3 hunks)integrations/react/src/__internal__/StructuredActivityComponentType.tsx(1 hunks)integrations/react/src/future/index.ts(1 hunks)integrations/react/src/future/loader/loaderPlugin.tsx(2 hunks)integrations/react/src/future/usePrepare.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- demo/src/activities/Article/Article.loading.tsx
- demo/src/activities/Article/Article.content.tsx
- integrations/react/src/future/usePrepare.ts
🧰 Additional context used
🧬 Code graph analysis (6)
demo/src/activities/Article/Article.tsx (2)
config/src/Register.ts (1)
Register(2-2)integrations/react/src/__internal__/StructuredActivityComponentType.tsx (1)
structuredActivityComponent(19-33)
demo/src/activities/Article/Article.layout.tsx (1)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (2)
layout(88-95)Layout(84-86)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (3)
config/src/RegisteredActivityName.ts (1)
RegisteredActivityName(3-6)config/src/InferActivityParams.ts (1)
InferActivityParams(5-8)integrations/react/src/future/lazy.tsx (1)
lazy(5-28)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (4)
core/src/interfaces/StackflowActions.ts (1)
StackflowActions(15-65)extensions/plugin-history-sync/src/makeHistoryTaskQueue.ts (1)
makeHistoryTaskQueue(7-30)extensions/plugin-history-sync/src/makeTemplate.ts (3)
makeTemplate(48-110)pathToUrl(5-7)urlSearchParamsToMap(9-17)core/src/event-utils/makeEvent.ts (1)
makeEvent(5-19)
integrations/react/src/future/loader/loaderPlugin.tsx (2)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (1)
isStructuredActivityComponent(35-43)core/src/utils/makeActions.ts (2)
pause(100-113)resume(114-127)
integrations/react/src/__internal__/PluginRenderer.tsx (2)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (7)
isStructuredActivityComponent(35-43)structuredActivityComponent(19-33)StructuredActivityComponentType(11-17)getContentComponent(60-82)loading(101-105)errorHandler(111-119)layout(88-95)core/src/Stack.ts (1)
Activity(28-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build all packages
- GitHub Check: Check whether the written test passes normally
- GitHub Check: Create PR or release packages
- GitHub Check: Workers Builds: stackflow-docs
🔇 Additional comments (8)
demo/src/activities/Article/Article.layout.tsx (1)
4-6: Clean use of layout HOC; props destructuring and children passthrough look solid.demo/src/activities/Article/Article.tsx (1)
5-12: Module augmentation for route params is correct and scoped; nice.extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
488-495: Nice: checkpoint gating is sensible.Only progressing the default-history queue when the stack consists solely of default entries is a good guard; else clearing tasks prevents unintended insertions after user actions.
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (1)
19-33: LGTM: structured component factory is clean and type-safe.Branding via a unique symbol and preserving option types looks good.
integrations/react/src/future/index.ts (1)
3-3: LGTM: re-export surfaces the new structured API.Makes the structured helpers available to consumers from a stable entry.
integrations/react/src/future/loader/loaderPlugin.tsx (1)
7-7: Import looks correctStructured-activity detection is correctly wired for lazy prefetch decisions.
integrations/react/src/__internal__/PluginRenderer.tsx (2)
92-119: Correct wrapper order (Suspense inside ErrorBoundary inside Layout)Good fix: renders as Layout(ErrorBoundary(Suspense(Content))). This ensures lazy-load errors are caught and layout wraps final UI.
42-47: Structured vs. function component dispatch looks goodThe branch correctly selects structured rendering when applicable.
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 (2)
integrations/react/src/stable/stackflow.tsx (1)
310-323: Bug: replace() returns a different activityId than the one dispatchedYou compute
activityIdbut dispatch with a new one. The returned ID won’t match the stack entry.Fix:
- replace(activityName, activityParams, options) { - const activityId = options?.activityId ?? makeActivityId(); + replace(activityName, activityParams, options) { + const activityId = options?.activityId ?? makeActivityId(); getCoreStore()?.actions.replace({ - activityId: options?.activityId ?? makeActivityId(), + activityId, activityName, activityParams, skipEnterActiveState: parseActionOptions(options).skipActiveState, }); return { activityId, }; },integrations/react/src/stable/useActions.ts (1)
94-106: Bug: replace() returns an activityId different from the one dispatchedSame mismatch as in stackflow.tsx. Return value won’t correspond to the replaced entry.
Apply:
- replace(activityName, activityParams, options) { - const activityId = makeActivityId(); + replace(activityName, activityParams, options) { + const activityId = options?.activityId ?? makeActivityId(); coreActions?.replace({ - activityId: options?.activityId ?? makeActivityId(), + activityId, activityName, activityParams, skipEnterActiveState: parseActionOptions(options).skipActiveState, }); return { activityId, }; },
🧹 Nitpick comments (2)
integrations/react/src/stable/stackflow.tsx (1)
136-144: Build the map without repeated object spreadsMinor perf/readability: avoid O(n²) spreads in reduce.
Apply:
- const activityComponentMap = Object.entries(options.activities).reduce( - (acc, [key, Activity]) => ({ - ...acc, - [key]: "component" in Activity ? Activity.component : Activity, - }), - {} as { - [key: string]: MonolithicActivityComponentType; - }, - ); + const activityComponentMap: { [key: string]: MonolithicActivityComponentType } = {}; + for (const [key, Activity] of Object.entries(options.activities)) { + activityComponentMap[key] = + "component" in Activity ? Activity.component : (Activity as MonolithicActivityComponentType); + }integrations/react/src/stable/index.ts (1)
3-3: Use type-only re-export to avoid accidental value exportPurely a type alias; prefer
export typeto keep emit clean.-export { MonolithicActivityComponentType as ActivityComponentType } from "../__internal__/MonolithicActivityComponentType"; +export type { MonolithicActivityComponentType as ActivityComponentType } from "../__internal__/MonolithicActivityComponentType";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- 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 (4)
integrations/react/src/stable/BaseActivities.ts(1 hunks)integrations/react/src/stable/index.ts(1 hunks)integrations/react/src/stable/stackflow.tsx(5 hunks)integrations/react/src/stable/useActions.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
integrations/react/src/stable/useActions.ts (1)
integrations/react/src/stable/index.ts (1)
MonolithicActivityComponentType(3-3)
integrations/react/src/stable/BaseActivities.ts (1)
integrations/react/src/stable/index.ts (1)
MonolithicActivityComponentType(3-3)
integrations/react/src/stable/stackflow.tsx (2)
integrations/react/src/stable/index.ts (1)
MonolithicActivityComponentType(3-3)integrations/react/src/__internal__/MonolithicActivityComponentType.ts (1)
MonolithicActivityComponentType(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: stackflow-docs
🔇 Additional comments (8)
integrations/react/src/stable/stackflow.tsx (3)
94-102: Type inference switchover to MonolithicActivityComponentType looks goodThe conditional inference for
useStepFlowparameters matches the new unified type.
109-111: Public addActivity typing aligned with MonolithicActivityComponentTypeConsistent with the new public surface. Good.
261-262: Assigning the raw component is correctThis matches the map’s value type and earlier construction.
integrations/react/src/stable/BaseActivities.ts (2)
2-2: Import update to MonolithicActivityComponentType is consistentMatches the new unified component type.
6-12: BaseActivities union updated correctlyBoth “monolithic” and
{ component, paramsSchema }forms are typed consistently.integrations/react/src/stable/useActions.ts (3)
4-4: Import update aligns with new unified typeLooks good.
31-38: Param inference switch to MonolithicActivityComponentType (push) is correctType inference remains intact.
48-55: Param inference switch to MonolithicActivityComponentType (replace) is correctConsistent with the new public surface.
This PR introduces default history setup options and structured activity components to enable richer deep link experiences in Stackflow applications.
1. Structured Activity Components
StructuredActivityComponentType<P>interface that separates concerns:content: Core component logic (required)layout?: Wrapper for common UI patterns (optional)loading?: Loading state component (optional)error?: Error boundary component (optional)2. Default History Feature
defaultHistoryroute option: Pre-seeds navigation stack for better deep link UXHistoryEntrytype: Strongly typed history entries with activity names and parameters