Add keyboard shortcuts and orientation control to split view#39
Add keyboard shortcuts and orientation control to split view#39remcostoeten wants to merge 1 commit intodaddyfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughThis change introduces split-view editing mode for notes, enabling users to simultaneously edit multiple notes side-by-side. The implementation adds UI state management, layout rendering with SplitEditorLayout, keyboard shortcuts for split-view control, and drag-and-drop support for assigning notes to panes. Changes
Sequence DiagramsequenceDiagram
actor User
participant AppLayoutManager
participant SplitEditorLayout
participant NoteEditor1
participant NoteEditor2
User->>AppLayoutManager: Click toggle-split-view button
activate AppLayoutManager
AppLayoutManager->>AppLayoutManager: Update isSplitView state<br/>Initialize splitPanes array
AppLayoutManager->>SplitEditorLayout: Render with initial panes
deactivate AppLayoutManager
activate SplitEditorLayout
SplitEditorLayout->>NoteEditor1: renderPane(pane1)
activate NoteEditor1
NoteEditor1-->>SplitEditorLayout: Render editor content
deactivate NoteEditor1
SplitEditorLayout->>NoteEditor2: renderPane(pane2)
activate NoteEditor2
NoteEditor2-->>SplitEditorLayout: Render editor content
deactivate NoteEditor2
deactivate SplitEditorLayout
User->>SplitEditorLayout: Drag note to secondary pane
activate SplitEditorLayout
SplitEditorLayout->>SplitEditorLayout: Detect drop (NOTE_DATA_TYPE)
SplitEditorLayout->>AppLayoutManager: onAssignNote(paneId, noteId)
deactivate SplitEditorLayout
activate AppLayoutManager
AppLayoutManager->>AppLayoutManager: Update splitPanes<br/>Set activePaneId
AppLayoutManager->>SplitEditorLayout: Re-render with updated panes
deactivate AppLayoutManager
User->>SplitEditorLayout: Drag divider to resize
activate SplitEditorLayout
SplitEditorLayout->>AppLayoutManager: onResize(newRatio)
AppLayoutManager->>AppLayoutManager: Update splitRatio
SplitEditorLayout->>SplitEditorLayout: Adjust layout dimensions
deactivate SplitEditorLayout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Greptile OverviewGreptile SummaryAdded keyboard-driven split view functionality to the editor with vertical/horizontal orientation control and drag-and-drop tab assignment between panes. Key Changes:
Minor Issue:
Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant TopToolbar
participant AppLayoutManager
participant SplitEditorLayout
participant ShortcutProvider
participant NoteEditor
User->>TopToolbar: Click split view button
TopToolbar->>AppLayoutManager: onToggleSplitView()
AppLayoutManager->>AppLayoutManager: setIsSplitView(true)
AppLayoutManager->>AppLayoutManager: setSplitPanes([primary, secondary])
AppLayoutManager->>SplitEditorLayout: Render with isSplit=true
SplitEditorLayout->>NoteEditor: renderPane(primaryPane)
SplitEditorLayout->>NoteEditor: renderPane(secondaryPane)
User->>ShortcutProvider: Press Ctrl+\
ShortcutProvider->>AppLayoutManager: handleToggleSplitView()
AppLayoutManager->>SplitEditorLayout: Update isSplit state
User->>ShortcutProvider: Press Ctrl+Shift+\
ShortcutProvider->>AppLayoutManager: handleSwapPanes()
AppLayoutManager->>AppLayoutManager: Swap panes[0] and panes[1]
AppLayoutManager->>SplitEditorLayout: Re-render with swapped panes
User->>ShortcutProvider: Press Ctrl+Alt+\
ShortcutProvider->>AppLayoutManager: handleCycleSplitOrientation()
AppLayoutManager->>AppLayoutManager: Toggle vertical/horizontal
AppLayoutManager->>SplitEditorLayout: Update orientation prop
User->>SplitEditorLayout: Drag tab to pane
SplitEditorLayout->>SplitEditorLayout: handleDrop(paneId, noteId)
SplitEditorLayout->>AppLayoutManager: onAssignNote(paneId, noteId)
AppLayoutManager->>AppLayoutManager: Update pane noteId
AppLayoutManager->>SplitEditorLayout: Re-render with updated pane
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
apps/web/features/editor/components/split-editor-layout.tsx (2)
57-85: Consider adding pointer capture for smoother resize experience.The resize handler works but could lose tracking if the pointer moves rapidly outside the divider element. Using
setPointerCaptureensures the element continues receiving pointer events even when the pointer leaves its bounds.const handlePointerDown = useCallback( (event: React.PointerEvent) => { if (!containerRef.current) return setIsDragging(true) + (event.target as HTMLElement).setPointerCapture(event.pointerId) const startPosition = orientation === 'vertical' ? event.clientX : event.clientY
52-55: Add defensive check for empty panes array.If
panesis empty,primaryPanewill beundefined, causingrenderPaneContainerto fail when accessingpane.id. While the parent component should always provide at least one pane, a defensive check improves robustness.const primaryPane = panes[0] const secondaryPane = panes[1] +if (!primaryPane) { + return <div className="flex h-full items-center justify-center text-muted-foreground">No pane available</div> +} + const clampedRatio = useMemo(() => Math.min(0.8, Math.max(0.2, splitRatio)), [splitRatio])Also applies to: 154-168
apps/web/components/layout/top-toolbar.tsx (1)
107-119: Inconsistent indentation detected.The changed lines use different indentation (appears to be spaces) compared to the unchanged portions of the file (which use tabs). While not a functional issue, this creates inconsistent formatting.
Consider running a formatter to normalize indentation across the file.
apps/web/components/layout/app-layout-manager.tsx (4)
67-77: Split-view and support state are well-structured; consider persistence as a follow-upThe split-related state (
isSplitView,splitPanes,activePaneId,splitRatio,splitOrientation) plusisMobile/isSearchOpen/hasEverLoadedRefare cleanly initialized and scoped. If you later want cross-route/session persistence of split orientation or ratio, this would be a good place to lift into a store or preference hook, but it’s not required for this PR.
130-137: Resetting split state when leaving note routes is appropriateThe
useEffectthat clearsisSplitView,splitPanes, andactivePaneIdwhen!isNoteRouteensures non-note routes don’t accidentally render editor split UI. You intentionally preservesplitRatioandsplitOrientation, which is a reasonable trade-off; if that’s not desired UX, they could be reset here as well.
210-227: Split toggle logic is sound; consider extracting the pane-initialization blockThe split toggle correctly:
- No-ops when not on a note route.
- Turns split off by just flipping
isSplitView(the collapse effect handles panes).- When enabling split, seeds two panes based on the active pane’s note or
currentNoteId.Embedding
setSplitPanesinside thesetIsSplitViewupdater is valid but a bit dense. You might consider extracting the pane-initialization logic into a small helper for readability, but behavior looks correct.
233-254: Pane-closing behavior is correct; tiny unreachable branch you can simplifyClosing a pane:
- No-ops when only one pane exists.
- For 2+ panes, updates
isSplitViewandactivePaneIdappropriately based on whether the closed pane was active and whether only one pane remains.The
if (!remaining.length)branch is currently unreachable because you early-return whenprev.length <= 1. Not an issue, but you can remove that branch later to simplify the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/components/layout/app-layout-manager.tsx(8 hunks)apps/web/components/layout/top-toolbar.tsx(4 hunks)apps/web/features/editor/components/editor-tabs-bar.tsx(1 hunks)apps/web/features/editor/components/split-editor-layout.tsx(1 hunks)apps/web/features/shortcuts/api/queries/get-shortcuts.ts(1 hunks)apps/web/features/shortcuts/shortcut-definitions.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/spec.mdc)
Use TypeScript-first schema with Skriuw SDK for the instantdb MVP
Files:
apps/web/features/editor/components/split-editor-layout.tsxapps/web/features/editor/components/editor-tabs-bar.tsxapps/web/features/shortcuts/shortcut-definitions.tsapps/web/components/layout/top-toolbar.tsxapps/web/components/layout/app-layout-manager.tsxapps/web/features/shortcuts/api/queries/get-shortcuts.ts
🧠 Learnings (1)
📚 Learning: 2025-12-04T14:57:24.760Z
Learnt from: CR
Repo: remcostoeten/skriuw PR: 0
File: .cursor/rules/shared-type-usage.mdc:0-0
Timestamp: 2025-12-04T14:57:24.760Z
Learning: Applies to apps/web/src/types/**/*.{ts,tsx} : Keep new utility types in `apps/web/src/types/` and update documentation when adding or changing shared primitives
Applied to files:
apps/web/components/layout/top-toolbar.tsxapps/web/components/layout/app-layout-manager.tsx
🧬 Code graph analysis (3)
apps/web/features/editor/components/split-editor-layout.tsx (1)
packages/ui/icons.tsx (1)
IconButton(96-202)
apps/web/components/layout/top-toolbar.tsx (1)
packages/ui/icons.tsx (1)
IconButton(96-202)
apps/web/components/layout/app-layout-manager.tsx (5)
packages/core-logic/use-media-query.ts (2)
useMediaQuery(7-26)MOBILE_BREAKPOINT(5-5)apps/web/features/editor/components/split-editor-layout.tsx (1)
SplitEditorLayout(33-237)apps/web/features/editor/components/note-editor.tsx (1)
NoteEditor(21-122)apps/web/components/layout/app-layout-shell.tsx (1)
AppLayoutShell(27-166)apps/web/stores/app-store.ts (1)
showSidebar(58-61)
🔇 Additional comments (25)
apps/web/features/shortcuts/api/queries/get-shortcuts.ts (1)
33-43: LGTM!The expanded shortcuts map correctly includes the new split-view shortcuts (
toggle-split-view,swap-split-panes,cycle-split-orientation) that align with the definitions inshortcut-definitions.ts. The existing shortcuts are preserved.apps/web/features/editor/components/split-editor-layout.tsx (3)
10-29: LGTM!The type definitions are well-structured. The
SplitPanetype andSplitEditorLayoutPropsprovide a clear contract for the component's interface.
112-152: LGTM!The pane controls and container rendering logic is well-implemented with appropriate conditional rendering for swap/close buttons based on split state and pane count. The visual feedback for active and dragging states is clear.
170-236: LGTM!The split layout structure is well-implemented with:
- Proper flex-direction toggling based on orientation
- Clean divider styling with appropriate cursor feedback
- Visual indication of dragging state
Good use of
flexBasisfor proportional sizing.apps/web/features/shortcuts/shortcut-definitions.ts (2)
81-104: LGTM!The new split-view shortcuts are well-designed:
toggle-split-view:Ctrl/Meta + \- intuitive for splittingswap-split-panes:Ctrl/Meta + Shift + \- extends the base shortcutcycle-split-orientation:Ctrl/Meta + Alt + \- consistent modifier patternThe backslash key is a good mnemonic for "split" operations.
105-109: Verify delete shortcut doesn't interfere with text editing.The
delete-itemshortcut is now enabled withDeleteandBackspacekeys. The description says "when context menu is open", but ensure the shortcut handler properly checks for context menu state to avoid interfering with normal text editing operations in the note editor.apps/web/components/layout/top-toolbar.tsx (2)
26-33: LGTM!The new props for split-view control (
showSplitToggle,isSplitView,onToggleSplitView) follow the existing optional prop pattern and maintain backward compatibility with sensible defaults.
120-127: LGTM!The split toggle button follows the established
IconButtonpattern with appropriate dynamic tooltip based onisSplitViewstate. Good consistency with other toolbar controls.apps/web/components/layout/app-layout-manager.tsx (17)
7-7: Imports forSplitEditorLayoutandNoteEditorare consistent and localizedBringing
SplitEditorLayoutandNoteEditorin at this level keeps layout concerns inAppLayoutManagerand leaves editor behavior encapsulated in editor components. No issues here.Also applies to: 36-36
125-128:currentNoteIndexmemoization and sentinel behavior look correctUsing
-1as the sentinel whensidebarActiveNoteIdis falsy keeps the navigation helpers simple and avoids out-of-bounds errors. Dependencies onsidebarActiveNoteIdandnotesInOrderare also correct.
138-157: Auto-syncing the current note into panes matches active-pane semanticsThe effect that, on
currentNoteIdchange, injects the note into the active pane (or first pane if the active one is missing) and fills anynullnoteIdslots gives sensible behavior:
- New route → active pane shows the routed note.
- Blank panes automatically adopt the new active note.
This seems aligned with the “active pane follows navigation” model and doesn’t override panes that already have content.
158-167: Collapsing back to a single primary pane when exiting split mode is consistentWhen
isSplitViewbecomes false, you normalize to a singleprimarypane with a sensiblefallbackNoteIdderived from the active pane orcurrentNoteId. This keeps subsequent logic simple and avoids lingering secondary panes in non-split mode.
169-173: GuardingcomputedTitlefor missingcurrentNoteavoids null accessShort-circuiting to
'Untitled'whencurrentNoteis falsy avoids potential null-access issues in the switch and gives a predictable title when on a note route but the note hasn’t resolved. Behavior looks safe and backwards-compatible with the existing fallbacks.
206-208: Editor mode toggle is a clean, side-effect‑free wrapper
handleToggleEditorModesimply delegates totogglePreference('rawMDXMode')and is memoized correctly. This keeps the toolbar integration straightforward.
229-231: Orientation cycling is minimal and correctToggling
splitOrientationbetween'vertical'and'horizontal'via a functional state update is straightforward and avoids stale closures.
256-267: Swapping the first two panes and updatingactivePaneIdis handled correctlyThe swap handler:
- Bails out when fewer than two panes.
- Swaps the first two entries and preserves the rest.
- Re-points
activePaneIdwhen it referred to one of the swapped panes.This matches expected UX and keeps pane IDs stable.
269-272: Assigning notes to panes and focusing them is straightforward
handleAssignNoteToPanecleanly updates the matching pane’snoteIdand focuses that pane viasetActivePaneId. Dependencies can safely be empty here since only stable setters are used.
274-276: Pane focus handler is appropriately minimal
handlePaneClickjust updatesactivePaneId, which is whatSplitEditorLayoutneeds to drive focus without extra side effects.
406-409:open-settingsshortcut wiring mirrors existing toggle behaviorThe new
open-settingsshortcut simply prevents default and callstoggleSettings(), consistent with the existingtoggle-shortcutsbehavior. No concerns.
411-414:search-notesshortcut correctly targets the shared search dialog stateToggling
isSearchOpenvia thesearch-notesshortcut is wired correctly and reuses the same state as the toolbar’s search button, so behavior stays consistent across entry points.
416-432: Split-view keyboard shortcuts are correctly guarded by route and split stateThe new shortcuts:
toggle-split-view: only acts on note routes, withpreventDefault()when handling.swap-split-panesandcycle-split-orientation: additionally requireisSplitViewbefore acting.These guards prevent shortcuts from interfering on non-note routes or when split view is off, and they delegate to the existing handlers, so behavior stays centralized.
434-442:resolvedSplitPanesprovides sensible fallbacks without overriding explicit assignmentsThe memoized
resolvedSplitPanes:
- Returns
splitPanesunchanged for non-note routes.- On note routes, fills only the first pane’s
noteIdusingcurrentNoteIdwhen missing, leaving other panes untouched.This plays nicely with the earlier effect that writes
currentNoteIdinto panes and ensures at least one pane has content without fighting explicit assignments.
444-470:noteContentcomposition cleanly isolates note routes from generic childrenRouting all note-route content through
SplitEditorLayout(withrenderPanedelegating toNoteEditoror a placeholder) and falling back tochildrenfor non-note routes keeps layout logic centralized here while leaving page components simple. No issues with the prop wiring toSplitEditorLayoutare apparent.
472-516: App layout wiring for toolbar and split controls looks consistentThe updated
AppLayoutShellusage:
- Keeps
LeftToolbar, sidebar, footer, right panel, and widgets behavior unchanged.- Extends
TopToolbarwithisRawMDXMode,onToggleEditorMode,showSidebar,showEditorModeToggle, and the split-view flags/handler, which matches the new state in this component.- Continues to use
canNavigatePrevious/canNavigateNextderived fromcurrentNoteIndex.This provides the right data for the toolbar to render editor- and split-related controls without leaking internal state details.
516-546: Main content composition with tabs, note content, and scroll container is well-factoredWrapping
EditorTabsBar(whenmultiNoteTabsis enabled) and thenoteContentarea in a flex column:
- Ensures tabs stay fixed at the top with the editor region scrolling.
- Adds a subtle top padding when tabs are present.
- Routes both split and non-split note routes through the same scrollable container, while non-note routes still render their
childrenas-is.
onFavoriteNoteandgetNoteDataplumbing intoEditorTabsBaralso look correct and consistent with the notes data model.
| function handleDragStart(e: React.DragEvent, index: number) { | ||
| // Onlywe allow dragging if not clicking on a button | ||
| const target = e.target as HTMLElement | ||
| if (target.tagName === 'BUTTON' || target.closest('button')) { | ||
| e.preventDefault() | ||
| return | ||
| } | ||
| setDraggedIndex(index) | ||
| e.dataTransfer.effectAllowed = 'move' | ||
| e.dataTransfer.setData('text/plain', tabs[index]?.noteId || '') // Required for Firefox | ||
| e.dataTransfer.setData('application/x-skriuw-note-id', tabs[index]?.noteId || '') | ||
| } |
There was a problem hiding this comment.
Typo in comment and minor improvement opportunity.
Line 72 has a typo: "Onlywe" should be "Only".
The custom data type addition aligns well with the NOTE_DATA_TYPE constant in split-editor-layout.tsx for split-view drag-and-drop support.
function handleDragStart(e: React.DragEvent, index: number) {
- // Onlywe allow dragging if not clicking on a button
+ // Only allow dragging if not clicking on a button
const target = e.target as HTMLElement📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function handleDragStart(e: React.DragEvent, index: number) { | |
| // Onlywe allow dragging if not clicking on a button | |
| const target = e.target as HTMLElement | |
| if (target.tagName === 'BUTTON' || target.closest('button')) { | |
| e.preventDefault() | |
| return | |
| } | |
| setDraggedIndex(index) | |
| e.dataTransfer.effectAllowed = 'move' | |
| e.dataTransfer.setData('text/plain', tabs[index]?.noteId || '') // Required for Firefox | |
| e.dataTransfer.setData('application/x-skriuw-note-id', tabs[index]?.noteId || '') | |
| } | |
| function handleDragStart(e: React.DragEvent, index: number) { | |
| // Only allow dragging if not clicking on a button | |
| const target = e.target as HTMLElement | |
| if (target.tagName === 'BUTTON' || target.closest('button')) { | |
| e.preventDefault() | |
| return | |
| } | |
| setDraggedIndex(index) | |
| e.dataTransfer.effectAllowed = 'move' | |
| e.dataTransfer.setData('text/plain', tabs[index]?.noteId || '') // Required for Firefox | |
| e.dataTransfer.setData('application/x-skriuw-note-id', tabs[index]?.noteId || '') | |
| } |
🤖 Prompt for AI Agents
In apps/web/features/editor/components/editor-tabs-bar.tsx around lines 71 to
82, fix the typo in the comment ("Onlywe" -> "Only") and replace the hard-coded
custom data type string with the shared NOTE_DATA_TYPE constant from
split-editor-layout.tsx (import NOTE_DATA_TYPE at top if not already imported);
keep the text/plain setData for Firefox and set the custom type using
NOTE_DATA_TYPE so split-view drag-and-drop uses the same data type across
components.
| const handleDragOver = useCallback((event: React.DragEvent, paneId: string) => { | ||
| if (event.dataTransfer.types.includes(NOTE_DATA_TYPE)) { | ||
| event.preventDefault() | ||
| setDraggingPane(paneId) | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
Drag-over check is inconsistent with drop handler.
handleDragOver only activates the drop indicator when NOTE_DATA_TYPE is present, but handleDrop falls back to text/plain. This means dragging from sources that only provide text/plain (e.g., Firefox fallback) won't show the visual indicator but the drop will still work.
const handleDragOver = useCallback((event: React.DragEvent, paneId: string) => {
- if (event.dataTransfer.types.includes(NOTE_DATA_TYPE)) {
+ if (event.dataTransfer.types.includes(NOTE_DATA_TYPE) || event.dataTransfer.types.includes('text/plain')) {
event.preventDefault()
setDraggingPane(paneId)
}
}, [])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDragOver = useCallback((event: React.DragEvent, paneId: string) => { | |
| if (event.dataTransfer.types.includes(NOTE_DATA_TYPE)) { | |
| event.preventDefault() | |
| setDraggingPane(paneId) | |
| } | |
| }, []) | |
| const handleDragOver = useCallback((event: React.DragEvent, paneId: string) => { | |
| if (event.dataTransfer.types.includes(NOTE_DATA_TYPE) || event.dataTransfer.types.includes('text/plain')) { | |
| event.preventDefault() | |
| setDraggingPane(paneId) | |
| } | |
| }, []) |
🤖 Prompt for AI Agents
In apps/web/features/editor/components/split-editor-layout.tsx around lines
101-106, the drag-over visual indicator only checks for NOTE_DATA_TYPE but the
drop handler accepts a 'text/plain' fallback; update handleDragOver to mirror
handleDrop by checking for NOTE_DATA_TYPE OR 'text/plain' (i.e., if
event.dataTransfer.types includes NOTE_DATA_TYPE || includes 'text/plain'), then
call event.preventDefault() and setDraggingPane(paneId) so sources that only
provide text/plain (Firefox fallback) show the drop indicator.
1 participant
Summary
Testing
Codex Task
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.