fix(exporters): preserve local assets in exports#290
Conversation
There was a problem hiding this comment.
Review mode: initial
Findings
No Blocker or Major issues found. The PR is well-structured, adds comprehensive unit tests, respects project constraints (no new deps, lazy-loading, path traversal protection), and implements the described functionality correctly.
Residual observations
- Minor / Nit: The asset inlining and rewriting functions (
inlineLocalAssetsInHtml,rewriteHtmlLocalAssetReferences) use regex-based scanning forurl()references across the entire HTML string. This works for typical cases but may produce false positives or miss references inside escaped content, HTML comments, or<style>blocks with unusual formatting. For a v1 this is acceptable; a future enhancement could use a proper CSS parser for inline<style>content. - Minor: The
exportAssetOptionsfunction inexporter-ipc.tssetsassetBasePathtopath.resolve(workspacePath, sourceDir). IfsourcePathis not provided,assetBasePathdefaults toworkspacePath. Path traversal is guarded byisInsideRootinassets.ts, so this is safe. However, consider documenting thatsourcePathshould be relative to workspace to avoid confusion. - Test coverage: Good. The new
assets.test.tscovers inlining, collecting, and rewriting. Existing tests for PDF, ZIP, HTML, PPTX, Markdown are extended. Missing: no test verifies thatinlineLocalAssetsInHtmlcorrectly skips remote URLs (e.g.,https://...or//cdn.com/...). TheisLocalReferencefunction does this, but a quick test would be nice. Not a blocker.
Linked issue
The PR body says "Addresses #284" (not "Closes"), which correctly reflects that the changes improve exporter fidelity for workspace-local assets. The diff satisfies the described scope. No closure claim to validate.
Summary
This is a substantial improvement to the exporter pipeline: it adds a shared workspace-local asset resolver, inlines assets for HTML/PDF, auto-bundles for ZIP, renders PPTX slides as Chrome screenshots by default (with fallback to editable mode), preserves Markdown tables, and exposes PDF header/footer/settle options. The IPC layer is extended to pass workspace context from the renderer. All changes follow project conventions (lazy-loading, path containment checks, no new prod deps). The PR is ready to merge after the two minor notes above are reviewed.
Testing
pnpm lintpasses on changed files.pnpm typecheckpasses.pnpm testpasses. All new tests are file-system isolated and clean up after themselves.- Not run: end-to-end Playwright tests (not applicable to this change set).
Open-CoDesign Bot
e9173b5 to
5078646
Compare
There was a problem hiding this comment.
Review mode: follow-up after new commits
This follow-up review confirms the changes are identical to those reviewed in the previous review (commit e9173b58 → 5078646e4a). The diff content is unchanged; only the commit SHA has updated (likely a rebase or amended commit with no functional change).
Findings
No new issues have been introduced. All residual observations from the previous review (regex-based scanning edge cases, documentation of assetBasePath derivation, lack of a test for remote URL skipping) remain applicable but are non-blocking.
Summary
The code is unchanged from the previous review. The implementation is sound, tests pass, constraints are respected, and the linked issue (#284) is correctly referenced as "Addresses" (not closed). The PR is ready to merge.
Testing
No new test changes since the previous review. All existing tests pass.
Open-CoDesign Bot
Summary
src,href,poster,srcset, and CSSurl()).assets/...references.Addresses #284
Testing
pnpm exec biome check apps/desktop/src/main/exporter-ipc.ts apps/desktop/src/main/exporter-ipc.test.ts apps/desktop/src/preload/index.ts apps/desktop/src/renderer/src/store/slices/generation.ts packages/exporters/src/assets.ts packages/exporters/src/assets.test.ts packages/exporters/src/html.ts packages/exporters/src/html.test.ts packages/exporters/src/index.ts packages/exporters/src/markdown.ts packages/exporters/src/markdown.test.ts packages/exporters/src/pdf.ts packages/exporters/src/pdf.test.ts packages/exporters/src/pptx.ts packages/exporters/src/pptx.test.ts packages/exporters/src/zip.ts packages/exporters/src/zip.test.ts .changeset/export-fidelity-assets.mdpnpm typecheckpnpm testLocal note
pnpm lintis blocked in this checkout by an unrelated untrackedissues_summary.jsonformatting issue. The changed files pass Biome, and CI should run on a clean checkout.