feat(slides): add +replace-slide shortcut with block-level editing#516
feat(slides): add +replace-slide shortcut with block-level editing#516
Conversation
|
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 a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant CLI as CLI (Parser/Validator)
participant Wiki as Wiki Resolver
participant XML as XML Transformer
participant Slides as Slides API
participant EH as Error Enricher
Client->>CLI: invoke +replace-slide (--presentation, --slide-id, --parts)
CLI->>CLI: parse/validate flags and JSON parts
CLI->>CLI: enforce parts bounds (1-200) and action whitelist
alt presentation is wiki URL
CLI->>Wiki: GET /open-apis/wiki/v2/spaces/get_node
Wiki-->>CLI: xml_presentation_id
end
CLI->>XML: ensureXMLRootID(replacement, block_id)
XML-->>CLI: normalized replacement (id injected/overridden)
CLI->>XML: ensureShapeHasContent(xml)
XML-->>CLI: xml with <content/> injected when needed
CLI->>Slides: POST /open-apis/slides_ai/v1/xml_presentations/{id}/slide/replace (parts)
Slides-->>CLI: success {revision_id, parts_count} or error
alt error code 3350001
CLI->>EH: add parts/context-aware hint
EH-->>CLI: enriched error
CLI-->>Client: exit with enriched hint
else success
CLI-->>Client: print result (xml_presentation_id, slide_id, parts_count, ...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@aceb5252cd8bc5bcecaf22db2fd6272acfb0ff4f🧩 Skill updatenpx skills add larksuite/cli#feat/slide_update -y -g |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
shortcuts/slides/slides_replace_slide.go (1)
105-119: DryRun URL should use path segment encoding for consistency with Execute.In
Execute(line 158), the presentation ID is encoded viavalidate.EncodePathSegment(presentationID), but here inDryRunthe ID is interpolated directly. While presentation IDs typically don't contain special characters, the dry-run output should accurately reflect what will be sent.♻️ Suggested fix
dry.POST(fmt.Sprintf("/open-apis/slides_ai/v1/xml_presentations/%s/slide/replace", presentationID)). + // Note: In actual Execute, presentationID is encoded via validate.EncodePathSegment Params(query). Body(body)Alternatively, apply the same encoding:
- dry.POST(fmt.Sprintf("/open-apis/slides_ai/v1/xml_presentations/%s/slide/replace", presentationID)). + dry.POST(fmt.Sprintf("/open-apis/slides_ai/v1/xml_presentations/%s/slide/replace", validate.EncodePathSegment(presentationID))).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/slides_replace_slide.go` around lines 105 - 119, The DryRun implementation builds the POST path by interpolating presentationID directly; make it mirror Execute by path-encoding the ID using validate.EncodePathSegment(presentationID) before constructing the POST URL in DryRun (the block that calls dry.POST(fmt.Sprintf(...))). Ensure presentationID assignment/override logic (including the wiki branch) stays the same, but pass the encoded value into the POST path and still return dry.Set("parts_count", len(parts)).shortcuts/slides/slides_replace_slide_test.go (1)
533-568: Test assertion doesn't match implementation's actual hint text.The test checks that
exitErr.Detail.Hintdoes not contain"missing <content/>", butbuildSlides3350001Hintnever produces that string. The actual hints are:
"mixed block_replace+block_insert...""common causes: (1) block_id not found..."This assertion will always pass regardless of whether enrichment occurred, making it a weak test.
♻️ Suggested fix
- if strings.Contains(exitErr.Detail.Hint, "missing <content/>") { - t.Fatalf("non-3350001 error should not get slides-specific hint, got %q", exitErr.Detail.Hint) + if strings.Contains(exitErr.Detail.Hint, "common causes") || + strings.Contains(exitErr.Detail.Hint, "mixed block_replace") { + t.Fatalf("non-3350001 error should not get slides-specific hint, got %q", exitErr.Detail.Hint) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/slides_replace_slide_test.go` around lines 533 - 568, The test TestReplaceSlideNon3350001ErrorNotEnriched currently asserts that exitErr.Detail.Hint does not contain "missing <content/>", but buildSlides3350001Hint never produces that text; update the assertion to check for the actual slides-specific hints instead: ensure exitErr.Detail.Hint does not contain the substrings that buildSlides3350001Hint can produce (e.g., "mixed block_replace+block_insert" and "common causes:"), or assert Hint is empty/nil; modify the check in TestReplaceSlideNon3350001ErrorNotEnriched to look for those real substrings on exitErr.Detail.Hint (referencing exitErr.Detail.Hint, buildSlides3350001Hint, and the test function name) so the test truly verifies that non-3350001 errors are not enriched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmdutil/secheader.go`:
- Around line 24-27: The code adds a hardcoded BOE test header (HeaderTtEnv /
TtEnvValue) and injects it in BaseSecurityHeaders(), forcing all traffic into
the BOE environment; remove this temporary override by deleting TtEnvValue (or
set it nil) and stop adding HeaderTtEnv in BaseSecurityHeaders() by default, or
gate its insertion behind an explicit opt-in (e.g., an environment variable or
feature flag) so the header is only added when that flag is set; update any
tests/configs that relied on the temporary header accordingly.
In `@internal/core/types.go`:
- Around line 43-45: Revert the temporary BOE endpoint override introduced in
commit c0f7751 that changed the feishu endpoints (the Open, Accounts, MCP fields
currently set to "https://open.feishu-boe.cn", "https://accounts.feishu-boe.cn",
"https://mcp.feishu-boe.cn"); undo that commit (git revert c0f7751) or restore
the original production URLs for those fields in the same struct/constants so
production traffic is not redirected, then verify the values in the same
symbol/block are back to the expected production endpoints and run CI/tests to
confirm no regressions.
In `@QA_feat_slide_update.md`:
- Around line 133-140: Test Case 24 incorrectly expects backend error 3350001
for a mixed-action input; instead update the test to assert the CLI/client-side
validation path for mixed actions (i.e., validate that providing both
block_replace and block_insert in the same request is rejected before hitting
the backend). Modify the Case 24 assertion to check for the CLI validation error
message/status returned by the validation routine (the mixed
block_replace+block_insert check) and remove the expectation of error code
3350001.
In `@shortcuts/slides/helpers.go`:
- Around line 166-170: The xmlIdAttrRegex is too permissive and can match
attributes like data-id or xml:id; update the regex used by ensureXMLRootID
(xmlIdAttrRegex) and any other occurrences (lines ~199-209) to require an exact
attribute name "id" not preceded by name characters or ':'/ '-' (e.g., use a
negative lookbehind like (?<![A-Za-z0-9:_-])id\s*=\s*(["'])(.*?)\1) so it only
matches the standalone id attribute and also ensure the closing quote is the
same as the opening quote (use backreference \1).
- Around line 248-251: The current substring check on afterOpen using
strings.Contains(xmlFragment[m[1]:], "<content") can false-positive on tags like
<contention/>; instead perform a proper tag match (e.g. use
regexp.MustCompile(`^<content(\s|>|/)`) or search for `"<content"` followed by
whitespace, `>` or `/`) against the sliced fragment (afterOpen) before returning
xmlFragment; update the check that references afterOpen and xmlFragment to use
this stricter pattern so only actual <content> tags are detected.
In `@skills/lark-slides/references/lark-slides-edit-workflows.md`:
- Around line 9-14: The third table row currently suggests using a single
`--parts` for "换标题 + 加图", which contradicts the mixed-action restriction; update
that row to explicitly state that when performing multiple changes that mix
`block_replace` and `block_insert` you must split them into separate same-action
calls (e.g., one atomic `--parts` batch of only `block_replace` operations and a
separate batch of only `block_insert` operations), and mention that mixed
`block_replace + block_insert` must not be combined in a single `--parts`.
In `@skills/lark-slides/references/lark-slides-replace-slide.md`:
- Around line 58-59: The "限制" rule currently forbids mixed actions in one
--parts batch but the provided batch example mixes `block_replace` and
`block_insert`; either split that example into two separate CLI calls (one all
`block_replace`, one all `block_insert`) or change the rule text to explicitly
allow mixed actions and update the example to match; make the same correction
for the other occurrence where the mixed-example appears (the block around the
later example that also shows `block_replace` + `block_insert`) so text and
examples are consistent.
---
Nitpick comments:
In `@shortcuts/slides/slides_replace_slide_test.go`:
- Around line 533-568: The test TestReplaceSlideNon3350001ErrorNotEnriched
currently asserts that exitErr.Detail.Hint does not contain "missing
<content/>", but buildSlides3350001Hint never produces that text; update the
assertion to check for the actual slides-specific hints instead: ensure
exitErr.Detail.Hint does not contain the substrings that buildSlides3350001Hint
can produce (e.g., "mixed block_replace+block_insert" and "common causes:"), or
assert Hint is empty/nil; modify the check in
TestReplaceSlideNon3350001ErrorNotEnriched to look for those real substrings on
exitErr.Detail.Hint (referencing exitErr.Detail.Hint, buildSlides3350001Hint,
and the test function name) so the test truly verifies that non-3350001 errors
are not enriched.
In `@shortcuts/slides/slides_replace_slide.go`:
- Around line 105-119: The DryRun implementation builds the POST path by
interpolating presentationID directly; make it mirror Execute by path-encoding
the ID using validate.EncodePathSegment(presentationID) before constructing the
POST URL in DryRun (the block that calls dry.POST(fmt.Sprintf(...))). Ensure
presentationID assignment/override logic (including the wiki branch) stays the
same, but pass the encoded value into the POST path and still return
dry.Set("parts_count", len(parts)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f856f9b4-a92d-4fdc-9f24-362a073a93f6
📒 Files selected for processing (15)
QA_feat_slide_update.mdinternal/cmdutil/secheader.gointernal/core/types.goshortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/examples.mdskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-replace-slide.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-get.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-replace.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/slides/slides_replace_slide_test.go (1)
47-60: Consider extracting a small helper for request body decode/assert patterns.Several tests repeat
json.Unmarshal+partsshape assertions; a helper would reduce duplication and make failures more uniform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/slides_replace_slide_test.go` around lines 47 - 60, Extract a small test helper to centralize the repeated json.Unmarshal + parts assertions: create a function (e.g. decodePartsOrFail or assertDecodedParts) that takes testing.T, the stub payload (stub.CapturedBody) and expected count, performs json.Unmarshal into the same anonymous/typed struct (with Parts []struct{Action, BlockID, Replacement string `json:"..."`}), fails via t.Fatalf with a consistent message on unmarshal or wrong length, and returns the parsed parts (so tests can use the returned slice instead of repeating the decode and len check); replace the repeated block in slides_replace_slide_test.go with a call to this helper and use the first element (previously got := body.Parts[0]) from the returned slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/slides/slides_replace_slide_test.go`:
- Around line 47-60: Extract a small test helper to centralize the repeated
json.Unmarshal + parts assertions: create a function (e.g. decodePartsOrFail or
assertDecodedParts) that takes testing.T, the stub payload (stub.CapturedBody)
and expected count, performs json.Unmarshal into the same anonymous/typed struct
(with Parts []struct{Action, BlockID, Replacement string `json:"..."`}), fails
via t.Fatalf with a consistent message on unmarshal or wrong length, and returns
the parsed parts (so tests can use the returned slice instead of repeating the
decode and len check); replace the repeated block in
slides_replace_slide_test.go with a call to this helper and use the first
element (previously got := body.Parts[0]) from the returned slice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5166458c-439c-42dc-9cfe-a25862e0b1d3
📒 Files selected for processing (6)
QA_feat_slide_update.mdshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.goskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-replace-slide.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-replace.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-slides/references/lark-slides-edit-workflows.md
- skills/lark-slides/references/lark-slides-replace-slide.md
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/slides/slides_replace_slide.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/lark-slides/references/lark-slides-xml-presentation-slide-replace.md`:
- Line 171: Update the Chinese wording in the table row that currently reads
"`replacement` / `insertion` 必须是合法 XML 片段,标签闭合 + 属性引号" to use the more natural
phrasing by changing "必须是合法 XML 片段" to "必须为合法的 XML 片段", i.e. make the cell read
"`replacement` / `insertion` 必须为合法的 XML 片段,标签闭合 + 属性引号"; locate the string in
the markdown table and replace it accordingly.
- Around line 116-128: The example shows mixed actions ("block_replace" +
"block_insert") in the same "parts" payload which conflicts with the PR's
shortcut behavior (the "+replace-slide" shortcut rejects mixed actions); update
the docs to avoid misleading users by either changing the example under "多条
parts 原子执行" to use only a single action (e.g., both entries as "block_replace"
or both as "block_insert"), or add a clear note under that subsection stating
that mixed actions are only supported by the low-level API and that the
"+replace-slide" shortcut requires splitting mixed operations into separate
calls (so users know to call the shortcut twice instead of sending mixed
"parts").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd683048-4b49-44b8-859f-59f94f1ef022
📒 Files selected for processing (3)
skills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-delete.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-replace.md
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/lark-slides/references/lark-slides-xml-presentation-slide-delete.md
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #516 +/- ##
==========================================
+ Coverage 60.19% 61.78% +1.58%
==========================================
Files 390 403 +13
Lines 33433 35284 +1851
==========================================
+ Hits 20125 21800 +1675
- Misses 11426 11456 +30
- Partials 1882 2028 +146 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3415d85 to
c7a490a
Compare
Introduces `lark-cli slides +replace-slide`, a shortcut over the
native `xml_presentation.slide.replace` API for element-level editing
of existing Lark Slides pages. Callers pass a JSON array of parts and
the CLI handles URL resolution, XML hygiene, client-side validation,
and 3350001 hint enrichment.
Why a dedicated shortcut
The native API has three sharp edges every caller hits:
1. URL formats. Users have /slides/<token> or /wiki/<token> URLs, not
bare xml_presentation_id.
2. Undocumented XML hygiene. `block_replace` requires id=<block_id> on
the replacement root; <shape> requires <content/>. Missing either
returns a catch-all 3350001 with no guidance.
3. 3350001 is a catch-all on the backend with no actionable message.
Code
shortcuts/slides/slides_replace_slide.go (new)
- Flags: --presentation (bare token | /slides/ URL | /wiki/ URL),
--slide-id, --parts (JSON array, max 200), --revision-id (-1 for
current, specific number for optimistic locking), --tid,
--as user|bot.
- Validation (pre-API): [1,200] item cap; action restricted to
block_replace / block_insert (str_replace rejected); per-action
required fields (block_id for block_replace, insertion for
block_insert).
- XML hygiene:
* injects id="<block_id>" on block_replace replacement roots;
* auto-expands self-closing <shape/> and injects <content/> on
shapes for SML 2.0 compliance.
Dry-run surfaces injection errors and renders the same
path-encoded presentationID that Execute sends.
- On backend 3350001 attaches a generic common-causes checklist
(missing block_id / invalid XML / coords out of 960×540).
shortcuts/slides/helpers.go
- ensureXMLRootID: regex tightened to `(?:^|\s)id` so data-id and
xml:id are not matched as root id.
- ensureShapeHasContent: regex `<content(?:\s|/|>)` avoids false
positives like <contention/>; self-closing branch preserves
trailing siblings.
shortcuts/slides/shortcuts.go: register SlidesReplaceSlide.
Tests
- helpers_test.go: regex edge cases, id override semantics, content
auto-inject across self-closing and open-tag shapes.
- slides_replace_slide_test.go: parameter validation table, URL
resolution (slides / wiki), action mix, size boundaries,
auto-inject behavior, 3350001 hint enrichment, and a tight
negative assertion that non-3350001 errors get no slides-specific
hint (substring of the real hint must not appear).
Docs (skills/lark-slides)
- SKILL.md trimmed to a command index with pointers into references/.
- New references:
* lark-slides-replace-slide.md — flags, parts schema, auto-inject
notes, mixed-action support, 200-item cap, revision_id
semantics, error table, and a "合法根元素速查" cheatsheet for
the eight supported root elements (shape / line / polyline /
img / icon / table / td / chart) with minimal verified XML
snippets. Explicit unsupported list: video / audio / whiteboard
(these appear only as <undefined> export placeholders in SML 2.0).
* lark-slides-edit-workflows.md — recipe-style edit flows.
* lark-slides-xml-presentation-slide-get.md — native read API with
block_id extraction examples.
- Fixes across existing references:
* replace / create / delete / presentations.get: add the .data
wrapper in return-value examples, correct jq paths.
* media-upload: fix jq path .file_token → .data.file_token.
* examples.md: annotate auto-inject behavior, replace the
incorrect failed_part_index example with the actual 3350001
error shape.
Empirical corrections (BOE-verified)
- revision_id: stale-but-existing values are accepted; only values
greater than current return 3350002.
- Wrong block_id returns 3350001, not a 200 with failed_part_index.
- Mixed block_replace + block_insert in one call is supported.
- Type-mismatched block_replace (e.g. shape id with a <td>
replacement) is silently accepted by the backend and may destroy
content; 3350001 specifically signals a missing block_id.
6994590 to
aceb525
Compare
Summary
Adds
lark-cli slides +replace-slide, a shortcut for element-level editing of existing Lark Slides pages. Callers pass a JSON array ofblock_replace/block_insertparts and the CLI handles URL resolution, XML hygiene, client-side validation, and 3350001 hint enrichment over the nativexml_presentation.slide.replaceAPI.Also refactors
skills/lark-slides/:SKILL.mdbecomes a command index, with edit workflows, examples, and per-API references split into dedicated files, and a cheatsheet for supported block root elements.Why a dedicated shortcut
The native API has three sharp edges every caller hits:
/slides/<token>or/wiki/<token>URLs, not barexml_presentation_id. The shortcut parses all three and resolves wiki URLs viawiki.get_node.block_replacerequiresid=<block_id>on the replacement root;<shape>requires<content/>. Missing either returns a catch-all3350001with no guidance. The shortcut auto-injects both and surfaces the result in--dry-run.3350001is a catch-all. The shortcut attaches a concrete common-causes checklist (missingblock_id/ invalid XML / coords out of 960×540) so agents and humans have a starting point.Highlights
--presentation: bare token,/slides/<token>URL, or/wiki/<token>URL (auto-resolved viawiki.get_node).--partslength ∈ [1, 200]; action restricted toblock_replace/block_insert(str_replacerejected); per-action required fields enforced.idon replacement roots;<content/>on shapes (including self-closing<shape/>).--dry-runrenders the exact injected body and path-encodes the presentation token to match whatExecutesends.references/lark-slides-replace-slide.md:shape / line / polyline / img / icon / table / td / chart. Explicit unsupported list:video / audio / whiteboard— these only appear as<undefined>export placeholders in SML 2.0, not as writable elements.Empirical corrections to existing docs (BOE-verified)
revision_id: stale-but-existing values are accepted; only values greater than current return3350002.block_id→3350001(not a 200 withfailed_part_index).block_replace(e.g. using a shape's id with a<td>replacement) is silently accepted by the backend and may destroy content;3350001specifically signals a missingblock_id.Test Plan
block_replace:idauto-injected into replacement root; correct existing id preserved, wrong id overridden.block_insert: self-closing<shape/>auto-expanded with<content/>; omittinginsert_before_block_idappends to page end.block_replace+block_insertin a single--partsexecutes successfully.--presentationwith/wiki/URL: resolves viaget_node; non-slidesobj_typereturns a clear error.--presentationwith special-char tokens is path-encoded consistently in--dry-runpreview and the real request URL.--dry-run: shows the fully injected request body without making an API call.3350001errors get the common-causes checklist hint; non-3350001errors get no slides-specific hint.go test ./shortcuts/slides/...passes.Summary by CodeRabbit
New Features
+replace-slideshortcut for block-level slide replace/insert with client-side validation (1–200 parts), automatic root ID/ injection, wiki-URL resolution, dry-run preview, and enriched error hints.Documentation
Tests