[PR #26] [MERGED] refactor(transport): unify node→phase label mapping in shared PhaseLabel #28

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-2350-hoist-phase-label

📝 Commits (1)

15db000 refactor(transport): unify node→phase label mapping in shared PhaseLabel

📊 Changes

3 files changed (+37 additions, -44 deletions)

View changed files

➕ Sources/ComfySwiftSDK/Internal/PhaseLabel.swift (+33 -0)
📝 Sources/ComfySwiftSDK/Internal/PollingFallback.swift (+1 -21)
📝 Sources/ComfySwiftSDK/Internal/WebSocketSession.swift (+3 -23)

📄 Description

ELI-5

The SDK tells you what stage your image job is in ("sampling", "vae_decode", "saving", …). There were two identical copies of the code that turns a node name into one of those labels — one for the live WebSocket path, one for the HTTP polling fallback. Two copies means that if someone later adds a new node type or renames a phase in one place and forgets the other, the same job would report a different phase depending on which transport was active. This PR deletes both copies and replaces them with one shared PhaseLabel.forNode(_:) that both paths call, so they can never drift apart.

What changed

New Sources/ComfySwiftSDK/Internal/PhaseLabel.swift — one internal enum PhaseLabel with static func forNode(_:) holding the node→phase mapping.
WebSocketSession.swift — removed the fileprivate func phaseLabel(for:); its 3 call sites now use PhaseLabel.forNode(_:).
PollingFallback.swift — removed the static func phaseLabel(forNode:); its call site (the failed-node branch of derivePhase) now uses PhaseLabel.forNode(_:). ReattachCoordinator inherits the fix transitively, since it routes through PollingFallback.derivePhase.

Behavior

No behavior change. The mapping logic was moved verbatim — byte-for-byte identical to the two removed copies (same check order, same string literals, same contains/== conditions). It's a pure function with no state.

Testing

swift build — clean.
swift test --no-parallel — 150 tests in 20 suites pass, including the derivePhase phase-emission tests (derivePhase uses node_type from execution_error on failed exercises the PhaseLabel.forNode path and still returns sampling).

Notes

Purely internal refactor — no public API surface changes (PhaseLabel is internal).
Local swift test required pointing SwiftPM at the CommandLineTools Testing.framework (this machine has no full Xcode); CI runs the suite normally.

_{🔄 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/26 **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:** `matt/be-2350-hoist-phase-label` --- ### 📝 Commits (1) - [`15db000`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/15db0009c09aab4d4fc94897c207ea096b3598e9) refactor(transport): unify node→phase label mapping in shared PhaseLabel ### 📊 Changes **3 files changed** (+37 additions, -44 deletions) <details> <summary>View changed files</summary> ➕ `Sources/ComfySwiftSDK/Internal/PhaseLabel.swift` (+33 -0) 📝 `Sources/ComfySwiftSDK/Internal/PollingFallback.swift` (+1 -21) 📝 `Sources/ComfySwiftSDK/Internal/WebSocketSession.swift` (+3 -23) </details> ### 📄 Description ## ELI-5 The SDK tells you what stage your image job is in ("sampling", "vae_decode", "saving", …). There were **two identical copies** of the code that turns a node name into one of those labels — one for the live WebSocket path, one for the HTTP polling fallback. Two copies means that if someone later adds a new node type or renames a phase in one place and forgets the other, the *same job* would report a different phase depending on which transport was active. This PR deletes both copies and replaces them with one shared `PhaseLabel.forNode(_:)` that both paths call, so they can never drift apart. ## What changed - New `Sources/ComfySwiftSDK/Internal/PhaseLabel.swift` — one internal `enum PhaseLabel` with `static func forNode(_:)` holding the node→phase mapping. - `WebSocketSession.swift` — removed the `fileprivate func phaseLabel(for:)`; its 3 call sites now use `PhaseLabel.forNode(_:)`. - `PollingFallback.swift` — removed the `static func phaseLabel(forNode:)`; its call site (the failed-node branch of `derivePhase`) now uses `PhaseLabel.forNode(_:)`. `ReattachCoordinator` inherits the fix transitively, since it routes through `PollingFallback.derivePhase`. ## Behavior **No behavior change.** The mapping logic was moved verbatim — byte-for-byte identical to the two removed copies (same check order, same string literals, same `contains`/`==` conditions). It's a pure function with no state. ## Testing - `swift build` — clean. - `swift test --no-parallel` — **150 tests in 20 suites pass**, including the `derivePhase` phase-emission tests (`derivePhase uses node_type from execution_error on failed` exercises the `PhaseLabel.forNode` path and still returns `sampling`). ## Notes - Purely internal refactor — no public API surface changes (`PhaseLabel` is `internal`). - Local `swift test` required pointing SwiftPM at the CommandLineTools `Testing.framework` (this machine has no full Xcode); CI runs the suite normally. --- <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:07 +00:00