Conversation
WalkthroughExtracts worker-routing logic from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/doc.ex (1)
78-87: Consider callinghandle_on_update_handler()in the rescue block.The rescue block returns the error tuple without processing any pending callback messages. If callbacks were queued before the error occurred, they will be lost. Consider adding
handle_on_update_handler()before returning the error tuple to ensure callbacks are processed even when errors occur.🔎 Proposed fix
wrapped_fun = fn -> try do result = fun.() handle_on_update_handler() result rescue e -> + handle_on_update_handler() {Yex.Doc, :reraise, e, __STACKTRACE__} end endlib/shared_type/map.ex (2)
328-337: Add documentation for the public API.The
observe/2function is missing a@docannotation. Public APIs should include documentation explaining their purpose, parameters, and return values.🔎 Suggested documentation
+ @doc """ + Observes changes to the map by registering a callback handler. + The handler will be invoked whenever the map is modified within a transaction. + + ## Parameters + * `map` - The map to observe + * `handler` - A function that receives updates (Yex.MapEvent.t()) and origin + + ## Returns + A reference that can be used to unobserve later + + ## Examples + iex> doc = Yex.Doc.new() + iex> map = Yex.Doc.get_map(doc, "map") + iex> ref = Yex.Map.observe(map, fn event, origin -> + ...> IO.inspect({event, origin}) + ...> end) + """ @spec observe( t, handler :: (update :: Yex.MapEvent.t(), origin :: term() -> nil) ) :: reference() def observe(%__MODULE__{doc: doc} = map, handler) do Yex.SharedType.observe(map, metadata: {Yex.ObserveCallbackHandler, handler}, notify_pid: doc.worker_pid ) end
339-348: Add documentation for the public API.The
observe_deep/2function is missing a@docannotation. Public APIs should include documentation explaining their purpose, parameters, and how they differ from regularobserve/2.🔎 Suggested documentation
+ @doc """ + Observes deep changes to the map and nested structures by registering a callback handler. + Unlike observe/2, this also triggers for changes in nested shared types. + + ## Parameters + * `map` - The map to observe + * `handler` - A function that receives a list of updates and origin + + ## Returns + A reference that can be used to unobserve later + + ## Examples + iex> doc = Yex.Doc.new() + iex> map = Yex.Doc.get_map(doc, "map") + iex> ref = Yex.Map.observe_deep(map, fn updates, origin -> + ...> IO.inspect({updates, origin}) + ...> end) + """ @spec observe_deep( t, handler :: (update :: list(Yex.event_type()), origin :: term() -> nil) ) :: reference() def observe_deep(%__MODULE__{doc: doc} = map, handler) do Yex.SharedType.observe_deep(map, metadata: {Yex.ObserveCallbackHandler, handler}, notify_pid: doc.worker_pid ) end
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
lib/doc.exlib/shared_type/array.exlib/shared_type/event.exlib/shared_type/map.exlib/shared_type/shared_type.exlib/shared_type/text.exlib/shared_type/xml_element.exlib/shared_type/xml_fragment.exlib/shared_type/xml_text.exlib/y_ex.extest/doc_test.exstest/shared_type/array_test.exstest/shared_type/map_test.exstest/shared_type/text_test.exstest/shared_type/xml_element_test.exstest/shared_type/xml_fragment_test.exstest/shared_type/xml_text_test.exs
🧰 Additional context used
🧬 Code graph analysis (7)
lib/shared_type/text.ex (13)
lib/shared_type/array.ex (2)
observe(395-400)observe_deep(406-411)lib/shared_type/map.ex (2)
observe(332-337)observe_deep(343-348)lib/shared_type/xml_element.ex (2)
observe(358-363)observe_deep(369-374)lib/shared_type/xml_fragment.ex (2)
observe(287-292)observe_deep(298-303)lib/shared_type/xml_text.ex (2)
observe(146-151)observe_deep(157-162)native/yex/src/event.rs (2)
observe(449-486)observe_deep(397-438)native/yex/src/sync.rs (4)
term(34-34)term(83-83)term(105-105)term(112-112)native/yex/src/map.rs (1)
reference(36-38)native/yex/src/array.rs (1)
reference(40-42)native/yex/src/text.rs (1)
reference(45-47)native/yex/src/weak.rs (1)
reference(37-39)native/yex/src/shared_type.rs (1)
reference(56-56)native/yex/src/xml.rs (3)
reference(50-52)reference(87-89)reference(116-118)
test/doc_test.exs (1)
lib/doc.ex (5)
on_update(318-320)on_update_v1(324-337)transaction(212-233)on_update_v2(341-354)on_subdocs(302-315)
test/shared_type/xml_text_test.exs (2)
lib/shared_type/xml_text.ex (2)
observe(146-151)observe_deep(157-162)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)
test/shared_type/text_test.exs (4)
lib/shared_type/array.ex (2)
observe(395-400)observe_deep(406-411)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)lib/shared_type/xml_text.ex (2)
observe(146-151)observe_deep(157-162)
lib/shared_type/xml_text.ex (6)
lib/shared_type/array.ex (2)
observe(395-400)observe_deep(406-411)lib/shared_type/map.ex (2)
observe(332-337)observe_deep(343-348)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)lib/shared_type/xml_element.ex (2)
observe(358-363)observe_deep(369-374)lib/shared_type/xml_fragment.ex (2)
observe(287-292)observe_deep(298-303)native/yex/src/event.rs (2)
observe(449-486)observe_deep(397-438)
lib/shared_type/array.ex (7)
lib/shared_type/map.ex (2)
observe(332-337)observe_deep(343-348)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)lib/shared_type/xml_element.ex (2)
observe(358-363)observe_deep(369-374)lib/shared_type/xml_fragment.ex (2)
observe(287-292)observe_deep(298-303)lib/shared_type/xml_text.ex (2)
observe(146-151)observe_deep(157-162)native/yex/src/event.rs (2)
observe(449-486)observe_deep(397-438)
lib/doc.ex (2)
native/yex/src/doc.rs (3)
doc_monitor_subdocs(455-487)doc_monitor_update_v1(297-328)doc_monitor_update_v2(330-360)lib/subscription.ex (1)
register(49-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (22)
lib/doc.ex (2)
317-354: LGTM!The callback-based update APIs (
on_update,on_update_v1,on_update_v2) are well-structured and consistent with the existingmonitor_updatepattern. The delegation to NIF functions and subscription registration is correct.
356-372: LGTM!The recursive message handling in
handle_on_update_handler/0correctly drains callback messages from the mailbox. The non-blocking receive (timeout 0) ensures it terminates when no more messages are available, and the pattern matching covers the different callback handler types appropriately.lib/shared_type/shared_type.ex (2)
40-57: LGTM!The addition of
:notify_pidoption with a default toself()is well-designed. It maintains backward compatibility while enabling callers to route notifications to a different process (e.g., the document's worker process).
88-105: LGTM!The
:notify_pidoption is consistently implemented inobserve_deep/2, matching the pattern inobserve/2.lib/y_ex.ex (1)
18-23: LGTM!The
event_typeunion type provides a clean type definition for all supported event structures. This enhances type safety and documentation for the observe/observe_deep APIs.lib/shared_type/xml_element.ex (1)
354-374: LGTM!The
observe/2andobserve_deep/2implementations correctly delegate toYex.SharedTypewith appropriate metadata wrapping and worker process notification routing. The pattern is consistent with other shared type modules in the codebase.lib/shared_type/xml_text.ex (1)
142-162: LGTM!The
observe/2andobserve_deep/2functions are correctly implemented, following the same pattern used across all other shared type modules (Map, Text, Array, XmlElement, XmlFragment). The implementation properly routes notifications through the document's worker process.lib/shared_type/event.ex (1)
112-128: LGTM!The
Yex.WeakEventmodule follows the established pattern of other event types in this file. The struct and type definitions are correct and consistent.test/shared_type/xml_element_test.exs (2)
789-804: LGTM!The test correctly verifies that the observe handler is invoked with the expected event type and origin. The implementation follows the established testing pattern.
806-821: LGTM!The test correctly verifies deep observation callback behavior. The implementation is consistent with the observe test and follows established patterns.
test/shared_type/text_test.exs (2)
761-779: LGTM!The test correctly verifies the observe callback functionality for Text. Implementation is consistent with the pattern used across other shared types.
781-799: LGTM!The test correctly validates deep observation behavior for Text, following the established testing pattern.
test/shared_type/xml_text_test.exs (2)
355-375: LGTM!The test correctly validates the observe callback functionality for XmlText. The implementation is consistent with the testing patterns used across other shared types.
377-397: LGTM!The test correctly verifies deep observation callback behavior for XmlText, following the established testing pattern.
test/shared_type/map_test.exs (1)
248-280: LGTM! Well-structured tests for the new observe APIs.The tests properly verify handler invocation, event types, and origin propagation through the transaction mechanism.
test/shared_type/xml_fragment_test.exs (1)
483-515: LGTM! Consistent test coverage for XmlFragment observation.The tests follow the established pattern and properly validate callback invocation with XmlEvent payloads.
lib/shared_type/text.ex (1)
205-225: LGTM! Clean implementation following established patterns.The observe/observe_deep implementations correctly delegate to
Yex.SharedTypewith proper metadata wrapping and worker PID routing, consistent with the patterns used in Map, Array, and XML shared types.test/shared_type/array_test.exs (1)
520-558: LGTM! Comprehensive test coverage for Array observation with handlers.The tests properly validate both
Array.observeandArray.observe_deepwith callback handlers, complementing the existing SharedType-based observation tests.test/doc_test.exs (1)
549-660: LGTM! Excellent comprehensive test coverage for the new callback-based APIs.The test suite thoroughly validates:
- Basic callback invocation and update data structure
- Origin propagation through transactions
- Both v1 and v2 update variants
- Multiple concurrent callbacks
- Practical integration usage (lines 627-645 demonstrate cross-document sync)
- Subdocs callback registration
The integration test at lines 627-645 is particularly valuable as it demonstrates real-world usage of the update callback to synchronize documents.
lib/shared_type/array.ex (1)
391-400: LGTM! Consistent implementation with other shared types.The
observe/2function correctly follows the established pattern used in Map, Text, XmlElement, XmlText, and XmlFragment modules by delegating toYex.SharedType.observewith appropriate metadata and notify_pid.lib/shared_type/xml_fragment.ex (2)
283-292: LGTM! Consistent implementation with other shared types.The
observe/2function correctly follows the established pattern by delegating toYex.SharedType.observewith appropriate metadata and notify_pid from the document's worker process.
294-303: LGTM! Consistent implementation with other shared types.The
observe_deep/2function correctly follows the established pattern by delegating toYex.SharedType.observe_deepwith appropriate metadata and notify_pid. The typespec correctly matches the function name.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/shared_type/array.ex (1)
402-411: Fix typespec name mismatch.The
@specon line 402 declaresobservebut the function defined on line 406 isobserve_deep. This causes the typespec to be incorrectly associated with theobserve/2function above.🔎 Proposed fix
- @spec observe( + @spec observe_deep( Yex.Array.t(), handler :: (update :: list(Yex.event_type()), origin :: term() -> nil) ) :: reference()lib/doc.ex (1)
302-315: Fix typo in module name.Line 305 references
Yex.Dox.CallbackHandlerwhich should beYex.ObserveCallbackHandlerto match the pattern inhandle_on_update_handler/0at line 366.🔎 Proposed fix
def on_subdocs(%__MODULE__{} = doc, handler) do result = run_in_worker_process(doc, - do: Yex.Nif.doc_monitor_subdocs(doc, self(), {Yex.Dox.CallbackHandler, handler}) + do: Yex.Nif.doc_monitor_subdocs(doc, self(), {Yex.ObserveCallbackHandler, handler}) )
🧹 Nitpick comments (2)
test/doc_test.exs (1)
647-660: Consider adding a test that actually triggers subdocs events.The current test only verifies callback registration via
refute_receive. While useful for ensuring registration doesn't crash, it doesn't validate the callback is actually invoked when subdocs events occur. Consider adding a follow-up test that creates/loads subdocuments to trigger the event.lib/doc.ex (1)
63-66: Clarify the typespec for the doc parameter.The
atom()type in the spec seems unnecessary since the function accessesdoc.worker_pid, which requires a map/struct. Consider using just the map type or theYex.Doc.t()type.🔎 Proposed fix
- @spec run_in_worker_process_fn( - atom() | %{:worker_pid => any(), optional(any()) => any()}, - any() - ) :: any() + @spec run_in_worker_process_fn( + %{:worker_pid => pid() | nil, optional(any()) => any()}, + (-> any()) + ) :: any()
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
lib/doc.exlib/shared_type/array.exlib/shared_type/event.exlib/shared_type/map.exlib/shared_type/shared_type.exlib/shared_type/text.exlib/shared_type/xml_element.exlib/shared_type/xml_fragment.exlib/shared_type/xml_text.exlib/y_ex.extest/doc_test.exstest/shared_type/array_test.exstest/shared_type/map_test.exstest/shared_type/text_test.exstest/shared_type/xml_element_test.exstest/shared_type/xml_fragment_test.exstest/shared_type/xml_text_test.exs
🚧 Files skipped from review as they are similar to previous changes (9)
- test/shared_type/xml_fragment_test.exs
- lib/shared_type/map.ex
- test/shared_type/map_test.exs
- lib/shared_type/xml_element.ex
- test/shared_type/array_test.exs
- lib/shared_type/event.ex
- lib/shared_type/shared_type.ex
- lib/shared_type/xml_fragment.ex
- lib/shared_type/text.ex
🧰 Additional context used
🧬 Code graph analysis (4)
test/shared_type/text_test.exs (2)
lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)
lib/doc.ex (2)
native/yex/src/doc.rs (3)
doc_monitor_subdocs(455-487)doc_monitor_update_v1(297-328)doc_monitor_update_v2(330-360)lib/subscription.ex (1)
register(49-53)
test/shared_type/xml_text_test.exs (2)
lib/shared_type/xml_text.ex (2)
observe(146-151)observe_deep(157-162)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)
test/shared_type/xml_element_test.exs (3)
lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/xml_element.ex (2)
observe(358-363)observe_deep(369-374)native/yex/src/event.rs (2)
observe(449-486)observe_deep(397-438)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (15)
lib/shared_type/xml_text.ex (1)
142-162: LGTM! Observer APIs follow established patterns.The
observe/2andobserve_deep/2implementations correctly delegate toYex.SharedTypewith appropriate metadata and routing notifications throughdoc.worker_pid, consistent with similar implementations across other shared types.lib/y_ex.ex (1)
18-23: LGTM! Event type union correctly defined.The
event_typealias appropriately includes all event types (XmlTextEvent, TextEvent, MapEvent, ArrayEvent, WeakEvent), supporting the new observer APIs across shared types.test/shared_type/xml_text_test.exs (1)
355-397: LGTM! Tests adequately cover new observer APIs.Both tests correctly validate that:
- Handlers are invoked with the expected event types (XmlTextEvent for observe, list of events for observe_deep)
- Origin values are properly propagated through the callback chain
The test structure aligns with existing observer tests in the module.
test/shared_type/xml_element_test.exs (1)
789-821: LGTM! Observer handler tests are well-structured.The tests properly verify:
- Handler registration via
XmlElement.observe/observe_deep- Event delivery with correct event types (XmlEvent for observe, list for observe_deep)
- Origin propagation through the transaction
Consistent with the module's existing observer test patterns.
test/shared_type/text_test.exs (1)
761-799: LGTM! Handler-based observer tests are correctly implemented.Both tests validate the new
Text.observe/observe_deepAPIs by:
- Registering handlers that send messages to the test process
- Performing mutations within a transaction with a test origin
- Asserting proper callback invocation with TextEvent and origin
The pattern matches existing observer tests in the module.
test/doc_test.exs (5)
548-564: LGTM!The test properly verifies the
on_updatecallback registration and invocation, including assertions for both the callback being called and the update data being a non-empty binary.
566-583: LGTM!Good test coverage for
on_update_v1with transaction origin propagation. The test correctly verifies that the origin value is passed through to the callback.
585-599: LGTM!Basic coverage for
on_update_v2callback registration and invocation.
601-625: LGTM!Excellent test for multiple callback registration. It verifies that both callbacks are invoked independently and receive consistent update sizes, which is important for ensuring the callback mechanism doesn't corrupt data between handlers.
627-645: LGTM!This is a valuable integration test that verifies the update data can be used to synchronize another document, demonstrating the practical utility of the callback mechanism.
lib/shared_type/array.ex (1)
391-400: LGTM!The
observe/2implementation correctly delegates toYex.SharedType.observewith the handler wrapped in metadata and uses the document's worker_pid for notifications.lib/doc.ex (4)
55-61: LGTM!Good refactoring to extract the worker process routing logic into a dedicated function. The macro now cleanly delegates to
run_in_worker_process_fn/2.
67-104: LGTM!The worker process routing logic is well-structured with proper error handling and cross-process exception propagation via
reraise. The three cases (self, nil, other pid) are handled correctly.
317-337: LGTM!The
on_updateandon_update_v1functions are implemented correctly, registering the handler with the appropriate metadata wrapper and returning a subscription reference.
339-354: LGTM!The
on_update_v2implementation follows the same pattern ason_update_v1, correctly using the v2 variant of the NIF function.
satoren
force-pushed
the
callback
branch
2 times, most recently
from
4eaa6c5 to
fb7c308
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/doc.ex (1)
317-321: Consider using a more precise return type spec.The
on_update/2,on_update_v1/2, andon_update_v2/2functions returnany()in their specs, but based on the implementation they return{:ok, reference()} | {:error, term()}. Consider aligning with the more specific return type used bymonitor_update/2for consistency and better documentation.🔎 Proposed fix
- @spec on_update(Yex.Doc.t(), handler :: (update :: binary(), origin :: term() -> nil)) :: any() + @spec on_update(Yex.Doc.t(), handler :: (update :: binary(), origin :: term() -> nil)) :: + {:ok, reference()} | {:error, term()} def on_update(%__MODULE__{} = doc, handler) do on_update_v1(doc, handler) end
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
lib/doc.exlib/shared_type/array.exlib/shared_type/event.exlib/shared_type/map.exlib/shared_type/shared_type.exlib/shared_type/text.exlib/shared_type/xml_element.exlib/shared_type/xml_fragment.exlib/shared_type/xml_text.exlib/y_ex.extest/doc_test.exstest/shared_type/array_test.exstest/shared_type/map_test.exstest/shared_type/text_test.exstest/shared_type/xml_element_test.exstest/shared_type/xml_fragment_test.exstest/shared_type/xml_text_test.exs
🚧 Files skipped from review as they are similar to previous changes (8)
- lib/shared_type/event.ex
- test/shared_type/xml_text_test.exs
- lib/shared_type/array.ex
- lib/shared_type/shared_type.ex
- lib/shared_type/text.ex
- test/shared_type/map_test.exs
- lib/shared_type/xml_element.ex
- test/shared_type/xml_element_test.exs
🧰 Additional context used
🧬 Code graph analysis (7)
test/shared_type/array_test.exs (2)
lib/shared_type/array.ex (2)
observe(395-400)observe_deep(406-411)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)
test/doc_test.exs (3)
lib/doc.ex (5)
new(123-125)on_update(318-320)on_update_v1(324-337)on_update_v2(341-354)on_subdocs(302-315)lib/y_ex.ex (1)
apply_update(103-105)lib/subscription.ex (1)
unsubscribe(66-78)
lib/shared_type/map.ex (6)
lib/shared_type/array.ex (2)
observe(395-400)observe_deep(406-411)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)lib/shared_type/xml_element.ex (2)
observe(358-363)observe_deep(369-374)lib/shared_type/xml_fragment.ex (2)
observe(287-292)observe_deep(298-303)lib/shared_type/xml_text.ex (2)
observe(146-151)observe_deep(157-162)
lib/shared_type/xml_fragment.ex (11)
lib/shared_type/array.ex (2)
observe(395-400)observe_deep(406-411)lib/shared_type/map.ex (2)
observe(332-337)observe_deep(343-348)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)lib/shared_type/xml_element.ex (2)
observe(358-363)observe_deep(369-374)lib/shared_type/xml_text.ex (2)
observe(146-151)observe_deep(157-162)native/yex/src/event.rs (2)
observe(449-486)observe_deep(397-438)native/yex/src/array.rs (1)
reference(40-42)native/yex/src/map.rs (1)
reference(36-38)native/yex/src/text.rs (1)
reference(45-47)native/yex/src/shared_type.rs (1)
reference(56-56)native/yex/src/xml.rs (3)
reference(50-52)reference(87-89)reference(116-118)
test/shared_type/text_test.exs (2)
lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)
lib/shared_type/xml_text.ex (6)
lib/shared_type/array.ex (2)
observe(395-400)observe_deep(406-411)lib/shared_type/map.ex (2)
observe(332-337)observe_deep(343-348)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)lib/shared_type/xml_element.ex (2)
observe(358-363)observe_deep(369-374)lib/shared_type/xml_fragment.ex (2)
observe(287-292)observe_deep(298-303)
test/shared_type/xml_fragment_test.exs (1)
lib/shared_type/xml_fragment.ex (2)
observe(287-292)observe_deep(298-303)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (17)
test/shared_type/array_test.exs (2)
520-538: LGTM!The test correctly verifies that
Array.observe/2invokes the handler with aYex.ArrayEventand the transaction origin.
540-558: LGTM!The test correctly verifies that
Array.observe_deep/2invokes the handler with a list of updates and the transaction origin.test/shared_type/xml_fragment_test.exs (2)
483-498: LGTM!The test correctly verifies that
XmlFragment.observe/2invokes the handler with aYex.XmlEventand the transaction origin.
500-515: LGTM!The test correctly verifies that
XmlFragment.observe_deep/2invokes the handler with a list of updates and the transaction origin.lib/shared_type/map.ex (2)
328-337: LGTM!The
observe/2implementation is consistent with other shared types and correctly wires the handler through the document's worker process.
339-348: LGTM!The
observe_deep/2implementation is consistent with other shared types and correctly wires the deep handler through the document's worker process.test/shared_type/text_test.exs (2)
761-779: LGTM!The test correctly verifies that
Text.observe/2invokes the handler with aYex.TextEventand the transaction origin.
781-799: LGTM!The test correctly verifies that
Text.observe_deep/2invokes the handler with a list of updates and the transaction origin.test/doc_test.exs (4)
4-4: LGTM!The alias addition for
Mapis appropriate for the new subdocs tests that useMap.set/3.
549-564: LGTM!The test correctly validates the
on_updatecallback registration, invocation, and data types.
566-645: LGTM!The
on_update_v1,on_update_v2, multiple callbacks, and sync tests are well-structured with correct callback signatures and comprehensive assertions.
662-692: LGTM!The subdocs event test correctly validates the callback receives
added,loaded, andremovedfields, and the error path test properly mocks the NIF with[:passthrough].lib/shared_type/xml_fragment.ex (1)
283-303: LGTM!The
observe/2andobserve_deep/2implementations are consistent with the pattern used across other shared types (Map,Text,Array,XmlElement,XmlText). The specs correctly referenceYex.XmlEvent.t()for shallow observation andlist(Yex.event_type())for deep observation.lib/shared_type/xml_text.ex (1)
142-162: LGTM!The
observe/2andobserve_deep/2implementations follow the same consistent pattern as other shared types, correctly usingYex.XmlTextEvent.t()for the observe callback and delegating toYex.SharedType.lib/doc.ex (3)
55-104: LGTM!The refactored
run_in_worker_processmacro and newrun_in_worker_process_fn/2cleanly encapsulate worker-routing logic with proper error propagation viareraise. The three cases (self, nil, other pid) are handled correctly with stacktrace preservation.
302-315: The code is correct as written.Yex.CallbackHandleris not a module but an atom constant used for message identification in thehandle_on_update_handler/0pattern matching (lines 358, 362, 366). This is intentional design—atoms tag different callback signatures. The distinction betweenYex.CallbackHandler(doc-level subscriptions) andYex.ObserveCallbackHandler(shared-type observers) is consistent throughout the codebase.
356-376: Verify NIF message formats match the receive patterns.The
handle_on_update_handler/0function handles multiple message formats. The patterns should match what the NIF sends:
- Line 358: 3-tuple
{_, arg1, {Yex.CallbackHandler, handler}}- Line 362: 4-tuple
{:subdocs, event, origin, {Yex.CallbackHandler, handler}}- Line 366: 4-tuple
{_, arg1, arg2, {Yex.CallbackHandler, handler}}- Line 370: 5-tuple
{_, _ref, event, origin, {Yex.ObserveCallbackHandler, handler}}These patterns need confirmation against the actual NIF message construction in
native/yex/src/doc.rsto ensure they match the tuples being sent.
Add callback function-based event handler registration functions to shared type modules: - Array, Map, Text, XmlElement, XmlFragment, and XmlText - observe: Subscribe to events with a callback function - observe_deep: Subscribe to deep events with a callback function This provides an alternative to the existing Subscription-based approach, allowing simpler event handling patterns for user applications.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
lib/shared_type/map.ex (2)
328-348: Consider adding documentation for the new public APIs.The
observe/2andobserve_deep/2functions are public APIs but lack@doccomments. Consider adding documentation that explains:
- What these functions do (observe changes to the map)
- Parameter descriptions (especially the handler function signature)
- Return value (reference for unsubscribing)
- Usage examples
Example:
@doc """ Observes changes to this map, invoking the handler function when modifications occur. ## Parameters * `map` - The map to observe * `handler` - A function `(event, origin) -> nil` called on each change ## Returns A reference that can be used to unobserve via `Yex.SharedType.unobserve/1` ## Examples iex> map = Yex.Doc.get_map(doc, "map") iex> ref = Yex.Map.observe(map, fn event, origin -> ...> IO.inspect({event, origin}) ...> end) """
328-348: Consider extracting the observe pattern to reduce duplication.The observe/observe_deep implementations are identical across Map, Text, Array, XmlElement, XmlFragment, and XmlText modules. While the current approach is clear and explicit, you could optionally extract this pattern using
defmacrooruseto reduce duplication and ensure consistency.However, given the simplicity of the delegation pattern, the current explicit approach is acceptable and may be preferred for clarity.
lib/shared_type/xml_element.ex (1)
354-374: Consider adding documentation for the new public APIs.The
observe/2andobserve_deep/2functions are public APIs but lack@doccomments. Adding documentation would help users understand:
- Purpose: Observe changes to the XML element and its descendants
- Handler function signature and when it's invoked
- Return value usage for unsubscribing
- Example usage patterns
This is consistent with the documentation pattern used elsewhere in the module for other public functions like
insert/3,delete/3, etc.lib/shared_type/array.ex (1)
391-411: Consider adding documentation for the new public APIs.The
observe/2andobserve_deep/2functions are public APIs but lack@doccomments. Consider documenting:
- Purpose: Observe changes to the array and its nested elements
- Handler parameter expectations
- Return value and how to unsubscribe
- Usage examples
The type specs are correctly defined, and the implementation follows the established pattern from other shared types.
lib/doc.ex (1)
55-61: Good refactoring to extract worker logic.Delegating from the macro to
run_in_worker_process_fn/2makes the worker-routing logic testable and reusable while keeping the macro as a convenient syntax wrapper.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
lib/doc.exlib/shared_type/array.exlib/shared_type/event.exlib/shared_type/map.exlib/shared_type/shared_type.exlib/shared_type/text.exlib/shared_type/xml_element.exlib/shared_type/xml_fragment.exlib/shared_type/xml_text.exlib/y_ex.extest/doc_test.exstest/shared_type/array_test.exstest/shared_type/map_test.exstest/shared_type/text_test.exstest/shared_type/xml_element_test.exstest/shared_type/xml_fragment_test.exstest/shared_type/xml_text_test.exs
🚧 Files skipped from review as they are similar to previous changes (4)
- test/shared_type/xml_element_test.exs
- test/doc_test.exs
- lib/shared_type/text.ex
- test/shared_type/array_test.exs
🧰 Additional context used
🧬 Code graph analysis (7)
lib/shared_type/array.ex (7)
lib/shared_type/map.ex (2)
observe(332-337)observe_deep(343-348)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)lib/shared_type/xml_element.ex (2)
observe(358-363)observe_deep(369-374)lib/shared_type/xml_fragment.ex (2)
observe(287-292)observe_deep(298-303)lib/shared_type/xml_text.ex (2)
observe(146-151)observe_deep(157-162)native/yex/src/event.rs (2)
observe(449-486)observe_deep(397-438)
lib/shared_type/map.ex (6)
lib/shared_type/array.ex (2)
observe(395-400)observe_deep(406-411)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)lib/shared_type/xml_element.ex (2)
observe(358-363)observe_deep(369-374)lib/shared_type/xml_fragment.ex (2)
observe(287-292)observe_deep(298-303)lib/shared_type/xml_text.ex (2)
observe(146-151)observe_deep(157-162)
lib/shared_type/xml_text.ex (7)
lib/shared_type/array.ex (2)
observe(395-400)observe_deep(406-411)lib/shared_type/map.ex (2)
observe(332-337)observe_deep(343-348)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)lib/shared_type/xml_element.ex (2)
observe(358-363)observe_deep(369-374)lib/shared_type/xml_fragment.ex (2)
observe(287-292)observe_deep(298-303)native/yex/src/event.rs (2)
observe(449-486)observe_deep(397-438)
test/shared_type/xml_text_test.exs (4)
lib/doc.ex (1)
get_xml_fragment(190-192)lib/shared_type/xml_fragment.ex (5)
push(161-163)fetch(256-260)observe(287-292)insert(74-78)observe_deep(298-303)lib/shared_type/xml_text.ex (4)
observe(146-151)insert(28-32)insert(39-44)observe_deep(157-162)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)
test/shared_type/map_test.exs (2)
lib/shared_type/map.ex (2)
observe(332-337)observe_deep(343-348)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)
lib/shared_type/xml_element.ex (7)
lib/shared_type/array.ex (2)
observe(395-400)observe_deep(406-411)lib/shared_type/map.ex (2)
observe(332-337)observe_deep(343-348)lib/shared_type/shared_type.ex (2)
observe(40-57)observe_deep(88-105)lib/shared_type/text.ex (2)
observe(209-214)observe_deep(220-225)lib/shared_type/xml_fragment.ex (2)
observe(287-292)observe_deep(298-303)lib/shared_type/xml_text.ex (2)
observe(146-151)observe_deep(157-162)native/yex/src/event.rs (2)
observe(449-486)observe_deep(397-438)
lib/doc.ex (2)
native/yex/src/doc.rs (3)
doc_monitor_subdocs(455-487)doc_monitor_update_v1(297-328)doc_monitor_update_v2(330-360)lib/subscription.ex (1)
register(49-53)
🔇 Additional comments (15)
lib/shared_type/shared_type.ex (1)
42-42: LGTM! Clean enhancement to support cross-process observation.The addition of
notify_pidas an optional parameter withself()as the default maintains backward compatibility while enabling callers to route notifications to a different process (e.g.,doc.worker_pidas shown in the context). The implementation is consistent across bothobserve/2andobserve_deep/2.Also applies to: 90-90
lib/y_ex.ex (1)
18-24: LGTM! Theevent_typeunion is now complete.The type union correctly includes all event types defined in
lib/shared_type/event.ex, includingYex.XmlEvent.t()(which was previously flagged as missing in past reviews) and the newly addedYex.WeakEvent.t(). This provides comprehensive type coverage for all observable events across shared types.test/shared_type/xml_text_test.exs (2)
355-375: LGTM! Well-structured test for XmlText observe functionality.The test correctly validates that:
- Handlers registered via
XmlText.observe/2are invoked on transactional changes- The handler receives the expected
Yex.XmlTextEventstruct and origin value- The observer pattern integrates properly with
Doc.transaction/3
377-397: LGTM! Comprehensive test for deep observation.The test properly validates the
observe_deepfunctionality, ensuring handlers receive update notifications with the correct origin when nested changes occur within a transaction.lib/shared_type/event.ex (1)
113-128: Verify thatWeakEventintentionally omits the:targetfield.Unlike other event types in this file (
ArrayEvent,MapEvent,TextEvent,XmlEvent,XmlTextEvent),Yex.WeakEventonly contains a:pathfield and lacks a:targetfield. While this may be intentional based on weak reference semantics, please confirm this design choice is correct for the use case.If the
:targetfield should be included, here's the proposed fix:🔎 Proposed fix to add target field
defmodule Yex.WeakEvent do @moduledoc """ Event when Weak type changes @see Yex.SharedType.observe/1 @see Yex.SharedType.observe_deep/1 """ defstruct [ - :path + :path, + :target ] @type t :: %__MODULE__{ - path: list(number() | String.t()) + path: list(number() | String.t()), + target: term() } endtest/shared_type/map_test.exs (2)
248-263: LGTM! Solid test coverage for Map observe functionality.The test validates that handlers registered via
Map.observe/2correctly receiveYex.MapEventnotifications when the map is modified within a transaction, including proper origin propagation.
265-280: LGTM! Comprehensive deep observation test.The test properly exercises the
Map.observe_deep/2functionality, ensuring deep observers are notified of changes with the correct origin value.test/shared_type/xml_fragment_test.exs (2)
483-498: LGTM! Handler-based observe test is well-structured.The test correctly validates that the handler is invoked with the expected event type and origin when a transaction modifies the XmlFragment.
500-515: LGTM! Handler-based observe_deep test is consistent.The test correctly validates that the deep handler is invoked with a list of updates and the origin. The detailed event structure validation is already covered by the existing observe_deep test at lines 321-371.
test/shared_type/text_test.exs (1)
761-799: LGTM! Handler-based observer tests are well-structured.Both tests correctly validate that the observe and observe_deep handlers are invoked with the expected event types and origin. The implementation is consistent with the tests in other shared type modules.
lib/shared_type/xml_text.ex (1)
142-162: LGTM! Consistent observer implementation.The
observe/2andobserve_deep/2functions follow the established pattern used across other shared types (Text, Array, Map, XmlElement, XmlFragment). Both correctly delegate toYex.SharedType, useYex.ObserveCallbackHandlerfor metadata, and route notifications viadoc.worker_pid.lib/shared_type/xml_fragment.ex (1)
283-303: LGTM! Observer functions correctly implemented.The
observe/2andobserve_deep/2implementations are consistent with the pattern used across all shared types in this PR. The event types (Yex.XmlEvent.t()andlist(Yex.event_type())) are appropriate for XmlFragment.lib/doc.ex (3)
63-104: Solid cross-process delegation with proper error handling.The three-branch logic correctly handles local execution, missing worker_pid, and cross-process delegation. The error propagation mechanism using
{Yex.Doc, :reraise, e, __STACKTRACE__}ensures exceptions are properly reraised in the calling process. Thehandle_on_update_handler/0calls in both paths ensure callbacks are invoked consistently.
302-315: LGTM! Doc-level subdocs monitoring correctly implemented.The
on_subdocs/2function correctly usesYex.CallbackHandler(for doc-level callbacks) and follows the same pattern ason_update_v1/2andon_update_v2/2.
317-354: LGTM! Update monitoring functions are well-structured.All three
on_update*functions follow a consistent pattern with correct specs indicating handlers receive two arguments(update :: binary(), origin :: term()). The delegation fromon_update/2toon_update_v1/2provides a sensible default.
| {_, arg1, {Yex.CallbackHandler, handler}} -> | ||
| handler.(arg1) | ||
| handle_on_update_handler() |
There was a problem hiding this comment.
Remove incorrect receive clause - no 3-tuple messages are sent.
This receive clause expects a 3-tuple {_, arg1, {Yex.CallbackHandler, handler}} and calls the handler with only one argument. However:
- The NIF code sends 4-tuples for doc-level callbacks (
subdocs,update_v1,update_v2) - All handler specs (lines 317, 322, 339) define handlers that take two arguments:
(update, origin) - There is no code path that sends a 3-tuple message
This clause will never match and would call handlers incorrectly if it did. The other clauses (lines 362-364, 366-368, 370-372) correctly handle all expected message formats.
🔎 Proposed fix
defp handle_on_update_handler() do
receive do
- {_, arg1, {Yex.CallbackHandler, handler}} ->
- handler.(arg1)
- handle_on_update_handler()
-
{:subdocs, event, origin, {Yex.CallbackHandler, handler}} ->
handler.(event, origin)
handle_on_update_handler()🤖 Prompt for AI Agents
In lib/doc.ex around lines 358 to 360, remove the incorrect receive clause that
matches a 3-tuple `{_, arg1, {Yex.CallbackHandler, handler}}`; it never occurs
(NIF sends 4-tuples) and would call handlers with a single arg contrary to
handler specs expecting two args. Delete that branch so only the existing
clauses that match 4-tuples and invoke handlers with (update, origin) remain;
ensure no other code references or relies on the removed pattern.
2 participants
…t, XmlFragment, and XmlText modules
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.