Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions apps/desktop/src/lib/electron-app/factories/app/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import { ignoreConsoleWarnings } from "../../utils/ignore-console-warnings";

ignoreConsoleWarnings(["Manifest version 2 is deprecated"]);

export async function makeAppSetup(createWindow: () => Promise<BrowserWindow>) {
export async function makeAppSetup(
createWindow: () => Promise<BrowserWindow>,
restoreWindows?: () => Promise<void>,
) {
if (ENVIRONMENT.IS_DEV) {
try {
await installExtension([REACT_DEVELOPER_TOOLS], {
Expand All @@ -24,7 +27,19 @@ export async function makeAppSetup(createWindow: () => Promise<BrowserWindow>) {
}
}

let window = await createWindow();
// Restore windows from previous session if available
if (restoreWindows) {
await restoreWindows();
}

// If no windows were restored, create a new one
const existingWindows = BrowserWindow.getAllWindows();
let window: BrowserWindow;
if (existingWindows.length > 0) {
window = existingWindows[0];
} else {
window = await createWindow();
}

app.on("activate", async () => {
const windows = BrowserWindow.getAllWindows();
Expand Down
7 changes: 6 additions & 1 deletion apps/desktop/src/main/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ app.on("open-url", (event, url) => {
registerWorkspaceIPCs();
registerPortIpcs();
registerDeepLinkIpcs();
const { registerWindowIPCs } = await import("main/lib/window-ipcs");
registerWindowIPCs();

await makeAppSetup(() => windowManager.createWindow());
await makeAppSetup(
() => windowManager.createWindow(),
() => windowManager.restoreWindows(),
);
})();
10 changes: 9 additions & 1 deletion apps/desktop/src/main/lib/window-ipcs.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ipcMain } from "electron";
import { BrowserWindow, ipcMain } from "electron";
import windowManager from "./window-manager";

export function registerWindowIPCs() {
Expand All @@ -14,4 +14,12 @@ export function registerWindowIPCs() {
};
}
});

ipcMain.handle("window-is-restored", async (event) => {
const senderWindow = BrowserWindow.fromWebContents(event.sender);
if (!senderWindow) {
return false;
}
return windowManager.isRestoredWindow(senderWindow);
});
}
121 changes: 118 additions & 3 deletions apps/desktop/src/main/lib/window-manager.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,97 @@
import type { BrowserWindow } from "electron";
import { MainWindow } from "../windows/main";
import windowStateManager from "./window-state-manager";

class WindowManager {
private windows: Set<BrowserWindow> = new Set();
private windowWorkspaces: Map<BrowserWindow, string | null> = new Map();
private restoredWindowIds: Set<number> = new Set();

async createWindow(): Promise<BrowserWindow> {
async createWindow(
restoreState?: { workspaceId: string | null; bounds?: Electron.Rectangle },
): Promise<BrowserWindow> {
const window = await MainWindow();

// Restore window bounds if provided
if (restoreState?.bounds) {
window.setBounds(restoreState.bounds);
}

this.windows.add(window);
// New windows start with no workspace - user must select one
this.windowWorkspaces.set(window, null);
const workspaceId = restoreState?.workspaceId ?? null;
this.windowWorkspaces.set(window, workspaceId);

// Mark as restored if we're restoring state
if (restoreState) {
this.restoredWindowIds.add(window.webContents.id);
}

// Save window state when workspace changes
if (workspaceId) {
windowStateManager.saveWindowState(window, workspaceId);
}

// Store window ID before it might be destroyed
const windowId = window.webContents.id;

// 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)
Comment on lines +37 to +55
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

// Get workspace ID from our map before window might be destroyed
const workspaceId = this.windowWorkspaces.get(window) ?? null;

try {
if (!window.isDestroyed()) {
const bounds = window.getBounds();
// Save using window ID to avoid issues if window is destroyed
windowStateManager.saveWindowStateById(windowId, workspaceId, bounds);
} else {
// Window already destroyed, use last known bounds from state
if (workspaceId) {
const lastState = windowStateManager.getWindowState(windowId);
if (lastState) {
windowStateManager.saveWindowStateById(
windowId,
workspaceId,
lastState.bounds,
);
}
}
}
} catch (error) {
// Silently fail if window is destroyed - we'll clean up in closed handler
if (!(error instanceof Error && error.message.includes("destroyed"))) {
console.error("[WindowManager] Failed to save window state on close:", error);
}
}
});

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);
});
Comment on lines 85 to 95
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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.


return window;
Expand All @@ -33,6 +111,43 @@ class WindowManager {

setWorkspaceForWindow(window: BrowserWindow, workspaceId: string | null): void {
this.windowWorkspaces.set(window, workspaceId);
// Persist the workspace association
windowStateManager.saveWindowState(window, workspaceId);
}

isRestoredWindow(window: BrowserWindow): boolean {
return this.restoredWindowIds.has(window.webContents.id);
}

async restoreWindows(): Promise<void> {
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,
);
}
}
Comment on lines +123 to +150
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

}
}

Expand Down
160 changes: 160 additions & 0 deletions apps/desktop/src/main/lib/window-state-manager.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
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();
Comment on lines +1 to +29
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

}

static getInstance(): WindowStateManager {
if (!WindowStateManager.instance) {
WindowStateManager.instance = new WindowStateManager();
}
return WindowStateManager.instance;
}

private ensureStateExists(): void {
// Create directory if it doesn't exist
if (!existsSync(this.stateDir)) {
mkdirSync(this.stateDir, { recursive: true });
}

// Create state file with default structure if it doesn't exist
if (!existsSync(this.statePath)) {
const defaultState: WindowStateConfig = {
windows: [],
};
writeFileSync(
this.statePath,
JSON.stringify(defaultState, null, 2),
"utf-8",
);
}
}

read(): WindowStateConfig {
try {
const content = readFileSync(this.statePath, "utf-8");
return JSON.parse(content) as WindowStateConfig;
} catch (error) {
console.error("Failed to read window state:", error);
return { windows: [] };
}
}

write(state: WindowStateConfig): boolean {
try {
writeFileSync(this.statePath, JSON.stringify(state, null, 2), "utf-8");
return true;
} catch (error) {
console.error("Failed to write window state:", error);
return false;
}
}

saveWindowState(window: BrowserWindow, workspaceId: string | null): void {
// Check if window is destroyed before accessing properties
if (window.isDestroyed()) {
return;
}

try {
const state = this.read();
const windowId = String(window.webContents.id);
const bounds = window.getBounds();

const existingIndex = state.windows.findIndex((w) => w.id === windowId);
const windowState: WindowState = {
id: windowId,
workspaceId,
bounds,
};

if (existingIndex >= 0) {
state.windows[existingIndex] = windowState;
} else {
state.windows.push(windowState);
}

this.write(state);
} catch (error) {
// Window might be destroyed between check and access
if (error instanceof Error && error.message.includes("destroyed")) {
return;
}
console.error("[WindowStateManager] Failed to save window state:", error);
}
}

saveWindowStateById(
windowId: number,
workspaceId: string | null,
bounds: Electron.Rectangle,
): void {
try {
const state = this.read();
const id = String(windowId);
const windowState: WindowState = {
id,
workspaceId,
bounds,
};

const existingIndex = state.windows.findIndex((w) => w.id === id);
if (existingIndex >= 0) {
state.windows[existingIndex] = windowState;
} else {
state.windows.push(windowState);
}

this.write(state);
} catch (error) {
console.error("[WindowStateManager] Failed to save window state by ID:", error);
}
}

removeWindowState(windowId: number): void {
const state = this.read();
state.windows = state.windows.filter((w) => w.id !== String(windowId));
this.write(state);
}

getWindowStates(): WindowState[] {
return this.read().windows;
}

getWindowState(windowId: number): WindowState | undefined {
const state = this.read();
return state.windows.find((w) => w.id === String(windowId));
}

clearAll(): void {
this.write({ windows: [] });
}
}

export default WindowStateManager.getInstance();

2 changes: 1 addition & 1 deletion apps/desktop/src/renderer/screens/main/MainScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ export function MainScreen() {
/>

{/* Main content area - conditionally render based on mode */}
<div className="flex-1 overflow-hidden">
<div className="flex-1 overflow-hidden p-2 gap-2">
<MainContentArea
mode={mode}
loading={loading}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ interface AppFrameProps {

export function AppFrame({ children }: AppFrameProps) {
return (
<div className="absolute inset-0 p-2 bg-stone-950 flex gap-2">
<div className="absolute inset-0 bg-stone-950 flex">
{children}
</div>
);
Expand Down
Loading
Loading