5. Add Cog playground API client

[anish-sahoo]

• cog api client for config, health, schema, predictions, sse, and file conversion
• includes http/sse/api unit tests
• cog-playground stack: 1. Add Go playground server and handlers #3109, 2. Add playground frontend scaffold and early CI #3108, 3. Add core playground types and utilities #3114, 4. Add shared playground UI and JSON editor #3110/5. Add Cog playground API client #3106, 6. Add playground connection feature #3105/7. Add playground input controls and validation #3111/8. Add playground prediction controls and output #3107, 9. Wire up Cog playground and documentation #3113, 10. Compose playground app and add browser e2e #3112

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR adds a proxy-backed Cog API client with bounded HTTP/SSE parsing and browser file conversion, plus comprehensive tests.

I found a couple of actionable issues:

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR adds a proxy-backed Cog API client with bounded HTTP/SSE parsing and browser file conversion, plus comprehensive tests.

I found a couple of actionable issues:

1. Resource leak in readSSE (sse.ts:37-40): If reader.cancel() throws, reader.releaseLock() is skipped because the exception propagates out of the finally block, leaving the stream locked. I posted a suggestion to wrap cancel() in a try/catch.

2. Missing size bound in fileToDataURI (files.ts:2): The PR description mentions "bounded HTTP/SSE parsing and browser file conversion," but fileToDataURI has no size limit. Loading a multi-gigabyte file into memory as a base64 data URI can crash the browser tab. I posted a suggestion to add a maxSize parameter with a default bound.

github run

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR adds a proxy-backed Cog API client with bounded HTTP/SSE parsing and browser file conversion, plus comprehensive tests. The previously reported issues (SSE cleanup and file size bounds) have been addressed.

I found one remaining issue:

1. Reader lock double-release in sse.ts and http.ts (high severity)
   Per the Streams spec, ReadableStreamDefaultReader.cancel() releases the reader lock synchronously before its returned promise settles. Calling reader.releaseLock() afterward unconditionally throws a TypeError because the reader is no longer active.

   • In sse.ts, this means any early termination of an SSE stream (e.g., the consumer breaks out of a for await...of loop) causes iterator.return() to reject with a TypeError instead of completing cleanly.
   • In http.ts, when a response exceeds the streaming size limit, reader.cancel() releases the lock and then releaseLock() in the finally block throws, masking the intended "Response body exceeds..." error with an unrelated TypeError.

   The existing test "releases an SSE reader lock when cancellation fails" uses a mock that does not faithfully implement the spec (it never releases the lock), so it gives a false sense of security. The test should also be updated to verify correct behavior with a spec-compliant reader.

   I've posted suggestions for both files.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR adds a proxy-backed Cog API client with bounded HTTP/SSE parsing and browser file conversion, plus comprehensive tests.

I found one remaining issue:

1. Reader lock double-release in sse.ts and http.ts (high severity) — Per the Streams spec, ReadableStreamDefaultReader.cancel() releases the reader lock synchronously. The current finally blocks then call reader.releaseLock() unconditionally, which throws a TypeError because the reader is no longer active. In sse.ts this breaks early stream termination; in http.ts it masks the size-limit error. I posted suggestions for both files, and the existing SSE cleanup test should be updated to use a spec-compliant mock.

github run

[anish-sahoo]

Follow-up disposition: validated against the Streams Standard and a local Web Streams probe. cancel() on an explicitly acquired reader does not release its lock, so the releaseLock() calls in sse.ts and http.ts are required and do not mask the intended errors. Both attached review threads are resolved.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR adds a proxy-backed Cog API client with bounded HTTP/SSE parsing and browser file conversion, plus comprehensive tests.

I found one remaining issue:

1. Missing unit tests for cancel and config methods (low severity) — These two public methods on CogApi have no test coverage. cancel involves URL construction and error handling, while config fetches and parses local playground configuration. Adding basic tests would prevent regressions for these entrypoints.

[ask-bonk]

I've completed the review of PR #3106. The batch review has been posted successfully.

Summary: I found one actionable issue — missing unit tests for the cancel and config methods on CogApi. I posted a suggestion comment with example tests for both methods to playground/src/services/cog/CogApi.test.ts.

The previously discussed issues (SSE cleanup and file size bounds) were already addressed in earlier review rounds. The author also provided a clear technical response regarding the reader lock behavior, which I accepted as resolved.

github run

[markphelps]

I spotted two type-boundary issues in the new API client. Both are small structurally, but worth fixing before callers start depending on these public types.

[ask-bonk]

LGTM

github run