feat(slides): add +update-slide to apply a page of XML by diffing it - #2131
Open
tianyouskrrr wants to merge 1 commit into
Open
feat(slides): add +update-slide to apply a page of XML by diffing it#2131tianyouskrrr wants to merge 1 commit into
tianyouskrrr wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesSlide update workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant UpdateShortcut
participant SlideAPI
participant ReplacementAPI
User->>UpdateShortcut: provide presentation, slide ID, and XML
UpdateShortcut->>SlideAPI: read current slide XML
SlideAPI-->>UpdateShortcut: return current slide
UpdateShortcut->>UpdateShortcut: validate XML and generate diff parts
UpdateShortcut->>ReplacementAPI: submit replacement request
ReplacementAPI-->>UpdateShortcut: return revision and statistics
UpdateShortcut-->>User: return update result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 3
🧹 Nitpick comments (1)
shortcuts/slides/shortcuts_alias_test.go (1)
69-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a coverage counter so this regression test cannot become vacuous.
The loop asserts nothing if no non-whole-page shortcut declares
--content. Today onlySlidesScreenshotreaches the assertion. If that flag is renamed or moved, the test still passes and the scoping guard disappears silently.TestShortcutsAttachFlagAliasesalready uses acountguard for the same reason.♻️ Proposed guard
func TestContentAliasesStayOffOtherShortcuts(t *testing.T) { wholePage := map[string]bool{"+update-slide": true, "+update": true} + checked := 0 for _, shortcut := range Shortcuts() { if wholePage[shortcut.Command] || !declaresFlag(shortcut.Flags, "content") { continue } if shortcut.PostMount == nil { continue } + checked++ cmd := &cobra.Command{Use: shortcut.Command} cmd.Flags().String("content", "", "content") cmd.Flags().String("presentation", "", "presentation reference") shortcut.PostMount(cmd) for _, alias := range contentFlagAliases { if err := cmd.Flags().Parse([]string{"--" + alias, "x"}); err == nil { t.Errorf("%s resolved --%s to --content; content aliases must be scoped to the whole-page commands", shortcut.Command, alias) } } } + if checked == 0 { + t.Fatal("expected at least one non-whole-page slides shortcut with --content") + } }🤖 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/shortcuts_alias_test.go` around lines 69 - 89, Add a coverage counter to TestContentAliasesStayOffOtherShortcuts, incrementing it whenever a non-whole-page shortcut declares the content flag and reaches the alias assertions, then fail the test unless at least one such shortcut was checked. Follow the existing count-guard pattern used by TestShortcutsAttachFlagAliases.
🤖 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 339-345: Strengthen error-path assertions in
shortcuts/slides/slides_update_slide_test.go at lines 339-345 for the
text_content and unclosed_tag malformed-XML cases by using errs.ProblemOf to
verify category, subtype, and param, and by confirming the cause attached by
prepareSlideUpdateXML is preserved; at lines 257-263 assert CategoryValidation
and SubtypeInvalidArgument alongside the message check; at lines 641-650 assert
CategoryAPI and a non-empty Subtype alongside Code and Hint.
In `@skills/lark-slides/references/lark-slides-update-slide.md`:
- Around line 20-47: Update
skills/lark-slides/references/lark-slides-update-slide.md lines 20-47 to add
scripts/xml_text_overlap_lint.py after XML edits, require reviewing warnings via
screenshot, and block +update-slide submission when summary.error_count is
non-zero. Update skills/lark-slides/SKILL.md line 85 to include +update-slide in
the mandatory pre-submit validation rule or link this workflow to the complete
preflight instructions.
- Around line 9-13: Align the +replace-slide risk guidance with its XML-based
behavior: update the comparison table in
skills/lark-slides/references/lark-slides-update-slide.md (lines 9-13), the
related guidance at lines 87-101, and the corresponding wording in
skills/lark-slides/SKILL.md (lines 113-115) to clarify that unchanged blocks are
preserved but metadata on replaced or inserted blocks may be discarded, or
otherwise remove the claim that it is risk-free.
---
Nitpick comments:
In `@shortcuts/slides/shortcuts_alias_test.go`:
- Around line 69-89: Add a coverage counter to
TestContentAliasesStayOffOtherShortcuts, incrementing it whenever a
non-whole-page shortcut declares the content flag and reaches the alias
assertions, then fail the test unless at least one such shortcut was checked.
Follow the existing count-guard pattern used by TestShortcutsAttachFlagAliases.
🪄 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: c4c77131-b24f-4ef2-b76d-a6a51ae5f94d
📒 Files selected for processing (8)
shortcuts/slides/helpers.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.md
Comment on lines
+339
to
+345
| var ve *errs.ValidationError | ||
| if !errors.As(err, &ve) { | ||
| t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) | ||
| } | ||
| if ve.Param != tt.wantParam { | ||
| t.Fatalf("error param = %q, want %q (err=%v)", ve.Param, tt.wantParam, err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Error-path tests assert messages and params, not typed error metadata. Three error-path tests in this file check only substrings, Param, Code, or Hint. None asserts Category and Subtype, and none verifies the cause that prepareSlideUpdateXML attaches with WithCause. Reverting the classification or dropping the cause would not fail any test.
shortcuts/slides/slides_update_slide_test.go#L339-L345: addCategory/Subtypeassertions and a cause check for the malformed-XML cases (text_content,unclosed_tag).shortcuts/slides/slides_update_slide_test.go#L257-L263: addCategory == errs.CategoryValidationandSubtype == errs.SubtypeInvalidArgumentassertions next to the message check.shortcuts/slides/slides_update_slide_test.go#L641-L650: addCategory == errs.CategoryAPIand a non-emptySubtypeassertion next to theCodeandHintchecks.
As per coding guidelines: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param) and verify cause preservation rather than relying only on message substrings."
📍 Affects 1 file
shortcuts/slides/slides_update_slide_test.go#L339-L345(this comment)shortcuts/slides/slides_update_slide_test.go#L257-L263shortcuts/slides/slides_update_slide_test.go#L641-L650
🤖 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_test.go` around lines 339 - 345,
Strengthen error-path assertions in shortcuts/slides/slides_update_slide_test.go
at lines 339-345 for the text_content and unclosed_tag malformed-XML cases by
using errs.ProblemOf to verify category, subtype, and param, and by confirming
the cause attached by prepareSlideUpdateXML is preserved; at lines 257-263
assert CategoryValidation and SubtypeInvalidArgument alongside the message
check; at lines 641-650 assert CategoryAPI and a non-empty Subtype alongside
Code and Hint.
Source: Coding guidelines
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@307e753b3f7e12b336096725d8a20719abb012f2🧩 Skill updatenpx skills add larksuite/cli#feat/slides-update-shortcut -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2131 +/- ##
==========================================
+ Coverage 75.44% 75.54% +0.09%
==========================================
Files 924 930 +6
Lines 98297 99087 +790
==========================================
+ Hits 74163 74853 +690
- Misses 18494 18554 +60
- Partials 5640 5680 +40 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tianyouskrrr
force-pushed
the
feat/slides-update-shortcut
branch
from
July 31, 2026 07:33
8eac7c5 to
afe1830
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/cli_e2e/slides/slides_update_slide_workflow_test.go (1)
50-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
encoding/jsoninstead of a hand-rolled encoder.jsonArrayescapes only". It does not escape\or control characters. The current fixtures contain neither, so the output is valid today. A future fixture with a backslash produces invalid JSON and an opaque failure.♻️ Proposed refactor
- jsonArray := func(xmls ...string) string { - quoted := make([]string, 0, len(xmls)) - for _, xml := range xmls { - quoted = append(quoted, `"`+strings.ReplaceAll(xml, `"`, `\"`)+`"`) - } - return "[" + strings.Join(quoted, ",") + "]" - } + jsonArray := func(xmls ...string) string { + encoded, err := json.Marshal(xmls) + require.NoError(t, err) + return string(encoded) + }Add
"encoding/json"to the import block.🤖 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_update_slide_workflow_test.go` around lines 50 - 56, Update the jsonArray helper to serialize each XML string with encoding/json rather than manually escaping only quotation marks; add the encoding/json import and preserve the existing array formatting and fixture behavior.tests/cli_e2e/slides/slides_update_slide_dryrun_test.go (1)
165-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the typed validation envelope, not only a substring. This is a Validate-stage rejection. The established convention is exit code 2, the typed JSON error envelope on stderr, and an empty stdout. Assert
2for the exit code and parseerror.type,error.subtype, anderror.paramfrom stderr. A substring check on+replace-slidepasses even if the classification regresses.♻️ Proposed strengthening
- require.NotEqual(t, 0, result.ExitCode, - "an element root must not be accepted\nstdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr) - // The error envelope goes to stderr; it must name the command that does - // handle element-level edits. - require.Contains(t, result.Stderr, "+replace-slide", - "the error should route the caller to the element-level command\nstderr:\n%s", result.Stderr) + result.AssertExitCode(t, 2) + require.Empty(t, result.Stdout, "stdout stays reserved for program data\nstderr:\n%s", result.Stderr) + require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String(), result.Stderr) + require.NotEmpty(t, gjson.Get(result.Stderr, "error.subtype").String(), result.Stderr) + require.Equal(t, "--content", gjson.Get(result.Stderr, "error.param").String(), result.Stderr) + // The error must name the command that does handle element-level edits. + require.Contains(t, gjson.Get(result.Stderr, "error.message").String(), "+replace-slide", + "the error should route the caller to the element-level command\nstderr:\n%s", result.Stderr)Based on learnings: "Validate-stage failures must exit with code 2, write the typed JSON validation envelope to result.Stderr, and leave result.Stdout empty so stdout remains reserved for program data. Parse and assert error.type, error.subtype, error.param, and error.message from stderr." As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam)".🤖 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_update_slide_dryrun_test.go` around lines 165 - 171, Strengthen the validation-failure assertions in the affected dry-run test by requiring exit code 2 and empty result.Stdout. Parse result.Stderr with errs.ProblemOf and assert the typed error category/type, subtype, param, and message, including the element-level command guidance instead of relying only on a +replace-slide substring.Sources: Coding guidelines, Learnings
🤖 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.go`:
- Around line 102-220: Add focused tests in slides_update_slide_test.go for
updateSlideValidate and updateSlideDryRun covering invalid --presentation
parsing, missing wiki:node:read scope, missing --slide-id, and ensureXMLRootID
failures from prepareSlideUpdateXML. Assert typed error metadata with
errs.ProblemOf(...) and the expected Param values, while preserving existing API
rejection coverage.
In `@skills/lark-slides/SKILL.md`:
- Line 315: Update the wiki URL resolution shortcut lists in the documentation
to include +update-slide alongside +replace-slide and +media-upload, so wiki
presentation URLs are shown as automatically resolved for the SlidesUpdateSlide
operation.
In `@tests/cli_e2e/slides/coverage.md`:
- Line 19: Update the permission note for slides_update_slide_workflow_test.go
to include the drive delete permission required by the cleanup command,
alongside the existing slides:presentation scopes; leave the dry-run credentials
note unchanged.
In `@tests/cli_e2e/slides/slides_update_slide_dryrun_test.go`:
- Around line 155-166: Capture the error returned by clie2e.RunCmd in the
dry-run test, require it to be nil before accessing result, and then retain the
existing exit-code assertion. Follow the pattern used by the other tests in this
file so a failed CLI launch is reported without dereferencing a nil result.
---
Nitpick comments:
In `@tests/cli_e2e/slides/slides_update_slide_dryrun_test.go`:
- Around line 165-171: Strengthen the validation-failure assertions in the
affected dry-run test by requiring exit code 2 and empty result.Stdout. Parse
result.Stderr with errs.ProblemOf and assert the typed error category/type,
subtype, param, and message, including the element-level command guidance
instead of relying only on a +replace-slide substring.
In `@tests/cli_e2e/slides/slides_update_slide_workflow_test.go`:
- Around line 50-56: Update the jsonArray helper to serialize each XML string
with encoding/json rather than manually escaping only quotation marks; add the
encoding/json import and preserve the existing array formatting and fixture
behavior.
🪄 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: 0c041e9b-a4a7-4304-bed6-e0d4fd753932
📒 Files selected for processing (11)
shortcuts/slides/helpers.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.gotests/cli_e2e/slides/slides_update_slide_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/slides/helpers.go
- shortcuts/slides/shortcuts_alias_test.go
- shortcuts/slides/slides_replace_slide.go
- shortcuts/slides/shortcuts.go
tianyouskrrr
force-pushed
the
feat/slides-update-shortcut
branch
from
July 31, 2026 08:21
afe1830 to
1c6522a
Compare
tianyouskrrr
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/slides/slides_update_slide_diff.go (1)
218-227: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winQuote attribute values in the canonical form.
The canonical string joins attributes as
name=valuewith a single space and no delimiters. Two different attribute sets can then produce the same canonical string, for examplea="b c=d"versusa="b" c="d". In that case the diff treats a changed element as unchanged and drops the edit without a report. Quote the value to make the encoding unambiguous.♻️ Proposed change
for _, attr := range t.Attr { - attrs = append(attrs, attr.Name.Local+"="+attr.Value) + attrs = append(attrs, fmt.Sprintf("%s=%q", attr.Name.Local, attr.Value)) }🤖 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_diff.go` around lines 218 - 227, Update the attribute serialization in the element canonicalization flow around the existing attrs construction and sort.Strings call to quote each attr.Value when building the name=value representation. Preserve the current single-space joining and sorting behavior while ensuring values containing spaces or delimiters cannot collide with separate attributes.shortcuts/slides/slides_update_slide.go (1)
244-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAttach the parse failure as the error cause.
parseWantedPagepreserves the underlying error withWithCause, but this path only interpolates it with%v. Callers and tests cannot then unwrap the XML parse error.♻️ Proposed change
current, err := parsePageDoc(content) if err != nil { - return pageDoc{}, errs.NewInternalError(errs.SubtypeInvalidResponse, "slide %s returned XML the CLI cannot parse: %v", slideID, err) + return pageDoc{}, errs.NewInternalError(errs.SubtypeInvalidResponse, "slide %s returned XML the CLI cannot parse: %v", slideID, err).WithCause(err) }🤖 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 244 - 247, Update the parsePageDoc error path in parseWantedPage to attach the original parse error as the internal error cause using the established WithCause pattern, while retaining the existing subtype and contextual message for slideID. Ensure callers can unwrap the XML parse failure.
🤖 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.
Nitpick comments:
In `@shortcuts/slides/slides_update_slide_diff.go`:
- Around line 218-227: Update the attribute serialization in the element
canonicalization flow around the existing attrs construction and sort.Strings
call to quote each attr.Value when building the name=value representation.
Preserve the current single-space joining and sorting behavior while ensuring
values containing spaces or delimiters cannot collide with separate attributes.
In `@shortcuts/slides/slides_update_slide.go`:
- Around line 244-247: Update the parsePageDoc error path in parseWantedPage to
attach the original parse error as the internal error cause using the
established WithCause pattern, while retaining the existing subtype and
contextual message for slideID. Ensure callers can unwrap the XML parse failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62a1dd25-98ad-46f6-b6bd-3e17955434a6
📒 Files selected for processing (12)
shortcuts/slides/helpers.goshortcuts/slides/shortcuts.goshortcuts/slides/shortcuts_alias_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_update_slide.goshortcuts/slides/slides_update_slide_diff.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.gotests/cli_e2e/slides/slides_update_slide_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- shortcuts/slides/slides_replace_slide.go
- shortcuts/slides/helpers.go
- shortcuts/slides/shortcuts_alias_test.go
- skills/lark-slides/SKILL.md
- shortcuts/slides/shortcuts.go
tianyouskrrr
force-pushed
the
feat/slides-update-shortcut
branch
from
July 31, 2026 08:50
1c6522a to
db5da6b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/slides/slides_update_slide.go (1)
224-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProject API response shapes into typed structs instead of ad hoc map access.
readCurrentPage(Lines 244-251) pulls"slide"/"content"out of the read response viacommon.GetMap/common.GetString, andupdateSlideExecute(Lines 224-232) pulls"failed_reason"/"revision_id"out of the replace response the same way. Neither response shape has a dedicated projection function; the field names are repeated as string literals at each call site, so a typo in a key (e.g."failed_reason") would fail silently rather than at compile time.Define one small struct per response shape (slide-read result, slide-replace result) and one projection function per shape, then use struct field access instead of the raw map lookups at these two sites.
As per coding guidelines: "Parse map[string]interface{} into typed structs at the boundary, use one projection function per shape."♻️ Suggested projection helpers
type slideReadData struct { Slide struct { Content string `json:"content"` } `json:"slide"` } type slideReplaceData struct { FailedReason string `json:"failed_reason"` RevisionID *float64 `json:"revision_id"` }Also applies to: 239-251
🤖 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 224 - 232, Define typed response structs and projection functions for the slide-read and slide-replace response shapes, including slide content, failed reason, and optional revision ID. Update readCurrentPage to use the slide-read projection and updateSlideExecute to use the slide-replace projection with field access instead of common.GetMap/common.GetString and repeated string keys, preserving the existing error and revision handling behavior.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.
Nitpick comments:
In `@shortcuts/slides/slides_update_slide.go`:
- Around line 224-232: Define typed response structs and projection functions
for the slide-read and slide-replace response shapes, including slide content,
failed reason, and optional revision ID. Update readCurrentPage to use the
slide-read projection and updateSlideExecute to use the slide-replace projection
with field access instead of common.GetMap/common.GetString and repeated string
keys, preserving the existing error and revision handling behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 45727d4c-9b85-4145-b1df-ada3e05a3cef
📒 Files selected for processing (12)
shortcuts/slides/helpers.goshortcuts/slides/shortcuts.goshortcuts/slides/shortcuts_alias_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_update_slide.goshortcuts/slides/slides_update_slide_diff.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.gotests/cli_e2e/slides/slides_update_slide_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- shortcuts/slides/helpers.go
- shortcuts/slides/slides_replace_slide.go
- tests/cli_e2e/slides/slides_update_slide_workflow_test.go
- skills/lark-slides/SKILL.md
- shortcuts/slides/slides_update_slide_diff.go
- shortcuts/slides/shortcuts.go
- shortcuts/slides/shortcuts_alias_test.go
- tests/cli_e2e/slides/coverage.md
tianyouskrrr
force-pushed
the
feat/slides-update-shortcut
branch
from
July 31, 2026 09:15
db5da6b to
018879f
Compare
github-actions
Bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
skills/lark-slides/references/lark-slides-update-slide.md (1)
135-135: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftWarn that splitting over-limit updates is not atomic.
Multiple
+replace-slidecalls can leave a partially updated slide if one call fails or another user edits the page between calls. Require a re-read and re-diff for each chunk, and document whether--tidprovides transaction protection.🤖 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-update-slide.md` at line 135, Update the over-200-part guidance near the +replace-slide batching instructions to warn that multiple chunked calls are not atomic and may leave partial updates. Require re-reading and re-diffing the slide before each chunk, and document whether --tid provides transaction protection; do not imply atomicity unless it is guaranteed.
🤖 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-update-slide.md`:
- Line 40: Update the fontFamily replacement command in the slide update
instructions to use an edit form compatible with both BSD/macOS and GNU sed, or
explicitly label the existing sed -i '' syntax as macOS-only; preserve the
replacement of all fontFamily values with 思源黑体.
- Around line 149-151: Update the read step in the +xml-get workflow to
explicitly request whiteboard export before passing the result to +update-slide.
Preserve the minimal-diff workflow and ensure the returned slide includes
whiteboardToken instead of an unexported placeholder; do not rely on
+update-slide to silently preserve or overwrite such placeholders.
- Line 76: Update the timeout retry guidance in the slide update documentation,
including the retry advice near the later retry section, to require reading the
latest XML and re-diffing the updated content before retrying. Explicitly warn
that retries for inserts without a stable id can duplicate elements because the
timed-out operation may have partially succeeded.
In `@tests/cli_e2e/slides/slides_update_slide_dryrun_test.go`:
- Around line 152-158: Strengthen the validation assertions in the dry-run test
around the existing result.ExitCode checks: require exit code 2 exactly and
require result.Stdout to be empty before parsing the typed error envelope from
result.Stderr. Preserve the existing stderr assertions for error.type,
error.subtype, error.param, and error.message.
---
Nitpick comments:
In `@skills/lark-slides/references/lark-slides-update-slide.md`:
- Line 135: Update the over-200-part guidance near the +replace-slide batching
instructions to warn that multiple chunked calls are not atomic and may leave
partial updates. Require re-reading and re-diffing the slide before each chunk,
and document whether --tid provides transaction protection; do not imply
atomicity unless it is guaranteed.
🪄 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: b5661a0a-dd0a-4c30-a66a-022d1c908f2e
📒 Files selected for processing (12)
shortcuts/slides/helpers.goshortcuts/slides/shortcuts.goshortcuts/slides/shortcuts_alias_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_update_slide.goshortcuts/slides/slides_update_slide_diff.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.gotests/cli_e2e/slides/slides_update_slide_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- shortcuts/slides/helpers.go
- shortcuts/slides/slides_replace_slide.go
- skills/lark-slides/SKILL.md
- shortcuts/slides/shortcuts_alias_test.go
- tests/cli_e2e/slides/slides_update_slide_workflow_test.go
- shortcuts/slides/slides_update_slide_diff.go
- shortcuts/slides/slides_update_slide.go
- tests/cli_e2e/slides/coverage.md
- shortcuts/slides/slides_update_slide_test.go
- shortcuts/slides/shortcuts.go
Comment on lines
+149
to
+151
| | **画板需要显式导出** | 画板只在读取时显式要求导出才会带 `whiteboardToken`；否则读回的是占位符，写回等于丢掉画板 | | ||
|
|
||
| 最后两条再次指向同一条建议：**以 `+xml-get` 的输出为基准做最小改动**，别手写整页。 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo files matching lark-slides reference:\n'
fd -a 'lark-slides-update-slide\.md$' . || true
file="$(fd 'lark-slides-update-slide\.md$' . | head -n 1)"
if [ -n "${file:-}" ]; then
printf '\n--- outline ---\n'
ast-grep outline "$file" || true
printf '\n--- relevant lines ---\n'
wc -l "$file"
sed -n '1,220p' "$file" | cat -n
fi
printf '\n--- related symbols/usages ---\n'
rg -n "whiteboardToken|whiteboard|whiteboards|xml-get|update-slide|画板|占位符|export" . --glob '!**/.git/**' || trueRepository: larksuite/cli
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Candidate slide whiteboard references:\n'
fd -a 'lark-slides.*whiteboard.*\.md$|lark-slides.*xml.*\.md$|lark-slides.*update-slide.*\.md$' skills || true
printf '\n--- focused whiteboard/xml references ---\n'
for f in $(fd -a 'lark-slides.*whiteboard.*\.md$|lark-slides.*xml.*\.md$', skills | sort); do
echo "### $f ($(wc -l < "$f") lines)"
rg -n -i "whiteboard|board_token|whiteboardToken|占位|placeholder|export|update-slide|xml-get|slide" "$f" || true
done
printf '\n--- relevant XML references ---\n'
for f in $(fd -a 'lark-slides.*xml.*\.md$', skills | sort); do
echo "### $f"
sed -n '1,220p' "$f" | cat -n
done
printf '\n--- focused update-slide mentions in docs ---\n'
rg -n "update-slide|xml-get|whiteboardToken|board_token|画板|placeholder|占位" skills --glob '*.md' || trueRepository: larksuite/cli
Length of output: 50045
Make the read step include the whiteboard export option.
The document says default reads can return a whiteboard placeholder, and the workflow shows reading the page with plain +xml-get. Change the read command to require the explicit whiteboard export option, or make +update-slide reject slides that contain an unexported whiteboard placeholder.
🤖 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-update-slide.md` around lines 149 -
151, Update the read step in the +xml-get workflow to explicitly request
whiteboard export before passing the result to +update-slide. Preserve the
minimal-diff workflow and ensure the returned slide includes whiteboardToken
instead of an unexported placeholder; do not rely on +update-slide to silently
preserve or overwrite such placeholders.
tianyouskrrr
force-pushed
the
feat/slides-update-shortcut
branch
2 times, most recently
from
July 31, 2026 10:02
24a99ce to
15a6a0a
Compare
Editing most of a page previously meant one block_replace part per element, each carrying that element's full XML (coordinates, size, font size included). Restyling every text block on a page was impractical as a result. +update-slide takes the page the caller wants, reads the page that is there, and sends one element-level part per difference. The indirection is forced: ReplacePart.block_id is validated as a short ELEMENT id (it must start with "b"), so a single part covering the page is impossible — the page's own id is "p"-prefixed and the background fill's id is "f"-prefixed, and both are rejected with 3350001 while element-level parts on the same page succeed. Element ids are the only handles this endpoint offers. --content is the page's target state. An element carrying its original id is replaced when it differs, an element without an id is inserted at the position it was written, an element missing from --content is deleted, and a missing <note> clears the speaker notes. Nothing differing means no request is sent at all. Comparison is canonical because the server returns pretty-printed XML with reordered attributes and injected style defaults the caller never wrote: attributes are namespace-qualified, quoted and sorted, and indentation between structural elements is dropped. Text inside a <p> subtree is compared verbatim and escaped — SML collapses a literal space between inline tags, but a preserved space written as   decodes to the very same token, so trimming would drop a real edit as unchanged; and without escaping, a paragraph holding the literal text "</p><p>" reads identically to two empty paragraphs. What is sent is the caller's exact bytes, so their formatting survives into the page. Anything the diff cannot express fails loudly instead of being silently dropped: a background change (<style> has no id of its own and its <fill> id is "f"-prefixed), a reorder of existing elements (there is no move operation), an id that does not exist on the page (omit the id to create an element), a root id that differs from --slide-id (the classic read-page-A-write-page-B mistake), any slide-level structure other than one <style>, one <data> and one <note>, and container attributes — the root additionally accepts the SML namespace in the three forms the repository's SXSD validator recognizes (the official identifier and the two server read-back spellings), since the primary workflow feeds +xml-get output straight back in. Unrepresentable structure in --content is a validation error; a page that comes back with it is a failed precondition. Pages carrying an <undefined> placeholder — the server's stand-in for an object it could not export, such as a whiteboard read without its export option — are refused outright. Whether the whole-page rewrite behind slide.replace preserves an untouched placeholder is a server-owned behavior that no self-contained test can pin down (boards cannot be created programmatically: the CLI has no whiteboard-create and SML has no whiteboard element), and editing on an unverifiable assumption risks silently destroying the one object the caller cannot see. +replace-slide remains the element-level path for such pages. --revision-id is documented as what it is. Passing a stale revision is not rejected by the backend: it means "apply against this snapshot", so pinning an older one discards every later edit to the page. -1, the default, is the safe value. slides:presentation:read sits in the enforced pre-flight scopes, not in ConditionalScopes: every execution reads the page before writing it, and ConditionalScopes is metadata only, so a write-only token would reach the GET before failing. Also: - +update as a hidden alias, derived from the canonical shortcut so scopes, identities and flags cannot drift. Agents reach for "slide update" before reading --help and used to burn turns on the error plus a help dump. - --xml / --slide-xml / --slide-content / --content-xml as hidden spellings of --content, scoped to the whole-page commands: --content exists on other slides shortcuts, and resolving these aliases there would rewrite a mistyped flag into one the caller never meant to use. - A backend failure reason becomes a typed API error rather than a field on a success envelope. - www.larkoffice.com joins the public-domain allowlist: it is the host of the SML namespace identifier, the same vendor domain family as the already-listed www.feishu.cn / www.larksuite.com, and the quality gate matches exact hostnames. - Live and dry-run E2E coverage per AGENTS.md. The live workflow walks replace, insert, delete and the no-op against the real API, checks a background change is refused with the page left untouched and the deck order holds, and is self-contained where that is knowable: for stored credentials it probes the cleanup scopes with a dry-run delete and refuses to create a deck it cannot delete (unexpected probe failures are fatal); environment tokens carry no scope metadata, so those runs rest on the documented requirement that the CI identity is provisioned with the cleanup scopes, and a cleanup failure stays fatal and visible. The dry-run suite pins the validation process contract (launch error, exit code 2, empty stdout, typed envelope on stderr). An earlier design passed every HTTP stub and was rejected outright by the real endpoint; coverage.md records that the live test skips without a user token, which is the blind spot that allowed it. - Skill docs: what the command does, what it refuses, why the background cannot be changed, and a portable (BSD/GNU) sed spelling in the primary example.
tianyouskrrr
force-pushed
the
feat/slides-update-shortcut
branch
from
July 31, 2026 10:20
15a6a0a to
307e753
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
lark-cli slides +update-slide: hand it the page you want, and it reads the page that is there, diffs the two, and sends one element-level part per difference.slide_idand the page's position in the deck are preserved. Restyling most of a page previously meant enumerating oneblock_replacepart per element and re-writing each element's full XML by hand — coordinates, size and font size included.The indirection is forced, not stylistic.
ReplacePart.block_idis validated as a short element id (it must start withb), so a single part covering the whole page cannot be expressed: the page's own id isp-prefixed and the background fill's id isf-prefixed, and both come back3350001while element-level parts on the same page succeed. Element ids are the only handles this endpoint offers.Changes
slides +update-slide(hidden alias+update).--contentis the page's target state: an element keeping its id is replaced when it differs, an element without an id is inserted where it was written, an element missing from--contentis deleted, and a missing<note>clears the speaker notes. When nothing differs, no request is sent and the result saysunchanged.<p>subtree is compared verbatim and escaped: SML collapses a literal inter-tag space, but a preserved space written as decodes to the very same token, so trimming would drop a real edit asunchanged— and without escaping, a paragraph holding the literal text</p><p>reads identically to two empty paragraphs. What goes on the wire is the caller's exact bytes, so their formatting survives into the page.<style>has no id of its own, its<fill>id isf-prefixed), a reorder of existing elements (there is no move operation), an id that does not exist on the page (omit the id to create an element instead), a root id differing from--slide-id(the read-page-A-write-page-B mistake, which element-id checks catch only incidentally), any slide-level structure beyond one<style>, one<data>and one<note>, and container attributes — the root additionally accepts the SML namespace in the three forms the repo's SXSD validator recognizes (official + both server read-back spellings), since the primary workflow feeds+xml-getoutput straight back in. All of this holds on either side of the diff, since a comparison of only the recognized parts could answerunchangedfor a change the caller asked for.<undefined>placeholder are refused outright (failed_precondition). The placeholder stands for an object the server could not export — a whiteboard read without its export option, video/audio embeds. Whether the whole-page rewrite preserves an untouched one is server-owned behavior that no self-contained test can pin down (boards cannot be created programmatically: no whiteboard-create in the CLI, no whiteboard element in SML), so the command refuses to edit such pages rather than proceed on an unverifiable assumption;+replace-slideremains the element-level path there.www.larkoffice.comjoins the public-domain allowlist — it is the host of the SML namespace identifier and the same vendor family as the already-listedwww.feishu.cn/www.larksuite.com; the quality gate matches exact hostnames, and the namespace literals live in the Go validation table mirroringsxsd_validator.py.alt="foo rotateWithShape=true"andalt="foo" rotateWithShape="true"no longer canonicalize identically (which would have dropped a real change as unchanged).slides:presentation:readis enforced pre-flight, not merely declared: every execution reads the page before writing it, andConditionalScopesis metadata only, so a write-only token would have reached the GET before failing.--revision-idis documented as what it is. A stale revision is not rejected by the backend — it means "apply against this snapshot", so pinning an older one discards every later edit to that page.-1, the default, is the safe value.--xml/--slide-xml/--slide-content/--content-xmlspellings of--content, scoped to these two commands —--contentexists on other slides shortcuts, and resolving the aliases there would rewrite a mistyped flag into one the caller never meant.Test Plan
go test ./shortcuts/... ./cmd/... ./internal/...go vet ./...,gofmt -l .,go mod tidy,golangci-lint run --new-from-rev=origin/main(0 issues), and the repository quality gatego run -C lint . --changed-from <merge-base> ..(clean — this is the gate that rejected the namespace literals before the allowlist entry)--dry-rundelete (whose pre-flight reads the stored grants) and skips before creating anything when they are missing, with unexpected probe failures fatal; environment tokens (TEST_USER_ACCESS_TOKEN) carry no scope metadata and no API exposes a token's grants without exercising them, so those runs rest on the documented requirement that the CI identity is provisioned with the cleanup scopes — and a cleanup failure stays fatal and visible. The fully-scoped run creates, edits and deletes its own deck (verified: top-level test green, no leaked presentations).<slide>root.The live workflow is the part that matters. An earlier version of this command sent a single part covering the whole page: it passed every HTTP stub and was rejected outright by the real endpoint.
tests/cli_e2e/slides/coverage.mdrecords the remaining blind spot — that test skips without a user token, so a CI run with only bot credentials leaves the backend behavior unverified.Related Issues