[client] Add dialWebSocket method to WASM client#5980
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a WASM-exposed Changes
Sequence DiagramsequenceDiagram
participant JS as JavaScript (Browser)
participant WASM as WASM module
participant NB as NetBird Client
participant Conn as WebSocket Conn
participant RL as Read Loop
JS->>WASM: dialWebSocket(url, protocols?, timeout?)
WASM->>WASM: validate inputs, create timeout context
WASM->>NB: Dial underlying stream
NB-->>WASM: net.Conn
WASM->>Conn: perform WS handshake (optional protocols)
Conn-->>WASM: established Conn
WASM-->>JS: resolve Promise with JS interface (send/close/events)
par background read loop
RL->>Conn: ReadMessage loop
alt Text
Conn-->>RL: text payload
RL->>JS: onmessage(string)
else Binary
Conn-->>RL: binary payload
RL->>JS: onmessage(Uint8Array)
else Close
Conn-->>RL: close(code, reason)
RL->>JS: onclose(code, reason)
else Error
Conn-->>RL: read error
RL->>JS: onerror(error)
end
end
JS->>Conn: send(data) / close()
Conn->>Conn: WriteText/WriteBinary / Close (idempotent)
Conn->>NB: frame writes over underlying connection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
lixmal
force-pushed
the
wasm-websocket-dial
branch
from
50f6577 to
a6cb0bd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
client/wasm/cmd/main.go (1)
573-587: Cachev.Length()to avoid redundant JS calls.Each
v.Length()call crosses the Go↔JS boundary viasyscall/js. The loop header re-invokes it on every iteration; caching it once is both a minor perf win and more idiomatic.♻️ Suggested refactor
func jsStringArray(v js.Value) ([]string, error) { if !v.InstanceOf(js.Global().Get("Array")) { return nil, fmt.Errorf("expected array") } - out := make([]string, v.Length()) - for i := 0; i < v.Length(); i++ { + n := v.Length() + out := make([]string, n) + for i := 0; i < n; i++ { el := v.Index(i) if el.Type() != js.TypeString { return nil, fmt.Errorf("element %d is not a string", i) } out[i] = el.String() } return out, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/cmd/main.go` around lines 573 - 587, The jsStringArray function makes repeated calls to v.Length() across the Go↔JS boundary; cache the length once (e.g., n := v.Length()) before allocating out and entering the for loop, then use that cached n in make([]string, n) and in the loop condition (for i := 0; i < n; i++) to avoid redundant syscall/js calls and improve performance.client/wasm/internal/websocket/websocket.go (1)
196-255: Optional: splitreadLoopto reduce cognitive complexity.SonarCloud flags cognitive complexity 25 (limit 20). Extracting the close-frame handling and message dispatch into small helpers (e.g.,
handleReadError,dispatchMessage) would make the control flow easier to follow and bring it under the threshold. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/websocket/websocket.go` around lines 196 - 255, The readLoop function is over the cognitive complexity threshold; refactor by extracting the error/close-frame handling and the message dispatch into small helpers: e.g., implement handleReadError(err error, conn *Conn, obj js.Value) that detects a *closeError, sets gotCloseFrame/closeCode/closeReason (or returns those values) and performs the RFC6455 conn.Close() response, and implement dispatchMessage(op ws.OpCode, payload []byte, obj js.Value) that looks up obj.Get("onmessage") and invokes the right JS value for ws.OpText and ws.OpBinary (using js.CopyBytesToJS for binary). Replace the corresponding inline blocks in readLoop (the errors.As closeError branch plus the onmessage switch) with calls to these helpers while preserving the existing onclose deferred logic and conn.closed check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/wasm/internal/websocket/websocket.go`:
- Line 188: The error message returned from the send branch currently only
mentions "string or Uint8Array" but jsToBytes accepts ArrayBuffer as well;
update the error text in the send path (the return in websocket.go) to include
ArrayBuffer so callers see "use string, Uint8Array, or ArrayBuffer". Locate the
send-related function and the jsToBytes usage to ensure the message matches the
accepted types and adjust the fmt.Errorf message accordingly.
- Around line 232-238: The error handling should use errors.Is to detect EOF and
skip treating net.ErrClosed as an error: change the condition that currently
checks "if err != io.EOF { ... onerror.Invoke(...)" to use errors.Is(err,
io.EOF) and also ignore net.ErrClosed (i.e., if errors.Is(err, net.ErrClosed) or
errors.Is(err, io.EOF) then simply return without calling the JS onerror
callback). Ensure you import the errors and net packages and keep the existing
obj.Get("onerror")/onerror.Invoke(js.ValueOf(err.Error())) behavior for real
errors.
- Around line 47-59: The buffered reader returned by d.Dial (seen in the Dial
call that assigns conn, br, _, err) may contain server frames from the upgrade;
calling ws.PutReader(br) unconditionally discards them and later
wsutil.ReadServerMessage on Conn.conn will miss those frames. Fix: before
returning from the function that builds &Conn (the Dial handling code), check
br.Buffered() > 0; if buffered data exists either (a) wrap conn in an
io.Reader/ReadCloser that first serves br's buffered bytes and then delegates to
conn, or (b) drain frames from br using ws.ReadFrame loop and process/queue them
into Conn before calling ws.PutReader(br). Ensure ws.PutReader(br) is only
called after buffered bytes are consumed or preserved so Conn.conn+Conn methods
(e.g., wsutil.ReadServerMessage) will see the server-sent frames.
---
Nitpick comments:
In `@client/wasm/cmd/main.go`:
- Around line 573-587: The jsStringArray function makes repeated calls to
v.Length() across the Go↔JS boundary; cache the length once (e.g., n :=
v.Length()) before allocating out and entering the for loop, then use that
cached n in make([]string, n) and in the loop condition (for i := 0; i < n; i++)
to avoid redundant syscall/js calls and improve performance.
In `@client/wasm/internal/websocket/websocket.go`:
- Around line 196-255: The readLoop function is over the cognitive complexity
threshold; refactor by extracting the error/close-frame handling and the message
dispatch into small helpers: e.g., implement handleReadError(err error, conn
*Conn, obj js.Value) that detects a *closeError, sets
gotCloseFrame/closeCode/closeReason (or returns those values) and performs the
RFC6455 conn.Close() response, and implement dispatchMessage(op ws.OpCode,
payload []byte, obj js.Value) that looks up obj.Get("onmessage") and invokes the
right JS value for ws.OpText and ws.OpBinary (using js.CopyBytesToJS for
binary). Replace the corresponding inline blocks in readLoop (the errors.As
closeError branch plus the onmessage switch) with calls to these helpers while
preserving the existing onclose deferred logic and conn.closed check.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5d2a4a7-9b67-4f71-9ea7-fe13cfbe607e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
client/wasm/cmd/main.goclient/wasm/internal/websocket/websocket.gogo.mod
lixmal
force-pushed
the
wasm-websocket-dial
branch
from
a6cb0bd to
4980336
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/wasm/internal/websocket/websocket.go (1)
143-166:sendreturn type is inconsistent.On success it returns
true, on write failurefalse, but on argument/type errors it returns a human-readable string ("send requires...", orerr.Error()fromjsToBytes). JS callers then cannot safely distinguish success from failure with a simple truthiness check — a non-empty error string is truthy. Consider returningfalse(andlog.Errorf-ing the reason) for all failure paths, or throwing a JS error viajs.Global().Get("Error").New(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/websocket/websocket.go` around lines 143 - 166, The send JS function currently returns mixed types (string on arg/type errors, bool on success/fail), break this by making all failure paths return js.ValueOf(false) and logging the reason via log.Errorf; specifically, in the obj.Set("send", js.FuncOf(...)) handler update the missing-argument branch to log an error and return js.ValueOf(false), change the jsToBytes error branch to log.Errorf("failed to convert JS value to bytes: %v", err) and return js.ValueOf(false), and keep the existing conn.WriteText/conn.WriteBinary failure branches but ensure they also return js.ValueOf(false) (already present) so callers can reliably use truthiness to detect success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/wasm/internal/websocket/websocket.go`:
- Around line 143-173: The send/close js.FuncOf callbacks created inline in
dialWebSocket are never released and leak; store each js.Func (e.g., sendFunc
and closeFunc) before calling obj.Set("send", ...) and obj.Set("close", ...),
and ensure you call sendFunc.Release() and closeFunc.Release() when the
connection is closed—best place is when readLoop exits (or immediately after
conn.Close() completes) so the callbacks are removed from Go's callback table;
update dialWebSocket to create named js.Funcs and release them on connection
teardown.
---
Nitpick comments:
In `@client/wasm/internal/websocket/websocket.go`:
- Around line 143-166: The send JS function currently returns mixed types
(string on arg/type errors, bool on success/fail), break this by making all
failure paths return js.ValueOf(false) and logging the reason via log.Errorf;
specifically, in the obj.Set("send", js.FuncOf(...)) handler update the
missing-argument branch to log an error and return js.ValueOf(false), change the
jsToBytes error branch to log.Errorf("failed to convert JS value to bytes: %v",
err) and return js.ValueOf(false), and keep the existing
conn.WriteText/conn.WriteBinary failure branches but ensure they also return
js.ValueOf(false) (already present) so callers can reliably use truthiness to
detect success.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a573371-2fb1-4474-be68-1acc297f2835
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
client/wasm/cmd/main.goclient/wasm/internal/websocket/websocket.gogo.mod
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- client/wasm/cmd/main.go
lixmal
force-pushed
the
wasm-websocket-dial
branch
from
4980336 to
3373329
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
client/wasm/cmd/main.go (1)
550-556: Type-checkargs[2]before calling.Int().
js.Value.Int()panics when the underlying JS value is not a number. A JS caller passingdialWebSocket(url, [], "5000")will crash the WASM runtime rather than get a clean promise rejection. Validate withargs[2].Type() != js.TypeNumber(same pattern used forportelsewhere) before extracting.🛡️ Proposed fix
timeout := dialWebSocketTimeout if len(args) >= 3 && !args[2].IsNull() && !args[2].IsUndefined() { - timeoutMs := args[2].Int() + if args[2].Type() != js.TypeNumber { + return js.ValueOf("error: timeoutMs must be a number") + } + timeoutMs := args[2].Int() if timeoutMs <= 0 { return js.ValueOf("error: timeout must be positive") } timeout = time.Duration(timeoutMs) * time.Millisecond }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/cmd/main.go` around lines 550 - 556, The code reads timeoutMs via args[2].Int() which will panic if args[2] is not a JS number; before calling Int() check args[2].Type() == js.TypeNumber (same pattern used for port handling) and if it's not a number return a JS error/promise rejection like "error: timeout must be a positive number"; only then parse timeoutMs, validate timeoutMs > 0 and set timeout = time.Duration(timeoutMs) * time.Millisecond to avoid runtime panics when callers pass non-number values to dialWebSocket.client/wasm/internal/websocket/websocket.go (2)
61-66: Returnbrto the pool when there are no buffered bytes.
bris always wrapped inbufferedConn, even whenbr.Buffered() == 0. That adds an unnecessary reader layer and—more importantly—ws.PutReader(br)is never called, so the internal*bufio.Readerpool is never replenished. Since this is an internal micro‑optimization in gobwas/ws, the cost is low, but the intent stated in the comment ("drained before reading from conn") is only meaningful whenbractually has data.♻️ Proposed tweak
- if br != nil { - conn = &bufferedConn{Conn: conn, r: io.MultiReader(br, conn)} - } + if br != nil { + if br.Buffered() > 0 { + conn = &bufferedConn{Conn: conn, r: io.MultiReader(br, conn)} + } else { + ws.PutReader(br) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/websocket/websocket.go` around lines 61 - 66, The current block always wraps br in bufferedConn even when br.Buffered() == 0, leaking bufio.Reader instances and adding an unnecessary layer; change the logic in the conditional that handles br so that if br != nil and br.Buffered() == 0 you call ws.PutReader(br) and do not wrap conn, otherwise (br != nil and br.Buffered() > 0) wrap conn = &bufferedConn{Conn: conn, r: io.MultiReader(br, conn)}; ensure you reference br, bufferedConn, conn, and ws.PutReader in the updated conditional so readers are returned to the pool when empty and only wrapped when they actually contain buffered bytes.
191-198: JS properties retain references to released callbacks; consider undefined-ing them earlier.When
readLoopexits,sendFuncandcloseFuncare released, but the JS object still hasobj.sendandobj.closepointing at them. Per Go's syscall/js documentation, invoking a released function logs "call to released function" to the browser console and returns without executing the handler.While this is safe from a crash perspective, the JS caller experiences silent failure—no exception is thrown. For consistency with standard WebSocket semantics (where
send()after close raisesInvalidStateError), consider setting these properties tojs.Undefined()before releasing, so subsequent calls surface an explicitTypeErrorin JavaScript.♻️ Suggested tweak
go func() { - defer sendFunc.Release() - defer closeFunc.Release() readLoop(conn, obj) + // Clear JS properties before releasing so post-close calls + // raise TypeError instead of failing silently. + obj.Set("send", js.Undefined()) + obj.Set("close", js.Undefined()) + sendFunc.Release() + closeFunc.Release() }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/websocket/websocket.go` around lines 191 - 198, The JS callbacks on the exported websocket object still point to released Go functions; update the goroutine that calls readLoop so that before releasing sendFunc and closeFunc you set the JS properties to js.Undefined() (i.e., call obj.Set("send", js.Undefined()) and obj.Set("close", js.Undefined())), then call sendFunc.Release() and closeFunc.Release(); keep readLoop(conn, obj) behavior but ensure the undefineding happens prior to Release() to surface a TypeError to JS callers instead of a silent "call to released function".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/wasm/internal/websocket/websocket.go`:
- Around line 216-265: The onclose invocation is inconsistent: update
handleReadError/readLoop to always produce a closeError (or non-nil sentinel) so
invokeOnClose always receives a code and reason; specifically, in
handleReadError map io.EOF and net.ErrClosed to a closeError with code 1005 and
empty reason, map other non-WebSocket-close errors to a closeError with code
1006 and reason err.Error(), keep the existing branch for errors.As(err, &ce)
returning server-provided closeError, and ensure invokeOnClose (used by
readLoop) then always calls onclose with int(ce.code) and ce.reason so JS
handlers receive a consistent (code, reason) tuple.
---
Nitpick comments:
In `@client/wasm/cmd/main.go`:
- Around line 550-556: The code reads timeoutMs via args[2].Int() which will
panic if args[2] is not a JS number; before calling Int() check args[2].Type()
== js.TypeNumber (same pattern used for port handling) and if it's not a number
return a JS error/promise rejection like "error: timeout must be a positive
number"; only then parse timeoutMs, validate timeoutMs > 0 and set timeout =
time.Duration(timeoutMs) * time.Millisecond to avoid runtime panics when callers
pass non-number values to dialWebSocket.
In `@client/wasm/internal/websocket/websocket.go`:
- Around line 61-66: The current block always wraps br in bufferedConn even when
br.Buffered() == 0, leaking bufio.Reader instances and adding an unnecessary
layer; change the logic in the conditional that handles br so that if br != nil
and br.Buffered() == 0 you call ws.PutReader(br) and do not wrap conn, otherwise
(br != nil and br.Buffered() > 0) wrap conn = &bufferedConn{Conn: conn, r:
io.MultiReader(br, conn)}; ensure you reference br, bufferedConn, conn, and
ws.PutReader in the updated conditional so readers are returned to the pool when
empty and only wrapped when they actually contain buffered bytes.
- Around line 191-198: The JS callbacks on the exported websocket object still
point to released Go functions; update the goroutine that calls readLoop so that
before releasing sendFunc and closeFunc you set the JS properties to
js.Undefined() (i.e., call obj.Set("send", js.Undefined()) and obj.Set("close",
js.Undefined())), then call sendFunc.Release() and closeFunc.Release(); keep
readLoop(conn, obj) behavior but ensure the undefineding happens prior to
Release() to surface a TypeError to JS callers instead of a silent "call to
released function".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7cdedc5-3f3b-4b23-b90a-c3984044c8bc
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
client/wasm/cmd/main.goclient/wasm/internal/websocket/websocket.gogo.mod
✅ Files skipped from review due to trivial changes (1)
- go.mod
lixmal
force-pushed
the
wasm-websocket-dial
branch
2 times, most recently
from
9962c77 to
a463c29
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/wasm/cmd/main.go (1)
530-574: Optional: group the new timeout constant and trim cognitive complexity.Two small refactors:
dialWebSocketTimeoutis declared in isolation at line 530; the other timeouts (clientStartTimeout,clientStopTimeout,pingTimeout,defaultSSHDetectionTimeout) live in the top const block at lines 24–34. Moving it there is more consistent.- SonarCloud flags cognitive complexity 21 (limit 20) on
createDialWebSocketMethod. Extracting the protocols/timeout arg parsing into helpers drops it below the threshold and improves readability.♻️ Proposed refactor
const ( clientStartTimeout = 30 * time.Second clientStopTimeout = 10 * time.Second pingTimeout = 10 * time.Second defaultLogLevel = "warn" defaultSSHDetectionTimeout = 20 * time.Second + dialWebSocketTimeout = 30 * time.Second icmpEchoRequest = 8 icmpCodeEcho = 0 pingBufferSize = 1500 )-const dialWebSocketTimeout = 30 * time.Second - func createDialWebSocketMethod(client *netbird.Client) js.Func { return js.FuncOf(func(_ js.Value, args []js.Value) any { if len(args) < 1 || args[0].Type() != js.TypeString { return js.ValueOf("error: dialWebSocket requires a URL string argument") } - url := args[0].String() - var protocols []string - if len(args) >= 2 && !args[1].IsNull() && !args[1].IsUndefined() { - arr, err := jsStringArray(args[1]) - if err != nil { - return js.ValueOf(fmt.Sprintf("error: protocols: %v", err)) - } - protocols = arr - } - - timeout := dialWebSocketTimeout - if len(args) >= 3 && !args[2].IsNull() && !args[2].IsUndefined() { - if args[2].Type() != js.TypeNumber { - return js.ValueOf("error: timeoutMs must be a number") - } - timeoutMs := args[2].Int() - if timeoutMs <= 0 { - return js.ValueOf("error: timeout must be positive") - } - timeout = time.Duration(timeoutMs) * time.Millisecond - } + protocols, perr := parseProtocolsArg(args) + if perr != nil { + return js.ValueOf(perr.Error()) + } + timeout, terr := parseTimeoutArg(args, dialWebSocketTimeout) + if terr != nil { + return js.ValueOf(terr.Error()) + } return createPromise(func(resolve, reject js.Value) { ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() conn, err := nbwebsocket.Dial(ctx, client, url, protocols) if err != nil { reject.Invoke(js.ValueOf(fmt.Sprintf("dial websocket: %v", err))) return } resolve.Invoke(nbwebsocket.NewJSInterface(conn)) }) }) } + +func parseProtocolsArg(args []js.Value) ([]string, error) { + if len(args) < 2 || args[1].IsNull() || args[1].IsUndefined() { + return nil, nil + } + arr, err := jsStringArray(args[1]) + if err != nil { + return nil, fmt.Errorf("error: protocols: %w", err) + } + return arr, nil +} + +func parseTimeoutArg(args []js.Value, def time.Duration) (time.Duration, error) { + if len(args) < 3 || args[2].IsNull() || args[2].IsUndefined() { + return def, nil + } + if args[2].Type() != js.TypeNumber { + return 0, fmt.Errorf("error: timeoutMs must be a number") + } + ms := args[2].Int() + if ms <= 0 { + return 0, fmt.Errorf("error: timeout must be positive") + } + return time.Duration(ms) * time.Millisecond, nil +}Based on static analysis hint: SonarCloud flagged cognitive complexity 21 exceeds the allowed 20.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/cmd/main.go` around lines 530 - 574, Move the dialWebSocketTimeout constant into the existing top const block with clientStartTimeout/clientStopTimeout/pingTimeout/defaultSSHDetectionTimeout for consistency, and reduce cognitive complexity in createDialWebSocketMethod by extracting the protocol and timeout parsing into small helpers (e.g., parseProtocolsArg(js.Value) ([]string, error) and parseTimeoutArg(js.Value, default time.Duration) (time.Duration, error)); have createDialWebSocketMethod call these helpers (and still validate arg types/values and return the same error strings) so the main function focuses only on promise creation and dialing and the overall complexity drops below the SonarCloud threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/wasm/internal/websocket/websocket.go`:
- Around line 195-203: The goroutine calling readLoop(conn, obj) doesn't ensure
conn.Close() on non-close-frame exit paths; add an unconditional defer
conn.Close() at the start of that goroutine (before calling readLoop) so the
underlying net.Conn is closed for EOF, net.ErrClosed, or other read errors—this
is safe because Conn.Close is idempotent via closeOnce; keep the existing
obj.Set("send"/"close") and sendFunc.Release()/closeFunc.Release() cleanup after
readLoop returns.
---
Nitpick comments:
In `@client/wasm/cmd/main.go`:
- Around line 530-574: Move the dialWebSocketTimeout constant into the existing
top const block with
clientStartTimeout/clientStopTimeout/pingTimeout/defaultSSHDetectionTimeout for
consistency, and reduce cognitive complexity in createDialWebSocketMethod by
extracting the protocol and timeout parsing into small helpers (e.g.,
parseProtocolsArg(js.Value) ([]string, error) and parseTimeoutArg(js.Value,
default time.Duration) (time.Duration, error)); have createDialWebSocketMethod
call these helpers (and still validate arg types/values and return the same
error strings) so the main function focuses only on promise creation and dialing
and the overall complexity drops below the SonarCloud threshold.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf63aab2-8e35-44d5-a62f-6e808b5e98ad
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
client/wasm/cmd/main.goclient/wasm/internal/websocket/websocket.gogo.mod
✅ Files skipped from review due to trivial changes (1)
- go.mod
lixmal
force-pushed
the
wasm-websocket-dial
branch
2 times, most recently
from
4b06d0f to
55db2fa
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/wasm/internal/websocket/websocket.go`:
- Around line 252-268: handleReadError currently calls conn.Close() on receiving
a server close frame, which sends StatusNormalClosure; change it to mirror the
server's close code/reason by invoking a close method that accepts the received
code and reason (e.g., use or add a method like Conn.CloseWith(code int, reason
string) or extend Conn.Close to accept those params) and pass the fields from
the parsed closeError (closeError.Code and closeError.Reason) when ce is
non-nil; keep the existing debug/error path but ensure the echoed close frame
uses ce.Code and ce.Reason instead of the fixed StatusNormalClosure.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1563b869-09ed-4286-8959-29919d2a6776
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
client/wasm/cmd/main.goclient/wasm/internal/websocket/websocket.gogo.mod
✅ Files skipped from review due to trivial changes (1)
- go.mod
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
client/wasm/internal/websocket/websocket.go (2)
60-70: Minor:brpool reuse is skipped when handshake buffered frames exist.When
br.Buffered() > 0, the*bufio.Readeris retained insidebufferedConnviaio.MultiReaderand never returned to the pool viaws.PutReader. It'll be GC'd eventually, so this is not a leak — just a missed pool-reuse opportunity. If you care about steady-state allocations, you could swap theMultiReaderfor a small wrapper that drainsbrfirst, then callsws.PutReader(br)and falls through toconn. Feel free to ignore if not worth the complexity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/websocket/websocket.go` around lines 60 - 70, When br.Buffered() > 0 you currently wrap conn with io.MultiReader(br, conn) and never return br to the ws pool; replace that with a small wrapper reader (e.g., drainThenPutReader) used by bufferedConn that reads from br until exhausted, calls ws.PutReader(br) immediately after the last byte is consumed, then delegates to conn; update the bufferedConn construction to use this wrapper instead of io.MultiReader and ensure br is always returned to the pool once drained.
295-301: Use gobwas/ws helpers for cleaner close frame parsing.
parseClosePayloadduplicatesws.ParseCloseFrameDatafrom the already-imported gobwas/ws library, which handles the<2 bytes → StatusNoStatusRcvdcase. Using the library helper plusws.StatusNoStatusRcvdreplaces the magic1005constant and keeps parsing consistent with the rest of the gobwas/ws surface used throughout this file.♻️ Optional refactor
-func parseClosePayload(payload []byte) (uint16, string) { - if len(payload) < 2 { - return 1005, "" // RFC 6455: No Status Rcvd - } - code := binary.BigEndian.Uint16(payload[:2]) - return code, string(payload[2:]) -} +func parseClosePayload(payload []byte) (uint16, string) { + if len(payload) < 2 { + return uint16(ws.StatusNoStatusRcvd), "" + } + code, reason := ws.ParseCloseFrameData(payload) + return uint16(code), reason +}If adopted, the
encoding/binaryimport can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/websocket/websocket.go` around lines 295 - 301, Replace the custom parseClosePayload implementation with the gobwas/ws helper: call ws.ParseCloseFrameData(payload) and return its parsed code and reason, using ws.StatusNoStatusRcvd instead of the hard-coded 1005; update any return handling to match ws.ParseCloseFrameData's signature and remove the now-unused encoding/binary import. Ensure you reference and use ws.ParseCloseFrameData and ws.StatusNoStatusRcvd in the parseClosePayload function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/wasm/internal/websocket/websocket.go`:
- Around line 60-70: When br.Buffered() > 0 you currently wrap conn with
io.MultiReader(br, conn) and never return br to the ws pool; replace that with a
small wrapper reader (e.g., drainThenPutReader) used by bufferedConn that reads
from br until exhausted, calls ws.PutReader(br) immediately after the last byte
is consumed, then delegates to conn; update the bufferedConn construction to use
this wrapper instead of io.MultiReader and ensure br is always returned to the
pool once drained.
- Around line 295-301: Replace the custom parseClosePayload implementation with
the gobwas/ws helper: call ws.ParseCloseFrameData(payload) and return its parsed
code and reason, using ws.StatusNoStatusRcvd instead of the hard-coded 1005;
update any return handling to match ws.ParseCloseFrameData's signature and
remove the now-unused encoding/binary import. Ensure you reference and use
ws.ParseCloseFrameData and ws.StatusNoStatusRcvd in the parseClosePayload
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fd82812-981e-4423-a222-b1250dce98ab
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
client/wasm/cmd/main.goclient/wasm/internal/websocket/websocket.gogo.mod
✅ Files skipped from review due to trivial changes (2)
- go.mod
- client/wasm/cmd/main.go
lixmal
force-pushed
the
wasm-websocket-dial
branch
from
55db2fa to
fbd74d3
Compare
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/wasm/internal/websocket/websocket.go (1)
152-159: Doc comment missedArrayBuffer.
jsToBytesacceptsUint8ArrayandArrayBuffer, but the doc comment advertises onlystring|Uint8Array.✏️ Suggested tweak
// NewJSInterface creates a JavaScript object wrapping the WebSocket connection. -// It exposes: send(string|Uint8Array), close(), and callback properties -// onmessage, onclose, onerror. +// It exposes: send(string|Uint8Array|ArrayBuffer), close(), and callback +// properties onmessage, onclose, onerror.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/websocket/websocket.go` around lines 152 - 159, Update the doc comment for NewJSInterface to reflect that jsToBytes accepts both Uint8Array and ArrayBuffer (in addition to string), e.g., change the exposed send type description from "string|Uint8Array" to "string|Uint8Array|ArrayBuffer" and/or add a note referencing jsToBytes so the documentation matches the actual accepted input types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/wasm/internal/websocket/websocket.go`:
- Around line 152-159: Update the doc comment for NewJSInterface to reflect that
jsToBytes accepts both Uint8Array and ArrayBuffer (in addition to string), e.g.,
change the exposed send type description from "string|Uint8Array" to
"string|Uint8Array|ArrayBuffer" and/or add a note referencing jsToBytes so the
documentation matches the actual accepted input types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbe0b2de-30ed-4c34-9544-c471d5ba9f8a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
client/wasm/cmd/main.goclient/wasm/internal/websocket/websocket.gogo.mod
✅ Files skipped from review due to trivial changes (1)
- go.mod
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.
Describe your changes
Adds a
dialWebSocket(url, protocols?, timeoutMs?)method to the WASM client. Opens a WebSocket over a NetBird TCP connection (viaclient.Dial) using gobwas/ws for the handshake and framing, and exposes a smallsend/close/onmessage/onclose/onerrorinterface to JS.client/wasm/internal/websocketpackage wrapping a netbird net.Conn with WS framingdialWebSocketfrom the WASM client with optional subprotocols and timeoutgobwas/wsas a dependencyWhy a custom WS layer and not the browser's native WebSocket
A browser-native
WebSocketopens its own TCP/TLS connection through the browser's network stack, which bypasses the NetBird tunnel. That's unusable for reaching peers inside the overlay.Alternatives considered:
WebSocket— opens its own socket outside the NetBird tunnel.jsbuild tag that delegates to the native browser WebSocket, so it hits the same problem.client.Dialas a raw duplex stream to JS and implement WS framing in TypeScript — doable but we'd be reimplementing framing, masking, and control-frame handling just to replace a thin Go wrapper. No existing JS WS client library can use a custom transport as its socket, so any approach on the JS side ends up being bespoke.Going through Go with gobwas/ws is a small amount of code that handles framing on bytes alone, with no dependency on browser networking APIs, so it works fine in the wasm target over a netbird
net.Conn.Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Internal WASM API, no public-facing docs page.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit