Add live_url action, tracked browser sessions listing, and web UI integration#532
Add live_url action, tracked browser sessions listing, and web UI integration#532
live_url action, tracked browser sessions listing, and web UI integration#532Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR adds a Key changes:
Confidence Score: 4/5Safe to merge after adding a timeout to One P1 issue:
Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent as LLM Agent
participant BrowserTool as BrowserTool (crates/tools)
participant BrowserManager as BrowserManager (crates/browser)
participant BrowserPool as BrowserPool
participant Sandbox as Sandbox Container (HTTP :9222)
participant TRACKED as TRACKED_BROWSER_SESSIONS (global static)
Agent->>BrowserTool: execute({action: "live_url"})
BrowserTool->>BrowserTool: inject sandbox mode + saved session_id
BrowserTool->>BrowserManager: handle_request(LiveUrl{interactive:true})
BrowserManager->>BrowserPool: get_or_create(session_id, sandbox=true)
BrowserPool-->>BrowserManager: sid
BrowserManager->>BrowserPool: get_page(&sid) [ensure page target exists]
BrowserPool-->>BrowserManager: Page
BrowserManager->>BrowserPool: sandbox_http_url(&sid)
BrowserPool-->>BrowserManager: "http://sandbox-host:9222"
BrowserManager->>Sandbox: GET /json/list [no timeout]
Sandbox-->>BrowserManager: [{devtoolsFrontendUrl: "/devtools/..."}]
BrowserManager-->>BrowserTool: BrowserResponse{live_url: "http://..."}
BrowserTool->>TRACKED: save_session(key, sid, sandboxed=true)
BrowserTool-->>Agent: {live_url: "http://sandbox:9222/devtools/..."}
Note over Agent,TRACKED: GET /api/browser/sessions
participant WebUI as Web UI (Settings - Tools)
WebUI->>WebUI: fetch("/api/browser/sessions")
WebUI->>TRACKED: list_tracked_browser_sessions()
TRACKED-->>WebUI: Vec<BrowserSessionOverview>
WebUI-->>WebUI: render sessions table
Reviews (1): Last reviewed commit: "feat(web): show tracked browser sessions..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let response = reqwest::get(&endpoint) | ||
| .await | ||
| .map_err(|e| Error::Cdp(format!("failed to query browser targets: {e}")))?; |
There was a problem hiding this comment.
reqwest::get — can hang indefinitely
reqwest::get() uses a one-off client with no timeout configured (the workspace's reqwest declaration has no timeout feature). If the sandbox browser's HTTP endpoint stalls or the container is unhealthy, this await will block forever, leaving the agent tool in a hung state with no way to recover.
Add an explicit timeout using a short-lived reqwest::Client:
| let response = reqwest::get(&endpoint) | |
| .await | |
| .map_err(|e| Error::Cdp(format!("failed to query browser targets: {e}")))?; | |
| let client = reqwest::Client::builder() | |
| .timeout(std::time::Duration::from_secs(10)) | |
| .build() | |
| .map_err(|e| Error::Cdp(format!("failed to build HTTP client: {e}")))?; | |
| let response = client | |
| .get(&endpoint) | |
| .send() | |
| .await | |
| .map_err(|e| Error::Cdp(format!("failed to query browser targets: {e}")))?; |
| static TRACKED_BROWSER_SESSIONS: LazyLock<StdRwLock<HashMap<String, BrowserSessionOverview>>> = | ||
| LazyLock::new(|| StdRwLock::new(HashMap::new())); | ||
|
|
||
| pub fn list_tracked_browser_sessions() -> Vec<BrowserSessionOverview> { | ||
| let guard = TRACKED_BROWSER_SESSIONS | ||
| .read() | ||
| .unwrap_or_else(|e| e.into_inner()); | ||
| let mut items: Vec<BrowserSessionOverview> = guard.values().cloned().collect(); | ||
| items.sort_by(|a, b| b.last_seen_unix_ms.cmp(&a.last_seen_unix_ms)); | ||
| items |
There was a problem hiding this comment.
BrowserTool instances exist
The eviction logic in save_session only checks session_ids.len() (the per-instance local map). If multiple BrowserTool instances exist in the same process, each instance adds entries to TRACKED_BROWSER_SESSIONS but the global cap is never enforced — the global can grow to N × MAX_TRACKED_SESSIONS entries (128 per instance). The doc comment on TRACKED_BROWSER_SESSIONS implies it is bounded, but this invariant only holds when there is exactly one live instance.
Additionally, after the eviction block releases the TRACKED_BROWSER_SESSIONS lock and before the second acquisition for the insert (lines 139–147), another concurrent save_session call from a different instance can interleave. Consider merging the two TRACKED_BROWSER_SESSIONS write-lock acquisitions inside save_session into one to remove that TOCTOU window and to apply a global cap atomically.
| #[tokio::test] | ||
| async fn tracked_browser_sessions_are_listed() { | ||
| let config = moltis_config::schema::BrowserConfig { | ||
| enabled: true, | ||
| ..Default::default() | ||
| }; | ||
| let tool = BrowserTool::from_config(&config).unwrap(); | ||
|
|
||
| tool.save_session("web:session:list", "browser-session-list", true) | ||
| .await; | ||
|
|
||
| let list = list_tracked_browser_sessions(); | ||
| assert!(list.iter().any(|entry| { | ||
| entry.chat_session_key == "web:session:list" | ||
| && entry.browser_session_id == "browser-session-list" | ||
| && entry.sandboxed | ||
| })); | ||
| } |
There was a problem hiding this comment.
TRACKED_BROWSER_SESSIONS is a process-level LazyLock<StdRwLock<...>> and all tests mutate it. Rust runs unit tests in parallel by default; entries inserted by saved_browser_sessions_are_scoped_by_chat_session, session_cache_evicts_when_full, or clearing_one_chat_session_keeps_other_browser_sessions will be visible in tracked_browser_sessions_are_listed. In the eviction test, a parallel test inserting into the global could cause the eviction branch to remove that test's entry from the global (because the evict key is taken from the local session_ids, and the same string key could have been inserted there by a different BrowserTool instance in parallel).
The any() predicate makes tracked_browser_sessions_are_listed resilient today, but the session_cache_evicts_when_full test's global-side assertions could flake. Consider using a per-test key prefix derived from the test name or using #[serial] (via the serial_test crate) for the tests that touch this static.
| var browserSessionsResult = results[2]; | ||
| var nextBrowserSessions = []; | ||
| if (browserSessionsResult.status === "fulfilled") { | ||
| nextBrowserSessions = browserSessionsResult.value | ||
| .json() | ||
| .then((payload) => (Array.isArray(payload?.sessions) ? payload.sessions : [])) | ||
| .catch(() => []); | ||
| } | ||
| setToolData(nextToolData); | ||
| setNodeInventory(nextNodeInventory); | ||
| return Promise.resolve(nextBrowserSessions); |
There was a problem hiding this comment.
setToolData and setNodeInventory are called in the first .then(), but setLoadingTools(false) lives in the second .then() (after response.json() has been awaited). This means React will render an intermediate state where tool data is populated but loadingTools is still true, causing a brief redundant spinner frame. Consider moving all three set* calls into the second handler so all state is set atomically:
.then((nextBrowserSessions) => {
setToolData(nextToolData);
setNodeInventory(nextNodeInventory);
setBrowserSessions(nextBrowserSessions);
setLoadingTools(false);
})(Move setToolData and setNodeInventory calls out of the first .then() and into the second one, after nextBrowserSessions has resolved.)
1 participant
Motivation
live_url/debugging.Description
BrowserAction::LiveUrlaction and defaultinteractiveflag viatypes.rs, and extendBrowserResponsewithlive_urland awith_live_urlhelper.BrowserManager::live_urlwhich queries the sandboxed browser HTTP endpoint and returns the DevTools frontend URL; addfetch_devtools_live_urlusingreqwestand addreqwestto thecrates/browserdependencies.BrowserPool::sandbox_http_urlto expose the sandbox HTTP base URL for a session and wire usage fromlive_urlto ensure a page target exists before querying/json/list.crates/tools::browserwith a globalTRACKED_BROWSER_SESSIONSmap andlist_tracked_browser_sessions()helper, updatesave_session/clear_sessionto maintain this map, and addBrowserSessionOverviewtype.api_browser_sessions_handleratGET /api/browser/sessionsand add the route in the web server router.page-settings.js) to fetch and display the tracked browser sessions table in Settings → Tools, and update docs (browser-automation.md) and tool help text/schema to includelive_url.Testing
test_browser_action_live_url_deserialize_defaults_interactiveincrates/browserto verifylive_urldeserializes with defaultinteractive = true, and it passes.crates/toolsasync tests (saved_browser_sessions_are_scoped_by_chat_session,empty_session_id_is_not_saved,session_cache_evicts_when_full,clearing_one_chat_session_keeps_other_browser_sessions,tracked_browser_sessions_are_listed) to cover session tracking behavior, and they pass undercargo test.cargo testacross the workspace and all automated tests succeeded.Codex Task