bito-code-review[bot] commented on code in PR #40856:
URL: https://github.com/apache/superset/pull/40856#discussion_r3376524219
##########
superset-websocket/src/index.ts:
##########
@@ -159,6 +159,67 @@ export const setLastFirehoseId = (id: string): void => {
lastFirehoseId = id;
};
+// WebSocket close code used when a connection is refused because a configured
+// connection limit has been reached (1013 = "Try Again Later").
+const CONNECTION_LIMIT_CLOSE_CODE = 1013;
+
+/**
+ * Returns whether the socket with the given id is currently active, i.e. it is
+ * still registered and its underlying connection is in an active readyState.
+ *
+ * Closed sockets are only removed from the registries asynchronously (via the
+ * `checkSockets`/`cleanChannel` GC routines), so connection-limit checks must
+ * filter on live socket state rather than trusting the raw registry sizes.
+ */
+const isSocketActive = (socketId: string): boolean => {
+ const socketInstance = sockets[socketId];
+ return (
+ !!socketInstance &&
+ SOCKET_ACTIVE_STATES.includes(socketInstance.ws.readyState)
+ );
+};
+
+/**
+ * Counts the sockets in the global registry that are still active.
+ */
+const activeSocketCount = (): number =>
+ Object.keys(sockets).filter(isSocketActive).length;
+
+/**
+ * Counts the active sockets currently registered on the given channel.
+ */
+const activeChannelSocketCount = (channel: string): number =>
+ channels[channel]?.sockets.filter(isSocketActive).length ?? 0;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new helpers</b></div>
<div id="fix">
The new helper functions `isSocketActive`, `activeSocketCount`, and
`activeChannelSocketCount` lack direct unit tests. BITO.md rule 11730
recommends comprehensive unit tests for new tools/features. Since these are
private (module-scoped), add `export` to make them testable. Existing
`connectionLimitReason` tests (lines 502-528) verify behavior indirectly, but
isolated tests for each helper would improve coverage.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset-websocket/src/index.ts
+++ superset-websocket/src/index.ts
@@ -171,7 +171,7 @@ const CONNECTION_LIMIT_CLOSE_CODE = 1013;
* counting them would spuriously reject new connections even when no
active
* connection is consuming capacity.
*/
-const isSocketActive = (socketId: string): boolean => {
+export const isSocketActive = (socketId: string): boolean => {
const socketInstance = sockets[socketId];
return (
!!socketInstance &&
@@ -182,12 +182,12 @@ const isSocketActive = (socketId: string): boolean =>
{
/**
* Counts the sockets in the global registry that are still active.
*/
-const activeSocketCount = (): number =>
+export const activeSocketCount = (): number =>
Object.keys(sockets).filter(isSocketActive).length;
/**
* Counts the active sockets currently registered on the given channel.
*/
-const activeChannelSocketCount = (channel: string): number =>
+export const activeChannelSocketCount = (channel: string): number =>
channels[channel]?.sockets.filter(isSocketActive).length ?? 0;
```
</div>
</details>
</div>
<small><i>Code Review Run #9bfee8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
