Improve compact diff API #24

2025-11-16T12:32:43Z

schmelczer commented

2025-11-16 12:32:43 +00:00

(Migrated from github.com)

Features

Stop depending on serde within wasm
Return a JS array instead of a JSON string from WASM
Add the inverse function for diff (undiff)

Breaks

Remove the irrelevant is_binary from the API
Remove cursors from diff-s

Additional changes

Refactor & improve docs
Convert isize::MAX wrapping into explicit panics

## Features - Stop depending on serde within wasm - Return a JS array instead of a JSON string from WASM - Add the inverse function for diff (undiff) ## Breaks - Remove the irrelevant `is_binary` from the API - Remove cursors from diff-s ## Additional changes - Refactor & improve docs - Convert `isize::MAX` wrapping into explicit panics

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-11-16 13:08:21 +00:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

This PR refactors the compact diff API to remove dependencies on serde within WASM and improve the API design. The key changes include removing the deprecated is_binary function, simplifying the diff representation by removing cursor positions, and returning native JavaScript arrays instead of JSON strings from WASM.

Key changes:

Replaced ChangeSet with NumberOrString enum for a simpler diff representation
Removed is_binary utility and integrated its logic directly into WASM functions
Changed WASM getCompactDiff to return JavaScript arrays instead of JSON strings

Reviewed Changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/wasm.rs	Updated tests to verify the new array-based return format from `getCompactDiff`
tests/test.rs	Updated test names and assertions to use new `to_changes`/`from_changes` API
src/wasm.rs	Refactored `get_compact_diff` to return `Vec<JsValue>`, removed `is_binary` export, integrated binary detection inline
src/utils/is_binary.rs	Removed deprecated `is_binary` utility module
src/utils.rs	Removed reference to deleted `is_binary` module
src/types/number_or_string.rs	Added new `NumberOrString` enum with WASM and serde support
src/types.rs	Added reference to new `number_or_string` module
src/operation_transformation/transport.rs	Removed deprecated `ChangeSet` and `SimpleOperation` types
src/operation_transformation/edited_text.rs	Replaced `to_change_set`/`from_change_set` with `to_changes`/`from_changes` using `NumberOrString`
src/operation_transformation.rs	Removed export of deprecated `ChangeSet`
src/lib.rs	Updated public API exports and documentation examples
scripts/test.sh	Added more granular feature flag testing
reconcile-js/src/index.ts	Updated TypeScript bindings to reflect new array return type and removed `isBinary` function
examples/website/src/index.html	Formatted script tag for consistency
Cargo.toml	Removed `serde_json` dependency from wasm feature, updated feature flags

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull Request Overview This PR refactors the compact diff API to remove dependencies on serde within WASM and improve the API design. The key changes include removing the deprecated `is_binary` function, simplifying the diff representation by removing cursor positions, and returning native JavaScript arrays instead of JSON strings from WASM. **Key changes:** - Replaced `ChangeSet` with `NumberOrString` enum for a simpler diff representation - Removed `is_binary` utility and integrated its logic directly into WASM functions - Changed WASM `getCompactDiff` to return JavaScript arrays instead of JSON strings ### Reviewed Changes Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | tests/wasm.rs | Updated tests to verify the new array-based return format from `getCompactDiff` | | tests/test.rs | Updated test names and assertions to use new `to_changes`/`from_changes` API | | src/wasm.rs | Refactored `get_compact_diff` to return `Vec<JsValue>`, removed `is_binary` export, integrated binary detection inline | | src/utils/is_binary.rs | Removed deprecated `is_binary` utility module | | src/utils.rs | Removed reference to deleted `is_binary` module | | src/types/number_or_string.rs | Added new `NumberOrString` enum with WASM and serde support | | src/types.rs | Added reference to new `number_or_string` module | | src/operation_transformation/transport.rs | Removed deprecated `ChangeSet` and `SimpleOperation` types | | src/operation_transformation/edited_text.rs | Replaced `to_change_set`/`from_change_set` with `to_changes`/`from_changes` using `NumberOrString` | | src/operation_transformation.rs | Removed export of deprecated `ChangeSet` | | src/lib.rs | Updated public API exports and documentation examples | | scripts/test.sh | Added more granular feature flag testing | | reconcile-js/src/index.ts | Updated TypeScript bindings to reflect new array return type and removed `isBinary` function | | examples/website/src/index.html | Formatted script tag for consistency | | Cargo.toml | Removed `serde_json` dependency from wasm feature, updated feature flags | </details> --- 💡 <a href="/schmelczer/reconcile/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

src/operation_transformation/edited_text.rs

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-11-16 13:08:20 +00:00

Using i64::MAX as a fallback for overflow in length conversion is problematic. If prev_length exceeds i64::MAX, this will create an incorrect positive number instead of properly handling the error. Consider using a saturating conversion or returning an error instead.

Using `i64::MAX` as a fallback for overflow in length conversion is problematic. If `prev_length` exceeds `i64::MAX`, this will create an incorrect positive number instead of properly handling the error. Consider using a saturating conversion or returning an error instead.

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-11-16 13:08:20 +00:00

Using i64::MAX as a fallback and then negating it will result in i64::MIN + 1 due to overflow, creating incorrect delete operation sizes. This should handle overflow errors explicitly or use saturating arithmetic.

                    let count = if let Ok(val) = i64::try_from(*deleted_character_count) {
                        val
                    } else {
                        i64::MAX
                    };
                    // Use saturating_neg to avoid overflow when negating i64::MAX
                    result.push(NumberOrString::Number(count.saturating_neg()));

Using `i64::MAX` as a fallback and then negating it will result in `i64::MIN + 1` due to overflow, creating incorrect delete operation sizes. This should handle overflow errors explicitly or use saturating arithmetic. ```suggestion let count = if let Ok(val) = i64::try_from(*deleted_character_count) { val } else { i64::MAX }; // Use saturating_neg to avoid overflow when negating i64::MAX result.push(NumberOrString::Number(count.saturating_neg())); ```

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-11-16 13:08:21 +00:00

Using usize::MAX as a fallback when converting from i64 to usize can cause incorrect behavior. If the conversion fails due to a negative number that passed the >= 0 check, this creates an enormous positive length. Use proper error handling instead.

Using `usize::MAX` as a fallback when converting from `i64` to `usize` can cause incorrect behavior. If the conversion fails due to a negative number that passed the `>= 0` check, this creates an enormous positive length. Use proper error handling instead.

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-11-16 13:08:21 +00:00

Using usize::MAX as a fallback for conversion failures can lead to incorrect delete operation sizes. This needs proper error handling to avoid creating operations with incorrect lengths.

                        match usize::try_from(-length) {
                            Ok(length) => {
                                operations.push(Operation::create_delete(order, length));
                                order += length;
                            }
                            Err(e) => {
                                eprintln!("Warning: Failed to convert delete length ({}): {}", -length, e);
                                // Skip this operation or handle as needed
                            }
                        }

Using `usize::MAX` as a fallback for conversion failures can lead to incorrect delete operation sizes. This needs proper error handling to avoid creating operations with incorrect lengths. ```suggestion match usize::try_from(-length) { Ok(length) => { operations.push(Operation::create_delete(order, length)); order += length; } Err(e) => { eprintln!("Warning: Failed to convert delete length ({}): {}", -length, e); // Skip this operation or handle as needed } } ```

src/wasm.rs

					
				@ -116,3 +117,4 @@

				    .text()

				}

				fn set_panic_hook() {

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-11-16 13:08:20 +00:00

[nitpick] The nested if-let pattern could be simplified using early returns. Consider checking each parameter separately and returning early for invalid inputs, which would improve readability and reduce nesting.

    let parent = match string_or_nothing(parent) {
        Some(p) => p,
        None => return right.to_vec(),
    };
    let left = match string_or_nothing(left) {
        Some(l) => l,
        None => return right.to_vec(),
    };
    let right_str = match string_or_nothing(right) {
        Some(r) => r,
        None => return right.to_vec(),
    };

    crate::reconcile(&parent, &left.into(), &right_str.into(), &*tokenizer)
        .apply()
        .text()
        .into_bytes()

[nitpick] The nested if-let pattern could be simplified using early returns. Consider checking each parameter separately and returning early for invalid inputs, which would improve readability and reduce nesting. ```suggestion let parent = match string_or_nothing(parent) { Some(p) => p, None => return right.to_vec(), }; let left = match string_or_nothing(left) { Some(l) => l, None => return right.to_vec(), }; let right_str = match string_or_nothing(right) { Some(r) => r, None => return right.to_vec(), }; crate::reconcile(&parent, &left.into(), &right_str.into(), &*tokenizer) .apply() .text() .into_bytes() ```

tests/wasm.rs

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-11-16 13:08:19 +00:00

The .clone() calls are unnecessary here. try_into() can consume the value directly from indexing. Remove .clone() to avoid unnecessary allocations.

    let second: String = result[1].try_into().unwrap();

The `.clone()` calls are unnecessary here. `try_into()` can consume the value directly from indexing. Remove `.clone()` to avoid unnecessary allocations. ```suggestion let second: String = result[1].try_into().unwrap(); ```

Sign in to join this conversation.

No reviewers