Skip to content

feat(cli): hyperframes pick — open the design picker, gated behind feature flag#975

Closed
vanceingalls wants to merge 1 commit into
graphite-base/975from
feat/cli-pick
Closed

feat(cli): hyperframes pick — open the design picker, gated behind feature flag#975
vanceingalls wants to merge 1 commit into
graphite-base/975from
feat/cli-pick

Conversation

@vanceingalls
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls commented May 19, 2026

Summary

hyperframes pick — new CLI command that builds the design picker (from PR #971#974), serves it on localhost:8723, and opens the browser. Gated behind a feature flag so it ships dark until you flip it on.

Scope

  • 7 files, ~430 lines
  • packages/cli/src/commands/pick.ts (new)
  • packages/cli/src/cli.ts (1 line: register pick)
  • packages/cli/src/help.ts (8 lines: conditionally show in --help)
  • packages/cli/src/utils/env.ts (new helper: isDesignPickerEnabled())
  • docs/packages/cli.mdx (new ### pick section under Preview tab)
  • .fallowrc.jsonc (exclude vendored picker assets from the fallow audit)
  • .gitignore (.hyperframes/ + /templates/ — picker output dirs)

Behavior

HYPERFRAMES_DESIGN_PICKER=1 npx hyperframes pick
HYPERFRAMES_DESIGN_PICKER=1 npx hyperframes pick --port 8723
HYPERFRAMES_DESIGN_PICKER=1 npx hyperframes pick --build-only
  • Builds .hyperframes/pick-design.html via the skill's Python script (PR feat(skills): design picker build script + skill docs #973)
  • Serves the result on localhost:8723 with a small built-in HTTP server (Node createServer, ~70 lines)
  • Routes /templates/<slug>/* to the installed skills presentations/ directory so the picker's iframes resolve regardless of where the page is served from
  • Runs the Python build from a neutral temp cwd so a project's local DESIGN.html doesn't auto-advance the picker past the templates grid
  • Reads .hyperframes/picker-data.json if present, writes a sensible default otherwise
  • Opens the browser via open after the server is up

Feature flag

isDesignPickerEnabled() in packages/cli/src/utils/env.ts returns true when:

  • Running from source (isDevMode() — contributor convenience, no env var needed)
  • OR HYPERFRAMES_DESIGN_PICKER is set to 1 / true / on / yes

In any other case:

  • hyperframes pick short-circuits with a hint pointing at the env var
  • The command is hidden from hyperframes --help output (omitted from the "Getting Started" group)
  • The command stays registered in cli.ts so direct invocation by an enabled user works

Why a feature flag

The picker depends on:

  • python3 on PATH (build script is Python)
  • skills/hyperframes/ installed in the project (via hyperframes skills)
  • A free port at 8723

That's a lot of preconditions for a CLI command in default user flow. Shipping behind a flag lets the picker mature before flipping on for everyone.

Hot spots for review

  • findSkillsRoot walks up from cwd looking for skills/hyperframes/templates/design-picker.html + skills/hyperframes/scripts/build-design-picker.py. Falls back to walking up from the CLI binary's own location for monorepo dev mode. Limited to 10 parent dirs.
  • serveStatic is the inline HTTP server. Single file mime-types, /templates/<slug>/* route, basic forbidden-path guard. No auth — local-only via 127.0.0.1 bind.
  • buildPicker invokes python3 with stdin JSON. Errors surface as a clack spinner failure.

Tests

No new unit tests. Behavior is covered by:

  • TypeScript type-checking (bun run typecheck passes)
  • Manual: bun x tsx packages/cli/src/cli.ts pick opens the picker (dev mode auto-enables flag)

What's not in scope

Test plan

  • HYPERFRAMES_DESIGN_PICKER=1 npx hyperframes pick in a project with hyperframes skills installed opens the picker
  • Without the env var, npx hyperframes pick short-circuits with a hint
  • Without the env var, hyperframes --help does not list pick
  • Running from the monorepo (bun x tsx packages/cli/src/cli.ts pick) auto-enables (dev mode)
  • Build (bun run build) succeeds with the new files
  • bunx oxlint packages/cli/src/commands/pick.ts packages/cli/src/utils/env.ts passes

Stack

Top of stack. Depends on #971, #972, #973, #974.

🤖 Generated with Claude Code

Copy link
Copy Markdown
Collaborator Author

Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read pick.ts end-to-end (376 lines), plus the cli.ts / help.ts / env.ts / .gitignore / .fallowrc.jsonc / docs changes. Posting as COMMENT (not stamping) per merge policy.

Strong points

  • Feature-flag implementation is clean. isDesignPickerEnabled() checks dev-mode OR env var (accepting 1/true/on/yes). The flag gates both the command path (early return with a hint pointing at the env var) AND the --help listing (GROUPS array conditionally includes pick). The command stays registered in cli.ts so direct invocation by a power user still works. Good shape.
  • findSkillsRoot priority is right: project-local first, then walk-up from CLI binary location (10-dir cap). Falls back gracefully when neither matches with an actionable error message.
  • serveStatic has path-traversal protection on both branches (filePath.startsWith(resolve(rootDir)) / startsWith(resolve(presentationsDir))). Important since this binds an HTTP server that serves arbitrary files.
  • findOpenPort checks 100 ports starting from the requested one — handles "8723 in use" gracefully.
  • buildCwd = mkdtempSync(...) neutralizes the "auto-detect design.md from cwd" footgun called out in #973's build-design-picker.py:main — that path tries os.path.exists('design.md') on the script's cwd, so passing a neutral tempdir prevents the picker from auto-advancing past Phase 1 on first open. Good defensive shape.

Substantive concerns

1. Scope expansion — unrelated worker-entry + lambda command removal

The diff against cli.ts shows two big removals not described in the PR title/summary:

  • 30-line worker_threads pool bootstrap (lines 1-31 of the prior version) for pngDecodeBlitWorkerPool / shaderTransitionWorkerPool — these env-var auto-sets come from hf#677. Removing them in this PR is surprising scope; if these HF_SHADER_WORKER_ENTRY / HF_PNG_DECODE_BLIT_WORKER_ENTRY env-var auto-sets are no longer needed (because the worker entry resolution is handled elsewhere now), the commit message + PR body should say why.
  • Entire lambda command import + the 95-line ## hyperframes lambda section in cli.mdx.

If these are intentional (e.g., already missing from the parent branch in this stack, and the diff is just showing cumulative state from a base higher than main), please clarify in the PR body. If unintentional (e.g., bad rebase), they need restoring. Reviewers reading the title will not expect either change.

(Specifically: this PR's base is feat/picker-chrome, not main — so the diff displayed against main may be cumulative. Worth confirming the lambda removal isn't an accidental drop.)

2. var instead of const in env.ts:22

var v = (process.env.HYPERFRAMES_DESIGN_PICKER || "").trim().toLowerCase();

Style nit — const v would be conventional. oxlint may or may not flag depending on the rule config.

3. fallow-ignore-next-line complexity markers in 4 spots

findSkillsRoot, serveStatic, pick run(), and showUsage all carry fallow-ignore-next-line complexity markers. The functions aren't terrible individually, but scattering ignore markers loses the Fallow signal. Two options:

  • Refactor at least the run() function — it does 7-8 distinct things sequentially and splits cleanly into validateEnv / findAssets / loadPickerData / buildPicker / startServer / openBrowser.
  • Or consolidate into .fallowrc.jsonc as a file-level exemption for commands/pick.ts if Fallow supports it.

4. No tests for the CLI helpers

findSkillsRoot, findOpenPort, serveStatic, isDesignPickerEnabled — all four are unit-testable in isolation. PR body says "No new unit tests. Behavior is covered by TypeScript type-checking" — which doesn't cover any of the logical branches (project-local vs walk-up resolution, env-var permutations, port-fallback search, traversal-guard). One test per helper would be the right investment for a CLI command that ships to users.

Cheapest first: isDesignPickerEnabled — parametrize across 1/true/TRUE/on/yes/No/0/empty/undefined + dev-mode-true. ~10 lines.

5. import("open").then(...).catch(() => {}) swallows errors silently

import("open").then((mod) => mod.default(url)).catch(() => {});

If open fails (headless host, missing xdg-open, BROWSER env unset on Linux), the user sees the printed URL but doesn't know whether the browser actually opened. Cheap improvement:

import("open")
  .then((mod) => mod.default(url))
  .catch(() => {
    console.log(`  ${c.dim("Could not open browser automatically — open the URL above")}`);
  });

6. No graceful shutdown on SIGINT

await new Promise(() => {}) keeps the process alive but Ctrl+C kills the HTTP server abruptly. Cheap fix:

const server = serveStatic(...);  // return the server
process.on("SIGINT", () => server.close(() => process.exit(0)));

Not a blocker for local dev, but means rapid restart can occasionally hit "EADDRINUSE" on the same port until the OS reclaims it.

7. Doesn't validate cwd is a hyperframes project

process.cwd() is used as both outDir parent and serveStatic root. Running hyperframes pick from /tmp/random-dir happily creates /tmp/random-dir/.hyperframes/ and serves random files there. Cheap defense: check for hyperframes.json or package.json and warn (or error) if neither is present.

8. --port arg parsing fragile

parseInt(args.port ?? "8723", 10) — if user passes --port foo, this returns NaN and findOpenPort(NaN) will probably hang or error obscurely. Validate explicitly with a clear error.

Cross-PR consistency

Verified against #973's build-design-picker.py:

  • The --template / --templates-dir / --presentations-dir / --output args passed in buildPicker() match what the build script's argparse expects. ✓
  • The presentationsDir is passed as both --templates-dir AND --presentations-dir (line ~245 of pick.ts). That's because the build script expects index.json at the parent of --templates-dir, but in this layout presentations/ doesn't have an index.json sibling. Worth a comment explaining why both args resolve to the same directory.
  • The picker-data.json schema (palettes/typepairs/architectures/moodboards/prompt/prompt_text) matches what the build script reads from stdin. ✓

The schema does NOT include prompt_desc in the DEFAULT_PICKER_DATA constant (line 36-104) — but the build script substitutes __PROMPT_DESC__ (which I flagged on #973 as not present in the picker template). So the field is referenced but never actually used. Cross-PR cleanup opportunity.

Non-blocking

  • 376-line pick.ts could split into pick.ts + pickerServer.ts for readability (HTTP server + port logic separate from CLI orchestration). The current file is already at the size where future contributors will start looking for where to find things.

CI: required green per merge state. Marked unstable only because of the dependent stack.

— Rames Jusso

@vanceingalls vanceingalls changed the base branch from feat/picker-chrome to graphite-base/975 May 19, 2026 22:46
@vanceingalls vanceingalls changed the base branch from graphite-base/975 to feat/picker-chrome May 19, 2026 22:47
@vanceingalls vanceingalls changed the base branch from feat/picker-chrome to graphite-base/975 May 19, 2026 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants