Skip to content

[feat] Added group threat ID support - #141

Open
foxdevuz wants to merge 2 commits into
appleboy:masterfrom
abdullajonov-lab:master
Open

[feat] Added group threat ID support#141
foxdevuz wants to merge 2 commits into
appleboy:masterfrom
abdullajonov-lab:master

Conversation

@foxdevuz

@foxdevuz foxdevuz commented Apr 8, 2026

Copy link
Copy Markdown
  • Telegram group topic id support

@foxdevuz

foxdevuz commented Apr 8, 2026

Copy link
Copy Markdown
Author

@appleboy can you please check it?

@appleboy appleboy left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Forum Topic (message_thread_id) Support

Good feature idea — forum topic support is valuable. However, there are several issues that should be addressed before merging.

1. RoundTrip Mutates the Original Request (Bug)

Per the http.RoundTripper contract, RoundTrip should not modify the request. The current implementation mutates req.URL.RawQuery directly. The correct pattern is to clone the request first:

func (t *threadIDTransport) RoundTrip(req *http.Request) (*http.Response, error) {
    r := req.Clone(req.Context())
    q := r.URL.Query()
    q.Set("message_thread_id", strconv.Itoa(t.threadID))
    r.URL.RawQuery = q.Encode()
    return t.base.RoundTrip(r)
}

2. Transport Hijacking is Over-broad

threadIDTransport injects message_thread_id into every HTTP request the bot makes — including non-message endpoints like getMe. While Telegram currently ignores unknown params, this is fragile. Consider whether upgrading go-telegram-bot-api to a version that supports message_thread_id natively (e.g., v5) would be a cleaner approach — it would allow setting the thread ID per-message instead of globally at the transport layer.

3. int Should Be int64

Telegram IDs can be large integers. Using int (32-bit on some platforms) risks overflow. This codebase already uses int64 for chat IDs in parseTo. For consistency and safety, MessageThreadID should be int64, using cli.Int64Flag / c.Int64() / strconv.FormatInt().

4. No Tests

No tests were added. At minimum, please add:

  • A unit test for threadIDTransport.RoundTrip verifying the query param is injected on a cloned request
  • A test that MessageThreadID == 0 does not wrap the transport
  • A test verifying correct behavior when SOCKS5 proxy is also configured

5. .gitignore — Missing Trailing Newline

The .gitignore change drops the trailing newline (No newline at end of file). Please add it back.


Recommendation: Please address the RoundTripper contract violation and the intint64 change at minimum. Tests and the design consideration (library upgrade vs transport wrapping) are strongly encouraged.

@foxdevuz

foxdevuz commented Apr 9, 2026

Copy link
Copy Markdown
Author

2. Transport Hijacking is Over-broad

threadIDTransport injects message_thread_id into every HTTP request the bot makes — including non-message endpoints like getMe. While Telegram currently ignores unknown params, this is fragile. Consider whether upgrading go-telegram-bot-api to a version that supports message_thread_id natively (e.g., v5) would be a cleaner approach — it would allow setting the thread ID per-message instead of globally at the transport layer.

I see the the the go-telegram-bot-api is not being updated for 4 years... and since the last commit Oct 20, 2022 is before the topic introduced in telegram Nov 5, 2022 it does not support the message_thread_id

@foxdevuz
foxdevuz requested a review from appleboy April 9, 2026 05:18
@foxdevuz

foxdevuz commented Apr 9, 2026

Copy link
Copy Markdown
Author

@appleboy could you please check once more time?

@foxdevuz

Copy link
Copy Markdown
Author

@appleboy

@ushmodin ushmodin mentioned this pull request May 11, 2026
@trungdlp

Copy link
Copy Markdown

@appleboy How about this item? I am willing to help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants