feat(plugin-history-sync): Add an option to skip default history setup transition #671
Conversation
🦋 Changeset detectedLatest commit: ec1c07d 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 CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a public route descriptor option to skip the default-history setup transition and updates history-sync initialization to support a normalized defaultHistory descriptor, conditional flattened vs sequenced event construction, unified event timing, and expanded restoration/navigation handling for root and step activities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant HistorySyncPlugin
participant HistoryStore
participant ActivityManager
Note over Client,HistorySyncPlugin: Initialization request with route + defaultHistory
Client->>HistorySyncPlugin: init(targetActivityRoute, defaultHistory)
HistorySyncPlugin->>HistorySyncPlugin: interpretDefaultHistoryOption(...)
alt descriptor.skipDefaultHistorySetupTransition == true
Note right of HistorySyncPlugin: Flatten descriptor.entries\nmap via historyEntryToEvents\nmark events skipEnterActiveState
HistorySyncPlugin->>HistoryStore: write SerialNavigationProcess (flattened events)
HistorySyncPlugin->>ActivityManager: push target activity (skipEnterActiveState)
else
Note right of HistorySyncPlugin: Build per-entry sequences\nmap entries -> events\nmark first Pushed to skip initial activation
HistorySyncPlugin->>HistoryStore: write sequenced navigation processes
HistorySyncPlugin->>ActivityManager: push target activity (normal push)
end
Note over HistoryStore,ActivityManager: onInit/onBeforePush/onBeforeReplace resolve paths and restore step/root navigation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 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 stackflow-demo with
|
| Latest commit: |
b7dd300
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b03e08ca.stackflow-demo.pages.dev |
| Branch Preview URL: | https://default-history-transition-o.stackflow-demo.pages.dev |
commit: |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
stackflow-docs | ec1c07d | Commit Preview URL | Jan 07 2026, 09:53 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: 1
🧹 Nitpick comments (2)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (2)
346-352: Consider simplifyingisFirstPushtracking.The
isFirstPushvariable is used to track whether we're processing the firstPushedevent, but the logic can be simplified since there's only onePushedevent perhistoryEntry(at the start of the array returned byhistoryEntryToEvents).The condition
index === 0 && isFirstPushcould be replaced with justindex === 0combined with checking if the event is the firstPushedevent in the entry (which it always is, givenhistoryEntryToEventsstructure).🔎 Proposed simplification
- ...defaultHistory.map((historyEntry, index) => () => { - let isFirstPush = true; - - return historyEntryToEvents(historyEntry).map((event) => { - if (event.name !== "Pushed") return event; - const skipEnterActiveState = index === 0 && isFirstPush; - - isFirstPush = false; + ...defaultHistory.map((historyEntry, index) => () => { + let pushedEventProcessed = false; + + return historyEntryToEvents(historyEntry).map((event) => { + if (event.name !== "Pushed") return event; + const skipEnterActiveState = index === 0 && !pushedEventProcessed; + + pushedEventProcessed = true;
413-425: Consider handlingStepReplacedsteps differently.Steps entered via
StepReplacedare currently pushed to history usingpushState. This could result in additional history entries for what were semantically replace operations. Consider usingreplaceStatefor steps wherestep.enteredBy.name === "StepReplaced".🔎 Proposed differentiation
for (const step of activity.steps) { if (!step.exitedBy && step.enteredBy.name !== "Pushed") { - pushState({ - history, - pathname: template.fill(step.params), - state: { - activity: activity, - step: step, - }, - useHash: options.useHash, - }); + const statePayload = { + history, + pathname: template.fill(step.params), + state: { + activity: activity, + step: step, + }, + useHash: options.useHash, + }; + if (step.enteredBy.name === "StepReplaced") { + replaceState(statePayload); + } else { + pushState(statePayload); + } } }
📜 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)
.changeset/shiny-ears-draw.mdextensions/plugin-history-sync/src/historySyncPlugin.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write source in TypeScript with strict typing enabled across the codebase
Files:
extensions/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/historySyncPlugin.tsx
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Include a Changeset entry for any user-facing package change
Files:
.changeset/shiny-ears-draw.md
🧬 Code graph analysis (1)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (6)
extensions/plugin-history-sync/src/RouteLike.ts (1)
HistoryEntry(15-24)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/makeTemplate.ts (3)
makeTemplate(48-110)urlSearchParamsToMap(9-17)pathToUrl(5-7)extensions/plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.ts (1)
DefaultHistoryActivityActivationMonitor(12-61)extensions/plugin-history-sync/src/historyState.ts (3)
parseState(39-45)replaceState(65-81)pushState(47-63)
⏰ 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 (6)
.changeset/shiny-ears-draw.md (1)
1-5: LGTM!The changeset entry correctly documents a minor version bump for the new
skipDefaultHistorySetupTransitionoption. As per coding guidelines, user-facing package changes include a changeset entry.extensions/plugin-history-sync/src/historySyncPlugin.tsx (5)
31-31: LGTM!The
HistoryEntrytype import is appropriately added to support the new helper function's type signature.
67-67: LGTM!The new optional
skipDefaultHistorySetupTransitionproperty is well-named and properly typed.
297-315: LGTM!The
createTargetActivityPushEventhelper cleanly encapsulates target activity event creation with appropriate fallback handling for parameter parsing.
370-377: LGTM!The event timing calculation correctly assigns ascending timestamps, ensuring proper event ordering with consistent relative offsets from a single captured timestamp.
284-295: Consider explicitly settingtargetActivityIdonStepPushedevents to improve code clarity.The
StepPushedevents generated foradditionalStepsdon't includetargetActivityId. While optional per the type definition and handled by fallback to the latest active activity, explicitly setting this field would make the association clearer and more robust, especially in scenarios involving multiple simultaneous activity initializations. ThePushedevent'sactivityIdshould be captured and reused for the correspondingStepPushedevents.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @extensions/plugin-history-sync/src/historySyncPlugin.tsx:
- Around line 391-393: The code uses a non-null assertion when assigning const
match = activityRoutes.find((r) => r.activityName === activity.name)! which will
throw if no route is found; change this to handle undefined by removing the `!`,
checking if match is falsy, and then either skip initialization for that
activity or throw a clearer error/log (e.g., processLogger.warn or throw new
Error with activity.name) so misconfigured activities don’t crash
initialization; update any subsequent use of match to be inside the guarded
branch.
📜 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/RouteLike.tsextensions/plugin-history-sync/src/historySyncPlugin.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:
extensions/plugin-history-sync/src/RouteLike.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/RouteLike.tsextensions/plugin-history-sync/src/historySyncPlugin.tsx
⏰ 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 (5)
extensions/plugin-history-sync/src/RouteLike.ts (1)
13-13: LGTM! Clean addition of the feature flag.The optional boolean property is properly typed and clearly named.
extensions/plugin-history-sync/src/historySyncPlugin.tsx (4)
31-31: LGTM! Import correctly adds HistoryEntry type.The import is properly used for typing the
historyEntryToEventshelper function.
340-340: Address or remove the TODO comment.The TODO states "Initial push must be marked as skipEnterActiveState: true", but lines 369-380 already handle marking the first push event with
skipEnterActiveState. This TODO may be outdated.Please verify whether:
- The TODO is still valid and needs addressing, or
- The TODO can be removed since lines 369-380 handle the requirement
362-380: LGTM! Event date calculation ensures proper chronological ordering.The logic correctly:
- Assigns timestamps in chronological order (earlier events get earlier timestamps)
- Identifies and marks the first push event with
skipEnterActiveState
257-292: Verify that each default history entry should compute its own path from itsactivityParamsrather than sharingcurrentPath.In
historyEntryToEvents(line 274), all synthetic history entries are assigned the samepath: currentPath, regardless of their individualactivityNameandactivityParams.The standard pattern in this codebase (lines 675, 698) is to compute paths dynamically: find the route matching the
activityName, then usetemplate.fill(activityParams)to generate the path. SincehistoryEntryToEventspre-sets the path, the lifecycle hooks' path computation logic (lines 665–680) is skipped for these entries.Each history entry represents a distinct activity with its own params—they should likely have distinct paths computed from their individual params using the appropriate route template, consistent with how normal activity pushes work.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @extensions/plugin-history-sync/src/historySyncPlugin.tsx:
- Around line 263-285: historyEntryToEvents incorrectly assigns path:
currentPath to all default history activities; instead derive each entry's own
path from its activityName and activityParams. Update historyEntryToEvents to
compute a per-entry path (e.g., call an existing helper like
buildPathForActivity(activityName, activityParams) or add a small helper
derivePathForActivity) and use that value for activityContext.path rather than
currentPath, ensuring each default history event reflects its own activity
route.
🧹 Nitpick comments (1)
extensions/plugin-history-sync/src/RouteLike.ts (1)
49-49: Typo in variable name:entiresOrDescriptor→entriesOrDescriptor.The variable is misspelled as "entires" instead of "entries".
✏️ Suggested fix
- const entiresOrDescriptor = option(params); + const entriesOrDescriptor = option(params); - if (Array.isArray(entiresOrDescriptor)) { - return { entries: entiresOrDescriptor }; + if (Array.isArray(entriesOrDescriptor)) { + return { entries: entriesOrDescriptor }; } - return entiresOrDescriptor; + return entriesOrDescriptor;
📜 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/RouteLike.tsextensions/plugin-history-sync/src/historySyncPlugin.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:
extensions/plugin-history-sync/src/historySyncPlugin.tsxextensions/plugin-history-sync/src/RouteLike.ts
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/historySyncPlugin.tsxextensions/plugin-history-sync/src/RouteLike.ts
🧬 Code graph analysis (1)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (9)
extensions/plugin-history-sync/src/RouteLike.ts (2)
interpretDefaultHistoryOption(39-56)HistoryEntry(22-31)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/index.ts (1)
makeTemplate(3-3)extensions/plugin-history-sync/src/makeTemplate.ts (1)
makeTemplate(48-110)extensions/plugin-history-sync/src/ActivityActivationMonitor/DefaultHistoryActivityActivationMonitor.ts (1)
DefaultHistoryActivityActivationMonitor(12-61)integrations/react/src/stable/stackflow.tsx (1)
getStack(287-292)core/src/makeCoreStore.ts (1)
getStack(88-90)extensions/plugin-history-sync/src/historyState.ts (3)
parseState(39-45)replaceState(65-81)pushState(47-63)
⏰ 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 (5)
extensions/plugin-history-sync/src/RouteLike.ts (1)
17-20: LGTM!The
DefaultHistoryDescriptortype andinterpretDefaultHistoryOptionfunction provide a clean abstraction for handling both array and descriptor forms of default history. The normalization logic correctly handles undefined, array, and descriptor cases.Also applies to: 39-56
extensions/plugin-history-sync/src/historySyncPlugin.tsx (4)
319-365: LGTM!The conditional logic correctly implements the
skipDefaultHistorySetupTransitionfeature:
- When
true, all events are batched into a single process step withskipEnterActiveState: true- When
false, each history entry creates a separate process step for sequential transitionsThe self-referencing
initialSetupProcess!is safe sincecaptureNavigationOpportunityis called after the assignment completes.
367-385: LGTM!The event date calculation correctly ensures chronological ordering by assigning earlier timestamps to earlier events. The first
Pushedevent is marked withskipEnterActiveState: trueto prevent the initial activity from animating in.
390-436: LGTM!The state initialization logic correctly handles the case when history state is absent:
- Root activities use
replaceStateto avoid adding to browser history- Non-root activities use
pushStateto enable back navigation- Steps are pushed only if not exited and not the initial step (where
enteredBy.name !== "Pushed")
31-35: LGTM!The imports correctly bring in the new
HistoryEntrytype andinterpretDefaultHistoryOptionfunction from the updatedRouteLikemodule.
skipDefaultHistorySetupTransitionis added to use in cases where sequential transition entrances are awkward