-
Notifications
You must be signed in to change notification settings - Fork 93
refactor: Implement tasks list #173
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
base: main
Are you sure you want to change the base?
Conversation
|
/gemini review |
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.
Code Review
This pull request introduces a new tasks/list endpoint. The implementation, however, contains several critical bugs that need to be addressed. These include incorrect filtering logic that fails to apply any filters, a type error in the in-memory store that would cause runtime failures, and a missing return statement in a utility function that breaks timestamp validation. Additionally, there are medium-severity issues related to parameter validation, handling of default values, and code duplication in generated types. I've provided detailed comments and suggestions to fix these issues.
|
/gemini review |
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.
Code Review
This pull request introduces a new tasks/list endpoint, which is a great addition. My review focuses on the implementation details of this new feature. I've found a critical issue where the code would crash due to an undefined variable, as well as several other bugs related to pagination logic, parameter validation, and data handling. These issues could lead to incorrect API behavior, such as wrong total counts, incorrect history slicing, and overly restrictive filtering. I've provided detailed comments and suggestions to address these points.
Description
The purpose of this PR is implementing the
tasks/listapi on the server side as for documentation.New
types.tshas been generated to include the parameters for the tasks/listThank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
CONTRIBUTINGGuide.fix:which represents bug fixes, and correlates to a SemVer patch.feat:represents a new feature, and correlates to a SemVer minor.feat!:, orfix!:,refactor!:, etc., which represent a breaking change (indicated by the!) and will result in a SemVer major.Fixes #172 🦕