Saved Searches Pagination by 0xnmn · Pull Request #44347 · sourcegraph/sourcegraph-public-snapshot

0xnmn · 2022-11-14T11:04:55Z

closes: https://github.com/sourcegraph/sourcegraph/issues/43623
closes: https://github.com/sourcegraph/sourcegraph/issues/43418

There is no pagination present (neither BE nor FE) for saved searches page under user settings and org settings. All the saved searches for under the user's namespace along with all saved searches under all the orgs the user is part of are fetched at once and then filtered based on the page on the frontend.

That sucks, so this PR introduces offset-based pagination at the backend, along with new resolvers to get saved searches by namespace. The new API is then used to show PageSelector component for pagination on the frontend.

TODO

set page size to 10 before merge
add unit tests after first review of the draft.

Screenshots

Org Settings Saved Searches

User Settings Saved Searches

admin view for non logged in user's saved searches

admin view for non member organisation's saved searches

non-admin access to an inaccessible page.

Test plan

https://www.loom.com/share/ebeec2dea8e94b52a680012f5e4761fd

sg-e2e-regression-test-bob · 2022-11-14T11:14:21Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (0.00 kb)	0.02% (+2.76 kb)	0.02% (+2.76 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits 18f3243 and a39ae21 or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

vdavid

I went through the code a few times, and didn't see any problems, it all seems decent to me!
BuildLimitOffsetArgs is a nice addition to the helpers; I find it weird that it didn't exist before.

vdavid · 2022-11-15T10:41:18Z

I've just recently been getting started with Go, so not sure if it's standard practice, but I've seen many contructor functions being called NewSomething. In this case, wouldn't NewLimitOffset be a better name for this function?
Edit: on a second thought, it may return nil, so perhaps the name NewLimitOffset could be misleading.

kopancek · 2022-11-15T11:38:10Z

All of our existing pagination is based on PageInfo type defined in graphql (IIRC). Which in turn is based on cursor pagination, not offset and limit.

I think we can reuse it instead of introducing our own thing. The convention is then to ask for

first: Int after: String

Where first is basically the limit as you use it. And after is the cursor that points to the item after which want to get the next page worth of items. You can see it in action on org members here.

@kopancek Agreed! I've already discussed this with @thenamankumar as well, here's some additional context: https://sourcegraph.slack.com/archives/C01C3NCGD40/p1668508044348929?thread_ts=1668499559.791709&cid=C01C3NCGD40

Ideally we can implement a proper cursor based navigation to avoid doing a LIMIT ... OFFSET ... query and avoid sequential scans on the database level :)

I mean, it probably is OK for saved searches to do limit offset from a performance point of view, as there's not going to be many rows. But having a generic solution for proper cursors would be best, agreed :)

So I'm not totally oposed to using limit/offset pagination on the DB level here for now, but would like to keep the existing interface on the graphql layer, as I do not see the benefit of changing it for this specific feature.

kopancek · 2022-11-15T11:39:35Z

Do we already have an existing paginated list component that you can just reuse here?

kopancek · 2022-11-15T11:40:41Z

I think we can hide the page selector if totalCount <= PAGE_SIZE, but feel free to verify this with some designer, e.g. Rob or Quinn Keast.

kopancek · 2022-11-15T11:53:07Z

Supplying NamespaceType should not be necessary as graphql.ID already encodes the namespace in it. E.g. it is just Org:42 or User:12 type of thing.

You can use UnmarshalNamespaceID or NamespaceByID to get what you need here.

So all in all, this should be sufficient:

Suggested change

func (r *schemaResolver) SavedSearchesByNamespace(ctx context.Context, args *struct {

NamespaceType string

NamespaceId graphql.ID

}) (*savedSearchesConnectionResolver, error) {

func (r *schemaResolver) SavedSearchesByID(ctx context.Context, args *struct { ID graphql.ID }) (*savedSearchesConnectionResolver, error) {

kopancek · 2022-11-15T11:59:55Z

Just trying to understand, would it be too expensive to ensure this on the SQL level as well?

kopancek · 2022-11-15T12:00:40Z

What happens if orgID is nil in this case?

* WIP add usePageinatedConnection * WIP add test and make first page navigatnion work * Add remaining functions * More test cases, backward pagination working

…n hook

philipp-spiess

Two ideas

philipp-spiess · 2022-12-02T16:00:02Z

+func (p *ConnectionPageInfo[N]) HasNextPage() bool {
+	if p.args.Before != nil {
+		return true
+	}
+
+	return p.fetchedNodesCount > p.args.Limit()
+}
+
+func (p *ConnectionPageInfo[N]) HasPreviousPage() bool {
+	if p.args.After != nil {
+		return true
+	}
+
+	if p.args.Before != nil {
+		return p.fetchedNodesCount > p.args.Limit()
+	}
+
+	return false
+}


I've been playing around with this locally and noticed a bug in this implementation in backward pagination mode. Try out this one:

Suggested change

func (p *ConnectionPageInfo[N]) HasNextPage() bool {

if p.args.Before != nil {

return true

}

return p.fetchedNodesCount > p.args.Limit()

}

func (p *ConnectionPageInfo[N]) HasPreviousPage() bool {

if p.args.After != nil {

return true

}

if p.args.Before != nil {

return p.fetchedNodesCount > p.args.Limit()

}

return false

}

func (p *ConnectionPageInfo[N]) HasNextPage() bool {

if p.args.First != nil {

return p.fetchedNodesCount > p.args.Limit()

}

return p.args.Before != nil

}

func (p *ConnectionPageInfo[N]) HasPreviousPage() bool {

if p.args.Last != nil {

return p.fetchedNodesCount > p.args.Limit()

}

return p.args.After != nil

}

philipp-spiess · 2022-12-02T16:01:07Z

+}
+
+type ConnectionNode interface {
+	ID() graphql.ID


Suggested change

ID() graphql.ID

The ID() is actually not required as we call to marshalCursor instead. I recommend we remove it because this way we can also make it work for legacy routes that can not be paginated in sql (e.g. git endpoints). E.g. I’m migrating the repo contributors which is not backend by a SQL query so the individual returns have no ID

0xnmn self-assigned this Nov 14, 2022

cla-bot Bot added the cla-signed label Nov 14, 2022

0xnmn changed the title ~~First working state~~ Saved Searches Pagination Nov 14, 2022

0xnmn force-pushed the naman/saved-searches-paginate branch from b704543 to 3b5a621 Compare November 15, 2022 07:47

0xnmn requested review from a team, AlicjaSuska and philipp-spiess November 15, 2022 08:02

0xnmn force-pushed the naman/saved-searches-paginate branch from 3b5a621 to aaaf67a Compare November 15, 2022 08:33

vdavid reviewed Nov 15, 2022

View reviewed changes

kopancek reviewed Nov 15, 2022

View reviewed changes

0xnmn added 2 commits November 16, 2022 20:51

Add pagination to saved searches

745bddb

WIP

0f62a5f

0xnmn force-pushed the naman/saved-searches-paginate branch from aaaf67a to 0f62a5f Compare November 16, 2022 16:55

0xnmn and others added 7 commits November 17, 2022 16:31

Create NewConnectionResolver

f4a2335

Add usePaginatedConnection (#44574)

86da8e4

* WIP add usePageinatedConnection * WIP add test and make first page navigatnion work * Add remaining functions * More test cases, backward pagination working

minor fe integration efforts & bug fix

ec833b6

fix hasNextPage hasPreviousPage

72f1dbb

add BE reversal for last

9228f2f

Add URL sync logic, fix tests, and clean up new usePaginatedConnectio…

236484e

…n hook

Update SavedSearchListPage.tsx

76004f1

philipp-spiess mentioned this pull request Nov 21, 2022

Add usePageSwitcherPagination hook #44664

Merged

WIP

18f3243

philipp-spiess removed the request for review from AlicjaSuska December 2, 2022 11:23

philipp-spiess reviewed Dec 2, 2022

View reviewed changes

0xnmn closed this Jan 3, 2023

-func (r *schemaResolver) SavedSearchesByNamespace(ctx context.Context, args *struct {
-	NamespaceType string
-	NamespaceId   graphql.ID
-}) (*savedSearchesConnectionResolver, error) {
+func (r *schemaResolver) SavedSearchesByID(ctx context.Context, args *struct { ID   graphql.ID }) (*savedSearchesConnectionResolver, error) {

Conversation

0xnmn commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Screenshots

Org Settings Saved Searches

User Settings Saved Searches

admin view for non logged in user's saved searches

admin view for non member organisation's saved searches

non-admin access to an inaccessible page.

Test plan

Uh oh!

sg-e2e-regression-test-bob commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

vdavid left a comment

Choose a reason for hiding this comment

Uh oh!

vdavid Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipp-spiess left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

0xnmn commented Nov 14, 2022 •

edited

Loading

sg-e2e-regression-test-bob commented Nov 14, 2022 •

edited

Loading

vdavid Nov 15, 2022 •

edited

Loading