feat: flexible split panes via drag & drop tabs (#543)#645
feat: flexible split panes via drag & drop tabs (#543)#645
Conversation
Implement Phase 2 of flexible split panes: - PaneManager: manages pane layout tree with per-pane TabManagers - PaneTreeView: recursive SwiftUI view rendering PaneNode splits - Drag tab to right edge → split right, bottom edge → split down - Drag tab between panes → move tab - PaneDividerView: draggable divider for resize with cursor feedback - Drop zone overlays with semi-transparent indicators - TabDragInfo: encoded drag data for cross-pane tab transfer - 25 unit tests covering PaneManager, TabDragInfo, PaneDropZone
batonogov
added
enhancement
✅ Code Coverage: 76.2%Threshold: 70% Coverage is above the minimum threshold. Generated by CI — see job summary for detailed file-level breakdown. |
- Replace magic numbers (300, 200) in drop zone detection with percentage-based thresholds using actual pane size from GeometryReader - Switch TabDragInfo from pipe-separated encoding to JSON (Codable) - Use custom UTType (.paneTabDrag) instead of .text for drag operations, registered as exported type in Info.plist - Add guard against repeated NSCursor.push() in onContinuousHover to prevent cursor stack leak - Remove dead paneTabDragUTType global variable, replaced by UTType extension - Add TODO for persisting split pane layout in SessionState - Update TabDragInfoTests for JSON encoding format
…nd exclude PaneTreeView from coverage - PaneManager: focus cycle, updateSplitRatio, move tab edge cases (invalid source, non-existent tab, content preservation), remove pane edge cases, rapid splits, nil tabURL split - TabDragInfo: unicode paths, deep paths, partial/null/array JSON, extra fields, multiple instances - PaneDropZone: all zones identity, sendable conformance, switch exhaustiveness - Exclude PaneTreeView.swift from coverage (pure SwiftUI view)
Resolve merge conflicts in AccessibilityIdentifiers, ContentView, PineApp, and ProjectManager to incorporate toast notifications, inline diff hunks, tab context menu actions, and git fetcher changes from main while preserving split panes functionality.
batonogov
left a comment
batonogov
left a comment
There was a problem hiding this comment.
Code Review — feat: flexible split panes via drag & drop tabs (#543)
REQUEST CHANGES (не могу формально поставить, т.к. свой PR — но это REQUEST CHANGES)
Архитектура в целом хорошая: PaneNode (immutable tree) + PaneManager (state) + PaneTreeView (recursive rendering). Но есть критические проблемы, которые блокируют мерж.
CRITICAL (must fix)
1. SinglePaneSplitDropDelegate использует .text UTType вместо .paneTabDrag
EditorAreaView.swift строка ~129:
.onDrop(of: [.text], delegate: SinglePaneSplitDropDelegate(...)А в validateDrop:
func validateDrop(info: DropInfo) -> Bool {
info.hasItemsConforming(to: [.text])
}При этом EditorTabBar и PaneEditorTabBar уже используют .paneTabDrag. SinglePaneSplitDropDelegate — единственный, кто ловит .text. Это означает:
- Любой внешний text drop (напр. из браузера) будет интерпретирован как tab drag и вызовет crash при decode
provider.loadItem(forTypeIdentifier: "public.text")внутриperformDrop— хардкод строки вместоUTType.paneTabDrag.identifier
Нужно заменить на .paneTabDrag и исправить loadItem.
2. Cursor leak в PaneDividerView
.onContinuousHover { phase in
case .active:
NSCursor.resizeLeftRight.push()
case .ended:
NSCursor.pop()
}NSCursor.push()/pop() — это стек. Если view деинициализируется или перестраивается SwiftUI без вызова .ended, курсор навсегда застрянет в resize-состоянии. Нужен onDisappear { if isCursorPushed { NSCursor.pop() } }.
3. @ObservationIgnored lazy var paneManager в ProjectManager
@ObservationIgnored + lazy var = если кто-то обратится к paneManager до полной инициализации tabManager, PaneManager получит дефолтный TabManager. Нужно убедиться, что порядок инициализации гарантирован, или использовать explicit init.
4. Drop zone calculation в SinglePaneSplitDropDelegate использует magic numbers
if location.x > 300 && location.x > location.y * 1.2 {
dropZone = .right
} else if location.y > 200 && location.y > location.x * 0.8 {
dropZone = .bottom
}Абсолютные пиксели (300, 200) — сломаются на маленьких окнах и больших мониторах. PaneSplitDropDelegate правильно использует процентные пороги (edgeThreshold: 0.7). SinglePaneSplitDropDelegate должен делать то же самое.
IMPORTANT (should fix)
5. PaneEditorTabBar — массивное дублирование EditorTabBar
PaneEditorTabBar (~80 строк) это copy-paste EditorTabBar с минимальными изменениями. Нет context menu, нет close-other/close-right. Нужно либо параметризовать EditorTabBar, либо извлечь общий компонент. Дублирование неизбежно разойдётся.
6. PaneLeafView не передаёт blameLines, diffHunks, onAcceptHunk, onRevertHunk
В PaneLeafView.codeEditorView захардкожено blameLines: [] и нет callback'ов для hunk accept/revert. В split panes git blame и accept/revert inline diff не работают. Если это Phase 2 — нужен // TODO: комментарий.
7. Session persistence не сохраняет split layout
TODO в SessionState.swift есть, но пользователь потеряет все split'ы при перезапуске. Нужно зафиксировать как known limitation в PR description.
8. moveTab не сохраняет все свойства таба
destination.tabs[idx].content = tab.content
destination.tabs[idx].savedContent = tab.savedContentНе копируются: cursorPosition, scrollOffset, foldState, cachedHighlightResult, cachedIndentation, isPinned, syntaxHighlightingDisabled. Пользователь потеряет позицию курсора и scroll при перетаскивании.
9. Нет onDisappear cleanup для onHover состояния
PaneDividerView — isHovering и isCursorPushed не сбрасываются при dealloc view. SwiftUI может переиспользовать или пересоздать view, оставив cursor stack в невалидном состоянии.
SUGGESTIONS (nice to have)
10. Тесты PaneDropZone — тривиальное padding
8 тестов на Equatable автогенерированного enum — padding. PaneDropZone = простой enum с 3 cases и compiler-synthesized Equatable. Лучше потратить тесты на drop zone calculation, edge cases пустого pane tree, cursor stack.
11. Нет UI-тестов
Для такой фичи хотя бы smoke UI test на split/unsplit нужен. Проект уже имеет 18+ UI test файлов.
12. Transferable вместо legacy NSItemProvider API
registerDataRepresentation с completion handler — legacy. На macOS 13+ лучше Transferable protocol.
Итог
4 critical + 5 important = REQUEST CHANGES. Основные блокеры: (1) неправильный UTType в SinglePaneSplitDropDelegate, (2) cursor leak, (3) неполный moveTab, (4) magic numbers в drop zone.
- Use custom UTType .paneTabDrag instead of .text in SinglePaneSplitDropDelegate to prevent external text drops from breaking the app - Add onDisappear cleanup for NSCursor push/pop in PaneDividerView to prevent cursor leaks when SwiftUI deallocates the view without hover .ended phase - Replace magic pixel numbers (300/200) with percentage-based thresholds (0.7) in SinglePaneSplitDropDelegate, unified with PaneSplitDropDelegate via shared PaneDropZone.zone(for:in:) static method - Fix moveTab to preserve all EditorTab state (cursorPosition, scrollOffset, foldState, isPinned, encoding, etc.) via new EditorTab.reidentified(from:) - Remove duplicated PaneEditorTabBar, reuse EditorTabBar with overridePaneID - Replace trivial PaneDropZone.Equatable tests with meaningful tests for drop zone calculation, EditorTab.reidentified, and tab state preservation
batonogov
left a comment
batonogov
left a comment
There was a problem hiding this comment.
Повторное code review PR #645
Проверка предыдущих замечаний
- SinglePaneSplitDropDelegate: .text -> .paneTabDrag — ИСПРАВЛЕНО.
EditorAreaView.swift:199использует.paneTabDrag,EditorTabBar.swiftтоже мигрирован на.paneTabDrag. - NSCursor leak в PaneDividerView — ИСПРАВЛЕНО.
onDisappear+isCursorPushedguard на месте (строки 164-168 PaneTreeView.swift). - Magic numbers -> PaneDropZone.zone(for:in:) — ИСПРАВЛЕНО. Единая статическая функция с
edgeThreshold = 0.7, используется и вSinglePaneSplitDropDelegate, и вPaneSplitDropDelegate. - moveTab теряет свойства — ИСПРАВЛЕНО.
EditorTab.reidentified(from:)копирует 13 мутируемых свойств + вызываетrecomputeContentCaches(). Все свойства изEditorTabпокрыты. - PaneEditorTabBar удален, EditorTabBar с overridePaneID — ИСПРАВЛЕНО.
PaneEditorTabBarотсутствует в кодбазе,EditorTabBarпринимаетoverridePaneID: PaneID?. - Тривиальные тесты -> поведенческие — ИСПРАВЛЕНО. 74 теста в
PaneManagerTestsиTabDragInfoTestsпокрывают split, remove, move, ratio, edge cases, roundtrip encoding.
Новые проблемы
Critical (must fix)
C1. PaneLeafView закрывает dirty tabs без подтверждения
PaneTreeView.swift, строка ~237-238:
onCloseTab: { tab in
tabManager.closeTab(id: tab.id, force: false)TabManager.closeTab(id:force: false) не проверяет isDirty — он только защищает pinned tabs. Диалог подтверждения ("Save/Don't Save/Cancel") реализован в ContentView.closeTabWithConfirmation, но PaneLeafView его не вызывает. Пользователь потеряет несохраненные изменения при закрытии таба в split pane.
C2. onCloseOtherTabs / onCloseTabsToTheRight / onCloseAllTabs не подключены
PaneLeafView создает EditorTabBar без этих callbacks. Контекстное меню таба будет показывать "Close Other Tabs", "Close Tabs to the Right", "Close All Tabs", но нажатие ничего не делает (callbacks = nil, guard в Button не сработает, просто no-op).
Important (should fix)
I1. Git diff markers не работают в split panes
PaneTreeView.swift, строка 185: @State private var lineDiffs: [GitLineDiff] = [] — никогда не обновляется. В ContentView lineDiffs заполняется через GitAndNotificationObserver. В PaneLeafView git gutter markers всегда пустые. Для split pane все diff markers молча пропадают.
I2. Git blame всегда [] в split panes
PaneTreeView.swift, строка 280: blameLines: [] — захардкожен пустой массив. Git blame не работает в split pane.
I3. diffHunks / onAcceptHunk / onRevertHunk отсутствуют
PaneLeafView не передает diffHunks, onAcceptHunk, onRevertHunk в CodeEditorView. Inline diff hunk accept/revert в gutter не работает в split panes.
I4. StatusBar отсутствует в split panes
PaneLeafView не рендерит StatusBarView. Пользователь теряет информацию о позиции курсора, line endings, encoding, file size при split.
Suggestions
S1. PaneTreeView.swift добавлен в CI exclude-списки для coverage (корректно, это view-файл). Но файл содержит ~200 строк логики (PaneDropZone, SinglePaneSplitDropDelegate, PaneSplitDropDelegate, PaneDropOverlay). Рекомендую вынести non-view логику (drop delegates, drop zone enum) в отдельные файлы, которые будут покрыты code coverage.
S2. PaneContent.terminal объявлен, но нигде не используется. Мертвый код — лучше удалить до Phase 2 или пометить // Phase 2.
Вердикт: REQUEST CHANGES. C1 и C2 — потеря данных и сломанное контекстное меню. Требуют исправления перед мерджем.
- PaneLeafView now shows dirty tab confirmation dialog on close - Connected onCloseOtherTabs/onCloseTabsToTheRight/onCloseAllTabs context menu handlers - Wired lineDiffs from GitStatusProvider for gutter diff markers - Wired blameLines from git blame for inline blame annotations - Added diffHunks/onAcceptHunk/onRevertHunk for inline diff - Added StatusBarView to each pane leaf - Removed dead PaneContent.terminal case from enum - Updated PaneNodeTests for single PaneContent case - Added PaneLeafCloseTests with 12 tests covering close logic
Code Review -- Round 3Замечания раунда 2 (dirty tabs, context menu, lineDiffs, blameLines, diffHunks, StatusBarView, dead code terminal) -- все адресованы. PaneLeafView теперь полноценный: есть git diff/blame refresh, StatusBarView, dirty-tab диалоги, код чистый. Количество тестов хорошее: 27 (PaneManager) + 12 (PaneLeafClose) + 47 (TabDragInfo) = 86 новых тестов. Однако есть новые проблемы, включая одну критическую. CRITICAL -- pm.tabManager не знает про split panesВсе menu commands, Cmd+W, Cmd+S, Save As, Duplicate, Ctrl+Tab, applicationShouldTerminate, applicationWillTerminate используют Конкретные поломки:
Fix: Все эти места должны использовать IMPORTANT -- Два .onDrop конфликтуют в EditorAreaView
.onDrop(of: [.fileURL], isTargeted: $isDragTargeted) { ... }
// ...
.onDrop(of: [.paneTabDrag], delegate: SinglePaneSplitDropDelegate(...))В SwiftUI второй IMPORTANT -- PaneLeafView.onTapGesture блокирует клики в редакторе.onTapGesture {
paneManager.activePaneID = paneID
}
IMPORTANT -- moveTab force-closes dirty tabs без предупреждения
Это не страшно в happy path, но стоит сделать IMPORTANT -- Session persistence не учитывает split panes
Suggestion -- PaneLeafCloseTests: синтаксические ошибки НЕ подтвержденыУказанные в задании строки (63, 94, 125, 158, 238) в текущей версии Suggestion -- EditorTab.reidentified не копирует contentVersion
Suggestion -- Нет left drop zone
Verdict: Requires ChangesКритическая проблема с |
…tap conflicts 1. pm.tabManager → pm.activeTabManager in all menu commands (Cmd+S/W, Save As, Duplicate, Ctrl+Tab, Cmd+1..9), CloseDelegate, and applicationShouldTerminate/WillTerminate. Multi-pane mode now correctly targets the focused pane. 2. Merged two .onDrop handlers in EditorAreaView into a single EditorAreaUnifiedDropDelegate — the second .onDrop was overriding the first, breaking file drops from Finder in single-pane mode. 3. Replaced .onTapGesture in PaneLeafView with PaneFocusDetector (NSView local event monitor) — the tap gesture was blocking clicks on the code editor text and tab bar buttons. 4. Reordered moveTab in PaneManager: add to destination first, then remove from source. Prevents tab loss if append fails. 5. Session persistence now collects tabs from ALL panes via pm.allTabs, so split-pane tabs survive save/restore cycles. 6. DocumentEditedTracker uses pm.hasUnsavedChanges (all panes) instead of single tabManager.
Code Review — Раунд 4Проверка замечаний раунда 31. pm.tabManager -> pm.activeTabManager -- ИСПРАВЛЕНО. ~20 мест в PineApp.swift обновлены, добавлены allTabs/allDirtyTabs/hasUnsavedChanges/saveAllPaneTabs в ProjectManager. Все меню-команды (Save, Find, Fold, Navigate) используют activeTabManager. CloseDelegate.closeActiveTab и windowShouldClose корректно используют activeTabManager/allDirtyTabs. Проверено. 2. Два .onDrop -> один EditorAreaUnifiedDropDelegate -- ИСПРАВЛЕНО. В EditorAreaView теперь один 3. .onTapGesture -> PaneFocusDetector -- ИСПРАВЛЕНО. Реализация через NSView с 4. moveTab: append ДО closeTab -- ИСПРАВЛЕНО. 5. Session persistence из allTabs -- ИСПРАВЛЕНО. Новые замечанияCRITICALC1. CloseDelegate.closeActiveTab не удаляет пустой pane. После Файл: if activeTM.tabs.isEmpty, projectManager.paneManager.root.leafCount > 1 {
projectManager.paneManager.removePane(projectManager.paneManager.activePaneID)
}IMPORTANTI1. QuickOpenView, SearchResultsView, SidebarView, FileNodeRow, WelcomeView -- все используют primary tabManager вместо activeTabManager. Когда у пользователя 2+ pane и фокус на правом, Cmd+P / клик по файлу в sidebar / клик по результату поиска откроет файл в первом (primary) pane, а не в активном. Это серьёзная UX-проблема для multi-pane workflow. Затронутые файлы и строки:
Исправление (минимальное): Эти view получают TabManager через I2. Массивное дублирование кода между PaneLeafView и ContentView. PaneLeafView содержит ~300 строк кода, который копирует логику из ContentView: refreshLineDiffs, refreshBlame, handleGutterAccept, handleGutterRevert, closeTabWithConfirmation, closeOtherTabsWithConfirmation, closeAllTabsWithConfirmation, confirmBulkClose. Любой баг-фикс или изменение поведения в одном месте нужно будет дублировать в другом. Сейчас ContentView используется когда Рекомендация: Для этого PR -- оставить как есть, но создать issue на рефакторинг: использовать PaneTreeView ВСЕГДА (и для одного pane тоже), а EditorAreaView в ContentView убрать. Тогда один code path. I3. weak var paneManager: PaneManager?Уже SUGGESTIONSS1. S2. TODO-комментарий в SessionState.swift. S3. ИтогВсе 5 замечаний раунда 3 исправлены корректно. Обнаружены 1 critical + 3 important + 3 suggestions. C1 (пустой pane после Cmd+W) -- обязательно к исправлению. I1 (sidebar/quickopen открывают в wrong pane) -- серьёзная UX-проблема, нужно решить до мержа или хотя бы задокументировать как known limitation. Статус: Требует доработки (C1 обязателен, I1 настоятельно рекомендуется). |
…cycle CloseDelegate.closeActiveTab() now removes the active pane when closing its last tab, matching PaneLeafView behavior. PaneFocusNSView.paneManager is now a weak reference to prevent retain cycles.
Code Review — Раунд 5 (финальный)Замечания раунда 4 — статус
Итоговая оценкаPR в хорошей форме. Оба замечания корректно исправлены и покрыты тестами. Critical/important проблем не обнаружено. Что сделано хорошо:
Suggestions (не блокирующие):
Вердикт: APPROVED |
- Resolve merge conflict in ContentView.swift (keep pane layout logic) - Remove stale onAcceptHunk/onRevertHunk params from PaneTreeView and ContentView (CodeEditorView no longer accepts them after main changes) - Add nonisolated(unsafe) on PaneFocusNSView.monitor for deinit access - Move @mainactor from individual test methods to struct level on PaneManagerTests, MultiPaneIntegrationTests, PaneLeafCloseTests, PaneFocusNSViewTests (matches convention from #690) - Replace force unwrapping with guard-let in MultiPaneIntegrationTests (SwiftLint fix)
1 participant
Summary
Split panes driven entirely by drag & drop — no menu items needed:
25 unit tests, build clean.
Closes #543
Test plan