Update share clip format - #2478
Conversation
shomix
commented
Jul 28, 2026
shomix
commented
- Shows who shared the recording and the recording title.
- Keeps the existing recording thumbnail.
- Uses one clear Open Recording button.
- Adds a short description of Clips and explains that recipients can reply directly to the sender.
Visual recap — screenshot failedA recap was published, but the PR-comment screenshot could not be captured or uploaded. Open the interactive recap directly: Open the full interactive recap Diagnostic: dark: screenshot captured but image upload failed |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Code Review Summary
PR #2478 adds an optional notification-content hook to the shared-resource registry and uses it for Clips recording shares. The Clips email now keeps the existing thumbnail hero, presents a single Open Recording CTA, identifies the sender and recording, and adds reply guidance; the core renderer also supports trusted post-CTA HTML with a plain-text equivalent. The approach is appropriately scoped, reuses the existing sender profile lookup, escapes dynamic markup, and adds focused renderer and security regression tests.
Risk assessment: Standard. The main concern is email-delivery robustness: the newly generated subject includes profile/title values, so header-safe normalization should be guaranteed before provider submission. The rest of the change has good fallback behavior for optional resolvers and preserves existing registrations.
Key Findings
Medium: The Clips notification subject can contain CR/LF from the sender display name, which may cause providers to reject the message.
🧪 Browser testing: Attempted after review, but browser automation was unavailable; all planned flows were reported as environment-unverified. The dev server was healthy and server-side verification confirmed the new email content.
| const senderName = ctx.sender.name.trim(); | ||
| const title = recording.title?.trim() || "Untitled recording"; | ||
| const heading = `${senderName} shared "${title}" with you`; | ||
|
|
||
| return { | ||
| subject: heading, |
There was a problem hiding this comment.
🟡 Strip line breaks before using the profile name in the subject
senderName is only trimmed, so an embedded \r or \n in the display name is copied into subject and passed to the email provider. That can make the notification fail to send; strip CR/LF from the sender/title before constructing the subject (and add a regression test).
Additional Info
Found by 1 of 2 parallel code-review agents; verified against the changed subject construction and provider send path.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 participants
