feat: add terminal encoding switcher for SSH sessions (#261)#272
feat: add terminal encoding switcher for SSH sessions (#261)#272
Conversation
Allow users to switch between UTF-8 and GB18030 encoding mid-session via a toolbar popover, fixing garbled output when viewing mixed-encoding logs on remote servers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7342b4a872
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
electron/bridges/sshBridge.cjs
Outdated
| stream.on("data", (data) => { | ||
| bufferData(data.toString("utf8")); | ||
| const enc = sessionEncodings.get(sessionId) || "utf-8"; | ||
| const decoded = enc === "utf-8" ? data.toString("utf8") : iconv.decode(data, enc); |
There was a problem hiding this comment.
This decodes each incoming chunk with iconv.decode(data, enc) independently, which corrupts multibyte encodings (especially GB18030) when a character is split across packet boundaries; those split bytes are common on SSH streams and will intermittently render replacement characters in terminal output. Because this feature is specifically for non-UTF8 decoding, the stream should keep decoder state per session/stream (e.g., iconv.getDecoder(enc) and incremental writes) before calling bufferData.
Useful? React with 👍 / 👎.
| </Button> | ||
| )} | ||
|
|
||
| {!hidesSftp && onSetTerminalEncoding && ( |
There was a problem hiding this comment.
The encoding control is rendered for every non-local/non-serial terminal, so it appears in Telnet and Mosh sessions too. In this commit, selecting it calls the SSH-only IPC handler (netcatty:ssh:setEncoding), but those non-SSH paths are handled in terminalBridge.cjs and do not read sessionEncodings, so the toggle does nothing in those sessions and presents a misleading broken setting. Please gate this UI/action to SSH sessions only.
Useful? React with 👍 / 👎.
- Replace per-chunk iconv.decode() with stateful iconv.getDecoder() to handle multibyte characters split across packet boundaries (P1) - Reset decoders when encoding is switched mid-session - Gate encoding popover to SSH sessions only, excluding Telnet/Mosh (P2) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1eb19a739
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const isLocalTerminal = host?.protocol === 'local' || host?.id?.startsWith('local-'); | ||
| const isSerialTerminal = host?.protocol === 'serial' || host?.id?.startsWith('serial-'); | ||
| const isSSHSession = !isLocalTerminal && !isSerialTerminal && host?.protocol !== 'telnet' && host?.protocol !== 'mosh'; |
There was a problem hiding this comment.
isSSHSession currently treats any non-local/non-serial/non-telnet host as SSH, but this codebase routes Mosh via host.moshEnabled while keeping host.protocol as 'ssh' (see the Mosh branch in terminal session startup and connection dialog logic), so the new encoding popover is shown for Mosh tabs. In that scenario, selecting an encoding calls the SSH IPC handler, but Mosh output decoding is handled elsewhere, so the control is a misleading no-op for users and can leave UI/backend state inconsistent; this should also gate on !host?.moshEnabled (or another definitive SSH-session check).
Useful? React with 👍 / 👎.
Mosh sessions keep host.protocol as 'ssh' but set host.moshEnabled, so also gate encoding popover on !host?.moshEnabled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64a1b8de3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (sessionRef.current) { | ||
| setSessionEncoding(sessionRef.current, encoding); |
There was a problem hiding this comment.
When the user changes encoding while the terminal is still connecting, sessionRef.current is null so this handler only updates React state and skips the IPC call; once the SSH session is established there is no follow-up sync, so the toolbar can show GB18030 while the backend keeps decoding as UTF-8. This causes incorrect decoding for initial output unless the user toggles encoding again after connect.
Useful? React with 👍 / 👎.
If the user changes encoding while still connecting, sessionRef is null so the IPC call is skipped. Now updateStatus syncs the encoding to the backend when status transitions to 'connected' and encoding is non-default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f38515d383
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
components/Terminal.tsx
Outdated
| onStatusChange?.(sessionId, next); | ||
| // Sync terminal encoding to backend when session becomes connected, | ||
| // in case user changed encoding while still connecting | ||
| if (next === "connected" && sessionRef.current && terminalEncodingRef.current !== "utf-8") { |
There was a problem hiding this comment.
The connect-time sync only runs when terminalEncodingRef.current !== "utf-8", but the SSH bridge pre-seeds GB hosts to gb18030; if a user switches to UTF-8 while the session is still connecting (before sessionRef is available), this condition skips the only post-connect sync and the backend keeps decoding as GB18030 while the UI shows UTF-8. This produces incorrect decoding for subsequent output on GB-configured hosts unless the user toggles encoding again after connection.
Useful? React with 👍 / 👎.
|
|
||
| const isLocalTerminal = host?.protocol === 'local' || host?.id?.startsWith('local-'); | ||
| const isSerialTerminal = host?.protocol === 'serial' || host?.id?.startsWith('serial-'); | ||
| const isSSHSession = !isLocalTerminal && !isSerialTerminal && host?.protocol !== 'telnet' && host?.protocol !== 'mosh' && !host?.moshEnabled; |
There was a problem hiding this comment.
The isSSHSession check treats any non-local/non-serial/non-telnet/non-mosh host as SSH, but this app routes hostname === "localhost" sessions through startLocal in Terminal.tsx; for those sessions the new encoding picker is shown even though setSessionEncoding only affects the SSH bridge, so the control is non-functional and misleading for localhost profiles configured as SSH.
Useful? React with 👍 / 👎.
- Remove utf-8 guard from connect-time sync so GB-preseeded hosts that get switched to UTF-8 during connect are synced correctly - Exclude hostname==='localhost' sessions from encoding popover since they route through startLocal, not the SSH bridge Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb80b8f60c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const isLocalTerminal = host?.protocol === 'local' || host?.id?.startsWith('local-'); | ||
| const isSerialTerminal = host?.protocol === 'serial' || host?.id?.startsWith('serial-'); | ||
| const isSSHSession = !isLocalTerminal && !isSerialTerminal && host?.protocol !== 'telnet' && host?.protocol !== 'mosh' && !host?.moshEnabled && host?.hostname !== 'localhost'; |
There was a problem hiding this comment.
The new isSSHSession predicate excludes hosts where hostname === 'localhost', so SSH sessions to localhost never show the encoding switcher even though they still use the SSH decoding path and may require non-UTF-8 output handling. Since local/serial sessions are already filtered separately, this extra hostname guard suppresses the feature for a valid SSH use case.
Useful? React with 👍 / 👎.
components/Terminal.tsx
Outdated
| if (next === "connected" && sessionRef.current) { | ||
| setSessionEncoding(sessionRef.current, terminalEncodingRef.current); |
There was a problem hiding this comment.
This call executes whenever any terminal transitions to connected, but updateStatus is used by SSH, Telnet, Mosh, Local, and Serial starters in components/terminal/runtime/createTerminalSessionStarters.ts. That means non-SSH sessions still invoke the SSH setEncoding IPC handler, which stores state in sessionEncodings/sessionDecoders (electron/bridges/sshBridge.cjs) without a matching non-SSH cleanup path, causing stale per-session decoder entries to accumulate.
Useful? React with 👍 / 👎.
- Remove hostname==='localhost' check since SSH to localhost is valid and local protocol sessions are already filtered by isLocalTerminal - Restrict updateStatus encoding sync to SSH sessions only, preventing stale decoder entries from accumulating for non-SSH session types Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c10076866
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (sessionRef.current) { | ||
| setSessionEncoding(sessionRef.current, encoding); |
There was a problem hiding this comment.
If the user changes encoding while the session is still connecting, this branch drops the backend update because sessionRef.current is still null, and the later catch-up in updateStatus("connected") happens too late. In components/terminal/runtime/createTerminalSessionStarters.ts (attachSessionLifecycle), connected is set only inside the first onSessionData callback (around lines 132-143), so that first chunk is already decoded with the old/default encoding and can render garbled text on non-UTF-8 hosts.
Useful? React with 👍 / 👎.
Move encoding sync from updateStatus("connected") to a new
onSessionAttached callback in attachSessionToTerminal, which fires
right after sessionRef is set but before the data listener is
registered. This ensures the first data chunk is decoded correctly
even if the user changed encoding during connection.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 237b4404dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sessionEncodings.set(sessionId, enc); | ||
| // Reset stateful decoders so new data uses the updated encoding | ||
| resetSessionDecoders(sessionId); |
There was a problem hiding this comment.
setSessionEncoding claims to act on an active session, but it unconditionally writes sessionEncodings/sessionDecoders without checking whether sessionId is still present in sessions. In practice this can be called after a tab has disconnected (the encoding UI remains available), returning { ok: true } for a dead session and creating map entries that never get cleaned up, which is both misleading to callers and a memory leak over time.
Useful? React with 👍 / 👎.
Check that sessionId exists in the sessions map before writing to sessionEncodings/sessionDecoders, preventing stale map entries and misleading ok:true responses for disconnected sessions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5cb590e0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const isLocalTerminal = host?.protocol === 'local' || host?.id?.startsWith('local-'); | ||
| const isSerialTerminal = host?.protocol === 'serial' || host?.id?.startsWith('serial-'); | ||
| const isSSHSession = !isLocalTerminal && !isSerialTerminal && host?.protocol !== 'telnet' && host?.protocol !== 'mosh' && !host?.moshEnabled; |
There was a problem hiding this comment.
Terminal.tsx starts a local session when host.hostname === "localhost" (see the startup branch around startLocal), but this predicate marks those same hosts as SSH because it only checks protocol/id flags. As a result, localhost local shells still show the encoding popover and trigger setSessionEncoding, which has no effect on their output decoding; this contradicts the expected behavior that local terminals should not expose SSH-only encoding controls.
Useful? React with 👍 / 👎.
electron/bridges/sshBridge.cjs
Outdated
| if (!sessions || !sessions.has(sessionId)) { | ||
| return { ok: false, encoding: encoding || "utf-8" }; | ||
| } |
There was a problem hiding this comment.
This handler accepts any ID present in the shared sessions map, but that map also contains local/telnet/serial sessions, so non-SSH calls can incorrectly return { ok: true }. Because only SSH stream-close paths clear sessionEncodings/sessionDecoders, these false-positive updates can also leave stale decoder state behind for non-SSH sessions. Validate the session shape/type (SSH stream/conn) before accepting encoding changes.
Useful? React with 👍 / 👎.
- Check session.stream in setSessionEncoding to reject non-SSH sessions that share the sessions map (local/telnet/serial) - Add hostname !== 'localhost' guard to isSSHSession in toolbar and onSessionAttached, since localhost routes through startLocal Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
iconv-litebased on selected encodingLanguagespopover toTerminalToolbar(matching SFTP toolbar pattern) to switch between UTF-8 and GB18030host.charsetwhen configured as a GB variant; only new data after switching uses the new encodingTest plan
echo "测试"— should display correctlyhost.charsetwhen configuredCloses #261
🤖 Generated with Claude Code