Feature: Grant followers modal #3505
Conversation
github-actions
bot
added
enhancement
QA Summary
Test CoverageCoverage report for `packages/client`
Coverage report for `packages/server`
|
Terraform Summary
Hint: If "Terraform Format & Style" failed, run OutputValidation OutputPlan Summary
Pusher: @greg-adams, Action: |
There was a problem hiding this comment.
@greg-adams I have two visual formatting suggestions (see comments in GrantFollowers.vue) for you to consider.
A bit more broadly, I don't think we need the changes to pagination behavior that this PR introduces. For clarification:
- The intended ordering behavior for the modal's list is most-recently-followed first (latest follower at the top, first follower at the bottom), not alphabetical.
- I'd like to generally move the API towards cursor pagination rather than traditional offset-based pagination.
(As always, happy to chat about this further if you have questions or think I missed something.)
Additional context to elaborate on the preference for cursor-based pagination, in case you're interested – feel free to disregard if not :) ...
With cursor-based pagination, the database returns a cursor (or "checkpoint") value pointing to the final item in the result set, which is then used as an "exclusive start" filter on a subsequent query. This is in contrast to offset-based pagination, where the query specifies a certain number of results to skip, based on a running total result count. As long as clients treat the cursor value (pagination.from in our case) as an arbitrary string, it allows us to more easily implement a different checkpoint mechanism for a particular retrieval scenario (e.g. maybe we return from: grant_followers.created_at instead of from: grant_followers.id).
One advantage of cursor pagination is that it's more accurate – offset-based pagination has a tendency to either skip or return duplicate items when the overall result set is being modified between queries. For example, in a scenario where an array of Alice, Bob, Charlie, Elise, Frank, George is being paged 1 result at a time, if the query returns Frank and then Dan is inserted between Charlie and Elise, an offset-based paginated request would return Frank a second time. A cursor-based paginated request would ensure that George (or another value alphabetically greater than Frank) always comes next.
Another advantage is that they tend to be more compatible with non-SQL backends, so it offers a certain amount of future-proofing in case we decide to move to an ordered result cache or other kind of precomputed index.
Note: Although we don't currently have a use-case for ordering followers alphabetically, it is possible to implement cursor-based pagination in a way that supports multiple ordering schemes – the paginate.from value would just need to be set to the ORDER BY column value of the last item in the page. We would also need to support an afterName parameter in getFollowersForGrant(), or else refactor to a more generic startAfter parameter.
|
Ok gotcha, this makes sense, thanks for the background. The story AC requests sorting alphabetically by name, if we don't need this change I can return to ordering by creation. While making this change I added support on the response to identify a final result set ( |
Yeah, I just noticed that; we'd discussed in the comments of the parent issue (#2960) and decided to go with with most-recent-first instead, but it seems the DoD on #3407 was never updated to reflect that decision 🤦 .
I think the look-ahead behavior that you implemented in the query is exactly right! Since cursor pagination is typically based on an exclusive filter, I think the way we would incorporate that as a conditional instruction for cursor-based pagination would look something like this: pagination: {
next: grantFollowersResult.length > limit ? grantFollowersResult[limit - 1].id : null,
},Essentially, we're instructing the client that
Note that the above is pretty implementation-specific, as it relies on the concept that a higher A more agnostic way to implement cursors that support multiple ordering schemes could be to provide the value of |
|
No worries - I've updated to maintain use of |
Ticket #3407
Description
Screenshots / Demo Video
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist