chore(excalidraw-skill): encode text-overlap + GitHub-SVG lessons; ad…#21
Merged
Merged
Conversation
…d rebuild script The recent dev-loop diagram exposed two failure modes the skill didn't guide against. Both got fixed twice (PR #20) instead of being prevented up front. This commit encodes the lessons so future diagrams ship right the first time. SKILL.md additions: Text Width Validation (Prevent Overlap) — new subsection in Layout Principles. Quantifies the issue: SVG <text> doesn't wrap; 150-char line at 14px = ~1260px wide and bleeds into adjacent columns if column width is ~840px. Includes a per-character-width table and the four ways out (shorten, multi-element wrap, single-column restructure, foreignObject). Cites the caught-in-the-wild example from PR #20's Gaps section. SVG Output for GitHub Embedding — new top-level section. Three rules for hand-written SVG to render correctly on GitHub: 1. explicit width + height (without it, GitHub downscales to ~50% and text becomes illegible) 2. white background <rect> as first child (without it, transparent background = invisible text on GitHub dark theme) 3. single-column for any text >80 chars (SVG has no auto-wrap) Plus guidance on when the .excalidraw and .svg drift (document it inline). Quality Checklist — bumped from 27 to 31 items, adding four explicit checks for the GitHub-SVG scenario. render_excalidraw.py changes: Path A (Playwright render) now writes a sibling .svg alongside the .png. New `_make_github_friendly()` helper post-processes Excalidraw's exportToSvg output to add the width/height attributes and white background rect — so Playwright-rendered SVGs land with the same GitHub-friendly shape that hand-written ones now require. Docstring + main() print line updated. .claude/scripts/rebuild-diagrams.sh (new): Walks docs/*.excalidraw, regenerates .png + .svg outputs for any source newer than its outputs. Skips up-to-date files unless --force. Single-file mode for targeted rebuilds. CI-friendly exit code on failure. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR adds GitHub-friendly SVG output alongside PNGs for Excalidraw renders, a bulk incremental rebuild script for ChangesSVG Output Generation and Automation
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 @.claude/scripts/rebuild-diagrams.sh:
- Around line 40-50: The find invocation in rebuild-diagrams.sh currently uses
-maxdepth 2 which limits discovery of .excalidraw files; update the TARGETS
population logic (the while/read block that uses find "$DOCS_DIR" -maxdepth 2
-name "*.excalidraw") to either remove the -maxdepth option so find searches
recursively, or make the depth configurable by introducing an environment
variable (e.g., MAX_DEPTH or EXCALIDRAW_MAXDEPTH) and substitute it into the
find command (falling back to no -maxdepth when unset); ensure you reference
DOCS_DIR and TARGETS so behavior remains the same and update the script header
comment to document the new behavior or variable.
- Around line 19-28: Add a fast-fail check that the external "uv" command is
available before attempting renders: after verifying RENDER_SCRIPT (and before
any per-file rendering loop), call a command-existence check (e.g., using
command -v or which) for "uv" and if it's not found emit a clear error like
"ERROR: required command 'uv' not found; please install or add to PATH" and exit
nonzero; update the script around the RENDER_SCRIPT validation to perform this
check so any missing dependency is reported immediately.
- Around line 66-71: The parameter expansion rel="${src#$REPO_ROOT/}" is
unquoted and triggers shellcheck SC2295; change the expansion to quote the
prefix so glob metacharacters in REPO_ROOT can't be interpreted — i.e., use
rel="${src#"$REPO_ROOT"/}" (adjusting the quoting around REPO_ROOT in the
${...#...} expression) to safely strip the repo root from src; update the line
that sets rel and run shellcheck to confirm the warning is resolved.
- Around line 78-84: The current command in the rebuild loop suppresses both
stdout and stderr (uv run python "$RENDER_SCRIPT" "$src" >/dev/null 2>&1),
hiding useful error output; change it to capture stderr to a temp file and only
suppress stdout so failures can print the error: create a temp file (mktemp)
before running the uv run python "$RENDER_SCRIPT" "$src" command, run it with
stdout redirected to /dev/null and stderr redirected into that temp file (e.g.,
>/dev/null 2>"$errfile"), on success remove the temp file and increment rebuilt,
on failure cat the temp file to echo the renderer error and increment failed,
and ensure you trap/cleanup the temp file (or fallback to simply only suppress
stdout by removing the 2>&1 redirection) — refer to RENDER_SCRIPT, the uv run
python invocation, and the rebuilt/failed counters to locate where to apply this
change.
In @.claude/skills/excalidraw-diagram/references/render_excalidraw.py:
- Around line 174-183: Validate the SVG string returned by page.evaluate before
post-processing and writing: check that svg_markup (from
page.evaluate("document.querySelector('`#root` svg').outerHTML")) is not None and
not an empty/whitespace-only string, and if it's invalid either log/raise an
error or skip writing the .svg file instead of passing it to
_make_github_friendly and svg_output_path.write_text; ensure any error includes
context (which page/selector) so callers can diagnose failures.
In @.claude/skills/excalidraw-diagram/SKILL.md:
- Line 436: Update the "Caught-in-the-wild example" paragraph (the line starting
"The cost of computing widths up front...") in SKILL.md to include a citation to
PR `#20` for the "Gaps" example; specifically append or insert a
parenthetical/reference like "(see PR `#20`)" or a markdown link to the PR after
the phrase that mentions the "Gaps" section so readers can find the full
context.
- Around line 494-503: Add blank lines before and after each fenced code block
so the triple-backtick fences are separated from surrounding text (fix MD031);
specifically, ensure there is an empty line before the "```xml" fence that opens
the <svg ...> example and an empty line after that closing "```", and likewise
add an empty line before and after the "```xml" fence that surrounds the <rect
width="1800" height="1500" fill="`#ffffff`"/> example so both fences are isolated
from neighboring paragraphs.
- Around line 629-642: Renumber the ordered lists under the "Visual Validation
(Render Required)" and "GitHub SVG (if the diagram is referenced from markdown)"
headings so the Visual Validation items are 1-7 (change current 22-27 to 2-7 and
make the first item 1) and the GitHub SVG items are 1-4 (change current 28-31 to
1-4); also insert a blank line immediately before the "GitHub SVG..." heading to
satisfy MD022 and ensure the file ends with a trailing newline to satisfy MD047.
Locate the two headings and the numbered list items in SKILL.md and update the
numeric prefixes accordingly, add the single blank line above the "GitHub SVG"
heading, and append a newline at EOF.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c81794c-414b-4e27-b734-195f23dce46b
📒 Files selected for processing (3)
.claude/scripts/rebuild-diagrams.sh.claude/skills/excalidraw-diagram/SKILL.md.claude/skills/excalidraw-diagram/references/render_excalidraw.py
All eight CodeRabbit findings on the initial commit were Quick-Win / Nit /
Minor severity — addressing each:
rebuild-diagrams.sh (4):
1. Add `uv` PATH check up front (was discovered at first render attempt,
hiding the real error in a redirect)
2. Drop `-maxdepth 2` from the find — discovers diagrams at any depth
under docs/ now (e.g., docs/a/b/c/diagram.excalidraw)
3. Quote $REPO_ROOT in parameter expansion: `${src#"$REPO_ROOT"/}` —
shellcheck SC2295, prevents accidental glob interpretation
4. Capture render output to a temp file instead of `>/dev/null 2>&1` —
on failure, print the last 20 lines of the actual error so the user
doesn't need to re-run manually to see what went wrong
render_excalidraw.py (1):
5. Guard `svg_markup` non-empty before writing the .svg sibling. Earlier
query_selector already proved the element exists, so this is defensive
— warn-and-continue on the unexpected empty-outerHTML case rather than
silently writing a broken SVG.
SKILL.md (3):
6. Add (PR #20) citation to the "Caught-in-the-wild example" so the
reference is concrete and grep-able
7. MD031 blank-lines-around-fences — added blank lines before/after the
three ```xml fences in "SVG Output for GitHub Embedding"
8. Restart ordered-list numbering at 1 in every Quality Checklist
subsection (was continuous 1-31 across all subsections — confused some
markdown parsers). Plus MD047 trailing newline on the file.
Not addressed: IDE's separate SonarLint warning about render() function
having cognitive complexity 19/15 — pre-existing on the function, not
flagged by CodeRabbit, and refactoring is scope creep.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
1 participant
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.
The recent dev-loop diagram exposed two failure modes the skill didn't guide against. Both got fixed twice (PR #20) instead of being prevented up front. This commit encodes the lessons so future diagrams ship right the first time.
SKILL.md additions:
Text Width Validation (Prevent Overlap) — new subsection in Layout Principles.
Quantifies the issue: SVG doesn't wrap; 150-char line at 14px = ~1260px
wide and bleeds into adjacent columns if column width is ~840px. Includes a
per-character-width table and the four ways out (shorten, multi-element wrap,
single-column restructure, foreignObject). Cites the caught-in-the-wild
example from PR #20's Gaps section.
SVG Output for GitHub Embedding — new top-level section. Three rules for
hand-written SVG to render correctly on GitHub:
1. explicit width + height (without it, GitHub downscales to ~50% and text becomes illegible)
2. white background as first child (without it, transparent background = invisible text on GitHub dark theme)
3. single-column for any text >80 chars (SVG has no auto-wrap) Plus guidance on when the .excalidraw and .svg drift (document it inline).
Quality Checklist — bumped from 27 to 31 items, adding four explicit checks
for the GitHub-SVG scenario.
render_excalidraw.py changes:
Path A (Playwright render) now writes a sibling .svg alongside the .png.
New
_make_github_friendly()helper post-processes Excalidraw's exportToSvgoutput to add the width/height attributes and white background rect — so
Playwright-rendered SVGs land with the same GitHub-friendly shape that
hand-written ones now require. Docstring + main() print line updated.
.claude/scripts/rebuild-diagrams.sh (new):
Walks docs/*.excalidraw, regenerates .png + .svg outputs for any source
newer than its outputs. Skips up-to-date files unless --force. Single-file
mode for targeted rebuilds. CI-friendly exit code on failure.
What changed
How it was built
Verification
Touches (check only if applies)
See CLAUDE.mdparaphrase (run/check-rulesto audit drift)Summary by CodeRabbit
New Features
Documentation