Repository navigation
Add native iOS diff navigation around shared renderer - #7925
lawrencecchen wants to merge 64 commits into
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 workspace diff loading from the Mac host, workspace-scoped authorization, an iOS diff surface with file navigation, and a WebKit bridge for rendering diff content and synchronizing file selection. ChangesMobile diff viewer
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceDetailView
participant MobileShellComposite
participant MobileCoreRPCClient
participant MobileHostService
participant TerminalController
participant MobileDiffWebView
WorkspaceDetailView->>MobileShellComposite: loadMobileDiff(workspaceID)
MobileShellComposite->>MobileCoreRPCClient: send mobile.diff.load
MobileCoreRPCClient->>MobileHostService: authorize workspace request
MobileHostService->>TerminalController: dispatch mobile.diff.load
TerminalController-->>MobileHostService: return Git patch document
MobileHostService-->>MobileCoreRPCClient: return diff payload
MobileCoreRPCClient-->>MobileShellComposite: decode MobileDiffDocument
WorkspaceDetailView->>MobileDiffWebView: render loaded document
MobileDiffWebView-->>WorkspaceDetailView: report file list and selection
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (20 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 |
This comment has been minimized.
This comment has been minimized.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adds a native iOS viewer for workspace changes. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (37): Last reviewed commit: "refactor: isolate diff sheet context" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let workspaceID = workspace.id | ||
| Task { @MainActor in | ||
| do { | ||
| let document = try await store.loadMobileDiff(workspaceID: workspaceID) | ||
| guard mobileDiffState === state else { return } | ||
| state.load(document) | ||
| } catch { | ||
| guard mobileDiffState === state else { return } |
There was a problem hiding this comment.
Two refresh tasks for the same open MobileDiffState both pass the identity guard. If the earlier request finishes last, it replaces the newer patch, so rapid refreshes can leave the viewer showing stale working-tree data.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
There was a problem hiding this comment.
Fixed in 63ca18c: each reload cancels its predecessor and checks cancellation plus state identity before applying.
— Claude Code
There was a problem hiding this comment.
Fixed in 63ca18c: each reload cancels its predecessor and validates cancellation plus state identity before applying.
— Claude Code
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| } else if let errorMessage = state.errorMessage, state.document == nil { | ||
| ContentUnavailableView { | ||
| Label(L10n.string("mobile.diff.loadFailed", defaultValue: "Couldn’t Load Diff"), systemImage: "exclamationmark.triangle") | ||
| } description: { | ||
| Text(errorMessage) | ||
| } actions: { | ||
| Button(L10n.string("mobile.common.retry", defaultValue: "Retry"), action: reload) | ||
| } | ||
| } else if state.document != nil { |
There was a problem hiding this comment.
fail preserves the previous document, but this error branch only runs when document == nil. A failed refresh or WebView navigation therefore keeps showing the old patch without the error or retry action, making stale data look current.
There was a problem hiding this comment.
Fixed in 63ca18c: refresh errors replace stale content with a localized retry screen.
— Claude Code
There was a problem hiding this comment.
Fixed in 63ca18c: refresh errors replace stale content with a localized retry screen.
— Claude Code
| guard untracked.status == 0 else { | ||
| throw LoadError(code: "git_error", message: stderrMessage(untracked.stderr, fallback: "Could not list untracked files")) | ||
| } | ||
| let paths = untracked.stdout.split(separator: 0).prefix(maximumUntrackedFiles) |
There was a problem hiding this comment.
When a repository has more than 200 untracked files, prefix omits the rest but the RPC still returns success. The phone then presents an apparently complete working-tree diff with required changes missing and no truncation warning.
There was a problem hiding this comment.
Fixed in 63ca18c: more than 200 untracked files now returns an explicit too_many_files failure.
— Claude Code
There was a problem hiding this comment.
Fixed in 63ca18c: more than 200 untracked files returns an explicit too_many_files failure.
— Claude Code
| } catch { | ||
| throw LoadError(code: "git_error", message: "Could not start Git") | ||
| } | ||
| let output = stdout.fileHandleForReading.readDataToEndOfFile() |
There was a problem hiding this comment.
Each Git command buffers all stdout before the 6 MiB check runs. A large tracked diff or one large untracked binary can allocate the entire patch first, causing severe memory pressure instead of stopping at the advertised limit.
There was a problem hiding this comment.
Fixed in 63ca18c: both pipes are drained concurrently and retained output is capped while excess bytes are discarded.
— Claude Code
There was a problem hiding this comment.
Fixed in 63ca18c: Git pipes drain concurrently into bounded buffers; excess output is discarded and reported.
— Claude Code
| let output = stdout.fileHandleForReading.readDataToEndOfFile() | ||
| let errorOutput = stderr.fileHandleForReading.readDataToEndOfFile() |
There was a problem hiding this comment.
The parent drains stdout to EOF before reading stderr. If Git fills the stderr pipe while stdout remains open, Git blocks on stderr and this thread blocks waiting for stdout, so the diff request never completes.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
There was a problem hiding this comment.
Fixed in 63ca18c: stdout and stderr now drain concurrently and process completion uses terminationHandler.
— Claude Code
There was a problem hiding this comment.
Fixed in 63ca18c: stdout and stderr drain concurrently and completion uses terminationHandler.
— Claude Code
![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: af85615cd9
ℹ️ 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 untracked.status == 0 else { | ||
| throw LoadError(code: "git_error", message: stderrMessage(untracked.stderr, fallback: "Could not list untracked files")) | ||
| } | ||
| let paths = untracked.stdout.split(separator: 0).prefix(maximumUntrackedFiles) |
There was a problem hiding this comment.
When a workspace has more than 200 untracked files, this .prefix(maximumUntrackedFiles) silently drops the rest and still returns a successful patch, so the phone presents an incomplete diff with no indication that files were omitted. Please either fail with an explicit too-large/too-many-files error or include a complete result rather than truncating the review data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63ca18c: untracked-file truncation now fails explicitly instead of returning incomplete data.
— Claude Code
There was a problem hiding this comment.
Fixed in 63ca18c: untracked-file truncation now fails instead of returning incomplete data.
— Claude Code
| let trackedArguments = hasHead | ||
| ? ["diff", "--no-ext-diff", "--binary", "HEAD", "--"] | ||
| : ["diff", "--no-ext-diff", "--binary", "--"] |
There was a problem hiding this comment.
For a repository with no HEAD, git diff -- compares the worktree to the index, so files staged for the first commit are omitted; they also no longer appear in ls-files --others, making the mobile diff look empty/incomplete. In the no-HEAD case, include the cached diff against the empty tree/root (or otherwise account for indexed files) before returning the document.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63ca18c: unborn repositories include cached and worktree diffs, with staged-file coverage.
— Claude Code
| let repositoryRoot = String(data: repoResult.stdout, encoding: .utf8)? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !repositoryRoot.isEmpty else { | ||
| throw LoadError(code: "not_found", message: "Workspace is not inside a Git repository") |
There was a problem hiding this comment.
AGENTS.md requires all user-facing strings to be localized, and this message is returned by mobile.diff.load and displayed by MobileDiffContent as Text(errorMessage), so non-English iOS users see English copy for common failures such as opening a non-Git workspace. Return a code and map it to localized L10n text on the client, or otherwise localize before display.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63ca18c: iOS shows localized recovery guidance instead of host Git diagnostics.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift (1)
666-693: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear
mobileDiffStatewhen toggling chat.toggleChatMode()only flipsisChatMode/pinnedChatSessionID; ifmobileDiffStatestays set, exiting chat falls through to.diffbecauseWorkspaceActiveSurface.deriveprefers diff over terminal. Route chat enter/exit through the same surface-reset path.🤖 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/WorkspaceDetailView.swift` around lines 666 - 693, Update toggleChatMode() to clear mobileDiffState when entering or exiting chat, using the same surface-reset behavior as the toolbar actions. Preserve the existing isChatMode and pinnedChatSessionID updates so chat transitions cannot fall through to the diff surface.
🤖 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/MobileHostAuthorizationTests.swift`:
- Around line 1168-1177: Update runGit to use `#require` for the
process.terminationStatus == 0 check instead of `#expect`, preserving the existing
fail-fast behavior so subsequent Git commands do not run after a failure.
In `@ios/cmux/Resources/Localizable.xcstrings`:
- Around line 2422-2426: Replace the singular file-count key
mobile.diff.fileCount with the existing plural-aware .one/.other catalog
pattern, defining the appropriate English and Japanese forms. Update the caller
that formats this count to select the correct plural localization based on the
file count, preserving localized output for both one file and multiple files.
In
`@Packages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffPane.swift`:
- Around line 128-166: Update MobileDiffFileList to read selectedFileID into an
immutable snapshot in the enclosing view body before the List(state.files)
content closure, then use that snapshot for the row checkmark and color
comparisons. Keep row actions using the existing state methods as needed, but do
not read observable properties such as state.selectedFileID inside the List row
closure.
- Around line 66-70: Update the file-count text in MobileDiffPane to use ICU
plural localization variants, selecting the `.one` key for exactly one file and
`.other` for all other counts, while preserving the localized count formatting
and existing display behavior.
In
`@Packages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift`:
- Around line 159-176: Add a concise safety rationale comment alongside the
`MobileDiffPatchSchemeHandler` `@unchecked Sendable` declaration and `NSLock`,
documenting that the lock guards shared `html` and `patch` data accessed across
WebKit scheme-handler and main-actor contexts, and that this is required because
`WKURLSchemeHandler` is not actor-isolated.
- Around line 63-86: Update apply(state:) so the current generation is marked as
handled before returning from the MobileDiffPatchSchemeHandler.assetsAvailable
failure path; set loadedGeneration before state.fail(message:) and the early
return, preventing repeated failure handling on subsequent renders while
preserving the existing missing-assets error.
In `@Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swift`:
- Around line 339-343: Add test coverage in MobileCoreRPCClientTests for
requestNeedsStackAuthFallback with the "mobile.diff.load" request, validating
the workspace-selection and ticket-coverage behavior exposed by
MobileCoreRPCClient. Keep the new case consistent with the existing host-status
and workspace-action authorization tests.
In
`@Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceActiveSurfaceTests.swift`:
- Around line 29-37: Add a chatTakesPrecedenceOverDiff test alongside
diffTakesPrecedenceOverBrowser, invoking WorkspaceActiveSurface.derive with chat
mode and a chosen chat session enabled, an active diff, no browser, and
asserting the result is .chat.
In `@Sources/TerminalController`+MobileDiff.swift:
- Around line 52-54: Sanitize user-facing errors in the TerminalController
diff-loading paths, including the guards around tracked, staged, and unstaged
git operations. Log the raw stderr server-side for diagnostics, but pass only a
generic product-level message or approved failure reason to LoadError and
state.fail(message:), avoiding filesystem paths and implementation details.
In `@webviews/src/mobile-diff-bridge.ts`:
- Around line 53-57: Update the mobile-diff bridge message in the surrounding
function to include the active state generation, then update the Swift message
handling near MobileDiffWebView’s files-message branch to read and validate that
generation before calling state.updateFiles(...). Ignore mismatched generations
so stale files and selectedItemId cannot replace the current state, while
preserving handling for matching messages.
In `@webviews/test/mobile-diff-bridge.test.ts`:
- Around line 4-21: Extend the mobile diff bridge tests beyond mobileDiffFiles
to cover installMobileDiffBridge lifecycle behavior: verify it publishes
window.__cmuxMobileDiff, posts files and selection through the WebKit handler,
and cleanup preserves a newer bridge instance instead of removing it.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 666-693: Update toggleChatMode() to clear mobileDiffState when
entering or exiting chat, using the same surface-reset behavior as the toolbar
actions. Preserve the existing isChatMode and pinnedChatSessionID updates so
chat transitions cannot fall through to the diff surface.
🪄 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: dd014f06-4ba4-4037-94ff-c116185bbc53
📒 Files selected for processing (27)
Packages/iOS/CmuxMobileBrowser/Package.swiftPackages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffPane.swiftPackages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffState.swiftPackages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swiftPackages/iOS/CmuxMobileBrowser/Tests/CmuxMobileBrowserTests/MobileDiffStateTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+Diff.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileDiffDocument.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceActiveSurface.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+Surfaces.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceActiveSurfaceTests.swiftResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/Mobile/MobileHostService+Capabilities.swiftSources/Mobile/MobileHostService.swiftSources/TerminalController+MobileDiff.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileHostAuthorizationTests.swiftios/cmux-ios.xcodeproj/project.pbxprojios/cmux/Resources/Localizable.xcstringswebviews/src/App.tsxwebviews/src/global.d.tswebviews/src/mobile-diff-bridge.tswebviews/src/styles.csswebviews/src/surfaces/diffSurface.tsxwebviews/test/mobile-diff-bridge.test.ts
| private func runGit(_ arguments: [String], at directory: URL) throws { | ||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/git") | ||
| process.arguments = ["-C", directory.path] + arguments | ||
| process.standardOutput = FileHandle.nullDevice | ||
| process.standardError = FileHandle.nullDevice | ||
| try process.run() | ||
| process.waitUntilExit() | ||
| #expect(process.terminationStatus == 0) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use #require instead of #expect in runGit for fail-fast behavior.
#expect records a failure but does not throw, so if git init fails, the test continues into git add and git commit, cascading confusing failures. #require throws immediately, producing a single clear failure point.
♻️ Proposed fix
private func runGit(_ arguments: [String], at directory: URL) throws {
let process = Process()
process.executableURL = URL(fileURLWithPath: "/usr/bin/git")
process.arguments = ["-C", directory.path] + arguments
process.standardOutput = FileHandle.nullDevice
process.standardError = FileHandle.nullDevice
try process.run()
process.waitUntilExit()
- `#expect`(process.terminationStatus == 0)
+ `#require`(process.terminationStatus == 0)
}📝 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.
| private func runGit(_ arguments: [String], at directory: URL) throws { | |
| let process = Process() | |
| process.executableURL = URL(fileURLWithPath: "/usr/bin/git") | |
| process.arguments = ["-C", directory.path] + arguments | |
| process.standardOutput = FileHandle.nullDevice | |
| process.standardError = FileHandle.nullDevice | |
| try process.run() | |
| process.waitUntilExit() | |
| #expect(process.terminationStatus == 0) | |
| } | |
| private func runGit(_ arguments: [String], at directory: URL) throws { | |
| let process = Process() | |
| process.executableURL = URL(fileURLWithPath: "/usr/bin/git") | |
| process.arguments = ["-C", directory.path] + arguments | |
| process.standardOutput = FileHandle.nullDevice | |
| process.standardError = FileHandle.nullDevice | |
| try process.run() | |
| process.waitUntilExit() | |
| `#require`(process.terminationStatus == 0) | |
| } |
🤖 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 `@cmuxTests/MobileHostAuthorizationTests.swift` around lines 1168 - 1177,
Update runGit to use `#require` for the process.terminationStatus == 0 check
instead of `#expect`, preserving the existing fail-fast behavior so subsequent Git
commands do not run after a failure.
| @Test func diffTakesPrecedenceOverBrowser() { | ||
| #expect(WorkspaceActiveSurface.derive( | ||
| isChatMode: false, | ||
| hasChosenChatSession: false, | ||
| hasActiveBrowser: true, | ||
| hasActiveDiff: true | ||
| ) == .diff) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test for chat-over-diff precedence.
diffTakesPrecedenceOverBrowser covers diff-vs-browser, but no test exercises hasActiveDiff: true together with isChatMode: true, hasChosenChatSession: true to confirm chat still wins. Given the precedence order is the exact mechanism protecting against the mutual-exclusion gap flagged in WorkspaceDetailView.swift, a test locking in "chat beats diff" would add real confidence here.
✅ Suggested additional test
`@Test` func chatTakesPrecedenceOverDiff() {
`#expect`(WorkspaceActiveSurface.derive(
isChatMode: true,
hasChosenChatSession: true,
hasActiveBrowser: false,
hasActiveDiff: true
) == .chat)
}🤖 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/Tests/CmuxMobileShellUITests/WorkspaceActiveSurfaceTests.swift`
around lines 29 - 37, Add a chatTakesPrecedenceOverDiff test alongside
diffTakesPrecedenceOverBrowser, invoking WorkspaceActiveSurface.derive with chat
mode and a chosen chat session enabled, an active diff, no browser, and
asserting the result is .chat.
| guard tracked.status == 0 else { | ||
| throw LoadError(code: "git_error", message: stderrMessage(tracked.stderr, fallback: "Git diff failed")) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Raw git stderr is forwarded verbatim as the user-facing error message.
stderrMessage returns the raw (truncated) stderr text from git, which becomes the RPC message surfaced to the iOS diff pane (state.fail(message:)). This can include local filesystem paths and other implementation-level details from the Mac, which the project's own error-copy guideline says not to expose in user-facing text/API error bodies. Consider logging the raw stderr server-side and returning a generic, sanitized message (or a small enum of known failure reasons) to the client.
As per coding guidelines: "Do not expose raw upstream errors, stack traces, ... User-facing errors should state what happened in product terms ... Keep provider, billing, database, and authentication implementation details in sanitized logs or internal telemetry, not user-visible text."
Also applies to: 58-60, 64-67, 112-115
🤖 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`+MobileDiff.swift around lines 52 - 54, Sanitize
user-facing errors in the TerminalController diff-loading paths, including the
guards around tracked, staged, and unstaged git operations. Log the raw stderr
server-side for diagnostics, but pass only a generic product-level message or
approved failure reason to LoadError and state.fail(message:), avoiding
filesystem paths and implementation details.
Source: Coding guidelines
| test("mobile diff bridge preserves file order and stats", () => { | ||
| const source = { | ||
| paths: ["Sources/App.swift", "README.md"], | ||
| pathToItemId: new Map([ | ||
| ["Sources/App.swift", "app"], | ||
| ["README.md", "readme"], | ||
| ]), | ||
| statsByPath: new Map([ | ||
| ["Sources/App.swift", { added: 12, deleted: 3 }], | ||
| ["README.md", { added: 2, deleted: 0 }], | ||
| ]), | ||
| } as any; | ||
|
|
||
| expect(mobileDiffFiles(source)).toEqual([ | ||
| { id: "app", path: "Sources/App.swift", added: 12, deleted: 3 }, | ||
| { id: "readme", path: "README.md", added: 2, deleted: 0 }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Test the bridge lifecycle, not only the mapper.
Add assertions that installMobileDiffBridge publishes window.__cmuxMobileDiff, posts files and selection to the WebKit handler, and cleanup does not remove a newer bridge instance.
🤖 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 `@webviews/test/mobile-diff-bridge.test.ts` around lines 4 - 21, Extend the
mobile diff bridge tests beyond mobileDiffFiles to cover installMobileDiffBridge
lifecycle behavior: verify it publishes window.__cmuxMobileDiff, posts files and
selection through the WebKit handler, and cleanup preserves a newer bridge
instance instead of removing it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a776492c72
ℹ️ 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 state.isLoading, state.document == nil { | ||
| ProgressView(L10n.string("mobile.diff.loading", defaultValue: "Loading changes…")) | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| } else if let errorMessage = state.errorMessage, state.document == nil { |
There was a problem hiding this comment.
When a refresh fails after a previous successful load (or WebKit reports an asset/navigation error after state.load), MobileDiffState.fail leaves document non-nil, so this state.document == nil guard prevents the error view from rendering and the user keeps seeing the stale diff as if the refresh succeeded. Please either clear the document on failure or surface the error while a stale document is present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63ca18c: error UI takes precedence even when a prior document exists, with localized Retry.
— Claude Code
| do { | ||
| let document = try await store.loadMobileDiff(workspaceID: workspaceID) | ||
| guard mobileDiffState === state else { return } | ||
| state.load(document) |
There was a problem hiding this comment.
When the user taps Refresh again before the first mobile.diff.load RPC returns, both unstructured tasks still reference the same MobileDiffState, so the older request can finish after the newer one and this state.load(document) will overwrite the newer patch. Track a per-request id or cancel the prior task before applying the response so only the latest refresh updates the pane.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63ca18c: reload cancellation and state-identity checks reject stale responses.
— Claude Code
| } catch { | ||
| throw LoadError(code: "git_error", message: "Could not start Git") | ||
| } | ||
| let output = stdout.fileHandleForReading.readDataToEndOfFile() |
There was a problem hiding this comment.
For a workspace with a large tracked diff or a single large untracked/binary file, this buffers the entire git diff stdout before maximumPatchBytes is checked, so the 6 MB limit does not prevent the Mac from allocating or waiting on a much larger patch before it can return too_large. Please stream/read with a byte cap and terminate the git process once the limit is exceeded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63ca18c: bounded concurrent pipe drains cap retained bytes during Git output.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Packages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift (1)
67-103: 🩺 Stability & Availability | 🔴 CriticalStill an infinite fail/re-render loop when viewer assets are missing.
The
assetsAvailableguard (lines 74-77) callsstate.fail(...)without settingloadedGeneration/pendingGeneration. Sincestate.failmutateserrorMessage, which drives a re-render →updateUIView→apply(state:), the same generation re-enters this branch and callsstate.failagain indefinitely for as long as the document is loaded and assets remain missing. This is the same issue raised in a prior review of this file and does not appear to have been addressed in this revision.Note the same gap exists in the (unchanged)
catchblock at lines 100-102 forviewerHTMLfailures — worth covering with the same fix while here.🐛 Mark the generation as handled before returning on failure
guard MobileDiffPatchSchemeHandler.assetsAvailable else { + loadedGeneration = state.generation state.fail(message: L10n.string("mobile.diff.assetsMissing", defaultValue: "Diff viewer assets are missing from this build.")) return }} catch { + loadedGeneration = state.generation state.fail(message: error.localizedDescription) }As per coding guidelines: "Do not mutate state from
bodyor helpers called bybody... That creates a re-render feedback loop and pegs the main thread."🤖 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/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift` around lines 67 - 103, In apply(state:), mark the current generation as handled before returning from both the assetsAvailable failure path and the viewerHTML catch path, setting the appropriate loadedGeneration/pendingGeneration state so repeated renders do not retry the same failed generation. Preserve the existing failure messages and ensure pendingGeneration is cleared consistently.Source: Coding guidelines
🤖 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/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift`:
- Around line 254-270: Add a concise comment immediately above the guard in
stopRequest explaining that activeRequests.remove must execute first to consume
requests stopped after begin, while short-circuiting prevents recording those
requests in stoppedRequests. Keep the existing guard behavior unchanged.
---
Duplicate comments:
In
`@Packages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift`:
- Around line 67-103: In apply(state:), mark the current generation as handled
before returning from both the assetsAvailable failure path and the viewerHTML
catch path, setting the appropriate loadedGeneration/pendingGeneration state so
repeated renders do not retry the same failed generation. Preserve the existing
failure messages and ensure pendingGeneration is cleared consistently.
🪄 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: 9783e253-613d-4781-8a77-4e779465e5c4
📒 Files selected for processing (1)
Packages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift
| func beginRequest(_ requestID: ObjectIdentifier) -> Bool { | ||
| guard stoppedRequests.remove(requestID) == nil else { | ||
| stoppedRequestOrder.removeAll { $0 == requestID } | ||
| return false | ||
| } | ||
| activeRequests.insert(requestID) | ||
| return true | ||
| } | ||
|
|
||
| func stopRequest(_ requestID: ObjectIdentifier) { | ||
| guard activeRequests.remove(requestID) == nil, | ||
| stoppedRequests.insert(requestID).inserted else { return } | ||
| stoppedRequestOrder.append(requestID) | ||
| if stoppedRequestOrder.count > 64 { | ||
| stoppedRequests.remove(stoppedRequestOrder.removeFirst()) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a short comment explaining the request-tracking race handling.
beginRequest/stopRequest correctly handle both "stop before begin" and "stop after begin" races, but the correctness of the "stop after begin" case relies on activeRequests.remove(requestID) mutating the set as a side effect even when it makes the guard's first condition fail (short-circuiting the stoppedRequests.insert call). That's subtle enough that a future edit could "simplify" it into a real bug. A one-line comment on why the guard is structured this way would help.
🤖 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/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift`
around lines 254 - 270, Add a concise comment immediately above the guard in
stopRequest explaining that activeRequests.remove must execute first to consume
requests stopped after begin, while short-circuiting prevents recording those
requests in stoppedRequests. Keep the existing guard behavior unchanged.
| private func failNavigation(with error: any Error) { | ||
| let nsError = error as NSError | ||
| guard nsError.domain != NSURLErrorDomain || nsError.code != NSURLErrorCancelled else { return } | ||
| state.fail(message: error.localizedDescription) | ||
| } |
There was a problem hiding this comment.
WebKit navigation errors are not tied to the generation that started the navigation. If generation N fails with a non-cancellation error after generation N+1 starts or finishes loading, this callback calls state.fail on the current state and replaces the newer diff with an error screen. Scheme-handler errors such as .badURL can take this path. Track the generation for each navigation and ignore failures that no longer match state.generation, as the script-message and configuration paths already do.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
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!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a65dc92cf7
ℹ️ 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".
| for pathData in paths { | ||
| guard let path = String(data: Data(pathData), encoding: .utf8), !path.isEmpty else { continue } | ||
| let result = try await runGit( | ||
| ["diff", "--no-index", "--no-textconv", "--binary", "--", "/dev/null", path], |
There was a problem hiding this comment.
When an untracked file is present and the user's environment/config defines an external diff helper, this git diff --no-index invocation will run that helper; I verified GIT_EXTERNAL_DIFF=... git diff --no-index does so, while the tracked diff path above explicitly passes --no-ext-diff. That can hang the mobile refresh or append helper-specific, non-patch output for untracked files, so add --no-ext-diff here too before accepting stdout as a Git patch.
Useful? React with 👍 / 👎.
| private func failNavigation(with error: any Error) { | ||
| let nsError = error as NSError | ||
| guard nsError.domain != NSURLErrorDomain || nsError.code != NSURLErrorCancelled else { return } | ||
| state.fail(message: error.localizedDescription) |
There was a problem hiding this comment.
Ignoring cancellation errors does not prevent an older navigation from failing after a newer generation loads. If generation N reports another error after generation N+1 succeeds, this calls state.fail on the current state and replaces the newer diff with an error screen. Associate each WKNavigation with its generation and ignore failures that no longer match state.generation.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/MobileDiffTests.swift`:
- Around line 50-58: Update the repository_root assertion in the
MobileWorkingTreeDiffLoader test to compare against
repository.resolvingSymlinksInPath().path rather than repository.path, while
leaving the other document and patch expectations unchanged.
In
`@Packages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileDiffAuthTests.swift`:
- Line 39: Replace the Date().addingTimeInterval fixture in the authorization
test with the suite’s injected virtual clock or a fixed deterministic expiry
value. Ensure time-dependent behavior is tested by advancing the virtual clock
explicitly rather than reading the real wall clock.
In `@Sources/MobileWorkingTreeDiffLoader.swift`:
- Around line 108-127: Update MobileDiffProcessCancellation and the
process-launch flow around process.run() to preserve cancellation that arrives
before Process.isRunning becomes true: record pending cancellation when
cancellation races with launch, then check that state immediately after
process.run() returns and terminate the process if needed. Keep the existing
terminationHandler and Task cancellation behavior intact.
In `@webviews/test/mobile-diff-bridge.test.ts`:
- Around line 22-30: Extend the mobile diff bridge tests beyond
mobileDiffMessage to cover installMobileDiffBridge/useMobileDiffBridge lifecycle
behavior: verify window bridge registration, WebKit postMessage forwarding, and
cleanup preserving a newer bridge instance rather than clobbering 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: fd7bd05c-ee06-4eaa-bee4-58ed94631d3c
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (21)
Packages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffPane.swiftPackages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffState.swiftPackages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileDiffAuthTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+MobileDiff.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+Surfaces.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/Mobile/MobileHostService.swiftSources/MobileDiffProcessCancellation.swiftSources/MobileWorkingTreeDiffLoadError.swiftSources/MobileWorkingTreeDiffLoader.swiftSources/TerminalController+MobileDiff.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileDiffTests.swiftcmuxTests/MobileHostAuthorizationTests.swiftios/cmux/Resources/Localizable.xcstringswebviews/src/App.tsxwebviews/src/mobile-diff-bridge.tswebviews/test/mobile-diff-bridge.test.ts
💤 Files with no reviewable changes (1)
- cmuxTests/MobileHostAuthorizationTests.swift
| let document = try await MobileWorkingTreeDiffLoader().load(directory: repository.path, title: "Fixture") | ||
| let patch = try #require(document["patch"] as? String) | ||
| #expect(document["repository_root"] as? String == repository.path) | ||
| #expect(document["title"] as? String == "Fixture") | ||
| #expect(patch.contains("diff --git a/tracked.txt b/tracked.txt")) | ||
| #expect(patch.contains("diff --git a/untracked.txt b/untracked.txt")) | ||
| #expect(patch.contains("+after")) | ||
| #expect(patch.contains("+new")) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== test file ==\n'
sed -n '1,220p' cmuxTests/MobileDiffTests.swift
printf '\n== loader symbols ==\n'
rg -n "MobileWorkingTreeDiffLoader|repository_root|show-toplevel|resolvingSymlinksInPath|temporaryDirectory" -S Sources cmuxTestsRepository: manaflow-ai/cmux
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== file list ==\n'
git ls-files | rg '^(Sources|cmuxTests)/.*Mobile.*Diff.*\.swift$|MobileWorkingTreeDiffLoader|WorkingTreeDiffLoader|MobileDiff'
printf '\n== candidate implementation outline ==\n'
fd -a 'MobileWorkingTreeDiffLoader.swift' Sources cmuxTests 2>/dev/null || true
fd -a 'WorkingTreeDiffLoader.swift' Sources cmuxTests 2>/dev/null || trueRepository: manaflow-ai/cmux
Length of output: 1124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline Sources/MobileWorkingTreeDiffLoader.swift --view expanded
printf '\n== relevant source ==\n'
sed -n '1,240p' Sources/MobileWorkingTreeDiffLoader.swiftRepository: manaflow-ai/cmux
Length of output: 7690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== repository_root references ==\n'
rg -n '"repository_root"|repositoryRoot|show-toplevel|resolvingSymlinksInPath|standardizedFileURL' Sources/MobileWorkingTreeDiffLoader.swift Sources/MobileDiff*.swift Sources/TerminalController+MobileDiff.swift Sources/MobileWorkingTreeDiffLoadError.swift
printf '\n== file with line numbers ==\n'
cat -n Sources/MobileWorkingTreeDiffLoader.swift | sed -n '1,260p'Repository: manaflow-ai/cmux
Length of output: 9435
Normalize the expected repository root before comparing. MobileWorkingTreeDiffLoader returns Git’s top-level path, while repository.path comes from the temp directory URL; compare against repository.resolvingSymlinksInPath().path instead.
Suggested change
- `#expect`(document["repository_root"] as? String == repository.path)
+ `#expect`(
+ document["repository_root"] as? String
+ == repository.resolvingSymlinksInPath().path
+ )📝 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.
| let document = try await MobileWorkingTreeDiffLoader().load(directory: repository.path, title: "Fixture") | |
| let patch = try #require(document["patch"] as? String) | |
| #expect(document["repository_root"] as? String == repository.path) | |
| #expect(document["title"] as? String == "Fixture") | |
| #expect(patch.contains("diff --git a/tracked.txt b/tracked.txt")) | |
| #expect(patch.contains("diff --git a/untracked.txt b/untracked.txt")) | |
| #expect(patch.contains("+after")) | |
| #expect(patch.contains("+new")) | |
| } | |
| let document = try await MobileWorkingTreeDiffLoader().load(directory: repository.path, title: "Fixture") | |
| let patch = try `#require`(document["patch"] as? String) | |
| `#expect`(document["repository_root"] as? String == repository.resolvingSymlinksInPath().path) | |
| `#expect`(document["title"] as? String == "Fixture") | |
| `#expect`(patch.contains("diff --git a/tracked.txt b/tracked.txt")) | |
| `#expect`(patch.contains("diff --git a/untracked.txt b/untracked.txt")) | |
| `#expect`(patch.contains("+after")) | |
| `#expect`(patch.contains("+new")) | |
| } |
🤖 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 `@cmuxTests/MobileDiffTests.swift` around lines 50 - 58, Update the
repository_root assertion in the MobileWorkingTreeDiffLoader test to compare
against repository.resolvingSymlinksInPath().path rather than repository.path,
while leaving the other document and patch expectations unchanged.
|
|
||
| test("mobile diff messages carry the renderer generation", () => { | ||
| expect(mobileDiffMessage(null, "item-1", 7)).toEqual({ | ||
| type: "files", | ||
| files: [], | ||
| generation: 7, | ||
| selectedItemId: "item-1", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Bridge lifecycle (install/cleanup/postMessage) still untested.
Only mobileDiffMessage's output shape is covered here; installMobileDiffBridge/useMobileDiffBridge (window bridge registration, WebKit postMessage call, and cleanup not clobbering a newer bridge instance) remain untested, as previously noted.
🤖 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 `@webviews/test/mobile-diff-bridge.test.ts` around lines 22 - 30, Extend the
mobile diff bridge tests beyond mobileDiffMessage to cover
installMobileDiffBridge/useMobileDiffBridge lifecycle behavior: verify window
bridge registration, WebKit postMessage forwarding, and cleanup preserving a
newer bridge instance rather than clobbering it.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift (1)
222-229: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPackages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift:222-229 — hop back to the main actor before finishing
WKURLSchemeTasks.MobileDiffPatchStoreis a plain actor, sofinishRequest/failRequestcalldidReceive/didFinishon the actor executor, not the main thread. Move thependingTaskcompletion ontoMainActorfirst.🤖 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/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift` around lines 222 - 229, Update the Task flow around MobileDiffPatchStore’s beginRequest/content/finishRequest calls so WKURLSchemeTask completion occurs on MainActor. Before invoking failRequest or finishRequest with pendingTask, hop to MainActor while preserving the existing request lifecycle and error behavior.
🤖 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.
Outside diff comments:
In
`@Packages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swift`:
- Around line 222-229: Update the Task flow around MobileDiffPatchStore’s
beginRequest/content/finishRequest calls so WKURLSchemeTask completion occurs on
MainActor. Before invoking failRequest or finishRequest with pendingTask, hop to
MainActor while preserving the existing request lifecycle and error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3fca04a-e943-49a0-a83a-0777b0c523e9
📒 Files selected for processing (3)
Packages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffState.swiftPackages/iOS/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileDiffWebView.swiftPackages/iOS/CmuxMobileBrowser/Tests/CmuxMobileBrowserTests/MobileDiffStateTests.swift
| throw MobileWorkingTreeDiffLoadError(code: "git_error", message: "Could not list untracked files") | ||
| } | ||
| for pathData in paths { | ||
| guard let path = String(data: Data(pathData), encoding: .utf8), !path.isEmpty else { continue } |
There was a problem hiding this comment.
git ls-files -z returns raw path bytes, and repository filenames are not guaranteed to be valid UTF-8. When this conversion fails, continue silently omits the untracked file while the RPC reports a complete diff. A repository with such a filename therefore shows incomplete changes on iOS. Return an explicit unsupported-path error instead of skipping the file.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55d4e166d3
ℹ️ 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".
| } | ||
|
|
||
| private struct MobileDiffFileList: View { | ||
| let state: MobileDiffState |
There was a problem hiding this comment.
AGENTS.md's “Snapshot boundary for list subtrees” rule says a view with List/ForEach rows must not hold an @Observable store; this new sheet keeps MobileDiffState and its rows call state.selectFile. When the renderer posts file/selection updates or refresh toggles loading/error state while the sheet is open, every store mutation can invalidate the whole row subtree and reintroduce the Lazy/List cache churn the guideline is meant to prevent. Pass immutable files/selectedFileID snapshots plus a select closure into this sheet instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1a8c720. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Adds a mobile working-tree diff surface to the iOS workspace menu.
The Mac returns an authenticated, workspace-scoped tracked and untracked patch. iOS renders it with the existing React/Pierre diff assets inside WKWebView while native SwiftUI owns changed-file navigation, stats, previous/next, refresh, loading, and errors.
Verified with the webview suite, CmuxMobileBrowser tests, a tagged macOS build, a full iOS workspace build, and an isolated simulator run against a three-file Git fixture.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
New authenticated RPC path and WebKit custom-scheme serving of patch/assets add integration surface, but changes are scoped to mobile iOS UI with explicit auth tests and generation-scoped render lifecycle guards.
Overview
Adds an iOS View Changes flow that loads a workspace working-tree patch from the paired Mac via
mobile.diff.load, gated bymobile.diff.v1and workspace-scoped attach-ticket auth.CmuxMobileBrowserintroduces native SwiftUI chrome (file list sheet, prev/next, refresh, errors) around the existing shared diff web assets inWKWebView, using a privatecmux-mobile-diff-datascheme handler and a JS bridge to keep file selection in sync withMobileDiffState.CmuxMobileShellUIwires the diff as a new workspace surface (above browser, below chat), toolbar entry, load lifecycle, and teardown when switching workspace or other chrome modes.Reviewed by Cursor Bugbot for commit 36687ad. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a native iOS diff viewer around our shared web renderer. iOS loads a workspace‑scoped patch via
mobile.diff.load, renders it inWKWebView, and serves assets/patch via acmux-mobile-diff-datascheme with fast preload/caching, robust path handling, and race‑free refresh. Also fixes and isolates the file list sheet presentation.New Features
mobileNativeChromeis true and exposeswindow.__cmuxMobileDiff.selectFile; messages carry a generation; the diff surface sits above the browser and below chat.mobile.diff.v1;mobile.diff.loaduses workspace‑scoped attach tickets and builds a bounded working‑tree patch (6MB cap, limited untracked) with a 15s timeout; a coordinator coalesces requests and limits concurrency; external untracked helpers are disabled.cmux-mobile-diff-dataserves bundled assets and the current patch with synchronous responses, off‑main preloading, and lazy caching.Bug Fixes
Written for commit 36687ad. Summary will update on new commits.
Summary by CodeRabbit