- Notifications
You must be signed in to change notification settings - Fork908
Combine tools#349
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Combine tools#349
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Pull Request Overview
This PR consolidates various GitHub notification tools into a single ManageNotifications tool to reduce redundant tooling and simplify the API.
- Combined separate functions for marking a notification as read, marking all notifications as read, and marking a notification as done into one unified tool.
- Updated tooling registration in pkg/github/tools.go and merged logic in pkg/github/notifications.go to support action-based behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/github/tools.go | Registering notifications tool now using GetNotifications and ManageNotifications. |
| pkg/github/notifications.go | Consolidated and refactored notification actions into a single switch-case structure. |
| if lastReadAt != "" { | ||
| lastReadTime, err := time.Parse(time.RFC3339, lastReadAt) | ||
| switch action { | ||
| case "mark_read": |
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.
The 'mark_read' action does not call any API to mark a notification as read; it directly returns a success message. Please add the appropriate API call to actually mark the notification as read.
Copilot uses AI. Check for mistakes.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
rfearing commentedApr 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sorry for the pings. I'm going to take another look at this first. |
4624a97 tod49b7a6CompareI did this work in the end as part of getting notifications stuff ready. Feel free to take a look at the original PR to see what happened. |
Uh oh!
There was an error while loading.Please reload this page.
👉🏼 👉🏼 Note the target branch is
notifications-tooling, notmasterFrommy previous PR into the same target branch, where I added "mark notification as done" I received the feedback to combine some of the tooling so we don't add as many tools for notification. This is my attempt to do that. Note, I definitely used
for help.
👇🏼 Previous feedback:
