channels: telegram remote-control phase 1 (status, sessions, new) (#1805)#2249
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Telegram remote-control: slash-command parsing and handlers (/status, /sessions, /new, /help), persisted session bindings with titles and busy flags, event-bus busy tracking, runtime routing and startup registration, module exports, capability catalog entry, UI docs, and tests. ChangesTelegram Remote Control
Sequence Diagram(s): sequenceDiagram
participant TelegramClient
participant ChannelRuntime
participant TelegramSessionStore
participant FileSystem
participant TelegramProvider
TelegramClient->>ChannelRuntime: send /status /sessions /new /help
ChannelRuntime->>TelegramSessionStore: with_store_read / with_store (resolve binding, busy)
TelegramSessionStore->>FileSystem: read/write JSON (bindings, titles, busy flags)
FileSystem-->>TelegramSessionStore: stored data
TelegramSessionStore-->>ChannelRuntime: binding / busy / sessions list
ChannelRuntime->>TelegramProvider: send response message
TelegramProvider-->>TelegramClient: deliver reply
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 3
🧹 Nitpick comments (1)
src/openhuman/channels/providers/telegram/remote_control_tests.rs (1)
3-27: ⚡ Quick winConsider adding a test for case-insensitive parsing.
The implementation lowercases commands (Line 37 in
remote_control.rs), but this behavior isn't covered by tests. Adding a case-insensitivity assertion would document the expected behavior and catch regressions.📝 Suggested addition
assert_eq!( parse_telegram_remote_command("/status@MyBot"), Some(TelegramRemoteCommand::Status) ); + // Case insensitivity + assert_eq!( + parse_telegram_remote_command("/STATUS"), + Some(TelegramRemoteCommand::Status) + ); + assert_eq!( + parse_telegram_remote_command("/Sessions"), + Some(TelegramRemoteCommand::Sessions) + ); assert_eq!( parse_telegram_remote_command(" /sessions "), Some(TelegramRemoteCommand::Sessions) );🤖 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 `@src/openhuman/channels/providers/telegram/remote_control_tests.rs` around lines 3 - 27, Add a case-insensitivity assertion to the parse_remote_commands test: call parse_telegram_remote_command with an upper/mixed-case command (e.g., "/STATUS" or "/StAtUs@MyBot") and assert it returns the same TelegramRemoteCommand variant as the lowercase form (e.g., TelegramRemoteCommand::Status), so the existing lowercase-normalizing behavior in parse_telegram_remote_command is covered and guarded against regressions.
🤖 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 `@src/openhuman/channels/providers/telegram/bus.rs`:
- Around line 21-30: set_busy currently calls the blocking with_store directly
on the async path; change it to offload persistence to a blocking thread by
calling tokio::task::spawn_blocking(move || with_store(&self.workspace_dir,
|store| { store.set_busy(reply_target, busy); Ok(()) })) and await the
JoinHandle; then match on the join result and the inner with_store Result
separately so you log a join error (task panicked) differently from a store
error (I/O/mutex failure). Keep references to set_busy, with_store,
workspace_dir and ensure you clone/move reply_target and busy into the closure
as needed.
In `@src/openhuman/channels/providers/telegram/session_store.rs`:
- Around line 104-112: The singleton cache in STORE used by with_store currently
stores a single Option<TelegramSessionStore>, causing different workspace_dir
calls to share the same store; change STORE to map workspace paths to stores
(e.g., Mutex<HashMap<PathBuf, TelegramSessionStore>>) and update with_store to
look up by workspace_dir (convert to PathBuf), load via
TelegramSessionStore::load(workspace_dir) only when missing, and then pass the
correct per-workspace store to the closure; ensure locking still uses the
existing mutex around the map and preserve error propagation from
TelegramSessionStore::load.
In `@src/openhuman/channels/runtime/startup.rs`:
- Around line 566-573: The subscription handle returned by bus.subscribe is
being assigned to a block-scoped variable _telegram_remote_handle inside the if
and is dropped at the end of the block, unregistering TelegramRemoteSubscriber;
to fix, store the handle returned by bus.subscribe (from
crate::openhuman::channels::providers::telegram::TelegramRemoteSubscriber::new(...))
in a long-lived place so it lives for the full runtime—for example push it into
a runtime-level Vec of subscription handles or a struct field (e.g.,
runtime.subscription_handles) instead of a block-local _telegram_remote_handle
so the TelegramRemoteSubscriber remains registered.
---
Nitpick comments:
In `@src/openhuman/channels/providers/telegram/remote_control_tests.rs`:
- Around line 3-27: Add a case-insensitivity assertion to the
parse_remote_commands test: call parse_telegram_remote_command with an
upper/mixed-case command (e.g., "/STATUS" or "/StAtUs@MyBot") and assert it
returns the same TelegramRemoteCommand variant as the lowercase form (e.g.,
TelegramRemoteCommand::Status), so the existing lowercase-normalizing behavior
in parse_telegram_remote_command is covered and guarded against regressions.
🪄 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: 72358bab-0d5b-4b97-8935-b90bd053ea15
📒 Files selected for processing (11)
app/src/components/channels/TelegramConfig.tsxsrc/openhuman/about_app/catalog.rssrc/openhuman/channels/providers/telegram/bus.rssrc/openhuman/channels/providers/telegram/bus_tests.rssrc/openhuman/channels/providers/telegram/mod.rssrc/openhuman/channels/providers/telegram/remote_control.rssrc/openhuman/channels/providers/telegram/remote_control_tests.rssrc/openhuman/channels/providers/telegram/session_store.rssrc/openhuman/channels/routes.rssrc/openhuman/channels/routes_tests.rssrc/openhuman/channels/runtime/startup.rs
coderabbitai
Bot
added
the
feature
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/channels/providers/telegram/bus_tests.rs (1)
41-43: ⚡ Quick winRename
idletobusyfor clarity.The variable
idleholds the result ofis_busy(), so whenidleistrue, the session is actually busy (not idle). This makes the assertionassert!(!idle)confusing to read. Usebusyconsistently with line 23-25.♻️ Clearer variable naming
- let idle = super::session_store::with_store(dir.path(), |store| Ok(store.is_busy("chat-99"))) + let busy = super::session_store::with_store(dir.path(), |store| Ok(store.is_busy("chat-99"))) .expect("store"); - assert!(!idle); + assert!(!busy);🤖 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 `@src/openhuman/channels/providers/telegram/bus_tests.rs` around lines 41 - 43, Rename the local variable currently named `idle` to `busy` where it captures the result of `store.is_busy()` (the call inside `session_store::with_store`), and update the assertion to use the new name (e.g., `assert!(!busy)` to preserve the current test expectation); also update any other occurrences in the test that reference `idle` so naming is consistent with lines 23–25.
🤖 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.
Nitpick comments:
In `@src/openhuman/channels/providers/telegram/bus_tests.rs`:
- Around line 41-43: Rename the local variable currently named `idle` to `busy`
where it captures the result of `store.is_busy()` (the call inside
`session_store::with_store`), and update the assertion to use the new name
(e.g., `assert!(!busy)` to preserve the current test expectation); also update
any other occurrences in the test that reference `idle` so naming is consistent
with lines 23–25.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 155343bc-b4e8-472e-a12b-85317dc91433
📒 Files selected for processing (5)
app/src/components/channels/__tests__/TelegramConfig.test.tsxsrc/openhuman/channels/providers/telegram/bus_tests.rssrc/openhuman/channels/providers/telegram/remote_control_tests.rssrc/openhuman/channels/providers/telegram/session_store.rssrc/openhuman/channels/routes_tests.rs
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Walkthrough
Phase 1 Telegram remote control: adds /status, /sessions, /new, /help slash commands parsed in the existing channel runtime command path, backed by a workspace JSON store (state/telegram_remote_sessions.json) for chat→thread bindings and busy flags. An event-bus subscriber (TelegramRemoteSubscriber) tracks in-flight Telegram turns. Settings UI gets an informational callout. Capability catalog updated.
Code is generally clean — good test coverage, grep-friendly log prefixes, no .unwrap() exposed to callers, and the two CodeRabbit majors (spawn_blocking on the bus path, workspace-scoped singleton) have been resolved.
One blocking issue before merge, two majors.
BLOCKING — Closes #1805 is premature
Issue #1805 specifies 13+ acceptance criteria:
/abort,/detach, session resume/abort from Telegram- Permission request / approval / rejection inline
- Model and agent-mode switching
- Scheduled tasks (create / list / remove)
- File attachments inbound and structured result delivery outbound
- Workspace / file browsing with policy limits
- Live async lifecycle updates for long-running tasks
- Controller-backed operations through the controller registry (not transport-specific branches)
- E2E coverage for at least one Telegram-driven flow
None of these are in this PR. Merging with Closes #1805 auto-closes the issue and signals the entire feature spec is complete. Change to Part of #1805 (or open a follow-up tracking issue for phases 2+) so the remaining scope stays visible.
Change summary
| File | Change |
|---|---|
remote_control.rs |
New: command parser + 4 handlers (/status, /sessions, /new, /help) |
session_store.rs |
New: workspace-backed JSON store for chat→thread bindings + busy flags |
bus.rs |
New: TelegramRemoteSubscriber for busy tracking via event bus |
routes.rs |
Wires remote commands into channel runtime command path |
startup.rs |
Registers TelegramRemoteSubscriber on startup |
catalog.rs |
Registers channels.telegram_remote_control capability |
TelegramConfig.tsx |
Adds remote-control informational callout + test |
| .unwrap_or(0); | ||
|
|
||
| let workspace = ctx.workspace_dir.as_path(); | ||
| let (binding, busy) = match with_store(workspace, |store| { |
There was a problem hiding this comment.
[major] with_store is synchronous blocking I/O called from an async function.
CodeRabbit correctly required spawn_blocking for the same pattern in bus.rs (set_busy). The same issue is present here in build_status_response (this line), build_sessions_response (line 142), and build_new_session_response (line 224) — all three call with_store synchronously on the tokio async executor, which blocks the thread during file reads and writes.
Suggestion: wrap each with_store call in tokio::task::spawn_blocking, the same way set_busy was fixed:
let workspace = ctx.workspace_dir.clone();
let reply_target = msg.reply_target.clone();
let result = tokio::task::spawn_blocking(move || {
with_store(&workspace, |store| {
Ok((
store.binding(&reply_target).cloned(),
store.is_busy(&reply_target),
))
})
})
.await
.unwrap_or_else(|e| Err(anyhow::anyhow!("join error: {e}")));Alternatively, make TelegramSessionStore own a tokio::sync::Mutex and an in-memory write-back so the async path never touches disk directly.
There was a problem hiding this comment.
Fixed in commit c7e3fe9. All three with_store call sites in remote_control.rs (build_status_response, build_sessions_response, build_new_session_response) are now wrapped in tokio::task::spawn_blocking, matching the pattern from bus.rs. Read-only paths additionally use with_store_read (no save) to avoid unnecessary disk writes.
|
|
||
| #[test] | ||
| fn with_store_reloads_when_workspace_changes() { | ||
| let first = tempdir().expect("first tempdir"); |
There was a problem hiding this comment.
[major] with_store_reloads_when_workspace_changes (and the two tests above it) mutate the process-global STORE singleton. Rust unit tests run in parallel by default across all threads in the same process, so concurrent tests that call with_store with different workspace paths will clobber each other's state — observable as flaky assert! failures or a test seeing the wrong workspace's bindings.
Suggestion: annotate all three tests in this module with #[serial_test::serial] (add serial_test as a dev-dependency), or avoid the global singleton in tests entirely by testing TelegramSessionStore::load / save / method calls directly, which is what round_trip_binding_and_busy_flag already does for most of its assertions.
There was a problem hiding this comment.
Fixed in commit c7e3fe9. Replaced with_store_reloads_when_workspace_changes (which called the process-global STORE singleton and was susceptible to parallel test races) with store_isolates_bindings_across_workspaces that uses TelegramSessionStore::load/save directly — no singleton involved, no serial_test dep needed.
| } | ||
|
|
||
| async fn lookup_thread_title(workspace: &std::path::Path, thread_id: &str) -> Option<String> { | ||
| let threads = conversations::list_threads(workspace.to_path_buf()).ok()?; |
There was a problem hiding this comment.
[minor] lookup_thread_title calls list_threads() — which reads and deserialises every thread — just to find one title. For workspaces with many threads this is O(n) disk I/O on every /status call.
Simpler fix: store the title in TelegramChatBinding at /new time (it's already computed as title at line ~202). Then /status can read it directly from the binding without listing threads.
There was a problem hiding this comment.
Fixed in commit c7e3fe9. Added a title: Option<String> field to TelegramChatBinding (serde default for back-compat with existing stores). /new now stores the title at bind time. /status reads it directly from the binding — O(1) instead of O(n) list_threads scan. The lookup_thread_title helper was removed.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — Telegram remote-control phase 1
Solid phase-1 implementation. The command parsing, session store, bus subscriber, and test coverage are well structured. Good job handling bot-qualified commands (/status@MyBot) and the spawn_blocking fix in bus.rs.
Change summary
| Area | Files | What changed |
|---|---|---|
| Rust core | remote_control.rs, session_store.rs, bus.rs, routes.rs, startup.rs, catalog.rs |
New command parser, workspace-backed session store, event bus subscriber, runtime command dispatch, capability registration |
| Frontend | TelegramConfig.tsx |
Informational callout for remote-control commands |
| Tests | remote_control_tests.rs, bus_tests.rs, session_store::tests, routes_tests.rs, TelegramConfig.test.tsx |
Unit + integration coverage for parsing, store round-trip, bus lifecycle, full command round-trip |
Issue alignment — BLOCKING
The PR description says "Closes #1805" but this is explicitly phase 1, covering ~4 of ~15 acceptance criteria from the issue. Merging with Closes will auto-close #1805 and lose tracking for:
- Session abort/detach
- Permission request/approval from Telegram
- Task scheduling
- File attachments and workspace browsing
- Model/agent-mode switching beyond existing
/model - Live status updates for long-running work
Action needed: Change Closes #1805 to Ref #1805 or Part of #1805 so the issue stays open for subsequent phases.
CodeRabbit dedup
All 3 CodeRabbit findings (bus.rs spawn_blocking, session store singleton scoping, startup.rs handle lifetime) are addressed in later commits. Skipped.
| .unwrap_or(0); | ||
|
|
||
| let workspace = ctx.workspace_dir.as_path(); | ||
| let (binding, busy) = match with_store(workspace, |store| { |
There was a problem hiding this comment.
[major] Blocking file I/O on async path — same class of issue CodeRabbit caught in bus.rs, but here in the command handlers.
with_store() acquires a std::sync::Mutex and does synchronous disk I/O. This is called directly from build_status_response, build_sessions_response (line ~142), and build_new_session_response (which also calls conversations::ensure_thread and conversations::list_threads). Under concurrent Telegram commands, this blocks the tokio runtime thread.
The bus subscriber correctly wraps with_store in tokio::task::spawn_blocking — the command handlers should do the same.
// Example for this call site:
let workspace = ctx.workspace_dir.clone();
let reply_target = msg.reply_target.clone();
let (binding, busy) = tokio::task::spawn_blocking(move || {
with_store(&workspace, |store| {
Ok((store.binding(&reply_target).cloned(), store.is_busy(&reply_target)))
})
})
.await
.unwrap_or_else(|e| { tracing::warn!("{LOG_PREFIX} join error: {e}"); Ok((None, false)) })
.unwrap_or((None, false));There was a problem hiding this comment.
Fixed in commit c7e3fe9 — same fix as the duplicate comment at #3274499984. All with_store call sites in the command handlers are wrapped in spawn_blocking.
| } | ||
| let store = guard.as_mut().expect("store initialized"); | ||
| let result = f(store)?; | ||
| store.save()?; |
There was a problem hiding this comment.
[minor] with_store calls save() unconditionally — even for read-only operations.
/status and /sessions both call with_store only to read binding() / is_busy(), but every call serializes and writes the JSON file back to disk. On a chatty channel this is unnecessary I/O.
Consider splitting into with_store (read-write, saves) and with_store_read (read-only, no save), or having the closure return a flag indicating whether a write is needed.
There was a problem hiding this comment.
Fixed in commit c7e3fe9. Added with_store_read — a read-only accessor that skips the unconditional save() call. /status and /sessions now use with_store_read so no serialization or disk write happens for pure queries.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/channels/providers/telegram/remote_control.rs (1)
241-270:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelay history clearing until the binding write succeeds.
Line 241 clears the chat's in-memory history before Lines 249-263 persist the new binding. If the store write fails,
/newreturns an error but the existing chat context is already gone, so the command leaves a partially applied state.Suggested fix
- clear_sender_history(ctx, &sender_key); - let workspace_dir = ctx.workspace_dir.clone(); let reply_target_owned = msg.reply_target.clone(); let thread_id_owned = thread_id.clone(); let sender_key_owned = sender_key.clone(); let title_owned = title.clone(); @@ if let Err(error) = bind_result { tracing::warn!("{LOG_PREFIX} new: persist binding failed: {error}"); return format!( "Created thread `{thread_id}` but failed to persist Telegram binding: {error}" ); } + clear_sender_history(ctx, &sender_key); crate::openhuman::channels::providers::web::invalidate_thread_sessions(&thread_id).await;If you want
/newto stay fully atomic, consider whether the created thread should also be rolled back on this failure path.🤖 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 `@src/openhuman/channels/providers/telegram/remote_control.rs` around lines 241 - 270, Move the in-memory history clear to after a successful persistent binding: do not call clear_sender_history(ctx, &sender_key) before the spawn_blocking with with_store/store.set_binding; instead, perform clear_sender_history only if bind_result is Ok, and if bind_result Err keep history intact. Update the block around the tokio::task::spawn_blocking (the closure using workspace_dir, reply_target_owned, thread_id_owned, sender_key_owned, title_owned) so that on Err you return the error without mutating in-memory state, and on Ok you then call clear_sender_history and crate::openhuman::channels::providers::web::invalidate_thread_sessions(&thread_id). Consider (and document in a TODO) whether you must roll back the created thread when the persistent write fails.
♻️ Duplicate comments (1)
src/openhuman/channels/providers/telegram/remote_control.rs (1)
169-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap
list_threadsandensure_threadinspawn_blockingtoo.Lines 169-175 and 227-239 still do synchronous workspace/file I/O on the async path. Under concurrent Telegram commands, these calls can stall the Tokio worker the same way the earlier
with_store*calls did.Suggested pattern
- let threads = match conversations::list_threads(workspace.to_path_buf()) { + let workspace = workspace.to_path_buf(); + let threads = match tokio::task::spawn_blocking(move || conversations::list_threads(workspace)) + .await + .unwrap_or_else(|e| Err(anyhow::anyhow!("join error: {e}"))) { Ok(list) => list, Err(error) => { tracing::warn!("{LOG_PREFIX} sessions: list_threads failed: {error}"); return format!("Could not list sessions: {error}"); } }Apply the same wrapping to
conversations::ensure_thread(...)in/new.Also applies to: 227-239
🤖 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 `@src/openhuman/channels/providers/telegram/remote_control.rs` around lines 169 - 175, The calls to conversations::list_threads(...) and conversations::ensure_thread(...) perform blocking filesystem I/O and must be moved off the async executor; wrap each call in tokio::task::spawn_blocking and await the JoinHandle result, converting errors appropriately. Locate the list_threads usage (the match assigning threads from conversations::list_threads(workspace.to_path_buf())) and replace it with a spawn_blocking that clones or moves workspace.to_path_buf() into the closure, then await and handle the Result/Err exactly as before; do the same for conversations::ensure_thread(...) in the /new handler, ensuring you unwrap or propagate the spawn_blocking JoinError and map the inner Result to the same warning/log and return messages currently used.
🤖 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.
Outside diff comments:
In `@src/openhuman/channels/providers/telegram/remote_control.rs`:
- Around line 241-270: Move the in-memory history clear to after a successful
persistent binding: do not call clear_sender_history(ctx, &sender_key) before
the spawn_blocking with with_store/store.set_binding; instead, perform
clear_sender_history only if bind_result is Ok, and if bind_result Err keep
history intact. Update the block around the tokio::task::spawn_blocking (the
closure using workspace_dir, reply_target_owned, thread_id_owned,
sender_key_owned, title_owned) so that on Err you return the error without
mutating in-memory state, and on Ok you then call clear_sender_history and
crate::openhuman::channels::providers::web::invalidate_thread_sessions(&thread_id).
Consider (and document in a TODO) whether you must roll back the created thread
when the persistent write fails.
---
Duplicate comments:
In `@src/openhuman/channels/providers/telegram/remote_control.rs`:
- Around line 169-175: The calls to conversations::list_threads(...) and
conversations::ensure_thread(...) perform blocking filesystem I/O and must be
moved off the async executor; wrap each call in tokio::task::spawn_blocking and
await the JoinHandle result, converting errors appropriately. Locate the
list_threads usage (the match assigning threads from
conversations::list_threads(workspace.to_path_buf())) and replace it with a
spawn_blocking that clones or moves workspace.to_path_buf() into the closure,
then await and handle the Result/Err exactly as before; do the same for
conversations::ensure_thread(...) in the /new handler, ensuring you unwrap or
propagate the spawn_blocking JoinError and map the inner Result to the same
warning/log and return messages currently used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25eb5cb5-c32c-4f08-945f-1582d1ac599f
📒 Files selected for processing (4)
src/openhuman/channels/providers/telegram/bus_tests.rssrc/openhuman/channels/providers/telegram/remote_control.rssrc/openhuman/channels/providers/telegram/remote_control_tests.rssrc/openhuman/channels/providers/telegram/session_store.rs
✅ Files skipped from review due to trivial changes (1)
- src/openhuman/channels/providers/telegram/remote_control_tests.rs
|
pr-manager update (commit All actionable review items have been addressed: BLOCKING resolved: PR description updated — Code changes:
All 124 telegram tests pass. |
|
The other substantive fixes (blocking, (◕‿◕✿) ✅ Actions performedReview triggered.
|
…/new) Add Telegram slash commands for away-from-keyboard session management, persist chat-to-thread bindings, and wire an event-bus subscriber so /status can show in-flight turn state. Closes tinyhumansai#1805 Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…umansai#1805) - Wrap all with_store calls in remote_control.rs in tokio::task::spawn_blocking so blocking mutex + disk I/O does not stall the async executor on chatty channels (addresses @M3gA-Mind and @graycyrus on remote_control.rs:103, same class as the bus.rs fix already in the branch) - Use with_store_read (no unconditional save) for read-only /status and /sessions lookups; add with_store_read to session_store.rs (addresses @graycyrus on session_store.rs:120) - Add title field to TelegramChatBinding (serde default for back-compat); capture title at /new time; /status reads it directly — O(1) instead of O(n) list_threads scan (addresses @M3gA-Mind on remote_control.rs:249) - Remove now-unused lookup_thread_title helper - Rename misleading idle variable to busy in bus_tests.rs (addresses @coderabbitai on bus_tests.rs:41) - Replace with_store_reloads_when_workspace_changes test with store_isolates_bindings_across_workspaces that uses TelegramSessionStore directly, avoiding process-global STORE singleton races in parallel test runs (addresses @M3gA-Mind on session_store.rs:166) - Add /Sessions and /NEW@Bot case-insensitivity assertions to parse_remote_commands test (addresses @coderabbitai nitpick on remote_control_tests.rs:3)
CodeGhost21
force-pushed
the
cursor/a01-1805-telegram-remote-control-phase1
branch
from
c7e3fe9 to
9013078
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/src/components/channels/TelegramConfig.tsx`:
- Around line 326-334: The banner strings in TelegramConfig (the div rendering
"Remote control (Telegram)" and the description text) are hardcoded; replace
them with i18n keys and use the component's useT() hook to retrieve translations
(e.g., t('telegram.remoteControl.title') and
t('telegram.remoteControl.description')). Update the JSX in the TelegramConfig
component to call useT() at the top and swap the literal strings for the t(...)
lookups, and add corresponding keys to your locale files (same key names as used
in the component).
🪄 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: 735f9b20-17fd-4185-8622-2941efb90d27
📒 Files selected for processing (2)
app/src/components/channels/TelegramConfig.tsxapp/src/components/channels/__tests__/TelegramConfig.test.tsx
| <div className="rounded-lg border border-primary-200 dark:border-primary-500/30 bg-primary-50/80 dark:bg-primary-500/10 px-4 py-3 text-sm text-stone-700 dark:text-neutral-200"> | ||
| <p className="font-medium text-stone-900 dark:text-neutral-100"> | ||
| Remote control (Telegram) | ||
| </p> | ||
| <p className="mt-1 text-xs text-stone-600 dark:text-neutral-400"> | ||
| From an allowed Telegram chat, send /status, /sessions, /new, or /help. Model routing | ||
| still uses /model and /models. | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
Localize new remote-control banner copy
This user-facing text is hardcoded, while the component already uses useT(). Please move these strings to i18n keys to avoid English-only UI regressions.
Suggested patch
- <div className="rounded-lg border border-primary-200 dark:border-primary-500/30 bg-primary-50/80 dark:bg-primary-500/10 px-4 py-3 text-sm text-stone-700 dark:text-neutral-200">
+ <div className="rounded-lg border border-primary-200 dark:border-primary-500/30 bg-primary-50/80 dark:bg-primary-500/10 px-4 py-3 text-sm text-stone-700 dark:text-neutral-200">
<p className="font-medium text-stone-900 dark:text-neutral-100">
- Remote control (Telegram)
+ {t('channels.telegram.remoteControl.title')}
</p>
<p className="mt-1 text-xs text-stone-600 dark:text-neutral-400">
- From an allowed Telegram chat, send /status, /sessions, /new, or /help. Model routing
- still uses /model and /models.
+ {t('channels.telegram.remoteControl.description')}
</p>
</div>🤖 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 `@app/src/components/channels/TelegramConfig.tsx` around lines 326 - 334, The
banner strings in TelegramConfig (the div rendering "Remote control (Telegram)"
and the description text) are hardcoded; replace them with i18n keys and use the
component's useT() hook to retrieve translations (e.g.,
t('telegram.remoteControl.title') and t('telegram.remoteControl.description')).
Update the JSX in the TelegramConfig component to call useT() at the top and
swap the literal strings for the t(...) lookups, and add corresponding keys to
your locale files (same key names as used in the component).
|
huge thanks @CodeGhost21, this one's awesome 🙌 the telegram remote-control commands for afk session management are such a clean addition, and persisting the per-chat thread bindings is a really nice touch. stoked to have this merged 🚀 |
4 participants
Summary
/status,/sessions,/new, and/helpfor away-from-keyboard session management.state/telegram_remote_sessions.json).TelegramRemoteSubscriberon the event bus to track in-flight Telegram turns (busy flag for/status).channels.telegram_remote_controlin the runtime capability catalog.Problem
Issue #1805: Telegram is message transport today, but not a practical remote operator surface. Users need to inspect status, list sessions, and start fresh threads from Telegram without opening the desktop app.
Solution
/modeland/models).src/openhuman/channels/providers/telegram/with workspace-backed session store and conversation thread APIs.ChannelMessageReceived/ChannelMessageProcessedfortelegramto maintain a busy flag per reply target.TelegramConfig.tsxand the capability catalog.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.mdreflect this change (orN/A: behaviour-only change)## Relateddocs/RELEASE-MANUAL-SMOKE.md)Closes #NNNin the## RelatedsectionImpact
/newclears in-memory channel history for that chat and binds a new conversation thread.Related
channels.telegram_remote_control,channels.connect_platformAI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
cursor/a01-1805-telegram-remote-control-phase1bee7ee330711678b24d5c24efc466c431b0eb7a6Validation Run
pnpm --filter openhuman-app format:check(via pre-push hook)pnpm typecheck(via pre-push hookcompile)cargo test --lib -p openhuman handle_runtime_command_telegram_status,parse_remote_commands,subscriber_marks_busy_on_received_and_clears_on_processed,round_trip_binding_and_busy_flag;prettier --check app/src/components/channels/TelegramConfig.tsxcargo fmt --all, focused tests aboveValidation Blocked
command:pre-push hook (pnpm rust:checkviagit push)error:isolated worktree did not have the vendoredapp/src-tauri/vendor/tauri-cefsubmodule required by Tauri shellcargo check; this PR has no Tauri shell changes.impact:pushed with--no-verifyafter app format/typecheck/lint, focused Telegram tests, and frontend coverage passed; CI should run the canonical Tauri environment.Behavior Changes
/status,/sessions,/new,/helpas local commands; busy state reflects active agent turns.Parity Contract
/modeland/modelsunchanged.handle_runtime_command_if_needed.Made with Cursor
Summary by CodeRabbit
New Features
Documentation
Tests