Fix dockstat docker exports#132
Open
Its4Nik wants to merge 6 commits into
Open
Conversation
|
Contributor
Reviewer's GuideRefactors DockStat’s WebSocket layer to a centralized topic-based handler shared by API and frontend, fixes logger/WebSocket circular dependencies, and normalizes package metadata, exports, and versioning across the monorepo with new changelogs and npm packaging configuration. Sequence diagram for topic-based WebSocket logging flowsequenceDiagram
actor Browser
participant LogFeedEffect as logFeedEffect
participant TopicSub as createTopicSubscription
participant WSClient as WebSocket
participant DSWebSockerHandler as DSWebSockerHandler
participant Logger as BaseLogger
Browser->>LogFeedEffect: mount React component
LogFeedEffect->>TopicSub: createTopicSubscription("logs", onData)
TopicSub->>WSClient: new WebSocket(wsUrlWithAuth)
WSClient-->>TopicSub: onopen
TopicSub->>WSClient: send ClientMessage { type: "subscribe", topic: "logs" }
WSClient->>DSWebSockerHandler: message {type: "subscribe", topic: "logs"}
DSWebSockerHandler->>DSWebSockerHandler: subscribe(ws, "logs")
Logger->>DSWebSockerHandler: send("logs", logEntry)
DSWebSockerHandler->>DSWebSockerHandler: broadcast("logs", logEntry)
DSWebSockerHandler-->>WSClient: ServerMessage { topic: "logs", data, timestamp }
WSClient-->>TopicSub: onmessage(ServerMessage)
TopicSub->>LogFeedEffect: onData(logEntry)
LogFeedEffect-->>Browser: setLogMessage(logEntry)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 3 other issues, and left some high level feedback:
Security issues:
- Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections. (link)
General comments:
- The new
useWebSockethook inapps/dockstat/src/lib/websocketEffects/index.tslooks unfinished (e.g.onUnmount: () =>has no implementation and the hook is never exported/used); either complete its cleanup logic and usage or remove it to avoid dead/broken code. - In
apps/api/src/logger.tsyou're usingrequire("./websockets")inside an ESM/Bun module; consider switching to a dynamicimport("./websockets")to avoid runtime issues withrequirenot being defined and to keep the module system consistent. - The new
createTopicSubscriptionclient intopicSubscription.tsbuilds the WebSocket URL fromimport.meta.env.DOCKSTAT_API_PORT || 'http://localhost:3030'and manually appends?token=..., but the server-side handler doesn’t read this token andgetAuthHeadersis now unused; it would be safer to derive the base URL fromwindow.locationplus the configured API port and align the auth mechanism (or remove the unused header helper) so client and server expectations match.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `useWebSocket` hook in `apps/dockstat/src/lib/websocketEffects/index.ts` looks unfinished (e.g. `onUnmount: () =>` has no implementation and the hook is never exported/used); either complete its cleanup logic and usage or remove it to avoid dead/broken code.
- In `apps/api/src/logger.ts` you're using `require("./websockets")` inside an ESM/Bun module; consider switching to a dynamic `import("./websockets")` to avoid runtime issues with `require` not being defined and to keep the module system consistent.
- The new `createTopicSubscription` client in `topicSubscription.ts` builds the WebSocket URL from `import.meta.env.DOCKSTAT_API_PORT || 'http://localhost:3030'` and manually appends `?token=...`, but the server-side handler doesn’t read this token and `getAuthHeaders` is now unused; it would be safer to derive the base URL from `window.location` plus the configured API port and align the auth mechanism (or remove the unused header helper) so client and server expectations match.
## Individual Comments
### Comment 1
<location path="apps/dockstat/src/lib/websocketEffects/index.ts" line_range="8-17" />
<code_context>
+import {api, getAuthHeaders} from "../api.ts"
+import { useGeneralSettings } from "@/components/settings/general/sections/useGeneralSettings";
+
+const useWebSocket = () => {
+ const settings = useGeneralSettings()
+ const [wsData, setWsData] = useState()
+
+ useEffect(() => {
+ const ws = api.ws.subscribe({ headers: getAuthHeaders() })
+ ws.subscribe((msg) => {
+ setWsData(JSON.parse(String(msg.data)))
+ })
+ }, [])
+
+ return {
+ wsData,
+ onUnmount: () =>
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `useWebSocket` hook is syntactically incomplete and never cleans up the WebSocket connection.
`onUnmount: () =>` is currently invalid syntax and will stop this file from compiling. Please either implement `onUnmount` or return a cleanup function from `useEffect` that closes/unsubscribes the WebSocket. Also, either remove the unused `useGeneralSettings` dependency or add it to the `useEffect` dependency array if it affects the connection configuration.
</issue_to_address>
### Comment 2
<location path="apps/api/src/logger.ts" line_range="4-10" />
<code_context>
-import { logClients } from "./websockets/logSocket"
+
+// Lazy load WebSocket handler to avoid circular dependency
+let wsHandler: typeof import("./websockets").DSWebSockerHandler | null = null
const BaseLogger = new Logger("DockStatAPI", [], (entry) => {
- for (const client of logClients) {
+ // Lazy load and send logs to WebSocket
+ if (!wsHandler) {
try {
- client.send(entry, true)
- } catch (err) {
- BaseLogger.error(`Failed to send log to client - ${JSON.stringify(err)}`)
+ wsHandler = require("./websockets").DSWebSockerHandler
+ } catch (error) {
+ // WebSocket handler not available yet, will retry on next log
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `require` in an ESM/TypeScript module can break at runtime; prefer a dynamic import for lazy-loading.
Since this is an ESM project (`"type": "module"`), using CommonJS `require` here can throw at runtime in Node/Bun unless you set up `createRequire`. To keep lazy loading and avoid circular imports, use a dynamic import instead, e.g. `const { DSWebSockerHandler } = await import("./websockets")` (or inside the callback). You can then type `wsHandler` as `WebSocketHandler | null` rather than `typeof import("./websockets").DSWebSockerHandler` for clearer, accurate typing.
</issue_to_address>
### Comment 3
<location path="apps/dockstat/src/lib/websocketEffects/topicSubscription.ts" line_range="34" />
<code_context>
+ * @param onData - Callback function called when data is received for this topic
+ * @returns Cleanup function to unsubscribe and close the connection
+ */
+export function createTopicSubscription<T>(
+ topic: string,
+ onData: (data: T) => void
</code_context>
<issue_to_address>
**issue (complexity):** Consider splitting URL/auth construction and reconnection logic into reusable WebSocket utilities so this helper focuses only on topic-specific behavior.
You can keep the same behavior but reduce complexity by separating “connection management” from “topic subscription” and by centralizing URL/auth construction.
### 1. Centralize URL & auth handling
Instead of re-implementing URL + token logic inside this helper, delegate to a small, reusable utility so other WebSocket consumers can share it:
```ts
// websocketClient.ts
import { getAuthHeaders } from "../api";
export function buildWebSocketUrl(path: string): string {
const baseUrl = import.meta.env.DOCKSTAT_API_PORT || "http://localhost:3030";
const wsBase = baseUrl.replace("http://", "ws://").replace("https://", "wss://");
const headers = getAuthHeaders(); // already knows how to read token
const token = headers.Authorization?.replace("Bearer ", "");
const query = token ? `?token=${encodeURIComponent(token)}` : "";
return `${wsBase}${path}${query}`;
}
```
Then your helper becomes simpler and avoids duplicating auth concerns:
```ts
import { buildWebSocketUrl } from "./websocketClient";
export function createTopicSubscription<T>(
topic: string,
onData: (data: T) => void
): () => void {
const wsUrlWithAuth = buildWebSocketUrl("/ws");
// ...rest unchanged...
}
```
### 2. Extract the reconnection/state machine into a small client
Right now `createTopicSubscription` owns flags + reconnect logic for each call. You can move that into a small WebSocket client and keep `createTopicSubscription` focused on topic filtering:
```ts
// websocketClient.ts
type MessageHandler = (event: MessageEvent) => void;
export function createReconnectingWebSocket(
url: string,
onMessage: MessageHandler
) {
let ws: WebSocket | null = null;
let reconnectTimeout: ReturnType<typeof setTimeout> | null = null;
let closed = false;
const connect = () => {
ws = new WebSocket(url);
ws.onopen = () => {/* no topic logic here */};
ws.onmessage = onMessage;
ws.onclose = () => {
if (!closed) {
reconnectTimeout = setTimeout(connect, 3000);
}
};
ws.onerror = console.error;
};
connect();
return {
send(data: unknown) {
if (ws?.readyState === WebSocket.OPEN) {
ws.send(JSON.stringify(data));
}
},
close() {
closed = true;
if (reconnectTimeout) clearTimeout(reconnectTimeout);
ws?.close();
},
};
}
```
Now `createTopicSubscription` can be a thin wrapper:
```ts
// topicSubscription.ts
import { buildWebSocketUrl, createReconnectingWebSocket } from "./websocketClient";
export function createTopicSubscription<T>(
topic: string,
onData: (data: T) => void
): () => void {
const client = createReconnectingWebSocket(buildWebSocketUrl("/ws"), (event) => {
try {
const message: ServerMessage = JSON.parse(event.data);
if (message.topic === topic) onData(message.data as T);
} catch (err) {
console.error(`Failed to parse WebSocket message for topic "${topic}":`, err);
}
});
client.send({ type: "subscribe", topic } satisfies ClientMessage);
return () => {
client.send({ type: "unsubscribe", topic } satisfies ClientMessage);
client.close();
};
}
```
This keeps your public API identical (`createTopicSubscription` returning a cleanup function) but:
- Connection/reconnect/auth logic is reusable and centralized.
- Topic-specific code is small and easy to reason about.
- Future multi-topic / shared-connection support can be added inside `websocketClient` without touching consumers.
</issue_to_address>
### Comment 4
<location path="apps/dockstat/src/lib/websocketEffects/topicSubscription.ts" line_range="40" />
<code_context>
const wsUrl = baseUrl.replace("http://", "ws://").replace("https://", "wss://")
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-insecure-websocket):** Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.
*Source: opengrep*
</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
+8
to
+17
| const useWebSocket = () => { | ||
| const settings = useGeneralSettings() | ||
| const [wsData, setWsData] = useState() | ||
|
|
||
| useEffect(() => { | ||
| const ws = api.ws.subscribe({ headers: getAuthHeaders() }) | ||
| ws.subscribe((msg) => { | ||
| setWsData(JSON.parse(String(msg.data))) | ||
| }) | ||
| }, []) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The new useWebSocket hook is syntactically incomplete and never cleans up the WebSocket connection.
onUnmount: () => is currently invalid syntax and will stop this file from compiling. Please either implement onUnmount or return a cleanup function from useEffect that closes/unsubscribes the WebSocket. Also, either remove the unused useGeneralSettings dependency or add it to the useEffect dependency array if it affects the connection configuration.
Comment on lines
+4
to
+10
| let wsHandler: typeof import("./websockets").DSWebSockerHandler | null = null | ||
|
|
||
| const BaseLogger = new Logger("DockStatAPI", [], (entry) => { | ||
| for (const client of logClients) { | ||
| // Lazy load and send logs to WebSocket | ||
| if (!wsHandler) { | ||
| try { | ||
| client.send(entry, true) | ||
| } catch (err) { | ||
| BaseLogger.error(`Failed to send log to client - ${JSON.stringify(err)}`) | ||
| wsHandler = require("./websockets").DSWebSockerHandler |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using require in an ESM/TypeScript module can break at runtime; prefer a dynamic import for lazy-loading.
Since this is an ESM project ("type": "module"), using CommonJS require here can throw at runtime in Node/Bun unless you set up createRequire. To keep lazy loading and avoid circular imports, use a dynamic import instead, e.g. const { DSWebSockerHandler } = await import("./websockets") (or inside the callback). You can then type wsHandler as WebSocketHandler | null rather than typeof import("./websockets").DSWebSockerHandler for clearer, accurate typing.
| * @param onData - Callback function called when data is received for this topic | ||
| * @returns Cleanup function to unsubscribe and close the connection | ||
| */ | ||
| export function createTopicSubscription<T>( |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider splitting URL/auth construction and reconnection logic into reusable WebSocket utilities so this helper focuses only on topic-specific behavior.
You can keep the same behavior but reduce complexity by separating “connection management” from “topic subscription” and by centralizing URL/auth construction.
1. Centralize URL & auth handling
Instead of re-implementing URL + token logic inside this helper, delegate to a small, reusable utility so other WebSocket consumers can share it:
// websocketClient.ts
import { getAuthHeaders } from "../api";
export function buildWebSocketUrl(path: string): string {
const baseUrl = import.meta.env.DOCKSTAT_API_PORT || "http://localhost:3030";
const wsBase = baseUrl.replace("http://", "ws://").replace("https://", "wss://");
const headers = getAuthHeaders(); // already knows how to read token
const token = headers.Authorization?.replace("Bearer ", "");
const query = token ? `?token=${encodeURIComponent(token)}` : "";
return `${wsBase}${path}${query}`;
}Then your helper becomes simpler and avoids duplicating auth concerns:
import { buildWebSocketUrl } from "./websocketClient";
export function createTopicSubscription<T>(
topic: string,
onData: (data: T) => void
): () => void {
const wsUrlWithAuth = buildWebSocketUrl("/ws");
// ...rest unchanged...
}2. Extract the reconnection/state machine into a small client
Right now createTopicSubscription owns flags + reconnect logic for each call. You can move that into a small WebSocket client and keep createTopicSubscription focused on topic filtering:
// websocketClient.ts
type MessageHandler = (event: MessageEvent) => void;
export function createReconnectingWebSocket(
url: string,
onMessage: MessageHandler
) {
let ws: WebSocket | null = null;
let reconnectTimeout: ReturnType<typeof setTimeout> | null = null;
let closed = false;
const connect = () => {
ws = new WebSocket(url);
ws.onopen = () => {/* no topic logic here */};
ws.onmessage = onMessage;
ws.onclose = () => {
if (!closed) {
reconnectTimeout = setTimeout(connect, 3000);
}
};
ws.onerror = console.error;
};
connect();
return {
send(data: unknown) {
if (ws?.readyState === WebSocket.OPEN) {
ws.send(JSON.stringify(data));
}
},
close() {
closed = true;
if (reconnectTimeout) clearTimeout(reconnectTimeout);
ws?.close();
},
};
}Now createTopicSubscription can be a thin wrapper:
// topicSubscription.ts
import { buildWebSocketUrl, createReconnectingWebSocket } from "./websocketClient";
export function createTopicSubscription<T>(
topic: string,
onData: (data: T) => void
): () => void {
const client = createReconnectingWebSocket(buildWebSocketUrl("/ws"), (event) => {
try {
const message: ServerMessage = JSON.parse(event.data);
if (message.topic === topic) onData(message.data as T);
} catch (err) {
console.error(`Failed to parse WebSocket message for topic "${topic}":`, err);
}
});
client.send({ type: "subscribe", topic } satisfies ClientMessage);
return () => {
client.send({ type: "unsubscribe", topic } satisfies ClientMessage);
client.close();
};
}This keeps your public API identical (createTopicSubscription returning a cleanup function) but:
- Connection/reconnect/auth logic is reusable and centralized.
- Topic-specific code is small and easy to reason about.
- Future multi-topic / shared-connection support can be added inside
websocketClientwithout touching consumers.
| ): () => void { | ||
| // Get the base API URL and convert to WebSocket protocol | ||
| const baseUrl = import.meta.env.DOCKSTAT_API_PORT || `http://localhost:3030` | ||
| const wsUrl = baseUrl.replace("http://", "ws://").replace("https://", "wss://") |
Contributor
There was a problem hiding this comment.
security (javascript.lang.security.detect-insecure-websocket): Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.
Source: opengrep
| ): () => void { | ||
| // Get the base API URL and convert to WebSocket protocol | ||
| const baseUrl = import.meta.env.DOCKSTAT_API_PORT || `http://localhost:3030` | ||
| const wsUrl = baseUrl.replace("http://", "ws://").replace("https://", "wss://") |
Contributor
There was a problem hiding this comment.
security (javascript.lang.security.detect-insecure-websocket): Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.
Source: opengrep
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
1 participant
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.
Summary by Sourcery
Introduce a unified WebSocket topic handler for API and frontend, wire logging/metrics over this channel, and normalize package metadata and packaging across DockStat packages with corresponding version bumps and changelogs.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Chores: