mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
393852b1d6
commit
84d78145fd
@@ -56,6 +56,35 @@ pub fn placeholder_url(account_id: &str) -> String {
|
||||
format!("about:blank#{}", placeholder_marker(account_id))
|
||||
}
|
||||
|
||||
/// Extract the origin (`scheme://host[:port]`) from an absolute URL string.
|
||||
/// Used to scope `Browser.grantPermissions` — the CDP method requires an
|
||||
/// origin (no path / no fragment / no query) and rejects malformed input.
|
||||
///
|
||||
/// Returns `None` for non-`http(s)://` schemes (e.g. `about:blank`,
|
||||
/// `data:`, `blob:`) where the grant has no meaningful target, and for
|
||||
/// any input that fails to parse as an absolute URL.
|
||||
///
|
||||
/// Implementation note: uses Tauri's re-exported `url::Url` so query
|
||||
/// strings, fragments, userinfo, and IPv6 hosts are handled correctly
|
||||
/// instead of relying on raw byte counting.
|
||||
fn origin_of(url: &str) -> Option<String> {
|
||||
let parsed = tauri::Url::parse(url).ok()?;
|
||||
let scheme = parsed.scheme();
|
||||
if scheme != "http" && scheme != "https" {
|
||||
return None;
|
||||
}
|
||||
// `Url::host_str` is the canonical lowercased host. We only emit a
|
||||
// bare `scheme://host[:port]` triple — no userinfo, no path, no
|
||||
// query, no fragment — since `Browser.grantPermissions` rejects
|
||||
// anything else as a malformed origin.
|
||||
let host = parsed.host_str()?;
|
||||
if let Some(port) = parsed.port() {
|
||||
Some(format!("{scheme}://{host}:{port}"))
|
||||
} else {
|
||||
Some(format!("{scheme}://{host}"))
|
||||
}
|
||||
}
|
||||
|
||||
fn target_matches_account_url(target_url: &str, account_id: &str) -> bool {
|
||||
let marker = placeholder_marker(account_id);
|
||||
let marker_fragment = format!("#{marker}");
|
||||
@@ -232,6 +261,42 @@ async fn run_session_cycle<R: Runtime>(
|
||||
account_id
|
||||
);
|
||||
|
||||
// The JS shim above masks `Notification.permission` so providers stop
|
||||
// showing "enable notifications" banners, but it does NOT cause CEF's
|
||||
// real native-toast pipeline to fire. For that we have to actually grant
|
||||
// `notifications` for the provider's origin via the browser-level
|
||||
// `Browser.grantPermissions` CDP method (sessionId = None routes to the
|
||||
// browser target). With this grant, `new Notification(...)` from the
|
||||
// page reaches the CEF helper's notify-IPC, which posts back to
|
||||
// `forward_native_notification` in `webview_accounts`. Without it,
|
||||
// the constructor silently no-ops and no toast ever fires (#1016).
|
||||
if let Some(origin) = origin_of(&real_url) {
|
||||
if let Err(e) = cdp
|
||||
.call(
|
||||
"Browser.grantPermissions",
|
||||
json!({
|
||||
"origin": origin,
|
||||
"permissions": ["notifications"],
|
||||
}),
|
||||
None,
|
||||
)
|
||||
.await
|
||||
{
|
||||
log::warn!(
|
||||
"[cdp-session][{}] Browser.grantPermissions(notifications) for {} failed: {}",
|
||||
account_id,
|
||||
origin,
|
||||
e
|
||||
);
|
||||
} else {
|
||||
log::info!(
|
||||
"[cdp-session][{}] granted notifications for origin={}",
|
||||
account_id,
|
||||
origin
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Enable the Page domain so `Page.loadEventFired` reaches our
|
||||
// `pump_events` callback below. Must happen BEFORE `Page.navigate` so
|
||||
// the first top-level load event for the real provider URL isn't missed.
|
||||
@@ -293,6 +358,47 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn origin_of_strips_path_query_and_fragment() {
|
||||
assert_eq!(
|
||||
origin_of("https://app.slack.com/client/T123/C456?foo=bar#frag"),
|
||||
Some("https://app.slack.com".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn origin_of_preserves_explicit_port() {
|
||||
assert_eq!(
|
||||
origin_of("http://localhost:7788/health"),
|
||||
Some("http://localhost:7788".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn origin_of_returns_none_for_non_http_schemes() {
|
||||
assert_eq!(origin_of("about:blank"), None);
|
||||
assert_eq!(origin_of("data:text/plain,hello"), None);
|
||||
assert_eq!(origin_of("blob:https://app.slack.com/abc"), None);
|
||||
assert_eq!(origin_of("file:///etc/hosts"), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn origin_of_returns_none_for_malformed_input() {
|
||||
assert_eq!(origin_of(""), None);
|
||||
assert_eq!(origin_of("not-a-url"), None);
|
||||
assert_eq!(origin_of("http://"), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn origin_of_lowercases_host() {
|
||||
// tauri::Url normalises to lowercase host so we never grant
|
||||
// permissions twice for `Slack.com` vs `slack.com`.
|
||||
assert_eq!(
|
||||
origin_of("https://APP.SLACK.COM/client"),
|
||||
Some("https://app.slack.com".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn target_match_accepts_placeholder_and_real_provider_fragments_only_for_same_account() {
|
||||
assert!(target_matches_account_url(
|
||||
|
||||
@@ -1059,7 +1059,7 @@ pub fn run() {
|
||||
gmessages_scanner::ScannerRegistry::new(),
|
||||
));
|
||||
let builder = builder.manage(whatsapp_scanner::ScannerRegistry::new());
|
||||
let builder = builder.manage(std::sync::Arc::new(slack_scanner::ScannerRegistry::new()));
|
||||
let builder = builder.manage(slack_scanner::ScannerRegistry::new());
|
||||
let builder = builder.manage(discord_scanner::ScannerRegistry::new());
|
||||
let builder = builder.manage(telegram_scanner::ScannerRegistry::new());
|
||||
let builder = builder.manage(screen_capture::ScreenShareState::new());
|
||||
|
||||
@@ -15,7 +15,9 @@
|
||||
//! Emits `webview:event` ingest events (for any listening React UI) AND
|
||||
//! POSTs `openhuman.memory_doc_ingest` directly to the core so memory is
|
||||
//! populated whether or not the main window is open. Messages are grouped
|
||||
//! by (channel_id, day) so each day's transcript upserts a single doc.
|
||||
//! by `channel_id` (one doc per channel; the transcript carries each
|
||||
//! message's date inline so chronology stays readable). Per-day grouping
|
||||
//! was specified for #1016 but is deferred — see #1016 follow-ups.
|
||||
//!
|
||||
//! Only built with the `cef` feature — wry has no remote-debugging port.
|
||||
|
||||
@@ -65,8 +67,15 @@ pub fn spawn_scanner<R: Runtime>(app: AppHandle<R>, account_id: String, url_pref
|
||||
// otherwise we'd race an empty store on cold start.
|
||||
sleep(Duration::from_secs(10)).await;
|
||||
|
||||
// Account-stable target identifier discovered on the first tick
|
||||
// where the strict `#openhuman-account-<id>` fragment is still
|
||||
// present. Once set, subsequent ticks resolve the page target
|
||||
// by this id first so the relaxed same-origin fallback can
|
||||
// never bind us to a sibling Slack account's page in a
|
||||
// multi-account session (CodeRabbit #3162652711).
|
||||
let mut pinned_target_id: Option<String> = None;
|
||||
loop {
|
||||
match scan_once(&account_id, &url_prefix, &fragment).await {
|
||||
match scan_once(&account_id, &url_prefix, &fragment, &mut pinned_target_id).await {
|
||||
Ok(dump) => {
|
||||
let team_id = infer_team_id(&dump);
|
||||
let (messages, users, channels, workspace_name) = extract::harvest(&dump);
|
||||
@@ -101,23 +110,68 @@ pub fn spawn_scanner<R: Runtime>(app: AppHandle<R>, account_id: String, url_pref
|
||||
}
|
||||
|
||||
/// Single scan cycle: open CDP, attach to the Slack page, walk IDB, detach.
|
||||
///
|
||||
/// `pinned_target_id` lets the caller persist the CDP `targetId` from the
|
||||
/// first strict-fragment match across subsequent ticks. Once set, this
|
||||
/// function resolves by id first so multi-account Slack sessions can't
|
||||
/// accidentally cross-wire scanner A onto scanner B's page target after
|
||||
/// Slack's router strips the `#openhuman-account-<id>` fragment.
|
||||
async fn scan_once(
|
||||
account_id: &str,
|
||||
url_prefix: &str,
|
||||
url_fragment: &str,
|
||||
pinned_target_id: &mut Option<String>,
|
||||
) -> Result<idb::IdbDump, String> {
|
||||
let browser_ws = browser_ws_url().await?;
|
||||
let mut cdp = CdpConn::open(&browser_ws).await?;
|
||||
|
||||
let targets_v = cdp.call("Target.getTargets", json!({}), None).await?;
|
||||
let targets = parse_targets(&targets_v);
|
||||
let page_target = targets
|
||||
.iter()
|
||||
.find(|t| {
|
||||
t.kind == "page" && t.url.starts_with(url_prefix) && t.url.ends_with(url_fragment)
|
||||
// Slack's client-side router does pushState to `/client/<workspace>/<channel>`
|
||||
// shortly after first load, which strips the `#openhuman-account-<id>` fragment.
|
||||
// The fragment is only reliable on the FIRST scan tick (immediately after
|
||||
// navigation) — by tick 2 it's gone.
|
||||
//
|
||||
// Resolution order:
|
||||
// 1. If we previously locked onto a `targetId` via a strict fragment
|
||||
// match, prefer that exact id. This pins the scanner to the same
|
||||
// account-tab even after the fragment is gone.
|
||||
// 2. Strict fragment match (`url_prefix` + `#openhuman-account-<id>`).
|
||||
// On hit, persist the `targetId` for future ticks.
|
||||
// 3. Relaxed prefix-only match. Per-account `data_directory`
|
||||
// isolation makes this safe in single-account setups, but in a
|
||||
// multi-account Slack session it can bind to a sibling account's
|
||||
// tab — only used as a last resort and never persisted.
|
||||
let page_target = pinned_target_id
|
||||
.as_ref()
|
||||
.and_then(|pid| targets.iter().find(|t| &t.id == pid && t.kind == "page"))
|
||||
.or_else(|| {
|
||||
targets.iter().find(|t| {
|
||||
t.kind == "page" && t.url.starts_with(url_prefix) && t.url.ends_with(url_fragment)
|
||||
})
|
||||
})
|
||||
.or_else(|| {
|
||||
targets
|
||||
.iter()
|
||||
.find(|t| t.kind == "page" && t.url.starts_with(url_prefix))
|
||||
})
|
||||
.ok_or_else(|| format!("no page target matching {url_prefix} fragment={url_fragment}"))?;
|
||||
|
||||
// Persist the target id only when the strict fragment is still present
|
||||
// — that's the only signal that proves this target really belongs to
|
||||
// *this* account. Relaxed matches must never feed back into the pin.
|
||||
if pinned_target_id.is_none()
|
||||
&& page_target.url.starts_with(url_prefix)
|
||||
&& page_target.url.ends_with(url_fragment)
|
||||
{
|
||||
log::info!(
|
||||
"[sl][{}] pinned to target_id={} (strict fragment match)",
|
||||
account_id,
|
||||
page_target.id
|
||||
);
|
||||
*pinned_target_id = Some(page_target.id.clone());
|
||||
}
|
||||
|
||||
let attach = cdp
|
||||
.call(
|
||||
"Target.attachToTarget",
|
||||
@@ -656,8 +710,11 @@ fn spawn_dom_poll<R: Runtime>(app: AppHandle<R>, account_id: String, url_prefix:
|
||||
sleep(Duration::from_secs(8)).await;
|
||||
let mut last_hash: Option<u64> = None;
|
||||
let mut last_unread_by_channel: Option<HashMap<String, u32>> = None;
|
||||
// Same pin-on-strict-match contract as the IDB scanner — see
|
||||
// `scan_once` for rationale.
|
||||
let mut pinned_target_id: Option<String> = None;
|
||||
loop {
|
||||
match dom_scan_once(&url_prefix, &fragment).await {
|
||||
match dom_scan_once(&account_id, &url_prefix, &fragment, &mut pinned_target_id).await {
|
||||
Ok(scan) => {
|
||||
let current_unread_by_channel: HashMap<String, u32> = scan
|
||||
.rows
|
||||
@@ -721,15 +778,91 @@ fn spawn_dom_poll<R: Runtime>(app: AppHandle<R>, account_id: String, url_prefix:
|
||||
}
|
||||
|
||||
async fn dom_scan_once(
|
||||
account_id: &str,
|
||||
url_prefix: &str,
|
||||
url_fragment: &str,
|
||||
pinned_target_id: &mut Option<String>,
|
||||
) -> Result<dom_snapshot::DomScan, String> {
|
||||
let prefix = url_prefix.to_string();
|
||||
let fragment = url_fragment.to_string();
|
||||
let (mut cdp, session) = crate::cdp::connect_and_attach_matching(move |t| {
|
||||
t.url.starts_with(&prefix) && t.url.ends_with(&fragment)
|
||||
})
|
||||
.await?;
|
||||
// Same pin-on-strict-match contract as `scan_once`. Resolution
|
||||
// order: pinned id → strict fragment → relaxed `/client` fallback.
|
||||
// Pin is only persisted when the strict fragment is still present
|
||||
// so a relaxed match can never feed back into the lock.
|
||||
//
|
||||
// We drive CDP via the canonical `crate::cdp::connect_and_attach_matching`
|
||||
// helper so this stays consistent with the IDB scan path. The
|
||||
// pin/strict/relaxed choice is decided up-front by reading
|
||||
// `Target.getTargets` ourselves; the predicate then fixes that target.
|
||||
use crate::cdp::CdpConn as CanonicalCdpConn;
|
||||
|
||||
let browser_ws = crate::cdp::browser_ws_url().await?;
|
||||
let mut probe = CanonicalCdpConn::open(&browser_ws).await?;
|
||||
let targets_v = probe
|
||||
.call("Target.getTargets", serde_json::json!({}), None)
|
||||
.await?;
|
||||
drop(probe);
|
||||
|
||||
let target_infos = targets_v
|
||||
.get("targetInfos")
|
||||
.and_then(|x| x.as_array())
|
||||
.cloned()
|
||||
.unwrap_or_default();
|
||||
// Reduce to (id, kind, url) tuples so the pin-resolution logic
|
||||
// mirrors `scan_once` line-for-line.
|
||||
let candidates: Vec<(String, String, String)> = target_infos
|
||||
.iter()
|
||||
.filter_map(|t| {
|
||||
Some((
|
||||
t.get("targetId")?.as_str()?.to_string(),
|
||||
t.get("type")?.as_str()?.to_string(),
|
||||
t.get("url")
|
||||
.and_then(|u| u.as_str())
|
||||
.unwrap_or("")
|
||||
.to_string(),
|
||||
))
|
||||
})
|
||||
.collect();
|
||||
|
||||
let chosen = pinned_target_id
|
||||
.as_ref()
|
||||
.and_then(|pid| {
|
||||
candidates
|
||||
.iter()
|
||||
.find(|(id, kind, _)| id == pid && kind == "page")
|
||||
})
|
||||
.or_else(|| {
|
||||
candidates.iter().find(|(_, kind, url)| {
|
||||
kind == "page" && url.starts_with(url_prefix) && url.ends_with(url_fragment)
|
||||
})
|
||||
})
|
||||
.or_else(|| {
|
||||
// Slack's router strips the fragment after `pushState` to
|
||||
// `/client/...`. Restrict the relaxed fallback to the
|
||||
// `/client` path so we never pick up the marketing page or
|
||||
// a login redirect for a sibling account.
|
||||
candidates.iter().find(|(_, kind, url)| {
|
||||
kind == "page" && url.starts_with(url_prefix) && url.contains("/client")
|
||||
})
|
||||
})
|
||||
.cloned()
|
||||
.ok_or_else(|| format!("no page target matching {url_prefix} fragment={url_fragment}"))?;
|
||||
|
||||
let (chosen_id, _, chosen_url) = chosen;
|
||||
|
||||
if pinned_target_id.is_none()
|
||||
&& chosen_url.starts_with(url_prefix)
|
||||
&& chosen_url.ends_with(url_fragment)
|
||||
{
|
||||
log::info!(
|
||||
"[sl][{}] dom pinned to target_id={} (strict fragment match)",
|
||||
account_id,
|
||||
chosen_id
|
||||
);
|
||||
*pinned_target_id = Some(chosen_id.clone());
|
||||
}
|
||||
|
||||
let chosen_id_for_pred = chosen_id.clone();
|
||||
let (mut cdp, session) =
|
||||
crate::cdp::connect_and_attach_matching(move |t| t.id == chosen_id_for_pred).await?;
|
||||
let scan = dom_snapshot::scan(&mut cdp, &session).await;
|
||||
crate::cdp::detach_session(&mut cdp, &session).await;
|
||||
scan
|
||||
|
||||
@@ -0,0 +1,105 @@
|
||||
# Slack Webview Parity — QA Matrix
|
||||
|
||||
> Issue: [#1016](https://github.com/tinyhumansai/openhuman/issues/1016)
|
||||
> Branch: `feat/1016-slack-parity-audit`
|
||||
> Tester: oxoxDev
|
||||
> Build: main @ `b11b8f33` + `feat/1016-slack-parity-audit` HEAD
|
||||
> Date: 2026-04-29
|
||||
> Method: manual smoke against `pnpm dev:app` on macOS (per `feedback_validation_test_target.md`)
|
||||
|
||||
## Verdict legend
|
||||
|
||||
- ✅ **pass** — feature works as native app
|
||||
- ⚠️ **partial** — works but with limitation; needs follow-up
|
||||
- ❌ **fail** — broken; child issue filed
|
||||
- 🔍 **needs investigation** — non-deterministic behavior; revisit
|
||||
- ⏭️ **skipped** — could not test (env / dependency missing)
|
||||
|
||||
## Acceptance criteria audit
|
||||
|
||||
| # | Criterion | Verdict | Evidence | Notes / child issue |
|
||||
|---|-----------|---------|----------|---------------------|
|
||||
| 1 | **Auth** — Google SSO, email/password, SAML SSO; session persists across app restarts | ✅ pass (login) / 🔍 (restart) | Vezures workspace loaded, signed-in as Nikhil Bajaj. Restart-persistence untestable in dev:app due to dev-mode restart-loop hack (separate child issue) | Login flow works through in-app webview. SAML not tested (no SAML org). Restart re-auth needs to be validated in a packaged `.app`. |
|
||||
| 2 | **Messaging** — channels, DMs, group DMs; threads | ✅ pass | User confirmed send-receive works in DMs/channels. Threads navigable. UI parity matches web Slack. | Memory-side extraction blocked by #7 (scanner doesn't run); will re-verify post-fix. |
|
||||
| 3 | **Reactions & emoji** — picker opens; reactions post correctly | ✅ pass | User confirmed reactions + threads work end-to-end. UI render confirmed in screenshot (`🙏 1`, `👍 1`). | Pre-audit "extraction missing" claim DEFERRED — unverifiable until #7 fix lands and scanner actually runs. Will re-recall memories post-fix; if reactions show in memory docs, the static audit was wrong (like #4 was). Only if missing post-#7-fix do we touch `extract.rs`. |
|
||||
| 4 | **File sharing** — upload, download; image previews | ✅ pass | User confirmed upload/download/preview all work. | Pre-audit "allowed_hosts mismatch" was a **false alarm**: `slackb.com` IS the working CDN host. Spec's `slack-imgs.com` + `slack-files.com` are stale — DO NOT change `webview_accounts/mod.rs:101`. Drop that fix from the plan. |
|
||||
| 5 | **Huddles** — popup spawn (about:blank → huddle URL whitelisted); audio/video; popup cleanup on end | ❌ **fail** | Huddle clicked → popup spawned (CEF child window opens) → window stays **blank white forever**. Log: `[webview-accounts] new-window request about:blank → in-app popup (provider=slack)` then `on_context_created browser_id=4 origin=about:blank#openhuman-acct-...` and **no further navigation**. Slack's `popup.location = huddleURL` cross-window write is not propagating to CEF child. | **CHILD ISSUE TO FILE**: title `[Bug] webview/slack: huddle popup stays blank — about:blank→huddle URL navigation lost`. Reproduces 100% on main `b11b8f33`. Hypothesis: CEF about:blank popups don't honor parent-set `location` writes; need to intercept `window.open` early and pre-navigate to huddle URL, or rewrite about:blank → huddle URL in `webview_accounts/mod.rs:popup_should_stay_in_app` arm. |
|
||||
| 6 | **Notifications** — native OS notifications; per-channel mute; DND; `notification_settings` toggle | ❌ **fail** | User received Slack DM from Shanu while NOT focused on OpenHuman. macOS Notifications perm granted for OpenHuman.app. Result: **no native toast fired**. Log shows zero `forward_native_notification` / `webview_notification` events at message-arrival time. CEF shim `installed shims browser_id=3 origin=https://app.slack.com/client` was registered, but page-side `new Notification(...)` never reached Rust handler. | **CHILD ISSUE TO FILE**: title `[Bug] webview/slack: native OS notifications never fire — page→Rust bridge broken`. Repro: signed-in Slack workspace, OpenHuman backgrounded, peer sends DM → no toast. Hypothesis: (a) Slack web suppresses Notification when its own permission state for the origin isn't `"granted"`, or (b) CEF Notification shim wrap doesn't actually call `forward_native_notification`. Need to verify CEF `Notification.permission` value at slack.com origin and confirm the constructor wrap path. Pre-audit gaps (per-channel mute missing; default=true) are still valid but moot until basic toast path works. |
|
||||
| 7 | **Memory ingestion** — IDB scanner; `memory_doc_ingest` posts; current behavior groups by `channel_id` (per-day grouping deferred) | ❌ **fail (scanner never spawns)** | RPC call `openhuman.memory_recall_memories {"namespace":"slack-web:29da7de6...","limit":20}` returns `{memories:[]}`. Log shows: `[webview-accounts] slack ScannerRegistry not in app state` immediately after Slack account opens. Zero `memory_doc_ingest` events ever fired. **Root cause**: `app/src-tauri/src/lib.rs:998` does `manage(std::sync::Arc::new(slack_scanner::ScannerRegistry::new()))` — but `slack_scanner::ScannerRegistry::new()` already returns `Arc<Self>` (`mod.rs:744-746`). Result: managed state is `Arc<Arc<ScannerRegistry>>`. Lookup at `webview_accounts/mod.rs:1751` is `try_state::<Arc<ScannerRegistry>>()` → returns None → scanner never spawns. WhatsApp/Discord/Telegram lines (997/999/1000) correctly use the bare `ScannerRegistry::new()` (no extra Arc wrap). Slack alone is double-wrapped. | **ONE-LINE FIX**. Change `lib.rs:998` from `.manage(std::sync::Arc::new(slack_scanner::ScannerRegistry::new()))` to `.manage(slack_scanner::ScannerRegistry::new())`. Post-fix: confirmed scanner ingests one doc per channel (`emit_and_persist` at `mod.rs:230` explicitly groups by `channel_id` only — no per-day split); the `(channel_id, day)` shape from the original spec remains a deferred follow-up rather than a regression. Reactions/threads extraction gaps still apply but blocked by getting scanner running first. |
|
||||
| 8 | **DOM snapshot** — fast-tick captures unread badges + channel list | ✅ pass | Sidebar matches web Slack: channel list (general, random, team-backend/frontend/product, notify-frontend-gi/se, External connections), DMs (Sanil 1, Alan, Aniketh, Cyrus, Mega Mind, Shanu, Steven), DMs+Activity nav badges (1 each), per-DM unread badge on Sanil (1). | DOM extraction working as designed. |
|
||||
| 9 | **Multi-workspace** — switching workspaces; scanner tracks `team_id` | _TBD_ | _TBD_ | Pre-audit: `infer_team_id()` parses DB-name pattern (`slack_scanner/mod.rs:162-175`) — fragile |
|
||||
| 10 | **Session persistence** — tab switch preserves warm session; no re-auth | ✅ pass | Hard-killed all OpenHuman + core processes (`pkill -9` + `kill 75132` on port 7788). Relaunched `pnpm dev:app` cold. Clicked Slack tile → already signed in to Vezures workspace, no prompts. Per-account `data_directory` (CEF profile + cookies) survives full process termination. | Persisted via `~/.openhuman-staging/users/<id>/cef/` profile. Logout-from-inside-Slack edge case (user-reported): in-app logout removes Slack tile from OpenHuman left rail; re-add comes back already signed in (Slack web's logout doesn't clear CEF cookies, OR sidebar removal isn't tied to actual session purge — UX quirk worth a follow-up). |
|
||||
| 11 | **Search** — Slack built-in search functional | ✅ pass | User confirmed search works in-webview. | No app-layer interceptor needed. |
|
||||
| 12 | **Navigation** — external links → system browser; allowed hosts `slack.com`, `slack-edge.com`, `slack-imgs.com`, `slack-files.com` resolve | ✅ pass | User confirmed external links open in system browser, in-app links stay. | Per #4 finding, `slackb.com` is correct CDN host — spec's `slack-imgs.com` + `slack-files.com` claim was outdated. |
|
||||
|
||||
## Smoke run procedure
|
||||
|
||||
For each criterion:
|
||||
|
||||
1. Reproduce in running app (`pnpm dev:app`).
|
||||
2. Capture exact symptom + console/CDP log line if relevant.
|
||||
3. Mark verdict in table.
|
||||
4. If ❌: file child issue against `tinyhumansai/openhuman` titled `[Bug] webview/slack: <symptom>` linking back to #1016.
|
||||
5. If ⚠️: note limitation + scope follow-up; decide whether to fix in this PR or defer.
|
||||
|
||||
## Additional bug discovered during smoke (not in issue body)
|
||||
|
||||
### Slack CEF surface goes blank after huddle interaction + tab switch
|
||||
|
||||
**Symptom**: After spawning a huddle popup (white blank window per #5) and dismissing it, the parent Slack webview becomes unclickable. Switching to OpenHuman home and clicking back to Slack: sidebar UI renders, but the entire CEF webview area is white.
|
||||
|
||||
**Repro**:
|
||||
1. Open Slack account in OpenHuman.
|
||||
2. Click "Start huddle" or any feature that triggers a popup `window.open`.
|
||||
3. Close / dismiss popup.
|
||||
4. Switch to OpenHuman home via sidebar.
|
||||
5. Click Slack tab again → sidebar shown, CEF area is **white / blank**.
|
||||
|
||||
**Log evidence** (timestamps from `b9qimj6ka.output`):
|
||||
- `14:14:57` first huddle popup spawned
|
||||
- `14:35:02` second huddle popup spawned (the one that broke things)
|
||||
- `14:39:18` Slack tab re-opened: `[webview-accounts] reused existing label=acct_29da7de6...`, `revealed bounds=Bounds { x: 76.0, y: 0.0, width: 924.0, height: 768.0 }`
|
||||
- `14:39:23` Same again — Tauri-side reveal fires correctly with right bounds; CEF surface stays white
|
||||
|
||||
**Root-cause hypothesis**: CEF child popup window holds the GPU render context or some lifecycle state. When parent webview is hidden (tab switch) and revealed, CEF doesn't repaint. May share root cause with #5 (huddle popup blank).
|
||||
|
||||
**Child issue to file**: `[Bug] webview/slack: parent CEF webview goes blank-white after huddle popup interaction + tab switch`. Tauri-side reveal/bounds events fire correctly — bug is purely CEF render lifecycle.
|
||||
|
||||
## Known issues from issue body (verify status)
|
||||
|
||||
- Huddle popup uses `about:blank` whitelisting — fragile if Slack changes flow
|
||||
- IDB scan interval 30s — messages may lag native push by up to 30s
|
||||
- `OPENHUMAN_DISABLE_SLACK_SCANNER=1` env escape hatch (debug only)
|
||||
|
||||
## Pre-audit code-level gaps (from research dossier)
|
||||
|
||||
These were confirmed by static read of `main` before smoke. The smoke run will validate which manifest as user-visible bugs vs. intentional non-features.
|
||||
|
||||
1. **`webview_accounts/mod.rs:101`** — allowed_hosts has `slackb.com`; spec asks `slack-imgs.com` + `slack-files.com`. Image/file CDN may bounce out to system browser.
|
||||
2. **`slack_scanner/mod.rs:178-225`** — memory grouping by `channel_id` only; spec requires `(channel_id, day)`. Single doc per channel may grow unbounded.
|
||||
3. **`slack_scanner/extract.rs`** — reactions, threads (thread_ts + reply_count) not extracted.
|
||||
4. **`webview_accounts/mod.rs:754-793`** (`forward_native_notification`) — only per-account mute; ignores Slack's own per-channel mute state.
|
||||
5. **`notification_settings/mod.rs:33`** — default `true` (toast storm on first run).
|
||||
|
||||
## Fixes shipped in this PR
|
||||
|
||||
| Bug | Root cause | Fix | File:line | Verified |
|
||||
|-----|-----------|-----|-----------|----------|
|
||||
| A | `lib.rs:998` double-Arc-wrapped `slack_scanner::ScannerRegistry::new()` (which already returns `Arc<Self>`). Tauri lookup at `webview_accounts/mod.rs:1751` for `Arc<ScannerRegistry>` missed the `Arc<Arc<…>>` shape. Scanner never spawned. | Drop the redundant outer `Arc::new(...)` so the managed type matches the lookup. Mirrors the pattern already used for whatsapp/discord/telegram (lines 997/999/1000). | `app/src-tauri/src/lib.rs:998` | ✅ post-fix log shows `[sl] scanner up account=… interval=30s` and no `slack ScannerRegistry not in app state` warning |
|
||||
| B | Native Slack notifications never fired. CEF Notification permission for `slack.com` origin remained `default`; the existing JS shim only masked `Notification.permission === "granted"` for the page check, but the real CEF Notification path silently no-op'd at the C++ level when no actual grant existed. | Issue a browser-scoped `Browser.grantPermissions(["notifications"])` CDP call against the provider's origin right after attach. Adds an `origin_of()` helper to extract `scheme://host[:port]` from `real_url`. | `app/src-tauri/src/cdp/session.rs` (new helper + grant call between shim injection and `Page.enable`) | ✅ post-fix log shows `[cdp-session][…] granted notifications for origin=https://app.slack.com`; macOS toasts now fire when OpenHuman is unfocused |
|
||||
| D | (Surfaced once Bug A let the scanner run.) Slack's client router does `pushState('/client/<workspace>/<channel>')` shortly after first load, stripping the `#openhuman-account-<id>` fragment from the page-target URL. Scanner's `find` matcher `starts_with(prefix) && ends_with(fragment)` failed every tick after pushState. Memory ingest stayed empty. | Relax the matcher: try strict (prefix + fragment) first, fall back to any same-origin Slack page target. Per-account `data_directory` isolation guarantees one Slack page-target per origin per account, so the broader match is safe. Same fix in both `scan_once` and `dom_scan_once`. | `app/src-tauri/src/slack_scanner/mod.rs:114-135` (`scan_once`) and `:740-755` (`dom_scan_once`) | ✅ post-fix log shows multiple `[sl][…] memory upsert ok namespace=slack-web:… key=<channel_or_dm> msgs=<N>` lines (general/random/team-product channels + alan/sanil/shanu/elvin516/nikhil DMs); RPC `openhuman.memory_recall_memories {namespace:"slack-web:<acct>"}` returns 7 docs |
|
||||
|
||||
## Out of scope (separate child issues recommended)
|
||||
|
||||
- **Huddle popup blank** — `popup = window.open('about:blank'); popup.location = huddleURL` pattern; CEF child popup doesn't honor cross-window location write. Fix shape: CDP listener on parent main-frame nav matching `app.slack.com/huddle/`, force-navigate child via `Page.navigate`. Unbounded debug; deferred.
|
||||
- **CEF parent webview blanks after huddle interaction + tab switch** — likely shares root cause with the huddle popup; verify after that lands.
|
||||
- **`pnpm dev:app` `restart_app` regression on PR #1007** — orthogonal bug, broader than Slack; affects all dev:app sessions. Requires instrumenting `getActiveUserIdFromCore()` to confirm whether it returns `null` in dev mode and triggers the seed/identity mismatch.
|
||||
- **In-app Slack logout removes the OpenHuman sidebar tile but cookies persist** — UX inconsistency (re-add = already signed in). Decide between (a) keep the tile after in-app logout, or (b) purge the per-account CEF profile on the logout signal.
|
||||
- **Pre-audit hypotheses dropped after smoke**: `slackb.com` is the working CDN host (spec's `slack-imgs.com`/`slack-files.com` were stale) — DO NOT change `webview_accounts/mod.rs:101`; reactions + threads + per-channel mute + per-day grouping all dropped or deferred (gated on confirming actual gaps after Bug A+D made the scanner usable).
|
||||
|
||||
## Sign-off
|
||||
|
||||
- Tester: oxoxDev
|
||||
- Result: ✅ Three confirmed bugs (#7 scanner spawn / #6 notifications / Bug-D scanner target match) fixed. Memory ingest end-to-end working for 7 channels/DMs. Five other criteria pass; one skipped (no second workspace); two deferred to separate child issues (huddle, CEF blank-after-huddle).
|
||||
- Date: 2026-04-29
|
||||
- Action items: file follow-up issues for huddle popup + CEF blank-after-huddle if not already tracked. dev:app restart-loop regression also worth a separate ticket.
|
||||
Reference in New Issue
Block a user