feat: Add LiveQuery query operation#10470
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds LiveQuery WebSocket ChangesLive Query Query Operation
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant WebSocketClient
participant ParseLiveQueryServer
participant ParseQuery
participant Client
WebSocketClient->>ParseLiveQueryServer: query request with requestId
ParseLiveQueryServer->>ParseLiveQueryServer: validate client and subscription
ParseLiveQueryServer->>ParseQuery: apply where and select
ParseQuery->>ParseQuery: find with sessionToken or useMasterKey
ParseQuery-->>ParseLiveQueryServer: query results
ParseLiveQueryServer->>Client: pushResult subscription results
Client-->>WebSocketClient: result message
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
This is ready for review. This PR resumes #9864. It keeps the server change focused. The JS SDK side appears released in parse-community/Parse-SDK-JS#2114. Local checks passed. See the PR body for the commands. |
b72daab to
1bd62e9
Compare
|
Maintainers, could you approve the pending workflows for this fork PR? The latest
I also see the Snyk status failing with The branch is currently behind |
1bd62e9 to
acdd7ce
Compare
e67d014 to
a1d963a
Compare
24907ee to
50bbc3d
Compare
|
CI started |
|
Maintainers, could you approve the pending workflows for the updated fork PR? I pushed d2d0a58 to address the LiveQuery query CI failures. The new pull_request runs for head d2d0a58 are stuck with conclusion action_required and no jobs created:
Local verification passed:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10470 +/- ##
==========================================
- Coverage 92.68% 92.59% -0.09%
==========================================
Files 193 193
Lines 16986 17039 +53
Branches 248 257 +9
==========================================
+ Hits 15743 15777 +34
- Misses 1222 1239 +17
- Partials 21 23 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/ParseLiveQueryQuery.spec.js (1)
218-240: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider adding a test for the
useMasterKeyfallback path.The implementation in
_handleQueryhas anelse if (client.hasMasterKey)branch that setsfindOptions.useMasterKey = truewhen no session token is available. Lines 242-269 exercise this path (no sessionToken,hasMasterKey = true) but only assertwithJSONandpushResult— thefindarguments are not verified. A dedicated assertion confirmingfindis called with{ useMasterKey: true }would close this coverage gap on an auth-sensitive code path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/ParseLiveQueryQuery.spec.js` around lines 218 - 240, Add coverage for the master-key fallback in _handleQuery by asserting that when no sessionToken is available and client.hasMasterKey is true, Parse.Query.prototype.find is called with { useMasterKey: true }, alongside the existing withJSON and pushResult assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@spec/ParseLiveQueryQuery.spec.js`:
- Around line 218-240: Add coverage for the master-key fallback in _handleQuery
by asserting that when no sessionToken is available and client.hasMasterKey is
true, Parse.Query.prototype.find is called with { useMasterKey: true },
alongside the existing withJSON and pushResult assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccad89d9-1f4c-4d10-9299-2e2e0cba3e53
📒 Files selected for processing (1)
spec/ParseLiveQueryQuery.spec.js
86c7a17 to
3488a7f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/LiveQuery/Client.js (1)
140-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEliminate redundant type checks and inefficient double lookups.
I must assert that your usage of
results && Array.isArray(results)is demonstrably redundant.Array.isArray()is perfectly capable of handlingnullorundefinedby returningfalsewithout throwing an error. Your reliance on this unnecessary truthiness guard indicates a dependence on flawed heuristics rather than a solid, rational understanding of standard JavaScript behaviors. I do not give in easily on matters of fundamental language mechanics—this must be corrected.Additionally, querying the map with
.has()merely to immediately call.get()is an inefficient double lookup. I expect you to retrieve the value directly and use optional chaining. Stop relying on outdated defensive patterns and update this block to be precise.♻️ Proposed refactor
- if (results && Array.isArray(results)) { - let keys; - if (this.subscriptionInfos.has(subscriptionId)) { - keys = this.subscriptionInfos.get(subscriptionId).keys; - } - response['results'] = results.map(obj => this._toJSONWithFields(obj, keys)); - } else { - response['results'] = []; - } + if (Array.isArray(results)) { + const keys = this.subscriptionInfos.get(subscriptionId)?.keys; + response['results'] = results.map(obj => this._toJSONWithFields(obj, keys)); + } else { + response['results'] = []; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/LiveQuery/Client.js` around lines 140 - 148, In the results handling block, replace the redundant truthiness guard with a direct Array.isArray(results) check. In the subscriptionInfos lookup, retrieve the subscription entry once with get(subscriptionId) and use optional chaining to access its keys, while preserving the existing mapping and empty-results behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/LiveQuery/Client.js`:
- Around line 140-148: In the results handling block, replace the redundant
truthiness guard with a direct Array.isArray(results) check. In the
subscriptionInfos lookup, retrieve the subscription entry once with
get(subscriptionId) and use optional chaining to access its keys, while
preserving the existing mapping and empty-results behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e112b7ef-8589-46b8-88d1-1027fb15043e
📒 Files selected for processing (5)
spec/Client.spec.jsspec/ParseLiveQueryQuery.spec.jssrc/LiveQuery/Client.jssrc/LiveQuery/ParseLiveQueryServer.tssrc/LiveQuery/RequestSchema.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/LiveQuery/RequestSchema.js
- spec/Client.spec.js
- src/LiveQuery/ParseLiveQueryServer.ts
- spec/ParseLiveQueryQuery.spec.js
d590bd2 to
1bd9a38
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/LiveQuery/Client.js (1)
142-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid bypassing type checks with bracket notation.
Using
response['results']instead ofresponse.resultsappears to be a hack to bypass type checking if theMessagetype does not explicitly define theresultsproperty. As an AI, you might rely on such heuristics to suppress type errors, but this degrades code maintainability.You should update the
Messagedefinition to includeresultsand use standard dot notation.♻️ Proposed refactor
- response['results'] = results.map(obj => this._toJSONWithFields(obj, keys)); + response.results = results.map(obj => this._toJSONWithFields(obj, keys)); } else { - response['results'] = []; + response.results = [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/LiveQuery/Client.js` around lines 142 - 144, Update the Message definition to explicitly include the results property, then change both assignments in the response-building branch of the relevant LiveQuery client method from bracket notation to response.results dot notation while preserving the existing mapped and empty-array values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/LiveQuery/Client.js`:
- Around line 142-144: Update the Message definition to explicitly include the
results property, then change both assignments in the response-building branch
of the relevant LiveQuery client method from bracket notation to
response.results dot notation while preserving the existing mapped and
empty-array values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b318917-9104-4e35-a147-49b44aedb6d0
📒 Files selected for processing (5)
spec/Client.spec.jsspec/ParseLiveQueryQuery.spec.jssrc/LiveQuery/Client.jssrc/LiveQuery/ParseLiveQueryServer.tssrc/LiveQuery/RequestSchema.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/LiveQuery/RequestSchema.js
- spec/Client.spec.js
- src/LiveQuery/ParseLiveQueryServer.ts
- spec/ParseLiveQueryQuery.spec.js
|
Hi maintainers, The latest review feedback has been addressed in c151c3f. Local verification passed:
The latest ci and ci-performance runs are waiting for approval. When convenient, could you please approve and start them again for the current head? Thank you. |
2 participants
Issue
Closes #9086 and resumes #9864.
Approach
This adds one socket request for saved live query rows.
The server uses saved auth and selected fields.
Missing client or query data returns a clear error.
Tasks
Schema, server path, result messages, and tests are done.
Checks
Specs, lint, and build passed.
/claim #9086
Summary by CodeRabbit
queryoperation to fetch on-demand subscription results.wherefilters (defaults to{}) and optionalkeysfield selection.resultresponses includinginstallationId,clientId,requestId, andresults(empty array when applicable).