Conversation
|
@Keavon review please ! |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new PDF export feature, allowing users to save their documents in a widely used, high-quality format. The implementation leverages the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
mohan-bee
force-pushed
the
feat/pdf-export
branch
from
4d89422 to
b783a4b
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces PDF export functionality by adding FileType::Pdf to the export options, handling its MIME type, and disabling scaling in the UI. The core implementation involves new dependencies (krilla, krilla-svg, usvg-045) to convert internally generated SVG content into PDF bytes via a new svg_to_pdf function. The rendering pipeline is updated to treat PDF exports similarly to SVG exports for internal processing. A review comment suggests an improvement in the svg_to_pdf function to remove a redundant dimension validation check, replacing it with an expect() call for cleaner and slightly more performant code.
editor/src/node_graph_executor.rs
Outdated
| let page_height = size.y.max(1.) as f32; | ||
| let page_settings = PageSettings::from_wh(page_width, page_height).ok_or_else(|| "Invalid page dimensions for PDF export".to_string())?; | ||
|
|
||
| let mut page = document.start_page_with(page_settings); |
There was a problem hiding this comment.
The check for valid dimensions is redundant. PageSettings::from_wh on line 536 already validates that page_width and page_height are valid. This second check is unnecessary and can be replaced with expect() to make the code cleaner and slightly more performant.
| let mut page = document.start_page_with(page_settings); | |
| let krilla_size = krilla::geom::Size::from_wh(page_width, page_height).expect("Dimensions already validated by PageSettings"); |
|
!build (Run ID 22892952382) |
|
Do you think it would be possible to also support pdf export from the cli? |
|
@TrueDoctor |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/graphene-cli/src/export.rs">
<violation number="1" location="node-graph/graphene-cli/src/export.rs:240">
P1: Duplicate `svg_to_pdf` function implementation</violation>
<violation number="2" location="node-graph/graphene-cli/src/export.rs:251">
P2: When the user doesn't specify explicit width/height, the page defaults to 1920×1080 regardless of the SVG's actual content dimensions. Per the krilla-svg docs, the recommended pattern is to derive the page size from `tree.size()`. This avoids aspect-ratio distortion and content-page size mismatch.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Screen.Recording.2026-03-10.at.3.39.16.PM.mov |
|
Does this support multiple artboards becoming multiple pages (in the order of the artboards' layer stacking)? It would be nice to have, although depending on how much work it would be to implement, maybe a follow-on PR is better. You can decide. |
|
@Keavon No it will not support the multiple artboard in multiple pages, This pdf export just works from converting the svg to pdf ! It could be implemented but need some discussion on that to have my intuition clear, also obviously it requires a new PR ! |
|
@TrueDoctor @Keavon review pls ! |
|
@mohan-bee could we reuse the svg_pdf conversion logic in both places by putting the function into a module which is accessible by both? e.g. in the renderer crate |
sure ! |
Keavon
force-pushed
the
master
branch
2 times, most recently
from
3697b7d to
90533e6
Compare
mohan-bee
force-pushed
the
feat/pdf-export
branch
from
0a65b16 to
cc10866
Compare
|
!build (Run ID 23003707423) |
|
|
@TrueDoctor ready for review |
|
@Keavon @TrueDoctor I have implemented the multiple pages pdf export the "Artboard as pages" option will app appear only when it is pdf and all artwork. here is the demo : Screen.Recording.2026-03-13.at.2.03.27.AM.mov |
|
Nice! But instead of a checkbox, please make this one of the dropdown menu entries. |
There was a problem hiding this comment.
4 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/libraries/rendering/src/svg_to_pdf.rs">
<violation number="1" location="node-graph/libraries/rendering/src/svg_to_pdf.rs:35">
P2: Redundant SVG parsing causes performance regression: `svg_to_pdf` parses the SVG into a `usvg_045::Tree` to extract dimensions, then passes the raw string to `add_svg_page` which parses it again. Refactor `add_svg_page` to accept a `&usvg_045::Tree` parameter instead of `&str` to eliminate this duplicated computational work.</violation>
</file>
<file name="editor/src/node_graph_executor.rs">
<violation number="1" location="editor/src/node_graph_executor.rs:472">
P1: PDF page accumulator uses a non-unique key `(name, pdf_pages_total)` that fails to identify specific export sessions, causing state leaks on failures and potential file corruption when retrying exports. The key should include `document_id` and a unique session identifier to prevent mixing pages from different export attempts or concurrent exports of same-named documents.</violation>
</file>
<file name="editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs">
<violation number="1" location="editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs:187">
P3: Grammatical typo: 'Artboard' should be plural 'Artboards' since the feature exports multiple artboards as separate pages.</violation>
<violation number="2" location="editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs:187">
P2: Missing layout properties causing visual misalignment. The new 'Artboards as Pages' row lacks `.table_align(true)` and `.min_width(120)` on TextLabel, and uses `SeparatorStyle::Related` instead of `SeparatorStyle::Unrelated`. This causes horizontal misalignment compared to other dialog rows.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if is_multipage_pdf { | ||
| // Accumulate this artboard's SVG; emit the PDF once all pages arrive. | ||
| let page_size = size.as_dvec2(); | ||
| let key = (name.clone(), pdf_pages_total); |
There was a problem hiding this comment.
P1: PDF page accumulator uses a non-unique key (name, pdf_pages_total) that fails to identify specific export sessions, causing state leaks on failures and potential file corruption when retrying exports. The key should include document_id and a unique session identifier to prevent mixing pages from different export attempts or concurrent exports of same-named documents.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/node_graph_executor.rs, line 472:
<comment>PDF page accumulator uses a non-unique key `(name, pdf_pages_total)` that fails to identify specific export sessions, causing state leaks on failures and potential file corruption when retrying exports. The key should include `document_id` and a unique session identifier to prevent mixing pages from different export attempts or concurrent exports of same-named documents.</comment>
<file context>
@@ -455,19 +462,44 @@ impl NodeGraphExecutor {
+ if is_multipage_pdf {
+ // Accumulate this artboard's SVG; emit the PDF once all pages arrive.
+ let page_size = size.as_dvec2();
+ let key = (name.clone(), pdf_pages_total);
+ let pages = self.pdf_page_accumulator.entry(key.clone()).or_default();
+ pages.push((svg, page_size.x.max(1.) as f32, page_size.y.max(1.) as f32));
</file context>
editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs
Outdated
Show resolved
Hide resolved
editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs
Outdated
Show resolved
Hide resolved
|
updated ui: Screen.Recording.2026-03-13.at.11.14.32.AM.mov |
|
@TrueDoctor @Keavon review pls |
3 participants
In this PR I have implemented the PDF export feature closes #3372
demo:
Screen.Recording.2026-03-10.at.10.44.02.AM.mov