Skip to content

fix(i18n): update language switch immediately without refresh#2525

Merged
graphieros merged 8 commits intonpmx-dev:mainfrom
BittuBarnwal7479:fix/header-nav-i18n-and-settings-icon
Apr 20, 2026
Merged

fix(i18n): update language switch immediately without refresh#2525
graphieros merged 8 commits intonpmx-dev:mainfrom
BittuBarnwal7479:fix/header-nav-i18n-and-settings-icon

Conversation

@BittuBarnwal7479
Copy link
Copy Markdown
Contributor

@BittuBarnwal7479 BittuBarnwal7479 commented Apr 14, 2026

🔗 Linked issue

#2526

🧭 Context

When switching language from Settings, the first change didn’t fully apply unless the page was refreshed.
This PR fixes both so the UI updates immediately and stays consistent.

📚 Description

I updated the language-change flow in Settings so locale updates are applied through the same path every time, including the first switch. That removes the partial-translation state and the refresh workaround.

I tested this manually by:

switching locales multiple times from the Settings page
checking header icons immediately after each switch
confirming shortcuts still navigate correctly

Before:

npmx.mp4

After:

Recording.2026-04-15.010332.mp4

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Apr 19, 2026 2:09pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Apr 19, 2026 2:09pm
npmx-lunaria Ignored Ignored Apr 19, 2026 2:09pm

Request Review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8503b4a8-33c3-4e14-9869-b13f80f37127

📥 Commits

Reviewing files that changed from the base of the PR and between 396cb0f and 50da992.

📒 Files selected for processing (1)
  • app/pages/settings.vue
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/pages/settings.vue

📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Improved language selection on the Settings page: locale reads are now proxied, writes are validated against known locales and persist to settings.
    • Simplified locale update flow: selection uses v-model-driven updates and no longer requires the previous explicit update handler, reducing unexpected behaviour when changing language.
    • Removed inline Open Graph image generation from the Settings page.

Walkthrough

The settings page locale handling was refactored: a computed currentLocale now proxies reads from locale.value and validates/writes new values (updating settings.value.selectedLocale and calling setNuxti18nLocale()); the previous v-model/@update:modelValue wiring and setLocale wrapper were removed.

Changes

Cohort / File(s) Summary
Locale State Refactoring
app/pages/settings.vue
Replaced destructured useI18n() locale usage with a computed currentLocale getter/setter that reads locale.value, validates new locale against locales.value.map(loc => loc.code), updates settings.value.selectedLocale, and calls setNuxti18nLocale(newLocale) only for recognised non-empty values. Removed the setLocale helper, the explicit @update:modelValue handler, and the defineOgImage(...) block.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and accurately summarizes the main change: fixing immediate language switching without requiring a page refresh.
Description check ✅ Passed The description is clearly related to the changeset, explaining the problem, the solution applied to settings.vue, and providing manual testing evidence.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

Hello! Thank you for opening your first PR to npmx, @BittuBarnwal7479! 🚀

Here’s what will happen next:

  1. Our GitHub bots will run to check your changes.
    If they spot any issues you will see some error messages on this PR.
    Don’t hesitate to ask any questions if you’re not sure what these mean!

  2. In a few minutes, you’ll be able to see a preview of your changes on Vercel

  3. One or more of our maintainers will take a look and may ask you to make changes.
    We try to be responsive, but don’t worry if this takes a few days.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2026

Codecov Report

❌ Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
app/pages/settings.vue 0.00% 9 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/components/AppHeader.vue (1)

313-320: ⚠️ Potential issue | 🟡 Minor

Keep ariaKeyshortcuts on these header links.

LinkBase still uses its ariaKeyshortcuts prop to set the aria-keyshortcuts attribute and render the visible <kbd> hint in app/components/Link/Base.vue:108-131. Adding :classicon fixes the icon, but dropping the shortcut prop regresses both accessibility metadata and the desktop hint badge for Compare and Settings.

Proposed fix
 <LinkBase
   v-for="link in desktopLinks"
   :key="link.name"
   class="border-none"
   variant="button-secondary"
   :to="link.to"
   :classicon="link.iconClass"
+  :ariaKeyshortcuts="link.keyshortcut"
 >
   {{ link.label }}
 </LinkBase>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/components/AppHeader.vue` around lines 313 - 320, The header LinkBase
loop removed the ariaKeyshortcuts prop, which breaks the aria-keyshortcuts
attribute and the visible <kbd> hint rendered by Link/Base.vue (see render logic
around lines 108-131); restore the binding by passing ariaKeyshortcuts from each
link (e.g., :ariaKeyshortcuts="link.ariaKeyshortcuts") in the v-for so Compare
and Settings keep their accessibility metadata and desktop hint badge while
retaining the :classicon change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@app/components/AppHeader.vue`:
- Around line 313-320: The header LinkBase loop removed the ariaKeyshortcuts
prop, which breaks the aria-keyshortcuts attribute and the visible <kbd> hint
rendered by Link/Base.vue (see render logic around lines 108-131); restore the
binding by passing ariaKeyshortcuts from each link (e.g.,
:ariaKeyshortcuts="link.ariaKeyshortcuts") in the v-for so Compare and Settings
keep their accessibility metadata and desktop hint badge while retaining the
:classicon change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b29319e8-2afa-4fbd-8bfc-38a0cbfcb883

📥 Commits

Reviewing files that changed from the base of the PR and between e331d86 and b0a3548.

📒 Files selected for processing (2)
  • app/components/AppHeader.vue
  • app/pages/settings.vue

@BittuBarnwal7479 BittuBarnwal7479 changed the title Fix/header nav i18n and settings icon fix: update language switch immediately and show settings icon in header Apr 14, 2026
Copy link
Copy Markdown
Contributor

@romansp romansp left a comment

Choose a reason for hiding this comment

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

Thanks for looking into that as this was bugging me too.

Let's keep this PR minimal though and fix language switch issue only.

Icons and shortcuts in AppHeader seem to be this way by design right now. I don't know the reason behind choosing , and s might actually be better. Anyways, I suggest to revert this part. Let's open a new issue if you think that's a problem and we address it there instead.

@BittuBarnwal7479
Copy link
Copy Markdown
Contributor Author

Thanks for looking into that as this was bugging me too.

Let's keep this PR minimal though and fix language switch issue only.

Icons and shortcuts in AppHeader seem to be this way by design right now. I don't know the reason behind choosing , and s might actually be better. Anyways, I suggest to revert this part. Let's open a new issue if you think that's a problem and we address it there instead.

Thanks, makes sense. I’ll keep this PR minimal and revert the AppHeader icon changes so it only fixes the language switch issue.

@BittuBarnwal7479 BittuBarnwal7479 changed the title fix: update language switch immediately and show settings icon in header fix(i18n): update language switch immediately without refresh Apr 15, 2026
@BittuBarnwal7479
Copy link
Copy Markdown
Contributor Author

@romansp please review.

@romansp
Copy link
Copy Markdown
Contributor

romansp commented Apr 15, 2026

This looks good to me. Thanks @BittuBarnwal7479.

To whoever will be merging I suggest use squash strategy to avoid revert commits appearing on main.

@graphieros graphieros added the needs review This PR is waiting for a review from a maintainer label Apr 17, 2026
Copy link
Copy Markdown
Contributor

@graphieros graphieros left a comment

Choose a reason for hiding this comment

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

Thank you for your PR :)
Please address the failing types test

Comment thread app/pages/settings.vue Outdated
:items="locales.map(loc => ({ label: loc.name ?? '', value: loc.code }))"
v-model="currentLocale"
@update:modelValue="setLocale($event as typeof currentLocale)"
:modelValue="currentLocale"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

currentLocale is evaluated as a locale string, not a string, hence the failing CI type test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@graphieros graphieros added this pull request to the merge queue Apr 20, 2026
Merged via the queue into npmx-dev:main with commit 13ac6c2 Apr 20, 2026
21 checks passed
@github-actions
Copy link
Copy Markdown

Thanks for your first contribution, @BittuBarnwal7479! 🤩

We'd love to welcome you to the npmx community. Come and say hi on Discord! And once you've joined, visit npmx.wamellow.com to claim the contributor role.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs review This PR is waiting for a review from a maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants