Conversation
Kept both Electron OAuth redirect flow (electron-oauth branch) and hosted workspace OAuth flow (main). All 5 conflicted files resolved: - App.tsx, OAuthFlowProgressSimple.tsx: import merges - mcp-oauth.ts: kept ELECTRON_MCP_CALLBACK_STATE_PREFIX + Electron state helpers alongside hosted OAuth interfaces; replaced getMCPOAuthRedirectUri with getRedirectUri from constants (consistent with main) - use-server-state.ts: kept buildElectronMcpCallbackUrl + shouldRetryOAuthConnectionFailure alongside restorePathAfterOAuthCallback + requiresFreshOAuthAuthorization - use-server-state.test.tsx: kept all Electron tests + main's restore-path test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ignaciojimenezr
temporarily deployed
to
preview-pr-1870
GitHub Actions
Inactive
dosubot
Bot
added
the
size:XXL
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
ignaciojimenezr
changed the title
Internal previewPreview URL: https://mcp-inspector-pr-1870.up.railway.app |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds comprehensive Electron-aware OAuth handling and hosted-auth utilities across renderer and main processes. Introduces Electron deep-link construction and detection, desktop return UI/component, Electron-specific OAuth state tagging, window IPC for opening external URLs, and guarded main-process navigation/opening logic. Client-side hooks and components short-circuit browser callback flows to emit or redirect to 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts (1)
592-618:⚠️ Potential issue | 🟡 MinorSilent fallthrough when
isElectronis true butelectronAPIis absent.If
window.isElectronistrueyetwindow.electronAPI?.app?.openExternalis missing (preload misconfiguration, older desktop build, etc.), the code silently performs in-appwindow.location.assign(...), which will navigate the Electron renderer itself to the provider — the very behavior this feature is meant to avoid, and one that can land the user at anmcpjam://redirect the browser engine inside Electron cannot resolve. Worth aconsole.warnat minimum so the fallback is observable in production logs.🛡️ Suggested hardening
- if (window.isElectron && window.electronAPI?.app?.openExternal) { - try { - await window.electronAPI.app.openExternal(authorizationUrl.toString()); - return; - } catch (error) { - console.error( - "Failed to open system browser for MCP OAuth, falling back to in-app navigation:", - error, - ); - } - } + if (window.isElectron) { + if (window.electronAPI?.app?.openExternal) { + try { + await window.electronAPI.app.openExternal( + authorizationUrl.toString(), + ); + return; + } catch (error) { + console.error( + "Failed to open system browser for MCP OAuth, falling back to in-app navigation:", + error, + ); + } + } else { + console.warn( + "Electron detected but electronAPI.app.openExternal is unavailable; falling back to in-app navigation.", + ); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts` around lines 592 - 618, In redirectToAuthorization detect the case where window.isElectron is true but window.electronAPI?.app?.openExternal is missing and emit a visible warning before falling back to in-app navigation; update redirectToAuthorization to log a console.warn (including serverName and the authorizationUrl) when window.isElectron && !window.electronAPI?.app?.openExternal so operators can see the preload/misconfiguration, then proceed to call this.navigateToUrl(url) as the existing fallback; keep existing localStorage usage (mcp-oauth-pending, mcp-oauth-return-hash) unchanged.
🧹 Nitpick comments (10)
mcpjam-inspector/server/__tests__/app-listeners.test.ts (1)
71-108: Add tests for the URL validation branches.This IPC handler is a security boundary; the suite currently covers sender trust but not invalid URLs or rejected protocols.
🧪 Suggested coverage
it("rejects stale senders after the trusted window changes", async () => { const openExternal = getOpenExternalHandler(); currentMainWindow = createWindow(2); await expect( openExternal({ sender: { id: 1 } }, "https://example.com/oauth"), ).rejects.toThrow("Refusing external open from untrusted renderer"); expect(openExternalMock).not.toHaveBeenCalled(); expect(logWarnMock).toHaveBeenCalledWith( "Ignoring open-external from untrusted sender (id: 1)", ); }); + + it("rejects invalid external URLs", async () => { + const openExternal = getOpenExternalHandler(); + + await expect( + openExternal({ sender: { id: 1 } }, "not a url"), + ).rejects.toThrow("Refusing to open invalid external URL"); + + expect(openExternalMock).not.toHaveBeenCalled(); + }); + + it("rejects non-HTTP external URLs", async () => { + const openExternal = getOpenExternalHandler(); + + await expect( + openExternal({ sender: { id: 1 } }, "file:///tmp/oauth"), + ).rejects.toThrow("Refusing to open non-HTTP external URL"); + + expect(openExternalMock).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/__tests__/app-listeners.test.ts` around lines 71 - 108, Add tests exercising the URL validation and protocol-rejection branches of the open-external IPC handler: use getOpenExternalHandler() to obtain the handler and call it with trusted sender objects (use currentMainWindow = createWindow(...) to set trusted id) but supply invalid URLs (e.g., malformed URI) and disallowed protocols (e.g., "file://", "ftp://") and assert the handler rejects/throws with the same error message used for protocol/URL checks, that openExternalMock is not called, and that logWarnMock (or the specific logger used) is invoked with the corresponding warning; reference the existing test helpers openExternalMock, logWarnMock, currentMainWindow, createWindow and the handler returned by getOpenExternalHandler to locate where to insert these cases.mcpjam-inspector/client/src/components/oauth/__tests__/OAuthDebugCallback.test.tsx (1)
4-21: Tight but thin: consider a negative case.The happy path is well-covered, yet
buildElectronDebugCallbackUrlhas a non-trivial environment-sensitive contract (expected to early-returnnulloutside the debug callback path, or when inside Electron). A companion assertion for those branches would keep future refactors honest without much ceremony.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/oauth/__tests__/OAuthDebugCallback.test.tsx` around lines 4 - 21, Add negative-case tests for buildElectronDebugCallbackUrl: one that sets window.isElectron = true (simulating Electron) and another that sets the URL to a non-debug callback path (e.g., not containing /oauth/callback/debug or missing code/state) and assert that buildElectronDebugCallbackUrl() returns null in both cases; use the same test harness setup (vi.clearAllMocks(), window.history.replaceState(), window.name as needed) and reference buildElectronDebugCallbackUrl to locate where to add these assertions so future refactors don't break the early-return behavior.mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts (1)
327-385: Good coverage for the Electron paths — one suggestion.The fallback test asserts
consoleErrorSpywas called but not with what. Tightening that toexpect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Failed to open system browser"), expect.any(Error))would prevent a future silent regression where the log message gets swapped for something unrelated yet still fires.Also consider asserting ordering —
openExternalmust be awaited beforenavigateToUrlfires — viaexpect(openExternal.mock.invocationCallOrder[0]).toBeLessThan(navigateSpy.mock.invocationCallOrder[0]), which locks in the "try Electron first, then fall back" contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts` around lines 327 - 385, Update the fallback test for MCPOAuthProvider.redirectToAuthorization to assert the specific console error shape and ordering: replace the loose expect(consoleErrorSpy).toHaveBeenCalled() with expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Failed to open system browser"), expect.any(Error)) to ensure the logged message and error are correct, and add an ordering assertion using the spies' invocationCallOrder (e.g., expect(openExternal.mock.invocationCallOrder[0]).toBeLessThan(navigateSpy.mock.invocationCallOrder[0])) to guarantee openExternal is awaited before navigateToUrl is invoked.mcpjam-inspector/src/ipc/listeners-register.ts (1)
7-15: Default factory undermines its own purpose — minor nit.The
getMainWindowparameter exists so consumers (notably theapp:open-externalhandler) can read the current main window at invocation time. The default() => mainWindowcloses over the value captured at registration, which is fine today because the caller inmain.tsalso closes over the same ref, but the default silently removes the recreate-safety the factory was introduced to provide. If feasible, consider makinggetMainWindowa required parameter so call sites must be intentional; otherwise it's worth a short comment noting the default is only safe whilemainWindowis never reassigned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/src/ipc/listeners-register.ts` around lines 7 - 15, The default factory getMainWindow closes over the initial mainWindow and defeats the safe-late-binding purpose of getMainWindow; update registerListeners to require getMainWindow instead of defaulting (i.e., remove the default parameter) so callers (e.g., the app:open-external handler) must pass a function that reads the current mainWindow, or if you must keep a default, replace the default with an explicit comment next to getMainWindow explaining it only remains safe while the mainWindow variable is never reassigned; adjust call sites accordingly (notably where registerListeners is invoked) to pass a live accessor.mcpjam-inspector/client/src/lib/__tests__/electron-hosted-auth.test.ts (1)
11-62: Coverage is a touch optimistic — happy paths only.The suite exercises one branch per helper. Given the number of conditionals in
createElectronHostedAuthState(object / undefined / primitive fallback),parseElectronHostedAuthState(null input, malformed JSON, missing tag),isElectronHostedAuthCallback(wrong pathname, missingcode/error, unparsablestate), andresolveWorkosRedirectUri(isElectron false, https, missingenvRedirect, custom protocol), a handful of negative cases would materially harden the Electron handoff against regressions. Not a blocker; a good candidate for a follow-up test sweep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/__tests__/electron-hosted-auth.test.ts` around lines 11 - 62, Tests only cover the happy paths; add negative-case unit tests to exercise alternate branches: for createElectronHostedAuthState add tests for undefined input and primitive input to verify fallback tagging behavior; for parseElectronHostedAuthState add tests for null/empty input, malformed JSON, and JSON missing ELECTRON_HOSTED_AUTH_STATE_KEY to ensure it returns null or original value per implementation; for isElectronHostedAuthCallback add tests for wrong pathname, missing code and missing error params, and unparsable state to assert it returns false; and for resolveWorkosRedirectUri add tests for isElectron=false, https origin, missing envRedirect, and a custom protocol envRedirect to validate each branch. Reference helpers by name (createElectronHostedAuthState, parseElectronHostedAuthState, isElectronHostedAuthCallback, buildElectronHostedAuthCallbackUrl, resolveWorkosRedirectUri, ELECTRON_HOSTED_AUTH_STATE_KEY) when adding these assertions.mcpjam-inspector/client/src/components/oauth/__tests__/OAuthDesktopReturnNotice.test.tsx (1)
48-63: StrictMode dedupe test is well-conceived.Guarding against double-firing under React 19's intensified StrictMode double-invocation is exactly the right instinct for a side-effecting redirect. One small addition: asserting
resetDesktopReturnAttemptsForTestsis what actually allows subsequent tests to fire would close the loop on the dedupe mechanism itself — otherwise the dedupe could become an accidental permanent lock and the suite wouldn't notice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/oauth/__tests__/OAuthDesktopReturnNotice.test.tsx` around lines 48 - 63, The test currently verifies desktopReturnRuntime.redirect is only called once under StrictMode but doesn't reset the dedupe state; update the test to also call or assert usage of resetDesktopReturnAttemptsForTests so subsequent tests aren't blocked—for example, after rendering OAuthDesktopReturnNotice (or in an afterEach) invoke resetDesktopReturnAttemptsForTests (or expect it to have been called) to clear the dedupe state created by the component's redirect logic; refer to desktopReturnRuntime.redirect, OAuthDesktopReturnNotice, and resetDesktopReturnAttemptsForTests to locate and update the test.mcpjam-inspector/client/src/components/OAuthFlowTab.tsx (1)
436-470: Electron callback bypasses thestateguard enforced elsewhere.
processOAuthCallbackwill reject a mismatchedstate, so the code path itself is safe — but note that here you pass the rawstatefrom the deep link straight through, meaning a stale/foreignstatewill surface as a user-facing "Invalid state parameter" error rather than a silent drop. That is likely the intended UX; flagging it so the choice is deliberate.Additionally, the caught
errorin the outertry/catchwill only trigger for malformed URLs (new URLthrowing). Consider surfacing a subtlerupdateOAuthFlowState({ error: ... })so the user sees why the debug flow stalled, rather than only a console trace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/OAuthFlowTab.tsx` around lines 436 - 470, handleElectronOAuthCallback currently forwards the raw deep-link state to processOAuthCallback and only logs URL parse errors to console; change it to validate the incoming state against your stored expected state (e.g., compare state to oauthStateRef.current or similar) before calling processOAuthCallback and treat mismatches by clearing exchangeTimeoutRef (if set) and calling updateOAuthFlowState({ error: "State mismatch" }) (or silently drop, pick the intended UX), so the user sees a clear message instead of an internal "Invalid state parameter"; also in the catch block replace the console.error with updateOAuthFlowState({ error: `Failed to process callback: ${String(error)}` }) and clear exchangeTimeoutRef there as well so the UI surfaces malformed-URL failures.mcpjam-inspector/client/src/hooks/use-server-state.ts (1)
872-881: Stale dependency:guardedTestConnectionno longer appears inside this callback.The OAuth completion path now delegates to
testConnectionAfterOAuth(Line 795), so listingguardedTestConnectionin theuseCallbackdeps keeps the identity churn without any corresponding closure reference. Tidy to remove.♻️ Proposed trim
[ dispatch, failPendingOAuthConnection, isAuthenticated, logger, storeInitInfo, testConnectionAfterOAuth, - guardedTestConnection, withWorkspaceClientCapabilities, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 872 - 881, The dependency array for the useCallback that includes dispatch, failPendingOAuthConnection, isAuthenticated, logger, storeInitInfo, testConnectionAfterOAuth, guardedTestConnection, withWorkspaceClientCapabilities should be trimmed: remove guardedTestConnection from the dependency list because the callback no longer references it (OAuth completion now delegates to testConnectionAfterOAuth). Update the useCallback dependency array to include the actual referenced symbols (keep testConnectionAfterOAuth, etc.) so React won't see spurious identity churn from guardedTestConnection.mcpjam-inspector/src/main.ts (1)
42-43: Single-slot buffer can swallow pre-bootstrap callbacks.
pendingProtocolUrlonly retains the most recent URL, so if more than oneopen-urlevent lands beforewhenReady()resolves, the earlier ones are quietly overwritten. In practice it's rare, but a small queue would be both safer and cheaper than debugging the intermittent loss later.♻️ Proposed tweak
-let pendingProtocolUrl: string | null = null; +const pendingProtocolUrls: string[] = []; @@ - if (!appBootstrapped) { - pendingProtocolUrl = url; - return; - } + if (!appBootstrapped) { + pendingProtocolUrls.push(url); + return; + } @@ - if (pendingProtocolUrl) { - const protocolUrl = pendingProtocolUrl; - pendingProtocolUrl = null; - await handleOAuthCallbackUrl(protocolUrl); - } + while (pendingProtocolUrls.length > 0) { + const protocolUrl = pendingProtocolUrls.shift()!; + await handleOAuthCallbackUrl(protocolUrl); + }Also applies to: 399-411
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/src/main.ts` around lines 42 - 43, pendingProtocolUrl is a single-slot buffer that can drop multiple early open-url events; replace it with a queue (e.g., pendingProtocolUrls: string[]), update the open-url event handler to push each incoming URL onto that queue and, if appBootstrapped is true, process it immediately, and change the whenReady()/bootstrap completion path (and the code referenced around pending handling, including the logic used near the later block at the other occurrence) to drain the queue by iterating over pendingProtocolUrls and handling each URL in order, then clear the queue; ensure you update any checks that referenced pendingProtocolUrl to handle an empty vs non-empty array.mcpjam-inspector/client/src/lib/electron-hosted-auth.ts (1)
13-33: Non-recordstategets quietly restructured underoriginalState.When callers pass a primitive or array
state, this helper stashes it under anoriginalStatefield.parseElectronHostedAuthStatereturns the wrapper as-is, so any downstream consumer expecting the original shape must know to unwrap it — an asymmetry that isn't documented. If in practice only object-shaped states are ever forwarded (the WorkOS/Electron flows here seem to), consider either narrowing the parameter type toRecord<string, unknown> | undefinedor adding a short JSDoc noting theoriginalStatewrapping contract so the next reader doesn't have to infer it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/electron-hosted-auth.ts` around lines 13 - 33, The helper createElectronHostedAuthState currently wraps non-object states under originalState which creates an undocumented asymmetry with parseElectronHostedAuthState; change the function signature to accept state?: Record<string, unknown> (i.e. narrow the parameter type to Record<string, unknown> | undefined) and remove the branch that wraps primitives/arrays in originalState so it only accepts and forwards object-shaped state, or alternatively add a concise JSDoc above createElectronHostedAuthState explaining the deliberate originalState wrapper and that parseElectronHostedAuthState returns the wrapper as-is so callers must unwrap it; reference createElectronHostedAuthState (and parseElectronHostedAuthState) when making the change so the behavior is consistent and discoverable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/App.tsx`:
- Around line 1536-1540: The Electron callback check needs to run before any
hook registrations: refactor App into a thin wrapper that performs the
electronMcpCallbackUrl check and returns <OAuthDesktopReturnNotice />
immediately, and move the existing hook-heavy UI into a new AppShell component;
specifically, extract the current App body (the code that registers useEffect,
useElectronOAuth, useEnsureDbUser, useAppState and related handlers) into
AppShell, have App render either the OAuthDesktopReturnNotice when
electronMcpCallbackUrl is truthy or <AppShell /> otherwise, and update any
exports/usage to render App as before so hooks (useEffect, useElectronOAuth,
useEnsureDbUser, useAppState) are only registered when AppShell mounts.
In `@mcpjam-inspector/client/src/components/oauth/OAuthFlowProgressSimple.tsx`:
- Around line 115-170: The handler handleElectronOAuthCallback accepts any code
for flow=debug without verifying the OAuth state; update it to mirror the check
in processOAuthCallback by extracting parsed.searchParams.get("state") and
comparing it against the current expected state for the in-progress debug flow
(the same source used elsewhere — e.g. flowState/stateRef/currentFlowRef
depending on this component's state management). If the state is missing or does
not match, return early (do not process the code); otherwise proceed with the
existing processedElectronCodeRef.current deduplication and updateFlowState
logic. Also consider whether both components can be mounted simultaneously and
ensure this state-check prevents both from processing the same callback.
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 137-139: Remove the local re-declaration of
isElectronMcpCallbackState in use-server-state.ts and instead use the exported
helper from the owning module: import isElectronMcpCallbackState (and/or
ELECTRON_MCP_CALLBACK_STATE_PREFIX if needed) from "@/lib/oauth/mcp-oauth";
replace usages of the local function with the imported
isElectronMcpCallbackState so the canonical prefix
(ELECTRON_MCP_CALLBACK_STATE_PREFIX) is the single source of truth and the
literal "electron_mcp:" is not hard-coded here.
In `@mcpjam-inspector/client/src/lib/electron-hosted-auth.ts`:
- Around line 98-108: The redirect selection logic around isElectron,
isBrowserHttp, envRedirect and location.origin is correct but lacks explanation;
add a concise inline comment above the block (near the conditional that checks
isElectron and isBrowserHttp) clarifying the precedence: HTTP contexts
(isBrowserHttp true) always use `${location.origin}/callback` even in Electron
dev, while non-HTTP contexts fall back to envRedirect
(VITE_WORKOS_REDIRECT_URI), so that envRedirect has no effect in common
Electron/local or browser/https deployments—this comment should mention the
specific symbols isElectron, isBrowserHttp, envRedirect and location.origin so
future readers understand why the browser callback is chosen first.
In `@mcpjam-inspector/src/main.ts`:
- Around line 263-288: The current logic navigates an existing mainWindow
(mainWindow.loadURL) which discards renderer state and can drop the ipcRenderer
listener before webContents.send("oauth-callback", ...) is called; change the
flow so that if mainWindow already exists you prefer sending the IPC directly
(mainWindow.webContents.send("oauth-callback", url)) without calling
createMainWindow/loadURL, and only call createMainWindow/loadURL for the
cold-start case (no mainWindow). If you must navigate an existing window, defer
the ipc send until the new page is ready by using
mainWindow.webContents.once("did-finish-load", () =>
mainWindow.webContents.send("oauth-callback", url)). Update the branches
handling rendererCallbackUrl, callbackFlow and hadMainWindow accordingly
(symbols: mainWindow, rendererCallbackUrl, createMainWindow, loadURL,
webContents.send, did-finish-load, callbackFlow, hadMainWindow).
---
Outside diff comments:
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 592-618: In redirectToAuthorization detect the case where
window.isElectron is true but window.electronAPI?.app?.openExternal is missing
and emit a visible warning before falling back to in-app navigation; update
redirectToAuthorization to log a console.warn (including serverName and the
authorizationUrl) when window.isElectron &&
!window.electronAPI?.app?.openExternal so operators can see the
preload/misconfiguration, then proceed to call this.navigateToUrl(url) as the
existing fallback; keep existing localStorage usage (mcp-oauth-pending,
mcp-oauth-return-hash) unchanged.
---
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/oauth/__tests__/OAuthDebugCallback.test.tsx`:
- Around line 4-21: Add negative-case tests for buildElectronDebugCallbackUrl:
one that sets window.isElectron = true (simulating Electron) and another that
sets the URL to a non-debug callback path (e.g., not containing
/oauth/callback/debug or missing code/state) and assert that
buildElectronDebugCallbackUrl() returns null in both cases; use the same test
harness setup (vi.clearAllMocks(), window.history.replaceState(), window.name as
needed) and reference buildElectronDebugCallbackUrl to locate where to add these
assertions so future refactors don't break the early-return behavior.
In
`@mcpjam-inspector/client/src/components/oauth/__tests__/OAuthDesktopReturnNotice.test.tsx`:
- Around line 48-63: The test currently verifies desktopReturnRuntime.redirect
is only called once under StrictMode but doesn't reset the dedupe state; update
the test to also call or assert usage of resetDesktopReturnAttemptsForTests so
subsequent tests aren't blocked—for example, after rendering
OAuthDesktopReturnNotice (or in an afterEach) invoke
resetDesktopReturnAttemptsForTests (or expect it to have been called) to clear
the dedupe state created by the component's redirect logic; refer to
desktopReturnRuntime.redirect, OAuthDesktopReturnNotice, and
resetDesktopReturnAttemptsForTests to locate and update the test.
In `@mcpjam-inspector/client/src/components/OAuthFlowTab.tsx`:
- Around line 436-470: handleElectronOAuthCallback currently forwards the raw
deep-link state to processOAuthCallback and only logs URL parse errors to
console; change it to validate the incoming state against your stored expected
state (e.g., compare state to oauthStateRef.current or similar) before calling
processOAuthCallback and treat mismatches by clearing exchangeTimeoutRef (if
set) and calling updateOAuthFlowState({ error: "State mismatch" }) (or silently
drop, pick the intended UX), so the user sees a clear message instead of an
internal "Invalid state parameter"; also in the catch block replace the
console.error with updateOAuthFlowState({ error: `Failed to process callback:
${String(error)}` }) and clear exchangeTimeoutRef there as well so the UI
surfaces malformed-URL failures.
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 872-881: The dependency array for the useCallback that includes
dispatch, failPendingOAuthConnection, isAuthenticated, logger, storeInitInfo,
testConnectionAfterOAuth, guardedTestConnection, withWorkspaceClientCapabilities
should be trimmed: remove guardedTestConnection from the dependency list because
the callback no longer references it (OAuth completion now delegates to
testConnectionAfterOAuth). Update the useCallback dependency array to include
the actual referenced symbols (keep testConnectionAfterOAuth, etc.) so React
won't see spurious identity churn from guardedTestConnection.
In `@mcpjam-inspector/client/src/lib/__tests__/electron-hosted-auth.test.ts`:
- Around line 11-62: Tests only cover the happy paths; add negative-case unit
tests to exercise alternate branches: for createElectronHostedAuthState add
tests for undefined input and primitive input to verify fallback tagging
behavior; for parseElectronHostedAuthState add tests for null/empty input,
malformed JSON, and JSON missing ELECTRON_HOSTED_AUTH_STATE_KEY to ensure it
returns null or original value per implementation; for
isElectronHostedAuthCallback add tests for wrong pathname, missing code and
missing error params, and unparsable state to assert it returns false; and for
resolveWorkosRedirectUri add tests for isElectron=false, https origin, missing
envRedirect, and a custom protocol envRedirect to validate each branch.
Reference helpers by name (createElectronHostedAuthState,
parseElectronHostedAuthState, isElectronHostedAuthCallback,
buildElectronHostedAuthCallbackUrl, resolveWorkosRedirectUri,
ELECTRON_HOSTED_AUTH_STATE_KEY) when adding these assertions.
In `@mcpjam-inspector/client/src/lib/electron-hosted-auth.ts`:
- Around line 13-33: The helper createElectronHostedAuthState currently wraps
non-object states under originalState which creates an undocumented asymmetry
with parseElectronHostedAuthState; change the function signature to accept
state?: Record<string, unknown> (i.e. narrow the parameter type to
Record<string, unknown> | undefined) and remove the branch that wraps
primitives/arrays in originalState so it only accepts and forwards object-shaped
state, or alternatively add a concise JSDoc above createElectronHostedAuthState
explaining the deliberate originalState wrapper and that
parseElectronHostedAuthState returns the wrapper as-is so callers must unwrap
it; reference createElectronHostedAuthState (and parseElectronHostedAuthState)
when making the change so the behavior is consistent and discoverable.
In `@mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts`:
- Around line 327-385: Update the fallback test for
MCPOAuthProvider.redirectToAuthorization to assert the specific console error
shape and ordering: replace the loose expect(consoleErrorSpy).toHaveBeenCalled()
with
expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Failed to
open system browser"), expect.any(Error)) to ensure the logged message and error
are correct, and add an ordering assertion using the spies' invocationCallOrder
(e.g.,
expect(openExternal.mock.invocationCallOrder[0]).toBeLessThan(navigateSpy.mock.invocationCallOrder[0]))
to guarantee openExternal is awaited before navigateToUrl is invoked.
In `@mcpjam-inspector/server/__tests__/app-listeners.test.ts`:
- Around line 71-108: Add tests exercising the URL validation and
protocol-rejection branches of the open-external IPC handler: use
getOpenExternalHandler() to obtain the handler and call it with trusted sender
objects (use currentMainWindow = createWindow(...) to set trusted id) but supply
invalid URLs (e.g., malformed URI) and disallowed protocols (e.g., "file://",
"ftp://") and assert the handler rejects/throws with the same error message used
for protocol/URL checks, that openExternalMock is not called, and that
logWarnMock (or the specific logger used) is invoked with the corresponding
warning; reference the existing test helpers openExternalMock, logWarnMock,
currentMainWindow, createWindow and the handler returned by
getOpenExternalHandler to locate where to insert these cases.
In `@mcpjam-inspector/src/ipc/listeners-register.ts`:
- Around line 7-15: The default factory getMainWindow closes over the initial
mainWindow and defeats the safe-late-binding purpose of getMainWindow; update
registerListeners to require getMainWindow instead of defaulting (i.e., remove
the default parameter) so callers (e.g., the app:open-external handler) must
pass a function that reads the current mainWindow, or if you must keep a
default, replace the default with an explicit comment next to getMainWindow
explaining it only remains safe while the mainWindow variable is never
reassigned; adjust call sites accordingly (notably where registerListeners is
invoked) to pass a live accessor.
In `@mcpjam-inspector/src/main.ts`:
- Around line 42-43: pendingProtocolUrl is a single-slot buffer that can drop
multiple early open-url events; replace it with a queue (e.g.,
pendingProtocolUrls: string[]), update the open-url event handler to push each
incoming URL onto that queue and, if appBootstrapped is true, process it
immediately, and change the whenReady()/bootstrap completion path (and the code
referenced around pending handling, including the logic used near the later
block at the other occurrence) to drain the queue by iterating over
pendingProtocolUrls and handling each URL in order, then clear the queue; ensure
you update any checks that referenced pendingProtocolUrl to handle an empty vs
non-empty array.
🪄 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: d205e64b-03cc-453d-8e7d-9e8cb7797924
📒 Files selected for processing (26)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/OAuthFlowTab.tsxmcpjam-inspector/client/src/components/oauth/OAuthAuthorizationModal.tsxmcpjam-inspector/client/src/components/oauth/OAuthDebugCallback.tsxmcpjam-inspector/client/src/components/oauth/OAuthDesktopReturnNotice.tsxmcpjam-inspector/client/src/components/oauth/OAuthFlowProgressSimple.tsxmcpjam-inspector/client/src/components/oauth/__tests__/OAuthAuthorizationModal.test.tsxmcpjam-inspector/client/src/components/oauth/__tests__/OAuthDebugCallback.test.tsxmcpjam-inspector/client/src/components/oauth/__tests__/OAuthDesktopReturnNotice.test.tsxmcpjam-inspector/client/src/components/sidebar/__tests__/sidebar-user-organizations.test.tsxmcpjam-inspector/client/src/components/sidebar/sidebar-user.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/useElectronOAuth.tsmcpjam-inspector/client/src/lib/__tests__/electron-hosted-auth.test.tsmcpjam-inspector/client/src/lib/electron-hosted-auth.tsmcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/main.tsxmcpjam-inspector/client/src/types/electron.d.tsmcpjam-inspector/server/__tests__/app-listeners.test.tsmcpjam-inspector/src/ipc/app/app-listeners.tsmcpjam-inspector/src/ipc/listeners-register.tsmcpjam-inspector/src/main.tsmcpjam-inspector/src/preload.tsmcpjam-inspector/vite.renderer.config.mts
| if (electronMcpCallbackUrl) { | ||
| return ( | ||
| <OAuthDesktopReturnNotice returnToElectronUrl={electronMcpCallbackUrl} /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
path = Path("mcpjam-inspector/client/src/App.tsx")
text = path.read_text().splitlines()
needles = [
"useEffect(() => {",
"useElectronOAuth();",
"useEnsureDbUser();",
"useAppState({",
"if (electronMcpCallbackUrl)",
]
for needle in needles:
for index, line in enumerate(text, start=1):
if needle in line:
print(f"{needle!r} first appears at line {index}")
break
PYRepository: MCPJam/inspector
Length of output: 299
🏁 Script executed:
#!/bin/bash
# Get the relevant section of App.tsx to understand function structure
head -n 1545 mcpjam-inspector/client/src/App.tsx | tail -n 1160 | cat -nRepository: MCPJam/inspector
Length of output: 43783
🏁 Script executed:
#!/bin/bash
# Find the function signature that contains these hooks
grep -n "^export default function App" mcpjam-inspector/client/src/App.tsx | head -5
grep -n "^function App" mcpjam-inspector/client/src/App.tsx | head -5Repository: MCPJam/inspector
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Confirm the App function boundaries
sed -n '291,310p' mcpjam-inspector/client/src/App.tsx
# Also find where the function closes
tail -50 mcpjam-inspector/client/src/App.tsx | head -30Repository: MCPJam/inspector
Length of output: 2179
🏁 Script executed:
#!/bin/bash
# Get the exact line count of the file to find the function end
wc -l mcpjam-inspector/client/src/App.tsx
# Show the very end of the file
tail -20 mcpjam-inspector/client/src/App.tsxRepository: MCPJam/inspector
Length of output: 703
🏁 Script executed:
#!/bin/bash
# Verify the actual line numbers for the conditional return
sed -n '1145,1160p' mcpjam-inspector/client/src/App.tsx | cat -nRepository: MCPJam/inspector
Length of output: 612
🏁 Script executed:
#!/bin/bash
# Get clearer view of lines 1147-1160 to see the early returns
sed -n '1147,1165p' mcpjam-inspector/client/src/App.tsx | cat -nRepository: MCPJam/inspector
Length of output: 728
🏁 Script executed:
#!/bin/bash
# Look at lines around 1536 where the early returns are
sed -n '1530,1560p' mcpjam-inspector/client/src/App.tsx | cat -nRepository: MCPJam/inspector
Length of output: 1173
Move the Electron handoff above the hook-heavy app shell.
The Electron callback check returns early at line 1536, but all hooks above it—including useEffect (line 394), useElectronOAuth() (line 539), useEnsureDbUser() (line 541), and useAppState (line 657)—are registered and executed first. Their effects still run on the /oauth/callback route before the desktop handoff. Extract App into a thin wrapper plus an AppShell so the relay page returns before effects, app state, hosted callback handlers, analytics, and sync effects are registered.
♻️ Suggested structure
-export default function App() {
+function AppShell() {
const [activeTab, setActiveTab] = useState("servers");
...
- const electronMcpCallbackUrl = buildElectronMcpCallbackUrl();
...
- if (electronMcpCallbackUrl) {
- return (
- <OAuthDesktopReturnNotice returnToElectronUrl={electronMcpCallbackUrl} />
- );
- }
-
if (hostedOAuthHandling) {
return <LoadingScreen />;
}
...
}
+
+export default function App() {
+ const electronMcpCallbackUrl = buildElectronMcpCallbackUrl();
+
+ if (electronMcpCallbackUrl) {
+ return (
+ <OAuthDesktopReturnNotice returnToElectronUrl={electronMcpCallbackUrl} />
+ );
+ }
+
+ return <AppShell />;
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/App.tsx` around lines 1536 - 1540, The Electron
callback check needs to run before any hook registrations: refactor App into a
thin wrapper that performs the electronMcpCallbackUrl check and returns
<OAuthDesktopReturnNotice /> immediately, and move the existing hook-heavy UI
into a new AppShell component; specifically, extract the current App body (the
code that registers useEffect, useElectronOAuth, useEnsureDbUser, useAppState
and related handlers) into AppShell, have App render either the
OAuthDesktopReturnNotice when electronMcpCallbackUrl is truthy or <AppShell />
otherwise, and update any exports/usage to render App as before so hooks
(useEffect, useElectronOAuth, useEnsureDbUser, useAppState) are only registered
when AppShell mounts.
| useEffect(() => { | ||
| const handleElectronOAuthCallback = (event: Event) => { | ||
| const callbackUrl = (event as CustomEvent<string>).detail; | ||
| if (!callbackUrl) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const parsed = new URL(callbackUrl); | ||
| if (parsed.searchParams.get("flow") !== "debug") { | ||
| return; | ||
| } | ||
|
|
||
| const error = parsed.searchParams.get("error"); | ||
| const errorDescription = parsed.searchParams.get("error_description"); | ||
| if (error) { | ||
| updateFlowState({ | ||
| latestError: new Error(errorDescription ?? error), | ||
| validationError: null, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const code = parsed.searchParams.get("code"); | ||
| if (!code || processedElectronCodeRef.current === code) { | ||
| return; | ||
| } | ||
|
|
||
| processedElectronCodeRef.current = code; | ||
| updateFlowState({ | ||
| authorizationCode: code, | ||
| latestError: null, | ||
| validationError: null, | ||
| statusMessage: { | ||
| type: "success", | ||
| message: | ||
| "Authorization received from your browser. Click Continue to exchange the code.", | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to process Electron OAuth callback:", error); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener( | ||
| "electron-oauth-callback", | ||
| handleElectronOAuthCallback as EventListener, | ||
| ); | ||
|
|
||
| return () => { | ||
| window.removeEventListener( | ||
| "electron-oauth-callback", | ||
| handleElectronOAuthCallback as EventListener, | ||
| ); | ||
| }; | ||
| }, [updateFlowState]); |
There was a problem hiding this comment.
Missing state validation opens a small CSRF seam in the debug flow.
Unlike OAuthFlowTab.tsx's processOAuthCallback, this handler accepts any code present on an electron-oauth-callback event with flow=debug without cross-checking the state parameter against the one generated for the in-progress flow. In practice the debug tab is a developer tool, so the blast radius is modest, but if both OAuthFlowTab and OAuthFlowProgressSimple are ever mounted for the same session, the first to process an incoming code wins — and here it wins unconditionally. Worth mirroring the state equality check used by the sibling listener.
Additionally, since useElectronOAuth dispatches electron-oauth-callback unconditionally and both components filter on flow=debug, consider whether both listeners can be active simultaneously; if so, the user will see two independent state updates for a single callback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/oauth/OAuthFlowProgressSimple.tsx`
around lines 115 - 170, The handler handleElectronOAuthCallback accepts any code
for flow=debug without verifying the OAuth state; update it to mirror the check
in processOAuthCallback by extracting parsed.searchParams.get("state") and
comparing it against the current expected state for the in-progress debug flow
(the same source used elsewhere — e.g. flowState/stateRef/currentFlowRef
depending on this component's state management). If the state is missing or does
not match, return early (do not process the code); otherwise proceed with the
existing processedElectronCodeRef.current deduplication and updateFlowState
logic. Also consider whether both components can be mounted simultaneously and
ensure this state-check prevents both from processing the same callback.
| if (isElectron) { | ||
| return isBrowserHttp | ||
| ? `${location.origin}/callback` | ||
| : envRedirect ?? "mcpjam://oauth/callback"; | ||
| } | ||
|
|
||
| if (isBrowserHttp) { | ||
| return `${location.origin}/callback`; | ||
| } | ||
|
|
||
| return envRedirect ?? `${location.origin}/callback`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -120 mcpjam-inspector/client/src/lib/electron-hosted-auth.tsRepository: MCPJam/inspector
Length of output: 2570
🏁 Script executed:
# Check if there are tests for this function
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | xargs grep -l "resolveWorkosRedirectUri" 2>/dev/null || echo "No test files found with this function"Repository: MCPJam/inspector
Length of output: 132
🏁 Script executed:
# Search for envRedirect usage in the codebase
rg "VITE_WORKOS_REDIRECT_URI|envRedirect" mcpjam-inspector/client --type ts --type tsx -B 2 -A 2Repository: MCPJam/inspector
Length of output: 87
🏁 Script executed:
# Correct search for envRedirect usage
rg "VITE_WORKOS_REDIRECT_URI|envRedirect" mcpjam-inspector/client -B 2 -A 2Repository: MCPJam/inspector
Length of output: 2780
🏁 Script executed:
# Examine the test file to understand expected behavior
cat mcpjam-inspector/client/src/lib/__tests__/electron-hosted-auth.test.tsRepository: MCPJam/inspector
Length of output: 1947
Add a clarifying comment on the redirect precedence.
The behavior is intentional: HTTP-based environments (both Electron dev and non-Electron) always use ${location.origin}/callback, while non-HTTP environments fall back to envRedirect. However, this precedence isn't obvious from the code. Since VITE_WORKOS_REDIRECT_URI has no effect in the most common deployment scenarios (Electron on localhost, or browser on https), a brief inline comment explaining why the browser callback takes precedence would spare future maintainers the deduction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/electron-hosted-auth.ts` around lines 98 -
108, The redirect selection logic around isElectron, isBrowserHttp, envRedirect
and location.origin is correct but lacks explanation; add a concise inline
comment above the block (near the conditional that checks isElectron and
isBrowserHttp) clarifying the precedence: HTTP contexts (isBrowserHttp true)
always use `${location.origin}/callback` even in Electron dev, while non-HTTP
contexts fall back to envRedirect (VITE_WORKOS_REDIRECT_URI), so that
envRedirect has no effect in common Electron/local or browser/https
deployments—this comment should mention the specific symbols isElectron,
isBrowserHttp, envRedirect and location.origin so future readers understand why
the browser callback is chosen first.
| if (!mainWindow) { | ||
| if (rendererCallbackUrl) { | ||
| mainWindow = createMainWindow(baseUrl); | ||
| mainWindow.loadURL(rendererCallbackUrl.toString()); | ||
| } else { | ||
| const debugCallbackUrl = new URL("/oauth/callback/debug", baseUrl); | ||
| for (const [key, value] of parsed.searchParams.entries()) { | ||
| if (key === "flow") continue; | ||
| debugCallbackUrl.searchParams.append(key, value); | ||
| } | ||
| mainWindow = createMainWindow(baseUrl); | ||
| mainWindow.loadURL(debugCallbackUrl.toString()); | ||
| } | ||
| } else if (rendererCallbackUrl) { | ||
| mainWindow.loadURL(rendererCallbackUrl.toString()); | ||
| } | ||
|
|
||
| if (mainWindow?.webContents && callbackFlow === "debug" && hadMainWindow) { | ||
| mainWindow.webContents.send("oauth-callback", url); | ||
| } else if ( | ||
| mainWindow?.webContents && | ||
| callbackFlow !== "mcp" && | ||
| callbackFlow !== "debug" | ||
| ) { | ||
| mainWindow.webContents.send("oauth-callback", url); | ||
| } |
There was a problem hiding this comment.
Existing-window callbacks nuke renderer state and can race the IPC listener.
When mainWindow already exists and the flow is non-debug, mainWindow.loadURL(rendererCallbackUrl) performs a full navigation — any in-flight work in the renderer (open tabs, modals, unsaved form fields) is discarded. For the non-mcp/non-debug branch it is doubly unfortunate: Line 277 navigates away, then Line 287 sends oauth-callback via webContents.send, but the ipcRenderer.on("oauth-callback", …) listener registered by the previous page (per src/preload.ts) is gone, and the new page's preload has not yet reattached. The message can arrive in a window where no one is listening.
Consider preferring IPC when the window already exists and reserving loadURL for the cold-start (no-window) case, or at minimum dispatch the IPC after the new page signals ready (webContents.once("did-finish-load", …)) so the two channels don't trip over each other.
🔧 Sketch
- if (!mainWindow) {
- if (rendererCallbackUrl) {
- mainWindow = createMainWindow(baseUrl);
- mainWindow.loadURL(rendererCallbackUrl.toString());
- } else {
- const debugCallbackUrl = new URL("/oauth/callback/debug", baseUrl);
- for (const [key, value] of parsed.searchParams.entries()) {
- if (key === "flow") continue;
- debugCallbackUrl.searchParams.append(key, value);
- }
- mainWindow = createMainWindow(baseUrl);
- mainWindow.loadURL(debugCallbackUrl.toString());
- }
- } else if (rendererCallbackUrl) {
- mainWindow.loadURL(rendererCallbackUrl.toString());
- }
-
- if (mainWindow?.webContents && callbackFlow === "debug" && hadMainWindow) {
- mainWindow.webContents.send("oauth-callback", url);
- } else if (
- mainWindow?.webContents &&
- callbackFlow !== "mcp" &&
- callbackFlow !== "debug"
- ) {
- mainWindow.webContents.send("oauth-callback", url);
- }
+ if (!mainWindow) {
+ const initialUrl =
+ rendererCallbackUrl?.toString() ??
+ (() => {
+ const debugCallbackUrl = new URL("/oauth/callback/debug", baseUrl);
+ for (const [key, value] of parsed.searchParams.entries()) {
+ if (key === "flow") continue;
+ debugCallbackUrl.searchParams.append(key, value);
+ }
+ return debugCallbackUrl.toString();
+ })();
+ mainWindow = createMainWindow(baseUrl);
+ mainWindow.loadURL(initialUrl);
+ } else if (callbackFlow !== "mcp") {
+ // Existing window: prefer IPC so the renderer preserves its state.
+ mainWindow.webContents.send("oauth-callback", url);
+ } else if (rendererCallbackUrl) {
+ mainWindow.loadURL(rendererCallbackUrl.toString());
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/src/main.ts` around lines 263 - 288, The current logic
navigates an existing mainWindow (mainWindow.loadURL) which discards renderer
state and can drop the ipcRenderer listener before
webContents.send("oauth-callback", ...) is called; change the flow so that if
mainWindow already exists you prefer sending the IPC directly
(mainWindow.webContents.send("oauth-callback", url)) without calling
createMainWindow/loadURL, and only call createMainWindow/loadURL for the
cold-start case (no mainWindow). If you must navigate an existing window, defer
the ipc send until the new page is ready by using
mainWindow.webContents.once("did-finish-load", () =>
mainWindow.webContents.send("oauth-callback", url)). Update the branches
handling rendererCallbackUrl, callbackFlow and hadMainWindow accordingly
(symbols: mainWindow, rendererCallbackUrl, createMainWindow, loadURL,
webContents.send, did-finish-load, callbackFlow, hadMainWindow).
2 participants
This PR makes Electron auth flows use the user’s real browser and return cleanly back into MCPJam Desktop.
What changed:
Electron account sign-in/sign-up now go through the system browser instead of trying to finish inside the app.
Electron server/integration OAuth also uses the system browser.
The OAuth debugger in Electron follows the same browser-based flow.
Electron-launched auth is tagged so the browser callback knows it should hand control back to the desktop app.
When a browser callback came from Electron, MCPJam now shows a simple “Continue in MCPJam Desktop / close this page” handoff screen instead of leaving the user stuck on WorkOS, a stale callback page, or a confusing browser state.
Callback handling was tightened for both “app already open” and “app opens from callback” cases.
Electron logout was fixed so it returns to the app root instead of the callback page, which avoids the blank/white screen after signing out.
Added focused test coverage around browser launch, callback handoff, and logout behavior.
What this does not change:
Normal web login behavior is unchanged.
This PR is specifically about Electron auth behavior.