Open
Conversation
This was for CasparCG 2.1, and is not a problem with 2.3+, which is all that Sofie supports now
Julusian
added
the
Contribution from SuperFly.tv
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 31 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (54)
WalkthroughThis PR refactors the i18n translation extraction workflow, updates dependencies across the monorepo (especially i18next major version and yarn), restructures WebUI React component type annotations to use explicit React.ComponentType declarations, migrates TFunction type imports from react-i18next to i18next, and updates MongoDB/TypeScript signatures in job-worker packages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/webui/src/client/lib/notifications/ReactNotification.tsx (1)
13-31:⚠️ Potential issue | 🟠 MajorTrack
props.childrenin the effect dependencies.The notification payload is built from
props.childrenon Line 19, but the effect never reruns when only the child content changes. That leaves the notification center showing stale content until some other prop changes or the component unmounts.🛠️ Proposed fix
- }, [props.level, props.source, props.actions, props.rank]) + }, [props.level, props.source, props.actions, props.rank, props.children])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/lib/notifications/ReactNotification.tsx` around lines 13 - 31, The effect in ReactNotification builds a Notification using props.children but children isn't included in the dependency array, so updates to child content won't rebuild/replace the notification; update the useEffect dependencies for ReactNotification to include props.children (along with props.level, props.source, props.actions, props.rank) so the effect reruns whenever the children change and the NotificationCenter gets the fresh payload.
🧹 Nitpick comments (6)
meteor/i18n/.gitignore (1)
1-1: Consider adding a trailing newline.The file is missing a trailing newline at the end. While this doesn't affect functionality, POSIX standards and most style guides recommend that text files end with a newline character for better compatibility with various tools.
📝 Suggested fix
*.json +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/i18n/.gitignore` at line 1, Add a trailing newline to the .gitignore file so the final line ("*.json") ends with a newline character; open the meteor/i18n/.gitignore (look for the "*.json" entry) and ensure you insert a single newline at EOF so the file conforms to POSIX/text-file conventions.meteor/scripts/translation/extract.mjs (2)
82-85: Consider logging unexpected errors during PO file parsing.The catch block silently ignores all errors, treating them the same as a missing file. This could mask unexpected issues like permission errors or disk failures.
Suggested improvement
} catch (err) { - // No existing .po file or parse error - start fresh - console.log(`No existing/valid .po file found for ${language}, starting with empty translations.`) + // No existing .po file or parse error - start fresh + if (err?.code !== 'ENOENT') { + console.warn(`Warning: Error reading ${poPath}: ${err.message}`) + } + console.log(`No existing/valid .po file found for ${language}, starting with empty translations.`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/scripts/translation/extract.mjs` around lines 82 - 85, The catch block that currently swallows all errors when reading/parsing the .po file for a given language should capture the exception (e.g., catch (err)) and log unexpected errors instead of treating everything as “file missing”; update the handler used in extract.mjs where .po parsing occurs to check the error (for example, if err.code === 'ENOENT' keep the existing console.log about starting with empty translations) and for any other error call console.error (including the language and the error object) so permission/disk or parsing failures are visible while still continuing with empty translations.
103-104:keysExtractedis overwritten by each locale iteration.Inside the
Promise.allmap,extractionStats.keysExtractedis assigned for every locale, so only the last assignment persists. While all locales should extract the same keys (making this functionally correct), it's fragile and harder to reason about.Suggested fix: assign once outside the loop
+ // All locales extract the same keys, so use the first result + if (results.length > 0) { + extractionStats.keysExtracted = Object.keys(results[0].newTranslations).length + } + await Promise.all( results.map(async (result) => { // ... - extractionStats.keysExtracted = newKeys.length + // (remove this line)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/scripts/translation/extract.mjs` around lines 103 - 104, The code assigns extractionStats.keysExtracted inside the Promise.all per-locale loop, so it gets overwritten each iteration; move the assignment out of the per-locale map: compute newKeys (or use the first locale's newKeys) during the mapping and set extractionStats.keysExtracted once after the Promise.all completes (or before the map starts) while continuing to push per-locale objects into extractionStats.locales; update references to extractionStats.keysExtracted, newKeys, language and the Promise.all map to reflect this single assignment.meteor/scripts/i18n-compile-json.mjs (1)
1-3: Resolve paths from the script location, not the current working directory.Both the input path (
'i18n') and output path ('..', 'packages', ...) are cwd-relative. That makes this script fragile outside themeteor/cwd, including ad-hoc runs and some CI setups.🛠️ Proposed fix
import { writeFile, mkdir } from 'node:fs/promises' -import { join } from 'node:path' +import { dirname, join } from 'node:path' +import { fileURLToPath } from 'node:url' import { getTranslations } from './translation/bundle.mjs' + +const scriptDir = dirname(fileURLToPath(import.meta.url)) +const meteorDir = join(scriptDir, '..') -const translations = await getTranslations('i18n', 'translations') +const translations = await getTranslations(join(meteorDir, 'i18n'), 'translations') const errors = [] for (const { language, data } of translations) { try { - const outDir = join('..', 'packages', 'webui', 'public', 'locales', language) + const outDir = join(meteorDir, '..', 'packages', 'webui', 'public', 'locales', language) await mkdir(outDir, { recursive: true }) const outPath = join(outDir, 'translations.json')Also applies to: 12-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/scripts/i18n-compile-json.mjs` around lines 1 - 3, The script currently uses cwd-relative paths for the input and output (e.g. 'i18n' and '..','packages',...) which makes runs outside meteor/ fragile; change path resolution to be relative to the script file by deriving the script directory from import.meta.url (use fileURLToPath + dirname) and then build inputDir = join(scriptDir, 'i18n') and outputDir = join(scriptDir, '..', 'packages', ...) and use those instead of plain 'i18n' or cwd-based joins; update any places that call getTranslations, mkdir, or writeFile to use these resolved paths and keep mkdir({ recursive: true }) and writeFile targets intact.packages/webui/src/client/ui/Settings/components/ConfigManifestOAuthFlow.tsx (1)
40-40: Consider using proper typing instead ofanycast.The
(e2.target as any).resultloses type safety.FileReader.onloadprovides aProgressEvent<FileReader>whereresultcan be typed.🔧 Suggested improvement
- const uploadFileContents = (e2.target as any).result + const uploadFileContents = e2.target?.resultOr for stricter typing:
- const uploadFileContents = (e2.target as any).result + const uploadFileContents = (e2.target as FileReader).result as string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/Settings/components/ConfigManifestOAuthFlow.tsx` at line 40, The code casts (e2.target as any).result which loses type safety; update the FileReader.onload handler signature to use ProgressEvent<FileReader> (e.g., onload = (e2: ProgressEvent<FileReader>) => ...) and access the result via e2.target?.result with a proper type assert (string | ArrayBuffer | null) or narrow it at runtime (check typeof result === 'string') before using it; update references in the uploadFileContents logic to handle the nullable result instead of using any.meteor/scripts/translation/bundle.mjs (1)
46-50: Consider preserving the original error for debugging.The catch block discards the original error, which could make debugging difficult when the directory read fails for reasons other than non-existence (e.g., permission issues).
🔧 Suggested improvement
try { entries = await readdir(i18nDir) - } catch { - throw new Error(`Failed to read directory: ${i18nDir}`) + } catch (err) { + throw new Error(`Failed to read directory: ${i18nDir}: ${err.message}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/scripts/translation/bundle.mjs` around lines 46 - 50, The catch block around the readdir call discards the original error (variables: entries, readdir, i18nDir); change the catch to capture the thrown error (e.g., "err") and rethrow a new Error that preserves the original error either by using the Error "cause" option or by including err.message/stack in the thrown message so callers have the original failure details for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@meteor/scripts/translation/extract.mjs`:
- Around line 66-69: The language extraction currently uses
result.path.split(...).at(-2) which returns "i18n" for paths like
"i18n/en.json"; change the extraction in the results.map handler to use
result.path.split(/[/\\]/).at(-1).replace(/\.json$/, '') to get the actual
language code, and update any references that expect that variable (the async
map callback where const language is defined). Do not assign
extractionStats.keysExtracted inside each iteration of the Promise.all
loop—either collect per-locale stats (e.g., a map keyed by language) or set
extractionStats.keysExtracted once after Promise.all completes using a
representative value to avoid overwriting/masking issues. Finally, modify the
catch block that swallows errors to log the caught error (include the error
object/message) before falling back, so parse failures and other unexpected
errors are visible.
In `@packages/webui/src/client/ui/i18n.ts`:
- Around line 151-153: The exported translator is currently captured as a
snapshot (i18nTranslator = container.i18nInstance.t) which bypasses the intended
init-time swap; instead export the container's indirection so early imports use
the fallback until init replaces it—replace the local binding and export
container.i18nTranslator directly (referencing container.i18nTranslator and
removing the use of container.i18nInstance.t and the i18nTranslator local
constant).
In
`@packages/webui/src/client/ui/Settings/components/ConfigManifestOAuthFlow.tsx`:
- Line 54: In ConfigManifestOAuthFlow update the three notification strings that
misspell "successfully" to the correct spelling; locate the t(...) calls in the
component that currently read "OAuth credentials succesfully uploaded." and the
other two occurrences "successfuly" and change them to "OAuth credentials
successfully uploaded." (or the appropriate surrounding text) so all three
t(...) translation keys use "successfully" consistently; ensure you update the
exact strings passed to t(...) in the ConfigManifestOAuthFlow component.
- Around line 36-38: The component calls this.setState({ uploadFileKey:
Date.now() }) but uploadFileKey is not declared on
IConfigManifestOAuthFlowComponentState; add uploadFileKey: number to the
IConfigManifestOAuthFlowComponentState interface and ensure the component's
initial state (constructor or state property) includes a default uploadFileKey
(e.g., Date.now() or 0) so TypeScript recognizes the field used by setState in
ConfigManifestOAuthFlow.
---
Outside diff comments:
In `@packages/webui/src/client/lib/notifications/ReactNotification.tsx`:
- Around line 13-31: The effect in ReactNotification builds a Notification using
props.children but children isn't included in the dependency array, so updates
to child content won't rebuild/replace the notification; update the useEffect
dependencies for ReactNotification to include props.children (along with
props.level, props.source, props.actions, props.rank) so the effect reruns
whenever the children change and the NotificationCenter gets the fresh payload.
---
Nitpick comments:
In `@meteor/i18n/.gitignore`:
- Line 1: Add a trailing newline to the .gitignore file so the final line
("*.json") ends with a newline character; open the meteor/i18n/.gitignore (look
for the "*.json" entry) and ensure you insert a single newline at EOF so the
file conforms to POSIX/text-file conventions.
In `@meteor/scripts/i18n-compile-json.mjs`:
- Around line 1-3: The script currently uses cwd-relative paths for the input
and output (e.g. 'i18n' and '..','packages',...) which makes runs outside
meteor/ fragile; change path resolution to be relative to the script file by
deriving the script directory from import.meta.url (use fileURLToPath + dirname)
and then build inputDir = join(scriptDir, 'i18n') and outputDir =
join(scriptDir, '..', 'packages', ...) and use those instead of plain 'i18n' or
cwd-based joins; update any places that call getTranslations, mkdir, or
writeFile to use these resolved paths and keep mkdir({ recursive: true }) and
writeFile targets intact.
In `@meteor/scripts/translation/bundle.mjs`:
- Around line 46-50: The catch block around the readdir call discards the
original error (variables: entries, readdir, i18nDir); change the catch to
capture the thrown error (e.g., "err") and rethrow a new Error that preserves
the original error either by using the Error "cause" option or by including
err.message/stack in the thrown message so callers have the original failure
details for debugging.
In `@meteor/scripts/translation/extract.mjs`:
- Around line 82-85: The catch block that currently swallows all errors when
reading/parsing the .po file for a given language should capture the exception
(e.g., catch (err)) and log unexpected errors instead of treating everything as
“file missing”; update the handler used in extract.mjs where .po parsing occurs
to check the error (for example, if err.code === 'ENOENT' keep the existing
console.log about starting with empty translations) and for any other error call
console.error (including the language and the error object) so permission/disk
or parsing failures are visible while still continuing with empty translations.
- Around line 103-104: The code assigns extractionStats.keysExtracted inside the
Promise.all per-locale loop, so it gets overwritten each iteration; move the
assignment out of the per-locale map: compute newKeys (or use the first locale's
newKeys) during the mapping and set extractionStats.keysExtracted once after the
Promise.all completes (or before the map starts) while continuing to push
per-locale objects into extractionStats.locales; update references to
extractionStats.keysExtracted, newKeys, language and the Promise.all map to
reflect this single assignment.
In
`@packages/webui/src/client/ui/Settings/components/ConfigManifestOAuthFlow.tsx`:
- Line 40: The code casts (e2.target as any).result which loses type safety;
update the FileReader.onload handler signature to use ProgressEvent<FileReader>
(e.g., onload = (e2: ProgressEvent<FileReader>) => ...) and access the result
via e2.target?.result with a proper type assert (string | ArrayBuffer | null) or
narrow it at runtime (check typeof result === 'string') before using it; update
references in the uploadFileContents logic to handle the nullable result instead
of using any.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fcb7dce-30f2-4a94-93b1-60849543753a
⛔ Files ignored due to path filters (3)
meteor/yarn.lockis excluded by!**/yarn.lock,!**/*.lockpackages/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (67)
.yarnrc.ymlDEVELOPER.mdmeteor/i18n/.gitignoremeteor/i18n/en.pometeor/i18n/nb.mometeor/i18n/nb.pometeor/i18n/nn.mometeor/i18n/nn.pometeor/i18n/sv.pometeor/package.jsonmeteor/scripts/extract-i18next-po.mjsmeteor/scripts/extract-i18next-pot.mjsmeteor/scripts/i18n-compile-json.mjsmeteor/scripts/translation/bundle.mjsmeteor/scripts/translation/config.mjsmeteor/scripts/translation/extract.mjspackage.jsonpackages/corelib/package.jsonpackages/corelib/src/TranslatableMessage.tspackages/documentation/package.jsonpackages/job-worker/package.jsonpackages/job-worker/src/__mocks__/collection.tspackages/job-worker/src/db/collection.tspackages/job-worker/src/db/collections.tspackages/live-status-gateway-api/package.jsonpackages/live-status-gateway/package.jsonpackages/meteor-lib/package.jsonpackages/mos-gateway/package.jsonpackages/openapi/package.jsonpackages/package.jsonpackages/playout-gateway/package.jsonpackages/server-core-integration/package.jsonpackages/shared-lib/package.jsonpackages/webui/package.jsonpackages/webui/public/locales/nb/translations.jsonpackages/webui/public/locales/nn/translations.jsonpackages/webui/public/locales/sv/translations.jsonpackages/webui/src/client/lib/ConnectionStatusNotification.tsxpackages/webui/src/client/lib/ModalDialog.tsxpackages/webui/src/client/lib/iconPicker.tsxpackages/webui/src/client/lib/notifications/ReactNotification.tsxpackages/webui/src/client/lib/notifications/notifications.tspackages/webui/src/client/lib/polyfill/polyfills.tspackages/webui/src/client/lib/polyfill/promise.allSettled.tspackages/webui/src/client/lib/rundown.tspackages/webui/src/client/ui/AfterBroadcastForm.tsxpackages/webui/src/client/ui/ClipTrimPanel/VideoEditMonitor.tsxpackages/webui/src/client/ui/PreviewPopUp/PreviewPopUpContent.tsxpackages/webui/src/client/ui/PreviewPopUp/Util/FloatingInspectorTimeInformationRow.tsxpackages/webui/src/client/ui/RundownList/__tests__/DisplayFormattedTime.test.tspackages/webui/src/client/ui/SegmentTimeline/Parts/SegmentTimelinePart.tsxpackages/webui/src/client/ui/SegmentTimeline/Renderers/MicSourceRenderer.tsxpackages/webui/src/client/ui/SegmentTimeline/Renderers/VTSourceRenderer.tsxpackages/webui/src/client/ui/SegmentTimeline/SegmentTimeline.tsxpackages/webui/src/client/ui/Settings/RundownLayoutEditor.tsxpackages/webui/src/client/ui/Settings/SettingsMenu.tsxpackages/webui/src/client/ui/Settings/SystemManagement.tsxpackages/webui/src/client/ui/Settings/Upgrades/Components.tsxpackages/webui/src/client/ui/Settings/components/ConfigManifestOAuthFlow.tsxpackages/webui/src/client/ui/Settings/components/FilterEditor.tsxpackages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/actionSelector/ActionSelector.tsxpackages/webui/src/client/ui/Shelf/DashboardActionButtonGroup.tsxpackages/webui/src/client/ui/Shelf/ExternalFramePanel.tsxpackages/webui/src/client/ui/Shelf/RundownViewBuckets.tsxpackages/webui/src/client/ui/Status/StatusCodePill.tsxpackages/webui/src/client/ui/Status/SystemStatus/DeviceItem.tsxpackages/webui/src/client/ui/i18n.ts
💤 Files with no reviewable changes (3)
- packages/webui/src/client/lib/polyfill/polyfills.ts
- packages/webui/src/client/lib/polyfill/promise.allSettled.ts
- meteor/scripts/extract-i18next-pot.mjs
fixes some issues with plurals not working due to misconfigured tools
Julusian
force-pushed
the
feat/update-i18next-parser
branch
from
b4f1de6 to
39e6bb5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About the Contributor
This pull request is posted on behalf of Superfly
Type of Contribution
This is a: Feature / Code improvement
This builds on the changes in #1714, to avoid merge conflicts as they both heavily touch the package.json and yarn.lock files
Follow up to Sofie-Automation/sofie-timeline-state-resolver#439, performing the same steps
Current Behavior
We are using an old version of i18next (5 major versions, 3+ years)
New Behavior
This updates all the i18next libs and tooling, based upon the recent TSR PR.
When doing this, I also discovered that our i18next was misconfigured, with the plural support being broken which has been addressed here too.
Various translation files have been regenerated as part of this. Some old keys from the po files have been purged by this change, from my spot checking none of them appear to be used (some of them were clearly really old too)
Testing
Affected areas
Time Frame
Other Information
Status