feat(chat-app): implement Slack adapter#7
Conversation
scion-gteam
Bot
force-pushed
the
scion/slack-chat-app
branch
from
7971bbb to
94b2280
Compare
…nt normalization, and Socket Mode support
scion-gteam
Bot
force-pushed
the
scion/slack-chat-app
branch
from
94b2280 to
67b115b
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a Slack adapter for the scion-chat-app, enabling multi-platform support alongside Google Chat. The changes include the Slack adapter implementation with support for Webhooks and Socket Mode, Block Kit rendering for cards, and App Home tab publishing. The core chatapp logic was refactored to manage multiple messengers and handle platform-specific user lookups and mentions. Feedback focuses on potential performance issues from synchronous network calls in the command flow and the lack of timeouts or cancellation mechanisms in background goroutines handling Slack events and interactions.
| PlatformID: el.event.UserID, | ||
| }, nil | ||
| } | ||
| user, err := el.messenger.GetUser(ctx, el.event.UserID) |
There was a problem hiding this comment.
The messenger.GetUser() call is performed synchronously within the command handling flow. Given that this can involve network I/O, consider if this should be cached or if the latency is acceptable for the command execution path.
| w.WriteHeader(http.StatusOK) | ||
|
|
||
| // Process asynchronously. | ||
| go a.processEventsAPIEvent(context.Background(), &eventsAPIEvent) |
There was a problem hiding this comment.
Using context.Background() in an asynchronous goroutine can lead to premature cancellation if the parent request context is used. However, it also means the operation lacks a timeout or cancellation mechanism. Consider passing a context with a timeout or using a dedicated background context with a timeout.
| }) | ||
|
|
||
| // Process asynchronously. | ||
| go a.processSlashCommand(context.Background(), cmd) |
There was a problem hiding this comment.
Similar to the event handler, this asynchronous processing lacks a timeout mechanism. Consider passing a context with a timeout to ensure long-running operations do not leak goroutines.
| w.WriteHeader(http.StatusOK) | ||
|
|
||
| // Process asynchronously. | ||
| go a.processInteraction(context.Background(), callback) |
There was a problem hiding this comment.
Asynchronous processing of interactions without a bounded context or timeout could lead to resource exhaustion under high load. Consider using a worker pool or a context with a timeout.
… handlers Cache GetUser results per-event in eventUserLookup to avoid redundant API calls during event processing. Replace unbounded context.Background() with 30-second timeout contexts in all async goroutines (HTTP and Socket Mode handlers) to prevent resource leaks from hung operations.
1 participant
Summary
internal/slack/) implementing thechatapp.Messengerinterface as the second platform provider after Google Chatchatapp.Dialogto SlackModalViewRequestwith text, textarea, select, and checkbox field typeschat:write.customizewith RoboHash avatars through anIconProviderabstractionCommandRouterandNotificationRelay— both now route messages to the correct platform adapter via amessengersmap<@UID>for Slack,<users/UID>for Google Chat) innotifications.gohelp,info,register,unregistercommandsFiles changed
internal/slack/adapter.gointernal/slack/blocks.gointernal/slack/modals.gointernal/slack/events.gointernal/slack/apphome.gointernal/slack/verify.gointernal/slack/adapter_test.gointernal/chatapp/commands.gointernal/chatapp/notifications.gointernal/chatapp/notifications_test.goTestFormatMentioninternal/chatapp/config.goSlackConfig(signing_secret, listen_address, socket_mode)cmd/scion-chat-app/main.gogo.mod/go.sumgithub.com/slack-go/slack v0.23.0Test plan
go test ./internal/slack/...)go test ./internal/chatapp/...)/scion help,/scion link,/scion subscribe)