Pipedrive - search-persons fetch all customFields #18966
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughPatch-level version bumps across many Pipedrive actions and sources; adds a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Search as search-persons Action
participant App as Pipedrive App
participant Utils as formatCustomFields
participant FieldsAPI as getPersonCustomFields
participant PersonsAPI as Persons API (v2)
User->>Search: Run search-persons (includeAllCustomFields = true)
Search->>App: call searchPersons (uses Persons API v2)
App->>PersonsAPI: search persons
PersonsAPI-->>Search: resp (items with custom_fields)
alt no persons found
Search-->>User: Summary "No persons found"
else includeAllCustomFields enabled
Search->>Utils: formatCustomFields(resp, getPersons, getPersonCustomFields)
Utils->>FieldsAPI: fetch field definitions (key → name)
FieldsAPI-->>Utils: field mappings
Utils->>PersonsAPI: fetch person resources (ids)
PersonsAPI-->>Utils: person resources
Utils-->>Search: transformed resp (custom_fields remapped)
Search-->>User: Return results with formatted custom fields
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (25)
components/pipedrive/actions/add-activity/add-activity.mjs(1 hunks)components/pipedrive/actions/add-deal/add-deal.mjs(1 hunks)components/pipedrive/actions/add-lead/add-lead.mjs(1 hunks)components/pipedrive/actions/add-note/add-note.mjs(1 hunks)components/pipedrive/actions/add-organization/add-organization.mjs(1 hunks)components/pipedrive/actions/add-person/add-person.mjs(1 hunks)components/pipedrive/actions/get-lead-by-id/get-lead-by-id.mjs(1 hunks)components/pipedrive/actions/get-person-details/get-person-details.mjs(1 hunks)components/pipedrive/actions/merge-deals/merge-deals.mjs(1 hunks)components/pipedrive/actions/merge-persons/merge-persons.mjs(1 hunks)components/pipedrive/actions/remove-duplicate-notes/remove-duplicate-notes.mjs(1 hunks)components/pipedrive/actions/search-leads/search-leads.mjs(1 hunks)components/pipedrive/actions/search-notes/search-notes.mjs(1 hunks)components/pipedrive/actions/search-persons/search-persons.mjs(4 hunks)components/pipedrive/actions/update-deal/update-deal.mjs(1 hunks)components/pipedrive/actions/update-person/update-person.mjs(1 hunks)components/pipedrive/common/utils.mjs(1 hunks)components/pipedrive/package.json(1 hunks)components/pipedrive/pipedrive.app.mjs(2 hunks)components/pipedrive/sources/new-deal-instant/new-deal-instant.mjs(1 hunks)components/pipedrive/sources/new-event-instant/new-event-instant.mjs(1 hunks)components/pipedrive/sources/new-person-instant/new-person-instant.mjs(1 hunks)components/pipedrive/sources/updated-deal-instant/updated-deal-instant.mjs(1 hunks)components/pipedrive/sources/updated-lead-instant/updated-lead-instant.mjs(1 hunks)components/pipedrive/sources/updated-person-instant/updated-person-instant.mjs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-12T19:23:09.039Z
Learnt from: jcortes
Repo: PipedreamHQ/pipedream PR: 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/pipedrive/package.json
📚 Learning: 2025-06-04T17:52:05.780Z
Learnt from: GTFalcao
Repo: PipedreamHQ/pipedream PR: 16954
File: components/salesloft/salesloft.app.mjs:14-23
Timestamp: 2025-06-04T17:52:05.780Z
Learning: In the Salesloft API integration (components/salesloft/salesloft.app.mjs), the _makeRequest method returns response.data which directly contains arrays for list endpoints like listPeople, listCadences, listUsers, and listAccounts. The propDefinitions correctly call .map() directly on these responses without needing to destructure a nested data property.
Applied to files:
components/pipedrive/pipedrive.app.mjscomponents/pipedrive/actions/search-persons/search-persons.mjs
🧬 Code graph analysis (2)
components/pipedrive/common/utils.mjs (2)
components/pipedrive/actions/search-persons/search-persons.mjs (1)
resp(73-82)components/pipedrive/pipedrive.app.mjs (1)
persons(32-38)
components/pipedrive/actions/search-persons/search-persons.mjs (1)
components/pipedrive/common/utils.mjs (2)
formatCustomFields(59-85)formatCustomFields(59-85)
⏰ 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: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (25)
components/pipedrive/sources/new-person-instant/new-person-instant.mjs (1)
9-9: LGTM! Standard version bump.The patch version increment is appropriate and consistent with the PR's updates across Pipedrive components.
components/pipedrive/actions/add-lead/add-lead.mjs (1)
9-9: Version bump only—core functionality changes missing.This file shows only a version bump. The PR's core functionality (formatCustomFields utility, includeAllCustomFields prop, search-persons logic changes) is not visible in the provided files. Verify that
components/pipedrive/actions/search-persons/search-persons.mjsandcomponents/pipedrive/common/utils.mjsare included in this PR and contain the stated changes.components/pipedrive/package.json (1)
1-21: Package version and dependencies look good.The version bump to 0.10.4 aligns with the broader release pattern across actions and sources. No new external dependencies were introduced—the feature is implemented via internal utilities. Dependencies are stable and appropriate.
components/pipedrive/actions/add-organization/add-organization.mjs (1)
8-8: Version bump looks good.This metadata-only update aligns with the coordinated package-wide version bumps across Pipedrive components.
components/pipedrive/actions/get-person-details/get-person-details.mjs (1)
8-8: Version bump looks good.Metadata-only update consistent with the PR's coordinated versioning strategy.
components/pipedrive/sources/new-event-instant/new-event-instant.mjs (1)
8-8: Version bump looks good.Metadata-only update aligned with package-wide versioning.
components/pipedrive/actions/merge-persons/merge-persons.mjs (1)
7-7: Version bump looks good.Metadata-only update consistent with coordinated package versioning.
components/pipedrive/sources/updated-deal-instant/updated-deal-instant.mjs (1)
10-10: Version bump looks good.Metadata-only update aligned with the PR's versioning strategy.
components/pipedrive/actions/add-activity/add-activity.mjs (1)
10-10: Version bump looks good.Metadata-only update consistent with coordinated package versioning.
components/pipedrive/actions/update-person/update-person.mjs (1)
9-9: Version bump looks good.Metadata-only update aligned with the PR's coordinated versioning.
components/pipedrive/sources/updated-lead-instant/updated-lead-instant.mjs (1)
10-10: Version bump looks good.Metadata-only update consistent with the package-wide versioning strategy.
components/pipedrive/actions/remove-duplicate-notes/remove-duplicate-notes.mjs (1)
8-8: LGTM: Version bumpStandard version increment as part of the package release cycle.
components/pipedrive/actions/search-notes/search-notes.mjs (1)
7-7: LGTM: Version bumpStandard version increment as part of the package release cycle.
components/pipedrive/sources/new-deal-instant/new-deal-instant.mjs (1)
9-9: LGTM: Version bumpStandard version increment as part of the package release cycle.
components/pipedrive/sources/updated-person-instant/updated-person-instant.mjs (1)
10-10: LGTM: Version bumpStandard version increment as part of the package release cycle.
components/pipedrive/actions/get-lead-by-id/get-lead-by-id.mjs (1)
7-7: LGTM: Version bumpStandard version increment as part of the package release cycle.
components/pipedrive/common/utils.mjs (1)
49-57: LGTM: Custom field name mappingThe logic correctly filters user-created custom fields and builds a key-to-name mapping.
components/pipedrive/pipedrive.app.mjs (2)
373-378: LGTM: New feature flag addedThe
includeAllCustomFieldsprop definition is correctly structured with appropriate metadata.
488-491: LGTM: API version upgradeThe switch to v2 PersonsApi aligns with the new custom fields feature and ensures compatibility with the enhanced search functionality.
components/pipedrive/actions/search-persons/search-persons.mjs (6)
5-5: LGTM: Import addedCorrect import of the new
formatCustomFieldsutility.
11-11: LGTM: Version bumpVersion increment reflects the new feature addition.
22-22: LGTM: Label capitalization improvementsThe label text updates improve consistency and readability in the UI.
Also applies to: 27-27, 34-34, 47-47, 54-54, 61-61
64-69: LGTM: Feature flag integrationCorrectly references the new
includeAllCustomFieldsprop definition from the app.
84-87: LGTM: Early exit for empty resultsGood UX improvement - provides clear feedback when no persons are found instead of returning an empty structure.
89-95: Verify bound method compatibility and consider immutabilityTwo considerations:
The code directly mutates
resp.data.items(line 90). While this works, returning a new response object would be more predictable and safer for downstream consumers.Verify that
this.pipedriveApp.getPersons.bind(this)correctly passes the bound context, sinceformatCustomFieldscalls the function with anidsparameter that may not match the expected method signature.Consider this refactor to avoid mutation:
if (this.includeAllCustomFields) { - resp.data.items = await formatCustomFields( + const formattedItems = await formatCustomFields( resp, this.pipedriveApp.getPersons.bind(this), this.pipedriveApp.getPersonCustomFields.bind(this), ); + resp = { + ...resp, + data: { + ...resp.data, + items: formattedItems, + }, + }; }
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/pipedrive/common/utils.mjs (1)
60-62: Fix parameter type and implement batching for large result sets.Two unresolved issues from previous review:
Type mismatch (critical): The Pipedrive v2 PersonsApi expects
idsas a comma-separated string (e.g.,"1,2,3"), but line 61 passes an array. This will cause the API call to fail or return unexpected results.Missing batching (major): The API accepts a maximum of 100 IDs per request. For search results with more than 100 persons, this will either fail or silently truncate the data, leading to incomplete custom field formatting.
Apply this diff to fix the type conversion and add batching:
- const { data: persons } = await getResourcesFn({ - ids: resp.data.items.map((item) => item.item.id), - }); + const ids = resp.data.items.map((item) => item.item.id); + const batchSize = 100; + const batches = []; + + for (let i = 0; i < ids.length; i += batchSize) { + const batchIds = ids.slice(i, i + batchSize).join(','); + const { data: batchPersons } = await getResourcesFn({ + ids: batchIds, + }); + batches.push(...batchPersons); + } + + const persons = batches;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/pipedrive/common/utils.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/pipedrive/common/utils.mjs (2)
components/pipedrive/actions/search-persons/search-persons.mjs (1)
resp(73-82)components/pipedrive/pipedrive.app.mjs (1)
persons(32-38)
⏰ 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: Lint Code Base
- GitHub Check: Ensure component commits modify component versions
- GitHub Check: Verify TypeScript components
🔇 Additional comments (4)
components/pipedrive/common/utils.mjs (4)
49-57: LGTM! Clean helper function.The
getCustomFieldNameshelper correctly filters user-created custom fields and builds a key-to-name mapping for downstream formatting.
65-68: Good fix: Object check correctly implemented.The guard now properly checks whether
custom_fieldsis an object with keys, addressing the previous review concern about treating it as an array.
73-81: Good fix: Unmapped fields are now skipped.The guard at line 78 correctly prevents assignment when
customFieldNames[key]is undefined, addressing the previous concern about unmapped custom fields.
82-88: LGTM! Clean object merging.The return statement correctly preserves the original person structure while updating the custom_fields with formatted values.
For Integration QA: |
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check test reports below for more information:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/pipedrive/common/utils.mjs (2)
60-62: Critical: Fix ids parameter type and implement batching.Two critical issues remain unresolved from previous review:
Type mismatch: Line 61 passes an array to
ids, but the Pipedrive v2 PersonsApi expects a comma-separated string (e.g.,"1,2,3"). This will cause the API call to fail or behave unexpectedly.Missing batching: The Pipedrive API supports a maximum of 100 IDs per request. Search results with >100 persons will either fail or silently truncate, causing incomplete custom field data.
Apply this diff to fix the type and add batching:
- const { data: persons } = await getResourcesFn({ - ids: resp.data.items.map((item) => item.item.id), - }); + const allIds = resp.data.items.map((item) => item.item.id); + const batches = []; + for (let i = 0; i < allIds.length; i += 100) { + batches.push(allIds.slice(i, i + 100)); + } + + const personBatches = await Promise.all( + batches.map((batch) => getResourcesFn({ ids: batch.join(',') })) + ); + const persons = personBatches.flatMap((batch) => batch.data);
66-74: Critical: Destructuring undefined throws before guards execute.Line 66 destructures
custom_fieldsimmediately frompersons.find(), but if the person ID doesn't exist in the fetched persons array,find()returnsundefinedand the destructuring throws a runtime error before the guards on lines 68 and 72 can handle it.Additionally, lines 68 and 72 contain redundant checks for
customFields.length === 0.Apply this diff to assign before checking and eliminate redundancy:
- const { custom_fields: customFields } = persons.find((p) => p.id === person.item.id); - - if (!person.item?.custom_fields || Object.keys(customFields).length === 0) { - return person; - } - - if (!customFields || Object.keys(customFields).length === 0) { + const foundPerson = persons.find((p) => p.id === person.item.id); + if (!foundPerson?.custom_fields || Object.keys(foundPerson.custom_fields).length === 0) { return person; } + const customFields = foundPerson.custom_fields;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/pipedrive/common/utils.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/pipedrive/common/utils.mjs (2)
components/pipedrive/actions/search-persons/search-persons.mjs (1)
resp(73-82)components/pipedrive/pipedrive.app.mjs (1)
persons(32-38)
⏰ 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: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (2)
components/pipedrive/common/utils.mjs (2)
49-57: LGTM: Clean helper for custom field name mapping.The logic correctly filters user-created fields and builds a key-to-name mapping for downstream formatting.
76-83: LGTM: Guard correctly prevents undefined keys.The check at line 80 ensures only mapped custom fields are included in the formatted output, preventing properties with undefined keys.
|
Hi everyone, all test cases are passed! Ready for release! Test reports
|
|
/approve |
Resolves #18939
Summary by CodeRabbit
New Features
Improvements
Chores