docs: improve Spark developer onboarding#24
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| python-version: "3.11" | ||
|
|
||
| - name: Set up Node | ||
| uses: actions/setup-node@v4 |
There was a problem hiding this comment.
WARNING: Node.js 22 is installed but never used in this workflow. The only steps that run are python3 -m unittest discover -s tests and python3 scripts/sass-compat.py --check assets/main.css. actions/setup-node@v4 adds ~30s of setup time and disk on every CI run for no benefit — remove the Node step until a Node-based check (lint, build, etc.) is actually wired in.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| run: python3 -m unittest discover -s tests | ||
|
|
||
| - name: Check committed CSS compatibility | ||
| run: python3 scripts/sass-compat.py --check assets/main.css |
There was a problem hiding this comment.
SUGGESTION: Add an explicit permissions: block (e.g. permissions: { contents: read }) and consider a concurrency: group with cancel-in-progress: true. A verify-only workflow should request the least privilege it needs, and canceling superseded runs saves minutes on every push to a PR.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| node-version: "22" | ||
|
|
||
| - name: Run tests | ||
| run: python3 -m unittest discover -s tests |
There was a problem hiding this comment.
SUGGESTION: sass-compat.py imports yaml (PyYAML) in several places; CI does not install it. If any test or sass-compat path hits a YAML parser, the workflow will fail with ModuleNotFoundError. Either add pip install pyyaml (or a requirements-ci.txt) to this step, or guard the YAML imports in the scripts.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| description: Paste relevant local command output. | ||
| placeholder: | | ||
| python3 -m unittest discover -s tests | ||
| python3 scripts/sass-compat.py --check assets/main.css |
There was a problem hiding this comment.
WARNING: Every other field in this template is required: true, but the checks "Verification" field is not. For a reproducible-bug template this is the most useful field for maintainers (it confirms whether the reporter ran the canonical tests). Consider making it required, or at minimum add required: false with an explicit note that it's optional, so the schema intent matches the prose.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| - Product detail page | ||
| - Cart or side cart | ||
| - App hook | ||
| - CSS build or compatibility |
There was a problem hiding this comment.
SUGGESTION: The Affected surface dropdown has no "Web Component" option even though Spark ships 6 Web Components (<spark-add-to-cart>, <spark-cart-drawer>, <spark-progress-bar>, <spark-quantity>, <spark-subscription>, <spark-upsell-item>) per CLAUDE.md. Bug reports against those components will be forced into "Other" or miscategorized. Add a "Web Component" option (and consider sub-options for the six components).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| ```bash | ||
| python3 -m unittest discover -s tests | ||
| python3 scripts/sass-compat.py --check assets/main.css |
There was a problem hiding this comment.
SUGGESTION: The verification section lists python3 scripts/sass-compat.py --check assets/main.css but does not install dependencies first. sass-compat.py parses YAML in places; a fresh clone running only this command may hit ModuleNotFoundError: yaml. Consider adding python3 -m pip install pyyaml (or a pinned requirements-dev.txt) as the first step, so the verification commands are actually self-sufficient on a clean machine.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| Spark follows human-readable release notes rather than a strict package-manager version contract. The current public version is shown in [README.md](README.md). When releasing a new version, update the README version, add a dated changelog section, and publish a Git tag or GitHub release when the repo is ready for external consumers to pin versions. | ||
|
|
||
| ## Unreleased |
There was a problem hiding this comment.
SUGGESTION: The previous entries use ## X.Y.Z - YYYY-MM-DD (dated). The new top section is just ## Unreleased with no date and no version. That is conventional, but to keep grep-ability and tooling friendly, consider including the in-progress version (e.g. ## Unreleased - 1.1.2) so the next release can simply replace the heading instead of inserting a new section in the middle of the file.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ntk push | ||
| ``` | ||
|
|
||
| Expected time: 5-10 minutes if you already have store credentials. If `ntk` reports that `-a/--apikey` and `-s/--store` are required, create `config.yml` with `ntk init` or pass those flags explicitly. If Python prints a local OpenSSL or `urllib3` warning before `ntk` output, use a newer Python virtual environment; the warning comes from the local Python TLS stack, not from Spark. |
There was a problem hiding this comment.
WARNING: "If ntk reports that -a/--apikey and -s/--store are required..." — this hard-codes the exact ntk CLI flag names and error message. If next-theme-kit renames the flags or changes the error wording, this README sentence will be silently wrong and contributors will lose trust in the docs. Either link to the canonical ntk --help / ntk README rather than quoting the error, or add a CI check that exercises ntk init and updates this paragraph when the wording changes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ntk push | ||
| ``` | ||
|
|
||
| Expected time: 5-10 minutes if you already have store credentials. If `ntk` reports that `-a/--apikey` and `-s/--store` are required, create `config.yml` with `ntk init` or pass those flags explicitly. If Python prints a local OpenSSL or `urllib3` warning before `ntk` output, use a newer Python virtual environment; the warning comes from the local Python TLS stack, not from Spark. |
There was a problem hiding this comment.
SUGGESTION: "If Python prints a local OpenSSL or urllib3 warning before ntk output, use a newer Python virtual environment..." This troubleshooting note is about the user's local Python TLS stack, not about Spark. It will date quickly (Python 3.12+ ships stricter TLS defaults) and crowds out Spark-specific troubleshooting. Consider moving it to a docs/troubleshooting.md or to a footnote, and keep the Quick Start focused on Spark-specific steps.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -9,6 +9,7 @@ This directory contains Spark's theme-developer, design-team, and architecture d | |||
| | [theme-settings-partials.md](theme-settings-partials.md) | Designers, theme developers | Catalog of Theme Settings-backed partials and template surfaces. | | |||
| | [figma-section-library-plan.md](figma-section-library-plan.md) | Design team, theme developers | Plan for mapping Figma section components to Spark section authoring units. | | |||
| | [section-specs/](section-specs/) | Design team, theme developers | Per-section specs for the current homepage section partials. | | |||
There was a problem hiding this comment.
SUGGESTION: The new row for extending-spark.md is sandwiched between section-specs/ (a directory of specs) and design-block-authoring.md. extending-spark.md is a higher-level "how to extend" guide, so it logically belongs as the first or second row of the Start Here table (before the per-section specs and authoring guides). Reorder so readers find the extension overview before the per-feature details.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 0 New Issues | Recommendation: Ready to merge (subject to carried-forward items) Overview
The follow-up commit Resolved in this commit
Carried forward (unchanged files / unchanged lines)
Notes on new code in this diff
Assumptions
Files Reviewed (2 files in this incremental diff)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit c0cbd3c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit c0cbd3c)Status: 7 Issues Found (5 resolved since prior review, 2 carried forward, 3 new) | Recommendation: Address the new CI/README findings before merge Overview
The follow-up commit Resolved in this commit
NEW issues (this commit)
Carried forward (on unchanged lines / unchanged scope)
Assumptions
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous review (commit b77da8c)Status: 18 Issues Found | Recommendation: Address before merge Overview
This PR is a docs/CI polish change — README quick-start split, new CONTRIBUTING/CHANGELOG, issue templates, a verify-only GitHub Actions workflow, and a new Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (10 files)
Reviewed by minimax-m3 · Input: 48.9K · Output: 4.3K · Cached: 184.3K |
|
|
||
| - name: Install CI dependencies | ||
| run: | | ||
| python3 -m pip install --upgrade pip |
There was a problem hiding this comment.
SUGGESTION: python3 here bypasses actions/setup-python@v5, which only puts the configured python (3.11) on PATH. On the Ubuntu runner, python3 resolves to the system Python (3.10 on older images, 3.12 on newer), so PyYAML and the unittest step may run against a different interpreter than the rest of the workflow advertises. Either drop setup-python and rely on the runner default, or use python -m pip install pyyaml / python -m unittest discover -s tests consistently so the 3.11 toolchain is what CI actually exercises.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| run: python3 -m unittest discover -s tests | ||
|
|
||
| - name: Run JavaScript module tests | ||
| run: node tests/js/membership-pricing.test.js |
There was a problem hiding this comment.
SUGGESTION: This step hardcodes a single JS test file. New tests/js/*.test.js files will be silently skipped by CI as the JS surface grows (cart, PDP, events, etc.). Consider looping over the directory so the step scales, e.g.:
- name: Run JavaScript module tests
run: |
set -e
for f in tests/js/*.test.js; do
echo "Running $f"
node "$f"
doneThat way adding a new JS test under tests/js/ is enough to get CI coverage without editing the workflow.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| | `make release` | Rebuild minified `assets/main.css` and stage it for commit | | ||
| | `make dev` | Legacy: run Tailwind watcher + ntk watcher in parallel | | ||
|
|
||
| ## Troubleshooting |
There was a problem hiding this comment.
SUGGESTION: A new top-level ## Troubleshooting section is wedged between the ## Development command table and ## CSS Compatibility Verification. Its two bullets (ntk credential errors, pre-ntk Python warnings) both describe Quick Start install pain, not Development-tooling pain, so it sits in the wrong section. Either move it under ### Install On A Store as a #### Troubleshooting (or > **Note:**) block, or rename to something like ## Quick Start Troubleshooting so readers find it next to the install steps. Note also that a second Troubleshooting quick read: block already lives under ## CSS Compatibility Verification at line 94, so keeping the two related to the sections they live in will reduce the impression of duplicate content.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Summary
Verification
Follow-up