refactor(slides): streamline create workflow and validation#1887
refactor(slides): streamline create workflow and validation#1887ethan-zhx wants to merge 9 commits into
Conversation
📝 WalkthroughWalkthroughSlides shortcuts now support bounded screenshot batches and flexible full- or single-slide XML retrieval. The XML linter adds schema, namespace, icon, text-overlap, and whiteboard checks, while Slides skill references document the revised creation, validation, readback, and template workflows. ChangesScreenshot selector limits
Slides XML retrieval
XML linting and Slides guidance
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant SlidesXMLGet
participant SlidesAPI
participant OutputWriter
CLI->>SlidesXMLGet: provide presentation and optional slide selector
SlidesXMLGet->>SlidesAPI: request full or single-slide XML
SlidesAPI-->>SlidesXMLGet: return XML content
SlidesXMLGet->>OutputWriter: emit JSON, raw XML, or file metadata
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| for icon in icons | ||
| if isinstance(icon, dict) and isinstance(icon.get("iconType"), str) and icon["iconType"] | ||
| } | ||
| _ICONPARK_ICON_TYPES_CACHE = icon_types |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1887 +/- ##
==========================================
- Coverage 74.66% 74.66% -0.01%
==========================================
Files 877 878 +1
Lines 91731 91869 +138
==========================================
+ Hits 68494 68596 +102
- Misses 17926 17949 +23
- Partials 5311 5324 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@7d67a41280157390aecb9c9d358451088eedc89a🧩 Skill updatenpx skills add larksuite/cli#refactor/slides-create-workflow-validation -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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_screenshot_test.go`:
- Around line 293-298: Update the error assertion in the slide screenshot test
to inspect typed metadata through errs.ProblemOf instead of matching the error
message substring. Assert the expected category, subtype, and parameter metadata
for the maximum-pages validation while retaining the existing requirement that
an error is returned.
In `@shortcuts/slides/slides_screenshot.go`:
- Around line 284-288: Update validateSlidesScreenshotSelectorLimit so the
validation error message contains only the limit violation details, and attach
the “request at most 10 pages at a time” recovery guidance via WithHint.
Construct the error directly with errs.NewValidationError and preserve the
existing nil return when count is within maxSlidesPerScreenshot.
In `@shortcuts/slides/slides_xml_get_test.go`:
- Around line 387-406: Update TestSlidesXMLGetRejectsConflictingSlideSelectors,
TestSlidesXMLGetRejectsEmptySlideID, and
TestSlidesXMLGetRejectsRemoveAttrIDForSingleSlide to validate errors through
errs.ProblemOf(err), matching TestSlidesXMLGetRejectsUnsafeOutputPath. Assert
the expected category, subtype, and parameter metadata, and verify the
underlying cause is preserved instead of only checking ValidationError.Param.
In `@skills/lark-slides/references/examples.md`:
- Around line 10-11: Update the two table-of-contents links for 示例 7 and 示例 8 to
use fragments generated from their exact example headings, including the correct
Markdown anchor normalization. Verify both targets resolve successfully and
preserve the existing link text.
- Around line 57-71: Update the presentation creation flow around
PRESENTATION_ID so the lark-cli slides +create exit status is captured
separately from writing create.json, preserving failures instead of allowing the
pipeline to report jq’s status. Extract .data.xml_presentation_id from
create.json with jq -e and validate that it is a non-empty string before saving
it for readback.
In `@skills/lark-slides/references/troubleshooting.md`:
- Line 13: Update the two-step creation guidance in the troubleshooting
documentation to use the registered command syntax `xml_presentation.slide
create` instead of `xml_presentation.slide.create`, while preserving the
surrounding recovery flow.
In `@skills/lark-slides/references/validation-checklist.md`:
- Line 92: Update the whiteboard coordinate validation guidance in the checklist
to include lower-bound checks: topLeftX and topLeftY must be at least zero, and
width and height must be non-negative, alongside the existing right and bottom
upper bounds.
In `@skills/lark-slides/references/xml-schema-quick-ref.md`:
- Line 149: 更新该 XML schema 快速参考中关于 iconType 的说明，明确要求其必须存在于离线 IconPark 索引
iconpark-index.json 中，而非仅使用“已验证路径”的表述；保持 fillColor 和 iconpark.md 的相关规则不变。
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 500-536: Update the shape text extraction in the element-building
loop so paragraph boundaries from separate <p> elements are preserved before
line-width estimation. Replace the strip_xml(content) path used for the
element’s “text” value with paragraph-aware normalization that emits newline
separators between paragraphs, and apply the same handling to the related text
extraction around the later reference. Keep existing tag removal and whitespace
behavior for content within each paragraph.
- Around line 733-734: Update lint_slide and the should_flag_overlap flow so
should_flag_horizontal_text_overflow is evaluated before the
declared-bounding-box intersection gate, allowing overflow across gaps to be
detected. Preserve intersection-based handling for other overlap cases, and add
a regression test covering adjacent text boxes with non-intersecting declared
boxes.
- Around line 408-462: Update validate_sml_tag_prefixes to create the Expat
parser with a namespace separator so StartNamespaceDeclHandler is enabled and
namespace_map is populated before prefixed-tag validation. Preserve the existing
declaration scoping and SML_NAMESPACE checks.
In `@skills/lark-slides/SKILL.md`:
- Line 13: Update the global hard constraint in the slide guidance to treat
960x540 as the default canvas size rather than an invariant. Instruct
implementations to preserve and use the declared dimensions of imported or
existing presentations, keeping all content within that presentation’s actual
page boundaries.
- Line 183: Replace the bare Chinese text under the XML <data> element with a
valid XML comment, preserving its guidance that shape, line, table, and chart
elements belong there.
🪄 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: 4d4fef28-2413-438b-b56e-985b09dcabb7
📒 Files selected for processing (25)
shortcuts/slides/slides_screenshot.goshortcuts/slides/slides_screenshot_test.goshortcuts/slides/slides_xml_get.goshortcuts/slides/slides_xml_get_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/examples.mdskills/lark-slides/references/iconpark.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-pptx-template-workflows.mdskills/lark-slides/references/lark-slides-replace-pages.mdskills/lark-slides/references/lark-slides-replace-slide.mdskills/lark-slides/references/lark-slides-screenshot.mdskills/lark-slides/references/lark-slides-xml-get.mdskills/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-get.mdskills/lark-slides/references/lark-slides-xml-presentations-get.mdskills/lark-slides/references/planning-layer.mdskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/validation-checklist.mdskills/lark-slides/references/visual-planning.mdskills/lark-slides/references/xml-schema-quick-ref.mdskills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
💤 Files with no reviewable changes (2)
- skills/lark-slides/references/lark-slides-xml-presentation-slide-create.md
- skills/lark-slides/references/lark-slides-media-upload.md
| - [示例 7: +replace-slide + block_insert 给已有页加图](#示例-7-replace-slide--block_insert-给已有页加图) | ||
| - [示例 8: +replace-slide + block_replace 替换一个块](#示例-8-replace-slide--block_replace-替换一个块) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the invalid TOC fragments.
Markdownlint reports both links as unresolved (MD051). Regenerate their fragments from the actual example headings so in-page navigation works.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 10-10: Link fragments should be valid
(MD051, link-fragments)
[warning] 11-11: Link fragments should be valid
(MD051, link-fragments)
🤖 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/examples.md` around lines 10 - 11, Update the
two table-of-contents links for 示例 7 and 示例 8 to use fragments generated from
their exact example headings, including the correct Markdown anchor
normalization. Verify both targets resolve successfully and preserve the
existing link text.
Source: Linters/SAST tools
| } | ||
| if strings.TrimSpace(runtime.Str("output")) == "" { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--output cannot be empty").WithParam("--output") | ||
| if err := validateSlidesXMLGetSelector(runtime); err != nil { |
There was a problem hiding this comment.
The refactor dropped the previous --revision-id lower-bound validation. Before this PR, runtime.Int("revision-id") < -1 returned a typed validation error, but now values such as --revision-id -2 pass validation and are sent to both the dry-run/API params. Please restore the -1 or non-negative check and add a regression test so invalid revisions fail before any API call.
| | 在 slide 中绘制柱/条/折线/面积/雷达/饼等有数据序列的图表 | 使用原生 `<chart>` 元素 | `xml-schema-quick-ref.md` | | ||
| | 在 slide 中绘制流程图、时序图、架构图、散点图、漏斗图或装饰图案 | 必须先用 Read 工具读取参考文档，再生成 `<whiteboard>` 元素 | [`lark-slides-whiteboard.md`](references/lark-slides-whiteboard.md) | | ||
| | 使用语义图标 | 先检索 IconPark，再写 `<icon iconType="...">` | `iconpark_tool.py search → resolve`、`iconpark.md` | | ||
| | 在 slide 中绘制图表 | 雷达图、饼图、环形图用 `<chart>`，柱状图、条形图、折线图用 `shape` + `line`，Mermaid 用 `<whiteboard>` | `xml-schema-quick-ref.md`、需要 `<whiteboard>` 再看 `lark-slides-whiteboard.md` | |
There was a problem hiding this comment.
This guidance points column/bar/line charts to hand-built shape + line drawings, but the schema and asset-planning docs treat column, bar, line, area, radar, pie, doughnut/ring, and combo as supported native <chart> visuals. If agents follow this quick reference, standard data charts become static drawings with worse data fidelity, harder edits, and inconsistent rendering. Please keep all supported standard data charts on native <chart> and reserve shape/line or <whiteboard> only for unsupported/custom visuals such as funnel or scatter.
|
|
||
| 1. **先规划再写 XML**：新建演示文稿或大幅改写页面时，必须先写入 `.lark-slides/plan/<deck-or-task-id>/slide_plan.json`；风格和大纲只能作为规划输入，不能绕过规划层 | ||
| 1. **先规划再写 XML**：新建演示文稿或大幅改写页面时，必须先写入 `.lark-slides/plan/<deck-or-task-id>/slide_plan.json`；模板、风格和大纲只能作为规划输入，不能绕过规划层 | ||
| 2. **创建流程**：简单短 XML（1-3 页、结构简单、特殊字符少）可用 `slides +create --slides '[...]'` 一步创建；复杂内容、含图片/中文大段文本/嵌套引号/较多特殊字符，或超过 10 页时，默认先 `slides +create` 创建空白 PPT，再用 `xml_presentation.slide.create` 逐页添加 |
There was a problem hiding this comment.
This top-level rule still uses the dotted xml_presentation.slide.create spelling, but the executable CLI form is lark-cli slides xml_presentation.slide create (create is the subcommand after the resource). Because this is in the core rules, agents will copy the wrong command form even if some examples below are correct. Please normalize these guidance mentions to xml_presentation.slide create or show the full lark-cli slides xml_presentation.slide create command consistently.
|
|
||
| **CRITICAL — 全局硬约束：PPT 的尺寸是 960x540，确保主体内容在页面边界内。** | ||
|
|
||
| **CRITICAL — 图片至关重要：必须有意识的主动多用图片！素材图使用生图工具和搜图工具，缺图时用生图工具生成配图补足；背景图必须使用生图工具，且生图指令中必须明确要求不要出现任何文字。** |
There was a problem hiding this comment.
This hard requirement conflicts with the new asset-planning rule below that asset_need is metadata only and must not require web search, local download, media upload, or external tools. Read literally, agents will start searching/generating images during planning or template-edit tasks, even when a concrete XML-native fallback is enough or the imported template's visuals should be preserved. Please scope this to the execution phase: plan asset needs and fallbacks first, then only search/generate/upload real assets when the user asks for real media or the chosen workflow explicitly supports it.
| sxsd_issues = validate_sxsd_tag_attributes(root) if root is not None else [] | ||
| iconpark_issues = validate_iconpark_icon_types(root) if root is not None else [] | ||
| top_level_issues = [*namespace_issues, *sxsd_issues, *iconpark_issues] | ||
| presentation = parse_presentation(xml) |
There was a problem hiding this comment.
The new prefixed-namespace lint path can still abort before returning the structured issues. For an input like <ns0:slide xmlns:ns0="http://www.larkoffice.com/sml/2.0">..., parse_xml_root succeeds and namespace_issues is populated, but this call still uses the regex-based parse_presentation, which only recognizes literal <slide> / <presentation> tags and raises XmlTextOverlapLintError. That makes test_lint_xml_rejects_prefixed_sml_tags error instead of reporting sml_prefixed_tag issues. Please either parse slides from the ElementTree root or return top-level namespace/schema issues before the regex presentation parser can fail on prefixed roots.
2 participants
Summary
Streamline the Lark Slides create/edit workflow and add validation coverage for XML, namespaces, icon types, text overlap, and screenshot requests.
Changes
Test Plan
Related Issues
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Quality