docs: add helper page lifecycle fix implementation plan
🤖 Generated with [Qoder][https://qoder.com]
This commit is contained in:
木炎
@@ -0,0 +1,475 @@
|
||||
# Helper Page Lifecycle Fix & Hidden Domain Support — Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Fix duplicate browser-helper.html pages caused by WebSocket reconnections, add cleanup on Drop, and introduce a config switch for hidden-domain page opening.
|
||||
|
||||
**Architecture:** Three changes: (1) lift `cached_host` from `serve_client()` to the outer `run()` loop so reconnections share one host, (2) enhance `Drop for LiveBrowserCallbackHost` to send a close-page command via browser WS, (3) add `use_hidden_domain: bool` parameter that selects between `sgBrowerserOpenPage`/`sgHideBrowerserOpenPage` and their corresponding close APIs.
|
||||
|
||||
**Tech Stack:** Rust, tungstenite WebSocket crate, SuperRPA browser WS protocol
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Add `use_hidden_domain` field and update `bootstrap_helper_page`
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/browser/callback_host.rs:28` (constant), `:44-51` (struct), `:215-252` (constructor), `:340-359` (bootstrap fn)
|
||||
|
||||
- [ ] **Step 1: Change `HELPER_BOOTSTRAP_ACTION` from constant to a function of `use_hidden_domain`**
|
||||
|
||||
Replace the constant and update `bootstrap_helper_page` to accept and use the flag:
|
||||
|
||||
```rust
|
||||
// DELETE this line:
|
||||
// const HELPER_BOOTSTRAP_ACTION: &str = "sgBrowerserOpenPage";
|
||||
|
||||
// REPLACE bootstrap_helper_page signature and body:
|
||||
fn bootstrap_helper_page(
|
||||
browser_ws_url: &str,
|
||||
request_url: &str,
|
||||
helper_url: &str,
|
||||
use_hidden_domain: bool,
|
||||
) -> Result<(), PipeError> {
|
||||
let (mut websocket, _) = connect(browser_ws_url)
|
||||
.map_err(|err| PipeError::Protocol(format!("browser websocket connect failed: {err}")))?;
|
||||
configure_bootstrap_socket(&mut websocket)?;
|
||||
websocket
|
||||
.send(Message::Text(
|
||||
r#"{"type":"register","role":"web"}"#.to_string().into(),
|
||||
))
|
||||
.map_err(|err| PipeError::Protocol(format!("browser websocket register failed: {err}")))?;
|
||||
let _ = recv_bootstrap_prelude(&mut websocket);
|
||||
let open_action = if use_hidden_domain {
|
||||
"sgHideBrowerserOpenPage"
|
||||
} else {
|
||||
"sgBrowerserOpenPage"
|
||||
};
|
||||
let payload = json!([
|
||||
request_url,
|
||||
open_action,
|
||||
helper_url,
|
||||
])
|
||||
.to_string();
|
||||
websocket
|
||||
.send(Message::Text(payload.into()))
|
||||
.map_err(|err| PipeError::Protocol(format!("helper bootstrap send failed: {err}")))?;
|
||||
Ok(())
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Add new fields to `LiveBrowserCallbackHost`**
|
||||
|
||||
```rust
|
||||
#[derive(Debug)]
|
||||
pub(crate) struct LiveBrowserCallbackHost {
|
||||
host: Arc<BrowserCallbackHost>,
|
||||
shutdown: Arc<AtomicBool>,
|
||||
server_thread: Mutex<Option<JoinHandle<()>>>,
|
||||
command_lock: Mutex<()>,
|
||||
result_timeout: Duration,
|
||||
browser_ws_url: String,
|
||||
use_hidden_domain: bool,
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Update `start_with_browser_ws_url` to accept and store the new parameter**
|
||||
|
||||
```rust
|
||||
impl LiveBrowserCallbackHost {
|
||||
pub(crate) fn start_with_browser_ws_url(
|
||||
browser_ws_url: &str,
|
||||
bootstrap_request_url: &str,
|
||||
ready_timeout: Duration,
|
||||
result_timeout: Duration,
|
||||
use_hidden_domain: bool,
|
||||
) -> Result<Self, PipeError> {
|
||||
let listener = TcpListener::bind("127.0.0.1:0").map_err(|err| {
|
||||
PipeError::Protocol(format!("failed to bind callback host listener: {err}"))
|
||||
})?;
|
||||
listener.set_nonblocking(true).map_err(|err| {
|
||||
PipeError::Protocol(format!("failed to configure callback host listener: {err}"))
|
||||
})?;
|
||||
let origin = format!(
|
||||
"http://{}",
|
||||
listener.local_addr().map_err(|err| {
|
||||
PipeError::Protocol(format!(
|
||||
"failed to resolve callback host listener address: {err}"
|
||||
))
|
||||
})?
|
||||
);
|
||||
let host = Arc::new(BrowserCallbackHost::with_urls(&origin, browser_ws_url));
|
||||
let shutdown = Arc::new(AtomicBool::new(false));
|
||||
let thread_host = host.clone();
|
||||
let thread_shutdown = shutdown.clone();
|
||||
let server_thread = thread::spawn(move || serve_loop(listener, thread_host, thread_shutdown));
|
||||
|
||||
bootstrap_helper_page(browser_ws_url, bootstrap_request_url, host.helper_url(), use_hidden_domain)?;
|
||||
wait_for_helper_ready(host.as_ref(), ready_timeout)?;
|
||||
|
||||
let live_host = Self {
|
||||
host,
|
||||
shutdown,
|
||||
server_thread: Mutex::new(Some(server_thread)),
|
||||
command_lock: Mutex::new(()),
|
||||
result_timeout,
|
||||
browser_ws_url: browser_ws_url.to_string(),
|
||||
use_hidden_domain,
|
||||
};
|
||||
Ok(live_host)
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Fix the inline test struct literal that constructs `LiveBrowserCallbackHost` directly**
|
||||
|
||||
In the `live_callback_host_treats_simulated_mouse_command_as_fire_and_forget` test (around line 1110), add the new fields:
|
||||
|
||||
```rust
|
||||
let host = LiveBrowserCallbackHost {
|
||||
host: Arc::new(BrowserCallbackHost::new()),
|
||||
shutdown: Arc::new(AtomicBool::new(false)),
|
||||
server_thread: Mutex::new(None),
|
||||
command_lock: Mutex::new(()),
|
||||
result_timeout: Duration::from_millis(10),
|
||||
browser_ws_url: "ws://127.0.0.1:12345".to_string(),
|
||||
use_hidden_domain: false,
|
||||
};
|
||||
```
|
||||
|
||||
- [ ] **Step 5: Run build to verify compilation**
|
||||
|
||||
Run: `cargo build 2>&1`
|
||||
Expected: 0 errors. The `HELPER_BOOTSTRAP_ACTION` constant removal and signature changes should all be internally consistent.
|
||||
|
||||
- [ ] **Step 6: Run tests to verify existing behavior is preserved**
|
||||
|
||||
Run: `cargo test -- callback_host 2>&1`
|
||||
Expected: All existing callback_host tests pass (including `live_callback_host_sends_bootstrap_open_page_command` which still checks for `sgBrowerserOpenPage` since no caller passes `true` yet).
|
||||
|
||||
- [ ] **Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add src/browser/callback_host.rs
|
||||
git commit -m "feat(callback_host): add use_hidden_domain param to bootstrap_helper_page"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Enhance `Drop` to close the helper page
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/browser/callback_host.rs:321-328` (Drop impl)
|
||||
|
||||
- [ ] **Step 1: Add `close_helper_page` helper function**
|
||||
|
||||
Add this function near `bootstrap_helper_page` (after line ~360):
|
||||
|
||||
```rust
|
||||
/// Best-effort attempt to close the helper page tab via browser WebSocket.
|
||||
/// Silently ignores all errors — this runs during Drop and must not panic.
|
||||
fn close_helper_page(browser_ws_url: &str, helper_url: &str, use_hidden_domain: bool) {
|
||||
let close_action = if use_hidden_domain {
|
||||
"sgHideBrowerserClosePage"
|
||||
} else {
|
||||
"sgBrowserClosePage"
|
||||
};
|
||||
|
||||
let result: Result<(), Box<dyn std::error::Error>> = (|| {
|
||||
// Use a raw TcpStream with timeouts instead of tungstenite::connect
|
||||
// which does not expose a connection timeout.
|
||||
let addr = browser_ws_url
|
||||
.trim_start_matches("ws://")
|
||||
.trim_start_matches("wss://");
|
||||
let stream = TcpStream::connect_timeout(
|
||||
&addr.parse().map_err(|e| format!("addr parse: {e}"))?,
|
||||
Duration::from_millis(100),
|
||||
)?;
|
||||
stream.set_read_timeout(Some(Duration::from_millis(200)))?;
|
||||
stream.set_write_timeout(Some(Duration::from_millis(200)))?;
|
||||
let (mut websocket, _) = tungstenite::client(
|
||||
browser_ws_url,
|
||||
stream,
|
||||
)?;
|
||||
websocket.send(Message::Text(
|
||||
r#"{"type":"register","role":"web"}"#.to_string().into(),
|
||||
))?;
|
||||
// Drain the welcome prelude (best-effort, ignore timeout).
|
||||
let _ = websocket.read();
|
||||
let payload = json!([helper_url, close_action, helper_url]).to_string();
|
||||
websocket.send(Message::Text(payload.into()))?;
|
||||
Ok(())
|
||||
})();
|
||||
|
||||
if let Err(err) = result {
|
||||
eprintln!("close_helper_page best-effort failed (harmless): {err}");
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Update `Drop for LiveBrowserCallbackHost` to call `close_helper_page`**
|
||||
|
||||
```rust
|
||||
impl Drop for LiveBrowserCallbackHost {
|
||||
fn drop(&mut self) {
|
||||
// Best-effort: tell the browser to close the helper page tab.
|
||||
close_helper_page(
|
||||
&self.browser_ws_url,
|
||||
self.host.helper_url(),
|
||||
self.use_hidden_domain,
|
||||
);
|
||||
|
||||
self.shutdown.store(true, Ordering::Relaxed);
|
||||
if let Some(handle) = self.server_thread.lock().unwrap().take() {
|
||||
let _ = handle.join();
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Run build to verify compilation**
|
||||
|
||||
Run: `cargo build 2>&1`
|
||||
Expected: 0 errors. `close_helper_page` uses types already imported (`TcpStream`, `Duration`, `json!`, `Message`).
|
||||
|
||||
- [ ] **Step 4: Run tests**
|
||||
|
||||
Run: `cargo test -- callback_host 2>&1`
|
||||
Expected: All pass. The Drop enhancement is best-effort and the test helper constructs hosts with `server_thread: Mutex::new(None)`, so Drop completes cleanly.
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add src/browser/callback_host.rs
|
||||
git commit -m "feat(callback_host): close helper page on Drop via browser WS"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Lift `cached_host` to outer loop and update `serve_client` signature
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/service/mod.rs:72-96` (run loop)
|
||||
- Modify: `src/service/server.rs:232-241` (serve_client signature and cached_host init)
|
||||
|
||||
- [ ] **Step 1: Change `serve_client` to accept `cached_host` as a parameter**
|
||||
|
||||
In `src/service/server.rs`, change the function signature and remove the local `cached_host` variable:
|
||||
|
||||
```rust
|
||||
pub fn serve_client(
|
||||
context: &AgentRuntimeContext,
|
||||
session: &ServiceSession,
|
||||
sink: Arc<ServiceEventSink>,
|
||||
browser_ws_url: &str,
|
||||
mac_policy: &MacPolicy,
|
||||
cached_host: &mut Option<Arc<LiveBrowserCallbackHost>>,
|
||||
) -> Result<(), PipeError> {
|
||||
// DELETE the line: let mut cached_host: Option<Arc<LiveBrowserCallbackHost>> = None;
|
||||
|
||||
loop {
|
||||
// ... rest of function body unchanged, `cached_host` is now the parameter
|
||||
```
|
||||
|
||||
The body references to `cached_host` remain identical — they just operate on the borrowed mutable reference instead of a local variable.
|
||||
|
||||
- [ ] **Step 2: Update `start_with_browser_ws_url` call to pass `false` for `use_hidden_domain`**
|
||||
|
||||
In `src/service/server.rs`, at the `LiveBrowserCallbackHost::start_with_browser_ws_url` call (around line 288), add the `false` argument:
|
||||
|
||||
```rust
|
||||
match LiveBrowserCallbackHost::start_with_browser_ws_url(
|
||||
browser_ws_url,
|
||||
&bootstrap_url,
|
||||
Duration::from_secs(15),
|
||||
BROWSER_RESPONSE_TIMEOUT,
|
||||
false, // use_hidden_domain: visible tab for now
|
||||
) {
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Lift `cached_host` into `run()` in `mod.rs`**
|
||||
|
||||
In `src/service/mod.rs`, declare `cached_host` before the loop and pass it to `serve_client`:
|
||||
|
||||
```rust
|
||||
// Add this import at the top of the function or file:
|
||||
use crate::browser::callback_host::LiveBrowserCallbackHost;
|
||||
|
||||
// Before the loop (after line 64, after `let session = ...`):
|
||||
let mut cached_host: Option<Arc<LiveBrowserCallbackHost>> = None;
|
||||
|
||||
loop {
|
||||
let (stream, _) = listener.accept()?;
|
||||
let websocket = accept(stream)
|
||||
.map_err(|err| PipeError::Protocol(format!("service websocket accept failed: {err}")))?;
|
||||
let sink = Arc::new(ServiceEventSink::from_websocket(websocket));
|
||||
match session.try_attach_client() {
|
||||
Ok(()) => {
|
||||
let result = serve_client(
|
||||
&runtime_context,
|
||||
&session,
|
||||
sink.clone(),
|
||||
browser_ws_url,
|
||||
&mac_policy,
|
||||
&mut cached_host,
|
||||
);
|
||||
session.detach_client();
|
||||
match result {
|
||||
Ok(()) | Err(PipeError::PipeClosed) => {}
|
||||
Err(err) => return Err(err),
|
||||
}
|
||||
}
|
||||
Err(message) => {
|
||||
sink.send_service_message(message)?;
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Update the `pub use` export if needed**
|
||||
|
||||
Check `src/service/mod.rs:17`:
|
||||
```rust
|
||||
pub use server::{serve_client, ServiceEventSink, ServiceSession};
|
||||
```
|
||||
The signature change is compatible — `serve_client` is still public with an added parameter. Any external callers will get a compile error guiding them to add the parameter, which is the desired behavior.
|
||||
|
||||
- [ ] **Step 5: Run build to verify compilation**
|
||||
|
||||
Run: `cargo build 2>&1`
|
||||
Expected: 0 errors. If there are external test files calling `serve_client`, they will fail here and need the new parameter added.
|
||||
|
||||
- [ ] **Step 6: Run full test suite**
|
||||
|
||||
Run: `cargo test 2>&1`
|
||||
Expected: All tests pass. External test files that call `serve_client` indirectly through the service protocol tests should still work because they use the WS protocol layer, not `serve_client` directly. (Verified: grep found 0 test files referencing `serve_client` or `LiveBrowserCallbackHost`.)
|
||||
|
||||
- [ ] **Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add src/service/mod.rs src/service/server.rs
|
||||
git commit -m "fix(service): lift cached_host to outer loop to prevent duplicate helper pages"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Add tests for hidden domain bootstrap
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/browser/callback_host.rs` (inline tests module, around line 1071)
|
||||
|
||||
- [ ] **Step 1: Update existing `live_callback_host_sends_bootstrap_open_page_command` test**
|
||||
|
||||
The test currently calls `start_with_browser_ws_url` with 4 args. Add the 5th arg `false`:
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn live_callback_host_sends_bootstrap_open_page_command() {
|
||||
let (ws_url, frames, handle) = start_fake_browser_status_server();
|
||||
|
||||
let result = LiveBrowserCallbackHost::start_with_browser_ws_url(
|
||||
&ws_url,
|
||||
"https://www.zhihu.com",
|
||||
Duration::from_millis(100),
|
||||
Duration::from_millis(50),
|
||||
false,
|
||||
);
|
||||
assert!(result.is_err(), "expected timeout because no real helper page loads");
|
||||
drop(result);
|
||||
handle.join().unwrap();
|
||||
|
||||
let sent = frames.lock().unwrap().clone();
|
||||
assert!(
|
||||
sent.iter().any(|frame| frame.contains("sgBrowerserOpenPage")),
|
||||
"bootstrap should send sgBrowerserOpenPage to the browser WS; sent frames: {sent:?}"
|
||||
);
|
||||
assert!(
|
||||
sent.iter().any(|frame| frame.contains("/sgclaw/browser-helper.html")),
|
||||
"bootstrap should include the helper page URL; sent frames: {sent:?}"
|
||||
);
|
||||
assert!(
|
||||
sent.iter().any(|frame| frame.contains("https://www.zhihu.com")),
|
||||
"bootstrap requestUrl should be the provided page URL; sent frames: {sent:?}"
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Add new test for hidden domain bootstrap**
|
||||
|
||||
Add this test after the existing one:
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn live_callback_host_hidden_domain_sends_hide_open_page_command() {
|
||||
let (ws_url, frames, handle) = start_fake_browser_status_server();
|
||||
|
||||
let result = LiveBrowserCallbackHost::start_with_browser_ws_url(
|
||||
&ws_url,
|
||||
"https://www.zhihu.com",
|
||||
Duration::from_millis(100),
|
||||
Duration::from_millis(50),
|
||||
true,
|
||||
);
|
||||
assert!(result.is_err(), "expected timeout because no real helper page loads");
|
||||
drop(result);
|
||||
handle.join().unwrap();
|
||||
|
||||
let sent = frames.lock().unwrap().clone();
|
||||
assert!(
|
||||
sent.iter().any(|frame| frame.contains("sgHideBrowerserOpenPage")),
|
||||
"hidden domain bootstrap should send sgHideBrowerserOpenPage; sent frames: {sent:?}"
|
||||
);
|
||||
assert!(
|
||||
!sent.iter().any(|frame| {
|
||||
frame.contains("\"sgBrowerserOpenPage\"")
|
||||
}),
|
||||
"hidden domain bootstrap should NOT send visible sgBrowerserOpenPage; sent frames: {sent:?}"
|
||||
);
|
||||
assert!(
|
||||
sent.iter().any(|frame| frame.contains("/sgclaw/browser-helper.html")),
|
||||
"bootstrap should include the helper page URL; sent frames: {sent:?}"
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Run all callback_host tests**
|
||||
|
||||
Run: `cargo test -- callback_host 2>&1`
|
||||
Expected: All 3 tests pass:
|
||||
- `live_callback_host_sends_bootstrap_open_page_command` — regression, visible domain
|
||||
- `live_callback_host_hidden_domain_sends_hide_open_page_command` — new, hidden domain
|
||||
- `live_callback_host_treats_simulated_mouse_command_as_fire_and_forget` — unchanged
|
||||
|
||||
- [ ] **Step 4: Run full test suite**
|
||||
|
||||
Run: `cargo test 2>&1`
|
||||
Expected: All tests pass.
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add src/browser/callback_host.rs
|
||||
git commit -m "test(callback_host): add hidden domain bootstrap test"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 5: Full build verification
|
||||
|
||||
**Files:** None (verification only)
|
||||
|
||||
- [ ] **Step 1: Clean build**
|
||||
|
||||
Run: `cargo build 2>&1`
|
||||
Expected: 0 errors. Warnings about dead code in unrelated modules are acceptable.
|
||||
|
||||
- [ ] **Step 2: Full test suite**
|
||||
|
||||
Run: `cargo test 2>&1`
|
||||
Expected: All tests pass. The pre-existing `lineloss_period_resolver_prompts_for_missing_period` failure (from previous work) is known and unrelated.
|
||||
|
||||
- [ ] **Step 3: Verify the key behavioral changes in code**
|
||||
|
||||
Manually confirm:
|
||||
1. `src/service/mod.rs` — `cached_host` is declared BEFORE the `loop`, not inside `serve_client`
|
||||
2. `src/browser/callback_host.rs` — `Drop::drop` calls `close_helper_page` before shutdown
|
||||
3. `src/browser/callback_host.rs` — `bootstrap_helper_page` uses `"sgHideBrowerserOpenPage"` when `use_hidden_domain == true` and `"sgBrowerserOpenPage"` when `false`
|
||||
4. `src/service/server.rs` — `start_with_browser_ws_url` call passes `false` as `use_hidden_domain`
|
||||
Reference in New Issue
Block a user