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

8.5 KiB
Raw Blame History

Phase 2 Implementation Review Notes

Reviewer: T-2.1/2.2/2.3 Test Agent
Date: 2026-05-15
Branch read: origin/feat/p2-t2-1-websocket, origin/feat/p2-t2-2-pairing, origin/feat/p2-t2-3-terminal

Branch Status at Discovery

Branch Appeared at attempt Files
feat/p2-t2-2-pairing Attempt 1 (~0 min) EMPTY — only base scaffold
feat/p2-t2-3-terminal Attempt 10 (~4.5 min) Terminal + Auth (both T-2.2 and T-2.3 files)
feat/p2-t2-1-websocket Attempt 13 (~6 min) Network layer + partial FrameCodecTests

Implementation Summary

feat/p2-t2-1-websocket — Network Layer

Files:

  • Sources/Core/Network/FrameCodec.swift — BinaryFrame, ClientToServer, ServerToClient, FrameCodec namespace
  • Sources/Core/Network/ResumeCursor.swift — UserDefaults-backed UInt64 cursor
  • Sources/Core/Network/WebSocketClient.swift — @MainActor Starscream wrapper with Combine subjects
  • Tests/CoreTests/FrameCodecTests.swift — Partial test file committed to this branch

Quality: High. IC-1 wire format is faithfully implemented. All JSON field names match the spec. BinaryFrame.decode correctly uses big-endian byte shifting.

feat/p2-t2-2-pairing — EMPTY

This branch contains no implementation files — only the base Xcode scaffold. The pairing code was found on feat/p2-t2-3-terminal instead (see below).

feat/p2-t2-3-terminal — Terminal + Auth (both T-2.2 and T-2.3)

Files:

  • Sources/Core/Auth/Keychain.swift — Generic Codable Keychain wrapper
  • Sources/Core/Auth/Pairing.swift — PairingService with parseQR and exchange
  • Sources/Core/Auth/SidecarCredential.swift — Codable credential model
  • Sources/UI/Pairing/PairingFlowView.swift — SwiftUI pairing flow
  • Sources/UI/Pairing/QRScannerView.swift — AVFoundation QR scanner
  • Sources/UI/Terminal/TerminalTheme.swift — ThemeColor + TerminalTheme + built-ins
  • Sources/UI/Terminal/ThemeStore.swift — @MainActor ObservableObject
  • Sources/UI/Terminal/FontStore.swift — @MainActor ObservableObject
  • Sources/UI/Terminal/TerminalFont.swift — Static font presets
  • Sources/UI/Terminal/TerminalViewController.swift — SwiftTerm wrapper
  • Sources/UI/Terminal/TerminalViewRepresentable.swift — UIViewControllerRepresentable

Quality: High. All types are Sendable-annotated correctly.

IC-1 Compliance Issues

✅ All Field Names Correct

Every JSON field name in FrameCodec.swift matches IC-1:

Frame Type field Extra fields Status
resume "resume" lastSeq (null or UInt64) ✅
key "key" name ✅
keys "keys" data ✅
paste "paste" data ✅
snapshot-request "snapshot-request" none ✅
state (server) "state" value, tool?, ts ✅
snapshot (server) "snapshot" seq, data ✅
session-meta (server) "session-meta" name, description?, createdAt ✅
error (server) "error" code, message ✅

✅ awaiting-input Raw Value Correct

PiState.awaitingInput has rawValue = "awaiting-input" (hyphenated), which is correct per IC-1. This is a common mistake point — the implementation got it right.

✅ lastSeq: null Encoding

ClientToServer.resume(lastSeq: nil) explicitly encodes JSON null (not omitting the key), which is what IC-1 requires. The comment in the code explains this intentional choice.

Swift 6 Concurrency Issues

⚠️ Minor: FrameCodec.encoder/decoder — static mutable-ish singletons

// FrameCodec.swift
private static let encoder: JSONEncoder = { ... }()
private static let decoder = JSONDecoder()

JSONEncoder and JSONDecoder conform to Sendable as of Swift 5.7/Foundation updates. These are static let (not var), so they are initialized once and never mutated. No Swift 6 compile error is expected. However, if the Foundation version on the CI runner predates the Sendable conformance, a warning may appear. Recommend verifying with -strict-concurrency=complete on the build server.

⚠️ WebSocketClient — DelegateAdapter.owner is a non-isolated var

From the partial view of WebSocketClient.swift:

private let delegateAdapter = DelegateAdapter()
// ...
delegateAdapter.owner = self  // set in init

If DelegateAdapter stores owner as weak var owner: WebSocketClient? (not actor-isolated), and Starscream calls the delegate from a background thread, there could be a data race in Swift 6. The comment says callbacks are hopped through Task { @MainActor in … } but owner assignment itself may not be safe. Needs audit when the full file is reviewed during PR.

✅ ThemeStore and FontStore — @MainActor Correct

Both stores are @MainActor final class ObservableObject. Singleton access via .shared from non-actor-isolated contexts will require await in Swift 6. Tests in this repo mark their ThemeStore test functions with @MainActor to handle this correctly.

✅ PairingService — Sendable Struct

PairingService is a struct Sendable, and parseQR is a static function. No concurrency issues.

✅ Keychain — final class Sendable

The implementation declares final class Keychain: Sendable. Since all methods delegate to thread-safe Security framework APIs and there is no mutable stored state, this is correct. No issues.

⚠️ Keychain.load throws .encodingFailed on decode failure

} catch {
    throw KeychainError.encodingFailed  // misleading name
}

The error case KeychainError.encodingFailed is thrown when decoding fails (reading from Keychain, not writing). The name is misleading — it should ideally be .decodingFailed. This is a minor naming issue, not a correctness problem, but callers that pattern-match .encodingFailed might be confused.

Structural Issues

🚨 feat/p2-t2-2-pairing Branch is Empty

The T-2.2 pairing agent pushed a branch with no implementation files. The Auth and Pairing UI code was implemented by the T-2.3 terminal agent instead. This means:

  1. The pairing branch PR will be empty/trivially mergeable.
  2. The terminal branch PR contains code for both T-2.2 and T-2.3.
  3. Recommendation: Credit the terminal agent with T-2.2 work, or have the pairing agent cherry-pick the Auth files before their PR is reviewed.

⚠️ TerminalFont is not Equatable

TerminalFont is Identifiable, Sendable but not Equatable. This makes it harder to assert equality in tests and may cause issues if SwiftUI ForEach needs to diff fonts. Recommend adding Equatable conformance (or at least equality on id).

⚠️ JetBrains Mono font not yet bundled

Documented in the source (T-2.12 deferred). Tests must not assume UIFont(name: "JetBrainsMono-Regular", size:) succeeds — the fallback to system monospace is correct behavior for now.

Recommendations for Merge Order

  1. feat/p2-t2-1-websocket — Merge first. Pure Foundation, no UI dependencies. All IC-1 wire types defined here; T-2.2 and T-2.3 code may eventually import from this layer.

  2. feat/p2-t2-3-terminal — Merge second. Contains both T-2.2 (Auth) and T-2.3 (Terminal) implementations. Depends on nothing from T-2.2's (empty) branch.

  3. feat/p2-t2-2-pairing — Merge last (or close without merge). The branch is empty and the implementation already landed via T-2.3. If the pairing agent re-uses the branch to deliver a QR-scanning integration test or additional pairing UI, it can be rebased on top of T-2.3's merge.

  4. feat/p2-tests (this branch) — Can merge any time after T-2.1; the tests import piRemote which will contain all source files after the three implementation branches are merged.

Test Files Created

File Tests Coverage
FrameCodecTests.swift 21 BinaryFrame.decode, ClientToServer encoding (all 5 cases), ServerToClient decoding (all 4 types), round-trip
ResumeCursorTests.swift 11 Save/load, update overwrites, zero seq, UInt64.max, clear, session isolation
PairingTests.swift 15 Valid URLs, missing params (3 cases), wrong scheme (2), empty/missing port, error type assertions
KeychainTests.swift 7 Round-trip, upsert, missing→notFound, delete clears, delete no-op, production key not touched, generic Codable
ThemeTests.swift 18 ansiColors count, color range, dark≠github, id values, background values, SwiftTerm conversion, ThemeStore select, Codable round-trip

Total: 72 unit tests across 5 files.