Add revertable diff interactions and hover affordance controls#5
Open
iSapozhnik wants to merge 8 commits intomainfrom
Open
Add revertable diff interactions and hover affordance controls#5iSapozhnik wants to merge 8 commits intomainfrom
iSapozhnik wants to merge 8 commits intomainfrom
Conversation
|
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 token-mode revert support: a resolver that finds revert candidates and builds revert actions, AppKit hover affordances and hit‑testing, SwiftUI binding support to apply actions, layout metadata for segment mapping, new public action and stroke‑style types, engine whitespace adjustments, and extensive tests. Changes
Sequence DiagramsequenceDiagram
actor User
participant View as NSTextDiffView
participant Resolver as DiffRevertActionResolver
participant Coord as Coordinator
User->>View: Hover over diff
View->>View: hit-test & show affordance
User->>View: Click revert icon
View->>Resolver: action(from: candidate, updated:)
Resolver-->>View: TextDiffRevertAction
View->>Coord: handle(action)
Coord->>Coord: update Binding (resultingUpdated)
Coord-->>View: invoke onRevertAction/callback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
iSapozhnik
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (9)
Tests/TextDiffTests/NSTextDiffViewTests.swift (1)
241-266: Inconsistent early-return patterns across hover tests.Lines 255–257 silently pass the test if no action centers are found, while other tests (lines 281, 336) use
Issue.record(...)before returning. Consider usingIssue.recordconsistently (ortry#require``) so a missing action center doesn't silently pass the test.♻️ Suggested: use `Issue.record` or `try `#require`` consistently
let centers = view._testingActionCenters() `#expect`(centers.count == 1) - guard let center = centers.first else { - return - } + guard let center = centers.first else { + Issue.record("Expected at least one action center") + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/TextDiffTests/NSTextDiffViewTests.swift` around lines 241 - 266, The test hoverLeaveSchedulesDismissNotImmediate currently returns early if no action centers are found, silently passing; update the early-return to record a test failure instead by replacing the guard-let that checks view._testingActionCenters() in hoverLeaveSchedulesDismissNotImmediate with a failure recording (e.g., call Issue.record(...) with a clear message or use try `#require` to assert a center is present) so the test fails when centers.isEmpty rather than passing silently; keep the rest of the test logic (calls to view._testingUpdateHover and subsequent expects) unchanged.Sources/TextDiff/AppKit/DiffTokenLayouter.swift (1)
186-188: Pre-existing quadratic text measurement noted.This
O(n²)approach (re-measuring the entire accumulated line on each token) is acknowledged by the TODO. The new performance test in this PR captures baselines but doesn't enforce thresholds, so a regression here would go unnoticed.Consider tracking this as a separate issue so it doesn't get lost.
Do you want me to open an issue to track replacing this with incremental width measurement?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/DiffTokenLayouter.swift` around lines 186 - 188, The code currently appends each token to lineText and calls lineText.size(withAttributes: [.font: font]) inside DiffTokenLayouter.swift (producing combinedWidth), causing O(n²) layout cost; change this to maintain an accumulatedWidth Double/CGFloat and for each token measure only that token's width (e.g., use NSString(token).size(withAttributes: [.font: font]).width or equivalent) and add it to accumulatedWidth instead of re-measuring the whole lineText, updating any logic that reads combinedWidth to use accumulatedWidth; if kerning/spacing needs per-token adjustments, compute those per-token and add them incrementally rather than recomputing the entire line.Sources/TextDiff/AppKit/DiffRevertActionResolver.swift (2)
220-254: The kind mapping can be simplified.The
DiffRevertCandidateKind→TextDiffRevertActionKindswitch (lines 237–244) maps identical case names 1:1. This is fine for clarity, but if you prefer brevity:♻️ Optional: Simplify kind mapping
- let actionKind: TextDiffRevertActionKind - switch candidate.kind { - case .singleInsertion: - actionKind = .singleInsertion - case .singleDeletion: - actionKind = .singleDeletion - case .pairedReplacement: - actionKind = .pairedReplacement - } + let actionKind: TextDiffRevertActionKind = { + switch candidate.kind { + case .singleInsertion: return .singleInsertion + case .singleDeletion: return .singleDeletion + case .pairedReplacement: return .pairedReplacement + } + }()Or add a computed property on
DiffRevertCandidateKindto convert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/DiffRevertActionResolver.swift` around lines 220 - 254, The switch in action(from:updated:) mapping candidate.kind to a TextDiffRevertActionKind is a 1:1 identity mapping; replace the verbose switch with a direct conversion (either use a TextDiffRevertActionKind initializer/initializer from rawValue if enums share raw values, or add a computed property on DiffRevertCandidate.Kind like asTextDiffRevertActionKind that returns the corresponding TextDiffRevertActionKind) and use that to set actionKind, keeping all other return fields the same.
80-162: Candidate generation logic is sound.The paired replacement detection correctly identifies adjacent delete→insert lexical pairs and advances by 2. Single insertions and deletions fall through cleanly. One minor observation:
originalCursorandupdatedCursoronIndexedSegmentare redundant withoriginalRange.locationandupdatedRange.locationrespectively (they're always equal). Consider dropping the cursor fields or making them computed.♻️ Optional: Remove redundant cursor fields
struct IndexedSegment { let segmentIndex: Int let segment: DiffSegment - let originalCursor: Int - let updatedCursor: Int let originalRange: NSRange let updatedRange: NSRange + + var originalCursor: Int { originalRange.location } + var updatedCursor: Int { updatedRange.location } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/DiffRevertActionResolver.swift` around lines 80 - 162, The IndexedSegment struct contains redundant originalCursor and updatedCursor fields that duplicate originalRange.location and updatedRange.location; remove these stored cursor properties (or make them computed properties returning originalRange.location/updatedRange.location) in the IndexedSegment definition and update all usages (for example in DiffRevertActionResolver.candidates where updatedCursor is used to build NSRange and any other places referencing originalCursor/updatedCursor) to read from originalRange.location/updatedRange.location (or the new computed properties), then run builds/tests to ensure no remaining references.Sources/TextDiff/AppKit/NSTextDiffView.swift (4)
96-101: Remove commented-out code from color constants.Lines 97–98 have commented-out alternative colors that look like leftover experimentation. These should be cleaned up before merging.
🧹 Proposed cleanup
- private let hoverButtonFillColor = NSColor.black//NSColor.windowBackgroundColor.withAlphaComponent(0.95) - private let hoverButtonStrokeColor = NSColor.clear//NSColor.controlAccentColor.withAlphaComponent(0.8) + private let hoverButtonFillColor = NSColor.black + private let hoverButtonStrokeColor = NSColor.clear🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/NSTextDiffView.swift` around lines 96 - 101, Remove the leftover commented-out alternative color expressions from the color constant declarations: clean up hoverButtonFillColor and hoverButtonStrokeColor by deleting the inline comments (e.g., "//NSColor.windowBackgroundColor.withAlphaComponent(0.95)" and "//NSColor.controlAccentColor.withAlphaComponent(0.8)") so the constants read only the intended values (hoverButtonFillColor and hoverButtonStrokeColor) while leaving hoverOutlineColor, hoverIconName, hoverButtonSize, and hoverButtonGap unchanged.
434-454:handleIconClickconsumes the click even when action resolution fails.If
DiffRevertActionResolver.action(from:updated:)returnsnil(line 450), the method still returnstrue(line 453), swallowing the click event. This is likely intentional (the click was on a valid target area), but worth confirming — the user would see no effect from their click.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/NSTextDiffView.swift` around lines 434 - 454, The method handleIconClick currently returns true even when DiffRevertActionResolver.action(from:updated:) yields nil, thus consuming clicks with no effect; update handleIconClick so that after resolving candidate (using interactionContext(for:), actionIDForHitTarget(at:layout:context:) and context.candidatesByID[actionID]) you only call onRevertAction and return true when the resolver returns a non-nil DiffRevertAction, otherwise return false to let the click propagate (i.e., change the branch around DiffRevertActionResolver.action(from:updated:) to return false when it returns nil).
422-432: Cursor push/pop could leak if the view is deallocated while hover is active.
NSCursor.pop()is never called if the view is torn down whileisPointingHandCursorActiveistrue. Consider adding adeinitto clean up.🛡️ Proposed deinit
+ deinit { + if isPointingHandCursorActive { + NSCursor.pop() + } + pendingHoverDismissWorkItem?.cancel() + } + override public func updateTrackingAreas() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/NSTextDiffView.swift` around lines 422 - 432, The view can leak the cursor stack if isPointingHandCursorActive remains true when the view is deallocated; add a deinit in NSTextDiffView that checks isPointingHandCursorActive and calls NSCursor.pop() if needed to restore the cursor stack, then set isPointingHandCursorActive = false; ensure this cleanup complements the existing setPointingHandCursorActive(_:) logic so pop is always called when the view goes away.
389-420: Hover dismiss scheduling is functionally correct but slightly misleading.The
DispatchWorkItemis stored primarily as a non-nil flag, while the actual staleness guard is thehoverDismissGenerationcounter (since.perform()insideasyncAfterdoesn't honor.cancel()). This works correctly, but the.cancel()call incancelPendingHoverDismissgives a false impression of built-in cancellation being leveraged.Not blocking — the generation-based guard is reliable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/NSTextDiffView.swift` around lines 389 - 420, The code schedules hover dismissal using hoverDismissGeneration as the real staleness guard, but cancelPendingHoverDismiss currently calls .cancel() on pendingHoverDismissWorkItem which is misleading because asyncAfter + .perform() ignores cancel; update cancelPendingHoverDismiss to stop relying on DispatchWorkItem.cancel(): increment hoverDismissGeneration (or set pendingHoverDismissWorkItem = nil) to invalidate the scheduled dismiss and clear pendingHoverDismissWorkItem, and remove or avoid calling .cancel(); touch the functions/vars scheduleHoverDismiss, cancelPendingHoverDismiss, hoverDismissGeneration, pendingHoverDismissWorkItem, hoverDismissDelay and clearHoverStateNow to ensure generation-based cancellation is used consistently.Tests/TextDiffTests/NSTextDiffSnapshotTests.swift (1)
118-131: Consider asserting the return value for the negative test.In
testCharacterModeDoesNotShowAffordance,_testingSetHoveredFirstRevertAction()should returnfalsebecause character mode produces no candidates. Asserting this would make the test self-documenting and catch regressions where candidates are unexpectedly generated.💡 Proposed improvement
configureView: { view in view.isRevertActionsEnabled = true - _ = view._testingSetHoveredFirstRevertAction() + XCTAssertFalse(view._testingSetHoveredFirstRevertAction()) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/TextDiffTests/NSTextDiffSnapshotTests.swift` around lines 118 - 131, In testCharacterModeDoesNotShowAffordance, assert that calling view._testingSetHoveredFirstRevertAction() returns false (since character mode produces no candidates) by capturing its return value inside the configureView closure and using an assertion like XCTAssertFalse; update the configureView block in the test to verify the boolean result from _testingSetHoveredFirstRevertAction() so the negative case is explicit and will fail if candidates are generated unexpectedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/TextDiff/AppKit/DiffRevertActionResolver.swift`:
- Around line 220-254: The switch in action(from:updated:) mapping
candidate.kind to a TextDiffRevertActionKind is a 1:1 identity mapping; replace
the verbose switch with a direct conversion (either use a
TextDiffRevertActionKind initializer/initializer from rawValue if enums share
raw values, or add a computed property on DiffRevertCandidate.Kind like
asTextDiffRevertActionKind that returns the corresponding
TextDiffRevertActionKind) and use that to set actionKind, keeping all other
return fields the same.
- Around line 80-162: The IndexedSegment struct contains redundant
originalCursor and updatedCursor fields that duplicate originalRange.location
and updatedRange.location; remove these stored cursor properties (or make them
computed properties returning originalRange.location/updatedRange.location) in
the IndexedSegment definition and update all usages (for example in
DiffRevertActionResolver.candidates where updatedCursor is used to build NSRange
and any other places referencing originalCursor/updatedCursor) to read from
originalRange.location/updatedRange.location (or the new computed properties),
then run builds/tests to ensure no remaining references.
In `@Sources/TextDiff/AppKit/DiffTokenLayouter.swift`:
- Around line 186-188: The code currently appends each token to lineText and
calls lineText.size(withAttributes: [.font: font]) inside
DiffTokenLayouter.swift (producing combinedWidth), causing O(n²) layout cost;
change this to maintain an accumulatedWidth Double/CGFloat and for each token
measure only that token's width (e.g., use NSString(token).size(withAttributes:
[.font: font]).width or equivalent) and add it to accumulatedWidth instead of
re-measuring the whole lineText, updating any logic that reads combinedWidth to
use accumulatedWidth; if kerning/spacing needs per-token adjustments, compute
those per-token and add them incrementally rather than recomputing the entire
line.
In `@Sources/TextDiff/AppKit/NSTextDiffView.swift`:
- Around line 96-101: Remove the leftover commented-out alternative color
expressions from the color constant declarations: clean up hoverButtonFillColor
and hoverButtonStrokeColor by deleting the inline comments (e.g.,
"//NSColor.windowBackgroundColor.withAlphaComponent(0.95)" and
"//NSColor.controlAccentColor.withAlphaComponent(0.8)") so the constants read
only the intended values (hoverButtonFillColor and hoverButtonStrokeColor) while
leaving hoverOutlineColor, hoverIconName, hoverButtonSize, and hoverButtonGap
unchanged.
- Around line 434-454: The method handleIconClick currently returns true even
when DiffRevertActionResolver.action(from:updated:) yields nil, thus consuming
clicks with no effect; update handleIconClick so that after resolving candidate
(using interactionContext(for:), actionIDForHitTarget(at:layout:context:) and
context.candidatesByID[actionID]) you only call onRevertAction and return true
when the resolver returns a non-nil DiffRevertAction, otherwise return false to
let the click propagate (i.e., change the branch around
DiffRevertActionResolver.action(from:updated:) to return false when it returns
nil).
- Around line 422-432: The view can leak the cursor stack if
isPointingHandCursorActive remains true when the view is deallocated; add a
deinit in NSTextDiffView that checks isPointingHandCursorActive and calls
NSCursor.pop() if needed to restore the cursor stack, then set
isPointingHandCursorActive = false; ensure this cleanup complements the existing
setPointingHandCursorActive(_:) logic so pop is always called when the view goes
away.
- Around line 389-420: The code schedules hover dismissal using
hoverDismissGeneration as the real staleness guard, but
cancelPendingHoverDismiss currently calls .cancel() on
pendingHoverDismissWorkItem which is misleading because asyncAfter + .perform()
ignores cancel; update cancelPendingHoverDismiss to stop relying on
DispatchWorkItem.cancel(): increment hoverDismissGeneration (or set
pendingHoverDismissWorkItem = nil) to invalidate the scheduled dismiss and clear
pendingHoverDismissWorkItem, and remove or avoid calling .cancel(); touch the
functions/vars scheduleHoverDismiss, cancelPendingHoverDismiss,
hoverDismissGeneration, pendingHoverDismissWorkItem, hoverDismissDelay and
clearHoverStateNow to ensure generation-based cancellation is used consistently.
In `@Tests/TextDiffTests/NSTextDiffSnapshotTests.swift`:
- Around line 118-131: In testCharacterModeDoesNotShowAffordance, assert that
calling view._testingSetHoveredFirstRevertAction() returns false (since
character mode produces no candidates) by capturing its return value inside the
configureView closure and using an assertion like XCTAssertFalse; update the
configureView block in the test to verify the boolean result from
_testingSetHoveredFirstRevertAction() so the negative case is explicit and will
fail if candidates are generated unexpectedly.
In `@Tests/TextDiffTests/NSTextDiffViewTests.swift`:
- Around line 241-266: The test hoverLeaveSchedulesDismissNotImmediate currently
returns early if no action centers are found, silently passing; update the
early-return to record a test failure instead by replacing the guard-let that
checks view._testingActionCenters() in hoverLeaveSchedulesDismissNotImmediate
with a failure recording (e.g., call Issue.record(...) with a clear message or
use try `#require` to assert a center is present) so the test fails when
centers.isEmpty rather than passing silently; keep the rest of the test logic
(calls to view._testingUpdateHover and subsequent expects) unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Tests/TextDiffTests/__Snapshots__/NSTextDiffSnapshotTests/character_mode_no_affordance.1.pngis excluded by!**/*.pngTests/TextDiffTests/__Snapshots__/NSTextDiffSnapshotTests/hover_pair_affordance.1.pngis excluded by!**/*.pngTests/TextDiffTests/__Snapshots__/NSTextDiffSnapshotTests/hover_single_addition_affordance.1.pngis excluded by!**/*.pngTests/TextDiffTests/__Snapshots__/NSTextDiffSnapshotTests/hover_single_deletion_affordance.1.pngis excluded by!**/*.png
📒 Files selected for processing (14)
Sources/TextDiff/AppKit/DiffRevertActionResolver.swiftSources/TextDiff/AppKit/DiffTextViewRepresentable.swiftSources/TextDiff/AppKit/DiffTokenLayouter.swiftSources/TextDiff/AppKit/NSTextDiffView.swiftSources/TextDiff/DiffTypes.swiftSources/TextDiff/TextDiffGroupStrokeStyle.swiftSources/TextDiff/TextDiffStyle.swiftSources/TextDiff/TextDiffView.swiftTests/TextDiffTests/DiffLayouterPerformanceTests.swiftTests/TextDiffTests/DiffRevertActionResolverTests.swiftTests/TextDiffTests/NSTextDiffSnapshotTests.swiftTests/TextDiffTests/NSTextDiffViewTests.swiftTests/TextDiffTests/SnapshotTestSupport.swiftTests/TextDiffTests/TextDiffEngineTests.swift
Introduce handling of word boundary spacing for standalone word deletions when generating a diff revert action. Add the capability for adjusting replacement text in the DiffRevertActionResolver to ensure proper spacing is maintained when standalone words are deleted or reinserted. This prevents incorrect merging of words when reverting changes. This change improves the accuracy of diff revert actions, especially for deletions of individual words that are adjacent to other word segments in text comparisons. Additionally, modify test cases to cover scenarios where standalone word deletions should revert to include proper spacing and validate the overall improvement in handling word boundary situations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Tests/TextDiffTests/DiffLayouterPerformanceTests.swift (1)
55-71: Move assertions outside themeasureblock.
XCTAssertFalse/XCTAssertNotNilrun on every iteration of the performance loop. Capture the results outside to keep the timer clean:♻️ Proposed refactor
+ var lastLayout: DiffLayout? + var lastContext: DiffRevertInteractionContext? measure(metrics: [XCTClockMetric()]) { let layout = DiffTokenLayouter.layout( segments: segments, style: style, availableWidth: availableWidth, contentInsets: contentInsets ) let context = DiffRevertActionResolver.interactionContext( segments: segments, runs: layout.runs, mode: .token, original: original, updated: updated ) - XCTAssertFalse(layout.runs.isEmpty) - XCTAssertNotNil(context) + lastLayout = layout + lastContext = context } + XCTAssertFalse(lastLayout?.runs.isEmpty ?? true) + XCTAssertNotNil(lastContext)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/TextDiffTests/DiffLayouterPerformanceTests.swift` around lines 55 - 71, The assertions are currently inside the performance measurement loop causing them to run each iteration; move the XCTAssertFalse(layout.runs.isEmpty) and XCTAssertNotNil(context) checks outside the measure block by declaring variables for layout and context (from DiffTokenLayouter.layout(...) and DiffRevertActionResolver.interactionContext(...)) before or capturing them inside the measure and then performing the XCTAssertFalse and XCTAssertNotNil assertions after the measure completes so the timing only includes the layout/context work.Sources/TextDiff/AppKit/NSTextDiffView.swift (2)
356-359:clearHoverState()is dead code — remove it.Every call site invokes
cancelPendingHoverDismiss()andclearHoverStateNow()directly; this wrapper is never called. Drop it to keep the hover lifecycle surface area clear.♻️ Proposed fix
- private func clearHoverState() { - cancelPendingHoverDismiss() - clearHoverStateNow() - } - private func clearHoverStateNow() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/NSTextDiffView.swift` around lines 356 - 359, Remove the dead wrapper function clearHoverState() from NSTextDiffView: delete its declaration and body (the function that calls cancelPendingHoverDismiss() and clearHoverStateNow()), verify there are no remaining references to clearHoverState, and update any comments if they mention it; then build/run tests to ensure the code compiles and hover lifecycle still uses cancelPendingHoverDismiss() and clearHoverStateNow() directly.
504-520:actionID(at:layout:context:)is dead code — remove it.All call sites now use
actionIDForHitTarget(at:layout:context:includeIcon:)(lines 340 and 444). This chip-only variant is no longer wired anywhere.♻️ Proposed fix
- private func actionID(at point: CGPoint, layout: DiffLayout, context: DiffRevertInteractionContext) -> Int? { - for actionID in context.runIndicesByActionID.keys.sorted() { - guard let runIndices = context.runIndicesByActionID[actionID] else { - continue - } - for runIndex in runIndices { - guard layout.runs.indices.contains(runIndex), - let chipRect = layout.runs[runIndex].chipRect else { - continue - } - if chipRect.contains(point) { - return actionID - } - } - } - return nil - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/NSTextDiffView.swift` around lines 504 - 520, Remove the unused dead method actionID(at:layout:context:) from NSTextDiffView: delete its entire private function implementation (the chip-only hit-test that iterates context.runIndicesByActionID and checks layout.runs[*].chipRect). Ensure there are no remaining call sites referencing actionID(at:layout:context:) (all callers should already use actionIDForHitTarget(at:layout:context:includeIcon:)); run a quick search/compile to confirm no references remain and adjust imports or access control if the removal uncovers any related dead imports or unused symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TextDiff/AppKit/DiffRevertActionResolver.swift`:
- Around line 334-337: The check around insertionLocation uses UTF-16 arithmetic
and can land on a low surrogate; update the logic in DiffRevertActionResolver
(the block referencing updated, insertionLocation and beforeIsWordLike calling
isWordLike) to obtain the preceding Swift Character via String.Index instead of
subtracting one UTF-16 code unit. Convert insertionLocation to a String.Index
(e.g. using updated.utf16 or updated.index(_:offsetBy:)), ensure it isn’t
startIndex, call updated.index(before: index) to get the previous Character and
pass that Character (or its String) into isWordLike; this avoids splitting
surrogate pairs and preserves correct word-like detection.
In `@Sources/TextDiff/AppKit/NSTextDiffView.swift`:
- Around line 97-98: Remove the commented-out alternative values from the
production constants hoverButtonFillColor and hoverButtonStrokeColor in
NSTextDiffView (they are debug leftovers); replace the current lines so they
only declare the intended NSColor values (keep hoverButtonFillColor =
NSColor.black and hoverButtonStrokeColor = NSColor.clear) with no trailing
commented code or commented alternatives.
---
Nitpick comments:
In `@Sources/TextDiff/AppKit/NSTextDiffView.swift`:
- Around line 356-359: Remove the dead wrapper function clearHoverState() from
NSTextDiffView: delete its declaration and body (the function that calls
cancelPendingHoverDismiss() and clearHoverStateNow()), verify there are no
remaining references to clearHoverState, and update any comments if they mention
it; then build/run tests to ensure the code compiles and hover lifecycle still
uses cancelPendingHoverDismiss() and clearHoverStateNow() directly.
- Around line 504-520: Remove the unused dead method
actionID(at:layout:context:) from NSTextDiffView: delete its entire private
function implementation (the chip-only hit-test that iterates
context.runIndicesByActionID and checks layout.runs[*].chipRect). Ensure there
are no remaining call sites referencing actionID(at:layout:context:) (all
callers should already use
actionIDForHitTarget(at:layout:context:includeIcon:)); run a quick
search/compile to confirm no references remain and adjust imports or access
control if the removal uncovers any related dead imports or unused symbols.
In `@Tests/TextDiffTests/DiffLayouterPerformanceTests.swift`:
- Around line 55-71: The assertions are currently inside the performance
measurement loop causing them to run each iteration; move the
XCTAssertFalse(layout.runs.isEmpty) and XCTAssertNotNil(context) checks outside
the measure block by declaring variables for layout and context (from
DiffTokenLayouter.layout(...) and
DiffRevertActionResolver.interactionContext(...)) before or capturing them
inside the measure and then performing the XCTAssertFalse and XCTAssertNotNil
assertions after the measure completes so the timing only includes the
layout/context work.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Sources/TextDiff/AppKit/DiffRevertActionResolver.swiftSources/TextDiff/AppKit/NSTextDiffView.swiftTests/TextDiffTests/DiffLayouterPerformanceTests.swiftTests/TextDiffTests/DiffRevertActionResolverTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/TextDiffTests/DiffRevertActionResolverTests.swift
Comment on lines
+334
to
+337
| if insertionLocation > 0 { | ||
| let previous = updated.substring(with: NSRange(location: insertionLocation - 1, length: 1)) | ||
| beforeIsWordLike = isWordLike(previous) | ||
| } else { |
There was a problem hiding this comment.
UTF-16 surrogate boundary in adjacent-character check.
insertionLocation is a UTF-16 code-unit offset. Stepping back one unit (insertionLocation - 1) can land on the low surrogate of a supplementary-plane character (e.g. 𝐀 U+1D400). isWordLike then sees an isolated surrogate, returns false, and the required leading space is silently dropped.
The failure is safe and the characters that trigger it (mathematical/emoji) are rare in diffs, but the fix is straightforward — decode the preceding Swift Character via String.Index instead of raw UTF-16 arithmetic:
🛠️ Proposed fix — use Swift `Character`-based access
- let beforeIsWordLike: Bool
- if insertionLocation > 0 {
- let previous = updated.substring(with: NSRange(location: insertionLocation - 1, length: 1))
- beforeIsWordLike = isWordLike(previous)
- } else {
- beforeIsWordLike = false
- }
-
- let afterIsWordLike: Bool
- if insertionLocation < updated.length {
- let next = updated.substring(with: NSRange(location: insertionLocation, length: 1))
- afterIsWordLike = isWordLike(next)
- } else {
- afterIsWordLike = false
- }
+ let swiftString = updated as String
+ let insertionIdx = swiftString.utf16.index(
+ swiftString.utf16.startIndex,
+ offsetBy: insertionLocation,
+ limitedBy: swiftString.utf16.endIndex
+ ).flatMap { swiftString.index($0, offsetBy: 0, limitedBy: swiftString.endIndex) }
+ ?? swiftString.endIndex
+
+ let beforeIsWordLike: Bool = {
+ guard insertionIdx > swiftString.startIndex else { return false }
+ let prev = swiftString[swiftString.index(before: insertionIdx)]
+ return isWordLike(String(prev))
+ }()
+
+ let afterIsWordLike: Bool = {
+ guard insertionIdx < swiftString.endIndex else { return false }
+ let next = swiftString[insertionIdx]
+ return isWordLike(String(next))
+ }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TextDiff/AppKit/DiffRevertActionResolver.swift` around lines 334 -
337, The check around insertionLocation uses UTF-16 arithmetic and can land on a
low surrogate; update the logic in DiffRevertActionResolver (the block
referencing updated, insertionLocation and beforeIsWordLike calling isWordLike)
to obtain the preceding Swift Character via String.Index instead of subtracting
one UTF-16 code unit. Convert insertionLocation to a String.Index (e.g. using
updated.utf16 or updated.index(_:offsetBy:)), ensure it isn’t startIndex, call
updated.index(before: index) to get the previous Character and pass that
Character (or its String) into isWordLike; this avoids splitting surrogate pairs
and preserves correct word-like detection.
Introduce logic to treat deletion of whitespace followed by
insertion of punctuation as a single reversible edit. Update
`isReplacementPair` function in `DiffRevertActionResolver.swift`
to support punctuation replacing whitespace.
Handle cases where punctuation replaces whitespace in
`tokenDiffSegments` of `TextDiffEngine.swift` ensuring that
whitespace deletion is visible.
Update corresponding unit tests to verify functionality:
- Test that deleted whitespace followed by inserted punctuation
is recognized as a single segment in `DiffRevertActionResolverTests.swift`.
- Confirm `DiffTokenLayouter.swift` renders deleted whitespace
as chip in such cases.
This improves diff clarity for text changes involving punctuation.
Add logic to adjust standalone word insertion and deletion in DiffRevertActionResolver to preserve boundary spacing. Introduce new tests to confirm correct handling of these cases, ensuring reverts maintain intended word boundaries and spacing adjustments are consistent. This change improves the accuracy of text transformations during diff reverts, especially for word-based operations.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Sources/TextDiff/AppKit/DiffRevertActionResolver.swift (1)
352-366: UTF-16 surrogate boundary — already flagged.This is the same issue identified in a previous review:
insertionLocation - 1can land on a low surrogate. The proposed fix to use SwiftCharacter-based access still applies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/DiffRevertActionResolver.swift` around lines 352 - 366, The code slices `updated` using UTF-16 offsets which can split surrogate pairs; instead convert `updated` to a Swift String and compute Character-safe indices using String.Index(utf16Offset:in:) for the existing UTF-16 offsets, then sample the previous/next Character via s.index(before: idx) and s.index(after: idx) and pass their String/Character to isWordLike; update the logic around insertionLocation, beforeIsWordLike, and afterIsWordLike to use these Character-safe indices (references: variables insertionLocation and updated and function isWordLike).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TextDiff/AppKit/DiffRevertActionResolver.swift`:
- Around line 378-407: The boundary checks in
adjustedStandaloneWordInsertionRemovalRange use NSString.substring(with:
NSRange(..., length: 1)) which can split UTF‑16 surrogate pairs; replace those
uses with String.Index-based character checks similar to the fix in
adjustedStandaloneWordDeletionReplacement so you inspect whole extended grapheme
clusters rather than single UTF‑16 code units. Convert the updated NSString to a
Swift String (or derive String.Index from updated.utf16 safely) and compute the
preceding and following character (if any) using String.index(before:) /
String.index(after:) or equivalent, then pass that full-character String to
isWhitespaceCharacter; keep the same returned NSRange adjustments but ensure
indices map back correctly to UTF‑16 offsets when building the NSRange to
return.
- Around line 56-65: In DiffRevertActionResolver inside the switch on
segment.kind (the .equal case), currently originalRange/updatedRange are
computed from originalCursor/updatedCursor but the cursors are only advanced if
textMatches succeeds; change this so the cursors are always advanced by
textLength after computing the ranges to avoid corrupting downstream ranges, and
add a debug-time assertion or logger when textMatches(segment.text, ...) returns
false so mismatches surface during development; reference the symbols
segment.kind == .equal, textMatches(...), originalCursor, updatedCursor,
originalRange and updatedRange when making the change.
In `@Sources/TextDiff/AppKit/NSTextDiffView.swift`:
- Around line 504-520: Remove the dead method actionID(at:layout:context:) —
it's unused and superseded by actionIDForHitTarget(at:layout:context:). Delete
the private func actionID(at point: CGPoint, layout: DiffLayout, context:
DiffRevertInteractionContext) implementation and any accompanying unused
references or imports specific to it so the class only exposes the active
actionIDForHitTarget(at:layout:context:) logic.
---
Duplicate comments:
In `@Sources/TextDiff/AppKit/DiffRevertActionResolver.swift`:
- Around line 352-366: The code slices `updated` using UTF-16 offsets which can
split surrogate pairs; instead convert `updated` to a Swift String and compute
Character-safe indices using String.Index(utf16Offset:in:) for the existing
UTF-16 offsets, then sample the previous/next Character via s.index(before: idx)
and s.index(after: idx) and pass their String/Character to isWordLike; update
the logic around insertionLocation, beforeIsWordLike, and afterIsWordLike to use
these Character-safe indices (references: variables insertionLocation and
updated and function isWordLike).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Sources/TextDiff/AppKit/DiffRevertActionResolver.swiftSources/TextDiff/AppKit/DiffTokenLayouter.swiftSources/TextDiff/AppKit/NSTextDiffView.swiftSources/TextDiff/TextDiffEngine.swiftSources/TextDiff/TextDiffView.swiftTests/TextDiffTests/DiffRevertActionResolverTests.swiftTests/TextDiffTests/TextDiffEngineTests.swift
Comment on lines
+56
to
+65
| switch segment.kind { | ||
| case .equal: | ||
| originalRange = NSRange(location: originalCursor, length: textLength) | ||
| updatedRange = NSRange(location: updatedCursor, length: textLength) | ||
| if textMatches(segment.text, source: originalNSString, at: originalCursor) { | ||
| originalCursor += textLength | ||
| } | ||
| if textMatches(segment.text, source: updatedNSString, at: updatedCursor) { | ||
| updatedCursor += textLength | ||
| } |
There was a problem hiding this comment.
Conditional cursor advancement for .equal segments could silently produce wrong ranges.
If textMatches fails for an .equal segment (lines 60-65), the cursor doesn't advance, but the range was already computed from the stale cursor position. Every subsequent segment gets wrong originalRange/updatedRange values. Since equal segments should always match by definition, this is defensive, but if it ever triggers, it silently corrupts all downstream indexing rather than failing loudly.
Consider either always advancing the cursor for .equal (trusting the diff engine) or adding an assertion/log so mismatches surface during development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TextDiff/AppKit/DiffRevertActionResolver.swift` around lines 56 - 65,
In DiffRevertActionResolver inside the switch on segment.kind (the .equal case),
currently originalRange/updatedRange are computed from
originalCursor/updatedCursor but the cursors are only advanced if textMatches
succeeds; change this so the cursors are always advanced by textLength after
computing the ranges to avoid corrupting downstream ranges, and add a debug-time
assertion or logger when textMatches(segment.text, ...) returns false so
mismatches surface during development; reference the symbols segment.kind ==
.equal, textMatches(...), originalCursor, updatedCursor, originalRange and
updatedRange when making the change.
Comment on lines
+378
to
+407
| private static func adjustedStandaloneWordInsertionRemovalRange( | ||
| _ range: NSRange, | ||
| updated: NSString | ||
| ) -> NSRange { | ||
| guard range.location >= 0, range.length >= 0 else { | ||
| return range | ||
| } | ||
| guard NSMaxRange(range) <= updated.length else { | ||
| return range | ||
| } | ||
|
|
||
| let hasLeadingWhitespace = range.location > 0 | ||
| && isWhitespaceCharacter(updated.substring(with: NSRange(location: range.location - 1, length: 1))) | ||
| let hasTrailingWhitespace = NSMaxRange(range) < updated.length | ||
| && isWhitespaceCharacter(updated.substring(with: NSRange(location: NSMaxRange(range), length: 1))) | ||
|
|
||
| if hasLeadingWhitespace, hasTrailingWhitespace { | ||
| return NSRange(location: range.location, length: range.length + 1) | ||
| } | ||
|
|
||
| if range.location == 0, hasTrailingWhitespace { | ||
| return NSRange(location: range.location, length: range.length + 1) | ||
| } | ||
|
|
||
| if NSMaxRange(range) == updated.length, hasLeadingWhitespace { | ||
| return NSRange(location: range.location - 1, length: range.length + 1) | ||
| } | ||
|
|
||
| return range | ||
| } |
There was a problem hiding this comment.
Same UTF-16 surrogate boundary issue as in adjustedStandaloneWordDeletionReplacement.
Lines 389-392 use NSString.substring(with: NSRange(location: ..., length: 1)) which reads a single UTF-16 code unit. If the character at that boundary is a supplementary-plane character (e.g., emoji), this splits a surrogate pair. isWhitespaceCharacter would return false (safe fallback — no range adjustment), so it's not a crash, but the range won't be adjusted when it should be.
The same fix from the past review comment on adjustedStandaloneWordDeletionReplacement applies here: use Swift String.Index-based character access instead of raw UTF-16 arithmetic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TextDiff/AppKit/DiffRevertActionResolver.swift` around lines 378 -
407, The boundary checks in adjustedStandaloneWordInsertionRemovalRange use
NSString.substring(with: NSRange(..., length: 1)) which can split UTF‑16
surrogate pairs; replace those uses with String.Index-based character checks
similar to the fix in adjustedStandaloneWordDeletionReplacement so you inspect
whole extended grapheme clusters rather than single UTF‑16 code units. Convert
the updated NSString to a Swift String (or derive String.Index from
updated.utf16 safely) and compute the preceding and following character (if any)
using String.index(before:) / String.index(after:) or equivalent, then pass that
full-character String to isWhitespaceCharacter; keep the same returned NSRange
adjustments but ensure indices map back correctly to UTF‑16 offsets when
building the NSRange to return.
Comment on lines
+504
to
+520
| private func actionID(at point: CGPoint, layout: DiffLayout, context: DiffRevertInteractionContext) -> Int? { | ||
| for actionID in context.runIndicesByActionID.keys.sorted() { | ||
| guard let runIndices = context.runIndicesByActionID[actionID] else { | ||
| continue | ||
| } | ||
| for runIndex in runIndices { | ||
| guard layout.runs.indices.contains(runIndex), | ||
| let chipRect = layout.runs[runIndex].chipRect else { | ||
| continue | ||
| } | ||
| if chipRect.contains(point) { | ||
| return actionID | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to actionID(at: in Swift files
rg -n 'actionID\(at:' --type swiftRepository: iSapozhnik/TextDiff
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Also search for actionIDForHitTarget to verify it exists
rg -n 'actionIDForHitTarget' --type swiftRepository: iSapozhnik/TextDiff
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Get more context around both methods to understand the relationship
rg -B5 -A15 'func actionID\(at:' Sources/TextDiff/AppKit/NSTextDiffView.swiftRepository: iSapozhnik/TextDiff
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Find the actionID method definition
rg -A20 'private func actionID\(at point:' Sources/TextDiff/AppKit/NSTextDiffView.swiftRepository: iSapozhnik/TextDiff
Length of output: 939
🏁 Script executed:
#!/bin/bash
# Get actionIDForHitTarget for comparison
rg -A20 'private func actionIDForHitTarget' Sources/TextDiff/AppKit/NSTextDiffView.swiftRepository: iSapozhnik/TextDiff
Length of output: 695
Remove actionID(at:layout:context:) — it's unused dead code.
The method has no call sites in the codebase. Its functionality has been replaced by actionIDForHitTarget(at:layout:context:), which is actively used (lines 340, 444) and includes additional hit target logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TextDiff/AppKit/NSTextDiffView.swift` around lines 504 - 520, Remove
the dead method actionID(at:layout:context:) — it's unused and superseded by
actionIDForHitTarget(at:layout:context:). Delete the private func actionID(at
point: CGPoint, layout: DiffLayout, context: DiffRevertInteractionContext)
implementation and any accompanying unused references or imports specific to it
so the class only exposes the active actionIDForHitTarget(at:layout:context:)
logic.
Introduce an option to render invisible characters with a debug overlay in NSTextDiffView, activated by a new `showsInvisibleCharacters` property. This feature aids in debugging by visualizing spaces, tabs, and newline characters using distinct symbols in red. This change doesn't alter the core functionality but enhances debugging capabilities, allowing developers to better understand the alignment and structure of rendered text, especially useful when analyzing unexpected layout issues in text diffs.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/TextDiff/AppKit/NSTextDiffView.swift (1)
520-536: Consider removing the unusedactionID(at:layout:context:)helper.
Hit-testing now flows throughactionIDForHitTarget. If this helper has no call sites, dropping it keeps things lean.#!/bin/bash # Verify whether actionID(at:layout:context:) has any call sites. rg -n '\bactionID\(at:' --type swift🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/NSTextDiffView.swift` around lines 520 - 536, The helper function actionID(at:layout:context:) appears unused now that hit-testing is handled by actionIDForHitTarget; remove the entire private method to avoid dead code (delete the actionID(at:layout:context:) declaration and body), run a project-wide search for "actionID(at:" to confirm there are no remaining call sites, and rebuild tests/compile to ensure no references remain; if any callers are found, either update them to use actionIDForHitTarget or keep/relay behavior as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/TextDiff/AppKit/NSTextDiffView.swift`:
- Around line 520-536: The helper function actionID(at:layout:context:) appears
unused now that hit-testing is handled by actionIDForHitTarget; remove the
entire private method to avoid dead code (delete the
actionID(at:layout:context:) declaration and body), run a project-wide search
for "actionID(at:" to confirm there are no remaining call sites, and rebuild
tests/compile to ensure no references remain; if any callers are found, either
update them to use actionIDForHitTarget or keep/relay behavior as appropriate.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Tests/TextDiffTests/__Snapshots__/NSTextDiffSnapshotTests/invisible_characters_debug_overlay.1.pngis excluded by!**/*.png
📒 Files selected for processing (6)
Sources/TextDiff/AppKit/DiffTextViewRepresentable.swiftSources/TextDiff/AppKit/DiffTokenLayouter.swiftSources/TextDiff/AppKit/NSTextDiffView.swiftSources/TextDiff/TextDiffView.swiftTests/TextDiffTests/NSTextDiffSnapshotTests.swiftTests/TextDiffTests/NSTextDiffViewTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TextDiff/AppKit/DiffTokenLayouter.swift (1)
202-206:⚠️ Potential issue | 🟠 MajorAvoid O(n²) measurement inside the token loop.
Measuring the whole accumulated line for every token scales poorly on long lines and will regress layout responsiveness.
Suggested fix
- lineText.append(text) - // TODO: Fix this later - // This now appends each token to lineText and calls size(withAttributes:) on the entire accumulated line every iteration, which makes layout cost grow quadratically with line length. On long unwrapped diffs (hundreds/thousands of tokens), this is a significant regression from the prior per-token measurement approach and can noticeably slow rendering even though the new performance tests only capture baselines and do not enforce thresholds. - let combinedWidth = lineText.size(withAttributes: [.font: font]).width - let textWidth = max(0, combinedWidth - lineTextWidth) + let textWidth = measuredStandaloneTextWidth(for: text, font: font) + lineText.append(text) + let combinedWidth = lineTextWidth + textWidth🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/AppKit/DiffTokenLayouter.swift` around lines 202 - 206, The loop in DiffTokenLayouter that appends to lineText then calls lineText.size(withAttributes: [.font: font]) each iteration causes O(n²) work; instead measure only the newly appended token and update the running width. Replace the combinedWidth/textWidth calculation with a per-token measurement: compute tokenWidth = text.size(withAttributes: [.font: font]).width (or an equivalent boundingRect call) and set textWidth = max(0, lineTextWidth + tokenWidth - lineTextWidth) / simply add tokenWidth to lineTextWidth; update lineTextWidth += tokenWidth and avoid calling size(withAttributes:) on the whole line. Keep references to lineText, lineTextWidth and font in your changes and remove the repeated combinedWidth computation.
🧹 Nitpick comments (2)
Sources/TextDiff/TextDiffView.swift (1)
137-137: Consider renaming “revertable” to “revertible” in preview text.Tiny wording polish for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TextDiff/TextDiffView.swift` at line 137, Update the preview text string in TextDiffView by correcting the spelling "revertable" to "revertible" (locate the literal Text("Diff by words and revertable") and change it to Text("Diff by words and revertible")); if this string is localized, update the corresponding localization key/value instead of the literal..claude/agents/project-manager-backlog.md (1)
57-57: Use “one-liner” for consistency.Line 57 should use hyphenated form:
one-liner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/project-manager-backlog.md at line 57, Update the heading text that currently reads "### **Title (one liner)**" to use the hyphenated form "one-liner" so it becomes "### **Title (one-liner)**" (locate the markdown heading with the exact text "### **Title (one liner)**" and replace the phrase).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/agents/project-manager-backlog.md:
- Line 3: The YAML front matter's description field contains unescaped colons
(e.g., "user:" and "assistant:") which break parsing; replace the inline
description value with a YAML block scalar (use ">" on the description key) and
indent the subsequent multi-line text so the examples and commentary remain
intact; ensure the description field is a single block scalar containing all
example text (preserving newlines and indentation under the block) so standard
YAML parsers can parse the file without ScannerError.
In `@Sources/TextDiff/AppKit/DiffTokenLayouter.swift`:
- Around line 91-96: The layouter uses two different width sources
(displayTextWidth for drawing and textMeasurement.textWidth for advancing)
causing overlap for unchanged runs; update the logic in DiffTokenLayouter (the
runWidth/requiredWidth calculation where isChangedLexical, displayTextWidth, and
textMeasurement.textWidth are used, and the similar block at lines ~114-118) to
pick a single width source for both drawing and cursor advance (e.g., always use
displayTextWidth or a unified computed width variable), and apply that
consistently when computing runWidth, requiredWidth and any textRect.width so
subsequent runs start at the same x-position as the drawn glyphs.
---
Outside diff comments:
In `@Sources/TextDiff/AppKit/DiffTokenLayouter.swift`:
- Around line 202-206: The loop in DiffTokenLayouter that appends to lineText
then calls lineText.size(withAttributes: [.font: font]) each iteration causes
O(n²) work; instead measure only the newly appended token and update the running
width. Replace the combinedWidth/textWidth calculation with a per-token
measurement: compute tokenWidth = text.size(withAttributes: [.font: font]).width
(or an equivalent boundingRect call) and set textWidth = max(0, lineTextWidth +
tokenWidth - lineTextWidth) / simply add tokenWidth to lineTextWidth; update
lineTextWidth += tokenWidth and avoid calling size(withAttributes:) on the whole
line. Keep references to lineText, lineTextWidth and font in your changes and
remove the repeated combinedWidth computation.
---
Nitpick comments:
In @.claude/agents/project-manager-backlog.md:
- Line 57: Update the heading text that currently reads "### **Title (one
liner)**" to use the hyphenated form "one-liner" so it becomes "### **Title
(one-liner)**" (locate the markdown heading with the exact text "### **Title
(one liner)**" and replace the phrase).
In `@Sources/TextDiff/TextDiffView.swift`:
- Line 137: Update the preview text string in TextDiffView by correcting the
spelling "revertable" to "revertible" (locate the literal Text("Diff by words
and revertable") and change it to Text("Diff by words and revertible")); if this
string is localized, update the corresponding localization key/value instead of
the literal.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.claude/agents/project-manager-backlog.mdAGENTS.mdSources/TextDiff/AppKit/DiffTokenLayouter.swiftSources/TextDiff/TextDiffView.swiftTests/TextDiffTests/TextDiffEngineTests.swiftbacklog/config.ymlbacklog/tasks/task-1 - Fix-missing-last-character-rendering-in-added-text.md
✅ Files skipped from review due to trivial changes (2)
- backlog/tasks/task-1 - Fix-missing-last-character-rendering-in-added-text.md
- AGENTS.md
| @@ -0,0 +1,193 @@ | |||
| --- | |||
| name: project-manager-backlog | |||
| description: Use this agent when you need to manage project tasks using the backlog.md CLI tool. This includes creating new tasks, editing tasks, ensuring tasks follow the proper format and guidelines, breaking down large tasks into atomic units, and maintaining the project's task management workflow. Examples: <example>Context: User wants to create a new task for adding a feature. user: "I need to add a new authentication system to the project" assistant: "I'll use the project-manager-backlog agent that will use backlog cli to create a properly structured task for this feature." <commentary>Since the user needs to create a task for the project, use the Task tool to launch the project-manager-backlog agent to ensure the task follows backlog.md guidelines.</commentary></example> <example>Context: User has multiple related features to implement. user: "We need to implement user profiles, settings page, and notification preferences" assistant: "Let me use the project-manager-backlog agent to break these down into atomic, independent tasks." <commentary>The user has a complex set of features that need to be broken down into proper atomic tasks following backlog.md structure.</commentary></example> <example>Context: User wants to review if their task description is properly formatted. user: "Can you check if this task follows our guidelines: 'task-123 - Implement user login'" assistant: "I'll use the project-manager-backlog agent to review this task against our backlog.md standards." <commentary>The user needs task review, so use the project-manager-backlog agent to ensure compliance with project guidelines.</commentary></example> | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read it
if [ -f ".claude/agents/project-manager-backlog.md" ]; then
echo "=== File exists ==="
head -20 ".claude/agents/project-manager-backlog.md"
else
echo "File not found at .claude/agents/project-manager-backlog.md"
# Try to find it
find . -name "project-manager-backlog.md" 2>/dev/null
fiRepository: iSapozhnik/TextDiff
Length of output: 2667
🏁 Script executed:
python -m pip -q install pyyaml
python - <<'PY'
from pathlib import Path
import re, yaml, sys
p = Path(".claude/agents/project-manager-backlog.md")
text = p.read_text(encoding="utf-8")
m = re.match(r"^---\n(.*?)\n---\n", text, re.S)
if not m:
print("No front matter found")
sys.exit(1)
front_matter = m.group(1)
print("=== Front matter extracted ===")
print(front_matter[:200], "...")
print("\n=== Attempting YAML parse ===")
try:
data = yaml.safe_load(front_matter)
print("✓ YAML parsed successfully")
print("Keys:", list(data.keys()))
if 'description' in data:
desc = data['description']
print(f"Description length: {len(desc)} characters")
print(f"First 100 chars: {desc[:100]}")
except Exception as e:
print(f"✗ YAML parse error: {type(e).__name__}: {e}")
sys.exit(2)
PYRepository: iSapozhnik/TextDiff
Length of output: 566
Fix YAML front matter to use block scalar syntax.
The description field on line 3 contains unquoted colons (in user: and assistant: tokens) that break YAML parsing. Standard YAML parsers fail with ScannerError: mapping values are not allowed here because colons followed by spaces are interpreted as mapping indicators.
Use a block scalar (>) instead:
Suggested fix
-description: Use this agent when you need to manage project tasks using the backlog.md CLI tool. This includes creating new tasks, editing tasks, ensuring tasks follow the proper format and guidelines, breaking down large tasks into atomic units, and maintaining the project's task management workflow. Examples: <example>Context: User wants to create a new task for adding a feature. user: "I need to add a new authentication system to the project" assistant: "I'll use the project-manager-backlog agent that will use backlog cli to create a properly structured task for this feature." <commentary>Since the user needs to create a task for the project, use the Task tool to launch the project-manager-backlog agent to ensure the task follows backlog.md guidelines.</commentary></example> <example>Context: User has multiple related features to implement. user: "We need to implement user profiles, settings page, and notification preferences" assistant: "Let me use the project-manager-backlog agent to break these down into atomic, independent tasks." <commentary>The user has a complex set of features that need to be broken down into proper atomic tasks following backlog.md structure.</commentary></example> <example>Context: User wants to review if their task description is properly formatted. user: "Can you check if this task follows our guidelines: 'task-123 - Implement user login'" assistant: "I'll use the project-manager-backlog agent to review this task against our backlog.md standards." <commentary>The user needs task review, so use the project-manager-backlog agent to ensure compliance with project guidelines.</commentary></example>
+description: >
+ Use this agent when you need to manage project tasks using the backlog.md CLI tool.
+ This includes creating new tasks, editing tasks, ensuring tasks follow the proper
+ format and guidelines, breaking down large tasks into atomic units, and maintaining
+ the project's task management workflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/project-manager-backlog.md at line 3, The YAML front matter's
description field contains unescaped colons (e.g., "user:" and "assistant:")
which break parsing; replace the inline description value with a YAML block
scalar (use ">" on the description key) and indent the subsequent multi-line
text so the examples and commentary remain intact; ensure the description field
is a single block scalar containing all example text (preserving newlines and
indentation under the block) so standard YAML parsers can parse the file without
ScannerError.
Comment on lines
+91
to
96
| var textSize = CGSize(width: displayTextWidth, height: textHeight) | ||
| let chipInsets = effectiveChipInsets(for: style) | ||
| var runWidth = isChangedLexical ? textSize.width + chipInsets.left + chipInsets.right : textSize.width | ||
| var runWidth = isChangedLexical | ||
| ? displayTextWidth + chipInsets.left + chipInsets.right | ||
| : textMeasurement.textWidth | ||
| let requiredWidth = leadingGap + runWidth |
There was a problem hiding this comment.
Use one width source for both drawing and cursor advance on unchanged runs.
textRect.width uses displayTextWidth, but unchanged runs still advance by textMeasurement.textWidth. When these differ, next runs can start too early and overlap/hit regions drift.
Suggested fix
- var runWidth = isChangedLexical
- ? displayTextWidth + chipInsets.left + chipInsets.right
- : textMeasurement.textWidth
+ var runWidth = isChangedLexical
+ ? displayTextWidth + chipInsets.left + chipInsets.right
+ : displayTextWidth
@@
- runWidth = isChangedLexical
- ? displayTextWidth + chipInsets.left + chipInsets.right
- : textMeasurement.textWidth
+ runWidth = isChangedLexical
+ ? displayTextWidth + chipInsets.left + chipInsets.right
+ : displayTextWidthAlso applies to: 114-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TextDiff/AppKit/DiffTokenLayouter.swift` around lines 91 - 96, The
layouter uses two different width sources (displayTextWidth for drawing and
textMeasurement.textWidth for advancing) causing overlap for unchanged runs;
update the logic in DiffTokenLayouter (the runWidth/requiredWidth calculation
where isChangedLexical, displayTextWidth, and textMeasurement.textWidth are
used, and the similar block at lines ~114-118) to pick a single width source for
both drawing and cursor advance (e.g., always use displayTextWidth or a unified
computed width variable), and apply that consistently when computing runWidth,
requiredWidth and any textRect.width so subsequent runs start at the same
x-position as the drawn glyphs.
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
updatedvia binding and also emit action metadatasolid/dashed) for the revert hover framePublic API Changes
DiffTypes.swiftTextDiffRevertActionKindTextDiffRevertActionTextDiffView.swiftinit(original:updated:style:mode:isRevertActionsEnabled:onRevertAction:)TextDiffStyle.swiftgroupStrokeStyle: TextDiffGroupStrokeStyle(default.solid)TextDiffGroupStrokeStyle.swift.solid,.dashedNSTextDiffView.swiftisRevertActionsEnabledandonRevertActionfor interaction handlingImplementation Notes
DiffRevertActionResolverto:LaidOutRunwithsegmentIndexfor stable run->segment action mappingNSTextDiffView:Tests
DiffRevertActionResolverTestsfor candidate detection + patch semanticsNSTextDiffViewTestswith:NSTextDiffSnapshotTestshover snapshots for AppKit affordancesDiffLayouterPerformanceTestsValidation
swift test 2>&1 | xcsiftSummary by CodeRabbit
New Features
Behavioral
Tests