Skip to content

fix(slides): improve text overflow and roundtrip linting#1963

Merged
ethan-zhx merged 2 commits into
mainfrom
codex/text-shape-overflow-pr
Jul 21, 2026
Merged

fix(slides): improve text overflow and roundtrip linting#1963
ethan-zhx merged 2 commits into
mainfrom
codex/text-shape-overflow-pr

Conversation

@ethan-zhx

@ethan-zhx ethan-zhx commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Improve Slides XML linting for text-shape overflow while avoiding false positives from server-filled chart roundtrip attributes.

Changes

  • Estimate wrapped text height from font size, line spacing, paragraph spacing, and shape padding, then emit a text_may_overflow_shape warning when content may exceed its box.
  • Document the overflow guidance and update lint coverage.
  • Exempt chart.updated and chartData.isStaticData from SXSD attribute validation because they are populated during server roundtrips.

Test Plan

  • Unit tests pass (not run locally per repository machine-safety policy)
  • git diff --check origin/main...HEAD
  • Confirmed the PR branch is exactly two commits ahead of origin/main

Related Issues

  • None

Summary by CodeRabbit

  • New Features
    • Added improved diagnostics that warn when slide text is likely to overflow its shapes.
    • Enhanced XML text parsing for accurate line breaks, wrapping behavior, and paragraph/spacing metadata.
    • Extended XML attribute validation to correctly allow server-managed “roundtrip” attributes.
  • Documentation
    • Updated XML schema quick reference examples with revised autoFit and wrap settings.
  • Tests
    • Expanded tests for overflow warnings, wrapping/line-height calculations, preserved <br/> breaks, and attribute validation scenarios.

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The XML linter now handles tag-specific roundtrip attributes, preserves paragraph breaks and spacing metadata, reads formatting from inner content elements, and reports likely text overflow. XML reference examples add auto-fit and wrapping attributes, with expanded validation and diagnostic tests.

Changes

XML text linting and documentation

Layer / File(s) Summary
Content attribute examples
skills/lark-slides/references/xml-schema-quick-ref.md
Title, body, and note content examples now include autoFit; the title example also specifies wrap.
XML extraction and attribute validation
skills/lark-slides/scripts/xml_text_overlap_lint.py, skills/lark-slides/scripts/xml_text_overlap_lint_test.py
SXSD validation recognizes tag-specific roundtrip attributes; paragraph extraction preserves hard breaks and spacing metadata; text formatting is read from inner content attributes.
Text overflow detection and diagnostics
skills/lark-slides/scripts/xml_text_overlap_lint.py, skills/lark-slides/scripts/xml_text_overlap_lint_test.py
Text height estimation accounts for wrapping, hard breaks, line spacing, paragraph spacing, and padding, and lint_slide emits overflow warnings covered by tests.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant XMLShape
  participant lint_slide
  participant detect_text_may_overflow_shapes
  participant SlideIssues
  XMLShape->>lint_slide: extracted text and paragraph metadata
  lint_slide->>detect_text_may_overflow_shapes: text shapes
  detect_text_may_overflow_shapes->>SlideIssues: text_may_overflow_shape warnings
Loading

Possibly related PRs

  • larksuite/cli#1950: Modifies text-shape diagnostics and issue generation in the same linter.

Suggested reviewers: liangshuo-1, fangshuyu-768

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: text overflow handling and roundtrip linting improvements.
Description check ✅ Passed The description matches the template with Summary, Changes, Test Plan, and Related Issues, and contains the needed details.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/text-shape-overflow-pr

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the size/L Large or sensitive change across domains or core paths label Jul 20, 2026
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.00%. Comparing base (78390f8) to head (8dba8e3).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1963      +/-   ##
==========================================
+ Coverage   74.98%   75.00%   +0.01%     
==========================================
  Files         896      898       +2     
  Lines       94403    94912     +509     
==========================================
+ Hits        70789    71184     +395     
- Misses      18198    18269      +71     
- Partials     5416     5459      +43     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ethan-zhx
ethan-zhx marked this pull request as ready for review July 20, 2026 10:58
@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8dba8e3b352ba4807ad85b24fc6bed526322e04d

🧩 Skill update

npx skills add larksuite/cli#codex/text-shape-overflow-pr -y -g

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`:
- Around line 677-679: Update the fontSize extraction in the element
construction logic to use extract_numeric_attribute instead of directly
converting the selected attribute with float(). Preserve the existing precedence
between content_attrs and attrs, and retain 16 as the fallback when no numeric
value can be parsed so non-numeric fontSize values do not abort linting.
- Around line 811-813: Update the estimated-height calculation in the
paragraph-processing logic to use line_height for the first line as well as
subsequent lines, replacing the font_size contribution while preserving
before_spacing, after_spacing, and paragraph line-count handling. Ensure
multi-paragraph text advances by the full line_height and produces the expected
overflow warning.
🪄 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: 6b999786-052f-437f-a2f1-cbe54b20b40d

📥 Commits

Reviewing files that changed from the base of the PR and between 7b98994 and 08ee29c.

📒 Files selected for processing (4)
  • skills/lark-slides/references/xml-format-guide.md
  • skills/lark-slides/references/xml-schema-quick-ref.md
  • skills/lark-slides/scripts/xml_text_overlap_lint.py
  • skills/lark-slides/scripts/xml_text_overlap_lint_test.py

Comment thread skills/lark-slides/scripts/xml_text_overlap_lint.py Outdated
Comment thread skills/lark-slides/scripts/xml_text_overlap_lint.py
@ethan-zhx
ethan-zhx force-pushed the codex/text-shape-overflow-pr branch from 08ee29c to f339ba7 Compare July 20, 2026 11:35

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 665-681: Update the text-height extraction around the element
update and its fontSize calculation to inspect inline span fontSize values
within each paragraph, carry the maximum discovered span size forward, and use
it before the content/shape fallback and default of 16. Preserve existing
behavior when no inline span specifies a valid font size.
🪄 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: e9f02fc7-d899-4d22-9030-9cb7992db0e1

📥 Commits

Reviewing files that changed from the base of the PR and between 08ee29c and f339ba7.

📒 Files selected for processing (3)
  • skills/lark-slides/references/xml-schema-quick-ref.md
  • skills/lark-slides/scripts/xml_text_overlap_lint.py
  • skills/lark-slides/scripts/xml_text_overlap_lint_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • skills/lark-slides/references/xml-schema-quick-ref.md
  • skills/lark-slides/scripts/xml_text_overlap_lint_test.py

Comment thread skills/lark-slides/scripts/xml_text_overlap_lint.py
R0bynZhu
R0bynZhu previously approved these changes Jul 20, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
skills/lark-slides/scripts/xml_text_overlap_lint.py (1)

665-682: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

fontSize still not read from inline <span fontSize="...">.

fontSize is only sourced from <content fontSize> then shape-level attrs; inline <span fontSize="..."> inside <p> is still dropped, defaulting to 16. This is a repeat of a prior review comment and is now more consequential: the new detect_text_may_overflow_shapes overflow estimator depends on element["fontSize"], and this file's own fixture samples (image-led-cover, content-grid, lines 45-73) exclusively use span-level fontSize (e.g. <span fontSize="42">) with no content-level fallback — so overflow detection will silently use the wrong font size for this common authoring pattern.

🛡️ Suggested approach
-                content_attrs = extract_tag_attributes(content, "content")
-                font_size = extract_numeric_attribute(content_attrs, "fontSize")
-                if font_size is None:
-                    font_size = extract_numeric_attribute(attrs, "fontSize")
+                content_attrs = extract_tag_attributes(content, "content")
+                font_size = extract_numeric_attribute(content_attrs, "fontSize")
+                if font_size is None:
+                    font_size = extract_numeric_attribute(attrs, "fontSize")
+                if font_size is None:
+                    span_font_sizes = [
+                        extract_numeric_attribute(span_attrs, "fontSize")
+                        for span_attrs in re.findall(r"<span\b([^>]*)>", content)
+                    ]
+                    span_font_sizes = [size for size in span_font_sizes if size is not None]
+                    if span_font_sizes:
+                        font_size = max(span_font_sizes)
🤖 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 665 - 682,
Update the fontSize extraction in the element-building flow around
extract_tag_attributes and element.update to inspect inline span fontSize values
within the paragraph content before falling back to content-level and
shape-level attributes. Ensure span-level fontSize populates element["fontSize"]
for samples without higher-level values, while retaining the existing fallback
to 16 when no font size is available.
🤖 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.

Duplicate comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 665-682: Update the fontSize extraction in the element-building
flow around extract_tag_attributes and element.update to inspect inline span
fontSize values within the paragraph content before falling back to
content-level and shape-level attributes. Ensure span-level fontSize populates
element["fontSize"] for samples without higher-level values, while retaining the
existing fallback to 16 when no font size is available.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f8d26a0e-1420-40af-8301-c7a872b83baa

📥 Commits

Reviewing files that changed from the base of the PR and between f339ba7 and 8dba8e3.

📒 Files selected for processing (3)
  • skills/lark-slides/references/xml-schema-quick-ref.md
  • skills/lark-slides/scripts/xml_text_overlap_lint.py
  • skills/lark-slides/scripts/xml_text_overlap_lint_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • skills/lark-slides/references/xml-schema-quick-ref.md

Comment thread skills/lark-slides/scripts/xml_text_overlap_lint.py
@ethan-zhx
ethan-zhx merged commit 6675e3c into main Jul 21, 2026
46 of 49 checks passed
@ethan-zhx
ethan-zhx deleted the codex/text-shape-overflow-pr branch July 21, 2026 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants