refactor(tmux): Swift code quality improvements (findings #35-40)#132
refactor(tmux): Swift code quality improvements (findings #35-40)#132
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses 6 low-severity findings (#35-40) from a comprehensive code review, improving Swift code quality across the tmux integration layer. It extracts common patterns into helpers, adds input validation, centralizes notification keys, and makes a log-only handler functional.
Changes:
- Extracts
surfaceView(from:)anddecodePayload(data:len:)helpers inGhostty.App.swiftto eliminate 25+ duplicated patterns, addsTmuxIdvalidation guards on 5 functions, and introducesTmuxNotificationKeyenum for typed notificationuserInfokeys - Promotes
handleSessionRenamedfrom a log-only stub to store the session name in a new@Published sessionNameproperty, with reset inprepareForReattach - Fixes the double-
%bug in thepaste-buffercommand (same as PR #131 finding #3)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Geistty/Sources/Ghostty/Ghostty.App.swift | Extracts surfaceView(from:) and decodePayload(data:len:) helpers; replaces all inline patterns; uses TmuxNotificationKey for all notification userInfo keys |
| Geistty/Sources/SSH/TmuxSessionManager.swift | Adds TmuxId validation guards to closeWindow, renameWindow, selectWindow, selectPane, pasteTmuxBuffer; promotes handleSessionRenamed with sessionName property; fixes double-% in paste-buffer command |
| Geistty/Sources/SSH/SSHSession.swift | Replaces all raw string notification userInfo key accesses with TmuxNotificationKey constants |
| Geistty/Sources/App/GeisttyApp.swift | Defines TmuxNotificationKey enum with 10 typed constants |
| Geistty/GeisttyTests/TmuxSessionManagerTests.swift | Updates tests for new validation behavior, sessionName storage, and %N-format pane IDs in paste tests |
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
db4d607 to
5535ff7
Compare
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Geistty/GeisttyTests/TmuxSessionManagerTests.swift:2251
- The doc comment is stale: it says "should still send the tmux command (fire-and-forget)" but the test now verifies the opposite — the validation guard rejects the invalid pane ID and no command is sent (line 2266). The comment should be updated to match the new behavior, e.g., "selectPane() with an invalid pane ID should be rejected by the validation guard."
/// Tapping the same pane twice should still send the command and route.
/// (Idempotent — tmux handles duplicate select-pane gracefully.)
| /// 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. |
There was a problem hiding this comment.
The doc comment at lines 2295-2296 still describes the old pre-guard behavior: "selectPane() with a pane ID that has no numeric component should still send the tmux command (fire-and-forget) but NOT call setActiveTmuxPaneInputOnly." However, the test now verifies the opposite — the guard rejects the invalid pane ID, no command is sent, and setActiveTmuxPaneInputOnly is not called. The comment should be updated to reflect the rejection behavior, e.g., "selectPane() with a pane ID that fails validation should reject the call: no command is sent and setActiveTmuxPaneInputOnly is not called."
Summary
Addresses 6 low-severity findings (#35-40) from the comprehensive code review, improving Swift code quality across the tmux integration layer.
// MARK: - Stub Handlersannotation for discoverabilitysurfaceView(from:)helper — replaces 25 identicalghostty_surface_userdataextraction patternsdecodePayload(data:len:)helper — replaces 5 identicalUnsafeRawBufferPointerconversion blocksTmuxIdformat validation guards toselectPane,pasteTmuxBuffer,closeWindow(windowId:),renameWindow(windowId:),selectWindow— prevents malformed IDs from reaching tmuxhandleSessionRenamednow stores the name in@Published sessionName(moved out of stub section); property reset onprepareForReattachTmuxNotificationKeyenum with 10 typed constants — replaces raw string keys across posting (Ghostty.App.swift) and consuming (SSHSession.swift) sidesAlso fixes the double-
%bug inpaste-buffercommand (same as S1 finding #3, independently fixed here since this branch predates PR #131).Testing
All tests pass (
./ci.sh test). Updated tests:testSelectPaneWithInvalidPaneIdDoesNotSendCommand— validates new guard behaviortestOnPaneTapWithMalformedPaneIdIsRejected— validates guard in tap contexttestHandleSessionRenamedUpdatesSessionName— verifies property storagetestPrepareForReattachClearsTransientState— verifiessessionNamereset%Npane IDsAI Disclosure
This PR was authored with AI assistance (Claude claude-opus-4.6 via OpenCode).
Closes #130