Add confirmation dialog before closing tabs #2678
Conversation
Prevents accidental tab closures by showing a confirmation modal when clicking the X button or selecting 'Close Tab' from menu. - Created ConfirmCloseTabModal component - Registered modal in modal registry - Modified handleCloseTab to show confirmation - Modal has OK (closes tab) and Cancel (keeps tab) buttons Addresses common UX issue where users accidentally close tabs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded@stevenvo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces a confirmation dialog for closing tabs. A new Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 1
🧹 Nitpick comments (1)
frontend/app/modals/confirmclosetab.tsx (1)
14-23: Guard against null workspace before dereferencingws.oidThe
atoms.workspaceatom can returnnull(whenwindowDatais unavailable), but this code accessesws.oidwithout a null check. While the workspace likely exists when the modal opens, a defensive check aligns with patterns used elsewhere in the codebase (e.g.,contextmenu.ts) and prevents crashes during transient states:- const ws = globalStore.get(atoms.workspace); - getApi().closeTab(ws.oid, tabId); - deleteLayoutModelForTab(tabId); - modalsModel.popModal(); + const ws = globalStore.get(atoms.workspace); + if (!ws) { + modalsModel.popModal(); + return; + } + getApi().closeTab(ws.oid, tabId); + deleteLayoutModelForTab(tabId); + modalsModel.popModal();Note: The same pattern appears in
keymodel.ts(simpleCloseStaticTabandisStaticTabPinned).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/modals/confirmclosetab.tsx(1 hunks)frontend/app/modals/modalregistry.tsx(2 hunks)frontend/app/tab/tabbar.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/app/tab/tabbar.tsx (1)
frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)
frontend/app/modals/confirmclosetab.tsx (3)
frontend/app/store/global.ts (2)
atoms(842-842)getApi(852-852)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)frontend/app/modals/modal.tsx (1)
Modal(149-149)
frontend/app/modals/modalregistry.tsx (1)
frontend/app/modals/confirmclosetab.tsx (1)
ConfirmCloseTabModal(39-39)
🔇 Additional comments (2)
frontend/app/modals/confirmclosetab.tsx (1)
25-35: Modal wiring and UX text look solid
onOk/onCancel/onCloseare consistently wired, and the copy matches the PR intent (“Close Tab? … cannot be undone”). Component is minimal and focused, which fits the existing modal patterns.frontend/app/modals/modalregistry.tsx (1)
10-20: Registration keyed bydisplayNameis consistent with existing modalsImporting
ConfirmCloseTabModaland registering it under[ConfirmCloseTabModal.displayName || "ConfirmCloseTabModal"]lines up with the existing pattern, and matches the"ConfirmCloseTabModal"key used inpushModal. No issues here.
| const handleCloseTab = (event: React.MouseEvent<HTMLButtonElement, MouseEvent> | null, tabId: string) => { | ||
| event?.stopPropagation(); | ||
| const ws = globalStore.get(atoms.workspace); | ||
| getApi().closeTab(ws.oid, tabId); | ||
| tabsWrapperRef.current.style.setProperty("--tabs-wrapper-transition", "width 0.3s ease"); | ||
| deleteLayoutModelForTab(tabId); | ||
|
|
||
| // Show confirmation modal before closing | ||
| modalsModel.pushModal("ConfirmCloseTabModal", { tabId }); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other closeTab call sites
echo "=== Searching for closeTab( calls ==="
rg -n "closeTab\(" frontend || echo "No matches found"
echo ""
echo "=== Searching for ConfirmCloseTabModal references ==="
rg -n "ConfirmCloseTabModal" frontend || echo "No matches found"
echo ""
echo "=== Searching for close-tab related patterns ==="
rg -n "close.*tab\|tab.*close" frontend -i -A 2 | head -100 || echo "No matches found"Repository: wavetermdev/waveterm
Length of output: 1064
🏁 Script executed:
# Examine the keymodel.ts file around line 135
sed -n '120,150p' frontend/app/store/keymodel.tsRepository: wavetermdev/waveterm
Length of output: 1226
🏁 Script executed:
# Search for calls to simpleCloseStaticTab
rg -n "simpleCloseStaticTab" frontend
# Also search for tab-close related keyboard shortcuts
rg -n "KeyBinding\|keyboard" frontend/app/store/keymodel.ts | head -20Repository: wavetermdev/waveterm
Length of output: 266
🏁 Script executed:
# Check context around line 200
echo "=== Context around line 200 ==="
sed -n '190,210p' frontend/app/store/keymodel.ts
echo ""
echo "=== Context around line 537 ==="
sed -n '525,545p' frontend/app/store/keymodel.tsRepository: wavetermdev/waveterm
Length of output: 1645
Close handler correctly delegates to confirmation modal; keyboard shortcut ⌘⇧W still bypasses it
Routing tab closes through modalsModel.pushModal("ConfirmCloseTabModal", { tabId }) cleanly introduces confirmation for the X button close path. However, the keyboard shortcut Cmd:Shift:w (line 537 in keymodel.ts) calls simpleCloseStaticTab() directly, bypassing the confirmation modal entirely. This creates an inconsistent user experience—users get confirmation when using the UI button but not when using the keyboard shortcut.
Consider routing the keyboard shortcut through the same confirmation flow to ensure consistent behavior across all close paths.
🤖 Prompt for AI Agents
In frontend/app/tab/tabbar.tsx around lines 621-626, the close handler opens the
ConfirmCloseTabModal but the keyboard shortcut in keymodel.ts (around line 537)
currently calls simpleCloseStaticTab() directly, bypassing confirmation; update
the keyboard shortcut handler to route through the same confirmation flow by
invoking modalsModel.pushModal("ConfirmCloseTabModal", { tabId }) (or refactor
shared logic into a single closeTabRequest(tabId) function used by both the X
button and the shortcut) and ensure simpleCloseStaticTab() is only executed
after the confirmation modal resolves affirmatively.
stevenvo
force-pushed
the
feature/tab-close-confirmation
branch
from
837fc86 to
a2aa158
Compare
1 participant
Summary
Prevents accidental tab closures by showing a confirmation modal.
Problem
Users can accidentally close tabs by clicking the X button, losing their work and terminal session.
Solution
Implementation
Test Plan
🤖 Generated with Claude Code