Skip to content

feat(slides): accept slide XML files in +create - #2197

Open
R0bynZhu wants to merge 1 commit into
mainfrom
feat/slides-create-slide-file-inputs
Open

feat(slides): accept slide XML files in +create#2197
R0bynZhu wants to merge 1 commit into
mainfrom
feat/slides-create-slide-file-inputs

Conversation

@R0bynZhu

@R0bynZhu R0bynZhu commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Assembling the --slides JSON array by hand is what callers keep getting wrong. A page of SML is multi-line and quote-heavy, and shell has no built-in way to JSON-escape it, so callers reached for jq -n --rawfile to build the array. In environments without jq the substitution silently became an empty string and the command ran on to create an empty deck, or the half-escaped XML reached the backend and came back as an opaque 3350001 after the presentation already existed.

Two input forms remove the escaping step:

--slides now declares Input{file, stdin}, so a finished array can be
read with --slides @deck.json or piped in with --slides -.

--slide is repeatable, takes one complete document (or @path),
and the CLI assembles the array. Repetition order is page order.

The forms are mutually exclusive: merging them would make page order depend on flag-parsing rules nobody wants to reason about.

Notes on the repeatable flag: the framework only resolves Flag.Input for single-valued string flags, so --slide resolves @path itself, through the same cmdutil.ReadInputFile the framework uses, keeping the "relative path under the current directory" rule identical. It rejects "-" outright, because a process has one stdin and that cannot mean "this occurrence" on a repeatable flag; the error names both forms that work.

Structural validation now runs on the assembled array, so both forms fail the same way, and it runs before the create call so a malformed page can no longer leave an orphaned empty presentation behind.

Also threads the source flag name through uploadSlidesPlaceholders, which previously reported +add-slide upload failures as --slides.

Docs: the create/troubleshooting references now teach the file inputs instead of the jq array-building template, and the follow-up snippet uses the CLI's own --jq instead of piping to an external jq.

Summary by CodeRabbit

  • New Features
    • Create presentations using repeated --slide XML inputs or --slides JSON from a file or stdin, preserving slide order and using local image placeholders.
  • Bug Fixes
    • Improved validation and handling for mutually exclusive inputs, missing/empty values, stdin rejection where applicable, and pre-creation malformed-slide checks (including the 10-slide limit).
    • Upload/create errors now clearly indicate which input flag caused the issue.
  • Documentation
    • Updated slide creation, troubleshooting, and examples to use --slide-based workflows and revised instructions.

@github-actions github-actions Bot added the size/L Large or sensitive change across domains or core paths label Aug 5, 2026
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds repeated --slide inputs to slides +create, supports file-based --slides JSON, validates slide sources before presentation creation, preserves flag-specific upload errors, and updates tests and documentation.

Changes

Slides create input handling

Layer / File(s) Summary
Input resolution and validation
shortcuts/slides/slides_create.go, shortcuts/slides/slides_create_test.go, tests/cli_e2e/slides/...
slides +create resolves repeated --slide inputs and --slides values. It supports files and stdin where permitted, rejects conflicting forms, enforces the 10-slide limit, and validates each <slide> before API calls.
Slide creation and placeholder uploads
shortcuts/slides/slides_create.go, shortcuts/slides/slides_add_slide.go, shortcuts/slides/slides_create_test.go
Execution uses resolved slide content, uploads local image placeholders, adds slides, records result counts and IDs, and attributes upload errors to the originating flag.
Documentation and coverage
skills/lark-slides/..., tests/cli_e2e/slides/coverage.md
Documentation and coverage describe the new input forms, validation rules, page limit, result fields, placeholder handling, recovery commands, and dry-run cases.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CLI as slides +create
  participant Source as File or stdin
  participant Upload as Placeholder upload
  participant API as Slides API

  User->>CLI: provide `--slide` or `--slides`
  CLI->>Source: read input
  Source-->>CLI: return slide XML or JSON array
  CLI->>CLI: validate source and slide XML
  CLI->>API: create presentation
  loop each resolved slide
    CLI->>Upload: upload local image placeholders
    Upload-->>CLI: return rewritten XML
    CLI->>API: add slide
    API-->>CLI: return slide ID or issue
  end
  CLI-->>User: return slide results or typed error
Loading

Possibly related PRs

  • larksuite/cli#2120: This PR also changes the slide creation and image-placeholder upload flow.

Suggested reviewers: evandance

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: accepting slide XML files in the +create command.
Description check ✅ Passed The description clearly explains the motivation, input forms, validation behavior, error handling, and documentation updates.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/slides-create-slide-file-inputs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 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.go`:
- Line 168: Add a focused test for the placeholder upload failure path in
uploadSlidesPlaceholders, using a valid placeholder setup and a failing upload
operation. Assert the returned error’s typed category and subtype, verify
ValidationError.Param is "--slide", and confirm the underlying cause is
preserved.

In `@shortcuts/slides/slides_create.go`:
- Around line 280-281: Preserve slide XML exactly as supplied: in
shortcuts/slides/slides_create.go:280-281, use a trimmed copy only for
empty-content and structure validation while retaining the original value in
slides; in shortcuts/slides/slides_create.go:300-322, trim only the selector
used to detect `@path` and -, appending literal slide values unchanged; in
shortcuts/slides/slides_create_test.go:820-857, add leading and trailing
whitespace to the fixture and assert the captured request preserves it.
- Around line 242-246: Update the mutual-exclusion validation around slideArgs
so it uses runtime.Cmd.Flags().Changed("slides") rather than slidesJSON != "" to
detect whether --slides was supplied, rejecting --slides "" together with
--slide. Add a test covering the empty --slides value combined with --slide.
- Around line 255-259: After json.Unmarshal in the --slides parsing flow, reject
a nil slides slice, including JSON null, with the same invalid-argument
validation error before any API call. Preserve valid empty-array handling if
supported, and add a no-API-call test covering --slides null.

In `@skills/lark-slides/SKILL.md`:
- Line 109: Update the critical XML preflight requirement in SKILL.md to include
slides +create --slides alongside the existing slide-submission commands.
Require its complete <slide> XML input to be saved locally and checked with
scripts/xml_text_overlap_lint.py, proceeding only when summary.error_count is 0.

In `@tests/cli_e2e/slides/slides_create_slide_inputs_dryrun_test.go`:
- Around line 136-138: Add assertions in the validation-error test for
error.subtype equal to "invalid_argument" and for result.Stdout being empty,
alongside the existing error.type, error.param, and error.message checks. Use
the existing gjson-based envelope assertions and preserve result.Stderr as the
diagnostic context.
🪄 Autofix

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: 16426d9b-5fd0-43cf-a03a-f2d8c6ac510d

📥 Commits

Reviewing files that changed from the base of the PR and between 875d20a and e837b38.

📒 Files selected for processing (8)
  • shortcuts/slides/slides_add_slide.go
  • shortcuts/slides/slides_create.go
  • shortcuts/slides/slides_create_test.go
  • skills/lark-slides/SKILL.md
  • skills/lark-slides/references/lark-slides-create.md
  • skills/lark-slides/references/troubleshooting.md
  • tests/cli_e2e/slides/coverage.md
  • tests/cli_e2e/slides/slides_create_slide_inputs_dryrun_test.go

placeholders := extractImagePlaceholderPaths([]string{slideXML})
if len(placeholders) > 0 {
tokens, uploaded, err := uploadSlidesPlaceholders(runtime, presentationID, placeholders)
tokens, uploaded, err := uploadSlidesPlaceholders(runtime, presentationID, placeholders, "--slide")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add direct coverage for the updated error parameter.

Add a test that drives a placeholder upload failure after validation. Assert the typed category and subtype, ValidationError.Param == "--slide", and cause preservation. The supplied changes do not include coverage for this changed error contract.

As per coding guidelines, every behavior change must have an accompanying test, and error-path tests must verify typed metadata and cause preservation.

🤖 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` at line 168, Add a focused test for the
placeholder upload failure path in uploadSlidesPlaceholders, using a valid
placeholder setup and a failing upload operation. Assert the returned error’s
typed category and subtype, verify ValidationError.Param is "--slide", and
confirm the underlying cause is preserved.

Source: Coding guidelines

Comment thread shortcuts/slides/slides_create.go Outdated
Comment on lines +255 to +259
if err := json.Unmarshal([]byte(slidesJSON), &slides); err != nil {
return nil, param, errs.NewValidationError(errs.SubtypeInvalidArgument, "--slides invalid JSON, must be an array of XML strings").
WithParam("--slides").
WithHint("to pass pages as XML files instead of building the array, repeat --slide @page.xml once per page")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 4 'json\.Unmarshal|slidesJSON|len\(slides\)' shortcuts/slides/slides_create.go
rg -n -C 3 --glob '*_test.go' '"null"|--slides' shortcuts/slides

Repository: larksuite/cli

Length of output: 19862


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== relevant create.go context =="
sed -n '35,275p' shortcuts/slides/slides_create.go | cat -n

echo
echo "== relevant validation tests context =="
sed -n '532,630p' shortcuts/slides/slides_create_test.go | cat -n

echo
echo "== Go json null probe =="
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT

cat > "$tmpdir/nullprober.go" <<'EOF'
package main

import (
	"encoding/json"
	"fmt"
)

type probe struct {
	Slice []string
}

func main() {
	var slides []string
	data, err := json.Marshal(slides)
	if err != nil {
		panic(err)
	}
	fmt.Println("nil-slice JSON:", string(data))

	var slides2 []string
	err = json.Unmarshal([]byte("null"), &slides2)
	if err != nil {
		fmt.Println("null unmarshal error:", err)
	} else {
		fmt.Println("null unmarshal error: nil")
	}
	fmt.Printf("null unmarshal len: %d is nil: %t\n", len(slides2), slides2 == nil)

	var p probe
	if err := json.Unmarshal([]byte("null"), &p); err != nil {
		panic(err)
	}
	fmt.Printf("null in struct slice len: %d is nil: %t\n", len(p.Slice), p.Slice == nil)

	var p2 probe
	if err := json.Unmarshal([]byte("{}"), &p2); err != nil {
		panic(err)
	}
	fmt.Printf("object in struct slice len: %d is nil: %t\n", len(p2.Slice), p2.Slice == nil)
}
EOF

cd "$tmpdir"
go run nullprober.go

Repository: larksuite/cli

Length of output: 14905


Reject JSON null for --slides.

--slides null passes because json.Unmarshal("null", &slides) leaves slides nil, which len(slides) == 0 treats as an empty presentation. After unmarshalling, reject a nil slice; add a no-API-call test for --slides null.

🤖 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 255 - 259, After
json.Unmarshal in the --slides parsing flow, reject a nil slides slice,
including JSON null, with the same invalid-argument validation error before any
API call. Preserve valid empty-array handling if supported, and add a
no-API-call test covering --slides null.

Comment on lines +280 to +281
for i, slideXML := range slides {
slides[i] = strings.TrimSpace(slideXML)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve supplied slide XML without trimming it.

strings.TrimSpace removes leading and trailing bytes from file and literal --slide content. This conflicts with the documented verbatim-file contract and changes caller input before the API request.

  • shortcuts/slides/slides_create.go#L280-L281: use a trimmed copy only to detect empty content and validate structure. Keep the original XML in slides.
  • shortcuts/slides/slides_create.go#L300-L322: use a trimmed selector only to detect @path and -. Append a literal slide value unchanged.
  • shortcuts/slides/slides_create_test.go#L820-L857: add leading and trailing whitespace to a fixture and assert the captured request preserves it.

As per coding guidelines, “When transcribing input or transforming requests, preserve values faithfully; never silently coerce unsupported inputs.”

📍 Affects 2 files
  • shortcuts/slides/slides_create.go#L280-L281 (this comment)
  • shortcuts/slides/slides_create.go#L300-L322
  • shortcuts/slides/slides_create_test.go#L820-L857
🤖 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 280 - 281, Preserve slide XML
exactly as supplied: in shortcuts/slides/slides_create.go:280-281, use a trimmed
copy only for empty-content and structure validation while retaining the
original value in slides; in shortcuts/slides/slides_create.go:300-322, trim
only the selector used to detect `@path` and -, appending literal slide values
unchanged; in shortcuts/slides/slides_create_test.go:820-857, add leading and
trailing whitespace to the fixture and assert the captured request preserves it.

Source: Coding guidelines

Comment thread skills/lark-slides/SKILL.md Outdated
**CRITICAL — 新建演示文稿或大幅改写页面时,规划 `asset_need` MUST 遵循 [asset-planning.md](references/asset-planning.md):只做元数据规划,必须有 `fallback_if_missing`,不得要求真实搜索、下载或上传素材。**

**CRITICAL — 将完整 `<slide>` XML 提交给 `slides +create --slides`、`slides +add-slide`、`xml_presentation.slide create` 或 `slides +replace-pages` 之前,MUST 先把待提交 XML 保存到本地文件并运行唯一版式准出入口 [`scripts/xml_text_overlap_lint.py`](scripts/xml_text_overlap_lint.py);`summary.error_count` 必须为 0 才能调用接口。**
**CRITICAL — 将完整 `<slide>` XML 提交给 `slides +create --slide`、`slides +add-slide`、`xml_presentation.slide create` 或 `slides +replace-pages` 之前,MUST 先把待提交 XML 保存到本地文件并运行唯一版式准出入口 [`scripts/xml_text_overlap_lint.py`](scripts/xml_text_overlap_lint.py);`summary.error_count` 必须为 0 才能调用接口。**

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Apply the XML preflight rule to --slides too.

slides +create --slides also submits complete <slide> documents. Add it to this mandatory lint requirement so the supported JSON-array input does not bypass the documented preflight workflow.

🤖 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/SKILL.md` at line 109, Update the critical XML preflight
requirement in SKILL.md to include slides +create --slides alongside the
existing slide-submission commands. Require its complete <slide> XML input to be
saved locally and checked with scripts/xml_text_overlap_lint.py, proceeding only
when summary.error_count is 0.

Comment thread tests/cli_e2e/slides/slides_create_slide_inputs_dryrun_test.go
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5781bde1e7cd00b740b5b3d7be8649276bab803a

🧩 Skill update

npx skills add larksuite/cli#feat/slides-create-slide-file-inputs -y -g

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.79310% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.00%. Comparing base (8e88492) to head (5781bde).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
shortcuts/slides/slides_create.go 88.69% 8 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2197      +/-   ##
==========================================
+ Coverage   75.89%   76.00%   +0.10%     
==========================================
  Files         962      966       +4     
  Lines      102085   102597     +512     
==========================================
+ Hits        77480    77981     +501     
+ Misses      18720    18709      -11     
- Partials     5885     5907      +22     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Assembling the --slides JSON array by hand is what callers keep getting
wrong. A page of SML is multi-line and quote-heavy, and shell has no
built-in way to JSON-escape it, so callers reached for `jq -n --rawfile`
to build the array. In environments without jq the substitution silently
became an empty string and the command ran on to create an empty deck,
or the half-escaped XML reached the backend and came back as an opaque
3350001 after the presentation already existed.

Two input forms remove the escaping step:

  --slides now declares Input{file, stdin}, so a finished array can be
  read with `--slides @deck.json` or piped in with `--slides -`.

  --slide is repeatable, takes one complete <slide> document (or @path),
  and the CLI assembles the array. Repetition order is page order.

The forms are mutually exclusive: merging them would make page order
depend on flag-parsing rules nobody wants to reason about.

Notes on the repeatable flag: the framework only resolves Flag.Input for
single-valued string flags, so --slide resolves @path itself, through
the same cmdutil.ReadInputFile the framework uses, keeping the
"relative path under the current directory" rule identical. It rejects
"-" outright, because a process has one stdin and that cannot mean "this
occurrence" on a repeatable flag; the error names both forms that work.

Structural validation now runs on the assembled array, so both forms
fail the same way, and it runs before the create call so a malformed
page can no longer leave an orphaned empty presentation behind.

Also threads the source flag name through uploadSlidesPlaceholders,
which previously reported +add-slide upload failures as --slides.

Docs: the create/troubleshooting references now teach the file inputs
instead of the jq array-building template, and the follow-up snippet
uses the CLI's own --jq instead of piping to an external jq.
@R0bynZhu
R0bynZhu force-pushed the feat/slides-create-slide-file-inputs branch from e837b38 to 5781bde Compare August 5, 2026 13:58
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
shortcuts/slides/slides_create_test.go (1)

803-812: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Decode the captured payload into a typed struct.

capturedSlideContent uses map[string]interface{} and unchecked type assertions at the JSON boundary. A missing slide.content silently becomes an empty string. Decode the request into a nested typed struct so the helper defines the expected payload shape.

As per coding guidelines, “Parse map[string]interface{} into typed structs at the boundary.”

🤖 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_test.go` around lines 803 - 812, Update
capturedSlideContent to unmarshal stub.CapturedBody into a typed struct with a
nested slide field containing content, instead of map[string]interface{} and
unchecked assertions. Keep the existing fatal handling for JSON decode failures
and return the typed slide content so malformed or missing payload fields are
not silently treated as empty strings.

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_create_test.go`:
- Around line 455-459: Strengthen the error-path assertions for the fixture in
the slide API failure test by requiring errs.ProblemOf(err) to report
CategoryAPI, a populated Subtype, and a preserved underlying API cause. Keep the
existing Hint assertion, and do not assert Param because this path exposes only
problem-level fields.
- Around line 880-882: Update the assertion around capturedSlideContent in the
slides file-input test to compare the complete captured value with the expected
decoded page content from deck.json, rather than using strings.Contains for the
<data/> marker. Keep the assertion focused on verifying that --slides `@file`
forwards the exact page content.

In `@skills/lark-slides/references/lark-slides-create.md`:
- Around line 18-19: Update the warning in the slide input documentation to
recommend file-based input without presenting it as mandatory. Ensure the
surrounding documentation continues to explicitly preserve the supported
--slides - stdin form and direct XML values for --slide, consistent with the
existing usage examples.

---

Nitpick comments:
In `@shortcuts/slides/slides_create_test.go`:
- Around line 803-812: Update capturedSlideContent to unmarshal
stub.CapturedBody into a typed struct with a nested slide field containing
content, instead of map[string]interface{} and unchecked assertions. Keep the
existing fatal handling for JSON decode failures and return the typed slide
content so malformed or missing payload fields are not silently treated as empty
strings.
🪄 Autofix

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: 12b10105-db1f-4a42-add5-c3cdfa181764

📥 Commits

Reviewing files that changed from the base of the PR and between bdb1964 and 5781bde.

📒 Files selected for processing (8)
  • shortcuts/slides/slides_add_slide.go
  • shortcuts/slides/slides_create.go
  • shortcuts/slides/slides_create_test.go
  • skills/lark-slides/SKILL.md
  • skills/lark-slides/references/lark-slides-create.md
  • skills/lark-slides/references/troubleshooting.md
  • tests/cli_e2e/slides/coverage.md
  • tests/cli_e2e/slides/slides_create_slide_inputs_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
  • shortcuts/slides/slides_create.go
  • skills/lark-slides/references/troubleshooting.md
  • shortcuts/slides/slides_add_slide.go
  • tests/cli_e2e/slides/coverage.md
  • tests/cli_e2e/slides/slides_create_slide_inputs_dryrun_test.go

Comment on lines +455 to +459
// Page 2 is a structurally valid <slide>, so it reaches the API and is
// rejected there — the case this test is about. A locally malformed page is
// now caught before the presentation is created at all, which is a different
// path with its own test.
slidesJSON := `["<slide xmlns=\"https://www.larkoffice.com/sml/2.0\"><data></data></slide>","<slide xmlns=\"https://www.larkoffice.com/sml/2.0\"><data><shape type=\"text\" height=\"-6\"/></data></slide>"]`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert API error metadata and cause preservation.

This fixture now reaches the slide API failure path. The test only checks that errs.ProblemOf(err) succeeds and that Hint contains text. It can pass if the failure loses its API classification or its wrapped cause.

Assert Category == errs.CategoryAPI, assert that Subtype is populated, and assert the API cause remains available. Do not assert Param for this API path because errs.ProblemOf returns only problem-level fields.

As per coding guidelines, “Error-path tests must assert typed metadata through errs.ProblemOf … and verify cause preservation.” Based on learnings, API errors should assert CategoryAPI and a populated 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_create_test.go` around lines 455 - 459, Strengthen
the error-path assertions for the fixture in the slide API failure test by
requiring errs.ProblemOf(err) to report CategoryAPI, a populated Subtype, and a
preserved underlying API cause. Keep the existing Hint assertion, and do not
assert Param because this path exposes only problem-level fields.

Sources: Coding guidelines, Learnings

Comment on lines +880 to +882
if got := capturedSlideContent(t, stubs[0]); !strings.Contains(got, "<data/>") {
t.Fatalf("slide content = %q, want the page from deck.json", got)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the exact page submitted from deck.json.

strings.Contains(got, "<data/>") permits altered or substituted slide content. Compare capturedSlideContent with the expected decoded page string. This directly verifies that --slides @file`` forwards the file’s page content.

Proposed test change
-	if got := capturedSlideContent(t, stubs[0]); !strings.Contains(got, "<data/>") {
-		t.Fatalf("slide content = %q, want the page from deck.json", got)
+	want := `<slide xmlns="https://www.larkoffice.com/sml/2.0"><data/></slide>`
+	if got := capturedSlideContent(t, stubs[0]); got != want {
+		t.Fatalf("slide content = %q, want %q", got, want)
 	}

As per coding guidelines, “contract tests must assert the changed field or behavior directly.”

📝 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
if got := capturedSlideContent(t, stubs[0]); !strings.Contains(got, "<data/>") {
t.Fatalf("slide content = %q, want the page from deck.json", got)
}
want := `<slide xmlns="https://www.larkoffice.com/sml/2.0"><data/></slide>`
if got := capturedSlideContent(t, stubs[0]); got != want {
t.Fatalf("slide content = %q, want %q", got, want)
}
🤖 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_test.go` around lines 880 - 882, Update the
assertion around capturedSlideContent in the slides file-input test to compare
the complete captured value with the expected decoded page content from
deck.json, rather than using strings.Contains for the <data/> marker. Keep the
assertion focused on verifying that --slides `@file` forwards the exact page
content.

Source: Coding guidelines

Comment on lines 18 to +19
> [!WARNING]
> `--slides '[...]'` 的风险点主要在 shell 参数传递,而不是单纯页数。即使只有 1 页,只要 XML 足够复杂,也建议使用两步创建法
> 页面 XML 一律走文件传入:`--slide @page-01.xml` 或 `--slides @deck.json`,不要写成命令行字面量,也不要用外部命令现拼数组

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Do not state that file input is mandatory.

The warning says “页面 XML 一律走文件传入”, but --slides - supports stdin and --slide supports direct XML values. This conflicts with Lines 36 and 90-104. Reword the warning as a file-input recommendation and keep the supported stdin and direct-value forms documented.

🤖 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-create.md` around lines 18 - 19,
Update the warning in the slide input documentation to recommend file-based
input without presenting it as mandatory. Ensure the surrounding documentation
continues to explicitly preserve the supported --slides - stdin form and direct
XML values for --slide, consistent with the existing usage examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant