Update theme skills for preview and visual QA - #36
Conversation
| status="modified" | ||
| fi | ||
| else | ||
| status="update" |
There was a problem hiding this comment.
CRITICAL: Non-strict semver versions silently fall back to status="update", contradicting the new README.
The regex gate ^[0-9]+([.][0-9]+){2}$ only matches strict X.Y.Z. Any installed or source version with a pre-release suffix (1.7.0-rc1), a missing component (1.7), or build metadata (1.7.0+build.1) skips the new comparison and lands on the old update status. The new README.md:55-58 paragraphs promise stale, modified, and local-newer rows, so users running ./skills.sh status will see plain update rows and assume nothing changed, when in reality their installed pre-release is older than the released source.
Either loosen the regex (and ignore pre-release/build in the comparison) or emit a new unknown-version status so the regression is visible.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| | **Recommended** | Clone `NextCommerceCo/skills` and run `./skills.sh`; choose your local agent target and this skill. | | ||
| | **No checkout** | Use `npx skills add NextCommerceCo/skills -g --skill next-theme-dev` and add `-a <agent>` when you want a specific agent. | | ||
| | **Fallback** | Load this `SKILL.md` as a system prompt, context file, rule, or chat upload if your tool does not support native skills. | | ||
| | **Version check** | From a source checkout, run `./skills.sh status all next-theme-dev`. A `stale` row names the source and installed versions; review with `dry-run` before refreshing. | |
There was a problem hiding this comment.
WARNING: Version-check row only documents the stale row, but skills.sh now reports three distinct statuses.
README.md:55-58 documents stale, modified, and local-newer. This row only mentions stale, so an agent following the skill will not know how to interpret local-newer (newer local copy) or modified (same version, divergent files) when they appear in ./skills.sh status output. Update the row to list all three or link to the README for the full taxonomy.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| | **Recommended** | Clone `NextCommerceCo/skills` and run `./skills.sh`; choose your local agent target and this skill. | | ||
| | **No checkout** | Use `npx skills add NextCommerceCo/skills -g --skill next-theme-figma` and add `-a <agent>` when you want a specific agent. | | ||
| | **Fallback** | Load this `SKILL.md` as a system prompt, context file, rule, or chat upload if your tool does not support native skills. | | ||
| | **Version check** | From a source checkout, run `./skills.sh status all next-theme-figma`. A `stale` row names source and installed versions; review with `dry-run` before refreshing. | |
There was a problem hiding this comment.
WARNING: Version-check row only documents the stale row, but skills.sh now reports three distinct statuses.
README.md:55-58 documents stale, modified, and local-newer. This row only mentions stale, so an agent running ./skills.sh status all next-theme-figma will not know how to interpret the other two statuses. Update the row to list all three or link to the README.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| and rationale. DOM metrics alone are not visual QA and must never be silently | ||
| substituted for screenshots. | ||
|
|
||
| This runbook does not relax the per-route desktop/tablet/mobile coverage gate. |
There was a problem hiding this comment.
WARNING: Runbook insists tablet coverage is still required but its own step 5 only mandates desktop + mobile.
Step 5 (ntk capture --viewports=desktop,mobile) drops tablet, yet the closing paragraph demands every route still receive desktop/tablet/mobile coverage with explicit missing-viewport status. An agent that follows step 5 verbatim will not produce the tablet screenshots this paragraph claims are mandatory. Either drop tablet from the per-route gate or extend step 5's example to --viewports=desktop,tablet,mobile.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| ```bash | ||
| ntk capture --url="/?preview_theme=<theme-id>&skip_cache=1" \ | ||
| --output=qa-output --viewports=desktop,mobile --json --no-progress |
There was a problem hiding this comment.
SUGGESTION: --output=qa-output is a relative path and behavior depends on the agent's CWD.
The skill does not state where the agent should run from; the destination directory will resolve against whatever cwd the agent happens to be in (frequently the theme root, repo root, or a tmp checkout). Make the example explicit (--output=./qa-output or an absolute path) or state "run from the theme root" alongside the command so QA artifacts land in a predictable location for the next handoff step.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge All 5 issues from the previous review at
No new issues were found in the changed lines. Files Reviewed (6 files)
Previous Review Summary (commit cc50794)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit cc50794)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (12 files)
Reviewed by minimax-m3 · Input: 23K · Output: 2.4K · Cached: 139.3K |
This updates
next-theme-devandnext-theme-figmabased on problems found during a real theme build.The developer skill now covers standalone custom product templates, explicit product assignment, cache bypass, preview sessions, validation, screenshots, mobile layouts, and touch interactions.
The Figma skill covers empty metadata responses, large-frame slicing, asset export limits, selection sync failures, and the visual evidence required before handoff.
skills.sh statusalso reports version drift across Claude, Codex, and Agents installations.Checks:
Related work: Theme Kit #34, Spark #39, and oscar-prime #5693.