fix(slides): add agent-friendly aliases for screenshot flags - #2156
Open
BD-ZERO wants to merge 5 commits into
Open
fix(slides): add agent-friendly aliases for screenshot flags#2156BD-ZERO wants to merge 5 commits into
BD-ZERO wants to merge 5 commits into
Conversation
github-actions
Bot
added
the
size/M
📝 WalkthroughWalkthroughThe Slides screenshot shortcut adds hidden slide aliases. Shared parsing handles slide numbers and IDs across validation, dry-run, and execution. Tests cover precedence, invalid values, request generation, and live screenshot output. ChangesSlides screenshot aliases
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant SlidesScreenshot
participant SelectorHelpers
participant APIRequest
CLI->>SlidesScreenshot: pass presentation and slide selectors
SlidesScreenshot->>SelectorHelpers: parse and normalize selectors
SelectorHelpers-->>SlidesScreenshot: return effective selectors
SlidesScreenshot->>APIRequest: build screenshot request
APIRequest-->>CLI: emit dry-run request or execute request
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_test.go`:
- Around line 157-166: Update the error assertions in the slide validation test
to capture the metadata returned by errs.ProblemOf(err) and assert category is
errs.CategoryValidation, subtype is errs.SubtypeInvalidArgument, and param is
"--slide". Keep the existing *errs.ValidationError type assertion if needed, but
ensure all required typed metadata is verified through errs.ProblemOf.
In `@tests/cli_e2e/slides/slides_screenshot_dryrun_test.go`:
- Around line 51-78: Add a live E2E workflow alongside
TestSlidesScreenshotAliasesDryRunE2E that creates the required presentation and
slides, invokes slides +screenshot using the aliases with bot credentials,
verifies the saved screenshot output, and cleans up all created resources via
test cleanup. Keep the existing dry-run request-generation assertions unchanged
and make the live test self-contained.
🪄 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: 4d78c342-a02e-48db-bfeb-d43f185994c8
📒 Files selected for processing (3)
shortcuts/slides/slides_screenshot.goshortcuts/slides/slides_screenshot_test.gotests/cli_e2e/slides/slides_screenshot_dryrun_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6f42b5d3b0dd43d206121207ab8eee53b44661cc🧩 Skill updatenpx skills add larksuite/cli#fix/slides-screenshot-parameter-robustness -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2156 +/- ##
==========================================
+ Coverage 75.65% 75.69% +0.04%
==========================================
Files 940 942 +2
Lines 99876 100110 +234
==========================================
+ Hits 75564 75782 +218
- Misses 18530 18538 +8
- Partials 5782 5790 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Accept common presentation and slide selector aliases while keeping canonical flags authoritative. Validate the ambiguous --slide alias using the SXSD short slide ID format and cover the behavior with unit and dry-run E2E tests.
Assert the full typed validation contract for ambiguous --slide values and add a self-contained bot workflow that creates a presentation, exercises screenshot aliases, verifies the saved image, and cleans up the resource.
BD-ZERO
force-pushed
the
fix/slides-screenshot-parameter-robustness
branch
from
August 3, 2026 12:14
82a83d0 to
9dece89
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/cli_e2e/slides/slides_screenshot_workflow_test.go`:
- Around line 48-53: Move the parentT.Cleanup registration in the slide
screenshot workflow to immediately after extracting and validating
presentationID, before extracting or asserting slideID. Ensure cleanup still
targets the created presentation so failures in slideID validation cannot leave
tenant data behind.
- Around line 95-97: Update the image inspection in the screenshot workflow test
to open imagePath through the repository’s internal/vfs API instead of os.Open,
while preserving the existing error assertion and deferred close. Remove the
now-unused os import; the fixture setup exception does not apply here.
🪄 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: a5425750-f3b2-40e6-9b90-1f04f3b67dea
📒 Files selected for processing (4)
shortcuts/slides/slides_screenshot.goshortcuts/slides/slides_screenshot_test.gotests/cli_e2e/slides/slides_screenshot_dryrun_test.gotests/cli_e2e/slides/slides_screenshot_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/cli_e2e/slides/slides_screenshot_dryrun_test.go
- shortcuts/slides/slides_screenshot_test.go
- shortcuts/slides/slides_screenshot.go
Comment on lines
+48
to
+53
| presentationID := gjson.Get(createResult.Stdout, "data.xml_presentation_id").String() | ||
| require.NotEmpty(t, presentationID, "stdout:\n%s", createResult.Stdout) | ||
| slideID := gjson.Get(createResult.Stdout, "data.slide_ids.0").String() | ||
| require.NotEmpty(t, slideID, "stdout:\n%s", createResult.Stdout) | ||
|
|
||
| parentT.Cleanup(func() { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Register cleanup before asserting slideID.
Line 51 can stop the test before Line 53 registers cleanup. If the create response has presentationID but omits slide_ids, the test leaves the created presentation in the tenant. Move the cleanup registration to immediately after Line 49.
As per coding guidelines, “New flows or behavior changes require live E2E coverage with a self-contained create/use/cleanup workflow and bot credentials where applicable.”
🤖 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 `@tests/cli_e2e/slides/slides_screenshot_workflow_test.go` around lines 48 -
53, Move the parentT.Cleanup registration in the slide screenshot workflow to
immediately after extracting and validating presentationID, before extracting or
asserting slideID. Ensure cleanup still targets the created presentation so
failures in slideID validation cannot leave tenant data behind.
Source: Coding guidelines
Comment on lines
+95
to
+97
| imageFile, err := os.Open(imagePath) | ||
| require.NoError(t, err) | ||
| defer imageFile.Close() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use internal/vfs to open the generated image.
Replace os.Open(imagePath) with the repository internal/vfs API, then remove the os import. The test-fixture exception for os.* applies only to fixture setup, not output inspection.
Based on learnings, the os.* exception applies only when creating or populating test fixtures. As per coding guidelines, “Use internal/vfs filesystem APIs instead of os filesystem APIs.”
🤖 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 `@tests/cli_e2e/slides/slides_screenshot_workflow_test.go` around lines 95 -
97, Update the image inspection in the screenshot workflow test to open
imagePath through the repository’s internal/vfs API instead of os.Open, while
preserving the existing error assertion and deferred close. Remove the
now-unused os import; the fixture setup exception does not apply here.
Sources: Coding guidelines, Learnings
There was a problem hiding this comment.
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`:
- Around line 288-291: The mixed-selector validation error must include its
parameter metadata and the E2E test must verify it. In
shortcuts/slides/slides_screenshot.go lines 288-291, update the error built in
the slideIDs/slideNumbers validation branch with a consistent WithParam value;
in tests/cli_e2e/slides/slides_screenshot_dryrun_test.go lines 97-100, assert
that result.Stderr contains the expected error.param value.
🪄 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: 9d4ea7e5-2977-410b-9fce-bbaf80ec9fde
📒 Files selected for processing (6)
shortcuts/slides/shortcuts_alias_test.goshortcuts/slides/slides_add_slide.goshortcuts/slides/slides_delete_slide.goshortcuts/slides/slides_screenshot.goshortcuts/slides/slides_screenshot_test.gotests/cli_e2e/slides/slides_screenshot_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/slides/slides_screenshot_test.go
Comment on lines
+288
to
+291
| if len(slideIDs) > 0 && len(slideNumbers) > 0 { | ||
| return nil, nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id and --slide-number cannot be used together"). | ||
| WithHint("choose either slide IDs or slide numbers for one screenshot request") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Complete the mixed-selector validation metadata contract.
The producer does not set Param, and the E2E test does not assert it.
shortcuts/slides/slides_screenshot.go#L288-L291: add a consistentWithParam(...)value to the mixed-selector validation error.tests/cli_e2e/slides/slides_screenshot_dryrun_test.go#L97-L100: assert the resultingerror.paramvalue fromresult.Stderr.
📍 Affects 2 files
shortcuts/slides/slides_screenshot.go#L288-L291(this comment)tests/cli_e2e/slides/slides_screenshot_dryrun_test.go#L97-L100
🤖 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 `@shortcuts/slides/slides_screenshot.go` around lines 288 - 291, The
mixed-selector validation error must include its parameter metadata and the E2E
test must verify it. In shortcuts/slides/slides_screenshot.go lines 288-291,
update the error built in the slideIDs/slideNumbers validation branch with a
consistent WithParam value; in
tests/cli_e2e/slides/slides_screenshot_dryrun_test.go lines 97-100, assert that
result.Stderr contains the expected error.param value.
Source: Coding guidelines
6 tasks
There was a problem hiding this comment.
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 `@tests/cli_e2e/slides/slides_screenshot_dryrun_test.go`:
- Around line 119-123: Update the dry-run validation assertions in the slides
screenshot test to verify the complete typed error envelope from result.Stderr:
assert the expected error.type, error.subtype, error.param, and error.message,
while preserving the exit-code 2 assertion. Also assert that result.Stdout is
empty.
🪄 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: 1201b473-e85f-42f6-b7d3-764257f6b278
📒 Files selected for processing (3)
shortcuts/slides/slides_screenshot.goshortcuts/slides/slides_screenshot_test.gotests/cli_e2e/slides/slides_screenshot_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/slides/slides_screenshot.go
- shortcuts/slides/slides_screenshot_test.go
Comment on lines
+119
to
+123
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 2) | ||
| require.Equal(t, "--presentation", gjson.Get(result.Stderr, "error.param").String(), result.Stderr) | ||
| require.Contains(t, gjson.Get(result.Stderr, "error.message").String(), "unsupported --presentation input") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete validation envelope and stream contract.
This test checks only error.param and error.message. It can pass if the CLI emits the wrong error type or subtype, or writes output to stdout. Assert the expected error.type, error.subtype, error.param, and error.message from result.Stderr. Also assert that result.Stdout is empty.
As per coding guidelines, error-path tests must verify typed metadata. Based on learnings, validation failures must exit with code 2, write the typed JSON envelope to stderr, and leave stdout empty.
🤖 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 `@tests/cli_e2e/slides/slides_screenshot_dryrun_test.go` around lines 119 -
123, Update the dry-run validation assertions in the slides screenshot test to
verify the complete typed error envelope from result.Stderr: assert the expected
error.type, error.subtype, error.param, and error.message, while preserving the
exit-code 2 assertion. Also assert that result.Stdout is empty.
Sources: Coding guidelines, Learnings
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improve
slides +screenshotcompatibility for common AI-generated presentation and slide selector flags while preserving the canonical interface.Changes
--presentation-idalias for--presentation--slidealias:p[A-Za-z]{2,}routes to slide ID--slide-ids--slide-numbers--slides--slidevalues with typed validation metadataTest Plan
GOTOOLCHAIN=go1.23.12 make buildGOTOOLCHAIN=go1.23.12 make unit-testGOTOOLCHAIN=go1.23.12 go vet ./...gofmt -l .produces no outputGOTOOLCHAIN=go1.23.12 go mod tidyleavesgo.modandgo.sumunchangedgolangci-lintreports 0 issuesRelated Issues
Summary by CodeRabbit
New Features
--presentation-id.Bug Fixes