feat(tauri): webview_accounts ingest for provider surfaces#801
Conversation
📝 WalkthroughWalkthroughAdds an async side-channel that forwards Changes
Sequence Diagram(s)sequenceDiagram
participant WVE as Webview Event<br/>(Recipe Event)
participant EXT as Event Extraction &<br/>Normalization
participant RPC as JSON-RPC<br/>Formatter
participant HTTP as HTTP Client<br/>(reqwest)
participant EP as External RPC<br/>Endpoint
participant LOG as Logger
WVE->>EXT: webview_recipe_event triggered<br/>(args.payload)
EXT->>EXT: Extract fields (entity_id, thread_id,<br/>title, sender, deep_link,<br/>requires_attention, timestamp)
EXT->>RPC: Normalized event data
RPC->>RPC: Wrap in JSON-RPC request<br/>(method, params)
RPC->>HTTP: POST with 10s timeout
HTTP->>EP: Send JSON-RPC request
alt Success Path
EP-->>HTTP: HTTP success response
HTTP-->>RPC: Decode response
RPC->>RPC: Check for RPC error field
alt No RPC Error
RPC->>LOG: Log successful forward (info/debug)
else RPC Error Present
RPC->>LOG: Log RPC error warning
end
else Failure Path
EP-->>HTTP: HTTP error or timeout
HTTP->>LOG: Log HTTP failure warning
else Decode Error
RPC->>LOG: Log decode error warning
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pairs with core provider_surfaces + app queue polling; merge after app PR if IPC shape depends on UI rollout. Made-with: Cursor
Fixes E0382 partial move when building WebviewEvent after post_provider_surfaces_event. Made-with: Cursor
jwalin-shah
force-pushed
the
pr/tauri-webview-retrieval-v1
branch
from
fa0aae0 to
6fe24e8
Compare
Both std::time::Duration (from this PR) and the cfg-gated
std::sync::{mpsc::sync_channel, OnceLock} (from main) are needed;
keep both import lines.
senamakel
left a comment
senamakel
left a comment
There was a problem hiding this comment.
PR #801 — feat(tauri): webview_accounts ingest bridge for provider surfaces
Walkthrough
This PR wires up a new fire-and-forget bridge in the Tauri shell's webview_recipe_event handler: whenever a provider webview fires an ingest event, the Tauri process normalises the raw payload into a ProviderEvent-shaped JSON struct and forwards it to the openhuman.provider_surfaces_ingest_event RPC endpoint on the core sidecar. The normalisation layer handles the naming divergence between provider schemas (e.g. threadId / chatId / conversationId) and the canonical ProviderEvent struct. A fresh reqwest client is built per call with a 10-second timeout. Errors are logged at warn level and swallowed — the existing webview:event emission path is unchanged.
Changes
| File | Summary |
|---|---|
app/src-tauri/src/webview_accounts/mod.rs |
Adds helper functions for payload extraction, timestamp normalisation, event normalisation, and an async HTTP bridge to openhuman.provider_surfaces_ingest_event; hooks the bridge into webview_recipe_event. Also had an unresolved merge-conflict marker in the import block (fixed in this review). |
Actionable comments (3)
1. app/src-tauri/src/webview_accounts/mod.rs:24-29 — Unresolved merge conflict marker (BLOCKER, fixed)
The file contained a raw <<<<<<< HEAD / ======= / >>>>>>> main conflict block in the use section. cargo check would have failed on CI and the binary would not build. Both import lines were needed: use std::time::Duration (from this PR) and the #[cfg(all(feature = "cef", target_os = "linux"))]-gated mpsc::sync_channel, OnceLock block (from main).
Fixed in this review — commit 70c00ecf; import ordering corrected by cargo fmt in 99e21fbc.
2. app/src-tauri/src/webview_accounts/mod.rs:479-498 — New reqwest::Client constructed on every ingest event (major)
post_provider_surfaces_event calls reqwest::Client::builder().build() on every invocation. On an active account with frequent ingest events this creates a new TCP connection pool each call, adds latency, and wastes file descriptors. The Tauri Axum/reqwest ecosystem expects a shared client.
Suggested change: store a reqwest::Client in WebviewAccountsState (initialised once in the Default impl) and pass it through, or use once_cell::sync::Lazy at module level:
// Before (inside post_provider_surfaces_event)
let client = reqwest::Client::builder()
.timeout(Duration::from_secs(10))
.build()
.map_err(|e| format!("http client: {e}"))?;
// After — module-level singleton
static CORE_RPC_CLIENT: once_cell::sync::Lazy<reqwest::Client> =
once_cell::sync::Lazy::new(|| {
reqwest::Client::builder()
.timeout(Duration::from_secs(10))
.build()
.expect("core RPC client init")
});
// then inside post_provider_surfaces_event:
let resp = CORE_RPC_CLIENT.post(&url).json(&body).send().await...3. app/src-tauri/src/webview_accounts/mod.rs:445-446 — requires_attention default is too aggressive (minor)
When the provider sends an event with no explicit requires_attention field, the bridge sets it to true whenever sender_name or snippet is non-None. Almost every ingest event for active conversations will have a sender and snippet, so the queue will fill with items marked actionable regardless of whether the user actually needs to respond. The original ProviderEvent struct defaults requires_attention to false.
// Before
let requires_attention = payload_bool(&args.payload, "requires_attention")
.unwrap_or(unread > 0 || sender_name.is_some() || snippet.is_some());
// After — conservative default matching ProviderEvent::default
let requires_attention = payload_bool(&args.payload, "requires_attention")
.unwrap_or(unread > 0);This still promotes events with explicit unread counts without falsely flagging every message that merely has a sender name.
Nitpicks
app/src-tauri/src/webview_accounts/mod.rs:477—std::env::var("OPENHUMAN_CORE_RPC_URL")is evaluated on every call. Cache the URL alongside the client (e.g. in theLazyinitializer orWebviewAccountsState) to avoid repeated env lookups under high event volume.app/src-tauri/src/webview_accounts/mod.rs:472—"id": 1is a hardcoded JSON-RPC request ID. Any JSON-RPC server that correlates on id will always see id=1. Consider"id": null(notifications) or a monotonic counter if correlation matters.- No unit tests for
normalize_provider_surfaces_eventorevent_timestamp_rfc3339. These are pure functions and easily unit-tested; the existing test block at the bottom of the file is the right place.
Verified / looks good
- RPC method name
openhuman.provider_surfaces_ingest_eventcorrectly matches therpc_method_nameconvention (openhuman.{namespace}_{function}=openhuman.provider_surfaces_ingest_event). raw_payloadfield is present inProviderEventandProviderEventuses#[serde(deny_unknown_fields)]— no silent drops.- Errors from
post_provider_surfaces_eventare logged and swallowed; thewebview:eventemit path is unaffected (no regression for existing consumers). webview_recipe_eventalready validates that the invoking webview label matches the account, so this bridge cannot be triggered by a malicious sibling webview.cargo checkpasses after the merge-conflict fix (only pre-existing unrelated warnings).
senamakel
left a comment
senamakel
left a comment
There was a problem hiding this comment.
Approving after resolving the unresolved merge-conflict marker in the import block (commits 70c00ec + 99e21fb on the PR branch). The bridge logic, field normalisation, error handling, and RPC method name are all correct. Two non-blocking follow-ups worth addressing before the queue sees heavy load: (1) share a single reqwest::Client instance instead of constructing one per event, and (2) narrow the requires_attention default to unread > 0 only. Full findings in the review comment above.
![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)
app/src-tauri/src/webview_accounts/mod.rs (1)
474-485: Reuse a singlereqwest::Clientacross events.A new
reqwest::Client(and a fresh connection pool / TLS state) is built on every recipe event. For an ingest stream that fires on the 2s recipe poll across N accounts, this throws away keep-alive and re-allocates TLS each time. Hoist the client into aOnceLockor store it onWebviewAccountsStateand reuse it.Also worth reading the URL once at startup; per-call
std::env::varis fine but fixes an env snapshot per request rather than at process init.♻️ Sketch
use std::sync::OnceLock; static INGEST_CLIENT: OnceLock<reqwest::Client> = OnceLock::new(); fn ingest_client() -> &'static reqwest::Client { INGEST_CLIENT.get_or_init(|| { reqwest::Client::builder() .timeout(Duration::from_secs(10)) .build() .expect("build reqwest client") }) }Then in
post_provider_surfaces_event:- let client = reqwest::Client::builder() - .timeout(Duration::from_secs(10)) - .build() - .map_err(|e| format!("http client: {e}"))?; - let resp = client + let resp = ingest_client() .post(&url) .json(&body) .send() .await .map_err(|e| format!("POST {url}: {e}"))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src-tauri/src/webview_accounts/mod.rs` around lines 474 - 485, The code currently builds a new reqwest::Client and reads OPENHUMAN_CORE_RPC_URL on every call (see the client builder and std::env::var usage in the post_provider_surfaces_event flow), which discards keep-alive/TLS state; fix this by creating a single shared client and cached URL at startup (e.g. store reqwest::Client in a static OnceLock or as a field on WebviewAccountsState and read OPENHUMAN_CORE_RPC_URL once during initialization), then use that shared client and cached URL in post_provider_surfaces_event (replace the per-call Client::builder() and env::var calls with references to the initialized client and URL).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src-tauri/src/webview_accounts/mod.rs`:
- Around line 441-443: The current default for requires_attention (computed via
payload_bool(&args.payload, "requires_attention").unwrap_or(...)) fires for
almost every ingest because it treats any message-shaped payload (sender_name or
snippet present) as requiring attention; change that fallback to a stricter
heuristic such as unwrap_or(unread > 0) or unwrap_or(unread > 0 &&
sender_name.is_some()) so requires_attention only becomes true for actual
new/unread activity; update the expression around requires_attention,
referencing the variables unread, sender_name, snippet and the payload_bool call
to implement the chosen tighter rule.
- Around line 375-460: Add unit tests for normalize_provider_surfaces_event and
the helper functions to cover the fallback and heuristic paths: write tests in
the existing tests module that assert entity_id precedence (payload keys
"entity_id" -> "threadId" -> "chatId" -> "snapshotKey" -> synthesized fallback
when none present), verify first_message_field returns the first message field
when messages is an array and returns None when messages is missing or not an
array, test event_timestamp_rfc3339 with None (produces now), a valid millis
value (produces expected RFC3339 string) and an invalid millis (ensure fallback
behavior), and exercise requires_attention resolution for explicit
requires_attention true/false as well as derived true when unread>0 and when
sender_name or snippet exist; call normalize_provider_surfaces_event with
crafted RecipeEventArgs payloads and assert fields ("entity_id", "thread_id",
"timestamp", "requires_attention", "snippet", "sender_name") in the returned
JSON.
- Around line 2346-2354: The current code awaits
post_provider_surfaces_event(&args).await before calling
app.emit("webview:event", …), which blocks the IPC reply on slow/unavailable
core RPC; instead, spawn the POST call onto the async runtime so the emit
happens immediately. Replace the direct await with a spawned task (e.g., using
tauri::async_runtime::spawn or tokio::spawn) that moves/clones args and inside
the task calls post_provider_surfaces_event(args).await and logs any Err as
before; keep the existing app.emit("webview:event", …) path
synchronous/unblocked so the webview event is emitted immediately. Ensure
spawned task captures necessary variables (args, process logger) by value and
preserves the original warning log behavior for failures.
---
Nitpick comments:
In `@app/src-tauri/src/webview_accounts/mod.rs`:
- Around line 474-485: The code currently builds a new reqwest::Client and reads
OPENHUMAN_CORE_RPC_URL on every call (see the client builder and std::env::var
usage in the post_provider_surfaces_event flow), which discards keep-alive/TLS
state; fix this by creating a single shared client and cached URL at startup
(e.g. store reqwest::Client in a static OnceLock or as a field on
WebviewAccountsState and read OPENHUMAN_CORE_RPC_URL once during
initialization), then use that shared client and cached URL in
post_provider_surfaces_event (replace the per-call Client::builder() and
env::var calls with references to the initialized client and URL).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fc18a49-1565-4ee4-b0da-01c5bfd2b76e
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
app/src-tauri/src/webview_accounts/mod.rs
| fn payload_string(payload: &serde_json::Value, key: &str) -> Option<String> { | ||
| payload | ||
| .get(key) | ||
| .and_then(|v| v.as_str()) | ||
| .map(str::trim) | ||
| .filter(|s| !s.is_empty()) | ||
| .map(str::to_string) | ||
| } | ||
|
|
||
| fn payload_bool(payload: &serde_json::Value, key: &str) -> Option<bool> { | ||
| payload.get(key).and_then(|v| v.as_bool()) | ||
| } | ||
|
|
||
| fn payload_i64(payload: &serde_json::Value, key: &str) -> Option<i64> { | ||
| payload.get(key).and_then(|v| v.as_i64()) | ||
| } | ||
|
|
||
| fn first_message_field(payload: &serde_json::Value, key: &str) -> Option<String> { | ||
| payload | ||
| .get("messages") | ||
| .and_then(|v| v.as_array()) | ||
| .and_then(|messages| messages.first()) | ||
| .and_then(|message| message.get(key)) | ||
| .and_then(|v| v.as_str()) | ||
| .map(str::trim) | ||
| .filter(|s| !s.is_empty()) | ||
| .map(str::to_string) | ||
| } | ||
|
|
||
| fn event_timestamp_rfc3339(ts_ms: Option<i64>) -> String { | ||
| ts_ms | ||
| .and_then(|ts| Utc.timestamp_millis_opt(ts).single()) | ||
| .unwrap_or_else(Utc::now) | ||
| .to_rfc3339() | ||
| } | ||
|
|
||
| fn normalize_provider_surfaces_event(args: &RecipeEventArgs) -> Option<serde_json::Value> { | ||
| if args.kind != "ingest" { | ||
| return None; | ||
| } | ||
|
|
||
| let entity_id = payload_string(&args.payload, "entity_id") | ||
| .or_else(|| payload_string(&args.payload, "threadId")) | ||
| .or_else(|| payload_string(&args.payload, "chatId")) | ||
| .or_else(|| payload_string(&args.payload, "snapshotKey")) | ||
| .unwrap_or_else(|| { | ||
| format!( | ||
| "{}:{}:{}", | ||
| args.provider, | ||
| args.account_id, | ||
| args.ts.unwrap_or_else(|| Utc::now().timestamp_millis()) | ||
| ) | ||
| }); | ||
|
|
||
| let thread_id = payload_string(&args.payload, "threadId") | ||
| .or_else(|| payload_string(&args.payload, "chatId")) | ||
| .or_else(|| payload_string(&args.payload, "conversationId")); | ||
| let title = payload_string(&args.payload, "title") | ||
| .or_else(|| payload_string(&args.payload, "chatName")) | ||
| .or_else(|| payload_string(&args.payload, "channelName")); | ||
| let snippet = payload_string(&args.payload, "snippet") | ||
| .or_else(|| first_message_field(&args.payload, "body")); | ||
| let sender_name = payload_string(&args.payload, "senderName") | ||
| .or_else(|| first_message_field(&args.payload, "from")); | ||
| let sender_handle = payload_string(&args.payload, "senderHandle"); | ||
| let deep_link = payload_string(&args.payload, "deepLink"); | ||
| let unread = payload_i64(&args.payload, "unread").unwrap_or(0); | ||
| let requires_attention = payload_bool(&args.payload, "requires_attention") | ||
| .unwrap_or(unread > 0 || sender_name.is_some() || snippet.is_some()); | ||
|
|
||
| Some(json!({ | ||
| "provider": args.provider, | ||
| "account_id": args.account_id, | ||
| "event_kind": args.kind, | ||
| "entity_id": entity_id, | ||
| "thread_id": thread_id, | ||
| "title": title, | ||
| "snippet": snippet, | ||
| "sender_name": sender_name, | ||
| "sender_handle": sender_handle, | ||
| "timestamp": event_timestamp_rfc3339(args.ts), | ||
| "deep_link": deep_link, | ||
| "requires_attention": requires_attention, | ||
| "raw_payload": args.payload, | ||
| })) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing unit tests for the new normalization helpers.
normalize_provider_surfaces_event has non-trivial fallback logic (entity_id key precedence, messages[0] field traversal, timestamp formatting, requires_attention heuristic) and is the contract the core RPC consumes — but no tests were added in the tests module below (which already covers comparable surface area for the zoom rewrite helpers).
Please add coverage for at least: entity_id key precedence (entity_id → threadId → chatId → snapshotKey → synthesized fallback), first_message_field happy path + missing/non-array messages, event_timestamp_rfc3339 with None/valid/invalid millis, and the requires_attention resolution paths.
As per coding guidelines: "Ship unit tests and coverage for behavior you are adding or changing before building additional features on top".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/webview_accounts/mod.rs` around lines 375 - 460, Add unit
tests for normalize_provider_surfaces_event and the helper functions to cover
the fallback and heuristic paths: write tests in the existing tests module that
assert entity_id precedence (payload keys "entity_id" -> "threadId" -> "chatId"
-> "snapshotKey" -> synthesized fallback when none present), verify
first_message_field returns the first message field when messages is an array
and returns None when messages is missing or not an array, test
event_timestamp_rfc3339 with None (produces now), a valid millis value (produces
expected RFC3339 string) and an invalid millis (ensure fallback behavior), and
exercise requires_attention resolution for explicit requires_attention
true/false as well as derived true when unread>0 and when sender_name or snippet
exist; call normalize_provider_surfaces_event with crafted RecipeEventArgs
payloads and assert fields ("entity_id", "thread_id", "timestamp",
"requires_attention", "snippet", "sender_name") in the returned JSON.
| let unread = payload_i64(&args.payload, "unread").unwrap_or(0); | ||
| let requires_attention = payload_bool(&args.payload, "requires_attention") | ||
| .unwrap_or(unread > 0 || sender_name.is_some() || snippet.is_some()); |
There was a problem hiding this comment.
requires_attention default fires for nearly every ingest event.
unread > 0 || sender_name.is_some() || snippet.is_some() evaluates true whenever an ingest payload carries any message-shaped data — which is the common case for the providers wired through messages[0].body/messages[0].from fallbacks (gmail, linkedin recipe). In practice this default reduces to "always true unless the caller passes requires_attention: false or sends an empty payload", which seems unlikely to be the intended signal.
Consider tightening the heuristic (e.g., unread > 0 only, or "unread > 0 AND we have a sender") so this flag still differentiates noisy polls from actual new activity downstream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/webview_accounts/mod.rs` around lines 441 - 443, The
current default for requires_attention (computed via payload_bool(&args.payload,
"requires_attention").unwrap_or(...)) fires for almost every ingest because it
treats any message-shaped payload (sender_name or snippet present) as requiring
attention; change that fallback to a stricter heuristic such as unwrap_or(unread
> 0) or unwrap_or(unread > 0 && sender_name.is_some()) so requires_attention
only becomes true for actual new/unread activity; update the expression around
requires_attention, referencing the variables unread, sender_name, snippet and
the payload_bool call to implement the chosen tighter rule.
| if let Err(err) = post_provider_surfaces_event(&args).await { | ||
| log::warn!( | ||
| "[webview-accounts] provider_surfaces ingest failed account={} provider={} kind={}: {}", | ||
| args.account_id, | ||
| args.provider, | ||
| args.kind, | ||
| err | ||
| ); | ||
| } |
There was a problem hiding this comment.
Sequential .await delays webview:event emit on slow/unavailable core RPC.
post_provider_surfaces_event(&args).await runs inline before the existing app.emit("webview:event", …) at line 2363. With the 10s reqwest timeout (line 477), a stalled or down core RPC will hold the IPC reply for up to 10s on every recipe event — the JS poll loop in runtime.js fires every 2s, so backpressure stacks up quickly and the React UI sees its webview:event stream visibly lag (or stop, since recipe sends are awaited from JS) until each POST resolves.
Since this is a fire-and-forget side-channel ("Failures … are caught and logged as warnings, while the existing webview:event emission path remains unchanged" per the summary), spawn it onto the runtime instead so the IPC return and the Tauri emit are not gated on the external HTTP call.
🛠️ Proposed fix: spawn the RPC post and emit immediately
- if let Err(err) = post_provider_surfaces_event(&args).await {
- log::warn!(
- "[webview-accounts] provider_surfaces ingest failed account={} provider={} kind={}: {}",
- args.account_id,
- args.provider,
- args.kind,
- err
- );
- }
-
let event = WebviewEvent {
account_id: args.account_id,
provider: args.provider,
kind: args.kind,
payload: args.payload,
ts: args.ts,
};
+ // Fire-and-forget the side-channel so a slow/unavailable core RPC
+ // can't stall the `webview:event` reply or the recipe poll loop.
+ {
+ let args_for_post = RecipeEventArgs {
+ account_id: event.account_id.clone(),
+ provider: event.provider.clone(),
+ kind: event.kind.clone(),
+ payload: event.payload.clone(),
+ ts: event.ts,
+ };
+ tauri::async_runtime::spawn(async move {
+ if let Err(err) = post_provider_surfaces_event(&args_for_post).await {
+ log::warn!(
+ "[webview-accounts] provider_surfaces ingest failed account={} provider={} kind={}: {}",
+ args_for_post.account_id,
+ args_for_post.provider,
+ args_for_post.kind,
+ err
+ );
+ }
+ });
+ }
app.emit("webview:event", &event)
.map_err(|e| format!("emit failed: {e}"))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/webview_accounts/mod.rs` around lines 2346 - 2354, The
current code awaits post_provider_surfaces_event(&args).await before calling
app.emit("webview:event", …), which blocks the IPC reply on slow/unavailable
core RPC; instead, spawn the POST call onto the async runtime so the emit
happens immediately. Replace the direct await with a spawned task (e.g., using
tauri::async_runtime::spawn or tokio::spawn) that moves/clones args and inside
the task calls post_provider_surfaces_event(args).await and logs any Err as
before; keep the existing app.emit("webview:event", …) path
synchronous/unblocked so the webview event is emitted immediately. Ensure
spawned task captures necessary variables (args, process logger) by value and
preserves the original warning log behavior for failures.
3 participants
Depends on: #803
...
Summary by CodeRabbit
New Features
Bug Fixes