Repository navigation
Live reload text file previews - #3608
Closed
lawrencecchen wants to merge 26 commits into
Closed
lawrencecchen wants to merge 26 commits into
lawrencecchen wants to merge 26 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughAdded a file-watching utility and threaded revision tracking through file-preview pipelines so external edits increment a per-panel revision and trigger revision-aware reloads across PDF, image, media, and QuickLook previews. Tests and project wiring for the new watcher were added. ChangesFile Watching and Revision-Aware Preview Refreshing
Sequence DiagramsequenceDiagram
participant User as User (edits file)
participant FS as Filesystem
participant Watcher as FilePreviewFileWatcher
participant Panel as FilePreviewPanel
participant Preview as Preview Container/Coordinator
User->>FS: write file on disk
FS->>Watcher: DispatchSource event
Watcher->>Panel: emit Event.changed / reappeared (MainActor)
Panel->>Panel: handleWatchedFileChange() (check closed/generation, bump revision)
Panel->>Preview: call setURL(url, revision)
Preview->>Preview: invalidate/cache/restore per-revision viewport/state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 |
Contributor
Greptile SummaryThis PR adds live reload across all file preview types (text, image, PDF, media, Quick Look) by introducing a
Confidence Score: 4/5Safe to merge; all previously flagged actor-isolation and task-lifecycle issues have been resolved in this iteration. The core watcher and task-cancellation machinery is well-structured. The outstanding carry-forward concern is that NSImage.init(data:) decoding still executes inside await MainActor.run in the image reload path, keeping potentially slow RAW/TIFF decodes on the main thread. FilePreviewPanel.swift also grew by 321 lines to nearly 4,000 lines, which the team's file-budget rule flags. Sources/Panels/FilePreviewPanel.swift — continued growth of a very large file; the image decode path inside MainActor.run is the item most worth revisiting before the next reload-heavy change. Important Files Changed
Sequence DiagramsequenceDiagram
participant FS as File System
participant FW as FilePreviewFileWatcher
participant MA as @MainActor (Panel)
participant TV as Text/PDF/Image View
FS->>FW: DispatchSource event (write/delete/rename)
FW->>FW: scheduleFileEventHop (lock-guarded Task)
FW->>MA: handleFileEvent via Task @MainActor
MA->>MA: enqueueEvent coalesce flush after Task.yield
MA->>MA: handleWatchedFileChange(event)
MA->>MA: fileContentRevision increment
MA->>MA: isFileUnavailable update
alt previewMode == .text
MA->>MA: loadTextContent replacingDirtyContent false
MA->>MA: applyTextLoadResult rebase dirty content if matches
else PDF or image
TV->>TV: setURL called by SwiftUI updateNSView
TV->>TV: Task.detached load doc or image data off-main
TV->>MA: MainActor.run applyLoadedDocument or applyLoadedImage
end
Reviews (16): Last reviewed commit: "Document file preview task slot isolatio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/FilePreviewTextFileWatcher.swift`:
- Around line 1-123: Extract FilePreviewTextFileWatcher into a new SwiftPM
library package (e.g., CMUXFileWatch) and make the class and any APIs used by
other targets public; move the file into the package target and keep its imports
(Darwin, Foundation) while removing app-target-only dependencies. In the app
Package.swift add CMUXFileWatch as a dependency and change site references to
import CMUXFileWatch instead of using the in-app Sources/Panels copy; ensure
startFileWatcher, startDirectoryWatcher, handleFileEvent, handleDirectoryEvent
and the FilePreviewTextFileWatcher type are public/internal-as-needed and update
any other duplicated DispatchSource watchers (e.g., in MarkdownPanel.swift,
KeyboardShortcutSettingsFileStore.swift, TerminalController.swift,
FileExplorerStore.swift, CmuxConfig.swift) to reuse the new package APIs.
Finally, run swift build/tests to verify visibility and fix any access-level or
API surface mismatches.
- Around line 78-87: handleFileEvent's delete/rename branch currently calls
stopFileWatcher(), onEvent(.movedOrDeleted) and start() but if start()
immediately rebinds the file it does so silently; update handleFileEvent to
mirror handleDirectoryEvent's behavior by ensuring that after calling start()
(or after detecting the file was recreated) you emit onEvent(.reappeared) when
you successfully rebind the file (i.e., when startFileWatcher() would be
invoked). Use the same identifying symbols: modify handleFileEvent to invoke
onEvent(.reappeared) upon successful rebind/startFileWatcher() so the API
contract matches handleDirectoryEvent.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f20a1cc7-7ffa-4f6d-aa27-87fb984e3911
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/Panels/FilePreviewPanel.swiftSources/Panels/FilePreviewTextFileWatcher.swiftcmuxTests/FilePreviewReviewFeedbackTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/FilePreviewFileWatcher.swift`:
- Around line 94-114: startDirectoryWatcher can miss a reappearance race because
the file may be recreated between opening the directory fd and the watcher
becoming active; after creating/resuming the DispatchSource (symbols:
startDirectoryWatcher, directorySource, source, fd) perform an immediate
existence check on the watched file (using directoryURL or url) and invoke the
same handling path (handleDirectoryEvent) on the main actor if the file is
present so you don't rely solely on later directory events; also ensure fd is
closed on any early return and that the check runs after directorySource =
source to avoid a race with cancellation.
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 2009-2014: Both branches of the conditional call
refreshPDFSmartFitWithoutViewportRestore(); simplify by calling
refreshPDFSmartFitWithoutViewportRestore() once before conditionally restoring
the viewportSnapshot. Specifically, move the call to
refreshPDFSmartFitWithoutViewportRestore() out of the if/else, then if let
viewportSnapshot { viewportSnapshot.restore(in: pdfView, scrollView:
pdfScrollView()) } to remove the redundant else branch and keep restore only
when viewportSnapshot is non-nil.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d79fcf81-1804-432f-b7ec-6b9f9d0bd2fe
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/Panels/FilePreviewFileWatcher.swiftSources/Panels/FilePreviewPanel.swiftcmuxTests/FilePreviewReviewFeedbackTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/FilePreviewPanel.swift (1)
1967-1969:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the user’s sidebar width across revision-only PDF reloads.
On same-file refreshes this resets
didUserResizeSidebarand overwriteslastSidebarWidth, so any manual sidebar sizing snaps back to the preferred width every time the file changes. Limit this reset to actual URL changes.♻️ Minimal fix
- let viewportSnapshot = currentURL == url ? capturePDFViewportSnapshot(anchor: .top) : nil + let isSameURL = currentURL == url + let viewportSnapshot = isSameURL ? capturePDFViewportSnapshot(anchor: .top) : nil currentURL = url currentRevision = revision pdfView.document = nil thumbnailView.setDocument(nil) outlineRoot = nil titleLabel.stringValue = url.lastPathComponent rotationAccumulator = 0 - didUserResizeSidebar = false - lastSidebarWidth = preferredSidebarWidthForCurrentMode() + if !isSameURL { + didUserResizeSidebar = false + lastSidebarWidth = preferredSidebarWidthForCurrentMode() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/FilePreviewPanel.swift` around lines 1967 - 1969, The reset block currently sets rotationAccumulator = 0, didUserResizeSidebar = false, and lastSidebarWidth = preferredSidebarWidthForCurrentMode() on every file refresh; change it so didUserResizeSidebar and lastSidebarWidth are only reset when the document URL actually changes: compare the new document's url to the stored/current url and only reset didUserResizeSidebar and recalc lastSidebarWidth via preferredSidebarWidthForCurrentMode() when they differ (always keep rotationAccumulator reset for non-URL changes if needed). Update the logic surrounding rotationAccumulator, didUserResizeSidebar, lastSidebarWidth and the URL comparison to preserve manual sidebar sizing across revision-only PDF reloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 1960-1963: When reloading the same PDF URL (the branch that calls
capturePDFViewportSnapshot(anchor:) and then sets pdfView.document = nil),
capture the current per-page rotation state (e.g., build a [pageIndex: rotation]
map by reading rotations applied via rotateCurrentPDFPage(by:) or
PDFPage.rotation) before you clear pdfView.document, persist that mapping
alongside viewportSnapshot, and then reapply those rotations inside
applyLoadedPDFDocument(...) after the new document is set so the refreshed
preview preserves rotations; apply the same capture-and-reapply fix for the
other reload path referenced around the 1991-2012 region.
- Around line 1978-1987: The nested Task.detached causes the PDF decode to run
outside structured concurrency and ignore cancellation; replace the detached
task so the PDFDocument(url: loadURL) decode runs as part of the cancellable
documentLoadTask (e.g. remove Task.detached and perform let document =
PDFDocument(url: loadURL) inside the outer Task or spawn a structured child Task
rather than Task.detached), keep the existing guard checks (Task.isCancelled,
self, currentURL/currentRevision) before calling applyLoadedPDFDocument, and
ensure any heavy work checks Task.isCancelled (or calls
Task.checkCancellation()) so stale decodes don't continue after
documentLoadTask?.cancel() (referencing documentLoadTask, Task.detached,
PDFDocument(url:), currentURL/currentRevision, and applyLoadedPDFDocument).
---
Outside diff comments:
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 1967-1969: The reset block currently sets rotationAccumulator = 0,
didUserResizeSidebar = false, and lastSidebarWidth =
preferredSidebarWidthForCurrentMode() on every file refresh; change it so
didUserResizeSidebar and lastSidebarWidth are only reset when the document URL
actually changes: compare the new document's url to the stored/current url and
only reset didUserResizeSidebar and recalc lastSidebarWidth via
preferredSidebarWidthForCurrentMode() when they differ (always keep
rotationAccumulator reset for non-URL changes if needed). Update the logic
surrounding rotationAccumulator, didUserResizeSidebar, lastSidebarWidth and the
URL comparison to preserve manual sidebar sizing across revision-only PDF
reloads.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 812881f0-75c1-4b44-96a0-ccb3a0c40d25
📒 Files selected for processing (1)
Sources/Panels/FilePreviewPanel.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/FilePreviewFileWatcher.swift`:
- Around line 68-76: In mergedEvent(_ current: Event?, _ next: Event) prefer the
latest availability transition by checking for reappearance on the "next" event
first (so a sequence like .movedOrDeleted then .reappeared yields .reappeared),
then fall back to reappeared on current, then handle movedOrDeleted (either next
or current), and finally return .changed; update the condition order in
mergedEvent to check next == .reappeared before any .movedOrDeleted checks and
include explicit checks for next/current where appropriate.
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 3164-3179: The nested Task.detached means the decode isn't
cancelled by imageLoadTask?.cancel(); instead, move the NSImage(contentsOf:
loadURL) call into the parent Task (imageLoadTask) so it respects parent
cancellation (check Task.isCancelled after the decode), and perform UI-affecting
work such as calling applyLoadedImage on the MainActor (use MainActor.run) to
keep thread-safety; keep the existing guards for currentURL and currentRevision
and ensure you do not spawn a detached task for the image decode.
In `@Sources/TerminalPaneDropTargetView.swift`:
- Around line 377-379: The guard can early-return while a fade-out animation is
still running, leaving the overlay stuck at alphaValue 0; before the guard
returns, cancel any in-flight fade-out on dropZoneOverlayView so the old
completion cannot leave the view hidden. Update the block around the guard to
call dropZoneOverlayView.layer?.removeAllAnimations() (or equivalent NSAnimation
cancel) and restore a sensible visible state (e.g., set alphaValue to the
current presentation opacity or 1 and isHidden = false) when
previousZone/activeZone changes, then proceed with the existing guard check;
reference dropZoneOverlayView, rectApproximatelyEqual(_:,_:), needsFrameUpdate,
activeZone and previousZone to locate the change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b92329fa-f88f-4d6c-b8e9-3a21ac87b4a5
📒 Files selected for processing (4)
Sources/Panels/FilePreviewFileWatcher.swiftSources/Panels/FilePreviewPanel.swiftSources/TerminalPaneDropTargetView.swiftcmuxTests/FilePreviewReviewFeedbackTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/FilePreviewPDFThumbnailSidebarTests.swift`:
- Around line 160-174: The helper keyEvent(keyCode: UInt16) currently hardcodes
characters/charactersIgnoringModifiers to NSDownArrowFunctionKey instead of
using the supplied keyCode; update keyEvent to either be split into a no-arg
downArrowKeyEvent() (if you only need down-arrow events) or compute the correct
character for the given keyCode and pass that into the NSEvent.keyEvent call so
characters and charactersIgnoringModifiers match the keyCode; locate and update
the keyEvent(...) helper (and any call sites that assume its signature) to
ensure consistency between keyCode and characters.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 316b537e-b837-4eff-b92e-67ee91975fa4
📒 Files selected for processing (2)
Sources/Panels/FilePreviewPDFThumbnailCollectionView.swiftcmuxTests/FilePreviewPDFThumbnailSidebarTests.swift
lawrencecchen
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
May 6, 2026 15:49
Stale CodeRabbit review from an earlier commit. The actionable inline findings have been addressed or are outdated, and the current CodeRabbit check is green.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Plan
Summary by cubic
Adds live reload across text, image, PDF, media, and Quick Look previews. Reloads are cancellable, coalesced, and preserve state; same‑file image/PDF reloads keep viewport/zoom/page rotations, PDFs stay visible while new content loads, and a primary click focuses the PDF canvas so arrow keys scroll.
New Features
fileContentRevision; Quick Look refreshes on revision changes.Bug Fixes
.tsopens in the text preview.FilePreviewTaskSlotand keep decode work off the main thread to avoid concurrency warnings.Written for commit fb64739. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests