feat(core, plugin-history-sync): Initial stack setup process exposure #630
Conversation
🦋 Changeset detectedLatest commit: 6a24370 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a required Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Plugin as historySyncPlugin
participant Proc as SerialNavigationProcess
participant Core as @stackflow/core
participant Mon as ActivationMonitors
participant Pub as Publisher
App->>Plugin: onInit()
Plugin->>Proc: captureNavigationOpportunity(stack=null, t0)
alt pending navigations exist
Proc-->>Plugin: [Pushed/StepPushed events]
Plugin->>Core: dispatchEvent(events)
Core-->>Plugin: updated Stack (includes events[])
Plugin->>Mon: captureStackChange(stack)
Plugin->>Pub: publish(status/progress)
else none
Proc-->>Plugin: []
Plugin->>Pub: publish(status/succeeded)
end
sequenceDiagram
autonumber
participant UI as UI Subscriber
participant Pub as Publisher
participant Mx as Mutex
participant Sub1 as Subscriber A
participant Sub2 as Subscriber B
UI->>Pub: subscribe(handler)
note over Pub: stores handler snapshot
UI->>Pub: publish(value)
Pub->>Mx: runExclusively(...)
activate Mx
Mx-->>Pub: lock acquired
par notify all subscribers
Pub->>Sub1: handler(value)
Sub1-->>Pub: resolved/rejected
Pub->>Sub2: handler(value)
Sub2-->>Pub: resolved/rejected
end
Pub-->>UI: Promise.allSettled results
deactivate Mx
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
stackflow-docs | 6a24370 | Commit Preview URL | Oct 02 2025, 02:38 AM |
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/activity-utils/makeStackReducer.ts (1)
132-144:Resumedevent not logged when replayingpausedEventsWhen resuming from a paused state, the
Resumedevent itself is omitted fromstack.events, breaking the “append each processed event” guarantee.- const { pausedEvents, ...rest } = stack; - return pausedEvents.reduce(reducer, { - ...rest, - globalTransitionState: "idle", - }); + const { pausedEvents, ...rest } = stack; + return pausedEvents.reduce(reducer, { + ...rest, + globalTransitionState: "idle", + // Log the Resumed event in processing order + events: [...rest.events, event], + });
🧹 Nitpick comments (18)
extensions/plugin-history-sync/src/useIsEntryActivity.ts (1)
3-6: Ensure the hook always returns a booleanThe current return can be
undefined. Coerce to boolean (and avoid leakinganybehavior).export function useIsEntryActivity(): boolean { const { context } = useActivity(); - return (context as any)?.isEntryActivity; + return Boolean((context as any)?.isEntryActivity); }demo/src/stackflow/stackflow.config.ts (1)
25-31: Default history now pre-includes an Article; verify duplication is intendedWhen landing on an Article route, this will likely produce two Article activities (one from defaultHistory + the target route). If that’s not desired, drop the hard-coded Article or make it conditional.
defaultHistory: () => [ { activityName: "Main", activityParams: {}, }, - { - activityName: "Article", - activityParams: { - articleId: 60547101, - title: "울랄라", - }, - }, ],Also,
decodeaccessesparams.titlebut the path doesn’t include:title; confirm you rely on query params for it.extensions/plugin-history-sync/src/withResolvers.ts (1)
1-15: Match Promise resolver types to lib definitionsWiden resolver signatures to align with TS Promise constructor (
T | PromiseLike<T>andany), avoiding assignability edge cases.export function withResolvers<T>(): { promise: Promise<T>; - resolve: (value: T) => void; - reject: (reason?: unknown) => void; + resolve: (value: T | PromiseLike<T>) => void; + reject: (reason?: any) => void; } { - let resolve!: (value: T) => void; - let reject!: (reason?: unknown) => void; + let resolve!: (value: T | PromiseLike<T>) => void; + let reject!: (reason?: any) => void; const promise = new Promise<T>((_resolve, _reject) => { resolve = _resolve; reject = _reject; });extensions/plugin-history-sync/src/NavigationEvent.ts (1)
19-29: Minor cleanup: avoid repetitive string comparisonsUse a Set for membership; easier to maintain if events grow.
-export function isNavigationEvent( - event: DomainEvent, -): event is NavigationEvent { - return ( - event.name === "Pushed" || - event.name === "Popped" || - event.name === "Replaced" || - event.name === "StepPushed" || - event.name === "StepPopped" || - event.name === "StepReplaced" - ); -} +const NAV_EVENT_NAMES = new Set([ + "Pushed", + "Popped", + "Replaced", + "StepPushed", + "StepPopped", + "StepReplaced", +] as const); + +export function isNavigationEvent(event: DomainEvent): event is NavigationEvent { + return NAV_EVENT_NAMES.has(event.name as (typeof NAV_EVENT_NAMES extends Set<infer U> ? U : never)); +}extensions/plugin-history-sync/src/Mutex.ts (2)
2-7: Naming nit: clarify queue tail field“latestlyBookedSession” is hard to parse. Consider a clearer name like “tail” or “lastBookedSession”. No behavior change.
export class Mutex { - private latestlyBookedSession: Promise<void> = Promise.resolve(); + private lastBookedSession: Promise<void> = Promise.resolve(); acquire(): Promise<{ release: () => void }> { return new Promise((resolveSessionHandle) => { - this.latestlyBookedSession = this.latestlyBookedSession.then( + this.lastBookedSession = this.lastBookedSession.then( () => new Promise((resolveSession) => resolveSessionHandle({ release: () => resolveSession() }), ), ); });
15-23: Optional: accept sync thunksBroaden the thunk type so callers can pass sync or async work; your
awaitalready handles both.- async runExclusively<T>(thunk: () => Promise<T>): Promise<Awaited<T>> { + async runExclusively<T>(thunk: () => T | Promise<T>): Promise<Awaited<T>> { const { release } = await this.acquire(); try { return await thunk(); } finally { release(); } }extensions/plugin-history-sync/src/InitialSetupProcessStatusContext.ts (1)
10-12: Use explicit null check to avoid false negativesIf
NavigationProcessStatusever becomes a falsy value,!statuswould throw incorrectly.- if (!status) { + if (status === null) { throw new Error("InitialSetupProcessStatusContext is not found"); }extensions/plugin-history-sync/src/NavigationProcess/StatusPublishingNavigationProcess.ts (2)
45-49: Status equality check may republish unnecessarily
!==compares by reference. IfNavigationProcessStatusis an object (not a string/enum), identical statuses with new object identities will trigger publishes.If status is an object, either:
- make
NavigationProcessStatusa stable string/enum, or- compare by a stable key (e.g.,
status.kind), or- add a shallow/deep compare utility.
Example (if
kindexists):- private refreshCurrentStatus(nextStatus: NavigationProcessStatus) { - if (nextStatus !== this.currentStatus) { + private refreshCurrentStatus(nextStatus: NavigationProcessStatus) { + if (nextStatus.kind !== this.currentStatus.kind) { this.currentStatus = nextStatus; this.publisher.publish(nextStatus); } }
8-12: Minor: mark fields readonlyThese dependencies aren’t reassigned; mark as
readonlyfor intent.export class StatusPublishingNavigationProcess implements NavigationProcess { - private process: NavigationProcess; - private publisher: Publisher<NavigationProcessStatus>; + private readonly process: NavigationProcess; + private readonly publisher: Publisher<NavigationProcessStatus>; private currentStatus: NavigationProcessStatus;core/src/activity-utils/makeStackReducer.ts (1)
83-85: Namenoopis misleading now that it logs eventsIt no longer “does nothing.” Consider renaming for clarity.
-function noop(stack: Stack, event: DomainEvent) { +function logOnly(stack: Stack, event: DomainEvent) { return { ...stack, events: [...stack.events, event] }; }And update call sites:
- Pushed: withPauseReducer(withActivitiesReducer(noop, context)), + Pushed: withPauseReducer(withActivitiesReducer(logOnly, context)),extensions/plugin-history-sync/src/NavigationProcess/NavigationProcess.ts (1)
12-27: Consider using an enum instead of const object for better type safetyWhile the current implementation works, using a TypeScript enum would provide better type safety and intellisense support.
-export const NavigationProcessStatus = { - IDLE: "idle", - PROGRESS: "progress", - SUCCEEDED: "succeeded", - FAILED: "failed", -} as const; - -export type NavigationProcessStatus = - (typeof NavigationProcessStatus)[keyof typeof NavigationProcessStatus]; +export enum NavigationProcessStatus { + IDLE = "idle", + PROGRESS = "progress", + SUCCEEDED = "succeeded", + FAILED = "failed", +}extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.ts (2)
32-37: Add null check before accessing stack.globalTransitionStateThe condition checks
stack !== nullbut then accessesstack.globalTransitionStatein a combined condition. This could be clearer and safer.- if (isTerminated(this.status)) return []; - if (stack !== null && stack.globalTransitionState !== "idle") return []; + if (isTerminated(this.status)) return []; + if (stack?.globalTransitionState !== "idle") return [];
62-65: Inefficient use of splice and flatMapUsing
splice(0, 1)returns an array with a single element, then usingflatMapon it is unnecessary. Consider simplifying.- const nextNavigation = this.pendingNavigations.splice(0, 1); - const nextNavigationEvents = nextNavigation.flatMap((navigation) => - navigation(navigationTime), - ); + const nextNavigation = this.pendingNavigations.shift(); + if (!nextNavigation) return []; + const nextNavigationEvents = nextNavigation(navigationTime);extensions/plugin-history-sync/src/NavigationProcess/CompositNavigationProcess.ts (1)
51-68: getStatus creates derived process as side effectThe getStatus method modifies state by creating the derived process. This violates the principle that getter methods should be side-effect free.
Consider creating the derived process in captureNavigationOpportunity only, or rename the method to indicate it has side effects (e.g.,
ensureDerivedAndGetStatus).extensions/plugin-history-sync/src/historySyncPlugin.tsx (4)
443-447: Null-safe navigation operator but no null return handlingThe code uses
initialSetupProcess?.captureNavigationOpportunitybut doesn't handle the case where initialSetupProcess might be null and thus return undefined.initialSetupProcess ?.captureNavigationOpportunity(stack, Date.now()) - .forEach((event) => + ?.forEach((event) => event.name === "Pushed" ? push(event) : stepPush(event), );
658-664: Duplicate code in onChanged handlerThe onChanged handler has identical logic to onInit for handling navigation events. Consider extracting to a shared function.
+ const processNavigationEvents = ( + stack: any, + push: any, + stepPush: any + ) => { + initialSetupProcess + ?.captureNavigationOpportunity(stack, Date.now()) + ?.forEach((event) => + event.name === "Pushed" ? push(event) : stepPush(event), + ); + }; // In onInit: - initialSetupProcess - ?.captureNavigationOpportunity(stack, Date.now()) - .forEach((event) => - event.name === "Pushed" ? push(event) : stepPush(event), - ); + processNavigationEvents(stack, push, stepPush); // In onChanged: - initialSetupProcess - ?.captureNavigationOpportunity(getStack(), Date.now()) - .forEach((event) => - event.name === "Pushed" ? push(event) : stepPush(event), - ); + processNavigationEvents(getStack(), push, stepPush);
108-123: Publisher wrapping could cause subscription timing issuesThe subscribe function wraps the subscriber in an async function, but the original subscriber might not be async. This could affect error handling and timing.
const subscribeInitialSetupProcessStatus = ( subscriber: (status: NavigationProcessStatus) => void, ) => { - return initialSetupProcessPublisher.subscribe(async (status) => - subscriber(status), - ); + return initialSetupProcessPublisher.subscribe(async (status) => { + try { + await subscriber(status); + } catch (error) { + console.error('Initial setup process status subscriber error:', error); + } + }); };
277-283: Optional callback receives complex subscription mechanismThe onInitialSetupProcessCreated callback receives a complex subscription wrapper. Document this API clearly for consumers.
Consider simplifying the API by directly passing the publisher or providing a cleaner subscription interface.
📜 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 (17)
core/src/Stack.ts(1 hunks)core/src/activity-utils/makeStackReducer.ts(5 hunks)core/src/aggregate.ts(1 hunks)core/src/interfaces/StackflowActions.ts(1 hunks)demo/src/stackflow/stackflow.config.ts(1 hunks)extensions/plugin-history-sync/src/InitialSetupProcessStatusContext.ts(1 hunks)extensions/plugin-history-sync/src/Mutex.ts(1 hunks)extensions/plugin-history-sync/src/NavigationEvent.ts(1 hunks)extensions/plugin-history-sync/src/NavigationProcess/CompositNavigationProcess.ts(1 hunks)extensions/plugin-history-sync/src/NavigationProcess/NavigationProcess.ts(1 hunks)extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.ts(1 hunks)extensions/plugin-history-sync/src/NavigationProcess/StatusPublishingNavigationProcess.ts(1 hunks)extensions/plugin-history-sync/src/Publisher.ts(1 hunks)extensions/plugin-history-sync/src/historySyncPlugin.tsx(6 hunks)extensions/plugin-history-sync/src/index.ts(1 hunks)extensions/plugin-history-sync/src/useIsEntryActivity.ts(1 hunks)extensions/plugin-history-sync/src/withResolvers.ts(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). (1)
- GitHub Check: Workers Builds: stackflow-docs
🔇 Additional comments (12)
core/src/interfaces/StackflowActions.ts (1)
2-2: LGTM — import-only changeType-only import addition is safe and doesn’t affect the public API.
core/src/aggregate.ts (1)
30-31: LGTM: safe initialization of the new events logInitializing
eventsto[]ensures reducers can append without guards.extensions/plugin-history-sync/src/index.ts (2)
3-3: LGTM: exposing initial setup process status hookRe-export looks good and matches the new context/hook.
12-12: LGTM:useIsEntryActivityre-exportNo issues.
core/src/activity-utils/makeStackReducer.ts (3)
94-95: LGTM: event logging on InitializedCorrectly appends the event; consistent with new
eventslog.
114-115: LGTM: event logging on ActivityRegisteredConsistent with the logging strategy.
125-126: LGTM: event logging on PausedPause event is logged prior to transition; consistent.
extensions/plugin-history-sync/src/NavigationProcess/StatusPublishingNavigationProcess.ts (1)
1-1: Incorrect — bare import 'Publisher' is valid in this packageextensions/plugin-history-sync/tsconfig.json sets "baseUrl": "./src" and extensions/plugin-history-sync/src/Publisher.ts exists, so import type { Publisher } from "Publisher" resolves; switching to "../Publisher" is optional for explicitness/consistency.
Likely an incorrect or invalid review comment.
extensions/plugin-history-sync/src/Publisher.ts (2)
15-23: LGTM! Robust concurrent publishing implementationThe publish method correctly uses a mutex to serialize publish operations and handles all subscribers via Promise.allSettled, ensuring that a failed subscriber doesn't affect others. The snapshot of subscribers via slice() prevents concurrent modification issues.
7-13: Subscriber management OK — no leak in current usagePublisher is only instantiated as initialSetupProcessPublisher in extensions/plugin-history-sync/src/historySyncPlugin.tsx; subscribe() returns an unsubscribe that removes the exact function reference, and useSyncExternalStore (wrapStack) will unsubscribe on unmount. If onInitialSetupProcessCreated hands the subscribe function to external callers, callers must call the returned unsubscribe to avoid leaks (document if needed).
extensions/plugin-history-sync/src/NavigationProcess/NavigationProcess.ts (1)
3-10: LGTM! Well-designed navigation process interfaceThe interface is clean and follows good TypeScript practices with proper return types for navigation event handling.
extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
290-291: Incomplete actions destructuring in onInitThe function signature shows
getStack, dispatchEvent, push, stepPushbut the AI summary indicates these are the only actions now available. Verify this doesn't break existing functionality.
extensions/plugin-history-sync/src/NavigationProcess/CompositNavigationProcess.ts
Outdated
Show resolved
Hide resolved
extensions/plugin-history-sync/src/NavigationProcess/CompositNavigationProcess.ts
Outdated
Show resolved
Hide resolved
extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.ts
Outdated
Show resolved
Hide resolved
extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.ts
Show resolved
Hide resolved
Deploying stackflow-demo with
|
| Latest commit: |
6a24370
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://55bd3247.stackflow-demo.pages.dev |
| Branch Preview URL: | https://default-history-setup-transi.stackflow-demo.pages.dev |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/activity-utils/makeStackReducer.ts (1)
131-149: Resumed without queued events keeps the stack stuck inpaused.Line 131: when a
Resumedevent arrives whileglobalTransitionState === "paused"butpausedEventsis empty, we now append the event to the log yet still return the stack withglobalTransitionState: "paused". Any subsequent activity event will be buffered again instead of being reduced, so the stack never actually resumes unless an extraResumedis dispatched. Please clear the pause before returning in that branch.- if (stack.globalTransitionState !== "paused" || !stack.pausedEvents) { - return { ...stack, events: [...stack.events, event] }; - } + if (stack.globalTransitionState !== "paused") { + return { ...stack, events: [...stack.events, event] }; + } + + if (!stack.pausedEvents) { + return { + ...stack, + globalTransitionState: "idle", + events: [...stack.events, event], + }; + }
🧹 Nitpick comments (8)
extensions/plugin-history-sync/src/ActivityActivationMonitor/CountPublishingActivityActivationMonitor.ts (1)
19-31: Avoid floating promise and align naming with domain (“activation” vs “focus”).Publish returns a Promise; explicitly ignore it and use consistent variable names.
captureActivitiesNavigation(stack: Stack): void { - const previousFocusCount = + const previousActivationCount = this.activityActivationMonitor.getActivationCount(); this.activityActivationMonitor.captureActivitiesNavigation(stack); - const currentFocusCount = + const currentActivationCount = this.activityActivationMonitor.getActivationCount(); - if (currentFocusCount !== previousFocusCount) { - this.publisher.publish(currentFocusCount); + if (currentActivationCount !== previousActivationCount) { + void this.publisher.publish(currentActivationCount); } }extensions/plugin-history-sync/src/Publisher.ts (1)
4-4: Broaden subscriber type (sync or async), fix typo, and keep snapshot naming consistent.This makes the API friendlier while retaining behavior; also corrects
targetSubcribers→targetSubscribers.export class Publisher<T> { - private subscribers: ((value: T) => Promise<void>)[] = []; + private subscribers: ((value: T) => void | Promise<void>)[] = []; private publishLock: Mutex = new Mutex(); - subscribe(subscriber: (value: T) => Promise<void>): () => void { + subscribe(subscriber: (value: T) => void | Promise<void>): () => void { this.subscribers.push(subscriber); return () => { this.subscribers = this.subscribers.filter((s) => s !== subscriber); }; } publish(value: T): Promise<PromiseSettledResult<void>[]> { - const targetSubcribers = this.subscribers.slice(); + const targetSubscribers = this.subscribers.slice(); return this.publishLock.runExclusively(() => Promise.allSettled( - targetSubcribers.map((subscriber) => subscriber(value)), + targetSubscribers.map((subscriber) => + Promise.resolve(subscriber(value)), + ), ), ); } }Also applies to: 7-7, 15-22
extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.ts (2)
62-65: Simplify queue consumption with shift().Clearer than splice+flatMap for a single element.
- const nextNavigation = this.pendingNavigations.splice(0, 1); - const nextNavigationEvents = nextNavigation.flatMap((navigation) => - navigation(navigationTime), - ); + const nextNavigation = this.pendingNavigations.shift()!; + const nextNavigationEvents = nextNavigation(navigationTime);
85-93: Optional: use a Set for membership checks.Building a Set of allowed ids reduces O(n²) scans in
verifyNoUnknownNavigationEvents.extensions/plugin-history-sync/src/historySyncPlugin.tsx (4)
188-196: SSR safety: providegetServerSnapshotto useSyncExternalStore.Prevents hydration mismatches during SSR. Suggest passing a stable initial value (e.g., IDLE and [] respectively).
506-513: Order of operations: dispatch follow-up events before running monitors.Run monitors against the post-dispatch stack for immediate consistency (you also run them in onPushed/onReplaced/onPopped; this just tightens init).
- runActivityActivationMonitors(stack); - - followUpEvents?.forEach((event) => - event.name === "Pushed" ? push(event) : stepPush(event), - ); + followUpEvents?.forEach((event) => + event.name === "Pushed" ? push(event) : stepPush(event), + ); + runActivityActivationMonitors(getStack());
325-344: Do we also want to monitor the entry activity?Monitors are added for defaultHistory Pushed events but not for the final target entry Pushed. If
useIsEntryActivity()relies on counts, consider monitoring the entry as well.- (navigationTime) => [ - makeEvent("Pushed", { - activityId: id(), - activityName: targetActivityRoute.activityName, - activityParams: - makeTemplate( - targetActivityRoute, - options.urlPatternOptions, - ).parse(currentPath) ?? - urlSearchParamsToMap(pathToUrl(currentPath).searchParams), - eventDate: navigationTime, - activityContext: { - path: currentPath, - lazyActivityComponentRenderContext: { - shouldRenderImmediately: true, - }, - }, - }), - ], + (navigationTime) => { + const event = makeEvent("Pushed", { + activityId: id(), + activityName: targetActivityRoute.activityName, + activityParams: + makeTemplate( + targetActivityRoute, + options.urlPatternOptions, + ).parse(currentPath) ?? + urlSearchParamsToMap(pathToUrl(currentPath).searchParams), + eventDate: navigationTime, + activityContext: { + path: currentPath, + lazyActivityComponentRenderContext: { + shouldRenderImmediately: true, + }, + }, + }); + activityActivationMonitors.push( + new DefaultHistoryActivityActivationMonitor( + event.activityId, + initialSetupProcess!, + ), + ); + return [event]; + },
103-129: Alternative: leverage CountPublishingActivityActivationMonitor to push changes, drop manual scan/caching.Wrapping monitors with the publishing monitor allows you to subscribe directly, avoiding manual diffing and caching logic here.
Also applies to: 163-184
📜 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 (2)
.yarn/cache/react18-use-npm-0.4.1-3dd4e3b3bc-e8d61ca4ae.zipis excluded by!**/.yarn/**,!**/*.zipyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.changeset/big-dodos-grow.md(1 hunks).pnp.cjs(3 hunks)core/src/activity-utils/makeStackReducer.ts(6 hunks)core/src/aggregate.spec.ts(54 hunks)core/src/produceEffects.spec.ts(32 hunks)extensions/plugin-history-sync/package.json(1 hunks)extensions/plugin-history-sync/src/ActivityActivationCountsContext.ts(1 hunks)extensions/plugin-history-sync/src/ActivityActivationMonitor/ActivityActivationMonitor.ts(1 hunks)extensions/plugin-history-sync/src/ActivityActivationMonitor/CountPublishingActivityActivationMonitor.ts(1 hunks)extensions/plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.ts(1 hunks)extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.ts(1 hunks)extensions/plugin-history-sync/src/Publisher.ts(1 hunks)extensions/plugin-history-sync/src/historySyncPlugin.tsx(7 hunks)extensions/plugin-history-sync/src/index.ts(1 hunks)extensions/plugin-history-sync/src/useDelayTransitionRender.ts(1 hunks)extensions/plugin-history-sync/src/useIsRenderInTransition.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/big-dodos-grow.md
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/plugin-history-sync/src/index.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). (2)
- GitHub Check: Workers Builds: stackflow-docs
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
.pnp.cjs (2)
6381-6381: Dependency entry matches runtime usageThe added
react18-usemapping slots cleanly into the workspace dependency list alongside its peers. This aligns with the new hook usage and ensures PnP resolution stays consistent.
15317-15337: Virtual package wiring looks correctThe generated block properly records the physical cache location plus peer expectations (
[email protected],@types/[email protected]). This should satisfy the shim’s requirements without additional tweaks.
...plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (8)
extensions/plugin-history-sync/src/ActivityActivationMonitor/ActivityActivationMonitor.ts(1 hunks)extensions/plugin-history-sync/src/ActivityActivationMonitor/CountPublishingActivityActivationMonitor.ts(1 hunks)extensions/plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.ts(1 hunks)extensions/plugin-history-sync/src/NavigationEvent.ts(1 hunks)extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.ts(1 hunks)extensions/plugin-history-sync/src/Publisher.ts(1 hunks)extensions/plugin-history-sync/src/historySyncPlugin.tsx(5 hunks)extensions/plugin-history-sync/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- extensions/plugin-history-sync/src/ActivityActivationMonitor/ActivityActivationMonitor.ts
- extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.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:
extensions/plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.tsextensions/plugin-history-sync/src/Publisher.tsextensions/plugin-history-sync/src/index.tsextensions/plugin-history-sync/src/ActivityActivationMonitor/CountPublishingActivityActivationMonitor.tsextensions/plugin-history-sync/src/NavigationEvent.tsextensions/plugin-history-sync/src/historySyncPlugin.tsx
extensions/plugin-*/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Plugins must implement only the documented lifecycle hooks (onInit, onBeforePush/onPushed, onBeforePop/onPopped, onBeforeReplace/onReplaced, onBeforeStepPush/onStepPushed, onBeforeStepPop/onStepPopped, onBeforeStepReplace/onStepReplaced, onChanged)
Files:
extensions/plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.tsextensions/plugin-history-sync/src/Publisher.tsextensions/plugin-history-sync/src/index.tsextensions/plugin-history-sync/src/ActivityActivationMonitor/CountPublishingActivityActivationMonitor.tsextensions/plugin-history-sync/src/NavigationEvent.tsextensions/plugin-history-sync/src/historySyncPlugin.tsx
🧬 Code graph analysis (5)
extensions/plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.ts (2)
extensions/plugin-history-sync/src/ActivityActivationMonitor/ActivityActivationMonitor.ts (1)
ActivityActivationMonitor(3-7)extensions/plugin-history-sync/src/NavigationEvent.ts (2)
ActivityNavigationEvent(11-11)isActivityNavigationEvent(20-28)
extensions/plugin-history-sync/src/Publisher.ts (1)
extensions/plugin-history-sync/src/Mutex.ts (1)
Mutex(1-24)
extensions/plugin-history-sync/src/ActivityActivationMonitor/CountPublishingActivityActivationMonitor.ts (2)
extensions/plugin-history-sync/src/ActivityActivationMonitor/ActivityActivationMonitor.ts (1)
ActivityActivationMonitor(3-7)extensions/plugin-history-sync/src/Publisher.ts (1)
Publisher(3-24)
extensions/plugin-history-sync/src/NavigationEvent.ts (7)
core/src/event-types/PushedEvent.ts (1)
PushedEvent(3-14)core/src/event-types/PoppedEvent.ts (1)
PoppedEvent(3-9)core/src/event-types/ReplacedEvent.ts (1)
ReplacedEvent(3-14)core/src/event-types/StepPushedEvent.ts (1)
StepPushedEvent(3-13)core/src/event-types/StepPoppedEvent.ts (1)
StepPoppedEvent(3-8)core/src/event-types/StepReplacedEvent.ts (1)
StepReplacedEvent(3-13)core/src/event-types/index.ts (1)
DomainEvent(12-22)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (10)
extensions/plugin-history-sync/src/Publisher.ts (1)
Publisher(3-24)extensions/plugin-history-sync/src/NavigationProcess/NavigationProcess.ts (3)
NavigationProcessStatus(12-17)NavigationProcessStatus(19-20)NavigationProcess(3-10)extensions/plugin-history-sync/src/ActivityActivationMonitor/ActivityActivationMonitor.ts (1)
ActivityActivationMonitor(3-7)core/src/Stack.ts (1)
Stack(50-57)integrations/react/src/__internal__/shims/useSyncExternalStore.ts (1)
useSyncExternalStore(3-17)extensions/plugin-history-sync/src/InitialSetupProcessStatusContext.ts (1)
InitialSetupProcessStatusContext(4-5)extensions/plugin-history-sync/src/ActivityActivationCountsContext.ts (1)
ActivityActivationCountsContext(3-5)extensions/plugin-history-sync/src/NavigationProcess/StatusPublishingNavigationProcess.ts (1)
StatusPublishingNavigationProcess(8-50)extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.ts (1)
SerialNavigationProcess(9-101)extensions/plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.ts (1)
DefaultHistoryActivityActivationMonitor(12-61)
⏰ 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 (1)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
181-182: Passundefinedwhen publishing avoidpayload
Publisher<void>.publishstill requires its single argument; omitting it causes a compile-time error (“Expected 1 arguments, but got 0”). Provideundefinedexplicitly (and optionally discard the returned Promise) so the code type-checks.- if (changeOccurred) { - activityActivationCountsChangeNotifier.publish(); - } + if (changeOccurred) { + void activityActivationCountsChangeNotifier.publish(undefined); + }
...plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.ts
Show resolved
Hide resolved
ENvironmentSet
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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)
extensions/plugin-history-sync/src/historySyncPlugin.tsx(5 hunks)extensions/plugin-history-sync/src/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write source in TypeScript with strict typing enabled across the codebase
Files:
extensions/plugin-history-sync/src/index.tsextensions/plugin-history-sync/src/historySyncPlugin.tsx
extensions/plugin-*/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Plugins must implement only the documented lifecycle hooks (onInit, onBeforePush/onPushed, onBeforePop/onPopped, onBeforeReplace/onReplaced, onBeforeStepPush/onStepPushed, onBeforeStepPop/onStepPopped, onBeforeStepReplace/onStepReplaced, onChanged)
Files:
extensions/plugin-history-sync/src/index.tsextensions/plugin-history-sync/src/historySyncPlugin.tsx
🧬 Code graph analysis (1)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (10)
extensions/plugin-history-sync/src/Publisher.ts (1)
Publisher(3-24)extensions/plugin-history-sync/src/NavigationProcess/NavigationProcess.ts (3)
NavigationProcessStatus(12-17)NavigationProcessStatus(19-20)NavigationProcess(3-10)extensions/plugin-history-sync/src/ActivityActivationMonitor/ActivityActivationMonitor.ts (1)
ActivityActivationMonitor(3-7)extensions/plugin-history-sync/src/makeHistoryTaskQueue.ts (1)
makeHistoryTaskQueue(7-30)core/src/Stack.ts (1)
Stack(50-57)extensions/plugin-history-sync/src/InitialSetupProcessStatusContext.ts (1)
InitialSetupProcessStatusContext(4-5)extensions/plugin-history-sync/src/ActivityActivationCountsContext.ts (1)
ActivityActivationCountsContext(3-5)extensions/plugin-history-sync/src/NavigationProcess/StatusPublishingNavigationProcess.ts (1)
StatusPublishingNavigationProcess(8-50)extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.ts (1)
SerialNavigationProcess(9-101)extensions/plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.ts (1)
DefaultHistoryActivityActivationMonitor(12-61)
⏰ 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 (1)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
181-182: Passundefinedwhen publishing void notifications.
Publisher<void>.publishstill requires an explicit argument; calling it without one (publish()) triggers “Expected 1 arguments, but got 0” understrictTS settings. Please passundefined(and optionally discard the Promise) to satisfy the signature.- activityActivationCountsChangeNotifier.publish(); + void activityActivationCountsChangeNotifier.publish(undefined);Based on learnings.
There was a problem hiding this comment.
useInitialSetupProcessStatus()hook is added to provide users easy way of accessing status of the stack initialization navigation processuseIsRenderInTransition()hook is added to allow users to figure out whether current render is for a navigation transition process.
These hooks can be consolidated into a single useIsActivatedActivity() hook, which can handle both use cases requiring this hook.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
167-169: Passundefinedtopublish()to satisfy TypeScript.Even though
activityActivationCountsChangeNotifieris typedPublisher<void>, thepublish(value: T)method signature requires an argument. WhenTisvoid, you must passundefined.Apply this diff:
if (changeOccurred) { - activityActivationCountsChangeNotifier.publish(); + activityActivationCountsChangeNotifier.publish(undefined); }Based on learnings.
📜 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)
extensions/plugin-history-sync/src/historySyncPlugin.tsx(5 hunks)extensions/plugin-history-sync/src/index.ts(1 hunks)extensions/plugin-history-sync/src/useIsActivatedActivity.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/plugin-history-sync/src/index.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:
extensions/plugin-history-sync/src/useIsActivatedActivity.tsextensions/plugin-history-sync/src/historySyncPlugin.tsx
extensions/plugin-*/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Plugins must implement only the documented lifecycle hooks (onInit, onBeforePush/onPushed, onBeforePop/onPopped, onBeforeReplace/onReplaced, onBeforeStepPush/onStepPushed, onBeforeStepPop/onStepPopped, onBeforeStepReplace/onStepReplaced, onChanged)
Files:
extensions/plugin-history-sync/src/useIsActivatedActivity.tsextensions/plugin-history-sync/src/historySyncPlugin.tsx
🧬 Code graph analysis (2)
extensions/plugin-history-sync/src/useIsActivatedActivity.ts (2)
integrations/react/src/__internal__/activity/useActivity.ts (1)
useActivity(8-8)extensions/plugin-history-sync/src/ActivityActivationCountsContext.ts (1)
ActivityActivationCountsContext(3-5)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (10)
extensions/plugin-history-sync/src/NavigationProcess/NavigationProcess.ts (1)
NavigationProcess(3-9)extensions/plugin-history-sync/src/ActivityActivationMonitor/ActivityActivationMonitor.ts (1)
ActivityActivationMonitor(3-7)extensions/plugin-history-sync/src/Publisher.ts (1)
Publisher(3-24)core/src/Stack.ts (1)
Stack(50-57)integrations/react/src/__internal__/shims/useSyncExternalStore.ts (1)
useSyncExternalStore(3-17)extensions/plugin-history-sync/src/ActivityActivationCountsContext.ts (1)
ActivityActivationCountsContext(3-5)extensions/plugin-history-sync/src/NavigationProcess/SerialNavigationProcess.ts (1)
SerialNavigationProcess(9-105)core/src/event-types/PushedEvent.ts (1)
PushedEvent(3-14)core/src/event-types/StepPushedEvent.ts (1)
StepPushedEvent(3-13)extensions/plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.ts (1)
DefaultHistoryActivityActivationMonitor(12-61)
⏰ 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: Check the status of Changesets
- GitHub Check: Workers Builds: stackflow-docs
🔇 Additional comments (9)
extensions/plugin-history-sync/src/useIsActivatedActivity.ts (1)
5-13: LGTM with a clarification note.The hook correctly retrieves the current activity ID, finds its activation count from context, and returns
truewhen the count is undefined (not yet tracked) or greater than zero (activated). The logic is sound for the initial setup scenario.extensions/plugin-history-sync/src/historySyncPlugin.tsx (8)
128-148: Cache comparison logic is correct given the invariant.The developer confirmed that
activityActivationMonitorsentries are never removed, only added. Given this invariant, the current cache comparison correctly detects changes: it checks length mismatch and whether any existing cached item has a different count in the current array. Since IDs are stable and never removed, the one-directional comparison is sufficient.Note: If the invariant changes in the future (e.g., monitors can be removed), this logic will need updating to a bidirectional comparison.
260-338: LGTM:SerialNavigationProcessintegration is well-structured.The refactor correctly constructs a
SerialNavigationProcesswith navigation factories fordefaultHistoryand the target activity. Each factory returns an array of events (PushedandStepPushed), and monitors are registered for each pushed activity. The finalcaptureNavigationOpportunity(null)call retrieves the initial events with adjusted timestamps.The logic properly handles:
- Mapping
defaultHistoryentries to event factories- Creating
DefaultHistoryActivityActivationMonitorinstances for eachPushedevent- Calling
captureNavigationOpportunityto retrieve the first batch of events
493-499: LGTM: Initial setup process continuation inonInit.After setting up the history listener, the code correctly:
- Calls
captureNavigationOpportunity(stack)to retrieve the next batch of events- Dispatches each event via
pushorstepPushbased on event name- Runs activity activation monitors on the updated stack
This properly integrates the
SerialNavigationProcessinto the plugin lifecycle.
710-720: LGTM:onChangedcorrectly continues setup and monitors activation.The
onChangedhook:
- Retrieves the current stack
- Calls
captureNavigationOpportunity(stack)to dispatch any remaining setup events- Dispatches events via
pushorstepPush- Runs activity activation monitors
This ensures the initial setup process progresses on each state change and activation counts are kept up-to-date.
172-721: Plugin lifecycle hooks comply with documented API.The plugin implements only the documented lifecycle hooks:
wrapStackoverrideInitialEventsonInitonPushedonStepPushedonReplacedonStepReplacedonBeforePushonBeforeReplaceonBeforeStepPoponBeforePoponChangedAll hooks match the documented plugin API for
@stackflow/react.As per coding guidelines.
18-18: Fix the import path for ActivityActivationCountsContext.The import uses a bare module specifier without the relative path prefix, which will cause a module resolution error.
Apply this diff:
-import { ActivityActivationCountsContext } from "./ActivityActivationCountsContext"; +import { ActivityActivationCountsContext } from "./ActivityActivationCountsContext";Wait—reviewing line 18 again, I see it already uses
"./ActivityActivationCountsContext"with the correct relative prefix. The past review comment referenced a different import that has since been fixed. No action needed here.
175-178: VerifyuseSyncExternalStoreusage for SSR safety.The hook is called unconditionally in
wrapStack, which is correct. However, ensure thatgetActivityActivationCounts()does not throw or return inconsistent values between server and client renders, asuseSyncExternalStorerequires stable snapshots.Run the following script to check if
getActivityActivationCountscan be called safely during SSR (whenwindowis undefined):
340-500: VerifypushandstepPushare defined on theactionsobject.The
onInithook destructures{ getStack, dispatchEvent, push, stepPush }fromactions. Ensure that the@stackflow/reactplugin API exposes these methods on theactionsobject passed toonInit.Run the following script to verify the
actionsshape in the plugin API:
Changes
plugin-history-sync
NavigationProcessmodeling a navigation process is addeduseInitialSetupProcessStatus()hook is added to provide users easy way of accessing status of the stack initialization navigation processuseIsRenderInTransition()hook is added to allow users to figure out whether current render is for a navigation transition process.core
Stackexposes events used to construct itself by.eventsproperty