[DTP-1078] Use lexico timeserials and siteCode field in StateMessages
#1926
Conversation
WalkthroughThe pull request introduces significant changes to the handling of timeserials across multiple classes within the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@VeskeR has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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/1926/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1926/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: 1
🧹 Outside diff range and nitpick comments (8)
src/plugins/liveobjects/liveobject.ts (1)
108-118: LGTM! Consider improving error messages.The validation and comparison logic is solid. The string-based comparison works correctly for lexicographically ordered timeserials.
Consider making the error messages more descriptive:
- throw new this._client.ErrorInfo(`Invalid timeserial: ${opOriginTimeserial}`, 50000, 500); + throw new this._client.ErrorInfo(`Operation timeserial cannot be undefined or empty: ${opOriginTimeserial}`, 50000, 500); - throw new this._client.ErrorInfo(`Invalid site code: ${opSiteCode}`, 50000, 500); + throw new this._client.ErrorInfo(`Operation site code cannot be undefined or empty: ${opSiteCode}`, 50000, 500);src/plugins/liveobjects/livecounter.ts (1)
51-52: Consider adding runtime validation for non-null assertions.While TypeScript's type system ensures these values exist, consider adding explicit runtime checks for better robustness:
- const opOriginTimeserial = msg.serial!; - const opSiteCode = msg.siteCode!; + if (!msg.serial || !msg.siteCode) { + throw new this._client.ErrorInfo( + `Missing serial or siteCode in state message for LiveCounter objectId=${this.getObjectId()}`, + 50000, + 500 + ); + } + const opOriginTimeserial = msg.serial; + const opSiteCode = msg.siteCode;src/plugins/liveobjects/statemessage.ts (2)
143-144: Consider enhancing siteCode documentationWhile the basic documentation is clear, it would be helpful to add:
- Format constraints for the site code
- Relationship with the serial property
- Example values
Example enhancement:
- /** Site code corresponding to this message's timeserial */ + /** + * Site code corresponding to this message's timeserial. + * Format: [constraints here] + * Example: "site1" + * @see serial - This property is used in conjunction with the serial property + */
Line range hint
1-374: Verify integration with dependent componentsThe addition of
siteCodeand changes to timeserial handling may affect components that:
- Create or process StateMessages
- Perform message ordering/comparison
- Handle message serialization/deserialization
Consider:
- Documenting the migration path for existing clients
- Adding integration tests for timeserial comparison scenarios
- Updating any message schemas or API documentation
src/plugins/liveobjects/livemap.ts (3)
371-381: Ensure safe logging of potentially undefined timeserialsIn the logging statement,
opOriginTimeserial?.toString()andexistingEntry.timeserial?.toString()may result in'undefined'being logged if the timeserials are not defined. Verify that this is acceptable or consider providing a default value for clearer logs.
421-432: Duplicate logic in_applyMapSetand_applyMapRemoveBoth
_applyMapSetand_applyMapRemovemethods include similar logic for checking if an operation should be applied based on timeserials. Consider refactoring this common logic into a shared helper method to reduce code duplication and improve maintainability.Apply this diff to introduce a common method for the timeserial check:
private _applyMapSet(op: StateMapOp, opOriginTimeserial: string | undefined): LiveMapUpdate | LiveObjectUpdateNoop { const { ErrorInfo, Utils } = this._client; const existingEntry = this._dataRef.data.get(op.key); - if (existingEntry && !this._canApplyOperationFollowingCRDTRules(existingEntry.timeserial, opOriginTimeserial)) { + if (!this._shouldApplyOperation(existingEntry?.timeserial, opOriginTimeserial, op.key, 'update')) { return { noop: true }; } // ... rest of the method ... private _applyMapRemove(op: StateMapOp, opOriginTimeserial: string | undefined): LiveMapUpdate | LiveObjectUpdateNoop { const existingEntry = this._dataRef.data.get(op.key); - if (existingEntry && !this._canApplyOperationFollowingCRDTRules(existingEntry.timeserial, opOriginTimeserial)) { + if (!this._shouldApplyOperation(existingEntry?.timeserial, opOriginTimeserial, op.key, 'remove')) { return { noop: true }; } // ... rest of the method ... + private _shouldApplyOperation( + entryTimeserial: string | undefined, + opTimeserial: string | undefined, + key: string, + action: string + ): boolean { + if (entryTimeserial && !this._canApplyOperationFollowingCRDTRules(entryTimeserial, opTimeserial)) { + this._client.Logger.logAction( + this._client.logger, + this._client.Logger.LOG_MICRO, + `LiveMap._applyMap${action.charAt(0).toUpperCase() + action.slice(1)}()`, + `skipping ${action} for key="${key}": op timeserial ${opTimeserial?.toString()} <= entry timeserial ${entryTimeserial?.toString()}; objectId=${this._objectId}`, + ); + return false; + } + return true; + }
499-499: Initialize 'tombstone' property explicitlyWhen creating
liveDataEntry, ensure that thetombstoneproperty is explicitly set tofalseif it's not a tombstone. This enhances code clarity.Apply this diff:
timeserial: entry.timeserial, - tombstone: entry.tombstone === true, + tombstone: entry.tombstone === true ? true : false, data: liveData,test/realtime/live_objects.test.js (1)
44-56: Consider adding input validation tolexicoTimeserialfunctionThe
lexicoTimeserialfunction currently does not validate its inputs, which could lead to unexpected behavior if negative numbers or non-integer values are passed. It's recommended to add input validation to ensure thattimestamp,counter, andindexare non-negative integers. This will prevent issues with padding and formatting the timeserial string.Apply this diff to add input validation:
function lexicoTimeserial(seriesId, timestamp, counter, index) { + if (!Number.isInteger(timestamp) || timestamp < 0) { + throw new Error('timestamp must be a non-negative integer'); + } + if (!Number.isInteger(counter) || counter < 0) { + throw new Error('counter must be a non-negative integer'); + } + if (index != null && (!Number.isInteger(index) || index < 0)) { + throw new Error('index must be a non-negative integer if provided'); + } const paddedTimestamp = timestamp.toString().padStart(14, '0'); const paddedCounter = counter.toString().padStart(3, '0'); const paddedIndex = index != null ? index.toString().padStart(3, '0') : undefined; // Example: // ... return `${paddedTimestamp}-${paddedCounter}@${seriesId}` + (paddedIndex ? `:${paddedIndex}` : ''); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/plugins/liveobjects/livecounter.ts(2 hunks)src/plugins/liveobjects/livemap.ts(8 hunks)src/plugins/liveobjects/liveobject.ts(2 hunks)src/plugins/liveobjects/statemessage.ts(3 hunks)src/plugins/liveobjects/timeserial.ts(0 hunks)test/common/modules/live_objects_helper.js(1 hunks)test/realtime/live_objects.test.js(19 hunks)
💤 Files with no reviewable changes (1)
- src/plugins/liveobjects/timeserial.ts
🔇 Additional comments (17)
src/plugins/liveobjects/liveobject.ts (1)
40-40: LGTM! Verify impact on derived classes.
The change from Timeserial object to string representation aligns well with making state messages opaque to the client. However, ensure that all derived classes properly handle string-based timeserial comparisons.
✅ Verification successful
LGTM! String timeserial handling is properly implemented.
The verification shows that:
- Only one derived class exists (
LiveCounter) and it correctly handles string timeserials:- Uses string comparison in log messages (line 58)
- Directly assigns string timeserials (line 64)
- Accepts string timeserials in state objects (line 128)
- No remaining
Timeserialtype usage in the codebase, only string representations in tests and message passing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check derived classes of LiveObject to ensure proper handling of string timeserials
# Find all derived classes
ast-grep --pattern 'class $_ extends LiveObject {
$$$
}'
# Check for any remaining Timeserial type usage
rg -i 'timeserial.*:.*Timeserial'
Length of output: 18799
src/plugins/liveobjects/livecounter.ts (3)
51-64: LGTM: Changes align with lexico timeserials objective.
The implementation correctly transitions to using string-based timeserials and site codes, maintaining consistency with the PR objectives.
128-128: LGTM: Simplified timeserials assignment.
The direct assignment of stateObject.siteTimeserials aligns with the transition to string-based timeserials and reduces complexity.
Line range hint 51-128: Verify consistent timeserial handling across the codebase.
Let's ensure the transition to string-based timeserials is consistent across all related components.
✅ Verification successful
String-based timeserial implementation is consistent across the codebase
The verification shows consistent implementation of string-based timeserials throughout the codebase:
- All timeserial type definitions use string type (
Record<string, string>for site timeserials) - Consistent string comparison for timeserial operations (
>,<=) - No instances of Timeserial class usage or instantiation found
- Proper string-based timeserial handling in:
- LiveObject base class
- LiveCounter implementation
- LiveMap implementation
- State message interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent timeserial handling
# Check for any remaining Timeserial object usage
rg -t typescript "new.*Timeserial|instanceof.*Timeserial" src/
# Check type definitions for timeserials
ast-grep --pattern 'type $name = { $$$timeserial$$$: $type }' src/
ast-grep --pattern 'interface $name { $$$timeserial$$$: $type }' src/
Length of output: 240
Script:
#!/bin/bash
# Let's try a different approach to check timeserial handling
# Search for timeserial-related patterns in TypeScript/JavaScript files
rg -g '*.{ts,js}' -i 'timeserial' src/ -B2 -A2
# Look for type annotations or interfaces containing 'serial'
rg -g '*.{ts,js}' '(interface|type).*\{.*serial.*\}' src/ -B1 -A3
# Search for specific operations with serials
rg -g '*.{ts,js}' '_siteTimeserials\[' src/ -B2 -A2
Length of output: 20371
test/common/modules/live_objects_helper.js (1)
223-228: LGTM! The changes correctly support testing the new state message format.
The implementation properly sets both serial and siteCode for each state message, aligning with the PR objectives to support lexico timeserials and the new siteCode field in StateMessages.
src/plugins/liveobjects/statemessage.ts (2)
362-369: LGTM! toString() update is consistent
The addition of siteCode to the string representation follows the established pattern and maintains consistency with other fields.
54-55: Documentation improvement looks good!
The clarification about treating nullish timeserials as "earliest possible" in MAP_CREATE operations provides better guidance for implementations.
Let's verify consistent handling across the codebase:
✅ Verification successful
Documentation is accurate and consistent with implementation
The codebase search confirms that the documentation about nullish timeserials in MAP_CREATE operations is accurate and properly implemented. Key findings:
- In
src/plugins/liveobjects/livemap.ts, MAP_CREATE operations explicitly use the entry-level timeserial (entry.timeserial) instead of message-level timeserial - When merging initial data in
_mergeInitialDataFromCreateOperation, the code correctly handles cases where timeserials might be missing - Test cases in
test/realtime/live_objects.test.jsverify the correct handling of timeserials in MAP_CREATE operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MAP_CREATE handling to ensure consistent treatment of nullish timeserials
ast-grep --pattern 'if ($operation === $_ || $action === $_) {
$$$
MAP_CREATE
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's try a different approach to find MAP_CREATE usage
rg "MAP_CREATE" -A 5 -B 5
Length of output: 17464
src/plugins/liveobjects/livemap.ts (2)
36-36: Update 'timeserial' type to 'string | undefined' ensures consistency
Changing timeserial to string | undefined in the MapEntry interface aligns with the updated handling of timeserials as strings throughout the codebase. This promotes consistency and simplifies timeserial management.
453-482: Verify lexicographical comparison of timeserials in CRDT rules
The method _canApplyOperationFollowingCRDTRules uses lexicographical comparison (opTimeserial > entryTimeserial) to determine the order of timeserials. Confirm that timeserials are formatted so that lexicographical ordering correctly represents their temporal ordering. If timeserials are not designed this way, the comparison may not function as intended.
test/realtime/live_objects.test.js (8)
34-40: Code structure enhancement in forScenarios function
The refactored forScenarios function improves readability by simplifying the iteration over scenarios and the determination of the appropriate it function. This change enhances maintainability.
104-105: Usage of lexicoTimeserial ensures consistency in serial generation
Replacing hardcoded serial strings with lexicoTimeserial function calls improves consistency and reduces the risk of errors in serial formatting across tests.
139-144: Proper integration of siteCode in STATE_SYNC messages
Adding the siteCode parameter to the processStateObjectMessageOnChannel function ensures that state synchronization messages include necessary site information, aligning with the changes introduced in the PR objectives.
311-312: Ensuring accurate state initialization in tests
Including siteTimeserials and initialEntries with lexicoTimeserial in the test setup enhances the accuracy of state initialization, leading to more reliable and meaningful test results.
672-673: Consistent use of lexicoTimeserial and siteCode in operation messages
The test cases correctly utilize the lexicoTimeserial function and include the siteCode, ensuring that operation messages in tests mirror the expected production behavior.
686-691: Comprehensive testing of timeserial scenarios
The array of scenarios in the tests effectively covers cases with varying site codes and timeserials, which is essential for verifying the correct application of operations based on timeserial comparisons.
1255-1269: Proper passing of helper in forScenarios
By modifying the forScenarios utility to pass helper directly to the test functions, the code increases modularity and clarity, making test functions easier to write and maintain.
846-850: Ensuring test data reflects realistic scenarios
The duplication of timeserials in test data (e.g., multiple keys with the same timeserial) accurately reflects potential real-world scenarios and helps ensure that the application logic handles such cases correctly.
VeskeR
force-pushed
the
DTP-1078/lexico-timeserials
branch
from
9232fb0
to
430573e
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1926/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1926/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/plugins/liveobjects/liveobject.ts (2)
108-118: Enhance error handling clarity and consistencyWhile the error handling is good, consider these improvements:
- The error messages could be more descriptive (e.g., "Missing timeserial" instead of "Invalid timeserial: undefined")
- Consider defining error codes as constants to avoid magic numbers
Here's a suggested improvement:
+const ERROR_CODES = { + INVALID_TIMESERIAL: 50000, + INVALID_SITE_CODE: 50001, +} as const; protected _canApplyOperation(opOriginTimeserial: string | undefined, opSiteCode: string | undefined): boolean { if (!opOriginTimeserial) { - throw new this._client.ErrorInfo(`Invalid timeserial: ${opOriginTimeserial}`, 50000, 500); + throw new this._client.ErrorInfo('Missing or invalid timeserial', ERROR_CODES.INVALID_TIMESERIAL, 500); } if (!opSiteCode) { - throw new this._client.ErrorInfo(`Invalid site code: ${opSiteCode}`, 50000, 500); + throw new this._client.ErrorInfo('Missing or invalid site code', ERROR_CODES.INVALID_SITE_CODE, 500); } const siteTimeserial = this._siteTimeserials[opSiteCode]; return !siteTimeserial || opOriginTimeserial > siteTimeserial; }
108-118: Consider documenting the lexicographical ordering contractThe implementation relies on string-based lexicographical ordering for timeserials. Consider adding JSDoc comments to document this contract, especially since it's a critical part of the state synchronization mechanism.
Example documentation:
/** * Determines if an operation can be applied based on lexicographical ordering of timeserials. * * @remarks * The implementation assumes timeserials are formatted in a way that lexicographical * string comparison (`>`) correctly determines their temporal order. This is crucial * for maintaining consistent state across distributed systems. * * @param opOriginTimeserial - The timeserial of the operation to be applied * @param opSiteCode - The site code where the operation originated * @throws {ErrorInfo} If either parameter is undefined * @returns {boolean} True if the operation should be applied */test/realtime/live_objects.test.js (1)
44-56: LGTM! Consider adding input validation.The
lexicoTimeserialfunction implementation is well-documented and correctly formats the timeserial string for lexicographical sorting. The padding ensures consistent string lengths.Consider adding input validation for the parameters to ensure they are within expected ranges:
function lexicoTimeserial(seriesId, timestamp, counter, index) { + if (typeof timestamp !== 'number' || timestamp < 0) { + throw new Error('timestamp must be a non-negative number'); + } + if (typeof counter !== 'number' || counter < 0) { + throw new Error('counter must be a non-negative number'); + } + if (index != null && (typeof index !== 'number' || index < 0)) { + throw new Error('index must be a non-negative number when provided'); + } const paddedTimestamp = timestamp.toString().padStart(14, '0'); const paddedCounter = counter.toString().padStart(3, '0'); const paddedIndex = index != null ? index.toString().padStart(3, '0') : undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
scripts/moduleReport.ts(0 hunks)src/plugins/liveobjects/livecounter.ts(2 hunks)src/plugins/liveobjects/livemap.ts(8 hunks)src/plugins/liveobjects/liveobject.ts(2 hunks)src/plugins/liveobjects/statemessage.ts(3 hunks)src/plugins/liveobjects/timeserial.ts(0 hunks)test/common/modules/live_objects_helper.js(1 hunks)test/realtime/live_objects.test.js(19 hunks)
💤 Files with no reviewable changes (2)
- scripts/moduleReport.ts
- src/plugins/liveobjects/timeserial.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/liveobjects/statemessage.ts
- test/common/modules/live_objects_helper.js
🔇 Additional comments (12)
src/plugins/liveobjects/liveobject.ts (1)
40-40: LGTM: Type change aligns with lexicographical ordering requirement
The change from Timeserial object to string representation simplifies the implementation while maintaining the required ordering semantics.
Let's verify that all timeserial-related code has been updated consistently:
✅ Verification successful
LGTM: Type change is consistent across the codebase
The search results confirm that the change from Timeserial to string type is consistent throughout the codebase. All timeserial-related code now uses string type for:
- The
_siteTimeserialsproperty in LiveObject base class - Entry timeserials in LiveMap
- Operation timeserials in state messages
- Comparison operations in
_canApplyOperationand related methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining Timeserial type references that might have been missed
rg -i "timeserial.*" --type typescript
Length of output: 75
Script:
#!/bin/bash
# Search for any remaining Timeserial type references that might have been missed
rg -i "timeserial"
Length of output: 24752
src/plugins/liveobjects/livecounter.ts (3)
53-64: LGTM: Simplified timeserial handling
The changes align well with the PR objectives:
- Streamlined timeserial comparison in
_canApplyOperation - Clear logging of timeserial comparisons
- Direct timeserial storage per site code
128-128: LGTM: Direct assignment of site timeserials
The simplified assignment aligns with the PR's objective of streamlining timeserial handling.
✅ Verification successful
Let me gather more information about the LiveCounter class to verify type compatibility.
Let me try a different approach to find the LiveCounter class definition.
Let me check the type declarations in the LiveCounter file.
Type compatibility verified for siteTimeserials assignment
The direct assignment is type-safe as both sides are of type Record<string, string>:
StateObject.siteTimeserialsis defined asRecord<string, string>in the interfaceLiveCounter._siteTimeserialsis used consistently with this type, as evidenced by the usage inapplyOperationmethod where string values are assigned to site codes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the type compatibility between StateObject.siteTimeserials and LiveCounter._siteTimeserials
ast-grep --pattern 'interface StateObject {
$$$
siteTimeserials
$$$
}'
Length of output: 1904
Script:
#!/bin/bash
# Check the type declaration of _siteTimeserials in LiveCounter class
ast-grep --pattern 'class LiveCounter {
$$$
_siteTimeserials
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Search for LiveCounter class definition and its properties
rg -A 10 "class LiveCounter" src/
Length of output: 778
Script:
#!/bin/bash
# Check the type declarations and member variables in LiveCounter file
rg -B 5 -A 20 "_siteTimeserials" src/plugins/liveobjects/livecounter.ts
Length of output: 2661
51-52: Verify null safety for msg.serial and msg.siteCode
The non-null assertions (!) on msg.serial and msg.siteCode could be risky if these fields are undefined. Consider adding explicit null checks or document why these fields are guaranteed to be defined.
✅ Verification successful
Let me gather more information about the StateMessage type and its usage.
Based on the analysis of the code, I can now generate the final response:
Non-null assertions on msg.serial and msg.siteCode are safe to use
The non-null assertions are safe because:
- The
StateMessageclass shows thatserialandsiteCodefields are added specifically for timeserial functionality - The codebase consistently uses these fields together in LiveMap and LiveCounter implementations
- The fields are used in logging messages and site timeserials tracking, indicating they are core to the synchronization mechanism
- The PR's objective of implementing lexico timeserials and adding siteCode field in StateMessages confirms this is an intentional design change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for StateMessage type definition and its usages to verify if serial and siteCode are required fields
ast-grep --pattern 'interface StateMessage {
$$$
}'
# Search for StateMessage construction/usage to verify field initialization
rg -A 5 'new StateMessage|StateMessage\('
Length of output: 565
Script:
#!/bin/bash
# Search for StateMessage class/interface definition
ast-grep --pattern 'class StateMessage {
$$$
}'
# Search for serial and siteCode property definitions/assignments
rg -A 3 'serial.*=|siteCode.*=' src/plugins/liveobjects/
# Search for files importing StateMessage to analyze usage
rg -l "import.*StateMessage"
Length of output: 25542
src/plugins/liveobjects/livemap.ts (4)
133-135: Potential risk with non-null assertions
The use of non-null assertions msg.serial! and msg.siteCode! assumes these properties are always defined in StateMessage. If they can be undefined, this may lead to runtime exceptions.
Line range hint 371-419: LGTM! Well-implemented map operation handlers
The _applyMapSet and _applyMapRemove methods are well-implemented with:
- Consistent error handling and logging
- Proper CRDT semantics
- Clear state transitions
Also applies to: 421-452
453-482: LGTM! Well-documented CRDT rules implementation
The _canApplyOperationFollowingCRDTRules method is excellently implemented with:
- Clear documentation of the CRDT semantics
- Comprehensive handling of edge cases
- Proper lexicographical comparison
36-36: Verify the impact of MapEntry interface changes
The type change from Timeserial to string | undefined is a breaking change that might affect consumers of the MapEntry interface.
✅ Verification successful
The type change from Timeserial to string | undefined is consistent with the codebase
The search results confirm that the change from Timeserial to string | undefined is part of a broader transition to string-based timeserials throughout the codebase:
- All timeserial comparisons in the codebase (
livemap.ts,livecounter.ts,liveobject.ts) are already using string operations - Test files show timeserials being used as strings via
lexicoTimeserialfunction - No remaining usage of
Timeserialtype was found in type declarations - The change aligns with the existing implementation in
statemessage.tswhich uses string format for timeserials
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of MapEntry interface
rg -l "MapEntry" | xargs rg "timeserial\s*:\s*Timeserial"
# Search for any remaining Timeserial type usage
rg "Timeserial[^a-zA-Z]"
Length of output: 16401
test/realtime/live_objects.test.js (4)
34-40: LGTM! Clean refactor of the test scenario execution.
The changes to forScenarios improve code readability by using modern async/await patterns and simplifying the test execution flow.
686-691: Well-structured test cases for timeserial ordering.
The test cases effectively verify the lexicographical ordering behavior with different site codes and timestamps. The comments clearly explain the expected behavior for each case.
1426-1437: Comprehensive test coverage for materialized entries.
The test cases thoroughly verify the interaction between site timeserials and materialized entries, covering various edge cases and timestamp combinations.
2078-2117: LGTM! Well-organized subscription test setup.
The subscription test scenarios are well-structured with proper setup and cleanup. The helper objects are correctly initialized with the new timeserial format.
src/plugins/liveobjects/livemap.ts
Outdated
| * Returns true if the origin timeserials of the given operation and entry indicate that | ||
| * the operation should be applied to the entry, following the CRDT semantics of this LiveMap. | ||
| */ | ||
| private _canApplyOperationFollowingCRDTRules( |
There was a problem hiding this comment.
Would a better name for this be _canApplyMapOperation?
There was a problem hiding this comment.
yes! I was struggling with naming this one 😅 , and just saying Map is better for sure, thanks!
VeskeR
force-pushed
the
DTP-1078/lexico-timeserials
branch
from
430573e
to
2f41e15
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1926/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1926/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/realtime/live_objects.test.js (2)
44-56: Well documented timeserial implementation, consider adding input validationThe function effectively generates lexicographically sortable timeserials with good documentation. Consider these improvements:
- Add input validation for numeric parameters
- Extract padding lengths as named constants
- Add JSDoc documentation for better IDE support
+ const TIMESTAMP_PADDING = 14; + const COUNTER_PADDING = 3; + const INDEX_PADDING = 3; + + /** + * Generates a lexicographically sortable timeserial string + * @param {string} seriesId - The series identifier + * @param {number} timestamp - The timestamp value + * @param {number} counter - The counter value + * @param {number} [index] - Optional index value + * @returns {string} Formatted timeserial string + * @throws {Error} If numeric parameters are invalid + */ function lexicoTimeserial(seriesId, timestamp, counter, index) { + if (!Number.isInteger(timestamp) || timestamp < 0) { + throw new Error('timestamp must be a non-negative integer'); + } + if (!Number.isInteger(counter) || counter < 0) { + throw new Error('counter must be a non-negative integer'); + } + if (index != null && (!Number.isInteger(index) || index < 0)) { + throw new Error('index must be a non-negative integer'); + } + - const paddedTimestamp = timestamp.toString().padStart(14, '0'); - const paddedCounter = counter.toString().padStart(3, '0'); - const paddedIndex = index != null ? index.toString().padStart(3, '0') : undefined; + const paddedTimestamp = timestamp.toString().padStart(TIMESTAMP_PADDING, '0'); + const paddedCounter = counter.toString().padStart(COUNTER_PADDING, '0'); + const paddedIndex = index != null ? index.toString().padStart(INDEX_PADDING, '0') : undefined;
684-689: Consider adding test cases for timeserial edge casesThe test scenarios thoroughly cover the happy path for timeserial ordering, but consider adding test cases for:
- Invalid inputs to lexicoTimeserial (negative numbers, non-integer values)
- Boundary conditions (MAX_SAFE_INTEGER for timestamp/counter)
- Lexicographical ordering edge cases (e.g., different length seriesIds)
- Documentation explaining the significance of timeserial ordering in the system
// Example additional test cases: { description: 'validates lexicoTimeserial inputs', action: async (ctx) => { expect(() => lexicoTimeserial('test', -1, 0)).to.throw('timestamp must be a non-negative integer'); expect(() => lexicoTimeserial('test', 1.5, 0)).to.throw('timestamp must be a non-negative integer'); // ... more validation tests } }, { description: 'maintains lexicographical ordering with varying seriesId lengths', action: async (ctx) => { const t1 = lexicoTimeserial('a', 1, 0); const t2 = lexicoTimeserial('aa', 1, 0); expect(t1 < t2).to.be.true; // ... more ordering tests } }Also applies to: 863-869, 990-996, 1113-1118, 1229-1235, 1461-1471, 1482-1488
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
scripts/moduleReport.ts(0 hunks)src/plugins/liveobjects/livecounter.ts(2 hunks)src/plugins/liveobjects/livemap.ts(8 hunks)src/plugins/liveobjects/liveobject.ts(2 hunks)src/plugins/liveobjects/statemessage.ts(3 hunks)src/plugins/liveobjects/timeserial.ts(0 hunks)test/common/modules/live_objects_helper.js(1 hunks)test/realtime/live_objects.test.js(24 hunks)
💤 Files with no reviewable changes (2)
- scripts/moduleReport.ts
- src/plugins/liveobjects/timeserial.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/plugins/liveobjects/livecounter.ts
- src/plugins/liveobjects/liveobject.ts
- src/plugins/liveobjects/statemessage.ts
- test/common/modules/live_objects_helper.js
🔇 Additional comments (8)
src/plugins/liveobjects/livemap.ts (7)
133-135: Potential risk with non-null assertions on 'msg.serial' and 'msg.siteCode'
The use of non-null assertions msg.serial! and msg.siteCode! assumes these properties are always defined in StateMessage. If they can be undefined, this may lead to runtime exceptions.
236-236: Add defensive programming for siteTimeserials assignment
Direct assignment of stateObject.siteTimeserials without validation could lead to issues if the value is undefined.
Line range hint 371-482: LGTM! Well-structured implementation of CRDT rules
The implementation correctly handles:
- Undefined timeserials
- Lexicographical comparison
- Edge cases
457-457: Consider renaming to _canApplyMapOperation
As suggested in a previous review, would _canApplyMapOperation be a better name for this method?
499-499: LGTM! Consistent with interface changes
The timeserial assignment aligns with the updated MapEntry interface.
146-146:
Add defensive programming for siteTimeserials indexing
Direct indexing with a non-null assertion this._siteTimeserials[opSiteCode!] could lead to runtime errors if opSiteCode is undefined.
- this._siteTimeserials[opSiteCode] = opOriginTimeserial;
+ if (opSiteCode) {
+ this._siteTimeserials[opSiteCode] = opOriginTimeserial;
+ }Likely invalid or redundant comment.
36-36: Verify the impact of breaking interface change
The type change from Timeserial to string | undefined is a breaking change that might affect consumers of the MapEntry interface.
✅ Verification successful
Let me gather more information about the usage of timeserial to understand if there are any type-dependent operations.
Let me try a different pattern to search for the interface and its usage.
Breaking interface change is safe to proceed
The type change from Timeserial to string | undefined is safe because:
- The
MapEntryinterface is only used internally within thelivemap.tsfile - All usages of
timeserialhave been updated to handle the new type:- Constructor creates entries with string timeserials
- CRDT comparison logic in
_canApplyOperationFollowingCRDTRulesacceptsstring | undefined - All timeserial comparisons use string operations
- External references in
filteredsubscriptions.tsandably.d.tsonly access timeserial through the extras object, not through the MapEntry interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of MapEntry interface and timeserial property access
rg -l "MapEntry" | xargs ast-grep --pattern 'interface $_ extends MapEntry'
rg "\.timeserial\b" --type ts
Length of output: 1449
Script:
#!/bin/bash
# Search for timeserial comparisons and operations
ast-grep --pattern 'timeserial $_ $$$'
# Search for MapEntry interface definition and its consumers
ast-grep --pattern 'interface MapEntry'
ast-grep --pattern 'class $_ implements $_MapEntry'
Length of output: 136
Script:
#!/bin/bash
# Search for MapEntry interface definition
ast-grep --pattern 'interface MapEntry {
$$$
}'
# Search for type references to MapEntry
rg ":\s*MapEntry[\[\]\s{},]" --type ts -A 2
# Search for timeserial comparisons
rg "_canApplyOperationFollowingCRDTRules" --type ts -A 5
Length of output: 2799
test/realtime/live_objects.test.js (1)
34-40: Clean and efficient implementation of forScenarios!
The function effectively handles test scenarios with proper async/await usage and test context management.
VeskeR
force-pushed
the
DTP-1078/lexico-timeserials
branch
from
2f41e15
to
f323c74
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1926/features
github-actions
bot
temporarily deployed
to
staging/pull/1926/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/plugins/liveobjects/livemap.ts (1)
454-480: LGTM: Well-implemented CRDT rules with comprehensive edge case handlingThe lexicographical comparison implementation is thorough and handles all edge cases correctly. Consider adding a brief example in the comments to illustrate the lexicographical ordering.
/** * Returns true if the origin timeserials of the given operation and entry indicate that * the operation should be applied to the entry, following the CRDT semantics of this LiveMap. + * + * Example: + * - "2024-01-01T12:00:00.000Z-site1" > "2024-01-01T11:59:59.999Z-site2" (true) + * - "2024-01-01T12:00:00.000Z-site1" > undefined (true) + * - undefined > "2024-01-01T12:00:00.000Z-site1" (false) */test/realtime/live_objects.test.js (2)
44-56: Add input validation tolexicoTimeserialfunctionWhile the function is well-documented and follows a clear format, it lacks input validation which could lead to runtime errors or incorrect timeserials.
Consider adding these validations:
function lexicoTimeserial(seriesId, timestamp, counter, index) { + if (typeof seriesId !== 'string' || seriesId.length === 0) { + throw new Error('seriesId must be a non-empty string'); + } + if (!Number.isInteger(timestamp) || timestamp < 0) { + throw new Error('timestamp must be a non-negative integer'); + } + if (!Number.isInteger(counter) || counter < 0) { + throw new Error('counter must be a non-negative integer'); + } + if (index !== undefined && (!Number.isInteger(index) || index < 0)) { + throw new Error('index must be a non-negative integer when provided'); + } const paddedTimestamp = timestamp.toString().padStart(14, '0'); const paddedCounter = counter.toString().padStart(3, '0'); const paddedIndex = index != null ? index.toString().padStart(3, '0') : undefined;
1254-1269: Enhance error handling in test scenario executionWhile the test implementation is solid, it could benefit from better error handling and cleanup.
Consider adding try-catch and cleanup:
/** @nospec */ forScenarios(applyOperationsScenarios, async function (helper, scenario) { const liveObjectsHelper = new LiveObjectsHelper(helper); const client = RealtimeWithLiveObjects(helper); + try { await helper.monitorConnectionThenCloseAndFinish(async () => { const channelName = scenario.description; const channel = client.channels.get(channelName, channelOptionsWithLiveObjects()); const liveObjects = channel.liveObjects; await channel.attach(); const root = await liveObjects.getRoot(); await scenario.action({ root, liveObjectsHelper, channelName, channel }); }, client); + } catch (error) { + console.error(`Failed to execute scenario: ${scenario.description}`, error); + throw error; + } finally { + // Cleanup any resources if needed + await liveObjectsHelper.cleanup?.(); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
scripts/moduleReport.ts(0 hunks)src/plugins/liveobjects/livecounter.ts(2 hunks)src/plugins/liveobjects/livemap.ts(8 hunks)src/plugins/liveobjects/liveobject.ts(2 hunks)src/plugins/liveobjects/statemessage.ts(3 hunks)src/plugins/liveobjects/timeserial.ts(0 hunks)test/common/modules/live_objects_helper.js(1 hunks)test/realtime/live_objects.test.js(24 hunks)
💤 Files with no reviewable changes (2)
- scripts/moduleReport.ts
- src/plugins/liveobjects/timeserial.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/plugins/liveobjects/livecounter.ts
- src/plugins/liveobjects/statemessage.ts
- test/common/modules/live_objects_helper.js
🔇 Additional comments (7)
src/plugins/liveobjects/liveobject.ts (2)
40-40: LGTM: Type change aligns with making timeserials opaque
The change from Record<string, Timeserial> to Record<string, string> aligns with the PR objective of making timeserials opaque to clients. This simplification improves encapsulation by removing direct access to Timeserial objects.
108-118: Verify error handling in upstream code
The method now includes proper validation for both timeserial and siteCode parameters, which is good. However, we should ensure that all callers are prepared to handle these potential errors.
Let's verify the error handling in all callers:
Additionally, a few suggestions to improve the implementation:
protected _canApplyOperation(opOriginTimeserial: string | undefined, opSiteCode: string | undefined): boolean {
- if (!opOriginTimeserial) {
- throw new this._client.ErrorInfo(`Invalid timeserial: ${opOriginTimeserial}`, 50000, 500);
+ if (!opOriginTimeserial?.trim()) {
+ throw new this._client.ErrorInfo('Timeserial cannot be empty or undefined', 50000, 500);
}
- if (!opSiteCode) {
- throw new this._client.ErrorInfo(`Invalid site code: ${opSiteCode}`, 50000, 500);
+ if (!opSiteCode?.trim()) {
+ throw new this._client.ErrorInfo('Site code cannot be empty or undefined', 50000, 500);
}
const siteTimeserial = this._siteTimeserials[opSiteCode];
return !siteTimeserial || opOriginTimeserial > siteTimeserial;
}The changes:
- Add trim() check to prevent whitespace-only strings
- Improve error messages to be more descriptive
✅ Verification successful
The previous pattern might have failed because _canApplyOperation is a protected method and might be called within other methods of the same class. Let's gather more context about its usage.
I want to see more context around these calls to understand if there's error handling at a higher level, since we don't see direct try-catch blocks around the _canApplyOperation calls.
I want to understand the higher-level context of where these operations are being called from and how errors are generally handled in this codebase, since we see ErrorInfo being used.
Error handling is properly implemented throughout the codebase
The validation in _canApplyOperation is part of a well-designed error handling system where:
- All errors are handled through a consistent
ErrorInfomechanism with standard error codes (50000) and HTTP status codes (500) - The method is called within
applyOperation, which is the main entry point for state changes - Error handling exists at multiple layers:
- Input validation in
_canApplyOperation - Object ID validation in the caller
- Operation-specific validation in various methods
- Input validation in
- The errors are propagated up to
LiveObjectsPoolwhich is the top-level orchestrator
The lack of explicit try-catch blocks around _canApplyOperation calls is intentional, as these errors should propagate up to be handled by the client's error handling infrastructure, which is evident from the consistent use of this._client.ErrorInfo throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to _canApplyOperation and verify try-catch blocks
ast-grep --pattern 'try {
$$$
_canApplyOperation($_, $_)
$$$
} catch'
Length of output: 77
Script:
#!/bin/bash
# Let's first find all direct calls to _canApplyOperation
rg "_canApplyOperation\(" -A 2 -B 2
# Also search for the class definition to understand the context
ast-grep --pattern 'class $_ {
$$$
_canApplyOperation($_, $_) {
$$$
}
$$$
}'
Length of output: 1503
Script:
#!/bin/bash
# Let's check the context of how these calls are handled in LiveCounter and LiveMap
rg "_canApplyOperation" -B 10 -A 10 src/plugins/liveobjects/livecounter.ts src/plugins/liveobjects/livemap.ts
# Also check if there's any error handling higher up in the call stack
rg "try.*catch" -B 5 -A 5 src/plugins/liveobjects/livecounter.ts src/plugins/liveobjects/livemap.ts
Length of output: 3636
Script:
#!/bin/bash
# Let's check where these methods are called from to understand the error handling at a higher level
rg "applyOperation" -B 5 -A 5 src/plugins/liveobjects/
# Also check for any error handlers or error processing in the client
rg "ErrorInfo" -B 5 -A 5 src/plugins/liveobjects/
Length of output: 21251
src/plugins/liveobjects/livemap.ts (5)
36-36: LGTM: Type change aligns with lexico timeserials implementation
The change from Timeserial to string | undefined supports lexicographical comparison and handles cases where timeserial isn't set.
133-135: Potential risk with non-null assertions
The use of non-null assertions msg.serial! and msg.siteCode! assumes these properties are always defined in StateMessage. If they can be undefined, this may lead to runtime exceptions.
236-237: LGTM: Safe initialization of siteTimeserials
Good use of nullish coalescing to ensure siteTimeserials is always initialized as an object.
372-382: LGTM: Consistent handling of string timeserials
The methods have been properly updated to handle string timeserials with appropriate null checks and logging.
Also applies to: 422-433
497-497: LGTM: Consistent type usage
The timeserial assignment is correctly updated to match the new string type.
VeskeR
force-pushed
the
DTP-963/liveobjects-customer-typings
branch
2 times, most recently
from
f921ce9
to
a1e29ff
Compare
Resolves DTP-1078
VeskeR
force-pushed
the
DTP-1078/lexico-timeserials
branch
from
f323c74
to
6e3e3e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/plugins/liveobjects/liveobject.ts (1)
108-118: Consider adding JSDoc documentation.The method would benefit from JSDoc documentation explaining:
- The purpose of the validation
- The expected format of timeserials
- The meaning of the error codes
- The comparison logic for determining if an operation can be applied
+ /** + * Determines if an operation can be applied based on its origin timeserial and site code. + * @param opOriginTimeserial - The operation's origin timeserial in lexicographical format + * @param opSiteCode - The operation's site code + * @throws {ErrorInfo} If the timeserial or site code is invalid + * @returns {boolean} True if the operation can be applied + * @internal + */ protected _canApplyOperation(opOriginTimeserial: string | undefined, opSiteCode: string | undefined): boolean {src/plugins/liveobjects/livemap.ts (2)
454-480: Add unit tests for _canApplyMapOperation edge casesThe implementation looks good and handles all edge cases well. However, it would be beneficial to add unit tests to verify the behavior of:
- Both timeserials undefined/empty
- Only entry timeserial undefined
- Only op timeserial undefined
- Lexicographical comparison cases
Would you like me to help generate the unit test cases?
497-497: Consider validating timeserial formatWhile the type change is correct, consider adding validation to ensure the timeserial string follows the expected format when creating a new MapEntry.
- timeserial: entry.timeserial, + timeserial: validateTimeserial(entry.timeserial),test/realtime/live_objects.test.js (1)
44-56: Well-structured implementation of lexicographical timeserial format!The implementation correctly creates lexicographically sortable timeserials with proper padding and clear format documentation. Consider adding JSDoc comments to document the parameters and return type.
+/** + * Generates a lexicographically sortable timeserial string. + * @param {string} seriesId - The unique identifier for the series + * @param {number} timestamp - The timestamp value + * @param {number} counter - The counter value + * @param {number} [index] - Optional index value + * @returns {string} Formatted timeserial string (e.g., "01726585978590-001@abcdefghij:001") + */ function lexicoTimeserial(seriesId, timestamp, counter, index) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
scripts/moduleReport.ts(0 hunks)src/plugins/liveobjects/livecounter.ts(2 hunks)src/plugins/liveobjects/livemap.ts(8 hunks)src/plugins/liveobjects/liveobject.ts(2 hunks)src/plugins/liveobjects/statemessage.ts(3 hunks)src/plugins/liveobjects/timeserial.ts(0 hunks)test/common/modules/live_objects_helper.js(1 hunks)test/realtime/live_objects.test.js(24 hunks)
💤 Files with no reviewable changes (2)
- scripts/moduleReport.ts
- src/plugins/liveobjects/timeserial.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/plugins/liveobjects/livecounter.ts
- src/plugins/liveobjects/statemessage.ts
- test/common/modules/live_objects_helper.js
🧰 Additional context used
📓 Learnings (1)
test/realtime/live_objects.test.js (1)
Learnt from: VeskeR
PR: ably/ably-js#1926
File: test/realtime/live_objects.test.js:2078-2117
Timestamp: 2024-11-28T02:13:58.582Z
Learning: In this project, Mocha tests have an inherent default timeout of 60 seconds, so adding explicit timeouts in the test code is unnecessary.
🔇 Additional comments (8)
src/plugins/liveobjects/liveobject.ts (2)
40-40: LGTM! Good simplification of timeserial representation.
The change from Timeserial objects to string representation aligns well with the PR objectives and simplifies the implementation by enabling direct string comparison of timeserials.
108-115: Verify the error code standardization.
The implementation looks good, but let's verify if error code 50000 follows the standard error codes used across the codebase.
✅ Verification successful
Error code 50000 is consistently used across the codebase for client-side validation errors
The error code 50000 is standardized across the codebase and is used in similar validation scenarios:
- Input validation failures (e.g., invalid object IDs, invalid parameters)
- Client-side implementation errors
- Transport and connectivity issues
- All these errors are paired with HTTP status code 500
The implementation in _canApplyOperation follows this established pattern correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of error code 50000 to verify consistency
rg -A 2 "50000" --type ts
Length of output: 8083
src/plugins/liveobjects/livemap.ts (4)
36-36: LGTM: Type change aligns with lexico timeserials requirement
The change from Timeserial object to string | undefined correctly implements the lexico timeserials requirement while maintaining type safety for cases where timeserial isn't set.
133-135: Potential risk with non-null assertions and direct assignment
The use of non-null assertions msg.serial! and msg.siteCode! assumes these properties are always defined. Additionally, direct assignment to _siteTimeserials without validation could lead to issues.
Also applies to: 146-146
236-237: LGTM: Good defensive programming with nullish coalescing
The use of nullish coalescing operator with an empty map default is a good practice, ensuring that future operations can always be applied to the object.
372-382: LGTM: Consistent type changes and improved logging
The changes maintain type consistency with the new string-based timeserials while providing clear logging for debugging operation application decisions.
Also applies to: 422-433
test/realtime/live_objects.test.js (2)
1461-1471: Excellent test coverage for timeserial ordering!
The test cases thoroughly cover various scenarios for timeserial ordering with clear comments explaining each case. The tests properly verify:
- Earlier vs later site CGO (Causal Global Order)
- Same site CGO handling
- Different site interactions
- Edge cases and impossible scenarios
Also applies to: 1482-1488
2078-2117: Well-structured subscription test scenarios!
The subscription test scenarios are comprehensive and properly structured. As per the project's convention, these tests correctly rely on Mocha's default 60-second timeout rather than implementing explicit timeouts.
Resolves DTP-1078
Summary by CodeRabbit
Release Notes
New Features
siteCodeproperty in theStateMessageclass for enhanced message tracking.lexicoTimeserialfor consistent timestamp formatting in state operation messages.Improvements
timeserialproperty in theStateMapEntryinterface.Bug Fixes
LiveObjectclass for timeserial comparisons.Tests
lexicoTimeserialfunction and includesiteCodein state operation messages, improving test clarity and maintainability.Chores
timeserial.tsfile from the allowed files for the LiveObjects plugin, refining bundle size management.