feat: add contact-book variable registry for campaign personalization#359
Open
feat: add contact-book variable registry for campaign personalization#359
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a new Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/server/service/campaign-service.ts (1)
188-247:⚠️ Potential issue | 🟡 MinorEmailRenderer variable matching is case-sensitive while HTML path is case-insensitive—this creates inconsistency.
In the JSON content path,
EmailRendereruses direct property access (this.variableValues[variable]) with no case normalization, making it strictly case-sensitive. However, in the HTML path,replaceContactVariablesnormalizes all keys to lowercase and compares allowedVariables case-insensitively.This means if a custom variable is registered as "FirstName" (mixed case):
- The HTML path correctly matches it (normalizes both to "firstname")
- The JSON path may fail to substitute it, depending on how the variable is referenced in the JSON content
Consider normalizing variable keys to a consistent casing (e.g., lowercase) when building
variableValuesforEmailRenderer, or document that custom variables must match the exact casing used in the JSON content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/service/campaign-service.ts` around lines 188 - 247, renderCampaignHtmlForContact builds variableValues for EmailRenderer in a case-sensitive way while replaceContactVariables lowercases keys, causing inconsistent substitutions; update the variableValues construction inside renderCampaignHtmlForContact so all custom variable keys are normalized (e.g., toLowerCase()) when inserted (use the same normalization used by replaceContactVariables/getContactReplacementValue) and ensure the renderer receives both normalized keys and core keys (email, firstName, lastName) in the matching normalized form so JSON-rendered content substitutes consistently with the HTML path.
🧹 Nitpick comments (6)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
170-175:useMemodependency should becontactBook, notcontactBook?.variables.
contactBookis a render-scope derived value (from.find()), not a ref-stable value. ListingcontactBook?.variablesinstead ofcontactBookin the dependency array will likely triggerreact-hooks/exhaustive-depslint warnings, and is less semantically clear. SincecontactBookchanging is the condition that should trigger a recompute, it belongs directly in the deps:♻️ Proposed fix
const editorVariables = useMemo(() => { const baseVariables = ["email", "firstName", "lastName"]; const registryVariables = contactBook?.variables ?? []; return Array.from(new Set([...baseVariables, ...registryVariables])); - }, [contactBook?.variables]); + }, [contactBook]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/campaigns/[campaignId]/edit/page.tsx around lines 170 - 175, The useMemo for editorVariables currently depends on contactBook?.variables but should depend on the whole contactBook because contactBook is a derived render-scope value; update the dependency array of useMemo to [contactBook] so editorVariables recomputes whenever contactBook changes (refer to the editorVariables constant and the useMemo call that reads contactBook?.variables).apps/web/src/server/service/campaign-service.ts (1)
111-144: DRY the built-in variable list to prevent drift.
replaceContactVariableshard-codes the built-ins even thoughBUILT_IN_CONTACT_VARIABLESexists. Using the constant avoids future inconsistencies.♻️ Suggested refactor
- const isBuiltIn = ["email", "firstname", "lastname"].includes( - normalizedKey, - ); + const isBuiltIn = BUILT_IN_CONTACT_VARIABLES.some( + (variable) => variable.toLowerCase() === normalizedKey, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/service/campaign-service.ts` around lines 111 - 144, The function replaceContactVariables currently hard-codes the built-in list ["email","firstname","lastname"]; replace that literal with the existing constant BUILT_IN_CONTACT_VARIABLES to avoid drift (use the same case-insensitive check you already do: normalize the compared value with normalizedKey and compare against BUILT_IN_CONTACT_VARIABLES in a case-insensitive way), keeping the rest of the logic intact (CONTACT_VARIABLE_REGEX, allowedVariables check, and getContactReplacementValue usage).apps/web/src/server/public-api/api/contacts/create-contact-book.ts (1)
44-63: Skip the extra update when only variables are provided.
createContactBookServicealready persistsvariables. The current truthy check triggers an immediateupdateContactBookeven when onlyvariablesare present (or an empty array), causing a redundant write.♻️ Suggested refactor
- // Update emoji/properties/variables if provided - if (body.emoji || body.properties || body.variables) { + // Update emoji/properties if provided (variables already set on create) + const needsUpdate = + body.emoji !== undefined || body.properties !== undefined; + if (needsUpdate) { const updated = await updateContactBook(contactBook.id, { emoji: body.emoji, properties: body.properties, variables: body.variables, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/public-api/api/contacts/create-contact-book.ts` around lines 44 - 63, The code performs an unnecessary update when only body.variables is present because createContactBookService already saved variables; change the conditional from if (body.emoji || body.properties || body.variables) to only run when emoji or properties are explicitly provided (e.g. if (body.emoji !== undefined || body.properties !== undefined)), and when calling updateContactBook(contactBook.id, ...) do not include body.variables in the payload so you avoid redundant writes; keep using the existing symbols createContactBookService, updateContactBook, contactBook.id, body.emoji, body.properties, and body.variables to locate and modify the logic.apps/web/src/app/(dashboard)/contacts/[contactBookId]/contact-list.tsx (1)
148-154: Redundantas stringcast.The
?? ""null-coalescing already narrows the type tostring; the trailingas stringassertion is unnecessary.♻️ Suggested cleanup
- ...(contactBookVariables ?? []).map((variable) => - escapeCell( - ((contact.properties as Record<string, string> | undefined)?.[ - variable - ] ?? "") as string, - ), - ), + ...(contactBookVariables ?? []).map((variable) => + escapeCell( + (contact.properties as Record<string, string> | undefined)?.[ + variable + ] ?? "", + ), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/contact-list.tsx around lines 148 - 154, The code maps contactBookVariables to cells using escapeCell but includes a redundant type assertion "as string" after the null-coalescing expression; remove the trailing "as string" in the mapping so the expression ((contact.properties as Record<string, string> | undefined)?.[variable] ?? "") is passed directly to escapeCell, leaving contactBookVariables, escapeCell, and contact.properties references unchanged.apps/web/src/server/public-api/api/contacts/get-contact-book.ts (1)
78-82: Redundant explicitvariablesfield in response.
...contactBookalready spreadsvariablesfrom Prisma; the extravariables: contactBook.variablesoverrides it with the same value.♻️ Suggested cleanup
return c.json({ ...contactBook, properties: contactBook.properties as Record<string, string>, - variables: contactBook.variables, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/public-api/api/contacts/get-contact-book.ts` around lines 78 - 82, The return object in the c.json call redundantly reassigns variables from contactBook (the spread already includes it); inside the get-contact-book response, remove the explicit "variables: contactBook.variables" property and simply return c.json({ ...contactBook, properties: contactBook.properties as Record<string, string> }) so the spread alone provides variables while keeping the properties cast.apps/web/src/server/service/contact-book-service.ts (1)
114-122:...dataspread passes the raw (un-normalized)variablesbefore the normalized override.When
data.variablesis defined,...dataspreads it first, then{ variables: normalizedVariables }overwrites it. This is functionally correct (normalized value always wins), but it means Prisma momentarily receives the un-normalized value in the spread object, which the subsequent key override corrects. The pattern is safe but slightly fragile. Extractingvariablesfromdatabefore the spread makes the intent explicit and prevents confusion.♻️ Suggested alternative
+ const { variables: _rawVariables, ...restData } = data; return db.contactBook.update({ where: { id: contactBookId }, data: { - ...data, - ...(normalizedVariables !== undefined - ? { variables: normalizedVariables } - : {}), + ...restData, + ...(normalizedVariables !== undefined ? { variables: normalizedVariables } : {}), }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/service/contact-book-service.ts` around lines 114 - 122, The update call currently spreads ...data which may include the raw variables before the normalized override; refactor the code that calls db.contactBook.update to destructure variables out of data first (e.g., const { variables, ...rest } = data), then pass rest plus the normalized variables (if defined) into db.contactBook.update so the raw variables are never part of the spread; update the db.contactBook.update call to use the new rest object and conditional variables: normalizedVariables to make the intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsx:
- Around line 224-243: The header-detection in parseContacts can misidentify a
data row as a header if a cell contains the word "Email"; update parseContacts
so hasHeader only returns true when the first line both contains a header label
(email/e-mail/email address) AND the first-line cells do not look like actual
data (e.g., none match an email regex and not all cells are numeric/typical data
values). Concretely, in parseContacts before setting headers, validate
firstLineParts by running a simple email regex (and optionally a numeric check)
against each part and only treat the row as a header if a header label exists
AND there are zero email-looking parts (or a low fraction of data-like parts);
keep using parseContactLine(headers) afterward.
In `@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/edit-contact.tsx:
- Around line 177-193: The FormLabel and Input for dynamic variables are not
linked, so generate a stable unique id per variable (e.g., using the variable
name and the contactBookId or another stable identifier) inside the
contactBookVariables.map, set that id on the Input and set FormLabel's htmlFor
to the same id (sanitize variable to remove spaces/unsafe chars); update the map
rendering that uses FormItem/FormLabel/FormControl/Input and keep using
variableValues/setVariableValues for state so each label is properly associated
with its input for screen readers.
- Around line 45-65: The state variableValues is only initialized once from
initialVariableValues and won't reflect later changes to contactBookVariables;
add a useEffect that runs when initialVariableValues changes and merges in any
new keys without clobbering user edits by calling setVariableValues(prev => ({
...initialVariableValues, ...prev })) so new variables are added (from
initialVariableValues) while existing user-typed values in variableValues are
preserved; reference the initialVariableValues, variableValues and
setVariableValues identifiers and place the effect near their declarations.
In `@apps/web/src/server/service/contact-variable-service.ts`:
- Around line 1-33: The validateContactBookVariables function currently only
checks RESERVED_CONTACT_VARIABLES; add a format check that enforces the same
placeholder regex used for campaign parsing (e.g., /^[A-Za-z0-9_]+$/) so names
like "first-name" or "company name" are rejected; define a shared constant
(e.g., CONTACT_VARIABLE_REGEX) near RESERVED_CONTACT_VARIABLES and use it in
validateContactBookVariables to throw a clear error when a variable doesn't
match the allowed pattern, ensuring normalizeContactBookVariables still
trims/dedups but that validateContactBookVariables validates against both
reserved names and the regex.
---
Outside diff comments:
In `@apps/web/src/server/service/campaign-service.ts`:
- Around line 188-247: renderCampaignHtmlForContact builds variableValues for
EmailRenderer in a case-sensitive way while replaceContactVariables lowercases
keys, causing inconsistent substitutions; update the variableValues construction
inside renderCampaignHtmlForContact so all custom variable keys are normalized
(e.g., toLowerCase()) when inserted (use the same normalization used by
replaceContactVariables/getContactReplacementValue) and ensure the renderer
receives both normalized keys and core keys (email, firstName, lastName) in the
matching normalized form so JSON-rendered content substitutes consistently with
the HTML path.
---
Nitpick comments:
In `@apps/web/src/app/`(dashboard)/campaigns/[campaignId]/edit/page.tsx:
- Around line 170-175: The useMemo for editorVariables currently depends on
contactBook?.variables but should depend on the whole contactBook because
contactBook is a derived render-scope value; update the dependency array of
useMemo to [contactBook] so editorVariables recomputes whenever contactBook
changes (refer to the editorVariables constant and the useMemo call that reads
contactBook?.variables).
In `@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/contact-list.tsx:
- Around line 148-154: The code maps contactBookVariables to cells using
escapeCell but includes a redundant type assertion "as string" after the
null-coalescing expression; remove the trailing "as string" in the mapping so
the expression ((contact.properties as Record<string, string> |
undefined)?.[variable] ?? "") is passed directly to escapeCell, leaving
contactBookVariables, escapeCell, and contact.properties references unchanged.
In `@apps/web/src/server/public-api/api/contacts/create-contact-book.ts`:
- Around line 44-63: The code performs an unnecessary update when only
body.variables is present because createContactBookService already saved
variables; change the conditional from if (body.emoji || body.properties ||
body.variables) to only run when emoji or properties are explicitly provided
(e.g. if (body.emoji !== undefined || body.properties !== undefined)), and when
calling updateContactBook(contactBook.id, ...) do not include body.variables in
the payload so you avoid redundant writes; keep using the existing symbols
createContactBookService, updateContactBook, contactBook.id, body.emoji,
body.properties, and body.variables to locate and modify the logic.
In `@apps/web/src/server/public-api/api/contacts/get-contact-book.ts`:
- Around line 78-82: The return object in the c.json call redundantly reassigns
variables from contactBook (the spread already includes it); inside the
get-contact-book response, remove the explicit "variables:
contactBook.variables" property and simply return c.json({ ...contactBook,
properties: contactBook.properties as Record<string, string> }) so the spread
alone provides variables while keeping the properties cast.
In `@apps/web/src/server/service/campaign-service.ts`:
- Around line 111-144: The function replaceContactVariables currently hard-codes
the built-in list ["email","firstname","lastname"]; replace that literal with
the existing constant BUILT_IN_CONTACT_VARIABLES to avoid drift (use the same
case-insensitive check you already do: normalize the compared value with
normalizedKey and compare against BUILT_IN_CONTACT_VARIABLES in a
case-insensitive way), keeping the rest of the logic intact
(CONTACT_VARIABLE_REGEX, allowedVariables check, and getContactReplacementValue
usage).
In `@apps/web/src/server/service/contact-book-service.ts`:
- Around line 114-122: The update call currently spreads ...data which may
include the raw variables before the normalized override; refactor the code that
calls db.contactBook.update to destructure variables out of data first (e.g.,
const { variables, ...rest } = data), then pass rest plus the normalized
variables (if defined) into db.contactBook.update so the raw variables are never
part of the spread; update the db.contactBook.update call to use the new rest
object and conditional variables: normalizedVariables to make the intent
explicit.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/web/prisma/migrations/20260224120000_add_contact_book_variables/migration.sqlapps/web/prisma/schema.prismaapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/contact-list.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/edit-contact.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsxapps/web/src/app/(dashboard)/contacts/add-contact-book.tsxapps/web/src/app/(dashboard)/contacts/edit-contact-book.tsxapps/web/src/lib/zod/contact-book-schema.tsapps/web/src/server/api/routers/contacts.tsapps/web/src/server/public-api/api/contacts/create-contact-book.tsapps/web/src/server/public-api/api/contacts/get-contact-book.tsapps/web/src/server/public-api/api/contacts/get-contact-books.tsapps/web/src/server/public-api/api/contacts/update-contact-book.tsapps/web/src/server/service/campaign-service.tsapps/web/src/server/service/contact-book-service.tsapps/web/src/server/service/contact-variable-service.ts
apps/web/src/app/(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsx
Show resolved
Hide resolved
apps/web/src/app/(dashboard)/contacts/[contactBookId]/edit-contact.tsx
Outdated
Show resolved
Hide resolved
KMKoushik
force-pushed
the
feat/contact-book-variable-registry
branch
from
d6f0664 to
1a5b5e3
Compare
Deploying usesend with
|
| Latest commit: |
cebc799
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c1fb5fb7.usesend.pages.dev |
| Branch Preview URL: | https://feat-contact-book-variable-r.usesend.pages.dev |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
KMKoushik
force-pushed
the
feat/contact-book-variable-registry
branch
from
a0b35d1 to
d6f0664
Compare
KMKoushik
force-pushed
the
feat/contact-book-variable-registry
branch
from
d6f0664 to
1bdd005
Compare
KMKoushik
force-pushed
the
useSend-double-opt-in
branch
from
70f0832 to
3cef365
Compare
KMKoushik
force-pushed
the
feat/contact-book-variable-registry
branch
from
cbb88cd to
2463fd9
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx (1)
167-175: Consider stabilizinguseMemodependencies for effective memoization.The
contactBookobject is derived via.find()on each render, producing a new reference even when the underlying data is unchanged. This causes theuseMemoto recompute on every render, negating its benefit.♻️ Suggested improvement
Depend on stable primitives instead of the object reference:
const contactBook = contactBooksQuery.data?.find( (book) => book.id === contactBookId, ); +const contactBookVariables = contactBook?.variables; const editorVariables = useMemo(() => { const baseVariables = ["email", "firstName", "lastName"]; - const registryVariables = contactBook?.variables ?? []; + const registryVariables = contactBookVariables ?? []; return Array.from(new Set([...baseVariables, ...registryVariables])); -}, [contactBook]); +}, [contactBookVariables]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/campaigns/[campaignId]/edit/page.tsx around lines 167 - 175, The memo is recomputing because it depends on the transient contactBook object; move the .find() lookup into the useMemo so the memo depends on stable primitives instead of an object reference — i.e., compute editorVariables inside a useMemo that depends on contactBookId and contactBooksQuery.data (or contactBooksQuery.data ?? []), then derive registryVariables by finding the matching book within that memo (referencing contactBooksQuery, contactBookId, contactBook, and editorVariables to locate the correct spot).apps/web/src/server/service/contact-book-service.unit.test.ts (1)
48-56: Consider adding tests for variable validation and normalization.The test suite verifies the happy path for variables, but coverage for validation failures (invalid characters, reserved names) and normalization behavior (trimming, deduplication) would strengthen confidence in the feature. These could be added to this file or a dedicated
contact-variable-service.unit.test.ts.Would you like me to generate test cases for variable validation and normalization, or open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/service/contact-book-service.unit.test.ts` around lines 48 - 56, Add unit tests that cover variable validation failures and normalization for the contact-variable flow within the existing contact-book-service suite: add tests that call the variable creation/registration path and assert that inputs with invalid characters or reserved names cause rejection and do not call mockDb.contactBook.create/update/findUnique, and add tests that pass inputs with extra whitespace and duplicates to verify trimming/deduplication behavior (assert final values passed to mockDb.contactBook.create/findMany are normalized). Use the existing mocks (mockCheckContactBookLimit, mockDb.contactBook.create/findMany/update/findUnique, mockValidateDomainFromEmail) to simulate required responses and to verify no DB writes occur on validation errors and correct normalized payloads on success; place tests either in this file or a new contact-variable-service.unit.test.ts.apps/web/src/server/service/campaign-service.ts (1)
97-126: Consider simplifying the Proxy-based case-insensitive lookup.While the Proxy implementation works, it adds complexity. A simpler approach would be to normalize all keys to lowercase in the initial object creation and do a single lowercase lookup.
♻️ Optional simplification
function createCaseInsensitiveVariableValues( values: Record<string, string | null | undefined>, -) { - const normalizedValues = Object.entries(values).reduce( - (acc, [key, value]) => { - if (value !== undefined) { - acc[key] = value; - acc[key.toLowerCase()] = value; - } - - return acc; - }, - {} as Record<string, string | null>, - ); - - return new Proxy(normalizedValues, { - get(target, prop, receiver) { - if (typeof prop === "string") { - const exact = Reflect.get(target, prop, receiver); - if (exact !== undefined) { - return exact; - } - - return Reflect.get(target, prop.toLowerCase(), receiver); - } - - return Reflect.get(target, prop, receiver); - }, - }) as Record<string, string | null>; +): Record<string, string | null> { + const result: Record<string, string | null> = {}; + for (const [key, value] of Object.entries(values)) { + if (value !== undefined) { + result[key] = value; + result[key.toLowerCase()] = value; + } + } + return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/service/campaign-service.ts` around lines 97 - 126, The Proxy is unnecessary—simplify createCaseInsensitiveVariableValues by normalizing keys to lowercase when building normalizedValues (e.g., for each [key,value] do if (value !== undefined) acc[key.toLowerCase()] = value) and return that plain object (typed as Record<string,string|null>) instead of wrapping it in a Proxy; remove the Proxy handler and any code that relied on its behavior.apps/web/src/app/(dashboard)/contacts/[contactBookId]/contact-list.tsx (1)
151-158: Type assertion assumes all property values are strings.The cast
contact.properties as Record<string, string>may throw if a property value is not a string. While the Python SDK type showsDict[str, str], Prisma's JSON type allows arbitrary values.Consider adding defensive handling:
🛡️ Safer property access
...(contactBookVariables ?? []).map((variable) => escapeCell( - (contact.properties as Record<string, string> | undefined)?.[ - variable - ] ?? "", + String( + (contact.properties as Record<string, unknown> | undefined)?.[ + variable + ] ?? "" + ), ), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/contact-list.tsx around lines 151 - 158, The code currently force-casts contact.properties to Record<string, string> which can break if a property value isn't a string; update the mapping that calls escapeCell so it reads the value without asserting its type, then coerce it to a safe string (e.g., if typeof value === 'string' use it, if null/undefined use "", otherwise JSON.stringify(value) or String(value)) before passing to escapeCell; target the expression around contact.properties, the variable map callback, and the escapeCell call to implement this defensive conversion.apps/web/src/app/(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsx (2)
112-119: Header row detection could skip valid data rows.If a data row's first email starts with "email" (e.g., "email123@example.com"), it would be skipped. Consider making the check stricter by requiring an exact match after case normalization.
🛡️ Stricter header detection
const firstPart = parts[0]?.toLowerCase(); if ( - firstPart === "email" || - firstPart === "e-mail" || - firstPart === "email address" + (firstPart === "email" || + firstPart === "e-mail" || + firstPart === "email address") && + !validateEmail(parts[0] ?? "") ) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsx around lines 112 - 119, The header-detection logic using firstPart = parts[0]?.toLowerCase() can incorrectly skip rows whose first cell begins with the word "email" (e.g., "email123@example.com"); change the check in bulk-upload-contacts.tsx so it only treats a row as a header when the entire first cell equals (after trim+lowercase) "email", "e-mail", or "email address" rather than when it merely starts with those strings — update the condition that references firstPart and parts to use full-string equality (trimmed and lowercased) for the header match.
215-221: Properties object is always empty in the non-header path.In the fallback positional parsing (lines 196-221),
propertiesis initialized but never populated, so the checkObject.keys(properties).length > 0always evaluates tofalseand returnsundefined. This is harmless but slightly inconsistent with the header-based path.♻️ Cleaner approach: remove unused properties check
return { email, firstName, lastName, - properties: Object.keys(properties).length > 0 ? properties : undefined, subscribed, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsx around lines 215 - 221, The fallback positional parsing returns an object with keys email, firstName, lastName, properties, subscribed but never populates properties so the conditional Object.keys(properties).length > 0 ? properties : undefined always yields undefined; remove that conditional and return properties directly (i.e., keep the properties key as-is) so the non-header path is consistent with the header-based path and no special emptiness check is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/app/`(dashboard)/campaigns/[campaignId]/edit/page.tsx:
- Around line 167-175: The memo is recomputing because it depends on the
transient contactBook object; move the .find() lookup into the useMemo so the
memo depends on stable primitives instead of an object reference — i.e., compute
editorVariables inside a useMemo that depends on contactBookId and
contactBooksQuery.data (or contactBooksQuery.data ?? []), then derive
registryVariables by finding the matching book within that memo (referencing
contactBooksQuery, contactBookId, contactBook, and editorVariables to locate the
correct spot).
In
`@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsx:
- Around line 112-119: The header-detection logic using firstPart =
parts[0]?.toLowerCase() can incorrectly skip rows whose first cell begins with
the word "email" (e.g., "email123@example.com"); change the check in
bulk-upload-contacts.tsx so it only treats a row as a header when the entire
first cell equals (after trim+lowercase) "email", "e-mail", or "email address"
rather than when it merely starts with those strings — update the condition that
references firstPart and parts to use full-string equality (trimmed and
lowercased) for the header match.
- Around line 215-221: The fallback positional parsing returns an object with
keys email, firstName, lastName, properties, subscribed but never populates
properties so the conditional Object.keys(properties).length > 0 ? properties :
undefined always yields undefined; remove that conditional and return properties
directly (i.e., keep the properties key as-is) so the non-header path is
consistent with the header-based path and no special emptiness check is used.
In `@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/contact-list.tsx:
- Around line 151-158: The code currently force-casts contact.properties to
Record<string, string> which can break if a property value isn't a string;
update the mapping that calls escapeCell so it reads the value without asserting
its type, then coerce it to a safe string (e.g., if typeof value === 'string'
use it, if null/undefined use "", otherwise JSON.stringify(value) or
String(value)) before passing to escapeCell; target the expression around
contact.properties, the variable map callback, and the escapeCell call to
implement this defensive conversion.
In `@apps/web/src/server/service/campaign-service.ts`:
- Around line 97-126: The Proxy is unnecessary—simplify
createCaseInsensitiveVariableValues by normalizing keys to lowercase when
building normalizedValues (e.g., for each [key,value] do if (value !==
undefined) acc[key.toLowerCase()] = value) and return that plain object (typed
as Record<string,string|null>) instead of wrapping it in a Proxy; remove the
Proxy handler and any code that relied on its behavior.
In `@apps/web/src/server/service/contact-book-service.unit.test.ts`:
- Around line 48-56: Add unit tests that cover variable validation failures and
normalization for the contact-variable flow within the existing
contact-book-service suite: add tests that call the variable
creation/registration path and assert that inputs with invalid characters or
reserved names cause rejection and do not call
mockDb.contactBook.create/update/findUnique, and add tests that pass inputs with
extra whitespace and duplicates to verify trimming/deduplication behavior
(assert final values passed to mockDb.contactBook.create/findMany are
normalized). Use the existing mocks (mockCheckContactBookLimit,
mockDb.contactBook.create/findMany/update/findUnique,
mockValidateDomainFromEmail) to simulate required responses and to verify no DB
writes occur on validation errors and correct normalized payloads on success;
place tests either in this file or a new contact-variable-service.unit.test.ts.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/web/prisma/migrations/20260224120000_add_contact_book_variables/migration.sqlapps/web/prisma/schema.prismaapps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/contact-list.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/edit-contact.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsxapps/web/src/app/(dashboard)/contacts/add-contact-book.tsxapps/web/src/app/(dashboard)/contacts/edit-contact-book.tsxapps/web/src/lib/zod/contact-book-schema.tsapps/web/src/server/api/routers/contacts.tsapps/web/src/server/public-api/api/contacts/create-contact-book.tsapps/web/src/server/public-api/api/contacts/get-contact-book.tsapps/web/src/server/public-api/api/contacts/get-contact-books.tsapps/web/src/server/public-api/api/contacts/update-contact-book.tsapps/web/src/server/service/campaign-service.tsapps/web/src/server/service/contact-book-service.tsapps/web/src/server/service/contact-book-service.unit.test.tsapps/web/src/server/service/contact-variable-service.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsx
- apps/web/prisma/schema.prisma
- apps/web/src/server/public-api/api/contacts/get-contact-book.ts
- apps/web/src/lib/zod/contact-book-schema.ts
- apps/web/prisma/migrations/20260224120000_add_contact_book_variables/migration.sql
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.
Summary
ContactBook.variablesregistry (plus migration) and validate/normalize variable names with reserved key protection foremail,firstName, andlastNameVerification
pnpmis not installed (pnpm: command not found)Summary by cubic
Adds a contact-book variable registry for controlled campaign personalization. Campaigns now replace only built‑in and whitelisted variables (case-insensitive with fallbacks), and custom contact properties are handled across upload, edit, list, and export with normalized casing and preservation of non‑registry fields.
New Features
Migration
Written for commit cebc799. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests