Skip to content

fix(web): preserve node progress on Play resume + drill-back#96

Merged
naorsabag merged 1 commit into
masterfrom
fix/preserve-progress-on-resume-and-drillback
May 9, 2026
Merged

fix(web): preserve node progress on Play resume + drill-back#96
naorsabag merged 1 commit into
masterfrom
fix/preserve-progress-on-resume-and-drillback

Conversation

@naorsabag
Copy link
Copy Markdown
Owner

@naorsabag naorsabag commented May 9, 2026

Two related bugs, same root: cumulative state (per-node progress + create/destroy) wasn't re-derived from the step index on key transitions.

What's fixed

1. Click-then-Play kept stale progress

Clicking a node fires its next outgoing step manually (good). But that increment lives in nodeProgressRef, which autoplay also uses. So:

  • Pause mid-step. Click node X → X's progress bar advances by 1.
  • Press Play → autoplay resumes from where it was. X's bar now permanently ahead of autoplay's expectation; gets pushed further every time autoplay fires X.

2. Drill-down → Back wiped parent's progress

<FlowCanvasInner> is keyed on the node-id list, so drilling into a sub-flow remounts the inner component on return — fresh nodeProgressRef = new Map(). Even though resumeFromStep was correctly seeded into stepIndexRef, the per-node bars all snapped to zero.

Implementation

  • Extract the inline cumulative-replay logic from goToStep into a shared applyEffectsThroughStep(idx) helper.
  • New mount-time useEffect: if stepIndexRef.current >= 0 on first render (i.e., startFromStep was set), replay 0..stepIndexRef and push into state. Fixes Compose roads from a single line #2.
  • Play-resume branch in the [playing] effect now also replays before re-entering the phase. Fixes fix: tighten road endpoints against node art #1.
  • goToStep refactored to use the same helper — no behavior change.

Verification

  • npm run typecheck — clean
  • npm test — 40/40
  • npm run format:check — clean

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Fixed animation playback to properly replay state when resuming mid-phase.
    • Improved animation consistency when starting from custom positions.
    • Manual scrubbing now applies the same cumulative effects as autoplay mode.

Review Change Stack

Two related bugs that share the same root: cumulative state (per-
node progress, create/destroy) wasn't re-derived from the step
index on key transitions.

1. Click a node mid-pause → that node's outgoing-step counter
   increments (handleNodeClick → setNodeStep). Press Play → autoplay
   resumes from where it was, but the node's progress bar stays
   ahead of where autoplay would actually be.

2. Drill into a sub-flow → click Back → parent flow's progress bars
   reset to zero. FlowCanvasInner's key includes node ids, so the
   parent component remounts fresh, dropping nodeProgressRef even
   though stepIndexRef was correctly seeded from resumeFromStep.

Both fixed by extracting the inline cumulative-replay logic from
goToStep into `applyEffectsThroughStep(idx)` and calling it from
two new sites:

- Mount-time effect: when stepIndexRef.current >= 0 on first render
  (i.e., startFromStep was set), replay 0..stepIndexRef and push
  into state so the parent flow's progress survives drill-back.
- Play-resume branch in the [playing] effect: replay 0..stepIndexRef
  before re-entering the phase, so any manual node clicks during
  the pause get snapped back to autoplay-expected state.

goToStep also refactored to use the same helper (no behavior change).
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 9, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 47eb6f23-9eb7-4f76-ab51-9c190f520355

📥 Commits

Reviewing files that changed from the base of the PR and between dc7abd7 and bc2c90d.

📒 Files selected for processing (1)
  • packages/web/src/hooks/useFlowAnimation.ts

Walkthrough

The PR refactors useFlowAnimation to centralize cumulative step effect replay through a new applyEffectsThroughStep helper function. This helper is now invoked during play-resume mid-phase, mount initialization with a non-zero startFromStep, and manual step navigation, ensuring consistent node progress and visibility state reconstruction across all entry points.

Changes

Cumulative Step Effects Replay

Layer / File(s) Summary
Cumulative Effects Helper
packages/web/src/hooks/useFlowAnimation.ts
Introduces applyEffectsThroughStep(idx) to reconstruct nodeProgress, activeNodes, and destroyedNodes for steps 0..idx. Includes mount-only effect that replays when startFromStep > 0 so initial state matches autoplay expectations.
Play-Resume Integration
packages/web/src/hooks/useFlowAnimation.ts
On playing resume with an active phase, cumulative effects through current stepIndexRef are replayed before re-entering React state, ensuring node/edge visibility is consistent with resumed playback.
goToStep Refactoring
packages/web/src/hooks/useFlowAnimation.ts
goToStep(idx) now calls shared applyEffectsThroughStep(idx) instead of duplicating cumulative create/destroy and per-node progress logic. Dependency list updated to include applyEffectsThroughStep.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • naorsabag/openhop#93: Both PRs modify the same hook with cumulative step effect replay and goToStep refactoring.
  • naorsabag/openhop#90: Both PRs modify useFlowAnimation to change phase-aware timing and cumulative step effect application.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR objectives address fixing node progress preservation on Play resume and drill-back, but the linked issues (#1, #2) concern road rendering composition and node handle positioning, which are unrelated coding requirements. Verify that the linked issues are correct. If issues #1 and #2 are unrelated, remove them and link to issues that define the node progress fix requirements instead.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(web): preserve node progress on Play resume + drill-back' clearly and specifically describes the main fix: preventing loss of node progress when resuming playback and returning from nested flows.
Out of Scope Changes check ✅ Passed Changes are focused on useFlowAnimation.ts and specifically address node progress replay logic, mount-time state restoration, and Play-resume reconciliation—all directly related to the PR's stated objectives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/preserve-progress-on-resume-and-drillback

Comment @coderabbitai help to get the list of available commands and usage tips.

@naorsabag naorsabag merged commit 28bfc8e into master May 9, 2026
7 checks passed
naorsabag added a commit that referenced this pull request May 9, 2026
* chore: bump @openhop/web 0.1.0-beta.1 → 0.1.0-beta.2 and openhop CLI 0.1.0-beta.3 → 0.1.0-beta.4

Web bumps for the canvas/UI work since beta.1:
- pause behavior fix + label overflow + back-edge corridor (#90)
- carrot click opens inspect with multi-target highlight + on-canvas
  playback controls (#93)
- bookmark-style toggle tabs for sidebar + inspector (#95)
- preserve node progress on Play resume + drill-back (#96)
- shrink sprite SVGs by 46% (3.7MB → 2.0MB) (#97)

CLI bumps to ship the new web bundle (its only change is the pinned
@openhop/web dependency moving to 0.1.0-beta.2). @openhop/server
stays at 0.1.0-beta.1 — no changes since last publish.

* fix(cli): bump hardcoded --version to 0.1.0-beta.4

Test contract.test.ts asserts `openhop --version` matches the
package.json version. The bump in 9b373aa missed the duplicate
hardcoded string in src/index.ts:53. Sync them up so CI passes.
@naorsabag naorsabag deleted the fix/preserve-progress-on-resume-and-drillback branch May 10, 2026 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compose roads from a single line

1 participant