Redesign share notification email with dual CTAs and org branding settings - #2497
Redesign share notification email with dual CTAs and org branding settings#2497timmilazzo wants to merge 5 commits into
Conversation
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.
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 2 potential issues 🟡
Review Details
Code Review Summary
PR #2497 redesigns Clips share-notification emails with sender-specific headings, a secondary CTA, treated link blocks, and closing copy through a new getShareEmailExtras registration hook. It also consolidates organization identity and branding into the Organization settings tab, removes the General profile card, adds branding previews, and updates navigation to the new hash-based tab.
The overall approach is sound: the email renderer preserves existing escaped inline-markup conventions, the new hook is optional and failure-isolated, and the settings component distinguishes loading/error/empty states rather than silently treating a failed request as missing branding. Core email-template coverage was expanded for both HTML and plain text.
Key Findings
- 🟡 MEDIUM — The removed
/settings/organizationroute has no backward-compatible redirect, so existing bookmarks or external links can produce a 404 after the settings relocation. - 🟡 MEDIUM — The Clips “copyable agent link” in the email is built from the normal recording URL. For private/org recordings, that URL requires browser authentication and is not the documented short-lived
agent_accessURL intended for external agents.
Risk assessment: Standard, because this combines shared email behavior and user-facing settings/navigation changes.
🧪 Browser testing: Attempted FULL verification, but all 16 planned cases could not be verified because the browser executor lacked Chrome automation tools; the dev server itself was healthy.
| }[]; | ||
| } | ||
|
|
||
| export default function OrganizationSettingsRoute() { |
There was a problem hiding this comment.
🟡 Preserve a redirect for the removed organization settings route
Removing this route leaves the previously exposed /settings/organization URL without a matching route. Existing bookmarks, shared links, and cached navigation can now land on a 404 instead of the relocated Organization tab; keep a lightweight redirect to /settings#organization (or an equivalent route-level migration).
Additional Info
Found by 2 of 4 code-review agents; browser test TC-08 could not verify live behavior because Chrome automation was unavailable.
There was a problem hiding this comment.
this is okay, I doubt anyone used this branding as I found some bugs yesterday. We can add it later
There was a problem hiding this comment.
Skipping per @shomix — the branding route was new and effectively unused, so we can add a redirect later if anyone reports a dead bookmark.
| linkBlock: { | ||
| intro: "Copy and paste this link for your own AI agent to summarize:", | ||
| url: ctx.href, |
There was a problem hiding this comment.
🟡 Generate a scoped agent URL for the copyable link
The new “Copy and paste this link for your own AI agent” block uses ctx.href, which is the normal recording URL. For private or organization-only recordings, that URL requires the recipient’s authenticated browser session, so an external agent cannot fetch the clip context; use the existing short-lived agent_access link mechanism for this block instead.
Additional Info
Found by 1 of 4 code-review agents; the Clips instructions document create-recording-agent-link as the two-hour agent_access URL, and browser TC-15 could not be exercised because Chrome automation was unavailable.
There was a problem hiding this comment.
Leaving the plain clip URL here. The scoped agent_access link is a two-hour token, and share emails are routinely opened hours or days later — a copyable link that is already dead by the time the recipient pastes it is worse than one that asks them to sign in. The recipient also just received a share grant, so the normal URL works for them; the block is aimed at pasting into their own agent, which for public clips works today. Happy to revisit if we add a longer-lived, revocable agent link.
shomix
left a comment
shomix
left a comment
There was a problem hiding this comment.
Nice, left some comments, but all in all looks good, will let builder address them.
| const { session } = useSession(); | ||
| const t = useT(); | ||
| const agentSettingsTabs = useAgentSettingsTabs(); | ||
| // Organization identity (name, logo, brand color) belongs with membership, |
| brandLogoUrl, | ||
| preheader: subject, | ||
| heading: "You've been given access", | ||
| paragraphs: [ |
There was a problem hiding this comment.
removing this paragraphs will remove it from all the other templates. Lets keep them and make them configurable. So that if a caller passes an option, they can override them
There was a problem hiding this comment.
Done — the default two paragraphs are back for every template, and a registration can now override them. ShareEmailExtras gained an optional paragraphs field: omit it to keep the default copy, or pass [] to send none. Clips passes [] so its heading sits directly above the thumbnail; nothing else changes.
|
@builderio-bot take a look at the comments on this PR and address them if they are valid and would improve the changes |
|
Addressed the PR feedback:
Core rebuilt, ⠀ |
|
Here's a visual recap of what changed: Open the full interactive recap |
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest commit adds ShareEmailExtras.paragraphs, allowing a resource-specific hook to intentionally replace the default share-email body with either custom paragraphs or an empty array. The ?? fallback correctly preserves the default body for resources that omit the field while allowing Clips to suppress redundant copy. Existing open findings about the removed /settings/organization route and the non-scoped agent link remain unresolved and are intentionally not reposted.
The new hook remains optional and resolver failures are isolated, and the existing email-template behavior and Clips closing-copy escaping are otherwise consistent with the current trusted-markup conventions. However, the new generic paragraph surface broadens the raw-HTML boundary without making that contract explicit or enforcing it.
New Finding
- 🟡 MEDIUM —
ShareEmailExtras.paragraphsis passed directly to the email renderer, which inserts paragraphs verbatim. A future resource hook that treats this new field as ordinary text and includes a user-controlled title or description could inject HTML into share emails. The contract should either escape ordinary paragraph text centrally and provide a typed trusted-markup helper, or explicitly document/enforce that this field accepts only treated markup.
Risk assessment: Standard.
🧪 Browser testing: Full verification was attempted, but all 15 planned cases were blocked because Chrome executor tools were unavailable; the dev server was healthy and route/action checks were performed non-visually.
3 participants
Summary
Reworks the share notification email to lead with who shared the resource, adds a second (dark) CTA and closing paragraphs via a new
getShareEmailExtrasregistration hook, and consolidates organization branding settings (name, logo, brand color, default visibility) into Settings → Organization.Problem
The primary share-recipient email opened with a generic "You've been given access" heading, included a redundant lead paragraph, and only supported a single CTA with no way to surface a copyable link or closing notes. Additionally, organization branding fields (like the logo used in these emails) were scattered/missing across Settings menus, with no clear place for a user to upload an org logo alongside the organization name.
Solution
Extended the core email template to support a secondary CTA button, closing paragraphs, and reused the existing treated-link block, then introduced a
getShareEmailExtrasresolver hook on the shareable resource registry so templates (like Clips) can supply resource-specific extras (secondary CTA, link block, closing text) without changing core sharing logic. The share heading was updated to"<sharer> shared "<title>" with you"and the reply-to is already routed to the resource sender. Organization branding UI was consolidated into a single Settings → Organization surface.Key Changes
packages/core/src/server/email-template.ts: AddedsecondaryCtaandclosingParagraphssupport torenderEmail, including HTML/text rendering for a dark-filled secondary button and closing paragraph block.packages/core/src/sharing/actions/share-resource.ts: Replaced the generic access-granted heading/paragraphs with"<sender> shared "<title>" with you", removed the redundant lead paragraph and footer text, and added support for callingreg.getShareEmailExtrasto fetch aShareEmailExtraspayload (secondary CTA, link block, closing paragraphs) with error handling if the resolver fails.templates/clips/server/lib/share-email-hero.ts: AddedrecordingShareEmailExtrasproducing the Clips-specific "Summarize with AI" secondary CTA, a copyable agent link block, and closing paragraphs that route replies to the sender.templates/clips/server/db/index.ts: WiredgetShareEmailExtrasinto the recording'sregisterShareableResourceconfig.templates/clips/shared/clips-ai-prefs.ts: Removed the unuseddisplayNamefield fromClipsUserPrefs.email-template.spec.tsfor the secondary CTA and closing paragraphs rendering in both HTML and plain text.To clone this PR locally use the Github CLI with command
gh pr checkout 2497You can tag me at @BuilderIO for anything you want me to fix or change