-
Notifications
You must be signed in to change notification settings - Fork 1.1k
(feat) Crashlytics tools improvements #9138
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
- Making interval handling more robust - Adding return messages to avoid undefined tool responses
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.
Summary of Changes
Hello @maxl0rd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the Crashlytics tools by introducing a new capability to batch retrieve events, which improves efficiency when fetching multiple event details. It also makes the handling of time-based filters more resilient by ensuring that report queries always have a complete time interval. Furthermore, the changes refine the API interactions for updating issues and deleting notes, providing clearer feedback and more consistent request structures. These improvements aim to make the Crashlytics integration more robust and user-friendly for developers.
Highlights
- New Batch Event Retrieval: Introduced a new
batchGetEvents
function and associated types, allowing for the efficient retrieval of multiple Crashlytics events by their resource names, with a limit of 100 events per batch. - Robust Interval Handling: Improved the handling of time intervals in Crashlytics reports by automatically setting
intervalEndTime
to the current time ifintervalStartTime
is provided without an end time, making interval queries more robust. - Enhanced Tool Responses and API Consistency: Updated the
deleteNote
function to return a confirmation message upon successful deletion and refined API request bodies for issue updates and tool definitions for better consistency and clarity. - Updated Tool Definitions and Prompts: Renamed Crashlytics tools in prompts (e.g.,
list_top_issues
toget_top_issues
) and added more detailed instructions for applying filters (likeissueErrorType
and time intervals) when using these tools.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands on the current page.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 several improvements to the Crashlytics tools. It adds a new batchGetEvents
function to fetch multiple events at once, including corresponding types and tests. It also makes interval handling more robust by setting a default end time if only a start time is provided. Additionally, it improves user feedback by adding a return message to the deleteNote
function. The changes are well-structured and include necessary updates to tests and type definitions. My review includes one suggestion to improve maintainability by replacing a magic number with a constant.
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.
A couple small things, nothing blocking from my end.
eventNames: string[], | ||
): Promise<BatchGetEventsResponse> { | ||
const requestProjectNumber = parseProjectNumber(appId); | ||
if (eventNames.length > 100) throw new FirebaseError("Too many events in batchGet request"); |
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.
Should we even let this many come through? I feel like we shouldn't pull back more than like 30 tops to keep the context window from blowing up.
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.
I haven't written the tool yet, but I think we could limit it there. These defs map to the api's limits.
intervalStartTime: z | ||
.string() | ||
.optional() | ||
.describe(`A timestamp in ISO 8601 string format. Defaults to 7 days ago.`), |
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.
Should we add the default value to zod?
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.
We wouldn't want a static timestamp burned into the tool definition.
* Format: "projects/{project}/apps/{app_id}/events/{event_id}" | ||
* @return A BatchGetEventsResponse including an array of Event. | ||
*/ | ||
export async function batchGetEvents( |
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.
It looks like we don't have a tool defined for this - do we want to use this now? Should we keep it out of here until we find we have a need for it?
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.
This should be fine to add the library method here, but I want to do more testing before adding the tool.
- Improving instructions and return values in crashlytics tools - Making interval handling more robust - Adding return messages to avoid undefined tool responses
- Improving instructions and return values in crashlytics tools - Making interval handling more robust - Adding return messages to avoid undefined tool responses
Description