fix for eng-1102

[martyna-mindsdb]

Linear ticket: https://linear.app/mindsdb/issue/ENG-1102/google-drive-file-picker-fails-with-403-when-chromes-active-account

---

What was fixed:

• src/main/index.ts:
  • forward the already-available accountEmail into openDrivePickerFlow (previously dropped).
• src/main/drive-picker-service.ts
  • thread accountEmail through to the picker page;
  • show which account is being used before the picker opens;
  • handle google.picker.Action.ERROR to surface a clear in-app message (account-mismatch hint + Reload button) instead of leaving the user on Google's raw 403;
  • HTML-escape the email now that it's rendered into markup.
• src/main/drive-picker-service.test.ts
  • updated call sites for the new signature;
  • added tests for the error-path result and HTML-escaping.

Note: this PR does not add a login_hint/authuser param because no real hook for it exists in this flow (token is minted server-side, no client-side OAuth call to attach a hint to). This PR covers detection + fallback messaging only.

[alecantu7]

Reviewed against ENG-1102. Ran npm test (12 pass), typecheck:main + typecheck:test (clean), and exercised the new error path against the real loopback server on this branch. The escaping work and the plumbing are solid, but I think two things need to change before this merges, and I've left the detail inline.

1. The detection signal probably never fires for this bug. I pulled the screenshot off the ticket: the tab is still our page (127.0.0.1:60067, title "Pick Google Drive files") and Google's 403 is rendered inside the picker's iframe, on the picker's own modal backdrop. So docs.google.com/picker served a static 403 error page instead of the widget. PICKED/CANCEL/ERROR all come back over gapi's postMessage/RPC relay, spoken by picker code running inside that iframe — a 403 error page has none, so it can't hand back any action. Action.ERROR is documented as "the Google Picker dialog has encountered an error" (widget loaded, then failed), not a load-failure signal. If that's right, the user in the ticket sees exactly what they see today — raw 403, our new card copy hidden under the backdrop, and the flow sitting until the 5-minute PICKER_TIMEOUT_MS (with no cancel affordance at all from the composer entry point). I couldn't stage a wrong-active-account Chrome to prove it, so: could you repro the 403 with two accounts signed into Chrome and confirm the ERROR branch actually fires? If it doesn't, ticket success criterion #2 isn't met and the detector needs to be a load timeout instead (details inline).

2. The "Reload and try again" button can't work. Verified empirically, not theorised — output inline. It returns our own 403 if clicked fast, and ERR_CONNECTION_REFUSED after ~300ms. So the one actionable affordance we're adding replaces Google's error page with a different error page.

On skipping login_hint/authuser — agree with the call, but let's write it back on the ticket. (Small correction to the PR body: the token is minted in the Electron main process, not server-side.) PickerBuilder exposes no account hint and the iframe URL isn't ours to parameterise; Google's own community guidance on this 403 class is that there's no reliable code workaround and the advice is to warn the user. Worth a comment on ENG-1102 so the "How to fix" section doesn't keep implying the hint was skipped for convenience — the ticket's own fallback clause is what's being implemented. Two things worth a timebox if the timeout detector isn't enough: (a) unofficial but widely used — after setVisible(true), append &authuser=<email> to the injected picker iframe's src; (b) long-term, browse Drive in-app via the API instead of the widget, which removes the browser-session dependency entirely — but that needs a broader read scope than drive.file, so it's a consent/verification change, not a swap.

Verified no regression risk elsewhere: index.ts:741 is the only call site, and accountEmail is validated non-empty at index.ts:737 and is the connected Google account (not the MindsHub login), so the new required positional param can't render undefined. Nits: title should be ENG-1102: <imperative summary> per repo convention, and the PR body doesn't state the security pass explicitly (the escaping work clearly was one).

[alecantu7]

This is the crux of the fix, and I don't think it fires for the failure mode in the ticket.

The screenshot on ENG-1102 shows Google's 403 rendered inside the picker's iframe (docs.google.com/picker), with our page still the top-level document. Every action — PICKED, CANCEL, ERROR — is delivered over gapi's postMessage/RPC relay by picker code running inside that iframe. A static 403 error page contains no picker code, so it can't complete the handshake or report anything. Action.ERROR is documented as "the Google Picker dialog has encountered an error", i.e. the widget rendered and then hit a problem — not "the widget failed to load".

Same reasoning as why CANCEL can't arrive from that state: the Cancel button lives inside the iframe too.

So I'd expect post-merge behaviour to be unchanged for the reported user: raw 403, our card hidden under the picker's backdrop, then a 5-minute wait to PICKER_TIMEOUT_MS (and from useGoogleDrivePicker.js:109 there's no cancel affordance, so that's a real hang).

I couldn't reproduce a wrong-active-account Chrome session to prove this, so please confirm either way with a live repro.

What would work: a load timeout. After builder.build().setVisible(true), start a ~8-10s timer; if no callback has arrived, picker.setVisible(false) (which uncovers this page's card) and show the mismatch guidance. That catches the 403 and any other silent widget-load failure, degrades safely (a slow-but-successful load just clears the timer), and unlike the ERROR action it's directly testable. Keep this branch too — it's harmless and does cover genuine in-widget errors.

[alecantu7]

This button can never succeed. The page is deliberately single-serve (stateConsumed, line 99 — see the token-leak comment at 84-88), and by the time the user can click, reportResult has already rejected the flow, so the finally block has torn the server down 300ms later.

Ran it against the real loopback server on this branch:

[1] initial page load:    200  <!doctype html>…
[2] flow result:          {"ok":false,"reason":"Google Picker could not open — mismatch."}
[3] reload immediately:   403  "Invalid state."
[4] reload after 600ms:   ECONNREFUSED

So we either replace Google's 403 with our own 403, or hand the user a browser connection-error page. Making reload work would mean re-serving a token-bearing page for an already-consumed state, which is exactly what stateConsumed exists to prevent.

Suggest: drop the button, change the copy to "close this tab and try again from Cowork", and add focusMainWindow() on the failure path in index.ts (today index.ts:742 returns before the focus call) so the in-app message we're pointing them at is actually in front of them.

[alecantu7]

Escaping is asymmetric here: setStatus above correctly gets escapeHtml(ACCOUNT_EMAIL) (it writes innerHTML), but this path sends the raw value back as reason, and both consumers render it (CustomizeView.jsx:321 → rendered at :529, useGoogleDrivePicker.js:110). Low risk in practice — it's the user's own connected account email and both render as React text — but worth being consistent given the rest of the PR is careful about exactly this.

[alecantu7]

Nit: unscoped button {} on a page the Picker injects its own parent-side DOM into (.picker-dialog and friends live in this document). Scope it to #reload-btn / .err button.

[alecantu7]

Nit: this only helps for the ~1s before the picker's modal covers the card — and in the failure case it stays covered. builder.setTitle('Choose files from ' + accountEmail) puts the account in the picker's own title bar, where it's visible during normal use. Cheap to do both.

[alecantu7]

This is a good unit test of the server-side plumbing, but it isn't a regression test for ENG-1102 — it feeds in a payload the browser will never actually send in the failing scenario (see my note on the Action.ERROR branch). Per the repo norm that a bug fix ships a regression test for the bug: at minimum assert the served page contains the error handling, and if the detector moves to a load timeout that behaviour becomes directly testable in the page-level logic.

[alecantu7]

Thanks for turning these around so fast — every point from the first pass is addressed, and two of the fixes are better than what I suggested. Re-reviewed at 0c6da99b: drive-picker-service.test.ts → 13 passed.

What's now right:

• Reload button gone, copy changed to "close this tab, and try again from Cowork" — no more dead affordance, and the unscoped button {} rule went with it.
• focusMainWindow() moved ahead of the !pickResult.ok return, so the guidance actually surfaces in front of the user.
• setTitle('Choose files from ' + ACCOUNT_EMAIL) — the account is now visible where the user will actually see it.
• Better than my suggestion: keeping the raw email in the reason (rather than escaping it) with a comment explaining that the renderer displays it as plain React text, so escaping would render literal entities. That's the correct call and I was wrong to ask for symmetry there.
• The old test's comment now honestly says it doesn't prove either detector fires. Good.

But the load timeout as written breaks the working path, and that's on me — my suggestion was wrong in a way I should have caught. I wrote "degrades safely (a slow-but-successful load just clears the timer)". Nothing clears the timer on a successful load: settled only flips inside the setCallback handler, on PICKED / CANCEL / ERROR. A picker that loads perfectly and sits there while the user browses is indistinguishable, to this timer, from one that never loaded. And the picker.setVisible(false) I suggested is what turns that into a destructive failure rather than a cosmetic one.

I ran the served page's own inline script in a stubbed browser (real pickerPage() output, stubbed gapi/google.picker/fetch/setTimeout), simulating the ordinary case: picker opens fine, user hasn't chosen a file yet, 9s elapses.

extracted script chars: 6404
picker opened. timeout armed at 9000 ms
user has NOT picked or cancelled yet — nothing settled.
setVisible calls -> [true,false]          <-- picker force-closed
posted to /result -> ["Google Picker could not open — the browser’s active Google account
                      may not match user@example.com."]
card shown -> Could not open Google Drive  This is usually caused by user@example.com not
              being the active Google account in this browser. Switch to that a…

So nine seconds after the picker opens, any user still browsing their Drive gets the dialog yanked out from under them and is told their Google account is wrong. Nine seconds is well inside normal file-picking — scrolling, switching to Shared drives, searching. And if they do pick at, say, 12s, the flow has already been rejected and the server torn down 300ms later, so their selection is silently dropped.

That trades a bug affecting multi-account users for one affecting everyone, so I'd hold the merge on it. Options inline; the short version is that the timeout needs either a real load signal or to stop being destructive.

Worth noting the new test can't catch this: expect(body).toContain('PICKER_LOAD_TIMEOUT_MS') asserts the source text is present, not what it does. That's literally what I asked for ("at minimum assert the served page contains the error handling"), so it's a fair implementation of a request I under-specified — but it's why 13 green tests and a real regression coexist here.

[alecantu7]

This timer measures "time until the user finishes with the picker", not "time until the widget loads" — settled is only set from inside setCallback, and a successful load emits no callback. Probe output from running this exact page script with a picker that loaded fine and a user who hasn't picked yet:

picker opened. timeout armed at 9000 ms
setVisible calls -> [true,false]
posted to /result -> ["Google Picker could not open — … may not match user@example.com."]

Three consequences, in severity order:

1. Every user who takes >9s to choose a file loses the picker and gets a wrong diagnosis.
2. A pick that lands after the timeout is silently dropped — the flow already rejected and finally closed the server 300ms later.
3. The 403 case it was written for is only sometimes caught, since a genuinely slow load and a failed load look identical.

As far as I can tell there's no clean client-side load signal available: the picker's iframe is cross-origin, onload fires for Google's 403 page exactly as it does for the widget, and the enum has no LOADED action. Two ways out that don't need one:

a) Make it non-destructive (smallest change). Drop the picker.setVisible(false) and the reportResult({error}) from the timeout. Instead let the timeout only stage the guidance — e.g. set a flag that upgrades the message the existing PICKER_TIMEOUT_MS path already produces, so a user who was merely slow keeps their picker and finishes normally, while a user who is genuinely stuck gets "…the browser's active Google account may not match …" instead of "Picker timed out" when the flow ends. Strictly better than today for ENG-1102's criterion #2, and it can't regress the happy path.

b) Keep it active but require a stronger signal. Only fire if the picker is still unsettled and the tab never became interactive with the widget — e.g. arm the timer, then cancel it on the first visibilitychange/focus/pointer event inside the dialog region. Cheaper heuristics exist but they all have false positives; (a) has none.

I'd take (a). If you'd rather keep an active detector, please raise the window a lot (60s+) and drop the teardown, so the worst case is a stale message rather than a cancelled pick.

Either way the account-mismatch copy is good and worth keeping — it's the trigger that needs to change, not the message.

[alecantu7]

Naming nit that would have made the bug above visible at the call site: settled reads as "the picker is done", but what it actually tracks is "the user reached a terminal action". There's no state for "the widget rendered", which is the thing the timeout wants to know about.

If you keep any form of timer, consider two separate flags (userActed vs loadConfirmed) — the absence of a way to ever set the second one is exactly the gap here, and making it explicit documents why the detector has to be non-destructive.

[alecantu7]

Regression is fixed — verified, not just read. I re-ran the same probe that caught it: the served page's own script against a stubbed gapi/google.picker, picker loaded fine, user hasn't picked, 9s elapses.

setVisible calls -> [true]                                     ← round 1 gave [true,false]
posted to /result -> [{state:…, signal:"suspected-account-mismatch"}]
card text -> ""                                                ← page doesn't even repaint

The picker stays open, nothing terminal is posted, and the flow stays alive — so a user who simply takes longer than 9s keeps their in-progress selection. drive-picker-service.test.ts → 17 passed (was 13), main project → 316, typecheck clean.

What I like about how you took it, beyond the fix itself:

• The state check still guards the signal, so a local process can't set the flag without the state token.
• buildPickerFailureReason is pure and exported, and the tests pin the part that's easy to get wrong — that a cancellation and a genuine Action.ERROR keep their own specific reason and only the generic timeout gets augmented.
• settled → userActed with a comment saying it tracks the user reaching a terminal action and not whether the widget loaded. That rename is the thing that makes the bug hard to reintroduce, more than the behaviour change is.
• The new test asserts not.toContain('setVisible(false)'), so the destructive version can't come back silently.

One consequence worth stating out loud, and then deliberately not fixing: the user this ticket is about now waits the full PICKER_TIMEOUT_MS (5 minutes) staring at Google's 403 before the guidance reaches them. Round 1 told them at 9s — wrong for everyone else, right for them.

I want to be explicit that I am not asking you to shorten it. Reducing the remaining wait when the signal arrives (say 5min → 45s) would reintroduce exactly the bug we just removed, one notch further out: a user who takes 9s to start browsing and 60s to find their file would lose the picker again. The current design is right because it refuses to act on a guess, and any faster feedback has to be non-destructive too — a hint in the app while the flow stays open, rather than a shorter cutoff. That's a follow-up if we ever decide the 5-minute wait is worth engineering around, not a change to this PR.

Nothing further from me — this addresses both blocking findings from the first round, and the earlier fixes (reload button gone, focusMainWindow on the failure path, setTitle carrying the account) are all still in place. I'll approve once someone's had a chance to sanity-check the picker on a real multi-account Chrome — the flows are all mocked, so nothing here proves the 403 case behaves as expected end to end.

[alecantu7]

Approving on the code — walking back the "once someone's checked it on a real multi-account Chrome" line from my previous comment, since on reflection that's a QA step and not a merge gate.

Still worth doing the multi-account check when someone has two Google accounts handy, as ENG-1102 verification rather than a blocker: sign in with the connected account not being Chrome's active one, open the picker, and confirm the in-app message eventually shows the account-mismatch guidance instead of a bare "Picker timed out".

Note it's 3 commits behind staging — worth a rebase before merge so the diff stays clean.