Add org logo, treated links, and first-view email to transactional emails - #2399
Conversation
…bc3e0b95875944d98162
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
builder-io-integration
Bot
changed the title
This comment has been minimized.
This comment has been minimized.
![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 4 potential issues 🔴
Review Details
Code Review Summary
PR #2399 expands the shared transactional-email surface with organization branding, safe link blocks, sender-name overrides, and a new first-view notification triggered from Clips view tracking. It also adds the /import Clips flow and related navigation/regression coverage. The email escaping and logical-key deduplication are thoughtful, and the focused tests/builds reported by the agents passed.
Risk: Standard, because this combines user-facing workflow changes with durable business-side effects and background email processing.
Key Findings
- 🔴 HIGH — The transactional queue defaults to process-local filesystem storage, which is not durable or shared across the supported Netlify/serverless deployment model. Enqueued emails can disappear between invocations, and separate instances cannot coordinate leases.
- 🟡 MEDIUM — A two-minute sending lease can expire while an unbounded provider request is still running, allowing another worker to reclaim and send the same email, causing duplicates.
- 🟡 MEDIUM — A crash after creating a
.takeovermarker can leave a permanent lock that prevents future operations for that logical key. - 🟡 MEDIUM — The new public
@agent-native/coreserver export/API has no changeset, so the release workflow may omit the package version/publication update.
🧪 Browser testing: Will run after this review (PR touches UI code); the planner attempted a full run, but all cases were blocked because browser automation tools were unavailable in the executor environment.
| const root = | ||
| options.root ?? | ||
| path.join(process.cwd(), "data", "clips-transactional-emails"); |
There was a problem hiding this comment.
🔴 Persist the hosted email queue in durable shared storage
The queue, claims, leases, and reconciliation cursors default to process.cwd()/data, which is process-local and non-durable in the supported Netlify/serverless deployment. An event handler can enqueue on one instance while a worker or later request reads another empty filesystem, so first-view/import/reminder emails can be lost; use the shared database or an explicitly configured durable queue for hosted mode.
Additional Info
Reported independently by 2 review agents.
There was a problem hiding this comment.
Not changed in this PR. I agree this is a real limitation for multi-instance/serverless hosting, but the feature was explicitly designed and approved around the repository-backed file queue. Moving jobs, claims, leases, and all reconciliation cursors to shared SQL is a persistence migration, not a safe localized review fix; a partial hosted-mode shim would make the guarantees less clear. This should be tracked and implemented as a dedicated queue-storage migration before enabling these jobs on horizontally scaled hosted deployments.
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable) |
|
Addressed all PR feedback and replied directly to every thread:
GitHub still evaluates the old remote commit as conflicting with Netlify checks pending. Push the resolved changes using the top-right Push button; CI and remote mergeability can only update afterward. |
|
Here's a visual recap of what changed: Open the full interactive recap |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@builderio-bot please /babysit-pr and auto fix what you agree with |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 35177285 | Triggered | Generic High Entropy Secret | fce183c | packages/docs/app/routes/_index.tsx | View secret |
| 35177286 | Triggered | Generic High Entropy Secret | fce183c | packages/docs/app/routes/_index.tsx | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Resolved both merge conflicts and addressed all PR feedback, including replying to the latest false-positive review. Verification passes: 879/879 Clips tests, typecheck, production build, formatting, and both previously failing security guards. GitHub still reports |
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest head fixes the previously reported undefined parseSendGridFrom runtime defect; the helper now exists and the corresponding review thread was resolved. The hosted queue durability and provider-send lease-expiry concerns remain open from earlier reviews and were not reposted. The latest changes add three new issues across the Clips UI and transactional/import flows.
Risk: Standard, because this combines user-facing navigation with background email dispatch and media-format handling.
Key Findings
- 🟡 MEDIUM — The new “Upload video” links point to plain
/recordrather than/record?autoUpload=1, so they open the normal recording screen instead of the advertised upload picker. - 🟡 MEDIUM — Two-Clip AI summary jobs are marked
ai_dispatchedbefore browser handoff and stale claims are only logged, so a closed tab or failed handoff can permanently strand the job. - 🟡 MEDIUM — Direct WebM/MOV/M4V imports are persisted as MP4, causing downstream playback, transcription, and export code to use the wrong decoder/extension.
🧪 Browser testing: Will run after this review (PR touches UI code); a full run was attempted, but all cases were blocked because browser automation tools were unavailable in the executor environment.
| oembed.thumbnail_url ?? existingRecording?.thumbnailUrl ?? null, | ||
| oembed?.thumbnail_url ?? existingRecording?.thumbnailUrl ?? null, | ||
| durationMs, | ||
| videoFormat: "mp4" as const, |
There was a problem hiding this comment.
🟡 Preserve the actual format of direct video imports
The direct-link path accepts WebM, MOV, and M4V and preserves the fetched MIME type, but this recording is always saved with videoFormat: "mp4" and later uploaded with an .mp4 name. Downstream playback, transcript fallback, and ffmpeg export select behavior from videoFormat, so non-MP4 bytes can be decoded with the wrong format. Derive a supported format/filename from the MIME type or normalize the media before storing it as MP4.
Additional Info
Found by 1 of 2 review agents; verified against direct-video MIME detection and downstream format-dependent code.
There was a problem hiding this comment.
Fixed locally: direct WebM imports now persist videoFormat: "webm" and upload with a .webm filename; MP4 remains MP4. Because the recording schema/player contract only supports MP4/WebM, the importer now rejects MOV/M4V instead of mislabeling those containers as MP4. Added a WebM regression test.
|
@builderio-bot CI still failing and more feedback - run the /babysit-pr skill and keep watching and fixing pm a loop until the PR is full green (no new feedback or CI dailures) for at least 30 minutes |
|
Fixed all new feedback and CI issues; local verification is green with 882 tests, production build, formatting, typecheck, all 32 guards, and zero unaddressed threads. The staged fixes and merge commits must be published via the top-right Push control before GitHub CI and the requested 30-minute green soak can begin. |
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest head fixes two previously reported issues: the library-grid Upload video link now adds autoUpload=1, and stale ai_dispatched jobs can be reclaimed by a later claimant. The provider-send lease concern is also addressed by the 60-second send timeout relative to the two-minute lease, so that thread was resolved. The hosted queue durability concern remains open and was not reposted.
The latest review found two new issues. The sidebar Upload video controls were not updated along with the grid control, and the narrowed direct-video URL guessing still accepts unsupported video/* response MIME types that the importer relabels as MP4.
Risk: Standard, because these changes affect user-facing recording workflows and persisted media semantics.
Key Findings
- 🟡 MEDIUM — Both expanded and collapsed sidebar Upload video controls still navigate to plain
/record, so they do not trigger the upload picker. - 🟡 MEDIUM —
video/quicktime,video/x-m4v, and other unsupportedvideo/*responses are accepted and then persisted as MP4 despite the Clips format contract supporting only WebM and MP4.
🧪 Browser testing: Will run after this review (PR touches UI code); a full run was attempted, but all cases were blocked because browser automation tools were unavailable in the executor environment.
| variant="ghost" | ||
| asChild | ||
| > | ||
| <NavLink to="/record"> |
There was a problem hiding this comment.
🟡 Make sidebar Upload video controls request auto-upload
Both sidebar Upload video controls still navigate to plain /record, while the recorder only opens the file picker when autoUpload=1 is present. They therefore behave like “New recording” instead of performing the advertised upload action. Use an upload href with autoUpload=1 for both expanded and collapsed sidebar entries, preserving any relevant context.
Additional Info
Found by 1 of 2 review agents and corroborated by repeated browser-planner source inspection. This is distinct from the resolved library-grid link issue.
There was a problem hiding this comment.
Fixed locally: both collapsed and expanded sidebar “Upload video” controls now navigate to /record?autoUpload=1; the separate New recording controls remain on plain /record. Typecheck, formatting, and affected UI tests pass.
| const mimeType = | ||
| (headerValue ?? "").split(";")[0]?.trim().toLowerCase() ?? ""; | ||
| if (mimeType.startsWith("video/")) return mimeType; |
There was a problem hiding this comment.
🟡 Reject unsupported direct-video MIME types
The latest URL-guessing change still accepts every video/* response type. A direct response with video/quicktime or video/x-m4v therefore reaches the importer, which maps every non-WebM MIME type to videoFormat: "mp4" and uploads it as an .mp4 recording. Restrict accepted response types to video/mp4 and video/webm, or add full container support before persisting these formats.
Additional Info
Found by 1 of 2 review agents and verified against import-loom-recording.ts format mapping and the schema/player contract.
There was a problem hiding this comment.
Fixed locally: normalizeDirectVideoMimeType now accepts only exact video/mp4 and video/webm response types (or infers those two from an octet-stream URL). Other video/* types such as QuickTime/M4V are rejected before upload, so they cannot be mislabeled as MP4.
|
@builderio-bot CI still failing and more feedback - run the /babysit-pr skill and keep watching and fixing pm a loop until the PR is full green (no new feedback or CI dailures) for at least 30 minutes |
|
Resolved the |
|
@builderio-bot resolve the merge conflicts plz |
|
Resolved and staged all three merge conflicts, preserving main’s sanitized logo/hero and sender-name changes alongside the PR’s link-block behavior. Verification passed: 16 email tests, core typecheck, formatting, relevant guards, and Git confirms no unmerged files. |
3 participants
Summary
Extends the shared email template and sending infrastructure with organization branding, treated link blocks, and custom sender/reply-to support, and wires up a new "first view" transactional email that fires when a recording's owner receives its first genuine non-owner view.
Problem
The existing transactional email system only supported a fixed Agent Native logo, plain CTA buttons, and a single verified sender address/name. There was also no notification sent to a recording owner when their Clip was first viewed by someone else, and no way to safely surface a copyable URL (e.g. for feeding a link to an AI agent) inside an email.
Solution
Enhanced
renderEmailto accept an optional organization logo and a "link block" that renders a safe, escaped, copyable URL either before or after the CTA in both HTML and plain-text output. ExtendedsendEmailto allow overriding the sender display name while keeping the verified email address, and to map a customReply-To. Added logic in theview-eventAPI route to detect the first time a recording transitions to a "counted" view by a non-owner, guard against duplicates/historical views predating email-enablement, and enqueue afirst-viewtransactional email to the owner.Key Changes
email-template.ts: addedbrandLogoUrl(falls back to the existingcid:embedded logo) andlinkBlock(intro,url,placement: "before-cta" | "after-cta") options, rendered safely in both HTML and text output.email.ts: addedfromNametoSendEmailArgs, allowing the display name on thefromfield to be overridden while preserving the verified sender address;Reply-Tomapping confirmed via new test coverage.core/package.json: exposed a new./user-profile/serversubpath export pointing to the user-profile store, for server-side consumption (e.g. resolving org logos/profile names for emails).templates/clips/server/routes/api/view-event.post.ts: on transition to a counted view, checks whether the current viewer/session is genuinely the first non-owner view (post enablement date) and enqueues afirst-viewtransactional email to the recording owner viatransactionalEmailStore; usesownerEmailMatchesfor case-insensitive owner comparison and skips enqueue for owner-initiated or already-counted views.email-template.spec.ts,email.spec.ts, andview-event.post.spec.tscovering org logo fallback, link block placement/escaping, sender name/reply-to overrides, and various first-view enqueue scenarios (anonymous transition, already-counted, owner view, historical view predating enablement, genuine first view).To clone this PR locally use the Github CLI with command
gh pr checkout 2399You can tag me at @BuilderIO for anything you want me to fix or change