Skip to content

fix(desktop): bake OAuth client IDs into main process at build time - #433

Merged
saddlepaddle merged 2 commits into
mainfrom
particular-flamingo-80aa8e
Dec 19, 2025
Merged

saddlepaddle merged 2 commits into
mainfrom
particular-flamingo-80aa8e

Conversation

@saddlepaddle

@saddlepaddle saddlepaddle commented Dec 19, 2025 •

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed desktop app build failing due to missing OAuth client IDs at runtime
  • Added GOOGLE_CLIENT_ID and GH_CLIENT_ID to the vite define block for main process
  • These are now baked into the bundle at build time instead of read from process.env at runtime

Test plan

  • Trigger desktop build workflow on this branch
  • Download and test the auth flow

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Desktop app now supports Google and GitHub sign-in.
    • Build-time configurable API/web endpoints and OAuth client IDs, allowing the desktop app to be pointed to custom backends and enable third‑party authentication.

✏️ Tip: You can customize this high-level summary in your review settings.

GOOGLE_CLIENT_ID and GH_CLIENT_ID were being passed during build but
not embedded into the main process bundle. Added them to the vite
define block so they're available at runtime in the packaged app.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Dec 19, 2025 •

Copy link
Copy Markdown
Contributor

Walkthrough

Expose four additional environment variables to the desktop Electron build: NEXT_PUBLIC_API_URL, NEXT_PUBLIC_WEB_URL, GOOGLE_CLIENT_ID, and GH_CLIENT_ID. Changes add build-time defines and an explicit runtime env listing; no control-flow or runtime logic changes.

Changes

Cohort / File(s) Change Summary
Vite build defines
apps/desktop/electron.vite.config.ts
Added JSON.stringify-wrapped process.env.NEXT_PUBLIC_API_URL, process.env.NEXT_PUBLIC_WEB_URL, process.env.GOOGLE_CLIENT_ID, and process.env.GH_CLIENT_ID into the define block for the Electron main process (in addition to existing NODE_ENV).
Main process runtime env listing
apps/desktop/src/main/env.main.ts
Replaced spreading of process.env with an explicit runtimeEnv object that lists NODE_ENV, NEXT_PUBLIC_API_URL, NEXT_PUBLIC_WEB_URL, GOOGLE_CLIENT_ID, and GH_CLIENT_ID so Vite can substitute them at build time.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Small, localized changes across two files with repetitive patterns.
  • Review focus:
    • Confirm default values and stringification in electron.vite.config.ts.
    • Verify env.main.ts exposes exactly the intended keys and doesn't omit required vars.
    • Ensure no sensitive secrets are being inappropriately bundled.

Possibly related PRs

Poem

🐰 I hopped through configs, soft and fleet,
Four tiny keys tucked in their seat,
Build-time whispers, strings so neat,
A rabbit's cheer for variables complete! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: baking OAuth client IDs into the main process at build time, which matches the core objective of the PR.
Description check ✅ Passed The description covers the problem, solution, and test plan but lacks several template sections like Related Issues and full Type of Change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch particular-flamingo-80aa8e

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.

❤️ Share

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

…ment

The spread of process.env only works at runtime but is empty in bundled
apps. Each env var must be explicitly listed so Vite can replace the
process.env.X references at build time.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/desktop/src/main/env.main.ts (1)

19-20: Add fallback defaults or explicit validation for OAuth client IDs to match other environment variables.

GOOGLE_CLIENT_ID and GH_CLIENT_ID are injected without fallback defaults in the Vite configuration (lines 69-72), while NEXT_PUBLIC_API_URL and NEXT_PUBLIC_WEB_URL have explicit fallbacks (lines 62-67). Either add fallback values for the OAuth IDs or add explicit build-time validation to ensure these critical variables are always defined, preventing undefined values from being stringified into the bundle.

🧹 Nitpick comments (1)
apps/desktop/src/main/env.main.ts (1)

23-32: Explicit environment variable listing enables correct build-time replacement.

The explicit listing of environment variables (lines 27-31) correctly enables Vite to perform build-time substitution for bundled apps, and the comment clearly explains the rationale. This approach properly addresses the PR objective of baking OAuth client IDs into the bundle.

Optional refactor: Consider removing the process.env spread.

The spread of process.env on line 24 may be redundant if only the schema-defined variables are needed. All validated variables are now explicitly listed below. Removing the spread would make the code clearer and ensure only validated variables are accessed.

🔎 Proposed refactor to remove redundant spread
 	runtimeEnv: {
-		...process.env,
 		// Explicitly list env vars so Vite can replace them at build time
 		// (spreading process.env only works at runtime, not for bundled apps)
 		NODE_ENV: process.env.NODE_ENV,
 		NEXT_PUBLIC_API_URL: process.env.NEXT_PUBLIC_API_URL,
 		NEXT_PUBLIC_WEB_URL: process.env.NEXT_PUBLIC_WEB_URL,
 		GOOGLE_CLIENT_ID: process.env.GOOGLE_CLIENT_ID,
 		GH_CLIENT_ID: process.env.GH_CLIENT_ID,
 	},

Only apply this change if no additional unvalidated environment variables need to be accessible in the main process.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82087c4 and fa5ede4.

📒 Files selected for processing (2)
  • apps/desktop/electron.vite.config.ts (1 hunks)
  • apps/desktop/src/main/env.main.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/desktop/electron.vite.config.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)

apps/desktop/**/*.{ts,tsx,js,jsx}: For Electron interprocess communication, ALWAYS use trpc as defined in src/lib/trpc
Use alias as defined in tsconfig.json when possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.

Files:

  • apps/desktop/src/main/env.main.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Avoid using any type in TypeScript - maintain type safety unless absolutely necessary

Files:

  • apps/desktop/src/main/env.main.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix

Files:

  • apps/desktop/src/main/env.main.ts
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers

Files:

  • apps/desktop/src/main/env.main.ts
apps/desktop/src/main/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()

Files:

  • apps/desktop/src/main/env.main.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in src/main/index.ts and electron.vite.config.ts with override: true before other imports
📚 Learning: 2025-12-18T23:19:10.405Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in src/main/index.ts and electron.vite.config.ts with override: true before other imports

Applied to files:

  • apps/desktop/src/main/env.main.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build - macOS (arm64)
  • GitHub Check: Build

@saddlepaddle
saddlepaddle merged commit ca75863 into main Dec 19, 2025
7 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

Service Status
Neon Database (Neon) ✅

Thank you for your contribution! 🎉


Preview resources have been processed for cleanup

This branch was previously deployed

1 inactive deployment
production — fa5ede4e Deployed Dec 19, 2025 by saddlepaddle via Build - macOS (arm64) #83
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant