Repository navigation
iOS native diff viewer (ndv2): GitHub Files-changed design over mobile.workspace.diffs RPC - #8154
Closed
azooz2003-bit wants to merge 7 commits into
Closed
azooz2003-bit wants to merge 7 commits into
azooz2003-bit wants to merge 7 commits into
Conversation
Contributor
|
Too many files changed for review. ( Bypass the limit by tagging |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a macOS Git diff engine, mobile diff RPC contracts and dispatch, and iOS SwiftUI diff screens with syntax highlighting, pagination, context expansion, persistence, navigation, quick notes, localization, and tests. ChangesWorkspace diff implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 22❌ Failed checks (2 warnings, 20 inconclusive)
✅ Passed checks (3 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 |
Collaborator
Author
|
Live preflight on ndv2-sim (iPhone 17 Pro, iOS 26.5) against the tagged ndv2 Mac app, seeded repo /tmp/dfv1-seed/basic (8 files: M/A/D/R + binary + untracked + large): workspace toolbar Changes entry (capability+connection gated), files-first tree (collapsible dirs with aggregate +/− counts, M/A/U status badges, binary at +0 −0, untracked as additions, 0-of-8 viewed progress), continuous diff with sticky per-file headers + viewed checkboxes + kebab, blue @@ hunk headers with send-to-agent + expand up/down/all controls, dual line-number gutters, add/del row tints with darker word-level intraline spans (signIn(with→using) pairing rendered exactly as GitHub), Swift syntax highlighting via HighlighterSwift, added-file all-additions rendering, GitHub density (no 44pt row gaps). NOT live-verified this round (unit-tested; sim HID automation degraded under host load): overflow menu (base picker / whitespace toggle / unified-split override), nav model B drawer, landscape split, iPad sidebar, send-to-agent sheet send path (needs an agent-chat workspace). Screenshots in the dogfood handoff. |
# Conflicts: # cmux.xcodeproj/project.pbxproj
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🤖 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 `@cmux.xcodeproj/project.pbxproj`:
- Around line 9133-9136: Commit the root workspace SwiftPM lockfile at
project.xcworkspace/xcshareddata/swiftpm/Package.resolved alongside the new
CmuxDiffEngine XCLocalSwiftPackageReference, ensuring it contains the updated
resolved dependency state.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/CodeHighlightBatcher.swift`:
- Around line 50-58: Update CodeHighlightBatcher.insert(_:for:) to amortize LRU
eviction instead of scanning for and removing only one entry on every
over-capacity insertion. When capacity is exceeded, identify and remove a
bounded batch of the oldest entries (such as roughly 10% of capacity), while
preserving the cache capacity limit and avoiding repeated full-cache eviction
work on the hot path.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffContinuousActions.swift`:
- Around line 4-13: Update DiffContinuousActions.swift:4-13 so asynchronous data
actions use async, cancellation-aware contracts that let callers own their
operation lifecycle. In DiffLiveScreen.swift:70-80, replace untracked
failure-recovery tasks with lifecycle-owned operations. In
DiffLiveScreen.swift:215-258, retain and cancel or otherwise bind file loading,
context expansion, baseline selection, filtering, and retry operations to
DiffLiveScreen’s lifecycle, avoiding fire-and-forget Task work.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffContinuousView.swift`:
- Around line 53-54: Move the full-diff work performed by highlightRequests out
of DiffContinuousView.body and into the data layer, such as DiffScreenStore,
computing it asynchronously and storing the result. Update the view to consume
the prepared requests without using .task(id: requests), so re-renders from
checkbox or collapse changes do not remap or compare the entire diff.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFixtureScreen.swift`:
- Around line 5-62: Exclude both debug-only fixture implementations from release
builds by wrapping the complete DiffFixtureScreen.swift DiffFixtureScreen
definition and DiffFixtureFactory.swift DiffFixtureFactory implementation in `#if`
DEBUG guards, covering all file contents that define these facilities.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLocalized.swift`:
- Around line 8-14: Update the format method to pass the current locale
explicitly when calling String(format:arguments:), ensuring numeric CVarArg
arguments use the user’s regional formatting while preserving the existing
localized string and argument flow.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffQuickNoteActions.swift`:
- Around line 6-18: Update DiffQuickNoteActions’ send contract and its host
implementation to return delivery success instead of Void, including a false
result when the session is unavailable. Propagate that outcome through
DiffQuickNoteSheet and dismiss only after send confirms delivery; keep
editInComposer unchanged.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffScreen.swift`:
- Around line 68-72: Update DiffScreenActions to expose callbacks for
selectBase, setIgnoreWhitespace, openQuickNote, and refresh, then have
DiffScreen forward those actions instead of supplying empty closures. Preserve
quickNoteAvailable as state while ensuring consumers can provide functional
handlers for all four view features.
- Around line 32-35: Update the DiffScreen state flow around _fileStates and
DiffPresentationBuilder().states so patchSet changes recompute presentation
states and refresh newly loaded diff bodies instead of relying on
State(initialValue:). Trigger this work via onChange(of: patchSet) or the
existing task/view-model lifecycle, and merge the computed states with current
fileStates to preserve local UI state such as isCollapsed. Avoid invoking the
expensive builder on every ordinary view update.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffScreenStore.swift`:
- Around line 91-104: Update selectBase and setIgnoreWhitespace, together with
the fetchSummary refresh flow, so baseSpec and ignoreWhitespace are not
committed until the corresponding summary succeeds. Keep pending query settings
separate from the committed configuration, then atomically commit the
configuration with summary and fileStates on success. On failure, roll back the
pending selection or disable operations that could use stale summary/fileStates,
ensuring one authoritative query state.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeProjection.swift`:
- Around line 26-50: Replace the repeated descendantStates scans in the
directory branch of append with a bottom-up aggregation: have append return
directory totals (additions, deletions, fileCount, and isViewed) while
processing each child once, and use those totals to build each
DiffTreeRowSnapshot. Remove the descendantStates dependency from this projection
path, preserving expansion behavior and the existing file-level summary/viewed
semantics.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeRowView.swift`:
- Around line 45-52: Update fileCountLabel to select explicit
diff.tree.fileCount.one and diff.tree.fileCount.other localization keys based on
row.fileCount, matching the pluralization pattern in
DiffSummaryHeaderView.swift; preserve the existing count interpolation and use
the singular key only for a count of one.
In `@Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffViewedStore.swift`:
- Around line 33-63: Replace JSONEncoder/JSONDecoder persistence in
DiffViewedStore’s setViewed and viewedKeys with UserDefaults
stringArray(forKey:) and set(_:forKey:), converting to and from Set<String> as
needed. Ensure all setViewed mutations remain confined to the `@MainActor`, and
avoid introducing caching unless it observes UserDefaults.didChangeNotification
to prevent stale state.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView`+Diffs.swift:
- Around line 60-69: Update diffAgentSession so it returns the explicitly chosen
ChatSessionDescriptor only when chosenChatSession is present and included in the
openable agent sessions; remove the fallback to openable.first and return nil
when no authoritative selection exists.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView`+Feedback.swift:
- Around line 9-16: Update openFeedbackComposerFromMenu and the feedback
submission flow around the referenced task to retain the submission Task, cancel
or invalidate any prior task before reopening, and associate each
submission/completion with a presentation or submission generation. Before
mutating UI state or dismissing the composer, verify the completion still
belongs to the current generation, preventing stale work from affecting a
reopened composer or enabling duplicate submissions.
In `@Packages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/CmuxDiffEngine.swift`:
- Around line 66-97: Update fileHunks and the DiffFilePage pagination contract
so the first page returns an opaque revision identifying the loaded diff
snapshot, and subsequent cursor requests must validate that revision before
applying the row offset. Reject stale cursors (or restart them according to the
existing API convention) rather than paginating against changed worktree
content; keep the revision derived from the same snapshot load as the hunks to
avoid a staleness window.
- Around line 187-205: Update resolveDefaultBranch so that when override is
provided, it is validated with verifiedObject and an invalid or unavailable
override immediately throws DiffEngineError.baselineUnavailable; only fall back
to symbolic or candidate branches when no override is supplied.
In
`@Packages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/GitDiffSnapshotLoader.swift`:
- Around line 35-50: Eliminate nested full-collection scans in
GitDiffSnapshotLoader: at
Packages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/GitDiffSnapshotLoader.swift:35-50,
precompute a [String: GitRawChange] dictionary keyed by path and use O(1) lookup
instead of changes.first; at
Packages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/GitDiffSnapshotLoader.swift:58-58,
precompute a Set<String> from trackedFiles.map(\.summary.path) before the loop
and use membership lookup instead of contains(where:).
- Line 83: Update the digest formatting in the SHA256 computation to use a fixed
hexadecimal lookup table and a preallocated output buffer instead of calling
String(format:) for each byte. Preserve the existing lowercase hexadecimal
digest value and joined string result.
In
`@Packages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/WorkingTreeFileReader.swift`:
- Around line 8-14: The regularFileData(path:) implementation must read from the
same validated file object rather than reopening its pathname. Replace the
lstat/Data(contentsOf:) flow with descriptor-relative traversal using openat and
O_NOFOLLOW, validate the opened descriptor via fstat as a regular file, and
construct/read Data from that descriptor while preserving nil for invalid or
unavailable paths.
- Around line 39-61: Escape the filename used in untrackedPatch when
constructing synthetic Git metadata, including both diff header paths and the
new-file path, using Git-compatible quoting so embedded newlines cannot create
parser-visible headers or hunks. Preserve the existing binary, addition-count,
and content-generation behavior.
In `@Sources/TerminalController`+MobileDiffs.swift:
- Around line 291-321: Update mobileDiffBaseline to decode the baseline store
once and select the latest matching record from that single structured snapshot,
rather than calling AppDelegate.latestAgentTurnDiffRepoRoot separately. Return
both repoRoot and baseCommit from the selected record, while preserving the
existing workspace, surface, session, and latest-capturedAt matching behavior.
- Line 5: Update the mobile diff logging statements that emit repository data,
including the success and failure paths near mobile diff processing, to use
private interpolation rather than .public. Keep the existing log messages and
behavior unchanged while ensuring repository filesystem paths and project names
are redacted in unified logging.
🪄 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: 3e591a79-92e1-4280-a319-0e088a74f7a5
⛔ Files ignored due to path filters (5)
Packages/iOS/CmuxDiffUI/Package.resolvedis excluded by!**/Package.resolvedPackages/iOS/CmuxMobileShellUI/Package.resolvedis excluded by!**/Package.resolvedcmux.xcworkspace/contents.xcworkspacedatais excluded by!**/*.xcworkspace/contents.xcworkspacedataios/cmux.xcworkspace/contents.xcworkspacedatais excluded by!**/*.xcworkspace/contents.xcworkspacedataios/cmuxPackage/Package.resolvedis excluded by!**/Package.resolved
📒 Files selected for processing (173)
Packages/iOS/CmuxDiffUI/Package.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/CodeHighlightBatcher.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/CodeHighlightCacheEntry.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/CodeHighlightCacheKey.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/CodeHighlightColor+SwiftUI.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/CodeHighlightColor.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/CodeHighlightRequest.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/CodeHighlightSpan.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/CodeHighlighting.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/Color+DiffAdaptive.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffClipboardWriter.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffCodeText.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffColorScheme+SwiftUI.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffColorScheme.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffComponentPreviews.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffContextExpansionRequest.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffContextSplicer.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffContinuousActions.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffContinuousView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffErrorBannerView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFailureView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFileBodyView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFileContent.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFileListRow.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFilePresentationState.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFileSectionHeader.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFileSectionSnapshot.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFileSnapshot.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFileTreeView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFixtureFactory.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFixtureScreen.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffHunkHeaderView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLanguageMapper.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLayoutOverride.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLayoutPreferenceStore.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLiveScreen.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLoadingView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLocalized.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffNavigationModel.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffPatchSet.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffPathLabel.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffPlaceholderKind.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffPlaceholderRow.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffPresentationBuilder.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffPromptFormatter.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffQuickNoteActions.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffQuickNoteSheet.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffQuickNoteTarget.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffQuickNoteTargetFactory.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffRenderMode.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffRowBuilder.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffRowIntralinePairer.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffRowKind.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffRowSnapshot.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffScreen.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffScreenActions.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffScreenErrorKind.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffScreenPhase.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffScreenStore.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffSplitCodeColumn.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffSplitRowView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffStatusBadge.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffStoreFailure.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffSummaryHeaderView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTheme.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeAccumulator.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeBuilder.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeNode.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeNodeKind.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeProgressView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeProjection.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeRowKind.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeRowSnapshot.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeRowView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffTreeScrollTargetResolver.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffUnifiedRowView.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffViewedButton.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffViewedStore.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/HighlightedCode.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/HighlighterSwiftCodeHighlighter.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/IntralineDiffer.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/IntralineSpan.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/IntralineToken.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/Resources/Localizable.xcstringsPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/SplitDiffPairer.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/SplitDiffRow.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/SplitDiffRowKind.swiftPackages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/TextRange.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/DiffLanguageMapperTests.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/DiffPromptFormatterTests.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/DiffRowBuilderTests.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/DiffScreenStoreTests.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/DiffTreeBuilderTests.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/DiffTreeScrollTargetResolverTests.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/DiffViewedStoreTests.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/FakeDiffResponse.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/FakeDiffTransportError.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/FakeMobileDiffsService.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/IntralineDifferTests.swiftPackages/iOS/CmuxDiffUI/Tests/CmuxDiffUITests/SplitDiffPairerTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffBaseInfo.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffBaseKind.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffBaseSpec.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffContextResponse.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffFileResponse.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffFileStatus.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffFileSummary.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffHunk.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffRow.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffRowKind.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffSummaryResponse.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileDiffTotals.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileDiffDTODecodeTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileDiffsService.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileDiffsServiceError.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileDiffsServing.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+Capabilities.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+Diffs.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileDiffsServiceTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileDiffsTestHost.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileDiffsTestRequest.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileDiffsTestRuntime.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileDiffsTestTransport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileDiffsTestTransportFactory.swiftPackages/iOS/CmuxMobileShellUI/Package.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDisplaySettings.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/Resources/Localizable.xcstringsPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+AgentChat.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+Diffs.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+Feedback.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+TerminalArtifacts.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDiffEntryGate.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileDisplaySettingsTests.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceDiffEntryGateTests.swiftPackages/macOS/CmuxDiffEngine/Package.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/CmuxDiffEngine.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffBaseInfo.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffBaseKind.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffBaseSpec.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffEngineError.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffFilePage.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffFileStatus.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffFileSummary.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffHunk.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffPageBuilder.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffRow.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffRowKind.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffSummary.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/DiffTotals.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/GitCommandExecutor.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/GitDiffFile.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/GitDiffSnapshot.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/GitDiffSnapshotLoader.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/GitNumstat.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/GitOutputParser.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/GitRawChange.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/ResolvedDiffBase.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/UnifiedDiffParser.swiftPackages/macOS/CmuxDiffEngine/Sources/CmuxDiffEngine/WorkingTreeFileReader.swiftPackages/macOS/CmuxDiffEngine/Tests/CmuxDiffEngineTests/CmuxDiffEngineErrorTests.swiftPackages/macOS/CmuxDiffEngine/Tests/CmuxDiffEngineTests/CmuxDiffEngineFileTests.swiftPackages/macOS/CmuxDiffEngine/Tests/CmuxDiffEngineTests/CmuxDiffEngineSummaryTests.swiftPackages/macOS/CmuxDiffEngine/Tests/CmuxDiffEngineTests/FakeCommandRunner.swiftPackages/macOS/CmuxDiffEngine/Tests/CmuxDiffEngineTests/FixtureRepository.swiftPackages/macOS/CmuxDiffEngine/Tests/CmuxDiffEngineTests/FixtureRepositoryError.swiftPackages/macOS/CmuxDiffEngine/Tests/CmuxDiffEngineTests/RecordingCommandRunner.swiftSources/Mobile/MobileHostService+Capabilities.swiftSources/TerminalController+MobileDiffs.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
Comment on lines
+9133
to
+9136
| D1FF00000000000000000001 /* XCLocalSwiftPackageReference "CmuxDiffEngine" */ = { | ||
| isa = XCLocalSwiftPackageReference; | ||
| relativePath = Packages/macOS/CmuxDiffEngine; | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Commit the root Package.resolved lockfile.
You are adding a new package reference (CmuxDiffEngine) to the Xcode project. As per path instructions, updates to Xcode-managed package references must include the root lockfile at cmux.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved. Relying on package-local resolution alone is not sufficient. Please ensure the updated workspace lockfile is committed.
🤖 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 `@cmux.xcodeproj/project.pbxproj` around lines 9133 - 9136, Commit the root
workspace SwiftPM lockfile at
project.xcworkspace/xcshareddata/swiftpm/Package.resolved alongside the new
CmuxDiffEngine XCLocalSwiftPackageReference, ensuring it contains the updated
resolved dependency state.
Source: Path instructions
Comment on lines
+50
to
+58
| private func insert(_ value: HighlightedCode, for key: CodeHighlightCacheKey) { | ||
| accessCounter &+= 1 | ||
| cache[key] = CodeHighlightCacheEntry(value: value, lastAccess: accessCounter) | ||
| guard cache.count > capacity, | ||
| let leastRecent = cache.min(by: { $0.value.lastAccess < $1.value.lastAccess })?.key else { | ||
| return | ||
| } | ||
| cache.removeValue(forKey: leastRecent) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Amortize LRU cache eviction to prevent O(N) scaling per insertion.
Finding the minimum value in the dictionary takes O(N) time. Once the cache reaches its capacity, every single insertion triggers this O(N) scan. During a large file load where thousands of lines are highlighted, this results in O(L × N) complexity on a hot path, violating the algorithmic complexity guidelines.
Consider evicting a batch of the oldest entries at once (e.g., 10%) to amortize the eviction cost. As per path instructions, avoid repeated hot-path sorting, filtering, or unbenchmarked slower algorithms for scalable collections.
⚡ Proposed fix to amortize eviction
private func insert(_ value: HighlightedCode, for key: CodeHighlightCacheKey) {
accessCounter &+= 1
cache[key] = CodeHighlightCacheEntry(value: value, lastAccess: accessCounter)
- guard cache.count > capacity,
- let leastRecent = cache.min(by: { $0.value.lastAccess < $1.value.lastAccess })?.key else {
+ guard cache.count > capacity else {
return
}
- cache.removeValue(forKey: leastRecent)
+ // Evict the oldest 10% to amortize the scan cost
+ let evictCount = max(1, capacity / 10)
+ let oldestKeys = cache
+ .sorted(by: { $0.value.lastAccess < $1.value.lastAccess })
+ .prefix(evictCount)
+ .map(\.key)
+
+ for oldestKey in oldestKeys {
+ cache.removeValue(forKey: oldestKey)
+ }
}📝 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.
Suggested change
| private func insert(_ value: HighlightedCode, for key: CodeHighlightCacheKey) { | |
| accessCounter &+= 1 | |
| cache[key] = CodeHighlightCacheEntry(value: value, lastAccess: accessCounter) | |
| guard cache.count > capacity, | |
| let leastRecent = cache.min(by: { $0.value.lastAccess < $1.value.lastAccess })?.key else { | |
| return | |
| } | |
| cache.removeValue(forKey: leastRecent) | |
| } | |
| private func insert(_ value: HighlightedCode, for key: CodeHighlightCacheKey) { | |
| accessCounter &+= 1 | |
| cache[key] = CodeHighlightCacheEntry(value: value, lastAccess: accessCounter) | |
| guard cache.count > capacity else { | |
| return | |
| } | |
| // Evict the oldest 10% to amortize the scan cost | |
| let evictCount = max(1, capacity / 10) | |
| let oldestKeys = cache | |
| .sorted(by: { $0.value.lastAccess < $1.value.lastAccess }) | |
| .prefix(evictCount) | |
| .map(\.key) | |
| for oldestKey in oldestKeys { | |
| cache.removeValue(forKey: oldestKey) | |
| } | |
| } |
🤖 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/CmuxDiffUI/Sources/CmuxDiffUI/CodeHighlightBatcher.swift` around
lines 50 - 58, Update CodeHighlightBatcher.insert(_:for:) to amortize LRU
eviction instead of scanning for and removing only one entry on every
over-capacity insertion. When capacity is exceeded, identify and remove a
bounded batch of the oldest entries (such as roughly 10% of capacity), while
preserving the cache capacity limit and avoiding repeated full-cache eviction
work on the hot path.
Source: Path instructions
Comment on lines
+4
to
+13
| let loadFile: @MainActor @Sendable (String, Bool) -> Void | ||
| let expandContext: @MainActor @Sendable (DiffContextExpansionRequest) -> Void | ||
| let toggleViewed: @MainActor @Sendable (String) -> Void | ||
| let toggleCollapsed: @MainActor @Sendable (String) -> Void | ||
| let collapseAll: @MainActor @Sendable () -> Void | ||
| let selectBase: @MainActor @Sendable (MobileDiffBaseKind) -> Void | ||
| let setIgnoreWhitespace: @MainActor @Sendable (Bool) -> Void | ||
| let openQuickNote: @MainActor @Sendable (DiffQuickNoteTarget) -> Void | ||
| let quickNoteAvailable: Bool | ||
| let refresh: @MainActor @Sendable () async -> Void |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Give asynchronous diff actions explicit lifecycle ownership. The synchronous callback contract forces the live screen to launch meaningful RPC operations as untracked tasks.
Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffContinuousActions.swift#L4-L13: make asynchronous data actions async and expose cancellation/caller ownership.Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLiveScreen.swift#L70-L80: replace untracked failure-recovery tasks with lifecycle-owned operations.Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLiveScreen.swift#L215-L258: retain/cancel or otherwise bind file, context, baseline, filtering, and retry operations to the screen lifecycle.
As per coding guidelines, “Do not create fire-and-forget Task { ... } work with meaningful lifecycle unless it is stored, cancellable, or tied to a caller-owned operation.”
📍 Affects 2 files
Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffContinuousActions.swift#L4-L13(this comment)Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLiveScreen.swift#L70-L80Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffLiveScreen.swift#L215-L258
🤖 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/CmuxDiffUI/Sources/CmuxDiffUI/DiffContinuousActions.swift`
around lines 4 - 13, Update DiffContinuousActions.swift:4-13 so asynchronous
data actions use async, cancellation-aware contracts that let callers own their
operation lifecycle. In DiffLiveScreen.swift:70-80, replace untracked
failure-recovery tasks with lifecycle-owned operations. In
DiffLiveScreen.swift:215-258, retain and cancel or otherwise bind file loading,
context expansion, baseline selection, filtering, and retry operations to
DiffLiveScreen’s lifecycle, avoiding fire-and-forget Task work.
Source: Coding guidelines
Comment on lines
+53
to
+54
| var body: some View { | ||
| let requests = highlightRequests(for: DiffColorScheme(colorScheme)) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid repeated O(N) mapping of the entire diff on every render.
Calling highlightRequests here processes every row of all files to build a massive array on every body evaluation. Furthermore, .task(id: requests) performs an O(N) equality check on this large array whenever the view re-renders (e.g., when a user toggles a viewed checkbox or collapses a file).
For workloads like large diffs, this repeated hot-path mapping will degrade main-thread rendering performance. As per path instructions, avoid repeated sort/filter/map work in hot paths for scalable user data. Consider computing these requests asynchronously in the data layer (e.g., DiffScreenStore), or chunking the highlighting per-file or per-visible-row so the rendering complexity isn't tied to the total diff size.
🤖 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/CmuxDiffUI/Sources/CmuxDiffUI/DiffContinuousView.swift` around
lines 53 - 54, Move the full-diff work performed by highlightRequests out of
DiffContinuousView.body and into the data layer, such as DiffScreenStore,
computing it asynchronously and storing the result. Update the view to consume
the prepared requests without using .task(id: requests), so re-renders from
checkbox or collapse changes do not remap or compare the entire diff.
Source: Path instructions
Comment on lines
+5
to
+62
| public struct DiffFixtureScreen: View { | ||
| @State private var renderMode: DiffRenderMode = .unified | ||
| private let defaults: UserDefaults | ||
| private let highlighter = HighlighterSwiftCodeHighlighter() | ||
|
|
||
| /// Localized label used by the DEBUG settings entry. | ||
| public static var settingsLabel: String { | ||
| DiffLocalized().string("diff.fixture.settingsLabel", defaultValue: "Diff rendering (ndv2)") | ||
| } | ||
|
|
||
| /// Creates the fixture harness with injected viewed-state persistence. | ||
| /// - Parameter defaults: Defaults suite used by the fixture's viewed store. | ||
| public init(defaults: UserDefaults) { | ||
| self.defaults = defaults | ||
| } | ||
|
|
||
| /// The full-screen fixture surface and unified/split toggle. | ||
| public var body: some View { | ||
| DiffScreen( | ||
| patchSet: DiffFixtureFactory().patchSet(), | ||
| renderMode: $renderMode, | ||
| viewedStore: DiffViewedStore(defaults: defaults), | ||
| highlighter: highlighter, | ||
| actions: DiffScreenActions( | ||
| loadLargeFile: { _ in }, | ||
| retryFile: { _ in }, | ||
| expandContext: { _ in } | ||
| ) | ||
| ) | ||
| .navigationTitle(navigationTitle) | ||
| .toolbar { | ||
| ToolbarItem(placement: .principal) { | ||
| Picker(modeLabel, selection: $renderMode) { | ||
| Text(unifiedLabel).tag(DiffRenderMode.unified) | ||
| Text(splitLabel).tag(DiffRenderMode.split) | ||
| } | ||
| .pickerStyle(.segmented) | ||
| .frame(maxWidth: 220) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private var navigationTitle: String { | ||
| DiffLocalized().string("diff.fixture.title", defaultValue: "Diff rendering") | ||
| } | ||
|
|
||
| private var modeLabel: String { | ||
| DiffLocalized().string("diff.mode.label", defaultValue: "Layout") | ||
| } | ||
|
|
||
| private var unifiedLabel: String { | ||
| DiffLocalized().string("diff.mode.unified", defaultValue: "Unified") | ||
| } | ||
|
|
||
| private var splitLabel: String { | ||
| DiffLocalized().string("diff.mode.split", defaultValue: "Split") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider guarding debug fixture facilities with #if DEBUG.
These files implement a debug-only fixture harness located in the production Sources/ path. As per path instructions, debug and fixture facilities should not be shipped in production source without gating them, unless they are intended to be a permanent, user-facing app feature. Consider wrapping the file contents in #if DEBUG to cleanly exclude them from release binaries.
Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFixtureScreen.swift#L5-L62: Wrap theDiffFixtureScreenimplementation in#if DEBUG.Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFixtureFactory.swift#L3-L255: Wrap theDiffFixtureFactoryimplementation in#if DEBUG.
📍 Affects 2 files
Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFixtureScreen.swift#L5-L62(this comment)Packages/iOS/CmuxDiffUI/Sources/CmuxDiffUI/DiffFixtureFactory.swift#L3-L255
🤖 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/CmuxDiffUI/Sources/CmuxDiffUI/DiffFixtureScreen.swift` around
lines 5 - 62, Exclude both debug-only fixture implementations from release
builds by wrapping the complete DiffFixtureScreen.swift DiffFixtureScreen
definition and DiffFixtureFactory.swift DiffFixtureFactory implementation in `#if`
DEBUG guards, covering all file contents that define these facilities.
Source: Path instructions
| isBinary: Bool, | ||
| patch: Data | ||
| ) -> GitDiffFile { | ||
| let digest = SHA256.hash(data: patch).map { String(format: "%02x", $0) }.joined() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Replace per-byte String(format:) with a lookup table.
Using String(format:) for every byte of the SHA256 digest creates heavy per-element allocations on a hot path. As per coding guidelines, use a fixed lookup table and a preallocated buffer to format strings without per-byte overhead.
⚡ Proposed fix using a preallocated buffer
- let digest = SHA256.hash(data: patch).map { String(format: "%02x", $0) }.joined()
+ let hexAlphabet = Array("0123456789abcdef")
+ var digestString = ""
+ digestString.reserveCapacity(64)
+ for byte in SHA256.hash(data: patch) {
+ digestString.append(hexAlphabet[Int(byte >> 4)])
+ digestString.append(hexAlphabet[Int(byte & 0xF)])
+ }
+ let digest = digestString📝 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.
Suggested change
| let digest = SHA256.hash(data: patch).map { String(format: "%02x", $0) }.joined() | |
| let hexAlphabet = Array("0123456789abcdef") | |
| var digestString = "" | |
| digestString.reserveCapacity(64) | |
| for byte in SHA256.hash(data: patch) { | |
| digestString.append(hexAlphabet[Int(byte >> 4)]) | |
| digestString.append(hexAlphabet[Int(byte & 0xF)]) | |
| } | |
| let digest = digestString |
🤖 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/CmuxDiffEngine/Sources/CmuxDiffEngine/GitDiffSnapshotLoader.swift`
at line 83, Update the digest formatting in the SHA256 computation to use a
fixed hexadecimal lookup table and a preallocated output buffer instead of
calling String(format:) for each byte. Preserve the existing lowercase
hexadecimal digest value and joined string result.
Source: Coding guidelines
Comment on lines
+8
to
+14
| func regularFileData(path: String) throws -> Data? { | ||
| let url = try absoluteURL(path: path) | ||
| var metadata = stat() | ||
| guard lstat(url.path, &metadata) == 0 else { return nil } | ||
| guard metadata.st_mode & S_IFMT == S_IFREG else { return nil } | ||
| return try Data(contentsOf: url, options: [.mappedIfSafe]) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Open the validated file descriptor instead of reopening the pathname.
A repository process can replace the checked file or an ancestor between resolvingSymlinksInPath/lstat and Data(contentsOf:), causing the RPC to read a file outside the repository. Traverse with openat/O_NOFOLLOW, verify the opened descriptor with fstat, and read from that descriptor.
Also applies to: 64-78
🤖 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/CmuxDiffEngine/Sources/CmuxDiffEngine/WorkingTreeFileReader.swift`
around lines 8 - 14, The regularFileData(path:) implementation must read from
the same validated file object rather than reopening its pathname. Replace the
lstat/Data(contentsOf:) flow with descriptor-relative traversal using openat and
O_NOFOLLOW, validate the opened descriptor via fstat as a regular file, and
construct/read Data from that descriptor while preserving nil for invalid or
unavailable paths.
Comment on lines
+39
to
+61
| func untrackedPatch(path: String, data: Data) -> (patch: Data, additions: Int, isBinary: Bool) { | ||
| let isBinary = data.contains(0) | ||
| if isBinary { | ||
| let header = "diff --git a/\(path) b/\(path)\nnew file mode 100644\nBinary files /dev/null and b/\(path) differ\n" | ||
| return (Data(header.utf8), 0, true) | ||
| } | ||
| let text = String(decoding: data, as: UTF8.self) | ||
| var rows = text.split(separator: "\n", omittingEmptySubsequences: false).map(String.init) | ||
| let hasTrailingNewline = data.last == 0x0A | ||
| if hasTrailingNewline, rows.last == "" { | ||
| rows.removeLast() | ||
| } | ||
| let additions = data.isEmpty ? 0 : rows.count | ||
| var patch = "diff --git a/\(path) b/\(path)\nnew file mode 100644\n--- /dev/null\n+++ b/\(path)\n" | ||
| if additions > 0 { | ||
| patch += "@@ -0,0 +1,\(additions) @@\n" | ||
| patch += rows.map { "+" + $0 }.joined(separator: "\n") | ||
| patch += "\n" | ||
| if !hasTrailingNewline { | ||
| patch += "\\ No newline at end of file\n" | ||
| } | ||
| } | ||
| return (Data(patch.utf8), additions, false) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Escape filenames before constructing synthetic patch metadata.
macOS filenames may contain newlines. Interpolating path directly lets an untracked filename inject @@ … @@ headers that UnifiedDiffParser treats as real hunks, corrupting the rendered diff. Encode metadata paths using Git-compatible quoting or construct untracked hunks structurally.
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 45-45: Prefer failable String(bytes:encoding:) initializer when converting Data to String
(optional_data_string_conversion)
🤖 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/CmuxDiffEngine/Sources/CmuxDiffEngine/WorkingTreeFileReader.swift`
around lines 39 - 61, Escape the filename used in untrackedPatch when
constructing synthetic Git metadata, including both diff header paths and the
new-file path, using Git-compatible quoting so embedded newlines cannot create
parser-visible headers or hunks. Preserve the existing binary, addition-count,
and content-generation behavior.
| import Foundation | ||
| import os | ||
|
|
||
| private let mobileDiffLog = Logger(subsystem: "dev.cmux", category: "mobile-diffs") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep repository paths private in unified logging.
repository contains user filesystem paths and project names but is emitted with .public on both success and failure. Use .private or log a non-sensitive identifier instead.
As per coding guidelines, dynamic personal data in production logs must remain redacted or private.
Also applies to: 264-266, 285-288
🤖 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`+MobileDiffs.swift at line 5, Update the mobile
diff logging statements that emit repository data, including the success and
failure paths near mobile diff processing, to use private interpolation rather
than .public. Keep the existing log messages and behavior unchanged while
ensuring repository filesystem paths and project names are redacted in unified
logging.
Source: Coding guidelines
Comment on lines
+291
to
+321
| nonisolated private static func mobileDiffBaseline( | ||
| storeURL: URL, | ||
| workspaceID: UUID, | ||
| surfaceID: UUID, | ||
| sessionID: String | ||
| ) -> (repository: String, commit: String)? { | ||
| guard let repository = AppDelegate.latestAgentTurnDiffRepoRoot( | ||
| storeURL: storeURL, | ||
| workspaceId: workspaceID, | ||
| surfaceId: surfaceID, | ||
| sessionId: sessionID | ||
| ), | ||
| let data = try? Data(contentsOf: storeURL), | ||
| let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any], | ||
| let records = object["records"] as? [[String: Any]] else { | ||
| return nil | ||
| } | ||
| let workspaceKey = workspaceID.uuidString.lowercased() | ||
| let surfaceKey = surfaceID.uuidString.lowercased() | ||
| let candidates = records.compactMap { record -> (commit: String, capturedAt: TimeInterval)? in | ||
| guard AppDelegate.normalizedOpenDiffViewerIdentifier(record["workspaceId"] as? String) == workspaceKey, | ||
| AppDelegate.normalizedOpenDiffViewerIdentifier(record["surfaceId"] as? String) == surfaceKey, | ||
| AppDelegate.normalizedOpenDiffViewerSessionId(record["sessionId"] as? String) == sessionID, | ||
| AppDelegate.normalizedOpenDiffViewerPath(record["repoRoot"] as? String) == repository, | ||
| let commit = AppDelegate.normalizedOpenDiffViewerIdentifier(record["baseCommit"] as? String) else { | ||
| return nil | ||
| } | ||
| return (commit, (record["capturedAt"] as? NSNumber)?.doubleValue ?? 0) | ||
| } | ||
| guard let latest = candidates.max(by: { $0.capturedAt < $1.capturedAt }) else { return nil } | ||
| return (repository, latest.commit) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Select the last-turn repository and commit from one store snapshot.
latestAgentTurnDiffRepoRoot reads the baseline store, then this helper reads it again to select the commit. A write between those reads can return a stale or mismatched baseline. Decode once, select the latest matching record, and return both repoRoot and baseCommit from that record.
As per path instructions, correctness-critical baseline selection must use one reliable structured source.
🤖 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`+MobileDiffs.swift around lines 291 - 321, Update
mobileDiffBaseline to decode the baseline store once and select the latest
matching record from that single structured snapshot, rather than calling
AppDelegate.latestAgentTurnDiffRepoRoot separately. Return both repoRoot and
baseCommit from the selected record, while preserving the existing workspace,
surface, session, and latest-capturedAt matching behavior.
Sources: Coding guidelines, Path instructions
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swift from main (#8270) fails to compile under Xcode 26.6, which marks the NSImage(size:flipped:) drawing closure so property access requires explicit self. Main-side fix needed too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added
the
stale-revisit
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Native SwiftUI diff viewer for cmux iOS mirroring the GitHub web "Files changed" information design, fed by live git data from the paired Mac over a new one-frame-paged RPC namespace. Independent implementation; no WKWebView, no code shared with #8107 or the closed #8014.
Mac side: new
Packages/macOS/CmuxDiffEngine(git via injectedCommandRunning, -z parsing, quotepath off, unborn-HEAD empty-tree base, R###/C### pathspecs, in-process untracked stats, batch numstat, isLarge gating, SHA256 patchDigest per file) behindmobile.workspace.diffs.{summary,file,context}with capabilityworkspace.diffs.v1; one prefix case inmobileHostHandleRPC, logic inTerminalController+MobileDiffs.swift.iOS side: DTOs in
CmuxMobileRPC;MobileDiffsServicevended byMobileShellComposite.makeDiffsService()riding the shell's live authenticated client (never a second transport); newPackages/iOS/CmuxDiffUIrendering package — summary header (files, +/− totals, viewed x of N), sticky per-file headers with collapse/viewed/kebab, dual line-number gutters, add/del row tints with darker word-level intraline spans, blue hunk headers with expand up/down/all wired to the context RPC, binary/renamed/untracked/no-newline/large-diff states, unified and split rendering (orientation-driven + manual override), collapsible file tree with GitHub-style single-child chain collapsing, HighlighterSwift (hljs) syntax highlighting behind a protocol seam with LRU cache, device-local viewed store keyed workspace+path+patchDigest, base picker (Working tree / Last agent turn / Branch base with baseline-missing fallback), whitespace-ignore toggle, send-to-agent note sheet (hunk header + file kebab) routing through the existing chat send path with an edit-in-composer escape, workspace toolbar entry gated on capability+connection, and both nav models (files-first tree / diff-first drawer) behind a Developer flag. All user-facing strings localized en+ja. Diff Lists pindefaultMinListRowHeightto 1 with zero row spacing for GitHub density.Tests: engine 16, DiffUI 33, service 6, DTO 3 (plus decoding/error-mapping suites); all package suites green; iOS Simulator SDK builds green per slice. Known out-of-scope failure:
lint-ios-package-conventions.shtrips on the pre-existingCmuxIrohTransport/CmxIrohTCPFirstActivationnamespace violation from #7908.Tag
ndv2(macOS + iOS simulator ndv2-sim). Dogfood evidence in the PR thread.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a native SwiftUI diff viewer for iOS with a GitHub-style Files changed UI over
mobile.workspace.diffs.{summary,file,context}. Supports unified/split layouts, a collapsible file tree, syntax highlighting, viewed state, base selection, whitespace ignore, and hunk context expansion.New Features
CmuxDiffUIwith continuous diffs (unified/split), dual gutters, intraline word highlights, hunk headers, and binary/large/rename-only states.workspace.diffs.v1) via the existing authenticated client inCmuxMobileShell(no new transport).HighlighterSwift.Bug Fixes
selfin anNSImagetint closure in the AppKit sidebar workspace row view.Written for commit e61f235. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes