Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces high-level signal APIs along with extensive tests that verify the behavior of various signal types (value, number, list, map, computed, and node signals). Key changes include new implementations for signal types and test cases for operations such as insertion, update, atomic increments, and verification conditions.
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| signals/src/test/java/com/vaadin/signals/impl/AsynchronousSignalTreeTest.java | Changed AsyncTestTree from package-private to public, expanding its visibility. |
| signals/src/main/java/com/vaadin/signals/* | New and updated signal API implementations for value, number, list, map, computed, and node signals. |
| signals/src/test/java/com/vaadin/signals/* | New test classes covering various signal operations and behaviors. |
Comments suppressed due to low confidence (1)
signals/src/test/java/com/vaadin/signals/impl/AsynchronousSignalTreeTest.java:23
- [nitpick] The change from package-private to public increases exposure of the test class. If broader visibility is intended, consider adding a comment explaining the rationale; otherwise, reverting to a more restricted access modifier may be appropriate.
public static class AsyncTestTree extends AsynchronousSignalTree {
taefi
left a comment
There was a problem hiding this comment.
Decided to leave some comments as early as possible, since some of them affect the review process for the rest of the PR.
Co-authored-by: Soroosh Taefi <taefi.soroosh@gmail.com>
mshabarov
left a comment
There was a problem hiding this comment.
Posting my first group of comments - reviewed Signal, SignalEnvironment, UsageTracker.
Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
mshabarov
left a comment
There was a problem hiding this comment.
A second round - reviewed Effect, SignalFactory.
mshabarov
left a comment
There was a problem hiding this comment.
Reviewed ComputedSignal.
signals/src/main/java/com/vaadin/signals/impl/ComputedSignal.java
Outdated
Show resolved
Hide resolved
signals/src/main/java/com/vaadin/signals/impl/UsageTracker.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Soroosh Taefi <taefi.soroosh@gmail.com> Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
signals/src/main/java/com/vaadin/signals/impl/ComputedSignal.java
Outdated
Show resolved
Hide resolved
mshabarov
left a comment
There was a problem hiding this comment.
The unit-tests validation fails because of javadocs errors, e.g. empty return tag, but otherwise looks good to me.
|
Javadoc fixes pushed |
|
|
This ticket/PR has been released with Vaadin 24.8.0.alpha5 and is also targeting the upcoming stable 24.8.0 version. |



No description provided.