-
Notifications
You must be signed in to change notification settings - Fork 16
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
Studio: Add prompt limit UI #197
Conversation
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.
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.
@kozer , the happy path works great.
We need to fix the unit tests.
I'm seeing this error Error: Uncaught [TypeError: Cannot read properties of undefined (reading 'assistantEnabled')]
in src/components/tests/user-settings.test.tsx
I suggested using the headers from fetchAssistant
to update the current usage, but its not a strong opinion.
One last thing I noticed is that when starting the app offline I see the count of my demo sites but the count of my prompts goes to zero. Would be possible to keep that number for consistency, or refetch it when I'm back online?
If yo prefer to handle that edge case in a separate PR I can create a new issue.
Happy Path:
8vWz2G.mp4
Steps to reproduce Offline edge case:
- Disconnect your wifi/internet
- Run the app
- Open the settings modal by clicking on your user icon
- Observe your prompts indicate 0 when it should be some number.
Thank you for your comments!
Yes, let's create another issue to handle that. Thanks! |
@sejas , I updated the PR to use assistant API to fetch the headers, fixed tests, and did a minor refactor! Thanks for your suggestions! cc: @katinthehatsite |
Hi @kozer - thanks for making updates! The changes look good on my end. I tested for this scenario: 1.Disconnect your wifi/internet And after updates, I can see that the number goes back up once I start the app back after being offline which is great. Would it be possible to keep it there even when offline? (At the moment, it goes to 0 when offline) Otherwise, everything looks 👍 |
As discussed above, in Antonio's comment, I'll tackle this in a different pr ( the offline stuff ). |
Sounds good @kozer ! I approved the PR ✅ Nice work!! |
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.
The PR looks great. I tested it I was able to see my usage updated. I noticed that the limit was restored to 0 from yesterday.
My only concern is that we are changing the layout for current users even if we didn't released our feature flag yet.
@wojtekn , what do you think about submitting this change that affects current users?
I think this specific change is harmless, but I understand if we prefer to keep the old demo site limit bar intact.
Before |
---|
After |
---|
@@ -7,6 +7,7 @@ import { useAssistant } from '../hooks/use-assistant'; | |||
import { useAssistantApi } from '../hooks/use-assistant-api'; | |||
import { useAuth } from '../hooks/use-auth'; | |||
import { useOffline } from '../hooks/use-offline'; | |||
import { usePromptUsage } from '../hooks/use-prompt-usage'; |
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.
Let's remove this unused import.
@@ -72,13 +74,11 @@ const SnapshotInfo = ( { | |||
const isOffline = useOffline(); | |||
const offlineMessage = __( 'Deleting demo sites requires an internet connection.' ); | |||
return ( | |||
<div className="flex gap-5 flex-col"> | |||
<h2 className="a8c-subtitle-small">{ __( 'Usage' ) }</h2> |
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.
We shouldn't change the old behaviour until we release the feature flag.
We are changing the old behaviour but that's ok in this case.
src/hooks/use-prompt-usage.tsx
Outdated
await new Promise( ( resolve, reject ) => { | ||
client.req.get( | ||
{ | ||
method: 'HEAD', | ||
path: '/studio-app/ai-assistant/chat', | ||
apiNamespace: 'wpcom/v2', | ||
}, | ||
( error: Error, _data: unknown, headers: Record< string, string > ) => { | ||
if ( error ) { | ||
return reject( error ); | ||
} | ||
if ( ! headers ) { | ||
reject( new Error( 'No headers in response' ) ); | ||
return; | ||
} | ||
updatePromptUsage( headers ); | ||
resolve( headers ); | ||
} | ||
); | ||
} ); |
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.
Do we need to create this promise and resolve the headers? Maybe we can await the client.req.get
directly.
await new Promise( ( resolve, reject ) => { | |
client.req.get( | |
{ | |
method: 'HEAD', | |
path: '/studio-app/ai-assistant/chat', | |
apiNamespace: 'wpcom/v2', | |
}, | |
( error: Error, _data: unknown, headers: Record< string, string > ) => { | |
if ( error ) { | |
return reject( error ); | |
} | |
if ( ! headers ) { | |
reject( new Error( 'No headers in response' ) ); | |
return; | |
} | |
updatePromptUsage( headers ); | |
resolve( headers ); | |
} | |
); | |
} ); | |
await client.req.get( | |
{ | |
method: 'HEAD', | |
path: '/studio-app/ai-assistant/chat', | |
apiNamespace: 'wpcom/v2', | |
}, | |
( error: Error, _data: unknown, headers: Record< string, string > ) => { | |
if ( error ) { | |
return; | |
} | |
if ( ! headers ) { | |
return; | |
} | |
updatePromptUsage( headers ); | |
} | |
); |
} finally { | ||
setIsLoading( false ); | ||
} | ||
const message = response?.choices?.[ 0 ]?.message?.content; | ||
|
||
updatePromptUsage( headers ); |
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.
Thanks for optimizing this ! 🙏
It's okay. We are slightly changing the layout to prepare it for incoming changes, but we are not enabling AI assistant UI in any form. |
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.
🚀
@kozer , feel free to merge after adressing:
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.
Fixes https://github.com/Automattic/dotcom-forge/issues/7409
Proposed Changes
This diff, adds a prompt limit UI into user settings
Testing Instructions
nvim use && STUDIO_AI=true npm start
Pre-merge Checklist