Skip to content

Conversation

@zibet27
Copy link
Collaborator

@zibet27 zibet27 commented Dec 16, 2025

Subsystem
WebRTC Client.

Motivation
KTOR-9188 WebRTC. IceServer.urls should be a list.

Solution
Change the type of urls. I think it's ok, because the API is still experimental. Moreover, IceServers are usually initialised only once.
I wanted to add more tests, ensuring ice servers are actually considered, but it doesn't work on emulators/simulators.

@zibet27 zibet27 requested review from bjhham and e5l December 16, 2025 13:44
@zibet27 zibet27 self-assigned this Dec 16, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 16, 2025

Walkthrough

WebRtc.IceServer urls changed from String to List<String> across public APIs and platform implementations; constructors, component/copy/getters, and platform-specific call sites (iOS, web, rs) were updated. WebRtcPeerConnection.runInConnectionScope visibility changed from internal to protected. JVM test utilities and build config tweaks were added.

Changes

Cohort / File(s) Change Summary
API files
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api, ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api, ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api, .../api/jvm/ktor-client-webrtc.api
WebRtc.IceServer public signatures updated to use List<String>: primary & synthetic constructors, component1(), copy(...), copy$default(...), and getUrls(); WebRtcPeerConnection adds/changes runInConnectionScope visibility.
Core data class
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt
data class IceServer: val urls: Stringval urls: List<String>; added secondary constructor constructor(url: String, username: String? = null, credential: String? = null) delegating to primary.
iOS platform
ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/Engine.kt
Construct RTCIceServer using the raw urls list (no extra wrapping), matching new List parameter.
Web platform
ktor-client/ktor-client-webrtc/web/src/io/ktor/client/webrtc/Browser.kt
Convert urls collection into a JS array (map over URLs) when creating ICE server entries to support multiple URLs.
Rust/rs backend
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Engine.kt
Pass it.urls directly to IceServer instead of wrapping with listOf(it.urls) to avoid List<List> mismatch.
Tests / JVM test utilities
ktor-client/ktor-client-webrtc/web/test/.../JsWebRtcMediaTest.kt, ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt, .../Ignore.jvm.kt
Added @OptIn(ExperimentalWasmJsInterop::class) to a web test; added JVM test utilities (stubbed createTestWebRtcClient, grantPermissions) and JVM-specific ignore/typealias stubs.
Visibility & build configs
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnection.kt, ktor-client/ktor-client-webrtc/gradle.properties, ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts, .../ktor-client-webrtc-rs/gradle.properties
runInConnectionScope changed internalprotected; enabled JVM target flag; updated Gobley plugin versions; removed iOS Rust build tasks and adjusted disabled targets.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

bug

Suggested reviewers

  • e5l

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: converting IceServer.urls from a single String to a List.
Description check ✅ Passed The description follows the template with all required sections (Subsystem, Motivation, Solution) properly filled. It references the related issue (KTOR-9188) and provides clear rationale.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

*/
public data class IceServer(
val urls: String,
val urls: List<String>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you introduce an overload constructor to mitigate the breaking change here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that should work. Forgot about it :)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt (2)

7-8: Consider using star imports per project guidelines.

The coding guidelines specify using star imports for io.ktor.* packages. Consider refactoring to:

import io.ktor.client.webrtc.*
import io.ktor.utils.io.*

As per coding guidelines, this aligns with the project's import conventions.


10-10: Use modern @OptIn syntax.

The markerClass parameter form is outdated. Modern Kotlin uses the simplified syntax:

-@OptIn(markerClass = [ExperimentalKtorApi::class])
+@OptIn(ExperimentalKtorApi::class)
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca6527e and 64bf33b.

📒 Files selected for processing (8)
  • ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt
  • ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnection.kt
  • ktor-client/ktor-client-webrtc/gradle.properties
  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Engine.kt
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/gradle.properties
🚧 Files skipped from review as they are similar to previous changes (1)
  • ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.kt: Follow Kotlin official style guide (https://kotlinlang.org/docs/coding-conventions.html)
Use star imports for io.ktor.* packages
Document all public APIs including parameters, return types, and exceptions
Mark internal APIs with @InternalAPI annotation
Run ./gradlew lintKotlin and fix all linting issues before giving control back to the user
Use ./gradlew formatKotlin to automatically fix formatting issues
Run ./gradlew updateLegacyAbi after making ABI changes to update ABI signature files
Binary compatibility is enforced - all public API changes must be tracked in the /api/ directories
Validate ABI with ./gradlew checkLegacyAbi and update with ./gradlew updateLegacyAbi
API changes must be intentional and well-documented
Error handling follows Kotlin conventions with specific Ktor exceptions

Files:

  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Engine.kt
  • ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnection.kt
  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt
**/test/**/*.kt

📄 CodeRabbit inference engine (CLAUDE.md)

Follow test naming pattern: DescribeWhatIsBeingTested

Files:

  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt
🧠 Learnings (19)
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Gobley Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module requires the kotlin("plugin.atomicfu") plugin as a dependency, so the atomicfu plugin should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module depends on the kotlin("plugin.atomicfu") plugin, so atomicfu should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/gradle.properties
  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Gobley Gradle plugin (dev.gobley.cargo and dev.gobley.uniffi) requires the kotlin("plugin.atomicfu") plugin for atomic types support in Rust-Kotlin bindings generated by UniFFI. The atomicfu plugin should not be removed even if there are no direct kotlinx.atomicfu imports in the Kotlin source code, as it's essential for proper Rust-Kotlin interoperability.

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
📚 Learning: 2025-07-01T10:54:53.751Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4970
File: build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts:67-72
Timestamp: 2025-07-01T10:54:53.751Z
Learning: In build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts, the KT-70915 and KT-77609 workarounds that limit parallelism for KotlinNativeLink and CInteropCommonizerTask should remain unconditional (not gated behind CI flag) as these limits are beneficial for both local and CI builds.

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/gradle.properties
  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Binary compatibility is enforced - all public API changes must be tracked in the `/api/` directories

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/gradle.properties
  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-11-27T10:07:26.686Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 5205
File: ktor-client/ktor-client-curl/desktop/interop/include/curl/multi.h:410-420
Timestamp: 2025-11-27T10:07:26.686Z
Learning: In the ktorio/ktor repository, do not review files under `ktor-client/ktor-client-curl/desktop/interop/include/curl/*` as these are upstream libcurl header files copied from the curl project and not maintained by the Ktor team.

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/gradle.properties
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : API changes must be intentional and well-documented

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/gradle.properties
  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-05-16T13:11:28.416Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4860
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.api:24-26
Timestamp: 2025-05-16T13:11:28.416Z
Learning: Binary incompatible changes (such as constructor signature changes) are acceptable in the ktor-server-di module when the version is not yet released, as confirmed by the development team.

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
📚 Learning: 2025-04-23T11:48:32.634Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4767
File: build-settings-logic/src/main/kotlin/ktorsettings.kotlin-user-project.settings.gradle.kts:136-146
Timestamp: 2025-04-23T11:48:32.634Z
Learning: For Kotlin Train builds in the Ktor project, the properties `atomicfu_version`, `coroutines_version`, and `serialization_version` are expected to be defined in the TeamCity CI environment, so using `.get()` without custom error handling is sufficient.

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
📚 Learning: 2025-08-15T09:12:47.243Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/senders.rs:88-96
Timestamp: 2025-08-15T09:12:47.243Z
Learning: In the webrtc Rust crate version used in the Ktor WebRTC RS implementation, the RTCRtpCodingParameters struct's rtx field is NOT optional and can be accessed directly as params.rtx.ssrc without needing Option handling.

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Use star imports for `io.ktor.*` packages

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.

Applied to files:

  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt
📚 Learning: 2025-05-19T14:56:46.265Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4861
File: teamcity.default.properties:5-7
Timestamp: 2025-05-19T14:56:46.265Z
Learning: The `kotlin.native.jvmargs` property should be kept separate from the `org.gradle.jvmargs` and from the `GRADLE_OPTS` environment variable in CI configuration.

Applied to files:

  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-05-14T15:56:33.598Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4856
File: teamcity.default.properties:5-5
Timestamp: 2025-05-14T15:56:33.598Z
Learning: The Ktor project runs binary-compatibility-validator tasks on CI which require substantial metaspace, so adding metaspace size limits (-XX:MaxMetaspaceSize) to Gradle options would cause these tasks to fail.

Applied to files:

  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: This is a multiplatform project (JVM, JS, Native), and platform-specific considerations should be respected.

Applied to files:

  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Project is multiplatform (JVM, JS, Native) with tests organized by platform in module's test directories

Applied to files:

  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Engine.kt
📚 Learning: 2025-09-30T07:52:14.769Z
Learnt from: yschimke
Repo: ktorio/ktor PR: 4013
File: ktor-client/ktor-client-android/jvm/src/io/ktor/client/engine/android/Android14URLConnectionFactory.kt:24-26
Timestamp: 2025-09-30T07:52:14.769Z
Learning: In the Ktor Android HTTP client engine (ktor-client-android), prefer using `URI.create(urlString).toURL()` over the `URL(urlString)` constructor when opening connections with Android's HttpEngine, as it avoids deprecated APIs and the different exception behavior (IllegalArgumentException vs MalformedURLException) is acceptable.

Applied to files:

  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Engine.kt
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Mark internal APIs with `InternalAPI` annotation

Applied to files:

  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt
🔇 Additional comments (9)
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Engine.kt (1)

36-36: LGTM! IceServer construction correctly updated.

The change properly reflects the updated IceServer API where urls is now List<String>. Passing it.urls directly (instead of wrapping in listOf()) is correct.

ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnection.kt (1)

133-133: Verify the visibility change is intentional and doesn't break module-internal usage.

The visibility modifier changed from internal to protected, restricting access to subclasses only. This change appears unrelated to the PR's stated objective of making IceServer.urls a list. While protected is more restrictive and appropriate for this abstract class pattern, please confirm:

  1. This change is intentional
  2. No other code within the module (but outside subclasses) depends on this function
ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt (3)

7-7: LGTM!

The typealias correctly maps to JUnit 5's @Disabled annotation, providing appropriate JVM-specific test ignore functionality.


9-10: LGTM!

The annotation is correctly defined with appropriate targets (CLASS and FUNCTION) for a test utility. The actual constructor properly implements the expect declaration.


1-3: Verify the copyright year.

The copyright includes 2026, but this PR was created in December 2025. Confirm this is intentional (e.g., expected merge date) or if it should be 2025.

⛔ Skipped due to learnings
Learnt from: osipxd
Repo: ktorio/ktor PR: 5205
File: ktor-client/ktor-client-curl/desktop/interop/include/curl/multi.h:410-420
Timestamp: 2025-11-27T10:07:26.686Z
Learning: In the ktorio/ktor repository, do not review files under `ktor-client/ktor-client-curl/desktop/interop/include/curl/*` as these are upstream libcurl header files copied from the curl project and not maintained by the Ktor team.
Learnt from: osipxd
Repo: ktorio/ktor PR: 5205
File: ktor-client/ktor-client-curl/libcurl/ports/curl/dependencies.patch:156-160
Timestamp: 2025-11-27T10:07:51.060Z
Learning: In the ktorio/ktor repository, do not review files under `ktor-client/ktor-client-curl/libcurl/ports/*` as these are vcpkg port files copied from the vcpkg repository and maintained upstream, not by the Ktor team.
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : API changes must be intentional and well-documented
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/gradle.properties (1)

6-7: No action required — target disablements are correctly configured.

The parent module (ktor-client-webrtc) sets target.darwin=false, which already disables tvOS, watchOS, and all macOS variants at the module level. The child module (ktor-client-webrtc-rs) only needs to override target.ios=false since iOS is explicitly enabled in the parent. This design correctly inherits the Darwin disablement while selectively disabling iOS for the rs module.

Likely an incorrect or invalid review comment.

ktor-client/ktor-client-webrtc/gradle.properties (1)

10-11: Update the comment to accurately describe the JVM srcset purpose.

The comment describes the JVM srcset as "dummy," but the jvm/test directory contains actual test utilities (ConnectionUtils.jvm.kt and Ignore.jvm.kt) that provide platform-specific implementations for JVM testing. The JVM target is needed because the ktor-client-webrtc-rs module uses the Cargo plugin to compile Rust code and generate JVM bindings via UniFFFI, which requires JVM compilation support.

Suggested update:

# JVM target is required for the Cargo plugin to generate JVM bindings from the Rust library

Likely an incorrect or invalid review comment.

ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt (1)

11-13: No action needed. The test classes that call createTestWebRtcClient() are annotated with @IgnoreJvm, so the TODO() stub on JVM will never be executed. The implementation is intentional and safe.

ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts (1)

11-14: Update or remove outdated version comment and justify using 0.3.5 instead of 0.3.7.

The comment referencing Gobley plugin version 0.3.8 is misleading—version 0.3.8 has not been released. Current available versions are up to 0.3.7, yet the code uses 0.3.5. Either update the versions to use the latest 0.3.7 or document why 0.3.5 is preferred over newer releases and remove the reference to the unreleased 0.3.8.

@zibet27 zibet27 requested a review from bjhham January 5, 2026 11:51
Copy link
Contributor

@bjhham bjhham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@zibet27 zibet27 force-pushed the zibet27/fix-webrtc-ice-server-urls branch from 177d3b2 to 1a032f5 Compare January 5, 2026 13:16
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Fix all issues with AI Agents 🤖
In @ktor-client/ktor-client-webrtc/gradle.properties:
- Around line 10-11: Update the misleading comment above target.jvm=true to
explain that the JVM target is intentionally enabled to declare and track the
JVM API surface (api/jvm/) for binary compatibility and to expose JVM bindings
for the Rust variant (ktor-client-webrtc-rs), not merely as a "dummy"
placeholder; mention the existence of api/jvm/ and that this enables JVM
consumers and Rust/JNI bindings to build correctly.
🧹 Nitpick comments (1)
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt (1)

92-108: IceServer.urls list migration and secondary constructor look correct; consider clarifying KDoc and named-arg impact

The switch to urls: List<String> plus the single‑URL secondary constructor is a good way to support multiple ICE URLs while keeping most existing call sites working. The only remaining break is for Kotlin callers that used a named argument like urls = "stun:..."; they’ll now need either urls = listOf("…") or url = "…", which seems acceptable for an experimental API.

You might want to slightly tighten the KDoc to make the collection type explicit, e.g. “List of URLs of the ICE server (e.g. STUN/TURN URIs)” and optionally mention the convenience single‑URL constructor.

As per coding guidelines, API changes here are intentional and documented.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 177d3b2 and 1a032f5.

📒 Files selected for processing (13)
  • gradle/artifacts/publishJvmAndCommonPublications.txt
  • ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api
  • ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api
  • ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api
  • ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt
  • ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnection.kt
  • ktor-client/ktor-client-webrtc/gradle.properties
  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/api/android/ktor-client-webrtc-rs.api
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Engine.kt
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/gradle.properties
🚧 Files skipped from review as they are similar to previous changes (5)
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
  • ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnection.kt
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Engine.kt
  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt
  • ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/gradle.properties
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.kt: Follow Kotlin official style guide (https://kotlinlang.org/docs/coding-conventions.html)
Use star imports for io.ktor.* packages
Document all public APIs including parameters, return types, and exceptions
Mark internal APIs with @InternalAPI annotation
Run ./gradlew lintKotlin and fix all linting issues before giving control back to the user
Use ./gradlew formatKotlin to automatically fix formatting issues
Run ./gradlew updateLegacyAbi after making ABI changes to update ABI signature files
Binary compatibility is enforced - all public API changes must be tracked in the /api/ directories
Validate ABI with ./gradlew checkLegacyAbi and update with ./gradlew updateLegacyAbi
API changes must be intentional and well-documented
Error handling follows Kotlin conventions with specific Ktor exceptions

Files:

  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
  • ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt
**/test/**/*.kt

📄 CodeRabbit inference engine (CLAUDE.md)

Follow test naming pattern: DescribeWhatIsBeingTested

Files:

  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
🧠 Learnings (21)
📚 Learning: 2025-04-22T12:33:16.705Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4805
File: build-logic/src/main/kotlin/ktorbuild/internal/publish/ValidatePublishedArtifactsTask.kt:74-82
Timestamp: 2025-04-22T12:33:16.705Z
Learning: The ValidatePublishedArtifactsTask in the Ktor build system is designed to collect artifacts from all PublishToMavenRepository tasks in the task graph, not just from the specifically named publish task. This is intentional to validate all publications triggered by a task, including those from task dependencies.

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Binary compatibility is enforced - all public API changes must be tracked in the `/api/` directories

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
  • ktor-client/ktor-client-webrtc/gradle.properties
  • ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : API changes must be intentional and well-documented

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
  • ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api
  • ktor-client/ktor-client-webrtc/gradle.properties
  • ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt
  • ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api
📚 Learning: 2025-05-16T13:11:28.416Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4860
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.api:24-26
Timestamp: 2025-05-16T13:11:28.416Z
Learning: Binary incompatible changes (such as constructor signature changes) are acceptable in the ktor-server-di module when the version is not yet released, as confirmed by the development team.

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
  • ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Document all public APIs including parameters, return types, and exceptions

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
  • ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module depends on the kotlin("plugin.atomicfu") plugin, so atomicfu should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Use star imports for `io.ktor.*` packages

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
📚 Learning: 2025-11-27T10:07:26.686Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 5205
File: ktor-client/ktor-client-curl/desktop/interop/include/curl/multi.h:410-420
Timestamp: 2025-11-27T10:07:26.686Z
Learning: In the ktorio/ktor repository, do not review files under `ktor-client/ktor-client-curl/desktop/interop/include/curl/*` as these are upstream libcurl header files copied from the curl project and not maintained by the Ktor team.

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Gobley Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module requires the kotlin("plugin.atomicfu") plugin as a dependency, so the atomicfu plugin should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
📚 Learning: 2025-11-27T10:07:51.060Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 5205
File: ktor-client/ktor-client-curl/libcurl/ports/curl/dependencies.patch:156-160
Timestamp: 2025-11-27T10:07:51.060Z
Learning: In the ktorio/ktor repository, do not review files under `ktor-client/ktor-client-curl/libcurl/ports/*` as these are vcpkg port files copied from the vcpkg repository and maintained upstream, not by the Ktor team.

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
📚 Learning: 2025-05-07T09:12:14.293Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4836
File: ktor-utils/build.gradle.kts:35-35
Timestamp: 2025-05-07T09:12:14.293Z
Learning: The Ktor project maintains a flat Gradle project structure (where projects are referenced without nested paths like `:ktor-test-base`) while keeping a hierarchical directory organization on disk.

Applied to files:

  • gradle/artifacts/publishJvmAndCommonPublications.txt
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.

Applied to files:

  • ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api
  • ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt
📚 Learning: 2025-08-15T08:43:28.980Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/connection.rs:222-236
Timestamp: 2025-08-15T08:43:28.980Z
Learning: In WebRTC.rs (webrtc crate), remote track removal detection is limited compared to browser WebRTC APIs. The onmute event is the most reliable indicator for track removal, as transceiver direction changes after renegotiation don't happen immediately and have asynchronous timing issues. Polling solutions are not recommended due to performance concerns.

Applied to files:

  • ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api
  • ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api
📚 Learning: 2025-07-01T10:54:53.751Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4970
File: build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts:67-72
Timestamp: 2025-07-01T10:54:53.751Z
Learning: In build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts, the KT-70915 and KT-77609 workarounds that limit parallelism for KotlinNativeLink and CInteropCommonizerTask should remain unconditional (not gated behind CI flag) as these limits are beneficial for both local and CI builds.

Applied to files:

  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-05-19T14:56:46.265Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4861
File: teamcity.default.properties:5-7
Timestamp: 2025-05-19T14:56:46.265Z
Learning: The `kotlin.native.jvmargs` property should be kept separate from the `org.gradle.jvmargs` and from the `GRADLE_OPTS` environment variable in CI configuration.

Applied to files:

  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: This is a multiplatform project (JVM, JS, Native), and platform-specific considerations should be respected.

Applied to files:

  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Project is multiplatform (JVM, JS, Native) with tests organized by platform in module's test directories

Applied to files:

  • ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.

Applied to files:

  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
📚 Learning: 2025-08-15T09:12:47.243Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/senders.rs:88-96
Timestamp: 2025-08-15T09:12:47.243Z
Learning: In the webrtc Rust crate version used in the Ktor WebRTC RS implementation, the RTCRtpCodingParameters struct's rtx field is NOT optional and can be accessed directly as params.rtx.ssrc without needing Option handling.

Applied to files:

  • ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
  • ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: Jetty's AbstractConnection class implements Closeable interface, so any class extending AbstractConnection (like JettyWebsocketConnection) can be used with Kotlin's use { } extension function for resource management.

Applied to files:

  • ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api
  • ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: JettyWebsocketConnection in ktor-server-jetty-jakarta implements Closeable interface (has close() method), so connection.use { } is valid Kotlin syntax and will compile correctly.

Applied to files:

  • ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api
🪛 detekt (1.23.8)
ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt

[warning] 15-16: This empty block of code can be removed.

(detekt.empty-blocks.EmptyFunctionBlock)

🔇 Additional comments (12)
gradle/artifacts/publishJvmAndCommonPublications.txt (1)

182-184: The artifact publication entries for ktor-client-webrtc-jvm are correctly formatted and positioned within the publication configuration. No additional API verification is required—the IceServer API with urls as List<String> is already tracked in the existing API definition files at ./ktor-client/ktor-client-webrtc/api/jvm/ and variant directories, with backward compatibility maintained through constructor overloads.

Likely an incorrect or invalid review comment.

ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api (2)

357-373: Android ABI for WebRtc.IceServer correctly preserves old ctor and adds List‑based URLs

The Android API surface now exposes both the old IceServer(String, String, String) ctor and the new IceServer(List, String, String) ctor, with component1, copy, and getUrls all using List. That’s the expected shape for the urls: List<String> migration and should keep existing JVM/Android binaries working while enabling multiple URLs.


766-796: Android WebRtcPeerConnection.runInConnectionScope protected helper matches common API

The added protected runInConnectionScope(Function0) on AndroidWebRtcPeerConnection’s base (WebRtcPeerConnection) lines up with the common declaration, giving platform implementations a scoped helper without expanding the public surface for consumers.

ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api (2)

164-178: runInConnectionScope is correctly surfaced in the klib ABI

The final inline fun runInConnectionScope(Function0<Unit>) entry on WebRtcPeerConnection is present with the expected signature, matching the common declaration and Android/JVM views. That keeps the helper available to platform subclasses without altering the public call surface for regular users.


597-615: Klib ABI for WebRtc.IceServer matches the new List URLs design

The klib dump now has:

  • Primary ctor IceServer(List<String>, String?, String?)
  • Secondary ctor IceServer(String, String?, String?)
  • urls getter, component1, and copy all typed as List<String>

This matches the updated Kotlin data class and mirrors the Android/JVM ABI shape. Existing native code compiled against the old urls: String property will need to be rebuilt, but given the experimental status that’s an acceptable, intentional evolution.

ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api (3)

1-200: New JVM ABI file aligns with existing common WebRTC APIs

This JVM .api surface mirrors the common/klib WebRTC types (data channels, tracks, connection config, stats, etc.), so it brings the JVM target under ABI tracking as required by the project’s /api conventions. Nothing here looks out of sync with the shared definitions.


252-268: JVM ABI for WebRtc.IceServer correctly encodes both String and List‑based constructors

On JVM, WebRtc$IceServer now exposes:

  • The existing IceServer(String, String, String) ctor (for binary compatibility)
  • The new IceServer(List, String, String) ctor
  • component1, copy, and getUrls returning List

That matches the Kotlin data class change to urls: List<String> while keeping previous binaries calling the String‑based ctor valid.


661-691: JVM WebRtcPeerConnection.runInConnectionScope is present as a protected helper

The JVM ABI for WebRtcPeerConnection includes the protected runInConnectionScope(Function0) method, consistent with the common and Android/klib definitions. This gives platform‑specific subclasses a unified way to run code in the connection scope without expanding the public API footprint.

ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt (4)

1-4: LGTM!

Standard copyright header is correct.


5-8: LGTM!

Package declaration and imports are appropriate.


15-16: LGTM! Empty implementation is intentional for JVM.

The empty implementation is correct for the JVM platform, where audio/video permissions don't require runtime grants (unlike Android/iOS). This is a valid no-op implementation of the actual function.

The static analysis warning can be safely ignored.


1-16: Run linting and formatting checks.

Per coding guidelines, run before finalizing:

./gradlew lintKotlin
./gradlew formatKotlin

Comment on lines +10 to +11
# jvm "dummy" srcset is needed for `ktor-client-webrtc-rs`
target.jvm=true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify JVM source structure for ktor-client-webrtc

echo "=== Directory structure of ktor-client-webrtc ==="
find ktor-client/ktor-client-webrtc -type d | head -30

echo -e "\n=== Kotlin/Java source files in ktor-client-webrtc ==="
find ktor-client/ktor-client-webrtc -type f \( -name "*.kt" -o -name "*.java" \) | head -20

echo -e "\n=== gradle.properties content ==="
cat ktor-client/ktor-client-webrtc/gradle.properties

echo -e "\n=== Check for api/ files related to webrtc ==="
find . -path "./api/*" -name "*webrtc*" 2>/dev/null | head -20

echo -e "\n=== Check src directory structure ==="
ls -la ktor-client/ktor-client-webrtc/src/ 2>/dev/null || echo "src/ directory not found"

echo -e "\n=== Look for srcSets definition ==="
find ktor-client/ktor-client-webrtc -name "build.gradle.kts" -exec head -100 {} \;

Repository: ktorio/ktor

Length of output: 8942


Clarify what "dummy" JVM srcset means and why it's required for the Rust component.

The comment's phrasing is misleading: while the main ktor-client-webrtc module has no actual JVM source files, the api/jvm/ directory exists, indicating that JVM API surface must be declared and tracked per binary compatibility requirements. Simply calling it "dummy" obscures this requirement.

Revise the comment to explain the actual purpose: the JVM target is enabled to expose common APIs to JVM consumers and support building the Rust variant (ktor-client-webrtc-rs) with JVM bindings, not as a placeholder.

🤖 Prompt for AI Agents
In @ktor-client/ktor-client-webrtc/gradle.properties around lines 10-11, Update
the misleading comment above target.jvm=true to explain that the JVM target is
intentionally enabled to declare and track the JVM API surface (api/jvm/) for
binary compatibility and to expose JVM bindings for the Rust variant
(ktor-client-webrtc-rs), not merely as a "dummy" placeholder; mention the
existence of api/jvm/ and that this enables JVM consumers and Rust/JNI bindings
to build correctly.

Comment on lines +10 to +13
@OptIn(markerClass = [ExperimentalKtorApi::class])
actual fun createTestWebRtcClient(): WebRtcClient {
TODO("Not yet implemented")
}
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for usage of createTestWebRtcClient in test files
echo "=== Searching for createTestWebRtcClient usage in ktor-client-webrtc ==="
rg -n --type=kotlin 'createTestWebRtcClient' ktor-client/ktor-client-webrtc/

echo ""
echo "=== Searching for expect declaration ==="
rg -n --type=kotlin 'expect.*createTestWebRtcClient' ktor-client/ktor-client-webrtc/

echo ""
echo "=== Checking file structure of ConnectionUtils ==="
fd 'ConnectionUtils' ktor-client/ktor-client-webrtc/

Repository: ktorio/ktor

Length of output: 2384


🏁 Script executed:

#!/bin/bash
# Check how createTestWebRtcClient is used in the test files
echo "=== WebRtcEngineTest.kt usage ==="
rg -n -A5 -B2 'createTestWebRtcClient' ktor-client/ktor-client-webrtc/common/test/io/ktor/client/webrtc/WebRtcEngineTest.kt | head -20

echo ""
echo "=== Check JVM implementation details ==="
cat -n ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt

echo ""
echo "=== Check if JVM tests are skipped ==="
rg -n 'skip|Skip|ignore|Ignore|@Ignore' ktor-client/ktor-client-webrtc/jvm/test/ | head -10

Repository: ktorio/ktor

Length of output: 1189


🏁 Script executed:

#!/bin/bash
# Check the web implementation to understand what's needed
echo "=== Web implementation ==="
cat -n ktor-client/ktor-client-webrtc/web/test/io/ktor/client/webrtc/utils/ConnectionUtils.web.kt

echo ""
echo "=== iOS implementation ==="
cat -n ktor-client/ktor-client-webrtc/ios/test/io/ktor/client/webrtc/utils/ConnectionUtils.ios.kt

Repository: ktorio/ktor

Length of output: 1917


Implement JVM WebRTC test client—the TODO blocks test execution.

The createTestWebRtcClient() function is called in common tests (WebRtcEngineTest, WebRtcDataChannelTest, WebRtcMediaTest) during @BeforeTest setup. The current TODO implementation will throw NotImplementedError and prevent JVM tests from running. All other platforms (Web, iOS, Android) have working implementations. Provide a proper JVM implementation for testing—either using an actual WebRTC backend or a mock equivalent compatible with the JVM environment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The real implementation should be merged soon. Now, the tests on JVM are disabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify that JVM tests are disabled using the IgnoreJvm annotation
echo "=== Checking for IgnoreJvm usage in common tests ==="
rg -n '@IgnoreJvm' ktor-client/ktor-client-webrtc/common/test/

echo ""
echo "=== Checking the IgnoreJvm definition ==="
cat ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt

Length of output: 759


@zibet27, thanks for clarifying! I can confirm that all the WebRTC tests (WebRtcEngineTest, WebRtcMediaTest, and WebRtcDataChannelTest) are properly annotated with @IgnoreJvm, which maps to JUnit's @Disabled. This means the TODO implementation won't cause test failures on JVM. This approach makes sense as a temporary state while awaiting the real implementation. 👍


🧠 Learnings used
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/senders.rs:88-96
Timestamp: 2025-08-15T09:12:47.243Z
Learning: In the webrtc Rust crate version used in the Ktor WebRTC RS implementation, the RTCRtpCodingParameters struct's rtx field is NOT optional and can be accessed directly as params.rtx.ssrc without needing Option handling.

Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: Jetty's AbstractConnection class implements Closeable interface, so any class extending AbstractConnection (like JettyWebsocketConnection) can be used with Kotlin's use { } extension function for resource management.

Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: JettyWebsocketConnection in ktor-server-jetty-jakarta implements Closeable interface (has close() method), so connection.use { } is valid Kotlin syntax and will compile correctly.

Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.

Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.

Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Run relevant tests with './gradlew jvmTest' to verify functionality before submission.

Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Start with JVM-only implementation and tests unless the user specifically requests otherwise

Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Start with JVM-only implementation and tests unless the user specifically requests otherwise.

Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : API changes must be intentional and well-documented

Learnt from: osipxd
Repo: ktorio/ktor PR: 5205
File: ktor-client/ktor-client-curl/desktop/interop/include/curl/multi.h:410-420
Timestamp: 2025-11-27T10:07:26.686Z
Learning: In the ktorio/ktor repository, do not review files under `ktor-client/ktor-client-curl/desktop/interop/include/curl/*` as these are upstream libcurl header files copied from the curl project and not maintained by the Ktor team.

Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module depends on the kotlin("plugin.atomicfu") plugin, so atomicfu should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.

@zibet27 zibet27 enabled auto-merge January 5, 2026 14:13
@zibet27 zibet27 merged commit a2ccfaf into main Jan 5, 2026
14 of 17 checks passed
@zibet27 zibet27 deleted the zibet27/fix-webrtc-ice-server-urls branch January 5, 2026 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants