feat(hang-demo): add connection settings form with broadcast auto-dis… #814
Conversation
…covery Add a collapsible settings panel to the watch demo that allows users to: - Enter a custom relay URL without restarting the dev server - Discover available broadcasts via MoQ SUBSCRIBE_NAMESPACE - Select broadcasts from a dropdown instead of URL query params - Share links with ?url=...&path=... query parameters This improves the developer experience by eliminating the need to manually edit URLs or restart the server when switching relays/broadcasts.
WalkthroughThis change introduces a broadcast discovery feature and form-based UI to the Hang demo. The CSS adds comprehensive styling for a Connection Settings Form with container layouts, input fields, buttons, and status displays. The HTML introduces a collapsible details element containing a form with a relay URL input, a Discover button to search for available broadcasts, a disabled Broadcast select dropdown, and a Connect button. The TypeScript implements a MoQ-based discovery workflow that connects to a relay, subscribes to broadcast announcements, collects available paths within a 3-second timeout, and populates the UI. URL parameter handling enables reading and persisting relay and path selections. The hang-watch element receives an id attribute for reference. Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
js/hang-demo/src/index.html (1)
54-60: Inconsistent indentation in form group.The second
.form-groupdiv and its children use different indentation compared to the first form group (lines 42-53). This appears to be a minor formatting inconsistency.🔎 Proposed fix
- <div class="form-group"> - <label for="broadcast-select">Broadcast</label> - <select id="broadcast-select" disabled> - <option value="">-- Click Discover to find broadcasts --</option> - </select> - <small id="discover-status"></small> - </div> + <div class="form-group"> + <label for="broadcast-select">Broadcast</label> + <select id="broadcast-select" disabled> + <option value="">-- Click Discover to find broadcasts --</option> + </select> + <small id="discover-status"></small> + </div>js/hang-demo/src/index.ts (3)
14-18: Add null checks for form elements to prevent runtime errors.These type assertions assume elements exist. If any element is missing (e.g., HTML typo), the code will fail with cryptic null-access errors at runtime. The
watchelement query above demonstrates the correct pattern.🔎 Proposed fix
-const form = document.getElementById("config-form") as HTMLFormElement; -const relayUrlInput = document.getElementById("relay-url") as HTMLInputElement; -const broadcastSelect = document.getElementById("broadcast-select") as HTMLSelectElement; -const discoverBtn = document.getElementById("discover-btn") as HTMLButtonElement; -const discoverStatus = document.getElementById("discover-status") as HTMLElement; +const form = document.getElementById("config-form") as HTMLFormElement | null; +const relayUrlInput = document.getElementById("relay-url") as HTMLInputElement | null; +const broadcastSelect = document.getElementById("broadcast-select") as HTMLSelectElement | null; +const discoverBtn = document.getElementById("discover-btn") as HTMLButtonElement | null; +const discoverStatus = document.getElementById("discover-status") as HTMLElement | null; + +if (!form || !relayUrlInput || !broadcastSelect || !discoverBtn || !discoverStatus) { + throw new Error("Missing required form elements"); +}
80-86: Misleading comment about connection reuse.The comment states the connection is kept "for potential reuse," but
discoveryConnectionis closed in the form submit handler (lines 180-183) and never reused. Either remove the comment or implement actual reuse (e.g., for subsequent Discover clicks to the same relay).🔎 Proposed fix - clarify the comment
- // Close the announced subscription but keep connection for potential reuse + // Close the announced subscription (connection will be closed on form submit or page unload) announced.close();
137-141: Consider using DOM methods for consistency.The static analysis flagged this
innerHTMLusage. While this specific case is safe (no user input in the string), using DOM methods likedocument.createElementwould be consistent with the pattern used elsewhere (lines 112-131) and avoid the lint warning.🔎 Proposed fix
} catch (err) { console.error("Discovery failed:", err); - broadcastSelect.innerHTML = '<option value="">-- Discovery failed --</option>'; + broadcastSelect.innerHTML = ""; + const option = document.createElement("option"); + option.value = ""; + option.textContent = "-- Discovery failed --"; + broadcastSelect.appendChild(option); discoverStatus.textContent = `Error: ${err instanceof Error ? err.message : "Connection failed"}`; discoverStatus.className = "error";
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
js/hang-demo/src/index.cssjs/hang-demo/src/index.htmljs/hang-demo/src/index.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Frando
Repo: moq-dev/moq PR: 794
File: justfile:150-156
Timestamp: 2025-12-20T13:22:14.684Z
Learning: In moq-relay, WebTransport connections automatically prepend the URL path (e.g., `/anon`) as a prefix to all broadcasts. However, raw QUIC and iroh connections have no HTTP layer, so there's no way to send a path. To match the expected broadcast path for the web UI, the prefix must be manually added to the broadcast name (e.g., `anon/{{name}}`) when publishing via iroh or raw QUIC.
📚 Learning: 2025-12-20T13:22:14.684Z
Learnt from: Frando
Repo: moq-dev/moq PR: 794
File: justfile:150-156
Timestamp: 2025-12-20T13:22:14.684Z
Learning: In moq-relay, WebTransport connections automatically prepend the URL path (e.g., `/anon`) as a prefix to all broadcasts. However, raw QUIC and iroh connections have no HTTP layer, so there's no way to send a path. To match the expected broadcast path for the web UI, the prefix must be manually added to the broadcast name (e.g., `anon/{{name}}`) when publishing via iroh or raw QUIC.
Applied to files:
js/hang-demo/src/index.ts
🧬 Code graph analysis (1)
js/hang-demo/src/index.ts (2)
js/hang/src/watch/element.ts (1)
HangWatch(18-209)js/lite/src/connection/established.ts (1)
Established(6-14)
🪛 ast-grep (0.40.3)
js/hang-demo/src/index.ts
[warning] 138-138: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: broadcastSelect.innerHTML = '-- Discovery failed --'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 108-108: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: broadcastSelect.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 138-138: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: broadcastSelect.innerHTML = '-- Discovery failed --'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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). (1)
- GitHub Check: Check
🔇 Additional comments (5)
js/hang-demo/src/index.css (1)
14-149: LGTM! Well-structured form styling.The CSS provides comprehensive styling with proper interactive states (hover, focus, disabled). The focus states correctly maintain visual indicators via
box-shadow, preserving accessibility.js/hang-demo/src/index.html (1)
33-35: LGTM!Adding
id="watch"aligns with the existing code examples in the documentation below and enables the TypeScript to interact with the element.js/hang-demo/src/index.ts (3)
20-32: LGTM!URL parameter handling correctly reads query params and applies them to both the watch element and the form input for consistency.
147-184: LGTM!Form submission handler properly validates inputs, updates the watch element attributes, syncs URL parameters for sharing, and cleans up the discovery connection.
186-191: LGTM!Proper cleanup of the discovery connection on page unload prevents resource leaks.
|
Thanks for the video. Could you remove the collapse functionality? Always show the server URL and broadcast name as input fields. The name needs to be an input field because some MoQ relays don't support announce yet. Can you also auto-discover broadcasts, instead of requiring a click? And clicking on them will populate the input box. And finally, could you move this code into a separate file? I want index.ts and index.css to be very simple because this is part demo, part hello world. It might be cool as a web component too? Also finally, minimize the styling so it doesn't stand out as much. The current demo is very bland on purpose. |
2 participants
Add connection settings form with broadcast auto-discovery
Closes #767
Summary
Adds a collapsible "Connection Settings" panel to the watch demo that allows users to:
Demo
cursorful-video-1767561529599.1.mp4
Changes