Exit wizard#737
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughEscape/tab-close behavior for wizard tabs now distinguishes tabs with prior user interaction (user messages, non-empty input, or staged images) from untouched wizards: interactive wizards open an exit confirmation; non-interacted wizards close immediately via tab handlers or call Changes
Sequence DiagramsequenceDiagram
participant User as User
participant WIP as WizardInputPanel
participant TH as TabHandlers
participant SS as SessionStore
participant Dlg as Dialog
User->>WIP: Press Escape
WIP->>WIP: Evaluate hasWizardInteraction (history/user msg, trimmed input, stagedImages)
alt interaction detected
WIP->>Dlg: Open "Close this wizard?" confirmation
Dlg->>User: Render confirm dialog
User->>Dlg: Choose Exit or Cancel
Dlg->>WIP: Return choice
opt Exit
WIP->>TH: Request performTabClose/closeTab
TH->>SS: closeTab(..., { skipHistory: true })
end
else no interaction
WIP->>TH: Request immediate closeTab / call onExitWizard
TH->>SS: closeTab(..., { skipHistory: true })
SS-->>TH: Confirm closed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR allows users to exit the inline wizard instantly via Escape (or Cmd+W) when no prior interaction has occurred, skipping the confirmation dialog. The implementation is clean and consistent across
Confidence Score: 4/5Safe to merge after fixing the inputValue undefined false-positive in hasWizardInteraction One P1 bug: undefined inputValue causes hasWizardInteraction to return true for legacy sessions, showing an unnecessary confirmation dialog via Cmd+W on fresh wizard tabs. No data loss or crash risk — worst case is an extra confirm step. The Escape-key path in WizardInputPanel is unaffected since it uses the prop directly. src/renderer/utils/tabHelpers.ts line 260 Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant WIP as WizardInputPanel
participant MKH as useMainKeyboardHandler
participant UTH as useTabHandlers
participant TH as tabHelpers
U->>WIP: Press Escape
WIP->>WIP: Check conversationHistory / inputValue / stagedImages
alt Has interaction
WIP->>WIP: setShowExitConfirm(true)
U->>WIP: Click Exit in dialog
WIP->>WIP: onExitWizard()
else No interaction & multiple tabs
WIP->>TH: closeTab(session, activeTabId, {skipHistory:true})
TH-->>WIP: Updated session (tab removed)
else No interaction & single tab
WIP->>WIP: onExitWizard()
end
U->>MKH: Press Cmd+W
MKH->>UTH: handleCloseCurrentTab()
UTH->>TH: hasActiveWizard(tab) + hasWizardInteraction(tab)
TH-->>UTH: isWizardTab, hasWizardUserInteraction
UTH-->>MKH: CloseCurrentTabResult
alt hasWizardUserInteraction
MKH->>MKH: openModal(confirm) → performTabClose on confirm
else isWizardTab only (no interaction)
MKH->>UTH: performTabClose (no dialog)
else plain tab / draft
MKH->>UTH: performTabClose or draft confirm
end
Reviews (1): Last reviewed commit: "fix: handle missing conversationHistory ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!tab.wizardState?.isActive) return false; | ||
| const hasUserMessages = | ||
| tab.wizardState.conversationHistory?.some((m) => m.role === 'user') ?? false; | ||
| const hasInput = tab.inputValue?.trim() !== ''; |
There was a problem hiding this comment.
inputValue undefined yields false positive
When tab.inputValue is undefined (possible for sessions persisted before this field was added to AITab), undefined?.trim() returns undefined, and undefined !== '' is true. This makes hasWizardInteraction return true even with no actual user input, causing Cmd+W to show the "your progress will be lost" confirmation dialog on a fresh wizard tab. The conversationHistory guard directly above correctly uses ?? false — apply the same pattern here.
| const hasInput = tab.inputValue?.trim() !== ''; | |
| const hasInput = (tab.inputValue?.trim() ?? '') !== ''; |
![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 (2)
src/renderer/components/InlineWizard/WizardInputPanel.tsx (1)
155-160: Use the shared wizard-interaction predicate here too.Lines 155-160 reimplement the same interaction check that
handleTabClose()/handleCloseCurrentTab()now get fromhasWizardInteraction(). Keeping a second copy here meansEsc, tab-close, andCmd/Ctrl+Wcan drift again the next time the rule changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InlineWizard/WizardInputPanel.tsx` around lines 155 - 160, The block that recomputes interaction state duplicates logic already encapsulated in hasWizardInteraction(); replace the inline checks (hasUserMessages, hasInput, hasImages and the if using them) with a single call to hasWizardInteraction(session, inputValue, stagedImages) or the existing signature of hasWizardInteraction so the panel uses that shared predicate (aligning with handleTabClose() / handleCloseCurrentTab()); ensure you import or reference the same hasWizardInteraction symbol and remove the duplicated local variables.src/__tests__/renderer/components/InlineWizard/WizardInputPanel.test.tsx (1)
476-486: Add coverage for the multi-tab no-interaction Escape path.Line 476 still renders the default one-tab session, so this only exercises the
onExitWizard()fallback. The newsetSessions(...closeTab...)branch forsession.aiTabs.length > 1is the risky part of the change, and it isn't asserted yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/InlineWizard/WizardInputPanel.test.tsx` around lines 476 - 486, Add a test that covers the multi-tab Escape path by rendering WizardInputPanel with a session that has aiTabs.length > 1 (e.g. extend defaultProps.session.aiTabs to two entries), pass a mocked setSessions and onExitWizard, fire Escape on the textarea, and assert that setSessions was called to close a tab (verify it was called once with a sessions array reflecting one fewer tab or the expected closed-tab shape), assert onExitWizard was NOT called, and assert no "Exit Wizard?" dialog is shown; reference WizardInputPanel, defaultProps, setSessions, and session.aiTabs.length > 1 to locate where to change the test input and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/utils/tabHelpers.ts`:
- Around line 256-262: The check in hasWizardInteraction incorrectly uses
optional chaining for inputValue (tab.inputValue?.trim() !== ''), which yields
undefined and can be truthy for missing input; change the logic to treat missing
input as empty by using nullish coalescing on tab.inputValue before trim (i.e.,
default to ''), so update the hasInput computation in hasWizardInteraction to
call trim() on (tab.inputValue ?? '') to correctly detect empty input and avoid
reporting interaction for partially hydrated tabs.
---
Nitpick comments:
In `@src/__tests__/renderer/components/InlineWizard/WizardInputPanel.test.tsx`:
- Around line 476-486: Add a test that covers the multi-tab Escape path by
rendering WizardInputPanel with a session that has aiTabs.length > 1 (e.g.
extend defaultProps.session.aiTabs to two entries), pass a mocked setSessions
and onExitWizard, fire Escape on the textarea, and assert that setSessions was
called to close a tab (verify it was called once with a sessions array
reflecting one fewer tab or the expected closed-tab shape), assert onExitWizard
was NOT called, and assert no "Exit Wizard?" dialog is shown; reference
WizardInputPanel, defaultProps, setSessions, and session.aiTabs.length > 1 to
locate where to change the test input and assertions.
In `@src/renderer/components/InlineWizard/WizardInputPanel.tsx`:
- Around line 155-160: The block that recomputes interaction state duplicates
logic already encapsulated in hasWizardInteraction(); replace the inline checks
(hasUserMessages, hasInput, hasImages and the if using them) with a single call
to hasWizardInteraction(session, inputValue, stagedImages) or the existing
signature of hasWizardInteraction so the panel uses that shared predicate
(aligning with handleTabClose() / handleCloseCurrentTab()); ensure you import or
reference the same hasWizardInteraction symbol and remove the duplicated local
variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b01c1ad-d7b0-4f18-b915-77fabf31aa78
📒 Files selected for processing (6)
src/__tests__/renderer/components/InlineWizard/WizardInputPanel.test.tsxsrc/__tests__/renderer/hooks/useTabHandlers.test.tssrc/renderer/components/InlineWizard/WizardInputPanel.tsxsrc/renderer/hooks/keyboard/useMainKeyboardHandler.tssrc/renderer/hooks/tabs/useTabHandlers.tssrc/renderer/utils/tabHelpers.ts
|
Thanks for the contribution, @scriptease! This is a nice UX improvement — skipping the confirmation dialog when there's nothing to lose is the right call. The code is clean and well-structured:
LGTM — approving this. |
…also wire on Esc to work as described in the wizard.
…pdate tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…se positives Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Weird, the failed unit test is from a other branch I pushed at the same time not this push request, when I run locally it succeeds |
|
Just like changing the commit message fixed it 🎉 |
Resolve conflict in tabHelpers.ts: both hasWizardInteraction (PR) and filterUnifiedTabOrderForUnread (rc) need to coexist.
|
Merged — thanks again for the contribution, @scriptease! Nice UX polish. 🙏 |
3 participants
Allow exiting the wizard with esc if no interaction happened yet, e.g. when the user misclicks or tries out the button.
Summary by CodeRabbit
New Features
Tests