Skip to content

fix(desktop): prevent injectTitle from expanding JS replacement patterns in titles - #6891

Open
xxiaoxiong wants to merge 1 commit into
nexu-io:mainfrom
xxiaoxiong:fix-injecttitle-replace-pattern
Open

fix(desktop): prevent injectTitle from expanding JS replacement patterns in titles#6891
xxiaoxiong wants to merge 1 commit into
nexu-io:mainfrom
xxiaoxiong:fix-injecttitle-replace-pattern

Conversation

@xxiaoxiong

@xxiaoxiong xxiaoxiong commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix PDF/artifact export bug where titles containing characters with special meaning to JavaScript's String.prototype.replace() (e.g. $$, $&amp;, $, $') got corrupted when the source HTML already contained a <title>` element to be replaced.

Root cause

apps/desktop/src/main/pdf-export.ts and apps/desktop/src/main/artifact-export.ts both call doc.replace(regex, tag) to overwrite an existing <title>...</title> block. The replacement string tag was passed directly as a string, which means JS interprets $$, $&amp;, $, $', $<digits> as backreferences. A title like Save $$ This Quarterwas silently stripped toSave $$ This Quarter, and Rock $'n Roll Tour` produced broken HTML because the $' replacement inserted the portion of the string after the match.

Repro

const doc = '<html><head><title>Old</title></head><body>BODY</body></html>';
doc.replace(/<title[^>]*>.*?<\/title>/is, `<title>Save $$ This Quarter</title>`);
// => '<html><head><title>Save 3035465 This Quarter</title>...</html>'  (one $ eaten)
// => '<html><head><title>Save 3035465$ This Quarter</title>...</html>' (none eaten on this single match, but chained matches corrupt)

After many titles, this corrupts PDF metadata, breaks browser tab titles, and on artifact-export.ts corrupts $ titles.

Fix

Wrap the replacement in an arrow function so the replacement string is treated literally instead of as a pattern:

- if (/<title[^>]*>.*?<\/title>/is.test(doc)) return doc.replace(/<title[^>]*>.*?<\/title>/is, tag);
+ if (/<title[^>]*>.*?<\/title>/is.test(doc)) return doc.replace(/<title[^>]*>.*?<\/title>/is, () => tag);

The other two replace calls in these helpers already use replacer functions, so the bug only affects the title branch.

Tests

Verified locally with a Node script (String.replace) that titles containing $$, $&, $``, $'round-trip throughinjectTitle` after the fix. The desktop project has no existing unit-test harness for these helpers; this PR is a minimal targeted fix following the repo's typical scope (no tests added to avoid bloating the change).

Fixes #6795

@xxiaoxiong
xxiaoxiong requested a review from a team as a code owner August 14, 2026 07:29
@lefarcen

lefarcen commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Hey @xxiaoxiong — thanks for jumping on this export-title bug so quickly. The scope here is nice and tight.

Two quick things before reviewer pickup:

  • PR fix(desktop): stop expanding $ replacement patterns in export titles #6796 is already open against the same issue and the same desktop export paths, so please coordinate there to avoid two parallel fixes landing on top of each other.
  • Could you add the missing Surface area and Validation details to the PR body? The current Summary already covers the problem well; we just need the checklist/scope plus what you actually ran to verify the fix.

@lefarcen
lefarcen requested a review from nettee August 14, 2026 07:32
@lefarcen lefarcen added size/XS PR changes <20 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix labels Aug 14, 2026
This was referenced Aug 14, 2026
Open
Open
@lefarcen lefarcen added the needs-validation Runtime change detected; needs human or /explore agent validation. label Aug 14, 2026
@lefarcen

lefarcen commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

🧪 Queued for QA validation — this PR changes exported artifact/PDF behavior, so it needs a manual QA pass before merge. Nothing needed from you right now; we’ll update here once validation is done. Thanks for the contribution!

nettee
nettee approved these changes Aug 14, 2026
View reviewed changes

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@xxiaoxiong

I reviewed both desktop export title-replacement branches and verified that the callback form now preserves literal replacement-pattern characters while leaving the existing insertion paths unchanged. A focused Node check covered $$, $&, $, $', and numeric/named $` sequences, and the PR's available workspace checks are green. Nice, focused fix for a subtle PDF/artifact export corruption issue—thank you!

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen

lefarcen commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Review and CI are both green now, so this is queued for manual QA validation before merge.

Also, when you have a moment, please fill in the missing PR-body pieces: Surface area, Bug fix verification, and Validation.

@lefarcen
lefarcen requested a review from ivy-ting August 14, 2026 07:48
@xxiaoxiong
xxiaoxiong force-pushed the fix-injecttitle-replace-pattern branch from f8c0d7c to 936fd1f Compare August 14, 2026 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nettee nettee nettee approved these changes
@ivy-ting ivy-ting Awaiting requested review from ivy-ting

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

needs-validation Runtime change detected; needs human or /explore agent validation. risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/XS PR changes <20 lines type/bugfix Bug fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Desktop PDF/image export corrupts titles containing $$, $&, $` or $' (injectTitle expands replace() patterns)

3 participants

@xxiaoxiong @lefarcen @nettee