feat: add contact-book variable registry for campaign personalization#359
Merged
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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: |
9a5e44c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1f897fd6.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
KMKoushik
force-pushed
the
feat/contact-book-variable-registry
branch
from
cebc799 to
8238f42
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
873-880:⚠️ Potential issue | 🟠 MajorRemove raw recipient emails from these info-level logs.
These branches now log
contact.emailand full CC/BCC recipient lists during normal suppression handling. That puts recipient PII into routine logs. PrefercontactIdplus counts here, and leave any unavoidable address redaction to the shared logger serializer.Based on learnings, enforce PIIdata redaction across the codebase by centralizing with a Pino serializer rather than ad-hoc fixes.🔒 Safer logging shape
logger.info( { - contactEmail: contact.email, + contactId: contact.id, campaignId: emailConfig.campaignId, teamId: emailConfig.teamId, }, "Contact email is suppressed. Creating suppressed email record.", ); @@ logger.info( { - originalCc: ccEmails, - filteredCc: filteredCcEmails, + suppressedCcCount: ccEmails.length - filteredCcEmails.length, + remainingCcCount: filteredCcEmails.length, campaignId: emailConfig.campaignId, teamId: emailConfig.teamId, }, "Some CC recipients were suppressed and filtered out from campaign email.", ); @@ logger.info( { - originalBcc: bccEmails, - filteredBcc: filteredBccEmails, + suppressedBccCount: bccEmails.length - filteredBccEmails.length, + remainingBccCount: filteredBccEmails.length, campaignId: emailConfig.campaignId, teamId: emailConfig.teamId, }, "Some BCC recipients were suppressed and filtered out from campaign email.", );Also applies to: 928-948
🤖 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 873 - 880, Replace any direct recipient PII in the info-level log call (currently logging contact.email and full CC/BCC lists) with non-PII identifiers and counts: log contact.id (or contactId) instead of contact.email and include counts for ccRecipients/bccRecipients (e.g., ccCount, bccCount) alongside emailConfig.campaignId and emailConfig.teamId in the logger.info call; update the same pattern in the related branch mentioned (around the other block at 928-948) so no raw email addresses are emitted and rely on the centralized Pino serializer for any redaction instead of ad-hoc email strings.
🧹 Nitpick comments (1)
apps/web/src/app/(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsx (1)
148-148: Minor:propertiesis always empty in the non-header path.The
propertiesobject initialized at line 148 is never populated whenheadersis undefined, so line 228 will always returnundefinedfor the non-header path. The code is correct but the initialization at line 148 is unnecessary in this branch.This is a minor readability nit and doesn't affect functionality.
♻️ Optional: move properties initialization inside header block
let subscribed: boolean | undefined = undefined; let firstName: string | undefined = undefined; let lastName: string | undefined = undefined; - const properties: Record<string, string> = {}; if (headers) { + const properties: Record<string, string> = {}; for (let i = 0; i < parts.length; i++) { // ... existing header parsing logic } return { email, firstName, lastName, subscribed, properties: Object.keys(properties).length > 0 ? properties : undefined, }; } // Non-header path (no properties) if (parts.length >= 4) { // ... } return { email, firstName, lastName, - properties: Object.keys(properties).length > 0 ? properties : undefined, subscribed, };Also applies to: 224-230
🤖 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 at line 148, The variable properties is initialized empty at the top but only used/populated when headers is present, so move the declaration/initialization of properties into the headers-handling branch (the block guarded by headers) where it is populated; remove the unnecessary top-level const properties: Record<string,string> = {}; so that the non-header path doesn't carry an unused empty object and the populated properties is local to the headers branch (affecting the logic around the headers handling and the code that reads properties later).
🤖 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/server/service/contact-service.ts`:
- Around line 83-86: The upsert path is overwriting stored JSON properties
because normalizeContactProperties(contact.properties, ...) is called even when
contact.properties is undefined; change addOrUpdateContact so it first fetches
existingContact (include properties: true in the select) and only compute/assign
normalizedProperties when contact.properties is provided, merging
normalizedProperties into existingContact.properties (rather than replacing)
before performing the update; ensure normalizeContactProperties is used for the
incoming fragment and the final payload merges with the existing JSON blob so
partial updates don't wipe stored properties.
---
Outside diff comments:
In `@apps/web/src/server/service/campaign-service.ts`:
- Around line 873-880: Replace any direct recipient PII in the info-level log
call (currently logging contact.email and full CC/BCC lists) with non-PII
identifiers and counts: log contact.id (or contactId) instead of contact.email
and include counts for ccRecipients/bccRecipients (e.g., ccCount, bccCount)
alongside emailConfig.campaignId and emailConfig.teamId in the logger.info call;
update the same pattern in the related branch mentioned (around the other block
at 928-948) so no raw email addresses are emitted and rely on the centralized
Pino serializer for any redaction instead of ad-hoc email strings.
---
Nitpick comments:
In
`@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsx:
- Line 148: The variable properties is initialized empty at the top but only
used/populated when headers is present, so move the declaration/initialization
of properties into the headers-handling branch (the block guarded by headers)
where it is populated; remove the unnecessary top-level const properties:
Record<string,string> = {}; so that the non-header path doesn't carry an unused
empty object and the populated properties is local to the headers branch
(affecting the logic around the headers handling and the code that reads
properties later).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c38c190-bbf6-4527-b688-9c20571c1065
📒 Files selected for processing (24)
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/contact-properties.tsapps/web/src/lib/contact-properties.unit.test.tsapps/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.api.test.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-service.tsapps/web/src/server/service/contact-service.unit.test.tsapps/web/src/server/service/contact-variable-service.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/server/public-api/api/contacts/get-contact-book.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/src/server/service/contact-variable-service.ts
- apps/web/src/server/public-api/api/contacts/update-contact-book.ts
- apps/web/src/lib/zod/contact-book-schema.ts
- apps/web/src/server/service/contact-book-service.unit.test.ts
- apps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsx
- apps/web/src/app/(dashboard)/contacts/edit-contact-book.tsx
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 (2)
apps/web/src/app/(dashboard)/contacts/[contactBookId]/add-contact.tsx (1)
80-93:⚠️ Potential issue | 🟡 MinorReset form state after successful submission.
When the dialog reopens (especially in controlled mode where the component isn't unmounted), the previous input will still be visible. The similar
BulkUploadContactscomponent explicitly clears its state on close (context snippet 2:setInputText("")).Proposed fix
{ onSuccess: async () => { utils.contacts.contacts.invalidate(); + contactsForm.reset(); if (controlledOpen === undefined) { setOpen(false); } else { onOpenChange?.(false); } toast.success("Contacts queued for processing"); },🤖 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]/add-contact.tsx around lines 80 - 93, The form state isn't being reset after a successful submit, so when the dialog is reopened (especially in controlled mode) previous input persists; update the onSuccess handler (the callback passed to the mutation) to clear the component's form state—e.g., call setInputText("") and reset any other local form state like file/fileInput or selected contact type (e.g., setFile(null), setSelectedContactType(undefined)) before closing (the block that currently calls setOpen(false) / onOpenChange(false) and toast.success).packages/email-editor/src/editor.tsx (1)
84-105:⚠️ Potential issue | 🟠 MajorAdd dependency tracking to
useEditor()to refresh extensions when variables or suggestion helper text change.Passing
variablesandvariableSuggestionsHelperTextintoextensions()doesn't automatically update the editor when those props change. Without a dependency array,useEditor()keeps the same editor instance with the original extension configuration locked in at creation time. When contact-book selection changes or suggestion data updates, the variable suggestion extension remains stale.Either add a dependencies array to
useEditor(deps)that includesvariablesandvariableSuggestionsHelperTextto recreate the editor when they change, or useuseRef()to keep the editor stable while having extension callbacks read live data from refs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/email-editor/src/editor.tsx` around lines 84 - 105, The editor instance created by useEditor currently never rebuilds when variables or variableSuggestionsHelperText change, so update the call to useEditor to include a dependency array that includes variables and variableSuggestionsHelperText (so the editor and its extensions(...) are recreated when those props change) or alternatively switch the extensions' callbacks to read from refs (useRef) that are kept in sync with variables and variableSuggestionsHelperText while keeping the same editor instance; update the code around useEditor, extensions, variables, and variableSuggestionsHelperText accordingly to implement one of these two approaches.
♻️ Duplicate comments (1)
apps/web/src/app/(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsx (1)
131-160:⚠️ Potential issue | 🟠 MajorResolve the email cell before rejecting header-based rows.
There is still a related header-mode edge case here: in header mode the email column can be anywhere, but this block still short-circuits on
parts[0]. Rows like,alice@example.comorEmail,alice@example.comunderfirstName,emailget dropped even though the email column is valid.Parse the email column first, then validate the row
- if (parts.length === 0 || !parts[0]) return null; - - const firstPart = parts[0]?.toLowerCase(); - if ( - firstPart === "email" || - firstPart === "e-mail" || - firstPart === "email address" - ) { - return null; - } - const getHeader = (index: number) => headers?.[index]?.trim().toLowerCase(); + const emailColumnIndex = headers?.findIndex((header) => { + const normalized = header.trim().toLowerCase(); + return ( + normalized === "email" || + normalized === "e-mail" || + normalized === "email address" + ); + }); const email = ( - headers - ? parts[ - headers.findIndex((header) => { - const normalized = header.trim().toLowerCase(); - return ( - normalized === "email" || - normalized === "e-mail" || - normalized === "email address" - ); - }) - ] - : parts[0] + headers && emailColumnIndex !== undefined && emailColumnIndex >= 0 + ? parts[emailColumnIndex] + : parts[0] )?.toLowerCase(); // Skip if doesn't look like an email if (!email || !email.includes("@")) 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 131 - 160, The code currently rejects a row early by inspecting parts[0] (firstPart), which drops valid header-mode rows where the email column is not at index 0; change the logic to resolve the email cell first (using the existing getHeader / headers.findIndex logic and the email variable) and only reject the row if, after resolving the email (either from headers or parts[0] when no headers), the email is missing or malformed; in practice: remove or guard the firstPart-based early return so it only applies when headers is falsy, and ensure the computed email (the email const) is used for the "doesn't look like an email" check.
🧹 Nitpick comments (2)
apps/web/src/app/(dashboard)/contacts/[contactBookId]/add-contact.tsx (1)
99-108: Simplify theonOpenChangehandler.The
if (nextOpen !== open)guard is unnecessary—React'ssetStateis already a no-op when the value is unchanged. A cleaner if-else structure would improve readability.Proposed refactor
- onOpenChange={(nextOpen) => { - if (controlledOpen === undefined) { - if (nextOpen !== open) { - setOpen(nextOpen); - } - return; - } - - onOpenChange?.(nextOpen); - }} + onOpenChange={(nextOpen) => { + if (controlledOpen === undefined) { + setOpen(nextOpen); + } else { + onOpenChange?.(nextOpen); + } + }}🤖 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]/add-contact.tsx around lines 99 - 108, The onOpenChange handler contains an unnecessary nextOpen !== open guard; simplify it by removing that check and directly calling setOpen(nextOpen) when uncontrolled (controlledOpen === undefined), otherwise call the external onOpenChange?.(nextOpen). Update the handler that references controlledOpen, open, setOpen and onOpenChange in add-contact.tsx to use this clearer if/else structure.apps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsx (1)
44-45: Use the~/source alias for these imports.These new relative imports bypass the repo-wide alias convention and make later moves and refactors noisier.
As per coding guidelines, `apps/web/src/**/*.{ts,tsx}`: Use the `~/` alias for imports from src in apps/web (e.g., `import { x } from "~/utils/x"`).Alias import diff
-import EditContactBook from "../edit-contact-book"; -import DeleteContactBook from "../delete-contact-book"; +import EditContactBook from "~/app/(dashboard)/contacts/edit-contact-book"; +import DeleteContactBook from "~/app/(dashboard)/contacts/delete-contact-book";🤖 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]/page.tsx around lines 44 - 45, Replace the relative imports for EditContactBook and DeleteContactBook in page.tsx with the repo alias import style (use the ~/ source alias) so they reference their modules via the apps/web src alias instead of relative paths; update the two import statements (EditContactBook, DeleteContactBook) to use the `~/` prefix consistent with apps/web src import conventions.
🤖 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/delete-contact-book.tsx:
- Around line 14-17: Add a new callback prop (e.g., onDeleted: (id: string) =>
void or onSuccess: () => void) to the component props alongside
trigger/open/onOpenChange and contactBook, update the props type and
destructuring in the component signature, and invoke that callback right after
the delete mutation completes successfully (inside the mutation's onSuccess/then
handler) so callers like the contact-book detail page can redirect or react;
ensure you call it with the deleted contactBook.id (or no args if onSuccess) and
still keep the existing cache refresh behavior.
In `@apps/web/src/app/`(dashboard)/contacts/edit-contact-book.tsx:
- Around line 30-33: Update contactBookSchema to parse and validate the
comma-separated variables string: in the zod schema for contactBookSchema,
replace variables: z.string().optional() with a transformer that splits on
commas, trims each name, filters out empty entries, and validates each token
against the registry rules (no spaces, no punctuation, allowed regex like
/^[A-Za-z_][A-Za-z0-9_]*$/) and rejects reserved keys (provide the same
reservedKeys array used elsewhere). Return the parsed array in the schema so the
form receives structured data, and update the form field rendering to show a
FormMessage for variables when schema validation fails so users get inline
feedback (ensure the mutation handlers that consume variables accept the new
array shape or map it back if needed).
- Around line 80-87: After a successful edit you only invalidate
utils.contacts.getContactBooks; also invalidate the detail query for the edited
contact book so the detail page updates. In the onSuccess handler (inside the
mutation in edit-contact-book.tsx) call the detail invalidate on the contacts
router (e.g., utils.contacts.getContactBook.invalidate(...) or
utils.contacts.getContactBook.invalidate({ id: contactBookId })) using the
contact book id available in scope (contactBookId or contactBook?.id), then keep
the existing UI close/toast logic.
- Around line 59-65: The form's defaultValues are only applied once by useForm,
so call contactBookForm.reset(...) to sync the form whenever the source
contactBook or the dialog open state changes; inside a useEffect that depends on
contactBook (and the dialog open prop/state), compute the same defaults you used
for useForm (name: contactBook.name || "" and variables: (contactBook.variables
?? []).join(", ")) and pass them to contactBookForm.reset to clear stale values
when the dialog reopens or the contactBook prop updates.
---
Outside diff comments:
In `@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/add-contact.tsx:
- Around line 80-93: The form state isn't being reset after a successful submit,
so when the dialog is reopened (especially in controlled mode) previous input
persists; update the onSuccess handler (the callback passed to the mutation) to
clear the component's form state—e.g., call setInputText("") and reset any other
local form state like file/fileInput or selected contact type (e.g.,
setFile(null), setSelectedContactType(undefined)) before closing (the block that
currently calls setOpen(false) / onOpenChange(false) and toast.success).
In `@packages/email-editor/src/editor.tsx`:
- Around line 84-105: The editor instance created by useEditor currently never
rebuilds when variables or variableSuggestionsHelperText change, so update the
call to useEditor to include a dependency array that includes variables and
variableSuggestionsHelperText (so the editor and its extensions(...) are
recreated when those props change) or alternatively switch the extensions'
callbacks to read from refs (useRef) that are kept in sync with variables and
variableSuggestionsHelperText while keeping the same editor instance; update the
code around useEditor, extensions, variables, and variableSuggestionsHelperText
accordingly to implement one of these two approaches.
---
Duplicate comments:
In
`@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsx:
- Around line 131-160: The code currently rejects a row early by inspecting
parts[0] (firstPart), which drops valid header-mode rows where the email column
is not at index 0; change the logic to resolve the email cell first (using the
existing getHeader / headers.findIndex logic and the email variable) and only
reject the row if, after resolving the email (either from headers or parts[0]
when no headers), the email is missing or malformed; in practice: remove or
guard the firstPart-based early return so it only applies when headers is falsy,
and ensure the computed email (the email const) is used for the "doesn't look
like an email" check.
---
Nitpick comments:
In `@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/add-contact.tsx:
- Around line 99-108: The onOpenChange handler contains an unnecessary nextOpen
!== open guard; simplify it by removing that check and directly calling
setOpen(nextOpen) when uncontrolled (controlledOpen === undefined), otherwise
call the external onOpenChange?.(nextOpen). Update the handler that references
controlledOpen, open, setOpen and onOpenChange in add-contact.tsx to use this
clearer if/else structure.
In `@apps/web/src/app/`(dashboard)/contacts/[contactBookId]/page.tsx:
- Around line 44-45: Replace the relative imports for EditContactBook and
DeleteContactBook in page.tsx with the repo alias import style (use the ~/
source alias) so they reference their modules via the apps/web src alias instead
of relative paths; update the two import statements (EditContactBook,
DeleteContactBook) to use the `~/` prefix consistent with apps/web src import
conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77c0c20c-bdc5-4d92-9499-e5d3614691ed
📒 Files selected for processing (9)
apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/add-contact.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/bulk-upload-contacts.tsxapps/web/src/app/(dashboard)/contacts/[contactBookId]/page.tsxapps/web/src/app/(dashboard)/contacts/delete-contact-book.tsxapps/web/src/app/(dashboard)/contacts/edit-contact-book.tsxpackages/email-editor/src/editor.tsxpackages/email-editor/src/extensions/index.tspackages/email-editor/src/nodes/variable.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/(dashboard)/campaigns/[campaignId]/edit/page.tsx
Comment on lines
30
to
33
| const contactBookSchema = z.object({ | ||
| name: z.string().min(1, { message: "Name is required" }), | ||
| variables: z.string().optional(), | ||
| }); |
Contributor
There was a problem hiding this comment.
Add inline validation for the variable registry.
variables is still just a free-form string here, so names with spaces, punctuation, or reserved keys can be submitted and only fail later as a generic mutation error. Parse the comma-separated list in contactBookSchema and surface a FormMessage for this field so the dialog matches the registry rules and inline feedback promised by this feature.
Also applies to: 136-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/`(dashboard)/contacts/edit-contact-book.tsx around lines 30
- 33, Update contactBookSchema to parse and validate the comma-separated
variables string: in the zod schema for contactBookSchema, replace variables:
z.string().optional() with a transformer that splits on commas, trims each name,
filters out empty entries, and validates each token against the registry rules
(no spaces, no punctuation, allowed regex like /^[A-Za-z_][A-Za-z0-9_]*$/) and
rejects reserved keys (provide the same reservedKeys array used elsewhere).
Return the parsed array in the schema so the form receives structured data, and
update the form field rendering to show a FormMessage for variables when schema
validation fails so users get inline feedback (ensure the mutation handlers that
consume variables accept the new array shape or map it back if needed).
Comment on lines
59
to
65
| const contactBookForm = useForm<z.infer<typeof contactBookSchema>>({ | ||
| resolver: zodResolver(contactBookSchema), | ||
| defaultValues: { | ||
| name: contactBook.name || "", | ||
| variables: (contactBook.variables ?? []).join(", "), | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/app/\(dashboard\)/contacts/edit-contact-book.tsxRepository: usesend/useSend
Length of output: 6373
Reset the form when the source contact book changes or the dialog reopens.
The dialog stays mounted and useForm only reads defaultValues at initialization. Without resetting the form when the dialog opens, reopening after a save or dismissing unsaved edits displays stale name and variable values from the previous session.
Sync the form from the latest props on open
-import { useState } from "react";
+import { useEffect, useState } from "react";
@@
const contactBookForm = useForm<z.infer<typeof contactBookSchema>>({
resolver: zodResolver(contactBookSchema),
defaultValues: {
name: contactBook.name || "",
variables: (contactBook.variables ?? []).join(", "),
},
});
+ const isOpen = controlledOpen ?? open;
+ const serializedVariables = (contactBook.variables ?? []).join(", ");
+
+ useEffect(() => {
+ if (!isOpen) return;
+
+ contactBookForm.reset({
+ name: contactBook.name || "",
+ variables: serializedVariables,
+ });
+ }, [
+ contactBookForm,
+ contactBook.id,
+ contactBook.name,
+ serializedVariables,
+ isOpen,
+ ]);
@@
- open={controlledOpen ?? open}
+ open={isOpen}Also applies to: 98-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/`(dashboard)/contacts/edit-contact-book.tsx around lines 59
- 65, The form's defaultValues are only applied once by useForm, so call
contactBookForm.reset(...) to sync the form whenever the source contactBook or
the dialog open state changes; inside a useEffect that depends on contactBook
(and the dialog open prop/state), compute the same defaults you used for useForm
(name: contactBook.name || "" and variables: (contactBook.variables ??
[]).join(", ")) and pass them to contactBookForm.reset to clear stale values
when the dialog reopens or the contactBook prop updates.
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 only replace built-in or whitelisted variables (case-insensitive with fallbacks), and contact properties are normalized across upload, edit, list, and export.
New Features
Migration
Written for commit 9a5e44c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests