Add integrated terminal dock - #431
Conversation
SergeSerb2
left a comment
There was a problem hiding this comment.
Terminal dock feature adds SwiftTerm dependency and integrated terminal UI. Critical issues block merge:
-
Missing Notification name extension —
.sergeCodeToggleTerminalDockis not defined. Need a Notification.Name extension in a Support file. -
Missing import + type definitions — TerminalStore.swift references types like
TerminalSessionSnapshot,TerminalMetadataStreamEvent,TerminalAttachStreamEvent,TerminalSessionStatus,TerminalSummary,TerminalCloseInput,TerminalClearInput,TerminalWriteInput,TerminalResizeInput,TerminalOpenInput,TerminalAttachInput,TerminalRestartInputthat must be generated from the Effect schemas inpackages/contracts/src/terminal.ts. T3Kit should expose these as Swift types. -
Diff truncated — TerminalStore.swift implementation is cut off around line 620. Cannot review the full attach/stream logic, input worker lifecycle, or error handling.
-
ThreadDetailView signature change — ContentView.swift adds
terminalPresented: $showTerminalbinding, but ThreadDetailView.swift implementation is not shown. Need to verify it accepts and uses this parameter. -
Default terminal ID — TerminalStore uses hardcoded
"term-1"but should importDEFAULT_TERMINAL_IDfrom contracts. -
Mock backend — MockTerminalRuntime uses manual
now()timestamps — will break when reset sinceDate()is unavailable in Effect scripts. Use a time-source injected at init instead.
Blocking verification: Run pnpm run verify to catch Swift build errors on missing types and TerminalStore compilation.
SurgeCode auto-review · model=codex/gpt-5.6-luna · head=1e837e30bcc5
| Button("Toggle Terminal") { | ||
| NotificationCenter.default.post( | ||
| name: .sergeCodeToggleTerminalDock, object: nil) | ||
| } |
There was a problem hiding this comment.
blocking: .sergeCodeToggleTerminalDock Notification.Name is not defined. Create a Support file (e.g., Notification+Extensions.swift) with:
extension Notification.Name {
static let sergeCodeToggleTerminalDock = Notification.Name("sergeCodeToggleTerminalDock")
}| @@ -0,0 +1,620 @@ | |||
| import Foundation | |||
| import Observation | |||
| import T3Kit | |||
There was a problem hiding this comment.
blocking: Missing imports for Terminal types. Add:
import T3Kit // for TerminalSessionSnapshot, TerminalMetadataStreamEvent, etc.Verify T3Kit exports all 12 types referenced below. If not, codegen the Swift bindings from packages/contracts/src/terminal.ts schemas.
| public func gridSize(for target: TerminalTarget) -> TerminalGridSize { | ||
| gridSizes[target] ?? .fallback | ||
| } | ||
|
|
There was a problem hiding this comment.
important: Uses hardcoded defaultTerminalID = "term-1" but should import from contracts. Use T3Kit's DEFAULT_TERMINAL_ID constant instead to stay in sync.
| Task { await self?.removeMetadataListener(id) } | ||
| } | ||
| metadataListeners[id] = continuation | ||
| continuation.yield(.snapshot(sessions.values.map(summary).sorted { |
There was a problem hiding this comment.
important: now() helper calls Date() directly — will crash in Effect/testing contexts that forbid Date/random. Inject a time source at MockTerminalRuntime init or use a static test clock.
| Group { | ||
| if let thread = multi.selectedThread { | ||
| ThreadDetailView(model: multi.activeModel, scenery: scenery, thread: thread) | ||
| ThreadDetailView( |
There was a problem hiding this comment.
important: ThreadDetailView receives new terminalPresented: $showTerminal binding, but the view signature and implementation are not shown in the diff. Verify ThreadDetailView accepts a @Binding<Bool> parameter and uses it to render the terminal dock.
| } | ||
| return String(describing: error) | ||
| } | ||
| } |
There was a problem hiding this comment.
blocking: Diff truncated mid-implementation. TerminalStore.swift is cut off around line 620. Cannot review the remaining stream handling, input-worker lifecycle, error propagation, or cleanup logic. Regenerate the full diff and verify the file is complete.
SergeSerb2
left a comment
There was a problem hiding this comment.
Integrated Terminal Dock — Code Review
Strengths:
- Bounded memory management: render chunks capped at 256 or 128KB per session
- Correct actor/MainActor isolation for concurrency safety
displayEquivalentnow includesworktreePath(fixes silent upsert bug)- SwiftTerm pinned to 1.11.2 with clear explanation of Metal shader issue
- BackendService extension uses optional-method pattern (non-breaking)
Gaps:
- No tests for TerminalStore (~620 lines, complex state machine)
- Empty PR body — should document scope, testing, and justify terminal feature
- MockTerminalRuntime is feature-complete but untested in isolation
- RecordGridSize resize debounce uses magic constant 75ms; document why
Minor concerns:
- swift-argument-parser in Package.resolved but not Package.swift (likely SwiftTerm transitive dependency; verify with
pnpm why swift-argument-parser) - Cmd+J shortcut: verify no conflict with existing shortcuts in CommandPalette
Before merge:
- Add PR description explaining feature scope
- Add size label (required per CLAUDE.md)
- Run
pnpm run verify --allto ensure no regressions
Could not anchor
- important
apps/mac/Sources/SurgeCodeMac/Model/TerminalStore.swift— New 620-line file managing terminal sessions. This is complex state machinery (attach/detach streams, action tracking, resize debounce, input queue). Consider adding unit tests for: session lifecycle, render chunk eviction, error propagation, and activeTarget transitions. MockBackend's MockTerminalRuntime would be a good test subject.
SurgeCode auto-review · model=codex/gpt-5.6-luna · head=955c2ca564ba
| id == other.id | ||
| && projectID == other.projectID | ||
| && worktreePath == other.worktreePath | ||
| && title == other.title |
There was a problem hiding this comment.
info: ✓ Correct: worktreePath addition ensures threads with different worktrees are considered different. This fixes the silent-upsert bug documented in CLAUDE.md (displayEquivalent field trap).
| gridSizes[target] = size | ||
| resizeTasks[target]?.cancel() | ||
| guard state(for: target).isRunning else { return } | ||
| resizeTasks[target] = Task { [weak self] in |
There was a problem hiding this comment.
nit: Magic constant 75 milliseconds for resize debounce. Add a comment explaining why this value (e.g., "debounce rapid resizes from window drag events" or reference macOS event coalescing window).
| .package(url: "https://github.com/FluidInference/FluidAudio.git", from: "0.15.0"), | ||
| .package(url: "https://github.com/swiftlang/swift-markdown.git", from: "0.5.0"), | ||
| .package(url: "https://github.com/sparkle-project/Sparkle.git", from: "2.6.0") | ||
| .package(url: "https://github.com/sparkle-project/Sparkle.git", from: "2.6.0"), |
There was a problem hiding this comment.
info: SwiftTerm pinned to 1.11.2 with clear explanation: Metal shader compilation breaks Command Line Tools builds. Good. Verify swift-argument-parser appearing in Package.resolved is a transitive dependency of SwiftTerm: pnpm why swift-argument-parser.
| CommandMenu("Terminal") { | ||
| Button("Toggle Terminal") { | ||
| NotificationCenter.default.post( | ||
| name: .sergeCodeToggleTerminalDock, object: nil) |
There was a problem hiding this comment.
nit: Cmd+J shortcut for terminal toggle. Verify this doesn't collide with CommandPalette or other global shortcuts. Check CommandMenu handlers and any conflicting keybindings.
The run-profile popover's effort control was a draggable slider: one level visible at a time, a drag-then-release gesture, and end labels that clipped. Swap it for a segmented picker where every level is on screen at once with its ramp color, symbol, and label, and one click commits the choice. The active segment's pill slides between stops via matched geometry, arrow keys and VoiceOver adjustable actions still step the ramp, and the cost-warning backdrop/badge are unchanged. The slider's pure step/haptic math moves onto EffortLevelStyle as steppedIndex/stepHaptic; the track layout math (and its tests) goes away with the slider.
…nter Add worktree control center
Add macOS usage and cost center
Replace reasoning effort slider with segmented picker
SergeSerb2
left a comment
There was a problem hiding this comment.
Summary
This PR adds terminal session support and worktree control capabilities to SurgeCode via new SwiftTerm dependency and BackendService protocol methods. Core infrastructure is sound (dependency pinning rationale is clear, error handling in worktree operations is defensive), but three critical gaps block compilation:
TerminalStoreclass referenced but not defined in this diffNotification.Name.sergeCodeToggleTerminalDocknotification identifier not definedThreadDetailViewparameter addition but implementation not shown
These aren't design flaws—they're just missing from the diff. Verify the implementation files exist in the branch (or this PR should include them).
Secondary: displayEquivalent add of worktreePath check is correct per memory (displayEquivalent-field-trap), but watch for memoization test gaps.
SurgeCode auto-review · model=codex/gpt-5.6-luna · head=792e5648641c
| /// Multiple device models may share one controller so ASR stays a single | ||
| /// process-wide resource. | ||
| public let dictation: DictationController | ||
| /// Per-backend terminal sessions and bounded history buffers. |
There was a problem hiding this comment.
blocking: TerminalStore class is instantiated here but not defined anywhere in this diff. Either:
- Add
TerminalStoredefinition to this PR (likely a new file) - Verify the class exists in
SurgeCodeMacalready and was imported
Without the definition, this will not compile.
| } | ||
| .onReceive( | ||
| NotificationCenter.default.publisher(for: .sergeCodeToggleTerminalDock) | ||
| ) { _ in |
There was a problem hiding this comment.
blocking: Posting to .sergeCodeToggleTerminalDock notification but the name is not defined. Add to Notification.Name extension:
extension Notification.Name {
static let sergeCodeToggleTerminalDock = Notification.Name("sergeCodeToggleTerminalDock")
}| Group { | ||
| if let thread = multi.selectedThread { | ||
| ThreadDetailView(model: multi.activeModel, scenery: scenery, thread: thread) | ||
| ThreadDetailView( |
There was a problem hiding this comment.
blocking: ThreadDetailView is instantiated with new terminalPresented: $showTerminal parameter, but the view's implementation is not shown in this diff. Verify ThreadDetailView init accepts this binding parameter, or add its signature change to this PR.
| id == other.id | ||
| && projectID == other.projectID | ||
| && worktreePath == other.worktreePath | ||
| && title == other.title |
There was a problem hiding this comment.
important: displayEquivalent now includes worktreePath check (added twice at lines 412 and 445). This is correct per memory (displayEquivalent-field-trap), but ensure the memoization test in ChatThread.test.swift (or wherever it lives) covers this new field to prevent silent upsert bugs. Watch for test gaps when running pnpm run verify.
Summary
Why
Builds, tests, development servers, and recovery commands now stay beside the active agent instead of requiring a separate terminal app.
Verification
pnpm run test:mac --filter Terminalpnpm run verifypnpm run verify --all