Skip to content

fix(desktop): restore visible Google sign-in popups in previews - #13348

Open
MatthewFeroz wants to merge 2 commits into
pingdotgg:mainfrom
MatthewFeroz:fix/preview-fedcm-sign-in
Open

MatthewFeroz wants to merge 2 commits into
pingdotgg:mainfrom
MatthewFeroz:fix/preview-fedcm-sign-in

Conversation

@MatthewFeroz

@MatthewFeroz MatthewFeroz commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

What Changed

Fix Google sign-in in desktop preview tabs: disable unsupported FedCM so Google uses its popup fallback, and make those popups non-modal children of the T3 window so clicking back on T3 cannot hide the account chooser behind it.

Why

Clicking Google sign-in on YNAB in the Windows Browser panel left the login page unchanged. The console reported Prompt dismissed and [GSI_LOGGER]: FedCM get() rejects with NetworkError: Error retrieving a token. Electron advertises the FedCM API without providing the browser-owned identity dialog, causing Google to choose a flow that cannot show its prompt.

This scopes the workaround to preview tabs. It preserves their sandbox and disabled Node integration, and leaves T3's own sign-in configuration unchanged.

Testing exposed a second issue: an already-open Google chooser could sit behind T3, making sign-in appear unresponsive until refresh opened another window. The reporter confirmed the hidden windows existed. Give popups a live host parent at creation so native window ordering keeps them above T3, without global always-on-top behavior or a modal dialog. Preserve context isolation, the opener, and the guest session; skip a destroyed parent.

Validation

  • Reproduced the failure in the installed Windows desktop app.
  • The reporter ran the changed development desktop app and confirmed Google sign-in works. This is reporter verification, not an automated end-to-end test.
  • Reporter confirmed the follow-up: open Google sign-in, click back on T3, and the popup stays above T3.
  • Added a service-level regression exercising the registered popup handler, live/destroyed host ownership, and security settings. It failed before the ownership fix and passes afterward. Native stacking was verified by the reporter, not by this mocked test.
  • vp test run src/preview/Manager.test.ts src/preview/WebviewPreferences.test.ts in apps/desktop: 100 passed.
  • Targeted lint: passed with one pre-existing test-file warning. Formatting passed.
  • vp run typecheck in apps/desktop: passed (existing Effect suggestions).
  • git diff --check: passed.

The change is desktop preview configuration shared by hosted browser tabs; no provider or wire-contract changes. macOS, Linux, mobile, and remote-hosted preview flows were not manually exercised.

UI Changes

Before: Google sign-in immediately dismisses its FedCM prompt and stays on YNAB's login page.

After: Google sign-in opens its account chooser in the development app, and the chooser stays above T3 when the reporter clicks back on T3.

User-recorded Windows evidence, cropped with account details blurred and audio removed. The newest after recording includes the popup ownership follow-up.

Before: sign-in does not open the chooser After: Google chooser opens and stays above T3
Before After

Before video · After video

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Model: GPT-6 Astra. Harness: Codex in T3 Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Sep 24, 2026
@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). and removed size:XS 0-9 changed lines (additions + deletions). labels Sep 24, 2026
@MatthewFeroz MatthewFeroz changed the title fix(desktop): use popup fallback for preview Google sign-in fix(desktop): restore visible Google sign-in popups in previews Sep 24, 2026
@MatthewFeroz
MatthewFeroz marked this pull request as ready for review September 24, 2026 03:25
@macroscopeapp

macroscopeapp Bot commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The PR changes the default webview configuration for all desktop previews and changes Google OAuth popup ownership and FedCM behavior on an existing authentication path. These are focused changes with tests, but the product-default and authentication scope warrant human review.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 95789fa3-5233-48b4-82c8-75f89f6cfd4e

📥 Commits

Reviewing files that changed from the base of the PR and between e4eb997 and 239ba60.

📒 Files selected for processing (4)
  • apps/desktop/src/preview/Manager.test.ts
  • apps/desktop/src/preview/Manager.ts
  • apps/desktop/src/preview/WebviewPreferences.test.ts
  • apps/desktop/src/preview/WebviewPreferences.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Preview guests now disable FedCm. Allowed sign-in popups use the live main window as their parent when one is available and not destroyed.

Changes

Preview sign-in flow

Layer / File(s) Summary
Disable FedCm in preview guests
apps/desktop/src/preview/WebviewPreferences.ts, apps/desktop/src/preview/WebviewPreferences.test.ts
Preview web preferences add disableBlinkFeatures=FedCm. Tests check the preference and the updated key set.
Parent sign-in popups to the main window
apps/desktop/src/preview/Manager.ts, apps/desktop/src/preview/Manager.test.ts
The window-open handler sets the live main window as the popup parent. Tests cover the popup options and the case where the host window is destroyed.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 239ba

The preview sign-in changes are mergeable after normal checks. No supported issue remains that would block popup fallback or ownership.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: restoring visible Google sign-in popups in desktop previews.
Description check ✅ Passed The description is complete and follows the repository template. It explains what changed, why, UI impact, validation results, scope, limitations, and checklist status.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant