fix: new style of language select #4422
Conversation
Nikuunj
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
WalkthroughReplaced native Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant LS as LanguageSelect
participant CFG as i18nextConfig/langMap
U->>LS: Click trigger button
LS->>LS: Toggle open state (show dropdown)
U->>LS: Click option (language)
LS->>LS: Call onChange(value), close dropdown
LS->>CFG: Resolve display label (if needed)
CFG-->>LS: Label or fallback
LS-->>U: Render selected label
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Nikuunj
changed the title
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4422--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
components/languageSelector/LanguageSelect.tsx (5)
1-5: Fix lint: import order and quotes to satisfy ESLint/prettier.Standardize to single quotes and move the
@/…import before relative imports. This should clear the pipeline errors.-import React, { useState } from "react"; +import React, { useState } from 'react'; import { twMerge } from 'tailwind-merge'; -import IconLanguage from "../icons/Language"; -import i18nextConfig from '@/next-i18next.config.cjs'; -import type { SelectProps } from "../form/Select"; +import i18nextConfig from '@/next-i18next.config.cjs'; +import IconLanguage from '../icons/Language'; +import type { SelectProps } from '../form/Select';
16-16: Type the CJS config to avoidanyand runtime surprises.Make
langMapexplicit; many TS setups treat.cjsasany.- const { langMap } = i18nextConfig; + const { langMap } = i18nextConfig as { langMap: Record<string, string> };
29-35: Remove trailing spaces and use twMerge idiomatically.Multiline template string introduces trailing spaces and fails prettier. Pass classes and
classNameas separate args.- className={twMerge( - `flex items-center w-full justify-between - rounded-md border border-gray-300 bg-white - px-3 py-2 text-sm text-gray-700 shadow-sm - hover:bg-gray-50 focus:outline-none ${className}` - )} + className={twMerge( + 'flex items-center w-full justify-between rounded-md border border-gray-300 bg-white px-3 py-2 text-sm text-gray-700 shadow-sm hover:bg-gray-50', + // keep focus styles accessible; see next comment + className + )}
46-47: Z-index and portal consideration.
z-10can be clipped under headers/overlays. Considerz-50or a portal if used inside modals/toolbars.If this renders inside positioned containers, validate visually in the Netlify preview across pages.
21-21: i18n placeholder.“Select language” should come from translations, not a hardcoded string.
- : 'Select language'; + : t?.('language.select') ?? 'Select language';If not using
useTranslationhere, wire it in or pass aplaceholderprop.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/languageSelector/LanguageSelect.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/languageSelector/LanguageSelect.tsx (1)
components/icons/Language.tsx (1)
IconLanguage(7-24)
🪛 GitHub Actions: PR testing - if Node project
components/languageSelector/LanguageSelect.tsx
[error] 1-66: Multiple linting errors detected in LanguageSelector.tsx: import order, prettier/prettier, quotes, and trailing spaces.
🔇 Additional comments (1)
components/languageSelector/LanguageSelect.tsx (1)
14-16: State/structure LGTM.Local
openstate and simple toggle are fine for this component’s scope.
| const selectedOption = options.find((o) => o.value === selected); | ||
| const selectedDisplayText = selectedOption | ||
| ? (langMap[selectedOption.text.toLowerCase() as keyof typeof langMap] || selectedOption.text) | ||
| : "Select"; |
There was a problem hiding this comment.
Derive labels by value, not display text; localize the placeholder.
Keying langMap by text.toLowerCase() is brittle. Prefer option.value (e.g., en, fr). Also, “Select” should be i18n‑ed.
- const selectedDisplayText = selectedOption
- ? (langMap[selectedOption.text.toLowerCase() as keyof typeof langMap] || selectedOption.text)
- : "Select";
+ const selectedDisplayText = selectedOption
+ ? (langMap[selectedOption.value as keyof typeof langMap] ?? selectedOption.text ?? selectedOption.value)
+ : 'Select language';
@@
- {langMap[option.text.toLowerCase() as keyof typeof langMap] || option.text}
+ {langMap[option.value as keyof typeof langMap] ?? option.text ?? option.value}Also applies to: 59-60
🤖 Prompt for AI Agents
In components/languageSelector/LanguageSelect.tsx around lines 18-21 (and
similarly at 59-60), the code keys langMap by selectedOption.text.toLowerCase()
and uses a hardcoded "Select" placeholder; change to derive display labels from
option.value (e.g., 'en', 'fr') when looking up langMap so it’s robust to
display text changes, and replace the hardcoded "Select" with a localized
placeholder via the i18n/localization utility used in the app; update both
occurrences so they use option.value as the lookup key and call the localization
function for the empty-state label.
| type="button" | ||
| onClick={() => setOpen((prev) => !prev)} | ||
| className={twMerge( | ||
| `flex items-center w-full justify-between | ||
| rounded-md border border-gray-300 bg-white | ||
| px-3 py-2 text-sm text-gray-700 shadow-sm | ||
| hover:bg-gray-50 focus:outline-none ${className}` | ||
| )} | ||
| > | ||
| <span className="flex items-center gap-2"> | ||
| <IconLanguage className="text-gray-600" /> | ||
| {selectedDisplayText} | ||
| </span> | ||
| <span className="ml-2 text-gray-500">▾</span> | ||
| </button> | ||
|
|
There was a problem hiding this comment.
A11y: add ARIA state and restore visible focus.
Custom select lost native semantics. Add aria-* on the trigger and a proper focus ring; avoid focus:outline-none without replacement.
<button
- type="button"
- onClick={() => setOpen((prev) => !prev)}
+ type='button'
+ aria-haspopup='listbox'
+ aria-expanded={open}
+ aria-controls='language-select-listbox'
+ onClick={() => setOpen((prev) => !prev)}
className={twMerge(
- 'flex items-center w-full justify-between rounded-md border border-gray-300 bg-white px-3 py-2 text-sm text-gray-700 shadow-sm hover:bg-gray-50',
+ 'flex items-center w-full justify-between rounded-md border border-gray-300 bg-white px-3 py-2 text-sm text-gray-700 shadow-sm hover:bg-gray-50 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-blue-500 focus-visible:ring-offset-2',
className
)}
>
<span className="flex items-center gap-2">
- <IconLanguage className="text-gray-600" />
+ <IconLanguage className='text-gray-600 dark:text-gray-300' />
{selectedDisplayText}
</span>
- <span className="ml-2 text-gray-500">▾</span>
+ <span className='ml-2 text-gray-500' aria-hidden>▾</span>
</button>📝 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.
| <button | |
| type="button" | |
| onClick={() => setOpen((prev) => !prev)} | |
| className={twMerge( | |
| `flex items-center w-full justify-between | |
| rounded-md border border-gray-300 bg-white | |
| px-3 py-2 text-sm text-gray-700 shadow-sm | |
| hover:bg-gray-50 focus:outline-none ${className}` | |
| )} | |
| > | |
| <span className="flex items-center gap-2"> | |
| <IconLanguage className="text-gray-600" /> | |
| {selectedDisplayText} | |
| </span> | |
| <span className="ml-2 text-gray-500">▾</span> | |
| </button> | |
| <button | |
| type='button' | |
| aria-haspopup='listbox' | |
| aria-expanded={open} | |
| aria-controls='language-select-listbox' | |
| onClick={() => setOpen((prev) => !prev)} | |
| className={twMerge( | |
| 'flex items-center w-full justify-between rounded-md border border-gray-300 bg-white px-3 py-2 text-sm text-gray-700 shadow-sm hover:bg-gray-50 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-blue-500 focus-visible:ring-offset-2', | |
| className | |
| )} | |
| > | |
| <span className="flex items-center gap-2"> | |
| <IconLanguage className='text-gray-600 dark:text-gray-300' /> | |
| {selectedDisplayText} | |
| </span> | |
| <span className='ml-2 text-gray-500' aria-hidden>▾</span> | |
| </button> |
🤖 Prompt for AI Agents
In components/languageSelector/LanguageSelect.tsx around lines 26 to 41, the
trigger button is missing ARIA state and the visible focus ring was removed; add
aria-haspopup="listbox", aria-expanded={open} and aria-controls="<listbox-id>"
(ensure the listbox element uses that id) so assistive tech sees the
relationship, and remove the insecure focus:outline-none replacement by adding
visible focus styles (e.g. focus-visible:ring-2 focus-visible:ring-offset-2
focus-visible:ring-indigo-500 or your design system's focus classes) so the
button retains keyboard focus indication while preserving native semantics.
🧩 Analysis chain
Preserve test selectors to avoid breaking E2E/unit tests.
data-testid was removed; add stable IDs on the trigger and options.
Run to find failing selectors and update tests accordingly:
Also applies to: 49-61
🏁 Script executed:
#!/bin/bash
# Look for old selectors in tests (adjust patterns as needed)
rg -nP -C2 "(data-testid=['\"]language|getByRole\\('combobox'\\)|getByTestId\\(['\"]language" --type ts --type tsx --type js --type jsx --type=spec --type=testLength of output: 72
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching repo for tests referencing language-related selectors..."
echo "1) data-testid=\"language\" or data-testid='language'"
rg -n -S -C2 "data-testid=[\"']language[\"']" --hidden --no-ignore || true
echo "2) getByTestId('language') / getByTestId(\"language\")"
rg -n -S -C2 "getByTestId\\([\"']language[\"']\\)" --hidden --no-ignore || true
echo "3) getByRole('combobox') / getByRole(\"combobox\")"
rg -n -S -C2 "getByRole\\([\"']combobox[\"']\\)" --hidden --no-ignore || true
echo "4) Broader: lines that mention data-testid and language together (if any)"
rg -n -S -C2 "data-testid" --hidden --no-ignore | rg -n "language" || true
echo "Search complete."Length of output: 419
Re-add stable test selectors (data-testid) to the language selector trigger and options.
data-testid='MobileNav-language-item' was found at components/navigation/MobileNavMenu.tsx:152 — removing data-testid from components/languageSelector/LanguageSelect.tsx (lines 26–41; options ~49–61) will likely break tests. Restore stable data-testid attributes on the trigger and each option (or update tests to use robust queries like getByRole/getByLabelText).
🤖 Prompt for AI Agents
In components/languageSelector/LanguageSelect.tsx around lines 26 to 41 (and
options around lines 49 to 61), the data-testid attributes used by tests were
removed from the language selector trigger and option elements; re-add stable
data-testid attributes—e.g., data-testid="MobileNav-language-item" (or the
original test id used in MobileNavMenu.tsx) on the trigger button and a
consistent data-testid on each option element—so tests remain stable, ensuring
the identifiers match existing tests or update tests to use accessible queries
instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4422 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 778 778
Branches 144 144
=========================================
Hits 778 778 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
FYI: #4388 (comment) |
3 participants
Description
issue - #4388
This PR introduces the new new design of language select.
✅ Applied updated design across components
✅ update style of language select
Summary by CodeRabbit
New Features
Refactor