feat: improve i18n (lunaria) status page#2064
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe PR migrates Lunaria’s dashboard rendering to consume an I18nStatus shape (including per-locale Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lunaria/components.ts (1)
260-261: Avoid!afterfind().If
localizationsdoes not containlang, this throws before any fallback status can be rendered. A small guard keeps this exported helper safe if the per-file view is brought back.As per coding guidelines, "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".Safer fallback
- const localization = localizations.find(l => l.lang === lang)! + const localization = localizations.find(l => l.lang === lang) + if (!localization) { + return html`<td>${EmojiFileLink(null, 'missing')}</td>` + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8dd22553-934d-4d27-9362-b2a69b3d653e
📒 Files selected for processing (3)
lunaria/components.tslunaria/lunaria.tslunaria/styles.ts
|
We want that page at npmx.dev, check #1485 |
So we keep both |
|
I'll work to include changes from #1485 |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
@userquin Ready for review, thx! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/pages/translation-status.vue (1)
189-195: Sharedcopiedstate affects all copy buttons.The
copiedref fromuseClipboard()is shared across all locale entries. When a user copies missing keys for one locale, all "Copy keys" buttons will momentarily display "Copied" text until the state resets.Consider tracking copied state per locale if distinct feedback is desired.
♻️ Potential approach
const copiedLocale = ref<string | null>(null) function copyMissingKeys(localeEntry: I18nLocaleStatus) { // ... existing copy logic ... copy(fullTemplate) copiedLocale.value = localeEntry.lang setTimeout(() => { if (copiedLocale.value === localeEntry.lang) { copiedLocale.value = null } }, 2000) }Then in template:
copiedLocale === localeEntry.lang ? $t('common.copied') : $t('i18n.copy_keys')app/components/ProgressBar.vue (1)
105-107: Misplaced RTL styling.This CSS rule targets
details[dir='rtl']elements with.icon-rtlchildren, which are not part of thisProgressBarcomponent. Since this is a scoped style block, this rule will never match any elements.This styling should be moved to the parent component (
translation-status.vue) or a shared stylesheet where it can affect the relevant<details>elements.♻️ Proposed fix - remove from this file
progress.full::-moz-progress-bar { `@apply` bg-green-700 dark:bg-green-500; } - -details[dir='rtl']:not([open]) .icon-rtl { - transform: scale(-1, 1); -} </style>app/pages/settings.vue (1)
245-246: Use the locale value type in the model update handler (not the Ref type).Line 246 currently casts
$eventtotypeof currentLocale, which is aRef, not the emitted locale value. This weakens type-safety and can mask payload mismatches.Suggested fix
- v-model="currentLocale" - `@update`:modelValue="setLocale($event as typeof currentLocale)" + :modelValue="currentLocale" + `@update`:modelValue="setLocale($event as typeof currentLocale.value)"As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39c3f89f-e1d4-4854-8fb9-b7d8cab8a083
📒 Files selected for processing (18)
app/components/AppFooter.vueapp/components/ProgressBar.vueapp/components/Translation/StatusByFile.unused.vueapp/composables/useI18nStatus.tsapp/pages/settings.vueapp/pages/translation-status.vueconfig/i18n.tsi18n/locales/en.jsoni18n/locales/uk-UA.jsoni18n/schema.jsonknip.tslunaria/components.tslunaria/lunaria.tsnuxt.config.tsserver/middleware/canonical-redirects.global.tsshared/types/i18n-status.tstest/nuxt/a11y.spec.tstest/unit/a11y-component-coverage.spec.ts
✅ Files skipped from review due to trivial changes (5)
- nuxt.config.ts
- app/components/AppFooter.vue
- i18n/locales/uk-UA.json
- knip.ts
- config/i18n.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lunaria/lunaria.ts
| "by_file": "Translation progress by file", | ||
| "complete_text": "This translation is complete, amazing job!", | ||
| "missing_text": "missing", | ||
| "missing_keys": "There is no missing translations | Missing translation | Missing translations", |
There was a problem hiding this comment.
Fix grammar in the zero-count pluralisation string.
The zero-form text is currently ungrammatical (There is no missing translations).
Suggested fix
- "missing_keys": "There is no missing translations | Missing translation | Missing translations",
+ "missing_keys": "There are no missing translations | Missing translation | Missing translations",📝 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.
| "missing_keys": "There is no missing translations | Missing translation | Missing translations", | |
| "missing_keys": "There are no missing translations | Missing translation | Missing translations", |
|
Check translation status page, expanding progress in narrow/mobile screens will expand to full width. Missing translation status link on menu. |
…/improve-lunaria-status-page
…/improve-lunaria-status-page
Both were fixed Mobile view
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/pages/translation-status.vue (1)
57-65: Remove inlinefocus-visibleutility class from button.The
focus-visible:outline-accent/70class on this button should be removed. Focus-visible styling for buttons is handled globally viamain.css. Based on learnings: "Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements... Rely on the global rule for consistency and maintainability."♻️ Proposed fix
<button type="button" - class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" + class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0" `@click`="router.back()" v-if="canGoBack" >lunaria/components.ts (1)
162-180: Clamp progress before computing class and inline width.Line 179 currently trusts
percentCompleteas-is. A small defensive clamp avoids invalid widths/classes if upstream values drift outside0..100.Suggested refactor
const ProgressBar = (percentComplete: number): string => { - let barClass = 'completed' + const safePercent = Number.isFinite(percentComplete) + ? Math.min(100, Math.max(0, percentComplete)) + : 0 + let barClass = 'completed' - if (percentComplete > 99) { + if (safePercent > 99) { barClass = 'completed' // dark-green - } else if (percentComplete > 90) { + } else if (safePercent > 90) { barClass = 'very-good' // green - } else if (percentComplete > 75) { + } else if (safePercent > 75) { barClass = 'good' // orange - } else if (percentComplete > 50) { + } else if (safePercent > 50) { barClass = 'help-needed' // red } else { barClass = 'basic' // dark-red } return html` <div class="progress-bar-wrapper" aria-hidden="true"> - <div class="progress-bar ${barClass}" style="width:${percentComplete}%;"></div> + <div class="progress-bar ${barClass}" style="width:${safePercent}%;"></div> </div> ` }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e6b64a2-b1ac-4a01-94a2-395b53a95164
📒 Files selected for processing (6)
app/components/AppHeader.vueapp/pages/translation-status.vuei18n/locales/en.jsoni18n/schema.jsonlunaria/components.tstest/nuxt/a11y.spec.ts
✅ Files skipped from review due to trivial changes (2)
- test/nuxt/a11y.spec.ts
- i18n/schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- i18n/locales/en.json
| const generatedAt = computed(() => { | ||
| const gat = status.value?.generatedAt | ||
| if (import.meta.client) { | ||
| return (nuxt.isHydrated ? new Date().toISOString() : gat) ?? new Date().toISOString() | ||
| } | ||
|
|
||
| return gat ?? new Date().toISOString() | ||
| }) |
There was a problem hiding this comment.
Bug: generatedAt returns current time instead of actual generation timestamp after hydration.
When nuxt.isHydrated is true, this returns new Date().toISOString() (current browser time) rather than the actual gat value from the status payload. This displays incorrect information to users about when the translation status was generated.
🐛 Proposed fix
const generatedAt = computed(() => {
const gat = status.value?.generatedAt
if (import.meta.client) {
- return (nuxt.isHydrated ? new Date().toISOString() : gat) ?? new Date().toISOString()
+ return gat ?? new Date().toISOString()
}
return gat ?? new Date().toISOString()
})If there was a specific reason for showing current time post-hydration (e.g., hydration mismatch avoidance), consider using a client-only rendering approach for the timestamp instead.
📝 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 generatedAt = computed(() => { | |
| const gat = status.value?.generatedAt | |
| if (import.meta.client) { | |
| return (nuxt.isHydrated ? new Date().toISOString() : gat) ?? new Date().toISOString() | |
| } | |
| return gat ?? new Date().toISOString() | |
| }) | |
| const generatedAt = computed(() => { | |
| const gat = status.value?.generatedAt | |
| if (import.meta.client) { | |
| return gat ?? new Date().toISOString() | |
| } | |
| return gat ?? new Date().toISOString() | |
| }) |
| <ButtonBase type="button" size="small" @click="copyMissingKeys(localeEntry)"> | ||
| {{ | ||
| copied | ||
| ? $t('common.copied', {}, { locale: localeEntry.lang }) | ||
| : $t('i18n.copy_keys', {}, { locale: localeEntry.lang }) | ||
| }} | ||
| </ButtonBase> |
There was a problem hiding this comment.
Shared copied state shows "Copied" on all expanded locales simultaneously.
The copied ref from useClipboard is shared across all locale entries. When a user expands multiple locales and clicks "Copy keys" on one, all expanded entries will display "Copied" text, which is misleading.
Consider tracking the last-copied locale to scope the feedback:
🛠️ Suggested approach
+const lastCopiedLang = shallowRef<string | null>(null)
+
function copyMissingKeys(localeEntry: I18nLocaleStatus) {
const template = localeEntry.missingKeys.map(key => ` "${key}": ""`).join(',\n')
const fullTemplate = `// Missing translations for ${localeEntry.label} (${localeEntry.lang})
// Add these keys to: i18n/locales/${localeEntry.lang}.json
${template}`
copy(fullTemplate)
+ lastCopiedLang.value = localeEntry.lang
}Then in the template, check copied && lastCopiedLang === localeEntry.lang for the button text.
There was a problem hiding this comment.
LGTM
Thx ❤️
We can iterate later the progress colors, the gray background on light theme maybe should be more darker and on dark theme more lighter (maybe we can use gray at light on dark theme and viceversa or just use the text color we have for the percentage).
We can also remove the padding start in the progress and in the final span:
3 participants
🧭 Context
i18n status page was designed to track multiple files. Since we are using a single file for each location - it could be improved
📚 Description
The following changes were done:
jsonStatusobject instead of lunaria-generated status objectBefore
After
UPDATED
After conversation with @userquin integrated his changes from https://github.com/npmx-dev/npmx.dev/pull/1485/changes
/translation-statusScreenshots
Details