Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Oct 9, 2025

What is the purpose of this pull request?

This pull request enhances the translation utilities in src/service/worker/runtime/graphql/utils/translations.ts by introducing a new state marker for translatable message strings, updating the parsing and formatting logic to handle this marker, and ensuring that already translated messages are bypassed further processing. These changes make the translation pipeline more robust and prevent unnecessary re-translation.

Support for message state and translation control:

  • Added support for a new [[[state]]] marker in translatable strings, updating the CONTENT_REGEX and introducing STATE_REGEX to match and extract the state (either 'original' or 'translated'). The TranslatableMessageV2 interface and related parsing/formatting functions were updated to handle this new field.
  • Modified parseTranslatableStringV2 to extract the state from the message and default to 'original' if not specified.
  • Updated formatTranslatableStringV2 to include the [[[state]]] marker when formatting messages, ensuring the state is preserved in the string representation.
  • In handleSingleString, added logic to immediately return the message content if its state is 'translated', preventing already translated messages from being processed again.

What problem is this solving?

How it works today:

The catalog returns the request data. A @translatableV2 notation causes some GraphQL keys to be intercepted and translated.

Where we want to go:

The catalog returns the data that needs to be translated already translated. The @translatableV2 flag is removed, centralizing the translation process in the catalog dataplane indexing.

How we'll do the from-to:

Important points

  • We can't just remove the @translatableV2 notation because we'll be delivering this as a partial rollout, so sometimes the keys will already be translated and sometimes not.
  • Creating another GraphQL schema to have the old and new schemas with and without the notation would require us to work on several different projects for the change to be applied.

This PR adds an additional parameter that marks a label as already translated, causing @vtex-node-api to intercept the label but return only the original text, even if the @translatableV2 flag is active.

How should this be manually tested?

Screenshots or example usage

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Requires change to documentation, which has been updated accordingly.

@ghost ghost requested a review from Copilot October 9, 2025 11:48
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for a new state parameter in translation parsing that can indicate whether content is 'original' or 'translated'. The change extends the existing translation system to handle a third type of directive alongside context and from parameters.

  • Added state field to TranslatableMessageV2 interface with union type 'original' | 'translated'
  • Introduced STATE_REGEX pattern [[[state]]] and updated CONTENT_REGEX to include it
  • Added early return logic to skip translation processing when state is 'translated'

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

content: content?.trim(),
context: context?.trim(),
from: from?.trim(),
state: state?.trim() as 'original' | 'translated' | undefined,
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

The type assertion bypasses TypeScript's type checking. The state?.trim() result could be any string, but you're asserting it's one of the expected literal types. Consider adding runtime validation to ensure the state value is actually 'original' or 'translated' before the type assertion.

Suggested change
state: state?.trim() as 'original' | 'translated' | undefined,
state: (state?.trim() === 'original' || state?.trim() === 'translated')
? (state?.trim() as 'original' | 'translated')
: undefined,

Copilot uses AI. Check for mistakes.

content: content?.trim(),
context: context?.trim(),
from: from?.trim(),
state: state?.trim() as 'original' | 'translated' | undefined,
Copy link
Author

@ghost ghost Oct 9, 2025

Choose a reason for hiding this comment

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

state: (state?.trim() == 'translated' ? 'translated' : 'original') as 'translated' | 'original'

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

content: content?.trim(),
context: context?.trim(),
from: from?.trim(),
state: (state?.trim() === 'translated' ? 'translated' : 'original') as 'translated' | 'original',
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

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

The type assertion is redundant since the ternary operator already ensures the return type matches the union type. Consider simplifying to: state: state?.trim() === 'translated' ? 'translated' : 'original'

Suggested change
state: (state?.trim() === 'translated' ? 'translated' : 'original') as 'translated' | 'original',
state: state?.trim() === 'translated' ? 'translated' : 'original',

Copilot uses AI. Check for mistakes.

export const formatTranslatableStringV2 = ({ from, content, context }: TranslatableMessageV2): string =>
`${content} ${context ? `(((${context})))` : ''} ${from ? `<<<${from}>>>` : ''}`
export const formatTranslatableStringV2 = ({ from, content, context, state }: TranslatableMessageV2): string =>
`${content} ${context ? `(((${context})))` : ''} ${from ? `<<<${from}>>>` : ''} ${state ? `[[[${state}]]]` : ''}`
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

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

The formatting function includes the state marker even when state is 'original', which may add unnecessary markers to strings. Consider only including the state marker when it's 'translated' to keep the output cleaner for the default case.

Suggested change
`${content} ${context ? `(((${context})))` : ''} ${from ? `<<<${from}>>>` : ''} ${state ? `[[[${state}]]]` : ''}`
`${content} ${context ? `(((${context})))` : ''} ${from ? `<<<${from}>>>` : ''} ${state === 'translated' ? `[[[${state}]]]` : ''}`

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant