[Dashboard] Fix: Purchase Data #7225
Conversation
…ved payment link handling
🦋 Changeset detectedLatest commit: ea2ad78 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
""" WalkthroughThe changes refine the typing of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PayPage
participant PayPageEmbed
participant PayEmbed
User->>PayPage: Access payment link (with purchaseData)
PayPage->>PayPageEmbed: Render with purchaseData prop
PayPageEmbed->>PayEmbed: Pass purchaseData in payOptions
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7225 +/- ##
=======================================
Coverage 55.62% 55.62%
=======================================
Files 908 908
Lines 58570 58570
Branches 4133 4133
=======================================
Hits 32577 32577
Misses 25886 25886
Partials 107 107
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
apps/dashboard/src/app/pay/[id]/page.tsx (1)
68-68: Add error handling for purchaseData access.Consider adding error handling to gracefully handle cases where
paymentLink.purchaseDatamight be undefined or contain unexpected data types.- purchaseData={paymentLink.purchaseData} + purchaseData={paymentLink.purchaseData ?? undefined}Alternatively, add validation:
+ // Validate purchaseData + const purchaseData = paymentLink.purchaseData && + typeof paymentLink.purchaseData === 'object' + ? paymentLink.purchaseData + : undefined; + return ( <PayPageEmbed // ... other props - purchaseData={paymentLink.purchaseData} + purchaseData={purchaseData} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/src/app/pay/[id]/page.tsx(1 hunks)apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Unit Tests
- GitHub Check: Build Packages
- GitHub Check: Size
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx (1)
55-55: Inconsistency with AI summary - verify placement of purchaseData.The AI summary states that
purchaseDatais forwarded intopayOptions.metadata, but the implementation adds it directly topayOptions. Please verify this is the intended behavior and ensure it aligns with the PayEmbed component's expected interface.#!/bin/bash # Description: Verify PayEmbed component interface and purchaseData usage # Expected: Find PayEmbed component definition and its payOptions interface ast-grep --pattern $'interface $_ { $$$ purchaseData$_ $$$ }' # Also search for PayEmbed component usage patterns rg -A 10 -B 5 "PayEmbed.*payOptions"Likely an incorrect or invalid review comment.
apps/dashboard/src/app/pay/[id]/page.tsx (1)
68-68:✅ Verification successful
Verify paymentLink.purchaseData property exists in the data model.
Ensure that the
purchaseDataproperty is properly defined in thepaymentLinkobject's type definition and that the API response includes this field.
🏁 Script executed:
#!/bin/bash # Description: Verify paymentLink type definition includes purchaseData # Expected: Find interface/type definitions for paymentLink ast-grep --pattern $'interface $_ { $$$ purchaseData$_ $$$ }' ast-grep --pattern $'type $_ = { $$$ purchaseData$_ $$$ }' # Search for getPaymentLink function return type rg -A 20 "getPaymentLink.*=>"Length of output: 166
🏁 Script executed:
#!/bin/bash # Locate any type or interface definitions for PaymentLink rg -n "export (interface|type) PaymentLink" # Search for all occurrences of 'purchaseData' to see where it’s referenced or defined rg -n "purchaseData"Length of output: 11919
paymentLink.purchaseData property is defined and mapped correctly
- apps/dashboard/src/@/api/universal-bridge/links.ts defines the
PaymentLinktype withpurchaseData: unknown- The API response maps
data.purchaseDatainto this fieldNo changes required.
| @@ -16,6 +16,7 @@ export function PayPageEmbed({ | |||
| image, | |||
| redirectUri, | |||
| theme, | |||
| purchaseData, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider more specific typing for purchaseData.
The unknown | undefined type is very broad and could lead to type safety issues downstream. Consider defining a more specific interface or using a generic type parameter to provide better type safety and developer experience.
- purchaseData: unknown | undefined;
+ purchaseData?: Record<string, unknown>;Or define a specific interface:
interface PurchaseData {
orderId?: string;
customFields?: Record<string, unknown>;
// other expected fields...
}Also applies to: 31-31
🤖 Prompt for AI Agents
In apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx at lines
19 and 31, the purchaseData prop is typed as unknown | undefined, which is too
broad and unsafe. Define a specific TypeScript interface describing the expected
structure of purchaseData, including fields like orderId and customFields, and
update the typing of purchaseData to use this interface for improved type safety
and clarity.
apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx
Outdated
Show resolved
Hide resolved
size-limit report 📦
|
…nt.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: greg <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx (1)
19-19: Address the typing issue from previous review.The
purchaseDataprop is still typed asunknown | undefined, which was flagged in the previous review as too broad and unsafe. This should be addressed with more specific typing.Also applies to: 31-31
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx
[error] 56-56: expected : but instead found sanitizedPurchaseData
Remove sanitizedPurchaseData
(parse)
[error] 58-58: expected , but instead found ;
Remove ;
(parse)
[error] 56-58: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Build Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx
Outdated
Show resolved
Hide resolved
…roved type safety - Updated the purchaseData type in PayPageEmbed component and PaymentLink type to use Record<string, unknown> | undefined for better clarity. - Modified webhook schema to make purchaseData optional, enhancing flexibility in handling webhook data.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/src/@/api/universal-bridge/links.ts(1 hunks)apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx(3 hunks)packages/thirdweb/src/bridge/Webhook.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Build Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/dashboard/src/@/api/universal-bridge/links.ts (1)
18-18: LGTM! Type refinement improves API clarity.The type change from
unknowntoRecord<string, unknown> | undefinedprovides better type safety and makes the API more predictable while maintaining backward compatibility.packages/thirdweb/src/bridge/Webhook.ts (1)
57-57:❓ Verification inconclusive
Breaking change: purchaseData is now optional.
The
purchaseDatafield has been changed from required to optional. While this aligns with the type refinements in other components, it's a breaking change for webhook consumers that expect this field to always be present.Verify that webhook consumers handle the optional
purchaseDataappropriately:
🏁 Script executed:
#!/bin/bash # Description: Search for webhook purchaseData usage to ensure optional handling # Expected: Find code that accesses purchaseData to verify it handles undefined values # Search for purchaseData usage in webhook processing rg -A 5 -B 5 "purchaseData" --type ts --type js # Look for direct property access that might not handle undefined rg "\.purchaseData\." --type ts --type jsLength of output: 69757
Verify external webhook consumers handle optional
purchaseDataThe Webhook schema in
packages/thirdweb/src/bridge/Webhook.ts(around line 57) has been updated to makepurchaseDataoptional:- purchaseData: z.record(z.string(), z.unknown()), + purchaseData: z.optional(z.record(z.string(), z.unknown())),Internal code search shows no direct property access (e.g.
purchaseData.foo) in this repo—everything passespurchaseDatathrough as an opaque object. However, this is a breaking change for external webhook integrations:data.purchaseDatamay now beundefinedinstead of an empty object.Please:
- Update the CHANGELOG/release notes to call out this breaking change.
- Bump the major version per semver to signal downstream impact.
- Ensure all external consumers of
parse(...)handledata.purchaseData === undefined(e.g. usedata.purchaseData || {}).- (Optional) Add a Webhook test case for a payload missing
purchaseDatato guard against regressions.
gregfromstl
added
the
merge-queue
Merge activity
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
packages/thirdweb/src/bridge/Webhook.test.ts (3)
314-332: 🛠️ Refactor suggestionAdd validation tests for the new transaction object structure.
The existing test only validates that
transactionsis an array, but doesn't test the internal structure of transaction objects (chainId and transactionHash validation).Add this test after the existing "invalid transactions array" test:
+ it("should throw error for invalid transaction object structure", async () => { + const invalidPayload = { + version: 2, + data: { + ...validPayload.data, + transactions: [ + { + chainId: "invalid", // should be number + transactionHash: "0x1234567890123456789012345678901234567890123456789012345678901234", + }, + ], + }, + }; + const payloadString = JSON.stringify(invalidPayload); + const signature = await generateSignature(testTimestamp, payloadString); + const headers = { + "x-payload-signature": signature, + "x-timestamp": testTimestamp, + }; + + await expect( + parse(payloadString, headers, secret), + ).rejects.toHaveProperty("name", "$ZodError"); + }); + + it("should throw error for invalid transaction hash format", async () => { + const invalidPayload = { + version: 2, + data: { + ...validPayload.data, + transactions: [ + { + chainId: 1, + transactionHash: "invalid-hash", // should be valid hex + }, + ], + }, + }; + const payloadString = JSON.stringify(invalidPayload); + const signature = await generateSignature(testTimestamp, payloadString); + const headers = { + "x-payload-signature": signature, + "x-timestamp": testTimestamp, + }; + + await expect( + parse(payloadString, headers, secret), + ).rejects.toHaveProperty("name", "$ZodError"); + });
354-372: 🛠️ Refactor suggestionAdd test case for undefined purchaseData.
The current test only checks for
nullvalues, but should also test theundefinedcase sincepurchaseDatais now optional.Add this test case to properly validate the optional nature of purchaseData:
+ it("should accept undefined purchaseData", async () => { + const validPayloadWithoutPurchaseData = { + version: 2, + data: { + ...validPayload.data, + }, + }; + delete validPayloadWithoutPurchaseData.data.purchaseData; + + const payloadString = JSON.stringify(validPayloadWithoutPurchaseData); + const signature = await generateSignature(testTimestamp, payloadString); + const headers = { + "x-payload-signature": signature, + "x-timestamp": testTimestamp, + }; + + const result = await parse(payloadString, headers, secret); + expect(result).toEqual(validPayloadWithoutPurchaseData); + });
66-66: 💡 Verification agent🧩 Analysis chain
Verify purchaseData validation with the new optional type.
The test uses an empty object for
purchaseData, but should also test the undefined case since the field is now optional.Run this script to verify the webhook schema supports both object and undefined values for purchaseData:
🏁 Script executed:
#!/bin/bash # Description: Check the webhook schema definition for purchaseData field # Expected: purchaseData should be defined as optional with Record<string, unknown> type rg -A 10 -B 5 "purchaseData.*:" packages/thirdweb/src/bridge/Webhook.tsLength of output: 400
Add test case for undefined
purchaseDataWe’ve confirmed in
packages/thirdweb/src/bridge/Webhook.tsthat the schema declarespurchaseData: z.optional(z.record(z.string(), z.unknown())),so payloads must accept both an object and
undefined. The existing test only covers{}. Please updatepackages/thirdweb/src/bridge/Webhook.test.tsto include a case wherepurchaseDatais omitted or explicitlyundefined:• File:
packages/thirdweb/src/bridge/Webhook.test.ts
• Around line 66 (inside the webhook-schema test suite), add:describe("webhook schema", () => { it("validates payload with empty purchaseData", () => { // existing test… }); + it("validates payload when purchaseData is undefined", () => { + const payload = { + // …all required fields except purchaseData + } as any; + expect(() => webhookSchema.parse(payload)).not.toThrow(); + }); });This ensures both the empty‐object and undefined scenarios are explicitly tested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/src/app/pay/page.tsx(1 hunks)packages/thirdweb/src/bridge/Webhook.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/src/app/pay/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Lint Packages
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
| transactions: [ | ||
| "0x1234567890123456789012345678901234567890", | ||
| "0x1234567890123456789012345678901234567890", | ||
| { | ||
| chainId: 1, | ||
| transactionHash: "0x1234567890123456789012345678901234567890", | ||
| }, | ||
| { | ||
| chainId: 1, | ||
| transactionHash: "0x1234567890123456789012345678901234567890", | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Fix transaction hash format and improve test data diversity.
The transaction hashes in the test data appear to be using address format (42 characters) instead of transaction hash format (66 characters). Additionally, both transaction objects use identical values, which reduces test effectiveness.
Apply this diff to fix the transaction hash format and add diversity:
transactions: [
{
chainId: 1,
- transactionHash: "0x1234567890123456789012345678901234567890",
+ transactionHash: "0x1234567890123456789012345678901234567890123456789012345678901234",
},
{
- chainId: 1,
- transactionHash: "0x1234567890123456789012345678901234567890",
+ chainId: 137,
+ transactionHash: "0xabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef",
},
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transactions: [ | |
| "0x1234567890123456789012345678901234567890", | |
| "0x1234567890123456789012345678901234567890", | |
| { | |
| chainId: 1, | |
| transactionHash: "0x1234567890123456789012345678901234567890", | |
| }, | |
| { | |
| chainId: 1, | |
| transactionHash: "0x1234567890123456789012345678901234567890", | |
| }, | |
| ], | |
| transactions: [ | |
| { | |
| chainId: 1, | |
| transactionHash: "0x1234567890123456789012345678901234567890123456789012345678901234", | |
| }, | |
| { | |
| chainId: 137, | |
| transactionHash: "0xabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef", | |
| }, | |
| ], |
🤖 Prompt for AI Agents
In packages/thirdweb/src/bridge/Webhook.test.ts around lines 54 to 63, the
transactionHash values are incorrectly formatted as 42-character addresses
instead of 66-character transaction hashes, and both transactions have identical
data. Update each transactionHash to a valid 66-character hash string and ensure
the two transactions have distinct values to improve test coverage and accuracy.
PR-Codex overview
This PR focuses on updating the handling of
purchaseDataacross various components and types in the application, ensuring it is correctly defined and utilized.Detailed summary
purchaseDatato beundefinedinapps/dashboard/src/app/pay/page.tsx.paymentLink.purchaseDatainapps/dashboard/src/app/pay/[id]/page.tsx.purchaseDatatype toRecord<string, unknown> | undefinedinapps/dashboard/src/@/api/universal-bridge/links.ts.purchaseDatato be optional inpackages/thirdweb/src/bridge/Webhook.ts.transactionsformat inpackages/thirdweb/src/bridge/Webhook.test.tsto includechainId.purchaseDataas a parameter inapps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx.Summary by CodeRabbit