Diff viewer: untracked files in unstaged, persisted viewer prefs, soft refresh, hunk/file navigation #6010
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -465,6 +465,10 @@ extension CMUXCLI { | |
| case scrollToBottom = "diffViewerScrollToBottom" | ||
| case scrollToTop = "diffViewerScrollToTop" | ||
| case openFileSearch = "diffViewerOpenFileSearch" | ||
| case nextHunk = "diffViewerNextHunk" | ||
| case prevHunk = "diffViewerPrevHunk" | ||
| case nextFile = "diffViewerNextFile" | ||
| case prevFile = "diffViewerPrevFile" | ||
|
|
||
| var defaultShortcut: DiffViewerShortcut { | ||
| switch self { | ||
|
|
@@ -481,6 +485,14 @@ extension CMUXCLI { | |
| ) | ||
| case .openFileSearch: | ||
| return DiffViewerShortcut(first: DiffViewerShortcutStroke(key: "/")) | ||
| case .nextHunk: | ||
| return DiffViewerShortcut(first: DiffViewerShortcutStroke(key: "n")) | ||
| case .prevHunk: | ||
| return DiffViewerShortcut(first: DiffViewerShortcutStroke(key: "p")) | ||
| case .nextFile: | ||
| return DiffViewerShortcut(first: DiffViewerShortcutStroke(key: "]")) | ||
| case .prevFile: | ||
| return DiffViewerShortcut(first: DiffViewerShortcutStroke(key: "[")) | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1267,9 +1279,55 @@ extension CMUXCLI { | |
| if let rawLayout { | ||
| return (try parseDiffViewerLayout(rawLayout, errorMessage: "--layout must be split|unified"), "explicit") | ||
| } | ||
| // The user's last in-viewer layout choice (persisted by the app's | ||
| // viewerPrefs bridge) wins over the settings-file default, so new diff | ||
| // panels open the way the user last left one (#5284). | ||
| if let persisted = persistedDiffViewerPreferences()["layout"] as? String { | ||
| return (persisted, "default") | ||
| } | ||
| return (diffViewerDefaultLayoutSetting() ?? "unified", "default") | ||
| } | ||
|
|
||
| /// Reads the diff viewer display preferences persisted by the app | ||
| /// (`~/Library/Application Support/cmux/diff-viewer/preferences.json`), | ||
| /// sanitized to known keys/values. Returns an empty dictionary when the | ||
| /// file is missing or unreadable. | ||
| private func persistedDiffViewerPreferences() -> [String: Any] { | ||
| let fileURL: URL | ||
| if let override = ProcessInfo.processInfo.environment["CMUX_DIFF_VIEWER_PREFS_PATH"], | ||
| !override.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| fileURL = URL(fileURLWithPath: override, isDirectory: false) | ||
| } else if let appSupport = FileManager.default.urls( | ||
| for: .applicationSupportDirectory, | ||
| in: .userDomainMask | ||
| ).first { | ||
| fileURL = appSupport | ||
| .appendingPathComponent("cmux", isDirectory: true) | ||
| .appendingPathComponent("diff-viewer", isDirectory: true) | ||
| .appendingPathComponent("preferences.json", isDirectory: false) | ||
| } else { | ||
| return [:] | ||
| } | ||
| guard let data = try? Data(contentsOf: fileURL), | ||
| let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any] else { | ||
| return [:] | ||
| } | ||
| var sanitized: [String: Any] = [:] | ||
| if let layout = object["layout"] as? String, layout == "split" || layout == "unified" { | ||
| sanitized["layout"] = layout | ||
| } | ||
| if let indicators = object["diffIndicators"] as? String, | ||
| indicators == "bars" || indicators == "classic" || indicators == "none" { | ||
| sanitized["diffIndicators"] = indicators | ||
| } | ||
| for key in ["wordWrap", "wordDiffs", "lineNumbers", "showBackgrounds", "expandUnchanged"] { | ||
| if let value = object[key] as? Bool { | ||
| sanitized[key] = value | ||
| } | ||
| } | ||
| return sanitized | ||
| } | ||
|
Comment on lines
+1295
to
+1329
Contributor
There was a problem hiding this comment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
|
|
||
| private func parseDiffViewerLayout(_ rawValue: String, errorMessage: String) throws -> String { | ||
| let normalized = rawValue | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
|
|
@@ -1436,7 +1494,15 @@ extension CMUXCLI { | |
| let sourceLabel: String | ||
| switch source { | ||
| case .unstaged: | ||
| // Untracked files are part of the unstaged working-tree state but | ||
| // plain `git diff` omits them, which silently hides files agents | ||
| // just created. Append an added-file patch per untracked path. | ||
| patch = try joinedGitDiffPatches( | ||
| [gitStdout(gitDiffPatchArguments(["--"]), in: repoRoot)] | ||
| + gitUntrackedPaths(in: repoRoot).map { path in | ||
| try gitAddedUntrackedPatch(path: path, in: repoRoot) | ||
| } | ||
| ) | ||
|
Comment on lines
+1500
to
+1505
Contributor
There was a problem hiding this comment.
There was a problem hiding this comment. One bad untracked fails unstagedMedium Severity Building the unstaged patch runs   Reviewed by Cursor Bugbot for commit 9fa2556. Configure here.
Comment on lines
+1497
to
+1505
There was a problem hiding this comment. 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift Bound untracked patch fan-out in Line 1502 spawns one As per coding guidelines, production scalable paths must flag per-target rescans and unbenchmarked algorithm choices. 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| sourceLabel = "git unstaged" | ||
| case .staged: | ||
| patch = try gitStdout(gitDiffPatchArguments(["--cached", "--"]), in: repoRoot) | ||
|
|
@@ -3888,10 +3954,14 @@ extension CMUXCLI { | |
| try? writeDiffViewerEmptyStatePage(message: error.message, page: page, sourceSet: sourceSet) | ||
| completion.completedPageURLs.insert(page.url) | ||
| return completion | ||
| } catch { | ||
| // A fallback candidate that cannot be read (empty, or e.g. | ||
| // last-turn without a workspace/surface context) is skipped; | ||
| // only the originally selected source's own failure may | ||
| // surface. Otherwise `cmux diff --unstaged` in a clean repo | ||
| // outside a cmux terminal dead-ends on a raw last-turn | ||
| // context error instead of the friendly empty state (#5246). | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| continue | ||
|
Comment on lines
+3957
to
3964
Contributor
There was a problem hiding this comment.
The outer Concrete failure: the user's The intended fix was specifically for the case where last-turn context throws a non- |
||
| } | ||
| } | ||
| // No source has changes: render the selected source's friendly empty | ||
|
|
@@ -5633,6 +5703,13 @@ extension CMUXCLI { | |
| "baseOptions": baseOptions.map(\.jsonObject), | ||
| "generatedAt": ISO8601DateFormatter().string(from: Date()) | ||
| ] | ||
| // Persisted display toggles (word wrap, line numbers, …) seed the | ||
| // page's initial options so first paint matches the user's last | ||
| // session; the page then re-syncs live through the viewerPrefs bridge. | ||
| let viewerOptions = persistedDiffViewerPreferences().filter { $0.key != "layout" } | ||
| if !viewerOptions.isEmpty { | ||
| payload["viewerOptions"] = viewerOptions | ||
| } | ||
| if let statusMessage { | ||
| payload["statusMessage"] = statusMessage | ||
| payload["statusIsError"] = statusIsError | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update CLI usage text for the new layout precedence.
Layout now prefers persisted viewer prefs when
--layoutis omitted, but Line 6191 still says the default is unified/settings-based. That help output is now misleading.Suggested help-text update
🤖 Prompt for AI Agents