18329 components espy #18336
18329 components espy #18336
Conversation
- Added new props for value and searchId in espy.app.mjs to facilitate lookups. - Implemented actions for email lookup, name lookup, and retrieving lookup results, each with corresponding methods for API interaction. - Updated package version to 0.1.0 and added dependency on @pipedream/platform. - Improved API request handling with a centralized method for making requests.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds three new ESPY actions (Email Lookup, Name Lookup, Get Lookup Results) and expands the ESPY app with an internal HTTP client, lookup methods, and prop definitions. Introduces axios via @pipedream/platform and updates package.json (version bump to 0.1.0, new dependency). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Action as Email/Name Lookup Action
participant App as ESPY App
participant API as ESPY API
User->>Action: Provide value
Action->>App: emailLookup/nameLookup({ value, lookupId })
App->>API: POST /developer/combined_email or /developer/combined_name
API-->>App: { id, ... }
App-->>Action: response
Action-->>User: $summary with response.id
sequenceDiagram
autonumber
actor User
participant Action as Get Lookup Results Action
participant App as ESPY App
participant API as ESPY API
User->>Action: Provide searchId (+ optional value)
Action->>App: getLookupResults({ searchId, data, params })
App->>API: GET /request-monitor/api-usage/{searchId}
API-->>App: results payload
App-->>Action: response
Action-->>User: $summary with searchId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks (2 passed, 2 warnings, 1 inconclusive)❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Poem
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.
Please see the documentation for more information. 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: 5
🧹 Nitpick comments (8)
components/espy/espy.app.mjs (3)
64-71: Explicitly use GET for results endpointMake the method explicit and rely on _makeRequest to place the key in params.
- async getLookupResults({ - searchId, ...args - }) { - return this._makeRequest({ - path: `/request-monitor/api-usage/${searchId}`, - ...args, - }); - }, + async getLookupResults({ searchId, ...args }) { + return this._makeRequest({ + path: `/request-monitor/api-usage/${searchId}`, + method: "get", + ...args, + }); + },
72-79: Prefer centralizing auth for GET; avoid duplicating key in params hereIf _makeRequest handles GET auth via params, you don’t need to set it per-call.
- async getSearchIds(args = {}) { - return this._makeRequest({ - path: "/request-monitor/api-usage", - params: { - key: `${this.$auth.api_key}`, - }, - ...args, - }); - }, + async getSearchIds(args = {}) { + return this._makeRequest({ + path: "/request-monitor/api-usage", + method: "get", + ...args, + }); + },
16-23: Harden options() and improve UX with labelsGuard against missing fields and provide labels so users see meaningful choices.
- async options() { - const response = await this.getSearchIds(); - const searchIds = response.list; - return searchIds.map(({ id }) => ({ - value: id, - })); - }, + async options() { + const response = await this.getSearchIds(); + const searchIds = Array.isArray(response?.list) ? response.list : []; + return searchIds.map(({ id }) => ({ + label: String(id), + value: String(id), + })); + },components/espy/actions/name-lookup/name-lookup.mjs (2)
23-25: Avoid magic number for lookupIdCentralize lookup IDs to prevent drift across actions and document their meaning.
Example (outside this file):
// components/espy/espy.constants.mjs export const LOOKUP_IDS = { EMAIL_COMBINED: 67, NAME_COMBINED: 149 };Then here:
- lookupId: 149, + lookupId: LOOKUP_IDS.NAME_COMBINED,And import the constant accordingly.
27-27: Tweak summary formattingDrop the extra quotes around the ID.
- $.export("$summary", `Successfully sent request. Use the ID to get the results: '${response.id}'`); + $.export("$summary", `Successfully sent request. Use the ID to get the results: ${response.id}`);components/espy/actions/email-lookup/email-lookup.mjs (1)
23-25: Avoid magic number for lookupId (67)Extract the constant and name it for clarity, or centralize in the app client.
Apply this diff to localize the constant:
@@ -import app from "../../espy.app.mjs"; - +import app from "../../espy.app.mjs"; +const LOOKUP_ID_EMAIL = 67; @@ - lookupId: 67, + lookupId: LOOKUP_ID_EMAIL,Follow-up (optional): Move
lookupIdhandling intothis.app.emailLookup()so actions don’t pass it at all.components/espy/actions/get-lookup-results/get-lookup-results.mjs (2)
32-34: Centralize auth in the app clientInline
params.keyduplicates responsibility and can drift from other actions.If the app client already injects auth, remove this block from here. Otherwise, consider moving auth injection into
_makeRequest()to keep actions clean.
33-33: Nit: unnecessary template literalPass the API key directly.
Apply this diff:
- key: `${this.app.$auth.api_key}`, + key: this.app.$auth.api_key,
📜 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 (5)
components/espy/actions/email-lookup/email-lookup.mjs(1 hunks)components/espy/actions/get-lookup-results/get-lookup-results.mjs(1 hunks)components/espy/actions/name-lookup/name-lookup.mjs(1 hunks)components/espy/espy.app.mjs(1 hunks)components/espy/package.json(2 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/espy/package.json
🧬 Code graph analysis (4)
components/espy/actions/name-lookup/name-lookup.mjs (2)
components/espy/actions/email-lookup/email-lookup.mjs (1)
response(20-26)components/espy/actions/get-lookup-results/get-lookup-results.mjs (1)
response(26-35)
components/espy/actions/email-lookup/email-lookup.mjs (2)
components/espy/actions/get-lookup-results/get-lookup-results.mjs (1)
response(26-35)components/espy/actions/name-lookup/name-lookup.mjs (1)
response(20-26)
components/espy/actions/get-lookup-results/get-lookup-results.mjs (3)
components/espy/actions/email-lookup/email-lookup.mjs (1)
response(20-26)components/espy/actions/name-lookup/name-lookup.mjs (1)
response(20-26)components/espy/espy.app.mjs (1)
response(17-17)
components/espy/espy.app.mjs (3)
components/espy/actions/email-lookup/email-lookup.mjs (1)
response(20-26)components/espy/actions/get-lookup-results/get-lookup-results.mjs (1)
response(26-35)components/espy/actions/name-lookup/name-lookup.mjs (1)
response(20-26)
⏰ 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: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (9)
components/espy/package.json (2)
3-3: Version bump LGTMMatches the scope of new app + actions.
15-17: @pipedream/platform version confirmed
^3.1.0 is the latest release, it exposesaxiosas a named export, and there have been no breaking changes since 3.1.0—no further pinning required.components/espy/espy.app.mjs (1)
26-28: ESPY API base URL confirmed
The base URLhttps://irbis.espysys.com/apicorrectly matches the production endpoints for combined_email (POST /developer/combined_email), combined_name (POST /developer/combined_name), and request-monitor/api-usage (GET /request-monitor/api-usage and /request-monitor/api-usage/{id}).components/espy/actions/name-lookup/name-lookup.mjs (2)
7-7: Action versioning checkConfirm 0.0.1 is intended for initial release under package 0.1.0; align with the other new actions for consistency.
6-6: Docs link correctly maps to combined_name endpoint
Confirmed that “Name SocialScan / Name WebScan (by name lookup)” in the docs refers to POST /api/developer/combined_name and that lookupId 149 is correct; no changes needed.components/espy/actions/email-lookup/email-lookup.mjs (2)
20-26: Auth handling consistencyThis action relies on
this.app.emailLookup()to add auth, whileget-lookup-resultspassesparams.keyexplicitly. Please standardize via the app client to avoid drift and accidental 401s.Would you confirm the app client always injects the API key for all endpoints? If yes, drop inline auth elsewhere; if not, add it here.
3-18: Action scaffold looks goodKey/name/version/props follow the Pipedream pattern and reuse prop definitions correctly.
components/espy/actions/get-lookup-results/get-lookup-results.mjs (2)
6-6: Verify docs URLThe slug includes “get-get”. If that’s not intentional, update the link.
3-9: LGTM on overall structureGood reuse of prop definitions and clear summary.
Also applies to: 17-23, 36-38
| data: { | ||
| value: this.value, | ||
| }, |
There was a problem hiding this comment.
Likely incorrect: sending a body on a GET
Fetching results should not require data (and some HTTP clients drop GET bodies). Remove it.
Apply this diff:
- data: {
- value: this.value,
- },📝 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.
| data: { | |
| value: this.value, | |
| }, |
🤖 Prompt for AI Agents
In components/espy/actions/get-lookup-results/get-lookup-results.mjs around
lines 29-31, the GET request is including a request body (data: { value:
this.value }) which is incorrect and may be dropped by HTTP clients; remove the
data/body entirely and instead pass the value as a query parameter (e.g., use
params/query or append to the URL) so the GET carries the input via the URL
rather than a request body.
There was a problem hiding this comment.
Quick question here @luancazarine is the get method also accepting data payload?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Hi @luancazarine Just had one question other than that lgtm! Ready for QA!
| data: { | ||
| value: this.value, | ||
| }, |
There was a problem hiding this comment.
Quick question here @luancazarine is the get method also accepting data payload?
Resolves #18329
Summary by CodeRabbit