fix(join): fire CHATHISTORY/WHO on server-initiated own JOIN#249
Merged
Conversation
PR #248 stopped the CHATHISTORY-FAIL-into-permanently-leaked needsWhoRequest leak when joinChannel() was the entry point. But joinChannel is only one of the two entry points. The other one -- server-initiated auto-join (UnrealIRCd's persistence module, sajoin, operblock perform-on-connect, InspIRCd auto-join) -- sends a JOIN that the client never asked for. The handler at users.ts:115-147 created the channel entry with needsWhoRequest=true and stopped there, never firing CHATHISTORY or WHO. Nothing downstream cleared the flag, so the nicklist stayed empty until something else triggered it (e.g. a manual /who). Why Ergo never showed this even though it auto-joins too: Ergo's auto-join sends a draft/chathistory BATCH alongside the JOIN, so the batch close fires CHATHISTORY_LOADING(false) -> messages.ts:2028 sees needsWhoRequest=true -> WHO goes out. UnrealIRCd and InspIRCd don't send a batch on auto-join, so nothing kicks the chain. Mirror joinChannel's tail in the own-join + new-channel branch: - draft/chathistory cap present: send `CHATHISTORY LATEST #chan * 50`, trigger CHATHISTORY_LOADING(true), leave needsWhoRequest=true so the existing batch-close -> WHO path runs. - cap absent (InspIRCd networks that don't ship the module): send `WHO #chan %cuhnfaro` immediately and clear needsWhoRequest. ircClient I/O runs OUTSIDE the setState callback -- triggerEvent inside setState causes nested-setState clobber (same reason batches.ts:215-218 captures chathistoryChannelName before firing). Tests cover all three branches: cap present, cap absent, and the joinChannel-already-ran path (must not double-send).
Contributor
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. Warning Review limit reached
More reviews will be available in 18 minutes and 50 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates the server-initiated own-JOIN event handler to detect the ChangesServer-initiated own-JOIN with capability-driven history loading
Sequence DiagramsequenceDiagram
participant Server
participant JoinHandler
participant Store
participant IrcClient
Server->>JoinHandler: JOIN event for own nick (isOurJoin)
JoinHandler->>JoinHandler: Check server capabilities for draft/chathistory
JoinHandler->>Store: Create channel with history-loading flags based on capability
Store->>JoinHandler: setState update completes
alt draft/chathistory capability present
JoinHandler->>IrcClient: sendRaw CHATHISTORY LATEST ... * 50
JoinHandler->>JoinHandler: Emit CHATHISTORY_LOADING with isLoading: true
else draft/chathistory not supported
JoinHandler->>IrcClient: sendRaw WHO ... %cuhnfaro
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/store/handlers/users.ts (1)
151-151: ⚡ Quick winRemove redundant boolean coercion.
hasChathistoryis already a boolean (derived from!!server.capabilities?.includes(...)on line 130), so the!!prefix on line 151 is redundant.♻️ Simplify to remove redundant coercion
- needsWhoRequest: !!hasChathistory, + needsWhoRequest: hasChathistory,🤖 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/store/handlers/users.ts` at line 151, Remove the redundant boolean coercion on the needsWhoRequest property: replace the double-bang expression with the existing boolean variable hasChathistory (i.e., set needsWhoRequest to hasChathistory). hasChathistory is already a boolean (derived from !!server.capabilities?.includes(...) earlier), so update the assignment in the object where needsWhoRequest is set to simply reference hasChathistory to avoid unnecessary coercion.
🤖 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.
Nitpick comments:
In `@src/store/handlers/users.ts`:
- Line 151: Remove the redundant boolean coercion on the needsWhoRequest
property: replace the double-bang expression with the existing boolean variable
hasChathistory (i.e., set needsWhoRequest to hasChathistory). hasChathistory is
already a boolean (derived from !!server.capabilities?.includes(...) earlier),
so update the assignment in the object where needsWhoRequest is set to simply
reference hasChathistory to avoid unnecessary coercion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b7bcc3e-1e96-461d-a6fe-bff18422b8f6
📒 Files selected for processing (2)
src/store/handlers/users.tstests/store/join.test.ts
matheusfillipe
previously approved these changes
Jun 5, 2026
ValwareIRC
added a commit
that referenced
this pull request
Jun 5, 2026
Six fixes for items mupuf raised against the bouncer-networks test build (issue #120, 2026-05-21). Items 5, 6, 7, 10, 11, 12, 13 from his list are general-client concerns that affect every connection, not bouncer-specific -- tracked separately. #1 deleteServer wipes every server sharing host:port --------------------------------------------------- The filter keyed on (host, port) to drop the saved row. Bouncer children share their parent's soju endpoint (only bouncerNetid distinguishes them), so deleting one bouncer-bound network silently removed the parent and every sibling from localStorage. On next reload nothing was there and the reconnect path surfaced as "Authentication required" spam. Fix: key the filter on the unique server id. New regression test pins it (deleteServerBouncer.test.ts, 3 cases). #2 ADDNETWORK shows a spinner forever; user reloads to see the network ------------------------------------------------------------------ The form's optimistic 500ms timer closed itself on no-FAIL, but the networks list never refreshed -- soju only pushes a BOUNCER NETWORK notification when the `-notify` cap is ACK'd, and some deployments don't advertise it. Fire BOUNCER LISTNETWORKS on the success path so the new row appears regardless of notify-cap state. #3 Connect button text was illegible --------------------------------- The button used `bg-primary` -- a CSS-var-driven class that resolves to almost-white in the dark theme, against `text-white`. Switched every primary-action button in the bouncer panels to `bg-discord-primary` (#5865F2) + `text-white`, matching the Invitations panel's convention. Also fixed the never-actually- defined `bg-primary-hover` class. #4 Networks not auto-joined ------------------------ Already addressed by PR #249's server-initiated-JOIN fix, which is on this branch via the main-merge. PR #249's handler is now also gated on isBouncerControl (see #9 below) so the meta connection doesn't try to CHATHISTORY the empty channels soju auto-joins there. #8 No way to set the WebSocket path (mupuf had to nginx-rewrite) ------------------------------------------------------------ ircClient.connect already preserves a custom path when the host field contains a URL with one, but it was undiscoverable. Added helper text under the Server Host input explaining that a full URL like `wss://host:port/socket` is accepted, with a soju reverse-proxy callout. Translated across all 18 locales. #9 "Cannot fetch chat history on bouncer connection" spam ------------------------------------------------------ Soju FAILs every CHATHISTORY against the bouncer-networks meta session -- it has no real channels of its own. On resume-from- suspend, the reconnect-time loop in connection.ts fired one CHATHISTORY per cached channel, producing one error toast each. Gate every CHATHISTORY send-site on !isBouncerControl: - joinChannel (IRCClient.ts) - requestChathistoryBefore (IRCClient.ts) - reconnect-loop setTimeout (connection.ts) - server-initiated own-JOIN handler (users.ts) The bouncer control session is now treated like a cap-less server: WHO fires immediately, no CHATHISTORY round-trip. Build + 882 tests green.
ValwareIRC
added a commit
that referenced
this pull request
Jun 5, 2026
Six fixes for items mupuf raised against the bouncer-networks test build (issue #120, 2026-05-21). Items 5, 6, 7, 10, 11, 12, 13 from his list are general-client concerns that affect every connection, not bouncer-specific -- tracked separately. #1 deleteServer wipes every server sharing host:port --------------------------------------------------- The filter keyed on (host, port) to drop the saved row. Bouncer children share their parent's soju endpoint (only bouncerNetid distinguishes them), so deleting one bouncer-bound network silently removed the parent and every sibling from localStorage. On next reload nothing was there and the reconnect path surfaced as "Authentication required" spam. Fix: key the filter on the unique server id. New regression test pins it (deleteServerBouncer.test.ts, 3 cases). #2 ADDNETWORK shows a spinner forever; user reloads to see the network ------------------------------------------------------------------ The form's optimistic 500ms timer closed itself on no-FAIL, but the networks list never refreshed -- soju only pushes a BOUNCER NETWORK notification when the `-notify` cap is ACK'd, and some deployments don't advertise it. Fire BOUNCER LISTNETWORKS on the success path so the new row appears regardless of notify-cap state. #3 Connect button text was illegible --------------------------------- The button used `bg-primary` -- a CSS-var-driven class that resolves to almost-white in the dark theme, against `text-white`. Switched every primary-action button in the bouncer panels to `bg-discord-primary` (#5865F2) + `text-white`, matching the Invitations panel's convention. Also fixed the never-actually- defined `bg-primary-hover` class. #4 Networks not auto-joined ------------------------ Already addressed by PR #249's server-initiated-JOIN fix, which is on this branch via the main-merge. PR #249's handler is now also gated on isBouncerControl (see #9 below) so the meta connection doesn't try to CHATHISTORY the empty channels soju auto-joins there. #8 No way to set the WebSocket path (mupuf had to nginx-rewrite) ------------------------------------------------------------ ircClient.connect already preserves a custom path when the host field contains a URL with one, but it was undiscoverable. Added helper text under the Server Host input explaining that a full URL like `wss://host:port/socket` is accepted, with a soju reverse-proxy callout. Translated across all 18 locales. #9 "Cannot fetch chat history on bouncer connection" spam ------------------------------------------------------ Soju FAILs every CHATHISTORY against the bouncer-networks meta session -- it has no real channels of its own. On resume-from- suspend, the reconnect-time loop in connection.ts fired one CHATHISTORY per cached channel, producing one error toast each. Gate every CHATHISTORY send-site on !isBouncerControl: - joinChannel (IRCClient.ts) - requestChathistoryBefore (IRCClient.ts) - reconnect-loop setTimeout (connection.ts) - server-initiated own-JOIN handler (users.ts) The bouncer control session is now treated like a cap-less server: WHO fires immediately, no CHATHISTORY round-trip. Build + 882 tests green.
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
Empty-nicklist-on-Unreal/InspIRCd bug that #248 addressed for one entry point and missed for the other. Reproduced on UnrealIRCd Support; confirmed root cause via Raw IRC Log Viewer (no
CHATHISTORYwas sent, noWHOwas sent) and recovery via manual/WHO #chan %cuhnfaro.Diagnosis
The JOIN → CHATHISTORY → WHO chain lives inside
IRCClient.joinChannel(). When the server auto-joins us — UnrealIRCd persistence module, sajoin, operblock perform-on-connect, InspIRCd auto-join —joinChannel()never runs. The JOIN handler atusers.ts:115-147created the channel entry withneedsWhoRequest: trueand stopped there. Nothing downstream cleared the flag; nicklist stayed empty forever.Why Ergo never showed this even though it auto-joins too: Ergo's auto-join sends a
draft/chathistoryBATCH alongside the JOIN, so the batch close firesCHATHISTORY_LOADING(false)→messages.ts:2028seesneedsWhoRequest=true→ WHO goes out. UnrealIRCd and InspIRCd don't send a batch on auto-join, so nothing kicks the chain.Fix
Mirror
joinChannel's tail in the own-join + new-channel branch:draft/chathistorycap present → sendCHATHISTORY LATEST #chan * 50+ triggerCHATHISTORY_LOADING(true); existing batch-close → WHO path then runs.WHO #chan %cuhnfaroimmediately, clearneedsWhoRequest.ircClientI/O runs outside thesetStatecallback —triggerEventinsidesetStatecauses the nested-setState clobber thatbatches.ts:215-218already documents.Test plan
needsWhoRequest=trueneedsWhoRequestjoinChannelalready ran (channel exists) → no double-send/WHO🤖 Generated with Claude Code
Summary by CodeRabbit