From 5535ff77318643dd49fbe176712dda05ffe5f0ee Mon Sep 17 00:00:00 2001 From: daiimus Date: Sun, 8 Mar 2026 21:32:54 -0700 Subject: [PATCH 1/3] refactor(tmux): Swift code quality improvements (findings #35-40) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address 6 low-severity findings from comprehensive code review: closeWindow, renameWindow, selectWindow (moved out of stub section), reset on prepareForReattach — replaces 10 raw string keys across posting and consuming sides Also fixes double-% in paste-buffer command (same bug as S1 finding #3, independently fixed here since this branch predates PR #131). Closes #130 --- .../TmuxSessionManagerTests.swift | 47 ++-- Geistty/Sources/App/GeisttyApp.swift | 19 ++ Geistty/Sources/Ghostty/Ghostty.App.swift | 200 ++++++------------ Geistty/Sources/SSH/SSHSession.swift | 30 +-- Geistty/Sources/SSH/TmuxSessionManager.swift | 46 +++- 5 files changed, 164 insertions(+), 178 deletions(-) diff --git a/Geistty/GeisttyTests/TmuxSessionManagerTests.swift b/Geistty/GeisttyTests/TmuxSessionManagerTests.swift index f4a002a..69e0507 100644 --- a/Geistty/GeisttyTests/TmuxSessionManagerTests.swift +++ b/Geistty/GeisttyTests/TmuxSessionManagerTests.swift @@ -2024,7 +2024,7 @@ extension TmuxSessionManagerTests { /// selectPane() with a non-numeric pane ID should still set focusedPaneId /// but NOT call setActiveTmuxPaneInputOnly (TmuxId.numericPaneId returns nil). @MainActor - func testSelectPaneWithInvalidPaneIdDoesNotCallSetActive() { + func testSelectPaneWithInvalidPaneIdDoesNotSendCommand() { let (mgr, log) = managerWithCommandLog() let mock = MockTmuxSurface() @@ -2034,13 +2034,14 @@ extension TmuxSessionManagerTests { mgr.selectPane("invalid") - // Command still sent (tmux will reject it, but we send it) - XCTAssertEqual(log.commands, ["select-pane -t 'invalid'\n"]) - // focusedPaneId still updated (local state) - XCTAssertEqual(mgr.focusedPaneId, "invalid") - // No setActiveTmuxPaneInputOnly call — numeric conversion failed + // Validation guard rejects malformed pane ID — no command sent + XCTAssertTrue(log.commands.isEmpty, + "No command should be sent for invalid pane ID") + // focusedPaneId unchanged (guard returned early) + XCTAssertEqual(mgr.focusedPaneId, "") + // No setActiveTmuxPaneInputOnly call XCTAssertTrue(mock.setActiveTmuxPaneInputOnlyCalls.isEmpty, - "setActiveTmuxPaneInputOnly should not be called for non-numeric pane IDs") + "setActiveTmuxPaneInputOnly should not be called for invalid pane IDs") } /// selectPane() without a tmuxQuerySurface should still send the command @@ -2247,10 +2248,10 @@ extension TmuxSessionManagerTests { /// selectPane() with a pane ID that has no numeric component should still /// send the tmux command (fire-and-forget) but NOT call setActiveTmuxPaneInputOnly. - /// This is a pre-existing test (testSelectPaneWithInvalidPaneIdDoesNotCallSetActive) + /// This is a pre-existing test (testSelectPaneWithInvalidPaneIdDoesNotSendCommand) /// but we re-verify it in the onPaneTap context. @MainActor - func testOnPaneTapWithMalformedPaneIdSendsCommandOnly() { + func testOnPaneTapWithMalformedPaneIdIsRejected() { let (mgr, log) = managerWithCommandLog() let mock = MockTmuxSurface() @@ -2258,14 +2259,14 @@ extension TmuxSessionManagerTests { mgr.tmuxQuerySurfaceOverride = mock #endif - // Simulate tap with a pane ID that can't be parsed as numeric + // Simulate tap with a pane ID that fails validation let onPaneTap = { mgr.selectPane("invalid") } onPaneTap() - XCTAssertEqual(log.commands, ["select-pane -t 'invalid'\n"], - "Command should still be sent even with invalid pane ID") + XCTAssertTrue(log.commands.isEmpty, + "No command should be sent for invalid pane ID") XCTAssertTrue(mock.setActiveTmuxPaneInputOnlyCalls.isEmpty, - "setActiveTmuxPaneInputOnly should NOT be called for non-numeric pane ID") + "setActiveTmuxPaneInputOnly should NOT be called for invalid pane ID") } } @@ -3375,6 +3376,8 @@ extension TmuxSessionManagerTests { #if DEBUG mgr.setPendingOutputForTesting(["%15": [Data([0x41])]]) #endif + mgr.handleSessionRenamed(name: "my-session") + XCTAssertEqual(mgr.sessionName, "my-session") mgr.prepareForReattach() @@ -3382,6 +3385,7 @@ extension TmuxSessionManagerTests { XCTAssertEqual(mgr.connectionState, .disconnected) XCTAssertTrue(mgr.pendingOutput.isEmpty) XCTAssertNil(mgr.currentSession) + XCTAssertEqual(mgr.sessionName, "") XCTAssertTrue(mgr.windows.isEmpty) XCTAssertFalse(mgr.viewerReady) // focusedWindowId and focusedPaneId are preserved for UI continuity @@ -4086,7 +4090,7 @@ extension TmuxSessionManagerTests { // Set clipboard content UIPasteboard.general.string = "clipboard text" - // Set focused pane (with % prefix, as in production) + // Set focused pane (must use %N format to pass validation) mgr.setFocusedPane("%5") mgr.pasteTmuxBuffer() @@ -4929,12 +4933,23 @@ extension TmuxSessionManagerTests { } @MainActor - func testHandleSessionRenamedDoesNotCrash() { + func testHandleSessionRenamedUpdatesSessionName() { let mgr = TmuxSessionManager() + // Initially empty + XCTAssertEqual(mgr.sessionName, "") + + // Updates to new name mgr.handleSessionRenamed(name: "my-session") - mgr.handleSessionRenamed(name: "") + XCTAssertEqual(mgr.sessionName, "my-session") + + // Updates to name with special characters mgr.handleSessionRenamed(name: "session with spaces and 'quotes'") + XCTAssertEqual(mgr.sessionName, "session with spaces and 'quotes'") + + // Handles empty rename (clears name) + mgr.handleSessionRenamed(name: "") + XCTAssertEqual(mgr.sessionName, "") } @MainActor diff --git a/Geistty/Sources/App/GeisttyApp.swift b/Geistty/Sources/App/GeisttyApp.swift index 55187ad..647a3c6 100644 --- a/Geistty/Sources/App/GeisttyApp.swift +++ b/Geistty/Sources/App/GeisttyApp.swift @@ -282,6 +282,25 @@ extension Notification.Name { static let showTmuxSessions = Notification.Name("showTmuxSessions") } +// MARK: - Typed Keys for tmux Notification userInfo + +/// Typed constants for tmux notification `userInfo` dictionary keys. +/// Eliminates raw string literals scattered across posting (Ghostty.App.swift) +/// and consuming (SSHSession.swift) code. All tmux notifications use these +/// keys to pass payload data through NotificationCenter. +enum TmuxNotificationKey { + static let windowCount = "windowCount" + static let paneCount = "paneCount" + static let reason = "reason" + static let content = "content" + static let isError = "isError" + static let windowId = "windowId" + static let text = "text" + static let name = "name" + static let paneId = "paneId" + static let value = "value" +} + /// Global application state @MainActor class AppState: ObservableObject { diff --git a/Geistty/Sources/Ghostty/Ghostty.App.swift b/Geistty/Sources/Ghostty/Ghostty.App.swift index e79c67a..1ea2847 100644 --- a/Geistty/Sources/Ghostty/Ghostty.App.swift +++ b/Geistty/Sources/Ghostty/Ghostty.App.swift @@ -190,6 +190,27 @@ extension Ghostty { // MARK: - Runtime Callbacks + /// Extract the SurfaceView from a ghostty_surface_t's userdata pointer. + /// Returns nil if the surface has no userdata set. + private static func surfaceView(from surface: ghostty_surface_t) -> SurfaceView? { + ghostty_surface_userdata(surface).map { + Unmanaged.fromOpaque($0).takeUnretainedValue() + } + } + + /// Decode a C data/len pair into a Swift String (UTF-8). + /// Returns an empty string if data is nil or len is 0. + private static func decodePayload(data: UnsafePointer?, len: Int) -> String { + guard len > 0, let ptr = data else { return "" } + return String( + decoding: UnsafeRawBufferPointer( + start: UnsafeRawPointer(ptr), + count: len + ), + as: UTF8.self + ) + } + private static func wakeup(_ userdata: UnsafeMutableRawPointer?) { guard let userdata = userdata else { return } let app = Unmanaged.fromOpaque(userdata).takeUnretainedValue() @@ -211,8 +232,7 @@ extension Ghostty { let titleData = action.action.set_title guard let titlePtr = titleData.title else { return false } let title = String(cString: titlePtr) - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { DispatchQueue.main.async { surfaceView.title = title } @@ -239,8 +259,7 @@ extension Ghostty { // Find the SurfaceView associated with this surface // The surface userdata points to the SurfaceView - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { DispatchQueue.main.async { surfaceView.updateScrollIndicator( total: scrollbar.total, @@ -263,8 +282,7 @@ extension Ghostty { let linkData = action.action.mouse_over_link - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { DispatchQueue.main.async { if linkData.len > 0, let urlPtr = linkData.url { let urlData = Data(bytes: urlPtr, count: linkData.len) @@ -315,8 +333,7 @@ extension Ghostty { let pwdData = action.action.pwd - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { DispatchQueue.main.async { if let pwdPtr = pwdData.pwd { let pwdString = String(cString: pwdPtr) @@ -336,8 +353,7 @@ extension Ghostty { let cellSizeData = action.action.cell_size - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { DispatchQueue.main.async { // IMPORTANT: cellSizeData is in backing pixels (physical pixels on retina). // SwiftUI's GeometryReader returns points (logical units). @@ -362,8 +378,7 @@ extension Ghostty { let shape = action.action.mouse_shape - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { DispatchQueue.main.async { surfaceView.currentMouseShape = shape } @@ -384,8 +399,7 @@ extension Ghostty { let health = action.action.renderer_health - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { DispatchQueue.main.async { surfaceView.healthy = (health == GHOSTTY_RENDERER_HEALTH_HEALTHY) if health != GHOSTTY_RENDERER_HEALTH_HEALTHY { @@ -442,8 +456,7 @@ extension Ghostty { let searchData = action.action.start_search - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { DispatchQueue.main.async { // Initialize search state with the initial needle from the action let initialNeedle: String @@ -471,8 +484,7 @@ extension Ghostty { return false } - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { DispatchQueue.main.async { surfaceView.searchState = nil logger.debug("🔍 Search ended") @@ -489,8 +501,7 @@ extension Ghostty { let totalData = action.action.search_total - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { let total: UInt? = totalData.total >= 0 ? UInt(totalData.total) : nil DispatchQueue.main.async { surfaceView.searchState?.total = total @@ -508,8 +519,7 @@ extension Ghostty { let selectedData = action.action.search_selected - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { let selected: UInt? = selectedData.selected >= 0 ? UInt(selectedData.selected) : nil DispatchQueue.main.async { surfaceView.searchState?.selected = selected @@ -527,8 +537,7 @@ extension Ghostty { let keyTableData = action.action.key_table - if let userdata = ghostty_surface_userdata(surface) { - let surfaceView = Unmanaged.fromOpaque(userdata).takeUnretainedValue() + if let surfaceView = surfaceView(from: surface) { DispatchQueue.main.async { switch keyTableData.tag { case GHOSTTY_KEY_TABLE_ACTIVATE: @@ -584,9 +593,7 @@ extension Ghostty { // by identity (notification.object as? SurfaceView === self.ghosttySurface). // Posting the raw ghostty_surface_t would fail the as? cast, bypassing // the multi-surface identity guard. - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) // Post synchronously — we're already on main thread (via tick()), // and eliminating the async hop reduces the window between viewer @@ -595,8 +602,8 @@ extension Ghostty { name: .tmuxStateChanged, object: surfaceView, userInfo: [ - "windowCount": tmuxState.window_count, - "paneCount": tmuxState.pane_count + TmuxNotificationKey.windowCount: tmuxState.window_count, + TmuxNotificationKey.paneCount: tmuxState.pane_count ] ) return true @@ -624,9 +631,7 @@ extension Ghostty { // Extract the SurfaceView from userdata so observers can filter // by identity (notification.object as? SurfaceView === self.ghosttySurface). - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) // Post synchronously — already on main thread via tick(). // Forward the reason so observers can differentiate voluntary @@ -634,7 +639,7 @@ extension Ghostty { NotificationCenter.default.post( name: .tmuxExited, object: surfaceView, - userInfo: ["reason": reason] + userInfo: [TmuxNotificationKey.reason: reason] ) return true @@ -651,9 +656,7 @@ extension Ghostty { // Extract the SurfaceView from userdata so observers can filter // by identity (notification.object as? SurfaceView === self.ghosttySurface). - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) // Post synchronously — already on main thread via tick(). // This ensures activateFirstTmuxPane() runs immediately, @@ -672,33 +675,20 @@ extension Ghostty { } let resp = action.action.tmux_command_response - let content: String - if resp.len > 0, let ptr = resp.data { - content = String( - decoding: UnsafeRawBufferPointer( - start: UnsafeRawPointer(ptr), - count: resp.len - ), - as: UTF8.self - ) - } else { - content = "" - } + let content = decodePayload(data: resp.data, len: resp.len) logger.info("tmux command response: \(resp.is_error ? "ERROR" : "OK") len=\(resp.len)") // Extract the SurfaceView from userdata so observers can filter // by identity (notification.object as? SurfaceView === self.ghosttySurface). - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) NotificationCenter.default.post( name: .tmuxCommandResponse, object: surfaceView, userInfo: [ - "content": content, - "isError": resp.is_error + TmuxNotificationKey.content: content, + TmuxNotificationKey.isError: resp.is_error ] ) return true @@ -719,15 +709,13 @@ extension Ghostty { // by identity (notification.object as? SurfaceView === self.ghosttySurface). // Posting the raw ghostty_surface_t would fail the as? cast, bypassing // the multi-surface identity guard. - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) // Post synchronously — already on main thread via tick(). NotificationCenter.default.post( name: .tmuxActiveWindowChanged, object: surfaceView, - userInfo: ["windowId": windowId] + userInfo: [TmuxNotificationKey.windowId: windowId] ) return true @@ -738,18 +726,7 @@ extension Ghostty { } let msg = action.action.tmux_message - let text: String - if msg.len > 0, let ptr = msg.data { - text = String( - decoding: UnsafeRawBufferPointer( - start: UnsafeRawPointer(ptr), - count: msg.len - ), - as: UTF8.self - ) - } else { - text = "" - } + let text = decodePayload(data: msg.data, len: msg.len) logger.info("tmux message: len=\(msg.len)") @@ -757,14 +734,12 @@ extension Ghostty { // by identity (notification.object as? SurfaceView === self.ghosttySurface). // Posting the raw ghostty_surface_t would fail the as? cast, bypassing // the multi-surface identity guard. - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) NotificationCenter.default.post( name: .tmuxMessage, object: surfaceView, - userInfo: ["text": text] + userInfo: [TmuxNotificationKey.text: text] ) return true @@ -775,29 +750,16 @@ extension Ghostty { } let payload = action.action.tmux_paste_buffer_changed - let name: String - if payload.len > 0, let ptr = payload.data { - name = String( - decoding: UnsafeRawBufferPointer( - start: UnsafeRawPointer(ptr), - count: payload.len - ), - as: UTF8.self - ) - } else { - name = "" - } + let name = decodePayload(data: payload.data, len: payload.len) logger.info("tmux paste buffer changed: len=\(payload.len)") - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) NotificationCenter.default.post( name: .tmuxPasteBufferChanged, object: surfaceView, - userInfo: ["name": name] + userInfo: [TmuxNotificationKey.name: name] ) return true @@ -808,29 +770,16 @@ extension Ghostty { } let payload = action.action.tmux_paste_buffer_deleted - let name: String - if payload.len > 0, let ptr = payload.data { - name = String( - decoding: UnsafeRawBufferPointer( - start: UnsafeRawPointer(ptr), - count: payload.len - ), - as: UTF8.self - ) - } else { - name = "" - } + let name = decodePayload(data: payload.data, len: payload.len) logger.info("tmux paste buffer deleted: len=\(payload.len)") - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) NotificationCenter.default.post( name: .tmuxPasteBufferDeleted, object: surfaceView, - userInfo: ["name": name] + userInfo: [TmuxNotificationKey.name: name] ) return true @@ -842,9 +791,7 @@ extension Ghostty { logger.info("tmux sessions changed") - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) NotificationCenter.default.post( name: .tmuxSessionsChanged, @@ -861,14 +808,12 @@ extension Ghostty { let paneId = action.action.tmux_pane_mode_changed.pane_id logger.info("tmux pane mode changed: %\(paneId)") - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) NotificationCenter.default.post( name: .tmuxPaneModeChanged, object: surfaceView, - userInfo: ["paneId": paneId] + userInfo: [TmuxNotificationKey.paneId: paneId] ) return true @@ -879,29 +824,16 @@ extension Ghostty { } let payload = action.action.tmux_session_renamed - let name: String - if payload.len > 0, let ptr = payload.data { - name = String( - decoding: UnsafeRawBufferPointer( - start: UnsafeRawPointer(ptr), - count: payload.len - ), - as: UTF8.self - ) - } else { - name = "" - } + let name = decodePayload(data: payload.data, len: payload.len) logger.info("tmux session renamed: len=\(payload.len)") - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) NotificationCenter.default.post( name: .tmuxSessionRenamed, object: surfaceView, - userInfo: ["name": name] + userInfo: [TmuxNotificationKey.name: name] ) return true @@ -914,14 +846,12 @@ extension Ghostty { let payload = action.action.tmux_focused_pane_changed logger.info("tmux focused pane changed: @\(payload.window_id) %\(payload.pane_id)") - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) NotificationCenter.default.post( name: .tmuxFocusedPaneChanged, object: surfaceView, - userInfo: ["windowId": payload.window_id, "paneId": payload.pane_id] + userInfo: [TmuxNotificationKey.windowId: payload.window_id, TmuxNotificationKey.paneId: payload.pane_id] ) return true @@ -941,14 +871,12 @@ extension Ghostty { logger.debug("tmux subscription changed: name=\(name)") - let surfaceView: SurfaceView? = ghostty_surface_userdata(surface).map { - Unmanaged.fromOpaque($0).takeUnretainedValue() - } + let surfaceView = surfaceView(from: surface) NotificationCenter.default.post( name: .tmuxSubscriptionChanged, object: surfaceView, - userInfo: ["name": name, "value": value] + userInfo: [TmuxNotificationKey.name: name, TmuxNotificationKey.value: value] ) return true diff --git a/Geistty/Sources/SSH/SSHSession.swift b/Geistty/Sources/SSH/SSHSession.swift index 320f834..6d1bd32 100644 --- a/Geistty/Sources/SSH/SSHSession.swift +++ b/Geistty/Sources/SSH/SSHSession.swift @@ -499,8 +499,8 @@ class SSHSession: ObservableObject, Identifiable { guard notifSurface === self.ghosttySurface else { return } } - let windowCount = notification.userInfo?["windowCount"] as? UInt ?? 0 - let paneCount = notification.userInfo?["paneCount"] as? UInt ?? 0 + let windowCount = notification.userInfo?[TmuxNotificationKey.windowCount] as? UInt ?? 0 + let paneCount = notification.userInfo?[TmuxNotificationKey.paneCount] as? UInt ?? 0 logger.info("tmux state changed: \(windowCount) windows, \(paneCount) panes, current state=\(self.controlModeState)") @@ -544,7 +544,7 @@ class SSHSession: ObservableObject, Identifiable { // Extract the exit reason forwarded from Ghostty's tmux viewer. // Known reasons: "detached" (voluntary), "server-exited" (crash), // "" (empty, e.g. session destroyed). - let reason = notification.userInfo?["reason"] as? String ?? "" + let reason = notification.userInfo?[TmuxNotificationKey.reason] as? String ?? "" logger.info("tmux control mode exited via TMUX_EXIT, reason: \(reason.isEmpty ? "(none)" : reason)") self.controlModeState = .inactive @@ -610,8 +610,8 @@ class SSHSession: ObservableObject, Identifiable { guard notifSurface === self.ghosttySurface else { return } } - let content = notification.userInfo?["content"] as? String ?? "" - let isError = notification.userInfo?["isError"] as? Bool ?? false + let content = notification.userInfo?[TmuxNotificationKey.content] as? String ?? "" + let isError = notification.userInfo?[TmuxNotificationKey.isError] as? Bool ?? false self.tmuxSessionManager?.handleCommandResponse(content: content, isError: isError) } @@ -628,7 +628,7 @@ class SSHSession: ObservableObject, Identifiable { guard notifSurface === self.ghosttySurface else { return } } - guard let windowId = notification.userInfo?["windowId"] as? UInt32 else { + guard let windowId = notification.userInfo?[TmuxNotificationKey.windowId] as? UInt32 else { return } @@ -647,7 +647,7 @@ class SSHSession: ObservableObject, Identifiable { guard notifSurface === self.ghosttySurface else { return } } - guard let text = notification.userInfo?["text"] as? String else { + guard let text = notification.userInfo?[TmuxNotificationKey.text] as? String else { return } @@ -665,7 +665,7 @@ class SSHSession: ObservableObject, Identifiable { guard notifSurface === self.ghosttySurface else { return } } - guard let name = notification.userInfo?["name"] as? String else { + guard let name = notification.userInfo?[TmuxNotificationKey.name] as? String else { return } @@ -683,7 +683,7 @@ class SSHSession: ObservableObject, Identifiable { guard notifSurface === self.ghosttySurface else { return } } - guard let name = notification.userInfo?["name"] as? String else { + guard let name = notification.userInfo?[TmuxNotificationKey.name] as? String else { return } @@ -715,7 +715,7 @@ class SSHSession: ObservableObject, Identifiable { guard notifSurface === self.ghosttySurface else { return } } - guard let paneId = notification.userInfo?["paneId"] as? UInt32 else { + guard let paneId = notification.userInfo?[TmuxNotificationKey.paneId] as? UInt32 else { return } @@ -733,7 +733,7 @@ class SSHSession: ObservableObject, Identifiable { guard notifSurface === self.ghosttySurface else { return } } - guard let name = notification.userInfo?["name"] as? String else { + guard let name = notification.userInfo?[TmuxNotificationKey.name] as? String else { return } @@ -751,8 +751,8 @@ class SSHSession: ObservableObject, Identifiable { guard notifSurface === self.ghosttySurface else { return } } - guard let windowId = notification.userInfo?["windowId"] as? UInt32, - let paneId = notification.userInfo?["paneId"] as? UInt32 else { + guard let windowId = notification.userInfo?[TmuxNotificationKey.windowId] as? UInt32, + let paneId = notification.userInfo?[TmuxNotificationKey.paneId] as? UInt32 else { return } @@ -770,8 +770,8 @@ class SSHSession: ObservableObject, Identifiable { guard notifSurface === self.ghosttySurface else { return } } - guard let name = notification.userInfo?["name"] as? String, - let value = notification.userInfo?["value"] as? String else { + guard let name = notification.userInfo?[TmuxNotificationKey.name] as? String, + let value = notification.userInfo?[TmuxNotificationKey.value] as? String else { return } diff --git a/Geistty/Sources/SSH/TmuxSessionManager.swift b/Geistty/Sources/SSH/TmuxSessionManager.swift index a55a186..d77aee2 100644 --- a/Geistty/Sources/SSH/TmuxSessionManager.swift +++ b/Geistty/Sources/SSH/TmuxSessionManager.swift @@ -52,6 +52,9 @@ class TmuxSessionManager: ObservableObject { /// Currently focused window ID (empty until first session-changed/layout event) @Published private(set) var focusedWindowId: String = "" + /// Name of the attached tmux session, updated on `%session-renamed` notifications + @Published private(set) var sessionName: String = "" + /// Connection state (legacy bool for compatibility) @Published private(set) var isConnected: Bool = false @@ -413,6 +416,7 @@ class TmuxSessionManager: ObservableObject { // on the new connection. We keep focusedWindowId and focusedPaneId so the // UI doesn't flash to a different window/pane during the brief reconnect. currentSession = nil + sessionName = "" sessions.removeAll() windows.removeAll() windowSplitTrees.removeAll() @@ -1213,7 +1217,10 @@ class TmuxSessionManager: ObservableObject { /// Close a specific window by ID func closeWindow(windowId: String) { - // Window IDs are system-generated (@N format) — no user input to escape + guard TmuxId.isValidWindowId(windowId) else { + logger.warning("closeWindow: invalid window ID '\(windowId)'") + return + } sendCommandFireAndForget("kill-window -t '\(windowId)'") } @@ -1226,6 +1233,10 @@ class TmuxSessionManager: ObservableObject { /// Rename a specific window by ID func renameWindow(windowId: String, name: String) { + guard TmuxId.isValidWindowId(windowId) else { + logger.warning("renameWindow: invalid window ID '\(windowId)'") + return + } // Escape single quotes in name to prevent command injection let safeName = name.replacingOccurrences(of: "'", with: "'\\''") sendCommandFireAndForget("rename-window -t '\(windowId)' '\(safeName)'") @@ -1233,7 +1244,11 @@ class TmuxSessionManager: ObservableObject { /// Select a window by ID func selectWindow(_ windowId: String) { - logger.info("📑 selectWindow: \(windowId)") + guard TmuxId.isValidWindowId(windowId) else { + logger.warning("selectWindow: invalid window ID '\(windowId)'") + return + } + logger.info("selectWindow: \(windowId)") logger.info("📑 Current windows: \(windows.keys.sorted().joined(separator: ", "))") logger.info("📑 Current split trees: \(windowSplitTrees.keys.sorted().joined(separator: ", "))") @@ -1341,6 +1356,10 @@ class TmuxSessionManager: ObservableObject { /// Select a pane by ID func selectPane(_ paneId: String) { + guard TmuxId.isValidPaneId(paneId) else { + logger.warning("selectPane: invalid pane ID '\(paneId)'") + return + } sendCommandFireAndForget("select-pane -t '\(paneId)'") focusedPaneId = paneId @@ -1577,6 +1596,18 @@ class TmuxSessionManager: ObservableObject { handler(content, isError) } + /// Handle a tmux `%session-renamed` notification from the Zig viewer. + /// Fired when the attached session is renamed. Updates the published + /// `sessionName` property so the UI can reflect the new name. + func handleSessionRenamed(name: String) { + logger.info("tmux session renamed: \(name)") + sessionName = name + } + + // MARK: - Stub Handlers (log-only, future work) + // These handlers receive tmux notifications but only log them. + // They are kept as extension points for future features. + /// Handle a tmux `%message` notification from the Zig viewer. /// These are informational messages from the tmux server (e.g., session /// created/destroyed notices). Currently log-only; future work may surface @@ -1613,13 +1644,6 @@ class TmuxSessionManager: ObservableObject { logger.info("tmux pane mode changed: %\(paneId)") } - /// Handle a tmux `%session-renamed` notification from the Zig viewer. - /// Fired when the attached session is renamed. - /// Currently log-only; future work may update the session title in the UI. - func handleSessionRenamed(name: String) { - logger.info("tmux session renamed: \(name)") - } - /// Handle a tmux `%window-pane-changed` notification from the Zig viewer. /// Fired when the focused pane within a window changes (e.g., via select-pane). /// If the window matches our focused window, update input routing to the new pane @@ -1690,8 +1714,8 @@ class TmuxSessionManager: ObservableObject { return } - guard !focusedPaneId.isEmpty else { - logger.warning("pasteTmuxBuffer: no focused pane ID — cannot target paste-buffer") + guard TmuxId.isValidPaneId(focusedPaneId) else { + logger.warning("pasteTmuxBuffer: invalid focused pane ID '\(focusedPaneId)' — cannot target paste-buffer") return } From e1b8d219aa64273dfb71273cbff82707e3cd2265 Mon Sep 17 00:00:00 2001 From: daiimus Date: Sun, 8 Mar 2026 22:18:31 -0700 Subject: [PATCH 2/3] fix(tmux): address Copilot round 2 review comments on PR #132 1. Move handleFocusedPaneChanged and handleSubscriptionChanged above the Stub Handlers MARK since they do real work (update state), not just log. 2. Clear sessionName in both controlModeExited() and cleanup() for consistency with other session metadata resets. 3. Add tests for window ID validation guards: closeWindow, renameWindow, and selectWindow all reject malformed IDs without sending commands. --- .../TmuxSessionManagerTests.swift | 45 ++++++++++++++ Geistty/Sources/SSH/TmuxSessionManager.swift | 62 ++++++++++--------- 2 files changed, 77 insertions(+), 30 deletions(-) diff --git a/Geistty/GeisttyTests/TmuxSessionManagerTests.swift b/Geistty/GeisttyTests/TmuxSessionManagerTests.swift index 69e0507..c8b023c 100644 --- a/Geistty/GeisttyTests/TmuxSessionManagerTests.swift +++ b/Geistty/GeisttyTests/TmuxSessionManagerTests.swift @@ -2044,6 +2044,51 @@ extension TmuxSessionManagerTests { "setActiveTmuxPaneInputOnly should not be called for invalid pane IDs") } + /// closeWindow() with an invalid window ID should not send any command. + @MainActor + func testCloseWindowWithInvalidWindowIdDoesNotSendCommand() { + let (mgr, log) = managerWithCommandLog() + + mgr.closeWindow(windowId: "invalid") + XCTAssertTrue(log.commands.isEmpty, + "No command should be sent for invalid window ID") + + mgr.closeWindow(windowId: "2") + XCTAssertTrue(log.commands.isEmpty, + "Window ID without @ prefix should be rejected") + } + + /// renameWindow(windowId:name:) with an invalid window ID should not send any command. + @MainActor + func testRenameWindowWithInvalidWindowIdDoesNotSendCommand() { + let (mgr, log) = managerWithCommandLog() + + mgr.renameWindow(windowId: "bad", name: "test") + XCTAssertTrue(log.commands.isEmpty, + "No command should be sent for invalid window ID") + + mgr.renameWindow(windowId: "3", name: "test") + XCTAssertTrue(log.commands.isEmpty, + "Window ID without @ prefix should be rejected") + } + + /// selectWindow() with an invalid window ID should not send any command + /// or update focusedWindowId. + @MainActor + func testSelectWindowWithInvalidWindowIdDoesNotSendCommand() { + let (mgr, log) = managerWithCommandLog() + + mgr.selectWindow("notawindow") + XCTAssertTrue(log.commands.isEmpty, + "No command should be sent for invalid window ID") + XCTAssertEqual(mgr.focusedWindowId, "", + "focusedWindowId should not be updated for invalid window ID") + + mgr.selectWindow("0") + XCTAssertTrue(log.commands.isEmpty, + "Window ID without @ prefix should be rejected") + } + /// selectPane() without a tmuxQuerySurface should still send the command /// and update focusedPaneId (graceful nil handling). @MainActor diff --git a/Geistty/Sources/SSH/TmuxSessionManager.swift b/Geistty/Sources/SSH/TmuxSessionManager.swift index d77aee2..f0ed0fa 100644 --- a/Geistty/Sources/SSH/TmuxSessionManager.swift +++ b/Geistty/Sources/SSH/TmuxSessionManager.swift @@ -317,6 +317,7 @@ class TmuxSessionManager: ObservableObject { availableSessions.removeAll() tmuxOptions.removeAll() pendingResponseHandlers.removeAll() + sessionName = "" // Clear output buffers and deferred surface creation pendingOutput.removeAll() @@ -1604,6 +1605,36 @@ class TmuxSessionManager: ObservableObject { sessionName = name } + /// Handle a tmux `%window-pane-changed` notification from the Zig viewer. + /// Fired when the focused pane within a window changes (e.g., via select-pane). + /// If the window matches our focused window, update input routing to the new pane + /// so that keystrokes are sent to the correct pane via send-keys. + func handleFocusedPaneChanged(windowId: UInt32, paneId: UInt32) { + logger.info("tmux focused pane changed: @\(windowId) %\(paneId)") + + // Only update focus if this is our currently focused window. + // focusedWindowId has "@" prefix (e.g. "@0"), so format windowId to match. + if "@\(windowId)" == focusedWindowId { + setFocusedPane("%\(paneId)") + } + } + + /// Handle a tmux `%subscription-changed` notification from the Zig viewer. + /// Fired when a format subscription value changes (registered via refresh-client -B). + /// The name identifies the subscription and the value is the new format expansion. + func handleSubscriptionChanged(name: String, value: String) { + logger.debug("tmux subscription changed: \(name), valueLength=\(value.count)") + + switch name { + case "status_left": + statusLeft = value + case "status_right": + statusRight = value + default: + break + } + } + // MARK: - Stub Handlers (log-only, future work) // These handlers receive tmux notifications but only log them. // They are kept as extension points for future features. @@ -1644,36 +1675,6 @@ class TmuxSessionManager: ObservableObject { logger.info("tmux pane mode changed: %\(paneId)") } - /// Handle a tmux `%window-pane-changed` notification from the Zig viewer. - /// Fired when the focused pane within a window changes (e.g., via select-pane). - /// If the window matches our focused window, update input routing to the new pane - /// so that keystrokes are sent to the correct pane via send-keys. - func handleFocusedPaneChanged(windowId: UInt32, paneId: UInt32) { - logger.info("tmux focused pane changed: @\(windowId) %\(paneId)") - - // Only update focus if this is our currently focused window. - // focusedWindowId has "@" prefix (e.g. "@0"), so format windowId to match. - if "@\(windowId)" == focusedWindowId { - setFocusedPane("%\(paneId)") - } - } - - /// Handle a tmux `%subscription-changed` notification from the Zig viewer. - /// Fired when a format subscription value changes (registered via refresh-client -B). - /// The name identifies the subscription and the value is the new format expansion. - func handleSubscriptionChanged(name: String, value: String) { - logger.debug("tmux subscription changed: \(name), valueLength=\(value.count)") - - switch name { - case "status_left": - statusLeft = value - case "status_right": - statusRight = value - default: - break - } - } - /// Copy the tmux paste buffer to the iOS clipboard. /// Sends `show-buffer` through the Zig viewer's command queue and /// writes the response content to `UIPasteboard.general` on success. @@ -2080,6 +2081,7 @@ class TmuxSessionManager: ObservableObject { connectionState = .disconnected focusedPaneId = "" focusedWindowId = "" + sessionName = "" // Reset resize tracking lastResizeCols = 0 From ea7a2697e4481e925b5b47e44167efe8c78f89cd Mon Sep 17 00:00:00 2001 From: daiimus Date: Sun, 8 Mar 2026 22:22:20 -0700 Subject: [PATCH 3/3] fix(test): update stale doc comment for selectPane validation test --- Geistty/GeisttyTests/TmuxSessionManagerTests.swift | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Geistty/GeisttyTests/TmuxSessionManagerTests.swift b/Geistty/GeisttyTests/TmuxSessionManagerTests.swift index c8b023c..deb7d33 100644 --- a/Geistty/GeisttyTests/TmuxSessionManagerTests.swift +++ b/Geistty/GeisttyTests/TmuxSessionManagerTests.swift @@ -2021,8 +2021,9 @@ extension TmuxSessionManagerTests { XCTAssertEqual(mock.setActiveTmuxPaneInputOnlyCalls, [7]) } - /// selectPane() with a non-numeric pane ID should still set focusedPaneId - /// but NOT call setActiveTmuxPaneInputOnly (TmuxId.numericPaneId returns nil). + /// selectPane() with an invalid pane ID should reject the call: + /// no command is sent, focusedPaneId is unchanged, and + /// setActiveTmuxPaneInputOnly is not called. @MainActor func testSelectPaneWithInvalidPaneIdDoesNotSendCommand() { let (mgr, log) = managerWithCommandLog()