feat: add session PDF and image export#3572
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
c96651a to
26cf155
Compare
esengine
left a comment
There was a problem hiding this comment.
Really nice — rolling the PDF writer by hand (raster XObject + per-page CM offset for pagination) and rasterizing via SVG foreignObject instead of pulling in html2canvas + jspdf keeps us dependency-free, which is exactly the right call for this project. The AppBindings interface + mock are both updated, both locales are in sync, safeExportFilename guards path traversal, and the honest note about the unrelated pre-existing test failure is appreciated.
Merging. Two follow-ups worth a quick pass later (neither blocks):
-
Surface export failures to the user. The whole export is wrapped in
catch (err) { console.error(...) }, so on a user-initiated action a failure is invisible — they click Export and nothing happens. A small notice/toast on failure would fit our "don't silently swallow" stance. -
Verify the foreignObject path on older WebKit + KaTeX. Rendering a cloned surface through
<foreignObject>to canvas is known to go blank or drop external resources on older Safari/WebKit (we've been bitten on macOS ≤12 before), and KaTeX's woff2 fonts referenced from the inlined CSS won't be fetched inside the SVG sandbox, so math may fall back to box glyphs. Worth a spot-check on a mac build and, if it's an issue, embedding the KaTeX font as a data URL.
Thanks!
Summary
Testing
Notes
go test ./...underdesktop/was not used as a gate because existingTestSetEffortPersistsAndAutoClearsfails on the current branch with provider effort persistence output unrelated to this change.