[WEB-4686] feat: propel tabs#7620
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughRefactors Tabs to a primitive-based composite API, removes the TabList component, adds a new Storybook story, and re-exports tabs from an index file. Public API changes include new composite Tabs with List/Trigger/Content/Indicator, removal of prior TabList and old Tabs props/types, and introduction of size on Trigger. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant T as Tabs (Root)
participant L as Tabs.List
participant Tr as Tabs.Trigger
participant C as Tabs.Content
participant I as Tabs.Indicator
participant P as TabsPrimitive
U->>T: Mount Tabs (defaultValue)
T->>P: Initialize Root
T->>L: Render list
L->>P: Initialize List
loop For each tab
L->>Tr: Render Trigger (value, size)
Tr->>P: Initialize Tab
end
T->>C: Render Content panels
C->>P: Initialize Panel(s)
T->>I: Render Indicator (decorative)
U->>Tr: Click Trigger (value)
Tr->>P: Set selected value
P-->>C: Show matching Panel
P-->>I: Update indicator position/state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
65-91: Fix nested interactive elements (anchor inside anchor) for accessibility and click handlingAn for the asset wraps another for download. Nested anchors are invalid HTML, harm accessibility, and cause event ambiguity.
Refactor to avoid nesting. For example, keep the outer container as a div and render two separate anchors inside:
- <a - href={asset.href} - className="relative group/asset-item h-12 flex items-center gap-2 pr-2 rounded border border-custom-border-200 hover:bg-custom-background-80 transition-colors" - > + <div className="relative group/asset-item h-12 flex items-center gap-2 pr-2 rounded border border-custom-border-200 hover:bg-custom-background-80 transition-colors"> <div className="flex-shrink-0 w-11 h-12 rounded-l bg-cover bg-no-repeat bg-center" style={{ backgroundImage: `url('${assetSrc}')`, }} /> <div className="flex-1 space-y-0.5 truncate"> - <p className="text-sm font-medium truncate">{asset.name}</p> + <a href={asset.href ?? "#"} className="text-sm font-medium truncate"> + {asset.name} + </a> <div className="flex items-end justify-between gap-2"> <p className="shrink-0 text-xs text-custom-text-200" /> - <a - href={assetDownloadSrc} - target="_blank" - rel="noreferrer noopener" - className="shrink-0 py-0.5 px-1 flex items-center gap-1 rounded text-custom-text-200 hover:text-custom-text-100 opacity-0 pointer-events-none group-hover/asset-item:opacity-100 group-hover/asset-item:pointer-events-auto transition-opacity" - > + <a + href={assetDownloadSrc} + target="_blank" + rel="noreferrer noopener" + className="shrink-0 py-0.5 px-1 flex items-center gap-1 rounded text-custom-text-200 hover:text-custom-text-100 opacity-0 pointer-events-none group-hover/asset-item:opacity-100 group-hover/asset-item:pointer-events-auto transition-opacity" + > <Download className="shrink-0 size-3" /> <span className="text-xs font-medium">{t("page_navigation_pane.tabs.assets.download_button")}</span> </a> </div> </div> - </a> + </div>If you need the whole row to be clickable, keep the container a div and add an onClick that navigates to asset.href while ensuring the download button calls e.stopPropagation().
apps/web/core/components/core/image-picker-popover.tsx (1)
125-136: Early return can leave UI stuck in “Uploading...”If workspaceSlug is falsy, handleSubmit returns after setIsImageUploading(true), leaving the button disabled and spinner active.
Patch the guard:
- } else { - if (!workspaceSlug) return; + } else { + if (!workspaceSlug) { + setIsImageUploading(false); + setToast({ title: "Image not uploaded", type: TOAST_TYPE.ERROR, message: "Workspace not found" }); + return; + }
🧹 Nitpick comments (25)
apps/web/core/components/analytics/work-items/modal/content.tsx (3)
70-75: Verify intent: tabs removed → all sections now mount and render simultaneouslySwitching from a tabbed panel to a stacked layout means TotalInsights, CreatedVsResolved, CustomizedInsights, and WorkItemsInsightTable all render at once. These analytics are typically heavy and may each trigger data fetches/reactions, leading to slower modal open, higher memory, and redundant queries. If the plan was to migrate to Propel Tabs, this file currently doesn’t use them; if the stacked layout is intentional, consider lazy-mounting/deferred rendering per section to mitigate cost.
Possible mitigations:
- Use the new Propel Tabs primitives here (if UX still expects tabs) so only the active pane mounts.
- If keeping the stacked layout, mount sections on first visibility (IntersectionObserver) or progressively hydrate below-the-fold content after the modal settles.
70-70: Define a fixed scroll region to avoid scroll chaining in modalsThe inner container scrolls, but without a fixed height and overscroll control, background scroll/scroll chaining can occur on some platforms. Recommend making the container explicitly fill the modal and contain overscroll.
- <div className="flex flex-col gap-14 overflow-y-auto p-6"> + <div className="flex h-full flex-col gap-14 overflow-y-auto overscroll-contain p-6">
71-75: DRY the peekView proppeekView={!fullScreen} is repeated. Hoist once for clarity and to prevent drift if the condition changes.
- <TotalInsights analyticsType="work-items" peekView={!fullScreen} /> + <TotalInsights analyticsType="work-items" peekView={peekView} /> <CreatedVsResolved /> - <CustomizedInsights peekView={!fullScreen} /> + <CustomizedInsights peekView={peekView} />Add outside the selected range (just before the return):
const peekView = !fullScreen;packages/ui/src/tabs/tab-list.tsx (1)
24-63: Controlled vs internal selection can diverge; ensure consumers wire both visual and logical selectionWhen selectedTab is provided, you override the Headless UI provided selected styling with (selectedTab === tab.key), but the logical selection inside Tab.Group remains driven by Headless UI’s internal index. If a consumer passes selectedTab without also controlling Tab.Group’s selectedIndex (or mapping keys->indices consistently), the highlighted tab could disagree with the rendered panel.
- Confirm all call sites that pass selectedTab also control the enclosing Tab.Group (selectedIndex/onChange) using the same key-to-index mapping.
- Consider documenting this contract in the component’s JSDoc.
Improve focus visibility for keyboard users by adding a focus-visible ring on Tab:
- "flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus:outline-none cursor-pointer transition-all rounded", + "flex items-center justify-center p-1 min-w-fit w-full font-medium outline-none focus:outline-none focus-visible:ring-2 focus-visible:ring-custom-primary-100/40 cursor-pointer transition-all rounded",packages/ui/src/tabs/composable-tabs.tsx (3)
1-3: Avoid duplicating Tabs primitives across packages to prevent driftThis UI-level Tabs wraps @base-ui-components directly, while propel also introduces Tabs primitives. Keeping two wrappers increases maintenance and styling drift risk.
- Prefer a single source of truth. Either:
- Move this wrapper to @plane/propel and re-export for UI consumers, or
- Have propel re-export the UI wrapper.
- If the workspace graph prevents that today, add a TODO with a plan to consolidate post-merge.
22-35: Verify the active-state attribute; add robust fallbacksBase UI’s Tabs.Tab may expose active state via aria-selected="true" and/or data-selected="true". You currently style via data-[state=active], which may not fire, depending on the library’s attribute semantics.
- Check in the browser DevTools which attribute is toggled on the active tab.
Add fallbacks for common patterns and avoid relying on one attribute only:
<TabsPrimitive.Tab data-slot="tabs-trigger" className={cn( - "flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus:outline-none cursor-pointer transition-all rounded", - "data-[state=active]:bg-custom-background-100 data-[state=active]:text-custom-text-100 data-[state=active]:shadow-sm", - "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", + "flex items-center justify-center p-1 min-w-fit w-full font-medium outline-none focus:outline-none cursor-pointer transition-all rounded", + // Active styles (support multiple attribute conventions) + "data-[state=active]:bg-custom-background-100 data-[state=active]:text-custom-text-100 data-[state=active]:shadow-sm", + "data-[selected=true]:bg-custom-background-100 data-[selected=true]:text-custom-text-100 data-[selected=true]:shadow-sm", + "aria-selected:bg-custom-background-100 aria-selected:text-custom-text-100 aria-selected:shadow-sm", + // Inactive/hover + "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", "disabled:text-custom-text-400 disabled:cursor-not-allowed", className )} />Also removed the base text-custom-text-100 to prevent conflicting color declarations.
42-52: Indicator relies on CSS variables that aren’t set; either wire them or defer the componentTabsIndicator uses --active-tab-width and --active-tab-left but nothing in this file populates them. Without values, the indicator may not render correctly.
- Confirm whether @base-ui-components emits these CSS vars on List or Tab via their indicator API. If not, wire them yourself.
Option A (quick fallback): Add safe fallbacks so the indicator doesn’t collapse:
- className={cn( - "absolute left-0 top-[50%] z-[-1] h-6 w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out", + className={cn( + "absolute left-0 top-[50%] z-[-1] h-6 w-[var(--active-tab-width,0)] translate-x-[var(--active-tab-left,0)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out", className )}Option B (preferred): Measure the active tab and set the vars on the List. For example, expose a TabsListWithIndicator that manages a ref, observes the active tab (via aria-selected), and sets style.setProperty("--active-tab-width", …) and "--active-tab-left" on change/resize.
apps/web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
103-109: Type-only React import to support React.FC annotationYou use React.FC in the file but don’t import the React namespace. Depending on tsconfig, this can error (“Cannot find namespace ‘React’”). Safer to import the type explicitly or drop the FC annotation.
Either remove the FC annotation, or import the type:
-import { useMemo } from "react"; +import { useMemo, type FC } from "react"; ... -export const PageNavigationPaneAssetsTabPanel: React.FC<Props> = observer((props) => { +export const PageNavigationPaneAssetsTabPanel: FC<Props> = observer((props) => {apps/web/core/components/pages/navigation-pane/root.tsx (2)
49-57: Guard against invalid tab values before updating the URL.If an unexpected value is passed to onValueChange (e.g., from manual URL edits or future refactors), we should ignore it to prevent polluting history with bad query params.
Apply this diff:
- const handleTabChange = useCallback( - (value: TPageNavigationPaneTab) => { - const updatedTab = value; + const handleTabChange = useCallback( + (value: TPageNavigationPaneTab) => { + if (!PAGE_NAVIGATION_PANE_TAB_KEYS.includes(value)) return; + const updatedTab = value; const isUpdatedTabInfo = updatedTab === "info"; const updatedRoute = updateQueryParams({ paramsToAdd: { [PAGE_NAVIGATION_PANE_TABS_QUERY_PARAM]: updatedTab }, paramsToRemove: !isUpdatedTabInfo ? [PAGE_NAVIGATION_PANE_VERSION_QUERY_PARAM] : undefined, }); router.push(updatedRoute); },
81-85: Inline sanitize the controlled value to a known tab.If the URL contains an unknown tab key, falling back prevents Tabs from entering an uncontrolled/empty state.
Apply this diff:
- <Tabs - value={activeTab} + <Tabs + value={PAGE_NAVIGATION_PANE_TAB_KEYS.includes(activeTab) ? activeTab : "outline"} onValueChange={handleTabChange} className="size-full p-3.5 pt-0 overflow-y-auto vertical-scrollbar scrollbar-sm outline-none" >apps/web/core/components/license/modal/card/base-paid-plan-card.tsx (1)
45-64: Control state should remain valid if prices don’t include "month".selectedPlan defaults to "month". If a plan exposes only yearly pricing, Tabs will be controlled with a value that has no matching TabsTrigger, leaving nothing selected. Prefer initializing from available prices and re-syncing when prices change.
Add an effect to keep selectedPlan within available options, and initialize from prices:
// Replace the initialization (outside this hunk) with: const [selectedPlan, setSelectedPlan] = useState<TBillingFrequency>(() => { const preferred: TBillingFrequency = "month"; const first = prices[0]?.recurring as TBillingFrequency | undefined; return (prices.some(p => p.recurring === preferred) ? preferred : first) ?? "month"; }); // Inside the component, co-locate this effect near the state: useEffect(() => { if (!prices.some(p => p.recurring === selectedPlan)) { const next = (prices[0]?.recurring as TBillingFrequency | undefined) ?? "month"; setSelectedPlan(next); } }, [prices, selectedPlan]);No API/UX change when both month and year exist; it only guards edge cases.
apps/web/core/components/cycles/active-cycle/cycle-stats.tsx (3)
39-45: Persist tab per workspace/project to avoid cross-project bleed.Using a global "activeCycleTab" key shares the selection across all workspaces/projects. If that’s unintended, namespace the storage key.
Apply this diff:
- const { storedValue: tab, setValue: setTab } = useLocalStorage<TActiveCycleStatsTab>("activeCycleTab", "Assignees"); + const storageKey = `activeCycleTab:${workspaceSlug}:${projectId}`; + const { storedValue: tab, setValue: setTab } = useLocalStorage<TActiveCycleStatsTab>(storageKey, "Assignees");
80-83: Handler type is fine; consider narrowing the signature.Casting value to TActiveCycleStatsTab is okay. If @plane/propel/tabs exposes generics, prefer a typed onValueChange to remove the cast; otherwise, this is acceptable.
250-287: Labels panel: verify color contrast for accessibility.Inline background colors may reduce contrast with text in some themes. If not already handled globally, consider computing a contrasting text color or adding a subtle border.
I can add a small utility that computes contrast and applies a text/border class based on WCAG thresholds.
apps/web/core/components/cycles/analytics-sidebar/progress-stats.tsx (1)
308-319: Minor UX consistency: align tab order across Modules vs CyclesHere the trigger order is States → Assignees → Labels, while Modules uses Assignees → Labels → States. Consider standardizing the order to reduce context switching for users navigating between views.
apps/web/core/components/modules/analytics-sidebar/progress-stats.tsx (1)
306-317: Minor UX consistency: align trigger order with CyclesConsider matching the tab order (States → Assignees → Labels) or vice versa, to keep navigation consistent across analytics surfaces.
apps/web/app/(all)/[workspaceSlug]/(projects)/analytics/[tabId]/page.tsx (1)
63-67: Guard against invalid tabId to avoid empty contentCasting tabId to TAnalyticsTabsBase without validation can yield a Tabs value that does not correspond to any trigger/content, rendering nothing until navigation. Validate against ANALYTICS_TABS.
Apply this diff to harden defaultTab:
- const defaultTab = (tabId as TAnalyticsTabsBase) || ANALYTICS_TABS[0].key; + const allowed = new Set(ANALYTICS_TABS.map((t) => t.key)); + const defaultTab = (allowed.has(tabId) ? (tabId as TAnalyticsTabsBase) : ANALYTICS_TABS[0].key);packages/ui/src/tabs/tabs.tsx (1)
29-87: Architecture note: two parallel Tab systems can driftThis component still wraps Headless UI, while newer code migrates to @plane/propel/tabs. Consider consolidating on a single Tabs system or adding a clear deprecation note for this one to avoid fragmentation.
apps/web/core/components/core/image-picker-popover.tsx (2)
199-264: Unsplash panel: minor resilience and UX tweaks
- alt can be null from Unsplash; fallback avoids empty alt attributes.
- Consider showing a lightweight error note if unsplashError exists (you already hide the trigger).
Apply this small tweak for alt:
- <img - src={image.urls.small} - alt={image.alt_description} + <img + src={image.urls.small} + alt={image.alt_description ?? "Unsplash image"} className="absolute left-0 top-0 h-full w-full cursor-pointer rounded object-cover" />
306-397: Dropzone and upload CTA UX are solid; consider i18nGreat drag state and rejection UI. For consistency with the rest of the app, consider localizing strings like “Drag & drop image here”, “Drop image here to upload”, “File rejected”, and button labels.
packages/propel/src/tabs/tabs.tsx (5)
9-20: TabsList layout: justify-between + overflow-auto makes triggers stretch apart and can hide the indicator behind the list
- justify-between will push first/last triggers to edges with large gaps in between; for scrollable tab rows this is usually undesirable.
- Using overflow-auto on both axes can introduce a vertical scrollbar; horizontal scrolling is what we want.
- Without an explicit z-index on the list, a negative z-index indicator can render behind the parent (see indicator comment below).
Suggest tightening layout and stacking:
- "flex w-full min-w-fit items-center justify-between gap-1.5 rounded-md text-sm p-0.5 bg-custom-background-80/60 relative overflow-auto", + "flex w-full min-w-fit items-center justify-start gap-1.5 rounded-md text-sm p-0.5 bg-custom-background-80/60 relative z-0 overflow-x-auto",
51-60: Indicator can render behind the list and capture events; add slot + make stacking safe
- z-[-1] risks placing the indicator behind the parent background (or leaking behind unrelated elements) unless the parent establishes a stacking context with z-0.
- It’s also safer if the indicator does not intercept pointer events in a scrollable header.
Proposed tweaks:
- <div + <div + data-slot="tabs-indicator" className={cn( - "absolute left-0 top-[50%] z-[-1] h-6 w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out", + "absolute left-0 top-[50%] z-0 pointer-events-none h-6 w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out", className )} {...props} />Optional: add fallbacks for the CSS vars (e.g., via inline style width: "var(--active-tab-width, 0px)") if the vars might be unset on first render.
If you keep z-[-1], please ensure the TabsList has z-0 (as suggested above) to avoid the indicator disappearing behind the parent.
1-2: Library naming check: Base Web vs Base UIPR description mentions “Base Web (baseui)”, but the import is from “@base-ui-components/react/tabs”. If that’s intentional (new Base UI primitives), we’re good; otherwise, confirm the intended library to avoid future confusion in docs and release notes.
22-45: Optional: focus-visible styles for keyboard usersAdding a visible focus state on TabsTrigger improves a11y without impacting hover/mouse users.
Example minimal addition (pairs well with your tokens):
- "flex items-center justify-center p-1 min-w-fit font-medium outline-none focus:outline-none cursor-pointer transition-all duration-200 ease-in-out rounded", + "flex items-center justify-center p-1 min-w-fit font-medium outline-none focus:outline-none focus-visible:ring-2 focus-visible:ring-custom-primary-100/50 cursor-pointer transition-all duration-200 ease-in-out rounded",
47-49: Consistency: add displayName for better DevTools labelsSmall DX win when debugging compound components:
Tabs.displayName = "Tabs"; TabsList.displayName = "Tabs.List"; TabsTrigger.displayName = "Tabs.Trigger"; TabsContent.displayName = "Tabs.Content"; TabsIndicator.displayName = "Tabs.Indicator";Also applies to: 51-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
apps/web/app/(all)/[workspaceSlug]/(projects)/analytics/[tabId]/page.tsx(2 hunks)apps/web/core/components/analytics/work-items/modal/content.tsx(1 hunks)apps/web/core/components/core/image-picker-popover.tsx(2 hunks)apps/web/core/components/cycles/active-cycle/cycle-stats.tsx(7 hunks)apps/web/core/components/cycles/analytics-sidebar/issue-progress.tsx(0 hunks)apps/web/core/components/cycles/analytics-sidebar/progress-stats.tsx(4 hunks)apps/web/core/components/license/modal/card/base-paid-plan-card.tsx(2 hunks)apps/web/core/components/modules/analytics-sidebar/issue-progress.tsx(0 hunks)apps/web/core/components/modules/analytics-sidebar/progress-stats.tsx(5 hunks)apps/web/core/components/pages/navigation-pane/root.tsx(3 hunks)apps/web/core/components/pages/navigation-pane/tab-panels/assets.tsx(1 hunks)apps/web/core/components/pages/navigation-pane/tab-panels/root.tsx(2 hunks)apps/web/core/components/pages/navigation-pane/tabs-list.tsx(2 hunks)packages/propel/package.json(1 hunks)packages/propel/src/tabs/index.ts(1 hunks)packages/propel/src/tabs/list.tsx(0 hunks)packages/propel/src/tabs/tabs.tsx(1 hunks)packages/ui/src/tabs/composable-tabs.tsx(1 hunks)packages/ui/src/tabs/tab-list.tsx(1 hunks)packages/ui/src/tabs/tabs.tsx(2 hunks)
💤 Files with no reviewable changes (3)
- apps/web/core/components/modules/analytics-sidebar/issue-progress.tsx
- apps/web/core/components/cycles/analytics-sidebar/issue-progress.tsx
- packages/propel/src/tabs/list.tsx
🧰 Additional context used
🧬 Code graph analysis (11)
apps/web/core/components/pages/navigation-pane/tab-panels/root.tsx (1)
apps/web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
PageNavigationPaneAssetsTabPanel(103-119)
apps/web/core/components/pages/navigation-pane/root.tsx (4)
packages/propel/src/tabs/tabs.tsx (1)
Tabs(68-68)packages/ui/src/tabs/composable-tabs.tsx (1)
Tabs(54-54)apps/web/core/components/pages/navigation-pane/tabs-list.tsx (1)
PageNavigationPaneTabsList(7-20)apps/web/core/components/pages/navigation-pane/tab-panels/root.tsx (1)
PageNavigationPaneTabPanelsRoot(20-35)
apps/web/core/components/license/modal/card/base-paid-plan-card.tsx (1)
packages/ui/src/billing/subscription.ts (1)
getSubscriptionBackgroundColor(35-73)
apps/web/app/(all)/[workspaceSlug]/(projects)/analytics/[tabId]/page.tsx (2)
packages/types/src/analytics.ts (1)
TAnalyticsTabsBase(33-33)packages/ui/src/tabs/tabs.tsx (1)
Tabs(29-88)
packages/ui/src/tabs/composable-tabs.tsx (1)
packages/propel/src/tabs/tabs.tsx (5)
Tabs(68-68)TabsList(68-68)TabsTrigger(68-68)TabsContent(68-68)TabsIndicator(68-68)
apps/web/core/components/pages/navigation-pane/tabs-list.tsx (1)
packages/i18n/src/store/index.ts (1)
t(210-231)
apps/web/core/components/modules/analytics-sidebar/progress-stats.tsx (2)
packages/hooks/src/use-local-storage.tsx (1)
useLocalStorage(24-55)apps/web/core/components/cycles/analytics-sidebar/progress-stats.tsx (3)
AssigneeStatComponent(73-125)LabelStatComponent(127-188)StateStatComponent(190-221)
apps/web/core/components/core/image-picker-popover.tsx (1)
packages/constants/src/file.ts (2)
ACCEPTED_COVER_IMAGE_MIME_TYPES_FOR_REACT_DROPZONE(9-14)MAX_FILE_SIZE(1-1)
apps/web/core/components/cycles/active-cycle/cycle-stats.tsx (1)
packages/hooks/src/use-local-storage.tsx (1)
useLocalStorage(24-55)
packages/ui/src/tabs/tabs.tsx (1)
packages/ui/src/tabs/tab-list.tsx (1)
TabListItem(7-13)
apps/web/core/components/cycles/analytics-sidebar/progress-stats.tsx (2)
packages/hooks/src/use-local-storage.tsx (1)
useLocalStorage(24-55)packages/ui/src/tabs/tabs.tsx (1)
Tabs(29-88)
🔇 Additional comments (31)
apps/web/core/components/analytics/work-items/modal/content.tsx (1)
74-74: Consistency check: should the table honor peekView as well?Other sections adapt layout via peekView; WorkItemsInsightTable does not receive it. If the table supports compact/peek behavior (pagination density, columns, etc.), consider passing the flag for a consistent peek experience.
- <WorkItemsInsightTable /> + <WorkItemsInsightTable peekView={peekView} />packages/propel/package.json (1)
45-46: Please verify type safety locallyThe sandbox environment doesn’t have the workspace-local TypeScript compiler installed, so I couldn’t confirm the change via
tsc. To ensure bumping@types/react-domto^18.3.0introduces no regressions, please run the following in your local checkout:# Install/update dependencies pnpm install # Run a full TS build for the propel package pnpm -w exec tsc -b packages/propel --noEmitpackages/ui/src/tabs/tab-list.tsx (1)
7-22: Generic tab keys are a welcome improvementMaking TabListItem and TTabListProps generic over string-like keys improves type safety for value-based routing and localStorage keys.
packages/propel/src/tabs/index.ts (1)
1-1: Re-export looks goodPublicly surfacing the tabs primitives via index.ts keeps import paths stable. No concerns.
apps/web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
111-116: Empty-state logic: clarify behavior when assetsList is undefinedWith assetsList?.length === 0, the empty state shows only when the list is defined and empty. If assetsList is undefined (e.g., loading), the else branch renders nothing due to assetsList?.map. Confirm intended behavior.
- If the desired UX is to show the empty state when undefined or empty, adjust as below.
- {assetsList?.length === 0 ? ( + {!assetsList || assetsList.length === 0 ? ( <PageNavigationPaneAssetsTabEmptyState /> ) : ( assetsList?.map((asset) => <AssetItem key={asset.id} asset={asset} page={page} />) )}apps/web/core/components/pages/navigation-pane/tabs-list.tsx (2)
12-18: Clean migration to composable Tabs triggers.Straightforward switch to TabsList/TabsTrigger; using ORDERED_PAGE_NAVIGATION_TABS_LIST keeps triggers and panels aligned by source of truth. Explicit type="button" is a safe default in case this is ever nested in a form.
12-18: TabsTrigger and TabsContent values are in sync
Bothapps/web/core/components/pages/navigation-pane/tabs-list.tsxandtab-panels/root.tsximport and map over the sameORDERED_PAGE_NAVIGATION_TABS_LISTfor theirvalueprops, and no hardcodedvalue=attributes were found. Triggers and panels remain aligned as expected.apps/web/core/components/pages/navigation-pane/root.tsx (1)
81-89: LGTM on the migration from Headless UI Tab.Group to composable Tabs.Value-driven selection, URL sync, and the new handler signature are all correctly wired. The Tabs provider wraps both list and panels, so context is intact.
apps/web/core/components/pages/navigation-pane/tab-panels/root.tsx (2)
24-33: Check TabsContent mount/unmount behavior to avoid unnecessary work.Depending on the implementation, TabsContent may keep all panels mounted. If panels are heavy (outline/assets) and don’t need to stay mounted, prefer unmounting hidden panels to save memory and effects work; if preserving internal state is important, keep them mounted. Ensure the chosen behavior matches prior UX.
If TabsContent supports a prop like forceMount or unmount, set it explicitly. Otherwise, confirm default behavior in @plane/propel/tabs and align with expectations.
26-31: Consistent source-of-truth for values.Using tab.key from ORDERED_PAGE_NAVIGATION_TABS_LIST here mirrors tabs-list.tsx, keeping values consistent across triggers and panels. Good.
apps/web/core/components/license/modal/card/base-paid-plan-card.tsx (4)
47-54: LGTM on Tabs composition and styling.Value-driven Tabs with TabsList/Trigger and TabsContent look correct; the background color utility ties variant to styling cleanly.
49-52: Avoid interactive children inside a TabsTrigger.Ensure renderPriceContent(price) does not return buttons/links; nested interactive elements inside a button can cause accessibility and event conflicts.
If needed, constrain renderPriceContent to text + non-interactive markup for the trigger, and render buttons only inside TabsContent.
56-63: Panels correctly keyed by price.recurring.Using recurring as the value keeps the mapping simple and stable. Action button rendering inside the active panel is appropriate.
66-84: Features block extraction looks good; verify vertical layout behavior.The grid spans toggle on verticalFeatureList; confirm this still matches design expectations compared to the previous Tab.Panel-based layout spacing.
apps/web/core/components/cycles/active-cycle/cycle-stats.tsx (4)
86-97: Confirm TabsTrigger supports size="sm".If TabsTrigger doesn’t accept a size prop, these attributes will be ignored or cause type errors. Falling back to className-based sizing is safer if size isn’t in the API.
99-190: Priority-Issues panel: behavior preserved; looks good.Infinite scroll, loaders, and filter application on click are intact. Nice reuse of shared components.
191-249: Assignees panel: UX intact.Mapping and filter wiring are unchanged; empty state still handled. No issues spotted.
84-90: Falsish cycleId returns null instead of a loader — confirm UX.Previously this likely showed a skeleton/loader. Returning null can cause layout shifts. If intentional, all good; otherwise, reintroduce a minimal placeholder to keep card heights stable.
apps/web/core/components/cycles/analytics-sidebar/progress-stats.tsx (4)
237-238: Typed tab key alias improves safetyDefining TCycleProgressStatsTabKey narrows the allowed values and prevents accidental persistence of invalid tab names. Good move.
252-255: LocalStorage wiring is correct and type-safeuseLocalStorage is correctly parameterized with TCycleProgressStatsTabKey and namespaced by cycleId, so tabs persist per cycle. No changes needed.
302-305: Stateless handler keeps Tabs controlled by LocalStoragehandleTabChange delegates to setCycleTab, which updates storedValue synchronously inside the hook—this keeps Tabs fully controlled via currentTab. Looks good.
321-347: TabsContent mapping is clean and segregatedPanels correctly scope each stat component with the right props. Nice, tidy composition.
apps/web/core/components/modules/analytics-sidebar/progress-stats.tsx (3)
224-224: Exported tab type is clear and reusableExporting TModuleProgressStatsTab helps share the union across consumers. All good.
252-255: Per-module tab persistence is correctKeyed by moduleId with a typed default, this will persist independently per module. Implementation looks solid.
300-303: Handler keeps Tabs controlled via LocalStorageSame pattern as cycles—minimal and effective.
apps/web/app/(all)/[workspaceSlug]/(projects)/analytics/[tabId]/page.tsx (1)
75-94: Overall migration to composable Tabs is cleanValue-driven Tabs with router-based onValueChange is straightforward and keeps URL as the source of truth. Header layout with actions is tidy.
packages/ui/src/tabs/tabs.tsx (1)
13-27: Good generic typing for Tab items and propsIntroducing TabItem and TTabsProps tightens types end-to-end, especially defaultTab’s coupling to tab keys. Nice improvement.
apps/web/core/components/core/image-picker-popover.tsx (2)
185-197: Default tab and triggers logic look gooddefaultValue="unsplash" and conditional rendering of triggers based on data/error is sensible. Nice guardrails to hide empty tabs.
266-304: Images panel wiring is correctProject cover thumbnails use getFileURL and onChange emits the raw path—consistent with the Upload/Unsplash behavior. LGTM.
packages/propel/src/tabs/tabs.tsx (2)
5-7: Nice lightweight wrapper around the primitivesClean pass-through of props and className. Good use of the cn utility and data-slot for targeting.
47-49: Panel wrapper looks good; keep it as a thin pass-throughThe relative and outline-none defaults are sensible here. No action needed.
apps/web/core/components/modules/analytics-sidebar/progress-stats.tsx
Outdated
Show resolved
Hide resolved
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
JayashTripathy
force-pushed
the
feat-propel-tabs
branch
from
8bf02b2 to
8aac784
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/propel/src/tabs/tabs.tsx (2)
5-20: Typed compound statics + forwardRef — LGTM.Nice job declaring TabsCompound and exposing statics via Object.assign with proper ref types. This addresses the prior feedback about typed statics and ref forwarding.
Also applies to: 89-94
44-65: Each trigger is forced to 100% width and has conflicting text colors; also missing visible keyboard focus.
- w-full makes every tab as wide as the container, causing overflow when multiple triggers render.
- Base text color sets both text-custom-text-100 and text-custom-text-400; the latter wins, causing confusion.
- focus:outline-none removes visible focus without a replacement, violating focus-visible requirements.
Fix by removing w-full, consolidating base color, adding aria-selected variants, and restoring a visible focus ring. Also layer triggers above the indicator.
- "flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus:outline-none cursor-pointer transition-all duration-200 ease-in-out rounded", - "data-[selected]:bg-custom-background-100 data-[selected]:text-custom-text-100 data-[selected]:shadow-sm", - "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", - "disabled:text-custom-text-400 disabled:cursor-not-allowed", + "flex items-center justify-center p-1 min-w-fit shrink-0 font-medium outline-none focus:outline-none cursor-pointer transition-all duration-200 ease-in-out rounded relative z-[1]", + "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", + "aria-selected:bg-custom-background-100 aria-selected:text-custom-text-100 data-[selected]:bg-custom-background-100 data-[selected]:text-custom-text-100 data-[selected]:shadow-sm", + "focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-custom-primary-100/40", + "disabled:text-custom-text-400 disabled:cursor-not-allowed",Also applies to: 51-55, 52-54, 56-59
🧹 Nitpick comments (6)
packages/propel/src/tabs/tabs.tsx (6)
22-27: Avoid hardcoding h-full on the root; it can collapse parents or force unwanted layouts.Root components rarely need full-height. Let consumers opt into height constraints.
Apply this diff:
- <TabsPrimitive.Root data-slot="tabs" className={cn("flex flex-col w-full h-full", className)} {...props} ref={ref} /> + <TabsPrimitive.Root data-slot="tabs" className={cn("flex flex-col w-full", className)} {...props} ref={ref} />
33-41: TabsList layout: justify-between fights with scrollable tabs; prefer justify-start + nowrap.“justify-between” stretches triggers across the width and interacts poorly with horizontal scrolling. Use start alignment and prevent wrapping.
- "flex w-full min-w-fit items-center justify-between gap-1.5 rounded-md text-sm p-0.5 bg-custom-background-80/60 relative overflow-auto", + "flex w-full min-w-fit items-center justify-start gap-1.5 whitespace-nowrap rounded-md text-sm p-0.5 bg-custom-background-80/60 relative z-0 overflow-x-auto",Also applies to: 36-37
46-47: Optional: add an equal-width mode instead of hardcoding full width.If you want equal-width tabs later, make it opt-in via a prop so content-sized tabs remain the default.
- React.ComponentProps<typeof TabsPrimitive.Tab> & { size?: "sm" | "md" | "lg" } ->(({ className, size = "md", ...props }, ref) => ( + React.ComponentProps<typeof TabsPrimitive.Tab> & { size?: "sm" | "md" | "lg"; fullWidth?: boolean } +>(({ className, size = "md", fullWidth = false, ...props }, ref) => ( <TabsPrimitive.Tab data-slot="tabs-trigger" className={cn( - "flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus:outline-none cursor-pointer transition-all duration-200 ease-in-out rounded", + "flex items-center justify-center p-1 min-w-fit shrink-0 font-medium outline-none focus:outline-none cursor-pointer transition-all duration-200 ease-in-out rounded relative z-[1]", + fullWidth ? "flex-1 basis-0" : undefined, ...Also applies to: 51-61
81-82: Verify the CSS variables powering the indicator are set by the primitives.The indicator relies on
--active-tab-widthand--active-tab-left. Please confirm these are authored by @base-ui-components Tabs.List/Root. If not, the indicator will render at 0 size/position.I can help wire these via an effect that reads the active Tab’s client rect and updates CSS variables on the list, or switch to the primitives’ built-in indicator if available.
44-65: Size prop adjusts font size only; consider padding/indicator height sync.Today,
sizemaps to text classes, but the indicator has a hardcodedh-6. Consider tying padding and the indicator height tosizefor visual balance.I can provide a small token map (sm|md|lg -> text class, px/py, indicator height) if helpful.
Also applies to: 56-59
1-96: Add basic stories/tests to lock behavior.
- Storybook: horizontal vs vertical, scroll overflow, disabled tabs, long labels, equal-width variant (if added).
- Tests: selected state styling (aria-selected), keyboard navigation (Left/Right, Home/End), focus-visible ring presence.
I can scaffold a story and a few RTL tests if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/propel/src/tabs/tabs.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/propel/src/tabs/tabs.tsx (2)
1-2: PR description/library mismatch: using Base UI primitives, not Base Web (Uber baseui).The import is from “@base-ui-components/react/tabs”, which is MUI’s Base UI primitives. The PR title/description says “Base Web (baseui)”. Please align wording and expectations (and any dependency notes) to avoid confusion for reviewers and consumers.
96-96: Named sub-exports alongside the compound — LGTM.Providing both the compound (
Tabs.*) and named exports (TabsList, etc.) gives flexibility to consumers. No issues here.
JayashTripathy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
packages/propel/.eslintrc.js (2)
4-4: Scope Storybook lint rules to stories/config files to avoid false positives and perf overhead.Move the Storybook extend into an overrides block so it only targets stories and .storybook/** files.
module.exports = { root: true, - extends: ["@plane/eslint-config/library.js", "plugin:storybook/recommended"], + extends: ["@plane/eslint-config/library.js"], parser: "@typescript-eslint/parser", + overrides: [ + { + files: ["**/*.stories.@(ts|tsx|js|jsx)", ".storybook/**/*.@(ts|tsx|js|jsx)"], + extends: ["plugin:storybook/recommended"], + }, + ], rules: {
4-4: Eslint-plugin-storybook Verified
Theplugin:storybook/recommendedextend is satisfied—eslint-plugin-storybookis installed at ^9.1.2 in packages/propel/package.json, so ESLint will load the Storybook rules without error.• Location: packages/propel/package.json →
eslint-plugin-storybook@^9.1.2• Optional: If you want to apply Storybook–specific linting only to
.stories.*files, consider moving the"plugin:storybook/recommended"entry into anoverridesblock in packages/propel/.eslintrc.js.turbo.json (1)
28-31: Tighten cache keys by adding inputs relevant to Storybook.This prevents stale caches when only stories/config change.
"tasks": { "build": { "dependsOn": ["^build"], "outputs": [".next/**", "dist/**"] }, "build-storybook": { "dependsOn": ["^build"], - "outputs": ["storybook-static/**"] + "outputs": ["storybook-static/**"], + "inputs": [ + "$TURBO_DEFAULT$", + "**/.storybook/**", + "**/*.stories.@(js|jsx|ts|tsx|mdx)", + "../../packages/tailwind-config/global.css" + ] },packages/propel/.storybook/main.ts (2)
14-18: Add recommended addons (essentials, a11y) to unlock controls/docs/accessibility checks.Running without addons is fine, but you’ll miss core functionality.
const config: StorybookConfig = { stories: ["../src/**/*.stories.@(ts|tsx)"], - addons: [], + addons: [ + getAbsolutePath("@storybook/addon-essentials"), + getAbsolutePath("@storybook/addon-a11y") + ], framework: { name: getAbsolutePath("@storybook/react-vite"), options: {}, }, };
9-11: Confirm require.resolve is usable with your TS/ESM settings.In some TS ESM setups, require is not typed/available. If you hit type/runtime issues, initialize require via createRequire.
-import { join, dirname } from "path"; +import { join, dirname } from "path"; +import { createRequire } from "module"; + +const require = createRequire(import.meta.url);packages/propel/src/tabs/tabs.stories.tsx (1)
36-66: Optionally add Indicator and Content panes to Sizes for visual completeness.Keeps parity with Basic and helps QA verify all sizes with the active underline/indicator.
<Tabs defaultValue="overview"> <Tabs.List> <Tabs.Trigger value="overview" size={size}> Overview </Tabs.Trigger> <Tabs.Trigger value="settings" size={size}> Settings </Tabs.Trigger> + <Tabs.Indicator /> </Tabs.List> + <Tabs.Content value="overview" className="p-4">Overview content</Tabs.Content> + <Tabs.Content value="settings" className="p-4">Settings content</Tabs.Content> </Tabs>packages/tailwind-config/global.css (1)
475-517: Respect reduced motion for the spinner animation.Add prefers-reduced-motion guard to reduce/disable animation for users who opt out.
-@keyframes fade { +@media (prefers-reduced-motion: reduce) { + div.web-view-spinner div { animation: none !important; } +} +@keyframes fade { from { opacity: 1; } to { opacity: 0.25; } } @-webkit-keyframes fade { from { opacity: 1; } to { opacity: 0.25; } }packages/propel/src/utils/index.ts (1)
1-1: LGTM: simple and clear barrel exportThe aggregator is fine. If you want a more explicit public API surface (to avoid surprise re-exports later), consider named re-exports.
Optional tweak:
-export * from "./classname"; +export { cn } from "./classname";packages/propel/src/utils/classname.tsx (2)
3-3: Spread the args into clsx (or add tailwind-merge) for the conventional cn helper
- Minimal:
clsx(...inputs)is the conventional form and slightly clearer.- Optional improvement: combine
clsxwithtailwind-mergeto resolve conflicting Tailwind utilities (common in design systems).Minimal change:
-export const cn = (...inputs: ClassValue[]) => clsx(inputs); +export const cn = (...inputs: ClassValue[]) => clsx(...inputs);Upgrade with tailwind-merge:
-import { clsx, type ClassValue } from "clsx"; +import { clsx, type ClassValue } from "clsx"; +import { twMerge } from "tailwind-merge"; -export const cn = (...inputs: ClassValue[]) => clsx(inputs); +export const cn = (...inputs: ClassValue[]) => twMerge(clsx(...inputs));
1-3: Suggestion: Rename util file to.tsand verify merge dependencyThe
packages/propel/src/utils/classname.tsxfile does not use any JSX syntax, so you can safely rename it toclassname.tsto avoid unnecessary TSX compilation overhead and keep utility modules consistent.Dependency checks (confirmed via your script):
clsxis present at version^2.1.1.tailwind-mergeis currently missing. If you decide to adopt the optionaltailwind-mergevariant, please add it to your dependencies.No blockers here—these are non-mandatory, optional refinements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.gitignore(1 hunks)packages/propel/.eslintrc.js(1 hunks)packages/propel/.storybook/main.ts(1 hunks)packages/propel/.storybook/preview.ts(1 hunks)packages/propel/package.json(3 hunks)packages/propel/src/tabs/tabs.stories.tsx(1 hunks)packages/propel/src/tabs/tabs.tsx(1 hunks)packages/propel/src/utils/classname.tsx(1 hunks)packages/propel/src/utils/index.ts(1 hunks)packages/tailwind-config/global.css(1 hunks)turbo.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/propel/package.json
- packages/propel/src/tabs/tabs.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/propel/src/tabs/tabs.stories.tsx (1)
packages/propel/src/tabs/tabs.tsx (1)
Tabs(94-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
packages/propel/.eslintrc.js (1)
7-30: Import order rule looks sane and consistent with common patterns.The custom React pathGroup and alphabetize settings are reasonable and should work well across the monorepo.
turbo.json (2)
28-31: LGTM: build-storybook task is correctly modeled with cacheable outputs.Depends on upstream builds and caches storybook-static/** as expected.
28-31: build-storybook targets verifiedThe
build-storybooknpm script is present in all relevant packages and matches the Turbo task:
- packages/ui/package.json:
"build-storybook": "storybook build"- packages/propel/package.json:
"build-storybook": "storybook build"No other packages reference Storybook or require this script, so the Turbo task will run without missing targets.
packages/propel/.storybook/main.ts (1)
13-19: Config shape and story glob look good.The framework resolution via getAbsolutePath matches Storybook’s monorepo guidance.
packages/propel/src/tabs/tabs.stories.tsx (1)
16-34: Nice, focused “Basic” story.Demonstrates the compound Tabs API clearly (List/Trigger/Content/Indicator).
packages/tailwind-config/global.css (1)
340-357: Theme remapping block is consistent and clear.Good use of data-theme remaps to derive sidebar tokens from core tokens.
packages/propel/.storybook/preview.ts (1)
1-11: Preview import is correct — no change needed
- Verified that
@storybook/react-vite@^9.1.2is installed inpackages/propeland there is no@storybook/reactdependency; importingPreviewfrom"@storybook/react-vite"is therefore correct.- Optional: you can enable the standard color/date matchers so Controls auto-detect common arg types:
const preview: Preview = { parameters: { controls: { - matchers: {}, + matchers: { + color: /(background|color)$/i, + date: /Date$/i, + }, }, }, };Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/propel/.eslintrc.js (1)
3-3: Remove duplicate "plugin:storybook/recommended" in extendsThe Storybook config is listed twice; keep a single entry.
- extends: ["@plane/eslint-config/library.js", "plugin:storybook/recommended", "plugin:storybook/recommended"], + extends: ["@plane/eslint-config/library.js", "plugin:storybook/recommended"],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/propel/.eslintrc.js(1 hunks)packages/propel/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/propel/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and lint web apps
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/propel/src/tabs/tabs.tsx (2)
50-71: Remove w-full from each trigger; fix conflicting text color; add aria-selected styles; optional fullWidth prop.w-full forces every tab to container width, causing overflow; text-custom-text-100 conflicts with text-custom-text-400; missing aria-selected variants reduces compatibility. Also raise triggers above indicator.
Apply:
const TabsTrigger = React.forwardRef< React.ElementRef<typeof TabsPrimitive.Tab>, - React.ComponentProps<typeof TabsPrimitive.Tab> & { size?: "sm" | "md" | "lg" } ->(({ className, size = "md", ...props }, ref) => ( + React.ComponentProps<typeof TabsPrimitive.Tab> & { size?: "sm" | "md" | "lg"; fullWidth?: boolean } +>(({ className, size = "md", fullWidth = false, ...props }, ref) => ( <TabsPrimitive.Tab data-slot="tabs-trigger" className={cn( - "flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus:outline-none cursor-pointer transition-all duration-200 ease-in-out rounded", - "data-[selected]:bg-custom-background-100 data-[selected]:text-custom-text-100 data-[selected]:shadow-sm", - "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", + "relative z-[1] flex items-center justify-center p-1 min-w-fit font-medium outline-none focus:outline-none cursor-pointer transition-all duration-200 ease-in-out rounded", + "aria-selected:bg-custom-background-100 aria-selected:text-custom-text-100 data-[selected]:bg-custom-background-100 data-[selected]:text-custom-text-100 data-[selected]:shadow-sm", + "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", "disabled:text-custom-text-400 disabled:cursor-not-allowed", { "text-xs": size === "sm", "text-sm": size === "md", "text-base": size === "lg", + "flex-1 basis-0": fullWidth, }, className )} {...props} ref={ref} /> ));
84-93: Indicator sits behind the list due to z-[-1]; disable pointer events.Negative z-index can hide the indicator under the list background; make it non-interactive.
Apply:
<div className={cn( - "absolute left-0 top-[50%] z-[-1] h-6 w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out", + "pointer-events-none absolute left-0 top-[50%] z-0 h-6 w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out", className )} {...props} ref={ref} />If the indicator relies on CSS variables from the primitive, confirm they’re set at runtime:
#!/bin/bash # Grep stories/usages to ensure TabsIndicator is placed inside Tabs.List and variables are present rg -nP -C2 'Tabs\.Indicator|--active-tab-(left|width)' --glob 'packages/**/tabs*.stories.tsx' --glob 'packages/**/*.tsx'
🧹 Nitpick comments (1)
packages/propel/src/tabs/tabs.tsx (1)
35-48: TabsList layout/stacking: prefer justify-start and set z-0.justify-between creates odd spacing with variable-width tabs; z-0 helps with indicator stacking.
Apply:
- "flex w-full min-w-fit items-center justify-between gap-1.5 rounded-md text-sm p-0.5 bg-custom-background-80/60 relative overflow-auto", + "flex w-full min-w-fit items-center justify-start gap-1.5 rounded-md text-sm p-0.5 bg-custom-background-80/60 relative z-0 overflow-auto",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/propel/src/tabs/tabs.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/propel/src/tabs/tabs.tsx (1)
packages/propel/src/utils/classname.tsx (1)
cn(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/propel/src/tabs/tabs.tsx (5)
6-21: Typed compound API + forwardRef types look solid.Good DX: statics are typed and ref-safe.
23-33: Root wrapper is clean and forwards refs correctly.No issues.
73-83: Content wrapper is straightforward and ref-safe.Looks good.
95-100: Compound export with satisfies TabsCompound is correct.Nice touch—keeps statics typed.
102-102: Named re-exports are fine.No concerns.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/propel/src/tabs/tabs.tsx (4)
36-41: Layering: ensure list is above its background but below triggersAdd z-0 here to cooperate with the indicator (z-0) and triggers (z-[1]).
- "flex w-full min-w-fit items-center justify-between gap-1.5 rounded-md text-sm p-0.5 bg-custom-background-80/60 relative overflow-auto", + "relative z-0 flex w-full min-w-fit items-center justify-start gap-1.5 rounded-md text-sm p-0.5 bg-custom-background-80/60 overflow-x-auto overflow-y-visible whitespace-nowrap",
1-3: Fix cn import or re-export it from the utils barrelThe utils barrel may not export cn. Either import from the source file or add a re-export.
Apply in this file:
-import { cn } from "../utils"; +import { cn } from "../utils/classname";—or update packages/propel/src/utils/index.ts:
+export { cn } from "./classname";
51-61: Trigger width, color conflict, and focus styles
- w-full forces every tab to container width, causing overflow in a row.
- Base text color sets both text-custom-text-100 and text-custom-text-400; the latter wins and is confusing.
- focus:outline-none removes visible focus without a replacement (a11y).
- Add z layering to keep indicator beneath triggers.
- "flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus:outline-none cursor-pointer transition-all duration-200 ease-in-out rounded", - "data-[selected]:bg-custom-background-100 data-[selected]:text-custom-text-100 data-[selected]:shadow-sm", - "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", + "relative z-[1] flex items-center justify-center p-1 min-w-fit font-medium cursor-pointer transition-all duration-200 ease-in-out rounded", + "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", + "aria-selected:bg-custom-background-100 aria-selected:text-custom-text-100 data-[selected]:bg-custom-background-100 data-[selected]:text-custom-text-100 data-[selected]:shadow-sm", "disabled:text-custom-text-400 disabled:cursor-not-allowed",If you don’t have a design-system ring yet, at minimum don’t suppress the UA outline. Optionally add a proper focus-visible ring later.
- "relative z-[1] flex items-center justify-center p-1 min-w-fit font-medium cursor-pointer transition-all duration-200 ease-in-out rounded", + "relative z-[1] flex items-center justify-center p-1 min-w-fit font-medium cursor-pointer transition-all duration-200 ease-in-out rounded",Does @base-ui-components/react/tabs set `data-selected` and `aria-selected` on Tab triggers? If both are present, confirm Tailwind’s `aria-selected:` variant is enabled by default.
78-87: Indicator sits behind the list due to negative z-indexz-[-1] often renders behind the parent background. Raise it and make it non-interactive.
- "absolute left-0 top-[50%] z-[-1] h-6 w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out", + "pointer-events-none absolute left-0 top-1/2 z-0 h-6 w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-1/2 rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out",
🧹 Nitpick comments (2)
packages/propel/src/tabs/tabs.tsx (2)
22-27: Consider dropping h-full on the rooth-full can cause unexpected stretching inside layouts. Prefer content-height by default; allow consumers to opt into full height.
- <TabsPrimitive.Root data-slot="tabs" className={cn("flex flex-col w-full h-full", className)} {...props} ref={ref} /> + <TabsPrimitive.Root data-slot="tabs" className={cn("flex flex-col w-full", className)} {...props} ref={ref} />
22-87: Add displayName for better DevTools/Storybook diagnosticsForwardRef components show as Anonymous without displayName.
@@ const TabsRoot = React.forwardRef< ... >(({ className, ...props }, ref) => ( <TabsPrimitive.Root data-slot="tabs" className={cn("flex flex-col w-full h-full", className)} {...props} ref={ref} /> )); +TabsRoot.displayName = "Tabs"; @@ const TabsList = React.forwardRef< ... >(({ className, ...props }, ref) => ( <TabsPrimitive.List @@ /> )); +TabsList.displayName = "TabsList"; @@ const TabsTrigger = React.forwardRef< ... >(({ className, size = "md", ...props }, ref) => ( <TabsPrimitive.Tab @@ /> )); +TabsTrigger.displayName = "TabsTrigger"; @@ const TabsContent = React.forwardRef< ... >(({ className, ...props }, ref) => ( <TabsPrimitive.Panel @@ /> )); +TabsContent.displayName = "TabsContent"; @@ const TabsIndicator = React.forwardRef<HTMLDivElement, React.ComponentProps<"div">>(({ className, ...props }, ref) => ( <div @@ /> )); +TabsIndicator.displayName = "TabsIndicator";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/propel/src/tabs/tabs.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/propel/src/tabs/tabs.tsx (1)
packages/propel/src/utils/classname.tsx (1)
cn(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/propel/src/tabs/tabs.tsx (1)
5-20: Typed compound + forwardRef: solid implementationThis addresses ref forwarding and TS typing for Tabs statics. Nice.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Tabs component from a single monolithic component to a composable API using primitive-based subcomponents. The new implementation provides better flexibility and follows modern React patterns while removing localStorage integration and internal state management.
- Replaced single Tabs component with composable subcomponents (List, Trigger, Content, Indicator)
- Added size variants (sm, md, lg) for tab triggers
- Removed localStorage integration and internal state management
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/propel/src/tabs/tabs.tsx | Complete rewrite to composable API with primitive subcomponents |
| packages/propel/src/tabs/tabs.stories.tsx | New Storybook examples showing basic usage and size variants |
| packages/propel/src/tabs/list.tsx | Removed obsolete TabList component |
| packages/propel/src/tabs/index.ts | Added exports for the new tabs module |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (5)
packages/propel/src/tabs/tabs.tsx:1
- Missing displayName for the TabsRoot component. This will make debugging and React DevTools inspection more difficult.
import * as React from "react";
packages/propel/src/tabs/tabs.tsx:1
- Missing displayName for the TabsList component. This will make debugging and React DevTools inspection more difficult.
import * as React from "react";
packages/propel/src/tabs/tabs.tsx:1
- Missing displayName for the TabsTrigger component. This will make debugging and React DevTools inspection more difficult.
import * as React from "react";
packages/propel/src/tabs/tabs.tsx:1
- Missing displayName for the TabsContent component. This will make debugging and React DevTools inspection more difficult.
import * as React from "react";
packages/propel/src/tabs/tabs.tsx:1
- Missing displayName for the TabsIndicator component. This will make debugging and React DevTools inspection more difficult.
import * as React from "react";
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/propel/src/tabs/tabs.tsx (3)
29-42: Tab list layout/scrolling and stacking context.justify-between + overflow-auto can create odd gaps/vertical scrollbars; add z-0 to ensure the indicator sits above the list background. Suggested tweak:
- "flex w-full min-w-fit items-center justify-between gap-1.5 rounded-md text-sm p-0.5 bg-custom-background-80/60 relative overflow-auto", + "relative z-0 flex w-full min-w-fit items-center justify-start gap-1.5 rounded-md text-sm p-0.5 bg-custom-background-80/60 overflow-x-auto overflow-y-visible whitespace-nowrap",
44-65: Triggers: fix full-width overflow, conflicting colors, stacking, and focus visibility.
- w-full forces every tab to container width → massive overflow in a row.
- Base text sets both text-custom-text-100 and text-custom-text-400; the latter wins.
- No visible focus style (outline suppressed).
- Add z-layer so the indicator can sit below triggers.
- React.ComponentProps<typeof TabsPrimitive.Tab> & { size?: "sm" | "md" | "lg" } ->(({ className, size = "md", ...props }, ref) => ( + React.ComponentProps<typeof TabsPrimitive.Tab> & { size?: "sm" | "md" | "lg"; fullWidth?: boolean } +>(({ className, size = "md", fullWidth, ...props }, ref) => ( <TabsPrimitive.Tab data-slot="tabs-trigger" className={cn( - "flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus:outline-none cursor-pointer transition-all duration-200 ease-in-out rounded", - "data-[selected]:bg-custom-background-100 data-[selected]:text-custom-text-100 data-[selected]:shadow-sm", - "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", - "disabled:text-custom-text-400 disabled:cursor-not-allowed", + "relative z-[1] flex items-center justify-center p-1 min-w-fit font-medium cursor-pointer transition-colors duration-200 ease-in-out rounded", + "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", + "data-[selected]:bg-custom-background-100 data-[selected]:text-custom-text-100 data-[selected]:shadow-sm", + "aria-selected:bg-custom-background-100 aria-selected:text-custom-text-100", + "disabled:text-custom-text-400 disabled:cursor-not-allowed focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-custom-border-100 focus-visible:ring-offset-2 focus-visible:ring-offset-transparent", { "text-xs": size === "sm", "text-sm": size === "md", "text-base": size === "lg", + "flex-1 basis-0": !!fullWidth, }, className )} {...props} ref={ref} /> ))Note: fullWidth is optional and opt-in; keep default content-sized tabs.
78-87: Indicator rendered behind the list due to negative z-index.A child with z-[-1] is painted behind the parent’s background. Also make it non-interactive. Pair with Trigger’s relative z-layer.
- "absolute left-0 top-[50%] z-[-1] h-6 w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out", + "pointer-events-none absolute left-0 top-[50%] z-0 h-6 w-[var(--active-tab-width)] translate-x-[var(--active-tab-left)] -translate-y-[50%] rounded-sm bg-custom-background-100 shadow-sm transition-[width,transform] duration-200 ease-in-out",
🧹 Nitpick comments (1)
packages/propel/src/tabs/tabs.tsx (1)
22-27: Add displayName for better DevTools names.Optional but helpful for debugging forwardRef components.
+TabsRoot.displayName = "Tabs"; +TabsList.displayName = "Tabs.List"; +TabsTrigger.displayName = "Tabs.Trigger"; +TabsContent.displayName = "Tabs.Content"; +TabsIndicator.displayName = "Tabs.Indicator";Also applies to: 29-42, 44-65, 67-77, 78-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/propel/src/tabs/tabs.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/propel/src/tabs/tabs.tsx (1)
packages/propel/src/utils/classname.tsx (1)
cn(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/propel/src/tabs/tabs.tsx (6)
1-3: Imports look correct.The Base UI tabs import and the cn import from the classname utility resolve cleanly.
5-20: Typed compound + statics: solid.ForwardRef typing for Root/List/Trigger/Content/Indicator is correct and will give consumers autocomplete and ref safety.
22-27: Root wrapper LGTM.Good: forwards ref, spreads props, and composes className via cn.
67-77: Content wrapper LGTM.Props/ref forwarding and minimal styling are fine.
51-54: Confirm Tailwind variant semantics for data-selected.If the primitive sets data-selected="true"/"false", consider targeting the explicit value for reliability and keep aria fallback:
- "data-[selected]:bg-custom-background-100 data-[selected]:text-custom-text-100 data-[selected]:shadow-sm", + "data-[selected=true]:bg-custom-background-100 data-[selected=true]:text-custom-text-100 data-[selected=true]:shadow-sm", + "aria-selected:bg-custom-background-100 aria-selected:text-custom-text-100",Would you like me to push the change after confirming Tailwind config supports arbitrary data variants?
89-96: Exports assignment looks good.Typed Object.assign with satisfies TabsCompound is correct; named exports are convenient.
Description
This PR adds baseui Tabs component to propel with stories
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit