Lead and Sales rewards metadata condition#3907
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughAdds metadata-aware reward conditions end-to-end: Zod schema + UI support for metadata fields on lead/sale, evaluation logic that resolves and normalizes metadata values, commission context inclusion for lead/sale metadata when present, and tests covering schema and evaluation behavior. ChangesMetadata support across rewards flow
Sequence Diagram sequenceDiagram
participant UI as "Admin UI\n(rewards editor)"
participant Client as "Browser\n(rewards logic)"
participant Server as "API\n(evaluate/track)"
participant Eval as "evaluateRewardConditions"
participant PartnerSvc as "Partner commission"
UI->>Client: user adds condition (entity=lead|sale, attribute=metadata, metadataField)
Client->>Client: validate with Zod schemas
Client->>Server: save reward or trigger evaluation
Server->>Eval: evaluateRewardConditions(context includes lead/sale.metadata?)
Eval->>Eval: resolveConditionFieldValue -> normalize metadata by operator
Eval-->>Server: evaluation result
Server->>PartnerSvc: createPartnerCommission(context includes metadata when present)
PartnerSvc-->>Server: commission created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/lib/zod/schemas/rewards.ts (1)
289-308:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
metadataFieldshould be required whenattributeismetadata.Right now schema accepts metadata conditions without a key, which silently become non-matching at evaluation time.
💡 Proposed fix
-export const rewardConditionSchema = z.object({ +export const rewardConditionSchema = z.object({ entity: z.enum( REWARD_CONDITION_ENTITIES.map(({ id }) => id) as [string, ...string[]], ), attribute: z.enum( REWARD_CONDITION_ATTRIBUTES.map(({ id }) => id) as [string, ...string[]], ), operator: z.enum(CONDITION_OPERATORS), value: z.union([ z.string(), z.number(), z.array(z.string()), z.array(z.number()), ]), label: z .string() .nullish() .describe("Product name used for display purposes in the UI."), metadataField: z.string().optional(), -}); +}).superRefine((data, ctx) => { + if (data.attribute === "metadata" && !data.metadataField?.trim()) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["metadataField"], + message: "Metadata field is required when attribute is metadata.", + }); + } +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/zod/schemas/rewards.ts` around lines 289 - 308, rewardConditionSchema currently allows attribute === "metadata" without a metadataField, which produces non-matching conditions; update rewardConditionSchema to enforce metadataField when attribute is "metadata" by adding a conditional refinement: after the object schema definition, call .superRefine or .refine on rewardConditionSchema to check if data.attribute === "metadata" and if so ensure data.metadataField is a non-empty string, otherwise add a validation issue targeting "metadataField" with a clear message; keep metadataField typed as z.string() (not optional) in the error path check but leave the original union for other attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/lib/partners/evaluate-reward-conditions.ts`:
- Around line 119-161: The bug is that textOperators currently includes "in" and
"not_in" so numeric metadata like 42 is coerced to the string "42" and won’t
match numeric condition values; update the coercion logic in
evaluate-reward-conditions so that "in" and "not_in" are treated like numeric
operators when the raw value is numeric (or a numeric string/boolean) — either
remove "in"/"not_in" from textOperators and add them to numberOperators, or add
a guard in the textOperators branch that checks op === "in" || op === "not_in"
and then runs the same numeric coercion used for numberOperators (using the
existing op, raw, Number(raw) checks) so numeric inputs return numbers instead
of strings.
In `@apps/web/lib/zod/schemas/rewards.ts`:
- Around line 229-235: The current construction using new Map(... .map(attr =>
[attr.id, attr])) collapses distinct attribute definitions that share the same
id (losing source-specific fields like `trial`), so change the dedup key to
preserve uniqueness across source/lead (e.g., use a composite key such as
`${attr.source}:${attr.id}` or include `attr.source` in the map key) or avoid
mapping to a Map entirely and keep the full flattened list; update any enum
creation logic that currently assumes unique ids so it builds enums from the
preserved per-source attributes (references: REWARD_CONDITIONS, the .flatMap
over entities -> attributes, and the Map key using attr.id).
---
Outside diff comments:
In `@apps/web/lib/zod/schemas/rewards.ts`:
- Around line 289-308: rewardConditionSchema currently allows attribute ===
"metadata" without a metadataField, which produces non-matching conditions;
update rewardConditionSchema to enforce metadataField when attribute is
"metadata" by adding a conditional refinement: after the object schema
definition, call .superRefine or .refine on rewardConditionSchema to check if
data.attribute === "metadata" and if so ensure data.metadataField is a non-empty
string, otherwise add a validation issue targeting "metadataField" with a clear
message; keep metadataField typed as z.string() (not optional) in the error path
check but leave the original union for other attributes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d30776d1-971f-40fd-8e0c-474e600294a9
📒 Files selected for processing (7)
apps/web/lib/api/conversions/track-lead.tsapps/web/lib/api/conversions/track-sale.tsapps/web/lib/partners/evaluate-reward-conditions.tsapps/web/lib/zod/schemas/rewards.tsapps/web/tests/rewards/reward-conditions.test.tsapps/web/ui/partners/program-reward-modifiers-tooltip.tsxapps/web/ui/partners/rewards/rewards-logic.tsx
|
@coderabbitai full review. any breaking changes? |
🧩 Analysis chainI'll do a thorough review of the PR and check for breaking changes by inspecting the actual code. 🏁 Script executed: Length of output: 47352 🏁 Script executed: Length of output: 47443 🏁 Script executed: Length of output: 379 🏁 Script executed: Length of output: 54599
Breaking ChangesNo hard runtime breaking changes — all schema additions are opt-in (optional fields, additive union types). However, there are a few areas to watch: 1. TypeScript Exhaustive-Switch Risk
// Example: this would break at compile time if not updated
switch (entity.id) {
case "partner": ...
case "customer": ...
case "sale": ...
// TS error: "lead" not handled
}2.
|
| Breaking? | Severity | |
|---|---|---|
RewardConditionEntity.id union expansion |
TypeScript compile risk | Medium |
REWARD_CONDITION_ATTRIBUTES duplicates |
Behavioral | Low–Medium |
_trackLead missing lead metadata in commission |
Functional bug | High |
UI in/not_in numeric list type mismatch |
Functional bug | Medium |
| Boolean metadata coercion | Behavioral edge case | Low |
superRefine missing entity check |
Data integrity | Low |
The most impactful issue to address before merging is #3 (_trackLead missing metadata) and #4 (numeric in/not_in from the UI).
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/ui/partners/rewards/rewards-logic.tsx (1)
778-794:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMetadata
in/not_insaves a string array, but the evaluator coerces raw metadata to number, so numeric-looking matches silently fail.The user enters comma-separated values in
InlineBadgePopoverInputs, which always storescondition.valueasstring[](e.g.,["1","2","3"]). For metadata conditions, the backendprepareMetadataFieldValuetries numeric parsing first forin/not_in, so a metadata value of5or"5"becomesfieldValue = 5(number), and the evaluator runs["1","2","3"].includes(5)→ alwaysfalse.Two options to keep the UI and evaluator consistent:
- In
prepareMetadataFieldValue, forin/not_in, look at the element types ofcondition.valueand coercefieldValueaccordingly (string array → return string; number array → return number).- Or, in this UI branch, when
isMetadataNumeric(or operator isin/not_inwith a numeric intent), parse entries as numbers before saving.Backend-side fix is preferable since it handles mixed/legacy data:
♻️ Suggested backend coercion (in evaluate-reward-conditions.ts)
- const numeric = parseMetadataNumeric(raw); - if (ordering) return numeric; - - return numeric !== undefined ? numeric : metadataRawToString(raw); + const numeric = parseMetadataNumeric(raw); + if (ordering) return numeric; + + // For equals_to/not_equals/in/not_in, match the type of condition.value + // so a UI-saved string array (e.g. ["1","2"]) still matches numeric raw. + const expectsString = + typeof condition.value === "string" || + (Array.isArray(condition.value) && + condition.value.every((v) => typeof v === "string")); + if (expectsString) return metadataRawToString(raw); + return numeric !== undefined ? numeric : metadataRawToString(raw);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/ui/partners/rewards/rewards-logic.tsx` around lines 778 - 794, The UI stores comma-separated metadata as string[] via InlineBadgePopoverInputs (condition.value), but prepareMetadataFieldValue (used by evaluate-reward-conditions.ts) currently parses metadata to number for in/not_in operators causing ["1","2"].includes(1) to fail; update prepareMetadataFieldValue to detect when the condition.operator is "in" or "not_in" and the condition.value is an array: inspect element types of condition.value (e.g., any element parses as number vs all non-numeric) and coerce the incoming fieldValue to match (convert fieldValue to string if condition.value elements are strings, or to number if elements are numeric), so comparisons use matching types; keep existing numeric-first parse for non-array cases to preserve legacy behavior.
🧹 Nitpick comments (2)
apps/web/ui/partners/program-reward-modifiers-tooltip.tsx (1)
172-178: 💤 Low valueOptional: truncate long
metadataFieldnames to keep the tooltip readable.
metadataFieldis a free-form user-entered string and isn't length-capped at the schema level. A long key like"reallyLongMetadataKeyNameThatBreaksLayout"will be rendered inline as-is here, but the form UI already truncates to 24 chars viatruncate(condition.metadataField.trim(), 24). Matching that here keeps tooltip layout predictable.♻️ Suggested change
- {(condition.entity === "lead" || - condition.entity === "sale") && - condition.attribute === "metadata" && - condition.metadataField?.trim() - ? `"${condition.metadataField.trim()}"` - : capitalize(attribute?.label)}{" "} + {(condition.entity === "lead" || + condition.entity === "sale") && + condition.attribute === "metadata" && + condition.metadataField?.trim() + ? `"${truncate(condition.metadataField.trim(), 24)}"` + : capitalize(attribute?.label)}{" "}And add
truncateto the@dub/utilsimport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/ui/partners/program-reward-modifiers-tooltip.tsx` around lines 172 - 178, Tooltip currently renders metadataField raw which can be arbitrarily long; import and use the existing truncate utility from `@dub/utils` and replace the inline `condition.metadataField.trim()` display with `truncate(condition.metadataField.trim(), 24)` (keep the same condition checks around `condition.entity === "lead" || condition.entity === "sale" && condition.attribute === "metadata"` and the surrounding JSX that currently renders `"${...}"`). Also add `truncate` to the `@dub/utils` import so the component (program-reward-modifiers-tooltip) compiles.apps/web/lib/zod/schemas/rewards.ts (1)
287-318: ⚡ Quick winConsider validating entity ↔ attribute compatibility in
superRefine.The schema validates that
metadataFieldis present whenattribute === "metadata", but it does not enforce that the(entity, attribute)pair is actually defined inREWARD_CONDITIONS. For example, a payload withentity: "partner"andattribute: "metadata", orentity: "lead"with a non-metadata attribute likecountry, will pass validation and persist, butresolveConditionFieldValuereturnsundefinedfor these combinations (silent never-match at evaluation time).A small cross-check would catch these on save:
♻️ Optional: enforce entity/attribute compatibility
.superRefine((data, ctx) => { + const allowedAttributes = new Set( + Object.values(REWARD_CONDITIONS) + .flatMap(({ entities }) => entities) + .filter((e) => e.id === data.entity) + .flatMap((e) => e.attributes.map((a) => a.id)), + ); + if (!allowedAttributes.has(data.attribute)) { + ctx.addIssue({ + code: "custom", + message: `Attribute "${data.attribute}" is not valid for entity "${data.entity}".`, + path: ["attribute"], + }); + } if (data.attribute !== "metadata") { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/zod/schemas/rewards.ts` around lines 287 - 318, The superRefine currently only enforces metadataField when attribute === "metadata" but doesn't validate that the entity/attribute combination actually exists in REWARD_CONDITIONS, causing silent evaluation failures in resolveConditionFieldValue; update rewardConditionSchema.superRefine to look up the REWARD_CONDITIONS entry for data.entity and ensure data.attribute is one of that entity's defined attributes (and if attribute === "metadata" that the entity supports metadata), and call ctx.addIssue with an appropriate message and path (e.g., ["attribute"] or ["entity"]) when the pair is invalid so invalid combinations are rejected at validation time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/lib/partners/evaluate-reward-conditions.ts`:
- Around line 139-144: The current parseMetadataNumeric function coerces
booleans to numbers (Number(true) => 1), causing boolean metadata to match
numeric operators; change the boolean branch so that when typeof raw ===
"boolean" the function returns undefined (i.e., treat booleans as non-numeric)
instead of Number(raw), leaving the rest of the logic unchanged; update any
tests that expected boolean-to-number coercion to reflect the new behavior.
In `@apps/web/tests/rewards/reward-conditions.test.ts`:
- Around line 2132-2174: Add tests to assert that using attribute: "metadata" is
only valid for allowed entities (e.g., "lead" and "sale") by adding negative
cases that safeParse returns false for unsupported entities (e.g., "customer",
"partner") and positive cases for allowed entities; update the existing
rewardConditionSchema unit tests by adding expectations that
rewardConditionSchema.safeParse({ entity: "customer", attribute: "metadata",
operator: "...", value: "...", metadataField: "plan" }).success is false (and
similar for "partner"), while keeping/adding positive tests for
rewardConditionSchema.safeParse({ entity: "lead", attribute: "metadata", ... })
and ({ entity: "sale", attribute: "metadata", ... }) to ensure only the
permitted entities accept metadata conditions.
- Around line 1961-1992: Add tests that cover UI-shaped metadata list values
(string[]) for the `in` and `not_in` operators: create cases where the condition
in `conditions` uses value: ['1','2','3'] and assert `evaluateRewardConditions({
conditions, context })` returns the expected condition when the sale metadata is
numeric (tier: 2) and when it is a numeric-string (tier: '2'), and likewise add
complementary `not_in` assertions to ensure non-matching behavior; locate the
test file around the existing "matches sale metadata in when metadata is numeric
and list values are numbers" block and add new test cases referencing
`evaluateRewardConditions` and the same condition shape (entity: 'sale',
attribute: 'metadata', metadataField: 'tier', operator: 'in'/'not_in') so the
string-array path is covered.
---
Outside diff comments:
In `@apps/web/ui/partners/rewards/rewards-logic.tsx`:
- Around line 778-794: The UI stores comma-separated metadata as string[] via
InlineBadgePopoverInputs (condition.value), but prepareMetadataFieldValue (used
by evaluate-reward-conditions.ts) currently parses metadata to number for
in/not_in operators causing ["1","2"].includes(1) to fail; update
prepareMetadataFieldValue to detect when the condition.operator is "in" or
"not_in" and the condition.value is an array: inspect element types of
condition.value (e.g., any element parses as number vs all non-numeric) and
coerce the incoming fieldValue to match (convert fieldValue to string if
condition.value elements are strings, or to number if elements are numeric), so
comparisons use matching types; keep existing numeric-first parse for non-array
cases to preserve legacy behavior.
---
Nitpick comments:
In `@apps/web/lib/zod/schemas/rewards.ts`:
- Around line 287-318: The superRefine currently only enforces metadataField
when attribute === "metadata" but doesn't validate that the entity/attribute
combination actually exists in REWARD_CONDITIONS, causing silent evaluation
failures in resolveConditionFieldValue; update rewardConditionSchema.superRefine
to look up the REWARD_CONDITIONS entry for data.entity and ensure data.attribute
is one of that entity's defined attributes (and if attribute === "metadata" that
the entity supports metadata), and call ctx.addIssue with an appropriate message
and path (e.g., ["attribute"] or ["entity"]) when the pair is invalid so invalid
combinations are rejected at validation time.
In `@apps/web/ui/partners/program-reward-modifiers-tooltip.tsx`:
- Around line 172-178: Tooltip currently renders metadataField raw which can be
arbitrarily long; import and use the existing truncate utility from `@dub/utils`
and replace the inline `condition.metadataField.trim()` display with
`truncate(condition.metadataField.trim(), 24)` (keep the same condition checks
around `condition.entity === "lead" || condition.entity === "sale" &&
condition.attribute === "metadata"` and the surrounding JSX that currently
renders `"${...}"`). Also add `truncate` to the `@dub/utils` import so the
component (program-reward-modifiers-tooltip) compiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b54f732-198f-43fd-8ed4-7ed09fc44e1c
📒 Files selected for processing (7)
apps/web/lib/api/conversions/track-lead.tsapps/web/lib/api/conversions/track-sale.tsapps/web/lib/partners/evaluate-reward-conditions.tsapps/web/lib/zod/schemas/rewards.tsapps/web/tests/rewards/reward-conditions.test.tsapps/web/ui/partners/program-reward-modifiers-tooltip.tsxapps/web/ui/partners/rewards/rewards-logic.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/web/tests/rewards/reward-conditions.test.ts (1)
479-566: ⚡ Quick winMissing negative test: substring needle should not over-match.
The new substring semantics for
in/not_inmeansvalue: "S"will matchcountry: "US"— a behavior shift from the previous array-membership model. Consider an explicit test asserting this is intended (or guarding against it), e.g.value: "U"withcountry: "US"should match, whilevalue: "USA"withcountry: "US"should not. Without an anchor, copy-pasted single-letter ISO-code conditions will produce surprising matches across many countries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/tests/rewards/reward-conditions.test.ts` around lines 479 - 566, Add a negative test in the "in (contains substring)" suite to prevent over-matching: using evaluateRewardConditions, create a condition with operator "in" on entity "customer" attribute "country" and assert that value: "USA" does NOT match context.customer.country = "US" (expect null), and also include a sanity check that a shorter needle like value: "U" DOES match "US" if you want to document current semantics; place the tests alongside the existing ones in the describe block to ensure the new substring semantics don't accidentally over-match.apps/web/lib/partners/evaluate-reward-conditions.ts (2)
94-107: 💤 Low valueType assertion gap: indexed access into
context.{customer,sale,partner}is no longer narrowed.The function's declared return type is
string | number | string[] | number[] | undefined, butcontext.customer?.[condition.attribute]etc. can in principle returnDate | boolean | null | objectdepending on theRewardContextshape (e.g.signupDateis aDatein the existing date-condition tests on lines 1786–1898). The code happens to work because downstreamNumber()/String()coerce, but the type contract is being violated and a future refactor relying on the return type could silently break. Consider returningunknownand tightening narrowing insideevaluateCondition, or adding an explicit cast/normalization here.
77-92: 💤 Low valueMetadata resolution silently ignores invalid entity/attribute combos.
When
condition.attribute === "metadata"butcondition.entityis neither"lead"nor"sale"(e.g."customer"or"partner"), this falls through toreturn undefined, which causesevaluateConditionto be skipped and the row to be treated as non-matching. The schema now rejects such combos at write time (per test at lines 2201–2221), but any pre-existing rows or out-of-band writes will be silently ignored rather than surfacing an error or being logged. Consider at minimum a debug log for observability when an unsupported entity is paired withattribute: "metadata".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/partners/evaluate-reward-conditions.ts` around lines 77 - 92, The metadata branch in evaluate-reward-conditions.ts silently returns undefined for unsupported entities, so update the logic in the block handling condition.attribute === "metadata" (the branch that uses metadataField, prepareMetadataFieldValue, context.lead and context.sale) to surface an observability signal instead of quietly returning; specifically, when condition.entity is not "lead" or "sale" log a debug/error with the condition (include condition.entity and metadataField) and the current context, and then return undefined (or throw if policy requires) so callers can detect the invalid combo—make the change where prepareMetadataFieldValue is invoked and ensure the log uses the existing logger used in this module or a clear console/error call if none exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/lib/partners/evaluate-reward-conditions.ts`:
- Around line 170-182: The `not_in` case ends up matching everything because
fieldContainsSubstring currently returns false for null/empty needles which is
then negated; update fieldContainsSubstring to return a tri-state
(null/undefined for invalid needle, true/false for actual match) and then change
the condition resolution in the not_in branch (in evaluate-reward-conditions) to
treat a null/invalid needle as a non-match (i.e., short-circuit to false or skip
the condition) instead of negating the invalid result; alternatively, without
changing the return type, add an explicit pre-check at the not_in call site to
detect needle === null/undefined or String(needle).trim() === "" and handle it
by returning false or skipping the condition.
- Around line 150-168: The current prepareMetadataFieldValue prefers numeric
parsing for equals/not_equals which causes asymmetric comparisons against
condition.value of a different type; change prepareMetadataFieldValue to choose
coercion based on the type of condition.value: when condition.operator is in
METADATA_EQUALS_OPS, inspect condition.value (and if it's an array, inspect its
first element's type) and if the condition.value is a number attempt numeric
parsing via parseMetadataNumeric (fall back to string if parse fails), but if
condition.value is a string prefer metadataRawToString; retain the existing
behavior for ordering ops (METADATA_NUMERIC_ORDER_OPS) and text ops
(METADATA_TEXT_OPS); ensure evaluateCondition's strict === comparisons will then
see matched types.
- Around line 212-215: The change flipped "in"/"not_in" semantics to substring
matching which breaks existing array-based conditions; update
evaluate-reward-conditions.ts to preserve backward-compatible behavior by
checking for Array.isArray(condition.value) inside the "in" and "not_in" cases:
if condition.value is an array, use array-membership semantics
(value.includes(fieldValue) or !includes), otherwise fall back to the existing
fieldContainsSubstring(fieldValue, condition.value) logic; also ensure the
Reward model's modifiers migration/backfill plan is executed if you intend to
normalize stored arrays to strings (add a DB migration that converts
array-valued condition.value to the expected string form before deploying this
semantic change).
---
Nitpick comments:
In `@apps/web/lib/partners/evaluate-reward-conditions.ts`:
- Around line 77-92: The metadata branch in evaluate-reward-conditions.ts
silently returns undefined for unsupported entities, so update the logic in the
block handling condition.attribute === "metadata" (the branch that uses
metadataField, prepareMetadataFieldValue, context.lead and context.sale) to
surface an observability signal instead of quietly returning; specifically, when
condition.entity is not "lead" or "sale" log a debug/error with the condition
(include condition.entity and metadataField) and the current context, and then
return undefined (or throw if policy requires) so callers can detect the invalid
combo—make the change where prepareMetadataFieldValue is invoked and ensure the
log uses the existing logger used in this module or a clear console/error call
if none exists.
In `@apps/web/tests/rewards/reward-conditions.test.ts`:
- Around line 479-566: Add a negative test in the "in (contains substring)"
suite to prevent over-matching: using evaluateRewardConditions, create a
condition with operator "in" on entity "customer" attribute "country" and assert
that value: "USA" does NOT match context.customer.country = "US" (expect null),
and also include a sanity check that a shorter needle like value: "U" DOES match
"US" if you want to document current semantics; place the tests alongside the
existing ones in the describe block to ensure the new substring semantics don't
accidentally over-match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2c82e13-68f5-4b6e-b541-b0e3e2084889
📒 Files selected for processing (4)
apps/web/lib/partners/evaluate-reward-conditions.tsapps/web/lib/zod/schemas/rewards.tsapps/web/tests/rewards/reward-conditions.test.tsapps/web/ui/partners/rewards/rewards-logic.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/lib/zod/schemas/rewards.ts
- apps/web/ui/partners/rewards/rewards-logic.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/tests/rewards/reward-conditions.test.ts (1)
1874-2128: ⚡ Quick winMissing test coverage for
in/not_inmetadata operators.The new metadata block covers
equals_to,greater_than, and null/empty/missing handling, but does not exercisein/not_inagainst metadata — which is exactly the path most likely to silently fail today (numeric metadata vs numeric/string list values, see the comment inevaluate-reward-conditions.ts). Adding these would lock in the intended semantics and surface the type-coercion gap.Proposed additions
test("matches sale metadata in with numeric list and numeric metadata", () => { const conditions = [ { operator: "AND" as const, type: "flat" as const, amountInCents: 300, conditions: [ { entity: "sale" as const, attribute: "metadata" as const, metadataField: "tier", operator: "in" as const, value: [1, 2, 3], }, ], }, ]; expect( evaluateRewardConditions({ conditions, context: { sale: { metadata: { tier: 2 } } }, }), ).toEqual(conditions[0]); }); test("matches sale metadata in with UI-shaped string list", () => { const conditions = [ { operator: "AND" as const, type: "flat" as const, amountInCents: 301, conditions: [ { entity: "sale" as const, attribute: "metadata" as const, metadataField: "tier", operator: "in" as const, value: ["1", "2", "3"], }, ], }, ]; expect( evaluateRewardConditions({ conditions, context: { sale: { metadata: { tier: 2 } } }, }), ).toEqual(conditions[0]); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/tests/rewards/reward-conditions.test.ts` around lines 1874 - 2128, Add tests exercising the metadata "in" and "not_in" operators for both numeric and string-list condition values to lock in type-coercion behavior: create cases that call evaluateRewardConditions with a condition whose inner condition uses entity "sale", attribute "metadata", a metadataField like "tier", operator "in" (and a separate test for "not_in"), and value = [1,2,3] asserting a numeric metadata value 2 matches, plus value = ["1","2","3"] asserting the same numeric metadata 2 also matches; also add a negative case asserting "not_in" returns null (or does not match) when the metadata is present in the list. Ensure tests reference evaluateRewardConditions, metadataField, and operator exactly so they exercise the code paths for list membership and type coercion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/lib/partners/evaluate-reward-conditions.ts`:
- Around line 110-115: The "in"/"not_in" branches treat metadata values as
strings (METADATA_TEXT_OPS) causing numeric mismatches; update evaluateCondition
(and the logic handling METADATA_TEXT_OPS) so when op is "in" or "not_in" you
detect the element type of condition.value (e.g., first element) and coerce the
fieldValue and the condition.value elements to that type (number if first
element is numeric, otherwise string), or alternatively attempt both numeric and
string comparisons and succeed if either matches; ensure you mirror the
equals_to/not_equals coercion strategy so numeric metadata like 42 will
correctly match arrays like [1,2,3] and string arrays like ["1","2","3"] behave
as expected.
---
Nitpick comments:
In `@apps/web/tests/rewards/reward-conditions.test.ts`:
- Around line 1874-2128: Add tests exercising the metadata "in" and "not_in"
operators for both numeric and string-list condition values to lock in
type-coercion behavior: create cases that call evaluateRewardConditions with a
condition whose inner condition uses entity "sale", attribute "metadata", a
metadataField like "tier", operator "in" (and a separate test for "not_in"), and
value = [1,2,3] asserting a numeric metadata value 2 matches, plus value =
["1","2","3"] asserting the same numeric metadata 2 also matches; also add a
negative case asserting "not_in" returns null (or does not match) when the
metadata is present in the list. Ensure tests reference
evaluateRewardConditions, metadataField, and operator exactly so they exercise
the code paths for list membership and type coercion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d8aec4e-f36f-4edb-8abb-82e02e678230
📒 Files selected for processing (4)
apps/web/lib/partners/evaluate-reward-conditions.tsapps/web/lib/zod/schemas/rewards.tsapps/web/tests/rewards/reward-conditions.test.tsapps/web/ui/partners/rewards/rewards-logic.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/lib/zod/schemas/rewards.ts
- apps/web/ui/partners/rewards/rewards-logic.tsx
Move post-sale side effects out of track-sale into the QStash workflow, rename triggerWorkflows to triggerQStashWorkflow, and add getErrorMessage.
Avoid conflating route names with QStash run IDs; use console in partner-approved and add trigger label for observability.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/lib/partners/evaluate-reward-conditions.ts`:
- Around line 209-213: The "not_contains" branch currently treats an empty or
whitespace-only condition.value as a universal match; change its fallback so
empty needle does NOT match everything. In the case "not_contains" block
(referencing condition.value and fieldValue), after confirming typeof
condition.value === "string" and doing const needle = condition.value.trim(),
return false when needle === "" (instead of true) so a blank condition does not
cause every record to match; keep the existing
!String(fieldValue).includes(needle) behavior for non-empty needles.
In `@apps/web/lib/zod/schemas/rewards.ts`:
- Around line 325-352: The refinement on rewardConditionSchema currently only
special-cases attribute === "metadata"; add a guard in the superRefine (on
rewardConditionSchema / rewardConditionBaseSchema) that if data.entity ===
"lead" and data.attribute !== "metadata" then call ctx.addIssue with a custom
error (e.g., message like "Lead entity only supports metadata attribute") and
path ["attribute"] to reject those invalid pairs; keep the existing metadata
validation (using REWARD_METADATA_CONDITION_ENTITIES and metadataField) intact.
In `@apps/web/ui/partners/rewards/rewards-logic.tsx`:
- Around line 415-424: availableConditionOperators is falling back to
STRING_CONDITION_OPERATORS for non-metadata string fields so the new
"contains"/"not_contains" ops never show; update the ternary so that when
attributeType represents a non-metadata string (e.g., attributeType === "string"
or the default branch for non-number/currency/enum/date non-metadata attributes)
it uses CONDITION_OPERATORS instead of STRING_CONDITION_OPERATORS. Locate the
const availableConditionOperators and replace the final fallback
(STRING_CONDITION_OPERATORS) with CONDITION_OPERATORS (or add an explicit
attributeType === "string" branch that returns CONDITION_OPERATORS) so fields
like sale.productId can pick the new operators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 221eb4c1-23f0-41a4-995d-8970c1586e71
📒 Files selected for processing (8)
apps/web/lib/api/conversions/track-lead.tsapps/web/lib/api/conversions/track-sale.tsapps/web/lib/partners/evaluate-reward-conditions.tsapps/web/lib/zod/schemas/rewards.tsapps/web/tests/rewards/reward-conditions.test.tsapps/web/ui/partners/program-reward-modifiers-tooltip.tsxapps/web/ui/partners/rewards/add-edit-reward-sheet.tsxapps/web/ui/partners/rewards/rewards-logic.tsx
pepeladeira
force-pushed
the
lead-sales-rewards-metadata-condition
branch
from
0c3d430 to
de4cdde
Compare
3 participants
Summary by CodeRabbit
New Features
UI
Refactor
Tests