fix(desktop): change split commands back & make more robust#261
fix(desktop): change split commands back & make more robust#261
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request enhances pane-splitting functionality by introducing a utility to resolve valid target panes within mosaic layouts, fixing focus management when panes are unavailable, and updating split hotkey bindings. The changes include a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
AviPeltz
changed the title
![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 (2)
apps/desktop/src/renderer/stores/tabs/utils.test.ts (1)
38-57: Consider splitting multi‑expect cases into single‑assert testsThe coverage for
findPanePathis strong and the layouts are easy to follow. The last two test cases, however, bundle multipleexpectcalls into oneitblock. For maximum clarity and to align with the “one assert per test” guideline, you could split those into separate tests per pane ID (e.g., one test for"pane-1", another for"pane-2", etc.), while reusing a shared layout fixture.As per coding guidelines, tests should have one assert per test.
Also applies to: 73-100
apps/desktop/src/renderer/screens/main/index.tsx (1)
2-2: Split target resolution and focus correction are robust; minor cleanups possibleUsing
activeWindowplusresolveSplitTargetis a nice way to guard split operations and automatically repair desynced focus when the focused pane no longer exists in the current layout. The fallback togetFirstPaneId, coupled withsetFocusedPane, should keep window and focus state coherent while still allowing splits to proceed.A couple of small nits you might consider (non‑blocking):
resolveSplitTargetalways returns an object, so theif (!target) return;checks in the three hotkey handlers are currently unreachable. Either remove those checks for clarity, or—if you expect a “no target” scenario later—make that part of the helper’s explicit return type.- Since
Windowis also a global DOM type name, you could optionally alias the imported store type (e.g.,import type { Window as TabsWindow }) to reduce ambiguity at call sites.Also applies to: 12-15, 45-49, 58-59, 70-87, 89-109, 111-128, 130-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/renderer/screens/main/index.tsx(5 hunks)apps/desktop/src/renderer/stores/tabs/utils.test.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(2 hunks)apps/desktop/src/shared/hotkeys.ts(1 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/stores/tabs/utils.test.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.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/stores/tabs/utils.test.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.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/renderer/stores/tabs/utils.test.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/stores/tabs/utils.test.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.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/stores/tabs/utils.test.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/index.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/index.tsx
🧠 Learnings (8)
📚 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/renderer/stores/tabs/utils.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/renderer/stores/tabs/utils.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 readable
Applied to files:
apps/desktop/src/renderer/stores/tabs/utils.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 fast
Applied to files:
apps/desktop/src/renderer/stores/tabs/utils.test.ts
📚 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 **/components/**/*.tsx : Co-locate tests next to the component file they test (e.g., `ComponentName.test.tsx`)
Applied to files:
apps/desktop/src/renderer/stores/tabs/utils.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 have one assert per test
Applied to files:
apps/desktop/src/renderer/stores/tabs/utils.test.ts
📚 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 apps/desktop/src/renderer/**/*.tsx : Call IPC methods using `window.ipcRenderer.invoke()` with object parameters - TypeScript will infer the exact response type automatically
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsx
📚 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 apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsx
🧬 Code graph analysis (3)
apps/desktop/src/renderer/stores/tabs/utils.test.ts (2)
apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
findPanePath(203-225)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)
apps/desktop/src/renderer/screens/main/index.tsx (4)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useWindowsStore(78-561)apps/desktop/src/renderer/stores/tabs/types.ts (1)
Window(25-31)apps/desktop/src/renderer/stores/tabs/utils.ts (2)
findPanePath(203-225)getFirstPaneId(192-197)apps/desktop/src/renderer/stores/tabs/pane-refs.ts (1)
getPaneDimensions(13-20)
⏰ 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/renderer/stores/tabs/utils.ts (1)
1-1: Pane path helper logic looks solidThe
findPanePathhelper correctly handles leaf and parent nodes, returns[]for a root leaf, and short‑circuits as soon as it finds a match. The traversal and return type line up with how split operations consume the path, and there are no obvious edge‑case or performance issues for typical layout depths.Also applies to: 199-225
apps/desktop/src/shared/hotkeys.ts (1)
93-109: Layout hotkey remaps and SPLIT_AUTO definition look consistentThe updated keybindings for
SPLIT_RIGHT,SPLIT_DOWN, and the reintroducedSPLIT_AUTOare self‑consistent, don’t collide with existing shortcuts in this table, and keep labels/descriptions aligned with the intended behavior.
AviPeltz
changed the title
AviPeltz
changed the title
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Bug Fixes
Changes
✏️ Tip: You can customize this high-level summary in your review settings.