fix (desktop): git lock fix and duplicate input bug#263
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplace direct BrowserWindow use with a getWindow() getter across routers and main window lifecycle; add TerminalManager.detachAllListeners(); detect git lock-file errors in createWorktree; and refactor terminal focus/keyboard handling to use a ref and a returned cleanup function. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Renderer as Renderer (Terminal UI)
participant IPC as IPC Handler (main)
participant Main as Main Window Manager
participant Router as TRPC Routers
participant TerminalMgr as TerminalManager
Renderer->>IPC: send request (via trpc) -> e.g., open/terminal actions
IPC->>Main: ask for current window via getWindow()
Main-->>IPC: return BrowserWindow or null
IPC->>Router: route request (routers use getWindow())
alt window present
Router->>Main: may invoke window methods (minimize/maximize/...), using getWindow()
Router-->>Renderer: success response
else no window
Router-->>Renderer: short-circuit error/false result
end
Note over Main,TerminalMgr: On window close
Main->>TerminalMgr: detachAllListeners()
TerminalMgr-->>Main: listeners removed (terminals remain)
Main->>IPC: detach window-specific handlers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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: 0
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
182-199: Lock error detection is solid; consider preserving original error detailsThe new
isLockErrorheuristics capture the common git lock failure shapes and running this check before the LFS branch is the right call, so this should fix misclassified lock/LFS errors.One small improvement: the thrown
Errordrops the original message, which can make debugging harder. You could either append it to the message or use an errorcauseso upstream handlers and logs retain full context, e.g.:if (isLockError) { console.error( `Git lock file error during worktree creation: ${errorMessage}`, ); throw new Error( `Failed to create worktree: The git repository is locked by another process. ` + `This usually happens when another git operation is in progress, or a previous operation crashed. ` + `Please wait for the other operation to complete, or manually remove the lock file ` + `(e.g., .git/config.lock or .git/index.lock) if you're sure no git operations are running. ` + `Original error: ${errorMessage}`, // Or, if your TS/Node target supports it: // { cause: error instanceof Error ? error : undefined }, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(6 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
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/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
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/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{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/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/lib/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in shared code like
src/lib/electron-router-dom.ts- this code runs in both main and renderer processes
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.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/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.tsx: Create one folder per component with structure:ComponentName/ComponentName.tsx+index.tsfor barrel export
Co-locate tests next to the component file they test (e.g.,ComponentName.test.tsx)
Co-locate dependencies (utils, hooks, constants, config, stories) next to the file using them
Use nestedcomponents/subdirectory within a parent component only if a sub-component is used 2+ times within that parent; otherwise keep it in the parent'scomponents/folder
One component per file - avoid multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/renderer/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods using
window.ipcRenderer.invoke()with object parameters - TypeScript will infer the exact response type automatically
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧠 Learnings (1)
📚 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 **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🔇 Additional comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
137-213: Keyboard handler setup + cleanup look correct and resolve stale-handler riskDefining the handler locally, attaching it once via
attachCustomKeyEventHandler, and returning a cleanup that replaces it with a no-op is a clean way to avoid stale closures or duplicate handlers when terminals are torn down/recreated. The existing Shift+Enter, Cmd+K, and app-hotkey forwarding behavior is preserved while ensuring React’s effect cleanup can fully detach the behavior.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
101-109: Ref-based focus handler avoids unnecessary terminal recreationUsing
handleTerminalFocusRefto hold the latest focus callback and wiringsetupFocusListenerto callhandleTerminalFocusRef.current()is a good way to keep the effect’s dependency list small and avoid tearing down/recreating the terminal whenever pane focus logic changes, while still always using the latest pane/windowId.
250-261: Keyboard handler lifecycle is now correctly tied to the terminal effectCapturing the return from
setupKeyboardHandlerand invokingcleanupKeyboard()in the effect cleanup, alongsidecleanupFocus/cleanupResize, properly scopes the custom key handling to the lifetime of this xterm instance. This should prevent stale handlers and duplicate input when the terminal effect re-runs (e.g., under React StrictMode or when dependencies change), while keeping the main effect dependency array focused on terminal creation inputs only.Also applies to: 262-265, 275-279, 289-289
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (2)
98-101: New no-window path inopenNew– ensure caller handles theerrorvariantThe early return when
getWindow()is null avoids dialog calls without a window, which is good. It does, however, introduce a new result variant{ canceled: false, error: "No window available" }(noproject). Please confirm the consumer of this mutation distinguishes between “user canceled” vs. “system error” (presence oferror) and doesn’t assumecanceled === falsealways implies a validproject.If you want to tighten the API, you could consider a small follow-up that formalizes the result as a discriminated union (e.g.,
type = "success" | "canceled" | "error") to make these states explicit in the client.
176-183: Consistent no-window handling incloneRepo– consider de-duplicating the error shapeThe guard around
getWindow()in the “no targetDir” path looks correct and matches the other error-style returns (canceled: false, success: false, error: …). This should avoid unexpected dialog calls when no window is available.If this “No window available” case is used in multiple routes (here and in
openNew), you might consider a tiny helper or shared constant for the error message/shape so callers can handle it consistently and tests don’t need to duplicate the literal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/projects/projects.ts(3 hunks)apps/desktop/src/main/lib/terminal-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/terminal-manager.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/lib/trpc/routers/projects/projects.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/lib/trpc/routers/projects/projects.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/lib/trpc/routers/projects/projects.ts
apps/desktop/src/lib/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in shared code like
src/lib/electron-router-dom.ts- this code runs in both main and renderer processes
Files:
apps/desktop/src/lib/trpc/routers/projects/projects.ts
🧬 Code graph analysis (1)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
apps/desktop/src/lib/trpc/routers/projects/index.ts (1)
createProjectsRouter(2-2)
⏰ 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 (1)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
83-83: All call sites properly updated for thegetWindowcallback patternThe one call site in
apps/desktop/src/lib/trpc/routers/index.tscorrectly passes thegetWindowcallback tocreateProjectsRouter, matching the new signature. The type is properly threaded through fromcreateAppRouter's parameter.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Bug Fixes
Improvements
New Features
✏️ Tip: You can customize this high-level summary in your review settings.