fix(labeled-response): inherit batch label onto inner messages#211
Merged
Conversation
When the server emits more than one line in reply to a labeled command (e.g. PRIVMSG to an /AWAY user, where the server sends both 301 RPL_AWAY and the echo), the IRCv3 labeled-response spec mandates wrapping them in a batch -- the label tag lives on the BATCH opener and inner messages only carry @Batch=ID. Our pending-message dedup (confirmPendingMessage) looked for mtags?.label directly on the PRIVMSG echo and never matched in the batched case. That left the optimistic placeholder to time out into the "crossed-out" failure state and rendered the server echo as a new, duplicate message. Fix: in the protocol layer, when a PRIVMSG arrives inside a labeled-response batch, propagate the batch's label onto the message's mtags before dispatching CHANMSG / USERMSG. Store-side label-matching code stays as-is, so the same code path handles single-line and batched echoes uniformly. An explicit inner label tag still wins (defensive; should not happen per spec).
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 (39)
📝 WalkthroughWalkthroughThe PR adds protocol-level label inheritance from labeled-response BATCH frames to inner messages, then introduces UI components and a store action to display and retry failed messages. Messages sent with new labels from failed retries now integrate with the label-hoisting mechanism for proper deduplication. ChangesPending and Failed Message Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/lib/labeledResponseBatch.test.ts (1)
1-134: ⚡ Quick winMove this suite under
tests/protocol/to match test-organization rules.This suite validates IRC protocol handler behavior, so it should live in the protocol test tree instead of
tests/lib/.As per coding guidelines,
tests/**/*.{ts,tsx}: “Mirrorsrc/structure intests/directory … and IRC protocol tests intests/protocol/”.🤖 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 `@tests/lib/labeledResponseBatch.test.ts` around lines 1 - 134, The test suite "labeled-response — batch label inheritance" (file containing tests referencing handleBatch and handlePrivmsg) is in the wrong test tree; move the entire test file into the protocol tests directory so it mirrors src/ and lives with other IRC protocol tests, then update its import paths as needed (e.g., the imports of "../../src/lib/irc/handlers/messages" and "../../src/lib/irc/IRCClientContext") so they resolve from the new location and run under the protocol test runner configuration. Ensure no code changes to tests themselves; just relocate the file and fix relative imports.
🤖 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 `@tests/lib/labeledResponseBatch.test.ts`:
- Around line 1-134: The test suite "labeled-response — batch label inheritance"
(file containing tests referencing handleBatch and handlePrivmsg) is in the
wrong test tree; move the entire test file into the protocol tests directory so
it mirrors src/ and lives with other IRC protocol tests, then update its import
paths as needed (e.g., the imports of "../../src/lib/irc/handlers/messages" and
"../../src/lib/irc/IRCClientContext") so they resolve from the new location and
run under the protocol test runner configuration. Ensure no code changes to
tests themselves; just relocate the file and fix relative imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30fb46c3-0a5d-4d2c-b876-b83569820a33
📒 Files selected for processing (2)
src/lib/irc/handlers/messages.tstests/lib/labeledResponseBatch.test.ts
|
Automated deployment preview for the PR in the Cloudflare Pages. |
Visual changes to the optimistic-send lifecycle in MessageItem: - Pending messages no longer flip into italic. Italic→non-italic on echo-arrival was a jarring layout shift; the opacity alone is enough signal that the message hasn't been confirmed yet. - After 500ms still pending (i.e. the labeled-response echo hasn't arrived), a small spinner appears next to the message. The 500ms delay means the common fast-echo case never flashes the spinner. - Failed messages now show a circular-refresh icon button next to the line-through text. Clicking it re-sends the original content with a fresh label and re-arms the 30s timeout via a new store action `resendFailedMessage(messageId)`. The spinner SVG is inlined to avoid pulling another react-icons sub- package; the retry icon is the same Material refresh glyph.
matheusfillipe
previously approved these changes
May 13, 2026
Two reviewer-driven cleanups on top of the labeled-response fix: - Matt (matheusfillipe) flagged that the inline SVGs in MessageStatusIndicator should live in the shared icon module that already exists for this exact purpose. Move the spinner and the refresh glyph into src/components/message/icons.tsx as SpinnerIcon / RefreshIcon, and consume them from MessageStatusIndicator. - CodeRabbit nitpick (and a repo convention point from CLAUDE.md): IRC-protocol tests live under tests/protocol/, not tests/lib/. Move tests/lib/labeledResponseBatch.test.ts to tests/protocol/. No import changes needed since the relative depth is identical. No behavioural change. Same 785 tests pass; build still clean.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/store/index.ts (1)
1777-1829: ⚖️ Poor tradeoffConsider storing the timeout handle to allow cancellation on retry.
When a user retries a failed message multiple times before the 30-second timeout elapses, multiple
setTimeoutcalls accumulate. Each timeout invokesfailPendingMessagewith its respective label. SincefailPendingMessagematches bypendingLabel, only the latest retry's timeout will take effect; earlier timeouts become no-ops. While harmless, this results in unnecessary pending timers.Optional enhancement: store timeout handle in message state
You could extend the
Messagetype with an optionalpendingTimeoutHandle?: NodeJS.Timeoutfield, then clear it on retry:const label = makeLabel(); +let timeoutHandle: NodeJS.Timeout | undefined; set((state) => { const list = state.messages[channelKey]; if (!list) return state; + const existing = list.find((m) => m.id === messageId); + if (existing?.pendingTimeoutHandle) clearTimeout(existing.pendingTimeoutHandle); const next = list.map((m) => m.id === messageId ? { ...m, status: "pending" as const, pendingLabel: label, timestamp: new Date(), + pendingTimeoutHandle: undefined, } : m, ); + timeoutHandle = setTimeout(() => { + get().failPendingMessage(msg.serverId, msg.channelId, label); + }, 30_000); + // Update the message again with the timeout handle + const final = next.map((m) => + m.id === messageId ? { ...m, pendingTimeoutHandle: timeoutHandle } : m + ); return { messages: { ...state.messages, - [channelKey]: next, + [channelKey]: final, }, }; });This avoids accumulating stale timeouts, though the current behavior is functionally correct.
🤖 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/index.ts` around lines 1777 - 1829, resendFailedMessage currently starts a new setTimeout for failPendingMessage without cancelling any existing pending timer, causing stale timers to accumulate; update the Message type to include an optional pendingTimeoutHandle (e.g., pendingTimeoutHandle?: ReturnType<typeof setTimeout> or NodeJS.Timeout), then in resendFailedMessage clear any existing timeout for that message before scheduling a new one (use clearTimeout on the existing pendingTimeoutHandle), store the new timeout handle in the message state when calling set(...), and ensure failPendingMessage (and any success path) clears and removes pendingTimeoutHandle so timers are not left running.
🤖 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/message/MessageStatusIndicator.tsx`:
- Around line 35-48: The JSX contains user-visible strings ("Sending", "Retry
sending") in MessageStatusIndicator that must be internationalized; import the t
macro from '@lingui/macro' at the top of MessageStatusIndicator.tsx and wrap the
aria-label and title prop values (and the "Sending" aria-label on the Spinner
span) with t`...` so they become translatable (e.g., aria-label={t`Sending`} and
aria-label={t`Retry sending`} and title={t`Retry sending`}), leaving onRetry and
existing handlers unchanged.
---
Nitpick comments:
In `@src/store/index.ts`:
- Around line 1777-1829: resendFailedMessage currently starts a new setTimeout
for failPendingMessage without cancelling any existing pending timer, causing
stale timers to accumulate; update the Message type to include an optional
pendingTimeoutHandle (e.g., pendingTimeoutHandle?: ReturnType<typeof setTimeout>
or NodeJS.Timeout), then in resendFailedMessage clear any existing timeout for
that message before scheduling a new one (use clearTimeout on the existing
pendingTimeoutHandle), store the new timeout handle in the message state when
calling set(...), and ensure failPendingMessage (and any success path) clears
and removes pendingTimeoutHandle so timers are not left running.
🪄 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: 009adc4d-efdd-4e87-860e-436f46ae3afc
📒 Files selected for processing (6)
src/components/message/MessageItem.tsxsrc/components/message/MessageStatusIndicator.tsxsrc/components/message/icons.tsxsrc/store/index.tstests/components/MessageStatusIndicator.test.tsxtests/protocol/labeledResponseBatch.test.ts
CodeRabbit pointed out that the new component's aria-label / title
attributes ("Sending", "Retry sending") were user-visible English
strings without i18n wrapping, which violates CLAUDE.md's policy
that aria-label / title in JSX should go through t`...` so
translators can pick them up.
Imported useLingui from @lingui/react/macro (the convention used by
the other components in src/components/message/) and wrapped the
three strings. No runtime behaviour change; the lingui-react test
mock passes English through verbatim so the existing
getByLabelText queries still match. Translations filled in across
all 18 non-English locale catalogs.
Skipped CodeRabbit's other suggestion (storing the setTimeout
handle on the Message to clear it on retry): it's marked "Poor
tradeoff / Nitpick" by CodeRabbit itself ("functionally correct
... unnecessary pending timers"), and threading a non-serializable
NodeJS.Timeout through Zustand state to avoid a small timer object
that fires once and no-ops isn't worth the type-system noise.
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
/AWAYuser, which produces both301 RPL_AWAYand the echo), the IRCv3 labeled-response spec wraps them in a batch. Thelabeltag lives on the BATCH opener; inner messages only carry@batch=ID.mtags?.labeldirectly on the PRIVMSG echo, so it missed the batched case. The optimistic placeholder timed out into the crossed-out failure state and the server's echo rendered as a brand-new, duplicate message.src/lib/irc/handlers/messages.ts): when a PRIVMSG arrives inside alabeled-responsebatch, propagate the batch'slabelonto the message's mtags before triggeringCHANMSG/USERMSG. Store-side label matching stays untouched.What was actually on the wire
The echo line carries
@batch=IDbut no@label=, so the prior matcher couldn't see it.Test plan
npm run format,npm run fix:unsafe,npm run test(781 pass),npm run buildcleantests/lib/labeledResponseBatch.test.tscovers:labeled-responsebatch openerlabeled-responsebatch openerlabeled-responsebatch (e.g.chathistory) does NOT pick up a stray labellabeltag wins over batch label (defensive)/AWAYset; expect a single message in the local view (not two with the optimistic one crossed out)Scope
Only
handlePrivmsgis touched.handleNotice/handleTagmsgdon't have label-matching downstream, so propagating there would be inert; left as-is. Nested labeled-response wrapping other batch types is also out of scope (not used in practice).Summary by CodeRabbit