ServiceNow new components #19676
ServiceNow new components #19676
Conversation
They were actually using the wrong entity. We're moving forward with generic record components only
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
WalkthroughMigrates ServiceNow integration to an OAuth-enabled, table-centric app: removes legacy ticket/sysid actions, adds table CRUD and aggregate actions, consolidates HTTP logic into a unified _makeRequest and new app methods, updates action keys/props, and bumps package version. Changes
Sequence Diagram(s)sequenceDiagram
participant Action as Action
participant App as ServiceNow App
participant Req as _makeRequest
participant API as ServiceNow API
Note over Action,API: OAuth table-based flow
Action->>App: invoke e.g. createTableRecord({ $, table, data, params })
App->>Req: build request (method, path, headers with OAuth token)
Req->>API: HTTP request to /table/{table} or /stats/{table}
API-->>Req: response
Req-->>App: parsed response
App-->>Action: return response (and $.export("$summary"))
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 9
🤖 Fix all issues with AI agents
In @components/servicenow/actions/create-table-record/create-table-record.mjs:
- Around line 22-26: The input schema for the create-table-record action allows
an empty object for recordData which can produce blank records; add a non-empty
validation check for recordData (referencing the recordData input in
create-table-record) and reject empty objects before making the ServiceNow API
call—e.g., verify Object.keys(recordData).length > 0 (or equivalent) in the
action handler and return/throw a clear validation error if it's empty so
accidental `{}` inputs are blocked.
In @components/servicenow/actions/delete-table-record/delete-table-record.mjs:
- Around line 4-12: The action object for the "Delete Table Record" action (key:
"servicenow_oauth_-delete-table-record", name: "Delete Table Record") has
version set to "0.0.1"; update the version property to "1.0.0" to match the
other new/rewritten table actions in this PR so versioning is consistent across
actions.
In
@components/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjs:
- Around line 90-121: The validation wrongly rejects "count"-only requests
because hasAggregateFields is initialized false and never considers
sysparm_count; change the logic so hasAggregateFields is initialized or set
based on this.count (e.g., set hasAggregateFields = !!this.count before the
avg/min/max/sum checks, or explicitly check if this.count is truthy and set
hasAggregateFields = true) so that the existing check that throws
ConfigurationError recognizes count as a valid aggregate operation; update the
block around params and the hasAggregateFields variable (the symbol names:
params, hasAggregateFields, this.count, ConfigurationError) accordingly.
- Around line 82-97: Remove the standalone sysparm_order_by entry from the
params object in the run method (where params is constructed) and instead inject
the orderBy value into sysparm_query using ServiceNow encoded-query ordering
syntax (prepend/append ^ORDERBYfield_name or ^ORDERBYDESCfield_name to
this.query/sysparm_query when this.orderBy is set); also update the orderBy prop
description to reference ORDERBY/ORDERBYDESC (not ^ORDER_BY/^ORDER_BYDESC) so
callers know to supply a field name and direction indicator that will be merged
into sysparm_query.
In @components/servicenow/actions/update-table-record/update-table-record.mjs:
- Around line 28-38: The action accepts updateFields and replaceRecord but
doesn't validate that updateFields is an object with at least one key, which can
produce a no-op for PATCH or accidentally clear data for PUT; add a fail-fast
validation that ensures updateFields is a non-empty object (typeof updateFields
=== "object" && Object.keys(updateFields).length > 0) and throw/return a clear
validation error when it fails; additionally, if replaceRecord is true enforce
the same non-empty check (or provide a specific error telling the caller that
replacing requires at least one field) so the action never proceeds with an
empty payload.
- Around line 4-7: The component key string
"servicenow_oauth_-update-table-record" contains a temporary oauth_ prefix;
change it to the permanent identifier "servicenow-update-table-record" in the
key field of the Update Table Record component (the object that defines
key/name/version), and if you intend to rename instead of restoring the
canonical key, increment the major version and add migration notes; otherwise
simply remove the oauth_ prefix so the component keeps its stable identity.
In @components/servicenow/servicenow.app.mjs:
- Around line 12-22: The options method is returning potentially large result
sets from getTableRecords for table "sys_db_object"; add a sysparm_limit to the
params to cap results (e.g., 20 or configurable) to improve responsiveness.
Modify the params object passed to getTableRecords in the options({ query })
function to include a sysparm_limit property alongside sysparm_query and
sysparm_fields, ensuring the call to getTableRecords still returns the same
fields but only up to the configured limit.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
components/servicenow/actions/create-case/create-case.mjscomponents/servicenow/actions/create-incident/create-incident.mjscomponents/servicenow/actions/create-table-record/create-table-record.mjscomponents/servicenow/actions/delete-table-record/delete-table-record.mjscomponents/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjscomponents/servicenow/actions/get-table-record-by-id/get-table-record-by-id.mjscomponents/servicenow/actions/get-table-record-by-sysid/get-table-record-by-sysid.mjscomponents/servicenow/actions/get-table-records/get-table-records.mjscomponents/servicenow/actions/update-table-record/update-table-record.mjscomponents/servicenow/package.jsoncomponents/servicenow/servicenow.app.mjs
💤 Files with no reviewable changes (3)
- components/servicenow/actions/get-table-record-by-sysid/get-table-record-by-sysid.mjs
- components/servicenow/actions/create-case/create-case.mjs
- components/servicenow/actions/create-incident/create-incident.mjs
🧰 Additional context used
🧬 Code graph analysis (6)
components/servicenow/actions/get-table-record-by-id/get-table-record-by-id.mjs (3)
components/servicenow/servicenow.app.mjs (1)
response(98-105)components/servicenow/actions/create-table-record/create-table-record.mjs (1)
response(59-70)components/servicenow/actions/update-table-record/update-table-record.mjs (1)
response(71-84)
components/servicenow/servicenow.app.mjs (5)
components/servicenow/actions/get-table-records/get-table-records.mjs (2)
query(92-92)response(117-129)components/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjs (1)
response(123-127)components/servicenow/actions/create-table-record/create-table-record.mjs (1)
response(59-70)components/servicenow/actions/get-table-record-by-id/get-table-record-by-id.mjs (1)
response(60-71)components/servicenow/actions/update-table-record/update-table-record.mjs (1)
response(71-84)
components/servicenow/actions/create-table-record/create-table-record.mjs (4)
components/servicenow/servicenow.app.mjs (1)
response(98-105)components/servicenow/actions/get-table-record-by-id/get-table-record-by-id.mjs (1)
response(60-71)components/servicenow/actions/update-table-record/update-table-record.mjs (1)
response(71-84)components/servicenow/actions/get-table-records/get-table-records.mjs (1)
response(117-129)
components/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjs (1)
components/servicenow/servicenow.app.mjs (1)
response(98-105)
components/servicenow/actions/get-table-records/get-table-records.mjs (3)
components/servicenow/servicenow.app.mjs (1)
response(98-105)components/servicenow/actions/create-table-record/create-table-record.mjs (1)
response(59-70)components/servicenow/actions/update-table-record/update-table-record.mjs (1)
response(71-84)
components/servicenow/actions/update-table-record/update-table-record.mjs (4)
components/servicenow/servicenow.app.mjs (1)
response(98-105)components/servicenow/actions/create-table-record/create-table-record.mjs (1)
response(59-70)components/servicenow/actions/get-table-record-by-id/get-table-record-by-id.mjs (1)
response(60-71)components/servicenow/actions/get-table-records/get-table-records.mjs (1)
response(117-129)
⏰ 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 (8)
components/servicenow/package.json (1)
3-4: Package metadata update looks fine.
0.8.0 + description tweak match the scope of the refactor.components/servicenow/actions/get-table-record-by-id/get-table-record-by-id.mjs (1)
59-71: Good use of the centralized app method; relies on safe param serialization.
If_makeRequeststrips undefined params (recommended), this params object is good and consistent with other actions.components/servicenow/actions/create-table-record/create-table-record.mjs (1)
58-70: Migration tothis.servicenow.createTableRecordlooks consistent with the new app API.
Param mapping matches the other table actions.components/servicenow/actions/get-table-records/get-table-records.mjs (1)
91-115: Good guardrail: require query or at least one filter before querying the table.
This prevents accidental full-table scans.components/servicenow/servicenow.app.mjs (1)
93-107: Harden_makeRequestfor empty responses + strip undefined query params.DELETE may return 204/no body; and passing
paramswith undefined keys can leak empty query params depending on serialization. Centralizing both behaviors here avoids repeating fixes across actions.Proposed change
methods: { async _makeRequest({ $ = this, headers, ...args }) { + // Avoid emitting `foo=` in querystrings for undefined values. + if (args?.params && typeof args.params === "object") { + args.params = Object.fromEntries( + Object.entries(args.params).filter(([, v]) => v !== undefined) + ); + } const response = await axios($, { baseURL: `https://${this.$auth.instance_name}.service-now.com/api/now`, headers: { ...headers, "Authorization": `Bearer ${this.$auth.oauth_access_token}`, }, ...args, }); - return response.result; + // Some endpoints (e.g., DELETE) can return an empty body. + return response?.result ?? null; },⛔ Skipped due to learnings
Learnt from: GTFalcao Repo: PipedreamHQ/pipedream PR: 18362 File: components/leonardo_ai/actions/generate-image/generate-image.mjs:103-105 Timestamp: 2025-09-15T22:01:11.472Z Learning: In Pipedream components, pipedream/platform's axios implementation automatically excludes undefined values from HTTP requests, so there's no need to manually check for truthiness before including properties in request payloads.components/servicenow/actions/delete-table-record/delete-table-record.mjs (1)
35-43: This concern is not applicable to this codebase. The pattern of unconditionally including optional parameters is standard across all servicenow actions (get-table-records, get-table-record-by-id, update-table-record, create-table-record). axios automatically stripsundefinedvalues from query parameters according to its standard behavior, so there is no risk of empty query params being sent. Applying conditional assignment only to this action would create inconsistency with the established pattern used throughout the component.Likely an incorrect or invalid review comment.
components/servicenow/actions/update-table-record/update-table-record.mjs (2)
39-68: AddexcludeReferenceLinksfor parity with other actions (smaller, more predictable responses).
get-table-record-by-idandcreate-table-recordmapsysparm_exclude_reference_link, but this action doesn’t, so the update response shape/payload can differ for no obvious reason.Proposed fix (prop + param)
props: { servicenow, @@ responseDataFormat: { propDefinition: [ servicenow, "responseDataFormat", ], }, + excludeReferenceLinks: { + propDefinition: [ + servicenow, + "excludeReferenceLinks", + ], + }, responseFields: { propDefinition: [ servicenow, "responseFields", ], }, @@ async run({ $ }) { const response = await this.servicenow.updateTableRecord({ @@ params: { sysparm_display_value: this.responseDataFormat, + sysparm_exclude_reference_link: this.excludeReferenceLinks, sysparm_fields: this.responseFields?.join?.() || this.responseFields, sysparm_input_display_value: this.inputDisplayValue, sysparm_view: this.responseView, sysparm_query_no_domain: this.queryNoDomain, }, });Also applies to: 77-83
⛔ Skipped due to learnings
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.
70-92: > Likely an incorrect or invalid review comment.
components/servicenow/actions/create-table-record/create-table-record.mjs
Show resolved
Hide resolved
components/servicenow/actions/delete-table-record/delete-table-record.mjs
Show resolved
Hide resolved
| orderBy: { | ||
| type: "string", | ||
| label: "Order By", | ||
| description: "Field to sort results by. Prefix with `^ORDER_BY` for ascending or `^ORDER_BYDESC` for descending (e.g., `^ORDER_BYDESC` + field name)", | ||
| optional: true, | ||
| }, | ||
| }, | ||
| async run({ $ }) { | ||
| const params = { | ||
| sysparm_count: this.count, | ||
| sysparm_query: this.query, | ||
| sysparm_group_by: this.groupByField, | ||
| sysparm_display_value: this.responseDataFormat, | ||
| sysparm_having: this.havingQuery, | ||
| sysparm_order_by: this.orderBy, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ServiceNow Aggregate API /api/now/stats table sysparm_order_by sysparm_display_value parameters
💡 Result:
Short answer
-
sysparm_display_value (true | false | all) on the stats (/api/now/stats/{table}) call controls whether reference/field display text or raw values (or both) are returned. Use true for display text, false for raw values (sys_id / internal value), and all for both. [1][2]
-
There is no separate, reliable sysparm_order_by for the Aggregate/Stats API. To sort results use ORDERBY / ORDERBYDESC inside an encoded sysparm_query (e.g. sysparm_query=active=true^ORDERBYsys_created_on). Putting an order clause after the & (as a separate sysparm) is not treated as part of the encoded query. [1][3]
Examples
-
Return counts with display values:
/api/now/stats/incident?sysparm_count=true&sysparm_query=priority=1&sysparm_display_value=true. [1] -
Order by created date (use in sysparm_query):
/api/now/stats/incident?sysparm_count=true&sysparm_query=active=true^ORDERBYsys_created_on. [3]
Notes / gotchas
- sysparm_display_value can have performance implications—use only when needed. [1]
- If you need per-field control of display vs raw values, consider dot-walking / explicit fields or using sysparm_display_value=all and reading the display_value/value pairs. Community posts document behaviors and edge cases. [2][3]
Sources
- ServiceNow developer docs — REST query parameter guidance (sysparm_display_value / encoded queries). [developer.servicenow.com]. [2]
- ServiceNow Community / StackOverflow discussions on sysparm_display_value (true/false/all) and examples. [1][2]
- Community guidance showing ORDERBY / ORDERBYDESC must be in sysparm_query (not a separate sysparm_orderby). [3]
If you want, I can draft the exact stats URL you need (count, group_by, having, order) for your table and fields — tell me the table name and desired aggregation.
🏁 Script executed:
cat components/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjsRepository: PipedreamHQ/pipedream
Length of output: 4723
🏁 Script executed:
rg "responseDataFormat" components/servicenow/ -A 5 -B 2Repository: PipedreamHQ/pipedream
Length of output: 10880
🏁 Script executed:
sed -n '/responseDataFormat:/,/options:/p' components/servicenow/servicenow.app.mjs | head -20Repository: PipedreamHQ/pipedream
Length of output: 250
🏁 Script executed:
sed -n '/responseDataFormat:/,/^\s*},/p' components/servicenow/servicenow.app.mjs | head -30Repository: PipedreamHQ/pipedream
Length of output: 358
🏁 Script executed:
cat components/servicenow/servicenow.app.mjs | grep -A 25 '"responseDataFormat":'Repository: PipedreamHQ/pipedream
Length of output: 47
🏁 Script executed:
sed -n '50,150p' components/servicenow/servicenow.app.mjs | grep -A 20 "responseDataFormat"Repository: PipedreamHQ/pipedream
Length of output: 47
🏁 Script executed:
rg "responseDataFormat" components/servicenow/servicenow.app.mjs -A 15Repository: PipedreamHQ/pipedream
Length of output: 503
🏁 Script executed:
rg "responseDataFormat" components/servicenow/servicenow.app.mjs -A 20 | tail -10Repository: PipedreamHQ/pipedream
Length of output: 314
Fix orderBy implementation: remove sysparm_order_by parameter and use ORDERBY/ORDERBYDESC in encoded query instead.
The ServiceNow Aggregate API (/api/now/stats/{table}) does not support a separate sysparm_order_by parameter. Ordering must be done using ORDERBY or ORDERBYDESC directly inside the encoded sysparm_query (e.g., sysparm_query=active=true^ORDERBYfield_name or ^ORDERBYDESCfield_name).
Current issues:
sysparm_order_by: this.orderByas a standalone parameter is ignored by the API- The
orderByprop description incorrectly mentions^ORDER_BYsyntax; should document that ordering goes inside the query string usingORDERBY/ORDERBYDESC sysparm_display_valuevalues (true/false/all) are correct and supported
🤖 Prompt for AI Agents
In
@components/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjs
around lines 82 - 97, Remove the standalone sysparm_order_by entry from the
params object in the run method (where params is constructed) and instead inject
the orderBy value into sysparm_query using ServiceNow encoded-query ordering
syntax (prepend/append ^ORDERBYfield_name or ^ORDERBYDESCfield_name to
this.query/sysparm_query when this.orderBy is set); also update the orderBy prop
description to reference ORDERBY/ORDERBYDESC (not ^ORDER_BY/^ORDER_BYDESC) so
callers know to supply a field name and direction indicator that will be merged
into sysparm_query.
| const params = { | ||
| sysparm_count: this.count, | ||
| sysparm_query: this.query, | ||
| sysparm_group_by: this.groupByField, | ||
| sysparm_display_value: this.responseDataFormat, | ||
| sysparm_having: this.havingQuery, | ||
| sysparm_order_by: this.orderBy, | ||
| }; | ||
|
|
||
| let hasAggregateFields = false; | ||
|
|
||
| // Add aggregate functions for each field type | ||
| if (this.avgFields?.length) { | ||
| params.sysparm_avg_fields = this.avgFields.join?.() || this.avgFields; | ||
| hasAggregateFields = true; | ||
| } | ||
| if (this.minFields?.length) { | ||
| params.sysparm_min_fields = this.minFields.join?.() || this.minFields; | ||
| hasAggregateFields = true; | ||
| } | ||
| if (this.maxFields?.length) { | ||
| params.sysparm_max_fields = this.maxFields.join?.() || this.maxFields; | ||
| hasAggregateFields = true; | ||
| } | ||
| if (this.sumFields?.length) { | ||
| params.sysparm_sum_fields = this.sumFields.join?.() || this.sumFields; | ||
| hasAggregateFields = true; | ||
| } | ||
|
|
||
| if (!hasAggregateFields) { | ||
| throw new ConfigurationError("You must provide at least one Aggregate Field. [See the documentation](https://www.servicenow.com/docs/bundle/zurich-api-reference/page/integrate/inbound-rest/concept/c_AggregateAPI.html#title_aggregate-GET-stats) for more information."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the exact file and verify it exists
fd "get-record-counts-by-field.mjs" -t fRepository: PipedreamHQ/pipedream
Length of output: 152
🏁 Script executed:
# Read the file to see the actual code
cat components/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjsRepository: PipedreamHQ/pipedream
Length of output: 4723
🏁 Script executed:
# Search for where this.count is defined (likely in the props or defaults)
rg -A 5 -B 5 "count:" components/servicenow/actions/get-record-counts-by-field/Repository: PipedreamHQ/pipedream
Length of output: 2676
🏁 Script executed:
# Look for tests related to this action
fd "get-record-counts-by-field" -t f --glob "*.test.*" --glob "*.spec.*"Repository: PipedreamHQ/pipedream
Length of output: 215
🏁 Script executed:
# Search for the getRecordCountsByField method implementation
rg "getRecordCountsByField" components/servicenow/Repository: PipedreamHQ/pipedream
Length of output: 296
🏁 Script executed:
# Check the servicenow.app.mjs for the actual API call
rg -A 20 "getRecordCountsByField" components/servicenow/servicenow.app.mjsRepository: PipedreamHQ/pipedream
Length of output: 236
🏁 Script executed:
# Search for test files in the repo
find components/servicenow -name "*test*" -o -name "*spec*" | head -20Repository: PipedreamHQ/pipedream
Length of output: 47
🏁 Script executed:
# Look for any documentation or examples of this action
rg -i "get-record-counts-by-field" --type md --type txtRepository: PipedreamHQ/pipedream
Length of output: 47
🏁 Script executed:
# Check if there are any other references to the aggregate fields validation
rg -B 5 -A 5 "hasAggregateFields" components/servicenow/Repository: PipedreamHQ/pipedream
Length of output: 4011
Bug: count-only requests fail validation despite count defaulting to true.
The action cannot be used in its primary configuration. With count enabled by default and no other aggregate fields provided, the validation throws a ConfigurationError, preventing the most common "group + count" usage. The validation logic does not account for sysparm_count as a valid aggregate operation.
Initialize hasAggregateFields to account for the count parameter:
Proposed fix
- let hasAggregateFields = false;
+ // Count itself is a valid aggregate for this endpoint.
+ let hasAggregateFields = !!this.count;🤖 Prompt for AI Agents
In
@components/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjs
around lines 90 - 121, The validation wrongly rejects "count"-only requests
because hasAggregateFields is initialized false and never considers
sysparm_count; change the logic so hasAggregateFields is initialized or set
based on this.count (e.g., set hasAggregateFields = !!this.count before the
avg/min/max/sum checks, or explicitly check if this.count is truthy and set
hasAggregateFields = true) so that the existing check that throws
ConfigurationError recognizes count as a valid aggregate operation; update the
block around params and the hasAggregateFields variable (the symbol names:
params, hasAggregateFields, this.count, ConfigurationError) accordingly.
| filterCreatedAtDate: { | ||
| type: "string", | ||
| label: "Filter by Date Created", | ||
| description: "Return records created only after the given date, in the format `YYYY-MM-DD HH:MM:SS` (e.g. `2026-01-01 00:00:00`).", | ||
| optional: true, | ||
| }, | ||
| sysparm_display_value: { | ||
| filterUpdatedAtDate: { | ||
| type: "string", | ||
| description: "Return field display values (true), actual values (false), or both (all) (default: false).", | ||
| label: "Filter by Date Updated", | ||
| description: "Return records updated only after the given date, in the format `YYYY-MM-DD HH:MM:SS` (e.g. `2026-01-01 00:00:00`).", | ||
| optional: true, | ||
| options: [ | ||
| "true", | ||
| "false", | ||
| "all", | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validate the date filter format (or switch to a date-time prop type if supported).
Right now any string is accepted, which can generate invalid encoded queries and confusing results.
Also applies to: 98-104
components/servicenow/actions/update-table-record/update-table-record.mjs
Show resolved
Hide resolved
components/servicenow/actions/update-table-record/update-table-record.mjs
Show resolved
Hide resolved
| async options({ query }) { | ||
| if (!(query?.length > 1)) { | ||
| return []; | ||
| } | ||
| const data = await this.getTableRecords({ | ||
| table: "sys_db_object", | ||
| params: { | ||
| sysparm_query: `nameLIKE${query}^ORlabelLIKE${query}`, | ||
| sysparm_fields: "name,label", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Add a sysparm_limit for table lookup options.
sys_db_object can be large; limiting results improves responsiveness and avoids loading huge option lists.
Proposed change
const data = await this.getTableRecords({
table: "sys_db_object",
params: {
sysparm_query: `nameLIKE${query}^ORlabelLIKE${query}`,
sysparm_fields: "name,label",
+ sysparm_limit: 50,
},
});📝 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.
| async options({ query }) { | |
| if (!(query?.length > 1)) { | |
| return []; | |
| } | |
| const data = await this.getTableRecords({ | |
| table: "sys_db_object", | |
| params: { | |
| sysparm_query: `nameLIKE${query}^ORlabelLIKE${query}`, | |
| sysparm_fields: "name,label", | |
| }, | |
| }); | |
| async options({ query }) { | |
| if (!(query?.length > 1)) { | |
| return []; | |
| } | |
| const data = await this.getTableRecords({ | |
| table: "sys_db_object", | |
| params: { | |
| sysparm_query: `nameLIKE${query}^ORlabelLIKE${query}`, | |
| sysparm_fields: "name,label", | |
| sysparm_limit: 50, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In @components/servicenow/servicenow.app.mjs around lines 12 - 22, The options
method is returning potentially large result sets from getTableRecords for table
"sys_db_object"; add a sysparm_limit to the params to cap results (e.g., 20 or
configurable) to improve responsiveness. Modify the params object passed to
getTableRecords in the options({ query }) function to include a sysparm_limit
property alongside sysparm_query and sysparm_fields, ensuring the call to
getTableRecords still returns the same fields but only up to the configured
limit.
|
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:
|
Throw a ConfigurationError if the search term is invalid.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/servicenow/servicenow.app.mjs`:
- Around line 93-107: The _makeRequest currently unconditionally returns
response.result which yields undefined for ServiceNow DELETE 204 responses;
update _makeRequest to detect a 204 No Content (response.status === 204) and
return a consistent value (e.g., an empty object or a standardized payload like
{ success: true } ) instead of undefined so deleteTableRecord and other callers
get a predictable result; modify the return logic inside _makeRequest to check
response.status and return the chosen consistent value for 204, otherwise return
response.result, and verify deleteTableRecord continues to call _makeRequest
unchanged.
♻️ Duplicate comments (1)
components/servicenow/servicenow.app.mjs (1)
12-22: Addsysparm_limitto prevent loading large option lists.The previous review feedback about adding
sysparm_limitto cap results fromsys_db_objecthas not been addressed. Large tables can degrade responsiveness.Proposed fix
const data = await this.getTableRecords({ table: "sys_db_object", params: { sysparm_query: `nameLIKE${query}^ORlabelLIKE${query}`, sysparm_fields: "name,label", + sysparm_limit: 50, }, });
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/servicenow/servicenow.app.mjs
🧰 Additional context used
🧬 Code graph analysis (1)
components/servicenow/servicenow.app.mjs (5)
components/servicenow/actions/get-table-records/get-table-records.mjs (2)
query(92-92)response(117-129)components/servicenow/actions/update-table-record/update-table-record.mjs (1)
response(71-84)components/servicenow/actions/create-table-record/create-table-record.mjs (1)
response(59-70)components/servicenow/actions/get-table-record-by-id/get-table-record-by-id.mjs (1)
response(60-71)components/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjs (1)
response(123-127)
⏰ 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: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (7)
components/servicenow/servicenow.app.mjs (7)
1-5: Imports and app definition look good.The
servicenow_oauth_app identifier is noted as temporary per PR description and will be migrated to the mainservicenowapp before merging.
31-35: LGTM!Clear and straightforward record ID definition.
36-90: LGTM!The prop definitions correctly map to ServiceNow Table API parameters (
sysparm_display_value,sysparm_exclude_reference_link,sysparm_fields,sysparm_input_display_value,sysparm_view,sysparm_query_no_domain).
108-116: LGTM!Correctly implements POST to the Table API endpoint.
117-127: LGTM!Correctly uses PUT for full replacement and PATCH for partial updates per ServiceNow Table API semantics.
128-136: LGTM!Endpoint structure is correct. The potential
undefinedreturn value is addressed in the_makeRequestcomment above.
137-160: LGTM!All GET methods correctly implement their respective ServiceNow API endpoints:
/table/{table}/{recordId}for single record retrieval/table/{table}for record listing/stats/{table}for Aggregate API counts
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| async _makeRequest({ | ||
| $ = this, | ||
| headers, | ||
| ...args | ||
| }) { | ||
| const response = await axios($, { | ||
| baseURL: `https://${this.$auth.instance_name}.service-now.com/api/now`, | ||
| headers: { | ||
| ...headers, | ||
| "Authorization": `Bearer ${this.$auth.oauth_access_token}`, | ||
| }, | ||
| ...args, | ||
| }); | ||
| return response.result; | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for deleteTableRecord method
rg -A 10 "deleteTableRecord" components/servicenow/Repository: PipedreamHQ/pipedream
Length of output: 1901
🏁 Script executed:
#!/bin/bash
# Also search for any DELETE method calls or patterns
rg "method.*DELETE|DELETE.*method" components/servicenow/ -iRepository: PipedreamHQ/pipedream
Length of output: 131
🌐 Web query:
ServiceNow Table API DELETE request response 204 No Content result field
💡 Result:
Short answer: A successful Table API DELETE returns HTTP 204 No Content and therefore contains no JSON "result" field — 204 means the server intentionally returns an empty body. [1][2][3]
Notes / options:
- If you need confirmation/data back, call GET before/after delete or use an API that returns the deleted record (or return 200 with a body from a custom endpoint). [2][3]
- Client libraries (e.g., pysnow) treat 204 as success for DELETE and will not expect a "result" field. [1]
Sources:
- pysnow request handling (treats DELETE 204 as success). [turn0search4]. [1]
- ServiceNow Table API spec / examples showing DELETE -> 204 (no content). [turn0search2]. [2]
- HTTP RFC / Postman explanation of 204 semantics (no message body). [turn1search6][turn1search2]. [3]
🏁 Script executed:
#!/bin/bash
# Search for all files that reference deleteTableRecord
rg "deleteTableRecord" --type mjsRepository: PipedreamHQ/pipedream
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Check what the other methods in servicenow.app.mjs return
rg -B 2 -A 8 "async (createTableRecord|updateTableRecord|getTableRecordById)" components/servicenow/servicenow.app.mjsRepository: PipedreamHQ/pipedream
Length of output: 750
🏁 Script executed:
#!/bin/bash
# Search for all deleteTableRecord usages across the repo
rg "deleteTableRecord" -A 2 -B 2Repository: PipedreamHQ/pipedream
Length of output: 835
🏁 Script executed:
#!/bin/bash
# Also check the delete-table-record action completely
cat -n components/servicenow/actions/delete-table-record/delete-table-record.mjsRepository: PipedreamHQ/pipedream
Length of output: 1687
Address inconsistent return behavior for DELETE operations.
The _makeRequest method always returns response.result, but ServiceNow's DELETE endpoint returns 204 No Content with no response body. This causes deleteTableRecord to return undefined, unlike other CRUD methods which return data.
While the current action doesn't depend on the return value, this creates an inconsistency in the API pattern that could confuse future callers or lead to issues if other code assumes deleteTableRecord returns a truthy value.
Proposed fix
async _makeRequest({
$ = this,
headers,
...args
}) {
const response = await axios($, {
baseURL: `https://${this.$auth.instance_name}.service-now.com/api/now`,
headers: {
...headers,
"Authorization": `Bearer ${this.$auth.oauth_access_token}`,
},
...args,
});
- return response.result;
+ return response?.result ?? response;
},🤖 Prompt for AI Agents
In `@components/servicenow/servicenow.app.mjs` around lines 93 - 107, The
_makeRequest currently unconditionally returns response.result which yields
undefined for ServiceNow DELETE 204 responses; update _makeRequest to detect a
204 No Content (response.status === 204) and return a consistent value (e.g., an
empty object or a standardized payload like { success: true } ) instead of
undefined so deleteTableRecord and other callers get a predictable result;
modify the return logic inside _makeRequest to check response.status and return
the chosen consistent value for 204, otherwise return response.result, and
verify deleteTableRecord continues to call _makeRequest unchanged.
|
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
🤖 Fix all issues with AI agents
In `@components/servicenow/servicenow.app.mjs`:
- Around line 13-16: The options method currently uses console.log for user
guidance which is not visible in the Pipedream UI; replace the console.log and
empty-array return with a user-facing placeholder option when the query is empty
or too short (e.g., return [{ label: "Please input a search term", value: "" }])
so the dropdown shows guidance; update the conditional in the same method (the
block that checks query?.length > 1) to return that placeholder option instead
of logging to console.
♻️ Duplicate comments (2)
components/servicenow/servicenow.app.mjs (2)
12-23: Addsysparm_limitto prevent large result sets.The
sys_db_objecttable can contain thousands of entries. Without a limit, this query may return an excessively large option list, degrading performance and UX.Proposed fix
const data = await this.getTableRecords({ table: "sys_db_object", params: { sysparm_query: `nameLIKE${query}^ORlabelLIKE${query}`, sysparm_fields: "name,label", + sysparm_limit: 50, }, });
94-108: Handle DELETE204 No Contentresponses consistently.ServiceNow's DELETE endpoint returns
204 No Contentwith no response body. The current implementation returnsresponse.result, which will beundefinedfor DELETE operations, creating an inconsistent return type across CRUD methods.Proposed fix
async _makeRequest({ $ = this, headers, ...args }) { const response = await axios($, { baseURL: `https://${this.$auth.instance_name}.service-now.com/api/now`, headers: { ...headers, "Authorization": `Bearer ${this.$auth.oauth_access_token}`, }, ...args, }); - return response.result; + return response?.result ?? response; },
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/servicenow/servicenow.app.mjs
🧰 Additional context used
🧬 Code graph analysis (1)
components/servicenow/servicenow.app.mjs (5)
components/servicenow/actions/get-table-records/get-table-records.mjs (2)
query(92-92)response(117-129)components/servicenow/actions/update-table-record/update-table-record.mjs (1)
response(71-84)components/servicenow/actions/create-table-record/create-table-record.mjs (1)
response(59-70)components/servicenow/actions/get-table-record-by-id/get-table-record-by-id.mjs (1)
response(60-71)components/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjs (1)
response(123-127)
⏰ 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 (4)
components/servicenow/servicenow.app.mjs (4)
32-91: LGTM!The propDefinitions are well-structured with appropriate types, labels, and descriptions that align with ServiceNow's Table API parameters.
109-134: LGTM!The
createTableRecordandupdateTableRecordmethods correctly implement ServiceNow Table API patterns with appropriate HTTP methods and Content-Type headers. Thereplaceparameter elegantly toggles between PUT and PATCH semantics.
135-151: LGTM!The
deleteTableRecordandgetTableRecordByIdmethods correctly construct the Table API URLs. The DELETE return value concern is addressed at the_makeRequestlevel.
152-167: LGTM!The
getTableRecordsandgetRecordCountsByFieldmethods correctly target the Table API (/table/{table}) and Aggregate API (/stats/{table}) endpoints respectively.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
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:
|
2 similar comments
|
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:
|
|
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:
|
Closes #19564
Note: components are currently using the
servicenow_oauth_app as it is configured with the proper authentication, so the components can be tested.This auth will be migrated to the main
servicenowapp right before we merge the PR, when I'll adjust the app slugs toservicenow.Summary by CodeRabbit
New Features
Refactor
Removed
✏️ Tip: You can customize this high-level summary in your review settings.