-
Notifications
You must be signed in to change notification settings - Fork 468
Make Block Close / Cmd-W more consistent #2417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…stead of directly calling nodeModel.onClose
… closed the last pinned tab
…nsistent. add a jiggle effect to show why we didn't actually close a block or tab
…irst, if weave ai is open, replace block, switch to wave ai... etc...
WalkthroughAdds chat-empty state tracking to WaveAIModel with isChatEmpty and hasNonEmptyInput, updates clear/load/error paths, and sets isChatEmpty to false on first send in aipanel. Introduces uxCloseBlock and extensive static-tab/AI-panel-aware close logic in keymodel, replaces direct close usage in blockframe, and updates Cmd+Shift+W behavior. replaceBlock now accepts a focus boolean and defers old block deletion; callers (launcher) updated. Adds TabBarModel singleton with jigglePinAtom and UI wiring in tab.tsx plus new CSS jiggle animation. Minor layoutModel comment/import reorder. workspace.go improves active-tab reassignment when deleting tabs. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/store/global.ts (1)
488-491
: Handle DeleteBlock rejectionWe schedule
ObjectService.DeleteBlock
insidesetTimeout
, but the async arrow isn’t awaited or wrapped. If the delete RPC fails, the rejected promise goes unhandled, which can surface as an unhandled-rejection crash in Electron. Please catch/log (or usefireAndForget
) so the failure is contained.Apply this diff to contain the rejection:
- setTimeout(async () => { - await ObjectService.DeleteBlock(blockId); - }, 300); + setTimeout(() => { + ObjectService.DeleteBlock(blockId).catch((err) => { + console.error("replaceBlock: failed to delete old block", blockId, err); + }); + }, 300);
🧹 Nitpick comments (1)
frontend/app/aipanel/aipanel.tsx (1)
357-357
: Consider adding a setter method for better encapsulation.Direct assignment to
model.isChatEmpty
works since the property is public, but it's inconsistent with the pattern used elsewhere inWaveAIModel
(e.g.,setError()
,clearError()
,setModel()
). For consistency and better encapsulation, consider adding asetIsChatEmpty(value: boolean)
method.Apply this change to the model:
In
frontend/app/aipanel/waveai-model.tsx
, add a setter method:setIsChatEmpty(value: boolean) { this.isChatEmpty = value; }Then update this line to:
- model.isChatEmpty = false; + model.setIsChatEmpty(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
frontend/app/aipanel/aipanel.tsx
(1 hunks)frontend/app/aipanel/waveai-model.tsx
(5 hunks)frontend/app/block/blockframe.tsx
(5 hunks)frontend/app/store/global.ts
(2 hunks)frontend/app/store/keymodel.ts
(6 hunks)frontend/app/tab/tab.scss
(1 hunks)frontend/app/tab/tab.tsx
(4 hunks)frontend/app/tab/tabbar-model.ts
(1 hunks)frontend/app/view/launcher/launcher.tsx
(1 hunks)frontend/layout/lib/layoutModel.ts
(2 hunks)pkg/wcore/workspace.go
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/app/block/blockframe.tsx (1)
frontend/app/store/keymodel.ts (1)
uxCloseBlock
(664-664)
frontend/app/store/keymodel.ts (5)
frontend/layout/lib/layoutModelHooks.ts (2)
deleteLayoutModelForTab
(50-52)getLayoutModelForStaticTab
(45-48)frontend/app/tab/tabbar-model.ts (1)
TabBarModel
(7-26)frontend/app/aipanel/waveai-model.tsx (1)
WaveAIModel
(24-231)frontend/util/util.ts (1)
fireAndForget
(411-411)frontend/app/store/focusManager.ts (1)
focusManager
(86-86)
frontend/app/tab/tab.tsx (1)
frontend/app/tab/tabbar-model.ts (1)
TabBarModel
(7-26)
frontend/app/store/global.ts (1)
pkg/waveobj/wtype.go (1)
BlockDef
(242-245)
⏰ 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). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (5)
frontend/app/block/blockframe.tsx (1)
19-19
: LGTM! Clean refactoring to centralize close behavior.The replacement of
onClose
callbacks withuxCloseBlock()
centralizes the block closing logic, making it easier to manage static-tab-aware and AI-panel-aware close behavior consistently across the UI.Also applies to: 44-44, 80-80, 155-155, 203-203
frontend/app/tab/tab.scss (1)
145-194
: LGTM! Well-designed jiggle animation.The animation provides clear visual feedback when attempting to close a pinned tab. The 0.5s duration aligns with the timeout in
tab.tsx
, and the animation correctly returns to the inherit color state at completion.frontend/app/tab/tab.tsx (1)
11-11
: LGTM! Jiggle animation integration is well-implemented.The effect correctly triggers the jiggle animation when the tab is active, pinned, and the trigger increments. The timeout cleanup and dependency array are properly configured, and the 500ms duration matches the CSS animation timing.
Also applies to: 15-15, 56-58, 149-157, 242-242
frontend/app/tab/tabbar-model.ts (1)
1-26
: LGTM! Clean singleton implementation.The
TabBarModel
singleton correctly manages the jiggle trigger state using a counter atom. The functional update injiggleActivePinnedTab()
ensures proper state transitions, and the singleton pattern is correctly implemented.frontend/app/aipanel/waveai-model.tsx (1)
37-37
: LGTM! Chat empty state tracking is correctly implemented.The
isChatEmpty
property andhasNonEmptyInput()
method correctly track chat state across all lifecycle paths (clear, load, error). The logic ensures accurate state transitions when starting new chats, loading existing chats, and handling errors.Also applies to: 131-131, 171-174, 196-198, 212-212
Don't allow tabs with active Wave AI sessions to get closed when we close the last block. Have Cmd-W close Wave AI if it is focused (rather than a random node). Also fixes some lurking bugs with the pinned tab functionality (and adds some nice visual feedback when we try to close a pinned tab).