Skip to content

fix(slides): improve missing screenshot selector guidance - #2177

Merged
BD-ZERO merged 3 commits into
mainfrom
fix/slides-screenshot-require-selector
Aug 4, 2026
Merged

fix(slides): improve missing screenshot selector guidance#2177
BD-ZERO merged 3 commits into
mainfrom
fix/slides-screenshot-require-selector

Conversation

@BD-ZERO

@BD-ZERO BD-ZERO commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Improve slides +screenshot handling when list mode is invoked without --slide-id or --slide-number. The CLI now returns actionable typed guidance without issuing an API request.

Changes

  • Add an actionable hint for omitted or empty screenshot selectors.
  • Keep Validate as the selector validation source and remove the unreachable DryRun check.
  • Document full-deck screenshots by enumerating page IDs or numbers and processing up to 10 pages sequentially.
  • Add unit and built-binary dry-run regression coverage.

Test Plan

  • make unit-test
  • go vet ./...
  • gofmt -l .
  • go mod tidy produced no changes
  • golangci-lint reported 0 issues
  • Slides screenshot dry-run E2E passed

Related Issues

  • None

Summary by CodeRabbit

  • Bug Fixes

    • Improved slide screenshot validation for missing or empty slide selectors, including clearer error details and correction guidance.
    • Dry-run requests now consistently return structured validation errors, including when an empty slide ID is combined with a slide number.
  • Documentation

    • Added guidance for capturing full-deck screenshots in batches of up to 10 slides.
    • Clarified single-slide support and recommended retrying only failed batches.

@github-actions github-actions Bot added the size/M Single-domain feat or fix with limited business impact label Aug 4, 2026
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 735ba541-b94a-48ef-8bd9-e6675cbdc78b

📥 Commits

Reviewing files that changed from the base of the PR and between 508225d and 633872b.

📒 Files selected for processing (3)
  • shortcuts/slides/slides_screenshot.go
  • shortcuts/slides/slides_screenshot_test.go
  • tests/cli_e2e/slides/slides_screenshot_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • shortcuts/slides/slides_screenshot_test.go
  • shortcuts/slides/slides_screenshot.go
  • tests/cli_e2e/slides/slides_screenshot_dryrun_test.go

📝 Walkthrough

Walkthrough

The screenshot command now uses shared structured errors for missing selectors and empty --slide-id values. Tests verify validation details and dry-run behavior. Documentation describes full-deck screenshots in serial batches of up to 10 pages.

Changes

Slide screenshot validation

Layer / File(s) Summary
Shared selector error handling
shortcuts/slides/slides_screenshot.go, shortcuts/slides/slides_screenshot_test.go, tests/cli_e2e/slides/slides_screenshot_dryrun_test.go
Missing selectors and empty --slide-id values return structured validation errors with hints. Tests verify metadata, exit status, and empty stdout.
Full-deck screenshot guidance
skills/lark-slides/SKILL.md, skills/lark-slides/references/lark-slides-screenshot.md
Documentation requires enumerating pages and processing them serially in batches of up to 10 pages.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • larksuite/cli#2120: This PR also covers shared slide-selector validation and empty --slide-id handling.
  • larksuite/cli#2156: This PR also modifies slide-selector parsing and validation in slides_screenshot.go.

Suggested reviewers: ethan-zhx

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the improved guidance for missing screenshot selectors.
Description check ✅ Passed The description includes the required Summary, Changes, Test Plan, and Related Issues sections with relevant details.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/slides-screenshot-require-selector

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@633872ba76ee9e7de2716ea175ba841585d2f870

🧩 Skill update

npx skills add larksuite/cli#fix/slides-screenshot-require-selector -y -g

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@shortcuts/slides/slides_screenshot.go`:
- Line 84: Update list-mode slide selector validation around normalizeSlideIDs
and slidesScreenshotMissingSelectorError so an explicitly supplied empty
--slide-id is rejected before empty values are discarded, rather than allowing
the accompanying slide number to proceed. Preserve normal trimming and
normalization for non-empty IDs, and add a regression test covering --slide-id
"" together with --slide-number 1.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 434161a0-412a-41af-976f-5aab3133db1f

📥 Commits

Reviewing files that changed from the base of the PR and between 3b66d47 and 508225d.

📒 Files selected for processing (5)
  • shortcuts/slides/slides_screenshot.go
  • shortcuts/slides/slides_screenshot_test.go
  • skills/lark-slides/SKILL.md
  • skills/lark-slides/references/lark-slides-screenshot.md
  • tests/cli_e2e/slides/slides_screenshot_dryrun_test.go

Comment thread shortcuts/slides/slides_screenshot.go
@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.71%. Comparing base (3b66d47) to head (633872b).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
shortcuts/slides/slides_screenshot.go 90.90% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2177   +/-   ##
=======================================
  Coverage   75.70%   75.71%           
=======================================
  Files         944      944           
  Lines      100288   100295    +7     
=======================================
+ Hits        75926    75935    +9     
+ Misses      18565    18564    -1     
+ Partials     5797     5796    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@BD-ZERO
BD-ZERO merged commit 2297435 into main Aug 4, 2026
31 of 44 checks passed
@BD-ZERO
BD-ZERO deleted the fix/slides-screenshot-require-selector branch August 4, 2026 08:48
@liangshuo-1 liangshuo-1 mentioned this pull request Aug 4, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants