Extract composer send-gating logic from Conversations page#1239
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR centralizes composer send gating by adding a composer-send decision module (evaluateComposerSend, handleComposerSlashCommand, getComposerBlockedSendFeedback) and wiring it into Conversations.tsx; tests and render-suite updates exercise slash commands, usage-limit blocking, and successful send persistence. (50 words) ChangesComposer Send Decision Pipeline
Sequence DiagramsequenceDiagram
participant User
participant Composer as Composer (UI)
participant Evaluator as evaluateComposerSend
participant Feedback as getComposerBlockedSendFeedback
participant Component as Conversations Component
participant Chat as Chat Service / threadApi
User->>Composer: Type message & click Send
Composer->>Component: handleSendMessage(rawText)
Component->>Evaluator: evaluateComposerSend({rawText, selectedThreadId, composerBlocked, isAtLimit, socketStatus})
alt shouldSend = true
Evaluator-->>Component: {shouldSend: true, trimmedText}
Component->>Component: persist local user message (threadApi.appendMessage)
Component->>Chat: send via chat service (chatSend)
Chat-->>Component: ack/response
Component->>Composer: clear textarea
else shouldSend = false
Evaluator-->>Component: {shouldSend: false, blockReason}
Component->>Feedback: getComposerBlockedSendFeedback(blockReason)
Feedback-->>Component: {showLimitModal?, error?} or null
Component->>Composer: show modal or set send error (if provided)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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. 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 |
…n-sym-85-extract-chat-composer-behavior-from # Conflicts: # app/src/pages/Conversations.tsx
senamakel
left a comment
senamakel
left a comment
There was a problem hiding this comment.
coverage tests are failing. do write the code to improve test coverage
senamakel
left a comment
senamakel
left a comment
There was a problem hiding this comment.
coverage tests are failing. do write the code to improve test coverage
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/pages/__tests__/Conversations.render.test.tsx (1)
625-628: ⚡ Quick winUse the existing mocks via static imports instead of
await import(...).These new assertions are already backed by hoisted
vi.mock(...)calls, so the extra dynamic imports just add async noise and drift from the repo’s import convention.♻️ Suggested cleanup
+import { chatSend } from '../../services/chatService'; +import { threadApi } from '../../services/api/threadApi'; @@ it('handles /new from the composer without sending chat text', async () => { const { textarea } = await renderSelectedConversation(); - const { chatSend } = await import('../../services/chatService'); - const { threadApi } = await import('../../services/api/threadApi'); @@ it('shows the usage-limit modal instead of sending when the account is at limit', async () => { const { textarea } = await renderSelectedConversation({ isAtLimit: true }); - const { chatSend } = await import('../../services/chatService'); @@ it('persists a local user message and sends through chat service for valid input', async () => { const { textarea, thread } = await renderSelectedConversation(); - const { chatSend } = await import('../../services/chatService'); - const { threadApi } = await import('../../services/api/threadApi');As per coding guidelines,
**/*.{ts,tsx}should “Prefer static imports at the top of the module; do not use dynamic imports (asyncimport(),React.lazy(),await import()).”Also applies to: 639-655
🤖 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 `@app/src/pages/__tests__/Conversations.render.test.tsx` around lines 625 - 628, Replace the dynamic imports used in the test (e.g., await import('../../services/chatService') and await import('../../services/api/threadApi')) with the statically imported mocks already hoisted for this test file so the assertions use the existing mocked implementations; locate the test that calls renderSelectedConversation() and destructures textarea and then references chatSend and threadApi, remove the await import(...) calls and use the top-level static imports (the mocked chatSend and threadApi symbols) instead; also apply the same change for the other occurrences around the 639-655 test block so all tests use the static, hoisted mocks consistently.
🤖 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 `@app/src/pages/Conversations.tsx`:
- Around line 498-515: The current flow calls evaluateComposerSend and returns
early for sendDecision.blockReason === 'missing_thread' before allowing slash
commands to run; move or replicate the slash handling so
handleSlashCommand(trimmed) is evaluated before the early return for
'missing_thread' (or explicitly allow 'missing_thread' to fall through if
trimmed starts with a slash command). Update the block around
evaluateComposerSend, sendDecision.blockReason checks, and handleSlashCommand
usage so that when selectedThreadId is null the trimmed input is still passed to
handleSlashCommand (referencing evaluateComposerSend, sendDecision.blockReason,
handleSlashCommand, selectedThreadId, and trimmed).
---
Nitpick comments:
In `@app/src/pages/__tests__/Conversations.render.test.tsx`:
- Around line 625-628: Replace the dynamic imports used in the test (e.g., await
import('../../services/chatService') and await
import('../../services/api/threadApi')) with the statically imported mocks
already hoisted for this test file so the assertions use the existing mocked
implementations; locate the test that calls renderSelectedConversation() and
destructures textarea and then references chatSend and threadApi, remove the
await import(...) calls and use the top-level static imports (the mocked
chatSend and threadApi symbols) instead; also apply the same change for the
other occurrences around the 639-655 test block so all tests use the static,
hoisted mocks consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f81f27d5-76d9-44b3-bd26-d6968953506e
📒 Files selected for processing (4)
app/src/pages/Conversations.tsxapp/src/pages/__tests__/Conversations.render.test.tsxapp/src/pages/conversations/composerSendDecision.test.tsapp/src/pages/conversations/composerSendDecision.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/pages/Conversations.tsx (2)
487-494: 💤 Low value
welcomeLockedis hardcoded tofalse;decision.blockedByWelcomeLockis unused.Consistent with the welcome-lock removal in
#1123, but the second argument and the returnedblockedByWelcomeLockfield are now effectively dead. If welcome-lock isn't coming back, consider simplifyinghandleComposerSlashCommand's signature incomposerSendDecision.ts(drop the parameter and theblockedByWelcomeLockfield). Otherwise, leaving the seam in place for future wiring is fine.🤖 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 `@app/src/pages/Conversations.tsx` around lines 487 - 494, The call in handleSlashCommand hardcodes welcomeLocked as false (handleComposerSlashCommand(command, false)) and never uses decision.blockedByWelcomeLock, leaving a dead parameter/field; either remove the unused boolean parameter and the blockedByWelcomeLock field from composerSendDecision.ts and update all call sites (including handleSlashCommand) to call handleComposerSlashCommand(command) and stop returning blockedByWelcomeLock, or if you want to keep the seam, thread the real welcome lock state into handleSlashCommand and pass that state instead of false and handle decision.blockedByWelcomeLock appropriately; update the function signature in composerSendDecision.ts and all references (handleComposerSlashCommand, blockedByWelcomeLock) accordingly.
496-538: 💤 Low valueSend-gating flow looks good — earlier
/newordering concern is addressed.
handleSlashCommand(trimmedInput)now runs beforeevaluateComposerSend(...), so/newand/clearwork even whenselectedThreadIdisnull(i.e. the very state/newis meant to recover from). Theif (!sendingThreadId) return;guard at Line 538 is redundant at runtime (themissing_threadearly return covers it) but is appropriate for TypeScript narrowing onselectedThreadId: string | null.One small observation:
checkPromptInjection(trimmed)at Line 519 runs and updatessendAdvisoryeven when the request is about to be blocked by usage-limit/socket reasons at Line 526. That means the advisory text can flash alongside a quota error for messages that never get sent. Probably fine UX-wise (advisory is informational), but worth a sanity check if you want the gate to short-circuit before any prompt-guard side effects.🤖 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 `@app/src/pages/Conversations.tsx` around lines 496 - 538, Prompt-injection check (checkPromptInjection) runs and sets sendAdvisory even when sendDecision later blocks sending (usage limit/socket), causing advisory flash; to fix, defer calling checkPromptInjection and setSendAdvisory until after you confirm sendDecision.shouldSend is true (and after handling showLimitModal/sendError), i.e., move the checkPromptInjection(trimmed) / setSendAdvisory logic to run only when sendDecision.shouldSend === true (use the existing sendDecision, getComposerBlockedSendFeedback, and setShowLimitModal/setSendError branches to gate it), keeping the early type-narrowing guard for selectedThreadId and preserving handleSlashCommand/evaluateComposerSend ordering.app/src/pages/__tests__/Conversations.render.test.tsx (1)
625-642: 💤 Low value
/newslash-command test verifies the regression fix — consider also covering/clear.This test pins the exact bug the previous review flagged:
/newworks even whenselectedThreadIdis null (empty thread state withmockGetThreadsleft pending so no auto-thread is created). Asserting bothcreateNewThreadwas called andchatSendwas not is the right behavior contract.
handleComposerSlashCommandtreats/clearidentically to/new, but only/newis exercised here. A one-liner mirror test (or ait.each) for/clearwould lock that branch in too.💡 Optional: parametrize over both commands
- it('handles /new from the composer without a selected thread or sending chat text', async () => { - mockGetThreads.mockReturnValue(new Promise(() => {})); - - await act(async () => { - await renderConversations({ thread: emptyThreadState, socket: socketState('connected') }); - }); - const textarea = await screen.findByPlaceholderText('Type a message...'); - vi.mocked(threadApi.createNewThread).mockClear(); - vi.mocked(chatSend).mockClear(); - - await submitComposerText(textarea, '/new'); - - await waitFor(() => { - expect(threadApi.createNewThread).toHaveBeenCalled(); - }); - expect(chatSend).not.toHaveBeenCalled(); - expect(textarea).toHaveValue(''); - }); + it.each(['/new', '/clear'])( + 'handles %s from the composer without a selected thread or sending chat text', + async command => { + mockGetThreads.mockReturnValue(new Promise(() => {})); + + await act(async () => { + await renderConversations({ thread: emptyThreadState, socket: socketState('connected') }); + }); + const textarea = await screen.findByPlaceholderText('Type a message...'); + vi.mocked(threadApi.createNewThread).mockClear(); + vi.mocked(chatSend).mockClear(); + + await submitComposerText(textarea, command); + + await waitFor(() => { + expect(threadApi.createNewThread).toHaveBeenCalled(); + }); + expect(chatSend).not.toHaveBeenCalled(); + expect(textarea).toHaveValue(''); + } + );🤖 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 `@app/src/pages/__tests__/Conversations.render.test.tsx` around lines 625 - 642, Add a mirror test to cover the `/clear` slash-command branch (or parametrize with it.each over ['/new','/clear']) in Conversations.render.test.tsx so the same regression is locked: reuse the existing setup (mockGetThreads pending, renderConversations with thread: emptyThreadState and socket: socketState('connected')), clear mocks for vi.mocked(threadApi.createNewThread) and vi.mocked(chatSend), call submitComposerText(textarea, '/clear') and assert threadApi.createNewThread was called, chatSend was not called, and the textarea is cleared; reference the existing helpers submitComposerText, renderConversations, threadApi.createNewThread, and chatSend to implement the new assertion.
🤖 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 `@app/src/pages/__tests__/Conversations.render.test.tsx`:
- Around line 625-642: Add a mirror test to cover the `/clear` slash-command
branch (or parametrize with it.each over ['/new','/clear']) in
Conversations.render.test.tsx so the same regression is locked: reuse the
existing setup (mockGetThreads pending, renderConversations with thread:
emptyThreadState and socket: socketState('connected')), clear mocks for
vi.mocked(threadApi.createNewThread) and vi.mocked(chatSend), call
submitComposerText(textarea, '/clear') and assert threadApi.createNewThread was
called, chatSend was not called, and the textarea is cleared; reference the
existing helpers submitComposerText, renderConversations,
threadApi.createNewThread, and chatSend to implement the new assertion.
In `@app/src/pages/Conversations.tsx`:
- Around line 487-494: The call in handleSlashCommand hardcodes welcomeLocked as
false (handleComposerSlashCommand(command, false)) and never uses
decision.blockedByWelcomeLock, leaving a dead parameter/field; either remove the
unused boolean parameter and the blockedByWelcomeLock field from
composerSendDecision.ts and update all call sites (including handleSlashCommand)
to call handleComposerSlashCommand(command) and stop returning
blockedByWelcomeLock, or if you want to keep the seam, thread the real welcome
lock state into handleSlashCommand and pass that state instead of false and
handle decision.blockedByWelcomeLock appropriately; update the function
signature in composerSendDecision.ts and all references
(handleComposerSlashCommand, blockedByWelcomeLock) accordingly.
- Around line 496-538: Prompt-injection check (checkPromptInjection) runs and
sets sendAdvisory even when sendDecision later blocks sending (usage
limit/socket), causing advisory flash; to fix, defer calling
checkPromptInjection and setSendAdvisory until after you confirm
sendDecision.shouldSend is true (and after handling showLimitModal/sendError),
i.e., move the checkPromptInjection(trimmed) / setSendAdvisory logic to run only
when sendDecision.shouldSend === true (use the existing sendDecision,
getComposerBlockedSendFeedback, and setShowLimitModal/setSendError branches to
gate it), keeping the early type-narrowing guard for selectedThreadId and
preserving handleSlashCommand/evaluateComposerSend ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a609ceb6-8d06-4e53-8768-d5e96bf3e667
📒 Files selected for processing (2)
app/src/pages/Conversations.tsxapp/src/pages/__tests__/Conversations.render.test.tsx
3 participants
Motivation
Conversations.tsxby extracting the composer send gating and slash-command decision logic into a small, testable seam so behavior can be unit tested and reasoned about independently.Description
app/src/pages/conversations/composerSendDecision.tsthat exportsevaluateComposerSend(...)for send-gating andhandleComposerSlashCommand(...)for/newand/clearhandling with welcome-lock awareness.app/src/pages/Conversations.tsxto delegate composer decision logic to the extracted functions and to guard thesendingThreadIdusage for type safety.app/src/pages/conversations/composerSendDecision.test.tsthat cover empty input, welcome-lock slash command behavior, usage-limit blocking, socket disconnected blocking, and the successful send-path setup.Testing
pnpm --dir app exec vitest run src/pages/conversations/composerSendDecision.test.ts src/pages/__tests__/Conversations.test.ts --config test/vitest.config.tsand the tests passed (2 test files, 9 tests passed).pnpm typecheckand it completed successfully.pnpm --dir app test -- chattarget but it did not finish in this environment, so validation used the focused Vitest commands above which succeeded.Codex Task
Summary by CodeRabbit
New Features
Improvements
Tests