Shopify - Bulk Import #15847
Shopify - Bulk Import #15847
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughA new module for Shopify bulk import has been added to enable the upload and processing of JSONL files containing mutation variables. The module defines methods to construct and execute staged uploads and bulk import mutations, handling file upload via axios and error propagation from GraphQL mutations. Additionally, the package configuration has been updated to a new version and a dependency has been introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BulkImport
participant Axios
participant ShopifyAPI
User->>BulkImport: call run({ filePath, mutation, clientID? })
BulkImport->>BulkImport: Prepare file path & data
BulkImport->>BulkImport: stagedUploadQuery()
BulkImport->>ShopifyAPI: Request staged upload targets
ShopifyAPI-->>BulkImport: Return upload URL
BulkImport->>Axios: Upload JSONL file
Axios-->>BulkImport: Confirm file upload
BulkImport->>ShopifyAPI: bulkImportMutation() call
ShopifyAPI-->>BulkImport: Return mutation result or errors
BulkImport->>User: Return summary or error message
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
components/shopify/actions/bulk-import/bulk-import.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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 (2)
components/shopify/actions/bulk-import/bulk-import.mjs (2)
116-122: Consider adding retry logic and robust error handling for the file upload step.
Currently, if the request fails, there's limited recovery. Incorporatingasync-retryor explicit error handling can improve reliability:await axios($, { url, method: "POST", headers: form.getHeaders(), data: form, debug: true, + // Example: to incorporate basic retry logic + // transformResponse: [ (data, headers) => { + // if (headers.status >= 400) throw new Error("Upload error"); + // return data; + // }], });
132-134: Consider returning or aggregating all user errors instead of throwing only the first.
Currently, only the first message is returned in the event of multiple user errors. Aggregating all errors can provide more detailed feedback to users.if (response.bulkOperationRunMutation.userErrors.length > 0) { - throw new Error(response.bulkOperationRunMutation.userErrors[0].message); + const messages = response.bulkOperationRunMutation.userErrors.map(e => e.message).join(" | "); + throw new Error(`Bulk import failed with errors: ${messages}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
components/shopify/actions/bulk-import/bulk-import.mjs(1 hunks)components/shopify/package.json(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint Code Base
components/shopify/actions/bulk-import/bulk-import.mjs
[warning] 14-14:
Component prop alert must have a label. See https://pipedream.com/docs/components/guidelines/#props
[warning] 14-14:
Component prop alert must have a description. See https://pipedream.com/docs/components/guidelines/#props
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (2)
components/shopify/package.json (2)
3-3: Version bump aligns with semantic versioning.
No issues identified with incrementing the version to 0.6.9.
16-16: Dependency addition looks appropriate.
Including"form-data"is suitable for file upload functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
components/shopify/actions/bulk-import/bulk-import.mjs (2)
116-122: Add error handling for file upload failures.The axios request for file upload lacks specific error handling. Consider adding a try/catch block to provide clearer error messages for upload failures.
- await axios($, { - url, - method: "POST", - headers: form.getHeaders(), - data: form, - }); + try { + await axios($, { + url, + method: "POST", + headers: form.getHeaders(), + data: form, + }); + } catch (error) { + throw new Error(`Failed to upload file to Shopify: ${error.message}`); + }
124-130: Improve error handling for the GraphQL mutation.The code currently handles user errors but could provide better context for other types of errors that might occur.
// perform bulk import -const response = await this.shopify._makeGraphQlRequest(this.bulkImportMutation(), { - mutation: this.mutation, - stagedUploadPath, - clientIdentifier: this.clientIdentifier, -}); +try { + const response = await this.shopify._makeGraphQlRequest(this.bulkImportMutation(), { + mutation: this.mutation, + stagedUploadPath, + clientIdentifier: this.clientIdentifier, + }); +} catch (error) { + throw new Error(`Failed to execute bulk import mutation: ${error.message}`); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/shopify/actions/bulk-import/bulk-import.mjs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
components/shopify/actions/bulk-import/bulk-import.mjs (2)
14-18: Add a label and description to the alert prop to comply with the component guidelines.According to component prop guidelines, alert props must include label and description fields.
alert: { type: "alert", alertType: "info", content: "Use the Shopify trigger \"New Event Emitted (Instant)\" with event type `bulk_operations/finish` to receive notifications when bulk imports are completed", + label: "Bulk Import Alert", + description: "Information about tracking the completion of bulk import operations", },
6-10: LGTM: Well-structured component with clear purpose.The overall structure of the component follows good practices with clear separation of concerns between methods, props definition, and the main execution flow.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/shopify/actions/bulk-import/bulk-import.mjs (2)
14-18:⚠️ Potential issueAdd a label and description to the alert prop to comply with the component guidelines.
According to the static analysis checks, component props of type
alertmust have alabelanddescription.alert: { type: "alert", alertType: "info", content: "Use the Shopify trigger \"New Event Emitted (Instant)\" with event type `bulk_operations/finish` to receive notifications when bulk imports are completed", + label: "Bulk Import Alert", + description: "Information about tracking the completion of bulk import operations", },🧰 Tools
🪛 GitHub Check: Lint Code Base
[warning] 14-14:
Component prop alert must have a label. See https://pipedream.com/docs/components/guidelines/#props
[warning] 14-14:
Component prop alert must have a description. See https://pipedream.com/docs/components/guidelines/#props
78-83: 🛠️ Refactor suggestionAdd file existence validation before proceeding.
The code currently assumes the file exists but doesn't validate this assumption. This could lead to cryptic errors if the file is not found.
async run({ $ }) { const filePath = this.filePath.includes("/tmp") ? this.filePath : `/tmp/${this.filePath}`; const filename = filePath.split("/").pop(); + + // Validate file exists + try { + await fs.promises.access(filePath, fs.constants.F_OK); + } catch (error) { + throw new Error(`File not found at path: ${filePath}`); + } // create staged upload path
🧹 Nitpick comments (2)
components/shopify/actions/bulk-import/bulk-import.mjs (2)
78-137: Wrap the entire run method in a try-catch block for better error handling.The run method should have comprehensive error handling to catch any unexpected errors that might occur during the process.
Consider wrapping the entire method in a try-catch block and adding more specific error messages:
async run({ $ }) { + try { const filePath = this.filePath.includes("/tmp") ? this.filePath : `/tmp/${this.filePath}`; const filename = filePath.split("/").pop(); // ... rest of the method ... $.export("$summary", "Successfully completed bulk import"); return response; + } catch (error) { + // If this is already a handled error, just rethrow it + if (error.message) { + throw error; + } + // Otherwise provide a more descriptive error + throw new Error(`Bulk import operation failed: ${error}`); + } },
79-82: Consider normalizing path handling.The current path handling logic checks if the path includes "/tmp", which might not be robust enough for all possible inputs.
- const filePath = this.filePath.includes("/tmp") - ? this.filePath - : `/tmp/${this.filePath}`; + // Ensure the path is correctly formatted with or without /tmp prefix + const filePath = this.filePath.startsWith("/tmp") + ? this.filePath + : `/tmp/${this.filePath.replace(/^\/+/, '')}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/shopify/actions/bulk-import/bulk-import.mjs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint Code Base
components/shopify/actions/bulk-import/bulk-import.mjs
[warning] 14-14:
Component prop alert must have a label. See https://pipedream.com/docs/components/guidelines/#props
[warning] 14-14:
Component prop alert must have a description. See https://pipedream.com/docs/components/guidelines/#props
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (1)
components/shopify/actions/bulk-import/bulk-import.mjs (1)
1-138: Overall, this is a well-structured implementation for Shopify bulk imports.The component provides a clean interface for executing bulk mutations in Shopify by uploading JSONL files. The GraphQL queries are well-structured, and the process flow is logical. With the suggested improvements to error handling and validation, this will be a robust and reliable component.
🧰 Tools
🪛 GitHub Check: Lint Code Base
[warning] 14-14:
Component prop alert must have a label. See https://pipedream.com/docs/components/guidelines/#props
[warning] 14-14:
Component prop alert must have a description. See https://pipedream.com/docs/components/guidelines/#props
Resolves #15352
Summary by CodeRabbit
New Features
Chores