Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Studio: Translate Studio Assistant Welcome messages #483

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Studio: Translate Studio Assistant Welcome messages #483

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e63c600
Change parameter handling
Aug 20, 2024
24d8e38
Remove locale parameter from welcome endpoint
Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/hooks/use-welcome-messages.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
useCallback,
useContext,
} from 'react';
import { getAppGlobals } from '../lib/app-globals';
import { useAuth } from './use-auth';
import { useOffline } from './use-offline';
import { useWindowListener } from './use-window-listener';
Expand All @@ -31,7 +30,6 @@ const WelcomeMessagesContext = createContext< WelcomeMessagesContext >( {
export const WelcomeMessagesProvider = ( { children }: { children: React.ReactNode } ) => {
const { client } = useAuth();
const isOffline = useOffline();
const locale = getAppGlobals().locale;
const [ messages, setMessages ] = useState< string[] >( [] );
const [ examplePrompts, setExamplePrompts ] = useState< string[] >( [] );
const isFetchingMessages = useRef( false );
Expand All @@ -45,9 +43,7 @@ export const WelcomeMessagesProvider = ( { children }: { children: React.ReactNo
const response = await client.req.get( {
path: '/studio-app/ai-assistant/welcome',
apiNamespace: 'wpcom/v2',
params: { locale },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with passing the parameter like this is that the request URL gets formatted like so:

wpcom/v2/studio-app/ai-assistant/welcome?_locale=pl with underscore which breaks the request to the API and the translation does not come through

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not adjust the code responsible for that in the custom request() function?

studio/src/components/auth-provider.tsx

Line 123 in 09f7f3d

const localeParamName =

Copy link
Contributor Author

@katinthehatsite katinthehatsite Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @wojtekn ! Interesting, looking at this setting, I see that when you switch to a language other than English, it appends the locale parameter to all requests going to different endpoints. If I remove the locale parameter from the /welcome endpoint, I see it also gets appended:

Captura de pantalla 2024-08-21 a las 9 33 49 a  m

Looking at the original PR where this was introduced - https://github.com/Automattic/local-environment/pull/91 - this seems to have been done so that the locale is set for the duration of the API call.

In regards to this, I have the following question:

  • Why do we need to set a specific locale for all of these API calls? From my understanding, we are not returning anything locale-specific in any of these endpoints beside the /welcome endpoint and even for the /welcome endpoint it does not seem to work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to set a specific locale for all of these API calls? From my understanding, we are not returning anything locale-specific in any of these endpoints beside the /welcome endpoint and even for the /welcome endpoint it does not seem to work?

I would think it's to let API endpoints return localized error messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that makes sense. So if I am testing this correctly, I am seeing the following:

  • When I pass locale into /welcome endpoint, I am getting the translations of the messages but I am not getting localized error messages
  • When I pass _locale, I am getting only the localized error messages and not the translated messages
  • The only way I found to make this work for both is to pass both locale and _locale - example of the URL:
Captura de pantalla 2024-08-21 a las 9 53 00 a  m

I am seeing two options for this case:

  • either we pass both as mentioned above if we want both error messages and the welcome messages
  • OR we pass only locale for this specific endpoint since we don't really display the localized error messages that come from the endpoint for the welcome messages.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: after Slack discussions, we will proceed with the fix of the API endpoint for the welcome messages. I will update here once it is done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wojtekn I prepared a fix here: D158963-code and it needs to be tested in combination with this PR (the testing steps in the PR are updated).

Let me know what you think!

} );

const data = response as WelcomeMessageResponse;
if ( data ) {
setMessages( data.messages );
Expand All @@ -59,7 +55,7 @@ export const WelcomeMessagesProvider = ( { children }: { children: React.ReactNo
} finally {
isFetchingMessages.current = false;
}
}, [ client, isOffline, locale ] );
}, [ client, isOffline ] );

useEffect( () => {
fetchMessages();
Expand Down
Loading