refactor(notification): channel notification bots with pull delivery and web UI#91
Conversation
…and web UI Move notifier delivery from runtime agents to type=notification channel bots, with relay pull/push, dedicated API routes, and supervisor fixes that read stored tokens for pull eligibility. Web UI adds notification sidebar section, simplified notifier profile controls, and CLI bot list limited to normal bots.
Use n-{name} ids for notification bots to avoid worker/IM collisions,
guard delete when user_id is shared, restrict bot GET/PATCH to notification
type, and log empty pull polls at debug.
Pass onAgentCreateBotKindChange to AgentProfileModal so Worker/notification tabs update state after workspace controller refactor.
|
|
||
| filtered := make([]Bot, 0, len(all)) | ||
| for _, b := range all { | ||
| if IsNotificationBot(b) { |
There was a problem hiding this comment.
This unconditional skip means the normal list API now hides notification bots for every channel. For the CLI behavior we want to avoid adding a separate bot notification list command: bot list --channel csgclaw should include both type=normal and type=notification, while bot list --channel feishu should continue to return only normal bots. Please move that channel-specific filtering into this list path and drop the separate notification-list surface/tests.
| r.Get("/", h.handleBotByID) | ||
| r.Patch("/", h.handleBotByID) | ||
| r.Delete("/", h.deleteBot) | ||
| r.Post("/notifications", h.pushNotificationBot) |
There was a problem hiding this comment.
Route structure suggestion: keep common bot CRUD on the generic /api/v1/channels/{channel}/bots and /api/v1/channels/{channel}/bots/{id} routes, and keep channel-specific actions on explicit channel routes only. In practice that means generic routes handle GET/POST /bots and GET/PATCH/DELETE /bots/{id}, /api/v1/channels/csgclaw/bots/{id}/notifications stays csgclaw-only, and /api/v1/channels/feishu/bots/{id}/events stays feishu-only. This avoids duplicating the csgclaw/feishu CRUD routes while also avoiding a generic /{channel}/bots/{id}/notifications endpoint for channels that do not support notifications.
|
|
||
| import "testing" | ||
|
|
||
| func TestNormalizeBotTypeDefaultsToNormal(t *testing.T) { |
There was a problem hiding this comment.
These tests are small and cover model.go behavior directly; consider moving them into internal/bot/model_test.go instead of keeping a separate file just for bot type.
| switch r.Method { | ||
| case http.MethodGet: | ||
| if r.URL.Query().Get("type") == bot.BotTypeNotification { | ||
| bots, err := h.botSvc.ListNotificationBots(channelName) |
There was a problem hiding this comment.
Prefer keeping this type-specific filtering inside internal/bot: the handler can pass type=notification as list criteria, and the service can decide whether to return normal, notification, or both for each channel.
| if len(fs.Args()) != 0 { | ||
| return fmt.Errorf("bot notification list does not accept positional arguments") | ||
| } | ||
| bots, err := run.APIClient(globals).ListNotificationBots(ctx, *channelName) |
There was a problem hiding this comment.
This should probably go through the same bot list path with a type filter instead of a notification-specific list API; that keeps CLI/API behavior aligned and avoids a separate notification list surface.
|
|
||
| import "testing" | ||
|
|
||
| func TestAdvertiseBaseURLForClient(t *testing.T) { |
There was a problem hiding this comment.
This is a small config helper test; consider moving it into internal/config/config_test.go instead of adding a separate test file for advertise_base_url.
| } | ||
|
|
||
| // AdvertiseBaseURLForClient returns the base URL exposed to Web UI and external webhook callers. | ||
| func AdvertiseBaseURLForClient(server ServerConfig) string { |
There was a problem hiding this comment.
Consider renaming this to ResolveAdvertiseBaseURL: the helper does more than return a client value, since it resolves server.advertise_base_url with a fallback derived from listen_addr.
Move notification bot listing into bot.List with channel-aware type
filtering; drop separate notification list CLI/API surfaces and add
bot list --type. Refactor routes so generic CRUD uses /{channel}/bots
while csgclaw push and feishu events stay channel-specific. Rename
AdvertiseBaseURLForClient to ResolveAdvertiseBaseURL and merge small
tests into model_test.go and config_test.go.
| Role: item.Role, | ||
| RuntimeKind: item.RuntimeKind, | ||
| Image: item.Image, | ||
| Env: item.Env, |
There was a problem hiding this comment.
This added Env assignment does not compile at the current PR head: apitypes.HubTemplate has no Env field, and hub.Template has no Env field either. go test ./cli ./internal/api ./internal/bot ./internal/apiclient fails before the API and CLI packages can build. Please either add the field to both template types or remove this assignment.
|
|
||
| filtered := make([]Bot, 0, len(all)) | ||
| for _, b := range all { | ||
| if !shouldIncludeBotInList(b, normalizedChannel, botType) { |
There was a problem hiding this comment.
The store-backed bots are filtered by botType, but configured Feishu bots are appended afterward without seeing that filter. As a result, /api/v1/channels/feishu/bots?type=notification can still return configured Feishu bots, even though those are normal bots and notification bots are meant to be excluded for Feishu. Consider skipping this append when botType normalizes to notification, or pass the type filter into appendConfiguredFeishuBots.
Drop hub template Env from presentHubTemplate on the notifier branch so API builds without hub types. Skip appendConfiguredFeishuBots when type=notification so configured Feishu apps are not returned for that filter.
|
LGTM |
2 participants
Move notifier delivery from runtime agents to type=notification channel bots, with relay pull/push, dedicated API routes, and supervisor fixes that read stored tokens for pull eligibility. Web UI adds notification sidebar section, simplified notifier profile controls, and CLI bot list limited to normal bots.