feat: Ask AI SSE streaming (performAskAIStreaming)#2
feat: Ask AI SSE streaming (performAskAIStreaming)#2CryptohopperCareers wants to merge 7 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
start() captured self weakly in checkAuthentication's onSuccess/onFail closures, but the only strong reference to the request lives in the caller's local variable, which goes out of scope when performAskAIStreaming returns. checkAuthentication's token-refresh path is async, so on an expired access token the request object could deallocate before the callback ran, silently dropping the request and never firing completion. Capture self strongly instead so the closure keeps the request alive until startRequest(accessToken:) hands retention to the URLSession delegate, or the fail path calls finish(...).
HopperAPIAskAIStreamingRequest's JSON fallback completion path decoded non-2xx responses into an error but never posted CH_DEVICE_UNAUTHORIZED on HTTP 402, unlike HopperAPIRequest.startRequest's common path. Mirror both of that path's 402 checks: a non-2xx HTTP 402, and a 2xx response whose body is a HopperCommonMessageResponse with error != nil and status == 402. Both now post the notification and fail with HopperError.DeviceUnauthorized.
The public API router sets a global application/json header before the askai SSE branch runs, so streams arrive with the wrong Content-Type. Sniff the body (SSE starts with "event:", "data:" or a comment line) both on first chunk and at completion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // Mirror HopperAPIRequest.startRequest's responseOK check: a 2xx | ||
| // body can still carry a common error payload with status 402. | ||
| if let errCode = try? decoder.decode(HopperCommonMessageResponse.self, from: jsonBody), | ||
| errCode.error != nil, errCode.status == 402 { |
There was a problem hiding this comment.
JSON fallback only surfaces the 402 common-error case; other in-band errors are swallowed as success.
The comment says this mirrors HopperAPIRequest.startRequest's responseOK, but responseOK calls onFail(err) for any common-error payload where error != nil && status != nil (see HopperAPIRequest.swift:144-154), not just status == 402. Here, only status == 402 is handled; everything else falls through to decode(HopperAPIPerformAskAIResponse.self).
Failure scenario: a 2xx response carrying {"error": 1, "status": 400, "message": "..."}. HopperAPIPerformAskAIResponse.answer maps to the "data" key, which is absent, so decoding succeeds with answer == nil and the caller receives .success(nil) — the backend error is silently reported as a successful (empty) answer instead of a failure.
| } | ||
| if isEventStream { | ||
| emit(parser.flush()) | ||
| finish(.success(nil)) |
There was a problem hiding this comment.
The event-stream completion path reports .success(nil) without checking the HTTP status or any in-band error event.
When isEventStream is true, didCompleteWithError unconditionally calls finish(.success(nil)). Two consequences:
-
HTTP status is ignored. A non-2xx response that is delivered as (or sniffed as)
text/event-stream— e.g. a 500, or a 402 arriving on the stream path — is reported tocompletionas success. Unlike the JSON branch below,triggerDeviceUnauthorized()is never posted for a 402 that comes through as a stream. -
In-band errors aren't reflected in
completion. A 200 stream that ends with anevent: error/event: donecarrying{"error": ...}frame produces an.errorviaonEvent, butcompletionstill fires.success(nil). A caller that dismisses its loading state oncompletionand only branches on.failurewill treat an errored stream as success.
Consider checking (task.response as? HTTPURLResponse)?.statusCode here, and/or tracking whether the stream emitted a terminal error so completion can reflect it.
| request.setValue(UIDevice.current.identifierForVendor?.uuidString ?? "", forHTTPHeaderField: "DeviceId") | ||
| request.setValue(config.apiBasicValidationValue, forHTTPHeaderField: config.apiBasicValidationKey) | ||
| request.setValue(accessToken, forHTTPHeaderField: "access-token") | ||
| request.httpBody = try? JSONSerialization.data(withJSONObject: ["page": page, "question": question]) |
There was a problem hiding this comment.
Streaming request always sends question (even empty) and drops suggestion_id, diverging from the non-streaming request.
HopperAPIPerformAskAIRequest omits question when it is empty and supports a suggestion_id body field (HopperAPIPerformAskAIRequest.swift:15-21). This body hardcodes ["page": page, "question": question].
Failure scenario: a suggestion-tap flow (empty question + a suggestion_id) cannot be expressed through performAskAIStreaming, and sending an empty question string may be rejected or mishandled by the backend where the non-streaming path would have omitted it. If suggestion-based asks are meant to stream too, the public API can't reach them.
pimfeltkamp
left a comment
There was a problem hiding this comment.
Automated review — 3 findings (details inline):
- [medium] JSON fallback swallows non-402 errors (
HopperAPIAskAIStreamingRequest.swift:157): a 2xx body with a common-error payload whose status isn't 402 decodes to an empty answer and is reported as.success(nil)instead of a failure, unlikeHopperAPIRequest.responseOK. - [medium] Stream completion ignores HTTP status & in-band errors (
:145): the event-stream path always finishes.success(nil)— a non-2xx stream (incl. a 402, skippingtriggerDeviceUnauthorized()) or a stream ending in anerrorevent is reported as success. - [low] Streaming request drops
suggestion_idand always sendsquestion(:74): diverges fromHopperAPIPerformAskAIRequest, so suggestion-tap flows can't stream.
The monolith's suggestion branch returns the answer as a plain string in
`data` instead of the {id, content, ...} object the typed-question path
uses; decode fell over on the type mismatch. Fall back to wrapping the
string in an AskAIAnswer.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| // Mirror HopperAPIRequest.startRequest's responseOK check: a 2xx | ||
| // body can still carry a common error payload with status 402. | ||
| if let errCode = try? decoder.decode(HopperCommonMessageResponse.self, from: jsonBody), | ||
| errCode.error != nil, errCode.status == 402 { |
There was a problem hiding this comment.
Medium — 2xx error envelope (non-402) is silently swallowed as success(nil).
This JSON fallback only surfaces an error when errCode.status == 402. The canonical path it says it mirrors (HopperAPIRequest.startRequest's responseOK) treats any error != nil && status != nil envelope as a failure: 402 → DeviceUnauthorized, everything else → onFail(CustomError(message)). Here a 2xx body such as {"error": 1, "status": 400, "message": "..."} skips this branch, gets decoded as a normal HopperAPIPerformAskAIResponse (whose new init(from:) never throws), and the caller receives .success(nil) — never learning the request failed. Consider mirroring the full responseOK check, not just the 402 case.
| } | ||
|
|
||
|
|
||
| required init(from decoder: Decoder) throws { |
There was a problem hiding this comment.
Medium — custom init(from:) never throws, so malformed/error data payloads decode as a valid answer with nil content.
Replacing the synthesized Codable init with this hand-written one changes behavior for both the new streaming fallback and the existing HopperAPIPerformAskAIRequest path (which decodes this same type via HopperAPIRequest.responseOK). Because neither try? branch rethrows, any data value that is neither an AskAIAnswer object nor a String (a number, an array, an error envelope, or a truncated body) leaves answer == nil and the decode succeeds. Previously such a body threw and reached onFail. Net effect: genuinely malformed responses are now reported to callers as an empty-but-successful answer instead of an error.
| } | ||
| if isEventStream { | ||
| emit(parser.flush()) | ||
| finish(.success(nil)) |
There was a problem hiding this comment.
Low/Medium — a stream that ends with an .error event still completes with .success(nil).
On any successfully-terminated event stream, completion is always .success(nil), even when the parser emitted .error(message:) for a done/final-with-error or an explicit error frame. Stream errors are only observable through onEvent; a caller that keys terminal state off completion (e.g. to show a failure UI or retry) will treat a failed answer as a success. Consider tracking whether an error event was seen and finishing with .failure in that case, or documenting that callers must inspect onEvent.
| request.setValue(UIDevice.current.identifierForVendor?.uuidString ?? "", forHTTPHeaderField: "DeviceId") | ||
| request.setValue(config.apiBasicValidationValue, forHTTPHeaderField: config.apiBasicValidationKey) | ||
| request.setValue(accessToken, forHTTPHeaderField: "access-token") | ||
| request.httpBody = try? JSONSerialization.data(withJSONObject: ["page": page, "question": question]) |
There was a problem hiding this comment.
Low — streaming request diverges from HopperAPIPerformAskAIRequest's body handling.
Two parity gaps vs the non-streaming request:
- It always sends
questionin the body, even when empty.HopperAPIPerformAskAIRequestguardsif let question = question, !question.isEmptybefore adding it, so an empty question here changes what the server sees. - It has no
suggestion_idsupport at all, while the non-streaming request accepts one. Given the response change in this same PR exists specifically to handle suggestion answers (bare-stringdata), a suggestion-driven Ask AI cannot use the streaming path. If that's intentional, a note would help; otherwise consider threadingsuggestionIdthrough.
| case "delta": | ||
| guard let content = payload["content"] as? String else { return nil } | ||
| return .delta(text: content) | ||
| case "done", "final": |
There was a problem hiding this comment.
Low — done/final handling drops the terminal event when data is not a JSON object, and ignores a non-string error.
parseFrame returns nil unless the joined data parses as [String: Any]. So a terminal frame whose payload is a plain sentinel (e.g. data: [DONE]) or is empty never produces a .done event, and any run_id/session_id it carried is lost. Separately, the error check payload["error"] as? String only fires for a string; if the backend sends "error": { ... } or a numeric code, it falls through to .done(...) and the failure is reported as a normal completion. Worth confirming the exact shapes the gateway emits for terminal/error frames.
pimfeltkamp
left a comment
There was a problem hiding this comment.
Automated review — 5 findings (details inline):
- Medium —
HopperAPIAskAIStreamingRequest: 2xx error envelope with a non-402 status is decoded as a normal answer and returned as.success(nil), dropping the general-error path the code claims to mirror. - Medium —
HopperAPIPerformAskAIResponse.init(from:)never throws, so malformed/errordatapayloads now decode to an empty-but-successful answer on both the streaming and existing non-streaming paths. - Low/Medium — a stream ending in an
.errorevent still completes with.success(nil); stream failures are only visible viaonEvent. - Low — streaming request always sends
question(even empty) and has nosuggestion_id, diverging fromHopperAPIPerformAskAIRequest. - Low —
done/finalframes are dropped whendataisn't a JSON object, and a non-stringerrorfield is treated as a normal completion.
Wat & waarom
Voegt SSE-streaming toe aan de Ask AI-endpoint zodat de iOS-app antwoorden token-voor-token kan tonen. Onderdeel van het vervangen van Intercom-support door native Ask AI-chat (app-PR volgt/hoort hierbij).
Wijzigingen
performAskAIStreamingopCryptohopperUser— SSE-request met JSON-fallback.AskAIStreamParser— incrementele SSE-parser (delta/tool_result/error events, CRLF-safe).AskAIStreamEvent— event-model.CH_DEVICE_UNAUTHORIZEDbij een 402-respons.application/jsonlabelt i.p.v.text/event-stream(bekende backend-header-bug; app is er nu tegen bestand).Tests
AskAIStreamParserSpectoegevoegd (delta-accumulatie, event-types).Merge-volgorde
Deze SDK-PR moet vóór of samen met de app-PR gemerged worden — de app bouwt lokaal tegen
../cryptohopper-ios-sdk(Podfile:path), maar CI/TestFlight-releases hebben de gepubliceerde SDK nodig.🤖 Generated with Claude Code