feat(telegram): first-token = new message, delete Thinking on done, register / commands

[chinkan]

Summary

Three small UX improvements to the Telegram platform:

1. First response token is delivered as a new message instead of editing the "⏳ Thinking..." placeholder. Previously the placeholder bubble was edited in place with the first tokens, causing it to visually morph mid-stream. Now current_msg_id starts as None in the streaming task and the placeholder is owned by handle_message.

2. "Thinking..." placeholder is deleted once the response is delivered. Cleanup happens in handle_message after stream_handle.await, on both success and error paths. Best-effort: failures are logged but do not block reporting the actual outcome.

3. Slash commands are published to Telegram at startup via bot.set_my_commands(...), so they appear in the "/" menu in Telegram clients. Existing manual command dispatch is unchanged. The legacy /query-rewrite was renamed to /queryrewrite to satisfy Telegram's BotCommand naming rules ([a-z0-9_]{1,32} — no hyphens). The old form remains accepted as an alias for back-compat.

Files changed

• src/platform/telegram.rs — only file touched (+134 / -9)

Tests

11/11 platform::telegram tests pass (4 new):

• test_first_token_does_not_inherit_placeholder_msg_id — source-inspection guard against re-introducing the placeholder seeding.
• test_placeholder_is_deleted_after_streaming — confirms the cleanup branch is present.
• test_supported_commands_lists_user_visible_commands — validates each registered command name matches Telegram's regex ([a-z0-9_]{1,32}), has non-empty description, and the canonical set of 6 commands is present.
• (existing test_stream_handle_does_not_require_placeholder_send still passes.)

Verification

• cargo fmt --all -- --check — clean.
• cargo test — 214 passed, 0 failed.
• cargo clippy -- -D warnings (matches CI) — clean.

Notes

• The error path keeps the placeholder cleanup: stream_handle.abort() runs first, then stream_handle.await.ok() swallows the cancellation error, then delete_message runs.
• cargo clippy --all-targets -- -D warnings flags 3 pre-existing unrelated test-only warnings on main (useless_vec in agent.rs test, unused_imports: super::* in two memory tests). Out of scope for this PR.

  

[github-actions]

Claude finished @chinkan's task in 1m 52s —— View job

---

Code Review

• Gather context and understand the PR
• Read changed file (src/platform/telegram.rs)
• Review correctness, logic, and UX behavior
• Review tests
• Check code quality, style, and conventions

---

Overall this is a clean, well-reasoned UX improvement. The three changes compose correctly and the sequencing through the async machinery is sound. One minor inefficiency and a couple of worth-noting observations below.

---

Minor Issue: supported_commands() called twice

src/platform/telegram.rs lines 75–81

match bot.set_my_commands(supported_commands()).await {
    Ok(_) => info!(
        "Registered {} Telegram commands",
        supported_commands().len()   // ← allocates a second Vec<BotCommand>
    ),
    Err(e) => warn!(error = %e, "Failed to register Telegram commands"),
}

supported_commands() is called twice: once to pass to set_my_commands, and again inside the Ok arm just to get the count. On success the inner call constructs and immediately drops a second Vec. The fix is trivial — bind to a variable first:

let commands = supported_commands();
let count = commands.len();
match bot.set_my_commands(commands).await {
    Ok(_) => info!("Registered {count} Telegram commands"),
    Err(e) => warn!(error = %e, "Failed to register Telegram commands"),
}

Fix this →

---

Observations (no action needed)

Placeholder visible throughout streaming (intentional trade-off)

With the new flow the "Thinking..." bubble is visible from the moment of initial send until after stream_handle.await finishes — meaning for the entire duration of streaming there are two visible items: the placeholder and the growing response message. The placeholder is only deleted as the very last step. This is intentional per the PR description ("standalone progress signal"), but on slow connections or long responses it could feel odd. Mentioning it so it's a conscious design decision, not a surprise.

Empty-response silent-success case

If the LLM produces zero streaming tokens and process_result is Ok, the function returns Ok(()) after deleting the placeholder — no message is sent to the user. This was already possible before this PR and the streaming task's old placeholder-delete branch handled it (else if let Some(msg_id) = current_msg_id { delete }). That branch is correctly removed since handle_message now owns placeholder cleanup. But the user-visible outcome (silent disappearance of "Thinking..." with no follow-up) is unchanged. Worth a future improvement but out of scope here.

Error path — partial streamed content lingers

When process_result is Err, stream_handle.abort() is called but any tokens already rendered as Telegram messages stay on screen. Only the placeholder is cleaned up. The subsequent error reply then follows those partial messages. This is pre-existing behavior; just noting it for completeness.

---

Tests

The four new tests are fit for purpose. Two observations:

• test_first_token_does_not_inherit_placeholder_msg_id and test_placeholder_is_deleted_after_streaming are source-inspection tests. They guard against known regression patterns and the PR author correctly constructs the needle at runtime to avoid the test body accidentally matching itself. Acceptable approach for invariants that are hard to test via the real API; the fragility trade-off is clearly documented in the comments.

• test_supported_commands_lists_user_visible_commands is a proper behavioral test. The Telegram regex check ([a-z0-9_]{1,32}) and non-empty description assertions give genuine protection. Good.

---

Summary

Approve with suggestion — the double supported_commands() call is the only actionable item and it's easy to fix. Logic, sequencing, error handling, and backward-compat alias are all correct. The help text and registered command names are consistent. CI should pass as reported.

[github-actions]

Claude finished @chinkan's task in 1m 51s —— View job

---

Code Review

• Gather context and understand the PR
• Read changed file (src/platform/telegram.rs)
• Review diff against origin/main
• Verify fix for supported_commands() double-call
• Review correctness, logic, and UX behavior
• Review tests
• Check code quality, style, and conventions

---

This PR is in good shape. The new commit (1e5d9fb) correctly addresses the previous review's only actionable item. Here's the full pass-through:

Previous issue: resolved ✅

src/platform/telegram.rs lines 75–80 — supported_commands() is now bound before the match, with count captured separately:

let commands = supported_commands();
let count = commands.len();
match bot.set_my_commands(commands).await {
    Ok(_) => info!("Registered {} Telegram commands", count),
    Err(e) => warn!(error = %e, "Failed to register Telegram commands"),
}

Exactly what was suggested. No second allocation.

---

Change 1: First token delivered as a new message

Line 331 — current_msg_id correctly starts as None in the streaming task:

let mut current_msg_id: Option<teloxide::types::MessageId> = None;

The streaming task is entirely self-contained: it creates its own first message from the first token, never touching the placeholder. The ownership separation between the placeholder (owned by handle_message) and the response (owned by the streaming task) is clean and well-commented.

Subtle correctness point: On the error path, stream_handle.abort() is called before process_result is stored, and stream_handle.await.ok() is called after notifier_handle.await.ok(). This sequence is correct — abort signals are sent, then both tasks are joined before the placeholder is deleted and the error message is sent. No ordering issue.

---

Change 2: Placeholder deletion

Lines 455–459 — deletion is best-effort, happens unconditionally after streaming completes:

if let Some(placeholder_id) = placeholder_msg_id {
    if let Err(e) = bot.delete_message(msg.chat.id, placeholder_id).await {
        tracing::warn!(error = %e, "Failed to delete thinking placeholder");
    }
}

The error path sequence is sound: abort() → notifier_handle.await → stream_handle.await → delete_message → error reply. Partial streamed content remains (acknowledged in PR), only the placeholder is cleaned up.

One minor thing worth knowing (no action required): the placeholder is visible for the full duration of streaming — there are two simultaneous bubbles (placeholder + growing response) until streaming finishes. The PR acknowledges this trade-off; it's intentional.

---

Change 3: Slash command registration

supported_commands() (lines 51–61) correctly uses only [a-z0-9_] names — no hyphens. The queryrewrite rename from query-rewrite satisfies the Telegram BotCommand constraint. The help text in /start is updated to match (line 151). The back-compat alias on line 225 handles both forms:

if text == "/queryrewrite" || text == "/query-rewrite" {

No issues here. The registration failure is correctly treated as non-fatal (network down at startup shouldn't stop the bot).

---

Tests

All four new tests are sound:

• test_first_token_does_not_inherit_placeholder_msg_id — source-inspection with runtime-constructed needle to avoid self-match. The approach is correct and the fragility is acceptable given there's no way to exercise this via the real Telegram API in unit tests.
• test_placeholder_is_deleted_after_streaming — anchors on the warning string "Failed to delete thinking placeholder". This string is stable enough.
• test_supported_commands_lists_user_visible_commands — proper behavioral test. The Telegram regex check ([a-z0-9_]{1,32}) and non-empty description assertion are genuine regression protection.
• Pre-existing tests still pass; the removed else if let Some(msg_id) = current_msg_id { delete } branch had no standalone test, and its replacement is covered by test_placeholder_is_deleted_after_streaming.

---

Summary

Approve. The previous review's only actionable item is resolved, all three UX changes are correctly implemented, error paths are safe, and the tests are fit for purpose. Ready to merge.

[github-actions]

Claude finished @chinkan's task in 1m 28s —— View job

---

Code Review

• Gather context and understand the PR
• Read changed file (src/platform/telegram.rs)
• Review diff against origin/main
• Verify fix for supported_commands() double-call
• Review correctness, logic, and UX behavior
• Review tests
• Check code quality, style, and conventions

---

Previous Review Issue: Resolved ✓

The double supported_commands() call flagged in the earlier review has been fixed in commit 1e5d9fb. The current code correctly binds to a local variable first:

let commands = supported_commands();
let count = commands.len();
match bot.set_my_commands(commands).await {
    Ok(_) => info!("Registered {} Telegram commands", count),
    Err(e) => warn!(error = %e, "Failed to register Telegram commands"),
}

No redundant allocation. Clean.

---

Correctness & Logic

Error path sequencing (src/platform/telegram.rs lines 436–466)

The abort-then-await-then-cleanup sequence is sound:

Err(e) => {
    stream_handle.abort();   // signal cancellation
    Err(e)
}
// ...
stream_handle.await.ok();   // swallow JoinError::Cancelled — correct
// delete placeholder
// send error reply

JoinError from a cancelled handle is properly absorbed by .ok(), so this doesn't panic or propagate. The placeholder is deleted before the error reply is sent, giving a clean ordering.

First-token-as-new-message (line 331)

current_msg_id initialized to None is correct. The streaming task never knows the placeholder's MessageId, so there is no accidental edit of the placeholder mid-stream. Ownership is clean.

Back-compat alias (line 225)

if text == "/queryrewrite" || text == "/query-rewrite" {

Both forms accepted. Only the canonical /queryrewrite is registered with set_my_commands, which is the right call — the alias is purely defensive for existing muscle memory.

---

Minor Observations (no action required)

Placeholder visible alongside streaming content

For the entire duration of streaming there are two items visible: "⏳ Thinking..." and the growing response. The placeholder is deleted only after stream_handle.await completes. This is intentional per the PR, but on slow connections or long responses the two bubbles will coexist noticeably. A future improvement could delete the placeholder as soon as the first streaming message is sent (by passing placeholder_msg_id into the stream task solely for deletion, not for editing). Not a bug — just a UX note.

500 ms throttle gap on first token

The first streaming message is not sent until last_action.elapsed() >= 500ms triggers. If the model's first token arrives in under 500 ms, the user sees "⏳ Thinking..." for at least 500 ms before any streamed text appears — potentially the placeholder + the first streamed message appear almost simultaneously. Very minor; the 500 ms throttle prevents Telegram rate-limiting and is a well-understood trade-off.

Empty-response silent-success case (pre-existing)

If the LLM produces zero tokens and process_result is Ok, the function returns Ok(()) after deleting the placeholder with no follow-up message. This was present before this PR and is unchanged — just confirming it's still in scope for a future fix.

---

Tests

The four new tests are well-constructed:

• test_first_token_does_not_inherit_placeholder_msg_id — constructs both needle variants at runtime to avoid the test body triggering its own assertion. Sound technique for source-inspection guards.
• test_placeholder_is_deleted_after_streaming — checks for the specific log-string sentinel. Will break if the message is rephrased, but that's acceptable fragility for this kind of invariant.
• test_supported_commands_lists_user_visible_commands — best test of the three: validates Telegram's [a-z0-9_]{1,32} constraint, non-empty descriptions, and exact command set. Behavioral, not source-inspection.
• All 4 new tests plus the 7 existing tests pass per the PR's CI report.

One small note: test_placeholder_is_deleted_after_streaming only checks that the log string exists somewhere in the file, not that it's reachable on both the success and error paths. The actual code does handle both paths correctly (placeholder deletion happens before the if let Err(e) = process_result branch), so this is fine — just worth knowing the test doesn't verify the error path independently.

---

Summary

Approve. The double supported_commands() call is fixed, the core UX changes are correctly sequenced, async error handling is sound, and backward compatibility is maintained. The code is clean and the tests are fit for purpose. No blocking issues.