feat(opentrons-ai-server): backend support for settings page #18837
Conversation
Elyorcv
changed the title
| {location.pathname === '/' && ( | ||
| <Box marginLeft={SPACING.spacing16}> | ||
| <SettingsButton onClick={handleSettingsClick} /> | ||
| </Box> | ||
| )} |
There was a problem hiding this comment.
why this change is neccessary?
it feels quite unusual that app settings can only be changed from the landing page.
There was a problem hiding this comment.
Good question! This is mainly because of the current implementation of state management. For example, if you go to the chat page, submit a prompt, navigate to settings, and come back, it restarts the generation process. This is not what we want.
The decision to restrict settings access to the landing page was made to:
- Prevent state loss: Avoid interrupting ongoing API calls or active chat sessions
- Simplify navigation: Users can access settings from a predictable, central location
- Avoid complexity: Prevents issues with navigating away during active operations
Fixing this would require refactoring the frontend state management to properly preserve application context when navigating between routes - a more substantial change that was out of scope for this implementation. This is my understanding.
If you think it is easy to implement let me know.
There was a problem hiding this comment.
@Elyorcv
could you post the screen capture that is what you mentioned to ai-dev cahnnel(For example, if you go to the chat page, submit a prompt, navigate to settings, and come back)?
| headers: { | ||
| ...config?.headers, | ||
| ...analyticsHeaders, | ||
| }, |
There was a problem hiding this comment.
why do you need this?
the data will be sent to Mixpanel from the frontend. what does the backend need to know about that?
There was a problem hiding this comment.
While the frontend handles user interaction analytics with Mixpanel, the backend handles chat analytics with Weave which is why we coordinate them with the x-enable-analytics header.
Your question reminds me to implement disabling mixpanel when toggle is off.
At the moment, when toggle is off, backend stops tracking whereas frontend analytics continue.
| logger.debug(f"Analytics {'enabled' if enable_analytics else 'disabled'} for this request") | ||
|
|
||
| # Initialize Weave on first call (regardless of analytics preference) | ||
| if not _weave_initialized: |
There was a problem hiding this comment.
If weave.init() fails for any reason, this implementation will continue to call it for every request. Is that what you want?
There was a problem hiding this comment.
Good catch. Now added a finally block that sets _weave_initialized = True regardless of success or failure. We would want to initialize only once.
| global _weave_initialized, _analytics_enabled | ||
|
|
||
| # Update analytics preference | ||
| _analytics_enabled = enable_analytics |
There was a problem hiding this comment.
Can you explain how these global variables work? Is there only one _analytics_enabled for the whole server, but each request can choose to set it to True or False? What if you get 2 requests at the same time, and they have differing preferences for enable_analytics?
There was a problem hiding this comment.
Good catch. Concurrency problem occurs! Completely removed the global _analytics_enabled variable and made analytics preference request-scoped.
Elyorcv
force-pushed
the
AUTH-2016-backend-settings-page
branch
from
90e50c4 to
909f1f1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #18837 +/- ##
=======================================
Coverage 26.84% 26.84%
=======================================
Files 3252 3252
Lines 257814 257814
Branches 32609 32609
=======================================
Hits 69223 69223
Misses 188567 188567
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Overview
Closes AUTH-2016
PR made changes to the backend with respect to frontend settings page.
Test Plan and Hands on Testing
CI
Review requests
Make sure everything is working as expected
Risk assessment
Low