Update mediasoup version to 0.19.1 #378
Conversation
WalkthroughVersion bumps to crate and project, addition of exposeInternalIp (and portRange/flags on TransportListenInfo) to types/structs, propagation of exposeInternalIp in WebRTC listen infos, and corresponding test updates. Minor formatting changes and removal of framemarking RTP header extensions from specific pipe transport test expectations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant WebRtcTransport.Options as Options
participant Normalizer as Normalizer
participant WebRtcServer as WebRtcServer
Caller->>Options: provide listenIps [{ip, announcedIp?, port?, exposeInternalIp?}]
Options->>Normalizer: normalize listenIps
Note right of Normalizer: For each listenIp<br/>set exposeInternalIp = provided or false
Normalizer-->>Options: listenInfos [{protocol, ip, announcedAddress?, port?, exposeInternalIp}]
Options->>WebRtcServer: create with listenInfos
WebRtcServer-->>Caller: initialized (listenInfos include exposeInternalIp)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
h3poteto
force-pushed
the
update/mediasoup/0.19.1
branch
from
12a01e5 to
12cb31c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/transport_listen_info_test.exs (1)
6-20: Fix expected structs to include exposeInternalIp: false.
create/3setsexposeInternalIpexplicitly; tests currently expect it to be nil. This is causing the failing test in CI.Apply:
test "create" do - assert %Mediasoup.TransportListenInfo{ - announcedAddress: "1.1.1.1", - ip: "127.0.0.1", - protocol: :udp - } == + assert %Mediasoup.TransportListenInfo{ + announcedAddress: "1.1.1.1", + ip: "127.0.0.1", + protocol: :udp, + exposeInternalIp: false + } == TransportListenInfo.create( %{ip: "127.0.0.1", announcedAddress: "1.1.1.1"}, :udp, false ) - assert %Mediasoup.TransportListenInfo{ip: "127.0.0.1", protocol: :tcp} == - TransportListenInfo.create("127.0.0.1", :tcp, false) + assert %Mediasoup.TransportListenInfo{ + ip: "127.0.0.1", + protocol: :tcp, + exposeInternalIp: false + } == + TransportListenInfo.create("127.0.0.1", :tcp, false)
🧹 Nitpick comments (1)
lib/transport_listen_info.ex (1)
14-18: New optional fields look good; consider narrowing types for clarity.
portRangeandflagsare typed asmap(). If their shapes are known (e.g.,%{min: integer, max: integer}), tighten the types to catch mistakes earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/plain_transport.ex(1 hunks)lib/transport_listen_info.ex(2 hunks)lib/webrtc_transport.ex(1 hunks)test/integration/test_plain_transport.ex(1 hunks)test/integration/test_webrtc_transport.ex(1 hunks)test/transport_listen_info_test.exs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/integration/test_plain_transport.ex (1)
lib/router.ex (2)
create_plain_transport(148-154)create_plain_transport(156-158)
test/transport_listen_info_test.exs (1)
lib/transport_listen_info.ex (2)
create(57-64)create(66-77)
lib/plain_transport.ex (1)
lib/transport_listen_info.ex (2)
create(57-64)create(66-77)
🪛 GitHub Actions: test
test/transport_listen_info_test.exs
[error] 6-6: mix test failed: Mediasoup.TransportListenInfoTest: 'test create' at test/transport_listen_info_test.exs:6.
test/integration/test_webrtc_transport.ex
[error] 452-452: Dialyzer: no_return. Function create_with_webrtc_server_succeeds/1 has no local return.
[error] 456-456: Dialyzer: call will not succeed. Mediasoup.Worker.create_webrtc_server(...) breaks the contract (t(), Mediasoup.WebRtcServer.Options.t()) -> {:ok, Mediasoup.WebRtcServer.t()} | {:error, :terminated}.
🔇 Additional comments (3)
lib/transport_listen_info.ex (1)
24-34: Struct extension LGTM.Fields align with the updated type and mediasoup API.
test/integration/test_plain_transport.ex (1)
220-223: Formatting-only change. LGTM.lib/webrtc_transport.ex (1)
138-140: Good addition of exposeInternalIp with sensible default.Matches the new TransportListenInfo field and keeps backwards compatibility.
| Map.get(option, :listen_info) || | ||
| TransportListenInfo.create(listen_ip, "udp", false, port), | ||
| rtcp_listen_info: | ||
| Map.get(option, :rtcp_listen_info) || | ||
| TransportListenInfo.create(listen_ip, "udp", nil) | ||
| TransportListenInfo.create(listen_ip, "udp", false, nil) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
Protocol should be atom, and avoid passing nil port to create/4.
Current calls pass "udp" (string) and nil port. This conflicts with the typed :tcp | :udp protocol and unnecessarily sets port: nil.
Apply:
- listen_info:
- Map.get(option, :listen_info) ||
- TransportListenInfo.create(listen_ip, "udp", false, port),
+ listen_info:
+ Map.get(option, :listen_info) ||
+ if is_integer(port) do
+ TransportListenInfo.create(listen_ip, :udp, false, port)
+ else
+ TransportListenInfo.create(listen_ip, :udp, false)
+ end,
@@
- rtcp_listen_info:
- Map.get(option, :rtcp_listen_info) ||
- TransportListenInfo.create(listen_ip, "udp", false, nil)
+ rtcp_listen_info:
+ Map.get(option, :rtcp_listen_info) ||
+ TransportListenInfo.create(listen_ip, :udp, false)To confirm there are no remaining string protocols:
🏁 Script executed:
#!/bin/bash
rg -nP 'TransportListenInfo\.create\([^)]*"(udp|tcp)"' -C2 --type=elixirLength of output: 849
Use atoms for protocol and avoid passing nil ports to TransportListenInfo.create
Protocol should be an atom (:tcp | :udp) and avoid passing nil for port. Apply the diff below for lib/plain_transport.ex and also update lib/pipe_transport.ex (line 55) to use :udp and not pass nil (wrap with an is_integer(port) conditional if needed).
- listen_info:
- Map.get(option, :listen_info) ||
- TransportListenInfo.create(listen_ip, "udp", false, port),
+ listen_info:
+ Map.get(option, :listen_info) ||
+ if is_integer(port) do
+ TransportListenInfo.create(listen_ip, :udp, false, port)
+ else
+ TransportListenInfo.create(listen_ip, :udp, false)
+ end,
@@
- rtcp_listen_info:
- Map.get(option, :rtcp_listen_info) ||
- TransportListenInfo.create(listen_ip, "udp", false, nil)
+ rtcp_listen_info:
+ Map.get(option, :rtcp_listen_info) ||
+ TransportListenInfo.create(listen_ip, :udp, false)- lib/pipe_transport.ex:55 — replace TransportListenInfo.create(listen_ip, "udp", port) with an atom protocol (:udp) and avoid passing nil (e.g., conditional on is_integer(port)).
🤖 Prompt for AI Agents
In lib/plain_transport.ex around lines 82 to 87, change the protocol argument
from the string "udp"/"tcp" to the atom :udp/:tcp and stop passing nil for the
port to TransportListenInfo.create; call TransportListenInfo.create(listen_ip,
:udp, false, port) only when port is an integer (otherwise omit or use a
separate constructor), and similarly replace "tcp" with :tcp where used; also
update lib/pipe_transport.ex line 55 to use :udp instead of "udp" and guard the
call so you only pass the port when is_integer(port) (e.g., conditional or
choose an overload that doesn't receive nil).
lib/transport_listen_info.ex
Outdated
| @spec create(binary() | %{:ip => any(), optional(any()) => any()}, any(), boolean()) :: struct() | ||
| def create(ip, protocol, expose) do | ||
| listen_ip = normalize_listen_ip(ip) | ||
| struct(__MODULE__, Map.merge(listen_ip, %{:protocol => protocol})) | ||
|
|
||
| struct( | ||
| __MODULE__, | ||
| Map.merge(listen_ip, %{:protocol => protocol, :exposeInternalIp => expose}) | ||
| ) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Narrow protocol type and add missing spec for create/4.
Spec currently uses any() and there’s no spec for the 4‑arity function. Tighten both to improve Dialyzer precision.
Apply:
-@spec create(binary() | %{:ip => any(), optional(any()) => any()}, any(), boolean()) :: struct()
-def create(ip, protocol, expose) do
+@spec create(binary() | listen_ip(), :tcp | :udp, boolean()) :: t()
+def create(ip, protocol, expose) do
listen_ip = normalize_listen_ip(ip)
@@
end
+
+@spec create(binary() | listen_ip(), :tcp | :udp, boolean(), integer() | nil) :: t()📝 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.
| @spec create(binary() | %{:ip => any(), optional(any()) => any()}, any(), boolean()) :: struct() | |
| def create(ip, protocol, expose) do | |
| listen_ip = normalize_listen_ip(ip) | |
| struct(__MODULE__, Map.merge(listen_ip, %{:protocol => protocol})) | |
| struct( | |
| __MODULE__, | |
| Map.merge(listen_ip, %{:protocol => protocol, :exposeInternalIp => expose}) | |
| ) | |
| end | |
| @spec create(binary() | listen_ip(), :tcp | :udp, boolean()) :: t() | |
| def create(ip, protocol, expose) do | |
| listen_ip = normalize_listen_ip(ip) | |
| struct( | |
| __MODULE__, | |
| Map.merge(listen_ip, %{:protocol => protocol, :exposeInternalIp => expose}) | |
| ) | |
| end | |
| @spec create(binary() | listen_ip(), :tcp | :udp, boolean(), integer() | nil) :: t() |
🤖 Prompt for AI Agents
In lib/transport_listen_info.ex around lines 56 to 64, tighten the types by
replacing the loose any() for the protocol with a narrower type (e.g. atom() |
String.t()) in the existing @spec for create/3, and add a new @spec for the
missing create/4 clause (use the same first three parameter types and set the
fourth to map() | keyword() depending on how options are passed) with the same
struct() return; update the function heads/signature names if needed to match
the new spec and ensure the normalized ip type stays as binary() | %{ip: any(),
optional(any()) => any()}.
lib/transport_listen_info.ex
Outdated
| def create(ip, protocol, expose, port) do | ||
| listen_ip = normalize_listen_ip(ip) | ||
|
|
||
| struct( | ||
| __MODULE__, | ||
| Map.merge(listen_ip, %{ | ||
| :protocol => protocol, | ||
| :port => port | ||
| :port => port, | ||
| :exposeInternalIp => expose | ||
| }) | ||
| ) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Consider normalizing string protocols to atoms here (or ensure all call sites pass atoms).
Some callers pass "udp" (see PlainTransport). Either normalize here or fix call sites; otherwise you’ll diverge from the :tcp | :udp type.
If you prefer normalizing here:
+defp normalize_protocol("udp"), do: :udp
+defp normalize_protocol("tcp"), do: :tcp
+defp normalize_protocol(p) when p in [:udp, :tcp], do: p
+defp normalize_protocol(other), do: other
@@
- Map.merge(listen_ip, %{:protocol => protocol, :exposeInternalIp => expose})
+ Map.merge(listen_ip, %{:protocol => normalize_protocol(protocol), :exposeInternalIp => expose})
@@
- :protocol => protocol,
+ :protocol => normalize_protocol(protocol),To verify call sites:
🏁 Script executed:
#!/bin/bash
# Locate string protocol usages in create calls
rg -nP 'TransportListenInfo\.create\([^)]*"(udp|tcp)"' -C2Length of output: 849
Normalize protocol to atoms in TransportListenInfo.create
Call sites still pass the string "udp" (lib/plain_transport.ex:83,86; lib/pipe_transport.ex:55). Normalize here (recommended) or update callers to pass atoms.
+defp normalize_protocol("udp"), do: :udp
+defp normalize_protocol("tcp"), do: :tcp
+defp normalize_protocol(p) when p in [:udp, :tcp], do: p
+defp normalize_protocol(other), do: other
@@
- Map.merge(listen_ip, %{:protocol => protocol, :exposeInternalIp => expose})
+ Map.merge(listen_ip, %{:protocol => normalize_protocol(protocol), :exposeInternalIp => expose})
@@
- :protocol => protocol,
+ :protocol => normalize_protocol(protocol),📝 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.
| def create(ip, protocol, expose, port) do | |
| listen_ip = normalize_listen_ip(ip) | |
| struct( | |
| __MODULE__, | |
| Map.merge(listen_ip, %{ | |
| :protocol => protocol, | |
| :port => port | |
| :port => port, | |
| :exposeInternalIp => expose | |
| }) | |
| ) | |
| end | |
| defp normalize_protocol("udp"), do: :udp | |
| defp normalize_protocol("tcp"), do: :tcp | |
| defp normalize_protocol(p) when p in [:udp, :tcp], do: p | |
| defp normalize_protocol(other), do: other | |
| def create(ip, protocol, expose, port) do | |
| listen_ip = normalize_listen_ip(ip) | |
| struct( | |
| __MODULE__, | |
| Map.merge(listen_ip, %{ | |
| :protocol => normalize_protocol(protocol), | |
| :port => port, | |
| :exposeInternalIp => expose | |
| }) | |
| ) | |
| end |
🤖 Prompt for AI Agents
In lib/transport_listen_info.ex around lines 66 to 77, the create/4 function
currently stores whatever is passed as protocol (callers still passing the
string "udp"); change it to normalize protocol to an atom before building the
struct by converting binaries to atoms and leaving atoms as-is (e.g.,
protocol_atom = if is_binary(protocol), do: String.downcase(protocol) |>
String.to_existing_atom() rescue String.to_atom(String.downcase(protocol)),
else: protocol), then use protocol_atom in the struct; this ensures callers can
pass either "udp" or :udp and the stored value is always an atom.
12cb31c to
1ac9a23
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/integration/test_webrtc_server.ex (3)
63-86: Explicit protocol + exposeInternalIp look correct; add a guard that internal IPs don’t leak into ICE candidates.Strengthen the test by asserting that no candidate exposes 127.0.0.1 (or other listen IPs) when exposeInternalIp: false.
Example addition (after obtaining ice_candidates):
refute Enum.any?(ice_candidates, &(&1["address"] in ["127.0.0.1", "0.0.0.0"]))
195-203: “No port specified” test: also assert socket counts (and optionally presence of a port field) to prove dynamic allocation.This makes the intent harder to regress.
Example additions:
dump = WebRtcServer.dump(webrtc_server) assert length(dump["udpSockets"]) == 1 assert length(dump["tcpServers"]) == 1 # Optional, if dump includes port fields: assert Map.has_key?(List.first(dump["udpSockets"]), "port") assert Map.has_key?(List.first(dump["tcpServers"]), "port")
230-232: Tighten failure assertions to check specific error reasons (if surfaced).Matching only on {:error, _} could hide regressions. If Mediasoup returns structured reasons, assert them (e.g., address not available, EADDRINUSE).
Example patterns (adjust to actual error types):
# Unavailable IP assert {:error, %Mediasoup.Error{reason: :address_not_available}} = Worker.create_webrtc_server(worker, opts) # Same UDP port twice in one server / across servers assert {:error, %Mediasoup.Error{reason: :eaddrinuse}} = Worker.create_webrtc_server(worker, opts)If reasons aren’t structured yet, consider asserting that the error message contains a stable substring (e.g., "address not available" / "address in use").
Also applies to: 243-251, 262-276
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/plain_transport.ex(1 hunks)lib/transport_listen_info.ex(2 hunks)lib/webrtc_transport.ex(1 hunks)test/integration/test_plain_transport.ex(1 hunks)test/integration/test_webrtc_server.ex(7 hunks)test/integration/test_webrtc_transport.ex(1 hunks)test/transport_listen_info_test.exs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/integration/test_plain_transport.ex
🚧 Files skipped from review as they are similar to previous changes (5)
- test/transport_listen_info_test.exs
- lib/webrtc_transport.ex
- lib/plain_transport.ex
- lib/transport_listen_info.ex
- test/integration/test_webrtc_transport.ex
🔇 Additional comments (1)
test/integration/test_webrtc_server.ex (1)
154-177: LGTM: Close path uses the new fields consistently.No issues; mirrors the success case with explicit protocol and exposeInternalIp.
h3poteto
force-pushed
the
update/mediasoup/0.19.1
branch
from
1ac9a23 to
9f0ba3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
lib/transport_listen_info.ex (2)
56-56: Fix specs: narrow protocol type, return t(), and add missing create/4 spec.Matches prior feedback; please update specs accordingly.
- @spec create(binary() | %{:ip => any(), optional(any()) => any()}, any(), boolean()) :: struct() + @type protocol :: :tcp | :udp + @spec create(binary() | listen_ip(), protocol(), boolean()) :: t()Also add for 4-arity:
+ @spec create(binary() | listen_ip(), protocol(), boolean(), integer()) :: t()
57-64: Normalize protocol to atoms and avoid persisting nil port.Still accepts and stores string "udp". Normalize here and handle nil-port by delegating to create/3.
+ defp normalize_protocol(:udp), do: :udp + defp normalize_protocol(:tcp), do: :tcp + defp normalize_protocol("udp"), do: :udp + defp normalize_protocol("tcp"), do: :tcp + defp normalize_protocol(other), do: other + def create(ip, protocol, expose) do listen_ip = normalize_listen_ip(ip) struct( __MODULE__, - Map.merge(listen_ip, %{:protocol => protocol, :exposeInternalIp => expose}) + Map.merge(listen_ip, %{ + :protocol => normalize_protocol(protocol), + :exposeInternalIp => expose + }) ) end + def create(ip, protocol, expose, nil), do: create(ip, protocol, expose) + def create(ip, protocol, expose, port) do listen_ip = normalize_listen_ip(ip) struct( __MODULE__, Map.merge(listen_ip, %{ - :protocol => protocol, + :protocol => normalize_protocol(protocol), :port => port, :exposeInternalIp => expose }) ) end#!/bin/bash # Find string protocol usage and nil ports passed to create/4 rg -nP --type=elixir 'TransportListenInfo\.create\([^)]*"(udp|tcp)"' -C2 rg -nP --type=elixir 'TransportListenInfo\.create\([^)]*,\s*[^,]+,\s*[^,]+,\s*nil\)' -C2Also applies to: 66-77
lib/pipe_transport.ex (1)
55-55: Use atom protocol and don’t pass nil as port.Align with spec and avoid storing nil by choosing arity based on port.
- listen_info = TransportListenInfo.create(listen_ip, "udp", false, port) + listen_info = + if is_integer(port) do + TransportListenInfo.create(listen_ip, :udp, false, port) + else + TransportListenInfo.create(listen_ip, :udp, false) + endlib/plain_transport.ex (1)
82-86: Protocol should be atom; avoid passing nil to create/4.Match type expectations and skip port when absent.
- listen_info: - Map.get(option, :listen_info) || - TransportListenInfo.create(listen_ip, "udp", false, port), + listen_info: + Map.get(option, :listen_info) || + if is_integer(port) do + TransportListenInfo.create(listen_ip, :udp, false, port) + else + TransportListenInfo.create(listen_ip, :udp, false) + end, @@ - rtcp_listen_info: - Map.get(option, :rtcp_listen_info) || - TransportListenInfo.create(listen_ip, "udp", false, nil) + rtcp_listen_info: + Map.get(option, :rtcp_listen_info) || + TransportListenInfo.create(listen_ip, :udp, false)
🧹 Nitpick comments (1)
lib/transport_listen_info.ex (1)
14-17: Tighten types for new fields.Consider defining concrete types instead of plain map() for portRange and flags to improve Dialyzer precision and docs.
Example:
+ @type port_range :: %{min: pos_integer(), max: pos_integer()} + @type flags :: map() @@ - optional(:portRange) => map(), - optional(:flags) => map(), + optional(:portRange) => port_range(), + optional(:flags) => flags(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/pipe_transport.ex(1 hunks)lib/plain_transport.ex(1 hunks)lib/transport_listen_info.ex(2 hunks)lib/webrtc_transport.ex(1 hunks)test/integration/test_plain_transport.ex(1 hunks)test/integration/test_webrtc_server.ex(7 hunks)test/integration/test_webrtc_transport.ex(1 hunks)test/transport_listen_info_test.exs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/integration/test_plain_transport.ex
🚧 Files skipped from review as they are similar to previous changes (4)
- test/integration/test_webrtc_server.ex
- test/integration/test_webrtc_transport.ex
- lib/webrtc_transport.ex
- test/transport_listen_info_test.exs
🧰 Additional context used
🧬 Code graph analysis (2)
lib/plain_transport.ex (1)
lib/transport_listen_info.ex (2)
create(57-64)create(66-77)
lib/pipe_transport.ex (1)
lib/transport_listen_info.ex (2)
create(57-64)create(66-77)
🔇 Additional comments (1)
lib/transport_listen_info.ex (1)
24-34: Struct additions look consistent with type.No issues found with new keys in defstruct.
h3poteto
force-pushed
the
update/mediasoup/0.19.1
branch
from
9f0ba3d to
5de51fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/transport_listen_info.ex (2)
56-64: Normalize protocol to atoms and tighten the @SPEC (callers pass "udp").Current spec is too loose and the function stores whatever comes in as protocol. Call sites (e.g., PipeTransport) pass "udp", which violates the declared t type (:tcp | :udp). Normalize here and narrow the spec.
Apply:
-@spec create(binary() | %{:ip => any(), optional(any()) => any()}, any(), boolean()) :: struct() -def create(ip, protocol, expose) do +@spec create(binary() | listen_ip(), :tcp | :udp, boolean()) :: t() +def create(ip, protocol, expose) do listen_ip = normalize_listen_ip(ip) - struct( - __MODULE__, - Map.merge(listen_ip, %{:protocol => protocol, :exposeInternalIp => expose}) - ) + struct( + __MODULE__, + Map.merge(listen_ip, %{ + :protocol => normalize_protocol(protocol), + :exposeInternalIp => expose + }) + ) endAdd this helper somewhere in the module:
# Accept "udp"/"tcp" or atoms; reject anything else to keep invariants tight. defp normalize_protocol(:udp), do: :udp defp normalize_protocol(:tcp), do: :tcp defp normalize_protocol("udp"), do: :udp defp normalize_protocol("tcp"), do: :tcp defp normalize_protocol(other), do: raise ArgumentError, "protocol must be :udp or :tcp, got: #{inspect(other)}"
66-77: Avoid inserting :port when nil; add missing @SPEC for create/4; normalize protocol.create/4 currently writes port: nil into the struct when port is nil (see PipeTransport), which violates the type expectation (integer()). Make port conditional, add the missing spec, and normalize protocol consistently.
Apply:
+@spec create(binary() | listen_ip(), :tcp | :udp, boolean(), integer() | nil) :: t() def create(ip, protocol, expose, port) do listen_ip = normalize_listen_ip(ip) struct( __MODULE__, - Map.merge(listen_ip, %{ - :protocol => protocol, - :port => port, - :exposeInternalIp => expose - }) + Map.merge( + listen_ip, + if is_nil(port) do + %{ + :protocol => normalize_protocol(protocol), + :exposeInternalIp => expose + } + else + %{ + :protocol => normalize_protocol(protocol), + :exposeInternalIp => expose, + :port => port + } + end + ) ) endlib/pipe_transport.ex (1)
55-55: Pass :udp (atom) and pick create arity based on port presence.String "udp" breaks the type invariant upstream. Also avoid calling create/4 with nil port.
Apply:
- listen_info = TransportListenInfo.create(listen_ip, "udp", false, port) + listen_info = + if is_nil(port) do + TransportListenInfo.create(listen_ip, :udp, false) + else + TransportListenInfo.create(listen_ip, :udp, false, port) + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/pipe_transport.ex(1 hunks)lib/plain_transport.ex(1 hunks)lib/transport_listen_info.ex(2 hunks)lib/webrtc_transport.ex(1 hunks)test/integration/test_pipe_transport.ex(0 hunks)test/integration/test_plain_transport.ex(1 hunks)test/integration/test_webrtc_server.ex(7 hunks)test/integration/test_webrtc_transport.ex(1 hunks)test/transport_listen_info_test.exs(1 hunks)
💤 Files with no reviewable changes (1)
- test/integration/test_pipe_transport.ex
✅ Files skipped from review due to trivial changes (1)
- test/integration/test_plain_transport.ex
🚧 Files skipped from review as they are similar to previous changes (5)
- test/integration/test_webrtc_transport.ex
- lib/webrtc_transport.ex
- test/integration/test_webrtc_server.ex
- lib/plain_transport.ex
- test/transport_listen_info_test.exs
🧰 Additional context used
🧬 Code graph analysis (1)
lib/pipe_transport.ex (1)
lib/transport_listen_info.ex (2)
create(57-64)create(66-77)
🔇 Additional comments (1)
lib/transport_listen_info.ex (1)
14-18: LGTM: struct and type extended correctly for exposeInternalIp/portRange/flagsNew optional fields added to both @type t and defstruct look consistent. No functional concerns here.
Also applies to: 24-34
lib/pipe_transport.ex
Outdated
| def normalize(%Options{listen_ip: listen_ip, port: port} = option) | ||
| when not is_nil(listen_ip) do | ||
| listen_info = TransportListenInfo.create(listen_ip, "udp", port) | ||
| listen_info = TransportListenInfo.create(listen_ip, "udp", false, port) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Scan and update other call sites still passing "udp".
There may be more create(..., "udp", ...) usages (e.g., PlainTransport). Either rely on the new normalization or change to :udp for consistency.
🏁 Script executed:
#!/bin/bash
# Find TransportListenInfo.create calls passing "udp" as a string
rg -nP 'TransportListenInfo\.create\([^)]*"(udp|tcp)"' -C2Length of output: 856
Use atom :udp for TransportListenInfo.create call sites (or ensure normalization accepts "udp")
Occurrences: lib/plain_transport.ex:83, lib/plain_transport.ex:86, lib/pipe_transport.ex:55 — change to :udp for consistency.
🤖 Prompt for AI Agents
In lib/pipe_transport.ex around line 55, TransportListenInfo.create is being
called with the string "udp"; change that argument to the atom :udp (or ensure
the create/normalization functions accept the string form) so the call is
consistent with other call sites (lib/plain_transport.ex) and uses the expected
atom type; update the literal to :udp to fix type/consistency issues.
5de51fe to
80ffde3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/transport_listen_info_test.exs (2)
15-16: Prefer pattern match and assert nil defaults for new fields.Make the intent explicit for new fields (port/portRange/flags) by pattern‑matching nils instead of full equality.
Apply:
- assert %Mediasoup.TransportListenInfo{ - announcedAddress: "1.1.1.1", - ip: "127.0.0.1", - protocol: :udp, - exposeInternalIp: false - } == - TransportListenInfo.create( - %{ip: "127.0.0.1", announcedAddress: "1.1.1.1"}, - :udp, - false - ) + assert %Mediasoup.TransportListenInfo{ + announcedAddress: "1.1.1.1", + ip: "127.0.0.1", + protocol: :udp, + exposeInternalIp: false, + port: nil, + portRange: nil, + flags: nil + } = + TransportListenInfo.create( + %{ip: "127.0.0.1", announcedAddress: "1.1.1.1"}, + :udp, + false + )
19-24: Broaden string‑input coverage (pattern match + IPv6).Pattern‑match for clarity and add an IPv6 case to exercise normalization.
Apply:
- assert %Mediasoup.TransportListenInfo{ - ip: "127.0.0.1", - protocol: :tcp, - exposeInternalIp: false - } == - TransportListenInfo.create("127.0.0.1", :tcp, false) + assert %Mediasoup.TransportListenInfo{ + ip: "127.0.0.1", + protocol: :tcp, + exposeInternalIp: false, + port: nil, + portRange: nil, + flags: nil + } = + TransportListenInfo.create("127.0.0.1", :tcp, false) + + # IPv6 string input + assert %Mediasoup.TransportListenInfo{ + ip: "::1", + protocol: :tcp, + exposeInternalIp: false + } = + TransportListenInfo.create("::1", :tcp, false)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/pipe_transport.ex(1 hunks)lib/plain_transport.ex(1 hunks)lib/transport_listen_info.ex(2 hunks)lib/webrtc_transport.ex(1 hunks)test/integration/test_pipe_transport.ex(0 hunks)test/integration/test_plain_transport.ex(1 hunks)test/integration/test_webrtc_server.ex(7 hunks)test/integration/test_webrtc_transport.ex(1 hunks)test/transport_listen_info_test.exs(1 hunks)
💤 Files with no reviewable changes (1)
- test/integration/test_pipe_transport.ex
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/transport_listen_info.ex
- lib/webrtc_transport.ex
- test/integration/test_webrtc_transport.ex
- test/integration/test_webrtc_server.ex
- test/integration/test_plain_transport.ex
- lib/plain_transport.ex
- lib/pipe_transport.ex
🧰 Additional context used
🧬 Code graph analysis (1)
test/transport_listen_info_test.exs (1)
lib/transport_listen_info.ex (2)
create(57-64)create(66-77)
🔇 Additional comments (1)
test/transport_listen_info_test.exs (1)
10-11: Add coverage for exposeInternalIp: true and create/4.Good assertion for the new field; also cover the true path and the create/4 arity (port) to prevent regressions. The provided rg returned no matches for create/4 — verify tests exercise create/4 or add the focused test below.
defmodule Mediasoup.TransportListenInfoTest do use ExUnit.Case @@ test "create" do @@ end + + test "create/4 sets port and exposeInternalIp" do + assert %Mediasoup.TransportListenInfo{ + ip: "127.0.0.1", + protocol: :udp, + exposeInternalIp: true, + port: 4000 + } = + TransportListenInfo.create( + %{ip: "127.0.0.1", announcedAddress: "1.1.1.1"}, + :udp, + true, + 4000 + ) + end
h3poteto
force-pushed
the
update/mediasoup/0.19.1
branch
from
80ffde3 to
bcbcb81
Compare
h3poteto
force-pushed
the
update/mediasoup/0.19.1
branch
from
ec74968 to
750f63d
Compare
h3poteto
force-pushed
the
update/mediasoup/0.19.1
branch
from
750f63d to
3b2913a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/transport_listen_info.ex (2)
56-57: Tighten specs and return type (Dialyzer).Use module types and narrow protocol to atoms; add missing spec for arity-3.
-@spec create(binary() | %{:ip => any(), optional(any()) => any()}, any()) :: struct() +@spec create(binary() | listen_ip(), :tcp | :udp) :: t() def create(ip, protocol) do+@spec create(binary() | listen_ip(), :tcp | :udp, non_neg_integer()) :: t() def create(ip, protocol, port) doAlso applies to: 66-66
60-63: Don’t clobber caller-provided exposeInternalIp; default only if missing. Also normalize protocol to atoms.Currently you always overwrite exposeInternalIp with false and you don’t normalize protocol. This breaks callers that pass exposeInternalIp: true and those still passing "udp"/"tcp".
Apply:
def create(ip, protocol) do listen_ip = normalize_listen_ip(ip) - - struct( - __MODULE__, - Map.merge(listen_ip, %{:protocol => protocol, :exposeInternalIp => false}) - ) + protocol = normalize_protocol(protocol) + struct( + __MODULE__, + listen_ip + |> Map.put_new(:exposeInternalIp, false) + |> Map.put(:protocol, protocol) + ) end @@ - Map.merge(listen_ip, %{ - :protocol => protocol, - :port => port, - :exposeInternalIp => false - }) + (fn -> + protocol = normalize_protocol(protocol) + listen_ip + |> Map.put_new(:exposeInternalIp, false) + |> Map.put(:protocol, protocol) + |> Map.put(:port, port) + end).()Add (outside this hunk):
defp normalize_protocol(p) when p in [:udp, :tcp], do: p defp normalize_protocol("udp"), do: :udp defp normalize_protocol("tcp"), do: :tcp defp normalize_protocol(other), do: raise(ArgumentError, "protocol must be :udp | :tcp (or \"udp\" | \"tcp\"), got: #{inspect(other)}")#!/bin/bash # Find create/ calls still passing "udp"/"tcp" rg -nP --type=elixir 'TransportListenInfo\.create\([^)]*"(udp|tcp)"' -C2Also applies to: 71-75
🧹 Nitpick comments (4)
lib/transport_listen_info.ex (2)
66-69: Validate port range (0..65535).Prevents invalid port values from leaking downstream.
def create(ip, protocol, port) do listen_ip = normalize_listen_ip(ip) + unless is_integer(port) and port in 0..65535 do + raise ArgumentError, "port must be 0..65535" + end
16-17: Model portRange more precisely.Gives callers/ Dialyzer a clearer contract.
- optional(:portRange) => map(), + optional(:portRange) => %{optional(:min) => non_neg_integer(), optional(:max) => non_neg_integer()}, optional(:flags) => map(),test/integration/test_webrtc_server.ex (1)
195-197: Exercise defaulting path: drop explicit exposeInternalIp here.Let the code default to false, so we cover both explicit and implicit cases.
- %{ - ip: "127.0.0.1", - announcedIp: "9.9.9.1", - protocol: :udp, - exposeInternalIp: false - }, + %{ip: "127.0.0.1", announcedIp: "9.9.9.1", protocol: :udp}, @@ - %{ - ip: "127.0.0.1", - announcedIp: "9.9.9.2", - protocol: :tcp, - exposeInternalIp: false - } + %{ip: "127.0.0.1", announcedIp: "9.9.9.2", protocol: :tcp}Also applies to: 201-203
lib/webrtc_server.ex (1)
14-21: Make exposeInternalIp optional or default it during normalization (avoid back-compat break).Type currently requires exposeInternalIp; many existing call sites may omit it.
Option A — make it optional:
@type webrtc_server_listen_info :: %{ :protocol => :udp | :tcp, :ip => String.t(), - :exposeInternalIp => boolean(), + optional(:exposeInternalIp) => boolean(), optional(:announcedAddress) => String.t() | nil, optional(:announcedIp) => String.t() | nil, optional(:port) => integer() | nil }Option B — keep it required, but default in Options.normalize (outside this hunk):
def normalize(%Options{listen_infos: listen_infos} = option) do listen_infos = for info <- listen_infos, into: [] do info |> normalize_listen_info() |> Map.put_new(:exposeInternalIp, false) end %{option | listen_infos: listen_infos} end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
lib/plain_transport.ex(1 hunks)lib/transport_listen_info.ex(3 hunks)lib/webrtc_server.ex(1 hunks)lib/webrtc_transport.ex(1 hunks)mix.exs(1 hunks)test/integration/test_pipe_transport.ex(0 hunks)test/integration/test_plain_transport.ex(1 hunks)test/integration/test_webrtc_server.ex(7 hunks)test/integration/test_webrtc_transport.ex(1 hunks)test/transport_listen_info_test.exs(1 hunks)
💤 Files with no reviewable changes (1)
- test/integration/test_pipe_transport.ex
✅ Files skipped from review due to trivial changes (2)
- test/integration/test_plain_transport.ex
- lib/plain_transport.ex
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/webrtc_transport.ex
- mix.exs
- test/integration/test_webrtc_transport.ex
- test/transport_listen_info_test.exs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (stable)
- GitHub Check: lint (stable)
🔇 Additional comments (1)
test/integration/test_webrtc_server.ex (1)
63-65: LGTM: exposeInternalIp added consistently across listen_infos.Matches the new API and keeps assertions readable.
Also applies to: 70-72, 77-79, 84-86, 154-156, 161-163, 168-170, 175-177, 195-197, 201-203, 230-232, 243-245, 249-251, 263-264, 275-276
Summary by CodeRabbit