[PR #28] [MERGED] refactor(transport): extract shared send() + api/view URL builder #30

New issue

Closed

opened 2026-10-26 22:34:08 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:34:08 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/28
Author: @mattmillerai
Created: 7/4/2026
Status: ✅ Merged
Merged: 7/5/2026
Merged by: @mattmillerai

Base: main ← Head: refactor/transport-send-helper

📝 Commits (2)

a844213 refactor(transport): extract shared send() + api/view URL builder
2964237 fix(transport): bound + clean up the download error-body read; preserve poll retry semantics

📊 Changes

2 files changed (+200 additions, -81 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/Transport.swift (+95 -81)
➕ Tests/ComfySwiftSDKTests/DownloadViewErrorBodyTests.swift (+105 -0)

📄 Description

ELI-5

Six of the Transport request methods each hand-wrote the same little dance: fire the request, translate any transport error, peek at the body on a 400/422 to turn it into a friendly error, then check the status code. This PR pulls that repeated tail into one tiny send(_:) helper and gives the two api/view download methods a shared URL builder — so the plumbing lives in one place instead of six.

Along the way it fixes a real bug: the video download path (downloadViewToTempFile) was the one place that forgot the 400/422 body check, so a rejected video download surfaced a generic .network error instead of the same .serverRejected reason the image path gives you. Now both agree.

What changed

New private func send(_ request:) -> (Data, URLResponse) — wraps session.data(for:) + translate + the 400/422 checkBody guard + checkStatus. The five session.data-based endpoints (uploadImage, submitPrompt, validateAuth, fetchJobStatus, downloadView) now route through it.
New private func viewURL(filename:subfolder:type:) — the identical api/view URL-composition block the two download methods duplicated, now shared (still fails fast on a malformed URL).
Kept per-caller (unchanged by design): applyAuth is still called by each method before send; response decoding stays in each method.
Correctness fix: downloadViewToTempFile keeps its own session.download(for:) call (it streams to a temp file and can't share send), but now mirrors the 400/422 checkBody by reading the small error payload back off disk — closing the inconsistency with downloadView.
Left alone: cancelJob (deliberately swallows errors via try?).

Net: ~60 lines of duplicated HTTP tail removed, two focused helpers added.

Judgment calls

validateAuth and fetchJobStatus now also run the 400/422 checkBody (previously only 3 of the methods did). This is the natural consequence of a single shared helper and is strictly more informative — a 400/422 on those endpoints now surfaces a .serverRejected reason instead of a generic .network(badServerResponse). It never turns an error into a success (on 400/422, checkBody either throws a more specific error or falls through to the same checkStatus failure as before).

Tests

New DownloadViewErrorBodyTests: asserts downloadViewToTempFile now maps a 400 error body to .serverRejected (the closed gap), that downloadView still does the same, and that the temp-file happy path still returns the file on a 200.
Full suite green locally (swift test --no-parallel): 153 tests, 21 suites (2 live smoke tests skipped as designed).

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-swift-sdk/pull/28 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/4/2026 **Status:** ✅ Merged **Merged:** 7/5/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `refactor/transport-send-helper` --- ### 📝 Commits (2) - [`a844213`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/a844213c41678866ae6b9a95bed580018ac0381e) refactor(transport): extract shared send() + api/view URL builder - [`2964237`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/2964237549277fe16aafc55254a71bee81eff956) fix(transport): bound + clean up the download error-body read; preserve poll retry semantics ### 📊 Changes **2 files changed** (+200 additions, -81 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/Transport.swift` (+95 -81) ➕ `Tests/ComfySwiftSDKTests/DownloadViewErrorBodyTests.swift` (+105 -0) </details> ### 📄 Description ## ELI-5 Six of the `Transport` request methods each hand-wrote the same little dance: fire the request, translate any transport error, peek at the body on a 400/422 to turn it into a friendly error, then check the status code. This PR pulls that repeated tail into one tiny `send(_:)` helper and gives the two `api/view` download methods a shared URL builder — so the plumbing lives in one place instead of six. Along the way it fixes a real bug: the **video** download path (`downloadViewToTempFile`) was the one place that *forgot* the 400/422 body check, so a rejected video download surfaced a generic `.network` error instead of the same `.serverRejected` reason the image path gives you. Now both agree. ## What changed - **New `private func send(_ request:) -> (Data, URLResponse)`** — wraps `session.data(for:)` + `translate` + the 400/422 `checkBody` guard + `checkStatus`. The five `session.data`-based endpoints (`uploadImage`, `submitPrompt`, `validateAuth`, `fetchJobStatus`, `downloadView`) now route through it. - **New `private func viewURL(filename:subfolder:type:)`** — the identical `api/view` URL-composition block the two download methods duplicated, now shared (still fails fast on a malformed URL). - **Kept per-caller (unchanged by design):** `applyAuth` is still called by each method *before* `send`; response decoding stays in each method. - **Correctness fix:** `downloadViewToTempFile` keeps its own `session.download(for:)` call (it streams to a temp file and can't share `send`), but now mirrors the 400/422 `checkBody` by reading the small error payload back off disk — closing the inconsistency with `downloadView`. - **Left alone:** `cancelJob` (deliberately swallows errors via `try?`). Net: ~60 lines of duplicated HTTP tail removed, two focused helpers added. ## Judgment calls - **`validateAuth` and `fetchJobStatus` now also run the 400/422 `checkBody`** (previously only 3 of the methods did). This is the natural consequence of a single shared helper and is strictly more informative — a 400/422 on those endpoints now surfaces a `.serverRejected` reason instead of a generic `.network(badServerResponse)`. It never turns an error into a success (on 400/422, `checkBody` either throws a more specific error or falls through to the same `checkStatus` failure as before). ## Tests - New `DownloadViewErrorBodyTests`: asserts `downloadViewToTempFile` now maps a 400 error body to `.serverRejected` (the closed gap), that `downloadView` still does the same, and that the temp-file happy path still returns the file on a 200. - Full suite green locally (`swift test --no-parallel`): 153 tests, 21 suites (2 live smoke tests skipped as designed). --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:34:08 +00:00