feat: implement observe/observe_deep #76
Conversation
satoren
force-pushed
the
observe
branch
4 times, most recently
from
4c1c70f to
3888ce4
Compare
WalkthroughThe pull request introduces several enhancements to the Yex library, including new functions for observing shared types in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
satoren
force-pushed
the
observe
branch
5 times, most recently
from
24cf968 to
c634c05
Compare
satoren
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (52)
native/yex/src/subscription.rs (1)
11-15: LGTM: Thread-safe resource implementationThe
arcmethod correctly implements a thread-safe wrapper for subscriptions usingMutexandResourceArc. This pattern ensures safe sharing of subscription resources across thread boundaries.Consider documenting the thread-safety guarantees and usage patterns for future maintainers, as this is a critical part of the subscription system's architecture.
test/shared_type/shared_type_test.exs (1)
5-51: Consider refactoring test cases for better maintainabilityWhile the error cases are comprehensive, the test structure could be improved for better maintainability and readability.
Consider refactoring using test case parameterization:
- test "raise error when wrong usage" do + describe "invalid inputs" do + test_cases = [ + {1, "integer"}, + {make_ref(), "reference"} + ] + + for {input, type} <- test_cases do + test "raises Protocol.UndefinedError for #{type}" do + doc = Doc.new() + + for {function, args} <- [ + {:unobserve, [input]}, + {:unobserve_deep, [input]}, + {:observe, [input]}, + {:observe, [input, []]}, + {:observe_deep, [input]}, + {:observe_deep, [input, []]} + ] do + assert_raise Protocol.UndefinedError, fn -> + apply(SharedType, function, args) + end + end + end + end + endThis refactoring would:
- Make it easier to add new test cases
- Reduce code duplication
- Provide clearer test descriptions
- Make the test structure more maintainable
lib/shared_type/event.ex (7)
5-7: Fix incorrect @see references in documentationThe @see tags reference incorrect function arities:
Yex.Array.observe/1should beobserve/2Yex.Array.observe_deep/1should beobserve_deep/2
15-19: Improve type specifications for better clarityConsider these improvements:
- The
changetype union could be more explicitly defined as a custom type- Document the purpose of the empty map
%{}in the change union type+ @type array_change :: + %{insert: list()} | + %{delete: number()} | + %{} # represents no change + @type t :: %__MODULE__{ path: list(number() | String.t()), target: Yex.Array.t(), - change: %{insert: list()} | %{delete: number()} | %{} + change: array_change() }
27-29: Fix incorrect @see references in documentationThe @see tags reference incorrect function arities:
Yex.Map.observe/1should beobserve/2Yex.Map.observe_deep/1should beobserve_deep/2
41-47: Improve type documentation for the keys fieldThe
keystype definition needs more clarity:
- The purpose of mapping to an empty map is not clear
- Consider adding a type description comment
- @type keys :: %{String.t() => %{}} + @typedoc """ + Represents the changed keys in the map. + The empty map value indicates the key was involved in the change. + """ + @type keys :: %{String.t() => %{}} @type t :: %__MODULE__{ path: list(number() | String.t()), target: Yex.Map.t(), keys: keys }
55-57: Fix incorrect @see references in documentationThe @see tags reference incorrect function arities:
Yex.Text.observe/1should beobserve/2
77-79: Fix incorrect @see references in XML documentationThe @see tags reference Text.observe instead of XML-related functions. Consider updating to reference the appropriate XML observation functions.
88-93: Standardize keys type definition across event typesThe
keysfield type inXmlEventdiffers fromMapEvent:
MapEventuses a customkeystype with a map of empty mapsXmlEventreuses the array-style change type
Consider standardizing this approach across event types.lib/shared_type/xml_fragment.ex (1)
102-114: Consider enhancing documentation and cleanup mechanismThe implementation looks correct but could benefit from some improvements:
- The function stores the subscription in the process dictionary but there's no documented cleanup mechanism. This could lead to memory leaks if subscriptions aren't properly cleaned up.
- The
:metadataoption usage should be documented.Consider:
- Adding documentation about how to unsubscribe/cleanup the observation
- Documenting the available options and their usage
- Adding a complementary
unobserve/1functionExample documentation enhancement:
@doc """ see `Yex.SharedType.observe/2` + + ## Options + + * `:metadata` - Additional metadata to be included with observation events + + ## Cleanup + + To stop observing and cleanup the subscription, use `unobserve/1` with the returned reference. """lib/shared_type/map.ex (2)
118-120: Documentation could be more comprehensive.While the documentation references external modules, it would be helpful to include:
- Example usage showing how to handle observation events
- List of events that can be observed
- Description of the difference between
observe/2andobserve_deep/2Example documentation structure:
@doc """ Observes changes to the map's direct key-value pairs. ## Events - `:insert` - Triggered when a new key-value pair is added - `:update` - Triggered when a value is modified - `:delete` - Triggered when a key-value pair is removed ## Examples iex> doc = Yex.Doc.new() iex> map = Yex.Doc.get_map(doc, "map") iex> ref = Yex.Map.observe(map) iex> Yex.Map.set(map, "key", "value") iex> receive do ...> {:insert, ^ref, "key", "value", metadata} -> :ok ...> after ...> 100 -> :error ...> end :ok """Also applies to: 129-131
124-125: Consider adding error handling for NIF operations.The NIF calls (
map_observeandmap_observe_deep) could potentially fail. Consider wrapping them in atry-rescueblock or handling potential errors.Example implementation:
- sub = Yex.Nif.map_observe(map, cur_txn(map), self(), ref, Keyword.get(opt, :metadata)) - Process.put(ref, sub) + case Yex.Nif.map_observe(map, cur_txn(map), self(), ref, Keyword.get(opt, :metadata)) do + {:ok, sub} -> + Process.put(ref, sub) + {:ok, ref} + {:error, reason} -> + {:error, reason} + endAlso applies to: 135-136
lib/shared_type/array.ex (3)
161-170: Consider adding cleanup mechanism and improving documentationThe implementation looks functionally correct, but there are a few considerations:
- The subscription stored in the process dictionary should have a cleanup mechanism to prevent memory leaks
- The
:metadataoption should be documented in the function's documentationConsider adding:
- A cleanup function to remove the subscription:
@doc """ Stops observing changes for the given reference. """ @spec unobserve(reference()) :: :ok def unobserve(ref) do if sub = Process.delete(ref) do Yex.Nif.array_unobserve(sub) end :ok end
- Enhanced documentation:
@doc """ Observes changes to the array. ## Options * `:metadata` - Additional metadata to be included with change events """
172-174: Enhance documentation to clarify deep observation behaviorThe current documentation only references
Yex.SharedType.observe_deep/2without explaining the difference between regular and deep observation.Consider expanding the documentation:
@doc """ Observes deep changes to the array and its nested elements. Deep observation will track changes to nested arrays and maps, unlike regular observation which only tracks changes to the immediate elements. See `Yex.SharedType.observe_deep/2` for more details. ## Options * `:metadata` - Additional metadata to be included with change events """
178-178: Remove unnecessary blank lineFor consistent formatting, remove the extra newline after
make_ref().test/shared_type/xml_text_test.exs (3)
152-170: Consider adding complementary test scenarios.While the current test effectively verifies the basic observation functionality, consider adding:
- Cleanup test to verify unsubscribe behavior
- Negative test cases (e.g., multiple observers, invalid operations)
Example structure for cleanup test:
test "observe cleanup", %{doc: doc, xml_text: text} do ref = SharedType.observe(text) :ok = SharedType.unobserve(text, ref) Doc.transaction(doc, "origin_value", fn -> XmlText.insert(text, 0, "123456") end) refute_receive {:observe_event, ^ref, _, _, _} end
172-172: Fix test name formatting.Remove the trailing space in the test name for consistency with other test names.
- test "observe delete ", %{doc: doc, xml_text: text} do + test "observe delete", %{doc: doc, xml_text: text} do
189-210: Enhance deep observation test coverage.Consider the following improvements:
- Verify the metadata content instead of using underscore pattern match
- Add test cases for nested structure changes to fully validate deep observation
Example structure for nested changes:
test "observe_deep with nested changes", %{doc: doc, xml_text: text} do ref = SharedType.observe_deep(text) :ok = Doc.transaction(doc, "origin_value", fn -> XmlText.insert(text, 0, "123") XmlText.format(text, 0, 3, %{"bold" => true}) # Add nested element changes here when supported end) assert_receive {:observe_deep_event, ^ref, [ %Yex.XmlTextEvent{ path: [], target: ^text, delta: [ %{attributes: %{"bold" => true}, insert: "123"} ] }, # Assert nested changes here ], "origin_value", metadata} # Verify metadata contents assert is_map(metadata) # Add specific metadata assertions endlib/shared_type/xml_element.ex (3)
144-146: Enhance documentation with more details.The current documentation only references
Yex.SharedType.observe/2without explaining the purpose, usage, or return value of this function. Consider adding:
- Purpose of observation
- Example usage
- Description of the optional keyword arguments
- Description of the events that will be received
147-156: Consider memory management for Process dictionary storage.The subscription is stored in the Process dictionary but there's no clear cleanup mechanism. This could lead to memory leaks if observers aren't properly unsubscribed.
Consider:
- Adding an
unobserve/1function to clean up subscriptions- Documenting the cleanup responsibility
- Using a supervised GenServer or Registry instead of Process dictionary for better lifecycle management
158-174: Clarify function visibility and consider code reuse.A few concerns about this implementation:
- The function is marked with
@doc falsebut has a public spec - this is inconsistent. If it's intended to be internal, consider making it private withdefp.- There's significant code duplication with
observe/2. Consider extracting the common pattern:- @doc false - @spec observe_deep(t, keyword()) :: reference() - def observe_deep(%__MODULE__{} = xml, opt \\ []) do - ref = make_ref() - - sub = - Yex.Nif.xml_element_observe_deep( - xml, - cur_txn(xml), - self(), - ref, - Keyword.get(opt, :metadata) - ) - - Process.put(ref, sub) - ref - end + defp do_observe(xml, opt, observer_fn) do + ref = make_ref() + sub = observer_fn.(xml, cur_txn(xml), self(), ref, Keyword.get(opt, :metadata)) + Process.put(ref, sub) + ref + end + + @doc false + @spec observe_deep(t, keyword()) :: reference() + def observe_deep(%__MODULE__{} = xml, opt \\ []) do + do_observe(xml, opt, &Yex.Nif.xml_element_observe_deep/5) + endThis would also simplify the
observe/2function:@spec observe(t, keyword()) :: reference() def observe(%__MODULE__{} = xml, opt \\ []) do - ref = make_ref() - - sub = - Yex.Nif.xml_element_observe(xml, cur_txn(xml), self(), ref, Keyword.get(opt, :metadata)) - - Process.put(ref, sub) - ref + do_observe(xml, opt, &Yex.Nif.xml_element_observe/5) endnative/yex/src/text.rs (3)
215-224: Add documentation for the text_observe function.Consider adding documentation that explains:
- The purpose of the observation mechanism
- The meaning and expected format of each parameter
- The subscription lifecycle
Add documentation like this:
/// Observes changes to the text content. /// /// # Parameters /// * `text` - The text to observe /// * `current_transaction` - Optional ongoing transaction /// * `pid` - Process ID to receive notifications /// * `ref_term` - Reference term for correlation /// * `metadata` - Additional metadata for the subscription /// /// # Returns /// A subscription resource that will be cleaned up when dropped
227-235: Add documentation for the text_observe_deep function.Similar to
text_observe, this function needs documentation explaining how deep observation differs from regular observation.Add documentation like this:
/// Deeply observes changes to the text content and its nested structures. /// /// Unlike `text_observe`, this function also tracks changes in nested data structures /// and attributes. /// /// # Parameters /// * `text` - The text to observe /// * `current_transaction` - Optional ongoing transaction /// * `pid` - Process ID to receive notifications /// * `ref_term` - Reference term for correlation /// * `metadata` - Additional metadata for the subscription /// /// # Returns /// A subscription resource that will be cleaned up when dropped
215-235: Consider adding parameter validation.Both observation functions could benefit from basic parameter validation:
- Ensure the text reference is valid
- Validate metadata format if there are expectations
Example implementation:
fn text_observe( text: NifText, current_transaction: Option<ResourceArc<TransactionResource>>, pid: rustler::LocalPid, ref_term: Term<'_>, metadata: Term<'_>, ) -> NifResult<ResourceArc<SubscriptionResource>> { // Validate text reference if !text.is_valid() { return Err(rustler::Error::Term(Box::new("Invalid text reference"))); } // Validate metadata if needed if !metadata.is_map() { return Err(rustler::Error::Term(Box::new("Metadata must be a map"))); } text.observe(current_transaction, pid, ref_term, metadata) }test/shared_type/text_test.exs (4)
174-196: Consider adding boundary test cases.While the test covers basic text insertion with attributes, consider adding test cases for:
- Empty string insertions
- Unicode character handling
- Maximum text length scenarios
198-216: Add test cases for overlapping operations.The delete test verifies basic deletion, but should also test:
- Deleting at boundaries (start/end of text)
- Deleting with overlapping formatting attributes
239-261: Add test cases for nested structure changes.While the test verifies basic deep observation, consider adding tests for:
- Nested attribute changes
- Multiple levels of depth in the text structure
- Concurrent modifications at different depths
263-281: Consider adding tests for selective unobserve_deep.The test covers basic unobserve_deep functionality well. Consider adding tests for:
- Unobserving specific paths while keeping others observed
- Verifying that sibling paths remain observed after partial unobserve
lib/protocols/awareness.ex (1)
1-5: Enhance module documentation to reflect new capabilitiesWhile the documentation is concise, it could be more comprehensive given the module's importance in the observation system. Consider adding:
- A brief overview of the awareness protocol's purpose and benefits
- Examples of common use cases
- Integration points with the new observation capabilities being added in this PR
@moduledoc """ - Awareness is an optional feature that works well together with Yjs. + Awareness is an optional feature that works well together with Yjs, providing real-time + presence and state synchronization capabilities across connected peers. + + ## Overview + The Awareness protocol enables: + - Real-time state synchronization between clients + - Presence detection and monitoring + - Integration with observation systems for state changes + + ## Common Use Cases + - Collaborative editing presence indicators + - User status synchronization + - Real-time cursor positions + + ## Integration with Observation System + The Awareness module works seamlessly with the observation system, allowing you to: + - Monitor state changes using `monitor_change/2` + - Track updates via `monitor_update/2` + - Integrate with other observable types like `Yex.Array` and `Yex.Map` """test/shared_type/xml_element_test.exs (3)
155-169: Consider expanding test coverageWhile the basic observation test is good, consider adding tests for:
- Multiple attribute changes in a single transaction
- Observer cleanup/unsubscribe functionality
- Edge cases (empty values, special characters)
171-171: Remove extra space in test nameThe test name "observe delete " contains a trailing space.
- test "observe delete ", %{doc: doc, xml_element: xml_element} do + test "observe delete", %{doc: doc, xml_element: xml_element} do
201-203: Use more descriptive variable namesConsider renaming variables to better describe their role in the structure:
- el2 = XmlElement.first_child(xml_element) - el3 = XmlElement.first_child(el2) - text = XmlElement.first_child(el3) + outer_span = XmlElement.first_child(xml_element) + inner_span = XmlElement.first_child(outer_span) + text_node = XmlElement.first_child(inner_span)lib/nif.ex (3)
43-46: Add documentation for text observation functions.Consider adding
@doccomments to describe:
- The purpose and behavior of these observation functions
- The expected format and usage of the metadata parameter
- The difference between regular and deep observation
74-77: Add type specifications for map observation functions.Consider adding
@specdeclarations to clearly define the expected types for:
- The map reference
- The transaction context
- The metadata parameter
- The return value
Line range hint
43-157: Document the observation pattern architecture.Consider adding module-level documentation to describe:
- The overall observation pattern and its implementation across different data types
- The difference between regular and deep observation behaviors
- The event propagation mechanism and how it integrates with the Rust NIF layer
- Guidelines for implementing new observable types in the future
This documentation would help maintain consistency as the codebase grows and new observable types are added.
native/yex/src/map.rs (1)
121-141: Consider adding documentation for the new observation functionsAdding doc comments to
map_observeandmap_observe_deepwould enhance code clarity and assist other developers in understanding their usage and parameters.lib/shared_type/shared_type.ex (5)
8-9: Fix grammatical error in documentationThe sentence should read: "Registers a change observer that will be messaged every time this shared type is modified."
26-27: Fix grammatical error in documentationThe sentence should read: "Registers a change observer that will be messaged every time this shared type is modified."
45-46: Fix grammatical error in documentationThe sentence should read: "Registers a change observer that will be messaged every time this shared type or any of its children is modified."
62-63: Fix grammatical error in documentationThe sentence should read: "Registers a change observer that will be messaged every time this shared type or any of its children is modified."
54-54: Fix grammatical errors in documentationChange "is a array of event struct that describes the change;" to "is an array of event structs that describe the changes;"
test/shared_type/map_test.exs (1)
116-116: Remove trailing whitespace in test namesThe test names on lines 116 and 139 have trailing spaces after
"set "and"delete ". Consider removing the extra spaces for consistency and clarity.Also applies to: 139-139
test/shared_type/xml_fragment_test.exs (1)
173-223: Refactorobserve_deeptest for improved readabilityThe
observe_deeptest involves complex nested structures and multiple operations within a transaction. To enhance readability and maintainability, consider the following suggestions:
- Extract helper functions: Break down the creation of nested XML elements and the manipulation within the transaction into separate helper functions.
- Add descriptive comments: Include comments explaining each step of the test to guide future reviewers and maintainers.
Apply this diff to extract helper functions:
defmodule YexXmlFragmentTest do # ... + defp setup_nested_xml(f) do + XmlFragment.push( + f, + XmlElementPrelim.new("span", [ + XmlElementPrelim.new("span", [ + XmlTextPrelim.from("text") + ]) + ]) + ) + end + + defp perform_deep_operations(doc, f, el2, el3, text) do + Doc.transaction(doc, "origin_value", fn -> + XmlFragment.push(f, XmlTextPrelim.from("1")) + XmlElement.insert_attribute(el2, "attr", "value") + XmlElement.push(el3, XmlElementPrelim.empty("div")) + XmlText.insert(text, 0, "text") + end) + end test "observe_deep", %{doc: doc, xml_fragment: f} do - XmlFragment.push( - f, - XmlElementPrelim.new("span", [ - XmlElementPrelim.new("span", [ - XmlTextPrelim.from("text") - ]) - ]) - ) + setup_nested_xml(f) el2 = XmlFragment.first_child(f) el3 = XmlElement.first_child(el2) text = XmlElement.first_child(el3) ref = SharedType.observe_deep(f) - :ok = - Doc.transaction(doc, "origin_value", fn -> - XmlFragment.push(f, XmlTextPrelim.from("1")) - XmlElement.insert_attribute(el2, "attr", "value") - XmlElement.push(el3, XmlElementPrelim.empty("div")) - XmlText.insert(text, 0, "text") - end) + perform_deep_operations(doc, f, el2, el3, text) assert_receive {:observe_deep_event, ^ref, [ %Yex.XmlEvent{ path: [], target: ^f, keys: %{}, delta: [%{retain: 1}, %{insert: [%Yex.XmlText{}]}] }, %Yex.XmlEvent{ path: [0], target: ^el2, keys: %{"attr" => %{action: :add, new_value: "value"}}, delta: [] }, %Yex.XmlEvent{ keys: %{}, path: [0, 0], target: ^el3, delta: [%{retain: 1}, %{insert: [%Yex.XmlElement{}]}] }, %Yex.XmlTextEvent{ path: [0, 0, 0], target: ^text, delta: [%{insert: "text"}] } ], "origin_value", _metadata} end endtest/shared_type/array_test.exs (1)
215-216: Consistent Use of Array Keys in TestsIn the tests, the array is retrieved using different keys such as
"text","data", and"array". For better consistency and clarity across all tests, consider standardizing the key used when callingDoc.get_array/2. Using a consistent key name can improve readability and maintainability.Also applies to: 234-235, 254-255, 273-274, 292-293, 337-338
native/yex/src/xml.rs (1)
203-223: Refactor duplicatedobserveandobserve_deepfunctionsThe
observeandobserve_deepfunctions forNifXmlFragment,NifXmlElement, andNifXmlTexthave nearly identical implementations. Consider abstracting the common logic into a generic function or macro to reduce code duplication and enhance maintainability.Also applies to: 391-411, 565-585
native/yex/src/event.rs (7)
128-132: Consider removing unnecessaryDecoderimplementationThe
Decoderimplementation forNifYArrayChangeis unimplemented usingunimplemented!(). If decoding is not required for this struct, consider removing theDecoderimplementation entirely to avoid confusion.
210-214: Consider removing unnecessaryDecoderimplementationThe
Decoderimplementation forNifYTextDeltais unimplemented. If decoding is unnecessary for this struct, it's better to remove theDecoderimplementation to maintain code clarity.
285-289: Consider removing unnecessaryDecoderimplementationThe
DecoderforNifYMapChangeis currently unimplemented. Remove it if decoding isn't needed for this struct to keep the codebase clean.
49-51: Simplify match expression usingimpl FromThe
Fromimplementation forPathSegmentcan be simplified for clarity.Apply the following change:
fn from(value: yrs::types::PathSegment) -> Self { match value { - yrs::types::PathSegment::Key(key) => PathSegment::Key(key.to_string()), + yrs::types::PathSegment::Key(key) => Self::Key(key.to_string()), - yrs::types::PathSegment::Index(index) => PathSegment::Index(index), + yrs::types::PathSegment::Index(index) => Self::Index(index), } }This utilizes
Selfto make the code more maintainable.
77-85: Enhance error handling inDecoderimplementation forNifPathIn the
DecoderforNifPath, consider providing more informative error messages instead of usingBadArg. This will help users understand why decoding failed.Modify the error handling as follows:
else { - Err(rustler::Error::BadArg) + Err(rustler::Error::Atom("invalid_path_segment")) }This change provides a specific error atom that can be matched on the Erlang side.
162-208: Optimize by avoiding unnecessary cloning ofdeltavectorIn the
Encoderimplementation forNifYTextDelta, thedeltavector is cloned before iteration. Avoiding this clone can improve performance.Apply the following change:
- self.delta.clone().into_iter() + self.delta.iter()Adjust subsequent code to work with references as needed.
237-283: Avoid unnecessary cloning ofchangeHashMapIn
NifYMapChange'sEncoder, cloning thechangeHashMap before iteration may not be necessary and can be avoided to enhance performance.Adjust the code as follows:
- self.change.clone().into_iter() + self.change.iter()Ensure the iteration works with references after this modification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
lib/nif.ex(5 hunks)lib/protocols/awareness.ex(1 hunks)lib/shared_type/array.ex(1 hunks)lib/shared_type/event.ex(1 hunks)lib/shared_type/map.ex(2 hunks)lib/shared_type/shared_type.ex(1 hunks)lib/shared_type/text.ex(1 hunks)lib/shared_type/xml_element.ex(1 hunks)lib/shared_type/xml_fragment.ex(1 hunks)lib/shared_type/xml_text.ex(1 hunks)native/yex/src/array.rs(3 hunks)native/yex/src/atoms.rs(2 hunks)native/yex/src/event.rs(1 hunks)native/yex/src/lib.rs(1 hunks)native/yex/src/map.rs(3 hunks)native/yex/src/subscription.rs(1 hunks)native/yex/src/text.rs(3 hunks)native/yex/src/xml.rs(8 hunks)test/shared_type/array_test.exs(2 hunks)test/shared_type/map_test.exs(2 hunks)test/shared_type/shared_type_test.exs(1 hunks)test/shared_type/text_test.exs(2 hunks)test/shared_type/xml_element_test.exs(2 hunks)test/shared_type/xml_fragment_test.exs(2 hunks)test/shared_type/xml_text_test.exs(2 hunks)
🔇 Additional comments (51)
native/yex/src/atoms.rs (2)
11-12: LGTM: Event type atoms follow consistent naming.
The new observation event atoms are well-named and align with the PR's objective of implementing observation capabilities.
28-32: LGTM: Action-related atoms are logically grouped.
The new action-related atoms (action, old_value, new_value, add, update) are appropriately placed and follow the established pattern. They provide the necessary metadata fields for tracking changes in the observation system.
Let's verify the usage of these new atoms:
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient evidence showing the usage of these atoms in the codebase.
Verified: Action-related atoms are correctly used in event handling
The atoms are properly used in the event system, particularly in native/yex/src/event.rs where they're used to represent different types of changes:
action,new_valuefor add operationsaction,old_valuefor delete operationsaction,old_value,new_valuefor update operations
The test files confirm these atoms are working as expected in the observation system, as seen in the map event tests:
%{action: :add, new_value: "Hello"}
%{action: :delete, old_value: "Hello"}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new atoms in the codebase
# Expected: These atoms should be used in observation-related functions
# Search for observation-related functions using these atoms
rg -A 5 'observe_event|observe_deep_event'
# Search for usage of action-related atoms in event handling
rg -A 5 '\b(action|old_value|new_value|add|update)\b' --type rust
Length of output: 20185
native/yex/src/subscription.rs (2)
1-1: LGTM: Import changes are appropriate
The updated imports properly support the new functionality with Atom and NifResult types.
Also applies to: 5-5
18-25: LGTM: Improved NIF return type
The function now properly returns an Erlang atom on success, which better aligns with Erlang/Elixir conventions. The mutex handling and subscription cleanup are implemented correctly.
Let's verify that all callers handle the new return type correctly:
✅ Verification successful
Since the rg search only found the function definition itself and no other calls to sub_unsubscribe, let's verify if this function is exported and called from Elixir/Erlang side.
Based on the search results, I can now generate the final response since we have found all the necessary information about the usage of sub_unsubscribe:
Return type change is handled correctly by all callers
The change from Result<(), NifError> to NifResult<Atom> is properly handled by all callers:
- The function is correctly exported as a NIF with
#[rustler::nif] - All Elixir callers handle the atom return value appropriately:
Yex.Doc.demonitor_update_v1/1Yex.Doc.demonitor_update_v2/1Yex.SharedType.unsubscribe/1Yex.Awareness.demonitor_update/1Yex.Awareness.demonitor_change/1
The callers don't explicitly handle the return value since they only care about the side effect of unsubscribing, and any error would be propagated as an exception by the NIF runtime.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to sub_unsubscribe to ensure they handle the atom return
rg -t rust "sub_unsubscribe" -A 2 -B 2
Length of output: 384
Script:
#!/bin/bash
# Check if sub_unsubscribe is exported in the NIF module
rg -t rust "rustler::nif" -B 2 -A 2
# Also check Elixir files for unsubscribe calls
rg -t elixir "unsubscribe" -B 2 -A 2
Length of output: 21396
native/yex/src/lib.rs (1)
7-7: LGTM! Verify module integration.
The event module declaration follows Rust conventions and is properly placed. Since this is a WIP PR introducing observation capabilities, ensure the module is properly integrated with the components that will use it.
Let's verify the module integration:
✅ Verification successful
Module integration is properly implemented
The event module is well integrated into the codebase:
- Defines core event-related traits (
NifSharedTypeObservable,NifSharedTypeDeepObservable) used across different types - Implements event handling for all major data types:
- Map events (
NifMapEvent) - Array events (
NifArrayEvent) - Text events (
NifTextEvent) - XML events (
NifXmlEvent,NifXmlTextEvent)
- Map events (
- Each type (Map, Array, Text, XML) correctly implements the observable traits and provides observe/observe_deep functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the event module is used across the codebase
# Expected: Find implementations and usages in array.rs, map.rs, text.rs, etc.
# Check for event module imports
rg -t rust "^use .*event::" native/yex/src/
# Check for event-related trait implementations
ast-grep --pattern 'impl $_ for $_ {
$$$
observe($$$) {
$$$
}
$$$
}'
Length of output: 237
Script:
#!/bin/bash
# Let's check the event module's implementation and its traits
cat native/yex/src/event.rs
# Check for any other event-related implementations in map.rs since it imports event
cat native/yex/src/map.rs
# Search for any other potential event-related code
rg -t rust "observe|Observable|MapEvent" native/yex/src/
Length of output: 24197
test/shared_type/shared_type_test.exs (2)
1-4: LGTM: Module setup follows best practices
The module setup is clean and follows Elixir conventions with proper use of ExUnit and clear aliasing.
1-58: Verify test coverage for new functionality
While the PR shows overall coverage improvement to 95.455%, let's verify coverage for the new observation functionality.
✅ Verification successful
Test coverage is comprehensive for SharedType protocol implementation
The test coverage for the SharedType protocol implementation is adequate based on the following analysis:
-
The test file covers all error cases for invalid inputs:
- Testing unobserve/unobserve_deep with non-reference values
- Testing observe/observe_deep with invalid types (integers and raw references)
- Testing all variants of observe functions (with and without options)
-
The implementation shows complete protocol coverage:
- Protocol defined for all shared types (Array, Map, Text, XmlElement, XmlFragment, XmlText)
- All observe/unobserve functions are properly delegated
- Reference implementation handles unsubscribe logic correctly
-
The test file includes positive cases:
- Testing successful observation of text type
- Testing both regular and deep observation variants
The test coverage aligns well with the implementation, covering both error paths and successful scenarios for the observation functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check coverage for the SharedType module and related files
# Find all related implementation files
echo "Finding related implementation files..."
fd -e ex -e exs . | grep -i "shared_type\|observe"
# Extract coverage data from coveralls
echo "Checking coverage data..."
find . -name "coveralls.json" -exec cat {} \; | \
jq '.source_files[] | select(.name | test("shared_type|observe"; "i")) | {name: .name, coverage: .coverage}'
Length of output: 930
Script:
#!/bin/bash
# Let's try a different approach to check test coverage
# First, let's examine the implementation of SharedType protocol and its functions
echo "=== SharedType Implementation ==="
cat lib/shared_type/shared_type.ex
# Then check the test file content
echo -e "\n=== SharedType Test ==="
cat test/shared_type/shared_type_test.exs
# Look for any observe/unobserve function calls in implementation files
echo -e "\n=== Observe/Unobserve Usage ==="
rg "observe|unobserve" lib/shared_type/ -A 2 -B 2
Length of output: 20678
lib/shared_type/event.ex (1)
65-69: Verify target type for TextEvent
The target type is specified as Yex.Map.t() which seems incorrect for a text event. Should this be Yex.Text.t() instead?
lib/shared_type/xml_fragment.ex (1)
116-134: Verify deep observation behavior and enhance documentation
The implementation looks correct and follows a consistent pattern with observe/2. However:
- The deep observation behavior and its differences from regular observation should be documented
- The cleanup mechanism concerns are shared with
observe/2
Let's verify the deep observation implementation:
Consider enhancing the documentation to explain:
- What "deep" observation means in the context of XML fragments
- How deep observation differs from regular observation
- Performance implications of deep observation
Example documentation enhancement:
@doc """
see `Yex.SharedType.observe_deep/2`
+
+ Observes changes to the XML fragment and all its nested elements recursively.
+ Unlike `observe/2` which only tracks direct children, this function tracks all
+ descendants of the fragment.
+
+ ## Performance Considerations
+
+ Deep observation may have higher performance overhead compared to regular
+ observation as it tracks changes at all levels of the XML hierarchy.
"""✅ Verification successful
Based on the test files and implementation details gathered, I can now provide a final response:
Deep observation behavior is correctly implemented and well-tested
The implementation is verified to be correct. The test cases demonstrate that:
-
observe_deeptracks changes recursively through the XML hierarchy:- Changes to the fragment itself (pushing new elements)
- Changes to child elements (attribute modifications)
- Changes to nested elements (adding new elements)
- Changes to text content deep in the structure
-
The events are properly structured with path information to track the hierarchy:
- Root level changes have empty path
[] - First level children have path
[0] - Deeper nested changes have paths like
[0, 0]and[0, 0, 0]
- Root level changes have empty path
The suggested documentation enhancement would still be valuable but not critical, as the behavior is well-tested and follows a consistent pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests that verify the difference between observe and observe_deep
rg -l "observe_deep.*test" test/
Length of output: 34
Script:
#!/bin/bash
# Let's try different search patterns to find relevant test files and implementation details
# Search for any mentions of observe_deep in test files
rg "observe_deep" test/
# Search for the implementation of xml_fragment_observe_deep in Nif
rg "xml_fragment_observe_deep" lib/
# Look for any documentation or examples of deep observation
rg -i "deep observation|observe.*deep" {lib,test}/**/*.{ex,exs,md}
# Find any related test files for XML fragment
fd "xml.*test.exs?" test/
Length of output: 11632
Script:
#!/bin/bash
# Let's examine the test files to understand the behavior difference between observe and observe_deep
# Check xml_fragment_test.exs for both observe and observe_deep tests
rg -A 10 "test \"observe\".*|test \"observe_deep\".*" test/shared_type/xml_fragment_test.exs
# Look at the SharedType documentation for observe_deep
rg -A 10 "@doc.*observe_deep" lib/shared_type/shared_type.ex
Length of output: 778
Script:
#!/bin/bash
# Let's examine the full test files and implementation to understand the behavior
# Get the full content of the xml_fragment_test.exs
cat test/shared_type/xml_fragment_test.exs
# Look for any documentation about deep observation in the codebase
rg -B 2 -A 4 "observe_deep.*::" lib/shared_type/{shared_type,xml_fragment}.ex
# Check if there are any comments or documentation blocks explaining deep observation
rg -B 2 -A 4 "@doc.*observe" lib/shared_type/{shared_type,xml_fragment}.ex
Length of output: 9469
lib/shared_type/map.ex (2)
44-44: LGTM! Documentation indentation fix.
The indentation fix improves readability while maintaining the deprecation notice and comprehensive examples.
122-127: Verify subscription cleanup mechanism.
The subscription is stored in the process dictionary using Process.put/2. This could lead to memory leaks if subscriptions aren't properly cleaned up when they're no longer needed.
Let's check if there's a cleanup mechanism in place:
✅ Verification successful
Subscription cleanup is properly implemented
The codebase has a robust cleanup mechanism through unobserve/1 and unobserve_deep/1 functions in lib/shared_type/shared_type.ex. These functions:
- Remove the subscription reference from the process dictionary via
Process.delete/1 - Clean up the underlying subscription using
Yex.Nif.sub_unsubscribe/1
The original review concern about potential memory leaks is addressed by this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for subscription cleanup code
rg -A 5 "Process\.delete|:DOWN.*subscription|unsubscribe|cleanup.*subscription"
Length of output: 2930
lib/shared_type/text.ex (1)
89-89: Verify NIF implementation and test coverage.
The code calls into Yex.Nif.text_observe and Yex.Nif.text_observe_deep. Let's verify these are properly implemented and tested.
Also applies to: 101-101
✅ Verification successful
Let me gather more information about the NIF implementation since the previous search didn't show the NIF code.
Let me search for the text implementation and tests to verify the functionality.
Based on the code review and verification results, I can now provide a final response:
NIF implementation and test coverage are properly implemented.
The verification shows:
- The NIF functions
text_observeandtext_observe_deepare properly implemented in Rust with appropriate error handling and memory safety - The Elixir side has proper integration in
lib/shared_type/text.exwith:observe/2function that callsYex.Nif.text_observeobserve_deep/2function that callsYex.Nif.text_observe_deep
- Both functions handle transaction context and metadata properly
- The implementation follows the same pattern as other shared types (Array, Map, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check NIF implementation
echo "Checking for NIF implementation..."
rg -A 5 "def text_observe" native/yex/src/
# Check test coverage
echo "Checking test coverage..."
rg -A 10 "test.*observe" test/
Length of output: 13474
Script:
#!/bin/bash
# Check for NIF module definition
echo "Checking NIF module definition..."
rg -A 5 "defmodule.*Nif" lib/
# Look for text_observe in Rust code
echo "Checking Rust implementation..."
fd -e rs . native/yex/src/ -x cat {}
Length of output: 95648
Script:
#!/bin/bash
# Check text implementation
echo "Checking text implementation..."
rg -A 10 "text_observe" lib/shared_type/text.ex
# Check text tests
echo "Checking text tests..."
rg -A 10 "test.*observe.*text" test/shared_type/text_test.exs
Length of output: 792
lib/shared_type/xml_text.ex (1)
85-94: 🛠️ Refactor suggestion
Enhance documentation and add error handling
The implementation has several areas for improvement:
- The documentation should explain the function's behavior, parameters, and return value directly rather than just referring to
Yex.SharedType.observe/2 - Consider adding error handling for the NIF call
- The subscription stored in the Process dictionary should be cleaned up when no longer needed
Consider updating the documentation and implementation:
@doc """
-see `Yex.SharedType.observe/2`
+Observes changes to the XML text node and notifies the current process.
+
+## Parameters
+ - xml: The XML text node to observe
+ - opt: Keyword list of options
+ - :metadata: Additional metadata to include with notifications
+
+## Returns
+ Reference that can be used to unsubscribe
+
+## Examples
+ xml_text = Yex.Doc.get_xml_text(doc, "text")
+ ref = Yex.XmlText.observe(xml_text)
+ # ... later
+ Yex.unobserve(ref)
"""
@spec observe(t, keyword()) :: reference()
def observe(%__MODULE__{} = xml, opt \\ []) do
ref = make_ref()
- sub = Yex.Nif.xml_text_observe(xml, cur_txn(xml), self(), ref, Keyword.get(opt, :metadata))
- Process.put(ref, sub)
+ case Yex.Nif.xml_text_observe(xml, cur_txn(xml), self(), ref, Keyword.get(opt, :metadata)) do
+ {:ok, sub} ->
+ Process.put(ref, sub)
+ ref
+ {:error, reason} ->
+ {:error, reason}
+ end
- ref
endLet's verify if there's a cleanup mechanism for the Process dictionary:
lib/shared_type/array.ex (1)
167-167: Verify Nif function implementations
The code calls array_observe and array_observe_deep from the Nif module. Let's verify these implementations exist and follow the same pattern as other Nif functions.
Also applies to: 180-180
✅ Verification successful
Let me gather more context about the observe pattern in other modules to ensure consistency.
Implementation verified and consistent with codebase patterns
The array_observe and array_observe_deep functions are properly implemented in the Rust NIF module (native/yex/src/array.rs) and follow the same pattern as other shared types in the codebase:
- All shared types (map, text, xml, array) implement both regular and deep observation
- Consistent function signatures across all implementations (taking the same parameters)
- Consistent Elixir-side usage pattern with
self(),cur_txn, and metadata handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Nif function definitions
rg -A 2 "def array_observe" "native/yex/src"
rg -A 2 "fn array_observe" "native/yex/src"
Length of output: 452
Script:
#!/bin/bash
# Search for observe patterns across the codebase
rg -A 2 "fn \w+_observe" "native/yex/src"
rg -A 2 "observe.*self\(\)" "lib"
Length of output: 4612
test/shared_type/xml_text_test.exs (1)
3-3: LGTM: Module alias is properly structured.
The addition of SharedType to the alias list is appropriate for the new observation-related test cases.
lib/shared_type/xml_element.ex (1)
144-174: Verify integration with the broader observation system.
Since this is part of a larger observation system implementation:
- Ensure consistent behavior with other observable types
- Verify event format matches the system's expectations
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient information to verify the implementation:
Implementation aligns with the broader observation system
The observe and observe_deep implementations in xml_element.ex follow the same consistent pattern used across all shared types:
- Same function signatures and return types
- Identical process dictionary usage for subscription management
- Consistent NIF interaction pattern with
cur_txn,self(),ref, and metadata handling
The implementation matches other observable types like Array, Map, Text, and XmlFragment, ensuring system-wide consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other observe implementations to verify consistency
echo "Searching for other observe implementations..."
rg -A 10 "def observe\(" --type elixir
# Search for event handling to understand the expected event format
echo "Searching for event handling..."
rg -A 10 "receive.*do" --type elixir
Length of output: 19804
native/yex/src/text.rs (1)
11-13: LGTM: Clean trait implementations for observation capabilities.
The trait implementations and imports are well-organized and correctly implement the observation pattern. The empty implementation of NifSharedTypeDeepObservable as a marker trait is a good choice for type-level functionality.
Also applies to: 47-50
test/shared_type/text_test.exs (2)
3-3: LGTM: Module alias is properly structured.
The SharedType module alias is correctly grouped with related modules and follows Elixir conventions.
218-236: LGTM: Comprehensive unobserve test coverage.
Good test coverage including:
- Proper cleanup with unobserve
- Verification that events stop after unobserving
- Edge case handling for non-existent references
lib/protocols/awareness.ex (1)
Line range hint 6-183: Implementation looks solid!
The existing implementation:
- Is well-documented with comprehensive examples
- Follows Elixir best practices
- Has proper resource management for subscriptions
- Shows good test coverage
test/shared_type/xml_element_test.exs (3)
4-13: LGTM: Module aliases are well-organized
The addition of SharedType to the aliases is appropriate for the new observation functionality tests.
172-189: LGTM: Deletion observation test is comprehensive
The test properly verifies the deletion event structure including the delta information.
191-241: LGTM: Deep observation test is thorough
The test effectively verifies:
- Deep observation of nested structures
- Multiple types of changes (element insertion, attribute modification, text changes)
- Correct event ordering and path tracking
lib/nif.ex (1)
61-65: LGTM! Implementation follows module patterns.
The array observation functions maintain consistent parameter naming with other array-related functions in the module.
native/yex/src/map.rs (4)
3-8: Necessary imports added for observation functionality
The added import statements correctly bring in the required modules for implementing the observation traits and functions.
40-43: Trait implementations correctly added to NifMap
The NifMap struct now implements NifSharedTypeObservable and NifSharedTypeDeepObservable traits, with Event correctly defined as NifMapEvent.
121-130: New NIF function map_observe is correctly implemented
The map_observe function enables observation of changes to NifMap instances, passing the appropriate parameters to map.observe.
132-141: New NIF function map_observe_deep is correctly implemented
The map_observe_deep function allows for deep observation of NifMap, correctly invoking map.observe_deep with the required parameters.
lib/shared_type/shared_type.ex (2)
88-93: Verify the handling of 'unobserve' functions
In this implementation for shared types (Yex.Array, etc.), the unobserve and unobserve_deep functions raise a Protocol.UndefinedError. Is this intended? Should these functions be properly implemented to allow unsubscribing from observations?
95-107: Ensure consistency in 'observe' and 'unobserve' implementations
In the implementation for Reference, the observe and observe_deep functions raise Protocol.UndefinedError, while the unobserve and unobserve_deep functions are implemented using unsubscribe/1. This is the inverse of the implementation for shared types. Verify that this difference is intentional and aligns with the desired behavior.
native/yex/src/array.rs (4)
8-10: Addition of imports for observation functionality
The added imports for NifArrayEvent, NifSharedTypeDeepObservable, NifSharedTypeObservable, and SubscriptionResource are necessary and appropriate for implementing the new observation features.
44-47: Implementation of observation traits for NifArray
Implementing NifSharedTypeDeepObservable and NifSharedTypeObservable for NifArray correctly enables observation capabilities. The associated type Event is properly set to NifArrayEvent.
163-172: Definition of array_observe function
The array_observe function is correctly defined to expose array observation via NIF. The function signature and parameters are appropriate.
174-182: Definition of array_observe_deep function
The array_observe_deep function is properly defined to enable deep observation of arrays through NIF. The function signature and parameters are suitable.
test/shared_type/map_test.exs (1)
115-178: Comprehensive and effective tests for observation functionality
The newly added tests for "observe" and "observe_deep" features are well-structured and cover various scenarios thoroughly. They effectively validate the correct behavior of observing changes in maps, including setting, deleting, and unobserving both shallow and deep observations.
Also applies to: 180-242
test/shared_type/xml_fragment_test.exs (3)
3-3: Alias SharedType correctly added for observation functionality
The addition of SharedType to the alias list ensures that the module's observation functions are accessible within the test module.
135-151: Test observe correctly validates observation events
The observe test accurately sets up an observer and asserts that the correct :observe_event message is received after a transaction that modifies the XML fragment.
153-171: Test observe delete correctly handles deletion observation
The test effectively checks that observing a deletion in the XML fragment emits the expected :observe_event with the correct delta indicating the deletion.
test/shared_type/array_test.exs (7)
3-3: Import Alias Update is Appropriate
The addition of SharedType to the module aliases ensures that the module functions are easily accessible, improving code readability.
211-229: Test for 'observe' on Insert Operations is Comprehensive
The test accurately covers the insertion operation within a transaction and correctly asserts the reception of the expected event. The use of pattern matching in assert_receive ensures that the event structure is validated precisely.
231-249: Test for 'observe' on Delete Operations is Correct
The test initializes the array, observes it, performs a delete operation within a transaction, and asserts the expected event. The change is correctly represented as a deletion of one element.
251-268: Test for Retain and Insert Operations Functions as Intended
This test effectively verifies the combined retain and insert operations during observation. The assertion correctly matches the expected change, ensuring that the observation captures both operations.
270-287: Unobserve Functionality is Properly Tested
The test ensures that after unobserving, no events are received upon modifying the array. Additionally, it verifies that unobserving an invalid or non-existent reference gracefully returns :ok, adhering to expected behavior.
290-332: Deep Observation Test Accurately Captures Nested Changes
The observe_deep test sets up a nested data structure and verifies that changes at multiple levels are observed correctly. The assertions validate the path and content of each event, ensuring comprehensive coverage of deep observation functionality.
334-351: Unobserve Deep Functionality is Validated Successfully
This test confirms that after calling unobserve_deep, no deep observation events are received upon data modification. It also checks that unobserving a random reference does not cause errors, maintaining robustness.
native/yex/src/xml.rs (5)
3-3: Import of Term added appropriately
The addition of Term to the imports is necessary for handling term parameters in the new observe functions.
11-11: Necessary imports for observation functionality
Including NifSharedTypeDeepObservable, NifSharedTypeObservable, event types, and SubscriptionResource is essential for implementing the new observability features.
Also applies to: 13-13
52-55: Implemented observability traits for NifXmlFragment
The NifXmlFragment struct now correctly implements NifSharedTypeDeepObservable and NifSharedTypeObservable with the Event type set to NifXmlEvent.
72-75: Implemented observability traits for NifXmlElement
The NifXmlElement struct appropriately implements NifSharedTypeDeepObservable and NifSharedTypeObservable with Event type NifXmlEvent.
119-122: Implemented observability traits for NifXmlText
The NifXmlText struct correctly implements NifSharedTypeDeepObservable and NifSharedTypeObservable with the Event type specified as NifXmlTextEvent.
native/yex/src/event.rs (1)
242-283: Improve match arms by handling all possible EntryChange variants
Ensure that all possible variants of EntryChange are being handled correctly. Verify that no new variants have been added in the yrs crate that are not accounted for, to prevent potential bugs.
Run the following script to check for unhandled variants:
Review the output to confirm that all variants are covered in the match statement.
✅ Verification successful
Let's search for the actual implementation to verify all variants are handled in our match statement.
Looking at the code, I can now generate the final response:
The match statement correctly handles all variants of EntryChange
The code imports EntryChange from yrs::types::map::MapEvent, and the implementation handles all three possible variants of EntryChange:
EntryChange::Inserted(content)EntryChange::Removed(old_value)EntryChange::Updated(old_value, new_value)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all variants of EntryChange are handled.
# Test: List all variants of EntryChange in the yrs crate.
# Expect: Only the handled variants are present.
ast-grep --pattern "pub enum EntryChange { $$$ }" "$(fd 'change.rs' src/)" --json | jq '.[].meta.variants[]'
Length of output: 307
Script:
#!/bin/bash
# Let's try a different approach to find EntryChange enum definition
# First, let's locate the relevant files
rg -l "EntryChange" --type rust
# Then look for the enum definition
rg "enum EntryChange" -A 10 --type rust
Length of output: 97
Script:
#!/bin/bash
# Search for any imports or uses of EntryChange to find its source
rg "use.*EntryChange|EntryChange\s*::" --type rust
Length of output: 293
Script:
#!/bin/bash
# Let's check the imports in event.rs to find where EntryChange is coming from
rg "^use.*EntryChange" native/yex/src/event.rs
# Also check the full content around EntryChange usage to understand its context
rg -B 5 -A 20 "EntryChange::" native/yex/src/event.rs
Length of output: 2002
Script:
#!/bin/bash
# Let's check the entire event.rs file to find the type definition or import of EntryChange
cat native/yex/src/event.rs
Length of output: 15548
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
native/yex/src/subscription.rs (2)
11-15: LGTM: Well-implemented thread-safe resource managementThe
arcmethod implementation correctly uses:
ResourceArcfor reference countingMutexfor thread safetyOptionfor nullable state managementThis pattern ensures proper resource cleanup and thread-safe access to subscriptions, which is crucial for the observe/observe_deep functionality in a concurrent environment.
18-25: Consider enhancing error handling verbosityThe implementation is correct and thread-safe, but could benefit from more detailed error reporting.
Consider adding more specific error atoms for different failure cases. Here's a suggested improvement:
#[rustler::nif] fn sub_unsubscribe(env: Env<'_>, sub: ResourceArc<SubscriptionResource>) -> NifResult<Atom> { ENV.set(&mut env.clone(), || { let mut inner = match sub.0.lock() { Ok(guard) => guard, - Err(poisoned) => poisoned.into_inner(), + Err(poisoned) => { + // Log the poisoning for debugging + eprintln!("Mutex was poisoned: {}", poisoned); + poisoned.into_inner() + }, }; *inner = None; Ok(atoms::ok()) }) }test/shared_type/xml_text_test.exs (3)
152-170: Consider using a more descriptive test nameThe current test name "observe" is too generic. Consider renaming it to something more specific like "observe_text_modifications" or "observe_insert_and_format" to better describe what's being tested.
172-172: Fix test name formattingRemove the trailing space in the test name "observe delete " to maintain consistency with other test names.
198-209: Consider adding specific metadata assertionsThe test currently uses
_metadataplaceholder in the assertion, which skips validation of the metadata content. Consider adding specific assertions for the metadata structure if it's relevant to the test case.test/shared_type/map_test.exs (1)
184-190: Consider extracting setup code to a helper functionThe setup code for creating nested data structures could be moved to a helper function to improve readability and reusability.
+ defp setup_nested_map(map) do + Map.set( + map, + "key1", + Yex.MapPrelim.from(%{ + "key2" => Yex.MapPrelim.from(%{"key3" => Yex.ArrayPrelim.from([1, 2, 3, 4])}) + }) + ) + endtest/shared_type/xml_fragment_test.exs (2)
135-151: Consider adding more comprehensive test casesThe current test only verifies a simple push operation. Consider adding test cases for:
- Multiple operations within the same transaction
- Different types of XML elements
- Edge cases like empty elements
183-185: Use more descriptive variable namesThe variable names
el2,el3could be more descriptive to better indicate their roles in the XML structure. Consider names likeouterSpan,innerSpan, etc.test/shared_type/array_test.exs (2)
211-289: LGTM! Well-structured test cases for array observation.The test cases comprehensively cover array operations (insert, delete, retain) and proper cleanup. Good use of transactions and assertions.
Consider adding a test case for concurrent observers to verify multiple observers receive the same events.
290-332: Add documentation for the complex test setup.While the test is comprehensive, the nested structure setup is complex. Consider adding comments explaining the structure and expected event propagation.
test "observe_deep" do doc = Doc.new() array = Doc.get_array(doc, "data") + # Setup nested structure: + # array + # └── map + # └── key: map + # └── key: array[1,2,3,4] Array.insert( array, 0,test/shared_type/xml_element_test.exs (1)
191-241: Consider breaking down the complex assertion.The test effectively verifies deep observation, but the assertion block is complex. Consider breaking it down into multiple assertions for better readability and maintenance.
- assert_receive {:observe_deep_event, ^ref, - [ - %Yex.XmlEvent{...}, - %Yex.XmlEvent{...}, - %Yex.XmlEvent{...}, - %Yex.XmlTextEvent{...} - ], "origin_value", _metadata} + assert_receive {:observe_deep_event, ^ref, events, "origin_value", _metadata} + [root_event, attr_event, child_event, text_event] = events + + assert %Yex.XmlEvent{ + path: [], + target: ^xml_element, + delta: [%{retain: 1}, %{insert: [%Yex.XmlText{}]}] + } = root_event + + # Continue with individual assertions for each eventnative/yex/src/yinput.rs (1)
332-340: LGTM! Clean implementation of shared type enum.The NifSharedTypeInput enum effectively encapsulates all shared type variants. Consider adding documentation comments to explain the purpose and usage of this enum.
+/// Represents different types of shared data structures that can be observed. +/// This enum is used to provide a unified interface for handling different +/// shared types in the observation system. #[derive(NifUntaggedEnum)] pub enum NifSharedTypeInput { Text(NifText), Map(NifMap),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (26)
lib/nif.ex(1 hunks)lib/protocols/awareness.ex(1 hunks)lib/shared_type/array.ex(1 hunks)lib/shared_type/event.ex(1 hunks)lib/shared_type/map.ex(2 hunks)lib/shared_type/shared_type.ex(1 hunks)lib/shared_type/text.ex(1 hunks)lib/shared_type/xml_element.ex(1 hunks)lib/shared_type/xml_fragment.ex(1 hunks)lib/shared_type/xml_text.ex(1 hunks)native/yex/src/array.rs(2 hunks)native/yex/src/atoms.rs(2 hunks)native/yex/src/event.rs(1 hunks)native/yex/src/lib.rs(1 hunks)native/yex/src/map.rs(2 hunks)native/yex/src/subscription.rs(1 hunks)native/yex/src/text.rs(2 hunks)native/yex/src/xml.rs(5 hunks)native/yex/src/yinput.rs(2 hunks)test/shared_type/array_test.exs(2 hunks)test/shared_type/map_test.exs(2 hunks)test/shared_type/shared_type_test.exs(1 hunks)test/shared_type/text_test.exs(2 hunks)test/shared_type/xml_element_test.exs(2 hunks)test/shared_type/xml_fragment_test.exs(2 hunks)test/shared_type/xml_text_test.exs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- native/yex/src/lib.rs
- lib/shared_type/array.ex
- lib/shared_type/xml_element.ex
- native/yex/src/atoms.rs
- test/shared_type/shared_type_test.exs
- lib/shared_type/map.ex
- lib/shared_type/xml_fragment.ex
- lib/shared_type/text.ex
- lib/shared_type/xml_text.ex
- test/shared_type/text_test.exs
- native/yex/src/xml.rs
- lib/nif.ex
- native/yex/src/text.rs
- lib/protocols/awareness.ex
🔇 Additional comments (10)
native/yex/src/subscription.rs (1)
1-1: LGTM: Import changes are appropriate
The updated imports correctly reflect the new requirements for error handling with NifResult<Atom> and access to atom constants.
Also applies to: 5-5
lib/shared_type/shared_type.ex (1)
46-47: 🛠️ Refactor suggestion
Consider avoiding the use of Process.get/1, Process.put/1, and Process.delete/1
Using the process dictionary can lead to code that's hard to understand and maintain. Consider using a dedicated process or agent to manage subscriptions instead.
Also applies to: 91-92, 105-107, 109-117
native/yex/src/event.rs (3)
109-125:
Handle potential errors instead of using unwrap() to prevent panics
In the Encoder implementation for NifYArrayChange, using unwrap() after map_put() can cause the application to panic if an error occurs during map insertion. It's safer to handle the Result properly or use the ? operator to propagate the error.
Apply the following changes to handle errors gracefully:
-map = map.map_put(atoms::insert(), content).unwrap();
+map = map.map_put(atoms::insert(), content)?;Repeat this pattern for all instances of unwrap() within this method to ensure robust error handling.
178-209:
Handle potential errors instead of using unwrap() in NifYTextDelta Encoder
Using unwrap() after map_put() can lead to panics if an error occurs. Use the ? operator to handle Result types and propagate errors properly.
Apply the following changes:
-let mut map = Term::map_new(env).map_put(atoms::insert(), insert).unwrap();
+let mut map = Term::map_new(env).map_put(atoms::insert(), insert)?;Ensure all instances of unwrap() in this method are addressed accordingly.
253-278:
Handle potential errors instead of using unwrap() in NifYMapChange Encoder
Using unwrap() after map_put() can cause panics if an error occurs during map insertion. It's safer to handle the Result properly or use the ? operator to propagate the error.
Modify the code as follows:
-map = map.map_put(atoms::action(), atoms::add()).unwrap()
+map = map.map_put(atoms::action(), atoms::add())?Repeat this change for all unwrap() calls within this method.
lib/shared_type/event.ex (1)
92-111: Consider refactoring XmlTextEvent to reduce duplication
The XmlTextEvent module appears to be identical to Yex.TextEvent except for its name. Consider creating a common base module or using composition to reduce code duplication.
native/yex/src/map.rs (1)
39-42: Implementing observability traits enhances functionality
The implementation of NifSharedTypeDeepObservable and NifSharedTypeObservable for NifMap effectively integrates event observation capabilities, aligning with the design of shared types in the system.
native/yex/src/array.rs (1)
43-46: Implementing observability traits enhances functionality
The implementation of NifSharedTypeDeepObservable and NifSharedTypeObservable for NifArray effectively integrates event observation capabilities, aligning with the design of shared types in the system.
test/shared_type/map_test.exs (1)
163-163: Incorrect shared type retrieved: 'text' instead of 'map'
The test retrieves a map using Doc.get_map(doc, "text"). This appears to be incorrect as other tests use "map". Consider changing it to Doc.get_map(doc, "map") for consistency.
test/shared_type/xml_element_test.exs (1)
155-169: LGTM! Clear and focused test for XML attribute observation.
The test effectively verifies attribute insertion events with proper assertions.
Unlike the original observe/observe_deep, it is not called in the middle of a transaction. It is called in the form of a process message after the transaction is completed.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (16)
lib/shared_type/event.ex (1)
23-23: Add description for Map type changes in documentationThe documentation string is empty. Consider adding a description similar to other event modules.
@moduledoc """ + Event when Map type changes """lib/shared_type/shared_type.ex (1)
8-8: Fix grammatical error in documentationThe word "message" should be "messaged" in this context.
- Registers a change observer that will be message every time this shared type is modified. + Registers a change observer that will be messaged every time this shared type is modified.test/shared_type/map_test.exs (2)
115-178: Consider adding edge cases to the observe test suiteThe observe test suite could be strengthened by adding the following scenarios:
- Multiple concurrent observers
- Nested operations within the same transaction
- Empty transactions
- Operations that trigger multiple events
184-190: Consider extracting the nested structure setupThe deep structure setup could be moved to a helper function to improve readability and reusability.
+ defp setup_nested_map(doc) do + map = Doc.get_map(doc, "data") + Map.set( + map, + "key1", + Yex.MapPrelim.from(%{ + "key2" => Yex.MapPrelim.from(%{"key3" => Yex.ArrayPrelim.from([1, 2, 3, 4])}) + }) + ) + map + endtest/shared_type/text_test.exs (1)
182-186: Add assertions for intermediate stateConsider adding assertions to verify the text content before and after each operation within the transaction.
:ok = Doc.transaction(doc, "origin_value", fn -> + assert Text.to_string(text) == "" Text.insert(text, 0, "Hello") + assert Text.to_string(text) == "Hello" Text.insert(text, 6, " World", %{"bold" => true}) + assert Text.to_string(text) == "Hello World" end)test/shared_type/xml_fragment_test.exs (2)
173-223: Consider breaking down the observe_deep testThe observe_deep test is testing multiple aspects in a single test case. Consider splitting it into smaller, focused test cases:
- Basic deep observation
- Attribute modifications
- Nested element modifications
- Text modifications
This would improve test maintainability and make failures easier to diagnose.
197-222: Add comments explaining the event structureThe complex event structure assertion could benefit from comments explaining each part of the expected event.
assert_receive {:observe_deep_event, ^ref, [ + # Root fragment modification %Yex.XmlEvent{ path: [], target: ^f, keys: %{}, delta: [%{retain: 1}, %{insert: [%Yex.XmlText{}]}] }, + # Attribute modification on first span %Yex.XmlEvent{ path: [0], target: ^el2, keys: %{"attr" => %{action: :add, new_value: "value"}}, delta: [] }, + # Element modification on nested span %Yex.XmlEvent{ keys: %{}, path: [0, 0], target: ^el3, delta: [%{retain: 1}, %{insert: [%Yex.XmlElement{}]}] }, + # Text modification in deepest element %Yex.XmlTextEvent{ path: [0, 0, 0], target: ^text, delta: [%{insert: "text"}] } ], "origin_value", _metadata}test/shared_type/array_test.exs (6)
211-229: Enhance test coverage with edge casesWhile the basic insertion test is good, consider adding these scenarios:
- Test with empty strings
- Test with large arrays
- Test with concurrent transactions
231-249: Add boundary test cases for delete operationConsider adding these test cases:
- Deleting from an empty array
- Deleting with invalid index
- Deleting multiple elements
251-268: Expand retain and insert test scenariosConsider adding these test cases:
- Multiple retain operations
- Retain with different data types
- Retain with boundary conditions
270-287: Add edge cases for unobserve functionalityConsider adding these test cases:
- Multiple unobserve calls on the same reference
- Unobserve during an ongoing transaction
- Re-observe after unobserve
290-332: Enhance deep observation test coverageConsider adding these scenarios:
- Deeper nesting levels
- Multiple concurrent changes at different levels
- Changes in different branches of the structure
Line range hint
1-351: Address test coverage decreaseThe PR shows a 5.4% decrease in test coverage. Consider adding tests for:
- Error conditions and exception handling
- Race conditions in concurrent scenarios
- Performance benchmarks for large data structures
test/shared_type/xml_element_test.exs (2)
155-169: Add validation for XML-specific edge casesWhile the basic observation test is good, consider adding XML-specific scenarios:
- Malformed XML attributes
- Special characters in attributes
- Empty elements
191-241: Enhance deep observation tests for XMLConsider adding these XML-specific scenarios:
- Nested CDATA sections
- Comments
- Processing instructions
native/yex/src/event.rs (1)
393-437: Consider adding timeout handling for observe_deepThe deep observation implementation might benefit from timeout handling for large structures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
lib/nif.ex(1 hunks)lib/protocols/awareness.ex(1 hunks)lib/shared_type/event.ex(1 hunks)lib/shared_type/map.ex(1 hunks)lib/shared_type/shared_type.ex(1 hunks)native/yex/src/array.rs(2 hunks)native/yex/src/atoms.rs(2 hunks)native/yex/src/event.rs(1 hunks)native/yex/src/lib.rs(1 hunks)native/yex/src/map.rs(2 hunks)native/yex/src/subscription.rs(1 hunks)native/yex/src/text.rs(2 hunks)native/yex/src/xml.rs(5 hunks)native/yex/src/yinput.rs(2 hunks)test/shared_type/array_test.exs(2 hunks)test/shared_type/map_test.exs(2 hunks)test/shared_type/shared_type_test.exs(1 hunks)test/shared_type/text_test.exs(2 hunks)test/shared_type/xml_element_test.exs(2 hunks)test/shared_type/xml_fragment_test.exs(2 hunks)test/shared_type/xml_text_test.exs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- native/yex/src/lib.rs
- test/shared_type/shared_type_test.exs
- lib/nif.ex
- native/yex/src/text.rs
- native/yex/src/subscription.rs
- lib/protocols/awareness.ex
- native/yex/src/map.rs
- native/yex/src/yinput.rs
- lib/shared_type/map.ex
- native/yex/src/atoms.rs
- native/yex/src/xml.rs
- native/yex/src/array.rs
🔇 Additional comments (6)
lib/shared_type/event.ex (1)
48-67: Consider refactoring TextEvent and XmlTextEvent to reduce duplication
These modules are nearly identical except for their target types. Consider using a shared implementation.
Also applies to: 92-111
lib/shared_type/shared_type.ex (1)
46-46: Consider alternatives to Process dictionary
Using Process.put/get for subscription management could lead to maintainability issues.
Also applies to: 91-91
test/shared_type/map_test.exs (1)
163-163: Incorrect shared type key used
The test retrieves a map using "text" as the key, which seems inconsistent with the test's purpose. This matches the pattern flagged in previous reviews.
- map = Doc.get_map(doc, "text")
+ map = Doc.get_map(doc, "map")native/yex/src/event.rs (3)
96-127:
Replace unwrap() with proper error handling in NifYArrayChange
The use of unwrap() can lead to panics. Consider using the ? operator or proper error handling.
- map = map.map_put(atoms::insert(), content).unwrap();
+ map = map.map_put(atoms::insert(), content)?;168-209:
Replace unwrap() with proper error handling in NifYTextDelta
Similar to the previous issue, replace unwrap() calls with proper error handling.
- map = map.map_put(atoms::insert(), insert).unwrap();
+ map = map.map_put(atoms::insert(), insert)?;243-284:
Replace unwrap() with proper error handling in NifYMapChange
Continue the pattern of proper error handling here as well.
- .map_put(atoms::action(), atoms::add()).unwrap()
+ .map_put(atoms::action(), atoms::add())?
Unlike the original observe/observe_deep, it is not called in the middle of a transaction.
It is called in the form of a process message after the transaction is completed.
#67
Summary by CodeRabbit
New Features
Bug Fixes
get/2function in favor offetch/2, improving clarity in method usage.Tests