Skip to content
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

feat(core): Adds Pagination to StreamGetBranches and increases limit requested by DUI #2440

Closed
wants to merge 2 commits into from

Conversation

jsdbroughton
Copy link
Contributor

@jsdbroughton jsdbroughton commented Apr 14, 2023

Closes: #2436

Description & motivation

Allows for use cases heard from Speckle users requiring more than 100 branches as they address cataloguing or complex team information management. The server allows a maximum of 100 results per query; the current solution takes this as a hard limit.

Changes:

Core / API / GraphQL / Client.BranchOperations :: StreamGetBranches

  • Adds pagination to the query

DUI2 / ViewModels / StreamViewModel

  • increases the Branches requested from 100 to 1000.

Validation of changes:

Rhino and Navisworks both tested against https://speckle.xyz/streams/01fca0eab7

Checklist:

  • My pull request follows the guidelines in the Contributing guide?
  • My pull request does not duplicate any other open Pull Requests for the same update/change?
  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • My code follows a similar style to existing code.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

@jsdbroughton jsdbroughton self-assigned this Apr 14, 2023
@jsdbroughton jsdbroughton added enhancement New feature or request desktopui issues related to the desktop ui. core issues related to the .net sdk. labels Apr 14, 2023
@jsdbroughton jsdbroughton marked this pull request as ready for review April 14, 2023 23:43
@jsdbroughton jsdbroughton changed the title feature: Adds Pagination to StreamGetBranches and increase limit requested by amend DUI feature: Adds Pagination to StreamGetBranches and increase limit requested by DUI Apr 14, 2023
@jsdbroughton jsdbroughton changed the title feature: Adds Pagination to StreamGetBranches and increase limit requested by DUI feat(core): Adds Pagination to StreamGetBranches and increase limit requested by DUI Apr 14, 2023
@jsdbroughton jsdbroughton changed the title feat(core): Adds Pagination to StreamGetBranches and increase limit requested by DUI feat(core): Adds Pagination to StreamGetBranches and increases limit requested by DUI Apr 14, 2023
@AlanRynne AlanRynne self-requested a review April 15, 2023 14:02
Copy link
Member

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would advise against this solution.

The api call should be "pure", meaning it does 1 call when you run it, not potentially 100s. It should be the responsibility of the project using this method to implement pagination on their end.

☝🏼 This would require adding a cursor input to this call, which I'd be happy to have.

If this is something commonly done in many connectors, we should create a separate function to get N branches while safely handling all other server considerations, but leave StreamGetBranches untouched.

var res = await ExecuteGraphQLRequest<StreamData>(request, cancellationToken).ConfigureAwait(false);
branches.AddRange(res.stream.branches.items);
nextCursor = res.stream.branches.cursor;
} while (branches.Count < branchesLimit && nextCursor != null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This while loop is questionable, depending on the inputs it could hit our rate-limit quite easily.

@jsdbroughton
Copy link
Contributor Author

That makes sense. I can review. The idea here would be to do that call recursion in DUI. Thanks

@teocomi
Copy link
Member

teocomi commented Apr 16, 2023

Agree with Alan :)

It also sounds more of a UI/UX problem; how would people deal with 100+ branches in the current DUI?
Or better, how are they planning to use them? This could be pretty informative for DUI3...

@jsdbroughton
Copy link
Contributor Author

It also sounds more like a UI/UX problem; how would people deal with 100+ branches in the current DUI?
Or better, how are they planning to use them? This could be pretty informative for DUI3...

This is a UI/UX problem that exists in both Connectors and the web. The specific use case I have referred to in Notion cc-ing @teocomi

Aside from the reporting user, we see increasing numbers of users with catalogue-like approaches to their Speckle data management. cc @paloknapo

Where users have greater than 100 branches to a stream, these will be accessible online by URL at least until this may be supported. In connectors, you cannot reach your data. #2437 Suggests a further enhancement to DUI as a UX solution for many branches.

An alternative consideration is to have a "fetch more branches" button.

@jsdbroughton jsdbroughton requested a review from AlanRynne April 22, 2023 19:50
@jsdbroughton jsdbroughton dismissed AlanRynne’s stale review April 22, 2023 20:08

I've refactored this content away

@teocomi
Copy link
Member

teocomi commented Jul 19, 2023

Closing as currently blocked by specklesystems/speckle-server#1549 - will reopen/revisit when the right time comes.

@teocomi teocomi closed this Jul 19, 2023
@jsdbroughton jsdbroughton deleted the dui2/jsdb/branches branch October 26, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the .net sdk. desktopui issues related to the desktop ui. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants