feat: ship auth MCP and release automation#403
feat: ship auth MCP and release automation#403DinoEgg-code wants to merge 1 commit intosiddharthvaddem:mainfrom
Conversation
📝 WalkthroughWalkthroughMassive rebrand from OpenScreen to Auto Screen with new backend auth/billing infrastructure, desktop authentication flows, MCP server integration, auto-edit and smart speed features, Korean localization, and extensive editor/UI updates. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes This is genuinely a big one—new backend system, auth infrastructure, MCP integration, multiple editor features, and KO localization all at once. The heterogeneity across backend, desktop, frontend, and i18n, plus the density of auth/command logic, makes this non-trivial. The sheer file count (200+) and the interconnected nature of auth state flowing through main/renderer/backend means you really gotta trace the whole loop. Possibly related PRs
Suggested reviewers
✨ Finishing Touches⚔️ Resolve merge conflicts
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 896c10d661
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let session = createSessionFromDirectCallback(parsed); | ||
| if (!session && parsed.code) { | ||
| session = await exchangeDesktopCode(parsed.code); |
There was a problem hiding this comment.
handleDesktopProtocolUrl trusts createSessionFromDirectCallback before attempting exchangeDesktopCode, so a deep link containing accessToken/refreshToken (plus optional plan/entitlements) is accepted without any backend validation. Because custom protocol URLs can be opened by any local app or browser page, this allows forged desktop sessions (including pro entitlement state) via a crafted autoscreen://auth/callback URL. Require server-side code exchange (or a signed callback payload) for all successful auth callbacks.
Useful? React with 👍 / 👎.
| await pool.query(`delete from phone_verifications where phone_number = $1 and purpose = $2`, [ | ||
| phoneNumber, | ||
| payload.purpose, | ||
| ]); |
There was a problem hiding this comment.
The hourly throttle checks count(*) from phone_verifications, but this path immediately deletes the prior row for the same phone/purpose before inserting the new request. As a result, the table never accumulates enough rows for phoneVerificationMaxRequestsPerHour to fire, so clients can keep requesting codes indefinitely (subject only to cooldown), which weakens abuse protection and can increase SMS cost.
Useful? React with 👍 / 👎.
| verificationToken: string; | ||
| }, | ||
| ) => { | ||
| return await findLocalUsername(payload); |
There was a problem hiding this comment.
This recovery handler always queries the local account store, even when the prior OTP verification was completed against backend APIs. In packaged/API-auth environments, verification tokens are server-side only, so find-id recovery fails for real backend accounts despite successful phone verification. Route this lookup (and matching reset flow) through backend endpoints when API auth is active.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/components/video-editor/ExportDialog.tsx (1)
231-243:⚠️ Potential issue | 🟡 MinorAudio phase shows under the wrong card header.
Right now audio phase can render
processingAudioShortwhile the label still saysFormat. tiny thing, but kinda cursed in-progress UX.🎯 Proposed fix
- <div className="text-[10px] text-slate-500 uppercase tracking-wider mb-1"> - {isCompiling || isFinalizing ? t("export.status") : t("export.format")} - </div> + <div className="text-[10px] text-slate-500 uppercase tracking-wider mb-1"> + {isCompiling || isFinalizing || isProcessingAudio + ? t("export.status") + : t("export.format")} + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/ExportDialog.tsx` around lines 231 - 243, The header currently uses only isCompiling || isFinalizing to decide between t("export.status") and t("export.format"), which causes the audio-processing state (isProcessingAudio with exportFormat === "mp4") to render the "Format" header incorrectly; update the header conditional in ExportDialog.tsx to treat (isProcessingAudio && exportFormat === "mp4") as a status state as well (i.e., use (isProcessingAudio && exportFormat === "mp4") || isCompiling || isFinalizing) so the header and the body text (which already checks isProcessingAudio) remain consistent.src/hooks/useScreenRecorder.ts (1)
228-266:⚠️ Potential issue | 🔴 CriticalSnapshot interaction buffers before async finalize (Line 261-Line 265).
kinda cursed race here:
finalizeRecordingawaits blobs, then readsclickSamples.current/keySamples.current. during restart, Line 592-Line 593 resets those refs for the next session before finalize stores the old one. that can mix telemetry across recordings.safe snapshot fix
- void (async () => { + const interactionClicks = [...clickSamples.current]; + const interactionKeys = [...keySamples.current]; + void (async () => { try { const screenBlob = await activeScreenRecorder.recordedBlobPromise; ... const result = await window.electronAPI.storeRecordedSession({ ... interactions: { version: 1, - clicks: clickSamples.current, - keys: keySamples.current, + clicks: interactionClicks, + keys: interactionKeys, }, });Also applies to: 591-593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 228 - 266, The async finalize block reads mutable refs clickSamples.current and keySamples.current after awaiting blobs, causing telemetry races; snapshot those arrays into local constants (e.g., const clicksSnapshot = [...clickSamples.current]; const keysSnapshot = [...keySamples.current]) before any awaits (before awaiting fixWebmDuration/storeRecordedSession) and pass those snapshots into window.electronAPI.storeRecordedSession and the interactions object instead of reading clickSamples.current/keySamples.current later; also ensure you take the snapshots after the discardRecordingId/current activeRecordingId check so they belong to the intended session (use activeRecordingId, clickSamples, keySamples, and window.electronAPI.storeRecordedSession to locate the code).src/components/video-editor/projectPersistence.ts (1)
336-353:⚠️ Potential issue | 🟠 MajorLoad-time defaults shouldn’t restyle older projects.
This normalizer runs when reopening persisted data, so changing these fallbacks means older project files that are missing the fields can load with different visuals/behavior and then re-export differently without any user edit. That’s kinda cursed for project persistence. I’d keep legacy-compatible fallbacks here and apply the new defaults only when creating a brand-new editor state, or bump the project version and migrate explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistence.ts` around lines 336 - 353, The normalizer in projectPersistence.ts is applying new visual defaults during load (fields like motionBlurAmount/motionBlurEnabled, shadowIntensity, borderRadius, padding, editMode, smartSpeedEnabled, smartSpeedIntensity) which will change appearance of older projects; revert this code to use legacy-compatible fallbacks when fields are missing and only apply the new defaults when creating a brand-new editor state (e.g., move new default logic into the editor factory/createNewEditorState function) or perform an explicit migration when bumping the project version so existing persisted projects are not silently restyled on load.src/components/video-editor/VideoEditor.tsx (1)
1430-1454:⚠️ Potential issue | 🟠 MajorAdd
accessPolicyandonUpgradeto the dependency arrays in these three export callbacks.All three are using stale auth checks—when the user's plan updates, logs in/out, or session refreshes, these callbacks keep the old
accessPolicyandonUpgradehandler, which kinda cursed. A user who downgrades still thinks they can export, one who upgrades stays blocked. Lowkey risky auth bug.Affects:
handleSaveUnsavedExport(1430-1454)handleExport(1456-1735)handleOpenExportDialog(1747-1811)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 1430 - 1454, Three export-related callbacks (handleSaveUnsavedExport, handleExport, handleOpenExportDialog) close over stale auth state because their useCallback dependency arrays are missing accessPolicy and onUpgrade; update each useCallback hook to include accessPolicy and onUpgrade so the callbacks re-create when the user's plan/session changes, ensuring the latest accessPolicy.canExport checks and the current onUpgrade handler are used.
🟡 Minor comments (19)
src/components/video-editor/videoPlayback/zoomRegionUtils.ts-139-151 (1)
139-151:⚠️ Potential issue | 🟡 Minoradd explicit check to prevent connecting overlapping zoom regions
the duration clamping is solid, but
shouldConnectRegionslets negative gaps (overlapping regions) slip through. they'd hit the fallback duration instead of getting rejected outright. there's no test coverage for overlapping regions either, which is kinda a gap.suggest adding:
Proposed fix
function shouldConnectRegions(currentRegion: ZoomRegion, nextRegion: ZoomRegion, gapMs: number) { - if (gapMs > CHAINED_ZOOM_PAN_GAP_MS) { + if (gapMs < 0 || gapMs > CHAINED_ZOOM_PAN_GAP_MS) { return false; }while you're at it, lowkey worth a test case for overlapping regions to make the intent clear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/videoPlayback/zoomRegionUtils.ts` around lines 139 - 151, The code currently computes transitionDuration and pushes a connection even when regions overlap (gapMs < 0); add an explicit guard in the pairing logic (in zoomRegionUtils.ts where currentRegion/nextRegion are processed and pairs are pushed) to skip/return false for overlapping regions (check gapMs < 0 before computing transitionDuration and before pushing into pairs), and add a unit test that constructs two overlapping regions and asserts they are not connected by shouldConnectRegions / the pairing logic to make the intent explicit.CONTRIBUTING.md-46-46 (1)
46-46:⚠️ Potential issue | 🟡 MinorIssue-reporting instructions need a direct link.
“official support/issues channel” is ambiguous; please include the exact URL so reports don’t get lost in the void at 2am.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 46, Update the issue-reporting line in CONTRIBUTING.md to replace the ambiguous phrase "official Auto Screen support/issues channel" with a direct, clickable URL to the exact support or issues page (e.g., the repository's Issues tab or external support portal); locate the sentence containing that phrase and insert the full absolute URL so users have a clear, unambiguous link to file bugs or feature requests.docs/clipwise-reference-notes.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorMachine-local absolute path in shared docs is kinda cursed.
/Users/admin/Desktop/clipwiseis not portable and can unintentionally leak local environment details. recommend removing it or replacing with a neutral placeholder.🧹 Suggested doc cleanup
-- 로컬 경로: /Users/admin/Desktop/clipwise +- 로컬 경로: (optional, developer-local only; do not commit absolute paths)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/clipwise-reference-notes.md` at line 5, The doc contains a machine-local absolute path "/Users/admin/Desktop/clipwise" which is non-portable and leaks local info; remove that exact token or replace it with a neutral placeholder (e.g. "<LOCAL_PATH>" or "path/to/clipwise") so the shared docs contain no user-specific paths, and update any surrounding text to read generically about the project location.backend/src/db/sql/006_billing_events.sql-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorAdd
gen_random_uuid()default or ensure billing_events inserts always supplyid.Currently,
idhas no default, which means any insert forgetting to provide it will hard-fail. While this table isn't being used yet (the subscription service is still stubbed), when you implement the actual billing webhook handler and start writing billing_events, make sure either:
- Add
default gen_random_uuid()to the id column, OR- Ensure the application always explicitly provides
idon insert.(Note: the codebase pattern has all uuid primary keys without defaults, so either approach is fine as long as it's consistent.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/db/sql/006_billing_events.sql` at line 2, Add a default UUID generation for the billing_events primary key or ensure inserts always provide an id: update the billing_events table definition (the id column) to include default gen_random_uuid() if you want DB-side UUID generation, or alternatively keep id without a default but verify the application-level insert logic in the billing webhook handler always supplies a non-null UUID; reference the billing_events table and the id column when making the change so it's consistently applied with other UUID primary keys.src/components/video-editor/timeline/TimelineEditor.tsx-1311-1313 (1)
1311-1313:⚠️ Potential issue | 🟡 MinorHardcoded smart-speed label skips localization (minor i18n regression).
Line 1312 is now a literal English string, so non-English locales won’t get translated text here. lowkey risky given the rest of this row is localized.
nit: cleaner localized variant
- label: region.id.startsWith("smart-speed-") - ? `Smart ${region.speed}×` - : t("labels.speedItem", { index: String(index + 1) }), + label: region.id.startsWith("smart-speed-") + ? t("labels.smartSpeedItem", { speed: String(region.speed) }) + : t("labels.speedItem", { index: String(index + 1) }),(You’ll also want
labels.smartSpeedItemadded in locale JSONs.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 1311 - 1313, The label for smart-speed regions is hardcoded ("Smart ${region.speed}×") and needs to be localized: in the conditional that checks region.id.startsWith("smart-speed-") (inside the label assignment that currently falls back to t("labels.speedItem")), replace the literal string with a call to the i18n helper (e.g., t("labels.smartSpeedItem", { speed: region.speed }) or similar interpolation your i18n uses) and add the corresponding labels.smartSpeedItem entries to your locale JSONs so translations exist.src/contexts/I18nContext.tsx-49-55 (1)
49-55:⚠️ Potential issue | 🟡 MinorLegacy fallback can be skipped when new key has junk value (lowkey risky).
On Line 49-50,
??means legacy storage is only read when the new key isnull. If the new key exists but is invalid, you’ll skip a valid legacy locale and drop to default.nit: cleaner fallback order + single source of default
import { type I18nNamespace, + DEFAULT_LOCALE, LEGACY_LOCALE_STORAGE_KEY, LOCALE_STORAGE_KEY, type Locale, SUPPORTED_LOCALES, } from "@/i18n/config"; function getInitialLocale(): Locale { try { - const stored = - localStorage.getItem(LOCALE_STORAGE_KEY) ?? localStorage.getItem(LEGACY_LOCALE_STORAGE_KEY); - if (stored && isSupportedLocale(stored)) return stored; + const stored = localStorage.getItem(LOCALE_STORAGE_KEY); + if (stored && isSupportedLocale(stored)) return stored; + + const legacyStored = localStorage.getItem(LEGACY_LOCALE_STORAGE_KEY); + if (legacyStored && isSupportedLocale(legacyStored)) return legacyStored; } catch { // localStorage may be unavailable } - return "ko"; + return DEFAULT_LOCALE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/I18nContext.tsx` around lines 49 - 55, The current retrieval uses the nullish coalescing operator so LEGACY_LOCALE_STORAGE_KEY is only checked when LOCALE_STORAGE_KEY is null, which lets a present-but-invalid new key skip a valid legacy value; change the logic in the I18nContext retrieval (the block using LOCALE_STORAGE_KEY, LEGACY_LOCALE_STORAGE_KEY and the stored variable) to read both keys (check LOCALE_STORAGE_KEY first, then LEGACY_LOCALE_STORAGE_KEY) and validate each with isSupportedLocale before returning, and ensure the fallback default ("ko") is returned from a single place if neither key yields a supported locale.scripts/test-mcp-client.mjs-5-8 (1)
5-8:⚠️ Potential issue | 🟡 MinorAdd CLI argument validation before URL parsing.
If args are missing, script throws early with a not-super-helpful error. quick usage guard makes this much friendlier.
🛠️ Proposed fix
const urlArg = process.argv[2]; const token = process.argv[3]; +if (!urlArg || !token) { + console.error("Usage: node scripts/test-mcp-client.mjs <url> <token>"); + process.exit(1); +} const preflight = await runPreflight(urlArg, token); const url = new URL(urlArg);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-mcp-client.mjs` around lines 5 - 8, The script currently reads process.argv[2] and [3] into urlArg and token and immediately calls runPreflight and new URL(urlArg), which throws if args are missing; add a CLI-guard at the top to validate presence of urlArg (and token if required) before calling runPreflight or instantiating URL, returning a friendly usage message and non-zero exit code when missing; update references around urlArg, token, runPreflight and the new URL(...) call so validation happens first and only then proceed to runPreflight and URL parsing.src/i18n/locales/ko/launch.json-5-9 (1)
5-9:⚠️ Potential issue | 🟡 Minor
restartvsresumelabels are identical.both map to
"녹화 다시 시작", so users can’t distinguish restart from resume. worth splitting wording for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/locales/ko/launch.json` around lines 5 - 9, The Korean translations for the keys restartRecording and resumeRecording are identical; update resumeRecording to a distinct phrase (e.g., "녹화 재개") so users can differentiate restarting from resuming; modify the value for the resumeRecording key in the same JSON block while keeping restartRecording as "녹화 다시 시작".src/i18n/config.ts-1-2 (1)
1-2:⚠️ Potential issue | 🟡 MinorNo browser/OS locale detection means new users land in Korean by default.
The
getInitialLocale()function (I18nContext.tsx:47-56) only checks stored preferences before falling back to "ko". There's nonavigator.languagedetection, so first-time users get Korean regardless of their actual locale setting. kinda cursed for global users hitting the app for the first time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/config.ts` around lines 1 - 2, getInitialLocale currently falls back to DEFAULT_LOCALE ("ko") without checking the browser/OS locale; update getInitialLocale in I18nContext.tsx to first check stored preferences, then inspect navigator.language / navigator.languages to derive the user's preferred locale, match or map that value to one of SUPPORTED_LOCALES (handle language-only codes like "en" -> "en", region variants like "zh-CN" -> "zh-CN"), and only if no match is found return DEFAULT_LOCALE; use the constants DEFAULT_LOCALE and SUPPORTED_LOCALES during this matching to keep behavior consistent.docs/local-mcp-workflow.md-50-54 (1)
50-54:⚠️ Potential issue | 🟡 MinorAdd a Windows-friendly example for starting the editor.
Line 53 is POSIX-only shell syntax, so Windows folks will hit a wall immediately. nit: cleaner to include a PowerShell variant or call out that this command assumes bash/zsh.
📝 Suggested doc tweak
```bash +# macOS / Linux AUTO_SCREEN_START_EDITOR=true npm run dev + +# PowerShell +$env:AUTO_SCREEN_START_EDITOR="true"; npm run dev</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/local-mcp-workflow.mdaround lines 50 - 54, The POSIX-only example
"AUTO_SCREEN_START_EDITOR=true npm run dev" will fail on Windows; update
docs/local-mcp-workflow.md to either note the command assumes a bash/zsh shell
or add a Windows-friendly PowerShell variant (e.g., show how to set
AUTO_SCREEN_START_EDITOR in PowerShell before running npm run dev) so Windows
users can start the editor without errors.</details> </blockquote></details> <details> <summary>src/components/video-editor/TutorialHelp.tsx-40-43 (1)</summary><blockquote> `40-43`: _⚠️ Potential issue_ | _🟡 Minor_ **This hardcodes word order into translated copy.** The new fragment stitching is lowkey broken for non-English locales. With the Korean strings added in this PR, this renders as an awkward `"제거할 포함된 ..."` sequence because the component forces ordering and spaces instead of letting the locale own the sentence. Use one translation entry with placeholders/rich-text markup so each locale controls ordering and spacing. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@src/components/video-editor/TutorialHelp.tsxaround lines 40 - 43, The
template hardcodes word order by concatenating multiple t(...) fragments
(tutorial.explanation, tutorial.explanationRemove, tutorial.explanationCovered,
tutorial.explanationEnd) which breaks non-English locales; replace these with a
single translation key that contains placeholders or rich-text markup and render
it with your i18n rich-text interpolation (e.g., using the Trans component or t
with interpolation) so each locale controls ordering and spacing, and preserve
the red-bold styling by mapping placeholders to the existing span
className="text-[#ef4444] font-bold" for the two emphasized parts.</details> </blockquote></details> <details> <summary>src/App.tsx-43-46 (1)</summary><blockquote> `43-46`: _⚠️ Potential issue_ | _🟡 Minor_ **This boot message is hardcoded in Korean.** Everyone hitting the editor boot state will see Korean text here, even with `es` / `zh-CN` / English selected. Since this PR is already expanding locale coverage, this should come from i18n too instead of being inline. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@src/App.tsxaround lines 43 - 46, The hardcoded Korean boot message ("Auto
Screen 시작 중입니다.") in App.tsx should be replaced with an i18n lookup so it
respects the user's locale; import and use the existing translation helper
(e.g., useTranslation from react-i18next or the project's i18n.t wrapper) in the
component that renders the div and replace the literal string with a call like
t('boot.autoScreenStarting') (or the project's equivalent key), and add the new
key/value to each locale file (en, es, zh-CN, ko) so all locales show the proper
translated message.</details> </blockquote></details> <details> <summary>docs/plans/auth-backend-mvp.md-212-217 (1)</summary><blockquote> `212-217`: _⚠️ Potential issue_ | _🟡 Minor_ **Keep the plan code consistent in the contract.** This example switches to `pro_monthly`, while the schema and earlier responses use `pro_monthly_15900_krw`. docs-driven clients will wire the wrong enum if this drifts. <details> <summary>📝 Suggested doc fix</summary> ```diff { - "plan": "pro_monthly", + "plan": "pro_monthly_15900_krw", "status": "active", "entitlements": ["mcp_editing", "advanced_auto_edit", "export_hd"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/auth-backend-mvp.md` around lines 212 - 217, The example JSON uses an inconsistent plan code "pro_monthly"; update it to the canonical enum used elsewhere ("pro_monthly_15900_krw") so the "plan" field matches the schema and earlier examples, and scan the surrounding text/examples to ensure all occurrences of the plan code use "pro_monthly_15900_krw".README.md-186-186 (1)
186-186:⚠️ Potential issue | 🟡 Minorcontribution guidance points to... nowhere?
"please use the official Auto Screen contribution channel and project board" but there's no link. readers won't know where to go.
either add the actual links or remove this sentence and just say contributions are welcome.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 186, The README sentence "please use the official Auto Screen contribution channel and project board" points to nowhere; either replace that sentence with direct links to the contribution channel and project board (add the URLs inline or as markdown links) or remove the clause and keep a simple "Contributions are welcome" line; update the README content containing that exact phrase to ensure readers have actionable links or a concise invite.README.md-24-26 (1)
24-26:⚠️ Potential issue | 🟡 Minorinvalid css values in style attributes
height: 0.2467andheight: 0.1678are missing units — browsers will ignore these. did you mean percentages or pixels?🐛 fix with actual units
- <img src="public/preview3.png" alt="Auto Screen App Preview 3" style="height: 0.2467; margin-right: 12px;" /> - <img src="public/preview4.png" alt="Auto Screen App Preview 4" style="height: 0.1678; margin-right: 12px;" /> + <img src="public/preview3.png" alt="Auto Screen App Preview 3" style="height: 247px; margin-right: 12px;" /> + <img src="public/preview4.png" alt="Auto Screen App Preview 4" style="height: 168px; margin-right: 12px;" />(adjust pixel values to whatever you actually want)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 24 - 26, The two <img> elements with alt texts "Auto Screen App Preview 3" and "Auto Screen App Preview 4" use invalid CSS heights ("height: 0.2467" and "height: 0.1678") without units; update their style attributes to include proper units (e.g., "height: 24px", "height: 24%", or "height: 0.2467rem" depending on desired sizing) so browsers honor the values and adjust the margin-right if needed for consistent spacing.backend/src/services/admin-service.ts-75-90 (1)
75-90:⚠️ Potential issue | 🟡 Minorsilently swallowing postgres errors makes debugging hard
when the postgres query fails and you fall back to file, you're catching the error without logging it. if something's misconfigured, you'll just silently get file data and have no idea postgres is failing.
at minimum, log the error before falling back.
🔧 add logging
- } catch { + } catch (err) { + console.error("[admin-service] postgres query failed, falling back to file:", err); const logs = await readFileAuditLogs();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/services/admin-service.ts` around lines 75 - 90, The catch block that falls back to readFileAuditLogs is silently swallowing the Postgres error; update the catch to accept the error (e.g., catch (err)) and log it before returning the file-fallback result. Use the existing logger instance (or console.error if none) to emit a clear message like "Postgres audit logs query failed, falling back to file" along with the error and stack, then proceed to call readFileAuditLogs() and return the same filtered/sorted/sliced payload; reference the catch block around the readFileAuditLogs() fallback and the readFileAuditLogs function.backend/src/services/sms-service.ts-46-61 (1)
46-61:⚠️ Potential issue | 🟡 Minormissing timeout on external API call
the
fetchto Solapi has no timeout configured. if the SMS provider hangs or is slow, this could block the request thread indefinitely and potentially cause cascading issues.consider using
AbortControllerwith a timeout:add timeout to fetch
try { const auth = buildSolapiAuthorization(); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); const response = await fetch(`${config.smsApiBaseUrl}/messages/v4/send-many/detail`, { method: "POST", headers: { "Content-Type": "application/json", Authorization: auth.header, }, body: JSON.stringify({ message: { from: config.smsSender, text: `[Auto Screen] 인증번호는 ${code} 입니다. 3분 안에 입력해주세요.`, }, messages: [{ to: phoneNumber }], }), + signal: controller.signal, }); + clearTimeout(timeoutId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/services/sms-service.ts` around lines 46 - 61, The fetch call in the SMS send flow (inside the try block that uses buildSolapiAuthorization) has no timeout; add an AbortController, set a timer (e.g., via setTimeout) to call controller.abort() after a reasonable timeout (e.g., 5s), pass controller.signal into the fetch options, and clear the timeout after fetch resolves to avoid leaks; update error handling in the same block to treat an AbortError as a timeout case so send logic fails fast.electron/preload.ts-106-111 (1)
106-111:⚠️ Potential issue | 🟡 Minorpreload phone verification types should expose the
purposefieldthe handlers in
electron/main.ts(lines 612, 652) accept an optionalpurpose?: "signup" | "recovery"param and handle it properly, but the preload types at lines 106-111 don't expose it. means renderer code can't override the default "signup" value even though the backend handlers support it. just addpurpose?: "signup" | "recovery"to both payload types for consistency and flexibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/preload.ts` around lines 106 - 111, The preload type definitions for requestLocalPhoneVerification and verifyLocalPhoneCode are missing the optional purpose field that main handlers accept; update the payload types in electron/preload.ts for requestLocalPhoneVerification (payload: { phoneNumber: string; deviceId?: string }) and verifyLocalPhoneCode (payload: { phoneNumber: string; code: string; deviceId?: string }) to include purpose?: "signup" | "recovery" so renderer callers can pass purpose through to ipcRenderer.invoke without changing the invoke calls themselves.src/components/video-editor/SettingsPanel.tsx-419-441 (1)
419-441:⚠️ Potential issue | 🟡 Minor
focusSection="mcp"doesn't actually focus anything.This effect opens
"mcp"and tries to scrollmcpSectionRef, but the ref is never attached to rendered JSX and there isn't an accordion item with value"mcp". So the navigation path is a no-op right now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` around lines 419 - 441, The focusSection handling currently tries to open and scroll "mcp" but nothing is wired to that ref/value; update the SettingsPanel JSX so the DOM node representing the MCP section uses the mcpSectionRef and ensure the accordion/item controlling that panel has the value "mcp" (or map the focusSection to an existing accordion value), then keep the effect as-is to call setOpenSections and scrollIntoView; specifically attach mcpSectionRef to the MCP section container element and confirm the AccordionItem/section value matches "mcp" (or adjust focusSection usage) so the scroll and open logic in useEffect(focusSection...) actually targets a real element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73348ccc-6655-4762-a9ac-cd03abc1b09f
⛔ Files ignored due to path filters (18)
assets/branding/autoscreen-brandmark.svgis excluded by!**/*.svgassets/branding/autoscreen-recording-brandmark.svgis excluded by!**/*.svgbackend/package-lock.jsonis excluded by!**/package-lock.jsonicons/icons/1024x1024.pngis excluded by!**/*.pngicons/icons/128x128.pngis excluded by!**/*.pngicons/icons/16x16.pngis excluded by!**/*.pngicons/icons/24x24.pngis excluded by!**/*.pngicons/icons/256x256.pngis excluded by!**/*.pngicons/icons/32x32.pngis excluded by!**/*.pngicons/icons/48x48.pngis excluded by!**/*.pngicons/icons/512x512.pngis excluded by!**/*.pngicons/icons/64x64.pngis excluded by!**/*.pngicons/icons/icon.icois excluded by!**/*.icoicons/icons/win/icon.icois excluded by!**/*.icoicons/source/autoscreen-base.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.jsonpublic/autoscreen-recording.pngis excluded by!**/*.pngpublic/autoscreen.pngis excluded by!**/*.png
📒 Files selected for processing (135)
.github/workflows/build.yml.gitignoreCONTRIBUTING.mdREADME.mdbackend/README.mdbackend/package.jsonbackend/src/app.tsbackend/src/config.tsbackend/src/db/pg.tsbackend/src/db/sql/001_users.sqlbackend/src/db/sql/002_devices.sqlbackend/src/db/sql/003_subscriptions.sqlbackend/src/db/sql/004_desktop_login_codes.sqlbackend/src/db/sql/005_phone_verifications.sqlbackend/src/db/sql/006_billing_events.sqlbackend/src/db/sql/007_signup_audit.sqlbackend/src/index.tsbackend/src/routes/admin.tsbackend/src/routes/auth.tsbackend/src/routes/billing.tsbackend/src/routes/entitlements.tsbackend/src/routes/health.tsbackend/src/services/admin-service.tsbackend/src/services/auth-postgres-service.tsbackend/src/services/auth-service.tsbackend/src/services/sms-service.tsbackend/src/services/subscription-service.tsbackend/tsconfig.jsonbiome.jsondocs/clipwise-reference-notes.mddocs/local-mcp-workflow.mddocs/mcp-demo.mddocs/plans/2026-04-09-mcp-editor-control.mddocs/plans/2026-04-auth-billing-desktop-architecture.mddocs/plans/auth-backend-mvp.mddocs/plans/billing-mvp.mddocs/plans/electron-auth-flow.mddocs/plans/landing-page-funnel.mddocs/plans/signup-abuse-policy.mddocs/release-checklist.mdelectron-builder.json5electron/auth/client.tselectron/auth/deviceFingerprint.tselectron/auth/localAccountStore.tselectron/auth/protocol.tselectron/auth/sessionStore.tselectron/electron-env.d.tselectron/i18n.tselectron/ipc/editorBridge.tselectron/ipc/handlers.tselectron/main.tselectron/mcp/auth.tselectron/mcp/server.tselectron/preload.tselectron/windows.tsicons/icons/icon.icnsicons/icons/mac/icon.icnsindex.htmlpackage.jsonplaywright.config.tsscripts/mcp-preflight.mjsscripts/print-mcp-config.mjsscripts/release-artifacts.mjsscripts/release-clean.mjsscripts/test-mcp-client.mjsscripts/test-mcp-demo-once.mjsscripts/test-mcp-mutations.mjssrc/App.tsxsrc/components/launch/LaunchWindow.tsxsrc/components/video-editor/EditorStartScreen.tsxsrc/components/video-editor/ExportDialog.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/ShortcutsConfigDialog.tsxsrc/components/video-editor/TutorialHelp.tsxsrc/components/video-editor/VideoEditor.module.csssrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/smartSpeedUtils.test.tssrc/components/video-editor/smartSpeedUtils.tssrc/components/video-editor/timeline/Row.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/zoomSuggestionUtils.test.tssrc/components/video-editor/timeline/zoomSuggestionUtils.tssrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/cursorVisualUtils.test.tssrc/components/video-editor/videoPlayback/cursorVisualUtils.tssrc/components/video-editor/videoPlayback/mathUtils.tssrc/components/video-editor/videoPlayback/zoomRegionUtils.test.tssrc/components/video-editor/videoPlayback/zoomRegionUtils.tssrc/contexts/I18nContext.tsxsrc/editor/commands/autoEdit.tssrc/editor/commands/helpers.tssrc/editor/commands/regions.tssrc/editor/commands/types.tssrc/editor/selectors/projectState.tssrc/editor/useEditorController.tssrc/features/auth/AuthGate.module.csssrc/features/auth/AuthGate.tsxsrc/features/auth/DesktopAuthDialog.tsxsrc/features/auth/TrialExpiredGate.tsxsrc/features/auth/authTypes.tssrc/features/auth/useAuthSession.tssrc/hooks/useEditorHistory.tssrc/hooks/useScreenRecorder.tssrc/i18n/config.tssrc/i18n/locales/en/dialogs.jsonsrc/i18n/locales/en/editor.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/dialogs.jsonsrc/i18n/locales/es/editor.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/ko/common.jsonsrc/i18n/locales/ko/dialogs.jsonsrc/i18n/locales/ko/editor.jsonsrc/i18n/locales/ko/launch.jsonsrc/i18n/locales/ko/settings.jsonsrc/i18n/locales/ko/shortcuts.jsonsrc/i18n/locales/ko/timeline.jsonsrc/i18n/locales/zh-CN/dialogs.jsonsrc/i18n/locales/zh-CN/editor.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/lib/customFonts.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/types.tssrc/lib/exporter/videoExporter.tssrc/lib/recordingSession.tssrc/lib/shortcuts.test.tssrc/lib/shortcuts.tssrc/lib/userPreferences.tssrc/utils/getTestId.tssrc/vite-env.d.tstests/e2e/record-flow.spec.ts
| tossBillingEnabled: process.env.TOSS_BILLING_ENABLED === "true", | ||
| databaseUrl: process.env.DATABASE_URL || "", | ||
| databaseSsl: process.env.DATABASE_SSL === "true", | ||
| adminApiToken: process.env.ADMIN_API_TOKEN || "", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how adminApiToken is validated in admin routes
rg -n -A5 'adminApiToken' --type=tsRepository: siddharthvaddem/openscreen
Length of output: 1044
empty adminApiToken default is an auth bypass waiting to happen
yeah, so here's the problem: when ADMIN_API_TOKEN isn't set, it defaults to "", and then the auth check in admin.ts does if (!config.adminApiToken) { return true; }. empty string is falsy, so this literally allows any request through when the token isn't configured. not great.
either throw on startup if the token isn't set (better), or at minimum check if (!config.adminApiToken || token !== config.adminApiToken) reject() so an empty configured token doesn't become a free pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/config.ts` at line 12, The adminApiToken default of "" creates an
auth bypass because admin.ts checks `if (!config.adminApiToken) { return true;
}`; change this so missing token fails fast: in config.ts stop defaulting to an
empty string (e.g., leave undefined or explicitly validate) and validate at
startup in the module that loads config (or in a new init function) to throw an
error if ADMIN_API_TOKEN is not set, and/or update the auth check in admin.ts to
require an exact match (e.g., ensure token !== undefined && token ===
config.adminApiToken) so an empty configured token cannot grant access; refer to
the symbol adminApiToken in config.ts and the auth logic in admin.ts to
implement the fix.
| function isAuthorized(token: string | undefined) { | ||
| if (!config.adminApiToken) { | ||
| return true; | ||
| } | ||
| return token === config.adminApiToken; |
There was a problem hiding this comment.
Fail closed when adminApiToken is missing.
Right now a missing config turns the whole admin router public, which is kinda cursed for /api/admin/*. A bad deploy or env typo becomes full unauthenticated access to audit data and storage status.
Suggested fix
function isAuthorized(token: string | undefined) {
- if (!config.adminApiToken) {
- return true;
- }
+ if (!config.adminApiToken) {
+ return false;
+ }
return token === config.adminApiToken;
}If local dev needs an escape hatch, gate that explicitly on a dev-only flag instead of making production misconfigurations fail open.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isAuthorized(token: string | undefined) { | |
| if (!config.adminApiToken) { | |
| return true; | |
| } | |
| return token === config.adminApiToken; | |
| function isAuthorized(token: string | undefined) { | |
| if (!config.adminApiToken) { | |
| return false; | |
| } | |
| return token === config.adminApiToken; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/routes/admin.ts` around lines 8 - 12, The isAuthorized function
currently treats a missing config.adminApiToken as allowing access; change it to
"fail closed" by returning false when adminApiToken is falsy unless an explicit
dev-only override is set: update isAuthorized(token) to return token ===
config.adminApiToken, but if config.adminApiToken is missing allow access only
when a new explicit flag (e.g., config.allowInsecureAdminForDev) is true AND the
runtime is development (e.g., process.env.NODE_ENV === 'development'); reference
the isAuthorized function and the
config.adminApiToken/config.allowInsecureAdminForDev flags so callers of the
admin router remain protected in production.
| billingRouter.post("/webhooks/toss", async (req, res) => { | ||
| const result = await handleTossWebhook(req.body); | ||
| res.json({ ok: true, ...result }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Verify body parser setup / middleware ordering around billing routes
rg -n -C4 --type=ts 'app\.use|express\.json|express\.raw|/api/billing|webhooks/toss' backend/src/app.ts backend/src/routes/billing.ts
# 2) Verify webhook signature/replay validation exists in service layer
rg -n -C4 --type=ts 'handleTossWebhook|signature|hmac|secret|timestamp|replay|verify' backend/src/services/subscription-service.ts backend/src/routes/billing.tsRepository: siddharthvaddem/openscreen
Length of output: 3216
Webhook authenticity checks are missing (critical).
The /webhooks/toss endpoint directly processes req.body without any signature verification, timestamp validation, or replay protection. handleTossWebhook just returns { accepted: true, received: payload } — literally no validation happens. This is exploitable; anyone can POST arbitrary payloads and they'll be processed.
To fix: implement signature verification using the Toss webhook secret (should be in config), validate timestamps to prevent replay attacks, and switch this endpoint to use express.raw() middleware so HMAC validation happens before body parsing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/routes/billing.ts` around lines 26 - 29, The /webhooks/toss route
currently trusts req.body and must validate authenticity: change the route
definition using express.raw({ type: 'application/json' }) so you can access the
raw buffer, compute the HMAC using the Toss webhook secret from config
(reference the billingRouter.post("/webhooks/toss") handler), verify the
signature header matches the computed HMAC, validate the webhook timestamp
header for freshness and reject replayed timestamps (store recent
nonces/timestamps or use a short allowed window), and only after
signature/timestamp checks parse the raw payload to JSON and call
handleTossWebhook(payload); return 401/400 on failure and include the
signature/timestamp validation logic and secret lookup in a helper used by the
route.
| function createStubSession(seed: string): DesktopSessionResponse { | ||
| return { | ||
| accessToken: `stub-access-${seed}`, | ||
| refreshToken: `stub-refresh-${seed}`, | ||
| expiresIn: SESSION_TTL_SECONDS, | ||
| user: { | ||
| id: `user_${seed}`, | ||
| email: "hello@autoscreen.app", | ||
| displayName: "Auto Screen User", | ||
| }, | ||
| subscription: { | ||
| plan: seed.includes("pro") ? "pro" : "free", | ||
| status: seed.includes("pro") ? "active" : "inactive", | ||
| }, | ||
| entitlements: seed.includes("pro") | ||
| ? ["basic_recording", "basic_editing", "pro_export", "mcp_editing"] | ||
| : ["basic_recording", "basic_editing"], | ||
| }; |
There was a problem hiding this comment.
Blocker: desktop auth still mints caller-controlled stub sessions.
These handlers never validate the exchange code or refresh token; they just feed user input into createStubSession(). That means a made-up code containing "pro" gets a pro subscription and entitlements back, which is kinda cursed for a real auth route. This needs to hard-fail or call the real session backend before release.
Also applies to: 323-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/services/auth-service.ts` around lines 184 - 201, The
createStubSession implementation currently mints caller-controlled sessions and
must not be used by real auth handlers; update the handlers that call
createStubSession (and any code paths at the other locations mentioned) to
validate the incoming exchange code or refresh token against the real session
backend or to hard-fail unless running in a non-production dev mode. Concretely:
remove or gate use of createStubSession behind an explicit DEV_STUB_SESSIONS
flag/env check, implement proper verification by calling the real session
creation/refresh endpoints (or return a 401/400 on invalid tokens) in the
exchange/refresh handlers that currently feed input into createStubSession, and
ensure the returned type still conforms to DesktopSessionResponse while denying
any caller-controlled escalation of subscription/entitlements.
| const code = String(Math.floor(Math.random() * 900000) + 100000); | ||
| const now = new Date().toISOString(); | ||
| const expiresAt = new Date(Date.now() + config.phoneVerificationCodeTtlMs).toISOString(); | ||
| const smsResult = await sendVerificationSms(phoneNumber, code); | ||
| if (!smsResult.success && !smsResult.previewCode) { | ||
| throw new Error(smsResult.error || "문자 인증 코드를 발송하지 못했습니다."); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -type f -name "auth-service.ts" | head -20Repository: siddharthvaddem/openscreen
Length of output: 108
🏁 Script executed:
# Once we find it, let's look at the specific lines and surrounding context
sed -n '385,410p' backend/src/services/auth-service.tsRepository: siddharthvaddem/openscreen
Length of output: 942
🏁 Script executed:
# Search for other uses of Math.random() in the services directory
rg "Math\.random" backend/src/services/ -nRepository: siddharthvaddem/openscreen
Length of output: 294
🏁 Script executed:
# Check for existing crypto usage in the file
rg "crypto\.|randomInt|getRandomValues" backend/src/services/auth-service.ts -nRepository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
# Check package.json for Node.js version requirements
find . -name "package.json" -type f | head -5 | xargs grep -l "node\|engines" 2>/dev/nullRepository: siddharthvaddem/openscreen
Length of output: 107
🏁 Script executed:
# Look at the main package.json
cat package.jsonRepository: siddharthvaddem/openscreen
Length of output: 3848
🏁 Script executed:
# Search for existing crypto imports in the backend
rg "import.*crypto|require.*crypto" backend/ -n | head -20Repository: siddharthvaddem/openscreen
Length of output: 440
🏁 Script executed:
# Check if there's a utility or common folder for shared functions
ls -la backend/src/Repository: siddharthvaddem/openscreen
Length of output: 520
Use a CSPRNG for verification codes—both here and in auth-postgres-service.ts.
Math.random() is lowkey predictable enough to be a real issue for OTPs that gate auth. Use crypto.randomInt(100000, 999999) instead. Also found the same vuln at backend/src/services/auth-postgres-service.ts:211—kinda important that both get fixed since they're doing the same thing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/services/auth-service.ts` around lines 394 - 399, Replace the
insecure Math.random() OTP generation with a CSPRNG: import { randomInt } from
"crypto" and replace the code assignment (the variable named code that currently
uses Math.random) with code = String(randomInt(100000, 1000000)); do the same
replacement for the identical generation in auth-postgres-service.ts (the other
variable named code around the phone verification logic) and ensure both modules
import randomInt from "crypto" and preserve the existing SMS sending and error
handling (sendVerificationSms and related checks).
| export async function restoreDesktopAuthSession() { | ||
| const stored = await readDesktopAuthSession(); | ||
| if (!stored) return null; | ||
| if (isDesktopAuthSessionExpired(stored)) { | ||
| return stored; | ||
| } | ||
| return stored; | ||
| } |
There was a problem hiding this comment.
Expired sessions are restored as if they're valid.
restoreDesktopAuthSession() returns stored in both branches, so expiration never actually gates anything. That means the app can boot into signed_in with stale credentials and only fail later when an API call trips over it.
straightforward fix
export async function restoreDesktopAuthSession() {
const stored = await readDesktopAuthSession();
if (!stored) return null;
if (isDesktopAuthSessionExpired(stored)) {
- return stored;
+ await clearDesktopAuthSession();
+ return null;
}
return stored;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/auth/protocol.ts` around lines 127 - 134, restoreDesktopAuthSession
currently returns the stored session even when expired because both branches
return stored; change the logic in restoreDesktopAuthSession to return null (or
remove/clear the stored session) when isDesktopAuthSessionExpired(stored) is
true so expired sessions are not treated as valid; locate the function
restoreDesktopAuthSession which calls readDesktopAuthSession and
isDesktopAuthSessionExpired and update the conditional to return null (and
optionally call the deletion/clear function for persisted sessions) when
expired.
| app.on("open-url", (event, url) => { | ||
| event.preventDefault(); | ||
| pendingProtocolUrl = url; | ||
| void handleDesktopProtocolUrl(url, editorWindow); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Electron on macOS, can the app "open-url" event fire before app.whenReady when launched via a custom protocol, and is deferring deep-link handling until ready the recommended pattern?
💡 Result:
Yes. On macOS, Electron can emit app’s "open-url" event before the app is “ready” (and therefore before await app.whenReady() resolves) when the app is launched via a custom protocol. Electron’s own docs warn that if you only register the "open-url" listener in/after "ready", you can miss the launch URL. [1]
Recommended pattern on macOS is:
- Register
"open-url"as early as possible during startup (commonly at top-level inmain.js, or in the"will-finish-launching"phase on macOS), so you never miss the initial deep link. [1][2] - Defer actual deep-link processing until after
app.whenReady(), typically by caching/queuing the URL(s) and then handling them once your windows / app state are initialized.
This “capture early, handle when ready” approach is consistent with Electron’s guidance that most app work happens at "ready", while certain macOS launch-time events must be subscribed to earlier. [1][2]
Sources:
[1] Electron app docs — "open-url" event note about missing URLs if registered on "ready": (electronjs.org)
[2] Electron app docs — "will-finish-launching" timing on macOS vs "ready": (electronjs.org)
Citations:
🏁 Script executed:
# First, locate the main.ts file in electron directory
fd -t f main.tsRepository: siddharthvaddem/openscreen
Length of output: 99
🏁 Script executed:
# Read the context around lines 435-439
sed -n '430,445p' electron/main.ts | cat -nRepository: siddharthvaddem/openscreen
Length of output: 605
🏁 Script executed:
# Read the context around lines 832-835
sed -n '825,845p' electron/main.ts | cat -nRepository: siddharthvaddem/openscreen
Length of output: 511
🏁 Script executed:
# Search for handleDesktopProtocolUrl function signature
rg "handleDesktopProtocolUrl" -A 5 -B 2Repository: siddharthvaddem/openscreen
Length of output: 1846
🏁 Script executed:
# Check if there's any protection against duplicate processing
rg "pendingProtocolUrl" -B 2 -A 2Repository: siddharthvaddem/openscreen
Length of output: 1107
Process protocol callbacks exactly once.
open-url stores pendingProtocolUrl and immediately calls handleDesktopProtocolUrl(), but on macOS cold launch it can fire before app.whenReady() resolves. Then the ready handler replays the same handler if the URL's still pending—so the deep link gets processed twice. kinda cursed.
🐛 One clean way to serialize deep-link handling
+async function flushPendingProtocolUrl() {
+ if (!pendingProtocolUrl) return;
+ const url = pendingProtocolUrl;
+ pendingProtocolUrl = null;
+ await handleDesktopProtocolUrl(url, editorWindow);
+}
+
app.on("open-url", (event, url) => {
event.preventDefault();
pendingProtocolUrl = url;
- void handleDesktopProtocolUrl(url, editorWindow);
+ if (app.isReady()) {
+ void flushPendingProtocolUrl();
+ }
});
...
- if (pendingProtocolUrl) {
- await handleDesktopProtocolUrl(pendingProtocolUrl, editorWindow);
- pendingProtocolUrl = null;
- }
+ await flushPendingProtocolUrl();Also applies to: will-finish-launching handler (similar pattern with deepLink)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/main.ts` around lines 435 - 439, The open-url handler currently sets
pendingProtocolUrl and immediately calls handleDesktopProtocolUrl causing
duplicate processing when the app's ready handler replays pendingProtocolUrl;
change the logic so open-url only sets pendingProtocolUrl if not already set and
does not call handleDesktopProtocolUrl when the app is not ready, or else
atomically clear pendingProtocolUrl before invoking handleDesktopProtocolUrl so
it cannot be replayed; update the app.whenReady()/will-finish-launching code
that replays deepLink to check-and-clear the same pendingProtocolUrl (or use a
processed flag) so each URL is handled exactly once, referencing the
app.on("open-url") listener, pendingProtocolUrl, handleDesktopProtocolUrl,
app.whenReady(), will-finish-launching and deepLink symbols.
|
Closing this PR because the changes were pushed to a public fork/release path by mistake and need to be moved to a private commercial deployment workflow. |
1 participant
Summary\n- ship desktop auth, local MCP control, and release automation updates\n- add GitHub tag-based cross-platform release publishing\n- include release cleanup/checklist/docs updates\n\n## Validation\n- npm run lint\n- npm test\n- npm run build-vite\n
Summary by CodeRabbit
New Features
Chores