[Components] Topdesk - new components #18844
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds a Topdesk integration: app module with HTTP/auth helpers, resource methods and pagination, package version bump, and six new actions for creating, retrieving, listing, and updating incidents plus listing knowledge items and statuses. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Action
participant App
participant TopdeskAPI
User->>Action: Trigger action with props
Action->>Action: Normalize props, build id-wrapped fields
alt Paginated list actions
Action->>App: app.paginate({ fn: listX, fnArgs, maxResults })
loop pages
App->>TopdeskAPI: GET /... (page_size, start, params)
TopdeskAPI-->>App: paged items + page info
App-->>Action: yield page items
end
else Single-record actions
Action->>App: app.createIncident / app.getIncident / app.updateIncident
App->>TopdeskAPI: POST/GET/PATCH (payload, auth)
TopdeskAPI-->>App: response
App-->>Action: parsed response
end
Action->>Action: $.export("$summary", ...)
Action-->>User: return response / items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
jcortes
force-pushed
the
topdesk-new-components
branch
from
7599331 to
3426e13
Compare
![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 (10)
components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs (1)
30-41: Harden summary count against response shape differencesIf
listKnowledgeItemStatusesreturns an array (not{ results }), the count will read 0. Make the summary resilient.Apply:
- const statusCount = response.results?.length || 0; + const statusCount = Array.isArray(response) + ? response.length + : (response?.results?.length ?? 0);components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs (2)
80-84: Add idempotent hint for read-only actionThis action performs a GET; mark it as idempotent for consistency with other read actions.
annotations: { readOnlyHint: true, destructiveHint: false, openWorldHint: true, + idempotentHint: true, },
100-108: Field normalization is solid; minor cleanup optionalYou already accept both
string[]andstring. Consider extracting this join-to-string logic into a small helper intopdesk.app.mjsto DRY across actions (incidents uses same pattern).components/topdesk/actions/get-incidents/get-incidents.mjs (1)
67-74: Fields input type vs normalizationProp
fieldsis typed asstring(comma-separated), but runtime also supportsstring[]. Either keep as-is (tolerant) or switch prop type tostring[]withoptionsfor a curated list (like knowledge items) for better UX.components/topdesk/actions/create-incident/create-incident.mjs (2)
20-30: Enforce partial incident requirementsIf status “partial” is selected, Topdesk requires a mainIncident reference. Add a simple guard to prevent invalid payloads.
async run({ $ }) { const { app, callerLookupId, status, + // ... mainIncidentId, // ... } = this; + if (status === "partial" && !mainIncidentId) { + throw new Error("Main Incident ID is required when status is 'partial'."); + }Also applies to: 103-108
293-371: Deduplicate id-wrapping logic across actionsThis idFields → reduce pattern is duplicated in update-incident. Extract a small helper to keep both in sync.
+// helpers/id.js (new) +export const wrapIds = (pairs = []) => + pairs.reduce((acc, { value, key }) => value ? { ...acc, [key]: { id: value } } : acc, {});Then:
- ...idFields.reduce((acc, { value, key }) => ({ ...acc, ...value && { [key]: { id: value } } }), {}), + ...wrapIds(idFields),components/topdesk/actions/update-incident/update-incident.mjs (2)
17-22: Use propDefinition for caller lookup for consistent UXCreate action uses the app’s personId selector; this action takes a raw string. Unify to reuse dynamic options and labels.
- callerLookupId: { - type: "string", - label: "Caller Lookup ID", - description: "Lookup value for filling in a registered caller's contact details (UUID). Can only be set by operators.", - optional: true, - }, + callerLookupId: { + propDefinition: [app, "personId"], + label: "Caller Lookup ID", + description: "Lookup value for a registered caller (UUID). Can only be set by operators.", + optional: true, + },
392-429: Allow status update if API supports itIf the API allows updating status, expose a status prop and forward it like the create action does. Keeps parity for users moving an incident to second line, etc.
+ status: { + type: "string", + label: "Status", + optional: true, + options: ["firstLine","secondLine","partial"], + },And include in data.
- data: { + data: { + status, briefDescription,components/topdesk/topdesk.app.mjs (2)
37-61: Knowledge item option: description vs APIDescription says “UUID or number”, but app.getKnowledgeItem uses an ID path parameter. If numbers aren’t valid IDs, adjust description or implement number lookup via query.
139-166: Consider exposing GET helpers tooYou added post/patch convenience methods; adding get(opts) keeps call sites uniform.
post(opts = {}) { return this._makeRequest({ method: "POST", ...opts, }); }, + get(opts = {}) { + return this._makeRequest({ + method: "GET", + ...opts, + }); + }, patch(opts = {}) {
📜 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 (8)
components/topdesk/actions/create-incident/create-incident.mjs(1 hunks)components/topdesk/actions/get-incident/get-incident.mjs(1 hunks)components/topdesk/actions/get-incidents/get-incidents.mjs(1 hunks)components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs(1 hunks)components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs(1 hunks)components/topdesk/actions/update-incident/update-incident.mjs(1 hunks)components/topdesk/package.json(2 hunks)components/topdesk/topdesk.app.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
components/topdesk/actions/get-incident/get-incident.mjs (2)
components/topdesk/actions/create-incident/create-incident.mjs (1)
response(373-405)components/topdesk/actions/update-incident/update-incident.mjs (1)
response(392-429)
components/topdesk/actions/create-incident/create-incident.mjs (1)
components/topdesk/actions/update-incident/update-incident.mjs (2)
idFields(317-390)response(392-429)
components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs (4)
components/topdesk/actions/create-incident/create-incident.mjs (1)
response(373-405)components/topdesk/actions/get-incident/get-incident.mjs (1)
response(30-33)components/topdesk/actions/update-incident/update-incident.mjs (1)
response(392-429)components/topdesk/topdesk.app.mjs (2)
response(42-47)response(209-216)
components/topdesk/actions/update-incident/update-incident.mjs (1)
components/topdesk/actions/create-incident/create-incident.mjs (2)
idFields(294-371)response(373-405)
components/topdesk/actions/get-incidents/get-incidents.mjs (2)
components/topdesk/topdesk.app.mjs (1)
incidents(16-21)components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs (1)
paginator(95-112)
components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs (2)
components/topdesk/topdesk.app.mjs (2)
items(48-48)items(219-223)components/topdesk/actions/get-incidents/get-incidents.mjs (1)
paginator(60-77)
components/topdesk/topdesk.app.mjs (6)
components/topdesk/actions/get-incidents/get-incidents.mjs (1)
incidents(59-59)components/topdesk/actions/create-incident/create-incident.mjs (1)
response(373-405)components/topdesk/actions/get-incident/get-incident.mjs (1)
response(30-33)components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs (1)
response(30-35)components/topdesk/actions/update-incident/update-incident.mjs (1)
response(392-429)components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs (1)
items(94-94)
⏰ 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: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (6)
components/topdesk/package.json (1)
3-17: Version bump and dependency look finePackage version and dependency addition align with new actions using platform helpers. No issues from this diff.
components/topdesk/actions/get-incident/get-incident.mjs (1)
24-37: LGTMProp definition and call pattern are consistent. Summary uses
response.id, which matches expected payloads.If
propDefinitions.incidentIdsupports search / dynamic options, ensure it handles archived incidents as needed.components/topdesk/actions/get-incidents/get-incidents.mjs (1)
83-85: Good summary and return shapeReturning the aggregated array and exporting a count summary matches other actions.
components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs (1)
24-41: Verify response structure consistency across endpointsThe code assumes
listKnowledgeItemStatusesreturns{ results: [...] }(accessingresponse.results?.length), but other similar list methods likelistOperatorsandlistPersonsare treated as direct arrays in their usage. This inconsistency warrants manual verification:
- Check if "Searchlists" endpoints (knowledgeItemStatuses) have a different contract than other list endpoints
- Confirm whether the Topdesk API actually returns
{ results: [...] }for this endpoint, or if it should return an array directly like other list methods- If it returns an array, the code should be simplified to access the array length directly without the
.resultspropertycomponents/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs (1)
73-78: No action required — language parameter is correctly implementedThe TOPdesk knowledge-base-v1 API supports filtering knowledge items by language using the language code, and the current implementation correctly passes the
languageparameter as a query parameter. The code will not produce silent no-ops; language filtering will work as intended.components/topdesk/actions/update-incident/update-incident.mjs (1)
6-8: PATCH is supported by TOPdesk API — no changes neededTOPdesk's Incident API supports both PATCH for partial updates and PUT for full replacement. The current implementation correctly uses PATCH at line 162 in
topdesk.app.mjs, called consistently from line 392 in the update-incident action. The documentation reference is also accurate.
| label: "Query", | ||
| description: `A FIQL query to filter the response. Use semicolons to combine multiple conditions. | ||
| **Available filter fields:** |
There was a problem hiding this comment.
I would suggest linking to the associated documentation page instead of listing all the filters here. It would keep the description more concise while also providing the information and guaranteeing it will be up-to-date.
jcortes
force-pushed
the
topdesk-new-components
branch
2 times, most recently
from
220a94c to
b0342a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
components/topdesk/actions/get-incidents/get-incidents.mjs (1)
59-77: Pagination param mismatch will break after first page.The
app.paginatehelper usesstart/page_sizeparameters (line 73), butapp.listIncidentsfield options in topdesk.app.mjs (lines 15-20) usepageStart/pageSize. This mismatch will cause pagination to stall after the first page since the paginator will pass parameters that don't match what the API expects.Apply this diff to use the correct parameter name:
- page_size: 100, + pageSize: 100,Alternatively, update
listIncidentsand the paginate helper to consistently usestart/page_sizeacross all endpoints (see the broader paginator fix needed in topdesk.app.mjs).components/topdesk/topdesk.app.mjs (3)
11-16: Destructuring undefinedprevContextwill throw.Line 12 destructures
pageStartfromprevContext, butprevContextcan beundefinedon the first call, causing a runtime exception.Apply this diff to safely access
prevContext:async options({ prevContext }) { - const { pageStart } = prevContext; + const pageStart = prevContext?.pageStart ?? 0; if (pageStart === null) { return []; }
86-101:personIdoptions returns array instead of pagination object.The
optionsfunction returns an array (lines 97-100), which prevents pagination from advancing. The Pipedream platform expects{ options, context }to support paginated dropdowns.Apply this diff to return the correct pagination structure:
async options({ prevContext }) { const persons = await this.listPersons({ params: { start: prevContext?.start || 0, page_size: 100, }, }); - return persons.map((person) => ({ - label: `${person.firstName} ${person.surName}`, - value: person.id, - })) || []; + return { + options: (persons || []).map((person) => ({ + label: `${person.firstName} ${person.surName}`, + value: person.id, + })), + context: { + start: persons?.length === 100 + ? (prevContext?.start || 0) + 100 + : null, + }, + }; },
654-705: Paginator skips the first page and has parameter naming inconsistencies.Line 668 sends
start: start + (fnArgs.params?.page_size || 100)on the first call. Sincestartis initialized to 0 (line 661), the first request usesstart=100, skipping the first page. Line 703 then incrementsstartagain, skipping subsequent pages as well.Additionally, the paginator hardcodes
start/page_sizeparameters, but several endpoints (listIncidents,listAssets) usepageStart/pageSizeinstead, causing pagination to fail for those endpoints.Apply this diff to fix both issues:
async *paginate({ fn, fnArgs = {}, maxResults = 600, dataField, }) { let resourcesCount = 0; - let start = 0; + const pageSize = fnArgs.params?.page_size ?? fnArgs.params?.pageSize ?? 100; + let start = fnArgs.params?.start ?? fnArgs.params?.pageStart ?? 0; + const startParam = fnArgs.params?.pageStart !== undefined ? "pageStart" : "start"; + const sizeParam = fnArgs.params?.pageSize !== undefined ? "pageSize" : "page_size"; - while (true) { + for (;;) { const response = await fn({ ...fnArgs, params: { ...fnArgs.params, - start: start + (fnArgs.params?.page_size || 100), - page_size: (fnArgs.params?.page_size || 100), + [startParam]: start, + [sizeParam]: pageSize, }, }); // Extract items from response based on dataField or use response directly const items = dataField - ? (response[dataField] || []) + ? (response?.[dataField] ?? []) : (Array.isArray(response) ? response : []); if (!items.length) { console.log("No items found"); return; } for (const item of items) { yield item; resourcesCount++; if (maxResults && resourcesCount >= maxResults) { console.log("Reached max results"); return; } } - const hasNextPage = response.next || (items.length === 100); + const hasNextPage = Boolean(response?.next) || (items.length === pageSize); if (!hasNextPage) { console.log("No more pages found"); return; } // Auto-increment pagination parameters - start += (fnArgs.params?.page_size || 100); + start += pageSize; } },
🧹 Nitpick comments (1)
components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs (1)
51-72: Consider condensing the filter documentation.The description lists all available filter fields in detail (22 lines). This could be more concise by primarily referencing the documentation link at line 72, while keeping a brief summary of common filters.
Example refactor:
description: `A FIQL query to filter the response. Use semicolons to combine multiple conditions. -**Available filter fields:** -- \`parent.id\`, \`parent.number\` -- \`visibility.sspVisibility\`, \`visibility.sspVisibleFrom\`, \`visibility.sspVisibleUntil\` -- \`visibility.sspVisibilityFilteredOnBranches\`, \`visibility.operatorVisibilityFilteredOnBranches\` -- \`visibility.publicKnowledgeItem\`, \`visibility.branches.id\`, \`visibility.branches.name\` -- \`manager.id\`, \`manager.name\` -- \`status.id\`, \`status.name\` -- \`standardSolution.id\`, \`standardSolution.name\` -- \`externalLink.id\`, \`externalLink.type\`, \`externalLink.date\` -- \`archived\`, \`news\` - -**Operators:** -- \`==\` (equals), \`!=\` (not equals) -- \`=gt=\` (greater than), \`=ge=\` (greater than or equal) -- \`=lt=\` (less than), \`=le=\` (less than or equal) -- \`=in=\` (in list), \`=out=\` (not in list) - **Example:** \`parent.id==3fa85f64-5717-4562-b3fc-2c963f66afa6;externalLink.id=in=(oneTool,otherTool)\` -[See the documentation](https://developers.topdesk.com/explorer/?page=knowledge-base#/Knowledge%20Items/getKnowledgeItems) for more information.`, +See the [FIQL documentation](https://developers.topdesk.com/explorer/?page=knowledge-base#/Knowledge%20Items/getKnowledgeItems) for available filter fields and operators.`,
📜 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 (8)
components/topdesk/actions/create-incident/create-incident.mjs(1 hunks)components/topdesk/actions/get-incident/get-incident.mjs(1 hunks)components/topdesk/actions/get-incidents/get-incidents.mjs(1 hunks)components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs(1 hunks)components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs(1 hunks)components/topdesk/actions/update-incident/update-incident.mjs(1 hunks)components/topdesk/package.json(2 hunks)components/topdesk/topdesk.app.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- components/topdesk/package.json
- components/topdesk/actions/create-incident/create-incident.mjs
- components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs
- components/topdesk/actions/get-incident/get-incident.mjs
🧰 Additional context used
🧬 Code graph analysis (4)
components/topdesk/actions/get-incidents/get-incidents.mjs (2)
components/topdesk/topdesk.app.mjs (1)
incidents(16-21)components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs (1)
paginator(97-114)
components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs (2)
components/topdesk/topdesk.app.mjs (4)
items(48-48)items(110-110)items(133-133)items(674-678)components/topdesk/actions/get-incidents/get-incidents.mjs (1)
paginator(60-77)
components/topdesk/actions/update-incident/update-incident.mjs (1)
components/topdesk/actions/create-incident/create-incident.mjs (2)
idFields(297-370)response(372-409)
components/topdesk/topdesk.app.mjs (4)
components/topdesk/actions/create-incident/create-incident.mjs (1)
response(372-409)components/topdesk/actions/get-incident/get-incident.mjs (1)
response(30-33)components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs (1)
response(30-35)components/topdesk/actions/update-incident/update-incident.mjs (1)
response(394-436)
⏰ 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: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (1)
components/topdesk/actions/update-incident/update-incident.mjs (1)
322-436: Well-structured data transformation logic.The idFields array and reduce pattern (lines 322-434) cleanly handles optional ID-based relations by conditionally wrapping values in
{ id: value }objects. This approach is consistent with the create-incident action and keeps the payload construction maintainable.
jcortes
force-pushed
the
topdesk-new-components
branch
from
b0342a0 to
0d30cec
Compare
|
/approve |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
components/topdesk/topdesk.app.mjs (3)
11-21: Crash risk: prevContext may be undefinedDestructuring
{ pageStart } = prevContextthrows when prevContext is undefined. Use optional chaining and a default.- async options({ prevContext }) { - const { pageStart } = prevContext; + async options({ prevContext }) { + const pageStart = prevContext?.pageStart ?? 0; if (pageStart === null) { return []; } const incidents = await this.listIncidents({ params: { pageStart, pageSize: 100, }, });Also consider returning
{ options, context }even whenpageStart === nullfor consistency.
90-101: Options must return { options, context } for paginationThis returns an array, breaking pagination and consistency with other fields. Return the object shape and advance start.
- async options({ prevContext }) { + async options({ prevContext }) { const persons = await this.listPersons({ params: { start: prevContext?.start || 0, page_size: 100, }, }); - return persons.map((person) => ({ - label: `${person.firstName} ${person.surName}`, - value: person.id, - })) || []; + return { + options: (persons || []).map((p) => ({ + label: `${p.firstName} ${p.surName}`, + value: p.id, + })), + context: { + start: (persons?.length === 100) + ? (prevContext?.start || 0) + 100 + : null, + }, + }; },
654-704: Paginator skips first page and assumes one param namingCurrent logic requests
start + page_sizeon first call and only supportsstart/page_size. Fix to (a) send the current cursor, (b) auto-detectstart/page_sizevspageStart/pageSize, and (c) use the actual page size for hasNext.- async *paginate({ - fn, - fnArgs = {}, - maxResults = 600, - dataField, - }) { - let resourcesCount = 0; - let start = 0; - - while (true) { - const response = await fn({ - ...fnArgs, - params: { - ...fnArgs.params, - start: start + (fnArgs.params?.page_size || 100), - page_size: (fnArgs.params?.page_size || 100), - }, - }); + async *paginate({ + fn, + fnArgs = {}, + maxResults = 600, + dataField, + // auto-detect param keys from provided args + startParam = (fnArgs.params?.pageStart !== undefined) ? "pageStart" : "start", + sizeParam = (fnArgs.params?.pageSize !== undefined) ? "pageSize" : "page_size", + }) { + let resourcesCount = 0; + const size = fnArgs.params?.[sizeParam] ?? 100; + let start = fnArgs.params?.[startParam] ?? 0; + + for (;;) { + const response = await fn({ + ...fnArgs, + params: { + ...fnArgs.params, + [startParam]: start, + [sizeParam]: size, + }, + }); - const items = dataField - ? (response[dataField] || []) - : (Array.isArray(response) - ? response - : []); + const items = dataField + ? (response?.[dataField] ?? []) + : (Array.isArray(response) ? response : []); if (!items.length) { console.log("No items found"); return; } for (const item of items) { yield item; resourcesCount++; if (maxResults && resourcesCount >= maxResults) { console.log("Reached max results"); return; } } - const hasNextPage = response.next || (items.length === 100); + const hasNextPage = Boolean(response?.next) || (items.length === size); if (!hasNextPage) { console.log("No more pages found"); return; } - // Auto-increment pagination parameters - start += (fnArgs.params?.page_size || 100); + start += size; } },components/topdesk/actions/create-incident/create-incident.mjs (1)
11-19: Clarify requiredness and copy for callerLookupIdThe description mentions an alternative “manual caller details” path that isn’t implemented. Make the field explicitly required or update the copy.
callerLookupId: { type: "string", label: "Caller Lookup ID", - description: "Lookup value for filling in a registered caller's contact details (UUID). **Required** - you must specify either this field or provide caller details manually.", + description: "Lookup value for a registered caller (UUID).", + optional: false, propDefinition: [ app, "personId", ], },If you intend to support manual caller input, add those props and validate exactly one path is provided.
components/topdesk/actions/get-incidents/get-incidents.mjs (1)
60-77: Fix paginator param mismatch and first-page skipYou pass page_size, but incidents endpoints elsewhere use pageStart/pageSize, and app.paginate currently skips the first page. Align keys and seed pageStart to guide the paginator.
Apply this diff here (pairs with the paginator fix in app.mjs):
- fnArgs: { + fnArgs: { $, params: { sort, query, - fields: Array.isArray(fields) && fields?.length - ? fields.join(",") - : typeof fields === "string" && fields.length - ? fields - : undefined, + fields: typeof fields === "string" && fields.length ? fields : undefined, all, - page_size: 100, + pageStart: 0, + pageSize: 100, }, },
🧹 Nitpick comments (8)
components/topdesk/actions/get-incidents/get-incidents.mjs (1)
29-34: Prop type doesn’t match runtime usagefields is declared string, but the code branches for arrays. Either change the prop to a string[] or simplify the mapper. Since the mapper is simplified above, consider also updating the prop to avoid confusion.
- fields: { - type: "string", + fields: { + type: "string", label: "Fields", description: "A comma-separated list of which fields should be returned. By default all fields will be returned.", optional: true, },(If you want multi-select later, switch to
type: "string[]"and keep the join logic.)components/topdesk/actions/create-incident/create-incident.mjs (2)
254-294: Validate partial incident requirementIf status is "partial", mainIncidentId should be required. Add a minimal guard before building the payload.
async run({ $ }) { const { app, callerLookupId, status, ... optionalFields2, } = this; + if (status === "partial" && !mainIncidentId) { + throw new Error("mainIncidentId is required when status is 'partial'."); + }
372-409: Prefer stable identifiers for object selectionobjectName maps to payload object.name. Names may be non-unique. Consider switching to an objectId prop and map
{ object: { id } }for reliability. If keeping names, ensure UI options clearly indicate duplicates.components/topdesk/topdesk.app.mjs (4)
262-276: Locations options aren’t paginated; add cursorOnly pageSize is set; you’ll list at most 100. Wire prevContext and return
{ options, context }.- async options() { - const response = await this.listLocations({ - params: { - pageSize: 100, - }, - }); - return response.map((item) => ({ + async options({ prevContext }) { + const response = await this.listLocations({ + params: { + pageStart: prevContext?.pageStart || 0, + pageSize: 100, + }, + }); + const options = response.map((item) => ({ label: item.name ? `${item.name}${item.roomNumber ? ` - ${item.roomNumber}` : ""}` : item.roomNumber || item.id, value: item.id, })); + return { + options, + context: { + pageStart: response.length === 100 + ? (prevContext?.pageStart || 0) + 100 + : null, + }, + }; },
346-363: Return labeled options for assets (objectName)Returning bare strings loses ids and label/value separation. Return objects for consistency; if you must use name as value, do so explicitly.
- const assets = response.dataSet || []; - return { - options: assets.map(({ name }) => name), + const assets = response.dataSet || []; + return { + options: assets.map(({ id, name }) => ({ + label: name, + value: name, // consider `value: id` if moving to object.id in payload + })),
124-137: Quote FIQL values to avoid parsing surprisesWhen building FIQL
parent.id==${categoryId}, consider quoting/escaping the value to handle special chars.- query: `parent.id==${categoryId}`, + query: `parent.id=="${categoryId}"`,
470-491: Safer URL joinIf api_url ends with a slash, you may end up with double slashes. Minor, but URL() avoids this.
- getUrl(path) { - return `${this.$auth.api_url}${path}`; - }, + getUrl(path) { + const base = this.$auth.api_url?.replace(/\/+$/, "") || ""; + return `${base}${path}`; + },components/topdesk/actions/update-incident/update-incident.mjs (1)
323-434: Consider extracting shared idFields logic.The idFields array construction (lines 323-392) and the reduce operation (lines 425-434) are nearly identical to the create-incident action. Consider extracting this pattern into a shared helper function in the app module to reduce duplication and improve maintainability.
Example approach - add to
topdesk.app.mjs:buildIdFields(fields) { return fields.reduce((acc, { value, key }) => ({ ...acc, ...(value && { [key]: { id: value }, }), }), {}); }Then in both actions:
const idFieldsMap = [ { value: callerLookupId, key: "callerLookup" }, { value: categoryId, key: "category" }, // ... other fields ]; const response = await app.updateIncident({ $, incidentId, data: { // ... scalar fields ...app.buildIdFields(idFieldsMap), }, });
📜 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 (8)
components/topdesk/actions/create-incident/create-incident.mjs(1 hunks)components/topdesk/actions/get-incident/get-incident.mjs(1 hunks)components/topdesk/actions/get-incidents/get-incidents.mjs(1 hunks)components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs(1 hunks)components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs(1 hunks)components/topdesk/actions/update-incident/update-incident.mjs(1 hunks)components/topdesk/package.json(2 hunks)components/topdesk/topdesk.app.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs
- components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs
🧰 Additional context used
🧬 Code graph analysis (5)
components/topdesk/actions/create-incident/create-incident.mjs (2)
components/topdesk/actions/update-incident/update-incident.mjs (2)
idFields(323-392)response(394-436)components/topdesk/topdesk.app.mjs (13)
response(42-47)response(109-109)response(128-132)response(146-146)response(159-159)response(172-172)response(185-185)response(198-198)response(211-211)response(224-224)response(237-237)response(250-250)response(263-267)
components/topdesk/actions/update-incident/update-incident.mjs (1)
components/topdesk/actions/create-incident/create-incident.mjs (2)
idFields(297-370)response(372-409)
components/topdesk/actions/get-incidents/get-incidents.mjs (2)
components/topdesk/topdesk.app.mjs (1)
incidents(16-21)components/topdesk/actions/get-knowledge-items/get-knowledge-items.mjs (1)
paginator(97-114)
components/topdesk/topdesk.app.mjs (5)
components/topdesk/actions/get-incidents/get-incidents.mjs (1)
incidents(59-59)components/topdesk/actions/create-incident/create-incident.mjs (1)
response(372-409)components/topdesk/actions/get-incident/get-incident.mjs (1)
response(30-33)components/topdesk/actions/get-knowledge-item-statuses/get-knowledge-item-statuses.mjs (1)
response(30-35)components/topdesk/actions/update-incident/update-incident.mjs (1)
response(394-436)
components/topdesk/actions/get-incident/get-incident.mjs (2)
components/topdesk/actions/create-incident/create-incident.mjs (1)
response(372-409)components/topdesk/actions/update-incident/update-incident.mjs (1)
response(394-436)
⏰ 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). (5)
- GitHub Check: Ensure component commits modify component versions
- GitHub Check: Lint Code Base
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
🔇 Additional comments (6)
components/topdesk/package.json (1)
3-17: Package metadata looks goodVersion bump and dependency entry are consistent with added components.
components/topdesk/actions/get-incident/get-incident.mjs (1)
24-37: LGTMStraightforward get by ID action using app surface; summary export is helpful.
components/topdesk/actions/update-incident/update-incident.mjs (4)
3-8: LGTM!The action metadata is well-structured with appropriate documentation reference.
270-275: LGTM!The annotations accurately reflect the characteristics of an update operation, correctly marking it as non-idempotent.
276-441: Logic is correct and follows consistent patterns.The run method correctly constructs the update payload and handles the response appropriately.
9-269: Let me check the TopDesk API documentation reference in create-incident and search more thoroughly:Let me view both action files completely to make a final determination:
The review comment contains factually incorrect claims:
mainIncidentId does not exist: This field is not present in the app's propDefinitions, nor is it supported by the TopDesk API. It cannot be used in either create or update operations.
Status field is available as processingStatusId: The update-incident action includes the
processingStatusIdfield from propDefinitions, which is how status updates are handled in this implementation. The create-incident action uses a custom inline "status" field, but both approaches manage incident status through the TopDesk API.The field differences between create and update are intentional, not an oversight. No verification against API documentation is needed—the code correctly implements the available fields.
Likely an incorrect or invalid review comment.
| all: { | ||
| type: "boolean", | ||
| label: "All", | ||
| description: "When set to true, will return all incidents including partials and archived. Otherwise only firstLine and secondLine incidents are returned.", | ||
| optional: true, | ||
| default: false, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm the ‘all’ filter name
Is all a valid query flag for GET /incidents? If the API expects different flags (e.g., archived/partials), adjust accordingly.
🌐 Web query:
TOPdesk incidents GET query parameters: which param(s) include archived and partial incidents? Is there an `all` flag?
💡 Result:
Short answer:
- To include archived incidents use the archived query parameter (e.g. &archived=true). [1][2]
- To include partial incidents filter by status (status=partial) or use a FIQL query (the incidents API supports status values like firstLine, secondLine, partial). [2][3]
- There is no single global "all" flag — omit filters or combine the appropriate parameters/queries (completed, closed, resolved, archived, status / FIQL) to get the set you want. [1][2]
Sources:
- TOPdesk docs — Retrieving a list / card selection (incidents endpoint usage).
- TOPdeskPS / TOPdesk API examples (Get‑TdIncident shows archived, completed, closed, resolved and status/partial usage).
- TOPdesk integrations / FIQL notes (incidents support FIQL and status filtering).
Remove invalid 'all' parameter — TOPdesk API doesn't support it
The TOPdesk incidents API has no single all flag. To include archived incidents, use archived=true. For partial incidents, filter by status=partial or use FIQL queries. Refactor this parameter to use the correct API-supported parameters instead.
🤖 Prompt for AI Agents
In components/topdesk/actions/get-incidents/get-incidents.mjs around lines 35 to
41, remove the invalid "all" parameter and replace it with API-supported
parameters: add an optional boolean "archived" (maps to archived=true when set)
and an optional "status" (enum or string) or a "fiql" string parameter to allow
filtering partial incidents (e.g., status=partial or FIQL queries). Update the
parameter labels/descriptions to explain their mapping to TOPdesk query params
and ensure defaults reflect current behavior (archived default false); wire
these new params into the request builder so they are translated into the
correct query string parameters instead of using a non-existent "all" flag.
| async options() { | ||
| const response = await this.listSLAs(); | ||
| return response.results?.map((item) => ({ | ||
| label: item.name, | ||
| value: item.id, | ||
| })); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
SLAs response shape likely inconsistent
You index response.results?.map, while other list* methods return arrays. Make this resilient to either shape.
- const response = await this.listSLAs();
- return response.results?.map((item) => ({
+ const response = await this.listSLAs();
+ const items = Array.isArray(response) ? response : (response?.results || []);
+ return items.map((item) => ({
label: item.name,
value: item.id,
}));🌐 Web query:
TOPdesk endpoint `/tas/api/incidents/slas`: does it return an array or an object with `results`? Provide example.
💡 Result:
I can check the official TOPdesk docs or test your instance and give an exact example — which would you prefer?
I need to search for official TOPdesk API documentation to determine the actual response format for the SLAs endpoint.
Let me check the codebase to understand the actual implementation of listSLAs and how it compares to other list methods.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find listSLAs and other list methods to understand response patterns
rg -n "listSLAs|list.*\(\)" components/topdesk/topdesk.app.mjs | head -20Length of output: 663
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Get listSLAs implementation
sed -n '606,650p' components/topdesk/topdesk.app.mjsLength of output: 1084
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check response handling patterns in options() methods
sed -n '108,115p' components/topdesk/topdesk.app.mjs
echo "---"
sed -n '145,152p' components/topdesk/topdesk.app.mjs
echo "---"
sed -n '171,178p' components/topdesk/topdesk.app.mjs
echo "---"
sed -n '223,230p' components/topdesk/topdesk.app.mjsLength of output: 906
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check more response handling patterns
sed -n '197,204p' components/topdesk/topdesk.app.mjs
echo "---"
sed -n '210,217p' components/topdesk/topdesk.app.mjs
echo "---"
sed -n '236,243p' components/topdesk/topdesk.app.mjs
echo "---"
sed -n '249,256p' components/topdesk/topdesk.app.mjs
echo "---"
sed -n '283,290p' components/topdesk/topdesk.app.mjsLength of output: 1032
Add defensive handling for listSLAs response shape
The codebase confirms inconsistency: 6 list methods expect bare arrays while listSLAs expects response.results. Since most endpoints return arrays directly, implement the suggested fix to handle both shapes:
const response = await this.listSLAs();
+ const items = Array.isArray(response) ? response : (response?.results || []);
- return response.results?.map((item) => ({
+ return items.map((item) => ({
label: item.name,
value: item.id,
}));This aligns with the exception handling seen in listCategories (which uses response.item || []) and makes the code resilient to the actual API response format.
📝 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() { | |
| const response = await this.listSLAs(); | |
| return response.results?.map((item) => ({ | |
| label: item.name, | |
| value: item.id, | |
| })); | |
| }, | |
| async options() { | |
| const response = await this.listSLAs(); | |
| const items = Array.isArray(response) ? response : (response?.results || []); | |
| return items.map((item) => ({ | |
| label: item.name, | |
| value: item.id, | |
| })); | |
| }, |
🤖 Prompt for AI Agents
In components/topdesk/topdesk.app.mjs around lines 223 to 229, the options()
implementation assumes listSLAs() returns an object with response.results which
is inconsistent with most list methods that return bare arrays; update the code
to defensively accept both shapes by extracting an array via response.results ||
response || [] and then map that array to {label: item.name, value: item.id},
ensuring it falls back to an empty array when neither is present.
WHY
Resolves #18806
Summary by CodeRabbit
New Features
Chores