feat(slides): add +add-slide and +delete-slide shortcuts - #2120
Open
R0bynZhu wants to merge 1 commit into
Open
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:
📝 WalkthroughWalkthroughAdds ChangesSlides page management
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant WikiAPI
participant MediaAPI
participant SlidesAPI
CLI->>WikiAPI: resolve --presentation wiki URL
CLI->>MediaAPI: upload @ image placeholders
CLI->>SlidesAPI: POST rewritten slide XML
SlidesAPI-->>CLI: return slide_id and result fields
sequenceDiagram
participant CLI
participant WikiAPI
participant SlidesAPI
CLI->>WikiAPI: resolve --presentation wiki URL
CLI->>SlidesAPI: DELETE slide with slide_id and revision_id
SlidesAPI-->>CLI: return deleted result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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@735e988e991657b4e1dfd1be781d7aede4a28719🧩 Skill updatenpx skills add larksuite/cli#feat/slides-slide-add-delete-shortcuts -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: 5
🤖 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/helpers.go`:
- Around line 166-172: Update validateImagePlaceholderFiles to distinguish
missing files from permission or other Stat failures before calling
slidesInputStatError. Use the existing path/error classification to apply “file
not found” only when the path truly does not exist; pass a generic stat-failure
message for other errors while preserving the underlying error details.
In `@shortcuts/slides/slides_add_slide_test.go`:
- Around line 205-281: Update TestAddSlideRejectsNonSlideRoot,
TestAddSlideRejectsEmptySlide, and TestAddSlideMissingImageFailsBeforeAnyCall to
inspect typed metadata through errs.ProblemOf, asserting the expected category,
subtype, and --slide parameter. Replace the message-only error checks with
cause-preservation assertions: verify the non-slide-root validation retains its
underlying cause and the missing-image error wraps the underlying Stat failure;
retain only message checks needed for behavior not represented by typed
metadata.
In `@shortcuts/slides/slides_delete_slide_test.go`:
- Around line 143-146: Update the error-path assertions in the slide deletion
tests, including the additional indicated case, to verify expected Category and
Subtype through errs.ProblemOf while retaining errors.As for
ValidationError.Param. Keep the existing parameter checks and message
assertions, adding typed metadata validation without reading Param from
errs.ProblemOf.
In `@skills/lark-slides/references/xml-schema-quick-ref.md`:
- Around line 235-240: Update the nearby img example’s `src` placeholder from
`file_token_or_url` to a name representing the supported file token or
local-path placeholder, removing any implication that external URLs are valid
while preserving the surrounding XML example.
In `@tests/cli_e2e/slides/slides_slide_add_delete_dryrun_test.go`:
- Around line 21-149: The existing TestSlidesAddSlideDryRunE2E and
TestSlidesDeleteSlideDryRunE2E tests cover only request generation; add a
credential-backed live E2E test that creates a presentation, adds a slide,
verifies the slide through the API, deletes it, and cleans up the created
presentation. Reuse the existing slides E2E helpers and ensure cleanup runs even
when an assertion or intermediate API call fails.
🪄 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: 3d4302de-6ccf-42ff-90d0-21b76b7452a1
📒 Files selected for processing (18)
shortcuts/slides/helpers.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_add_slide.goshortcuts/slides/slides_add_slide_test.goshortcuts/slides/slides_create.goshortcuts/slides/slides_delete_slide.goshortcuts/slides/slides_delete_slide_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-add-slide.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-delete-slide.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-delete.mdskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/xml-schema-quick-ref.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_slide_add_delete_dryrun_test.go
💤 Files with no reviewable changes (1)
- skills/lark-slides/references/troubleshooting.md
Comment on lines
+143
to
+146
| var ve *errs.ValidationError | ||
| if !errors.As(err, &ve) || ve.Param != "--slide-id" { | ||
| t.Fatalf("want *errs.ValidationError on --slide-id, got %v", err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert typed metadata for error paths.
These tests only check a concrete type/param or message text. Assert the expected Category and Subtype with errs.ProblemOf; retain errors.As for ValidationError.Param.
As per coding guidelines, error-path tests must assert typed metadata. Based on learnings, errs.ProblemOf does not expose Param; read it through errors.As.
Also applies to: 209-214
🤖 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_delete_slide_test.go` around lines 143 - 146, Update
the error-path assertions in the slide deletion tests, including the additional
indicated case, to verify expected Category and Subtype through errs.ProblemOf
while retaining errors.As for ValidationError.Param. Keep the existing parameter
checks and message assertions, adding typed metadata validation without reading
Param from errs.ProblemOf.
Sources: Coding guidelines, Learnings
Comment on lines
+21
to
+149
| func TestSlidesAddSlideDryRunE2E(t *testing.T) { | ||
| setSlidesDryRunEnv(t) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| t.Cleanup(cancel) | ||
|
|
||
| const slideXML = `<slide xmlns="http://www.larkoffice.com/sml/2.0"><data></data></slide>` | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| args []string | ||
| assertion func(t *testing.T, stdout string) | ||
| }{ | ||
| { | ||
| name: "append to end", | ||
| args: []string{ | ||
| "slides", "+add-slide", | ||
| "--presentation", "presAddDryRun", | ||
| "--slide", slideXML, | ||
| "--dry-run", | ||
| }, | ||
| assertion: func(t *testing.T, stdout string) { | ||
| require.Equal(t, slideXML, gjson.Get(stdout, "data.api.0.body.slide.content").String(), stdout) | ||
| // Appending is expressed by omitting before_slide_id: an empty | ||
| // string is rejected by the backend as an unknown slide. | ||
| require.False(t, gjson.Get(stdout, "data.api.0.body.before_slide_id").Exists(), stdout) | ||
| require.Equal(t, int64(-1), gjson.Get(stdout, "data.api.0.params.revision_id").Int(), stdout) | ||
| require.False(t, gjson.Get(stdout, "data.api.0.params.tid").Exists(), stdout) | ||
| require.Equal(t, int64(0), gjson.Get(stdout, "data.images_to_upload").Int(), stdout) | ||
| }, | ||
| }, | ||
| { | ||
| name: "insert before a page with locking", | ||
| args: []string{ | ||
| "slides", "+add-slide", | ||
| "--presentation", "presAddDryRun", | ||
| "--slide", slideXML, | ||
| "--before-slide-id", "slide_target", | ||
| "--revision-id", "12", | ||
| "--tid", "tx_e2e", | ||
| "--dry-run", | ||
| }, | ||
| assertion: func(t *testing.T, stdout string) { | ||
| require.Equal(t, "slide_target", gjson.Get(stdout, "data.api.0.body.before_slide_id").String(), stdout) | ||
| require.Equal(t, int64(12), gjson.Get(stdout, "data.api.0.params.revision_id").Int(), stdout) | ||
| require.Equal(t, "tx_e2e", gjson.Get(stdout, "data.api.0.params.tid").String(), stdout) | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: tt.args, | ||
| DefaultAs: "bot", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
|
|
||
| require.Equal(t, "POST", gjson.Get(result.Stdout, "data.api.0.method").String(), result.Stdout) | ||
| require.Equal(t, | ||
| "/open-apis/slides_ai/v1/xml_presentations/presAddDryRun/slide", | ||
| gjson.Get(result.Stdout, "data.api.0.url").String(), | ||
| result.Stdout, | ||
| ) | ||
| tt.assertion(t, result.Stdout) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestSlidesDeleteSlideDryRunE2E covers the delete request shape and, more | ||
| // importantly, pins that the shortcut runs without --yes. The raw | ||
| // xml_presentation.slide.delete command is high-risk-write and would exit 10 | ||
| // with confirmation_required here; the shortcut is Risk "write" on purpose. | ||
| func TestSlidesDeleteSlideDryRunE2E(t *testing.T) { | ||
| setSlidesDryRunEnv(t) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| t.Cleanup(cancel) | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{ | ||
| "slides", "+delete-slide", | ||
| "--presentation", "presDeleteDryRun", | ||
| "--slide-id", "slide_gone", | ||
| "--tid", "tx_e2e", | ||
| "--dry-run", | ||
| }, | ||
| DefaultAs: "bot", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
|
|
||
| require.Equal(t, "DELETE", gjson.Get(result.Stdout, "data.api.0.method").String(), result.Stdout) | ||
| require.Equal(t, | ||
| "/open-apis/slides_ai/v1/xml_presentations/presDeleteDryRun/slide", | ||
| gjson.Get(result.Stdout, "data.api.0.url").String(), | ||
| result.Stdout, | ||
| ) | ||
| require.Equal(t, "slide_gone", gjson.Get(result.Stdout, "data.api.0.params.slide_id").String(), result.Stdout) | ||
| require.Equal(t, int64(-1), gjson.Get(result.Stdout, "data.api.0.params.revision_id").Int(), result.Stdout) | ||
| require.Equal(t, "tx_e2e", gjson.Get(result.Stdout, "data.api.0.params.tid").String(), result.Stdout) | ||
| } | ||
|
|
||
| // TestSlidesDeleteSlideWikiDryRunE2E proves the wiki URL is resolved as a | ||
| // declared first step instead of being sent to the slides API as a token. | ||
| func TestSlidesDeleteSlideWikiDryRunE2E(t *testing.T) { | ||
| setSlidesDryRunEnv(t) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| t.Cleanup(cancel) | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{ | ||
| "slides", "+delete-slide", | ||
| "--presentation", "https://example.feishu.cn/wiki/wikcnE2ETOKEN", | ||
| "--slide-id", "slide_gone", | ||
| "--dry-run", | ||
| }, | ||
| DefaultAs: "bot", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
|
|
||
| require.Equal(t, "GET", gjson.Get(result.Stdout, "data.api.0.method").String(), result.Stdout) | ||
| require.Equal(t, "/open-apis/wiki/v2/spaces/get_node", gjson.Get(result.Stdout, "data.api.0.url").String(), result.Stdout) | ||
| require.Equal(t, "wikcnE2ETOKEN", gjson.Get(result.Stdout, "data.api.0.params.token").String(), result.Stdout) | ||
| require.Equal(t, "DELETE", gjson.Get(result.Stdout, "data.api.1.method").String(), result.Stdout) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add a live add/delete workflow test.
These dry-run tests do not exercise the write endpoints. Add a credential-backed, self-contained create → add → verify → delete → cleanup flow so the new destructive behavior is validated against the API.
As per coding guidelines, new flows under tests/cli_e2e/ require live E2E coverage with create/use/cleanup.
🤖 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_slide_add_delete_dryrun_test.go` around lines 21
- 149, The existing TestSlidesAddSlideDryRunE2E and
TestSlidesDeleteSlideDryRunE2E tests cover only request generation; add a
credential-backed live E2E test that creates a presentation, adds a slide,
verifies the slide through the API, deletes it, and cleans up the created
presentation. Reuse the existing slides E2E helpers and ensure cleanup runs even
when an assertion or intermediate API call fails.
Source: Coding guidelines
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2120 +/- ##
==========================================
+ Coverage 75.19% 75.43% +0.24%
==========================================
Files 914 926 +12
Lines 96789 98214 +1425
==========================================
+ Hits 72778 74087 +1309
- Misses 18413 18485 +72
- Partials 5598 5642 +44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
R0bynZhu
force-pushed
the
feat/slides-slide-add-delete-shortcuts
branch
from
July 30, 2026 13:21
333b542 to
8e2ee4c
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/helpers_test.go`:
- Around line 426-448: Update assertValidationProblem to accept an expected
error instead of the boolean wantCause, and assert that the wrapped cause
matches it using errors.Is. Adjust every call site to pass the relevant sentinel
or expected error, such as fs.ErrNotExist, while preserving the existing typed
metadata assertions.
In `@shortcuts/slides/slides_delete_slide.go`:
- Around line 146-163: Update deleteSlideID and deleteSlideQuery to preserve raw
user-supplied slide-id and tid values in requests. Use a trimmed slide-id only
for the empty-value validation, return the original runtime.Str("slide-id"), and
check tid for emptiness without trimming the value stored in query["tid"].
In `@skills/lark-slides/references/lark-slides-create.md`:
- Around line 15-16: Resolve the conflicting complex-XML guidance in the
creation workflow: make the `jq --rawfile` example explicitly apply only to
intentional one-step `+create --slides` usage, while preserving the two-step
`+create` followed by `+add-slide` recommendation for complex XML. Update the
relevant guidance near the `+create --slides` example without changing the
existing append/insert behavior.
In `@skills/lark-slides/SKILL.md`:
- Line 108: Update the pre-submission workflow around the CRITICAL XML
submission requirement so only xml_text_overlap_lint.py runs before invoking
slides +create, slides +add-slide, xml_presentation.slide create, or slides
+replace-pages; retain the requirement that summary.error_count is 0. Move
warning-related screenshot verification to after creation or update the existing
post-creation workflow to require screenshots and readback there, unless a
documented local renderer is provided for pre-submit verification.
🪄 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: 7231638e-31d3-4170-8065-36ee51073555
📒 Files selected for processing (22)
shortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_add_slide.goshortcuts/slides/slides_add_slide_test.goshortcuts/slides/slides_create.goshortcuts/slides/slides_delete_slide.goshortcuts/slides/slides_delete_slide_test.goshortcuts/slides/slides_errors.goshortcuts/slides/slides_errors_test.goshortcuts/slides/slides_media_upload.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-add-slide.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-delete-slide.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-delete.mdskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/xml-schema-quick-ref.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_slide_add_delete_dryrun_test.go
💤 Files with no reviewable changes (1)
- skills/lark-slides/references/troubleshooting.md
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/lark-slides/references/lark-slides-media-upload.md
- skills/lark-slides/references/xml-schema-quick-ref.md
Comment on lines
+146
to
+163
| func deleteSlideID(runtime *common.RuntimeContext) (string, error) { | ||
| id := strings.TrimSpace(runtime.Str("slide-id")) | ||
| if id == "" { | ||
| return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty").WithParam("--slide-id") | ||
| } | ||
| return id, nil | ||
| } | ||
|
|
||
| // deleteSlideQuery builds the query params shared by dry-run and execute. | ||
| func deleteSlideQuery(runtime *common.RuntimeContext, slideID string) map[string]interface{} { | ||
| query := map[string]interface{}{ | ||
| "slide_id": slideID, | ||
| "revision_id": runtime.Int("revision-id"), | ||
| } | ||
| if tid := strings.TrimSpace(runtime.Str("tid")); tid != "" { | ||
| query["tid"] = tid | ||
| } | ||
| return query |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not silently normalize opaque identifiers.
Trimming --slide-id or --tid changes user-supplied values; " slide_123 " can delete slide_123 rather than fail or be sent faithfully. Use trimming only to detect an empty slide-id, then retain the raw values in the request.
Proposed fix
func deleteSlideID(runtime *common.RuntimeContext) (string, error) {
- id := strings.TrimSpace(runtime.Str("slide-id"))
- if id == "" {
+ id := runtime.Str("slide-id")
+ if strings.TrimSpace(id) == "" {
return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty").WithParam("--slide-id")
}
return id, nil
}
@@
- if tid := strings.TrimSpace(runtime.Str("tid")); tid != "" {
+ if tid := runtime.Str("tid"); tid != "" {
query["tid"] = tid
}📝 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 deleteSlideID(runtime *common.RuntimeContext) (string, error) { | |
| id := strings.TrimSpace(runtime.Str("slide-id")) | |
| if id == "" { | |
| return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty").WithParam("--slide-id") | |
| } | |
| return id, nil | |
| } | |
| // deleteSlideQuery builds the query params shared by dry-run and execute. | |
| func deleteSlideQuery(runtime *common.RuntimeContext, slideID string) map[string]interface{} { | |
| query := map[string]interface{}{ | |
| "slide_id": slideID, | |
| "revision_id": runtime.Int("revision-id"), | |
| } | |
| if tid := strings.TrimSpace(runtime.Str("tid")); tid != "" { | |
| query["tid"] = tid | |
| } | |
| return query | |
| func deleteSlideID(runtime *common.RuntimeContext) (string, error) { | |
| id := runtime.Str("slide-id") | |
| if strings.TrimSpace(id) == "" { | |
| return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty").WithParam("--slide-id") | |
| } | |
| return id, nil | |
| } | |
| // deleteSlideQuery builds the query params shared by dry-run and execute. | |
| func deleteSlideQuery(runtime *common.RuntimeContext, slideID string) map[string]interface{} { | |
| query := map[string]interface{}{ | |
| "slide_id": slideID, | |
| "revision_id": runtime.Int("revision-id"), | |
| } | |
| if tid := runtime.Str("tid"); tid != "" { | |
| query["tid"] = tid | |
| } | |
| return query | |
| } |
🤖 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_delete_slide.go` around lines 146 - 163, Update
deleteSlideID and deleteSlideQuery to preserve raw user-supplied slide-id and
tid values in requests. Use a trimmed slide-id only for the empty-value
validation, return the original runtime.Str("slide-id"), and check tid for
emptiness without trimming the value stored in query["tid"].
Source: Coding guidelines
R0bynZhu
force-pushed
the
feat/slides-slide-add-delete-shortcuts
branch
from
July 30, 2026 13:31
8e2ee4c to
9a69581
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/slides/slides_create.go (1)
253-274: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
uploadSlidesPlaceholdershardcodes--slidesas the typedParam, but it's now shared with+add-slide(flag--slide).Lines 258 and 261 always attribute Stat/IsRegular failures to
--slides.shortcuts/slides/slides_add_slide.goExecute calls this same function (line 169) for the--slideflag with no way to override the param. If a placeholder file is removed/altered betweenValidateandExecute(or any other reused caller emerges), the resultingValidationError.Paramwill incorrectly read--slidesfor an+add-slideinvocation — contradicting the explicit "agents route on the typed Param" design documented inslides_add_slide.go.Thread the flag name through as a parameter instead of hardcoding it.
🐛 Proposed fix
-func uploadSlidesPlaceholders(runtime *common.RuntimeContext, presentationID string, paths []string) (map[string]string, int, error) { +func uploadSlidesPlaceholders(runtime *common.RuntimeContext, param, presentationID string, paths []string) (map[string]string, int, error) { tokens := make(map[string]string, len(paths)) for i, path := range paths { stat, err := runtime.FileIO().Stat(path) if err != nil { - return tokens, i, slidesInputStatError(err, "--slides", fmt.Sprintf("@%s", path)) + return tokens, i, slidesInputStatError(err, param, fmt.Sprintf("@%s", path)) } if !stat.Mode().IsRegular() { - return tokens, i, errs.NewValidationError(errs.SubtypeInvalidArgument, "@%s: must be a regular file", path).WithParam("--slides") + return tokens, i, errs.NewValidationError(errs.SubtypeInvalidArgument, "@%s: must be a regular file", path).WithParam(param) }And update both call sites:
uploadSlidesPlaceholders(runtime, "--slides", presentationID, placeholders)in this file,uploadSlidesPlaceholders(runtime, "--slide", presentationID, placeholders)inshortcuts/slides/slides_add_slide.go.🤖 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_create.go` around lines 253 - 274, Update uploadSlidesPlaceholders to accept the flag name as a parameter and use it when creating Stat and regular-file ValidationErrors instead of hardcoding "--slides". Pass "--slides" from its existing caller and "--slide" from the +add-slide caller in Execute, preserving the existing validation behavior and typed Param attribution.
♻️ Duplicate comments (1)
shortcuts/slides/slides_delete_slide.go (1)
146-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrimming mutates the transmitted slide-id/tid instead of just validating emptiness.
id := strings.TrimSpace(runtime.Str("slide-id"))(Line 147) andtid := strings.TrimSpace(runtime.Str("tid"))(Line 160) both use the trimmed value in the request, not just to detect blankness." slide_123 "would deleteslide_123rather than failing or being sent verbatim.🐛 Proposed fix to preserve raw values
func deleteSlideID(runtime *common.RuntimeContext) (string, error) { - id := strings.TrimSpace(runtime.Str("slide-id")) - if id == "" { + id := runtime.Str("slide-id") + if strings.TrimSpace(id) == "" { return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty").WithParam("--slide-id") } return id, nil } @@ - if tid := strings.TrimSpace(runtime.Str("tid")); tid != "" { + if tid := runtime.Str("tid"); strings.TrimSpace(tid) != "" { query["tid"] = tid }As per coding guidelines, "never silently coerce unsupported inputs, ignore unhonored options, default missing identities, or discard writes."
🤖 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_delete_slide.go` around lines 146 - 164, Update deleteSlideID and deleteSlideQuery to preserve the raw slide-id and tid values in the transmitted request while still rejecting a slide-id that is empty after trimming. Use trimmed values only for emptiness validation, return the original runtime.Str("slide-id"), and add tid based on validation without replacing its raw value.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_delete_slide_test.go`:
- Around line 142-143: Update the validation-error assertions at both call sites
of assertValidationProblem in the slide deletion tests to inspect errs.ProblemOf
metadata, explicitly verifying the expected Category, Subtype, and "--slide-id"
parameter, while also asserting the original cause is preserved. If
assertValidationProblem does not provide these checks, extend or replace it so
every affected test performs the required typed metadata and cause assertions.
In `@tests/cli_e2e/slides/slides_slide_add_delete_workflow_test.go`:
- Around line 121-124: Ensure each ordering assertion in the slide add/delete
workflow first verifies both compared body markers with require.Contains. Update
the assertions around bodyFirst/bodyAppended and the analogous later comparison
so strings.Index is only evaluated after both substrings are confirmed present,
preserving the existing ordering checks.
---
Outside diff comments:
In `@shortcuts/slides/slides_create.go`:
- Around line 253-274: Update uploadSlidesPlaceholders to accept the flag name
as a parameter and use it when creating Stat and regular-file ValidationErrors
instead of hardcoding "--slides". Pass "--slides" from its existing caller and
"--slide" from the +add-slide caller in Execute, preserving the existing
validation behavior and typed Param attribution.
---
Duplicate comments:
In `@shortcuts/slides/slides_delete_slide.go`:
- Around line 146-164: Update deleteSlideID and deleteSlideQuery to preserve the
raw slide-id and tid values in the transmitted request while still rejecting a
slide-id that is empty after trimming. Use trimmed values only for emptiness
validation, return the original runtime.Str("slide-id"), and add tid based on
validation without replacing its raw 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: 4238c143-0c99-402e-a435-3c07182d04c2
📒 Files selected for processing (23)
shortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_add_slide.goshortcuts/slides/slides_add_slide_test.goshortcuts/slides/slides_create.goshortcuts/slides/slides_delete_slide.goshortcuts/slides/slides_delete_slide_test.goshortcuts/slides/slides_errors.goshortcuts/slides/slides_errors_test.goshortcuts/slides/slides_media_upload.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-add-slide.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-delete-slide.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-delete.mdskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/xml-schema-quick-ref.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_slide_add_delete_dryrun_test.gotests/cli_e2e/slides/slides_slide_add_delete_workflow_test.go
💤 Files with no reviewable changes (1)
- skills/lark-slides/references/troubleshooting.md
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/slides/slides_media_upload.go
- skills/lark-slides/references/xml-schema-quick-ref.md
- skills/lark-slides/references/lark-slides-media-upload.md
- tests/cli_e2e/slides/coverage.md
Comment on lines
+142
to
+143
| assertValidationProblem(t, err, "--slide-id", false) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Confirm assertValidationProblem asserts typed Category/Subtype, not just Param.
Per coding guidelines, error-path tests must assert errs.ProblemOf metadata (category, subtype, param) and cause preservation, not just param/message. assertValidationProblem is not defined in this file, so its coverage of Category/Subtype can't be confirmed from the given context. This mirrors a past review comment raised on this exact call site.
Also applies to: 209-214.
#!/bin/bash
# Locate assertValidationProblem's definition to confirm it checks Category/Subtype via errs.ProblemOf.
rg -n -A 20 'func assertValidationProblem' --type=go🤖 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_delete_slide_test.go` around lines 142 - 143, Update
the validation-error assertions at both call sites of assertValidationProblem in
the slide deletion tests to inspect errs.ProblemOf metadata, explicitly
verifying the expected Category, Subtype, and "--slide-id" parameter, while also
asserting the original cause is preserved. If assertValidationProblem does not
provide these checks, extend or replace it so every affected test performs the
required typed metadata and cause assertions.
Source: Coding guidelines
R0bynZhu
force-pushed
the
feat/slides-slide-add-delete-shortcuts
branch
from
July 30, 2026 13:53
9a69581 to
b31c209
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/slides/slides_delete_slide.go (1)
146-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't silently trim opaque
--slide-id/--tidvalues before sending them.
strings.TrimSpaceis applied to the value stored/returned, not just to the emptiness check, so" slide_123 "silently becomesslide_123in the outgoing DELETE request. As per coding guidelines,**/*.gocode must "preserve values faithfully; never silently coerce unsupported inputs... Return a typed validation error when a requested behavior cannot be honored." Trim only for the blank check, keep the raw value in the request.🐛 Proposed fix
func deleteSlideID(runtime *common.RuntimeContext) (string, error) { - id := strings.TrimSpace(runtime.Str("slide-id")) - if id == "" { + id := runtime.Str("slide-id") + if strings.TrimSpace(id) == "" { return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty").WithParam("--slide-id") } return id, nil } @@ - if tid := strings.TrimSpace(runtime.Str("tid")); tid != "" { + if tid := runtime.Str("tid"); strings.TrimSpace(tid) != "" { query["tid"] = tid }🤖 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_delete_slide.go` around lines 146 - 164, The deleteSlideID and deleteSlideQuery helpers must preserve raw opaque --slide-id and --tid values when constructing the DELETE request. Use trimmed copies only to determine whether either input is blank, return the existing typed validation error for an empty slide ID, and store the original untrimmed values in the returned ID and query.Source: Coding guidelines
🧹 Nitpick comments (1)
shortcuts/slides/slides_add_slide.go (1)
187-194: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRename the shared error enrichment helper to avoid replace-slide naming.
enrichSlidesReplaceErroronly attachesslides3350001Hint, but that constant is a replace-slide checklist aboutblock_id,block_replace/block_insert, andinsert_before_block_id; for +add-slide it can attach add-slide-specific content such asno page was addedand image-upload failure details. Rename/relocate this helper and constants so add-slide failures cannot inherit replace-only troubleshooting text.🤖 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_add_slide.go` around lines 187 - 194, The add-slide error path must stop using the replace-slide-specific enrichSlidesReplaceError helper and slides3350001Hint. Rename or relocate the shared enrichment logic and constants, then provide add-slide-specific error enrichment covering “no page was added” and image-upload failure details while keeping replace-slide troubleshooting isolated to the replace flow; update the call site around the placeholders handling accordingly.
🤖 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 `@skills/lark-slides/references/lark-slides-xml-presentation-slide-create.md`:
- Around line 171-173: Update the command reference in the local-image upload
guidance to use the registered space-separated spelling “xml_presentation.slide
create” consistently, including any related wording that could be copied
directly. Preserve the existing upload sequence and shortcut references.
---
Duplicate comments:
In `@shortcuts/slides/slides_delete_slide.go`:
- Around line 146-164: The deleteSlideID and deleteSlideQuery helpers must
preserve raw opaque --slide-id and --tid values when constructing the DELETE
request. Use trimmed copies only to determine whether either input is blank,
return the existing typed validation error for an empty slide ID, and store the
original untrimmed values in the returned ID and query.
---
Nitpick comments:
In `@shortcuts/slides/slides_add_slide.go`:
- Around line 187-194: The add-slide error path must stop using the
replace-slide-specific enrichSlidesReplaceError helper and slides3350001Hint.
Rename or relocate the shared enrichment logic and constants, then provide
add-slide-specific error enrichment covering “no page was added” and
image-upload failure details while keeping replace-slide troubleshooting
isolated to the replace flow; update the call site around the placeholders
handling accordingly.
🪄 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: 726669d1-444f-452c-b55a-6d480f520f6a
📒 Files selected for processing (23)
shortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_add_slide.goshortcuts/slides/slides_add_slide_test.goshortcuts/slides/slides_create.goshortcuts/slides/slides_delete_slide.goshortcuts/slides/slides_delete_slide_test.goshortcuts/slides/slides_errors.goshortcuts/slides/slides_errors_test.goshortcuts/slides/slides_media_upload.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-add-slide.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-delete-slide.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-delete.mdskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/xml-schema-quick-ref.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_slide_add_delete_dryrun_test.gotests/cli_e2e/slides/slides_slide_add_delete_workflow_test.go
💤 Files with no reviewable changes (1)
- skills/lark-slides/references/troubleshooting.md
🚧 Files skipped from review as they are similar to previous changes (3)
- skills/lark-slides/references/lark-slides-media-upload.md
- tests/cli_e2e/slides/coverage.md
- skills/lark-slides/references/xml-schema-quick-ref.md
Comment on lines
+171
to
+173
| > **本地图片必须先上传**：`xml_presentation.slide.create` 不识别 `@./local.png` 占位符，直接调本接口时必须先用 [`slides +media-upload`](lark-slides-media-upload.md) 拿到 `file_token`，再写进 `<img src="<file_token>">`。 | ||
| > | ||
| > 如果是从零开始建带图 PPT，**强烈建议改用 [`slides +create --slides '[...]'`](lark-slides-create.md#本地图片path-占位符)** 一步搞定（自动上传 + 替换 token）。 | ||
| > 占位符是 shortcut 的能力：给已有 PPT 加带图新页用 [`+add-slide`](lark-slides-add-slide.md)，从零建带图 PPT 用 [`+create --slides`](lark-slides-create.md#本地图片path-占位符)，都会自动上传并替换 token。 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the registered command spelling consistently.
Line 171 writes xml_presentation.slide.create, but the documented command is xml_presentation.slide create. Change this to the space-separated command form or describe it as the native API operation to avoid copy/paste failures. Based on learnings, the registered command is the singular xml_presentation.slide create.
🤖 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-xml-presentation-slide-create.md`
around lines 171 - 173, Update the command reference in the local-image upload
guidance to use the registered space-separated spelling “xml_presentation.slide
create” consistently, including any related wording that could be copied
directly. Preserve the existing upload sequence and shortcut references.
Source: Learnings
Add two single-page slide shortcuts on top of the raw xml_presentation.slide create/delete APIs. slides +add-slide appends or inserts one page into an existing presentation. It accepts --presentation as a token, a /slides/ URL or a /wiki/ URL (resolved via wiki.spaces.get_node and checked for obj_type=slides), takes the page XML through --slide as a literal, @file or stdin so the document never has to be escaped into JSON and then into the shell, and auto-uploads <img src="@./local.png"> placeholders, replacing them with the returned file_token. Omitting --before-slide-id appends to the end; the field is dropped from the body rather than sent empty, which the backend rejects as an unknown slide. slides +delete-slide removes one page by slide_id with the same --presentation resolution. It is deliberately Risk "write" rather than the raw command's high-risk-write, so it does not require --yes: it targets a single explicit page and the deck keeps its version history. Both take one page at a time so that batching stays an explicit loop and every call has an unambiguous outcome. The image placeholder validation used by +create is extracted into a shared helper so both commands fail before any API call when a referenced file is missing, is not a regular file or exceeds the 20 MB upload limit. Covered by unit tests and by dry-run e2e tests through the built binary, which is the only layer that proves a full <slide> document survives flag parsing intact. Reference docs are added for both commands and the existing slides skill docs now route to them.
R0bynZhu
force-pushed
the
feat/slides-slide-add-delete-shortcuts
branch
from
July 30, 2026 14:09
b31c209 to
735e988
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
shortcuts/slides/slides_delete_slide.go (1)
146-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't silently normalize opaque
--slide-id/--tidvalues.
strings.TrimSpaceonruntime.Str("slide-id")/runtime.Str("tid")mutates the value actually sent to the API. Use trimming only to detect emptiness, then send the raw flag value.As per coding guidelines: "never silently coerce unsupported inputs, ignore unhonored options, default missing identities, or discard writes."
🐛 Proposed fix
func deleteSlideID(runtime *common.RuntimeContext) (string, error) { - id := strings.TrimSpace(runtime.Str("slide-id")) - if id == "" { + id := runtime.Str("slide-id") + if strings.TrimSpace(id) == "" { return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty").WithParam("--slide-id") } return id, nil } @@ - if tid := strings.TrimSpace(runtime.Str("tid")); tid != "" { + if tid := runtime.Str("tid"); strings.TrimSpace(tid) != "" { query["tid"] = tid }🤖 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_delete_slide.go` around lines 146 - 164, The deleteSlideID and deleteSlideQuery helpers currently send trimmed opaque identifiers. Use trimmed values only for emptiness checks, but return and place the original raw runtime.Str values in the API query, including non-empty tid, without silently normalizing whitespace.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_add_slide_test.go`:
- Around line 521-560: Strengthen the error-path assertions in
TestAddSlideReportsUploadedImagesWhenPageFails and the corresponding test around
the second reported range: after errs.ProblemOf(err), assert the expected
Category and Subtype values, and validate the required Param and preserved cause
according to the typed error contract. Keep the existing uploaded-image hint
assertion, but do not rely on it as the only verification.
In `@skills/lark-slides/references/lark-slides-delete-slide.md`:
- Line 5: 更新 lark-slides 删除幻灯片文档中的参数说明，将“ID 是普通 flag”和“Risk 是
write”改为更自然明确的中文，表述为 ID 使用普通 flag 而非 --params JSON 串，并说明风险级别为 write 且不需要 --yes。
---
Duplicate comments:
In `@shortcuts/slides/slides_delete_slide.go`:
- Around line 146-164: The deleteSlideID and deleteSlideQuery helpers currently
send trimmed opaque identifiers. Use trimmed values only for emptiness checks,
but return and place the original raw runtime.Str values in the API query,
including non-empty tid, without silently normalizing whitespace.
🪄 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: 8e393a46-0971-4bf7-aba4-d609eb064a08
📒 Files selected for processing (23)
shortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_add_slide.goshortcuts/slides/slides_add_slide_test.goshortcuts/slides/slides_create.goshortcuts/slides/slides_delete_slide.goshortcuts/slides/slides_delete_slide_test.goshortcuts/slides/slides_errors.goshortcuts/slides/slides_errors_test.goshortcuts/slides/slides_media_upload.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-add-slide.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-delete-slide.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-delete.mdskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/xml-schema-quick-ref.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_slide_add_delete_dryrun_test.gotests/cli_e2e/slides/slides_slide_add_delete_workflow_test.go
💤 Files with no reviewable changes (1)
- skills/lark-slides/references/troubleshooting.md
🚧 Files skipped from review as they are similar to previous changes (3)
- skills/lark-slides/references/lark-slides-media-upload.md
- tests/cli_e2e/slides/coverage.md
- skills/lark-slides/references/xml-schema-quick-ref.md
Comment on lines
+521
to
+560
| func TestAddSlideReportsUploadedImagesWhenPageFails(t *testing.T) { | ||
| dir := t.TempDir() | ||
| if err := os.WriteFile(filepath.Join(dir, "chart.png"), []byte("png-bytes"), 0o600); err != nil { | ||
| t.Fatalf("write fixture: %v", err) | ||
| } | ||
| withSlidesTestWorkingDir(t, dir) | ||
|
|
||
| f, stdout, _, reg := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "POST", | ||
| URL: "/open-apis/drive/v1/medias/upload_all", | ||
| Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{"file_token": "tok_chart"}}, | ||
| }) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "POST", | ||
| URL: "/open-apis/slides_ai/v1/xml_presentations/pres_img/slide", | ||
| Body: map[string]interface{}{"code": 3350001, "msg": "invalid slide xml"}, | ||
| }) | ||
|
|
||
| slideXML := `<slide xmlns="http://www.larkoffice.com/sml/2.0"><data>` + | ||
| `<img src="@./chart.png" topLeftX="10" topLeftY="10" width="100" height="100"/>` + | ||
| `</data></slide>` | ||
|
|
||
| err := runSlidesShortcut(t, f, stdout, SlidesAddSlide, []string{ | ||
| "+add-slide", | ||
| "--presentation", "pres_img", | ||
| "--slide", slideXML, | ||
| "--as", "user", | ||
| }) | ||
| if err == nil { | ||
| t.Fatal("expected the backend rejection to surface") | ||
| } | ||
| problem, ok := errs.ProblemOf(err) | ||
| if !ok { | ||
| t.Fatalf("err = %v, want typed problem metadata", err) | ||
| } | ||
| if !strings.Contains(problem.Hint, "1 image(s) were uploaded before the page failed") { | ||
| t.Fatalf("Hint = %q, want the already-uploaded count", problem.Hint) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert typed Category/Subtype, not just problem.Hint.
Both tests fetch errs.ProblemOf(err) and only check ok plus a Hint substring; neither asserts Category/Subtype. A future regression that changes the error's classification (e.g., from a typed API error to an untyped wrap) would still pass these tests as long as the hint string survives.
✅ Example addition
problem, ok := errs.ProblemOf(err)
if !ok {
t.Fatalf("err = %v, want typed problem metadata", err)
}
+ if problem.Category != errs.CategoryAPI {
+ t.Fatalf("Category = %q, want %q", problem.Category, errs.CategoryAPI)
+ }
if !strings.Contains(problem.Hint, "1 image(s) were uploaded before the page failed") {As per path instructions: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param) and verify cause preservation rather than relying only on message substrings."
Also applies to: 565-601
🤖 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_add_slide_test.go` around lines 521 - 560, Strengthen
the error-path assertions in TestAddSlideReportsUploadedImagesWhenPageFails and
the corresponding test around the second reported range: after
errs.ProblemOf(err), assert the expected Category and Subtype values, and
validate the required Param and preserved cause according to the typed error
contract. Keep the existing uploaded-image hint assertion, but do not rely on it
as the only verification.
Source: Path instructions
|
|
||
| 从演示文稿删除**一页**，按 `slide_id` 指定。只改一页里的局部内容用 [`+replace-slide`](lark-slides-replace-slide.md)，不要删了重建。 | ||
|
|
||
| 相比原生 [`xml_presentation.slide delete`](lark-slides-xml-presentation-slide-delete.md)：`--presentation` 接受 token / `/slides/` URL / `/wiki/` URL，ID 是普通 flag 而不是 `--params` JSON 串；Risk 是 `write`，不需要 `--yes`。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the risk/flag wording.
ID 是普通 flag and Risk 是 write are awkward in Chinese. Prefer wording such as: ID 使用普通 flag，而不是 --params JSON 串；风险级别为 write，不需要 --yes。
🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: 您不可用“是”来表达个形容词。大部分人都平常用“很”来代替“是”。您是不是想表达"很普通"？
Context: ...oken / /slides/ URL / /wiki/ URL，ID 是普通 flag 而不是 --params JSON 串；Risk 是 `writ...
(SHI_ADHECTIVE_ERROR)
🤖 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-delete-slide.md` at line 5, 更新
lark-slides 删除幻灯片文档中的参数说明，将“ID 是普通 flag”和“Risk 是 write”改为更自然明确的中文，表述为 ID 使用普通
flag 而非 --params JSON 串，并说明风险级别为 write 且不需要 --yes。
Source: Linters/SAST tools
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.
Add two single-page slide shortcuts on top of the raw xml_presentation.slide create/delete APIs.
slides +add-slide appends or inserts one page into an existing presentation. It accepts --presentation as a token, a /slides/ URL or a /wiki/ URL (resolved via wiki.spaces.get_node and checked for obj_type=slides), takes the page XML through --slide as a literal, @file or stdin so the document never has to be escaped into JSON and then into the shell, and auto-uploads
placeholders, replacing them with the returned file_token. Omitting --before-slide-id appends to the end; the field is dropped from the body rather than sent empty, which the backend rejects as an unknown slide.
slides +delete-slide removes one page by slide_id with the same --presentation resolution. It is deliberately Risk "write" rather than the raw command's high-risk-write, so it does not require --yes: it targets a single explicit page and the deck keeps its version history.
Both take one page at a time so that batching stays an explicit loop and every call has an unambiguous outcome.
The image placeholder validation used by +create is extracted into a shared helper so both commands fail before any API call when a referenced file is missing, is not a regular file or exceeds the 20 MB upload limit.
Covered by unit tests and by dry-run e2e tests through the built binary, which is the only layer that proves a full document survives flag parsing intact. Reference docs are added for both commands and the existing slides skill docs now route to them.
Summary by CodeRabbit
slides +add-slideto append/insert a single<slide>with--before-slide-id, and image placeholders (<img src="@...">) automatically validated and uploaded with tokens substituted.slides +delete-slideto delete one slide byslide_id, supporting token,/slides/URLs, and/wiki/URLs (with dry-run planning).@image placeholder paths (missing/unsafe/non-regular/size limit) and clearer--slide/--file-scoped error details, including retry progress hints.+add-slide/+delete-slideand refreshed placeholder guidance.