refactor: reorganize lark slides skill references - #2207
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 reorganizes ChangesSlides XML workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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@1cb74f1c4384f7e65da209bc5b122122355ec16a🧩 Skill updatenpx skills add larksuite/cli#refactor/reorg_slides -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: 12
🧹 Nitpick comments (2)
skills/lark-slides/scripts/xml_lint.py (1)
2968-2975: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
--helpkey check.
parse_argsstrips the leading--from each token, so the options dictionary never contains the key"--help". Onlyoptions.get("help")can be true.♻️ Proposed cleanup
- if options.get("help") or options.get("--help"): + if options.get("help"):🤖 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/scripts/xml_lint.py` around lines 2968 - 2975, Remove the unreachable options.get("--help") check from run_cli, leaving the existing options.get("help") handling and usage behavior unchanged.skills/lark-slides/scripts/xml_text_overlap_lint.py (1)
2-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the compatibility stub.
xml_text_overlap_lintis only referenced in the stub itself, while existing docs/test/workflow usages point toscripts/xml_lint.py, so the wrapper module should be removed to avoid accidental use.🤖 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/scripts/xml_text_overlap_lint.py` around lines 2 - 7, Remove the xml_text_overlap_lint compatibility stub entirely, including its imports and re-exports; retain the canonical xml_lint module and its run_cli and XmlLayoutLintError symbols unchanged.
🤖 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/cli/lark-slides-media-upload.md`:
- Around line 28-36: Align the media upload response example with the token
extraction commands: either nest file_token under data or update the jq paths in
the documented upload flows, including the references around lines 66-68 and
108, to read the top-level field. Ensure the copy-paste commands return the
actual token rather than null.
In `@skills/lark-slides/references/cli/lark-slides-replace-slide.md`:
- Around line 132-138: Update the `<table>` example to include explicit `width`
and `height` attributes alongside `topLeftX` and `topLeftY`, ensuring it
satisfies the XML contract described in `SKILL.md` while preserving the existing
table structure.
In
`@skills/lark-slides/references/cli/lark-slides-xml-presentation-slide-replace.md`:
- Around line 42-49: Update the direct block_replace examples in this document
to include id="bab" on each replacement root, matching the corresponding
block_id so copied commands satisfy the required replacement contract; leave all
block_insert examples unchanged.
In `@skills/lark-slides/references/iconpark-index.json`:
- Around line 1-5: The legacy references/iconpark-index.json path must remain
valid JSON for existing consumers. Replace the Markdown migration notice in
iconpark-index.json with a symlink or equivalent copy of
xml/iconpark-index.json, and place the migration notice in a separate Markdown
file.
In `@skills/lark-slides/references/lark-slides-add-slide.md`:
- Line 3: Update the migration notices in
skills/lark-slides/references/lark-slides-add-slide.md:3 and
skills/lark-slides/references/lark-slides-delete-slide.md:3 to point to existing
destination documents under skills/lark-slides, or create the referenced target
documents before linking to them.
In `@skills/lark-slides/references/lark-slides-edit-workflows.md`:
- Line 3: Fix the broken migration links in
skills/lark-slides/references/lark-slides-edit-workflows.md:3-3 and
skills/lark-slides/references/lark-slides-pptx-template-workflows.md:3-3 by
moving the referenced documents to the stated workflow/ paths or updating both
links to their actual existing locations, ensuring all migration notices and
back-references resolve correctly.
In `@skills/lark-slides/references/lark-slides-history.md`:
- Line 1: Remove the two stray backticks immediately before the Markdown heading
marker in the slides history document so the line begins directly with # and
renders as a heading.
In `@skills/lark-slides/references/slides_xml_schema_definition.xml`:
- Around line 1-5: Remove the Markdown compatibility notice from
skills/lark-slides/references/slides_xml_schema_definition.xml and update all
remaining references to use xml/slides_xml_schema_definition.xml, or replace it
with parseable XML containing the notice in an XML comment. Apply the same
treatment to skills/lark-slides/references/slides_chart_demo.xml, directing
references to xml/slides_chart_demo.xml; ensure both .xml paths remain valid for
XML parsers and xml_lint.py.
In `@skills/lark-slides/references/xml/iconpark.md`:
- Line 29: Update the icon fallback guidance to choose the nearest semantically
relevant icon when no suitable match is found, rather than selecting randomly
from frequent examples. If no semantic fallback exists, remove the icon and
reclaim its layout space, preserving the surrounding icon-search guidance.
In `@skills/lark-slides/references/xml/slides_chart_demo.xml`:
- Around line 226-229: Correct the footer page-count text in each visible footer
shape, including the instances near the referenced sections, so the seven-slide
presentation is numbered sequentially from 01 / 07 through 07 / 07. Preserve the
existing footer styling and update only the displayed pagination values.
In `@skills/lark-slides/references/xml/slides_xml_schema_definition.xml`:
- Around line 955-968: Update the shadow documentation text in the annotation
above the shadow attributes to name the attribute as align instead of
shadowAlign, matching the declared xs:attribute and avoiding
unsupported-attribute guidance.
In `@skills/lark-slides/scripts/xml_lint.py`:
- Around line 325-344: Update load_iconpark_icon_types to handle a missing or
unreadable ICONPARK_INDEX_PATH before read_text, following
iconpark_tool.load_index’s existence-check-and-fail pattern. Route the failure
through fail(...) so the CLI’s existing XmlLayoutLintError handling emits the
standard error message, while preserving the current JSON decoding and
validation behavior.
---
Nitpick comments:
In `@skills/lark-slides/scripts/xml_lint.py`:
- Around line 2968-2975: Remove the unreachable options.get("--help") check from
run_cli, leaving the existing options.get("help") handling and usage behavior
unchanged.
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 2-7: Remove the xml_text_overlap_lint compatibility stub entirely,
including its imports and re-exports; retain the canonical xml_lint module and
its run_cli and XmlLayoutLintError symbols unchanged.
🪄 Autofix
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: 4b78c468-0ff2-43dc-bdcd-7181e604cb8f
📒 Files selected for processing (36)
skills/lark-slides/SKILL.mdskills/lark-slides/references/cli/lark-slides-media-upload.mdskills/lark-slides/references/cli/lark-slides-replace-pages.mdskills/lark-slides/references/cli/lark-slides-replace-slide.mdskills/lark-slides/references/cli/lark-slides-xml-presentation-slide-get.mdskills/lark-slides/references/cli/lark-slides-xml-presentation-slide-replace.mdskills/lark-slides/references/cli/lark-slides-xml-presentations-get.mdskills/lark-slides/references/iconpark-index.jsonskills/lark-slides/references/iconpark.mdskills/lark-slides/references/lark-slides-add-slide.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-delete-slide.mdskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-history.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-xml-presentation-slide-get.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-replace.mdskills/lark-slides/references/lark-slides-xml-presentations-get.mdskills/lark-slides/references/planning-layer.mdskills/lark-slides/references/slides_chart_demo.xmlskills/lark-slides/references/slides_xml_schema_definition.xmlskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/validation-checklist.mdskills/lark-slides/references/xml-schema-quick-ref.mdskills/lark-slides/references/xml/iconpark-index.jsonskills/lark-slides/references/xml/iconpark.mdskills/lark-slides/references/xml/slides_chart_demo.xmlskills/lark-slides/references/xml/slides_xml_schema_definition.xmlskills/lark-slides/scripts/iconpark_tool.pyskills/lark-slides/scripts/xml_lint.pyskills/lark-slides/scripts/xml_lint_test.pyskills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
a3e9003 to
ad82da4
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
🧹 Nitpick comments (2)
skills/lark-slides/scripts/xml_lint.py (1)
2968-2975: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
--helpkey check.
parse_argsstrips the leading--from each token, so the options dictionary never contains the key"--help". Onlyoptions.get("help")can be true.♻️ Proposed cleanup
- if options.get("help") or options.get("--help"): + if options.get("help"):🤖 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/scripts/xml_lint.py` around lines 2968 - 2975, Remove the unreachable options.get("--help") check from run_cli, leaving the existing options.get("help") handling and usage behavior unchanged.skills/lark-slides/scripts/xml_text_overlap_lint.py (1)
2-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the compatibility stub.
xml_text_overlap_lintis only referenced in the stub itself, while existing docs/test/workflow usages point toscripts/xml_lint.py, so the wrapper module should be removed to avoid accidental use.🤖 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/scripts/xml_text_overlap_lint.py` around lines 2 - 7, Remove the xml_text_overlap_lint compatibility stub entirely, including its imports and re-exports; retain the canonical xml_lint module and its run_cli and XmlLayoutLintError symbols unchanged.
🤖 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/cli/lark-slides-media-upload.md`:
- Around line 28-36: Align the media upload response example with the token
extraction commands: either nest file_token under data or update the jq paths in
the documented upload flows, including the references around lines 66-68 and
108, to read the top-level field. Ensure the copy-paste commands return the
actual token rather than null.
In `@skills/lark-slides/references/cli/lark-slides-replace-slide.md`:
- Around line 132-138: Update the `<table>` example to include explicit `width`
and `height` attributes alongside `topLeftX` and `topLeftY`, ensuring it
satisfies the XML contract described in `SKILL.md` while preserving the existing
table structure.
In
`@skills/lark-slides/references/cli/lark-slides-xml-presentation-slide-replace.md`:
- Around line 42-49: Update the direct block_replace examples in this document
to include id="bab" on each replacement root, matching the corresponding
block_id so copied commands satisfy the required replacement contract; leave all
block_insert examples unchanged.
In `@skills/lark-slides/references/iconpark-index.json`:
- Around line 1-5: The legacy references/iconpark-index.json path must remain
valid JSON for existing consumers. Replace the Markdown migration notice in
iconpark-index.json with a symlink or equivalent copy of
xml/iconpark-index.json, and place the migration notice in a separate Markdown
file.
In `@skills/lark-slides/references/lark-slides-add-slide.md`:
- Line 3: Update the migration notices in
skills/lark-slides/references/lark-slides-add-slide.md:3 and
skills/lark-slides/references/lark-slides-delete-slide.md:3 to point to existing
destination documents under skills/lark-slides, or create the referenced target
documents before linking to them.
In `@skills/lark-slides/references/lark-slides-edit-workflows.md`:
- Line 3: Fix the broken migration links in
skills/lark-slides/references/lark-slides-edit-workflows.md:3-3 and
skills/lark-slides/references/lark-slides-pptx-template-workflows.md:3-3 by
moving the referenced documents to the stated workflow/ paths or updating both
links to their actual existing locations, ensuring all migration notices and
back-references resolve correctly.
In `@skills/lark-slides/references/lark-slides-history.md`:
- Line 1: Remove the two stray backticks immediately before the Markdown heading
marker in the slides history document so the line begins directly with # and
renders as a heading.
In `@skills/lark-slides/references/slides_xml_schema_definition.xml`:
- Around line 1-5: Remove the Markdown compatibility notice from
skills/lark-slides/references/slides_xml_schema_definition.xml and update all
remaining references to use xml/slides_xml_schema_definition.xml, or replace it
with parseable XML containing the notice in an XML comment. Apply the same
treatment to skills/lark-slides/references/slides_chart_demo.xml, directing
references to xml/slides_chart_demo.xml; ensure both .xml paths remain valid for
XML parsers and xml_lint.py.
In `@skills/lark-slides/references/xml/iconpark.md`:
- Line 29: Update the icon fallback guidance to choose the nearest semantically
relevant icon when no suitable match is found, rather than selecting randomly
from frequent examples. If no semantic fallback exists, remove the icon and
reclaim its layout space, preserving the surrounding icon-search guidance.
In `@skills/lark-slides/references/xml/slides_chart_demo.xml`:
- Around line 226-229: Correct the footer page-count text in each visible footer
shape, including the instances near the referenced sections, so the seven-slide
presentation is numbered sequentially from 01 / 07 through 07 / 07. Preserve the
existing footer styling and update only the displayed pagination values.
In `@skills/lark-slides/references/xml/slides_xml_schema_definition.xml`:
- Around line 955-968: Update the shadow documentation text in the annotation
above the shadow attributes to name the attribute as align instead of
shadowAlign, matching the declared xs:attribute and avoiding
unsupported-attribute guidance.
In `@skills/lark-slides/scripts/xml_lint.py`:
- Around line 325-344: Update load_iconpark_icon_types to handle a missing or
unreadable ICONPARK_INDEX_PATH before read_text, following
iconpark_tool.load_index’s existence-check-and-fail pattern. Route the failure
through fail(...) so the CLI’s existing XmlLayoutLintError handling emits the
standard error message, while preserving the current JSON decoding and
validation behavior.
---
Nitpick comments:
In `@skills/lark-slides/scripts/xml_lint.py`:
- Around line 2968-2975: Remove the unreachable options.get("--help") check from
run_cli, leaving the existing options.get("help") handling and usage behavior
unchanged.
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 2-7: Remove the xml_text_overlap_lint compatibility stub entirely,
including its imports and re-exports; retain the canonical xml_lint module and
its run_cli and XmlLayoutLintError symbols unchanged.
🪄 Autofix
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: 4b78c468-0ff2-43dc-bdcd-7181e604cb8f
📒 Files selected for processing (36)
skills/lark-slides/SKILL.mdskills/lark-slides/references/cli/lark-slides-media-upload.mdskills/lark-slides/references/cli/lark-slides-replace-pages.mdskills/lark-slides/references/cli/lark-slides-replace-slide.mdskills/lark-slides/references/cli/lark-slides-xml-presentation-slide-get.mdskills/lark-slides/references/cli/lark-slides-xml-presentation-slide-replace.mdskills/lark-slides/references/cli/lark-slides-xml-presentations-get.mdskills/lark-slides/references/iconpark-index.jsonskills/lark-slides/references/iconpark.mdskills/lark-slides/references/lark-slides-add-slide.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-delete-slide.mdskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-history.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-xml-presentation-slide-get.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-replace.mdskills/lark-slides/references/lark-slides-xml-presentations-get.mdskills/lark-slides/references/planning-layer.mdskills/lark-slides/references/slides_chart_demo.xmlskills/lark-slides/references/slides_xml_schema_definition.xmlskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/validation-checklist.mdskills/lark-slides/references/xml-schema-quick-ref.mdskills/lark-slides/references/xml/iconpark-index.jsonskills/lark-slides/references/xml/iconpark.mdskills/lark-slides/references/xml/slides_chart_demo.xmlskills/lark-slides/references/xml/slides_xml_schema_definition.xmlskills/lark-slides/scripts/iconpark_tool.pyskills/lark-slides/scripts/xml_lint.pyskills/lark-slides/scripts/xml_lint_test.pyskills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
🛑 Comments failed to post (4)
skills/lark-slides/references/iconpark-index.json (1)
1-5: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the legacy
.jsonpath valid JSON.This Markdown content is not JSON. A consumer that still opens
references/iconpark-index.jsonas the advertised compatibility path will fail during JSON parsing. Keep a valid JSON compatibility asset, such as a symlink or equivalent copy ofxml/iconpark-index.json, and move this migration notice to a Markdown file.🧰 Tools
🪛 Biome (2.5.6)
[error] 1-1: unexpected character
#(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: unexpected character
（(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: unexpected character
）(parse)
[error] 3-3: String values must be double quoted.
(parse)
[error] 3-3: unexpected character ```
(parse)
[error] 3-3: expected
,but instead foundxml(parse)
[error] 3-3: unexpected character
/(parse)
[error] 3-3: expected
,but instead foundiconpark(parse)
[error] 3-3: Minus must be followed by a digit
(parse)
[error] 3-3: expected
,but instead foundindex(parse)
[error] 3-3: unexpected character
.(parse)
[error] 3-3: expected
,but instead foundjson(parse)
[error] 3-3: End of file expected
(parse)
[error] 3-3: unexpected character ```
(parse)
[error] 3-3: unexpected character
((parse)
[error] 3-3: String values must be double quoted.
(parse)
[error] 3-3: unexpected character
/(parse)
[error] 3-3: String values must be double quoted.
(parse)
[error] 3-3: Minus must be followed by a digit
(parse)
[error] 3-3: String values must be double quoted.
(parse)
[error] 3-3: unexpected character
.(parse)
[error] 3-3: String values must be double quoted.
(parse)
[error] 3-3: unexpected character
)(parse)
[error] 3-3: unexpected character
。(parse)
[error] 5-5: String values must be double quoted.
(parse)
[error] 5-5: unexpected character
；(parse)
[error] 5-5: String values must be double quoted.
(parse)
[error] 5-5: unexpected character
。(parse)
🤖 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/iconpark-index.json` around lines 1 - 5, The legacy references/iconpark-index.json path must remain valid JSON for existing consumers. Replace the Markdown migration notice in iconpark-index.json with a symlink or equivalent copy of xml/iconpark-index.json, and place the migration notice in a separate Markdown file.Source: Linters/SAST tools
skills/lark-slides/references/slides_xml_schema_definition.xml (1)
1-5: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Markdown migration notices kept under
.xmlfile extensions. Both files retain the.xmlextension while their content is now Markdown starting with#. Any XML parser, schema loader, or.xmlglob — including thexml_lint.pyscript added in this stack — fails on these paths instead of reading a notice. The shared root cause is one compatibility strategy applied to artifacts that are not Markdown.
skills/lark-slides/references/slides_xml_schema_definition.xml#L1-L5: delete the path and update the remaining references, or replace the body with a parseable XML document that carries the notice in an XML comment.skills/lark-slides/references/slides_chart_demo.xml#L1-L5: apply the same treatment, pointing toxml/slides_chart_demo.xml.📍 Affects 2 files
skills/lark-slides/references/slides_xml_schema_definition.xml#L1-L5(this comment)skills/lark-slides/references/slides_chart_demo.xml#L1-L5🤖 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/slides_xml_schema_definition.xml` around lines 1 - 5, Remove the Markdown compatibility notice from skills/lark-slides/references/slides_xml_schema_definition.xml and update all remaining references to use xml/slides_xml_schema_definition.xml, or replace it with parseable XML containing the notice in an XML comment. Apply the same treatment to skills/lark-slides/references/slides_chart_demo.xml, directing references to xml/slides_chart_demo.xml; ensure both .xml paths remain valid for XML parsers and xml_lint.py.skills/lark-slides/references/xml/slides_xml_schema_definition.xml (1)
955-968: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the documentation attribute name with the declared attribute.
The documentation names the attribute
shadowAlign. The declaration on Line 968 names italign. An author who follows the documentation writes an attribute thatxml_lint.pyreports assxsd_unsupported_attr.📝 Proposed documentation fix
对齐方式: - - shadowAlign: 阴影相对于元素的对齐方式（默认为top-left左上对齐） + - align: 阴影相对于元素的对齐方式（默认为top-left左上对齐）📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.对齐方式: - align: 阴影相对于元素的对齐方式（默认为top-left左上对齐） 注意:空标签表示默认阴影效果(color=rgba(0, 0, 0, 0.25), offset=15, blur=35, angle=45) </xs:documentation> </xs:annotation> <xs:attribute name="color" type="sml:Color" use="optional" default="rgba(0, 0, 0, 0.25)"/> <xs:attribute name="offset" type="sml:Range0To200DotType" use="optional" default="15" /> <xs:attribute name="blur" type="sml:PercentageType" use="optional" default="35"/> <xs:attribute name="angle" type="sml:RotationType" use="optional" default="45"/> <xs:attribute name="hScale" type="sml:ScaleType" use="optional" default="1"/> <xs:attribute name="vScale" type="sml:ScaleType" use="optional" default="1"/> <xs:attribute name="hSkew" type="sml:SkewType" use="optional" default="0"/> <xs:attribute name="vSkew" type="sml:SkewType" use="optional" default="0"/> <xs:attribute name="align" type="sml:ShadowAlignType" use="optional" default="top-left"/>🤖 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/xml/slides_xml_schema_definition.xml` around lines 955 - 968, Update the shadow documentation text in the annotation above the shadow attributes to name the attribute as align instead of shadowAlign, matching the declared xs:attribute and avoiding unsupported-attribute guidance.skills/lark-slides/scripts/xml_lint.py (1)
325-344: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle a missing iconpark index file.
load_iconpark_icon_typescatches onlyjson.JSONDecodeError. Ifreferences/xml/iconpark-index.jsonis absent or unreadable,read_textraisesOSError. The__main__block catches onlyXmlLayoutLintError, so the CLI prints a traceback instead of thexml-lint error: ...message. This PR moves that file into a new directory, so a stale or wrong path is a realistic failure.
iconpark_tool.load_indexalready uses the intended pattern: check existence, then callfail(...).🛡️ Proposed fix
- try: - index_data = json.loads(ICONPARK_INDEX_PATH.read_text(encoding="utf-8")) - except json.JSONDecodeError as error: - fail(f"invalid iconpark index JSON: {error}") + try: + index_data = json.loads(ICONPARK_INDEX_PATH.read_text(encoding="utf-8")) + except OSError as error: + fail(f"iconpark index not found: {ICONPARK_INDEX_PATH} ({error})") + except json.JSONDecodeError as error: + fail(f"invalid iconpark index JSON: {error}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def load_iconpark_icon_types() -> set[str]: global _ICONPARK_ICON_TYPES_CACHE if _ICONPARK_ICON_TYPES_CACHE is not None: return _ICONPARK_ICON_TYPES_CACHE try: index_data = json.loads(ICONPARK_INDEX_PATH.read_text(encoding="utf-8")) except OSError as error: fail(f"iconpark index not found: {ICONPARK_INDEX_PATH} ({error})") except json.JSONDecodeError as error: fail(f"invalid iconpark index JSON: {error}") icons = index_data.get("icons") if not isinstance(icons, list): fail("iconpark index must contain an icons array") icon_types = { icon["iconType"] for icon in icons if isinstance(icon, dict) and isinstance(icon.get("iconType"), str) and icon["iconType"] } _ICONPARK_ICON_TYPES_CACHE = icon_types return icon_types🤖 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/scripts/xml_lint.py` around lines 325 - 344, Update load_iconpark_icon_types to handle a missing or unreadable ICONPARK_INDEX_PATH before read_text, following iconpark_tool.load_index’s existence-check-and-fail pattern. Route the failure through fail(...) so the CLI’s existing XmlLayoutLintError handling emits the standard error message, while preserving the current JSON decoding and validation behavior.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 similar comment
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
skills/lark-slides/references/cli/lark-slides-replace-pages.md (1)
5-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the blank line between the two blockquotes.
markdownlint reports MD028 (
no-blanks-blockquote) at line 6. A blank line between two>blocks can render as one merged quote in some renderers. Separate the blocks with a>continuation line, or merge them into one blockquote.♻️ Proposed fix
> **灰度期间的重要兼容说明**：当前命令的 `--help` 及输出中的 `deprecated` 字段会提示改用 `+update-slide`，但该提示依赖尚未全量的 Slide Engine 后端能力。在本参考页随 skill 发布期间，必须忽略该提示并继续使用 `+replace-pages`，不得切换到 `+update-slide`。后端全量并由新版 skill 恢复路由后，再使用 `+update-slide`。 - -> 重要：这是多步编排，不是后端原子事务。CLI 对每页执行“先创建新页到旧页前，再删除旧页”；创建失败时旧页会保留。删除失败时可能出现新旧页同时存在，需要按返回结果继续处理。 +> +> 重要：这是多步编排，不是后端原子事务。CLI 对每页执行“先创建新页到旧页前，再删除旧页”；创建失败时旧页会保留。删除失败时可能出现新旧页同时存在，需要按返回结果继续处理。🤖 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/cli/lark-slides-replace-pages.md` around lines 5 - 7, Remove the blank line between the two blockquotes in the reference content, either by adding a `>` continuation line or merging both paragraphs into one blockquote, so the markdown satisfies MD028.Source: Linters/SAST tools
skills/lark-slides/SKILL.md (1)
82-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the document references in the Quick Reference table use consistent paths.
Rows 86 and 87 use the new
xml/prefix. The other rows still use bare file names such aslark-slides-replace-slide.md,iconpark.md,validation-xml.md, andslides_chart_demo.xml. Those files now live inreferences/cli/,references/xml/, orreferences/workflow/. A reader who resolves a bare name lands on the legacy stub instead of the canonical document. Add the same subdirectory prefix to every row.🤖 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/SKILL.md` around lines 82 - 95, Update the Quick Reference table in the document so every referenced file uses its canonical references/ subdirectory path, including entries under references/cli/, references/xml/, and references/workflow/. Apply this consistently to all rows, not only the existing xml/ references, while leaving command names and non-file references unchanged.skills/lark-slides/scripts/xml_lint_test.py (1)
862-862: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDistinguish the two chart roundtrip test names.
test_lint_xml_ignores_chart_parsed_values_roundtrip_tagsandtest_lint_xml_ignores_chart_parsed_values_roundtrip_tagdiffer only by a trailings. The bodies differ only in whetherchartDatacarriesisStaticData="true". A reader cannot tell which behavior each test pins. Rename both to state the distinguishing condition, for example..._roundtrip_tag_with_static_dataand..._roundtrip_tag_without_static_data.Also applies to: 885-885
🤖 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/scripts/xml_lint_test.py` at line 862, Rename the two chart roundtrip tests to clearly distinguish their static-data conditions: update test_lint_xml_ignores_chart_parsed_values_roundtrip_tags and test_lint_xml_ignores_chart_parsed_values_roundtrip_tag to names such as ..._roundtrip_tag_with_static_data and ..._roundtrip_tag_without_static_data, matching each test’s chartData isStaticData="true" usage.skills/lark-slides/references/workflow/validation-xml.md (1)
55-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the remaining release-blocking codes to the table.
xml_lint.pyalso emitsduplicate_element_idandtext_may_overflow_shapeaterrorlevel. Both block release, andduplicate_element_idhas a non-obvious remediation: remove theidattribute instead of inventing a new one. Add rows for them so the table covers every code that can setrelease_ready == false.♻️ Proposed additions
| `bbox_overlap` | 文本元素的估算绘制区域明显重叠 | 拉开文本坐标、缩小文本框/字号，或改成明确的分栏/分组结构 | +| `duplicate_element_id` | 多个元素共用同一个 `id` | 新写的元素直接删除 `id` 属性；改写回读 XML 时只保留原元素上的服务端 ID，不要自造新 ID | +| `text_may_overflow_shape` | 估算文本高度超出 `<shape>` 自身内容框 | 增大 `height`、精简文字，或设置 `wrap="true" autoFit="normal-auto-fit"` | | `*_out_of_canvas` | 元素边界超出页面画布 | 根据 `measurement.overflow` 移回画布或缩小尺寸 |🤖 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/workflow/validation-xml.md` around lines 55 - 71, 在“常见 code 的处理方向”表中补充 duplicate_element_id 和 text_may_overflow_shape 两行，确保覆盖 xml_lint.py 以 error 级别阻止 release 的代码；明确 duplicate_element_id 的处理方式是移除 id 属性而不是创建新 ID，并为 text_may_overflow_shape 指明按 lint 提示调整文本框尺寸、字号或文本内容以避免溢出。
🤖 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/cli/lark-slides-replace-slide.md`:
- Line 258: Update the link label at
skills/lark-slides/references/cli/lark-slides-replace-slide.md:258 from
lark-slides-edit-workflows.md to slides_editing.md while preserving the target
../workflow/slides_editing.md. Also update the label at
skills/lark-slides/references/lark-slides-create.md:8 from
lark-slides-pptx-template-workflows.md to template-editing.md while preserving
the target workflow/template-editing.md.
---
Nitpick comments:
In `@skills/lark-slides/references/cli/lark-slides-replace-pages.md`:
- Around line 5-7: Remove the blank line between the two blockquotes in the
reference content, either by adding a `>` continuation line or merging both
paragraphs into one blockquote, so the markdown satisfies MD028.
In `@skills/lark-slides/references/workflow/validation-xml.md`:
- Around line 55-71: 在“常见 code 的处理方向”表中补充 duplicate_element_id 和
text_may_overflow_shape 两行，确保覆盖 xml_lint.py 以 error 级别阻止 release 的代码；明确
duplicate_element_id 的处理方式是移除 id 属性而不是创建新 ID，并为 text_may_overflow_shape 指明按 lint
提示调整文本框尺寸、字号或文本内容以避免溢出。
In `@skills/lark-slides/scripts/xml_lint_test.py`:
- Line 862: Rename the two chart roundtrip tests to clearly distinguish their
static-data conditions: update
test_lint_xml_ignores_chart_parsed_values_roundtrip_tags and
test_lint_xml_ignores_chart_parsed_values_roundtrip_tag to names such as
..._roundtrip_tag_with_static_data and ..._roundtrip_tag_without_static_data,
matching each test’s chartData isStaticData="true" usage.
In `@skills/lark-slides/SKILL.md`:
- Around line 82-95: Update the Quick Reference table in the document so every
referenced file uses its canonical references/ subdirectory path, including
entries under references/cli/, references/xml/, and references/workflow/. Apply
this consistently to all rows, not only the existing xml/ references, while
leaving command names and non-file references unchanged.
🪄 Autofix
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: 588d31eb-c5c3-495e-923a-fd7ddf25e045
📒 Files selected for processing (43)
skills/lark-slides/SKILL.mdskills/lark-slides/references/cli/lark-slides-media-upload.mdskills/lark-slides/references/cli/lark-slides-replace-pages.mdskills/lark-slides/references/cli/lark-slides-replace-slide.mdskills/lark-slides/references/cli/lark-slides-xml-presentation-slide-get.mdskills/lark-slides/references/cli/lark-slides-xml-presentation-slide-replace.mdskills/lark-slides/references/cli/lark-slides-xml-presentations-get.mdskills/lark-slides/references/iconpark-index.jsonskills/lark-slides/references/iconpark.mdskills/lark-slides/references/lark-slides-add-slide.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-delete-slide.mdskills/lark-slides/references/lark-slides-edit-workflows.mdskills/lark-slides/references/lark-slides-history.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-xml-presentation-slide-get.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-replace.mdskills/lark-slides/references/lark-slides-xml-presentations-get.mdskills/lark-slides/references/planning-layer.mdskills/lark-slides/references/slides_chart_demo.xmlskills/lark-slides/references/slides_xml_schema_definition.xmlskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/validation-checklist.mdskills/lark-slides/references/workflow/error-handling.mdskills/lark-slides/references/workflow/slides_editing.mdskills/lark-slides/references/workflow/template-editing.mdskills/lark-slides/references/workflow/validation-xml.mdskills/lark-slides/references/xml-schema-quick-ref.mdskills/lark-slides/references/xml/iconpark-index.jsonskills/lark-slides/references/xml/iconpark.mdskills/lark-slides/references/xml/lark-slides-add-slide.mdskills/lark-slides/references/xml/lark-slides-delete-slide.mdskills/lark-slides/references/xml/slides_chart_demo.xmlskills/lark-slides/references/xml/slides_xml_schema_definition.xmlskills/lark-slides/references/xml/xml-schema-quick-ref.mdskills/lark-slides/scripts/iconpark_tool.pyskills/lark-slides/scripts/xml_lint.pyskills/lark-slides/scripts/xml_lint_test.pyskills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
🚧 Files skipped from review as they are similar to previous changes (30)
- skills/lark-slides/references/xml-schema-quick-ref.md
- skills/lark-slides/references/slides_xml_schema_definition.xml
- skills/lark-slides/references/lark-slides-history.md
- skills/lark-slides/references/lark-slides-media-upload.md
- skills/lark-slides/references/slides_chart_demo.xml
- skills/lark-slides/references/cli/lark-slides-media-upload.md
- skills/lark-slides/references/planning-layer.md
- skills/lark-slides/references/xml/iconpark.md
- skills/lark-slides/references/cli/lark-slides-xml-presentation-slide-get.md
- skills/lark-slides/references/lark-slides-xml-presentations-get.md
- skills/lark-slides/references/lark-slides-edit-workflows.md
- skills/lark-slides/references/lark-slides-xml-presentation-slide-get.md
- skills/lark-slides/references/lark-slides-replace-pages.md
- skills/lark-slides/references/iconpark.md
- skills/lark-slides/scripts/iconpark_tool.py
- skills/lark-slides/references/lark-slides-pptx-template-workflows.md
- skills/lark-slides/references/lark-slides-replace-slide.md
- skills/lark-slides/references/xml/xml-schema-quick-ref.md
- skills/lark-slides/references/lark-slides-delete-slide.md
- skills/lark-slides/references/cli/lark-slides-xml-presentations-get.md
- skills/lark-slides/references/cli/lark-slides-xml-presentation-slide-replace.md
- skills/lark-slides/references/troubleshooting.md
- skills/lark-slides/references/lark-slides-xml-presentation-slide-replace.md
- skills/lark-slides/references/workflow/error-handling.md
- skills/lark-slides/scripts/xml_text_overlap_lint.py
- skills/lark-slides/references/xml/slides_xml_schema_definition.xml
- skills/lark-slides/references/validation-checklist.md
- skills/lark-slides/references/workflow/slides_editing.md
- skills/lark-slides/references/xml/lark-slides-add-slide.md
- skills/lark-slides/references/xml/slides_chart_demo.xml
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-im/references/lark-im-messages-search.md`:
- Line 9: Update the --as parameter documentation in the message search
reference to state that both user and bot identities are supported, and
accurately document its current default behavior. Keep the description
consistent with the existing --as bot usage and the identity guidance near the
top of the document.
🪄 Autofix
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: 3cb7c6c4-6870-4341-ac0f-ae201ee766d0
📒 Files selected for processing (6)
affordance/im.mdinternal/affordance/im_source_test.goskills/lark-im/references/lark-im-messages-search.mdskills/lark-slides/scripts/xml_lint_test.pyskills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2207 +/- ##
==========================================
+ Coverage 76.34% 76.41% +0.06%
==========================================
Files 993 1010 +17
Lines 106691 111104 +4413
==========================================
+ Hits 81450 84895 +3445
- Misses 19037 19737 +700
- Partials 6204 6472 +268 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7aa8e5b to
c7f3994
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
skills/lark-slides/references/cli/lark-slides-replace-pages.md (1)
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the difference between
--dry-runand--validate-only.Both rows describe generating a replacement plan without calling create or delete. A reader cannot choose between them. State the observable difference, for example the output shape or which API calls each flag still performs.
🤖 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/cli/lark-slides-replace-pages.md` around lines 24 - 26, Clarify the descriptions of --dry-run and --validate-only in the options table so their observable behavior is distinct: specify which Slides API calls each mode still performs and how their outputs differ. Keep --continue-on-error unchanged.skills/lark-slides/scripts/xml_lint_test.py (3)
4100-4100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
SML_NAMESPACEto the top of the module.
SML_NAMESPACEis defined at Line 4100 but first used at Line 240. The reference resolves at call time, so the tests pass. A reader who starts at Line 240 cannot find the definition. Declare it next to the imports.🤖 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/scripts/xml_lint_test.py` at line 4100, Move the module-level SML_NAMESPACE constant from its current late definition to the import section at the top of the module, before its first use. Remove the original duplicate declaration and preserve the existing namespace value.
1012-1035: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
mock.patch.objectfor the loader override.The test replaces
xml_lint.load_iconpark_icon_typesby assignment and restores it infinally.mock.patch.objectis already imported at Line 13 and handles restoration through the context manager.♻️ Proposed refactor
def test_lint_xml_does_not_load_iconpark_index_without_icons(self) -> None: - original_loader = xml_lint.load_iconpark_icon_types - def fail_if_loaded() -> set[str]: raise AssertionError("iconpark index should not be loaded without <icon iconType>") - xml_lint.load_iconpark_icon_types = fail_if_loaded - try: + with mock.patch.object( + xml_lint, "load_iconpark_icon_types", side_effect=fail_if_loaded + ): result = xml_lint.lint_xml( """ <slide xmlns="https://www.larkoffice.com/sml/2.0"> <data> <shape type="text" topLeftX="80" topLeftY="80" width="300" height="60"> <content><p>No icons here</p></content> </shape> </data> </slide> """ ) - finally: - xml_lint.load_iconpark_icon_types = original_loader🤖 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/scripts/xml_lint_test.py` around lines 1012 - 1035, Update test_lint_xml_does_not_load_iconpark_index_without_icons to use mock.patch.object for temporarily replacing xml_lint.load_iconpark_icon_types with fail_if_loaded, wrapping the lint_xml call in the patch context and removing the manual assignment and finally restoration.
19-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test classes and helper after the module rename.
The module under test is now
xml_lint, but the classes are stillXmlTextOverlapLintGeometryTestandXmlTextOverlapLintDensityTest, and the helper isassertNoXmlTextOverlapLintErrors. The suite covers schema validation, canvas bounds, blank slides, and density, not only text overlap. Rename them to match the current scope, for exampleXmlLintGeometryTest,XmlLintDensityTest, andassertNoXmlLintErrors.Also applies to: 3308-3308
🤖 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/scripts/xml_lint_test.py` around lines 19 - 20, Rename the test classes XmlTextOverlapLintGeometryTest and XmlTextOverlapLintDensityTest to XmlLintGeometryTest and XmlLintDensityTest, and rename assertNoXmlTextOverlapLintErrors to assertNoXmlLintErrors throughout the test file, including all call sites, so names reflect the broader xml_lint coverage.skills/lark-slides/references/workflow/template-editing.md (1)
71-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference the XML lint release gate in the readback step.
SKILL.mdrequiresscripts/xml_lint.pyto returnsummary.error_count == 0before callingslides +replace-pages, and requires validation pervalidation-xml.mdafter a large rewrite. This template workflow describes readback and manual tuning only. An agent that follows this file alone can skip the gate. Add a pointer tovalidation-xml.mdandscripts/xml_lint.py.🤖 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/workflow/template-editing.md` around lines 71 - 85, Update the “Readback And Tune” section to reference validation-xml.md and scripts/xml_lint.py as the XML release gate. Require confirming summary.error_count == 0 before slides +replace-pages, and applying the validation-xml.md checks after large rewrites, while preserving the existing manual readback and tuning guidance.skills/lark-slides/references/xml/lark-slides-delete-slide.md (1)
1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving this CLI shortcut reference under
references/cli/.This file documents the
slides +delete-slideshortcut. The sibling shortcut references+replace-slide,+replace-pages,+media-upload, and+xml-getnow live inreferences/cli/. This file andlark-slides-add-slide.mdstay inreferences/xml/. The split makes the directory boundary unclear. Nothing breaks today becauseSKILL.mdpoints at the current paths, so treat this as an organization choice to confirm.🤖 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/xml/lark-slides-delete-slide.md` around lines 1 - 7, Move the delete-slide shortcut reference documented in the current XML reference into references/cli/, alongside the other CLI shortcut references. Update all references, including SKILL.md and any links to lark-slides-delete-slide.md, to use the new location while preserving the documented command behavior.skills/lark-slides/scripts/xml_lint.py (1)
423-435: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
elseafter the earlyreturn.The
ifbranch at Lines 423-434 ends withreturn. Theelseat Line 435 adds a nesting level with no effect. Dedent its body.🤖 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/scripts/xml_lint.py` around lines 423 - 435, Remove the redundant else following the early return in the supported-tag validation branch, and dedent its body so execution continues directly after the if block while preserving the existing behavior.skills/lark-slides/references/workflow/validation-xml.md (1)
55-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the remaining blocking codes to the handling table.
xml_lint.pyemits two further error-level codes that block release but have no row here:duplicate_element_idandtext_may_overflow_shape. Line 36 already lists the text-height check, so the table is inconsistent with the described scope. Without a row, the agent has no remediation direction for a blocking result. Forduplicate_element_idin particular, the linter hint forbids inventing replacement IDs, so the guidance matters.📝 Proposed rows
| `bbox_overlap` | 文本元素的估算绘制区域明显重叠 | 拉开文本坐标、缩小文本框/字号，或改成明确的分栏/分组结构 | +| `text_may_overflow_shape` | 估算文本高度超出形状自身内容框 | 增大 `height`、精简文本，或设置 `wrap="true" autoFit="normal-auto-fit"` | +| `duplicate_element_id` | 同一 `id` 被多个元素使用 | 不要臆造新 ID；新写元素删除 `id`，回读 XML 只在原元素上保留服务端 ID | | `*_out_of_canvas` | 元素边界超出页面画布 | 根据 `measurement.overflow` 移回画布或缩小尺寸 |🤖 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/workflow/validation-xml.md` around lines 55 - 71, Add rows to the code-handling table for duplicate_element_id and text_may_overflow_shape. Describe duplicate_element_id remediation as removing duplicate IDs while preserving existing IDs and not inventing replacements; describe text_may_overflow_shape remediation as shortening text, reducing font size, enlarging the shape, or otherwise ensuring text fits within its bounds.skills/lark-slides/SKILL.md (1)
82-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the document names in the Quick Reference table consistent.
Some cells use path-qualified names (
xml/lark-slides-add-slide.md,xml/lark-slides-delete-slide.md). Other cells use bare names for files that also moved (lark-slides-replace-slide.md,lark-slides-replace-pages.md,lark-slides-xml-presentations-get.md,xml-schema-quick-ref.md,slides_chart_demo.xml,iconpark.md,error-handling.md,validation-xml.md). Legacy stubs with the same bare names still exist, so a bare name is ambiguous. Use the same path-qualified form in every cell.🤖 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/SKILL.md` around lines 82 - 95, Update every document reference in the Quick Reference table to use its path-qualified location, including the bare names in the replace-slide, replace-pages, XML presentation, schema, chart demo, IconPark, and error-handling entries. Keep existing command references unchanged and ensure no ambiguous bare document names remain.
🤖 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/cli/lark-slides-replace-pages.md`:
- Around line 5-7: Remove the blank line between the two consecutive blockquote
paragraphs so they form one continuous blockquote, preserving both paragraphs’
content and wording.
In `@skills/lark-slides/references/iconpark-index.json`:
- Around line 1-5: Update the legacy iconpark-index.json compatibility file so
it remains valid JSON while preserving access to the migrated
xml/iconpark-index.json content; alternatively, move the migration notice to a
non-JSON path and update all references to the legacy entry point.
In `@skills/lark-slides/references/slides_xml_schema_definition.xml`:
- Around line 1-5: Replace the Markdown notices in
skills/lark-slides/references/slides_xml_schema_definition.xml (lines 1-5) and
skills/lark-slides/references/slides_chart_demo.xml (lines 1-5) with minimal
XML-parseable stubs, such as an XML comment and a single root element, or remove
both legacy files; ensure consumers of these paths receive valid XML while
directing future references to the migrated files.
In `@skills/lark-slides/scripts/xml_lint.py`:
- Around line 2968-2981: Update run_cli to validate that options["input"] is a
string/path value rather than merely truthy before constructing the Path, so a
missing --input value is routed through fail and produces the standard
user-facing error. Also remove the unreachable options.get("--help") check and
rely on the normalized "help" key returned by parse_args.
In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py`:
- Around line 1-11: Replace the os.execv delegation in the module-level entry
point with re-exports of the test cases and test functions from xml_lint_test,
preserving their original names so standard test discovery finds them through
the legacy module. Keep direct execution compatibility as needed, and remove the
subprocess-only delegation approach.
---
Nitpick comments:
In `@skills/lark-slides/references/cli/lark-slides-replace-pages.md`:
- Around line 24-26: Clarify the descriptions of --dry-run and --validate-only
in the options table so their observable behavior is distinct: specify which
Slides API calls each mode still performs and how their outputs differ. Keep
--continue-on-error unchanged.
In `@skills/lark-slides/references/workflow/template-editing.md`:
- Around line 71-85: Update the “Readback And Tune” section to reference
validation-xml.md and scripts/xml_lint.py as the XML release gate. Require
confirming summary.error_count == 0 before slides +replace-pages, and applying
the validation-xml.md checks after large rewrites, while preserving the existing
manual readback and tuning guidance.
In `@skills/lark-slides/references/workflow/validation-xml.md`:
- Around line 55-71: Add rows to the code-handling table for
duplicate_element_id and text_may_overflow_shape. Describe duplicate_element_id
remediation as removing duplicate IDs while preserving existing IDs and not
inventing replacements; describe text_may_overflow_shape remediation as
shortening text, reducing font size, enlarging the shape, or otherwise ensuring
text fits within its bounds.
In `@skills/lark-slides/references/xml/lark-slides-delete-slide.md`:
- Around line 1-7: Move the delete-slide shortcut reference documented in the
current XML reference into references/cli/, alongside the other CLI shortcut
references. Update all references, including SKILL.md and any links to
lark-slides-delete-slide.md, to use the new location while preserving the
documented command behavior.
In `@skills/lark-slides/scripts/xml_lint_test.py`:
- Line 4100: Move the module-level SML_NAMESPACE constant from its current late
definition to the import section at the top of the module, before its first use.
Remove the original duplicate declaration and preserve the existing namespace
value.
- Around line 1012-1035: Update
test_lint_xml_does_not_load_iconpark_index_without_icons to use
mock.patch.object for temporarily replacing xml_lint.load_iconpark_icon_types
with fail_if_loaded, wrapping the lint_xml call in the patch context and
removing the manual assignment and finally restoration.
- Around line 19-20: Rename the test classes XmlTextOverlapLintGeometryTest and
XmlTextOverlapLintDensityTest to XmlLintGeometryTest and XmlLintDensityTest, and
rename assertNoXmlTextOverlapLintErrors to assertNoXmlLintErrors throughout the
test file, including all call sites, so names reflect the broader xml_lint
coverage.
In `@skills/lark-slides/scripts/xml_lint.py`:
- Around line 423-435: Remove the redundant else following the early return in
the supported-tag validation branch, and dedent its body so execution continues
directly after the if block while preserving the existing behavior.
In `@skills/lark-slides/SKILL.md`:
- Around line 82-95: Update every document reference in the Quick Reference
table to use its path-qualified location, including the bare names in the
replace-slide, replace-pages, XML presentation, schema, chart demo, IconPark,
and error-handling entries. Keep existing command references unchanged and
ensure no ambiguous bare document names remain.
🪄 Autofix
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: d9053747-be0f-4e0b-b830-fee4f7881be7
📒 Files selected for processing (42)
skills/lark-slides/SKILL.mdskills/lark-slides/references/cli/lark-slides-media-upload.mdskills/lark-slides/references/cli/lark-slides-replace-pages.mdskills/lark-slides/references/cli/lark-slides-replace-slide.mdskills/lark-slides/references/cli/lark-slides-xml-presentation-slide-get.mdskills/lark-slides/references/cli/lark-slides-xml-presentation-slide-replace.mdskills/lark-slides/references/cli/lark-slides-xml-presentations-get.mdskills/lark-slides/references/iconpark-index.jsonskills/lark-slides/references/iconpark.mdskills/lark-slides/references/lark-slides-add-slide.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-delete-slide.mdskills/lark-slides/references/lark-slides-edit-workflows.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-xml-presentation-slide-get.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-replace.mdskills/lark-slides/references/lark-slides-xml-presentations-get.mdskills/lark-slides/references/planning-layer.mdskills/lark-slides/references/slides_chart_demo.xmlskills/lark-slides/references/slides_xml_schema_definition.xmlskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/validation-checklist.mdskills/lark-slides/references/workflow/error-handling.mdskills/lark-slides/references/workflow/slides_editing.mdskills/lark-slides/references/workflow/template-editing.mdskills/lark-slides/references/workflow/validation-xml.mdskills/lark-slides/references/xml-schema-quick-ref.mdskills/lark-slides/references/xml/iconpark-index.jsonskills/lark-slides/references/xml/iconpark.mdskills/lark-slides/references/xml/lark-slides-add-slide.mdskills/lark-slides/references/xml/lark-slides-delete-slide.mdskills/lark-slides/references/xml/slides_chart_demo.xmlskills/lark-slides/references/xml/slides_xml_schema_definition.xmlskills/lark-slides/references/xml/xml-schema-quick-ref.mdskills/lark-slides/scripts/iconpark_tool.pyskills/lark-slides/scripts/xml_lint.pyskills/lark-slides/scripts/xml_lint_test.pyskills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
🚧 Files skipped from review as they are similar to previous changes (26)
- skills/lark-slides/scripts/iconpark_tool.py
- skills/lark-slides/references/lark-slides-replace-slide.md
- skills/lark-slides/references/lark-slides-media-upload.md
- skills/lark-slides/references/lark-slides-delete-slide.md
- skills/lark-slides/references/lark-slides-add-slide.md
- skills/lark-slides/references/cli/lark-slides-xml-presentations-get.md
- skills/lark-slides/references/lark-slides-edit-workflows.md
- skills/lark-slides/references/lark-slides-xml-presentations-get.md
- skills/lark-slides/references/validation-checklist.md
- skills/lark-slides/references/workflow/error-handling.md
- skills/lark-slides/references/troubleshooting.md
- skills/lark-slides/references/lark-slides-xml-presentation-slide-get.md
- skills/lark-slides/references/xml-schema-quick-ref.md
- skills/lark-slides/references/cli/lark-slides-xml-presentation-slide-replace.md
- skills/lark-slides/references/lark-slides-replace-pages.md
- skills/lark-slides/references/xml/lark-slides-add-slide.md
- skills/lark-slides/references/xml/xml-schema-quick-ref.md
- skills/lark-slides/references/cli/lark-slides-media-upload.md
- skills/lark-slides/references/workflow/slides_editing.md
- skills/lark-slides/references/iconpark.md
- skills/lark-slides/references/xml/slides_xml_schema_definition.xml
- skills/lark-slides/references/xml/slides_chart_demo.xml
- skills/lark-slides/references/xml/iconpark.md
- skills/lark-slides/references/lark-slides-pptx-template-workflows.md
- skills/lark-slides/references/cli/lark-slides-xml-presentation-slide-get.md
- skills/lark-slides/references/planning-layer.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (9)
skills/lark-slides/references/cli/lark-slides-replace-pages.md (1)
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the difference between
--dry-runand--validate-only.Both rows describe generating a replacement plan without calling create or delete. A reader cannot choose between them. State the observable difference, for example the output shape or which API calls each flag still performs.
🤖 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/cli/lark-slides-replace-pages.md` around lines 24 - 26, Clarify the descriptions of --dry-run and --validate-only in the options table so their observable behavior is distinct: specify which Slides API calls each mode still performs and how their outputs differ. Keep --continue-on-error unchanged.skills/lark-slides/scripts/xml_lint_test.py (3)
4100-4100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
SML_NAMESPACEto the top of the module.
SML_NAMESPACEis defined at Line 4100 but first used at Line 240. The reference resolves at call time, so the tests pass. A reader who starts at Line 240 cannot find the definition. Declare it next to the imports.🤖 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/scripts/xml_lint_test.py` at line 4100, Move the module-level SML_NAMESPACE constant from its current late definition to the import section at the top of the module, before its first use. Remove the original duplicate declaration and preserve the existing namespace value.
1012-1035: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
mock.patch.objectfor the loader override.The test replaces
xml_lint.load_iconpark_icon_typesby assignment and restores it infinally.mock.patch.objectis already imported at Line 13 and handles restoration through the context manager.♻️ Proposed refactor
def test_lint_xml_does_not_load_iconpark_index_without_icons(self) -> None: - original_loader = xml_lint.load_iconpark_icon_types - def fail_if_loaded() -> set[str]: raise AssertionError("iconpark index should not be loaded without <icon iconType>") - xml_lint.load_iconpark_icon_types = fail_if_loaded - try: + with mock.patch.object( + xml_lint, "load_iconpark_icon_types", side_effect=fail_if_loaded + ): result = xml_lint.lint_xml( """ <slide xmlns="https://www.larkoffice.com/sml/2.0"> <data> <shape type="text" topLeftX="80" topLeftY="80" width="300" height="60"> <content><p>No icons here</p></content> </shape> </data> </slide> """ ) - finally: - xml_lint.load_iconpark_icon_types = original_loader🤖 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/scripts/xml_lint_test.py` around lines 1012 - 1035, Update test_lint_xml_does_not_load_iconpark_index_without_icons to use mock.patch.object for temporarily replacing xml_lint.load_iconpark_icon_types with fail_if_loaded, wrapping the lint_xml call in the patch context and removing the manual assignment and finally restoration.
19-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test classes and helper after the module rename.
The module under test is now
xml_lint, but the classes are stillXmlTextOverlapLintGeometryTestandXmlTextOverlapLintDensityTest, and the helper isassertNoXmlTextOverlapLintErrors. The suite covers schema validation, canvas bounds, blank slides, and density, not only text overlap. Rename them to match the current scope, for exampleXmlLintGeometryTest,XmlLintDensityTest, andassertNoXmlLintErrors.Also applies to: 3308-3308
🤖 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/scripts/xml_lint_test.py` around lines 19 - 20, Rename the test classes XmlTextOverlapLintGeometryTest and XmlTextOverlapLintDensityTest to XmlLintGeometryTest and XmlLintDensityTest, and rename assertNoXmlTextOverlapLintErrors to assertNoXmlLintErrors throughout the test file, including all call sites, so names reflect the broader xml_lint coverage.skills/lark-slides/references/workflow/template-editing.md (1)
71-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference the XML lint release gate in the readback step.
SKILL.mdrequiresscripts/xml_lint.pyto returnsummary.error_count == 0before callingslides +replace-pages, and requires validation pervalidation-xml.mdafter a large rewrite. This template workflow describes readback and manual tuning only. An agent that follows this file alone can skip the gate. Add a pointer tovalidation-xml.mdandscripts/xml_lint.py.🤖 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/workflow/template-editing.md` around lines 71 - 85, Update the “Readback And Tune” section to reference validation-xml.md and scripts/xml_lint.py as the XML release gate. Require confirming summary.error_count == 0 before slides +replace-pages, and applying the validation-xml.md checks after large rewrites, while preserving the existing manual readback and tuning guidance.skills/lark-slides/references/xml/lark-slides-delete-slide.md (1)
1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving this CLI shortcut reference under
references/cli/.This file documents the
slides +delete-slideshortcut. The sibling shortcut references+replace-slide,+replace-pages,+media-upload, and+xml-getnow live inreferences/cli/. This file andlark-slides-add-slide.mdstay inreferences/xml/. The split makes the directory boundary unclear. Nothing breaks today becauseSKILL.mdpoints at the current paths, so treat this as an organization choice to confirm.🤖 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/xml/lark-slides-delete-slide.md` around lines 1 - 7, Move the delete-slide shortcut reference documented in the current XML reference into references/cli/, alongside the other CLI shortcut references. Update all references, including SKILL.md and any links to lark-slides-delete-slide.md, to use the new location while preserving the documented command behavior.skills/lark-slides/scripts/xml_lint.py (1)
423-435: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
elseafter the earlyreturn.The
ifbranch at Lines 423-434 ends withreturn. Theelseat Line 435 adds a nesting level with no effect. Dedent its body.🤖 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/scripts/xml_lint.py` around lines 423 - 435, Remove the redundant else following the early return in the supported-tag validation branch, and dedent its body so execution continues directly after the if block while preserving the existing behavior.skills/lark-slides/references/workflow/validation-xml.md (1)
55-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the remaining blocking codes to the handling table.
xml_lint.pyemits two further error-level codes that block release but have no row here:duplicate_element_idandtext_may_overflow_shape. Line 36 already lists the text-height check, so the table is inconsistent with the described scope. Without a row, the agent has no remediation direction for a blocking result. Forduplicate_element_idin particular, the linter hint forbids inventing replacement IDs, so the guidance matters.📝 Proposed rows
| `bbox_overlap` | 文本元素的估算绘制区域明显重叠 | 拉开文本坐标、缩小文本框/字号，或改成明确的分栏/分组结构 | +| `text_may_overflow_shape` | 估算文本高度超出形状自身内容框 | 增大 `height`、精简文本，或设置 `wrap="true" autoFit="normal-auto-fit"` | +| `duplicate_element_id` | 同一 `id` 被多个元素使用 | 不要臆造新 ID；新写元素删除 `id`，回读 XML 只在原元素上保留服务端 ID | | `*_out_of_canvas` | 元素边界超出页面画布 | 根据 `measurement.overflow` 移回画布或缩小尺寸 |🤖 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/workflow/validation-xml.md` around lines 55 - 71, Add rows to the code-handling table for duplicate_element_id and text_may_overflow_shape. Describe duplicate_element_id remediation as removing duplicate IDs while preserving existing IDs and not inventing replacements; describe text_may_overflow_shape remediation as shortening text, reducing font size, enlarging the shape, or otherwise ensuring text fits within its bounds.skills/lark-slides/SKILL.md (1)
82-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the document names in the Quick Reference table consistent.
Some cells use path-qualified names (
xml/lark-slides-add-slide.md,xml/lark-slides-delete-slide.md). Other cells use bare names for files that also moved (lark-slides-replace-slide.md,lark-slides-replace-pages.md,lark-slides-xml-presentations-get.md,xml-schema-quick-ref.md,slides_chart_demo.xml,iconpark.md,error-handling.md,validation-xml.md). Legacy stubs with the same bare names still exist, so a bare name is ambiguous. Use the same path-qualified form in every cell.🤖 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/SKILL.md` around lines 82 - 95, Update every document reference in the Quick Reference table to use its path-qualified location, including the bare names in the replace-slide, replace-pages, XML presentation, schema, chart demo, IconPark, and error-handling entries. Keep existing command references unchanged and ensure no ambiguous bare document names remain.
🤖 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/cli/lark-slides-replace-pages.md`:
- Around line 5-7: Remove the blank line between the two consecutive blockquote
paragraphs so they form one continuous blockquote, preserving both paragraphs’
content and wording.
In `@skills/lark-slides/references/iconpark-index.json`:
- Around line 1-5: Update the legacy iconpark-index.json compatibility file so
it remains valid JSON while preserving access to the migrated
xml/iconpark-index.json content; alternatively, move the migration notice to a
non-JSON path and update all references to the legacy entry point.
In `@skills/lark-slides/references/slides_xml_schema_definition.xml`:
- Around line 1-5: Replace the Markdown notices in
skills/lark-slides/references/slides_xml_schema_definition.xml (lines 1-5) and
skills/lark-slides/references/slides_chart_demo.xml (lines 1-5) with minimal
XML-parseable stubs, such as an XML comment and a single root element, or remove
both legacy files; ensure consumers of these paths receive valid XML while
directing future references to the migrated files.
In `@skills/lark-slides/scripts/xml_lint.py`:
- Around line 2968-2981: Update run_cli to validate that options["input"] is a
string/path value rather than merely truthy before constructing the Path, so a
missing --input value is routed through fail and produces the standard
user-facing error. Also remove the unreachable options.get("--help") check and
rely on the normalized "help" key returned by parse_args.
In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py`:
- Around line 1-11: Replace the os.execv delegation in the module-level entry
point with re-exports of the test cases and test functions from xml_lint_test,
preserving their original names so standard test discovery finds them through
the legacy module. Keep direct execution compatibility as needed, and remove the
subprocess-only delegation approach.
---
Nitpick comments:
In `@skills/lark-slides/references/cli/lark-slides-replace-pages.md`:
- Around line 24-26: Clarify the descriptions of --dry-run and --validate-only
in the options table so their observable behavior is distinct: specify which
Slides API calls each mode still performs and how their outputs differ. Keep
--continue-on-error unchanged.
In `@skills/lark-slides/references/workflow/template-editing.md`:
- Around line 71-85: Update the “Readback And Tune” section to reference
validation-xml.md and scripts/xml_lint.py as the XML release gate. Require
confirming summary.error_count == 0 before slides +replace-pages, and applying
the validation-xml.md checks after large rewrites, while preserving the existing
manual readback and tuning guidance.
In `@skills/lark-slides/references/workflow/validation-xml.md`:
- Around line 55-71: Add rows to the code-handling table for
duplicate_element_id and text_may_overflow_shape. Describe duplicate_element_id
remediation as removing duplicate IDs while preserving existing IDs and not
inventing replacements; describe text_may_overflow_shape remediation as
shortening text, reducing font size, enlarging the shape, or otherwise ensuring
text fits within its bounds.
In `@skills/lark-slides/references/xml/lark-slides-delete-slide.md`:
- Around line 1-7: Move the delete-slide shortcut reference documented in the
current XML reference into references/cli/, alongside the other CLI shortcut
references. Update all references, including SKILL.md and any links to
lark-slides-delete-slide.md, to use the new location while preserving the
documented command behavior.
In `@skills/lark-slides/scripts/xml_lint_test.py`:
- Line 4100: Move the module-level SML_NAMESPACE constant from its current late
definition to the import section at the top of the module, before its first use.
Remove the original duplicate declaration and preserve the existing namespace
value.
- Around line 1012-1035: Update
test_lint_xml_does_not_load_iconpark_index_without_icons to use
mock.patch.object for temporarily replacing xml_lint.load_iconpark_icon_types
with fail_if_loaded, wrapping the lint_xml call in the patch context and
removing the manual assignment and finally restoration.
- Around line 19-20: Rename the test classes XmlTextOverlapLintGeometryTest and
XmlTextOverlapLintDensityTest to XmlLintGeometryTest and XmlLintDensityTest, and
rename assertNoXmlTextOverlapLintErrors to assertNoXmlLintErrors throughout the
test file, including all call sites, so names reflect the broader xml_lint
coverage.
In `@skills/lark-slides/scripts/xml_lint.py`:
- Around line 423-435: Remove the redundant else following the early return in
the supported-tag validation branch, and dedent its body so execution continues
directly after the if block while preserving the existing behavior.
In `@skills/lark-slides/SKILL.md`:
- Around line 82-95: Update every document reference in the Quick Reference
table to use its path-qualified location, including the bare names in the
replace-slide, replace-pages, XML presentation, schema, chart demo, IconPark,
and error-handling entries. Keep existing command references unchanged and
ensure no ambiguous bare document names remain.
🪄 Autofix
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: d9053747-be0f-4e0b-b830-fee4f7881be7
📒 Files selected for processing (42)
skills/lark-slides/SKILL.mdskills/lark-slides/references/cli/lark-slides-media-upload.mdskills/lark-slides/references/cli/lark-slides-replace-pages.mdskills/lark-slides/references/cli/lark-slides-replace-slide.mdskills/lark-slides/references/cli/lark-slides-xml-presentation-slide-get.mdskills/lark-slides/references/cli/lark-slides-xml-presentation-slide-replace.mdskills/lark-slides/references/cli/lark-slides-xml-presentations-get.mdskills/lark-slides/references/iconpark-index.jsonskills/lark-slides/references/iconpark.mdskills/lark-slides/references/lark-slides-add-slide.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-delete-slide.mdskills/lark-slides/references/lark-slides-edit-workflows.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-xml-presentation-slide-get.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-replace.mdskills/lark-slides/references/lark-slides-xml-presentations-get.mdskills/lark-slides/references/planning-layer.mdskills/lark-slides/references/slides_chart_demo.xmlskills/lark-slides/references/slides_xml_schema_definition.xmlskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/validation-checklist.mdskills/lark-slides/references/workflow/error-handling.mdskills/lark-slides/references/workflow/slides_editing.mdskills/lark-slides/references/workflow/template-editing.mdskills/lark-slides/references/workflow/validation-xml.mdskills/lark-slides/references/xml-schema-quick-ref.mdskills/lark-slides/references/xml/iconpark-index.jsonskills/lark-slides/references/xml/iconpark.mdskills/lark-slides/references/xml/lark-slides-add-slide.mdskills/lark-slides/references/xml/lark-slides-delete-slide.mdskills/lark-slides/references/xml/slides_chart_demo.xmlskills/lark-slides/references/xml/slides_xml_schema_definition.xmlskills/lark-slides/references/xml/xml-schema-quick-ref.mdskills/lark-slides/scripts/iconpark_tool.pyskills/lark-slides/scripts/xml_lint.pyskills/lark-slides/scripts/xml_lint_test.pyskills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
🚧 Files skipped from review as they are similar to previous changes (26)
- skills/lark-slides/scripts/iconpark_tool.py
- skills/lark-slides/references/lark-slides-replace-slide.md
- skills/lark-slides/references/lark-slides-media-upload.md
- skills/lark-slides/references/lark-slides-delete-slide.md
- skills/lark-slides/references/lark-slides-add-slide.md
- skills/lark-slides/references/cli/lark-slides-xml-presentations-get.md
- skills/lark-slides/references/lark-slides-edit-workflows.md
- skills/lark-slides/references/lark-slides-xml-presentations-get.md
- skills/lark-slides/references/validation-checklist.md
- skills/lark-slides/references/workflow/error-handling.md
- skills/lark-slides/references/troubleshooting.md
- skills/lark-slides/references/lark-slides-xml-presentation-slide-get.md
- skills/lark-slides/references/xml-schema-quick-ref.md
- skills/lark-slides/references/cli/lark-slides-xml-presentation-slide-replace.md
- skills/lark-slides/references/lark-slides-replace-pages.md
- skills/lark-slides/references/xml/lark-slides-add-slide.md
- skills/lark-slides/references/xml/xml-schema-quick-ref.md
- skills/lark-slides/references/cli/lark-slides-media-upload.md
- skills/lark-slides/references/workflow/slides_editing.md
- skills/lark-slides/references/iconpark.md
- skills/lark-slides/references/xml/slides_xml_schema_definition.xml
- skills/lark-slides/references/xml/slides_chart_demo.xml
- skills/lark-slides/references/xml/iconpark.md
- skills/lark-slides/references/lark-slides-pptx-template-workflows.md
- skills/lark-slides/references/cli/lark-slides-xml-presentation-slide-get.md
- skills/lark-slides/references/planning-layer.md
🛑 Comments failed to post (4)
skills/lark-slides/references/iconpark-index.json (1)
1-5: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the legacy path valid JSON.
This
.jsonfile now contains Markdown. Any consumer that parses the legacy path as JSON will fail before it can receive the migration notice. Keep valid JSON at this path, or move the notice to a non-JSON file and update all legacy references.🧰 Tools
🪛 Biome (2.5.6)
[error] 1-1: unexpected character
#(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: unexpected character
（(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: unexpected character
）(parse)
[error] 3-3: String values must be double quoted.
(parse)
[error] 3-3: unexpected character ```
(parse)
[error] 3-3: expected
,but instead foundxml(parse)
[error] 3-3: unexpected character
/(parse)
[error] 3-3: expected
,but instead foundiconpark(parse)
[error] 3-3: Minus must be followed by a digit
(parse)
[error] 3-3: expected
,but instead foundindex(parse)
[error] 3-3: unexpected character
.(parse)
[error] 3-3: expected
,but instead foundjson(parse)
[error] 3-3: End of file expected
(parse)
[error] 3-3: unexpected character ```
(parse)
[error] 3-3: unexpected character
((parse)
[error] 3-3: String values must be double quoted.
(parse)
[error] 3-3: unexpected character
/(parse)
[error] 3-3: String values must be double quoted.
(parse)
[error] 3-3: Minus must be followed by a digit
(parse)
[error] 3-3: String values must be double quoted.
(parse)
[error] 3-3: unexpected character
.(parse)
[error] 3-3: String values must be double quoted.
(parse)
[error] 3-3: unexpected character
)(parse)
[error] 3-3: unexpected character
。(parse)
[error] 5-5: String values must be double quoted.
(parse)
[error] 5-5: unexpected character
；(parse)
[error] 5-5: String values must be double quoted.
(parse)
[error] 5-5: unexpected character
。(parse)
🤖 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/iconpark-index.json` around lines 1 - 5, Update the legacy iconpark-index.json compatibility file so it remains valid JSON while preserving access to the migrated xml/iconpark-index.json content; alternatively, move the migration notice to a non-JSON path and update all references to the legacy entry point.Source: Linters/SAST tools
skills/lark-slides/references/slides_xml_schema_definition.xml (1)
1-5: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Locate consumers of the legacy XML reference paths. rg -n --hidden -g '!.git' 'slides_chart_demo\.xml|slides_xml_schema_definition\.xml' | rg -v 'references/xml/'Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Legacy stub contents:" cat -n skills/lark-slides/references/slides_xml_schema_definition.xml printf '\n---\n' cat -n skills/lark-slides/references/slides_chart_demo.xml printf '\nRelevant consumers outside references/xml/:\n' rg -n --hidden -g '!.git' 'slides_chart_demo\.xml|slides_xml_schema_definition\.xml' || true printf '\nCheck exact legacy path consumers with quotes only:\n' python3 - <<'PY' import pathlib, re root=pathlib.Path('skills/lark-slides/references') targets={ root/'slides_xml_schema_definition.xml', root/'slides_chart_demo.xml', } for p in sorted(root.rglob('*')): if p.is_file() and p not in targets: text=p.read_text(errors='replace') if any(t.name in text for t in targets): print(f"\n-- {p} --") for i,line in enumerate(text.splitlines(),1): if any(t.name in line for t in targets): print(f"{i}: {line}") PY printf '\nNew XML files metadata/outline:\n' for f in skills/lark-slides/references/xml/slides_xml_schema_definition.xml skills/lark-slides/references/xml/slides_chart_demo.xml; do printf '\n-- %s --\n' "$f" wc -l "$f" sed -n '1,40p' "$f" | cat -n doneRepository: larksuite/cli
Length of output: 772
Keep legacy
.xmlstubs XML-parseable. These files are still present and may still be consumed as XML; Markdown-style notices are not valid XML text. Either remove the legacy paths or keep the path while returning a minimal valid XML response, such as an XML comment plus a root element forskills/lark-slides/references/slides_xml_schema_definition.xmlandskills/lark-slides/references/slides_chart_demo.xml.No known consumers still resolve these legacy XML paths outside
references/xml/.📍 Affects 2 files
skills/lark-slides/references/slides_xml_schema_definition.xml#L1-L5(this comment)skills/lark-slides/references/slides_chart_demo.xml#L1-L5🤖 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/slides_xml_schema_definition.xml` around lines 1 - 5, Replace the Markdown notices in skills/lark-slides/references/slides_xml_schema_definition.xml (lines 1-5) and skills/lark-slides/references/slides_chart_demo.xml (lines 1-5) with minimal XML-parseable stubs, such as an XML comment and a single root element, or remove both legacy files; ensure consumers of these paths receive valid XML while directing future references to the migrated files.skills/lark-slides/scripts/xml_lint.py (1)
2968-2981: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
--inputwithout a value crashes with an unhandledTypeError.
parse_argsstoresTruewhen a flag has no following value (Line 102).options.get("input")is then truthy, so the guard at Line 2973 passes.Path(True)at Line 2977 raisesTypeError, which the handler at Line 2987 does not catch. The user sees a raw traceback instead ofxml-lint error: ....The second clause at Line 2970 is also dead.
parse_argsstrips the--prefix at Line 99, so no key can ever be"--help".🐛 Proposed fix
def run_cli(argv: list[str] | None = None) -> None: options = parse_args(argv or sys.argv[1:]) - if options.get("help") or options.get("--help"): + if options.get("help"): print_usage() raise SystemExit(0) - if not options.get("input"): + requested_path = options.get("input") + if not isinstance(requested_path, str) or not requested_path: print_usage() - fail("--input is required") - requested_path = options["input"] + fail("--input requires a file path") resolved_path = Path(requested_path).resolve()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def run_cli(argv: list[str] | None = None) -> None: options = parse_args(argv or sys.argv[1:]) if options.get("help"): print_usage() raise SystemExit(0) requested_path = options.get("input") if not isinstance(requested_path, str) or not requested_path: print_usage() fail("--input requires a file path") resolved_path = Path(requested_path).resolve() result = lint_xml(read_file(resolved_path), requested_path) print(json.dumps(result, ensure_ascii=False, indent=2)) if result["summary"]["error_count"] > 0: raise SystemExit(1)🧰 Tools
🪛 ast-grep (0.45.0)
[info] 2978-2978: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result, ensure_ascii=False, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.(use-jsonify)
🤖 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/scripts/xml_lint.py` around lines 2968 - 2981, Update run_cli to validate that options["input"] is a string/path value rather than merely truthy before constructing the Path, so a missing --input value is routed through fail and produces the standard user-facing error. Also remove the unreachable options.get("--help") check and rely on the normalized "help" key returned by parse_args.skills/lark-slides/scripts/xml_text_overlap_lint_test.py (1)
1-11: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Find how the lark-slides script tests are executed. set -uo pipefail rg -n 'xml_text_overlap_lint_test|xml_lint_test|unittest discover|pytest' \ --glob '!**/xml_lint_test.py' \ --glob '!**/xml_text_overlap_lint_test.py' \ -C 2 fd -t f -e yml -e yaml . .github 2>/dev/null | while IFS= read -r f; do rg -n -C 3 'lark-slides|unittest|pytest' "$f" && echo "--- $f" doneRepository: larksuite/cli
Length of output: 302
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== files ==" git ls-files | rg 'xml_(text_overlap_)?lint_test\.py$|skills/lark-slides/scripts' || true echo "== current shim ==" if [ -f skills/lark-slides/scripts/xml_text_overlap_lint_test.py ]; then cat -n skills/lark-slides/scripts/xml_text_overlap_lint_test.py fi echo "== target test outline/content sample ==" if [ -f skills/lark-slides/scripts/xml_lint_test.py ]; then wc -l skills/lark-slides/scripts/xml_lint_test.py ast-grep outline skills/lark-slides/scripts/xml_lint_test.py --view expanded || true sed -n '1,220p' skills/lark-slides/scripts/xml_lint_test.py fi echo "== discovery behavior probe ==" python3 - <<'PY' import importlib.util, tempfile, os, subprocess, pathlib, sys, textwrap sample = textwrap.dedent(''' def add(x, y=""): return x + y class MyTestCase: pass ''') with tempfile.TemporaryDirectory() as d: d = pathlib.Path(d) shim = d / "compat_test.py" target = d / "old_test.py" target.write_text(sample) shim.write_text(textwrap.dedent(''' import sys, os if __name__ == "__main__": target=__file__.replace("compat_test.py", "old_test.py") import sys, os os.execv(sys.executable, [sys.executable, target] + sys.argv[1:]) ''')) mod = importlib.util.module_from_spec(importlib.util.find_spec("importlib")) spec = importlib.util.spec_from_file_location("compat_test", shim) m = spec.loader.load_module() print("imported_compat_module=", repr(m)) print("module_attrs=", [a for a in dir(m) if not a.startswith("_")]) for runner_name, runner_cmd in [ ("shim_import_via_python_minus_i", [sys.executable, "-c", f'import compat_test; print("attrs", [k for k in dir(compat_test) if not k.startswith("_")])']), ("shim_direct", [sys.executable, str(shim), "-v"]), ]: try: out = subprocess.run(runner_cmd, cwd=d, text=True, capture_output=True, timeout=5) print(f"\n== {runner_name} ==") print("returncode=", out.returncode) print("stdout=\n", out.stdout) print("stderr=\n", out.stderr) except Exception as e: print(f"\n== {runner_name} exception ==") print(repr(e)) PYRepository: larksuite/cli
Length of output: 29945
Re-export the test cases instead of using
os.execv.This shim only delegates when the module is executed directly. Standard discovery imports the module instead, so it sees no
TestCasesubclasses or test functions. Re-export the original test names fromxml_lint_test; then discovery can run them while the legacy namespace remains available and compatibility can be covered by a separate regression test.🧰 Tools
🪛 Ruff (0.16.1)
[error] 11-11: Starting a process without a shell
(S606)
🤖 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/scripts/xml_text_overlap_lint_test.py` around lines 1 - 11, Replace the os.execv delegation in the module-level entry point with re-exports of the test cases and test functions from xml_lint_test, preserving their original names so standard test discovery finds them through the legacy module. Keep direct execution compatibility as needed, and remove the subprocess-only delegation approach.Source: Linters/SAST tools
c7f3994 to
bc71ddc
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
bc71ddc to
8e0622e
Compare
8e0622e to
1cb74f1
Compare
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Approved after re-review at 1cb74f1. The remaining compatibility concerns were reviewed and accepted by the maintainers.
2 participants
Summary
Reorganize the lark-slides skill reference files into logical subdirectories (cli/, xml/) and split monolithic lint scripts into focused modules. This improves discoverability and makes it easier for AI agents to navigate the skill's reference material.
Changes
Test Plan
Related Issues
Summary by CodeRabbit