feat(plugin-history-sync): Add an option to skip default history setup transition #671
Conversation
🦋 Changeset detectedLatest commit: a210e28 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 |
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughA changeset entry documents a minor version bump for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying stackflow-demo with
|
| Latest commit: |
a210e28
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://18f9ff4e.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 | a210e28 | Commit Preview URL | Dec 26 2025, 01:29 PM |
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.
| const match = activityRoutes.find( | ||
| (r) => r.activityName === activity.name, | ||
| )!; |
There was a problem hiding this comment.
Add defensive handling for missing route match.
The non-null assertion on activityRoutes.find() could throw if an activity's name doesn't match any configured route. While this is unlikely in normal operation, it could cause initialization failures for edge cases or misconfigured activities.
🔎 Proposed defensive handling
- const match = activityRoutes.find(
- (r) => r.activityName === activity.name,
- )!;
+ const match = activityRoutes.find(
+ (r) => r.activityName === activity.name,
+ );
+ if (!match) continue;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const match = activityRoutes.find( | |
| (r) => r.activityName === activity.name, | |
| )!; | |
| const match = activityRoutes.find( | |
| (r) => r.activityName === activity.name, | |
| ); | |
| if (!match) continue; |
🤖 Prompt for AI Agents
In extensions/plugin-history-sync/src/historySyncPlugin.tsx around lines 388 to
390, the code uses a non-null assertion on activityRoutes.find(...) which will
throw if no route matches an activity name; replace the assertion with a safe
lookup: check the result of find, if it's undefined log a warning (including
activity.name for debugging) and skip or provide a sensible fallback handler so
initialization doesn't crash; ensure subsequent code branches handle the absent
route case gracefully.
skipDefaultHistorySetupTransitionis added to use in cases where sequential transition entrances are awkward