Fix screenshot support: schema, file filtering, and step numbering - #19
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for optional extension screenshots by validating screenshots/ folders during submission checks and emitting screenshotUrls in the generated gallery output so the Command Palette app (and gallery consumers) can display preview images.
Changes:
- Add screenshot discovery in
.github/scripts/generate.pyand emitscreenshotUrlsintoextensions.json. - Add optional
screenshots/folder validation (type/count/size) in.github/scripts/validate.py. - Document the screenshot folder structure/rules in
docs/CONTRIBUTING.mdanddocs/ARCHITECTURE.md, and regenerateextensions.jsonto include screenshots for one extension.
Reviewed changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
extensions.json |
Regenerated gallery metadata/entries; adds screenshotUrls for nielslaute.event-viewer. |
docs/CONTRIBUTING.md |
Adds contributor-facing guidance and rules for screenshots. |
docs/ARCHITECTURE.md |
Updates repo layout and generation/validation notes to include screenshots. |
.github/scripts/validate.py |
Validates optional screenshots/ folder (extensions, max count, max size). |
.github/scripts/generate.py |
Discovers screenshot images and emits screenshotUrls into gallery entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| filenames = [] | ||
| for name in os.listdir(screenshots_dir): | ||
| ext = os.path.splitext(name)[1].lower() | ||
| if ext in VALID_SCREENSHOT_EXTENSIONS: | ||
| filenames.append(name) | ||
|
|
||
| filenames.sort() | ||
|
|
||
| return [ | ||
| f"{BASE_RAW_URL}/extensions/{author}/{ext_name}/screenshots/{name}" | ||
| for name in filenames | ||
| ] |
There was a problem hiding this comment.
discover_screenshots() uses os.listdir() and only filters by extension, so it can accidentally include non-file entries (e.g., a directory named foo.png) and it will include more than the documented max of 5 screenshots if validation is skipped. Consider filtering with os.path.isfile(...) and defensively capping the returned list (e.g., first 5 after sorting) to match the documented/validated limits.
There was a problem hiding this comment.
Fixed in commit ac2c7da. discover_screenshots() now uses sorted(os.listdir(...)) with an os.path.isfile() guard and slices to MAX_SCREENSHOTS (5) before building URLs.
| # 10. Screenshots validation (optional folder) | ||
| screenshots_dir = folder / "screenshots" |
There was a problem hiding this comment.
The section comment here is labeled # 10, but earlier validation steps skip numbers (e.g., it jumps from # 7 to # 9). Consider renumbering these step comments to be sequential so future contributors can keep the docs/checklist aligned with the code more easily.
There was a problem hiding this comment.
Fixed in commit ac2c7da. Renumbered # 9 → # 8 and # 10 → # 9 so the steps are sequential (1–9).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/CmdPal-Extensions/sessions/b537131b-c6e9-416d-9185-7133004277a5 Co-authored-by: niels9001 <9866362+niels9001@users.noreply.github.com>
….py step numbering Agent-Logs-Url: https://github.com/microsoft/CmdPal-Extensions/sessions/ad7d164c-b952-492f-9c2f-6323f185c1af Co-authored-by: niels9001 <9866362+niels9001@users.noreply.github.com>
Three follow-up fixes to the screenshot support feature:
gallery.schema.json— Created the missing.github/schemas/gallery.schema.jsonthatgenerate.pyandextensions.jsonalready referenced via$schema/GALLERY_SCHEMA_URL. IncludesscreenshotUrls(optional, max 5 URIs) alongside all existing gallery entry fields.generate.py—discover_screenshots()hardeningos.path.isfile()to skip directories that happen to have image-like namessorted(os.listdir(...))and cap with[:MAX_SCREENSHOTS](new constant, mirrorsvalidate.py)validate.py— step numbering — Renumbered# 9→# 8and# 10→# 9to close the gap left by missing step 8.