feat: bundle server+web into npm package; add openhop demo (closes B3, B9, W6)#66
Conversation
…op demo Closes READINESS-AUDIT B3, B9 (and the 6 sub-tasks B9.1, B9.4, B9.5, B9.6) and W6. @openhop/server (publishable): - drop private:true, version 0.1.0, files:[dist], main:dist/server.js, exports map (types: src, default: dist), esbuild build script + prepack - factor named exports buildApp/startServer + RunningServer type, with auto-start guard so `node dist/server.js` and `npx tsx src/index.ts` both still work - @openhop/shared moves to devDependencies (bundled at build time) @openhop/web (publishable): - drop private:true, version 0.1.0, files:[dist] — published tarball ships only the prebuilt static assets; build deps are devDependencies so consumers don't pull react/elkjs/etc openhop CLI: - depend on @openhop/server, @openhop/web, fastify, @fastify/static - replace spawn-based serve with in-process startServer + new startWebServer (fastify-static of @openhop/web/dist/, SPA fallback) - new `openhop demo` subcommand: bootstraps API+web, posts a bundled starter flow, opens the browser via xdg-open/open/start (no runtime dep on `open` package) - esbuild externals updated for the new deps Documentation cleanup: - skills/openhop/SKILL.md: drop the "git clone the repo" fallback; agents run `npx openhop demo` or `npx openhop serve` instead (B9.4) - docker-compose.yml: header comment marks it contributor-only (B9.5) - README "Try it in 60 seconds": lead with `npx openhop demo`; install table rewritten with 4 scenarios (B9.6) - README: drop the `#18-#37` blockquote (W6) — those issues are all closed and the install paths they tracked are now real Verified end-to-end: `npm pack` of all three workspaces, install in a clean dir, `npx openhop demo` boots both servers, /health responds, the starter flow gets POSTed, the SPA fallback works, SIGTERM shuts down cleanly. Test suites: shared 93/93, server 19/19, CLI 83/83. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ 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 due to trivial changes (2)
WalkthroughAdded an in-process ChangesDemo Command & Server Refactoring
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI (npx openhop demo)
participant APIServer as API Server (startServer)
participant WebServer as Web Server (startWebServer)
participant Flows as Flows API
User ->> CLI: Run npx openhop demo
CLI ->> APIServer: startServer() on port 8787
APIServer -->> CLI: Running (url)
CLI ->> WebServer: startWebServer() on port 8788
WebServer -->> CLI: Running (url)
CLI ->> Flows: POST /api/flows (STARTER_FLOW_YAML)
Flows -->> CLI: Returns flowId
CLI ->> User: Print readiness & flow URL
CLI ->> User: Open browser (if enabled)
User ->> WebServer: GET /flow/{flowId}
WebServer -->> User: Serve SPA (index.html)
User ->> CLI: SIGINT (Ctrl-C)
CLI ->> APIServer: close()
CLI ->> WebServer: close()
CLI -->> User: Exit
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 unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/server/src/index.ts (1)
77-93: 💤 Low valueConsider displaying a user-friendly URL when binding to
0.0.0.0.When
hostis0.0.0.0(e.g., fromHOST=0.0.0.0in Docker), the returnedurlwill behttp://0.0.0.0:8787, which isn't directly usable in a browser. Consider mapping0.0.0.0to127.0.0.1orlocalhostfor display purposes while keeping the actual bind address unchanged.Suggested change
await app.listen({ port, host }) - const url = `http://${host}:${port}` + const displayHost = host === '0.0.0.0' ? '127.0.0.1' : host + const url = `http://${displayHost}:${port}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/index.ts` around lines 77 - 93, The returned URL uses the actual bind host (so startServer currently returns "http://0.0.0.0:port" when host is "0.0.0.0"); change startServer so it keeps binding to the real host for app.listen but computes a displayHost (e.g., if host === "0.0.0.0" set displayHost to "127.0.0.1" or "localhost") and build the returned url from displayHost instead of host; update the returned object (url field) accordingly while leaving app.listen({ port, host }) and close(): () => app.close() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/package.json`:
- Around line 17-27: The package.json currently points "types" and the exports
"types" to ./src/index.ts while the "files" array only includes "dist", so
published packages will lack the source types; fix by either (A) add "src" to
the "files" array so ./src/index.ts is published (update the "files" array to
include "src"), or (B) change your build to emit declaration files (.d.ts) into
dist (enable "declaration": true / "declarationDir" in tsconfig and update your
build script) and then update "types" and the exports "types" entry to point to
the generated .d.ts in dist (e.g., dist/index.d.ts).
In `@README.md`:
- Line 66: Update the install table entry that currently shows "`npm install -g
openhop && openhop serve`" as the primary command so it leads with "`npx openhop
serve`" as the default (zero‑setup) invocation and list the global install "`npm
install -g openhop && openhop serve`" as an optional alternative; edit the table
row containing the text "`Long-lived local server`" to swap the order and
wording accordingly so the first example shown is "`npx openhop serve`" and the
second mentions the global install as an alternative.
---
Nitpick comments:
In `@packages/server/src/index.ts`:
- Around line 77-93: The returned URL uses the actual bind host (so startServer
currently returns "http://0.0.0.0:port" when host is "0.0.0.0"); change
startServer so it keeps binding to the real host for app.listen but computes a
displayHost (e.g., if host === "0.0.0.0" set displayHost to "127.0.0.1" or
"localhost") and build the returned url from displayHost instead of host; update
the returned object (url field) accordingly while leaving app.listen({ port,
host }) and close(): () => app.close() unchanged.
🪄 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: 10233868-5b3f-4259-821c-f11362a90ec7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (11)
README.mddocker-compose.ymlpackages/cli/package.jsonpackages/cli/src/demo.tspackages/cli/src/help-json.tspackages/cli/src/index.tspackages/cli/src/web-server.tspackages/server/package.jsonpackages/server/src/index.tspackages/web/package.jsonskills/openhop/SKILL.md
Address CodeRabbit feedback on PR #66. @openhop/server (Major): - types field pointed at src/index.ts but files:[dist] excluded src/, so TS consumers of the published package would fail to resolve types. - Switch to emitting declarations: tsconfig.build.json excludes test files, build now runs `tsc -p tsconfig.build.json --emitDeclarationOnly` before esbuild, types/exports.types point to dist/index.d.ts. - Add `prepare: npm run build` so workspace installs build server before the CLI's own prepare runs (CLI's tsc resolves @openhop/server types via dist/index.d.ts after this). README (Minor): - Install table's "Long-lived local server" row now leads with `npx openhop serve`; global install kept as parenthetical alternative. Matches the new zero-setup positioning of the rest of the table. Verified end-to-end: rebuilt all three workspaces, re-packed, installed in a clean dir, `npx openhop demo` boots cleanly. Server tarball now ships 10 files (server.js + 4 d.ts + 4 d.ts.map) instead of 2; web and CLI tarball file counts unchanged. All workspace typechecks pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…od CVE) @fastify/static v8 has two moderate CVEs that #66 dragged into the production tree when it added the package as a direct dep on the CLI: - GHSA-pr96-94w5-mx2h (path traversal in directory listing) - GHSA-x428-ghpx-8j92 (route-guard bypass via encoded separators) Verified: - npm audit --omit=dev --audit-level=moderate → 0 vulnerabilities - All workspaces' tests pass (shared 93/93, server 19/19, cli 83/83, web 22/22 = 217 total) - All workspaces' coverage thresholds pass - Live serve smoke: /health ok, web UI + assets serve, SPA fallback intact, path-traversal probes return safe index.html (CVE patch confirmed in the running app). The dev-only `esbuild`-via-`vitest@1` advisories that the original W7 finding called out are intentionally NOT addressed here. Per `01:38` they're not a launch blocker, and the new audit gate in the follow-up PR (#69) only fails dev-tree at high+. A vitest 4 bump would force lowering coverage thresholds because @vitest/coverage-v8@4 instruments arrow functions / inline callbacks more aggressively; not worth the trade for non-blocking dev moderates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…od CVE) @fastify/static v8 has two moderate CVEs that #66 dragged into the production tree when it added the package as a direct dep on the CLI: - GHSA-pr96-94w5-mx2h (path traversal in directory listing) - GHSA-x428-ghpx-8j92 (route-guard bypass via encoded separators) Verified: - npm audit --omit=dev --audit-level=moderate → 0 vulnerabilities - All workspaces' tests pass (shared 93/93, server 19/19, cli 83/83, web 22/22 = 217 total) - All workspaces' coverage thresholds pass - Live serve smoke: /health ok, web UI + assets serve, SPA fallback intact, path-traversal probes return safe index.html (CVE patch confirmed in the running app). The dev-only `esbuild`-via-`vitest@1` advisories that the original W7 finding called out are intentionally NOT addressed here. Per `01:38` they're not a launch blocker, and the new audit gate in the follow-up PR (#69) only fails dev-tree at high+. A vitest 4 bump would force lowering coverage thresholds because @vitest/coverage-v8@4 instruments arrow functions / inline callbacks more aggressively; not worth the trade for non-blocking dev moderates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…od CVE) (#68) @fastify/static v8 has two moderate CVEs that #66 dragged into the production tree when it added the package as a direct dep on the CLI: - GHSA-pr96-94w5-mx2h (path traversal in directory listing) - GHSA-x428-ghpx-8j92 (route-guard bypass via encoded separators) Verified: - npm audit --omit=dev --audit-level=moderate → 0 vulnerabilities - All workspaces' tests pass (shared 93/93, server 19/19, cli 83/83, web 22/22 = 217 total) - All workspaces' coverage thresholds pass - Live serve smoke: /health ok, web UI + assets serve, SPA fallback intact, path-traversal probes return safe index.html (CVE patch confirmed in the running app). The dev-only `esbuild`-via-`vitest@1` advisories that the original W7 finding called out are intentionally NOT addressed here. Per `01:38` they're not a launch blocker, and the new audit gate in the follow-up PR (#69) only fails dev-tree at high+. A vitest 4 bump would force lowering coverage thresholds because @vitest/coverage-v8@4 instruments arrow functions / inline callbacks more aggressively; not worth the trade for non-blocking dev moderates. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous single `npm audit --audit-level=high` gate let W7's moderate `@fastify/static` CVE land on master via #66 without flagging it. Splitting the audit step: - prod tree (`--omit=dev --audit-level=moderate`) — anything moderate+ that ships to end users blocks the merge. - full tree (`--audit-level=high`) — dev-only advisories don't reach users, so block on high+critical only to avoid noise. Verified locally on master @ c8392c7 (pre-W7-fix tree): the new prod-tree gate exits 1 (catches the @fastify/static GHSA), while the full-tree gate stays at exit 0. Once #68 merges, both gates pass. Also enabled Dependabot vulnerability alerts + automated security fixes via the GitHub API on naorsabag/OpenHop. With those on, security updates produce PRs that bypass dependabot.yml's major-version ignore filter — closing the gap that let GHSA-pr96-94w5-mx2h linger silently between #66 and #68. Annotated dependabot.yml's ignore block to call out the security-update bypass so future tightening doesn't accidentally close the gap again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous single `npm audit --audit-level=high` gate let W7's moderate `@fastify/static` CVE land on master via #66 without flagging it. Splitting the audit step: - prod tree (`--omit=dev --audit-level=moderate`) — anything moderate+ that ships to end users blocks the merge. - full tree (`--audit-level=high`) — dev-only advisories don't reach users, so block on high+critical only to avoid noise. Verified locally on master @ c8392c7 (pre-W7-fix tree): the new prod-tree gate exits 1 (catches the @fastify/static GHSA), while the full-tree gate stays at exit 0. Once #68 merges, both gates pass. Also enabled Dependabot vulnerability alerts + automated security fixes via the GitHub API on naorsabag/OpenHop. With those on, security updates produce PRs that bypass dependabot.yml's major-version ignore filter — closing the gap that let GHSA-pr96-94w5-mx2h linger silently between #66 and #68. Annotated dependabot.yml's ignore block to call out the security-update bypass so future tightening doesn't accidentally close the gap again. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
openhop-launch/READINESS-AUDIT.md: B3 (openhop democommand) and B9 (server+web were monorepo-only) plus all 6 of B9's "don't miss" sub-tasks (B9.1–B9.6) and W6 (closed#18–#37README link).npx openhopis a runnable tool, not a config-shipper. The published tarball (or, more precisely, the publishedopenhop+@openhop/server+@openhop/webset) is everything a fresh machine needs to render a flow — no clone, no docker.Design choices
Followed the Storybook / Remotion shape (Option A from the audit): three independent npm packages with
openhopas the user-facing one, depending on@openhop/serverand@openhop/web.@openhop/server— exportsbuildApp()andstartServer({ port, host, logger })returning aRunningServerhandle. Still auto-starts when invoked as a script (node dist/server.jsornpx tsx src/index.ts) via animport.meta.url === fileURLToPath(process.argv[1])guard, so the dev experience is unchanged.@openhop/web— ships the prebuilt Vite output as a static-assets-only package; build-time deps (react, elkjs, tailwind, vite) all moved to devDependencies so consumer installs don't pay for them.openhopCLI — replaces the oldspawn('npx tsx ../../server/src/index.ts')+spawn('npm run dev')pair with an in-processstartServer()call plus a tiny static server (@fastify/staticover@openhop/web/dist/with SPA fallback for/flow/<id>).openhop demo— new subcommand: pick ports, boot both servers in-process, POST a bundled starter flow, open the browser viaxdg-open/open/start(no runtime dep on theopenpackage). Closes the abort criterion in01-repo-readiness.md:37.Verification
npm test --workspacesnpm run lintuseFlowPolling.ts, none from this PR)npm run format:checknpm packper workspace/tmpdir +./node_modules/.bin/openhop demo --no-open --port 18787 --web-port 18788openhop: ready api=… web=… flow=…on stdout,/healthreturns{"status":"ok"}, SPA fallback/flow/<id>returnsindex.html, SIGTERM shuts down cleanlyThis is the v0.1 acceptance gate the audit asks for — proves what
npm publishwould deliver, without actually publishing.Test plan
npm packfor all three packages, installs in a clean dir, runsnpx openhop demo, confirms the browser opens at the printed/flow/<id>URLnpx openhop serve(the unchanged-from-user-POV command) still works the same way it did before this PRskills/openhop/SKILL.mdno longer tells agents togit cloneOut of scope (deliberately deferred)
npm publishof any of the three packages — the audit's B4 blocker, gated on this landing firstgit tag v0.1.0+ GitHub Release notes (B5)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
npx openhop demo(starts API + web UI, opens browser by default) and improvedserveCLI with in-process API/web startup, configurable ports, and options like--no-openand--no-web.Documentation