Fix null-unsafe payments config validation for partial overrides#1363
Fix null-unsafe payments config validation for partial overrides#1363mantrakp04 wants to merge 5 commits intodevfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughEnhanced type-safety in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/stack-shared/src/config/schema.test.ts (1)
31-56: ConsidertoMatchInlineSnapshotfor error-message assertions.Per the repo's testing guideline ("prefer
.toMatchInlineSnapshotover other selectors, if possible"), the exact-stringrejects.toThrow(...)assertions here would be easier to update when the error format evolves if expressed as snapshots. Not blocking — the current assertions are correct and sufficiently precise.Example refactor
- })).rejects.toThrow('Product "pro" specifies product line ID "missing-line", but that product line does not exist'); + })).rejects.toThrowErrorMatchingInlineSnapshot();As per coding guidelines: "When writing tests, prefer .toMatchInlineSnapshot over other selectors, if possible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-shared/src/config/schema.test.ts` around lines 31 - 56, Replace the explicit string assertions in the two tests that call branchPaymentsSchema.validate with Jest inline-snapshot assertions: instead of await expect(...).rejects.toThrow('...'), use await expect(...).rejects.toThrowErrorMatchingInlineSnapshot(`...`) (or .toThrowErrorMatchingInlineSnapshot for async rejects) for both the "rejects a product that references a missing product line" test and the "rejects a product whose customer type differs from its product line" test, and paste the current error strings into the inline snapshots so future message changes are easier to update via snapshot tooling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stack-shared/src/config/schema.ts`:
- Around line 188-190: Guard against non-object product entries before accessing
product.productLineId: inside the loop that iterates Object.entries(products)
check isObjectLike(product) (or equivalent) and continue if false, then only
compute productLine via getOrUndefined(productLines, product.productLineId) when
product is object-like; this ensures products, product, productLineId,
isObjectLike, getOrUndefined and the Object.entries(products) loop tolerate
null/primitive entries instead of throwing.
---
Nitpick comments:
In `@packages/stack-shared/src/config/schema.test.ts`:
- Around line 31-56: Replace the explicit string assertions in the two tests
that call branchPaymentsSchema.validate with Jest inline-snapshot assertions:
instead of await expect(...).rejects.toThrow('...'), use await
expect(...).rejects.toThrowErrorMatchingInlineSnapshot(`...`) (or
.toThrowErrorMatchingInlineSnapshot for async rejects) for both the "rejects a
product that references a missing product line" test and the "rejects a product
whose customer type differs from its product line" test, and paste the current
error strings into the inline snapshots so future message changes are easier to
update via snapshot tooling.
🪄 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: 88852a85-2731-48c4-81c7-881a24be1d68
📒 Files selected for processing (2)
packages/stack-shared/src/config/schema.test.tspackages/stack-shared/src/config/schema.ts
Greptile SummaryThis PR makes the Confidence Score: 5/5Safe to merge — the fix is narrowly scoped to the cross-field validator and the test coverage is appropriate. All changes are defensive guards that cannot regress existing valid inputs; the only semantic shift (falsy → No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["branchPaymentsSchema .test()"] --> B{"value is falsy?"}
B -- Yes --> C["return true (pass)"]
B -- No --> D["products = Reflect.get(value, 'products')"]
D --> E{"isObjectLike(products)?"}
E -- No --> C
E -- Yes --> F["productLines = Reflect.get(value, 'productLines')"]
F --> G["iterate Object.entries(products)"]
G --> H{"isObjectLike(product)?"}
H -- No --> G
H -- Yes --> I{"product.productLineId == null?"}
I -- Yes --> G
I -- No --> J{"isObjectLike(productLines)?"}
J -- No --> K["productLine = undefined"]
J -- Yes --> L["productLine = getOrUndefined(productLines, productLineId)"]
K --> M{"productLine === undefined?"}
L --> M
M -- Yes --> N["createError: missing product line"]
M -- No --> O{"isObjectLike(productLine)?"}
O -- No --> G
O -- Yes --> P{"customerType mismatch?"}
P -- Yes --> Q["createError: type mismatch"]
P -- No --> G
G -- done --> R["return true (pass)"]
Reviews (2): Last reviewed commit: "fix: guard null payments config entries" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates the branchPaymentsSchema custom Yup validator in packages/stack-shared to avoid runtime crashes when validating partial payments overrides (e.g., missing products / productLines), and adds targeted Vitest regression coverage around those cases and existing referential/customer-type validation behavior.
Changes:
- Make the
branchPaymentsSchemacustom validator tolerant of missingpayments.products/payments.productLines. - Add
branchPaymentsSchemaunit tests covering partial configs and key validation failures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/stack-shared/src/config/schema.ts |
Hardens the product/productLine cross-field validator against absent products / productLines. |
packages/stack-shared/src/config/schema.test.ts |
Adds focused regression tests for partial configs and validation error cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("rejects a product that references a missing product line", async () => { | ||
| await expect(branchPaymentsSchema.validate({ | ||
| products: { | ||
| pro: { | ||
| customerType: "user", | ||
| productLineId: "missing-line", | ||
| }, | ||
| }, | ||
| })).rejects.toThrow('Product "pro" specifies product line ID "missing-line", but that product line does not exist'); | ||
| }); | ||
|
|
||
| it("rejects a product whose customer type differs from its product line", async () => { | ||
| await expect(branchPaymentsSchema.validate({ | ||
| productLines: { | ||
| teamLine: { | ||
| customerType: "team", | ||
| }, | ||
| }, | ||
| products: { | ||
| pro: { | ||
| customerType: "user", | ||
| productLineId: "teamLine", | ||
| }, | ||
| }, | ||
| })).rejects.toThrow('Product "pro" has customer type "user" but its product line "teamLine" has customer type "team"'); |
There was a problem hiding this comment.
These two rejection tests build products.pro objects that are not otherwise valid productSchema instances (notably missing required prices). Right now the custom referential-integrity test likely runs before nested validation, so you get the desired error, but this is order-dependent and could become flaky if Yup/test ordering changes. Consider making the product objects minimally valid (e.g., include a valid prices value) so the failure reason is unambiguously the missing product line / customer-type mismatch.
| const productLines = Reflect.get(value, "productLines"); | ||
| for (const [productId, product] of Object.entries(products)) { | ||
| if (product.productLineId == null) continue; | ||
| const productLine = isObjectLike(productLines) ? getOrUndefined(productLines, product.productLineId) : undefined; |
There was a problem hiding this comment.
In this custom test, products being object-like doesn’t guarantee each product value is object-like. Because Yup runs parent .test(...) callbacks before it validates nested schemas/records, a config like products: { pro: null } (or a non-object) will still reach product.productLineId and can throw at runtime instead of returning a validation error. Consider guarding product with isObjectLike(product) (and reading fields via Reflect.get/type checks) before accessing productLineId/customerType, letting the products record validation report the shape error.
|
@greptile-ai review |
nams1570
left a comment
nams1570
left a comment
There was a problem hiding this comment.
Test-wise, why not just append to the unit tests in schema.ts? Also, is there a reason behind not using inline snapshots? that's more in line with how the codebase handles it.
Also, it would be nice to have them run abortEarly = false tests if possible since by default that's what smart-request runs.
| const productLines = value.productLines; | ||
| for (const [productId, product] of Object.entries(products)) { | ||
| if (!isObjectLike(product)) continue; | ||
| if (product.productLineId == null) continue; |
There was a problem hiding this comment.
Discussion: this is a stricter check than what we had before. Is there a reason you switched it? This change would mean empty product line ids are not skipped.
…nition with improved validation
Summary
branchPaymentsSchemacustom validator tolerant of partial override objectspayments.productsorpayments.productLinesare absent during validationTesting
packages/stack-sharedSummary by CodeRabbit
Bug Fixes
Tests