Fix SVG dimension bug - apply configured width/height to SVG output#25
Fix SVG dimension bug - apply configured width/height to SVG output#25man8 wants to merge 10 commits into
Conversation
- Modified parseSvgConfig to include variant width/height in SVG configuration - Updated applySvgAttributes to apply dimensions to SVG root element - Added comprehensive test suite for SVG dimension handling - Fixes issue where SVG assets ignored explicit dimension configuration The fix ensures SVG assets respect the same dimension configuration as PNG assets by: - Including variant width/height in the SVG configuration object - Applying these dimensions as standard SVG width/height attributes - Preserving existing functionality like preserveAspectRatio and viewBox - Following existing code patterns for SVG attribute manipulation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Fix Bug 1: Replace flawed includes('width=') logic with robust regex patterns that target SVG root element specifically, preventing false matches with stroke-width attributes
- Fix Bug 2: Correct dimension prioritisation in parseSvgConfig() to ensure variant-level width/height takes precedence over variant.svg properties
- Add comprehensive tests for both bugs including stroke-width false positive scenarios and dimension prioritisation validation
- All 8 SVG dimension tests passing, linting clean
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…bute handling - Replace fragile regex-based attribute manipulation with DOM-based approach - Eliminate false positives from stroke-width matching width= attributes - Add comprehensive error handling with fallback to original content - Add test coverage for malformed SVG parsing scenarios - Maintain existing svgo optimization pipeline integration - All 82 tests passing with improved robustness
- jsdom is used in production code (applySvgAttributes method) - prevents build failures in production environments - maintains @types/jsdom in devDependencies for TypeScript compilation
There was a problem hiding this comment.
Bug: SVG Parsing Overwrites Dimensions Incorrectly
The parseSvgConfig method incorrectly overrides width and height values from variant.svg with undefined when variant.width or variant.height are not explicitly set. This occurs because width: variant.width and height: variant.height are unconditionally assigned after spreading variant.svg, preventing svg-level dimensions from being used as fallbacks. This behavior is inconsistent with how other properties handle fallbacks.
src/processors/image-processor.ts#L560-L563
assetmill/src/processors/image-processor.ts
Lines 560 to 563 in 4a6498d
Was this report helpful? Give feedback by reacting with 👍 or 👎
- Preserve original viewBox when applying new dimensions to ensure content scales properly - Configure SVGO to not remove viewBox attributes during optimization - Add comprehensive test cases for viewBox preservation and explicit viewBox handling - Fixes issue where SVGs only changed container size without scaling content The core issue was that SVGO's preset-default configuration was removing viewBox attributes before the applySvgAttributes method could preserve them. This caused SVGs to only change their container dimensions (width/height) without scaling the actual content, resulting in margin adjustments rather than proper scaling.
|
@blocks Resolve the merge conflicts |
|
Merge conflicts resolved and pushed. The conflicts were in |
|
❌ Cannot revive Devin session - the session is too old. Please start a new session instead. |
Keeps jsdom and @types/jsdom from PR branch alongside upgraded fs-extra, glob, js-yaml, and @types/node versions from main.
📝 WalkthroughWalkthroughThis pull request fixes SVG dimension attribute handling by replacing error-prone regex replacements with jsdom-based DOM manipulation. Configuration merging precedence is updated to prioritise ChangesSVG Dimension Handling Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
66-66: ⚡ Quick winConsider upgrading
@types/jsdomto align with the runtime library version.The gap between
jsdom(26.1.0) and@types/jsdom(21.1.7) is larger than typical, though this is normal since@typespackages are maintained separately by DefinitelyTyped and updated asynchronously. Newer versions exist (@types/jsdom28.0.1 as of March 2026) that are fully compatible withjsdom26.1.0 and would provide better alignment. No security advisories apply tojsdom26.1.0.🤖 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 `@package.json` at line 66, Update the `@types/jsdom` devDependency to a version aligned with the runtime jsdom package (replace the existing "@types/jsdom" entry with a newer compatible release such as "^28.0.1") so type definitions match "jsdom": "26.1.0"; after modifying the package.json entry for "@types/jsdom" run your package manager (npm install or yarn install) to refresh the lockfile and verify TypeScript compiles cleanly and tests pass (targets: the package.json dependency "@types/jsdom" and the runtime "jsdom" reference).
🤖 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.
Nitpick comments:
In `@package.json`:
- Line 66: Update the `@types/jsdom` devDependency to a version aligned with the
runtime jsdom package (replace the existing "@types/jsdom" entry with a newer
compatible release such as "^28.0.1") so type definitions match "jsdom":
"26.1.0"; after modifying the package.json entry for "@types/jsdom" run your
package manager (npm install or yarn install) to refresh the lockfile and verify
TypeScript compiles cleanly and tests pass (targets: the package.json dependency
"@types/jsdom" and the runtime "jsdom" reference).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39d9ea42-c245-4fe7-a022-dc2a0d3b6a17
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
CHANGELOG.mdpackage.jsonsrc/__tests__/svg-dimensions.test.tssrc/processors/image-processor.ts
2 participants
Fix SVG viewBox preservation for proper content scaling
Summary
This PR fixes a critical bug where SVGs were only getting container dimension changes (width/height attributes) but not actual content scaling. The issue occurred because SVGO optimization was removing
viewBoxattributes before they could be preserved, causing content to remain at original coordinates while only container dimensions changed.Root cause: SVGO's
preset-defaultconfiguration includesremoveViewBox: true, which stripped viewBox attributes during optimization, preventing proper content scaling.Solution:
removeViewBoxin preset overridesapplySvgAttributesmethod to preserve original viewBox when applying new dimensionsReview & Testing Checklist for Human
Recommended test plan: Use the test SVG in
/tmp/svg-test/and generate variants with different dimensions to visually confirm content scales rather than just adding margins.Diagram
%%{ init : { "theme" : "default" }}%% graph TB CLI["cli/index.js"] IP["src/processors/<br/>image-processor.ts"]:::major-edit Tests["src/__tests__/<br/>svg-dimensions.test.ts"]:::minor-edit SVGO["SVGO Optimization"] Output["SVG Output Files"] CLI --> IP IP --> SVGO SVGO --> Output Tests --> IP IP -- "processSvgOutput:<br/>Configure SVGO to preserve viewBox" --> SVGO IP -- "applySvgAttributes:<br/>Preserve original viewBox" --> Output subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#ADD8E6 classDef context fill:#FFFFFFNotes
removeViewBoxin SVGO may result in slightly larger file sizes, but this is necessary for proper scaling functionalitySession requested by: Louis Mandelstam (@man8)
Devin session: https://app.devin.ai/sessions/af8a710843d14c529ba7cbb4f4981b57
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Chores