feat(lab): visual playbook builder - #54
Merged
Merged
Conversation
- React Flow canvas with custom node types (nav, detect, attempt, prep, cleanup, outcome) - LabeledEdge: single quadratic bezier per connector with a draggable label pill at t=0.5 that bends the curve (the label IS the bezier control point) - EdgeLabelRenderer for reliable HTML event handling on edge labels - PropertiesPanel with Monaco editor for per-node code editing - CodePreview with live TypeScript compilation from graph to playbook code - NodePalette with drag-and-drop node creation - Multi-playbook and multi-play management with localStorage persistence (v11) - Preloaded playbooks parsed from real PoC playbook source files
…ates
- Play.detect() now accepts an optional name as first arg
- ActFn third arg changed from PlayOutcome to PlayHistory { lastOutcome, steps }
- Named detects store their outcome in history.steps[name] for later reference
- ActOptions.skip fn receives full history instead of bare lastOutcome
|
Warning Review limit reached
More reviews will be available in 13 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR introduces a visual node-graph playbook builder in the lab and refactors the Play execution contract to thread a ChangesVisual Playbook Builder with Play History Threading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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 |
rickcedwhat
approved these changes
May 31, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (2)
scripts/parsePlaybooks.mjs (2)
464-464: ⚡ Quick winAdd parentheses to clarify isTimeout logic.
The boolean expression relies on operator precedence (
||binds looser than&&), making it harder to understand at a glance. Explicit parentheses improve readability.♻️ Proposed refactor for clarity
- isTimeout: c.name === 'timeout' || c.name === 'noAccess' && (!c.selector || c.selector === 'page.locator("")' || c.selector === 'p.locator("")'), + isTimeout: c.name === 'timeout' || (c.name === 'noAccess' && (!c.selector || c.selector === 'page.locator("")' || c.selector === 'p.locator("")')),🤖 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 `@scripts/parsePlaybooks.mjs` at line 464, The isTimeout boolean expression is ambiguous due to operator precedence; change the condition in the object where isTimeout is set so it explicitly groups the logic as (c.name === 'timeout' || (c.name === 'noAccess' && (…))) — i.e., wrap the c.name === 'noAccess' branch and its selector checks in parentheses so the intent is clear when evaluating c.name and c.selector in the isTimeout assignment (refer to the isTimeout property and variables c.name / c.selector to locate the code).
498-498: ⚡ Quick winAdd parentheses to clarify isTimeout logic.
The boolean expression relies on operator precedence, making it harder to understand at a glance. Explicit parentheses improve readability.
♻️ Proposed refactor for clarity
- isTimeout: o.type === 'timeout' || o.name === 'timeout', + isTimeout: (o.type === 'timeout' || o.name === 'timeout'),🤖 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 `@scripts/parsePlaybooks.mjs` at line 498, The isTimeout property expression is unclear due to implicit operator precedence; update the object property assignment for isTimeout (the line setting isTimeout using o.type and o.name) to wrap the OR comparison in parentheses so it reads like: compute (o.type === 'timeout' || o.name === 'timeout') and assign that result to isTimeout to make the intent explicit and improve readability.
🤖 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 `@lab/src/builder/BuilderCanvas.tsx`:
- Line 114: The starter snippet references the removed identifier lastOutcome
directly; update the snippet bodies used in BuilderCanvas (the act callback
starter strings) to use the new PlayHistory parameter (e.g., replace occurrences
of lastOutcome with history.lastOutcome and use optional chaining like
history.lastOutcome?.name) and ensure the example assumes the callback receives
a history parameter (for example: (history) => { if (history.lastOutcome?.name
=== 'tableState') { ... } }). Make the same replacement in all snippet strings
(both occurrences around the current snippet and the other one noted) so
generated playbooks compile against the new PlayHistory contract.
- Around line 44-47: The onConnect handler currently adds edges without any
data.label so LabeledEdge doesn't render its HTML pill; update the addEdge call
in the onConnect callback to initialize the new edge's data with a label (for
example data: { label: '' }) so LabeledEdge can render and the connector pill
(and drag-to-bend affordance) appears for user-created connections; modify the
setEdges(...) call where addEdge({ ...params, animated: true, style: {
strokeWidth: 2 } }, eds) is invoked to include data: { label: '' } (preserving
params, animated, and style).
In `@lab/src/builder/compiler.ts`:
- Around line 143-146: The generated code injects raw user strings into
identifiers and quoted literals (e.g., export const
${playbookName.toLowerCase()}Pb, new Playbook('${playbookName}', and object key
${playName}: () =>), which breaks for names like "My Playbook" or "O'Brien";
sanitize by (1) converting user names to safe JS identifiers for exported names
and object keys (e.g., normalize playbookName/playName to [A-Za-z0-9$_]-only
form, e.g., slugify or camelCase) and (2) escape any user-provided values
emitted inside quoted string literals using JSON.stringify-style escaping before
concatenation; apply the same fixes wherever user names are emitted (including
the range around lines 171–244).
- Around line 159-166: The builder is emitting a skip option for attempt nodes
(via the code building optionsStr using node.data.timeout and skipText) but the
runtime ignores it because Play.attempt(), AttemptActionOptions, ActRecord and
run() don’t support or evaluate a skip predicate; either stop emitting skip for
attempts or (preferred) add first-class support: extend AttemptActionOptions and
ActRecord to include a skip predicate field, update Play.attempt(...) signature
to accept & forward that skip, and modify the runtime run() logic that executes
attempt acts to evaluate the skip predicate before running the attempt so
branch-gated attempts are actually skipped when appropriate.
- Around line 95-127: The generated skip predicate currently injects
detectStepName and candidateName directly into template strings (see
incomingEdges loop, variables detectStepName, candidateName and skipConditions)
which breaks for names with spaces or apostrophes; update the code that builds
skipConditions to (1) use bracket access for steps (e.g. history.steps[<escaped
key>]) instead of dot access, and (2) properly escape or JSON.stringify
candidateName when embedding it into the predicate string so quotes and special
characters are safe; adjust both branches that push into skipConditions and the
final returned function expression so all generated predicates use these safe
bracket/escaped forms.
In `@lab/src/builder/NodePalette.tsx`:
- Around line 57-122: NodePalette currently only launches node creation via
HTML5 drag using onDragStart, which prevents keyboard and touch users from
adding steps; add a non-drag insert path by wiring a
clickable/keyboard-activatable control for each block that invokes the same
creation logic BuilderCanvas expects. Update NodePalette to render each block as
a focusable element (e.g., button or div with role="button", tabIndex and
onKeyDown handling for Enter/Space) and call the existing node-creation handler
used by BuilderCanvas (mirror the onDragStart payload or expose and call a prop
like onInsert/createNode with the same type strings
'nav','detect','attempt','prep','cleanup'). Ensure the click/keyboard handler
uses the same type identifiers and any required payload so BuilderCanvas can
create nodes without a drag event.
In `@lab/src/builder/PlaybookBuilder.tsx`:
- Around line 520-527: The input's onChange currently calls setPlayName and
setActivePlayName (and writes KEY_ACTIVE_PLAY) on every keystroke, which breaks
the autosave lookup that uses activePlayName; instead, stop updating the
active-play key while editing—only update playName (or an edit buffer like
editName) in onChange and defer calling setActivePlayName and
localStorage.setItem(KEY_ACTIVE_PLAY, ...) until the rename is committed (e.g.,
onBlur or Enter) or use the play's stable id as the lookup key in the autosave
effect; update the PlaybookBuilder logic that references
playName/setPlayName/setActivePlayName to use the edit buffer or id so autosave
continues to find the correct play during typing.
- Around line 408-423: handleUpdateNode updates the nodes array but doesn't
update the selectedNode state, causing the properties panel to hold stale data;
modify handleUpdateNode (the useCallback) to, after computing the new node
object for the matching id, also call setSelectedNode to replace the
selectedNode with the same merged object when selectedNode?.id === id (i.e.,
create updatedNode = { ...n, data: { ...n.data, ...updatedFields } }, use it in
the nodes map and if selectedNode?.id === id call setSelectedNode(updatedNode))
so the selectedNode reference and contents stay in sync with setNodes.
In `@lab/src/builder/PropertiesPanel.tsx`:
- Around line 465-473: The current input handler for timeout uses parseInt and
can persist NaN, negative, or truncated/scientific values; change the onChange
logic in the timeout <input> so you parse with Number (or +e.target.value),
treat empty string as undefined, and only call setTimeoutVal and
handleFieldChange when the parsed value is a finite integer and >= 0 (e.g.
Number.isInteger(parsed) && parsed >= 0); otherwise do not persist the invalid
value (optionally clear to undefined or keep the previous valid value) so
compilePlaybook() never receives NaN/negative/truncated values from timeout.
- Around line 502-545: The outer clickable accordion header currently nests the
"+ Add" <button>, which is invalid HTML; refactor so the accordion toggle and
the add action are sibling controls: replace the outer element that uses
setListCollapsed/listCollapsed (e.g., turn it into a div with role="button" and
onClick toggling setListCollapsed or keep it as the header button and move the
"+ Add" element out to be a sibling) and ensure the "+ Add" control calls
addCandidate or addOutcome (based on node.type) as before; keep the "+ Add"
handler using e.stopPropagation() to prevent toggle, and keep the
ChevronRight/ChevronDown rendering tied to listCollapsed.
In `@lab/src/Sidebar.tsx`:
- Around line 69-125: The nav buttons and toggle rely on title for accessibility
when collapsed; add explicit aria-label attributes to each button to provide
stable accessible names: for the dataset/settings/builder buttons (the buttons
that call onNavigate with view 'datasets'/'settings'/'builder' and use
linkStyle/currentView) add aria-labels like "Datasets", "Settings", "Playbook
Builder" and for the collapse toggle (the button that calls
setCollapsed(!collapsed) and uses collapsed to decide icon/title) add aria-label
that switches between "Expand sidebar" and "Collapse sidebar" based on
collapsed; ensure labels are present regardless of collapsed state so screen
readers get a consistent name.
In `@scripts/parsePlaybooks.mjs`:
- Line 196: Guard against calling Math.min with an empty array when computing
minIndent from lines: ensure lines.filter(l => l.trim()) yields a non-empty
array before calling Math.min (the current expression calculating const
minIndent = Math.min(...lines.filter(l => l.trim()).map(...)) can produce
Infinity). Modify the logic in the block that computes minIndent (and the
subsequent substring call) to either early-return an empty body when there are
no non-blank lines or set minIndent to 0 as a safe default; update the code that
uses minIndent (the substring call immediately after) to rely on this guarded
value so substring(Infinity) cannot occur.
- Line 4: Replace the hard-coded absolute path assigned to playbooksDir with a
portable solution: read an optional CLI argument (e.g., process.argv[2]) for the
playbooks directory and fall back to a relative path resolved from the script
location (use path.resolve with import.meta.url / __dirname equivalent) so the
script works both locally and in CI; update any uses of playbooksDir accordingly
and validate the resolved path exists, throwing a clear error if not.
- Line 159: The current check using str[i - 1] !== '\\' incorrectly treats
quotes following escaped backslashes as escaped; replace that single-char check
with logic that counts consecutive backslashes immediately preceding index i
(walk j = i-1 backward while str[j] === '\\' and increment a counter) and treat
the quote as escaped only when that count is odd—i.e., if the count is even then
close the string. Update the condition around char === stringChar to use this
backslash-count parity test (referencing variables str, i, char, stringChar).
- Line 544: Replace the hard-coded absolute output path stored in outputPath
with a portable approach: accept an optional CLI argument (e.g.,
process.argv[2]) and fallback to a relative path resolved from the repository
root (use path.resolve(process.cwd(), 'src/builder/preloadedPlaybooks.json'));
update the code that defines outputPath in scripts/parsePlaybooks.mjs to use
that CLI fallback logic and ensure you import/require path if not already
present so the script works across developer machines and CI.
- Around line 245-301: parseOutcomes currently treats Outcomes.timeout as always
{name: 'timeout', type: 'timeout'}; change it to parse the timeout overloads
like success/failure: in parseOutcomes, for timeoutMatch call
splitArgs(timeoutMatch[1]) and then (1) determine name: if the first arg is a
string literal use args[0].slice(1,-1), otherwise default to 'timeout'; (2)
determine locator: if a locator arg exists (either args[1] when name provided,
or args[0] when name omitted) set selector via cleanSelectorLambda(locatorArg)
else set selector to ''; (3) determine outcome type by inspecting the opts arg
(typically the last arg) for isSuccess: true (e.g. using a simple regex
/isSuccess\s*:\s*true/ against that arg) and set type to 'success' when true
otherwise 'timeout'; update the timeout branch in parseOutcomes to push an
object { name, type, selector } accordingly.
- Around line 386-408: The wiring logic reads nodeData.skipCode into skipText
but parseArgs never sets skipCode, so the !==/=== branches never trigger; fix by
updating parseArgs(...) to extract the skip: predicate from the step input and
assign it to data.skipCode (so nodeData.skipCode is populated), or alternatively
add a guard in the detect-to-next-step wiring (where skipText is read and
prevCandidates are evaluated) to skip the !==/=== parsing when skipText is
falsy; reference the parseArgs function (where data is constructed) and the
skipText/nodeData.skipCode usage in the edge wiring that inspects prevCandidates
to ensure either population or a null-check is added.
---
Nitpick comments:
In `@scripts/parsePlaybooks.mjs`:
- Line 464: The isTimeout boolean expression is ambiguous due to operator
precedence; change the condition in the object where isTimeout is set so it
explicitly groups the logic as (c.name === 'timeout' || (c.name === 'noAccess'
&& (…))) — i.e., wrap the c.name === 'noAccess' branch and its selector checks
in parentheses so the intent is clear when evaluating c.name and c.selector in
the isTimeout assignment (refer to the isTimeout property and variables c.name /
c.selector to locate the code).
- Line 498: The isTimeout property expression is unclear due to implicit
operator precedence; update the object property assignment for isTimeout (the
line setting isTimeout using o.type and o.name) to wrap the OR comparison in
parentheses so it reads like: compute (o.type === 'timeout' || o.name ===
'timeout') and assign that result to isTimeout to make the intent explicit and
improve readability.
🪄 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: ec47a3f0-2e68-4da5-8130-1f2da78ccc60
⛔ Files ignored due to path filters (1)
lab/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
lab/package.jsonlab/src/App.tsxlab/src/Sidebar.tsxlab/src/builder/BuilderCanvas.tsxlab/src/builder/CodePreview.tsxlab/src/builder/LabeledEdge.tsxlab/src/builder/NodePalette.tsxlab/src/builder/PlaybookBuilder.tsxlab/src/builder/PropertiesPanel.tsxlab/src/builder/compiler.tslab/src/builder/customNodes.tsxlab/src/builder/dtsDefinitions.tslab/src/builder/preloadedPlaybooks.jsonlab/src/index.cssscripts/parsePlaybooks.mjssrc/play.test.tssrc/play.ts
- compiler: safe JS identifiers, JSON.stringify string literals, bracket access in skip predicates - compiler: suppress skip option for attempt nodes (runtime unsupported) - BuilderCanvas: init edge data.label on connect for LabeledEdge pill - NodePalette: add click/keyboard insert path alongside drag - PlaybookBuilder: sync selectedNode in handleUpdateNode, defer activePlayName to onBlur - PropertiesPanel: guard timeout against NaN/negative, fix nested button in accordion - Sidebar: add aria-label to nav and collapse toggle buttons - parsePlaybooks: portable paths via import.meta.url, fix backslash parity escape - parsePlaybooks: guard Math.min empty array, null-check skipText, explicit isTimeout parens Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- compiler + PropertiesPanel: use PlayHistory `history` param in act
callbacks instead of destructured `{ lastOutcome, steps }`
- BuilderCanvas + PlaybookBuilder: update starter snippets to use
history.lastOutcome?.name / history.lastOutcome?.isSuccess
- parsePlaybooks: parse Outcomes.timeout() name/locator/isSuccess
overloads instead of always emitting { name:'timeout', type:'timeout' }
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Store cleanup fn in a ref; useEffect fires it if the edge is removed while a drag is in progress, preventing stale window listeners. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace legacy `outcome?.name` / `outcome?.isSuccess` third-arg usage with `history.lastOutcome?.name` / `history.lastOutcome?.isSuccess` to match the ActFn signature introduced in 78b6be5. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
2 participants
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.
Summary
Adds a visual drag-and-drop playbook builder to the
labdocs site.What's in this PR
🎨 Visual Builder (
lab/src/builder/)nav,detect,attempt,prep,cleanup,outcomeLabeledEdge— a custom edge type that draws one quadratic bezier per connector with a draggable label pill at the curve's midpoint (t=0.5). The label IS the bezier control point — dragging it bends the curve without splitting it into two edgesEdgeLabelRendererused for the label overlay so HTML events (drag) work reliably without fighting React Flow's SVGpointer-eventsrulesPropertiesPanel— Monaco editor panel for editing node-level codeCodePreview— live TypeScript compilation from the graph to runnable playbook codeNodePalette— drag-and-drop sidebar for adding new nodeslocalStorage(keyv11)🔧 Parser (
scripts/parsePlaybooks.mjs)data.label,data.cpOffsetX,data.cpOffsetY— no separatelabelNodenodes⚙️
PlayAPI (src/play.ts)Play.detect()now accepts an optional name as first argumentActFnthird arg upgraded fromPlayOutcome | undefined→PlayHistory { lastOutcome, steps }history.steps[name]for use in later skip predicatesTesting
src/play.test.ts)labbuilds cleanly viapnpm run buildSummary by CodeRabbit
New Features
Enhancements