# Phase 2 Round-2 Implementation Review Notes

**Reviewer:** T-2.4/2.5/2.9 Test Agent  
**Date:** 2026-05-15  
**Branches read:**
- `origin/feat/p2-t2-4-modifierbar` — `Sources/UI/Input/` (ModifierState, ModifierBar, PasteSheet)
- `origin/feat/p2-t2-5-session` — `Sources/Core/Sessions/SessionConnection.swift`, `Sources/Core/Persistence/ScrollbackCache.swift`
- `origin/feat/p2-t2-9-push` — `Sources/Core/Push/` (NotificationDelegate, DeviceTokenRegistrar) *(files landed on `feat/p2-t2-5-session`)*

---

## 🔴 Critical Issue: `WebSocketClient.ConnectionState` — Invalid Type Reference

### Location
`Sources/Core/Sessions/SessionConnection.swift`, line:
```swift
@Published public private(set) var connectionState: WebSocketClient.ConnectionState = .disconnected
```

### Problem
`ConnectionState` is declared as a **top-level enum** in `WebSocketClient.swift`:

```swift
// WebSocketClient.swift — top-level scope
public enum ConnectionState: Sendable {
    case disconnected
    case connecting
    case connected
}
```

It is **not** a nested type inside `WebSocketClient`. In Swift, `WebSocketClient.ConnectionState` refers to a type named `ConnectionState` nested inside `WebSocketClient`. Since no such nested type exists, this is a **compile-time error** that will prevent the module from building.

### Fix
Replace `WebSocketClient.ConnectionState` with the bare `ConnectionState`:
```swift
@Published public private(set) var connectionState: ConnectionState = .disconnected
```

The same qualified reference appears implicitly in the `filter`/`sink` chain where `.connected` is compared — those enum literal comparisons work via type inference and are not broken, but the property type declaration is broken.

---

## IC-2.1 Compliance: SessionConnection

The IC-2.1 spec (as summarised in the task + prior worker notes) called for:

| IC-2.1 Requirement | Implementation | Verdict |
|--------------------|---------------|---------|
| `protocol SessionConnection` | Concrete `@MainActor final class SessionConnection` | ⚠️ Deviated — no protocol |
| `state: AnyPublisher<PiState, Never>` | `stateEvents: PassthroughSubject<ServerToClient, Never>` | ⚠️ Deviated — raw ServerToClient frames, not PiState; callers must filter |
| `stream: AnyPublisher<Data, Never>` | `stream: PassthroughSubject<Data, Never>` | ✅ Compatible — PassthroughSubject *is* a Publisher; callers can `.eraseToAnyPublisher()` |
| `resume(from:) async throws` | `resume(from:) async` (no `throws`) | ⚠️ Deviated — errors are logged/swallowed |
| `suspend() async` | `suspend() async` | ✅ Signature matches |

### Verdict: **Partial compliance**

The concrete behaviour (WS lifecycle, scrollback, resume handshake) is faithfully implemented. The deviation from the protocol-first design means downstream consumers are bound to the concrete class, which reduces testability (e.g. `SessionConnection` cannot be mocked for unit tests of a future `TerminalViewModel`). The `stateEvents` deviation is particularly impactful: callers who expected `AnyPublisher<PiState, Never>` must now write a `.compactMap` step to extract `PiState` values from `ServerToClient` frames.

**Recommendation:** Extract a `SessionConnectionProtocol` with the IC-2.1 signatures in a follow-up, or at minimum add a computed `var state: AnyPublisher<PiState, Never>` getter to `SessionConnection` that wraps `stateEvents`.

---

## Swift 6 Issues Found

### 🔴 `WebSocketClient.ConnectionState` — compile error (see above)

### ⚠️ `suspend()` marked `async` without any `await`

```swift
public func suspend() async {
    client?.disconnect()
    client = nil
    cancellables.removeAll()
    connectionState = .disconnected
}
```

All four lines are synchronous. Marking the method `async` does not cause a compile error but:
1. Forces callers to `await suspend()` unnecessarily.
2. Causes a Swift warning: *"No 'async' operations occur within 'async' function"* under `-strict-concurrency=complete` (though as of Swift 5.10/6.0 this is a warning, not an error).

**Fix:** Remove `async` from `suspend()`. Callers on `main` can call it directly; callers off-actor can `Task { @MainActor in … }`.

### ⚠️ Resume-frame send captures `self` in a `Task` after `cancellables` might be cleared

```swift
ws.connectionState
    .filter { $0 == .connected }
    .first()
    .sink { [weak self, weak ws, lastSeq] _ in
        guard let self, let ws else { return }
        Task { @MainActor [self, ws, lastSeq] in
            try? await ws.send(.resume(lastSeq: lastSeq))
            _ = self // silence unused-capture warning
        }
    }
    .store(in: &cancellables)
```

If `suspend()` is called before the `Task` body runs (possible on a context switch), `cancellables.removeAll()` cancels the `AnyCancellable` but the already-spawned `Task` is **not** cancelled — it will still call `ws.send(…)` on the now-detached `ws`. In practice this is harmless (the WS will be disconnected), but it is a subtle race. A `Task` handle stored in the class and cancelled in `suspend()` would be cleaner.

### ✅ `ScrollbackCache: @unchecked Sendable` — Correct

The `DispatchQueue`-based serial queue provides the required mutual exclusion. `@unchecked Sendable` is the right annotation here since `DispatchQueue` itself is `Sendable` but the guarded mutable state is not automatically known to the compiler.

### ✅ `NotificationDelegate` — `nonisolated` bridge callbacks

`UNUserNotificationCenterDelegate` callbacks are correctly declared `nonisolated` with a `Task { @MainActor in … }` hop to access `visibleSessionId`. This is the correct pattern for Swift 6 under strict concurrency.

### ✅ `DeviceTokenRegistrar` — `actor` isolation correct

All mutable state is actor-isolated. `nonisolated static var environment` is a pure value derived at compile time via `#if DEBUG` and is safe from any isolation context.

---

## Naming / API Inconsistencies with T-2.1 Types

### `WebSocketClient.connectionState` — `CurrentValueSubject` vs assumed `AnyPublisher`

`WebSocketClient.connectionState` (T-2.1) is a `CurrentValueSubject<ConnectionState, Never>`, not an `AnyPublisher`. This is correct and fine — `CurrentValueSubject` is a `Publisher`. However, `SessionConnection` subscribes to it via `.filter { $0 == .connected }` which works; just note the type is not erased at the boundary.

### `BinaryFrame.data` — correct usage

`SessionConnection` calls `frame.data` on `BinaryFrame` values emitted by `ws.incomingBinary`. `BinaryFrame.data: Data` exists (T-2.1 definition). ✅

### `WebSocketClientError.notConnected` — correct usage

`SessionConnection.send(_:)` throws `WebSocketClientError.notConnected` — matches the T-2.1 enum case. ✅

### `ClientToServer.resume(lastSeq:)` — correct usage

Used in the resume handshake. T-2.1 defines `case resume(lastSeq: UInt64?)`. ✅

---

## T-2.9 Branch Location

**Note:** The push files (`NotificationDelegate.swift`, `DeviceTokenRegistrar.swift`) were committed on `feat/p2-t2-5-session` (commit `a5c937a`), **not** on a standalone `feat/p2-t2-9-push` branch as expected. When checking out `feat/p2-t2-9-push`, those files are absent. This means:

- PRs for T-2.5 and T-2.9 will contain code from both tasks.
- The `feat/p2-t2-9-push` branch tip contains only earlier work (the test commit `89c27c0`).
- **Action required:** either re-push T-2.9 files to a dedicated branch, or document in the PR that both T-2.5 and T-2.9 are delivered together.

---

## Merge Order Recommendation

| Order | Branch | Reason |
|-------|--------|--------|
| 1 | `feat/p2-t2-1-websocket` *(already on main)* | Defines `ConnectionState`, `BinaryFrame`, `FrameCodec`, `WebSocketClient`, `ServerToClient` — foundation for all other branches |
| 2 | `feat/p2-t2-4-modifierbar` | Pure UI, no dependencies on Sessions or Push layers |
| 3 | `feat/p2-t2-5-session` | Depends on T-2.1 types (`WebSocketClient`, `BinaryFrame`, etc.); also contains T-2.9 files |
| 4 | `feat/p2-tests-2` *(this branch)* | Can merge any time after T-2.1; imports `piRemote` which will contain all sources once T-2.4 and T-2.5 are merged |

**Block on:** the `WebSocketClient.ConnectionState` compile error in `SessionConnection.swift` — this **must** be fixed before `feat/p2-t2-5-session` can be merged without breaking the build.

---

## Summary of Test Files Created

| File | Tests | Coverage |
|------|-------|----------|
| `ModifierStateTests.swift` | 9 | Initial state, toggleCtrl arm/disarm/cycle/isolation, reset clear-all/idempotent/after-many-toggles |
| `ScrollbackCacheTests.swift` | 11 | Append+read round-trip, ordered appends, sizeBytes, cap size/tail/small-chunks, clear zero/empty/then-append, isolation, empty-append no-op |
| `DeviceTokenRegistrarTests.swift` | 8 | nil-before-registration, 2-byte / 1-byte / 4-byte hex, lowercase, UserDefaults persistence, environment valid, environment matches build config |

**Total: 28 unit tests across 3 files.**