Skip to content

Add iOS support and set default text colors#8

Open
iSapozhnik wants to merge 13 commits into
mainfrom
feature/ios-support
Open

Add iOS support and set default text colors#8
iSapozhnik wants to merge 13 commits into
mainfrom
feature/ios-support

Conversation

@iSapozhnik
Copy link
Copy Markdown
Owner

@iSapozhnik iSapozhnik commented Apr 18, 2026

This pull request restructures the TextDiff package to support both macOS and iOS platforms, modularizes the codebase into platform-specific and shared components, and updates the SwiftUI interface to be cross-platform. The most significant changes include the introduction of new library and target modules, refactoring of the main SwiftUI view to support both platforms, and moving core logic into a new TextDiffCore target.

Cross-platform and modularization updates:

  • The Swift package (Package.swift) now supports both macOS and iOS, and defines new library products and targets: TextDiffCore, TextDiffUICommon, TextDiffMacOSUI, and TextDiffIOSUI, along with platform-conditional dependencies and test targets. [1] [2]
  • The main TextDiff target now re-exports the core and UI modules, and conditionally includes the macOS or iOS UI module depending on the platform. (Sources/TextDiff/TextDiff.swift)

SwiftUI view refactorings:

  • TextDiffView in Sources/TextDiff/TextDiffView.swift is refactored to use platform-conditional logic, delegating to either a macOS (TextDiffMacOSRepresentable) or iOS (TextDiffIOSRepresentable) implementation. This enables the same SwiftUI interface to work seamlessly across platforms. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]
  • Platform-specific types such as PlatformFont and TextDiffEdgeInsets are now used in place of AppKit-only types, making the style configuration cross-platform. [1] [2] [3] [4]

Core logic reorganization:

  • Core diff logic (DiffSegmentIndexer and related types) has been moved from Sources/TextDiff to a new Sources/TextDiffCore target, and access levels have been updated to use Swift's new package visibility.
  • A file-level comment was added to the new Sources/TextDiffCore/TextDiff.swift for clarity.

Cleanup and removals:

  • The macOS-specific default change style extension was removed from TextDiffChangeStyleDefaults.swift, further decoupling platform-specific code.

These changes lay the foundation for a clean, modular, and cross-platform Swift package architecture for text diffing.

Summary by CodeRabbit

  • New Features

    • Added iOS 18 support and a native UIKit text-diff view with precomputed/result rendering.
    • Cross-platform SwiftUI view now dispatches platform-specific rendering and includes macOS-only binding initializer and revert affordances.
  • Breaking Changes

    • Removed shared showsInvisibleCharacters parameter from several initializers.
    • Styling types and defaults moved to platform-agnostic color/font/edge-inset abstractions.
  • Refactor

    • Library reorganized into modular platform-focused components.
  • Documentation

    • README updated for iOS usage, platform APIs, and testing guidance.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 18, 2026

Warning

Review limit reached

@iSapozhnik, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 46 minutes and 21 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3e284225-c729-4c94-8b68-c3f8196d1341

📥 Commits

Reviewing files that changed from the base of the PR and between 50bce44 and 3a97e3d.

📒 Files selected for processing (1)
  • Sources/TextDiffCore/DiffSegmentIndexer.swift
📝 Walkthrough

Walkthrough

This PR restructures TextDiff into separate core/UI-common/macOS/iOS modules, adds iOS 18 support, introduces platform-agnostic UI primitives, implements an iOS UITextDiffView, refactors TextDiffView to platform-dispatch via representables, adds revert hit-testing, and reorganizes tests across the new modules.

Changes

Cross-platform modularization

Layer / File(s) Summary
Package manifest & documentation updates
Package.swift, README.md
iOS 18 added; new products TextDiffCore, TextDiffUICommon, TextDiffMacOSUI, TextDiffIOSUI; test targets reorganized; README updated with product/import guidance and iOS/UIKit examples.
Root module API exports & TextDiffView refactor
Sources/TextDiff/TextDiff.swift, Sources/TextDiff/TextDiffView.swift
Root module re-exports TextDiffCore/TextDiffUICommon and conditionally exports platform UI modules; TextDiffView refactored to platform-dispatch, OS-gated binding initializers, and previews updated to platform-agnostic types.
iOS representable & SwiftUI previews
Sources/TextDiff/TextDiffView.swift
Add TextDiffIOSRepresentable (UIViewRepresentable) with coordinator to sync updatedBinding and propagate revert actions; add iOS previews.
iOS UITextDiffView implementation
Sources/TextDiffIOSUI/UITextDiffView.swift
New UIKit UITextDiffView with public API, intrinsic sizing, width-keyed layout caching, batching updates, chip rendering, revert tap handling and hit-testing, and test hooks.
macOS UI adjustments & imports
Sources/TextDiffMacOSUI/AppKit/*.swift
Import TextDiffCore/TextDiffUICommon into macOS files, switch to TextDiffEdgeInsets for layout, and simplify batching invalidation by removing pendingStyleInvalidation.
Platform-agnostic UI primitives & style
Sources/TextDiffUICommon/*
Introduce PlatformColor/PlatformFont aliases and TextDiffEdgeInsets; migrate TextDiffStyle/TextDiffChangeStyle/styling to platform-agnostic types and add default addition/removal presets.
Layout metrics & token layouter
Sources/TextDiffUICommon/DiffTokenLayouter.swift, DiffTextLayoutMetrics.swift
Move layout helpers to package scope, use PlatformFont/PlatformColor/TextDiffEdgeInsets, add RGBA helper and adjust luminance logic.
Core indexer visibility
Sources/TextDiffCore/DiffSegmentIndexer.swift
Make IndexedDiffSegment/DiffSegmentIndexer package-scoped and change .equal segment handling to conditionally advance cursors based on validated text matches.
Revert resolver & hit-testing infrastructure
Sources/TextDiffUICommon/DiffRevertActionResolver.swift, DiffRevertHitResolver.swift
Expose revert candidate/context types and resolver entry points as package; add DiffRevertHitResolver for exact/expanded chip hit-testing and nearest-candidate selection.
Tests: reorg and new coverage
Tests/*
Re-target tests to TextDiffCore, TextDiffUICommon, and TextDiffMacOSUI; add TextDiffCoreTests and TextDiffUICommonTests; add macOS UI test suites and new hit/resolver tests.

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

  • iSapozhnik/TextDiff#7: Both PRs touch precomputed result rendering paths and shared diff indexing used by result-driven rendering and revert handling.
  • iSapozhnik/TextDiff#5: Prior revert/binding interaction and OS-gated initializer work that is extended by this PR.

Poem

A monolith split with tidy art,
macOS kept, iOS joins the part.
Platform types and hit-test song,
chips and diffs now get along. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.08% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding iOS support and setting default text colors, which aligns with the PR's primary objectives of cross-platform support and modularization.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/ios-support

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
Package.swift (1)

43-72: TESTING flag scope makes #if !TESTING assert(...) blocks effectively dead.

TESTING is defined for every debug build, not just test runs. Since assert is a no-op in release, the guarded assertions in DiffSegmentIndexer (and any similar guards) never execute in any configuration. If the intent is to suppress assertions only while running tests, consider moving the TESTING define to the test target via an -D unsafe flag / test plan, or use #if DEBUG && !TESTING with the flag set only in a dedicated configuration.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Package.swift` around lines 43 - 72, The TESTING build define is applied to
all debug targets making any guards like `#if !TESTING` in DiffSegmentIndexer
effectively dead; update the build settings so TESTING is only defined for
actual test runs (e.g., remove `.define("TESTING", .when(configuration:
.debug))` from the targets and instead add the flag only to the test target via
an unsafe `-DTESTING` test-only flag or a dedicated test configuration), or
adjust the conditionals to `#if DEBUG && !TESTING` and ensure TESTING is set
only in the test configuration so assertions in DiffSegmentIndexer behave as
intended.
Sources/TextDiffCore/TextDiff.swift (1)

1-1: Consider deleting this placeholder file.

A file containing only a comment adds noise without carrying anything the compiler or reader uses. If TextDiffCore needs at least one source file to compile, keep it; otherwise drop it. If kept, a brief note pointing to the actual entry points (e.g. DiffSegmentIndexer.swift, TextDiffEngine.swift) would be more useful than the current one-liner.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/TextDiffCore/TextDiff.swift` at line 1, This file is a no-op
placeholder; either remove TextDiff.swift to eliminate noise (if TextDiffCore
builds without it) or replace the comment with a concise guide pointing to the
real entry points (e.g., reference DiffSegmentIndexer.swift and
TextDiffEngine.swift) so readers know where the public API lives; update the
repository accordingly and ensure the module still compiles after deleting or
editing TextDiff.swift.
Sources/TextDiff/TextDiffView.swift (1)

363-401: Add iOS previews for the new SwiftUI path.

The macOS branch has previews, but the new iOS representable path has none. Add a couple of iOS #Previews, for example default rendering and precomputed-result rendering.

As per coding guidelines, SwiftUI views should always include #Preview with a few state configurations.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/TextDiff/TextDiffView.swift` around lines 363 - 401, Add SwiftUI
previews for the iOS representable by creating one or more `#Preview` blocks that
instantiate TextDiffIOSRepresentable with (1) live inputs using
original/updated/mode/style to show default rendering and (2) a precomputed
TextDiffResult passed to the result parameter to show the precomputed-result
rendering; reference TextDiffIOSRepresentable (the makeUIView/updateUIView
paths) and ensure the previews compile under `#if` os(iOS) and exercise both
branches (result != nil and result == nil).
🤖 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/TextDiffIOSUI/UITextDiffView.swift`:
- Around line 6-57: Add three public properties to
UITextDiffView—isRevertActionsEnabled: Bool, showsInvisibleCharacters: Bool, and
onRevertAction: ((DiffSegment) -> Void)?—with sensible defaults and didSet
observers that mirror NSTextDiffView semantics: when changed, set any relevant
internal state and call the same update/invalidation flow used elsewhere (e.g.
set contentSource = .text and call updateSegmentsIfNeeded() or call
invalidateCachedLayout()/mark pendingStyleInvalidation as appropriate). Ensure
the onRevertAction is stored and invoked by whatever revert-action UI/path the
view exposes, and reference existing helpers like updateSegmentsIfNeeded(),
invalidateCachedLayout(), contentSource, pendingStyleInvalidation, and segments
to locate where to hook the new behavior.

In `@Sources/TextDiffUICommon/TextDiffChangeStyleDefaults.swift`:
- Around line 9-14: The defaultRemoval TextDiffChangeStyle currently sets
strikethrough to false causing deletions to render without a strike; update the
defaultRemoval initializer (TextDiffChangeStyle defaultRemoval) to set
strikethrough: true so deleted text uses the strikethrough style by default,
leaving other properties (fillColor, strokeColor, textColorOverride) unchanged.

In `@Sources/TextDiffUICommon/TextDiffViewModel.swift`:
- Around line 3-4: Remove the duplicate import by deleting one of the repeated
"import TextDiffCore" statements in TextDiffViewModel.swift so the module is
only imported once; keep a single "import TextDiffCore" line and remove the
redundant one.

---

Nitpick comments:
In `@Package.swift`:
- Around line 43-72: The TESTING build define is applied to all debug targets
making any guards like `#if !TESTING` in DiffSegmentIndexer effectively dead;
update the build settings so TESTING is only defined for actual test runs (e.g.,
remove `.define("TESTING", .when(configuration: .debug))` from the targets and
instead add the flag only to the test target via an unsafe `-DTESTING` test-only
flag or a dedicated test configuration), or adjust the conditionals to `#if
DEBUG && !TESTING` and ensure TESTING is set only in the test configuration so
assertions in DiffSegmentIndexer behave as intended.

In `@Sources/TextDiff/TextDiffView.swift`:
- Around line 363-401: Add SwiftUI previews for the iOS representable by
creating one or more `#Preview` blocks that instantiate TextDiffIOSRepresentable
with (1) live inputs using original/updated/mode/style to show default rendering
and (2) a precomputed TextDiffResult passed to the result parameter to show the
precomputed-result rendering; reference TextDiffIOSRepresentable (the
makeUIView/updateUIView paths) and ensure the previews compile under `#if` os(iOS)
and exercise both branches (result != nil and result == nil).

In `@Sources/TextDiffCore/TextDiff.swift`:
- Line 1: This file is a no-op placeholder; either remove TextDiff.swift to
eliminate noise (if TextDiffCore builds without it) or replace the comment with
a concise guide pointing to the real entry points (e.g., reference
DiffSegmentIndexer.swift and TextDiffEngine.swift) so readers know where the
public API lives; update the repository accordingly and ensure the module still
compiles after deleting or editing TextDiff.swift.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9add5936-e6ca-4644-b093-7beb1377b8f9

📥 Commits

Reviewing files that changed from the base of the PR and between b5815ad and 34f69f0.

⛔ Files ignored due to path filters (18)
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/character_mode_no_affordance.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/character_suffix_refinement.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/custom_style_spacing_strikethrough.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/hover_pair_affordance.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/hover_single_addition_affordance.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/hover_single_deletion_affordance.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/invisible_characters_debug_overlay.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/multiline_insertion_wrap.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/narrow_width_wrapping.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/punctuation_replacement.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/token_basic_replacement.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/TextDiffSnapshotTests/character_suffix_refinement.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/TextDiffSnapshotTests/custom_style_spacing_strikethrough.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/TextDiffSnapshotTests/multiline_insertion_wrap.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/TextDiffSnapshotTests/precomputed_result_rendering.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/TextDiffSnapshotTests/punctuation_replacement.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/TextDiffSnapshotTests/token_basic_replacement.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/TextDiffSnapshotTests/whitespace_only_layout_change.1.png is excluded by !**/*.png
📒 Files selected for processing (34)
  • Package.swift
  • Sources/TextDiff/TextDiff.swift
  • Sources/TextDiff/TextDiffChangeStyleDefaults.swift
  • Sources/TextDiff/TextDiffView.swift
  • Sources/TextDiffCore/DiffSegmentIndexer.swift
  • Sources/TextDiffCore/DiffTypes.swift
  • Sources/TextDiffCore/MyersDiff.swift
  • Sources/TextDiffCore/TextDiff.swift
  • Sources/TextDiffCore/TextDiffEngine.swift
  • Sources/TextDiffCore/TextDiffResult.swift
  • Sources/TextDiffCore/Tokenizer.swift
  • Sources/TextDiffIOSUI/UITextDiffView.swift
  • Sources/TextDiffMacOSUI/AppKit/DiffRevertActionResolver.swift
  • Sources/TextDiffMacOSUI/AppKit/DiffTextViewRepresentable.swift
  • Sources/TextDiffMacOSUI/AppKit/NSTextDiffContentSource.swift
  • Sources/TextDiffMacOSUI/AppKit/NSTextDiffView.swift
  • Sources/TextDiffUICommon/DiffTextLayoutMetrics.swift
  • Sources/TextDiffUICommon/DiffTokenLayouter.swift
  • Sources/TextDiffUICommon/TextDiffChangeStyle.swift
  • Sources/TextDiffUICommon/TextDiffChangeStyleDefaults.swift
  • Sources/TextDiffUICommon/TextDiffGroupStrokeStyle.swift
  • Sources/TextDiffUICommon/TextDiffPlatformTypes.swift
  • Sources/TextDiffUICommon/TextDiffStyle.swift
  • Sources/TextDiffUICommon/TextDiffStyling.swift
  • Sources/TextDiffUICommon/TextDiffViewModel.swift
  • Tests/TextDiffCoreTests/TextDiffEngineTests.swift
  • Tests/TextDiffMacOSUITests/DiffLayouterPerformanceTests.swift
  • Tests/TextDiffMacOSUITests/DiffRevertActionResolverTests.swift
  • Tests/TextDiffMacOSUITests/NSTextDiffSnapshotTests.swift
  • Tests/TextDiffMacOSUITests/NSTextDiffViewTests.swift
  • Tests/TextDiffMacOSUITests/SnapshotTestSupport.swift
  • Tests/TextDiffMacOSUITests/TextDiffSnapshotTests.swift
  • Tests/TextDiffMacOSUITests/TextDiffStyleAndLayouterTests.swift
  • Tests/TextDiffMacOSUITests/TextDiffViewModelTests.swift
💤 Files with no reviewable changes (1)
  • Sources/TextDiff/TextDiffChangeStyleDefaults.swift

Comment thread Sources/TextDiffIOSUI/UITextDiffView.swift
Comment thread Sources/TextDiffUICommon/TextDiffChangeStyleDefaults.swift
Comment thread Sources/TextDiffUICommon/TextDiffViewModel.swift Outdated
@iSapozhnik
Copy link
Copy Markdown
Owner Author

@coderabbitai review the code

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the new iOS UI file
cat Sources/TextDiffIOSUI/UITextDiffView.swift

# Check platform types
cat Sources/TextDiffUICommon/TextDiffPlatformTypes.swift

# Check TextDiff.swift re-exports
cat Sources/TextDiff/TextDiff.swift

# Check Package.swift
cat Package.swift

# Check TextDiffView.swift
cat Sources/TextDiff/TextDiffView.swift

# Check TextDiffChangeStyleDefaults in UICommon
cat Sources/TextDiffUICommon/TextDiffChangeStyleDefaults.swift

# Check TextDiffStyle
cat Sources/TextDiffUICommon/TextDiffStyle.swift

# Check if there are any TODO/FIXME comments
rg -n "TODO|FIXME|HACK|XXX" --type swift

Length of output: 32741



@iSapozhnik Here's the review of PR #8:


Summary

The modular split is solid and the cross-platform approach is clean. A few things worth fixing:


🔴 High Priority

1. @_exported import is a private Swift compiler API

Sources/TextDiff/TextDiff.swift uses @_exported import, which is not part of the stable Swift ABI. It can silently break across toolchain updates and is explicitly discouraged for public libraries. Consider documenting that consumers should import the individual modules directly, or accept the risk and add a comment.

2. DiffProvider typealias is missing public

In UITextDiffView.swift:

typealias DiffProvider = (String, String, TextDiffComparisonMode) -> [DiffSegment]

This is internal, yet the #if TESTING init takes it as a parameter. If test targets outside the module need it, it should be public.

3. TESTING flag leaks into all debug builds

In Package.swift, every non-test target defines:

.define("TESTING", .when(configuration: .debug))

This means the test-only init(original:updated:style:mode:diffProvider:) in UITextDiffView is compiled and accessible in any debug build by any consumer of the library, not just your own tests. Either use a dedicated test target or gate it behind a custom flag that only your test targets set.

4. No test coverage for TextDiffIOSUI

There's a TextDiffCoreTests and TextDiffMacOSUITests but nothing for UITextDiffView or the iOS rendering path. At minimum, basic layout and setContent tests would catch regressions.


🟡 Medium Priority

5. swift-snapshot-testing is an unconditional package dependency

It's only used in TextDiffMacOSUITests, but it's resolved on all platforms (including iOS). Move it to a conditional or platform-specific test target, or at minimum note the limitation.

6. Redundant dependencies in TextDiffMacOSUITests

dependencies: [
    "TextDiff",        // re-exports TextDiffCore, TextDiffUICommon, TextDiffMacOSUI
    "TextDiffCore",    // redundant
    "TextDiffUICommon",// redundant
    "TextDiffMacOSUI", // redundant
    ...
]

Since TextDiff already re-exports everything, listing them individually is redundant. Either drop the individual ones or drop TextDiff and keep the explicit ones (the latter is more explicit and avoids the @_exported dependency).

7. Dead code in setContent(result:style:)

public func setContent(result: TextDiffResult, style: TextDiffStyle) {
    isBatchUpdating = true
    defer {
        isBatchUpdating = false
        pendingStyleInvalidation = false  // ← reset, but never read
    }
    self.style = style                    // sets pendingStyleInvalidation = true
    apply(result: result)                 // always calls invalidateCachedLayout()
}

pendingStyleInvalidation is set to true by the style didSet, then immediately reset to false in the defer without being acted on. This is harmless because apply(result:) always invalidates layout, but it's misleading. Either use the flag or remove it from this path.


🟢 Low Priority

8. iOS 18 minimum is very restrictive

.iOS(.v18)

iOS 16 or 17 would cover significantly more devices. Unless you're using iOS 18-only APIs (not obvious from the diff), consider lowering this.

9. TextDiffStyle.default as static let won't track dynamic color changes

static let is evaluated once. The defaultTextColorValue is a dynamic system color (labelColor/.label), but since TextDiffStyle.default copies it into a stored property at first access, if a user applies .default before the window's color scheme is established, it could snap to the wrong value. Using static var would re-evaluate each time.

10. Misleading iOS preview

`#Preview`("iOS Representable Precomputed Result") {
    TextDiffIOSRepresentable(
        result: TextDiffEngine.result(...),
        original: "",   // ← ignored but confusing
        updated: "",
        ...
    )
}

The original/updated are ignored when result is non-nil, but passing empty strings here is misleading. Could be original: result.original or just omit them from the preview call for clarity.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
Sources/TextDiff/TextDiffView.swift (1)

490-491: 💤 Low value

Empty strings are confusing when result contains actual values.

The result already has the original and updated strings baked in. Passing empty strings here is misleading to anyone reading the preview. Use the actual strings for clarity.

Suggested fix
-        original: "",
-        updated: "",
+        original: "Track old values in storage.",
+        updated: "Track new values in storage.",
🤖 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/TextDiff/TextDiffView.swift` around lines 490 - 491, The preview is
passing empty strings for the original/updated text which is confusing; update
the call in TextDiffView (where original: "" and updated: "") to pass the actual
strings from the diff result (e.g., use result.original and result.updated or
the appropriate properties on the `result` object) so the preview shows real
content; locate the initializer/call that supplies original/updated in
TextDiffView.swift and replace the empty literals with the corresponding
`result` properties.
Sources/TextDiffIOSUI/UITextDiffView.swift (1)

7-7: 💤 Low value

DiffProvider typealias is internal but exposed via TESTING init.

The DiffProvider typealias has default internal access, but the #if TESTING initializer accepts it as a parameter. If tests in a separate module need this initializer, DiffProvider needs to be public. If tests are only within this module, it's fine as-is.

Also note: PR objectives mention that TESTING is set for all debug builds (.when(configuration: .debug)), so this init leaks into any consumer's debug build, not just test targets.

Also applies to: 119-139

🤖 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/TextDiffIOSUI/UITextDiffView.swift` at line 7, The DiffProvider
typealias is currently internal but is exposed as a parameter on the
UITextDiffView initializer that’s compiled under `#if` TESTING; make the access
levels consistent by marking the typealias public (change "typealias
DiffProvider = ..." to "public typealias DiffProvider = ...") so it can be used
from test modules, or alternatively restrict the TESTING-only initializer (the
UITextDiffView init under the TESTING block) to internal/private or narrow the
compile flag so it doesn’t leak into consumer debug builds—update the
DiffProvider declaration or the TESTING init accordingly.
🤖 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.

Nitpick comments:
In `@Sources/TextDiff/TextDiffView.swift`:
- Around line 490-491: The preview is passing empty strings for the
original/updated text which is confusing; update the call in TextDiffView (where
original: "" and updated: "") to pass the actual strings from the diff result
(e.g., use result.original and result.updated or the appropriate properties on
the `result` object) so the preview shows real content; locate the
initializer/call that supplies original/updated in TextDiffView.swift and
replace the empty literals with the corresponding `result` properties.

In `@Sources/TextDiffIOSUI/UITextDiffView.swift`:
- Line 7: The DiffProvider typealias is currently internal but is exposed as a
parameter on the UITextDiffView initializer that’s compiled under `#if` TESTING;
make the access levels consistent by marking the typealias public (change
"typealias DiffProvider = ..." to "public typealias DiffProvider = ...") so it
can be used from test modules, or alternatively restrict the TESTING-only
initializer (the UITextDiffView init under the TESTING block) to
internal/private or narrow the compile flag so it doesn’t leak into consumer
debug builds—update the DiffProvider declaration or the TESTING init
accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 00b917c8-8146-4486-b554-a616cbc26b7b

📥 Commits

Reviewing files that changed from the base of the PR and between 34f69f0 and b1a411e.

⛔ Files ignored due to path filters (4)
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/custom_style_spacing_strikethrough.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/hover_pair_affordance.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/NSTextDiffSnapshotTests/narrow_width_wrapping.1.png is excluded by !**/*.png
  • Tests/TextDiffMacOSUITests/__Snapshots__/TextDiffSnapshotTests/custom_style_spacing_strikethrough.1.png is excluded by !**/*.png
📒 Files selected for processing (10)
  • Package.swift
  • README.md
  • Sources/TextDiff/TextDiffView.swift
  • Sources/TextDiffIOSUI/UITextDiffView.swift
  • Sources/TextDiffMacOSUI/AppKit/NSTextDiffView.swift
  • Sources/TextDiffUICommon/DiffRevertActionResolver.swift
  • Sources/TextDiffUICommon/DiffRevertHitResolver.swift
  • Sources/TextDiffUICommon/TextDiffViewModel.swift
  • Tests/TextDiffUICommonTests/DiffRevertActionResolverTests.swift
  • Tests/TextDiffUICommonTests/DiffRevertHitResolverTests.swift
💤 Files with no reviewable changes (1)
  • Sources/TextDiffUICommon/TextDiffViewModel.swift
✅ Files skipped from review due to trivial changes (1)
  • README.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • Package.swift

- Keep indexed segment cursors aligned with source text when equal display segments are synthetic
- Refresh UITextDiffView content immediately after revert actions
- Reject stale revert candidates whose updated fragment no longer matches
- Add regression coverage for shifted ranges
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Sources/TextDiffCore/DiffSegmentIndexer.swift (1)

35-50: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace the ineffective assert checks with a release-active diagnostic

In Sources/TextDiffCore/DiffSegmentIndexer.swift (lines 35-50), the assert(...) calls are inside #if !TESTING, and TESTING is defined for the TextDiffCore and TextDiffUICommon targets only for .debug, so those assertions don’t compile in Debug. In Release (-O), Swift assert() is compiled out/no-op, so mismatches produce no signal in any build.

  • Drop the dead asserts, or replace them with a check that stays active in Release (e.g., precondition/preconditionFailure or a lightweight logging/error path).
♻️ Remove the ineffective asserts
                 let originalMatches = textMatches(segment.text, source: originalNSString, at: originalCursor)
                 let updatedMatches = textMatches(segment.text, source: updatedNSString, at: updatedCursor)
-                `#if` !TESTING
-                assert(
-                    originalMatches,
-                    "Equal segment text mismatch in original at \(originalCursor) for segment \(index): \(segment.text)"
-                )
-                assert(
-                    updatedMatches,
-                    "Equal segment text mismatch in updated at \(updatedCursor) for segment \(index): \(segment.text)"
-                )
-                `#endif`
                 if originalMatches {
                     originalCursor += textLength
                 }
                 if updatedMatches {
                     updatedCursor += textLength
                 }
🤖 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/TextDiffCore/DiffSegmentIndexer.swift` around lines 35 - 50, The
assert(...) checks in DiffSegmentIndexer.swift around
originalMatches/updatedMatches are ineffective in some builds; replace them with
release-active diagnostics: change the assert calls that reference
originalMatches, updatedMatches, originalCursor, updatedCursor, segment.text and
textLength to use precondition or call preconditionFailure with the same
descriptive messages (so mismatches raise in Release), or alternatively log and
throw a fatal error if that fits your error model; ensure the checks occur
before advancing originalCursor/updatedCursor in the same function (e.g., in the
scope where those asserts currently live).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@Sources/TextDiffCore/DiffSegmentIndexer.swift`:
- Around line 35-50: The assert(...) checks in DiffSegmentIndexer.swift around
originalMatches/updatedMatches are ineffective in some builds; replace them with
release-active diagnostics: change the assert calls that reference
originalMatches, updatedMatches, originalCursor, updatedCursor, segment.text and
textLength to use precondition or call preconditionFailure with the same
descriptive messages (so mismatches raise in Release), or alternatively log and
throw a fatal error if that fits your error model; ensure the checks occur
before advancing originalCursor/updatedCursor in the same function (e.g., in the
scope where those asserts currently live).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3017b79e-7ea9-42a4-8acc-8771d51c22b2

📥 Commits

Reviewing files that changed from the base of the PR and between b1a411e and 50bce44.

📒 Files selected for processing (4)
  • Sources/TextDiffCore/DiffSegmentIndexer.swift
  • Sources/TextDiffIOSUI/UITextDiffView.swift
  • Sources/TextDiffUICommon/DiffRevertActionResolver.swift
  • Tests/TextDiffUICommonTests/DiffRevertActionResolverTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
  • Sources/TextDiffIOSUI/UITextDiffView.swift

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant