ConnectWise PSA - efficient pagination #18623
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds lastId-aware pagination to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Trigger as Source Trigger
participant Source as Base Source
participant App as connectwise_psa.app.mjs
participant API as ConnectWise PSA API
Trigger->>Source: processEvent()
Source->>Source: getParams() → { pageSize:500, orderBy:"id desc" }
Source->>App: paginate({ resourceFn, params, max, lastId })
loop fetch pages
App->>API: resourceFn(params + paging)
API-->>App: items (id desc)
App->>App: for each item: if item.id > lastId → yield
App-->>Source: yielded newer items
end
Source->>Source: firstNewId = id of first yielded item
Source->>Source: collect items
alt any new items
Source->>Source: _setLastId(firstNewId)
end
Source->>Trigger: emit collected items reversed with generateMeta
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/connectwise_psa/connectwise_psa.app.mjs (1)
342-342: Add a default value for thelastIdparameter.The
lastIdparameter lacks a default value. Ifpaginateis called withoutlastId, the comparisonitem.id <= lastId(line 359) will evaluate toitem.id <= undefined, which is alwaysfalse, causing all items to pass through. Whilebase.mjsensureslastIdis at least0, other callers might not.Apply this diff to provide a sensible default:
- async *paginate({ - resourceFn, - params, - max, - lastId, - }) { + async *paginate({ + resourceFn, + params, + max, + lastId = 0, + }) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/connectwise_psa/connectwise_psa.app.mjs(2 hunks)components/connectwise_psa/sources/common/base.mjs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/connectwise_psa/connectwise_psa.app.mjs (1)
components/connectwise_psa/sources/common/base.mjs (1)
lastId(41-41)
components/connectwise_psa/sources/common/base.mjs (1)
components/connectwise_psa/connectwise_psa.app.mjs (1)
items(350-352)
⏰ 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 (5)
components/connectwise_psa/sources/common/base.mjs (4)
44-49: LGTM!The iterator creation correctly passes all required parameters (
resourceFn,params,max,lastId) to thepaginatefunction. The use of an iterator pattern is appropriate for handling potentially large result sets.
61-63: LGTM!The conditional update of
lastIdcorrectly ensures that the stored value is only updated when new items are found. This prevents overwriting the last known id with an undefined or null value.
65-67: LGTM!Reversing the items before emission ensures they are emitted in chronological order (oldest first), which aligns with typical event source patterns. The use of
generateMetaprovides consistent metadata for each emitted item.
28-31: pageSize of 500 is within API limits
500 is under the ConnectWise PSA API’s maximum of 1,000 records per request. No change required.components/connectwise_psa/connectwise_psa.app.mjs (1)
357-364: Guard against missing or invaliditem.idand verify result ordering
- Change
if (item.id <= lastId)toif (item.id == null || item.id <= lastId)to skip items without a validid.- Confirm the PSA API request uses
orderBy: "id desc"(or sort client-side) so the earlyreturncorrectly stops pagination.
| const items = []; | ||
| let firstNewId; | ||
|
|
||
| for await (const item of iterator) { | ||
| if (!firstNewId) { | ||
| firstNewId = item.id; | ||
| } | ||
| items.push(item); | ||
| } |
There was a problem hiding this comment.
Consider memory usage when max is undefined.
The code collects all items in memory before emitting them in reverse order. When max is not provided (e.g., in the run method), this could consume significant memory for large result sets. The paginate function has a lastId early-termination mechanism, but for sources with many new items, the in-memory collection could grow large.
Consider one of the following:
- Add a reasonable default
maxvalue in therunmethod to cap memory usage. - Document the memory implications for integrators who extend this base class.
- Stream items without collecting them if reverse order is not strictly necessary (though this would require design changes).
If the current behavior is intentional and memory usage is not a concern for typical use cases, consider adding a comment explaining the trade-off.
🤖 Prompt for AI Agents
In components/connectwise_psa/sources/common/base.mjs around lines 51-59 the
code collects all iterator items into memory before reversing and emitting them,
which can cause unbounded memory growth when max is undefined; update the
run/paginate usage to enforce a sensible default cap (e.g., default max = 1000)
when callers don't provide max, or if changing callers is undesirable, add an
explicit comment in this file explaining the memory trade-off and that callers
must supply max for large result sets; implement the default by checking for
undefined max at the entry point and limiting iteration or early-terminating
when the cap is reached, and document the behavior in the method docstring.
WHY
Summary by CodeRabbit
Bug Fixes
Performance
Improvements
Chores