feat: add Telegram topicId for forum group support notification#2405
Conversation
📝 WalkthroughWalkthroughChangesTelegram notification targets now support an optional topic ID across OpenAPI contracts, generated API types, internal models, file persistence, frontend mappings, notification drafts, and the Telegram Telegram topic support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NotificationUI
participant NotificationAPI
participant NotificationService
participant TelegramBotAPI
NotificationUI->>NotificationAPI: submit optional topicId
NotificationAPI->>NotificationService: map topicId to TopicID
NotificationService->>TelegramBotAPI: send message_thread_id when configured
TelegramBotAPI-->>NotificationService: return delivery response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/notification/service.go`:
- Around line 1336-1343: Update the Telegram payload construction around the
payload map and json.Marshal call so message_thread_id is serialized as a JSON
number rather than a string. Replace the all-string payload with a typed
structure or parse target.Telegram.TopicID into an integer before marshaling,
while preserving omission when TopicID is empty; update the related test
assertion to expect a numeric JSON value.
In
`@ui/src/features/dags/components/dag-details/notifications/NotificationSections.tsx`:
- Around line 325-333: Add an accessible name to the Topic ID Input in
NotificationSections by setting aria-label to “Telegram topic ID” or associating
it with a visible label, while preserving its existing value and onChange
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b71d550-0fb0-4bb6-9686-6104e7a084fe
📒 Files selected for processing (11)
api/v1/api.gen.goapi/v1/api.yamlinternal/notification/model.gointernal/persis/file/notification/store.gointernal/persis/file/notification/store_test.gointernal/service/frontend/api/v1/notifications.gointernal/service/notification/service.gointernal/service/notification/service_test.goui/src/api/v1/schema.tsui/src/features/dags/components/dag-details/notifications/NotificationSections.tsxui/src/features/dags/components/dag-details/notifications/notificationDrafts.ts
| payload := map[string]string{ | ||
| "chat_id": target.Telegram.ChatID, | ||
| "text": messageForEvents(target.Telegram.MessageTemplate, events, s.publicURL()), | ||
| }) | ||
| } | ||
| if target.Telegram.TopicID != "" { | ||
| payload["message_thread_id"] = target.Telegram.TopicID | ||
| } | ||
| body, err := json.Marshal(payload) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
if rg -n -F 'map[string]string' \
internal/service/notification/service.go; then
echo "Telegram payload still forces message_thread_id to a JSON string" >&2
exit 1
fi
python3 - <<'PY'
import json
payload = json.loads(
'{"chat_id":"12345","text":"test","message_thread_id":67890}'
)
assert isinstance(payload["message_thread_id"], int)
PYRepository: dagucloud/dagu
Length of output: 450
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n== service.go around payload ==\n'
sed -n '1300,1365p' internal/service/notification/service.go
printf '\n== tests mentioning Telegram notification payload ==\n'
rg -n -A 3 -B 3 'message_thread_id|TopicID|sendMessage|Telegram' internal/service -g '*_test.go'Repository: dagucloud/dagu
Length of output: 4803
🌐 Web query:
Telegram Bot API sendMessage message_thread_id Integer JSON string accepted
💡 Result:
In the Telegram Bot API, the message_thread_id parameter used in methods like sendMessage is defined as an Integer [1][2]. When sending data via JSON (such as in an HTTP POST request with application/json), this value should be represented as a standard JSON number (e.g., {"message_thread_id": 123}), not as a string [1][2]. While some Telegram client libraries may handle type conversion automatically, the official API specification strictly requires the Integer type [1][2][3].
Citations:
- 1: https://core.telegram.org/bots/api?referrer=wordpress.com
- 2: https://core.telegram.org/bots/api
- 3: https://core.telegram.org/bots/API
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n== notification service references ==\n'
rg -n -A 2 -B 2 'message_thread_id|TopicID|sendMessage|json.Marshal\(map\[string\]string\)' internal/service/notification
printf '\n== tests in notification package ==\n'
rg -n -A 3 -B 3 'message_thread_id|TopicID|sendMessage|Telegram' internal/service/notification -g '*_test.go'Repository: dagucloud/dagu
Length of output: 4341
Serialize message_thread_id as a number
payload is a map[string]string, so message_thread_id is sent as a JSON string. Telegram’s sendMessage API expects this field as an integer, so topic replies can fail. Use a typed payload (or parse TopicID before marshaling) and update the test to assert the JSON number.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/notification/service.go` around lines 1336 - 1343, Update
the Telegram payload construction around the payload map and json.Marshal call
so message_thread_id is serialized as a JSON number rather than a string.
Replace the all-string payload with a typed structure or parse
target.Telegram.TopicID into an integer before marshaling, while preserving
omission when TopicID is empty; update the related test assertion to expect a
numeric JSON value.
| <Input | ||
| value={draft.telegram.topicId} | ||
| placeholder="Topic ID (optional, for forum groups)" | ||
| onChange={(event) => | ||
| update({ | ||
| telegram: { ...draft.telegram, topicId: event.target.value }, | ||
| }) | ||
| } | ||
| /> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an accessible name to the Topic ID field.
This new input relies only on placeholder text, which is not a durable label for screen-reader users. Add aria-label="Telegram topic ID" or a visible <label>.
Proposed fix
<Input
+ aria-label="Telegram topic ID"
value={draft.telegram.topicId}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Input | |
| value={draft.telegram.topicId} | |
| placeholder="Topic ID (optional, for forum groups)" | |
| onChange={(event) => | |
| update({ | |
| telegram: { ...draft.telegram, topicId: event.target.value }, | |
| }) | |
| } | |
| /> | |
| <Input | |
| aria-label="Telegram topic ID" | |
| value={draft.telegram.topicId} | |
| placeholder="Topic ID (optional, for forum groups)" | |
| onChange={(event) => | |
| update({ | |
| telegram: { ...draft.telegram, topicId: event.target.value }, | |
| }) | |
| } | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ui/src/features/dags/components/dag-details/notifications/NotificationSections.tsx`
around lines 325 - 333, Add an accessible name to the Topic ID Input in
NotificationSections by setting aria-label to “Telegram topic ID” or associating
it with a visible label, while preserving its existing value and onChange
behavior.
9e15dd9 to
57af24f
Compare
| } | ||
| if target.Telegram.TopicID != "" { | ||
| if topicID, err := strconv.Atoi(target.Telegram.TopicID); err == nil { | ||
| payload["message_thread_id"] = topicID |
There was a problem hiding this comment.
Should this return an error? An invalid TopicID sends to the default topic.
There was a problem hiding this comment.
Added the validation on notification channels input, instead of checking here
57af24f to
ee41ae8
Compare
yohamta0
left a comment
yohamta0
left a comment
There was a problem hiding this comment.
LGTM 🚀🚀🚀 Thanks for adding topicId support for Telegram notification!
2 participants
Summary
Add optional
topicIdfield to Telegram notification targets for forum group message thread support.Changes
topicIdto Telegram target model and storage schemastopicIdin notification channels and targetsmessage_thread_idto Telegram send payload whentopicIdis settopicIdChecklist
Summary by cubic
Adds optional
topicIdto Telegram notification targets and channels to post into specific forum topics. When set to a positive integer, we send Telegram’smessage_thread_idso messages go to the selected thread.topicIdacross Telegram models, storage, and public API (server and UI schemas); map inapi/v1channel/target requests and responses; UI form/drafts include it.topicIdas a trimmed, positive integer during normalize; reject invalid values with tests for bad inputs.message_thread_idin send payload when valid; update storage round‑trip and send tests to cover thread ID.Written for commit ee41ae8. Summary will update on new commits.
Summary by CodeRabbit