feat(slides):slide screenshot#1358
Open
ethan-zhx wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR introduces a new ChangesSlides Screenshot Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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@0dda6f5db918da7705cfbbac047cb9c80704d252🧩 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
Comment on lines
+238
to
+252
| func TestSlidesScreenshotListRequiresSelector(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | ||
|
|
||
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | ||
| "+screenshot", | ||
| "--presentation", "pres_abc", | ||
| "--as", "user", | ||
| }) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| if !strings.Contains(err.Error(), "--slide-id or --slide-number is required") { | ||
| t.Fatalf("error = %v, want missing selector error", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert typed error metadata via errs.ProblemOf, not just message substrings.
This validation error test violates the coding guideline for **/*_test.go: error-path tests must assert typed metadata (category/subtype/param) via errs.ProblemOf, not message substrings alone. Checking only the message reduces confidence that the error type, subtype, and parameter metadata are correctly populated.
✅ Recommended pattern using errs.ProblemOf
if err == nil {
t.Fatal("expected error")
}
- if !strings.Contains(err.Error(), "--slide-id or --slide-number is required") {
- t.Fatalf("error = %v, want missing selector error", err)
+ problem, ok := errs.ProblemOf(err)
+ if !ok {
+ t.Fatalf("error type = %T, want typed errs.Problem", err)
+ }
+ if problem.Category != errs.CategoryValidation {
+ t.Fatalf("category = %v, want Validation", problem.Category)
+ }
+ if problem.Subtype != errs.SubtypeInvalidArgument {
+ t.Fatalf("subtype = %v, want InvalidArgument", problem.Subtype)
}As per coding guidelines: "Error-path tests must assert typed metadata via errs.ProblemOf (category/subtype/param) and cause preservation, not message substrings alone".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestSlidesScreenshotListRequiresSelector(t *testing.T) { | |
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | |
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | |
| "+screenshot", | |
| "--presentation", "pres_abc", | |
| "--as", "user", | |
| }) | |
| if err == nil { | |
| t.Fatal("expected error") | |
| } | |
| if !strings.Contains(err.Error(), "--slide-id or --slide-number is required") { | |
| t.Fatalf("error = %v, want missing selector error", err) | |
| } | |
| } | |
| func TestSlidesScreenshotListRequiresSelector(t *testing.T) { | |
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | |
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | |
| "+screenshot", | |
| "--presentation", "pres_abc", | |
| "--as", "user", | |
| }) | |
| if err == nil { | |
| t.Fatal("expected error") | |
| } | |
| problem, ok := errs.ProblemOf(err) | |
| if !ok { | |
| t.Fatalf("error type = %T, want typed errs.Problem", err) | |
| } | |
| if problem.Category != errs.CategoryValidation { | |
| t.Fatalf("category = %v, want Validation", problem.Category) | |
| } | |
| if problem.Subtype != errs.SubtypeInvalidArgument { | |
| t.Fatalf("subtype = %v, want InvalidArgument", problem.Subtype) | |
| } | |
| } |
🤖 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_test.go` around lines 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.
Source: Coding guidelines
Comment on lines
+325
to
+340
| func TestSlidesScreenshotRenderRejectsSlideSelectors(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | ||
|
|
||
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | ||
| "+screenshot", | ||
| "--content", `<slide xmlns="http://www.larkoffice.com/sml/2.0"><data></data></slide>`, | ||
| "--slide-id", "slide_1", | ||
| "--as", "user", | ||
| }) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| if !strings.Contains(err.Error(), "--content cannot be used with --slide-id or --slide-number") { | ||
| t.Fatalf("error = %v, want content/slide selector conflict", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert typed error metadata via errs.ProblemOf, not just message substrings.
This validation error test violates the coding guideline: error-path tests must assert typed metadata (category/subtype/param) via errs.ProblemOf. The test should verify that the error is a ValidationError with the appropriate subtype and parameter metadata, not just check the message string.
✅ Recommended pattern
if err == nil {
t.Fatal("expected error")
}
- if !strings.Contains(err.Error(), "--content cannot be used with --slide-id or --slide-number") {
- t.Fatalf("error = %v, want content/slide selector conflict", err)
+ problem, ok := errs.ProblemOf(err)
+ if !ok {
+ t.Fatalf("error type = %T, want typed errs.Problem", err)
+ }
+ if problem.Category != errs.CategoryValidation {
+ t.Fatalf("category = %v, want Validation", problem.Category)
+ }
+ if problem.Subtype != errs.SubtypeInvalidArgument {
+ t.Fatalf("subtype = %v, want InvalidArgument", problem.Subtype)
}As per coding guidelines: "Error-path tests must assert typed metadata via errs.ProblemOf (category/subtype/param) and cause preservation, not message substrings alone".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestSlidesScreenshotRenderRejectsSlideSelectors(t *testing.T) { | |
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | |
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | |
| "+screenshot", | |
| "--content", `<slide xmlns="http://www.larkoffice.com/sml/2.0"><data></data></slide>`, | |
| "--slide-id", "slide_1", | |
| "--as", "user", | |
| }) | |
| if err == nil { | |
| t.Fatal("expected error") | |
| } | |
| if !strings.Contains(err.Error(), "--content cannot be used with --slide-id or --slide-number") { | |
| t.Fatalf("error = %v, want content/slide selector conflict", err) | |
| } | |
| } | |
| func TestSlidesScreenshotRenderRejectsSlideSelectors(t *testing.T) { | |
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | |
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | |
| "+screenshot", | |
| "--content", `<slide xmlns="http://www.larkoffice.com/sml/2.0"><data></data></slide>`, | |
| "--slide-id", "slide_1", | |
| "--as", "user", | |
| }) | |
| if err == nil { | |
| t.Fatal("expected error") | |
| } | |
| problem, ok := errs.ProblemOf(err) | |
| if !ok { | |
| t.Fatalf("error type = %T, want typed errs.Problem", err) | |
| } | |
| if problem.Category != errs.CategoryValidation { | |
| t.Fatalf("category = %v, want Validation", problem.Category) | |
| } | |
| if problem.Subtype != errs.SubtypeInvalidArgument { | |
| t.Fatalf("subtype = %v, want InvalidArgument", problem.Subtype) | |
| } | |
| } |
🤖 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_test.go` around lines 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.
Source: Coding guidelines
Comment on lines
+342
to
+357
| func TestSlidesScreenshotRenderRejectsListOnlyFlags(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | ||
|
|
||
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | ||
| "+screenshot", | ||
| "--content", `<slide xmlns="http://www.larkoffice.com/sml/2.0"><data></data></slide>`, | ||
| "--presentation", "pres_abc", | ||
| "--as", "user", | ||
| }) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| if !strings.Contains(err.Error(), "--presentation cannot be used with --content") { | ||
| t.Fatalf("error = %v, want presentation/content conflict", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert typed error metadata via errs.ProblemOf, not just message substrings.
This validation error test violates the coding guideline: error-path tests must assert typed metadata via errs.ProblemOf. Verify the error category, subtype, and param instead of only checking the message string.
✅ Recommended pattern
if err == nil {
t.Fatal("expected error")
}
- if !strings.Contains(err.Error(), "--presentation cannot be used with --content") {
- t.Fatalf("error = %v, want presentation/content conflict", err)
+ problem, ok := errs.ProblemOf(err)
+ if !ok {
+ t.Fatalf("error type = %T, want typed errs.Problem", err)
+ }
+ if problem.Category != errs.CategoryValidation {
+ t.Fatalf("category = %v, want Validation", problem.Category)
+ }
+ if problem.Subtype != errs.SubtypeInvalidArgument {
+ t.Fatalf("subtype = %v, want InvalidArgument", problem.Subtype)
}As per coding guidelines: "Error-path tests must assert typed metadata via errs.ProblemOf (category/subtype/param) and cause preservation, not message substrings alone".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestSlidesScreenshotRenderRejectsListOnlyFlags(t *testing.T) { | |
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | |
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | |
| "+screenshot", | |
| "--content", `<slide xmlns="http://www.larkoffice.com/sml/2.0"><data></data></slide>`, | |
| "--presentation", "pres_abc", | |
| "--as", "user", | |
| }) | |
| if err == nil { | |
| t.Fatal("expected error") | |
| } | |
| if !strings.Contains(err.Error(), "--presentation cannot be used with --content") { | |
| t.Fatalf("error = %v, want presentation/content conflict", err) | |
| } | |
| } | |
| func TestSlidesScreenshotRenderRejectsListOnlyFlags(t *testing.T) { | |
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | |
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | |
| "+screenshot", | |
| "--content", `<slide xmlns="http://www.larkoffice.com/sml/2.0"><data></data></slide>`, | |
| "--presentation", "pres_abc", | |
| "--as", "user", | |
| }) | |
| if err == nil { | |
| t.Fatal("expected error") | |
| } | |
| problem, ok := errs.ProblemOf(err) | |
| if !ok { | |
| t.Fatalf("error type = %T, want typed errs.Problem", err) | |
| } | |
| if problem.Category != errs.CategoryValidation { | |
| t.Fatalf("category = %v, want Validation", problem.Category) | |
| } | |
| if problem.Subtype != errs.SubtypeInvalidArgument { | |
| t.Fatalf("subtype = %v, want InvalidArgument", problem.Subtype) | |
| } | |
| } |
🤖 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_test.go` around lines 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.
Source: Coding guidelines
Comment on lines
+402
to
+418
| func TestSlidesScreenshotRejectsBadOutputDir(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | ||
|
|
||
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | ||
| "+screenshot", | ||
| "--presentation", "pres_abc", | ||
| "--slide-id", "slide_1", | ||
| "--output-dir", "../outside", | ||
| "--as", "user", | ||
| }) | ||
| if err == nil { | ||
| t.Fatal("expected error for unsafe output dir") | ||
| } | ||
| if !strings.Contains(err.Error(), "--output-dir invalid") { | ||
| t.Fatalf("error = %v, want output-dir validation", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert typed error metadata via errs.ProblemOf, not just message substrings.
This validation error test violates the coding guideline: error-path tests must assert typed metadata via errs.ProblemOf. For this --output-dir validation failure, the test should verify the error category, subtype, and especially the param field to confirm it identifies the problematic flag.
✅ Recommended pattern
if err == nil {
t.Fatal("expected error for unsafe output dir")
}
- if !strings.Contains(err.Error(), "--output-dir invalid") {
- t.Fatalf("error = %v, want output-dir validation", err)
+ problem, ok := errs.ProblemOf(err)
+ if !ok {
+ t.Fatalf("error type = %T, want typed errs.Problem", err)
+ }
+ if problem.Category != errs.CategoryValidation {
+ t.Fatalf("category = %v, want Validation", problem.Category)
+ }
+ if problem.Subtype != errs.SubtypeInvalidArgument {
+ t.Fatalf("subtype = %v, want InvalidArgument", problem.Subtype)
+ }
+ if problem.Param != "--output-dir" {
+ t.Fatalf("param = %v, want --output-dir", problem.Param)
}As per coding guidelines: "Error-path tests must assert typed metadata via errs.ProblemOf (category/subtype/param) and cause preservation, not message substrings alone".
🤖 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_test.go` around lines 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.
Source: Coding guidelines
Comment on lines
+420
to
+470
| func TestSlidesScreenshotNoImagesErrorIncludesRawDataAndLogID(t *testing.T) { | ||
| dir := t.TempDir() | ||
| withSlidesTestWorkingDir(t, dir) | ||
|
|
||
| f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "POST", | ||
| URL: "/open-apis/slides_ai/v1/xml_presentations/pres_abc/slide_images", | ||
| Headers: map[string][]string{ | ||
| "Content-Type": []string{"application/json"}, | ||
| "X-Tt-Logid": []string{"log-123"}, | ||
| }, | ||
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{ | ||
| "unexpected": "shape", | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| err := runSlidesShortcut(t, f, stdout, SlidesScreenshot, []string{ | ||
| "+screenshot", | ||
| "--presentation", "pres_abc", | ||
| "--slide-id", "pJJ", | ||
| "--as", "user", | ||
| }) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| var exitErr *output.ExitError | ||
| if !errors.As(err, &exitErr) { | ||
| t.Fatalf("error type = %T, want ExitError", err) | ||
| } | ||
| if exitErr.Detail == nil || exitErr.Detail.Detail == nil { | ||
| t.Fatalf("missing error detail: %+v", exitErr) | ||
| } | ||
| detail, ok := exitErr.Detail.Detail.(map[string]interface{}) | ||
| if !ok { | ||
| t.Fatalf("detail = %#v, want map", exitErr.Detail.Detail) | ||
| } | ||
| if detail["log_id"] != "log-123" { | ||
| t.Fatalf("log_id = %v, want log-123", detail["log_id"]) | ||
| } | ||
| raw, ok := detail["raw_data"].(map[string]interface{}) | ||
| if !ok { | ||
| t.Fatalf("raw_data = %#v, want map", detail["raw_data"]) | ||
| } | ||
| if raw["unexpected"] != "shape" { | ||
| t.Fatalf("raw_data.unexpected = %v, want shape", raw["unexpected"]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Also assert typed error metadata via errs.ProblemOf for the underlying error.
While this test correctly verifies the ExitError structure and detail fields (log_id, raw_data), it should also assert the underlying typed error metadata to ensure proper error classification. Per coding guidelines, error-path tests must verify typed metadata (category/subtype) via errs.ProblemOf.
✅ Add typed error assertions
After line 452, add:
if !errors.As(err, &exitErr) {
t.Fatalf("error type = %T, want ExitError", err)
}
+
+ // Verify the underlying typed error
+ problem, ok := errs.ProblemOf(err)
+ if !ok {
+ t.Fatalf("underlying error is not typed, want errs.Problem")
+ }
+ // Verify appropriate category/subtype for "no images in response" scenario
+ if problem.Category != errs.CategoryInternal {
+ t.Fatalf("category = %v, want Internal for unexpected API shape", problem.Category)
+ }
+
if exitErr.Detail == nil || exitErr.Detail.Detail == nil {
t.Fatalf("missing error detail: %+v", exitErr)
}As per coding guidelines: "Error-path tests must assert typed metadata via errs.ProblemOf (category/subtype/param) and cause preservation, not message substrings alone".
🤖 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_test.go` around lines 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).
Source: Coding guidelines
| return "", err | ||
| } | ||
| if err := vfs.MkdirAll(safeOutputDir, 0o755); err != nil { | ||
| return "", output.Errorf(output.ExitAPI, "io_error", "create output directory %s: %v", outputDir, err) |
There was a problem hiding this comment.
Use typed error for file I/O failures.
Per coding guidelines: "Use errs.NewInternalError(errs.SubtypeFileIO, ...) for local file I/O failures after path validation with validate.SafeInputPath or SafeOutputPath". Replace output.Errorf with the typed error constructor.
🔧 Suggested fix
if err := vfs.MkdirAll(safeOutputDir, 0o755); err != nil {
- return "", output.Errorf(output.ExitAPI, "io_error", "create output directory %s: %v", outputDir, err)
+ return "", errs.NewInternalError(errs.SubtypeFileIO, "create output directory %s", outputDir).WithCause(err)
}Note: Add "github.com/larksuite/cli/pkg/errs" to imports if not already present.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return "", output.Errorf(output.ExitAPI, "io_error", "create output directory %s: %v", outputDir, err) | |
| if err := vfs.MkdirAll(safeOutputDir, 0o755); err != nil { | |
| return "", errs.NewInternalError(errs.SubtypeFileIO, "create output directory %s", outputDir).WithCause(err) | |
| } |
🤖 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 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.
Source: Coding guidelines
| if os.IsExist(err) { | ||
| continue | ||
| } | ||
| return "", output.Errorf(output.ExitAPI, "io_error", "write screenshot %s: %v", path, err) |
There was a problem hiding this comment.
Use typed errors for file I/O failures.
Per coding guidelines: "Use errs.NewInternalError(errs.SubtypeFileIO, ...) for local file I/O failures after path validation with validate.SafeInputPath or SafeOutputPath". Replace all output.Errorf calls in this function with the typed error constructor and preserve underlying errors with .WithCause(err).
🔧 Suggested fix
f, err := vfs.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644)
if err != nil {
if os.IsExist(err) {
continue
}
- return "", output.Errorf(output.ExitAPI, "io_error", "write screenshot %s: %v", path, err)
+ return "", errs.NewInternalError(errs.SubtypeFileIO, "write screenshot %s", path).WithCause(err)
}
n, err := f.Write(imageBytes)
if err != nil {
_ = f.Close()
- return "", output.Errorf(output.ExitAPI, "io_error", "write screenshot %s: %v", path, err)
+ return "", errs.NewInternalError(errs.SubtypeFileIO, "write screenshot %s", path).WithCause(err)
}
if n != len(imageBytes) {
_ = f.Close()
- return "", output.Errorf(output.ExitAPI, "io_error", "write screenshot %s: wrote %d of %d bytes", path, n, len(imageBytes))
+ return "", errs.NewInternalError(errs.SubtypeFileIO, "write screenshot %s: wrote %d of %d bytes", path, n, len(imageBytes))
}
if err := f.Close(); err != nil {
- return "", output.Errorf(output.ExitAPI, "io_error", "write screenshot %s: %v", path, err)
+ return "", errs.NewInternalError(errs.SubtypeFileIO, "write screenshot %s", path).WithCause(err)
}
return path, nil
}
path := filepath.Join(outputDir, base+"."+ext)
- return "", output.Errorf(output.ExitAPI, "io_error", "write screenshot %s: too many duplicate file names", path)
+ return "", errs.NewInternalError(errs.SubtypeFileIO, "write screenshot %s: too many duplicate file names", path)
}Note: Add "github.com/larksuite/cli/pkg/errs" to imports if not already present.
Also applies to: 479-479, 483-483, 486-486, 491-491
🤖 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 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.
Source: Coding guidelines
Comment on lines
+96
to
+99
| ## 获取 slide_id | ||
|
|
||
| 不知道页面 ID，传 slide id 即可。 | ||
|
|
There was a problem hiding this comment.
Fix the selector guidance when slide_id is unknown.
Line 98 currently says “不知道页面 ID，传 slide id 即可”, which is contradictory and not actionable. It should instruct using --slide-number when slide_id is unavailable.
Suggested doc patch
## 获取 slide_id
-不知道页面 ID，传 slide id 即可。
+不知道页面 ID 时，可直接使用 `--slide-number` 选择页面截图；已知 short ID 时再用 `--slide-id`。🤖 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 `@skills/lark-slides/references/lark-slides-screenshot.md` around lines 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.
ethan-zhx
force-pushed
the
feat/slide_image
branch
from
0dda6f5 to
ed5ad8b
Compare
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
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
New Features
Documentation
Tests