Add iOS-parity web client#9
Conversation
Web client (Sources/RelayServer/WebResources): full 4-tab UI mirroring the iOS app — Workspaces (cards + per-workspace surfaces, create/rename/close), Active (terminal mirror with surface chips, composer, keypad, file attach), Inbox (event-frame notifications), Settings (guide/status/unpair). Sizes the terminal to the surface grid (CJK-aware width), pins to bottom, and self-heals revoked bearer tokens by re-registering on WS upgrade rejection. Also bundles in-progress relay/iOS/script/doc updates already in the tree.
📝 WalkthroughWalkthroughThis PR changes the relay server's default port from 4399 to 80, converts the macOS deployment from a per-user launchd agent to a root system daemon, adds a singleton instance lock, and embeds a browser-based web client (HTML/CSS/JS) served directly by the relay's HTTP server. Documentation, install/uninstall scripts, and iOS app defaults are updated accordingly. ChangesRelay Server Core
Deployment Scripts
Documentation and iOS Defaults
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant AppJS as app.js
participant Routes as Routes.serveStatic
participant HTTPServer
participant CmuxRelay
Browser->>HTTPServer: GET /
HTTPServer->>Routes: handle(request)
Routes->>Routes: serveStatic(path)
Routes-->>HTTPServer: HTTPResponseLite(index.html, headers)
HTTPServer-->>Browser: 200 OK + Content-Type + Cache headers
Browser->>AppJS: load /app.js
AppJS->>HTTPServer: POST /v1/devices/me/register
HTTPServer-->>AppJS: bearer token + device id
AppJS->>CmuxRelay: WebSocket connect (bearer.<token>)
CmuxRelay-->>AppJS: hello ack
AppJS->>CmuxRelay: RPC surface.subscribe
CmuxRelay-->>AppJS: screen.full push
AppJS->>Browser: render terminal
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
Sources/RelayServer/CmuxRelay.swift (1)
228-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePID file not truncated before write; stale bytes can persist.
Opening with
O_CREAT | O_RDWR(no truncation) and thenwrite()-ing the PID leaves trailing bytes from a previous, longer PID string if the new PID is shorter, corrupting the debug info in the lock file.♻️ Proposed fix
// Record the PID for debugging — `ps`/`launchctl` can confirm ownership. let pidStr = "\(ProcessInfo.processInfo.processIdentifier)\n" + ftruncate(fd, 0) _ = pidStr.withCString { write(fd, $0, pidStr.utf8.count) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/RelayServer/CmuxRelay.swift` around lines 228 - 231, The PID write in CmuxRelay’s PID-file handling leaves stale bytes because the file is opened without truncation before `write()`. Update the PID-file setup in the same block that writes `pidStr` to ensure the file is truncated or otherwise overwritten from zero before writing the new PID, so shorter PIDs don’t leave trailing characters behind.Sources/RelayServer/Routes.swift (1)
217-224: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider ETag/Last-Modified instead of full no-store.
no-cache, no-store, must-revalidateforces a full re-download ofapp.js/style.csson every request. Adding a content-hash or mtime-basedETag/Last-Modifiedwould let the browser send conditional requests, preserving freshness after reinstalls while avoiding always re-transferring the full body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/RelayServer/Routes.swift` around lines 217 - 224, The static asset response in Routes.swift is overusing "Cache-Control: no-cache, no-store, must-revalidate", which prevents conditional caching for app.js/style.css. Update the response-building path in the asset-serving logic to use an ETag and/or Last-Modified derived from the bundled asset content or timestamp, and relax the cache policy so browsers can revalidate instead of always re-downloading. Keep the stale-after-reinstall safety behavior, but make the headers in the asset response conditional-cache friendly in the same code that builds the .ok response.Sources/RelayServer/WebResources/index.html (2)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
user-scalable=nodisables pinch-zoom.Combined with
maximum-scale=1, this blocks users who need to zoom text for readability (WCAG 1.4.4). Given this is a chat/terminal-heavy UI, consider allowing zoom.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/RelayServer/WebResources/index.html` at line 5, The viewport meta tag in index.html is preventing users from zooming because it includes user-scalable=no and maximum-scale=1. Update the viewport settings to allow pinch-zoom and text scaling while keeping the existing layout behavior, and verify the change in the meta tag used by the app shell.
2-2: 🎯 Functional Correctness | 🔵 TrivialMixed English/Korean UI copy.
<html lang="ko">is set, and several instructional/empty-state strings are Korean (e.g. Line 59-60, Line 134-135, the tutorial steps at Lines 149-153, Line 175), while every other label, button, and tab in the file is English. This is inconsistent and will confuse non-Korean-speaking users on exactly the screens (tutorial, empty states) meant to guide first-time setup.Confirm the intended primary locale and either translate the whole UI or move the Korean strings behind proper i18n rather than leaving a partial mix.
Also applies to: 59-60, 134-135, 149-153, 175-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/RelayServer/WebResources/index.html` at line 2, The UI text in index.html is inconsistently mixed between English and Korean, so align the locale by either translating all labels/buttons/tabs and empty-state/tutorial copy to one language or moving the Korean strings behind proper i18n in the same places used by the tutorial and empty-state content. Check the html lang attribute and the affected instructional strings in the main template so the visible copy is consistent for users across the whole page.scripts/install-launchd.sh (1)
206-212: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBundle discovery via
find | head -1can pick a stale/wrong build.If
.buildcontains more than one path matching*release*(e.g. builds for multiple architectures, or a stale artifact from an earlier build),head -1picks an arbitrary one, potentially installing web assets that don't match the freshly built$BIN_SRC. Deriving the bundle path from$(dirname "$BIN_SRC")(same build output directory as the binary) would be more deterministic than a tree-widefind.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-launchd.sh` around lines 206 - 212, The bundle lookup in the install script can select the wrong release artifact because the current find/head approach searches the whole .build tree and may return a stale or mismatched bundle. Update the Bundle discovery logic in scripts/install-launchd.sh to derive CmuxRemote_RelayServer.bundle from the same build output directory as BIN_SRC (for example using the binary’s dirname) instead of scanning .build, so the installed web assets always match the freshly built binary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/CmuxRemote/Settings/SettingsView.swift`:
- Line 9: The port Stepper in SettingsView currently excludes the documented
default of 80, so update the Stepper range used with the port `@AppStorage`
binding to include 80 (for example, widen it to start at 80 or 1). Locate the
Stepper/port control in SettingsView and adjust its allowed range so fresh
installs can keep the default port instead of being forced away from it.
In `@README.md`:
- Around line 399-405: The daemon command swap is incomplete in the README’s
system daemon section: the pause flow still shows an unprivileged launchctl
bootout command, which will not work for the root-owned service. Update the
daemon-management examples around the existing
SERVICE="system/com.genie.cmuxremote" block so the stop/pause path also uses the
privileged system-domain form, matching the kickstart and print examples.
In `@scripts/install-launchd.sh`:
- Around line 198-201: The install flow overwrites BIN_DEST in place before
booting out any existing launchd job, which can crash a still-running daemon.
Update install-launchd.sh so the binary replacement in the install path is
atomic: write the new binary to a temporary location and rename it into BIN_DEST
only after the old instance is safely stopped, and apply the same ordering
around the bootout/bootstrap sequence referenced in the launchd setup logic. Use
the existing install binary step and daemon lifecycle steps to locate the
affected cp/bootout/bootstrap flow.
- Around line 83-96: The dry-run fallback in install-launchd.sh still fails when
USER is unset because OWNER_USER becomes the literal "unknown" and real_home()
then triggers fail on the dscl lookup. Update the dry-run branch around
OWNER_USER and real_home() so dry-run can preview safely without requiring a
resolvable account, e.g. by skipping the OWNER_HOME lookup and any fail path
when DRY_RUN=1 and OWNER_USER is not available/resolvable, while keeping the
non-dry-run behavior unchanged.
In `@Sources/RelayServer/WebResources/app.js`:
- Around line 15-20: The HTTPS default port logic in app.js is using an
inconsistent scheme value, so the PORT fallback never selects 443 and both BASE
and WS_URL are built with the wrong port. Update the SCHEME/WS_SCHEME and PORT
setup together so the HTTPS case is detected correctly in the top-level
constants, then rebuild BASE and WS_URL from those corrected values.
- Around line 427-441: Shift-only printable input is being treated as a shortcut
in the `attachCustomKeyEventHandler` handler, so characters like capitals and
symbols get intercepted instead of reaching xterm. Update the shortcut detection
logic in this handler to ignore `shift` when deciding whether a key event should
be sent through `sendKey(...)`, while still allowing true modifier shortcuts
that include `ctrl`, `alt`, or `cmd`; keep the existing `mapDomKey(event)` and
`isSpecialKey(keyName)` flow intact.
In `@Sources/RelayServer/WebResources/index.html`:
- Line 8: The xterm.js assets in the WebResources page are still loaded from
jsDelivr, so update the HTML to serve both the xterm CSS and JS locally instead
of using the CDN. Make the change in index.html where the terminal assets are
referenced, and keep the existing xterm-related includes consistent with the
local vendored files so the page no longer depends on third-party delivery.
---
Nitpick comments:
In `@scripts/install-launchd.sh`:
- Around line 206-212: The bundle lookup in the install script can select the
wrong release artifact because the current find/head approach searches the whole
.build tree and may return a stale or mismatched bundle. Update the Bundle
discovery logic in scripts/install-launchd.sh to derive
CmuxRemote_RelayServer.bundle from the same build output directory as BIN_SRC
(for example using the binary’s dirname) instead of scanning .build, so the
installed web assets always match the freshly built binary.
In `@Sources/RelayServer/CmuxRelay.swift`:
- Around line 228-231: The PID write in CmuxRelay’s PID-file handling leaves
stale bytes because the file is opened without truncation before `write()`.
Update the PID-file setup in the same block that writes `pidStr` to ensure the
file is truncated or otherwise overwritten from zero before writing the new PID,
so shorter PIDs don’t leave trailing characters behind.
In `@Sources/RelayServer/Routes.swift`:
- Around line 217-224: The static asset response in Routes.swift is overusing
"Cache-Control: no-cache, no-store, must-revalidate", which prevents conditional
caching for app.js/style.css. Update the response-building path in the
asset-serving logic to use an ETag and/or Last-Modified derived from the bundled
asset content or timestamp, and relax the cache policy so browsers can
revalidate instead of always re-downloading. Keep the stale-after-reinstall
safety behavior, but make the headers in the asset response conditional-cache
friendly in the same code that builds the .ok response.
In `@Sources/RelayServer/WebResources/index.html`:
- Line 5: The viewport meta tag in index.html is preventing users from zooming
because it includes user-scalable=no and maximum-scale=1. Update the viewport
settings to allow pinch-zoom and text scaling while keeping the existing layout
behavior, and verify the change in the meta tag used by the app shell.
- Line 2: The UI text in index.html is inconsistently mixed between English and
Korean, so align the locale by either translating all labels/buttons/tabs and
empty-state/tutorial copy to one language or moving the Korean strings behind
proper i18n in the same places used by the tutorial and empty-state content.
Check the html lang attribute and the affected instructional strings in the main
template so the visible copy is consistent for users across the whole page.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 421e0eca-817c-443d-9813-dbf822a9d212
📒 Files selected for processing (22)
.gitignorePRIVACY.mdPackage.swiftREADME.en.mdREADME.mdSources/RelayCore/ConfigLoader.swiftSources/RelayServer/CmuxRelay.swiftSources/RelayServer/HTTPServer.swiftSources/RelayServer/Routes.swiftSources/RelayServer/WebResources/app.jsSources/RelayServer/WebResources/index.htmlSources/RelayServer/WebResources/style.cssdocs/connection-guide.en.mddocs/connection-guide.mdios/CmuxRemote/CmuxRemoteApp.swiftios/CmuxRemote/Demo/DemoContent.swiftios/CmuxRemote/Settings/SettingsView.swiftscripts/generate_launch_assets.pyscripts/install-launchd.shscripts/relay.plist.tmplscripts/smoke-relay.shscripts/uninstall-launchd.sh
| var onTriggerTestNotification: (@MainActor () -> TestNotificationResult)? = nil | ||
| @AppStorage("cmux.host") private var host: String = "" | ||
| @AppStorage("cmux.port") private var port: Int = 4399 | ||
| @AppStorage("cmux.port") private var port: Int = 80 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'ios/CmuxRemote/Settings/SettingsView.swift' 'README.md' 'docs/*' '*.md' | sed -n '1,120p'
echo '--- SettingsView.swift ---'
cat -n ios/CmuxRemote/Settings/SettingsView.swift
echo '--- search cmux.port ---'
rg -n 'cmux\.port|port' ios/CmuxRemote README.md docs -g '!**/DerivedData/**' || trueRepository: NewTurn2017/cmux-remote
Length of output: 36026
🌐 Web query:
SwiftUI Stepper(value:in:) out of range bound value behavior clamp increment decrement Apple documentation
💡 Result:
When using the SwiftUI Stepper(value:in:step:) initializer, SwiftUI automatically manages the value within the provided ClosedRange [1][2][3]. The component exhibits the following behaviors: Clamping and Bounds Enforcement: When you provide a range (in:) to the Stepper, SwiftUI automatically clamps the bound value to ensure it never exceeds the specified minimum or maximum [4][2]. If a user attempts to increment or decrement the value beyond these bounds, the corresponding button (+ or -) is automatically disabled by the framework [4][2]. You do not need to implement manual guard logic to prevent the value from leaving the range [4][2]. Increment and Decrement Behavior: The Stepper uses the provided step value to adjust the binding [4][3]. If an increment or decrement would cause the value to exceed the boundary, SwiftUI prevents the operation and clamps the value to the boundary limit [2]. Important Distinction regarding Initializers: There are two primary ways to initialize a Stepper in SwiftUI [5]: 1. Value-based (with range): Using init(value:in:step:...), SwiftUI handles all range checking and clamping for you [4][2]. This is the recommended approach for maintaining data integrity [4]. 2. Callback-based (onIncrement/onDecrement): If you choose to use the initializers that provide onIncrement and onDecrement callbacks instead of a range, SwiftUI does not automatically enforce bounds [4]. In this case, you are responsible for manually checking the value and implementing guard statements to prevent the value from going out of your intended range [4]. In summary, for the value-based Stepper, clamping and button disabling are native, automatic features of the component [4][2]. If you require custom logic alongside range constraints, you must either manage the state manually or create a wrapper view that combines the behaviors [5].
Citations:
- 1: https://developer.apple.com/documentation/swiftui/stepper
- 2: https://mehmetbaykar.com/posts/swiftui-stepper-range-and-step/
- 3: https://www.codecademy.com/resources/docs/swiftui/views/stepper
- 4: https://soarias.com/swiftui/how-to-build-stepper-input/
- 5: https://stackoverflow.com/questions/65781501/how-to-use-increment-decrement-functions-in-stepper-while-also-using-onchange
Include the default port in the Stepper range
The port Stepper excludes 80, so fresh installs get pushed off the documented default. Expand the range to include 80 (for example 80...65535 or 1...65535).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ios/CmuxRemote/Settings/SettingsView.swift` at line 9, The port Stepper in
SettingsView currently excludes the documented default of 80, so update the
Stepper range used with the port `@AppStorage` binding to include 80 (for example,
widen it to start at 80 or 1). Locate the Stepper/port control in SettingsView
and adjust its allowed range so fresh installs can keep the default port instead
of being forced away from it.
| SERVICE="system/com.genie.cmuxremote" | ||
|
|
||
| # 재시작 (재빌드 없이 — 가장 자주 씀) | ||
| launchctl kickstart -k "$SERVICE" | ||
| sudo launchctl kickstart -k "$SERVICE" | ||
|
|
||
| # 상태 (state / pid / last exit code) | ||
| launchctl print "$SERVICE" | grep -E "state|pid|last exit" | ||
| sudo launchctl print "$SERVICE" | grep -E "state|pid|last exit" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Finish the system-daemon command swap.
The switch to system/... is right, but this section still leaves the later launchctl bootout example unprivileged, so the documented pause flow will fail under the new root daemon model.
🔧 Proposed fix
- launchctl bootout "$SERVICE"
+ sudo launchctl bootout "$SERVICE"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 399 - 405, The daemon command swap is incomplete in
the README’s system daemon section: the pause flow still shows an unprivileged
launchctl bootout command, which will not work for the root-owned service.
Update the daemon-management examples around the existing
SERVICE="system/com.genie.cmuxremote" block so the stop/pause path also uses the
privileged system-domain form, matching the kickstart and print examples.
| if [ -z "$OWNER_USER" ]; then | ||
| if [ "$DRY_RUN" -eq 1 ]; then | ||
| OWNER_USER="${USER:-unknown}" | ||
| note "dry-run without sudo; previewing owner as '$OWNER_USER'" | ||
| else | ||
| fail "could not determine owner user; run under sudo or set CMUX_REMOTE_USER" | ||
| fi | ||
| fi | ||
|
|
||
| real_home() { | ||
| dscl . -read "/Users/$1" NFSHomeDirectory 2>/dev/null | awk '{print $2}' | ||
| } | ||
| OWNER_HOME="$(real_home "$OWNER_USER")" | ||
| [ -n "$OWNER_HOME" ] || fail "could not resolve home for user '$OWNER_USER' (dscl lookup failed)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
fail() can trigger even in dry-run if $USER is unset.
When run without sudo and without CMUX_REMOTE_USER, dry-run falls back to OWNER_USER="${USER:-unknown}". If $USER isn't set (common in non-interactive/CI shells), OWNER_USER becomes the literal string unknown, and the subsequent dscl lookup for /Users/unknown will fail, causing fail (exit 1) even though this is supposed to be a non-destructive dry-run preview.
🔧 Proposed fix
- OWNER_USER="${USER:-unknown}"
+ OWNER_USER="${USER:-$(id -un)}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -z "$OWNER_USER" ]; then | |
| if [ "$DRY_RUN" -eq 1 ]; then | |
| OWNER_USER="${USER:-unknown}" | |
| note "dry-run without sudo; previewing owner as '$OWNER_USER'" | |
| else | |
| fail "could not determine owner user; run under sudo or set CMUX_REMOTE_USER" | |
| fi | |
| fi | |
| real_home() { | |
| dscl . -read "/Users/$1" NFSHomeDirectory 2>/dev/null | awk '{print $2}' | |
| } | |
| OWNER_HOME="$(real_home "$OWNER_USER")" | |
| [ -n "$OWNER_HOME" ] || fail "could not resolve home for user '$OWNER_USER' (dscl lookup failed)" | |
| if [ -z "$OWNER_USER" ]; then | |
| if [ "$DRY_RUN" -eq 1 ]; then | |
| OWNER_USER="${USER:-$(id -un)}" | |
| note "dry-run without sudo; previewing owner as '$OWNER_USER'" | |
| else | |
| fail "could not determine owner user; run under sudo or set CMUX_REMOTE_USER" | |
| fi | |
| fi | |
| real_home() { | |
| dscl . -read "/Users/$1" NFSHomeDirectory 2>/dev/null | awk '{print $2}' | |
| } | |
| OWNER_HOME="$(real_home "$OWNER_USER")" | |
| [ -n "$OWNER_HOME" ] || fail "could not resolve home for user '$OWNER_USER' (dscl lookup failed)" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-launchd.sh` around lines 83 - 96, The dry-run fallback in
install-launchd.sh still fails when USER is unset because OWNER_USER becomes the
literal "unknown" and real_home() then triggers fail on the dscl lookup. Update
the dry-run branch around OWNER_USER and real_home() so dry-run can preview
safely without requiring a resolvable account, e.g. by skipping the OWNER_HOME
lookup and any fail path when DRY_RUN=1 and OWNER_USER is not
available/resolvable, while keeping the non-dry-run behavior unchanged.
| note "installing binary under $PREFIX (root-owned); config/logs under $DEST" | ||
| mkdir -p "$PREFIX/bin" "$DEST" "$LOGDIR" "$LAUNCH_DAEMONS_DIR" | ||
| cp "$BIN_SRC" "$BIN_DEST" | ||
| chmod 755 "$BIN_DEST" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Binary is overwritten in-place while a prior daemon may still be running it.
cp "$BIN_SRC" "$BIN_DEST" (line 200) truncates/rewrites the destination file's existing inode via O_TRUNC. The bootout of any prior running system-domain instance happens later, at lines 243-251 — after the binary has already been overwritten. If a previous install of this daemon is currently executing $BIN_DEST as root and bound to port 80, truncating its backing file mid-execution risks a SIGBUS/crash of the live production daemon during what's meant to be a routine upgrade, and worse, a subsequent KeepAlive crash-loop against a half-written binary before bootout/bootstrap runs.
Copy to a temp file and atomically rename into place so any running process keeps its original (now-unlinked) inode intact:
🔧 Proposed fix: atomic binary replace
-cp "$BIN_SRC" "$BIN_DEST"
-chmod 755 "$BIN_DEST"
+cp "$BIN_SRC" "$BIN_DEST.new"
+chmod 755 "$BIN_DEST.new"
+mv -f "$BIN_DEST.new" "$BIN_DEST"Also applies to: 243-251
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-launchd.sh` around lines 198 - 201, The install flow
overwrites BIN_DEST in place before booting out any existing launchd job, which
can crash a still-running daemon. Update install-launchd.sh so the binary
replacement in the install path is atomic: write the new binary to a temporary
location and rename it into BIN_DEST only after the old instance is safely
stopped, and apply the same ordering around the bootout/bootstrap sequence
referenced in the launchd setup logic. Use the existing install binary step and
daemon lifecycle steps to locate the affected cp/bootout/bootstrap flow.
| const SCHEME = location.protocol === 'https:' ? 'https' : 'http'; | ||
| const WS_SCHEME = location.protocol === 'https:' ? 'wss' : 'ws'; | ||
| const HOST = location.hostname; | ||
| const PORT = location.port || (SCHEME === 'https:' ? '443' : '80'); | ||
| const BASE = `${SCHEME}://${HOST}:${PORT}`; | ||
| const WS_URL = `${WS_SCHEME}://${HOST}:${PORT}/v1/ws`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
HTTPS default port resolves to 80. SCHEME is assigned 'https' (no trailing colon) on Line 15, so the check SCHEME === 'https:' on Line 18 is always false. On an HTTPS page served without an explicit port, PORT becomes '80' instead of '443', breaking both BASE and WS_URL.
🐛 Proposed fix
-const PORT = location.port || (SCHEME === 'https:' ? '443' : '80');
+const PORT = location.port || (location.protocol === 'https:' ? '443' : '80');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const SCHEME = location.protocol === 'https:' ? 'https' : 'http'; | |
| const WS_SCHEME = location.protocol === 'https:' ? 'wss' : 'ws'; | |
| const HOST = location.hostname; | |
| const PORT = location.port || (SCHEME === 'https:' ? '443' : '80'); | |
| const BASE = `${SCHEME}://${HOST}:${PORT}`; | |
| const WS_URL = `${WS_SCHEME}://${HOST}:${PORT}/v1/ws`; | |
| const SCHEME = location.protocol === 'https:' ? 'https' : 'http'; | |
| const WS_SCHEME = location.protocol === 'https:' ? 'wss' : 'ws'; | |
| const HOST = location.hostname; | |
| const PORT = location.port || (location.protocol === 'https:' ? '443' : '80'); | |
| const BASE = `${SCHEME}://${HOST}:${PORT}`; | |
| const WS_URL = `${WS_SCHEME}://${HOST}:${PORT}/v1/ws`; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/RelayServer/WebResources/app.js` around lines 15 - 20, The HTTPS
default port logic in app.js is using an inconsistent scheme value, so the PORT
fallback never selects 443 and both BASE and WS_URL are built with the wrong
port. Update the SCHEME/WS_SCHEME and PORT setup together so the HTTPS case is
detected correctly in the top-level constants, then rebuild BASE and WS_URL from
those corrected values.
| term.attachCustomKeyEventHandler((event) => { | ||
| if (event.type !== 'keydown') return true; | ||
| const mods = []; | ||
| if (event.ctrlKey) mods.push('ctrl'); | ||
| if (event.altKey) mods.push('alt'); | ||
| if (event.shiftKey) mods.push('shift'); | ||
| if (event.metaKey) mods.push('cmd'); | ||
| const keyName = mapDomKey(event); | ||
| if (!keyName) return true; | ||
| if (mods.length === 0 && !isSpecialKey(keyName)) return true; | ||
| const ordered = ['ctrl', 'alt', 'shift', 'cmd'].filter(m => mods.includes(m)); | ||
| const encoded = ordered.length ? ordered.join('+') + '+' + keyName : keyName; | ||
| sendKey(encoded); | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
xterm.js attachCustomKeyEventHandler return false onData
💡 Result:
In xterm.js, attachCustomKeyEventHandler and onData serve different purposes, and you cannot return false from an onData handler to influence key processing. 1. attachCustomKeyEventHandler: This method is designed to intercept KeyboardEvents before they are processed by the terminal [1][2]. The callback function you provide receives a KeyboardEvent and is expected to return a boolean [3][1]. Returning false from this handler tells xterm.js that the event should not be processed further (effectively "canceling" it) [1][4]. This is the correct mechanism to use if you want to prevent specific key combinations (like shortcuts or browser hotkeys) from being handled by xterm.js [1][5][6]. 2. onData: The onData event listener fires after the terminal has already processed user input (typing, pasting, etc.) and determined what data should be sent to the backing pseudoterminal (pty) [7][8]. The callback receives a string of data [9][8]. Because onData is an event emitter listener, it does not support returning false to prevent terminal behavior; it is strictly a notification that data has been generated [8]. If your goal is to prevent xterm.js from responding to specific keys, you must use attachCustomKeyEventHandler, not onData [1][5]. Example of preventing a key combination: term.attachCustomKeyEventHandler(event => { // Return false to prevent xterm.js from processing the event if (event.ctrlKey && event.key === 'c') { return false; } // Return true (or undefined) to allow default processing return true; });
Citations:
- 1: https://en.1991421.cn/2024/04/13/xtermattachcustomkeyeventhandler/
- 2: https://github.com/xtermjs/xterm.js/blob/d3fd3d0f2254a966238e575f3505d5cac31cff91/src/browser/Terminal.ts
- 3: https://xtermjs.org/docs/api/terminal/classes/terminal/
- 4: https://github.com/xtermjs/xterm.js/blob/2e8410da4bf4ccc44b7277ec22a0d473565c9d13/src/Terminal.ts
- 5: Browser Copy/Paste support documentation xtermjs/xterm.js#2478
- 6: https://stackoverflow.com/questions/58948835/xtermjs-can-not-copy-paste
- 7: https://github.com/xtermjs/xterm.js/blob/master/README.md
- 8: https://github.com/xtermjs/xterm.js/blob/2fdb46919ce7a329afe65fe69bcf948d310a2b8a/typings/xterm.d.ts
- 9: Terminal.on is not a function xtermjs/xterm.js#2571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== xterm version references ==\n'
rg -n --hidden --no-messages 'xterm|attachCustomKeyEventHandler|onData' \
package.json package-lock.json yarn.lock pnpm-lock.yaml Sources README.md . \
| head -n 200
printf '\n== target file context ==\n'
sed -n '400,460p' Sources/RelayServer/WebResources/app.js
printf '\n== bundling clues ==\n'
git ls-files | rg '(^|/)(package\.json|package-lock\.json|yarn\.lock|pnpm-lock\.yaml|app\.js|xterm)'Repository: NewTurn2017/cmux-remote
Length of output: 2417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relevant functions ==\n'
sed -n '1,140p' Sources/RelayServer/WebResources/app.js
printf '\n== key handler region ==\n'
sed -n '420,455p' Sources/RelayServer/WebResources/app.jsRepository: NewTurn2017/cmux-remote
Length of output: 6638
Shift-only printable keys are intercepted here. event.shiftKey makes mods.length > 0, so typing capitals or symbols like @/! falls into sendKey(...) and returns false instead of reaching xterm’s normal text input path. Exclude shift from the shortcut trigger check for printable keys.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/RelayServer/WebResources/app.js` around lines 427 - 441, Shift-only
printable input is being treated as a shortcut in the
`attachCustomKeyEventHandler` handler, so characters like capitals and symbols
get intercepted instead of reaching xterm. Update the shortcut detection logic
in this handler to ignore `shift` when deciding whether a key event should be
sent through `sendKey(...)`, while still allowing true modifier shortcuts that
include `ctrl`, `alt`, or `cmd`; keep the existing `mapDomKey(event)` and
`isSpecialKey(keyName)` flow intact.
| <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=no, viewport-fit=cover"> | ||
| <meta name="color-scheme" content="dark"> | ||
| <title>cmux Remote</title> | ||
| <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@xterm/[email protected]/css/xterm.css"> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files\n'
git ls-files Sources/RelayServer/WebResources/index.html Sources/RelayServer/Routes.swift Package.swift 2>/dev/null || true
printf '\n## index.html\n'
nl -ba Sources/RelayServer/WebResources/index.html | sed -n '1,240p'
printf '\n## Routes.swift\n'
nl -ba Sources/RelayServer/Routes.swift | sed -n '1,260p'
printf '\n## Package.swift\n'
nl -ba Package.swift | sed -n '1,260p'Repository: NewTurn2017/cmux-remote
Length of output: 317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any references to xterm assets and static-file handling.
rg -n --hidden --glob '!**/.git/**' -S 'xterm|serveStatic|contentType|WebResources|cdn.jsdelivr.net' Sources Package.swiftRepository: NewTurn2017/cmux-remote
Length of output: 1743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the surrounding HTML around the CDN references and any local resource hints.
nl -ba Sources/RelayServer/WebResources/index.html | sed -n '1,220p'Repository: NewTurn2017/cmux-remote
Length of output: 201
Vendor xterm.js locally instead of loading it from jsDelivr. The page pulls both the terminal CSS and JS from a third-party CDN without integrity/crossorigin, which adds a failure point for a Tailscale-only deployment and exposes the app’s authenticated session to CDN tampering or MITM risk. If CDN loading must remain, add SRI plus crossorigin="anonymous".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/RelayServer/WebResources/index.html` at line 8, The xterm.js assets
in the WebResources page are still loaded from jsDelivr, so update the HTML to
serve both the xterm CSS and JS locally instead of using the CDN. Make the
change in index.html where the terminal assets are referenced, and keep the
existing xterm-related includes consistent with the local vendored files so the
page no longer depends on third-party delivery.
1 participant
Summary
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation