CONSOLE-5428: i18next-cli migration follow up - #16864
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
logonoff
changed the title
|
@logonoff: This pull request references CONSOLE-5428 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/label px-approved |
openshift-ci
Bot
added
component/core
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughThe PR migrates dynamic-demo-plugin internationalization to i18next-cli, adds console-extension key extraction, translates plugin UI components, updates English catalogs, and changes console-app pluralization keys. ChangesInternationalization migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/label plugin-api-changed |
|
@logonoff: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.md`:
- Line 117: Correct the deprecated tooling reference in the release note by
changing `i8next-parser` to `i18next-parser`; leave the surrounding migration
guidance unchanged.
- Around line 136-137: Update the i18n configuration example’s defaultNS field
to use a quoted, valid placeholder namespace such as “plugin__my-plugin” instead
of an inline comment, so the example can be copied as valid TypeScript.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e13db201-2d9f-4d95-9a7e-f2f086db8115
📒 Files selected for processing (7)
dynamic-demo-plugin/i18next.config.tsfrontend/i18next.config.tsfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.mdfrontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.mdfrontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsfrontend/packages/console-dynamic-plugin-sdk/src/i18n/ConsoleExtensionsPlugin.tsfrontend/packages/console-dynamic-plugin-sdk/src/lib-webpack.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- dynamic-demo-plugin/i18next.config.ts
- frontend/packages/console-dynamic-plugin-sdk/src/lib-webpack.ts
- frontend/packages/console-dynamic-plugin-sdk/src/i18n/ConsoleExtensionsPlugin.ts
- frontend/i18next.config.ts
eb0f264 to
ff5927f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.md`:
- Line 136: Update the defaultNS example in the internationalization
configuration to use the documented double-underscore namespace convention,
changing the value from plugin_my-swanker-plugin to plugin__my-swanker-plugin so
it matches the extension translation keys and the convention used elsewhere in
the release notes.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5a181515-1d38-45f2-9c58-93ad37bd4d1d
📒 Files selected for processing (7)
dynamic-demo-plugin/i18next.config.tsfrontend/i18next.config.tsfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.mdfrontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.mdfrontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsfrontend/packages/console-dynamic-plugin-sdk/src/i18n/ConsoleExtensionsPlugin.tsfrontend/packages/console-dynamic-plugin-sdk/src/lib-webpack.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- dynamic-demo-plugin/i18next.config.ts
- frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts
- frontend/packages/console-dynamic-plugin-sdk/src/lib-webpack.ts
- frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md
- frontend/packages/console-dynamic-plugin-sdk/src/i18n/ConsoleExtensionsPlugin.ts
- frontend/i18next.config.ts
3c25a30 to
6e51756
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
670c63c to
91bc23f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/console-dynamic-plugin-sdk/src/i18n/ConsoleExtensionsI18nextCliPlugin.ts`:
- Around line 41-50: Update the placeholder extraction logic in the parse
callback to validate the entire placeholder with one strict allow-list regex
that captures the namespace and key, rejecting extra delimiters, trailing
content, and other malformed values before pushing to extracted. Replace the
broad /^%(.+)%$/ match and subsequent split logic while preserving valid
namespace~key registration.
- Around line 31-36: Update the readFile error handling in the paths mapping
within ConsoleExtensionsI18nextCliPlugin so only ENOENT errors return the empty
manifest fallback and warning; rethrow permission, directory, transient I/O, and
all other errors so extraction fails rather than silently omitting keys.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a905e746-bd27-4384-a634-bf2b82cbfa7b
📒 Files selected for processing (7)
dynamic-demo-plugin/i18next.config.tsfrontend/i18next.config.tsfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.mdfrontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.mdfrontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsfrontend/packages/console-dynamic-plugin-sdk/src/i18n/ConsoleExtensionsI18nextCliPlugin.tsfrontend/packages/console-dynamic-plugin-sdk/src/lib-webpack.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md
- dynamic-demo-plugin/i18next.config.ts
- frontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.md
- frontend/i18next.config.ts
- frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts
| const files = await Promise.all( | ||
| paths.map((path) => | ||
| readFile(path, 'utf-8').catch(() => { | ||
| console.warn(chalk.yellowBright(`Warning: Could not read file at ${path}. Skipping.`)); | ||
| return '{}'; | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not turn every read failure into an empty manifest.
Permission, directory, and transient I/O errors currently allow extraction to succeed with missing keys. Catch only ENOENT when absent files are intentionally optional; rethrow other errors.
Proposed adjustment
- readFile(path, 'utf-8').catch(() => {
+ readFile(path, 'utf-8').catch((error) => {
+ if (error?.code !== 'ENOENT') {
+ throw error;
+ }
console.warn(chalk.yellowBright(`Warning: Could not read file at ${path}. Skipping.`));
return '{}';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const files = await Promise.all( | |
| paths.map((path) => | |
| readFile(path, 'utf-8').catch(() => { | |
| console.warn(chalk.yellowBright(`Warning: Could not read file at ${path}. Skipping.`)); | |
| return '{}'; | |
| }), | |
| const files = await Promise.all( | |
| paths.map((path) => | |
| readFile(path, 'utf-8').catch((error) => { | |
| if (error?.code !== 'ENOENT') { | |
| throw error; | |
| } | |
| console.warn(chalk.yellowBright(`Warning: Could not read file at ${path}. Skipping.`)); | |
| return '{}'; | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/i18n/ConsoleExtensionsI18nextCliPlugin.ts`
around lines 31 - 36, Update the readFile error handling in the paths mapping
within ConsoleExtensionsI18nextCliPlugin so only ENOENT errors return the empty
manifest fallback and warning; rethrow permission, directory, transient I/O, and
all other errors so extraction fails rather than silently omitting keys.
| const extracted: { key: string }[] = []; | ||
|
|
||
| try { | ||
| parse( | ||
| content, | ||
| (_key, value) => { | ||
| if (typeof value === 'string') { | ||
| const match = value.match(/^%(.+)%$/); | ||
| if (match && match[1]) { | ||
| extracted.push({ key: match[1] }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the complete placeholder before registering it.
^%(.+)%$ accepts malformed values such as %ns~key%extra%, while split('~', 2) silently truncates values such as %ns~a~b%. Parse namespace and key with one strict allow-list regex and reject malformed placeholders.
As per path instructions, validate trust-boundary formats with an allow-list rather than a broad .+ matcher.
Also applies to: 62-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/i18n/ConsoleExtensionsI18nextCliPlugin.ts`
around lines 41 - 50, Update the placeholder extraction logic in the parse
callback to validate the entire placeholder with one strict allow-list regex
that captures the namespace and key, rejecting extra delimiters, trailing
content, and other malformed values before pushing to extracted. Replace the
broad /^%(.+)%$/ match and subsequent split logic while preserving valid
namespace~key registration.
Source: Path instructions
Directly write the legacy i18n strings into the t function instead of having the CLI produce the new string format
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dynamic-demo-plugin/i18next.config.ts`:
- Around line 20-35: Update the placeholder extraction in the JSON parsing
callback to accept only values containing exactly two non-empty components
separated by “~”, using allow-list validation for the namespace and key formats.
Reject placeholders with additional separators instead of passing them to the
extracted list, so the later fullKey.split('~', 2) in the extraction loop cannot
truncate malformed values.
In `@dynamic-demo-plugin/locales/en/plugin__console-demo-plugin.json`:
- Around line 2-5: Update the `_other` translation entries for `"{{count}} Cron
Job_other"` and `"{{count}} Worker Node_other"` to use plural wording, while
leaving the `_one` entries singular and preserving the existing count
interpolation.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0cff7a63-825f-4e51-bfcb-7370be8b0c5c
⛔ Files ignored due to path filters (1)
dynamic-demo-plugin/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
dynamic-demo-plugin/README.mddynamic-demo-plugin/i18next-parser.config.jsdynamic-demo-plugin/i18next.config.tsdynamic-demo-plugin/locales/en/plugin__console-demo-plugin.jsondynamic-demo-plugin/locales/en/plugin__console-plugin-template.jsondynamic-demo-plugin/package.jsondynamic-demo-plugin/src/components/APIServerEvents.tsxdynamic-demo-plugin/src/components/ClusterOverview/Activity.tsxdynamic-demo-plugin/src/components/ErrorBoundary/DemoErrorBoundary.tsxdynamic-demo-plugin/src/components/Modals/ModalPage.tsxdynamic-demo-plugin/src/components/stubPages.tsxdynamic-demo-plugin/src/i18n.tsfrontend/i18n-scripts/README.mdfrontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/quick-starts/utils/quick-start-context.tsx
💤 Files with no reviewable changes (4)
- dynamic-demo-plugin/i18next-parser.config.js
- dynamic-demo-plugin/locales/en/plugin__console-plugin-template.json
- dynamic-demo-plugin/src/i18n.ts
- frontend/i18n-scripts/README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/packages/console-app/src/components/quick-starts/utils/quick-start-context.tsx
- dynamic-demo-plugin/README.md
- dynamic-demo-plugin/src/components/ErrorBoundary/DemoErrorBoundary.tsx
- frontend/packages/console-app/locales/en/console-app.json
- dynamic-demo-plugin/src/components/APIServerEvents.tsx
- dynamic-demo-plugin/src/components/ClusterOverview/Activity.tsx
- dynamic-demo-plugin/package.json
- dynamic-demo-plugin/src/components/stubPages.tsx
- dynamic-demo-plugin/src/components/Modals/ModalPage.tsx
|
/pipeline required |
|
Scheduling tests matching the |
openshift-ci-robot
added
the
verified
|
@logonoff: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@logonoff: This pull request references CONSOLE-5428 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
/test analyze backend frontend images okd-scos-images |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@logonoff: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1a14384
into
openshift:main
3 participants
Analysis / Root cause:
follow ups to #16796
Solution description:
Expose ConsoleExtensionsPlugin so that demo plugin (and console plugin template in NO-JIRA: bump deps + migrate to i18next-cli console-plugin-template#116) can use the code instead of the snippet needing excessive copy/pastingwill be done in another follow-upSummary by CodeRabbit