feature: add subscription.find #2735
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughAdds LiveQuery query execution and result delivery: introduces QUERY op and RESULT event in LiveQueryClient, passes client into LiveQuerySubscription, implements subscription.find() to send a query over the WebSocket, and handles RESULT messages by emitting mapped ParseObject results. Updates typings and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Application
participant Sub as LiveQuerySubscription
participant LQC as LiveQueryClient
participant WS as WebSocket
participant PS as Parse Server
App->>Sub: subscription.find()
alt client attached
Sub->>LQC: await connectPromise
Sub->>WS: send QUERY {requestId: sub.id, query,...}
WS->>PS: QUERY
PS-->>WS: RESULT {requestId, results:[...]}
WS-->>LQC: message(OP_EVENTS.RESULT)
LQC->>Sub: emit('result', [ParseObject,...])
Sub-->>App: 'result' event with results
else no client
Sub-->>App: no-op (find() gated by client)
end
note over LQC,Sub: RESULT handling maps payload to ParseObject before emission
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #2735 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 63 63
Lines 6185 6195 +10
Branches 1456 1459 +3
=========================================
+ Hits 6185 6195 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
types/LiveQuerySubscription.d.ts (2)
92-92: Consider stronger typing for theclientfield.The
clientfield is currently typed asany, which loses type safety. While a directLiveQueryClienttype would create a circular dependency, consider defining a minimal interface or usingimport typeto preserve type information without runtime circularity.Example approach:
+import type LiveQueryClient from './LiveQueryClient'; + declare class LiveQuerySubscription { id: string | number; query: ParseQuery; sessionToken?: string; subscribePromise: any; unsubscribePromise: any; subscribed: boolean; - client: any; + client?: LiveQueryClient; emitter: EventEmitter; on: EventEmitter['on']; emit: EventEmitter['emit']; - constructor(id: string | number, query: ParseQuery, sessionToken?: string, client?: any); + constructor(id: string | number, query: ParseQuery, sessionToken?: string, client?: LiveQueryClient);Also applies to: 96-96
103-107: Enhance JSDoc to document preconditions.The documentation for
find()should clarify what happens whenclientis not set, and whether callers need to wait for connection before calling./** * Execute a query on this subscription. * The results will be delivered via the 'result' event. + * Note: This method requires a client to be set and will only send the query + * after the client's connection promise resolves. */ find(): void;src/__tests__/LiveQueryClient-test.js (1)
1155-1184: LGTM! Consider adding edge case test.The test correctly verifies that
subscription.find()sends a properly formatted query message with the correctopandrequestId. The test appropriately waits for the connection promise before callingfind().Consider adding a test for the edge case where
find()is called whenclientis not set or before connection is established:it('find() does nothing when client is not set', () => { const subscription = new LiveQuerySubscription(1, new ParseQuery('Test')); expect(() => subscription.find()).not.toThrow(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/LiveQueryClient.ts(5 hunks)src/LiveQuerySubscription.ts(2 hunks)src/__tests__/LiveQueryClient-test.js(3 hunks)types/LiveQuerySubscription.d.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/LiveQueryClient-test.js (2)
src/__tests__/ParseQuery-test.js (2)
LiveQuerySubscription(35-35)ParseQuery(34-34)src/__tests__/ParseLiveQuery-test.js (2)
LiveQuerySubscription(16-16)ParseQuery(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (Node 20, 20.15.1)
- GitHub Check: build (Node 18, 18.20.4)
🔇 Additional comments (8)
src/LiveQueryClient.ts (4)
24-24: LGTM!The addition of the
QUERYoperation type follows the existing pattern and is correctly placed.
38-38: LGTM!The
RESULTevent type addition is correctly placed and follows the existing pattern.
58-58: LGTM!The
RESULTemitter type addition is correctly placed and follows the existing pattern.
218-218: LGTM!Passing the client instance to the subscription constructor enables
subscription.find()to send messages over the WebSocket. This is essential for the new query execution feature.src/__tests__/LiveQueryClient-test.js (3)
1-1: LGTM!Unmocking
LiveQuerySubscriptionand importing it directly is necessary to test the newfind()method and verify prototype behavior. This follows the same pattern used in other test files.Also applies to: 41-41
1096-1132: LGTM!The test comprehensively verifies that the RESULT event handler correctly maps multiple JSON objects to ParseObject instances and emits them via the subscription. Test structure follows existing patterns.
1134-1136: LGTM!Both tests appropriately verify that
find()exists on theLiveQuerySubscriptionprototype and on subscription instances. The instance test also confirms the correct type.Also applies to: 1138-1153
src/LiveQuerySubscription.ts (1)
95-95: LGTM!The client field and constructor parameter additions are correctly implemented. The client reference is properly stored for use by the
find()method.Also applies to: 103-109
| case OP_EVENTS.RESULT: { | ||
| if (subscription) { | ||
| const objects = data.results.map(json => ParseObject.fromJSON(json, false)); | ||
| subscription.emit(SUBSCRIPTION_EMMITER_TYPES.RESULT, objects); | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
Add null safety check for data.results.
The RESULT handler assumes data.results is always an array. If the server sends a malformed message without the results field, data.results.map() will throw a runtime error.
Consider adding a defensive check:
case OP_EVENTS.RESULT: {
if (subscription) {
+ if (!data.results || !Array.isArray(data.results)) {
+ break;
+ }
const objects = data.results.map(json => ParseObject.fromJSON(json, false));
subscription.emit(SUBSCRIPTION_EMMITER_TYPES.RESULT, objects);
}
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case OP_EVENTS.RESULT: { | |
| if (subscription) { | |
| const objects = data.results.map(json => ParseObject.fromJSON(json, false)); | |
| subscription.emit(SUBSCRIPTION_EMMITER_TYPES.RESULT, objects); | |
| } | |
| break; | |
| } | |
| case OP_EVENTS.RESULT: { | |
| if (subscription) { | |
| if (!data.results || !Array.isArray(data.results)) { | |
| break; | |
| } | |
| const objects = data.results.map(json => ParseObject.fromJSON(json, false)); | |
| subscription.emit(SUBSCRIPTION_EMMITER_TYPES.RESULT, objects); | |
| } | |
| break; | |
| } |
🤖 Prompt for AI Agents
In src/LiveQueryClient.ts around lines 431 to 437, the RESULT branch assumes
data.results is always present and an array; add a defensive null/type check
before calling .map to avoid runtime errors when the server sends malformed
messages. Update the handler to verify data.results is an array (or provide an
empty array fallback) and only map when valid, then emit the subscription result
using the safe array; also consider logging or ignoring invalid payloads so
malformed messages don't throw.
| find() { | ||
| if (this.client) { | ||
| this.client.connectPromise.then(() => { | ||
| this.client.socket.send(JSON.stringify({ | ||
| op: 'query', | ||
| requestId: this.id, | ||
| })); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling and consider user feedback for missing client.
The find() method has several potential issues:
- Silent failure when client is missing: If
this.clientis not set, the method does nothing without notifying the caller. This could lead to confusion. - No error handling: If
connectPromiserejects orsocket.send()throws, errors are swallowed. - No socket existence check: If
this.client.socketis undefined, accessingsendwill throw.
Consider these improvements:
find() {
if (this.client) {
this.client.connectPromise.then(() => {
- this.client.socket.send(JSON.stringify({
- op: 'query',
- requestId: this.id,
- }));
+ if (this.client.socket) {
+ this.client.socket.send(JSON.stringify({
+ op: 'query',
+ requestId: this.id,
+ }));
+ }
+ }).catch(error => {
+ this.emit('error', error);
});
+ } else {
+ this.emit('error', new Error('Cannot execute find: client not available'));
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| find() { | |
| if (this.client) { | |
| this.client.connectPromise.then(() => { | |
| this.client.socket.send(JSON.stringify({ | |
| op: 'query', | |
| requestId: this.id, | |
| })); | |
| }); | |
| } | |
| } | |
| find() { | |
| if (this.client) { | |
| this.client.connectPromise.then(() => { | |
| if (this.client.socket) { | |
| this.client.socket.send(JSON.stringify({ | |
| op: 'query', | |
| requestId: this.id, | |
| })); | |
| } | |
| }).catch(error => { | |
| this.emit('error', error); | |
| }); | |
| } else { | |
| this.emit('error', new Error('Cannot execute find: client not available')); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/LiveQuerySubscription.ts around lines 141 to 150, the find() method
silently no-ops when this.client is missing, swallows connection/send errors,
and assumes this.client.socket exists; update it to explicitly handle the
missing-client case (either throw an Error or return/reject a Promise with a
clear message so callers get feedback), call
this.client.connectPromise.then(...).catch(...) to handle connection rejection,
and inside the then block check that this.client.socket is present before
calling socket.send (wrap send in try/catch and surface/log the error or reject
the returned Promise). Ensure the method returns a Promise (or consistently
signals failures) so callers can observe errors and add any contextual error
logging or event emission as appropriate.
2 participants
Pull Request
Issue
parse-community/parse-server#9864
Closes: #2114
Approach
Tasks
Summary by CodeRabbit
New Features
Tests
Documentation