Skip to content

Comments

Add SVG and PNG rendering capabilities to projects#276

Merged
bpowers merged 3 commits intomainfrom
claude/expose-pdf-api-engine-uWlkX
Feb 20, 2026
Merged

Add SVG and PNG rendering capabilities to projects#276
bpowers merged 3 commits intomainfrom
claude/expose-pdf-api-engine-uWlkX

Conversation

@bpowers
Copy link
Owner

@bpowers bpowers commented Feb 20, 2026

Summary

This PR adds native SVG and PNG rendering functionality to the Simlin engine, allowing projects to render their stock-and-flow diagrams directly without requiring external diagram processing pipelines.

Key Changes

Core Rendering Implementation

  • Engine (TypeScript/WASM): Added simlin_project_render_png() FFI function to render diagrams as PNG images with optional width/height parameters
  • Python bindings: Implemented render_svg(), render_svg_string(), and render_png() methods on the Project class with proper thread-safety and error handling
  • Worker protocol: Extended worker backend to support PNG rendering requests with width/height parameters

Server Simplification

  • Removed complex rendering pipeline: Eliminated the multi-step process (JSON → DataModel → SVG → PNG conversion) that required separate diagram and core packages
  • Simplified render.ts: Now directly calls engineProject.renderPng() instead of deserializing to JSON, converting to DataModel, rendering SVG, and using worker threads
  • Removed dependencies: Deleted render-inner.ts and render-worker.ts files; removed @simlin/diagram package dependency from server

Testing

  • Python tests: Added comprehensive test suite covering SVG/PNG rendering, dimension validation, aspect ratio preservation, and error handling
  • TypeScript tests: Added tests for both high-level API and direct backend rendering with dimension verification
  • Integration tests: Updated preview rendering tests to use the new native rendering instead of the old pipeline

Implementation Details

  • PNG rendering supports flexible sizing: intrinsic dimensions (0,0), explicit width/height, or width-only with aspect ratio preservation
  • All rendering operations are thread-safe in Python via existing lock mechanisms
  • PNG output is validated by checking file signatures and parsing IHDR chunks for dimension verification
  • The rendering is now performed entirely within the engine, eliminating the need for external SVG-to-PNG conversion tools in the server

https://claude.ai/code/session_012WrnfoGCS23uqHEkhxyxFX

The recent commit introduced PNG rendering in libsimlin's C FFI.
This change threads that capability through the full stack:

- src/engine: adds simlin_project_render_png WASM binding,
  EngineBackend.projectRenderPng, DirectBackend/WorkerBackend
  implementations, worker protocol message, and Project.renderPng()
- src/server: replaces the resvg-wasm + worker thread pipeline with
  a single call to the engine's renderPng, removing render-inner.ts,
  render-worker.ts, and the resvg-wasm dependency
- src/pysimlin: adds render_svg/render_png FFI helpers and
  Project.render_svg/render_svg_string/render_png methods

Tests cover PNG signature validation, dimension scaling with aspect
ratio preservation, and error handling for nonexistent models.

https://claude.ai/code/session_012WrnfoGCS23uqHEkhxyxFX
@bpowers
Copy link
Owner Author

bpowers commented Feb 20, 2026

@codex review

@claude
Copy link

claude bot commented Feb 20, 2026

Code Review

I reviewed the full diff across all 21 changed files covering the Rust FFI, TypeScript WASM bindings, worker protocol, Python bindings, server simplification, and all test files.

Findings

No bugs found.

Notes

  • The new simlin_project_render_png FFI function in import-export.ts follows the exact same allocation/error-handling/cleanup pattern as the existing simlin_project_render_svg, including proper free() calls in the finally block and correct error propagation via SimlinError.
  • The worker protocol (worker-protocol.ts, worker-server.ts, worker-backend.ts) is correctly wired end-to-end: the new projectRenderPng message type is added to WorkerRequest, VALID_REQUEST_TYPES, the WorkerServer dispatch, and the WorkerBackend sender.
  • The Python FFI functions (render_svg, render_png) properly use check_out_error before accessing the output buffer and free the Rust-allocated buffer in a finally block. Thread safety is handled via self._lock in the Project methods.
  • The server's render.ts now correctly uses try/finally for engineProject.dispose(), which is an improvement over the old code where dispose() was called mid-pipeline before the PNG was generated.
  • The preview scaling behavior changed from max-dimension-based (old render-inner.ts scaled so the largest dimension fit in 800px) to width-based (new code always sets width=800), but this is clearly intentional given the updated tests.

Overall Correctness

Correct. The patch is well-structured, follows established patterns, and includes comprehensive tests across all layers. Existing code and tests should not break.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 009e05985c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

}
});
try {
return await engineProject.renderPng('main', PREVIEW_WIDTH);

Choose a reason for hiding this comment

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

P1 Badge Scale previews by max dimension to avoid oversized PNGs

The new preview path always calls renderPng('main', PREVIEW_WIDTH) with width-only scaling, which makes portrait/tall diagrams render at very large heights because the engine preserves aspect ratio from width. The previous implementation scaled by the larger SVG dimension, so outputs stayed bounded near the preview target; with this change, tall models can produce much larger PNG payloads (and potentially fail rendering for extreme aspect ratios), increasing DB/storage and response size for /preview in production.

Useful? React with 👍 / 👎.

@codecov
Copy link

codecov bot commented Feb 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.52%. Comparing base (0402644) to head (9a39787).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #276   +/-   ##
=======================================
  Coverage   76.52%   76.52%           
=======================================
  Files         118      118           
  Lines       27473    27473           
=======================================
  Hits        21023    21023           
  Misses       6450     6450           

☔ View full report in Codecov by Sentry.
📢 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.

The previous change always constrained by width, which meant
portrait/tall diagrams could produce very large PNGs. This
matches the original resvg-wasm behavior: scale by the larger
of width/height so neither dimension exceeds the target.

Extracts previewDimensions() and parseSvgDimensions() as pure
functions with dedicated unit tests covering landscape, portrait,
square, edge cases, and extreme aspect ratios.

https://claude.ai/code/session_012WrnfoGCS23uqHEkhxyxFX
@claude
Copy link

claude bot commented Feb 20, 2026

Code Review

Reviewed all 21 changed files in this PR. The change replaces the multi-step server-side rendering pipeline (engine → JSON → DataModel → SVG via @simlin/diagram → PNG via resvg-wasm) with direct engine WASM calls for both SVG and PNG rendering, and adds the corresponding Python bindings.

Analysis

Engine TypeScript bindings (backend.ts, direct-backend.ts, import-export.ts, project.ts, worker-backend.ts, worker-protocol.ts, worker-server.ts): The new projectRenderPng FFI follows the exact same pattern as the existing projectRenderSvg — proper memory allocation via allocOutPtr/allocOutUsize, error checking, copyFromWasm, and cleanup in finally. The worker protocol and server dispatch are consistent with all existing request types.

Server simplification (render.ts): The new previewDimensions correctly clamps the larger dimension to maxSize and derives the smaller dimension to preserve aspect ratio. Math.ceil on the derived dimension can never exceed maxSize because the ratio svgSmaller/svgLarger is ≤ 1. The parseSvgDimensions regex handles the engine's own SVG output (always double-quoted viewBox), and the fallback to {0, 0} (intrinsic dimensions) is a safe default. The try/finally with dispose() in renderToPNG is an improvement over the old code, which lacked a worker.on('error') handler.

Python bindings (_ffi.py, project.py): Both render_svg and render_png follow the existing FFI patterns with proper check_out_error and simlin_free in finally. The project.py methods correctly use the _lock for thread safety. The C header (simlin.h) declares simlin_project_render_png without a #if defined(...) guard, so _ffi_build.py's header-to-cdef transform will include it.

Dependency cleanup: @simlin/diagram and resvg-wasm are correctly removed from package.json, pnpm-lock.yaml, and tsconfig.json references.

Tests: Good coverage across direct-backend, high-level API, Python rendering, preview dimension logic, and SVG parsing. The PNG signature and IHDR dimension verification is a solid approach.

Findings

No bugs found.

Overall Correctness

Correct. The patch is clean, follows existing patterns consistently, has proper error handling and resource cleanup, and is well-tested. Existing code and tests should not break.

@bpowers
Copy link
Owner Author

bpowers commented Feb 20, 2026

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b5a79a7b60

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const svg = await engineProject.renderSvgString('main');
const intrinsic = parseSvgDimensions(svg);
const dims = previewDimensions(intrinsic.width, intrinsic.height, MAX_PREVIEW_SIZE);
return await engineProject.renderPng('main', dims.width, dims.height);

Choose a reason for hiding this comment

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

P2 Badge Pass only one constrained dimension to renderPng

renderToPNG computes bounded {width,height} but then passes both into renderPng; the new PNG API gives precedence to non-zero width, so the height bound is ignored. For portrait diagrams where previewDimensions rounds width up with Math.ceil, this can produce previews larger than MAX_PREVIEW_SIZE (e.g., intrinsic 101x2000 gives dims 41x800, but width-precedence rendering yields height 812). That breaks the size cap guarantee and can increase preview storage/transfer costs for narrow/tall models.

Useful? React with 👍 / 👎.

The engine gives width precedence when both width and height are
non-zero, which means the height bound is silently ignored for
portrait diagrams. For example, intrinsic 101x2000 with max 800
previously computed {width: 41, height: 800}, but the engine used
width=41 and derived height=812, exceeding the limit.

previewDimensions now returns only the constraining dimension as
non-zero (the other as 0), letting the engine derive the
unconstrained dimension from the aspect ratio.

https://claude.ai/code/session_012WrnfoGCS23uqHEkhxyxFX
@claude
Copy link

claude bot commented Feb 20, 2026

Code Review

Reviewed the full diff adding native SVG/PNG rendering to the engine and simplifying the server rendering pipeline.

Findings

No bugs found.

Notes

  • The new simlin_project_render_png FFI wrapper in import-export.ts correctly follows the existing simlin_project_render_svg pattern: allocate out-pointers, call the WASM function, check for errors, copy the result buffer, and free all pointers in finally.
  • previewDimensions correctly works around the documented engine behavior where width takes precedence when both dimensions are non-zero, by only passing one non-zero dimension. The test coverage for this is thorough.
  • parseSvgDimensions handles edge cases (missing viewBox, malformed values, extra whitespace) gracefully by falling back to {0, 0}, which then causes previewDimensions to return {0, 0} (intrinsic sizing).
  • The old renderToPNG had a latent issue where worker.on('error') was never registered, meaning worker failures would leave the promise hanging. The new code uses try/finally with proper error propagation, which is an improvement.
  • Resource cleanup is correct: engineProject.dispose() is always called in finally blocks in both render.ts and the test helpers.
  • Python bindings follow the existing lock/_check_alive pattern and the _ffi layer properly frees the output buffer after copying.

Overall Correctness

Correct. The patch follows existing patterns consistently, has proper resource cleanup, and introduces no bugs. Tests cover the new functionality including edge cases and error paths.

@bpowers bpowers merged commit 521fd8f into main Feb 20, 2026
9 checks passed
@bpowers bpowers deleted the claude/expose-pdf-api-engine-uWlkX branch February 20, 2026 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants