feat(react): utilize synchronously inspectable promise to optimize suspense fallback rendering #658
Conversation
🦋 Changeset detectedLatest commit: 82ce76b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces React.lazy indirection with a preloadable lazy component and introduces SyncInspectablePromise and useThenable utilities; loader and lazy-loading code paths were updated to use synchronous promise inspection and preload semantics, and one legacy use() helper was removed. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / Router
participant Loader as loaderPlugin
participant Activity as Preloadable Component
participant SIP as SyncInspectablePromise
App->>Loader: request activity + initialLoaderData
Loader->>SIP: resolve(initialLoaderData) -- wrap loader data
Loader->>Activity: call preload() (preloadableLazyComponent)
Activity->>SIP: cachedLoad() → returns SyncInspectablePromise
SIP-->>Activity: status (PENDING / FULFILLED / REJECTED)
alt both fulfilled
Activity->>App: provide Component (default export)
Loader->>App: provide loaderData (fulfilled value)
else pending
SIP-->>App: throws pending promise -> React Suspense boundary suspends
else rejected
SIP-->>App: throws rejection reason -> error boundary handles
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
stackflow-docs | 82ce76b | Commit Preview URL | Dec 16 2025, 07:03 AM |
ENvironmentSet
changed the title
Deploying stackflow-demo with
|
| Latest commit: |
7a93506
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9f97f3bf.stackflow-demo.pages.dev |
| Branch Preview URL: | https://sync-promise-for-suspense-op.stackflow-demo.pages.dev |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
integrations/react/src/__internal__/LazyActivityComponentType.ts (1)
3-6: Breaking change: Type no longer extendsReact.LazyExoticComponent.As noted by the author, this is a breaking change. Consumers relying on
LazyActivityComponentTypebeing aReact.LazyExoticComponent(e.g., for type checks or specific React.lazy behavior) will encounter type errors.Ensure this breaking change is:
- Documented in release notes/changelog
- Reflected in a semver major version bump (or follows the project's versioning policy for breaking changes)
- Migration guidance is provided for affected consumers
🧹 Nitpick comments (2)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (1)
91-109: Redundant double-wrapping withliftValue.On line 93,
content()already returns aSyncInspectablePromise(from the caching logic at lines 42-54), so wrapping it again withliftValueis unnecessary whencontentis a function. Additionally, on lines 104-108,contentPromiseis already aSyncInspectablePromise, so wrapping the.then()result withliftValuecreates another layer of wrapping.Consider simplifying:
const { Component: ContentComponent } = preloadableLazyComponent(() => { const content = structuredActivityComponent.content; - const contentPromise = liftValue( - typeof content === "function" ? content() : { default: content }, - ); + const contentPromise = + typeof content === "function" ? content() : liftValue({ default: content }); const state = inspect(contentPromise); if (state.status === PromiseStatus.FULFILLED) { return liftValue({ default: state.value.default.component, }); } - return liftValue( - contentPromise.then((value) => ({ + return makeSyncInspectable( + contentPromise.then((value) => ({ default: value.default.component, - })), - ); + })), + ) as SyncInspectablePromise<{ default: Content<{}>["component"] }>; });Alternatively, if the current approach is intentional for defensive coding, a brief comment would clarify the intent.
integrations/react/src/future/loader/loaderPlugin.tsx (1)
120-128: Consider extracting the nested ternary for readability.The logic is correct but the nested ternary is dense. Consider extracting to a helper function or using early returns.
- const lazyComponentPromise = liftValue( - isStructuredActivityComponent(matchActivityComponent) && - typeof matchActivityComponent.content === "function" - ? matchActivityComponent.content() - : "_load" in matchActivityComponent && - typeof matchActivityComponent._load === "function" - ? matchActivityComponent._load() - : undefined, - ); + const getLazyComponentResult = () => { + if ( + isStructuredActivityComponent(matchActivityComponent) && + typeof matchActivityComponent.content === "function" + ) { + return matchActivityComponent.content(); + } + if ( + "_load" in matchActivityComponent && + typeof matchActivityComponent._load === "function" + ) { + return matchActivityComponent._load(); + } + return undefined; + }; + const lazyComponentPromise = liftValue(getLazyComponentResult());
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
integrations/react/src/__internal__/LazyActivityComponentType.ts(1 hunks)integrations/react/src/__internal__/StructuredActivityComponentType.tsx(3 hunks)integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx(1 hunks)integrations/react/src/__internal__/utils/SyncInspectablePromise.ts(1 hunks)integrations/react/src/future/lazy.tsx(1 hunks)integrations/react/src/future/loader/loaderPlugin.tsx(6 hunks)integrations/react/src/future/stackflow.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write source in TypeScript with strict typing enabled across the codebase
Files:
integrations/react/src/future/stackflow.tsxintegrations/react/src/__internal__/utils/PreloadableLazyComponent.tsxintegrations/react/src/__internal__/utils/SyncInspectablePromise.tsintegrations/react/src/future/lazy.tsxintegrations/react/src/future/loader/loaderPlugin.tsxintegrations/react/src/__internal__/StructuredActivityComponentType.tsxintegrations/react/src/__internal__/LazyActivityComponentType.ts
integrations/react/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use .tsx files for React components and JSX in the React integration package
Files:
integrations/react/src/future/stackflow.tsxintegrations/react/src/__internal__/utils/PreloadableLazyComponent.tsxintegrations/react/src/future/lazy.tsxintegrations/react/src/future/loader/loaderPlugin.tsxintegrations/react/src/__internal__/StructuredActivityComponentType.tsx
🧬 Code graph analysis (6)
integrations/react/src/future/stackflow.tsx (1)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (2)
SyncInspectablePromise(3-7)liftValue(73-89)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (1)
integrations/react/src/__internal__/utils/isPromiseLike.ts (1)
isPromiseLike(1-8)
integrations/react/src/future/lazy.tsx (4)
integrations/react/src/__internal__/StaticActivityComponentType.ts (1)
StaticActivityComponentType(3-4)integrations/react/src/__internal__/LazyActivityComponentType.ts (1)
LazyActivityComponentType(3-6)integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx (1)
preloadableLazyComponent(9-35)integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (1)
liftValue(73-89)
integrations/react/src/future/loader/loaderPlugin.tsx (2)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (5)
SyncInspectablePromise(3-7)liftValue(73-89)inspect(29-49)PromiseStatus(9-13)PromiseStatus(14-14)integrations/react/src/__internal__/StructuredActivityComponentType.tsx (1)
isStructuredActivityComponent(59-67)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (2)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (5)
SyncInspectablePromise(3-7)liftValue(73-89)inspect(29-49)PromiseStatus(9-13)PromiseStatus(14-14)integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx (1)
preloadableLazyComponent(9-35)
integrations/react/src/__internal__/LazyActivityComponentType.ts (1)
integrations/react/src/__internal__/StaticActivityComponentType.ts (1)
StaticActivityComponentType(3-4)
⏰ 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 (22)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (2)
5-12: LGTM!The imports are correctly organized, bringing in the necessary utilities for the new preloadable lazy loading pattern and synchronously inspectable promises.
36-54: LGTM! Well-structured caching with proper rejected-state refresh.The caching logic correctly:
- Returns a pre-resolved
SyncInspectablePromisefor static content vialiftValue- Refreshes the cache when the previous promise was rejected
- Maintains a single cached promise for successful loads
integrations/react/src/future/stackflow.tsx (4)
23-26: LGTM!Appropriate imports for the new SyncInspectablePromise-based loader data handling.
67-74: LGTM!The type changes correctly align the cache and
loadDatasignature with the newSyncInspectablePromisepattern, enabling synchronous inspection of loader data state.
92-97: LGTM!Wrapping the loader result with
liftValuecorrectly handles both synchronous values and promises, converting them toSyncInspectablePromise.
127-131: Rethrowing in.then()rejection handler is a no-op for unhandled rejection propagation.The
throw erroron line 130 inside the rejection callback won't propagate the error to callers ofloadData()— it only causes the returned promise from.then()to reject (which is ignored). The originalloaderDatapromise's rejection is already propagated. Thethrowhere might be intentional to surface the error in dev tools, but if so, a comment would clarify intent.Is the
throw errorintentional for logging/debugging, or should it be removed sinceloaderDataalready propagates rejections to consumers?integrations/react/src/future/lazy.tsx (2)
3-4: LGTM!Correctly imports the new preloadable lazy loading utilities.
9-22: LGTM! Clean refactor to preloadable pattern.The implementation correctly:
- Uses
preloadableLazyComponentwithliftValue(load())to wrap the loader- Exposes
_loadthat awaitspreload()and returns the wrapped Component- Maintains the
LazyActivityComponentType<T>contractintegrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx (1)
9-34: LGTM! Well-designed preloadable lazy component pattern.The implementation correctly:
- Caches the loading promise and refreshes on rejection (lines 15-21)
- Uses React's
usehook for suspense integration (line 26)- Provides a
preloadfunction for eager loading (line 33)This pattern effectively enables synchronous inspection of the promise state, which is the core optimization for avoiding unnecessary suspense fallbacks.
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (6)
3-7: Well-designed interface for synchronous promise inspection.The interface correctly extends
Promise<T>with the additional fields needed for synchronous state inspection, which aligns with React's recommended pattern for optimizinguse()hook rendering.
9-14: LGTM!The
as constpattern with dual const/type export is idiomatic TypeScript and provides good type safety.
16-27: LGTM!Clean discriminated union type that enables proper type narrowing when handling promise states.
29-49: Good defensive validation in the inspect function.The function correctly validates both the
statusfield and the presence ofvalue/reasonfields before returning the typed state. The fallback error throw at line 47-48 handles any potential invalid state.
51-71: LGTM with a note on async status updates.The
.then()handlers update status asynchronously (microtask), meaning if an already-resolved nativePromiseis wrapped, the status won't reflectFULFILLEDsynchronously. This is acceptable for the intended use case (data loaders, dynamic imports) which are inherently async. The key optimization benefits come from preserving already-inspectable promises vialiftValue's early return path.
73-89: Correct handling of all value types.The function properly handles three cases:
- Existing
SyncInspectablePromiseinstances are reused (preserving optimization).- Plain
PromiseLikeobjects are wrapped withmakeSyncInspectable.- Raw values are immediately wrapped as fulfilled promises with synchronously-available status.
The double type assertion at line 79 is a pragmatic choice given the runtime validation at lines 76-77.
integrations/react/src/future/loader/loaderPlugin.tsx (7)
8-13: LGTM!Clean imports with proper use of type-only import for the interface.
23-26: LGTM!The explicit
SyncInspectablePromise<unknown>return type ensures consistent promise handling throughout the loader flow.
46-46: LGTM!Using
liftValueensuresinitialLoaderDatais normalized to aSyncInspectablePromise, enabling consistent handling regardless of whether the initial data is a raw value or a promise.
65-70: LGTM!Good use of
Promise.allSettledto ensure errors are logged without disrupting the flow.
98-101: LGTM!Consistent type signature with the parent function.
132-137: LGTM!The short-circuit evaluation correctly handles the optional
loaderData, andinspect()provides synchronous status checks as intended for the suspense optimization.
140-153: LGTM!
Promise.allSettledhandles the potentially undefinedloaderDatagracefully (treating it as an immediately-fulfilled value), and the.finally()ensuresresume()is always called.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/wild-houses-see.md (1)
1-5: Changeset looks correct; consider clarifying the breaking type change.This is a valid Changesets entry and it’s good to see
@stackflow/reactis called out. The description is quite terse—consider explicitly naming the affected public type(s) and what changed (e.g.,LazyActivityComponentTypeno longer being aReact.LazyExoticComponent, and any_load/preload expectations). Also please double-check thatmajormatches your SemVer/breaking-change intent for consumers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/happy-ghosts-peel.md(1 hunks).changeset/wild-houses-see.md(1 hunks)integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/happy-ghosts-peel.md
🚧 Files skipped from review as they are similar to previous changes (1)
- integrations/react/src/internal/utils/PreloadableLazyComponent.tsx
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Include a Changeset entry for any user-facing package change
Files:
.changeset/wild-houses-see.md
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
integrations/react/package.json (1)
44-45: Consider aligning peerDependencies with react18-use requirements.The
react18-usepackage (as its name suggests) is designed for React 18, butpeerDependenciescurrently allowsreact >=16.8.0. Users on React 16/17 may encounter runtime issues with theusehook polyfill. Consider updating the peer dependency toreact >=18.0.0or documenting that the/futureexport path requires React 18+.integrations/react/src/__internal__/StructuredActivityComponentType.tsx (1)
92-110: Slight redundancy inliftValuecalls, but functionally correct.The implementation correctly handles both sync and async content resolution. However,
liftValueon lines 100-102 wraps a synchronously created object, andliftValueon lines 105-109 wraps the chained promise. Both are necessary for the return type, but the pattern could be simplified by always using thecontentPromise.then(...)path sinceliftValuehandles already-fulfilled promises efficiently.Consider simplifying to a single return path:
- if (state.status === PromiseStatus.FULFILLED) { - return liftValue({ - default: state.value.default.component, - }); - } - - return liftValue( - contentPromise.then((value) => ({ + return liftValue( + contentPromise.then((value) => ({ default: value.default.component, })), ); - });Since
liftValuealready optimizes for fulfilled promises internally, this achieves the same suspense optimization with less code.integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (1)
73-90: Consider optimizing the status check for better performance.Lines 75-78 perform a heuristic check to detect if the value is already a
SyncInspectablePromise. The check on line 78 usesObject.values(PromiseStatus).some(...), which iterates through all status values on every call.For better performance, consider one of these alternatives:
Option 1: Direct comparison (most efficient)
if ( value instanceof Promise && "status" in value && - Object.values(PromiseStatus).some((status) => status === value.status) + (value.status === PromiseStatus.PENDING || + value.status === PromiseStatus.FULFILLED || + value.status === PromiseStatus.REJECTED) ) {Option 2: Use a Set for O(1) lookup (if you prefer DRY)
const VALID_STATUSES = new Set(Object.values(PromiseStatus)); export function liftValue<T>(value: T): SyncInspectablePromise<Awaited<T>> { if (isPromiseLike(value)) { if ( value instanceof Promise && "status" in value && VALID_STATUSES.has(value.status) ) { return value as SyncInspectablePromise<Awaited<T>>; } // ...
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.pnp.cjs(2 hunks)integrations/react/package.json(1 hunks)integrations/react/src/__internal__/StructuredActivityComponentType.tsx(3 hunks)integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx(1 hunks)integrations/react/src/__internal__/utils/SyncInspectablePromise.ts(1 hunks)integrations/react/src/future/lazy.tsx(1 hunks)integrations/react/src/future/loader/use.ts(0 hunks)integrations/react/src/future/loader/useLoaderData.ts(1 hunks)integrations/react/src/future/stackflow.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- integrations/react/src/future/loader/use.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- integrations/react/src/internal/utils/PreloadableLazyComponent.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write source in TypeScript with strict typing enabled across the codebase
Files:
integrations/react/src/future/loader/useLoaderData.tsintegrations/react/src/future/lazy.tsxintegrations/react/src/future/stackflow.tsxintegrations/react/src/__internal__/utils/SyncInspectablePromise.tsintegrations/react/src/__internal__/StructuredActivityComponentType.tsx
integrations/react/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use .tsx files for React components and JSX in the React integration package
Files:
integrations/react/src/future/lazy.tsxintegrations/react/src/future/stackflow.tsxintegrations/react/src/__internal__/StructuredActivityComponentType.tsx
🧬 Code graph analysis (3)
integrations/react/src/future/lazy.tsx (2)
integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx (1)
preloadableLazyComponent(9-35)integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (4)
liftValue(73-90)inspect(29-49)PromiseStatus(9-13)PromiseStatus(14-14)
integrations/react/src/future/stackflow.tsx (1)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (2)
SyncInspectablePromise(3-7)liftValue(73-90)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (1)
integrations/react/src/__internal__/utils/isPromiseLike.ts (1)
isPromiseLike(1-8)
⏰ 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 (11)
.pnp.cjs (1)
6857-6857: The [email protected] dependency addition is valid and secure.The
.pnp.cjschanges are correctly auto-generated by Yarn based on theintegrations/react/package.jsonupdate. Verification confirms:
- Version constraint
^0.4.1in package.json matches the entry in.pnp.cjs- Package
[email protected]exists on npm with no known security vulnerabilities- Virtual path hashes are consistent across both hunks
integrations/react/src/future/stackflow.tsx (4)
23-26: LGTM!Imports for
liftValueandSyncInspectablePromiseare correctly added to support the new promise handling pattern.
67-74: LGTM!Type updates for
loaderDataCacheMapandloadDatacorrectly reflect the transition toSyncInspectablePromise<unknown>for synchronous promise inspection.
92-97: LGTM!Wrapping the loader result with
liftValuecorrectly converts it to aSyncInspectablePromise, enabling synchronous status inspection. This handles the case whereloaderis undefined (returning a fulfilled promise withundefined).
127-129: LGTM!The simplified promise chain correctly handles cache invalidation. Since
loaderDatais aSyncInspectablePromise, rejection errors are captured in itsstatusandreasonproperties for downstream inspection, making explicit re-throwing unnecessary here.integrations/react/src/future/lazy.tsx (3)
3-8: LGTM!Imports correctly bring in the utilities needed for the new preloadable lazy component pattern.
13-15: LGTM!The factory function correctly wraps the dynamic import with
liftValueto create aSyncInspectablePromise, enabling synchronous status inspection withinpreloadableLazyComponent.
17-30: LGTM!The
_loadmethod correctly implements the suspense optimization pattern:
- Returns a synchronously fulfilled promise when preload is complete, avoiding fallback exposure
- Chains on the pending preload otherwise, maintaining correct async behavior
This aligns well with the PR objective of optimizing suspense fallback rendering.
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (2)
5-12: LGTM!Imports correctly updated to support the new preloadable lazy loading pattern with
SyncInspectablePromiseutilities.
36-55: LGTM!Content caching logic correctly:
- Invalidates cache on rejection to allow retry
- Reuses pending/fulfilled promises to prevent duplicate loads
- Uses
liftValueto wrap the content promise for synchronous inspectionintegrations/react/src/future/loader/useLoaderData.ts (1)
2-2: Switch toreact18-useis compatible with your setup.The
react18-useversion (^0.4.1) is correctly configured for React 18.3.1. Theuse()hook properly handles theSyncInspectablePromise(an internal stackflow utility) passed through activity context since it extends the standard Promise interface. Note: the status inspection capabilities (isResolved,isPending,isRejected) come from stackflow's internalSyncInspectablePromiseutility, not fromreact18-useitself—react18-useprovides the Promise suspension mechanism that React 18'suse()hook needs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integrations/react/src/future/lazy.tsx (1)
18-34: Logic is sound; consider caching _load result for efficiency.The _load implementation correctly handles all three states (fulfilled, rejected, pending) and properly delegates to the preloadable component mechanism. The flow is:
- When preload is already fulfilled → immediate return
- When preload is rejected → returns error via liftError
- When preload is pending → returns promise that resolves after preload completes
However, each _load invocation creates a new
liftValue(preload())wrapper (line 22), even though the underlying component load is cached in preloadableLazyComponent. While functionally correct, this could be optimized by caching the first _load result.Consider caching the _load result to avoid redundant promise wrapping:
export function lazy<T extends { [K in keyof T]: any } = {}>( load: () => Promise<{ default: StaticActivityComponentType<T> }>, ): LazyActivityComponentType<T> { const { Component, preload } = preloadableLazyComponent(() => liftValue(load()), ); + let cachedLoadResult: ReturnType<LazyActivityComponentType<T>['_load']> | null = null; + const LazyActivityComponent: LazyActivityComponentType<T> = Object.assign( Component, { _load: () => { + if (cachedLoadResult) { + return cachedLoadResult; + } + const preloadTask = liftValue(preload()); const preloadTaskState = inspect(preloadTask); if (preloadTaskState.status === PromiseStatus.FULFILLED) { - return liftValue({ default: Component }); + cachedLoadResult = liftValue({ default: Component }); } else if (preloadTaskState.status === PromiseStatus.REJECTED) { return liftError(preloadTaskState.reason); + } else { + cachedLoadResult = liftValue(preloadTask.then(() => ({ default: Component }))); } - return liftValue(preloadTask.then(() => ({ default: Component }))); + return cachedLoadResult; }, }, ); return LazyActivityComponent; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts(1 hunks)integrations/react/src/future/lazy.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integrations/react/src/internal/utils/SyncInspectablePromise.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write source in TypeScript with strict typing enabled across the codebase
Files:
integrations/react/src/future/lazy.tsx
integrations/react/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use .tsx files for React components and JSX in the React integration package
Files:
integrations/react/src/future/lazy.tsx
🧬 Code graph analysis (1)
integrations/react/src/future/lazy.tsx (4)
integrations/react/src/__internal__/StaticActivityComponentType.ts (1)
StaticActivityComponentType(3-4)integrations/react/src/__internal__/LazyActivityComponentType.ts (1)
LazyActivityComponentType(3-6)integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx (1)
preloadableLazyComponent(9-35)integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (5)
liftValue(73-90)inspect(29-49)PromiseStatus(9-13)PromiseStatus(14-14)liftError(92-97)
⏰ 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 (2)
integrations/react/src/future/lazy.tsx (2)
3-9: LGTM! Clean integration with new preloadable utilities.The imports correctly bring in the new preloadableLazyComponent utility and SyncInspectablePromise utilities (inspect, liftError, liftValue, PromiseStatus) that form the foundation of the refactored lazy loading implementation.
14-16: LGTM! Correct usage of preloadableLazyComponent.The initialization properly wraps the user-provided load function with liftValue to create a SyncInspectablePromise, which aligns with the preloadableLazyComponent's expected signature. The lazy wrapper ensures load() is only invoked when needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (1)
93-113: Implementation correctly enables the suspense optimization.The promise state inspection allows synchronous resolution when content is already loaded, avoiding unnecessary Suspense fallbacks per the PR objective.
Minor readability suggestion: The final return block implicitly handles the
PENDINGstate. Consider adding a comment or making this explicit for clarity:} else if (state.status === PromiseStatus.REJECTED) { return liftError(state.reason); } + // PENDING: content is still loading, return a promise that resolves to the component return liftValue( contentPromise.then((value) => ({ default: value.default.component, })), );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write source in TypeScript with strict typing enabled across the codebase
Files:
integrations/react/src/__internal__/StructuredActivityComponentType.tsx
integrations/react/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use .tsx files for React components and JSX in the React integration package
Files:
integrations/react/src/__internal__/StructuredActivityComponentType.tsx
🧬 Code graph analysis (1)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (2)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (6)
SyncInspectablePromise(3-7)inspect(29-49)PromiseStatus(9-13)PromiseStatus(14-14)liftValue(73-90)liftError(92-97)integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx (1)
preloadableLazyComponent(9-35)
⏰ 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 (2)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (2)
5-13: LGTM!The import changes properly reflect the migration from
React.lazyto the newpreloadableLazyComponentapproach withSyncInspectablePromiseutilities. Using type-only imports for React types is a good practice.
37-56: LGTM!The caching logic correctly mirrors the pattern used in
preloadableLazyComponent. The implementation properly:
- Caches the promise per structured activity component instance
- Refreshes on rejection to enable retry behavior
- Uses
liftValueto ensure the promise is synchronously inspectable
orionmiz
left a comment
orionmiz
left a comment
There was a problem hiding this comment.
Both motivation and implementation make senses.
My only suggestion is to choose familiar names for promise utility functions.
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts
Outdated
Show resolved
Hide resolved
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts
Outdated
Show resolved
Hide resolved
ENvironmentSet
force-pushed
the
sync-promise-for-suspense-opt
branch
from
c747cf7 to
b7360c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (2)
3-7: Type safety consideration: Optional fields without discriminated union.The interface allows
valueandreasonto be undefined regardless ofstatus, which could lead to unsafe access. However, theinspect()function properly returns a discriminated union (PromiseState<T>) that enforces the relationship between status and field presence, mitigating this concern for external consumers.Consider documenting that raw
SyncInspectablePromiseshould only be accessed viainspect()for type-safe state queries.
73-90: Consider strengthening SyncInspectablePromise detection with a unique marker.Lines 75-79 rely on duck-typing to detect existing
SyncInspectablePromiseinstances by checking for astatusproperty with matching enum values. While this works in practice since native Promises don't have astatusproperty by default, it could theoretically misidentify any Promise that someone manually assigns astatusproperty with a value matching PromiseStatus enum values, potentially causinginspect()to throw "Invalid promise state" ifvalueorreasonproperties are missing.Adding an explicit marker would make detection more explicit and robust:
export interface SyncInspectablePromise<T> extends Promise<T> { __isSyncInspectable: true; status: PromiseStatus; value?: T; reason?: unknown; } // In resolve function if ( value instanceof Promise && "__isSyncInspectable" in value && value.__isSyncInspectable === true ) { return value as SyncInspectablePromise<Awaited<T>>; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
.changeset/happy-ghosts-peel.md(1 hunks)integrations/react/src/__internal__/LazyActivityComponentType.ts(1 hunks)integrations/react/src/__internal__/StructuredActivityComponentType.tsx(3 hunks)integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx(1 hunks)integrations/react/src/__internal__/utils/SyncInspectablePromise.ts(1 hunks)integrations/react/src/__internal__/utils/useThenable.ts(1 hunks)integrations/react/src/future/lazy.tsx(1 hunks)integrations/react/src/future/loader/loaderPlugin.tsx(6 hunks)integrations/react/src/future/loader/use.ts(0 hunks)integrations/react/src/future/loader/useLoaderData.ts(1 hunks)integrations/react/src/future/stackflow.tsx(0 hunks)
💤 Files with no reviewable changes (2)
- integrations/react/src/future/stackflow.tsx
- integrations/react/src/future/loader/use.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/happy-ghosts-peel.md
- integrations/react/src/future/loader/useLoaderData.ts
- integrations/react/src/internal/utils/PreloadableLazyComponent.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write source in TypeScript with strict typing enabled across the codebase
Files:
integrations/react/src/__internal__/StructuredActivityComponentType.tsxintegrations/react/src/future/lazy.tsxintegrations/react/src/future/loader/loaderPlugin.tsxintegrations/react/src/__internal__/utils/SyncInspectablePromise.tsintegrations/react/src/__internal__/LazyActivityComponentType.tsintegrations/react/src/__internal__/utils/useThenable.ts
integrations/react/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use .tsx files for React components and JSX in the React integration package
Files:
integrations/react/src/__internal__/StructuredActivityComponentType.tsxintegrations/react/src/future/lazy.tsxintegrations/react/src/future/loader/loaderPlugin.tsx
🧬 Code graph analysis (6)
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (3)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (6)
SyncInspectablePromise(3-7)inspect(29-49)PromiseStatus(9-13)PromiseStatus(14-14)resolve(73-90)reject(92-97)config/src/InferActivityParams.ts (1)
InferActivityParams(5-8)integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx (1)
preloadableLazyComponent(9-35)
integrations/react/src/future/lazy.tsx (4)
integrations/react/src/__internal__/StaticActivityComponentType.ts (1)
StaticActivityComponentType(3-4)integrations/react/src/__internal__/LazyActivityComponentType.ts (1)
LazyActivityComponentType(3-6)integrations/react/src/__internal__/utils/PreloadableLazyComponent.tsx (1)
preloadableLazyComponent(9-35)integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (5)
resolve(73-90)inspect(29-49)PromiseStatus(9-13)PromiseStatus(14-14)reject(92-97)
integrations/react/src/future/loader/loaderPlugin.tsx (1)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (5)
SyncInspectablePromise(3-7)resolve(73-90)inspect(29-49)PromiseStatus(9-13)PromiseStatus(14-14)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (1)
integrations/react/src/__internal__/utils/isPromiseLike.ts (1)
isPromiseLike(1-8)
integrations/react/src/__internal__/LazyActivityComponentType.ts (1)
integrations/react/src/__internal__/StaticActivityComponentType.ts (1)
StaticActivityComponentType(3-4)
integrations/react/src/__internal__/utils/useThenable.ts (1)
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (4)
resolve(73-90)inspect(29-49)PromiseStatus(9-13)PromiseStatus(14-14)
⏰ 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 (12)
integrations/react/src/__internal__/utils/useThenable.ts (1)
3-14: LGTM! Clean suspense integration.The implementation correctly follows React's suspense pattern by:
- Converting the thenable to a synchronously inspectable promise
- Returning the value when fulfilled
- Throwing the error when rejected
- Throwing the promise when pending to trigger suspense boundaries
integrations/react/src/__internal__/utils/SyncInspectablePromise.ts (3)
29-49: LGTM! Proper discriminated union handling.The
inspect()function correctly returns a type-safe discriminated union by checking both the status and property existence before accessingvalueorreason. The defensive error throw for invalid states is good practice.
51-71: Promise mutation pattern is correct but unconventional.The function creates a promise and mutates it by attaching
status,value, andreasonproperties throughthenhandlers. This is the core mechanism enabling synchronous inspection and is working as intended. The promise returned at line 59 is intentionally not stored since we only need the side effect of updating the promise state.
92-97: LGTM! Straightforward rejected promise creation.The function correctly creates a rejected promise and immediately marks it with the appropriate status and reason, ensuring synchronous inspectability.
integrations/react/src/__internal__/StructuredActivityComponentType.tsx (3)
1-13: LGTM! Imports correctly support the new preloadable architecture.The imports include the necessary utilities for synchronously inspectable promises and preloadable lazy components.
37-56: LGTM! Proper caching with error recovery.The implementation correctly caches the content loading promise and automatically retries on rejection, which provides good error recovery behavior. The use of
resolve()ensures the promise is synchronously inspectable.
93-113: LGTM! Comprehensive state handling in content component loading.The rewrite correctly handles all three promise states:
- FULFILLED: Immediately returns the resolved component
- REJECTED: Propagates the error
- PENDING: Returns a promise that will resolve to the component when ready
The use of
preloadableLazyComponentintegrates seamlessly with the new synchronously inspectable promise system.integrations/react/src/future/lazy.tsx (1)
11-37: LGTM! Proper integration with preloadable lazy components.The rewrite correctly:
- Uses
preloadableLazyComponentto create a component that leverages cached loading- Implements
_loadmethod that returns synchronously inspectable promises- Handles all three states (fulfilled, rejected, pending) appropriately
- Ensures
Componentis ready to render without suspending after preload completesintegrations/react/src/future/loader/loaderPlugin.tsx (4)
41-49: LGTM! Consistent SyncInspectablePromise wrapping.Wrapping
initialLoaderDatawithresolve()ensures that the loader data is always a synchronously inspectable promise, maintaining consistency with the new architecture.
63-70: LGTM! Simplified and consistent error handling.The change to always treat
loaderDataas aSyncInspectablePromiseeliminates the need for runtime type checking withisPromiseLike, resulting in cleaner and more predictable code.
120-128: LGTM! Robust lazy component promise resolution with defensive checks.The implementation correctly handles multiple component types (structured with function content, structured with direct content, lazy with
_load, and static). The added type check fortypeof matchActivityComponent._load === "function"on line 125 provides good defensive programming.
132-154: LGTM! Key optimization using synchronous promise inspection.The updated pending state check using
inspect()is the core optimization of this PR. It allows checking promise status synchronously, avoiding unnecessary pauses and suspense fallback rendering when promises are already settled. ThePromise.allSettledpattern correctly handles both loader data and lazy component promises, including the case whereloaderDatais undefined.
React team is going to recommend passing synchronously inspectable promise to
usefor avoiding unnecessary fallback exposure. I followed their guidelines to optimize renderings incorporating data loaders/lazy componentsThis optimization matters since react applies default delay before removing fallbacks have shown.