You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2025/07/10 06:19:27 UTC
[PR] fix: frontend translation framework crashes on string errors [superset]
mistercrunch opened a new pull request, #34118:
URL: https://github.com/apache/superset/pull/34118
2 fixes here:
- one was around full on app crashes when a msgid contains a parameter that's not provided on the frontend (is happening on `master` on japanese right now)
- some strings labeled as fuzzy by babel were not making their way to the frontend. I don't fully understand what gets some strings labeled as fuzzy and others not, but without the `--fuzzy` flag the language pack generated by po2json was missing some strings in french
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: frontend translation framework crashes on string errors [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34118:
URL: https://github.com/apache/superset/pull/34118#discussion_r2201837778
##########
superset-frontend/packages/superset-ui-core/src/translation/Translator.ts:
##########
@@ -86,20 +86,35 @@ export default class Translator {
}
translate(input: string, ...args: unknown[]): string {
- return this.i18n.translate(input).fetch(...args);
+ try {
+ return this.i18n.translate(input).fetch(...args);
+ } catch (err) {
+ logging.warn(`Translation failed for key "${input}" with args:`, args);
+ logging.warn(err);
+ return input;
+ }
}
translateWithNumber(key: string, ...args: unknown[]): string {
- const [plural, num, ...rest] = args;
- if (typeof plural === 'number') {
+ try {
+ const [plural, num, ...rest] = args;
+ if (typeof plural === 'number') {
+ return this.i18n
+ .translate(key)
+ .ifPlural(plural, key)
+ .fetch(plural, num, ...args);
Review Comment:
I think i'm going to roll with #1 . I'm guessing it's ok to rely on hard coded pure t like `{num} dashboard(s) were saved.` instead of hoping the gettext pluralization to work ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: frontend translation framework crashes on string errors [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34118:
URL: https://github.com/apache/superset/pull/34118#discussion_r2201837778
##########
superset-frontend/packages/superset-ui-core/src/translation/Translator.ts:
##########
@@ -86,20 +86,35 @@ export default class Translator {
}
translate(input: string, ...args: unknown[]): string {
- return this.i18n.translate(input).fetch(...args);
+ try {
+ return this.i18n.translate(input).fetch(...args);
+ } catch (err) {
+ logging.warn(`Translation failed for key "${input}" with args:`, args);
+ logging.warn(err);
+ return input;
+ }
}
translateWithNumber(key: string, ...args: unknown[]): string {
- const [plural, num, ...rest] = args;
- if (typeof plural === 'number') {
+ try {
+ const [plural, num, ...rest] = args;
+ if (typeof plural === 'number') {
+ return this.i18n
+ .translate(key)
+ .ifPlural(plural, key)
+ .fetch(plural, num, ...args);
Review Comment:
I think i'm going to roll with #1 . I'm guessing it's ok to rely on hard coded pure t like `{num} dashboard(s) were saved.` instead of hoping the gettext pluralization to work (?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: frontend translation framework crashes on string errors [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34118:
URL: https://github.com/apache/superset/pull/34118#discussion_r2196704194
##########
superset-frontend/scripts/po2json.sh:
##########
@@ -22,14 +22,16 @@
set -e
+export NODE_NO_WARNINGS=1
Review Comment:
### Overly broad warning suppression <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Suppressing all Node.js warnings could hide important runtime issues unrelated to the translation process.
###### Why this matters
While the intent is to clean the console output, this global warning suppression could mask critical warnings about memory leaks, deprecation notices, or other runtime problems that need attention.
###### Suggested change ∙ *Feature Preview*
Instead of suppressing all Node.js warnings, consider using a more targeted approach by specifically handling the warnings you want to suppress. For example, you could redirect stderr for just the po2json command:
```bash
po2json --domain superset --format jed1.x --fuzzy $file $filename.json 2>/dev/null
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d15561f-5803-4d04-b0ec-fa1f1513f44e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d15561f-5803-4d04-b0ec-fa1f1513f44e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d15561f-5803-4d04-b0ec-fa1f1513f44e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d15561f-5803-4d04-b0ec-fa1f1513f44e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d15561f-5803-4d04-b0ec-fa1f1513f44e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:689c6951-1d51-4d94-b41a-dc98f0b1f735 -->
[](689c6951-1d51-4d94-b41a-dc98f0b1f735)
##########
superset-frontend/packages/superset-ui-core/src/translation/Translator.ts:
##########
@@ -86,20 +86,35 @@ export default class Translator {
}
translate(input: string, ...args: unknown[]): string {
- return this.i18n.translate(input).fetch(...args);
+ try {
+ return this.i18n.translate(input).fetch(...args);
+ } catch (err) {
+ logging.warn(`Translation failed for key "${input}" with args:`, args);
+ logging.warn(err);
Review Comment:
### Inefficient Multiple Logging Calls <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple consecutive logging calls for the same error event creates unnecessary I/O operations
###### Why this matters
Each logging operation is potentially an I/O operation that can impact performance, especially in error scenarios where multiple translations might fail
###### Suggested change ∙ *Feature Preview*
Combine the log messages into a single operation:
```typescript
logging.warn(`Translation failed for key "${input}" with args:`, args, '\nError:', err);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8b5888b-7020-4cd2-97bb-d491b24ecff6/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8b5888b-7020-4cd2-97bb-d491b24ecff6?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8b5888b-7020-4cd2-97bb-d491b24ecff6?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8b5888b-7020-4cd2-97bb-d491b24ecff6?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8b5888b-7020-4cd2-97bb-d491b24ecff6)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3b30cd60-7a66-49c2-84a6-309ea70d4df2 -->
[](3b30cd60-7a66-49c2-84a6-309ea70d4df2)
##########
superset-frontend/packages/superset-ui-core/src/translation/Translator.ts:
##########
@@ -86,20 +86,35 @@
}
translate(input: string, ...args: unknown[]): string {
- return this.i18n.translate(input).fetch(...args);
+ try {
+ return this.i18n.translate(input).fetch(...args);
+ } catch (err) {
+ logging.warn(`Translation failed for key "${input}" with args:`, args);
+ logging.warn(err);
+ return input;
+ }
}
translateWithNumber(key: string, ...args: unknown[]): string {
- const [plural, num, ...rest] = args;
- if (typeof plural === 'number') {
+ try {
+ const [plural, num, ...rest] = args;
+ if (typeof plural === 'number') {
+ return this.i18n
+ .translate(key)
+ .ifPlural(plural, key)
+ .fetch(plural, num, ...args);
+ }
return this.i18n
.translate(key)
- .ifPlural(plural, key)
- .fetch(plural, num, ...args);
+ .ifPlural(num as number, plural as string)
+ .fetch(...rest);
+ } catch (err) {
+ logging.warn(
+ `Plural translation failed for key "${key}" with args:`,
+ args,
+ );
+ logging.warn(err);
+ return key;
}
Review Comment:
### Duplicate Error Logging <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Similar to the first translate method, the error logging is split into two separate warn statements.
###### Why this matters
Multiple log statements for the same error make it harder to track related log entries and increase log verbosity.
###### Suggested change ∙ *Feature Preview*
Combine the error logging into a single statement:
```typescript
catch (err) {
logging.warn(`Plural translation failed for key "${key}" with args:`, args, err);
return key;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd060d9b-26ec-474d-8c0f-243dd28f8af6/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd060d9b-26ec-474d-8c0f-243dd28f8af6?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd060d9b-26ec-474d-8c0f-243dd28f8af6?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd060d9b-26ec-474d-8c0f-243dd28f8af6?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd060d9b-26ec-474d-8c0f-243dd28f8af6)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f2b3e414-c720-4420-b6de-94fda99cb3ae -->
[](f2b3e414-c720-4420-b6de-94fda99cb3ae)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: frontend translation framework crashes on string errors [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34118:
URL: https://github.com/apache/superset/pull/34118#discussion_r2201831457
##########
superset-frontend/packages/superset-ui-core/src/translation/Translator.ts:
##########
@@ -86,20 +86,35 @@ export default class Translator {
}
translate(input: string, ...args: unknown[]): string {
- return this.i18n.translate(input).fetch(...args);
+ try {
+ return this.i18n.translate(input).fetch(...args);
+ } catch (err) {
+ logging.warn(`Translation failed for key "${input}" with args:`, args);
+ logging.warn(err);
+ return input;
+ }
}
translateWithNumber(key: string, ...args: unknown[]): string {
- const [plural, num, ...rest] = args;
- if (typeof plural === 'number') {
+ try {
+ const [plural, num, ...rest] = args;
+ if (typeof plural === 'number') {
+ return this.i18n
+ .translate(key)
+ .ifPlural(plural, key)
+ .fetch(plural, num, ...args);
Review Comment:
oh right, good catch, though it looks like it was wrong before too (!?)
Grepping through the repo, it appears `translateWithNumber` is only tied to `tn('...')` calls, which are not used anywhere except in a few unit tests ...
Not sure what to do:
- delete unused code and references to it?
- fix/improve the function and write unit tests for it?
- [not for this PR], start using `tn()` in more places? Guessing it's meant to be used in strings like "You saved {num} dashboards(s)"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: frontend translation framework crashes on string errors [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #34118:
URL: https://github.com/apache/superset/pull/34118#discussion_r2196887812
##########
superset-frontend/packages/superset-ui-core/src/translation/Translator.ts:
##########
@@ -86,20 +86,35 @@ export default class Translator {
}
translate(input: string, ...args: unknown[]): string {
- return this.i18n.translate(input).fetch(...args);
+ try {
+ return this.i18n.translate(input).fetch(...args);
+ } catch (err) {
+ logging.warn(`Translation failed for key "${input}" with args:`, args);
+ logging.warn(err);
+ return input;
+ }
}
translateWithNumber(key: string, ...args: unknown[]): string {
- const [plural, num, ...rest] = args;
- if (typeof plural === 'number') {
+ try {
+ const [plural, num, ...rest] = args;
+ if (typeof plural === 'number') {
+ return this.i18n
+ .translate(key)
+ .ifPlural(plural, key)
+ .fetch(plural, num, ...args);
Review Comment:
Should that be `.fetch(plural, num, ...rest)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: frontend translation framework crashes on string errors [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #34118:
URL: https://github.com/apache/superset/pull/34118
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org