Closed
Conversation
…ntend Implements a minimal terminal over WebSocket for the dashboard. Backend (librefang-api): - New route: GET /api/terminal/ws with Bearer/?token= auth - Per-IP connection limit via ws_tracker() from existing ws module - Idle timeout and ping/pong via existing ws_idle_timeout_secs config - Protocol: Client sends input/resize/close; server sends started/output/exit/error with UTF-8/binary detection (base64 encoding for non-UTF-8 output) - PTY abstraction via portable-pty with shell selection: Unix: $SHELL fallback /bin/sh, Windows: %COMSPEC% fallback cmd.exe - Exit notifications on client close, idle timeout, and server close - WsConnectionGuard properly scoped into handler Frontend (dashboard): - TerminalPage with @xterm/xterm + @xterm/addon-fit - Dark theme, auto-reconnect with 2s delay - Full i18n: en.json and zh.json translations - Terminal tab in sidebar Advanced section (after Comms) - Command palette entry Fixes: WsConnectionGuard scope, PTY orphan processes, shell path accuracy, binary flag, validate() call, i18n, auto-reconnect, exit notifications, msg.data/content validation.
Clone PluginRuntime before moves in loops (prewarm, on_event spawn), fix Cow<str> trait bounds (AsRef<OsStr> for cmd.env, Display for tracing), and use std::io::Error::other per clippy.
Update pnpm-lock.yaml for @xterm dependencies and add `just install-full` that copies built dashboard to ~/.librefang/dashboard with a .version file to prevent sync_dashboard from overwriting with stale GitHub release assets.
- Spawn interactive shell without -c flag instead of broken "exec 0" - Prevent infinite reconnect loop after manual disconnect - Detect child process exit and notify client - Fix binary output decoding with Uint8Array for bytes > 127 - Send initial resize on WebSocket connect - Stabilize useEffect to avoid terminal reinit on re-render - Include cwd in started message - Move Terminal nav item between Comms and Network - Set terminal height to 70% on screens wider than 1000px
Reviewer's GuideAdds a WebSocket-powered interactive terminal feature exposed via the API and dashboard, backed by a portable PTY implementation, and performs small runtime fixes/refactors to support cloning runtime values and logging improvements. Sequence diagram for the new WebSocket terminal session flowsequenceDiagram
actor User
participant Dashboard
participant TerminalPage
participant BrowserWS as WebSocket
participant ApiServer
participant TerminalRoute
participant WsHelpers
participant PtySession
participant ShellProcess
User->>Dashboard: Click nav.terminal
Dashboard->>TerminalPage: Render TerminalPage
TerminalPage->>BrowserWS: new WebSocket(/api/terminal/ws)
BrowserWS->>ApiServer: WS upgrade request
ApiServer->>TerminalRoute: terminal_ws
TerminalRoute->>ApiServer: Validate API tokens and sessions
TerminalRoute->>WsHelpers: try_acquire_ws_slot(ip, max_ws_per_ip)
WsHelpers-->>TerminalRoute: Option<WsConnectionGuard>
alt slot available
TerminalRoute->>BrowserWS: Accept upgrade
BrowserWS->>TerminalRoute: WebSocket connection
TerminalRoute->>TerminalRoute: handle_terminal_ws
else slot unavailable
TerminalRoute-->>BrowserWS: HTTP 429 TooManyRequests
end
loop on connection start
TerminalRoute->>PtySession: PtySession::spawn()
PtySession->>ShellProcess: Spawn interactive shell
PtySession-->>TerminalRoute: (pty, pty_rx)
TerminalRoute->>WsHelpers: send_json(sender, Started)
WsHelpers-->>BrowserWS: {type: started, shell, pid, cwd}
end
loop PTY output
PtySession-->>TerminalRoute: data via pty_rx
TerminalRoute->>WsHelpers: send_json(Output or Output{binary:true})
WsHelpers-->>BrowserWS: {type: output, data}
BrowserWS-->>TerminalPage: message event
TerminalPage->>TerminalPage: xterm.write(data)
end
loop User input
User->>TerminalPage: Type in terminal
TerminalPage->>BrowserWS: {type: input, data}
BrowserWS->>TerminalRoute: Text message
TerminalRoute->>TerminalRoute: Parse ClientMessage
TerminalRoute->>PtySession: write(data)
end
loop Resize
TerminalPage->>BrowserWS: {type: resize, cols, rows}
BrowserWS->>TerminalRoute: Text message
TerminalRoute->>PtySession: resize(cols, rows)
end
alt Client closes
TerminalPage->>BrowserWS: {type: close}
BrowserWS->>TerminalRoute: Text message
TerminalRoute->>WsHelpers: send_json(Exit{code:0})
WsHelpers-->>BrowserWS: {type: exit, code:0}
BrowserWS-->>TerminalPage: close
else Idle timeout
TerminalRoute->>WsHelpers: send_json(Exit{code:124})
WsHelpers-->>BrowserWS: {type: exit, code:124}
TerminalRoute->>BrowserWS: Close frame
else Shell exits
PtySession-->>TerminalRoute: pty_rx ends
TerminalRoute->>WsHelpers: send_json(Exit)
WsHelpers-->>BrowserWS: {type: exit}
end
TerminalRoute->>WsHelpers: WsConnectionGuard dropped
WsHelpers->>WsHelpers: Decrement connection count
Class diagram for terminal WebSocket route messages and helpersclassDiagram
class ClientMessage {
<<enum>>
+validate() Result<(), String>
}
class ClientInput {
+String data
+Option<u64> timestamp
}
class ClientResize {
+u16 cols
+u16 rows
}
class ClientClose {
}
ClientMessage <|-- ClientInput
ClientMessage <|-- ClientResize
ClientMessage <|-- ClientClose
class ServerMessage {
<<enum>>
}
class ServerStarted {
+String shell
+u32 pid
+Option<String> cwd
}
class ServerOutput {
+String data
+Option<bool> binary
}
class ServerExit {
+u32 code
+Option<String> signal
}
class ServerError {
+String content
}
ServerMessage <|-- ServerStarted
ServerMessage <|-- ServerOutput
ServerMessage <|-- ServerExit
ServerMessage <|-- ServerError
class TerminalRouteModule {
+MAX_WS_MSG_SIZE usize
+terminal_health(state: Arc<AppState>) async
+terminal_ws(ws: WebSocketUpgrade, state: Arc<AppState>, addr: SocketAddr, headers: HeaderMap, uri: Uri) async
+handle_terminal_ws(socket: WebSocket, state: Arc<AppState>, client_ip: IpAddr, guard: WsConnectionGuard) async
}
class WsModuleExports {
+ws_tracker() &'static DashMap<IpAddr, AtomicUsize>
+try_acquire_ws_slot(ip: IpAddr, max_ws_per_ip: usize) Option<WsConnectionGuard>
+send_json(sender: &Arc<Mutex<SplitSink<WebSocket, Message>>>, value: &serde_json::Value) async Result<(), axum::Error>
}
class PtySession {
+pid u32
+shell String
+spawn() std::io::Result<(PtySession, mpsc::Receiver<Vec<u8>>)>$
}
TerminalRouteModule --> ClientMessage : parses
TerminalRouteModule --> ServerMessage : sends
TerminalRouteModule --> WsModuleExports : uses
TerminalRouteModule --> PtySession : controls PTY session
WsModuleExports ..> WsConnectionGuard : manages
Class diagram for the new dashboard TerminalPage and WebSocket interactionsclassDiagram
class TerminalPage {
-HTMLDivElement containerRef
-Terminal terminalRef
-FitAddon fitAddonRef
-WebSocket wsRef
-Timeout reconnectTimeoutRef
-bool intentionalDisconnect
-bool isConnected
-Option<String> error
+TerminalPage()
+connect() void
+disconnect() void
}
class XtermTerminal {
+open(container: HTMLDivElement) void
+write(data: any) void
+onData(handler: function) void
+onResize(handler: function) void
+dispose() void
}
class FitAddon {
+fit() void
}
class ServerMessageTs {
+"started" | "output" | "exit" | "error" type
+String shell
+number pid
+String data
+boolean binary
+number code
+String signal
+String content
}
class ApiHelpers {
+buildAuthenticatedWebSocketUrl(path: String) String$
}
TerminalPage --> XtermTerminal : creates
TerminalPage --> FitAddon : uses
TerminalPage --> ServerMessageTs : parses
TerminalPage --> ApiHelpers : builds WS URL
TerminalPage ..> WebSocket : communicates with API
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
terminal_ws, the idle-timeout branch usesws_idle_timeout.saturating_sub(last_activity.elapsed())insidetokio::time::sleep, which becomessleep(0)once the timeout is exceeded and can cause a busy loop; consider tracking a fixed deadlineInstantand breaking whenInstant::now() >= deadlineinstead of recalculating a saturated duration each iteration. - In
TerminalPage.tsx,ws.onerrorsets a hard-coded English string ("WebSocket connection error") instead of using the i18n translation keys used elsewhere; it would be more consistent to surface this viat("terminal.error_connection")(or a similar key) and reuse that in the subtitle/error text. - The
shell_for_current_osfunction returns a(String, &str)pair but the flag part is unused by the PTY session (you always spawn an interactive shell with no args); either drop the flag from the API or start using it where appropriate to avoid dead/unnecessary values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `terminal_ws`, the idle-timeout branch uses `ws_idle_timeout.saturating_sub(last_activity.elapsed())` inside `tokio::time::sleep`, which becomes `sleep(0)` once the timeout is exceeded and can cause a busy loop; consider tracking a fixed deadline `Instant` and breaking when `Instant::now() >= deadline` instead of recalculating a saturated duration each iteration.
- In `TerminalPage.tsx`, `ws.onerror` sets a hard-coded English string (`"WebSocket connection error"`) instead of using the i18n translation keys used elsewhere; it would be more consistent to surface this via `t("terminal.error_connection")` (or a similar key) and reuse that in the subtitle/error text.
- The `shell_for_current_os` function returns a `(String, &str)` pair but the flag part is unused by the PTY session (you always spawn an interactive shell with no args); either drop the flag from the API or start using it where appropriate to avoid dead/unnecessary values.
## Individual Comments
### Comment 1
<location path="crates/librefang-api/dashboard/src/pages/TerminalPage.tsx" line_range="99-100" />
<code_context>
+ }
+ };
+
+ ws.onerror = () => {
+ setError("WebSocket connection error");
+ };
+
</code_context>
<issue_to_address>
**suggestion:** WebSocket error text is hardcoded in English instead of going through i18n like the rest of the terminal UI.
This handler currently uses the hardcoded string `"WebSocket connection error"`. To keep the terminal UI fully localized and avoid mixing translated and untranslated text, route this through i18n, e.g. `setError(t("terminal.websocket_error"))`.
Suggested implementation:
```typescript
ws.onerror = () => {
setError(t("terminal.websocket_error"));
};
```
Make sure that the `terminal.websocket_error` key exists in your i18n resource files (e.g. `en/translation.json`, etc.) with the appropriate localized strings. No other TypeScript changes should be necessary since `t` is already in scope and used elsewhere in this component.
</issue_to_address>
### Comment 2
<location path="crates/librefang-api/dashboard/src/pages/TerminalPage.tsx" line_range="103-109" />
<code_context>
+ setError("WebSocket connection error");
+ };
+
+ ws.onclose = () => {
+ setIsConnected(false);
+ if (intentionalDisconnectRef.current) {
+ intentionalDisconnectRef.current = false;
+ return;
+ }
+ reconnectTimeoutRef.current = setTimeout(() => {
+ if (
+ wsRef.current === null ||
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The auto‑reconnect loop does not distinguish between transient failures and hard failures (e.g. auth issues), which could cause repeated failed reconnect attempts.
In `ws.onclose` a reconnect is always scheduled (when not intentional) after `RECONNECT_DELAY_MS`. If the server is actively rejecting connections (auth failure, rate limiting, etc.), the client will continue retrying indefinitely. Consider inspecting the `CloseEvent` (e.g. `event.code`) to detect non‑transient close reasons and either stop reconnecting or surface a clearer error instead of retrying forever.
Suggested implementation:
```typescript
ws.onerror = () => {
setError("WebSocket connection error");
};
ws.onclose = (event: CloseEvent) => {
setIsConnected(false);
if (intentionalDisconnectRef.current) {
intentionalDisconnectRef.current = false;
return;
}
// Treat certain close codes as non-transient and avoid infinite reconnect loops.
// - 1008: Policy violation (often auth/authorization issues)
// - 1011: Internal error
// - 4xxx: Application-specific errors (e.g. auth, rate limiting)
const isAppSpecificError = event.code >= 4000 && event.code <= 4999;
const isNonTransientCloseCode = event.code === 1008 || event.code === 1011 || isAppSpecificError;
if (isNonTransientCloseCode) {
setError(
event.reason ||
t("terminal.connection_closed_non_recoverable")
);
return;
}
reconnectTimeoutRef.current = setTimeout(() => {
if (
wsRef.current === null ||
wsRef.current.readyState === WebSocket.CLOSED
) {
connect();
}
}, RECONNECT_DELAY_MS);
};
```
1. Ensure that a translation key like `terminal.connection_closed_non_recoverable` exists in your i18n resources with a user-friendly message explaining that the connection was closed due to a non-recoverable error (e.g., auth failure or rate limiting).
2. If your project uses a custom WebSocket type or different typings, you may need to adjust `CloseEvent` to match your environment (e.g. from the DOM lib or a specific WebSocket implementation).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+99
to
+100
| ws.onerror = () => { | ||
| setError("WebSocket connection error"); |
There was a problem hiding this comment.
suggestion: WebSocket error text is hardcoded in English instead of going through i18n like the rest of the terminal UI.
This handler currently uses the hardcoded string "WebSocket connection error". To keep the terminal UI fully localized and avoid mixing translated and untranslated text, route this through i18n, e.g. setError(t("terminal.websocket_error")).
Suggested implementation:
ws.onerror = () => {
setError(t("terminal.websocket_error"));
};Make sure that the terminal.websocket_error key exists in your i18n resource files (e.g. en/translation.json, etc.) with the appropriate localized strings. No other TypeScript changes should be necessary since t is already in scope and used elsewhere in this component.
Comment on lines
+103
to
+109
| ws.onclose = () => { | ||
| setIsConnected(false); | ||
| if (intentionalDisconnectRef.current) { | ||
| intentionalDisconnectRef.current = false; | ||
| return; | ||
| } | ||
| reconnectTimeoutRef.current = setTimeout(() => { |
There was a problem hiding this comment.
suggestion (bug_risk): The auto‑reconnect loop does not distinguish between transient failures and hard failures (e.g. auth issues), which could cause repeated failed reconnect attempts.
In ws.onclose a reconnect is always scheduled (when not intentional) after RECONNECT_DELAY_MS. If the server is actively rejecting connections (auth failure, rate limiting, etc.), the client will continue retrying indefinitely. Consider inspecting the CloseEvent (e.g. event.code) to detect non‑transient close reasons and either stop reconnecting or surface a clearer error instead of retrying forever.
Suggested implementation:
ws.onerror = () => {
setError("WebSocket connection error");
};
ws.onclose = (event: CloseEvent) => {
setIsConnected(false);
if (intentionalDisconnectRef.current) {
intentionalDisconnectRef.current = false;
return;
}
// Treat certain close codes as non-transient and avoid infinite reconnect loops.
// - 1008: Policy violation (often auth/authorization issues)
// - 1011: Internal error
// - 4xxx: Application-specific errors (e.g. auth, rate limiting)
const isAppSpecificError = event.code >= 4000 && event.code <= 4999;
const isNonTransientCloseCode = event.code === 1008 || event.code === 1011 || isAppSpecificError;
if (isNonTransientCloseCode) {
setError(
event.reason ||
t("terminal.connection_closed_non_recoverable")
);
return;
}
reconnectTimeoutRef.current = setTimeout(() => {
if (
wsRef.current === null ||
wsRef.current.readyState === WebSocket.CLOSED
) {
connect();
}
}, RECONNECT_DELAY_MS);
};- Ensure that a translation key like
terminal.connection_closed_non_recoverableexists in your i18n resources with a user-friendly message explaining that the connection was closed due to a non-recoverable error (e.g., auth failure or rate limiting). - If your project uses a custom WebSocket type or different typings, you may need to adjust
CloseEventto match your environment (e.g. from the DOM lib or a specific WebSocket implementation).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new terminal feature, enabling real-time terminal sessions via WebSockets and PTY integration. The implementation includes a new backend route handler for terminal communication and a corresponding frontend page using xterm.js. My review highlights several areas for improvement, including performance concerns regarding session cleanup in the WebSocket handler, redundant validation logic, unnecessary memory allocations, and maintainability issues in the frontend cleanup logic.
Comment on lines
+168
to
+174
| let mut sessions = state.active_sessions.write().await; | ||
| sessions.retain(|_, st| { | ||
| !crate::password_hash::is_token_expired( | ||
| st, | ||
| crate::password_hash::DEFAULT_SESSION_TTL_SECS, | ||
| ) | ||
| }); |
There was a problem hiding this comment.
The WebSocket handler performs session cleanup (sessions.retain(...)) for every new connection attempt. This operation requires a write lock on the active_sessions map and iterates over all active sessions, which can become a significant performance bottleneck under load. This cleanup logic should be moved out of the hot path for handling new connections and into a periodic background task.
Comment on lines
+79
to
+81
| } catch { | ||
| terminalRef.current?.write(msg.data); | ||
| } |
There was a problem hiding this comment.
The fallback in this catch block is not ideal. If atob() fails, it means the data received from the server is not valid Base64. Writing this raw data directly to the terminal will likely result in garbage output for the user. A better approach would be to log the error for debugging and display a clear error message in the terminal.
Suggested change
| } catch { | |
| terminalRef.current?.write(msg.data); | |
| } | |
| } catch (e) { | |
| console.error("Failed to decode base64 data from server:", e); | |
| terminalRef.current?.write("\r\n[Error: Corrupted binary data received from server]\r\n"); | |
| } |
Comment on lines
+176
to
+189
| return () => { | ||
| window.removeEventListener("resize", handleResize); | ||
| if (reconnectTimeoutRef.current) { | ||
| clearTimeout(reconnectTimeoutRef.current); | ||
| } | ||
| if (wsRef.current) { | ||
| intentionalDisconnectRef.current = true; | ||
| wsRef.current.send(JSON.stringify({ type: "close" })); | ||
| wsRef.current.close(); | ||
| wsRef.current = null; | ||
| } | ||
| setIsConnected(false); | ||
| term.dispose(); | ||
| }; |
There was a problem hiding this comment.
The cleanup function in this useEffect hook duplicates most of the logic from the disconnect function. This can lead to maintenance issues where one is updated but the other is forgotten. For instance, the cleanup function is missing reconnectTimeoutRef.current = null; which is present in disconnect.
To improve maintainability and avoid code duplication, you should call the disconnect function from within the cleanup logic.
return () => {
window.removeEventListener("resize", handleResize);
disconnect();
term.dispose();
};
| #[serde(rename = "input")] | ||
| Input { | ||
| data: String, | ||
| timestamp: Option<u64>, |
There was a problem hiding this comment.
The timestamp field in ClientMessage::Input is being deserialized but it's not used anywhere in the handler. If this field is not planned for future use, it should be removed to simplify the data model and reduce unnecessary data transfer.
Comment on lines
+83
to
+89
| const MAX_INPUT_SIZE: usize = 64 * 1024; | ||
| if data.len() > MAX_INPUT_SIZE { | ||
| return Err(format!( | ||
| "Input too large: {} bytes (max {MAX_INPUT_SIZE})", | ||
| data.len() | ||
| )); | ||
| } |
There was a problem hiding this comment.
The size validation for ClientMessage::Input data is redundant. The handle_terminal_ws function already checks the total WebSocket message size against MAX_WS_MSG_SIZE. Since MAX_INPUT_SIZE is the same as MAX_WS_MSG_SIZE, and the JSON message itself has overhead, the check on data.len() will never fail. The outer check on the total message size is sufficient and this redundant check can be removed.
// The check for input size is redundant and can be removed.
// The overall message size is already checked in `handle_terminal_ws`.
Ok(())
Comment on lines
+247
to
+260
| let output_msg = match String::from_utf8(data.clone()) { | ||
| Ok(s) => serde_json::json!({ | ||
| "type": "output", | ||
| "data": s | ||
| }), | ||
| Err(_) => { | ||
| use base64::Engine; | ||
| serde_json::json!({ | ||
| "type": "output", | ||
| "data": base64::engine::general_purpose::STANDARD.encode(&data), | ||
| "binary": true | ||
| }) | ||
| } | ||
| }; |
There was a problem hiding this comment.
The data vector is being cloned before being passed to String::from_utf8. This is an unnecessary allocation. You can avoid the clone by using the FromUtf8Error::into_bytes method to recover the original vector in case of a parsing failure.
let output_msg = match String::from_utf8(data) {
Ok(s) => serde_json::json!({
"type": "output",
"data": s
}),
Err(e) => {
use base64::Engine;
let data = e.into_bytes();
serde_json::json!({
"type": "output",
"data": base64::engine::general_purpose::STANDARD.encode(&data),
"binary": true
})
}
};
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type
Summary
Changes
Attribution
Co-authored-by, commit preservation, or explicit credit in the PR body)Testing
cargo clippy --workspace --all-targets -- -D warningspassescargo test --workspacepassesSecurity
Summary by Sourcery
Add a browser-based terminal feature backed by a PTY over authenticated WebSocket, expose supporting APIs, and integrate it into the dashboard navigation and install workflow.
New Features:
install-fulljust task to install both the CLI and a fresh dashboard build into the user configuration directory.Enhancements:
Build:
portable-ptycrate dependency for PTY support in the API service.Documentation:
Tests: