ci(sync-device-svgs): refresh the Resources/images catalog the app actually loads#2114
ci(sync-device-svgs): refresh the Resources/images catalog the app actually loads#2114bruschill wants to merge 2 commits into
Conversation
…tually loads The nightly Sync Device SVGs job cloned meshtastic/web-flasher and wrote device imagesets into Meshtastic/Assets.xcassets. But the app loads device images at runtime from the bundled 'images' subdirectory and DeviceHardware.json (see MeshtasticAPI.swift), maintained by scripts/download_images.py and the Xcode 'Download Node SVG Images' build phase. The xcassets device imagesets are no longer the runtime source, so the job wasn't keeping the real catalog current — it drifted until someone ran a clean build (which force-refreshes it) and committed the result. Retarget the workflow to run the repo's own download_images.py with --force against Meshtastic/Resources/images + DeviceHardware.json, so the committed snapshot always matches the upstream device hardware API. Also add explicit 'contents: write' permission so the scheduled auto-commit can push, and stage new/updated/pruned device SVGs via 'git add -A'.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe device resource sync workflow now uses Python to refresh device images and ChangesDevice resource synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/sync_device_svgs.yml (1)
65-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrevent code injection via template expansion.
Avoid using expression expansion (
${{ ... }}) directly inside inline bash scripts. Whilehas_changesis strictly controlled in this context and not exploitable, mapping it to an environment variable is a safer practice and resolves warnings from static analysis tools.♻️ Proposed fix
- name: Summary + env: + HAS_CHANGES: ${{ steps.check_changes.outputs.has_changes }} run: | - if [ "${{ steps.check_changes.outputs.has_changes }}" == "true" ]; then + if [ "$HAS_CHANGES" == "true" ]; then echo "✅ Device images and hardware catalog updated" else🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/sync_device_svgs.yml around lines 65 - 71, Update the Summary step’s shell condition to read has_changes through a step-level environment variable populated from steps.check_changes.outputs.has_changes, rather than expanding the GitHub expression directly inside the bash script. Preserve the existing true and no-changes messages.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/sync_device_svgs.yml:
- Around line 51-64: Update the commit-and-push step in the sync workflow to
push HEAD explicitly to the intended branch instead of using an implicit git
push from detached HEAD. Use the workflow’s branch context to construct the
destination, and retain the existing commit behavior; if branch protection
rejects the push, configure checkout authentication with a token authorized to
bypass those rules.
---
Nitpick comments:
In @.github/workflows/sync_device_svgs.yml:
- Around line 65-71: Update the Summary step’s shell condition to read
has_changes through a step-level environment variable populated from
steps.check_changes.outputs.has_changes, rather than expanding the GitHub
expression directly inside the bash script. Preserve the existing true and
no-changes messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f06ffe9-1ec2-4a91-b202-3e60d7012aeb
📒 Files selected for processing (1)
.github/workflows/sync_device_svgs.yml
Address review feedback:
- actions/checkout leaves a detached HEAD, so a bare 'git push' fails. Push
HEAD explicitly to the branch (git push origin HEAD:<ref_name>).
- Route github.ref_name and the has_changes output through step-level env vars
instead of expanding ${{ }} inside inline bash (avoids the shell-injection
pattern flagged by static analysis).
|
Thanks @coderabbitai — addressed both findings in 8fe7844: 1. Detached HEAD push (major). 2. Expression expansion inside bash (nitpick). Both Operational note (branch protection): if |
|
Tip For best results, initiate chat on the files or code changes.
The branch-protection note is a useful operational caveat; a PR-based sync would be an appropriate future alternative if direct scheduled pushes are not permitted. 🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
What changed?
Retargets the nightly Sync Device SVGs workflow (
.github/workflows/sync_device_svgs.yml) so it refreshes the device catalog the app actually loads.meshtastic/web-flasher→ svgo → write imagesets intoMeshtastic/Assets.xcassets" steps with a single call to the repo's ownscripts/download_images.py --force, writing toMeshtastic/Resources/images/+Meshtastic/Resources/DeviceHardware.json— the same script and output location the Xcode "Download Node SVG Images" build phase uses.permissions: contents: writeso the scheduled job can actually commit/push the refresh.git add -A, and detects changes (including brand-new untracked SVGs) with a path-scopedgit status --porcelain.Net: +37 / −127 (removes the now-unnecessary web-flasher clone, svgo install, and xcassets generation).
Why did it change?
The app loads device images at runtime from the bundled
imagessubdirectory andDeviceHardware.json(MeshtasticAPI.swift—Bundle.main.url(forResource:withExtension:subdirectory:"images")and the bundled hardware catalog), maintained byscripts/download_images.py. The device imagesets underAssets.xcassetsare no longer the runtime source (the remaining imagesets there are AppIcons). So the nightly job was updating a legacy location and not keeping the real catalog current — it drifted until someone happened to run a clean build (which force-refreshes the catalog) and committed the result.Concretely, this catalog is currently behind upstream: a new RAK6421 device is present in the API but missing from the committed snapshot. Pointing the automation at the correct script + path means the bundled catalog stays in sync with
https://api.meshtastic.org/resource/deviceHardwareautomatically, and contributors stop seeing spuriousDeviceHardware.json/image_manifest.jsondiffs after clean builds.How is this tested?
python3 -c "import yaml; yaml.safe_load(...)"— workflow YAML validates.scripts/download_images.pyis the exact script already run by the Xcode build phase (unchanged), so the workflow reproduces the same output the app bundle expects; running it locally with these args produces the expected catalog refresh (e.g. addsrak6421.svg+ its manifest/JSON entries).schedule+workflow_dispatchonly (no untrusted/fork input), andcontents: writeis the minimum permission required for the auto-commit. It can be dry-run via workflow_dispatch after merge to confirm the commit/push path.Screenshots/Videos (when applicable)
N/A — CI workflow change only.
Checklist
docs/user/ordocs/developer/. This is a CI-only change with noMeshtastic/Views/**or@Modelchanges, so no doc update is required — adding theskip-docs-checklabel.Summary by CodeRabbit