-
Notifications
You must be signed in to change notification settings - Fork 49
fix(api-arg): js clients has broken arguments so we have to use list of args #1844
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
Conversation
Reviewer's GuideThis PR fixes broken JS client argument handling by refactoring workspace helper functions to pass explicit positional parameters to the v2 API methods and updates Cypress tests to accommodate the increased workspace limit (10000) and wildcard endpoint patterns. Sequence Diagram: Updated
|
Change | Details | Files |
---|---|---|
Refactor workspace helpers for JS v2 client argument list |
|
src/helpers/workspaces/workspaces-helper.ts |
Update Cypress tests for new limit and endpoint patterns |
|
cypress/e2e/workspaces-detail.cy.ts cypress/e2e/filter-workspaces.cy.ts |
Tips and commands
Interacting with Sourcery
- Trigger a new review: Comment
@sourcery-ai review
on the pull request. - Continue discussions: Reply directly to Sourcery's review comments.
- Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with@sourcery-ai issue
to create an issue from it. - Generate a pull request title: Write
@sourcery-ai
anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title
on the pull request to (re-)generate the title at any time. - Generate a pull request summary: Write
@sourcery-ai summary
anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment@sourcery-ai summary
on the pull request to
(re-)generate the summary at any time. - Generate reviewer's guide: Comment
@sourcery-ai guide
on the pull
request to (re-)generate the reviewer's guide at any time. - Resolve all Sourcery comments: Comment
@sourcery-ai resolve
on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore. - Dismiss all Sourcery reviews: Comment
@sourcery-ai dismiss
on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review
to trigger a new review!
Customizing Your Experience
Access your dashboard to:
- Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others. - Change the review language.
- Add, remove or edit custom review instructions.
- Adjust other review settings.
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.
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.
Hey @karelhala - I've reviewed your changes - here's some feedback:
- Consider refactoring the helper methods to define default values for the extra parameters so callers don’t need to pass
false
and{}
every time. - The Cypress intercept patterns using
**/api/rbac/v2/workspaces/*
are quite broad—narrowing them to the exact endpoints (e.g. including query parameters or specific path segments) will help avoid unintended matches.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
statusCode: 200, | ||
body: mockWorkspaces, | ||
}).as('getWorkspaces'); | ||
|
||
// mock the workspace | ||
cy.intercept('GET', '**/api/rbac/v2/workspaces/asd-sda-asd-dsa-asd', { | ||
cy.intercept('GET', '**/api/rbac/v2/workspaces/asd-sda-asd-dsa-asd/*', { |
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.
suggestion (bug_risk): Detail intercept may not match the actual endpoint
The appended /*
pattern requires an extra path segment and won't match /workspaces/:id
. Use **/workspaces/asd-sda-asd-dsa-asd*
or explicitly handle query parameters to ensure correct interception.
/retest |
1 similar comment
/retest |
/retest |
Description
Currently JS clients have bug in which we have to pass full list of arguments with v2 client. Object does not work at all, so let's use this for workspaces.
This PR also fixes bad configuration on workspaces requests in cypress which started to fail because of increased limit to 10000 workspaces.
Screenshots
Before:
After:
Checklist ☑️
Summary by Sourcery
Use full positional arguments for v2 workspaces JS client methods and update Cypress tests to handle increased workspace limit and dynamic endpoint URLs.
Bug Fixes:
Tests: