[DTP-948] Surface errors when user has not attached to a channel with state modes #1956
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ast-grep (0.31.1)test/realtime/live_objects.test.jsAn unexpected error occurred while running ast-grep. WalkthroughThis pull request introduces comprehensive error handling and state mode validation across the LiveObjects ecosystem. The changes focus on adding checks to ensure that methods can only be executed when the channel is in the appropriate state mode (subscribe or publish). New methods like Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (4)
✨ Finishing Touches
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/1956/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1956/features
github-actions
bot
temporarily deployed
to
staging/pull/1956/typedoc
![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
🧹 Nitpick comments (6)
src/plugins/liveobjects/livecounter.ts (1)
Line range hint
149-156: Consider extracting the type validation to a shared method.While the state publish mode check and type validation are good, the type check logic is duplicated between increment and decrement methods. Consider extracting it to a shared validation method.
+ private _validateAmount(amount: number, operation: 'increment' | 'decrement'): void { + if (typeof amount !== 'number' || !isFinite(amount)) { + throw new this._client.ErrorInfo(`Counter value ${operation} should be a valid number`, 40013, 400); + } + } async increment(amount: number): Promise<void> { this._liveObjects.throwIfMissingStatePublishMode(); + this._validateAmount(amount, 'increment'); const stateMessage = LiveCounter.createCounterIncMessage(this._liveObjects, this.getObjectId(), amount); return this._liveObjects.publish([stateMessage]); } async decrement(amount: number): Promise<void> { this._liveObjects.throwIfMissingStatePublishMode(); - if (typeof amount !== 'number' || !isFinite(amount)) { - throw new this._client.ErrorInfo('Counter value decrement should be a valid number', 40013, 400); - } + this._validateAmount(amount, 'decrement'); return this.increment(-amount); }🧰 Tools
🪛 Biome (1.9.4)
[error] 152-152: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.(lint/suspicious/noGlobalIsFinite)
src/plugins/liveobjects/liveobjects.ts (1)
387-396: Enhance error messages with more context.While the implementation is correct, the error messages could be more descriptive to help users understand and fix the issue.
private _throwIfMissingChannelMode(expectedMode: 'state_subscribe' | 'state_publish'): void { if (this._channel.modes != null && !this._channel.modes.includes(expectedMode)) { - throw new this._client.ErrorInfo(`"${expectedMode}" channel mode must be set for this operation`, 40160, 400); + throw new this._client.ErrorInfo( + `Channel "${this._channel.name}" requires "${expectedMode}" mode for this operation. Current modes: ${this._channel.modes.join(', ')}`, + 40160, + 400 + ); } if (!this._client.Utils.allToLowerCase(this._channel.channelOptions.modes ?? []).includes(expectedMode)) { - throw new this._client.ErrorInfo(`"${expectedMode}" channel mode must be set for this operation`, 40160, 400); + throw new this._client.ErrorInfo( + `Channel "${this._channel.name}" requires "${expectedMode}" mode in channelOptions for this operation. Current modes: ${(this._channel.channelOptions.modes ?? []).join(', ')}`, + 40160, + 400 + ); } }test/realtime/live_objects.test.js (4)
Line range hint
60-69: LGTM! Consider adding type annotations.The helper function correctly implements async error validation. Consider adding JSDoc type annotations for better code maintainability and IDE support.
-async function expectToThrowAsync(fn, errorStr) { +/** + * Helper to verify that an async function throws an expected error + * @param {Function} fn - Async function that should throw + * @param {string} errorStr - Expected error message substring + * @returns {Promise<void>} + */ +async function expectToThrowAsync(fn, errorStr) {
3476-3527: LGTM! Consider standardizing error messages.The function provides comprehensive validation of state mode error handling. The error messages are consistent within each mode type but could be standardized across both modes.
Consider extracting the error messages into constants to ensure consistency:
+const STATE_MODE_ERRORS = { + SUBSCRIBE: '"state_subscribe" channel mode must be set for this operation', + PUBLISH: '"state_publish" channel mode must be set for this operation' +}; const expectToThrowMissingStateMode = async ({ liveObjects, map, counter }) => { await expectToThrowAsync( async () => liveObjects.getRoot(), - '"state_subscribe" channel mode must be set for this operation', + STATE_MODE_ERRORS.SUBSCRIBE, ); // ... apply similar changes to other error strings
3529-3540: LGTM! Consider unifying error checking approach.The function correctly validates state mode errors in batch context. However, it uses a different error checking approach compared to
expectToThrowMissingStateMode.Consider using a helper function to standardize error checking:
+const expectToThrow = (fn, errorStr) => { + let threw = false; + try { + fn(); + } catch (error) { + threw = true; + expect(error.message).to.have.string(errorStr); + } + expect(threw, 'Expected function to throw an error').to.be.true; +}; const expectToThrowMissingStateModeInBatchContext = ({ ctx, map, counter }) => { - expect(() => ctx.getRoot()).to.throw('"state_subscribe" channel mode must be set for this operation'); + expectToThrow(() => ctx.getRoot(), STATE_MODE_ERRORS.SUBSCRIBE); // ... apply similar changes to other checks
3542-3625: LGTM! Consider adding edge case scenarios.The test scenarios provide good coverage of missing state mode handling. Consider adding tests for edge cases.
Add scenarios for:
- Dynamically changing channel modes during operations
- Race conditions between mode changes and API calls
- Recovery after temporary mode loss
Example scenario:
{ description: 'handles dynamic mode changes during operations', action: async (ctx) => { const { liveObjects, channel, map } = ctx; const setOp = map.set('key', 'value'); channel.modes = []; // Remove modes mid-operation await expectToThrowAsync( async () => setOp, '"state_publish" channel mode must be set for this operation' ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/plugins/liveobjects/batchcontext.ts(1 hunks)src/plugins/liveobjects/batchcontextlivecounter.ts(1 hunks)src/plugins/liveobjects/batchcontextlivemap.ts(2 hunks)src/plugins/liveobjects/livecounter.ts(3 hunks)src/plugins/liveobjects/livemap.ts(4 hunks)src/plugins/liveobjects/liveobject.ts(3 hunks)src/plugins/liveobjects/liveobjects.ts(6 hunks)test/common/modules/private_api_recorder.js(1 hunks)test/realtime/live_objects.test.js(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (21)
src/plugins/liveobjects/batchcontextlivecounter.ts (3)
18-21: LGTM! Correctly validates state_subscribe mode for read operation.The state mode check is properly placed before any other operations, ensuring early validation.
24-28: LGTM! Correctly validates state_publish mode for write operation.The state mode check is properly placed before constructing and queueing the state message.
Line range hint
31-40: LGTM! Correctly validates state_publish mode and maintains existing type safety.The implementation properly:
- Validates state_publish mode first
- Maintains existing type safety check
- Reuses increment() for the actual operation
src/plugins/liveobjects/batchcontextlivemap.ts (4)
Line range hint
15-23: LGTM! Correctly validates state_subscribe mode for read operation.The state mode check is properly placed before accessing the map value and handling LiveObject instances.
26-29: LGTM! Correctly validates state_subscribe mode for read operation.The state mode check is properly placed before accessing the map size.
32-36: LGTM! Correctly validates state_publish mode for write operation.The state mode check is properly placed before constructing and queueing the state message.
Line range hint
39-43: LGTM! Correctly validates state_publish mode for write operation.The state mode check is properly placed before constructing and queueing the state message.
src/plugins/liveobjects/batchcontext.ts (1)
27-30: LGTM! Correctly validates state_subscribe mode for root access.The state mode check is properly placed before accessing the root object.
src/plugins/liveobjects/liveobject.ts (2)
Line range hint
68-77: LGTM! Correctly validates state_subscribe mode for subscription.The state mode check is properly placed before setting up the subscription.
Line range hint
80-89: LGTM! Well-documented decision to skip mode checks for unsubscribe operations.The comments clearly explain why unsubscribe operations can proceed without state mode checks, as their results are independent of channel modes.
Also applies to: 93-95
test/common/modules/private_api_recorder.js (1)
144-144: LGTM!The addition of
'write.channel.channelOptions.modes'to the private API identifiers list correctly tracks the new state mode checks being implemented.src/plugins/liveobjects/livecounter.ts (2)
126-127: LGTM!The state subscribe mode check ensures that the counter value can only be read when the channel is in the correct state.
140-143: LGTM!The state publish mode check ensures that counter increments can only occur when the channel is in the correct state.
src/plugins/liveobjects/liveobjects.ts (4)
52-53: LGTM!The state subscribe mode check ensures that the root object can only be accessed when the channel is in the correct state.
66-67: LGTM!The state publish mode check ensures that batch operations can only be executed when the channel is in the correct state.
89-90: LGTM!The state publish mode check ensures that map creation can only occur when the channel is in the correct state.
115-116: LGTM!The state publish mode check ensures that counter creation can only occur when the channel is in the correct state.
src/plugins/liveobjects/livemap.ts (4)
269-270: LGTM!The state subscribe mode check ensures that map values can only be read when the channel is in the correct state.
308-309: LGTM!The state subscribe mode check ensures that the map size can only be read when the channel is in the correct state.
344-344: LGTM!The state publish mode check ensures that map values can only be set when the channel is in the correct state.
359-359: LGTM!The state publish mode check ensures that map values can only be removed when the channel is in the correct state.
VeskeR
force-pushed
the
DTP-1035/batch-write-api
branch
from
da39bcb to
a58dcd0
Compare
VeskeR
force-pushed
the
DTP-948/error-wrong-state-mode-attach
branch
from
1baa197 to
80855de
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1956/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1956/features
github-actions
bot
temporarily deployed
to
staging/pull/1956/typedoc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/plugins/liveobjects/liveobjects.ts (1)
387-396: Consider improving error message clarity.The error message could be more descriptive by mentioning the operation that requires the mode.
- throw new this._client.ErrorInfo(`"${expectedMode}" channel mode must be set for this operation`, 40160, 400); + throw new this._client.ErrorInfo(`"${expectedMode}" channel mode must be set for ${operation} operation`, 40160, 400);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/plugins/liveobjects/batchcontext.ts(1 hunks)src/plugins/liveobjects/batchcontextlivecounter.ts(1 hunks)src/plugins/liveobjects/batchcontextlivemap.ts(2 hunks)src/plugins/liveobjects/livecounter.ts(3 hunks)src/plugins/liveobjects/livemap.ts(4 hunks)src/plugins/liveobjects/liveobject.ts(3 hunks)src/plugins/liveobjects/liveobjects.ts(6 hunks)test/common/modules/private_api_recorder.js(1 hunks)test/realtime/live_objects.test.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/plugins/liveobjects/batchcontextlivemap.ts
- test/common/modules/private_api_recorder.js
- src/plugins/liveobjects/batchcontext.ts
- src/plugins/liveobjects/liveobject.ts
- src/plugins/liveobjects/batchcontextlivecounter.ts
- src/plugins/liveobjects/livecounter.ts
- src/plugins/liveobjects/livemap.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (5)
src/plugins/liveobjects/liveobjects.ts (2)
244-256: LGTM! Well-structured internal validation methods.The new validation methods provide a clear separation of concerns for checking state_subscribe and state_publish modes.
52-53: LGTM! Comprehensive validation coverage.The state mode validations are correctly integrated at all required entry points:
- getRoot() - state_subscribe
- batch() - state_publish
- createMap() - state_publish
- createCounter() - state_publish
Also applies to: 66-67, 89-90, 115-116
test/realtime/live_objects.test.js (3)
60-60: LGTM! Improved function naming.The rename from
expectRejectedWithtoexpectToThrowAsyncbetter reflects the function's purpose.
3476-3540: LGTM! Comprehensive error validation helpers.The helper functions
expectToThrowMissingStateModeandexpectToThrowMissingStateModeInBatchContextprovide thorough validation coverage for all LiveObjects operations.
3542-3625: LGTM! Well-structured test scenarios.The test scenarios thoroughly cover both attached and detached channel states, ensuring proper error handling when required modes are missing.
VeskeR
force-pushed
the
DTP-1035/batch-write-api
branch
from
a58dcd0 to
1aef8c8
Compare
VeskeR
force-pushed
the
DTP-948/error-wrong-state-mode-attach
branch
from
80855de to
79f3b83
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1956/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1956/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1956/features
There was a problem hiding this comment.
Looks good. One suggestion - since state_subscribe is the minimum mode needed to interact with live objects realtime client, should we throw if state_subscribe is not specified when accessing the liveObjects plugin on the channel? (I think the existing subscribe mode checks are still needed in case the modes are updated via setOptions)
It's not really. We provide
that was my exact train of thought, channel modes can change during interaction with liveobjects if user calls |
VeskeR
force-pushed
the
DTP-1035/batch-write-api
branch
from
1aef8c8 to
45c7863
Compare
VeskeR
force-pushed
the
DTP-948/error-wrong-state-mode-attach
branch
from
79f3b83 to
2b79cfe
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1956/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1956/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1956/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/plugins/liveobjects/liveobjects.ts (1)
399-408: Consider adding error code constants for better maintainability.The error code (40160) and status (400) are hardcoded. Consider extracting these to named constants for better maintainability and reusability.
+ private static readonly ERROR_MISSING_CHANNEL_MODE = { + code: 40160, + statusCode: 400 + }; private _throwIfMissingChannelMode(expectedMode: 'state_subscribe' | 'state_publish'): void { if (this._channel.modes != null && !this._channel.modes.includes(expectedMode)) { - throw new this._client.ErrorInfo(`"${expectedMode}" channel mode must be set for this operation`, 40160, 400); + throw new this._client.ErrorInfo( + `"${expectedMode}" channel mode must be set for this operation`, + LiveObjects.ERROR_MISSING_CHANNEL_MODE.code, + LiveObjects.ERROR_MISSING_CHANNEL_MODE.statusCode + ); } if (!this._client.Utils.allToLowerCase(this._channel.channelOptions.modes ?? []).includes(expectedMode)) { - throw new this._client.ErrorInfo(`"${expectedMode}" channel mode must be set for this operation`, 40160, 400); + throw new this._client.ErrorInfo( + `"${expectedMode}" channel mode must be set for this operation`, + LiveObjects.ERROR_MISSING_CHANNEL_MODE.code, + LiveObjects.ERROR_MISSING_CHANNEL_MODE.statusCode + ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/plugins/liveobjects/batchcontext.ts(1 hunks)src/plugins/liveobjects/batchcontextlivecounter.ts(1 hunks)src/plugins/liveobjects/batchcontextlivemap.ts(2 hunks)src/plugins/liveobjects/livecounter.ts(3 hunks)src/plugins/liveobjects/livemap.ts(4 hunks)src/plugins/liveobjects/liveobject.ts(3 hunks)src/plugins/liveobjects/liveobjects.ts(6 hunks)test/common/modules/private_api_recorder.js(1 hunks)test/realtime/live_objects.test.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- test/common/modules/private_api_recorder.js
- src/plugins/liveobjects/batchcontextlivemap.ts
- src/plugins/liveobjects/liveobject.ts
- src/plugins/liveobjects/batchcontext.ts
- src/plugins/liveobjects/batchcontextlivecounter.ts
- src/plugins/liveobjects/livemap.ts
- src/plugins/liveobjects/livecounter.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (14)
src/plugins/liveobjects/liveobjects.ts (5)
52-53: LGTM! State mode validation added to getRoot method.The state_subscribe mode check is correctly placed at the start of the method before any operations.
66-67: LGTM! State mode validation added to batch method.The state_publish mode check is correctly placed at the start of the method before any operations.
89-90: LGTM! State mode validation added to createMap method.The state_publish mode check is correctly placed at the start of the method before any operations.
121-122: LGTM! State mode validation added to createCounter method.The state_publish mode check is correctly placed at the start of the method before any operations.
256-269: LGTM! New internal methods added for state mode validation.The methods are well-documented with @internal JSDoc tags and provide a clean interface for state mode validation.
test/realtime/live_objects.test.js (9)
2126-2170: LGTM! Comprehensive tests for counter increment validation.Good coverage of various invalid input types including undefined, null, NaN, infinity, strings, BigInt, boolean, Symbol, objects, arrays, and self-reference.
2228-2272: LGTM! Comprehensive tests for counter decrement validation.Good coverage of various invalid input types including undefined, null, NaN, infinity, strings, BigInt, boolean, Symbol, objects, arrays, and self-reference.
2354-2369: LGTM! Comprehensive tests for map set validation.Good coverage of invalid key and value types.
2417-2425: LGTM! Comprehensive tests for map remove validation.Good coverage of invalid key types.
2555-2595: LGTM! Comprehensive tests for counter creation validation.Good coverage of various invalid input types.
2808-2850: LGTM! Comprehensive tests for map creation validation.Good coverage of invalid input types for both the map entries object and individual values.
3818-3869: LGTM! Well-structured helper function for testing missing state mode errors.The helper function provides good coverage of all public API methods that require state mode validation.
3871-3882: LGTM! Well-structured helper function for testing missing state mode errors in batch context.The helper function provides good coverage of all batch context methods that require state mode validation.
3884-3967: LGTM! Comprehensive test scenarios for missing channel modes.Good coverage of both attached and detached states with missing channel modes.
VeskeR
force-pushed
the
DTP-1035/batch-write-api
branch
from
45c7863 to
a28901d
Compare
…te modes - `state_subscribe` mode is required for access/subscribe API - `state_publish` mode is required for create/edit API Resolves DTP-948
VeskeR
force-pushed
the
DTP-948/error-wrong-state-mode-attach
branch
from
2b79cfe to
5d78643
Compare
state_subscribemode is required for access/subscribe APIstate_publishmode is required for create/edit APII think the existing subscribe mode checks are still needed in case the modes are updated via setOptions
Channel modes can change post channel attachment (if user calls
channel.setOptions) at which point user may have already obtained arootand other live object instances. This means that every public live objects API method needs to check for the required modes set on a channel separately.Resolves DTP-948
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores