[APP] PostNL - new action components #18264
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds a PostNL app implementation with propDefinitions and HTTP helpers, two actions to fetch shipment status by barcode or reference, and updates package metadata and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Action as Get Status By Barcode
participant App as PostNL App
participant API as PostNL API
User->>Action: invoke with `barcode` and params
Action->>App: getStatusByBarcode({ $, barcode, params })
App->>App: getUrl() / getHeaders() -> makeRequest()
App->>API: GET /shipment/v2/status/barcode/{barcode}
API-->>App: JSON response
App-->>Action: return response
Action-->>User: result (+ $summary)
sequenceDiagram
autonumber
actor User
participant Action as Get Status By Reference
participant App as PostNL App
participant API as PostNL API
User->>Action: invoke with reference params
Action->>App: getStatusByReference({ $, params })
App->>App: getUrl() / getHeaders() -> makeRequest()
App->>API: GET /shipment/v2/status/reference?query
API-->>App: JSON response
App-->>Action: return response
Action-->>User: result (+ $summary)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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: 3
🧹 Nitpick comments (8)
components/postnl/package.json (1)
3-17: Fix ESLint failure: add trailing newline and re-run checksCI is failing with “Newline required at end of file but not found” on this file. Add a single newline at EOF and re-run the PR checks.
} -} +} + ← ensure there’s a newline at EOFcomponents/postnl/postnl.app.mjs (5)
59-62: Safer URL joinIf
this.$auth.api_urlends with “/”, simple concatenation can produce “//shipment…”. Normalize the base before joining.- getUrl(path) { - const { api_url: baseUrl } = this.$auth; - return `${baseUrl}${path}`; - }, + getUrl(path) { + const { api_url: baseUrl } = this.$auth; + const normalized = baseUrl?.endsWith("/") + ? baseUrl.slice(0, -1) + : baseUrl; + return `${normalized}${path.startsWith("/") ? "" : "/"}${path}`; + },
63-69: Headers: add Accept and avoid forcing Content-Type on GETAdd Accept for clarity. Consider only setting Content-Type when sending a body.
- getHeaders(headers) { - return { - "Content-Type": "application/json", - "apikey": this.$auth.api_key, - ...headers, - }; - }, + getHeaders(headers) { + const base = { + Accept: "application/json", + apikey: this.$auth.api_key, + }; + return { + ...base, + ...headers, + }; + },
70-78: Add a sensible request timeoutPrevent long-hanging requests with a default timeout (overrideable via config).
- makeRequest({ - $ = this, path, headers, ...config - }) { - return axios($, { - url: this.getUrl(path), - headers: this.getHeaders(headers), - ...config, - }); - }, + makeRequest({ $ = this, path, headers, timeout = 10000, ...config }) { + return axios($, { + url: this.getUrl(path), + headers: this.getHeaders(headers), + timeout, + ...config, + }); + },
12-21: customerCode prop UX: default from auth instead of single-choice optionsReturning only
[this.$auth.customer_code]inoptions()yields a dropdown with one value. Prefer defaulting from auth and letting users override if needed.customerCode: { type: "string", label: "Customer Code", description: "The customer code for reference tracking", - async options() { - return [ - this.$auth.customer_code, - ]; - }, + default: ({ app }) => app.$auth?.customer_code, + optional: true, },
94-94: Add trailing newline to satisfy ESLintThis file is also flagged by CI for missing EOF newline. Add one line break at the very end.
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs (1)
63-66: EOF newlineAdd a trailing newline to satisfy ESLint.
components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs (1)
71-74: EOF newlineAdd a trailing newline at EOF to fix the CI ESLint error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs(1 hunks)components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs(1 hunks)components/postnl/package.json(2 hunks)components/postnl/postnl.app.mjs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-12T19:23:09.039Z
Learnt from: jcortes
PR: PipedreamHQ/pipedream#14935
File: components/sailpoint/package.json:15-18
Timestamp: 2024-12-12T19:23:09.039Z
Learning: When developing Pipedream components, do not add built-in Node.js modules like `fs` to `package.json` dependencies, as they are native modules provided by the Node.js runtime.
Applied to files:
components/postnl/package.json
🧬 Code graph analysis (2)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs (1)
components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs (1)
response(59-69)
components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs (1)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs (1)
response(52-61)
🪛 GitHub Actions: Pull Request Checks
components/postnl/package.json
[error] 18-18: Command failed: pnpm exec eslint components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs components/postnl/package.json components/postnl/postnl.app.mjs pnpm-lock.yaml. ESLint: Newline required at end of file but not found. (eol-last)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (3)
components/postnl/package.json (1)
15-17: Dependency looks correct; no built-ins addedAdding @pipedream/platform is appropriate, and you avoided adding Node built-ins like fs (per past guidance). LGTM.
components/postnl/postnl.app.mjs (1)
39-49: Verify supported language codesCurrent options include NL, EN, CN, DE, FR. Please confirm these against the latest ShippingStatus API docs before release.
If confirmed, consider exposing a short description per language for clarity. General PostNL guidance indicates language parameters and Accept headers, but specific sets can change. (developer.postnl.nl)
components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs (1)
59-69: Looks good; params align with API expectationsPassing customerCode, customerNumber, and reference as query params matches the reference endpoint usage. LGTM.
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs
Show resolved
Hide resolved
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs
Show resolved
Hide resolved
jcortes
force-pushed
the
postnl-new-components
branch
from
a74c0f7 to
6a24b71
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/postnl/postnl.app.mjs (1)
79-86: Fix barcode path construction: accept full barcode, drop customerCode, encode, and validateCurrent path prepends
3S${customerCode}which will break when users pass a full barcode (e.g., "3S..."). The API expects the full barcode as a single path segment. Also add a basic presence check and URL-encode the segment.- getStatusByBarcode({ - barcode, customerCode, ...args - } = {}) { - return this.makeRequest({ - path: `/shipment/v2/status/barcode/3S${customerCode}${barcode}`, - ...args, - }); - }, + getStatusByBarcode({ barcode, ...args } = {}) { + if (!barcode?.trim()) throw new Error("barcode is required"); + const bc = encodeURIComponent(barcode.trim()); + return this.makeRequest({ + path: `/shipment/v2/status/barcode/${bc}`, + ...args, + }); + },If you want, I can push the matching updates to the action props/usages.
🧹 Nitpick comments (8)
components/postnl/postnl.app.mjs (8)
59-62: Harden URL joining to avoid double/missing slashesUse the URL constructor to robustly join base and path.
- getUrl(path) { - const { api_url: baseUrl } = this.$auth; - return `${baseUrl}${path}`; - }, + getUrl(path) { + const { api_url: baseUrl } = this.$auth; + return new URL(path, `${baseUrl}`).toString(); + }, ``` <!-- review_comment_end --> --- `63-69`: **Set Accept header; only send Content-Type when there's a body** Sending Content-Type on GETs is unnecessary; prefer Accept. We'll set Content-Type conditionally in makeRequest. ```diff - getHeaders(headers) { - return { - "Content-Type": "application/json", - "apikey": this.$auth.api_key, - ...headers, - }; - }, + getHeaders(headers) { + return { + "Accept": "application/json", + "apikey": this.$auth.api_key, + ...headers, + }; + }, ``` <!-- review_comment_end --> --- `70-78`: **Add sane defaults (timeout, method) and set Content-Type conditionally** Prevents hanging requests and avoids incorrect headers on GET. ```diff - makeRequest({ - $ = this, path, headers, ...config - }) { - return axios($, { - url: this.getUrl(path), - headers: this.getHeaders(headers), - ...config, - }); - }, + makeRequest({ $ = this, path, headers, ...config }) { + const cfg = { + method: config.method ?? "GET", + timeout: config.timeout ?? 10000, + url: this.getUrl(path), + headers: this.getHeaders(headers), + ...config, + }; + if (cfg.data && !cfg.headers["Content-Type"]) { + cfg.headers["Content-Type"] = "application/json"; + } + return axios($, cfg); + }, ``` <!-- review_comment_end --> --- `7-11`: **Clarify expected input for barcode prop** State that a full PostNL barcode is expected and give an example to reduce misuse. ```diff - description: "The barcode of the shipment to track", + description: "Full PostNL barcode of the shipment to track (e.g., 3SABCD123456789)", + placeholder: "3SXXXXXXXXXXXXXX", ``` <!-- review_comment_end --> --- `12-21`: **Make customerCode optional and clarify it’s for reference tracking, not barcode** This avoids user confusion when using the barcode endpoint. ```diff - customerCode: { + customerCode: { type: "string", label: "Customer Code", - description: "The customer code for reference tracking", + description: "Customer code (used for reference tracking; not required for barcode lookups)", + optional: true, async options() { return [ this.$auth.customer_code, ]; }, }, ``` <!-- review_comment_end --> --- `38-50`: **Provide a default language** Docs indicate Dutch is the default; set it explicitly for clarity. ```diff options: [ "NL", "EN", "CN", "DE", "FR", ], optional: true, + default: "NL", ``` <!-- review_comment_end --> --- `51-56`: **Confirm allowed range for maxDays and enforce it if known** If the API has limits, add min/max to prevent 400s. I can update this prop with min/max once you confirm the valid range from the PostNL docs. <!-- review_comment_end --> --- `87-92`: **Centralize query param mapping and validation for reference lookups** Map `referenceId` to the API param `reference`, validate required params, and pass query params from here to reduce duplication in actions. ```diff - getStatusByReference(args = {}) { - return this.makeRequest({ - path: "/shipment/v2/status/reference", - ...args, - }); - }, + getStatusByReference({ + reference, + referenceId, + customerCode, + customerNumber, + detail, + language, + maxDays, + ...args + } = {}) { + const ref = reference ?? referenceId; + if (!ref || !customerCode || !customerNumber) { + throw new Error("reference, customerCode, and customerNumber are required"); + } + return this.makeRequest({ + path: "/shipment/v2/status/reference", + params: { reference: ref, customerCode, customerNumber, detail, language, maxDays }, + ...args, + }); + }, ``` <!-- review_comment_end --> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro **💡 Knowledge Base configuration:** - MCP integration is disabled by default for public repositories - Jira integration is disabled by default for public repositories - Linear integration is disabled by default for public repositories You can enable these sources in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a74c0f7cbbd31bc6af4213f7de6326654a18d1df and 6a24b718096b22f79b08d39f1e9a1d5fcf8fe39f. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (4)</summary> * `components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs` (1 hunks) * `components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs` (1 hunks) * `components/postnl/package.json` (2 hunks) * `components/postnl/postnl.app.mjs` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs * components/postnl/package.json * components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)</summary> * GitHub Check: Lint Code Base * GitHub Check: Publish TypeScript components * GitHub Check: Verify TypeScript components </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>components/postnl/postnl.app.mjs (1)</summary><blockquote> `6-57`: **No changes needed: actions already pass detail, language, and maxDays via params** Both `get-status-by-reference` and `get-status-by-barcode` include `detail`, `language`, and `maxDays` inside the `params` object when calling the respective app methods. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
jcortes
force-pushed
the
postnl-new-components
branch
from
6a24b71 to
e48b99c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs (1)
11-16: Good removal of customerCode; action now accepts the full barcode only.This aligns with the endpoint contract and avoids conflicting/duplicated inputs.
🧹 Nitpick comments (1)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs (1)
55-55: PII-aware summary: mask the barcode in logs/summary.Helps debugging while avoiding leaking full identifiers.
- $.export("$summary", "Successfully retrieved status by barcode"); + $.export("$summary", `Retrieved status for barcode …${String(barcode).slice(-4)}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs(1 hunks)components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs(1 hunks)components/postnl/package.json(2 hunks)components/postnl/postnl.app.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs
- components/postnl/package.json
- components/postnl/postnl.app.mjs
🧰 Additional context used
🧬 Code graph analysis (1)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs (1)
components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs (1)
response(59-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Lint Code Base
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (3)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs (3)
1-1: Import path looks correct.The relative path resolves from the action directory back to the app file. No issues.
8-8: Confirm intended action versioning.Package/app bumped to 0.1.0 elsewhere; if you want action versions aligned, bump this to 0.1.0. If you version actions independently, keep as-is.
- version: "0.0.1", + version: "0.1.0",
45-53: Verify query params and response shape
- Filter out undefined values from
params(e.g. viaObject.fromEntries) to prevent sendingundefinedin the query string.- Confirm whether
app.getStatusByBarcodereturns the parsed payload or the full HTTP response; if it’s the latter, returnresponse.datafor consistent output.
jcortes
force-pushed
the
postnl-new-components
branch
from
e48b99c to
b154e8c
Compare
jcortes
force-pushed
the
postnl-new-components
branch
2 times, most recently
from
fc08255 to
2a63931
Compare
|
/approve |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs (1)
9-35: Resolved: customerCode removed from props for barcode action.Matches the app method and API shape. Thanks for fixing.
🧹 Nitpick comments (13)
components/postnl/package.json (1)
1-18: Add repository and license metadata.Minor packaging polish for npm consumers.
Apply:
{ "name": "@pipedream/postnl", "version": "0.1.0", "description": "Pipedream PostNL Components", "main": "postnl.app.mjs", + "license": "MIT", + "repository": { + "type": "git", + "url": "https://github.com/PipedreamHQ/pipedream.git", + "directory": "components/postnl" + }, "keywords": [ "pipedream", "postnl" ],components/postnl/postnl.app.mjs (5)
16-21: Guard options() when customer_code is absent.Avoid returning [undefined] in the dropdown if auth is misconfigured.
- async options() { - return [ - this.$auth.customer_code, - ]; - }, + async options() { + const code = this.$auth?.customer_code; + return code ? [code] : []; + },
59-62: Normalize base URL + path joining.Prevents accidental double slashes or missing slash.
- getUrl(path) { - const { api_url: baseUrl } = this.$auth; - return `${baseUrl}${path}`; - }, + getUrl(path) { + const { api_url: baseUrl } = this.$auth; + const base = (baseUrl ?? "").replace(/\/+$/, ""); + const p = String(path ?? ""); + return `${base}${p.startsWith("/") ? "" : "/"}${p}`; + },
70-78: Set sane defaults and guardrails on requests.Add method/timeout and early validation to avoid silent hangs.
- makeRequest({ - $ = this, path, headers, ...config - }) { - return axios($, { - url: this.getUrl(path), - headers: this.getHeaders(headers), - ...config, - }); - }, + makeRequest({ + $ = this, path, headers, method = "GET", timeout = 30000, ...config + }) { + if (!path) throw new Error("path is required"); + if (!this?.$auth?.api_url) throw new Error("Missing API base URL ($auth.api_url)"); + return axios($, { + method, + url: this.getUrl(path), + headers: this.getHeaders(headers), + timeout, + ...config, + }); + },
87-92: Validate required params for reference lookup.Fail fast if customerCode, customerNumber, or reference is missing.
- getStatusByReference(args = {}) { - return this.makeRequest({ - path: "/shipment/v2/status/reference", - ...args, - }); - }, + getStatusByReference({ params, ...args } = {}) { + const { customerCode, customerNumber, reference } = params ?? {}; + if (!customerCode || !customerNumber || !reference) { + throw new Error("customerCode, customerNumber, and reference are required"); + } + return this.makeRequest({ + path: "/shipment/v2/status/reference", + params, + ...args, + }); + },
63-69: Confirm API key header name and add Accept header
- Use lowercase
"apikey"per PostNL docs.- Include
"Accept": "application/json"for clarity.getHeaders(headers) { return { + "Accept": "application/json", "Content-Type": "application/json", "apikey": this.$auth.api_key, ...headers, }; },components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs (2)
36-57: Mask barcode in summary to reduce noise/PII.Keep the summary useful without dumping full identifiers.
- $.export("$summary", "Successfully retrieved status by barcode"); + const masked = `${barcode}`.replace(/^(.{3}).*(.{4})$/, "$1…$2"); + $.export("$summary", `Retrieved status for barcode ${masked}`);
8-8: Optional: align action version with package version.Not required, but keeps artifacts in sync.
- version: "0.0.1", + version: "0.1.0",components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs (5)
48-58: Avoid shadowing and filter undefined params before sending to the API.Alias the app instance in run for clarity, and prevent leaking
undefined/null/""as query params.- async run({ $ }) { - const { - app, - customerCode, - customerNumber, - reference, - detail, - language, - maxDays, - } = this; + async run({ $ }) { + const { + app: postnl, + customerCode, + customerNumber, + reference, + detail, + language, + maxDays, + } = this; - const response = await app.getStatusByReference({ - $, - params: { - customerCode, - customerNumber, - reference, - detail, - language, - maxDays, - }, - }); + const params = Object.fromEntries( + Object.entries({ + customerCode, + customerNumber, + reference, + detail, + language, + maxDays, + }).filter(([, v]) => v !== undefined && v !== null && v !== "") + ); + + const response = await postnl.getStatusByReference({ $, params });Also applies to: 59-69
23-28: Align prop naming with propDefinition to reduce cognitive load.Prop key is
referencebut the propDefinition is"referenceId". Consider renaming the prop toreferenceIdand mapping it toreferencein the request, or update the propDefinition name if the API expectsreference. Keeps UX consistent across app + actions.Example:
- reference: { + referenceId: { propDefinition: [ app, "referenceId", ], },And in run:
- reference, + referenceId,- const params = { ..., reference, ... } + const params = { ..., reference: referenceId, ... }
71-72: More informative summary.Include the reference to aid users scanning logs.
- $.export("$summary", "Successfully retrieved status by reference"); + $.export("$summary", `Retrieved status for reference ${String(reference).slice(0, 32)}${String(reference).length > 32 ? "…" : ""}`);
59-69: Confirm return shape consistency.If
app.getStatusByReferencereturns an Axios response, consider returningresponse.data(and do the same across related actions) to keep outputs stable and JSON-friendly.- return response; + return response.data ?? response;
35-46: Add default for language and enforce range on maxDays in postnl.app.mjs
- In components/postnl/postnl.app.mjs, language already lists options [“NL”, “DE”, “FR”]; add default: "NL" to align with API’s default citestderr:38-50
- For maxDays, include min and max (e.g. min: 0, max: 365) in its propDefinition so the UI blocks out-of-range values citestderr:51-56
📜 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 (4)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs(1 hunks)components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs(1 hunks)components/postnl/package.json(2 hunks)components/postnl/postnl.app.mjs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (4)
components/postnl/package.json (1)
3-3: LGTM on version bump and dependency.Version to 0.1.0 and adding @pipedream/platform ^3.1.0 look good.
Also applies to: 15-17
components/postnl/postnl.app.mjs (2)
27-31: Confirm naming: referenceId vs API param “reference”.The API expects query param reference. Ensure actions map propDefinition "referenceId" to params.reference (or rename to "reference" here).
Would you like me to align names and update the get-status-by-reference action accordingly?
38-50: Language codes verified: options match the API’s accepted uppercase two-letter values (NL, EN, CN, DE, FR); no changes required.components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs (1)
3-8: Solid action scaffold and metadata.Key, name, description, type, and version look consistent with Pipedream action patterns.
|
/approve |
jcortes
force-pushed
the
postnl-new-components
branch
from
2a63931 to
de206e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/postnl/postnl.app.mjs (1)
79-86: Validate and encode barcode; accept full 3S code onlyPrevents empty/whitespace input and malformed URLs.
- getStatusByBarcode({ - barcode, ...args - } = {}) { - return this.makeRequest({ - path: `/shipment/v2/status/barcode/${barcode}`, - ...args, - }); - }, + getStatusByBarcode({ barcode, ...args } = {}) { + if (!barcode || !String(barcode).trim()) { + throw new Error("barcode is required"); + } + const id = encodeURIComponent(String(barcode).trim()); + return this.makeRequest({ + path: `/shipment/v2/status/barcode/${id}`, + ...args, + }); + },
🧹 Nitpick comments (4)
components/postnl/postnl.app.mjs (4)
59-62: Harden URL joining to avoid double/missing slashesPrevents baseUrl/path concatenation issues.
- getUrl(path) { - const { api_url: baseUrl } = this.$auth; - return `${baseUrl}${path}`; - }, + getUrl(path) { + const base = String(this.$auth?.api_url || "").replace(/\/+$/, ""); + const p = String(path || "").replace(/^\/+/, ""); + return `${base}/${p}`; + },
70-78: Set sane axios defaults (method, timeout) while allowing overrideImproves resiliency to hangs; callers can override.
makeRequest({ $ = this, path, headers, ...config }) { return axios($, { - url: this.getUrl(path), - headers: this.getHeaders(headers), - ...config, + url: this.getUrl(path), + headers: this.getHeaders(headers), + method: "GET", + timeout: 10000, + ...config, // allow overrides }); },
63-69: Validate API key and add Accept header
Fail fast if$auth.api_keyis missing; include anAcceptheader for clarity. PostNL expects the HTTP header nameapikey.getHeaders(headers) { - return { - "Content-Type": "application/json", - "apikey": this.$auth.api_key, - ...headers, - }; + const apiKey = this.$auth?.api_key; + if (!apiKey) throw new Error("Missing PostNL API key ($auth.api_key)."); + return { + "Accept": "application/json", + "Content-Type": "application/json", + "apikey": apiKey, + ...headers, + }; },
6-57: Tighten prop definitions and constrain inputs
- In customerCode.options(), guard against unbound auth:
async options() {return [ this.$auth.customer_code ];
const v = this.$auth?.customer_code;return v ? [v] : [];
- On maxDays, add sensible bounds:
optional: true,
- min: 1,
- max: 90, // default; PostNL doesn’t publish a public cap—confirm with support
Language codes are correct (“CN” is valid for Chinese). </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2a639317fbb80a8d13d46a713b943a80b21fd831 and de206e64145e0e260bc1f6ee221fdc2023c52146. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (4)</summary> * `components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs` (1 hunks) * `components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs` (1 hunks) * `components/postnl/package.json` (2 hunks) * `components/postnl/postnl.app.mjs` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * components/postnl/package.json * components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs * components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>components/postnl/postnl.app.mjs (3)</summary><blockquote> `1-1`: **Import looks good** Correct usage of axios from @pipedream/platform. --- `87-91`: **Only `params.reference` is required—remove validation for `customerCode` and `customerNumber`.** Replace the previous diff with: ```diff components/postnl/postnl.app.mjs:87 getStatusByReference(args = {}) { - const params = args.params || {}; - if (!params.customerCode || !params.customerNumber || !params.reference) { - throw new Error("params.customerCode, params.customerNumber, and params.reference are required"); - } + const params = args.params || {}; + if (!params.reference) { + throw new Error("params.reference is required"); + } return this.makeRequest({ path: "/shipment/v2/status/reference", ...args, });Likely an incorrect or invalid review comment.
79-91: Sanity-check downstream action usage
- Pass
$when invoking the app methods, e.g.
await postnl.getStatusByBarcode({ barcode, $ })
await postnl.getStatusByReference({ params: { reference }, $ })- Use
params.reference(not thereferenceIdprop) for the reference lookup.
jcortes
force-pushed
the
postnl-new-components
branch
from
de206e6 to
6fd6c48
Compare
|
/approve |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/postnl/postnl.app.mjs (1)
79-86: Validate and encode the barcode path segmentGuard against empty values and ensure safe URLs.
- getStatusByBarcode({ - barcode, ...args - } = {}) { - return this.makeRequest({ - path: `/shipment/v2/status/barcode/${barcode}`, - ...args, - }); - }, + getStatusByBarcode({ barcode, ...args } = {}) { + if (!barcode || !String(barcode).trim()) { + throw new Error("barcode is required"); + } + const id = encodeURIComponent(String(barcode).trim()); + return this.makeRequest({ + path: `/shipment/v2/status/barcode/${id}`, + ...args, + }); + },
🧹 Nitpick comments (5)
components/postnl/postnl.app.mjs (5)
59-62: Harden URL constructionAvoid double slashes and string concat edge cases.
- getUrl(path) { - const { api_url: baseUrl } = this.$auth; - return `${baseUrl}${path}`; - }, + getUrl(path) { + const { api_url: baseUrl } = this.$auth; + return new URL(String(path || ""), baseUrl).toString(); + },
12-21: Prefer defaulting customerCode over a single-option dropdownAuto-fill from auth; keep it optional since barcode flow doesn’t use it.
- customerCode: { - type: "string", - label: "Customer Code", - description: "The customer code for reference tracking", - async options() { - return [ - this.$auth.customer_code, - ]; - }, - }, + customerCode: { + type: "string", + label: "Customer Code", + description: "The customer code for reference tracking", + default: ({ auth }) => auth?.customer_code, + optional: true, + },Please confirm Pipedream supports
default: ({ auth }) => ...in apppropDefinitions.
27-31: Align naming with API:referenceinstead ofreferenceIdMatches the Send & Track parameter and reduces mapping friction.
- referenceId: { + reference: { type: "string", - label: "Reference ID", - description: "The reference number to track", + label: "Reference", + description: "The reference number to track", },If you rename, verify call sites in actions are updated accordingly.
87-91: Validate required params for reference lookups (or assert callers do)Ensure
customerCode,customerNumber, andreferenceare present to avoid opaque 400s.Would you prefer lightweight guards here, or are the actions already validating? If unsure, I can scan the actions and add checks.
70-78: Consider basic retry/backoff for transient 429/5xxShipping status endpoints can rate-limit; a small capped exponential backoff improves resilience.
If desired, I can propose a minimal retry wrapper around
makeRequestwith jitter.
📜 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 (4)
components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs(1 hunks)components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs(1 hunks)components/postnl/package.json(2 hunks)components/postnl/postnl.app.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/postnl/actions/get-status-by-barcode/get-status-by-barcode.mjs
- components/postnl/actions/get-status-by-reference/get-status-by-reference.mjs
- components/postnl/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Publish TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
components/postnl/postnl.app.mjs (2)
51-56: Please share the PostNL developer documentation (or a screenshot) for the/shipment/v2/statusendpoint showing themaxDaysparameter’s permitted range so I can verify and update the suggestion accordingly.
38-49: Add default language “NL” to PostNL config
Includedefault: "NL"in thelanguageproperty; the allowed codes (NL,EN,CN,DE,FR) are correct andCNis accepted by the ShippingStatus API.
2 participants
WHY
Resolves #18229
Summary by CodeRabbit
New Features
Chores