[Component] InMobile - Send SMS messages #18760
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds a new InMobile "Send SMS Messages" action, introduces safe JSON/array parsing utilities, refactors the InMobile app to centralize HTTP requests (including a sendSms method), and bumps package version while adding an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as Action: send-sms-messages
participant UTL as Utils
participant APP as InMobile App
participant API as InMobile REST API
U->>A: invoke action with `app` + `messages`
A->>UTL: parseArray(messages)
UTL-->>A: parsed messages[]
A->>APP: sendSms({ messages: parsed messages })
APP->>APP: getAuth() / getUrl("/sms/outgoing")
APP->>API: POST /sms/outgoing (auth + payload)
API-->>APP: response
APP-->>A: response
A-->>U: returns response and summary ("N messages sent")
note right of APP: makeRequest/post centralize HTTP calls
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 4
🧹 Nitpick comments (1)
components/inmobile/common/utils.mjs (1)
58-60: Flatten nested arrays and decrement depth in parseArrayEnsure consistent depth handling and normalize nested arrays (common when strings contain JSON arrays).
Apply this diff:
- if (Array.isArray(input)) { - return input.map((item) => parseArray(item, maxDepth)); - } + if (Array.isArray(input)) { + return input.flatMap((item) => { + const v = parseArray(item, maxDepth - 1); + return Array.isArray(v) ? v : [v]; + }); + }
📜 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/inmobile/actions/send-sms-messages/send-sms-messages.mjs(1 hunks)components/inmobile/common/utils.mjs(1 hunks)components/inmobile/inmobile.app.mjs(1 hunks)components/inmobile/package.json(2 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: Lint Code Base
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
🔇 Additional comments (1)
components/inmobile/package.json (1)
3-3: Version bump and platform dependency look goodNo issues spotted. Please confirm the runtime uses @pipedream/platform >= 3.1.x to avoid runtime mismatches.
Also applies to: 15-17
jcortes
force-pushed
the
inmobile-send-sms-messges-action
branch
from
2e5902a to
b512325
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/inmobile/actions/send-sms-messages/send-sms-messages.mjs (2)
18-55: Critical: Prop type contradicts documentation and example.The
messagesprop is declared asstring[](line 18), but the entire description (lines 20-54) documents message objects with fields liketo,text,from, etc., and the example (lines 39-53) shows an array of objects—not strings.This type mismatch will confuse users and break at runtime when the action attempts to use string elements as objects.
Resolution options:
- Preferred: Change the prop type to accept objects directly so users can pass
[{ to: "...", text: "...", from: "..." }]without manual JSON stringification.- Alternative: Keep
string[]but clarify that each string must be a valid JSON object (not a top-level JSON array), and update the example to show individual stringified objects.Would you like me to generate the updated prop definition for option 1?
63-71: Critical: Missing validation, incorrect count, and nested array risk.The current implementation has multiple critical issues:
No flattening (line 66):
utils.parseArray(messages)can return nested arrays if any element is a JSON array string (e.g.,["[{...}, {...}]"]becomes[[{...}, {...}]]), breaking the API call.No message count validation: The InMobile API requires 1-250 messages. This is not enforced.
No required field validation: The code doesn't check that each message has
to,text, andfromfields, leading to cryptic API errors.Incorrect summary (line 70): Uses original
messages.lengthinstead of the parsed/flattened count, so the summary may report the wrong number.No idempotency safeguard: Without enforcing or auto-assigning
messageId, retries will send duplicate messages.Apply this diff to address all issues:
+ // Normalize inputs: accept string[], object[], or JSON array strings within the array + const normalizedMessages = (Array.isArray(messages) ? messages : [messages]) + .flatMap((m) => { + const v = utils.parseArray(m); + return Array.isArray(v) ? v : [v]; + }); + + if (normalizedMessages.length < 1 || normalizedMessages.length > 250) { + throw new Error("Messages must contain between 1 and 250 items."); + } + normalizedMessages.forEach((msg, i) => { + if (!msg || typeof msg !== "object") { + throw new Error(`Message at index ${i} is not an object.`); + } + const { to, text, from } = msg; + if (!to || !text || !from) { + throw new Error(`Message at index ${i} is missing required field(s): to, text, from.`); + } + }); + const response = await app.sendSms({ $, data: { - messages: utils.parseArray(messages), + messages: normalizedMessages, }, }); - $.export("$summary", `Successfully sent ${messages.length} SMS message(s)`); + $.export("$summary", `Successfully sent ${normalizedMessages.length} SMS message(s)`); return response;Note on idempotency: Consider requiring or auto-assigning
messageIdfor each message to prevent duplicate sends on retries.
📜 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/inmobile/actions/send-sms-messages/send-sms-messages.mjs(1 hunks)components/inmobile/common/utils.mjs(1 hunks)components/inmobile/inmobile.app.mjs(1 hunks)components/inmobile/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/inmobile/package.json
- components/inmobile/inmobile.app.mjs
🧰 Additional context used
🧬 Code graph analysis (1)
components/inmobile/common/utils.mjs (1)
components/google_cloud/sources/bigquery-query-results/bigquery-query-results.mjs (1)
key(48-48)
⏰ 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: pnpm publish
- GitHub Check: Lint Code Base
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (1)
components/inmobile/common/utils.mjs (1)
65-68: LGTM!The default export correctly exposes both utility functions.
jcortes
force-pushed
the
inmobile-send-sms-messges-action
branch
from
b512325 to
a7a1c0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/inmobile/inmobile.app.mjs (1)
16-24: Previous review concerns remain unaddressed: request hardening needed.The issues flagged in the earlier review still apply:
- Defaulting
$ = thisrisks passing incorrect context to axios- Spreading
...argsafterurlandauthallows callers to override authentication or endpoints- Missing timeout protection can cause hanging requests
- No default headers
Apply the diff suggested in the previous review to harden this method:
- makeRequest({ - $ = this, path, ...args - }) { - return axios($, { - url: this.getUrl(path), - auth: this.getAuth(), - ...args, - }); - }, + makeRequest({ + $, path, ...args + }) { + if (!$) throw new Error("Axios context `$` is required"); + const { headers, timeout, ...rest } = args; + return axios($, { + ...rest, + url: this.getUrl(path), + auth: this.getAuth(), + headers: { + Accept: "application/json", + "Content-Type": "application/json", + ...(headers || {}), + }, + timeout: timeout ?? 30000, + }); + },
🧹 Nitpick comments (1)
components/inmobile/inmobile.app.mjs (1)
7-9: Consider adding path validation for robustness.While current usage provides correctly formatted paths (starting with
/), adding validation would prevent potential bugs if the method is called incorrectly in the future.getUrl(path) { + if (!path.startsWith("/")) { + throw new Error(`Path must start with '/': ${path}`); + } return `https://api.inmobile.com/v4${path}`; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/inmobile/actions/send-sms-messages/send-sms-messages.mjs(1 hunks)components/inmobile/common/utils.mjs(1 hunks)components/inmobile/inmobile.app.mjs(1 hunks)components/inmobile/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/inmobile/actions/send-sms-messages/send-sms-messages.mjs
- components/inmobile/common/utils.mjs
⏰ 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: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
components/inmobile/package.json (1)
3-3: LGTM! Version bump and dependency addition are appropriate.The minor version increment correctly reflects the addition of new SMS functionality, and the
@pipedream/platformdependency enables the HTTP utilities used in the app file.Also applies to: 15-16
components/inmobile/inmobile.app.mjs (1)
10-15: LGTM! Auth implementation now follows InMobile API requirements.The username placeholder "x" correctly follows the InMobile API convention for Basic authentication with API keys.
| post(args = {}) { | ||
| return this.makeRequest({ | ||
| method: "POST", | ||
| ...args, | ||
| }); | ||
| }, | ||
| sendSms(args = {}) { | ||
| return this.post({ | ||
| path: "/sms/outgoing", | ||
| ...args, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the SMS endpoint path.
The implementation looks clean. Please confirm that /sms/outgoing is the correct InMobile API v4 endpoint for sending SMS messages.
🌐 Web query:
What is the correct InMobile REST API v4 endpoint path for sending SMS messages?
💡 Result:
Use POST to /v4/sms/outgoing — e.g. https://api.inmobile.com/v4/sms/outgoing?apikey=[your_api_key]. [1][2]
Sources:
[1] inMobile Klaviyo webhook guide (shows https://api.inmobile.com/v4/sms/outgoing?apikey=...).
[2] inMobile SDK / Packagist documentation (v4 API messaging endpoints).
Fix the SMS endpoint path to include the API version prefix.
The endpoint path is missing the /v4/ prefix. The correct InMobile REST API v4 endpoint path for sending SMS messages is /v4/sms/outgoing, not /sms/outgoing.
Update line 33:
path: "/v4/sms/outgoing",🤖 Prompt for AI Agents
In components/inmobile/inmobile.app.mjs around lines 25 to 36, the sendSms call
uses the wrong endpoint path; change the path from "/sms/outgoing" to include
the API version prefix "/v4/sms/outgoing" so the request targets the InMobile
REST API v4 endpoint.
WHY
Resolves #9188
Summary by CodeRabbit
New Features
Refactor
Chores