Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new File Preview panel and UI plus build registration, command-palette and sidebar wiring, drag/pasteboard registry, workspace APIs and session persistence, localized strings, drag-routing/test updates, and a vendor bonsplit submodule pointer bump. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant Explorer as "FileExplorer"
participant PB as "Pasteboard / Drag Registry"
participant Workspace as "Workspace"
participant Panel as "FilePreviewPanel"
participant UI as "FilePreviewPanelView"
User->>Explorer: Drag file or double-click
Explorer->>PB: Write FilePreviewDragPasteboardWriter (UUID, path, title)
PB->>Workspace: Drop / read registry entry
Workspace->>Workspace: Determine target pane / insertion index
Workspace->>Panel: Create or focus FilePreviewPanel(filePath)
Panel->>Panel: Classify type (text/pdf/image/media/QL)
Panel->>Panel: Load content (encoding sniff, read bytes)
Panel->>Workspace: Publish displayTitle, isDirty, availability
Workspace->>UI: Render FilePreviewPanelView in pane
User->>UI: Edit / navigate / press Cmd+S
UI->>Panel: Save/update content
Panel->>Panel: Persist file, update dirty state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 SummaryAdds file-preview panels opened by dragging local sidebar files into Bonsplit insert/split targets. Text files get an editable
Confidence Score: 4/5Safe to merge for typical use, but synchronous main-thread file I/O in init is a real freeze risk for large files and should be addressed before shipping. One P1 finding (blocking main-thread I/O in FilePreviewPanel.init) that affects responsiveness for large files. All other findings are P2 style/quality concerns. The Workspace, persistence, and drag plumbing are well-structured and match existing patterns. Sources/Panels/FilePreviewPanel.swift — init file I/O and saveTextContent encoding Important Files Changed
Sequence DiagramsequenceDiagram
participant FE as FileExplorerView
participant PW as FilePreviewDragPasteboardWriter
participant DR as FilePreviewDragRegistry
participant BS as BonsplitController
participant WS as Workspace
participant FP as FilePreviewPanel
FE->>PW: init(filePath, displayTitle)
PW->>DR: register(entry) → UUID
PW->>NSPasteboard.drag: mirrorTransferDataToDragPasteboard()
Note over FE,BS: User drags onto Bonsplit insert/split target
BS->>WS: handleExternalTabDrop(request)
WS->>DR: consume(id: tabId)
DR-->>WS: FilePreviewDragEntry
alt insert into existing pane
WS->>WS: newFilePreviewSurface(paneId, filePath)
else split pane
WS->>WS: splitPaneWithFilePreview(paneId, orientation, filePath)
end
WS->>FP: init(workspaceId, filePath)
Note over FP: sniffLooksLikeText + loadTextContent() run synchronously on main thread
WS->>BS: createTab / splitPane
WS->>WS: installFilePreviewPanelSubscription(panel)
Note over WS,FP: Combine subscription syncs displayTitle + isDirty to tab badge
Reviews (1): Last reviewed commit: "Add sidebar file preview panels" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| if previewMode == .text { | ||
| loadTextContent() | ||
| } else { | ||
| isFileUnavailable = !FileManager.default.fileExists(atPath: filePath) | ||
| } | ||
| } | ||
|
|
||
| func focus() { | ||
| guard let textView else { return } | ||
| textView.window?.makeFirstResponder(textView) | ||
| } | ||
|
|
There was a problem hiding this comment.
init
FilePreviewPanel is @MainActor, so init runs on the main thread. It calls FilePreviewKindResolver.mode(for:) — which may invoke sniffLooksLikeText(url:) and perform a synchronous disk read — and then immediately calls loadTextContent(), which reads the entire file via Data(contentsOf:). For large text files (big logs, CSVs, generated source), this freezes the UI until the read completes.
Consider deferring the I/O to a background Task so init returns immediately, then publishing results back on the main actor.
| isFileUnavailable = false | ||
| } catch { | ||
| isFileUnavailable = true | ||
| } | ||
| } | ||
|
|
||
| private static func decodeText(_ data: Data) -> String? { | ||
| if let decoded = String(data: data, encoding: .utf8) { | ||
| return decoded | ||
| } | ||
| if let decoded = String(data: data, encoding: .utf16) { |
There was a problem hiding this comment.
saveTextContent() silently re-encodes files to UTF-8
loadTextContent() uses decodeText(), which accepts UTF-16 and ISO-Latin-1 as fallbacks. However, saveTextContent() always writes with .utf8. If a user opens a UTF-16 or Latin-1 file, edits it, and saves, the file will be silently re-encoded as UTF-8. Tools that depend on the original encoding will silently receive broken output.
Consider tracking the detected encoding and round-tripping it, or showing a brief notice in the header when the file's original encoding differs from UTF-8.
| if Thread.isMainThread { | ||
| write() | ||
| } else { | ||
| DispatchQueue.main.async(execute: write) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| enum FilePreviewMode: Equatable { | ||
| case text | ||
| case quickLook | ||
| } |
There was a problem hiding this comment.
mirrorTransferDataToDragPasteboard() called redundantly
The method is invoked once at the end of init and again every time pasteboardPropertyList(forType:) is called for bonsplitTransferType. During a single drag session the system can call pasteboardPropertyList multiple times, so NSPasteboard.drag gets written to repeatedly. The call in init alone is sufficient.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
498-550:⚠️ Potential issue | 🟠 MajorPersist dirty text-preview state in the session snapshot.
SessionFilePreviewPanelSnapshotonly carriesfilePath, butFilePreviewPanelhas editabletextContentandisDirty. Restoring throughnewFilePreviewSurface(...)rebuilds the panel from disk, so unsaved edits and the dirty badge are silently lost after relaunch/session restore.Also applies to: 566-567, 788-798
🧹 Nitpick comments (4)
Sources/ContentView.swift (1)
6071-6072: Expand file-preview keywords for better switcher discoverability (optional).Line 6072 currently skips media-centric terms, so file-preview panels may be harder to find via command-palette search.
♻️ Suggested tweak
case .filePreview: - return ["file", "preview", "text", "pdf", "image"] + return ["file", "preview", "text", "pdf", "image", "media", "video", "audio", "quick look"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 6071 - 6072, The switch case for .filePreview currently returns a small keyword list which omits common media/search terms; update the return for the .filePreview branch (the case .filePreview in the switch that returns ["file","preview","text","pdf","image"]) to include more discoverability keywords such as "image", "photo", "picture", "video", "media", "attachment", "document", "viewer", "open", "thumbnail", "preview", "file", "pdf", and "text" so the file-preview panels surface in the command palette when users search with media-centric terms.Sources/Panels/FilePreviewPanel.swift (3)
536-543: Cmd-S saves unconditionally and ignores failure.
performKeyEquivalentcallspanel?.saveTextContent()regardless ofpanel.isDirty, and returnstruewithout inspecting the result. If the save fails, the panel silently flips toisFileUnavailable = true(see save path) and the user gets no direct feedback tied to the keystroke. Gating onisDirtymirrors the SwiftUI button's.disabled(!panel.isDirty)and avoids needless rewrites that can touch mtime/permissions.♻️ Suggested tweak
override func performKeyEquivalent(with event: NSEvent) -> Bool { if event.modifierFlags.intersection(.deviceIndependentFlagsMask) == .command, event.charactersIgnoringModifiers?.lowercased() == "s" { - panel?.saveTextContent() - return true + guard let panel, panel.isDirty else { return false } + panel.saveTextContent() + return true } return super.performKeyEquivalent(with: event) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/FilePreviewPanel.swift` around lines 536 - 543, The keyboard handler should only attempt a save when the document is dirty and must surface the save outcome: in performKeyEquivalent, first unwrap panel and check panel.isDirty; if false, fall through to return super.performKeyEquivalent(...). If true, call panel.saveTextContent(), capture its Bool result, and return true only if the save succeeded (or otherwise let failure propagate/produce UI feedback as appropriate) so the keystroke does not silently claim success when save fails; reference performKeyEquivalent, panel?.saveTextContent(), and panel.isDirty (and note isFileUnavailable is set on failure in the save path).
618-632: Pointer observer fires on every left-mouse-down in the window.
addLocalMonitorForEvents(matching: [.leftMouseDown])sees every left-mouse-down in the window, and this class only filters by its own bounds. So clicks on the header revert/save buttons, the Quick Look chrome, scrollers, and the text view's own selection drags all enqueue an asynconRequestPanelFocus()call. That's probably harmless but it's a lot of extra focus churn (and log/telemetry noise if any is added later).If the intent is "user interacted with this panel", the monitor is fine; if the intent is "user clicked into the panel content from outside", consider also checking that the first-responder / focus state actually needs to change before invoking
onPointerDown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/FilePreviewPanel.swift` around lines 618 - 632, The current eventMonitor added in init (via addLocalMonitorForEvents) triggers on every leftMouseDown for the window and unconditionally enqueues onPointerDown when the click is inside bounds; change the closure to first check whether the panel already has focus or the window's firstResponder is already a descendant of this view before calling onPointerDown: inside the event monitor (eventMonitor / addLocalMonitorForEvents closure) compute the clicked point as you do, then if bounds.contains(point) verify whether window?.firstResponder is nil or isDescendant(of: self) (or use hitTest on self to find responder) and only dispatch onPointerDown when the current firstResponder is not already within this view (i.e., focus actually needs to change); keep the existing isHiddenOrHasHiddenAncestor and weak self guards.
355-357: Use the two-parameteronChange(of:)closure.The single-parameter closure form of
onChange(of:)is deprecated on macOS 14+/iOS 17+. Prefer the{ oldValue, newValue in ... }form (or{ _, _ in ... }if both are unused) to silence the deprecation warning and stay on the supported API.♻️ Proposed change
- .onChange(of: panel.focusFlashToken) { _ in - triggerFocusFlashAnimation() - } + .onChange(of: panel.focusFlashToken) { _, _ in + triggerFocusFlashAnimation() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/FilePreviewPanel.swift` around lines 355 - 357, The onChange(of:) call using a single-parameter closure is deprecated; update the invocation that watches panel.focusFlashToken to use the two-parameter form onChange(of:panel.focusFlashToken) { oldValue, newValue in ... } and call triggerFocusFlashAnimation() inside that closure (use `{ _, _ in triggerFocusFlashAnimation() }` if you don't need the values) so the code uses the supported API and removes the deprecation warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 286-299: The current catchless decode failure branch treats
decodeText(data) == nil the same as an I/O error by setting isFileUnavailable =
true, which incorrectly shows "file moved or deleted"; add a new Bool state
(e.g., isDecodeFailure) and set it to true when guard let decoded fails (and set
isFileUnavailable = false), assign textContent/originalTextContent/isDirty only
on successful decode, and ensure both flags are cleared appropriately on
successful load; update any UI bindings to show a decode-specific message and
optionally offer Quick Look when isDecodeFailure is true instead of relying on
isFileUnavailable.
- Around line 302-312: decodeText(_:) currently falls back through UTF-16/Latin1
but saveTextContent() always writes as .utf8, corrupting non-UTF8 files; modify
decodeText(_:) to record the detected String.Encoding in a new property (e.g.,
detectedTextEncoding), then update saveTextContent() to write using
detectedTextEncoding (or, if it’s not .utf8, either prompt/require user
confirmation or block the save and surface a clear conversion-warning), and
ensure originalTextContent/isDirty logic remains consistent; also stop reusing
isFileUnavailable for write failures—add a distinct save error state/property
(e.g., saveError or isSaveFailed) and set that in saveTextContent()'s catch so
UI can show an appropriate "save failed" message instead of "file
moved/deleted".
- Around line 237-249: The initializer FilePreviewPanel.init currently calls
loadTextContent() on the main actor, which performs synchronous
Data(contentsOf:) and can block the UI; change the logic to first fetch file
size via URL.resourceValues(forKeys: [.fileSizeKey]) and if size exceeds a
threshold switch previewMode to .quickLook or set a "file too
large"/isFileUnavailable state, otherwise start an asynchronous load (e.g.,
Task.detached { read Data and decode String } then await MainActor.run { assign
textContent, isLoading=false } ) instead of calling loadTextContent() directly;
also update the revert action (the button handler that currently calls
loadTextContent()) to invoke the same async loader so all file reads happen off
the main actor and results are marshalled back to the main actor for UI updates.
- Around line 194-209: The UTF-16 sniff in sniffLooksLikeText is too permissive;
update sniffLooksLikeText(url:) to only accept UTF-16 when a BOM is present or
when a stronger textual heuristic passes: first read the initial bytes (existing
data variable) and if the first two bytes are 0xFF 0xFE or 0xFE 0xFF treat it as
UTF-16 text, otherwise do not return true solely because String(data: ,
encoding: .utf16) succeeded — either drop the .utf16 branch or gate it behind
the BOM check (or add a printable-character ratio check on the decoded string
before returning true). Ensure you still keep the NUL-byte check and existing
UTF-8 check and close the FileHandle as before.
- Around line 65-89: The init(filePath:displayTitle:) in FilePreviewPanel
currently registers a UUID in FilePreviewDragRegistry and calls
mirrorTransferDataToDragPasteboard() immediately; move those side effects into
pasteboardPropertyList(forType:) so they only occur when an actual drag starts.
Keep building fileURL, displayTitle and the encoded transferData in init but
defer calling FilePreviewDragRegistry.shared.register(...) (and storing the
returned dragId into the MirrorTabItem payload) and defer calling
mirrorTransferDataToDragPasteboard() until pasteboardPropertyList(forType:) is
invoked; update pasteboardPropertyList(forType:) to create/register the
FilePreviewDragEntry, construct the MirrorTabTransferData (or update
transferData with the registry UUID) and perform the pasteboard write there.
Ensure you reference FilePreviewDragRegistry.register, MirrorTabTransferData /
MirrorTabItem, mirrorTransferDataToDragPasteboard(), and
pasteboardPropertyList(forType:) when making the change.
In `@Sources/Workspace.swift`:
- Around line 7485-7512: The subscription for FilePreviewPanel is lost when
panels are detached/moved because panelSubscriptions is cleared but
attachDetachedSurface(_:inPane:atIndex:focus:) never reinstalls it; modify
attachDetachedSurface(_:inPane:atIndex:focus:) to detect when the reattached
surface is a FilePreviewPanel (use a type check or unwrap the surface to
FilePreviewPanel) and call installFilePreviewPanelSubscription(filePreviewPanel)
after reattachment (and after any panel id/state restoration) so title and
dirty-state publishers are re-subscribed; ensure you also avoid duplicate
subscriptions by checking panelSubscriptions[filePreviewPanel.id] before
installing.
- Around line 9734-9750: This split-drop path currently only calls
bonsplitController.selectTab(...) and filePreviewPanel.focus(), bypassing the
workspace focus machinery and leaving focusedPanelId, sidebar metadata, and
keyboard focus stale; change the flow to route focus through the normal
workspace focus path used by newMarkdownSplit/newBrowserSplit by invoking the
same selection/focus helper that runs applyTabSelection (rather than calling
filePreviewPanel.focus() directly) so the destination pane/panel becomes the
canonical focusedPanelId and sidebar/keyboard focus are updated; locate the
split-drop helper (where bonsplitController.selectTab and
installFilePreviewPanelSubscription are called) and replace the direct
filePreviewPanel.focus() with the workspace-level selection call that triggers
applyTabSelection for the newly selected tab.
---
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 6071-6072: The switch case for .filePreview currently returns a
small keyword list which omits common media/search terms; update the return for
the .filePreview branch (the case .filePreview in the switch that returns
["file","preview","text","pdf","image"]) to include more discoverability
keywords such as "image", "photo", "picture", "video", "media", "attachment",
"document", "viewer", "open", "thumbnail", "preview", "file", "pdf", and "text"
so the file-preview panels surface in the command palette when users search with
media-centric terms.
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 536-543: The keyboard handler should only attempt a save when the
document is dirty and must surface the save outcome: in performKeyEquivalent,
first unwrap panel and check panel.isDirty; if false, fall through to return
super.performKeyEquivalent(...). If true, call panel.saveTextContent(), capture
its Bool result, and return true only if the save succeeded (or otherwise let
failure propagate/produce UI feedback as appropriate) so the keystroke does not
silently claim success when save fails; reference performKeyEquivalent,
panel?.saveTextContent(), and panel.isDirty (and note isFileUnavailable is set
on failure in the save path).
- Around line 618-632: The current eventMonitor added in init (via
addLocalMonitorForEvents) triggers on every leftMouseDown for the window and
unconditionally enqueues onPointerDown when the click is inside bounds; change
the closure to first check whether the panel already has focus or the window's
firstResponder is already a descendant of this view before calling
onPointerDown: inside the event monitor (eventMonitor / addLocalMonitorForEvents
closure) compute the clicked point as you do, then if bounds.contains(point)
verify whether window?.firstResponder is nil or isDescendant(of: self) (or use
hitTest on self to find responder) and only dispatch onPointerDown when the
current firstResponder is not already within this view (i.e., focus actually
needs to change); keep the existing isHiddenOrHasHiddenAncestor and weak self
guards.
- Around line 355-357: The onChange(of:) call using a single-parameter closure
is deprecated; update the invocation that watches panel.focusFlashToken to use
the two-parameter form onChange(of:panel.focusFlashToken) { oldValue, newValue
in ... } and call triggerFocusFlashAnimation() inside that closure (use `{ _, _
in triggerFocusFlashAnimation() }` if you don't need the values) so the code
uses the supported API and removes the deprecation warning.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85e0a7fd-750c-43d7-82ef-1af50e5d9dff
📒 Files selected for processing (10)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/ContentView.swiftSources/FileExplorerView.swiftSources/Panels/FilePreviewPanel.swiftSources/Panels/Panel.swiftSources/Panels/PanelContentView.swiftSources/SessionPersistence.swiftSources/Workspace.swiftSources/cmuxApp.swift
| init(filePath: String, displayTitle: String) { | ||
| self.fileURL = URL(fileURLWithPath: filePath) | ||
| let dragId = FilePreviewDragRegistry.shared.register( | ||
| FilePreviewDragEntry(filePath: filePath, displayTitle: displayTitle) | ||
| ) | ||
| let transfer = MirrorTabTransferData( | ||
| tab: MirrorTabItem( | ||
| id: dragId, | ||
| title: displayTitle, | ||
| hasCustomTitle: false, | ||
| icon: FilePreviewKindResolver.tabIconName(for: URL(fileURLWithPath: filePath)), | ||
| iconImageData: nil, | ||
| kind: "filePreview", | ||
| isDirty: false, | ||
| showsNotificationBadge: false, | ||
| isLoading: false, | ||
| isPinned: false | ||
| ), | ||
| sourcePaneId: UUID(), | ||
| sourceProcessId: Int32(ProcessInfo.processInfo.processIdentifier) | ||
| ) | ||
| self.transferData = (try? JSONEncoder().encode(transfer)) ?? Data() | ||
| super.init() | ||
| mirrorTransferDataToDragPasteboard() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how the bonsplit transfer payload is consumed for .filePreview panels.
rg -nP --type=swift -C4 'bonsplitTransferType|MirrorTabTransferData|filePreview'
rg -nP --type=swift -C3 'FilePreviewDragRegistry'Repository: manaflow-ai/cmux
Length of output: 29341
🏁 Script executed:
# Find where FilePreviewDragPasteboardWriter is instantiated
rg -n 'FilePreviewDragPasteboardWriter' --type=swiftRepository: manaflow-ai/cmux
Length of output: 310
🏁 Script executed:
# Check the full FilePreviewDragPasteboardWriter class definition and its parent
rg -n -A 50 'class FilePreviewDragPasteboardWriter' Sources/Panels/FilePreviewPanel.swift | head -100Repository: manaflow-ai/cmux
Length of output: 1987
🏁 Script executed:
# Look for NSPasteboard or pasteboard writing protocol adoption to understand lifecycle
rg -n 'NSPasteboard' Sources/Panels/FilePreviewPanel.swift -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 1059
🏁 Script executed:
# Check the context around line 213 in FileExplorerView.swift
sed -n '200,230p' Sources/FileExplorerView.swiftRepository: manaflow-ai/cmux
Length of output: 1537
🏁 Script executed:
# Look for the full method containing the instantiation
rg -n -B 15 'FilePreviewDragPasteboardWriter' Sources/FileExplorerView.swift | head -40Repository: manaflow-ai/cmux
Length of output: 869
🏁 Script executed:
# Check FilePreviewDragRegistry to understand the 60-second timer concern
rg -n -A 30 'class FilePreviewDragRegistry' Sources/Panels/FilePreviewPanel.swiftRepository: manaflow-ai/cmux
Length of output: 1080
Defer FilePreviewDragPasteboardWriter registration and pasteboard writes to actual drag start.
Every call to pasteboardWriterForItem: (line 213 in FileExplorerView.swift) instantiates a writer, but NSOutlineView calls this method repeatedly during mouse hover and probing, not just when a drag actually begins. This causes the init to unconditionally:
- Register a new UUID in
FilePreviewDragRegistrywith a 60-second timer - Write to the
.dragpasteboard viamirrorTransferDataToDragPasteboard()
Both operations happen speculatively and accumulate short-lived registry entries and unnecessary pasteboard mutations. Move the register() call and pasteboard write into the pasteboardPropertyList(forType:) method, which is only invoked during an actual drag operation.
The sourcePaneId and sourceProcessId fields in the transfer payload are not used by the consumer (Workspace.swift:11683 keys off the registry UUID in MirrorTabItem.id), so no functional issue there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` around lines 65 - 89, The
init(filePath:displayTitle:) in FilePreviewPanel currently registers a UUID in
FilePreviewDragRegistry and calls mirrorTransferDataToDragPasteboard()
immediately; move those side effects into pasteboardPropertyList(forType:) so
they only occur when an actual drag starts. Keep building fileURL, displayTitle
and the encoded transferData in init but defer calling
FilePreviewDragRegistry.shared.register(...) (and storing the returned dragId
into the MirrorTabItem payload) and defer calling
mirrorTransferDataToDragPasteboard() until pasteboardPropertyList(forType:) is
invoked; update pasteboardPropertyList(forType:) to create/register the
FilePreviewDragEntry, construct the MirrorTabTransferData (or update
transferData with the registry UUID) and perform the pasteboard write there.
Ensure you reference FilePreviewDragRegistry.register, MirrorTabTransferData /
MirrorTabItem, mirrorTransferDataToDragPasteboard(), and
pasteboardPropertyList(forType:) when making the change.
| private static func sniffLooksLikeText(url: URL) -> Bool { | ||
| guard let handle = try? FileHandle(forReadingFrom: url) else { return false } | ||
| defer { try? handle.close() } | ||
| let data = (try? handle.read(upToCount: 4096)) ?? Data() | ||
| guard !data.isEmpty else { return true } | ||
| if data.contains(0) { | ||
| return false | ||
| } | ||
| if String(data: data, encoding: .utf8) != nil { | ||
| return true | ||
| } | ||
| if String(data: data, encoding: .utf16) != nil { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
sniffLooksLikeText UTF-16 check is too permissive.
String(data:encoding: .utf16) succeeds on almost any even-length byte sequence without a BOM (it just interprets pairs of bytes as UTF-16 code units in native endianness). Combined with the "no NUL bytes in first 4 KB" heuristic, this can classify random binary payloads as text and route them into the editable text path rather than Quick Look — where they'd then hit the "decode failure" path or (worse) be editable and saved back as UTF-8 garbage.
Consider either dropping the UTF-16 sniff entirely, or only treating it as text when a BOM is present (FF FE / FE FF) or when the decoded string's character profile looks like text (ratio of printable/control characters, etc.).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` around lines 194 - 209, The UTF-16
sniff in sniffLooksLikeText is too permissive; update sniffLooksLikeText(url:)
to only accept UTF-16 when a BOM is present or when a stronger textual heuristic
passes: first read the initial bytes (existing data variable) and if the first
two bytes are 0xFF 0xFE or 0xFE 0xFF treat it as UTF-16 text, otherwise do not
return true solely because String(data: , encoding: .utf16) succeeded — either
drop the .utf16 branch or gate it behind the BOM check (or add a
printable-character ratio check on the decoded string before returning true).
Ensure you still keep the NUL-byte check and existing UTF-8 check and close the
FileHandle as before.
| init(workspaceId: UUID, filePath: String) { | ||
| self.id = UUID() | ||
| self.workspaceId = workspaceId | ||
| self.filePath = filePath | ||
| self.displayTitle = URL(fileURLWithPath: filePath).lastPathComponent | ||
| self.previewMode = FilePreviewKindResolver.mode(for: URL(fileURLWithPath: filePath)) | ||
|
|
||
| if previewMode == .text { | ||
| loadTextContent() | ||
| } else { | ||
| isFileUnavailable = !FileManager.default.fileExists(atPath: filePath) | ||
| } | ||
| } |
There was a problem hiding this comment.
Synchronous disk I/O in @MainActor init can block the UI.
init runs on the main actor and, for text previews, calls loadTextContent() → Data(contentsOf: fileURL) (line 287) on the main thread. There's no size cap, so dropping a large text-ish file (e.g., a multi-hundred-MB log that still passes sniffLooksLikeText) will freeze the window until the read completes and the full string is materialized.
Consider:
- Checking
resourceValues(forKeys: [.fileSizeKey])and short-circuiting to.quickLook(or a "file too large" state) above some threshold. - Loading the contents asynchronously (e.g., a
Task.detachedthat hands the decoded string back to the main actor) so the panel can render a placeholder first.
Same concern applies to the revert button (line 374) invoking loadTextContent() synchronously on main.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` around lines 237 - 249, The
initializer FilePreviewPanel.init currently calls loadTextContent() on the main
actor, which performs synchronous Data(contentsOf:) and can block the UI; change
the logic to first fetch file size via URL.resourceValues(forKeys:
[.fileSizeKey]) and if size exceeds a threshold switch previewMode to .quickLook
or set a "file too large"/isFileUnavailable state, otherwise start an
asynchronous load (e.g., Task.detached { read Data and decode String } then
await MainActor.run { assign textContent, isLoading=false } ) instead of calling
loadTextContent() directly; also update the revert action (the button handler
that currently calls loadTextContent()) to invoke the same async loader so all
file reads happen off the main actor and results are marshalled back to the main
actor for UI updates.
| do { | ||
| let data = try Data(contentsOf: fileURL) | ||
| let decoded = Self.decodeText(data) | ||
| guard let decoded else { | ||
| isFileUnavailable = true | ||
| return | ||
| } | ||
| textContent = decoded | ||
| originalTextContent = decoded | ||
| isDirty = false | ||
| isFileUnavailable = false | ||
| } catch { | ||
| isFileUnavailable = true | ||
| } |
There was a problem hiding this comment.
Decode failure on an existing file is surfaced as "File unavailable".
When the file exists but decodeText returns nil (e.g., binary content that slipped through sniffLooksLikeText, or an unsupported encoding), this path sets isFileUnavailable = true, which renders the localized message "The file may have been moved or deleted." That message is factually wrong in this case and will confuse users.
Consider introducing a distinct state (e.g., isDecodeFailure / "Unable to decode file contents") so the UI can show an accurate message, and optionally offer a fallback to Quick Look.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` around lines 286 - 299, The current
catchless decode failure branch treats decodeText(data) == nil the same as an
I/O error by setting isFileUnavailable = true, which incorrectly shows "file
moved or deleted"; add a new Bool state (e.g., isDecodeFailure) and set it to
true when guard let decoded fails (and set isFileUnavailable = false), assign
textContent/originalTextContent/isDirty only on successful decode, and ensure
both flags are cleared appropriately on successful load; update any UI bindings
to show a decode-specific message and optionally offer Quick Look when
isDecodeFailure is true instead of relying on isFileUnavailable.
| private func installFilePreviewPanelSubscription(_ filePreviewPanel: FilePreviewPanel) { | ||
| let subscription = Publishers.CombineLatest( | ||
| filePreviewPanel.$displayTitle.removeDuplicates(), | ||
| filePreviewPanel.$isDirty.removeDuplicates() | ||
| ) | ||
| .receive(on: DispatchQueue.main) | ||
| .sink { [weak self, weak filePreviewPanel] newTitle, isDirty in | ||
| guard let self, | ||
| let filePreviewPanel, | ||
| let tabId = self.surfaceIdFromPanelId(filePreviewPanel.id) else { return } | ||
| guard let existing = self.bonsplitController.tab(tabId) else { return } | ||
|
|
||
| if self.panelTitles[filePreviewPanel.id] != newTitle { | ||
| self.panelTitles[filePreviewPanel.id] = newTitle | ||
| } | ||
| let resolvedTitle = self.resolvedPanelTitle(panelId: filePreviewPanel.id, fallback: newTitle) | ||
| let titleUpdate: String? = existing.title == resolvedTitle ? nil : resolvedTitle | ||
| let dirtyUpdate: Bool? = existing.isDirty == isDirty ? nil : isDirty | ||
| guard titleUpdate != nil || dirtyUpdate != nil else { return } | ||
| self.bonsplitController.updateTab( | ||
| tabId, | ||
| title: titleUpdate, | ||
| hasCustomTitle: self.panelCustomTitles[filePreviewPanel.id] != nil, | ||
| isDirty: dirtyUpdate | ||
| ) | ||
| } | ||
| panelSubscriptions[filePreviewPanel.id] = subscription | ||
| } |
There was a problem hiding this comment.
Reinstall this subscription when a preview tab is reattached.
Detach/move flows clear panelSubscriptions, but attachDetachedSurface(_:inPane:atIndex:focus:) never calls installFilePreviewPanelSubscription(...). After moving a file-preview tab between panes/workspaces, later title changes and dirty-state updates stop propagating to Bonsplit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 7485 - 7512, The subscription for
FilePreviewPanel is lost when panels are detached/moved because
panelSubscriptions is cleared but attachDetachedSurface(_:inPane:atIndex:focus:)
never reinstalls it; modify attachDetachedSurface(_:inPane:atIndex:focus:) to
detect when the reattached surface is a FilePreviewPanel (use a type check or
unwrap the surface to FilePreviewPanel) and call
installFilePreviewPanelSubscription(filePreviewPanel) after reattachment (and
after any panel id/state restoration) so title and dirty-state publishers are
re-subscribed; ensure you also avoid duplicate subscriptions by checking
panelSubscriptions[filePreviewPanel.id] before installing.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1b15f1784
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let node = item as? FileExplorerNode, !node.isDirectory else { return nil } | ||
| guard store.provider is LocalFileExplorerProvider else { return nil } | ||
| return NSURL(fileURLWithPath: node.path) | ||
| return FilePreviewDragPasteboardWriter(filePath: node.path, displayTitle: node.name) |
There was a problem hiding this comment.
This change makes sidebar file drags publish com.splittabbar.tabtransfer with synthetic tab IDs, but only Workspace.handleExternalTabDrop consumes FilePreviewDragRegistry. Browser-pane targets use BrowserPaneDropTargetView.performDragOperation (in Sources/BrowserWindowPortal.swift) and call AppDelegate.moveBonsplitTab, which requires a real tab ID, so dropping a sidebar file onto a browser pane insert/split zone fails with tabNotFound instead of opening a preview panel.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
You’re at about 91% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/FilePreviewPanel.swift">
<violation number="1" location="Sources/Panels/FilePreviewPanel.swift:199">
P2: The null byte check prevents UTF-16 text files from being correctly identified, causing them to incorrectly fall back to QuickLook.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| defer { try? handle.close() } | ||
| let data = (try? handle.read(upToCount: 4096)) ?? Data() | ||
| guard !data.isEmpty else { return true } | ||
| if data.contains(0) { |
There was a problem hiding this comment.
P2: The null byte check prevents UTF-16 text files from being correctly identified, causing them to incorrectly fall back to QuickLook.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/FilePreviewPanel.swift, line 199:
<comment>The null byte check prevents UTF-16 text files from being correctly identified, causing them to incorrectly fall back to QuickLook.</comment>
<file context>
@@ -0,0 +1,647 @@
+ defer { try? handle.close() }
+ let data = (try? handle.read(upToCount: 4096)) ?? Data()
+ guard !data.isEmpty else { return true }
+ if data.contains(0) {
+ return false
+ }
</file context>
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
498-550:⚠️ Potential issue | 🟠 MajorDirty previews are not actually session-restorable.
SessionFilePreviewPanelSnapshotonly savesfilePath, and restore recreates the panel by callingnewFilePreviewSurface(...). SinceFilePreviewPanel.init(workspaceId:filePath:)reloads from disk, any unsaved text buffer and dirty state are lost across restart even though this PR advertises session restore for editable previews. Persist the in-memory text/dirty metadata, or explicitly exclude dirty previews from restore and force the user to save first.Also applies to: 788-798
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 498 - 550, The SessionFilePreviewPanelSnapshot currently only stores filePath, so dirty (unsaved) in-memory buffers are lost on restore; update the snapshot flow in Workspace.swift to either persist the in-memory preview contents and dirty flag into SessionFilePreviewPanelSnapshot and use those values when recreating the preview (reference SessionFilePreviewPanelSnapshot, FilePreviewPanel.init(workspaceId:filePath:), and newFilePreviewSurface(...)), or explicitly prevent restoring dirty previews by checking the FilePreviewPanel's dirty state before creating a snapshot and exclude them (or mark them non-restorable) so users must save first; choose one approach and apply it consistently where filePreview snapshots are created (also update the corresponding restore code paths).
♻️ Duplicate comments (2)
Sources/Workspace.swift (2)
9730-9768:⚠️ Potential issue | 🟠 MajorUse the normal selection path after split-drop.
This split helper only does
selectTab+filePreviewPanel.focus(). Unlike the other split flows, it never focuses the destination pane or runsapplyTabSelection(...), so split-target drops can leavefocusedPanelId, sidebar metadata, and keyboard focus stale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 9730 - 9768, The splitPaneWithFilePreview helper currently only calls bonsplitController.selectTab(...) and filePreviewPanel.focus(), which skips the normal selection path and leaves focusedPanelId, sidebar metadata, and keyboard focus stale; update splitPaneWithFilePreview to mirror the other split flows by focusing the destination pane and invoking the central selection handler (e.g., call applyTabSelection(...) or the same method used elsewhere after bonsplitController.selectTab) so the workspace's focusedPanelId, sidebar state, and keyboard focus are updated, then proceed to installFilePreviewPanelSubscription(filePreviewPanel) as before.
7485-7512:⚠️ Potential issue | 🟠 MajorMoved preview tabs still lose their Bonsplit subscription.
This helper is wired for newly created previews, but
attachDetachedSurface(_:inPane:atIndex:focus:)still never callsinstallFilePreviewPanelSubscription(...). After moving a file-preview tab between panes/workspaces, later title and dirty-state changes stop propagating to Bonsplit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 7485 - 7512, attachDetachedSurface(_:inPane:atIndex:focus:) never wires up the Combine subscription for moved FilePreviewPanel instances, so ensure that when a surface being attached is a FilePreviewPanel you call installFilePreviewPanelSubscription(_:) (and manage panelSubscriptions[filePreviewPanel.id] appropriately to avoid duplicates) after the surface is attached; use surfaceIdFromPanelId(_:) and panelCustomTitles/panelTitles logic already used in installFilePreviewPanelSubscription to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/FileExplorerView.swift`:
- Around line 224-231: The current
outlineView(_:draggingSession:endedAt:operation:) calls
FilePreviewDragRegistry.discardAll(), which clears the global registry and can
orphan entries for concurrent drags; instead, make cleanup session-scoped by
having each FilePreviewDragPasteboardWriter hold its registered UUID (from
register()) or add FilePreviewDragRegistry.discard(id:) and have the writer
expose its id, then in outlineView(_:draggingSession:endedAt:operation:)
consume/discard only the IDs associated with that NSDraggingSession (e.g., store
the writer IDs on session or retrieve them from the session's draggingItems) and
call FilePreviewDragRegistry.consume(id:) or discard(id:) for each, removing the
global discardAll() call.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 498-550: The SessionFilePreviewPanelSnapshot currently only stores
filePath, so dirty (unsaved) in-memory buffers are lost on restore; update the
snapshot flow in Workspace.swift to either persist the in-memory preview
contents and dirty flag into SessionFilePreviewPanelSnapshot and use those
values when recreating the preview (reference SessionFilePreviewPanelSnapshot,
FilePreviewPanel.init(workspaceId:filePath:), and newFilePreviewSurface(...)),
or explicitly prevent restoring dirty previews by checking the
FilePreviewPanel's dirty state before creating a snapshot and exclude them (or
mark them non-restorable) so users must save first; choose one approach and
apply it consistently where filePreview snapshots are created (also update the
corresponding restore code paths).
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 9730-9768: The splitPaneWithFilePreview helper currently only
calls bonsplitController.selectTab(...) and filePreviewPanel.focus(), which
skips the normal selection path and leaves focusedPanelId, sidebar metadata, and
keyboard focus stale; update splitPaneWithFilePreview to mirror the other split
flows by focusing the destination pane and invoking the central selection
handler (e.g., call applyTabSelection(...) or the same method used elsewhere
after bonsplitController.selectTab) so the workspace's focusedPanelId, sidebar
state, and keyboard focus are updated, then proceed to
installFilePreviewPanelSubscription(filePreviewPanel) as before.
- Around line 7485-7512: attachDetachedSurface(_:inPane:atIndex:focus:) never
wires up the Combine subscription for moved FilePreviewPanel instances, so
ensure that when a surface being attached is a FilePreviewPanel you call
installFilePreviewPanelSubscription(_:) (and manage
panelSubscriptions[filePreviewPanel.id] appropriately to avoid duplicates) after
the surface is attached; use surfaceIdFromPanelId(_:) and
panelCustomTitles/panelTitles logic already used in
installFilePreviewPanelSubscription to keep behavior consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d653b633-24a1-462d-851a-605963e49844
📒 Files selected for processing (5)
Sources/ContentView.swiftSources/FileExplorerView.swiftSources/Panels/FilePreviewPanel.swiftSources/RightSidebarPanelView.swiftSources/Workspace.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/ContentView.swift
- Sources/Panels/FilePreviewPanel.swift
| func outlineView( | ||
| _ outlineView: NSOutlineView, | ||
| draggingSession session: NSDraggingSession, | ||
| endedAt screenPoint: NSPoint, | ||
| operation: NSDragOperation | ||
| ) { | ||
| FilePreviewDragRegistry.shared.discardAll() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm FilePreviewDragPasteboardWriter is used from more than one call site,
# which would make discardAll() here racy with other drag sources.
rg -nP --type=swift -C2 '\bFilePreviewDragPasteboardWriter\s*\('
rg -nP --type=swift -C2 '\bFilePreviewDragRegistry\.shared\.(register|consume|discardAll)\b'Repository: manaflow-ai/cmux
Length of output: 1660
🏁 Script executed:
# Check FilePreviewDragPasteboardWriter implementation
fd -e swift FileExplorerView.swift -x cat -n {} | sed -n '210,240p'Repository: manaflow-ai/cmux
Length of output: 1568
🏁 Script executed:
# Check FilePreviewDragPasteboardWriter class definition and init
rg -nA15 'class FilePreviewDragPasteboardWriter' --type=swiftRepository: manaflow-ai/cmux
Length of output: 1214
🏁 Script executed:
# Check FilePreviewPanel registration context
rg -nB5 -A10 'FilePreviewDragRegistry.*register' Sources/Panels/FilePreviewPanel.swiftRepository: manaflow-ai/cmux
Length of output: 797
🏁 Script executed:
# Check FilePreviewDragRegistry implementation
rg -nA30 'class FilePreviewDragRegistry\|struct FilePreviewDragRegistry' --type=swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Search for FilePreviewDragRegistry in all swift files
rg -n 'FilePreviewDragRegistry' --type=swift | head -20Repository: manaflow-ai/cmux
Length of output: 542
🏁 Script executed:
# Find where FilePreviewDragRegistry is defined
fd -e swift -x rg -l 'FilePreviewDragRegistry' {} \;Repository: manaflow-ai/cmux
Length of output: 159
🏁 Script executed:
# Search for the actual registry class definition
rg -nB2 -A20 'final class FilePreviewDragRegistry\|class FilePreviewDragRegistry\|struct FilePreviewDragRegistry' --type=swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Read the FilePreviewDragRegistry implementation
sed -n '14,50p' Sources/Panels/FilePreviewPanel.swiftRepository: manaflow-ai/cmux
Length of output: 1036
🏁 Script executed:
# Also check FileExplorerView usage context around line 221 to understand when writes happen
sed -n '218,232p' Sources/FileExplorerView.swiftRepository: manaflow-ai/cmux
Length of output: 750
🏁 Script executed:
# Check Workspace consume usage context to understand the full lifecycle
sed -n '11695,11710p' Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 756
🏁 Script executed:
# Check if there are any other drag-related callbacks or multi-drag patterns
rg -n 'draggingSession|pasteboardWriter|NSDraggingSession' Sources/FileExplorerView.swiftRepository: manaflow-ai/cmux
Length of output: 246
🏁 Script executed:
# Verify if FilePreviewPanel has any drag source code we missed
rg -n 'draggingSession\|pasteboardWriter\|FilePreviewDragRegistry' Sources/Panels/FilePreviewPanel.swiftRepository: manaflow-ai/cmux
Length of output: 42
discardAll() is too broad for per-drag cleanup — consider tracking this session's registered IDs instead.
FilePreviewDragRegistry provides only register() / consume(id:) / discardAll(). Every FilePreviewDragPasteboardWriter instantiation (from FileExplorerView's pasteboardWriterForItem delegate method) registers a fresh UUID. Calling discardAll() from the outline view's draggingSession:endedAt: clears every entry in the process-wide registry without session association, which risks orphaning entries if multiple file drags overlap in timing—even from the same source.
Safer options:
- Track UUIDs per
NSDraggingSession(e.g., store the ID in the writer and retrieve it on end, or maintain a session→[IDs] map) andconsume(id:)only those IDs. - Add
discard(id:)to the registry and have the writer remember its ID, then discard it specifically inendedAt. - Add auto-expiry to
FilePreviewDragRegistry(mirroringSessionDragRegistryatSources/SessionIndexStore.swift:180-205) and remove thediscardAll()call entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/FileExplorerView.swift` around lines 224 - 231, The current
outlineView(_:draggingSession:endedAt:operation:) calls
FilePreviewDragRegistry.discardAll(), which clears the global registry and can
orphan entries for concurrent drags; instead, make cleanup session-scoped by
having each FilePreviewDragPasteboardWriter hold its registered UUID (from
register()) or add FilePreviewDragRegistry.discard(id:) and have the writer
expose its id, then in outlineView(_:draggingSession:endedAt:operation:)
consume/discard only the IDs associated with that NSDraggingSession (e.g., store
the writer IDs on session or retrieve them from the session's draggingItems) and
call FilePreviewDragRegistry.consume(id:) or discard(id:) for each, removing the
global discardAll() call.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79891a1750
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func saveTextContent() { | ||
| guard previewMode == .text else { return } | ||
| do { | ||
| try textContent.write(to: fileURL, atomically: true, encoding: .utf8) |
There was a problem hiding this comment.
This save path always writes UTF-8, but loadTextContent()/decodeText() explicitly accepts UTF-16 and Latin-1 inputs. When a user edits a non-UTF-8 file (for example UTF-16 config files), saving rewrites it in a different encoding, which can corrupt non-ASCII content or break tools expecting the original encoding. Track the detected encoding and write back in that encoding (or explicitly block editing for unsupported encodings).
Useful? React with 👍 / 👎.
| } catch { | ||
| isFileUnavailable = true |
There was a problem hiding this comment.
On any write error (permission denied, read-only file, disk full), this marks the file as unavailable, which immediately swaps the editor for the “moved or deleted” placeholder even though the file may still exist and the unsaved text is still in memory. That creates a misleading state and interrupts recovery workflows; save errors should be surfaced without flipping to the unavailable view.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da449ff480
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func saveTextContent() { | ||
| guard previewMode == .text else { return } | ||
| do { | ||
| try textContent.write(to: fileURL, atomically: true, encoding: .utf8) |
There was a problem hiding this comment.
Using String.write(..., atomically: true, ...) against fileURL will replace a symlink path with a new regular file instead of updating the symlink target, so editing a preview opened through a symlink can silently break that link and leave the original target unchanged. This is a data-loss regression for common workflows (dotfiles/repos that rely on symlinked config files).
Useful? React with 👍 / 👎.
| let data = try Data(contentsOf: fileURL) | ||
| let decoded = Self.decodeText(data) |
There was a problem hiding this comment.
FilePreviewPanel is @MainActor, and loadTextContent() performs synchronous Data(contentsOf:) plus full decode on that actor, so opening or reverting a large/remote text file blocks the UI thread and can freeze interaction for noticeable periods. This should be done off-main (with size/timeout guards) and only publish state updates back on main.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxTests/WindowAndDragTests.swift (1)
1212-1270:⚠️ Potential issue | 🟡 MinorTest name implies a conditional gate that isn't actually set, and sibling test message is now stale.
Two small consistency issues worth cleaning up:
The name
testOverlayDoesNotCaptureFileDragLifecycleWhenPanePreviewDropsAreEnabledreads as if it exercises a "pane preview drops enabled" mode, but the body never enables such a state — it just constructs aFileDropOverlayViewin isolation and asserts it returns the empty/falselifecycle results unconditionally. If there is a runtime flag/feature toggle for pane-preview drops, flip it explicitly insetUp(and consider adding a paired test for the disabled path); if not, drop the "WhenPanePreviewDropsAreEnabled" suffix so the name matches what is verified.The sibling test at Lines 1206-1209 still justifies its assertion with
"File-drop overlay should resolve portal-hosted browser panes so Finder uploads still reach WKWebView". Per this new test, Finder file drops no longer reach the WKWebView through the root overlay at all, so that rationale is now misleading. Update the message (or fold the two tests together) so the intent ofwebViewUnderPointresolution versus drag-lifecycle forwarding is clear to future readers.Also optional:
DragSpyWebView's drag overrides now exist purely to assert absence — consider renaming to something likeDragAssertingWebViewor inlining a singleXCTAssert(webView.dragCalls.isEmpty, …)without the overrides, since they can no longer be reached.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WindowAndDragTests.swift` around lines 1212 - 1270, The test testOverlayDoesNotCaptureFileDragLifecycleWhenPanePreviewDropsAreEnabled is misleading because it never enables any "pane preview drops" mode; either enable the feature toggle in setUp (or within the test) before creating FileDropOverlayView, or rename the test to remove the "WhenPanePreviewDropsAreEnabled" suffix so the name matches the verified behavior; also update the sibling test's failure message that references "Finder file drops should reach WKWebView" to reflect that Finder drops are expected to be handled by pane-level resolution (adjust the XCTAssert message near webViewUnderPoint / DragSpyWebView usage), and consider renaming DragSpyWebView to DragAssertingWebView or replace its overrides with a direct XCTAssert(webView.dragCalls.isEmpty, ...) to clarify that drag overrides are only asserting absence.tests/test_bonsplit_tab_drag_overlay_gate.py (1)
181-219:⚠️ Potential issue | 🟡 MinorLGTM — expectations correctly flip to pane-level routing for file URL drags.
The matrix asserts that when a file URL is on the drag pasteboard:
- FileDropOverlayView hit/drop gates stay closed (overlay does not capture the drag),
- Terminal portal passes through only during DRAG_EVENTS, while NON_DRAG_EVENTS + ["none"] remain closed to prevent stale-payload hijacking.
The omission of
assert_hit_chain_routes_to_panefor fileurl payloads is intentional — file drags route through FileDropOverlayView's direct pane-drop mechanism rather than via Workspace hit-chain routing (which tabtransfer and sidebar_reorder use). Pane routing is still validated through theassert_drop_gateassertions (expected=False), which confirm drops bypass the overlay and reach pane targets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bonsplit_tab_drag_overlay_gate.py` around lines 181 - 219, Add a short clarifying comment above the file URL test block explaining why assert_hit_chain_routes_to_pane is intentionally omitted for fileurl payloads: file drags route via FileDropOverlayView's direct pane-drop mechanism rather than workspace hit-chain routing, and pane delivery is already validated by the existing assert_drop_gate checks; reference the test assertions assert_drop_gate and assert_portal_gate and the omitted symbol assert_hit_chain_routes_to_pane so future readers understand the intent.
♻️ Duplicate comments (2)
Sources/Workspace.swift (2)
7488-7515:⚠️ Potential issue | 🟠 MajorReinstall this subscription when a preview tab is reattached.
attachDetachedSurface(_:inPane:atIndex:focus:)still only reinstalls browser observers. After aFilePreviewPanelis moved between panes/workspaces,panelSubscriptionsstays empty for it, so later title and dirty-state changes stop propagating to Bonsplit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 7488 - 7515, The FilePreviewPanel subscription isn't recreated when a preview tab is reattached, so update attachDetachedSurface(_:inPane:atIndex:focus:) to detect when the reattached surface is a FilePreviewPanel and call installFilePreviewPanelSubscription(_:) (or otherwise recreate the subscription and store it in panelSubscriptions[filePreviewPanel.id]); ensure you only reinstall if panelSubscriptions[filePreviewPanel.id] is nil (or cancel/replace an existing subscription), and reference the panel's id, panelSubscriptions, and bonsplitController.updateTab flow so title/isDirty changes resume propagating.
9733-9771:⚠️ Potential issue | 🟠 MajorRoute split-drop focus through the normal workspace selection path.
This helper still does direct
selectTab+filePreviewPanel.focus(). That bypassesapplyTabSelection(...), so split-target drops can leavefocusedPanelId, sidebar metadata, and keyboard focus stale.Suggested fix
- bonsplitController.selectTab(newTab.id) - filePreviewPanel.focus() + bonsplitController.focusPane(paneId) + bonsplitController.selectTab(newTab.id) + applyTabSelection(tabId: newTab.id, inPane: paneId) installFilePreviewPanelSubscription(filePreviewPanel) return filePreviewPanel🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 9733 - 9771, The current splitPaneWithFilePreview creates a tab then calls bonsplitController.selectTab(...) and filePreviewPanel.focus(), which bypasses the workspace selection flow and leaves focusedPanelId, sidebar metadata, and keyboard focus stale; replace those direct calls by routing selection through the workspace selection handler — call applyTabSelection(newTab.id) (the existing applyTabSelection(...) method) instead of bonsplitController.selectTab(...) and remove the direct filePreviewPanel.focus() call, and ensure installFilePreviewPanelSubscription(filePreviewPanel) remains after applyTabSelection so subscriptions are set up once the workspace selection state (focusedPanelId, sidebar metadata, keyboard focus) is correctly applied.
🧹 Nitpick comments (2)
Resources/Localizable.xcstrings (1)
86656-86915: Optional: consolidate duplicated PDF/image zoom labels.
filePreview.pdf.zoomIn,filePreview.pdf.zoomOut,filePreview.pdf.zoomToFit, andfilePreview.pdf.actualSizecarry identical English text and identical translations to theirfilePreview.image.*counterparts. Unless divergent wording per mode is anticipated, a single shared key (e.g.filePreview.zoomIn,filePreview.zoomOut,filePreview.zoomToFit,filePreview.actualSize) would reduce catalog duplication and keep translations from drifting between modes.No behavior change required if you'd prefer to keep them namespaced for future per-mode wording.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/Localizable.xcstrings` around lines 86656 - 86915, The PDF and image zoom/localization keys (filePreview.pdf.zoomIn/out/toFit/actualSize and filePreview.image.zoomIn/out/toFit/actualSize) are duplicated; please consolidate them by adding shared keys (e.g. filePreview.zoomIn, filePreview.zoomOut, filePreview.zoomToFit, filePreview.actualSize) with the same localization blocks, remove the duplicated pdf/image entries, and update all code sites that reference the old keys to use the new shared keys (search for uses of filePreview.pdf.* and filePreview.image.* to update). If you need to avoid immediate runtime breakage, keep the old keys as thin aliases that reference the same stringUnit/value while you migrate callers. Ensure string identifiers exactly match the proposed shared names so translations remain consistent.Sources/ContentView.swift (1)
2547-2549: Add inline debug event logging for the new sidebar file-preview action.This introduces a new UI action path but doesn’t emit a debug event at the view callsite.
♻️ Suggested update
onOpenFilePreview: { filePath in + `#if` DEBUG + cmuxDebugLog( + "sidebar.filePreview.open pathBytes=\((filePath as NSString).length)" + ) + `#endif` openFilePreviewFromSidebar(filePath: filePath) }As per coding guidelines:
Sources/**/*View.swift: Mouse and UI events must be logged inline in views (ContentView, BrowserPanelView, etc.) using the debug event log.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 2547 - 2549, Add an inline debug event log call inside the onOpenFilePreview closure (before calling openFilePreviewFromSidebar(filePath:)) so the new sidebar file-preview action is emitted to the debug event log; specifically, locate the onOpenFilePreview: { filePath in ... } closure in ContentView and insert a debug-event invocation (using the project's debug event logger API) that includes the event name (e.g., "sidebar.openFilePreview") and the filePath as metadata, then call openFilePreviewFromSidebar(filePath: filePath) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 780-781: The localized page label currently uses String(format:)
which is not locale-aware for digits or argument reordering; change the
construction so the localized format is produced with
String.localizedStringWithFormat (or use the localized interpolated
defaultValue) and assign that to pageLabel.stringValue; update the use sites
around the format constant named format and the assignment to
pageLabel.stringValue to call String.localizedStringWithFormat(format, pageIndex
+ 1, document.pageCount) (or replace the localized string with an interpolated
localized value) so numbers and positional arguments are formatted correctly in
non‑Latin locales.
- Around line 307-327: loadTextContent currently only sets isFileUnavailable
when the file is missing or read fails, leaving textContent, originalTextContent
and isDirty stale; update loadTextContent so that in the guard branch
(FileManager.default.fileExists check) and in the catch branch you also reset
textContent and originalTextContent to an empty string (or nil if your types
allow) and set isDirty = false in addition to isFileUnavailable = true; locate
the function loadTextContent and the bindings textContent, originalTextContent,
isDirty, isFileUnavailable (and the decode path Self.decodeText /
Data(contentsOf: fileURL)) and apply the symmetric reset behavior in both the
missing-file and error branches.
- Around line 613-619: FilePreviewPDFContainerView.setURL(_:) is creating
PDFDocument(url:) on the main thread; change it to capture the incoming URL into
a local loadURL, perform PDFDocument(url:) creation on a background queue, then
dispatch back to the main queue to assign pdfView.document and call
pdfView.autoScales/updatePageControls only if currentURL == loadURL; apply the
same pattern in FilePreviewImageContainerView.setURL(_:) for
NSImage(contentsOf:) (create the NSImage off‑main, then assign on main guarded
by the same currentURL == loadURL check to avoid clobbering newer loads).
In `@Sources/Workspace.swift`:
- Around line 9683-9730: The close-confirmation logic currently only checks for
TerminalPanel in splitTabBar(_:shouldCloseTab:inPane:), so dirty
FilePreviewPanel instances can be closed without prompt; update
splitTabBar(_:shouldCloseTab:inPane:) to look up the panel via
surfaceIdToPanelId/newTabId (or panels[panelId]) and treat FilePreviewPanel the
same as TerminalPanel by checking its isDirty flag (FilePreviewPanel.isDirty)
and routing through the close-confirmation path when dirty; ensure you reference
FilePreviewPanel and TerminalPanel types and use the existing panels
dictionary/surfaceIdToPanelId mapping to determine the panel before deciding to
show confirmation.
In `@vendor/bonsplit`:
- Line 1: The parent repo was updated to point vendor/bonsplit at commit
19255c6e85224f32bb1363914478113063427cf3 but that commit is not yet on the
submodule remote; push that commit to the submodule's origin main branch first
(checkout the submodule, ensure the commit exists locally, git push origin
<branch-or-commit-ref>:main), verify the commit is now reachable from
origin/main (e.g., git fetch && git branch -r --contains 19255c6e8 or git
ls-remote), then return to the parent repo and update the submodule pointer
(stage vendor/bonsplit, commit the pointer change) so the parent only pins a
commit that is published on the submodule remote.
---
Outside diff comments:
In `@cmuxTests/WindowAndDragTests.swift`:
- Around line 1212-1270: The test
testOverlayDoesNotCaptureFileDragLifecycleWhenPanePreviewDropsAreEnabled is
misleading because it never enables any "pane preview drops" mode; either enable
the feature toggle in setUp (or within the test) before creating
FileDropOverlayView, or rename the test to remove the
"WhenPanePreviewDropsAreEnabled" suffix so the name matches the verified
behavior; also update the sibling test's failure message that references "Finder
file drops should reach WKWebView" to reflect that Finder drops are expected to
be handled by pane-level resolution (adjust the XCTAssert message near
webViewUnderPoint / DragSpyWebView usage), and consider renaming DragSpyWebView
to DragAssertingWebView or replace its overrides with a direct
XCTAssert(webView.dragCalls.isEmpty, ...) to clarify that drag overrides are
only asserting absence.
In `@tests/test_bonsplit_tab_drag_overlay_gate.py`:
- Around line 181-219: Add a short clarifying comment above the file URL test
block explaining why assert_hit_chain_routes_to_pane is intentionally omitted
for fileurl payloads: file drags route via FileDropOverlayView's direct
pane-drop mechanism rather than workspace hit-chain routing, and pane delivery
is already validated by the existing assert_drop_gate checks; reference the test
assertions assert_drop_gate and assert_portal_gate and the omitted symbol
assert_hit_chain_routes_to_pane so future readers understand the intent.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 7488-7515: The FilePreviewPanel subscription isn't recreated when
a preview tab is reattached, so update
attachDetachedSurface(_:inPane:atIndex:focus:) to detect when the reattached
surface is a FilePreviewPanel and call installFilePreviewPanelSubscription(_:)
(or otherwise recreate the subscription and store it in
panelSubscriptions[filePreviewPanel.id]); ensure you only reinstall if
panelSubscriptions[filePreviewPanel.id] is nil (or cancel/replace an existing
subscription), and reference the panel's id, panelSubscriptions, and
bonsplitController.updateTab flow so title/isDirty changes resume propagating.
- Around line 9733-9771: The current splitPaneWithFilePreview creates a tab then
calls bonsplitController.selectTab(...) and filePreviewPanel.focus(), which
bypasses the workspace selection flow and leaves focusedPanelId, sidebar
metadata, and keyboard focus stale; replace those direct calls by routing
selection through the workspace selection handler — call
applyTabSelection(newTab.id) (the existing applyTabSelection(...) method)
instead of bonsplitController.selectTab(...) and remove the direct
filePreviewPanel.focus() call, and ensure
installFilePreviewPanelSubscription(filePreviewPanel) remains after
applyTabSelection so subscriptions are set up once the workspace selection state
(focusedPanelId, sidebar metadata, keyboard focus) is correctly applied.
---
Nitpick comments:
In `@Resources/Localizable.xcstrings`:
- Around line 86656-86915: The PDF and image zoom/localization keys
(filePreview.pdf.zoomIn/out/toFit/actualSize and
filePreview.image.zoomIn/out/toFit/actualSize) are duplicated; please
consolidate them by adding shared keys (e.g. filePreview.zoomIn,
filePreview.zoomOut, filePreview.zoomToFit, filePreview.actualSize) with the
same localization blocks, remove the duplicated pdf/image entries, and update
all code sites that reference the old keys to use the new shared keys (search
for uses of filePreview.pdf.* and filePreview.image.* to update). If you need to
avoid immediate runtime breakage, keep the old keys as thin aliases that
reference the same stringUnit/value while you migrate callers. Ensure string
identifiers exactly match the proposed shared names so translations remain
consistent.
In `@Sources/ContentView.swift`:
- Around line 2547-2549: Add an inline debug event log call inside the
onOpenFilePreview closure (before calling openFilePreviewFromSidebar(filePath:))
so the new sidebar file-preview action is emitted to the debug event log;
specifically, locate the onOpenFilePreview: { filePath in ... } closure in
ContentView and insert a debug-event invocation (using the project's debug event
logger API) that includes the event name (e.g., "sidebar.openFilePreview") and
the filePath as metadata, then call openFilePreviewFromSidebar(filePath:
filePath) as before.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40208c21-29d3-45d4-8f14-bfd83e4ff6ce
📒 Files selected for processing (8)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/Panels/FilePreviewPanel.swiftSources/TerminalWindowPortal.swiftSources/Workspace.swiftcmuxTests/WindowAndDragTests.swifttests/test_bonsplit_tab_drag_overlay_gate.pyvendor/bonsplit
✅ Files skipped from review due to trivial changes (1)
- Sources/TerminalWindowPortal.swift
| func loadTextContent() { | ||
| guard FileManager.default.fileExists(atPath: filePath) else { | ||
| isFileUnavailable = true | ||
| return | ||
| } | ||
|
|
||
| do { | ||
| let data = try Data(contentsOf: fileURL) | ||
| let decoded = Self.decodeText(data) | ||
| guard let decoded else { | ||
| isFileUnavailable = true | ||
| return | ||
| } | ||
| textContent = decoded | ||
| originalTextContent = decoded | ||
| isDirty = false | ||
| isFileUnavailable = false | ||
| } catch { | ||
| isFileUnavailable = true | ||
| } | ||
| } |
There was a problem hiding this comment.
loadTextContent leaves stale editor state when the file vanishes.
If a file is opened successfully, the user makes no edits, then the file is deleted and the user clicks Revert, we only flip isFileUnavailable = true. textContent, originalTextContent, and isDirty keep the previous buffer — so if the file re-appears later (e.g., via VCS), the panel's isDirty state is still derived from old content, and any subsequent updateTextContent comparison is against a stale originalTextContent.
♻️ Suggested guard
func loadTextContent() {
guard FileManager.default.fileExists(atPath: filePath) else {
+ textContent = ""
+ originalTextContent = ""
+ isDirty = false
isFileUnavailable = true
return
}Same applies to the catch branch if you keep the behavior symmetric.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` around lines 307 - 327,
loadTextContent currently only sets isFileUnavailable when the file is missing
or read fails, leaving textContent, originalTextContent and isDirty stale;
update loadTextContent so that in the guard branch
(FileManager.default.fileExists check) and in the catch branch you also reset
textContent and originalTextContent to an empty string (or nil if your types
allow) and set isDirty = false in addition to isFileUnavailable = true; locate
the function loadTextContent and the bindings textContent, originalTextContent,
isDirty, isFileUnavailable (and the decode path Self.decodeText /
Data(contentsOf: fileURL)) and apply the symmetric reset behavior in both the
missing-file and error branches.
| func setURL(_ url: URL) { | ||
| guard currentURL != url else { return } | ||
| currentURL = url | ||
| pdfView.document = PDFDocument(url: url) | ||
| pdfView.autoScales = true | ||
| updatePageControls() | ||
| } |
There was a problem hiding this comment.
Synchronous PDF/image loading on the main thread — large assets will freeze the UI.
Both FilePreviewPDFContainerView.setURL(_:) (line 616) and FilePreviewImageContainerView.setURL(_:) (line 830) materialize the full asset on the main thread:
PDFDocument(url: url)will block until the entire PDF is parsed (easy to hit with multi‑MB PDFs dragged in from a repo).NSImage(contentsOf: url)is lazy for some formats but eagerly decodes others (e.g., large TIFF/PNG/JPEG captured via asset catalog) during first draw on the main thread.
For previews opened from sidebar drag-and-drop, this lands on the same main actor that's servicing drag events, so a large file will stall pane routing, animations, and key input for seconds.
Consider loading off-main and assigning back on main:
♻️ Proposed fix (PDF path)
func setURL(_ url: URL) {
guard currentURL != url else { return }
currentURL = url
- pdfView.document = PDFDocument(url: url)
- pdfView.autoScales = true
- updatePageControls()
+ pdfView.document = nil
+ pdfView.autoScales = true
+ updatePageControls()
+ let loadURL = url
+ DispatchQueue.global(qos: .userInitiated).async { [weak self] in
+ let document = PDFDocument(url: loadURL)
+ DispatchQueue.main.async {
+ guard let self, self.currentURL == loadURL else { return }
+ self.pdfView.document = document
+ self.pdfView.autoScales = true
+ self.updatePageControls()
+ }
+ }
}Apply the same pattern for NSImage(contentsOf:) in FilePreviewImageContainerView.setURL(_:), guarding the assignment with a currentURL == loadURL check so a rapid re-drop doesn't clobber a newer load.
Also applies to: 827-836
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` around lines 613 - 619,
FilePreviewPDFContainerView.setURL(_:) is creating PDFDocument(url:) on the main
thread; change it to capture the incoming URL into a local loadURL, perform
PDFDocument(url:) creation on a background queue, then dispatch back to the main
queue to assign pdfView.document and call pdfView.autoScales/updatePageControls
only if currentURL == loadURL; apply the same pattern in
FilePreviewImageContainerView.setURL(_:) for NSImage(contentsOf:) (create the
NSImage off‑main, then assign on main guarded by the same currentURL == loadURL
check to avoid clobbering newer loads).
| let format = String(localized: "filePreview.pdf.pageCount", defaultValue: "Page %d of %d") | ||
| pageLabel.stringValue = String(format: format, pageIndex + 1, document.pageCount) |
There was a problem hiding this comment.
Use String.localizedStringWithFormat so non-Latin digit systems format correctly.
String(format:) with a printf-style format treats %d locale‑agnostically and doesn't support positional argument reordering that translators may need (e.g., %2$d / %1$d). For a UI label shown to end users in all supported locales, prefer String.localizedStringWithFormat or the modern interpolated defaultValue:
♻️ Proposed fix
- let format = String(localized: "filePreview.pdf.pageCount", defaultValue: "Page %d of %d")
- pageLabel.stringValue = String(format: format, pageIndex + 1, document.pageCount)
+ let format = String(localized: "filePreview.pdf.pageCount", defaultValue: "Page %d of %d")
+ pageLabel.stringValue = String.localizedStringWithFormat(
+ format, pageIndex + 1, document.pageCount
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` around lines 780 - 781, The localized
page label currently uses String(format:) which is not locale-aware for digits
or argument reordering; change the construction so the localized format is
produced with String.localizedStringWithFormat (or use the localized
interpolated defaultValue) and assign that to pageLabel.stringValue; update the
use sites around the format constant named format and the assignment to
pageLabel.stringValue to call String.localizedStringWithFormat(format, pageIndex
+ 1, document.pageCount) (or replace the localized string with an interpolated
localized value) so numbers and positional arguments are formatted correctly in
non‑Latin locales.
| @discardableResult | ||
| func newFilePreviewSurface( | ||
| inPane paneId: PaneID, | ||
| filePath: String, | ||
| focus: Bool? = nil, | ||
| targetIndex: Int? = nil | ||
| ) -> FilePreviewPanel? { | ||
| let shouldFocusNewTab = focus ?? (bonsplitController.focusedPaneId == paneId) | ||
| let previousFocusedPanelId = focusedPanelId | ||
| let previousHostedView = focusedTerminalPanel?.hostedView | ||
|
|
||
| let filePreviewPanel = FilePreviewPanel(workspaceId: id, filePath: filePath) | ||
| panels[filePreviewPanel.id] = filePreviewPanel | ||
| panelTitles[filePreviewPanel.id] = filePreviewPanel.displayTitle | ||
|
|
||
| guard let newTabId = bonsplitController.createTab( | ||
| title: filePreviewPanel.displayTitle, | ||
| icon: filePreviewPanel.displayIcon, | ||
| kind: SurfaceKind.filePreview, | ||
| isDirty: filePreviewPanel.isDirty, | ||
| isLoading: false, | ||
| isPinned: false, | ||
| inPane: paneId | ||
| ) else { | ||
| panels.removeValue(forKey: filePreviewPanel.id) | ||
| panelTitles.removeValue(forKey: filePreviewPanel.id) | ||
| return nil | ||
| } | ||
|
|
||
| surfaceIdToPanelId[newTabId] = filePreviewPanel.id | ||
| if let targetIndex { | ||
| _ = bonsplitController.reorderTab(newTabId, toIndex: targetIndex) | ||
| } | ||
| if shouldFocusNewTab { | ||
| bonsplitController.focusPane(paneId) | ||
| bonsplitController.selectTab(newTabId) | ||
| filePreviewPanel.focus() | ||
| applyTabSelection(tabId: newTabId, inPane: paneId) | ||
| } else { | ||
| preserveFocusAfterNonFocusSplit( | ||
| preferredPanelId: previousFocusedPanelId, | ||
| splitPanelId: filePreviewPanel.id, | ||
| previousHostedView: previousHostedView | ||
| ) | ||
| } | ||
|
|
||
| installFilePreviewPanelSubscription(filePreviewPanel) | ||
| return filePreviewPanel |
There was a problem hiding this comment.
Dirty file previews can still close without confirmation.
These surfaces are editable and can become dirty, but splitTabBar(_:shouldCloseTab:inPane:) only routes TerminalPanel through the close-confirmation path. A dirty FilePreviewPanel can currently be closed immediately, which risks discarding unsaved edits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 9683 - 9730, The close-confirmation
logic currently only checks for TerminalPanel in
splitTabBar(_:shouldCloseTab:inPane:), so dirty FilePreviewPanel instances can
be closed without prompt; update splitTabBar(_:shouldCloseTab:inPane:) to look
up the panel via surfaceIdToPanelId/newTabId (or panels[panelId]) and treat
FilePreviewPanel the same as TerminalPanel by checking its isDirty flag
(FilePreviewPanel.isDirty) and routing through the close-confirmation path when
dirty; ensure you reference FilePreviewPanel and TerminalPanel types and use the
existing panels dictionary/surfaceIdToPanelId mapping to determine the panel
before deciding to show confirmation.
| @@ -1 +1 @@ | |||
| Subproject commit fc54dcc3735741279274169f8c30a7897e5d55d3 | |||
| Subproject commit 19255c6e85224f32bb1363914478113063427cf3 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Ensure the submodule is initialized and fetched
git submodule update --init vendor/bonsplit
cd vendor/bonsplit
git fetch origin main --quiet
PINNED=$(git -C "$OLDPWD" ls-tree HEAD vendor/bonsplit | awk '{print $3}')
echo "Pinned commit: $PINNED"
# Show commit metadata
git log -1 --format='%H%n%an <%ae>%n%ad%n%s' "$PINNED" || {
echo "Pinned commit not found locally in vendor/bonsplit"; exit 1; }
# Ancestry check against origin/main
if git merge-base --is-ancestor "$PINNED" origin/main; then
echo "OK: pinned commit is an ancestor of origin/main"
else
echo "FAIL: pinned commit is NOT on origin/main — push it before bumping the pointer"
exit 1
fiRepository: manaflow-ai/cmux
Length of output: 684
Push the submodule commit to origin/main before bumping the pointer in the parent repo.
Verification confirms that commit 19255c6e85224f32bb1363914478113063427cf3 (Support external file drops) is not yet an ancestor of vendor/bonsplit's origin/main. Per project policy, the submodule commit must be published to its remote main branch before the parent repo pins it. Push the commit to the submodule's remote and then update the parent repo pointer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/bonsplit` at line 1, The parent repo was updated to point
vendor/bonsplit at commit 19255c6e85224f32bb1363914478113063427cf3 but that
commit is not yet on the submodule remote; push that commit to the submodule's
origin main branch first (checkout the submodule, ensure the commit exists
locally, git push origin <branch-or-commit-ref>:main), verify the commit is now
reachable from origin/main (e.g., git fetch && git branch -r --contains
19255c6e8 or git ls-remote), then return to the parent repo and update the
submodule pointer (stage vendor/bonsplit, commit the pointer change) so the
parent only pins a commit that is published on the submodule remote.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/FilePreviewPanel.swift`:
- Line 1016: Replace the hard-coded percent label assignment that uses bare
interpolation with a locale-aware, localized string: format the percentage using
NumberFormatter (or NumberFormatter.localizedString(from: number, number:
.percent)) using the `scale` value (e.g., `scale` -> percent number), then put
that formatted value into a localized string via String(localized: "...",
defaultValue: "...") before assigning to `zoomLabel.stringValue`; update the
assignment in FilePreviewPanel (the code referencing `zoomLabel` and `scale`) so
the percent is rendered with locale-aware digits and percent placement and the
overall UI string goes through String(localized:).
- Around line 1184-1218: QuickLookPreviewView never calls QLPreviewView.close(),
so implement dismantleNSView(in:context:) in QuickLookPreviewView to cast nsView
to QLPreviewView and call previewView.close() to release QuickLook resources
when the SwiftUI view is torn down (mirror makeNSView/previewView creation and
use the same context.coordinator.item(for:...)). Also change the zoom label
assignment to use localization instead of a raw string: replace the raw
"\(Int((scale * 100).rounded()))%" with String(localized:
"filePreview.image.zoomPercentage", defaultValue: "\(Int((scale *
100).rounded()))%") for zoomLabel (add the "filePreview.image.zoomPercentage"
key to Resources/Localizable.xcstrings with EN/JP translations).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07f0e28e-99e3-4cff-b70f-de8e8b434b8d
📒 Files selected for processing (1)
Sources/Panels/FilePreviewPanel.swift
| ) | ||
| documentView.scaledImageSize = scaledSize | ||
| documentView.needsLayout = true | ||
| zoomLabel.stringValue = "\(Int((scale * 100).rounded()))%" |
There was a problem hiding this comment.
Unlocalized / non‑locale‑aware percentage label.
zoomLabel.stringValue = "\(Int((scale * 100).rounded()))%" is a user-facing string built with bare interpolation and a hard-coded %. This violates the localization guideline (every UI string must go through String(localized:) with a catalog key) and also uses non-locale-aware digits/percent formatting. Prefer NumberFormatter.localizedString(from:number:style:) with .percent, which handles digit scripts and the percent symbol position per locale.
♻️ Proposed fix
- zoomLabel.stringValue = "\(Int((scale * 100).rounded()))%"
+ let formatter = NumberFormatter()
+ formatter.numberStyle = .percent
+ formatter.maximumFractionDigits = 0
+ zoomLabel.stringValue = formatter.string(from: NSNumber(value: Double(scale))) ?? ""As per coding guidelines: "All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text") ... Never use bare string literals in SwiftUI Text(), Button(), alert titles, etc." The same rule applies to AppKit labels shown in the UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` at line 1016, Replace the hard-coded
percent label assignment that uses bare interpolation with a locale-aware,
localized string: format the percentage using NumberFormatter (or
NumberFormatter.localizedString(from: number, number: .percent)) using the
`scale` value (e.g., `scale` -> percent number), then put that formatted value
into a localized string via String(localized: "...", defaultValue: "...") before
assigning to `zoomLabel.stringValue`; update the assignment in FilePreviewPanel
(the code referencing `zoomLabel` and `scale`) so the percent is rendered with
locale-aware digits and percent placement and the overall UI string goes through
String(localized:).
| private struct QuickLookPreviewView: NSViewRepresentable { | ||
| let url: URL | ||
| let title: String | ||
|
|
||
| func makeNSView(context: Context) -> NSView { | ||
| guard let previewView = QLPreviewView(frame: .zero, style: .normal) else { | ||
| return NSView() | ||
| } | ||
| previewView.autostarts = true | ||
| previewView.previewItem = context.coordinator.item(for: url, title: title) | ||
| return previewView | ||
| } | ||
|
|
||
| func updateNSView(_ nsView: NSView, context: Context) { | ||
| guard let previewView = nsView as? QLPreviewView else { return } | ||
| previewView.previewItem = context.coordinator.item(for: url, title: title) | ||
| } | ||
|
|
||
| func makeCoordinator() -> Coordinator { | ||
| Coordinator() | ||
| } | ||
|
|
||
| final class Coordinator { | ||
| private var item: FilePreviewQLItem? | ||
|
|
||
| func item(for url: URL, title: String) -> FilePreviewQLItem { | ||
| if let item, item.url == url, item.title == title { | ||
| return item | ||
| } | ||
| let next = FilePreviewQLItem(url: url, title: title) | ||
| item = next | ||
| return next | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its basic structure
wc -l Sources/Panels/FilePreviewPanel.swiftRepository: manaflow-ai/cmux
Length of output: 103
🏁 Script executed:
# Read the QuickLookPreviewView section (lines 1184-1218)
sed -n '1184,1218p' Sources/Panels/FilePreviewPanel.swiftRepository: manaflow-ai/cmux
Length of output: 1164
🏁 Script executed:
# Read the zoom label section (around line 1016)
sed -n '1010,1025p' Sources/Panels/FilePreviewPanel.swiftRepository: manaflow-ai/cmux
Length of output: 749
🏁 Script executed:
# Search for dismantleNSView or close() calls related to QLPreviewView in the file
rg -n "dismantleNSView|close\(\)" Sources/Panels/FilePreviewPanel.swiftRepository: manaflow-ai/cmux
Length of output: 125
🏁 Script executed:
# Search for any QLPreviewView usage patterns in the codebase
rg -n "QLPreviewView" Sources/Repository: manaflow-ai/cmux
Length of output: 302
🏁 Script executed:
# Search for other NSViewRepresentable implementations in the codebase to see if they use dismantleNSView
rg -A 20 "NSViewRepresentable" Sources/ | grep -B 3 -A 10 "dismantleNSView"Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Check if there are any other QLPreviewView usages in the codebase
rg -B 5 -A 15 "QLPreviewView" Sources/Repository: manaflow-ai/cmux
Length of output: 2238
🏁 Script executed:
# Look for FilePreviewQLItem to understand the preview item lifecycle
rg -B 2 -A 10 "class FilePreviewQLItem\|struct FilePreviewQLItem" Sources/Panels/FilePreviewPanel.swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Check if there's a localized version nearby to see the pattern for localization in this file
rg -n "String\(localized:" Sources/Panels/FilePreviewPanel.swift | head -20Repository: manaflow-ai/cmux
Length of output: 1783
QLPreviewView is never close()-d — potential resource leak on teardown.
QuickLookPreviewView creates a QLPreviewView in makeNSView but lacks a dismantleNSView implementation to call previewView.close(). Per Apple's documentation, QLPreviewView holds onto preview contents and QuickLook resources until either the window closes (if shouldCloseWithWindow = true) or close() is explicitly called. Since the preview panel can be torn down while the window remains open (surface replacement, pane movement, or panel closure), previously-previewed items (large PDFs, movies, archives) remain resident without explicit cleanup.
Implement dismantleNSView to release the preview view:
♻️ Proposed fix
private struct QuickLookPreviewView: NSViewRepresentable {
let url: URL
let title: String
func makeNSView(context: Context) -> NSView {
guard let previewView = QLPreviewView(frame: .zero, style: .normal) else {
return NSView()
}
previewView.autostarts = true
+ context.coordinator.previewView = previewView
previewView.previewItem = context.coordinator.item(for: url, title: title)
return previewView
}
func updateNSView(_ nsView: NSView, context: Context) {
guard let previewView = nsView as? QLPreviewView else { return }
previewView.previewItem = context.coordinator.item(for: url, title: title)
}
+ static func dismantleNSView(_ nsView: NSView, coordinator: Coordinator) {
+ coordinator.previewView?.close()
+ coordinator.previewView = nil
+ }
+
func makeCoordinator() -> Coordinator {
Coordinator()
}
final class Coordinator {
private var item: FilePreviewQLItem?
+ var previewView: QLPreviewView?
func item(for url: URL, title: String) -> FilePreviewQLItem {
if let item, item.url == url, item.title == title {
return item
}
let next = FilePreviewQLItem(url: url, title: title)
item = next
return next
}
}
}Line 1016: Zoom percentage label must be localized.
The zoom label at line 1016 uses a bare string "\(Int((scale * 100).rounded()))%" without localization. Follow the file's established pattern for localized strings:
zoomLabel.stringValue = String(localized: "filePreview.image.zoomPercentage", defaultValue: "\(Int((scale * 100).rounded()))%")Add the key to Resources/Localizable.xcstrings with translations for English and Japanese.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` around lines 1184 - 1218,
QuickLookPreviewView never calls QLPreviewView.close(), so implement
dismantleNSView(in:context:) in QuickLookPreviewView to cast nsView to
QLPreviewView and call previewView.close() to release QuickLook resources when
the SwiftUI view is torn down (mirror makeNSView/previewView creation and use
the same context.coordinator.item(for:...)). Also change the zoom label
assignment to use localization instead of a raw string: replace the raw
"\(Int((scale * 100).rounded()))%" with String(localized:
"filePreview.image.zoomPercentage", defaultValue: "\(Int((scale *
100).rounded()))%") for zoomLabel (add the "filePreview.image.zoomPercentage"
key to Resources/Localizable.xcstrings with EN/JP translations).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90c2e611ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| do { | ||
| try textContent.write(to: fileURL, atomically: true, encoding: .utf8) |
There was a problem hiding this comment.
FilePreviewPanel is @MainActor, and saveTextContent() performs a synchronous disk write (String.write) on that actor. When users save larger files or files on slow/network-backed volumes, this blocks the UI thread and can freeze interaction until I/O completes. The write path should run off-main and only post the resulting state updates (isDirty, isFileUnavailable) back to the main actor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/Panels/FilePreviewPanel.swift (1)
163-194: Extract the repeated UTType→mode mapping.
mode(for:)applies the same.image/.movie|.audiovisualContent|.audioconformance ladder twice — once for the resolvedcontentTypeand once for the extension-derivedUTType(filenameExtension:). A small helper removes the duplication and keeps the two paths in lockstep if new modes are added later.♻️ Proposed refactor
static func mode(for url: URL) -> FilePreviewMode { if isTextFile(url: url) { return .text } - if let type = try? url.resourceValues(forKeys: [.contentTypeKey]).contentType { - if type.conforms(to: .pdf) { - return .pdf - } - if type.conforms(to: .image) { - return .image - } - if type.conforms(to: .movie) - || type.conforms(to: .audiovisualContent) - || type.conforms(to: .audio) { - return .media - } - } - if let fallbackType = UTType(filenameExtension: url.pathExtension.lowercased()) { - if fallbackType.conforms(to: .pdf) { - return .pdf - } - if fallbackType.conforms(to: .image) { - return .image - } - if fallbackType.conforms(to: .movie) - || fallbackType.conforms(to: .audiovisualContent) - || fallbackType.conforms(to: .audio) { - return .media - } - } + let resolved = (try? url.resourceValues(forKeys: [.contentTypeKey]).contentType) + ?? UTType(filenameExtension: url.pathExtension.lowercased()) + if let type = resolved { + if type.conforms(to: .pdf) { return .pdf } + if type.conforms(to: .image) { return .image } + if type.conforms(to: .movie) + || type.conforms(to: .audiovisualContent) + || type.conforms(to: .audio) { + return .media + } + } return .quickLook }Note: this slightly changes semantics — the current code falls through from a resolved-but-non-matching
contentTypeto the extension-based lookup. If that fallthrough is load-bearing (e.g., misdetected resource values on some volumes), keep the two-phase structure but factor the inner ladder into amediaMode(for:UTType) -> FilePreviewMode?helper instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/FilePreviewPanel.swift` around lines 163 - 194, The mode(for:) function duplicates the UTType-to-FilePreviewMode mapping; extract that mapping into a small helper (e.g., mediaMode(for:) -> FilePreviewMode?) and call it from both branches: after obtaining contentType from url.resourceValues and again for the fallback UTType(filenameExtension:), returning the helper's value if non-nil; keep the outer two-phase structure (still call isTextFile(url:) first and fall back to quickLook) so semantics remain identical while removing the repeated .pdf/.image/.movie|.audiovisualContent|.audio conformance checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 32-57: FilePreviewDragRegistry is leaking entries because
register(_:) never expires or removes entries if drags cancel; update the
registry so each pending entry is stored with a registration timestamp (wrap
FilePreviewDragEntry in a struct with Date) and implement a small GC that
removes entries older than a chosen TTL (e.g. N seconds) whenever register(_:)
or consume(id:) is called, and also provide an explicit cancel API (e.g.
discard(id: UUID)) that the drag source can call from
draggingSession(_:endedAt:operation:) when the operation is .cancel; ensure you
update pending access in register(_:), consume(id:), discardAll(), and any
callers to remove by id so stale entries are reclaimed.
- Around line 303-305: The close() method currently only nils the weak textView
and thus silently discards any unsaved edits; update close() behavior so it
checks the text preview's dirty state (textView?.isDirty or the panel's isDirty
helper) and handles it: either perform an auto-save write-through when dirty, or
notify/return a signal allowing Workspace to present the standard
unsaved-changes confirmation before teardown, or persist the buffer to a
recoverable cache file (e.g., in ~/Library/Caches) for recovery; reference the
close(), textView, isDirty, and Workspace integration points to implement one of
these flows and ensure no dirty content is dropped on deinit/close.
---
Nitpick comments:
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 163-194: The mode(for:) function duplicates the
UTType-to-FilePreviewMode mapping; extract that mapping into a small helper
(e.g., mediaMode(for:) -> FilePreviewMode?) and call it from both branches:
after obtaining contentType from url.resourceValues and again for the fallback
UTType(filenameExtension:), returning the helper's value if non-nil; keep the
outer two-phase structure (still call isTextFile(url:) first and fall back to
quickLook) so semantics remain identical while removing the repeated
.pdf/.image/.movie|.audiovisualContent|.audio conformance checks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: edec5b5f-813f-4532-8c3d-97dd39b35a75
📒 Files selected for processing (2)
Resources/Localizable.xcstringsSources/Panels/FilePreviewPanel.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Resources/Localizable.xcstrings
| final class FilePreviewDragRegistry { | ||
| static let shared = FilePreviewDragRegistry() | ||
|
|
||
| private let lock = NSLock() | ||
| private var pending: [UUID: FilePreviewDragEntry] = [:] | ||
|
|
||
| func register(_ entry: FilePreviewDragEntry) -> UUID { | ||
| let id = UUID() | ||
| lock.lock() | ||
| pending[id] = entry | ||
| lock.unlock() | ||
| return id | ||
| } | ||
|
|
||
| func consume(id: UUID) -> FilePreviewDragEntry? { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return pending.removeValue(forKey: id) | ||
| } | ||
|
|
||
| func discardAll() { | ||
| lock.lock() | ||
| pending.removeAll() | ||
| lock.unlock() | ||
| } | ||
| } |
There was a problem hiding this comment.
FilePreviewDragRegistry entries leak on cancelled drags.
register(_:) inserts into pending with no TTL or cleanup hook. Entries are only removed via consume(id:) (called when Bonsplit accepts a valid drop) or the never-invoked discardAll(). Given the already-flagged speculative writer instantiation during NSOutlineView's probing of pasteboardWriterForItem:, plus any real drag that's cancelled (Esc, dropped on a non-Bonsplit target, dropped back on the sidebar), the dict grows unbounded for the lifetime of the process.
At minimum, tag each entry with a registration timestamp and sweep anything older than N seconds on register/consume, or expose a cancellation hook the drag source can call when draggingSession(_:endedAt:operation:) fires with .cancel.
♻️ Sketch: simple timestamp-based sweep
final class FilePreviewDragRegistry {
static let shared = FilePreviewDragRegistry()
private let lock = NSLock()
- private var pending: [UUID: FilePreviewDragEntry] = [:]
+ private var pending: [UUID: (entry: FilePreviewDragEntry, registeredAt: Date)] = [:]
+ private static let entryTTL: TimeInterval = 60
func register(_ entry: FilePreviewDragEntry) -> UUID {
let id = UUID()
lock.lock()
- pending[id] = entry
+ sweepLocked()
+ pending[id] = (entry, Date())
lock.unlock()
return id
}
func consume(id: UUID) -> FilePreviewDragEntry? {
lock.lock()
defer { lock.unlock() }
- return pending.removeValue(forKey: id)
+ sweepLocked()
+ return pending.removeValue(forKey: id)?.entry
}
func discardAll() {
lock.lock()
pending.removeAll()
lock.unlock()
}
+
+ private func sweepLocked() {
+ let cutoff = Date().addingTimeInterval(-Self.entryTTL)
+ pending = pending.filter { $0.value.registeredAt >= cutoff }
+ }
}📝 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.
| final class FilePreviewDragRegistry { | |
| static let shared = FilePreviewDragRegistry() | |
| private let lock = NSLock() | |
| private var pending: [UUID: FilePreviewDragEntry] = [:] | |
| func register(_ entry: FilePreviewDragEntry) -> UUID { | |
| let id = UUID() | |
| lock.lock() | |
| pending[id] = entry | |
| lock.unlock() | |
| return id | |
| } | |
| func consume(id: UUID) -> FilePreviewDragEntry? { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| return pending.removeValue(forKey: id) | |
| } | |
| func discardAll() { | |
| lock.lock() | |
| pending.removeAll() | |
| lock.unlock() | |
| } | |
| } | |
| final class FilePreviewDragRegistry { | |
| static let shared = FilePreviewDragRegistry() | |
| private let lock = NSLock() | |
| private var pending: [UUID: (entry: FilePreviewDragEntry, registeredAt: Date)] = [:] | |
| private static let entryTTL: TimeInterval = 60 | |
| func register(_ entry: FilePreviewDragEntry) -> UUID { | |
| let id = UUID() | |
| lock.lock() | |
| sweepLocked() | |
| pending[id] = (entry, Date()) | |
| lock.unlock() | |
| return id | |
| } | |
| func consume(id: UUID) -> FilePreviewDragEntry? { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| sweepLocked() | |
| return pending.removeValue(forKey: id)?.entry | |
| } | |
| func discardAll() { | |
| lock.lock() | |
| pending.removeAll() | |
| lock.unlock() | |
| } | |
| private func sweepLocked() { | |
| let cutoff = Date().addingTimeInterval(-Self.entryTTL) | |
| pending = pending.filter { $0.value.registeredAt >= cutoff } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` around lines 32 - 57,
FilePreviewDragRegistry is leaking entries because register(_:) never expires or
removes entries if drags cancel; update the registry so each pending entry is
stored with a registration timestamp (wrap FilePreviewDragEntry in a struct with
Date) and implement a small GC that removes entries older than a chosen TTL
(e.g. N seconds) whenever register(_:) or consume(id:) is called, and also
provide an explicit cancel API (e.g. discard(id: UUID)) that the drag source can
call from draggingSession(_:endedAt:operation:) when the operation is .cancel;
ensure you update pending access in register(_:), consume(id:), discardAll(),
and any callers to remove by id so stale entries are reclaimed.
| func close() { | ||
| textView = nil | ||
| } |
There was a problem hiding this comment.
close() silently discards unsaved edits in text previews.
Text preview mode is editable (textView.isEditable = true, Cmd+S save), so isDirty can be true when the panel is torn down via tab close / pane collapse / workspace teardown. close() only nils the weak textView reference — it doesn't auto-save, prompt, or surface the unsaved state. Any pending edits are lost with no user notification.
Consider at least one of:
- Auto-save on close when
isDirty(write-through behavior). - Let
WorkspacequeryisDirtybefore callingclose()and present a standard "unsaved changes" confirmation, same as any document-backed editor. - Route the dirty content to a backup file (
~/Library/Caches/...) on close so a user can recover after accidental teardown.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/FilePreviewPanel.swift` around lines 303 - 305, The close()
method currently only nils the weak textView and thus silently discards any
unsaved edits; update close() behavior so it checks the text preview's dirty
state (textView?.isDirty or the panel's isDirty helper) and handles it: either
perform an auto-save write-through when dirty, or notify/return a signal
allowing Workspace to present the standard unsaved-changes confirmation before
teardown, or persist the buffer to a recoverable cache file (e.g., in
~/Library/Caches) for recovery; reference the close(), textView, isDirty, and
Workspace integration points to implement one of these flows and ensure no dirty
content is dropped on deinit/close.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
You’re at about 93% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/FilePreviewPanel.swift">
<violation number="1" location="Sources/Panels/FilePreviewPanel.swift:655">
P2: Reset the PDF rotation gesture accumulator when loading a new URL; otherwise partial rotation from the previous document can trigger an unintended rotation in the next one.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a58873304e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static func mode(for url: URL) -> FilePreviewMode { | ||
| if isTextFile(url: url) { | ||
| return .text |
There was a problem hiding this comment.
FilePreviewKindResolver.mode(for:) checks isTextFile before it checks UTType conformance for .pdf, .image, and media. Because isTextFile falls back to sniffLooksLikeText, binary files with ASCII-heavy headers and no NUL bytes in the first 4KB (a common PDF shape) get classified as .text first, so they open in the text editor instead of the PDF/image/media preview path. Reordering detection to evaluate known non-text UTTypes first (or bypassing sniffing for known binary UTTypes) avoids this misclassification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7104964416
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func updateTextContent(_ nextContent: String) { | ||
| guard textContent != nextContent else { return } | ||
| textContent = nextContent | ||
| isDirty = nextContent != originalTextContent |
There was a problem hiding this comment.
This panel now tracks unsaved edits via isDirty, but the close-confirmation path in Workspace.splitTabBar(_:didAllowCloseTab:fromPane:) only checks TerminalPanel and returns early for every non-terminal panel. In practice, editing a text preview and then closing the tab (or closing a workspace tab containing it) discards unsaved changes without any warning, which is a data-loss regression for the new editable preview flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
You’re at about 93% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/FilePreviewPanel.swift">
<violation number="1" location="Sources/Panels/FilePreviewPanel.swift:954">
P3: Fix the auto layout conflict by adjusting the container width from 58 to 57 to match the sum of its internal constrained elements.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| NSLayoutConstraint.activate([ | ||
| leftFloatingChrome.topAnchor.constraint(equalTo: contentHost.topAnchor, constant: 10), | ||
| leftFloatingChrome.leadingAnchor.constraint(equalTo: contentHost.leadingAnchor, constant: 10), | ||
| leftFloatingChrome.widthAnchor.constraint(equalToConstant: 58), |
There was a problem hiding this comment.
P3: Fix the auto layout conflict by adjusting the container width from 58 to 57 to match the sum of its internal constrained elements.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/FilePreviewPanel.swift, line 954:
<comment>Fix the auto layout conflict by adjusting the container width from 58 to 57 to match the sum of its internal constrained elements.</comment>
<file context>
@@ -910,22 +949,31 @@ private final class FilePreviewPDFContainerView: NSView, NSOutlineViewDataSource
- zoomControl.bottomAnchor.constraint(equalTo: rightFloatingChrome.bottomAnchor, constant: -4),
+ leftFloatingChrome.topAnchor.constraint(equalTo: contentHost.topAnchor, constant: 10),
+ leftFloatingChrome.leadingAnchor.constraint(equalTo: contentHost.leadingAnchor, constant: 10),
+ leftFloatingChrome.widthAnchor.constraint(equalToConstant: 58),
+ leftFloatingChrome.heightAnchor.constraint(equalToConstant: Metrics.floatingChromeHeight),
+ sidebarIconView.leadingAnchor.constraint(equalTo: leftFloatingChrome.leadingAnchor, constant: 12),
</file context>
| leftFloatingChrome.widthAnchor.constraint(equalToConstant: 58), | |
| leftFloatingChrome.widthAnchor.constraint(equalToConstant: 57), |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80961a2591
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var displayIcon: String? { | ||
| FilePreviewKindResolver.tabIconName(for: fileURL) |
There was a problem hiding this comment.
displayIcon is a computed property that calls FilePreviewKindResolver.tabIconName(for:), which can fall through to sniffLooksLikeText and synchronously read file bytes. Because FilePreviewPanelView observes panel, text edits publish on every keystroke, re-evaluate the header, and therefore re-run this icon computation on the main thread. For extensionless/unknown-type files (for example README), this puts disk I/O in the typing path and can cause visible input lag on slower or network-backed filesystems. Cache the icon/classification at initialization (or derive it from previewMode) instead of recomputing from disk during view updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 551d41d1ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| let fitButton = makeToolbarButton( | ||
| systemSymbolName: "arrow.up.left.and.arrow.down.right", | ||
| fallbackTitle: "Fit", |
There was a problem hiding this comment.
This adds a hardcoded user-facing fallback label ("Fit") for the image toolbar button path when SF Symbols are unavailable, which violates the localization rule in /workspace/cmux/AGENTS.md (“All user-facing strings must be localized”). The same pattern appears for nearby fallback titles ("1x", "L", "R"), so users can see untranslated English strings in non-English locales on fallback rendering paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8037791251
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case terminal | ||
| case browser | ||
| case markdown | ||
| case filePreview |
There was a problem hiding this comment.
This new enum case uses filePreview (camel-case), but V2 command parsing still does PanelType(rawValue: s.lowercased()) in TerminalController.v2PanelType, so a round-tripped type like "filePreview" can never parse back to .filePreview and callers silently fall back to other behavior (usually terminal/default paths). That breaks CLI/API automation that reads surface_type and then reuses it in follow-up commands.
Useful? React with 👍 / 👎.
| guard previewMode == .text else { return } | ||
| do { | ||
| let currentContent = textView?.string ?? textContent | ||
| textContent = currentContent | ||
| try currentContent.write(to: fileURL, atomically: true, encoding: textEncoding) |
There was a problem hiding this comment.
saveTextContent() writes to disk whenever preview mode is text, even when nothing changed, and SavingTextView.performKeyEquivalent invokes it on every Cmd+S regardless of dirty state. That means users can trigger unnecessary writes (mtime churn) and, on read-only/protected files, hit the save error path and flip the panel into the unavailable state without any edits. Add an isDirty/content-change guard before writing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b675f4b67b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if shouldFocusNewTab { | ||
| bonsplitController.focusPane(paneId) | ||
| bonsplitController.selectTab(newTabId) | ||
| filePreviewPanel.focus() |
There was a problem hiding this comment.
newFilePreviewSurface requests focus immediately, but FilePreviewPanel.focus() only works when textView is already attached. For first-open text previews (e.g., sidebar double-click or split drop), the focus request can run before FilePreviewTextEditor.makeNSView calls attachTextView, so the editor never becomes first responder and initial typing stays on the previous responder until a manual click. Persist the focus intent and apply it when the NSTextView is attached.
Useful? React with 👍 / 👎.
| func writableTypes(for pasteboard: NSPasteboard) -> [NSPasteboard.PasteboardType] { | ||
| [Self.bonsplitTransferType] |
There was a problem hiding this comment.
This writer advertises only com.splittabbar.tabtransfer, so file drags are treated as ordinary Bonsplit tab drags by other drop handlers. In particular, sidebar workspace drop paths accept the payload and invoke moveBonsplitTab with the synthetic drag UUID, which fails with tabNotFound, producing a drop target that appears valid but does nothing. Emit a dedicated file-preview drag type (or discriminator) so workspace tab-drop handlers can reject or reroute these drags.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5775d5f183
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| override func performKeyEquivalent(with event: NSEvent) -> Bool { | ||
| if event.modifierFlags.intersection(.deviceIndependentFlagsMask) == .command, | ||
| event.charactersIgnoringModifiers?.lowercased() == "s" { | ||
| panel?.saveTextContent() |
There was a problem hiding this comment.
The new Cmd+S handling is hardcoded in the panel (performKeyEquivalent) instead of going through the shared shortcut system, which violates the shortcut policy in /workspace/cmux/AGENTS.md (“Every new cmux-owned keyboard shortcut must be added to KeyboardShortcutSettings, configurable via settings.json, and documented”). In practice, users who remap save-related shortcuts won’t be able to trigger file-preview save consistently, and config/docs will drift from runtime behavior.
Useful? React with 👍 / 👎.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1fc14288a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return hasBonsplitTabTransfer(pasteboardTypes) | ||
| || hasSidebarTabReorder(pasteboardTypes) | ||
| || hasFileURL(pasteboardTypes) |
There was a problem hiding this comment.
Including hasFileURL in portal pass-through forces portal-hosted browser panes to yield drag hit-testing to Bonsplit drop targets for every Finder file drag, so drops over a web page no longer reach WKWebView upload handlers. In this commit, file drags that bypass the web view are handled by workspace-level external file-drop routing (which opens file-preview tabs/splits), so common flows like dragging a file into a browser upload area regress into pane-drop behavior instead of page-level upload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04817d9038
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "bash", "c", "cc", "cfg", "conf", "cpp", "cs", "css", "csv", "env", | ||
| "fish", "go", "h", "hpp", "htm", "html", "ini", "java", "js", "json", | ||
| "jsx", "kt", "log", "m", "markdown", "md", "mdx", "mm", "plist", "py", | ||
| "rb", "rs", "sh", "sql", "swift", "toml", "ts", "tsx", "tsv", "txt", |
There was a problem hiding this comment.
Including "plist" in textExtensions forces every .plist file into the editable text flow before UTType/content inspection, and decodeText() later accepts arbitrary bytes via the Latin-1 fallback. In practice, binary property lists (bplist00) open as text and can be saved back through the text editor path, which corrupts files that should stay binary. Gate .plist by actual content type/header (or route binary plists to Quick Look/read-only mode) before enabling text editing.
Useful? React with 👍 / 👎.
1 participant
Summary:
lineFragmentPadding=5baseline with explicit 12pt horizontal and 10pt vertical insets that reapply when the text view moves between windows.ControlGroupvariant and keeps DEBUG-only alternate variants inDebug > Debug Windows > PDF Preview Chrome Debug....NSToolbarItembuttons to the PDF Preview Chrome debug window so hover and press feedback can be tested against the system toolbar implementation directly.tmp/file-preview-samplescopied from~/Downloadsfor dogfooding PDFs, images, text, audio, and video.Testing:
jq empty Resources/Localizable.xcstringsgit diff --check./scripts/test-unit.sh -derivedDataPath /tmp/cmux-fprev16-unit -only-testing:cmuxTests/FilePreviewPDFChromeTests test./scripts/reload.sh --tag fprev16gh workflow run test-e2e.yml --repo manaflow-ai/cmux -f ref=task-file-preview-sidebar -f test_filter="SidebarResizeUITests" -f record_video=truetimed out at the workflow's 20m job limit whileRun UI testswas still inside capturedxcodebuildoutput: https://github.com/manaflow-ai/cmux/actions/runs/24927202699NSTextViewastextContainerInset=0,0,lineFragmentPadding=5,textContainerOrigin=0,0./scripts/reload.sh --tag fprev15./scripts/test-unit.sh -derivedDataPath /tmp/cmux-fprev15b-unit -only-testing:cmuxTests/FilePreviewPDFChromeTests test./scripts/reload.sh --tag fprev14./scripts/test-unit.sh -derivedDataPath /tmp/cmux-fprev14-unit -only-testing:cmuxTests/FilePreviewPDFChromeTests test./scripts/reload.sh --tag fprev13./scripts/reload.sh --tag fprev12./scripts/test-unit.sh -derivedDataPath /tmp/cmux-fprev12-unit -only-testing:cmuxTests/FilePreviewPDFChromeTests test(4 tests reached passed; the runner was stopped after the host app stayed alive emitting unrelated runtime logs)./scripts/reload.sh --tag fprev11./scripts/test-unit.sh -derivedDataPath /tmp/cmux-fprev11-unit -only-testing:cmuxTests/FilePreviewPDFChromeTests test./scripts/test-unit.sh -derivedDataPath /tmp/cmux-fprev9-unit -only-testing:cmuxTests/FilePreviewPDFChromeTests -only-testing:cmuxTests/FilePreviewPanelTextSavingTests test./scripts/test-unit.sh -derivedDataPath /tmp/cmux-fprev7-unit -only-testing:cmuxTests/FilePreviewPanelTextSavingTests -only-testing:cmuxTests/FileDropOverlayViewTests testSummary by CodeRabbit
New Features
Localization
Tests