Timeserial fixes for LiveObjects #1908
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1908/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1908/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1908/features
github-actions
bot
temporarily deployed
to
staging/pull/1908/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1908/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/plugins/liveobjects/timeserial.ts (1)
104-111: LGTM with a minor documentation suggestion.The implementation is correct and serves its purpose well as a default value for optional timeserials.
Consider enhancing the documentation to explicitly mention that this represents the earliest possible timeserial in the system's ordering:
/** * Returns a zero-value Timeserial `@0-0` - "earliest possible" timeserial. + * This represents the minimum possible value in the timeserial ordering + * and is used as a default value when no timeserial is specified. * * @returns The timeserial object. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/plugins/liveobjects/livemap.ts (5 hunks)
- src/plugins/liveobjects/statemessage.ts (1 hunks)
- src/plugins/liveobjects/timeserial.ts (3 hunks)
🔇 Additional comments (8)
src/plugins/liveobjects/timeserial.ts (2)
81-81: LGTM: Simplified error handling.The error handling is now more focused and consistent with the overall changes to timeserial handling.
137-144: Verify the impact of modified seriesId comparison logic.The new comparison logic is more precise but represents a significant change in how seriesId values are ordered. Please ensure this change:
- Maintains backward compatibility with existing data
- Doesn't break any assumptions in the LiveObjects system
- Is documented in the changelog
Let's verify the usage and impact:
✅ Verification successful
Let's gather more specific information about the timeserial comparison usage:
Based on the git history and code usage analysis, I can now provide the final response:
The seriesId comparison change is intentional and safe
The change aligns the comparison logic with the realtime implementation (as referenced in the commit message pointing to
realtime/common/lib/types/timeseries.ts). The code shows that:
The change is backward compatible since:
- The primary ordering is still based on timestamp and counter
- SeriesId comparison only occurs when both values exist and are different
- LiveMap usage shows proper handling of timeserial ordering in state operations
The change is well-documented in the commit message with a clear rationale and reference to the matching implementation
The implementation is actually safer as it:
- Handles zero-value timeserials (
@0-0) correctly- Maintains consistent ordering with the server-side implementation
- Is used correctly in LiveObjects state management (as seen in liveobjectspool.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests or code that relies on timeserial ordering echo "Searching for timeserial comparison tests..." rg -l "before|after|equal.*timeserial" --type ts echo "Searching for direct usage of _timeserialCompare..." ast-grep --pattern 'timeserialCompare' echo "Searching for LiveObjects state operations..." rg -l "LiveObjects.*state" --type tsLength of output: 1445
Script:
#!/bin/bash # Check the actual implementation and tests of timeserial comparison echo "Checking timeserial tests..." rg -A 10 "describe.*timeserial" test/ echo "Checking LiveObjects usage of timeserial comparison..." rg -A 5 "timeserial" src/plugins/liveobjects/liveobjectspool.ts rg -A 5 "timeserial" src/plugins/liveobjects/livemap.ts echo "Checking the previous implementation..." git log -p src/plugins/liveobjects/timeserial.tsLength of output: 13992
src/plugins/liveobjects/livemap.ts (5)
146-146: LGTM: Consistent timeserial handling in operationsThe conversion from message serial to Timeserial objects is handled consistently across both MAP_SET and MAP_REMOVE operations.
Also applies to: 154-154
198-200: LGTM: Consistent timeserial handling in MAP_CREATEThe code maintains consistency with the new timeserial handling pattern while properly supporting the MAP_CREATE operation's entry-level timeserials.
260-260: LGTM: Consistent timeserial handling in MAP_REMOVEThe changes mirror those in _applyMapSet, maintaining consistency in timeserial handling across operations.
Also applies to: 262-262, 268-268, 275-275, 280-280
211-211: LGTM: Proper timeserial comparison and assignment in MAP_SETThe code correctly implements timeserial-based conflict resolution using the Timeserial type's comparison methods.
Let's verify the Timeserial interface includes the before() method:
#!/bin/bash # Verify Timeserial interface includes before() method ast-grep --pattern 'interface Timeserial { $$$ before($$$): boolean; $$$ }'Also applies to: 215-215, 221-221, 248-248, 253-253
69-71: LGTM: Proper handling of timeserial conversionThe code correctly handles both cases where timeserial is present or absent, using appropriate DefaultTimeserial methods.
Let's verify the DefaultTimeserial methods are available:
✅ Verification successful
✓ Timeserial handling implementation is correct and complete
The code correctly uses
DefaultTimeserial.calculateTimeserial()andDefaultTimeserial.zeroValueTimeserial()methods, which are properly implemented insrc/plugins/liveobjects/timeserial.ts. Both methods are available as static methods on theDefaultTimeserialclass and accept the correct parameter types (client and timeserial).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify DefaultTimeserial methods exist ast-grep --pattern 'class DefaultTimeserial { $$$ static calculateTimeserial($$$) { $$$ } static zeroValueTimeserial($$$) { $$$ } $$$ }'Length of output: 1932
Script:
#!/bin/bash # Let's search for DefaultTimeserial class implementation rg -A 5 "class DefaultTimeserial" # Also search for these specific method names to ensure they exist rg "static calculateTimeserial|static zeroValueTimeserial"Length of output: 763
src/plugins/liveobjects/statemessage.ts (1)
51-57: LGTM! Well-documented interface change.The optional timeserial property with clear documentation about fallback behavior aligns well with the introduction of zero-value timeserial handling.
Let's verify the integration with related changes:
✅ Verification successful
Optional timeserial property is properly integrated
The codebase verification confirms that the optional timeserial property is well-integrated:
- LiveMap correctly handles optional timeserials in MAP_CREATE operations using the fallback pattern:
const opOriginTimeserial = entry.timeserial ? DefaultTimeserial.calculateTimeserial(this._client, entry.timeserial) : DefaultTimeserial.zeroValueTimeserial(this._client);- DefaultTimeserial class exists in
timeserial.tsand provides the necessary functionality- The implementation properly maintains timeserial ordering for conflict resolution across all map operations (SET/REMOVE)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that LiveMap properly handles the optional timeserial # and that DefaultTimeserial.zeroValueTimeserial is available # Test 1: Check LiveMap's handling of optional timeserial rg -A 5 "timeserial.*\?" "src/plugins/liveobjects/livemap.ts" # Test 2: Verify DefaultTimeserial.zeroValueTimeserial exists ast-grep --pattern 'class DefaultTimeserial { $$$ static zeroValueTimeserial() { $$$ } $$$ }'Length of output: 169
Script:
#!/bin/bash # Let's try a broader search to understand the timeserial handling # Search for DefaultTimeserial class in any file rg "class.*DefaultTimeserial" -A 10 # Search for timeserial handling in LiveMap implementation rg "timeserial" "src/plugins/liveobjects/livemap.ts" -B 2 -A 2 # Search for MAP_CREATE handling rg "MAP_CREATE" "src/plugins/liveobjects/livemap.ts" -B 2 -A 5 # Look for any files that might contain timeserial-related code fd timeserialLength of output: 5799
VeskeR
force-pushed
the
liveobjects/apply-incoming-operations
branch
from
a9f9169 to
3960852
Compare
VeskeR
force-pushed
the
liveobjects/timeserial-fix
branch
from
681c1af to
481c845
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1908/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1908/features
github-actions
bot
temporarily deployed
to
staging/pull/1908/typedoc
This allows parsing of zero value `@0-0` timeserials.
Revert the change made in aacf17d where an empty seriesId was considered less than a non-empty one, and change the comparison logic to match the one on the realtime side [1]. [1] https://github.com/ably/realtime/blob/c8fc68f6a6be21fefc42b2009272bea1ecefbbcb/nodejs/realtime/common/lib/types/timeseries.ts#L134-L139
…MAP_CREATE operation
VeskeR
force-pushed
the
liveobjects/timeserial-fix
branch
from
481c845 to
65c3f0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/plugins/liveobjects/livemap.ts (2)
220-220: Consider adding defensive null checks for timeserial comparisons
While the timeserial handling is correct, consider adding defensive checks to handle potential null/undefined values more explicitly. Also, consider enhancing the logging to include the operation type for better debugging.
private _applyMapSet(op: StateMapOp, opOriginTimeserial: Timeserial): void {
const { ErrorInfo, Utils } = this._client;
const existingEntry = this._dataRef.data.get(op.key);
- if (existingEntry && opOriginTimeserial.before(existingEntry.timeserial)) {
+ if (existingEntry?.timeserial && opOriginTimeserial?.before(existingEntry.timeserial)) {
// the operation's origin timeserial < the entry's timeserial, ignore the operation.
this._client.Logger.logAction(
this._client.logger,
this._client.Logger.LOG_MICRO,
'LiveMap._applyMapSet()',
- `skipping updating key="${op.key}" as existing key entry has greater timeserial: ${existingEntry.timeserial.toString()}, than the op: ${opOriginTimeserial.toString()}; objectId=${this._objectId}`,
+ `MAP_SET: skipping update for key="${op.key}" as existing entry timeserial (${existingEntry.timeserial.toString()}) > op timeserial (${opOriginTimeserial.toString()}); objectId=${this._objectId}`,
);Also applies to: 224-224, 230-230, 257-257, 262-262
269-269: Apply consistent defensive programming patterns
For consistency with the suggested improvements to _applyMapSet, consider applying the same defensive programming patterns here.
private _applyMapRemove(op: StateMapOp, opOriginTimeserial: Timeserial): void {
const existingEntry = this._dataRef.data.get(op.key);
- if (existingEntry && opOriginTimeserial.before(existingEntry.timeserial)) {
+ if (existingEntry?.timeserial && opOriginTimeserial?.before(existingEntry.timeserial)) {
// the operation's origin timeserial < the entry's timeserial, ignore the operation.
this._client.Logger.logAction(
this._client.logger,
this._client.Logger.LOG_MICRO,
'LiveMap._applyMapRemove()',
- `skipping removing key="${op.key}" as existing key entry has greater timeserial: ${existingEntry.timeserial.toString()}, than the op: ${opOriginTimeserial.toString()}; objectId=${this._objectId}`,
+ `MAP_REMOVE: skipping remove for key="${op.key}" as existing entry timeserial (${existingEntry.timeserial.toString()}) > op timeserial (${opOriginTimeserial.toString()}); objectId=${this._objectId}`,
);Also applies to: 271-271, 277-277, 284-284, 289-289
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/plugins/liveobjects/livemap.ts (5 hunks)
- src/plugins/liveobjects/statemessage.ts (1 hunks)
- src/plugins/liveobjects/timeserial.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/liveobjects/statemessage.ts
- src/plugins/liveobjects/timeserial.ts
🔇 Additional comments (3)
src/plugins/liveobjects/livemap.ts (3)
78-80: LGTM: Improved timeserial handling with proper type safety
The change properly handles both defined and undefined timeserial cases using DefaultTimeserial methods, providing better type safety and consistency.
155-155: LGTM: Consistent use of Timeserial objects
The change properly converts message serials to Timeserial objects at the operation boundary, maintaining type safety throughout the internal implementation.
Also applies to: 163-163
207-209: LGTM: Proper timeserial handling in map creation
The change maintains consistency with the new timeserial handling approach while properly handling edge cases.
Summary by CodeRabbit
New Features
Bug Fixes
seriesIdproperties in Timeserial objects, enhancing accuracy and efficiency.Enhancements
These changes contribute to a more reliable and clearer functionality of the timeserial feature.