tooltip for delete tab + kill processes when delete workspace#254
tooltip for delete tab + kill processes when delete workspace#254
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds terminal-aware workspace deletion: UI tooltip on the delete button, dialog polling revealing active terminal counts, TerminalManager methods to count and kill sessions by workspace, and TRPC canDelete/delete integration to surface counts, warnings, and to terminate terminals before workspace removal. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer as Renderer (React)
participant TRPC as TRPC Router
participant Main as TerminalManager
participant DB as Database
User->>Renderer: open DeleteWorkspaceDialog
Renderer->>TRPC: canDelete(workspaceId) [polled every 2s]
TRPC->>Main: getSessionCountByWorkspaceId(workspaceId)
Main-->>TRPC: activeTerminalCount
TRPC-->>Renderer: { canDelete, reason, activeTerminalCount }
alt User confirms delete
Renderer->>TRPC: delete(workspaceId)
TRPC->>Main: killByWorkspaceId(workspaceId)
Main-->>TRPC: {killed, failed}
TRPC->>DB: remove workspace/worktree records
DB-->>TRPC: deletion result
TRPC-->>Renderer: delete result (includes terminalWarning/teardownError)
Renderer->>User: show success/warnings
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
176-198: Tooltip‑wrapped delete button looks good; consider more descriptive labelsWrapping the delete icon button in a tooltip preserves the existing click behavior and improves discoverability. For better accessibility and clarity (especially for screen readers and when multiple tabs are present), consider including the workspace title in both the aria label and tooltip text:
- <Button + <Button type="button" variant="ghost" size="icon" @@ - aria-label="Delete workspace" + aria-label={`Delete workspace ${title}`} > <HiMiniXMark /> </Button> </TooltipTrigger> - <TooltipContent side="bottom" showArrow={false}> - Delete workspace - </TooltipContent> + <TooltipContent side="bottom" showArrow={false}> + {`Delete workspace "${title}"`} + </TooltipContent>This keeps behavior the same while making the control’s target more explicit.
Please verify that interpolating
titlehere doesn’t conflict with any existing i18n strategy and that the tooltip renders as expected for long workspace names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(2 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/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.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/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.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/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.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/TopBar/WorkspaceTabs/WorkspaceItem.tsx
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/TopBar/WorkspaceTabs/WorkspaceItem.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/TopBar/WorkspaceTabs/WorkspaceItem.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/TopBar/WorkspaceTabs/WorkspaceItem.tsx
⏰ 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/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
3-3: Tooltip import is minimal and consistent with new usageThe added
Tooltip,TooltipTrigger, andTooltipContentimport is used directly below and keeps the diff tight with no unused symbols or style violations.Please sanity‑check against other
@superset/ui/tooltipusages in the repo to ensure pattern consistency (e.g.,Tooltip→TooltipTrigger asChild→TooltipContent).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
637-638: Avoid hardcoded timeouts in tests for better reliability.The use of
setTimeout(resolve, 100)introduces timing dependencies that can make tests flaky on slower CI systems or under load. Consider using event-driven approaches or mocking time.For instance, at lines 637-638, the 100ms wait is used to ensure cleanup completes. Instead of waiting, you could:
- Listen for specific cleanup events if the API supports it
- Use mock timers (e.g.,
jest.useFakeTimers()or bun's equivalent)- Make the test await the actual cleanup promise if exposed
As per learnings, tests should be repeatable and reliable across different environments.
Also applies to: 668-668, 730-731
apps/desktop/src/main/lib/terminal-manager.ts (1)
351-360: Document the forced cleanup behavior.When a process doesn't respond to SIGKILL within 500ms (line 348-361), the code forcibly marks
isAlive = falseand deletes the session, but logs that "Process may still be running." This is the correct defensive approach, but the implications should be clearly documented.Consider adding a JSDoc comment above
killByWorkspaceIdexplaining:
- The termination stages and timeouts
- What happens if a process can't be killed
- Potential for orphaned processes
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
354-356: Consider more informative terminal warning message.The current warning only reports failed kills:
"${terminalResult.failed} terminal process(es) may still be running".If
killed=5andfailed=2, users might benefit from knowing that 5 terminals were successfully terminated, and 2 may still be running. This provides better context for the warning.Consider:
const terminalWarning = terminalResult.failed > 0 - ? `${terminalResult.failed} terminal process(es) may still be running` + ? `Terminated ${terminalResult.killed} terminal(s); ${terminalResult.failed} process(es) may still be running` : undefined;Also applies to: 433-438
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (1)
35-36: Consider extracting polling interval as a named constant.The
2000ms refetch interval is a magic number that appears only once but represents a UX decision about terminal count freshness.For better maintainability:
+const TERMINAL_COUNT_POLL_INTERVAL_MS = 2000; + const { data: canDeleteData, isLoading } = trpc.workspaces.canDelete.useQuery( { id: workspaceId }, { enabled: open, - refetchInterval: open ? 2000 : false, + refetchInterval: open ? TERMINAL_COUNT_POLL_INTERVAL_MS : false, }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(7 hunks)apps/desktop/src/main/lib/terminal-manager.test.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
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/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsxapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.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/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsxapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.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/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsxapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
**/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/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx
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/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.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/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/main/lib/terminal-manager.test.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/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.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/workspaces/workspaces.ts
🧠 Learnings (3)
📚 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/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx
📚 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/**/*.test.{ts,tsx,js,jsx} : Tests should be independent
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.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/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.ts
🧬 Code graph analysis (2)
apps/desktop/src/main/lib/terminal-manager.ts (1)
apps/desktop/src/main/lib/terminal-history.ts (1)
cleanup(131-138)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(506-506)
⏰ 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 (8)
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
583-735: Comprehensive test coverage for new terminal management features.The test suites for
killByWorkspaceIdandgetSessionCountByWorkspaceIdcover the key scenarios effectively:
- Workspace isolation
- Non-existent workspace handling
- History cleanup
- Dead session handling
- Active session counting
The tests follow the coding guidelines with clear, independent test cases.
apps/desktop/src/main/lib/terminal-manager.ts (2)
399-403: Simple and correct implementation.The
getSessionCountByWorkspaceIdmethod is straightforward and correctly filters for both workspace ID and alive status.
300-397: Test the actual SIGTERM→SIGKILL→force cleanup timeout behavior.The staged termination logic is well-structured with defensive cleanup guards (resolved flag, idempotent closeHistory, safe event handler removal). However, the test suite uses mocked PTY that exits via
setImmediate, so the actual 2000ms SIGTERM→SIGKILL and 500ms SIGKILL→force cleanup timeouts are never exercised. Consider adding tests that verify the timeout progression works correctly with real delays.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
287-342: Consistent terminal count integration across all branches.The
activeTerminalCountis properly added to all return paths ofcanDelete:
- Workspace not found (line 287)
- Worktree exists check (lines 315, 324)
- No worktree error (line 331)
- No associated worktree (line 341)
This ensures the UI can always display the terminal count regardless of the workspace state.
354-356: Proper sequencing of terminal cleanup before workspace deletion.Invoking
terminalManager.killByWorkspaceId(input.id)before the teardown and worktree removal ensures terminals are properly terminated first. This prevents potential issues with file locks or running processes during deletion.apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (3)
31-37: Good refetch pattern for keeping terminal count fresh.The refetchInterval tied to the
openstate ensures the terminal count updates while the dialog is visible, without unnecessary polling when closed. The 2-second interval strikes a good balance between freshness and network/CPU usage.
46-58: Clear separation of teardown and terminal warnings.The separate toast warnings for
teardownErrorandterminalWarningprovide clear, specific feedback to users about different failure modes during workspace deletion. The 100ms delay allows the success toast to appear first.
94-99: Clear terminal count messaging with proper pluralization.The conditional rendering and pluralization correctly handles single/multiple terminals, providing clear expectations to users about what will happen during deletion.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
354-358: Consider handlingkillByWorkspaceIdfailures so deletion isn’t blocked by terminal errorsRight now, if
terminalManager.killByWorkspaceIdthrows for any reason, the mutation will abort before worktree removal and DB cleanup, which can leave the user unable to delete a problematic workspace even though orphaned terminals are often non‑critical. You already model partial failure viaterminalWarning, so it would be more robust to treat terminal-kill failures as a warning rather than a hard failure.You can centralize both the kill and
terminalWarningcomputation in a small try/catch block:- // Kill all terminal processes in this workspace first - const terminalResult = await terminalManager.killByWorkspaceId( - input.id, - ); + // Kill all terminal processes in this workspace first + let terminalWarning: string | undefined; + try { + const terminalResult = await terminalManager.killByWorkspaceId( + input.id, + ); + if (terminalResult.failed > 0) { + terminalWarning = `${terminalResult.failed} terminal process(es) may still be running`; + } + } catch (error) { + console.error( + "Failed to kill terminal processes for workspace", + input.id, + error, + ); + terminalWarning = "Terminal processes may still be running"; + } @@ - const terminalWarning = - terminalResult.failed > 0 - ? `${terminalResult.failed} terminal process(es) may still be running` - : undefined; - - return { success: true, teardownError, terminalWarning }; + return { success: true, teardownError, terminalWarning };This way, workspace deletion proceeds even if process termination is imperfect, and callers still get a clear signal that there may be lingering terminals.
Also applies to: 435-441
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(7 hunks)
🧰 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/workspaces/workspaces.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/workspaces/workspaces.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/workspaces/workspaces.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/workspaces/workspaces.ts
🧬 Code graph analysis (1)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(506-506)
⏰ 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/lib/trpc/routers/workspaces/workspaces.ts (2)
4-4: Terminal manager import looks appropriate for TRPC router usageBringing
terminalManagerin at the router layer is a good place to centralize workspace/terminal coordination and keeps IPC flowing through tRPC as per the guidelines. No changes needed here.
277-343:activeTerminalCountplumbing incanDeleteis consistent and usefulThe way
activeTerminalCountis computed once and then returned on every code path (including error and “workspace not found” cases) is clean and keeps the tRPC output shape consistent. This should make it straightforward for the UI to display terminal activity state alongside delete affordances.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Accessibility
Bug Fixes / Reliability
Tests
✏️ Tip: You can customize this high-level summary in your review settings.