access setting issues in chatbox#1897
Open
ignaciojimenezr wants to merge 4 commits intoMCPJam:mainfrom
Open
Conversation
Contributor
✅ 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
the
size:XL
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
WalkthroughThese changes extend the chatbox builder to support email invitations for draft chatboxes before saving. A new 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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
mcpjam-inspector/client/src/components/chatboxes/builder/ChatboxBuilderView.tsx (1)
834-852: Optimistic success toast lands before the invite loop resolves.
toast.success("Chatbox created")fires at Line 834, then per-email failures toast afterwards. The chatbox genuinely was created, so the message is technically accurate, but sequencing "created" before "failed to invite X" reads as if the whole flow succeeded. A short deferral (or a combined summary toast after the loop, e.g.,Chatbox created — 2 invites failed) would give clearer feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/chatboxes/builder/ChatboxBuilderView.tsx` around lines 834 - 852, Move the optimistic success toast so it runs after the invite loop and surface a combined result instead of firing before invites complete: after creating the chatbox (created.chatboxId) keep the emailsToInvite loop that calls upsertChatboxMember and track failures (hadInviteFailure or a failure count), await all invite attempts, then call toast.success with a summary like "Chatbox created" or "Chatbox created — N invites failed" (or call toast.error if all invites failed), ensuring you reference toast.success/toast.error, emailsToInvite, upsertChatboxMember, hadInviteFailure (or failureCount) and created.chatboxId when implementing the change.mcpjam-inspector/client/src/components/chatboxes/builder/__tests__/ChatboxBuilderView.test.tsx (1)
379-407: Test name slightly overstates what's being gated.Save here is disabled because the
internal-qastarter has noselectedServerIds(sosetupHasBlockingSectionsis true via the servers section), not because of staged-invite state. The assertions are still valid, but the title reads as if the invite state is the gate. A rename like"does not call createChatbox or upsertChatboxMember while setup has attention sections"would match the mechanism, or addselectedServerIds: [httpsServer._id]to actually isolate the invite-pending gate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/chatboxes/builder/__tests__/ChatboxBuilderView.test.tsx` around lines 379 - 407, The test titled "keeps the staged invite email in place when save is blocked and does not attempt an invite" misattributes why Save is disabled — it's due to the `internal-qa` starter lacking `selectedServerIds` which sets `setupHasBlockingSections` via the servers section; update the test either by renaming it to something like "does not call createChatbox or upsertChatboxMember while setup has attention sections" to reflect the actual gating, or modify the draft created from `CHATBOX_STARTERS.find(s => s.id === "internal-qa")!.createDraft("openai/gpt-5-mini")` to include `selectedServerIds: [httpsServer._id]` so the save-disabled state is caused solely by the staged invite flow; keep the rest of the assertions (checking `emailInput` value and that `mockCreateChatbox`/`mockUpsertChatboxMember` were not called) unchanged.mcpjam-inspector/client/src/components/chatboxes/builder/setup-checklist-panel.tsx (1)
521-528: Consider signalling when a duplicate email is swallowed.The parent's
onStagedAccessInviteEmailAdddedupes silently, so re-staginga@b.comafterA@B.COMclears the input with no indication that nothing changed. A one-line toast (or inline hint) would spare users the mystery. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/chatboxes/builder/setup-checklist-panel.tsx` around lines 521 - 528, The code currently clears the input silently when an invite is a duplicate (handled by inviteChatboxMember) so add a user-facing notification in the branch where inviteChatboxMember is truthy: inside handleAccessInvite, instead of doing nothing when inviteChatboxMember is true, call your app's notification/toast helper (or add a new prop like onDuplicateAccessInvite) to show a short message such as "Email already invited" and then keep current behavior (optionally still clear the input with setInviteInputEmail("")). Reference handleAccessInvite, inviteChatboxMember, onStagedAccessInviteEmailAdd, normalizedInviteInputEmail and setInviteInputEmail when making the change.
🤖 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/builder/ChatboxBuilderView.tsx`:
- Around line 829-854: The code unconditionally calls
setStagedAccessInviteEmails([]) after attempting invites, which discards
addresses that failed and leaves no retry breadcrumb; change the logic around
emailsToInvite/upsertChatboxMember to collect failed emails (e.g., failedEmails
array when catching in the for loop) and only clear stagedAccessInviteEmails for
emails that succeeded (or if no failures occurred), by calling
setStagedAccessInviteEmails(failedEmails) when there are failures; ensure you
reference and use stagedAccessInviteEmails, emailsToInvite, upsertChatboxMember,
created.chatboxId and toast.error so failed addresses remain staged (or are fed
back into the saved-chatbox invite panel) while successful invites are removed.
In
`@mcpjam-inspector/client/src/components/chatboxes/builder/setup-checklist-panel.tsx`:
- Around line 806-851: The validation message paragraph for emailValidationError
is not programmatically linked to the Input, so add a stable id to the error <p>
(e.g. "access-invite-email-error") and update the Input (id
"access-invite-email") to include aria-describedby that references that id only
when emailValidationError is present; ensure the referenced symbol names are
emailValidationError, inviteInputEmail/normalizedInviteInputEmail,
accessInviteBusy and the handler handleAccessInvite so the enabled/disabled and
error states remain consistent.
---
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/chatboxes/builder/__tests__/ChatboxBuilderView.test.tsx`:
- Around line 379-407: The test titled "keeps the staged invite email in place
when save is blocked and does not attempt an invite" misattributes why Save is
disabled — it's due to the `internal-qa` starter lacking `selectedServerIds`
which sets `setupHasBlockingSections` via the servers section; update the test
either by renaming it to something like "does not call createChatbox or
upsertChatboxMember while setup has attention sections" to reflect the actual
gating, or modify the draft created from `CHATBOX_STARTERS.find(s => s.id ===
"internal-qa")!.createDraft("openai/gpt-5-mini")` to include `selectedServerIds:
[httpsServer._id]` so the save-disabled state is caused solely by the staged
invite flow; keep the rest of the assertions (checking `emailInput` value and
that `mockCreateChatbox`/`mockUpsertChatboxMember` were not called) unchanged.
In
`@mcpjam-inspector/client/src/components/chatboxes/builder/ChatboxBuilderView.tsx`:
- Around line 834-852: Move the optimistic success toast so it runs after the
invite loop and surface a combined result instead of firing before invites
complete: after creating the chatbox (created.chatboxId) keep the emailsToInvite
loop that calls upsertChatboxMember and track failures (hadInviteFailure or a
failure count), await all invite attempts, then call toast.success with a
summary like "Chatbox created" or "Chatbox created — N invites failed" (or call
toast.error if all invites failed), ensuring you reference
toast.success/toast.error, emailsToInvite, upsertChatboxMember, hadInviteFailure
(or failureCount) and created.chatboxId when implementing the change.
In
`@mcpjam-inspector/client/src/components/chatboxes/builder/setup-checklist-panel.tsx`:
- Around line 521-528: The code currently clears the input silently when an
invite is a duplicate (handled by inviteChatboxMember) so add a user-facing
notification in the branch where inviteChatboxMember is truthy: inside
handleAccessInvite, instead of doing nothing when inviteChatboxMember is true,
call your app's notification/toast helper (or add a new prop like
onDuplicateAccessInvite) to show a short message such as "Email already invited"
and then keep current behavior (optionally still clear the input with
setInviteInputEmail("")). Reference handleAccessInvite, inviteChatboxMember,
onStagedAccessInviteEmailAdd, normalizedInviteInputEmail and setInviteInputEmail
when making the change.
🪄 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: 1d454c7e-ced8-4c11-973c-3c0e3cf5490d
📒 Files selected for processing (5)
mcpjam-inspector/client/src/components/chatboxes/builder/ChatboxBuilderExperience.tsxmcpjam-inspector/client/src/components/chatboxes/builder/ChatboxBuilderView.tsxmcpjam-inspector/client/src/components/chatboxes/builder/__tests__/ChatboxBuilderView.test.tsxmcpjam-inspector/client/src/components/chatboxes/builder/__tests__/setup-checklist-panel.test.tsxmcpjam-inspector/client/src/components/chatboxes/builder/setup-checklist-panel.tsx
Comment on lines
+829
to
+854
| const emailsToInvite = | ||
| draftChatboxConfig.mode === "invited_only" | ||
| ? stagedAccessInviteEmails | ||
| : []; | ||
|
|
||
| toast.success("Chatbox created"); | ||
|
|
||
| let hadInviteFailure = false; | ||
| for (const email of emailsToInvite) { | ||
| try { | ||
| await upsertChatboxMember({ | ||
| chatboxId: created.chatboxId, | ||
| email, | ||
| sendInviteEmail: true, | ||
| }); | ||
| } catch (error) { | ||
| hadInviteFailure = true; | ||
| toast.error( | ||
| error instanceof Error | ||
| ? error.message | ||
| : `Failed to invite ${email}` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| setStagedAccessInviteEmails([]); |
Contributor
There was a problem hiding this comment.
Failed invites vanish from the staged list with no retry breadcrumb.
setStagedAccessInviteEmails([]) clears the list unconditionally, so a user whose tester@example.com invite rejects only sees a transient toast.error — the email isn't re-surfaced anywhere. Since the post-save UI swaps to ChatboxShareSection (saved-chatbox path), they have to recall the failures from memory. Consider keeping failed emails staged, or feeding them back to the saved-chatbox invite panel.
🛠 Sketch: preserve failures
- let hadInviteFailure = false;
+ const failedEmails: string[] = [];
for (const email of emailsToInvite) {
try {
await upsertChatboxMember({
chatboxId: created.chatboxId,
email,
sendInviteEmail: true,
});
} catch (error) {
- hadInviteFailure = true;
+ failedEmails.push(email);
toast.error(
error instanceof Error
? error.message
: `Failed to invite ${email}`
);
}
}
- setStagedAccessInviteEmails([]);
+ setStagedAccessInviteEmails(failedEmails);
+ const hadInviteFailure = failedEmails.length > 0;📝 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 emailsToInvite = | |
| draftChatboxConfig.mode === "invited_only" | |
| ? stagedAccessInviteEmails | |
| : []; | |
| toast.success("Chatbox created"); | |
| let hadInviteFailure = false; | |
| for (const email of emailsToInvite) { | |
| try { | |
| await upsertChatboxMember({ | |
| chatboxId: created.chatboxId, | |
| email, | |
| sendInviteEmail: true, | |
| }); | |
| } catch (error) { | |
| hadInviteFailure = true; | |
| toast.error( | |
| error instanceof Error | |
| ? error.message | |
| : `Failed to invite ${email}` | |
| ); | |
| } | |
| } | |
| setStagedAccessInviteEmails([]); | |
| const emailsToInvite = | |
| draftChatboxConfig.mode === "invited_only" | |
| ? stagedAccessInviteEmails | |
| : []; | |
| toast.success("Chatbox created"); | |
| const failedEmails: string[] = []; | |
| for (const email of emailsToInvite) { | |
| try { | |
| await upsertChatboxMember({ | |
| chatboxId: created.chatboxId, | |
| email, | |
| sendInviteEmail: true, | |
| }); | |
| } catch (error) { | |
| failedEmails.push(email); | |
| toast.error( | |
| error instanceof Error | |
| ? error.message | |
| : `Failed to invite ${email}` | |
| ); | |
| } | |
| } | |
| setStagedAccessInviteEmails(failedEmails); | |
| const hadInviteFailure = failedEmails.length > 0; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/components/chatboxes/builder/ChatboxBuilderView.tsx`
around lines 829 - 854, The code unconditionally calls
setStagedAccessInviteEmails([]) after attempting invites, which discards
addresses that failed and leaves no retry breadcrumb; change the logic around
emailsToInvite/upsertChatboxMember to collect failed emails (e.g., failedEmails
array when catching in the for loop) and only clear stagedAccessInviteEmails for
emails that succeeded (or if no failures occurred), by calling
setStagedAccessInviteEmails(failedEmails) when there are failures; ensure you
reference and use stagedAccessInviteEmails, emailsToInvite, upsertChatboxMember,
created.chatboxId and toast.error so failed addresses remain staged (or are fed
back into the saved-chatbox invite panel) while successful invites are removed.
Comment on lines
+806
to
+851
| <div className="flex flex-1 items-center rounded-md border border-input focus-within:ring-2 focus-within:ring-ring focus-within:ring-offset-2"> | ||
| <Input | ||
| id="access-invite-email" | ||
| type="email" | ||
| autoComplete="email" | ||
| placeholder="colleague@company.com" | ||
| disabled={!inviteChatboxMember} | ||
| value={accessInviteEmail} | ||
| placeholder="Add people, emails..." | ||
| disabled={accessInviteBusy} | ||
| value={inviteInputEmail} | ||
| onChange={(event) => | ||
| setAccessInviteEmail(event.target.value) | ||
| setInviteInputEmail(event.target.value) | ||
| } | ||
| onKeyDown={(event) => { | ||
| if (event.key === "Enter") { | ||
| event.preventDefault(); | ||
| void handleAccessInvite(); | ||
| } | ||
| }} | ||
| /> | ||
| <Button | ||
| type="button" | ||
| className="shrink-0" | ||
| disabled={ | ||
| !inviteChatboxMember || | ||
| !accessInviteEmail.trim() || | ||
| accessInviteBusy | ||
| aria-invalid={ | ||
| emailValidationError ? true : undefined | ||
| } | ||
| onClick={() => void handleAccessInvite()} | ||
| > | ||
| {accessInviteBusy && | ||
| accessInviteEmail.trim() ? ( | ||
| <Loader2 className="size-4 animate-spin" /> | ||
| ) : ( | ||
| "Invite" | ||
| )} | ||
| </Button> | ||
| className="flex-1 border-0 focus-visible:ring-0 focus-visible:ring-offset-0" | ||
| /> | ||
| </div> | ||
| {!inviteChatboxMember ? ( | ||
| <p className="text-xs text-muted-foreground"> | ||
| Save the chatbox to invite people by email. | ||
| </p> | ||
| ) : null} | ||
| <Button | ||
| type="button" | ||
| className="shrink-0" | ||
| disabled={ | ||
| !normalizedInviteInputEmail || | ||
| !!emailValidationError || | ||
| accessInviteBusy | ||
| } | ||
| onClick={() => void handleAccessInvite()} | ||
| > | ||
| {accessInviteBusy && | ||
| normalizedInviteInputEmail ? ( | ||
| <Loader2 className="size-4 animate-spin" /> | ||
| ) : ( | ||
| "Invite" | ||
| )} | ||
| </Button> | ||
| </div> | ||
| {emailValidationError ? ( | ||
| <p className="text-sm text-destructive"> | ||
| {emailValidationError} | ||
| </p> | ||
| ) : null} |
Contributor
There was a problem hiding this comment.
Associate the validation error with the input for assistive tech.
aria-invalid is wired, but the error paragraph at Line 848 isn't programmatically linked to the input, so screen readers announce the field as invalid without revealing why. Give the <p> an id and reference it via aria-describedby when the error is present.
🛠 Proposed fix
<Input
id="access-invite-email"
type="email"
autoComplete="email"
placeholder="Add people, emails..."
disabled={accessInviteBusy}
value={inviteInputEmail}
onChange={(event) =>
setInviteInputEmail(event.target.value)
}
onKeyDown={(event) => {
if (event.key === "Enter") {
event.preventDefault();
void handleAccessInvite();
}
}}
aria-invalid={
emailValidationError ? true : undefined
}
+ aria-describedby={
+ emailValidationError
+ ? "access-invite-email-error"
+ : undefined
+ }
className="flex-1 border-0 focus-visible:ring-0 focus-visible:ring-offset-0"
/>
</div>
@@
{emailValidationError ? (
- <p className="text-sm text-destructive">
+ <p
+ id="access-invite-email-error"
+ className="text-sm text-destructive"
+ >
{emailValidationError}
</p>
) : null}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/components/chatboxes/builder/setup-checklist-panel.tsx`
around lines 806 - 851, The validation message paragraph for
emailValidationError is not programmatically linked to the Input, so add a
stable id to the error <p> (e.g. "access-invite-email-error") and update the
Input (id "access-invite-email") to include aria-describedby that references
that id only when emailValidationError is present; ensure the referenced symbol
names are emailValidationError, inviteInputEmail/normalizedInviteInputEmail,
accessInviteBusy and the handler handleAccessInvite so the enabled/disabled and
error states remain consistent.
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.
Lets users add invite emails while creating a new invite-only chatbox.
The invites are shown as pending and are only sent when the chatbox is created.