Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Jan 29, 2026

Benchmark PR from agentic-review-benchmarks#7

Summary by CodeRabbit

  • Tests
    • Centralized internationalization mock utilities for test files, consolidating scattered inline mocks into a reusable factory approach for more consistent test configuration.

✏️ Tip: You can customize this high-level summary in your review settings.

hyoban and others added 5 commits January 25, 2026 12:05
Consolidate scattered i18next mock implementations across test files into
a single source of truth. This reduces duplication and ensures consistent
mock behavior.

- Create test/i18n-mock.ts with reusable factory functions
- Update vitest.setup.ts to use the centralized helpers
- Remove redundant mock definitions from 8 test files
- Update testing.md documentation

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…eateReactI18nextMock` and detail global mock provisions.
@coderabbitai
Copy link

coderabbitai bot commented Jan 29, 2026

Walkthrough

This PR centralizes i18n (react-i18next) mocking across the test suite. A new test utility module web/test/i18n-mock.ts provides factory functions for creating configurable translation mocks. Multiple test files are refactored to either use this centralized factory via createReactI18nextMock() or rely on the global mock setup in web/vitest.setup.ts, eliminating scattered inline mock implementations.

Changes

Cohort / File(s) Summary
New i18n Mock Utility
web/test/i18n-mock.ts
Introduces factory functions (createTFunction, createUseTranslationMock, createTransMock, createReactI18nextMock) and TranslationMap type for lightweight, customizable react-i18next mocking in tests.
Global Test Setup
web/vitest.setup.ts
Replaces inline react-i18next mock with call to createReactI18nextMock() from the new test utility module.
Documentation
.claude/skills/frontend-testing/assets/component-test.template.tsx, .claude/skills/frontend-testing/references/mocking.md, web/testing/testing.md
Updated to document the new centralized mock factory pattern, usage examples, and deprecation of inline translation mocks.
Removed Local i18n Mocks
web/app/components/app/configuration/config/agent-setting-button.spec.tsx, web/app/components/app/configuration/config/config-audio.spec.tsx, web/app/components/billing/pricing/footer.spec.tsx, web/app/components/datasets/create/index.spec.tsx, web/app/components/datasets/documents/create-from-pipeline/processing/index.spec.tsx, web/app/components/plugins/install-plugin/install-from-local-package/steps/uploading.spec.tsx, web/app/components/plugins/plugin-detail-panel/subscription-list/create/common-modal.spec.tsx, web/app/components/plugins/plugin-detail-panel/subscription-list/create/oauth-client.spec.tsx, web/app/components/plugins/plugin-detail-panel/subscription-list/edit/index.spec.tsx
Removed inline i18n mocks; tests now rely on the global mock configured in vitest setup.
Replaced with Factory Function
web/app/components/base/inline-delete-confirm/index.spec.tsx, web/app/components/base/input-with-copy/index.spec.tsx, web/app/components/base/input/index.spec.tsx
Replaced inline i18n mocks with createReactI18nextMock() factory, passing explicit translation keys as parameters.
Removed Multiple i18n Hooks
web/app/components/plugins/card/index.spec.tsx, web/app/components/plugins/plugin-mutation-model/index.spec.tsx
Removed mocks for useTranslation, useMixedTranslation, and useGetLanguage hooks, relying on global/external mocks instead.
Updated Mock with Dynamic Import
web/app/components/plugins/install-plugin/install-from-local-package/steps/install.spec.tsx
Refactored react-i18next mock to use async dynamic import and merge helpers from the centralized i18n-mock module while retaining custom Trans implementation.
Updated Mock Behavior
web/app/components/plugins/marketplace/index.spec.tsx
Modified mock to use getFixedT with namespace prefixing; updated test expectations to assert on namespaced translation keys (e.g., plugin.category.all).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Mocks once scattered, now consolidated,
A factory springs forth, translations articulated,
From inline chaos to centralized grace,
Tests now hop together at a steady pace!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is minimal and only references a benchmark PR link, missing required sections like Summary, Screenshots, and specific issue association with proper syntax. Add a detailed Summary section explaining the changes and motivation, link the associated issue with 'Fixes #', and include Before/After screenshots if applicable.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'test: unify i18next mocks into centralized helpers' is concise and accurately summarizes the main change: consolidating scattered i18next mock implementations into centralized helpers.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches
  • 📝 Generate docstrings

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

Copy link

@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: 1

🤖 Fix all issues with AI agents
In `@web/test/i18n-mock.ts`:
- Around line 6-9: Update the docstring in web/test/i18n-mock.ts to match the
actual behavior of the t function: state that the implementation checks the
namespaced key (fullKey / `${ns}.${key}`) first, then falls back to the simple
key (`key`), and finally returns the namespaced key as the default fallback;
reference the t function / createT helper and the variables fullKey and key when
editing the comment to accurately reflect the precedence order used in the code.
🧹 Nitpick comments (1)
web/test/i18n-mock.ts (1)

4-4: Consider simplifying the interface declaration.

The empty interface body {} is redundant since it only extends Record<string, string | string[]>. Consider using a type alias instead.

♻️ Suggested simplification
-interface TranslationMap extends Record<string, string | string[]> {}
+type TranslationMap = Record<string, string | string[]>
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00585fa and 3c622b1.

📒 Files selected for processing (21)
  • .claude/skills/frontend-testing/assets/component-test.template.tsx
  • .claude/skills/frontend-testing/references/mocking.md
  • web/app/components/app/configuration/config/agent-setting-button.spec.tsx
  • web/app/components/app/configuration/config/config-audio.spec.tsx
  • web/app/components/base/inline-delete-confirm/index.spec.tsx
  • web/app/components/base/input-with-copy/index.spec.tsx
  • web/app/components/base/input/index.spec.tsx
  • web/app/components/billing/pricing/footer.spec.tsx
  • web/app/components/datasets/create/index.spec.tsx
  • web/app/components/datasets/documents/create-from-pipeline/processing/index.spec.tsx
  • web/app/components/plugins/card/index.spec.tsx
  • web/app/components/plugins/install-plugin/install-from-local-package/steps/install.spec.tsx
  • web/app/components/plugins/install-plugin/install-from-local-package/steps/uploading.spec.tsx
  • web/app/components/plugins/marketplace/index.spec.tsx
  • web/app/components/plugins/plugin-detail-panel/subscription-list/create/common-modal.spec.tsx
  • web/app/components/plugins/plugin-detail-panel/subscription-list/create/oauth-client.spec.tsx
  • web/app/components/plugins/plugin-detail-panel/subscription-list/edit/index.spec.tsx
  • web/app/components/plugins/plugin-mutation-model/index.spec.tsx
  • web/test/i18n-mock.ts
  • web/testing/testing.md
  • web/vitest.setup.ts
💤 Files with no reviewable changes (11)
  • web/app/components/plugins/card/index.spec.tsx
  • web/app/components/datasets/create/index.spec.tsx
  • web/app/components/plugins/plugin-detail-panel/subscription-list/create/oauth-client.spec.tsx
  • web/app/components/datasets/documents/create-from-pipeline/processing/index.spec.tsx
  • web/app/components/plugins/plugin-detail-panel/subscription-list/edit/index.spec.tsx
  • web/app/components/app/configuration/config/config-audio.spec.tsx
  • web/app/components/plugins/plugin-mutation-model/index.spec.tsx
  • web/app/components/plugins/plugin-detail-panel/subscription-list/create/common-modal.spec.tsx
  • web/app/components/plugins/install-plugin/install-from-local-package/steps/uploading.spec.tsx
  • web/app/components/app/configuration/config/agent-setting-button.spec.tsx
  • web/app/components/billing/pricing/footer.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
web/app/components/base/inline-delete-confirm/index.spec.tsx (1)
web/test/i18n-mock.ts (1)
  • createReactI18nextMock (74-79)
web/app/components/base/input/index.spec.tsx (1)
web/test/i18n-mock.ts (1)
  • createReactI18nextMock (74-79)
web/app/components/base/input-with-copy/index.spec.tsx (1)
web/test/i18n-mock.ts (1)
  • createReactI18nextMock (74-79)
web/vitest.setup.ts (1)
web/test/i18n-mock.ts (1)
  • createReactI18nextMock (74-79)
web/app/components/plugins/install-plugin/install-from-local-package/steps/install.spec.tsx (1)
web/test/i18n-mock.ts (1)
  • createReactI18nextMock (74-79)
🔇 Additional comments (16)
.claude/skills/frontend-testing/references/mocking.md (1)

56-77: Documentation updates look good.

The documentation clearly explains the global mock behavior and when to override with custom translations. The example using createReactI18nextMock is consistent with the actual implementation in web/test/i18n-mock.ts.

web/test/i18n-mock.ts (2)

39-49: LGTM - useTranslation mock is appropriately simple for testing.

The mock provides the essential t function and i18n object. Tests can use fully qualified translation keys (e.g., 'namespace.key') in their translation maps to handle namespacing.


74-79: Clean composition pattern for the combined mock.

The factory function correctly combines both useTranslation and Trans mocks, providing a convenient single-function API for most test scenarios.

web/vitest.setup.ts (1)

89-96: Good use of centralized mock factory in global setup.

The implementation correctly:

  1. Imports the actual react-i18next module to preserve other exports
  2. Uses dynamic import for the mock helper (required since mocks are hoisted)
  3. Spreads the centralized mock to override useTranslation and Trans

This ensures all tests get consistent i18n mock behavior by default.

.claude/skills/frontend-testing/assets/component-test.template.tsx (1)

31-39: Template comments updated correctly.

The updated comments clearly document:

  • What the global mock provides (useTranslation, Trans, useMixedTranslation, useGetLanguage)
  • When no local override is needed
  • How to override with custom translations using the factory function

This provides good guidance for developers writing new tests.

web/app/components/plugins/install-plugin/install-from-local-package/steps/install.spec.tsx (1)

67-80: Good use of centralized mock with custom Trans override.

This test correctly:

  1. Imports the actual module with importOriginal
  2. Uses the centralized createReactI18nextMock() for useTranslation
  3. Provides a custom Trans implementation that handles the components.trustSource prop needed by this specific component

This is the appropriate pattern when tests need specialized mock behavior beyond what the centralized helper provides.

web/app/components/base/input/index.spec.tsx (1)

3-10: Clean migration to centralized i18n mock.

The test correctly uses createReactI18nextMock with the specific translations needed for assertions (e.g., testing that the placeholder shows "Please input" and the search input shows "Search"). This replaces what was likely a more verbose inline mock.

web/app/components/base/inline-delete-confirm/index.spec.tsx (1)

3-12: Centralized i18n mock wiring looks good. The helper-based mock with explicit translations keeps assertions deterministic and consistent across tests.

web/app/components/plugins/marketplace/index.spec.tsx (6)

33-40: Namespace-aware getFixedT mock matches intended behavior. The prefixing logic aligns downstream expectations with the updated i18n strategy.


620-622: Expectation updated for namespace prefix. Matches the new mock behavior.


641-642: Namespaced key assertion is aligned with getFixedT. Looks consistent.


2759-2767: Namespaced label expectations are consistent with the mock. Good update.


2797-2797: Click target now uses namespaced label. This keeps the interaction test in sync with mock output.


2819-2819: Active state lookup uses namespaced key. Consistent with updated render text.

web/app/components/base/input-with-copy/index.spec.tsx (1)

3-19: Centralized i18n mock adoption looks good. The explicit translations keep copy-state assertions deterministic.

web/testing/testing.md (1)

332-352: Docs now clearly reflect the centralized i18n mock flow. The guidance and example should keep test mocks consistent.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment on lines +6 to +9
/**
* Create a t function with optional custom translations
* Checks translations[key] first, then translations[ns.key], then returns ns.key as fallback
*/
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Docstring is inconsistent with implementation.

The docstring says "Checks translations[key] first, then translations[ns.key]" but the actual implementation (lines 16-21) checks fullKey (ns.key) first, then falls back to key. The implementation is correct (more specific key should take precedence), but the docstring should be updated.

📝 Suggested fix
 /**
  * Create a t function with optional custom translations
- * Checks translations[key] first, then translations[ns.key], then returns ns.key as fallback
+ * Checks translations[ns.key] first, then translations[key], then returns ns.key as fallback
  */
📝 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
/**
* Create a t function with optional custom translations
* Checks translations[key] first, then translations[ns.key], then returns ns.key as fallback
*/
/**
* Create a t function with optional custom translations
* Checks translations[ns.key] first, then translations[key], then returns ns.key as fallback
*/
🤖 Prompt for AI Agents
In `@web/test/i18n-mock.ts` around lines 6 - 9, Update the docstring in
web/test/i18n-mock.ts to match the actual behavior of the t function: state that
the implementation checks the namespaced key (fullKey / `${ns}.${key}`) first,
then falls back to the simple key (`key`), and finally returns the namespaced
key as the default fallback; reference the t function / createT helper and the
variables fullKey and key when editing the comment to accurately reflect the
precedence order used in the code.

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.

4 participants