fix(slides): name the wrong --parts field instead of non-empty replacement - #2174
Merged
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe slide replacement parser now enforces action-specific fields for ChangesSlide replacement validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as slides +replace-slide
participant Parser as slides_replace_slide parser
participant Schema as action field schema
CLI->>Parser: Parse --parts
Parser->>Schema: Validate action fields
Schema-->>Parser: Return valid fields or diagnostics
Parser-->>CLI: Return validation result
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 |
github-actions
Bot
added
the
size/M
![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
🤖 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_slide_test.go`:
- Around line 870-875: Update the “several bad fields report deterministically”
test to parse the same payload repeatedly and assert that the complete error
output is identical on every run, preserving the expected sorted-first field
message while ensuring nondeterministic map iteration cannot pass accidentally.
- Around line 758-759: Isolate CLI configuration in the affected tests: in
shortcuts/slides/slides_replace_slide_test.go at lines 758-759, remove
t.Parallel() from TestReplaceSlideUnknownPartField and call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before each
cmdutil.TestFactory call; at lines 879-882, remove subtest parallelism so shared
environment state is safe; and at lines 920-923, remove t.Parallel() from
TestReplaceSlideAllKnownPartFieldsAccepted and set the temporary config
directory before cmdutil.TestFactory.
- Around line 287-289: Update shortcuts/slides/slides_replace_slide_test.go
lines 287-289 to assert errs.ProblemOf(err) equals errs.CategoryValidation and
errs.SubtypeInvalidArgument for the empty replacement case, while retaining
errors.As to validate ValidationError.Param is "--parts"; also add the same
category and subtype assertions at lines 901-913 beside the existing parameter
and hint checks, reading Param from *errs.ValidationError rather than
errs.ProblemOf.
🪄 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: 2084264b-9af1-4626-9163-c20bb7702d26
📒 Files selected for processing (5)
shortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-replace-slide.md
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@79e607e0d8fcf2c8ef5a9dd777e017926112c231🧩 Skill updatenpx skills add larksuite/cli#fix/slides-parts-field-name-hints -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2174 +/- ##
==========================================
+ Coverage 75.70% 75.72% +0.01%
==========================================
Files 944 944
Lines 100288 100355 +67
==========================================
+ Hits 75926 75994 +68
Misses 18565 18565
+ Partials 5797 5796 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tianyouskrrr
force-pushed
the
fix/slides-parts-field-name-hints
branch
from
August 4, 2026 07:23
efbe816 to
7c24321
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-replace-slide.md`:
- Line 79:
更新该文档中未知字段错误提示的两处描述：明确说明提示可能给出相似字段、其他操作中的字段，或在没有匹配项时给出完整合法字段集合；同时保留“不要修改字段值”的指导。
🪄 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: 42224e23-f5c1-4af3-9f1b-dcb9818a91ec
📒 Files selected for processing (5)
shortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-replace-slide.md
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/lark-slides/SKILL.md
- skills/lark-slides/references/lark-slides-edit-workflows.md
- shortcuts/slides/slides_replace_slide_test.go
- shortcuts/slides/slides_replace_slide.go
tianyouskrrr
force-pushed
the
fix/slides-parts-field-name-hints
branch
from
August 4, 2026 08:45
7c24321 to
70024cc
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 `@tests/cli_e2e/slides/slides_replace_slide_dryrun_test.go`:
- Around line 82-89: Extend the empty-payload rejection assertions in the test
around result.AssertExitCode to require that result.Stdout is empty, matching
TestSlidesReplaceSlideUnknownFieldDryRunE2E, while preserving the existing
stderr envelope checks.
- Around line 94-136: Add a live E2E test alongside
TestSlidesReplaceSlideDryRunE2E that creates a temporary presentation, invokes
slides +replace-slide with both block_replace and block_insert parts, reads the
target slide back, and asserts the replacement and insertion XML changes
persisted. Use bot credentials and the existing live-test helpers, then ensure
the temporary presentation is deleted via cleanup even on failure; update
slides/coverage.md to record the new live coverage.
- Around line 127-135: Extend the assertions for the block_insert entry in the
mixed batch test to verify that body[1].Get("insertion").String() exactly
matches the requested insertion XML. Keep the existing action and
insert_before_block_id assertions unchanged, using the expected transformed
value if the request pipeline modifies the XML.
🪄 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: 49067be5-c734-4442-8fe9-f6fa52887a39
📒 Files selected for processing (7)
shortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-replace-slide.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_replace_slide_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/lark-slides/SKILL.md
- skills/lark-slides/references/lark-slides-edit-workflows.md
- shortcuts/slides/slides_replace_slide_test.go
- shortcuts/slides/slides_replace_slide.go
tianyouskrrr
force-pushed
the
fix/slides-parts-field-name-hints
branch
from
August 4, 2026 08:50
70024cc to
02975b1
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_slide.go`:
- Around line 221-223: Update parseReplaceParts to detect normalized variants of
the "action" key, such as "Action" and "action-", before schema lookup, and emit
the corrective diagnostic `did you mean "action"?` instead of allowing empty
p.Action to bypass checkReplacePartFields. Add a regression test covering this
input and preserve normal validation for the exact key.
- Line 53: Update the tool schema description near the parts field to explicitly
mark action as required, document the exact accepted fields for each action, and
state that any extra keys outside the selected action schema are rejected.
In `@skills/lark-slides/references/lark-slides-replace-slide.md`:
- Line 5: 更新 --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 Plus
Run ID: 94c3ce2e-82a6-42cc-bb7f-e53be32c5197
📒 Files selected for processing (7)
shortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-replace-slide.mdtests/cli_e2e/slides/coverage.mdtests/cli_e2e/slides/slides_replace_slide_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- skills/lark-slides/SKILL.md
- skills/lark-slides/references/lark-slides-edit-workflows.md
- shortcuts/slides/slides_replace_slide_test.go
- tests/cli_e2e/slides/coverage.md
- tests/cli_e2e/slides/slides_replace_slide_dryrun_test.go
…cement" XML written into a field name this shortcut does not accept — most often "content", because <shape> nests a <content> child — was silently dropped, so the part failed the required-field check and reported "requires non-empty replacement". That reads as "the value is empty", which sends callers rewriting the value instead of the key. Reject fields outside the action's own set and name the field the caller most likely meant, with a correct one-liner attached as a hint. Matching folds case and separators so "Content", "newXml" and "block-id" resolve too, while the whitelist itself stays exact: the API accepts only snake_case, so "Replacement" must be rejected rather than slip through. Only block_replace and block_insert parts are checked, so missing / str_replace / unknown actions keep their existing errors, and an actually-empty payload still reports the non-empty wording. The alias list covers only names that plausibly carry a fragment. A shape attribute like "fill" is deliberately absent: whoever writes it means "recolor this block", not "here is my XML", so answering did-you-mean "replacement" would be guessing. The unknown-field error already names the valid set, which is true under either reading. Docs carry the same constraint at the three points a caller can hit first: SKILL.md, the +replace-slide reference (warning + counter-examples + error table), and the read-modify-write workflow. The --parts flag description now spells the field names out instead of eliding them behind "...". Note: this tightens parsing. Extra keys inside a part used to be ignored; they are now rejected.
tianyouskrrr
force-pushed
the
fix/slides-parts-field-name-hints
branch
from
August 4, 2026 09:27
02975b1 to
79e607e
Compare
fangshuyu-768
approved these changes
Aug 4, 2026
fangshuyu-768
left a comment
fangshuyu-768
left a comment
Collaborator
There was a problem hiding this comment.
Verified the updated head and the new built-binary dry-run E2E coverage. Targeted unit tests, vet, repeated parser checks, CLI E2E, formatting, and skill checks pass. No remaining blockers.
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
2 participants
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
When a
--partsentry carries the XML fragment under a field name the shortcut does not accept —contentbeing the common one, since<shape>nests a<content>child — the key was silently dropped and the part then failed the required-field check withrequires non-empty replacement. That message reads as "the value is empty", so callers rewrite the value instead of the key and never converge.Changes
content,xml,block, …) points atreplacement/insertion, a field belonging to the other action says so, and anything else falls back to listing the valid set. Every such error carries a correct one-linehint._/-separators when matching the suggestion, soContent,newXmlandblock-idresolve too. The whitelist itself stays an exact match, since the API accepts only snake_case.requires non-empty …wording, and parts carrying an action this shortcut does not expose keep their existing errors.--partsflag description instead of eliding them behind....+replace-slidereference (warning, counter-examples, error table) and the read-modify-write workflow.Note: this tightens parsing — extra keys inside a part used to be ignored, and are now rejected.
Test Plan
go test ./shortcuts/slides/— 20 new cases covering each payload alias, casing / separator variants, cross-action fields, unlisted typos, deterministic reporting when several fields are wrong, and the empty-payload regressiongo vet ./shortcuts/slides/gofmt -l .produces no output--new-from-rev=origin/mainreports 0 issuesgo run -C lint .) passnode scripts/skill-format-check/index.jsandscripts/check-skill-wire-vocab.shpassblock_replace+block_insertbatch still passesRelated Issues
Summary by CodeRabbit
Improvements
Documentation
Tests