fix: use JSON.stringify for caption text escaping in generated JS#625
fix: use JSON.stringify for caption text escaping in generated JS#625hobostay wants to merge 1 commit into
Conversation
The generated JavaScript used manual single-quote escaping (replace(/\\/g, '\\\\').replace(/'/g, \"'\\\\''\")) which missed newlines, carriage returns, and other special characters. Captions containing line breaks would produce syntactically broken JS. Use JSON.stringify which handles all JS string special characters correctly, including newlines, unicode, and control characters. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jrusso1020
left a comment
There was a problem hiding this comment.
Verified — generator.ts:306 only handles \\ and ', so a caption segment with a literal newline (or a U+2028/U+2029) emits broken JS. Switching to JSON.stringify is the right call: it returns a complete double-quoted JS string literal that handles all control chars and line separators, and the surrounding quotes are correctly removed at the call sites. LGTM.
vanceingalls
left a comment
There was a problem hiding this comment.
Additive review at 49d4dbba — @jrusso1020 already verified and approved.
Audited
packages/studio/src/captions/generator.ts:306-313(theJSON.stringifyreplacement)- CI: all required green ✓
Strength
James covered the correctness — JSON.stringify produces a complete double-quoted JS string literal that handles control chars, U+2028/U+2029, embedded quotes, and backslashes correctly. The two failure modes the old manual regex missed (literal newlines, line separators) are both fixed. ✓
Important — supersedes the caption portion of #794 and #795
Both #794 (Jefsky) and #795 (Jefsky) ship the same caption fix (JSON.stringify(seg.text)) but bundle it with broken LayersPanel.tsx new-file additions that would clobber existing code on merge. Recommend merging this single-purpose PR and closing #794 + the caption portion of #795 to keep the surface clean. I've flagged the same in those PRs.
Nit
- A positive-pin test for the contract would be small and high-value here —
expect(generateCaptionJs("line1\nline2")).toContain('"line1\\nline2"')pins the JSON.stringify behavior against a future "let's roll our own escaping again" regression. Out of scope for this PR, but worth a follow-up.
Verdict
Verdict: COMMENT
Reasoning: Fix is correct (James verified); single-purpose diff, CI green. Vance check before merging — this is external-contributor code but it's the canonical one of three competing PRs for this bug. Recommend landing this and closing #794 + the caption portion of #795.
— Vai
vanceingalls
left a comment
There was a problem hiding this comment.
hobostay's caption JSON.stringify escape fix. @jrusso1020 already APPROVED on this SHA. CLEAN merge state. (Note: there's a duplicate-shape #794 from Jefsky against a broken bundle — close that one in favor of this.) Vance authorized the stamp.
Verdict: APPROVE
Reasoning: James verified, correct fix shape, CLEAN.
— Vai
Summary
JSON.stringify()in caption JS generationDetails
Affected file:
packages/studio/src/captions/generator.ts(lines 306-313)A caption segment containing a literal newline (e.g. from a multiline caption) would produce broken JS like:
JSON.stringifyhandles all special characters correctly and produces a double-quoted string literal that is always valid JS.Test plan
🤖 Generated with Claude Code