feat: implement shared signals command rejection, latency compensation, and MapSignal - #4890
Open
Artur- wants to merge 22 commits into
Open
feat: implement shared signals command rejection, latency compensation, and MapSignal#4890Artur- wants to merge 22 commits into
Artur- wants to merge 22 commits into
Conversation
Artur-
commented
Mar 16, 2026
Artur-
commented
Member
- Server (InternalSignal): add accepted/reason fields to command JSON, notify only the originating client on rejection, handle server-originated commands without NPE
- Client (FullStackSignal): route rejected commands via $handleRejection
- ValueSignal: track confirmed value and pending sets, revert on rejection
- NumberSignal: optimistic incrementBy with pending tracking, revert on rejection
- ListSignal: optimistic insert/remove, add clear(), revert on rejection
- Add MapSignal (TS + Java transfer type + generator plugin mapping)
- Add ClearCommand, PutCommand, RemoveByKeyCommand to commands.ts
- Re-enable all 4 integration tests (BasicSignalIT, NumberSignalIT, SignalBasicSecurityIT, ParamsIT)
- Fix test services: use peek() instead of get() for non-reactive context
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4890 +/- ##
========================================
Coverage 85.94% 85.95%
========================================
Files 129 131 +2
Lines 8821 9012 +191
Branches 1345 1381 +36
========================================
+ Hits 7581 7746 +165
- Misses 1216 1247 +31
+ Partials 24 19 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
platosha
requested changes
Mar 20, 2026
platosha
left a comment
platosha
left a comment
Member
There was a problem hiding this comment.
This extends the current implementation of optimistic updates, which has known design issues with its core approach.
A better design is sketched here: #3968 (comment), and some implementation work was started in this branch: main...refactor/node-signals
The main differences are:
- There is a common unconfirmed commands queue in the signal for all commands instead of ad-hoc command-specific queues here and there
- There is no
setValueQuietlyat all, the method is removed entirely. Instead, the client-side signal state consists of the confirmed state and the unconfirmed commands queue. The value of the signal is always computed by applying the current unconfirmed commands over the current confirmed state. - Rejection removes the unconfirmed command from the unconfirmed queue. No manual reverting of the state is needed — modifying the unconfirmed queue triggers re-computation of the value explained above, the value is expected to update accordingly.
The branch also contains some good foundation cleanup, would be great to incorporate that. Maybe we can even merge it here and continue from then.
github-project-automation
Bot
moved this to 🔎Iteration reviews
in Vaadin Flow | Hilla | Kits ongoing work
…n, and MapSignal - Server (InternalSignal): add accepted/reason fields to command JSON, notify only the originating client on rejection, handle server-originated commands without NPE - Client (FullStackSignal): route rejected commands via $handleRejection - ValueSignal: track confirmed value and pending sets, revert on rejection - NumberSignal: optimistic incrementBy with pending tracking, revert on rejection - ListSignal: optimistic insert/remove, add clear(), revert on rejection - Add MapSignal (TS + Java transfer type + generator plugin mapping) - Add ClearCommand, PutCommand, RemoveByKeyCommand to commands.ts - Re-enable all 4 integration tests (BasicSignalIT, NumberSignalIT, SignalBasicSecurityIT, ParamsIT) - Fix test services: use peek() instead of get() for non-reactive context
Fix line-break style violation in InternalSignal.java and add comprehensive tests for MapSignal covering put, removeKey, clear, snapshot processing, rejection handling, and promise resolution.
Add tests for ListSignal (optimistic insert/remove, clear, rejection reversal), NumberSignal (optimistic increment, rejection reversal, snapshot reset), and ValueSignal (confirmed value tracking, rejection reversal, multiple pending sets).
…architecture Switch from per-signal-type pending tracking (#pendingInserts, #pendingSetIds, etc.) to a unified confirmed tree + unconfirmed commands queue. The displayed value is now computed as: confirmed tree + all unconfirmed commands applied in order. Rejection simply removes from the queue and the computed re-derives. Key changes: - Create NodeTree.ts with pure applyCommand() function for all command types - Extract Connection.ts from FullStackSignal for cleaner separation - Rewrite FullStackSignal with #confirmed/#unconfirmed/#derived architecture - Simplify all signal subclasses (ValueSignal, NumberSignal, ListSignal, MapSignal) - Delete CollectionSignal.ts and types.ts (no longer needed) - Switch core.ts to import from @preact/signals-core - Add comprehensive NodeTree unit tests - Update all existing tests for new architecture
…ignals-react The switch to @preact/signals-core dropped React hook re-exports that other packages (react-i18n, test apps) depend on.
Since FullStackSignal no longer extends Signal, it does not inherit the
$$typeof, type, props, and ref properties that @preact/signals-react/runtime
patches onto Signal.prototype. Copy these properties in the constructor
so that FullStackSignal instances render directly in JSX ({signal} syntax).
Instead of copying $$typeof/type/props/ref from Signal.prototype (which
may not be patched yet at construction time), define them directly using
the React element symbol and useSignals hook from @preact/signals-react/runtime.
This ensures FullStackSignal instances render in JSX ({signal} syntax)
regardless of import order.
When a NumberSignal is created but hasn't received a server snapshot yet, the tree root value is undefined. Optimistic incrementBy() calls would fail silently (applyCommand returned null) because undefined is not a number. Now undefined is treated as 0, matching the server-side default.
- NumberSignal.valueAsInt() - ListSignal.insertAt() with custom position - NodeTree insert after, between, and between constraint violation - FullStackSignal fallback when unconfirmed command fails to apply - FullStackSignal valueOf/toString/toJSON delegation - Disconnect without active subscription (no-op)
- Non-Error rejection wrapping in FullStackSignal.sendCommand - peek() method on FullStackSignal - Remove map child via remove command in NodeTree
Extract the anonymous type function to a named SignalValue component so that SonarCloud and React hook lint rules accept the useSignals call.
sinon's rejects() wraps strings in Error objects, so use Promise.reject(42) directly to test the non-Error branch.
…ead of signals-core Exporting signal/computed/effect from @preact/signals-core bypasses the React integration setup in @preact/signals-react/runtime. This caused i18n and grid tests to fail because signals weren't triggering React re-renders. Since @preact/signals-react re-exports the same functions from @preact/signals-core, switching the export source restores React integration without changing functionality.
Without this import, tree-shaking can prevent the React integration patches (Signal.prototype.$$typeof, etc.) from being loaded when consumers only import non-React primitives like effect or signal. This caused i18n and grid tests to fail because signal reads inside React components weren't triggering re-renders.
…tern Named re-exports with side-effect import caused i18n react integration tests to fail in CI (signals not triggering React re-renders). The export * pattern used on main ensures the full module is evaluated, which is required for @preact/signals-react's React integration to work correctly when consumed via vitest's browser mode in CI.
- Remove @preact/signals-core from dependencies since we only import types from it (now imported from @preact/signals-react instead) - Having both as direct dependencies could cause duplicate module instances in CI, breaking React signal integration - Use export * from @preact/signals-react in core.ts (matches main) - Remove unnecessary type arguments on createInsertCommand and createPutCommand that caused ESLint @typescript-eslint/ no-unnecessary-type-arguments rule to crash
The @typescript-eslint/no-unnecessary-type-arguments rule crashes on createPutCommand<string> in test/MapSignal.spec.ts. The type is inferred from the value parameter so the explicit argument is redundant.
Member
Author
|
Removed unnecessary changes |
Port 10 behavioral tests from main to cover: - Remove non-existing entries (server command + client call) - Insert at specific positions (after/before) - Set command targeting child node - Snapshot with missing child value nodes (graceful filtering) - Position condition commands - Adopt-at commands (move children, specific positions, non-existing) - Promise resolution for non-existing entry removal Also fix deriveValue in ListSignal and MapSignal to use optional chain check (childNode?.value !== undefined) instead of 'value' in childNode, since NodeData always has the value key as optional.
Add Selenium IT tests for SharedListSignal covering: - Add item and sync to other client - Toggle child value and sync to other client - Remove item and sync to other client - Clear all and sync to other client Includes ListSignalService (Java endpoint) and SharedListSignal frontend view used by the tests.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.