Conversation
|
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:
WalkthroughThe diff introduces per-subscription queries and shifts subscription management from channel-centric to slot-centric across Android, Apple, Flutter, React Native, and Web templates. RealtimeCallback/RealtimeSubscription types now include a queries field and many channel collections changed to sets. Subscribe APIs were extended to accept queries (and on some platforms an explicit payloadType). Socket/URI construction now aggregates channels across slots and encodes per-slot queries. Backend subscription-id ↔ slot mappings and handling of a "connected" message were added so events are routed by backend subscription IDs. Tests and examples were updated to cover query and no-query subscriptions. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
templates/react-native/src/client.ts.twig (1)
278-285: Remove or document queries parameter—Appwrite Realtime does not support server-side query filtering.Appwrite's Realtime API uses channel-based subscriptions only and does not accept the
queries[]parameter. Database query filtering is available exclusively on REST/list endpoints, not realtime subscriptions. The code aggregates queries from multiple subscriptions and sends them to the realtime WebSocket endpoint (lines 283–285, 377–379, and similar in other SDKs), but these parameters are silently ignored by the server. Remove the query aggregation and cleanup logic, or document it as legacy/planned-future behavior if query filtering is intended to be supported later.templates/apple/Sources/Services/Realtime.swift.twig (1)
74-81: Add URL encoding for query parameters to prevent URL corruption.Query strings are JSON-encoded and contain special characters (quotes, brackets) that must be percent-encoded when appended to the URL. The Web SDK implementation uses
encodeURIComponent()for this; the Swift version currently does not, creating an inconsistency. Channels have the same issue (pre-existing pattern), but queries from theQueryclass are more likely to contain characters requiring encoding due to their JSON format.🔧 Suggested fix using percent encoding
for channel in activeChannels { - queryParams += "&channels[]=\(channel)" + queryParams += "&channels[]=\(channel.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? channel)" } for query in activeQueries { - queryParams += "&queries[]=\(query)" + queryParams += "&queries[]=\(query.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? query)" }
🤖 Fix all issues with AI agents
In
`@templates/android/library/src/main/java/io/package/services/Realtime.kt.twig`:
- Around line 65-68: The loop in Realtime.kt.twig that appends queries using
queryParamBuilder and activeQueries must URL-encode each query instead of raw
string concatenation; replace the manual append of "&queries[]=$it" with adding
each query via OkHttp's HttpUrl.Builder.addQueryParameter("queries[]", query)
(same approach used in Client.kt.twig) so project names, channels and query
strings are properly encoded and won't corrupt the URL; locate the activeQueries
iteration and switch to building the request URL with
HttpUrl.Builder.addQueryParameter for each query value.
🧹 Nitpick comments (1)
templates/flutter/test/src/realtime_subscription_test.dart.twig (1)
10-14: LGTM with a minor suggestion.The test correctly passes the new
queriesparameter to match the updated constructor signature. Consider adding an assertion to verify the queries field (e.g.,expect(subscription.queries, equals(const []))), though this is optional since the primary purpose of this test is validating stream and close function wiring.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/languages/android/Tests.kt`:
- Around line 94-104: The test currently calls realtime.subscribe("tests",
payloadType = TestPayload::class.java, queries = ...) with an empty callback and
then immediately sets realtimeResponseWithQueries = "WS:/v1/realtime:passed",
which doesn't verify the subscription or query filtering; change the
subscription call to provide a non-empty callback that inspects the incoming
payload (from TestPayload) and sets realtimeResponseWithQueries based on the
payload contents (similar to the first subscription's callback pattern), and
remove the hardcoded assignment so the test only passes when the callback
receives the expected message; optionally add onOpen/onError handlers or
expectation logic if needed to mirror the Apple test behavior.
🧹 Nitpick comments (1)
tests/languages/flutter/tests.dart (1)
170-182: Inconsistent verification between subscriptions.The first subscription (line 172) verifies and prints the message payload, while the second subscription only checks that no exception is thrown when awaiting a message. The
message2variable on line 177 is unused.Consider verifying the message content consistently:
Suggested improvement
// Assert realtime outputs in a deterministic order (no-query then with-query) final message1 = await rtsub.stream.first.timeout(Duration(seconds: 10)); print(message1.payload["response"]); await rtsub.close(); - String realtimeResponseWithQueries = "WS:/v1/realtime:passed"; try { final message2 = await rtsubWithQueries.stream.first.timeout(Duration(seconds: 10)); + print("WS:/v1/realtime:passed"); } catch (e) { - realtimeResponseWithQueries = "Realtime failed!"; + print("Realtime failed!"); } - print(realtimeResponseWithQueries); await rtsubWithQueries.close();
ArnabChatterjee20k
force-pushed
the
realtime-query
branch
from
ca81782 to
8665c91
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/android/library/src/main/java/io/package/services/Realtime.kt.twig (1)
145-188: Queries are stored per-subscription but ignored during event dispatch.The
handleResponseEvent()method dispatches callbacks based only on channel matching (event.channels.any { subscription.channels.contains(it) }), without filtering againstsubscription.queries. SinceRealtimeResponseEventlacks query information, clients cannot perform per-subscription query filtering. If the backend emits events for the union of all subscribed queries, callbacks will receive all events regardless of their requested filters. Either confirm the server enforces per-subscription filtering server-side, or add query metadata toRealtimeResponseEventfor client-side filtering.
🧹 Nitpick comments (2)
tests/languages/flutter/tests.dart (1)
167-174: Consider adding error context for timeout failures.The assertions correctly validate both subscription paths sequentially. However, if a timeout occurs, the
TimeoutExceptionwon't indicate which subscription failed.♻️ Optional: Add context to timeout errors
// Assert realtime outputs in a deterministic order (no-query then with-query) - final message1 = await rtsub.stream.first.timeout(Duration(seconds: 10)); + final message1 = await rtsub.stream.first.timeout( + Duration(seconds: 10), + onTimeout: () => throw TimeoutException('No message received for no-query subscription'), + ); print(message1.payload["response"]); await rtsub.close(); - final message2 = await rtsubWithQueries.stream.first.timeout(Duration(seconds: 10)); + final message2 = await rtsubWithQueries.stream.first.timeout( + Duration(seconds: 10), + onTimeout: () => throw TimeoutException('No message received for with-query subscription'), + ); print(message2.payload["response"]); await rtsubWithQueries.close();This requires importing
dart:asyncforTimeoutException.tests/languages/apple/Tests.swift (1)
48-63: Inconsistent nil handling between subscriptions.Line 50 uses force unwrapping (
message.payload!["response"] as! String), which will crash if the payload is nil or missing the "response" key. Line 61 correctly uses safe optional chaining with a fallback. Consider aligning both to use the safer pattern for test robustness.♻️ Suggested improvement for consistency
// Subscribe without queries try await realtime.subscribe(channels: ["tests"]) { message in - realtimeResponse = message.payload!["response"] as! String + realtimeResponse = message.payload?["response"] as? String ?? realtimeResponse expectation.fulfill() }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/languages/flutter/tests.dart`:
- Around line 43-48: The queries parameter to realtime.subscribe is being passed
as a nested list causing a type mismatch: change the queries argument from a
List<List<String>> to a List<String> by removing the extra array wrapping around
the Query.equal(...) call (update the rtsubWithQueries realtime.subscribe
invocation so queries contains Query.equal(...) directly rather than wrapped in
an additional list).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@templates/apple/Sources/Services/Realtime.swift.twig`:
- Around line 74-83: The current query parameter encoding uses .urlQueryAllowed
which leaves reserved chars unescaped; update the encoding calls for project,
channel and query values to use a stricter character set (e.g.
CharacterSet.alphanumerics or a custom allowed set) when calling
addingPercentEncoding(withAllowedCharacters:), replacing the uses of
.urlQueryAllowed for encodedChannel and encodedQuery and also percent-encode
client.config["project"] when building queryParams; ensure you fallback to the
original value only if encoding returns nil.
In `@tests/languages/apple/Tests.swift`:
- Around line 54-63: The closure passed to realtime.subscribe is force-casting
message.payload?["response"] as! String which can crash; change it to safely
unwrap using an optional cast (as?) and a guard let binding inside the closure
(referencing realtime.subscribe, the closure body, realtimeResponseWithQueries
and expectationWithQueries) — if the cast fails, call XCTFail or otherwise
handle the error and return early; only assign to realtimeResponseWithQueries
and call expectationWithQueries.fulfill() after the guard succeeds.
- Around line 45-63: The subscriptions created with Realtime.subscribe are never
closed, which can lead to over-fulfillment and flaky tests; update the test to
capture the returned RealtimeSubscription handles (e.g., let sub = try await
realtime.subscribe(...)) for both the no-query and with-queries calls, and after
calling expectation.fulfill() / expectationWithQueries.fulfill() call await
sub.close() (handling throws) to stop further messages; ensure you reference the
captured handles when closing so only the intended message triggers each
expectation.
🧹 Nitpick comments (1)
templates/android/library/src/main/java/io/package/services/Realtime.kt.twig (1)
57-72: Consider using%20encoding for spaces instead of+.
URLEncoder.encode()encodes spaces as+, which is correct forapplication/x-www-form-urlencodedbut not for URL query parameters per RFC 3986 (which expects%20). While most servers handle both, for strict compliance consider replacing+with%20:♻️ Proposed fix
activeChannels.forEach { channel -> - val encodedChannel = java.net.URLEncoder.encode(channel, "UTF-8") + val encodedChannel = java.net.URLEncoder.encode(channel, "UTF-8").replace("+", "%20") queryParams += "&channels[]=$encodedChannel" } activeQueries.forEach { query -> - val encodedQuery = java.net.URLEncoder.encode(query, "UTF-8") + val encodedQuery = java.net.URLEncoder.encode(query, "UTF-8").replace("+", "%20") queryParams += "&queries[]=$encodedQuery" }Also, the
projectparameter at line 57 should be encoded for consistency with channels and queries.
| fun <T> subscribe( | ||
| vararg channels: Channel<*>, | ||
| payloadType: Class<T>, | ||
| queries: List<String> = emptyList(), |
There was a problem hiding this comment.
Let's use a Set here for free deduplication
| val channels: Collection<String>, | ||
| val queries: Collection<String>, |
There was a problem hiding this comment.
Let's change both of these to Set
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/android/library/src/main/java/io/package/services/Realtime.kt.twig (1)
132-192: Snapshotqueriesto avoid external mutation side effects.
queriesis aSetinterface and can be backed by a mutable set. If callers mutate it after subscribing, active query tracking and cleanup can drift. Taking a snapshot prevents hard‑to‑debug state bugs.🛠️ Suggested fix (snapshot the queries set)
val counter = subscriptionsCounter++ activeChannels.addAll(channels) - activeQueries.addAll(queries) + val querySnapshot = queries.toSet() + activeQueries.addAll(querySnapshot) activeSubscriptions[counter] = RealtimeCallback( channels.toSet(), - queries, + querySnapshot, payloadType, callback as (RealtimeResponseEvent<*>) -> Unit ) @@ return RealtimeSubscription { activeSubscriptions.remove(counter) - cleanUp(channels = channels.toList(), queries = queries) + cleanUp(channels = channels.toList(), queries = querySnapshot) createSocket() }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/languages/apple/Tests.swift`:
- Around line 67-75: The failure-case subscription uses the same query as the
success case so its callback will incorrectly match; in the realtime.subscribe
call that sets realtimeResponseWithQueriesFailure and fulfills
expectationWithQueriesFailure, change the Query.equal(...) predicate to use a
non-matching key/value (e.g., compare "type" or a different expected value than
"WS:/v1/realtime:passed") so the failure subscription does not match the
successful message and realtimeResponseWithQueriesFailure remains "Realtime
failed!".
♻️ Duplicate comments (1)
tests/languages/apple/Tests.swift (1)
62-65: Guard payload extraction to avoid potential crash.The optional chaining followed by force cast
as!can crash ifpayloadis nil, doesn't contain"response", or the value isn't aString. Use optional binding with a guard statement instead.🛠️ Suggested fix
) { message in - realtimeResponseWithQueries = message.payload?["response"] as! String + guard let response = message.payload?["response"] as? String else { + XCTFail("Missing or invalid 'response' in realtime payload") + return + } + realtimeResponseWithQueries = response expectationWithQueries.fulfill() }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/languages/apple/Tests.swift`:
- Around line 47-76: The test erroneously fulfills expectationWithQueriesFailure
immediately, so change the expectation to be inverted by setting
expectationWithQueriesFailure.isInverted = true and remove the immediate
expectationWithQueriesFailure.fulfill() call; instead call
expectationWithQueriesFailure.fulfill() only inside the non-matching
subscription callback (the closure passed to realtime.subscribe for
Query.equal("response", value: ["failed"])) so the inverted expectation will
fail if that callback is invoked and pass when no message arrives.
In `@tests/languages/flutter/tests.dart`:
- Around line 185-193: The test prints the same "Realtime failed!" in both the
success (timeout) and failure (message received) branches, making results
ambiguous; change the TimeoutException branch to print a success message such as
"Realtime passed: no matching message received" while keeping the
message-received branch as "Realtime failed!" so
rtsubWithQueriesFailure.stream.first.timeout and the TimeoutException handling
clearly reflect expected vs. unexpected outcomes.
♻️ Duplicate comments (2)
tests/languages/apple/Tests.swift (2)
51-74: Close realtime subscriptions after first message to avoid flakiness.Without closing, additional messages can over-fulfill expectations and make tests nondeterministic. Capture the returned
RealtimeSubscriptionhandles and close them after fulfillment.♻️ Proposed fix
- try await realtime.subscribe(channels: ["tests"]) { message in + var subscriptionNoQueries: RealtimeSubscription? + subscriptionNoQueries = try await realtime.subscribe(channels: ["tests"]) { message in realtimeResponse = message.payload!["response"] as! String expectation.fulfill() + Task { try? await subscriptionNoQueries?.close() } } - try await realtime.subscribe( + var subscriptionWithQueries: RealtimeSubscription? + subscriptionWithQueries = try await realtime.subscribe( channels: ["tests"], queries: [ Query.equal("response", value: ["WS:/v1/realtime:passed"]) ] ) { message in realtimeResponseWithQueries = message.payload?["response"] as! String expectationWithQueries.fulfill() + Task { try? await subscriptionWithQueries?.close() } }
51-74: Avoid force-unwrap/force-cast on realtime payloads.Line 52, Line 63, and Line 73 can crash if the payload is missing or not a
String. Prefer guarded optional casts withXCTFail.🛠️ Proposed fix
- try await realtime.subscribe(channels: ["tests"]) { message in - realtimeResponse = message.payload!["response"] as! String + try await realtime.subscribe(channels: ["tests"]) { message in + guard let response = message.payload?["response"] as? String else { + XCTFail("Missing or invalid `response` in realtime payload") + return + } + realtimeResponse = response expectation.fulfill() }(Apply the same guard pattern to the with-queries and failure subscriptions.)
| try { | ||
| final message3 = await rtsubWithQueriesFailure.stream.first.timeout(Duration(seconds: 10)); | ||
| // If we receive a message, it means the query filtering failed, so realtime failed | ||
| print("Realtime failed!"); | ||
| } on TimeoutException { | ||
| // Timeout means no matching message was received, which is expected for a failure query | ||
| print("Realtime failed!"); | ||
| } | ||
| await rtsubWithQueriesFailure.close(); |
There was a problem hiding this comment.
Misleading test output: both branches print the same "Realtime failed!" message.
The failure test logic prints "Realtime failed!" in both scenarios:
- When a message is received (line 188) - indicating query filtering failed
- When a timeout occurs (line 191) - indicating query filtering worked correctly
This makes it impossible to distinguish between actual test failures and expected behavior. The timeout case (query successfully filtered the message) should print a success message.
🐛 Proposed fix
try {
final message3 = await rtsubWithQueriesFailure.stream.first.timeout(Duration(seconds: 10));
// If we receive a message, it means the query filtering failed, so realtime failed
- print("Realtime failed!");
+ print("Realtime query filtering failed - unexpected message received!");
} on TimeoutException {
// Timeout means no matching message was received, which is expected for a failure query
- print("Realtime failed!");
+ print("Realtime query filtering passed!");
}📝 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.
| try { | |
| final message3 = await rtsubWithQueriesFailure.stream.first.timeout(Duration(seconds: 10)); | |
| // If we receive a message, it means the query filtering failed, so realtime failed | |
| print("Realtime failed!"); | |
| } on TimeoutException { | |
| // Timeout means no matching message was received, which is expected for a failure query | |
| print("Realtime failed!"); | |
| } | |
| await rtsubWithQueriesFailure.close(); | |
| try { | |
| final message3 = await rtsubWithQueriesFailure.stream.first.timeout(Duration(seconds: 10)); | |
| // If we receive a message, it means the query filtering failed, so realtime failed | |
| print("Realtime query filtering failed - unexpected message received!"); | |
| } on TimeoutException { | |
| // Timeout means no matching message was received, which is expected for a failure query | |
| print("Realtime query filtering passed!"); | |
| } | |
| await rtsubWithQueriesFailure.close(); |
🤖 Prompt for AI Agents
In `@tests/languages/flutter/tests.dart` around lines 185 - 193, The test prints
the same "Realtime failed!" in both the success (timeout) and failure (message
received) branches, making results ambiguous; change the TimeoutException branch
to print a success message such as "Realtime passed: no matching message
received" while keeping the message-received branch as "Realtime failed!" so
rtsubWithQueriesFailure.stream.first.timeout and the TimeoutException handling
clearly reflect expected vs. unexpected outcomes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/languages/web/index.html`:
- Around line 50-59: The subscribe calls pass a double-nested queries array
([[Query.equal(...)]]), but the Web SDK expects a flat array of queries
(queries?: (string | Query)[]); update the calls to use [Query.equal('response',
["WS:/v1/realtime:passed"])] instead of [[Query.equal(...)]], and do the same
for the second subscribe so each Query.equal(...) is a direct element of the
queries array in the subscribe(...) invocation (refer to the subscribe function
and Query.equal references and the responseRealtimeWithQueries variable to
locate the calls).
🧹 Nitpick comments (1)
tests/languages/android/Tests.kt (1)
107-115: Clarify the intent of this failure path test.The callback hardcodes
"WS:/v1/realtime:passed"instead of reading from the payload. Given the query filters forresponse: "failed"and the server sends"WS:/v1/realtime:passed", the callback should never fire, leavingrealtimeResponseWithQueriesFailureas"Realtime failed!".If the callback does fire (indicating query filtering failed), it would incorrectly log
"WS:/v1/realtime:passed"- making the test output indistinguishable from success. Consider either:
- Keeping the payload assignment like the other subscription for consistency
- Adding a comment explaining this is intentionally testing that the callback does NOT fire
Suggestion for clearer test intent
realtimeWithFailure.subscribe( "tests", payloadType = TestPayload::class.java, queries = setOf( Query.equal("response", listOf("failed")) ) ) { - realtimeResponseWithQueriesFailure = "WS:/v1/realtime:passed" + // This callback should NOT fire since query filters for "failed" + // but server sends "WS:/v1/realtime:passed" + realtimeResponseWithQueriesFailure = it.payload.response }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/languages/apple/Tests.swift`:
- Around line 52-56: The test currently force-unwraps message.payload with
message.payload! inside the realtime.subscribe callback which can crash if
payload is nil; change the callback in the realtime.subscribe invocation to
safely unwrap message.payload and the "response" value (e.g., use guard let
payload = message.payload, let resp = payload["response"] as? String) before
assigning to realtimeResponse and calling expectation.fulfill(), leaving
realtimeResponse and expectation untouched if the payload or response is
missing.
♻️ Duplicate comments (3)
tests/languages/apple/Tests.swift (3)
53-77: Capture subscription handles and close after fulfillment to prevent test flakiness.The
subscribecalls returnRealtimeSubscriptionhandles that are being discarded. If the server sends multiple messages, the expectations could be over-fulfilled, causing flaky tests. Capture the handles and close subscriptions after fulfillment:♻️ Suggested pattern
- try await realtime.subscribe(channels: ["tests"]) { message in + let subscriptionNoQueries = try await realtime.subscribe(channels: ["tests"]) { message in // ... handle message ... expectation.fulfill() + Task { try? await subscriptionNoQueries.close() } } - try await realtime.subscribe( + let subscriptionWithQueries = try await realtime.subscribe( channels: ["tests"], queries: [...] ) { message in // ... handle message ... expectationWithQueries.fulfill() + Task { try? await subscriptionWithQueries.close() } }
58-67: Guard realtime payload extraction to avoid crash.The optional chaining on
payload?["response"]followed by force castas!will crash if the response value is missing or not a String. Use an optional cast with a guard statement instead.🛠️ Suggested fix
try await realtime.subscribe( channels: ["tests"], queries: [ Query.equal("response", value: ["WS:/v1/realtime:passed"]) ] ) { message in - realtimeResponseWithQueries = message.payload?["response"] as! String + guard let response = message.payload?["response"] as? String else { + XCTFail("Missing or invalid `response` in realtime payload") + return + } + realtimeResponseWithQueries = response expectationWithQueries.fulfill() }
69-77: Same force cast issue in failure path callback.Apply the same safe unwrapping pattern here for consistency:
🛠️ Suggested fix
try await realtimeWithFailure.subscribe( channels: ["tests"], queries: [ Query.equal("response", value: ["failed"]) ] ) { message in - realtimeResponseWithQueriesFailure = message.payload?["response"] as! String + guard let response = message.payload?["response"] as? String else { + XCTFail("Missing or invalid `response` in realtime payload") + return + } + realtimeResponseWithQueriesFailure = response expectationWithQueriesFailure.fulfill() }
🧹 Nitpick comments (1)
tests/languages/apple/Tests.swift (1)
196-200: Remove redundant conditional—isInvertedis alwaystrue.The
isInvertedproperty was set totrueon line 50 and never changes, making theelsebranch dead code. The intent appears to be confirming the callback was never invoked by printing the unchanged initial value. Simplify by removing the conditional:♻️ Suggested simplification
wait(for: [expectationWithQueriesFailure], timeout: 10.0) - if expectationWithQueriesFailure.isInverted { - print(realtimeResponseWithQueriesFailure) - } else { - print("Realtime failed") - } + // Prints initial value "Realtime failed!" confirming callback was never invoked + print(realtimeResponseWithQueriesFailure)
| // Subscribe without queries | ||
| try await realtime.subscribe(channels: ["tests"]) { message in | ||
| realtimeResponse = message.payload!["response"] as! String | ||
| expectation.fulfill() | ||
| } |
There was a problem hiding this comment.
Guard against nil payload to avoid crash.
Line 54 uses message.payload! which will crash if payload is nil. This is inconsistent with lines 65 and 75 which use optional chaining (payload?). Consider using safe unwrapping:
🛠️ Suggested fix
try await realtime.subscribe(channels: ["tests"]) { message in
- realtimeResponse = message.payload!["response"] as! String
+ guard let response = message.payload?["response"] as? String else {
+ XCTFail("Missing or invalid `response` in realtime payload")
+ return
+ }
+ realtimeResponse = response
expectation.fulfill()
}🤖 Prompt for AI Agents
In `@tests/languages/apple/Tests.swift` around lines 52 - 56, The test currently
force-unwraps message.payload with message.payload! inside the
realtime.subscribe callback which can crash if payload is nil; change the
callback in the realtime.subscribe invocation to safely unwrap message.payload
and the "response" value (e.g., use guard let payload = message.payload, let
resp = payload["response"] as? String) before assigning to realtimeResponse and
calling expectation.fulfill(), leaving realtimeResponse and expectation
untouched if the payload or response is missing.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/web/src/services/realtime.ts.twig (1)
494-579:⚠️ Potential issue | 🟠 MajorFix query matching to avoid cross-channel or subset false positives.
Flattening all query sets across channels and treating subset matches as valid can deliver events to subscriptions whose queries didn’t match for the relevant channel (e.g., multi-channel events or another subscription with a subset). Match query sets per channel and require set equality.
🛠️ Suggested fix
- // Check if subscription matches any query set from the response + // Check if subscription matches any query set for matching channels let matchesQueries = false; if (subscription.queries.size === 0) { // Subscription has no queries - it listens to all events matchesQueries = true; } else if (queriesData && queriesData.length > 0) { - // Flatten to get all query sets: [[q1, q2], [q3], [q4]] - const allQuerySets = queriesData.flat(); - // Normalize subscription queries for comparison (parse and re-stringify) const normalizedSubQueries = new Set<string>(); for (const q of subscription.queries) { try { normalizedSubQueries.add(JSON.stringify(JSON.parse(q))); } catch { normalizedSubQueries.add(q); } } - // Check if any query set from response matches the subscription's query set - for (const querySet of allQuerySets) { - if (!Array.isArray(querySet)) continue; - - // Check if this query set matches the subscription's queries - // A match means all queries in the response query set are in the subscription - const querySetMatches = querySet.length > 0 && querySet.every(query => { - // Normalize the query string from response - let normalizedQuery = query; - try { - normalizedQuery = JSON.stringify(JSON.parse(query)); - } catch { - // If parsing fails, use original - } - return normalizedSubQueries.has(normalizedQuery); - }); - - if (querySetMatches) { - matchesQueries = true; - break; - } - } + // Check query sets only for channels this subscription matches + for (let i = 0; i < channels.length; i++) { + if (!subscription.channels.has(channels[i])) continue; + const channelQuerySets = queriesData[i] ?? []; + + for (const querySet of channelQuerySets) { + if (!Array.isArray(querySet)) continue; + + const querySetMatches = + querySet.length === normalizedSubQueries.size && + querySet.every(query => { + let normalizedQuery = query; + try { + normalizedQuery = JSON.stringify(JSON.parse(query)); + } catch { + // If parsing fails, use original + } + return normalizedSubQueries.has(normalizedQuery); + }); + + if (querySetMatches) { + matchesQueries = true; + break; + } + } + if (matchesQueries) break; + } } else { // No queries in response, match all subscriptions matchesQueries = true; }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@templates/android/library/src/main/java/io/package/services/Realtime.kt.twig`:
- Around line 204-206: subscriptionsCounter++ is not atomic and can race when
subscribe() is called concurrently; change subscriptionsCounter to an
AtomicInteger and replace the two-step increment+read in subscribe() with a
single atomic call (e.g. subscriptionsCounter.incrementAndGet()) to obtain slot,
updating any imports/usages accordingly so slot assignment is a single atomic
operation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@templates/android/library/src/main/java/io/package/services/Realtime.kt.twig`:
- Around line 72-77: The query string builds queryParams using
client.config["project"] without URL-encoding, causing inconsistency with the
encoded channels/queries; update the initialization of queryParams so the
project value is encoded via
java.net.URLEncoder.encode(client.config["project"], "UTF-8") (same encoder used
for channels in the allChannels loop) so the project parameter is safely escaped
and consistent with the handling in the channels and queries logic.
- Around line 208-209: The line declaring an unused local variable queryList in
Realtime.kt.twig should be removed to eliminate dead code; locate the assignment
"val queryList = queries.toList()" and delete it (ensure no other code expects
queryList—if any subsequent logic intended to use the list, replace usages to
operate directly on queries or on queries.toList() in-place).
🧹 Nitpick comments (1)
templates/android/library/src/main/java/io/package/services/Realtime.kt.twig (1)
303-304:@Suppress("UNCHECKED_CAST")is unnecessary here.The cast
as? List<*>is a safe cast, andmapNotNull { it as? String }uses safe casts too. No unchecked cast warning is generated, so the suppression annotation can be removed.Proposed fix
- `@Suppress`("UNCHECKED_CAST") - val subscriptions = (messageData["subscriptions"] as? List<*>)?.mapNotNull { it as? String } + val subscriptions = (messageData["subscriptions"] as? List<*>)?.mapNotNull { it as? String }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/tests.yml:
- Around line 19-50: The CI matrix currently has almost all SDK entries
commented out (e.g., CLIBun10, DartBeta, DotNet60, FlutterStable, Go112,
KotlinJava8, Node16, PHP80, Python39, Ruby27, Swift56, WebChromium, etc. as
shown in the commented list), so restore full test coverage by uncommenting
those SDK matrix entries in the workflow file (re-enable each commented SDK
line) before merging to master; if you intended a temporary restriction for
staging, instead move the commented/limited matrix into a separate branch or add
a short-lived workflow override rather than landing the reduced matrix on
master.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@templates/android/library/src/main/java/io/package/services/Realtime.kt.twig`:
- Line 10: Remove the unused import `import {{ sdk.namespace | caseDot
}}.extensions.gson` from Realtime.kt.twig; the file uses the `fromJson` and
`jsonCast` extension functions (referenced/used in methods calling
fromJson/jsonCast) which are imported separately, so delete the dead `gson`
import to eliminate the unused symbol.
- Around line 270-288: The clear-then-populate sequence in
handleResponseConnected can let concurrent handleResponseEvent reads see empty
maps; to fix, either (A) build new local maps, populate them from subscriptions
and then inside synchronized(subscriptionLock) replace slotToSubscriptionId and
subscriptionIdToSlot with the new maps in one atomic swap, or (B) ensure
handleResponseEvent reads the maps inside synchronized(subscriptionLock) as
well; update code around handleResponseConnected, slotToSubscriptionId,
subscriptionIdToSlot and subscriptionLock to use one of these approaches so
there is no window where events observe cleared maps.
🧹 Nitpick comments (1)
templates/android/library/src/main/java/io/package/services/Realtime.kt.twig (1)
232-242: Unsubscribe closure correctly coordinates map cleanup undersubscriptionLock.The three-map cleanup (activeSubscriptions, slotToSubscriptionId, subscriptionIdToSlot) is properly guarded, and
createSocket()is called after the lock is released to rebuild the connection with remaining subscriptions.One minor note: unlike
subscribe()which debounces socket recreation,unsubscribetriggerscreateSocket()immediately. Rapid successive unsubscribes will each recreate the socket. Consider whether a debounce is warranted here as well for burst-unsubscribe scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@templates/android/library/src/main/java/io/package/services/Realtime.kt.twig`:
- Around line 59-102: The createSocket() method iterates activeSubscriptions
without holding subscriptionLock which can race with unsubscribe; to serialize
socket management, change the unsubscribe closure to dispatch createSocket()
onto Dispatchers.Main instead of calling it directly—e.g., replace the direct
createSocket() call in the unsubscribe/remove path with launching a coroutine on
Dispatchers.Main (using the same coroutine approach used in the subscribe/launch
path), so all calls to createSocket() run on the Main dispatcher and avoid
concurrent snapshot inconsistencies of activeSubscriptions.
🧹 Nitpick comments (1)
templates/android/library/src/main/java/io/package/services/Realtime.kt.twig (1)
223-230: Debounce viasubCallDepthrelies on Main-thread confinement — document or assert.
subCallDepthis a plainvarthat's safe only because alllaunch {}calls use the defaultDispatchers.Maincontext from theCoroutineScope. If someone later adds an explicit dispatcher or calls from a different scope, this becomes a race. A brief comment noting the Main-thread invariant (or an assertion) would make this less fragile.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/languages/apple/Tests.swift`:
- Around line 196-201: The conditional checking
expectationWithQueriesFailure.isInverted is tautological because isInverted is
always set true for expectationWithQueriesFailure; remove the unreachable else
branch and simplify the block to directly print
realtimeResponseWithQueriesFailure (replace the if/else around
expectationWithQueriesFailure/isInverted with a single print of
realtimeResponseWithQueriesFailure). Keep references to
expectationWithQueriesFailure and realtimeResponseWithQueriesFailure when
editing so you modify the correct test code.
🧹 Nitpick comments (2)
tests/languages/flutter/tests.dart (1)
174-177: UnhandledTimeoutExceptionon the happy-path subscriptions.Lines 175 and 179 call
.timeout(Duration(seconds: 10))without atry/catch. If either stream doesn't receive a message within 10 seconds, the test crashes with an unhandledTimeoutExceptionand no diagnostic output. This may be intentional (fail-fast), but adding atry/catchwith a descriptive error message (e.g.,"Realtime: timed out waiting for message on no-query subscription") would make CI failures much easier to debug.tests/languages/apple/Tests.swift (1)
190-196: Consider combining the two non-inverted expectations into a singlewaitcall to reduce test duration.
expectationandexpectationWithQueriescan be waited on together since they're both non-inverted. The inverted one must remain separate. This avoids serializing two 20-second timeouts.♻️ Suggested change
- wait(for: [expectation], timeout: 20.0) - print(realtimeResponse) - - wait(for: [expectationWithQueries], timeout: 20.0) - print(realtimeResponseWithQueries) + wait(for: [expectation, expectationWithQueries], timeout: 20.0) + print(realtimeResponse) + print(realtimeResponseWithQueries)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@templates/android/library/src/main/java/io/package/services/Realtime.kt.twig`:
- Around line 310-323: Callbacks are being invoked from the IO dispatcher inside
onMessage (launched with launch(IO)), which can crash if the callback updates
the UI; update the invocation path in the subscriptions.forEach loop (where
subscriptionIdToSlot, activeSubscriptions, event.copy and subscription.callback
are used) to ensure callbacks run on the main thread — e.g., check
Looper.getMainLooper().isCurrentThread and, if not, call the callback via
withContext(Dispatchers.Main) (or always wrap subscription.callback(typedEvent)
in withContext(Dispatchers.Main)) so UI updates are executed on
Dispatchers.Main.
- Line 84: The call to Query.select in the selectAllQuery assignment uses a Char
literal list (listOf('*')) but Query.select expects List<String>; update the
argument to a List<String> (e.g., use double-quoted "*" instead of single-quoted
'*') so Query.select(listOf("*")).toString() produces the correct type for
selectAllQuery.
In `@tests/languages/flutter/tests.dart`:
- Around line 174-181: Wrap the two awaits that call .timeout(Duration(seconds:
10)) (the calls on rtsub.stream.first and rtsubWithQueries.stream.first) in
try/catch blocks that catch TimeoutException and call the test failure helper
(e.g., fail(...) or expect(false, ...)) with a descriptive message (matching the
style used for the failure case at line 183) so a timeout on either realtime
subscription reports a clear test failure instead of aborting the whole suite;
ensure each catch message names which subscription timed out.
🧹 Nitpick comments (2)
tests/languages/android/Tests.kt (2)
106-114: Add a comment clarifying the negative-test intent.The third subscription is a negative test: the query filters for
"failed", so the callback should never fire if query filtering works correctly. The initial"Realtime failed!"value remaining would paradoxically indicate success.A brief inline comment would save future readers from confusion, especially since the callback hardcodes
"WS:/v1/realtime:passed"— which would only appear in the output if filtering is broken.Suggested clarification
- realtime.subscribe( + // Negative test: query won't match the server message, so the callback + // should never fire and the variable should stay "Realtime failed!" + realtime.subscribe( "tests", payloadType = TestPayload::class.java, queries = setOf( Query.equal("response", listOf("failed")) ) ) { realtimeResponseWithQueriesFailure = "WS:/v1/realtime:passed" }
216-216: Consider replacing the fixed 30-second delay with a signaling mechanism.A 30-second unconditional wait makes the test suite significantly slower and is still race-prone (it could be too short on a loaded CI runner or too long for local development). Using a
CompletableDeferredorCountDownLatchto signal when the expected callbacks have fired would make the test both faster and more reliable.Sketch using CompletableDeferred
// At the top of the test, alongside the response variables: val realtimeDone = CompletableDeferred<Unit>() // In the first (no-query) subscription callback: realtime.subscribe("tests", payloadType = TestPayload::class.java) { realtimeResponse = it.payload.response realtimeDone.complete(Unit) } // Then replace the fixed delay with a bounded await: withTimeout(30_000) { realtimeDone.await() }This way the test completes as soon as the message arrives (typically a few seconds) but still fails deterministically after 30 s.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@templates/android/library/src/main/java/io/package/services/Realtime.kt.twig`:
- Around line 207-209: Remove the extra blank line reported by Twig linting
after the statement that assigns val slot =
subscriptionsCounter.incrementAndGet(); locate the occurrence in
Realtime.kt.twig where the subscriptionsCounter.incrementAndGet() call is used
(the line declaring slot) and delete the blank line immediately following it so
there are no consecutive blank lines.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/web/src/client.ts.twig (1)
574-584:⚠️ Potential issue | 🟠 MajorEvent dispatch doesn't leverage the
subscriptionsfield from the backend.The handler routes events by matching
data.channelsagainst each subscription'schannelsarray. Based on learnings, the backend always provides asubscriptionsfield in event messages. Now that subscriptions carry query context, channel-only matching is insufficient for correct per-subscription dispatch.Consider adding query-aware filtering here, for example by checking
data.subscriptionsagainst the subscription's stored identifier or by additionally matchingsubscription.queriesagainst the event's metadata.
🤖 Fix all issues with AI agents
In `@templates/web/src/client.ts.twig`:
- Around line 504-506: The deprecated subscribe method currently appends all
realtime.queries to the connection URL (channels.append('queries[]', query)) so
events are routed only by channel and all subscriptions on a channel receive
every event; fix by wiring per-subscription filtering into this path: when
creating a subscription in subscribe, include a unique subscription identifier
or encode that subscription's queries (not the global realtime.queries) in the
channel/URL params, and then, in the event dispatch path that routes by channel,
consult the per-subscription entry in realtime.subscriptions (or the
subscription ID sent by the server) and compare the incoming event against that
subscription's queries before invoking its callback. Ensure subscribe, the
URL-building code that calls channels.append('queries[]', ...), and the event
router that currently matches only by channel use the per-subscription
queries/ID stored on realtime.subscriptions to perform client-side filtering.
🧹 Nitpick comments (4)
templates/web/src/client.ts.twig (2)
679-680: Redundant null-coalescing?? [].
queriesalready has a default value of[](line 662), so it can never benullorundefinedat this point. The?? []is unnecessary.Proposed fix
- const queryStrings = (queries ?? []).map(q => typeof q === 'string' ? q : q.toString()); + const queryStrings = queries.map(q => typeof q === 'string' ? q : q.toString());
597-620: Query cleanup logic mirrors channel cleanup — correct, with a minor inconsistency.The approach of checking whether any remaining subscription still references a query before removing it from the global set is sound and mirrors the channel cleanup pattern.
Line 613 uses optional chaining (
subscription.queries?.includes(query)) even though the type on line 207 declaresqueries: string[]as non-optional. This won't cause a bug, but it's inconsistent with the channel cleanup at line 601 which doesn't use optional chaining forsubscription.channels. Consider aligning them for consistency.Proposed fix
- return subscription.queries?.includes(query); + return subscription.queries.includes(query);templates/android/library/src/main/java/io/package/services/Realtime.kt.twig (2)
169-185: Non-genericsubscribeoverloads don't expose thequeriesparameter.Users must switch to the generic overloads (lines 187+/201+) and provide an explicit
payloadTypeto use query-based subscriptions. Consider adding convenience overloads that acceptquerieswithout requiringpayloadType:♻️ Suggested additions
+ fun subscribe( + vararg channels: String, + queries: Set<String>, + callback: (RealtimeResponseEvent<Any>) -> Unit, + ) = subscribe( + channels = channels, + payloadType = Any::class.java, + queries = queries, + callback = callback + ) + + fun subscribe( + vararg channels: Channel<*>, + queries: Set<String>, + callback: (RealtimeResponseEvent<Any>) -> Unit, + ) = subscribe( + channels = channels.map { channelToString(it) }.toTypedArray(), + payloadType = Any::class.java, + queries = queries, + callback = callback + )
222-229: Debounce viasubCallDepthrelies onlaunch {}running onDispatchers.Main.This works because the
coroutineContextisDispatchers.Main + job(line 31), so alllaunch {}calls are serialized on the main thread. However,subCallDepthis a companion-objectvarshared across allRealtimeinstances — if multiple instances exist with differentCoroutineScopecontexts, this could break. Currently safe given the single-context design, but worth a comment in the code.
| this.realtime.queries.forEach(query => { | ||
| channels.append('queries[]', query); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the realtime service template to understand event dispatch
fd "realtime.ts.twig" --type fRepository: appwrite/sdk-generator
Length of output: 109
🏁 Script executed:
# Look for how subscriptions field is handled in the realtime service
rg -i "subscriptions" --type twig -B 3 -A 3Repository: appwrite/sdk-generator
Length of output: 46645
🏁 Script executed:
# Check the onMessage handler in realtime service to see if it uses subscriptions field
rg "onMessage" --type twig -B 5 -A 10Repository: appwrite/sdk-generator
Length of output: 24250
Per-subscription query filtering not applied in deprecated code path.
The subscribe method (marked @deprecated on line 627) aggregates all queries globally in the URL (queries[] at lines 504–506) and routes events solely by channel matching (line 580). This means:
- Two subscriptions on the same channel with different queries both receive all events for that channel
- The per-subscription
queriesfield stored at line 685 is never consulted during event dispatch
The new realtime.ts.twig service correctly implements per-subscription routing using backend-provided subscription IDs, but the deprecated code path does not use this mechanism. While server-side filtering will still apply based on the union of all queries, client-side dispatch will not respect individual subscription query boundaries.
🤖 Prompt for AI Agents
In `@templates/web/src/client.ts.twig` around lines 504 - 506, The deprecated
subscribe method currently appends all realtime.queries to the connection URL
(channels.append('queries[]', query)) so events are routed only by channel and
all subscriptions on a channel receive every event; fix by wiring
per-subscription filtering into this path: when creating a subscription in
subscribe, include a unique subscription identifier or encode that
subscription's queries (not the global realtime.queries) in the channel/URL
params, and then, in the event dispatch path that routes by channel, consult the
per-subscription entry in realtime.subscriptions (or the subscription ID sent by
the server) and compare the incoming event against that subscription's queries
before invoking its callback. Ensure subscribe, the URL-building code that calls
channels.append('queries[]', ...), and the event router that currently matches
only by channel use the per-subscription queries/ID stored on
realtime.subscriptions to perform client-side filtering.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
3 participants
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Breaking Changes
Tests