fix(slides): detect visual elements outside canvas#1950
Conversation
📝 WalkthroughWalkthroughThe slide linter now extracts element rotation and performs rotation-aware canvas-bound validation for tables, charts, and text shapes, with element-specific diagnostics and expanded tests. ChangesGeneralized canvas validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SlideXML
participant lint_slide
participant detect_elements_out_of_canvas
participant Diagnostics
SlideXML->>lint_slide: extract elements with rotation
lint_slide->>detect_elements_out_of_canvas: validate elements against canvas
detect_elements_out_of_canvas->>Diagnostics: return bbox and overflow
Diagnostics->>lint_slide: emit element-specific lint issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.py`:
- Line 1114: Update the standalone-slide test around the shape defined in
xml_text_overlap_lint_test.py lines 206-220 to expect the shape_out_of_canvas
diagnostic produced by detect_elements_out_of_canvas. Keep the existing
standalone-slide assertions unchanged except for this expected error.
- Line 608: Update the rotation handling in element_canvas_bbox to validate the
extracted value with a finite-number check before passing it to math.sin or
math.cos. Preserve valid rotations, but replace inf, -inf, and other non-finite
values with 0 so linting continues and reports diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 525cd02f-d313-4a13-a970-bdf9768c00f7
📒 Files selected for processing (2)
skills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@4d3910bf3070d0cd353a4d0fb50cb8e2bfd84d81🧩 Skill updatenpx skills add larksuite/cli#feat/lint_outside_canvas -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1950 +/- ##
=======================================
Coverage 75.02% 75.02%
=======================================
Files 894 894
Lines 94294 94294
=======================================
Hits 70746 70746
Misses 18137 18137
Partials 5411 5411 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
41c89fb to
4d3910b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/lark-slides/scripts/xml_text_overlap_lint_test.py (1)
206-221: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest name no longer matches its own assertions.
test_lint_xml_single_slide_uses_default_canvas_without_bounds_checksnow assertserror_count == 1andcode == "shape_out_of_canvas"— i.e., bounds checks now fire on the default canvas, which is the opposite of what the method name promises. Left as-is, this will confuse future readers into thinking default-canvas bounds checking is still disabled.✏️ Suggested rename
- def test_lint_xml_single_slide_uses_default_canvas_without_bounds_checks(self) -> None: + def test_lint_xml_single_slide_uses_default_canvas_and_reports_out_of_canvas_shape(self) -> None:🤖 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 206 - 221, Rename the test method test_lint_xml_single_slide_uses_default_canvas_without_bounds_checks to reflect that the default canvas performs bounds checks and reports shape_out_of_canvas. Keep the test body and assertions 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.
Outside diff comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py`:
- Around line 206-221: Rename the test method
test_lint_xml_single_slide_uses_default_canvas_without_bounds_checks to reflect
that the default canvas performs bounds checks and reports shape_out_of_canvas.
Keep the test body and assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de44d709-36d4-48bc-a453-35d85f1d1a79
📒 Files selected for processing (2)
skills/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 (1)
- skills/lark-slides/scripts/xml_text_overlap_lint.py
Summary
Extend the Slides XML lint to report text shapes and charts that exceed the canvas. The reported bounds account for rotation while images and whiteboards continue to permit intentional bleed.
Changes
Test Plan
lark-cli <domain> <command>flow works as expected (not run; this change only updates the lint script and its unit tests)Related Issues
Summary by CodeRabbit
Bug Fixes
Tests