fix desktop PDF/image export injectTitle replacement-pattern corruption - #6931
fix desktop PDF/image export injectTitle replacement-pattern corruption#6931buiducnhat wants to merge 2 commits into
Conversation
|
Hey @buiducnhat — thanks for the tight regression-focused fix here. I've queued this for review, and because it touches the desktop export path we'll run a manual QA pass before merge. |
nettee
left a comment
There was a problem hiding this comment.
I reviewed the two desktop export injectTitle changes and the new regression coverage for replacement-pattern titles. The callback replacements preserve literal $ sequences while retaining HTML escaping; the focused regression test (10/10), full desktop Vitest suite (336 passed, 1 skipped), desktop typecheck, repository guard, and workspace typecheck all pass.
Thanks for the focused fix and the clear regression coverage—this closes a subtle user-data corruption path cleanly. Nice work!
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.…ct-title # Conflicts: # apps/desktop/src/main/artifact-export.ts # apps/desktop/src/main/pdf-export.ts
|
Heads-up: PR #6947 is also open against issue #6795. The current head here is now test-only ( |
|
Heads-up: PR #6947 has now merged and issue #6795 is closed. The current diff here looks limited to alternative regression coverage in |
nettee
left a comment
There was a problem hiding this comment.
The replacement-pattern regression coverage is passing, and I verified the existing real-export regression test, full desktop suite (346 passed, 1 skipped), desktop/workspace typechecks, and repository guard. Thanks for adding focused coverage for this subtle title-corruption path; I left one non-blocking maintainability suggestion inline.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| ): (doc: string, title: string) => string { | ||
| const escape = stripTypeAnnotations(extractFunction(source, escapeName)); | ||
| const injectTitle = stripTypeAnnotations(extractFunction(source, "injectTitle")); | ||
| const factory = new Function(`${escape}\n${injectTitle}\nreturn injectTitle;`); |
There was a problem hiding this comment.
[non-blocking] Keep this regression at the real export seam
apps/desktop/tests/main/export-title-replacement-patterns.test.ts already drives exportPdfFromHtml and exportArtifact with the same replacement-pattern cases. This new Function path reads TypeScript text, strips annotations, and executes a reconstructed helper, so it duplicates coverage while bypassing the actual import/call path; later helper formatting or type-syntax changes can break this test without a product regression. Please move the exact-document assertion and the extra combined HTML/$& case into the existing export integration test, then remove this source-eval harness (or expose a shared pure helper if direct unit coverage is needed).
|
Hey @buiducnhat — thanks for the quick follow-up here. @nettee's note is non-blocking, but I do think it's the right cleanup direction: if you want to keep this extra regression coverage on top of the shipped fix from #6947, rebasing around the real export-seam test and folding that suggestion in would make this much easier to maintain. Otherwise, closing this one as superseded is also totally reasonable. |
Fixes #6795
Why
Exporting an artifact to PDF (or the
od exportPDF/image fallback path) corrupts any title containing JavaScript replacement-pattern sequences ($$,$&,$\``,$'). I reproduced this against the real source functions:injectTitle()passes the user-derived title as the **string** replacement argument ofString.prototype.replace, so ECMA-262GetSubstitutionexpands those sequences — dropping$characters, splicing the matched<title>` back into the title, and duplicating the document tail into the rendered output. This silently corrupts real user data at export time, and the corruption also lands in the PDF's Title metadata.What users will see
Exporting a PDF/image whose title contains
$(for exampleSave $$$ This QuarterorRock $'n Roll Tour) now renders the title verbatim in the document<title>and PDF metadata — HTML-escaped only, exactly as the surroundingescapeHtmlText()/escapeText()calls intend. No more dropped characters, prematurely-closed titles, or duplicated document content.Surface area
Screenshots
Not applicable — no UI surface changed.
Bug fix verification
apps/desktop/tests/main/inject-title-replacement-pattern.test.tsmainand green on this branch? yes — the test extracts the realinjectTitle+ escape helper from bothpdf-export.tsandartifact-export.tsand fails with 10 assertions onmain(all four replacement patterns, both files) before the fix, then passes on this branch.Validation
vitest run -c vitest.config.ts tests/main/inject-title-replacement-pattern.test.ts— 10 tests pass.apps/desktop/src/main/pdf-export.tsandapps/desktop/src/main/artifact-export.ts, switching the<title>branch to a function replacement (matching the siblinginjectBaseHref/injectStylehelpers that already do this).Adjacent issues
assembleExample()inapps/daemon/src/routes/static-resource.tsinterpolates a skill-derived title through the same string-replacement mechanism. Its trigger surface is much smaller and it sits outside this issue's described boundary, so I left it for a follow-up rather than expanding this PR.