feat(slides): accept slide XML files in +create - #2197
Conversation
github-actions
Bot
added
the
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds repeated ChangesSlides create input handling
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
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (8)
shortcuts/slides/slides_add_slide.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/troubleshooting.mdtests/cli_e2e/slides/coverage.mdtests/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") |
There was a problem hiding this comment.
📐 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
| for i, slideXML := range slides { | ||
| slides[i] = strings.TrimSpace(slideXML) |
There was a problem hiding this comment.
🎯 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 inslides.shortcuts/slides/slides_create.go#L300-L322: use a trimmed selector only to detect@pathand-. 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-L322shortcuts/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
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@bacd0d3c32d8cf1ad8da42a83f7fc63497f3c0d7🧩 Skill updatenpx skills add larksuite/cli#feat/slides-create-slide-file-inputs -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2197 +/- ##
==========================================
+ Coverage 76.08% 76.22% +0.14%
==========================================
Files 983 986 +3
Lines 103429 104326 +897
==========================================
+ Hits 78692 79522 +830
- Misses 18752 18757 +5
- Partials 5985 6047 +62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e837b38 to
5781bde
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
shortcuts/slides/slides_create_test.go (1)
803-812: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecode the captured payload into a typed struct.
capturedSlideContentusesmap[string]interface{}and unchecked type assertions at the JSON boundary. A missingslide.contentsilently 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
📒 Files selected for processing (8)
shortcuts/slides/slides_add_slide.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/troubleshooting.mdtests/cli_e2e/slides/coverage.mdtests/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
5781bde to
b7fe37b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
b7fe37b to
bab092d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
bab092d to
278c2bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-create.md`:
- Line 92: Update the --slide value documentation to explicitly state that "-"
is invalid for --slide and that stdin input is supported only via --slides -.
Preserve the existing descriptions of direct slide XML and `@path` values.
🪄 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: 4af9082b-6c86-468d-9853-eedd3be3b883
📒 Files selected for processing (8)
shortcuts/slides/slides_add_slide.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/troubleshooting.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_create_slide_inputs_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/slides/slides_add_slide.go
- tests/cli_e2e/slides/coverage.md
- skills/lark-slides/references/troubleshooting.md
- shortcuts/slides/slides_create_test.go
- tests/cli_e2e/slides/slides_create_slide_inputs_dryrun_test.go
- shortcuts/slides/slides_create.go
|
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. |
278c2bb to
3b5b9ba
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
3b5b9ba to
fd515e3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
c4fb9a2 to
c07b737
Compare
ethan-zhx
left a comment
ethan-zhx
left a comment
There was a problem hiding this comment.
Request changes. Reviewed current head c07b737840e9064056ae406195585a2c4161b38e against merge-base 9759167542f279a7ac131c166a9033dc637bb96c, checked current review threads, and used static verification only. Remote CI is green; I did not run local tests.
P1 - --slides null creates a blank deck instead of rejecting invalid input
Location: shortcuts/slides/slides_create.go:261
Trigger: lark-cli slides +create --title Deck --slides null --as user
Problem: json.Unmarshal("null", &slides) succeeds and leaves slides == nil; the later len(slides) == 0 path skips slide validation and execution proceeds to create the presentation. That reintroduces the PR core failure mode: invalid or failed slide input can be reported as a successful empty deck.
Example: input --slides null -> current result: POST /xml_presentations runs and no slide.create call is planned or executed.
If fixed: --slides null returns a typed validation/invalid_argument error before any API call, while --slides [] can remain the explicit empty-array behavior.
If not fixed: generators that marshal a nil []string as null leave users with orphaned blank presentations and a success envelope.
Smallest safe repair: after unmarshalling --slides, reject slides == nil with the same --slides invalid JSON, must be an array of XML strings validation shape, and add a no-API-call regression for --slides null.
P2 - Skill preflight rule omits the still-supported --slides path
Location: skills/lark-slides/SKILL.md:109
Trigger: an agent follows the skill and uses documented slides +create --slides @deck.json or --slides -.
Problem: the critical lint gate now names slides +create --slide, +add-slide, and +replace-pages, but the create reference still documents --slides as a supported way to submit complete <slide> XML. That lets the JSON-array create path bypass the mandatory xml_text_overlap_lint.py workflow in the skill instructions.
Example: input slides +create --slides @deck.json -> current skill guidance does not require local lint before calling the API.
If fixed: both --slide and --slides users get the same preflight requirement before page XML is submitted.
If not fixed: agents can create decks through a supported path without the lint gate that catches overlap, overflow, and structural slide issues.
Smallest safe repair: update line 109 to include slides +create --slides alongside slides +create --slide.
| if slides[i] == "" { | ||
| return nil, param, errs.NewValidationError(errs.SubtypeInvalidArgument, "%s: page %d is empty", param, i+1).WithParam(param) | ||
| } | ||
| if err := validateCompleteSlideXML(slides[i]); err != nil { |
There was a problem hiding this comment.
[P2] Reject unsupported XML declarations before creating the deck
validateCompleteSlideXML accepts processing instructions, so a new --slide @page.xml file with a common <?xml ...?> prolog passes this preflight. The repository’s own xml_text_overlap_lint.py classifies that input as sxsd_unsupported_declaration because the Slides SXSD write format does not support declarations. Execute will therefore create the presentation first and only then fail the slide request, recreating the orphaned-empty-deck outcome this preflight is intended to prevent. Please reject (or safely remove) a leading XML declaration before Step 1 and add a regression test.
| if err != nil { | ||
| return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide: %v", err).WithParam("--slide").WithCause(err) | ||
| } | ||
| slides = append(slides, string(data)) |
There was a problem hiding this comment.
[P2] Strip the UTF-8 BOM from --slide files
This manual @file path bypasses resolveInputFlags, but unlike the framework path it appends ReadInputFile bytes without removing a leading UTF-8 BOM. A page saved by a BOM-writing editor therefore reaches validateCompleteSlideXML with U+FEFF before <slide> and is rejected as “non-whitespace text outside root”; meanwhile --slides @deck.json already strips the BOM. Apply the same leading-BOM normalization here and add a file-input regression test.
| **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` 或 `slides +replace-pages` 之前，MUST 先把待提交 XML 保存到本地文件并运行唯一版式准出入口 [`scripts/xml_text_overlap_lint.py`](scripts/xml_text_overlap_lint.py)；`summary.error_count` 必须为 0 才能调用接口。** |
There was a problem hiding this comment.
这里漏了 slides +create --slides。同一 PR 的创建文档仍指导 Agent 使用 --slides @deck.json / --slides -，但这条 MUST 门禁只列了 --slide；Agent 按该合法路径执行时，会跳过 xml_text_overlap_lint.py，而 CLI 的结构校验无法发现重叠、越界、空白页等版式问题。
请同时覆盖 slides +create --slide 和 slides +create --slides，并明确 --slides 数组中的每个 XML 元素都要先保存并通过 lint（summary.error_count == 0）后才能调用接口。
c07b737 to
462370e
Compare
462370e to
a738140
Compare
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.
Three inputs the first round of review found still slipping through are
now rejected or normalized before the create call. `--slides null` is
valid JSON for a slice, so it parsed without error and left the array
nil, which read as "no pages given" and produced the blank deck
reported as success that the empty-value check exists to prevent. An
`<?xml ...?>` prolog is well-formed XML, so the parser accepted it and
only the backend rejected it, with 4001000 buildSnNode, after the
presentation already existed; it is now caught in the shared slide
validator, which covers +add-slide and +replace-pages too. And a
leading UTF-8 BOM made `--slide @page.xml` reject a file that
`--slides @deck.json` accepted, because the framework strips it for
Input flags and the repeatable flag resolved @path itself;
StripUTF8BOM is exported so both paths normalize the same way.
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.
The lint gate in SKILL.md now names `slides +create` as a whole rather
than only its --slide form.
a738140 to
bacd0d3
Compare
4 participants
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 --rawfileto 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.jsonor 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
--slideXML inputs or--slidesJSON from a file or stdin, preserving slide order and supporting local image placeholders.