feat(slides):slide screenshot#1358
Merged
Merged
Conversation
github-actions
Bot
added
the
size/L
|
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:
📝 WalkthroughWalkthroughThis PR introduces a new ChangesSlides Screenshot Feature
Sequence DiagramsequenceDiagram
participant CLI
participant SlidesScreenshot
participant SlideImagesAPI
participant SlideRenderAPI
participant FileSystem
CLI->>SlidesScreenshot: Execute with flags
SlidesScreenshot->>SlidesScreenshot: Validate inputs
alt Render mode (--content)
SlidesScreenshot->>SlideRenderAPI: POST /slide_image/render (content)
SlideRenderAPI-->>SlidesScreenshot: slide_image (base64 data)
else List mode (--presentation)
SlidesScreenshot->>SlideImagesAPI: POST /xml_presentation/.../slide_images (slide_ids/slide_numbers)
SlideImagesAPI-->>SlidesScreenshot: slide_images[] (base64 array)
end
SlidesScreenshot->>FileSystem: Decode base64, write files (unique names)
SlidesScreenshot-->>CLI: JSON metadata (no base64)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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@3f3e8ac4a356eab8221349df54102eddcefbb299🧩 Skill updatenpx skills add larksuite/cli#feat/slide_image -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
shortcuts/slides/slides_screenshot.go (1)
406-422: ⚖️ Poor tradeoffAvoid hand-building API error envelopes.
Per coding guidelines: "Use
runtime.CallAPITyped(shortcuts) orerrclass.BuildAPIError(raw responses) for Lark API errors — never hand-build error envelopes". While this handles malformed response data rather than initial API failures, consider usingerrclass.BuildAPIErrororerrs.NewInternalError(errs.SubtypeUnknown, ...).WithCause(err)for unclassified errors to maintain consistent error typing.🤖 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 406 - 422, The slidesScreenshotAPIDataError function is hand-building an API error envelope; replace this with the project's standardized error constructors (use errclass.BuildAPIError for raw API-response style errors or errs.NewInternalError(errs.SubtypeUnknown, "...").WithCause(err) for unclassified/internal issues) so error typing stays consistent—update slidesScreenshotAPIDataError to construct and return one of those standardized error types (referencing slidesScreenshotAPIDataError, summarizeScreenshotAPIData, and the ErrDetail fields currently used) and include the same raw_data/log_id details as the cause or metadata on the returned error.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 402-418: Update TestSlidesScreenshotRejectsBadOutputDir to assert
typed error metadata using errs.ProblemOf instead of checking error message
substrings: call errs.ProblemOf(err) and verify the returned Problem's Category
and Subtype match the expected validation error and that Problem.Param equals
the flag name (e.g., "output-dir" or "--output-dir") for the runSlidesShortcut
invocation (SlidesScreenshot). Also assert cause preservation if applicable
(e.g., ErrInvalidPath) rather than relying on strings.Contains.
- Around line 342-357: Update TestSlidesScreenshotRenderRejectsListOnlyFlags to
assert typed error metadata using errs.ProblemOf instead of substring matching:
after runSlidesShortcut returns a non-nil err, call p := errs.ProblemOf(err) and
assert p != nil, then check p.Category, p.Subtype (the validation subtype for
presentation/content conflict) and p.Param equals the conflicting flag (e.g.,
"--presentation" or "--content") as appropriate; keep the existing
runSlidesShortcut and SlidesScreenshot references and preserve the existing
fatal checks when the problem metadata is missing or mismatched.
- Around line 238-252: The test TestSlidesScreenshotListRequiresSelector
currently asserts the missing-selector error by checking the error message
substring; update it to use errs.ProblemOf to assert the typed error metadata
instead: call errs.ProblemOf(err) (or equivalent helper) and assert the expected
Problem.Category, Problem.Subtype and Param (e.g., the selector param name) and
that the original cause is preserved for the error returned from
runSlidesShortcut when invoking SlidesScreenshot; replace the strings.Contains
check with these typed assertions while keeping the same runSlidesShortcut
invocation.
- Around line 325-340: In TestSlidesScreenshotRenderRejectsSlideSelectors
replace the substring-based assertion with a typed assertion using
errs.ProblemOf: call errs.ProblemOf(err, errs.Validation) (or the codebase's
Validation identifier) to get the problem, assert it's non-nil, then check the
problem's Subtype/Kind equals the validation subtype for conflicting arguments
(e.g. "conflicting-args" or the repo's specific subtype) and that the
problem.Param or Params include the "--content" parameter (and/or the slide
selector param like "--slide-id"/"--slide-number"); keep the existing error
generation by runSlidesShortcut and SlidesScreenshot but verify via
errs.ProblemOf rather than strings.Contains.
- Around line 420-470: The test must assert typed error metadata via
errs.ProblemOf for the underlying error; after obtaining exitErr, call
errs.ProblemOf on the underlying cause (e.g. errors.Unwrap(exitErr) or
exitErr.Cause) and assert the returned problem is non-nil and its Category and
Subtype match the expected values for the "no images" error (add these
assertions into TestSlidesScreenshotNoImagesErrorIncludesRawDataAndLogID near
where exitErr is inspected).
In `@shortcuts/slides/slides_screenshot.go`:
- Line 474: Replace the output.Errorf(...) calls used for local file I/O
failures in slides_screenshot.go (inside the function handling screenshots where
validate.SafeInputPath / SafeOutputPath is already used) with typed errors using
errs.NewInternalError(errs.SubtypeFileIO, "<context message>") and attach the
original error via .WithCause(err); update each occurrence (the shown return at
write screenshot, and the other instances at the lines noted) to return
errs.NewInternalError(errs.SubtypeFileIO, fmt.Sprintf("write screenshot %s: %v",
path, err)).WithCause(err) (or similar context-specific messages), and add the
import "github.com/larksuite/cli/pkg/errs" if missing.
- Line 285: The return uses output.Errorf for a local file I/O failure; replace
it with the typed error constructor errs.NewInternalError(errs.SubtypeFileIO,
...) (include the original error and a descriptive message like "create output
directory %s") and add the import "github.com/larksuite/cli/pkg/errs" if
missing; update the return in the function that creates the output directory
(the line returning output.Errorf(...)) to return the errs.NewInternalError(...)
instance instead.
- Line 19: Remove the import of github.com/larksuite/cli/internal/vfs and stop
calling vfs.MkdirAll and vfs.OpenFile; instead use runtime.FileIO() (the
fileio.FileIO instance) and its Save and Stat methods: replace any vfs.MkdirAll
usage with relying on fileIO.Save (which creates parent dirs) and replace
vfs.OpenFile(..., O_EXCL) patterns by implementing a no-overwrite flow that
checks file existence via fileIO.Stat and picks a unique filepath (e.g., append
a suffix or increment a counter) in a loop before calling fileIO.Save; add a
small helper (e.g., makeUniquePath or ensureUniqueSave) near the existing save
logic to encapsulate the Stat+unique-name loop, and remove all references to
vfs.OpenFile/vfs.MkdirAll in the functions that currently use them.
In `@skills/lark-slides/references/lark-slides-screenshot.md`:
- Around line 96-99: Replace the contradictory guidance that reads “不知道页面 ID，传
slide id 即可” with a clear instruction that if slide_id is unknown the user
should pass the slide index using the --slide-number option; update the text
mentioning slide_id to instead instruct using --slide-number (and optionally
give the expected numeric format) so the docs consistently direct users to use
--slide-number when slide_id is unavailable.
---
Nitpick comments:
In `@shortcuts/slides/slides_screenshot.go`:
- Around line 406-422: The slidesScreenshotAPIDataError function is
hand-building an API error envelope; replace this with the project's
standardized error constructors (use errclass.BuildAPIError for raw API-response
style errors or errs.NewInternalError(errs.SubtypeUnknown, "...").WithCause(err)
for unclassified/internal issues) so error typing stays consistent—update
slidesScreenshotAPIDataError to construct and return one of those standardized
error types (referencing slidesScreenshotAPIDataError,
summarizeScreenshotAPIData, and the ErrDetail fields currently used) and include
the same raw_data/log_id details as the cause or metadata on the returned error.
🪄 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
Run ID: 3ec276fc-8760-4124-b78b-a897eb3abdc0
📒 Files selected for processing (9)
internal/registry/scope_priorities.jsonshortcuts/common/runner.goshortcuts/common/runner_args_test.goshortcuts/common/types.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_screenshot.goshortcuts/slides/slides_screenshot_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-screenshot.md
ethan-zhx
force-pushed
the
feat/slide_image
branch
from
0dda6f5 to
ed5ad8b
Compare
ethan-zhx
force-pushed
the
feat/slide_image
branch
from
ed5ad8b to
cfc1dfc
Compare
ethan-zhx
force-pushed
the
feat/slide_image
branch
from
cfc1dfc to
fd15a95
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 427-430: The file slides_screenshot_test.go has formatting issues
that do not comply with Go's standard formatting rules as enforced by gofmt. Run
the command gofmt -w shortcuts/slides/slides_screenshot_test.go to automatically
reformat the file to the correct standards, including the Headers map
initialization around line 427-430, and then verify the changes are correct
before committing.
- Around line 487-496: The test in the error handling section for the
slide-number API path (starting around the errs.ProblemOf call) currently only
asserts LogID and Hint fields, but is missing complete typed metadata
validation. Add assertions for Problem.Category and Problem.Subtype after the
existing LogID check. Additionally, since this appears to be a validation error,
use errors.As to extract the underlying errs.ValidationError type and assert the
Param field to verify the validation error includes the expected parameter
information. This ensures full typed metadata coverage and proper cause
preservation as per the coding guidelines.
In `@shortcuts/slides/slides_screenshot.go`:
- Around line 400-404: In the slidesScreenshotImageDataCauseError function,
replace the direct type assertion `if apiErr, ok := err.(*errs.APIError); ok`
with the errors.As pattern to properly handle wrapped errors. Use errors.As to
unwrap the error and check if it contains an *errs.APIError type, which will
correctly handle cases where the error is wrapped and will pass the errorlint
validation.
- Around line 425-428: The error handling for the runtime.DoAPI call is
incorrectly wrapping all errors as internal errors, which downgrade typed errors
and misclassify network failures. Instead of using errs.WrapInternal(err) for
the error returned from runtime.DoAPI, check if the error is already a typed
error and preserve it; for actual network/transport failures, use
errs.NewNetworkError(errs.SubtypeNetworkTransport, ...) to properly classify
them as network errors rather than internal errors. This ensures lower-layer
typed errors pass through unchanged and transport failures are correctly
categorized.
🪄 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
Run ID: b92c4c29-92cc-4a9b-bee2-befbea09eab1
📒 Files selected for processing (9)
internal/registry/scope_priorities.jsonshortcuts/common/runner.goshortcuts/common/runner_args_test.goshortcuts/common/types.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_screenshot.goshortcuts/slides/slides_screenshot_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-screenshot.md
✅ Files skipped from review due to trivial changes (5)
- shortcuts/slides/shortcuts.go
- shortcuts/common/types.go
- skills/lark-slides/SKILL.md
- skills/lark-slides/references/lark-slides-screenshot.md
- internal/registry/scope_priorities.json
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/common/runner_args_test.go
ethan-zhx
force-pushed
the
feat/slide_image
branch
2 times, most recently
from
6626491 to
d946e23
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1358 +/- ##
==========================================
+ Coverage 73.72% 73.80% +0.07%
==========================================
Files 781 787 +6
Lines 74217 76142 +1925
==========================================
+ Hits 54717 56193 +1476
- Misses 15437 15741 +304
- Partials 4063 4208 +145 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ethan-zhx
force-pushed
the
feat/slide_image
branch
from
d946e23 to
436647d
Compare
ethan-zhx
force-pushed
the
feat/slide_image
branch
from
436647d to
0f522e0
Compare
fangshuyu-768
previously approved these changes
Jun 18, 2026
ethan-zhx
force-pushed
the
feat/slide_image
branch
from
0f522e0 to
3f3e8ac
Compare
PR Quality SummaryThe semantic review system could not produce a fully trusted result. This is not reported as a code defect. System status
|
fangshuyu-768
approved these changes
Jun 22, 2026
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 a new
slides +screenshotshortcut to fetch slide screenshots and save them as local image files instead of printing Base64 payloads to stdout.What changed
slides +screenshotfor existing slide pages:--presentation--slide-idor--slide-number--content @file/ stdin.lark-slides/screenshotsby defaultint_arrayflag parsing coverage needed by--slide-numberslides:presentation:screenshotscope priorityTests
int_arrayflag parsingSummary by CodeRabbit
slides +screenshotshortcut to generate local PNG/JPEG screenshots from either rendered slide XML (--content) or existing slides (--presentationwith--slide-id/--slide-number), with Base64 suppressed in output.int_arrayflags to allow repeated integer arguments (e.g., multi-page slide-number selection).slides +screenshotquick reference and full command documentation.int_arrayparsing.