feat(emails): welcome steps mirror the app's four checkpoints (chat#1889 item 1, part 4) - #787
Open
sweetmantech wants to merge 6 commits into
Open
feat(emails): welcome steps mirror the app's four checkpoints (chat#1889 item 1, part 4)#787sweetmantech wants to merge 6 commits into
sweetmantech wants to merge 6 commits into
Conversation
Welcome, valuation, and weekly-report emails should share one visual language (DESIGN.md — four-font system, shadow-as-border, achromatic chrome), but each outbound email was assembled ad hoc (body + bare footer). This establishes the shared template (recoupable/chat#1885 consistency pass). - `renderEmailLayout` (`lib/emails/`) — one header + footer + optional-CTA wrapper: Recoup wordmark header, achromatic shadow-as-border card, the DESIGN.md font stack, centered fixed-max-width container. Email-client-safe (inline styles, literal hex tokens, webfonts degrade to system fonts). - `processAndSendEmail` — adopts the wrapper as the proof point. This is the path the live weekly-report email flows through (agent `send_email` → `processAndSendEmail`), so the already-live weekly report now renders in the shared house style, with the existing footer carried in as the layout footer. Welcome (api#774) and valuation (api#773) emails adopt the same wrapper once merged — this PR establishes the shared template. TDD: RED→GREEN for the layout renderer (body/footer/CTA + house-style markers) and for the processAndSendEmail adoption. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t-stack quoting Completes the consistency pass — previously the wrapper was only adopted in the weekly-report path, so welcome/valuation still shipped their own chrome. - buildWelcomeEmail + renderValuationReportHtml now render through renderEmailLayout (body + cta + footer), dropping their duplicated outer page/card chrome; each keeps only its own content. - Fix: FONT_STACK used double-quoted font names inside double-quoted style="…" attributes, which terminated the attribute early — silently dropping the font (clients fell back to serif) AND every declaration after it (the CTA button lost its background/padding/radius). Single-quote the font names; add a regression test. - Layout footer tagline: em dash -> comma (house copy rule). - Consistency assertions added to both email tests. 199 lib/emails tests pass; lint + format clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
chat#1889 item 1, part 4. The welcome email numbered five steps while chat derives four checkpoints (artists, socials, catalog, task) and RosterSocialsFlow counted two — the same account was told three different things about how much setup was left. The valuation is not a derivable checkpoint: `catalogs` persists no valuation and the number is computed live from catalog measurements, so a fifth predicate would either duplicate `catalog` or push a network call into the pure deriveOnboardingState. Numbering the payoff alongside the chores was also the wrong model. - welcomeOnboardingSteps drops to the app's four checkpoints, in order. - New renderValuationPayoff renders the baseline valuation after the list as the reward the steps unlock, still linking /setup/valuation. - New welcomeOnboardingSteps test asserts titles + order + paths match the app's checkpoints; chat and api share no package, so this test is what keeps the mirror honest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughEmail rendering now uses a shared layout for consistent structure, while welcome emails separate four onboarding steps from the baseline valuation payoff. Valuation reports and outbound email delivery now pass body, CTA, and footer content through the shared renderer. ChangesEmail rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 13 files
Confidence score: 3/5
- In
lib/emails/renderEmailLayout.ts,EmailLayoutCta.labelis interpolated as HTML andcta.urlis not attribute-escaped, so non-static inputs could inject or rewrite CTA content in outbound emails and mislead recipients — treat labels as plain text and HTML/attribute-escape both fields before rendering. - In
lib/emails/renderEmailLayout.ts, references toDESIGN.md/the “four-font system” point to docs that do not exist, which makes the rationale for brand-token/font decisions unverifiable and increases maintenance risk — update or remove those citations and anchor comments to real sources in-repo.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/emails/renderEmailLayout.ts">
<violation number="1" location="lib/emails/renderEmailLayout.ts:19">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The comments cite 'DESIGN.md §3' and 'DESIGN.md four-font system' as the source for brand tokens and the font stack, but a full-repo search confirms no DESIGN.md exists. Remove the fabricated document references from the comments so they describe the actual intent locally without pointing to a non-existent file.</violation>
<violation number="2" location="lib/emails/renderEmailLayout.ts:62">
P2: CTA labels containing markup are emitted as HTML, so any non-static caller can alter the email content. Treat `EmailLayoutCta.label` as text and HTML-escape it before interpolation; attribute-escape `cta.url` as well.</violation>
</file>
Architecture diagram
sequenceDiagram
participant EmailBuilder as Welcome Email Builder
participant Layout as renderEmailLayout()
participant Steps as renderWelcomeSteps()
participant Payoff as renderValuationPayoff()
participant StepsList as WELCOME_ONBOARDING_STEPS
participant LayoutTest as renderEmailLayout.test.ts
participant StepsTest as welcomeOnboardingSteps.test.ts
Note over EmailBuilder,StepsList: Welcome Email Assembly
EmailBuilder->>Layout: renderEmailLayout({bodyHtml, cta, footerHtml})
Layout->>Steps: renderWelcomeSteps(CHAT_APP_URL)
Steps->>StepsList: Iterate 4 checkpoints
StepsList-->>Steps: [artists, socials, catalog, tasks]
Steps-->>Layout: Numbered step list HTML
Layout->>Payoff: renderValuationPayoff(CHAT_APP_URL)
Payoff->>Payoff: Build payoff block with album cover
Payoff-->>Layout: "Then: baseline valuation" section
Layout->>Layout: Wrap in house-style chrome
Layout-->>EmailBuilder: Complete email HTML
Note over Layout,LayoutTest: Consistency Verification
LayoutTest->>LayoutTest: Check for wordmark header
LayoutTest->>LayoutTest: Verify shadow-as-border card
LayoutTest->>LayoutTest: Assert font stack quoting
LayoutTest->>LayoutTest: Validate no stray double quotes in styles
Note over StepsList,StepsTest: App Checkpoint Mirror Test
StepsTest->>StepsTest: Assert 4 steps in exact order
StepsTest->>StepsTest: Verify no "/setup/valuation" in steps
StepsTest->>StepsList: Compare titles against app checkpoints
StepsList-->>StepsTest: [Confirm artists, Verify socials, Claim catalog, Automate tasks]
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| // Brand tokens (DESIGN.md §3) — kept literal because email clients can't read | ||
| // CSS custom properties. Achromatic chrome; color comes from content. | ||
| const INK = "#0a0a0a"; // --foreground |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The comments cite 'DESIGN.md §3' and 'DESIGN.md four-font system' as the source for brand tokens and the font stack, but a full-repo search confirms no DESIGN.md exists. Remove the fabricated document references from the comments so they describe the actual intent locally without pointing to a non-existent file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/renderEmailLayout.ts, line 19:
<comment>The comments cite 'DESIGN.md §3' and 'DESIGN.md four-font system' as the source for brand tokens and the font stack, but a full-repo search confirms no DESIGN.md exists. Remove the fabricated document references from the comments so they describe the actual intent locally without pointing to a non-existent file.</comment>
<file context>
@@ -0,0 +1,88 @@
+
+// Brand tokens (DESIGN.md §3) — kept literal because email clients can't read
+// CSS custom properties. Achromatic chrome; color comes from content.
+const INK = "#0a0a0a"; // --foreground
+const MUTED_INK = "#6b6b6b"; // --muted-foreground
+const PAGE_BG = "#f7f7f7"; // --muted (page canvas)
</file context>
| <div style="padding:24px 0 4px;"> | ||
| <a href="${cta.url}" target="_blank" rel="noopener noreferrer" | ||
| style="display:inline-block;background:${CTA_BG};color:${CTA_FG};font-family:${FONT_STACK};font-weight:600;font-size:14px;text-decoration:none;padding:12px 20px;border-radius:8px;"> | ||
| ${cta.label} |
There was a problem hiding this comment.
P2: CTA labels containing markup are emitted as HTML, so any non-static caller can alter the email content. Treat EmailLayoutCta.label as text and HTML-escape it before interpolation; attribute-escape cta.url as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/renderEmailLayout.ts, line 62:
<comment>CTA labels containing markup are emitted as HTML, so any non-static caller can alter the email content. Treat `EmailLayoutCta.label` as text and HTML-escape it before interpolation; attribute-escape `cta.url` as well.</comment>
<file context>
@@ -0,0 +1,88 @@
+<div style="padding:24px 0 4px;">
+ <a href="${cta.url}" target="_blank" rel="noopener noreferrer"
+ style="display:inline-block;background:${CTA_BG};color:${CTA_FG};font-family:${FONT_STACK};font-weight:600;font-size:14px;text-decoration:none;padding:12px 20px;border-radius:8px;">
+ ${cta.label}
+ </a>
+</div>`.trim()
</file context>
24 tasks
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/emails/renderEmailLayout.ts (1)
51-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this 38-line renderer into private layout helpers.
Extract the header, CTA, and footer renderers so
renderEmailLayoutonly composes them. As per coding guidelines, “Flag functions longer than 20 lines or classes with >200 lines”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/emails/renderEmailLayout.ts` around lines 51 - 88, Refactor renderEmailLayout into private helper functions for rendering the header, CTA block, and footer block, keeping each helper responsible for its existing markup and conditional behavior. Leave renderEmailLayout focused on composing the helper results and the surrounding layout, preserving the current output and styling.Source: Coding guidelines
lib/emails/processAndSendEmail.ts (1)
47-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the email orchestration function below the size limit.
processAndSendEmailis 40 lines and combines room/footer lookup, HTML rendering, delivery, and response mapping. Extract payload preparation and result normalization into private helpers so this coordinator stays focused.As per coding guidelines, “Flag functions longer than 20 lines” and “Keep functions small and focused.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/emails/processAndSendEmail.ts` around lines 47 - 59, Refactor processAndSendEmail so it remains under 20 lines and only coordinates the workflow. Extract room/footer lookup and HTML payload construction into a private helper, and extract sendEmailWithResend response mapping into another private helper; preserve the existing email content, delivery arguments, and returned result behavior.Source: Coding guidelines
lib/emails/valuationReport/renderValuationReportHtml.ts (1)
37-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the valuation body and styling configuration.
renderValuationReportHtmlspans 29 lines and mixes orchestration with repeated inline style values such as#6b6b6b. Move body construction into a private helper and centralize email style tokens in a configuration object.As per coding guidelines, “Flag functions longer than 20 lines,” “Use constants for repeated values,” and “Use configuration objects instead of hardcoded values.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/emails/valuationReport/renderValuationReportHtml.ts` around lines 37 - 52, Refactor renderValuationReportHtml so it delegates valuation body construction to a private helper, keeping the top-level function focused on assembling the layout and CTA. Add a centralized email styling configuration object for repeated values such as `#6b6b6b`, then update the body and footer markup to reuse those tokens instead of hardcoding them.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/emails/renderEmailLayout.ts`:
- Around line 60-62: Update the CTA rendering in renderEmailLayout to
HTML-escape both cta.url and cta.label before interpolation, and validate URLs
as approved absolute HTTP(S) URLs before placing them in the href attribute.
Replace any existing → CTA label values with the literal → character so
escaped text preserves the intended label.
---
Nitpick comments:
In `@lib/emails/processAndSendEmail.ts`:
- Around line 47-59: Refactor processAndSendEmail so it remains under 20 lines
and only coordinates the workflow. Extract room/footer lookup and HTML payload
construction into a private helper, and extract sendEmailWithResend response
mapping into another private helper; preserve the existing email content,
delivery arguments, and returned result behavior.
In `@lib/emails/renderEmailLayout.ts`:
- Around line 51-88: Refactor renderEmailLayout into private helper functions
for rendering the header, CTA block, and footer block, keeping each helper
responsible for its existing markup and conditional behavior. Leave
renderEmailLayout focused on composing the helper results and the surrounding
layout, preserving the current output and styling.
In `@lib/emails/valuationReport/renderValuationReportHtml.ts`:
- Around line 37-52: Refactor renderValuationReportHtml so it delegates
valuation body construction to a private helper, keeping the top-level function
focused on assembling the layout and CTA. Add a centralized email styling
configuration object for repeated values such as `#6b6b6b`, then update the body
and footer markup to reuse those tokens instead of hardcoding them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a3371914-834c-4c5b-b7f9-51a807b32d36
⛔ Files ignored due to path filters (7)
lib/emails/__tests__/buildWelcomeEmail.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/__tests__/processAndSendEmail.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/__tests__/renderEmailLayout.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/valuationReport/__tests__/renderValuationReportHtml.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/welcome/__tests__/renderValuationPayoff.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/welcome/__tests__/renderWelcomeSteps.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/welcome/__tests__/welcomeOnboardingSteps.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (6)
lib/emails/buildWelcomeEmail.tslib/emails/processAndSendEmail.tslib/emails/renderEmailLayout.tslib/emails/valuationReport/renderValuationReportHtml.tslib/emails/welcome/renderValuationPayoff.tslib/emails/welcome/welcomeOnboardingSteps.ts
Comment on lines
+60
to
+62
| <a href="${cta.url}" target="_blank" rel="noopener noreferrer" | ||
| style="display:inline-block;background:${CTA_BG};color:${CTA_FG};font-family:${FONT_STACK};font-weight:600;font-size:14px;text-decoration:none;padding:12px 20px;border-radius:8px;"> | ||
| ${cta.label} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape CTA URLs and labels at the layout boundary.
cta.url and cta.label are interpolated as raw HTML. A quote-bearing or data-driven CTA can inject attributes/markup; escape both fields before rendering and restrict CTA URLs to approved absolute HTTP(S) URLs. Change existing → labels to a literal → when escaping text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/emails/renderEmailLayout.ts` around lines 60 - 62, Update the CTA
rendering in renderEmailLayout to HTML-escape both cta.url and cta.label before
interpolation, and validate URLs as approved absolute HTTP(S) URLs before
placing them in the href attribute. Replace any existing → CTA label values
with the literal → character so escaped text preserves the intended label.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 4 of item 1 in recoupable/chat#1889. Stacked on #784 (shared email layout) — it rewrites
buildWelcomeEmail, so this branches off it rather than conflicting. Review/merge after #784.Why
Three progress models disagreed:
ONBOARDING_STEP_IDS)RosterSocialsFlowSame account, three answers to "how much setup is left."
An earlier revision of the issue proposed fixing this by adding a fifth
valuationcheckpoint to the app. That was wrong on two counts, and the issue now carries a dated correction:valuationcheckpoint is not derivable.catalogsisid, name, created_at, updated_at— no persisted valuation. The number is computed live from catalog measurements at read time, so a fifth predicate would either duplicate thecatalogpredicate or push a network call intoderiveOnboardingState, which is deliberately pure over three cheap sources.What changed
welcomeOnboardingStepsdrops to the app's four checkpoints, in order.renderValuationPayoffrenders the baseline valuation after the numbered list, visually distinct ("Then: your baseline valuation"), still linking/setup/valuation. So the payoff gets more prominence, not less.welcomeOnboardingSteps.test.tsasserts titles, order, and paths match the app's derived checkpoints. chat and api share no package, so the list is necessarily a deliberate mirror — this test is what keeps the mirror honest instead of pretending to a shared source.Verification
TDD, red → green:
```
RED — all three assertions fail against the 5-step list
FAIL lib/emails/welcome/tests/welcomeOnboardingSteps.test.ts
Test Files 1 failed (1)
Tests 3 failed (3)
```
Two pre-existing tests encoded the old 5-step contract and were updated to the new one (
buildWelcomeEmail > walks the app's four derived checkpoints in order, andrenderWelcomeSteps > points every step at its /setup route), plus a new guard that the valuation is not numbered among the steps.```
pnpm exec vitest run lib/emails
Test Files 33 passed (33)
Tests 206 passed (206)
pnpm exec vitest run # full suite, no regressions
Test Files 786 passed (786)
Tests 4254 passed (4254)
```
pnpm exec tsc --noEmitreports 236 errors on this branch and 236 on untouchedapi/main— identical pre-existing baseline inlib/trigger/__tests__; none in the files this PR touches.Live send verification pending (needs a preview send to confirm the payoff block renders in a real client).
Tracked in chat#1889 (matrix row 7).
Summary by cubic
Updates the welcome email to match the app’s four onboarding checkpoints and reframes valuation as a payoff, while adding a shared email layout so all automated emails look consistent.
New Features
/setup/valuation.Refactors
renderEmailLayoutand adopted it in the welcome email, valuation report, and theprocessAndSendEmailweekly-report path; removed duplicated chrome from individual builders.Written for commit 59191a4. Summary will update on new commits.
Summary by CodeRabbit