feat(voice): WebRTC voice/stream channels with text chat#198
Merged
Conversation
Add a Discord-style voice channel UI for the new `^` channel type served by the obbyircd voice-channels module + hosted-backend SFU. - src/lib/voice.ts: VoiceClient handling RTCPeerConnection, signaling via TAGMSG (`+obsidianirc/rtc` JSON tag), getUserMedia, screen share toggle, and AudioContext-based VAD that broadcasts speaking state. - src/components/layout/VoiceChannelView.tsx: participant grid with speaking ring, mic/video/screen/deafen/leave controls, hidden audio sink for Safari autoplay. - AppLayout: route `^` channels to VoiceChannelView instead of ChatArea and hide the member list (the grid is the member list). - ChannelList: dedicated voice section with speaker icon and `^`-prefix add input.
Previously the release APK came out as app-universal-release-unsigned.apk
and Android refused to install it ("App not installed as package appears
to be invalid").
Add a signingConfig that prefers a keystore.properties file at the gradle
root if present (production / Play Store), and falls back to the standard
~/.android/debug.keystore for local sideload builds. No-op if neither
exists.
…ons, hand-raise, PTT, per-peer mute, quality dots
End-to-end overhaul of voice/video so every flow we hit during testing
actually works: mid-session camera/screen toggling, late joiners,
multiple Firefox tabs in the same room, screen-share keyframe
delivery.
src/lib/voice.ts
* Module-level VoiceClient registry keyed by serverId+channel so
the call lifecycle is decoupled from the React component. Acquire/
release; the unmount of VoiceChannelView no longer parts the call
and the user can navigate away to a text channel mid-conversation
without flapping JOIN/PART (and tripping 'too many joins, please
wait a while').
* Hidden document-level <audio> sink lives in voice.ts so audio
keeps playing across React unmount.
* Re-entrance guard on onJoined: a duplicate 'joined' envelope
(from a leaked subscription on an old VoiceClient) no longer
spawns a second RTCPeerConnection and flood the channel with
duplicate ICE candidates from the orphan PC.
* leave() always runs teardown (was: returned early if state was
already 'idle', leaking the TAGMSG handler).
* trackOwners + midOwners hint maps populated from every SDP-carrying
envelope (joined / offer / answer). attachInboundTrack resolves
a remote track to a publisher in this priority:
1. transceiver.mid -- the only ID Firefox reliably preserves on
mid-session-added tracks.
2. trackOwners[track.id] -- works when the browser honors a=msid.
3. MediaStream.id matched against members.
4. track.id prefix split.
Tracks that can't be resolved are dropped with a warn rather
than minting a phantom UUID-named member.
* Pending-ICE buffer drained after every successful setRemoteDescription
-- ICE that arrives before the first remoteDescription used to
trip 'DOMException: No remoteDescription'.
* Server-pushed 'offer' handler: setRemoteDescription, createAnswer,
setLocalDescription, and emit 'answer' back. Without this the SFU's
follow-up renegotiations (when a new peer publishes a track on
an already-connected viewer's PC) are never acknowledged and the
viewer's PC stays stuck in have-local-offer.
* sendSignal chunks SDP-carrying envelopes when the raw SDP exceeds
~3 KB. Each chunk shares an 'id' and carries seq/total; the SFU
stitches them back. Without chunking, a video offer/answer's IRCv3-
escaped form blew through the server's CLIENT_TAG_SIZE_LIMIT.
* setMic / setVideo / setScreenShare update the self-member tile so
the user sees their own preview locally.
* setScreenShare re-checks this.pc *after* getDisplayMedia awaits;
the screen-picker dialog can sit open for many seconds and the
call can be torn down underneath it (hangup, error). Previously
crashed with 'addTrack of undefined' and leaked the captured
MediaStream.
* setHandRaised / sendReaction / setMemberMuted / setPushToTalk +
a 3 s pc.getStats() pump that scores each peer's connection
quality from packet loss.
* teardown clears trackOwners, midOwners, localMutes, pendingIce,
statsTimer.
src/components/layout/VoiceChannelView.tsx
* Click-to-spotlight: clicking a tile centers it large with the
rest in a thumbnail strip below.
* Per-tile mute (silences inbound audio for me only via track.enabled).
* Hand-raise toggle + yellow hand badge on the tile.
* Push-to-talk mode: hold Space to talk while pttMode is on.
* Connection-quality dot on each remote tile (good / ok / poor).
* Emoji reaction picker; reactions float up and fade out via the
new keyframe in index.css.
* VoiceClient ownership moved out of the component effect; navigating
away from the channel no longer disconnects.
src/index.css
* @Keyframes react-float for the floating-emoji reaction animation.
src/lib/ircUtils.tsx
* parseMessageTags now properly un-escapes IRCv3 message-tag values
(\: -> ;, \s -> space, \\ -> \, \r, \n). Without this the
SDP / ICE payloads server-emitted with escapes (every space
becomes \s) hit JSON.parse with literal '\s' and threw.
src/lib/irc/IRCClient.ts
* Add 'draft/account-2fa' to the CAP REQ batch so the 2FA settings
button can detect server support.
…-echo - Add obsidianirc/voice to ourCaps so the new ircd-side gate accepts the JOIN to ^channels. Without this REQ the server now rejects voice-channel JOINs with ERR_NOSUCHCHANNEL. - Replace fixed 3000-raw-byte SDP chunks with a wire-size budget of 7800 bytes. The previous heuristic underestimated the JSON+IRC-tag escape blowup (CR/LF go 1 byte raw -> 3 on the wire, spaces 1 -> 2) and tripped MAXTAGSIZE=8192 on dense video offers. The chunker now walks the SDP accumulating per-character encoded cost so each emitted TAGMSG fits regardless of newline density. - Drop inbound audio tracks resolved to selfNick before piping them to the audio sink. The SFU already filters self in fan-out, but a stale peer left over from a nick-collision rename could still leak own audio back; this is defense-in-depth at the sink.
Replaces the ergo+echobot dev/testing harness with a single obsidian service exposed via traefik (host rule, TLS), wiring VITE_DEFAULT_* build args from environment and dropping the gen-certs / docker mock ircd / chatbot helpers that don't belong in the deploy stack.
ChromeOS dispatches https/http deep links through org.chromium.arc.intent.action.VIEW rather than android.intent.action.VIEW, so the existing intent-filters never matched and the app didn't claim the URL. Add the ARC++ action alongside the standard one in both deep-link filters.
Adds the client-side counterpart to the obbyircd commit that introduced $-prefixed stream channels. The room mode (voice vs stream) is read from VoiceState.mode and drives the VoiceChannelView layout (streamer-publisher controls vs viewer chat). Channel-list rendering, the channel-list modal, the AppLayout dispatch, ircUtils helpers and the message handlers all now treat $ as a channel prefix on equal footing with #/^. Also ships the 401 (no such nick/channel) fixes to the message-routing path and the small biome/TS cleanups (noParameterAssign rename in VoiceClient.onSignal, pttMode field declaration) needed to clear the pipeline.
Switch inbound audio routing from a single shared <audio> sink to a per-peer hidden sink so each streamer's volume can be set independently on this client. The shared sink stays mounted to inherit the existing user-gesture autoplay grant; actual playback is per-peer. Adds VoiceClient.setMemberVolume(nick, vol) which sets member.volume in [0,1] and applies it to the per-peer sink; volume == 0 also flips the local-mute flag, so the existing mute button and the slider stay in sync. In stream rooms the ParticipantTile bottom bar now shows a 0..100% range slider in place of the binary mute icon. Voice rooms keep the compact mute button since their tile grid is denser.
`^` voice channels accept PRIVMSG just like any other channel — the voice-channels module on the IRCd only intercepts SDP-bearing TAGMSG. The client was hiding chat for `^`-channels but rendering it for `$`- channels through a duplicated branch. Unify both into the same voice-on-top + ChatArea-below split so users on a voice call can also talk in the channel.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughImplements WebRTC voice/stream support: a VoiceClient WebRTC library with SFU signaling via IRC TAGMSG, IRC protocol updates for voice capability and channel detection, phantom-channel self-heal JOINs, voice UI (VoiceChannelView, ChannelList, ChatHeader/ChatArea updates), reaction animation CSS, and Docker compose routing changes. ChangesVoice Channel WebRTC Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
The slider was previously gated to $-stream rooms only; in ^-voice rooms peers got just a mute icon. Now both controls render side-by-side in both modes, so users can adjust per-peer volume in voice channels too. The unused `showVolumeSlider` prop is dropped.
…nnels
- Hide the "Voice Channels" and "Streams" sections in ChannelList when
the server didn't negotiate obsidianirc/voice. Networks without the
cap can't talk to the SFU, so the rows are nothing but dead links.
- Chat header now renders FaMicrophone for ^-channels and FaDesktop for
$-channels instead of the # hashtag. The displayed channel name is
stripped of its prefix (we already render the icon, so "^test"
becoming "test" beside the mic is the right visual).
- Add & to isChannelTarget so RFC 1459/2811 local channels round-trip
through CHANMSG/USERMSG and MODE dispatch correctly. Replaced the
scattered `target.startsWith("#")` checks with isChannelTarget in
IRCClient.ts, protocol/mode.ts, store/handlers/channels.ts and
store/handlers/metadata.ts so future CHANTYPES additions only need
one update. The text-channel filter in ChannelList already includes
any non-^/$ prefix so &-channels appear in the text section
automatically. The "new channel" input accepts either # or & verbatim.
- getChannelDisplayName now strips any of [#&^$], not just #, so the
prefix-icon + bare-name pattern is consistent across the UI.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
compose.yaml (1)
1-19: Consider production-readiness enhancements for WebRTC deployment.Given that this PR adds WebRTC voice/stream channels requiring secure WebSocket (WSS) and media connections, the deployment should ensure:
Healthchecks: Re-add container healthchecks to verify the web server is responding (removed per AI summary). Example:
healthcheck: test: ["CMD", "wget", "--quiet", "--tries=1", "--spider", "http://localhost:80/"] interval: 30s timeout: 10s retries: 3Service dependencies: If Traefik is in the same compose stack, add
depends_onto ensure proper startup order.Resource limits: WebRTC can be resource-intensive; consider adding memory/CPU limits for predictable performance.
Logging configuration: Ensure container logs are accessible for debugging connection issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compose.yaml` around lines 1 - 19, Add production-ready deployment settings to the obsidian service: reintroduce a healthcheck block (use the healthcheck key and a simple HTTP check against localhost:80), add depends_on to ensure Traefik starts before obsidian if Traefik is in the same stack, add resource limits using deploy.resources (limits and reservations for memory/cpu) to cap WebRTC load, and add a logging configuration block (driver and options) so container logs are persisted/accessible for debugging; update the compose service named obsidian to include these keys (healthcheck, depends_on, deploy.resources, logging) alongside the existing build/image/labels entries.src/index.css (1)
864-873: ⚡ Quick winRespect reduced-motion preferences for the new floating reaction animation.
Please add a
prefers-reduced-motionoverride to avoid motion-heavy effects for sensitive users.🎯 Proposed patch
.animate-react-float { animation: react-float 2.4s cubic-bezier(0.16, 1, 0.3, 1) forwards; } + +@media (prefers-reduced-motion: reduce) { + .animate-react-float { + animation: none; + opacity: 1; + transform: none; + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.css` around lines 864 - 873, Add a prefers-reduced-motion override so users who request reduced motion won't see the react-float animation: wrap a `@media` (prefers-reduced-motion: reduce) block that targets the `@keyframes` react-float and the .animate-react-float class (the unique symbols to change) and disable the animation (e.g., set .animate-react-float { animation: none !important; } and/or force static end-state styles like transform: translateY(0) scale(1); opacity: 1;) so the floating reaction becomes non-animating for reduced-motion users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compose.yaml`:
- Around line 14-18: The Traefik TLS labels are incomplete: update the Docker
service labels for the obsidian router by adding an entrypoint label
(traefik.http.routers.obsidian.entrypoints, e.g. set to "websecure") and a cert
resolver label (traefik.http.routers.obsidian.tls.certresolver, e.g.
"letsencrypt") so Traefik knows which entrypoint to use and how to obtain
certificates; ensure you replace "websecure" and "letsencrypt" with the actual
entrypoint and certResolver names used in your Traefik configuration.
- Around line 12-18: The obsidian service uses expose and Traefik labels but is
not attached to Traefik's network; update the obsidian service (the service that
includes expose: "80" and labels like traefik.http.routers.obsidian.*) to
include a networks: entry referencing the Traefik network name, and add a
matching top-level networks: definition (or use the existing Traefik network
name if Traefik is in this compose). Ensure the network name matches the one
Traefik is listening on so Traefik can discover and route to the obsidian
container.
- Around line 6-8: The docker-compose build args reference infra variables
(IRC_FQDN, NETWORK_NAME, WEB_FQDN) that are not documented; add a
deployment-level .env (or extend .env.example) and/or a README "Deployment
(docker-compose)" section that lists and explains IRC_FQDN, NETWORK_NAME, and
WEB_FQDN, shows example values and instructs users to export or create the .env
at repo root before running docker-compose up so the VITE_* build args in
compose.yaml resolve correctly.
In `@src/components/layout/AppLayout.tsx`:
- Around line 66-68: The mobile pager still exposes the MemberList for
voice/stream channels because the mobile-page visibility logic doesn't reuse the
voice-channel guard; update the mobile-page calculation to include the same
!isVoiceChannel condition used in shouldShowMemberList (which currently is:
const shouldShowMemberList = isMemberListVisible && !selectedPrivateChatId &&
!isVoiceChannel) so that the mobile member page is suppressed for voice/stream
channels as well; locate the mobile pager/mobile-page visibility variable and
add the !isVoiceChannel check alongside isMemberListVisible and
!selectedPrivateChatId.
In `@src/components/layout/ChannelList.tsx`:
- Around line 376-385: The add-channel flow mangles `&` prefixes because a
leading `#` can be present before `&` (e.g. `#&name`), so update
handleAddChannel to normalize that case: compute a cleaned string from
newChannelName.trim() that strips a leading `#` only when the next char is `&`
(e.g. if trimmed.startsWith("#&") then cleaned = trimmed.slice(1) else cleaned =
trimmed), then build channelName by checking cleaned.startsWith("#") or
cleaned.startsWith("&") and otherwise prefixing "#" — use the existing symbols
newChannelName, trimmed, cleaned (new variable) and handleAddChannel to
implement this.
In `@src/components/layout/VoiceChannelView.tsx`:
- Around line 53-54: The local booleans videoOn and screenOn in the
VoiceChannelView component are being optimistically toggled before the async
publishing calls (setVideo() / setScreenShare()) complete, causing the UI to get
out of sync if those calls fail or the browser ends screen share; change this to
derive publish state from the connected self member (e.g., inspect
self.publications or self.track/publish state) or only call
setVideoOn/setScreenOn after the async client call resolves successfully (and
revert on failure); update the handlers that call setVideo() and
setScreenShare() (and the footer button rendering logic) to read from the
authoritative self member state instead of flipping local state immediately, and
ensure any cleanup (browser picker cancelling) updates the same derived state
path.
- Around line 553-656: The outer tile is currently a <button> containing
interactive children (range input and inner button) which is invalid; change the
outer element used in VoiceChannelView (the element with onClick and className
that wraps the whole tile) from a <button> to a non-interactive <div> (preserve
the same className, aspect, large logic and ring classes) and remove the onClick
from that wrapper; then add a dedicated interactive spotlight button that uses
the existing onClick handler (create a new <button> overlay or small control
inside the tile, give it aria-label/role and tabIndex for keyboard access) so
volume <input> and mute button are not nested inside another button; ensure
existing handlers that call onMuteClick, onSetVolume, videoRef, and
accessibility props (title/aria-label) remain unchanged and add stopPropagation
where needed on inner controls to avoid triggering the spotlight button.
In `@src/components/ui/ChannelListModal.tsx`:
- Around line 597-599: The fallback that shows channel.channel in
ChannelListModal is not stripping the '&' prefix, causing '&' channels to
display the raw prefix; update the fallback logic (the span that renders
{displayName || channel.channel.replace(...)} ) to strip '&' as well—e.g.
include '&' in the character class used by the replace (or call the shared
channel-name sanitiser if one exists) so the regex removes leading '#', '^',
'$', and '&' before rendering.
In `@src/lib/voice.ts`:
- Around line 440-445: The publish path currently calls this.pc.addTrack(track,
this.localStream) and sets selfMember.videoTrack but the off/toggle path only
stops the MediaStreamTrack and removes it from the MediaStream, leaving
RTCRtpSenders/transceivers attached; modify the publish/unpublish logic to track
the RTCRtpSender returned by this.pc.addTrack (store it on selfMember e.g.
selfMember.videoSender) and when turning a track off remove that sender from the
RTCPeerConnection (call this.pc.removeTrack(sender)) or call
sender.replaceTrack(null) and clear the stored sender, also ensure equivalents
for audio/screen-share paths and update members[selfNick] before calling await
this.renegotiate() so stale senders/transceivers are not advertised.
- Around line 357-359: The setDeafened method currently only toggles
this.opts.audioSink.muted; extend it to also mute/unmute every per-peer sink in
memberAudioSinks (e.g., iterate memberAudioSinks.values() and set eachSink.muted
= on) and store the deafened state on the instance (this.deafened = on) so that
code which adds new sinks (where memberAudioSinks entries are created) can apply
the muted state when attaching a sink for a joining peer.
- Around line 317-329: The handler currently only filters by serverId so
signaling for all channels on the same server is processed; restrict it to the
active channel by adding a check that p.channelName matches this instance's
active channel identifier (e.g. this.channelName or this.activeChannel) before
parsing the RTC_TAG and calling this.onSignal; update the handler function (the
closure that receives p, checks this.opts.serverId, reads RTC_TAG, parses
SignalEnvelope and calls this.onSignal) to return early unless p.channelName ===
this.<active channel property>.
---
Nitpick comments:
In `@compose.yaml`:
- Around line 1-19: Add production-ready deployment settings to the obsidian
service: reintroduce a healthcheck block (use the healthcheck key and a simple
HTTP check against localhost:80), add depends_on to ensure Traefik starts before
obsidian if Traefik is in the same stack, add resource limits using
deploy.resources (limits and reservations for memory/cpu) to cap WebRTC load,
and add a logging configuration block (driver and options) so container logs are
persisted/accessible for debugging; update the compose service named obsidian to
include these keys (healthcheck, depends_on, deploy.resources, logging)
alongside the existing build/image/labels entries.
In `@src/index.css`:
- Around line 864-873: Add a prefers-reduced-motion override so users who
request reduced motion won't see the react-float animation: wrap a `@media`
(prefers-reduced-motion: reduce) block that targets the `@keyframes` react-float
and the .animate-react-float class (the unique symbols to change) and disable
the animation (e.g., set .animate-react-float { animation: none !important; }
and/or force static end-state styles like transform: translateY(0) scale(1);
opacity: 1;) so the floating reaction becomes non-animating for reduced-motion
users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc9cc9b7-996b-43c9-a64d-ce07cd2982fe
⛔ Files ignored due to path filters (2)
src-tauri/gen/android/app/build.gradle.ktsis excluded by!**/gen/**src-tauri/gen/android/app/src/main/AndroidManifest.xmlis excluded by!**/gen/**
📒 Files selected for processing (17)
compose.yamlsrc/components/layout/AppLayout.tsxsrc/components/layout/ChannelList.tsxsrc/components/layout/ChatArea.tsxsrc/components/layout/ChatHeader.tsxsrc/components/layout/VoiceChannelView.tsxsrc/components/ui/ChannelListModal.tsxsrc/index.csssrc/lib/irc/IRCClient.tssrc/lib/irc/handlers/messages.tssrc/lib/ircUtils.tsxsrc/lib/voice.tssrc/protocol/mode.tssrc/store/handlers/channels.tssrc/store/handlers/messages.tssrc/store/handlers/metadata.tssrc/store/index.ts
Comment on lines
+6
to
+8
| args: | ||
| VITE_DEFAULT_IRC_SERVER: wss://${IRC_FQDN} | ||
| VITE_DEFAULT_IRC_SERVER_NAME: ${NETWORK_NAME} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if Dockerfile declares and uses the build args
rg -n 'ARG (VITE_DEFAULT_IRC_SERVER|VITE_DEFAULT_IRC_SERVER_NAME)' DockerfileRepository: ObsidianIRC/ObsidianIRC
Length of output: 132
🏁 Script executed:
# Check for existing environment variable documentation
find . -maxdepth 2 -type f \( -name ".env.example" -o -name ".env" -o -name "README*" \) | head -20Repository: ObsidianIRC/ObsidianIRC
Length of output: 94
🏁 Script executed:
# Search for references to IRC_FQDN and NETWORK_NAME in documentation/config files
rg -i "(IRC_FQDN|NETWORK_NAME|WEB_FQDN)" --type markdown --type txt --type yaml . | grep -v "compose.yaml" | head -20Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
# Read .env.example to see what's documented
cat -n ./.env.exampleRepository: ObsidianIRC/ObsidianIRC
Length of output: 619
🏁 Script executed:
# Search README for environment variable section
head -100 README.mdRepository: ObsidianIRC/ObsidianIRC
Length of output: 5066
🏁 Script executed:
# Search for docker or deployment documentation
find . -maxdepth 2 -type f \( -name "docker*" -o -name "DOCKER*" -o -name "deploy*" \) | grep -v ".git"Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
# Check if there's a docker-compose override or additional config
ls -la | grep -E "compose|docker"Repository: ObsidianIRC/ObsidianIRC
Length of output: 260
🏁 Script executed:
# Check what's in the docker directory
ls -la docker/Repository: ObsidianIRC/ObsidianIRC
Length of output: 368
🏁 Script executed:
# Check if there's a .env file (not .env.example)
ls -la | grep "\.env"Repository: ObsidianIRC/ObsidianIRC
Length of output: 133
Document required environment variables for docker-compose deployment.
The build args in compose.yaml (lines 6-8) reference ${IRC_FQDN}, ${NETWORK_NAME}, and ${WEB_FQDN} (line 16), but these infrastructure-level variables are undocumented. The existing .env.example documents application variables (VITE_*), not the environment variables required for docker-compose deployment. Without documentation, users deploying via docker-compose will not know these required variables must be set.
Add a .env file or deployment documentation clarifying the required variables:
📝 Recommendation
Create a .env file at the repository root documenting infrastructure deployment variables:
# Required for docker-compose deployment
IRC_FQDN=irc.example.com
NETWORK_NAME="My IRC Network"
WEB_FQDN=web.example.comOr add a deployment section to README.md explaining these variables and how to set them before running docker-compose up.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@compose.yaml` around lines 6 - 8, The docker-compose build args reference
infra variables (IRC_FQDN, NETWORK_NAME, WEB_FQDN) that are not documented; add
a deployment-level .env (or extend .env.example) and/or a README "Deployment
(docker-compose)" section that lists and explains IRC_FQDN, NETWORK_NAME, and
WEB_FQDN, shows example values and instructs users to export or create the .env
at repo root before running docker-compose up so the VITE_* build args in
compose.yaml resolve correctly.
Comment on lines
+12
to
+18
| expose: | ||
| - "80" | ||
| labels: | ||
| - traefik.enable=true | ||
| - traefik.http.routers.obsidian.rule=Host(`${WEB_FQDN}`) | ||
| - traefik.http.routers.obsidian.tls=true | ||
| - traefik.http.services.obsidian.loadbalancer.server.port=80 |
Contributor
There was a problem hiding this comment.
Missing Traefik network configuration.
The service has been changed from published ports to expose, meaning it's only accessible via the Docker internal network. The Traefik labels route traffic to this service, but there's no networks: section connecting the obsidian service to Traefik's network.
Without a shared network, Traefik cannot discover or route to this container, making the service unreachable.
🔧 Proposed fix
Define and attach a network that Traefik uses:
labels:
- traefik.enable=true
- traefik.http.routers.obsidian.rule=Host(`${WEB_FQDN}`)
- traefik.http.routers.obsidian.tls=true
- traefik.http.services.obsidian.loadbalancer.server.port=80
+ networks:
+ - traefik-network
+
+networks:
+ traefik-network:
+ external: trueAlternatively, if Traefik is defined in this same compose file, use that network name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@compose.yaml` around lines 12 - 18, The obsidian service uses expose and
Traefik labels but is not attached to Traefik's network; update the obsidian
service (the service that includes expose: "80" and labels like
traefik.http.routers.obsidian.*) to include a networks: entry referencing the
Traefik network name, and add a matching top-level networks: definition (or use
the existing Traefik network name if Traefik is in this compose). Ensure the
network name matches the one Traefik is listening on so Traefik can discover and
route to the obsidian container.
Comment on lines
+14
to
+18
| labels: | ||
| - traefik.enable=true | ||
| - traefik.http.routers.obsidian.rule=Host(`${WEB_FQDN}`) | ||
| - traefik.http.routers.obsidian.tls=true | ||
| - traefik.http.services.obsidian.loadbalancer.server.port=80 |
Contributor
There was a problem hiding this comment.
Incomplete Traefik TLS configuration.
The labels enable TLS (traefik.http.routers.obsidian.tls=true) but don't specify:
- Which Traefik entrypoint to use (typically
websecurefor HTTPS on port 443) - How to obtain TLS certificates (cert resolver for Let's Encrypt, etc.)
Without these, HTTPS routing may not work as expected.
🔒 Proposed additions
labels:
- traefik.enable=true
- traefik.http.routers.obsidian.rule=Host(`${WEB_FQDN}`)
+ - traefik.http.routers.obsidian.entrypoints=websecure
- traefik.http.routers.obsidian.tls=true
+ - traefik.http.routers.obsidian.tls.certresolver=letsencrypt
- traefik.http.services.obsidian.loadbalancer.server.port=80Adjust websecure and letsencrypt to match your Traefik configuration. If using a different cert resolver or entrypoint name, update accordingly.
📝 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.
Suggested change
| labels: | |
| - traefik.enable=true | |
| - traefik.http.routers.obsidian.rule=Host(`${WEB_FQDN}`) | |
| - traefik.http.routers.obsidian.tls=true | |
| - traefik.http.services.obsidian.loadbalancer.server.port=80 | |
| labels: | |
| - traefik.enable=true | |
| - traefik.http.routers.obsidian.rule=Host(`${WEB_FQDN}`) | |
| - traefik.http.routers.obsidian.entrypoints=websecure | |
| - traefik.http.routers.obsidian.tls=true | |
| - traefik.http.routers.obsidian.tls.certresolver=letsencrypt | |
| - traefik.http.services.obsidian.loadbalancer.server.port=80 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@compose.yaml` around lines 14 - 18, The Traefik TLS labels are incomplete:
update the Docker service labels for the obsidian router by adding an entrypoint
label (traefik.http.routers.obsidian.entrypoints, e.g. set to "websecure") and a
cert resolver label (traefik.http.routers.obsidian.tls.certresolver, e.g.
"letsencrypt") so Traefik knows which entrypoint to use and how to obtain
certificates; ensure you replace "websecure" and "letsencrypt" with the actual
entrypoint and certResolver names used in your Traefik configuration.
Comment on lines
+66
to
+68
| // Hide member list for private chats and voice channels (voice has its own grid) | ||
| const shouldShowMemberList = | ||
| isMemberListVisible && !selectedPrivateChatId && !isVoiceChannel; |
Contributor
There was a problem hiding this comment.
Hide the mobile member page for voice/stream channels too.
This only suppresses the desktop/sidebar member list. On narrow view, the mobile pager still treats the channel as having a member page, so voice channels can still swipe into the old MemberList screen even though the voice grid is supposed to replace it. Reuse the same !isVoiceChannel condition in the mobile-page calculation as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/AppLayout.tsx` around lines 66 - 68, The mobile pager
still exposes the MemberList for voice/stream channels because the mobile-page
visibility logic doesn't reuse the voice-channel guard; update the mobile-page
calculation to include the same !isVoiceChannel condition used in
shouldShowMemberList (which currently is: const shouldShowMemberList =
isMemberListVisible && !selectedPrivateChatId && !isVoiceChannel) so that the
mobile member page is suppressed for voice/stream channels as well; locate the
mobile pager/mobile-page visibility variable and add the !isVoiceChannel check
alongside isMemberListVisible and !selectedPrivateChatId.
Comment on lines
376
to
385
| const handleAddChannel = () => { | ||
| if (selectedServerId && newChannelName.trim()) { | ||
| const channelName = newChannelName.trim().startsWith("#") | ||
| ? newChannelName.trim() | ||
| : `#${newChannelName.trim()}`; | ||
| // Keep any user-typed text channel prefix (`#` or `&`); default | ||
| // to `#` so the most common case still Just Works. | ||
| const trimmed = newChannelName.trim(); | ||
| const channelName = | ||
| trimmed.startsWith("#") || trimmed.startsWith("&") | ||
| ? trimmed | ||
| : `#${trimmed}`; | ||
|
|
Contributor
There was a problem hiding this comment.
& channel prefix still gets mangled in add flow.
This handler tries to preserve &, but current text-input normalization still yields #&name, so & channels are not joined correctly.
Proposed fix
const handleAddChannel = () => {
if (selectedServerId && newChannelName.trim()) {
- const trimmed = newChannelName.trim();
- const channelName =
- trimmed.startsWith("#") || trimmed.startsWith("&")
- ? trimmed
- : `#${trimmed}`;
+ const trimmed = newChannelName.trim();
+ // Input path currently prefixes "#" on every keystroke; recover intended "&" names.
+ const normalized = trimmed.startsWith("#&") ? trimmed.slice(1) : trimmed;
+ const channelName =
+ normalized.startsWith("#") || normalized.startsWith("&")
+ ? normalized
+ : `#${normalized}`;
joinAndSelectChannel(selectedServerId, channelName);
setNewChannelName("");
}
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/ChannelList.tsx` around lines 376 - 385, The
add-channel flow mangles `&` prefixes because a leading `#` can be present
before `&` (e.g. `#&name`), so update handleAddChannel to normalize that case:
compute a cleaned string from newChannelName.trim() that strips a leading `#`
only when the next char is `&` (e.g. if trimmed.startsWith("#&") then cleaned =
trimmed.slice(1) else cleaned = trimmed), then build channelName by checking
cleaned.startsWith("#") or cleaned.startsWith("&") and otherwise prefixing "#" —
use the existing symbols newChannelName, trimmed, cleaned (new variable) and
handleAddChannel to implement this.
Comment on lines
+553
to
+656
| <button | ||
| type="button" | ||
| onClick={onClick} | ||
| className={`relative bg-discord-dark-300 rounded-lg ${aspect} ${large ? "w-full h-full" : ""} overflow-hidden flex items-center justify-center text-left ${ | ||
| showSpeakingRing | ||
| ? "ring-2 ring-discord-green" | ||
| : "ring-1 ring-discord-dark-200" | ||
| }`} | ||
| > | ||
| {member.videoTrack ? ( | ||
| <video | ||
| ref={videoRef} | ||
| className={`w-full h-full ${member.screenSharing ? "object-contain" : "object-cover"}`} | ||
| autoPlay | ||
| playsInline | ||
| muted={isSelf} | ||
| /> | ||
| ) : ( | ||
| <div | ||
| className={`${large ? "w-32 h-32 text-5xl" : "w-16 h-16 text-2xl"} rounded-full bg-discord-dark-100 flex items-center justify-center text-white font-bold`} | ||
| > | ||
| {member.nick[0]?.toUpperCase() ?? "?"} | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Hand-raised badge */} | ||
| {member.handRaised && ( | ||
| <div className="absolute top-1 left-1 px-1.5 py-1 rounded bg-yellow-500/90 text-white text-xs flex items-center gap-1"> | ||
| <FaHandPaper /> | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Floating emoji reactions */} | ||
| <div className="pointer-events-none absolute inset-0 overflow-hidden"> | ||
| {reactions.map((r, i) => ( | ||
| <span | ||
| key={r.id} | ||
| className="absolute text-3xl animate-react-float" | ||
| style={{ | ||
| left: `${30 + ((i * 17) % 40)}%`, | ||
| bottom: "20%", | ||
| }} | ||
| > | ||
| {r.emoji} | ||
| </span> | ||
| ))} | ||
| </div> | ||
|
|
||
| {/* Connection quality dot */} | ||
| {member.quality !== "unknown" && !isSelf && ( | ||
| <div | ||
| className="absolute top-1 right-1" | ||
| title={`Connection: ${member.quality}`} | ||
| > | ||
| <FaCircle | ||
| className={ | ||
| member.quality === "good" | ||
| ? "text-discord-green" | ||
| : member.quality === "ok" | ||
| ? "text-yellow-400" | ||
| : "text-discord-red" | ||
| } | ||
| style={{ fontSize: 8 }} | ||
| /> | ||
| </div> | ||
| )} | ||
|
|
||
| <div className="absolute bottom-1 left-1 right-1 px-2 py-0.5 rounded bg-black/50 text-white text-xs flex items-center gap-1.5"> | ||
| <span className="truncate flex-1">{member.nick}</span> | ||
| {!member.micOn && !isSelf && ( | ||
| <FaMicrophoneSlash className="text-discord-red flex-shrink-0" /> | ||
| )} | ||
| {isSelf && !selfMicOn && ( | ||
| <FaMicrophoneSlash className="text-discord-red flex-shrink-0" /> | ||
| )} | ||
| {member.deafened && ( | ||
| <FaVolumeMute className="text-discord-red flex-shrink-0" /> | ||
| )} | ||
| {!isSelf && ( | ||
| <input | ||
| type="range" | ||
| min={0} | ||
| max={1} | ||
| step={0.01} | ||
| value={member.volume} | ||
| onClick={(e) => e.stopPropagation()} | ||
| onChange={(e) => onSetVolume(member.nick, Number(e.target.value))} | ||
| className="w-20 h-1 accent-discord-primary flex-shrink-0 cursor-pointer" | ||
| title={`Volume: ${Math.round(member.volume * 100)}%`} | ||
| aria-label={`Volume for ${member.nick}`} | ||
| /> | ||
| )} | ||
| {!isSelf && ( | ||
| <button | ||
| type="button" | ||
| onClick={onMuteClick} | ||
| className="text-discord-text-muted hover:text-white flex-shrink-0" | ||
| title={muted ? "Unmute (local)" : "Mute for me only"} | ||
| > | ||
| {muted ? <FaVolumeMute /> : <FaVolumeUp />} | ||
| </button> | ||
| )} | ||
| </div> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Don't put the volume controls inside a tile <button>.
ParticipantTile renders a button that contains both a range input and another button. That interactive nesting is invalid HTML and usually breaks keyboard/focus behavior for the local mute and volume controls. Use a non-interactive wrapper for the tile and make the spotlight action its own button or overlay.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/VoiceChannelView.tsx` around lines 553 - 656, The outer
tile is currently a <button> containing interactive children (range input and
inner button) which is invalid; change the outer element used in
VoiceChannelView (the element with onClick and className that wraps the whole
tile) from a <button> to a non-interactive <div> (preserve the same className,
aspect, large logic and ring classes) and remove the onClick from that wrapper;
then add a dedicated interactive spotlight button that uses the existing onClick
handler (create a new <button> overlay or small control inside the tile, give it
aria-label/role and tabIndex for keyboard access) so volume <input> and mute
button are not nested inside another button; ensure existing handlers that call
onMuteClick, onSetVolume, videoRef, and accessibility props (title/aria-label)
remain unchanged and add stopPropagation where needed on inner controls to avoid
triggering the spotlight button.
Comment on lines
597
to
599
| {displayName || | ||
| getChannelDisplayName(channel.channel, {})} | ||
| channel.channel.replace(/^[#^$]/, "")} | ||
| </span> |
Contributor
There was a problem hiding this comment.
Channel display-name fallback misses & prefix stripping.
& channels now show raw prefix in this modal while other views strip it.
Proposed fix
- {displayName ||
- channel.channel.replace(/^[#^$]/, "")}
+ {displayName ||
+ channel.channel.replace(/^[#&^$]/, "")}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/ChannelListModal.tsx` around lines 597 - 599, The fallback
that shows channel.channel in ChannelListModal is not stripping the '&' prefix,
causing '&' channels to display the raw prefix; update the fallback logic (the
span that renders {displayName || channel.channel.replace(...)} ) to strip '&'
as well—e.g. include '&' in the character class used by the replace (or call the
shared channel-name sanitiser if one exists) so the regex removes leading '#',
'^', '$', and '&' before rendering.
Comment on lines
+317
to
+329
| const handler = (p: { | ||
| serverId: string; | ||
| sender: string; | ||
| channelName: string; | ||
| mtags?: Record<string, string>; | ||
| }) => { | ||
| if (p.serverId !== this.opts.serverId) return; | ||
| if (!p.mtags) return; | ||
| const raw = p.mtags[RTC_TAG]; | ||
| if (!raw) return; | ||
| try { | ||
| const env = JSON.parse(raw) as SignalEnvelope; | ||
| this.onSignal(p.sender, p.channelName, env); |
Contributor
There was a problem hiding this comment.
Scope RTC TAGMSG handling to the active channel.
The handler only filters by serverId, so every live VoiceClient on the same server will parse SDP/ICE for every other voice room. Since the registry keeps one client per (serverId, channel), joining a second room or leaving one running in the background will cross-wire signaling between rooms.
Suggested fix
const handler = (p: {
serverId: string;
sender: string;
channelName: string;
mtags?: Record<string, string>;
}) => {
if (p.serverId !== this.opts.serverId) return;
+ if (p.channelName !== this.opts.channel) return;
if (!p.mtags) return;
const raw = p.mtags[RTC_TAG];
if (!raw) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/voice.ts` around lines 317 - 329, The handler currently only filters
by serverId so signaling for all channels on the same server is processed;
restrict it to the active channel by adding a check that p.channelName matches
this instance's active channel identifier (e.g. this.channelName or
this.activeChannel) before parsing the RTC_TAG and calling this.onSignal; update
the handler function (the closure that receives p, checks this.opts.serverId,
reads RTC_TAG, parses SignalEnvelope and calls this.onSignal) to return early
unless p.channelName === this.<active channel property>.
Comment on lines
+357
to
+359
| setDeafened(on: boolean): void { | ||
| if (this.opts.audioSink) this.opts.audioSink.muted = on; | ||
| } |
Contributor
There was a problem hiding this comment.
Make deafen mute the audio that actually plays.
Playback is attached to the per-peer sinks in memberAudioSinks, but setDeafened only toggles the shared hidden sink. Existing peer audio keeps playing, and peers that arrive while already deafened will start audible too unless this state is applied to those sinks as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/voice.ts` around lines 357 - 359, The setDeafened method currently
only toggles this.opts.audioSink.muted; extend it to also mute/unmute every
per-peer sink in memberAudioSinks (e.g., iterate memberAudioSinks.values() and
set eachSink.muted = on) and store the deafened state on the instance
(this.deafened = on) so that code which adds new sinks (where memberAudioSinks
entries are created) can apply the muted state when attaching a sink for a
joining peer.
Comment on lines
+440
to
+445
| this.pc.addTrack(track, this.localStream); | ||
| selfMember.videoTrack = track; | ||
| selfMember.videoOn = true; | ||
| this.members[selfNick] = selfMember; | ||
| await this.renegotiate(); | ||
| } catch (err) { |
Contributor
There was a problem hiding this comment.
Remove RTCRtpSenders when a publish track is turned off.
Stopping a track and removing it from the MediaStream does not detach its sender from the RTCPeerConnection. After a few camera/screen-share toggles, this will leave dead senders/transceivers behind and renegotiation can advertise stale or duplicate outbound tracks. Track the senders returned by addTrack() and removeTrack() or replaceTrack(null) in the off path.
Also applies to: 450-458, 498-515, 529-538
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/voice.ts` around lines 440 - 445, The publish path currently calls
this.pc.addTrack(track, this.localStream) and sets selfMember.videoTrack but the
off/toggle path only stops the MediaStreamTrack and removes it from the
MediaStream, leaving RTCRtpSenders/transceivers attached; modify the
publish/unpublish logic to track the RTCRtpSender returned by this.pc.addTrack
(store it on selfMember e.g. selfMember.videoSender) and when turning a track
off remove that sender from the RTCPeerConnection (call
this.pc.removeTrack(sender)) or call sender.replaceTrack(null) and clear the
stored sender, also ensure equivalents for audio/screen-share paths and update
members[selfNick] before calling await this.renegotiate() so stale
senders/transceivers are not advertised.
ChatHeader at the top of the panel already shows the channel icon, name, and topic. The voice grid was rendering a second header below it with the same name + a settings cog (already reachable from ChatHeader), so every ^/$ channel had two stacked headers. Replace it with a thin status strip that only shows up when there's something worth saying: the joining/failed banner, the streamer/viewer role + count for $-rooms, or the "Exit spotlight" button while focused. When none apply the strip doesn't render and the grid runs flush up to the channel header.
Previously the layout was [VoiceChannelView][ChatArea] vertically, with ChatArea rendering its own ChatHeader at the top of its slot — so the real channel header was sandwiched between the voice grid and the chat scroll instead of sitting at the very top of the panel. Make ChatArea accept a `topSlot` prop and render it between ChatHeader and TopicMediaStrip. For ^/$ channels, AppLayout drops the manual flex split and just passes <VoiceChannelView/> as the topSlot. Result: one ChatHeader at the top, voice grid in the middle (~55% of the panel), chat messages + input below.
The grid was `grid-cols-2 sm:grid-cols-3 lg:grid-cols-4` with tiles locked to `aspect-video`. With 1-3 participants that produced a row of small tiles at the top of `<main>` and a huge empty gap above the controls footer. Pick column count from participant count (1→1col, 2-4→2, 5-9→3, 10+→4), derive row count, and grid-template both axes as `1fr` so tiles fill the available area. Tiles drop the fixed aspect-video and stretch to their cell; spotlight side-thumbnails get an explicit `h-24` because they're outside the grid context.
Previous attempt enlarged the tiles to fill a 55%-tall slot, which is the opposite of what was wanted. The voice grid should sit at its natural size at the top with the controls footer flush beneath, and all the remaining panel height belongs to the chat scroll. - VoiceChannelView outer drops `h-full`; <main> drops `flex-1 min-h-0` and overflow-hidden, so the component is exactly as tall as its grid + footer. - Grid reverts to the fixed `grid-cols-2 sm:grid-cols-3 lg:grid-cols-4` with aspect-video tiles. With few participants you get a single short row, not a wall of inflated tiles. - ChatArea's topSlot wrapper drops the fixed `h-[55%]` and uses `max-h-[60%]` instead, so a huge participant list still has a ceiling but the wrapper shrinks to fit when there are only a few. - Chat messages area picks up everything else via its existing flex-grow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/components/layout/VoiceChannelView.tsx (2)
123-137:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftOptimistic
videoOn/screenOnflags can desync from the real publish state.Same concern as the prior review pass:
setVideo()/setScreenShare()are async and can fail (permission denied, device busy, user cancels the screen-picker), and a screen share can end on its own when the user clicks the browser's "Stop sharing" pill. In those cases the local boolean staystrueand the footer button reflects the wrong state. Derive these from the connected self member (e.g. presence of a video/screen publication on the self member) or only commit the local state after the client confirms (and revert on rejection).Worth raising again because the fix interacts with the new spotlight/streamer logic —
largetiles andisHostrely on truthful publish state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/VoiceChannelView.tsx` around lines 123 - 137, The optimistic updates in onToggleVideo and onToggleScreen can desync from actual publish state; change them to await the async clientRef.current?.setVideo(...) and setScreenShare(...) calls (or subscribe to the connected self member's publications) and only call setVideoOn/setScreenOn after the operation succeeds, and on rejection revert/update the boolean accordingly; alternatively derive the videoOn/screenOn values from the connected self member's publications (presence of a video/screen track) instead of local toggles so large/isHost/spotlight logic reads the authoritative publish state (update onToggleVideo, onToggleScreen and any code that reads videoOn/screenOn to use the member-derived value or confirmation-based updates).
548-651:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalid HTML: interactive controls nested inside the tile
<button>.The wrapper is still a
<button>, and it contains a<input type="range">plus another<button>(per-peer mute). Buttons cannot contain interactive descendants — this breaks screen-reader semantics and keyboard focus management (Tab into the slider may get unexpected results, Space on the slider can also click the tile), and React 19's stricter HTML validation will warn in dev. Replace the outer<button>with a<div role="button">+ key handler (or, better, make the spotlight target a dedicated overlay button covering the non-control area of the tile), andstopPropagationis already in place on the slider/mute.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/VoiceChannelView.tsx` around lines 548 - 651, The tile currently renders an outer <button> containing interactive descendants (the volume <input> and per-peer mute <button>), which is invalid; change the outer element in VoiceChannelView from a <button> to a non-interactive container with button semantics (e.g., <div role="button">) while keeping the existing onClick handler logic (onClick) and visual classes, add keyboard support by implementing onKeyDown to trigger the same click behavior for Enter/Space, preserve aria attributes (aria-label/aria-pressed as appropriate) and focusability (tabIndex=0) so the tile remains keyboard-accessible, and keep the existing stopPropagation on the range and the onMuteClick handler for the inner button so those interactive controls work independently.
🧹 Nitpick comments (8)
src/components/layout/VoiceChannelView.tsx (7)
175-190: ⚡ Quick win
Object.values(state.members)order is not stable — grid tiles will reorder on every state update.When
state.membersis keyed by nick,Object.valuesiteration order in JS is insertion order, so any time the SFU re-broadcasts the member set in a different order (rejoins, reconnect, role flip), tile positions in the grid jump. Sort by something stable (joinedAt, or nick alphabetically) before slicing into streamer/viewer arrays.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/VoiceChannelView.tsx` around lines 175 - 190, The members array is built from Object.values(state.members) which yields non-stable insertion order; before using it to derive streamerMembers and viewerMembers, sort members by a stable key (e.g., m.joinedAt or m.nick) to ensure deterministic ordering. Modify the code that defines members (and subsequently streamerMembers and viewerMembers) so you map Object.values(state.members) to a sorted array (use compare on joinedAt if present, fallback to nick) and then apply the existing filter logic in streamerMembers and viewerMembers to avoid grid tile reordering on state updates.
159-165: 💤 Low value
onLeavedoesn't fall back gracefully if there's no#text channel to select.After leaving, the code looks for the first
#text channel and navigates there if found — but if the server has only^/$channels (or none yet), the user is left on the now-released voice view, which will then operate on a nullclientRef. Either fall back to server-notices / a blank state, or clear the per-server selection explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/VoiceChannelView.tsx` around lines 159 - 165, The onLeave handler can leave the UI pointing at a now-invalid voice view when no "#" text channel exists; update the onLeave function (which calls releaseVoiceClient and sets clientRef.current = null) to also clear or reset the per-server channel selection: after finding srv via useStore.getState().servers.find and attempting to get firstText, if firstText is undefined call selectChannel with a safe fallback (e.g., the server-notices channel id or null/empty-id) or explicitly clear the selection state so the UI shows a server-notices/blank state instead of continuing to reference the released clientRef; ensure the handler uses the same serverId and selectChannel API so the change is localized to onLeave.
345-355: 💤 Low valueInvite button is rendered inside a
<span>— that's fine, but the parent span is keyed onv.nick; consider disabling instead of conditionally rendering.When
streamerNicks.length === streamMaxStreamers, the invite button disappears entirely, which can feel like a UI flicker as people join/leave the stream. A disabled-with-tooltip variant ("Stream is full") communicates the constraint more clearly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/VoiceChannelView.tsx` around lines 345 - 355, The Invite button currently is conditionally rendered based on streamerNicks.length < streamMaxStreamers causing it to disappear; instead always render the button (so the parent span keyed by v.nick stays stable) but set its disabled state when streamerNicks.length >= streamMaxStreamers, change the onClick to no-op when disabled (or guard against calling clientRef.current?.promote), and update the title/tooltip to show "Stream is full" when disabled; modify the element using the same conditional check around the disabled prop and add a disabled CSS style (and aria-disabled) while leaving references like isHost, streamerNicks, streamMaxStreamers, clientRef.current?.promote and v.nick intact.
526-536: 💤 Low valueStale
MediaStreamwhenvideoTrackbecomes null, plus consider releasing prior streams.Minor concerns:
- On every truthy
member.videoTrackchange you allocate a newMediaStream. The previous stream is dropped by replacingsrcObject, but if you ever attach additional tracks later, holding a reference would be cleaner. Not a leak in practice for video-only.videoRef.current.play().catch(() => {})silently swallows all play failures, including ones that aren't autoplay-policy rejections. At minimum, log at debug level so audio/video stalls are diagnosable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/VoiceChannelView.tsx` around lines 526 - 536, The useEffect handling video refs can leave the previous MediaStream's tracks active and silently swallow play errors; update the effect for videoRef/member.videoTrack so it retains a reference to the current MediaStream (e.g., a local ref or let currentStream) and when replacing or clearing srcObject stop all tracks on the old stream before discarding it; when member.videoTrack is truthy create a new MediaStream from it, assign to videoRef.current.srcObject, and call play().catch(err => console.debug('video play failed', err)) (or your logger) instead of an empty catch; also ensure when member.videoTrack becomes null you set srcObject = null and stop any remaining tracks on the stored stream to fully release media resources.
615-650: ⚖️ Poor tradeoffVolume slider doesn't persist across the session — TODO in the PR description matches.
The PR explicitly notes "per-peer volume persistence for the session remains TODO." Worth wiring a per-(serverId,channelName,nick) value to localStorage (or the existing zustand store) before merge so users don't have to re-set every peer's volume on every rejoin. Happy to draft the persistence layer if useful.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/VoiceChannelView.tsx` around lines 615 - 650, Persist per-peer volume by wiring volume changes in VoiceChannelView to a session-scoped storage key and restoring it on mount: derive a stable key from (serverId, channelName, member.nick) and onSetVolume (the handler used in the range input) save Number(e.target.value) to localStorage (or the existing zustand store) under that key; on component initialization/read of member.volume, load the stored value (parseFloat, clamp 0–1) and fall back to the original member.volume if none exists; ensure the range input's value uses the loaded/stored value and keep the existing onClick stopPropagation behavior.
100-121: ⚡ Quick winPTT swallows Space globally; consider scoping and the modifier-key edge cases.
A couple of concerns with the global listeners:
e.preventDefault()on keydown for Space, when focus is on an unrelated<button>or<a>(i.e., not INPUT/TEXTAREA), prevents the default activation of that control. That's likely surprising for users who enabled PTT and happen to be tabbing through UI. Consider also bailing out whendocument.activeElementis an interactive element (button,a,[role="button"], contenteditable, etc.).- The keyup handler unconditionally calls
setMic(false)for any Space release, even one that never had a corresponding PTT-handled keydown (e.g., focus was on a textarea during keydown, then user clicked away before releasing). Tracking a "PTT key currently held by us" boolean would avoid spurious remutes.- If the window loses focus while Space is held, the keyup never fires and the mic remains open. A
blurlistener that re-mutes would close that gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/VoiceChannelView.tsx` around lines 100 - 121, The current PTT useEffect installs global onDown/onUp handlers that can unexpectedly prevent default Space behavior and leave the mic stuck; update the handlers to 1) in onDown (inside the useEffect) additionally bail out if document.activeElement is an interactive element (button, a, [role="button"], contentEditable, selectable inputs) instead of only checking tagName, and only call e.preventDefault() after that check; 2) add a ref flag (e.g., pttHeldRef) that you set true in onDown when you actually activated PTT and setMic(true)/setMicOn(true), and in onUp only proceed to setMic(false)/setMicOn(false) if pttHeldRef is true (then clear it); and 3) add a window "blur" listener in the same useEffect to clear pttHeldRef and call clientRef.current?.setMic(false) and setMicOn(false) so losing focus won't leave the mic open; keep cleanup for all listeners in the return.
597-613: 💤 Low valueQuality dot exposes the connection state via color only.
good/ok/poor are conveyed solely by color (text-discord-green/text-yellow-400/text-discord-red). Thetitleattribute helps mouse users but not touch users or screen-reader users in many scenarios. Consider addingaria-label(already partially covered viatitleon the wrapper, butFaCirclehas no accessible name on its own) and a non-color indicator (e.g., bar count, or text fallback forquality === "poor").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/VoiceChannelView.tsx` around lines 597 - 613, The quality dot currently conveys connection state only via color (member.quality) using the FaCircle icon, which is inaccessible to touch and screen-reader users; update the JSX around FaCircle in VoiceChannelView.tsx to provide an accessible name and non-color cue: add an aria-label (e.g., `aria-label={`Connection: ${member.quality}`}`) or role="img" on the FaCircle and include a visually-hidden text node (or an extra small indicator like bar count or distinct icon class) that spells out "good/ok/poor" for screen readers and touch users, and keep the wrapper title for mouse users. Ensure the visual change preserves current styling while the hidden text or alternative shape provides the non-color indicator.src/components/layout/ChatArea.tsx (1)
2015-2019: 💤 Low valueConsider making the voice grid height responsive/configurable.
The hardcoded
h-[55%]allocates exactly 55% of the panel to the voice grid for every viewport and channel size. On very tall windows this leaves a lot of empty space in the grid, and on short windows it can squeeze the chat scroll area below uncomfortably. Worth considering aflex-1split with a minimum or a user-resizable splitter, but as a follow-up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/ChatArea.tsx` around lines 2015 - 2019, The hardcoded height on the topSlot wrapper (h-[55%]) makes the voice grid non-responsive; update the ChatArea component to replace that fixed class with a responsive layout (e.g., use flex-1 on the topSlot container plus a sensible min-height like min-h-[160px] or expose a configurable prop such as topSlotHeight/topSlotMinHeight) so the top panel grows/shrinks naturally and doesn't overly squeeze the chat area — modify the div that renders {topSlot} (currently using h-[55%]) to use the new flex-based classes or prop-driven classNames and ensure the parent layout remains a column flex container so the split works as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/layout/VoiceChannelView.tsx`:
- Around line 65-71: pushReaction schedules a 2400ms timeout that calls
setReactions and can fire after the component unmounts; fix by tracking timeout
IDs in a ref and clearing them on unmount (or using an isMountedRef to guard
setReactions). Update pushReaction to save the timer ID returned by setTimeout
into a timersRef (e.g. timersRef.current.push(timerId)) and optionally remove
that ID when the timeout runs, and add a useEffect cleanup that iterates
timersRef.current and calls clearTimeout on each to prevent setState on an
unmounted component; keep existing reactionIdRef and setReactions logic
otherwise.
- Around line 73-77: selfNick is computed via useMemo calling
ircClient.getCurrentUser(serverId) which stays stale on NICK events; replace
that memo with a reactive subscription to the app store's current user using
useStore (subscribe to the store slice that contains the current user or
currentUsers[serverId]) and derive username (user?.username || "you") so nick
updates trigger re-renders; update any uses (acquireVoiceClient, the isHost
check using streamerNicks[0] === selfNick, and ParticipantTile isSelf logic) to
read the new reactive selfNick so host and self checks change immediately when
the store emits NICK changes.
---
Duplicate comments:
In `@src/components/layout/VoiceChannelView.tsx`:
- Around line 123-137: The optimistic updates in onToggleVideo and
onToggleScreen can desync from actual publish state; change them to await the
async clientRef.current?.setVideo(...) and setScreenShare(...) calls (or
subscribe to the connected self member's publications) and only call
setVideoOn/setScreenOn after the operation succeeds, and on rejection
revert/update the boolean accordingly; alternatively derive the videoOn/screenOn
values from the connected self member's publications (presence of a video/screen
track) instead of local toggles so large/isHost/spotlight logic reads the
authoritative publish state (update onToggleVideo, onToggleScreen and any code
that reads videoOn/screenOn to use the member-derived value or
confirmation-based updates).
- Around line 548-651: The tile currently renders an outer <button> containing
interactive descendants (the volume <input> and per-peer mute <button>), which
is invalid; change the outer element in VoiceChannelView from a <button> to a
non-interactive container with button semantics (e.g., <div role="button">)
while keeping the existing onClick handler logic (onClick) and visual classes,
add keyboard support by implementing onKeyDown to trigger the same click
behavior for Enter/Space, preserve aria attributes (aria-label/aria-pressed as
appropriate) and focusability (tabIndex=0) so the tile remains
keyboard-accessible, and keep the existing stopPropagation on the range and the
onMuteClick handler for the inner button so those interactive controls work
independently.
---
Nitpick comments:
In `@src/components/layout/ChatArea.tsx`:
- Around line 2015-2019: The hardcoded height on the topSlot wrapper (h-[55%])
makes the voice grid non-responsive; update the ChatArea component to replace
that fixed class with a responsive layout (e.g., use flex-1 on the topSlot
container plus a sensible min-height like min-h-[160px] or expose a configurable
prop such as topSlotHeight/topSlotMinHeight) so the top panel grows/shrinks
naturally and doesn't overly squeeze the chat area — modify the div that renders
{topSlot} (currently using h-[55%]) to use the new flex-based classes or
prop-driven classNames and ensure the parent layout remains a column flex
container so the split works as intended.
In `@src/components/layout/VoiceChannelView.tsx`:
- Around line 175-190: The members array is built from
Object.values(state.members) which yields non-stable insertion order; before
using it to derive streamerMembers and viewerMembers, sort members by a stable
key (e.g., m.joinedAt or m.nick) to ensure deterministic ordering. Modify the
code that defines members (and subsequently streamerMembers and viewerMembers)
so you map Object.values(state.members) to a sorted array (use compare on
joinedAt if present, fallback to nick) and then apply the existing filter logic
in streamerMembers and viewerMembers to avoid grid tile reordering on state
updates.
- Around line 159-165: The onLeave handler can leave the UI pointing at a
now-invalid voice view when no "#" text channel exists; update the onLeave
function (which calls releaseVoiceClient and sets clientRef.current = null) to
also clear or reset the per-server channel selection: after finding srv via
useStore.getState().servers.find and attempting to get firstText, if firstText
is undefined call selectChannel with a safe fallback (e.g., the server-notices
channel id or null/empty-id) or explicitly clear the selection state so the UI
shows a server-notices/blank state instead of continuing to reference the
released clientRef; ensure the handler uses the same serverId and selectChannel
API so the change is localized to onLeave.
- Around line 345-355: The Invite button currently is conditionally rendered
based on streamerNicks.length < streamMaxStreamers causing it to disappear;
instead always render the button (so the parent span keyed by v.nick stays
stable) but set its disabled state when streamerNicks.length >=
streamMaxStreamers, change the onClick to no-op when disabled (or guard against
calling clientRef.current?.promote), and update the title/tooltip to show
"Stream is full" when disabled; modify the element using the same conditional
check around the disabled prop and add a disabled CSS style (and aria-disabled)
while leaving references like isHost, streamerNicks, streamMaxStreamers,
clientRef.current?.promote and v.nick intact.
- Around line 526-536: The useEffect handling video refs can leave the previous
MediaStream's tracks active and silently swallow play errors; update the effect
for videoRef/member.videoTrack so it retains a reference to the current
MediaStream (e.g., a local ref or let currentStream) and when replacing or
clearing srcObject stop all tracks on the old stream before discarding it; when
member.videoTrack is truthy create a new MediaStream from it, assign to
videoRef.current.srcObject, and call play().catch(err => console.debug('video
play failed', err)) (or your logger) instead of an empty catch; also ensure when
member.videoTrack becomes null you set srcObject = null and stop any remaining
tracks on the stored stream to fully release media resources.
- Around line 615-650: Persist per-peer volume by wiring volume changes in
VoiceChannelView to a session-scoped storage key and restoring it on mount:
derive a stable key from (serverId, channelName, member.nick) and onSetVolume
(the handler used in the range input) save Number(e.target.value) to
localStorage (or the existing zustand store) under that key; on component
initialization/read of member.volume, load the stored value (parseFloat, clamp
0–1) and fall back to the original member.volume if none exists; ensure the
range input's value uses the loaded/stored value and keep the existing onClick
stopPropagation behavior.
- Around line 100-121: The current PTT useEffect installs global onDown/onUp
handlers that can unexpectedly prevent default Space behavior and leave the mic
stuck; update the handlers to 1) in onDown (inside the useEffect) additionally
bail out if document.activeElement is an interactive element (button, a,
[role="button"], contentEditable, selectable inputs) instead of only checking
tagName, and only call e.preventDefault() after that check; 2) add a ref flag
(e.g., pttHeldRef) that you set true in onDown when you actually activated PTT
and setMic(true)/setMicOn(true), and in onUp only proceed to
setMic(false)/setMicOn(false) if pttHeldRef is true (then clear it); and 3) add
a window "blur" listener in the same useEffect to clear pttHeldRef and call
clientRef.current?.setMic(false) and setMicOn(false) so losing focus won't leave
the mic open; keep cleanup for all listeners in the return.
- Around line 597-613: The quality dot currently conveys connection state only
via color (member.quality) using the FaCircle icon, which is inaccessible to
touch and screen-reader users; update the JSX around FaCircle in
VoiceChannelView.tsx to provide an accessible name and non-color cue: add an
aria-label (e.g., `aria-label={`Connection: ${member.quality}`}`) or role="img"
on the FaCircle and include a visually-hidden text node (or an extra small
indicator like bar count or distinct icon class) that spells out "good/ok/poor"
for screen readers and touch users, and keep the wrapper title for mouse users.
Ensure the visual change preserves current styling while the hidden text or
alternative shape provides the non-color indicator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b16018e0-03ff-4df7-b7df-7f7100851b10
📒 Files selected for processing (3)
src/components/layout/AppLayout.tsxsrc/components/layout/ChatArea.tsxsrc/components/layout/VoiceChannelView.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/layout/AppLayout.tsx
Comment on lines
+65
to
+71
| const pushReaction = useCallback((nick: string, emoji: string) => { | ||
| const id = ++reactionIdRef.current; | ||
| setReactions((rs) => [...rs, { id, nick, emoji }]); | ||
| setTimeout(() => { | ||
| setReactions((rs) => rs.filter((r) => r.id !== id)); | ||
| }, 2400); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
setTimeout callback runs after unmount and will set state on an unmounted component.
pushReaction schedules a 2.4s timer that calls setReactions unconditionally. If the user leaves the voice channel (component unmounts) while reactions are still in-flight, those timers will fire after unmount and trigger setState-on-unmounted warnings (and a small leak of timer handles). Track the timeout IDs in a ref and clear them on unmount, or guard with an isMountedRef.
♻️ Suggested fix
+ const reactionTimersRef = useRef<Set<ReturnType<typeof setTimeout>>>(new Set());
+
const pushReaction = useCallback((nick: string, emoji: string) => {
const id = ++reactionIdRef.current;
setReactions((rs) => [...rs, { id, nick, emoji }]);
- setTimeout(() => {
+ const t = setTimeout(() => {
+ reactionTimersRef.current.delete(t);
setReactions((rs) => rs.filter((r) => r.id !== id));
}, 2400);
+ reactionTimersRef.current.add(t);
}, []);
+
+ useEffect(() => {
+ const timers = reactionTimersRef.current;
+ return () => {
+ timers.forEach((t) => clearTimeout(t));
+ timers.clear();
+ };
+ }, []);📝 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.
Suggested change
| const pushReaction = useCallback((nick: string, emoji: string) => { | |
| const id = ++reactionIdRef.current; | |
| setReactions((rs) => [...rs, { id, nick, emoji }]); | |
| setTimeout(() => { | |
| setReactions((rs) => rs.filter((r) => r.id !== id)); | |
| }, 2400); | |
| }, []); | |
| const reactionTimersRef = useRef<Set<ReturnType<typeof setTimeout>>>(new Set()); | |
| const pushReaction = useCallback((nick: string, emoji: string) => { | |
| const id = ++reactionIdRef.current; | |
| setReactions((rs) => [...rs, { id, nick, emoji }]); | |
| const t = setTimeout(() => { | |
| reactionTimersRef.current.delete(t); | |
| setReactions((rs) => rs.filter((r) => r.id !== id)); | |
| }, 2400); | |
| reactionTimersRef.current.add(t); | |
| }, []); | |
| useEffect(() => { | |
| const timers = reactionTimersRef.current; | |
| return () => { | |
| timers.forEach((t) => clearTimeout(t)); | |
| timers.clear(); | |
| }; | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/VoiceChannelView.tsx` around lines 65 - 71,
pushReaction schedules a 2400ms timeout that calls setReactions and can fire
after the component unmounts; fix by tracking timeout IDs in a ref and clearing
them on unmount (or using an isMountedRef to guard setReactions). Update
pushReaction to save the timer ID returned by setTimeout into a timersRef (e.g.
timersRef.current.push(timerId)) and optionally remove that ID when the timeout
runs, and add a useEffect cleanup that iterates timersRef.current and calls
clearTimeout on each to prevent setState on an unmounted component; keep
existing reactionIdRef and setReactions logic otherwise.
Comment on lines
+73
to
+77
| // Self nick from the IRC client. | ||
| const selfNick = useMemo(() => { | ||
| const u = ircClient.getCurrentUser(serverId); | ||
| return u?.username || "you"; | ||
| }, [serverId]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the store's "current user" selector and confirm it's reactive on nick changes,
# and find how ircClient surfaces NICK changes.
rg -nP --type=ts -C3 '\bgetCurrentUser\s*\('
rg -nP --type=ts -C3 '"NICK"|case\s+"NICK"|\bonNick\b'Repository: ObsidianIRC/ObsidianIRC
Length of output: 25111
🏁 Script executed:
# Check the NICK handler to see if it updates ircClient.currentUsers
sed -n '218,300p' src/store/handlers/users.ts
# Check how ircClient maintains currentUsers
sed -n '1710,1730p' src/lib/irc/IRCClient.ts
# Look for any IRC event handlers that update currentUsers
rg -n 'currentUsers\.set|currentUsers\.delete' src/lib/irc/Repository: ObsidianIRC/ObsidianIRC
Length of output: 3857
🏁 Script executed:
# Check if NICK handler calls any ircClient methods
grep -n "ircClient\." src/store/handlers/users.ts | grep -A5 -B5 "NICK"
# Look at full NICK handler and surrounding code
sed -n '215,350p' src/store/handlers/users.ts | head -100
# Check VoiceChannelView for selfNick usage
sed -n '1,150p' src/components/layout/VoiceChannelView.tsx | grep -A10 -B10 'selfNick'Repository: ObsidianIRC/ObsidianIRC
Length of output: 6143
🏁 Script executed:
# Check where currentUsers is set in the codebase
rg -n 'currentUsers\.set' src/
# Check if NICK handler or any event handler updates ircClient state
sed -n '1,50p' src/lib/irc/IRCClient.ts | grep -E "currentUsers|class IRCClient"
# Verify if ircClient has any method to update user nick
rg -n 'updateNick|setNick|updateCurrentUser' src/lib/irc/Repository: ObsidianIRC/ObsidianIRC
Length of output: 284
🏁 Script executed:
# Verify that store.currentUser is actually being updated on NICK
# and that components can subscribe to it
rg -n 'useStore.*currentUser' src/components/ | head -20
# Check how many components use ircClient.getCurrentUser vs store
echo "=== Using ircClient.getCurrentUser ==="
rg 'ircClient\.getCurrentUser' src/components/ | wc -l
echo "=== Using store currentUser ==="
rg 'useStore.*currentUser' src/components/ | wc -lRepository: ObsidianIRC/ObsidianIRC
Length of output: 252
selfNick won't refresh after a NICK change.
The memo only re-evaluates when serverId changes, but the user can change their nick mid-call via /nick. After that, selfNick stays stale. The IRC client's currentUsers is not updated when NICK events fire—only the store is—so reading from ircClient.getCurrentUser() in a memo returns a stale value. This stale nick then feeds into the acquireVoiceClient call, the isHost comparison (streamerNicks[0] === selfNick), and every isSelf check in ParticipantTile.
Use useStore to subscribe to the current user from the store (which is reactive on nick changes) instead of reading from the stale ircClient inside a memo.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/VoiceChannelView.tsx` around lines 73 - 77, selfNick is
computed via useMemo calling ircClient.getCurrentUser(serverId) which stays
stale on NICK events; replace that memo with a reactive subscription to the app
store's current user using useStore (subscribe to the store slice that contains
the current user or currentUsers[serverId]) and derive username (user?.username
|| "you") so nick updates trigger re-renders; update any uses
(acquireVoiceClient, the isHost check using streamerNicks[0] === selfNick, and
ParticipantTile isSelf logic) to read the new reactive selfNick so host and self
checks change immediately when the store emits NICK changes.
Comment on lines
+82
to
+96
| useEffect(() => { | ||
| const client = acquireVoiceClient({ | ||
| serverId, | ||
| channel: channelName, | ||
| selfNick, | ||
| onState: (s) => setState(s), | ||
| onError: (e) => setError(e), | ||
| onReaction: (nick, emoji) => pushReaction(nick, emoji), | ||
| }); | ||
| clientRef.current = client; | ||
| setState(client.getState()); | ||
| return () => { | ||
| clientRef.current = null; | ||
| }; | ||
| }, [serverId, channelName, selfNick]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how acquireVoiceClient/releaseVoiceClient are reference-counted and
# whether any higher-level component (AppLayout etc.) tears the client down.
rg -nP --type=ts -C5 '\bacquireVoiceClient\b|\breleaseVoiceClient\b'
rg -nP --type=ts -C3 'VoiceClient' src/lib 2>/dev/null || rg -nP --type=ts -C3 'VoiceClient'Repository: ObsidianIRC/ObsidianIRC
Length of output: 9335
🏁 Script executed:
rg -nP --type=ts 'shutdownAllVoiceClients'Repository: ObsidianIRC/ObsidianIRC
Length of output: 139
🏁 Script executed:
# Check for any "in call" or "voice" related state/indicators in the UI
rg -nP --type=tsx 'inCall|inVoice|activeCall|voiceActive' src/componentsRepository: ObsidianIRC/ObsidianIRC
Length of output: 95
🏁 Script executed:
# Look at AppLayout to see if/how it manages voice client lifecycle
fd -type f 'AppLayout\|App\.tsx' src/components src/pagesRepository: ObsidianIRC/ObsidianIRC
Length of output: 238
🏁 Script executed:
rg 'shutdownAllVoiceClients' --type tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 134
🏁 Script executed:
# Find AppLayout and app setup files
fd 'AppLayout|App\.tsx|main\.tsx|index\.tsx' srcRepository: ObsidianIRC/ObsidianIRC
Length of output: 128
🏁 Script executed:
# Search for "in call" or voice-related state/UI indicators
rg 'inCall|voiceActive|activeCall' src --type tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
# Check ircClient for connection drop/disconnect handlers
rg 'disconnect|drop|logout' src/lib/ircClient.ts --type ts -A2 -B2Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
# Examine App.tsx for setup and connection lifecycle
cat -n src/App.tsx | head -100Repository: ObsidianIRC/ObsidianIRC
Length of output: 4119
🏁 Script executed:
# Check AppLayout for voice state visibility
cat -n src/components/layout/AppLayout.tsx | head -150Repository: ObsidianIRC/ObsidianIRC
Length of output: 6694
🏁 Script executed:
# Check if shutdownAllVoiceClients is imported or referenced anywhere
rg 'shutdownAllVoiceClients' srcRepository: ObsidianIRC/ObsidianIRC
Length of output: 134
🏁 Script executed:
# Check useConnectionResilience hook for disconnect handling
cat -n src/hooks/useConnectionResilience.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 2158
🏁 Script executed:
# Check for unload/beforeunload handlers and connection drop handlers
rg 'beforeunload|unload|disconnect.*voice|connection.*drop' src --type ts -B2 -A2Repository: ObsidianIRC/ObsidianIRC
Length of output: 681
🏁 Script executed:
# Check store for voice-related state (in-call indicator)
rg 'voice|inCall' src/store --type tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
# Final comprehensive search - any import or reference to shutdownAllVoiceClients
rg 'shutdownAllVoiceClients' src -lRepository: ObsidianIRC/ObsidianIRC
Length of output: 84
🏁 Script executed:
# Check ircClient for disconnect/error events that might cleanup voice
grep -n 'disconnect\|error\|close' src/lib/ircClient.ts | head -30Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
# Check if there are any handlers for IRC connection drops in App.tsx continuation
tail -100 src/App.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 3310
Voice client stays alive indefinitely when component unmounts without explicit "Leave" button.
The design intentionally keeps clients at module scope to avoid server join-throttle on channel navigation, but the cleanup mechanism is incomplete. The shutdownAllVoiceClients() function exists for this purpose but is never called when the IRC connection drops or the app logs out—only when the user clicks "Leave". If the user navigates away, refreshes, or the app crashes before hanging up, the WebRTC session (mic/camera capture, peer connections) remains active in the background.
Either wire up shutdownAllVoiceClient(serverId, channelName) calls when IRC disconnects / on logout (or app unload), and add a visible "in call" indicator outside VoiceChannelView so the session isn't silently abandoned. Or move lifecycle ownership higher (app-level store) with explicit teardown on server disconnect.
| args: | ||
| VITE_DEFAULT_IRC_SERVER: wss://${IRC_FQDN} | ||
| VITE_DEFAULT_IRC_SERVER_NAME: ${NETWORK_NAME} | ||
| image: obsidianirc:local |
Contributor
There was a problem hiding this comment.
this imagdoesn't exist publicy @ValwareIRC
DONT TOUCH THIS IN THIS PR
Comment on lines
+28
to
+49
| signingConfigs { | ||
| create("release") { | ||
| val keystorePropsFile = rootProject.file("keystore.properties") | ||
| if (keystorePropsFile.exists()) { | ||
| val keystoreProps = Properties().apply { | ||
| keystorePropsFile.inputStream().use { load(it) } | ||
| } | ||
| storeFile = file(keystoreProps.getProperty("storeFile")) | ||
| storePassword = keystoreProps.getProperty("storePassword") | ||
| keyAlias = keystoreProps.getProperty("keyAlias") | ||
| keyPassword = keystoreProps.getProperty("keyPassword") | ||
| } else { | ||
| val debugKeystore = File(System.getProperty("user.home"), ".android/debug.keystore") | ||
| if (debugKeystore.exists()) { | ||
| storeFile = debugKeystore | ||
| storePassword = "android" | ||
| keyAlias = "androiddebugkey" | ||
| keyPassword = "android" | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Hmm I think this might break CI builds
# Conflicts: # src/lib/irc/IRCClient.ts # src/store/index.ts
# Conflicts: # src/lib/irc/IRCClient.ts
# Conflicts: # src/lib/irc/IRCClient.ts # src/lib/ircUtils.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
^(voice) and$(stream) channels backed by the obbyircd voice-channels module bridged to a hosted-backend SFU^and$channels now render the regular textChatAreabelow the voice grid so users can chat while on a call (server-side already accepts PRIVMSG on these channels — only SDP-bearing TAGMSG is intercepted)Test plan
^channel — voice grid shows on top, text chat shows below, can send PRIVMSG and see history$stream channel — streamer/viewer split renders, chat below worksobsidianirc/voiceis REQ'd before any signaling fires; clients without the cap stay out of the bridgeSummary by CodeRabbit
New Features
Improvements
Infrastructure