Skip to content

Fix crash in authentication flow caused by storage IPC errors #238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Kitenite wants to merge 1 commit into from
+62 −10
Closed

Fix crash in authentication flow caused by storage IPC errors #238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 28 additions & 3 deletions apps/desktop/src/main/lib/storage-ipcs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,42 @@ import { store } from "./storage-manager";
*/
export function registerStorageHandlers() {
ipcMain.handle("storage:get", async (_event, input: { key: string }) => {
return store.get(input.key);
try {
if (!input || typeof input.key !== "string") {
console.error("[storage:get] Invalid input:", input);
return undefined;
}
return store.get(input.key);
} catch (error) {
console.error("[storage:get] Error getting value:", error);
return undefined;
}
});

ipcMain.handle(
"storage:set",
async (_event, input: { key: string; value: unknown }) => {
store.set(input.key, input.value);
try {
if (!input || typeof input.key !== "string") {
console.error("[storage:set] Invalid input:", input);
return;
}
store.set(input.key, input.value);
} catch (error) {
console.error("[storage:set] Error setting value:", error);
}
},
);

ipcMain.handle("storage:delete", async (_event, input: { key: string }) => {
store.delete(input.key);
try {
if (!input || typeof input.key !== "string") {
console.error("[storage:delete] Invalid input:", input);
return;
}
store.delete(input.key);
} catch (error) {
console.error("[storage:delete] Error deleting value:", error);
}
});
}
Comment on lines 8 to 48
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 2, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the tRPC router definition and example procedures
echo "=== Searching for tRPC router setup ==="
fd -e ts -e tsx "trpc" apps/desktop/src/lib/ apps/desktop/src/main/lib/ 2>/dev/null | head -20

echo -e "\n=== Looking for existing tRPC procedure examples ==="
rg -n -A 10 "\.procedure\(" apps/desktop/src/lib/ apps/desktop/src/main/lib/ 2>/dev/null | head -50

echo -e "\n=== Checking if other IPC handlers exist ==="
rg -n "ipcMain\.handle" apps/desktop/src/main/ -g "!storage-ipcs.ts" 2>/dev/null | head -20

echo -e "\n=== Checking tRPC directory structure ==="
fd . apps/desktop/src/lib/trpc 2>/dev/null | head -20

Repository: superset-sh/superset

Length of output: 1230

🏁 Script executed:

cat -n apps/desktop/src/lib/trpc/index.ts

Repository: superset-sh/superset

Length of output: 669

🏁 Script executed:

echo "=== Searching for 'storage' in tRPC routers ==="
rg -r "storage" apps/desktop/src/lib/trpc/ 2>/dev/null

echo -e "\n=== Checking all routers index to see what's exposed ==="
cat -n apps/desktop/src/lib/trpc/routers/index.ts

echo -e "\n=== Looking at one existing router (config) for pattern ==="
cat -n apps/desktop/src/lib/trpc/routers/config/config.ts | head -50

echo -e "\n=== Checking if storage-ipcs.ts is only storage implementation ==="
rg -l "storage" apps/desktop/src/main/ --type ts

Repository: superset-sh/superset

Length of output: 3416

🏁 Script executed:

echo "=== Checking how storage-ipcs is registered in main/index.ts ==="
rg -n -B 5 -A 5 "storage" apps/desktop/src/main/index.ts | head -40

echo -e "\n=== Checking the full storage-ipcs.ts file ==="
cat -n apps/desktop/src/main/lib/storage-ipcs.ts

echo -e "\n=== Checking if storage is used in renderer (settings router pattern) ==="
rg -n "storage:" apps/desktop/src/lib/trpc/routers/settings/ 2>/dev/null

Repository: superset-sh/superset

Length of output: 2413

🏁 Script executed:

echo "=== Check if there are any other raw ipcMain handlers besides storage ==="
rg -n "ipcMain\.handle\|ipcMain\.on" apps/desktop/src/main/lib/ --type ts

Repository: superset-sh/superset

Length of output: 137

Migrate storage operations to tRPC procedures instead of raw IPC handlers.

This implementation uses raw ipcMain.handle for storage operations, which violates the project's guideline requiring all Electron interprocess communication to use tRPC (defined in src/lib/trpc). All other domain operations (config, settings, projects, etc.) are properly exposed through tRPC procedures.

Storage should be implemented as a tRPC router with get, set, and delete procedures following the existing pattern in other routers (e.g., apps/desktop/src/lib/trpc/routers/config/config.ts). The error handling improvements here are valuable but cannot substitute for proper architectural alignment with the established tRPC infrastructure.

6 changes: 3 additions & 3 deletions apps/desktop/src/preload/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ declare global {
App: typeof API;
ipcRenderer: typeof ipcRendererAPI;
electronStore: {
get: (key: string) => unknown;
set: (key: string, value: unknown) => void;
delete: (key: string) => void;
get: (key: string) => Promise<unknown>;
set: (key: string, value: unknown) => Promise<void>;
delete: (key: string) => Promise<void>;
};
Comment on lines 13 to 17
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 2, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Type declarations correctly updated to async, but underlying IPC should use tRPC.

The change from synchronous to Promise-based return types is correct and aligns with the async IPC implementation. However, this entire electronStore API should be implemented using tRPC instead of raw IPC channels.

Based on coding guidelines, all Electron IPC should use tRPC as defined in src/lib/trpc. The raw IPC approach bypasses the type-safety and structure that tRPC provides.

🤖 Prompt for AI Agents 
In apps/desktop/src/preload/index.ts around lines 13 to 17, the electronStore
methods were changed to async but still rely on raw IPC; replace those raw IPC
calls with tRPC client calls from src/lib/trpc so the API uses the app-wide
typed RPC layer. Import and initialize the tRPC client appropriate for the
preload context, then implement electronStore.get/set/delete to call the
corresponding tRPC procedures (e.g., query/mutation names used by your backend)
and return their Promises; remove any ipcRenderer.invoke usage for these methods
and ensure the exposed types remain Promise<unknown>/Promise<void> and preserve
error propagation.

webUtils: {
getPathForFile: (file: File) => string;
Expand Down
35 changes: 31 additions & 4 deletions apps/desktop/src/renderer/lib/electron-storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,41 @@ import { createJSONStorage } from "zustand/middleware";
*/
const electronStorageAdapter = {
getItem: async (name: string): Promise<string | null> => {
const value = await window.electronStore.get(name);
return value as string | null;
try {
const value = await window.electronStore.get(name);

// Validate that the value is either null, undefined, or a string
if (value === null || value === undefined) {
return null;
}

if (typeof value === "string") {
return value;
}

// If we got a non-string value, it's corrupted data
console.error("[electronStorage] Corrupted data for key:", name, "Expected string, got:", typeof value);
return null;
} catch (error) {
console.error("[electronStorage] Failed to get item:", name, error);
return null;
}
},
setItem: async (name: string, value: string): Promise<void> => {
await window.electronStore.set(name, value);
try {
await window.electronStore.set(name, value);
} catch (error) {
console.error("[electronStorage] Failed to set item:", name, error);
// Don't throw - let Zustand handle the failure gracefully
}
},
removeItem: async (name: string): Promise<void> => {
await window.electronStore.delete(name);
try {
await window.electronStore.delete(name);
} catch (error) {
console.error("[electronStorage] Failed to remove item:", name, error);
// Don't throw - let Zustand handle the failure gracefully
}
},
};

Expand Down
Loading