Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new Electron-based desktop application under apps/old-desktop: project config, build tooling, a multi-target Vite/Electron setup, main process (IPC, windows, menus), persistence/storage layer and migrations, tmux/terminal/port/proxy subsystems, typed IPC contracts and preload bridge, and a React renderer with contexts, components, and utilities. Also trims/simplifies parts of existing apps/desktop. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant OS
participant Main as ElectronMain
participant Stores as DesktopStores
participant IPC as IPCRegistry
participant WindowMgr as WindowManager
participant Renderer
participant Tmux as TmuxManager
participant PortDet as PortDetector
participant Proxy as ProxyManager
Note right of Main: app startup (old-desktop)
OS->>Main: launch
Main->>Main: requestSingleInstanceLock
Main->>Stores: DesktopStores.initialize()
Stores-->>Main: initialized
Main->>IPC: register*IPCs (workspace/ui/terminal/port/window/deep-link)
IPC-->>Main: handlers registered
Main->>WindowMgr: restoreWindows() / createWindow()
WindowMgr-->>Main: BrowserWindow ready
Main->>Renderer: load renderer (dev server or file)
Renderer->>IPC: invoke("workspace-activate") / invoke("deep-link-get-url")
IPC->>Stores: read/write domain & UI state
Renderer->>IPC: terminal-create / terminal-exec
IPC->>Tmux: spawn/attach terminal
Tmux->>PortDet: startMonitoring(pid)
PortDet-->>IPC: port-detected / port-closed events
IPC->>Proxy: updateTargets(workspace)
Proxy-->>Renderer: proxy-updated (via IPC)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Focus areas during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/menu.ts (1)
17-29: Incomplete Open Repository implementation.The Open Repository dialog is shown and a path is selected, but no actual repository opening logic is implemented. The misleading console.log on Line 28 says "Event sent" but no event is actually sent. This creates a broken user flow where users can select a repository but nothing happens.
Consider one of the following:
- Remove or disable this menu item until the implementation is complete
- Implement the repository opening logic
- Update the console.log to accurately reflect that the feature is not yet implemented
If this is work-in-progress, apply this diff to make the incomplete state clear:
const repoPath = result.filePaths[0]; - console.log("[Menu] Event sent"); + console.log("[Menu] TODO: Implement repository opening for:", repoPath);
🧹 Nitpick comments (49)
apps/old-desktop/.gitignore (1)
1-25: Standard Node/Electron coverage looks solid; consider adding Vite cache patterns.The patterns are comprehensive for a typical Node + Electron + Vite setup. One optional addition: if Vite caching is enabled and you want to exclude the cache directory, add
.vite/. Also consider adding IDE/editor config patterns (.idea/,.vscode/) if your team prefers consistent tooling—though many monorepos commit these for consistency.Optional enhancement:
electron.vite.config.*.mjs + +# Vite cache +.vite/ + +# IDE (optional, depending on team preferences) +.idea/ +.vscode/apps/old-desktop/src/shared/constants.ts (1)
5-9: LGTM! Consider adding an unknown platform indicator.The
PLATFORMconstant correctly identifies the three main desktop operating systems supported by Electron.Optionally, you could add an
IS_UNKNOWNflag or a helper to catch unsupported platforms, though this is unlikely to be an issue in practice:export const PLATFORM = { IS_MAC: process.platform === "darwin", IS_WINDOWS: process.platform === "win32", IS_LINUX: process.platform === "linux", + IS_UNKNOWN: !["darwin", "win32", "linux"].includes(process.platform), };apps/old-desktop/src/renderer/screens/main/types.ts (1)
3-14: Consider using a timestamp type forlastUpdated.The
lastUpdatedfield is typed asstring, which is suitable for display purposes but may complicate sorting, filtering, or date calculations. If you need to perform date operations, consider usingDateor a numeric timestamp alongside the display string.Example refactor if date operations are needed:
export type UITask = { id: string; slug: string; name: string; status: TaskStatus; branch: string; description: string; assignee: string; assigneeAvatarUrl: string; - lastUpdated: string; + lastUpdated: string; // Display string + lastUpdatedTimestamp: number; // For sorting/filtering };apps/old-desktop/src/renderer/screens/main/constants.ts (1)
3-14: Note: External avatar URLs and static timestamps.The mock data uses:
- External avatar URLs from
pravatar.cc- these require network access and could fail if the service is unavailable- Static relative timestamps like "2 hours ago" - these won't update and will become stale
Both are acceptable for mock data marked with a TODO, but consider using local placeholder images or generated avatar initials for offline development reliability.
apps/old-desktop/tsconfig.json (1)
15-15: Remove redundantallowSyntheticDefaultImportsoption.
"allowSyntheticDefaultImports": trueis redundant when"esModuleInterop": trueis already enabled; esModuleInterop subsumes this behavior."esModuleInterop": true, - "sourceMap": true, + "sourceMap": true,Simply remove line 15.
apps/desktop/src/lib/electron-router-dom.ts (1)
3-3: Consider documenting the port choice.The DEFAULT_PORT constant is well-defined, but the specific port number 4927 lacks context. Consider adding a brief comment explaining why this port was chosen or referencing any related configuration.
apps/old-desktop/src/renderer/screens/main/utils.ts (1)
98-98: Consider renaming "lastUpdated" for semantic clarity.The field is named
lastUpdatedbut usesworktree.createdAt, which represents creation time rather than modification time. This naming might confuse developers expecting a "last modified" timestamp.If worktrees are never modified after creation, consider renaming to reflect this:
- lastUpdated: formatRelativeTime(new Date(worktree.createdAt)), + createdRelative: formatRelativeTime(new Date(worktree.createdAt)),Alternatively, if modification tracking is needed, derive
lastUpdatedfrom an actual modification timestamp.apps/old-desktop/src/shared/types.ts (5)
1-12: Clarify coupling toregisterRouteand unify Electron type usageDeriving
RoutefromParameters<typeof registerRoute>[0]keeps window props in sync with the router, but it also tightly couples this shared types module to that specific function signature. IfregisterRoute’s first parameter ever changes shape (or is overloaded),WindowPropswill break even though the window model may not have changed. Consider introducing an explicitRouteinterface exported from your router package, or narrowing this type toPick<Route, "id" | "query">to reduce fragility.Separately,
BrowserWindowOrNullandWindowPropsuse the ambientElectron.*namespace while you also importBrowserWindowfrom"electron". For consistency and to avoid mixing module and ambient namespaces, it may be cleaner to express these as:export type BrowserWindowOrNull = BrowserWindow | null; export interface WindowProps extends BrowserWindowConstructorOptions { /* … */ }(using imported types rather than
Electron.*where possible).
14-18: AlignWindowCreationByIPCnullability with actual usage
window(): BrowserWindowOrNull;allowsnull, butcallback(window: BrowserWindow, event: IpcMainInvokeEvent)assumes a non-null window. That implies a convention thatcallbackmust never be invoked whenwindow()returnsnull, which isn’t expressed in the type system.You could either (a) change the factory to return
BrowserWindow(and handle failures upstream), or (b) make the callback acceptBrowserWindowOrNulland explicitly guard at call sites. Either way, aligning these signatures will make IPC wiring less error-prone.
23-56: Tighten tab discriminated union and avoid triple-statecommandThe
Tabmodel is generally clear, but a couple of points could be tightened:
command?: string | null;gives three possible states (undefined,null, and a string). Unless you actively need both “unset” and “explicitly cleared”, this is likely to add noise to consumers. Prefer eithercommand?: stringorcommand: string | nulland stick to a single way to represent absence.The comment on Line 53 says group tabs “cannot contain nested group tabs”, but the type
tabs?: Tab[];allows recursion. If you want static enforcement, you might instead model:type NonGroupTab = Omit<Tab, "tabs" | "mosaicTree"> & { type: Exclude<TabType, "group"> }; interface GroupTab extends Omit<Tab, "type"> { type: "group"; tabs: NonGroupTab[]; mosaicTree?: MosaicNode<string>; } export type Tab = NonGroupTab | GroupTab;This keeps the current API but encodes the “no nested groups” invariant in the types.
58-82: Unify port-related shapes to reduce duplication and mismatch riskPort-related data is currently expressed in three different shapes:
Workspace.ports?: Array<number | { name: string; port: number }>(Line 81)Worktree.detectedPorts?: Record<string, number>(Line 64)DetectedPortwithport, optionalservice,terminalId,detectedAt(Lines 142–147)This makes it easy for the detection layer, workspace config, and per-worktree state to drift apart or require repetitive mapping logic. Consider extracting shared aliases, e.g.:
export interface WorkspacePort { port: number; name?: string; } export type WorkspacePortsConfig = WorkspacePort[]; // or a keyed map if preferableand/or reusing
DetectedPortwhere appropriate, instead of bespoke shapes per layer. That will simplify IPC contracts and persistence migrations later.Also applies to: 142-147
129-139: Consider more structured setup output/error modeling
SetupResultflattens all output intooutput: stringand an optionalerror?: string. If you anticipate richer UX around setup logs (filtering, streaming, persisting per-command), it may be worth splitting or structuring this now, e.g.:interface SetupResult { success: boolean; stdout: string; stderr: string; errorMessage?: string; }Not strictly required for this PR, but it will be harder to evolve once this is persisted and passed across IPC.
apps/old-desktop/src/shared/utils/slug.ts (2)
11-64: Solid slug implementation; consider guarding edge‑case paramsCore behavior looks good: normalization is strict URL/path‑safe, empty titles fall back to
"worktree", and the truncation heuristic respectsmaxLengthfor normal values ofmaxLengthandrandomLength.Two edge cases worth tightening up:
- If a caller passes inconsistent values like
maxLength < randomLength + 1,availableLengthbecomes negative and the returned slug can exceed the documentedmaxLength(suffix + hyphen alone are longer thanmaxLength).- If
randomLength === 0, you still end up with a trailing-becauserandomSuffixis empty but the template always adds the hyphen.You might either:
- Document the expected preconditions (
maxLength >= randomLength + 1andrandomLength > 0), or- Add a small runtime guard (e.g., clamp or throw) and optionally skip adding the hyphen when
randomLength === 0.For example, minimally enforcing sane arguments:
export function generateSlug( title: string, maxLength = 50, - randomLength = 4, + randomLength = 4, ): string { + if (randomLength <= 0 || maxLength <= randomLength + 1) { + throw new Error("generateSlug: randomLength must be > 0 and maxLength > randomLength + 1"); + } @@ - const availableLength = maxLength - randomLength - 1; + const availableLength = maxLength - randomLength - 1;This keeps the current behavior for all valid usages while failing fast on obviously invalid combinations instead of silently violating the
maxLengthcontract.
75-85: Branch name helper works; optional length and determinism tweaks
generateBranchNameis straightforward and readable: it delegates togenerateSlug, cleans trailing slashes from the prefix, and buildsprefix/slugas expected.Two optional considerations:
- If you intend a global maximum length for the full branch name (prefix +
/+ slug), you currently only cap the slug at 50 characters, ignoring the prefix length. You could derive a smallermaxLengthwhen callinggenerateSlugif a prefix is present.- Because
generateSlugalways appends randomness, repeated calls with the same title/prefix yield different branch names. If you ever need deterministic names (e.g., re‑deriving a branch name from an existing ticket title), it may be worth adding an overload or option to disable the random suffix or pass throughrandomLength.Nothing blocking here; just potential follow‑ups if these edge behaviors start to matter for your workflows.
apps/desktop/src/renderer/contexts/AppProviders.tsx (1)
7-9: Simplified provider composition looks good; fragment wrapper is optional.The component correctly renders children. The fragment wrapper
<></>is unnecessary since you're only returningchildren.Apply this diff for a more direct implementation:
export function AppProviders({ children }: AppProvidersProps) { - return <>{children}</>; + return children; }apps/old-desktop/.vscode/settings.json (1)
34-34: Note: Using experimental Tailwind CSS setting.The
tailwindCSS.experimental.configFilesetting may change or be removed in future Tailwind CSS IntelliSense updates. Monitor the extension's changelog for breaking changes.apps/old-desktop/src/shared/utils.ts (1)
7-17: Misleading function: transformation happens outside, not inside.The JSDoc claims the function "transforms to kebab case if the id is not provided," but the function simply returns its parameter. The actual transformation happens at module load time (lines 3-5). This makes the function unnecessarily indirect.
Option 1: Export the computed appId directly (simplest)
-/** - * @param {string} id - * @description Create the app id using the name and author from package.json transformed to kebab case if the id is not provided. - * @default 'com.{author}.{app}' - the author and app comes from package.json - * @example - * makeAppId('com.example.app') - * // => 'com.example.app' - */ -export function makeAppId(id: string = appId): string { - return id; -} +/** + * Default app identifier derived from package.json + * Format: com.{author-in-kebab-case}.{name} + * @example 'com.superset-sh.superset' + */ +export const defaultAppId = appId;Option 2: Make the function actually transform (if you need the flexibility)
-export function makeAppId(id: string = appId): string { - return id; +export function makeAppId(id?: string): string { + if (id) return id; + + const author = _author.name ?? _author; + const authorInKebabCase = author.replace(/\s+/g, "-"); + return `com.${authorInKebabCase}.${name}`.toLowerCase(); }apps/old-desktop/src/shared/ipc-channels/external.ts (1)
1-17: Confirmed: Systematic IPC channel duplication across apps—refactor to shared package.Verification confirms that not only
ExternalChannelsbut the entireipc-channelsdirectory structure is duplicated betweenapps/old-desktopandapps/desktop. Bothexternal.tsandtypes.tsare byte-for-byte identical, with all 11 IPC channel definition files matching in size and structure.Consider extracting the entire
src/shared/ipc-channels/directory to a shared package (e.g.,packages/ipc-channels/) and having both apps import from it. This will eliminate the duplication and ensure consistency across the codebase.apps/old-desktop/src/renderer/screens/main/components/Background/Background.tsx (1)
1-36: Consider marking decorative background elements as aria-hiddenSince the SVG filter and noise/gradient layers are purely visual, consider adding
aria-hidden="true"(e.g., on the<svg>and overlay<div>s) so they stay out of the accessibility tree while preserving the effect.apps/old-desktop/src/renderer/hooks/useDeepLink.ts (1)
1-60: Polling logic looks solid; small TypeScript typing nitThe polling/cleanup pattern (mounted flag +
clearTimeoutin cleanup) looks correct for avoiding work after unmount and handling IPC errors gracefully. As a minor TS nit, you might prefer an environment-agnostic timeout type:let timeoutId: ReturnType<typeof setTimeout>;instead of
NodeJS.Timeout, which can be brittle across different DOM/Node lib combinations in Electron renderer code.apps/old-desktop/package.json (1)
1-90: Consider marking the desktop package as privateIf
@superset/old-desktopisn’t intended to be published to npm, adding"private": trueat the top level would prevent accidental publishes while keeping workspace behavior unchanged.apps/desktop/src/main/windows/main.ts (1)
37-37: Remove empty close handler.The empty
closeevent handler serves no purpose and adds unnecessary code. If no cleanup is needed, the handler can be removed entirely.Apply this diff to remove the empty handler:
- window.on("close", () => {}); - return window;apps/old-desktop/src/renderer/routes.tsx (1)
58-61: Add error handling for clipboard write.The
navigator.clipboard.writeText()call may fail (e.g., due to permissions) but has no error handling. Consider adding user feedback for both success and failure cases.Apply this diff to add error handling:
<button type="button" onClick={() => { const errorText = `Error: ${error?.toString()}\n\nStack Trace:\n${error?.stack}`; - navigator.clipboard.writeText(errorText); + navigator.clipboard.writeText(errorText) + .then(() => { + // Optional: Show success toast/notification + console.log('[ErrorPage] Error details copied to clipboard'); + }) + .catch((err) => { + console.error('[ErrorPage] Failed to copy to clipboard:', err); + // Optional: Show error toast/notification + }); }} className="rounded-md border-2 border-neutral-700 bg-neutral-900 px-6 py-3 text-base font-medium text-neutral-200 hover:bg-neutral-800 hover:border-neutral-600 transition-colors" >apps/old-desktop/src/main/windows/main.ts (1)
47-90: Consider workspace lookup optimization.On every port event, the code iterates through all workspaces and their worktrees to find the matching workspace (O(n×m) complexity). For applications with many workspaces and worktrees, this could become a performance bottleneck, especially if port events are frequent.
Consider maintaining a
worktreeId -> workspaceIdlookup map in the workspace manager, or add agetWorkspaceByWorktreeId(worktreeId)method to avoid the nested iteration. This would reduce complexity to O(1) for the lookup.apps/old-desktop/src/shared/ipc-channels/types.ts (1)
8-12: Consider making response state mutually exclusive.The
IpcResponse<T>interface allows bothdataanderrorto be present simultaneously whensuccessistrueorfalse, which could lead to ambiguous states. Consider making these mutually exclusive for clearer semantics.Apply this diff to make success/error states mutually exclusive:
/** * Standard response format for operations */ -export interface IpcResponse<T = void> { - success: boolean; - data?: T; - error?: string; -} +export type IpcResponse<T = void> = + | { success: true; data: T; error?: never } + | { success: false; data?: never; error: string };Note: If you need to allow
datato be optional even on success, you can adjust to:export type IpcResponse<T = void> = | { success: true; data?: T; error?: never } | { success: false; data?: never; error: string };apps/old-desktop/src/shared/ipc-channels/worktree.ts (1)
14-14: Replace inline dynamic import with static import.The inline
import("../types").SetupResultis inconsistent with the static imports at the top of the file. This can affect type resolution and IDE support.Apply this diff:
-import type { CreateWorktreeInput, Worktree } from "../types"; +import type { CreateWorktreeInput, Worktree, SetupResult } from "../types"; import type { IpcResponse, SuccessResponse } from "./types";"worktree-create": { request: CreateWorktreeInput; response: { success: boolean; worktree?: Worktree; - setupResult?: import("../types").SetupResult; + setupResult?: SetupResult; error?: string; }; };apps/old-desktop/electron-builder.ts (2)
16-19: Enhance author name sanitization for appId generation.The current sanitization only replaces whitespace with hyphens and lowercases the result. This could produce invalid or problematic identifiers if the author name contains special characters (e.g., punctuation, symbols, or non-ASCII characters).
Consider a more robust sanitization approach:
const author = _author?.name ?? _author; const currentYear = new Date().getFullYear(); -const authorInKebabCase = author.replace(/\s+/g, "-"); +const authorInKebabCase = author + .replace(/[^a-zA-Z0-9\s]/g, "") // Remove special characters + .replace(/\s+/g, "-") // Replace spaces with hyphens + .toLowerCase(); const appId = `com.${authorInKebabCase}.${name}`.toLowerCase();
21-21: Simplify artifact name template construction.Using array join for simple string concatenation is unnecessarily complex.
Apply this diff:
-const artifactName = [`${name}-v${version}`, "-${os}.${ext}"].join(""); +const artifactName = `${name}-v${version}-\${os}.\${ext}`;Note: The
${os}and${ext}are electron-builder template variables and should remain as literal strings with escaped braces.apps/old-desktop/src/main/index.ts (2)
8-18: Document the upward traversal depth limit.The hardcoded limit of 6 levels could fail in deeply nested directory structures or be unclear to maintainers.
Consider extracting as a constant with documentation:
+// Maximum number of parent directories to search for .env file +const MAX_ENV_SEARCH_DEPTH = 6; + function findEnvFile(): string | undefined { let currentDir = __dirname; - for (let i = 0; i < 6; i++) { + for (let i = 0; i < MAX_ENV_SEARCH_DEPTH; i++) { const envPath = resolve(currentDir, ".env"); if (existsSync(envPath)) { return envPath; } currentDir = dirname(currentDir); } return undefined; }
22-23: Document the environment variable override behavior.The
override: trueoption means .env values will override existing environment variables. This could be unexpected in deployment environments where environment variables are set by the hosting platform.Add a comment explaining this behavior:
if (envPath) { - // Use override: true to ensure .env values take precedence over inherited env vars + // Use override: true to ensure .env values take precedence over inherited env vars. + // Note: This means .env will override system/platform environment variables. + // In production, consider using override: false if platform env vars should take precedence. config({ path: envPath, override: true }); console.log(`Loaded .env from ${envPath}`); } else {apps/old-desktop/src/main/lib/menu.ts (3)
82-89: Improve repository name extraction and validation.The repository name extraction using simple string split is fragile and could produce invalid or empty names for edge cases (e.g., trailing slashes, root paths, Windows paths).
Apply this diff:
+ const path = await import("node:path"); + // Create workspace with repo name and current branch - const repoName = repoPath.split("/").pop() || "Repository"; + const repoName = path.basename(repoPath) || "Repository";
75-78: Add error handling for renderer event sending.The
webContents.send()calls could fail if the renderer process crashed or the window was closed between operations. While rare, this could cause unhandled errors.Wrap the send operations with safety checks:
if (existingWorkspace) { // Workspace already exists, just switch to it console.log( "[Menu] Workspace already exists, switching to:", existingWorkspace, ); - mainWindow.webContents.send( - "workspace-opened", - existingWorkspace, - ); + if (!mainWindow.isDestroyed()) { + mainWindow.webContents.send( + "workspace-opened", + existingWorkspace, + ); + } return; }Apply similar wrapping to lines 104-107.
Also applies to: 104-107
153-155: Add window validity check before closing.The window could be destroyed between when the menu was created and when the action is triggered.
Apply this diff:
label: "Close Window", accelerator: "CmdOrCtrl+Shift+W", click: () => { - mainWindow.close(); + if (!mainWindow.isDestroyed()) { + mainWindow.close(); + } },apps/old-desktop/src/shared/ipc-channels/index.ts (1)
53-53: Consider renaming IpcResponse_ to avoid confusion.The underscore suffix in
IpcResponse_is unusual and could cause confusion with the importedIpcResponsetype. This naming suggests a conflict or workaround that might be better resolved differently.Consider one of these alternatives:
Option 1: Use a more descriptive name:
-export type IpcResponse_<T extends IpcChannelName> = IpcChannels[T]["response"]; +export type IpcChannelResponse<T extends IpcChannelName> = IpcChannels[T]["response"];Option 2: If this is meant to be the primary response type, consider whether the imported
IpcResponseis needed at all in consuming code, or if it should be scoped differently.apps/old-desktop/src/renderer/screens/main/MainScreen.tsx (2)
89-107: Add timeout handling for IPC call.The IPC call to "tab-delete" has no timeout, which could cause the UI to hang if the main process is unresponsive or the operation takes too long.
Consider wrapping the IPC call with a timeout:
const handleCloseTab = async () => { // Close the currently selected tab (terminal/preview/port) if (selectedTab && currentWorkspace && selectedWorktreeId) { try { + const timeoutPromise = new Promise((_, reject) => + setTimeout(() => reject(new Error("Tab close timeout")), 5000) + ); + - const result = await window.ipcRenderer.invoke("tab-delete", { + const result = await Promise.race([ + window.ipcRenderer.invoke("tab-delete", { - workspaceId: currentWorkspace.id, - worktreeId: selectedWorktreeId, - tabId: selectedTab.id, - }); + workspaceId: currentWorkspace.id, + worktreeId: selectedWorktreeId, + tabId: selectedTab.id, + }), + timeoutPromise, + ]); if (result.success) { // Trigger workspace refresh window.dispatchEvent(new CustomEvent("workspace-changed")); } } catch (error) { console.error("[MainScreen] Failed to close tab:", error); } } };
109-126: Track or implement pending shortcut handlers.Five of six keyboard shortcuts are not implemented (marked as TODO). Registering shortcuts that don't work could confuse users who try to use them.
Consider one of these approaches:
- Remove unimplemented shortcuts from registration until they're ready
- Implement the shortcuts
- Track this technical debt with an issue
Would you like me to open an issue to track these pending implementations?
apps/old-desktop/src/shared/ipc-channels/workspace.ts (2)
76-92: Clarify request shape for active selection channelsMost workspace channels use an object request
{ workspaceId: string; ... }, but"workspace-get-active-selection"takes a barestringwhile"workspace-set-active-selection"uses an object withworkspaceId. For consistency and clearer call sites, consider making the getter also take{ workspaceId: string }(or at least documenting that thestringis a workspaceId).If other code already calls
"workspace-get-active-selection"with an object, a quick grep or type check would catch a mismatch; please verify the intended shape.
114-223: Factor out shared workspace activation/rescan payload typesThe
"workspace-activate"and"workspace-rescan"responses inline large, nearly duplicated structures forworkspace,worktrees.ui, andui(withtabs,mosaicTree,activeTabId, timestamps, etc.), and also useunknownfor some UI fields. This makes it easy for these contracts to drift from each other and from theUiChannels/Tabtypes.Consider extracting shared types for:
- Worktree UI state (including
tabsandmosaicTree)- Workspace UI state wrapper
and reusing them here (potentially leveraging the existing
Tab/MosaicNodetypes) so that UI-related data stays in sync across IPC surfaces.apps/old-desktop/src/shared/ipc-channels/tab.ts (1)
13-73: Tab IPC contracts mirror the desktop implementation; consider harmonizing response envelopesThe tab channels are well-typed and match the desktop
TabChannelssnippet, which is good for parity between apps. One minor consistency nit:"tab-create"returns a custom{ success; tab?; error? }shape while the other mutation channels useIpcResponse. If you want a uniform envelope everywhere, consider wrappingtab-createinIpcResponse<Tab>(or conversely standardizing on the{ success; error? }shape).apps/old-desktop/src/shared/ipc-channels/ui.ts (1)
33-55: Consider making per-worktree fields optional inui-workspace-updatepatchesIn the
patch.worktreesobject, each worktree’s value requires the full structure (path,branch,tabs,activeTabId,updatedAt, etc.). If callers often want to update just a few fields (e.g.,activeTabIdortabs), it might be more ergonomic to make properties inside the worktree value optional (or reuse aPartial<UiWorktreeState>-style helper type) so you don’t need to resend the full object every time.apps/old-desktop/electron.vite.config.ts (1)
58-71: Reuse a singlegetPortSync()result to avoid port desync
getPortSync()is called twice: once forimport.meta.env.DEV_SERVER_PORTand once forserver.port. IfgetPortSync()does any probing for a free port or has side effects, these calls might not return the same value, and even with a stable return,strictPort: falselets Vite pick a fallback port on conflict—potentially diverging from the port embedded inDEV_SERVER_PORTand any main-process code that relies on it.Consider:
const devServerPort = getPortSync(); renderer: { define: { "import.meta.env.DEV_SERVER_PORT": JSON.stringify(devServerPort), // ... }, server: { port: devServerPort, strictPort: true, // if you rely on this port matching exactly }, // ... }to keep all consumers in sync and avoid subtle dev-only connection issues.
Verify how
getPortSync()behaves today (pure vs probing) to decide whetherstrictPortshould betrueand whether a single cached value is needed.apps/old-desktop/src/preload/index.ts (2)
15-18: PreloadAPIis fine; consider a more portable username sourceExposing a tiny
APIsurface from preload is a good pattern. One minor nit:username: process.env.USERwill beundefinedon Windows where the common variable isUSERNAME. If this value is user-facing or relied on elsewhere, consider something like:username: process.env.USER ?? process.env.USERNAME ?? nullor make the field optional/null to reflect that it may not always be available.
Please confirm how/if
API.usernameis used in the renderer before relying on it being non-null.
20-62: Typed IPC bridge and listener cleanup look good; tighten listener typing if desiredThe typed
invokewrapper overipcRenderer.invokeplusIpcChannelName/IpcRequest<T>/IpcResponse_<T>is a solid way to enforce IPC contracts at compile time, and thelistenerMap+on/offpairing correctly avoids leaking wrapped listeners.If you want stricter typing, you could replace
WeakMap<Function, Function>with:const listenerMap = new WeakMap< (...args: any[]) => void, (event: Electron.IpcRendererEvent, ...args: any[]) => void >();to document expected shapes. Otherwise, behavior here looks correct.
Since this relies on Electron’s IPC types, a quick TS compile against your Electron version will confirm that the handler signatures match
ipcRenderer.on/removeListener.apps/desktop/src/main/index.ts (1)
21-24: Deep-link event is stubbed; track or implement before relying on protocol registrationYou’re registering
superset://as a protocol handler but theopen-urllistener only callsevent.preventDefault()and has a TODO. That’s fine for now, but before you depend on deep linking in production, it’d be good to either wire this up (parsingurland routing it through your IPC stack) or open a tracked issue so it doesn’t get forgotten.apps/old-desktop/src/renderer/lib/keyboard-shortcuts.ts (3)
23-45: Shortcut matching logic is strict (no extra modifiers), which is good but worth noting
matchesShortcutrequires an exact match on modifier state (event.ctrlKey === hasCtrl, etc.), so pressing extra modifiers (e.g., Ctrl+Shift when only Ctrl is specified) intentionally won’t trigger the shortcut. That’s a good default for predictability; just be aware this differs from some apps that allow “at least these modifiers” semantics.
50-64:createShortcutHandlerworks; manually callingpreventDefaultin handlers is redundant
createShortcutHandlerwill callevent.preventDefault()whenever a handler returnsfalse. In your shortcut definitions you also callevent.preventDefault()inside each handler and then returnfalse, sopreventDefaultruns twice. That’s harmless but redundant; you could rely solely on thefalsereturn convention to keep the shortcut definitions slightly cleaner.
70-96:formatShortcutassumes a browser-like environment
formatShortcutusesnavigator.platformto detect macOS. That’s fine in the Electron renderer, but if this utility ever gets imported into non-DOM contexts (SSR, Node-side tools, or certain tests without a browser-like global), it will throw. If you expect reuse there, you could guard it:const isMac = typeof navigator !== "undefined" && navigator.platform.toLowerCase().includes("mac");Otherwise, this is a nice, readable formatter.
apps/old-desktop/src/renderer/lib/shortcuts.ts (2)
31-96: Workspace shortcut definitions are straightforward; you can lean on the shared handler semanticsEach workspace shortcut calls
event.preventDefault()and then returnsfalse. SincecreateShortcutHandleralready doespreventDefault()when a handler returnsfalse, you could simplify the handlers to just call the provided callback and returnfalse, leaving the lower-level utility to handle default suppression. Not mandatory, but it would reduce repetition.
98-210: Tab/terminal shortcuts look good; double-check overlappingMeta+WbindingsThe tab and terminal shortcut groups are well organized, and the Cmd+1–9 generation for tab jumps is a nice touch. One thing to be aware of: both tab and terminal groups define
Meta+Was “close current tab/terminal”. If these groups are ever active at the same time in the same keydown handler, only one semantic will “win” depending on how you route events. If tabs and terminals are mutually exclusive contexts, this is fine; otherwise, you may want to namespace handlers or differentiate the bindings slightly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/old-desktop/src/resources/build/icons/icon.icois excluded by!**/*.icoapps/old-desktop/src/resources/public/illustration.svgis excluded by!**/*.svg
📒 Files selected for processing (77)
.env.example(0 hunks)DISTRIBUTION_PLAN.md(0 hunks)apps/desktop/package.json(1 hunks)apps/desktop/src/lib/electron-app/factories/app/instance.ts(0 hunks)apps/desktop/src/lib/electron-app/factories/app/setup.ts(1 hunks)apps/desktop/src/lib/electron-router-dom.ts(1 hunks)apps/desktop/src/main/index.ts(2 hunks)apps/desktop/src/main/lib/menu.ts(1 hunks)apps/desktop/src/main/windows/main.ts(1 hunks)apps/desktop/src/renderer/contexts/AppProviders.tsx(1 hunks)apps/desktop/src/renderer/contexts/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/MainScreen.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Background/Background.tsx(1 hunks)apps/desktop/src/renderer/screens/main/constants.ts(0 hunks)apps/desktop/src/renderer/screens/main/hooks/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/types.ts(0 hunks)apps/desktop/src/renderer/screens/main/utils.ts(0 hunks)apps/old-desktop/.github/FUNDING.yml(1 hunks)apps/old-desktop/.gitignore(1 hunks)apps/old-desktop/.npmrc(1 hunks)apps/old-desktop/.vscode/extensions.json(1 hunks)apps/old-desktop/.vscode/settings.json(1 hunks)apps/old-desktop/components.json(1 hunks)apps/old-desktop/electron-builder.ts(1 hunks)apps/old-desktop/electron.vite.config.ts(1 hunks)apps/old-desktop/index.d.ts(1 hunks)apps/old-desktop/package.json(1 hunks)apps/old-desktop/src/lib/electron-app/factories/app/instance.ts(1 hunks)apps/old-desktop/src/lib/electron-app/factories/app/setup.ts(1 hunks)apps/old-desktop/src/lib/electron-app/factories/windows/create.ts(1 hunks)apps/old-desktop/src/lib/electron-app/utils/ignore-console-warnings.ts(1 hunks)apps/old-desktop/src/lib/electron-router-dom.ts(1 hunks)apps/old-desktop/src/main/index.ts(1 hunks)apps/old-desktop/src/main/lib/menu.ts(1 hunks)apps/old-desktop/src/main/windows/main.ts(1 hunks)apps/old-desktop/src/preload/index.ts(1 hunks)apps/old-desktop/src/renderer/contexts/AppProviders.tsx(1 hunks)apps/old-desktop/src/renderer/contexts/index.ts(1 hunks)apps/old-desktop/src/renderer/env.d.ts(1 hunks)apps/old-desktop/src/renderer/globals.css(1 hunks)apps/old-desktop/src/renderer/hooks/useDeepLink.ts(1 hunks)apps/old-desktop/src/renderer/index.html(1 hunks)apps/old-desktop/src/renderer/index.tsx(1 hunks)apps/old-desktop/src/renderer/lib/dnd.ts(1 hunks)apps/old-desktop/src/renderer/lib/keyboard-shortcuts.ts(1 hunks)apps/old-desktop/src/renderer/lib/shortcuts.ts(1 hunks)apps/old-desktop/src/renderer/lib/utils.ts(1 hunks)apps/old-desktop/src/renderer/routes.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/MainScreen.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/AppFrame/AppFrame.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/AppFrame/index.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Background/Background.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Background/index.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/constants.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/hooks/index.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/index.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/types.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/utils.ts(1 hunks)apps/old-desktop/src/shared/constants.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/deep-link.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/external.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/index.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/proxy.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/tab.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/terminal.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/types.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/ui.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/window.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/workspace.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/worktree.ts(1 hunks)apps/old-desktop/src/shared/types.ts(1 hunks)apps/old-desktop/src/shared/utils.ts(1 hunks)apps/old-desktop/src/shared/utils/slug.ts(1 hunks)apps/old-desktop/trusted-dependencies-scripts.json(1 hunks)apps/old-desktop/tsconfig.json(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (7)
- DISTRIBUTION_PLAN.md
- apps/desktop/src/renderer/screens/main/types.ts
- apps/desktop/src/renderer/contexts/index.ts
- apps/desktop/src/renderer/screens/main/utils.ts
- .env.example
- apps/desktop/src/renderer/screens/main/constants.ts
- apps/desktop/src/lib/electron-app/factories/app/instance.ts
🧰 Additional context used
🧬 Code graph analysis (6)
apps/old-desktop/src/shared/ipc-channels/window.ts (1)
apps/desktop/src/shared/ipc-channels/window.ts (1)
WindowChannels(7-17)
apps/old-desktop/src/shared/ipc-channels/external.ts (1)
apps/desktop/src/shared/ipc-channels/external.ts (1)
ExternalChannels(7-17)
apps/old-desktop/src/shared/ipc-channels/proxy.ts (1)
apps/desktop/src/shared/ipc-channels/proxy.ts (1)
ProxyChannels(7-17)
apps/old-desktop/src/shared/ipc-channels/tab.ts (1)
apps/desktop/src/shared/ipc-channels/tab.ts (1)
TabChannels(13-74)
apps/old-desktop/src/shared/ipc-channels/worktree.ts (1)
apps/desktop/src/shared/ipc-channels/worktree.ts (1)
WorktreeChannels(8-184)
apps/desktop/src/main/index.ts (2)
apps/desktop/src/lib/electron-app/factories/app/setup.ts (1)
makeAppSetup(13-66)apps/desktop/src/main/windows/main.ts (1)
MainWindow(7-40)
🔇 Additional comments (52)
apps/old-desktop/.github/FUNDING.yml (1)
1-2: Verify that GitHub recognizes the FUNDING.yml location.GitHub's funding workflow typically expects
FUNDING.ymlat the repository root (.github/FUNDING.yml), not within subdirectories. Placing this file atapps/old-desktop/.github/FUNDING.ymlmay prevent GitHub from recognizing it and displaying the funding options on the repository.If this file is intended to apply to the entire repository, move it to the root
.github/FUNDING.yml. If it's scoped to the old-desktop app only, note that GitHub does not currently support per-directory funding configuration.apps/desktop/src/renderer/screens/main/components/Background/Background.tsx (1)
5-8: LGTM! Correct accessibility enhancement.Adding
aria-hidden="true"to this decorative SVG is the right approach—it prevents screen readers from announcing a filter definition that provides no meaningful content to users.apps/old-desktop/src/renderer/lib/utils.ts (1)
1-6: LGTM! Standard Tailwind utility pattern.This is a well-implemented, idiomatic utility for composing Tailwind classes. The combination of
clsx(for conditional class handling) andtwMerge(for intelligent Tailwind class conflict resolution) is the recommended pattern and is widely used across the Tailwind ecosystem.apps/old-desktop/src/shared/constants.ts (1)
1-3: LGTM! Clean environment detection.The
ENVIRONMENTconstant correctly detects development mode using the standardNODE_ENVcheck.apps/old-desktop/src/renderer/screens/main/types.ts (2)
16-28: LGTM: Well-structured discriminated union.The
isPending: trueliteral type enables type-safe discrimination between pending and persisted worktrees. The optionaltaskDataprovides flexibility for partial task information during optimistic updates.
30-30: LGTM: Clean union type.The
AppModetype clearly defines the two application modes.apps/old-desktop/src/renderer/screens/main/constants.ts (1)
4-178: LGTM: Comprehensive mock data set.The 15 mock tasks cover a good variety of:
- Different statuses (working, needs-feedback, planning, ready-to-merge)
- Realistic task types (features, bugs, devops, performance)
- Various assignees with distinct avatars
- Diverse time ranges
This provides solid coverage for UI development and testing. The TODO reminder to replace with backend data is appropriately noted.
apps/desktop/src/lib/electron-router-dom.ts (2)
5-11: Summary inconsistency: DEV_SERVER_PORT is still parsed.The AI summary states getPort() is "hard-coded to return DEFAULT_PORT, removing any reliance on import.meta.env.DEV_SERVER_PORT," but the code still checks and parses DEV_SERVER_PORT on lines 7-8 before falling back to DEFAULT_PORT.
13-18: LGTM!The router creation delegates port resolution to
getPort()appropriately, and the type configuration looks correct. Assuming the port validation issue is addressed, this segment is good.apps/old-desktop/src/lib/electron-app/factories/app/instance.ts (1)
3-7: Clarify the intended usage before implementing the suggested refactor.Verification reveals that
makeAppWithSingleInstanceLockis currently not called anywhere in the codebase. The desktop version even explicitly comments "Allow multiple instances - removed single instance lock." Before implementing the suggested second-instance handler pattern, confirm whether this function is intended for future use or should be removed. If it is actively used, the refactor suggestion stands as valid Electron practice; otherwise, address whether this utility is needed.apps/desktop/src/renderer/screens/main/hooks/index.ts (1)
1-1: Dead code artifact—no actual breaking changes detected.The empty
index.tsfile contains no consumers withinapps/desktop, so there is no breaking change for this application. The hooks are not implemented or imported anywhere in the new desktop app; they appear to have been intentionally removed as part of a fresh architectural redesign.This file can simply be deleted, or if it's intended as a WIP placeholder for future hooks, it should contain a clarifying comment.
apps/old-desktop/src/lib/electron-app/factories/windows/create.ts (1)
9-13: The path construction using__dirnamewithjoin(__dirname, "../renderer/index.html")is standard practice in electron-vite and is documented in the official electron-vite troubleshooting guide as the correct approach. The build output places main and renderer code in sibling directories, making the relative path valid in production builds.Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/screens/main/MainScreen.tsx (1)
8-8: LGTM—DnD unification correctly applied.The switch to
manager={dragDropManager}aligns with the PR's DnD backend unification approach.apps/old-desktop/src/renderer/screens/main/utils.ts (2)
30-44: LGTM!The status derivation logic is sound, with appropriate fallback priority and safe property access.
46-55: LGTM!The slug generation logic is correct, and the fallback at line 94 handles edge cases where the branch name might produce an empty slug.
apps/old-desktop/src/shared/types.ts (1)
89-127: DTO shapes for workspace/worktree/tab operations look coherentThe various
Create*Input/Update*Inputinterfaces align well with the core models: they carry user-supplied fields while leaving IDs and timestamps to the orchestration/persistence layer. The optional fields (branch?,createBranch?,cloneTabsFromWorktreeId?,copyFromTabId?, etc.) give you room to extend behavior without forcing breaking changes in the persistedWorkspace/Worktreeshapes.No issues here from a typing/API perspective.
apps/desktop/package.json (1)
3-3: LGTM! Package scoped correctly.The rename from "Superset" to "@superset/desktop" follows standard monorepo conventions and aligns with the workspace structure.
apps/old-desktop/.vscode/extensions.json (1)
1-7: LGTM! Editor configuration aligns with project tooling.The extension recommendations correctly promote Biome and Tailwind while discouraging ESLint and Prettier, which is consistent with the project's tooling choices.
apps/old-desktop/src/shared/ipc-channels/proxy.ts (1)
1-17: LGTM! Type-safe IPC channel definition.The proxy status channel provides a clean, type-safe contract for querying proxy state. The optional
targetandservicefields appropriately model different proxy configurations.package.json (1)
15-19: LGTM! Convenient per-package dev scripts.The new filtered dev scripts provide a better developer experience by allowing targeted package development without running the entire monorepo.
apps/old-desktop/src/lib/electron-router-dom.ts (1)
1-29: LGTM! Well-documented router setup with clear constraints.The implementation correctly handles port resolution with Vite build-time injection and includes excellent documentation about the shared module constraints. The warnings about Node.js imports will help prevent common mistakes.
apps/old-desktop/src/lib/electron-app/factories/app/setup.ts (2)
65-68: LGTM! Proper terminal cleanup on app quit.The
before-quithandler correctly cleans up terminal processes viaterminalManager.cleanup(), preventing resource leaks. This is essential for PTY instances and child processes.
14-71: Well-structured app setup with proper lifecycle management.The setup flow appropriately handles:
- DevTools installation in development
- Window restoration with fallback to new window creation
- App activation behavior across platforms
- Navigation security in production
- Clean shutdown with terminal cleanup
apps/desktop/src/lib/electron-app/factories/app/setup.ts (1)
63-63: The concern is based on an incomplete understanding of the codebase state.The review comment assumes
apps/desktopis an active application that should maintain terminal cleanup. However, verification shows:
apps/desktopis not referenced in build/CI configurations orpackage.jsonbuild scriptsapps/old-desktopis the active parallel implementation that contains theterminalManagerimport and proper cleanup viaterminalManager.cleanup()in itsbefore-quithandler (line 67)- Removing the cleanup from
apps/desktopis intentional—this code is deprecated and has been migrated toapps/old-desktopThere is no resource leak concern because the actively maintained application (
apps/old-desktop) properly handles terminal process cleanup.Likely an incorrect or invalid review comment.
apps/old-desktop/trusted-dependencies-scripts.json (1)
1-8: LGTM!The trusted dependencies list includes legitimate build and development tools appropriate for an Electron + Vite project.
apps/old-desktop/src/renderer/screens/main/components/AppFrame/index.ts (1)
1-1: LGTM!Standard barrel export pattern for clean imports.
apps/old-desktop/src/renderer/screens/main/index.ts (1)
1-1: LGTM!Standard barrel export pattern.
apps/old-desktop/src/renderer/screens/main/components/Background/index.ts (1)
1-1: LGTM!Standard barrel export pattern.
apps/old-desktop/src/renderer/index.tsx (1)
9-9: The review comment is incorrect; no changes needed.The HTML verification confirms that both
apps/old-desktop/src/renderer/index.htmlandapps/desktop/src/renderer/index.htmlintentionally use<app></app>as the root element. The selectordocument.querySelector("app")correctly targets this custom HTML element, which is a valid and standard pattern in modern web development. Custom elements (Web Components) are part of the web platform specification.Likely an incorrect or invalid review comment.
apps/old-desktop/index.d.ts (1)
25-26: Security-sensitive webview properties are safely scoped.Verification found that
disablewebsecurityis not used anywhere in the codebase, andallowpopupsis only enabled in the preview tab with partition isolation (persist:preview-${tab.id}), which is an intentional, bounded context. No unsafe combinations or misuse patterns were detected.apps/old-desktop/src/shared/ipc-channels.ts (1)
1-5: LGTM! Good backward compatibility pattern.The barrel re-export maintains existing import paths while allowing internal modularization.
apps/old-desktop/src/shared/ipc-channels/deep-link.ts (1)
1-12: LGTM! Clean type-safe IPC channel definition.The deep linking channel interface is well-structured and type-safe.
apps/old-desktop/src/renderer/env.d.ts (1)
1-1: LGTM! Standard Vite client types reference.apps/old-desktop/src/renderer/lib/dnd.ts (1)
1-5: LGTM! Good singleton pattern for DnD.Centralizing the DragDropManager avoids nested DnD context issues and ensures consistent behavior across components.
apps/old-desktop/src/shared/ipc-channels/window.ts (1)
1-17: Window IPC channel typings look consistent and correct
WindowChannelscleanly models thewindow-createandwindow-is-restoredcontracts and matches the existing desktop definition; no issues spotted.apps/old-desktop/src/renderer/contexts/AppProviders.tsx (1)
1-42: Provider composition and lifted state look appropriateHoisting
selectedWorktreeId/selectedTabIdintoAppProvidersand threading them throughWorkspaceProviderandTabProviderprovides a clear single source of truth for selection state; the provider nesting and props look coherent.apps/desktop/src/main/lib/menu.ts (1)
11-11: Empty New Window handler.The New Window menu item has an empty click handler, which means clicking it does nothing. This creates a confusing user experience.
Verify whether this feature is intended to be implemented later or should be removed from the menu until ready.
apps/old-desktop/src/renderer/routes.tsx (1)
72-80: LGTM!The routing setup is clean and well-structured. The integration of error handling at the route level provides good UX for catching unexpected errors.
apps/old-desktop/src/renderer/screens/main/hooks/index.ts (1)
1-6: LGTM!Clean barrel export pattern for the main screen hooks. The named exports maintain good tree-shaking support.
apps/old-desktop/src/main/windows/main.ts (1)
95-112: LGTM!The startup proxy initialization is well-implemented with appropriate error handling and guards for missing workspace or ports.
apps/old-desktop/src/renderer/globals.css (1)
1-212: LGTM!Comprehensive global stylesheet with well-organized Tailwind integration, theme variables, and Electron-specific utilities. The dark mode support and custom scrollbar styling are appropriate for a desktop application.
apps/old-desktop/src/shared/ipc-channels/types.ts (1)
14-27: LGTM!The helper types
NoRequest,NoResponse, andSuccessResponseprovide clean abstractions for common IPC patterns. The naming is clear and the types are well-documented.apps/old-desktop/src/renderer/contexts/index.ts (1)
1-10: LGTM!Clean barrel export consolidating context providers and hooks for the renderer process. The organization is clear and follows standard patterns.
apps/old-desktop/electron-builder.ts (1)
42-42: Verify that notarization is intentionally disabled.Notarization is disabled for macOS builds. While this is acceptable for development, production builds distributed outside the Mac App Store should typically be notarized to avoid Gatekeeper warnings.
Confirm this is intentional for the current development phase, and consider enabling it before public distribution.
apps/old-desktop/src/main/lib/menu.ts (1)
13-24: LGTM!Excellent error handling for window creation with proper user feedback via error dialog.
apps/old-desktop/src/renderer/screens/main/MainScreen.tsx (1)
136-224: Well-structured component with proper state management.The MainScreen component demonstrates good practices:
- Clean separation of concerns with context hooks
- Proper event listener cleanup
- Defensive programming (pending worktree guards)
- Clear component composition
apps/old-desktop/src/shared/ipc-channels/terminal.ts (1)
1-48: LGTM!Well-defined terminal IPC channels with proper type safety. The inclusion of sequence numbers in terminal-resize is a good practice for preventing race conditions during rapid resize events.
apps/old-desktop/src/shared/ipc-channels/workspace.ts (1)
12-42: Workspace CRUD and basic getters look consistent and type-safeChannel names and request/response types for list/get/create/update/delete/last-opened align with the rest of the IPC surface and follow a predictable pattern using
Workspace,CreateWorkspaceInput,UpdateWorkspaceInput, andIpcResponse. No issues from a typing perspective.apps/old-desktop/src/shared/ipc-channels/ui.ts (2)
8-31: UI workspace state contract is clear and consistent
"ui-workspace-get"returns a well-structured snapshot keyed by worktree path/id, withTab[],MosaicNode<string>, and timestamps. This aligns nicely with the rest of the IPC surface and gives the renderer a single source of truth for workspace UI state.
67-82: Settings IPC looks fine; shape leaves room for future prefsThe settings channels (
ui-settings-get/ui-settings-update) are straightforward and flexible viapreferences?: Record<string, unknown>, which should work well as a generic bag for future UI prefs. No issues here.apps/desktop/src/main/index.ts (1)
26-34: Simplified main-process startup viaMainWindowfactory looks correctThe new flow
await makeAppSetup(() => MainWindow());matches themakeAppSetupsignature (createWindow: () => Promise<BrowserWindow>) given thatMainWindowitself is async. With single entry-point window creation consolidated in theMainWindowfactory, this should be easier to evolve than the previous scattered IPC/window wiring.apps/old-desktop/src/renderer/lib/shortcuts.ts (1)
10-29: Shortcut handler interface is comprehensive and easy to wire
ShortcutHandlerscleanly separates workspace, tab, and terminal behaviors, and the use ofPick<ShortcutHandlers, ...>in each factory keeps consumers honest about which callbacks they must implement. This is a solid base for wiring keyboard navigation into the main screen.
| const getPort = (): number => { | ||
| // In renderer process, Vite injects this at build time | ||
| if (import.meta.env.DEV_SERVER_PORT) { | ||
| return Number.parseInt(import.meta.env.DEV_SERVER_PORT as string, 10); | ||
| } | ||
| return 4927; // Default fallback | ||
| return DEFAULT_PORT; | ||
| }; |
There was a problem hiding this comment.
Validate the parsed port to prevent NaN.
If DEV_SERVER_PORT contains a non-numeric string, Number.parseInt() will return NaN, which could cause runtime errors when creating the router.
Apply this diff to add validation:
const getPort = (): number => {
// In renderer process, Vite injects this at build time
if (import.meta.env.DEV_SERVER_PORT) {
- return Number.parseInt(import.meta.env.DEV_SERVER_PORT as string, 10);
+ const parsed = Number.parseInt(import.meta.env.DEV_SERVER_PORT as string, 10);
+ if (!Number.isNaN(parsed) && parsed > 0) {
+ return parsed;
+ }
}
return DEFAULT_PORT;
};📝 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.
| const getPort = (): number => { | |
| // In renderer process, Vite injects this at build time | |
| if (import.meta.env.DEV_SERVER_PORT) { | |
| return Number.parseInt(import.meta.env.DEV_SERVER_PORT as string, 10); | |
| } | |
| return 4927; // Default fallback | |
| return DEFAULT_PORT; | |
| }; | |
| const getPort = (): number => { | |
| // In renderer process, Vite injects this at build time | |
| if (import.meta.env.DEV_SERVER_PORT) { | |
| const parsed = Number.parseInt(import.meta.env.DEV_SERVER_PORT as string, 10); | |
| if (!Number.isNaN(parsed) && parsed > 0) { | |
| return parsed; | |
| } | |
| } | |
| return DEFAULT_PORT; | |
| }; |
🤖 Prompt for AI Agents
In apps/desktop/src/lib/electron-router-dom.ts around lines 5 to 11, the parsed
DEV_SERVER_PORT may be NaN if it contains non-numeric text; update the function
to parse the value, validate it (e.g., use Number.parseInt then ensure the
result is a finite number and not NaN), and if validation fails fall back to
DEFAULT_PORT; ensure the function returns DEFAULT_PORT for any invalid or
missing env value to avoid runtime errors when creating the router.
| <MainContentArea mode={mode} /> | ||
| </div> | ||
| </div> | ||
| <div className="flex flex-col h-full w-full">Hi</div> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the placeholder content is intentional.
The MainScreen has been reduced to a simple "Hi" placeholder. Given the AI summary mentions removing "most state, context usage, side-effect hooks, modals, and nested UI components," please confirm this dramatic simplification is intentional scaffolding for the new desktop implementation and not an accidental removal of functionality.
Consider adding a TODO comment or temporary UI to clarify the implementation status:
- <div className="flex flex-col h-full w-full">Hi</div>
+ <div className="flex flex-col h-full w-full">
+ {/* TODO: Implement MainScreen UI */}
+ <div className="text-center mt-8">MainScreen - Under Construction</div>
+ </div>🏁 Script executed:
#!/bin/bash
# Examine the MainScreen.tsx file to understand full context
echo "=== MainScreen.tsx Content ==="
cat apps/desktop/src/renderer/screens/main/MainScreen.tsx
echo -e "\n=== Checking for related screen files ==="
fd -e tsx -e ts "Screen.tsx" apps/desktop/src/renderer/screens/
echo -e "\n=== Looking for TODO/FIXME comments in screen files ==="
rg -n "TODO|FIXME|Under Construction" apps/desktop/src/renderer/screens/ -A2 -B2
echo -e "\n=== Checking for documentation about refactoring ==="
cat README.md 2>/dev/null || echo "No README found"Length of output: 3657
🏁 Script executed:
#!/bin/bash
# Examine AppFrame component to understand expected structure
echo "=== AppFrame Component ==="
cat apps/desktop/src/renderer/screens/main/components/AppFrame.tsx
echo -e "\n=== Background Component ==="
cat apps/desktop/src/renderer/screens/main/components/Background.tsx
echo -e "\n=== Checking for placeholder patterns in codebase ==="
rg -n "placeholder|under construction|wip|work in progress" apps/desktop/src/ -i -A1 -B1 | head -20
echo -e "\n=== Checking entire screens directory structure ==="
find apps/desktop/src/renderer/screens -type f -name "*.tsx" -o -name "*.ts" | head -20
echo -e "\n=== Looking for unused imports or commented code ==="
rg -n "^[[:space:]]*//.*import|^[[:space:]]*import.*from" apps/desktop/src/renderer/screens/main/ -A1Length of output: 1475
🏁 Script executed:
#!/bin/bash
# Examine the actual component implementations
echo "=== AppFrame Component ==="
cat apps/desktop/src/renderer/screens/main/components/AppFrame/AppFrame.tsx
echo -e "\n=== Background Component ==="
cat apps/desktop/src/renderer/screens/main/components/Background/Background.tsx
echo -e "\n=== Types Definition ==="
cat apps/desktop/src/renderer/screens/main/types.ts
echo -e "\n=== Constants Definition ==="
cat apps/desktop/src/renderer/screens/main/constants.ts
echo -e "\n=== Utils Definition ==="
cat apps/desktop/src/renderer/screens/main/utils.ts
echo -e "\n=== Hooks Index ==="
cat apps/desktop/src/renderer/screens/main/hooks/index.tsLength of output: 1148
Add a TODO comment to clarify the MainScreen is under construction.
The placeholder is part of intentional scaffolding—the AppFrame and Background components are well-implemented, suggesting this is early-stage work. However, without documentation, future developers won't understand the implementation status. Add a TODO to signal this is WIP:
<div className="flex flex-col h-full w-full">Hi</div>
+ <div className="flex flex-col h-full w-full">
+ {/* TODO: Implement MainScreen content */}
+ </div>🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/MainScreen.tsx around line 11, the
placeholder div lacks an inline TODO indicating the screen is a
work-in-progress; add a single-line TODO comment immediately above (or inside)
the div that clearly marks MainScreen as under construction/WIP and optionally
notes what remains to be implemented (e.g., layout/content/props) so future
developers understand this is intentional scaffolding.
| auto-install-peers=true | ||
| shamefully-hoist=true | ||
| strict-peer-dependencies=false |
There was a problem hiding this comment.
Consider the trade-offs of permissive dependency settings.
These settings prioritize convenience over strictness:
shamefully-hoist=truecan enable phantom dependencies (importing packages not declared in package.json)strict-peer-dependencies=falsemay mask peer dependency compatibility issuesauto-install-peers=trueautomatically resolves peers, potentially hiding conflicts
While common in monorepos for compatibility, these can lead to subtle bugs that surface only in CI or production. Consider whether this level of permissiveness is necessary.
🤖 Prompt for AI Agents
In apps/old-desktop/.npmrc around lines 1 to 3, the repository-wide npm settings
are very permissive (auto-install-peers=true, shamefully-hoist=true,
strict-peer-dependencies=false); change them to stricter values as appropriate
for this codebase (for example set shamefully-hoist=false,
strict-peer-dependencies=true, and auto-install-peers=false or remove the
setting), update/commit the chosen configuration, add a short comment or README
note explaining the chosen policy, and run the repo's install/test CI locally to
verify no new dependency issues arise.
| { | ||
| "$schema": "https://ui.shadcn.com/schema.json", | ||
| "style": "new-york", | ||
| "rsc": false, | ||
| "tsx": true, | ||
| "tailwind": { | ||
| "config": "", | ||
| "css": "src/renderer/globals.css", | ||
| "baseColor": "neutral", | ||
| "cssVariables": true, | ||
| "prefix": "" | ||
| }, | ||
| "aliases": { | ||
| "components": "renderer/components", | ||
| "utils": "renderer/utils", | ||
| "ui": "renderer/components/ui", | ||
| "lib": "renderer/lib", | ||
| "hooks": "renderer/hooks" | ||
| }, | ||
| "iconLibrary": "lucide" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify path aliases and CSS file existence.
The configuration file is structurally sound, but the path aliases (lines 13–19) and the CSS path (line 8) should be validated to ensure they correspond to actual files in the renderer directory structure. Additionally, the empty "config" value on line 7 should be confirmed as intentional (relying on shadcn's auto-detection of tailwind.config.ts).
Run the following script to verify the path structure and file existence:
🏁 Script executed:
#!/bin/bash
# Description: Verify shadcn/ui path aliases and CSS file exist in the renderer structure
cd apps/old-desktop || exit 1
echo "=== Checking renderer directory structure ==="
fd . src/renderer --type d --max-depth 2 | head -20
echo -e "\n=== Checking for globals.css ==="
if [ -f "src/renderer/globals.css" ]; then
echo "✓ globals.css exists"
else
echo "✗ globals.css not found"
fi
echo -e "\n=== Checking for required alias directories ==="
for alias_dir in "components" "utils" "lib" "hooks" "components/ui"; do
if [ -d "src/renderer/$alias_dir" ]; then
echo "✓ src/renderer/$alias_dir exists"
else
echo "✗ src/renderer/$alias_dir not found"
fi
done
echo -e "\n=== Checking for tailwind.config.* files ==="
fd 'tailwind\.config\.(ts|js|cjs)' . --max-depth 2Length of output: 512
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for any utils-related files or directories to determine if utils exists elsewhere
cd apps/old-desktop || exit 1
echo "=== Searching for utils files/directories in entire project ==="
fd -i utils . --max-depth 3 --type d
echo -e "\n=== Searching for utils imports or references in code ==="
rg -i 'utils|@utils' --type ts --type tsx --type js -C 2 | head -30
Length of output: 170
---
🏁 Script executed:
```shell
#!/bin/bash
# Retry search with correct rg syntax and check tsconfig
cd apps/old-desktop || exit 1
echo "=== Checking for current utils imports/usage in code ==="
rg '@utils|from.*utils' --type ts --type js -C 2 | head -40
echo -e "\n=== Checking tsconfig.json for path mappings ==="
if [ -f "tsconfig.json" ]; then
cat tsconfig.json | jq '.compilerOptions.paths // "No paths configured"'
else
echo "tsconfig.json not found in apps/old-desktop"
fi
echo -e "\n=== Contents of src/shared/utils directory ==="
ls -la src/shared/utils/ 2>/dev/null || echo "Directory not accessible"
echo -e "\n=== Listing all TypeScript/JavaScript files in src/renderer ==="
fd '\.(ts|tsx|js)$' src/renderer --max-depth 3 | head -20
Length of output: 3552
Remove unused "utils" alias that points to non-existent directory.
The "utils": "renderer/utils" alias (line 17) references a non-existent directory. Current code imports utilities directly via "renderer/lib/utils" or relative imports, not via the @utils alias. Remove this unused alias to prevent configuration drift and avoid confusion for future developers. If renderer-wide utilities are needed, create the directory first or adjust the alias to point to renderer/lib instead.
🤖 Prompt for AI Agents
In apps/old-desktop/components.json around lines 1 to 21, remove the unused
"utils" alias that points to "renderer/utils" (a non-existent directory); update
the JSON by deleting the "utils" key from the "aliases" object so aliases only
reference valid paths, or if you intended a renderer-wide utilities alias
instead, change its value to "renderer/lib" or create the "renderer/utils"
directory before keeping the alias.
| config({ path: resolve(__dirname, "../../.env"), override: true }); | ||
|
|
||
| const [nodeModules, devFolder] = normalize(dirname(main)).split(/\/|\\/g); | ||
| const devPath = [nodeModules, devFolder].join("/"); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
devPath derivation is brittle; consider using dirname(main) directly
const [nodeModules, devFolder] = normalize(dirname(main)).split(/\/|\\/g); assumes that dirname(main) always has at least two path segments and that you only care about the first two. If main ever changes shape (e.g., dist/main/index.js vs something deeper), devPath can become incorrect (dist/undefined or truncated paths), which will break the main/preload/renderer build outputs.
If the intention is “use the folder that contains the main bundle” as the dev output root, a more robust version would be:
const devPath = dirname(main);or, if you truly need a specific parent:
const mainDir = normalize(dirname(main));
const devPath = resolve(mainDir); // or resolve(mainDir, "..") as appropriatePlease double-check the actual main value in this package.json and ensure the resulting devPath matches where Electron expects built files.
🏁 Script executed:
#!/bin/bash
# Locate and display the relevant files
echo "=== Finding package.json in apps/old-desktop ==="
fd 'package.json' apps/old-desktop
echo -e "\n=== Finding electron.vite.config.ts ==="
fd 'electron.vite.config.ts' apps/old-desktop
echo -e "\n=== Display apps/old-desktop/package.json ==="
if [ -f "apps/old-desktop/package.json" ]; then
cat "apps/old-desktop/package.json"
fi
echo -e "\n=== Display electron.vite.config.ts (full file) ==="
if [ -f "apps/old-desktop/electron.vite.config.ts" ]; then
cat "apps/old-desktop/electron.vite.config.ts"
fiLength of output: 5669
Refactor devPath derivation to be more maintainable and robust
The current implementation relies on splitting dirname(main) and taking only the first two segments. While this works for the current main value (./node_modules/.dev/main/index.js → node_modules/.dev), it's fragile and will break silently if main ever changes structure.
If main were dist/main/index.js, the split would yield ["dist", "main"], destructuring to devPath = "dist/main", causing builds to nest incorrectly (e.g., dist/main/main). Similarly, deeper paths are truncated without warning.
A more maintainable approach:
const devPath = dirname(dirname(main));or
const mainDir = normalize(dirname(main));
const devPath = resolve(mainDir, "..");Both are clearer about intent (extract the parent directory of main) and don't depend on a fixed segment count.
🤖 Prompt for AI Agents
In apps/old-desktop/electron.vite.config.ts around lines 14 to 18, the current
devPath derivation splits dirname(main) and takes the first two segments which
is fragile; replace that logic with a clear parent-directory computation (e.g.,
use dirname(dirname(main)) or get mainDir = normalize(dirname(main)) and then
devPath = resolve(mainDir, "..")) so devPath consistently points to the parent
directory of main without relying on segment counts.
| interface AppFrameProps { | ||
| children: React.ReactNode; | ||
| } | ||
|
|
||
| export function AppFrame({ children }: AppFrameProps) { | ||
| return <div className="absolute inset-0 bg-stone-950 flex">{children}</div>; | ||
| } |
There was a problem hiding this comment.
Missing React import.
The component uses JSX and React.ReactNode type but doesn't import React. This will cause a runtime error.
Apply this diff to add the missing import:
+import React from "react";
+
interface AppFrameProps {
children: React.ReactNode;
}📝 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.
| interface AppFrameProps { | |
| children: React.ReactNode; | |
| } | |
| export function AppFrame({ children }: AppFrameProps) { | |
| return <div className="absolute inset-0 bg-stone-950 flex">{children}</div>; | |
| } | |
| import React from "react"; | |
| interface AppFrameProps { | |
| children: React.ReactNode; | |
| } | |
| export function AppFrame({ children }: AppFrameProps) { | |
| return <div className="absolute inset-0 bg-stone-950 flex">{children}</div>; | |
| } |
🤖 Prompt for AI Agents
In apps/old-desktop/src/renderer/screens/main/components/AppFrame/AppFrame.tsx
around lines 1 to 7, the file uses JSX and React.ReactNode but doesn't import
React; add an import for React at the top of the file (e.g. import React from
'react';) so the React type and JSX runtime are available, then save.
| export function findTabRecursive( | ||
| tabs: Tab[] | undefined, | ||
| tabId: string, | ||
| ): { tab: Tab; parent?: Tab } | null { | ||
| if (!tabs) return null; | ||
|
|
||
| for (const tab of tabs) { | ||
| if (tab.id === tabId) { | ||
| return { tab }; | ||
| } | ||
| // Check if this tab is a group tab with children | ||
| if (tab.type === "group" && tab.tabs) { | ||
| for (const childTab of tab.tabs) { | ||
| if (childTab.id === tabId) { | ||
| return { tab: childTab, parent: tab }; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Function is not truly recursive—fails to find deeply nested tabs.
Despite the name and comment, this function only searches one level deep into group tabs. If groups can be nested multiple levels (e.g., a group containing another group with tabs), those deeply nested tabs won't be found.
Apply this diff to make the function truly recursive:
export function findTabRecursive(
tabs: Tab[] | undefined,
tabId: string,
): { tab: Tab; parent?: Tab } | null {
if (!tabs) return null;
for (const tab of tabs) {
if (tab.id === tabId) {
return { tab };
}
// Check if this tab is a group tab with children
if (tab.type === "group" && tab.tabs) {
- for (const childTab of tab.tabs) {
- if (childTab.id === tabId) {
- return { tab: childTab, parent: tab };
- }
- }
+ const result = findTabRecursive(tab.tabs, tabId);
+ if (result) {
+ // If the found tab has no parent, this group is its parent
+ return result.parent ? result : { ...result, parent: tab };
+ }
}
}
return null;
}📝 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.
| export function findTabRecursive( | |
| tabs: Tab[] | undefined, | |
| tabId: string, | |
| ): { tab: Tab; parent?: Tab } | null { | |
| if (!tabs) return null; | |
| for (const tab of tabs) { | |
| if (tab.id === tabId) { | |
| return { tab }; | |
| } | |
| // Check if this tab is a group tab with children | |
| if (tab.type === "group" && tab.tabs) { | |
| for (const childTab of tab.tabs) { | |
| if (childTab.id === tabId) { | |
| return { tab: childTab, parent: tab }; | |
| } | |
| } | |
| } | |
| } | |
| return null; | |
| } | |
| export function findTabRecursive( | |
| tabs: Tab[] | undefined, | |
| tabId: string, | |
| ): { tab: Tab; parent?: Tab } | null { | |
| if (!tabs) return null; | |
| for (const tab of tabs) { | |
| if (tab.id === tabId) { | |
| return { tab }; | |
| } | |
| // Check if this tab is a group tab with children | |
| if (tab.type === "group" && tab.tabs) { | |
| const result = findTabRecursive(tab.tabs, tabId); | |
| if (result) { | |
| // If the found tab has no parent, this group is its parent | |
| return result.parent ? result : { ...result, parent: tab }; | |
| } | |
| } | |
| } | |
| return null; | |
| } |
| branch: pending.branch, | ||
| path: "", // Pending worktrees don't have a path yet | ||
| tabs: [], | ||
| createdAt: new Date().toISOString(), |
There was a problem hiding this comment.
All pending worktrees receive identical timestamps.
Using new Date().toISOString() means all pending worktrees created in the same batch will have the same createdAt timestamp, which may affect sorting or chronological display.
Consider capturing timestamps earlier in the workflow (when each pending worktree is created) or adding a sequence number:
- createdAt: new Date().toISOString(),
+ createdAt: pending.createdAt || new Date().toISOString(),This assumes PendingWorktree can optionally carry a createdAt field captured at creation time.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/old-desktop/src/renderer/screens/main/utils.ts around line 69, the use
of new Date().toISOString() inside the batch construct gives identical createdAt
values for all pending worktrees; change the code to assign a per-worktree
timestamp at the moment each pending worktree is instantiated (or accept a
createdAt parameter) so each item gets its own new Date().toISOString() (or a
monotonically increasing sequence number) rather than sharing one timestamp
generated for the whole batch — update the PendingWorktree creation callsite(s)
to supply createdAt and adjust the type if needed.
| "target": "esnext", | ||
| "lib": ["esnext", "dom", "dom.iterable"], | ||
| "jsx": "react-jsx", | ||
| "importHelpers": true, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify tslib is installed as a dependency.
"importHelpers": true directs TypeScript to import helper functions from tslib instead of inlining them. If tslib is not declared in package.json, the build will fail at runtime.
Please verify that tslib is listed as a dependency:
🏁 Script executed:
#!/bin/bash
# Search for tslib in package.json
fd -e json -name "package.json" -path "*/old-desktop/*" -exec grep -l "tslib" {} \;Length of output: 230
🏁 Script executed:
#!/bin/bash
# Find and display package.json from old-desktop
fd -type f -name "package.json" -path "*/old-desktop/*" | head -1 | xargs cat | grep -A 5 -B 5 "tslib"Length of output: 170
🏁 Script executed:
#!/bin/bash
# Find package.json in old-desktop and search for tslib
fd "package.json" "apps/old-desktop" | xargs cat | grep "tslib"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Directly display old-desktop package.json
cat apps/old-desktop/package.jsonLength of output: 2929
Add tslib as a dependency in apps/old-desktop/package.json.
With "importHelpers": true enabled in tsconfig.json, TypeScript generates code that imports helper utilities from tslib. However, tslib is not listed in dependencies or devDependencies. This will cause runtime failures when the compiled code attempts to import from tslib.
Add "tslib" to the dependencies section of apps/old-desktop/package.json.
🤖 Prompt for AI Agents
In apps/old-desktop/tsconfig.json around line 7, "importHelpers": true is
enabled which causes the compiler to emit imports from tslib, but
apps/old-desktop/package.json currently lacks tslib as a dependency; open
apps/old-desktop/package.json and add "tslib" to the dependencies object (not
devDependencies), pin an appropriate semver (e.g., ^2.5.0 or the project's
standard tslib version), save the file and run package manager install to ensure
tslib is present at runtime.
| "paths": { | ||
| "*": ["src/*"], | ||
| "~/*": ["./*"] | ||
| } |
There was a problem hiding this comment.
Catch‑all path alias risks shadowing Node builtins and npm packages.
The path pattern "*": ["src/*"] is a catch-all that will intercept any import not matching prior patterns, including Node builtins (fs, path, etc.) and third-party packages. This can lead to silent resolution failures or unexpected behavior.
Replace the catch-all with explicit, narrow aliases instead:
"paths": {
- "*": ["src/*"],
"~/*": ["./*"]
+ "@/*": ["src/*"]
}Then update imports throughout the codebase from import Foo from "Foo" to import Foo from "@/Foo".
📝 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.
| "paths": { | |
| "*": ["src/*"], | |
| "~/*": ["./*"] | |
| } | |
| "paths": { | |
| "~/*": ["./*"], | |
| "@/*": ["src/*"] | |
| } |
🤖 Prompt for AI Agents
In apps/old-desktop/tsconfig.json around lines 20–23, the current catch-all path
mapping ("*": ["src/*"]) can shadow Node builtins and npm packages; remove that
catch-all and replace it with explicit, narrow aliases (for example map "@/ *"
or "~/*" to "src/*" instead of "*"), then update all imports that relied on the
catch-all (e.g., change imports like import Foo from "Foo" to import Foo from
"@/Foo" or the chosen alias), run a repo-wide search-and-replace to update
imports, verify no imports reference builtins or packages via the alias, and run
the TypeScript build/tests to confirm resolution and adjust any remaining import
paths.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/desktop/electron-builder.ts (1)
15-19: Consider more robust electron version extraction.The regex
/^\^/only removes a leading caret. Ifpackage.jsonuses other version prefixes (~,>=, etc.) or ranges, they won't be stripped properly. Consider a more comprehensive extraction.- electronVersion: pkg.devDependencies.electron.replace(/^\^/, ""), + electronVersion: pkg.devDependencies.electron.replace(/^[\^~>=<]+/, ""),Alternatively, use
semver.coerce()for more robust version parsing:import semver from "semver"; // ... electronVersion: semver.coerce(pkg.devDependencies.electron)?.version ?? pkg.devDependencies.electron,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
apps/desktop/.npmrc(0 hunks)apps/desktop/electron-builder.ts(1 hunks)apps/desktop/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/.npmrc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (4)
apps/desktop/package.json (2)
3-3: LGTM: Scoped package name follows best practices.The scoped package name
@superset/desktopis consistent with other workspace packages in the dependencies and follows npm monorepo conventions.
8-11: No action required—the license removal is correct.The monorepo uses inherited licensing from the root: the Apache-2.0 license in the repository root applies to all packages. Most workspace packages (blog, docs, website, and now desktop) correctly omit the
licensefield to inherit the root license. The removal of "MIT" fromapps/desktop/package.jsonaligns with this monorepo licensing strategy and is intentional.apps/desktop/electron-builder.ts (2)
1-8: LGTM: Clean imports and centralized configuration approach.Using
package.jsonas the single source of truth for build metadata is a good practice that reduces duplication and potential inconsistencies.
21-35: LGTM: Files configuration properly includes build artifacts.The files array correctly includes the
distdirectory (which should containmain/index.jsper package.json),package.json, and resources. This aligns with the production build structure.
| const currentYear = new Date().getFullYear(); | ||
| const author = pkg.author?.name ?? pkg.author; | ||
| const authorInKebabCase = author.replace(/\s+/g, "-"); | ||
| const appId = `com.${authorInKebabCase}.${name}`.toLowerCase(); | ||
| const appId = `com.${authorInKebabCase}.${pkg.name}`.toLowerCase(); |
There was a problem hiding this comment.
appId may contain invalid characters due to scoped package name.
The appId construction uses pkg.name directly, which is now @superset/desktop. This produces an appId like com.superset.@superset/desktop, containing @ and / characters. These special characters may cause issues with platform-specific app identifiers (macOS bundle IDs, Windows app IDs).
Consider sanitizing the package name before using it:
const currentYear = new Date().getFullYear();
const author = pkg.author?.name ?? pkg.author;
const authorInKebabCase = author.replace(/\s+/g, "-");
-const appId = `com.${authorInKebabCase}.${pkg.name}`.toLowerCase();
+const appId = `com.${authorInKebabCase}.${pkg.name.replace(/[@\/]/g, "")}`.toLowerCase();📝 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.
| const currentYear = new Date().getFullYear(); | |
| const author = pkg.author?.name ?? pkg.author; | |
| const authorInKebabCase = author.replace(/\s+/g, "-"); | |
| const appId = `com.${authorInKebabCase}.${name}`.toLowerCase(); | |
| const appId = `com.${authorInKebabCase}.${pkg.name}`.toLowerCase(); | |
| const currentYear = new Date().getFullYear(); | |
| const author = pkg.author?.name ?? pkg.author; | |
| const authorInKebabCase = author.replace(/\s+/g, "-"); | |
| const appId = `com.${authorInKebabCase}.${pkg.name.replace(/[@\/]/g, "")}`.toLowerCase(); |
| // macOS | ||
| mac: { | ||
| artifactName, | ||
| icon: `${resources}/build/icons/icon.icns`, | ||
| icon: join(pkg.resources, "build/icons/icon.icns"), | ||
| category: "public.app-category.utilities", | ||
| target: ["zip", "dmg", "dir"], | ||
| target: [ | ||
| { | ||
| target: "default", | ||
| arch: ["universal"], | ||
| }, | ||
| ], | ||
| hardenedRuntime: true, | ||
| gatekeeperAssess: false, | ||
| notarize: false, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Consider enabling notarization for production macOS builds.
The macOS configuration includes hardenedRuntime: true but notarize: false. For production releases distributed outside the Mac App Store, notarization is required by Apple to avoid Gatekeeper warnings. Verify this is intentional for development builds only.
🌐 Web query:
What are the requirements for macOS notarization in Electron apps?
💡 Result:
Short checklist (requirements) for notarizing Electron macOS apps:
- Apple Developer Program membership and Team ID; a Developer ID signing certificate (Developer ID Application / Installer). [1][2]
- Build/sign on macOS with Xcode tools installed (xcode/xcrun/notarytool available). [1][2]
- Code sign the .app and all nested executables/helper bundles with your Developer ID certificate before upload. [1][2]
- Enable the Hardened Runtime when signing; include required entitlements for any JIT or special behavior (e.g., com.apple.security.cs.allow-jit; older Electron/Chromium setups sometimes need com.apple.security.cs.allow-unsigned-executable-memory). Apply entitlements per-binary as needed. [1][2][3]
- Authenticate notarization uploads using an app‑specific password (Apple ID) or an App Store Connect API key (JWT .p8) and the correct Team ID. [3][1]
- Use Apple’s notarytool (recommended) or legacy altool to submit the signed app/installer; wait for “Ready to distribute”, then staple the notarization ticket to your artifact (xcrun stapler). [2][3]
- Test Gatekeeper locally (spctl/xattr) and inspect notarization logs if rejected; ensure all nested code is signed and entitlements are correct. [2][8]
Sources: Electron docs and tooling for signing/notarize, and Apple notarization/upload docs. [1][2][3]
References
[1] Electron — Code signing & notarizing macOS builds. (electronjs.org)
[2] Apple — Upload a macOS app to be notarized (Xcode docs). (help.apple.com)
[3] @electron/notarize / npm & GitHub (usage, auth methods, entitlements notes). (npmjs.com)
Citations:
- 1: https://www.electronjs.org/docs/latest/tutorial/code-signing?utm_source=openai
- 2: https://help.apple.com/xcode/mac/current/en.lproj/dev88332a81e.html?utm_source=openai
- 3: https://www.npmjs.com/package/%40electron/notarize/v/2.3.0?utm_source=openai
I need to examine the full electron-builder.ts file to understand the context better. Let me check whether this is a production configuration and if notarization handling is conditional or deferred elsewhere.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the full electron-builder.ts file to understand context
cat apps/desktop/electron-builder.ts
Length of output: 1765
Let me check if there are other build configurations or scripts that handle production builds differently.
🏁 Script executed:
#!/bin/bash
# Check for other build scripts or configurations
fd -e json -e ts -e js | grep -E "(build|package)" | head -20Length of output: 559
🏁 Script executed:
#!/bin/bash
# Check package.json for build scripts
cat apps/desktop/package.json | grep -A 20 '"scripts"'Length of output: 909
Let me check if @electron/notarize is used elsewhere or if there are build hooks handling notarization:
🏁 Script executed:
#!/bin/bash
# Search for @electron/notarize usage
rg '@electron/notarize' -n -C3Length of output: 3462
🏁 Script executed:
#!/bin/bash
# Check for electron-vite config and build hooks
fd -e ts -e js | xargs rg -l 'notarize|beforeBuild|afterBuild' | head -10Length of output: 69
Let me check the old-desktop version to see if there's a pattern or if this is a known issue:
---
🏁 Script executed:
```shell
#!/bin/bash
# Compare with old-desktop electron-builder.ts
cat apps/old-desktop/electron-builder.ts | grep -A 10 "mac:"
Length of output: 231
Enable notarization for production macOS builds.
Notarization is required for production macOS apps distributed outside the App Store to avoid Gatekeeper warnings, and requires code signing with Developer ID certificate and authentication via app-specific password or API key. The current configuration has notarize: false hardcoded at line 50, and since the config is static with no environment-based conditionals, this applies to production builds invoked via the release script (electron-builder --publish always). Update the configuration to either set notarize: true and configure the necessary Apple Developer credentials, or implement environment-based logic to disable notarization only for development builds.
| "description": "The last developer tool you'll ever need", | ||
| "version": "0.0.0", | ||
| "main": "./node_modules/.dev/main/index.js", | ||
| "main": "./dist/main/index.js", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the build process creates the main entry point.
The main entry point has shifted from a development path to ./dist/main/index.js. Ensure that the build process (via compile:app script) correctly generates this file.
🏁 Script executed:
#!/bin/bash
# Description: Verify that the build configuration creates dist/main/index.js
# Check Vite/Electron-Vite config for main entry build
fd -e ts -e js "electron-vite|vite.config" apps/desktop --exec cat {}Length of output: 2478
Build configuration is incompatible with new main entry point.
The devPath calculation in apps/desktop/vite.config.ts expects a path with at least 3 segments. With main now set to ./dist/main/index.js, the logic produces:
dirname(main)=./dist/main- After splitting and taking first two segments:
devPath = "dist/main" - Output directory becomes
resolve("dist/main", "main")=dist/main/main
This outputs the built file to dist/main/main/index.js, but package.json expects it at dist/main/index.js.
Update apps/desktop/vite.config.ts line 11 to adjust the devPath calculation to work with the new directory structure.
🤖 Prompt for AI Agents
In apps/desktop/vite.config.ts around line 11, the current devPath logic takes
the first two segments of dirname(main) which yields "dist/main" and makes the
output folder dist/main/main; instead, compute devPath by taking dirname(main)
and removing its last path segment (i.e., drop the final folder like "main") so
devPath becomes "dist" for "./dist/main/index.js"; update the code to split
dirname(main) into segments and join all but the last segment (with a fallback
to dirname if there is only one segment) so resolve(devPath, "main") produces
dist/main and the built file lands at dist/main/index.js.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores