-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Mastodon - add pagination props to Get Accounts Following #19110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/mastodon/actions/get-accounts-following/get-accounts-following.mjs (1)
63-81: Issue:paginationLinksmay not correspond to the actual data returned whenmaxlimit is hit.The
paginationLinksvariable is updated inside the loop (line 70) and will contain the Link header from the last API call. When the loop exits becauseaccounts.length >= max, the results are trimmed to exactlymax(lines 79-81), butpaginationLinksstill points to the continuation of the last fetched page, not to where the trimmed data actually ends.Example:
max = 100- Fetch page 1: 80 accounts (total: 80)
- Fetch page 2: 80 accounts (total: 160),
paginationLinksset to page 2 links- Loop exits, trim to 100 accounts
- Return 100 accounts with
paginationLinksthat would start from account 161+, not 101Suggested fix:
SetpaginationLinkstonullor omit it when results are trimmed due to hitting themaxlimit, since the pagination links won't accurately represent the continuation point.if (accounts.length > max) { accounts.length = max; + paginationLinks = null; // Links don't correspond to trimmed data }Also applies to: 84-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/mastodon/actions/get-accounts-following/get-accounts-following.mjs(5 hunks)components/mastodon/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-08T15:33:38.240Z
Learnt from: LucBerge
Repo: PipedreamHQ/pipedream PR: 14080
File: components/nocodb/nocodb.app.mjs:133-133
Timestamp: 2024-10-08T15:33:38.240Z
Learning: When implementing pagination with an offset, incrementing `args.params.offset` within the loop ensures correct tracking of the offset, particularly when a maximum count limit (`max`) is used.
Applied to files:
components/mastodon/actions/get-accounts-following/get-accounts-following.mjs
🔇 Additional comments (4)
components/mastodon/package.json (1)
3-3: LGTM: Version bump aligns with pagination feature addition.The patch version bump from 0.2.1 to 0.2.2 is appropriate for adding optional pagination props to the get-accounts-following action.
components/mastodon/actions/get-accounts-following/get-accounts-following.mjs (3)
7-7: LGTM: Version bump is appropriate.Patch version increment to 0.0.3 correctly reflects the addition of optional pagination props.
84-87: Note: This is a breaking change for existing consumers.The return value has changed from an array of accounts to an object containing
{ accounts, paginationLinks }. Existing workflows or code that consume this action will need to be updated to access.accountsinstead of using the return value directly.Ensure this breaking change is:
- Clearly documented in release notes or migration guides
- Communicated to users who may be using this action
- Justified by the pagination enhancement value
Additionally, ensure the
paginationLinksaccuracy issues identified in the previous comment are resolved.
22-39: The Mastodon API explicitly supports using these parameters together.Since Mastodon v3.3.0, min_id and max_id can be used together on many endpoints. The parameters define boundaries, not conflicts: max_id returns results with ID < max_id (older items), while min_id returns results immediately newer than that ID. Using them together creates a valid range query with no conflicting constraints.
Likely an incorrect or invalid review comment.
components/mastodon/actions/get-accounts-following/get-accounts-following.mjs
Show resolved
Hide resolved
luancazarine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @michelle0927, LGTM! Ready for QA!
|
Hi everyone, all test cases are passed! Ready for release! Test reports
|
Adds prop
maxId,minId, andsinceIdto help support pagination of large data sets.Followup to #19014
Summary by CodeRabbit