Repository navigation
iOS diff viewer: native tree + Pierre web rendering over new mobile git RPC - #8014
azooz2003-bit wants to merge 16 commits into
Conversation
Mac host: workspace-scoped git status (porcelain+numstat, renames, binary, untracked-as-added) and size-capped per-path unified diff (4MB soft cap, truncated/too_large reporting) via WorkspaceGitService in CmuxGit. iOS: response decoders + auth registration in CmuxMobileRPC. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
payload.mobileHost hides desktop chrome and exposes window.cmuxMobileDiff (scrollToFile/nextFile/prevFile/setLayout/setThemeMode) plus throttled ready/stats/currentFile/error messages to webkit cmuxMobileDiff handler. Split uses Pierre diffStyle. macOS behavior unchanged without the flag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
DEBUG-gated Changes toolbar button on workspace detail opens a fullScreenCover: native collapsible file tree (M/A/D/R badges, +/- counts, renames) from mobile.workspace.git.status, and a continuous Pierre-rendered diff in a WKWebView served via cmux-mobile-diff:// scheme handler that streams batched mobile.workspace.git.diff patches. Native sticky header (current file, prev/next, tree sheet), unified portrait / split landscape, light/dark themes, loading/empty/error and too-large states. Debug flag cmux.mobile.debug.diffViewerChangesEnabled. Localized en+ja (37 keys). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 end-to-end mobile workspace Git status/diff support, including macOS Git parsing and RPC endpoints, iOS RPC models and native SwiftUI browsing, a bundled mobile WebKit viewer, authorization updates, and tests. ChangesWorkspace Git backend
Mobile RPC transport
Mobile web diff bridge
Native iOS viewer
Application integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceDetailView
participant MobileDiffViewerModel
participant MobileDiffRPCService
participant TerminalController
participant WorkspaceGitService
participant MobileDiffWebView
WorkspaceDetailView->>MobileDiffViewerModel: Present Changes viewer
MobileDiffViewerModel->>MobileDiffRPCService: loadStatus()
MobileDiffRPCService->>TerminalController: mobile.workspace.git.status
TerminalController->>WorkspaceGitService: status(forDirectory:)
WorkspaceGitService-->>TerminalController: WorkspaceGitStatus
TerminalController-->>MobileDiffRPCService: status response
MobileDiffWebView->>MobileDiffRPCService: patchStream(paths:)
MobileDiffRPCService->>TerminalController: mobile.workspace.git.diff
TerminalController->>WorkspaceGitService: diff(forDirectory:paths:)
WorkspaceGitService-->>TerminalController: WorkspaceGitDiff
TerminalController-->>MobileDiffWebView: streamed patch chunks
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 2 warnings)
✅ Passed checks (19 passed)
✨ 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 |
|
Dogfood seed recipe used for evidence (creates /tmp/dfv1-seed/{basic,perf,clean}): #!/bin/bash
# Seed repos for iOS diff viewer phase 1 acceptance criteria.
# Creates /tmp/dfv1-seed/basic (M/A/D/R + binary + large + untracked) and /tmp/dfv1-seed/perf (100 files / ~10k changed lines).
set -euo pipefail
ROOT=/tmp/dfv1-seed
rm -rf "$ROOT"
mkdir -p "$ROOT"
# ---------- basic repo ----------
B="$ROOT/basic"
mkdir -p "$B" && cd "$B"
git init -q -b main
git config user.email dfv1@example.com && git config user.name "DFV1 Seed"
mkdir -p Sources/App Sources/Util Docs
# file to modify
cat > Sources/App/Login.swift <<'EOF'
import Foundation
struct LoginValidator {
func validate(email: String, password: String) -> Bool {
guard email.contains("@") else { return false }
guard password.count >= 8 else { return false }
return true
}
func normalize(_ email: String) -> String {
return email.lowercased()
}
}
EOF
# file to delete
cat > Sources/Util/Legacy.swift <<'EOF'
// Legacy helpers kept for compatibility.
func legacyThing() -> Int { return 42 }
func legacyOther() -> Int { return 7 }
EOF
# file to rename
cat > Sources/Util/StringHelpers.swift <<'EOF'
extension String {
var trimmed: String { trimmingCharacters(in: .whitespacesAndNewlines) }
var isBlank: Bool { trimmed.isEmpty }
}
EOF
# binary file (small PNG)
printf '\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01\x08\x06\x00\x00\x00\x1f\x15\xc4\x89\x00\x00\x00\nIDATx\x9cc\x00\x01\x00\x00\x05\x00\x01\r\n\x2d\xb4\x00\x00\x00\x00IEND\xaeB\x60\x82' > Docs/logo.png
# large text file
seq 1 3000 | awk '{print "line " $1 ": stable content alpha beta gamma"}' > Docs/big.txt
# typescript file for highlighting variety
cat > Sources/App/api.ts <<'EOF'
export interface User { id: string; name: string }
export async function fetchUser(id: string): Promise<User> {
const res = await fetch(`/api/users/${id}`)
if (!res.ok) throw new Error(`failed: ${res.status}`)
return res.json() as Promise<User>
}
EOF
git add -A && git commit -qm "seed base"
# modified (word-level changes on some lines)
sed -i '' -e 's/password.count >= 8/password.count >= 12/' -e 's/email.lowercased()/email.trimmingCharacters(in: .whitespaces).lowercased()/' Sources/App/Login.swift
cat >> Sources/App/Login.swift <<'EOF'
extension LoginValidator {
func strength(of password: String) -> Int {
var score = password.count
if password.rangeOfCharacter(from: .decimalDigits) != nil { score += 5 }
if password.rangeOfCharacter(from: .uppercaseLetters) != nil { score += 5 }
return score
}
}
EOF
# modified typescript (intra-line)
sed -i '' -e 's|/api/users/|/api/v2/users/|' -e 's/failed:/fetchUser failed:/' Sources/App/api.ts
# added (staged)
cat > Sources/App/Signup.swift <<'EOF'
import Foundation
struct SignupFlow {
let validator = LoginValidator()
func canSubmit(email: String, password: String) -> Bool {
validator.validate(email: email, password: password)
}
}
EOF
git add Sources/App/Signup.swift
# deleted
git rm -q Sources/Util/Legacy.swift
# renamed (with tiny modification to keep >50% similarity)
git mv Sources/Util/StringHelpers.swift Sources/Util/TextHelpers.swift
printf '\n// moved 2026-07\n' >> Sources/Util/TextHelpers.swift
git add Sources/Util/TextHelpers.swift
# binary modified
printf '\x00\x01\x02extra' >> Docs/logo.png
# large diff in big file: change a band of lines + insert block
sed -i '' -e 's/stable content alpha/CHANGED content omega/' Docs/big.txt
# untracked
cat > Docs/notes.md <<'EOF'
# Release notes draft
- diff viewer phase 1
- untracked file must appear as Added
EOF
# ---------- perf repo ----------
P="$ROOT/perf"
mkdir -p "$P" && cd "$P"
git init -q -b main
git config user.email dfv1@example.com && git config user.name "DFV1 Seed"
mkdir -p src
for i in $(seq 1 100); do
f="src/module_$(printf '%03d' "$i").swift"
{ echo "import Foundation"; echo "struct Module$i {";
for j in $(seq 1 80); do echo " func f${j}_v1() -> Int { return $((i*100+j)) }"; done;
echo "}"; } > "$f"
done
git add -A && git commit -qm "seed base"
# change ~50 lines in every file (~5000 removed + ~5000 added = ~10k changed lines)
for i in $(seq 1 100); do
f="src/module_$(printf '%03d' "$i").swift"
sed -i '' -e 's/_v1() -> Int { return \([0-9]*[02468]\) }/_v2() -> Int { return \1 + 1 }/' "$f"
done
echo "Seeded:"
cd "$ROOT/basic" && echo "-- basic:" && git status --porcelain=v1 | head -12
cd "$ROOT/perf" && echo "-- perf: $(git diff --numstat | wc -l | tr -d ' ') files changed, $(git diff --shortstat)" |
Greptile SummaryThis PR adds a read-only diff viewer to the iOS app. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (10): Last reviewed commit: "iOS diff viewer: serve scheme responses ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| arguments: ["diff", "HEAD", "--numstat", "-z", "-M", "--"], | ||
| acceptedExitStatuses: [0], | ||
| operation: "status.numstat" | ||
| ) | ||
|
|
||
| let parser = WorkspaceGitStatusParser() |
There was a problem hiding this comment.
When the workspace is a newly initialized repository with no commits, git diff HEAD fails because HEAD does not resolve. The status RPC then reports an error instead of returning the untracked files as additions, so the diff viewer cannot open for that repository.
| var untrackedNumstatByPath: [String: String] = [:] | ||
| for entry in porcelainEntries where entry.untracked { | ||
| untrackedNumstatByPath[entry.path] = try await runGit( | ||
| in: repoRoot, | ||
| arguments: ["diff", "--no-index", "--numstat", "--no-color", "--", "/dev/null", entry.path], | ||
| acceptedExitStatuses: [0, 1], | ||
| operation: "status.untracked_numstat" | ||
| ) |
There was a problem hiding this comment.
This runs a separate git diff --no-index process serially for every untracked file. A workspace with a generated or dependency directory containing thousands of files can launch thousands of subprocesses before status returns, leaving the Changes screen loading for an excessive time and consuming substantial host resources.
Rule Used: Flag production code that adds nested full-collect... (source)
| guard isLive(identifier) else { throw CancellationError() } | ||
| if !chunk.tooLargePaths.isEmpty { | ||
| onTooLargePaths(chunk.tooLargePaths) | ||
| } | ||
| if !chunk.data.isEmpty { | ||
| task.didReceive(chunk.data) |
There was a problem hiding this comment.
The liveness check and didReceive call are separate operations. If WebKit stops the scheme task after the check while a patch chunk arrives during reload or dismissal, this path can call didReceive on an invalidated WKURLSchemeTask, which can raise a WebKit exception and terminate the app.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHeaderView.swift`:
- Around line 17-20: Update the Image frame sizing in the back, previous, and
next Button declarations to provide at least a 44×44pt tap target. Preserve the
existing chevron imagery and button actions while applying the same minimum
dimensions consistently across all three navigation controls.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHostPage.swift`:
- Around line 34-43: Add focused unit coverage for htmlData() using a title
containing </script>, &, <, >, and ". Assert the generated HTML neutralizes the
script-closing sequence and applies the expected HTML escaping in the title
element, preserving the existing escaping behavior.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffRPCService.swift`:
- Around line 6-73: Move the response decoding performed by
MobileDiffRPCService.loadStatus(), loadDiff(), and the patchStream() batch flow
off the main actor by adding an explicit non-main execution hop or applying
`@concurrent` where supported. Preserve the existing request, retry, cancellation,
yielding, and error-propagation behavior while ensuring MobileDiffStatusSnapshot
and MobileSyncGitDiffResponse parsing cannot block UI rendering.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffScreen.swift`:
- Around line 40-43: Replace the root GeometryReader in MobileDiffScreen with
onGeometryChange, storing the measured size or width needed to derive
MobileDiffHostPage.Layout. Preserve the existing landscape-to-split and
portrait-to-unified behavior while keeping the rest of the screen outside a
geometry container.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeDirectory.swift`:
- Around line 10-11: Update MobileDiffTreeDirectory so fileCount is computed
once during tree construction and stored as an immutable value, rather than
recursively recalculating descendants on every access. Adjust the directory
initializer/building logic and MobileDiffTree.append usage as needed while
preserving the existing count for each directory row.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeView.swift`:
- Around line 14-16: Cache the flattened rows derived from snapshot.files
instead of constructing MobileDiffTree and calling visibleRows inside body.
Update that cached result only when snapshot changes, such as through the view’s
snapshot-change lifecycle, while continuing to apply collapsedDirectories and
selectedPath interactions without rebuilding the tree for each render.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffWebViewCoordinator.swift`:
- Around line 50-53: Update the "error" handling in MobileDiffWebViewCoordinator
so it never forwards the bridge’s raw message to controller.showError. Always
display a curated user-safe error message such as Self.renderError, while
retaining any raw detail only for internal diagnostics if needed.
In
`@Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileDiffTreeTests.swift`:
- Around line 32-35: Update the assertions in MobileDiffTreeTests around
visibleRows so they do not index visible after non-fatal `#expect` checks. Compare
visible.map(\.id) directly with the expected IDs, or use try `#require` to
establish the row count before indexing, while preserving the existing expected
order.
In
`@Packages/macOS/CmuxGit/Sources/CmuxGit/Parsing/WorkspaceGitStatusParser.swift`:
- Around line 5-13: Update parse in WorkspaceGitStatusParser so constructing
numstatByPath cannot trap when trackedEntries contains duplicate paths. Replace
Dictionary(uniqueKeysWithValues:) with a uniquing approach and define the
duplicate behavior explicitly, such as retaining the first or last entry, while
preserving the existing parsing flow.
In `@Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift`:
- Around line 55-63: Update the untracked-file numstat logic around
untrackedNumstatByPath to execute git calls concurrently with a bounded task
group or equivalent concurrency limit, rather than awaiting each runGit
sequentially. Preserve each entry’s path-to-numstat association, accepted exit
statuses, operation name, and existing error behavior while preventing unbounded
subprocess concurrency.
🪄 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: 09654c73-b5d4-4abd-957c-2d76462953be
📒 Files selected for processing (70)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncGitDiffResponse.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncGitDiffTooLargeFile.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncGitStatusFile.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncGitStatusResponse.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileSyncGitResponseTests.swiftPackages/iOS/CmuxMobileShellUI/Package.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffAppearance.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffBatchPlanner.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffFileChange.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHeaderView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHostPage.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffMIMEType.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffMutableDirectory.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffPatchChunk.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffRPCClientFactory.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffRPCService.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffStatusSnapshot.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTree.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeBuilder.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeDirectory.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeRow.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeRowView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffURLSchemeHandler.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffViewerCover.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffViewerModel.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffWebView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffWebViewController.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffWebViewCoordinator.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileDiffBatchPlannerTests.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileDiffMIMETypeTests.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileDiffTreeTests.swiftPackages/macOS/CmuxGit/README.mdPackages/macOS/CmuxGit/Sources/CmuxGit/Model/WorkspaceGitDiff.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/Model/WorkspaceGitStatus.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/Model/WorkspaceGitStatusFile.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/Model/WorkspaceGitTooLargePath.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/Parsing/WorkspaceGitNumstatEntry.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/Parsing/WorkspaceGitParseError.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/Parsing/WorkspaceGitPorcelainEntry.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/Parsing/WorkspaceGitStatusParser.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitDiffAccumulator.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitServiceError.swiftPackages/macOS/CmuxGit/Tests/CmuxGitTests/WorkspaceGitDiffAccumulatorTests.swiftPackages/macOS/CmuxGit/Tests/CmuxGitTests/WorkspaceGitStatusParserTests.swiftResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/Mobile/MobileHostService.swiftSources/TerminalController+MobileGit.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojios/cmux-ios.xcodeproj/project.pbxprojios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swiftwebviews/src/App.tsxwebviews/src/diff-stream.tswebviews/src/global.d.tswebviews/src/mobile-diff.tswebviews/src/pierre-options.tswebviews/src/styles.csswebviews/src/surfaces/diffSurface.tsxwebviews/src/types.tswebviews/test/app.test.tsxwebviews/test/mobile-diff.test.tswebviews/test/pierre-options.test.ts
| Button(action: back) { | ||
| Image(systemName: "chevron.left") | ||
| .frame(width: 32, height: 36) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Chevron buttons are smaller than Apple's minimum tap-target guidance.
back, previous, and next use .frame(width: 32, height: 36), below Apple's recommended 44×44pt minimum hit target. These are frequently-tapped primary navigation controls in the sticky header, so undersized targets increase mis-taps, especially for users with motor impairments.
♿️ Proposed fix to widen tap targets
Button(action: back) {
Image(systemName: "chevron.left")
- .frame(width: 32, height: 36)
+ .frame(width: 44, height: 44)
}Apply similarly to the previous and next buttons.
Also applies to: 51-54, 60-63
🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHeaderView.swift`
around lines 17 - 20, Update the Image frame sizing in the back, previous, and
next Button declarations to provide at least a 44×44pt tap target. Preserve the
existing chevron imagery and button actions while applying the same minimum
dimensions consistently across all three navigation controls.
| let jsonData = try JSONSerialization.data(withJSONObject: config, options: [.sortedKeys]) | ||
| guard var json = String(data: jsonData, encoding: .utf8) else { | ||
| throw CocoaError(.fileWriteInapplicableStringEncoding) | ||
| } | ||
| json = json.replacingOccurrences(of: "</", with: "<\\/") | ||
| let escapedTitle = title | ||
| .replacingOccurrences(of: "&", with: "&") | ||
| .replacingOccurrences(of: "<", with: "<") | ||
| .replacingOccurrences(of: ">", with: ">") | ||
| .replacingOccurrences(of: "\"", with: """) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Consider unit tests for the HTML/JSON escaping in htmlData().
The </script> neutralization and <title> HTML-escaping are the only defenses against injection from a user-editable title (workspace name). A small test asserting a title containing </script>, &, <, >, " renders safely would guard against silent regressions here.
🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHostPage.swift`
around lines 34 - 43, Add focused unit coverage for htmlData() using a title
containing </script>, &, <, >, and ". Assert the generated HTML neutralizes the
script-closing sequence and applies the expected HTML escaping in the title
element, preserving the existing escaping behavior.
| GeometryReader { geometry in | ||
| let layout: MobileDiffHostPage.Layout = geometry.size.width > geometry.size.height | ||
| ? .split | ||
| : .unified |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd Package.swift Packages/iOS/CmuxMobileShellUI --exec cat {}Repository: manaflow-ai/cmux
Length of output: 2879
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffScreen.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 6193
Replace the root GeometryReader with onGeometryChange Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffScreen.swift:40-43
CmuxMobileShellUI targets iOS 18, so this split/unified choice can use onGeometryChange instead of wrapping the whole screen in GeometryReader. That keeps the measurement localized and avoids making the entire view hierarchy depend on a geometry container.
🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffScreen.swift`
around lines 40 - 43, Replace the root GeometryReader in MobileDiffScreen with
onGeometryChange, storing the measured size or width needed to derive
MobileDiffHostPage.Layout. Preserve the existing landscape-to-split and
portrait-to-unified behavior while keeping the rest of the screen outside a
geometry container.
Source: Coding guidelines
| var fileCount: Int { | ||
| files.count + directories.reduce(0) { $0 + $1.fileCount } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Compute directory file counts once.
fileCount recursively walks every descendant each time it is read, while MobileDiffTree.append consumes it once per directory row. Deep or large trees therefore repeat subtree scans and can degrade toward O(n²). Compute the count during tree construction and store it as an immutable value.
As per path instructions, tree building/rendering should avoid repeated full-collection scans.
🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeDirectory.swift`
around lines 10 - 11, Update MobileDiffTreeDirectory so fileCount is computed
once during tree construction and stored as an immutable value, rather than
recursively recalculating descendants on every access. Adjust the directory
initializer/building logic and MobileDiffTree.append usage as needed while
preserving the existing count for each directory row.
Source: Path instructions
| func parse( | ||
| porcelain: String, | ||
| trackedNumstat: String, | ||
| untrackedNumstatByPath: [String: String] | ||
| ) throws -> [WorkspaceGitStatusFile] { | ||
| let porcelainEntries = try parsePorcelain(porcelain) | ||
| let trackedEntries = try parseNumstat(trackedNumstat) | ||
| var numstatByPath = Dictionary(uniqueKeysWithValues: trackedEntries.map { ($0.path, $0) }) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='Packages/macOS/CmuxGit/Sources/CmuxGit/Parsing/WorkspaceGitStatusParser.swift'
wc -l "$FILE"
echo '---'
cat -n "$FILE" | sed -n '1,220p'
echo '--- related references ---'
rg -n "parseNumstat|uniqueKeysWithValues|numstatByPath|WorkspaceGitStatusParser" Packages/macOS/CmuxGit/Sources/CmuxGit -nRepository: manaflow-ai/cmux
Length of output: 9003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift | cat -n
echo '--- tests/search ---'
rg -n "WorkspaceGitStatusParser|parseNumstat|trackedNumstat|untrackedNumstatByPath|numstat" Packages/macOS/CmuxGit -g '*Test*' -g '*Tests*' -g '*.swift'Repository: manaflow-ai/cmux
Length of output: 13300
🌐 Web query:
git diff --numstat -z duplicate path entries same path duplicate numstat records rename copy documentation
💡 Result:
When using git diff --numstat, you may observe multiple records for the same path or confusing output in specific scenarios, primarily due to how Git handles unmerged paths (merge conflicts) or rename/copy detection [1][2][3]. Unmerged Paths: When a repository has unresolved merge conflicts, git diff --numstat may show multiple entries for the same path [1][2]. This occurs because Git displays the unmerged status (e.g., stage #1, #2, #3 entries) in addition to or instead of the standard diff between a specific stage and the working tree [2]. These entries represent the distinct states of the file in the index during a conflict [2]. While --numstat is intended for machine consumption, these duplicate path entries in the output can be misleading if a script assumes a one-to-one mapping between paths and output lines [1]. Rename and Copy Detection: Git's rename and copy detection (controlled by -M and -C options) transforms the list of file changes [4]. When a file is renamed or copied, the output of git diff --numstat reflects the change using a path-pairing format (e.g., {old_path => new_path}) [5]. If rename/copy detection is active, the paths are reported as the transformation pair rather than as two separate, simple file entries [4]. Key Considerations: 1. Machine Consumption: Because --numstat can output multiple records for a single path in complex states (like unmerged entries), script writers should be aware that the output is not guaranteed to be a unique list of paths [1]. 2. Rename/Copy Formatting: The output format for renames and copies is specifically designed to indicate the relationship between the source and destination paths using brace expansion notation [5]. 3. Reliability: For parsing needs, some Git developers have suggested using --raw output in conjunction with or instead of --numstat to more clearly distinguish the status of each path (e.g., modified, added, renamed) [2]. For further details on how Git processes these changes, consult the gitdiffcore(7) manual page, which details the transformation process for renames and copies [4].
Citations:
- 1: https://public-inbox.org/git/7vfw3sqoup.fsf@alter.siamese.dyndns.org/t/
- 2: https://public-inbox.org/git/7v398mgfdu.fsf@alter.siamese.dyndns.org/
- 3: https://public-inbox.org/git/1354051310-29093-6-git-send-email-gitster@pobox.com/
- 4: https://git-scm.com/docs/gitdiffcore
- 5: http://public-inbox.org/git/CAD1gVxMg0ZiKqFABrnwt0v_=wr2X_3ErkR92RmCAtFQnuM+L5w@mail.gmail.com/T/
Dictionary(uniqueKeysWithValues:) can crash on duplicate numstat paths
Packages/macOS/CmuxGit/Sources/CmuxGit/Parsing/WorkspaceGitStatusParser.swift:12 builds numstatByPath from raw git diff --numstat -z -M output. If Git emits the same path twice in an edge-case repo state, this traps the host process instead of failing the parse. Use a uniquing initializer or handle duplicates explicitly.
🤖 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
`@Packages/macOS/CmuxGit/Sources/CmuxGit/Parsing/WorkspaceGitStatusParser.swift`
around lines 5 - 13, Update parse in WorkspaceGitStatusParser so constructing
numstatByPath cannot trap when trackedEntries contains duplicate paths. Replace
Dictionary(uniqueKeysWithValues:) with a uniquing approach and define the
duplicate behavior explicitly, such as retaining the first or last entry, while
preserving the existing parsing flow.
…, quotepath, stream resilience Status: compute untracked add counts/binary in-process (no per-file git spawn, 2000-entry cap + truncated_untracked), diff against empty tree on unborn HEAD, consume C-entry second record, GIT_OPTIONAL_LOCKS=0, core.quotepath=off. Diff RPC paths carry old_path so -M pairs renames. Capabilities advertise the git methods. iOS: batched truncated retries, mid-stream failure keeps rendered content with localized retry banner, scroll-to-file retries as items stream, tree built once per snapshot, shared path splitter, UTType MIME, single feature-flag seam. Web: raw useEffect removed, currentFile reporting on owned event points. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
| let handle = try FileHandle(forReadingFrom: url) | ||
| defer { try? handle.close() } | ||
| let data = try handle.read(upToCount: untrackedReadByteCap) ?? Data() |
There was a problem hiding this comment.
The new in-process stats path opens every retained untracked path and reads it without a timeout or file-type check. If the repository contains an untracked FIFO, opening it can succeed while the read waits indefinitely for a writer. The status RPC and Changes screen then remain stuck. Check that the path is a regular file before reading it, or use a cancellation-aware bounded operation.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
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 (3)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeView.swift (1)
43-53: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse modern
String(localized:defaultValue:)with interpolation.Using
String(format:)with manually passed arguments prevents automatic ICU pluralization in the string catalog (resulting in "1 files changed"). As per the coding guidelines, useString(localized:defaultValue:)with string interpolation, which natively handles locale-aware number formatting and automatic pluralization rules.♻️ Proposed refactor
private var summary: String { String( - format: L10n.string( - "mobile.diff.summaryFormat", - defaultValue: "%d files changed +%d −%d" - ), - snapshot.files.count, - snapshot.totalAdditions, - snapshot.totalDeletions + localized: "mobile.diff.summaryFormat", + defaultValue: "\(snapshot.files.count) files changed +\(snapshot.totalAdditions) −\(snapshot.totalDeletions)" ) }🤖 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 `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeView.swift` around lines 43 - 53, Update the summary computed property to use String(localized:defaultValue:) with interpolation instead of String(format:) and manually passed arguments. Preserve the existing mobile.diff.summaryFormat localization key and default message while allowing locale-aware number formatting and pluralization.Sources: Coding guidelines, Learnings
Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift (1)
123-138: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftAvoid repeated full-repository rescans during batched diff generation.
The
diffRPC processes paths in batches of up to 20, butWorkspaceGitService.diffexecutes a fullgit statusover the entire repository for every batch just to discover which requested paths are untracked. As per path instructions, production code must avoid repeated batch rescans for scalable workloads.
Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift#L123-L138: Remove the redundantgit statusexecution anduntrackedPathsextraction; instead, accept theuntrackedboolean as a property onWorkspaceGitDiffPath.Sources/TerminalController+MobileGit.swift#L63-L81: Read theuntrackedboolean for each path from the JSON payload (the client already has this from the initial status response) and pass it into theWorkspaceGitDiffPathinitializer.🤖 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 `@Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift` around lines 123 - 138, Remove the full-repository git status call and untrackedPaths extraction from WorkspaceGitService.diff; use the untracked property supplied by each WorkspaceGitDiffPath instead. In Sources/TerminalController+MobileGit.swift lines 63-81, read each path’s untracked boolean from the existing JSON payload and pass it to the WorkspaceGitDiffPath initializer.Source: Path instructions
Sources/TerminalController+MobileGit.swift (1)
114-126: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve the workspace globally to avoid active-window bias.
Because the mobile RPC payload only provides
workspace_idwithout awindow_id,v2ResolveTabManager(params:)will default to the host's currently active window. If the target workspace lives in a background window,tabManager.tabs.firstwill fail and incorrectly return"Workspace not found".As per learnings, apply the established pattern to locate the correct workspace across all windows if it is not found in the active manager (e.g., using
AppDelegate.shared?.locateSurface(surfaceId:)or iteratingAppDelegate.shared?.mainWindowContextsto locate the targetworkspace_id).🤖 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/TerminalController`+MobileGit.swift around lines 114 - 126, Update the workspace lookup in the method containing v2ResolveTabManager(params:) so it searches all window contexts when the active tabManager does not contain workspaceID. Reuse the established global lookup pattern, such as AppDelegate.shared?.locateSurface(surfaceId:) or mainWindowContexts, and return "Workspace not found" only after the workspace is absent globally.Source: Learnings
♻️ Duplicate comments (5)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffRPCService.swift (2)
10-17:⚠️ Potential issue | 🟠 MajorMove diff parsing off the main actor.
loadStatus()decodes the Git status payload. Under Swift 6NonisolatedNonsendingByDefault, this nonisolated async function will inherit the caller's actor isolation (e.g.,@MainActor). As per path instructions, mark heavy async parsing helpers with@concurrentto ensure they execute off the main actor and don't stall rendering.⚡ Proposed fix
- func loadStatus() async throws -> MobileDiffStatusSnapshot { + `@concurrent` + func loadStatus() async throws -> MobileDiffStatusSnapshot {🤖 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 `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffRPCService.swift` around lines 10 - 17, Mark the async loadStatus() parsing helper with `@concurrent` so MobileSyncGitStatusResponse.decode(data) and MobileDiffStatusSnapshot construction execute off the caller’s actor, while preserving the existing request and return behavior.Source: Path instructions
56-64:⚠️ Potential issue | 🟡 MinorMove diff parsing off the main actor.
Similarly,
loadDiff()decodes the diff payload. While it currently runs on the global pool when called frompatchStream, marking it with@concurrentensures it remains safely off the main actor regardless of future caller isolation changes.⚡ Proposed fix
- private func loadDiff(paths: [MobileDiffRequestPath]) async throws -> MobileSyncGitDiffResponse { + `@concurrent` + private func loadDiff(paths: [MobileDiffRequestPath]) async throws -> MobileSyncGitDiffResponse {🤖 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 `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffRPCService.swift` around lines 56 - 64, Mark the loadDiff(paths:) method with `@concurrent` so its RPC response decoding via MobileSyncGitDiffResponse.decode remains off the main actor regardless of caller isolation.Source: Path instructions
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffScreen.swift (1)
37-83: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace the root
GeometryReaderwithonGeometryChange.As per coding guidelines, prefer
onGeometryChangeoverGeometryReaderfor measuring SwiftUI layout to avoid making the entire view hierarchy depend on a geometry container.♻️ Proposed refactor
struct MobileDiffScreen: View { let model: MobileDiffViewerModel // ... `@State` private var controller: MobileDiffWebViewController `@State` private var isTreePresented = false + `@State` private var layout: MobileDiffHostPage.Layout = .unified // ... var body: some View { - GeometryReader { geometry in - let layout: MobileDiffHostPage.Layout = geometry.size.width > geometry.size.height - ? .split - : .unified - VStack(spacing: 0) { + VStack(spacing: 0) { MobileDiffHeaderView( path: controller.currentPath, index: controller.currentIndex, total: controller.total, back: back, openTree: openTree, previous: controller.previousFile, next: controller.nextFile ) Divider() ZStack { Color(uiColor: .systemBackground) MobileDiffWebView( controller: controller, service: model.service, files: snapshot.files, layout: layout, theme: colorScheme, title: workspaceTitle, onTooLargePaths: model.markTooLarge, onPartialFailure: model.markPartialDiffFailure ) if let errorMessage = controller.errorMessage { diffError(errorMessage) } if let message = model.partialDiffErrorMessage { partialDiffError(message) } } .overlay(alignment: .top) { if !controller.isReady, controller.errorMessage == nil { ProgressView() .progressViewStyle(.linear) .accessibilityLabel( L10n.string("mobile.diff.loadingDiff", defaultValue: "Loading diff…") ) } } - } - .background(Color(uiColor: .systemBackground)) } + .background(Color(uiColor: .systemBackground)) + .onGeometryChange(for: CGSize.self) { proxy in + proxy.size + } action: { size in + layout = size.width > size.height ? .split : .unified + } .toolbar(.hidden, for: .navigationBar) .sheet(isPresented: $isTreePresented) { treeSheet } }🤖 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 `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffScreen.swift` around lines 37 - 83, Replace the root GeometryReader in MobileDiffScreen.body with onGeometryChange, storing the container size in local state and deriving the split/unified Layout from that size. Preserve the existing MobileDiffHeaderView, MobileDiffWebView, error overlays, and loading overlay behavior while removing the geometry container dependency.Source: Coding guidelines
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHeaderView.swift (1)
17-20: 📐 Maintainability & Code Quality | 🟡 Minor | 💤 Low valueChevron buttons are smaller than Apple's minimum tap-target guidance.
As noted in a previous review, the navigation chevrons use
.frame(width: 32, height: 36), which is below Apple's recommended 44×44pt minimum hit target. These are frequently-tapped primary navigation controls in the sticky header, so undersized targets increase mis-taps, especially for users with motor impairments.
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHeaderView.swift#L17-L20: Widen the tap target for thebackbutton to at least 44×44.Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHeaderView.swift#L51-L54: Widen the tap target for thepreviousbutton to at least 44×44.Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHeaderView.swift#L60-L63: Widen the tap target for thenextbutton to at least 44×44.🤖 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 `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHeaderView.swift` around lines 17 - 20, Increase the frame of the back button in MobileDiffHeaderView.swift at lines 17-20 to at least 44×44 points. Apply the same minimum tap-target size to the previous button at lines 51-54 and the next button at lines 60-63; update all three button frames while preserving their existing actions and layout behavior.Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffWebViewCoordinator.swift (1)
52-55: 🔒 Security & Privacy | 🟡 Minor | 💤 Low valueAvoid forwarding raw exception text across the mobile diff bridge.
The previous review comment noted that forwarding
error.messageverbatim from the renderer could surface implementation details to the user, and requested that a curated user-safe message be used instead.The current implementation (
message.flatMap { $0.isEmpty ? nil : $0 } ?? Self.renderError) only usesSelf.renderErrorif the raw message is empty or missing. If a raw message is provided, it is still forwarded directly to the UI.Always display a curated user-safe error message, while retaining the raw detail only for internal diagnostics if needed.
🔒️ Proposed fix to always use a safe user-facing error
case "error": - let message = (body["message"] as? String)? - .trimmingCharacters(in: .whitespacesAndNewlines) - controller.showError(message.flatMap { $0.isEmpty ? nil : $0 } ?? Self.renderError) + if let rawMessage = body["message"] as? String, !rawMessage.isEmpty { + // Retain rawMessage for internal diagnostics if necessary, e.g., logging + } + controller.showError(Self.renderError)🤖 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 `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffWebViewCoordinator.swift` around lines 52 - 55, Update the "error" case in MobileDiffWebViewCoordinator so controller.showError always receives the curated Self.renderError message, regardless of any renderer-provided body["message"] value. Do not forward the raw message to the UI; retain it only for internal diagnostics if existing diagnostic handling requires it.
🤖 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 `@Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift`:
- Around line 231-236: Update untrackedStats(for:in:) to inspect the target with
FileManager.default.attributesOfItem and continue only when its type is a
regular file, returning the existing empty/error result for FIFOs, devices, and
other non-regular paths. Also adjust the status loop that processes untracked
paths to avoid monopolizing the cooperative pool, using the surrounding async
flow to offload the batch or yield between reads.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeView.swift`:
- Around line 43-53: Update the summary computed property to use
String(localized:defaultValue:) with interpolation instead of String(format:)
and manually passed arguments. Preserve the existing mobile.diff.summaryFormat
localization key and default message while allowing locale-aware number
formatting and pluralization.
In `@Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift`:
- Around line 123-138: Remove the full-repository git status call and
untrackedPaths extraction from WorkspaceGitService.diff; use the untracked
property supplied by each WorkspaceGitDiffPath instead. In
Sources/TerminalController+MobileGit.swift lines 63-81, read each path’s
untracked boolean from the existing JSON payload and pass it to the
WorkspaceGitDiffPath initializer.
In `@Sources/TerminalController`+MobileGit.swift:
- Around line 114-126: Update the workspace lookup in the method containing
v2ResolveTabManager(params:) so it searches all window contexts when the active
tabManager does not contain workspaceID. Reuse the established global lookup
pattern, such as AppDelegate.shared?.locateSurface(surfaceId:) or
mainWindowContexts, and return "Workspace not found" only after the workspace is
absent globally.
---
Duplicate comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHeaderView.swift`:
- Around line 17-20: Increase the frame of the back button in
MobileDiffHeaderView.swift at lines 17-20 to at least 44×44 points. Apply the
same minimum tap-target size to the previous button at lines 51-54 and the next
button at lines 60-63; update all three button frames while preserving their
existing actions and layout behavior.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffRPCService.swift`:
- Around line 10-17: Mark the async loadStatus() parsing helper with `@concurrent`
so MobileSyncGitStatusResponse.decode(data) and MobileDiffStatusSnapshot
construction execute off the caller’s actor, while preserving the existing
request and return behavior.
- Around line 56-64: Mark the loadDiff(paths:) method with `@concurrent` so its
RPC response decoding via MobileSyncGitDiffResponse.decode remains off the main
actor regardless of caller isolation.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffScreen.swift`:
- Around line 37-83: Replace the root GeometryReader in MobileDiffScreen.body
with onGeometryChange, storing the container size in local state and deriving
the split/unified Layout from that size. Preserve the existing
MobileDiffHeaderView, MobileDiffWebView, error overlays, and loading overlay
behavior while removing the geometry container dependency.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffWebViewCoordinator.swift`:
- Around line 52-55: Update the "error" case in MobileDiffWebViewCoordinator so
controller.showError always receives the curated Self.renderError message,
regardless of any renderer-provided body["message"] value. Do not forward the
raw message to the UI; retain it only for internal diagnostics if existing
diagnostic handling requires it.
🪄 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: 4326cad1-053c-42c1-96b5-e1fc54406b0d
⛔ Files ignored due to path filters (1)
ios/cmux.xcworkspace/xcshareddata/swiftpm/Package.resolvedis excluded by!**/Package.resolved
📒 Files selected for processing (40)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncGitStatusResponse.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileSyncGitResponseTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffBatchPlanner.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffHeaderView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffMIMEType.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffPath.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffRPCService.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffRPCServiceError.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffRequestPath.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffStatusSnapshot.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeBuilder.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeRowView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffTreeView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffURLSchemeHandler.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffViewerFeature.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffViewerModel.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffWebView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffWebViewController.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDiffWebViewCoordinator.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileDiffBatchPlannerTests.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileDiffTreeTests.swiftPackages/macOS/CmuxGit/README.mdPackages/macOS/CmuxGit/Sources/CmuxGit/Model/WorkspaceGitDiffPath.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/Model/WorkspaceGitStatus.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/Model/WorkspaceGitStatusFile.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/Parsing/WorkspaceGitStatusParser.swiftPackages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swiftPackages/macOS/CmuxGit/Tests/CmuxGitTests/Fixtures/RealGitRepository.swiftPackages/macOS/CmuxGit/Tests/CmuxGitTests/WorkspaceGitServiceTests.swiftPackages/macOS/CmuxGit/Tests/CmuxGitTests/WorkspaceGitStatusParserTests.swiftResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/TerminalController+MobileGit.swiftSources/TerminalController.swiftios/cmux/Resources/Localizable.xcstringswebviews/src/App.tsxwebviews/src/mobile-diff.tswebviews/test/mobile-diff.test.ts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Greptile P1: opening an untracked FIFO/socket blocks the status RPC indefinitely; symlink content in git is the target path, not target bytes. lstat-gate before reading; non-regular files report 0/0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| let attributes = try FileManager.default.attributesOfItem(atPath: url.path) | ||
| guard let type = attributes[.type] as? FileAttributeType, type == .typeRegular else { | ||
| return WorkspaceGitNumstatEntry( | ||
| path: path, | ||
| oldPath: nil, | ||
| additions: 0, | ||
| deletions: 0, | ||
| binary: false | ||
| ) | ||
| } | ||
| let handle = try FileHandle(forReadingFrom: url) |
There was a problem hiding this comment.
FileHandle(forReadingFrom:) opens that path in a separate operation. If a build process replaces an untracked regular file with a FIFO between these calls, the open can block indefinitely waiting for a writer. Git command timeouts and Swift task cancellation do not interrupt this synchronous open, so the status request and Changes screen can still hang. Open the descriptor non-blockingly and validate that descriptor with fstat before reading it.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift (1)
234-245: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winThread starvation risk remains unaddressed.
While this
lstatgate successfully prevents deadlocks on FIFOs and sockets, thestatusloop (lines 66-78) still executes this synchronous file-I/O up to 2,000 times back-to-back without yielding. As noted in the previous review, this monopolizes a thread on the cooperative pool and can starve the Swift runtime.To fully resolve the issue, please add
await Task.yield()inside the loop processing untracked paths.⚡ Proposed fix for the `status` loop (around line 75)
for entry in porcelainEntries { guard entry.untracked else { retainedEntries.append(entry) continue } guard untrackedCount < Self.maximumUntrackedEntries else { truncatedUntracked = true continue } untrackedCount += 1 await Task.yield() // Yield to prevent cooperative pool starvation retainedEntries.append(entry) untrackedStatsByPath[entry.path] = Self.untrackedStats(for: entry.path, in: repoRoot) }🤖 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 `@Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift` around lines 234 - 245, In the status loop that processes untracked entries, add await Task.yield() before each synchronous untracked-stat calculation, after the entry limit and count checks. Keep retained entries, truncation handling, and untrackedStatsByPath updates unchanged.
🤖 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.
Duplicate comments:
In `@Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift`:
- Around line 234-245: In the status loop that processes untracked entries, add
await Task.yield() before each synchronous untracked-stat calculation, after the
entry limit and count checks. Keep retained entries, truncation handling, and
untrackedStatsByPath updates unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1d2d763-3071-4980-8b69-bf291f0d1a20
📒 Files selected for processing (1)
Packages/macOS/CmuxGit/Sources/CmuxGit/WorkspaceGitService.swift
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Structured os_log at the three diff seams (Mac git RPC handler, iOS scheme-handler patch stream, web-posted renderer errors) so failures localize from unified logs. UTType maps .bin to application/macbinary on CI, so the fallback test case needs an unregistered extension. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Changes feature minted its own client from activeRoute+activeTicket; attach tickets are short-TTL, so diffs failed auth minutes after attach and the button was silently disabled on restored sessions. Expose the composite's shared remoteClient (connected-only) and drop the factory, env key, and viewer-owned disconnects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-apply the mobile git RPC ticket allowlist in the relocated MobileHostService+TicketAuthorization.swift; keep both sides of the pbxproj wiring (MobileGit + MobileChatArtifacts). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
git add -A during the merge swept the Xcode-generated ios/cmux.xcworkspace/xcshareddata lockfile back in; the resolved-policy gate rejects that location. Ignore it so resolution artifacts stay out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
…s them WKWebView's fetch() surfaces a non-HTTP URLResponse from a WKURLSchemeHandler as status 0, so the shared diff-stream client threw "Loading diff… (0)" the moment /patch headers arrived and the viewer showed "Changes unavailable" on every open while the stream completed underneath. Route all MobileDiffURLSchemeHandler responses through MobileDiffHTTPResponseFactory, which emits HTTPURLResponse 200 with Content-Type (charset only for text/*), Cache-Control: no-store, and Content-Length when the size is known. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Final fix record for the last open defect on this branch: WKWebView's On-sim end-to-end verification was attempted and abandoned: the shared dev Mac's tagged host app livelocked at ~78% CPU (the known sidebar LazyStack bug, fixes pending in #8067 / #8068), starving mobile RPCs, and the simulator's WKWebView WebContent process stalled under host load. Both blockers are unrelated to this change. This hybrid (Pierre-in-WKWebView) approach is superseded by the fully native SwiftUI viewer being built on |
|
Superseded by #8107 — the diff viewer was rebuilt from scratch as a fully native SwiftUI implementation (no WKWebView/Pierre bundle) over a new mobile.workspace.changes.* RPC, per owner direction. The WKWebView status-0 fetch fix investigated here is recorded at #8014 (comment) for future webview work. |
This branch was successfully deployed
1 participant
Adds a GitHub-quality read-only diff viewer to the iOS app, phase 1 of the diff viewer program.
Mac host gains
mobile.workspace.git.statusandmobile.workspace.git.diffRPCs (workspace-scoped ticket auth, worktree-vs-HEAD baseline, rename detection, untracked-as-added, binary flags, per-path diff batching with a 4MB soft cap and truncated/too_large reporting) backed by a newWorkspaceGitServiceinPackages/macOS/CmuxGit. The diff web bundle (webviews/) gains apayload.mobileHostmode: desktop chrome hidden,window.cmuxMobileDiffcontrol API (scrollToFile,nextFile/prevFile,setLayoutunified/split via PierrediffStyle,setThemeMode), and throttledready/stats/currentFile/errormessages to acmuxMobileDiffWebKit handler; macOS behavior is unchanged without the flag.On iOS, a DEBUG-gated Changes toolbar button on the workspace screen opens a full-screen viewer: native collapsible file tree (M/A/D/R badges, +/- counts, rename old→new) from the status RPC, then a continuous Pierre-rendered diff in a WKWebView served through a
cmux-mobile-diff://scheme handler that streams RPC patch batches. Native sticky header shows the current file with prev/next chevrons and reopens the tree as a sheet; portrait renders unified, landscape true side-by-side; light/dark themes registered from static cmux Ghostty palettes; loading, empty, error, and too-large states; 37 localized strings (en+ja). Debug flag:cmux.mobile.debug.diffViewerChangesEnabled.Supersedes the earlier prototype in #5629.
Dogfood seed: a repo with modified/added/deleted/renamed/binary/untracked files plus a 100-file, ~10k-line-diff repo; recipe in the first PR comment.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a read-only diff viewer to iOS with a native changed-files tree and Pierre-rendered diffs, powered by new mobile Git RPCs. Phase 1 and DEBUG-gated; macOS behavior is unchanged.
New Features
old_path), binary flags, untracked-as-added, 20-path batching, andtruncated/too_largereporting.CmuxGit:WorkspaceGitServicevia system Git; joins porcelain+numstat; computes untracked line counts/binary in-process (2,000 cap,truncated_untracked); tests added.window.cmuxMobileDiffcontrols (next/prev/scrollToFile/setLayout/setThemeMode) andready/stats/currentFile/errormessages; tags binary patches; split/unified and light/dark via Pierre options.cmux-mobile-diff://handler streaming batched patches with truncated retries; sticky header, portrait unified/landscape split, light/dark themes, too‑large and error-with-retry states; localized (en, ja).mobileDiffRPCClient(shared session) to avoid short‑lived attach-ticket expiry.Bug Fixes
fetch()accepts them in WKWebView; UTType-based MIME mapping with a true-unknown extension fallback; single feature-flag seam.core.quotepath=off,GIT_OPTIONAL_LOCKS=0, consumeC-entry second record, diff against the empty tree for unbornHEAD, and only read regular files for untracked stats (non-regular files report 0/0); client and host enforcemobile.workspace.git.*auth; re-applied host ticket allowlist after merge.os_logat the Mac RPC handler, iOS scheme-handler patch stream, and web-posted renderer errors to localize failures across seams.originHash; ignored the workspace-level SwiftPM lockfile path and removed the stray file; adopted main’s submodule pointers.Written for commit 3f8b807. Summary will update on new commits.
Summary by CodeRabbit