feat(slides): add +update-slide to replace a page with the XML you hand it - #2143
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:
📝 WalkthroughWalkthroughThe PR adds ChangesWhole-slide update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant SlidesUpdateSlide
participant XMLValidator
participant SlideAPI
CLI->>SlidesUpdateSlide: submit slide ID and XML content
SlidesUpdateSlide->>XMLValidator: validate and normalize slide XML
XMLValidator-->>SlidesUpdateSlide: validated page payload
SlidesUpdateSlide->>SlideAPI: send one block_replace request
SlideAPI-->>SlidesUpdateSlide: return result or failure reason
SlidesUpdateSlide-->>CLI: output metadata or error
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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@81f21368907719acf0b2792971399467c1f319ba🧩 Skill updatenpx skills add larksuite/cli#feat/slides-update-slide-via-page-id -y -g |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
shortcuts/slides/slides_update_slide.go (1)
262-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the
ensureXMLRootIDcause inupdateSlideContent.When
ensureXMLRootIDfails, return the command-specificValidationErrorwrapped around the original error instead of discardingerr:return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "--content root <slide> is written in a form the page id cannot be attached to\...").WithCause(err).WithParam("--content")This keeps
errors.Is/errors.Aspathability while keeping the user-facing message.🤖 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_update_slide.go` around lines 262 - 270, Update the ensureXMLRootID error path in updateSlideContent to preserve err by wrapping it in the command-specific ValidationError with subtype invalid argument and parameter --content. Keep the existing user-facing message unchanged while attaching err as the cause so errors.Is/errors.As remain pathable.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_update_slide_test.go`:
- Around line 313-316: The error-path tests lack complete typed metadata
assertions. In shortcuts/slides/slides_update_slide_test.go lines 313-316,
require errors.As to succeed, remove the tt.name exclusion, and assert category,
subtype, and param via errs.ProblemOf, including param --content; in lines
177-186, retain the failed_reason message check and add errs.ProblemOf
assertions for category and subtype, while verifying the underlying cause is
preserved.
- Around line 177-186: Enhance the error assertions in the failed_reason test
around updateSlideExecute by extracting typed metadata with errs.ProblemOf(err)
and asserting the expected category, subtype, and param values, including the
failed_reason hint. Keep the existing backend-message and no-success-envelope
assertions.
In `@skills/lark-slides/SKILL.md`:
- Line 113: Update the layout lint requirement near the complete <slide> XML
submission guidance to include +update-slide alongside +create --slides,
xml_presentation.slide create, and +replace-pages. Keep the existing lint
command and other workflow references unchanged.
In `@tests/cli_e2e/slides/slides_update_slide_dryrun_test.go`:
- Around line 155-163: Update errorEnvelope to require the typed error envelope
on result.Stderr and assert result.Stdout is empty for validate-stage failures;
remove the fallback that accepts stdout. At the two call sites around the
data.api assertions, replace those ineffective envelope checks with explicit
stdout-empty assertions, and correct the nearby comment to state that the
envelope is written to stderr.
---
Nitpick comments:
In `@shortcuts/slides/slides_update_slide.go`:
- Around line 262-270: Update the ensureXMLRootID error path in
updateSlideContent to preserve err by wrapping it in the command-specific
ValidationError with subtype invalid argument and parameter --content. Keep the
existing user-facing message unchanged while attaching err as the cause so
errors.Is/errors.As remain pathable.
🪄 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: 8fc121c6-f2f1-468e-ab47-c4d196b44047
📒 Files selected for processing (10)
shortcuts/register.goshortcuts/slides/shortcuts.goshortcuts/slides/shortcuts_alias_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_update_slide.goshortcuts/slides/slides_update_slide_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-update-slide.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_update_slide_dryrun_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2143 +/- ##
==========================================
+ Coverage 75.65% 75.69% +0.03%
==========================================
Files 940 944 +4
Lines 99876 100230 +354
==========================================
+ Hits 75564 75867 +303
- Misses 18530 18564 +34
- Partials 5782 5799 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6e88409 to
1585195
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_replace_pages.go`:
- Around line 26-34: The documentation in
shortcuts/slides/slides_replace_pages.go lines 26-34 should claim preservation
only for slide_id and page order, and state that elements omitted from --content
are removed while elements without IDs are inserted. In
skills/lark-slides/references/lark-slides-edit-workflows.md lines 3-14, remove
the claim that all element IDs remain unchanged and link readers to the
whole-slide replacement behavior and deletion warning; do not imply comments or
deep links anchored to element IDs are preserved.
In `@skills/lark-slides/SKILL.md`:
- Around line 84-85: Update the wiki URL guidance sentences near the core
concepts section to include +update-slide alongside the existing +replace-slide
and +media-upload shortcuts, keeping the documented URL-resolution behavior
consistent with the new command guidance.
In `@tests/cli_e2e/slides/slides_update_slide_dryrun_test.go`:
- Around line 148-150: Update the cross-page validation assertions in the slide
update dry-run test to verify the error envelope has ok=false, type validation,
subtype invalid_argument, and param --content, matching
TestSlidesUpdateSlideRefusesElementRootDryRunE2E. Keep the existing message
assertion and rely on errorEnvelope’s stdout validation.
🪄 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: dd3acafe-b76d-4f61-b1f7-d7985919a0c0
📒 Files selected for processing (13)
shortcuts/register.goshortcuts/slides/shortcuts.goshortcuts/slides/shortcuts_alias_test.goshortcuts/slides/slides_replace_pages.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_update_slide.goshortcuts/slides/slides_update_slide_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-replace-pages.mdskills/lark-slides/references/lark-slides-update-slide.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_update_slide_dryrun_test.go
💤 Files with no reviewable changes (1)
- skills/lark-slides/references/lark-slides-replace-pages.md
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/slides/shortcuts_alias_test.go
- shortcuts/register.go
- shortcuts/slides/slides_replace_slide.go
- shortcuts/slides/shortcuts.go
- shortcuts/slides/slides_update_slide_test.go
- shortcuts/slides/slides_update_slide.go
1585195 to
2154d0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cli_e2e/slides/slides_history_workflow_test.go (1)
87-100: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd direct coverage for deck-position preservation.
This workflow creates one slide, so it cannot detect a replacement that moves the target slide. Create a second slide and assert that both slide IDs remain in their original order after the update.
As per coding guidelines, contract tests must assert the changed field or behavior directly.
🤖 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_history_workflow_test.go` around lines 87 - 100, Extend the slides history workflow test around the update command to create a second slide before updating the first, then assert the resulting deck lists both slide IDs in their original order. Keep the existing slide_id assertion and add a direct assertion against the deck-order field returned by the relevant CLI response, using the created IDs.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 `@skills/lark-slides/SKILL.md`:
- Around line 84-85: Update the “整页覆盖” guidance in SKILL.md to clarify that only
elements in --content with their original IDs retain those IDs; elements without
IDs are inserted as new elements and receive new IDs, while preserving the
existing slide_id and page-order behavior.
---
Nitpick comments:
In `@tests/cli_e2e/slides/slides_history_workflow_test.go`:
- Around line 87-100: Extend the slides history workflow test around the update
command to create a second slide before updating the first, then assert the
resulting deck lists both slide IDs in their original order. Keep the existing
slide_id assertion and add a direct assertion against the deck-order field
returned by the relevant CLI response, using the created IDs.
🪄 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: cf4f901c-5045-4b72-afc0-e67d8d388f02
📒 Files selected for processing (15)
shortcuts/register.goshortcuts/slides/shortcuts.goshortcuts/slides/shortcuts_alias_test.goshortcuts/slides/slides_replace_pages.goshortcuts/slides/slides_replace_pages_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_update_slide.goshortcuts/slides/slides_update_slide_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-replace-pages.mdskills/lark-slides/references/lark-slides-update-slide.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_history_workflow_test.gotests/cli_e2e/slides/slides_update_slide_dryrun_test.go
💤 Files with no reviewable changes (3)
- skills/lark-slides/references/lark-slides-replace-pages.md
- shortcuts/slides/slides_replace_pages_test.go
- shortcuts/slides/slides_replace_pages.go
🚧 Files skipped from review as they are similar to previous changes (9)
- shortcuts/slides/slides_replace_slide.go
- tests/cli_e2e/slides/coverage.md
- shortcuts/register.go
- shortcuts/slides/shortcuts_alias_test.go
- shortcuts/slides/shortcuts.go
- tests/cli_e2e/slides/slides_update_slide_dryrun_test.go
- shortcuts/slides/slides_update_slide.go
- skills/lark-slides/references/lark-slides-edit-workflows.md
- shortcuts/slides/slides_update_slide_test.go
2154d0b to
0cde957
Compare
b1fa292 to
72f1fff
Compare
97c309f to
5c0fc24
Compare
7e6b8c8 to
22ae5b8
Compare
Add an in-place whole-page slide update shortcut with validation, aliases, docs, unit tests, and dry-run E2E coverage. Deprecate the superseded +replace-pages: the binary keeps the command working for a deprecation window, with the replacement named in its --help description and in a `deprecated` field on every output (dry-run, validate-only and real runs), while the skill no longer routes to it. Multi-page updates now call +update-slide once per page. The XML/revision helpers it shared with +add-slide / +delete-slide move to slides_shared.go so its eventual removal cannot break them.
22ae5b8 to
81f2136
Compare
What this changes
Adds
slides +update-slide, which applies one complete<slide>XML document to an existing page in a singleblock_replacerequest. The request uses the pageslide_idasblock_id, so the page is updated in place and keeps both itsslide_idand its position in the deck.Command surface
slides +update-slideshortcut.+updateand singularslidealiases for common agent-generated spellings.--contentaliases:--xml,--slide-xml,--slide-content, and--content-xml.slides +replace-pageswhile keeping it fully working: the shortcut carries the standard GoDeprecated:marker, its--helpdescription leads with the replacement, and every output shape (dry-run, validate-only, real runs) carries adeprecatedfield naming it. The skill no longer routes to it — decision tables, command list, and its reference page are removed — so nothing steers a new caller there, but existing callers keep working for a deprecation window. An old skill copy on a new binary therefore still works, and learns about+update-slidefrom the output of its very first call.+add-slide/+delete-slidewith the shared presentation-flag contract (requiredPresentationRefFlag()), so their--presentationaccepts the same alias spellings (--token,--url, …) as every other Slides shortcut; a contract test now pins this across all of them.+replace-pagesrebuilt each page with create-then-delete, changing the page ID and every element ID while exposing a non-atomic partial-failure flow. For multi-page work, agents now call+update-slideonce per page. The XML/revision helpers it shared with+add-slide/+delete-slidemoved to a shared file, so removing the command after the window cannot break them.Behavior worth reviewing
--contentdescribes the final page state, not a patch. Elements omitted from it are deleted.<style>and<note>follow the supplied XML, which allows page background and speaker-note changes.<slide>root. Malformed XML, a bare element, trailing content, and a root ID that names a different page are rejected before any API request is built.slides +replace-slide.Tests and documentation
+update-slideround trip and assert thatslide_idis preserved.--content.+update-slidereference doc to the house skeleton shared by the sibling shortcut docs (命令 / 参数 / 成功输出 / 常见错误); scope requirements now live in the 403 row of 常见错误 like everywhere else, instead of a dedicated permissions section.+replace-pagesstays executable and each of its output shapes carries the full deprecation note.Local checks completed:
make unit-testgo vet ./...gofmt -l .go mod tidywith no module-file changesgolangci-lintwith 0 issuesgo test ./tests/cli_e2e/slides -count=1Backend dependency
This command relies on the server accepting the page ID as
block_idfor a whole-page replacement. The rollout is tracked separately. Please do not merge before that capability is available in the target environment. Until then, the default CI proof is dry-run coverage; the real API round trip remains opt-in throughLARK_SLIDES_HISTORY_E2E=1.Summary by CodeRabbit
New Features
+update-slidefor whole-slide XML replacement while preserving slide IDs and page order.slideas an alias for the slides service.Documentation
Removed
+replace-pages; use per-slide+update-slideoperations instead.