Closed greg-adams closed 1 day ago
QA Check | Result |
---|---|
๐ Client Tests | โ |
๐ Server Tests | โ |
๐ค E2E Tests | โ |
๐ ESLint | โ |
๐งน TFLint | โ |
Step | Result |
---|---|
๐ Terraform Format & Style | โ |
โ๏ธ Terraform Initialization | โ |
๐ค Terraform Validation | โ |
๐ Terraform Plan | โ |
Hint: If "Terraform Format & Style" failed, run terraform fmt -recursive
from the terraform/
directory and commit the results.
Pusher: @greg-adams, Action: pull_request_target
, Workflow: Continuous Integration
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 (next
) to avoid the user clicking "Show More" an extra time at the end, do you have a preference for supporting this in cursor-based approach? e.g. paginate.before
@greg-adams
The story AC requests sorting alphabetically by name, if we don't need this change I can return to ordering by creation.
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 ๐คฆ .
While making this change I added support on the response to identify a final result set (
next
) to avoid the user clicking "Show More" an extra time at the end, do you have a preference for supporting this in cursor-based approach? e.g.paginate.before
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
pagination.next
is not null
, then there are more results.Note that the above is pretty implementation-specific, as it relies on the concept that a higher grant_followers.id
value implies a higher grant_followers.created_at
value as well. If we were paginating by user name if alphabetical order, this wouldn't work. In fact, it could be argued that the more "correct" way to provide a cursor for follower recency would be to provide pagination.next
as a created_at
timestamp (I chose to go with id
in the original spec because it avoids an edge case stemming from more than one row with the same created_at
value). But that shouldn't really matter as long as the client is treating the cursor as an arbitrary value.
A more agnostic way to implement cursors that support multiple ordering schemes could be to provide the value of grantFollowersResult[orderBy]
as the pagination.next
cursor, but I think that gets a little dicey/brittle without having a clearer sense of what scenarios we'd need to support.
No worries - I've updated to maintain use of created_at
, keeping the next
property to indicate an additional set
Ticket #3407
Description
Screenshots / Demo Video
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist