sending incomplete mutation on chatbox-access settings#1890
sending incomplete mutation on chatbox-access settings#1890ignaciojimenezr wants to merge 1 commit intoMCPJam:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
dosubot
Bot
added
size:S
WalkthroughThe chatbox sharing functionality was refactored to use a helper function that constructs complete update payloads containing all core configuration fields alongside access control settings. The update logic now passes derived server identifiers and guest access permissions together. Corresponding test coverage was expanded with improved fixtures, helper functions, and three new async test cases validating guest access toggle behavior and state transitions. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@mcpjam-inspector/client/src/components/chatboxes/__tests__/ChatboxShareSection.test.tsx`:
- Around line 219-231: The test is flaky because the expectation for
mockUpdateChatbox runs immediately after the waitFor that detects
mockSetChatboxMode; fold the mockUpdateChatbox assertion into a waitFor (or
await it) so the test waits for updateChatbox to be called after
handleAccessPresetChange completes; specifically, wrap the
expect(mockUpdateChatbox).toHaveBeenCalledWith(expectedUpdatePayload(...))
inside a waitFor (similar to the existing waitFor for mockSetChatboxMode) to
ensure handleAccessPresetChange (which awaits setChatboxMode then calls
updateChatbox) has finished before asserting, and keep the other assertions
(mockSetChatboxMode and onUpdated) as appropriate.
In `@mcpjam-inspector/client/src/components/chatboxes/ChatboxShareSection.tsx`:
- Around line 32-51: The buildChatboxUpdatePayload function is omitting
ChatboxSettings.welcomeDialog and .feedbackDialog, which can cause those fields
to be cleared on update; update buildChatboxUpdatePayload to include
welcomeDialog and feedbackDialog in the returned payload (same keys used
elsewhere, e.g., ChatboxBuilderView's update path) so the server receives and
preserves those dialog fields when toggling allowGuestAccess.
🪄 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: a44739e6-407e-41f4-a5a0-e3483924a8e3
📒 Files selected for processing (2)
mcpjam-inspector/client/src/components/chatboxes/ChatboxShareSection.tsxmcpjam-inspector/client/src/components/chatboxes/__tests__/ChatboxShareSection.test.tsx
| await waitFor(() => { | ||
| expect(mockSetChatboxMode).toHaveBeenCalledWith({ | ||
| chatboxId: "cb-1", | ||
| mode: "invited_only", | ||
| }); | ||
| }); | ||
| expect(mockUpdateChatbox).toHaveBeenCalledWith( | ||
| expectedUpdatePayload(modeUpdatedChatbox, false), | ||
| ); | ||
| expect(mockSetChatboxMode).toHaveBeenCalledTimes(1); | ||
| await waitFor(() => { | ||
| expect(onUpdated).toHaveBeenCalledWith(fullyUpdatedChatbox); | ||
| }); |
There was a problem hiding this comment.
Second assertion should live inside waitFor to avoid a microtask race.
handleAccessPresetChange awaits setChatboxMode before invoking updateChatbox, so the moment the first waitFor observes mockSetChatboxMode was called, updateChatbox may not have been dispatched yet on the very next line. Under load this is a classic flaky-test shape. Fold the updateChatbox expectation into a waitFor (or await it separately) so the test settles deterministically.
🧪 Suggested adjustment
await waitFor(() => {
expect(mockSetChatboxMode).toHaveBeenCalledWith({
chatboxId: "cb-1",
mode: "invited_only",
});
});
- expect(mockUpdateChatbox).toHaveBeenCalledWith(
- expectedUpdatePayload(modeUpdatedChatbox, false),
- );
- expect(mockSetChatboxMode).toHaveBeenCalledTimes(1);
+ await waitFor(() => {
+ expect(mockUpdateChatbox).toHaveBeenCalledWith(
+ expectedUpdatePayload(modeUpdatedChatbox, false),
+ );
+ });
+ expect(mockSetChatboxMode).toHaveBeenCalledTimes(1);
await waitFor(() => {
expect(onUpdated).toHaveBeenCalledWith(fullyUpdatedChatbox);
});📝 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.
| await waitFor(() => { | |
| expect(mockSetChatboxMode).toHaveBeenCalledWith({ | |
| chatboxId: "cb-1", | |
| mode: "invited_only", | |
| }); | |
| }); | |
| expect(mockUpdateChatbox).toHaveBeenCalledWith( | |
| expectedUpdatePayload(modeUpdatedChatbox, false), | |
| ); | |
| expect(mockSetChatboxMode).toHaveBeenCalledTimes(1); | |
| await waitFor(() => { | |
| expect(onUpdated).toHaveBeenCalledWith(fullyUpdatedChatbox); | |
| }); | |
| await waitFor(() => { | |
| expect(mockSetChatboxMode).toHaveBeenCalledWith({ | |
| chatboxId: "cb-1", | |
| mode: "invited_only", | |
| }); | |
| }); | |
| await waitFor(() => { | |
| expect(mockUpdateChatbox).toHaveBeenCalledWith( | |
| expectedUpdatePayload(modeUpdatedChatbox, false), | |
| ); | |
| }); | |
| expect(mockSetChatboxMode).toHaveBeenCalledTimes(1); | |
| await waitFor(() => { | |
| expect(onUpdated).toHaveBeenCalledWith(fullyUpdatedChatbox); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/components/chatboxes/__tests__/ChatboxShareSection.test.tsx`
around lines 219 - 231, The test is flaky because the expectation for
mockUpdateChatbox runs immediately after the waitFor that detects
mockSetChatboxMode; fold the mockUpdateChatbox assertion into a waitFor (or
await it) so the test waits for updateChatbox to be called after
handleAccessPresetChange completes; specifically, wrap the
expect(mockUpdateChatbox).toHaveBeenCalledWith(expectedUpdatePayload(...))
inside a waitFor (similar to the existing waitFor for mockSetChatboxMode) to
ensure handleAccessPresetChange (which awaits setChatboxMode then calls
updateChatbox) has finished before asserting, and keep the other assertions
(mockSetChatboxMode and onUpdated) as appropriate.
| function buildChatboxUpdatePayload( | ||
| chatbox: ChatboxSettings, | ||
| allowGuestAccess: boolean, | ||
| ) { | ||
| return { | ||
| chatboxId: chatbox.chatboxId, | ||
| name: chatbox.name, | ||
| description: chatbox.description, | ||
| hostStyle: chatbox.hostStyle, | ||
| systemPrompt: chatbox.systemPrompt, | ||
| modelId: chatbox.modelId, | ||
| temperature: chatbox.temperature, | ||
| requireToolApproval: chatbox.requireToolApproval, | ||
| serverIds: chatbox.servers.map((server) => server.serverId), | ||
| optionalServerIds: chatbox.servers | ||
| .filter((server) => server.optional) | ||
| .map((server) => server.serverId), | ||
| allowGuestAccess, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the Convex updateChatbox mutation to determine how it treats
# omitted optional fields (welcomeDialog, feedbackDialog), and confirm
# the exact argument shape the handler expects.
fd -t f 'chatboxes' | rg -n '\.(ts|js)$'
rg -nP -C5 '\bupdateChatbox\s*[:=]\s*mutation' --type=ts
rg -nP -C3 '\bwelcomeDialog\b|\bfeedbackDialog\b' --type=tsRepository: MCPJam/inspector
Length of output: 50372
🏁 Script executed:
# 1. Examine the buildChatboxUpdatePayload function directly
cat -n mcpjam-inspector/client/src/components/chatboxes/ChatboxShareSection.tsx | sed -n '30,55p'Repository: MCPJam/inspector
Length of output: 1034
🏁 Script executed:
# 2. Search for the updateChatbox mutation definition in server code
rg -n 'updateChatbox' mcpjam-inspector/server --type=ts -A 10 | head -100Repository: MCPJam/inspector
Length of output: 42
🏁 Script executed:
# 3. Look for mutation handler that shows how it processes the payload
fd -t f -path '*/server/*' -name '*chatbox*' | rg '\.(ts|js)$'Repository: MCPJam/inspector
Length of output: 228
🏁 Script executed:
# Search more broadly for updateChatbox in server directory
rg -n 'updateChatbox' mcpjam-inspector/server --type ts | head -30Repository: MCPJam/inspector
Length of output: 42
🏁 Script executed:
# Look for mutation definitions in Convex files
rg -n 'mutation.*updateChatbox|export.*mutation' mcpjam-inspector/server --type ts -B 2 -A 15 | head -80Repository: MCPJam/inspector
Length of output: 42
🏁 Script executed:
# Find chatbox mutation/update related files
fd -t f 'chatbox' mcpjam-inspector/serverRepository: MCPJam/inspector
Length of output: 170
🏁 Script executed:
# Examine the server-side chatboxes file
cat -n mcpjam-inspector/server/routes/web/chatboxes.ts | head -200Repository: MCPJam/inspector
Length of output: 3903
🏁 Script executed:
# Search for update or mutation patterns in the file
rg -n 'update|mutation|patch' mcpjam-inspector/server/routes/web/chatboxes.ts -i -A 5Repository: MCPJam/inspector
Length of output: 42
🏁 Script executed:
# Find where buildChatboxUpdatePayload is called
rg -n 'buildChatboxUpdatePayload' mcpjam-inspector --type ts --type tsxRepository: MCPJam/inspector
Length of output: 87
🏁 Script executed:
# Look for Convex backend/mutation code
fd -t d 'convex|backend|mutations' mcpjam-inspectorRepository: MCPJam/inspector
Length of output: 42
🏁 Script executed:
# Check repo structure at root level
ls -la mcpjam-inspector/ | head -30Repository: MCPJam/inspector
Length of output: 1827
🏁 Script executed:
# Find buildChatboxUpdatePayload usage (fix tsx syntax)
rg -n 'buildChatboxUpdatePayload' mcpjam-inspectorRepository: MCPJam/inspector
Length of output: 316
🏁 Script executed:
# Look inside server directory for mutation/convex code
ls -la mcpjam-inspector/server/Repository: MCPJam/inspector
Length of output: 1140
🏁 Script executed:
# Search for updateChatbox in entire repo
rg -n 'updateChatbox' mcpjam-inspector -A 10Repository: MCPJam/inspector
Length of output: 17506
🏁 Script executed:
# Look in shared directory for mutation definitions
find mcpjam-inspector/shared -type f -name "*.ts"Repository: MCPJam/inspector
Length of output: 1457
🏁 Script executed:
# Search for "chatboxes:updateChatbox" definition
rg -n 'chatboxes.*updateChatbox' mcpjam-inspector --type ts -B 5 -A 15Repository: MCPJam/inspector
Length of output: 1953
🏁 Script executed:
# Look for Convex-related files in backend
find mcpjam-inspector -name '*.convex.ts' -o -name 'convex.config.ts'Repository: MCPJam/inspector
Length of output: 42
🏁 Script executed:
# Search for mutation handler definitions in server
rg -n 'export.*mutation|mutation\(' mcpjam-inspector/server --type ts -A 10 | grep -A 10 -i 'update.*chatbox'Repository: MCPJam/inspector
Length of output: 42
🏁 Script executed:
# Look for files that might contain mutation definitions
find mcpjam-inspector/server -name "*mutation*" -o -name "*chatbox*.ts" | head -20Repository: MCPJam/inspector
Length of output: 170
🏁 Script executed:
# Search for schema or handler patterns
rg -n 'updateChatbox.*=' mcpjam-inspector/server --type ts -B 2 -A 10Repository: MCPJam/inspector
Length of output: 42
🏁 Script executed:
# Check src directory at root level
ls -la mcpjam-inspector/src/Repository: MCPJam/inspector
Length of output: 351
🏁 Script executed:
# Search across entire repo for updateChatbox function definition
rg -n 'function updateChatbox|const updateChatbox' mcpjam-inspector --type ts -A 15Repository: MCPJam/inspector
Length of output: 1492
🏁 Script executed:
# Search for where the mutation payload is actually handled
rg -n 'welcomeDialog.*feedbackDialog|payload.*chatbox' mcpjam-inspector --type ts -B 3 -A 5 | head -100Repository: MCPJam/inspector
Length of output: 3511
🏁 Script executed:
# Check if there's any test that shows how updateChatbox handles payloads
rg -n 'updateChatbox' mcpjam-inspector/client/src/components/chatboxes/__tests__/ChatboxShareSection.test.tsx -B 5 -A 15 | head -80Repository: MCPJam/inspector
Length of output: 610
🏁 Script executed:
# Look for any mock or test that shows payload structure expectations
rg -n 'welcomeDialog|feedbackDialog' mcpjam-inspector/client/src/components/chatboxes/__tests__/ChatboxShareSection.test.tsx -B 2 -A 2Repository: MCPJam/inspector
Length of output: 42
Include welcomeDialog and feedbackDialog in the share-section update payload to prevent inconsistency.
buildChatboxUpdatePayload (lines 32–51) omits welcomeDialog and feedbackDialog, yet these fields exist in ChatboxSettings and are explicitly included in other update paths (e.g., ChatboxBuilderView lines 775–776). If the server mutation treats omitted optional fields as "clear to null" rather than preserving them, toggling guest access could silently erase dialog configuration. Match the approach used elsewhere by forwarding these fields through.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/chatboxes/ChatboxShareSection.tsx`
around lines 32 - 51, The buildChatboxUpdatePayload function is omitting
ChatboxSettings.welcomeDialog and .feedbackDialog, which can cause those fields
to be cleared on update; update buildChatboxUpdatePayload to include
welcomeDialog and feedbackDialog in the returned payload (same keys used
elsewhere, e.g., ChatboxBuilderView's update path) so the server receives and
preserves those dialog fields when toggling allowGuestAccess.
2 participants
Summary
Fixes the chatbox Share panel Access settings dropdown by sending the full payload required by chatboxes:updateChatbox instead of an incomplete partial update.
Adds regression coverage for the access-setting transitions that were hitting this path.
Context
The bug was that changing chatbox access reused updateChatbox, which is a large general-purpose mutation, even though this UI only needed to update access behavior.
Follow-up
Add a dedicated narrow Convex mutation for chatbox access updates so this UI no longer depends on the full updateChatbox contract.
perhaps:
setChatboxAccess({
chatboxId,
mode,
allowGuestAccess,
})