Conversation
There was a problem hiding this comment.
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.
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.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Sorry for the pings. I'm going to take another look at this first. |
SamMorrowDrums
force-pushed
the
notifications-tooling
branch
2 times, most recently
from
4624a97 to
d49b7a6
Compare
|
I 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. |
3 participants
👉🏼 👉🏼 Note the target branch is
notifications-tooling, notmasterFrom my 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:
