Fix crash in authentication flow caused by storage IPC errors#238
Fix crash in authentication flow caused by storage IPC errors#238Kitenite wants to merge 1 commit into
Conversation
This fixes critical bugs in the electron-store IPC layer that were causing crashes during authentication state persistence and app initialization. **Changes:** - Added error handling and input validation to all storage IPC handlers - Fixed type declarations for electronStore API to correctly reflect Promise-based operations - Added data validation in storage adapter to prevent crashes from corrupted data - Added comprehensive error logging for debugging storage issues **Bugs Fixed:** 1. Missing error handling in storage-ipcs.ts caused unhandled exceptions to crash the main process 2. Type mismatch in preload/index.ts declared synchronous functions but actual implementation was async 3. Unsafe type assertion in electron-storage.ts could crash on corrupted data 4. Missing input validation allowed malformed IPC calls to throw errors These issues manifested as crashes when: - App initializes and tries to hydrate authentication state - Storage operations fail due to file system errors - Corrupted data exists in electron-store - Multiple async storage operations happen simultaneously 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughEnhanced IPC storage handlers with input validation and error handling, and transitioned the electronStore API from synchronous to asynchronous Promise-based methods. Error-handling guards were added to the renderer storage adapter, preventing unhandled exceptions across the storage layer. Changes
Sequence DiagramsequenceDiagram
participant Renderer
participant Preload as Preload (IPC Interface)
participant Main as Main (IPC Handler)
participant Store as Electron Store
Note over Renderer,Store: Previous (Synchronous)
Renderer->>Preload: electronStore.get(key)
Preload->>Main: invoke('storage:get', key)
Main->>Store: store.get(key)
Store-->>Main: value
Main-->>Preload: value
Preload-->>Renderer: value (immediate)
Note over Renderer,Store: New (Asynchronous with Error Handling)
Renderer->>Preload: await electronStore.get(key)
Preload->>Main: invoke('storage:get', {key, ...validation})
rect rgb(200, 220, 255)
Note over Main: Input Validation
Main->>Main: Check input exists, key is string
end
rect rgb(200, 220, 255)
Note over Main: Try/Catch Protection
Main->>Store: store.get(key)
Store-->>Main: value
end
Main-->>Preload: Promise<value>
Preload-->>Renderer: value (awaited)
rect rgb(255, 200, 200)
Note over Main: Error Path
Main->>Main: Log error, return undefined
Main-->>Preload: Promise<undefined>
Preload-->>Renderer: undefined (awaited)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/preload/index.ts (1)
85-90: Replace raw IPC invocations with tRPC client calls.This implementation uses
ipcRenderer.invokewith custom channel names, which violates the coding guidelines requiring tRPC for all Electron IPC communication.Based on coding guidelines, these should be tRPC procedure calls rather than raw IPC invocations.
apps/desktop/src/renderer/lib/electron-storage.ts (1)
47-47: Note: This storage adapter relies on raw IPC implementation.While the adapter logic itself is well-implemented, it depends on
window.electronStorewhich uses raw IPC channels instead of tRPC (as flagged in the other files).Once the underlying IPC layer is migrated to tRPC per coding guidelines, this adapter should be updated to use the tRPC client.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/storage-ipcs.ts (1)
25-29: Consider validating the value parameter.While the key is validated, the
valueparameter is not checked. Althoughstore.setlikely handles various types, explicit validation would be more defensive.Consider adding validation:
try { if (!input || typeof input.key !== "string") { console.error("[storage:set] Invalid input:", input); return; } + if (input.value === undefined) { + console.error("[storage:set] Missing value for key:", input.key); + return; + } store.set(input.key, input.value); } catch (error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/main/lib/storage-ipcs.ts(1 hunks)apps/desktop/src/preload/index.ts(1 hunks)apps/desktop/src/renderer/lib/electron-storage.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/preload/index.tsapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/renderer/lib/electron-storage.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/preload/index.tsapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/renderer/lib/electron-storage.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code
Files:
apps/desktop/src/preload/index.tsapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/renderer/lib/electron-storage.ts
apps/desktop/src/main/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Node.js modules (fs, path, os, net, etc.) can be used in main process code only
Files:
apps/desktop/src/main/lib/storage-ipcs.ts
apps/desktop/src/main/lib/*-ipcs.ts
📄 CodeRabbit inference engine (AGENTS.md)
IPC handlers must accept a single object parameter, not positional parameters, to match type-safe renderer calls
Files:
apps/desktop/src/main/lib/storage-ipcs.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Files:
apps/desktop/src/renderer/lib/electron-storage.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/main/lib/*-ipcs.ts : IPC handlers must accept a single object parameter, not positional parameters, to match type-safe renderer calls
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/main/lib/*-ipcs.ts : IPC handlers must accept a single object parameter, not positional parameters, to match type-safe renderer calls
Applied to files:
apps/desktop/src/preload/index.tsapps/desktop/src/main/lib/storage-ipcs.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/renderer/**/*.tsx : Call IPC methods using `window.ipcRenderer.invoke()` with object parameters - TypeScript will infer the exact response type automatically
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define all IPC channel types with request and response interfaces before implementing handlers
Applied to files:
apps/desktop/src/main/lib/storage-ipcs.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Applied to files:
apps/desktop/src/renderer/lib/electron-storage.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/storage-ipcs.ts (1)
apps/desktop/src/main/lib/storage-manager.ts (1)
store(8-11)
⏰ 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 (2)
apps/desktop/src/renderer/lib/electron-storage.ts (2)
8-27: Excellent type validation and error handling in getItem.The implementation correctly:
- Validates the returned value type
- Returns
nullfor null/undefined/non-string values- Logs errors for corrupted data
- Wraps everything in try-catch to prevent crashes
- Returns
nullon IPC failuresThis defensive approach will prevent the crashes mentioned in the PR objectives.
29-43: Good error handling pattern for setItem and removeItem.Both methods correctly:
- Wrap IPC calls in try-catch blocks
- Log errors with context
- Don't throw exceptions, allowing Zustand to handle failures gracefully
This approach prevents unhandled exceptions from crashing the renderer process.
| 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); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 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 -20Repository: superset-sh/superset
Length of output: 1230
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/index.tsRepository: 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 tsRepository: 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/nullRepository: 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 tsRepository: 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.
| 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>; | ||
| }; |
There was a problem hiding this comment.
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.
Kitenite
deleted the
cmux/fix-bug-causing-crash-in-authentication-flow-pkvmq
branch
1 participant
Summary
This PR fixes critical bugs in the electron-store IPC layer that were causing crashes during authentication state persistence and app initialization.
Bugs Fixed
1. Missing Error Handling in Storage IPC Handlers (
apps/desktop/src/main/lib/storage-ipcs.ts)2. Type Mismatch in Window Interface (
apps/desktop/src/preload/index.ts)Promise<T>return types3. Unsafe Type Assertions (
apps/desktop/src/renderer/lib/electron-storage.ts)value as string | nullcould crash on corrupted data4. Missing Input Validation
Impact
These bugs manifested as crashes when:
Changes
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.