refactor(ui): add tabs underline variant and adopt across surfaces#1571
Merged
Conversation
- Extend `Tabs` with `variant` ('pill' | 'underline') and `triggerClassName`
- Remove `LocaleTabs` in favor of the underline `Tabs` variant
- Adopt `Tabs` in notification bell, onedrive picker, conversation starters
- Rename dialog and dropdown background token from bg-muted to bg-card
- Introduce `--tab` CSS var; tune dark-mode border/muted tokens
- Replace locale/theme tabs in user-button with inline button group
- Minor cosmetic polish for arena selector, dictation button, upload dialogs
6 tasks
📝 WalkthroughWalkthroughThis PR removes the Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/components/ui/navigation/tabs.tsx`:
- Line 52: The code uses truthy checks for tab content (e.g., const hasContent =
items.some((item) => item.content)) which will treat valid ReactNode values like
0 or '' as missing; change these to explicit undefined/null checks. Update
hasContent to use item.content !== undefined && item.content !== null (or
item.content != null) and similarly replace any conditional rendering checks
around item.content (the rendering logic that shows panels for each item) to
explicitly check against undefined/null so zero or empty-string content still
renders.
- Around line 24-39: The current named-variant styling uses plain object
literals (listStyles and triggerStyles) instead of the project's CVA pattern;
convert both listStyles and triggerStyles into cva-based variant factories:
import cva from class-variance-authority (or the project's cva helper) and
create cva() calls that place the shared base classes as the first argument and
declare a variants object with keys for the named variant values ('pill' and
'underline') containing the corresponding class strings, and set a sensible
defaultVariants.variant if needed; ensure the exported names (listStyles and
triggerStyles) remain the same so the Tabs component's variant prop continues to
work unchanged.
In `@services/platform/app/components/user-button.tsx`:
- Around line 232-234: The locale options array in user-button.tsx hardcodes
labels ('EN','DE','FR'); replace these with translation keys using the
component's translation hook (e.g., t or useTranslation) so labels read
t('locale.en'), t('locale.de'), t('locale.fr') (or your project's equivalent
keys); ensure the translation hook is imported/initialized in the UserButton
component (or the function that defines the options) and use the translated
strings for the label field instead of literal text.
- Around line 198-209: The icon-only theme tabs (the objects with value
'system', 'light', 'dark' that currently set label to <Monitor>, <Sun>, <Moon>)
lack text alternatives; update each tab definition to provide an accessible name
using translation keys (e.g., t('theme.system'), t('theme.light'),
t('theme.dark')) as the aria-label for the tab trigger (or by adding a hidden
text node while keeping the icon aria-hidden). Modify the entries where label is
set (Monitor, Sun, Moon) to include an aria-label attribute or a visually-hidden
text child, using the app's i18n function so screen readers announce each
option.
- Line 194: Replace the unsafe cast in the onValueChange handler by validating
the incoming string before calling setTheme: remove "as 'system' | 'light' |
'dark'", add a type guard that checks whether v is one of the Theme union values
('system', 'light', 'dark') (e.g., an includes check against a const array or a
small helper isTheme function) and only call setTheme(v) when the guard passes;
otherwise handle the unexpected value (ignore or fallback to a default). Ensure
you update the onValueChange callback and any helper used (isTheme or themes
array) rather than casting.
In
`@services/platform/app/features/documents/components/onedrive-import/onedrive-picker-stage.tsx`:
- Line 112: Remove the unsafe cast in the Tabs onValueChange handler by
narrowing the incoming string before calling onTabChange: in the onValueChange
callback for the Tabs component, check whether the provided value equals the
known SourceTab literals ('onedrive' or 'sharepoint') and only then call
onTabChange with that narrowed value (otherwise ignore or handle unexpected
values), replacing the current "(v) => onTabChange(v as SourceTab)" with a type
guard that ensures v is a valid SourceTab.
In
`@services/platform/app/features/notifications/components/notification-bell.tsx`:
- Line 171: The onValueChange handler currently casts the string to
NotificationsFilter; instead, narrow the type with a guard: inside the Tabs
onValueChange callback check that v === 'unread' || v === 'all' and only then
call handleFilterChange(v) (otherwise ignore or handle unexpected values);
update the handler reference in notification-bell.tsx so it uses this type guard
rather than the `as NotificationsFilter` cast, referencing the
NotificationsFilter type and the handleFilterChange function.
In `@services/platform/app/globals.css`:
- Line 167: The dark theme border variable --border currently at "0, 0%, 18.04%"
is too close to the dark card color --card ("0 0% 9.02%") and fails the 3:1
non-text contrast requirement; update --border in globals.css (the --border
variable) to a higher lightness value (e.g., increase the middle percentage to
around ~30% or another value that yields >=3:1 against --card), verify the new
value meets a 3:1 contrast ratio against --card, and keep the change scoped to
the same CSS variable so all components using --border benefit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83949623-172f-4617-aaa6-dd46950a1074
📒 Files selected for processing (17)
services/platform/app/components/ui/dialog/dialog.tsxservices/platform/app/components/ui/navigation/locale-tabs.stories.tsxservices/platform/app/components/ui/navigation/locale-tabs.test.tsxservices/platform/app/components/ui/navigation/locale-tabs.tsxservices/platform/app/components/ui/navigation/tabs.stories.tsxservices/platform/app/components/ui/navigation/tabs.tsxservices/platform/app/components/ui/overlays/dropdown-menu.tsxservices/platform/app/components/user-button.tsxservices/platform/app/features/automations/components/automation-create-dialog.tsxservices/platform/app/features/chat/components/arena/arena-model-selector.tsxservices/platform/app/features/chat/components/dictation-button.tsxservices/platform/app/features/documents/components/document-comparison/comparison-file-selector.tsxservices/platform/app/features/documents/components/document-upload-dialog.tsxservices/platform/app/features/documents/components/onedrive-import/onedrive-picker-stage.tsxservices/platform/app/features/notifications/components/notification-bell.tsxservices/platform/app/globals.cssservices/platform/app/routes/dashboard/$id/agents/$agentId/conversation-starters.tsx
💤 Files with no reviewable changes (3)
- services/platform/app/components/ui/navigation/locale-tabs.test.tsx
- services/platform/app/components/ui/navigation/locale-tabs.stories.tsx
- services/platform/app/components/ui/navigation/locale-tabs.tsx
Comment on lines
+24
to
+39
| variant?: 'pill' | 'underline'; | ||
| /** Optional actions rendered to the right of the tab list */ | ||
| actions?: ReactNode; | ||
| } | ||
|
|
||
| const listStyles = { | ||
| pill: 'scrollbar-hide inline-flex items-center overflow-x-auto bg-muted p-1 text-muted-foreground rounded-lg', | ||
| underline: | ||
| 'scrollbar-hide inline-flex items-center gap-4 overflow-x-auto border-b border-border text-muted-foreground', | ||
| } as const; | ||
|
|
||
| const triggerStyles = { | ||
| pill: 'ring-offset-background focus-visible:ring-ring data-[state=active]:bg-tab data-[state=active]:text-foreground inline-flex items-center justify-center rounded-md px-3 py-1 text-sm font-medium whitespace-nowrap transition-all focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:outline-none disabled:pointer-events-none disabled:opacity-50 data-[state=active]:shadow-sm', | ||
| underline: | ||
| 'ring-offset-background focus-visible:ring-ring relative inline-flex items-center justify-center border-b-2 border-transparent px-1 pb-2 text-sm font-medium whitespace-nowrap transition-colors focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:outline-none disabled:pointer-events-none disabled:opacity-50 data-[state=active]:border-primary data-[state=active]:text-foreground', | ||
| } as const; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -path "*/services/platform/app/components/ui/navigation/tabs.tsx" -type fRepository: tale-project/tale
Length of output: 118
🏁 Script executed:
wc -l services/platform/app/components/ui/navigation/tabs.tsxRepository: tale-project/tale
Length of output: 119
🏁 Script executed:
cat -n services/platform/app/components/ui/navigation/tabs.tsxRepository: tale-project/tale
Length of output: 3933
🏁 Script executed:
cd services/platform && grep -r "from 'class-variance-authority'" --include="*.ts" --include="*.tsx" | head -5Repository: tale-project/tale
Length of output: 579
🏁 Script executed:
cd services/platform && grep -r "import.*cva" --include="*.ts" --include="*.tsx" | head -5Repository: tale-project/tale
Length of output: 579
🏁 Script executed:
cd services/platform && head -50 app/components/layout/panel-header.tsx | cat -nRepository: tale-project/tale
Length of output: 1367
Refactor named variant styling to cva in this shared primitive.
The variant system with named variants ('pill' | 'underline') should use cva per project standards. Currently implemented as object literals (listStyles and triggerStyles), these should be converted to CVA functions to maintain consistency with other components in the codebase.
💡 Suggested change
+import { cva } from 'class-variance-authority';
import * as TabsPrimitive from '@radix-ui/react-tabs';
import { type ReactNode } from 'react';
import { cn } from '@/lib/utils/cn';
@@
-const listStyles = {
- pill: 'scrollbar-hide inline-flex items-center overflow-x-auto bg-muted p-1 text-muted-foreground rounded-lg',
- underline:
- 'scrollbar-hide inline-flex items-center gap-4 overflow-x-auto border-b border-border text-muted-foreground',
-} as const;
+const listVariants = cva('scrollbar-hide inline-flex items-center overflow-x-auto', {
+ variants: {
+ variant: {
+ pill: 'bg-muted rounded-lg p-1 text-muted-foreground',
+ underline: 'gap-4 border-b border-border text-muted-foreground',
+ },
+ },
+});
-const triggerStyles = {
- pill: 'ring-offset-background focus-visible:ring-ring data-[state=active]:bg-tab data-[state=active]:text-foreground inline-flex items-center justify-center rounded-md px-3 py-1 text-sm font-medium whitespace-nowrap transition-all focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:outline-none disabled:pointer-events-none disabled:opacity-50 data-[state=active]:shadow-sm',
- underline:
- 'ring-offset-background focus-visible:ring-ring relative inline-flex items-center justify-center border-b-2 border-transparent px-1 pb-2 text-sm font-medium whitespace-nowrap transition-colors focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:outline-none disabled:pointer-events-none disabled:opacity-50 data-[state=active]:border-primary data-[state=active]:text-foreground',
-} as const;
+const triggerVariants = cva(
+ 'ring-offset-background focus-visible:ring-ring inline-flex items-center justify-center text-sm font-medium whitespace-nowrap focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:outline-none disabled:pointer-events-none disabled:opacity-50',
+ {
+ variants: {
+ variant: {
+ pill: 'rounded-md px-3 py-1 transition-all data-[state=active]:bg-tab data-[state=active]:text-foreground data-[state=active]:shadow-sm',
+ underline: 'relative border-b-2 border-transparent px-1 pb-2 transition-colors data-[state=active]:border-primary data-[state=active]:text-foreground',
+ },
+ },
+ },
+);
@@
- <TabsPrimitive.List className={cn(listStyles[variant], listClassName)}>
+ <TabsPrimitive.List className={cn(listVariants({ variant }), listClassName)}>
@@
- className={cn(triggerStyles[variant], triggerClassName)}
+ className={cn(triggerVariants({ variant }), triggerClassName)}Per project guidelines: "ALWAYS USE cva for named variants (e.g., variant: 'primary' | 'secondary')." This refactoring aligns with the established pattern used throughout the codebase in components like PanelHeader and other UI primitives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/navigation/tabs.tsx` around lines 24 -
39, The current named-variant styling uses plain object literals (listStyles and
triggerStyles) instead of the project's CVA pattern; convert both listStyles and
triggerStyles into cva-based variant factories: import cva from
class-variance-authority (or the project's cva helper) and create cva() calls
that place the shared base classes as the first argument and declare a variants
object with keys for the named variant values ('pill' and 'underline')
containing the corresponding class strings, and set a sensible
defaultVariants.variant if needed; ensure the exported names (listStyles and
triggerStyles) remain the same so the Tabs component's variant prop continues to
work unchanged.
| variant = 'pill', | ||
| actions, | ||
| }: TabsProps) { | ||
| const hasContent = items.some((item) => item.content); |
There was a problem hiding this comment.
Use explicit undefined checks for tab content rendering.
The truthy checks can skip valid ReactNode values (e.g., 0 or empty string), causing panels to disappear unexpectedly.
💡 Suggested change
- const hasContent = items.some((item) => item.content);
+ const hasContent = items.some((item) => item.content !== undefined);
@@
- item.content && (
+ item.content !== undefined && (
<TabsPrimitive.ContentAlso applies to: 79-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/navigation/tabs.tsx` at line 52, The code
uses truthy checks for tab content (e.g., const hasContent = items.some((item)
=> item.content)) which will treat valid ReactNode values like 0 or '' as
missing; change these to explicit undefined/null checks. Update hasContent to
use item.content !== undefined && item.content !== null (or item.content !=
null) and similarly replace any conditional rendering checks around item.content
(the rendering logic that shows panels for each item) to explicitly check
against undefined/null so zero or empty-string content still renders.
| </div> | ||
| <Tabs | ||
| value={theme} | ||
| onValueChange={(v) => setTheme(v as 'system' | 'light' | 'dark')} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n services/platform/app/components/user-button.tsx | sed -n '185,205p'Repository: tale-project/tale
Length of output: 768
🏁 Script executed:
cat -n services/platform/app/components/user-button.tsx | head -50Repository: tale-project/tale
Length of output: 2091
🏁 Script executed:
cat -n services/platform/app/components/user-button.tsx | sed -n '50,100p'Repository: tale-project/tale
Length of output: 2155
🏁 Script executed:
cat -n services/platform/app/components/theme/theme-provider.tsxRepository: tale-project/tale
Length of output: 3964
🏁 Script executed:
cat -n services/platform/app/components/ui/navigation/tabs.tsxRepository: tale-project/tale
Length of output: 3933
Remove the as 'system' | 'light' | 'dark' cast and use a type guard instead.
The onValueChange handler receives a string from the Tabs component, but setTheme expects a Theme type ('system' | 'light' | 'dark'). Instead of casting, narrow the type with a guard:
Suggested change
- onValueChange={(v) => setTheme(v as 'system' | 'light' | 'dark')}
+ onValueChange={(v) => {
+ if (v === 'system' || v === 'light' || v === 'dark') {
+ setTheme(v);
+ }
+ }}This follows the coding guideline: "DO NOT use type casting (as). Use type guards, generics, or proper type narrowing 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.
Suggested change
| onValueChange={(v) => setTheme(v as 'system' | 'light' | 'dark')} | |
| onValueChange={(v) => { | |
| if (v === 'system' || v === 'light' || v === 'dark') { | |
| setTheme(v); | |
| } | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/user-button.tsx` at line 194, Replace the
unsafe cast in the onValueChange handler by validating the incoming string
before calling setTheme: remove "as 'system' | 'light' | 'dark'", add a type
guard that checks whether v is one of the Theme union values ('system', 'light',
'dark') (e.g., an includes check against a const array or a small helper isTheme
function) and only call setTheme(v) when the guard passes; otherwise handle the
unexpected value (ignore or fallback to a default). Ensure you update the
onValueChange callback and any helper used (isTheme or themes array) rather than
casting.
Comment on lines
+198
to
+209
| { | ||
| value: 'system', | ||
| label: <Monitor className="size-4" />, | ||
| }, | ||
| { | ||
| value: 'light', | ||
| label: <Sun className="size-4" />, | ||
| }, | ||
| { | ||
| value: 'dark', | ||
| label: <Moon className="size-4" />, | ||
| }, |
There was a problem hiding this comment.
Add accessible names for icon-only theme tabs.
Lines 198-209 render icon-only tab labels; these triggers need text alternatives so screen readers can announce each option.
💡 Suggested change
{
value: 'system',
- label: <Monitor className="size-4" />,
+ label: (
+ <span className="inline-flex items-center justify-center">
+ <Monitor className="size-4" aria-hidden />
+ <span className="sr-only">
+ {t('userButton.theme.system')}
+ </span>
+ </span>
+ ),
},
{
value: 'light',
- label: <Sun className="size-4" />,
+ label: (
+ <span className="inline-flex items-center justify-center">
+ <Sun className="size-4" aria-hidden />
+ <span className="sr-only">
+ {t('userButton.theme.light')}
+ </span>
+ </span>
+ ),
},
{
value: 'dark',
- label: <Moon className="size-4" />,
+ label: (
+ <span className="inline-flex items-center justify-center">
+ <Moon className="size-4" aria-hidden />
+ <span className="sr-only">
+ {t('userButton.theme.dark')}
+ </span>
+ </span>
+ ),
},As per coding guidelines: "ALWAYS provide text alternatives for non-text content" and "ALWAYS use translation keys for aria-label values."
📝 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
| { | |
| value: 'system', | |
| label: <Monitor className="size-4" />, | |
| }, | |
| { | |
| value: 'light', | |
| label: <Sun className="size-4" />, | |
| }, | |
| { | |
| value: 'dark', | |
| label: <Moon className="size-4" />, | |
| }, | |
| { | |
| value: 'system', | |
| label: ( | |
| <span className="inline-flex items-center justify-center"> | |
| <Monitor className="size-4" aria-hidden /> | |
| <span className="sr-only"> | |
| {t('userButton.theme.system')} | |
| </span> | |
| </span> | |
| ), | |
| }, | |
| { | |
| value: 'light', | |
| label: ( | |
| <span className="inline-flex items-center justify-center"> | |
| <Sun className="size-4" aria-hidden /> | |
| <span className="sr-only"> | |
| {t('userButton.theme.light')} | |
| </span> | |
| </span> | |
| ), | |
| }, | |
| { | |
| value: 'dark', | |
| label: ( | |
| <span className="inline-flex items-center justify-center"> | |
| <Moon className="size-4" aria-hidden /> | |
| <span className="sr-only"> | |
| {t('userButton.theme.dark')} | |
| </span> | |
| </span> | |
| ), | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/user-button.tsx` around lines 198 - 209, The
icon-only theme tabs (the objects with value 'system', 'light', 'dark' that
currently set label to <Monitor>, <Sun>, <Moon>) lack text alternatives; update
each tab definition to provide an accessible name using translation keys (e.g.,
t('theme.system'), t('theme.light'), t('theme.dark')) as the aria-label for the
tab trigger (or by adding a hidden text node while keeping the icon
aria-hidden). Modify the entries where label is set (Monitor, Sun, Moon) to
include an aria-label attribute or a visually-hidden text child, using the app's
i18n function so screen readers announce each option.
- Guard Tabs onValueChange handlers with string unions instead of unsafe type assertions (user-button, notification-bell, onedrive-picker-stage) - Rename inner `items` variable in conversation-starters to avoid shadow - Reorder tailwind classes in tabs.tsx header row
- Migrate Tabs variants to cva (project standard) - Use !== undefined checks for tab content to respect falsy ReactNodes - Add ariaLabel to TabItem and wire up for icon-only theme tabs in user-button - Bump dark --border to 24% lightness for clearer component boundaries
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tabsprimitive with avariantprop (pilldefault,underlinenew) and atriggerClassNameescape hatch. Tabs without content panels now render as a segmented control.LocaleTabscomponent in favor of the underline variant. Conversation starters, onedrive picker, and notification bell migrate ontoTabs.bg-cardinstead ofbg-muted; a new--tabCSS var backs the active tab fill; dark-mode--muted/--borderadjusted.bg-card/30).Test plan
UnderlineandHeadlessNoContentstories onTabs.Summary by CodeRabbit
New Features
Improvements