feat(slides): support explicit screenshot output paths - #2180
Merged
Conversation
github-actions
Bot
added
the
size/M
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe slide screenshot shortcut adds single-file output through ChangesScreenshot output handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant ScreenshotShortcut
participant ScreenshotAPI
participant ImageWriter
participant Filesystem
CLI->>ScreenshotShortcut: Pass screenshot options
ScreenshotShortcut->>ScreenshotAPI: Request selected slide image
ScreenshotAPI-->>ScreenshotShortcut: Return image bytes and format
ScreenshotShortcut->>ImageWriter: Resolve target and overwrite mode
ImageWriter->>Filesystem: Write screenshot
Filesystem-->>ImageWriter: Return actual path
ImageWriter-->>ScreenshotShortcut: Return output metadata
🚥 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@63cee60accf4882f76c03a422fbd6a5b2c045c32🧩 Skill updatenpx skills add larksuite/cli#feat/slides-screenshot-output-path -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2180 +/- ##
==========================================
+ Coverage 75.85% 75.87% +0.01%
==========================================
Files 958 958
Lines 101701 101792 +91
==========================================
+ Hits 77150 77239 +89
+ Misses 18684 18681 -3
- Partials 5867 5872 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
shortcuts/slides/slides_screenshot.go (1)
550-550: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass output routing as a typed value.
saveSlideScreenshotImageacceptsoutputDir,outputName,fallbackName, andoutputPathas positionalstringvalues. A caller can swap these values without a compiler error. PassslidesScreenshotOutputTarget, or a smaller dedicated write-target type, through the save helper chain.As per coding guidelines, prefer distinct types when same-typed values could be silently swapped.
🤖 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` at line 550, Update saveSlideScreenshotImage and its caller/helper chain to accept a typed output-routing value, using slidesScreenshotOutputTarget or a smaller dedicated write-target type instead of separate positional strings for outputDir, outputName, fallbackName, and outputPath. Adjust all call sites and field access accordingly so same-typed routing values cannot be accidentally swapped.Source: Coding guidelines
🤖 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 1281-1313: Update
shortcuts/slides/slides_screenshot_test.go:1281-1313 in
TestSlidesScreenshotOutputWritesRequestedRenderPath to request shots/preview.jpg
for the PNG response, then assert the PNG is written to shots/preview.png and
verify requested_output, output, output_adjusted, and omitted output_dir
metadata directly. Update shortcuts/slides/slides_screenshot_test.go:1456-1475
with a --content dry-run case using --output, asserting output is present and
output_dir is absent.
- Around line 564-597: Update
TestSlidesScreenshotOutputRejectsConflictingNamingFlags to assert the error’s
typed metadata with errs.ProblemOf, requiring errs.CategoryValidation and
errs.SubtypeInvalidArgument, while retaining the existing errors.As check for
ValidationError.Param and the conflict-message assertion.
In `@shortcuts/slides/slides_screenshot.go`:
- Around line 474-488: Reject outer whitespace in validateScreenshotOutputPath
with a typed --output validation error instead of trimming the value. In
shortcuts/slides/slides_screenshot.go lines 299-313, retain and use the original
requested value for path resolution and result metadata; in lines 284-288,
report that same unmodified value in dry-run output.
- Around line 763-783: Update writeScreenshotOutputPath and
writeUniqueScreenshotPath to use an atomic no-replace save whenever overwrite is
disabled, rather than relying only on the preceding Stat check. Treat an atomic
collision as an existing-file condition; in writeScreenshotOutputPath return the
existing validation error, while in writeUniqueScreenshotPath retry with the
next suffix. Preserve overwrite behavior and existing error handling for
non-collision failures.
In `@tests/cli_e2e/slides/slides_screenshot_workflow_test.go`:
- Around line 134-136: In the test flow around wantOutput, replace
filepath.EvalSymlinks with the existing internal/vfs EvalSymlinks API while
preserving the current error assertion and resolved-path behavior.
---
Nitpick comments:
In `@shortcuts/slides/slides_screenshot.go`:
- Line 550: Update saveSlideScreenshotImage and its caller/helper chain to
accept a typed output-routing value, using slidesScreenshotOutputTarget or a
smaller dedicated write-target type instead of separate positional strings for
outputDir, outputName, fallbackName, and outputPath. Adjust all call sites and
field access accordingly so same-typed routing values cannot be accidentally
swapped.
🪄 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: ce1c0e3e-fba4-4570-bbc7-2dd1d3788e7f
📒 Files selected for processing (6)
shortcuts/slides/slides_screenshot.goshortcuts/slides/slides_screenshot_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-screenshot.mdtests/cli_e2e/slides/slides_screenshot_dryrun_test.gotests/cli_e2e/slides/slides_screenshot_workflow_test.go
BD-ZERO
force-pushed
the
feat/slides-screenshot-output-path
branch
from
August 5, 2026 03:45
7a48886 to
a3a83a6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
BD-ZERO
commented
Aug 5, 2026
fangshuyu-768
approved these changes
Aug 5, 2026
fangshuyu-768
left a comment
fangshuyu-768
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed the current head. No actionable findings; targeted Slides tests, race checks, vet, formatting, build, and dry-run E2E passed, and all required CI checks are green.
zkh-bytedance
pushed a commit
that referenced
this pull request
Aug 6, 2026
Adds AI-friendly output path handling to `slides +screenshot`. - Supports `--output` for a single screenshot in both existing-slide and XML render modes. - Validates selector count, conflicting output flags, unsafe paths, directories, whitespace, and unsupported extensions with structured errors. - Reconciles the requested filename with the server’s actual PNG/JPEG format and reports the final path through `output`, `requested_output`, and `output_adjusted`. - Avoids replacing existing screenshots by appending `_2`, `_3`, and subsequent suffixes. - Keeps `--output-dir` for multi-page screenshots and preserves `--output-name` for render mode. - Updates the Slides Skill with explicit `--slide-number` / `--slide-id` guidance and task-scoped screenshot directories. - Adds unit, dry-run E2E, and live workflow coverage for validation, path handling, format adjustment, and collision behavior. Co-authored-by: TRAE CLI <traecli@bytedance.com>
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
2 participants
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
Add AI-friendly
--outputsupport toslides +screenshot, aligning single-image screenshots with other file-producing shortcuts while preserving the server-returned image format and avoiding normal name collisions.Changes
--output <file>for exactly one screenshot in both existing-slide and XML render modes..png,.jpg, and.jpegextensions with the actual server response format before choosing the final path._2,_3, and so on.requested_output, actual absoluteoutput, andoutput_adjusted: truewhenever extension reconciliation or conflict avoidance changes the path.--output-diroutput on the same conflict-avoidance naming rule.--overwriteflag is introduced.Test Plan
GOTOOLCHAIN=go1.23.0 make buildGOTOOLCHAIN=go1.23.0 make unit-testGOTOOLCHAIN=go1.23.0 make vetGOTOOLCHAIN=go1.23.0 make fmt-checkGOTOOLCHAIN=go1.23.0 go mod tidyproduces no changesRelated Issues
Summary by CodeRabbit
New Features
--output.--output-dir.Bug Fixes
Documentation