feat(skills): design picker build script + skill docs#973
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
2aca5e9 to
b909b29
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Read build-design-picker.py end-to-end (436 lines), plus the SKILL.md changes and the references/ docs structure. Posting as COMMENT (not stamping) per merge policy. Several substantive findings on the build-script side.
Strong points
- Clean argument parsing surface; the
__X_JSON__placeholder substitution scheme is consistent and matches the design-picker.html template (cross-checked the placeholders in #974 against the substitutions here). generate_user_designidempotency is good —shutil.copy2only fires if the destination doesn't exist, which preserves agent-crafted design.html / summary.html over the generic scaffold. That's the right behavior for an iterative workflow.--tp-*token aliasing in the generated user-designtemplate.html(line ~95) matches the contract from #971's vendored templates. Cross-PR token consistency holds.
Substantive concerns
1. Cross-PR placeholder mismatch — __PROMPT_DESC__
build-design-picker.py substitutes __PROMPT_DESC__ (line ~397) but design-picker.html in #974 does not contain that placeholder. Verified by grepping the picker HTML for __[A-Z_]+__ — __PROMPT_DESC__ is absent. Result: the substitution is a silent no-op, and the data['prompt_desc'] value passed in via picker-data.json is never rendered.
Either:
- The picker template was changed without updating the script, or
__PROMPT_DESC__was meant to be inserted somewhere
Worth tracing to confirm intent. If the field is genuinely unused, drop it from the script + the picker-data.json schema; if it's meant to render somewhere, add the placeholder to the picker template.
2. 150-line inline-template HTML inside generate_user_design
Lines 76-225 of build-design-picker.py are a hardcoded template.html for the user-design path, embedded as a Python f-string. That's:
- ~150 lines of CSS/HTML buried inside a Python file (hard to discover; reviewers / future editors will look in
templates/first) - Duplicates patterns from the vendored templates (
.s1-headline,.s1-sub,.s2-photo, etc. mirror theeditorial-vienna/bold-postershapes) - Not lint-checked / format-checked the way the templates/ HTML files presumably are
Suggest extracting this to skills/hyperframes/templates/presentations/user-design/template.html (alongside the 34 vendored templates) and reading it with open().read() + format(). Then the user-design template participates in the same review/lint surface as the others.
3. Brittle regex parsing in parse_design_md
The function uses heuristic regexes that silently fall through to defaults:
primary = find_color(r'colors\.primary\}.*?(#[0-9a-fA-F]{6})') or \
find_color(r'primary[^#]{0,40}(#[0-9a-fA-F]{6})')
# ...
p_accent = primary or '#024ad8' # silent fallbackIf the user's design.md doesn't match either regex, the function returns #024ad8 as accent — a default that looks like a real value but isn't traceable to the source. Two concrete asks:
- Log when each role falls through to a default (
logger.warn("no accent in design.md, using fallback #024ad8")). Otherwise downstream debugging becomes "why does my picker show this blue?" - Consider failing the build (or refusing to generate user-design) when too many fields fall through. A
design.mdthat produces 4 defaults is functionally the same as nodesign.md— but the current code generates a "your design" card with the defaults, which is confusing.
4. Dynamic-import of sibling script (line ~313)
from importlib import util as imp_util
build_tp_path = os.path.join(script_dir, 'build-template-picker.py')
spec = imp_util.spec_from_file_location('build_tp', build_tp_path)
build_tp = imp_util.module_from_spec(spec)
spec.loader.exec_module(build_tp)This works but is harder to follow than a normal import. The reason for the dance is that build-template-picker.py has a hyphen in the filename (not a valid Python module name). Easiest cleanup: rename to build_template_picker.py (underscore) and use a normal from build_template_picker import extract_preview, extract_color_vars — both the build-script + the import become readable. Same for the other 8 build-*.py / tokenize-*.py / etc. scripts if they ever need to be imported.
Not a blocker; the current code works. Worth doing because the script bundle is now 9 files and dynamic imports compound.
5. SKILL.md size — 527 lines
Per the skill-creator rubric the main SKILL.md should stay lean and push detail to references/. The PR does add 8 references/*.md files (good!), but SKILL.md itself grew to 527 lines. Worth one more pass to see what's actually pulled into reading vs reference. Concrete cuts:
- The "Brief (exploratory requests only)" section's lettered-MCQ guidance feels like a
references/exploratory-brief.md. Keeps SKILL.md focused on the workflow stages. - The "Step 1: Design system" branch table is detailed — could move to
references/design-picker.md(already exists per this PR's added file) and leave SKILL.md with the high-level "check for DESIGN.html → either Picker / Style / Fast path" branch.
Not blocking. The skill works as-is; this is a maintainability nit.
6. CLI argument validation gap
--templates-dir is required but main() never validates that the directory exists or that index.json is at the expected sibling path. A bad invocation surfaces as a FileNotFoundError deep in the script (line 326's os.path.join(os.path.dirname(args.templates_dir), 'index.json')). One os.path.isdir check at the top + an explicit parser.error would save a future debugging session.
7. Hardcoded block-frame fallback in main() (line ~359)
base_design = os.path.join(args.presentations_dir, 'block-frame', 'design.html')If block-frame/ is ever removed or renamed in the templates/ directory, the user-design path silently breaks (the os.path.exists(base_design) check just skips the user-design generation, no warning). Worth either:
- A list of fallback candidates (block-frame, then a sibling, then any), OR
- An explicit warning when the base_design doesn't exist
Skill docs (references/*)
Read the structure, did not deep-read each of the 8 reference files. The split matches the topics described in the PR body. Naming is consistent (design-picker.md, design-html.md, design-showcase.md, shader-backgrounds.md, etc.) — good for grep-ability.
Non-blocking
- Test plan claim "
python3 build-design-picker.py --helpruns without import errors" is true but very weak — it doesn't exercise any of the parsing/substitution logic. A small unit test against a stubpicker-data.json+ a stubdesign-picker.htmlwould catch the__PROMPT_DESC__mismatch I called out above.
CI: required green per merge state. No CI test for the Python scripts that I could see — worth adding to a follow-up if you want regression coverage.
— Rames Jusso
scripts/build-design-picker.py — renders the picker HTML from the design-picker.html template, injecting template/palette/typepair JSON and a manifest of installed presentations. Auto-detects a project's DESIGN.html and pre-seeds a 'user-design' specimen when present. Supporting build scripts: tokenize-templates, build-design-templates, build-summaries, detokenize-summaries, inject-tp-tokens, lint-design-html, extract-template-structure, build-template-picker. Skill docs: design-picker.md is the contract for picker data and the polarity rule for supplemental palettes (primary = light, secondary = dark); design-html.md / design-html-templates.md / design-showcase.md / shader-backgrounds.md / figma-to-design-html.md / prompt-expansion.md cover the surrounding workflow. SKILL.md / visual-styles.md / house-style.md updated to reference the picker.
b909b29 to
44c8912
Compare
afcb858 to
65cd003
Compare

Summary
The picker's build pipeline and the skill documentation that describes it. No UI yet — that lands in PR #974. No CLI entry point yet — that lands in PR #975. This layer is the script that turns the template assets (PR #971 + #972) into a static picker HTML page, plus the markdown contract for callers.
Scope
skills/hyperframes/scripts/*.py— 9 Python scriptsskills/hyperframes/references/*.md— 8 reference documentsskills/hyperframes/SKILL.md,visual-styles.md,house-style.md— top-level skill docsWhat's here
Scripts
build-design-picker.py— the headline. Reads stdin JSON (palettes, typepairs, moodboards, prompt context) + a template manifest. Substitutes tokens indesign-picker.html. Writes a fully self-contained picker page to.hyperframes/pick-design.html. Auto-detects a project's localDESIGN.htmland seeds auser-designspecimen so existing-design projects open straight into fine-tune mode.tokenize-templates.py— rewires hard-coded palettes in vendored templates to the--tp-*token contract (already applied to the templates in PR feat(skills): vendor 34 tokenized HTML presentation templates #971).build-design-templates.py— drives bulk regeneration ofdesign.htmlfiles from a template-spec format.build-summaries.py/detokenize-summaries.py— produce / restore the picker-friendlysummary.htmlfiles (per template).build-template-picker.py— legacy single-phase picker builder, kept for compatibility.inject-tp-tokens.py— codemod that audits a template and inserts missing--tp-*aliases.lint-design-html.py— checks eachdesign.htmlfor the contract violations (raw hex in component styles, missing token aliases, bad polarity).extract-template-structure.py— walks the slide DOM and emits a structured description for prompt-expansion downstream.Skill docs
references/design-picker.md— the contract for picker data shape, the polarity rule for supplemental palettes (primary= light canvas,secondary= dark ink — applies even in dark-mode themes), and the picker workflow.references/design-html.md/design-html-templates.md— how to read and author adesign.html.references/design-showcase.md— guidance for the picker preview surface.references/shader-backgrounds.md— three.js shader background contract (used in PR feat(skills): design picker UI (Phase 1 templates index + Phase 2 fine-tune) #974's Phase 2).references/figma-to-design-html.md— Figma →design.htmlconversion guide.references/prompt-expansion.md— surrounding prompt-expansion step that consumes picker output.SKILL.md,visual-styles.md,house-style.md— top-level skill entrypoints updated to point at the picker workflow.Why this layer
The build script + skill docs are the contract for everything downstream. Splitting them out means:
design-picker.md) and the script that implements it without wading through the picker chromeWhat's not in scope
design-picker.html) — that's PR feat(skills): design picker UI (Phase 1 templates index + Phase 2 fine-tune) #974hyperframes pick) — that's PR feat(cli): hyperframes pick — open the design picker, gated behind feature flag #975Test plan
python3 skills/hyperframes/scripts/build-design-picker.py --helpruns without import errorsStack
Depends on #971, #972. Followed by #974, #975.
🤖 Generated with Claude Code