chore: resolve actionable TODO/FIXME comments#73
Conversation
1b47706 to
501d281
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Resolves four actionable TODO/FIXME items discovered in an audit by replacing them with clarified documentation and a small correctness/performance improvement in MIME type matching.
Changes:
- Replace MIME type
.to_uppercase()comparisons witheq_ignore_ascii_case()for allocation-free ASCII-only comparisons. - Convert TODO/FIXME markers into explanatory comments with relevant RFC references.
- Clarify mDNS default behavior as opt-in with additional wrapper configuration requirements.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rtc/src/rtp_transceiver/rtp_sender/rtp_codec.rs | Uses eq_ignore_ascii_case() for MIME type matching and removes a misleading TODO. |
| rtc/src/peer_connection/configuration/setting_engine.rs | Replaces a misleading TODO with an explanation of the opt-in mDNS design. |
| rtc-turn/src/proto/evenport.rs | Replaces an uncertain FIXME with an RFC-backed explanation of the R-flag bit mask. |
| rtc-sctp/src/endpoint/mod.rs | Replaces a DoS TODO with documentation of existing protections and RFC references for future hardening. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #73 +/- ##
==========================================
- Coverage 71.17% 71.16% -0.01%
==========================================
Files 442 442
Lines 67330 67330
==========================================
- Hits 47922 47916 -6
- Misses 19408 19414 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- rtc-turn/proto/evenport.rs: Clarify EVEN-PORT R-flag constant — the FIXME suggested (1<<8)-1=255 but RFC 5766 §14.6 requires only the MSB; existing value 0b10000000 is correct. Replace FIXME with an explanatory comment. - rtc/rtp_transceiver/rtp_sender/rtp_codec.rs: Replace TODO comment on unicode case-folding with eq_ignore_ascii_case(), which is allocation-free and correct for ASCII-only MIME types. - rtc-sctp/src/endpoint/mod.rs: Replace DoS TODO with a precise comment referencing RFC 4960 §5.1.3 and documenting the existing protection. - rtc/peer_connection/configuration/setting_engine.rs: Replace misleading mDNS "Re-enable it to QueryOnly" TODO with a comment explaining the opt-in design. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix rustfmt issue in rtp_codec.rs (long method chain line) - Remove brittle function name references in comments per Copilot review: - setting_engine.rs: replace with_mdns_mode() reference with behavioral description - endpoint/mod.rs: replace handle_first_packet() reference with behavioral description Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
a9330e8 to
12eedbd
Compare
|
Rebased onto upstream/master so this PR contains only its own changes. Previous branch structure caused merge conflicts when PRs were merged in sequence. Each PR is now independently mergeable. |
Summary
Cleans up four concrete TODO/FIXME items found in a codebase-wide audit:
rtc-turn/src/proto/evenport.rs:37FIXME? (1 << 8) - 1-- the existing0b10000000value is correct per RFC 5766 §14.6 (only the MSB is the R flag); replaced the FIXME with an explanatory commentrtc/src/rtp_transceiver/rtp_sender/rtp_codec.rs:145TODO: unicode case-folding+.to_uppercase()witheq_ignore_ascii_case()-- allocation-free and correct for ASCII-only MIME typesrtc-sctp/src/endpoint/mod.rs:133concurrent_associationsprotection and referencing RFC 4960 §5.1.3 for a future stateless cookie implementationrtc/src/peer_connection/configuration/setting_engine.rs:175Re-enable it to QueryOnlyTODO with a comment explaining the opt-in design (both the core and the async wrapper require explicit configuration)Test plan
cargo buildpassescargo test -p rtcpasses (246 tests)cargo test -p rtc-sctppassescargo test -p rtc-turnpassescargo fmt --checkpassescargo clippypassesGenerated with Claude Code