Upload translations on builds of master
#4002
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jfdoming
force-pushed
the
jfdoming/translation-exports
branch
from
8dd4842 to
2e9a3df
Compare
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/i18n.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces changes related to internationalization (i18n) and project setup across multiple files. The modifications include adding a new GitHub Actions workflow for extracting and uploading i18n strings, updating the setup action to install Yarn globally, modifying the The new GitHub Actions workflow automates the extraction and uploading of i18n strings, triggered on pushes to the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
.github/actions/setup/action.yml (2)
10-13: Consider using built-in yarn installationThe Node.js setup action (
actions/setup-node) supports yarn installation directly through itscacheparameter. Consider replacing the manual yarn installation with the built-in support:- name: Install node uses: actions/setup-node@v4 with: node-version: 18.16.0 - - name: Install yarn - run: npm install -g yarn - shell: bash - if: ${{ env.ACT }} + cache: 'yarn'
Line range hint
1-1: Add description to action metadataThe action is missing a description in its metadata, which is important for documentation and usability.
Add a description field:
name: Setup +description: Sets up Node.js environment with yarn and caching for the project.github/workflows/i18n-string-extract-master.yml (3)
30-35: Improve error handling and loggingThe file existence check is good, but we can enhance the error handling and logging:
- name: Upload i18n strings run: | + echo "Checking for translation file..." if [[ ! -f packages/desktop-client/locale/en.json ]]; then echo "File packages/desktop-client/locale/en.json not found. Ensure the file was generated correctly." exit 1 fi + echo "Validating JSON format..." + if ! jq empty packages/desktop-client/locale/en.json 2>/dev/null; then + echo "Invalid JSON format in en.json" + exit 1 + fi + echo "Uploading translations to Weblate..." wlc upload --author-name "Actual Budget" --author-email "[email protected]" --method add --input packages/desktop-client/locale/en.json + echo "Upload completed successfully"
3-8: Verify workflow triggersThe workflow is correctly triggered on
masterbranch pushes and manual dispatch. However, consider adding:
- Path filters to only trigger on relevant file changes
- Concurrency control to prevent parallel uploads
on: push: branches: - master + paths: + - 'packages/desktop-client/src/**' + - 'packages/**/i18n/**' workflow_dispatch: +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
27-28: Add error handling for i18n generationThe
generate:i18ncommand should have proper error handling.- name: Generate i18n strings - run: yarn generate:i18n + run: | + echo "Generating i18n strings..." + if ! yarn generate:i18n; then + echo "Failed to generate i18n strings" + exit 1 + fipackages/desktop-client/src/i18n.ts (2)
6-6: Consider using a relative path and supporting nested locales.The current glob pattern has two potential issues:
- The absolute path
/locale/*.jsonmight be fragile across different environments.- The pattern doesn't support nested locale structures (e.g.,
locale/en-US/common.json).Consider this improvement:
-const languages = import.meta.glob('/locale/*.json'); +const languages = import.meta.glob('../locale/**/*.json');
11-13: Enhance error handling with available locales informationThe error message could be more helpful by including a list of available locales.
- console.error(`Unknown locale ${language}`); - throw new Error(`Unknown locale ${language}`); + const availableLocales = Object.keys(languages) + .map(path => path.match(/\/([^/]+)\.json$/)?.[1]) + .filter(Boolean); + console.error(`Unknown locale "${language}". Available locales: ${availableLocales.join(', ')}`); + throw new Error(`Unknown locale "${language}". Available locales: ${availableLocales.join(', ')}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4002.mdis excluded by!**/*.md
📒 Files selected for processing (5)
.github/actions/setup/action.yml(1 hunks).github/workflows/i18n-string-extract-master.yml(1 hunks)packages/desktop-client/.gitignore(1 hunks)packages/desktop-client/i18next-parser.config.js(1 hunks)packages/desktop-client/src/i18n.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/desktop-client/.gitignore
- packages/desktop-client/i18next-parser.config.js
🧰 Additional context used
🪛 Biome (1.9.4)
packages/desktop-client/src/i18n.ts
[error] 10-10: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🪛 actionlint (1.7.4)
.github/workflows/i18n-string-extract-master.yml
15-15: description is required in metadata of "Setup" action at "/inmem/22/695974e7-23d0-4a25-91ab-0f3dd4a9e226/home/jailuser/git/.github/actions/setup/action.yml"
(action)
🔇 Additional comments (1)
packages/desktop-client/src/i18n.ts (1)
6-14: Verify Vite compatibility in build environment
The use of import.meta.glob requires Vite support. Let's ensure this is compatible with your build setup.
✅ Verification successful
Vite configuration is properly set up for import.meta.glob usage
The codebase has proper Vite support with:
- Vite v5.2.14 installed as a dev dependency
- A valid vite.config.mts configuration file
- Build script configured to use Vite (
vite build) - Proper module resolution and build configuration that supports dynamic imports
The import.meta.glob usage in i18n.ts is compatible with the build setup, though the locale directory appears to be missing. This is likely because it's generated during the build process or managed elsewhere.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Vite is configured in the project
if [ -f "vite.config.ts" ] || [ -f "vite.config.js" ]; then
echo "✓ Vite configuration found"
cat vite.config.* | grep -A 5 "build"
else
echo "⚠ No Vite configuration found"
# Check package.json for build setup
cat package.json | jq '.scripts.build, .dependencies["vite"], .devDependencies["vite"]'
fi
Length of output: 229
Script:
#!/bin/bash
# Let's check the package.json in the desktop-client directory since that's where the file is located
cat packages/desktop-client/package.json | jq '.scripts.build, .dependencies["vite"], .devDependencies["vite"]'
# Also check for vite config in the desktop-client directory
if [ -f "packages/desktop-client/vite.config.ts" ] || [ -f "packages/desktop-client/vite.config.js" ]; then
echo "✓ Vite configuration found in desktop-client"
cat packages/desktop-client/vite.config.* | grep -A 5 "build"
else
echo "⚠ No Vite configuration found in desktop-client"
fi
# Let's also check what build tool is being used
cat packages/desktop-client/package.json | jq '.scripts, .devDependencies'
Length of output: 3380
Script:
#!/bin/bash
# Let's check for the vite config file with .ts or .js extension in the desktop-client directory
fd -t f "vite.config" packages/desktop-client
# Also check if there's any import.meta.glob usage in other files
rg "import\.meta\.glob" packages/desktop-client/src -A 2 -B 2
Length of output: 493
Script:
#!/bin/bash
# Let's examine the Vite config file content
cat packages/desktop-client/vite.config.mts
# Also check if the locale directory exists and contains JSON files
fd -t f ".json" packages/desktop-client/locale
Length of output: 5129
🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/i18n-string-extract-master.yml (3)
3-7: Consider adding path filters to optimize workflow triggersThe workflow currently runs on every push to master. Consider adding path filters to run only when relevant files containing i18n strings are modified:
on: push: branches: - master + paths: + - 'packages/desktop-client/src/**/*.{ts,tsx,js,jsx}' + - 'packages/**/i18n/**'
14-15: Add description to setup action metadataThe setup action at
./.github/actions/setupis missing a description in its metadata file. This description helps other developers understand the action's purpose.Please add a description to
.github/actions/setup/action.yml:name: 'Setup' description: 'Sets up the build environment with required dependencies'🧰 Tools
🪛 actionlint (1.7.4)
15-15: description is required in metadata of "Setup" action at "/inmem/21/d90ae3f3-7cb6-41b8-b48d-8b106b5e052a/home/jailuser/git/.github/actions/setup/action.yml"
(action)
16-18: Enhance i18n client configuration robustnessThe current setup could benefit from:
- Version pinning for wlc to ensure consistent behavior
- Verification of successful installation
- name: Configure i18n client run: | - pip install wlc + pip install wlc==1.2.0 + if ! command -v wlc &> /dev/null; then + echo "Error: wlc installation failed" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/i18n-string-extract-master.yml(1 hunks)packages/desktop-client/src/i18n.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/i18n.ts
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/i18n-string-extract-master.yml
15-15: description is required in metadata of "Setup" action at "/inmem/21/d90ae3f3-7cb6-41b8-b48d-8b106b5e052a/home/jailuser/git/.github/actions/setup/action.yml"
(action)
🔇 Additional comments (1)
.github/workflows/i18n-string-extract-master.yml (1)
28-35: Verify Weblate project configuration
Let's verify that the Weblate project and component are correctly configured.
✅ Verification successful
Weblate project and component configuration verified successfully
The Weblate integration is correctly configured:
- Project "Actual Budget" exists with ID 5716
- Component "actual" is properly set up with:
- Correct source language (English)
- i18nextv4 file format
- Template file: en.json
- Proper repository configuration pointing to actualbudget/translations
- Correct file mask (*.json)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Weblate project configuration
# Note: Using curl instead of wlc for verification to avoid authentication
# Check if the project exists
curl -s "https://hosted.weblate.org/api/projects/actualbudget/components/" | \
jq -r '.results[] | select(.slug == "actual") | .name'
# Check component configuration
curl -s "https://hosted.weblate.org/api/components/actualbudget/actual/en/" | \
jq -r '.name'
Length of output: 286
Script:
#!/bin/bash
# Let's verify the Weblate configuration more thoroughly
# First, check if the project exists and get its details
curl -s "https://hosted.weblate.org/api/projects/actualbudget/" | jq '.'
# Then check the components list
curl -s "https://hosted.weblate.org/api/projects/actualbudget/components/" | jq '.'
# Finally, check the specific component
curl -s "https://hosted.weblate.org/api/components/actualbudget/actual/" | jq '.'
Length of output: 15760
As it says on the tin. I also fixed some other issues with loading translations.