fix(webview/slack): first-load + Google auth (#1036)#1249
fix(webview/slack): first-load + Google auth (#1036)#1249oxoxDev wants to merge 11 commits intotinyhumansai:mainfrom
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:
📝 WalkthroughWalkthroughThreads a RevealTrigger through webview load signaling, records per-account provider and spawn timestamps, expands Slack allowlist for Google/OAuth hosts, updates popup navigation for Google-SSO-capable providers, and surfaces trigger in emit_load_finished and webview_account_reveal (now using RevealArgs). ChangesWebview Load Instrumentation & Slack Google OAuth Support
Sequence DiagramsequenceDiagram
participant Client as Frontend
participant Service as WebviewAccountService
participant Tauri as Tauri/WebviewAccountsState
participant CDP as CDP Session
participant Webview as Embedded Webview
rect rgba(200, 220, 255, 0.5)
Note over Client,Tauri: Cold spawn records provider & spawn time
Client->>Service: webview_account_open(...)
Service->>Tauri: webview_account_open command
Tauri->>Tauri: account_providers[account_id] = provider
Tauri->>Tauri: spawn_started_at[account_id] = Instant::now()
Tauri->>Webview: create & navigate webview
end
par Load completion
rect rgba(144, 238, 144, 0.5)
Webview->>CDP: Page.loadEventFired
CDP->>Tauri: emit_load_finished(..., state="finished", trigger=Load)
Tauri->>Service: emit webview-account:load { account_id, url, trigger: "load" }
end
and Watchdog fallback
rect rgba(255, 200, 200, 0.5)
CDP->>CDP: timeout elapsed
CDP->>Tauri: emit_load_finished(..., state="timeout", trigger=Watchdog)
Tauri->>Service: emit webview-account:load { account_id, url, trigger: "watchdog" }
end
end
rect rgba(220, 200, 255, 0.5)
Service->>Service: handleWebviewAccountLoad(payload)
Service->>Tauri: invoke webview_account_reveal(RevealArgs{..., trigger})
Tauri->>Tauri: provider = account_providers[account_id] || "unknown"
Tauri->>Tauri: elapsed_ms = now - spawn_started_at[account_id]
Tauri->>Tauri: log(provider, elapsed_ms, trigger)
Tauri->>Webview: restore bounds & show
Tauri->>Tauri: remove spawn_started_at[account_id]
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/services/__tests__/webviewAccountService.loadListener.test.ts (1)
184-195: 💤 Low valueConsider adding a test case for watchdog trigger propagation.
The tests cover
trigger: 'load'(the default), but there's no explicit test verifying thattrigger: 'watchdog'from the payload propagates correctly to the reveal call. This would lock the behavior for the timeout-recovery-then-late-load scenario.💡 Optional test case for watchdog trigger
it('propagates watchdog trigger to reveal when payload specifies watchdog', async () => { const bounds = { x: 0, y: 0, width: 800, height: 600 }; await openWebviewAccount({ accountId: ACCOUNT_ID, provider: 'telegram', bounds }); vi.mocked(invoke).mockClear(); await fireLoadEvent({ state: 'finished', trigger: 'watchdog', url: 'https://web.telegram.org/' }); expect(vi.mocked(invoke)).toHaveBeenCalledWith('webview_account_reveal', { args: { account_id: ACCOUNT_ID, bounds, trigger: 'watchdog' }, }); });🤖 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/services/__tests__/webviewAccountService.loadListener.test.ts` around lines 184 - 195, Add a test that verifies the `trigger: 'watchdog'` from the load payload is propagated to the reveal call: use openWebviewAccount to open the account (referencing ACCOUNT_ID and bounds), clear vi.mocked(invoke), call fireLoadEvent with { state: 'finished', trigger: 'watchdog', url: 'https://web.telegram.org/' }, and assert vi.mocked(invoke) was called with 'webview_account_reveal' and args { account_id: ACCOUNT_ID, bounds, trigger: 'watchdog' } (mirroring the existing tests that use openWebviewAccount, fireLoadEvent and invoke).
🤖 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 `@app/src/services/__tests__/webviewAccountService.loadListener.test.ts`:
- Around line 184-195: Add a test that verifies the `trigger: 'watchdog'` from
the load payload is propagated to the reveal call: use openWebviewAccount to
open the account (referencing ACCOUNT_ID and bounds), clear vi.mocked(invoke),
call fireLoadEvent with { state: 'finished', trigger: 'watchdog', url:
'https://web.telegram.org/' }, and assert vi.mocked(invoke) was called with
'webview_account_reveal' and args { account_id: ACCOUNT_ID, bounds, trigger:
'watchdog' } (mirroring the existing tests that use openWebviewAccount,
fireLoadEvent and invoke).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7905dcd-60cf-450d-8229-3fdf5860a2f0
📒 Files selected for processing (4)
app/src-tauri/src/cdp/session.rsapp/src-tauri/src/webview_accounts/mod.rsapp/src/services/__tests__/webviewAccountService.loadListener.test.tsapp/src/services/webviewAccountService.ts
|
Addressed the CodeRabbit watchdog-trigger test nit in df18b80. Added explicit coverage that a load payload with trigger='watchdog' propagates to webview_account_reveal; targeted Vitest passes 11/11. |
senamakel
left a comment
senamakel
left a comment
There was a problem hiding this comment.
resolve the merge conflicts and push again
# Conflicts: # app/src-tauri/src/webview_accounts/mod.rs
|
Resolved the merge conflicts against upstream/main in signed merge 20d34c5, then cleaned the final diff back to the four #1036 files in 8f3fc56. Local focused validation passed: webview_accounts 69/69, cdp::session 10/10, agent::prompts 35/35, and targeted Vitest load listener 11/11. CI is re-running on head 8f3fc56. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src-tauri/src/webview_accounts/mod.rs (1)
2721-2735: 💤 Low valueInconsistent mutex handling for diagnostic lookups.
account_providersuses.lock().unwrap()(panics on poison) whilespawn_started_atuses.lock().ok()(graceful fallback). Since both are for diagnostic logging only, consider using consistent handling. Given the codebase pattern uses.unwrap()elsewhere, using.unwrap()forspawn_started_atas well would be consistent.♻️ Suggested consistency fix
let elapsed_ms = state .spawn_started_at .lock() - .ok() + .unwrap() - .and_then(|mut g| g.remove(&args.account_id)) + .remove(&args.account_id) .map(|started| started.elapsed().as_millis()) .map(|ms| ms.to_string()) .unwrap_or_else(|| "unknown".to_string());🤖 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-tauri/src/webview_accounts/mod.rs` around lines 2721 - 2735, The diagnostic lookup for spawn_started_at should match the established panic-on-poison pattern used for account_providers: replace the current graceful .lock().ok() path with a direct .lock().unwrap() usage so both mutexes behave consistently; locate the block that reads spawn_started_at (using state.spawn_started_at and args.account_id) and change the locking + removal sequence to use .lock().unwrap() before removing the entry and computing elapsed, keeping the same map.remove/started.elapsed()/as_millis()->to_string flow as the surrounding code.
🤖 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 `@app/src-tauri/src/webview_accounts/mod.rs`:
- Around line 2721-2735: The diagnostic lookup for spawn_started_at should match
the established panic-on-poison pattern used for account_providers: replace the
current graceful .lock().ok() path with a direct .lock().unwrap() usage so both
mutexes behave consistently; locate the block that reads spawn_started_at (using
state.spawn_started_at and args.account_id) and change the locking + removal
sequence to use .lock().unwrap() before removing the entry and computing
elapsed, keeping the same map.remove/started.elapsed()/as_millis()->to_string
flow as the surrounding code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fbc3eb0-5d02-4155-9b66-18607c69c31d
📒 Files selected for processing (1)
app/src-tauri/src/webview_accounts/mod.rs
|
Addressed the latest CodeRabbit mutex-consistency nit in signed commit d452231 by aligning the reveal diagnostic |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src-tauri/src/webview_accounts/mod.rs (1)
2228-2238:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStart the reveal timer before
add_child.
elapsed_msis supposed to capture cold-spawn → reveal time, but the clock starts only afteradd_childreturns, so browser creation time is missing from the new diagnostic.Proposed fix
+ let spawn_started = Instant::now(); let webview = parent_window .add_child(builder, initial_position, initial_size) .map_err(|e| format!("add_child failed: {e}"))?; @@ state .spawn_started_at .lock() .unwrap() - .insert(args.account_id.clone(), Instant::now()); + .insert(args.account_id.clone(), spawn_started);As per coding guidelines, "
**/*.rs: Default to verbose diagnostics on new/changed flows with entry/exit logging, branches, external calls, retries/timeouts, state transitions, and errors`.🤖 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-tauri/src/webview_accounts/mod.rs` around lines 2228 - 2238, Move the cold-spawn timer start to before the call to parent_window.add_child so the measured elapsed_ms includes browser creation time: acquire the spawn_started_at lock and insert args.account_id -> Instant::now() before invoking parent_window.add_child (the code that currently assigns webview), keeping the same data structure and error handling around add_child and preserving the existing webview variable assignment and logging paths.
🧹 Nitpick comments (1)
app/src-tauri/src/webview_accounts/mod.rs (1)
265-317: 🏗️ Heavy liftConsider splitting the new Google-SSO policy and reveal telemetry out of
mod.rs.This file is already carrying too many responsibilities, and these additions make navigation/reveal regressions harder to reason about. Pulling the new provider-policy helpers and reveal-trigger/state plumbing into small submodules would make future fixes safer.
As per coding guidelines, "
**/*.rs: Prefer modules ≤ ~500 lines; split growing modules`.Also applies to: 617-655, 1337-1585
🤖 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-tauri/src/webview_accounts/mod.rs` around lines 265 - 317, The module mod.rs is getting large and mixing concerns: extract the Google-SSO popup policy helpers (provider_supports_google_sso, popup_should_navigate_parent, and any helpers like is_google_auth_popup) into a small submodule (e.g., webview_accounts/google_sso.rs) and move the reveal/telemetry trigger and state plumbing mentioned elsewhere into a separate submodule (e.g., webview_accounts/reveal_telemetry.rs); update mod.rs to re-export the functions or call them via the new submodules, keep public APIs unchanged, and ensure unit tests/imports are updated to reference the new module paths so mod.rs stays under ~500 lines and responsibilities are separated.
🤖 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-tauri/src/webview_accounts/mod.rs`:
- Around line 1359-1363: The current from_ipc(raw: Option<&str>) silently maps
any unknown trigger to Self::Load; change it to surface unexpected IPC values by
either (A) adding an explicit Unknown(String) variant to the RevealTrigger enum
and returning Unknown(raw.unwrap_or_default().to_string()) for anything not
matching "watchdog" or "load" (and add a tracing::warn or log entry with the raw
value), or (B) change from_ipc to return a Result<RevealTrigger, String> and
return Err(format!("unknown reveal trigger: {:?}", raw)) for unexpected inputs
while logging the value; update all call sites of from_ipc (e.g., where
RevealTrigger is constructed) to handle the new Unknown/Result case so unknown
IPC inputs are logged and not silently remapped to Load.
---
Outside diff comments:
In `@app/src-tauri/src/webview_accounts/mod.rs`:
- Around line 2228-2238: Move the cold-spawn timer start to before the call to
parent_window.add_child so the measured elapsed_ms includes browser creation
time: acquire the spawn_started_at lock and insert args.account_id ->
Instant::now() before invoking parent_window.add_child (the code that currently
assigns webview), keeping the same data structure and error handling around
add_child and preserving the existing webview variable assignment and logging
paths.
---
Nitpick comments:
In `@app/src-tauri/src/webview_accounts/mod.rs`:
- Around line 265-317: The module mod.rs is getting large and mixing concerns:
extract the Google-SSO popup policy helpers (provider_supports_google_sso,
popup_should_navigate_parent, and any helpers like is_google_auth_popup) into a
small submodule (e.g., webview_accounts/google_sso.rs) and move the
reveal/telemetry trigger and state plumbing mentioned elsewhere into a separate
submodule (e.g., webview_accounts/reveal_telemetry.rs); update mod.rs to
re-export the functions or call them via the new submodules, keep public APIs
unchanged, and ensure unit tests/imports are updated to reference the new module
paths so mod.rs stays under ~500 lines and responsibilities are separated.
🪄 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: d370c450-4b2b-4a52-8c01-d6ef717c53fe
📒 Files selected for processing (1)
app/src-tauri/src/webview_accounts/mod.rs
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)
app/src-tauri/src/webview_accounts/mod.rs (1)
292-300:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle direct Google OAuth authorize popups, not just signin/account-chooser URLs.
This new Slack path only stays in-app when
is_google_auth_popup(url)returns true, but that helper currently looks forsignin/ServiceLogin/ chooser /SetSIDmarkers. A plain Google authorize URL such asaccounts.google.com/o/oauth2/...would still miss this branch and fall through to the system-browser handoff, which is exactly the Slack SSO breakage this PR is trying to close.Suggested fix
fn is_google_auth_popup(url: &Url) -> bool { let Some(host) = url.host_str() else { return false; }; if !is_google_sso_host(host) { return false; } let path = url.path().to_ascii_lowercase(); if path.contains("signin") || path.contains("servicelogin") || path.contains("accountchooser") || path.contains("chooseaccount") + || path.contains("oauth") || path.contains("setsid") { return true; } url.query_pairs().any(|(key, value)| { let k = key.to_ascii_lowercase(); let v = value.to_ascii_lowercase(); matches!(k.as_str(), "flowname" | "service" | "continue") && (v.contains("signin") || v.contains("servicelogin") || v.contains("accountchooser") || v.contains("chooseaccount") + || v.contains("oauth") || v.contains("meet.google.com") || v.contains("mail.google.com")) }) }🤖 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-tauri/src/webview_accounts/mod.rs` around lines 292 - 300, popup_should_navigate_parent currently only returns the popup URL for Google SSO when is_google_auth_popup matches signin/chooser markers, missing plain OAuth authorize endpoints (e.g., accounts.google.com/o/oauth2/...); update is_google_auth_popup (or add a new check used by popup_should_navigate_parent) to also detect Google OAuth authorize URLs by checking the host (accounts.google.com) and the path prefix "/o/oauth2" (and common variants like "/o/oauth2/v2/auth"), or by detecting OAuth-specific query params such as "client_id" and "redirect_uri"; keep provider_supports_google_sso and the existing about-scheme guard and return Some(url.clone()) when this new OAuth-authorize detection matches.
🤖 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 `@app/src-tauri/src/webview_accounts/mod.rs`:
- Around line 292-300: popup_should_navigate_parent currently only returns the
popup URL for Google SSO when is_google_auth_popup matches signin/chooser
markers, missing plain OAuth authorize endpoints (e.g.,
accounts.google.com/o/oauth2/...); update is_google_auth_popup (or add a new
check used by popup_should_navigate_parent) to also detect Google OAuth
authorize URLs by checking the host (accounts.google.com) and the path prefix
"/o/oauth2" (and common variants like "/o/oauth2/v2/auth"), or by detecting
OAuth-specific query params such as "client_id" and "redirect_uri"; keep
provider_supports_google_sso and the existing about-scheme guard and return
Some(url.clone()) when this new OAuth-authorize detection matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95458fdc-32e3-4537-9dab-d4136b11a0e7
📒 Files selected for processing (1)
app/src-tauri/src/webview_accounts/mod.rs
2 participants
Summary
google.com.load/watchdog), elapsed spawn-to-reveal time, and bounds for future first-paint investigations.Problem
popup_should_navigate_parentonly enabled Google auth popup parent-navigation forgoogle-meet; Slack had an existing Google auth detector but never reached it.Solution
provider_supports_google_sso(provider)and enabled it forgoogle-meetandslack.popup_should_navigate_parentto use that helper, while keeping themeet.google.comroom rewrite scoped togoogle-meetonly.accounts.google.com,accounts.googleusercontent.com,ssl.gstatic.com,fonts.gstatic.com,lh3.googleusercontent.com,oauth2.googleapis.com, andwww.googleapis.com.webview_account_reveal.reveal trigger=load elapsed_ms=2271; no geometry move, JS injection, or Commit E mitigation needed.Submission Checklist
docs/TESTING-STRATEGY.md10.1.4 Slack Connectionremains the affected row.## Relateddocs/TESTING-STRATEGY.md)Closes #NNNin the## RelatedsectionImpact
Related
10.1.4 Slack ConnectionSummary by CodeRabbit
New Features
Improvements
Tests