Skip to content

feat(interface): add lang selector in settings page #525

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FredGuiou
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Jun 25, 2025

Codecov Report

Attention: Patch coverage is 35.71429% with 27 lines in your changes missing coverage. Please review.

Project coverage is 56.45%. Comparing base (28ee6a9) to head (024bb89).
Report is 202 commits behind head on master.

Files with missing lines Patch % Lines
src/commands/lang.js 6.66% 14 Missing ⚠️
workspaces/server/src/config.js 7.14% 13 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##            master     #525       +/-   ##
============================================
- Coverage   100.00%   56.45%   -43.55%     
============================================
  Files           15       38       +23     
  Lines          649     2790     +2141     
  Branches         0        1        +1     
============================================
+ Hits           649     1575      +926     
- Misses           0     1215     +1215     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fraxken fraxken requested a review from Copilot June 25, 2025 20:19
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new language selector to the settings page, wires it end-to-end through the server config and CLI, and updates tests and translations accordingly.

  • Persist a lang field in config get/set and in the HTTP server
  • Add a dropdown in the web UI and reload on language change
  • Extend CLI lang command, update client code, tests, and translation keys

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
workspaces/server/src/config.js Include lang in config retrieval and fallback
workspaces/server/test/httpServer.test.js Adjust HTTP PUT /config test to expect lang
workspaces/server/test/config.test.js Add lang assertions in config tests
views/index.html Add <select> for language choice in settings UI
src/commands/lang.js Persist new lang to cache in CLI lang command
public/main.js Reload page and apply lang when settings change
public/components/views/settings/settings.js Hook up langSelector and dispatch updated lang
i18n/french.js & i18n/english.js Add translation keys for language panel
Comments suppressed due to low confidence (3)

src/commands/lang.js:19

  • The command’s cache update logic inside the try block isn’t covered by existing tests. Add tests to ensure the CLI lang command persists the new language in appCache.
  try {

public/components/views/settings/settings.js:234

  • This assignment reverts the updated lang back to the old value. You should assign this.config = newConfig so the new language persists in the component state.
    this.config = { ...newConfig, lang: this.config.lang };

@PierreDemailly PierreDemailly requested a review from fraxken June 26, 2025 15:28
@FredGuiou
Copy link
Contributor Author

@fraxken Here is a proposal for lang application management in settings page.
Stay to complete testing, but your comments are welcome in a first review. 🙂

Comment on lines +107 to +108
fr: "french",
en: "english"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably add a sub key for that (right now we only have two but what if tomorrow we have 15?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants