feat(slides): add layout density lint for sparse/empty containers #2022
Merged
Conversation
- add XML-based sparse container detection with coverage metrics - return structured, actionable lint JSON - document lint usage and visual-review workflow - add regression tests for density-lint behavior
- add XML-based sparse container detection with coverage metrics - return structured, actionable lint JSON - document lint usage and visual-review workflow - add regression tests for density-lint behavior
|
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 slide XML lint now performs broader layout validation, emits schema-versioned diagnostics with slide statuses, and applies error/warning release gates. Documentation and tests cover out-of-canvas, sparse-content, blank-slide, and warning-level results. ChangesSlide XML layout lint
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant XML as Slide XML
participant Lint as lint_xml
participant Rules as Layout rules
participant Result as build_result
XML->>Lint: Parse slide XML
Lint->>Rules: Run canvas, overlap, density, and blank-slide checks
Rules-->>Lint: Return lint issues and measurements
Lint->>Result: Normalize issues and compute statuses
Result-->>Lint: Return structured release-gate result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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@d87ea4a6bab8e412eb7d7a73697abab0237f35dd🧩 Skill updatenpx skills add larksuite/cli#fix/slides_lint_empty -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: 2
🤖 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/scripts/xml_text_overlap_lint_test.py`:
- Around line 233-237: Rename the test method
test_lint_xml_single_slide_uses_default_canvas_without_bounds_checks to reflect
that single-slide linting reports both shape_out_of_canvas and blank_slide
errors, while preserving its existing assertions and behavior.
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 1499-1503: Guard the float conversion in the declared_font_sizes
comprehension within the overlap-lint parsing flow, matching the existing base
font-size parsing behavior: ignore descendant fontSize values that raise
ValueError instead of allowing them to escape as uncaught exceptions. Preserve
collection of valid numeric sizes and the existing XmlLayoutLintError handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00b04a3d-9300-4c50-88e2-14430d9f17f9
📒 Files selected for processing (5)
skills/lark-slides/SKILL.mdskills/lark-slides/references/troubleshooting.mdskills/lark-slides/references/validation-checklist.mdskills/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 #2022 +/- ##
==========================================
+ Coverage 75.06% 75.13% +0.06%
==========================================
Files 902 905 +3
Lines 95944 96130 +186
==========================================
+ Hits 72025 72224 +199
+ Misses 18380 18348 -32
- Partials 5539 5558 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Skip non-numeric fontSize attributes on descendant nodes instead of letting float() raise uncaught, matching the existing base_font_size guard. Also rename a test whose name claimed no bounds checks occur while its own assertions verify shape_out_of_canvas and blank_slide errors are reported.
fangshuyu-768
left a comment
fangshuyu-768
left a comment
Collaborator
There was a problem hiding this comment.
I found four actionable issues in the new layout-lint release gate. The existing test suite passes, but these cases leave gaps in the advertised detection and schema-v2 contract.
fangshuyu-768
left a comment
fangshuyu-768
left a comment
Collaborator
There was a problem hiding this comment.
Re-review of the latest head: the rotation and schema-v2 hint issues are fixed. The alpha and overlap-measurement fixes are only partial, and the expanded regression pass found three additional gaps.
… gaps in layout lint Support single-quoted XML attributes and stop self-closing shape/table tags from absorbing a later element's content in extract_elements. Extract polyline geometry so it participates in out-of-canvas and blank/sparse content checks. Fix has_matching_image_overlay to divide by container area instead of min(container, image), so small images no longer suppress sparse-container warnings. Require layout containers and nested-panel exemptions to be visually rendered (alpha > 0). Measure bbox_overlap against the visual text bbox instead of the declared shape box.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 83-86: Update the attribute-matching regex in the relevant helper
to allow optional whitespace around the equals sign while preserving support for
both single- and double-quoted values. Keep the existing match-group selection
and None behavior unchanged so geometry extraction continues to work for compact
and spaced XML attributes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00f41e95-b13f-4613-9eba-af3e1a3e499b
📒 Files selected for processing (1)
skills/lark-slides/scripts/xml_text_overlap_lint.py
…in sync with the decision - Count a line's rendered stroke instead of its declared (often zero-area) bounding box - Report bbox_overlap measurements using the same element objects should_flag_overlap decided with, instead of a separately re-parsed set whose fontSize can diverge - Exclude the element itself from has_similar_short_card_peer's peer count - Drop the dead schema_version "1.0" literals that normalize_issue always overwrites to "2.0" - Split blank_slide's visibility check into its own permissive is_slide_content_present (any rendered element counts, including textless decorative shapes), keeping slide_content_visual_bbox's stricter density bar for sparse_slide_content/sparse_container_content so a full-bleed background rect still can't trivially satisfy them - Exempt the server-echoed <chartParsedValues> roundtrip tag from sxsd_unsupported_tag - Tolerate sub-pixel canvas overflow from rotated-bbox floating-point rounding
…st-peer false negatives - Accept optional whitespace around attribute "=" in extract_attribute (e.g. `topLeftX = "80"`), which previously dropped the element's geometry entirely and could misfire blank_slide - Treat a full-canvas plain rect with no text as a background panel, not content, so a slide with only a background fill still reports blank_slide instead of bypassing it - Ignore invisible (alpha=0) rects when counting has_similar_short_card_peer, so ghost siblings can no longer promote a real card into an unwanted sparse_container_content warning
fangshuyu-768
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
summary gained status, release_ready, and screenshot_review_required fields
table for the newly added issue codes
Test plan
paths for image-overlay cards and edge-spanning layout panels, out-of-canvas detection across all element kinds
Summary by CodeRabbit