-
Notifications
You must be signed in to change notification settings - Fork 63
fix: Disable AI Assistant when Apollo not configured #4392
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
Conversation
Fixes #4354 When Apollo environment variables (APOLLO_ENDPOINT and AI_ASSISTANT_API_KEY) are not configured, the AI Assistant button is now disabled with a clear tooltip message. This prevents users from attempting to use a feature that cannot function, avoiding silent failures. Changes: - Thread aiAssistantEnabled prop from LiveView through component chain - Compose disabled state in Header (Apollo unavailable + pinned version) - Show tooltip: "Your instance does not have build-time AI enabled" - Disable Cmd+K keyboard shortcut when Apollo unavailable
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4392 +/- ##
==========================================
- Coverage 89.39% 89.37% -0.02%
==========================================
Files 425 425
Lines 20051 20052 +1
==========================================
- Hits 17924 17922 -2
- Misses 2127 2130 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Changes default from true to false to ensure AI Assistant is disabled
by default when the prop isn't explicitly set. This prevents accidental
exposure of AI features when Apollo isn't configured.
Also extracts disabled message logic to a const for better readability
and makes the prop required in intermediate components to ensure
explicit prop threading.
Tests updated to only specify aiAssistantEnabled={true} where they
specifically test AI button behavior, rather than on every test.
elias-ba
left a comment
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.
Hey @lmac-1 this is looking great to me, thanks a lot. Nice job 👏🏽. I left few comments they are details but I think they may be important considerations for UX.
assets/js/collaborative-editor/components/AIAssistantPanelWrapper.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| // Determine AI button disabled message based on priority | ||
| const aiButtonDisabledMessage = !aiAssistantEnabled | ||
| ? 'Your instance does not have build-time AI enabled. Contact your administrator or support@openfn.org to configure it.' |
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.
Why do we use the expression "build-time AI" here ? Is that clear to the end user ? I know that expression because I have heard it being used internally multiple times. Is it the case for the end user in Lesotho using this app ? Wouldn't they stay minutes thinking about that term and asking themselve what does this mean ? Why not just use the term "AI Assistant" ? cc @taylordowns2000
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.
@taylordowns2000 I used the message you mentioned on Slack but let me know if it should be changed. I agree that "AI assistant" could be a bit clearer for the user.
This error message will trigger when the env variables aren't set up for AI assistant - can someone also confirm that's the scenario Roina was reporting (with local builds, etc.)?
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.
@theroinaochieng we just need clarification / confirmation of this message. If it's okay we can approve and merge, if not I can make the update.
The tests were using Object.defineProperty to mock window.location, which doesn't work with the useURLState hook. Use the existing urlState mock pattern from other tests instead.
The AIAssistantPanelWrapper component is always called with props,
so the = {} default parameter is no longer needed.
theroinaochieng
left a comment
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.
Looks great Lucy!
Problem
AI Assistant button was clickable when Apollo wasn't configured, causing silent failures where messages sent but never received responses.
Solution
APOLLO_ENDPOINTorAI_ASSISTANT_API_KEYare missingCloses #4354
Validation steps
APOLLO_ENDPOINTorAI_ASSISTANT_API_KEYenvironment variables and confirm that AI assistant button is disabled. There should be a tooltip text advising the user. Make sure you can't open panel either with Cmd/Ctrl + KAdditional notes for the reviewer
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)