jay
/
pi-remote-ios
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
pi-remote-ios/Tests/CoreTests/REVIEW_NOTES_2.md

8.8 KiB
Raw Blame History

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:

@Published public private(set) var connectionState: WebSocketClient.ConnectionState = .disconnected

Problem

ConnectionState is declared as a top-level enum in WebSocketClient.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:

@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

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

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.