feat(desktop): add Linux build pipeline and stabilize dev auth callba…#1477
Conversation
…ck flow Enable Linux AppImage build/release artifacts and Linux updater support, and make desktop OAuth callback reliable in Linux development by using a localhost callback fallback with matching CORS/trusted-origin updates. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Linux desktop build/release support and artifact stabilization; introduces a desktop automation MCP package with CDP/puppeteer tools; expands desktop OAuth local-callback plumbing and CORS dev origins; adds workspace branding (dock icon/title), port mapping and static-ports subscription, migrations, and various desktop packaging, docs, and UI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant DA as Desktop App
participant API as API Server
participant WEB as Web App
participant NS as Notifications Server
participant BW as Browser Window
DA->>API: GET /auth/desktop/connect?local_callback=<url>
API->>API: validate local_callback (scheme=http, host=localhost|127.0.0.1, path=/auth/callback)
API-->>DA: redirect to Web success URL with desktop_local_callback param
WEB->>WEB: parse desktop_local_callback → localCallbackUrl
WEB->>NS: GET localCallbackUrl (browser hits local notifications server)
NS->>NS: handle /auth/callback (validate token/expiresAt/state, call handleAuthCallback)
NS->>BW: focus/restore main window
NS-->>WEB: 200 OK (success page)
WEB->>DA: navigate to localCallbackUrl (or fallback deep-link if failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/app/auth/desktop/success/page.tsx`:
- Around line 83-85: localCallbackUrl is built by concatenating strings which
breaks when localCallbackBase already contains query params; instead, construct
the URL with the URL API: if localCallbackBase is defined, create const url =
new URL(localCallbackBase) (or new URL(localCallbackBase,
window.location.origin) if needed), then use url.searchParams.set('token',
token), url.searchParams.set('expiresAt', expiresAt.toISOString()), and
url.searchParams.set('state', state), and finally set localCallbackUrl =
url.toString(); keep the undefined fallback when localCallbackBase is falsy.
This updates the logic around the localCallbackBase/localCallbackUrl variables
to robustly handle existing query parameters.
🧹 Nitpick comments (6)
apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
91-94: Thelocal_callbackis sent unconditionally — is this intentional for production?This appends the localhost callback URL to every OAuth flow, including production builds. If the intent is a fallback only for Linux/dev environments where custom URI handlers are unreliable, consider gating this behind a condition (e.g., platform check or dev-mode flag) to avoid leaking internal infrastructure details in the query string for macOS/Windows users where deep links work reliably.
Also, the
/auth/callbackpath is duplicated here and innotifications/server.ts— consider extracting it to a shared constant.apps/desktop/src/main/lib/notifications/server.ts (2)
181-211: Add prefixed console logging for the auth callback route.This is a significant operation (OAuth callback completion) with no logging. Per coding guidelines, entry/exit of significant operations and error conditions should have
[domain/operation] messagelogging. This will be valuable for debugging auth issues in Linux/dev environments.♻️ Suggested logging additions
app.get("/auth/callback", async (req, res) => { const token = req.query.token; const expiresAt = req.query.expiresAt; const state = req.query.state; if ( typeof token !== "string" || typeof expiresAt !== "string" || typeof state !== "string" ) { + console.warn("[auth/callback] Missing or invalid auth params"); return res.status(400).json({ success: false, error: "Missing auth params" }); } const result = await handleAuthCallback({ token, expiresAt, state }); if (!result.success) { + console.warn("[auth/callback] Auth callback failed:", result.error); return res.status(400).json(result); } + console.log("[auth/callback] Auth callback succeeded, focusing main window"); const mainWindow = BrowserWindow.getAllWindows()[0];As per coding guidelines: "Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions."
196-198: Unhandled rejection ifhandleAuthCallbackthrows unexpectedly.While Express 5 propagates async errors,
handleAuthCallbackcould theoretically throw (e.g., ifsaveTokenfails with a filesystem error). The error would surface as a generic 500 with no context. Consider wrapping in try/catch for a more informative error response.🛡️ Defensive error handling
- const result = await handleAuthCallback({ token, expiresAt, state }); - if (!result.success) { - return res.status(400).json(result); - } + let result: { success: boolean; error?: string }; + try { + result = await handleAuthCallback({ token, expiresAt, state }); + } catch (err) { + console.error("[auth/callback] Unexpected error:", err); + return res.status(500).json({ success: false, error: "Internal auth error" }); + } + if (!result.success) { + return res.status(400).json(result); + }apps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsx (1)
19-36: Extract retry constants to named values.The retry count (
6) and delay (250) are magic numbers. Extracting them improves readability and makes tuning easier.♻️ Suggested refactor
+"use client"; + +import Image from "next/image"; +import Link from "next/link"; +import { useEffect, useState } from "react"; + +const LOCAL_CALLBACK_MAX_ATTEMPTS = 6; +const LOCAL_CALLBACK_RETRY_DELAY_MS = 250; + export function DesktopRedirect({Then reference the constants in the loop:
- for (let attempt = 0; attempt < 6; attempt++) { + for (let attempt = 0; attempt < LOCAL_CALLBACK_MAX_ATTEMPTS; attempt++) { ... - await new Promise((resolve) => window.setTimeout(resolve, 250)); + await new Promise((resolve) => window.setTimeout(resolve, LOCAL_CALLBACK_RETRY_DELAY_MS));As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic".
apps/api/src/proxy.ts (1)
5-18: Dev-only CORS origins are properly gated — consider extracting the shared definition.The development guard is correct and the dual localhost/127.0.0.1 coverage is appropriate. However,
desktopDevPortanddesktopDevOriginsare duplicated inpackages/auth/src/server.ts(per the summary). If the port or allowed hostnames ever change, both files must be updated in lockstep.Consider extracting the shared origin list to a small utility in
packages/constantsor similar, so both consumers reference the same source of truth..github/workflows/build-desktop.yml (1)
144-237: Consider extracting shared steps to a composite action to reduce duplication.The steps for checkout, Bun setup, caching, dependency install, version suffix, clean, and compile are identical between
build-macosandbuild-linux. If a third platform is added or these steps change, both jobs must be updated. A composite action could reduce this surface. This is a common trade-off in GHA and acceptable to defer.
Add release-script checks for jq and robust workflow run lookup by tag SHA, print Linux AppImage download URL, and include lint formatting cleanups in updated desktop auth callback files. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/notifications/server.ts`:
- Around line 183-213: The async Express route handler for
app.get("/auth/callback") currently awaits handleAuthCallback without error
handling; wrap the entire handler body in a try/catch, call await
handleAuthCallback(...) inside the try, and in the catch log the error with
contextual info (including the state/token/expiresAt if safe) and return a 500
JSON response (e.g., { success: false, error: "Internal server error" }); ensure
you still perform the BrowserWindow restore/show/focus only on success, and do
not let rejections from handleAuthCallback or underlying functions like
saveToken bubble unhandled.
🧹 Nitpick comments (3)
apps/desktop/create-release.sh (1)
182-185: Install hint is Linux-specific.The error message only suggests
sudo apt install jq, but macOS developers (likely the majority for a desktop app) would needbrew install jq. Consider a more generic hint.Suggested improvement
- error "jq is required but not installed.\nInstall it with your package manager (e.g. sudo apt install jq)" + error "jq is required but not installed.\nInstall it with your package manager (e.g. brew install jq or sudo apt install jq)"apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
91-94: Extract the callback path to a shared constant to avoid duplication.The path
/auth/callbackis hardcoded here and also inapps/desktop/src/main/lib/notifications/server.ts(Line 183). Consider extracting it to a constant inshared/constantsso both usages stay in sync.apps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsx (1)
18-56: Consider aborting in-flight fetch requests on cleanup.The
isCancelledflag prevents state updates after unmount, but any in-flightfetchcall will still complete. Using anAbortControllerwould properly cancel pending network requests.♻️ Suggested improvement
useEffect(() => { let isCancelled = false; + const controller = new AbortController(); const runRedirect = async () => { if (localCallbackUrl) { for (let attempt = 0; attempt < 6; attempt++) { try { const response = await fetch(localCallbackUrl, { method: "GET", credentials: "omit", + signal: controller.signal, }); if (response.ok && !isCancelled) { setStatus("connected"); return; } } catch { // Retry shortly in case desktop server is still initializing. } await new Promise((resolve) => window.setTimeout(resolve, 250)); if (isCancelled) return; } // ... return () => { isCancelled = true; + controller.abort(); }; }, [url, localCallbackUrl]);
Switch to a namespace zod import so the chunks route schema initializes reliably under the Vitest SSR runtime, unblocking the streams test suite in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the fetch()-based localhost callback with a full browser redirect. Browsers block mixed-content fetch (HTTPS->HTTP) but allow full-page navigations, fixing auth on Linux production builds where the success page is served from https://app.superset.sh. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/scripts/patch-dev-protocol.ts (1)
130-167:⚠️ Potential issue | 🟠 MajorUnsanitized
DISPLAY_NAMEinterpolated into shell commands — breaks on special characters.
DISPLAY_NAME(and transitivelyworkspaceName) is injected directly into PlistBuddy command strings without escaping. A workspace name containing a single quote, double quote, backtick, or$will break theseexecSynccalls or produce corrupt plist values. For example, a worktree directory namedfeature/it's-donewould produceSuperset (it's-done)inside single quotes, breaking the shell parse.Consider sanitizing or constraining
workspaceNameearly in the script (e.g., reject or strip non-alphanumeric/dash/underscore characters), or switch to an array-basedexecFileSyncinvocation that avoids shell interpretation entirely.Proposed fix — use execFileSync to avoid shell
+import { execFileSync } from "node:child_process"; + +function plistBuddy(...cmds: string[]): string { + const args: string[] = []; + for (const cmd of cmds) { + args.push("-c", cmd); + } + args.push(PLIST_PATH); + return execFileSync("/usr/libexec/PlistBuddy", args, { + encoding: "utf-8", + }).trim(); +}Then replace patterns like:
-execSync( - `/usr/libexec/PlistBuddy -c "Set :CFBundleIdentifier ${BUNDLE_ID}" "${PLIST_PATH}"`, -); +plistBuddy(`Set :CFBundleIdentifier ${BUNDLE_ID}`);This avoids shell interpretation of the values entirely.
🤖 Fix all issues with AI agents
In `@apps/desktop/scripts/patch-dev-protocol.ts`:
- Around line 178-211: The unlink/rename/symlink sequence around
ELECTRON_APP_PATH can leave Electron.app broken if unlinkSync(ELECTRON_APP_PATH)
succeeds but a later renameSync or symlinkSync throws; wrap the inner branch
that does unlinkSync, renameSync, and symlinkSync (the block handling
stats.isSymbolicLink() and the else real-directory branch) in its own try/catch
and on failure restore the original symlink target (using symlinkSync with the
prior currentTarget or recreate a symlink to the oldTargetPath) and/or log a
clear recovery instruction; update references to ELECTRON_APP_PATH,
DESIRED_APP_NAME, currentTarget, oldTargetPath, renameSync and symlinkSync in
that block so the code attempts to recover the original state before rethrowing
or continuing.
In `@apps/desktop/src/lib/electron-app/factories/app/setup.ts`:
- Around line 85-91: Replace the raw process.env access and hardcoded fallback
with the Zod-validated value from the shared env module: use
env.DESKTOP_AUTOMATION_PORT (or the already-imported env object) when computing
cdpPort inside the development block in setup.ts instead of
String(process.env.DESKTOP_AUTOMATION_PORT || 9223); also extract the numeric
default (9223) to a named constant at the top of the module (e.g.,
DEFAULT_DESKTOP_AUTOMATION_PORT) and use that only if
env.DESKTOP_AUTOMATION_PORT is undefined, then pass the computed cdpPort to
app.commandLine.appendSwitch as before.
In `@apps/desktop/src/main/lib/dock-icon.ts`:
- Around line 61-74: The third branch in getIconPath uses __dirname which can be
unreliable with electron-vite bundling in non-packaged, non-development runs;
add a concise comment above that return explaining this branch only applies to
uncommon test/preview builds and why __dirname is used (and that electron-vite
may change its value), and optionally replace or guard the fallback with a more
robust resolution (e.g., prefer app.getAppPath() or check process.resourcesPath
first) so the path resolves consistently under electron-vite; reference
getIconPath and the __dirname return when making the change.
- Around line 107-129: The findContentBounds function can return inverted bounds
when no pixel exceeds the alpha threshold; update findContentBounds to detect
that case (e.g., if top > bottom or left > right after the loops) and return a
safe empty rectangle (top=0,left=0,bottom=0,right=0 or other sentinel) so
downstream callers like drawBorder and setWorkspaceDockIcon never receive
inverted coordinates; reference findContentBounds and ensure callers (including
setWorkspaceDockIcon/icon.isEmpty logic) handle the normalized empty bounds
consistently.
In `@apps/desktop/src/main/lib/local-db/index.ts`:
- Around line 90-96: The try/catch around migrate(localDb, { migrationsFolder })
currently swallows all errors and always logs "[local-db] Migrations complete";
change it to only suppress known idempotent SQLite migration errors (e.g., “no
such table/column” or other benign codes your migration lib documents) while
logging full error context via processLogger or console.error, and rethrow any
unknown/unexpected errors so failures don't allow the app to continue with an
incomplete schema; also move the success log ("[local-db] Migrations complete")
inside the successful path (after migrate returns) so it is only emitted on
success. Ensure you reference the migrate call and the catch block around it
when implementing this behavior.
In `@packages/db/drizzle/0018_sandbox_and_slack_users.sql`:
- Around line 65-69: The migration drops the "repositories" table with DROP
TABLE "repositories" CASCADE which already removes referencing foreign keys,
then tries to drop the same constraint tasks_repository_id_repositories_id_fk
and index tasks_repository_id_idx causing a "constraint does not exist" error;
fix by either removing the explicit DROP CONSTRAINT
"tasks_repository_id_repositories_id_fk" and DROP INDEX
"tasks_repository_id_idx" lines, or reorder the statements so that ALTER TABLE
"tasks" DROP CONSTRAINT "tasks_repository_id_repositories_id_fk" and DROP INDEX
"tasks_repository_id_idx" occur before DROP TABLE "repositories" CASCADE.
In `@packages/desktop-mcp/src/mcp/connection/connection-manager.ts`:
- Around line 21-27: getPage() can start multiple simultaneous connect() calls
when this.page is unset; add an in-flight promise guard (e.g. a private member
like connectingPromise) to serialize connection creation: before calling
connect() check if connectingPromise exists and await it, otherwise set
connectingPromise = connect(), await it, then clear connectingPromise; ensure
getPage() still calls focusLock.inject(this.page) when the page is available and
that connect() resolves to and assigns this.page/browser so subsequent calls see
the established connection.
In `@packages/desktop-mcp/src/mcp/dom-inspector/dom-inspector.ts`:
- Around line 45-46: The code constructs cssSelector using testId without
escaping which can produce malformed selectors; update the branch that sets
cssSelector (the else if handling testId) to pass testId through
CSS.escape()—mirroring the earlier use for node.id—so cssSelector becomes an
attribute selector built from the escaped testId value.
In `@packages/desktop-mcp/src/mcp/focus-lock/focus-lock.ts`:
- Around line 57-65: The load event handler in attach(page) uses an async
callback and calls this.inject(page) without guarding against rejection; wrap
the await this.inject(page) call in a try/catch inside the page.on("load", ...)
listener, and on error call the module logger (or processLogger) with contextual
information including the page and that inject failed (do not swallow the error
silently) then either rethrow or handle per local error policy; ensure you
reference the attach method and the this.inject(page) invocation when making the
change.
In `@packages/desktop-mcp/src/mcp/tools/click/click.ts`:
- Around line 14-16: The selector-building for testId in click.ts is vulnerable
because testId is interpolated directly into '[data-testid="' + testId + '"]';
update the code that assigns el (the document.querySelectorAll call) to use a
sanitized selector by applying CSS.escape to testId before interpolation (i.e.,
use CSS.escape(testId) when constructing the '[data-testid="..."]' selector) so
querySelectorAll('[data-testid="' + CSS.escape(testId) + '"]')[index] cannot
break or match unintended elements.
In `@packages/desktop-mcp/src/mcp/tools/evaluate-js/evaluate-js.ts`:
- Around line 22-25: The text field can become undefined because
JSON.stringify(undefined) returns undefined; update the logic in evaluate-js.ts
around the result handling (the variable used to set text in the returned
content) to explicitly handle undefined before calling JSON.stringify — e.g. if
result === undefined set text to the string "undefined" (or use String(result)),
otherwise if typeof result === "string" use result, else use
JSON.stringify(result, null, 2) and if that returns undefined fall back to
String(result); adjust the code that builds the content/text to follow this
order so text is always a string.
In `@packages/desktop-mcp/src/mcp/tools/type-text/type-text.ts`:
- Around line 29-33: The clear-first logic in the type routine uses a hardcoded
Meta modifier (page.keyboard.down("Meta") / press("a") / up("Meta")) which only
works on macOS; update the args.clearFirst branch in the type-text logic to
detect the OS (e.g., check process.platform === "darwin") and choose the
modifier variable accordingly (use "Meta" for macOS, "Control" for
Linux/others), then use that modifier when calling page.keyboard.down(mod),
page.keyboard.press("a"), and page.keyboard.up(mod) so Select All works
cross-platform.
🧹 Nitpick comments (29)
apps/desktop/src/main/terminal-host/session-lifecycle.test.ts (1)
367-368: All tests in this file are now skipped — consider tracking re-enablement.With these two being the last active tests, every test case in this suite is now
it.skip. This is reasonable for unblocking Linux CI, but without a tracking issue the skips tend to become permanent. Consider adding aTODO(issue-url)linking to an issue for fixing the bun/node-pty ABI mismatch so these can be re-enabled.Also applies to: 404-405
apps/desktop/src/lib/trpc/routers/ports/ports.ts (2)
190-216: Subscription captures workspace list only at subscribe-time; late additions won't be watched.
allWorkspacesis queried once when the subscription starts (line 192). Any workspace created after the subscription is established won't be watched, so its static-port file changes will be silently missed until the client re-subscribes.If this is acceptable for your use-case (e.g., the UI re-mounts or the user navigates), no action needed. Otherwise, consider listening to a workspace-creation event and dynamically adding watches.
203-205:onChangeemits for any workspace change, not just the ones this subscription watches.The
staticPortsWatcheris a shared singleton. If anothersubscribeStaticcall watches an additional workspace, thisonChangehandler will also fire for that workspace since there's no filter againstwatchedIds. The consumer currently just invalidates the fullgetAllStaticcache, so this is benign, but it's worth noting for future callers that expect scoped events..superset/teardown.sh (1)
120-128:((killed++))post-increment returns exit 1 on first invocation — harmless here but worth knowing.When
killedis0,((killed++))evaluates to0(the old value), which bash treats as falsy (exit code 1). The|| truesaves you, but this is a classic bash gotcha that could trip someone up if the|| trueis ever removed. Using pre-increment (++killed) avoids the ambiguity since the result is always ≥ 1.Suggested tweak
- kill "$pid" 2>/dev/null && ((killed++)) || true + kill "$pid" 2>/dev/null && ((++killed)) || true.superset/setup.sh (1)
189-202: Copying a live SQLite database withcpmay produce an inconsistent snapshot.If the source database is actively being written to (e.g., by another dev workspace),
cpcan capture a mid-transaction state where the main DB file and WAL are out of sync. Consider usingsqlite3 "$source_db" ".backup '$dest_db'"for an atomic, consistent copy. WAL/SHM files won't need separate handling with.backup.Suggested fix
- if ! cp "$source_db" "$dest_db"; then - error "Failed to copy local database" - return 1 - fi - chmod 600 "$dest_db" - - # Copy WAL and SHM files if present for consistency - for suffix in "-wal" "-shm"; do - if [ -f "${source_db}${suffix}" ]; then - cp "${source_db}${suffix}" "${dest_db}${suffix}" 2>/dev/null || true - chmod 600 "${dest_db}${suffix}" 2>/dev/null || true - fi - done + if command -v sqlite3 &> /dev/null; then + if ! sqlite3 "$source_db" ".backup '$dest_db'"; then + error "Failed to clone local database via sqlite3 backup" + return 1 + fi + else + warn "sqlite3 not found — falling back to file copy (may be inconsistent if DB is active)" + if ! cp "$source_db" "$dest_db"; then + error "Failed to copy local database" + return 1 + fi + for suffix in "-wal" "-shm"; do + if [ -f "${source_db}${suffix}" ]; then + cp "${source_db}${suffix}" "${dest_db}${suffix}" 2>/dev/null || true + chmod 600 "${dest_db}${suffix}" 2>/dev/null || true + fi + done + fi + chmod 600 "$dest_db"packages/desktop-mcp/src/mcp/tools/navigate/navigate.ts (1)
18-47: Unhandled navigation errors will surface as raw exceptions to the MCP client.If
page.goto()fails (timeout, net error, invalid URL), the raw Puppeteer error propagates. Wrapping in a try/catch would let you return a structuredisError: trueresponse consistent with the "no url/path" branch.Also, the
as stringcast on line 22 is unnecessary — the truthy check already narrowsstring | undefinedtostring.Suggested improvement
async (args) => { - const page = await getPage(); - - if (args.url) { - await page.goto(args.url as string); - } else if (args.path) { - await page.evaluate( - `window.location.hash = ${JSON.stringify(`#${args.path}`)}`, - ); - } else { + if (!args.url && !args.path) { return { content: [ - { - type: "text" as const, - text: "Must provide url or path", - }, + { type: "text" as const, text: "Must provide url or path" }, ], isError: true, }; } - const currentUrl = page.url(); - return { - content: [ - { - type: "text" as const, - text: `Navigated to ${currentUrl}`, - }, - ], - }; + try { + const page = await getPage(); + + if (args.url) { + await page.goto(args.url); + } else if (args.path) { + await page.evaluate( + `window.location.hash = ${JSON.stringify(`#${args.path}`)}`, + ); + } + + return { + content: [ + { type: "text" as const, text: `Navigated to ${page.url()}` }, + ], + }; + } catch (err) { + return { + content: [ + { + type: "text" as const, + text: `Navigation failed: ${err instanceof Error ? err.message : String(err)}`, + }, + ], + isError: true, + }; + }packages/desktop-mcp/src/mcp/console-capture/console-capture.ts (1)
40-47:getLogsreturns a direct reference to the internallogsarray when no filters are applied.If
levelis undefined andlimitis undefined,filteredis the same reference asthis.logs. A caller doingresult.push(...)orresult.length = 0would corrupt internal state. Consider returning a shallow copy.Suggested fix
getLogs({ level, limit, }: { level?: number; limit?: number; }): ConsoleLogEntry[] { - let filtered = this.logs; + let filtered: ConsoleLogEntry[] = this.logs; if (level !== undefined) { filtered = filtered.filter((log) => log.level === level); } if (limit !== undefined) { filtered = filtered.slice(-limit); } + // Return a copy if no filter/limit produced a new array + if (filtered === this.logs) { + filtered = [...filtered]; + } return filtered; }packages/desktop-mcp/src/mcp/focus-lock/focus-lock.ts (1)
85-89: Silent catch should log for debuggability.Per coding guidelines, errors should not be swallowed silently. A debug-level log would help troubleshoot automation issues without adding noise.
Suggested fix
try { await page.evaluate(UNLOCK_SCRIPT); } catch { - // page may have navigated or been destroyed + console.debug("[FocusLock] unlock failed — page may have navigated or been destroyed"); }packages/desktop-mcp/src/mcp/connection/connection-manager.ts (1)
29-58:connect()does not handle errors — a failed connection leaves no path to retry.If
puppeteer.connect()or page lookup throws, internal state staysnullwhich is fine, but the error propagates unlogged. Per coding guidelines, errors should at minimum be logged with context before rethrowing. Also, theprotocolTimeoutof 60 s is an inline magic number.Suggested improvement
+const PROTOCOL_TIMEOUT_MS = 60_000; + private async connect(): Promise<Page> { - this.browser = await puppeteer.connect({ - browserURL: `http://127.0.0.1:${CDP_PORT}`, - protocolTimeout: 60_000, - defaultViewport: null, - }); + try { + this.browser = await puppeteer.connect({ + browserURL: `http://127.0.0.1:${CDP_PORT}`, + protocolTimeout: PROTOCOL_TIMEOUT_MS, + defaultViewport: null, + }); + } catch (err) { + console.error("[desktop-mcp/connect] CDP connection failed", err); + throw err; + }packages/desktop-mcp/src/zod.ts (2)
47-50:NavigateRequestSchemaallows an empty object — is that intentional?Both
urlandpathare optional, so{}passes validation. If the tool requires at least one, add a refinement:export const NavigateRequestSchema = z.object({ url: z.string().optional(), path: z.string().optional(), }).refine(d => d.url || d.path, { message: "Either url or path is required" });If an empty payload is valid (e.g., "return current URL"), ignore this.
110-116:ConsoleLogEntrySchema.levelisz.number()whileConsoleLogsRequestSchema.levelis a string enum.The request filters by
"log" | "warn" | "error" | "info" | "debug"but the response entry storeslevelas a number. This is likely correct (CDP uses numeric log levels internally), but consumers will need to map between the two representations. Consider documenting the mapping or using a consistent representation.packages/desktop-mcp/src/bin.ts (1)
5-7: No error handling around startup — an unhandled rejection crashes silently.If
createMcpServer()orserver.connect()throws, the process exits with an opaque unhandled-rejection trace. A minimal wrapper improves debuggability:Suggested improvement
-const server = createMcpServer(); -const transport = new StdioServerTransport(); -await server.connect(transport); +try { + const server = createMcpServer(); + const transport = new StdioServerTransport(); + await server.connect(transport); +} catch (err) { + console.error("[desktop-mcp] Failed to start MCP server", err); + process.exit(1); +}packages/desktop-mcp/src/mcp/tools/evaluate-js/evaluate-js.ts (1)
14-39: Missing error handling forgetPage()failure.If the CDP connection is down,
getPage()will throw, but it's called outside thetry/catchblock. This would propagate as an unhandled error to the MCP framework rather than returning a user-friendly error response.Move `getPage()` inside the try block
async (args) => { - const page = await getPage(); try { + const page = await getPage(); const result = await page.evaluate(args.code as string);packages/desktop-mcp/src/mcp/tools/take-screenshot/take-screenshot.ts (2)
11-21: Consider adding.positive()constraints onwidthandheight.Zero or negative values for
width/heightwould cause puppeteer'sscreenshot()to throw a confusing error. Adding validation at the schema level gives a clearer error message.Proposed schema tightening
rect: z .object({ x: z.number().describe("X coordinate of capture region"), y: z.number().describe("Y coordinate of capture region"), - width: z.number().describe("Width of capture region"), - height: z.number().describe("Height of capture region"), + width: z.number().positive().describe("Width of capture region"), + height: z.number().positive().describe("Height of capture region"), })
24-47: No error handling —getPage()orpage.screenshot()failures will propagate unhandled.Other tools like
evaluate_jswrap the handler in try/catch and returnisError: true. Screenshots can fail for various reasons (page not ready, invalid clip region). Consider wrapping for consistency and a better agent experience.packages/desktop-mcp/src/mcp/tools/inspect-dom/inspect-dom.ts (2)
36-64: Consider wrapping the handler in try/catch for robustness.If the DOM inspector script throws (e.g., invalid selector passed by the agent), the error propagates unhandled. A try/catch returning
isError: truewould give the agent actionable feedback, consistent with howevaluate_jshandles errors.
5-15:DomElementinterface could be shared.This interface represents the contract of
DOM_INSPECTOR_SCRIPT's output. If it's also used or referenced elsewhere (e.g., indom-inspector.ts), consider co-locating it with the script to keep the contract in one place.packages/desktop-mcp/src/mcp/tools/click/click.ts (2)
77-87: Coordinate click succeeds even when only one ofx/yis provided.If a caller passes
xwithouty(or vice versa), the coordinate branch is skipped and the tool falls through to the element-search path with all-null selectors, returning a confusing "Element not found" error instead of a clear validation message.Proposed fix
// Click by coordinates - if (args.x !== undefined && args.y !== undefined) { + const hasX = args.x !== undefined; + const hasY = args.y !== undefined; + if (hasX !== hasY) { + return { + content: [ + { type: "text" as const, text: "Both x and y coordinates are required" }, + ], + isError: true, + }; + } + if (hasX && hasY) { await page.mouse.click(args.x as number, args.y as number);
90-96: Redundant?? 0/?? true— Zod.default()guarantees these values.
indexandfuzzyhave.default()in the input schema, so they will never beundefinedafter parsing. The nullish coalescing is dead code.packages/desktop-mcp/src/mcp/tools/send-keys/send-keys.ts (2)
72-75: Pressing a held-down modifier again may produce unexpected double key events.When only modifiers are provided (e.g.,
["Shift"]), the code doesdown("Shift")thenpress("Shift")(which isdown+up) thenup("Shift"). This results in twokeydownand twokeyupevents for the same key, which could confuse some applications. Consider whether this branch is truly needed — if the intent is "just press the modifier once", skip thedown/upwrapper and only callpress.
77-79:modifiers.reverse()mutates the array in place.
Array.prototype.reverse()mutates the original array. This is safe here sincemodifiersisn't reused, but using[...modifiers].reverse()ormodifiers.toReversed()would be clearer about intent and safer against future refactors.apps/desktop/scripts/patch-dev-protocol.ts (3)
108-126: Emptycatch {}blocks silently swallow errors.Lines 112 and 126 have bare
catch {}that hide failures without any logging. The outer catch (line 126) guards the "already patched?" check — a failure there means the script falls through to re-patch, which is acceptable behavior, but a debug log would help troubleshooting. The inner catch (line 112) onreadFileSyncsimilarly hides I/O errors.At minimum, log a debug-level message so failures are diagnosable. As per coding guidelines: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."
Proposed fix
try { pathTxtCorrect = readFileSync(pathTxtCheck, "utf-8").trim() === `${DISPLAY_NAME}.app/Contents/MacOS/Electron`; - } catch {} + } catch (err) { + console.log("[patch-dev-protocol] Could not read path.txt, will re-patch:", err); + } if ( currentBundleId === BUNDLE_ID && @@ ... process.exit(0); } -} catch {} +} catch (err) { + console.log("[patch-dev-protocol] Could not read current plist state, will re-patch:", err); +}
102-106:electronPkgDirpath is computed twice.
electronPkgCheck(line 102) andelectronPkgDir(line 230) resolve the same../node_modules/electronpath. Extract it as a top-level constant alongsideELECTRON_DIST_DIRto avoid duplication.Proposed fix
+const ELECTRON_PKG_DIR = resolve(import.meta.dirname, "../node_modules/electron"); const ELECTRON_DIST_DIR = resolve( import.meta.dirname, - "../node_modules/electron/dist", + ELECTRON_PKG_DIR, + "dist", );Then use
ELECTRON_PKG_DIRin both the check (line 102) and the write (line 230).Also applies to: 230-231
139-144: Another silentcatch {}.Same concern as the earlier empty catches — the
Delete :CFBundleDisplayNamefailure is expected when the key doesn't exist, but logging at debug level would still help distinguish that case from unexpected errors.apps/desktop/src/main/lib/dock-icon.ts (2)
226-233: Extract magic numbers to named constants.Several tuning values are inlined. Per the coding guidelines, hardcoded magic numbers should be extracted to named constants at module top.
♻️ Suggested extraction
+// Dock icon border styling constants +const BORDER_SATURATION = 75; +const BORDER_LIGHTNESS = 55; +const BORDER_THICKNESS_RATIO = 0.038; +const CORNER_RADIUS_RATIO = 0.22; +const CONTENT_ALPHA_THRESHOLD = 10; +const BORDER_ALPHA_THRESHOLD = 0.001; + /** * Sets the macOS dock icon with a colored border based on the workspace name.Then reference them in the body:
- const rgb = hslToRgb(hue, 75, 55); + const rgb = hslToRgb(hue, BORDER_SATURATION, BORDER_LIGHTNESS); - const thickness = Math.round(size.width * 0.038); - const cornerRadius = Math.round(size.width * 0.22); + const thickness = Math.round(size.width * BORDER_THICKNESS_RATIO); + const cornerRadius = Math.round(size.width * CORNER_RADIUS_RATIO);And similarly for the thresholds at lines 119 and 184.
As per coding guidelines: "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic."
80-102:sdfRoundedRectuses 7 positional parameters.Per coding guidelines, functions with 2+ params should use object parameters. That said, this is a standard SDF math primitive where positional args follow established convention. Consider refactoring if this function is reused elsewhere; acceptable as-is for an internal utility.
As per coding guidelines: "Use object parameters for functions with 2 or more parameters instead of positional arguments."
apps/desktop/src/lib/trpc/routers/changes/status.ts (1)
162-162: Silentcatch {}swallows errors without any logging.This bare catch makes it difficult to diagnose file-read failures for untracked files. Consider logging with context at a debug/warn level so issues are at least observable.
Suggested fix
- } catch {} + } catch (err) { + console.warn(`[changes/status] Failed to count lines for untracked file: ${file.path}`, err); + }As per coding guidelines: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.ts (2)
140-176: Inconsistent error-handling pattern:syncBranchreturns result objects while sibling procedures throw.The other procedures in this router (
reorder,update,setUnread,setActive) thrownew Error(...)for invalid states (e.g., workspace not found), butsyncBranchreturns{ success: false, reason: ... }. This forces callers to inspect return values rather than rely on tRPC's standard error propagation, and it means the caller must handle two different error shapes depending on which procedure is called.Consider either aligning with the existing throw pattern (ideally using
TRPCErrorwith appropriate codes likeNOT_FOUND/BAD_REQUEST) or documenting why this procedure intentionally diverges.As per coding guidelines: "Use appropriate TRPCError codes consistently: NOT_FOUND (resource doesn't exist), BAD_REQUEST (invalid input)."
156-160:updatedAtnot refreshed on branch change.The direct
localDb.updatehere doesn't setupdatedAt. The local-db SQLite schema uses$defaultFn(insert-time only), not$onUpdate, so the timestamp won't auto-update. Other mutations in this file usetouchWorkspace(line 91) to handle this. IfupdatedAtaccuracy matters for cache freshness or UI ordering, consider including it:localDb .update(workspaces) - .set({ branch }) + .set({ branch, updatedAt: Date.now() }) .where(eq(workspaces.id, workspaceId)) .run();
| try { | ||
| migrate(localDb, { migrationsFolder }); | ||
| } catch (error) { | ||
| const sqliteError = error as Error & { code?: string }; | ||
| const errorCode = sqliteError.code?.toLowerCase() ?? ""; | ||
| const errorMessage = sqliteError.message?.toLowerCase() ?? ""; | ||
|
|
||
| const isSqliteError = errorCode === "sqlite_error"; | ||
| const isIdempotentMessage = | ||
| errorMessage.includes("duplicate column name") || | ||
| errorMessage.includes("already exists") || | ||
| errorMessage.includes("no such column"); | ||
|
|
||
| if (isSqliteError && isIdempotentMessage) { | ||
| console.log(`[local-db] Skipped idempotent error: ${sqliteError.message}`); | ||
| } else { | ||
| throw error; | ||
| } | ||
| console.error("[local-db] Migration failed:", error); | ||
| } | ||
|
|
||
| console.log("[local-db] Migrations complete"); |
There was a problem hiding this comment.
Swallowing all migration errors risks silent data corruption.
The previous code selectively handled known-idempotent SQLite errors; this change catches everything and continues. If a migration fails due to disk issues, schema corruption, or a genuine bug, the app proceeds with an incomplete schema — likely causing confusing downstream failures. Line 96 also misleadingly logs "Migrations complete" after a failure.
At minimum, re-throw unknown/unexpected errors and only swallow truly idempotent ones. As per coding guidelines: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."
Proposed fix
try {
migrate(localDb, { migrationsFolder });
} catch (error) {
- console.error("[local-db] Migration failed:", error);
+ const message = error instanceof Error ? error.message : String(error);
+ const isIdempotent =
+ message.includes("already exists") ||
+ message.includes("duplicate column name");
+ if (isIdempotent) {
+ console.warn("[local-db] Idempotent migration warning (safe to ignore):", message);
+ } else {
+ console.error("[local-db] Migration failed:", error);
+ throw error;
+ }
}
console.log("[local-db] Migrations complete");📝 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.
| try { | |
| migrate(localDb, { migrationsFolder }); | |
| } catch (error) { | |
| const sqliteError = error as Error & { code?: string }; | |
| const errorCode = sqliteError.code?.toLowerCase() ?? ""; | |
| const errorMessage = sqliteError.message?.toLowerCase() ?? ""; | |
| const isSqliteError = errorCode === "sqlite_error"; | |
| const isIdempotentMessage = | |
| errorMessage.includes("duplicate column name") || | |
| errorMessage.includes("already exists") || | |
| errorMessage.includes("no such column"); | |
| if (isSqliteError && isIdempotentMessage) { | |
| console.log(`[local-db] Skipped idempotent error: ${sqliteError.message}`); | |
| } else { | |
| throw error; | |
| } | |
| console.error("[local-db] Migration failed:", error); | |
| } | |
| console.log("[local-db] Migrations complete"); | |
| try { | |
| migrate(localDb, { migrationsFolder }); | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| const isIdempotent = | |
| message.includes("already exists") || | |
| message.includes("duplicate column name"); | |
| if (isIdempotent) { | |
| console.warn("[local-db] Idempotent migration warning (safe to ignore):", message); | |
| } else { | |
| console.error("[local-db] Migration failed:", error); | |
| throw error; | |
| } | |
| } | |
| console.log("[local-db] Migrations complete"); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/local-db/index.ts` around lines 90 - 96, The
try/catch around migrate(localDb, { migrationsFolder }) currently swallows all
errors and always logs "[local-db] Migrations complete"; change it to only
suppress known idempotent SQLite migration errors (e.g., “no such table/column”
or other benign codes your migration lib documents) while logging full error
context via processLogger or console.error, and rethrow any unknown/unexpected
errors so failures don't allow the app to continue with an incomplete schema;
also move the success log ("[local-db] Migrations complete") inside the
successful path (after migrate returns) so it is only emitted on success. Ensure
you reference the migrate call and the catch block around it when implementing
this behavior.
AviPeltz
force-pushed
the
feat/desktop-linux-build-and-auth-hardening
branch
from
6e46435 to
397b900
Compare
… Linux PR #1477 unconditionally sent local_callback for all platforms, which bypassed the reliable deep link flow on macOS. Only send the localhost callback on Linux where deep links are unreliable, restoring the original macOS auth behavior.
1 participant
…ck flow
Enable Linux AppImage build/release artifacts and Linux updater support, and make desktop OAuth callback reliable in Linux development by using a localhost callback fallback with matching CORS/trusted-origin updates.
Description
Test plan
psqlpooled and unpooled connectivity verified after env fixhttp://localhost:5173 -> /api/auth/get-sessionresolved after allowlist updates*-linux.ymlmanifest uploadAppImage,canary-linux.yml,latest-linux.yml)EOF
)"
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores