Wire workflow step lifecycle events in PaywallViewModel#3487
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vegaro
changed the title
vegaro
changed the title
In a multi-step workflow paywall, toggling dark/light mode while on a non-first step would silently navigate the user back to step 1. The configuration change calls `updateState()`, which re-runs the full workflow setup — including resetting `WorkflowNavigator` to the initial step — instead of just rebuilding the cached step states for the new color scheme. Introduces `rebuildWorkflowStepStates()`, called from the configuration-change path instead of `updateState()` when the active paywall is a workflow. It: - Clears the existing `workflowStepStateCache`. - Rebuilds the current step's `PaywallState.Loaded.Components` against the new color scheme. - Leaves `WorkflowNavigator`'s `currentStepId` and `backStack` untouched, so the user stays on the step they were on. - Re-kicks the off-thread pre-warm so visited and unvisited steps repopulate with the new colors. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes workflow paywall refresh behavior to rebuild step UI state in-place instead of reloading the workflow, which could impact navigation/state caching if edge cases exist. Covered by new unit tests that assert navigation position and network call count are preserved. > > **Overview** > Fixes a workflow-paywall bug where changing the Compose `ColorScheme` (e.g., dark/light toggle) could reset multi-step workflow navigation back to the initial step. > > When a workflow is active, `refreshStateIfColorsChanged` now rebuilds workflow step states via `rebuildWorkflowStepStates`/`buildWorkflowStates` (clearing and repopulating the step cache using the *current* step) instead of calling `updateState()` and re-fetching/resetting the `WorkflowNavigator`. Adds tests ensuring the current workflow step is preserved and that `awaitGetWorkflow` is not called again on color refresh. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 8472463. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Facundo Menzella <facumenzella@gmail.com> feat: track workflow step lifecycle events (step_started / step_completed) Implements the workflows_step_started and workflows_step_completed event pipeline, routing WorkflowEvent through EventsManager → BackendStoredEvent → BackendEvent.Workflows → /v1/events. - Add WorkflowEvent sealed class (StepStarted / StepCompleted) implementing FeatureEvent; @SerialName discriminators ensure stable serialisation - Add BackendEvent.Workflows wire model with nested Context and Properties - Register BackendStoredEvent.Workflows in both JsonProvider and EventsManager polymorphic modules - Add WorkflowEvent.toBackendStoredEvent() converter - Track step_started on successful initial workflow load (guarded so a failed load does not fire spurious events) - Track step_completed + step_started on forward and back navigation - Track step_completed (toStepId = null) on paywall close - Track step_completed and clear traceId when workflow state is cleared on error Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> feat: align workflow event properties with monetization event naming spec Remove trace_id (not in spec) and add workflow_type ("paywall"), step_type (from WorkflowStep.type), and screen_type (from WorkflowStep.screenType, defaults to emptyList) to both StepStarted and StepCompleted events and their BackendEvent wire shape. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> test: add non-empty screen_type coverage for workflow events Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> fix: remove workflow_type/step_type/screen_type from BackendEvent wire model Khepri derives these from its own DB at event time — no need for the SDK to send them. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… trackWorkflowStepStarted Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
vegaro
force-pushed
the
cesar/workflow-events-wiring
branch
from
ed4537f to
f447516
Compare
vegaro
force-pushed
the
cesar/workflow-events-definition
branch
from
be17d14 to
3f91e95
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f447516. Configure here.
| fromStepId = null, | ||
| entryReason = "start", | ||
| ) | ||
| } |
There was a problem hiding this comment.
Color change triggers spurious StepStarted tracking event
Medium Severity
rebuildWorkflowStepStates() delegates to buildWorkflowStates(), which unconditionally fires trackWorkflowStepStarted with entryReason = "start". This means every color scheme change (e.g., light/dark mode toggle) while a workflow is active emits a spurious StepStarted event. The function's own doc comment and PR description state it handles color changes "without re-triggering navigation events," but the shared buildWorkflowStates helper now contains tracking logic that both callers execute.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f447516. Configure here.
| currentWorkflowStep()?.let { currentStep -> | ||
| trackWorkflowStepCompleted(step = currentStep, toStepId = null) | ||
| } | ||
| } |
There was a problem hiding this comment.
Spurious StepCompleted for package step on initial load error
Low Severity
In buildWorkflowStates, when singleStepFallbackId points to a different step that builds successfully, _workflowState.value gets set to point at that package step. If the actual currentStep then fails to build, the error-path tracking in buildStateFromStep calls currentWorkflowStep() which returns the package step, firing a StepCompleted event for a step that was never "started" from the user's perspective — it was only built for cache warming.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f447516. Configure here.
1 participant
Stacked on #3486.
Wires
WorkflowEventtracking intoPaywallViewModelImpl:StepStartedon initial step display and on every forward/back navigationStepCompletedwhen navigating away from a step, on dismiss, and on error during step transitionrebuildWorkflowStepStates()to handle color scheme changes without re-triggering navigation eventsNote
Medium Risk
Touches workflow navigation/teardown logic in
PaywallViewModelImpland adds new analytics events, which could cause incorrect step state or duplicate/missing tracking if edge cases are missed.Overview
Adds workflow step lifecycle tracking to
PaywallViewModelImplby emittingWorkflowEvent.StepStartedon initial workflow render and on forward/back navigation, andWorkflowEvent.StepCompletedwhen leaving a step (including dismiss and workflow-to-error transitions).Introduces helpers to derive the current step and determine terminal steps (no step-trigger actions), and expands workflow tests to assert correct event ordering/fields and that no events fire on failed initial loads.
Reviewed by Cursor Bugbot for commit f447516. Bugbot is set up for automated code reviews on this repo. Configure here.