feat(accounts): loading overlay for first-time webview opens (#867)#887
Conversation
Drives the frontend loading overlay from the Rust side. `spawn_session` now spins a detached watchdog that emits `webview-account:load` with `state=timeout` after 15s so the UI never hangs on a flaky network. The per-account CDP session enables the Page domain before navigating and re-uses its pump_events loop as a belt-and-braces fallback to the native on_page_load handler — the server-side dedup in `emit_load_finished` keeps both signals idempotent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mansai#867) Fixes the first-open blank-frame flash: the CEF subview now spawns at 1×1 at the caller's requested position so the React loading spinner in `WebviewHost` is not covered by an empty native view. Once the native `on_page_load`, CDP `Page.loadEventFired`, or 15s watchdog fires, `emit_load_finished` grows the webview back to its requested bounds server-side. Dedup via `loaded_accounts` keeps the three signals idempotent and `requested_bounds` stores the rect for replay across reconnects. Warm re-opens emit a synthetic `state=reused` event so the frontend can flip straight to `open` without the loading cycle. Adds `webview_account_reveal` as a belt-and-braces frontend-driven reveal path — redundant once the server-side reveal runs but useful if the measured bounds diverge from the ones cached at open time. Closes tinyhumansai#867. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…umansai#867) Adds `'loading'` to `AccountStatus` between `'pending'` and `'open'` and wires the frontend to the Rust-side `webview-account:load` event. The service caches per-account bounds on every resize so the load handler can invoke `webview_account_reveal` with the most recent rect; bounds invokes are skipped while an account is still loading so the off-screen CEF subview doesn't get dragged back on-screen over the React spinner. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…yhumansai#867) Renders a centered spinner inside the placeholder div while the account status is `pending` or `loading`. The native CEF subview is parked at 1×1 by Rust during this window so the overlay is unobstructed — no z-index fight with the WKWebView layer. Carries a `webview-loading-{id}` testid so E2E regressions can assert the lifecycle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Vitest coverage for the `webview-account:load` listener — mocks Tauri `listen`/`invoke`, drives synthetic events, and asserts: - `openWebviewAccount` transitions pending → loading - `finished` and `timeout` both trigger `webview_account_reveal` with the cached bounds and flip status to `open` - Resize during loading updates the cache but skips the bounds invoke - `reused` (warm re-open) path is treated as a successful reveal - Events for unmounted accounts are a no-op Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughSpawned CDP sessions now return a Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend (React)
participant Svc as webviewAccountService
participant App as Tauri App
participant CDP as CDP Session
participant WD as Watchdog
FE->>Svc: openWebviewAccount(accountId, bounds)
Svc->>App: invoke webview_account_open(...)
App->>CDP: spawn_session(accountId, url) -> returns SpawnedSession(session, watchdog)
Note right of CDP: session runs run_session_cycle...\nPage.enable + navigate
CDP-->>Svc: (none yet) -- first paint waits for load event
par Page finishes
CDP->>App: emit webview-account:load {state:"finished", bounds?}
and Timeout expires
WD->>App: emit webview-account:load {state:"timeout"}
end
App->>Svc: frontend receives webview-account:load
Svc->>FE: dispatch open + invoke webview_account_reveal(bounds)
FE->>App: (optional) setWebviewAccountBounds + show
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/services/webviewAccountService.ts (1)
146-160: Consider clearing module-level Maps on service stop.
lastBoundsByAccountandloadingAccountsare not cleared instopWebviewAccountService(). If the service is stopped and restarted (e.g., hot reload in development), stale entries could cause unexpected behavior.♻️ Suggested addition to stopWebviewAccountService
export function stopWebviewAccountService(): void { if (unlisten) { unlisten(); unlisten = null; } if (unlistenNotifyClick) { unlistenNotifyClick(); unlistenNotifyClick = null; } if (unlistenLoad) { unlistenLoad(); unlistenLoad = null; } + lastBoundsByAccount.clear(); + loadingAccounts.clear(); started = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/webviewAccountService.ts` around lines 146 - 160, stopWebviewAccountService currently tears down listeners but leaves module-level Maps lastBoundsByAccount and loadingAccounts populated, which can leak stale state across restarts; update stopWebviewAccountService to clear both Maps (call their clear() methods or reassign new Map instances) after removing listeners so lastBoundsByAccount and loadingAccounts are emptied when the service stops, ensuring a clean state on subsequent starts.app/src-tauri/src/cdp/session.rs (1)
86-94: Watchdog task runs even after account close — consider cancellation.The detached watchdog continues running even if the account is closed/purged before the 15s timeout. While
emit_load_finisheddeduplicates, the task still holds clonedAppHandleandaccount_idreferences unnecessarily.This is a minor resource concern since the task is short-lived (15s max) and the emit is no-op after close, but for cleaner resource management you could store the
JoinHandleand abort it inwebview_account_close/webview_account_purgealongside the CDP session handle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src-tauri/src/cdp/session.rs` around lines 86 - 94, The watchdog spawned with tokio::spawn (sleep(LOAD_TIMEOUT) -> emit_load_finished) keeps cloned AppHandle and account_id alive even after account closure; change it to capture and store the JoinHandle when creating the watchdog (instead of detaching), associate it with the CDP session state, and on webview_account_close and webview_account_purge call join_handle.abort() (or await join_handle with timeout) to cancel the task; update the code paths that create the watchdog to return/store the JoinHandle and the cleanup paths in webview_account_close / webview_account_purge to abort the stored handle so the task does not hold resources after account removal.app/src/services/__tests__/webviewAccountService.loadListener.test.ts (1)
56-62: Consider usingvi.waitForor multiple ticks for async draining.The
await Promise.resolve()at line 61 drains a single microtask. IfhandleWebviewAccountLoadtriggers multiple chained promises (e.g., theinvokecall internally), some assertions might flake.Consider using
await vi.waitFor(() => expect(...))for assertions that depend on async side effects, or add a small explicit wait:await new Promise(r => setTimeout(r, 0));This is a minor robustness concern given the current implementation works.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/__tests__/webviewAccountService.loadListener.test.ts` around lines 56 - 62, The test helper fireLoadEvent currently uses a single microtask drain (await Promise.resolve()) which can flake if handleWebviewAccountLoad triggers chained async work (e.g., internal invoke calls); replace the microtask-only drain with a more robust wait: either use vi.waitFor in tests when asserting side effects that depend on async work, or change the final await in fireLoadEvent to await a next macrotask tick (e.g., await new Promise(r => setTimeout(r, 0))) so that chained promises and any invoke() completions have time to settle before assertions run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src-tauri/src/cdp/session.rs`:
- Around line 86-94: The watchdog spawned with tokio::spawn (sleep(LOAD_TIMEOUT)
-> emit_load_finished) keeps cloned AppHandle and account_id alive even after
account closure; change it to capture and store the JoinHandle when creating the
watchdog (instead of detaching), associate it with the CDP session state, and on
webview_account_close and webview_account_purge call join_handle.abort() (or
await join_handle with timeout) to cancel the task; update the code paths that
create the watchdog to return/store the JoinHandle and the cleanup paths in
webview_account_close / webview_account_purge to abort the stored handle so the
task does not hold resources after account removal.
In `@app/src/services/__tests__/webviewAccountService.loadListener.test.ts`:
- Around line 56-62: The test helper fireLoadEvent currently uses a single
microtask drain (await Promise.resolve()) which can flake if
handleWebviewAccountLoad triggers chained async work (e.g., internal invoke
calls); replace the microtask-only drain with a more robust wait: either use
vi.waitFor in tests when asserting side effects that depend on async work, or
change the final await in fireLoadEvent to await a next macrotask tick (e.g.,
await new Promise(r => setTimeout(r, 0))) so that chained promises and any
invoke() completions have time to settle before assertions run.
In `@app/src/services/webviewAccountService.ts`:
- Around line 146-160: stopWebviewAccountService currently tears down listeners
but leaves module-level Maps lastBoundsByAccount and loadingAccounts populated,
which can leak stale state across restarts; update stopWebviewAccountService to
clear both Maps (call their clear() methods or reassign new Map instances) after
removing listeners so lastBoundsByAccount and loadingAccounts are emptied when
the service stops, ensuring a clean state on subsequent starts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4119932-84db-4a64-82d8-d473e5f73f41
📒 Files selected for processing (7)
app/src-tauri/src/cdp/session.rsapp/src-tauri/src/lib.rsapp/src-tauri/src/webview_accounts/mod.rsapp/src/components/accounts/WebviewHost.tsxapp/src/services/__tests__/webviewAccountService.loadListener.test.tsapp/src/services/webviewAccountService.tsapp/src/types/accounts.ts
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Overall this is a well-structured implementation. The triple-signal dedup (native on_page_load + CDP Page.loadEventFired + 15 s watchdog), the 1x1 spawn rationale, and the server-side set_size in emit_load_finished (avoiding the blank-frame flash a round-trip IPC approach would have) are all solid design choices. Unit test coverage is meaningfully better than average for this area — the resize-during-load race and warm-reopen short-circuit are both exercised.
Two major items and three minor ones noted inline, all project-pattern specific:
- major: Detached watchdog handle leaks across close/purge, can cause a subsequent reopen to permanently swallow its load signal (
cdp/session.rs) - major:
stopWebviewAccountServicedoes not clear module-level maps; staleloadingAccountsentries survive a renderer restart and silently drop bounds updates (webviewAccountService.ts) - minor:
isLoadingistrueonundefinedstatus, showing an eternal spinner for accounts that have never been opened (WebviewHost.tsx) - minor: Full provider URLs (including OAuth codes / auth fragments) logged at
infolevel (webview_accounts/mod.rs) - minor: Double
vi.clearAllMocks()inbeforeEachmakes the mock config fragile (loadListener.test.ts)
| @@ -62,11 +69,33 @@ | |||
There was a problem hiding this comment.
[major] The watchdog task is spawned detached — its JoinHandle is immediately dropped. If the account is closed and purged within the 15-second window, the watchdog still fires emit_load_finished after close/purge have already cleaned up loaded_accounts and requested_bounds. The dedup guard in emit_load_finished inserts a fresh entry into loaded_accounts at that point, so a subsequent real reopen of the same account ID would silently swallow its first genuine load signal and the spinner would never dismiss.
Store the watchdog handle so it can be aborted in close/purge alongside the session handle. Simplest path: change spawn_session to return a tuple and add a watchdog_tasks map to WebviewAccountsState:
pub fn spawn_session<R: Runtime>(
app: AppHandle<R>,
account_id: String,
real_url: String,
) -> (JoinHandle<()>, JoinHandle<()>) { // (session, watchdog)
let watchdog = {
let app = app.clone();
let id = account_id.clone();
let url = real_url.clone();
tokio::spawn(async move {
sleep(LOAD_TIMEOUT).await;
emit_load_finished(&app, &id, "timeout", &url);
})
};
let session = tokio::spawn(async move {
run_session_forever(app, account_id, real_url).await
});
(session, watchdog)
}Abort both handles in webview_account_close / webview_account_purge.
There was a problem hiding this comment.
Resolved in 81178f5. spawn_session now returns SpawnedSession { session, watchdog }; webview_account_open stores both in WebviewAccountsState, and load_watchdogs is aborted in close + purge alongside cdp_sessions.
| /// brief blank frame that would otherwise sit between the load event and | ||
| /// the frontend's reveal call. | ||
| pub(crate) fn emit_load_finished<R: Runtime>( | ||
| app: &AppHandle<R>, |
There was a problem hiding this comment.
[minor] The full provider URL (including query string and fragment) is logged at info level in emit_load_finished. Some providers embed auth material in URLs — Telegram's #tgWebAppData fragment can carry session tokens, and OAuth callbacks land with ?code=.... Either log at debug level or strip query/fragment before logging:
let safe_url = url.split(|c: char| c == '?' || c == '#').next().unwrap_or(url);
log::info!("[webview-accounts][{}] load event state={} url={}", account_id, state, safe_url);There was a problem hiding this comment.
Resolved in 81178f5. New redact_url_for_log strips query + fragment via Url::parse (with split(['?', '#']) fallback). Full URL still flows over the Tauri event for in-process consumers.
| } | ||
| try { | ||
| // Rust emits `webview-account:load` from three independent signals | ||
| // (native `on_page_load`, CDP `Page.loadEventFired`, 15 s watchdog). |
There was a problem hiding this comment.
[major] loadingAccounts and lastBoundsByAccount are module-level singletons that outlive the service lifecycle. stopWebviewAccountService removes the Tauri listener but does not clear these maps. If the service restarts (renderer HMR reload, or in tests as papered over by calling closeWebviewAccount in beforeEach), stale loadingAccounts entries cause setWebviewAccountBounds to silently skip the Rust invoke for accounts that were loading when the service was stopped. In production this means a renderer reload while an account is loading leaves the webview stuck at 1x1 forever.
Add these two lines to stopWebviewAccountService:
lastBoundsByAccount.clear();
loadingAccounts.clear();There was a problem hiding this comment.
Resolved in c3b56f8. stopWebviewAccountService clears lastBoundsByAccount + loadingAccounts after listener teardown.
| provider: AccountProvider; | ||
| } | ||
|
|
||
| const LOADING_STATUSES: ReadonlySet<AccountStatus> = new Set(['pending', 'loading']); |
There was a problem hiding this comment.
[minor] isLoading is true when status === undefined, which means any account that has never been opened renders the spinner indefinitely. If WebviewHost is ever rendered before openWebviewAccount is called — pre-mounted route, Storybook story, test — it will spin forever with no dismissal path.
Restrict to accounts actively in flight:
const isLoading = status === "pending" || status === "loading";When status is undefined the webview has not been opened yet — an empty placeholder is more correct than a spinning loader.
There was a problem hiding this comment.
Resolved in c3b56f8. isLoading is now status !== undefined && LOADING_STATUSES.has(status) — undefined-status hosts no longer render the spinner.
| startWebviewAccountService, | ||
| stopWebviewAccountService, | ||
| } from '../webviewAccountService'; | ||
|
|
There was a problem hiding this comment.
[minor] beforeEach calls vi.clearAllMocks() twice: once before closeWebviewAccount and once after. The first clear wipes the invoke mock's resolved-value configuration, so closeWebviewAccount's internal invoke('webview_account_close', ...) call runs against the bare default (resolves undefined — fine today). If the default ever changes to reject, beforeEach will throw and every test will fail with a confusing error. Consider a single vi.clearAllMocks() at the end of setup, or call vi.mocked(invoke).mockResolvedValue(undefined) explicitly after the first clear.
There was a problem hiding this comment.
Resolved in 676d5ec. Dropped redundant vi.clearAllMocks(); fireLoadEvent now drains a macrotask (setTimeout(0)) so chained .then/.catch on the internal invoke() settle before assertions. 7/7 still pass.
|
Reviews are damn nice @graycyrus |
…tinyhumansai#867) Two review fixes for tinyhumansai#887: - The 15s load-event watchdog was spawned detached, so a close or purge within the watchdog window left the task running until it fired `webview-account:load{state:"timeout"}` for a now-vanished account. Worst case: the same account_id was reused (e.g. across a restart with persisted ids) before the timer expired, and the stale watchdog flipped the freshly-spawned account to `'open'` before its page had loaded. Fix: `spawn_session` now returns both the session and the watchdog `JoinHandle` (`SpawnedSession`); the watchdog handle is stored in `WebviewAccountsState.load_watchdogs` and aborted in `webview_account_close` / `webview_account_purge` alongside the CDP session task. - `emit_load_finished` was logging the full URL at info, which for some providers (Telegram WebApp `#tgWebAppData=…`, OAuth callbacks) contains auth material that ends up in the long-lived shell log. Strip query and fragment via the new `redact_url_for_log` helper before logging; the full URL still flows over the Tauri event so any in-process consumer that needs it has access. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nyhumansai#867) Two review fixes for tinyhumansai#887: - `stopWebviewAccountService` removed the Tauri listeners but left the module-level `lastBoundsByAccount` and `loadingAccounts` Maps populated. A subsequent start (HMR or shutdown→restart) would see stale entries and silently drop bounds updates because `loadingAccounts.has(...)` was true for accounts whose webviews had long since been destroyed. Clear both Maps as part of stop. - `WebviewHost.isLoading` was true on `status === undefined`, so a host rendered for an account not in the store (e.g. a render race with `addAccount`) would spin forever. Tighten the guard to `status !== undefined && (pending|loading)` — the brief microtask between mount and the `setAccountStatus('pending')` dispatch is visually indistinguishable from no overlay, so this is safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nsai#867) Two review fixes for tinyhumansai#887: - `beforeEach` was calling `vi.clearAllMocks()` twice — once before `closeWebviewAccount` and once after. The first clear wiped the default `mockResolvedValue(undefined)` config so any test that relied on `closeWebviewAccount` actually round-tripping through the mocked `invoke` would have been brittle. Drop the redundant clear; rely on `stopWebviewAccountService` (now clearing the module-level Maps) for state teardown. - `fireLoadEvent` drained a single microtask via `await Promise.resolve()`, which was sufficient for the current handler but would miss chained `.then()` / `.catch()` on the internal `invoke()` promise. Switch to a macrotask drain (`setTimeout(0)`) so future handler changes don't silently flake. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sibility, rustdoc order
- allow-core-process.toml: add webview_account_reveal and webview_account_purge
to the allowed-commands list (ACL gap reported by reviewer)
- webviewAccountService.ts: move setAccountStatus('open') into .finally() so
it fires after webview_account_reveal settles rather than immediately
- webview_accounts/mod.rs: drop unnecessary pub(crate) on load_watchdogs field
- cdp/mod.rs: resolve merge conflict — keep placeholder_url + SpawnedSession exports
- cdp/session.rs: move rustdoc comment to sit directly above spawn_session fn,
above the SpawnedSession struct definition it documents
| if method == "Page.loadEventFired" { | ||
| emit_load_finished(&cb_app, &cb_account_id, "finished", &cb_real_url); | ||
| } | ||
| }) |
There was a problem hiding this comment.
Open question: should Page.loadEventFired filter on real_url prefix before calling emit_load_finished?
The current callback fires emit_load_finished on any Page.loadEventFired event regardless of which URL triggered it. During a typical provider login flow the webview may traverse several OAuth/SSO redirects (e.g. accounts.google.com → oauth2.example.com → provider.com) before reaching the real provider URL. Each redirect fires Page.loadEventFired, so the "finished" signal can arrive while the webview is still on an interim OAuth page — triggering a premature reveal of an incomplete (still loading) webview.
A guard like:
if method == "Page.loadEventFired" {
// Only signal finished when the webview has reached the actual provider domain,
// not on OAuth / SSO interim redirects.
if cb_target_url.starts_with(&cb_real_url) {
emit_load_finished(&cb_app, &cb_account_id, "finished", &cb_real_url);
}
}would require tracking the current target URL (e.g. via Page.frameNavigated events or a shared Arc<Mutex<String>> updated by the navigation handler) and comparing against real_url before emitting.
Trade-off: the watchdog fires after 15 s anyway, so a premature reveal is recoverable. But for providers with long OAuth flows (LinkedIn, Google Meet) this could be a visible UX regression — the spinner disappears while the page is still mid-redirect.
Is intentional that the reveal can fire on OAuth interims, or should we add the URL prefix check?
There was a problem hiding this comment.
Good catch — this is intentional, and the origin filter as proposed would actually break the cold-login UX. Flow:
- User opens an account for the first time (not logged in)
Page.navigate(mail.google.com)→ 302 →accounts.google.com/loginloadEventFiredfires foraccounts.google.com— with thestarts_with(real_url)guard we'd reject it- Webview stays at 1×1 for the entire OAuth flow, so the user can't see or interact with the login form
- Stuck until the 15 s watchdog fires, at which point mail.google.com hasn't been reached yet either
So the current "reveal on first loadEventFired" behaviour is load-bearing for first-login: the user needs to see whatever CEF painted first (login form, consent screen, interim redirect) to interact with it. A reveal that lands on an OAuth page is a correct user state, not a premature one.
The silent-SSO-hop case you're flagging — an already-logged-in user seeing a brief flash of accounts.google.com before the instant redirect to mail.google.com — is real, but:
- Typical duration is <200 ms on a warm connection
loaded_accountsdedup locks the first fire, so subsequent navigations are free to render instantly without re-triggering reveal- The native
on_page_loadhandler usually wins the race on CEF (verified in the spike logs), so the CDP path is belt-and-braces, not primary
If we do want to remove that flash in follow-up, the right shape is probably "filter out same-origin redirects within the first N ms" via Page.frameNavigated + a freshness window, not a hard origin gate — but that's a polish follow-up, not a blocker for this PR.
Happy to open a follow-up issue tracking the silent-SSO flash if you'd like.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/services/webviewAccountService.ts (3)
170-203: Consider adding success logging after reveal settles.The handler logs on entry (line 176) and on error (line 195), but there's no log when the reveal succeeds or when the
'open'status is dispatched. Per the coding guidelines, logging at exit points aids debugging. A brief log in.finally()or after the else-branch dispatch would complete the trace.Suggested addition
.finally(() => { store.dispatch(setAccountStatus({ accountId, status: 'open' })); + log('load complete account=%s → open', accountId); }); } else { store.dispatch(setAccountStatus({ accountId, status: 'open' })); + log('load complete (no cached bounds) account=%s → open', accountId); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/webviewAccountService.ts` around lines 170 - 203, Add an exit/ success log to handleWebviewAccountLoad so we trace successful reveals and state dispatches: when invoking invoke('webview_account_reveal', ...) add a success/info log in the .finally() alongside the existing store.dispatch(setAccountStatus({ accountId, status: 'open' })) to indicate reveal settled and status set, and add the same info log immediately after the else-branch dispatch for the case where bounds is falsy; reference lastBoundsByAccount, loadingAccounts, invoke, and setAccountStatus to locate the spots to change.
62-69: The| stringfallback weakens the typed union.Line 67's
state: 'finished' | 'timeout' | 'reused' | stringcollapses to juststringat the type level, losing narrowing and autocomplete for known states. If forward-compat is needed, handle unknown states explicitly in the handler instead.Suggested fix
interface WebviewAccountLoadPayload { account_id: string; // `'finished'` — native `on_page_load` or CDP `Page.loadEventFired` fired // `'timeout'` — 15 s watchdog elapsed; reveal anyway so spinner isn't stuck // `'reused'` — warm re-open of already-loaded account; reveal synchronously - state: 'finished' | 'timeout' | 'reused' | string; + state: 'finished' | 'timeout' | 'reused'; url: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/webviewAccountService.ts` around lines 62 - 69, The WebviewAccountLoadPayload type currently weakens the union by ending with "| string"; change the declaration so state is strictly 'finished' | 'timeout' | 'reused' (remove the trailing "| string"), and update any consumers/handlers that read WebviewAccountLoadPayload.state to explicitly handle unexpected values via a default/fallback branch (e.g., an explicit else/default case or a runtime typeof/string check) so unknown states are handled safely rather than relying on the type system to permit arbitrary strings.
1-976: Consider splitting this service into submodules.The file is now ~976 lines, exceeding the ~500-line guideline. The Google Meet transcript pipeline (lines 454-798), notification bypass helpers (lines 906-975), and the core webview lifecycle logic are largely independent. Extracting them into co-located submodules (e.g.,
webviewAccountService/meetTranscript.ts) would improve maintainability.This is a pre-existing concern and doesn't need to block this PR — flagging for future cleanup. As per coding guidelines: "Source files should be ≤ ~500 lines; split components and modules when growing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/webviewAccountService.ts` around lines 1 - 976, This file is too large; split independent concerns into submodules: extract the Google Meet transcript pipeline (types MeetCaptionRow, MeetCallStartedPayload, MeetCaptionsPayload, MeetCallEndedPayload, CaptionSnapshot, MeetingSession, TranscriptSegment, MAX_MEET_SNAPSHOTS, activeMeetings, and functions handleMeetCallStarted, handleMeetCaptions, handleMeetCallEnded, flushMeetingSession, collapseToSegments, renderTranscript, handoffToOrchestrator, and any helpers they use) into a new module (e.g., meetTranscript) that exports the handler functions and flushMeetingIfAny if needed, then import and call those from webviewAccountService; likewise move the notification bypass helpers (setAccountMuted, setGlobalDnd, getBypassPrefs, setFocusedAccount) into a notificationBypass module and import them back; ensure all references (activeMeetings usage, flushMeetingIfAny, MEET_ORCHESTRATOR_MODEL, threadApi/chatSend/callCoreRpc/store.dispatch/log/errLog) are either passed in or imported, and preserve existing semantics and exported signatures so callers (openWebviewAccount, closeWebviewAccount, purgeWebviewAccount, etc.) keep working.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/services/webviewAccountService.ts`:
- Around line 170-203: Add an exit/ success log to handleWebviewAccountLoad so
we trace successful reveals and state dispatches: when invoking
invoke('webview_account_reveal', ...) add a success/info log in the .finally()
alongside the existing store.dispatch(setAccountStatus({ accountId, status:
'open' })) to indicate reveal settled and status set, and add the same info log
immediately after the else-branch dispatch for the case where bounds is falsy;
reference lastBoundsByAccount, loadingAccounts, invoke, and setAccountStatus to
locate the spots to change.
- Around line 62-69: The WebviewAccountLoadPayload type currently weakens the
union by ending with "| string"; change the declaration so state is strictly
'finished' | 'timeout' | 'reused' (remove the trailing "| string"), and update
any consumers/handlers that read WebviewAccountLoadPayload.state to explicitly
handle unexpected values via a default/fallback branch (e.g., an explicit
else/default case or a runtime typeof/string check) so unknown states are
handled safely rather than relying on the type system to permit arbitrary
strings.
- Around line 1-976: This file is too large; split independent concerns into
submodules: extract the Google Meet transcript pipeline (types MeetCaptionRow,
MeetCallStartedPayload, MeetCaptionsPayload, MeetCallEndedPayload,
CaptionSnapshot, MeetingSession, TranscriptSegment, MAX_MEET_SNAPSHOTS,
activeMeetings, and functions handleMeetCallStarted, handleMeetCaptions,
handleMeetCallEnded, flushMeetingSession, collapseToSegments, renderTranscript,
handoffToOrchestrator, and any helpers they use) into a new module (e.g.,
meetTranscript) that exports the handler functions and flushMeetingIfAny if
needed, then import and call those from webviewAccountService; likewise move the
notification bypass helpers (setAccountMuted, setGlobalDnd, getBypassPrefs,
setFocusedAccount) into a notificationBypass module and import them back; ensure
all references (activeMeetings usage, flushMeetingIfAny,
MEET_ORCHESTRATOR_MODEL,
threadApi/chatSend/callCoreRpc/store.dispatch/log/errLog) are either passed in
or imported, and preserve existing semantics and exported signatures so callers
(openWebviewAccount, closeWebviewAccount, purgeWebviewAccount, etc.) keep
working.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb62eb6d-8194-490b-bb87-439e8b989769
📒 Files selected for processing (7)
app/src-tauri/permissions/allow-core-process.tomlapp/src-tauri/src/cdp/mod.rsapp/src-tauri/src/cdp/session.rsapp/src-tauri/src/lib.rsapp/src-tauri/src/webview_accounts/mod.rsapp/src/services/webviewAccountService.tsapp/src/types/accounts.ts
✅ Files skipped from review due to trivial changes (3)
- app/src-tauri/permissions/allow-core-process.toml
- app/src-tauri/src/lib.rs
- app/src-tauri/src/webview_accounts/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/types/accounts.ts
- app/src-tauri/src/cdp/mod.rs
- app/src-tauri/src/cdp/session.rs
…ansai#867) (tinyhumansai#887) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
4 participants
Summary
WebviewBuilder::on_page_load, CDPPage.loadEventFired, and a 15s watchdog — dedup server-side vialoaded_accounts, first signal wins.AccountStatus = 'loading'variant andwebview-account:loadTauri event ({account_id, state: 'finished' | 'timeout' | 'reused', url}).Problem
Clicking into an account (Telegram, WhatsApp, LinkedIn, Slack, …) for the first time shows an empty webview for several seconds while CEF loads the provider page. There is no indication that anything is happening, and earlier in-session attempts at a React overlay,
Webview::hide(), or off-screen spawn all hit one of: the CEFNSViewpaints over the WKWebView layer (z-index can't reach across native layers),set_visibleraces NSView allocation, or the native page-loaded signal doesn't propagate for CDP-driven navigations.Solution
Rust
webview_account_openspawns the CEF subview at 1×1 at the requested position (behind the React spinner) and stores the requested rect inWebviewAccountsState.requested_bounds.emit_load_finishedgrows the webview back to full bounds + callsshow(), then emitswebview-account:load. Dedup viaWebviewAccountsState.loaded_accountsmeans the three signal sources converge to exactly one event per cold open.cdp::sessionnow enables the Page domain before navigating and firesemit_load_finishedfrom itspump_eventscallback whenPage.loadEventFiredarrives. A detached 15s watchdog guarantees the overlay is released even if CDP is quiet.WebviewBuilder::on_page_loadis wired as the third signal; adata:URL filter keeps the placeholder from triggering it.state=reusedevent so the frontend flips straight toopenwithout the loading cycle.webview_account_revealTauri command is kept as a belt-and-braces idempotent call — Rust already did the reveal, this just reapplies the latest measured rect if it diverged during load.Frontend
AccountStatusgets a new'loading'state sitting between'pending'and'open'.webviewAccountServicecaches last bounds per account (updated on every resize) so thewebview-account:loadhandler can issue the idempotent reveal without a second round-trip. Bounds invokes are skipped while loading so nothing drags the webview back on-screen.WebviewHostrenders a centered spinner overlay when the account status is'pending' | 'loading', carrying awebview-loading-{id}testid.Why 1×1 instead of off-screen
An earlier attempt parked the webview off-screen (e.g.
y + parent_height + 2000). CEF loaded the page fine, but afterset_positionmoved it back the NSView contents sometimes stayed blank until a reload forced a repaint. Keeping position stable and only toggling size sidesteps the repaint edge case.Submission Checklist
app/src/services/__tests__/webviewAccountService.loadListener.test.tscovers the listener flow (pending → loading → open, bounds cache,finished/timeout/reusedbranches, unmounted-account no-op).///onemit_load_finished,webview_account_reveal, new state fields; JSDoc-ish comments on the frontend module-locallastBoundsByAccount/loadingAccountsmaps and theAccountStatuslifecycle.on_page_load.Impact
wryfallback keeps existing behavior — no loading overlay, no regressions.Page.enablecall per cold open, a detached 15stokio::spawnthat exits immediately once the first signal fires (emit_load_finisheddedup), and oneset_size+showper cold open. Negligible.'loading'is an additiveAccountStatusvariant; existing reducers treat unknown statuses safely (verified in Vitest).webview-account:loadis a new event; no existing listeners to conflict with.data:placeholder check onon_page_loadguards against leaking frontend events for the internal CDP bootstrap URL.Related
Summary by CodeRabbit
New Features
Bug Fixes
Tests