feat(web): on-canvas playback controls + carrot-click inspect with multi-target highlight#93
Merged
Merged
Conversation
Three coupled UX changes that simplify the carrot-click flow.
1. Drop the on-canvas data label that floated next to each carrot.
It overlapped roads, doubled the visual noise vs. the inspect
panel, and went stale fast at any zoom. Aria label remains for
screen readers; canvas ergonomics rely on the inspect panel
instead.
2. Drop the click-to-pin DataPopup overlay (and its file). Pinned
popups duplicated the panel's content over the canvas and forced
a separate close gesture. Removed:
- DataPopup component file
- pinnedEdge / setPinnedEdge state in FlowCanvas
- handlePinPopup callback
- edgeStepsById lookup map (only used by the popup)
- onPaneClick that cleared pinnedEdge
3. Carrot click now opens the inspect panel on that step AND
highlights the specific FlowData entry the carrot was carrying.
Multi-data steps emit one carrot per data entry; without the
highlight you can't tell which one was clicked. Implementation:
- DataPixel.onPixelClick signature → (step, focusData?)
- FlowCanvas wires it to onInspectStep
- App.tsx + AppFragment.tsx track inspectedFocusData and pass it
down; both reset when the flow changes
- DataInspectionPanel accepts focusData; the matching DataBlock
gets a brand-cyan left bar + soft tint and scrolls into view
via scrollIntoView({block:'nearest', behavior:'smooth'})
- handleInspectStep also opens the inspector (was previously a
no-op when closed) so a click reliably surfaces the data.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors the data inspection workflow, replacing a pinned-popup overlay with an inline highlighting and scrolling mechanism. App and AppFragment track focus data state and pass it to the inspection panel, which uses it to highlight and auto-scroll to a specific data block within the inspected step. The old DataPopup component is removed, and DataPixel and FlowCanvas are updated to emit and relay focus data instead of coordinates. ChangesData Inspection Focus Refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant Pixel as DataPixel
participant Canvas as FlowCanvas
participant App
participant Panel as DataInspectionPanel
User->>Pixel: click visual pixel
Pixel->>Pixel: derive focusData from dataOverride/step.data
Pixel->>Canvas: onPixelClick(step, focusData)
Canvas->>App: onInspectStep(step, focusData)
App->>Panel: render DataInspectionPanel with focusData
Panel->>StepBody: highlight matching DataBlock
StepBody->>DataBlock: scrollIntoView() highlighted block
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
![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
🤖 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/DataPixel.tsx`:
- Around line 181-201: The interactive carrot is currently a non-focusable <div>
with an onClick handler (see the pixelRef element and its onClick that calls
onPixelClick), which prevents keyboard users from triggering the inspect flow;
make the element keyboard-accessible by adding tabIndex={0} and a semantic role
(e.g., role="button"), and wire a keyboard handler (onKeyDown or onKeyUp) that
invokes the exact same click logic when Enter or Space is pressed (reusing the
focusData calculation and onPixelClick call) and keep existing mouse handlers
(onClick, onMouseEnter/onMouseLeave) and aria-label intact so behavior is
identical across input methods.
🪄 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: 0bc332c7-7d96-472c-b003-e4a3bce5504e
📒 Files selected for processing (6)
packages/web/src/App.tsxpackages/web/src/AppFragment.tsxpackages/web/src/components/DataInspectionPanel.tsxpackages/web/src/components/DataPixel.tsxpackages/web/src/components/DataPopup.tsxpackages/web/src/components/FlowCanvas.tsx
💤 Files with no reviewable changes (1)
- packages/web/src/components/DataPopup.tsx
Two follow-ups for the carrot-click → inspect-panel flow: 1. Pause autoplay when the click provides focusData. The user could click a carrot during autoplay and the focus highlight would land for ~1 frame before handleStepChange's setInspectedFocusData(null) nuked it on the next step advance. Result: clicks felt like they did nothing. Now a click both surfaces the data AND freezes the canvas so the user can read it. 2. Crank up the highlighted DataBlock's visual weight: 4px cyan bar (was 3px), 0.18 background opacity (was 0.08), and a soft cyan glow via box-shadow. Was too subtle to read against the panel's dark theme; users couldn't tell what changed.
… Next)
Move Play/Pause off the header and into a vertical panel on the
canvas's top-right edge, alongside three new controls:
- ⏮ Restart — clears step pointer + node progress + active/destroyed
node sets, snaps back to step -1, sets playing = false
- ⏪ Prev — snap visualization to step (current - 1)
- ▶/⏸ Play — toggle (lifted from the header)
- ⏩ Next — snap to step (current + 1)
Implementation:
- useFlowAnimation gains `restart()` and `goToStep(idx)`. Both clear
the timer chain, the phase ref, and the pause-offset ref before
mutating state, so a scrub during autoplay won't get clobbered by
a pending tick. `goToStep` is a snapshot — it doesn't replay
cumulative create/destroy effects from earlier steps; pressing Play
resumes correctly from there.
- FlowCanvas mounts a <Panel position="top-right"> with a small
PlaybackButton helper. Disabled state for Prev/Next at the
boundaries.
- Header Play button removed in App.tsx and AppFragment.tsx; both
pass `onTogglePlay={() => setPlaying(p => \!p)}` to FlowCanvas.
…lative effects
Four issues from manual test of example-order-flow:
1. Play button stayed in Pause state after Restart/Prev/Next.
2. Pressing Prev/Next while paused briefly resumed the current step
before snapping to the new one.
Both same root cause: `goToStep`/`restart` only flipped the hook's
internal `playing` to false; the parent App still had `playing =
true`, so DataPixel received `paused={\!playing}` = false and kept
animating. Fix: thread `onPause: () => setPlaying(false)` from
App + AppFragment → FlowCanvas, and wrap each panel button's
onClick to call onPause first. Now parent state matches the
scrubbed visualization, button label flips correctly, and pixel
freezes at the new step.
3. Drill-down step intermittently failed after using Prev/Next.
4. Create/destroy steps weren't undone when scrubbing — destroyed
nodes stayed gone, created nodes stayed visible.
Both same root cause: `goToStep` was a snapshot — it only updated
activeStep/activeEdges, leaving nodeProgress/activeNodes/
destroyedNodes from the previous run-through. Drill-down detection
reads node state, so stale state broke it. Fix: `goToStep` now
replays steps 0..idx CUMULATIVELY — resets all three refs, then
loops applying create/destroy/progress effects so the state
matches what autoplay would produce if you'd watched up to that
step.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/components/FlowCanvas.tsx (1)
619-622: 💤 Low valueComment contradicts the implementation.
The comment states "without replaying cumulative create/destroy effects" but
goToStepinuseFlowAnimation.tsexplicitly replays all effects from step 0 to the target index:for (let i = 0; i <= idx; i++) applyStepEffects(i)This could mislead future maintainers. The cumulative replay is actually the correct behavior as it ensures node visibility state matches what autoplay would have produced.
📝 Suggested fix
- {/* Playback controls — right side, vertical column. Restart rewinds - to step -1 with all progress cleared; Prev/Next snap the - visualization to the adjacent step (without replaying cumulative - create/destroy effects — press Play to advance from there). */} + {/* Playback controls — right side, vertical column. Restart rewinds + to step -1 with all progress cleared; Prev/Next snap the + visualization to the adjacent step, replaying cumulative + create/destroy effects so node visibility stays consistent. */}🤖 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/components/FlowCanvas.tsx` around lines 619 - 622, The comment in FlowCanvas.tsx is incorrect about goToStep's behavior; update the comment to state that goToStep (in useFlowAnimation.ts) replays cumulative create/destroy effects by iterating applyStepEffects from 0 to idx so the node/edge visibility matches what autoplay would produce, rather than snapping without replaying effects—mention goToStep and applyStepEffects by name so future maintainers can find the 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.
Nitpick comments:
In `@packages/web/src/components/FlowCanvas.tsx`:
- Around line 619-622: The comment in FlowCanvas.tsx is incorrect about
goToStep's behavior; update the comment to state that goToStep (in
useFlowAnimation.ts) replays cumulative create/destroy effects by iterating
applyStepEffects from 0 to idx so the node/edge visibility matches what autoplay
would produce, rather than snapping without replaying effects—mention goToStep
and applyStepEffects by name so future maintainers can find the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 02fe92a5-056d-4c75-8668-e227e0957bfc
📒 Files selected for processing (4)
packages/web/src/App.tsxpackages/web/src/AppFragment.tsxpackages/web/src/components/FlowCanvas.tsxpackages/web/src/hooks/useFlowAnimation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/AppFragment.tsx
After auto-firing the drill-down callback for step N, FlowCanvas remembered N in lastDrilldownStepRef so the same step wouldn't fire twice. But this memo lived across user-driven scrubbing too — Prev back over a drill-down step then Next forward would land on N again, the effect would see the memo == currentStepIndex, and silently skip the drill on the second pass. Reset the memo to -1 inside each panel button's onClick (Restart, Prev, Next), so a re-traversal of a drill-down step always re-fires.
When Prev/Next/Restart called goToStep, phaseRef was reset to null and pauseOffsetMs to 0. The next Play hit the useEffect's fresh-start branch, which fires advanceStep() — and advanceStep increments stepIndexRef before applying state, so the step the user just scrubbed to was skipped (step N+1 → advanceStep → N+2). Two changes: - goToStep now sets phaseRef = 'pixel-active' (we're showing the step's pixel-active visualization, paused at progress 0). - The Play useEffect's resume guard drops the `offset > 0` check. We re-enter the captured phase whenever phaseRef is set — offset = 0 just means "start of phase", which is the right thing for both pause-mid-step (offset captured) and post-scrub plays.
The drill-down useEffect didn't gate on `playing`. Two failure modes: 1. Scrubbing past a drill-down step queued a 1500ms timer for that step. The next Next-click changed the activeStep, the effect re-ran, but the new step's branch returned early WITHOUT canceling the pending timer — so the drill fired anyway, ~1.5s later, in the middle of scrubbing. 2. After that accidental drill, lastDrilldownStepRef was set to the drill-down index. When the user later did Prev back to before the drill and pressed Play, autoplay reached the drill step but the memo matched and the effect skipped scheduling. Fix: - Effect now early-returns when \!playing, and explicitly clearTimeout on the pending drill-down timer in that branch (so pause + scrub kills any in-flight queued drill). - `playing` added to the effect deps so flipping play/pause re-runs the gate. Verified end-to-end: scrub past drill → Play → cycle end → Prev back → Play → drill-down fires when the step is reached.
Two related carrot-click bugs:
1. Broadcast steps (e.g. `from: order-service, to: [db, cache], data: {…}`)
share one data object reference across all targets. Clicking the
carrot to `db` would highlight BOTH the db and cache sections
because `d === focusData` matched both — same object, both targets.
2. Parallel steps (`parallel: [{from: api, to: order-service, data: …},
{from: authz, to: order-service, data: …}]`) inspected the clicked
sub-step rather than the parent. Result: clicking one carrot made
the sibling sub-flow disappear from the panel entirely instead of
leaving it visible with the clicked one highlighted.
Both fixed by replacing the `focusData: FlowData` argument with a
`focus: { from?, to?, data? }` triplet:
- DataPixel.onPixelClick simplified to (focusData?: FlowData) — the
pixel only knows its own data slice; FlowCanvas layers in the
edge identity (fromId, toId of edgeFlow) so the panel can match
the click to a specific (target, data) pair.
- FlowCanvas now passes `animState.activeStep` (the parent) as the
inspect step + the click's edge from/to. Manual click-to-fire
pixels store `inspectStep / fromId / toId` on the ManualPixel so
they preserve the parent context after the autoplay state moves.
- DataInspectionPanel highlights a DataBlock only if the section's
(from, to) matches focus AND the data ref matches focus.data.
Verified end-to-end:
- Multi-data on a single edge (charge request / order context) —
click highlights exactly one
- Broadcast (persist order → db|cache) — click highlights only the
clicked target's section
- Parallel (api / authz → order-service) — both sub-flows remain
visible, only the clicked one highlighted
naorsabag
changed the title
Adds role="button", tabIndex=0, and onKeyDown(Enter/Space) so keyboard users can fire the inspect-panel flow that the new mouse-only onClick introduced. Extracts the click logic into `emitPixelClick` so mouse + keyboard share one path. Per CodeRabbit review on PR #93.
CI on f24e09a hit two failures: 1. Prettier formatcheck — DataInspectionPanel.tsx and FlowCanvas.tsx had local style drift from the multi-prop callback edits in cb11823. Ran `prettier --write` on both. 2. npm audit (prod tree) — fast-uri <=3.1.1 had a pair of newly disclosed CVEs (path traversal via percent-encoded dot segments, host confusion via percent-encoded authority delimiters). Pulled in transitively from fastify@5.8.5 / @fastify/swagger / ajv. `npm audit fix` bumped to 3.1.2 cleanly — package-lock-only change, no direct deps touched.
This was referenced May 9, 2026
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.
This was referenced May 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A connected set of UX changes around the canvas: replaces the old click-to-pin popup with a richer inspect-panel highlight, adds Restart/Prev/Next controls next to Play, and fixes a handful of regressions surfaced while testing the order-flow demo. 8 commits, each scoped to one concern.
What's in
562516397b49d0e88ad176d0ada8playingstate on Restart/Prev/Next +goToStepreplays cumulative effects (create/destroy/progress)013792b4e5cec5pauseOffsetMs > 0)bee1034playing+ cancels pending timer on pause; stops scrub from queuing accidental drillscb11823(from, to, data)triplet — fixes broadcast (shared data ref) lighting up all targets, and parallel sub-step inspect making siblings disappearManual test of
example-order-flowcoversorder-service → db|cache) → click each target's carrot, only that target's section highlightsapi / authz → order-service) → click one carrot, both sub-flows still render, only the clicked one highlightsgoToStepnow applies cumulativecreate:/destroy:effects so a destroyed node reappears when scrubbing back across its destroy stepVerification
npm run typecheck— cleannpm test— 40/40npm run lint— 0 errors🤖 Generated with Claude Code