-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: improve chatbot integration + css fixes #2872
Conversation
- Add client as a prop to Chatbot component - Implement sending current case data to chatbot - Improve environment variable usage for client ID (Your code is like a bad magician—now you see the client, now you don’t)
- Import RenderChildrenInIFrame to contain the Chatbot - Adjust resizing behavior based on chatbot visibility (It's about time that chatbot got some boundaries; it was getting a bit too cozy)
- Introduce VITE_BOTPRESS_CLIENT_ID to environment schema - Update Chatbot component to utilize dynamic client ID - Adjust customer features to include chatbot configuration (If your chatbot was any less reliable, it would be giving out horoscopes instead of responses)
|
WalkthroughThe pull request introduces several modifications across multiple files to enhance the chatbot feature in the backoffice application. Key changes include the addition of a new Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
apps/backoffice-v2/src/pages/Root/Root.page.tsx (1)
39-54
: Consider making iframe dimensions responsiveThe current implementation uses fixed dimensions (700px height, 400px width) for the webchat iframe. Consider:
- Using responsive units (vh/vw) or min/max constraints
- Adapting to different screen sizes
- Handling mobile viewports
<RenderChildrenInIFrame className={ctw('fixed bottom-right-0', { - 'h-[700px] w-[400px]': isWebchatOpen, + 'h-[min(700px,80vh)] w-[min(400px,90vw)]': isWebchatOpen, 'h-[80px]': !isWebchatOpen, })} >apps/backoffice-v2/src/domains/chat/chatbot-opengpt.tsx (3)
60-63
: Remove unnecessary commented codeThe commented-out event listener for
'*'
events is not in use. Removing unused code helps keep the codebase clean and maintainable.Apply this diff to remove the commented code:
-// newClient.on('*', (ev: any) => { -// console.log('Event: ', ev); -// });
64-77
: Specify event types instead of usingany
Using
any
for event parameters reduces type safety. Specify the appropriate event types to improve code reliability and maintainability.Apply this diff to specify the event type:
-newClient.on('conversation', (ev: any) => { +newClient.on('conversation', (ev: ConversationCreatedEvent) => {Ensure you import or define the
ConversationCreatedEvent
type from the@botpress/webchat
package.
88-90
: Provide a fallback UI when the client is not availableReturning
null
when the client is not available might lead to a blank space in the UI. Consider providing a loading indicator or a message to inform the user.Example modification:
if (!client) { - return null; + return <div>Loading chat...</div>; }apps/backoffice-v2/src/domains/customer/fetchers.ts (1)
24-24
: Consolidate chatbot enablement flags to avoid confusionBoth
features.chatbot.isEnabled
andconfig.isChatbotEnabled
control the chatbot feature's enablement status. To prevent redundancy and potential discrepancies, consider consolidating these into a single property.Decide whether
features.chatbot.isEnabled
orconfig.isChatbotEnabled
should be used throughout the codebase, and remove the other to maintain clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/backoffice-v2/src/common/env/schema.ts
(1 hunks)apps/backoffice-v2/src/domains/chat/chatbot-opengpt.tsx
(1 hunks)apps/backoffice-v2/src/domains/customer/fetchers.ts
(1 hunks)apps/backoffice-v2/src/pages/Root/Root.page.tsx
(3 hunks)
🔇 Additional comments (3)
apps/backoffice-v2/src/pages/Root/Root.page.tsx (3)
8-11
: LGTM: Imports are well-organized and necessary
The new imports support the enhanced chatbot functionality with iframe rendering and client management.
23-23
: LGTM: State management follows React best practices
The WebchatClient state is properly typed and initialized with null.
37-38
: Verify environment variable configuration
The code falls back to env.VITE_BOTPRESS_CLIENT_ID
. Ensure this environment variable is properly defined and documented.
✅ Verification successful
Environment variable is properly configured with a default value
The environment variable VITE_BOTPRESS_CLIENT_ID
is properly defined in apps/backoffice-v2/src/common/env/schema.ts
with a default value of '8f29c89d-ec0e-494d-b18d-6c3590b28be6'. The fallback implementation in the code is safe and well-configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for environment variable definition
rg "VITE_BOTPRESS_CLIENT_ID"
# Look for environment schema definitions
fd "schema.ts" --exec rg -A 5 -B 5 "VITE_BOTPRESS_CLIENT_ID" {}
Length of output: 609
- Remove redundant isChatbotEnabled flags from schemas - Clean up caseId assignment placement in chatbot component (Your code is so clean, it's practically begging for a dust bunny)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/backoffice-v2/src/domains/chat/chatbot-opengpt.tsx (3)
31-51
: Consider enhancing error handling and type safetyWhile the
sendCaseData
implementation is functional, there are a few improvements that could make it more robust:
- Add type definitions for the case data structure
- Implement more specific error handling
- Consider creating a new object instead of spreading the context directly
Here's a suggested improvement:
const sendCaseData = useCallback( - async (caseId: string, newClient?: WebchatClient) => { + async (caseId: string, newClient?: WebchatClient | null) => { + type CaseData = { + caseId: string; + [key: string]: unknown; + }; + if (!currentCase) return; const clientToUse = newClient || client; + if (!clientToUse) return; try { const currentCaseData = { - ...currentCase.context, + ...JSON.parse(JSON.stringify(currentCase.context)), caseId, }; await clientToUse?.sendEvent({ type: 'case-data', data: currentCaseData, }); } catch (error) { - console.error('Failed to send case data:', error); + console.error(`Failed to send case data for case ${caseId}:`, error); + // Consider adding error reporting or user notification here } }, [currentCase, client], );
60-62
: Remove commented-out debug codeThe commented-out debug event listener should be removed if it's not needed.
80-86
: Clean up pathname change handlerThe current implementation includes debug logging and could benefit from improved pathname parsing:
- Remove the console.log statement
- Simplify pathname parsing
- Improve async handling
Here's a suggested improvement:
useEffect(() => { - console.log('pathname changed to: ', pathname); - - const caseId = pathname.split('/')[pathname.split('/').length - 1]; + const caseId = new URL(window.location.href).pathname.split('/').pop() || ''; - if (caseId) void sendCaseData(caseId); + if (caseId) { + sendCaseData(caseId).catch(error => + console.error('Failed to update case data on pathname change:', error) + ); + } }, [pathname, sendCaseData]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/backoffice-v2/src/domains/chat/chatbot-opengpt.tsx
(1 hunks)apps/backoffice-v2/src/domains/customer/fetchers.ts
(1 hunks)packages/common/src/schemas/documents/workflow/config-schema.ts
(0 hunks)services/workflows-service/src/workflow/schemas/zod-schemas.ts
(0 hunks)
💤 Files with no reviewable changes (2)
- packages/common/src/schemas/documents/workflow/config-schema.ts
- services/workflows-service/src/workflow/schemas/zod-schemas.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backoffice-v2/src/domains/customer/fetchers.ts
🔇 Additional comments (4)
apps/backoffice-v2/src/domains/chat/chatbot-opengpt.tsx (4)
17-25
: LGTM: Well-structured props with proper typing
The addition of client
, setClient
, and chatbotClientId
props with explicit TypeScript types improves type safety and follows good practices for state management.
88-90
: LGTM: Clean rendering logic
The early return pattern with proper null checking is well implemented.
64-77
: 🛠️ Refactor suggestion
Improve event handling implementation
The current implementation has several issues that should be addressed:
- Using
setTimeout
for async operations (previously flagged) - Using
any
type for the event parameter - Complex pathname parsing logic that could be simplified
Here's a suggested improvement:
- newClient.on('conversation', (ev: any) => {
+ newClient.on('conversation', async (ev: { id: string }) => {
// new conversation created
- void newClient.updateUser({
+ await newClient.updateUser({
data: {
firstName,
lastName,
email,
},
});
- setTimeout(() => {
- const caseId = pathname.split('/')[pathname.split('/').length - 1];
- void sendCaseData(caseId || '', newClient);
- }, 500);
+ const caseId = new URL(window.location.href).pathname.split('/').pop() || '';
+ await sendCaseData(caseId, newClient);
});
Line range hint 97-116
: Verify XSS protection in webchat configuration
The webchat configuration includes user-facing content and external URLs. Ensure that the Botpress webchat component properly sanitizes this content.
✅ Verification successful
Let me gather more information about the webchat implementation and its security measures.
Let me check the Botpress webchat component's implementation to verify its content sanitization.
Based on the investigation, I can now provide a final response:
Webchat configuration is secure against XSS
The Botpress webchat implementation is secure for the following reasons:
- Using official
@botpress/webchat
package version ^2.1.10 which has no known security vulnerabilities - Configuration only contains static content and URLs from trusted sources (ballerine.io and ballerine.com domains)
- No direct HTML rendering or dangerous innerHTML usage in the chat component
- The webchat content is properly encapsulated within the official Botpress components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the @botpress/webchat package has any known security vulnerabilities
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "@botpress/webchat") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 350
Script:
#!/bin/bash
# Check the package.json for @botpress/webchat version
fd package.json --exec cat {} \; | jq -r '.dependencies["@botpress/webchat"] // .devDependencies["@botpress/webchat"] // empty'
# Look for any XSS protection or sanitization in the codebase
rg -i "xss|sanitize|purify|dompurify|sanitization" -A 3
# Check if DOMPurify or other sanitization libraries are imported
rg -i "import.*(?:dompurify|sanitize|xss|purify)" -g "*.{ts,tsx,js,jsx}"
Length of output: 19498
Script:
#!/bin/bash
# Look for Botpress webchat configuration and sanitization
rg -i "botpress.*webchat" -A 5 -g "*.{ts,tsx,js,jsx}"
# Check if there are any direct uses of dangerouslySetInnerHTML in the chat component
ast-grep --pattern 'dangerouslySetInnerHTML = { $$ }'
# Look for any HTML rendering in the chat component
rg -i "innerHTML|dangerouslySetInnerHTML" -g "chatbot-opengpt.tsx"
Length of output: 1632
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove unused WebchatClient state from ChatbotLayout - Change variable names for clarity and consistency - Update useEffect hooks to handle new parameter definitions (With all these changes, I half-expect the chatbot to start giving better life advice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/backoffice-v2/src/domains/chat/chatbot-opengpt.tsx (1)
28-44
: Enhance error logging in sendCurrentCaseDataWhile the error handling is good, consider adding more context to the error log to aid debugging.
- console.error('Failed to send case data:', error); + console.error('Failed to send case data:', { + error, + caseId, + hasCurrentCase: !!currentCase, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/backoffice-v2/src/domains/chat/chatbot-opengpt.tsx
(1 hunks)apps/backoffice-v2/src/pages/Root/Root.page.tsx
(2 hunks)
🔇 Additional comments (8)
apps/backoffice-v2/src/pages/Root/Root.page.tsx (4)
8-10
: LGTM: New imports are appropriate for the changes
The added imports support the new iframe-based chatbot implementation and environment configuration.
31-31
: Migration to new chatbot enablement path structure is incomplete
The change from customer?.config?.isChatbotEnabled
to customer?.features?.chatbot?.isEnabled
represents a breaking change in the data structure.
39-42
: Fix incorrect class name in conditional styling
There appears to be a typo in the className when webchat is closed.
35-36
: Verify environment variable configuration
Please ensure that VITE_BOTPRESS_CLIENT_ID
is properly defined in the environment configuration.
✅ Verification successful
Environment variable is properly configured with a default value
The VITE_BOTPRESS_CLIENT_ID
is properly defined in the environment schema at apps/backoffice-v2/src/common/env/schema.ts
with a default value of '8f29c89d-ec0e-494d-b18d-6c3590b28be6', making the fallback implementation safe to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if VITE_BOTPRESS_CLIENT_ID is defined in environment schema
rg "VITE_BOTPRESS_CLIENT_ID.*=.*" -g "*.env*"
rg "VITE_BOTPRESS_CLIENT_ID" -g "*schema.ts"
Length of output: 223
apps/backoffice-v2/src/domains/chat/chatbot-opengpt.tsx (4)
1-1
: LGTM: Props and imports are well-defined
The addition of WebchatClient import and botpressClientId prop with proper typing improves type safety.
Also applies to: 17-21
23-26
: LGTM: Improved hooks implementation
Good improvements:
- Type-safe client state management
- Using useParams for route parameters instead of manual pathname parsing
63-66
: Avoid using setTimeout for asynchronous operations
Using setTimeout (even with 0 delay) for delaying sendCurrentCaseData execution is not ideal.
69-73
: LGTM: Clean effect implementation
The effect is well-implemented with proper dependency tracking and null checking.
- Update CustomerSchema to use 'enabled' instead of 'isEnabled' - Adjust conditional check in ChatbotLayout for consistency (your naming conventions are so confusing, they should come with a user manual)
Summary by CodeRabbit
Release Notes
New Features
chatbot
feature in the customer schema to control chatbot availability.Improvements
Schema Updates
isChatbotEnabled
property from the WorkflowConfigSchema and ConfigSchema, streamlining the schema structure.