feat(web): inline YAML editor — Add/Edit modal + Delete (closes #74, partial #12)#76
Merged
Conversation
…partial #12) Lets users author and edit flows directly in the browser. The same authoring path as the CLI (`openhop push` / `openhop remove`), now inline in the sidebar — no terminal required. UI surface: - Sidebar: "+ New flow" primary button at the top; per-flow ✎ Edit and ✕ Delete affordances revealed on row hover. - FlowEditorModal: CodeMirror 6 YAML editor with live validation via parseFlowYaml() from @openhop/shared (sub-100ms feedback). Errors render with their path + suggestion fields, matching the CLI's validate --json output. Cmd/Ctrl-Enter saves; Esc cancels; click-outside cancels. Wiring: - New file `useFlowMutations.ts` — POST + DELETE wrappers around /api/flows. Distinguishes validation / server / network errors so the modal can render server-side details (e.g. node-ref lookups that local validation can't catch). - New file `FlowEditorModal.tsx` — modal component, mode-agnostic. - `useFlowList` gains a `reload()` so create/delete refresh the sidebar without a remount. - `App.tsx` owns the modal state machine ({closed, new, edit}) and the delete confirmation. Edit pre-populates by GETting the stored flow and round-tripping through yaml.stringify(). Scope decisions per #74: - Edit is a full re-push (POST), creating a new flow ID. Patch-ops in-place editing is the CLI's `openhop patch` flow and is out of scope (track separately if needed). - Delete uses native window.confirm — simpler than a styled dialog, upgradeable later. - Static-deploy "fragment mode" for #75 deferred — the modal takes a minimal prop surface so it can grow that mode without a rewrite. New deps (web only): @uiw/react-codemirror, @codemirror/lang-yaml, yaml. Web is bundled at build time, so these don't ship to npm package consumers — only to the static web build. Verified: - shared 93/93, server 19/19, cli 83/83, web 27/27 (5 new) = 222/222 - lint/typecheck/format:check/build all clean across the monorepo - both audit gates green (no new prod-tree CVEs) - live API smoke against `openhop serve`: POST valid → 201 with {id,version,title}; POST invalid → 400 with {details:[{path,message}]}; GET /api/flows/:id → {id,meta,flow} (round-trip through YAML.stringify re-parses cleanly); DELETE → 204; DELETE non-existent → 404 (treated as success in the hook). Manual browser smoke is owed before merge — kicking off `openhop demo` or `openhop serve` and exercising New / Edit / Delete in a real browser. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughAdds a YAML-backed flow editor and CRUD wiring: a modal CodeMirror YAML editor with live parse/validation, create/delete mutation hook, sidebar create/edit/delete UI, flow-list reload API, Vitest tests, and dev dependencies for YAML/CodeMirror. ChangesFlow CRUD and Editor Integration
Sequence DiagramsequenceDiagram
actor User
participant Sidebar
participant App
participant FlowEditorModal
participant Mutations as useFlowMutations
participant Server
User->>Sidebar: Click "New flow" / "Edit" / "Delete"
Sidebar->>App: onCreateAt / onEditFlow / onDeleteFlow
App->>FlowEditorModal: open(initialYaml)
FlowEditorModal->>FlowEditorModal: parseFlowYaml (live)
User->>FlowEditorModal: Edit YAML
FlowEditorModal->>FlowEditorModal: update validation state
User->>FlowEditorModal: Save
FlowEditorModal->>App: onSave(yamlText)
App->>Mutations: createFlow(yamlText)
Mutations->>Server: POST /api/flows (text/yaml)
alt Success
Server-->>Mutations: 200 { id, meta, flow }
Mutations-->>App: result
App->>App: selectFlow(id), close modal
App->>Sidebar: reload()
else Validation Error (400)
Server-->>Mutations: 400 { details }
Mutations-->>App: validation error
App->>FlowEditorModal: display serverError (details)
else Server/Network Error
Server-->>Mutations: non-OK / network fail
Mutations-->>App: server/network error
App->>FlowEditorModal: display serverError
end
User->>Sidebar: Click Delete
Sidebar->>App: onDeleteFlow(id)
App->>Mutations: deleteFlow(id)
Mutations->>Server: DELETE /api/flows/{id}
Server-->>Mutations: 200 or 404
Mutations-->>App: success
App->>Sidebar: reload()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 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)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/hooks/useFlowPolling.ts (1)
24-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
reload()fetches against stale responses.Now that
reload()can trigger overlapping/api/flowsrequests, an older response can arrive last and overwrite the freshly reloaded list right after create/delete.Suggested fix
useEffect(() => { - fetch(`${API_BASE}/api/flows`) + const controller = new AbortController() + setLoading(true) + fetch(`${API_BASE}/api/flows`, { signal: controller.signal }) .then((r) => r.json()) .then((data) => { setFlows(data) setLoading(false) }) - .catch(() => setLoading(false)) + .catch((err) => { + if (err instanceof DOMException && err.name === 'AbortError') return + setLoading(false) + }) + return () => controller.abort() }, [tick])🤖 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/useFlowPolling.ts` around lines 24 - 32, The fetch in useEffect (in useFlowPolling.ts) can produce stale responses when reload() triggers overlapping /api/flows requests; update the effect to ignore or cancel out-of-order responses by either (a) using an AbortController per request and aborting the previous controller before starting a new fetch, or (b) tracking an incrementing request id in a ref (e.g., latestFetchIdRef) and capturing the id when starting the fetch, then only calling setFlows / setLoading if the response id matches the latest id; apply this change inside the useEffect that currently calls fetch(`${API_BASE}/api/flows`) so setFlows and setLoading only run for the most recent request.
🤖 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/__tests__/flow-mutations.test.ts`:
- Around line 47-65: The test for parseFlowYaml does not assert the suggestion
field even though the test title promises it; update the test block (the it(...)
that defines const result = parseFlowYaml(bad) and const err = result.errors[0])
to also assert the suggestion (e.g., check err.suggestion is truthy or contains
an expected hint) so the contract of parseFlowYaml (path, message, suggestion)
is validated; locate the test in flow-mutations.test.ts around the it('reports
path + message + suggestion for an unknown step ref') and add an assertion
referencing err.suggestion.
In `@packages/web/src/App.tsx`:
- Around line 80-87: handleDeleteFlow currently reads mutations.error after
awaiting mutations.deleteFlow(flowId), which can be stale; change deleteFlow to
return its result/error (e.g., return { ok: boolean, error?: Error } or throw
the Error) and update handleDeleteFlow to use the returned value (or catch the
thrown error) to populate the alert message instead of reading mutations.error.
Locate the deleteFlow implementation (the mutations object) and make it return
the error payload (or throw), then in handleDeleteFlow use the returned
error.message (or the caught error.message) when ok is false to show the real
error instead of mutations.error?.message.
In `@packages/web/src/components/FlowEditorModal.tsx`:
- Around line 98-116: FlowEditorModal currently sets role="dialog" and
aria-modal but does not manage focus — implement focus management in the
FlowEditorModal component: when the modal opens, programmatically move focus
into the modal (e.g., focus the first focusable element or a container with
tabIndex={-1}), trap focus inside the modal (handle Tab/Shift+Tab or use a
focus-trap library) so keyboard users cannot tab into the underlying page, and
restore focus to the previously focused element when onCancel is called/when the
modal closes; target the modal container (data-testid="flow-editor-modal") and
integrate with the existing onCancel flow.
- Around line 75-89: Guard keyboard and close handlers to ignore actions while a
save is in progress: check the component's saving state (e.g., saving boolean)
at the top of the onKey handler and in any cancel/close/backdrop handlers so
that when saving is true you do not call onSaveRef.current, onCancelRef.current,
or trigger modal close; additionally, prevent Ctrl/Cmd+Enter from invoking
onSaveRef.current by requiring !saving before parsing with
parseFlowYaml(textRef.current) and calling onSaveRef.current(textRef.current).
Apply the same guard to the other referenced handlers around lines 103-105,
137-139, and 200-215 so all pathways (Esc, backdrop, Cancel, Close, and keyboard
save) are no-ops while saving.
In `@packages/web/src/components/Sidebar.tsx`:
- Around line 157-165: The title on the edit button in Sidebar.tsx incorrectly
claims an unimplemented "E" keyboard shortcut; remove the "(E)" hint from the
title attribute on the button (the element that sets title="Edit (E)" and uses
data-testid={`sidebar-edit-${flowId}`} and onClick calling onEditFlow) so it
simply reads "Edit", or alternatively implement a key handler in the Sidebar
component that listens for the "e" key and invokes onEditFlow(flowId) if you
want to keep the hint—do one or the other, not both.
---
Outside diff comments:
In `@packages/web/src/hooks/useFlowPolling.ts`:
- Around line 24-32: The fetch in useEffect (in useFlowPolling.ts) can produce
stale responses when reload() triggers overlapping /api/flows requests; update
the effect to ignore or cancel out-of-order responses by either (a) using an
AbortController per request and aborting the previous controller before starting
a new fetch, or (b) tracking an incrementing request id in a ref (e.g.,
latestFetchIdRef) and capturing the id when starting the fetch, then only
calling setFlows / setLoading if the response id matches the latest id; apply
this change inside the useEffect that currently calls
fetch(`${API_BASE}/api/flows`) so setFlows and setLoading only run for the most
recent request.
🪄 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: b01a6003-9c40-4c88-87d9-80f48783acfe
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (7)
packages/web/__tests__/flow-mutations.test.tspackages/web/package.jsonpackages/web/src/App.tsxpackages/web/src/components/FlowEditorModal.tsxpackages/web/src/components/Sidebar.tsxpackages/web/src/hooks/useFlowMutations.tspackages/web/src/hooks/useFlowPolling.ts
…utton UX change requested post-#76 review: instead of a single "+ New flow" button at the top of the sidebar, every folder gets its own "+" affordance that opens a small menu. The menu's two options: 📄 New flow → opens the editor with `meta.path` pre-set to the folder path, so the new flow lands in the right tree position on save. 📁 New folder → window.prompt() asks for a folder name, splices it onto the parent path, and opens the editor with meta.path: <parent>/<name>. Since folders are derived from each flow's meta.path, "New folder" is "New flow with this path prefix" — the user can edit the title and body before saving. Root-level "+" lives next to the search input (not as a big button). Per-folder "+" is hover-revealed alongside the existing edit/delete icons on flow rows. Menu closes on outside click or Esc. New `buildStarterYaml(path?)` helper in FlowEditorModal injects the path into the canonical starter YAML; locked in by 3 new tests. Verified: shared 93/93, server 19/19, cli 83/83, web 30/30 (8 in flow-mutations, 3 new) = 225/225. Lint/typecheck/format:check/build clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cross-contamination bug from 62fd4b0: a folder row's <li.group> wraps its child flow rows' <li.group>, so hovering one flow triggered group-hover on EVERY descendant — every flow in that folder revealed its edit/delete icons at once. Same nesting also masked the folder's "+" button on hover. Fix: name the groups so each row's hover is scoped to itself. - Folder row: <li class="group/folder"> + the "+" uses group-hover/folder:opacity-100. - Flow row: <li class="group/flow"> + edit/delete icons use group-hover/flow:opacity-100. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/web/src/App.tsx (1)
105-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t read
mutations.errorafterawait deleteFlow(); use a returned error payload (or throw).This read can be stale due to closure timing, so alerts may show
unknown errorinstead of the real failure.Suggested direction
- const ok = await mutations.deleteFlow(flowId) - if (!ok) { - window.alert(`Failed to delete flow: ${mutations.error?.message ?? 'unknown error'}`) + const result = await mutations.deleteFlow(flowId) + if (!result.ok) { + window.alert(`Failed to delete flow: ${result.error?.message ?? 'unknown error'}`) return }And adjust
useFlowMutations.deleteFlowto return{ ok: boolean; error?: MutationError }.🤖 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/App.tsx` around lines 105 - 107, The caller reads mutations.error after awaiting mutations.deleteFlow(flowId), which can be stale; update the deleteFlow implementation in useFlowMutations to return a structured result (e.g. { ok: boolean; error?: MutationError }) or throw, update its type signature accordingly, and modify the caller in App.tsx to use the returned payload (const { ok, error } = await mutations.deleteFlow(flowId)) and show error.message from that returned error instead of accessing mutations.error; ensure any places using deleteFlow are updated to the new return shape or handle thrown errors.packages/web/src/components/FlowEditorModal.tsx (2)
113-131:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd focus management (initial focus, trap, restore) for the dialog.
role="dialog"+aria-modalalone is insufficient; keyboard focus can still escape to background UI.🤖 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/FlowEditorModal.tsx` around lines 113 - 131, FlowEditorModal's modal wrapper (the div with role="dialog" and data-testid="flow-editor-modal") currently lacks focus management; add an initial focus, focus trap, and restore behavior: in the FlowEditorModal component create a ref for the dialog container, save document.activeElement when opening, on mount set focus to the dialog or its first focusable child, add keyboard handling to close on Escape by calling onCancel and implement a Tab/Shift+Tab focus-trap (or integrate a well-tested library such as focus-trap-react or react-focus-lock) to prevent focus escaping to background, and on unmount restore the previously focused element; ensure these behaviors are tied to the existing onCancel flow so closing via backdrop click or Escape restores focus.
90-105:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFreeze modal actions while
savingto prevent duplicate creates and “cancelled” in-flight saves.
Esc, backdrop click, close/cancel buttons, and Ctrl/Cmd+Enter still act duringsaving, which can produce duplicate POSTs or close the dialog mid-save.Suggested fix
const onSaveRef = useRef(onSave) const onCancelRef = useRef(onCancel) const textRef = useRef(text) + const savingRef = useRef(saving) useEffect(() => { onSaveRef.current = onSave onCancelRef.current = onCancel textRef.current = text + savingRef.current = saving }) useEffect(() => { if (!open) return const onKey = (e: KeyboardEvent) => { - if (e.key === 'Escape') { + if (savingRef.current) return + if (e.key === 'Escape') { e.preventDefault() onCancelRef.current() } else if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') { e.preventDefault() if (parseFlowYaml(textRef.current).success) { onSaveRef.current(textRef.current) } } } @@ - onClick={(e) => { - if (e.target === e.currentTarget) onCancel() - }} + onClick={(e) => { + if (!saving && e.target === e.currentTarget) onCancel() + }} @@ - onClick={onCancel} + onClick={() => !saving && onCancel()} + disabled={saving} @@ - onClick={onCancel} + onClick={() => !saving && onCancel()} + disabled={saving}Also applies to: 118-120, 150-153, 216-217
🤖 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/FlowEditorModal.tsx` around lines 90 - 105, The keyboard handler and all modal action handlers must short-circuit when a save is in progress: update the onKey function in the useEffect to check a saving flag (e.g., if (saving) return) before handling Escape or Ctrl/Cmd+Enter, and likewise guard the backdrop-click handler and the close/cancel button handlers so they return immediately when saving; only call onCancelRef.current() or onSaveRef.current(textRef.current) (after parseFlowYaml succeeds) if saving is false. Ensure the same saving check is applied to the handlers referenced elsewhere (the close/cancel click handlers and any backdrop click handler) so no duplicate POSTs or modal closes happen while saving.packages/web/src/components/Sidebar.tsx (1)
290-298:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the unimplemented
Eshortcut hint from the Edit action.The UI currently promises a keyboard shortcut that isn’t wired, which is misleading.
Suggested fix
- title="Edit (E)" + title="Edit"🤖 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/Sidebar.tsx` around lines 290 - 298, The Edit button currently shows a fake keyboard hint "(E)" in its title; remove the unimplemented shortcut hint by updating the title prop on the button in Sidebar.tsx (the element with aria-label={`Edit ${node.name}`} and data-testid={`sidebar-edit-${flowId}`}) so it no longer contains " (E)"; leave the onClick handler (onEditFlow) and aria-label as-is, or alternatively implement a real keyboard handler if you intend to keep the hint.
🤖 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/FlowEditorModal.tsx`:
- Line 37: The regex literal /^meta:\n title: New flow\n/ contains literal
double spaces which triggers the no-regex-spaces lint rule; update that regex
(located where FlowEditorModal.tsx defines the pattern) to use a quantifier
instead of two literal spaces, e.g. replace the " " with "\s{2}" (resulting in
/^meta:\n\s{2}title: New flow\n/) or "\x20{2}" if you want to match only space
characters.
- Line 33: Move the non-component helper buildStarterYaml (and its constant
STARTER_YAML) out of the component file into a new utility module (suggested
name: utils/flowEditorSeed.ts); implement STARTER_YAML and export function
buildStarterYaml(path?: string) there using the same logic, then replace the
current local definition in FlowEditorModal.tsx with an import {
buildStarterYaml } from the new module (also update any other files that import
it, e.g., App.tsx) so the component file only exports React components and
complies with react-refresh/only-export-components.
---
Duplicate comments:
In `@packages/web/src/App.tsx`:
- Around line 105-107: The caller reads mutations.error after awaiting
mutations.deleteFlow(flowId), which can be stale; update the deleteFlow
implementation in useFlowMutations to return a structured result (e.g. { ok:
boolean; error?: MutationError }) or throw, update its type signature
accordingly, and modify the caller in App.tsx to use the returned payload (const
{ ok, error } = await mutations.deleteFlow(flowId)) and show error.message from
that returned error instead of accessing mutations.error; ensure any places
using deleteFlow are updated to the new return shape or handle thrown errors.
In `@packages/web/src/components/FlowEditorModal.tsx`:
- Around line 113-131: FlowEditorModal's modal wrapper (the div with
role="dialog" and data-testid="flow-editor-modal") currently lacks focus
management; add an initial focus, focus trap, and restore behavior: in the
FlowEditorModal component create a ref for the dialog container, save
document.activeElement when opening, on mount set focus to the dialog or its
first focusable child, add keyboard handling to close on Escape by calling
onCancel and implement a Tab/Shift+Tab focus-trap (or integrate a well-tested
library such as focus-trap-react or react-focus-lock) to prevent focus escaping
to background, and on unmount restore the previously focused element; ensure
these behaviors are tied to the existing onCancel flow so closing via backdrop
click or Escape restores focus.
- Around line 90-105: The keyboard handler and all modal action handlers must
short-circuit when a save is in progress: update the onKey function in the
useEffect to check a saving flag (e.g., if (saving) return) before handling
Escape or Ctrl/Cmd+Enter, and likewise guard the backdrop-click handler and the
close/cancel button handlers so they return immediately when saving; only call
onCancelRef.current() or onSaveRef.current(textRef.current) (after parseFlowYaml
succeeds) if saving is false. Ensure the same saving check is applied to the
handlers referenced elsewhere (the close/cancel click handlers and any backdrop
click handler) so no duplicate POSTs or modal closes happen while saving.
In `@packages/web/src/components/Sidebar.tsx`:
- Around line 290-298: The Edit button currently shows a fake keyboard hint
"(E)" in its title; remove the unimplemented shortcut hint by updating the title
prop on the button in Sidebar.tsx (the element with aria-label={`Edit
${node.name}`} and data-testid={`sidebar-edit-${flowId}`}) so it no longer
contains " (E)"; leave the onClick handler (onEditFlow) and aria-label as-is, or
alternatively implement a real keyboard handler if you intend to keep the hint.
🪄 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: e0ae188a-8295-42da-9cf3-f9290e9c6630
📒 Files selected for processing (4)
packages/web/__tests__/flow-mutations.test.tspackages/web/src/App.tsxpackages/web/src/components/FlowEditorModal.tsxpackages/web/src/components/Sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/tests/flow-mutations.test.ts
| ` | ||
|
|
||
| /** Build the seed YAML for a "New flow" inside a given folder path. */ | ||
| export function buildStarterYaml(path?: string): string { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and show its content around line 33
fd -p "FlowEditorModal.tsx" packages/web/src/Repository: naorsabag/OpenHop
Length of output: 109
🏁 Script executed:
# Get the file content to verify what's at line 33
cat -n packages/web/src/components/FlowEditorModal.tsx | head -50Repository: naorsabag/OpenHop
Length of output: 2006
🏁 Script executed:
# Search for buildStarterYaml to understand its usage and where it's exported
rg "buildStarterYaml" packages/web/src/ -B 2 -A 5Repository: naorsabag/OpenHop
Length of output: 2431
🏁 Script executed:
# Check for any drop-shadow filter usage in FlowEditorModal.tsx to verify guideline compliance
rg "drop-shadow" packages/web/src/components/FlowEditorModal.tsxRepository: naorsabag/OpenHop
Length of output: 43
Move buildStarterYaml to a separate utility module to comply with react-refresh/only-export-components.
Exporting non-component functions from component files breaks React's fast refresh and causes CI failures. Move this utility function and STARTER_YAML to a dedicated helper module.
Suggested refactor
- export function buildStarterYaml(path?: string): string {
+ // moved to packages/web/src/utils/flowEditorSeed.ts// packages/web/src/utils/flowEditorSeed.ts
const STARTER_YAML = `meta:
title: New flow
...`
export function buildStarterYaml(path?: string): string {
if (!path) return STARTER_YAML
return STARTER_YAML.replace(
/^meta:\n title: New flow\n/,
`meta:\n title: New flow\n path: ${path}\n`
)
}Then update App.tsx import to:
import { buildStarterYaml } from './utils/flowEditorSeed'🧰 Tools
🪛 GitHub Check: build & test (node 20)
[failure] 33-33:
Fast refresh only works when a file only exports components. Use a new file to share constants or functions between components
🪛 GitHub Check: build & test (node 22)
[failure] 33-33:
Fast refresh only works when a file only exports components. Use a new file to share constants or functions between components
🤖 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/FlowEditorModal.tsx` at line 33, Move the
non-component helper buildStarterYaml (and its constant STARTER_YAML) out of the
component file into a new utility module (suggested name:
utils/flowEditorSeed.ts); implement STARTER_YAML and export function
buildStarterYaml(path?: string) there using the same logic, then replace the
current local definition in FlowEditorModal.tsx with an import {
buildStarterYaml } from the new module (also update any other files that import
it, e.g., App.tsx) so the component file only exports React components and
complies with react-refresh/only-export-components.
Two reports of the folder "+" not appearing on hover even after the named-group fix. Rather than keep guessing at the hover-state path (opacity-0 → group-hover/folder:opacity-100 with @media (hover: hover) gating), just render it visible at all times. UX trade-off: a small "+" sits permanently on every folder row's right edge. Subtle (rgba 0.55) so it doesn't dominate; brightens to accent (#7df9ff) when its menu is open. Discoverability beats minimalism for the primary "create here" affordance. Flow rows keep the hover-fade for ✎/✕ (those are destructive and should stay subtle until needed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e subtree
Screenshot evidence (folder rows had no visible "+", but stray "+" symbols
appeared mid-tree at random vertical positions — including over child flow
rows on hover) revealed the real bug: the folder's <li className="group/folder
relative"> wrapped both the header AND the expanded children <ul>. Two
bad consequences:
1. The "+"'s `position: absolute; top: 50%` was relative to the entire
<li> — i.e. the geometric center of (header height + children height).
For deep / many-children folders this lands somewhere in the middle
of the child flow rows, not next to the folder name.
2. `:where(.group/folder):hover *` matched ANY descendant of the folder,
including children inside the <ul>. Hovering a child flow row
triggered the parent folder's group-hover, revealing the (mis-positioned)
"+" over the flow.
Fix: introduce a <div className="group/folder relative"> inside the <li>,
containing only the header row (toggle button + "+" + popover). The <ul>
of children is now a sibling of the .group/folder, not a descendant. So:
- "top: 50%" lands at the center of just the row
- hover state isolates to the header row — children no longer trigger it
- "+" still hover-revealed (opacity-0 → group-hover/folder:opacity-100),
per the requested UX (only on hover, not always visible)
Also adds `server.allowedHosts: true` to vite.config.ts so the dev server
accepts requests from `host.docker.internal` and other non-localhost
hostnames — needed when running OpenHop inside docker-compose / a dev
container so the user (or tooling) can reach the dev server through the
docker host's published port without tripping vite's "host not allowed" 403.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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/vite.config.ts`:
- Around line 11-17: Replace the insecure global override allowedHosts: true in
the Vite config with an explicit allowlist: modify the exported config where
allowedHosts is defined (symbol: allowedHosts) to use a whitelist array of
allowed hostnames (e.g., 'localhost', '127.0.0.1', and any known dev hostnames)
and read additional entries from an environment variable (e.g.,
process.env.VITE_ALLOWED_HOSTS split on commas) so dynamic
container/bridge/tunnel hosts can be appended; ensure the final value assigned
to allowedHosts is the array of strings rather than true.
🪄 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: ebac77b6-b3d1-4654-a5d1-00a70ceb0f77
📒 Files selected for processing (2)
packages/web/src/components/Sidebar.tsxpackages/web/vite.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/components/Sidebar.tsx
…w rows
Three changes that move together:
1. **Delete a whole folder** (✕ button next to + on every non-root folder
row, hover-revealed). Folders are virtual — they exist as a derived
view of each flow's meta.path — so there's no server-side bulk-delete
endpoint. App.handleDeleteFolder iterates every flow whose meta.path
equals the folder OR starts with `${folder}/`, confirms the count
up-front ("Delete folder X and all N flows inside?"), then issues
DELETE per flow. Stops on first failure with an alert. The
descendant-match predicate is locked in by 5 new tests so we don't
accidentally delete sibling paths like "billing-tax" when deleting
"billing".
2. **Synthetic root row "/"**. The sidebar's previous "+ at root" lived
next to the search input — a separate affordance with its own popover
logic. Replace it with a real root TreeItem at the top of the tree:
- always expanded, no toggle caret, no delete (per user ask: "root
dir that can't be deleted")
- same hover-revealed "+" menu as other folders, opens with parentPath=""
- non-clickable label "/", visually emphasized vs other folders so it
reads as the workspace root
The search bar simplifies back to just the input.
3. **More noticeable flow-row hover**. Inactive flow rows previously
had no background change on hover — a minor discoverability gap.
Now they pick up `hover:bg-white/5` (5% white tint, same as the
AddMenu menu items) and `hover:text-text` (matches folder rows
transitioning from text/70 → text on hover). Active rows are
unchanged because they already have the cyan highlight.
Verified locally: shared 93/93, server 19/19, cli 83/83, web 35/35
(5 new in flow-mutations) = 230/230. Lint/typecheck/format:check clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r takes effect
Inline style={{ background: 'transparent', color: '...' }} was winning
against the hover:bg-white/5 / hover:text-text Tailwind classes added in
the previous commit — inline styles outrank class-based ones. So the
class-based hover never applied; the row stayed transparent.
Move colors and background to className with arbitrary-value tw classes
for the active-cyan tones. Inline style now carries only the dynamic
paddingLeft (depth * 16 + 8). Now the inactive-row hover actually fires.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 of 8 comments resolved; 1 deferred.
1. flow-mutations.test.ts — assert err.suggestion. Switched the typo
ref from "nonexistent" (too far for findClosest) to "dbb" so the
validator actually emits the hint, then locked path/message/
suggestion together.
2. App.tsx — handleDeleteFlow / handleDeleteFolder no longer read
mutations.error after `await mutations.deleteFlow()`. The post-await
closure still pointed at the pre-setState `mutations`, so the
fallback alert always said "unknown error". Changed deleteFlow to
return MutationError | null directly; callers use the returned
value. (createFlow already returns its result + sets state, so the
modal's serverError={mutations.error} prop wiring is fine — modal
re-renders pick up the new error via props.)
3. FlowEditorModal — freeze every dismiss + save trigger while
`saving` is true. Added a savingRef updated each render; the
keyboard handler bails out early if saving, the backdrop click
ignores while saving, and Cancel + Close (✕) get `disabled={saving}`
plus disabled:opacity-40. Save button was already gated via
disabled={\!canSave}. Without this, Cmd+Enter could fire a second
POST while the first was in flight, creating duplicate flows.
4. (DEFERRED) Focus trap on the dialog. Real a11y fix — focus needs
to move into the modal on open, be trapped on Tab, restore on
close. Heavier lift than the rest of these and not a regression
from the current PR; tracking separately rather than expanding
scope here.
5. Sidebar.tsx — drop the bogus "(E)" hint from the edit button's
title; the keyboard shortcut isn't implemented.
6. Move buildStarterYaml + STARTER_YAML out of FlowEditorModal.tsx
and into a new src/lib/starter-yaml.ts. Exporting non-component
helpers from a component file trips
react-refresh/only-export-components and breaks Vite fast refresh.
App.tsx + the test file now import from the lib path.
7. starter-yaml.ts — replace the regex's literal ` ` (two spaces) with
`\s{2}` to satisfy no-regex-spaces. Same match, no lint warning.
8. vite.config.ts — replace `allowedHosts: true` (DNS-rebinding
liability) with an explicit allowlist: `host.docker.internal`,
`openhop` (docker-compose service name), and `.localhost` for
subdomain coverage. Added a `VITE_ADDITIONAL_ALLOWED_HOSTS` env
var for tunneled hostnames (ngrok / cloudflared / etc.) so
contributors don't have to edit the config.
Verified: shared 93/93, server 19/19, cli 83/83, web 35/35 = 230/230.
Lint/typecheck/format:check/build all clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Round 2 review comments addressed in cb9193d. 7 of 8 actioned, 1 deferred.
230/230 tests, lint/typecheck/format:check/build all clean. |
…le-overwrite race) Outside-diff comment from CodeRabbit's first review pass — caught after the round-2 reply. Once useFlowList exposed `reload()` (so create/delete can refresh the sidebar without remounting), two overlapping GET /api/flows requests could resolve out of order: the older one arriving last would overwrite the freshly-reloaded list, putting just-deleted flows back in the sidebar. Fix: AbortController per fetch effect; cleanup aborts the previous controller when `tick` bumps. AbortError is silently ignored — losing that race is the desired outcome, not a UI failure to surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Found and addressed the outside-diff comment in 2935d7a — the one I missed in round 2.
Fix: AbortController per fetch effect. Cleanup aborts the previous controller when Apologies for not catching this on the first sweep — it was tucked under the |
…deRabbit comment) The remaining open review comment from #76 — the focus trap I'd deferred as scope-creep last round. Reversing that decision: it's a genuine a11y bug, the implementation is small, and it was the only unaddressed item left on the PR. Three pieces: - On open: save the previously-focused element to `previouslyFocusedRef` and move focus into the dialog (the first enabled focusable child, with the dialog itself as a fallback via tabIndex={-1}). - On close: restore focus to the previously-focused element. - While open: trap Tab / Shift-Tab inside the dialog. Tabbing past the last focusable wraps to the first; Shift-Tab past the first wraps to the last; if focus is somehow outside (e.g. a portal refocus stole it), the next Tab pulls it back in. The Tab handler shares the keydown listener that already handles Esc / Cmd-Enter, but Tab is processed first and unconditionally — the saving-freeze gate intentionally does NOT apply to Tab, since keyboard nav inside the modal should still work while a save is in flight (e.g. to read the validation feedback). The focusable-elements selector matches the standard set (button:not([disabled]), [href], input/select/textarea, anything with a non-negative tabIndex). CodeMirror's editor surface is included because `cm-content` carries `tabindex="0"`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Focus management addressed in 1b3fd5b — reversing the deferral from round 2.
The Tab handler shares the existing keydown effect but is processed BEFORE the saving-freeze gate (keyboard nav inside the modal should still work while a save is in flight — e.g. to read validation feedback). CodeMirror is included in the focusable set because Looking at the current comment list, every line-level comment is now either ackd by CodeRabbit ( |
This was referenced May 5, 2026
3 tasks
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.
Summary
Lets users author and edit flows directly in the browser. The same authoring path as the CLI (
openhop push/openhop remove), now inline in the sidebar — no terminal required.Closes #74 (inline YAML editor — Add/Edit modal in left menu) and partially closes #12 (P11: enable editing flows from the UI).
What's new
Sidebar:
FlowEditorModal (new component):
parseFlowYaml()from@openhop/shared— sub-100ms feedback.path+suggestionfields (mirrors CLIvalidate --json).Wiring:
useFlowMutations.ts— POST + DELETE wrappers. Distinguishesvalidation/server/networkerrors so the modal can render path-prefixed details.useFlowListgains areload()so create/delete refresh the sidebar without remounting.App.tsxowns the modal state machine ({closed, new, edit}) and the delete confirmation. Edit pre-populates by GETting the stored flow and round-tripping throughyaml.stringify.Scope decisions (per #74)
openhop patchflow — out of scope, track separately if needed.window.confirm— simpler than a styled dialog, upgradeable later.New deps (web only)
Web is bundled at build time — none of these ship to npm package consumers, only to the static web build.
Test plan
flow-mutations.test.tscovering: STARTER_YAML passes validation; invalid step refs surface path + message; round-trip serialize → parse preserves shape; canned VALID_YAML accepts.openhop serve:{id, version, title}(whatcreateFlowconsumes){details: [{path, message, suggestion}]}(what the modal renders)/api/flows/:id→{id, meta, flow}(round-trip throughYAML.stringifyre-parses cleanly)openhop serve, exercise New / Edit / Delete in a real browser, verify flows render after save.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
New / Improved APIs
Quality / Tests
Chores