Skip to content

Conversation

@kixelated
Copy link
Collaborator

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 12, 2026

Walkthrough

This pull request strengthens error handling and validation across the WebTransport protocol implementation. Changes include converting non-fatal warnings into hard errors for unsupported deltas and subgroup IDs, restructuring error handling in the connection layer to separate concerns between stream processing and error logging, and improving type annotations for protocol variables. The modifications affect both the TypeScript and Rust implementations, with emphasis on preventing silent failures by treating previously-ignored edge cases as exceptions that propagate to callers.

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess relevance to the changeset. Add a description explaining why non-zero sub-group streams should be dropped rather than warned about.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: converting non-zero sub-group handling from warnings to errors that drop streams.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch better-uninterop

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
js/lite/src/ietf/connection.ts (1)

286-293: Consider the double stream.stop() path when handleGroup catches internally.

When an error occurs inside handleGroup's internal try/catch (e.g., Frame.decode throws), handleGroup calls stream.stop(e) and returns normally. Then the .then() handler calls stream.stop(new Error("cancel")) a second time.

This is harmless because Reader.stop() is effectively idempotent—it calls this.#reader?.cancel(reason).catch(() => void 0), which silently suppresses any rejection from cancelling an already-cancelled reader. However, it creates two different error-handling paths:

  1. Errors thrown before handleGroup's try block (e.g., non-zero subGroupId) → propagate to .catch() → single stream.stop(err).
  2. Errors inside handleGroup's try block → caught with stream.stop(e).then() fires → stream.stop(new Error("cancel")).

Consider having handleGroup re-throw after cleanup so all errors flow consistently through .catch().


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.

@kixelated kixelated enabled auto-merge (squash) February 12, 2026 12:43
@kixelated kixelated merged commit 85a6df9 into main Feb 12, 2026
1 check passed
@kixelated kixelated deleted the better-uninterop branch February 12, 2026 12:48
This was referenced Feb 12, 2026
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.

1 participant