Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds per-window state persistence and restoration, refactors IPC channel typings into a modular hub, exposes window restoration checks to the renderer, and updates startup flow to attempt restoring windows before creating new ones; several renderer UI spacing and modal-behavior adjustments included. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup
participant Setup as makeAppSetup
participant WM as WindowManager
participant WSM as WindowStateManager
participant Renderer as Renderer (useWorkspace)
App->>Setup: start (createWindow, restoreWindows)
Setup->>WM: restoreWindows()
WM->>WSM: getWindowStates()
WSM-->>WM: saved window entries
alt saved entries exist
WM->>WM: createWindow(restoreState) for each entry
WM->>WM: mark window as restored
else no saved entries
Setup->>WM: createWindow() (new window)
end
Note over WM,WSM: WM persists bounds/workspace on move/resize/close via WSM
Renderer->>WM: IPC "window-is-restored"?
WM-->>Renderer: boolean
alt restored && no workspace
Renderer->>Renderer: suppress workspace selection modal
else show modal if needed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes
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 (1)
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: 6
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx (3)
43-51: Add error handling for IPC call.The Settings button invokes an IPC call without error handling. If the call fails, the user receives no feedback.
Apply this diff to add error handling:
<Button variant="ghost" - onClick={() => { - window.ipcRenderer.invoke("open-app-settings"); + onClick={async () => { + try { + await window.ipcRenderer.invoke("open-app-settings"); + } catch (error) { + console.error("Failed to open settings:", error); + } }} >
70-81: Remove onClick handlers from disabled action cards.The "Clone repo" and "Connect via SSH" cards have
onClickhandlers but are also markeddisabled. While the disabled attribute prevents the handlers from firing, having them present is confusing and may mislead future developers.Apply this diff to remove the unused handlers:
<button type="button" - onClick={handleOpenProject} className="group relative p-6 rounded-lg border border-neutral-700 bg-neutral-800/50 hover:bg-neutral-800 hover:border-neutral-600 transition-all text-left opacity-50 cursor-not-allowed" disabled title="Coming soon" > <Download className="w-6 h-6 text-neutral-400 group-hover:text-blue-500 transition-colors mb-3" /> <div className="text-sm font-medium text-white"> Clone repo </div> </button> <button type="button" - onClick={handleOpenProject} className="group relative p-6 rounded-lg border border-neutral-700 bg-neutral-800/50 hover:bg-neutral-800 hover:border-neutral-600 transition-all text-left opacity-50 cursor-not-allowed" disabled title="Coming soon" > <Terminal className="w-6 h-6 text-neutral-400 group-hover:text-blue-500 transition-colors mb-3" /> <div className="text-sm font-medium text-white"> Connect via SSH </div> </button>Also applies to: 83-94
141-151: Minor text inconsistency in empty state.The empty state message says 'Click "Open project" to get started', but the action card label is "Open folder". Consider updating the text for consistency.
Apply this diff:
<p className="text-neutral-500 text-xs"> - Click "Open project" to get started + Click "Open folder" to get started </p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/src/lib/electron-app/factories/app/setup.ts(2 hunks)apps/desktop/src/main/index.ts(1 hunks)apps/desktop/src/main/lib/window-ipcs.ts(2 hunks)apps/desktop/src/main/lib/window-manager.ts(2 hunks)apps/desktop/src/main/lib/window-state-manager.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx(2 hunks)apps/desktop/src/renderer/screens/main/hooks/useWorkspace.ts(2 hunks)apps/desktop/src/shared/ipc-channels.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/lib/electron-app/factories/app/setup.ts (2)
apps/desktop/src/main/lib/window-manager.ts (2)
createWindow(10-98)restoreWindows(122-151)apps/desktop/src/lib/electron-app/factories/windows/create.ts (1)
createWindow(6-18)
apps/desktop/src/main/lib/window-manager.ts (1)
apps/desktop/src/main/windows/main.ts (1)
MainWindow(19-123)
apps/desktop/src/main/index.ts (2)
apps/desktop/src/main/lib/window-ipcs.ts (1)
registerWindowIPCs(4-25)apps/desktop/src/lib/electron-app/factories/app/setup.ts (1)
makeAppSetup(14-71)
🔇 Additional comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx (3)
1-6: LGTM!The imports are appropriate for the new UI design with action cards and icons.
7-19: LGTM!The props interface is well-defined and supports the new workspace selection workflow.
28-29: Verify that workspaces are sorted by recency.The code slices the first 5 workspaces to display as "Recent projects", but there's no guarantee in the type signature or prop name that the workspaces array is sorted by recency. Ensure that the parent component sorts the workspaces before passing them to this modal.
Run the following script to verify how workspaces are provided to this component:
| // Save window bounds periodically and on move/resize | ||
| const saveBounds = () => { | ||
| // Check if window still exists and is not destroyed | ||
| if (window.isDestroyed() || !this.windows.has(window)) { | ||
| return; | ||
| } | ||
| const currentWorkspaceId = this.windowWorkspaces.get(window) ?? null; | ||
| windowStateManager.saveWindowState(window, currentWorkspaceId); | ||
| }; | ||
|
|
||
| window.on("moved", saveBounds); | ||
| window.on("resized", saveBounds); | ||
|
|
||
| window.on("close", () => { | ||
| // Remove event listeners to prevent them from firing after close | ||
| window.removeListener("moved", saveBounds); | ||
| window.removeListener("resized", saveBounds); | ||
|
|
||
| // Save final state before closing (window is still valid here) |
There was a problem hiding this comment.
Debounce the synchronous save on move/resize.
Line 41 invokes saveWindowState, which performs a full sync read/write on every moved/resized event. Those events fire dozens of times per drag, so the main process stalls and the UI janks. Please throttle/debounce the write so you only hit disk after movement settles (and flush on close). (electronjs.org)
Apply this diff:
- const saveBounds = () => {
- // Check if window still exists and is not destroyed
- if (window.isDestroyed() || !this.windows.has(window)) {
- return;
- }
- const currentWorkspaceId = this.windowWorkspaces.get(window) ?? null;
- windowStateManager.saveWindowState(window, currentWorkspaceId);
- };
-
- window.on("moved", saveBounds);
- window.on("resized", saveBounds);
+ let saveBoundsTimer: NodeJS.Timeout | null = null;
+ const scheduleSaveBounds = () => {
+ if (saveBoundsTimer) {
+ clearTimeout(saveBoundsTimer);
+ }
+ saveBoundsTimer = setTimeout(() => {
+ saveBoundsTimer = null;
+ if (window.isDestroyed() || !this.windows.has(window)) {
+ return;
+ }
+ const currentWorkspaceId =
+ this.windowWorkspaces.get(window) ?? null;
+ windowStateManager.saveWindowState(window, currentWorkspaceId);
+ }, 200);
+ };
+
+ window.on("moved", scheduleSaveBounds);
+ window.on("resized", scheduleSaveBounds);And inside the close handler add:
+ if (saveBoundsTimer) {
+ clearTimeout(saveBoundsTimer);
+ saveBoundsTimer = null;
+ }Committable suggestion skipped: line range outside the PR's diff.
| window.on("closed", () => { | ||
| // Remove from state after window is fully closed | ||
| // Use stored window ID since window is now destroyed | ||
| setTimeout(() => { | ||
| windowStateManager.removeWindowState(windowId); | ||
| }, 100); | ||
|
|
||
| this.windows.delete(window); | ||
| this.windowWorkspaces.delete(window); | ||
| this.restoredWindowIds.delete(windowId); | ||
| }); |
There was a problem hiding this comment.
Don’t delete the state you just saved.
Line 89 schedules removeWindowState(windowId) for every closed window, so by the time the app exits cleanly the JSON is empty and nothing can be restored on the next launch—the feature is effectively disabled. Keep the entry for workspace-backed windows and only prune orphaned/null workspaces.
Apply this diff:
- // Remove from state after window is fully closed
- // Use stored window ID since window is now destroyed
- setTimeout(() => {
- windowStateManager.removeWindowState(windowId);
- }, 100);
+ const lastState = windowStateManager.getWindowState(windowId);
+ if (!lastState?.workspaceId) {
+ setTimeout(() => {
+ windowStateManager.removeWindowState(windowId);
+ }, 100);
+ }📝 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.
| window.on("closed", () => { | |
| // Remove from state after window is fully closed | |
| // Use stored window ID since window is now destroyed | |
| setTimeout(() => { | |
| windowStateManager.removeWindowState(windowId); | |
| }, 100); | |
| this.windows.delete(window); | |
| this.windowWorkspaces.delete(window); | |
| this.restoredWindowIds.delete(windowId); | |
| }); | |
| window.on("closed", () => { | |
| const lastState = windowStateManager.getWindowState(windowId); | |
| if (!lastState?.workspaceId) { | |
| setTimeout(() => { | |
| windowStateManager.removeWindowState(windowId); | |
| }, 100); | |
| } | |
| this.windows.delete(window); | |
| this.windowWorkspaces.delete(window); | |
| this.restoredWindowIds.delete(windowId); | |
| }); |
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/window-manager.ts around lines 85 to 95, the
current on("closed") handler unconditionally calls setTimeout(() =>
windowStateManager.removeWindowState(windowId), 100) which deletes saved window
state and prevents restoration; change this so you do not remove the saved state
for windows backed by a workspace. Instead, determine whether the closed window
has a valid workspace (e.g., via this.windowWorkspaces.get(window) or stored
workspaceId) and only call removeWindowState(windowId) when the workspace is
null/orphaned/unmanaged; still delete the runtime maps
(this.windows.delete(window), this.windowWorkspaces.delete(window)) and remove
restoredWindowIds.delete(windowId) as before, but guard the state-removal call
behind the orphaned-workspace check or remove the timeout entirely and perform
conditional cleanup synchronously.
| const savedStates = windowStateManager.getWindowStates(); | ||
|
|
||
| // Only restore windows that have a workspace assigned | ||
| // Windows without workspace were likely closed intentionally | ||
| const windowsToRestore = savedStates.filter((state) => state.workspaceId); | ||
| const windowsWithoutWorkspace = savedStates.filter( | ||
| (state) => !state.workspaceId, | ||
| ); | ||
|
|
||
| // Clean up windows without workspaces (they were closed intentionally) | ||
| for (const state of windowsWithoutWorkspace) { | ||
| windowStateManager.removeWindowState(Number.parseInt(state.id, 10)); | ||
| } | ||
|
|
||
| // Restore all saved windows with workspaces | ||
| for (const state of windowsToRestore) { | ||
| try { | ||
| await this.createWindow({ | ||
| workspaceId: state.workspaceId, | ||
| bounds: state.bounds, | ||
| }); | ||
| } catch (error) { | ||
| console.error( | ||
| `[WindowManager] Failed to restore window ${state.id}:`, | ||
| error, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Skip restoring windows that are owned by another live instance.
With multi-instance explicitly supported (see comment in apps/desktop/src/main/index.ts Lines 39-44), this loop will resurrect every workspace window that a still-running sibling already has open—each instance keeps its state file entry while active—so the second process spawns duplicates immediately. Please tag each persisted WindowState with an owner PID/session and have restoreWindows ignore entries whose owner process is still alive (e.g., record ownerPid = process.pid on save and filter using process.kill(ownerPid, 0) with appropriate guards). Without this guard, restoration breaks multi-instance usage.
| import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; | ||
| import os from "node:os"; | ||
| import path from "node:path"; | ||
| import type { BrowserWindow } from "electron"; | ||
|
|
||
| interface WindowState { | ||
| id: string; // webContents.id as string | ||
| workspaceId: string | null; | ||
| bounds: { | ||
| x: number; | ||
| y: number; | ||
| width: number; | ||
| height: number; | ||
| }; | ||
| } | ||
|
|
||
| interface WindowStateConfig { | ||
| windows: WindowState[]; | ||
| } | ||
|
|
||
| class WindowStateManager { | ||
| private static instance: WindowStateManager; | ||
| private statePath: string; | ||
| private stateDir: string; | ||
|
|
||
| private constructor() { | ||
| this.stateDir = path.join(os.homedir(), ".superset"); | ||
| this.statePath = path.join(this.stateDir, "window-state.json"); | ||
| this.ensureStateExists(); |
There was a problem hiding this comment.
Persist window state under Electron’s userData directory.
Line 27 writes into ~/.superset, which ignores custom --user-data-dir values, portable installs, and per-instance sandboxes. All instances on the same machine will now trample the same JSON file, so secondary instances can corrupt or resurrect the wrong windows. Please resolve the base path via app.getPath("userData") (or accept an injected override) before app initialization. (electronjs.org)
Apply this diff:
-import os from "node:os";
+import { app } from "electron";
import path from "node:path";
import type { BrowserWindow } from "electron";
@@
- this.stateDir = path.join(os.homedir(), ".superset");
- this.statePath = path.join(this.stateDir, "window-state.json");
+ const userDataDir = app.getPath("userData");
+ this.stateDir = path.join(userDataDir, "window-state");
+ this.statePath = path.join(this.stateDir, "window-state.json");🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/window-state-manager.ts around lines 1 to 29, the
code currently hardcodes ~/.superset for persisting window state which ignores
Electron's userData and can cause different instances to share the same file;
replace that with Electron's app.getPath("userData") or accept a path override
injected into the WindowStateManager constructor (use the injected value if
provided, otherwise call app.getPath("userData") and fall back to the previous
homedir path as a last resort), import app from "electron" at the top, and
ensure the manager is constructed after or in a phase where app is available so
the resolved stateDir/statePath are set from the proper userData directory.
| <Dialog open={isOpen} onOpenChange={() => { }}> | ||
| <DialogContent | ||
| className="max-w-[600px] max-h-[80vh] flex flex-col" | ||
| className="max-w-[900px] max-h-[85vh] flex flex-col p-0 bg-[#1e1e1e] border-neutral-800" | ||
| showCloseButton={false} | ||
| > |
There was a problem hiding this comment.
Modal cannot be closed by the user.
The combination of onOpenChange={() => { }} (empty handler), showCloseButton={false}, and no other close mechanism traps users in the modal. If workspace selection fails or the user wants to cancel, they have no way to dismiss the dialog.
Consider either:
- Providing a close/cancel button if users should be able to dismiss the modal
- If the modal must be blocking, ensure all workflows (action cards, workspace selection) have robust error handling to prevent users from being permanently stuck
Apply this diff to add a close callback prop and enable dismissal:
interface WorkspaceSelectionModalProps {
isOpen: boolean;
workspaces: Workspace[];
onSelectWorkspace: (workspaceId: string) => void;
onCreateWorkspace: () => void;
+ onClose?: () => void;
}
export function WorkspaceSelectionModal({
isOpen,
workspaces,
onSelectWorkspace,
onCreateWorkspace,
+ onClose,
}: WorkspaceSelectionModalProps) {
// ...
return (
- <Dialog open={isOpen} onOpenChange={() => { }}>
+ <Dialog open={isOpen} onOpenChange={(open) => { if (!open && onClose) onClose(); }}>
<DialogContent
className="max-w-[900px] max-h-[85vh] flex flex-col p-0 bg-[#1e1e1e] border-neutral-800"
- showCloseButton={false}
+ showCloseButton={!!onClose}
>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx
around lines 32-36, the Dialog is currently rendered with onOpenChange={() => {
}} and showCloseButton={false}, which prevents users from dismissing the modal;
change the component to accept a close callback prop (e.g., onClose or
onOpenChange) and wire it to the Dialog's onOpenChange so closing the dialog
toggles parent state, and enable a visible close affordance (either set
showCloseButton to true or add a Cancel/Close button that calls the new onClose
prop); ensure any existing flows that must block keep robust error handling but
still surface a dismiss option for failures.
| {workspaces.length > 5 && ( | ||
| <button | ||
| type="button" | ||
| className="text-xs text-neutral-400 hover:text-neutral-300 transition-colors" | ||
| > | ||
| View all ({workspaces.length}) | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
"View all" button is non-functional.
The "View all ({workspaces.length})" button renders when there are more than 5 workspaces but has no onClick handler. Users will click it expecting to see all workspaces, but nothing happens.
Either implement the functionality or remove the button to avoid misleading users.
Apply this diff to remove the button temporarily:
<div className="flex items-center justify-between mb-4">
<h3 className="text-sm font-medium text-neutral-300">
Recent projects
</h3>
- {workspaces.length > 5 && (
- <button
- type="button"
- className="text-xs text-neutral-400 hover:text-neutral-300 transition-colors"
- >
- View all ({workspaces.length})
- </button>
- )}
</div>Or add an onClick handler if "View all" functionality is intended for this release:
{workspaces.length > 5 && (
<button
type="button"
+ onClick={onViewAllWorkspaces}
className="text-xs text-neutral-400 hover:text-neutral-300 transition-colors"
>
View all ({workspaces.length})
</button>
)}📝 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.
| {workspaces.length > 5 && ( | |
| <button | |
| type="button" | |
| className="text-xs text-neutral-400 hover:text-neutral-300 transition-colors" | |
| > | |
| View all ({workspaces.length}) | |
| </button> | |
| )} | |
| {workspaces.length > 5 && ( | |
| <button | |
| type="button" | |
| className="text-xs text-neutral-400 hover:text-neutral-300 transition-colors" | |
| > | |
| View all ({workspaces.length}) | |
| </button> | |
| )} |
🤖 Prompt for AI Agents
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx
around lines 104 to 111: the "View all ({workspaces.length})" button renders
when workspaces.length > 5 but has no onClick so it is non-functional; either
remove the button or implement its behavior — add a local state (e.g., showAll
boolean) and an onClick handler that toggles showAll, then use showAll to
control whether the component shows all workspaces or only the first 5; if you
prefer a temporary fix for this release, remove the button markup entirely to
avoid misleading users.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx (1)
30-32: Simplify redundant border classes.The border classes contain redundancy:
border-x border-r:border-xalready applies left and right borders, makingborder-rredundantborder-b border-b-black: Can be simplified to justborder-b-blackApply this diff to clean up the classes:
- ${isSelected - ? "text-white border-t border-x border-r border-b border-b-black -mb-px" - : "text-neutral-400 hover:text-neutral-200 hover:bg-neutral-800/50" + ${isSelected + ? "text-white border-t border-x border-b-black -mb-px" + : "text-neutral-400 hover:text-neutral-200 hover:bg-neutral-800/50"
Inconsistency between PR summary and file changes.
The AI-generated PR summary describes "window restoration: persisted per-window state (workspace and bounds)... WindowManager and WindowStateManager... IPCs and renderer logic" but this file only contains UI styling changes to tab buttons. The summary appears to describe changes from different files not included in this review.
apps/desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx (1)
202-217: Tab content wrapper spacing looks intentional; consider consolidating padding/marginsThe added
rounded-sm m-2plus existingp-2gives the base tab content a “card” feel, which seems aligned with the tabs UI update. Given the parent container already hasp-2, you might want to confirm this doesn’t produce more outer spacing than intended; if it does, consider consolidating spacing between the parent and this wrapper.apps/desktop/src/renderer/screens/main/MainScreen.tsx (1)
167-190:gap-2has no effect without a flex/grid displayOn Line 167 the container uses
gap-2but isn’tflex/grid, so the gap currently does nothing. If you plan to lay out multiple children here, consider addingflex(orgrid); otherwise you can removegap-2to avoid confusion.apps/desktop/src/shared/ipc-channels/window.ts (1)
1-17: IPC channel typings are clear and consistent
WindowChannelsmatches the established IPC typing pattern (NoRequest,SuccessResponse) and adds a simplebooleanfor"window-is-restored", which should be easy to consume on both sides. If you later need more metadata around restoration, you might promote the boolean into a small response object, but this is perfectly fine for now.apps/desktop/src/shared/ipc-channels/proxy.ts (1)
1-17: Proxy channel typing looks good; consider a named status typeThe
"proxy-get-status"channel definition is clear and matches the shared IPC conventions. For readability and reuse, you might consider extracting the status object into a named type (e.g.,ProxyStatus) if it’s used in multiple places, but that’s optional.apps/desktop/src/shared/ipc-channels.ts (1)
1-5: Re-export strategy preserves IPC public surfaceRe-exporting everything from
./ipc-channels/indexis a reasonable way to keep existingipc-channelsimports working while moving definitions into modular files. Using the explicit./ipc-channels/indexpath also avoids resolution conflicts with this file. Just be aware thatexport *will expose any new symbols added to the modular index in the future, which is usually fine for a typed IPC barrel.apps/desktop/src/shared/ipc-channels/types.ts (1)
1-27: Shared IPC types are coherent; minor duplication is acceptable
IpcResponse<T>,NoRequest,NoResponse, andSuccessResponseform a consistent set of primitives and match how they’re consumed in the other channel definitions. There is some overlap betweenIpcResponse<T>andSuccessResponse(both carrysuccessand optionalerror), but that’s a minor stylistic duplication rather than a correctness issue and can be left as-is unless you want a single canonical envelope type later.apps/desktop/src/shared/ipc-channels/tab.ts (1)
60-67: Consider aligningmosaicTreetyping withTab.mosaicTree
Tabin../typesusesmosaicTree?: MosaicNode<string>, while"tab-update-mosaic-tree"takesmosaicTree: MosaicNode<string> | null | undefined. Unless you explicitly need to distinguishnullvsundefinedvs “field absent”, you could simplify and align with the Tab shape (e.g.,mosaicTree?: MosaicNode<string> | null) to reduce the number of possible states you have to handle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/desktop/src/renderer/screens/main/MainScreen.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/AppFrame/AppFrame.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/PRActions.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/SidebarToggle.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx(1 hunks)apps/desktop/src/shared/ipc-channels.ts(1 hunks)apps/desktop/src/shared/ipc-channels/deep-link.ts(1 hunks)apps/desktop/src/shared/ipc-channels/external.ts(1 hunks)apps/desktop/src/shared/ipc-channels/index.ts(1 hunks)apps/desktop/src/shared/ipc-channels/proxy.ts(1 hunks)apps/desktop/src/shared/ipc-channels/tab.ts(1 hunks)apps/desktop/src/shared/ipc-channels/terminal.ts(1 hunks)apps/desktop/src/shared/ipc-channels/types.ts(1 hunks)apps/desktop/src/shared/ipc-channels/window.ts(1 hunks)apps/desktop/src/shared/ipc-channels/workspace.ts(1 hunks)apps/desktop/src/shared/ipc-channels/worktree.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
apps/desktop/src/shared/ipc-channels/window.ts (2)
apps/desktop/src/shared/ipc-channels/index.ts (2)
NoRequest(20-20)SuccessResponse(22-22)apps/desktop/src/shared/ipc-channels/types.ts (2)
NoRequest(17-17)SuccessResponse(27-27)
apps/desktop/src/shared/ipc-channels/types.ts (1)
apps/desktop/src/shared/ipc-channels/index.ts (4)
IpcResponse(19-19)NoRequest(20-20)NoResponse(21-21)SuccessResponse(22-22)
apps/desktop/src/shared/ipc-channels/deep-link.ts (2)
apps/desktop/src/shared/ipc-channels/index.ts (1)
NoRequest(20-20)apps/desktop/src/shared/ipc-channels/types.ts (1)
NoRequest(17-17)
apps/desktop/src/shared/ipc-channels/proxy.ts (2)
apps/desktop/src/shared/ipc-channels/index.ts (1)
NoRequest(20-20)apps/desktop/src/shared/ipc-channels/types.ts (1)
NoRequest(17-17)
apps/desktop/src/shared/ipc-channels/external.ts (1)
apps/desktop/src/shared/ipc-channels/types.ts (3)
NoResponse(22-22)NoRequest(17-17)SuccessResponse(27-27)
apps/desktop/src/shared/ipc-channels/worktree.ts (2)
apps/desktop/src/shared/types.ts (2)
CreateWorktreeInput(95-103)Worktree(58-68)apps/desktop/src/shared/ipc-channels/types.ts (2)
IpcResponse(8-12)SuccessResponse(27-27)
apps/desktop/src/shared/ipc-channels/tab.ts (2)
apps/desktop/src/shared/types.ts (4)
CreateTabInput(105-115)Tab(44-56)UpdatePreviewTabInput(117-122)MosaicNode(35-35)apps/desktop/src/shared/ipc-channels/types.ts (1)
IpcResponse(8-12)
apps/desktop/src/shared/ipc-channels/terminal.ts (2)
apps/desktop/src/shared/ipc-channels/index.ts (1)
NoResponse(21-21)apps/desktop/src/shared/ipc-channels/types.ts (1)
NoResponse(22-22)
apps/desktop/src/shared/ipc-channels/workspace.ts (2)
apps/desktop/src/shared/ipc-channels/types.ts (2)
NoRequest(17-17)IpcResponse(8-12)apps/desktop/src/shared/types.ts (3)
Workspace(70-82)CreateWorkspaceInput(89-93)UpdateWorkspaceInput(124-127)
apps/desktop/src/shared/ipc-channels/index.ts (8)
apps/desktop/src/shared/ipc-channels/workspace.ts (1)
WorkspaceChannels(12-113)apps/desktop/src/shared/ipc-channels/worktree.ts (1)
WorktreeChannels(8-184)apps/desktop/src/shared/ipc-channels/tab.ts (1)
TabChannels(8-69)apps/desktop/src/shared/ipc-channels/terminal.ts (1)
TerminalChannels(7-42)apps/desktop/src/shared/ipc-channels/proxy.ts (1)
ProxyChannels(7-17)apps/desktop/src/shared/ipc-channels/external.ts (1)
ExternalChannels(7-17)apps/desktop/src/shared/ipc-channels/deep-link.ts (1)
DeepLinkChannels(7-12)apps/desktop/src/shared/ipc-channels/window.ts (1)
WindowChannels(7-17)
🔇 Additional comments (10)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/SidebarToggle.tsx (1)
17-17: PR summary describes window restoration, but this file only contains a UI spacing change.The PR-level summary describes adding window restoration functionality with WindowManager, WindowStateManager, and IPC logic, but this file only adds
pt-1padding. The spacing adjustment itself looks fine and aligns with the broader layout changes mentioned in the file summary.apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx (1)
28-28: PR summary describes window restoration, but this file only contains UI spacing adjustments.The PR-level summary describes adding window restoration functionality, but this file only adjusts padding (
pl-14→pl-16andpx-2→px-1). The spacing changes themselves are fine and appear coordinated with broader layout adjustments across the desktop UI.Also applies to: 32-32
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/PRActions.tsx (1)
29-29: LGTM! Consistent button sizing applied.Both PR action buttons now have consistent sizing with
h-6height andtext-xstext, providing visual uniformity across the PR actions UI.Also applies to: 51-51
apps/desktop/src/renderer/screens/main/components/AppFrame/AppFrame.tsx (1)
7-9: Frame layout simplification looks goodDropping the padding and gap here and pushing spacing into inner layout containers makes
AppFramea cleaner, responsibility-focused shell. No issues from a layout or structure standpoint.apps/desktop/src/shared/ipc-channels/external.ts (1)
1-17: External IPC channel contracts look consistent
ExternalChannelscleanly reuses the sharedNoRequest,NoResponse, andSuccessResponsehelpers, and the request/response shapes for"open-external"and"open-app-settings"are straightforward and coherent with typical Electron IPC usage. No issues from a typing or API-shape perspective.apps/desktop/src/shared/ipc-channels/deep-link.ts (1)
1-12: Deep link channel typing is clear and adequate
DeepLinkChannels["deep-link-get-url"]correctly models a no-payload request and a nullable URL response; it composes cleanly withNoRequestand fits the IPC pattern established in this PR.apps/desktop/src/shared/ipc-channels/terminal.ts (1)
1-42: Terminal IPC channel typings are consistent and non-breaking
TerminalChannelsaccurately captures the expected payloads for create/execute/history/resize/signal/detach operations, reusingNoResponsewhere appropriate and returning a terminal id for"terminal-create". The shapes look internally consistent and in line with typical terminal IPC contracts, with no obvious type or API issues.apps/desktop/src/shared/ipc-channels/worktree.ts (1)
8-183: Worktree IPC contracts look consistent and completeThe WorktreeChannels interface is cohesive: request shapes are consistent (workspaceId/worktreeId where expected), Git status/diff/PR operations have clear, strongly-typed payloads, and error/success flags align with the shared IPC patterns. I don’t see correctness or typing issues here.
apps/desktop/src/shared/ipc-channels/workspace.ts (1)
12-113: Workspace IPC typings look solid and consistentThe WorkspaceChannels definitions are coherent with the shared
Workspace/CreateWorkspaceInput/UpdateWorkspaceInputtypes and follow the same request/response patterns used elsewhere (including selection and window workspace state for restoration). I don’t see any correctness or typing issues here.apps/desktop/src/shared/ipc-channels/index.ts (1)
57-70:isValidChannel/getAllChannelNamesare unused and brokenThese functions are never called or imported anywhere in the codebase, making them dead code. While the reviewer's technical assessment is correct—
Object.keys({} as IpcChannels)returns[]at runtime due to TypeScript type erasure—there is no actual impact since nothing relies on them.Since these are exported but unused, you have two options:
- Remove them entirely if they're not needed
- Fix and use them if they were intended as helpers by replacing the type-cast approach with a real runtime constant (see original review for the suggested pattern)
Recommend clarifying the intent and removing the dead code if no longer needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/ModeContent.tsx (1)
5-11: UnusedisActiveprop in the component signature.The
isActiveprop is declared in theModeContentPropsinterface (line 7) but is not destructured or used in the component implementation (line 11). This creates a misleading API where callers can pass the prop, but it has no effect on the component's behavior.Recommended actions:
- If
isActiveis intended for future use, add a TODO comment and consider using it to conditionally style or control the component.- If it's not needed, remove it from the interface to keep the API surface clean.
Apply this diff if implementing active state styling:
-export function ModeContent({ mode, children }: ModeContentProps) { +export function ModeContent({ mode, isActive, children }: ModeContentProps) { return ( <div - className="overflow-y-auto h-full" + className={`overflow-y-auto h-full ${isActive ? 'active-mode' : ''}`} style={{ scrollSnapAlign: "start", scrollSnapStop: "always", }} >Or apply this diff to remove the unused prop:
interface ModeContentProps { mode: SidebarMode; - isActive: boolean; children: ReactNode; }
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/ModeContent.tsx (1)
21-21: Padding reduction frompx-3topx-1.The horizontal padding has been reduced from 12px to 4px, making the content container more compact. Ensure this spacing aligns with the overall design system and doesn't create visual inconsistencies with other components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/ModeContent.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx (1)
apps/desktop/src/renderer/screens/main/components/DiffView/FileTree.tsx (1)
FileTree(169-190)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/constants.ts (1)
modeLabels(9-12)
🔇 Additional comments (4)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx (3)
1-3: LGTM! Clean imports and type-safe label retrieval.The Tooltip component imports and
modeLabelsconstant are properly integrated. The label lookup at line 31 is type-safe sincemodeLabelsis defined asRecord<SidebarMode, string>, ensuring all mode keys are present.Also applies to: 31-31
21-21: Verify the padding change is intentional.The container padding changed from
py-2(vertical padding) topt-2(top padding only), removing the bottom padding. Ensure this aligns with the intended design and doesn't negatively impact vertical spacing.
34-50: LGTM! Well-implemented tooltip enhancement.The Tooltip integration properly uses the
asChildpattern withTooltipTrigger, preserving the original button behavior while adding hover labels. The implementation is clean and follows React 19 best practices.apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx (1)
359-366: LGTM! Clean UI simplification.The streamlined structure directly wraps the FileTree in a scrollable container with appropriate padding, removing unnecessary nesting. The component props are correctly provided and the conditional rendering logic remains sound.
1 participant
Summary by CodeRabbit
New Features
Behavior Changes
Style