Skip to content

Conversation

@elibosley
Copy link
Member

@elibosley elibosley commented Oct 24, 2025

Summary

  • stabilize the JsonForms i18n helper so locale and message updates reactively refresh translated fields
  • align Connect Settings and API Key forms with shared JsonForms props to reuse the configured renderers, AJV instance, and i18n state
  • add Vitest coverage that exercises JsonForms translation updates when locales change or new messages arrive

Testing

  • pnpm --filter web test (fails: known repository suites cannot resolve @unraid/ui/styles during lint-staged execution)

https://chatgpt.com/codex/tasks/task_e_68faf98cb39483238b1db61aa3e6106a

Summary by CodeRabbit

  • New Features

    • Enhanced form internationalization support enabling dynamic locale switching and real-time translation updates without page reloads
    • Added comprehensive translated error messages for form validation with support for multiple languages
  • Refactor

    • Simplified form component configuration management by consolidating property bindings
  • Tests

    • Added integration tests validating form internationalization functionality including locale switching and dynamic message updates

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 24, 2025

Walkthrough

A new i18n helper with translate and translateError functions is introduced, with reactive message synchronization via watchers. Vue components are refactored to consolidate JsonForms props into single bound objects. Comprehensive test coverage validates locale-switching and live message updates.

Changes

Cohort / File(s) Change Summary
Test Suite for i18n Helper
web/__test__/helpers/jsonforms-i18n.test.ts
New test file introducing TestHarness component, renderJsonForms helper, and two test cases validating translation switching and live message updates via mergeLocaleMessage.
I18n Helper Refactoring
web/src/helpers/jsonforms-i18n.ts
Added toStringIfNeeded utility, translate and translateError functions with fallback logic. Converted from computed return to reactive ref-based state with watchers for locale and message synchronization.
JsonForms Props Consolidation
web/src/components/ApiKey/ApiKeyCreate.vue, web/src/components/ConnectSettings/ConnectSettings.standalone.vue
Introduced jsonFormsProps computed property aggregating renderers, config, ajv, and i18n; replaced individual prop bindings with v-bind="jsonFormsProps".

Sequence Diagram

sequenceDiagram
    participant Component
    participant I18nHelper as useJsonFormsI18n
    participant Vue_I18n
    
    Note over Component,Vue_I18n: Initialization
    Component->>I18nHelper: Call useJsonFormsI18n()
    I18nHelper->>Vue_I18n: useI18n() to get locale, messages
    I18nHelper->>I18nHelper: Create ref<JsonFormsI18nState>
    I18nHelper->>I18nHelper: Set up watch(locale)
    I18nHelper->>I18nHelper: Set up watch(messages[locale])
    I18nHelper-->>Component: Return reactive state ref
    
    Note over Component,Vue_I18n: Locale Change
    Vue_I18n->>Vue_I18n: Update locale to French
    I18nHelper->>I18nHelper: Locale watcher triggers
    I18nHelper->>I18nHelper: Update state with new locale
    I18nHelper->>I18nHelper: Regenerate translate & translateError
    Component->>Component: Re-render with new translations
    
    Note over Component,Vue_I18n: Runtime Message Update
    Component->>Vue_I18n: mergeLocaleMessage(locale, newMessages)
    Vue_I18n->>Vue_I18n: Update messages[locale]
    I18nHelper->>I18nHelper: Messages watcher triggers (deep)
    I18nHelper->>I18nHelper: Regenerate translate & translateError
    Component->>Component: Re-render with updated messages
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The refactoring introduces new translation logic with fallback handling and reactive watchers in the helper, requiring careful verification of message prioritization and synchronization behavior. Component changes are repetitive and straightforward. Test coverage validates the i18n behavior across locale switching and live updates.

Poem

🐰 Translations dance across the globe,
French and English in a helpful robe,
Props now bundled, watchers keep the beat,
i18n state and tests—the change complete!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Fix JsonForms i18n integration and add coverage" directly aligns with the primary objectives of this pull request. It accurately captures the two main aspects of the changeset: the stabilization of the JsonForms i18n helper through reactive synchronization improvements (as evidenced by the changes to jsonforms-i18n.ts involving watch listeners for locale and message updates), and the addition of comprehensive test coverage (the new test suite in jsonforms-i18n.test.ts). The title is concise, clear, and specific enough that a teammate reviewing commit history would understand the primary focus without additional context.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/add-e2e-tests-for-jsonforms-translations

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

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +115 to +126
i18n.global.mergeLocaleMessage('en', {
jsonforms: {
test: {
provider: {
label: 'Provider label (updated)',
description: 'Updated English description',
},
},
},
errors: {
required: 'Field is required (updated)',
},

Choose a reason for hiding this comment

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

P1 Badge Use correct path when updating error translation messages

The new i18n test updates error strings via mergeLocaleMessage under a top‑level errors key, but translateError looks for messages under jsonforms.errors.*. As written, the merged value is never read, so the assertion that 'Field is required (updated)' appears will always time out and the spec cannot pass. Move the errors object inside jsonforms (mirroring the initial message structure) or adjust the code under test to read from the root key.

Useful? React with 👍 / 👎.

Copy link
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.

Actionable comments posted: 5

🧹 Nitpick comments (1)
web/src/helpers/jsonforms-i18n.ts (1)

47-69: Locale and message watchers ensure reactive translation updates.

The watchers correctly force state updates when locale or messages change, ensuring JsonForms re-renders with new translations. Note that when locale changes, both watchers trigger (first from locale change, second from the watched messages object changing from messages['en'] to messages['fr']), causing two state updates. This redundancy is harmless but could be optimized if needed.

Consider combining the watchers to eliminate the double update on locale change:

-  watch(
-    locale,
-    (currentLocale) => {
-      state.value = {
-        locale: currentLocale,
-        translate,
-        translateError,
-      };
-    },
-    { immediate: true }
-  );
-
-  watch(
-    () => messages.value?.[locale.value],
-    () => {
-      state.value = {
-        locale: locale.value,
-        translate,
-        translateError,
-      };
-    },
-    { deep: true }
-  );
+  watch(
+    [locale, () => messages.value?.[locale.value]],
+    ([currentLocale]) => {
+      state.value = {
+        locale: currentLocale,
+        translate,
+        translateError,
+      };
+    },
+    { immediate: true, deep: true }
+  );
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ff2906e and a6fd691.

📒 Files selected for processing (4)
  • web/__test__/helpers/jsonforms-i18n.test.ts (1 hunks)
  • web/src/components/ApiKey/ApiKeyCreate.vue (2 hunks)
  • web/src/components/ConnectSettings/ConnectSettings.standalone.vue (2 hunks)
  • web/src/helpers/jsonforms-i18n.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/components/**/*.vue

📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)

Some Vue files may require explicit imports like ref or computed due to Nuxt auto-imports not applying in tests

Files:

  • web/src/components/ConnectSettings/ConnectSettings.standalone.vue
  • web/src/components/ApiKey/ApiKeyCreate.vue
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}

📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)

{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly

Files:

  • web/__test__/helpers/jsonforms-i18n.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start

Files:

  • web/__test__/helpers/jsonforms-i18n.test.ts
  • web/src/helpers/jsonforms-i18n.ts
{api,web}/**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests

Files:

  • web/__test__/helpers/jsonforms-i18n.test.ts
web/__test__/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place Vue component tests under web/test; run with pnpm test

Files:

  • web/__test__/helpers/jsonforms-i18n.test.ts
web/__test__/**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

web/__test__/**/*.{test,spec}.{ts,tsx}: Use mount from Vue Test Utils for component testing
Stub complex child components that aren’t the focus of the test
Mock external dependencies and services in component tests
Test component behavior and output, not implementation details
Use createTestingPinia() for mocking stores in components
Prefer semantic queries like find('button') over data-test IDs
Use await nextTick() for DOM updates before assertions
For store tests, use createPinia() and setActivePinia
Only use createTestingPinia when its special features are needed
Let stores initialize with natural default state; don’t mock the store under test

Files:

  • web/__test__/helpers/jsonforms-i18n.test.ts
🧠 Learnings (1)
📚 Learning: 2025-03-12T13:35:43.900Z
Learnt from: pujitm
PR: unraid/api#1211
File: web/codegen.ts:14-14
Timestamp: 2025-03-12T13:35:43.900Z
Learning: The JSON scalar type in web/codegen.ts was temporarily changed from 'string' to 'any' for compatibility with JsonForms integration. This change facilitates the implementation of the Connect settings web component.

Applied to files:

  • web/src/components/ConnectSettings/ConnectSettings.standalone.vue
🧬 Code graph analysis (1)
web/__test__/helpers/jsonforms-i18n.test.ts (1)
web/src/helpers/jsonforms-i18n.ts (1)
  • useJsonFormsI18n (16-72)
🪛 GitHub Actions: CI - Main (API)
web/__test__/helpers/jsonforms-i18n.test.ts

[error] 1-1: Testing failure in jsonforms i18n tests: Unable to find an element with the text 'English description'. This indicates translations rendering did not match expectations in the active locale.

🪛 GitHub Check: Test API
web/__test__/helpers/jsonforms-i18n.test.ts

[failure] 91-91: test/helpers/jsonforms-i18n.test.ts > useJsonFormsI18n > responds to updated translations for the active locale
TestingLibraryElementError: Found multiple elements with the text: Provider label (en)

Here are the matching elements:

Ignored nodes: comments, script, style
<label
class="label required"
for="#/properties/provider-input"

Provider label (en)
<span
class="asterisk"

*

Ignored nodes: comments, script, style
<label
class="label required"
for="#/properties/provider2-input"

Provider label (en)
<span
class="asterisk"

*

(If this is intentional, then use the *AllBy* variant of the query (like queryAllByText, getAllByText, or findAllByText)).

Ignored nodes: comments, script, style

Provider label (en) *
    <input
      class="input"
      id="#/properties/provider-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
Provider label (en) *
    <input
      class="input"
      id="#/properties/provider2-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>

Ignored nodes: comments, script, style

Provider label (en) *
    <input
      class="input"
      id="#/properties/provider-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
Provider label (en) *
    <input
      class="input"
      id="#/properties/provider2-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
❯ waitForWrapper ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/wait-for.js:163:27 ❯ ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/query-helpers.js:86:33 ❯ renderJsonForms __test__/helpers/jsonforms-i18n.test.ts:91:16 ❯ __test__/helpers/jsonforms-i18n.test.ts:113:28

[failure] 101-101: test/helpers/jsonforms-i18n.test.ts > useJsonFormsI18n > translates labels, descriptions, and errors when the locale changes
TestingLibraryElementError: Unable to find an element with the text: English description. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

Ignored nodes: comments, script, style

Provider label (en) *
    <input
      class="input"
      id="#/properties/provider-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
❯ Object.getElementError ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/config.js:37:19 ❯ ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/query-helpers.js:76:38 ❯ ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/query-helpers.js:52:17 ❯ ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/query-helpers.js:95:19 ❯ __test__/helpers/jsonforms-i18n.test.ts:101:19

[failure] 91-91: test/helpers/jsonforms-i18n.test.ts > useJsonFormsI18n > responds to updated translations for the active locale
TestingLibraryElementError: Found multiple elements with the text: Provider label (en)

Here are the matching elements:

Ignored nodes: comments, script, style
<label
class="label required"
for="#/properties/provider-input"

Provider label (en)
<span
class="asterisk"

*

Ignored nodes: comments, script, style
<label
class="label required"
for="#/properties/provider2-input"

Provider label (en)
<span
class="asterisk"

*

(If this is intentional, then use the *AllBy* variant of the query (like queryAllByText, getAllByText, or findAllByText)).

Ignored nodes: comments, script, style

Provider label (en) *
    <input
      class="input"
      id="#/properties/provider-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
Provider label (en) *
    <input
      class="input"
      id="#/properties/provider2-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>

Ignored nodes: comments, script, style

Provider label (en) *
    <input
      class="input"
      id="#/properties/provider-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
Provider label (en) *
    <input
      class="input"
      id="#/properties/provider2-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
❯ waitForWrapper ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/wait-for.js:163:27 ❯ ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/query-helpers.js:86:33 ❯ renderJsonForms __test__/helpers/jsonforms-i18n.test.ts:91:16 ❯ __test__/helpers/jsonforms-i18n.test.ts:113:28

[failure] 101-101: test/helpers/jsonforms-i18n.test.ts > useJsonFormsI18n > translates labels, descriptions, and errors when the locale changes
TestingLibraryElementError: Unable to find an element with the text: English description. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

Ignored nodes: comments, script, style

Provider label (en) *
    <input
      class="input"
      id="#/properties/provider-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
❯ Object.getElementError ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/config.js:37:19 ❯ ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/query-helpers.js:76:38 ❯ ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/query-helpers.js:52:17 ❯ ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/query-helpers.js:95:19 ❯ __test__/helpers/jsonforms-i18n.test.ts:101:19
🔇 Additional comments (5)
web/src/helpers/jsonforms-i18n.ts (3)

6-14: LGTM: Type coercion utility is sound.

The toStringIfNeeded helper correctly handles strings, null/undefined, and other types, ensuring translation results are always strings as expected by JsonForms.


19-30: LGTM: Translation function with proper fallback chain.

The translate function correctly implements a fallback strategy: use translated value if available, otherwise defaultMessage, then id. The use of toStringIfNeeded ensures consistent string output.


32-39: LGTM: Error translation follows JsonForms conventions.

The translateError function correctly constructs i18n keys from error keywords and provides sensible fallbacks.

web/src/components/ConnectSettings/ConnectSettings.standalone.vue (1)

132-132: LGTM: v-bind spreads consolidated props.

Using v-bind="jsonFormsProps" cleanly applies all JsonForms configuration without repetitive individual bindings.

web/src/components/ApiKey/ApiKeyCreate.vue (1)

481-481: LGTM: Props spread with v-bind.

Clean application of consolidated JsonForms props.

Comment on lines +11 to +26
const schema = {
type: 'object',
properties: {
provider: {
type: 'string',
minLength: 1,
i18n: 'jsonforms.test.provider',
},
},
required: ['provider'],
} as const;

const uischema = {
type: 'Control',
scope: '#/properties/provider',
} as const;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Schema description not rendering in tests.

The test expects to find "English description" (line 101), but static analysis shows it's not rendered. The vanilla renderers may not display schema descriptions by default. Consider adding uischema options to enable description display:

 const uischema = {
   type: 'Control',
   scope: '#/properties/provider',
+  options: {
+    showDescription: true,
+  },
 } as const;

Alternatively, verify that the vanilla renderers support description rendering and whether additional configuration is needed.

🤖 Prompt for AI Agents
In web/__test__/helpers/jsonforms-i18n.test.ts around lines 11 to 26, the test
expects the string "English description" but the current schema and uischema
don't render schema descriptions by default; update the provider property's
schema to include a description field with the expected text and add uischema
options to enable description rendering (e.g., add an options object that turns
on description display supported by your vanilla renderers), or alternatively
adjust the test to use a renderer/config that shows schema descriptions if your
current renderers cannot; ensure the final setup actually renders the
description string the test asserts.

Comment on lines +78 to +94
const renderJsonForms = async (locale: 'en' | 'fr' = 'en') => {
const i18n = createI18n({
legacy: false,
locale,
messages,
});

const utils = render(TestHarness, {
global: {
plugins: [i18n],
},
});

await screen.findByText('Provider label (en)');

return { i18n, ...utils };
};
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Add cleanup between tests to prevent duplicate DOM elements.

Static analysis shows "Found multiple elements with the text: Provider label (en)" at line 91, indicating previous test renders aren't being cleaned up. Add cleanup to prevent test pollution:

-import { describe, expect, it } from 'vitest';
+import { afterEach, describe, expect, it } from 'vitest';
+import { cleanup } from '@testing-library/vue';

 // ... schemas and component ...

 describe('useJsonFormsI18n', () => {
+  afterEach(() => {
+    cleanup();
+  });
+
   it('translates labels, descriptions, and errors when the locale changes', async () => {

As per coding guidelines.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Check: Test API

[failure] 91-91: test/helpers/jsonforms-i18n.test.ts > useJsonFormsI18n > responds to updated translations for the active locale
TestingLibraryElementError: Found multiple elements with the text: Provider label (en)

Here are the matching elements:

Ignored nodes: comments, script, style
<label
class="label required"
for="#/properties/provider-input"

Provider label (en)
<span
class="asterisk"

*

Ignored nodes: comments, script, style
<label
class="label required"
for="#/properties/provider2-input"

Provider label (en)
<span
class="asterisk"

*

(If this is intentional, then use the *AllBy* variant of the query (like queryAllByText, getAllByText, or findAllByText)).

Ignored nodes: comments, script, style

Provider label (en) *
    <input
      class="input"
      id="#/properties/provider-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
Provider label (en) *
    <input
      class="input"
      id="#/properties/provider2-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>

Ignored nodes: comments, script, style

Provider label (en) *
    <input
      class="input"
      id="#/properties/provider-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
Provider label (en) *
    <input
      class="input"
      id="#/properties/provider2-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
❯ waitForWrapper ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/wait-for.js:163:27 ❯ ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/query-helpers.js:86:33 ❯ renderJsonForms __test__/helpers/jsonforms-i18n.test.ts:91:16 ❯ __test__/helpers/jsonforms-i18n.test.ts:113:28

[failure] 91-91: test/helpers/jsonforms-i18n.test.ts > useJsonFormsI18n > responds to updated translations for the active locale
TestingLibraryElementError: Found multiple elements with the text: Provider label (en)

Here are the matching elements:

Ignored nodes: comments, script, style
<label
class="label required"
for="#/properties/provider-input"

Provider label (en)
<span
class="asterisk"

*

Ignored nodes: comments, script, style
<label
class="label required"
for="#/properties/provider2-input"

Provider label (en)
<span
class="asterisk"

*

(If this is intentional, then use the *AllBy* variant of the query (like queryAllByText, getAllByText, or findAllByText)).

Ignored nodes: comments, script, style

Provider label (en) *
    <input
      class="input"
      id="#/properties/provider-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
Provider label (en) *
    <input
      class="input"
      id="#/properties/provider2-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>

Ignored nodes: comments, script, style

Provider label (en) *
    <input
      class="input"
      id="#/properties/provider-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
Provider label (en) *
    <input
      class="input"
      id="#/properties/provider2-input"
    />
    
  </div>
  <div
    class="error"
  >
    jsonforms.test.provider.error.custom
  </div>
</div>
❯ waitForWrapper ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/wait-for.js:163:27 ❯ ../node_modules/.pnpm/@testing-library+dom@9.3.4/node_modules/@testing-library/dom/dist/query-helpers.js:86:33 ❯ renderJsonForms __test__/helpers/jsonforms-i18n.test.ts:91:16 ❯ __test__/helpers/jsonforms-i18n.test.ts:113:28
🤖 Prompt for AI Agents
In web/__test__/helpers/jsonforms-i18n.test.ts around lines 78 to 94, tests are
leaving rendered DOM between runs causing "Found multiple elements with the
text: Provider label (en)"; add test cleanup to unmount/remove previous renders
by importing the testing-library cleanup helper and invoking it after each test
(e.g., add an afterEach(cleanup) hook) or call cleanup() in a beforeEach to
ensure a fresh DOM before renderJsonForms is used.

Comment on lines +112 to +134
it('responds to updated translations for the active locale', async () => {
const { i18n } = await renderJsonForms('en');

i18n.global.mergeLocaleMessage('en', {
jsonforms: {
test: {
provider: {
label: 'Provider label (updated)',
description: 'Updated English description',
},
},
},
errors: {
required: 'Field is required (updated)',
},
});

await nextTick();
await screen.findByText('Provider label (updated)');

expect(screen.getByText('Updated English description')).toBeTruthy();
expect(screen.getByText('Field is required (updated)')).toBeTruthy();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix incorrect translation key for error messages.

Line 124-126 updates errors.required, but the translateError function in the helper (line 33 of jsonforms-i18n.ts) constructs keys as jsonforms.errors.{keyword}. The error translation will not work as expected.

Apply this diff:

     i18n.global.mergeLocaleMessage('en', {
       jsonforms: {
         test: {
           provider: {
             label: 'Provider label (updated)',
             description: 'Updated English description',
           },
         },
+        errors: {
+          required: 'Field is required (updated)',
+        },
       },
-      errors: {
-        required: 'Field is required (updated)',
-      },
     });
📝 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
it('responds to updated translations for the active locale', async () => {
const { i18n } = await renderJsonForms('en');
i18n.global.mergeLocaleMessage('en', {
jsonforms: {
test: {
provider: {
label: 'Provider label (updated)',
description: 'Updated English description',
},
},
},
errors: {
required: 'Field is required (updated)',
},
});
await nextTick();
await screen.findByText('Provider label (updated)');
expect(screen.getByText('Updated English description')).toBeTruthy();
expect(screen.getByText('Field is required (updated)')).toBeTruthy();
});
it('responds to updated translations for the active locale', async () => {
const { i18n } = await renderJsonForms('en');
i18n.global.mergeLocaleMessage('en', {
jsonforms: {
test: {
provider: {
label: 'Provider label (updated)',
description: 'Updated English description',
},
},
errors: {
required: 'Field is required (updated)',
},
},
});
await nextTick();
await screen.findByText('Provider label (updated)');
expect(screen.getByText('Updated English description')).toBeTruthy();
expect(screen.getByText('Field is required (updated)')).toBeTruthy();
});
🤖 Prompt for AI Agents
In web/__test__/helpers/jsonforms-i18n.test.ts around lines 112 to 134, the test
merges error translations under the top-level key "errors.required" but the
helper's translateError builds keys as "jsonforms.errors.{keyword}", so the
error translation isn't found; update the merged locale messages to place the
error translation under jsonforms.errors.required (i.e., nest the errors object
inside the jsonforms object as jsonforms: { errors: { required: 'Field is
required (updated)' } }) so the translateError lookup matches and the test
asserts the updated error text correctly.

Comment on lines +109 to +121
const jsonFormsConfig = {
restrict: false,
trim: false,
};
const renderers = [...jsonFormsRenderers];
const jsonFormsProps = computed(() => ({
renderers,
config: jsonFormsConfig,
ajv: jsonFormsAjv,
i18n: jsonFormsI18n.value,
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

LGTM: Consistent JsonForms prop consolidation pattern.

The jsonFormsProps computed centralizes JsonForms configuration, matching the pattern in ConnectSettings.standalone.vue and promoting consistency across the codebase.

🤖 Prompt for AI Agents
In web/src/components/ApiKey/ApiKeyCreate.vue around lines 109 to 121, there is
no issue — the jsonFormsProps computed consolidates JsonForms configuration
consistently; leave this code as-is to match the pattern used in
ConnectSettings.standalone.vue and no change is required.

Comment on lines +97 to +102
const jsonFormsProps = computed(() => ({
renderers,
config: jsonFormsConfig,
ajv: jsonFormsAjv,
i18n: jsonFormsI18n.value,
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

LGTM: Consolidating JsonForms props improves maintainability.

The computed jsonFormsProps object centralizes JsonForms configuration (renderers, config, ajv, i18n) into a single source of truth, simplifying prop management and aligning with the pattern used in ApiKeyCreate.vue.

🤖 Prompt for AI Agents
In web/src/components/ConnectSettings/ConnectSettings.standalone.vue around
lines 97 to 102, the new computed jsonFormsProps consolidates renderers, config,
ajv, and i18n into a single prop object; keep this refactor as-is, ensure the
component consumers are updated to use jsonFormsProps instead of individual
props, and mirror the same pattern used in ApiKeyCreate.vue for consistency.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant