Add arabic translation and rtl css override... - #82
Merged
Merged
Conversation
Note: Need more reviews from 2 or 3 arab countries... Known bugs: The notifications box @ media < 991px. Usage: - Create a language and copy/paste the key/value pairs or download the pack and activate it... - Install the css override in Server Settings > Branding > Enable Use custom theme > Select to extend dark or light themes, set the css-overrides\Arabic_RTL_Override.css > Save > enjoy :)
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughPR adds Arabic language support to the translator with two changes: infrastructure improvements for HTTP client certificate handling in TranslationExtractor (allowing localhost TLS bypass and better URL format error handling), and registration of the Arabic locale entry in manifest.json with translation file path, maintainer, and timestamp. ChangesArabic Language Support and HTTP Client Infrastructure
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@css-overrides/Arabic_RTL_Override.css`:
- Around line 76-79: The NotificationsDropdown element at lines 76-79 uses a
hardcoded -200px offset in the inset property that is not properly overridden in
the `@media` (max-width: 991px) media query block at lines 81-90. To fix this, add
an inset property override within the `@media` (max-width: 991px) block for
`#NotificationsDropdown` that uses mobile-friendly positioning values appropriate
for smaller screens, removing the hardcoded -200px horizontal offset and
ensuring the dropdown stays within the viewport on devices with max-width of
991px or less.
- Line 5: The comment on line 5 violates the stylelint
`comment-whitespace-inside` rule due to missing whitespace before the closing
delimiter. In the comment `/* Right menu - Hide the scrollbar...*/`, add a space
between the closing text and the `*/` to ensure proper whitespace is present
both after the opening `/*` and before the closing `*/`.
- Around line 1-90: The Arabic_RTL_Override.css file is not being included in
the manifest because ManifestGenerator.cs only processes translation JSON files
and the manifest workflow is triggered only on translation file changes. To
resolve this, either extend ManifestGenerator.cs and the manifest schema to
support CSS file assets and ensure the manifest generation workflow includes CSS
files, OR add documentation clarifying that CSS overrides are managed separately
outside the translation package packaging flow and are not discovered via the
manifest. Choose the approach based on your intended distribution model for this
CSS file.
🪄 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: 89250851-5927-49cf-8135-f1fd13fb7770
📒 Files selected for processing (2)
css-overrides/Arabic_RTL_Override.csstranslations/arabic.json
Collaborator
|
Hi @AdilElFarissi , thank you for the job you've done. Can you explain me what is the .css file ? |
Contributor
Author
Hi @teamssUTXO , the arabic is a Right To Left (rtl) language and need some css adaptations to render it correctly... you can test it with English to see what an Arabic reader will see if she/he install the Arabic language pack without these CSS adjustements... Thank you for asking :) |
Member
|
About the known bug, can you open an issue on BTCpay? I don't think we use css files that are in this repo at all. |
Contributor
Author
Hi @NicolasDorier , the bug is in my amateur in action css :) and the css/folder is just to show what needs a change for the RTL languages... not a big deal... just margins and directions adaptations. you can remove it after testings. but the missing RTL support is a BTCPay matter! i opened an issue here with an easily codable suggestion. should i open it on the BTCPay repo? Thank you & Have a nice day :) |
Member
|
@AdilElFarissi yes please, open the issue on BTCPay repo, and remove the CSS from this PR. You can copy the CSS in the issue you open, that can be helpful when we work on the issue. |
Member
|
LGTM, for RTL: btcpayserver/btcpayserver#7408 |
Collaborator
|
The JSON file seems to be not up to date. We currently have 2600 keys string, and the file just add 2200 keys. Can you check this ? |
Contributor
Author
|
@teamssUTXO , Indeed, my JSON is missing over 400 keys, but that's all the Translator returns! The keys are from Translations.Default.cs... i wrote 2 lines to compare and extract the missing keys from your translation :) |
Member
|
@teamssUTXO @AdilElFarissi the I think we should merge that for the time being. |
i think is ready for more reviews... arabic is a complex language and geo-contextual... a word may mean other things in other arab countries.
Contributor
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 `@manifest.json`:
- Line 180: In the manifest.json file, locate the "Maintainer" entry with the
value "AdilElFarissi|https://github.com/AdilElFarissi" and remove the extra
space between the closing quote and the comma so that it reads
"AdilElFarissi|https://github.com/AdilElFarissi", matching the formatting style
of other entries in the manifest.
🪄 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: 52f4670c-7cbb-439a-9f89-713cef60924e
📒 Files selected for processing (2)
manifest.jsontranslations/arabic.json
The Translator return an error when you try to connect with --btcpay-url and the localhost is under https with a self signed cert (BTCPay DEV> --launch-profile Bitcoin-HTTPS)...
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Translator/Services/TranslationExtractor.cs (1)
43-43: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider extracting duplicate log statement.
The same log message appears in both branches. Moving it before the
ifblock reduces duplication.♻️ Proposed refactor
Uri uri = new(url, UriKind.Absolute); HttpResponseMessage response; + _logger.LogInformation("Fetching translations from BTCPay Server at {Url}", url); if((uri.Host == "localhost" || uri.Host == "127.0.0.1") && uri.Scheme == "https") { var handler = new HttpClientHandler { ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator }; - _logger.LogInformation("Fetching translations from BTCPay Server at {Url}", url); using var client = new HttpClient(handler, disposeHandler: true); response = await client.GetAsync(url); } else { - _logger.LogInformation("Fetching translations from BTCPay Server at {Url}", url); response = await _httpClient.GetAsync(url); }Also applies to: 50-50
🤖 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 `@Translator/Services/TranslationExtractor.cs` at line 43, The _logger.LogInformation call that logs "Fetching translations from BTCPay Server at {Url}" appears in duplicate across multiple branches in the conditional logic. Extract this log statement by moving it before the if block so it executes regardless of the branch taken, eliminating the redundant logging code while maintaining the same functionality.
🤖 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 `@Translator/Services/TranslationExtractor.cs`:
- Around line 38-46: The HttpClientHandler and HttpClient created in the code
block are not safely disposed. Replace the manual instantiation and disposal
pattern with using declarations to ensure proper cleanup even if GetAsync throws
an exception. Specifically, wrap both the HttpClientHandler and HttpClient
creation in using statements, and pass disposeHandler: true to the HttpClient
constructor so that the handler is properly disposed when the client is
disposed. Remove the manual client.Dispose() call and instead rely on the using
block to handle cleanup automatically.
---
Nitpick comments:
In `@Translator/Services/TranslationExtractor.cs`:
- Line 43: The _logger.LogInformation call that logs "Fetching translations from
BTCPay Server at {Url}" appears in duplicate across multiple branches in the
conditional logic. Extract this log statement by moving it before the if block
so it executes regardless of the branch taken, eliminating the redundant logging
code while maintaining the same functionality.
🪄 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: 2c5e8513-e0df-4039-8e01-18d94e126ace
📒 Files selected for processing (3)
Translator/Services/TranslationExtractor.csmanifest.jsontranslations/arabic.json
✅ Files skipped from review due to trivial changes (1)
- manifest.json
Comment on lines
+38
to
+46
| var handler = new HttpClientHandler | ||
| { | ||
| ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator | ||
| }; | ||
|
|
||
| _logger.LogInformation("Fetching translations from BTCPay Server at {Url}", url); | ||
| var client = new HttpClient(handler); | ||
| response = await client.GetAsync(url); | ||
| client.Dispose(); |
Contributor
There was a problem hiding this comment.
Resource leak: HttpClient and HttpClientHandler not safely disposed.
If GetAsync throws, the client is never disposed. Additionally, the handler is never disposed since HttpClient by default does not take ownership. Use using declarations and pass disposeHandler: true.
🛠️ Proposed fix
if((uri.Host == "localhost" || uri.Host == "127.0.0.1") && uri.Scheme == "https")
{
var handler = new HttpClientHandler
{
ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator
};
_logger.LogInformation("Fetching translations from BTCPay Server at {Url}", url);
- var client = new HttpClient(handler);
- response = await client.GetAsync(url);
- client.Dispose();
+ using var client = new HttpClient(handler, disposeHandler: true);
+ response = await client.GetAsync(url);
}📝 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.
Suggested change
| var handler = new HttpClientHandler | |
| { | |
| ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator | |
| }; | |
| _logger.LogInformation("Fetching translations from BTCPay Server at {Url}", url); | |
| var client = new HttpClient(handler); | |
| response = await client.GetAsync(url); | |
| client.Dispose(); | |
| var handler = new HttpClientHandler | |
| { | |
| ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator | |
| }; | |
| _logger.LogInformation("Fetching translations from BTCPay Server at {Url}", url); | |
| using var client = new HttpClient(handler, disposeHandler: true); | |
| response = await client.GetAsync(url); |
🤖 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 `@Translator/Services/TranslationExtractor.cs` around lines 38 - 46, The
HttpClientHandler and HttpClient created in the code block are not safely
disposed. Replace the manual instantiation and disposal pattern with using
declarations to ensure proper cleanup even if GetAsync throws an exception.
Specifically, wrap both the HttpClientHandler and HttpClient creation in using
statements, and pass disposeHandler: true to the HttpClient constructor so that
the handler is properly disposed when the client is disposed. Remove the manual
client.Dispose() call and instead rely on the using block to handle cleanup
automatically.
Member
|
@teamssUTXO anything missing here or it can be merged? |
teamssUTXO
approved these changes
Jun 22, 2026
Collaborator
|
LGTM |
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
3 participants
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.
Note:
Known bugs: CSS - The notifications box @ media < 991px.
Usage:
Thank you for your work!