IBX-12091: Added TypeScript translation extraction support#1967
IBX-12091: Added TypeScript translation extraction support#1967mikadamczyk wants to merge 6 commits into
Conversation
5198656 to
8b44786
Compare
26d6cc2 to
8b9bbb0
Compare
|
| use Symfony\Component\Process\InputStream; | ||
| use Symfony\Component\Process\Process; | ||
|
|
||
| final class TypeScriptExtractorClient |
There was a problem hiding this comment.
| final class TypeScriptExtractorClient | |
| final readonly class TypeScriptExtractorClient |
|
|
||
| final class TypeScriptExtractorClient | ||
| { | ||
| private const DEFAULT_RESPONSE_TIMEOUT_SECONDS = 30.0; |
There was a problem hiding this comment.
| private const DEFAULT_RESPONSE_TIMEOUT_SECONDS = 30.0; | |
| private const float DEFAULT_RESPONSE_TIMEOUT_SECONDS = 30.0; |
| $this->parserScriptPath = $parserScriptPath ?? __DIR__ . '/typescript_translation_extractor.mjs'; | ||
| } | ||
|
|
||
| public function __destruct() |
There was a problem hiding this comment.
Can this have a return type?
| use SplFileInfo; | ||
| use Twig\Node\Node as TwigNode; | ||
|
|
||
| final class TypeScriptFileVisitor implements FileVisitorInterface, LoggerAwareInterface |
There was a problem hiding this comment.
| final class TypeScriptFileVisitor implements FileVisitorInterface, LoggerAwareInterface | |
| final readonly class TypeScriptFileVisitor implements FileVisitorInterface, LoggerAwareInterface |
| * @param array<mixed> $ast | ||
| */ | ||
| public function visitPhpFile(SplFileInfo $file, MessageCatalogue $catalogue, array $ast): void | ||
| { |
There was a problem hiding this comment.
Should we provide a comment why it is empty? 😉 Same goes for the Twig below.
|
|
||
| public function testServeModeProcessesMultipleFilesSequentiallyAndSurvivesPerFileErrors(): void | ||
| { | ||
| $goodFile = tempnam(sys_get_temp_dir(), 'ts_extractor_') . '.ts'; |
There was a problem hiding this comment.
tempnam() creates the file immediately. Appending .ts only modifies the returned path, so file_put_contents() creates a second file and the original temporary file is never removed. Could we rename the file instead, as done in testExtractsExpectedMessages()? The same applies to the other tempnam(...).'.ts' occurrences.
|
|
||
| foreach ($warnings as $warning) { | ||
| if (is_string($warning)) { | ||
| $this->logger?->error($warning); |
There was a problem hiding this comment.
Nitpick but shouldn't it be warning per method's name and variables' names?
| $extractionResponse['error'], | ||
| )); | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Would it make sense to narrow down the return type to an empty array since the method's name implies messages? In this case we won't need check from line 40.
alongosz
left a comment
There was a problem hiding this comment.
SonarQube analysis is relevant to the changes and should be addressed. It doesn't mean it's correct. In some cases (maybe even most) it's false-positive, but then it needs to be marked as such, so we don't miss a relevant one.
For that reason putting this in "request changes" state.
Some of the issues might require Front-end Team input.
Summary of that + my own findings:
| satis-network-key: ${{ secrets.SATIS_NETWORK_KEY }} | ||
| satis-network-token: ${{ secrets.SATIS_NETWORK_TOKEN }} | ||
|
|
||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
outdated action version
| - uses: actions/setup-node@v4 | |
| - uses: actions/setup-node@v7 |
| satis-network-key: ${{ secrets.SATIS_NETWORK_KEY }} | ||
| satis-network-token: ${{ secrets.SATIS_NETWORK_TOKEN }} | ||
|
|
||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
| - uses: actions/setup-node@v4 | |
| - uses: actions/setup-node@v7 |
| satis-network-key: ${{ secrets.SATIS_NETWORK_KEY }} | ||
| satis-network-token: ${{ secrets.SATIS_NETWORK_TOKEN }} | ||
|
|
||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
| - uses: actions/setup-node@v4 | |
| - uses: actions/setup-node@v7 |
| node-version: '24' | ||
|
|
||
| - name: Install frontend dependencies | ||
| run: yarn install |
There was a problem hiding this comment.
SonarQube:
Lifecycle scripts are enabled by default in Yarn v2+.
See more on https://sonarcloud.io/project/issues?id=ibexa_admin-ui&issues=AZ9_yweGtPeG6EAUOR9u&open=AZ9_yweGtPeG6EAUOR9u&pullRequest=1967
This looks relevant to the case, but if it's false positive, please mark it as false positive. I'm not able to assess this one on my own.
(you can use GitHub to log in to Sonar, so you can access full report, lmk if you have questions)
| }; | ||
|
|
||
| const extractFromFile = (filePath) => { | ||
| const source = fs.readFileSync(filePath, 'utf8'); |
There was a problem hiding this comment.
SonarQube:
LLMs running this code with faulty CLI arguments can escape file system restrictions. Refactor this code to validate the constructed path before accessing the file system.
This is an interesting one, I think we might LLM assessment on that :D
| namespace Ibexa\AdminUi\Translation\Extractor; | ||
|
|
||
| use JsonException; | ||
| use RuntimeException; |
There was a problem hiding this comment.
If this is run in CLI, then one of Symfony\Console\ exceptions should be used.
Comes from SonarQube analysis, but I find this valid.
| { | ||
| if ($this->isRuntimeReady) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Architecture: is there a better way of checking readiness? For e.g., existence of some file?
- This creates stateful service, which tends to cause problems and is not reliable really, as it lives in the current process memory only
- it doesn't check if the runtime did its job.
I don't have a solution, because I don't know what this runtime does, but maybe it's worth looking into.
| { | ||
| } |
There was a problem hiding this comment.
Add a nested comment explaining why this method is empty, throw an Exception or complete the implementation.
Not just SonarQube idea, we've been actually practicing this elsewhere, if spotted.
| public function visitTwigFile(SplFileInfo $file, MessageCatalogue $catalogue, TwigNode $ast): void | ||
| { | ||
| } |


Description:
This PR adds TypeScript translation extraction support for
Translator.trans()andTranslator.transChoice()calls, including translation descriptions from@Desccomments. The extractor uses a persistent Node.js process with runtime validation, response timeouts, error handling, and automatic restart after failures. The implementation separates the JMS visitor from process management and installs frontend dependencies in the backend CI job so the tests can run correctly.For QA:
Documentation: