fix(web): pause behavior + label overflow + back-edge corridor#90
Conversation
Two animation regressions, both surfaced when reviewing the order-flow
demo:
1. Pause used to clear all active state (activeStep, activeEdgeIds,
activeFromIds, activeToIds, activeNodes, destroyedNodes) and the
timer chain restarted from advanceStep on resume — so the pause
visibly nuked the current step and play jumped to the NEXT step.
Refactor: track the current phase ('pixel-active' or 'gap') with
timestamps, cancel the timer on pause WITHOUT clearing active
state, and re-enter the same phase with the captured offset on
resume. Result: pause freezes the visible step, resume continues
it from where it left off.
2. The DataPixel rAF parsed React Flow's viewport `transform` style
with a regex that didn't cover every form RF emits. When the
regex missed, scale fell back to 1 — carrots stayed at base size
and drifted off the path on zoom. Replace with the official
`useViewport()` hook so size + path coords stay in lockstep with
the rest of the canvas. Bonus: the rAF now keeps ticking while
paused so zooming during pause re-applies the scale.
Two related fixes inside DataPixel:
- Initialize `startTimeRef` on every first tick (paused or not);
skipping init in the paused branch left it at 0, computing
`elapsed = timestamp` (huge), snapping the carrot to the edge
end — the manual click-to-fire bug.
- Manual click-to-fire pixels no longer receive the `paused` prop;
pause is for the autoplay loop only, a click should always play
out its single step regardless of global state.
The label `<span>` was previously laid out as a flex column child inside a node wrapper that auto-sized to the building's 108px width — so the span was pinned to 108px regardless of its content or its maxWidth. Long labels overflowed and overlapped neighboring nodes. Refactor: - Wrap the label in a 48px-tall, full-width slot so the progress bar below keeps the right vertical position - Position the label `<span>` absolutely inside the slot with `left: 50% / transform: translateX(-50%)`. Absolute positioning escapes the flex parent's cross-size collapse, so the label can render at its actual content width. - Cap with maxWidth (default 200px, exposed as the `--openhop-label-max-width` CSS variable for live tweaking from DevTools) and clamp to 2 lines + ellipsis for overflow. - pointer-events: none on the absolute label so it doesn't intercept clicks meant for the building underneath when an extended label visually overlaps a neighbor. Skill update: - Add a validation rule telling agents to keep node `label` ≤ 4 words (matches the 2-line × 2-words-per-line clamp). Belt-and-suspenders: the UI clamps long labels visually, the skill nudges agents to produce short ones in the first place.
…oads Adds four `elk.spacing` options that force ELK to keep edge corridors apart, both within a layer and between layers: - `elk.spacing.edgeEdge: 40` - `elk.spacing.edgeNode: 40` - `elk.layered.spacing.edgeEdgeBetweenLayers: 40` - `elk.layered.spacing.edgeNodeBetweenLayers: 40` Symptom this fixes: in the orion flow, the back-edge `results → api` (the persistence step) was routing vertically at x=774, parallel to `api → jenkins` at x=864 — visually two roads between the same pair of nodes, only one of which actually animated. With the bumped spacing, ELK pushes the back-edge under the layout (via y=720 and the far-left margin) so the api↔jenkins corridor reads as a single road.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds pause/resume support with phase-aware timing, switches DataPixel to use the live React Flow viewport for per-frame positioning, ties autoplay pixels to the global ChangesPause/Resume Animation Flow
UI Label and Layout Refinements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/web/src/hooks/useFlowAnimation.ts (1)
200-230: 💤 Low valueThe
enterPhasecallback with empty deps creates a stale closure overadvanceStep.
enterPhasehas[]deps but callsadvanceStep(line 226), which has[steps.length]deps. Ifstepschanges mid-playback,enterPhasewill invoke a staleadvanceStepreference. The eslint-disable acknowledges this, and it's acceptable if steps are immutable during playback, but worth documenting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/hooks/useFlowAnimation.ts` around lines 200 - 230, enterPhase currently has an empty dependency array and therefore closes over a possibly-stale advanceStep; either add advanceStep to enterPhase's dependency array (i.e., useCallback(..., [advanceStep])) and remove the eslint-disable, or store advanceStep in a stable ref (advanceStepRef.current = advanceStep) and call advanceStepRef.current() from inside enterPhase; update references to enterPhase, advanceStep and phaseRef accordingly so the callback always calls the current advanceStep implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/components/nodes/FlowNode.tsx`:
- Around line 242-247: The span that renders the clamped label in the FlowNode
component has title={label} but pointerEvents: 'none' prevents the browser
tooltip from appearing; move the title attribute off the inner span and onto the
outer clickable node container (the element wrapping the node in the FlowNode
render that handles clicks/hover) and remove title from the span so the tooltip
works while preserving pointerEvents: 'none' on the clamped label element.
In `@skills/openhop/SKILL.md`:
- Line 87: The Node.label guidance is inconsistent: keep the Later "Node.label"
schema text in SKILL.md aligned with the earlier rule by updating the freeform
sentence (the paragraph describing Node.label) to state the validation rule
“Node label must be ≤ 4 words” and clarify that labels should be short noun
phrases; update any sentence that currently says “freeform, anything
human-readable” to instead reference the validation constraint and give the same
examples/format as the earlier rule (e.g., “Order Service”, “Auth API”, not long
descriptive names) so the Node.label schema and the rule are consistent.
---
Nitpick comments:
In `@packages/web/src/hooks/useFlowAnimation.ts`:
- Around line 200-230: enterPhase currently has an empty dependency array and
therefore closes over a possibly-stale advanceStep; either add advanceStep to
enterPhase's dependency array (i.e., useCallback(..., [advanceStep])) and remove
the eslint-disable, or store advanceStep in a stable ref (advanceStepRef.current
= advanceStep) and call advanceStepRef.current() from inside enterPhase; update
references to enterPhase, advanceStep and phaseRef accordingly so the callback
always calls the current advanceStep implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: faed6dd8-b4b7-40f2-819f-7dd3abda4c41
📒 Files selected for processing (6)
packages/web/src/components/DataPixel.tsxpackages/web/src/components/FlowCanvas.tsxpackages/web/src/components/nodes/FlowNode.tsxpackages/web/src/hooks/useFlowAnimation.tspackages/web/src/lib/flow-layout.tsskills/openhop/SKILL.md
- FlowNode: move title attribute from the absolute-positioned label span (which has pointer-events: none, so the browser tooltip never fired) to the outer clickable node container. - SKILL.md: align the Schema Reference 'Node.label' line with the earlier ≤4-word validation rule. Previously the schema text said 'freeform, anything human-readable' which contradicted the rule. - useFlowAnimation: capture advanceStep in a ref (advanceStepRef) so enterPhase — whose deps stay [] to keep the timer chain stable — always invokes the current closure instead of a stale snapshot from when enterPhase was first created.
* 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.
Three independent fixes from a debug session over the order-flow demo. Each commit covers one concern; reviewable separately.
1. Pause freezes current step + carrots scale with zoom (
f408bbc)Pause regression — pause used to clear
activeStep/activeEdgeIds/ etc, so the visible step nuked itself; on resume the chain restarted fromadvanceStepand jumped to the next step.Refactor
useFlowAnimation:'pixel-active'or'gap') + a timestampstartTimeRefsoelapsedstays constant) so zoom changes still applyZoom drift — the rAF was regex-parsing
viewport.style.transform; some transform formats didn't match andscalefell back to 1 → carrot stayed at base size and drifted off the path on zoom. Replaced withuseViewport()from@xyflow/react.Subtle bugs cleaned up along the way:
startTimeRefinitialized on the first tick regardless of pause state — without this, a manual click-to-fire pixel that started while paused sawstartTimeRef = 0, computedelapsed = timestamp(huge), and the carrot snapped to the destination nodepaused— pause is for autoplay only2. Node labels render outside their parent flex bound (
b23c5fc)The label
<span>was a flex column child inside a wrapper that auto-sized to the 108px building, so the span was pinned to 108px regardless ofmaxWidth. Long labels overlapped neighbors.left: 50% / translateX(-50%)— escapes the flex parent's cross-size constraint, label sizes to its content--openhop-label-max-width(default 200px, live-tweakable from DevTools), clamp to 2 lines + ellipsis past thatpointer-events: noneon the absolute label so it doesn't intercept clicks meant for an adjacent node when it overlapsSkill update: agents are told to keep
label≤ 4 words (matches the 2-line × 2-words clamp).3. Back-edges no longer shadow forward roads (
d77837e)Adds four
elk.spacingoptions so ELK pulls edge corridors apart. Symptom: orion flow'sresults → apiback-edge was routing at x=774, parallel toapi → jenkinsat x=864 — looked like two roads between the same pair, only one actually animating. With the bumped spacing, the back-edge now routes under the layout via the far-left margin.Verification
npm run typecheck— cleannpm run lint— 0 errors, 18 warnings (no net change from master)npm test— 40/40 pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI/UX Improvements
Layout
Documentation