Repository navigation
fix(design): keep icon generator from corrupting SVGs or truncating output - #526
Conversation
…utput apply_viewbox_size rewrote every width/height attribute in the document, including stroke-width and child elements, and only handled double quotes. Restrict it to the root <svg> tag and set width and height independently. generate_icon capped output at 4096 tokens, which truncates detailed icons. Raise it to 16384 and report finish_reason MAX_TOKENS separately from empty or malformed output. Fixes nextlevelbuilder#525
…n batch generate_batch still indexed candidates[0].content.parts unguarded and printed a bare "No valid SVGs generated." Share the response-text extraction and the no-SVG diagnosis with generate_icon, treat text=None as an empty response, and warn when a batch is cut off at MAX_TOKENS after returning fewer icons than requested. Refs nextlevelbuilder#525
clark-cant
left a comment
clark-cant
left a comment
There was a problem hiding this comment.
Summary: Focused fix for #525: root-only SVG sizing, higher single-icon token budget, and shared empty/truncated-response diagnostics. Risk: Medium.
Mandatory gates: duplicate/prior implementation clear (related merged #408 fixes preview HTML, not this generator; relevant git history has no prior fix). Project standards: CLAUDE.md and sibling-skill sync contract checked; source/mirror identical and npm run check:assets passes. Strategic necessity: clear reliability value addressing confirmed #525. No structural anti-slop concerns.
Important — _set_root_attr matches attribute-looking text inside another quoted attribute value. Reproduced offline on this exact head: valid input (where \x27 denotes an apostrophe) becomes <svg width="24" aria-label="height="24"" viewBox="0 0 24 24">. ElementTree parses the input but rejects the output at column 36. The negative lookbehind excludes stroke-width but does not establish that the match is outside quoted attribute values. Please scan actual root attributes with quote-aware tokenization (or safely parse XML), preserving unrelated attribute values; add regression tests for attribute-looking width/height text inside both quote styles and quoted > characters. Sync the bundled mirror.
Verification: 18/18 new offline tests across source and mirror pass; asset-sync check passes. The additional quoted-attribute regression above fails. No paid API calls. GitHub currently exposes no status checks and mergeStateStatus UNSTABLE.
Verdict: Request changes. Keep issue #525 open until the fix is complete.
Posted by /ck:review-pr at 2026-10-12T02:07:33Z
_set_root_attr searched the root tag with a regex, so width= or height= text inside another quoted attribute value (aria-label="height='2'") was rewritten and the output stopped being valid XML. Walk the tag's attributes instead, consuming each quoted value whole, and only replace an attribute whose name is exactly width or height. Refs nextlevelbuilder#525
clark-cant
left a comment
clark-cant
left a comment
There was a problem hiding this comment.
Summary: Re-reviewed head 296bf1c after the requested quote-aware root attribute fix. The tokenizer consumes whole quoted values, preserving attribute-looking width/height text and quoted > characters. Root-only sizing, independent dimensions, single/batch token diagnostics remain scoped to #525. Risk: Medium.
Mandatory gates: duplicate/prior implementation clear (related #408 addresses preview HTML, not this generator; relevant history and open/closed searches checked). Project standards: CLAUDE.md and sibling-skill mirror contract checked; source and bundled generator/tests are byte-identical, npm run check:assets passes. Strategic necessity: clear reliability value for confirmed #525. No critical/important or structural anti-slop findings remain.
Verification: 26 offline tests across source and mirror pass, including four quote-aware XML-preservation regressions per copy; asset sync passes. No paid model calls. No GitHub status checks exposed. Prior important finding resolved on this exact head.
Verdict: Approve. Posted by /ck:review-pr (cron-safe maintenance).
What does this PR change?
apply_viewbox_sizenow only edits the root<svg>tag, handles single-quoted attributes, and sets width and height independently. The icon generator's token cap goes from 4096 to 16384, and aMAX_TOKENScutoff is reported separately from empty or malformed output.generate_batchshares the same response guard and diagnosis.Why?
Fixes #525.
Previously every
width/heightin the document was rewritten (includingstroke-widthand child elements), and detailed icons were truncated at 4096 tokens. Edits are in.claude/skills/design/scripts/icon/generate.pyas the maintainer asked for this skill, then synced tocli/assets.Written with Claude Code (AI-assisted); I reviewed the change and ran the tests.
Checklist
src/ui-ux-pro-max/(source of truth), not directly in.claude/or.factory/(this skill lives in.claude/skills/design, per the maintainer)npm run sync:assets && npm run check:assetsincli/if data/scripts/templates changed.claude/skills/*/scripts/tests/,cli/tests/e2e/)feat:,fix:,docs:, etc.)main